docs/ai-chat-cancellation-design.md

Rama master · réplica de lectura del repositorio original.

# AI Chat: Cancellation, State, and Persistence Design Issues

This document captures the architectural issues in the AI chat stack that
have surfaced as a recent cluster of bugs around stop / queueing / tool
dispatch, and proposes a phased fix.

## The pattern

The bugs are all expressions of the same underlying issue. Catalog:

1. **Silent stop orphans the queue.** User sends A, A streams a reply,
   user presses Stop, user sends B. B never gets dispatched.
   - Root: `ChatAgent.stop()` called `conversation.stop()` →
     `controller.cancel()`, which set state to `.ground` *silently* and
     never fired the delegate callback. The completion chain from
     `AITermController` → `AIConversation.delegate` →
     `ChatAgent.fetchCompletion` → `ChatService.finishTurn` died at the
     first link. The queue head pointer (A) was never popped; the
     `enqueue(B)` saw `queue.count == 2` and skipped `startNextTurn`.
   - Patched in `ChatAgent.stop()` by routing through
     `conversation.cancelOutstandingOperation()` instead. Pinned by
     `test_chat_stopThenSend_secondMessageStillDispatches`.

2. **Stop during a parked tool dispatch is a no-op.** Same shape as (1),
   but the stop fires while the controller is waiting for the LLM
   framework's function-output. `parseStreamingResponse(final: true)`
   sets `state = .ground` after dispatching the function call, so
   `handle(.cancel)` and `handle(.error)` are *both* ignored by the
   state machine for the entire duration of the tool execution —
   typically seconds.
   - Patched in `AIConversation.cancelOutstandingOperation()` by
     falling back to directly firing
     `delegate.aitermControllerDidCancelOutstandingRequest(controller)`
     when `delegate.busy` is still true after the state machine call.
     Pinned by `test_chat_orphanToolResponseAfterStop_doesNotOrphanQueuedMessage`.

3. **Orphan tool response silently cancels the next queued turn.**
   After (1) and (2) are fixed, the in-flight `PTYSession` command
   eventually finishes and publishes a `.remoteCommandResponse`. The
   dispatcher that was supposed to consume it has been cancelled, so
   `handleRemoteCommandResponse` returns false. The code fell through
   to `fetchCompletionForRegularMessage`, which called
   `conversation.complete → prepare → cancel()` — the *silent* variant
   that sets `delegate.completion = noop`. The actively in-flight
   queued turn (B) had its completion silently overwritten and never
   fired `finishTurn`.
   - Patched in `ChatAgent.fetchCompletion` by dropping orphan
     `.remoteCommandResponse` messages with `completion(nil); return`
     instead of falling through. Pinned by the same test as (2).

4. **Auto-approved `tool_use` not persisted to chat DB.**
   `ChatClient.processRemoteCommandRequest` returns `nil` from the
   broker processor on the `.always` permission path.
   `ChatBroker.publish` interprets `nil` as a full squelch — skips both
   subscriber delivery *and* `listModel.append`. The `tool_use` record
   carrying the LLM's `function_call` (with its `toolu_…` id) never
   lands in the DB. For the current turn that's fine because
   `AIConversation.messages` is in-memory; but when the next turn
   dequeues from the queue, `ChatAgent.load(messages: history)`
   reconstructs from DB, sees an orphan `tool_result` with no
   preceding `tool_use`, and Anthropic 400s.
   - Patched in `ChatAgent.translate` by synthesizing the missing
     `tool_use` whenever a `tool_result` references a request id that
     wasn't persisted. Pinned by
     `test_chat_squelchedToolUseHistoryDoesNotBreakNextTurn`.

5. **`dropLast()` assumes the wrong invariant.**
   `ChatService.startNextTurn` builds B's history as
   `Array(self.messages(chatID:).dropLast())`, on the assumption that
   the last DB row is the in-flight user message. With B typed during
   A's tool round-trip, the DB ends up persistence-ordered as
   `[A, B, tool_use(if not squelched), tool_result, A_reply]`. The
   last row is `A_reply` — `dropLast` drops it. So B's history is
   missing A's full turn outcome (A_reply), and B itself is included
   in history (then re-added via `conversation.add(userAIMessage)`
   in `fetchCompletionForRegularMessage`, duplicating it on the wire).
   - **Not yet fixed.** The current `translate` + `enforceToolUseAdjacency`
     hacks paper over the worst symptoms but the structural assumption
     is wrong.

