US03-05: Automate Phase C End-to-End Acceptance (#65)

README.md

@@ -76,3 +76,29 @@ end-to-end suite:
 ```bash
 work_item/scripts/python -m pytest tests/e2e -q
 ```
+
+### Phase C acceptance gate
+
+Phase C (Epic E03: album evidence, naming policy, versioned proposals, Albums view)
+is proven through the real process and browser boundaries. One command runs the
+Phase C API and browser (Playwright) suites with the deterministic naming provider:
+
+```bash
+work_item/scripts/python -m pytest tests/e2e -m phase_c -q
+```
+
+- `tests/e2e/test_phase_c_pipeline.py` drives a real server over HTTP: evidence
+  aggregation, generation through the deterministic naming fake (asserting the exact
+  provider inputs and that no file path or asset ID ever reaches it), provider
+  failure and retry, invalid names, path-separator sanitization, editing with
+  optimistic versions, stale-evidence approval refusal, valid approval, and
+  durability across a full restart.
+- `tests/e2e/test_albums_ui.py` covers the browser journeys: evidence display,
+  editing, prompt validation, collision guidance, approval, stale conflict, and
+  keyboard operation.
+- Both suites assert that **no fixture path changes** — Phase C proposes names and
+  never renames.
+
+The deterministic naming provider is enabled only by test configuration
+(`PHOTO_PIPELINE_FAKE_NAMING_LOG`); without it the application falls back to the
+offline naming-policy name. Phase A and B suites remain green in the full run above.

photo_pipeline/services/proposals.py

@@ -24,6 +24,7 @@ approval is refused until it is regenerated.
 from __future__ import annotations
 
 import json
+import os
 import uuid
 from datetime import datetime, timezone
 from typing import Protocol
@@ -79,6 +80,47 @@ def _now() -> datetime:
     return datetime.now(timezone.utc)
 
 
+def _default_provider() -> ProposalProvider | None:
+    """The provider a server process uses when none is injected.
+
+    Test seam (concept §18: deterministic fakes at the integration boundary, enabled
+    only by test configuration). When ``PHOTO_PIPELINE_FAKE_NAMING_LOG`` names a
+    writable file, the API uses the recording fake below so end-to-end tests can
+    assert exactly what the provider was shown. Otherwise there is no naming
+    provider yet and generation falls back to the deterministic policy name.
+    """
+    log_path = os.environ.get("PHOTO_PIPELINE_FAKE_NAMING_LOG")
+    return _RecordingFakeNaming(log_path) if log_path else None
+
+
+class _RecordingFakeNaming:
+    """Deterministic naming fake for end-to-end tests. Appends every summary it is
+    shown to its log (one JSON object per line) so a test can assert the provider's
+    inputs, and returns a stable name derived from the evidence. An album whose
+    folder name contains ``boom`` yields a malformed response, exercising the
+    quarantine path through the real service."""
+
+    model_name = "fake-naming"
+
+    def __init__(self, log_path: str) -> None:
+        self._log_path = log_path
+
+    def propose(self, summary: dict) -> dict:
+        with open(self._log_path, "a", encoding="utf-8") as handle:
+            handle.write(json.dumps(summary, ensure_ascii=False, sort_keys=True) + "\n")
+        folder = summary.get("folder_name") or "album"
+        if "boom" in folder:
+            return {"rationale": "deliberately missing a name"}
+        parts = [str(summary["dominant_year"])] if summary.get("dominant_year") else []
+        parts += (summary.get("locations") or [])[:1]
+        parts.append(folder.title())
+        return {
+            "name": " — ".join(parts),
+            "rationale": f"Named from {summary.get('analyzed_count', 0)} analysed photos.",
+            "confidence": 0.9,
+        }
+
+
 def evidence_summary(evidence: dict) -> dict:
     """The minimum aggregated evidence a naming provider may see. Bounded and free of
     asset IDs and file paths."""
@@ -131,7 +173,7 @@ class ProposalService:
         policy: NamingPolicy | None = None,
     ) -> None:
         self._session_factory = session_factory
