From 0b48c80d8700336c0d88eff9473982903c8f7f30 Mon Sep 17 00:00:00 2001 From: Jim Lancaster <40281233+zoltan57@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:42:20 -0500 Subject: [PATCH] Update instructions - Part 2, phase 3 --- .../instructions/providers.instructions.md | 120 ++++++++++++++++++ tests/test_meta_contract_guards.py | 7 + tests/test_provider_boundaries.py | 74 +++++++++++ 3 files changed, 201 insertions(+) create mode 100644 .github/instructions/providers.instructions.md create mode 100644 tests/test_provider_boundaries.py diff --git a/.github/instructions/providers.instructions.md b/.github/instructions/providers.instructions.md new file mode 100644 index 0000000..2e60a6b --- /dev/null +++ b/.github/instructions/providers.instructions.md @@ -0,0 +1,120 @@ +--- +description: Provider adapter rules for evidence capture, secret safety, and client lifecycle. +applyTo: 'src/transcription/providers/**/*.py' +--- + +# Provider Adapters + +Primary references: + +- `docs/invariant/ai_evidence_and_provenance.md` (canonical; provider adapters own provider-boundary evidence capture) +- `docs/architecture.md` +- `docs/schema.md` + +The provider layer is where an external API becomes application data. It is also the only place +that can capture what actually crossed the wire — once a response reaches a service, the evidence +it did not preserve is gone permanently. Treat capture correctness as the primary job of this +layer and text extraction as secondary. + +## Layer Boundary + +- Adapters may depend on `transcription.config`, `transcription.providers.*`, the HTTP client, and + the provider SDK. They must not import `services`, `db`, `ui`, or `api`. +- Provider specifics — headers, model slugs, payload shapes, SDK types, error classes — stop here. + Callers receive `TranscriptionResult` and `ProviderError` subclasses only. +- Adapters raise provider/domain exceptions. They must not emit user-facing text, notifications, + or remediation wording; that translation belongs to services and UI. See + [error-handling instructions](./error-handling.instructions.md). +- Adapters do not persist. They return evidence; services decide what is written and when. + +Enforced by `tests/test_provider_boundaries.py`. + +## Contract Surface + +- Every adapter satisfies the `TranscriptionProvider` protocol in `base.py`, including + `current_request_manifest` and `current_transport_evidence`, which exist so a *failed* call still + yields evidence. +- `TranscriptionResult`, `RequestManifest`, and `TransportEvidence` are `extra="forbid"` and frozen. + Add a field to the contract rather than smuggling data through an untyped dict. +- Evidence contracts in `evidence.py` are versioned (`schema_name` + `schema_version`). A change to + the meaning or shape of a captured field requires a version bump, not a silent redefinition — + stored evidence must keep its original meaning. + +## Transport Evidence + +The rules below implement `docs/invariant/ai_evidence_and_provenance.md` §3.4-3.5. That document +wins if this file drifts from it. + +- Capture the response body **at the HTTP boundary, before SDK parsing**, so fields the SDK does + not model are not lost. `_CapturingAsyncClient` exists for this; do not replace it with a + post-parse `model_dump()` and call the result transport evidence. +- Reset per-call capture state at the start of every call. Without it, a connection failure can + attach the *previous* call's response as evidence for this one. Guarded by + `tests/test_v42_evidence.py::test_openrouter_does_not_reuse_prior_response_on_connection_failure`. +- Handle the streamed-body case (`httpx.ResponseNotRead`) rather than assuming `response.content` + is always available. +- When no response arrives — timeout, DNS, connection reset — emit + `TransportEvidence(response_received=False)`. Absence of a response is itself evidence and must + be explicit, never an empty body or a missing record. +- Preserve safe response evidence for **unsuccessful** calls too, whenever a response was received. +- Never relabel an SDK snapshot or normalized metadata as transport evidence, and never backfill + it into an execution that predates capture. + +## Secret Safety + +- Persist response headers only through `filter_safe_response_headers` and the + `SAFE_RESPONSE_HEADERS` allowlist. Allowlist, never denylist: capture-then-redact is prohibited, + because an unknown header is unsafe by default. +- Adding a header to the allowlist is a deliberate evidence decision. Confirm it carries no + credential, cookie, or session material, and state why it is needed for correlation, content + interpretation, rate-limit diagnosis, or audit. +- API keys, `Authorization`, and cookies must never appear in a manifest, evidence record, log + line, or exception message. +- The request manifest references source content by identity (digest, size, media type, page). + Do not duplicate base64 source bytes into it — `_replace_embedded_media` exists for this. + +## Execution Specification + +The manifest must let a reader reconstruct what was asked, per invariant §3.3: + +- Provider, requested model, full effective prompt text, and prompt digest. +- Every explicitly supplied parameter, and — separately — which optional parameters were + **omitted**. Omission is not the same as a null value or an assumed provider default; the + `optional_parameter_states` distinction between `omitted`, `null`, and `value` is deliberate. +- Timeout budget, retry policy, source reference, and `SoftwareContext` versions. +- Manifest digests use `canonical_json_bytes`. Do not hash a plain `json.dumps()`; key order and + separators must stay deterministic or digests become uncomparable. + +## Client Lifecycle and Async Safety + +- Reuse one pooled `AsyncClient` per adapter instance; do not construct a client per request. +- Accept an injected client so tests can drive the adapter without network access. +- Derive timeouts from `Settings` (`worker_provider_timeout_seconds`) rather than hard-coding, and + keep the client timeout aligned with the configured budget so the SDK cannot expire first and + hide the real failure. +- Implement `aclose()` and release pooled resources. An adapter that creates a client owns closing + it; one given a client must not close a caller-owned resource it did not create. +- Never block the event loop. Offload CPU-bound work (hashing large payloads, image encoding) with + `asyncio.to_thread`. +- Propagate `asyncio.CancelledError` untouched — do not convert cancellation into a provider error. + +## Failure Handling + +- Raise `ProviderAuthError` for authentication, `ProviderResponseError` for malformed or unusable + responses, and `ProviderError` otherwise. +- Always attach `request_manifest`, `transport_evidence`, and an accurate `failure_phase` to raised + errors. `failure_phase` must distinguish a received-but-failed response from a call that never + reached the provider. +- Validate responses with Pydantic rather than indexing into raw dicts. +- Invalid *optional* metadata (for example unparsable token counts) must not discard an otherwise + valid transcript. Degrade the metadata, not the result. + +## Contract Sync Rule + +If capture behavior, evidence schema, or the header allowlist changes: + +1. Update `docs/invariant/ai_evidence_and_provenance.md` only if the durable preservation contract + itself is changing — that revision is deliberate and reviewed, not incidental. +2. Update `docs/schema.md` when persisted evidence fields change. +3. Update or add tests in the same change (`tests/providers/`, `tests/test_v42_evidence.py`). +4. Bump the affected evidence `schema_version` when a field's meaning changes. diff --git a/tests/test_meta_contract_guards.py b/tests/test_meta_contract_guards.py index 9ab70f8..f79ba3b 100644 --- a/tests/test_meta_contract_guards.py +++ b/tests/test_meta_contract_guards.py @@ -18,6 +18,7 @@ ACTIVE_CONTRACT_FILES = ( ".github/instructions/services.instructions.md", ".github/instructions/ui.instructions.md", ".github/instructions/error-handling.instructions.md", + ".github/instructions/providers.instructions.md", ".github/skills/python-code-reviewer/skill.md", ".github/skills/evidence-provenance-auditor/skill.md", ".github/skills/test-effectiveness-auditor/skill.md", @@ -237,6 +238,12 @@ def test_canonical_authority_references_are_present(): "docs/error_handling.md", "docs/requirements.md", ), + ".github/instructions/providers.instructions.md": ( + "docs/invariant/ai_evidence_and_provenance.md", + "docs/schema.md", + "tests/test_provider_boundaries.py", + "SAFE_RESPONSE_HEADERS", + ), ".github/skills/python-code-reviewer/skill.md": ( "docs/*", "docs/schema.md", diff --git a/tests/test_provider_boundaries.py b/tests/test_provider_boundaries.py new file mode 100644 index 0000000..aec355f --- /dev/null +++ b/tests/test_provider_boundaries.py @@ -0,0 +1,74 @@ +"""Structural rules for the providers package. + +`.github/instructions/providers.instructions.md` requires provider specifics to stop at the +adapter boundary: adapters translate an external API into `TranscriptionResult` and +`ProviderError`, and know nothing about persistence, services, or the UI. Without this guard the +rule is only advice, and a single convenience import of a service or model would invert the +dependency direction the architecture depends on. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +PROVIDERS_DIR = Path(__file__).resolve().parents[1] / "src" / "transcription" / "providers" + +# Application packages an adapter must never reach into. `config` is intentionally absent: +# adapters read Settings for timeouts and credentials. +FORBIDDEN_PACKAGES = frozenset({"services", "db", "ui", "api", "worker", "worker_service"}) + + +def _module_paths() -> list[Path]: + return sorted(PROVIDERS_DIR.glob("*.py")) + + +def _imported_application_packages(tree: ast.Module) -> set[str]: + """Return first-level `transcription.` names imported by this module.""" + imported: set[str] = set() + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module: + parts = node.module.split(".") + if node.level == 0 and parts[0] == "transcription" and len(parts) > 1: + imported.add(parts[1]) + elif isinstance(node, ast.Import): + for alias in node.names: + parts = alias.name.split(".") + if parts[0] == "transcription" and len(parts) > 1: + imported.add(parts[1]) + return imported + + +def test_provider_modules_are_discovered(): + """Guard the guard: the rules below are meaningless if nothing is scanned.""" + assert {path.stem for path in _module_paths()} >= {"base", "evidence", "openrouter"} + + +def test_provider_modules_do_not_import_application_layers(): + """A provider adapter must not depend on services, persistence, UI, API, or the worker.""" + violations: dict[str, list[str]] = {} + for path in _module_paths(): + tree = ast.parse(path.read_text(encoding="utf-8")) + found = sorted(_imported_application_packages(tree) & FORBIDDEN_PACKAGES) + if found: + violations[path.name] = found + assert violations == {} + + +def test_response_headers_are_filtered_through_the_allowlist(): + """Header persistence must be allowlist-based, not capture-then-redact.""" + from transcription.providers.evidence import SAFE_RESPONSE_HEADERS + from transcription.providers.evidence import filter_safe_response_headers + + filtered = filter_safe_response_headers( + { + "Content-Type": "application/json", + "Authorization": "Bearer super-secret", + "Set-Cookie": "session=super-secret", + "X-Unknown-Future-Header": "unreviewed", + } + ) + + assert filtered == {"content-type": "application/json"} + assert "authorization" not in SAFE_RESPONSE_HEADERS + assert "set-cookie" not in SAFE_RESPONSE_HEADERS