## Why these are all the same bug

Four observations:

### (a) Four independent representations of "in flight"

| Layer | State for "a turn is running" |
|---|---|
| `AITermController` | `state: State` enum + `cancellation` token |
| `AIConversation` | `delegate.busy` boolean |
| `ChatAgent` | `pendingRemoteCommands: [UUID: PendingRemoteCommand]` + `conversation.busy` |
| `ChatService` | `pendingMessages: [String: [Message]]` |

These four states evolve independently. The bugs are all
"layer X thinks the turn is done, layer Y thinks it's still running" —
or the reverse.

### (b) Silent cancellation paths

`controller.cancel()` (AITerm.swift:205), `AIConversation.cancel()` in
`prepare` (AIConversation.swift:222), and the previous
`AIConversation.stop()` (line 306) all set state to clean and
overwrite `delegate.completion = noop`. They do not fire the delegate
callback. The outer completion chain dies silently.

The recently-fixed bugs are all silent-cancel paths leaking into
visible behavior. There may be more we haven't hit yet:

- `delegate.streaming = nil` in `cancel()` could orphan a partial
  streaming reply.
- `delegate.createVectorStoreCompletion = nil` etc. in `cancel()` could
  orphan a vector-store creation in flight.

### (c) State-machine dead zones

`.ground` ignores every event (AITerm.swift:227-229). But the
controller can sit in `.ground` for arbitrary wall-clock time while
the function-dispatch layer is waiting for a tool result. Cancellation,
errors, even unexpected web responses during that window all evaporate.

The state machine has no `.awaitingFunctionOutput` state. The handoff
from `parseStreamingResponse(final: true)` to `doFunctionCall`
implicitly uses `.ground` as that state, but `.ground`'s catch-all
ignore behavior is wrong for it.

### (d) Three things conflated into one stream

- **The LLM conversation** — strict ordering: alternating user/assistant
  with tool_use immediately followed by tool_result. Used to build the
  API request.
- **The chat history** — what the user sees in bubble UI, persisted to
  SQLite. Lossier and more permissive than the LLM conversation.
- **The dispatch queue** — pending user messages waiting their turn.

All three live in the same `ChatBroker` stream, in the same SQLite
table, with the same ordering. When B is typed during A's in-flight,
B is *immediately* persisted (broker.publish → listModel.append) and
added to ChatService's `pendingMessages` simultaneously. The DB ends
up persistence-ordered, which doesn't match LLM-required ordering
when turns interleave.

The LLM conversation is *reconstructed* from chat history at every
`fetchCompletion` call (`ChatAgent.load(messages: history)`). That
reconstruction has accumulated workarounds:

- `enforceToolUseAdjacency` in `AnthropicRequestBuilder` reorders
  tool_use/tool_result pairs.
