Repository navigation
feat(chat): render an answer's state, and keep the part that arrived - #149
Merged
Merged
Conversation
Fourth step of the chat execution epic (#138), for #142. #139 decided what a turn is, #140 gave it an owner, #141 put the SDK's own events on the wire — and the transcript was still rendering `isStreaming` booleans and a `finishReason` copied into the metadata bag, because it had nothing else to read. It has the record now: - **`shared/utils/answerState.ts` is the only mapping from `status` to what the reader sees.** Live or ended, which line explains it, whether partial text is kept. One function, so the answer that is arriving and the same answer after a restart cannot drift: both hand the same fields to the same code. - **Four states get one muted line beside the answer**: `truncated` (the output ceiling), `blocked` (the provider refused), `aborted` (the reader stopped it), `failed` (generation broke, with the reason). None is an error card, none replaces the text. `completed` says nothing, and neither does a row written before the column existed — unknown is not a claim. - **A failed or stopped turn keeps its partial answer.** It used to be replaced by `❌ Error: …`, which threw away the only thing the reader had. That is the visible change of this PR. - **`isStreaming` / `isReasoningStreaming` are gone**, from the message type and from the store. Liveness comes from `status`; "the model is still thinking" comes from the assembled message's own part state (`isReasoningLive`), so the store no longer tracks it from events. The renderer's placeholder starts as `pending` and becomes `streaming` on the first event — the same transition the execution makes. - **The #137 bridge is removed.** The provider's finish reason is no longer copied into `metadata`: the record has a `finish_reason` column, and one fact belongs in one place. `parseFinishReason` goes with it. A race the tests caught on the way: the outcome event can arrive while the last chunks are still being assembled, so applying it immediately dropped exactly the partial answer this PR exists to keep. The outcome is now applied *after* the assembler drains. Also fixed: `loadMessages` no longer patches an in-flight message with an `isStreaming` flag — the row's own `status` is already `streaming` while the turn runs, so the overlay only supplies the text that is not persisted yet. Verified: `npm test` 324 pass (5 new in `test/answerState.test.ts`; the store and manager tests updated to the status-driven shape), `npm run typecheck`, `npm run check:design`, `npm run build`, `npm run build:unpack` + `smoke:packaged` (23 checks), `npx eslint` on the changed files (0 problems). The renderer bundle is unchanged (6,093.40 kB). `DESIGN.md` gains the answer-state contract next to the per-zone state table.
mrsibe
added a commit
that referenced
this pull request
Sep 27, 2026
…urable ceiling Closes #150, the first half of #143. Three things the turn lifecycle could not do, all of them about a turn that goes wrong rather than about what a turn *is*. **A stream that goes quiet is cancelled.** Nothing noticed a provider that stopped producing while holding the connection open: the turn sat in `streaming` forever, the composer stayed locked, and a hung provider looked exactly like a slow one. There is a deadline now, reset by every event — reasoning deltas included, so a model that thinks for a long time is not cut off mid-thought. It *cancels* rather than settling, because whether a silent provider is worth another attempt is the retry policy's call, so `ChatExecution` learned to cancel without deciding anything. **A transient failure before the first token is retried.** Retrying is the wrong answer for a deterministic failure — same wait, same error, plus tokens — so only recognisably transient failures qualify (rate limit, 429, 5xx, overloaded, timeout, the transport's own errors), and anything unrecognised is reported as it is. Two more conditions: nothing the reader would see may have been sent, and the attempts are bounded with backoff. Structural events (`start`, `start-step`) do not count as content, which is what makes the retry reachable at all: verified against the pinned SDK, a second attempt's parts append to an empty message, so the reader sees one answer. **The output ceiling stops being a hard-coded constant.** `DEFAULT_MAX_TOKENS = 2048` was sent on every call and was not configurable anywhere. On a reasoning model the thinking tokens share that budget with the answer, so the reader saw `truncated` early even with a perfectly healthy stream. The connection now carries `maxOutputTokens`, and absent means no ceiling is sent at all — the model's own default applies. It is a per-connection field for the same reason the model id is: the right value belongs to the model, not to KnowNote. Settings shows Automatic or a custom value, chat connections only. Each attempt gets its own signal (`ChatExecution.beginAttempt`), so a retry after a timeout or a cancel does not inherit an aborted one, and the outcome a cancelled attempt reports is translated to `failed(timeout)` however the cancellation surfaced — a rejected read, or the SDK turning it into an `abort` chunk. Verified: `npm test` 336 pass (6 new in `test/retryPolicy.test.ts`; five in `test/chatStreamManager.test.ts` covering a retry that succeeds, a transient failure after content that is *not* retried, a deterministic failure that is not retried, bounded attempts, and a silent stream that is cancelled and fails as a timeout; plus the ceiling's two cases in `test/streamChat.test.ts`), `npm run typecheck`, `npm run check:design`, `npm run build`, `npm run build:unpack` + `smoke:packaged` (23 checks). Based on #142's branch: #149 is not merged yet, and this work is independent of it.
mrsibe
added a commit
that referenced
this pull request
Sep 27, 2026
…urable ceiling (#152) Closes #150, the first half of #143. Three things the turn lifecycle could not do, all of them about a turn that goes wrong rather than about what a turn *is*. **A stream that goes quiet is cancelled.** Nothing noticed a provider that stopped producing while holding the connection open: the turn sat in `streaming` forever, the composer stayed locked, and a hung provider looked exactly like a slow one. There is a deadline now, reset by every event — reasoning deltas included, so a model that thinks for a long time is not cut off mid-thought. It *cancels* rather than settling, because whether a silent provider is worth another attempt is the retry policy's call, so `ChatExecution` learned to cancel without deciding anything. **A transient failure before the first token is retried.** Retrying is the wrong answer for a deterministic failure — same wait, same error, plus tokens — so only recognisably transient failures qualify (rate limit, 429, 5xx, overloaded, timeout, the transport's own errors), and anything unrecognised is reported as it is. Two more conditions: nothing the reader would see may have been sent, and the attempts are bounded with backoff. Structural events (`start`, `start-step`) do not count as content, which is what makes the retry reachable at all: verified against the pinned SDK, a second attempt's parts append to an empty message, so the reader sees one answer. **The output ceiling stops being a hard-coded constant.** `DEFAULT_MAX_TOKENS = 2048` was sent on every call and was not configurable anywhere. On a reasoning model the thinking tokens share that budget with the answer, so the reader saw `truncated` early even with a perfectly healthy stream. The connection now carries `maxOutputTokens`, and absent means no ceiling is sent at all — the model's own default applies. It is a per-connection field for the same reason the model id is: the right value belongs to the model, not to KnowNote. Settings shows Automatic or a custom value, chat connections only. Each attempt gets its own signal (`ChatExecution.beginAttempt`), so a retry after a timeout or a cancel does not inherit an aborted one, and the outcome a cancelled attempt reports is translated to `failed(timeout)` however the cancellation surfaced — a rejected read, or the SDK turning it into an `abort` chunk. Verified: `npm test` 336 pass (6 new in `test/retryPolicy.test.ts`; five in `test/chatStreamManager.test.ts` covering a retry that succeeds, a transient failure after content that is *not* retried, a deterministic failure that is not retried, bounded attempts, and a silent stream that is cancelled and fails as a timeout; plus the ceiling's two cases in `test/streamChat.test.ts`), `npm run typecheck`, `npm run check:design`, `npm run build`, `npm run build:unpack` + `smoke:packaged` (23 checks). Based on #142's branch: #149 is not merged yet, and this work is independent of it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #142. Fourth step of the chat execution epic (#138): #139 decided what a turn is, #140 gave it an owner, #141 put the SDK's own events on the wire — and the transcript was still rendering
isStreamingbooleans and afinishReasoncopied into the metadata bag, because it had nothing else to read.It has the record now.
What changes
shared/utils/answerState.tsis the only mapping fromstatusto what the reader sees: live or ended, which line explains it, whether partial text is kept. One function, so the answer that is arriving and the same answer after a restart cannot drift — both hand the same fields to the same code. That is the acceptance item "running-turn overlay and reloaded-from-database render the same thing" expressed as a property rather than a promise.text-xs text-subtle-foreground, the treatment the retrieval lines already use):statustruncatedblockedabortedfailedNone is an error card, and none replaces the text.
completedsays nothing; a row written before the column existed says nothing either, because unknown is not a claim.❌ Error: …, which threw away the only thing the reader had. This is the visible change of the PR.isStreaming/isReasoningStreamingare gone, from the message type and from the store. Liveness comes fromstatus; "the model is still thinking" comes from the assembled message's own part state (isReasoningLive), so the store no longer tracks it from events. The renderer's placeholder starts aspendingand becomesstreamingon the first event — the same transition the execution makes in Main.metadata: the record has afinish_reasoncolumn, and one fact belongs in one place.parseFinishReasongoes with it, and the manager test now asserts the metadata bag stays free of it.loadMessagesno longer patches an in-flight message with anisStreamingflag — the row's ownstatusis alreadystreamingwhile the turn runs, so the overlay supplies only the text that is not persisted yet.A race the tests caught
The outcome event can arrive while the last chunks are still being assembled. Applying it immediately dropped exactly the partial answer this PR exists to keep, so a stopped turn could render empty. The assembler now exposes
drained, and the outcome is applied after it resolves. Found bytest/chatTurnEvents.test.ts, which is the reason that test exists.Verification
npm testtest/answerState.test.tsnpm run typechecknpm run check:designnpm run buildnpm run build:unpack+smoke:packagednpx eslint <changed files>DESIGN.mdgains the answer-state contract next to the per-zone state table, anddocs/architecture.md's chat-execution section already describes the protocol this renders from.Not hand-verified: the lines themselves in the GUI (both themes). They are three i18n strings per locale behind one pure mapping; the states they render are the ones the tests drive.
What is left of the epic
#143 (stream reliability) is the last child. With this merged, every Definition-of-Done item in #138 apart from that issue is satisfied.