From 02dca888d28a4898983f0a6a2773d74bad93b801 Mon Sep 17 00:00:00 2001 From: Jim Lancaster <40281233+zoltan57@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:47:53 -0500 Subject: [PATCH] Code-review complete --- docs/reviews/2026-09-02-code-review.md | 896 +++++++++++++++++++++++++ 1 file changed, 896 insertions(+) create mode 100644 docs/reviews/2026-09-02-code-review.md diff --git a/docs/reviews/2026-09-02-code-review.md b/docs/reviews/2026-09-02-code-review.md new file mode 100644 index 0000000..d766207 --- /dev/null +++ b/docs/reviews/2026-09-02-code-review.md @@ -0,0 +1,896 @@ +# 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//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: + + ```python + # 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. + + ```python + # 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` 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: + ```python + 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 (...)` 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 `@cache`d 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 + +```python +# 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) |