- `translate` in `ChatAgent` synthesizes orphan `function_output`
  fillers (line 340-356, for tool_use without tool_result, "iTerm2
  quit mid-tool-call").
- `translate` now also synthesizes orphan `function_call` fillers
  (the recent patch for bug 4).

Each of these is a partial fix for "DB persistence order is not LLM
conversation order."

## Proposed fix

Four pieces, in roughly increasing risk:

### Phase 1 — Eliminate silent cancellation

Make every cancellation that goes through the controller / conversation
fire the delegate callback. Specifically:

- In `AITermController.handle(event:)` for `.ground` state: if
  `delegate.busy` (or `cancellation != nil`), `.cancel` fires
  `aitermControllerDidCancelOutstandingRequest` and `.error` fires
  `didFailWithError`. `.ground` stops being a black hole.

- `AITermController.cancel()` (the silent one used by
  `AIConversation.cancel()`) gets renamed to `tearDown()` and its
  contract documented as "no callbacks; only for teardown after the
  outer caller has already collected what it needs." All current
  callers audited: the `prepare()` site is the suspect one — it's
  used to clean up before starting a fresh `conversation.complete`,
  but the silent overwrite of `delegate.completion = noop` orphans
  any genuinely-in-flight prior call. Replace with
  `cancelOutstandingOperation()` so the prior call's completion fires
  with `.failure(PendingCommandCanceled)` and ChatAgent's existing
  handler does the right thing.

- `AIConversation.stop()` was already changed to use
  `cancelOutstandingOperation()`. Remove the now-unused silent
  variant.

This is a tactical change, contained to two files
(`AITerm.swift` + `AIConversation.swift`). Low risk. Directly
eliminates the bug class described in (b).

### Phase 2 — Real state for function dispatch

Add `.awaitingFunctionOutput` to `AITermController.State`. The
transition from `parseStreamingResponse(final: true)` after a
`function_call` goes to that state instead of `.ground`. The
transition out happens when `doFunctionCall`'s impl.invoke
completion fires (either `.success(output)` → `.querySent` via a new
HTTP request, or `.failure(error)` → state machine processes the
error).

In `.awaitingFunctionOutput`, `.cancel` and `.error` both fire the
delegate. `.word` and `.webResponse` from the original HTTP request
are ignored (the request is done).

Eliminates bug class (c). Localized to `AITermController`. Combined
with Phase 1, makes the existing state of the code self-consistent.

### Phase 3 — Separate dispatch queue from chat persistence

User typing lands in `ChatService.pendingMessages` *only*. Persistence
to the DB and broker delivery happen at *dispatch time*, in the order
the LLM will see them.

Concretely:

- New API: `ChatService.acceptUserMessage(message)` adds to
  pendingMessages without broker.publish.
- UI (ChatViewController) calls this instead of `broker.publish` for
  user-typed messages.
- `startNextTurn` calls `broker.publish` on the dequeued message
  *first* (which persists + delivers to UI subscribers), then
  dispatches.

Consequences:

- `dropLast()` becomes well-defined ("the last DB row IS the in-flight
  user message," by construction).
- `translate` doesn't need orphan-synthesis hacks; the chat history
  matches LLM-required ordering by construction.
- `enforceToolUseAdjacency` becomes unnecessary.
- The `ChatClient.processRemoteCommandRequest` squelch-vs-persist
  conflict goes away: `ChatAgent` itself creates and persists the
  `.remoteCommandRequest` at the right moment in the LLM-history
  order, so there's no need for a separate "show it in UI" path that
  the broker processor was conflating with persistence.

Bigger change. Estimated week of work. Eliminates bug class (d).

### Phase 4 — Structured concurrency for turns

Each turn becomes a `Task`. `ChatAgent.fetchCompletion` becomes an
`async` function. Cancellation propagates through the task hierarchy.
The four-layer callback chain collapses into one async function
where each `await` is a natural cancellation point.

Eliminates bug class (a). Biggest change. Probably 2-3 weeks of
careful refactoring with the live harness tests as the safety net.
Would also unlock substantial readability improvements: the current
ChatAgent has 1000+ lines partly because of the manual completion
plumbing.

## Recommended sequencing

**Phase 1 + Phase 2 together.** They're contained to two files, they
eliminate the entire silent-cancel/dead-zone bug class, and they make
the rest of the code base self-consistent. Ship the live tests we
already have as the regression suite.

**Then Phase 3** if the data-model conflation continues to produce
bugs (it will).

**Phase 4** is the long-term cleanup. Don't block on it.

## What this document is for

These bugs are converging on the same root cause faster than we can
patch the symptoms. This document is the place where the next person
(or the same person on a different day) can read the *shape* of the
problem instead of just the catalog of individual patches.

If you find a fifth bug that looks like one of the patterns above
(silent cancel, state-machine dead zone, persistence-vs-LLM-order,
four-state-machines-out-of-sync), update this doc with the example
and resist the urge to ship another targeted patch.