gpt-5.3 codex review: Phase 4

tests/api/test_error_responses.py

@@ -33,13 +33,13 @@ class TestApiErrorResponses:
         assert response.status_code == 400
         payload = response.json()
         assert payload["error_id"] == "abc12345"
-        assert payload["category"] == "validation_error"
+        assert payload["category"] == "validation"
         assert payload["message"] == "Bad upload payload"
         assert payload["suggestion"] == "Upload a non-empty file"
         assert "timestamp" in payload
 
-    def test_unexpected_error_returns_internal_unexpected_envelope(self):
-        """Unexpected exceptions map to internal_unexpected_error with 500."""
+    def test_unexpected_error_returns_internal_envelope(self):
+        """Unexpected exceptions map to canonical internal category with 500."""
         app = FastAPI()
         register_error_handlers(app)
 
@@ -52,6 +52,42 @@ class TestApiErrorResponses:
 
         assert response.status_code == 500
         payload = response.json()
-        assert payload["category"] == "internal_unexpected_error"
+        assert payload["category"] == "internal"
         assert "error_id" in payload
         assert payload["suggestion"]
+
+    @pytest.mark.parametrize(
+        ("category", "expected_status", "expected_envelope_category"),
+        [
+            (ErrorCategory.USER_INPUT, 400, "validation"),
+            (ErrorCategory.NOT_FOUND, 404, "not_found"),
+            (ErrorCategory.CONFLICT, 409, "conflict"),
+            (ErrorCategory.EXTERNAL_PROVIDER, 503, "external"),
+            (ErrorCategory.INFRA_TRANSIENT, 503, "timeout"),
+            (ErrorCategory.INFRA_PERSISTENT, 500, "internal"),
+            (ErrorCategory.INTERNAL_UNEXPECTED, 500, "internal"),
+        ],
+    )
+    def test_app_error_category_mapping_uses_canonical_envelope_taxonomy(
+        self,
+        category: ErrorCategory,
+        expected_status: int,
+        expected_envelope_category: str,
+    ):
+        app = FastAPI()
+        register_error_handlers(app)
+
+        @app.get("/category")
+        def category_route() -> dict[str, str]:
+            raise AppError(
+                "Category test",
+                category=category,
+                suggestion="retry",
+                error_id="cat12345",
+            )
+
+        client = TestClient(app)
+        response = client.get("/category")
+
+        assert response.status_code == expected_status
+        assert response.json()["category"] == expected_envelope_category

tests/api/test_v4_documents.py

@@ -224,4 +224,4 @@ def test_duplicate_document_person_link_returns_conflict_envelope(tmp_path):
 
     assert second.status_code == 409
     payload = second.json()
-    assert payload["category"] == "conflict_error"
+    assert payload["category"] == "conflict"

tests/services/test_v45_candidates.py

