Repository navigation
Treat interrupted AI streams as incomplete - #1844
Conversation
laboef1900
left a comment
There was a problem hiding this comment.
Independent Astra-high review of published fc7547d: correct, no actionable findings. EOF without done retains partial/error and bypasses success, promotion and final-application callbacks; explicit errors retain partial, cancellation remains separate, neither consumer stops at the first done marker. Cached completion and later metadata remain supported; embeddings retain their distinct complete event. Inspected actual-worktree browser EOF/error/cached/post-terminal evidence and real backend/loopback cancellation with retained partial and empty editor, desktop/mobile light/dark captures, focused275+corrected137, final frontend5805/rootunit30, lint/type and preceding backend/build/contracts/audit provenance. Initial fixture/diagram failures are retained and corrected; browser-proofed production bytes unchanged. Read-only reviewer did not rerun checks or touch services. No GitHub approval claimed; final-head CI tracked separately.
laboef1900
left a comment
There was a problem hiding this comment.
Independent review: PR #1844
Issue: #1801 (bug; no NO AI label). Target: dev.
Head: fc7547d1e9c6f00bf82b882290ade68fd3daf1eb
Base: 5edff9e08dad25e9db1c5668ff211c794fc9c2fd
Verdict
Correct; no confirmed patch-introduced actionable findings. Confidence: 0.96. Review disposition: COMMENT (read-only source/evidence assessment, not newly executed verification or GitHub approval).
Scope and boundary review
Full twelve-file diff and changed regression contexts; SSE decoder, all three SSE consumers, backend cached/stream frame producers, Generate Save and dock callback/application paths. Embedding WorkersTab retains its existing separate type=complete requirement. Retained Paper desktop and Graphite mobile captures inspected; no new browser session.
Acceptance criteria
-
Require route done before success/final application — FULLY ADDRESSED. AiContext.tsx:1447-1553 gates completeExchange/onComplete on done; use-improve-stream.ts:91-128 gates done status; ImprovePanel.tsx:148-164 requires done for Replace/Insert. Generate Save and dock Apply results are populated by onComplete only.
-
Incomplete output distinct and partial text recoverable; explicit errors/cancel preserved — FULLY ADDRESSED. AiContext failLastMessage retains partial with isError; editor retains output plus error and disables application. AbortError keeps prior cancellation semantics without success callbacks. Regressions cover EOF/error/nonpromotion and editor controls.
-
Cached done without final and later metadata still processed — FULLY ADDRESSED. Neither consumer breaks on done. Conversation metadata deferred until successful completion; finalContent still replaces accumulated output. Hook/thread tests and retained cached/post-terminal browser captures cover these cases.
Findings
None.
Evidence inspected (reused, not executed here)
- pr-1844-metadata.json: exact-head Typecheck & Lint, backend/frontend Tests, Playwright E2E and Semgrep SUCCESS
- epic-1827-evidence/1801/browser-final-light-eof.json and visual-final-{light-desktop,dark-mobile}.png: retained partial, interruption and no Save
- epic-1827-evidence/1801/browser-cached-done.txt and browser-post-terminal.txt: valid cached Save and cleaned replacement
- epic-1827-evidence/1801/browser-real-cancel.json: aborted real request with empty editor
- epic-1827-evidence/1801/published-source-attestation.json: final source manifest linkage
Evidence limits
Static and retained-evidence review only; no new browser/build/test/formatter/DB service. Context launcher/detector not executed under read-only no-check assignment. Does not claim arbitrary network interruption or upstream protocol coverage beyond terminal frames arriving at these frontend consumers. Initial malformed cached fixture, obsolete missing-done fixture, Mermaid correction and inherited nonloopback startup caveats remain disclosed in PR.
Suggested targeted verification
Parent only: npx vitest run src/features/ai/AiContext.threads.test.tsx src/shared/components/article/use-improve-stream.test.ts src/shared/components/article/EditorBubbleMenu.test.tsx src/features/ai/modes/GenerateMode.test.tsx from frontend; isolated browser EOF/error/cached-done/post-done metadata and Stop-response scenarios if fresh smoke is required.
Cross-PR interaction
No conflicting behavior identified among this assigned group by source inspection.
No source edits, branch changes, services, GitHub reviews, pushes or merges performed.
Parent validation and publication
No additional actionable findings from parent triage.
Current final-head GitHub CI is passing. This is an independent source/evidence review, not a newly rerun full Docker/browser matrix or a GitHub approval. Source-only findings are identified as such. No code modifications or merges performed.
Reviewed by PR Reviewer Agent (eight independent review contexts; parent-validated publication).
laboef1900
left a comment
There was a problem hiding this comment.
Automated PR Review
PR: #1844 -- Treat interrupted AI streams as incomplete
Branch: feature/1801-incomplete-ai-streams -> dev (reviewed at fc7547d1)
Diff size: 399 lines (+141/-9)
Reviewed at: 2026-11-01 10:43 UTC
Static Checks
| Check | Status | Details |
|---|---|---|
| TypeScript | PASS | npm run typecheck |
| Lint | PASS | npm run lint |
| Backend Tests | PASS* | 1 failed |
| Frontend Tests | PASS | 5805 passed (5805) |
| Production Build | PASS | npm run build |
| npm audit | FAIL | pre-existing: 11 advisories (10 moderate, 1 high systeminformation) in the shared lockfile; package-lock.json not changed by this PR |
| Branch Target | PASS | feature/* → dev |
Docker App Test
Built from source (docker-compose.yml + docker-compose.build.yml, native arch) on an isolated project pr-review-1844; API reached through the nginx proxy (the backend publishes no host port by design).
| Check | Status |
|---|---|
Containers Healthy (postgres, redis, backend, frontend) |
PASS |
Migrations (fresh DB → Migrations complete, through 129) |
PASS |
Backend /api/health + /api/health/ready |
PASS |
Fresh-instance setup (POST /api/setup/admin → 201) + Login |
PASS |
API /api/pages, /api/search?q=…, /api/analytics/trending, /api/settings |
PASS (200) |
API /api/admin/settings, /api/admin/llm-providers, /api/notifications/count |
PASS (200) |
Frontend loads (GET / → 200) |
PASS |
| No error/fatal logs (level ≥ 50) | PASS |
E2E Browser Tests
CI recipe (E2E_CI=1 npx playwright test: built backend + vite preview, fresh kb_e2e database, provisioning via e2e/helpers/ci-setup.ts).
| Project | Status |
|---|---|
e2e helper unit tests (vitest --config e2e/vitest.config.ts) |
PASS |
| chromium + collab | PASS (68 passed (3.3m)) |
Code Review Findings
| # | Severity | File:Line | Finding |
|---|---|---|---|
| 1 | WARNING | frontend/src/features/ai/AiContext.tsx:1439 | failLastMessage now folds the whole partial answer into the error message string ("${text}\n\nPartial output:\n${accumulated}"). Both renderers paint isError messages as plain text in a destructive <p> with no whitespace-pre-wrap (AiAssistantPage.tsx:111, dock/DockPanel.tsx:701-706), so a multi-paragraph Markdown/code partial collapses into one red run with its line breaks and structure gone — the "recoverable partial text" is retained but not usefully readable or copyable. The same string is also what the primed role="alert" announcers speak (AiAssistantPage.tsx:323-326, DockPanel.tsx:262-264), so an interruption after a long generation pushes thousands of characters into an assertive live region. Keep content: accumulated on the message and carry the interruption separately (e.g. an isIncomplete/errorText field) so the partial renders through the normal Markdown path with a distinct notice and only the notice is announced. |
| 2 | INFO | frontend/src/features/ai/AiContext.tsx:1525-1528, frontend/src/shared/components/article/use-improve-stream.ts:121 | The new "loop ended normally while the signal is already aborted" branches (controller.signal.aborted && !completed → commit; if (controller.signal.aborted) return;) have no test. Every abort test (AiContext.threads.test.tsx:868-894, bubble-menu/hook abort tests) throws AbortError from the generator and exercises the catch path only, so the branch that keeps a cancel from being reported as "interrupted before completion" is unverified. |
| 3 | INFO | CLAUDE.md:14-20 | The new SSE-consumer contract paragraph sits in the project intro between "Source-of-truth docs" and "Mandatory Rules", away from the LLM/SSE guidance it belongs with (LLM Provider Model section, or beside the shared SSE audit-lifecycle note near line 1446). Readers scanning the AI sections will not find it there. |
| 4 | INFO | frontend/src/shared/components/article/ImprovePanel.tsx:225,240 | {!hasResult && <div className="flex gap-1"> … </div>} is written inline on the opening/closing lines, unlike every other conditional block in this file ({cond && ( on its own line, )} closing). Cosmetic; worth matching the file's style. |
Acceptance Criteria (Issue #1801)
| AC | Criterion | Status | Evidence |
|---|---|---|---|
| AC-1 | Require the route's terminal event (done: true) before success/final application |
PASS | AiContext.tsx:1449,1455 sets completed only on chunk.done === true; :1529-1533 fails the turn and returns before commitToMessages/completeExchange/onComplete when !completed. use-improve-stream.ts:91,111,121-126 same gate before setStatus('done'). ImprovePanel.tsx:153,162 disable Replace/Insert unless stream.status === 'done'; the only callers of onReplace/onInsertBelow are those buttons (EditorBubbleMenu.tsx:560-561, EditorBlockMenu.tsx:377-378). Backend contract confirmed: provider streamChat always ends with {content:'', done:true} (openai-compatible-client.ts:602,646), sendCachedSSE emits done:true on the content frame (_helpers.ts:248), so every healthy route (incl. diagram/quality with no extras frame) still completes. All runStream consumers apply via onComplete only (GenerateMode.tsx:419, DiagramMode.tsx:185, ImproveMode.tsx:225, use-dock-actions.ts:157,252,278,337); no onContent consumer exists. Tests: AiContext.threads.test.tsx:791-802, use-improve-stream.test.ts:98-105, EditorBubbleMenu.test.tsx:724-734. |
| AC-2 | Surface incomplete output distinctly while retaining recoverable partial text | PASS (see Finding 1) | AiContext.tsx:1437-1441 keeps accumulated in the isError message; :1530 distinct "interrupted before completion" text. use-improve-stream.ts:122-123 keeps output, sets error/status: 'error'; ImprovePanel.tsx:222-224 shows role="alert" while the preview with the partial stays mounted. Tests assert partial + message: AiContext.threads.test.tsx:798-799, use-improve-stream.test.ts:102-104, EditorBubbleMenu.test.tsx:730-733. Rendering of the partial inside the error bubble is degraded (Finding 1). |
| AC-3 | Preserve legitimate cached responses whose terminal done has no separate final field |
PASS | completed is read off any frame with done: true, including the cached content frame (AiContext.tsx:1455, use-improve-stream.ts:111); matches sendCachedSSE (_helpers.ts:248-253). Tests: AiContext.threads.test.tsx:816-825 ({content, done:true} then final frame), use-improve-stream.test.ts:36,59 ({content, done:true} with no final). |
| AC-4 | Preserve explicit cancel/error behaviour | PASS | Cancel: AiContext.tsx:1525-1528 (aborted && !completed → commit partial, no error) plus unchanged AbortError catch at :1557-1561; use-improve-stream.ts:121 returns silently on abort before the incomplete check; Stop control still calls abortRef.current?.abort() (DockPanel.tsx:527). Error: in-band {error} still breaks first (AiContext.tsx:1451-1454, use-improve-stream.ts:106-110) so completed is never set by {error, done:true}. Tests: AiContext.threads.test.tsx:868-894 (stopped first answer not promoted), :804-814 (explicit error keeps partial), EditorBubbleMenu.test.tsx:737+. Gap: abort branch where the loop ends normally is untested (Finding 2). |
| AC-5 | Do not discard valid metadata frames that follow a completion marker | PASS | Loop does not break on done (AiContext.tsx:1455-1511): finalContent, conversationId, sources, refused, historyTruncated, originalMarkdown, layoutTokensLost after done are all still read; conversationId write deferred to the completed path (:1534-1536) so an incomplete stream never adopts an id. use-improve-stream.ts:113-115 still applies finalContent after done. Tests: AiContext.threads.test.tsx:816-825 (navigates to /ai/c/c-cached from metadata after done), use-improve-stream.test.ts:87-96 (finalContent after done). |
Story Validation (Issue #1801)
Issue intent: A truncated SSE stream (EOF before the route's terminal done event) must not be presented or applied as a completed AI answer; partial text should stay visible as an interrupted result while cached, cancelled and errored streams keep their existing behaviour and post-done metadata is still honoured.
Story completion: FULLY ADDRESSED
User journey: Before: streamSSE ended silently at EOF and runStream/useImproveStream ran their success path, so a cut-off answer was committed as a normal message, promoted to a conversation, offered for Save/Replace/Insert, and onComplete consumers (Generate save panel, Diagram insert, Improve diff) applied it -> After: EOF without done: true leaves the partial in an error-styled turn with "interrupted before completion", no promotion/onComplete, and the editor preview keeps the partial visible but with Replace/Insert disabled; cached {content, done:true} frames, explicit cancel, in-band errors and post-done final/finalContent frames behave as before. The embedding consumer already required type: 'complete' (WorkersTab.tsx:295-305) and is unchanged.
Gaps: sse.ts (named in the issue) is deliberately untouched — classification lives in the consumers, which is a valid design; Finding 1 (partial text readability/a11y inside the error bubble) is the one quality gap; abort-ends-normally branch lacks a test.
Evidence: frontend/src/features/ai/AiContext.tsx:1437-1441,1449-1455,1520-1536; frontend/src/shared/components/article/use-improve-stream.ts:91,111,121-126; frontend/src/shared/components/article/ImprovePanel.tsx:153,162,222-241; tests AiContext.threads.test.tsx:791-825, use-improve-stream.test.ts:98-105, EditorBubbleMenu.test.tsx:724-734; fixture updates adding the terminal frame in AiAssistantPage.test.tsx, GenerateMode.test.tsx, AiContext.threads.test.tsx:545; docs CHANGELOG.md, CLAUDE.md:14-20, docs/architecture/04-frontend-structure.md:69-91, docs/architecture/09-flow-rag-chat.md:130-134.
Suggested Actions
- [SHOULD FIX]
frontend/src/features/ai/AiContext.tsx:1439— keep the partial answer as the messagecontent(rendered through the normal Markdown path) and carry the interruption as a separate field/notice instead of string-concatenating it into the plain-text,role="alert"-announced error body; add renderer support inAiAssistantPage.tsx:108-111anddock/DockPanel.tsx:701-706. - [CONSIDER] Add tests for the abort-then-normal-EOF branch in
AiContext.tsx:1525-1528anduse-improve-stream.ts:121(generator that yields a chunk, then returns after the signal is aborted without throwing) to lock in that a cancel is never reported as "interrupted". - [OPTIONAL] Move the
CLAUDE.md:14-20paragraph next to the existing LLM/SSE guidance; align the inline{!hasResult && <div …>/</div>}atImprovePanel.tsx:225,240with the file's conditional-block style.
Verdict: COMMENT
Reason: All static checks, the Docker smoke run and the Playwright suite pass and issue #1801 is fully addressed (EOF before done: true is now an incomplete stream in both runStream and useImproveStream; cancel, in-band error, cached and post-done metadata frames are preserved). One WARNING: the partial answer is string-concatenated into the plain-text isError message, so a long Markdown/code partial renders as one collapsed red run and is pushed whole into the role="alert" announcer — keep the partial as message content and carry the interruption separately.
Posted as a comment: GitHub rejects APPROVE/REQUEST_CHANGES from the PR author's own account, so the verdict above is advisory.
Reviewed by PR Reviewer Agent | 2026-11-01
…t notice (#1801) failLastMessage folded the retained partial into the error string, so both renderers painted a multi-paragraph/code partial as one red plain-text run and the role="alert" announcers spoke all of it. The partial now stays the message content and the failure rides as a separate `interruption` field; renderers show it as a short destructive notice beneath the Markdown body.
|
Addressed the review WARNING in |
CLAUDE.md was split into docs/claude/*.md on dev (#1877); this branch's guidance paragraph moves to the matching split file.
Fixes #1801
Part of #1827.
Change
AI stream consumers require
done: truebefore success or application, retain incomplete/error partial text, and continue reading post-completion metadata. Cached done without a separate final frame remains valid. The editor preview prevents replacement or insertion of incomplete output; the embedding consumer already requires its distincttype: completeevent.Acceptance and verification
finalContentafter done replaces the prior text. Enter submits. Desktop Paper and mobile Graphite were exercised with no horizontal overflow.5edff9e08dad25e9db1c5668ff211c794fc9c2fdplus the then-current patch. They are reused: the subsequent changes were a Mermaid label and a test fixture adding the required terminal frame, not production source. Final global-gate source manifest SHA-256:328a90c2963838b3109b722f5a29399ec7aac66ef9e2de7e67b9a4821007c132.npxin PATH; the direct Node retry passed. Raw evidence is retained outside the worktree underepic-1827-evidence/1801/(includingglobal-r1,global-r2, and browser captures).env -i; zero pages and no provider request were observed during that startup, but absence of all external traffic was not independently established. Later provider traffic used loopback only.Published revision:
fc7547d1e9c6f00bf82b882290ade68fd3daf1eb.