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]>
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]>
Pure remediation; no behavior change. Every item traces to a finding in
docs/architecture_code_review_2026-08-17.md.
Deletions
- Delete app_state.py, which had zero importers and whose get_session_factory
raised TypeError at runtime [HIGH-01].
- Delete services/transcription.py and point build_prompt_execution imports at
services/sources.py; drop the store.py compatibility aliases [MED-05].
- Delete ServiceBase.queue and its unparameterized asyncio.Queue [MED-07].
- Delete db/operations.get_next_queued_job, a divergent duplicate [CRIT-01].
- Drop the discarded load_docs parameter from list_jobs [LOW-03].
Config
- Delete worker_retry_backoff_seconds; no backoff behavior existed anywhere, so
wiring it would have been a new feature [MED-02].
- Wire sqlite_check_same_thread through get_engine. The engine hardcoded the
setting's own default, so this preserves behavior exactly [MED-02].
- Replace DATABASE_URL in docker-compose.yml with the nested DATABASE__DRIVER /
DATABASE__PATH names. Settings uses env_nested_delimiter with extra="ignore",
so DATABASE_URL was silently discarded [MED-10].
UI
- Move the 23KB inline VIBESCRIBE_LOGO_SVG to ui/static/vibescribe_logo.svg and
load it through a cached read_svg sibling of read_css [MED-09].
- Route the portrait upload failure through error_presenter.show_error [LOW-07].
- Cancel the job detail auto-refresh timer instead of only deactivating it, and
name its interval constant [LOW-06].
Worker
- Make WorkerNotifier runtime_checkable and validate the resolved object in
resolve_worker_notifier, which previously returned any non-None attribute
unchecked [LOW-04].
Docs and lint
- Fix two stale paths in services.instructions.md, one of which pointed at the
module deleted here [LOW-02].
- ruff check --fix to zero [LOW-01].
Verified: 264 passed, 4 skipped; ruff check clean.
Co-authored-by: Copilot App <[email protected]>
Jim Lancaster
John Lancaster