transcription/docs/reviews/2026-09-02-code-review.md

# 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:

  ```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<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:
  ```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 <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 `@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) |