diff --git a/.agent/phases/todo/67_llm_retry/00_phase.md b/.agent/phases/todo/67_llm_retry/00_phase.md new file mode 100644 index 0000000..d11d455 --- /dev/null +++ b/.agent/phases/todo/67_llm_retry/00_phase.md @@ -0,0 +1,51 @@ +# Phase 67 — LLM Retry with Live "Trying Again" Feedback + +**Source:** `TODO.md` L3 — "Add a .env configurable retry in case the LLM server fails to respond. Allow 3 retries by default, with 5 seconds between each retry. Update the user interface to show 'communication interrupted, trying again' or something like that if the LLM server stops communicating." +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) +**Context:** +- `app/rag/llm.py` — `LLMClient.chat_stream` is the single streaming surface (deflected turns + every agent round); `LLMError` is the typed failure the API layer already turns into an SSE `error` frame (`app/api/chat.py` — `except LLMError` around the piece loop; `EmbeddingError` around the pre-stream `llm.embed_one(request.message)`). +- `app/rag/agent.py` — `run_agent` issues one `chat_stream` per round (plus a final `tools=None` call at the round cap); the phase-48 teardown binds each stream and closes it in a `finally`. +- `app/config.py` — the `BOR_*` settings block (LLM section: `llm_base_url`, `llm_chat_model`, …); `agent_max_rounds` shows the house pattern for a tunable with a startup validator. +- `app/schemas.py` — the SSE event family (`ChatThinkingEvent`, `ChatToolEvent`, `ChatDoneEvent`, `ChatErrorEvent`). +- `frontend/assets/app.js` — `runTurn`'s `readSSE` callback is the event state machine; the `tool` branch is the house pattern for a server-driven STATUS change (`#send-status` + typing-indicator `aria-label`, no new bubble, `clearTurnTimeout()` because a frame arrived). `tests/e2e/test_agent_document_tools.py` (L236+) records every `#send-status` value during a turn for assertions. +- `tests/e2e/mock_llm.py` — deterministic marker-driven OpenAI-compatible stand-in (chat + embeddings), run as a uvicorn subprocess by `tests/e2e/conftest.py`; the marker flow is discriminated statelessly from the request. +- **Not in scope (owner-locked A1):** the one-shot `LLMClient.chat()` path (document summaries, KB overview) and the sync probe (`check_models`) — those are admin/import paths with their own fail-fast behavior (phase 41) and no live user to notify. + +## Objective +When the aipi endpoint dies mid-turn, the app retries the LLM request automatically — `.env`-tunable, **3 retries / 5 s delay by default** — and the UI tells the user what is happening ("Communication interrupted — retrying (n of N)…") instead of the turn dead-ending in an error banner. A retry only ever restarts a request that has **not yet streamed a single output frame** to the client (locked A2), so no answer token is ever duplicated. + +## Dependencies +- `66_history_auto_save_copy` (todo, preceding — no functional dependency; ordering by number) + +## Tasks +1. `01_config_and_retry_primitive.md` — `BOR_LLM_RETRIES` / `BOR_LLM_RETRY_DELAY` settings + the `RetryPiece` + `chat_stream_retried()` primitive in `app/rag/llm.py`. +2. `02_chat_endpoint_retry.md` — the `retry` SSE event, the embedding retry loop, and the deflected-stream retry in `app/api/chat.py`. +3. `03_agent_round_retry.md` — per-round retries inside `run_agent` (loop rounds + final no-tools call). +4. `04_frontend_retry_status.md` — the `retry` branch in `runTurn`'s SSE handler: the live "retrying (n of N)…" status. +5. `05_e2e_and_commit.md` — `mock_llm.py` failure injection, `tests/e2e/test_llm_retry.py`, regressions, commit. + +## Testing & Quality +- Unit: `tests/unit/test_config.py` (new vars, defaults, validators), `tests/unit/test_llm_client.py` (`chat_stream_retried` semantics — retry only before the first piece, `RetryPiece` ordering, exhaustion, `retries=0`), `tests/unit/test_agent.py` (per-round retry), `tests/unit/test_frontend_tool_states.py` pattern (JS pins for the `retry` branch). +- Integration: `tests/integration/test_chat_api.py` — SSE frame ordering (embed-fail → `retry` frame → completed turn; embed-exhausted → `retry` frames + terminal `error` frame; mid-stream failure AFTER a delta → no retry, `error` frame). +- E2E (mandatory, house rule): `tests/e2e/test_llm_retry.py`, run in isolation (mock LLM with deterministic failure injection; `BOR_LLM_RETRY_DELAY=0` on the test server so the suite stays fast). +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] `BOR_LLM_RETRIES` (default 3) and `BOR_LLM_RETRY_DELAY` (default 5 s) are honored end to end and documented in `.env.example`. +- [ ] A dead-then-recovered endpoint: the turn completes with a normal answer and the UI showed the "retrying" status while waiting; a dead endpoint: after N attempts the existing terminal error banner appears. +- [ ] A stream failure after the first output frame still terminates with the `error` event — no retry, no duplicated tokens. +- [ ] `uv run pytest` green; coverage TOTAL >90%; `uv run ruff check . && uv run pyright` clean. +- [ ] `uv run pytest tests/e2e/test_llm_retry.py -v --no-cov` green in isolation (DB up). +- [ ] Regression E2E suites green in isolation: `test_chat_rag.py`, `test_agent_document_tools.py`, `test_stop_generation.py`, `test_retry_answer.py`. +- [ ] One `--no-gpg-sign` commit; phase dir moved to `.agent/phases/complete/`. + +## Locked decisions +- **Owner-locked (2026-09-01, roadmap confirmation, A1):** scope is the **chat turn only** — question embedding + the answer stream (deflected path and every agent round). `LLMClient.chat()` (summaries, KB overview) and `check_models` (sync probe) are untouched. +- **Owner-locked (2026-09-01, roadmap confirmation, A2):** a retry restarts the LLM request **only if no output frame has been streamed to the client yet** for that request (no thinking/tool/delta emitted). Once tokens are flowing, the failure stays terminal (the existing `error` frame) — a partial answer is never redone. +- **Owner-locked (2026-09-01, roadmap confirmation, A3):** env names `BOR_LLM_RETRIES` (int, default **3**) and `BOR_LLM_RETRY_DELAY` (seconds, default **5**) — a flat delay between attempts, no exponential backoff (the TODO specifies a fixed 5 s). +- **Owner-locked (2026-09-01, roadmap confirmation, A4):** UI copy — `#send-status` reads `Communication interrupted — retrying (n of N)…` (n = current attempt, N = the configured retry count) on the existing status line; no new banner, no bubble. + +## Commit +```bash +git add -A .agent/ app/ tests/ frontend/ && git commit --no-gpg-sign -m "feat(rag): retry a failed LLM request before the first token lands — BOR_LLM_RETRIES/BOR_LLM_RETRY_DELAY with a live 'retrying' status" +``` diff --git a/.agent/phases/todo/67_llm_retry/01_config_and_retry_primitive.md b/.agent/phases/todo/67_llm_retry/01_config_and_retry_primitive.md new file mode 100644 index 0000000..cb69b02 --- /dev/null +++ b/.agent/phases/todo/67_llm_retry/01_config_and_retry_primitive.md @@ -0,0 +1,44 @@ +# Task 01 — Retry Settings + the `chat_stream_retried` Primitive + +**Phase:** `67_llm_retry` · **Source:** `TODO.md:3` — "Add a .env configurable retry in case the LLM server fails to respond. Allow 3 retries by default, with 5 seconds between each retry. Update the user interface to show 'communication interrupted, trying again' or something like that if the LLM server stops communicating." +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) + +## Objective +The two `.env` knobs (`BOR_LLM_RETRIES=3`, `BOR_LLM_RETRY_DELAY=5`) and one shared streaming primitive — `chat_stream_retried()` in `app/rag/llm.py` — that the chat endpoint (task 02) and the agent loop (task 03) both build on. The primitive is the ONLY place the retry-before-first-piece rule (locked A2) lives. + +## Work +1. `app/config.py` — in the `--- LLM ---` settings block (next to `llm_chat_model` / `stream_thinking`), add: + - `llm_retries: int = 3` — comment: retries of a failed LLM request when the endpoint stops responding (phase 67, `BOR_LLM_RETRIES`); `0` = no retries (the turn fails on the first error, pre-phase-67 behavior). + - `llm_retry_delay: float = 5.0` — comment: flat seconds to wait between attempts (phase 67, `BOR_LLM_RETRY_DELAY`); the TODO-locked 5 s, no backoff. + - Startup validators (house pattern: `agent_max_rounds` rejects negatives at startup): `llm_retries >= 0`, `llm_retry_delay >= 0`. +2. `.env.example` — in the LLM section, add the two commented defaults next to `BOR_LLM_CHAT_MODEL`: + - `# BOR_LLM_RETRIES=3 # retry a dead LLM request before the first token lands (phase 67); 0 = off` + - `# BOR_LLM_RETRY_DELAY=5 # seconds between LLM retries (phase 67)` +3. `app/rag/llm.py` — add, next to the other piece dataclasses: + - `RetryPiece` — frozen dataclass, fields `attempt: int` (1-based attempt number that is about to be tried), `max_attempts: int` (total attempts = `llm_retries + 1`). One per wait; the API layer turns it into an SSE `retry` frame. + - `async def chat_stream_retried(llm: LLMClient, messages: list[dict[str, str]], *, tools: list[dict[str, Any]] | None = None, retries: int = 0, delay: float = 0.0) -> AsyncGenerator[StreamPiece | ToolCallPiece | RetryPiece, None]`: + - Loop `attempt` over `range(1, retries + 2)` (i.e. `retries + 1` attempts). + - Each attempt: open `stream = llm.chat_stream(cast(...), tools=tools)`; track `emitted = False`. + - `async for piece in stream`: mark `emitted = True`, `yield piece`. + - On `LLMError`: if `emitted` → **re-raise unchanged** (terminal — locked A2: tokens already flowed, never redo a partial). Else if this was the final attempt → re-raise (exhausted). Else: `logger.warning("llm stream failed before the first piece (attempt %d/%d) — retrying in %.1fs: %s", ...)`, `yield RetryPiece(attempt, retries + 1)`, `await asyncio.sleep(delay)`, and start the next attempt with the SAME `messages`/`tools` (the request is restarted byte-identical — `chat_stream` is stateless). + - `finally: await stream.aclose()` per attempt (keeps phase 48's deterministic teardown for every attempt's stream, including a consumer abandon during the sleep or a mid-attempt GeneratorExit). + - `retries=0` → exactly one attempt, never a `RetryPiece` (the pre-phase-67 path, the kill-switch). + - Docstring: state the A2 rule explicitly and that `RetryPiece` always precedes its sleep (the API frame must reach the client before the wait starts). +4. Unit tests — `tests/unit/test_llm_client.py` (append; a fake `LLMClient` whose `chat_stream` is scripted): + - failure on attempt 1, success on attempt 2 → pieces = `[RetryPiece(1, N), *answer pieces]`, `chat_stream` called twice, sleep awaited with the delay (monkeypatch `asyncio.sleep` and record calls). + - failure on every attempt with `retries=2` → `RetryPiece(1, 3)`, `RetryPiece(2, 3)` then `LLMError` raised; 3 calls total. + - failure AFTER the first piece → `LLMError` raised immediately, no `RetryPiece`, sleep never awaited, no second call (the A2 pin). + - `retries=0` → one call, error propagates, no `RetryPiece`. + - `delay=0` → sleep called with 0 (the e2e fast-path). + - a consumer abandon (close the outer generator) mid-sleep and mid-attempt → no exception leaks, inner stream `aclose()` awaited. +5. `tests/unit/test_config.py` — defaults (`3` / `5.0`), `BOR_LLM_RETRIES=0` and `BOR_LLM_RETRY_DELAY=1.5` honored, negative values rejected at startup. + +## Testing & Quality +- Unit/integration: as listed in Work (4). +- Coverage: **>90%** on this task's new/modified code. + +## Completion Criteria +- [ ] `Settings()` defaults: `llm_retries == 3`, `llm_retry_delay == 5.0`; `.env.example` documents both. +- [ ] `chat_stream_retried` passes all unit pins, including the A2 no-retry-after-first-piece rule. +- [ ] `uv run pytest tests/unit/test_llm_client.py tests/unit/test_config.py -v` green; `uv run ruff check . && uv run pyright` clean. +- [ ] no behavior change in completed work (the plain `llm.chat_stream` is untouched — the new function is additive). diff --git a/.agent/phases/todo/67_llm_retry/02_chat_endpoint_retry.md b/.agent/phases/todo/67_llm_retry/02_chat_endpoint_retry.md new file mode 100644 index 0000000..ec633cb --- /dev/null +++ b/.agent/phases/todo/67_llm_retry/02_chat_endpoint_retry.md @@ -0,0 +1,40 @@ +# Task 02 — The `retry` SSE Event + Endpoint-Level Retries (Embedding, Deflected Stream) + +**Phase:** `67_llm_retry` · **Source:** `TODO.md:3` — "…Update the user interface to show 'communication interrupted, trying again' or something like that if the LLM server stops communicating." +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) + +## Objective +`POST /api/chat` survives a dead endpoint: the pre-stream embedding and the deflected-turn stream retry (with SSE `retry` frames the UI can render live), and a `RetryPiece` from any answer stream becomes a `retry` SSE frame. Exhaustion and post-token failures keep today's terminal `error` frames. + +## Work +1. `app/schemas.py` — add `ChatRetryEvent` next to `ChatErrorEvent`: + - `type: Literal["retry"] = "retry"`, `attempt: int` (the attempt number that just failed / the retry currently in flight — document which the endpoint sends: the attempt being tried next, 1-based), `max_attempts: int` (`llm_retries + 1`). + - Docstring: sibling of the other SSE events; signals "the LLM request was restarted before any token landed" (locked A2); the client shows a transient status, not an error. +2. `app/api/chat.py` — the embedding step (currently: `try: question_vec = await llm.embed_one(...) except EmbeddingError → error frame`): + - Replace with an explicit attempt loop over `settings.llm_retries + 1` attempts (`attempt` 1-based): + - on `EmbeddingError` before the final attempt: `logger.warning("chat: question=%r embedding failed (attempt %d/%d) — retrying in %.1fs", ...)`, `yield sse_event(ChatRetryEvent(attempt=attempt + 1, max_attempts=settings.llm_retries + 1).model_dump())`, `await asyncio.sleep(settings.llm_retry_delay)`. + - on the final failure: the EXISTING terminal `error` frame ("I couldn't reach the embedding model — please try again.") unchanged — retries are exhausted, the copy stays (it reads correctly after N tries). + - `llm_retries=0` → byte-identical to today (single attempt, same frame on failure). + - track the turn-level retry count (`retries_used`) for the per-turn log line (step 4). +3. `app/api/chat.py` — the answer stream: + - deflected path: `answer_stream = llm.chat_stream(messages)` → `chat_stream_retried(llm, messages, tools=None, retries=settings.llm_retries, delay=settings.llm_retry_delay)`. + - grounded path: unchanged call to `run_agent(...)` (task 03 makes IT retry internally); the shared piece loop below handles its `RetryPiece`s. + - piece loop: add the `RetryPiece` branch (alongside the `ToolCallPiece` branch): `yield sse_event(ChatRetryEvent(attempt=piece.attempt, max_attempts=piece.max_attempts).model_dump())` and fold the attempt into `retries_used` (count each `RetryPiece`). No other state changes (the thinking/clock/timeout handling is the client's job). +4. `app/api/chat.py` — per-turn log line (PLAN §9): append a `retries=N` field (0 when nothing retried — the grounded/deflected/deflected-empty shapes all carry it, so the line shape is uniform; update the log-format comments and any test that pins the line shape). +5. Integration tests — `tests/integration/test_chat_api.py` (fake LLM client injected via the existing dependency override pattern): + - embedding fails once then succeeds → frames: `retry` (attempt 2) then the normal `delta`/`done` sequence; the answer completes. + - embedding fails on every attempt (`llm_retries=2`) → `retry` frames (attempts 2, 3) then the terminal `error` frame; the detail is the existing embedding copy. + - deflected stream: first attempt `LLMError` before any piece, second attempt streams → `retry` frame then `delta` frames + `done`. + - deflected stream: `LLMError` AFTER one delta frame → no `retry` frame, the existing terminal `error` frame ("The chat model dropped the connection — try again?"). + - `llm_retries=0` → today's behavior (one attempt, error frame, no `retry` frames). + - per-turn log line carries `retries=N` (assert on the captured log record). +6. `tests/unit/test_frontend_tool_states.py` pattern — `tests/unit/test_frontend_feedback.py` (or the sibling JS-pin file the executor finds for SSE-branch ordering): pin that `ev.type === "retry"` is a first-class branch in `runTurn`'s handler (task 04 implements it; the pin lands with this task's contract so the shape is locked early). If a dedicated branch-order pin file already exists, extend it instead of creating one. + +## Testing & Quality +- Unit/integration: as listed in Work (5, 6). +- Coverage: **>90%** on this task's new/modified code. + +## Completion Criteria +- [ ] A simulated dead-then-recovered endpoint returns a completed turn with a `retry` frame in between; a dead endpoint returns the existing terminal error frame after `llm_retries + 1` attempts. +- [ ] `uv run pytest tests/integration/test_chat_api.py -v` green; `uv run ruff check . && uv run pyright` clean. +- [ ] no behavior change when `BOR_LLM_RETRIES=0` (pre-phase-67 wire shape, byte-identical frames). diff --git a/.agent/phases/todo/67_llm_retry/03_agent_round_retry.md b/.agent/phases/todo/67_llm_retry/03_agent_round_retry.md new file mode 100644 index 0000000..cc8f439 --- /dev/null +++ b/.agent/phases/todo/67_llm_retry/03_agent_round_retry.md @@ -0,0 +1,31 @@ +# Task 03 — Per-Round Retries Inside the Agent Loop + +**Phase:** `67_llm_retry` · **Source:** `TODO.md:3` — "Add a .env configurable retry in case the LLM server fails to respond. Allow 3 retries by default, with 5 seconds between each retry." +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) + +## Objective +A grounded turn survives a dead endpoint in the MIDDLE of the agent loop: every model request `run_agent` makes (each tool round and the forced final no-tools call) goes through `chat_stream_retried`, so a round that dies before its first piece is restarted with the same messages — while the round-cap, tool-counting, and phase-48 teardown semantics stay exactly as they are. + +## Work +1. `app/rag/agent.py` — `run_agent`: + - loop round: `stream = llm.chat_stream(cast("list[dict[str, str]]", messages), tools=tools)` → `stream = chat_stream_retried(llm, cast(...), tools=tools, retries=settings.llm_retries, delay=settings.llm_retry_delay)`. Keep the phase-48 binding + `try/finally: await stream.aclose()` shape AROUND the new generator — closing the outer generator propagates `GeneratorExit` into `chat_stream_retried`, whose own `finally` closes the in-flight inner `chat_stream` (task 01). The inner stream's teardown therefore still happens deterministically on consumer abandon. + - forced final no-tools call: same substitution with `tools=None`. + - the `async for piece in stream` loop already yields every piece — `RetryPiece` values flow through to the API layer unchanged (no filtering); `calls` still collects only `ToolCallPiece`s. + - `retries=0` (or `agent_max_rounds=0`'s single `tools=None` request) → identical to today: one plain attempt. + - update the module docstring + the `run_agent` docstring: a failed round is retried before its first piece (phase 67, locked A2); a round that already streamed pieces fails the turn as before. +2. Unit tests — `tests/unit/test_agent.py` (scripted fake `LLMClient.chat_stream`): + - round 1 dies before any piece, round 1 retry succeeds with a `list_documents` tool call → pieces include a `RetryPiece` BEFORE the tool call; the tool executes; the final answer streams; `holder.tool_calls == 1`; `tool_calls` log line still emitted once. + - a round dies AFTER a content piece → `LLMError` propagates out of `run_agent`, no retry (A2), the holder is untouched. + - the forced final no-tools call (round cap reached) dies before its first piece → retried; the answer from the retry streams. + - `settings.llm_retries=0` → no `RetryPiece` ever; a dead round raises immediately (pre-phase-67 behavior). + - consumer abandon (close `run_agent`) while a retried round is mid-sleep → no leaked exception, inner stream closed. + - round-cap counting is unaffected by retries: a failing-then-succeeding round consumes ONE round (retries are invisible to the cap). + +## Testing & Quality +- Unit/integration: as listed in Work (2). +- Coverage: **>90%** on this task's new/modified code. + +## Completion Criteria +- [ ] A grounded turn whose agent loop's endpoint dies-then-recovers completes with the tool flow intact and a `RetryPiece` in the stream. +- [ ] `uv run pytest tests/unit/test_agent.py -v` green; `uv run ruff check . && uv run pyright` clean. +- [ ] the round cap, `holder` accounting, and teardown semantics are unchanged (existing `tests/unit/test_agent.py` pins still green). diff --git a/.agent/phases/todo/67_llm_retry/04_frontend_retry_status.md b/.agent/phases/todo/67_llm_retry/04_frontend_retry_status.md new file mode 100644 index 0000000..9aadde9 --- /dev/null +++ b/.agent/phases/todo/67_llm_retry/04_frontend_retry_status.md @@ -0,0 +1,39 @@ +# Task 04 — The Live "Communication interrupted — retrying" Status + +**Phase:** `67_llm_retry` · **Source:** `TODO.md:3` — "Update the user interface to show 'communication interrupted, trying again' or something like that if the LLM server stops communicating." +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) + +## Objective +When a `retry` SSE frame arrives, the composer status line tells the user exactly what is happening — `Communication interrupted — retrying (n of N)…` — using the house status pattern (the `tool` frames' treatment: `#send-status` + typing-indicator `aria-label`, no new bubble, turn-timeout reset). No CSS change. + +## Work +1. `frontend/assets/app.js` — `runTurn`'s `readSSE` callback, add a first-class branch (between the `tool` and `delta` branches — the branch-order unit pin from task 02 expects it): + ```js + } else if (ev.type === "retry") { + clearTurnTimeout(); // the stream is alive — the server is restarting the LLM request + const attempt = Number(ev.attempt) || 1; + const max = Number(ev.max_attempts) || 1; + const retryStatus = + `Communication interrupted — retrying (${attempt} of ${max})…`; + if (uiState === UI_STATE.thinking || uiState === UI_STATE.streaming) { + sendStatus.textContent = retryStatus; + document + .querySelector("#typing-indicator .bubble") + ?.setAttribute("aria-label", retryStatus); + } + } + ``` + - The status gate covers BOTH live states: a retry arrives only before the current request's first piece (server rule A2), but the UI may already be `streaming` when a LATER agent round restarts after an earlier round emitted content (the rare content+tool-call stream). + - NO new bubble, NO tool line, NO banner — it is a transient status; the next `thinking`/`tool`/`delta` frame replaces it via the existing branches. +2. `frontend/assets/app.js` — the file-header doc comment: add the `retry` frame to the SSE-event inventory (the header documents every frame type; keep the house convention). +3. JS unit pin (task 02's contract) — the pin file from task 02 now asserts: the branch exists between `tool` and `delta`; it calls `clearTurnTimeout()`; the locked copy literal `Communication interrupted — retrying (${attempt} of ${max})…` appears; no `addMessage`/`appendToolLine` in the branch. +4. A11y (PLAN §7 / AGENTS.md rule 5): the status line already announces through the existing `#send-status` live-region and the typing-indicator `aria-label` — verify the branch reuses both (no new DOM, no new region). Contrast/focus unaffected (no new element). + +## Testing & Quality +- Unit: the JS pin (Work 3). +- Coverage: **>90%** on `app/` (no `app/` change this task — the gate holds at its current level). + +## Completion Criteria +- [ ] The branch-order + copy + no-DOM pins are green. +- [ ] `uv run ruff check . && uv run pyright` clean (no JS check — house style: JS is E2E-gated in task 05). +- [ ] no behavior change for existing frame types (the branch is purely additive). diff --git a/.agent/phases/todo/67_llm_retry/05_e2e_and_commit.md b/.agent/phases/todo/67_llm_retry/05_e2e_and_commit.md new file mode 100644 index 0000000..801b555 --- /dev/null +++ b/.agent/phases/todo/67_llm_retry/05_e2e_and_commit.md @@ -0,0 +1,38 @@ +# Task 05 — Failure-Injection Mock, E2E Suite, Regressions, Commit + +**Phase:** `67_llm_retry` · **Source:** `TODO.md:3` — the full item (end-to-end proof: `.env` retry + the live UI feedback). +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) + +## Objective +Prove the whole loop in a browser: a dead-then-recovered LLM endpoint shows the "retrying" status mid-turn and the answer still completes; a dead endpoint exhausts the retries and lands on the existing error banner. One dedicated Playwright suite, green in isolation, plus the regression pass and the phase commit. + +## Work +1. `tests/e2e/mock_llm.py` — deterministic failure injection (module-level counters, reset per trigger phrase — the mock is single-conversation per e2e server): + - `RETRY_TRIGGER = "fail then answer"` — user message containing it: the first **2** streaming `chat/completions` requests respond `500` (JSON body, like a dead proxy); the 3rd streams the normal composed answer. (2 = 1 original attempt + 1 retry under the default `BOR_LLM_RETRIES=3`, so the e2e exercises a real retry without waiting for exhaustion.) + - `ALWAYS_FAIL_TRIGGER = "always fail"` — user message containing it: every streaming `chat/completions` request responds `500` (exhaustion path). + - `EMBED_FAIL_TRIGGER = "embed fail once"` — the first `embeddings` request responds `500`; the next returns the normal bag-of-words vector (covers the embedding retry loop in task 02). + - Document all three in the module docstring's marker list (house convention). +2. `tests/e2e/conftest.py` (or the `app_server` env block that sets `BOR_LLM_BASE_URL`) — add `BOR_LLM_RETRY_DELAY=0` to the e2e server env so retry waits are instant (the e2e pins the MECHANISM; the 5 s default is unit-pinned via config). `BOR_LLM_RETRIES` stays at its default (3) — the exhaustion test relies on the real default. +3. `tests/e2e/test_llm_retry.py` — the dedicated suite (reuse the `#send-status` value-recording `add_init_script` pattern from `tests/e2e/test_agent_document_tools.py` L236+): + - **dead-then-recovered (deflected path):** import one fixture doc; ask an unrelated question containing `fail then answer` (LOW turn → deflected stream retry): the recorded status values contain `Communication interrupted — retrying (2 of 4)…`; the turn settles with a deflected answer bubble, NO error banner. + - **dead-then-recovered (grounded path):** ask a KB question containing `fail then answer` (HIGH turn → agent round retries): same status assertion; the answer completes (sources present). + - **embedding retry:** an `embed fail once` question completes with a normal answer and a `retry` status recorded (the embedding loop is visible to the UI). + - **exhaustion:** an `always fail` question → the error banner (`role="alert"`) appears with the existing copy ("The chat model dropped the connection — try again?"); the last recorded status is the highest attempt (`… retrying (4 of 4)…`); the send button is re-enabled (the banner path settles the state machine). + - **zero-retry kill switch is unit-pinned only** (no e2e server variant needed). +4. Regression pass (each in isolation, DB up): `uv run pytest tests/e2e/test_chat_rag.py -v --no-cov`, `test_agent_document_tools.py`, `test_stop_generation.py`, `test_retry_answer.py` — all green (the turn state machine, tool flow, stop, and redo must be untouched). +5. Full gate: `uv run pytest --cov=app --cov-report=term-missing` (TOTAL >90%), `uv run ruff check . && uv run pyright`. +6. Commit (AGENTS.md rule 8 — one atomic phase commit; stage the phase's code + its dir move): + ```bash + git add -A .agent/ app/ tests/ frontend/ && git commit --no-gpg-sign -m "feat(rag): retry a failed LLM request before the first token lands — BOR_LLM_RETRIES/BOR_LLM_RETRY_DELAY with a live 'retrying' status" + ``` + Then move the phase dir to `.agent/phases/complete/67_llm_retry/` and include the move in the SAME commit (house convention: commit first, move + amend the tree, one atomic commit — follow exactly what phase 66's commit did). + +## Testing & Quality +- E2E: as listed (Work 3–4). +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] `uv run pytest tests/e2e/test_llm_retry.py -v --no-cov` green in isolation (DB up). +- [ ] The four regression suites green in isolation. +- [ ] `uv run pytest --cov=app` TOTAL >90%; `uv run ruff check . && uv run pyright` clean. +- [ ] One `--no-gpg-sign` Conventional Commits commit; phase dir in `.agent/phases/complete/`. diff --git a/.agent/phases/todo/68_search_tool/00_phase.md b/.agent/phases/todo/68_search_tool/00_phase.md new file mode 100644 index 0000000..1735d19 --- /dev/null +++ b/.agent/phases/todo/68_search_tool/00_phase.md @@ -0,0 +1,46 @@ +# Phase 68 — Agent Search Tool: grep the Indexed Documents + +**Source:** `TODO.md` L4 — "Add a search tool that allows the LLM to grep through the uploaded documents for a given string" +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) +**Context:** +- `app/rag/agent.py` — `AGENT_TOOLS` currently defines two OpenAI functions (`list_documents`, `read_document`); `_execute_tool` runs them server-side against the DB with fixed refusal strings (`ALREADY_IN_CONTEXT`, `UNKNOWN_TOOL`, `MISSING_READ_ARGS`); `AgentHolder` counts executed calls (`tool_calls`) and context additions (`read_docs`). +- `documents` table (`app/models.py`) — `content` is the FULL document text (`Text` column), so a grep is a plain in-process scan: no new index, no migration. +- `app/schemas.py` — `ChatToolEvent` (`{type: "tool", name, argument}`): `argument` is `"source/path"` for `read_document`, null otherwise. +- `frontend/assets/app.js` — `runTurn`'s `tool` branch builds the status label (`${brand()} is reading …` / `… is listing documents`) and `appendToolLine` (L796) renders the per-call line (`📄 Reading path` / `🔎 Listing documents`); both special-case by tool name. +- `tests/e2e/mock_llm.py` — the marker-driven deterministic tool flow (`use your tools` → list → read → answer) is the template for a search flow; `tests/e2e/test_agent_document_tools.py` is the pattern for the E2E assertions (`.tool-call` lines, `#send-status` recording). +- Phases 37/45/63 (complete) established the tool infrastructure: native tool-calling, unlimited calls bounded by the round cap, labeled `source:/path:` catalog lines. + +## Objective +A third agent tool, `search_documents`, lets the model grep every indexed document (or one named document) for an exact string and get back `path:line: text` matches — so it can LOCATE content cheaply and then `read_document` the winner, instead of reading whole documents hoping the string is in them. + +## Dependencies +- `67_llm_retry` (todo, preceding — no functional dependency; ordering by number) + +## Tasks +1. `01_search_tool_backend.md` — the tool definition, the `grep_document` helper, and the `_execute_tool` branch. +2. `02_search_tool_api_ui.md` — the SSE `tool` argument mapping and the frontend status/tool-line for the search. +3. `03_e2e_and_commit.md` — the mock search flow, the dedicated E2E suite, regressions, commit. + +## Testing & Quality +- Unit: `tests/unit/test_agent.py` — match semantics (case-insensitivity, 1-based line numbers, the 20-match cap, 200-char line truncation, scoped single-doc search, no-match/missing-arg/unknown-doc refusals, `tool_calls` counting, `read_docs` untouched). +- Integration: `tests/integration/test_agent_tools.py` — the tool appears in `AGENT_TOOLS` with the locked parameter shape; `tests/integration/test_chat_api.py` (or the SSE pin file) — a `search_documents` call streams `argument = pattern`. +- E2E (mandatory, house rule): `tests/e2e/test_search_tool.py`, run in isolation (deterministic mock flow: the model searches, sees the match line, answers from it). +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] `search_documents` is the third entry in `AGENT_TOOLS`; a model call with `pattern` (optionally `source`+`path`) returns grep-style matches or a no-match line. +- [ ] The UI shows `Brain is searching for '…'` in the status line and a `🔎 Searching for ''` tool line, persisted/restored like the other tool lines. +- [ ] `uv run pytest` green; coverage TOTAL >90%; `uv run ruff check . && uv run pyright` clean. +- [ ] `uv run pytest tests/e2e/test_search_tool.py -v --no-cov` green in isolation (DB up). +- [ ] Regression E2E suites green in isolation: `test_agent_document_tools.py`, `test_agent_unlimited_tools.py`. +- [ ] One `--no-gpg-sign` commit; phase dir moved to `.agent/phases/complete/`. + +## Locked decisions +- **Owner-locked (2026-09-01, roadmap confirmation, A5):** the match is a **case-insensitive fixed substring** (no regex — no ReDoS surface, a simple contract for the model); output is grep-style `source/path:LINE: text` lines; **20 matches per call** maximum (global cap across documents, in catalog order), each line truncated to **200 chars**; a search does **not** add the document to the answer context (`read_document` remains the only context-adder — `holder.read_docs` is untouched by a search). +- **Owner-locked (2026-09-01, roadmap confirmation, A6):** the tool name is `search_documents` (alongside `list_documents` / `read_document`). +- Scope: search is offered on grounded (HIGH) turns only, exactly like the existing tools — deflected turns keep `tools=None` (A8 byte-identical deflection path), and `BOR_AGENT_MAX_ROUNDS=0` stays the no-tools kill switch (phase 45). + +## Commit +```bash +git add -A .agent/ app/ tests/ frontend/ && git commit --no-gpg-sign -m "feat(agent): search_documents tool — the model can grep the indexed documents for an exact string" +``` diff --git a/.agent/phases/todo/68_search_tool/01_search_tool_backend.md b/.agent/phases/todo/68_search_tool/01_search_tool_backend.md new file mode 100644 index 0000000..2e20480 --- /dev/null +++ b/.agent/phases/todo/68_search_tool/01_search_tool_backend.md @@ -0,0 +1,45 @@ +# Task 01 — `search_documents`: Tool Definition, Grep Helper, Execution Branch + +**Phase:** `68_search_tool` · **Source:** `TODO.md:4` — "Add a search tool that allows the LLM to grep through the uploaded documents for a given string" +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) + +## Objective +The tool exists end to end server-side: it is in `AGENT_TOOLS` with a model-legible contract, and `_execute_tool` executes it — case-insensitive fixed-substring grep over `documents.content`, grep-style output, hard caps, and the house refusal strings. + +## Work +1. `app/rag/agent.py` — module constants (next to the caps/refusals): + - `SEARCH_MAX_MATCHES = 20` — global per-call cap, catalog order (owner-locked A5). + - `SEARCH_LINE_LIMIT = 200` — per-line output truncation (owner-locked A5). + - `MISSING_SEARCH_ARGS = "search_documents requires a string argument 'pattern'."` + - `NO_MATCHES = "No matches for '{pattern}' in the knowledge base."` / scoped variant `"No matches for '{pattern}' in {source}/{path}."` +2. `app/rag/agent.py` — `grep_document(content: str, pattern: str) -> list[tuple[int, str]]` (module-level so unit tests can use/monkeypatch it, house pattern of `list_catalog`/`find_document`): + - split `content` on `\n`; a line matches when `pattern.lower() in line.lower()` (case-insensitive fixed substring — owner-locked A5); return `(1-based line number, line.rstrip())` pairs. +3. `app/rag/agent.py` — `AGENT_TOOLS`: append the third function definition: + - `name`: `search_documents` (owner-locked A6). + - `description`: "Search every indexed document for an exact string (case-insensitive) and return up to 20 matching lines as 'source/path:line: text' — use this to locate content, then read_document the winner. Optionally pass 'source' and 'path' (as shown in list_documents) to search one document only." + - `parameters`: `pattern` (string, **required** — "The exact text to search for (a plain substring, not a regex)"); `source` + `path` (strings, optional — the same "as shown after 'source: '/'path: ' in the list_documents output" wording `read_document` uses, phase 63 labeled fields). +4. `app/rag/agent.py` — `_execute_tool` branch (`call.name == "search_documents"`, placed after the `read_document` branch, before the `UNKNOWN_TOOL` fallback): + - `pattern = call.arguments.get("pattern")`; must be a non-empty string after `.strip()` → else `MISSING_SEARCH_ARGS`. + - if BOTH `source` and `path` are non-empty after strip: `find_document(db, source, path)` → `None` → `"No document at {source}/{path} — check the list_documents output."` (the existing read_document refusal style); search only that document (scoped no-match message). + - if only ONE of `source`/`path` is given → treat it as a missing pair: `MISSING_SEARCH_ARGS` (a half-specified target is a model error, not a whole-KB search — fail loud, house style). + - else: iterate `list_catalog(db)` in `(source, path)` order, load each `Document.content` via `find_document` (or one bulk `select(Document)` ordered by source,path — executor's call, note which in the commit body), accumulating `f"{doc.source}/{doc.path}:{lineno}: {line[:SEARCH_LINE_LIMIT]}"` until `SEARCH_MAX_MATCHES` total; stop scanning once the cap is hit. + - no matches → the no-match line (pattern quoted; a pattern longer than 100 chars is truncated in the message to keep it short). + - success: `holder.tool_calls += 1` (an executed call, re-searches included — same counting as `list_documents`); `holder.read_docs` is **not** touched (locked A5 — the search never adds context). + - log line: the existing `logger.info("agent tool=%s args=%s round=%d/%d", ...)` already covers it (the `arguments` dump includes `pattern`). +5. `app/rag/agent.py` — update the module docstring: three tools now (list/read/search); a search is a locator, not a context-adder. +6. Unit tests — `tests/unit/test_agent.py`: + - case-insensitive match across multiple lines, 1-based line numbers, multi-line and repeated matches. + - the 20-match global cap across two documents (catalog order); line truncation at 200 chars (a 300-char line yields 200 + no crash). + - scoped search: found doc, missing doc (refusal), single-arg (only `source`) → `MISSING_SEARCH_ARGS`. + - no-match (whole KB and scoped) messages; empty/whitespace `pattern` → `MISSING_SEARCH_ARGS`; non-string `pattern` → `MISSING_SEARCH_ARGS`. + - `holder.tool_calls` counts a search; `holder.read_docs` unchanged after a search. + - `AGENT_TOOLS` shape: three tools, `search_documents` has `required: ["pattern"]` (and optional `source`/`path`). + +## Testing & Quality +- Unit/integration: Work 6 + `tests/integration/test_agent_tools.py` (the new tool is offered and executed through `run_agent` with a scripted `ToolCallPiece`). +- Coverage: **>90%** on this task's new/modified code. + +## Completion Criteria +- [ ] A scripted `search_documents` call through `run_agent` returns the grep-style result text and bumps `tool_calls` without touching `read_docs`. +- [ ] `uv run pytest tests/unit/test_agent.py tests/integration/test_agent_tools.py -v` green; `uv run ruff check . && uv run pyright` clean. +- [ ] no behavior change for `list_documents` / `read_document` (existing pins green). diff --git a/.agent/phases/todo/68_search_tool/02_search_tool_api_ui.md b/.agent/phases/todo/68_search_tool/02_search_tool_api_ui.md new file mode 100644 index 0000000..48b396a --- /dev/null +++ b/.agent/phases/todo/68_search_tool/02_search_tool_api_ui.md @@ -0,0 +1,46 @@ +# Task 02 — SSE Argument Mapping + the Frontend Search Status/Tool Line + +**Phase:** `68_search_tool` · **Source:** `TODO.md:4` — "Add a search tool that allows the LLM to grep through the uploaded documents for a given string" +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) + +## Objective +The search call is visible in the UI like the other two tools: the SSE `tool` frame carries the pattern as its `argument`, the status line reads `Brain is searching for 'pattern'`, and a `🔎 Searching for ''` line lands above the answer — persisted and restored with the conversation like the existing tool lines. + +## Work +1. `app/api/chat.py` — the `ToolCallPiece` branch currently computes `argument` as `f"{source}/{path}"` for `read_document`, else `None`: + - extend: `elif piece.name == "search_documents":` → `argument = piece.arguments.get("pattern")` (the raw string; a non-string pattern — a model error the backend refuses — yields `None`). +2. `app/schemas.py` — `ChatToolEvent`: update the docstring + field comments — `name` is `"list_documents" | "read_document" | "search_documents"`; `argument` is `"source/path"` for `read_document`, the **search pattern** for `search_documents`, null otherwise. (No field-shape change.) +3. `frontend/assets/app.js` — `runTurn`'s `tool` branch, the `toolStatus` computation: + ```js + const toolStatus = + name === "read_document" && argument + ? `${brand()} is reading ${argument}` + : name === "search_documents" && argument + ? `${brand()} is searching for ${argument}` + : `${brand()} is listing documents`; + ``` +4. `frontend/assets/app.js` — `appendToolLine` (L796): add the search branch BEFORE the `else` fallback: + ```js + } else if (name === "search_documents" && argument) { + line.textContent = "🔎 Searching for "; + const code = document.createElement("code"); + code.textContent = argument; // the pattern is data, never markup + line.appendChild(code); + } + ``` + (The `else` keeps `"🔎 Listing documents"` for `list_documents` and any unknown name.) The pattern goes in a `` element exactly like the read path — data, never markup (the existing XSS-safe convention). +5. Persistence/restore: the `toolAcc` record is already `{name, argument}`-generic and the restore path calls the same `appendToolLine(t.name, arg)` (L1205) — no extra work; verify the restore branch renders the search line (covered by the E2E in task 03 only if a reload happens in that suite — otherwise by the unit pin below). +6. JS unit pins — `tests/unit/test_frontend_tool_states.py` (the phase-37 frontend contract file): + - the status ternary contains the locked `is searching for` branch with the correct name/argument gate; + - `appendToolLine` contains the `search_documents` branch with the `` element (pattern-as-data pin); + - the persisted tool record still serializes `{name, argument}` generically (no per-tool shape). +7. Integration pin — `tests/integration/test_chat_api.py` (or the SSE pin module): a scripted `search_documents` `ToolCallPiece` streams as `{type: "tool", name: "search_documents", argument: ""}`. + +## Testing & Quality +- Unit: Work 6 (JS pins) + Work 7 (SSE shape). +- Coverage: **>90%** on this task's new/modified code. + +## Completion Criteria +- [ ] A `search_documents` tool frame streams with `argument = pattern` and renders the locked status + tool line in the browser (E2E in task 03). +- [ ] `uv run pytest tests/unit/test_frontend_tool_states.py tests/integration/test_chat_api.py -v` green; `uv run ruff check . && uv run pyright` clean. +- [ ] no behavior change for the existing two tool frames (their SSE shapes + UI lines are untouched). diff --git a/.agent/phases/todo/68_search_tool/03_e2e_and_commit.md b/.agent/phases/todo/68_search_tool/03_e2e_and_commit.md new file mode 100644 index 0000000..7480bdd --- /dev/null +++ b/.agent/phases/todo/68_search_tool/03_e2e_and_commit.md @@ -0,0 +1,39 @@ +# Task 03 — Mock Search Flow, E2E Suite, Regressions, Commit + +**Phase:** `68_search_tool` · **Source:** `TODO.md:4` — the full item (end-to-end proof: the model greps, sees the match, answers from it). +**Story:** n/a (TODO-derived — owner roadmap confirmation 2026-09-01) + +## Objective +Prove the tool in a browser with the deterministic mock: a grounded question makes the mock model call `search_documents`, the match line reaches the model, and the answer quotes the found content — with the search visible in the status line and tool lines. One dedicated Playwright suite, green in isolation, plus the regression pass and the phase commit. + +## Work +1. `tests/e2e/mock_llm.py` — a new marker flow, following the `use your tools` flow's structure (stateless discrimination from the messages, streaming only): + - `SEARCH_TRIGGER = "search your documents"` (checked BEFORE the plain `use your tools` check — it is more specific, same convention as `think in paragraphs`): + - request 1 (`tools` offered, no tool results yet): stream ONLY a `tool_calls` delta — `search_documents` with `{"pattern": ""}` (id `call_0`); `` is a sentinel string the e2e places in a fixture document (e.g. `reese-sentinel-42` — the sentinel convention from the `show the end of your notes` marker). + - request 2 (a `tool`-role search result in the messages — recognizable as a search result by its `source/path:line: text` shape or the sentinel in its content): the content answer, deterministic: `Found ` — so the suite can assert the search result reached the model and landed in the answer. + - document the flow in the module docstring's marker list. +2. `tests/e2e/test_search_tool.py` — the dedicated suite (DB up; import one fixture document containing the sentinel line, via the existing admin import fixtures in `conftest.py`/`auth_helpers.py`): + - **live search flow:** ask a KB question containing `search your documents` → + - a `.msg.brain .tool-call` line appears containing `🔎 Searching for` and the sentinel in a `` (assert via `to_contain_text("Searching for")` + the code element text); + - the recorded `#send-status` values contain `is searching for ` (the init-script status-recording pattern from `test_agent_document_tools.py`); + - the answer bubble contains the deterministic `Found …` echo (the match reached the model); + - NO error banner; the turn settles to idle with the send button re-enabled. + - **context accounting:** the search does not add a source by itself — if the mock flow searches and then answers WITHOUT a read, `done.sources` reflects only the retrieval docs (assert the sources row is unchanged by the search alone). (If the executor finds the mock flow must also read to produce a stable answer, keep the flow search-only and assert the sources row equals the retrieval baseline.) + - **regression-safe markers:** the existing `use your tools` questions in `test_agent_document_tools.py` / `test_agent_unlimited_tools.py` do NOT contain the new trigger (verify — the trigger phrase must not appear in any other suite's fixture text). +3. Regression pass (each in isolation, DB up): `uv run pytest tests/e2e/test_agent_document_tools.py -v --no-cov`, `uv run pytest tests/e2e/test_agent_unlimited_tools.py -v --no-cov`, `uv run pytest tests/e2e/test_chat_rag.py -v --no-cov` — the list/read flow, the unlimited-calls behavior, and the plain RAG turn must be untouched (the third tool changes the `tools` payload — confirm no existing suite pins an exact two-tool payload; if one does, update it to expect three and note it in the commit body). +4. Full gate: `uv run pytest --cov=app --cov-report=term-missing` (TOTAL >90%), `uv run ruff check . && uv run pyright`. +5. Commit (AGENTS.md rule 8 — one atomic phase commit): + ```bash + git add -A .agent/ app/ tests/ frontend/ && git commit --no-gpg-sign -m "feat(agent): search_documents tool — the model can grep the indexed documents for an exact string" + ``` + Then move the phase dir to `.agent/phases/complete/68_search_tool/` and include the move in the SAME commit (house convention — mirror phase 67's commit/move pattern). + +## Testing & Quality +- E2E: as listed (Work 2–3). +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] `uv run pytest tests/e2e/test_search_tool.py -v --no-cov` green in isolation (DB up). +- [ ] The three regression suites green in isolation. +- [ ] `uv run pytest --cov=app` TOTAL >90%; `uv run ruff check . && uv run pyright` clean. +- [ ] One `--no-gpg-sign` Conventional Commits commit; phase dir in `.agent/phases/complete/`.