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]>
Review log [55]. Three historical local_timeout rows recorded 0.4-2.0s more
than the configured budget because the measurement window opened before the
provider call.
The plan named two causes, and both were already gone. Diffed against
f86c0ff~1: at V4.6 the window held resolve_provider_input (async;
normalization + artifact write + DB work) and a session.commit(). Phase 1
deleted both. What remains between the clock and the wait_for is
build_provider_input, now pure field copying because normalization moved to
ingest and file_hash is already stored: 6.2 us per call, zero awaits, so it
cannot yield to the event loop.
A third cause was still there and is not in the plan. The regression test
below measured 890ms where ~200ms was expected. services.sources.provider is
a lazy property that appears as an argument expression to _call_transcriber,
so it is evaluated after the clock starts but before wait_for begins timing.
Constructing OpenRouterTranscriptionProvider costs 475ms on first access and
0.001ms after, so the first attempt of every worker process booked half a
second of HTTP client construction as provider latency. That plausibly
accounts for the low end of the historical overshoot.
workflows.py
- Re-capture monotonic_started_at immediately before the wait_for, reusing
the same variable. The pre-loop assignment stays as the fallback: binding
a new name inside the try would leave the general-exception handler
referencing an unbound variable when build_provider_input raises. All
three duration write sites (success, TimeoutError, general failure) then
measure the correct window with no further change.
- Hoist the provider property above the per-source loop. It is
loop-invariant, so this also removes the repeated lookup from the two
evidence-capture sites.
tests/services/test_workflows_reliability.py
test_timeout_duration_excludes_pre_call_setup simulates 400ms of blocking
setup against a 200ms budget and asserts the recorded duration sits near
the budget and well clear of budget+setup. Confirmed to fail on the pre-fix
code (assert 625 < 540) and pass after, so it guards behaviour rather than
restating it. This is the plan's verification criterion as a test.
ui/pages/sources_page.py
_format_duration renders >=1s as "27.6 s" and below that as "612 ms",
replacing the raw "27612 ms". No test asserted the old format.
Plan task 3 (record preprocessing as its own value) declined and logged as a
deviation: after Phase 1 there is no preprocessing left to record, and a
preprocessing_ms column to measure 6 us of attribute copying is complexity
without a reader.
Verified: 293 passed, 4 skipped, 0 ruff, 0 ty.
Co-authored-by: Copilot App <[email protected]>
Decompose SourceService along the aggregate boundary and then correct the
instruction file that caused it to grow, in that order. The refactor is the
empirical test of the rule.
services/evidence.py (new)
EvidenceService owns ExecutionAttempt: read_latest_execution_attempt,
list_execution_attempts, promote_machine_attempt, build_evidence_export,
plus the LatestExecutionAttempt projection. Moved verbatim from sources.py.
services/errors.py (new)
The five-class error hierarchy (PromptLoadError, TranscriptionError,
TranscriptionNotFoundError, SourceDeleteBlockedError,
CandidatePromotionError) moved out of sources.py. evidence.py needs
TranscriptionNotFoundError, and test_service_boundaries.py correctly
rejected the sibling import. errors.py defines no *Service class, so it is
a legal shared home. This was the boundary test doing its job, not an
obstacle to route around.
sources.py 1,389 -> 885 lines (1,063 after Phase 2).
services/__init__.py
ServiceBundle and from_session_factory register evidence. Note that
field-by-field ServiceBundle construction silently binds services to the
process-global session factory via default_factory; from_session_factory is
the only safe constructor. Two test bundles were fixed for this.
.github/instructions/services.instructions.md
Rewritten to describe the boundaries the decomposition actually produced,
per plan Phase 3 task 7 and review log [59].
- "1 service class per data model" -> one service class per aggregate.
The table-shaped rule is the measured cause of sources.py reaching
1,389 lines; DocumentType has no lifecycle without Document.
- New Model Ownership section. Junctions are owned by their lifecycle
owner, the service that creates and deletes the rows: document_person
to PeopleService (sole writer, measured), job_source to SourceService.
Two carve-outs are stated rather than left as silent violations:
cascade deletion when a service deletes its own aggregate root, and
status transitions that create and delete nothing (cancel_job,
resubmit_failed_sources), which are Job lifecycle events on the work
queue. EvidenceService.promote_machine_attempt's two-field write to
Source is named and scoped.
- Mandatory CRUD softened to intent. It was already false: five modules
define no service class, EvidenceService has no create/delete because
ExecutionAttempt is append-only, RegistryService uses <op>_entry.
- Separated reading across models via eager loads from the owning root,
which is allowed, from importing another service, which is not. The old
line 13 and lines 75-77 read as contradictory.
- Typo: picutre.
No code was moved to satisfy the rule.
tests/test_service_boundaries.py
Docstring no longer cites the instruction file by line number; that anchor
would desynchronise silently. errors.py added to the neutral-module list.
Verified: 292 passed, 4 skipped, 0 ruff, 0 ty. All 25 /ui/* routes walked
against the live app; 24x 200. /ui/documents/{id}/sources 404s via a 307 that
drops the /ui prefix, confirmed pre-existing (last touched in 6a3ee26) and
left alone as out of scope.
Co-authored-by: Copilot App <[email protected]>
Jim LancasterandCopilot App