Skip to content

fix(chat): settle a turn once, so an abort is not a completion - #144

Merged
mrsibe merged 1 commit into
mainfrom
refactor/chat-execution-semantics
Sep 27, 2026
Merged

mrsibe merged 1 commit into
mainfrom
refactor/chat-execution-semantics

Conversation

@mrsibe

@mrsibe mrsibe commented Sep 27, 2026

Copy link
Copy Markdown
Owner

Step 1 of the chat execution epic (#138), closing #139. It fixes the last two ways a turn was reported as something it wasn't, and puts the decision in one place so the next event type cannot invent a third.

What was still wrong after #137

#137 made an in-stream error part fail the turn instead of completing it. Two siblings of that bug were left:

A user abort was a completion. ModelClient's catch called onComplete() whenever the signal was aborted, so chatHandlers persisted the partial text as a finished turn. Whether the renderer also received the terminal finish chunk depended on whether the SDK's delayed usage / finishReason promises rejected — the same user action could land in either state.

An EOF was a completion. Falling out of for await was treated as success. The AI SDK's contract says the opposite — "Only errors that stop the stream, such as network errors, are thrown" — so an iterator that ends normally carries no claim about the answer. await result.finishReason would have hidden this: it synthesises 'unknown' when no terminal part arrived.

What this changes

type ChatExecutionStatus = 'pending' | 'streaming'
                         | 'completed' | 'truncated' | 'blocked' | 'aborted' | 'failed'

status is KnowNote's statement; finishReason stays the provider's fact, and they are stored as two fields precisely because they are not the same statement:

stop · tool-calls      → completed
length                 → truncated      the answer is kept and explained, not failed
content-filter         → blocked
an error part / throw  → failed (error)
no terminal signal     → failed (unexpected_eof)
user abort             → aborted
  • One decision, taken once. shared/utils/chatExecution.ts holds the mapping plus settleOutcome, the first-terminal-wins rule. A stream can deliver several terminal-looking events — the SDK keeps delivering text after an error part, and transport cleanup can throw after a legitimate finish — and promoting one of them re-labels an answer the reader already has.
  • ModelClient reports, it does not decide. An abort is its own callback, onError carries whether the provider reported the failure or the stream broke, and the reason comes from the SDK's finish part instead of the aggregate promise. The four positional callbacks became an object: with onError / onAbort / onComplete all meaning "the turn ended, differently", a positional call site is one reordering away from this exact bug.
  • The outcome is stored. chat_messages gains status, finish_reason, error, usage, finished_at — migration 0017, additive. A row written earlier reads back status = NULL, which means unknown; the same distinction parseRetrievalStatus already makes, and deliberately not completed.
  • Persist first, announce second. The terminal event now goes out after the write returns, so a failed write cannot leave the transcript claiming a turn the database does not have.

createMessage gained an optional status, and the assistant row is now created as streaming — the row is the turn record.

Scope

Per the epic's split, this is only the semantics and the storage. Not here: ChatStreamManager (#140), the chunk-stream protocol and the deletion of the hand-made done chunk (#141), and everything the transcript renders from status (#142). One consequence worth stating: aborted is persisted but not announced, because the renderer cleared its streaming state when it asked for the stop — announcing it would be a second state change nobody requested. The reload path is #142's job.

Two behaviour changes worth naming:

  • a failed turn keeps its partial content, so a reload shows the partial answer plus its failure where it used to show an empty message;
  • a turn that failed is now counted into the session token estimate rather than skipped (an aborted turn already was, through onComplete).

Verification

npm test 297 pass, 0 fail — 9 new: test/chatExecution.test.ts (7) pins the mapping, the missing-terminal case, and first-terminal-wins in both orders; test/chatStreamOutcome.test.ts gained the abort case and a test that pins the SDK's synthesised finish part, which is what makes "no terminal part at all" a fact rather than an assumption
npm run typecheck clean (node, web, test)
npm run check:design no violations
npm run build:unpack + npm run smoke:packaged PASS — 23 checks, one new: a terminal outcome round-trips through the packaged database (JSON + timestamp columns, partial content kept)
migration 0017 on a real database applied by hand to a copy of an existing pre-migration DB: the five ALTERs are additive, and a legacy row keeps status = NULL
npx eslint <changed files> 0 errors; the 3 no-explicit-any warnings are pre-existing lines

The stream tests drive ModelClient with a scripted provider from ai/test, including an abortable stream that errors on the signal the way a real transport does — a fake that ignored the signal would leave the SDK's iterator open forever, which is a property of that fake rather than of the code.

Not verified by hand: the transcript's rendering of a stopped or failed turn. The states exist and are stored; showing them is #142. GUI confirmation is part of #125's pass.

Next

#140 (ChatStreamManager) is the next step and is what turns this fixed vocabulary into a real lifecycle owner — abort recording, subscriber bookkeeping, and a bounded buffer for opening a notebook mid-turn.

@github-actions github-actions Bot added the bug Something isn't working label Sep 27, 2026
First step of #138, the chat execution epic. A turn used to have exactly one way
to end as far as the code was concerned — `onComplete()` — and three different
situations reached it:

- a user abort. `ModelClient`'s catch called `onComplete()` whenever the signal
  was aborted, so `chatHandlers` persisted the partial text as a finished turn.
  Whether the renderer also received the terminal `finish` chunk depended on
  whether the SDK's delayed `usage` / `finishReason` promises rejected.
- an EOF. Falling out of `for await` was treated as success. The AI SDK's
  contract is the opposite: only errors that stop the stream are thrown.
- a truncation. Since #137 the reason is persisted, but nothing reads it as a
  state, and the renderer infers `completed` from a chunk that says otherwise.

There is now one decision, taken once, by whoever owns the turn.

- `shared/utils/chatExecution.ts` maps a terminal signal to a `status`:
  `stop` / `tool-calls` → `completed`, `length` → `truncated`,
  `content-filter` → `blocked`, `error` → `failed`, no terminal signal at all →
  `failed(unexpected_eof)`. `status` is the product's statement, `finishReason`
  stays the provider's fact, and they are stored as two fields because they are
  not the same statement — a truncated answer is kept and explained, not failed.
- `settleOutcome` is the first-terminal-wins rule. A stream can deliver several
  terminal-looking events (the SDK keeps delivering text after an `error` part,
  and transport cleanup can throw after a legitimate `finish`); promoting one of
  them re-labels an answer the reader already has.
- `ModelClient` reports what it observed and stops deciding. An abort is its own
  callback, and the terminal reason comes from the SDK's `finish` part rather
  than from the aggregate promise, which synthesises `'unknown'` and would hide
  the case that must not look like success. The four positional callbacks became
  an object: with `onError` / `onAbort` / `onComplete` all meaning "the turn
  ended, differently", a positional call site is one reordering away from this
  exact bug.
- The outcome is stored. `chat_messages` gains `status`, `finish_reason`,
  `error`, `usage` and `finished_at` (additive migration `0017`). A row written
  before the migration reads back `status = NULL`, which means unknown — not
  `completed`.
- The terminal state is persisted before the renderer is told the turn ended, so
  a failed write can no longer leave the transcript claiming a turn the database
  does not have.

Not in this PR: the `ChatStreamManager` (#140), the chunk-stream protocol
(#141), and anything the transcript renders from the status (#142). `aborted`
is therefore persisted but not announced to the renderer, which already cleared
its streaming state when it asked for the stop.

Two behaviour changes worth naming: a failed turn now keeps its partial content
(a reload shows the answer plus its failure, where it used to show an empty
message), and a turn that failed is now counted into the session token estimate
rather than skipped.

Verified: `npm test` 297 pass (9 new: 7 in `test/chatExecution.test.ts` for the
state machine, 5 rewritten/added in `test/chatStreamOutcome.test.ts` including
one that pins the SDK's synthesised `finish` part), `npm run typecheck`,
`npm run check:design`, `npm run build:unpack` + `smoke:packaged` (23 checks —
one new, a terminal outcome round-tripping through the packaged database), and
migration `0017` applied by hand to a copy of a real pre-migration database to
confirm the ALTERs are additive and a legacy row keeps `status = NULL`.
@mrsibe
mrsibe force-pushed the refactor/chat-execution-semantics branch from 56c4068 to 0d429a4 Compare September 27, 2026 04:45
@mrsibe
mrsibe merged commit de61433 into main Sep 27, 2026
3 checks passed
@mrsibe
mrsibe deleted the refactor/chat-execution-semantics branch September 27, 2026 04:51
mrsibe added a commit that referenced this pull request Sep 27, 2026
…mManager (#145)

Second step of #138, closing #140. #144 made the turn's semantics correct; this
makes them live in one owner instead of in an IPC handler's closure.

The live state of every running turn used to be:

    const activeStreams = new Map<string, AbortController>()

plus a handful of local variables in `send-message` — the accumulated text, the
reasoning, the usage, the settled outcome, and four closures (`settle`,
`finishTurn`, `accountTurnTokens`, `announceOutcome`) that only existed for the
duration of one call. Nothing could answer "what is this turn doing", nothing
could be reached after the handler returned, and every future feature on the
roadmap — a timeout, a retry, attach-and-replay, a renderer overlay — would have
had to add another closure here.

Now:

| Role | What it is |
| --- | --- |
| The turn's state and rules | `ChatExecution` — one object per turn: accumulated answer, provider reason and usage, and the one terminal outcome |
| The registry and lifecycle | `ChatStreamManager` — one entry per running turn, and the only place a turn is settled, persisted and announced |
| Persistence port | `ChatTurnStore`, implemented by `services/chat/turnStore.ts` over `db/queries` |

`chatHandlers.ts` is 260 lines shorter and assembles the prompt — user message,
history, RAG, provenance — then hands the turn over. Every ending goes through
one path in the manager: a stop the renderer asked for, a stream that failed or
finished or just broke, and a turn that could not start at all.

Three things are deliberate:

- **The manager imports neither `db/queries` nor Electron.** It writes through the
  store port and reports through an `emit` callback supplied per turn, which is why
  `test/chatStreamManager.test.ts` can drive a complete turn — chunks in, one
  persisted record out — with no database and no Electron process. The portable
  `estimateTokens` moved from `SessionAutoSwitchService` to
  `shared/utils/tokenEstimate.ts` for the same reason: it is a pure function, and it
  was only unreachable outside Electron because it lived on a class that needs a
  database.
- **`abort` settles before it cancels.** The `AbortError` the SDK returns afterwards
  is then noise after a decision rather than a second opinion about it, which is what
  the issue asked for and what a test now pins.
- **A finished turn is removed from the registry before the renderer is told**, so a
  late signal cannot find it and cannot start a second life for it.

Not in this PR, per the issue's scope: the chunk-stream protocol (#141), the
renderer overlay (#142), retries, Continue, and the output-ceiling UI. One part of
the issue is deferred rather than done — `seq` and the subscriber/attach API. A
sequence number nobody reads is structure without a consumer, and #141's envelope is
where it gets one; the buffer exists for the same reason. #140's checklist is updated
to say so.

Two behaviour notes: a failed or aborted turn now stores the *grounded* citation
subset rather than the whole evidence list (the same #70 rule delivered answers
already had), and a turn that cannot start writes its provenance before failing.

Verified: `npm test` 306 pass (9 new in `test/chatStreamManager.test.ts`, which drive
the real manager through the real `ModelClient` with a scripted provider — including
an abortable stream that errors on the signal the way a transport does),
`npm run typecheck`, `npm run check:design`, `npm run build:unpack` +
`smoke:packaged` (23 checks), and the packaged smoke still passes after the message
table's new columns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant