Fixes the three ui.instructions.md violations recorded as [HIGH-07] and extracts
the page-level duplication catalogued in review section 4.
Boundary violations
- jobs_page no longer imports session_scope or manages a transaction.
store.create_document_job and store.create_job_for_document accept an optional
session_factory and open their own session scope when the caller supplies
neither a session nor a factory.
- sources_page no longer calls sqlalchemy.inspect. SourceService
.read_latest_execution_attempt now returns a LatestExecutionAttempt read model
carrying a plain transport_body_deferred flag, so ORM loader state stays inside
the service. Rendered output is unchanged.
- Deletes ui/components/document_panzoom.py, its export, and its CSS. The
component was exported but used by no page. Pan-zoom is planned for a clean
reintroduction in V4.7 alongside the other photo/image work.
Extracted duplication
- ui/components/media_urls.py: pure upload-URL resolution taking upload_dir and
base_url, replacing two identical ~60-line copies in sources_page and
people_page.
- ui/components/guards.py: parse-then-render-terminal-message, replacing 28
hand-written guard labels across five pages.
- ui/components/confirm_delete.py: the blocked-dependency notice and the
delete/cancel action row, from four delete pages.
- ui/components/upload_panel.py: the auto-uploading file picker, from three
pages. Source accept lists now derive from services.source_media
.SOURCE_EXTENSIONS instead of being hard-coded.
- ui/components/table/registry.py: the two hand-rolled label-registry tables on
the settings page now go through build_table, which gained selection and
rows_per_page options.
- ui/components/formatters.py gains parse_uuid and parse_iso_date, replacing
five and two private copies.
- ui/runtime.py owns resolve_runtime_settings, replacing three copies and
removing get_settings from every page module.
[LOW-05]
- Upload handlers are annotated with events.UploadEventArguments.
- The Document and Person form builders return DocumentFormFields and
PersonFormFields dataclasses instead of dict[str, Any].
Verification
- tests/test_ui_boundaries.py asserts no page imports a session scope, a session
factory, get_settings, sqlalchemy, or sqlmodel, and that no component imports
request or application state.
- 275 passed, 4 skipped. ruff check clean.
Findings: HIGH-07, LOW-05
Co-authored-by: Copilot App <[email protected]>
Removes the duplicated registry CRUD, the hand-written not-found raises, and
the three divergent media writers. Behavior is preserved: every existing
Document Type and Person Role test passes unchanged, which is the primary
proof for MED-11.
[MED-11] Generic registry service
- New services/registry.py owns RegistryService[ModelT]: list, list with
counts, create with IntegrityError -> conflict mapping, read, update,
delete with built-in and referenced guards, is_referenced, and label
normalization/casefold keying.
- DocumentTypeRegistry and PersonRoleRegistry declare only the model, error
class, noun, short noun, retainer phrase, and reference columns.
- DocumentService and PeopleService keep their public method names and
delegate. Every user-facing message, error category, and suggestion string
is reproduced verbatim; only the noun is templated.
- Deleted _normalize_registry_label, _document_type_label_key,
_normalize_role_label, _person_role_label_key,
_document_type_is_referenced, and _person_role_is_referenced.
[MED-12] Shared not-found lookup
- ServiceBase._get_or_raise(model, id, *, session, error, noun, suggestion,
options) loads by primary key or raises the caller's error type.
- documents.py: local _get_document_or_raise deleted; replaced by _read_document
and adopted at read_document, delete_document, and set_document_type, which
previously bypassed the helper and hand-wrote the raise.
- sources.py: 8 identical Source raises and 1 Job raise collapsed into
_read_source / _get_or_raise.
- jobs.py and people.py already funneled through local _not_found builders and
were left alone.
[MED-13][MED-01] Single media writer
- New services/media_storage.py owns validate -> name -> mkdir -> write ->
wrap OSError. The write runs in asyncio.to_thread, so uploads no longer block
the event loop.
- store_source_file, store_person_portrait, and store_homepage_image now share
it and are async. Callers in store.py, people_page.py, and home_page.py await
them. mkdir failures are now also translated to a domain error instead of
escaping as a raw OSError.
- homepage_store gains HomepageStorageError so its write reports like the others.
[MED-14, partial] Service independence
- New services/source_media.py owns SOURCE_MIME_TYPES, SOURCE_EXTENSIONS,
lookup_source_mime_type, and supported_source_formats.
- documents.py no longer imports services/sources.py. Its print projection uses
the non-raising lookup and raises DocumentError, so DocumentService no longer
emits a TranscriptionError.
- api/v4_print.py imports the mapping from the policy module.
- store.py and workflows.py still import sources.py; both are orchestration
modules, which services.instructions.md:75-77 explicitly permits.
- Splitting SourceService itself remains deferred to V4.7.
[LOW-08] Query shape
- list_sources_detail filters job_id with a JOIN on JobSource instead of
loading every Source and filtering in Python.
- read_source_navigation replaces the full ordered-id scan and .index() with
two row-value comparisons bounded by LIMIT 1.
- list_processing_artifacts gains the limit parameter its summary sibling
already had.
- build_evidence_export runs artifact integrity hashing and file reads through
asyncio.to_thread.
Tests
- tests/test_service_boundaries.py: AST guard asserting no service module
imports a sibling service module, plus a guard that the scan is non-empty.
- tests/services/test_transcription_service.py: asserts the job_id filter emits
a JOIN, and that navigation emits exactly two LIMIT queries.
- tests/services/test_store.py: the two storage tests are now async.
Verified: 276 passed, 4 skipped; ruff check clean.
Claim jobs atomically [CRIT-01]
- Replace JobService.read_next_queued_job with claim_next_queued_job, which
selects and transitions QUEUED -> PROCESSING inside one transaction. The old
read-then-write sequence left a window in which two workers could observe the
same QUEUED row.
- Add the missing .limit(1). The poll previously ordered the entire queued set
and discarded all but the first row.
- Drop the eager loads from the hot poll entirely. They were pure waste:
process_queued_job immediately re-reads the job through read_job with the
relationships it actually needs.
- Guard the row with with_for_update(skip_locked=True) on PostgreSQL so the
claim stays correct once more than one worker exists. On SQLite the claim is a
bounded single-writer transaction.
- Correct the comment at the remaining direct-call claim site, which described
the hazard rather than the guarantee.
Reuse the provider connection [HIGH-02]
- Build the ServiceBundle once per worker loop instead of once per job, and
close it at loop shutdown. Every job previously constructed a new
SourceService, and with it a new provider adapter and a new httpx.AsyncClient,
paying a full TLS handshake per page and discarding the connection pool.
- process_next_queued_job now accepts an optional caller-owned bundle and only
closes bundles it created itself.
Uncap the provider timeout [HIGH-03]
- Remove le=20.0 from worker_provider_timeout_seconds. The cap equalled the
default, so the ceiling could never be raised, and dense-page vision
transcription routinely needs longer. Default raised to 180s.
- Pass an explicit httpx.Timeout to the OpenRouter AsyncClient. httpx defaults
every phase to 5 seconds, so the real read budget was 5s regardless of the
configured value; the outer asyncio.wait_for could never be the binding
constraint. Connect stays at 10s.
Tighten the provider boundary [MED-03]
- Declare model, current_request_manifest, current_transport_evidence, and
aclose on the TranscriptionProvider Protocol.
- Delete the per-call inspect.signature(adapter.transcribe).parameters
reflection and the untyped kwargs dict it fed. The Protocol had declared
requested_model all along, so the reflection was dead defensive weight on the
hot path.
- Replace the three getattr probes for aclose and the evidence attributes with
direct typed access.
Deduplicate bundle construction [MED-06]
- Add ServiceBundle.from_session_factory and ServiceBundle.aclose, replacing the
duplicated four-service instantiation blocks in app.py and worker.py.
- _recover_stale_processing_jobs now uses the bundle built moments earlier
instead of constructing a second JobService.
Tests
- Claiming returns the oldest job, marks it PROCESSING, never hands the same job
out twice, and emits exactly one unadorned SELECT carrying LIMIT and no JOIN.
- The worker loop threads one bundle through consecutive jobs and closes it once
at shutdown; a caller-owned bundle is left open.
- Settings accepts a timeout above 20 seconds and still rejects zero.
- The OpenRouter client's read, write, and pool timeouts track the configured
budget rather than the httpx default.
Note: .env in this checkout still pins WORKER_PROVIDER_TIMEOUT_SECONDS=20 and
should be raised to pick up this fix.
Verified: 268 passed, 4 skipped; ruff check clean.
Co-authored-by: Copilot App <[email protected]>
These changes all regenerate the same schema, so they land together and revert
together. A partially applied schema pass is not a valid state.
Remove hand-rolled migrations [HIGH-05]
- Delete upgrade_schema and the _upgrade_person_family_search_id /
_upgrade_v42_evidence_tables / _upgrade_v45_selection_columns chain, plus the
two tests that exercised them. The DDL was SQLite-shaped raw SQL that would
not have run on PostgreSQL. create_all now derives everything from metadata
and remains gated by Settings.should_bootstrap_schema. No raw ALTER TABLE or
CREATE INDEX string remains in src.
Break the foreign key cycle [HIGH-08]
- Declare Source.preferred_execution_attempt_id with use_alter=True and an
explicit constraint name. source / job_source / execution_attempt formed an
unresolvable cycle that made metadata.sorted_tables emit an SAWarning and
order execution_attempt before source, which would have been a hard
create_all failure on PostgreSQL and was invisible on SQLite.
- As a side effect the column is now a dialect-aware Uuid rather than the
hardcoded CHAR(32) the raw upgrade DDL produced, so it emits native UUID on
PostgreSQL.
Index the hot filters [HIGH-04]
- Add composite Index("ix_job_status_date_created", "status", "date_created")
for the worker poll, and index the foreign keys the worker and detail pages
filter on: job.document_id, source.document_id, job_source.job_id,
job_source.source_id, document.document_type_id, and the three
document_person foreign keys.
Stop preloading by default [CRIT-02]
- Flip 16 relationships from lazy="selectin" to lazy="raise". The bidirectional
selectin defaults meant loading one Job pulled a large connected subgraph.
- Three further relationships (ExecutionAttempt.job_source,
ProcessingArtifact.execution_attempt, ProcessingArtifact.source) declared no
lazy at all and defaulted to "select", which raises MissingGreenlet under
async. These are now "raise" as well.
- Only 5 of 262 tests failed under the flip; the service layer already carried
explicit eager loads. Fixes went into the service queries, never back into
the models:
- PeopleService._finalize_link refreshes document, person, and role_ref so
the DocumentPerson write endpoints can still project them.
- JobService.update_job_state loads job_sources -> source so the Job it
returns still answers .error_detail and .filename.
- Two tests that bypassed the service layer now load explicitly.
- Audited every UI relationship access against its feeding service method; all
resolve to *_detail / list_*_detail variants with complete eager loads.
Tests
- Assert the composite and hot foreign key indexes exist in a fresh schema.
- Assert metadata.sorted_tables raises no SAWarning and orders source before
execution_attempt.
- Assert preferred_execution_attempt_id is a Uuid that compiles to UUID on
PostgreSQL and that its foreign key carries use_alter.
- Guard CRIT-02 from regression: no mapped relationship may declare a lazy
strategy outside {raise, noload}.
The development database was rebuilt from metadata rather than upgraded; the
previous file is retained out of tree as the Phase 8 migration source.
Verified: 266 passed, 4 skipped; ruff check clean.
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]>
zoltan57andCopilot App
John Lancaster