Files
transcription/.github/instructions/services.instructions.md
T
Jim LancasterandCopilot App de18c2e9da
Quality Gate / gate (push) Failing after 48s
Fix workflow commit atomicity, error path leak, and UI error boundary
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]>
2026-08-23 18:02:04 -05:00

9.4 KiB

description, applyTo
description applyTo
Follow these guidelines when editing the services src/transcription/services/*.py

Services

Structure

  • Project core data models are defined in models
  • One service class per aggregate, not per table. An aggregate is a root model plus the models that have no independent lifecycle of their own. DocumentType has no meaning without Document, so it belongs to DocumentService; it does not get its own service. Splitting per table produces services that must reach across each other for every real operation, which is what line 13 forbids.
  • Only services interact with the database, and only through async methods.
  • A service module must not import another service module. This is enforced by test_service_boundaries. Shared types go in a neutral module that defines no service class (see errors).
  • Not every module in this package is a service. Helper modules that define no *Service class (base, errors, normalization, prompts, quality, media_storage, source_media) are free-function modules and are exempt from the service rules below.
  • Cross-cutting error behavior must follow error-handling instructions.

Model Ownership

Every model has exactly one owning service. The owner defines that model's invariants and is the only service that may create or delete its rows.

Model Owner
Document, DocumentType DocumentService
Source, JobSource SourceService
Job JobService
Person, PersonRole, DocumentPerson PeopleService
ExecutionAttempt SourceService

Junction tables

A junction table is owned by the service that creates and deletes its rows — its lifecycle owner. The service on the other side may read through the junction (via selectinload) but must not create rows in it.

  • document_person -> PeopleService. Every write is there; DocumentService only eager-loads through it.
  • job_source -> SourceService, which creates the row, records each page's outcome, and deletes it.

Two consequences follow, and both are deliberate:

  • Cascade deletion is not a violation. A service deleting the aggregate root it owns may delete junction rows referencing that root, because they cannot outlive it (JobService.delete_job_with_guardrails).
  • Ownership governs creation and deletion, not every state transition. job_source is both a link and the transcription work queue. JobService.cancel_job and resubmit_failed_sources transition job_source.status across a whole job, because that transition is a Job lifecycle event, not a per-page outcome. They create and delete nothing.

EvidenceService is read-focused and projection-focused. It may coordinate selection flows, but append-only attempt creation remains in SourceService write paths.

If a new operation cannot be expressed within one owner, it belongs in an orchestration module, not in a cross-service import.

Error Handling

  • Errors used by a single service are defined at the top of that module and inherit from AppError.
  • Errors shared by more than one service go in errors, which defines no service class and is therefore importable by any of them.
  • Use a context manager for large try/except blocks, like handle_transcription_errors in sources.
  • Category mapping, retry behavior, and translation boundaries are defined in error-handling instructions.
  • Service-edge exception translation must be deterministic: map to canonical categories and preserve clear provider->service->API/UI boundaries.

Checklist

  • Uses ServiceBase for common logic
  • Session kwarg for AsyncSession to pass a session object into each method
  • Services use self._session_scope in their methods to pass the session through
    • Multiple operations on the same object(s) require sharing a session between all the methods used
  • Every model the module touches is either owned by it or reached read-only
  • Evidence writes preserve append-only semantics

CRUD Methods

  • Name format <operation>_<model>, for example create_document or update_job.
  • Where a service exposes create/read/update/delete for its root model, define them at the top of the class in that order, before derived reads and workflow helpers.
  • Not every aggregate needs all four. ExecutionAttempt is append-only evidence written by SourceService workflow-facing methods, so EvidenceService deliberately exposes reads and no create or delete. Do not add unused CRUD methods to satisfy symmetry.
  • RegistryService is generic across small lookup models and uses <operation>_entry naming instead.

Transaction Finalization

When a service method accepts an optional session kwarg, write methods must use self._finalize to finalize the transaction properly according to whether or not they are sharing a session.

  • If session is None: the method owns the transaction and should commit().
  • If session is provided: the method must not commit; it should flush() so IDs and FK values are available to the caller's transaction.
  • Use refresh() on returned ORM objects when the caller needs DB-populated values (defaults, triggers, merged state).

Workflow Transaction Boundaries

For multi-step job lifecycles, orchestration functions must use explicit transaction phases.

  • Transaction A (claim): transition JobStatus.QUEUED -> JobStatus.PROCESSING and commit immediately.
  • Perform provider/network work outside database transactions.
  • Transaction B (terminal success): write transcript content and set JobStatus.TRANSCRIBED in the same shared-session commit.
  • Transaction B (terminal failure): write transcript error detail and set JobStatus.FAILED in the same shared-session commit.
  • Transaction C (retry path): write transcript error detail, increment retry count, and set JobStatus.QUEUED in one shared-session commit.

Atomicity rules:

  • Never commit transcript updates separately from the paired terminal/retry job status change.
  • Terminal state (TRANSCRIBED or FAILED) and transcript row changes must succeed or roll back together.
  • Retry persistence (QUEUED + retry increment + error detail) must succeed or roll back together.

Multi-page batches

These two requirements are in tension for multi-page jobs: each page should be durable as soon as its provider call returns, but the last page must commit together with the terminal status. process_queued_job resolves it by committing every page except the last one individually, then deferring the final page's write into _finalize_batch_outcome so it shares the terminal transaction.

Both paths are shielded against cancellation, so the final page is no less durable than the pages before it. Enforced by tests/integration/test_pipeline_atomicity.py; per-page durability is separately enforced by tests/services/test_workflows_reliability.py::TestWorkflowReliability::test_transcribed_page_is_committed_before_next_provider_call_finishes.

Contract Alignment

  • Treat docs/ as the active architecture and requirements baseline.
  • Legacy revision trees are out of scope for active implementation decisions and must not be referenced as authoritative service guidance.
  • Treat src/transcription/db/models.py as runtime schema ground truth and docs/schema.md as the field-accurate contract mirror.
  • Job.status success path is TRANSCRIBED.
  • JobSource.status is queue/projection state only (PENDING, TRANSCRIBED, FAILED, CANCELLED).
  • Source ingest may normalize media before persistence; persisted bytes/hash are canonical for processing and provenance.
  • ExecutionAttempt is append-only evidence history; do not mutate historical attempt rows in runtime code.
  • Source.raw_transcription is a projection, not authoritative history.
  • Service/UI read paths that touch relationships must be eager-loaded for lazy="raise" compatibility.
  • If model fields, enums, constraints, indexes, or relationship-loading semantics change, update docs/schema.md in the same change.
  • If Settings fields or defaults change in src/transcription/config.py, update .env.example in the same change so keys/defaults remain synchronized and no stale settings remain documented.

Schema Drift and Legacy Compatibility Policy

  • Prefer schema migration over startup reconciliation or runtime compatibility paths in service writes.
  • Do not add legacy read/write compatibility code in service workflows by default.
  • If drift is discovered and a migration decision is ambiguous (for example, one-way destructive DDL, uncertain data retention impact, or unknown deployment sequence), pause and ask the user to choose migration vs compatibility before coding.
  • If a temporary compatibility path is explicitly approved, document an expiration/removal plan in the same change.

Service Composition

A service method may read across models it does not own, using eager loads from its own aggregate root. What it may not do is import another service.

Operations that must write models owned by more than one service are composed in an orchestration module (store, workflows).