phase: 123_chat_image_questions
All gates green. Verification complete. **Phase 123 — final verification pass (all 4 tasks already in `complete/`)** - Verified the full implementation is in the working tree: `app/api/chat_images.py` (upload/serve pair), `ChatRequest.image`/`ChatMessage.image` (path-validated, omitted-when-None), toggle-off + stale-file hinted error frames, `build_user_content` multimodal build at both sites (chat.py deflected branch + `run_agent`), config-gated composer attach/preview/upload-then-send, restore + shared rendering, CSP `img-src 'self' data:` carve-out, mock-LLM capture buffer. - `uv run pytest` → **2796 passed**, exit 0 (unit + integration). - `uv run pytest --cov=app --cov-report=term-missing` → **TOTAL 99%** (29/4615 missed; phase-123 modules 99–100%). - `uv run pytest tests/e2e/test_chat_image_questions.py -v --no-cov` → **5 passed** in isolation. - `uv run ruff check . && uv run pyright` → clean (0 errors). **Completion criteria:** (1) attach→send→multimodal text+image to the model, bubble/reload/shared all render it, saved chat stores the PATH with `"base64" not in json.dumps(stored)` — **verified** (E2E tests 1–4 + integration round-trip); (2) `BOR_IMAGES=false` — control hidden, exact hinted error frame, zero model calls / no query_log row — **verified** (E2E test 5 + integration); (3) text-only byte-identical (`content` stays a plain `str`) — **verified** (unit + integration); (4) all gates green — **verified**; (5) commit + phase move — left to the harness per pipeline rules (no `git add`/`commit` run). No defects found; no live-infrastructure changes (repo + local dev DB only). **Next pending phase: none** — 123 is the last phase in `todo/`.
This commit is contained in:
@@ -11,7 +11,9 @@ Requires: podman compose up -d db
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import base64
|
||||
import hashlib
|
||||
import io
|
||||
import json
|
||||
import logging
|
||||
import math
|
||||
@@ -29,6 +31,7 @@ from sqlalchemy import delete, func, select, text
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from app.api import chat as chat_api
|
||||
from app.api import chat_images
|
||||
from app.config import Settings, get_settings
|
||||
from app.main import app as fastapi_app
|
||||
from app.models import Chunk, Document, GitSource, QueryLog
|
||||
@@ -2197,3 +2200,310 @@ def test_text_only_grounded_turn_frame_has_no_image_url_key_anywhere(
|
||||
]
|
||||
for ref in done["sources"] + done["related"]:
|
||||
assert set(ref) == {"source", "path", "title"} # the pre-122 key set
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Phase 123 (task 01): image questions — the upload → chat flow delivers
|
||||
# the multimodal user message to the model (both construction sites), the
|
||||
# toggle-off / stale frames settle before any model call (no record),
|
||||
# text-only turns stay byte-identical, and the saved/shared chat round-
|
||||
# trips the stored PATH (never base64 — LOCKED A5).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
#: A real 1×1 transparent PNG (the phase-122 fixture bytes) — the server
|
||||
#: is content-agnostic (the extension + size gates only), but a well-
|
||||
#: formed fixture keeps the data-URL pin honest.
|
||||
PNG_1X1 = base64.b64decode(
|
||||
"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVR4nGP4DwQACfsD/fteaysAAAAASUVORK5CYII="
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def chat_image_store(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path:
|
||||
"""Point the question-image store (the upload/serve pair) AND the
|
||||
turn pipeline at a tmp dir, with the ``images`` toggle ON — the
|
||||
env-calibrated settings (the conftest mock thresholds) plus the
|
||||
phase-123 pair (``images`` + ``chat_image_dir``); the monkeypatch
|
||||
fixture reverts both patches."""
|
||||
store = tmp_path / "chat-images"
|
||||
store.mkdir()
|
||||
settings = Settings(
|
||||
_env_file=None, # pyright: ignore[reportCallIssue]
|
||||
images=True,
|
||||
chat_image_dir=str(store),
|
||||
)
|
||||
monkeypatch.setattr(chat_images, "get_settings", lambda: settings)
|
||||
monkeypatch.setattr(chat_api, "get_settings", lambda: settings)
|
||||
return store
|
||||
|
||||
|
||||
def _upload_image(client: TestClient, name: str = "screenshot.png") -> str:
|
||||
"""One question-image upload (the composer's step before send) —
|
||||
returns the served path (the value the chat request accepts)."""
|
||||
r = client.post(
|
||||
"/api/chat-images",
|
||||
files={"file": (name, io.BytesIO(PNG_1X1), "image/png")},
|
||||
)
|
||||
assert r.status_code == 200, r.text
|
||||
return r.json()["path"]
|
||||
|
||||
|
||||
def _stream_chat_with(
|
||||
client: TestClient, body: dict[str, Any]
|
||||
) -> tuple[int, str, list[dict[str, Any]]]:
|
||||
"""``_stream_chat`` with an arbitrary body (the ``image`` key)."""
|
||||
with client.stream("POST", "/api/chat", json=body) as r:
|
||||
assert r.status_code == 200
|
||||
assert r.headers["content-type"].startswith("text/event-stream")
|
||||
buf = ""
|
||||
frames: list[dict[str, Any]] = []
|
||||
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:"):
|
||||
frames.append(json.loads(frame.removeprefix("data:").strip()))
|
||||
assert buf.strip() == "", "stream must end on a frame boundary"
|
||||
return r.status_code, r.headers["content-type"], frames
|
||||
|
||||
|
||||
def _expected_image_content(question: str) -> list[dict[str, Any]]:
|
||||
"""The multimodal user content list the model must receive for a
|
||||
question that carried the fixture image (the text part + the
|
||||
image_url part — a data URL built server-side from the stored bytes
|
||||
+ the phase-122 mime map)."""
|
||||
return [
|
||||
{"type": "text", "text": question},
|
||||
{
|
||||
"type": "image_url",
|
||||
"image_url": {
|
||||
"url": f"data:image/png;base64,{base64.b64encode(PNG_1X1).decode('ascii')}"
|
||||
},
|
||||
},
|
||||
]
|
||||
|
||||
|
||||
def test_image_turn_delivers_the_multimodal_user_message(
|
||||
client, db, seeded_kb: FakeRagLLM, chat_image_store: Path
|
||||
) -> None:
|
||||
"""The full flow (LOCKED A5): the client uploads FIRST, then the
|
||||
turn request carries the returned path — and the GROUNDED branch
|
||||
(construction site 2: ``run_agent`` builds its own ``[system,
|
||||
*history, user]`` from the value chat.py passes — the pinned flow)
|
||||
delivers the multimodal content list to the model: the text part +
|
||||
the image_url data URL that decodes to the stored bytes."""
|
||||
fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: seeded_kb
|
||||
try:
|
||||
path = _upload_image(client)
|
||||
_, _, frames = _stream_chat_with(
|
||||
client, {"message": QUESTION, "image": path}
|
||||
)
|
||||
finally:
|
||||
fastapi_app.dependency_overrides.clear()
|
||||
|
||||
assert frames[-1]["type"] == "done"
|
||||
assert frames[-1]["deflected"] is False
|
||||
assert len(seeded_kb.seen_messages) == 1
|
||||
assert seeded_kb.seen_messages[0][-1] == {
|
||||
"role": "user",
|
||||
"content": _expected_image_content(QUESTION),
|
||||
}
|
||||
# the data URL really is the stored bytes (decode + compare)
|
||||
url = seeded_kb.seen_messages[0][-1]["content"][1]["image_url"]["url"]
|
||||
assert base64.b64decode(url.split(",", 1)[1]) == PNG_1X1
|
||||
|
||||
|
||||
def test_deflected_image_turn_delivers_the_multimodal_user_message(
|
||||
client, db, seeded_kb: FakeRagLLM, chat_image_store: Path
|
||||
) -> None:
|
||||
"""Construction site 1 (the deflected branch consumes chat.py's
|
||||
``messages`` list directly): an off-topic question + image still
|
||||
delivers the SAME multimodal list — the LOW prompt path never
|
||||
drops the attachment."""
|
||||
fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: seeded_kb
|
||||
try:
|
||||
path = _upload_image(client)
|
||||
_, _, frames = _stream_chat_with(
|
||||
client, {"message": OFF_TOPIC, "image": path}
|
||||
)
|
||||
finally:
|
||||
fastapi_app.dependency_overrides.clear()
|
||||
|
||||
assert frames[-1]["type"] == "done"
|
||||
assert frames[-1]["deflected"] is True
|
||||
assert len(seeded_kb.seen_messages) == 1
|
||||
assert seeded_kb.seen_messages[0][-1] == {
|
||||
"role": "user",
|
||||
"content": _expected_image_content(OFF_TOPIC),
|
||||
}
|
||||
|
||||
|
||||
def test_image_turn_with_toggle_off_yields_the_hinted_frame_and_calls_nothing(
|
||||
client, db, seeded_kb: FakeRagLLM, chat_image_store: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""The ``BOR_IMAGES`` gate: off with an image set → the phase-114
|
||||
error frame with the EXACT detail + hint (ONE terminal frame — no
|
||||
``done``), and NO model call (zero embeds, zero requests) and NO
|
||||
record (the rejected turn saves nothing — the existing error-path
|
||||
convention)."""
|
||||
monkeypatch.setattr(
|
||||
chat_api,
|
||||
"get_settings",
|
||||
lambda: Settings(
|
||||
_env_file=None, # pyright: ignore[reportCallIssue]
|
||||
images=False,
|
||||
chat_image_dir=str(chat_image_store),
|
||||
),
|
||||
)
|
||||
fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: seeded_kb
|
||||
try:
|
||||
path = _upload_image(client)
|
||||
_, _, frames = _stream_chat_with(
|
||||
client, {"message": "What is in this image?", "image": path}
|
||||
)
|
||||
finally:
|
||||
fastapi_app.dependency_overrides.clear()
|
||||
|
||||
assert [f["type"] for f in frames] == ["error"]
|
||||
assert frames[0] == {
|
||||
"type": "error",
|
||||
"detail": "Image support is turned off on this server.",
|
||||
"hint": "Enable BOR_IMAGES in the server's .env (and restart) to ask with an image.",
|
||||
}
|
||||
# NO model call: the QUESTION embed never ran (``question_embeds``
|
||||
# counts ``embed_one`` calls — the import's batch embeds are a
|
||||
# different counter) and the answer endpoint was never asked.
|
||||
assert seeded_kb.question_embeds == []
|
||||
assert seeded_kb.seen_messages == []
|
||||
assert db.scalars(select(QueryLog)).all() == [] # NO query_log row
|
||||
|
||||
|
||||
def test_image_turn_with_a_missing_stored_file_yields_the_stale_frame(
|
||||
client, db, seeded_kb: FakeRagLLM, chat_image_store: Path
|
||||
) -> None:
|
||||
"""The stale-path edge (the file was deleted out-of-band): the SAME
|
||||
frame shape with the "no longer available" detail and NO hint (the
|
||||
banner's default copy is the honest fallback) — again before any
|
||||
model call, no record."""
|
||||
fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: seeded_kb
|
||||
try:
|
||||
stale = "/api/chat-images/" + "e" * 32 + ".png" # well-formed, absent
|
||||
_, _, frames = _stream_chat_with(
|
||||
client, {"message": "What is in this image?", "image": stale}
|
||||
)
|
||||
finally:
|
||||
fastapi_app.dependency_overrides.clear()
|
||||
|
||||
assert [f["type"] for f in frames] == ["error"]
|
||||
assert frames[0] == {
|
||||
"type": "error",
|
||||
"detail": "That image is no longer available.",
|
||||
"hint": None,
|
||||
}
|
||||
assert seeded_kb.question_embeds == [] # NO model call (see the toggle-off test)
|
||||
assert seeded_kb.seen_messages == []
|
||||
assert db.scalars(select(QueryLog)).all() == []
|
||||
|
||||
|
||||
def test_text_only_turn_is_byte_identical_with_images_enabled(
|
||||
client, db, seeded_kb: FakeRagLLM, chat_image_store: Path
|
||||
) -> None:
|
||||
"""``image=None`` with the toggle ON: the user message is the PLAIN
|
||||
STRING (not a content list) — the multimodal branch is inert, and
|
||||
the turn behaves exactly as pre-phase (done frame, the question
|
||||
embedded whole)."""
|
||||
fastapi_app.dependency_overrides[chat_api.get_llm] = lambda: seeded_kb
|
||||
try:
|
||||
_, _, frames = _stream_chat_with(client, {"message": QUESTION})
|
||||
finally:
|
||||
fastapi_app.dependency_overrides.clear()
|
||||
|
||||
assert frames[-1]["type"] == "done"
|
||||
assert seeded_kb.seen_messages[0][-1] == {"role": "user", "content": QUESTION}
|
||||
assert isinstance(seeded_kb.seen_messages[0][-1]["content"], str)
|
||||
assert seeded_kb.question_embeds == [QUESTION[: 1200]]
|
||||
|
||||
|
||||
def test_saved_chat_round_trips_the_user_image_path(
|
||||
client, db, chat_image_store: Path
|
||||
) -> None:
|
||||
"""Persistence (LOCKED A5): the user record carries the stored
|
||||
PATH — the stored JSONB and the saved-chat response round-trip it,
|
||||
the brain record carries NO image key, and NOTHING base64 crosses
|
||||
the storage boundary. A text-only record stays without the key
|
||||
(the phase-50 byte-identical contract for text-only chats)."""
|
||||
db.execute(text("TRUNCATE saved_chats"))
|
||||
db.commit()
|
||||
try:
|
||||
path = _upload_image(client)
|
||||
r = client.post(
|
||||
"/api/chats",
|
||||
json={
|
||||
"messages": [
|
||||
{"who": "user", "text": "What is in this image?", "image": path},
|
||||
{"who": "brain", "text": "A cat, by the look of it."},
|
||||
]
|
||||
},
|
||||
)
|
||||
assert r.status_code == 201, r.text
|
||||
body = r.json()
|
||||
assert body["messages"][0]["image"] == path
|
||||
assert "image" not in body["messages"][1] # brain records never carry it
|
||||
|
||||
# The RAW stored JSONB: the path, never base64 (LOCKED A5)
|
||||
raw = db.execute(
|
||||
text("SELECT messages FROM saved_chats WHERE id = :id"),
|
||||
{"id": body["id"]},
|
||||
).scalar_one()
|
||||
assert raw[0]["image"] == path
|
||||
assert "base64" not in json.dumps(raw)
|
||||
|
||||
# The admin GET round-trips it losslessly
|
||||
got = client.get(f"/api/chats/{body['id']}")
|
||||
assert got.status_code == 200
|
||||
assert got.json()["messages"][0]["image"] == path
|
||||
assert "image" not in got.json()["messages"][1]
|
||||
finally:
|
||||
db.execute(text("TRUNCATE saved_chats"))
|
||||
db.commit()
|
||||
|
||||
|
||||
def test_shared_chat_serves_the_user_image_path(
|
||||
client, db, chat_image_store: Path
|
||||
) -> None:
|
||||
"""The shared view is faithful: the public snapshot carries the
|
||||
user's image path (the public ``messages`` shape already includes
|
||||
the optional key), and the image bytes themselves are publicly
|
||||
servable (the image is part of the chat's content — phase 55 A1)."""
|
||||
db.execute(text("TRUNCATE saved_chats"))
|
||||
db.commit()
|
||||
try:
|
||||
path = _upload_image(client)
|
||||
r = client.post(
|
||||
"/api/chats",
|
||||
json={
|
||||
"share": True,
|
||||
"messages": [
|
||||
{"who": "user", "text": "What is in this image?", "image": path},
|
||||
{"who": "brain", "text": "A cat, by the look of it."},
|
||||
],
|
||||
},
|
||||
)
|
||||
assert r.status_code == 201, r.text
|
||||
share_url = r.json()["share_url"]
|
||||
token = share_url.rsplit("/", 1)[-1]
|
||||
|
||||
anon = TestClient(fastapi_app)
|
||||
got = anon.get(f"/api/shared/{token}")
|
||||
assert got.status_code == 200
|
||||
messages = got.json()["messages"]
|
||||
assert messages[0]["image"] == path
|
||||
assert "image" not in messages[1]
|
||||
|
||||
img = anon.get(path)
|
||||
assert img.status_code == 200
|
||||
assert img.content == PNG_1X1
|
||||
finally:
|
||||
db.execute(text("TRUNCATE saved_chats"))
|
||||
db.commit()
|
||||
|
||||
@@ -24,6 +24,7 @@ Requires: podman compose up -d db
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import re
|
||||
import time
|
||||
import uuid
|
||||
@@ -799,6 +800,105 @@ def test_public_read_returns_snapshot_without_private_keys(
|
||||
assert body["messages"] == _expect([_user(FIRST_QUESTION), FULL_BRAIN])
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Phase 123 (tasks 03/04): the question's attached image — the saved
|
||||
# and shared shape carries the stored PATH (LOCKED A5: never base64),
|
||||
# on the USER record only (the attachment belongs to the question — a
|
||||
# brain record never carries the key). Text-only records stay WITHOUT
|
||||
# the key (absent, never null — the phase-50 byte-identical contract,
|
||||
# pinned by ``_expect`` above; the ``ChatMessage`` wrap serializer does
|
||||
# the dropping, so every surface — the stored JSONB, the admin GET,
|
||||
# the public shared read — inherits it).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
#: A well-formed stored path (the ``POST /api/chat-images`` response's
|
||||
#: shape — the chat-images router names the file
|
||||
#: ``<uuid4().hex>.<ext>``; the API boundary itself only bounds the
|
||||
#: string to 500 chars, the pattern gate is the chat request's).
|
||||
IMAGE_PATH = "/api/chat-images/" + "a" * 32 + ".png"
|
||||
|
||||
|
||||
def _user_with_image(text: str) -> dict[str, Any]:
|
||||
return {"who": "user", "text": text, "image": IMAGE_PATH}
|
||||
|
||||
|
||||
def test_create_round_trips_the_user_image_path(
|
||||
admin_client: TestClient, db: Session
|
||||
) -> None:
|
||||
"""The user record carries the PATH at every boundary: the 201
|
||||
body, the RAW stored JSONB (never base64 — the A5 contract), and
|
||||
the admin GET — and the brain record stays WITHOUT the key.
|
||||
A text-only record in the SAME chat stays key-free too."""
|
||||
r = admin_client.post(
|
||||
"/api/chats",
|
||||
json={
|
||||
"messages": [
|
||||
_user(FIRST_QUESTION), # text-only — stays WITHOUT the key
|
||||
_user_with_image("What is in this image?"),
|
||||
{"who": "brain", "text": "A cat, by the look of it."},
|
||||
]
|
||||
},
|
||||
)
|
||||
assert r.status_code == 201, r.text
|
||||
body = r.json()
|
||||
assert "image" not in body["messages"][0]
|
||||
assert body["messages"][1]["image"] == IMAGE_PATH
|
||||
assert "image" not in body["messages"][2]
|
||||
|
||||
# The RAW stored JSONB (the DB is the durable boundary — the
|
||||
# served bodies could in principle re-derive it): the path, and
|
||||
# NOTHING base64 anywhere in the payload.
|
||||
raw = db.execute(
|
||||
text("SELECT messages FROM saved_chats WHERE id = :id"),
|
||||
{"id": body["id"]},
|
||||
).scalar_one()
|
||||
assert "image" not in raw[0]
|
||||
assert raw[1]["image"] == IMAGE_PATH
|
||||
assert "image" not in raw[2]
|
||||
assert "base64" not in json.dumps(raw)
|
||||
|
||||
got = admin_client.get(f"/api/chats/{body['id']}")
|
||||
assert got.status_code == 200
|
||||
assert "image" not in got.json()["messages"][0]
|
||||
assert got.json()["messages"][1]["image"] == IMAGE_PATH
|
||||
assert "image" not in got.json()["messages"][2]
|
||||
|
||||
|
||||
def test_shared_serve_includes_the_user_image_path(
|
||||
admin_client: TestClient,
|
||||
) -> None:
|
||||
"""The shared view is faithful (task 03's pin): the PUBLIC
|
||||
snapshot's user record carries the image path — the public
|
||||
``messages`` shape already gains the one optional key, so no new
|
||||
shared-shape field exists — and the brain / text-only records
|
||||
stay WITHOUT it. The image bytes themselves ride the public
|
||||
serve route (phase 55 A1 — the image is part of the chat's
|
||||
content, the token is the credential): pinned in
|
||||
``test_chat_api.py`` alongside the upload."""
|
||||
r = admin_client.post(
|
||||
"/api/chats",
|
||||
json={
|
||||
"messages": [
|
||||
_user(FIRST_QUESTION), # text-only — stays WITHOUT the key
|
||||
_user_with_image("What is in this image?"),
|
||||
{"who": "brain", "text": "A cat, by the look of it."},
|
||||
]
|
||||
},
|
||||
)
|
||||
assert r.status_code == 201, r.text
|
||||
share_url = _share(admin_client, r.json()["id"])["share_url"]
|
||||
|
||||
anon = TestClient(fastapi_app) # fresh jar: truly anonymous
|
||||
got = anon.get(f"/api{share_url}")
|
||||
assert got.status_code == 200
|
||||
body = got.json()
|
||||
assert set(body) == SHARED_OUT_KEYS # no new shared-shape field
|
||||
messages = body["messages"]
|
||||
assert "image" not in messages[0]
|
||||
assert messages[1]["image"] == IMAGE_PATH
|
||||
assert "image" not in messages[2]
|
||||
|
||||
|
||||
def test_public_read_wrong_and_revoked_tokens_404_with_one_detail(
|
||||
admin_client: TestClient,
|
||||
) -> None:
|
||||
|
||||
@@ -8,7 +8,11 @@ API JSON, static assets, even the static catch-all's 404s — carries:
|
||||
|
||||
* ``Content-Security-Policy`` — the exact A1 string (``default-src 'self';
|
||||
base-uri 'none'; frame-ancestors 'none'`` → clickjacking closed, no
|
||||
inline anything because the No-CDN frontend has none);
|
||||
inline anything because the No-CDN frontend has none), extended by
|
||||
the phase-123 ``img-src 'self' data:`` carve-out (the question-image
|
||||
composer's data-URL preview + live bubble — see ``CSP`` in
|
||||
``app/core/security_headers.py``; the ``data:`` allowance is scoped
|
||||
to img-src only);
|
||||
* ``X-Frame-Options: DENY`` — the legacy no-framing fallback;
|
||||
* ``X-Content-Type-Options: nosniff`` — the MIME-confusion belt.
|
||||
|
||||
@@ -69,9 +73,7 @@ def test_page_carries_all_three_headers(client: TestClient, db: Session) -> None
|
||||
response = client.get("/")
|
||||
assert response.status_code == 200
|
||||
_assert_security_headers(response)
|
||||
assert response.headers["content-security-policy"] == (
|
||||
"default-src 'self'; base-uri 'none'; frame-ancestors 'none'"
|
||||
)
|
||||
assert response.headers["content-security-policy"] == CSP
|
||||
|
||||
|
||||
def test_api_health_carries_all_three_headers(client: TestClient) -> None:
|
||||
|
||||
Reference in New Issue
Block a user