phase: 96_oneshot_resilience
All checks complete. Final report: **Phase 96 (one-shot resilience) — final verification pass, all green** (all 4 task files already in `complete/`; verified the working-tree implementation against the design) - `LLMClient.chat()` empty-content retry (D1–D3) via `_chat_once` + `_EmptyContentError` (carries `finish_reason`), under `BOR_LLM_RETRIES`/`BOR_LLM_RETRY_DELAY` — verified in diff - `missing_folder_summaries()` + `generate_folder_summaries(only_missing=…)` — verified; `folder_summary_table_empty` deleted, both sync gates switched to the gap probe - `.env.example` comments updated (chat-turn stream + one-shot summary calls) **Test / lint / coverage results** - `uv run pytest --cov=app --cov-report=term-missing` → **1988 passed**, coverage **99%** (gate >90%) - `uv run pytest tests/e2e/test_oneshot_llm_retry.py -v --no-cov` → **2 passed** (isolated) - Regressions, each isolated: `test_ls_tree_drilldown` 3 passed, `test_sync_button` 3 passed, `test_local_directory_sources` 3 passed, `test_llm_retry` 4 passed - `uv run ruff check . && uv run pyright` → clean (0 errors) **Completion criteria:** retry-then-recover unit-pinned ✓ · exhaustion + `BOR_LLM_RETRIES=0` byte-identical ✓ · streaming path untouched ✓ · gap-fill both sync paths, other rows byte-identical incl. `updated_at` ✓ · no-gap zero-burn ✓ · phase E2E green ✓ · regression E2Es green ✓ · full suite + >90% + lint/types ✓ · no completed-phase behavior change (full suite green) ✓. Commit left to the harness per executor rules (working tree, 16 files). **Deviations:** none. **Next pending phase:** `97_kb_tree_catalog`.
This commit is contained in:
@@ -0,0 +1,90 @@
|
||||
# Phase 96 — One-shot LLM resilience: retry empty replies, self-heal missing folder summaries
|
||||
|
||||
**Source:** 2026-09-11 incident (owner chat) — the then-`lite` model intermittently returned empty completions: its internal monologue (the wire's `reasoning_content` field) consumed the entire `max_tokens=2048` budget, so the reply arrived as `content=""` + `finish_reason="length"`. Measured 2 empties in 90 folder-summary calls on the `deploy` source sync (≈2–7% per call in follow-up probes), losing two stored rows: `deploy/Deployments` (283 docs) and `deploy/Deployments/stackexpected/website/app` (7 docs). The app handled it exactly as designed (`LLMClient.chat` refuses to store a silent summary; `generate_folder_summaries` is per-folder fail-soft) — but nothing retried, and nothing ever filled the hole: the folder-summary gate (`changed KB or empty table`) skips regeneration on unchanged re-syncs, so missing rows persist until a KB change. The owner replaced the `lite` model server-side (it no longer emits `reasoning_content` — verified live: 16/16 clean, no `reasoning_content` field); this phase removes the remaining app-side exposure so ANY future transient empty reply from ANY one-shot consumer cannot silently lose data.
|
||||
**Story:** n/a (incident-driven resilience — one Playwright file per phase, run in isolation, A16).
|
||||
**Context:**
|
||||
- `app/rag/llm.py` — `LLMClient.chat()` (~L335–378) is the one-shot surface: transport failure → `LLMError` (wrapped), choiceless reply → `LLMError`, `content` None/blank → `LLMError("…returned empty content — refusing to store a silent summary")`. No retry. Phase 67's retry machinery (`BOR_LLM_RETRIES`=3 / `BOR_LLM_RETRY_DELAY`=5 s, flat; `chat_stream_retried`, `RetryPiece`) covers the **streaming** chat-turn path only — phase 67's owner-locked A1 explicitly scoped it to the chat turn and left `chat()` out ("admin/import paths with their own fail-fast behavior").
|
||||
- `chat()` consumers (all fail-soft per call, all protected once `chat()` retries): `app/rag/summarizer.py` L113 (document summaries — failure leaves `documents.summary` NULL + a `summary_errors` count), `app/rag/overview.py` L182 (KB overview — failure keeps the old row; its `_overview_row_exists` gate already self-heals a missing row on the next unchanged walk), `app/rag/folder_summaries.py` L267 via `summarize_folder` → `generate_folder_summaries` (per-folder fail-soft; **no self-heal** — the gap this phase fills), `app/rag/llm.py` L641 `check_models` (the sync-time "ping" probe).
|
||||
- `app/config.py` — `llm_retries: int = 3`, `llm_retry_delay: float = 5` (phase 67; validators already enforce the house kill-switch pattern: 0 disables, negative fails startup).
|
||||
- The openai SDK's own `max_retries=2` (kept by `LLMClient`) already re-POSTs **wire-level** 500s — the gap is the **semantic** failure: the endpoint is healthy, answers, and says nothing.
|
||||
- `scripts/import_docs.py` `_run()` (~L335–360) and `app/api/sync.py` `_run_sync()` (L276–281) — the folder-summary gate, one copy per path: `changed = summary.added + summary.updated > 0`; script path `folders_due = changed or _folder_summaries_table_empty()` (`--limit` skips entirely); API path `if changed or folder_summary_table_empty(fs_db)`. `folder_summary_table_empty` (`app/rag/folder_summaries.py`) is the bounded `LIMIT 1` probe.
|
||||
- `app/rag/folder_summaries.py` — `group_by_folder` (the one recursive-subtree concept), `MIN_DOCS_PER_FOLDER = 2`, `generate_folder_summaries` (sorted candidate iteration, per-folder fail-soft upsert, then a prune pass deleting rows whose folder lost ≥ 2 docs; flushes only — the sync path owns the transaction).
|
||||
- `tests/unit/test_llm_client.py` — the fake-client harness (`_FakeCompletion`, `_make_chat_client`, `SimpleNamespace` completions); existing pins `test_chat_missing_content_raises_llm_error` / `test_chat_whitespace_only_content_raises_llm_error` (~L989–1000) match `"empty content"`.
|
||||
- `tests/unit/test_folder_summaries.py` — the duck-typed fake LLM with a `.calls` counter (the generator's unit pattern).
|
||||
- `tests/integration/test_sync_folder_summaries.py` + `tests/integration/test_import_docs_overview.py` — the change-gate integration pattern (fake `LLMClient` keyed on the mode marker; zero-call assertions on unchanged re-runs).
|
||||
- `tests/e2e/mock_llm.py` — `compose_answer`'s `FOLDER_SUMMARY_MODE` branch (L1504) returns the deterministic one-liner `Fixture folder summary for <folder>.` from the user message's `Folder: …` header (imported `FOLDER_HEADER_PREFIX` — the mock can never drift from it); phase 67's failure-injection machinery: module-level `_fail_posts` counters + `_bump_fail`, keyed by trigger phrase, **reset after the success they guard**.
|
||||
- `tests/e2e/test_ls_tree_drilldown.py` (phase 94) — the scripted `ls` drill-down turn pattern (the mock echoes the tool result into its grounded answer — the E2E's only lens on the LLM context) and the temp-local-source seed + `POST /api/sync` pattern; `tests/e2e/test_llm_retry.py` (phase 67) — the test-server env-override pattern (`BOR_LLM_RETRY_DELAY=0`).
|
||||
|
||||
## Objective
|
||||
A transient empty one-shot LLM reply can no longer silently lose data: `LLMClient.chat()` retries an empty-content reply under the existing `BOR_LLM_RETRIES`/`BOR_LLM_RETRY_DELAY` policy (logged per attempt) before raising, and an **unchanged**-KB sync detects folder-summary gaps (candidate folders with no stored row) and fills **only** the missing rows — so a failed folder summary self-heals on the next sync instead of persisting until a KB change.
|
||||
|
||||
## Dependencies
|
||||
- `67_llm_retry` (complete) — the retry policy knobs (`BOR_LLM_RETRIES`/`BOR_LLM_RETRY_DELAY`), the logging/kill-switch conventions, and the mock's failure-injection pattern.
|
||||
- `94_ls_tree_drilldown` (complete) — `folder_summaries`, `generate_folder_summaries`, the change-gate, the E2E drill-down + sync patterns.
|
||||
|
||||
## Decisions recorded here (no PLAN.md change needed — no new technology, no new env vars, no new SSE events)
|
||||
- **D1 — knob reuse:** the one-shot path honors the **existing** `BOR_LLM_RETRIES` (default 3) / `BOR_LLM_RETRY_DELAY` (default 5 s) — no new settings. Phase 67's owner-locked A3 named these the house LLM-retry policy; extending them to `chat()` is a deliberate extension of that policy, not a deviation (the lock scoped *what phase 67 shipped*, it does not prohibit `chat()` from honoring the same knobs). `.env.example` comments are updated to say so. (If the owner prefers a separate one-shot knob, task 01 is a one-line change.)
|
||||
- **D2 — retry condition:** `chat()` retries **empty content only** (`content` None or whitespace — the exact incident failure class). A choiceless reply still raises immediately (no retry), and transport failures keep today's behavior (the SDK's `max_retries=2` already covers wire-level 500s; an app-level transport retry would double-stack the SDK policy and multiply the 300 s timeout).
|
||||
- **D3 — exhaustion contract:** after `1 + llm_retries` empty attempts, `chat()` raises the same `LLMError` type with an updated message naming the attempts (`…returned empty content on all {N} attempts — refusing to store a silent summary`); with `llm_retries=0` the message stays **byte-identical** to today's (the feature is fully off — phase 67 kill-switch convention). All consumers' fail-soft behavior is untouched (document summary → NULL + count, overview → old row, folder → skip + old row, probe → sync fails loudly as today).
|
||||
- **D4 — targeted gap-fill:** `generate_folder_summaries` gains `only_missing=False`. The changed-KB path is **unchanged** (full regeneration + prune). The unchanged-KB gate changes from "table empty?" to "any candidate row missing?" (subsumes the table-empty first-run case exactly); when a gap exists it runs `only_missing=True` — only the missing candidates are generated, existing rows stay byte-identical (text AND `updated_at`), and the prune pass still runs (a no-op on an unchanged KB, the invariant kept). `--limit` (script only) still skips generation entirely.
|
||||
- **Non-goals:** the streaming path (`chat_stream` / `chat_stream_retried`) is untouched (phase 67 territory); no `documents.summary`-NULL catch-up (the retry eliminates the failure class — expected per-doc loss p⁴; the importer's unchanged-skip would be a separate, larger change); no KB-overview catch-up (its `_overview_row_exists` gate already self-heals); no UI/SSE/sync-status surface change (folder stats stay log-only, phase 94 contract); no aipi/model-side change (owner already done).
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
|
||||
### The one-shot retry (task 01)
|
||||
`app/rag/llm.py` `LLMClient`:
|
||||
- Extract the single-attempt body of `chat()` into a private `_chat_once(messages, model) -> str` (the existing try/except transport wrap + choiceless check + empty-content check, verbatim behavior). `chat()` becomes: attempt 1 via `_chat_once`; on the **empty-content** `LLMError` only, while attempts remain: `logger.warning` (PLAN §9: model, `finish_reason` of the empty reply when available, `attempt n of N`) + `await asyncio.sleep(settings.llm_retry_delay)` (flat, phase 67) + retry via `_chat_once`. Any other `LLMError` propagates immediately.
|
||||
- `N = 1 + settings.llm_retries` total attempts. Exhaustion → the D3 message. `llm_retries=0` → one attempt, the byte-identical legacy message.
|
||||
- The empty-content check must capture the reply's `finish_reason` for the log line (the incident's signature was `finish_reason="length"` — it makes the warning greppable and self-explaining).
|
||||
- `chat()` docstring: the phase-30 "a silent empty summary must never be stored" contract stays, extended with the retry policy (D1/D2/D3). `.env.example`: the two phase-67 comments gain "(chat-turn stream and one-shot summary calls)".
|
||||
- `check_models` benefits automatically (no change).
|
||||
|
||||
### The gap detector + targeted fill (task 02)
|
||||
`app/rag/folder_summaries.py`:
|
||||
- `missing_folder_summaries(db) -> list[tuple[str, str]]` — the candidate folders (the generator's exact candidate computation: one catalog query ordered by `(source, path)`, `group_by_folder`, subtrees with ≥ `MIN_DOCS_PER_FOLDER`) that have **no stored row**, sorted by `(source, folder_path)`. The gate probe and the fill both use this one function — one concept, as with `group_by_folder`.
|
||||
- `generate_folder_summaries(db, llm, *, skip=False, only_missing=False)` — `only_missing=True` restricts the sorted candidate iteration to the keys from `missing_folder_summaries(db)` (computed from the SAME catalog pass — no second full query inside the generator: derive the missing set from the candidates already grouped). Upsert, fail-soft, and the prune pass are otherwise unchanged; stats dict shape unchanged (the caller logs the mode).
|
||||
- `folder_summary_table_empty` is deleted in task 03 (both call sites replaced — the function becomes dead code; its unit tests move to `missing_folder_summaries`).
|
||||
|
||||
### The sync gates (task 03)
|
||||
Both paths, identical shape (the phase-94 "one gate, two call sites" convention):
|
||||
- Changed KB → `generate_folder_summaries(session, llm)` — full regeneration (today's behavior, byte-identical).
|
||||
- Unchanged KB → `missing = missing_folder_summaries(session)`; non-empty → `generate_folder_summaries(session, llm, only_missing=True)` + a log line naming the gap-fill (script: the summary line token becomes `folder_summaries=<g>/<f>/<p> (gap-fill)`; API: `sync: folder_summaries gap-fill stats=…`); empty → today's skip log.
|
||||
- `--limit` (script) → unchanged full skip. Transaction convention unchanged (generator flushes, the path commits).
|
||||
|
||||
### The E2E (task 04)
|
||||
`tests/e2e/mock_llm.py` — the `FOLDER_SUMMARY_MODE` branch gains the incident-shape injection, keyed by the folder LABEL (the seeded KB's folder names carry the trigger — deterministic, no env, no request-order state beyond the phase-67 per-key counter, reset after the success it guards):
|
||||
- label ends with `/e2e_empty_once` → the FIRST non-stream POST for that label returns the incident envelope: same OpenAI shape, `message.content=""`, `finish_reason="length"`; later POSTs return the normal `Fixture folder summary for <folder>.` line.
|
||||
- label ends with `/e2e_empty_always` → every non-stream POST for that label returns the empty envelope.
|
||||
`tests/e2e/test_oneshot_llm_retry.py` (new; `app_server` + `mock_llm` conftest fixtures; admin login via `tests/e2e/auth_helpers.py`; sync via `POST /api/sync` per `test_sync_button.py`; the test server boots with `BOR_LLM_RETRY_DELAY=0` per the phase-67 pattern; default `BOR_LLM_RETRIES=3`):
|
||||
1. Seed a temp local-dir source with three ≥ 2-doc folders: `e2e_empty_once/`, `e2e_empty_always/`, `normal/`.
|
||||
2. Sync #1 → a scripted `ls <source>` drill-down turn (the phase-94 echo pattern) asserts: the `e2e_empty_once/` line carries `: Fixture folder summary for <src>/e2e_empty_once.` (the retry recovered the row — without task 01 it would be absent), the `normal/` line does too, and the `e2e_empty_always/` line has NO `: …` suffix (exhausted → fail-soft → absent) while the sync still reports success (the folder-stats failure never flips the run, phase 94).
|
||||
3. Delete the `normal` stored row directly (simulating a historical failure) → Sync #2 (KB unchanged) → the `normal` row is back with its deterministic text; the `e2e_empty_once` row's `updated_at` is **unchanged** (targeted fill — no full regeneration); `e2e_empty_always` remains absent (the gap-fill attempts it, exhausts, stays absent, sync still succeeds).
|
||||
|
||||
## Tasks
|
||||
1. `01_chat_retry.md` — `chat()` empty-content retry under `BOR_LLM_RETRIES`/`BOR_LLM_RETRY_DELAY` + logging + unit pins
|
||||
2. `02_gapfill_generator.md` — `missing_folder_summaries` + `generate_folder_summaries(only_missing=…)` + unit pins
|
||||
3. `03_sync_gates.md` — the gap gate in `scripts/import_docs.py` + `app/api/sync.py` (replacing the table-empty probe) + integration pins
|
||||
4. `04_e2e_oneshot_retry.md` — the mock's incident-shape injection + `tests/e2e/test_oneshot_llm_retry.py` + regressions + commit
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: `tests/unit/test_llm_client.py` (retry matrix: empty-then-success = 2 attempts + 1 sleep; all-empty = `1 + retries` attempts + the exhaustion message; first-success = 1 attempt, 0 sleeps, byte-identical return; choiceless reply = no retry, immediate error; transport error = no app-level retry; `llm_retries=0` = legacy byte-identical message; sleep value honored — record `asyncio.sleep`); `tests/unit/test_folder_summaries.py` (`missing_folder_summaries` shapes: fresh table → all candidates, full table → empty, one row deleted → that folder; `only_missing=True` → only missing keys generated, existing rows byte-identical incl. `updated_at`, zero LLM calls when there is no gap, prune still runs; `only_missing=False` unchanged).
|
||||
- Integration: `tests/integration/test_sync_folder_summaries.py` + the `test_import_docs_overview.py` pattern (both sync paths: unchanged + gap → targeted fill of exactly the missing rows; unchanged + no gap → zero `FOLDER_SUMMARY_MODE` calls; changed → full regeneration as today; `--limit` skips).
|
||||
- E2E (mandatory, A16): `tests/e2e/test_oneshot_llm_retry.py`, in isolation, deterministic mock, `BOR_LLM_RETRY_DELAY=0`.
|
||||
- Coverage: **>90%** on `app/` (`uv run pytest --cov=app --cov-report=term-missing`).
|
||||
- Lint/types: `uv run ruff check . && uv run pyright`.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] a one-shot `chat()` call whose first reply is empty content (None or whitespace) is retried up to `BOR_LLM_RETRIES` times with `BOR_LLM_RETRY_DELAY` between attempts, one `WARNING` per retry (model + finish_reason + attempt count), and succeeds when a later attempt returns content (unit-pinned)
|
||||
- [ ] an all-empty `chat()` raises `LLMError` naming the attempts after exactly `1 + BOR_LLM_RETRIES` attempts; `BOR_LLM_RETRIES=0` reproduces today's single-attempt behavior and message byte-identically
|
||||
- [ ] the streaming path is untouched: `test_llm_retry.py` (phase 67) green in isolation
|
||||
- [ ] an unchanged-KB sync with a missing folder-summary row regenerates exactly that row (both `scripts/import_docs.py` and `POST /api/sync`), leaves every other row byte-identical (incl. `updated_at`), and logs the gap-fill (integration-pinned)
|
||||
- [ ] an unchanged-KB sync with no gap burns zero `lite` folder-summary calls (the phase-94 zero-burn invariant holds)
|
||||
- [ ] `uv run pytest tests/e2e/test_oneshot_llm_retry.py -v --no-cov` green in isolation: the `e2e_empty_once` folder summary is recovered by the retry (visible in the `ls` drill-down), `e2e_empty_always` stays absent with the sync green, and the deleted-row gap self-heals on the next unchanged sync
|
||||
- [ ] regression E2E green in isolation: `test_ls_tree_drilldown.py`, `test_sync_button.py`, `test_local_directory_sources.py`, `test_llm_retry.py`
|
||||
- [ ] `uv run pytest` green, coverage >90%, `uv run ruff check . && uv run pyright` clean
|
||||
- [ ] no behavior change in completed phases; one atomic Conventional Commit, `--no-gpg-sign`
|
||||
|
||||
## Commit
|
||||
```bash
|
||||
git add -A .agents/ app/ scripts/ tests/ && git commit --no-gpg-sign -m "fix(rag): retry empty one-shot LLM replies and self-heal missing folder summaries on unchanged syncs"
|
||||
```
|
||||
@@ -0,0 +1,38 @@
|
||||
# Task 01 — `chat()` retries an empty one-shot reply under the house retry policy
|
||||
|
||||
**Phase:** `96_oneshot_resilience` · **Story:** n/a (incident-driven resilience — phase 67's owner-locked A1 left the one-shot path out of scope; this task closes that gap per owner request 2026-09-11).
|
||||
|
||||
## Objective
|
||||
`LLMClient.chat()` (the one-shot surface used by document summaries, the KB overview, folder summaries, and the sync probe) retries an **empty-content** reply up to `BOR_LLM_RETRIES` times with `BOR_LLM_RETRY_DELAY` between attempts, logs each retry, and only then raises — so a transient "the model answered but said nothing" reply (the 2026-09-11 incident: `content=""`, `finish_reason="length"`, the whole budget spent in `reasoning_content`) no longer loses a summary on its first attempt.
|
||||
|
||||
## Work
|
||||
1. `app/rag/llm.py` — `LLMClient`:
|
||||
- Extract `chat()`'s single-attempt body verbatim into a private `_chat_once(self, messages, model) -> str`: the existing `try/except` transport wrap (→ `LLMError` with the sanitized base URL), the choiceless check (`…returned no choices`), and the empty-content check (`content` None or `not content.strip()`). `_chat_once` must also surface the empty reply's `finish_reason` to its caller for the log line — e.g. raise a small internal signal or return it alongside; keep the public error messages exactly as they are today for the no-retry cases.
|
||||
- Rewrite `chat()` around it: `N = 1 + self.settings.llm_retries` total attempts. Attempt 1 → on the **empty-content** failure only: while attempts remain, `logger.warning(…)` with the model name, the empty reply's `finish_reason`, and `attempt {n} of {N}` (PLAN §9 ample logging — this line is the greppable record of the incident class), then `await asyncio.sleep(self.settings.llm_retry_delay)` (flat delay — the phase-67 convention), then the next attempt. Every other `LLMError` (transport, no-choices) propagates immediately — **no** app-level retry (the openai SDK's own `max_retries=2` already re-POSTs wire-level failures; an app-level transport retry would stack on top of it).
|
||||
- Exhaustion: raise `LLMError` with the updated message `f"…returned empty content on all {N} attempts — refusing to store a silent summary"` (same base-URL sanitization as today). When `llm_retries == 0`, raise the **current** message verbatim (`…returned empty content — refusing to store a silent summary`) — the kill-switch must be byte-identical to pre-phase-96 behavior (house byte-identical convention).
|
||||
- Update `chat()`'s docstring: the phase-30 contract ("a silent empty summary must never be stored") stands; add the retry policy (D1/D2/D3 of `00_phase.md`): which failures retry (empty content only), the knobs, the flat delay, the exhaustion message.
|
||||
- `check_models` (the "ping" probe) needs no change — it benefits automatically.
|
||||
2. `.env.example` — the `BOR_LLM_RETRIES` / `BOR_LLM_RETRY_DELAY` comments gain that they now cover "the chat-turn stream (phase 67) and one-shot summary calls (phase 96)". No new settings, no `app/config.py` change (the validators for both knobs already exist from phase 67).
|
||||
|
||||
- ASSUMPTION: the retry loop lives inside `chat()` itself (one place, every consumer protected) — NOT in each caller; the generators' per-folder fail-soft semantics are untouched (they still catch the post-exhaustion `LLMError` exactly as today).
|
||||
- ASSUMPTION: `finish_reason` availability — the empty reply is already parsed (`resp.choices[0]`); reading `.finish_reason` off it is free. If a provider omits it, log `finish_reason=None` (the line must never crash on the diagnostic path).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit — `tests/unit/test_llm_client.py` (extend the existing `_FakeCompletion`/`_make_chat_client` harness; the fake completions object can be scripted to yield a sequence of replies per `create()` call):
|
||||
- empty-then-success: 2 attempts, exactly 1 sleep of `llm_retry_delay`, returns the second reply's trimmed content; the `WARNING` fired once (caplog).
|
||||
- all-empty with `llm_retries=3` (default): 4 attempts, 3 sleeps, `LLMError` matching `all 4 attempts`; with a custom `llm_retries=1`: 2 attempts, message names 2.
|
||||
- first-attempt success: exactly 1 `create()` call, **zero** sleeps, return value byte-identical to today's behavior (the happy path is untouched).
|
||||
- choiceless reply: 1 attempt, no retry, the existing `no choices` error.
|
||||
- transport failure: 1 attempt, no retry, the existing wrapped error.
|
||||
- `llm_retries=0`: empty reply → 1 attempt, zero sleeps, the **legacy** message byte-identical (assert the exact string).
|
||||
- sleep value: record `asyncio.sleep` calls (monkeypatch) — flat `llm_retry_delay` each time, never a growing backoff.
|
||||
- update the two existing pins (`test_chat_missing_content_raises_llm_error`, `test_chat_whitespace_only_content_raises_llm_error`, ~L989–1000) to the new contract — either pin the exhaustion path with a low-retry setting or set `llm_retries=0` there to keep the legacy-message assertion; both `None` and whitespace-only content must still refuse.
|
||||
- Coverage: **>90%** on the modified `app/rag/llm.py` lines (`uv run pytest --cov=app --cov-report=term-missing`).
|
||||
- No integration/E2E in this task (tasks 03/04 cover the paths end to end).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_llm_client.py -v` green with the new pins above
|
||||
- [ ] full suite green, coverage >90%, `uv run ruff check . && uv run pyright` clean
|
||||
- [ ] the streaming path (`chat_stream`, `chat_stream_retried`) is byte-identical — no diff outside `chat()`/`_chat_once` in `app/rag/llm.py`
|
||||
- [ ] `.env.example` comments updated; no new settings
|
||||
- [ ] no behavior change in completed work (phase 67's retry suite green)
|
||||
@@ -0,0 +1,36 @@
|
||||
# Task 02 — Gap detector + targeted fill in the folder-summary generator
|
||||
|
||||
**Phase:** `96_oneshot_resilience` · **Story:** n/a.
|
||||
|
||||
## Objective
|
||||
Give `app/rag/folder_summaries.py` the two primitives the sync gate (task 03) and the self-heal need: `missing_folder_summaries(db)` — the candidate folders whose stored row is absent (one concept, the generator's exact candidate computation) — and `generate_folder_summaries(…, only_missing=False)`, which restricts the upsert pass to those missing keys so an unchanged-KB gap-fill never re-burns `lite` for a folder that already has a good summary.
|
||||
|
||||
## Work
|
||||
1. `app/rag/folder_summaries.py`:
|
||||
- `missing_folder_summaries(db) -> list[tuple[str, str]]`:
|
||||
- Run the generator's catalog query verbatim (`select(Document.source, Document.path, Document.title, Document.summary).order_by(Document.source, Document.path)`), group via `group_by_folder`, keep the candidates (subtree ≥ `MIN_DOCS_PER_FOLDER`) — the SAME computation `generate_folder_summaries` does, so the gap can never disagree with what a regeneration would cover.
|
||||
- Subtract the stored keys: one `select(FolderSummary.source, FolderSummary.folder_path)` (bounded, no count scan — the `folder_summary_table_empty` house pattern).
|
||||
- Return `sorted(missing)` by `(source, folder_path)`. Empty list when nothing is missing (including an empty table over an empty KB — no candidates, no gap).
|
||||
- `generate_folder_summaries(db, llm, *, skip=False, only_missing=False)`:
|
||||
- When `only_missing=True`, after computing `groups`/`candidates` (the existing pass), intersect the iteration keys with `missing_folder_summaries(db)` computed from the SAME grouped rows (do not re-run the catalog query — derive the missing set from the candidates minus the one stored-key select). Upsert loop, per-folder fail-soft, and the prune pass are otherwise **unchanged**; the stats dict keeps its `{"generated", "failed", "pruned"}` shape (the caller logs the mode, not the generator).
|
||||
- When `only_missing=False` (default) the function is byte-identical in behavior to today.
|
||||
- Update the docstring: the new parameter, the "existing rows stay byte-identical (text AND `updated_at`) under `only_missing`" contract, and that the prune pass still runs in both modes (a no-op on an unchanged KB, the invariant kept).
|
||||
- Do NOT delete `folder_summary_table_empty` here — task 03 replaces both call sites and removes it (its unit tests move there).
|
||||
|
||||
- ASSUMPTION: "candidate" means exactly what `generate_folder_summaries` already means (the `00_phase.md`/phase-94 recursive-subtree rule) — a single-document folder is still not a candidate, so it can never appear as a "gap".
|
||||
- ASSUMPTION: `only_missing` is a **fill**, not a refresh — a stored row is never regenerated or re-stamped by an `only_missing` run, even a stale-looking one (staleness is the changed-KB regeneration's job, as today).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit — `tests/unit/test_folder_summaries.py` (extend the existing duck-typed fake-LLM + in-memory/SQLite-DB patterns used by that file):
|
||||
- `missing_folder_summaries`: fresh table + populated KB → exactly the candidate set; fully-populated table → `[]`; delete one stored row → `[(that source, that folder)]`; a single-document folder present in the KB and absent from the table → NOT listed (not a candidate); a stored row for a folder that dropped below 2 docs → not listed as missing (it's stale, not missing — the prune pass owns it).
|
||||
- `only_missing=True` with two missing candidates + two present ones: exactly 2 `llm.chat` calls (the missing keys, sorted order); the two present rows are byte-identical afterward (summary text **and** `updated_at` unchanged — capture both before/after); stats `generated=2, failed=0, pruned=0`.
|
||||
- `only_missing=True` with no gap: **zero** `llm.chat` calls, no rows touched (the zero-burn invariant the gate relies on).
|
||||
- `only_missing=True` + a folder that lost ≥ 2 docs since its row was written (seed a stale row manually): the prune pass still deletes it (`pruned=1`) while the genuine missing folders are filled.
|
||||
- `only_missing=False` regression: existing generator tests stay green unchanged (full regeneration + prune).
|
||||
- Coverage: **>90%** on the new/modified `app/rag/folder_summaries.py` lines.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_folder_summaries.py -v` green with the pins above
|
||||
- [ ] `generate_folder_summaries` default path is behavior-identical (all pre-existing tests unmodified and green)
|
||||
- [ ] full suite green, coverage >90%, `uv run ruff check . && uv run pyright` clean
|
||||
- [ ] no behavior change in completed work (phase 94's generator contract intact)
|
||||
@@ -0,0 +1,43 @@
|
||||
# Task 03 — The gap gate in both sync paths (replace the table-empty probe)
|
||||
|
||||
**Phase:** `96_oneshot_resilience` · **Story:** n/a.
|
||||
|
||||
## Objective
|
||||
Wire task 02's primitives into the folder-summary gate in BOTH sync paths so an unchanged-KB sync self-heals missing rows (targeted fill) while a no-gap unchanged sync still burns zero `lite` calls and a changed-KB sync still regenerates everything exactly as today. This is the half of the fix that turns "a failed folder summary persists until a KB change" into "the next sync heals it".
|
||||
|
||||
## Work
|
||||
1. `scripts/import_docs.py` — the `_run()` folder-summary block (the change-gated region next to the KB-overview regeneration):
|
||||
- Keep the `--limit` full skip exactly as today (an incomplete debug walk must never regenerate or gap-fill).
|
||||
- Changed KB (`summary.added + summary.updated > 0`) → `generate_folder_summaries(session, llm)` — full regeneration (unchanged).
|
||||
- Unchanged KB → replace `folders_due = _folder_summaries_table_empty()` with `missing = missing_folder_summaries(session)` and `folders_due = bool(missing)`; when due, call `generate_folder_summaries(session, llm, only_missing=True)`. (This subsumes the table-empty first-run case exactly: empty table ⇒ every candidate is missing ⇒ `only_missing` over all candidates == a full generation.)
|
||||
- The run's summary line token stays `folder_summaries=<g>/<f>/<p>`; when the run took the gap-fill path, append ` (gap-fill)` to the token (the PLAN §9 greppable-cron-safe line — the line-extension house rule). The `folder_stats is None` → `skipped` rendering is unchanged.
|
||||
- Update the module docstring's folder-summary paragraph (the phase-94 text) to name the gap-fill trigger alongside the table-empty one.
|
||||
2. `app/api/sync.py` — `_run_sync()` (the `fs_db` block, ~L271–283):
|
||||
- `changed = summary.added + summary.updated > 0`. `changed` → full regeneration (unchanged). Unchanged → `missing = missing_folder_summaries(fs_db)`; non-empty → `generate_folder_summaries(fs_db, llm, only_missing=True)` + `logger.info("sync: folder_summaries gap-fill stats=%s", …)`; empty → the existing `sync: folder_summaries skipped (KB unchanged)` log.
|
||||
- Update the block's docstring comment (the phase-94 text) the same way.
|
||||
- No `sync_status` surface change (folder stats stay log-only — the phase-94 contract).
|
||||
3. `app/rag/folder_summaries.py` — remove `folder_summary_table_empty` now that BOTH call sites use `missing_folder_summaries` (it becomes dead code). If anything else imports it (grep first), keep it and note why in the commit body; the expectation is exactly the two sync paths.
|
||||
4. Imports: `missing_folder_summaries` in both sync modules (replace the `folder_summary_table_empty` imports).
|
||||
|
||||
- ASSUMPTION: the gate's "unchanged" decision is the importer's `added + updated` count exactly as today — a prune-only run (files deleted, none added/updated) is unchanged for the gate's purposes, so a prune that drops a folder below 2 docs is healed by the FULL regeneration on the next CHANGED run, not by a gap-fill (prune and gap-fill never race).
|
||||
- ASSUMPTION: transaction convention unchanged — the generator flushes, each path commits in its own short-lived session (phase-53), so a gap-fill never half-writes and never blocks the `bump_sources_version` (the bump stays change-gated on the KB, not the summaries).
|
||||
|
||||
## Testing & Quality
|
||||
- Integration — extend `tests/integration/test_sync_folder_summaries.py` (the phase-94 suite) and mirror the `test_import_docs_overview.py` fake-`LLMClient`-keyed-on-`FOLDER_SUMMARY_MODE` pattern; the fake records its `FOLDER_SUMMARY_MODE` calls so zero-burn and targeted-fill are directly asserted:
|
||||
- **Script path, unchanged + gap:** import a changed KB (rows land), delete one stored row directly, re-run with no KB change → exactly **one** new `FOLDER_SUMMARY_MODE` call (the deleted row's folder only), that row is back with the deterministic fake text, every other row byte-identical (summary + `updated_at`), summary line carries `folder_summaries=…(gap-fill)`.
|
||||
- **Script path, unchanged + no gap:** re-run with no change and a complete table → **zero** `FOLDER_SUMMARY_MODE` calls, token `folder_summaries=skipped` (the phase-94 zero-burn invariant, unchanged).
|
||||
- **Script path, changed:** a KB change still triggers FULL regeneration (call count == candidate count, all rows re-stamped) — today's behavior.
|
||||
- **Script path, `--limit`:** still skips generation entirely (zero calls, `skipped`).
|
||||
- **API path** (`tests/integration/test_sync_api.py` extension or the folder suite's API variant): the same three branches via `POST /api/sync` — unchanged+gap → targeted fill + the `gap-fill` log; unchanged+no-gap → zero calls; changed → full.
|
||||
- **First-run subsumption:** a fresh table (no rows) + populated KB on an unchanged walk → the gap-fill path generates every candidate (equivalent to the old table-empty trigger) — assert the full candidate set lands.
|
||||
- The existing `test_sync_api.py` + `test_import_docs_git.py` + the phase-94 suite stay green (status shape + full-regeneration behavior unchanged).
|
||||
- Coverage: **>90%** on the modified `app/` lines (`app/api/sync.py`; `scripts/` is outside the `app/` coverage gate but the integration tests exercise it).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] unchanged-KB sync with one deleted row regenerates exactly that row on BOTH paths, leaves the rest byte-identical, and logs the gap-fill (integration-pinned)
|
||||
- [ ] unchanged-KB sync with a complete table burns zero folder-summary `lite` calls (both paths)
|
||||
- [ ] changed-KB sync regenerates all candidates (both paths) — byte-identical to today's behavior
|
||||
- [ ] `--limit` (script) skips generation entirely; `sync_status` shape unchanged (API)
|
||||
- [ ] `folder_summary_table_empty` removed (or retained with a documented reason); no dead imports
|
||||
- [ ] `uv run pytest` green, coverage >90%, `uv run ruff check . && uv run pyright` clean
|
||||
- [ ] no behavior change in completed work (phase 94 + phase 53 bump contract intact)
|
||||
@@ -0,0 +1,41 @@
|
||||
# Task 04 — E2E: the incident shape, retried and healed (the dedicated story suite)
|
||||
|
||||
**Phase:** `96_oneshot_resilience` · **Source:** 2026-09-11 incident (owner chat — the whole item: the empty-reply failure class + the missing-row gap). **Story:** n/a (A16 — one Playwright file per phase, run in isolation).
|
||||
|
||||
## Objective
|
||||
`tests/e2e/test_oneshot_llm_retry.py` proves the whole fix end to end against the deterministic mock: a folder whose FIRST one-shot reply arrives in the exact incident shape (`content=""`, `finish_reason="length"`) still ends up with its stored summary (the task-01 retry recovered it, visible in the `ls` drill-down), a folder whose replies are ALWAYS empty stays absent without failing the sync (task-01 exhaustion + the fail-soft contract), and a row deleted behind the app's back is self-healed by the next unchanged sync with the other rows untouched (tasks 02/03 targeted fill).
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/mock_llm.py` — the `compose_answer` `FOLDER_SUMMARY_MODE` branch (L1504) gains the incident-shape injection, keyed by the folder LABEL parsed from the `Folder: …` header (the existing parse — the seeded KB's folder names carry the trigger, so the injection is a pure function of the request, the house marker-flow convention):
|
||||
- A module-level per-label counter (the phase-67 `_fail_posts`/`_bump_fail` pattern; the mock is single-conversation per E2E server — reset each label's counter after the success it guards, so a second sync re-drives the sequence deterministically).
|
||||
- Label ends with `/e2e_empty_once` → the FIRST non-stream POST for that label returns the incident envelope: the mock's normal OpenAI chat-completion shape with `choices[0].message.content = ""` and `choices[0].finish_reason = "length"`; every later POST returns the normal `Fixture folder summary for {folder}.` line. (Inject at the NON-STREAM response path — `chat()` posts `stream=false`; the phase-67 `_llm_500` injection shows where the non-stream branch answers.)
|
||||
- Label ends with `/e2e_empty_always` → EVERY non-stream POST for that label returns the empty envelope.
|
||||
- No other mock branch changes; the trigger strings are this suite's own folder names, so no other E2E can hit them (they seed different trees).
|
||||
2. `tests/e2e/test_oneshot_llm_retry.py` (new — isolated: `tests/e2e/conftest.py` fixtures `app_server` + `mock_llm`; admin login via `tests/e2e/auth_helpers.py`; the test server boots with `BOR_LLM_RETRY_DELAY=0` per the phase-67 pattern in `test_llm_retry.py`, default `BOR_LLM_RETRIES=3`):
|
||||
- **Seed:** one temp local-dir source (the `test_local_directory_sources.py` registration + Sync pattern) with three ≥ 2-doc folders — `e2e_empty_once/` (2 `.md` files), `e2e_empty_always/` (2), `normal/` (2) — small, distinct contents.
|
||||
- **Sync #1** via `POST /api/sync` (the `test_sync_button.py` pattern; wait for completion) → then a scripted `ls <source>` drill-down turn (the phase-94 `test_ls_tree_drilldown.py` echo pattern — the mock's grounded answer carries the `ls` result, the E2E's only lens on the LLM context) asserting:
|
||||
- the `e2e_empty_once/` line is `e2e_empty_once/ — 2 documents: Fixture folder summary for <src>/e2e_empty_once.` — the retry recovered the row (without task 01 this line has no `: …` suffix);
|
||||
- the `normal/` line carries its summary;
|
||||
- the `e2e_empty_always/` line is `e2e_empty_always/ — 2 documents` with **no** `: …` suffix (all 4 attempts empty → `LLMError` → per-folder fail-soft → no row);
|
||||
- the sync reported success (the status endpoint / UI success state — a folder-summary exhaustion never flips the run, phase 94).
|
||||
- **Gap-fill:** delete the `normal` stored row directly from the DB (the test process has DB access via the conftest engine — the same connection the app uses), with no KB change, capture `e2e_empty_once`'s row `updated_at` → **Sync #2** → assert:
|
||||
- a second scripted `ls <source>` turn: `normal/` carries `: Fixture folder summary for <src>/normal.` again (healed); `e2e_empty_once/` still carries its summary;
|
||||
- the DB: the `normal` row's summary is the deterministic text; the `e2e_empty_once` row's `updated_at` is **unchanged** (targeted fill — a full regeneration would have re-stamped it);
|
||||
- `e2e_empty_always` is STILL absent (the gap-fill attempted it, exhausted, stayed absent) and the sync still succeeded.
|
||||
3. Run in isolation: `uv run pytest tests/e2e/test_oneshot_llm_retry.py -v --no-cov` (db up: `podman compose up -d db`).
|
||||
4. Regression gate, in isolation: `test_ls_tree_drilldown.py` (phase 94 — the drill-down + folder-summary surface), `test_sync_button.py`, `test_local_directory_sources.py`, `test_llm_retry.py` (phase 67 — the streaming retry path must be untouched).
|
||||
|
||||
- ASSUMPTION: assertions on what the LLM's context carried go through the deterministic mock echoing the `ls` result into its answer (the house pattern from the agent-tool suites); DB-level assertions (row presence, `updated_at`) use the test process's direct DB access — the E2E's two established lenses.
|
||||
- ASSUMPTION: the mock's per-label counters reset per sync (after the success they guard), so sync #2 re-drives `e2e_empty_once` from "first POST empty" — which is FINE here: the row already exists, so the gap-fill never calls it at all (the `updated_at` assertion proves that); `e2e_empty_always` exhausts again by design.
|
||||
- ASSUMPTION: `BOR_LLM_RETRY_DELAY=0` on the test server keeps the 4-attempt exhaustion paths instant (phase 67's fast-suite convention); default `BOR_LLM_RETRIES=3` means the `e2e_empty_always` folder costs 4 POSTs per sync that attempts it — the suite stays fast.
|
||||
|
||||
## Testing & Quality
|
||||
- This IS the E2E task; the mock's new branch is covered by the suite itself (the mock is a real OpenAI-compatible server — no separate unit needed, per the existing agent/tool suites' practice).
|
||||
- Coverage: **>90%** on `app/` (this task adds test code only — keep the gate green).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/e2e/test_oneshot_llm_retry.py -v --no-cov` green in isolation and encoding: incident-shaped empty reply → retried → summary visible in the `ls` drill-down; always-empty folder → absent with the sync green; deleted row → self-healed on the next unchanged sync with `updated_at` untouched on the other rows
|
||||
- [ ] the mock injection is label-keyed and resets deterministically (a re-run of the suite is green without manual state cleanup)
|
||||
- [ ] regression E2E green in isolation: `test_ls_tree_drilldown.py`, `test_sync_button.py`, `test_local_directory_sources.py`, `test_llm_retry.py`
|
||||
- [ ] full test suite green, coverage >90%, `uv run ruff check . && uv run pyright` clean
|
||||
- [ ] one atomic Conventional Commit, `--no-gpg-sign` (message in `00_phase.md`), phase dir moved to `.agents/phases/complete/`
|
||||
Reference in New Issue
Block a user