phase: 120_failed_turn_retry
All verification complete. Final report: **Phase 120 — Failed-turn retry: verification pass (all 3 tasks were done; final verification + 1 regression fix)** **Verified:** `ChatMessage.failed`/`error` (≤500, `extra="forbid"` intact); `finalizeFailedTurn` funnel on the 3 failure paths (catch-else, stream-drop guard, zero-frame fallback) with `failed: true` + capped detail + `markLastRetryable`; `appendFailedNote` restore branch (Save-as-doc/Tune excluded); `showErrorBanner`/`retryLastTurn` byte-pinned untouched; only the three paths persist `failed: true` (grep + unit pin); no test asserts the old broken behavior. **Defect found & fixed (rule 7):** a real navigate-away mid-turn let the browser's teardown fetch rejection (TypeError, not AbortError) leak into the failed funnel, persisting a phantom failed brain record — `test_sources_midstream_bug.py::test_no_orphan_brain_message_when_navigated_before_first_token` failed (2 `.msg` after reload) and violated the phase-20 navigate-away convention. Fixed: turn-scoped `leftThePage` flag (set unconditionally on `pagehide`, reset in `runTurn`) skips the funnel in the catch-else branch; pinned by new unit test `test_navigate_away_is_not_a_failed_turn`. No phase-overview/PLAN/todo/complete files touched; no commits made. **Gates (exact):** - `uv run pytest` → 2577 passed - `uv run pytest --cov=app --cov-report=term-missing` → TOTAL 4271 stmts, 99% (>90%) - `uv run pytest tests/e2e/test_failed_turn_retry.py -v --no-cov` → 4 passed (isolated) - `uv run ruff check . && uv run pyright` → clean (0 errors) - Regression E2E, isolated: `test_sources_midstream_bug.py` 6/6 (was 5/6); `test_llm_retry`/`test_tool_scaffolding_guardrails`/`test_stop_generation`/`test_navbar_refresh` 17/17 **Completion criteria:** (1) network error → banner + in-bubble Retry, re-ask without re-typing ✅ (E2E A); (2) refresh restores failed bubble + working Retry, no "new chat" ✅ (E2E C); (3) stopped/successful turns byte-identical ✅ (negative E2E, stop suite, byte-identity units); (4) pytest/coverage/lint/types ✅; (5) commit + phase move — left to the harness per pass rules. **Notable:** deviation = the regression fix above (a navigation is not a failed turn; phase-20 partial-persist convention restored). Next pending phase: `121_git_source_tokens`.
This commit is contained in:
@@ -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: <detail or fallback>, failed: true, error: <detail> }` 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"
|
||||
```
|
||||
Reference in New Issue
Block a user