Records the implementation plan derived from the 2026-08-23 review: per-task
acceptance criteria, the verification baseline, and environment constraints.
Lives in docs/reviews/ so it is discoverable from the repo rather than from
session state, and is indexed from docs/reviews/README.md. Non-canonical, like
everything under docs/reviews/**.
Co-authored-by: Copilot App <[email protected]>
Phase 1 of docs/reviews/2026-08-23-code-review.md.
HIGH-01: process_queued_job committed page evidence and the terminal job
status in separate transactions, so a crash between them left a transcript
persisted against a job stuck in PROCESSING that the worker never reclaims.
The final page's write is now deferred into _finalize_batch_outcome so it
shares the terminal transaction. Intermediate pages remain individually
durable, and the terminal commit is shielded against cancellation the same
way per-page writes already were.
HIGH-04: added tests/integration/test_pipeline_atomicity.py covering both
Transaction B and Transaction C. Confirmed failing against the previous
implementation before the fix.
HIGH-03: classify_unexpected_error interpolated the raw exception into
AppError.message, which the UI renders and the API serializes, leaking the
database path from OperationalError. message is now generic. Because message
also feeds format_error_detail, which writes evidence records, the root cause
is preserved on a new internal-only AppError.detail field rather than
discarded.
HIGH-02: replaced 8 hand-rolled ui.notify error calls in home_page and
people_page with error_presenter.show_error, restoring the correlation
error_id, canonical category, and suggestion. Added an AST guard to
test_ui_boundaries.py so pages cannot hand-roll error notifications again.
Docs updated per documentation-sync: the message/detail split in
docs/error_handling.md and the multi-page atomicity rule in
services.instructions.md.
Verification: ruff clean, 381 tests passing, ty unchanged at 10 known
SQLAlchemy descriptor false positives.
Co-authored-by: Copilot App <[email protected]>
Apply the highest-value typing fixes from the ty baseline pass:
- align migration row typing with SQLAlchemy RowMapping sequences
- accept refreshable callback return type in homepage gallery
- guard nullable media URL before ui.image in people photos
- guard nullable source MIME type before startswith checks
- fix tests/test_db collect() return annotation to match 4-tuple
This clears all actionable ty findings from that set and leaves only
known SQLModel/SQLAlchemy descriptor false positives.
Co-authored-by: Copilot App <[email protected]>
The pre-commit hooks declared `language: system` with bare `ruff`/`ty`
entries, but both are uv-managed dev dependencies and are not on PATH, so every
commit failed with `Executable 'ruff' not found`. Route both through
`uv run`; keep ruff blocking and make ty advisory (verbose) until its 18
whole-project diagnostics are cleared.
With the gate working, clear `ruff check .` to zero:
- 18 auto-fixes (import sorting, blank lines, `max()` simplification,
`with` merging, unused imports).
- Real defects: `SourceNavigation` annotated but never imported in
sources_page; two naive `datetime.now()` calls in migration.py now use
`datetime.now(UTC)`.
- Dead parameters removed: `source_has_photo_table` (computed, passed, never
read), `_serialize_value(key=...)`, and unused `request` on two NiceGUI
page handlers where the framework injects it optionally.
- Mechanical line-length wrapping and one `startswith` tuple collapse.
- `# noqa: PLR0915` / `# noqa: PLR1702` on five long UI/migration
functions, following the convention already used in jobs_page and
settings_page, rather than refactoring during stabilization.
Full suite green (377 tests, `-m "not external"`).
Co-authored-by: Copilot App <[email protected]>
The reviewer skill recorded four deterministic checks as unenforced or partial. Add tests so they fail the build instead of relying on a reviewer noticing.
tests/test_model_contract_guards.py:
- Status vocabulary: flags string literals compared against or assigned to status/purpose attributes, plus a narrower sweep that requires every status-valued literal in the package to be a known non-status use.
- Relationship loading: every Relationship must declare lazy='raise' except documented exceptions, and the exception set must match the Relationship Loading Contract in docs/schema.md.
- Schema fidelity: the Field-Accurate Table Contracts tables must match db/models.py on table coverage, field names, and declaration order, and the Authoritative Enumerations section must match the enum members.
tests/test_orphan_sweep.py:
- Locks the set of unreferenced public definitions. Route handlers registered by decorator are exempt, string entrypoint references count, and tests/ and tools/ count as consumers. KNOWN_ORPHANS records the four current orphans with rationale; a new one fails the build.
Each guard was mutation-tested: reverting the fix below, dropping a documented field, widening a lazy strategy, and adding a stranded function each fail their respective test.
Also fix the one violation the status guard found: sources_page.py compared attempt.status.value to the literal 'transcribed' instead of JobSourceStatus.TRANSCRIBED, which would survive an enum rename.
Co-authored-by: Copilot App <[email protected]>
Update the reviewer skill so its procedure matches how this repo actually works:
- Route review reports to docs/reviews/ and mark them non-canonical, resolving the conflict where reports landed in the same docs/ tree they resolve findings against.
- Pin verification commands to uv (uv run ruff check / ty check / pytest -m 'not external').
- Record the pytest contract: strict markers, strict asyncio mode, and the never-awaited-coroutine warning promoted to an error.
- Convert the deterministic checks to a table with an Enforced by column; three checks are unenforced and one only partial, which are now findings by construction.
- Add a consequence-based severity rubric and a Direction column for bidirectional drift.
- Escalate test-suite concerns to test-effectiveness-auditor.
Also fix tests/test_db.py, which was missing 'from sqlalchemy import text' while using it in 14 places. Three tests were failing with NameError. Wrapped the pre-existing long lines in the same file so it lints clean.
Document the deliberate nicegui==3.13.0 pin in pyproject.toml, a new runbook dependency upgrade policy, and the reviewer skill, so the pin is not flagged as a defect or widened as incidental cleanup.
Co-authored-by: Copilot App <[email protected]>
Adds .github/workflows/quality-gate.yml, running the gate on push and pull
request. CI invokes `pre-commit run --all-files` rather than restating the
`ruff check` and `ty check` commands, so the checks keep a single definition
in .pre-commit-config.yaml and local and CI cannot drift (plan task 2).
The workflow writes a .env file rather than exporting an environment
variable. The two are not equivalent here: Settings reads the .env file,
while the external-test skip guard reads os.getenv, so an exported variable
un-skips the external tests and sends them to the network. Measured in CI:
no .env gave 115 failures and 18 errors, an exported dummy key gave 3
failures, and a written .env file reproduced the local baseline exactly.
Negative-tested on a scratch branch: a deliberate lint error failed the run
at `ruff check` with exactly the planted errors, confirming the gate blocks
rather than merely reporting (plan task 4). The subsequent clean run passed
ruff and ty and reported 295 passed, 4 skipped, matching local and
confirming the four credential-gated tests skip cleanly (plan task 3).
That first green run caught a real platform-dependent defect. PromptStore
rejected non-direct-child names via `Path(name).name != name`, which is
platform-dependent: on POSIX a backslash is an ordinary filename character,
so "nested\prompt.md" passed the guard and failed later as NOT_FOUND rather
than VALIDATION. Windows cannot reproduce it. No traversal was possible,
since the path.parent != root check still held, so the impact was a wrong
error category and a red gate. Both separators are now rejected explicitly,
matching the ^[^/\\]+$ pattern config.PromptFilename already used.
Also exports docs/ver4.7/review_log_v4_7.md, the working record kept across
all six phases: 50 entries, 1 still open. The open entry is a pre-existing
/ui redirect defect found during the Phase 3 UI walk and deliberately left
unfixed as outside the V4.7 scope boundary.
Co-authored-by: Copilot App <[email protected]>
Review log [8]. classify_unexpected_error already returned retriable=False and
the verdict was logged and then thrown away. Measured across src/: retriable
was assigned in 9 places and read in none.
The plan asks for a test that a programming error "does not silently retry".
Probing with an injected AttributeError showed that is not what happens, and
the two real failure modes need different fixes.
Mode A, raised after the claim commits (inside advance_job): raised exactly
once, job left at PROCESSING, retry_count 0, never re-claimed, because
claim_next_queued_job filters status == QUEUED. A permanently stranded job
with one swallowed log line, not a retry. advance_job's PROCESSING branch,
commented "Recover mid-flight jobs", is unreachable from the worker for the
same reason.
Mode B, raised before or during the claim: 20 raises in 1.2s, an unbounded hot
spin at the poll interval. It never reaches the per-job retry machinery, so
WORKER_MAX_RETRIES does not cap it and the plan's 60s worst case understates
this path.
services/workflows.py
_advance_job_with_containment wraps advance_job. Any escaping exception is
classified and the job driven to terminal FAILED, which is visible in the UI
and resubmittable. The caller session is rolled back first and the terminal
write runs in its own transaction, so it stays atomic even when the failure
left that session dirty (plan task 3). The loop continues, so one poison job
cannot halt transcription for every other job.
worker.py
handle_worker_exceptions re-raises non-retriable faults rather than
suppressing them; retriable ones are still suppressed so transient
conditions do not stop work. run_worker_loop catches that, logs CRITICAL and
returns cleanly. Returning rather than propagating matters: the exception
would otherwise surface only at app shutdown, through the wait_for in
worker_consumer_lifespan.
tests
test_run_worker_loop_survives_process_next_exception asserted the loop
SURVIVES a RuntimeError and continues, which is the Mode B defect written
down as an expectation. Replaced by
test_run_worker_loop_stops_on_non_retriable_exception, with a new
test_run_worker_loop_survives_retriable_exception so suppression of genuinely
transient faults stays covered, and
test_error_after_claim_fails_the_job_instead_of_stranding_it for Mode A.
All three were verified to fail on pre-fix code. The Mode B guard fails by
timing out, which is the infinite spin made visible.
Verified: 295 passed, 4 skipped, 0 ruff, 0 ty.
Co-authored-by: Copilot App <[email protected]>
Jim LancasterandCopilot App