The handoff brief was a work order for phases 2-5. That work is done, so the
document now describes a future that already happened and would misdirect
anyone who found it.
The review report itself had the same problem in weaker form: its findings read
as open. Adds a status banner marking it closed and retained for reasoning only.
The banner also records that two of its recommendations were wrong on contact.
The HIGH-03 fix as written would have stripped root-cause data from
ExecutionAttempt provenance, and the HIGH-01 fix had to preserve per-page
durability the report never mentioned. Leaving that unstated invites someone to
'restore' the report's version later.
Co-authored-by: Copilot App <[email protected]>
Nothing in this repo was auto-read by an agent at the start of a session.
The .github/instructions files only attach once a matching file is edited,
which is too late to steer strategy, and the canonical docs set is not
discoverable without already knowing to look for it.
AGENTS.md routes rather than duplicates: it states the authority order
(docs/* first, docs/reviews/** explicitly non-canonical), the uv-only
command set, the test-enforced boundaries, and the change protocol.
The Traps section records failure modes this codebase has actually produced
rather than generic advice: the message/detail split that leaked paths in one
direction and degraded provenance in the other, the two competing atomicity
invariants in workflows.py where the obvious simplification breaks multi-page
durability, and the habit of changing a shared symbol without enumerating its
consumers.
Verified: cited test paths exist, full suite passes, ruff/ty clean.
Co-authored-by: Copilot App <[email protected]>
The agent file had drifted from the skill it delegates to, in two ways that
would corrupt a review run.
Report target: the agent said write reports to ./docs, but the skill targets
./docs/reviews/<date>-code-review.md and explicitly marks docs/reviews/** as
non-canonical. Following the agent would place a dated, opinionated review
inside the canonical authority set that findings are supposed to resolve
against.
Verification commands: the agent said run 'ruff check', 'pytest', and 'ty'.
None are on PATH in this uv project, so an agent following its own instruction
gets command-not-found and is pushed toward guessing instead of verifying.
The agent now defers to the skill for all specifics rather than restating them,
which is what let the two copies drift apart. Also carries forward the
consumer-tracing rule and states the read-only scope explicitly.
Co-authored-by: Copilot App <[email protected]>
Three gaps surfaced by implementing the 2026-08-23 review's recommendations.
1. Recommendations were never verified the way claims were. The report's fix for
the error path leak would have stripped root-cause data from evidence records,
because the review traced one consumer of AppError.message and missed that
format_error_detail writes it to ExecutionAttempt.error_detail. Adds workflow
step 9 (validate recommendations against consumers), a Blast Radius field on
findings, and the worked example so the failure mode is concrete.
2. Fixes that sit between competing invariants were not flagged. The atomicity
recommendation did not note that per-page durability and terminal-status
atomicity pull in opposite directions, so the obvious simplification silently
breaks multi-page durability. Recommendations must now name both invariants,
the test guarding each, and the over-correction to avoid.
3. Severity could not express reachability. Two findings were latent behind a
default setting and a single-instance deployment, which is a sequencing
constraint: they must be fixed before the change that makes them live. Adds an
explicit Reachability field with Live / Latent / Theoretical.
Verified: meta contract guards and traceability tests pass.
Co-authored-by: Copilot App <[email protected]>
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]>
Jim Lancaster