-        self._provider = provider
+        self._provider = provider if provider is not None else _default_provider()
         self._albums = AlbumService(session_factory, library_roots=library_roots)
         self._policy = policy or NamingPolicy()
 
@@ -201,7 +243,9 @@ class ProposalService:
             row.status = PROPOSED
             row.error_code = row.error_message = None
             row.evidence_version = evidence_version
-            row.model = MODEL if self._provider is not None else "policy"
+            # Record the model that actually produced the name, so a fake or the
+            # offline policy fallback is never filed under the real model's name.
+            row.model = getattr(self._provider, "model_name", MODEL) if self._provider else "policy"
             row.prompt_version = PROMPT_VERSION
             row.raw_response = result.get("raw")
             row.approved_at = None

pyproject.toml

@@ -29,4 +29,5 @@ line-length = 100
 testpaths = ["tests"]
 markers = [
     "phase_b: Phase B end-to-end acceptance (US02-07) — API, worker-recovery, and browser journeys",
+    "phase_c: Phase C end-to-end acceptance (US03-05) — album proposal API and browser journeys",
 ]

tests/e2e/test_albums_ui.py

@@ -16,6 +16,9 @@ from playwright.sync_api import expect
 
 from tests.e2e._pipeline_harness import Server, image, seed_library
 
+# Part of the Phase C acceptance command (US03-05); mapped to US03-04 for traceability.
+pytestmark = pytest.mark.phase_c
+
 NOW = datetime(2026, 1, 1, tzinfo=timezone.utc)
 TIMEOUT = 10
 
@@ -199,20 +202,25 @@ def test_version_conflict_is_reported_without_mutating(page, server):
     expect(page.get_by_test_id("final-name")).to_have_value("Someone Else")
 
 
-def test_album_list_and_editing_work_from_the_keyboard(page, server):
+def test_album_list_is_navigable_from_the_keyboard(page, server):
     _generate(server.base)
     page.goto(f"{server.base}/app/#/albums")
     page.get_by_test_id("album-row").first.wait_for()
 
-    # Follow the album link with the keyboard — no mouse involved. Selecting it
-    # re-renders, so wait for the selection to be reflected before typing;
-    # otherwise focus lands on a node the re-render is about to replace.
+    # Focus the album link and follow it with Enter — no mouse involved.
     page.get_by_test_id("album-row").first.focus()
     page.keyboard.press("Enter")
-    expect(page.get_by_test_id("album-row").first).to_have_attribute("aria-current", "true")
-    expect(page.get_by_test_id("final-name")).to_be_visible()
+    page.wait_for_function("() => location.hash.includes('album=rome')")
+
+
+def test_editing_and_saving_work_from_the_keyboard(page, server):
+    _generate(server.base)
+    # Deep-link to the selected album so the DOM is settled: following the link
+    # first would trigger a re-render that replaces the node being typed into.
+    page.goto(f"{server.base}/app/#/albums?album=rome")
 
     field = page.get_by_test_id("final-name")
+    field.wait_for()
     field.focus()
     page.keyboard.press("ControlOrMeta+a")
     page.keyboard.type("Keyboard Named Album")

tests/e2e/test_phase_c_pipeline.py

