phase: 124_image_chips_and_answers

All green. Final report: **Phase 124 final verification pass — all criteria verified, no defects found (nothing to fix)** - Verified the implemented phase (all 7 tasks done, changes in working tree): `BOR_ANSWER_IMAGES` toggle, `show_image` tool + refusals + `holder.shown_images`, toggle-gated prompt clause, `answer_image` frame field (None-omitting serializer) + `ChatMessage` persistence, `merge_cited_docs` auto-cite bug fix, answer-figure UI (live/restore/shared/CSS), mock triggers + E2E suite. Also confirmed the bytes route → public change matches the phase-123 shared-chat precedent (task 06 design). - `uv run pytest --cov=app --cov-report=term-missing` → **2848 passed**, TOTAL **99%** (>90% gate) - `bash .agents/validate.sh` → **validation OK** (tests + coverage + ruff + pyright) - `uv run ruff check . && uv run pyright` → clean, 0 errors - `uv run pytest tests/e2e/test_answer_images.py -v --no-cov` (isolated) → **5/5 passed** - Mock-inert spot checks: `tests/e2e/test_image_documents.py` 4/4, `tests/e2e/test_agent_document_tools.py` 4/4 **Completion criteria:** ① zero-read suggested-tier image doc chips with figure — verified (unit matrix, integration `test_zero_read_grounded_turn_auto_cites_suggested_image_doc`, E2E bug-fix flows); ② show_image → answer figure below text/above chips, second call refused, persists + restore + shared — verified (unit handler matrix, integration round-trip, E2E feature flow); ③ default-off byte-identical (3 tools, no clause, no key) — verified (unit + integration + E2E toggle-off); ④ full suite green / >90% / lint clean — verified above; ⑤ commit + phase move — harness responsibility (changes left uncommitted in working tree, no live-infrastructure changes made). **Next pending phase:** none — `todo/` holds only this phase; the pipeline is clear once the harness moves it.
2026-09-26 17:06:09 -04:00
parent 7bd6335929
commit 62e4f6b572
65 changed files with 4909 additions and 46 deletions
@@ -2507,3 +2507,286 @@ def test_shared_chat_serves_the_user_image_path(
    finally:
        db.execute(text("TRUNCATE saved_chats"))
        db.commit()
+
+
+# ---------------------------------------------------------------------------
+# Phase 124, task 04 — the done frame's OPTIONAL answer_image (the
+# shown document's ref: the LLM's ``show_image`` pick) + the byte-
+# identical-off contract (every other turn: no key, no null).
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture()
+def answer_images_on(monkeypatch: pytest.MonkeyPatch) -> None:
+    """The chat endpoint's settings view with ``answer_images`` ON
+    (the ``BOR_ANSWER_IMAGES`` feature path; the monkeypatch fixture
+    reverts the patch, the conftest leak-guard pins the default OFF
+    for every other test in the module)."""
+    settings = Settings(
+        _env_file=None,  # pyright: ignore[reportCallIssue]
+        answer_images=True,
+    )
+    monkeypatch.setattr(chat_api, "get_settings", lambda: settings)
+
+
+def test_show_image_turn_done_frame_carries_answer_image(
+    client, db, seeded_kb: FakeRagLLM, answer_images_on: None
+) -> None:
+    """A grounded turn whose mock client scripts a ``show_image`` tool
+    call on a seeded image doc (the scripted-tool mechanism — the
+    ``use your tools`` precedent) → the final SSE ``done`` frame JSON
+    carries ``answer_image`` with ``image_url ==
+    /api/documents/<id>/image`` and the cited ``sources`` include the
+    same doc (its scripted read — the phase-119 citation tier)."""
+    doc = _seed_image_doc(db)
+    scripted = FakeRagLLM(
+        tool_script=[
+            [
+                ToolCallPiece(
+                    id="call_1",
+                    name="read",
+                    arguments={"path": "docs/pic.png"},
+                )
+            ],
+            [
+                ToolCallPiece(
+                    id="call_2",
+                    name="show_image",
+                    arguments={"path": "docs/pic.png"},
+                )
+            ],
+        ]
+    )
+    fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: scripted
+    try:
+        _, _, frames = _stream_chat(client, QUESTION)
+    finally:
+        fastapi_app.dependency_overrides.clear()
+
+    done = frames[-1]
+    assert done["deflected"] is False
+    assert done["answer_image"] == {
+        "source": "docs",
+        "path": "pic.png",
+        "title": "pic",
+        "image_url": f"/api/documents/{doc.id}/image",
+    }
+    # The cited tier includes the same doc (the scripted read).
+    assert ("docs", "pic.png") in [(s["source"], s["path"]) for s in done["sources"]]
+    # The toggle was honoured on the wire: the first (agent) request
+    # offered the 4th tool.
+    offered = scripted.seen_tools[0]
+    assert offered is not None
+    assert [t["function"]["name"] for t in offered] == [
+        "ls",
+        "read",
+        "grep",
+        "show_image",
+    ]
+
+
+def test_toggle_off_same_flow_offers_three_tools_and_no_answer_image_key(
+    client, db, seeded_kb: FakeRagLLM
+) -> None:
+    """The SAME scripted flow with ``answer_images=False`` (the
+    default) → ``show_image`` is NOT in the offered tools (the mock's
+    recorded request carries exactly the three base tools) and the
+    ``done`` frame has NO ``answer_image`` key — the feature's
+    server-side gate is the OFFER (a model that was never offered the
+    tool never calls it, so no such call occurs in the flow)."""
+    _seed_image_doc(db)  # resolvable, but the tool is never offered
+    scripted = FakeRagLLM(
+        tool_script=[
+            [
+                ToolCallPiece(
+                    id="call_1",
+                    name="read",
+                    arguments={"path": "docs/pic.png"},
+                )
+            ]
+        ]
+    )
+    fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: scripted
+    try:
+        _, _, frames = _stream_chat(client, QUESTION)
+    finally:
+        fastapi_app.dependency_overrides.clear()
+
+    done = frames[-1]
+    assert done["deflected"] is False
+    assert "answer_image" not in done  # ABSENT, not null
+    # The offer: exactly the three base tools — no ``show_image``.
+    offered = scripted.seen_tools[0]
+    assert offered is not None
+    assert [t["function"]["name"] for t in offered] == ["ls", "read", "grep"]
+
+
+def test_zero_tool_grounded_turn_done_frame_has_no_answer_image_key(
+    client, db, seeded_kb: FakeRagLLM, answer_images_on: None
+) -> None:
+    """A zero-tool grounded turn (no scripted calls — the toggle is ON,
+    so the offer carries ``show_image``, but the model never calls it)
+    → the ``done`` frame has NO ``answer_image`` key — byte-identical
+    to pre-phase, checked on the raw wire text (not a re-serialized
+    dict)."""
+    scripted = FakeRagLLM()  # no tool_script: a zero-tool turn
+    fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: scripted
+    try:
+        with client.stream("POST", "/api/chat", json={"message": QUESTION}) as r:
+            assert r.status_code == 200
+            buf = ""
+            raw_frames: list[str] = []
+            for part in r.iter_text():
+                buf += part
+                while "\n\n" in buf:
+                    frame, buf = buf.split("\n\n", 1)
+                    frame = frame.strip()
+                    if frame.startswith("data:"):
+                        raw_frames.append(frame)
+    finally:
+        fastapi_app.dependency_overrides.clear()
+
+    done_raw = [f for f in raw_frames if '"type": "done"' in f]
+    assert len(done_raw) == 1
+    # The BYTE check: the key is absent from the wire text itself.
+    assert "answer_image" not in done_raw[0]
+    done = json.loads(done_raw[0].removeprefix("data:").strip())
+    assert done["deflected"] is False
+    assert "answer_image" not in done
+
+
+def test_deflected_turn_done_frame_has_no_answer_image_key_even_toggle_on(
+    client, db, seeded_kb: FakeRagLLM, answer_images_on: None
+) -> None:
+    """A deflected turn runs no agent and never shows an image (the
+    phase's locked design): even with the toggle ON, the ``done``
+    frame has NO ``answer_image`` key — byte-identical to pre-phase."""
+    scripted = FakeRagLLM()
+    fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: scripted
+    try:
+        _, _, frames = _stream_chat(client, OFF_TOPIC)
+    finally:
+        fastapi_app.dependency_overrides.clear()
+
+    done = frames[-1]
+    assert done["deflected"] is True
+    assert "answer_image" not in done
+
+
+# ---------------------------------------------------------------------------
+# Phase 124, task 05 — bug fix: a suggested-tier IMAGE doc earns its
+# source chip WITHOUT a ``read`` (its seed block IS its entire content —
+# content == summary == the vision description, phase 122 task 03 — so
+# "suggested" ≡ "read" for it and the phase-119 cite discipline is
+# honored, not weakened). Text docs keep the read-docs-only rule.
+# ---------------------------------------------------------------------------
+
+
+def _seed_image_doc_with_chunk(db) -> Document:
+    """An ``is_image`` documents row WITH a retrievable chunk — the
+    phase-122 pipeline embeds the vision description as the image doc's
+    content (the only embedded text of the document), so the fixture
+    chunk mirrors that: a description tuned to overlap ``QUESTION``
+    (the mock embeddings are token-overlap vectors, as in the E2E
+    mock), which lands the image doc in the SUGGESTED tier (top-5
+    fused rank, no floor — A3) where the auto-cite merge operates.
+    The ``tsv`` column is generated (``to_tsvector`` of the content),
+    so the chunk also hits the FTS half. The fixture's teardown
+    truncates the tables."""
+    doc = _seed_image_doc(db)
+    # The phase-122 invariant for image docs: content == summary == the
+    # vision description (the ONE embedded text of the document — no
+    # pointer line on the summary). The description is tuned to overlap
+    # ``QUESTION`` (the mock embeddings are token-overlap vectors, as in
+    # the E2E mock), which lands the image doc in the SUGGESTED tier
+    # (top-5 fused rank, no floor — A3) where the auto-cite merge
+    # operates.
+    description = "Diagram of how my Kubernetes cluster is set up"
+    doc.content = description
+    doc.summary = description
+    db.add(
+        Chunk(
+            id=uuid.uuid4(),
+            document_id=doc.id,
+            position=0,
+            content=description,
+            embedding=_token_vec(description),
+        )
+    )
+    db.commit()
+    return doc
+
+
+def test_zero_read_grounded_turn_auto_cites_suggested_image_doc(
+    client, db, seeded_kb: FakeRagLLM
+) -> None:
+    """The bug fix pinned end-to-end (TODO L3): a grounded turn with
+    NO scripted tool calls (the mock answers straight from the seed —
+    a real model never ``read``s an image doc: its seed block already
+    carries 100% of its content) whose suggested tier contains an
+    image doc → the done frame's ``sources`` include the image ref
+    WITH ``image_url`` set (the chip + figure data — pre-phase-124 a
+    zero-read turn chipped NOTHING, the phase-122 E2E had to script a
+    ``read`` to earn the chip), and ``related`` does NOT contain the
+    same ``(source, path)`` (the auto-cited doc leaves the "Nearby
+    docs" row — no duplicate figure)."""
+    doc = _seed_image_doc_with_chunk(db)
+    scripted = FakeRagLLM()  # no tool_script: a zero-read grounded turn
+    fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: scripted
+    try:
+        _, _, frames = _stream_chat(client, QUESTION)
+    finally:
+        fastapi_app.dependency_overrides.clear()
+
+    done = frames[-1]
+    assert done["deflected"] is False
+    # The image doc IS a citation chip now — and its ref (and ONLY its
+    # ref) carries the bytes route the figure renders from.
+    sources = done["sources"]
+    img_refs = [s for s in sources if (s["source"], s["path"]) == ("docs", "pic.png")]
+    assert len(img_refs) == 1  # once — not duplicated by the read merge
+    assert img_refs[0]["image_url"] == f"/api/documents/{doc.id}/image"
+    for ref in sources:
+        if ref is not img_refs[0]:
+            assert "image_url" not in ref  # the text refs stay pre-122-shaped
+    # Consequence (a): the related tier already excludes cited-seen
+    # members — the auto-cited image doc is NOT also a "nearby doc".
+    related_keys = {(s["source"], s["path"]) for s in done["related"]}
+    assert ("docs", "pic.png") not in related_keys
+    # Consequence (b): the observability fields are UNCHANGED — the
+    # durable record (query_log.sources) carried the suggested image
+    # doc before the phase (it was suggested, not read); auto-citation
+    # touches the citation tier only, not the record.
+    row = db.scalars(select(QueryLog)).one()
+    assert "docs/pic.png" in row.sources
+    assert "docs/homelab/kubernetes.md" in row.sources
+    # The seed contract held: the HIGH prompt seeded the image doc's
+    # summary — its vision description, the ENTIRE content (image docs
+    # store no ``Source:`` pointer line — the summary mirrors the
+    # description verbatim, phase 122 task 03) — as a ``<document>``
+    # block. This is WHY "suggested" ≡ "read" for the doc: a ``read``
+    # would add only the marker line.
+    system = scripted.seen_messages[0][0]["content"]
+    assert '<document source="docs" path="pic.png"' in system
+    assert "Diagram of how my Kubernetes cluster is set up" in system
+
+
+def test_zero_read_text_only_turn_still_chips_nothing(
+    client, db, seeded_kb: FakeRagLLM
+) -> None:
+    """Text docs are unaffected (consequence c): a zero-read grounded
+    turn on the plain text-only KB (no image docs anywhere in the
+    retrieval) → ``sources`` is ``[]`` — byte-identical to pre-phase
+    (no auto-cite without image docs) and no ``image_url`` key on any
+    ref (the omission rule at the frame level)."""
+    fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: seeded_kb
+    try:
+        _, _, frames = _stream_chat(client, QUESTION)
+    finally:
+        fastapi_app.dependency_overrides.clear()
+
+    done = frames[-1]
+    assert done["deflected"] is False
+    assert done["sources"] == []  # phase-119 read-docs-only rule intact
+    for ref in done["sources"] + done["related"]:
+        assert "image_url" not in ref