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]>
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]>
Move orientation normalization to the Source-ingest boundary and delete the
ProcessingArtifact subsystem it was built to serve.
Stored pages are now already upright, so nothing downstream derives a rotated
copy: every stored byte is the byte a provider is later sent. Rotation runs in
store_source_file ahead of hashing, so source.file_hash and file_size_bytes
describe exactly what is on disk. normalize_orientation becomes bytes-in /
bytes-out, and JPEG output reuses the source quantization tables and chroma
subsampling instead of re-quantizing at a fixed quality - measured at 50.3-56.1
dB PSNR at -6% size, against 50.0-53.5 dB at +38% for quality=95.
ProcessingArtifact held 2 rows against 77 successful transcriptions; the
subsystem effectively never ran. Deleting it removes the artifact cluster from
sources.py, the derivative resolution in workflows.py, the pre-provider commit
that only existed to make an artifact row durable, and the artifact evidence
dump from the Source detail page. The transcription_quality_warnings payload
folds into execution_attempt.normalized_metadata, so that feature keeps working
without the table.
tools/migrate_v46_to_v47.py carries steps 1 and 2: it rotated the 58 stored
images carrying EXIF orientation 3 in place, updated their recorded hash and
size, dropped processing_artifact and removed its one external file. It is
idempotent, keyed on state rather than a version marker.
tools/migrate_v45_to_v46.py is deleted. That migration is complete, and after
V4.7 it would restore a V4.5 backup into a schema that no longer matches.
Also fixes tests/test_config.py, which read the developer's local .env and
failed whenever WORKER_MAX_RETRIES was set.
Co-authored-by: Copilot App <[email protected]>
Baseline was 207 diagnostics. Two real bugs were hiding in the noise:
- tools/run_destructive_tests.py imported ctypes.wintypes at module scope,
which raises on non-Windows, and called fcntl unconditionally. The Windows
and POSIX implementations now live under a module-level sys.platform split.
- tests/ui/test_sources_page.py constructed Source(...) without document_id.
Structural fixes, not suppressions:
- New src/transcription/db/loading.py owns the SQLModel-field to
QueryableAttribute reinterpretation via orm_attribute()/selectinload()/
defer(). This removed 42 "# pyright: ignore[reportArgumentType]" comments
across documents/jobs/people/sources. Its docstring records that
selectinload(A.b, B.c) is NOT equivalent to the chained form: varargs
applies the selectin strategy only to the last path element, which under
lazy="raise" raises InvalidRequestError at render time.
- db/session.py transaction_scope no longer accepts or yields
AsyncSessionTransaction. No caller ever passed one, sessionmaker.begin()
yields an AsyncSession, and the dead branch was latently buggy because
services call .exec(). Cleared 7 workflows.py diagnostics.
- services/registry.py RegistryService is bound by a new RegistryEntry
Protocol instead of bare SQLModel, so the shared implementation can read
id/label/normalized_label/is_active. Cleared 9 diagnostics.
- Column expressions in sources.py/jobs.py/test_store.py wrap in sqlmodel
col(), the idiom already used in registry.py.
- read_source_navigation wraps its literal tuple bounds in literal().
- normalization.py narrows with isinstance(image, TiffImageFile) rather than
comparing image.format, since tag_v2 is TIFF-only.
- linked_people.render uses @ui.refreshable_method, the NiceGUI API for bound
methods.
- The OpenRouter capturing client re-raises ResponseNotRead when the response
stream is not async rather than mis-wrapping it.
Tooling gate:
- New .pre-commit-config.yaml runs ruff check and ty check as blocking hooks.
No pre-commit config previously existed. Negative-tested: injecting a type
error fails both hooks.
- The last two "# pyright: ignore" comments (config.py) are removed; ty does
not honor pyright directives. One "# ty: ignore" remains, in
tests/test_prompts.py, where the test deliberately assigns to a frozen
field to assert ValidationError.
- asyncio_default_fixture_loop_scope is pinned to "function" so
pytest-asyncio behavior does not shift on upgrade.
Verification: ruff check clean, ty check reports 0 diagnostics, 292 passed
and 4 skipped, pre-commit passes and demonstrably fails on a regression, and
tools/run_destructive_tests.py runs on Windows.
Co-authored-by: Copilot App <[email protected]>
ui/homepage_store.py was the only storage path in the codebase derived from
Path(__file__).parents[3] rather than from Settings. That made it the one
storage root the operator could not relocate, and it resolved incorrectly
outside a source checkout - an installed distribution would have written
homepage content into the package directory in site-packages.
- config.py: add homepage_dir, defaulting to ./data/homepage so the location
is unchanged for anyone launching from the repository root.
- homepage_store.py: resolve the directory and markdown path from Settings,
with an optional settings parameter on every function so callers and tests
can override without patching module constants. HOME_PAGE_DIR and
HOME_PAGE_MARKDOWN_PATH constants are replaced by homepage_dir() and
homepage_markdown_path().
- tests/ui/test_homepage_store.py: covers the setting being honored, markdown
round-tripping, image storage and listing, and two configurations not
sharing storage.
Note: the default is now CWD-relative, matching artifact_dir and upload_dir,
rather than anchored to the repository root.
Verification: ruff check src tests clean; 292 passed, 4 skipped.
Co-authored-by: Copilot App <[email protected]>
MED-01 - move remaining blocking work off the event loop:
- normalization.py gains normalize_orientation_async; the Pillow decode,
transpose, and re-encode now run via asyncio.to_thread. The sync entry point
stays for tests and documents that it blocks.
- OrientationNormalization.digest_sha256 becomes a stored field computed inside
normalize_orientation, which already runs off-loop, instead of a property that
hashed page-sized derivative bytes on the caller's thread.
- SourceService._write_and_digest_artifact performs the artifact write and its
sha256 in a single worker-thread hop; both external-artifact write sites are
now dispatched through to_thread.
- transcribe_image dispatches load_source_payload and build_prompt_execution
through to_thread.
MED-04 - replace functools.cache on the engine and session factories with
explicit URL-keyed registries. dispose_engine and dispose_session_factory now
evict only the requested URL; previously cache_clear() tore down every other
database in the process, and dispose_engine would construct an engine for an
unknown URL purely to throw it away. New tests/test_engine_registry.py covers
distinct engines per URL, targeted eviction, and the unknown-URL no-op.
config.py - replace object.__setattr__ in normalize_provider_models with a
model_validator(mode="before") over the raw input, so the derived selector is
produced by normal construction rather than by mutating a frozen instance.
model_copy(update=...) was tried first and rejected: pydantic-settings does not
support a top-level validator returning anything other than self when validating
via __init__. provider_model is now stripped as well as the tuple entries.
models.py - add onupdate to the five updated_at columns and to Job.date_updated,
and drop the 10 manual "updated_at = datetime.now(UTC)" assignments across the
document, job, people, registry, and source services. Verified DDL-neutral by
hashing CreateTable output for every table on both the sqlite and postgresql
dialects before and after: identical, so this stays in Phase 6 and Phase 2 does
not need re-verification. New tests/services/test_timestamps.py asserts an
update through each service advances the timestamp.
MED-08 - Job.filename no longer swallows every exception to None. Relationships
declare lazy="raise", so the new _loaded_attribute helper inspects load state
explicitly and returns None only for genuinely unloaded attributes; real errors
now surface. Job.error_detail uses the same helper, which also removes its
unguarded read of the lazy="raise" job_sources relationship.
Verification: ruff check src tests clean; 288 passed, 4 skipped.
Co-authored-by: Copilot App <[email protected]>
Completes the table consolidation deferred within Phase 5. Section 4 of the
review lists table construction as a duplication pattern; three call sites had
been left bypassing the canonical builder.
- table/common.py: build_table gains a row_key parameter so callers with a
non-"id" primary key can use it.
- linked_people.py: replaces its hand-rolled ui.table with build_table
(row_key="person_id", selection="multiple", rows_per_page=0, no search).
- print_preview_page.py: _render_metadata_table and _render_job_table now share
a local _render_print_table helper. Print tables deliberately do not use
build_table - they must never paginate or render a search box, and they carry
print-only styling. The helper docstring records that rationale.
- tests/test_ui_boundaries.py: new AST guard asserting ui.table() is called from
exactly two modules - components/table/common.py and pages/print_preview_page.py.
Also closes the intermittent tests/ui/test_jobs_page.py failure observed once
after Phase 5 as environmental. Unreproduced across ~54 sequential full-suite
runs (including a 25-run soak) and 5 concurrent-process runs. No code change.
Verification: ruff check src tests clean; 275 passed, 4 skipped.
Co-authored-by: Copilot App <[email protected]>
Jim Lancaster
