Export V4.6 review log and mark the architecture review as historical

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]>

docs/ver4.6/review_log_v4_6.md

@@ -0,0 +1,489 @@
+# V4.6 Implementation Review Log
+
+Working record kept during the V4.6 remediation release and the V4.7 / V4.8 planning that followed.
+
+This file is the canonical reference for citations of the form **`review log [N]`** in the V4.6, V4.7, and V4.8 planning documents. The numbers below are those `N` values.
+
+The log was maintained live in a session-scoped database and exported here so the citations remain resolvable in later sessions. It is a historical record: entries are not rewritten after the fact, so some capture reasoning that was later revised. Where an entry conflicts with a committed planning document, **the planning document wins**.
+
+## Legend
+
+| Field | Meaning |
+| :--- | :--- |
+| `kind` | `question` - needed a decision; `comment` - observation; `deviation` - departure from plan; `risk` - identified hazard |
+| `status` | `open` - unresolved; `answered` - resolved by a decision; `noted` - recorded, no action required |
+| `finding` | Finding ID in [architecture_code_review_2026-08-17.md](../architecture_code_review_2026-08-17.md), where one applies |
+
+**70 entries** - 8 open, 30 answered, 32 noted.
+
+## Still Open
+
+These carry forward. Most are scoped into V4.7; see [V4.7 scope boundary](../ver4.7/scope_boundary_v4_7.md).
+
+| ID | Finding | Summary | Disposition |
+| :--- | :--- | :--- | :--- |
+| [8] | - | handle_worker_exceptions swallows everything | V4.7 Phase 5 |
+| [18] | - | One flaky failure observed once, then five clean full runs | Watch item - no action |
+| [40] | HIGH-06 | No CI workflow enforces the gate | V4.7 Phase 6 |
+| [45] | n/a | The same JobSourceStatus enum is persisted two different ways | V4.7 Phase 2 (absorbed into the enum migration) |
+| [50] | HIGH-03 | Worst-case stall latency is now 60s (2 x 30s), down from 360s | Accepted risk - mitigated by 30s timeout and max_retries=1 |
+| [53] | HIGH-03 | PROVIDER_MODELS still offers two models that cannot finish a dense page within 30s | Operator judgement - deliberately left open |
+| [54] | n/a | Run-time telemetry is captured but has no aggregate view | V4.8, gated on V4.7 Phase 4 |
+| [55] | n/a | duration_ms measures end-to-end page processing, not provider latency | V4.7 Phase 4 |
+
+## Full Log
+
+### V4.6 Phase 1 - deletions and quick wins
+
+#### [1] worker_retry_backoff_seconds deleted, not wired
+
+*deviation* - `LOW-05` - **noted**
+
+No backoff behavior existed anywhere in the codebase. Wiring it would have been a new feature, which V4.6 forbids. Deleted the setting and its test instead.
+
+#### [2] sqlite_check_same_thread wired, not deleted
+
+*deviation* - `MED-02` - **noted**
+
+Opposite call from the one above: the engine hardcoded the setting's own default value, so wiring it through preserved behavior exactly.
+
+### V4.6 Phase 2 - schema re-level
+
+#### [3] Dev DB safe to discard?
+
+*question* - **answered**
+
+Asked before the atomic schema re-level. You chose "Safe to discard, proceed". The old file was moved to data/transcription.db.pre-v46.bak rather than deleted, because Phase 8 needs it as the migration source.
+
+#### [4] Guard test found 3 relationships the review missed
+
+*comment* - `CRIT-02` - **noted**
+
+The new lazy-load regression test caught ExecutionAttempt.job_source, ProcessingArtifact.execution_attempt, and ProcessingArtifact.source declaring no lazy strategy at all, so they silently defaulted to "select". The review had only catalogued the 16 explicit selectinload cases.
+
+#### [5] lazy="raise" not smoke-tested in a browser
+
+*risk* - `CRIT-02` - **answered**
+
+Every relationship access was audited against its feeding service method and all resolve to detail variants with complete eager loads, and the suite is green. But no manual UI walkthrough was done. A missed path would raise at render time rather than silently N+1. [PARTIALLY RESOLVED] Post-migration smoke test against the real 282-row corpus: a service-layer walk over all 8 documents and every source exercised list_documents, list_jobs, read_document, list_sources, read_source_navigation and list_processing_artifacts with no lazy-load error, and all six /ui pages returned HTTP 200. NiceGUI renders over websocket, so this is not a substitute for clicking through a live browser session, but every query path is now exercised against real data.
+
+### V4.6 Phase 3 - worker and provider reliability
+
+#### [6] httpx default timeout was the real bug
+
+*comment* - `HIGH-03` - **noted**
+
+The review said the 20s cap was too low. The actual defect was larger: httpx.AsyncClient was built with no timeout at all, so every phase defaulted to 5s and the outer asyncio.wait_for could never bind. Real read budget was 5s, not 20s.
+
+#### [7] Your .env still pins WORKER_PROVIDER_TIMEOUT_SECONDS=20
+
+*question* - `HIGH-03` - **answered**
+
+I deliberately did not edit your .env. The new default is 180s but your local file overrides it. Do you want it raised, and to what value? [RESOLVED 2026-08-17] .env lines 51-52 replaced: the stale "[0-20]" comment is gone and WORKER_PROVIDER_TIMEOUT_SECONDS is now 180.0. Verified effective value via get_settings() = 180.0. .env is gitignored, so this is a local-only change with no commit. [REVISED 2026-08-18] User challenged 180.0 as too long to feel responsive. Queried the 80 migrated execution_attempt rows: 77 succeeded with max 18.5s, median 4.9s, p95 18.5s. The only 3 attempts over 20s are exactly the 3 local_timeout FAILURES (20.4/20.8/22.0s), so the distribution is CENSORED - the true duration of those 3 is unknown. Set to 60.0: ~3.2x the slowest observed success, while keeping time-to-discovery of a stall at one minute rather than three. User was right; evidence does not support 180.
+
+#### [8] handle_worker_exceptions swallows everything
+
+*comment* - **open**
+
+A stale monkeypatched signature raised TypeError, which the handler suppressed, spinning run_worker_loop forever. Same hazard exists in production code: any programming error inside the loop becomes an infinite silent retry. Out of V4.6 scope.
+
+### V4.6 Phase 4 - service layer consolidation
+
+#### [9] Registry consolidation did not reduce line count much
+
+*comment* - `MED-11` - **noted**
+
+Public method names had to survive so registry tests could pass unchanged, so each service keeps thin delegating wrappers. Net production code is down ~150 lines overall, but the registry work itself is roughly break-even. The win is single-source-of-truth behavior, not brevity.
+
+#### [10] MED-14 done only for documents.py
+
+*deviation* - `MED-14` - **answered**
+
+store.py and workflows.py still import sources.py. Both are orchestration modules, which services.instructions.md:75-77 permits, so I scoped the boundary test to service-class modules only. Flagging in case you read the rule more strictly. [DEFERRED TO V4.7 by user, 2026-08-17] The MED-14 boundary test stays scoped to service-class modules for V4.6; store.py and workflows.py continue to import sources.py under the Service Composition allowance. V4.7 should decide whether to widen the rule and the AST guard.
+
+#### [11] Two behavior changes in the media writer
+
+*deviation* - `MED-13` - **answered**
+
+mkdir failures now raise a domain error instead of escaping as raw OSError, and homepage image writes gained error handling they never had. Both are strictly better but are behavior changes, not pure refactors. [VERIFIED 2026-08-17] Grepped all of src/ for "except OSError": every hit is either the new media_storage.py:44 wrapper itself, a prompts.py file read, a normalization.py decode guard, or a best-effort cleanup/unlink (store.py:325-330, documents.py:259, jobs.py:349, sources.py:504). No caller wraps a media WRITE in except OSError, so translating mkdir failures into a domain error changes no existing handler behaviour.
+
+#### [12] DocumentService print path no longer raises TranscriptionError
+
+*deviation* - `MED-14` - **answered**
+
+_print_media_type raises DocumentError for an unsupported extension where it previously raised TranscriptionError. Only reachable with a corrupt stored filename. Changed because a DocumentService emitting a transcription error is itself the boundary leak MED-14 is about. [VERIFIED 2026-08-17] Grepped all of src/ for "except TranscriptionError": the single hit is store.py:353, which wraps validate_source_content on the UPLOAD path and is unrelated to the print projection in documents.py. No handler anywhere catches TranscriptionError around _print_media_type, so the class change is behaviourally inert. Also clarified to the user that this item is a disclosure of a change already made, not an outstanding error, and is unrelated to .env.
+
+### V4.6 Phase 5 - UI boundaries and duplication
+
+#### [13] document_panzoom: delete or fix?
+
+*question* - `HIGH-07` - **answered**
+
+Unused 170-line component plus ~35 lines of CSS, superseded in practice by dark_room_viewer. You chose to delete it now and rebuild it cleanly in the next revision alongside other photo/image features. ACTION FOR V4.7: pan-zoom must be reintroduced.
+
+### V4.6 Phase 5 - UI boundaries and duplication
+
+#### [14] sources_page transport_body deferral question (answered; premise corrected)
+
+*question* - **answered**
+
+Initially believed the deferred-body branch was dead. Re-check showed _transport_display() is called with latest_attempt from read_latest_execution_attempt(), which DOES defer transport_body. Current behavior is already "Omitted from Source Detail". Fix is therefore a pure boundary move: read_latest_execution_attempt returns a LatestExecutionAttempt read model carrying transport_body_deferred: bool, and sources_page drops sqlalchemy.inspect. No behavior change. User preference recorded: simplest, most supportable, most robust; full bytes remain persisted and retrievable via Export Evidence.
+
+#### [15] Guard-message ordering changed on two Source routes
+
+*deviation* - **noted**
+
+sources_page previously parsed the route id BEFORE rendering the navigation header, then rendered the invalid-id message after it. Adopting the shared parsed_record_id() helper moved the nav header above the parse. Net rendered output is identical; only the internal call order changed.
+
+#### [16] Settings-page registry tables now render inside build_table
+
+*deviation* - **answered**
+
+The two label-registry tables on the settings page were hand-rolled ui.table calls. They now go through build_table via a new components/table/registry.py. build_table wraps its table in a ui.column, so the tables gain one extra container div. Search is disabled and rows-per-page stays 0, so visible behavior is unchanged. | RESOLVED (user directed consolidation): build_table gained row_key; linked_people.py converted; print_preview_page.py shares a local _render_print_table helper (print tables intentionally bypass build_table - no pagination, no search). New AST guard test_only_the_designated_owners_construct_a_raw_table pins ui.table() to exactly table/common.py and print_preview_page.py.
+
+#### [17] Two hand-rolled ui.table instances deliberately left alone
+
+*comment* - **noted**
+
+print_preview_page.py has two print-layout tables and linked_people.py has a component-local editor table. Neither wants build_table search or pagination, so converting them would add indirection without removing duplication. Flagging in case you want them unified later. | CLOSED AS ENVIRONMENTAL: unreproduced after ~54 sequential full-suite runs (incl. a dedicated 25-run soak with -rA traceback capture, 0 failures) plus 5 concurrent-process runs (2x tests/ui, 3x full suite). Not attributable to any V4.6 change; the single observed failure occurred immediately after a burst of bulk file rewrites. No code change made. Re-open if it recurs.
+
+#### [18] One flaky failure observed once, then five clean full runs
+
+*risk* - **open**
+
+tests/ui/test_jobs_page.py::test_job_delete_page_allows_deletion_for_queued_or_completed_job failed once and passed on every subsequent run (5 consecutive full-suite runs, 275 passed / 4 skipped). This matches the known pre-existing aiosqlite event-loop teardown noise that lands on a random test. Not introduced by Phase 5, but worth confirming during Phase 6/7.
+
+#### [19] store.create_document_job / create_job_for_document now own their session
+
+*comment* - **noted**
+
+To remove session_scope from jobs_page, both orchestration functions accept an optional session plus an optional session_factory and open their own scope when neither is supplied. Existing callers that pass a session are unaffected; tests pass unchanged.
+
+#### [20] Upload accept lists are now derived from SOURCE_EXTENSIONS
+
+*comment* - **noted**
+
+The job upload picker previously hard-coded .jpg,.jpeg,.png,.tif,.tiff,.pdf. It now derives the list from services.source_media.SOURCE_EXTENSIONS, so adding a Source format in one place updates the picker. The portrait and homepage pickers share a separate IMAGE_UPLOAD_EXTENSIONS list because they accept gif/webp/bmp, which are not valid Source formats.
+
+### V4.6 Phase 6 - async I/O and configuration hygiene
+
+#### [21] model_copy(update=...) rejected by pydantic-settings
+
+*deviation* - `MED-04` - **noted**
+
+Plan offered "model_copy(update=...) or a computed property" to replace object.__setattr__ in normalize_provider_models. model_copy failed: pydantic-settings warns "A custom validator is returning a value other than self ... isn't supported when validating via __init__" and 3 config tests failed. A computed property would have required renaming the env-facing provider_models field. Implemented as a model_validator(mode="before") over the raw input dict instead, so the derived value is produced by normal construction with no frozen-instance mutation. All 23 config tests pass.
+
+#### [22] provider_model is now trimmed
+
+*deviation* - **noted**
+
+The old object.__setattr__ path assigned provider_model without stripping whitespace; only the provider_models tuple entries were stripped. The before-validator now strips provider_model too. This is a behavior change, judged a correctness improvement since an untrimmed model id would be sent to the provider. No test asserted the old behavior.
+
+#### [23] onupdate confirmed DDL-neutral
+
+*comment* - **noted**
+
+Plan said onupdate moves to Phase 2 if it alters emitted DDL. Verified by hashing CreateTable output for every table on both the sqlite and postgresql dialects before and after the change: identical (b33ad56a...). onupdate stays in Phase 6; Phase 2 does not need re-verification.
+
+#### [24] No-op updates no longer bump the timestamp
+
+*deviation* - **noted**
+
+Removing the 10 manual "updated_at = datetime.now(UTC)" assignments means an update call that changes nothing no longer marks the row dirty, so onupdate does not fire and the timestamp stays put. Previously the manual assignment always bumped it. Judged more correct for a column that is supposed to track modification, but it is an observable change for any caller that relied on update-as-touch.
+
+#### [25] Homepage markdown I/O left unwrapped
+
+*question* - `MED-01` - **answered**
+
+ui/homepage_store.py reads and writes a single small local markdown file synchronously from home_page.py handlers. MED-01 names Pillow normalization, artifact writes, and evidence hashing; this is none of those and the payload is trivial. Left unwrapped to avoid scope creep. Flagging in case you want it wrapped anyway. [RESOLVED 2026-08-17] User decision: leave it synchronous. No change made.
+
+#### [26] Evidence manifest hashing left on the loop
+
+*comment* - `MED-01` - **noted**
+
+providers/evidence.py digest() hashes a small in-memory JSON manifest (microseconds), so it was left inline. The hashing that actually mattered was over page-sized image bytes: the derivative digest is now precomputed inside normalize_orientation (already off-loop) and the artifact digest now shares the same worker-thread hop as the write.
+
+#### [27] dispose_engine on an unknown URL changed behavior
+
+*comment* - `MED-04` - **noted**
+
+The old functools.cache version called get_engine(url) inside dispose_engine, which would construct an engine just to dispose it, and then cache_clear() wiped every other engine too. The registry version pops only the requested URL and no-ops on an unknown one. Covered by tests/test_engine_registry.py.
+
+#### [28] Added homepage_dir setting (user-approved scope addition)
+
+*deviation* - **answered**
+
+ui/homepage_store.py was the only storage path in the codebase derived from Path(__file__).parents[3] rather than from Settings, making it unconfigurable and wrong under a wheel install (it would resolve into site-packages). Not tied to a review finding ID, so it is a deliberate scope addition, approved by the user in-flight. Added Settings.homepage_dir (default ./data/homepage) and rewrote the module to resolve from Settings, with an optional settings parameter on every function. Covered by tests/ui/test_homepage_store.py.
+
+#### [29] homepage default is now CWD-relative
+
+*risk* - **answered**
+
+The old default resolved to <repo>/data/homepage regardless of working directory. The new default Path("./data/homepage") is relative to the process CWD, matching artifact_dir and upload_dir. Running the app from the repo root gives the identical location; running it from elsewhere does not. Consistent with every other storage root, but worth confirming against your deployment/launch scripts. [RESOLVED 2026-08-17] User confirmed the app is only ever launched from the repo root, so CWD-relative ./data/homepage and the old repo-anchored path are identical. Verified live: resolves to C:\GitHub\transcription\data\homepage containing the real homepage.md and portrait. No change needed. Revisit only if a service or scheduled task with its own working directory is introduced.
+
+#### [30] Homepage markdown I/O stays synchronous
+
+*comment* - `MED-01` - **answered**
+
+User question resolved: the async-wrapping question was dropped as negligible (one small local markdown file). The underlying concern turned out to be the hardcoded storage path, addressed separately via Settings.homepage_dir.
+
+### V4.6 Phase 7 - type checking and quality gate
+
+#### [31] selectinload varargs is not equivalent to chaining
+
+*deviation* - `HIGH-06` - **noted**
+
+selectinload(A.b, B.c) and selectinload(A.b).selectinload(B.c) produce an identical .path but the varargs form applies the selectin strategy ONLY to the last element. With lazy="raise" everywhere (Phase 2) the varargs form raises InvalidRequestError at render time. Cost 12 test failures before it was caught. Documented in the db/loading.py docstring.
+
+#### [32] New module src/transcription/db/loading.py
+
+*deviation* - `HIGH-06` - **noted**
+
+Rather than sprinkle 42 suppressions, the SQLModel-field to QueryableAttribute reinterpretation now has one documented home: orm_attribute(), selectinload(), defer(). All 42 "# pyright: ignore[reportArgumentType]" comments in documents/jobs/people/sources were removed as a result.
+
+#### [33] transaction_scope no longer accepts or yields AsyncSessionTransaction
+
+*deviation* - `HIGH-06` - **noted**
+
+AsyncSessionTransaction appeared nowhere outside db/session.py; no caller ever passed one, and sessionmaker.begin() was verified at runtime to yield an AsyncSession. The branch was also latently buggy: services call .exec() which a transaction object does not have. Removing the union cleared 7 downstream workflows.py diagnostics.
+
+#### [34] RegistryService is now bound by a RegistryEntry Protocol
+
+*deviation* - `HIGH-06` - **noted**
+
+RegistryService[ModelT: SQLModel] gave ty no visibility into id/label/normalized_label/is_active. A structural Protocol replaces the bare SQLModel bound - a genuine typing improvement rather than a suppression. Cleared 9 diagnostics.
+
+#### [35] normalization.py now uses isinstance(image, TiffImageFile) instead of image.format == "TIFF"
+
+*deviation* - `HIGH-06` - **noted**
+
+tag_v2 only exists on TiffImageFile. The isinstance check is semantically equivalent and types correctly.
+
+#### [36] linked_people.render switched from @ui.refreshable to @ui.refreshable_method
+
+*deviation* - `HIGH-06` - **noted**
+
+refreshable_method is the NiceGUI API intended for bound methods; the plain decorator mistyped self. render.refresh() call sites are unchanged.
+
+#### [37] read_source_navigation now wraps literal bounds in sqlalchemy.literal()
+
+*deviation* - `HIGH-06` - **noted**
+
+tuple_() rejects raw Python values under typing. literal() is the correct explicit coercion and preserves the emitted SQL.
+
+#### [38] openrouter capturing client re-raises ResponseNotRead for a sync stream
+
+*deviation* - `HIGH-06` - **noted**
+
+response.stream is typed SyncByteStream | AsyncByteStream. The narrowing guard re-raises rather than silently mis-wrapping, which is the honest behavior on an async client.
+
+#### [39] No pre-commit config existed; one was created
+
+*comment* - `HIGH-06` - **noted**
+
+The plan said "wire it into the existing pre-commit setup", but there was no .pre-commit-config.yaml (pre-commit was only a dev dependency, and there are no CI workflows either). A local-repo config with blocking ruff and ty hooks was created and negative-tested. NOTE: hooks use language: system, so the venv Scripts dir must be on PATH.
+
+#### [40] No CI workflow enforces the gate
+
+*risk* - `HIGH-06` - **open**
+
+.github/workflows/ is empty, so ruff/ty/pytest are only enforced locally via pre-commit, and only if the developer has installed the hooks (pre-commit install). Consider adding a CI workflow in a later release.
+
+#### [41] ty check driven from 207 diagnostics to 0
+
+*comment* - `HIGH-06` - **noted**
+
+Two real bugs were fixed en route: tools/run_destructive_tests.py imported ctypes.wintypes at module scope (raising on non-Windows) and used fcntl unconditionally; tests/ui/test_sources_page.py constructed Source(...) without the required document_id. Only two suppressions remain in the whole tree: one "# ty: ignore[invalid-assignment]" in tests/test_prompts.py which deliberately assigns to a frozen field to assert ValidationError.
+
+#### [42] asyncio_default_fixture_loop_scope pinned to "function"
+
+*comment* - `HIGH-06` - **noted**
+
+Set explicitly in pyproject.toml so pytest-asyncio behavior does not shift on upgrade.
+
+### V4.6 Phase 8 - data migration
+
+#### [43] V4.6 re-level changed no columns at all
+
+*comment* - `review 1a` - **noted**
+
+Diffing the backup schema against the current SQLModel metadata showed identical table sets and identical column sets for all 10 tables. What V4.6 actually changed is index coverage (9 new indexes: ix_document_document_type_id, ix_document_person_document_id, ix_document_person_person_id, ix_document_person_role_id, ix_job_document_id, ix_job_source_job_id, ix_job_source_source_id, ix_job_status_date_created, ix_source_document_id - none lost), the use_alter break in the FK cycle, and the relationship loading strategy. The migration is therefore a faithful FK-ordered row copy rather than a transformation.
+
+#### [44] Migration reads the backup with raw sqlite3, not the ORM
+
+*deviation* - `review 1a` - **noted**
+
+The plan anticipated ORM reads carrying explicit eager loads under lazy="raise". Reading raw rows is strictly safer: the V4.5 file is not guaranteed to satisfy the V4.6 mappers, and no relationship is ever traversed, so lazy="raise" cannot bite at all. Writes still go through SQLAlchemy Core against the live metadata, so the script will work against PostgreSQL unchanged.
+
+#### [45] The same JobSourceStatus enum is persisted two different ways
+
+*risk* - **open**
+
+job_source.status declares values_callable and stores lowercase VALUES ("transcribed"); execution_attempt.status does not and stores uppercase NAMES ("TRANSCRIBED"). Both columns use the identical JobSourceStatus enum. This is a genuine latent inconsistency: any raw SQL, reporting query, or future cross-dialect move has to know which spelling each column uses. It is NOT a finding in the review, so under the pure-remediation rule I did not change it - the migration accepts either spelling and round-trips both faithfully. RECOMMEND scheduling this for V4.7.
+
+#### [46] Should the migration be applied to the live data/transcription.db?
+
+*question* - **answered**
+
+The script is fully verified against a throwaway target: 282 rows copied, every table byte-identical to the backup cell-for-cell, idempotent re-run inserts 0, artifact integrity passes, no on-disk file touched. The live data/transcription.db currently holds only bootstrap seed rows (document_type 6, person_role 3) whose UUIDs differ from the backup, so a straight migration would ADD the backup rows alongside the seeds and likely trip the normalized_label uniqueness constraint. Applying cleanly requires replacing the live file. Awaiting user decision. [RESOLVED] User chose to back up and replace. data/transcription.db.seed-20260817-200555.bak holds the old seed file; a fresh DB was created and all 282 rows migrated with artifact integrity verified.
+
+#### [47] Provider timeout set to 60s on evidence, not on the review's suggested figure
+
+*comment* - `HIGH-03` - **noted**
+
+The review recommended 120s and the V4.6 plan used 180s, both chosen without data. The migrated corpus provides data: 77/80 attempts succeeded, all within 18.5s. 60s is the smallest value with real headroom that still surfaces a stall quickly. Revisit only if a genuine local_timeout occurs at 60s.
+
+#### [48] Three historical local_timeout failures are worth re-running
+
+*deviation* - **answered**
+
+All 3 FAILED execution_attempts were killed by the old 20s ceiling and carry response_received=1, meaning a response had begun arriving when the budget expired. With the ceiling now at 60s these three pages may well succeed on a retry. Their evidence rows were migrated unchanged, so the originals are preserved either way. | RESOLVED 2026-08-17: not 3 pages but ONE page (source 302aa684) x 3 models. Re-ran each model 2x with a 300s uncensored ceiling: gemini-flash 9.0/22.5s, claude-opus-5 27.0/27.6s, gpt-5.6 64.5/75.3s. All 6 succeeded - no hangs. gpt-5.6 exceeds the 60s value that was set, so .env raised to 120.0 (~1.6x slowest success). Historical stats were ~96% gemini-flash and understated the budget.
+
+#### [49] Reporting gap: 28 review_log entries were never surfaced to the user
+
+*risk* - **answered**
+
+My end-of-run summaries filtered on status IN (open, answered), which silently excluded every entry recorded as "noted" - 28 of 46. The user caught this. All 28 are now presented. Lesson: "noted" is not the same as "reported".
+
+### Post-V4.6 - timeout calibration and tuning
+
+#### [50] Worst-case stall latency is now 60s (2 x 30s), down from 360s
+
+*risk* - `HIGH-03` - **open**
+
+Superseded by the 2026-08-18 calibration: WORKER_MAX_RETRIES=1 and WORKER_PROVIDER_TIMEOUT_SECONDS=30.0 give a worst case of 60s. The underlying concern stands but is much reduced: handle_worker_exceptions (review_log id 8) still swallows every exception, so a programming error would burn 2 attempts silently with no UI feedback. Keep id 8 as the real fix.
+
+#### [51] Removed WORKER_RETRY_BACKOFF_SECONDS from .env
+
+*comment* - `HIGH-03` - **noted**
+
+The setting was deleted from Settings in Phase 1 (LOW-05). Because Settings uses extra="ignore" it sat in .env silently inert, which is exactly the DATABASE_URL trap the review flagged. Removed from .env so the file matches the model. No behavior change.
+
+#### [52] Timeout set to 30.0s and max_retries to 1 by user decision
+
+*comment* - `HIGH-03` - **answered**
+
+Full dropdown measured twice on the densest page in the corpus with a 300s uncensored ceiling. Fast cluster: gemini-2.5-flash 9.0/22.5, gpt-4o 21.6/22.9, claude-sonnet-4 26.2/26.7, claude-opus-5 27.0/27.6. Slow cluster: gemini-2.5-pro 49.5/79.4, gpt-5.6 64.5/75.3. User chose 30.0s at the low edge of the 27.6-49.5s gap because dense forms are <10 of ~3k documents and ejecting a stalled outlier is preferred over waiting. Worst case is now 2x30=60s. Agent recommended 40s for margin; user declined with stated rationale. Accepted.
+
+#### [53] PROVIDER_MODELS still offers two models that cannot finish a dense page within 30s
+
+*risk* - `HIGH-03` - **open**
+
+gemini-2.5-pro and gpt-5.6 remain selectable in the jobs page dropdown (ui/pages/jobs_page.py:146 reads settings.provider_models). Both exceed 30s on dense forms by design of the chosen budget, though both should still succeed on the ~99.7% of pages that are not dense forms. Left in the list deliberately - not removed - so the user retains them for quality comparison. Revisit if dense-form failures become noisy.
+
+### Post-V4.6 - V4.7 candidates identified
+
+#### [54] Run-time telemetry is captured but has no aggregate view
+
+*comment* - **open**
+
+execution_attempt.duration_ms is a required non-null field written on all three paths in services/workflows.py (success 278, TimeoutError 295, general failure 330); failures use a monotonic clock, so timeout durations are trustworthy. started_at/finished_at are also stored, and normalized_metadata.usage carries token counts on the same row, so tokens/sec is already derivable per attempt. Gaps: (1) sources_page.py:400 renders it raw as "27612 ms" rather than seconds; (2) it is only visible for the latest attempt of one source at a time - there is no rollup, so answering "which model is slow" required hand-written SQL against the database. A small model-performance rollup is a V4.7 candidate.
+
+#### [55] duration_ms measures end-to-end page processing, not provider latency
+
+*comment* - **open**
+
+services/workflows.py:221 sets monotonic_started_at BEFORE provider_input preparation (image normalization, artifact persistence, session.commit() at line 228), and line 251 computes elapsed_seconds from it. But the asyncio.wait_for timeout at lines 240-249 wraps ONLY _call_transcriber. So duration_ms covers a strictly wider window than the budget that governs it. Empirical proof: the three historical local_timeout rows recorded 20.4/20.8/22.0s against a 20.0s timeout, i.e. roughly 0.4-2.0s of non-provider work is folded in. Consequence: duration_ms cannot be used to isolate provider performance, and any model-performance rollup built on it would be polluted by preprocessing time that varies with image size. V4.7 candidate: record provider latency as a separate column, or move monotonic_started_at to just before the wait_for.
+
+### V4.7 planning
+
+#### [56] Release split agreed: V4.7 = architectural cleanup, V4.8 = features
+
+*comment* - `MED-14` - **answered**
+
+User asked whether the sources.py decomposition (architectural) should be separated from pan-zoom and photo work (features). Agreed and documented. Rationale: V4.6 succeeded because it had a binary gate - behavior-identical, suite unchanged. A refactor can be held to that standard; features cannot, since they require new tests. Bundling them destroys the ability to attribute a test delta to a bug versus expected new behavior. The two also touch disjoint trees under different instruction files (services vs ui). Created docs/ver4.7/scope_boundary_v4_7.md, docs/ver4.7/implementation_plan_v4_7.md, docs/ver4.8/feature_backlog_v4_8.md. All cross-links verified.
+
+#### [57] Rejected the original proposal to move update_job_source_transcription to workflows.py
+
+*deviation* - `MED-14` - **noted**
+
+The V4.6 Phase 5 deferral note suggested moving it as orchestration. Rejected in the V4.7 boundary. services.instructions.md:63-65 requires transcript updates and the paired terminal status change to commit or roll back together, and the method writes JobSource plus ExecutionAttempt in one session scope, deriving attempt_number from ExecutionAttempt at lines 595-600. Line 72 assigns session-aware write helpers to services and commit-boundary control to orchestration, so moving a multi-table write into workflows.py inverts the stated architecture. Scope reduced from three moves to two (artifacts.py, evidence.py); expected sources.py ~930 lines rather than the deeper cut originally implied.
+
+#### [58] Pan-zoom renumbered from V4.7 to V4.8, intent preserved
+
+*comment* - `HIGH-07` - **noted**
+
+Commit 6a3ee26 states pan-zoom would return "in V4.7 alongside the other photo/image work". It now sits in the V4.8 backlog. The commit intent was grouping with the photo work, not the specific number, and that grouping is preserved. Recorded in the V4.8 backlog so the git history is not silently contradicted. Also noted there: document_panzoom.py was exported but wired to no page, so no user has seen it - which makes reintroduction a new feature rather than a restoration, and is what puts it on the feature side of the split.
+
+#### [59] Service ownership model for ExecutionAttempt / ProcessingArtifact is undecided
+
+*question* - `MED-14` - **answered**
+
+services.instructions.md:11 says "1 service class per data model" but there are 10 persisted models and 6 service classes. The 4 unnamed models landed arbitrarily: DocumentPerson->people.py, and JobSource + ExecutionAttempt + ProcessingArtifact all -> sources.py. Line count tracks model count: jobs.py 438L (1 model), documents.py 473L (1+registry), people.py 554L (2+registry), sources.py 1389L (4). Evidence gathered 2026-08-18: both tables were added in V4.2 commit 6bd4cbb ("Updated what ai_raw_response data is being captured"), i.e. AFTER the 4-component design. ExecutionAttempt is docstringed "Immutable evidence for one provider call attempt" (request manifest, transport body, router ids, sdk snapshot, software context, timing); ProcessingArtifact is "Provider-neutral, versioned output derived from a Source" with a XOR CheckConstraint on inline vs external content, and a NULLABLE execution_attempt_id, so an artifact can exist with no attempt. Both are append-only provenance, not mutable domain entities. Four options were put to the user (evidence-as-own-subsystem; evidence split with ProcessingArtifact under Source; strict lifecycle into JobService/SourceService; draft the revised instructions first). USER DEFERRED - continuing the discussion interactively, formulating further questions. Do not proceed with V4.7 Phase 1/2 until this is settled, since the chosen model determines the module split.
+
+#### [60] V4.7 boundary overstates the case against moving update_job_source_transcription
+
+*deviation* - `MED-14` - **answered**
+
+The scope boundary as written says moving it to workflows.py would violate services.instructions.md. On re-reading, lines 38-47 (the _finalize contract: commit when service-owned, flush when caller-owned) and line 72 describe exactly the mechanism that makes a multi-service atomic write safe, so the document permits it. The honest objection is weaker: keeping the paired JobSource + ExecutionAttempt write in one method makes atomicity enforced by locality, whereas splitting it makes atomicity depend on every future caller sharing the session correctly. That is a robustness argument, not a rule violation. Correct the wording in docs/ver4.7/scope_boundary_v4_7.md section 1 before that document is treated as frozen.
+
+### V4.7 design - evidence model simplification
+
+#### [61] job_source duplicates execution_attempt columns byte-for-byte
+
+*comment* - `MED-14` - **answered**
+
+Measured on live DB: job_source.raw_transcription 77/77 identical to latest attempt; ai_metadata vs normalized_metadata 77/77; raw_api_response vs sdk_response_snapshot 77/77; error_detail 2/2. The table split itself is justified by cardinality (1:N attempts) but the 1:N is exercised in only 1 of 79 job_source rows. The 4 duplicated columns are an undocumented, unenforced denormalized cache.
+
+#### [62] status mismatch cross-confirms the enum persistence defect
+
+*risk* - `45` - **answered**
+
+job_source.status vs execution_attempt.status compared 0/79 identical - job_source stores lowercase (transcribed), execution_attempt stores uppercase (TRANSCRIBED). Independent confirmation of finding [45].
+
+#### [63] Orientation normalization was NOT a red herring - proven visually
+
+*comment* - `ProcessingArtifact` - **answered**
+
+59 of 79 source images carry EXIF orientation=3 (rotate 180). Rendered the exact page the user described (Pioneer Days page 00, a typed table of contents): raw decoded pixels are genuinely upside down; the 180-rotated version is upright. So sending raw bytes did send an inverted page to the model. resolve_provider_input (workflows.py:226 -> sources.py:850) is on the current hot path, so normalization now runs, but only 1 orientation artifact exists - the other 58 rotated pages were transcribed before normalization was wired.
+
+#### [64] job_source cannot be deleted - it is the work queue, not just a junction
+
+*risk* - `MED-14` - **answered**
+
+store.py:249,313 create JobSource with status=PENDING at job creation, before any provider call. workflows.py:438-440 selects pending work by status != TRANSCRIBED. jobs.py:411-424 retry mutates FAILED back to PENDING and clears fields. jobs.py:378-384 cancel writes FAILED/Cancelled by user with NO provider call, so no execution_attempt row could exist to carry it. An append-only table cannot express queued-not-yet-attempted or cancelled-before-call. Recommend STRIP not DELETE: keep (id, job_id, source_id, status); drop raw_transcription, ai_metadata, raw_api_response, executed_at.
+
+#### [65] Entire artifact subsystem has executed exactly once
+
+*comment* - `ProcessingArtifact` - **answered**
+
+Only 2 rows exist, both from the same job on 2026-08-16 (13:59:57 orientation, 14:00:07 quality). workflows.py:560-571 writes a transcription_quality_warnings artifact on EVERY successful page, yet 77 successful transcriptions produced 1 row - so the code path postdates nearly all data. Ingest already copies bytes via media_storage.py:57 write_bytes, so normalize-at-upload is viable. Caveat: Pillow re-encodes JPEG at quality 95, a permanent generational loss for an archival corpus - recommend retaining original bytes as a sibling file.
+
+#### [66] Retry history already exists in execution_attempt - no resubmitted flag needed
+
+*comment* - `MED-14` - **answered**
+
+ExecutionAttempt UniqueConstraint(job_id, source_id, attempt_number) at models.py:389 already implements keep-the-failed-row-and-add-a-new-one. Proven in live data: attempt 1 FAILED/local_timeout 20.4s and attempt 2 TRANSCRIBED 13.6s both retained. Adding a second job_source row would duplicate that and break the one-row-per-(job,page) assumption in read_job_source_for_job and sources.py:570-574, where uniqueness is enforced in CODE not by a DB constraint.
+
+#### [67] Add JobSourceStatus.CANCELLED to retire job_source.error_detail
+
+*comment* - `MED-14` - **answered**
+
+Cancel currently overloads FAILED plus free text Cancelled by user (jobs.py:378-384). A distinct CANCELLED status separates user cancellation from genuine provider failure and removes the last consumer of job_source.error_detail, reducing job_source from 9 columns to 4: id, job_id, source_id, status.
+
+#### [68] Lossless 180-degree JPEG rotation is viable for 57 of 58 rotated images
+
+*comment* - `ProcessingArtifact` - **answered**
+
+Pillow always round-trips through decoded pixels (normalization.py:76-86), so quality=95 re-encode loss is inherent to the library, not required by the task. A 180 rotation is expressible as a lossless DCT transform when both dimensions are multiples of the 16px MCU. Measured across the corpus: 57/58 qualify; the sole exception is 2306x2019. Alternative that needs no new dependency: normalize at upload and retain the original bytes as the archival master.
+
+#### [69] Quantization-table reuse beats both current settings and the lossless-DCT route
+
+*comment* - `ProcessingArtifact` - **answered**
+
+Measured single-generation rotate-and-restore on 5 rotated JPEGs. Current settings (quality=95, subsampling=0, normalization.py:84-85): PSNR 50.0-53.5 dB, file size +38 percent. Reusing the source quantization tables and subsampling (qtables=im.quantization, subsampling=JpegImagePlugin.get_sampling(im), optimize=True): PSNR 51.5-55.0 dB, max channel delta 7-9/255, file size slightly SMALLER (636KB->595KB). Better on quality and size simultaneously. Critically it works at any dimensions, so the 2306x2019 MCU-misaligned outlier needs no rejection path - the edge case only exists on the lossless-jpegtran route, which would also require an external C binary. Recommend Pillow with qtables reuse; drop the lossless-DCT option.
+
+#### [70] Evidence-model simplification decisions settled by user
+
+*comment* - `DECISIONS` - **answered**
+
+1) job_source is STRIPPED not deleted - keeps id, job_id, source_id, status (9 columns to 4). Drop raw_transcription, ai_metadata, raw_api_response, executed_at, error_detail. All evidence reads move to execution_attempt. 2) Add JobSourceStatus.CANCELLED so cancel no longer overloads FAILED plus free text, retiring error_detail. 3) Retry keeps its current FAILED-to-PENDING reset - history already lives in execution_attempt via UniqueConstraint(job_id, source_id, attempt_number). 4) PENDING-at-job-creation is unchanged. 5) ProcessingArtifact table REMOVED; orientation normalization moves to upload/ingest; transcription_quality_warnings payload folds into execution_attempt.normalized_metadata. 6) Rotation uses Pillow with qtables + subsampling reuse (visually lossless, ~52 dB PSNR, no size growth, no external dependency, no MCU rejection path). No archival master retained. 7) One-time backfill of the 58 already-ingested EXIF-orientation-3 images.
+
+## Related Local References
+
+- [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md) - finding IDs
+- [V4.6 Scope Boundary](scope_boundary_v4_6.md)
+- [V4.6 Implementation Plan](implementation_plan_v4_6.md)
+- [V4.7 Scope Boundary](../ver4.7/scope_boundary_v4_7.md)
+- [V4.7 Implementation Plan](../ver4.7/implementation_plan_v4_7.md)
+- [V4.8 Feature Backlog](../ver4.8/feature_backlog_v4_8.md)