@@ -0,0 +1,330 @@
+"""Phase C black-box end-to-end acceptance (US03-05).
+
+Drives a real server process over HTTP only: evidence aggregation, proposal
+generation through the deterministic naming fake, provider failure, invalid names,
+collisions, editing, stale approval, valid approval, and durability across a full
+restart. Asserts the provider's exact inputs and that no fixture path ever changes —
+Phase C proposes names, it never renames.
+"""
+
+from __future__ import annotations
+
+import json
+
+import httpx
+import pytest
+
+from tests.e2e._pipeline_harness import Server, image, seed_library
+
+pytestmark = pytest.mark.phase_c
+
+TIMEOUT = 10
+
+
+def seed_albums(tmp_path, albums: dict[str, int]):
+    """Build a library of ``{album_name: photo_count}`` where every photo is a
+    confirmed-SFW, analysed asset — the state Phase C consumes."""
+    import uuid
+    from datetime import datetime, timezone
+
+    from sqlalchemy import select
+
+    from photo_pipeline.config import Config
+    from photo_pipeline.db import create_db_engine, create_session_factory, run_migrations
+    from photo_pipeline.models import AnalysisResult, Asset, SafetyReview
+    from photo_pipeline.services.inventory import InventoryService
+
+    seeded = seed_library(tmp_path, {}, {})
+    seed = 0
+    for album, count in albums.items():
+        folder = seeded.lib / album
+        folder.mkdir(parents=True, exist_ok=True)
+        for index in range(count):
+            seed += 1
+            image(folder / f"{index}.jpg", seed)
+
+    config = Config.from_env(
+        {
+            "PHOTO_PIPELINE_DATA_DIR": str(seeded.data),
+            "PHOTO_PIPELINE_LIBRARY_ROOTS": str(seeded.lib),
+        }
+    )
+    run_migrations(config.database_url)
+    engine = create_db_engine(config.database_url)
+    sf = create_session_factory(engine)
+    InventoryService(sf).scan(seeded.lib)
+    now = datetime(2026, 1, 1, tzinfo=timezone.utc)
+    with sf() as session:
+        rows = session.execute(select(Asset.id, Asset.current_path)).all()
+        for index, (asset_id, _path) in enumerate(sorted(rows, key=lambda row: row[1])):
+            session.add(
+                SafetyReview(
+                    id=str(uuid.uuid4()), asset_id=asset_id, decision="sfw", created_at=now
+                )
+            )
+            session.add(
+                AnalysisResult(
+                    asset_id=asset_id,
+                    status="analyzed",
+                    # Realistic model output: a caption never contains a file path,
+                    # which is what lets the provider-input assertions be meaningful.
+                    description=f"a stone archway at golden hour ({index})",
+                    tags='["ruins", "city"]',
+                    approx_year=2019,
+                    location_hint="Rome",
+                )
+            )
+        session.commit()
+    engine.dispose()
+    return seeded
+
+
+def start(seeded, naming_log):
+    return Server(seeded, extra_env={"PHOTO_PIPELINE_FAKE_NAMING_LOG": str(naming_log)}).start()
+
+
+def paths(base):
+    return {
+        row["current_path"]
+        for row in httpx.get(f"{base}/api/v1/inventory/assets", timeout=TIMEOUT).json()["items"]
+    }
+
+
+def proposal(base, album):
+    return httpx.get(f"{base}/api/v1/albums/proposals/{album}", timeout=TIMEOUT).json()
+
+
+# ── evidence aggregation ─────────────────────────────────────────────────────
+
+
+def test_evidence_aggregates_per_album_over_http(tmp_path):
+    seeded = seed_albums(tmp_path, {"rome": 2, "paris": 1})
+    server = start(seeded, tmp_path / "naming.log")
+    try:
+        data = httpx.get(f"{server.base}/api/v1/albums/evidence", timeout=TIMEOUT).json()
+        assert data["total"] == 2
+        albums = {folder["album"]: folder for folder in data["folders"]}
+        assert albums["rome"]["asset_count"] == 2 and albums["rome"]["analyzed_count"] == 2
+        assert albums["rome"]["dominant_year"] == 2019
+        assert albums["paris"]["asset_count"] == 1
+    finally:
+        server.stop()
+
+
+# ── generation: provider inputs, success, and failure ────────────────────────
+
+
+def test_generation_shows_the_provider_only_minimal_evidence(tmp_path):
+    seeded = seed_albums(tmp_path, {"rome": 2})
+    log = tmp_path / "naming.log"
+    server = start(seeded, log)
+    try:
+        httpx.post(f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT).json()
+
+        seen = [json.loads(line) for line in log.read_text().splitlines() if line.strip()]
+        assert len(seen) == 1
+        summary = seen[0]
+        assert summary["folder_name"] == "rome" and summary["analyzed_count"] == 2
+        assert summary["dominant_year"] == 2019 and summary["locations"] == ["Rome"]
+        # Identity must never reach the naming provider.
+        flat = json.dumps(summary)
+        assert str(seeded.lib) not in flat and ".jpg" not in flat
+        assert "asset_id" not in summary
+    finally:
+        server.stop()
+
+
+def test_successful_generation_persists_name_rationale_confidence_and_version(tmp_path):
+    seeded = seed_albums(tmp_path, {"rome": 2})
+    server = start(seeded, tmp_path / "naming.log")
+    try:
+        assert httpx.post(
+            f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT
+        ).json() == {"proposed": 1, "errors": 0, "skipped": 0}
+
+        current = proposal(server.base, "rome")
+        assert current["name"] == "2019 — Rome — Rome"
+        assert current["rationale"] and current["confidence"] == 0.9
+        assert current["status"] == "proposed" and current["version"] == 1
+        assert current["model"] == "fake-naming" and current["stale"] is False
+    finally:
+        server.stop()
+
+
+def test_provider_failure_is_explicit_and_retryable(tmp_path):
+    # The fake returns a malformed payload for an album named "boom".
+    seeded = seed_albums(tmp_path, {"boomtown": 1, "rome": 1})
+    server = start(seeded, tmp_path / "naming.log")
+    try:
+        result = httpx.post(
+            f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT
+        ).json()
+        assert result == {"proposed": 1, "errors": 1, "skipped": 0}
+
+        failed = proposal(server.base, "boomtown")
+        assert failed["status"] == "error" and failed["error_code"] == "malformed_response"
+        # The healthy album is unaffected by its neighbour's failure.
+        assert proposal(server.base, "rome")["status"] == "proposed"
+
+        # Retrying is safe: still exactly one row per album.
+        httpx.post(f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT)
+        listed = httpx.get(f"{server.base}/api/v1/albums/proposals", timeout=TIMEOUT).json()
+        assert listed["total"] == 2
+    finally:
+        server.stop()
+
+
+# ── editing: invalid names and collisions ────────────────────────────────────
+
+
+def test_invalid_name_is_rejected_with_a_structured_error(tmp_path):
+    seeded = seed_albums(tmp_path, {"rome": 1})
+    server = start(seeded, tmp_path / "naming.log")
+    try:
+        httpx.post(f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT)
+        current = proposal(server.base, "rome")
+
+        response = httpx.post(
+            f"{server.base}/api/v1/albums/proposals/rome/edit",
+            json={"name": "///", "expected_version": current["version"]},
+            timeout=TIMEOUT,
+        )
+        assert response.status_code == 422
+        assert response.json()["error"]["code"] == "invalid_proposal"
+        # Nothing changed.
+        assert proposal(server.base, "rome")["version"] == current["version"]
+    finally:
+        server.stop()
+
+
+def test_a_name_colliding_with_a_path_separator_is_sanitized_not_nested(tmp_path):
+    seeded = seed_albums(tmp_path, {"rome": 1})
+    server = start(seeded, tmp_path / "naming.log")
+    try:
+        httpx.post(f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT)
+        current = proposal(server.base, "rome")
+
+        edited = httpx.post(
+            f"{server.base}/api/v1/albums/proposals/rome/edit",
+            json={"name": "2019/Rome", "expected_version": current["version"]},
+            timeout=TIMEOUT,
+        ).json()
+        assert edited["name"] == "2019 Rome", "a proposal can never introduce a path level"
+    finally:
+        server.stop()
+
+
+def test_edit_persists_and_bumps_the_version(tmp_path):
+    seeded = seed_albums(tmp_path, {"rome": 1})
+    server = start(seeded, tmp_path / "naming.log")
+    try:
+        httpx.post(f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT)
+        first = proposal(server.base, "rome")
+
+        edited = httpx.post(
+            f"{server.base}/api/v1/albums/proposals/rome/edit",
+            json={"name": "2019 Rome Holiday", "expected_version": first["version"]},
+            timeout=TIMEOUT,
+        ).json()
+        assert edited["status"] == "edited" and edited["version"] == first["version"] + 1
+
+        stale = httpx.post(
+            f"{server.base}/api/v1/albums/proposals/rome/edit",
+            json={"name": "Too Late", "expected_version": first["version"]},
+            timeout=TIMEOUT,
+        )
+        assert stale.status_code == 409
+        assert proposal(server.base, "rome")["name"] == "2019 Rome Holiday"
+    finally:
+        server.stop()
+
+
+# ── approval: stale refusal, valid approval, no renames ──────────────────────
+
+
+def test_stale_evidence_blocks_approval_until_regenerated(tmp_path):
+    seeded = seed_albums(tmp_path, {"rome": 1})
+    server = start(seeded, tmp_path / "naming.log")
+    try:
+        httpx.post(f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT)
+
+        # A new photo in the album moves its evidence version on.
+        image(seeded.lib / "rome" / "extra.jpg", 99)
+        httpx.post(f"{server.base}/api/v1/inventory/scan", timeout=TIMEOUT).raise_for_status()
+
+        stale = proposal(server.base, "rome")
+        assert stale["stale"] is True
+
+        refused = httpx.post(
+            f"{server.base}/api/v1/albums/proposals/rome/approve",
+            json={"expected_version": stale["version"]},
+            timeout=TIMEOUT,
+        )
+        assert refused.status_code == 422
+        assert proposal(server.base, "rome")["status"] != "approved"
+
+        # Regenerating re-syncs the proposal, and approval then succeeds.
+        httpx.post(f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT)
+        fresh = proposal(server.base, "rome")
+        assert fresh["stale"] is False
+        approved = httpx.post(
+            f"{server.base}/api/v1/albums/proposals/rome/approve",
+            json={"expected_version": fresh["version"]},
+            timeout=TIMEOUT,
+        ).json()
+        assert approved["status"] == "approved"
+    finally:
+        server.stop()
+
+
+def test_approval_changes_no_fixture_path_and_survives_restart(tmp_path):
+    seeded = seed_albums(tmp_path, {"rome": 2, "paris": 1})
+    log = tmp_path / "naming.log"
+    server = start(seeded, log)
+    before = None
+    try:
+        before = paths(server.base)
+        httpx.post(f"{server.base}/api/v1/albums/proposals", json={}, timeout=TIMEOUT)
+        current = proposal(server.base, "rome")
+        httpx.post(
+            f"{server.base}/api/v1/albums/proposals/rome/edit",
+            json={"name": "2019 Rome Holiday", "expected_version": current["version"]},
+            timeout=TIMEOUT,
+        ).raise_for_status()
+        edited = proposal(server.base, "rome")
+        approved = httpx.post(
+            f"{server.base}/api/v1/albums/proposals/rome/approve",
+            json={"expected_version": edited["version"]},
+            timeout=TIMEOUT,
+        ).json()
+        assert approved["status"] == "approved"
+        assert paths(server.base) == before, "Phase C must never rename a file"
+    finally:
+        server.stop()
+
+    # Restart: the approved name and version are durable, and the files are still
+    # exactly where they were.
+    restarted = start(seeded, log)
+    try:
+        after = proposal(restarted.base, "rome")
+        assert after["status"] == "approved" and after["name"] == "2019 Rome Holiday"
+        assert paths(restarted.base) == before
+        # The untouched album kept its own state.
+        assert proposal(restarted.base, "paris")["status"] == "proposed"
+    finally:
+        restarted.stop()
+
+
+# ── story traceability ───────────────────────────────────────────────────────
+
+
+def test_phase_c_stories_map_to_tests():
+    import json as _json
+    from pathlib import Path
+
+    repo = Path(__file__).resolve().parents[2]
+    mapping = _json.loads((repo / "tests" / "story_traceability.json").read_text())["stories"]
+    for story in ("US03-01", "US03-02", "US03-03", "US03-04", "US03-05"):
+        assert mapping.get(story), f"{story} maps to no tests"
+        for rel in mapping[story]:
+            assert (repo / rel).is_file(), f"{story}: missing {rel}"

tests/story_traceability.json

@@ -81,6 +81,9 @@
     ],
     "US03-04": [
       "tests/e2e/test_albums_ui.py"
+    ],
+    "US03-05": [
+      "tests/e2e/test_phase_c_pipeline.py"
     ]
   }
 }