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]>
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]>
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]>
Preserves the traceability the V4.7 plan depends on ahead of starting
implementation in a fresh session. Documentation only.
The V4.6 review log was maintained in a session-scoped database and cited
by number throughout the V4.6, V4.7, and V4.8 planning documents as
"review log [N]". Those citations were unresolvable outside the session
that produced them. The log is now exported verbatim to
docs/ver4.6/review_log_v4_6.md: 70 entries, of which 8 remain open, each
mapped to its disposition (V4.7 phase, accepted risk, or operator
judgement).
The architecture review report is retained rather than deleted. It is the
canonical registry of the 32 finding IDs cited across six documents, so
removing it would orphan every CRIT/HIGH/MED/LOW reference in the planning
corpus. Instead it now carries a status banner marking it as a pre-V4.6
snapshot, warning that its paths, line numbers, and baseline metrics are
stale, recording that all 32 findings were dispositioned in V4.6 with only
MED-14 and HIGH-06 carrying into V4.7, and noting the two recommendations
later revised on evidence - the cancelled services/artifacts.py extraction
and the assumption that job_source and execution_attempt were
complementary rather than duplicated.
Co-authored-by: Copilot App <[email protected]>
Jim LancasterandCopilot App