bbchops/transcription
1
0
Fork 0
generated from john/python-template
Code Issues 4 Pull Requests Actions Packages Projects Releases Wiki Activity
Files
065acad125cb46d98d002fe62d4844d55c50ecb7
transcription/docs/reviews/2026-09-02-code-review.md
T
Jim Lancaster d5e798825d
Quality Gate / gate (push) Successful in 2m31s
Details
Implement Rec - Phase 1
2026-09-02 15:59:56 -05:00

63 KiB
Raw Blame History

Architecture & Code Review Report

Repository Target: C:\Github\transcription\ Target Stack: Python 3.12+ | FastAPI | NiceGUI | SQLModel/SQLAlchemy | Pydantic V2 | asyncio | OpenRouter Review Date: 2026-09-02 Canonical Baseline: V6.1 (docs/index.md)

0. Verification Commands and Outcomes

All four commands were executed in this checkout before any finding was written. This report records the exact outcomes rather than assuming them.

Command Outcome
uv run pytest -q -m "not external" 410 passed, 0 failed, 0 errors (exit 0)
uv run ruff check . All checks passed!
uv run ruff format --check . 191 files already formatted
uv run ty check All checks passed!

The stated green baseline is real. No finding below is a test failure; every finding is a behavior, contract, or guard-coverage defect that the passing suite does not detect.

