diff --git a/.agents/phases/todo/120_failed_turn_retry/00_phase.md b/.agents/phases/todo/120_failed_turn_retry/00_phase.md new file mode 100644 index 0000000..b2e0a11 --- /dev/null +++ b/.agents/phases/todo/120_failed_turn_retry/00_phase.md @@ -0,0 +1,51 @@ +# Phase 120 — Failed-turn retry: network errors and refresh survive a failed turn + +**Source:** `TODO.md` L3–4 — "Retry doesn't seem to work on network error" + "Refreshing the page after an error shows only the chat message you sent and no options to retry the message, forcing the user to click 'new chat' or be stuck." +**Story:** n/a (bug-fix follow-up; extends the phase-49/53 redo-in-place retry, phase-67 LLM retry, and phase-111 banner Retry assets). +**Context:** `frontend/assets/app.js` — `showErrorBanner(detail, opts)` (L2210) reveals `#banner-retry` only when `opts.retryable && lastBrainWrap` (L2222); `retryLastTurn(wrap)` (L2276) pops the LAST brain record and re-asks the user question immediately before it (the invariant `every brain record follows its user record`); `rememberBrainTurn(rawText, meta, replaceIndex)` (L2108) pushes/replaces the brain record in `conversation` + `saveConversation()` + `persistConversation()` (the phase-55 auto-save rides the same call). `lastBrainWrap` is assigned only on three paths: the `done` settle (L2609), the zero-frame fallback bubble (L2671), and the user-stop finalize (L2699) — **never on a turn error**. The error catch (the `else` branch at ~L2690) calls `setUiState(UI_STATE.error, detail, { hint })` with NO brain record persisted, whether or not a partial `wrap` exists. The stream-drop guard (~L2651, `!sawDone && !aborted && (acc || thinkingAcc)`) also lands in the error state with nothing persisted. Restore: `renderStoredMessage(m)` (L1642) renders `m.stopped` via `appendStoppedNote` (L486); the restore loop sets `lastBrainWrap` on the last restored brain bubble (L1694) and calls `markLastRetryable()` (L1708, L560 — removes all `.retry-btn`, re-adds on the LAST `.brain-wrap`). `app/schemas.py` — `ChatMessage` (L742) is `extra="forbid"` with fields `who`, `text` (≤32 000), `sources?`, `related?`, `deflected?`, `suggestions?`, `thinking?` (≤32 000), `tools?`, `stopped?: bool | None` (L786); `SavedChatCreate/Update` messages are non-empty, ≤200 (phase 83). `tests/e2e/mock_llm.py` + `tests/e2e/test_llm_retry.py` hold the existing LLM-failure mock pattern for the E2E. + +## Objective +A failed chat turn — network error (zero frames), SSE `error` frame, or mid-stream drop — leaves a **retryable error state** both live (the banner Retry and an in-bubble Retry both work) and after a page refresh (the failed turn restores as an error bubble with a working Retry button). No failed turn strands the user with a bare question and no recovery. + +## Dependencies +- `119_name_signal_read_chips` (complete) — pipeline predecessor (execution order) only. +- Code dependencies (all complete): phase 49/53 `retryLastTurn` redo-in-place, phase 111 `#banner-retry`, phase 48 `stopped` persistence + `appendStoppedNote` pattern, phase 55 auto-save riding `rememberBrainTurn`. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **Failed record (task 01, server side):** two new OPTIONAL fields on `ChatMessage`, the phase-48 `stopped` precedent (L786): `failed: bool | None = None` and `error: str | None = Field(default=None, max_length=500)` (the persisted error detail; 500 caps a hostile detail string in the phase-83 style). `extra="forbid"` stays — the keys are now declared, unknown keys still 422. No change in `app/api/chats.py` logic (the schema flows through `SavedChatCreate/Update`); the shared-chat shape (`SharedChatOut.messages`) carries failed records verbatim (text renders as-is on the shared page — no change needed there). +- **Failed turn = a brain record (LOCKED A1):** a failed turn persists `{ who: "brain", text: , failed: true, error: }` via `rememberBrainTurn` — so it lands in localStorage AND the server-side saved chat through the existing phase-55 auto-save ride. There is no separate error table and no new API: `retryLastTurn`'s pop-the-last-brain-record-then-re-ask-the-preceding-question logic works on a failed record UNCHANGED (the invariant holds — the question's user record immediately precedes it). +- **Live error paths (task 01, frontend):** the error catch's `else` branch (non-abort, non-stop) and the stream-drop guard BOTH funnel into one new helper `finalizeFailedTurn(detail, { acc, thinking, tools })`: + - **Partial exists** (`wrap` with streamed text): close the thinking block + tool calls (the stop-finalize pattern), `appendFailedNote(wrap, detail)` (new, mirrors `appendStoppedNote` L486 — an in-bubble error line with the detail), persist via `rememberBrainTurn(acc, { thinking, tools, failed: true, error: detail }, leavePartialIndex)`, `lastBrainWrap = wrap`. + - **No wrap** (network error, zero frames): create a brain bubble with a fixed fallback text (a short honest "my answer didn't make it" line — NOT the `EMPTY_ANSWER_FALLBACK` answer text; the `appendFailedNote` carries the real detail), persist the same record shape, `lastBrainWrap = fwrap`. + - Then `markLastRetryable()` — the in-bubble Retry button appears, and `showErrorBanner`'s existing `opts.retryable && lastBrainWrap` condition (L2222) now holds on a turn error, so the phase-111 banner Retry appears too — **no change to `showErrorBanner`** (it binds `() => retryLastTurn(lastBrainWrap)` at reveal; `lastBrainWrap` is set before `setUiState(UI_STATE.error, …)` runs). + - The zero-frame-but-stream-completed case keeps its existing fallback bubble (L2663–2673) — now ALSO marked `failed: true` + error note (it is a failed turn; the bubble text stays `EMPTY_ANSWER_FALLBACK` so the record keeps a meaningful `text`). +- **Restore (task 02):** `renderStoredMessage(m)` gains the failed branch — a `m.failed` record renders as a brain bubble (the persisted `text`), gets `appendFailedNote(wrap, m.error)`, and gets NO Save-as-doc button (a note, not an answer — the `m.stopped` exclusion at L1687 precedent: `if (!m.stopped && !m.failed) appendSaveAsDocButton(…)`). No other restore change is required: the restore loop's `lastBrainWrap = wrap` (L1694) + `markLastRetryable()` (L1708) already target the last `.brain-wrap`, which is now the failed bubble → the in-bubble Retry button renders on refresh. `retryLastTurn` needs no change (the failed record is the last brain record; its preceding user record is the question). +- **Interaction with `stopped`:** a turn is either stopped (user engaged, partial kept, `stopped: true`) or failed (`failed: true`) — mutually exclusive by construction (the stop path is the catch's `stoppedByUser`/`AbortError` branch, which this phase does not touch). +- **NOT touched:** `retryLastTurn` itself, the stop path, the done path, the server save/restore API logic (schema fields only), the shared page rendering, and every non-chat `showErrorBanner` caller. + +## Tasks +1. `01_persist_failed_turn.md` — `ChatMessage.failed`/`error` fields + the live error paths persist a failed brain record with a rendered error bubble (banner Retry works on network errors). +2. `02_restore_failed_turn.md` — restore renders a `failed` record as an error bubble with a working Retry button (the refresh case). +3. `03_failed_turn_tests.md` — unit + integration + isolated E2E `test_failed_turn_retry.py`. + +## Testing & Quality +- Unit: `tests/unit/test_chat_message_failed.py` (new, task 03) — `ChatMessage` accepts `failed`/`error`, `error` >500 chars 422s, unknown keys still 422, omitted keys round-trip `None`; `tests/unit/test_frontend_failed_turn.py` (new, task 03) — house-style source assertions: the error catch + stream-drop guard route through the failed-turn finalize (persist `failed: true`, call `markLastRetryable`), `appendFailedNote` exists and mirrors the stopped-note structure, the restore branch renders the note and excludes Save-as-doc, `showErrorBanner` is byte-unchanged (the `lastBrainWrap` condition untouched). +- Integration: `tests/integration/test_chats_api.py` (extend) — `POST`/`PUT /api/chats` with a `failed: true` + `error` record round-trips byte-identically (the phase-50 contract); a shared chat carrying a failed record still serves (public shape unchanged). +- E2E: `tests/e2e/test_failed_turn_retry.py` (new, task 03) — run in isolation per AGENTS.md §4. Scenarios (the `mock_llm.py` failure pattern from `test_llm_retry.py`): (A) network-class failure (zero frames) → banner with a visible Retry → click re-asks without re-typing; (B) SSE `error` frame after partial deltas → partial bubble keeps its text + error note + Retry → click re-asks; (C) reload the page after a failed turn → the failed bubble restores with a working Retry button → click re-asks. +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] A network-error turn shows a Retry (banner and/or in-bubble); clicking it re-asks the last question without re-typing. +- [ ] Reloading the page after a failed turn shows the failed bubble (with the error detail) and a working Retry — no "new chat" required. +- [ ] Stopped turns (phase 48) and successful turns behave byte-identically to before. +- [ ] `uv run pytest` green; `uv run pytest --cov=app --cov-report=term-missing` TOTAL >90%; `uv run ruff check . && uv run pyright` clean. +- [ ] One `--no-gpg-sign` commit; phase dir moved to `.agents/phases/complete/` by the pipeline gate. + +## Locked decisions +- **A1 — a failed turn persists as a brain record with a `failed` marker (+ capped `error` detail), the phase-48 `stopped` precedent; no separate error table, no new API, `retryLastTurn` reused unchanged (owner-confirmed 2026-09-24, roadmap confirmation).** +- **Banner Retry stays as-is** — the phase-111 `opts.retryable && lastBrainWrap` condition is kept; this phase makes `lastBrainWrap` exist on the error paths so the existing button finally appears (owner-confirmed: same mechanism, no `showErrorBanner` change). + +## Commit +```bash +git add app/ frontend/ tests/ .agents/phases/ && git commit --no-gpg-sign -m "fix(chat): persist failed turns so retry works on network errors and survives a refresh" +``` diff --git a/.agents/phases/todo/120_failed_turn_retry/01_persist_failed_turn.md b/.agents/phases/todo/120_failed_turn_retry/01_persist_failed_turn.md new file mode 100644 index 0000000..c280e99 --- /dev/null +++ b/.agents/phases/todo/120_failed_turn_retry/01_persist_failed_turn.md @@ -0,0 +1,37 @@ +# Task 01 — Persist the failed turn: schema fields + live error paths + +**Phase:** `120_failed_turn_retry` · **Source:** `TODO.md:3–4` — "Retry doesn't seem to work on network error" + "Refreshing the page after an error shows only the chat message you sent and no options to retry the message, forcing the user to click 'new chat' or be stuck." + +## Objective +Every failed chat turn (network error, SSE `error` frame, stream drop) persists a `failed: true` brain record and renders a retryable error bubble live — so the phase-111 banner Retry finally appears on network errors (its `lastBrainWrap` precondition now holds). + +## Work +1. `app/schemas.py` — `ChatMessage` (L742, `extra="forbid"`): add, next to `stopped` (L786), + ```python + failed: bool | None = None + error: str | None = Field(default=None, max_length=500) + ``` + Extend the docstring: the phase-48 `stopped` precedent — a FAILED turn (network/SSE error/stream drop) stores `{who: "brain", text: , failed: true, error: }`; `error` is the persisted banner detail, capped at 500 (phase-83 value-bounds style). No serializer change — `None` values flow as absent/None exactly like `stopped` today (the phase-50 byte-identical round-trip contract covers the new keys automatically). +2. `frontend/assets/app.js`: + - New `appendFailedNote(wrap, detail)` — mirror of `appendStoppedNote` (L486): one `.failed-note` per bubble (guard query), a visible error line inside the brain bubble carrying `detail` (the banner keeps its role="alert" summary; the note is the in-bubble, refresh-surviving copy). + - New `finalizeFailedTurn(detail, { acc, thinking, tools, wrap, leavePartialIndex })` — the single funnel for every non-stop, non-abort turn failure: + - `wrap` exists (partial streamed): `closeThinkingBlock(wrap)` + `closeToolCalls(wrap)` (the stop-finalize pattern, ~L2685), `appendFailedNote(wrap, detail)`, `rememberBrainTurn(acc, { thinking: thinking || undefined, tools: tools.length ? tools : undefined, failed: true, error: detail }, leavePartialIndex)`, `lastBrainWrap = wrap`. + - no `wrap` (network error, zero frames): `fwrap = addMessage("brain", FAILED_TURN_TEXT)` where `FAILED_TURN_TEXT` is a new short honest constant ("My answer didn't make it — the connection dropped. Use Retry to ask again.") — NOT `EMPTY_ANSWER_FALLBACK` (that constant stays for the zero-frame-but-completed case); `appendFailedNote(fwrap, detail)`, `rememberBrainTurn(FAILED_TURN_TEXT, { failed: true, error: detail }, leavePartialIndex)`, `lastBrainWrap = fwrap`. + - end with `markLastRetryable()`. + - `detail` is the trimmed error string, truncated to 500 chars before persistence (the schema cap is the backstop). + - The error catch's `else` branch (~L2690, currently `setUiState(UI_STATE.error, detail, { hint })` with no persistence): call `finalizeFailedTurn(detail, {…})` BEFORE `setUiState(UI_STATE.error, detail, err.hint ? { hint: err.hint } : {})` (the banner stays — now with its Retry revealed because `lastBrainWrap` is set). + - The stream-drop guard (~L2651, `!sawDone && !aborted && (acc || thinkingAcc)` → currently a bare `setUiState(UI_STATE.error, "The stream ended before my answer finished — try again?")`): route through the same `finalizeFailedTurn` with that detail (the partial persists as failed — a half-answer is a failed answer, and Refresh must restore what the user saw + a Retry). + - The zero-frame-but-completed fallback bubble (L2663–2673): add `failed: true` + `error: "The model answered with nothing."` to its `rememberBrainTurn` call and `appendFailedNote(fwrap, …)` — the bubble text stays `EMPTY_ANSWER_FALLBACK`. + - Do NOT touch: `showErrorBanner` (L2210), `retryLastTurn` (L2276), the stop branch, the done settle (L2609), `markLastRetryable` (L560). +3. `frontend/assets/styles.css` — `.failed-note`: the in-bubble error line treatment (the `.stopped-note` family, error-colored per the current theme's error token — contrast ≥4.5:1, PLAN §7). +4. ASSUMPTION: the zero-frame-but-completed case (stream returns, no events, no throw) is also marked failed — it is a failed turn, and its record previously persisted with no marker (inconsistent with the refresh case this phase fixes). + +## Testing & Quality +- Unit: `tests/unit/test_chat_message_failed.py` (shipped with task 03's test task — this task ships the code): `ChatMessage` accepts `failed: true` + `error`; `error` >500 chars → 422; an unknown key still → 422; a record without the new keys is byte-identical to before. `tests/unit/test_frontend_failed_turn.py` (task 03): the catch `else` branch + stream-drop guard + zero-frame fallback all route through the failed finalize (persist `failed: true`, call `markLastRetryable`); `appendFailedNote` exists; `showErrorBanner` and `retryLastTurn` sources are untouched (byte-pinned). +- Coverage: **>90%** on `app/` for the schema change (the frontend JS is pinned by source-assertion unit tests). + +## Completion Criteria +- [ ] `ChatMessage` round-trips `failed`/`error` (unit tests green). +- [ ] A zero-frame network error (E2E scenario A, task 03) shows the banner WITH a visible Retry button and a failed bubble with the error detail. +- [ ] No call site outside the three failed paths persists `failed: true` (grep). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/120_failed_turn_retry/02_restore_failed_turn.md b/.agents/phases/todo/120_failed_turn_retry/02_restore_failed_turn.md new file mode 100644 index 0000000..87861e6 --- /dev/null +++ b/.agents/phases/todo/120_failed_turn_retry/02_restore_failed_turn.md @@ -0,0 +1,25 @@ +# Task 02 — Restore the failed turn: refresh keeps the Retry + +**Phase:** `120_failed_turn_retry` · **Source:** `TODO.md:4` — "Refreshing the page after an error shows only the chat message you sent and no options to retry the message, forcing the user to click 'new chat' or be stuck." + +## Objective +A `failed` record restores as an error bubble (persisted text + error note) that carries a working Retry button — the refreshed page is the same retryable state the live error was, and Retry re-asks the question through the unchanged `retryLastTurn`. + +## Work +1. `frontend/assets/app.js` — `renderStoredMessage(m)` (L1642): add the failed branch, mirroring the `m.stopped` handling (L1685–1688): + - a `m.failed` brain record renders its `text` (the persisted detail or fallback line) as the bubble content, then `appendFailedNote(wrap, m.error)` (the note is absent when `m.error` is null — the record's `text` already carries it), and + - `if (!m.stopped && !m.failed) appendSaveAsDocButton(wrap, m.text);` — a failed turn is a note, not an answer (the stopped exclusion precedent at L1687). + - No Tune button for failed records either (a note, not an answer — same scope as the `m.stopped` exclusion around L584–588 if the restore call site applies it there). + - No other restore change: the restore loop already sets `lastBrainWrap = wrap` on the last restored brain bubble (L1694) and calls `markLastRetryable()` (L1708), which removes every `.retry-btn` and re-adds it on the LAST `.brain-wrap` — the failed bubble. `retryLastTurn` works unchanged: the failed record is the last brain record, the user record immediately before it is the question (the invariant holds by construction — task 01 persists the brain record right after the user record), so the redo-in-place pops the failed record and re-asks. +2. `frontend/assets/app.js` — the shared-chat restore (`frontend/assets/shared.js` / the shared page): failed records render their `text` as a plain brain bubble (no note, no Retry — the shared view is read-only and text-only by design; no change beyond confirming the `renderStoredMessage`-equivalent there does not choke on the unknown-looking `failed`/`error` keys — it renders `text` only). +3. ASSUMPTION: a failed bubble restored at the END of the conversation gets the Retry; a failed bubble in the MIDDLE of a longer conversation does not (the phase-49 last-bubble-only rule, unchanged). + +## Testing & Quality +- Unit: `tests/unit/test_frontend_failed_turn.py` (task 03): the restore branch renders the failed note, excludes Save-as-doc (and Tune, where applicable), and the restore path is the only place `m.failed` is read for rendering; the shared page renders failed records text-only. +- E2E: scenario C of `tests/e2e/test_failed_turn_retry.py` (task 03) pins this task end-to-end. +- Coverage: n/a (frontend) — the validate.sh `app/` gate must stay green. + +## Completion Criteria +- [ ] Reload after a failed turn (E2E scenario C): the failed bubble shows with its error detail and a Retry button; clicking Retry re-asks the preceding question without re-typing and the failed record is replaced by the new answer. +- [ ] A mid-conversation failed record restores with NO Retry button (last-bubble-only rule intact). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/120_failed_turn_retry/03_failed_turn_tests.md b/.agents/phases/todo/120_failed_turn_retry/03_failed_turn_tests.md new file mode 100644 index 0000000..31a5c44 --- /dev/null +++ b/.agents/phases/todo/120_failed_turn_retry/03_failed_turn_tests.md @@ -0,0 +1,34 @@ +# Task 03 — Failed-turn tests: unit + integration + isolated E2E + +**Phase:** `120_failed_turn_retry` · **Source:** `TODO.md:3–4` — "Retry doesn't seem to work on network error" + "Refreshing the page after an error shows only the chat message you sent and no options to retry the message …" + +## Objective +Pin the whole failed-turn contract: the schema boundary, the save/restore round-trip, the three live failure paths, and the two user scenarios (retry on network error; retry after refresh) as an isolated Playwright suite. + +## Work +1. `tests/unit/test_chat_message_failed.py` (new): + - `ChatMessage` accepts `{who: "brain", text: "…", failed: true, error: "detail"}`; `error` of 501 chars → 422; an unknown key (e.g. `"foo": 1`) → 422 (`extra="forbid"` intact); a record WITHOUT the new keys serializes byte-identically to a pre-phase record (the phase-50 contract). +2. `tests/unit/test_frontend_failed_turn.py` (new) — house-style source assertions (the phase-111 `test_frontend_banner_retry.py` pattern): + - the error catch `else` branch, the stream-drop guard, and the zero-frame fallback bubble all persist `failed: true` (grep the three sites for the `failed: true` persist) and each funnel path ends with `markLastRetryable`; + - `appendFailedNote` exists and guards against duplicates (one `.failed-note` per bubble); + - `FAILED_TURN_TEXT` is a distinct constant (not `EMPTY_ANSWER_FALLBACK`); + - `showErrorBanner` and `retryLastTurn` are byte-unchanged (pin their source — the phase's explicit "NOT touched" contract); + - the restore branch renders `m.failed` (note + Save-as-doc exclusion). +3. `tests/integration/test_chats_api.py` (extend): + - `POST /api/chats` with a brain record `{text, failed: true, error: "…"}` returns it byte-identically; `PUT` re-Save round-trips it; `error` >500 chars → 422; + - a shared chat (the phase-51 `share` path) carrying a failed record still serves `GET /api/shared/{token}` (public shape — `title` + `messages` — unchanged). +4. `tests/e2e/test_failed_turn_retry.py` (new — ONE file, run in isolation per AGENTS.md §4: `uv run pytest tests/e2e/test_failed_turn_retry.py -v --no-cov`), reusing `tests/e2e/mock_llm.py`'s failure pattern from `tests/e2e/test_llm_retry.py`: + - **A — network error:** mock the chat endpoint to fail with ZERO frames (connection reset / immediate close — the same failure `test_llm_retry.py` exercises past its retry budget, or a hard 500 if the mock supports it) → assert: the banner is visible with a working Retry button AND a failed bubble with the error detail exists → click banner Retry → the question is re-asked (mock now succeeds) → a grounded answer renders and the failed bubble is gone. + - **B — SSE error frame with partial:** mock streams some `delta` frames then an `error` frame → assert: the partial bubble KEEPS its streamed text + shows the error note + carries the in-bubble Retry → click it → re-asked in place (redo-in-place: the failed record is replaced by the new answer). + - **C — refresh:** fail a turn (as in A) → `page.reload()` → assert: the question + the failed bubble restore (error detail visible) + the Retry button is present on the failed bubble → click it → re-asked → answer renders. + - Negative: a STOPPED turn (user Stop) still restores with the "Answer stopped." note and NOT a failed note (phase 48 unchanged). +5. Run the full gate: `uv run pytest` (unit + integration), `uv run pytest --cov=app --cov-report=term-missing` (TOTAL >90%), the isolated E2E file, `uv run ruff check . && uv run pyright`. + +## Testing & Quality +- This task IS the phase's test suite (see Work). +- Coverage: **>90%** on `app/` — the only `app/` code in this phase is the `ChatMessage` schema (100% by the unit cases). + +## Completion Criteria +- [ ] All four test artifacts exist and pass; the isolated E2E file passes standalone. +- [ ] `uv run pytest --cov=app` TOTAL >90%; lint + types clean. +- [ ] No test asserts the old (broken) behavior — grep for any assertion that a network error shows NO Retry (must not exist). diff --git a/.agents/phases/todo/121_git_source_tokens/00_phase.md b/.agents/phases/todo/121_git_source_tokens/00_phase.md new file mode 100644 index 0000000..1ad1107 --- /dev/null +++ b/.agents/phases/todo/121_git_source_tokens/00_phase.md @@ -0,0 +1,48 @@ +# Phase 121 — Private git sources: a token that never reaches the UI or the API + +**Source:** `TODO.md` L5 — "Need a way to add private repos without exposing the token in the UI (like when adding an https repo `https://myuser:ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@github.com/myuser/my-private-repo.git`)" +**Story:** n/a (feature request; extends the phase-28/35/38 git/local sources and phase-89 per-source settings assets). +**Context:** `app/models.py:245` — `GitSource`: `url: Text UNIQUE NOT NULL`, `kind` ("git"/"local"), `path`, `ignore_paths` (JSONB), `include_hidden`, `added_at` — no token column. `app/schemas.py` — `GitSourceIn` (L561: `kind`, `url` min 1/max 500, `path`, `ignore_paths`, `include_hidden`; `_trim_url` before-validator L594), `GitSourceOut` (L605: `url: str`), `GitSourceRow` (L626: `url`), `GitSourcePatchIn` (L651). `app/api/git_sources.py` — `URL_RE = ^(?:https?://|ssh://|git@)` (L195; a prefix match, so `user:token@` URLs pass); POST validates the prefix (L344–345) and duplicates via `GitSource.url == url` (L349); GET list / GET single return `row.url` RAW (L235, L250, L297, L423) — an embedded token is echoed to any browser (the leak); the local-source upload endpoint (L432). `app/api/sync.py:296` — `clone_or_pull(row.url, sources_root / repo_name(row.url))`; `scripts/git_sync.py` — `clone_or_pull`, `repo_name`; `scripts/import_docs.py::_resolve_sources` — the CLI's second clone caller (both consume `app.rag.git_sources.effective_sources`, L27). `frontend/assets/git-sources.js` — add form `#git-source-url` (L241; submit `body: (url) => ({ url })` L945); every display site renders `s.url` raw (list cell L395–406 incl. the `title` attr, delete row L533, edit modal L697, ignore-list context L763/L830). `alembic/` — migrations. + +## Objective +Private repos are added with a bare URL plus an optional MASKED token field. The token lives in a dedicated DB column, is injected only into the clone URL at sync/clone time, and is absent from every API response and UI surface — including legacy rows that already embed the token in `url` (those are sanitized on output but keep working). + +## Dependencies +- `120_failed_turn_retry` (todo) — pipeline predecessor (execution order) only; no code dependency. +- Code dependencies (all complete): phase 35/38 `GitSource` kinds + `effective_sources`, phase 89 per-source settings (the PATCH-field precedent), phase 28 `clone_or_pull`/`repo_name`. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **Storage (task 01, LOCKED A2):** `GitSource.token` — `Text NULL` (NULL = public/no credential; **plaintext by necessity** — the repo must remain cloneable, so the raw credential must be recoverable; the Postgres DB is the trusted store and is never served to the UI; there is deliberately no external secrets backend). `GitSourceIn.token: str | None = None` (max 500, trimmed); `GitSourcePatchIn.token: str | None = None` — PATCH semantics: **absent/None = no change, non-empty = replace, empty string = clear** (the UI offers replace; clear exists for API completeness). `GitSourceOut`/`GitSourceRow` gain NO token field — no response shape ever carries it (LOCKED A2). +- **Normalization (task 02):** on POST (and PATCH when a new url/token arrives), the server normalizes: if the incoming URL contains userinfo (`user:pass@host`, only for `https?://` URLs — ssh/`git@` carry no userinfo), the **userinfo is stripped** for storage and the embedded credential is moved into `token` — UNLESS the caller also sent an explicit `token` field, which WINS (explicit beats embedded). Pasting the old-style `https://user:ghp_…@github.com/x/y.git` URL still works and ends up token-column-clean. The duplicate check (L349) runs on the NORMALIZED bare URL, so the same repo with a different token is still the same source (409, not a second row). +- **Clone-time credential (task 02):** `clone_url_for(row) -> str` in `app/rag/git_sources.py` (next to `effective_sources`): `row.url` unchanged when `token` is NULL; otherwise inject `https://x-access-token:@/` (https rows only — a token on a non-https row is a no-op with a warning log). `repo_name` keeps operating on the bare `row.url`. Callers switch from `row.url` to `clone_url_for(row)`: `app/api/sync.py:296` and `scripts/import_docs.py::_resolve_sources` (both already import from `app.rag.git_sources`). +- **Output sanitization (task 02, LOCKED A2):** every API surface that returns a git URL runs it through `sanitize_url(url)` (new, in `app/rag/git_sources.py`): strips the userinfo component (`https://…@host/…` → `https://host/…`), leaves ssh/`git@`/local paths untouched. Applied to `GitSourceOut.url` / `GitSourceRow.url` construction (GET list L235/L297, GET single, the `BOR_GIT_SOURCES` env fallback rows L250 — env rows can embed tokens too) and to any sync-status field echoing a repo URL (grep for `url=` in the sync responses). Belt-and-braces for legacy embedded-token rows whose credential is NOT in the `token` column: their DB value is untouched (the clone still authenticates from the stored URL) but no API/UI output ever shows the credential. +- **UI (task 03):** the add form gains a second field — a masked ``, optional, labelled "Token (private repos)" with a visible "optional" hint; submit sends `{ url, token }` (token omitted when blank). The edit modal mirrors it with placeholder "leave blank to keep the current token" (blank → omit from PATCH = no change). Every display site keeps rendering `s.url` — now bare by server sanitization, so list cells, `title` attributes, the delete row, and the ignore-list context become token-free with no per-site change. No new CSS beyond reusing the existing form-field styles (the theme's input treatment). +- **NOT touched:** local-kind sources (no URL credential), the `BOR_GIT_SOURCES` env parsing (its rows are sanitized on OUTPUT only), the upload endpoint, sync scheduling, and the Sources page layout. + +## Tasks +1. `01_token_storage.md` — migration + `GitSource.token` + input schemas (`GitSourceIn`/`GitSourcePatchIn`); no token in any output shape. +2. `02_clone_url_and_sanitization.md` — URL/token normalization on write, `clone_url_for` at clone time, `sanitize_url` on every output. +3. `03_ui_token_field.md` — masked token field in the add form + edit modal; display stays `s.url` (now bare). +4. `04_token_tests.md` — unit + integration + isolated E2E `test_git_source_tokens.py`. + +## Testing & Quality +- Unit: `tests/unit/test_git_source_token.py` (new, task 04) — `sanitize_url` (https userinfo stripped, ssh/git@/local untouched, no-userinfo unchanged), `clone_url_for` (NULL token → bare URL; token → injected; non-https token → bare + no crash), normalization (embedded token moved to the column when no explicit token; explicit token wins; duplicate on bare URL). +- Integration: `tests/integration/test_git_sources_api.py` (extend) — POST with `token` → GET list/single responses contain the token NOWHERE (assert on the raw JSON text) and show the bare URL; POST with an old-style embedded-token URL → stored bare + token column populated, responses clean; PATCH token replace/clear semantics; the sync flow builds the clone URL with the injected token (mock `clone_or_pull`). +- E2E: `tests/e2e/test_git_source_tokens.py` (new, task 04) — isolated run per AGENTS.md §4: add a private repo through the Sources UI (bare URL + token) → the list row shows the bare URL, the token is absent from the page text, the `title` attribute, and `GET /api/git-sources` JSON; edit the row (blank token) → no 4xx, token kept. +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] A private repo added via the UI (or a pasted embedded-token URL) syncs/clones fine, and its token appears in NO API response, NO page text, and NO attribute. +- [ ] Legacy embedded-token rows (pre-phase) still clone, and their API/UI output is token-free. +- [ ] Public repos and local sources behave byte-identically to before. +- [ ] `uv run pytest` green; `uv run pytest --cov=app --cov-report=term-missing` TOTAL >90%; `uv run ruff check . && uv run pyright` clean. +- [ ] One `--no-gpg-sign` commit; phase dir moved to `.agents/phases/complete/` by the pipeline gate. + +## Locked decisions +- **A2 — the token is stored PLAINTEXT in a dedicated `GitSource.token` column (cloneability requires the raw credential; no external secrets backend), is NEVER returned by any API shape, and legacy embedded-token URLs are sanitized on output while keeping their stored value for clones (owner-confirmed 2026-09-24, roadmap confirmation).** +- **A6 — pasting an old-style embedded-token URL is accepted and normalized (userinfo → `token` column); an explicit `token` field wins over an embedded one (owner-confirmed: same confirmation — the proposed design).** + +## Commit +```bash +git add app/ alembic/ frontend/ scripts/ tests/ .agents/phases/ && git commit --no-gpg-sign -m "feat(sources): add private git repos with a masked token that never reaches the UI or API" +``` diff --git a/.agents/phases/todo/121_git_source_tokens/01_token_storage.md b/.agents/phases/todo/121_git_source_tokens/01_token_storage.md new file mode 100644 index 0000000..4276460 --- /dev/null +++ b/.agents/phases/todo/121_git_source_tokens/01_token_storage.md @@ -0,0 +1,38 @@ +# Task 01 — Token storage: model, migration, input schemas + +**Phase:** `121_git_source_tokens` · **Source:** `TODO.md:5` — "Need a way to add private repos without exposing the token in the UI (like when adding an https repo `https://myuser:ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@github.com/myuser/my-private-repo.git`)" + +## Objective +The `git_sources` table can hold a per-row token, and the add/patch input shapes can carry one — while NO output shape (`GitSourceOut`, `GitSourceRow`, list, single, env-fallback rows) ever can. + +## Work +1. `app/models.py` — `GitSource` (L245): add + ```python + #: Private-repo credential (phase 121, LOCKED A2): the PAT the owner + #: types into the masked Sources-page field. NULL = public repo (or a + #: legacy row whose credential is still embedded in ``url``). Stored + #: plaintext BY NECESSITY — the repo must remain cloneable, so the + #: raw credential must be recoverable at sync time; the DB is the + #: trusted store and is never served to the UI. Injected into the + #: clone URL ONLY at clone time + #: (:func:`app.rag.git_sources.clone_url_for`); NEVER returned by + #: any API shape (the output models gain no token field). + token: Mapped[str | None] = mapped_column(Text, default=None) + ``` + (docstring first, then the column — the phase-89/105 field-docstring house style). +2. `alembic/versions/` — new revision (head of the current chain): `op.add_column("git_sources", sa.Column("token", sa.Text(), nullable=True))` + downgrade `op.drop_column`. Follow the existing migration file conventions (check the latest revision for the revision/down_revision pattern). +3. `app/schemas.py`: + - `GitSourceIn` (L561): add `token: str | None = Field(default=None, max_length=500)` with a `before`-mode trim validator (the `_trim_url` L594 precedent) — plus the docstring note: masked input from the Sources page; absent/None = no credential. + - `GitSourcePatchIn` (L651): add `token: str | None = Field(default=None, max_length=500)` — docstring the PATCH tri-state: **absent/None = no change, non-empty = replace, empty string = clear**. + - `GitSourceOut` (L605) / `GitSourceRow` (L626): add NO field; extend their docstrings with the explicit "no token — never a response field (phase 121)" note so the omission is a documented contract, not an accident. +4. No endpoint changes in this task (acceptance of `token` and normalization are task 02) — the extra field on the input models is inert until then (Pydantic would currently just pass it through unused; task 02 consumes it). + +## Testing & Quality +- Unit: `tests/unit/test_git_source_token.py` (task 04 extends): `GitSourceIn`/`GitSourcePatchIn` accept/trim `token`; `GitSourceOut`/`GitSourceRow` reject a `token` key (they are output models built from rows — assert constructing them with a token kwarg raises). +- Integration: `tests/integration/test_git_sources_api.py` (task 04) — after `alembic upgrade head`, `git_sources.token` exists (a `SELECT` sanity check in the existing test fixtures). +- Coverage: **>90%** on `app/` for the touched modules. + +## Completion Criteria +- [ ] `uv run alembic upgrade head` applies the new revision on a clean DB and the downgrade removes the column. +- [ ] The ORM round-trips a `token` value; output models have no token field (grep + unit assertion). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/121_git_source_tokens/02_clone_url_and_sanitization.md b/.agents/phases/todo/121_git_source_tokens/02_clone_url_and_sanitization.md new file mode 100644 index 0000000..74a3891 --- /dev/null +++ b/.agents/phases/todo/121_git_source_tokens/02_clone_url_and_sanitization.md @@ -0,0 +1,29 @@ +# Task 02 — Clone-time credential + output sanitization + +**Phase:** `121_git_source_tokens` · **Source:** `TODO.md:5` — "Need a way to add private repos without exposing the token in the UI …" + +## Objective +The token is injected into the clone URL only at clone time, old-style embedded-token URLs are normalized into the column on write, and every URL that leaves the API is token-free — including legacy rows and env-fallback rows. + +## Work +1. `app/rag/git_sources.py` — add two pure helpers (unit-testable, no DB): + - `sanitize_url(url: str) -> str` — strip the userinfo component of `https?://` URLs (`https://user:pass@host/path` → `https://host/path`); leave `ssh://`, `git@`, and local paths untouched; idempotent. Use a small regex (`^(https?://)([^/@]+)@` → `\1`) — never a URL parser that re-serializes (byte-identical output for clean URLs is a requirement: the phase-50/35 contract is that stored URLs surface verbatim when they carry no credential). + - `clone_url_for(row) -> str` — `row.url` when `row.token` is falsy; for an `https://` row with a token, inject `https://x-access-token:@/` (replace any existing userinfo with the column credential); for a non-https row with a token, log a warning and return `row.url` unchanged (a token cannot authenticate ssh — the owner must use a deploy key/agent there). + - Also `normalize_credential(url, token) -> (bare_url, effective_token)` — the write-path normalizer: if `url` (https only) contains userinfo, strip it → bare URL, and the embedded credential becomes `effective_token` UNLESS `token` is non-None (explicit wins, LOCKED A6). Returns the input untouched for clean URLs. +2. `app/api/git_sources.py`: + - POST git source (L342–355): run `normalize_credential(payload.url, payload.token)`; store the BARE url + `effective_token`; the duplicate check (L349) runs on the bare URL. + - PATCH (the url/token branch): when `payload.url` or `payload.token` is present, re-normalize the (current or new) pair with the same rules — PATCH token tri-state from task 01 (None = no change, `""` = clear → store NULL, non-empty = replace). + - Every output construction runs `sanitize_url` on the URL before it enters the response: the DB-row list path (L235/L297), the GET single path (L423), and the `BOR_GIT_SOURCES` env-fallback rows (L250 — an env URL can embed a token; the ENV VALUE itself is untouched, only the response is masked). Grep the router for any other `url=` response field (including sync-status echoes — `app/api/sync.py` responses that surface a repo URL get the same treatment) and sanitize those too. +3. `app/api/sync.py` (L296) + `scripts/import_docs.py::_resolve_sources` — swap `row.url` → `clone_url_for(row)` at the clone call site (`clone_or_pull(clone_url_for(row), sources_root / repo_name(row.url))` — `repo_name` stays on the bare URL so the checkout directory name is credential-free). +4. ASSUMPTION: `x-access-token` as the injected userinfo username (GitHub-agnostic — any git host that accepts `https://user:token@` treats the first component opaquely; `oauth2:` is also common, but `x-access-token` works on GitHub and GitLab and reads as non-identifying). + +## Testing & Quality +- Unit: `tests/unit/test_git_source_token.py` (task 04 finalizes) — `sanitize_url` (strip/no-op/idempotent/ssh/git@/local), `clone_url_for` (NULL token; token injected; non-https token no-op), `normalize_credential` (embedded→column, explicit wins, clean URL untouched). +- Integration: `tests/integration/test_git_sources_api.py` (task 04) — POST embedded-token URL → row.url bare + row.token populated; GET list JSON (raw text) contains the token NOWHERE; sync with a token row (mock `clone_or_pull`) receives the injected URL and a credential-free checkout path. +- Coverage: **>90%** on the touched modules. + +## Completion Criteria +- [ ] No token string in ANY API response for a token-bearing row (integration assertion on raw JSON text). +- [ ] A legacy row (token embedded in the stored `url`, `token` NULL) still produces the ORIGINAL stored URL at clone time (the credential keeps working) but its API output is masked. +- [ ] `repo_name` / checkout paths are credential-free. +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/121_git_source_tokens/03_ui_token_field.md b/.agents/phases/todo/121_git_source_tokens/03_ui_token_field.md new file mode 100644 index 0000000..30217cf --- /dev/null +++ b/.agents/phases/todo/121_git_source_tokens/03_ui_token_field.md @@ -0,0 +1,30 @@ +# Task 03 — UI: masked token field on add + edit + +**Phase:** `121_git_source_tokens` · **Source:** `TODO.md:5` — "Need a way to add private repos without exposing the token in the UI …" + +## Objective +The Sources-page git-source form takes a separate masked token field (add and edit); every display surface shows the bare URL (server-sanitized) and the token is nowhere in the DOM. + +## Work +1. `frontend/assets/git-sources.js`: + - Add form (the `#git-source-url` field at L241, submit wiring at L942–945): add a second labelled field + ```html + + + ``` + (reuse the existing form-field markup/CSS classes from the url field — the theme's input treatment, no new CSS needed beyond the existing `.field-hint` or an equivalent inline span). Submit body becomes `(url, token) => ({ url, ...(token ? { token } : {}) })` — blank token = key omitted (None = no credential). + - Edit modal (the url display/edit at L697): the same masked token field, placeholder "leave blank to keep the current token"; PATCH body includes `token` ONLY when non-blank (blank → omitted → no change — the task-01 tri-state). + - Display sites (L395–406 list cell incl. the `title` attribute, L533 delete row, L763/L830 ignore-list context): keep rendering `s.url` UNCHANGED — the server now returns bare URLs, so nothing to do per site. Add a source-comment note (one line) that URLs arrive sanitized server-side (phase 121) and the UI must never re-embed a credential. +2. `frontend/assets/styles.css` — only if the "optional" hint span has no existing class to reuse: a minimal `.field-hint` (muted color, contrast ≥4.5:1 per PLAN §7, small). +3. ASSUMPTION: the password field is `type="password"` with `autocomplete="off"` (a PAT is not a site credential; browsers must not offer to save it). + +## Testing & Quality +- Unit: `tests/unit/test_git_source_token.py` (task 04) — house-style source assertions: `#git-source-token` is `type="password"` and `autocomplete="off"`; the submit body omits a blank token; the edit PATCH omits a blank token; no display site concatenates a token. +- E2E: `tests/e2e/test_git_source_tokens.py` (task 04) — the UI scenarios. +- Coverage: n/a (frontend) — the validate.sh `app/` gate must stay green. + +## Completion Criteria +- [ ] Adding a private repo through the UI with a token succeeds; the list row shows the bare URL. +- [ ] The token is absent from the rendered page text, the `title` attribute, and the Sources-page DOM (E2E assertion). +- [ ] Editing with a blank token keeps the existing credential (integration: the PATCH tri-state). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/121_git_source_tokens/04_token_tests.md b/.agents/phases/todo/121_git_source_tokens/04_token_tests.md new file mode 100644 index 0000000..ef8374a --- /dev/null +++ b/.agents/phases/todo/121_git_source_tokens/04_token_tests.md @@ -0,0 +1,36 @@ +# Task 04 — Token tests: unit + integration + isolated E2E + +**Phase:** `121_git_source_tokens` · **Source:** `TODO.md:5` — "Need a way to add private repos without exposing the token in the UI …" + +## Objective +Pin the whole credential contract: helpers are pure and correct, no token ever crosses the API boundary (raw-JSON assertion), legacy rows stay cloneable and masked, and the UI never renders a credential. + +## Work +1. `tests/unit/test_git_source_token.py` (new): + - `sanitize_url` — https userinfo stripped (`https://myuser:ghp_x@github.com/x/y.git` → `https://github.com/x/y.git`), clean https unchanged byte-identically, `ssh://git@host/x.git` untouched, `git@github.com:x/y.git` untouched, a local path untouched, idempotent on already-clean URLs. + - `clone_url_for` — `token` NULL → `row.url` verbatim; https + token → `https://x-access-token:@host/path` (existing userinfo REPLACED); non-https + token → `row.url` (no crash, warning logged). + - `normalize_credential` — embedded userinfo → bare URL + token populated; explicit token wins over embedded; clean URL + None token → unchanged. + - Output models: `GitSourceOut`/`GitSourceRow` reject a `token` kwarg (no response field can ever carry it). + - Frontend source assertions (house style): `#git-source-token` is `type="password"` + `autocomplete="off"`; submit/PATCH omit a blank token. +2. `tests/integration/test_git_sources_api.py` (extend): + - POST `{url: "https://github.com/acme/private.git", token: "ghp_test123"}` → 201; `GET /api/git-sources` raw response TEXT does not contain `ghp_test123`; the row's `url` is the bare URL; `GET` single likewise. + - POST an old-style `https://myuser:ghp_legacy@github.com/acme/legacy.git` (no token field) → stored `url` bare, `token` = `ghp_legacy`; responses token-free. + - POST the same repo a second time (different token) → 409 (duplicate on the bare URL). + - PATCH token tri-state: absent → kept; non-empty → replaced (clone URL uses the new one); `""` → cleared (clone URL bare again). + - Legacy-row simulation (insert a row directly with the embedded URL, `token` NULL): `GET` output masked; the sync path (mock `clone_or_pull`) still receives the ORIGINAL stored URL (clone works). + - Sync flow: a token row → `clone_or_pull` called with the injected URL; the checkout path is credential-free. +3. `tests/e2e/test_git_source_tokens.py` (new — isolated run per AGENTS.md §4: `uv run pytest tests/e2e/test_git_source_tokens.py -v --no-cov`): + - Open the Sources page (admin), add a git source with a bare URL + a distinctive fake token (`ghp_e2esecret…`); + - assert: the list row renders the BARE URL; the token string is absent from `document.body.innerText`, from every `title` attribute, and from the `GET /api/git-sources` JSON (via a `page.request.get` inside the test); + - open the edit modal: the token field is blank (never pre-filled — a password must not be echoed back, so it is simply empty by design); save with it blank → 200, row intact; + - remove the source (cleanup) — the list is empty again. +4. Run the full gate: `uv run pytest`, `uv run pytest --cov=app --cov-report=term-missing` (TOTAL >90%), the isolated E2E file, `uv run ruff check . && uv run pyright`. + +## Testing & Quality +- This task IS the phase's test suite (see Work). +- Coverage: **>90%** on `app/` — the phase's `app/` code (helpers + router + schema + model) is fully exercised by the unit/integration cases. + +## Completion Criteria +- [ ] All test artifacts exist and pass; the isolated E2E file passes standalone. +- [ ] The raw-JSON "token nowhere" assertion covers list AND single AND sync-status surfaces. +- [ ] `uv run pytest --cov=app` TOTAL >90%; lint + types clean. diff --git a/.agents/phases/todo/122_image_documents/00_phase.md b/.agents/phases/todo/122_image_documents/00_phase.md new file mode 100644 index 0000000..24a2a4f --- /dev/null +++ b/.agents/phases/todo/122_image_documents/00_phase.md @@ -0,0 +1,54 @@ +# Phase 122 — Image documents: standalone images become first-class, retrievable documents + +**Source:** `TODO.md` L6 — "Need to support images. Images uploaded as part of documents or as standalone images should be read, summarized, and retrieved like any other document. Note that the embedding model won't support images, so the only embedded part of an image will be the summary generated by the model. The user should be able to turn on and off image support in their .env depending on whether their model supports it. Images retrieved by the RAG should be shown in the chat nicely and users should be able to submit images as part of their question in brain of reese." +**Story:** n/a (feature request; extends the phase-28/30/38 import pipeline, phase-90 no-scan uploads, and the RAG/chat assets). +**Context:** `app/config.py` — `Settings` (`BOR_` prefix; `upload_dir` L361, `sources_dir` L355, `upload_max_mb` L370 — raw-string/`expanduser` house convention). `app/rag/importer.py` — `iter_importable_files` (L235, extension filter via `llm.settings.import_extension_set` + `match_extension` L217), `import_sources` (L279, `prune` L283), `_index_file` (L435 — `read_text` L453, sha256 over text, Document upsert, the unchanged/hash path with the phase-118 summary backfill), `_store_summary` (L582 — lite-model summary + the position −1 `is_summary` chunk), `_prune` (L649 — deletes docs of the imported sources not in `seen`). `app/models.py` — `Document` (L102: `content`, `content_hash`, `summary` L140, `created_at`/`created_at_manual`), `Chunk` (L148: `is_summary` L161). `app/rag/summarizer.py` + `app/rag/llm.py` — the lite summary path + the chat-model client (`Settings` model names, `check_models`). `app/rag/archive_upload.py` — archive unpack into `upload_dir` (image members land on disk today, then get filtered out by the extension walk). `app/api/git_sources.py:432` — the upload endpoint. `app/api/docs.py` — the document content endpoint (document viewer). `app/rag/retriever.py` / `app/rag/agent.py` (read tool, message build L1446–1448) / `app/api/chat.py` — RAG + the SSE sources frames. `app/api/config.py:30` — `GET /api/config` public flags dict (task 01 of phase 123 extends it). Frontend: `frontend/assets/app.js` (chat source chips), `frontend/assets/document.js` + `frontend/document.html` (`#doc-content` L144), `frontend/assets/sources.js` (Sources page). `alembic/` — migrations. + +## Objective +With `BOR_IMAGES=true`, a standalone image file — arriving as a direct upload, inside an uploaded archive, or as a file in a git/local source — becomes a first-class document: the vision model (the chat model) describes it, the description is the document's content AND summary, only the description is embedded (the embedding model never sees pixels), the image bytes persist and are served, and the image shows up in the Sources page, the document viewer, and the chat — with retrieved image docs rendered inline in the answer's sources. + +## Dependencies +- `121_git_source_tokens` (todo) — pipeline predecessor (execution order) only; no code dependency. +- Code dependencies (all complete): phase 30 summary pipeline (`_store_summary`, `is_summary` chunk), phase 90 no-scan upload, phase 89/105 per-source walk options, the RAG agent + SSE sources frames. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **Toggle (task 01, LOCKED A3):** three new `Settings` fields — `images: bool = False` (`BOR_IMAGES`, `0`/`false` off — the phase-67 `llm_retries` bool style), `image_extensions: str = "png,jpg,jpeg,webp,gif,bmp"` (`BOR_IMAGE_EXTENSIONS`, comma-separated, lowercased into a frozenset by the importer — the `import_extension_set` property precedent), and `image_dir: str = "~/bor-sources/images"` (`BOR_IMAGE_DIR`, raw-string/`expanduser` convention — the persistent home for image bytes, deliberately separate from `sources_dir`/`upload_dir`). `.env.example` gets all three with a comment: **off by default — enable only when your chat model supports vision, because image descriptions are generated by the chat model.** `GET /api/config` (task 01) gains `images: bool` so the UI can gate affordances (consumed by phase 123; the Sources page can show an "images off" hint — optional, not required). +- **Why the bytes are copied (task 02):** upload dirs are REPLACED on every upload (`archive_upload.swap_in`), git checkouts are re-cloned, and local dirs are user-edited — a served image must outlive its source file. The importer copies each ingested image to `image_dir/.` (created on demand) and stores that path in `Document.image_path`. The copy happens ONLY when the doc is new or its hash changes; a replaced image deletes the stale copy; pruned docs delete their copy. +- **Storage (task 02):** `Document.is_image: bool` (server default `false` — every pre-phase-122 row is a text doc) + `Document.image_path: str | None` (NULL for text docs). One migration, one downgrade. +- **Ingest (task 02):** the walk: `iter_importable_files`/`import_sources` accept the image frozenset IN ADDITION to `import_extension_set`, ONLY when `settings.images` is true (images are never user-configurable via `BOR_IMPORT_EXTENSIONS` — the toggle is the single knob, LOCKED A3/A4). `_index_file` branches on image extension: read BYTES (not `read_text`), sha256 over the bytes (the digest rule is unchanged — content identity), copy to `image_dir`, set `is_image` + `image_path`, and `content` = the vision description (task 03). The normal chunk pipeline then embeds the content (= the description) — that is exactly the TODO's "the only embedded part of an image will be the summary generated by the model"; the phase-30 summary chunk (`is_summary`, position −1) mirrors `Document.summary`, which equals the description too. Title = the file stem (the non-markdown rule at L505–509). The unchanged/hash path works unmodified (byte digest → "unchanged" skips re-describing; the phase-118 backfill path re-describes a NULL-summary image doc on its next sync — same fail-soft). **Prune guard:** `_prune` (L649) must NOT delete `is_image` docs while `settings.images` is false (an image doc is invisible to an images-off walk, not a deleted file — otherwise turning the toggle off and syncing would silently destroy the image documents). Toggle ON → normal prune semantics (a deleted image file prunes its doc + copy). +- **Description (task 03, LOCKED A3):** `describe_image` in `app/rag/summarizer.py` (one function, the summarizer module owns model-text generation): a SINGLE chat-model call (`Settings.llm_chat_model` — the vision model; the lite summary model is NOT assumed vision-capable, LOCKED A3) with a multimodal user message `[{type: "text", text: }, {type: "image_url", image_url: {url: }}]`; the prompt asks for a faithful, retrieval-oriented description (what is shown, any text/labels/diagram content, salient details — the description is the ONLY thing retrievable, so it must carry the image's meaning). Output capped at `settings.summary_max_chars` (the description IS the doc's summary; the phase-30 cap keeps it uniform). Stored: `Document.summary = Document.content = description`. **Fail-soft:** a failed/empty description → the doc is SKIPPED (no row, `ImportSummary` counts it in a new `images_failed` counter + a `logger.warning` with source/path) — an undescribed image is unsearchable noise; the sync continues (the importer's existing fail-soft convention). +- **Serve + display (task 04):** `GET /api/documents/{doc_id}/image` (new route in `app/api/docs.py`) — 404 for missing docs and non-image docs; serves `image_path` bytes with the correct `Content-Type` (ext → mime map: png/jpeg/webp/gif/bmp) — PUBLIC like the document content itself (this app's document content is already anonymous-readable; the image is part of that content). The document content endpoint (the one `frontend/assets/document.js` boots against) gains `is_image: bool` + `image_url` (the new route's path, absent for text docs) so `document.html` renders `` (max-width 100%, the theme's image treatment) with the summary/description text below it instead of the markdown content; the Sources page row for an image doc shows a small thumbnail (lazy-loaded, `loading="lazy"`, aspect-ratio box) or the existing doc icon when the fetch is not yet possible offline — the thumbnail is a progressive enhancement (a fetch failure falls back to the icon). +- **RAG display (task 05):** the SSE sources frames (and any sources-list shape the chat bubble renders from) carry an OPTIONAL `image_url` on image docs (the retriever/agent know the `Document` row — add the field where `SourceRef`-shaped frames are built in `app/api/chat.py`/`app/rag/retriever.py`); the chat's sources block renders a compact inline `` (capped height, the summary as caption/alt) for image docs — "shown in the chat nicely" (TODO L6). The agent's `read` tool on an image doc returns its description prefixed with a one-line marker (e.g. `Image document — description generated from the image:`) so the model knows what it is reading. `alt` text = the summary everywhere (WCAG). +- **NOT touched (this phase):** chat-side image submission (phase 123), the lite summary path for TEXT docs, archive unpacking (image members already land on disk — only the walk filter changes), and git/local sync scheduling. +- **Locked assumptions:** **A3** — descriptions use the CHAT model (`BOR_LLM_CHAT_MODEL`, must be vision-capable); `BOR_IMAGES` defaults to **false**; generation failure → doc skipped + logged. **A4** — "images uploaded as part of documents" = standalone image files arriving via direct upload / uploaded archives / source walks — NOT embedded-image extraction from PDFs/DOCX. + +## Tasks +1. `01_image_toggle.md` — `BOR_IMAGES` / `BOR_IMAGE_EXTENSIONS` / `BOR_IMAGE_DIR` settings + `.env.example` + `GET /api/config` flag; off = byte-identical behavior. +2. `02_image_ingest.md` — `Document.is_image`/`image_path` + migration; walk accepts image extensions when on; `_index_file` binary branch + persistent copy; prune guard when off. +3. `03_image_description.md` — `describe_image` (chat-model vision), content = summary = description, fail-soft skip + counter. +4. `04_serve_and_display.md` — `GET /api/documents/{id}/image`; document viewer + Sources page rendering. +5. `05_rag_display.md` — `image_url` on chat source frames + inline image in the chat sources block + the agent `read` marker. +6. `06_image_tests.md` — unit + integration + isolated E2E `test_image_documents.py`. + +## Testing & Quality +- Unit: `tests/unit/test_image_documents.py` (new, task 06) — settings parsing (toggle off by default, extensions frozenset, mime map), the `_index_file` image branch (bytes digest, copy to `image_dir`, `is_image`/`image_path` set, text `content` never read for an image), the prune guard (toggle off → image docs survive; toggle on → deleted image prunes), `describe_image` prompt shape (multimodal content list, chat model, cap) with a mock client, and the fail-soft skip path. +- Integration: `tests/integration/test_docs_api.py` (extend) — the image route (200 + correct Content-Type for a seeded image doc; 404 for text docs and missing ids); the content endpoint exposes `is_image`/`image_url` for image docs and omits them for text docs (byte-identical text-doc responses); `import_sources` end-to-end with `images=True` and a mock vision client (a fixture PNG → doc row with description content + `is_summary` chunk embedding; `images=False` → the file is ignored, pre-existing image doc survives prune). +- E2E: `tests/e2e/test_image_documents.py` (new, task 06) — isolated run per AGENTS.md §4, `BOR_IMAGES=true` for this suite's app instance: upload a small fixture PNG (via the existing upload endpoint's UI or `page.request`) → sync → the Sources page lists it (thumbnail or icon) → open the document viewer → the image renders with its description → ask a question the mock LLM grounds on the image doc → the chat's sources block shows the inline image. +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] With `BOR_IMAGES=true`: an uploaded standalone image (direct or in an archive) and an image file in a git/local source become documents whose content/summary is the vision description and whose ONLY embedded text is that description. +- [ ] With `BOR_IMAGES=false` (the default): every request, walk, and response is byte-identical to pre-phase; existing image docs (if any) survive a sync. +- [ ] The image renders in the document viewer and in the chat's sources block (inline, with alt text); a failed description skips the doc and logs — the sync completes. +- [ ] `uv run pytest` green; `uv run pytest --cov=app --cov-report=term-missing` TOTAL >90%; `uv run ruff check . && uv run pyright` clean. +- [ ] One `--no-gpg-sign` commit; phase dir moved to `.agents/phases/complete/` by the pipeline gate. + +## Locked decisions +- **A3 — image descriptions are generated by the CHAT model (`BOR_LLM_CHAT_MODEL`, vision-capable); `BOR_IMAGES` defaults to false; a failed/empty description skips the doc and logs (owner-confirmed 2026-09-24, roadmap confirmation).** +- **A4 — "images uploaded as part of documents" = standalone image files via direct upload / uploaded archives / source walks — no embedded-image extraction from PDFs/DOCX (owner-confirmed 2026-09-24).** +- **Prune guard (derived from A3/A4, same confirmation):** images-off syncs never prune `is_image` docs — turning the toggle off must not destroy image documents. + +## Commit +```bash +git add app/ alembic/ frontend/ tests/ .env.example .agents/phases/ && git commit --no-gpg-sign -m "feat(rag): index standalone images as documents — described, embedded, and displayed via the vision model" +``` diff --git a/.agents/phases/todo/122_image_documents/01_image_toggle.md b/.agents/phases/todo/122_image_documents/01_image_toggle.md new file mode 100644 index 0000000..d1b8e2a --- /dev/null +++ b/.agents/phases/todo/122_image_documents/01_image_toggle.md @@ -0,0 +1,25 @@ +# Task 01 — Image toggle: BOR_IMAGES + extensions + dir, off by default + +**Phase:** `122_image_documents` · **Source:** `TODO.md:6` — "The user should be able to turn on and off image support in their .env depending on whether their model supports it." + +## Objective +The single env knob for image support exists and is surfaced — `BOR_IMAGES` (default **false**), `BOR_IMAGE_EXTENSIONS`, `BOR_IMAGE_DIR` — with `GET /api/config` exposing the flag for UI gating. Toggle off = byte-identical behavior to pre-phase. + +## Work +1. `app/config.py` — three new `Settings` fields (house docstring style, the `upload_dir`/`llm_retries` precedents): + - `images: bool = False` — `BOR_IMAGES`, `0`/`false` off (LOCKED A3 default). Docstring: master switch for image-document indexing (phase 122) — off by default, enable only when the chat model supports vision (descriptions come from it). + - `image_extensions: str = "png,jpg,jpeg,webp,gif,bmp"` — `BOR_IMAGE_EXTENSIONS`, comma-separated, case-insensitive; a property/parse into a lowercased-dotted frozenset (the `import_extension_set` precedent) — the image set is SEPARATE from `import_extension_set` (images are never user-added via `BOR_IMPORT_EXTENSIONS`). + - `image_dir: str = "~/bor-sources/images"` — `BOR_IMAGE_DIR`, raw string, `Path.expanduser()` applied by the importer (the `sources_dir`/`upload_dir` convention) — the persistent home for image bytes (uploads are replaced, checkouts re-cloned — the copy must outlive the source file). +2. `.env.example` — the three entries with the comment block: off by default + the vision-model dependency note (LOCKED A3). +3. `app/api/config.py:30` — the `app_config` dict gains `"images": settings.images` (the dict is `str | bool`-valued — bools already allowed). Extend the docstring: consumed by the chat composer (phase 123) to show/hide the attach control, optionally by the Sources page. +4. ASSUMPTION: `GET /api/config` is already anonymous-readable (the UI gates on it pre-login in phase 123 — no auth change here). + +## Testing & Quality +- Unit: `tests/unit/test_image_documents.py` (task 06 finalizes) — defaults (`images` False, extensions frozenset `{".png", …}` with the dotted form the matchers expect, dir default), env overrides, the frozenset parse is case-insensitive and trims spaces. +- Integration: the existing `GET /api/config` test asserts the new `images` key (default false in the test env). +- Coverage: **>90%** on the touched modules. + +## Completion Criteria +- [ ] `Settings()` with no env: `images is False`, the extension set is the six defaults, `image_dir` is the default path. +- [ ] `GET /api/config` returns `images: false` in the default test env (byte-check the other keys unchanged). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/122_image_documents/02_image_ingest.md b/.agents/phases/todo/122_image_documents/02_image_ingest.md new file mode 100644 index 0000000..dd1404c --- /dev/null +++ b/.agents/phases/todo/122_image_documents/02_image_ingest.md @@ -0,0 +1,30 @@ +# Task 02 — Image ingest: model fields, walk filter, binary index path, prune guard + +**Phase:** `122_image_documents` · **Source:** `TODO.md:6` — "Images uploaded as part of documents or as standalone images should be read, summarized, and retrieved like any other document." + +## Objective +When `BOR_IMAGES=true`, standalone image files in ANY ingest path (direct upload, uploaded archive, git/local source walk) become `Document` rows — bytes persisted to `image_dir`, `is_image`/`image_path` set, `content` = the vision description (task 03) — and an images-off sync never prunes existing image docs. + +## Work +1. `app/models.py` — `Document` (L102): add, with house docstrings (the `summary` L140 / `created_at_manual` precedent): + - `is_image: Mapped[bool] = mapped_column(Boolean, default=False, server_default=text("false"), nullable=False)` — True iff the doc's content is a vision description of an image (phase 122); the image bytes live at `image_path`. + - `image_path: Mapped[str | None] = mapped_column(Text, default=None)` — absolute path of the persistent copy in `settings.image_dir`; NULL for text docs. +2. `alembic/versions/` — new revision: both columns (`is_image` NOT NULL server_default 'false'; `image_path` nullable) + downgrade. +3. `app/rag/importer.py`: + - `iter_importable_files` (L235) / the walk in `import_sources` (L279): when `llm.settings.images`, accept a file iff its extension matches `import_extension_set` OR the image frozenset (task 01) — pass the image set in (the function takes explicit extension sets; the image set is NOT merged into `import_extension_set`). + - `_index_file` (L435): image branch FIRST (before the `read_text` at L453) — if the path's extension is in the image set: `data = full_path.read_bytes()`, `digest = sha256(data)`, and on new/changed: copy `data` to `image_dir/.` (dir created with `mkdir(parents=True, exist_ok=True)`), set `is_image=True` + `image_path` on the `Document` row, `content` = the description (task 03's `describe_image` — this task wires the call; the function lands in task 03, so for THIS task store `content = ""` placeholder ONLY if task 03 is not yet merged — the phases run task-ordered, so in practice task 03's function exists; wire it directly and let task 03 implement it. If implementing strictly per task: this task stores `content` via a `_describe_or_skip` hook that task 03 fills — keep the seam single and commented). + - A CHANGED image (hash differs) deletes the stale `image_path` copy before replacing it. + - The unchanged/hash path (L470+) works unmodified for images (byte digest); the phase-118 summary-backfill branch (L480) re-describes an image doc whose `summary` is NULL on the next sync (same fail-soft). + - `_prune` (L649): the prune guard (LOCKED derived decision) — when `settings.images` is FALSE, skip every `is_image` doc (invisible to the walk ≠ deleted); toggle TRUE → normal prune + delete the `image_path` copy of each pruned image doc (also on the normal prune path when the file is gone). + - `ImportSummary` (L100): new `images_failed: int = 0` counter + its slot in `format_counts`/`log` (L135–151) — task 03 increments it; add it now so the log shape is stable. +4. ASSUMPTION (A4 re-stated): only standalone image FILES are ingested — no archive-of-documents extraction, no PDF/DOCX embedded-image pulls (the archive unpacker already places image members on disk; the walk now just accepts them). + +## Testing & Quality +- Unit: `tests/unit/test_image_documents.py` (task 06) — the walk accepts `.png` only when `images=True` (off → ignored, the byte-identical default), the binary branch (digest over bytes, copy made, fields set, `read_text` never called for an image), the changed-image stale-copy delete, the prune guard (off → image doc survives; on + file gone → pruned + copy deleted), `images_failed` in the log line. +- Integration: `tests/integration/test_docs_api.py` (task 06) — the `import_sources` end-to-end cases. +- Coverage: **>90%** on the touched modules. + +## Completion Criteria +- [ ] `alembic upgrade head` applies; a seeded image walk with `images=True` creates the doc row + `image_dir` copy; `images=False` ignores the file entirely. +- [ ] A sync with `images=False` leaves a pre-existing image doc untouched (prune guard). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/122_image_documents/03_image_description.md b/.agents/phases/todo/122_image_documents/03_image_description.md new file mode 100644 index 0000000..f21623d --- /dev/null +++ b/.agents/phases/todo/122_image_documents/03_image_description.md @@ -0,0 +1,28 @@ +# Task 03 — Image description: the vision model writes the only embedded text + +**Phase:** `122_image_documents` · **Source:** `TODO.md:6` — "the only embedded part of an image will be the summary generated by the model." + +## Objective +`describe_image` generates the image's description with the CHAT model (vision), the description becomes BOTH `Document.content` and `Document.summary` (so the chunk pipeline embeds exactly that text — and only that text), and a failed description fails soft (skip + count + log, sync continues). + +## Work +1. `app/rag/summarizer.py` — new `async def describe_image(llm, data: bytes, mime: str, settings=None) -> str | None` (the summarizer module owns model-text generation; follow the existing summary-call conventions — client, model, timeout, the `summary_max_chars` cap): + - ONE chat-model call (`settings.llm_chat_model` — LOCKED A3; the lite summary model is not assumed vision-capable) with messages `[{role: "user", content: [{type: "text", text: }, {type: "image_url", image_url: {url: f"data:{mime};base64,{b64}"}}]}]` — the multimodal content-list shape the OpenAI-compatible API expects. + - `DESCRIBE_PROMPT` (a module constant, pinned by a unit test): a faithful, retrieval-oriented description — what is depicted, any visible text/labels/titles, diagram/table structure, salient details; 2–4 sentences of substance (the description is the ONLY retrievable text of the doc, so it must carry the image's meaning). + - Return the stripped text capped at `settings.summary_max_chars` (the phase-30 cap — the description IS the summary); return `None` on any client error, empty response, or non-2xx (the caller fails soft). No retries beyond the SDK's own — a description failure must not stall a sync. +2. `app/rag/importer.py` — wire the task-02 seam: the image branch's `content`/`summary` come from `describe_image` — + - description `None` → **skip the doc entirely** (no row, no `image_dir` copy kept — delete the copy if it was made, or make the copy AFTER a successful description so a failure never leaves an orphan), `summary.images_failed += 1`, `logger.warning("import: image description failed source=%s path=%s", source, rel)` — the fail-soft skip (LOCKED A3). + - success → `content = description`, then the existing `_store_summary` path (L582) runs with the description as the summary (the `is_summary` position −1 chunk mirrors it — phase-30 behavior, unchanged), and the normal content chunks embed the description (for a short description that is typically ONE content chunk + the summary chunk — the chunker's existing behavior, no special case). + - the phase-118 backfill branch (unchanged image doc, `summary is None`) calls the SAME path — a description failure there keeps the doc as-is and logs (no row mutation). +3. `app/rag/llm.py` — no new client: `describe_image` reuses the existing `llm.chat`-equivalent client the summarizer already uses for text summaries (verify the exact client method name in `app/rag/summarizer.py` and match it — the multimodal payload is a plain `list[dict]` message, so no client change is needed; IF the existing client hard-codes text-only `content: str` typing, extend its signature to accept `content: str | list` — pyright-clean). +4. ASSUMPTION (A3 re-stated): the CHAT model describes; if the owner's chat model lacks vision, `describe_image` returns `None` (the SDK errors) and every image doc is skipped + logged — honest, visible failure (the `images_failed` counter in the sync log is the signal). + +## Testing & Quality +- Unit: `tests/unit/test_image_documents.py` (task 06) — with a MOCK client: the prompt shape (text part + `image_url` data-URL part, correct model), the cap is applied, whitespace stripped; `None` on mock error / empty string / client exception; the importer's skip path (no row, `images_failed == 1`, warning logged, no orphan copy) and the success path (content == summary == description, `is_summary` chunk present, embedding called with the description text — the ONLY text embedded). +- Integration: the mock-vision `import_sources` end-to-end (task 06). +- Coverage: **>90%** on the touched modules. + +## Completion Criteria +- [ ] A fixture PNG through the mock vision client yields a doc whose `content` == `summary` == the description, with its embedding(s) derived from that text only. +- [ ] A failing mock client skips the doc, bumps `images_failed`, logs, and the sync completes with the other docs indexed. +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/122_image_documents/04_serve_and_display.md b/.agents/phases/todo/122_image_documents/04_serve_and_display.md new file mode 100644 index 0000000..fa19ce6 --- /dev/null +++ b/.agents/phases/todo/122_image_documents/04_serve_and_display.md @@ -0,0 +1,31 @@ +# Task 04 — Serve the image + render it in the document viewer and Sources page + +**Phase:** `122_image_documents` · **Source:** `TODO.md:6` — "Images … should be read, summarized, and retrieved like any other document." + +## Objective +Image bytes are served through a dedicated document route, the document viewer renders the image with its description, and the Sources page shows an image affordance — image docs read like first-class documents in every existing surface. + +## Work +1. `app/api/docs.py` — new route `GET /api/documents/{doc_id}/image`: + - 404 (the router's existing "unknown document" shape) for a missing doc and for a doc with `is_image` false / `image_path` NULL; + - 404 if the file is missing on disk (defensive — the row exists but the copy was lost); + - otherwise `FileResponse` (or a `Response` with the bytes) with `Content-Type` from an ext→mime map (`png`→`image/png`, `jpg`/`jpeg`→`image/jpeg`, `webp`→`image/webp`, `gif`→`image/gif`, `bmp`→`image/bmp` — the map lives in `app/rag/importer.py` or a small shared spot the unit tests can import; default `application/octet-stream` for an unexpected ext) and `Cache-Control: private, max-age=3600` (the image bytes are content-hashed — long enough, bustable by re-upload). + - PUBLIC, like the document content endpoint (this app serves document content to anonymous visitors — the image is part of that content). + - The document CONTENT endpoint the viewer boots against (same module): response gains `is_image: bool` (always present) + `image_url` (the `/api/documents/{id}/image` path — ABSENT for text docs, the `_drop_absent_share_url` omission precedent; never `null`). Text-doc responses gain only `is_image: false` — one new key, documented in the response schema's docstring. +2. `frontend/assets/document.js` + `frontend/document.html`: + - boot reads `is_image`; when true, `#doc-content` renders `{summary}` (block, `max-width: 100%`, the theme's surface treatment) with the description/summary text in the normal content slot below it (the document's readable content IS the description — no markdown render of a non-markdown string is needed; render it as the existing plain-content path). + - an `` error fallback: on `onerror` the image area shows a small "image unavailable" note (the 404-on-missing-file case) — the page still shows the description. +3. `frontend/assets/sources.js` — the Sources page row for an image doc: a small thumbnail (48px box, `object-fit: cover`, `loading="lazy"`, `alt = summary`) where the doc icon sits; the thumbnail is a PROGRESSIVE enhancement — a failed fetch (or the row rendered before the fetch resolves) falls back to the existing icon (no layout shift beyond the fixed box). The doc title/path columns are unchanged. +4. `frontend/assets/styles.css` — the viewer image block + the Sources thumbnail box (theme tokens; WCAG: alt text everywhere, no contrast concerns for decorative images). +5. ASSUMPTION: the thumbnail uses the SAME full-size route (no separate thumb route) — a KB-scale image set makes a thumb pipeline unjustified; lazy loading keeps the Sources page fast. + +## Testing & Quality +- Integration: `tests/integration/test_docs_api.py` (task 06) — the image route (200 + exact `Content-Type` per ext for a seeded doc; 404 for a text doc; 404 for a missing id; 404 for a row whose file is deleted); the content endpoint: `is_image` present in ALL responses, `image_url` absent for text docs and present for image docs. +- Unit: `tests/unit/test_image_documents.py` (task 06) — the ext→mime map (all six + the octet-stream default); house-style source assertions: the viewer renders the `img` from `image_url` with `alt = summary`, the sources row falls back to the icon on image error, no `null` in the text-doc content response. +- E2E: `test_image_documents.py` scenarios (task 06) cover viewer + Sources rendering. +- Coverage: **>90%** on the touched modules. + +## Completion Criteria +- [ ] `GET /api/documents/{id}/image` serves the exact uploaded bytes with the right Content-Type; text docs 404. +- [ ] The document viewer shows the image + its description; the Sources page shows the thumbnail (or the icon fallback). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/122_image_documents/05_rag_display.md b/.agents/phases/todo/122_image_documents/05_rag_display.md new file mode 100644 index 0000000..cc9d314 --- /dev/null +++ b/.agents/phases/todo/122_image_documents/05_rag_display.md @@ -0,0 +1,24 @@ +# Task 05 — RAG display: image docs in the chat sources + the agent read marker + +**Phase:** `122_image_documents` · **Source:** `TODO.md:6` — "Images retrieved by the RAG should be shown in the chat nicely." + +## Objective +When a retrieved/agent-read document is an image, the chat shows it: the sources block renders a compact inline image with its summary as caption/alt, and the agent's `read` tool tells the model it is reading a generated image description. + +## Work +1. `app/rag/retriever.py` / `app/api/chat.py` — the sources frames the chat bubble renders (the SSE `sources`/related-doc frames and the agent-sourced doc list): add an OPTIONAL `image_url` field to the per-doc ref shape — populated (the `/api/documents/{id}/image` path) iff the doc row has `is_image`, absent otherwise (the omission rule — text-doc frames stay byte-identical). The retriever already has the `Document` row; the agent's doc refs (the read-tool results / source list) do too — set it at the frame-build sites (grep for the source-ref construction in both modules; one shared helper `source_ref_with_image(doc, …)` keeps the two sites in lockstep). +2. `frontend/assets/app.js` — the chat's sources block renderer: when a source ref carries `image_url`, render a compact inline `` (max-height ~96px, `object-fit: contain`, the theme's surface, `alt` + visible caption = the doc summary — the "shown nicely" requirement) in place of / beside the existing doc chip text (keep the title + the existing chip affordance — the image is additive, not a replacement). A failed image load collapses to the plain chip (never a broken-image icon). +3. `app/rag/agent.py` — the `read` tool's result for an image doc: prefix the description with the marker line `Image document — the text below is a description generated from the image:` (a module constant) so the model reasons about what it is reading; non-image docs' results are byte-identical. +4. ASSUMPTION: the chat QUESTION side (users submitting images) is phase 123 — this task only covers RETRIEVED images in the answer's sources. +5. ASSUMPTION: the sources-frame `image_url` is the only new frame field — no doc-id leak beyond what the frame already carries (the path encodes the doc id, same as the content endpoint). + +## Testing & Quality +- Integration: `tests/integration/test_chat_api.py` (extend, task 06) — a mocked grounded answer that includes an image doc in its sources → the SSE frame carries `image_url` for that ref only; a text-only grounding has NO `image_url` key anywhere (byte check). +- Unit: `tests/unit/test_image_documents.py` (task 06) — the frame-helper (present/absent), the agent marker (image vs non-image result), house-style source assertions: the sources renderer reads `image_url`, sets `alt`, and falls back on image error. +- E2E: `test_image_documents.py` scenario (task 06) — ask a question the mock LLM grounds on the fixture image doc → the chat sources block shows the inline image with its caption. +- Coverage: **>90%** on the touched modules. + +## Completion Criteria +- [ ] A chat answer grounded on an image doc shows the inline image + caption in its sources block; text-doc answers render byte-identically to before. +- [ ] The agent `read` result for an image doc carries the marker; the model sees the description, not raw bytes. +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/122_image_documents/06_image_tests.md b/.agents/phases/todo/122_image_documents/06_image_tests.md new file mode 100644 index 0000000..36fd7d6 --- /dev/null +++ b/.agents/phases/todo/122_image_documents/06_image_tests.md @@ -0,0 +1,33 @@ +# Task 06 — Image tests: unit + integration + isolated E2E + +**Phase:** `122_image_documents` · **Source:** `TODO.md:6` — "Need to support images …" + +## Objective +Pin the whole image-document contract: the off-by-default byte-identity, the ingest/description/serve pipeline, the RAG display, and the end-to-end user path (upload → Sources → viewer → chat) as an isolated Playwright suite. + +## Work +1. `tests/unit/test_image_documents.py` (new) — consolidates the per-task unit cases (the tasks ship code; this task ships the full pin): + - settings: defaults (`images` False, six-extension frozenset, dir default), env overrides, case-insensitive parse (task 01); + - the walk: image accepted iff `images=True`; off → the file is ignored (the default byte-identity); + - `_index_file` image branch: digest over BYTES, copy to `image_dir`, `is_image`/`image_path` set, changed-image stale-copy delete, prune guard (off → survives; on + gone → pruned + copy deleted), `images_failed` in the log line (task 02); + - `describe_image`: mock-client prompt shape (multimodal parts, chat model), cap, `None` on error/empty, the importer skip path (no row, no orphan, counter, warning) and the success path (content == summary == description; embedding called with the description only) (task 03); + - the ext→mime map (task 04); + - the source-frame `image_url` helper (present/absent) + the agent `read` marker + house-style frontend assertions (viewer `img` + alt + fallback; sources thumbnail fallback; chat sources inline image + alt) (task 05). +2. `tests/integration/test_docs_api.py` (extend, task 04's cases) — the image route (200 + Content-Type per ext; 404 text doc / missing id / missing file), the content endpoint's `is_image`/`image_url` omission rules; `tests/integration/test_chat_api.py` (extend, task 05's case) — the SSE `image_url` frame; `tests/integration/` (new file `test_image_import.py` or the existing import test file — follow whichever exists) — `import_sources` end-to-end: `images=True` + mock vision → the fixture PNG becomes a doc (description content, `is_summary` chunk, one content chunk); `images=False` → ignored + a pre-seeded image doc survives prune; a failing mock → `images_failed == 1`, no row, other docs indexed. + - Fixtures: a tiny valid PNG (a few bytes, generated in-test or a committed fixture under `tests/` — check the existing fixture conventions), a mock vision client (the existing mock-LLM test patterns in `tests/`). +3. `tests/e2e/test_image_documents.py` (new — isolated run per AGENTS.md §4: `uv run pytest tests/e2e/test_image_documents.py -v --no-cov`). The suite's app instance runs with `BOR_IMAGES=true` (env override in the E2E fixture — the `conftest.py` pattern for per-suite app env): + - upload a fixture PNG (the Sources-page upload flow or `page.request` against the upload endpoint, then trigger the sync through the UI as the Sources page does); + - the Sources page lists the image doc (thumbnail or icon fallback); + - open the document viewer → the image renders + the description text below it; + - ask a question the mock LLM grounds on the image doc (the existing mock-LLM grounding pattern) → the chat's sources block shows the inline image with its caption; + - negative: with the DEFAULT env (`BOR_IMAGES` unset/false), the same upload produces NO image doc (the default-off contract). +4. Run the full gate: `uv run pytest`, `uv run pytest --cov=app --cov-report=term-missing` (TOTAL >90%), the isolated E2E file, `uv run ruff check . && uv run pyright`. + +## Testing & Quality +- This task IS the phase's test suite (see Work). +- Coverage: **>90%** on `app/` — the phase's `app/` surface (config, importer, summarizer, docs API, chat frames, agent marker) is fully exercised. + +## Completion Criteria +- [ ] All test artifacts exist and pass; the isolated E2E file passes standalone. +- [ ] The default-off byte-identity is asserted (unit + integration + the E2E negative case). +- [ ] `uv run pytest --cov=app` TOTAL >90%; lint + types clean. diff --git a/.agents/phases/todo/123_chat_image_questions/00_phase.md b/.agents/phases/todo/123_chat_image_questions/00_phase.md new file mode 100644 index 0000000..fcdec4b --- /dev/null +++ b/.agents/phases/todo/123_chat_image_questions/00_phase.md @@ -0,0 +1,50 @@ +# Phase 123 — Chat image questions: attach an image to a question + +**Source:** `TODO.md` L6 — "…users should be able to submit images as part of their question in brain of reese." +**Story:** n/a (feature request; completes the phase-122 image capability on the question side). +**Context:** Phase 122 (todo, this pipeline) — `BOR_IMAGES` toggle + `GET /api/config` `images` flag (task 01), the ext→mime map, `image_dir` storage convention. `app/schemas.py:60` — `ChatRequest` (`message` min 1/max 4000, `history` ≤100 — `HistoryTurn` is text-only), `ChatMessage` (L742, `extra="forbid"`, phase-83 value bounds). `app/api/chat.py` — the turn pipeline: the user message is built at L645 (`{"role": "user", "content": request.message}`; a grounded turn runs `run_agent`, a deflected turn a direct `chat_stream` on the same `messages`), and `app/rag/agent.py:1370/1448` — `run_agent(..., user_message: str)` builds its own `[system, user]` (verify the data flow — if `run_agent` receives the already-built `messages`, the single edit site is chat.py). The phase-114 SSE error-frame-with-hint pattern (the "question too long" frame — `ChatErrorEvent.detail` + optional `hint`, consumed by the banner at app.js L2210). `frontend/index.html` — the composer (label L287, `#message-input` L292, `#send-btn` L325). `frontend/assets/app.js` — `handleSend` (L2306), `runTurn` (the turn driver + the user append/save-point-1 at send), `addMessage("user", …)` (user bubble), `rememberBrainTurn` (L2108, the brain save point), `renderStoredMessage` (L1642, user branch). `frontend/assets/shared.js` — the shared page's message render (text-only today). `app/api/config.py:30` — the public flags dict (phase 122 task 01 added `images`). + +## Objective +The user attaches one image to a question: a masked-by-server upload stores the bytes, the vision model (the chat model) receives a multimodal message, the user's bubble renders the image, the record persists the image path (not base64) so refresh and shared chats render it, and `BOR_IMAGES=false` rejects the request with a helpful hint. + +## Dependencies +- `122_image_documents` (todo) — CODE dependency: the `BOR_IMAGES`/`images` config flag (the toggle gates this feature), the ext→mime map, and the `image_dir` storage convention (this phase's `chat_image_dir` follows it). +- Code dependencies (all complete): phase 14/50/55 conversation persistence, phase 74 history mapping, phase 114 SSE error-hint frames, phase 51 shared chats. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **Storage (task 01, LOCKED A5):** user question-images are server-stored, NOT base64-in-saved-chats: `Settings.chat_image_dir: str = "~/bor-sources/chat-images"` (`BOR_CHAT_IMAGE_DIR`, the `image_dir` convention — a sibling of phase 122's `image_dir`, separate because question-images are per-conversation, not per-source) + `Settings.chat_image_max_mb: int = 10` (`BOR_CHAT_IMAGE_MAX_MB`, the ~10 MB cap of A5; `upload_max_mb`'s fail-loud validator precedent for `<= 0`). `POST /api/chat-images` (multipart, in `app/api/chat.py` or a small new `app/api/chat_images.py` router — the executor's call, following the repo's one-concern-per-module style): accepts an image file, validates the mime/ext against the SAME six-extension set as phase 122 (reuse the frozenset; the Content-Type header is a hint — the EXTENSION is the source of truth, the archive-uploader precedent), rejects oversize with a 413 (the fixed-detail style), stores `chat_image_dir/.`, returns `{ "path": "/api/chat-images/." }`. `GET /api/chat-images/{filename}` serves the bytes (404 on missing/unknown — the filename is a uuid, no enumeration value) with the phase-122 mime map; PUBLIC like saved-chat content (a saved chat's id is already its credential — phase 55 A1 — the image is part of that content). +- **Request (task 01):** `ChatRequest.image: str | None = Field(default=None, max_length=500)` — a STORED PATH, pattern-validated (`^/api/chat-images/[0-9a-fA-F]{32}\.(png|jpe?g|webp|gif|bmp)$` — the stored filename is `uuid4().hex.`) — never a raw data URL (the upload endpoint already did the size/mime enforcement; re-validating a 10 MB base64 string in the schema would be the anti-pattern). Toggle OFF (`settings.images` false) with `image` set → the turn settles with the phase-114 SSE error frame: `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." (the question itself is NOT persisted — a rejected turn saves nothing, the existing error-path convention). `image` set but file missing → the same frame shape with a "that image is no longer available" detail (a stale-path edge: the stored file was deleted out-of-band). +- **Multimodal (task 01):** the user message becomes `{"role": "user", "content": [{"type": "text", "text": request.message}, {"type": "image_url", "image_url": {"url": }}]}` at BOTH construction sites (chat.py:645 and agent.py:1448 if it builds independently — verify the flow; when `request.image` is None the content stays the plain string, byte-identical to today). The data URL is built server-side from the stored bytes + mime map (the phase-122 `describe_image` data-URL construction — reuse it). `HistoryTurn`/`history_to_messages` are UNCHANGED (LOCKED A7): prior turns' images are never replayed into the model's history — the history budget is text, and a 10 MB image per past turn would blow every budget; the model simply sees the text of a prior turn that had an image. +- **Persistence (tasks 01+02):** `ChatMessage.image: str | None = Field(default=None, max_length=500)` — the stored path, on the USER record (the image belongs to the question). The user record is saved at save-point-1 (send), BEFORE the turn resolves — so the client uploads FIRST (`POST /api/chat-images`) and stores the returned path in the user record, then POSTs `/api/chat` with `image=`. Saved chats, shared chats, and the localStorage shape all carry the path (≤500 chars — no phase-83 cap pressure). A brain record never carries `image` (the answer may reference the image's sources, but the attachment is the user's). +- **Composer (task 02):** the attach control appears ONLY when `GET /api/config` says `images: true` (phase 122's flag; fetched once at boot like the other config — the composer reads the existing cached config if present). A paperclip button (SVG, the icon style of the other composer glyphs, `aria-label="Attach an image"`) before the input → hidden `` → on select: a preview strip above the input (thumbnail ≤48px, the filename, a remove ✕) + the file's data URL kept client-side until send; on send with an attachment: `POST /api/chat-images` (the file) → the returned path goes into the user record + the `/api/chat` body → the preview clears. Upload failure (oversize, non-image, server down) → the phase-114-style out-of-turn banner ("Couldn't attach the image — …") and the send is BLOCKED (no question without the image the user attached — ASSUMPTION A8, locked below). The user bubble renders the image (from the data URL live, from the stored path after restore) with `alt = filename`, capped height, above/beside the text (the theme's bubble treatment; the image is part of the question, visible in both the live bubble and the restore). +- **Restore + shared (task 03):** `renderStoredMessage`'s user branch: `m.image` present → the user bubble includes `…` (a load failure collapses to a small "image unavailable" line — never a broken icon). The shared page (`shared.js`) renders the user image the same way (the image route is public — the shared view is faithful; no new shared-shape field beyond `ChatMessage.image`, which the public `messages` shape already carries). +- **NOT touched:** the history budget/trimming, the honesty gate, the suggestion chips, phase-122's document-image pipeline (a QUESTION image is a separate concern — it is NOT indexed as a document), and the stop/failed-turn paths (they persist whatever records exist, including the new `image` key, unmodified). + +## Tasks +1. `01_vision_request.md` — `POST/GET /api/chat-images`, `ChatRequest.image` + `ChatMessage.image`, the toggle-off/stale error frames, the multimodal user message at both construction sites. +2. `02_composer_attach.md` — the config-gated attach control, preview, upload-then-send, the user bubble's image. +3. `03_restore_and_shared.md` — `ChatMessage.image` on restore (chat page) and on the shared page. +4. `04_chat_image_tests.md` — unit + integration + isolated E2E `test_chat_image_questions.py`. + +## Testing & Quality +- Unit: `tests/unit/test_chat_image_questions.py` (new, task 04) — the path pattern validator (accepts well-formed, rejects data URLs / wrong ext / traversal), the multimodal message build (both sites; `image=None` → byte-identical plain string), the toggle-off + stale-file error frames (detail + hint shapes), the upload endpoint's mime/size/ext rules (tmp-dir settings), the serve route (200/404), `ChatMessage.image` bounds + omission. +- Integration: `tests/integration/test_chat_api.py` (extend, task 04) — upload → `POST /api/chat` with `image=` → the mock client RECEIVES the multimodal content list (text part + image_url data URL); `image` with `images=false` → the SSE error frame with the hint and NO model call, no persisted record; `image=None` requests are byte-identical to pre-phase; a saved chat round-trips a user record with `image`; a shared chat serves it. +- E2E: `tests/e2e/test_chat_image_questions.py` (new, task 04) — isolated run per AGENTS.md §4, `BOR_IMAGES=true`: attach a fixture PNG in the composer → preview + remove works → send → the user bubble shows the image → the (mock) answer streams → reload → the user bubble restores WITH its image → open the shared link → the shared page shows the image. Plus the default-off negative: with `BOR_IMAGES` unset, the attach control is ABSENT from the DOM. +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] With `BOR_IMAGES=true`: attach → send → the vision model gets text+image; the user bubble, the refreshed page, and the shared chat all show the image; the saved chat stores the PATH (assert no base64 in the stored payload). +- [ ] With `BOR_IMAGES=false`: the attach control is absent, an API request with `image` gets the hinted error frame, and no model call / record happens. +- [ ] Text-only questions behave byte-identically to pre-phase (the multimodal branch is inert). +- [ ] `uv run pytest` green; `uv run pytest --cov=app --cov-report=term-missing` TOTAL >90%; `uv run ruff check . && uv run pyright` clean. +- [ ] One `--no-gpg-sign` commit; phase dir moved to `.agents/phases/complete/` by the pipeline gate. + +## Locked decisions +- **A5 — one image per question; the ~10 MB cap (`BOR_CHAT_IMAGE_MAX_MB`); server-stored bytes under `chat_image_dir`; the saved/shared record carries the path, never base64 (owner-confirmed 2026-09-24, roadmap confirmation).** +- **A7 — a question's image applies to the CURRENT turn only; prior turns' images are never replayed into the model's history (the text of a prior turn stands alone) (owner-confirmed: same confirmation — the proposed design).** +- **A8 — if the image upload fails, the send is blocked with a banner (the question is never sent without the image the user attached) (owner-confirmed: same confirmation).** + +## Commit +```bash +git add app/ frontend/ tests/ .env.example .agents/phases/ && git commit --no-gpg-sign -m "feat(chat): attach an image to a question — vision input, in-bubble render, persisted and shared" +``` diff --git a/.agents/phases/todo/123_chat_image_questions/01_vision_request.md b/.agents/phases/todo/123_chat_image_questions/01_vision_request.md new file mode 100644 index 0000000..740fe59 --- /dev/null +++ b/.agents/phases/todo/123_chat_image_questions/01_vision_request.md @@ -0,0 +1,31 @@ +# Task 01 — Vision request: upload/serve endpoints, request + message schemas, multimodal build + +**Phase:** `123_chat_image_questions` · **Source:** `TODO.md:6` — "…users should be able to submit images as part of their question in brain of reese." + +## Objective +The server side of the image question: a uuid-named upload/serve pair for question images, `ChatRequest.image` (stored path) + `ChatMessage.image` (persistence), the toggle-off/stale error frames, and the multimodal user message at both construction sites — with text-only requests byte-identical to pre-phase. + +## Work +1. `app/config.py` — `chat_image_dir: str = "~/bor-sources/chat-images"` (`BOR_CHAT_IMAGE_DIR`, the phase-122 `image_dir` convention) + `chat_image_max_mb: int = 10` (`BOR_CHAT_IMAGE_MAX_MB`, the A5 cap; the `upload_max_mb` fail-loud `<= 0` validator precedent). `.env.example` entries. +2. New router (a small `app/api/chat_images.py`, registered in `app/main.py` next to the chat router — the one-concern-per-module house style): + - `POST /api/chat-images` — `UploadFile` (the git-sources upload endpoint L432 pattern): extension must be in the phase-122 image frozenset (the EXTENSION is the source of truth — a Content-Type header is a hint); total bytes capped at `chat_image_max_mb` (stream-count the bytes — reject with 413 + a fixed detail that names the cap, never echoing the filename); store `chat_image_dir/.` (dir created on demand); response `{"path": "/api/chat-images/."}`. + - `GET /api/chat-images/{filename}` — filename must be `.` (the regex guard → 404 otherwise, no path traversal by construction); 404 on missing file; serve the bytes with the phase-122 ext→mime map + `Cache-Control: private, max-age=3600` (public, like saved-chat content — phase 55 A1). +3. `app/schemas.py`: + - `ChatRequest` (L60): `image: str | None = Field(default=None, max_length=500)` + a `field_validator` — when set, it must match `^/api/chat-images/[0-9a-fA-F]{32}\.(png|jpe?g|webp|gif|bmp)$` (the uuid4().hex shape — 32 hex chars; adjust if the uuid format differs) with a fixed 422 detail ("image must be an uploaded chat image path" — no echo). Docstring: the path from `POST /api/chat-images` (task 01) — never a data URL; the upload endpoint owns size/mime enforcement. + - `ChatMessage` (L742, `extra="forbid"`): `image: str | None = Field(default=None, max_length=500)` — on the USER record only (the question's attachment); the docstring notes brain records never carry it and the saved/shared shape therefore gains one optional key (omitted when None — the phase-50 byte-identical contract for text-only chats holds). +4. `app/api/chat.py` — the turn pipeline: + - pre-stream validation (BEFORE any model call, at the top of the turn handler): `request.image` set → `settings.images` false → yield the phase-114 error frame (`ChatErrorEvent(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.")`) and return (NO model call, NO record — the existing error-path convention); file missing on disk → the same frame shape, `detail="That image is no longer available."` + a generic reachability-free hint (or no hint — the banner's default is fine). + - the user message (L645): when `request.image` is set, `{"role": "user", "content": [{"type": "text", "text": request.message}, {"type": "image_url", "image_url": {"url": }}]}` — the data URL built from the stored bytes + the phase-122 mime map (REUSE the data-URL construction from `describe_image` — factor it to a shared helper if it is buried in `app/rag/summarizer.py`); `request.image` None → the plain-string content, byte-identical. + - `app/rag/agent.py` — verify the data flow: if `run_agent` (L1370) receives the already-built `messages` from chat.py, NO change here (the L1448 build is for a different entry); if it builds its own user message from `user_message`, extend `run_agent`'s signature (`user_message: str | list | None` — pyright-clean) and make chat.py pass the multimodal content. Pin the chosen flow in a code comment. + - a QUESTION image is NEVER indexed as a document (no importer call) — it is turn-local storage. +5. ASSUMPTION (A7 re-stated): `HistoryTurn`/`history_to_messages` unchanged — prior turns' images are not replayed (text-only history stands). + +## Testing & Quality +- Unit: `tests/unit/test_chat_image_questions.py` (task 04 finalizes) — the path validator (well-formed ok; a data URL, a wrong ext, a traversal, and a 31-hex-char uuid all 422); the multimodal builder (both sites; None → plain string); the error frames' exact detail/hint strings; the upload endpoint's ext/size rules (tmp `chat_image_dir`); the serve route 200/404 + Content-Type; `ChatMessage.image` (bounds, omission, the `extra="forbid"` boundary intact). +- Integration: `tests/integration/test_chat_api.py` (task 04) — the upload → chat flow asserts the MOCK client received the multimodal content list; the toggle-off frame + no model call; text-only byte-identity; saved + shared round-trips with `image`. +- Coverage: **>90%** on the touched modules. + +## Completion Criteria +- [ ] `POST /api/chat-images` stores a uuid-named file and returns its path; `GET` serves it; oversize/non-image → 413/422 with fixed details. +- [ ] `POST /api/chat` with `image` (toggle on) delivers a multimodal user message to the model; toggle off → the hinted error frame, no model call; `image=None` → byte-identical behavior. +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/123_chat_image_questions/02_composer_attach.md b/.agents/phases/todo/123_chat_image_questions/02_composer_attach.md new file mode 100644 index 0000000..c29cef0 --- /dev/null +++ b/.agents/phases/todo/123_chat_image_questions/02_composer_attach.md @@ -0,0 +1,38 @@ +# Task 02 — Composer: attach control, preview, upload-then-send, the user bubble's image + +**Phase:** `123_chat_image_questions` · **Source:** `TODO.md:6` — "…users should be able to submit images as part of their question in brain of reese." + +## Objective +The composer (when `GET /api/config` says `images: true`) takes one attached image — preview + remove before send, upload on send, the image in the user's bubble — and the user's conversation record carries the stored `image` path. + +## Work +1. `frontend/index.html` — the composer (the label L287 / `#message-input` L292 / `#send-btn` L325 region): before the input, the attach control + ```html + + + ``` + plus the preview strip container (after the input row, `#attach-preview`, `hidden` by default — a thumbnail ≤48px + filename + a remove ✕ button `#attach-remove`). `#attach-btn` is `hidden` by default — JS reveals it only when the config flag is on (task 02 step 3); the hidden-by-default markup keeps the flag-off DOM byte-identical (A5's default-off contract). +2. `frontend/assets/app.js`: + - boot: read `images` from the `GET /api/config` fetch (the composer already consumes the cached whoami/config boot — extend that fetch's result use; ONE request, no extra round-trip) → `attachBtn.hidden = !images`. + - attach flow: `#attach-btn` click → `attachFile.click()`; on change: validate the file's extension against the six (client-side pre-check, the server re-validates — a bad pick → the out-of-turn banner "Only PNG, JPEG, WebP, GIF, and BMP images can be attached." and no state change); keep `{ file, dataUrl (for the live preview) }` in a turn-local `attachedImage` var; show `#attach-preview` (thumbnail from the data URL, the filename, the remove ✕); the remove ✕ (or a new selection) clears the state + hides the strip. + - send flow (`handleSend` L2306 / `runTurn`): when `attachedImage` is set: + 1. `POST /api/chat-images` (the File) — on failure (413/422/5xx) → the phase-114-style out-of-turn banner with the server's detail ("Couldn't attach the image — try again.") and the send is BLOCKED (LOCKED A8 — the question is never sent without its image; the input text stays). + 2. success → `runTurn(text, { image: })`; `runTurn`'s user append (save point 1 — the user record) stores `{ who: "user", text, image: }` (the `image` key joins the `bor.chat.v1` record — the phase-14 shape gains the optional key; `saveConversation()` + the phase-55 auto-save ride the existing path); + 3. the USER bubble renders the image: extend `addMessage("user", text)` with an optional `image` arg (data URL live, path after restore) → `{filename}` in the bubble (capped height ~240px, `max-width: 100%`, the theme's bubble treatment, `loading="lazy"`); + 4. clear `attachedImage` + the preview strip AFTER the user bubble is rendered (the strip must not linger into the turn). + - text-only sends: `attachedImage` null → the request body omits `image`, the user record omits the key, the bubble is byte-identical to pre-phase. +3. `frontend/assets/styles.css` — `.attach-btn` (the composer glyph button treatment — match the send-btn family, focus-visible ring per PLAN §7), `#attach-preview` (the strip: flex row, thumbnail box, filename ellipsis, the ✕), the user-bubble image block. +4. ASSUMPTION (A8 re-stated): an upload failure blocks the send (no partial question-without-image) — the banner tells the user what failed; the typed question is preserved. + +## Testing & Quality +- Unit: `tests/unit/test_chat_image_questions.py` (task 04) — house-style source assertions: `#attach-btn` is `hidden` by default + `aria-label`; the reveal is gated on the config `images` flag; the extension pre-check list matches the server's six; the send path uploads BEFORE `runTurn` and blocks on failure (the A8 ordering); the user record gains `image` only when attached; the user bubble renders the `img` with `alt`. +- E2E: `tests/e2e/test_chat_image_questions.py` (task 04) — the composer scenarios. +- Coverage: n/a (frontend) — the validate.sh `app/` gate must stay green. + +## Completion Criteria +- [ ] Flag on: attach → preview → remove all work; send with an attachment uploads, the user bubble shows the image, and the question reaches the model. +- [ ] Flag off: the attach button is ABSENT from the DOM; a hand-crafted `image` request still gets the server's error frame (task 01's contract, unchanged). +- [ ] A text-only send produces the same request body and DOM as pre-phase (byte-check in the E2E where practical). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/123_chat_image_questions/03_restore_and_shared.md b/.agents/phases/todo/123_chat_image_questions/03_restore_and_shared.md new file mode 100644 index 0000000..418d9d5 --- /dev/null +++ b/.agents/phases/todo/123_chat_image_questions/03_restore_and_shared.md @@ -0,0 +1,26 @@ +# Task 03 — Restore + shared: the question's image survives a refresh and a share link + +**Phase:** `123_chat_image_questions` · **Source:** `TODO.md:6` — "…users should be able to submit images as part of their question in brain of reese." + +## Objective +A user record carrying `image` (the stored path) renders its image when the chat is restored from localStorage / the saved-chat API, and on the shared-chat page — a load failure degrades to a small note, never a broken icon. + +## Work +1. `frontend/assets/app.js` — `renderStoredMessage(m)` (L1642), the USER branch: when `m.image` is present, the restored user bubble includes `{m.text || 'attached image'}` through the SAME bubble-image helper task 02 built for the live bubble (one renderer — the live bubble passes the data URL, restore passes the path; the helper takes any `src`). `onerror` → replace the image with a small "image unavailable" line (the file was deleted out-of-band — the row keeps its path, the render degrades). + - The restore paths that call `renderStoredMessage` (the localStorage restore ~L1700 and the saved-chat restore ~L1770) need NO other change — the record's `image` key flows through the phase-14/50 restore as any optional key. + - `retryLastTurn` / regenerate: a RE-ASK of a question that had an image does NOT re-attach the image (the redo re-sends `prev.text` only — LOCKED A7, the image is turn-local to the original send; the restored image stays visible in the replaced record until the redo pops it, which is the existing redo-in-place behavior). +2. `frontend/assets/shared.js` — the shared page's message render (its text-only loop over `messages`): the user-record branch gains the same image render (the image route is public — a shared chat is faithful; the `alt` + `onerror` degradation are identical to the chat page). +3. `frontend/assets/styles.css` — no new rules beyond what task 02 added (the shared page reuses the bubble-image block; verify the shared page's bubble class shares it — if the shared page uses a different bubble class, scope the image rule to both). +4. ASSUMPTION: the `image` key is optional and absent in every pre-phase saved chat — no data migration, no backfill (old chats have no question-images to restore). + +## Testing & Quality +- Unit: `tests/unit/test_chat_image_questions.py` (task 04) — house-style source assertions: the user-branch render reads `m.image` and reuses the bubble-image helper; the `onerror` degradation exists on BOTH pages; the shared render includes the image; the redo path sends `prev.text` only (no `image` on the re-ask). +- Integration: `tests/integration/test_chats_api.py` (extend, task 04) — a user record with `image` round-trips `POST`/`PUT /api/chats` and serves through `GET /api/shared/{token}` (the public shape carries it). +- E2E: the refresh + shared scenarios of `tests/e2e/test_chat_image_questions.py` (task 04). +- Coverage: n/a (frontend) — the validate.sh `app/` gate must stay green. + +## Completion Criteria +- [ ] Reload after an image question: the user bubble shows the image (from the stored path) + the rest of the conversation is unchanged. +- [ ] The shared link renders the image on the shared page. +- [ ] A deleted image file degrades to the "image unavailable" line on both pages (no broken-image icon). +- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/todo/123_chat_image_questions/04_chat_image_tests.md b/.agents/phases/todo/123_chat_image_questions/04_chat_image_tests.md new file mode 100644 index 0000000..4106192 --- /dev/null +++ b/.agents/phases/todo/123_chat_image_questions/04_chat_image_tests.md @@ -0,0 +1,39 @@ +# Task 04 — Chat-image tests: unit + integration + isolated E2E + +**Phase:** `123_chat_image_questions` · **Source:** `TODO.md:6` — "…users should be able to submit images as part of their question in brain of reese." + +## Objective +Pin the whole question-image contract: the upload/serve rules, the multimodal model payload, the toggle-off rejection, the persistence shape (path, never base64), the composer gating, and the refresh/share rendering — as an isolated Playwright suite. + +## Work +1. `tests/unit/test_chat_image_questions.py` (new) — consolidates the per-task unit cases (tasks ship code; this task ships the full pin): + - config: `chat_image_dir`/`chat_image_max_mb` defaults + env overrides; + - the `ChatRequest.image` validator (well-formed path ok; data URL / wrong ext / traversal / malformed uuid → 422, fixed details, no echo); + - the multimodal builder (text part + `image_url` data-URL part, correct mime; `image=None` → the plain string, byte-identical); the data-URL helper is shared with `describe_image` (assert the import, not a copy); + - the error frames: toggle-off (exact detail + hint strings), stale file (exact detail) — both settle the turn WITHOUT a model call (the mock client must see zero calls); + - the upload endpoint: the six exts accepted, others 422/413-style per the spec, oversize → 413 (fixed detail naming the cap), the stored filename is `.`; + - the serve route: 200 + Content-Type per ext, 404 for missing/unknown/traversal filenames; + - `ChatMessage.image` (max 500, omission when None, `extra="forbid"` intact — an unknown key still 422s); + - frontend source assertions (tasks 02+03): the attach button hidden-by-default + config-gated reveal, the A8 upload-before-send ordering + block-on-failure, the user record's `image` key, the shared bubble render, the `onerror` degradation, the redo sends text-only. +2. `tests/integration/test_chat_api.py` (extend, task 01's cases): + - `POST /api/chat-images` → `POST /api/chat` with the returned path → the MOCK client received the multimodal content list (text == the question, data URL decodes to the uploaded bytes); + - `images=false` + `image` → the SSE error frame with the hint; the mock client got NO call; NO saved record; + - `image=None` → the model payload is byte-identical to a pre-phase request; + - a saved chat (and a shared one) round-trips a user record with `image` — and assert the stored payload contains NO base64 (the path only — the A5 contract). +3. `tests/integration/test_chats_api.py` (extend, task 03's case): the shared-chat serve includes the user record's `image` path. +4. `tests/e2e/test_chat_image_questions.py` (new — isolated run per AGENTS.md §4: `uv run pytest tests/e2e/test_chat_image_questions.py -v --no-cov`), `BOR_IMAGES=true` for this suite's app instance (the phase-122 E2E env-override pattern): + - attach a fixture PNG in the composer → the preview strip shows (thumbnail + filename) → remove → the strip clears and the file state is gone; + - re-attach → send → the user bubble shows the image; the mock LLM's (text-only) answer streams normally (the mock ignores the image part — the assertion is on the REQUEST the server built, verified via the mock's capture); + - `page.reload()` → the user bubble restores WITH its image (the stored path, not the data URL — the request count for the image route confirms the path fetch); + - share the chat (the existing share flow) → open the shared link → the shared page shows the user's image; + - default-off negative (a second app instance or the suite's flag-off fixture): `#attach-btn` is ABSENT from the DOM; a direct `POST /api/chat` with an `image` path returns the hinted error frame (no model call). +5. Run the full gate: `uv run pytest`, `uv run pytest --cov=app --cov-report=term-missing` (TOTAL >90%), the isolated E2E file, `uv run ruff check . && uv run pyright`. + +## Testing & Quality +- This task IS the phase's test suite (see Work). +- Coverage: **>90%** on `app/` — the phase's `app/` surface (config, the chat-images router, the chat pipeline, the schemas) is fully exercised. + +## Completion Criteria +- [ ] All test artifacts exist and pass; the isolated E2E file passes standalone. +- [ ] The multimodal payload, the no-base64-in-storage, and the toggle-off rejection are each asserted at the unit AND integration level. +- [ ] `uv run pytest --cov=app` TOTAL >90%; lint + types clean.