@@ -121,3 +121,105 @@ async def test_retranscription_job_locks_source_and_frozen_model(default_session
     assert loaded.model == "vendor/alternate"
     assert loaded.user_prompt == "Transcribe verbatim."
     assert [link.source_id for link in loaded.job_sources] == [source.id]
+
+
+@pytest.mark.integration
+@pytest.mark.asyncio
+async def test_promoting_candidate_does_not_mutate_execution_attempt_history(default_session_factory):
+    settings = Settings(openrouter_api_key="test-key", provider_models=None)
+    services = _services(default_session_factory, settings)
+    source = await _seed_source(services)
+
+    first_job = await services.jobs.create_job(Job(document_id=source.document_id))
+    second_job = await services.jobs.create_job(Job(document_id=source.document_id))
+    await services.sources.create_job_source(JobSource(job_id=first_job.id, source_id=source.id))
+    await services.sources.create_job_source(JobSource(job_id=second_job.id, source_id=source.id))
+
+    await services.sources.update_job_source_transcription(
+        job_id=first_job.id,
+        source_id=source.id,
+        text="baseline",
+        provider="fixture",
+        model="model-a",
+    )
+    await services.sources.update_job_source_transcription(
+        job_id=second_job.id,
+        source_id=source.id,
+        text="candidate",
+        provider="fixture",
+        model="model-b",
+    )
+
+    before = [
+        (
+            attempt.id,
+            attempt.job_id,
+            attempt.source_id,
+            attempt.attempt_number,
+            attempt.status.value,
+            attempt.raw_transcription,
+            attempt.error_category,
+            attempt.error_detail,
+            attempt.failure_phase,
+        )
+        for attempt in await services.evidence.list_execution_attempts(source_id=source.id)
+    ]
+
+    candidate = next(
+        attempt
+        for attempt in await services.evidence.list_execution_attempts(source_id=source.id)
+        if attempt.raw_transcription == "candidate"
+    )
+    await services.evidence.promote_machine_attempt(source_id=source.id, execution_attempt_id=candidate.id)
+
+    after = [
+        (
+            attempt.id,
+            attempt.job_id,
+            attempt.source_id,
+            attempt.attempt_number,
+            attempt.status.value,
+            attempt.raw_transcription,
+            attempt.error_category,
+            attempt.error_detail,
+            attempt.failure_phase,
+        )
+        for attempt in await services.evidence.list_execution_attempts(source_id=source.id)
+    ]
+
+    assert after == before
+
+
+@pytest.mark.integration
+@pytest.mark.asyncio
+async def test_retry_appends_new_attempt_instead_of_rewriting_history(default_session_factory):
+    settings = Settings(openrouter_api_key="test-key", provider_models=None)
+    services = _services(default_session_factory, settings)
+    source = await _seed_source(services)
+
+    job = await services.jobs.create_job(Job(document_id=source.document_id))
+    await services.sources.create_job_source(JobSource(job_id=job.id, source_id=source.id))
+
+    await services.sources.update_job_source_transcription(
+        job_id=job.id,
+        source_id=source.id,
+        text=None,
+        error_detail="timeout",
+        error_category="timeout",
+        provider="fixture",
+        model="model-a",
+        failure_phase="provider_call",
+    )
+    await services.sources.update_job_source_transcription(
+        job_id=job.id,
+        source_id=source.id,
+        text="retry-success",
+        provider="fixture",
+        model="model-a",
+    )
+
+    attempts = list(await services.evidence.list_execution_attempts(source_id=source.id))
+    assert len(attempts) == 2
+    assert [attempt.attempt_number for attempt in attempts] == [1, 2]
+    assert attempts[0].raw_transcription is None
+    assert attempts[1].raw_transcription == "retry-success"

tests/services/test_workflows_reliability.py

@@ -181,6 +181,74 @@ class TestWorkflowReliability:
         assert duration_ms >= int(budget_seconds * 1000 * 0.9)
         assert duration_ms < int((budget_seconds + setup_seconds) * 1000 * 0.9)
 
+    @pytest.mark.asyncio
+    async def test_attempt_metadata_persists_provider_and_processing_durations(
+        self,
+        default_session_factory,
+        monkeypatch,
+    ):
+        """Execution metadata records both provider-only and end-to-end durations."""
+        services = ServiceBundle.from_session_factory(default_session_factory)
+        async with services.jobs._session_scope() as session:
+            document = Document(id=uuid4(), name="timing-metadata-doc")
+            session.add(document)
+            await session.flush()
+            job = Job(document_id=document.id, status=JobStatus.QUEUED)
+            session.add(job)
+            await session.flush()
+            source = Source(
+                document_id=document.id,
+                page_number=1,
+                upload_name="timing.jpg",
+                filename="timing.jpg",
+                file_path=str(Path("tests/fixtures/images/real/Book Two - page 02.jpg")),
+                file_hash="f" * 64,
+                file_size_bytes=1,
+            )
+            session.add(source)
+            await session.flush()
+            session.add(JobSource(job_id=job.id, source_id=source.id, status=JobSourceStatus.PENDING))
+            await session.commit()
+            loaded = await services.jobs.read_job(job_id=job.id, session=session)
+
+        async def _returns_text(*args, **kwargs):
+            _ = (args, kwargs)
+            await asyncio.sleep(0.03)
+            return TranscriptionResult(text="timed output", provider="fixture", model="fixture-model")
+
+        monkeypatch.setattr("transcription.services.workflows.transcribe_document_image", _returns_text)
+
+        result = await process_queued_job(
+            job=loaded,
+            services=services,
+            settings=Settings(openrouter_api_key="test-key", worker_provider_timeout_seconds=2.0),
+        )
+        assert result is not None
+        assert result.status == JobStatus.TRANSCRIBED
+
+        async with services.jobs._session_scope() as session:
+            attempts = (
+                (
+                    await session.exec(
+                        select(ExecutionAttempt).where(
+                            col(ExecutionAttempt.job_source_id).in_([js.id for js in result.job_sources])
+                        )
+                    )
+                )
+                .all()
+            )
+        assert len(attempts) == 1
+        attempt = attempts[0]
+        timing = (attempt.normalized_metadata or {}).get("processing_timing")
+        assert isinstance(timing, dict)
+        provider_call_ms = timing.get("provider_call_duration_ms")
+        processing_ms = timing.get("processing_duration_ms")
+        assert isinstance(provider_call_ms, int)
+        assert isinstance(processing_ms, int)
+        assert provider_call_ms >= 0
+        assert processing_ms >= provider_call_ms
+        assert attempt.duration_ms == provider_call_ms
+
     @pytest.mark.asyncio
     async def test_error_after_claim_fails_the_job_instead_of_stranding_it(
         self,
@@ -239,7 +307,7 @@ class TestWorkflowReliability:
             assert final.status == JobStatus.FAILED
 
     @pytest.mark.asyncio
-    async def test_completed_page_is_committed_before_next_provider_call_finishes(
+    async def test_transcribed_page_is_committed_before_next_provider_call_finishes(
         self,
         default_session_factory,
         monkeypatch,

tests/test_errors.py

@@ -4,6 +4,8 @@ import pytest
 
 from transcription.errors import AppError
 from transcription.errors import ErrorCategory
+from transcription.errors import build_error_envelope
+from transcription.errors import canonical_error_category
 from transcription.errors import classify_unexpected_error
 from transcription.errors import new_error_id
 
@@ -44,3 +46,14 @@ class TestAppErrorHelpers:
         assert "boom" in err.message
         assert err.suggestion
         assert err.error_id
+
+    def test_envelope_categories_use_canonical_contract_values(self):
+        """API/UI envelope categories are normalized to canonical short identifiers."""
+        validation = AppError("x", category=ErrorCategory.USER_INPUT)
+        timeout = AppError("x", category=ErrorCategory.INFRA_TRANSIENT)
+        internal = AppError("x", category=ErrorCategory.INTERNAL_UNEXPECTED)
+
+        assert canonical_error_category(validation) == "validation"
+        assert canonical_error_category(timeout) == "timeout"
+        assert canonical_error_category(internal) == "internal"
+        assert build_error_envelope(validation).category == "validation"

tests/test_media_path_safety.py

@@ -0,0 +1,30 @@
+"""Guardrails for UI/API media path safety contracts."""
+
+from __future__ import annotations
+
+from pathlib import Path
+
+
+PROJECT_ROOT = Path(__file__).resolve().parents[1]
+UI_ROOT = PROJECT_ROOT / "src" / "transcription" / "ui"
+API_ROOT = PROJECT_ROOT / "src" / "transcription" / "api"
+SOURCES_PAGE = UI_ROOT / "pages" / "sources_page.py"
+
+
+def _python_files(root: Path) -> list[Path]:
+    return sorted(root.rglob("*.py"))
+
+
+def test_no_ui_or_api_python_file_uses_file_scheme_links():
+    violations: list[str] = []
+    for path in [*_python_files(UI_ROOT), *_python_files(API_ROOT)]:
+        text = path.read_text(encoding="utf-8")
+        if "file://" in text:
+            violations.append(str(path.relative_to(PROJECT_ROOT)).replace("\\", "/"))
+    assert violations == []
+
+
+def test_sources_page_does_not_render_raw_file_path_value():
+    text = SOURCES_PAGE.read_text(encoding="utf-8")
+    assert 'metadata_row("Stored Path:", source.file_path)' not in text
+    assert "public_media_path_label(source.file_path" in text

tests/test_meta_contract_guards.py

@@ -0,0 +1,99 @@
+"""Regression guards for canonical documentation and instruction contracts."""
+
+from __future__ import annotations
+
+from pathlib import Path
+
+
+PROJECT_ROOT = Path(__file__).resolve().parents[1]
+
+
+ACTIVE_CONTRACT_FILES = (
+    ".github/instructions/services.instructions.md",
+    ".github/instructions/ui.instructions.md",
+    ".github/instructions/error-handling.instructions.md",
+    ".github/skills/python-code-reviewer/skill.md",
+    ".github/skills/evidence-provenance-auditor/skill.md",
+    "docs/ver4/index_v4.md",
+    "docs/ver4/architecture_v4.md",
+    "docs/ver4/requirements_v4.md",
+    "docs/ver4/schema_v4.md",
+    "docs/ver4/error_handling_v4.md",
+    "docs/ui/README.md",
+)
+
+
+LEGACY_REFERENCE_MARKERS = (
+    "docs-v4x-archive",
+    "docs/ver4/history.md",
+    "docs/ver4.0",
+    "docs/ver4.1",
+    "docs/ver4.2",
+    "docs/ver4.3",
+    "docs/ver4.4",
+    "docs/ver4.5",
+    "docs/ver4.6",
+    "docs/ver4.7",
+)
+
+
+def _read(relative_path: str) -> str:
+    return (PROJECT_ROOT / relative_path).read_text(encoding="utf-8")
+
+
+def test_active_contract_files_are_present():
+    """Guard the guard: ensure all expected authority files are scanned."""
+    missing = [path for path in ACTIVE_CONTRACT_FILES if not (PROJECT_ROOT / path).exists()]
+    assert missing == []
+
+
+def test_no_legacy_authority_references_in_active_contract_files():
+    """Active contracts must not route authority through removed V4 revision trees."""
+    violations: dict[str, list[str]] = {}
+    for relative_path in ACTIVE_CONTRACT_FILES:
+        text = _read(relative_path)
+        found = [marker for marker in LEGACY_REFERENCE_MARKERS if marker in text]
+        if found:
+            violations[relative_path] = found
+    assert violations == {}
+
+
+def test_canonical_authority_references_are_present():
+    """Critical instruction and skill files must keep canonical references explicit."""
+    required_fragments = {
+        ".github/instructions/services.instructions.md": (
+            "docs/ver4/",
+            "./error-handling.instructions.md",
+            "src/transcription/db/models.py",
+            "docs/ver4/schema_v4.md",
+            "append-only",
+        ),
+        ".github/instructions/ui.instructions.md": (
+            "docs/ver4/",
+            "./error-handling.instructions.md",
+            "src/transcription/db/models.py",
+            "docs/ver4/schema_v4.md",
+        ),
+        ".github/instructions/error-handling.instructions.md": (
+            "docs/ver4/error_handling_v4.md",
+            "docs/ver4/requirements_v4.md",
+        ),
+        ".github/skills/python-code-reviewer/skill.md": (
+            "docs/ver4/*",
+            "docs/ver4/schema_v4.md",
+            ".github/instructions/error-handling.instructions.md",
+        ),
+        ".github/skills/evidence-provenance-auditor/skill.md": (
+            "docs/ver4/schema_v4.md",
+            "docs/ver4/requirements_v4.md",
+            "docs/ver4/error_handling_v4.md",
+        ),
+    }
+
+    missing: dict[str, list[str]] = {}
+    for relative_path, fragments in required_fragments.items():
+        text = _read(relative_path)
+        absent = [fragment for fragment in fragments if fragment not in text]
+        if absent:
+            missing[relative_path] = absent
+    assert missing == {}

tests/test_models.py

@@ -157,6 +157,21 @@ class TestJobModel:
 
         assert job.status == JobStatus.TRANSCRIBED
 
+    def test_status_enums_match_v4_lifecycle_contract(self):
+        assert [status.value for status in JobStatus] == [
+            "queued",
+            "processing",
+            "transcribed",
+            "partial_success",
+            "failed",
+        ]
+        assert [status.value for status in JobSourceStatus] == [
+            "pending",
+            "transcribed",
+            "failed",
+            "cancelled",
+        ]
+
 
 class TestSourceModel:
     def test_can_be_created_for_document(self, session):

tests/ui/test_media_urls.py

@@ -0,0 +1,34 @@
+from pathlib import Path
+
+from transcription.ui.components.media_urls import public_media_path_label
+
+
+def test_public_media_path_label_maps_managed_absolute_path_to_upload_route(tmp_path):
+    upload_dir = tmp_path / "uploads"
+    managed_path = upload_dir / "documents" / "abc" / "page.jpg"
+    managed_path.parent.mkdir(parents=True, exist_ok=True)
+    managed_path.write_bytes(b"x")
+
+    label = public_media_path_label(str(managed_path), upload_dir=upload_dir)
+    assert label == "/uploads/documents/abc/page.jpg"
+
+
+def test_public_media_path_label_hides_unmanaged_absolute_path(tmp_path):
+    upload_dir = tmp_path / "uploads"
+    unmanaged_path = tmp_path / "other-root" / "secret" / "page.jpg"
+
+    label = public_media_path_label(str(unmanaged_path), upload_dir=upload_dir)
+    assert label == "page.jpg"
+
+
+def test_public_media_path_label_preserves_upload_relative_route(tmp_path):
+    upload_dir = tmp_path / "uploads"
+    label = public_media_path_label("documents/person-1/photo.png", upload_dir=upload_dir)
+    assert label == "/uploads/documents/person-1/photo.png"
+
+
+def test_public_media_path_label_keeps_existing_upload_route(tmp_path):
+    upload_dir = tmp_path / "uploads"
+    assert public_media_path_label("/uploads/documents/d1/page.png", upload_dir=upload_dir) == (
+        "/uploads/documents/d1/page.png"
+    )