1. Executive Summary

  • The system's core evidence guarantees hold. ExecutionAttempt is genuinely append-only, attempt numbering is allocated with bounded conflict retry, transport evidence is captured at the HTTP boundary before SDK parsing, and header persistence uses a true allowlist. Provenance invariant families A–E and G pass.
  • Both competing atomicity invariants in services/workflows.py are real and both guards genuinely enforce them. I injected-fault-verified the tests rather than trusting the docstrings: test_pipeline_atomicity.py fails on a split final-page commit, and test_workflows_reliability.py:318 reads intermediate attempts through a separate session, so it would fail if intermediate pages stopped committing individually.
  • The most significant defect is a privacy leak that a prior review believed it had closed. The 2026-08-23 review moved root-cause text out of AppError.message into AppError.detail to keep filesystem paths away from users. That text now reaches users anyway, because the UI renders ExecutionAttempt.error_detail verbatim (HIGH-01). The leak was relocated, not closed.
  • A second, independent path leak exists in five explicit raise sites that the existing guard never covered — it tests only classify_unexpected_error (HIGH-02).
  • Provenance invariant family F (path safety) fails, and it fails inconsistently within one file: sources_page.py:443 carefully sanitizes a stored path through public_media_path_label, then sources_page.py:484 dumps raw error_detail forty lines later.
  • The orphan sweep does not do what its docstring claims. It matches definitions by bare name, so an entirely dead module passes whenever its function names collide with live ones. ui/pages/tags_page.py is the proof: 93 lines never imported by anything (MED-01/LOW-01).
  • On the three flagged open items: the V4/V6.1 doc drift is confirmed (MED-02); the .env.production coupling is real but currently correct and loud-failing, so Medium not High (MED-03); and the Tags roadmap is right — the route is genuinely not registered, so the module is dead code rather than a live retired route.
  • Two latent concurrency defects carry ordering constraints and must be fixed before the changes that would make them live (MED-04, MED-05), not after.
  • Guidance-file accuracy: the recently revised .github/instructions/* files were verified against code rather than trusted. They are accurate as written; the code is what diverges from them. The one exception is that error-handling.instructions.md states a detail rule the UI layer has never followed, which makes it an unenforced claim rather than a wrong one.

2. Executive Architecture Assessment

Verdict: architecturally sound, with a concentrated failure in the last mile of error presentation.

Domain cohesion and dependency direction are good and, unusually, mechanically enforced. test_service_boundaries.py and test_ui_boundaries.py AST-scan for violations using allowlists rather than blocklists, which is the correct choice — a newly added persistence helper cannot slip through under an unlisted name. workflows.py imports only the abstract providers types and never openrouter, so provider details genuinely stop at the adapter. Transaction ownership is explicit and well-reasoned: ServiceBase._finalize commits for service-owned sessions and flushes for caller-owned ones, which is what lets orchestration modules compose multi-aggregate writes without services importing each other.

The evidence layer is the strongest part of the system and shows real care. The distinction between transport response, SDK-parsed response, and normalized metadata is maintained in code, not just in prose — _CapturingAsyncClient exists specifically to retain the exact wire body before the SDK can discard unknown fields, and TransportEvidence(response_received=False) explicitly represents "no response was received" rather than conflating it with an empty one.

The weakness is at the boundary where internal diagnostic text becomes pixels. Every layer below the UI respects the message/detail split; the UI layer reads the internal field directly and renders it. The architecture defines the contract correctly and then has no enforcement at the one layer that violates it.

Top systemic risks:

  1. Internal diagnostic text reaches users through the evidence display path (HIGH-01). The rule is documented in three places and enforced in none of them at the UI boundary.
  2. Path-safety discipline is applied per-call-site rather than structurally (HIGH-02, HIGH-01). It is correct wherever someone remembered; there is no guard that makes forgetting fail.
  3. Guard coverage is narrower than guard docstrings claim. Two guards (test_orphan_sweep.py, test_errors.py) assert something meaningfully weaker than the invariant they are named for, which converts them into a false sense of enforcement.
  4. Worker safety currently rests on single-process sequential execution, not on configuration (MED-04, MED-05). Nothing is wrong today; two plausible future changes each make something wrong.

3. Findings by Severity

Critical Severity

None. No evidence loss, append-only violation, secret leakage, or silent-wrong-output defect was found. The candidates in this class (provider evidence mis-attribution, stale-job double processing) are latent and are reported at High/Medium with their unblocking conditions.

High Severity

[HIGH-01] Internal-only error_detail is rendered directly to users, reopening the leak the 2026-08-23 fix was meant to close

  • Location:

    • Write side: src/transcription/errors.py:99-139 (classify_unexpected_error → detail, format_error_detail → persisted text)
    • Persist: src/transcription/services/workflows.py:720 (error_detail=format_error_detail(page.error))
    • Render (Source Detail): src/transcription/ui/pages/sources_page.py:481-484
    • Render (Sources list): src/transcription/ui/pages/sources_page.py:121 → src/transcription/ui/components/table/sources.py:39,90-95 ("Error Detail" column)
    • Render (Maintenance): src/transcription/ui/pages/settings_page.py:562, written by src/transcription/services/maintenance.py:206
    • Contract violated: docs/error_handling.md:107-114; .github/instructions/error-handling.instructions.md:86; docs/invariant/error_handling.md:59; docs/invariant/ai_evidence_and_provenance.md:103

  • Reachability: Live. Concrete path, no configuration required: a page fails with any non-AppError exception → workflows.py:388 calls classify_unexpected_error(exc) → errors.py:118 sets detail=f"{type(exc).__name__}: {exc}" → format_error_detail (errors.py:135-139) emits ... | detail=OSError: [Errno 13] Permission denied: '/app/uploads/documents/<uuid>/page-1.jpg' | ... → persisted to ExecutionAttempt.error_detail → rendered verbatim at sources_page.py:484 and in the /sources table column. A SQLAlchemy OperationalError carries the database path by the same route.

  • Problem & Consequence: docs/error_handling.md:110 states detail is "Internal only" and that its only surfaces are format_error_detail (evidence) and logs; error-handling.instructions.md:86 says "Never rendered to users or serialized into an envelope." The UI reads it anyway. The consequence is not hypothetical drift — it is the precise defect the previous review's fix existed to prevent. That fix made message generic and moved the root cause to detail on the stated grounds that detail never reaches users. That premise was never true: error_detail had a UI consumer the whole time. The result is that the filesystem-path leak was relocated from the notification banner to the Source Detail card and the Sources table, while the test suite records the leak as fixed (tests/test_errors.py:56-78).

    The inconsistency is visible inside a single file: sources_page.py:443 deliberately routes a stored path through public_media_path_label (ui/components/media_urls.py:58-72), which correctly degrades an absolute path to its bare filename — and then sources_page.py:484 renders unsanitized text that may contain an absolute path.

  • Blast Radius: Enumerated by grepping every reader of .detail and error_detail:

    • errors.py:137 — format_error_detail, the only reader of AppError.detail. Must keep the root cause.
    • services/workflows.py:720 — the only writer of ExecutionAttempt.error_detail.
    • services/maintenance.py:206 — the only writer of MaintenanceRun.error_detail.
    • services/evidence.py:195 — build_evidence_export emits error_detail. Export is an operator-initiated evidence artifact; per invariant 3.7.1 it must retain it.
    • db/models.py:508-522 — Source.latest_error_detail projection, consumed only by sources_page.py:121.
    • ui/pages/sources_page.py:481-484, ui/components/table/sources.py, ui/pages/settings_page.py:562 — the three render sites.
    • Tests asserting on persisted text: tests/test_v42_evidence.py:284, tests/services/test_workflows_reliability.py (timeout detail), tests/services/test_maintenance_service.py. A fix that changes what is stored breaks these; a fix that changes what is displayed does not.

  • Recommendation — two invariants conflict here; both must be named.

    Invariant 1 (evidence): ExecutionAttempt.error_detail must retain the root cause. docs/requirements.md:30 (REQ-4-021) and docs/invariant/ai_evidence_and_provenance.md:33 require it; guarded by tests/test_v42_evidence.py::test_attempts_are_append_only_and_exported_with_integrity and tests/services/test_workflows_reliability.py.

    Invariant 2 (privacy): user-facing surfaces must not expose local filesystem details. docs/invariant/error_handling.md:59; guarded (partially) by tests/test_errors.py::test_unexpected_error_does_not_leak_filesystem_paths.

    The over-correction to avoid is stripping root-cause text out of detail or format_error_detail to make the UI safe. That is exactly the mistake documented in the reviewer skill's worked example, and it would silently destroy the provenance record this system exists to preserve while making every guard still pass.

    Fix at the render boundary, not the write boundary. Add a presentation-layer projection and route all three UI sites through it, leaving the persisted evidence untouched:

    # src/transcription/ui/components/error_presenter.py  (new)
def display_failure_detail(error_detail: str | None) -> str | None:
    """Render persisted failure detail without machine-local paths.

    `ExecutionAttempt.error_detail` is provenance and keeps the full root cause
    (docs/error_handling.md). This projection is the only thing a page may show.
    """

    It should preserve the [category], suggestion=, and error_id= segments (which are what make the display actionable) and reduce any absolute path inside detail= to its basename, mirroring public_media_path_label. The operator keeps diagnosability — required by docs/ui/pages/sources.md:43 and docs/requirements.md:59 (REQ-6-014) — without the container filesystem layout being published to the browser.

    Then decide and record which resolution was chosen: either the UI shows the sanitized projection (recommended), or docs/error_handling.md:107-114 and error-handling.instructions.md:86 are revised to state that operator-facing evidence displays may render error_detail and that the guarantee moves to "no machine-local detail ever enters detail" — which would be a much harder guarantee to keep. Do not leave the current state, where the docs claim one thing and three pages do another.

  • Effort: M

[HIGH-02] Absolute filesystem paths are embedded in user-facing AppError.message at five explicit raise sites

  • Location:

    • src/transcription/services/sources.py:856 — f"Prompt file not found: {prompt_path}"
    • src/transcription/services/sources.py:864 — f"Prompt file is empty: {prompt_path}"
    • src/transcription/services/sources.py:914 — f"Source file not found: {path}"
    • src/transcription/services/prompts.py:99 — f"Prompt directory is unavailable: {root}"
    • src/transcription/services/prompts.py:186-191 — _filesystem_error builds f"{message}: {exc}"
    • Contract violated: .github/instructions/error-handling.instructions.md:74,85; docs/invariant/error_handling.md:59

  • Reachability: Live, on an ordinary user path. sources.py:845 resolves prompt_root = runtime_settings.prompt_dir.resolve(), so prompt_path is absolute (/app/prompts/transcribe_document.md in the container). load_prompt_text is invoked by build_prompt_execution (sources.py:829-831), which runs on every document upload via services/store.py:94 and store.py:162. The resulting PromptLoadError is an AppError subclass, so it flows through run_ui_action → show_error (ui/components/error_presenter.py:51-66), which renders error.message into both a ui.notify banner and a card label, and through build_error_envelope (errors.py:88-96) into API responses.

  • Problem & Consequence: error-handling.instructions.md:85 requires message to "Stay generic. Never embed exception text, provider payloads, or filesystem paths." These five sites embed exactly that. prompts.py:186-191 violates the rule in both directions at once: it puts {exc} — an OSError whose str() includes the offending filename — into message, and it sets no detail=, so the internal field that is supposed to carry the root cause is empty while the user-facing field carries all of it.

    This is not a new regression; it is coverage that the existing guard never had. tests/test_errors.py:56-78 verifies only that classify_unexpected_error — the catch-all path — does not leak. Every deliberate raise SomeError(f"... {path}") in the codebase is outside its scope, so the suite reports the invariant as enforced while five live sites violate it.

  • Blast Radius: Verified by grepping all consumers of these exception types. PromptLoadError/PromptStoreError/TranscriptionError messages are consumed by: ui/components/error_presenter.py:55,63 (render), errors.py:92 (API envelope), errors.py:135 (format_error_detail → evidence). Because the recommended change adds a detail and shortens message, format_error_detail output still contains the path — so evidence value is preserved, not reduced. Tests asserting on these messages: tests/test_prompts.py, tests/services/test_prompt_store.py, tests/services/test_transcription_service.py. These assert on message prefixes ("Prompt file not found"), not on the interpolated path, and were checked to survive the change — but re-run them, since prompts.py:186 currently produces a message whose suffix some assertion could depend on.

  • Recommendation: Apply the pattern errors.py:113-119 already establishes — generic message, root cause on detail, raise ... from exc. Use path.name when a filename is genuinely useful to the user.

    # sources.py:855 — before
raise PromptLoadError(f"Prompt file not found: {prompt_path}", ...)
# after
raise PromptLoadError(
    f"Prompt file not found: {prompt_path.name}",
    category=ErrorCategory.INFRA_PERSISTENT,
    suggestion="Verify PROMPT_DIR and prompt file configuration, then retry.",
    detail=f"Prompt file missing at {prompt_path}",
)

# prompts.py:186 — before
return PromptStoreError(f"{message}: {exc}", category=..., suggestion=...)
# after
return PromptStoreError(
    message,
    category=ErrorCategory.INFRA_PERSISTENT,
    suggestion="Check prompt directory permissions and available disk space, then retry.",
    detail=f"{type(exc).__name__}: {exc}",
)

    Then widen the guard so this class cannot recur — see MED-07. Note the dependency: HIGH-02 and HIGH-01 must be fixed together, because moving the path from message to detail while the UI still renders error_detail relocates the leak instead of closing it. That is the same mistake that produced HIGH-01.

  • Effort: S (fix) / M (with the guard)

Medium Severity

[MED-01] The orphan sweep matches by bare name and therefore cannot detect a dead module

  • Location: tests/test_orphan_sweep.py:101-163 (_public_definitions, _orphans)

  • Reachability: Live — the guard is running now and reporting a clean sweep that is not clean.

  • Problem & Consequence: _public_definitions() keys definitions by bare name (definitions[node.name], line 111) and _orphans() marks a definition referenced if that bare name appears anywhere in src/, tests/, or tools/ (lines 157-162). Two different modules that define the same public name are therefore indistinguishable, and neither can ever be reported as an orphan.

    src/transcription/ui/pages/tags_page.py demonstrates the consequence. Its only public definition is register_page (line 18). Seven live page modules define a function of the same name and ui/__init__.py:37-43 calls all seven — so register_page is heavily referenced and tags_page.register_page is scored as reachable. In fact nothing imports tags_page at all (verified: the only repo-wide references to the module are the file itself and tests/ui/test_tags_page.py, which merely asserts the route 404s). 93 lines of code, including a lazy-load-unsafe relationship traversal at tags_page.py:71-74, sit outside the sweep's reach.

    The sweep also never asks whether a module is imported, only whether its definitions' names appear somewhere — so this is a structural gap, not a one-off miss.

  • Blast Radius: tests/test_orphan_sweep.py only; KNOWN_ORPHANS entries are keyed by the same bare/dotted names and would need re-keying if qualification is added. Expect the stricter sweep to surface additional true orphans on first run — triage them into KNOWN_ORPHANS with rationales rather than weakening the check.

  • Recommendation: Qualify definitions by module (f"{module_path}:{name}") and add a separate, cheap module-reachability pass: a module under src/transcription/ is reachable if any other module imports it, or it is a declared entrypoint (app.py, __main__.py, worker_service.py). Report unreachable modules as orphans in their own right. Also fix test_public_definitions_are_discovered (line 169), whose >= 420 snapshot threshold is a weak assertion that drifts upward silently — the 2026-08-23 review already flagged the same pattern at the then-current >= 200 and it was raised rather than replaced.

  • Effort: M

[MED-02] Canonical invariant document declares a V4 baseline while the canonical baseline is V6.1

  • Location: docs/invariant/ai_evidence_and_provenance.md:130

  • Reachability: Live (documentation), no runtime impact.

  • Problem & Consequence: Section 6.1 reads "Canonical V4 architecture, schema, requirements, and error-policy documents define how current behavior satisfies this invariant." docs/index.md:1,29-32 establishes V6.1 as the baseline and states that every canonical document asserts the same baseline. This is the ownership clause of the invariant that governs the entire evidence model — the clause that tells a reader which documents are authoritative — and it points at a superseded generation. A reader following it lands on stale authority precisely when resolving an evidence question, which is the highest-stakes case.

    A baseline-currency guard does exist — tests/test_meta_contract_guards.py::test_canonical_docs_declare_one_consistent_baseline (lines 89-112) — and docs/invariant/ai_evidence_and_provenance.md is not in BASELINE_SCAN_EXCLUSIONS (lines 56-64), so the file is scanned. The claim escapes for two independent reasons, either of which alone would be sufficient:

    1. _CURRENT_VERSION_CLAIM (line 67) matches only the words current or active before a version. This line says "Canonical V4", a third phrasing the pattern does not know.
    2. Both patterns require V(\d+\.\d+) — a mandatory minor version. The bare token V4 cannot match either regex under any phrasing.

    The guard is therefore not absent but phrase-shaped: it enforces currency only for the two sentence forms someone thought of, against version strings that carry a minor. That is a weaker property than its docstring implies ("Every canonical doc that names the current baseline must name the same one").

  • Blast Radius: Documentation only; no code reads this string. Widening the guard's patterns will re-scan all canonical docs — expect it to surface further stale mentions on first run (docs/architecture.md, docs/schema.md, docs/requirements.md, and docs/error_handling.md each contain 2-3 version tokens), which should be triaged rather than excluded.

  • Recommendation: Two parts, and the second matters more than the first.

    1. Change "Canonical V4" to "Canonical V6.1" at line 130.
    2. Fix the guard's shape rather than adding a third phrase to the list. Accept an optional minor (V(\d+)(?:\.(\d+))?) and invert the matching: flag every V<n> token in a scanned canonical doc that is not the declared baseline, rather than only those preceded by an approved adjective. Phrase-list matching fails open — each new phrasing silently reopens the hole — whereas token matching fails closed and forces an explicit exclusion.

    See §8.1 for the alternative the maintainer is considering: dropping version labels from canonical docs entirely, which removes the failure mode instead of guarding it.

  • Effort: S

[MED-03] Settings resolve .env.production relative to the process working directory, and the isolation fix exists only in the test harness

  • Location: src/transcription/config.py:66-75 (env_file=".env.production"); workaround at tests/conftest.py:27-50; guarded by tests/test_config_isolation.py; depended on by .github/workflows/quality-gate.yml and docker-compose.production.yml

  • Reachability: Live but currently correct. I verified the production path rather than assuming it: Dockerfile sets WORKDIR /app in the runtime stage, and docker-compose.production.yml mounts ./.env.production to /app/.env.production for both the app and worker services, so the relative path resolves correctly today.

  • Problem & Consequence: Correct configuration loading depends on an implicit, undocumented contract between config.py and the process working directory. Nothing in config.py states it, and nothing tests it. The failure mode is not silent — openrouter_api_key is required with no default, so a wrong cwd produces a ValidationError at startup rather than a partially configured process — which is why this is Medium rather than High.

    The more telling symptom is what the coupling forced on the test harness. conftest.py:45-50 cannot escape it by passing an argument; it must mutate the Pydantic class-level model_config dict at runtime and restore it in a finally. That is a global, order-sensitive side effect adopted because the module offers no seam. It also silently repairs a second consumer: ui/runtime_settings_store.py:402 reads the same Settings.model_config["env_file"] to decide where the Settings page writes. Two subsystems are coupled through a mutable class attribute.

  • Blast Radius: Every Settings construction. Consumers of model_config["env_file"]: ui/runtime_settings_store.py:402 (write-target resolution, contract documented at docs/ui/pages/settings.md:27) and tests/conftest.py:45-50. A change must preserve the documented three-step resolution order — explicit override, RUNTIME_SETTINGS_ENV_FILE, then the configured default — or docs/ui/pages/settings.md:27 becomes wrong.

  • Recommendation: Introduce one explicit resolution function that both Settings construction and runtime_settings_store call, honoring an ENV_FILE environment variable and falling back to a path anchored to a known root rather than to os.getcwd(). Tests then pass a path instead of mutating class state, and tests/test_config_isolation.py can assert against the seam rather than against the monkeypatch. If instead the cwd contract is accepted as deliberate, document it in config.py and in docs/production-runbook.md and add a guard asserting WORKDIR/cwd alignment — an implicit contract with a container image is exactly the kind of rule the invariant routing table exists to place.

  • Effort: M

[MED-04] Stale-job reclaim threshold is not derived from maximum job duration; safety currently comes from single-process sequencing

  • Location: src/transcription/config.py:116-117 (worker_provider_timeout_seconds=30.0, worker_stale_job_seconds=30.0); sweep at src/transcription/worker.py:222-228; reclaim at src/transcription/services/jobs.py:242-268

  • Reachability: Latent. Unblocked by either of: (a) running more than one worker replica (adding deploy.replicas > 1 to the worker service in docker-compose.production.yml), or (b) setting RUN_EMBEDDED_WORKER=true on the app service while the standalone worker container is also running. It is safe today only because docker-compose.production.yml sets RUN_EMBEDDED_WORKER: "false" on app and defines exactly one worker, and because within a single loop run_worker_loop awaits process_next_queued_job to completion before returning to the stale sweep — so the sweep can never observe a job that this same process is actively working.

  • Problem & Consequence: The stale threshold (30s) equals the per-page provider timeout (30s), leaving zero margin even for a single-page job. A multi-page document is legitimately PROCESSING for up to N × 30s. Job.date_updated carries an onupdate (db/models.py:372-375), but between the initial claim and the terminal write the only touch is sources.py:522-523 reassigning job.provider/job.model to values they usually already hold, which SQLAlchemy resolves to no net change and therefore no UPDATE. I did not empirically confirm the no-UPDATE behavior, so treat that specific step as unverified — but the finding does not depend on it, because even a per-page refresh leaves only a 30s margin against a 30s timeout.

    With a second concurrent worker, the sweep would requeue a job that is mid-provider-call. Both workers then process the same job, producing duplicate ExecutionAttempt rows for the same logical work and racing terminal status writes. Append-only history would be preserved but no longer faithful: the evidence would show attempts that do not correspond to distinct application decisions.

    This is worth flagging because jobs.py:191-197 explicitly implements and documents SKIP LOCKED row locking "so concurrent workers never contend for the same job." The claim path is built for multi-worker operation; the reclaim path is not. A reader who trusts the claim docstring would reasonably scale the worker.

  • Blast Radius: requeue_stale_processing_jobs has one production caller (worker.py:226) and tests in tests/test_worker.py and tests/services/test_job_service.py. Changing the default affects tests/test_config.py declared-defaults assertions — check those before editing the default.

  • Recommendation: Fix before adding a second worker replica, not after. Two parts: (1) Make the threshold a function of the real bound rather than a coincidental peer of the page timeout — at minimum default worker_stale_job_seconds to a multiple of worker_provider_timeout_seconds with headroom, and add a model validator rejecting a stale threshold at or below the provider timeout. (2) Preferably make reclaim heartbeat-based: have _persist_page_outcome bump Job.date_updated explicitly so liveness reflects progress rather than elapsed time since claim. Add a guard asserting a multi-page job in flight is not reclaimed by a concurrently-invoked sweep.

  • Effort: M

[MED-05] Provider evidence capture is per-instance mutable state, making the adapter non-reentrant by contract

  • Location: src/transcription/providers/openrouter.py:197-199, 264-267, 274-275, 297-298, 397-412; _CapturingAsyncClient.last_response/last_body at openrouter.py:66-94; contract at src/transcription/providers/base.py:110-118 (current_request_manifest, current_transport_evidence)

  • Reachability: Latent. Unblocked by any concurrent transcribe() on a single adapter instance — most plausibly by processing a job's pages in parallel (workflows.py:274 is currently a sequential for loop) or by any second consumer sharing one SourceService.provider. Verified safe today: workflows.py:272 resolves one provider for the loop and awaits each page; the worker's ServiceBundle (worker.py:206) is distinct from app.state.services (app.py:43), so the UI cannot share the worker's adapter instance, and the UI only enqueues jobs (ui/pages/jobs_page.py:186-208).

  • Problem & Consequence: The TranscriptionProvider protocol defines evidence retrieval as "the most recent call" state read after the fact. workflows.py:369-370 relies on this on the timeout path, reading provider.current_request_manifest / current_transport_evidence when no result object exists. Under concurrency, page B's response overwrites _CapturingAsyncClient.last_response before page A's timeout handler reads it, and page A's ExecutionAttempt is written with page B's transport evidence.

    The consequence is evidence mis-attribution — a provenance-integrity failure, which this project's own rubric treats as its most serious class. It would also be near-undetectable after the fact: the attempt row would be well-formed, internally consistent, and wrong. The application-level design that makes this safe (sequential pages) is not expressed in the provider contract, so the constraint lives only in workflows.py's loop structure.

  • Blast Radius: Changing the protocol touches providers/base.py:102-136, providers/openrouter.py:221-231, the two read sites at workflows.py:369-370, and the fakes in tests/providers/test_openrouter.py, tests/services/test_workflows_reliability.py, and tests/test_provider_boundaries.py, all of which implement or assert the current property-based contract.

  • Recommendation: Fix before introducing any intra-job page concurrency. The durable fix is to stop returning evidence through instance state: attach request_manifest and transport_evidence to the raised exception on every failure path — which ProviderError already supports (providers/base.py:18-29) and which the timeout path cannot currently use because asyncio.wait_for raises TimeoutError from outside the adapter. A narrower option is to have transcribe() accept a caller-owned capture sink so evidence is scoped to the call rather than to the adapter. As an immediate, near-zero-cost step, document the non-reentrancy on the protocol in providers/base.py so the constraint is visible where it is depended upon.

  • Effort: M

[MED-06] Provider error bodies reach user-facing text while three provider failure paths persist no detail

  • Location: src/transcription/services/sources.py:923-947 (handle_transcription_errors); message construction at src/transcription/providers/openrouter.py:414-431 (_transport_error_message)

  • Reachability: Live for the message half (any provider failure during a UI-initiated transcription surfaces through show_error).

  • Problem & Consequence: Two mirrored halves of the same rule are broken in one function.

    • sources.py:943 builds f"Provider transcription failed: {exc}", and exc is a ProviderError whose message may embed up to 500 characters of the provider's error body (openrouter.py:430). That is a provider payload in message, which error-handling.instructions.md:85 explicitly forbids.
    • None of the three handlers (lines 929, 935, 942) passes detail=. Per error-handling.instructions.md:89-92, omitting it degrades the provenance record.

    I checked whether the provenance half is actually harmful before reporting it, and it is substantially mitigated: workflows.py:391 calls _find_provider_error, which walks __cause__/__context__ (workflows.py:806-813) to recover the original ProviderError and persists its transport_evidence — status code, safe headers, and the exact response body — onto the attempt. So the root cause is preserved in transport evidence even though error_detail is thin. This is why the finding is Medium rather than High. The residual cost is that the human-readable failure summary is uninformative for the two paths (ProviderAuthError, ProviderResponseError) whose messages are entirely generic.

  • Blast Radius: handle_transcription_errors is used on the transcription path in sources.py; TranscriptionError.message is consumed by error_presenter.show_error, build_error_envelope, and format_error_detail. Assertions on these messages live in tests/services/test_transcription_service.py and tests/providers/test_openrouter.py.

  • Recommendation: Move the interpolated provider text from message to detail on all three handlers, keeping the generic message the other two already use:

    except ProviderError as exc:
    raise TranscriptionError(
        "Provider transcription failed",
        category=ErrorCategory.EXTERNAL_PROVIDER,
        suggestion="Retry the transcription from jobs. If repeated, check provider availability.",
        retriable=True,
        detail=f"{type(exc).__name__}: {exc}",
    ) from exc

    Apply the same detail= addition to the ProviderAuthError and ProviderResponseError handlers. Note the interaction with HIGH-01: until the render boundary is sanitized, moving text into detail still reaches users through the error_detail display. Sequence accordingly.

  • Effort: S

[MED-07] No deterministic guard covers the message/detail split at explicit raise sites

  • Location: tests/test_errors.py:56-78; rule at .github/instructions/error-handling.instructions.md:78-98; canonical statement at docs/error_handling.md:102-115

  • Reachability: Live — this coverage gap is what allowed HIGH-02 and MED-06 to exist in a fully green suite.

  • Problem & Consequence: docs/error_handling.md:115 names tests/test_errors.py::test_unexpected_error_does_not_leak_filesystem_paths as the enforcement for the message/detail split. That test exercises exactly one function, classify_unexpected_error. Every direct raise SomeAppError(...) in src/ — roughly 50 sites by grep — is unenforced. The documentation therefore overstates the enforcement, which is worse than having no guard: a contributor reading error_handling.md:115 reasonably concludes the rule is mechanically protected.

    Per the reviewer skill, where a check is unenforced, recommending the deterministic test is itself a finding.

  • Blast Radius: Tests only.

  • Recommendation: Add an AST guard, tests/test_error_message_safety.py, that scans src/ for raise <AppError subclass>(...) and fails when the first positional argument is an f-string containing a formatted value whose name matches a path-like or exception-like identifier (path, _path, root, dir, exc, err, e). Model it on the existing AST guards, which are the established pattern here (test_ui_boundaries.py, test_service_boundaries.py, test_orphan_sweep.py). Pair it with a second guard asserting that no UI module reads error_detail without routing through the sanitizing projection from HIGH-01 — that one closes the render side, which is where the real leak is.

  • Effort: M

Low Severity

[LOW-01] ui/pages/tags_page.py is dead code; the V6.1 roadmap is correct

  • Location: src/transcription/ui/pages/tags_page.py (93 lines); registration list at src/transcription/ui/__init__.py:37-43
  • Reachability: Not reachable. This resolves the flagged open item: the route is genuinely not registered. register_pages calls seven page registrars and tags_page is not among them; nothing anywhere imports the module. docs/roadmap_plan.md:47 ("Retire the Tags page") is accurate, and tests/ui/test_tags_page.py correctly asserts /ui/tags returns 404 — though it passes trivially, since an unimported module cannot register anything.
  • Problem & Consequence: No runtime risk; purely stranded code. It is worth noting that if it were ever re-registered, tags_page.py:71-74 traverses document.document_tags and link.tag_ref inside a page render, and those relationships are configured lazy="raise" (docs/architecture.md:200-203) — so re-enabling this module without adding eager loads to list_documents would raise on first render.
  • Recommendation: Delete src/transcription/ui/pages/tags_page.py. Retain tests/ui/test_tags_page.py as the retirement guard. Fixing MED-01 first would make this finding reproducible by the suite rather than by manual inspection.
  • Effort: S

[LOW-02] benchmarking.py ships in the runtime package but is referenced only by tests

  • Location: src/transcription/benchmarking.py (69 lines); sole consumers tests/test_v42_evidence.py:15-16 (EditorialAssessment, score_transcription)
  • Reachability: Live as importable API; never invoked by application code.
  • Problem & Consequence: No defect. It supports the model-evaluation policy in docs/invariant/ai_evidence_and_provenance.md:113-126, which is legitimate, but it currently has no production caller and no tooling entrypoint, so it is indistinguishable from drift.
  • Recommendation: Either move it under tools/ alongside the other operator utilities, or add a KNOWN_ORPHANS-style rationale recording that it is retained as the evaluation-policy implementation. Do not silently keep it unlabeled.
  • Effort: S

[LOW-03] Two overlapping prompt error types split across modules

  • Location: src/transcription/services/errors.py:15-16 (PromptLoadError) and src/transcription/services/prompts.py:19 (PromptStoreError)
  • Reachability: Live; no misbehavior observed.
  • Problem & Consequence: services/errors.py:1-8 documents itself as the neutral home for exceptions raised by more than one service, precisely so a caller's except clause does not change when an operation moves. PromptStoreError is defined outside that module and covers an overlapping domain (prompt file access), so a caller wanting to handle "any prompt failure" must import from two modules and know which is which. sources.py:855 raises PromptLoadError for a missing prompt file while prompts.py:132 raises PromptStoreError for the same condition reached through the Settings page.
  • Recommendation: Move PromptStoreError into services/errors.py next to PromptLoadError, or make one a subclass of the other so a single except covers prompt failures. Low urgency; do it opportunistically when HIGH-02 touches both files anyway.
  • Effort: S

4. Architectural Drift & Gap Analysis

Area / Component Direction Documented / Intended Rule Actual Implementation State Severity Recommended Resolution
Error presentation doc->code docs/error_handling.md:110 — detail is internal only, surfaced by format_error_detail and logs sources_page.py:484, table/sources.py:90, settings_page.py:562 render error_detail verbatim to users High Sanitizing render projection (HIGH-01); do not strip detail
User-facing messages doc->code invariant/error_handling.md:59 — no local filesystem detail in user-facing messages 5 live sites interpolate absolute paths into AppError.message High Generic message, path on detail (HIGH-02)
Evidence invariant ownership doc->doc docs/index.md:1 — baseline is V6.1 invariant/ai_evidence_and_provenance.md:130 names "Canonical V4"; the currency guard scans the file but its regexes match neither the phrasing nor a minor-less V4 Medium Update text; make the guard token-based, or drop version labels entirely (MED-02, §8.1)
Enforcement claim doc->code docs/error_handling.md:115 — split "Enforced by tests/test_errors.py::…" That test covers only classify_unexpected_error; explicit raises unguarded Medium Add AST guard (MED-07)
Orphan sweep doc->code test_orphan_sweep.py:1-13 — sweep is "deterministic" and "conservative" Bare-name matching; cannot see a dead module (tags_page.py) Medium Qualify by module + module-reachability pass (MED-01)
Worker scaling code->doc jobs.py:191-197 — SKIP LOCKED so "concurrent workers never contend" Claim path is multi-worker-safe; stale-reclaim path is not Medium Derive stale threshold from job duration; document single-worker constraint until fixed (MED-04)
Provider adapter contract code->doc providers/base.py:110-118 — evidence read as "most recent call" state Contract is silently non-reentrant; safety lives in workflows.py's sequential loop Medium Scope evidence to the call; document non-reentrancy (MED-05)
Settings env file code->doc config.py:66-75 — env_file=".env.production" Correctness depends on an undocumented cwd contract with Dockerfile WORKDIR /app Medium Explicit resolver seam, or document + guard the contract (MED-03)
Tags page (no drift) roadmap_plan.md:47 — Tags page retired Route genuinely unregistered; module is stranded code Low Delete the module (LOW-01)

5. Invariant Inventory & Routing Recommendations

Invariant / Constraint Current Location Recommended Target Layer Rationale
detail/error_detail never rendered to users docs + instructions Deterministic test + sanitizing projection Stated in three documents and violated in three files; prose has demonstrably failed to hold it
message carries no paths or exception text instructions; partial test Deterministic test (AST, all raise sites) Existing guard covers one function; the gap produced HIGH-02
ExecutionAttempt.error_detail retains root cause docs + test_v42_evidence.py Keep in tests — already correct Counterweight to the above; must be named in any fix so it is not over-corrected
Intermediate pages commit individually workflows.py docstring + test_workflows_reliability.py:318 Keep in tests — verified genuine Cross-session read makes it a real durability assertion
Final page atomic with terminal status services.instructions.md + test_pipeline_atomicity.py Keep in tests — verified genuine Fault injection makes a split commit fail
Canonical baseline version consistency docs/index.md + test_meta_contract_guards.py:89 Repair existing test, or remove the labels Guard exists but matches by approved phrase and requires a minor version, so it fails open on new phrasings (MED-02)
Module-level reachability / dead modules test_orphan_sweep.py (ineffective) Deterministic test (repair existing) Guard exists but cannot detect the case (MED-01)
Stale threshold > max job duration (unenforced) Config validator + test Currently a coincidence of two equal defaults (MED-04)
Provider adapter non-reentrancy (unenforced, implicit) Instructions + protocol docstring A design constraint callers must know before adding concurrency (MED-05)
Env-file resolution independent of cwd tests/conftest.py monkeypatch Code seam + docs/production-runbook.md A test-only fix for a production coupling is misrouted enforcement (MED-03)

6. Stack-Specific Analysis

Python 3.12+. Modern and consistent. PEP 695 generics are used correctly and non-trivially (RegistryService[ModelT: RegistryEntry] in services/registry.py:58, UiActionOutcome[T], _get_or_raise[ModelT]), type statements appear in db/session.py:15,50, and X | None is used throughout. structural Protocol bounds (RegistryEntry, WorkerNotifier, TranscriptionProvider) are used to avoid type suppressions rather than to decorate. ty passes clean with no suppressions found. The two # noqa uses (workflows.py:228 PLR0915, workflows.py:383 BLE001) are both justified in context — the broad catch is a deliberate per-page containment boundary that immediately classifies and re-records.

FastAPI. Lifespan is handled via @asynccontextmanager (app.py:36), not the deprecated @app.on_event. Session factories are injected through Depends (SessionFactoryDep, db/session.py:50) rather than reached as globals from routes. api/errors.py centralizes envelope translation. One residual: get_settings is @cached and read as a module-level fallback in ~10 modules; this is acceptable given the documented restart-to-apply contract (docs/ui/pages/settings.md:28) but means the cache is process-lifetime and unclearable.

NiceGUI (pinned 3.13.0). The pin is a recorded release-stability decision and is not reported as a defect. Boundaries are enforced structurally: test_ui_boundaries.py uses an import allowlist, which is the right polarity. Blocking work is dispatched off the event loop via run_blocking (settings_page.py:818,822). The one boundary that is not enforced is presentation of internal fields (HIGH-01) — pages are prevented from touching persistence but not from rendering internal-only text.

SQLModel / SQLAlchemy. Strong. lazy="raise" on relationships forces explicit eager loading; read paths declare selectinload chains with comments explaining why each is needed (sources.py:309-316 is a good example). expire_on_commit=False (db/session.py:28) is set deliberately, which is what makes post-commit attribute access in evidence.py:159-202 safe. claim_next_queued_job (jobs.py:186-241) branches correctly on dialect — SKIP LOCKED on PostgreSQL, conditional UPDATE ... RETURNING on SQLite — rather than assuming one engine. Attempt-number allocation uses begin_nested() with bounded retry (sources.py:598-616), the right pattern for a monotonic per-parent sequence. No N+1 patterns were found in the read paths sampled.

Pydantic V2 & Settings. Fully V2; no @validator, class Config, .dict(), or parse_obj anywhere. Evidence contracts use ConfigDict(extra="forbid", frozen=True) (providers/evidence.py:47), which is exactly right for persisted provenance — an unexpected field fails loudly rather than being silently dropped. SecretStr guards the API key. The discriminated SqliteSettings | PostgresSettings union is clean. normalize_provider_models correctly runs mode="before" so the derived tuple is produced by construction rather than by mutating a frozen model — a subtlety that is easy to get wrong. Sole issue: the cwd-coupled env_file (MED-03).

Asyncio Workers. Notably careful. asyncio.shield wraps both the per-page commit and the terminal commit (workflows.py:601-614, 650-663), with the except CancelledError: await task; raise pattern that actually completes the shielded work rather than merely deferring cancellation — a detail most implementations get wrong. handle_worker_exceptions (worker.py:157-182) distinguishes retriable from non-retriable faults and stops the loop rather than spinning. _advance_job_with_containment (worker.py/workflows.py:507-540) guarantees a claimed job cannot strand in PROCESSING. worker_consumer_lifespan has a bounded shutdown with escalation to cancel(). Gaps are MED-04 and MED-05, both latent and both with stated unblocking conditions.

OpenRouter / Adapter Boundary. Encapsulation holds: test_provider_boundaries.py enforces it, and workflows.py imports only providers abstractions. _CapturingAsyncClient is a well-judged design — it captures the exact transport body before SDK parsing without altering what the SDK consumes, including the streamed case. Timeout construction (openrouter.py:200-206) correctly overrides httpx's 5s per-phase default that would otherwise silently cap the configured budget. SAFE_RESPONSE_HEADERS (providers/evidence.py:29-41) was reviewed field-by-field: all nine entries are non-secret correlation, content, or rate-limit headers, and filter_safe_response_headers is a true allowlist filter with no redaction-after-capture — this satisfies invariant 3.8.2 exactly. _replace_embedded_media correctly substitutes a source reference for base64 payloads, satisfying 3.8.3. The one structural weakness is MED-05.

Testing & Quality Tooling. 410 tests, all green, with genuinely strong contract guards (boundaries, model contract, media path safety, evidence append-only, atomicity). Marker strictness and asyncio_mode = "strict" are configured, and no unawaited-coroutine warnings appeared. Two guards, however, assert meaningfully less than their names and docstrings claim (test_orphan_sweep.py — MED-01; test_errors.py path-leak coverage — MED-07), and the >= 420 snapshot threshold at test_orphan_sweep.py:169 repeats a weak-assertion pattern the 2026-08-23 review already flagged at >= 200; it was raised rather than replaced with set-membership.

7. Duplication & Consolidation Report

Pattern / Duplication Locations Proposed Canonical Home Estimated Lines Removed
f"{type(exc).__name__}: {exc}" detail construction errors.py:118, maintenance.py:71,95,135,206, runtime_settings_store.py:388,476,554 errors.py::exception_detail(exc) ~8 (consistency > line count)
Filesystem AppError construction from OSError prompts.py:186-191, runtime_settings_store.py:384-389,472-477,550-555 errors.py::filesystem_error(message, exc, *, suggestion) ~20
Overlapping prompt error types services/errors.py:15, services/prompts.py:19 services/errors.py (LOW-03) ~5
Duplicated provider_duration_ms / processing_duration_ms max-clamp arithmetic workflows.py:341-345, 361-368, 397-404 workflows.py::_page_durations(started_at, finished_at, monotonic_started_at) ~20
_utc_now_naive defined per module workflows.py:51, jobs.py:29, db/models.py, sources.py Single helper in db/models.py, imported ~12

Proposed Canonical Abstractions

# src/transcription/errors.py
def exception_detail(exc: BaseException) -> str:
    """Internal-only root-cause text for AppError.detail. Never user-facing."""


def filesystem_error[E: AppError](error_type: type[E], message: str, exc: OSError, *, suggestion: str) -> E:
    """Build a filesystem AppError with a generic message and the path on detail."""


# src/transcription/ui/components/error_presenter.py
def display_failure_detail(error_detail: str | None) -> str | None:
    """Sanitize persisted failure detail for UI rendering (HIGH-01)."""

8. Meta-Tooling & Instruction Update Recommendations

  1. docs/invariant/ai_evidence_and_provenance.md:130 — resolve the V4 label. Two viable routes, and the maintainer has proposed the second:
    • (a) Repair the guard. Fix the text to V6.1 and make test_canonical_docs_declare_one_consistent_baseline token-based rather than phrase-based (MED-02). Keeps version labels as navigational anchors.
    • (b) Remove version labels from canonical docs. While the project has a single principal user and no released versions to support, "canonical" and "current" are the same thing, so the label carries no information a reader can act on — it only creates a second thing to keep in sync. Retain the baseline declaration in docs/index.md alone as the release marker, keep version language in docs/roadmap_plan.md and the migration/deployment docs (already excluded from the scan for exactly this reason), and replace in-body references with unversioned phrasing ("the canonical architecture, schema, requirements, and error-policy documents"). The guard then inverts: assert that no canonical doc outside the exclusion set contains a version token at all, which is a stricter and much cheaper property to hold than agreement between many labels. Requirement IDs (REQ-4-021, REQ-6-014) are stable identifiers, not currency claims, and should be left alone.
  2. docs/error_handling.md:107-115 — either add the sanitizing-projection rule for UI display of error_detail, or revise the detail "Surfaces" row to admit operator-facing evidence displays. Update the "Enforced by" line once MED-07's guard lands, since it currently overstates coverage.
  3. .github/instructions/error-handling.instructions.md — add an explicit clause under "User-Safe Messaging" stating that persisted error_detail is subject to the same no-paths rule at any render boundary. The current table (line 86) states the rule for AppError.detail and stops there, so the persisted-then-rendered path falls between the lines.
  4. .github/instructions/providers.instructions.md — record the adapter non-reentrancy constraint (MED-05); it is currently an undocumented precondition of workflows.py.
  5. .github/instructions/services.instructions.md — the two competing atomicity invariants are well described and both guards verified; no change needed. Worth adding the stale-reclaim threshold constraint (MED-04) alongside them, since it is a third worker-lifecycle rule with no documented home.
  6. tests/test_orphan_sweep.py — repair per MED-01 and replace the >= 420 threshold with set-membership assertions.
  7. New tests/test_error_message_safety.py — AST guard per MED-07, covering both the raise sites and the UI render sites.
  8. docs/production-runbook.md — document the cwd/WORKDIR contract for .env.production resolution if MED-03 is resolved by documentation rather than by a code seam.

9. Prioritized Dependency-Ordered Action Plan

Phase 1 — Blocking fixes (privacy; ordered, HIGH-01 first)

  1. HIGH-01 — add display_failure_detail and route sources_page.py:484, table/sources.py:90-95, and settings_page.py:562 through it. Do this first: it closes the render boundary, so the Phase-1.2 fix cannot relocate a leak again.
  2. HIGH-02 — move paths from message to detail at the five sites, including the prompts.py:186 double violation.
  3. MED-06 — move provider payload text to detail; add detail= to all three handle_transcription_errors handlers.

Phase 2 — Enforcement hardening (make Phase 1 permanent) 4. MED-07 — AST guard for raise-site message safety and for UI reads of error_detail. 5. MED-01 — qualify orphan definitions by module; add module-reachability; replace the snapshot threshold. 6. MED-02 — fix the V4/V6.1 text and extend the meta-contract guard to baseline-version currency.

Phase 3 — Reliability & concurrency (latent; each must precede its unblocking change) 7. MED-04 — derive worker_stale_job_seconds from worker_provider_timeout_seconds with a rejecting validator, ideally plus a progress heartbeat. Must land before any second worker replica. 8. MED-05 — scope provider evidence to the call rather than the instance. Must land before any intra-job page concurrency. Document non-reentrancy immediately as an interim step.

Phase 4 — Consolidation & refactoring 9. MED-03 — explicit env-file resolution seam shared by Settings and runtime_settings_store; remove the model_config monkeypatch from conftest.py. 10. LOW-01 — delete tags_page.py (after MED-01, so the suite reproduces the finding). 11. LOW-03 and the §7 consolidations — fold in opportunistically while Phase 1 touches these files.

Phase 5 — Non-blocking governance/documentation depth 12. LOW-02 — relocate or annotate benchmarking.py. 13. Instruction/doc updates §8.3–§8.5, §8.8.

10. Preserved Strengths

  • Append-only evidence is real, not aspirational. Every provider call produces a distinct ExecutionAttempt; no runtime path mutates a historical row. Projection writes onto Source.raw_transcription are clearly separated from history, and promote_machine_attempt (evidence.py:117-146) repoints the projection without rewriting evidence — with a docstring that explains exactly why that one write lives in a read-oriented service.
  • Transport-layer terminology is honored in code. _CapturingAsyncClient exists specifically so the stored body is the application-boundary capture rather than an SDK-parsed object, and TransportEvidence(response_received=False) explicitly represents "no response" instead of conflating it with an empty one. This is invariant 3.4/3.5 implemented rather than asserted.
  • Header allowlisting is done the hard, correct way — filter-before-store with an explicit frozenset, never capture-then-redact (providers/evidence.py:29-41,130-134).
  • Boundaries are enforced by allowlist, not blocklist. test_ui_boundaries.py:20-25 states the reasoning explicitly; it means a newly added persistence helper cannot slip through under an unlisted name.
  • The two competing atomicity invariants are both correctly implemented and both genuinely guarded, with the tests structured so that the naive over-correction fails.
  • Cancellation safety in the worker is unusually well handled — asyncio.shield plus await task on CancelledError actually completes the commit rather than merely deferring cancellation.
  • Comments explain rationale, not mechanics. workflows.py:269-271, openrouter.py:200-202, config.py:114-115, and jobs.py:191-197 each record why a non-obvious choice was made, several citing the review log entry that motivated it. This is what made verifying the atomicity and timeout invariants tractable in this review.
  • Documentation-to-code traceability is strong overall. Page contracts, schema field tables, and requirement IDs are maintained and guarded; the drift found in this review is narrow and specific rather than systemic.

Appendix A — Repo-Specific Deterministic Checks

# Check Result Evidence
1 Service boundary rule: no service-to-service imports Pass tests/test_service_boundaries.py green; AST scan, allowlist-based; workflows.py composes via ServiceBundle
2 UI boundary rule: no persistence access from pages/components Pass (structurally) tests/test_ui_boundaries.py green. Caveat: it guards data access, not presentation of internal-only fields — see HIGH-01
3 Status vocabulary conformance; no stringly-typed literals Pass tests/test_model_contract_guards.py green; enum members verified against db/models.py
4 Evidence ownership: append-only history, projections not history mutation Pass test_v42_evidence.py::test_attempts_are_append_only_and_exported_with_integrity verified non-vacuous (asserts both retained attempts and export integrity at lines 281-290)
5 Canonical authority: findings resolve against docs/* first Pass with defect test_canonical_authority_references_are_present green. The companion baseline-currency guard (test_canonical_docs_declare_one_consistent_baseline) scans the offending file but fails open on its phrasing and on minor-less version tokens — MED-02
6 Schema contract fidelity: docs/schema.md field-accurate Pass test_model_contract_guards.py + test_meta_contract_guards.py green
7 Media boundary: record-validated media, controlled URL resolver Pass test_media_path_safety.py, tests/ui/test_media_urls.py green; public_media_path_label verified path-safe
8 Eager-loading conformance vs lazy="raise" Pass Declaration-side guard green; sampled read paths declare explicit selectinload chains. Note: dead tags_page.py:71-74 would violate it if re-registered (LOW-01)
9 Cross-cutting error conformance FAIL Guards green but coverage is narrower than documented: HIGH-01, HIGH-02, MED-06, MED-07
10 Orphan/dead-code conformance FAIL Guard green but structurally unable to detect a dead module: MED-01, proven by LOW-01

Appendix B — Evidence & Provenance Auditor Families

Family Subject Result Evidence
A Attempt history append-only Pass No update/delete path to ExecutionAttempt; insert-only with begin_nested + bounded sequence retry (sources.py:556-616)
B Attempt numbering monotonic per source Pass insert_with_sequence_retry; uniqueness constraint plus retry on conflict
C Transport evidence captured at the transport boundary Pass _CapturingAsyncClient retains the exact wire body pre-SDK-parse (openrouter.py:66-94)
D Absent response distinguished from empty response Pass TransportEvidence.response_received is explicit, not inferred
E Response header persistence is allowlist-based Pass SAFE_RESPONSE_HEADERS (providers/evidence.py:29-41) — all nine entries verified non-secret; filter-before-store
F No machine-local detail on user-facing surfaces FAIL error_detail rendered verbatim at three UI sites (HIGH-01); paths in message at five sites (HIGH-02)
G Request manifest excludes embedded media payloads Pass _replace_embedded_media (openrouter.py:377-395) substitutes a source reference for base64 data
H Evidence attribution is correct under concurrency Pass today / at risk Correct in the current sequential single-worker deployment; the contract itself is non-reentrant (MED-05) and reclaim has no margin (MED-04)
Reference in New Issue View Git Blame Copy Permalink