diff --git a/README.md b/README.md index 2f8a603..8f7ea29 100644 --- a/README.md +++ b/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. diff --git a/photo_pipeline/services/proposals.py b/photo_pipeline/services/proposals.py index 264b5b6..17d7556 100644 --- a/photo_pipeline/services/proposals.py +++ b/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 diff --git a/pyproject.toml b/pyproject.toml index 21eec90..2e7f3b8 100644 --- a/pyproject.toml +++ b/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", ] diff --git a/tests/e2e/test_albums_ui.py b/tests/e2e/test_albums_ui.py index 62b4fc2..9f77c47 100644 --- a/tests/e2e/test_albums_ui.py +++ b/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") diff --git a/tests/e2e/test_phase_c_pipeline.py b/tests/e2e/test_phase_c_pipeline.py new file mode 100644 index 0000000..579eba8 --- /dev/null +++ b/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}" diff --git a/tests/story_traceability.json b/tests/story_traceability.json index a98fb0d..6d2a901 100644 --- a/tests/story_traceability.json +++ b/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" ] } }