generated from john/python-template
V4.7 & V4.8 planning: evidence model simplification and feature backlog
Plans the next two releases following the V4.6 architecture remediation. Documentation only - no code or schema changes. V4.7 is an architectural cleanup and evidence-model re-alignment release, scoped from measurements taken against the live database: - job_source and execution_attempt duplicate the same evidence. Measured 77/77 identical on raw_transcription, ai_metadata vs normalized_metadata, and raw_api_response vs sdk_response_snapshot. job_source is stripped to its original junction role plus queue state (9 columns -> 4); all evidence reads move to execution_attempt. - job_source is stripped rather than deleted because it is also the work queue: rows are created PENDING before any provider call, and cancellation writes a terminal state with no provider call at all. An append-only evidence table cannot express either. - JobSourceStatus.CANCELLED is added so cancellation stops overloading FAILED plus free text, which retires job_source.error_detail. This absorbs the dual-spelling fix [45], since both rewrite the same persistence. - ProcessingArtifact is removed. Two rows exist against 77 successful transcriptions, so the subsystem has effectively never run. Orientation normalization moves to ingest, where it is applied once and needs no derivative. - Orientation normalization itself is retained: 58 of 79 stored images carry EXIF orientation 3, and their raw decoded pixels are genuinely inverted. Rotation switches to quantization-table reuse, measured better than the current quality=95 settings on both fidelity (51.5-55.0 dB PSNR vs 50.0-53.5) and size (-6% vs +38%). - The planned services/artifacts.py extraction is cancelled. The cluster is deleted rather than moved, establishing a delete-before-refactor ordering. V4.8 parks feature work: pan and zoom, homepage gallery, multi-portrait support, image descriptions, and the model-performance rollup, which stays gated on the V4.7 run-time measurement fix. Co-authored-by: Copilot App <[email protected]>
This commit is contained in:
@@ -0,0 +1,201 @@
|
||||
# Implementation Plan (Version 4.7)
|
||||
|
||||
## Goal
|
||||
|
||||
Collapse the duplicated evidence model, remove the `ProcessingArtifact` subsystem and move orientation normalization to ingest, complete the `SourceService` decomposition deferred from V4.6 ([MED-14]), correct the run-time measurement window, and close the remaining correctness and tooling items opened during V4.6. No new user-facing behavior.
|
||||
|
||||
## Planning Status
|
||||
|
||||
**Frozen.** The boundary is [`scope_boundary_v4_7.md`](scope_boundary_v4_7.md). Feature work is parked in [`../ver4.8/feature_backlog_v4_8.md`](../ver4.8/feature_backlog_v4_8.md).
|
||||
|
||||
## Planning Constraints
|
||||
|
||||
- Every change traces to a finding ID or a V4.6 review-log entry.
|
||||
- `ruff check` clean and `ty check` at **0 diagnostics** at the end of every phase, matching the V4.6 exit state.
|
||||
- The full suite passes at the end of every phase.
|
||||
- **Test changes are expected in Phases 1 and 2.** This differs from V4.6, where the mechanical moves required no test logic changes. Measured blast radius: 33 references to the removed `job_source` evidence fields across 8 test files, and 10 `ProcessingArtifact` references across 3. Only Phase 4 retains the "no test logic changes" rule.
|
||||
- Use `.\.venv\Scripts\python.exe -m pytest` (the system interpreter has no packages).
|
||||
- Back up `data/transcription.db` **and** `data/documents/` before running any migration step. The image backfill rewrites files in place.
|
||||
- One phase, one commit.
|
||||
|
||||
## Expected Project Impact
|
||||
|
||||
| Area | Before | After |
|
||||
| :--- | :--- | :--- |
|
||||
| `job_source` columns | 9 | **4** - `id`, `job_id`, `source_id`, `status` |
|
||||
| `JobSourceStatus` on disk | two spellings across two tables | one spelling, plus a new `CANCELLED` member |
|
||||
| `processing_artifact` | table, model, ~283 lines of service code, 2 rows | removed |
|
||||
| Orientation normalization | derived per transcription, artifact-backed | applied once at ingest, no derivative |
|
||||
| Stored image rotation | `quality=95, subsampling=0`, +38% size | qtables reuse, ~6% smaller, higher PSNR |
|
||||
| `services/sources.py` | 1,389 lines, 4 domain models | ~900 lines, `Source` + `JobSource` |
|
||||
| `services/evidence.py` | does not exist | ~174 lines, `ExecutionAttempt` reads and export |
|
||||
| `services/artifacts.py` | planned | **cancelled** - deleted rather than extracted |
|
||||
| `duration_ms` | provider call + normalization + artifact write + commit | the operation the timeout governs |
|
||||
| Worker loop errors | every `Exception` logged and suppressed | programming errors distinguishable from provider faults |
|
||||
| Quality gate | local pre-commit only, inert until installed | enforced in CI |
|
||||
|
||||
## Migration Handling
|
||||
|
||||
All schema and data changes are delivered by a single idempotent `tools/migrate_v46_to_v47.py`, following the `tools/migrate_v45_to_v46.py` conventions: never invoked at startup, never run by the test suite.
|
||||
|
||||
The tool is **built incrementally** - Phase 1 creates it with its own step, Phase 2 appends the next - and is **run at the end of each of those phases** so the live database stays usable at every phase boundary. Idempotency is what makes re-running safe.
|
||||
|
||||
Steps, in execution order:
|
||||
|
||||
1. Rotate the 58 stored images carrying EXIF orientation 3, in place, using qtables reuse; strip the orientation tag.
|
||||
2. Drop the `processing_artifact` table and remove its external artifact files.
|
||||
3. Normalize `execution_attempt.status` to the single declared spelling ([45]).
|
||||
4. Drop `job_source.raw_transcription`, `ai_metadata`, `raw_api_response`, `executed_at`, `error_detail`.
|
||||
|
||||
`tools/migrate_v45_to_v46.py` deliberately accepts both enum spellings because it reads historical backups. Leave that tolerance in place.
|
||||
|
||||
## Implementation Phases
|
||||
|
||||
### 1. Ingest Normalization and ProcessingArtifact Removal
|
||||
|
||||
Deletion comes first, so that Phase 4 never restructures code that is on its way out.
|
||||
|
||||
Tasks:
|
||||
|
||||
1. Move orientation normalization into the ingest path in `media_storage`, ahead of `write_bytes` (`media_storage.py:57`). Rotate, strip the EXIF orientation tag, then store.
|
||||
2. Change the encode settings at `normalization.py:84-85` from `quality=95, subsampling=0` to `qtables=im.quantization`, `subsampling=JpegImagePlugin.get_sampling(im)`, `optimize=True`. Import `JpegImagePlugin` explicitly - it is not reachable as an attribute of `PIL.Image`.
|
||||
3. Delete `resolve_provider_input` and its call at `workflows.py:226`. The stored file is now already upright, so the transcription path reads it directly.
|
||||
4. Fold the `transcription_quality_warnings` payload (`workflows.py:560-571`) into `execution_attempt.normalized_metadata`.
|
||||
5. Delete the artifact cluster from `sources.py` (lines 732-1015) and the artifact branch of `build_evidence_export`.
|
||||
6. Delete the `ProcessingArtifact` model and the `CheckConstraint`.
|
||||
7. Remove the "Orientation normalized" badge at `sources_page.py:174-178`, the artifact evidence dump at line 417, and the quality-warnings render at line 661.
|
||||
8. Update `test_normalization.py`, `test_v42_evidence.py`, and `test_db.py`. Normalization tests should now assert on ingest behavior rather than on artifact creation.
|
||||
9. Create `tools/migrate_v46_to_v47.py` with steps 1 and 2. Back up, run, verify.
|
||||
|
||||
Verification: re-check EXIF orientation across `data/documents/` - no stored image should report orientation 3, 6, or 8. Spot-check one backfilled page visually.
|
||||
|
||||
Exit: suite green, `ty check` at 0, `processing_artifact` gone from schema and code.
|
||||
|
||||
### 2. Evidence Model Simplification
|
||||
|
||||
Tasks:
|
||||
|
||||
1. Add `JobSourceStatus.CANCELLED`. Update `jobs.py:378-384` to write it instead of `FAILED` plus `"Cancelled by user"`.
|
||||
2. Declare one spelling for `JobSourceStatus` across both `job_source.status` and `execution_attempt.status` ([45]). `job_source.status` already declares `values_callable`; `execution_attempt.status` does not.
|
||||
3. Remove `raw_transcription`, `ai_metadata`, `raw_api_response`, `executed_at`, and `error_detail` from the `JobSource` model.
|
||||
4. Redirect every read to `execution_attempt`:
|
||||
- `transcript.py:103-119` sorts by `executed_at` - sort by `ExecutionAttempt.finished_at`.
|
||||
- `sources_page.py:390-393` reads `ai_metadata` and `raw_api_response`.
|
||||
- `sources_page.py:337-375` renders status, executed time, and error detail.
|
||||
- `models.py:266-278` and `models.py:328-343` derive transcript and error from `job_sources`.
|
||||
5. Shrink `update_job_source_transcription` (`sources.py:524-679`) to write only the surviving `JobSource` columns. Keep the method in `sources.py` and keep both writes in one session scope.
|
||||
6. Confirm `jobs.py:411-424` retry still works unchanged. It resets `FAILED` to `PENDING`; the attempt history it appears to discard is preserved by `ExecutionAttempt`'s unique constraint.
|
||||
7. Confirm `workflows.py:432-442` work selection is unaffected. It filters `status != TRANSCRIBED` within `job.job_sources`, so `CANCELLED` pages are excluded from a re-run only if that is the intent - **decide explicitly** whether cancelled pages should be re-attempted, and encode the answer in the filter rather than leaving it implicit.
|
||||
8. Update the 33 affected test references across the 8 files identified.
|
||||
9. Append migration steps 3 and 4. Back up, run, verify row counts before and after.
|
||||
|
||||
Exit: suite green, `ty check` at 0, `job_source` at 4 columns.
|
||||
|
||||
### 3. Stage B - Extract `services/evidence.py` ([MED-14])
|
||||
|
||||
Read-side only, now applied to a substantially smaller `sources.py`.
|
||||
|
||||
Move:
|
||||
|
||||
- `read_latest_execution_attempt` (216-245), including the `LatestExecutionAttempt` read model
|
||||
- `promote_machine_attempt` (679-710)
|
||||
- `list_execution_attempts` (710-732)
|
||||
- `build_evidence_export` (1015-1107)
|
||||
|
||||
Tasks:
|
||||
|
||||
1. Create `services/evidence.py` with an `EvidenceService(ServiceBase)` following the `DocumentService` conventions.
|
||||
2. Move the methods and the `LatestExecutionAttempt` dataclass verbatim. Preserve signatures, keyword-only arguments, error types, and `_session_scope` usage exactly.
|
||||
3. Preserve every explicit `selectinload()` chain. **Chain, never varargs** - `selectinload(A.b).selectinload(B.c)` and `selectinload(A.b, B.c)` produce an identical `.path` but are not equivalent, and under the `lazy="raise"` default set in V4.6 the varargs form raises at render time. See `db/loading.py`.
|
||||
4. Update `ServiceBundle` to construct and expose the new service via the `from_session_factory` constructor added in V4.6.
|
||||
5. Update call sites in `sources_page.py` and `workflows.py`.
|
||||
6. Run `ruff check --fix` **in the same pass** as the import edits - autofix removes imports that are unused at that moment.
|
||||
|
||||
Note that line numbers above are pre-Phase-1 positions and will have shifted. Locate by symbol, not by line.
|
||||
|
||||
Exit: suite green **with no test logic changes** beyond import paths, `ty check` at 0. Walk every `/ui/*` page and confirm a 200, since `lazy="raise"` turns a missed eager load into a runtime error rather than a slow query.
|
||||
|
||||
### 4. Run-Time Measurement Window (review log [55])
|
||||
|
||||
Tasks:
|
||||
|
||||
1. In `workflows.py`, make the recorded duration cover only the operation the `wait_for` at lines 240-249 governs. Either move `monotonic_started_at` (line 221) to immediately before the `wait_for`, or capture provider latency separately and record that.
|
||||
2. Apply the same treatment to all three write sites: success (line 278), `TimeoutError` (line 295), and general failure (line 330). The failure paths must keep using the monotonic clock.
|
||||
3. If preprocessing time is still worth keeping, record it as its own value rather than folding it into `duration_ms`.
|
||||
4. Update `sources_page.py:400`, which renders the raw integer as `"27612 ms"`.
|
||||
|
||||
Phase 1 already removes normalization and the artifact write from this window, which narrows the gap but does not close it - the `session.commit()` at line 228 remains inside it.
|
||||
|
||||
Verification: a recorded timeout duration should sit at or just under the configured budget, not 0.4-2.0 s above it as in the three historical `local_timeout` rows.
|
||||
|
||||
### 5. Worker Exception Handling (review log [8])
|
||||
|
||||
`worker.py:96-106`.
|
||||
|
||||
Tasks:
|
||||
|
||||
1. Separate genuinely retriable faults from programming errors. `classify_unexpected_error` is already called at line 101 and its result is currently only logged.
|
||||
2. Ensure a non-retriable error reaches a terminal state instead of being retried.
|
||||
3. Keep terminal-state and retry persistence atomic per `services.instructions.md:63-65`.
|
||||
4. Add a test that a deliberate programming error in the loop does not silently retry.
|
||||
|
||||
Context: with `WORKER_MAX_RETRIES=1` and a 30 s timeout the worst-case silent burn is 60 s, down from 360 s, so this is no longer urgent - but it remains the real fix behind that risk.
|
||||
|
||||
### 6. CI Enforcement ([HIGH-06], review log [40])
|
||||
|
||||
Tasks:
|
||||
|
||||
1. Add a workflow under `.github/workflows/` running `ruff check`, `ty check`, and `pytest` on push and pull request.
|
||||
2. Use the same commands as `.pre-commit-config.yaml` so local and CI gates cannot drift.
|
||||
3. Confirm the 4 tests that skip without `OPENROUTER_API_KEY` skip cleanly in CI rather than failing.
|
||||
4. Negative-test the workflow by pushing a deliberate lint error on a scratch branch.
|
||||
|
||||
## Sequencing Constraints
|
||||
|
||||
- **Phase 1 before Phase 3.** Code scheduled for deletion is never extracted first. This is why the previously planned `services/artifacts.py` is cancelled.
|
||||
- **Phase 1 before Phase 2.** Both touch `workflows.py` write paths; separating them keeps any regression attributable.
|
||||
- **Phase 4 before any V4.8 telemetry work.** A model-performance rollup built on the current `duration_ms` would chart preprocessing mixed with provider latency.
|
||||
|
||||
## Test Strategy
|
||||
|
||||
- **Phase 1:** normalization tests move from asserting artifact creation to asserting ingest behavior. Add a test that a stored image never retains EXIF orientation 3, 6, or 8.
|
||||
- **Phase 2:** assert that evidence reads resolve through `execution_attempt`; assert `CANCELLED` is distinguishable from `FAILED`; verify both status columns round-trip identically and existing rows read back correctly after the fix-up.
|
||||
- **Phase 3:** the suite passes with **no test logic changes**. Only import paths update. A required behavioral change signals the move was not mechanical - stop and re-examine.
|
||||
- **Phase 4:** assert the recorded duration is bounded by the configured timeout.
|
||||
- **Phase 5:** new test that a programming error does not silently retry.
|
||||
- **Phase 6:** CI must fail on an injected lint error.
|
||||
|
||||
## Risks
|
||||
|
||||
| Risk | Mitigation |
|
||||
| :--- | :--- |
|
||||
| The image backfill corrupts originals - it rewrites files in place with no archival master | Back up `data/documents/` before running; idempotent step keyed on the EXIF tag so a second run is a no-op; visually spot-check a backfilled page |
|
||||
| Dropping `job_source` columns loses data that turns out not to be duplicated | Verified 77/77 identical on all three evidence columns before dropping; re-run that comparison inside the migration and abort on any mismatch |
|
||||
| A read still expects a removed `job_source` column and fails only at render time | Grep-driven checklist in Phase 2 task 4; walk every `/ui/*` page after the phase |
|
||||
| Cancelled pages are silently re-attempted, or silently never re-attempted | Phase 2 task 7 forces an explicit decision in the work-selection filter |
|
||||
| A moved query loses an eager load and trips `lazy="raise"` at render time | Preserve `selectinload` chains verbatim; walk every `/ui/*` page after Phase 3 |
|
||||
| `ruff check --fix` deletes an import mid-move | Edit imports and usages in the same pass, as in V4.6 |
|
||||
| Circular imports between `sources.py` and `evidence.py` | Composition is sanctioned by `services.instructions.md:75-77`; keep the dependency one-directional |
|
||||
| Scope creep into refactoring `update_job_source_transcription` | Out of scope; the boundary records why |
|
||||
|
||||
## Delivery Order
|
||||
|
||||
1. Ingest normalization and `ProcessingArtifact` removal
|
||||
2. Evidence model simplification
|
||||
3. Stage B - `evidence.py`
|
||||
4. Measurement window
|
||||
5. Worker exception handling
|
||||
6. CI enforcement
|
||||
|
||||
## Done Criteria
|
||||
|
||||
Every item in the scope boundary's Acceptance Criteria is satisfied, the suite is green, `ty check` reports 0 diagnostics, and no user-facing behavior has changed.
|
||||
|
||||
## Related Local References
|
||||
|
||||
- [V4.7 Scope Boundary](scope_boundary_v4_7.md)
|
||||
- [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md)
|
||||
- [V4.6 Implementation Plan](../ver4.6/implementation_plan_v4_6.md)
|
||||
- [V4.8 Feature Backlog](../ver4.8/feature_backlog_v4_8.md)
|
||||
- `.github/instructions/services.instructions.md`
|
||||
- `src/transcription/db/loading.py` - the `selectinload` varargs trap
|
||||
@@ -0,0 +1,191 @@
|
||||
# V4.7 Scope Boundary
|
||||
|
||||
This document defines the frozen boundary for V4.7, an **architectural cleanup and evidence-model re-alignment release**. V4.6 remains the behavioral baseline. V4.7 introduces **no new user-facing features**; it completes the structural work V4.6 deferred, simplifies the evidence model down to what the application actually uses, and closes the correctness items opened during V4.6 implementation.
|
||||
|
||||
Every item in scope is traceable either to a review finding ID in [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md) or to a numbered entry in the V4.6 implementation review log. Any change that cannot be traced to one of those is out of scope.
|
||||
|
||||
All image *presentation*, media, and telemetry-presentation work is deferred to V4.8. See [`ver4.8/feature_backlog_v4_8.md`](../ver4.8/feature_backlog_v4_8.md).
|
||||
|
||||
## Purpose
|
||||
|
||||
- Collapse the duplicated evidence model so that `job_source` records **membership and queue state** and `execution_attempt` records **evidence**, with no overlap.
|
||||
- Remove the `ProcessingArtifact` subsystem, which has executed exactly once in the application's history, and move orientation normalization to ingest where it belongs.
|
||||
- Complete [MED-14] by decomposing `SourceService`, which still owns four domain models.
|
||||
- Correct the run-time measurement window so provider latency can be trusted before anything is built on top of it.
|
||||
- Stop the worker from silently swallowing programming errors.
|
||||
- End the dual-spelling persistence of `JobSourceStatus`.
|
||||
- Make the `ruff` / `ty` gate enforceable in CI rather than only on a developer machine that has run `pre-commit install`.
|
||||
|
||||
## Confirmed Operating Context
|
||||
|
||||
These answers are frozen for V4.7 and govern every decision below.
|
||||
|
||||
| Question | Answer |
|
||||
| :--- | :--- |
|
||||
| Database | **SQLite only.** PostgreSQL remains the intended destination. The V4.6 re-level already resolved the FK cycle with `use_alter=True`. |
|
||||
| Topology | **Single user, single process, single worker.** Unchanged from V4.6. |
|
||||
| Schema evolution | **Re-level from current metadata**, exactly as V4.6. No Alembic, no `_upgrade_*` chain. |
|
||||
| Existing data | The live database is populated. V4.7 is **schema-affecting**: three structural changes plus a one-time image backfill, delivered by a single `tools/migrate_v46_to_v47.py`. |
|
||||
| Release character | **Architectural cleanup and evidence-model re-alignment.** No new features. |
|
||||
| Image fidelity | **Visually lossless is sufficient.** Measured at 51.5-55.0 dB PSNR for a single re-encode generation. Bit-exact preservation was considered and rejected as unnecessary complexity. |
|
||||
| Provider settings | `WORKER_PROVIDER_TIMEOUT_SECONDS=30.0`, `WORKER_MAX_RETRIES=1`, calibrated 2026-08-18. Not revisited in V4.7. |
|
||||
|
||||
## Evidence Gathered
|
||||
|
||||
The decisions below rest on measurements taken against the live database on 2026-08-18, not on inspection alone.
|
||||
|
||||
| Measurement | Result |
|
||||
| :--- | :--- |
|
||||
| `job_source.raw_transcription` vs latest attempt | **77/77 identical** |
|
||||
| `job_source.ai_metadata` vs `normalized_metadata` | **77/77 identical** |
|
||||
| `job_source.raw_api_response` vs `sdk_response_snapshot` | **77/77 identical** |
|
||||
| `job_source.error_detail` vs attempt `error_detail` | 2/2 identical |
|
||||
| `job_source.status` vs `execution_attempt.status` | 0/79 textually identical - the dual-spelling defect [45] |
|
||||
| `job_source` rows with more than one attempt | 1 of 79 |
|
||||
| `processing_artifact` rows in existence | **2**, both from one job on 2026-08-16, against 77 successful transcriptions |
|
||||
| Source images carrying EXIF orientation 3 | **58 of 79**, of which only 1 was ever normalized |
|
||||
|
||||
## In Scope
|
||||
|
||||
### 1. Evidence Model Simplification
|
||||
|
||||
`job_source` began as the many-to-many link between `job` and `source` and accreted response-capture fields over time. `execution_attempt`, added later in V4.2 (commit `6bd4cbb`), captures the same information in more detail. The measurements above show the overlap is total, not partial.
|
||||
|
||||
**`job_source` is stripped, not deleted.** It cannot be folded into `execution_attempt`, because it carries state that exists when no provider call has occurred:
|
||||
|
||||
- `store.py:249,313` create rows with `status=PENDING` **at job creation**, before any call.
|
||||
- `workflows.py:432-442` selects work by `status != TRANSCRIBED` on `job.job_sources`.
|
||||
- `jobs.py:378-384` cancel writes a terminal state with **no provider call at all**, so no attempt row could carry it.
|
||||
|
||||
An append-only evidence table cannot express "queued, not yet attempted" or "cancelled before any call". The junction survives; the duplicated evidence does not.
|
||||
|
||||
**Retained:** `id`, `job_id`, `source_id`, `status`.
|
||||
|
||||
**Removed:** `raw_transcription`, `ai_metadata`, `raw_api_response`, `executed_at`, `error_detail`.
|
||||
|
||||
**Added:** `JobSourceStatus.CANCELLED`, so cancellation stops overloading `FAILED` plus the free-text string `"Cancelled by user"`. This is what retires `error_detail`.
|
||||
|
||||
**Unchanged:** the retry reset at `jobs.py:411-424`. Flipping `FAILED` back to `PENDING` loses no history, because `ExecutionAttempt`'s `UniqueConstraint(job_id, source_id, attempt_number)` (`models.py:389`) already preserves every prior attempt. This is confirmed in live data: attempt 1 `FAILED`/`local_timeout` and attempt 2 `TRANSCRIBED` are both retained. Adding a second `job_source` row per retry would duplicate that mechanism 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 database constraint**.
|
||||
|
||||
This item absorbs review log [45], since both changes rewrite `JobSourceStatus` persistence and must land as one migration.
|
||||
|
||||
### 2. ProcessingArtifact Removal and Ingest Normalization
|
||||
|
||||
`ProcessingArtifact` is a generic container for derived data products, with a `CheckConstraint` enforcing that content is either inline JSON or an external file, never both. Two rows exist. The quality-warnings path at `workflows.py:560-571` writes one on **every** successful page, yet 77 successful transcriptions produced a single row, so the subsystem postdates nearly all data and has effectively never run.
|
||||
|
||||
Orientation normalization itself is **not** dispensable and was **not** a red herring. 58 of 79 stored images carry EXIF orientation 3, and the raw decoded pixels of the page that prompted the original investigation are genuinely upside down. Sending those bytes unrotated sends an inverted page to the model.
|
||||
|
||||
The fix is to normalize at ingest rather than derive at transcription time:
|
||||
|
||||
- Rotate on upload, in `media_storage`, before the image is stored. Every stored byte is then already upright and no derivative needs to exist.
|
||||
- Use Pillow with `qtables=im.quantization`, `subsampling=JpegImagePlugin.get_sampling(im)`, `optimize=True`. Measured against the current `quality=95, subsampling=0` settings at `normalization.py:84-85`, this is **better on both axes**: 51.5-55.0 dB PSNR versus 50.0-53.5 dB, and roughly 6% smaller output versus 38% larger.
|
||||
- Strip the EXIF orientation tag after rotating.
|
||||
- No archival master is retained. No external `jpegtran` dependency is introduced. No MCU-alignment rejection path is needed, because Pillow handles any dimensions - including the single 2306x2019 outlier.
|
||||
|
||||
Then delete: the `processing_artifact` table, the `ProcessingArtifact` model, the ~283-line artifact cluster in `sources.py` (lines 732-1015), `resolve_provider_input`, and the artifact branch of `build_evidence_export`. The `transcription_quality_warnings` payload folds into `execution_attempt.normalized_metadata`.
|
||||
|
||||
Deleting stored images is not involved; the 58 already-ingested rotated images are rotated **in place** by the migration. No live integrity check is invalidated: `Source` has no digest column, and the only stored digests are `ExecutionAttempt.request_manifest_sha256` - a hash of the request manifest, correct as history - and `ProcessingArtifact.payload_sha256`, which is removed with the table.
|
||||
|
||||
### 3. SourceService Decomposition ([MED-14])
|
||||
|
||||
`services/sources.py` is **1,389 lines** and `SourceService` owns `Source`, `JobSource`, `ExecutionAttempt`, and `ProcessingArtifact`.
|
||||
|
||||
Item 2 removes the `ProcessingArtifact` responsibility by **deletion rather than extraction**. The previously planned `services/artifacts.py` is therefore cancelled - extracting ~283 lines into a new module and then deleting that module would be wasted work.
|
||||
|
||||
What remains is the `ExecutionAttempt` cluster, moved to **`services/evidence.py` (~174 lines)**: `read_latest_execution_attempt` (216-245) with its `LatestExecutionAttempt` read model, `promote_machine_attempt` (679-710), `list_execution_attempts` (710-732), and `build_evidence_export` (1015-1107).
|
||||
|
||||
**`update_job_source_transcription` stays in `sources.py`.** The V4.6 deferral note proposed moving it to `workflows.py` as orchestration; that proposal is not adopted. The method writes `JobSource` and `ExecutionAttempt` inside one session scope and derives `attempt_number` at lines 595-600, and `services.instructions.md:63-65` requires the transcript update and the paired terminal status change to commit or roll back together. `services.instructions.md:72` assigns session-aware write helpers to services and commit-boundary control to orchestration, so the current placement already satisfies the instruction file. Splitting the two writes across modules is the most plausible way that atomicity later gets broken. The method will shrink under item 1, since several of the fields it writes cease to exist.
|
||||
|
||||
Expected result: `sources.py` lands near **900 lines**.
|
||||
|
||||
### 4. Run-Time Measurement Window (review log [55])
|
||||
|
||||
`services/workflows.py:221` sets `monotonic_started_at` **before** provider-input preparation and the `session.commit()` at line 228. Line 251 computes `elapsed_seconds` from it. But the `asyncio.wait_for` timeout at lines 240-249 wraps **only** `_call_transcriber`.
|
||||
|
||||
`duration_ms` therefore measures a strictly wider window than the budget that governs it. This is observable in the migrated data: three historical `local_timeout` rows recorded 20.4 / 20.8 / 22.0 s against a 20.0 s timeout.
|
||||
|
||||
In scope: either record provider latency as a distinct value, or move `monotonic_started_at` to immediately before the `wait_for`. Whichever is chosen, the resulting figure must be the quantity the timeout actually governs. Item 2 also removes normalization from this window entirely, which shrinks the discrepancy but does not by itself fix it.
|
||||
|
||||
This item **must land before any V4.8 telemetry presentation work**.
|
||||
|
||||
### 5. Worker Exception Handling (review log [8])
|
||||
|
||||
`worker.py:96-106`, `handle_worker_exceptions`, catches bare `Exception`, logs it, and suppresses it. A programming error inside the worker loop is therefore indistinguishable from a transient provider fault and is retried silently with no UI signal.
|
||||
|
||||
In scope: distinguish genuinely retriable faults from programming errors, and ensure a non-retriable error surfaces rather than looping. Retry counting and terminal-state transitions remain governed by `services.instructions.md:63-65`.
|
||||
|
||||
### 6. CI Enforcement of the Quality Gate ([HIGH-06], review log [40])
|
||||
|
||||
`.github/workflows/` is empty. The `ruff check` and `ty check` gate established in V4.6 Phase 7 exists only in `.pre-commit-config.yaml`, which is inert until a developer runs `pre-commit install`.
|
||||
|
||||
In scope: a CI workflow running `ruff check`, `ty check`, and `pytest` on push and pull request, using the same commands as the local hooks so the two cannot drift.
|
||||
|
||||
## Out of Scope
|
||||
|
||||
- **All image and media presentation work.** Pan and zoom on Source Detail, the homepage gallery, multi-portrait support, image descriptions, and background wallpaper are V4.8.
|
||||
- **The model-performance rollup** (review log [54]). It depends on item 4 and is a new user-facing view.
|
||||
- **Reducing `update_job_source_transcription`.** See section 3.
|
||||
- **Bit-exact image preservation.** Considered and rejected; see Confirmed Operating Context.
|
||||
- **PostgreSQL cutover.**
|
||||
- **Re-tuning `WORKER_PROVIDER_TIMEOUT_SECONDS` or `WORKER_MAX_RETRIES`.** Calibrated 2026-08-18 against measured per-model durations.
|
||||
- **Removing slow models from `PROVIDER_MODELS`** (review log [53]). A configuration judgement, deliberately left with the operator.
|
||||
- **Any new feature.**
|
||||
|
||||
## Locked Design Decisions
|
||||
|
||||
### A. Cleanup Only
|
||||
|
||||
V4.7 changes structure and correctness. It does not change what the application does for a user. If a change would be visible on a page as new capability, it belongs in V4.8.
|
||||
|
||||
### B. One Home Per Fact
|
||||
|
||||
After V4.7, any given piece of evidence is stored in exactly one place. `job_source` holds membership and state; `execution_attempt` holds evidence. Denormalized convenience copies are not reintroduced, and if a read becomes awkward the fix is a query or a read model, not a duplicated column.
|
||||
|
||||
### C. Delete Before Refactor
|
||||
|
||||
Item 2 deletes the artifact subsystem before item 3 restructures what remains. Code scheduled for deletion is never extracted, renamed, or moved first.
|
||||
|
||||
### D. Simplicity Over Edge-Case Management
|
||||
|
||||
Where two approaches both satisfy the requirement, the one with fewer moving parts wins. This is why rotation uses Pillow rather than a lossless DCT transform, and why no archival master is kept.
|
||||
|
||||
### E. Measurement Before Presentation
|
||||
|
||||
Item 4 precedes all V4.8 telemetry work. A dashboard built on a conflated metric looks authoritative and quietly misleads.
|
||||
|
||||
### F. One Migration, Backed Up
|
||||
|
||||
All schema and data changes land in a single `tools/migrate_v46_to_v47.py`: idempotent, never invoked at startup, never run by the test suite, following the `tools/migrate_v45_to_v46.py` conventions. `data/transcription.db` **and** `data/documents/` are backed up before it runs, because the image backfill rewrites files in place.
|
||||
|
||||
### G. The Instruction Files Are the Standard
|
||||
|
||||
`.github/instructions/services.instructions.md` and `ui.instructions.md` govern. Where this document and an instruction file disagree, the instruction file wins.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- `job_source` carries exactly `id`, `job_id`, `source_id`, `status`; every evidence read resolves through `execution_attempt`.
|
||||
- `JobSourceStatus.CANCELLED` exists and cancellation no longer writes free text into a removed column.
|
||||
- `job_source.status` and `execution_attempt.status` persist with one spelling, and existing rows are consistent.
|
||||
- The `processing_artifact` table, its model, and its service cluster no longer exist.
|
||||
- Newly uploaded images are stored upright with no EXIF orientation tag, and the 58 pre-existing rotated images have been backfilled.
|
||||
- `services/sources.py` is materially smaller, with `ExecutionAttempt` responsibilities in `services/evidence.py` and `update_job_source_transcription` unmoved.
|
||||
- The recorded duration reflects only the operation the timeout governs.
|
||||
- A programming error in the worker loop is distinguishable from a provider fault.
|
||||
- `ruff check` reports no findings; `ty check` reports **0 diagnostics**, the V4.6 exit state.
|
||||
- The full test suite passes.
|
||||
- CI runs the same `ruff` / `ty` / `pytest` gate as the local hooks.
|
||||
- No new user-facing behavior.
|
||||
|
||||
## Scope Freeze Gate
|
||||
|
||||
This boundary is frozen. Adding an item requires a finding ID or a review-log entry, and an explicit note recording the addition.
|
||||
|
||||
## Related Local References
|
||||
|
||||
- [V4.7 Implementation Plan](implementation_plan_v4_7.md)
|
||||
- [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md) - finding IDs
|
||||
- [V4.6 Scope Boundary](../ver4.6/scope_boundary_v4_6.md) - the baseline this release builds on
|
||||
- [V4.6 Implementation Plan](../ver4.6/implementation_plan_v4_6.md)
|
||||
- [V4.8 Feature Backlog](../ver4.8/feature_backlog_v4_8.md) - where deferred feature work is parked
|
||||
- `.github/instructions/services.instructions.md`
|
||||
- `.github/instructions/ui.instructions.md`
|
||||
@@ -0,0 +1,111 @@
|
||||
# V4.8 Feature Backlog
|
||||
|
||||
**Status: not scoped.** This is a parking document, not a frozen boundary. It records feature work deferred out of V4.6 and V4.7 together with the evidence gathered so far, so that scoping V4.8 does not start from a blank page.
|
||||
|
||||
V4.8 is the first release since V4.5 to add **new user-facing behavior**. V4.6 was pure remediation and V4.7 is architectural cleanup; both were held to "no new features." That constraint ends here, which means V4.8 needs a different verification gate: V4.6 and V4.7 could be validated by "the suite still passes unchanged," and V4.8 cannot.
|
||||
|
||||
## Dependency on V4.7
|
||||
|
||||
**The model-performance rollup below must not begin until V4.7 Phase 4 lands.** `duration_ms` currently measures provider call *plus* image normalization, artifact persistence, and a DB commit, while the timeout governs only the provider call. A rollup built on it would chart preprocessing time mixed with provider latency and look authoritative while quietly misleading. V4.7 Phase 1 removes normalization and artifact persistence from that window, but the commit remains inside it until Phase 4. See [V4.7 scope boundary section 4](../ver4.7/scope_boundary_v4_7.md).
|
||||
|
||||
## Candidate Features
|
||||
|
||||
### 1. Pan and Zoom on Source Detail
|
||||
|
||||
**Practicality: high. Effort: S.**
|
||||
|
||||
`ui/components/document_panzoom.py` existed and was **deleted in V4.6 Phase 5** (`6a3ee26`) because it was exported but wired to no page. It is 136 lines and recoverable:
|
||||
|
||||
```
|
||||
git show 6a3ee26^:src/transcription/ui/components/document_panzoom.py
|
||||
```
|
||||
|
||||
It already handled both images and PDFs (the latter via an iframe).
|
||||
|
||||
Two things must change on reintroduction - this is not a straight revert:
|
||||
|
||||
- It loaded Panzoom from the **unpkg CDN**. For an archival application the library should be vendored locally, otherwise the viewer breaks offline and depends on a third party staying available.
|
||||
- It carried its own `_document_url()` helper. V4.6 Phase 5 extracted exactly that logic into `ui/components/media_urls.py` as `resolve_media_url`. Reintroducing the old helper would recreate the duplication Phase 5 removed.
|
||||
|
||||
Scope note: apply it to **Source Detail only**. `dark_room_viewer` (`ui/components/viewers.py`) is shared by four pages - `sources_page.py:268`, `home_page.py:25` and `:88`, `people_page.py:453`, `documents_page.py:524` - so a flag on it would leak pan-zoom into the homepage and document detail, which is not wanted. Add a separate component and use it only at `sources_page.py:268`.
|
||||
|
||||
Numbering note: the Phase 5 commit message states pan-zoom would return "in V4.7 alongside the other photo/image work." Moving it to V4.8 preserves that **intent** - it stays grouped with the photo work - and changes only the release number.
|
||||
|
||||
### 2. Homepage Image Gallery
|
||||
|
||||
**Practicality: high. Effort: S. Recommended first feature.**
|
||||
|
||||
The storage layer is already built:
|
||||
|
||||
- `ui/homepage_store.py:82` `list_homepage_images()` already returns **every** stored image, sorted by modification time.
|
||||
- `store_homepage_image()` already accumulates files rather than overwriting.
|
||||
- Today the UI calls only `latest_homepage_image()` and displays one image. `list_homepage_images()` is currently exercised **only by tests**.
|
||||
|
||||
So multi-image upload is effectively done; what is missing is presentation. NiceGUI 3.13.0 provides `ui.carousel` for left/right navigation and `ui.timer` for rotation.
|
||||
|
||||
Sub-items:
|
||||
|
||||
- Multi-image display with left/right navigation - small, mostly wiring.
|
||||
- Optional slideshow rotating every ~10 minutes.
|
||||
|
||||
**Performance caveat:** `list_homepage_images()` performs a directory scan with a `stat()` per file on every call, and `home_page.py` already performs blocking I/O in the page handler (V4.6 review log [25], which was deliberately left alone). A rotating timer that re-enumerates on every tick would repeat that scan indefinitely. Enumerate once at page load and cache the list.
|
||||
|
||||
### 3. Multiple Person Portraits
|
||||
|
||||
**Practicality: medium. Effort: M/L. Defer behind item 2.**
|
||||
|
||||
`Person.portrait_path` is a **single string column**. Supporting multiple portraits requires a new table, a data migration, and upload UI - a materially larger job than item 2, and a different one.
|
||||
|
||||
### 4. Image Descriptions
|
||||
|
||||
**Practicality: medium, conditional. Effort: M.**
|
||||
|
||||
Homepage images are **filesystem-only with no metadata store**, so a caption has nowhere to live today. This needs either a sidecar JSON file or a real table.
|
||||
|
||||
This is cheap **only if** item 3 is being done at the same time, since both need the same metadata layer. Designing that layer twice would be wasteful; design it once or not at all.
|
||||
|
||||
### 5. Model-Performance Rollup (V4.6 review log [54])
|
||||
|
||||
**Practicality: high, but blocked. Effort: M.**
|
||||
|
||||
Run-time telemetry is already captured and is per page: `execution_attempt.duration_ms` is a required non-null field written on all three paths in `workflows.py` (success 278, `TimeoutError` 295, general failure 330), with failures using a monotonic clock. Verified against the live database: 80 rows across 80 distinct (job, source, attempt) combinations, one row per page - the largest job has 60 attempts across 60 distinct pages - and zero nulls. Token counts live on the same row in `normalized_metadata.usage`, so tokens-per-second is already derivable without a join.
|
||||
|
||||
What is missing is **aggregation**. The figure is visible only for the latest attempt of one source at a time (`sources_page.py:400`), rendered raw as `"27612 ms"`. There is no rollup by model, prompt, or document.
|
||||
|
||||
The gap is concrete: calibrating the provider timeout on 2026-08-18 required hand-written SQL against the database, because the application could not answer "which model is slow."
|
||||
|
||||
Proposed shape: median / p95 / max duration, tokens per second, and a timeout rate, grouped by model. **Blocked on V4.7 Phase 4.**
|
||||
|
||||
### 6. Desaturated Background Wallpaper
|
||||
|
||||
**Practicality: low. Recommendation: do not build, or gate behind a setting defaulted off.**
|
||||
|
||||
Trivial to implement (`ui.add_css` with a CSS `filter`), but this is a dense archival data application - transcripts, JSON evidence panels, data tables. A background image behind all of that costs contrast and legibility on every page, for aesthetic gain only.
|
||||
|
||||
## Suggested Grouping
|
||||
|
||||
If V4.8 is scoped as one release, the natural split is:
|
||||
|
||||
**Track A - image experience:** items 1 and 2. Both are small, both are self-contained UI work, and item 2's storage layer already exists. This is the highest value for the least risk.
|
||||
|
||||
**Track B - metadata layer:** items 3 and 4 together, since they share a table. Only worth starting if both are wanted.
|
||||
|
||||
**Track C - telemetry:** item 5, gated on V4.7 Phase 4.
|
||||
|
||||
Item 6 is not recommended.
|
||||
|
||||
## Open Questions for Scoping
|
||||
|
||||
- Should Track B happen at all, or is one portrait per person sufficient?
|
||||
- Should the slideshow interval be configurable, or fixed?
|
||||
- Should the model-performance rollup be its own page, or a panel on an existing one?
|
||||
- Should vendored Panzoom be committed to the repository, or fetched at build time?
|
||||
|
||||
## Related Local References
|
||||
|
||||
- [V4.7 Scope Boundary](../ver4.7/scope_boundary_v4_7.md) - the blocking dependency for item 5
|
||||
- [V4.6 Scope Boundary](../ver4.6/scope_boundary_v4_6.md)
|
||||
- [Architecture & Code Review Report](../architecture_code_review_2026-08-17.md)
|
||||
- `.github/instructions/ui.instructions.md`
|
||||
- `src/transcription/ui/homepage_store.py` - existing multi-image storage
|
||||
- `src/transcription/ui/components/media_urls.py` - canonical URL resolution
|
||||
Reference in New Issue
Block a user