From 008c6bd76527489316a46e65c618b38e5f637570 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 09:40:17 -0300 Subject: [PATCH 01/13] docs(specs): client-tool continuation correctness fixes Design for five defects found reviewing the shipped client-tool continuation stack (#782-#805). Four violate one unstated invariant: the server thread must never hold a client tool call without a result. Adds flush() to ClientToolsCapability so a settled result can be made durable without continuing the run, and maps all five fixes onto it. Co-Authored-By: Claude Opus 5 --- ...7-client-tool-continuation-fixes-design.md | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-07-client-tool-continuation-fixes-design.md diff --git a/docs/superpowers/specs/2026-08-07-client-tool-continuation-fixes-design.md b/docs/superpowers/specs/2026-08-07-client-tool-continuation-fixes-design.md new file mode 100644 index 000000000..b917521bf --- /dev/null +++ b/docs/superpowers/specs/2026-08-07-client-tool-continuation-fixes-design.md @@ -0,0 +1,121 @@ +# Client-Tool Continuation Correctness Fixes + +**Date:** 2026-08-07 +**Status:** Approved for implementation +**Supersedes nothing.** Builds on `2026-07-07-client-tool-continuation-architecture-design.md`, which shipped as PRs #782–#805. + +## Problem + +A code review of the shipped client-tool continuation stack found five defects. Four of them violate a single invariant that the architecture depends on but never states: + +> **The server thread must never hold a client tool call without a corresponding tool result.** + +When it is violated, the thread contains an `AIMessage(tool_calls=[…])` with no following `ToolMessage`. Most providers reject that history outright ("an assistant message with `tool_calls` must be followed by tool messages responding to each `tool_call_id`"), so the next user turn fails with a 400 and the thread is unusable. + +### Defect 1 — `followUp: false` corrupts LangGraph threads + +`client-tools-coordinator.ts` computes `hasFollowUp = calls.some(tc => registry[tc.name]?.followUp !== false)`. When every tool in a group is terminal, `hasFollowUp` is `false`, the `groupComplete && group.hasFollowUp` branch never fires, and only `settle()` is ever called — `resolve()` never is. + +In `libs/langgraph/src/lib/client-tools.ts`, `settle()` pushes onto a local `toolMessageBuffer` that is drained **only inside `resolve()`**. So the tool messages accumulate in a volatile browser array and never reach the server. + +AG-UI is unaffected: its `settle()` calls `source.addMessage(...)`, so the message lands in the outgoing list and rides along on the next `runAgent()`. This asymmetry is the root cause — `settle()`'s contract is under-specified, and the two adapters implemented different meanings. + +Existing tests miss it because `client-tools-coordinator.spec.ts` asserts `expect(resolve).not.toHaveBeenCalled()` against a **fake** capability, making the adapter-level consequence invisible. + +### Defect 2 — Aborted tool calls re-execute + +`runFunctionTool` returns without settling when `signal.aborted`. The call keeps `result === undefined` and never enters `resolvedIds`, so it stays in `pending()`. After the next run ends, the executor re-dispatches it. Without the opt-in execution guard (the default), a side-effecting handler runs a second time. + +### Defect 3 — The max-turns guard discards results + +`settleClientToolCall` starts with `if (!group.allowed) return;`. An `ask` tool's user-supplied answer is silently dropped, and the pending calls are never settled — the same dangling-tool-call corruption. + +### Defect 4 — `agent.stop` wrapper stacking + +`startClientToolExecutor` reassigns `agent.stop` and never restores it. `chat.component.ts`'s guard is keyed on the **coordinator** (`connected === coord`), so a coordinator swap against a long-lived (root-provided) agent stacks wrappers without bound. + +The patch itself is legitimate: the stop button lives in `chat-input.component.ts:169`, which has no coordinator reference, so intercepting `agent.stop` is the only available seam. The bug is that the patch is neither idempotent nor reversible. + +### Defect 5 — Postgres `tenant_id` is never enforced + +`postgres-client-tool-execution-store.ts` writes `tenant_id` on insert but omits it from `PRIMARY KEY (thread_id, tool_call_id)` and from every `WHERE` clause in `claim`/`lookup`. It provides no tenant isolation. + +## Design + +### The contract change + +`ClientToolsCapability` gains one method, making the three settlement verbs distinct: + +| Verb | Meaning | +|---|---| +| `settle(id, result)` | Record the result locally and stage it for durability. Never continues. | +| `flush()` | Make everything staged durable server-side. Never continues. | +| `resolve(id, result)` | `settle` + `flush` + continue, in a single run. | + +This is a **public API change**. Both adapters ship in lockstep at `0.0.x` and no backward compatibility is required, per explicit direction. New public exports require `npm run generate-api-docs` to be run and committed. + +### Adapter implementations + +**AG-UI** — `settle()` unchanged (still `addMessage`, already durable in the outgoing list). `flush()` is a no-op. `resolve()` unchanged. + +**LangGraph** — `settle()` buffers as today. `flush()` writes the entire buffer in one call: + +```ts +manager.updateState(threadId, { messages: buffer }) // no asNode +``` + +No `asNode` is passed, so `add_messages` appends and the graph's resume point is untouched. Because `updateState` is **optional** on `AgentTransport` (`agent.types.ts:229`), when it is absent `flush()` retains the buffer and `submit()` drains it at the existing `mergeClientTools` seam (`agent.fn.ts:455`). That degradation path is a strictly weaker but still-correct fallback. + +### Fix mapping + +Every defect below resolves to the same shape — **settle, then flush, without continuing**. + +1. **`followUp: false`** — on completion of an all-terminal group, the coordinator calls `flush()` instead of doing nothing. +2. **Abort** — settle with a cancelled error, flush, and `record()` the cancelled result to the execution guard. Entering `resolvedIds` also removes the call from `pending()`, which fixes re-execution. +3. **Max-turns** — settle every stopped call, then flush. Tools that produced a real result (an `ask` answer, a completed handler) settle with **that result**; only tools blocked *before executing* settle with a "continuation limit reached" error. Nothing is discarded. +4. **`agent.stop`** — keep the interception, add a `WeakSet` ownership marker so wrapping is idempotent, and restore the original in `destroyRef.onDestroy`. +5. **Postgres** — `tenant_id TEXT NOT NULL DEFAULT ''`, added to the primary key and to every `WHERE` in `claim`/`lookup`/`record`. + +### Error handling + +`flush()` failure must never lose data. The buffer is cleared **only on success**; on any failure the results stay staged and the next `flush()` or `submit()` drains them. + +- **409 conflict** — by construction no run is in flight, but a concurrent user submit can still race. Retry once, then fall back to submit-drain. Only the 409 is retried, and the retry is capped. +- **Empty buffer** — no-op, no round-trip. +- **Concurrent `flush()`** — guarded by an in-flight promise so the write never duplicates. +- **Missing `threadId`** — keep buffering rather than failing. + +### Behavior changes + +Two changes go beyond bug fixing and should be called out in the PR description: + +- Abort now **writes to the server** (a cancelled tool result) where it was previously a silent no-op. +- The max-turns guard now **writes to the server** where it previously dropped results. + +Both are required to maintain the invariant, but both are observable. + +## Testing + +### Deterministic local tests + +- **Coordinator** — all-terminal group produces N settles and one flush with `resolve` never called; mixed groups behave as today; max-turns preserves real results and errors only blocked calls. +- **Executor** — abort mid-handler settles cancelled, flushes, records to the guard, and the call does not reappear in `pending()`; the stop-patch is idempotent across two `connect()` calls and is restored on destroy. +- **LangGraph capability** — `flush()` issues exactly one `updateState`; the buffer clears only on success; both failure and missing-`updateState` fall through to submit-drain; an empty buffer makes no call. +- **AG-UI capability** — `flush()` is a no-op; `settle()` still calls `addMessage`. +- **Middleware** — two tenants sharing a `thread_id`/`tool_call_id` claim independently. + +### Live browser verification + +A unit test cannot prove the corruption is gone, because the failure is a provider rejection of persisted server state. `examples/chat` gains a terminal `followUp: false` view tool and a low `maxTurns` toggle, then, driven in Chrome against a real LLM: + +1. Trigger the terminal tool → no follow-up run fires, and `GET /threads/{id}/state` shows the `ToolMessage` present. +2. **Reload, then send another message** → no provider 400. This is the decisive test. +3. Start a slow client tool → Stop → send a new message → the tool does not re-execute. +4. Low `maxTurns` → the forced loop stops cleanly and the next message does not 400. + +Environment notes from prior runs: export only the API key rather than sourcing root `.env` (it sets an internal token that enables auth middleware and produces a misleading 401), and do not run e2e while a live serve holds the same ports. + +## Out of scope + +- Any change to the explicit continuation model itself. The protocol-visible `pending()`/`resolve()` contract stands. +- Migration tooling for the Postgres schema change; the table is recreated. From fa32291827d91483c107a212a7c3afad5c73a0ff Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 09:43:31 -0300 Subject: [PATCH 02/13] docs(plans): client-tool continuation fixes implementation plan Ten tasks covering all five defects, TDD per task, with a live browser verification gate whose decisive step is reload-then-continue. Co-Authored-By: Claude Opus 5 --- ...26-08-07-client-tool-continuation-fixes.md | 999 ++++++++++++++++++ 1 file changed, 999 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-07-client-tool-continuation-fixes.md diff --git a/docs/superpowers/plans/2026-08-07-client-tool-continuation-fixes.md b/docs/superpowers/plans/2026-08-07-client-tool-continuation-fixes.md new file mode 100644 index 000000000..ddac6b177 --- /dev/null +++ b/docs/superpowers/plans/2026-08-07-client-tool-continuation-fixes.md @@ -0,0 +1,999 @@ +# Client-Tool Continuation Correctness Fixes Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix five defects in the shipped client-tool continuation stack so the server thread never holds a client tool call without a result. + +**Architecture:** Add an optional `flush()` to `ClientToolsCapability` meaning "make staged results durable without continuing the run." AG-UI implements it as a no-op (its `settle()` already calls `addMessage`); LangGraph implements it as one batched `threads.updateState` write, falling back to draining into the next `submit()` when `updateState` is unavailable. The coordinator and executor then route the `followUp:false`, abort, and max-turns paths through settle-then-flush. + +**Tech Stack:** Angular 21 signals, Nx monorepo, Vitest, LangGraph SDK, `@ag-ui/client`, Postgres (tagged-template SQL). + +**Spec:** `docs/superpowers/specs/2026-08-07-client-tool-continuation-fixes-design.md` + +--- + +## File Structure + +**Modify:** +- `libs/chat/src/lib/client-tools/client-tools-capability.ts` — add `flush?()` to the contract +- `libs/chat/src/lib/client-tools/client-tools-coordinator.ts` — terminal-group flush (defect 1), max-turns settle+flush (defect 3) +- `libs/chat/src/lib/client-tools/client-tool-executor.ts` — abort settles+flushes+records (defect 2), idempotent stop patch (defect 4) +- `libs/chat/src/lib/client-tools/index.ts` — export new types +- `libs/ag-ui/src/lib/client-tools.ts` — no-op `flush()` +- `libs/langgraph/src/lib/client-tools.ts` — batched `flush()`, `drainToolMessages()` +- `libs/langgraph/src/lib/agent.fn.ts` — supply `persistFn`, drain buffer into `submit()` +- `libs/middleware/src/langgraph/postgres-client-tool-execution-store.ts` — tenant isolation (defect 5) +- `examples/chat/angular/src/app/client-tools.ts` — demo terminal tool + +**Test:** each module's adjacent `.spec.ts`. + +--- + +### Task 1: Add `flush()` to the capability contract, AG-UI no-op + +**Files:** +- Modify: `libs/chat/src/lib/client-tools/client-tools-capability.ts` +- Modify: `libs/ag-ui/src/lib/client-tools.ts` +- Test: `libs/ag-ui/src/lib/client-tools.spec.ts` + +- [ ] **Step 1: Write the failing test** + +Append to `libs/ag-ui/src/lib/client-tools.spec.ts` inside the top-level `describe`: + +```ts + it('exposes a no-op flush that does not start a run', async () => { + const source = { addMessage: vi.fn() }; + const store = makeStore(); + const continueRun = vi.fn(async () => undefined); + const cap = createClientToolsCapability(source, store, continueRun); + cap.setCatalog([{ name: 'get_weather', description: 'w', parameters: {} }]); + + cap.settle?.('t1', { ok: true, value: 'sunny' }); + await cap.flush?.(); + + expect(source.addMessage).toHaveBeenCalledTimes(1); + expect(continueRun).not.toHaveBeenCalled(); + }); +``` + +Use the same `makeStore()` helper the surrounding tests already use. If the file has no such helper, build the store inline exactly as the neighbouring `pending()` tests do. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `npx nx test ag-ui -- -t "no-op flush"` +Expected: FAIL — `cap.flush is not a function`. + +- [ ] **Step 3: Add `flush` to the contract** + +In `libs/chat/src/lib/client-tools/client-tools-capability.ts`, add below `settle?`: + +```ts + /** + * Make every result recorded via {@link settle} durable on the server + * WITHOUT continuing the run. No-op for adapters whose settle() is already + * durable. Adapters that buffer locally MUST clear their buffer only on a + * successful write, so a failure degrades to a later flush or submit. + */ + flush?(): void | Promise; +``` + +- [ ] **Step 4: Implement the AG-UI no-op** + +In `libs/ag-ui/src/lib/client-tools.ts`, add to the `clientTools` object literal after `settle`: + +```ts + // AG-UI's settle() already calls source.addMessage(), which places the + // ToolMessage in the outgoing message list. Nothing further is needed to + // make it durable — the next run carries it. + flush(): void { + /* no-op: settle() is already durable */ + }, +``` + +- [ ] **Step 5: Run test to verify it passes** + +Run: `npx nx test ag-ui -- -t "no-op flush"` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add libs/chat/src/lib/client-tools/client-tools-capability.ts libs/ag-ui/src/lib/client-tools.ts libs/ag-ui/src/lib/client-tools.spec.ts +git commit -m "feat(chat): add flush() to ClientToolsCapability" +``` + +--- + +### Task 2: LangGraph batched `flush()` with fallback drain + +**Files:** +- Modify: `libs/langgraph/src/lib/client-tools.ts` +- Modify: `libs/langgraph/src/lib/agent.fn.ts:414-457` +- Test: `libs/langgraph/src/lib/client-tools.spec.ts` + +- [ ] **Step 1: Write the failing tests** + +Append to `libs/langgraph/src/lib/client-tools.spec.ts`: + +```ts +describe('flush', () => { + const spec = { name: 'get_weather', description: 'w', parameters: {} }; + + function setup(persist?: (m: readonly unknown[]) => Promise) { + const submitFn = vi.fn(async () => undefined); + const applied: Array<[string, unknown]> = []; + const store = { + toolCalls: signal([] as readonly ToolCall[]), + isLoading: signal(false), + applyClientResult: (id: string, patch: unknown) => { applied.push([id, patch]); }, + }; + const cap = createClientToolsCapability(submitFn, store, persist); + cap.setCatalog([spec]); + return { cap, submitFn }; + } + + it('writes all buffered messages in a single persist call', async () => { + const persist = vi.fn(async () => undefined); + const { cap, submitFn } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + cap.settle?.('t2', { ok: true, value: 'b' }); + await cap.flush?.(); + + expect(persist).toHaveBeenCalledTimes(1); + expect(persist.mock.calls[0][0]).toEqual([ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + { type: 'tool', role: 'tool', tool_call_id: 't2', content: 'b' }, + ]); + expect(submitFn).not.toHaveBeenCalled(); + }); + + it('makes no call when the buffer is empty', async () => { + const persist = vi.fn(async () => undefined); + const { cap } = setup(persist); + await cap.flush?.(); + expect(persist).not.toHaveBeenCalled(); + }); + + it('keeps the buffer when persist fails so a later drain retries', async () => { + const persist = vi.fn(async () => { throw new Error('boom'); }); + const { cap } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + await cap.flush?.(); + + expect(cap.drainToolMessages()).toEqual([ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + ]); + }); + + it('keeps the buffer when no persist function is supplied', async () => { + const { cap } = setup(undefined); + cap.settle?.('t1', { ok: true, value: 'a' }); + await cap.flush?.(); + expect(cap.drainToolMessages()).toHaveLength(1); + }); + + it('drainToolMessages empties the buffer', async () => { + const { cap } = setup(undefined); + cap.settle?.('t1', { ok: true, value: 'a' }); + cap.drainToolMessages(); + expect(cap.drainToolMessages()).toEqual([]); + }); +}); +``` + +Ensure `signal` from `@angular/core` and `ToolCall` from `@threadplane/chat` are imported at the top of the spec; add them if absent. + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `npx nx test langgraph -- -t "flush"` +Expected: FAIL — `cap.flush is not a function`. + +- [ ] **Step 3: Implement in `libs/langgraph/src/lib/client-tools.ts`** + +Add the exported payload type above `createClientToolsCapability`: + +```ts +/** Wire shape for a settled client-tool result awaiting durability. */ +export interface BufferedToolMessage { + readonly type: 'tool'; + readonly role: 'tool'; + readonly tool_call_id: string; + readonly content: string; +} + +/** Writes settled tool messages into server thread state without starting a run. */ +export type PersistToolMessagesFn = ( + messages: readonly BufferedToolMessage[], +) => Promise; +``` + +Change the factory signature to accept the persister: + +```ts +export function createClientToolsCapability( + submitFn: SubmitFn, + store: ClientToolsStore, + persistFn?: PersistToolMessagesFn, +): ClientToolsCapability & { + catalog: Signal; + drainToolMessages(): BufferedToolMessage[]; +} { +``` + +Add an in-flight guard beside the buffer declaration: + +```ts + let flushInFlight: Promise | undefined; +``` + +Add these members to the returned `capability` object: + +```ts + /** Remove and return every buffered tool message. */ + drainToolMessages(): BufferedToolMessage[] { + const drained = [...toolMessageBuffer]; + toolMessageBuffer.length = 0; + return drained; + }, + + flush(): Promise { + if (flushInFlight) return flushInFlight; + if (toolMessageBuffer.length === 0) return Promise.resolve(); + if (!persistFn) return Promise.resolve(); + + // Snapshot first: the buffer is cleared ONLY after a successful write, so + // a failure leaves the results staged for the next flush or submit drain. + const batch = [...toolMessageBuffer]; + flushInFlight = persistFn(batch) + .then(() => { + toolMessageBuffer.splice(0, batch.length); + }) + .catch((err: unknown) => { + console.warn( + `Client tool flush failed; ${batch.length} result(s) remain staged for the next run.`, + err, + ); + }) + .finally(() => { + flushInFlight = undefined; + }); + return flushInFlight; + }, +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `npx nx test langgraph -- -t "flush"` +Expected: PASS (5 tests) + +- [ ] **Step 5: Wire the persister and submit-drain in `agent.fn.ts`** + +Replace the `createClientToolsCapability(...)` call at `libs/langgraph/src/lib/agent.fn.ts:419-427` with: + +```ts + const clientToolsCap = createClientToolsCapability( + (payload, opts) => manager.submit(payload, opts), + { + toolCalls: toolCallsNeutral, + isLoading, + applyClientResult: (id, patch) => + clientResultOverrides.update((m) => new Map(m).set(id, patch)), + }, + // Durable write without a run. Absent asNode: add_messages appends and the + // graph's resume point is untouched. Undefined when the transport has no + // updateState — flush() then keeps the buffer for the submit drain below. + manager.updateState + ? async (messages) => { + const threadId = lastThreadId; + if (!threadId) throw new Error('no threadId for client tool flush'); + await manager.updateState!( + threadId, + { messages: [...messages] }, + new AbortController().signal, + ); + } + : undefined, + ); +``` + +Then replace the payload construction inside `submit` at line 455 with: + +```ts + // Drain any results settled but not yet made durable (flush unavailable + // or a prior flush failed) so they ride along with this run. + const staged = clientToolsCap.drainToolMessages(); + const withStaged = staged.length > 0 + ? mergeStagedToolMessages(request.payload, staged) + : request.payload; + const payload = mergeClientTools(withStaged, clientToolsCap.catalog()); + return manager.submit(payload, request.options); +``` + +Add this helper to `libs/langgraph/src/lib/client-tools.ts` and export it: + +```ts +/** + * Prepend staged tool messages to a run payload's message list. + * + * Mirrors mergeClientTools: a null payload signals a no-input resume and must + * stay null, so staged messages cannot ride along and are left buffered. + */ +export function mergeStagedToolMessages( + payload: unknown, + staged: readonly BufferedToolMessage[], +): unknown { + if (staged.length === 0) return payload; + if (payload === null || payload === undefined) return payload; + if (typeof payload !== 'object' || Array.isArray(payload)) return payload; + const record = payload as Record; + const existing = Array.isArray(record['messages']) ? record['messages'] : []; + return { ...record, messages: [...staged, ...existing] }; +} +``` + +Import `mergeStagedToolMessages` alongside `mergeClientTools` at `agent.fn.ts:70`. + +**Note:** when `payload` is null the staged messages are returned to the buffer by design — `drainToolMessages()` already emptied it, so guard the drain: + +```ts + const staged = request.payload === null || request.payload === undefined + ? [] + : clientToolsCap.drainToolMessages(); +``` + +Use that guarded form instead of the unguarded drain above. + +- [ ] **Step 6: Add the submit-drain test** + +Append to `libs/langgraph/src/lib/client-tools.spec.ts`: + +```ts +describe('mergeStagedToolMessages', () => { + const staged = [ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + ] as const; + + it('prepends staged messages ahead of the payload messages', () => { + const out = mergeStagedToolMessages({ messages: [{ type: 'human', content: 'hi' }] }, staged); + expect(out).toEqual({ + messages: [ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + { type: 'human', content: 'hi' }, + ], + }); + }); + + it('leaves a null payload unchanged', () => { + expect(mergeStagedToolMessages(null, staged)).toBeNull(); + }); + + it('returns the payload unchanged when nothing is staged', () => { + const payload = { messages: [] }; + expect(mergeStagedToolMessages(payload, [])).toBe(payload); + }); +}); +``` + +Add `mergeStagedToolMessages` to the spec's import from `./client-tools`. + +- [ ] **Step 7: Run the full langgraph suite** + +Run: `npx nx test langgraph` +Expected: PASS + +- [ ] **Step 8: Commit** + +```bash +git add libs/langgraph/src/lib/client-tools.ts libs/langgraph/src/lib/client-tools.spec.ts libs/langgraph/src/lib/agent.fn.ts +git commit -m "feat(langgraph): batched flush() with submit-drain fallback" +``` + +--- + +### Task 3: Coordinator flushes terminal groups (defect 1) + +**Files:** +- Modify: `libs/chat/src/lib/client-tools/client-tools-coordinator.ts:149-176` +- Test: `libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts` + +- [ ] **Step 1: Write the failing test** + +Replace the existing `it('settles a fully-terminal group without resolving', ...)` test body's assertions by appending a new test after it: + +```ts + it('flushes a fully-terminal group so results reach the server', () => { + const registry = tools({ + terminal_card: view( + 'Show terminal card', + z.object({ city: z.string() }), + FakeViewComponent as never, + { followUp: false }, + ), + }); + const { pending, settle, resolve, flush, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + pending.set([{ id: 'v1', name: 'terminal_card', args: { city: 'LA' }, status: 'running' }]); + TestBed.flushEffects(); + + expect(settle).toHaveBeenCalledWith('v1', { ok: true, value: { shown: true } }); + expect(flush).toHaveBeenCalledTimes(1); + expect(resolve).not.toHaveBeenCalled(); + }); +``` + +Update `makeFakeCapability()` in this spec so the returned capability includes `flush: vi.fn()` and the helper returns it alongside `settle`/`resolve`. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `npx nx test chat -- -t "flushes a fully-terminal group"` +Expected: FAIL — `flush` called 0 times. + +- [ ] **Step 3: Implement** + +In `client-tools-coordinator.ts`, replace the tail of `settleClientToolCall` (the block from `if (groupComplete && group.hasFollowUp)` to the end) with: + +```ts + if (groupComplete && group.hasFollowUp) { + cap.resolve(tc.id, result); + currentGroup = undefined; + return; + } + + cap.settle(tc.id, result); + if (groupComplete) { + // Terminal group: nothing will continue the run, so make the settled + // results durable ourselves or the server keeps an unanswered tool call. + flushSettledResults(cap); + currentGroup = undefined; + } +``` + +Add above `settleClientToolCall`: + +```ts + function flushSettledResults(cap: ClientToolsCapability): void { + if (!cap.flush) { + console.warn( + 'Client tool group settled with no follow-up, but the agent capability does not implement flush(); results may not reach the server.', + ); + return; + } + void Promise.resolve(cap.flush()).catch((err: unknown) => { + console.error('Client tool flush failed', err); + }); + } +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `npx nx test chat -- -t "flushes a fully-terminal group"` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add libs/chat/src/lib/client-tools/client-tools-coordinator.ts libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts +git commit -m "fix(chat): flush terminal client-tool groups to the server" +``` + +--- + +### Task 4: Max-turns settles and flushes without discarding results (defect 3) + +**Files:** +- Modify: `libs/chat/src/lib/client-tools/client-tools-coordinator.ts:139-176` +- Test: `libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts` + +- [ ] **Step 1: Write the failing test** + +```ts + it('settles blocked calls with a limit error and preserves real ask results', () => { + const registry = tools({ + confirm: ask('Confirm', z.object({ q: z.string() }), FakeAskComponent as never), + }); + const { pending, settle, resolve, flush, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry, { + continuationPolicy: { maxTurns: 1 }, + }); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + // Turn 1 consumes the single allowed continuation. + pending.set([{ id: 'a1', name: 'confirm', args: { q: 'x' }, status: 'running' }]); + TestBed.flushEffects(); + coordinator.handleRenderEvent(agent, { + type: 'result', elementKey: 'confirm', value: { confirmed: true }, + } as never); + + settle.mockClear(); + resolve.mockClear(); + flush.mockClear(); + + // Turn 2 exceeds maxTurns: the user's answer must still be recorded. + pending.set([{ id: 'a2', name: 'confirm', args: { q: 'y' }, status: 'running' }]); + TestBed.flushEffects(); + coordinator.handleRenderEvent(agent, { + type: 'result', elementKey: 'confirm', value: { confirmed: false }, + } as never); + + expect(settle).toHaveBeenCalledWith('a2', { ok: true, value: { confirmed: false } }); + expect(flush).toHaveBeenCalled(); + expect(resolve).not.toHaveBeenCalled(); + }); +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `npx nx test chat -- -t "preserves real ask results"` +Expected: FAIL — `settle` not called (current code returns early on `!group.allowed`). + +- [ ] **Step 3: Implement** + +In `settleClientToolCall`, replace `if (!group.allowed) return;` with: + +```ts + // Over the continuation limit: still record the result so the server never + // keeps an unanswered tool call, but never continue the run. + if (!group.allowed) { + if (group.settledIds.has(tc.id)) return; + group.settledIds.add(tc.id); + if (cap.settle) { + cap.settle(tc.id, result); + flushSettledResults(cap); + } + return; + } +``` + +Then make blocked function tools settle with an explicit limit error. In `connect()`, change the executor wiring so a blocked call is settled rather than silently skipped — replace the `shouldExecuteToolCall` option with: + +```ts + shouldExecuteToolCall: (tc) => { + if (shouldHandleClientToolCall(agent, cap, tc)) return true; + // Blocked before executing: record why, so the model sees a reason + // instead of an unanswered tool call. + settleClientToolCall(cap, agent, tc, { + ok: false, + error: `client tool continuation limit reached; ${tc.name} was not executed`, + }); + return false; + }, +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `npx nx test chat -- -t "preserves real ask results"` +Expected: PASS + +- [ ] **Step 5: Run the full coordinator spec** + +Run: `npx nx test chat -- -t "createClientToolsCoordinator"` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add libs/chat/src/lib/client-tools/client-tools-coordinator.ts libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts +git commit -m "fix(chat): record results when the continuation limit stops a group" +``` + +--- + +### Task 5: Abort settles, flushes, and records (defect 2) + +**Files:** +- Modify: `libs/chat/src/lib/client-tools/client-tool-executor.ts:80-157` +- Test: `libs/chat/src/lib/client-tools/client-tool-executor.spec.ts` + +- [ ] **Step 1: Write the failing test** + +```ts + it('settles an aborted handler so it cannot re-execute', async () => { + const settled: Array<[string, unknown]> = []; + let release!: () => void; + const registry = tools({ + slow: action('Slow', z.object({}), async () => { + await new Promise((r) => { release = r; }); + return 'done'; + }), + }); + const pending = signal([ + { id: 's1', name: 'slow', args: {}, status: 'running' }, + ] as readonly ToolCall[]); + const agent = makeAgentWithPending(pending, (id, result) => settled.push([id, result])); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry, { + settleToolCall: (tc, result) => settled.push([tc.id, result]), + }); + }); + TestBed.flushEffects(); + + await agent.stop(); + release(); + await drainMicrotasks(); + + expect(settled).toHaveLength(1); + expect(settled[0][0]).toBe('s1'); + expect((settled[0][1] as { ok: boolean }).ok).toBe(false); + }); +``` + +Build `makeAgentWithPending` following the existing helpers in this spec file; it must expose `clientTools.pending`, a `resolve`/`settle` pair, and a real `stop()` returning a promise. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `npx nx test chat -- -t "cannot re-execute"` +Expected: FAIL — `settled` is empty (aborted path returns without settling). + +- [ ] **Step 3: Add the cancelled-result helper** + +In `libs/chat/src/lib/client-tools/client-tool-execution-guard.ts`, add: + +```ts +/** Result recorded when the user stops a run while a client tool is running. */ +export function cancelledClientToolResult(toolCallId: string): ClientToolResult { + return { + ok: false, + error: `client tool execution cancelled before completion: ${toolCallId}`, + }; +} +``` + +Export it from `libs/chat/src/lib/client-tools/index.ts` alongside the other guard helpers. + +- [ ] **Step 4: Implement the abort path** + +In `client-tool-executor.ts`, replace every bare `if (signal.aborted) return;` and `if (!signal.aborted) settleToolCall(...)` guard in `runFunctionTool` and `recordOrResolveGuardFailure` so an abort settles instead of dropping. Concretely, in `runFunctionTool` replace the no-guard branch: + +```ts + if (!executionGuard || !shouldClaimBeforeExecute(def)) { + const result = await executeFunctionTool(def, rawArgs, { signal }); + settleToolCall(toolCall, signal.aborted ? cancelledClientToolResult(toolCallId) : result); + return; + } +``` + +the post-claim branch: + +```ts + if (claim === 'claimed') { + const result = await executeFunctionTool(def, rawArgs, { signal }); + const finalResult = signal.aborted ? cancelledClientToolResult(toolCallId) : result; + await recordOrResolveGuardFailure( + executionGuard, key, finalResult, toolCall, toolCallId, settleToolCall, + ); + return; + } +``` + +and the pre-claim abort check: + +```ts + if (signal.aborted) { + settleToolCall(toolCall, cancelledClientToolResult(toolCallId)); + return; + } +``` + +In `recordOrResolveGuardFailure`, drop the `signal` parameter and the `!signal.aborted` conditions so the guard is always recorded: + +```ts +async function recordOrResolveGuardFailure( + executionGuard: ClientToolExecutionGuard, + key: ClientToolExecutionKey, + result: ClientToolResult, + toolCall: ToolCall, + toolCallId: string, + settleToolCall: (toolCall: ToolCall, result: ClientToolResult) => void, +): Promise { + try { + await executionGuard.store.record(key, result); + } catch (err) { + settleToolCall(toolCall, clientToolGuardFailureResult(toolCallId, err)); + return; + } + settleToolCall(toolCall, result); +} +``` + +Update both call sites to drop the `signal` argument. Import `cancelledClientToolResult`. + +- [ ] **Step 5: Run test to verify it passes** + +Run: `npx nx test chat -- -t "cannot re-execute"` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add libs/chat/src/lib/client-tools/client-tool-executor.ts libs/chat/src/lib/client-tools/client-tool-execution-guard.ts libs/chat/src/lib/client-tools/index.ts libs/chat/src/lib/client-tools/client-tool-executor.spec.ts +git commit -m "fix(chat): settle aborted client tools instead of leaving them pending" +``` + +--- + +### Task 6: Idempotent, reversible `agent.stop` patch (defect 4) + +**Files:** +- Modify: `libs/chat/src/lib/client-tools/client-tool-executor.ts:37-50` +- Test: `libs/chat/src/lib/client-tools/client-tool-executor.spec.ts` + +- [ ] **Step 1: Write the failing test** + +```ts + it('wraps agent.stop once across repeated executor starts', () => { + const registry = tools({ noop: action('n', z.object({}), async () => 'x') }); + const pending = signal([] as readonly ToolCall[]); + const agent = makeAgentWithPending(pending, () => undefined); + const original = agent.stop; + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + startClientToolExecutor(agent, registry); + }); + + expect(agent.stop).not.toBe(original); + const afterFirstWrap = agent.stop; + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + }); + expect(agent.stop).toBe(afterFirstWrap); + }); +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `npx nx test chat -- -t "wraps agent.stop once"` +Expected: FAIL — each start installs a new wrapper. + +- [ ] **Step 3: Implement** + +In `client-tool-executor.ts`, add above `startClientToolExecutor`: + +```ts +/** Agents whose stop() this module has already wrapped. */ +const patchedAgents = new WeakSet(); +``` + +Replace the patch block with: + +```ts + // The stop button lives in chat-input, which has no coordinator reference, + // so wrapping agent.stop is the only interception seam. Wrap at most once + // per agent and restore on destroy — agents often outlive components. + if (!patchedAgents.has(agent)) { + patchedAgents.add(agent); + const originalStop = agent.stop.bind(agent); + agent.stop = async (): Promise => { + abortAll(); + await originalStop(); + }; + destroyRef.onDestroy(() => { + agent.stop = originalStop; + patchedAgents.delete(agent); + }); + } + destroyRef.onDestroy(abortAll); +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `npx nx test chat -- -t "wraps agent.stop once"` +Expected: PASS + +- [ ] **Step 5: Run the full chat client-tools suite** + +Run: `npx nx test chat` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add libs/chat/src/lib/client-tools/client-tool-executor.ts libs/chat/src/lib/client-tools/client-tool-executor.spec.ts +git commit -m "fix(chat): wrap agent.stop once and restore it on destroy" +``` + +--- + +### Task 7: Postgres tenant isolation (defect 5) + +**Files:** +- Modify: `libs/middleware/src/langgraph/postgres-client-tool-execution-store.ts` +- Test: `libs/middleware/src/postgres-client-tool-execution-store.spec.ts` + +- [ ] **Step 1: Write the failing test** + +```ts +it('isolates identical thread/tool ids across tenants', async () => { + const rows: Array> = []; + const sql = vi.fn(async (strings: TemplateStringsArray, ...values: unknown[]) => { + rows.push({ text: strings.join('?'), values }); + return []; + }) as never; + + const storeA = createPostgresClientToolExecutionStore(sql, { tenantId: 'a' }); + await storeA.lookup('thread-1', ['tc-1']); + + const last = rows[rows.length - 1]; + expect(String(last['text'])).toContain('tenant_id'); + expect(last['values']).toContain('a'); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `npx nx test middleware -- -t "isolates identical thread"` +Expected: FAIL — `lookup` emits no `tenant_id` predicate. + +- [ ] **Step 3: Implement** + +Replace the schema constant: + +```ts +export const THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA = ` +CREATE TABLE IF NOT EXISTS threadplane_client_tool_executions ( + tenant_id text NOT NULL DEFAULT '', + thread_id text NOT NULL, + tool_call_id text NOT NULL, + status text NOT NULL, + result jsonb, + created_at timestamptz NOT NULL DEFAULT now(), + updated_at timestamptz NOT NULL DEFAULT now(), + PRIMARY KEY (tenant_id, thread_id, tool_call_id) +); +`; +``` + +Change the tenant default to a non-null empty string: + +```ts + const tenantId = opts.tenantId ?? ''; +``` + +Add `tenant_id` to every conflict target and predicate: + +```ts + ON CONFLICT (tenant_id, thread_id, tool_call_id) DO NOTHING +``` + +```ts + WHERE tenant_id = ${tenantId} + AND thread_id = ${key.threadId} + AND tool_call_id = ${key.toolCallId} +``` + +```ts + WHERE tenant_id = ${tenantId} + AND thread_id = ${threadId} + AND tool_call_id = ANY(${[...toolCallIds]}) +``` + +and in `record`, update the conflict target to `(tenant_id, thread_id, tool_call_id)`. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `npx nx test middleware -- -t "isolates identical thread"` +Expected: PASS + +- [ ] **Step 5: Run the full middleware suite** + +Run: `npx nx test middleware` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add libs/middleware/src/langgraph/postgres-client-tool-execution-store.ts libs/middleware/src/postgres-client-tool-execution-store.spec.ts +git commit -m "fix(middleware): enforce tenant isolation in the execution store" +``` + +--- + +### Task 8: Demo terminal tool in examples/chat + +**Files:** +- Modify: `examples/chat/angular/src/app/client-tools.ts` + +- [ ] **Step 1: Read the existing registry** + +Read `examples/chat/angular/src/app/client-tools.ts` in full and match its import style, schema conventions, and component patterns before adding anything. + +- [ ] **Step 2: Add a terminal view tool** + +Add one `view` tool declared with `{ followUp: false }` that renders a short trip-summary card from the itinerary the demo already models. Reuse an existing card component if one fits; otherwise create a sibling standalone component following `day-card.component.ts`'s structure exactly (signal inputs, encapsulated CSS on `--ds-*` tokens — utility classes do not compile in example apps). + +Register it in the exported `tools({ ... })` map with a name the graph can call, e.g. `show_trip_summary`. + +- [ ] **Step 3: Verify the example builds** + +Run: `npx nx build examples-chat --configuration=production` +Expected: SUCCESS. (Production config carries a bundle budget that dev builds do not.) + +If the project name differs, discover it with `npx nx show projects | grep chat`. + +- [ ] **Step 4: Commit** + +```bash +git add examples/chat/angular/src/app +git commit -m "feat(examples): add terminal client tool to the chat demo" +``` + +--- + +### Task 9: Regenerate API docs and run the full verification gate + +**Files:** +- Modify: generated API docs + +- [ ] **Step 1: Regenerate** + +Run: `npm run generate-api-docs` + +- [ ] **Step 2: Lint the touched projects** + +Run: `npx nx run-many -t lint -p chat ag-ui langgraph middleware 2>&1 | grep -cE ' error '` +Expected: `0`. Warnings are tolerated by CI; errors are not. + +- [ ] **Step 3: Test the touched projects** + +Run: `npx nx run-many -t test -p chat ag-ui langgraph middleware` +Expected: PASS + +- [ ] **Step 4: Commit** + +```bash +git add -A +git commit -m "docs: regenerate API docs for flush() capability" +``` + +--- + +### Task 10: Live browser verification + +**Files:** none (verification only) + +- [ ] **Step 1: Serve the demo with a real key** + +Export **only** the model API key. Sourcing root `.env` also exports an internal token that switches on auth middleware and produces a misleading 401. Confirm no dev server already holds the ports before starting. + +- [ ] **Step 2: Terminal tool does not continue** + +Drive the chat in Chrome until the model calls `show_trip_summary`. Confirm the card renders and **no** follow-up assistant turn begins. Then fetch `GET /threads/{threadId}/state` and confirm a `ToolMessage` with the matching `tool_call_id` is present. + +- [ ] **Step 3: The decisive test — reload and continue** + +Reload the page, then send another message. Expected: a normal assistant reply, **no** provider 400. Before this change this step fails. + +- [ ] **Step 4: Abort does not re-execute** + +Trigger a slow client tool, press Stop mid-execution, then send a new message. Confirm the tool does not run a second time and the thread has no unanswered tool call. + +- [ ] **Step 5: Max-turns stops cleanly** + +Configure a low `maxTurns`, drive a loop until the guard fires, then send another message. Expected: clean stop, no 400. + +- [ ] **Step 6: Record the evidence** + +Capture console output and the thread-state JSON for the PR description. Do not claim any step passed without the observed output. + +--- + +## Self-Review + +**Spec coverage:** Defect 1 → Tasks 1–3. Defect 2 → Task 5. Defect 3 → Task 4. Defect 4 → Task 6. Defect 5 → Task 7. `flush()` contract → Task 1. LangGraph fallback drain → Task 2. Behavior-change callouts → PR description (Task 10 evidence). Live verification → Task 10. Demo surface → Task 8. + +**Type consistency:** `flush?()` is declared optional in Task 1 and every call site guards on `cap.flush`. `BufferedToolMessage`, `PersistToolMessagesFn`, `mergeStagedToolMessages`, and `drainToolMessages()` are defined in Task 2 and used consistently in Tasks 2 and 3. `cancelledClientToolResult` is defined in Task 5 Step 3 before its Step 4 use. `recordOrResolveGuardFailure`'s signature loses `signal` in Task 5 and both call sites are updated in the same step. + +**Known risk:** Task 4's `shouldExecuteToolCall` change makes a predicate perform a side effect. Verify in review that it is invoked exactly once per blocked call; if the effect re-runs, move the settle into the executor loop instead. From 7e05393144af395e15ee4a97e3a8df609fd7118e Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 09:48:13 -0300 Subject: [PATCH 03/13] feat(chat): add flush() to ClientToolsCapability --- libs/ag-ui/src/lib/client-tools.spec.ts | 19 +++++++++++++++++++ libs/ag-ui/src/lib/client-tools.ts | 9 +++++++++ .../client-tools/client-tools-capability.ts | 7 +++++++ 3 files changed, 35 insertions(+) diff --git a/libs/ag-ui/src/lib/client-tools.spec.ts b/libs/ag-ui/src/lib/client-tools.spec.ts index 500901dd3..4cf242549 100644 --- a/libs/ag-ui/src/lib/client-tools.spec.ts +++ b/libs/ag-ui/src/lib/client-tools.spec.ts @@ -245,6 +245,25 @@ describe('createClientToolsCapability', () => { ).toolCallId)).toEqual(['c1', 'c2']); }); + // ---- flush ----------------------------------------------------------------- + + it('exposes a no-op flush that does not start a run', async () => { + const source = makeSource(); + const store = makeStore(); + const cap = createClientToolsCapability(source, store); + cap.setCatalog([WEATHER_SPEC]); + + // Assert presence explicitly: `cap.flush?.()` short-circuits when the + // member is missing, so the optional call alone cannot fail the test. + expect(typeof cap.flush).toBe('function'); + + cap.settle?.('t1', { ok: true, value: 'sunny' }); + await cap.flush?.(); + + expect(source.addMessage).toHaveBeenCalledTimes(1); + expect(source.continueRun).not.toHaveBeenCalled(); + }); + // ---- resolve — error result ------------------------------------------------ it('resolve(error) writes { error } result + error + status=error onto the store tool call', () => { diff --git a/libs/ag-ui/src/lib/client-tools.ts b/libs/ag-ui/src/lib/client-tools.ts index 0302a000a..dcd115c4f 100644 --- a/libs/ag-ui/src/lib/client-tools.ts +++ b/libs/ag-ui/src/lib/client-tools.ts @@ -46,6 +46,8 @@ function safeStringify(v: unknown): string { * ask component re-renders with its emitted value as props and can branch to * a frozen state), and adds a ToolMessage via source.addMessage without * starting a run. + * - flush(): no-op. settle() already made the result durable by adding it to + * the source's message list, so there is nothing left to write. * - resolve(id, result): settles the result, then requests a continuation * through the adapter-owned run gateway. Any ToolMessages previously * settled into the source are flushed by that single run. @@ -126,6 +128,13 @@ export function createClientToolsCapability( settleResult(id, result); }, + // AG-UI's settle() already calls source.addMessage(), which places the + // ToolMessage in the outgoing message list. Nothing further is needed to + // make it durable — the next run carries it. + flush(): void { + /* no-op: settle() is already durable */ + }, + resolve(id: string, result: ClientToolResult): void { settleResult(id, result); void continueRun(); diff --git a/libs/chat/src/lib/client-tools/client-tools-capability.ts b/libs/chat/src/lib/client-tools/client-tools-capability.ts index c6dee3f9b..8a391a8bc 100644 --- a/libs/chat/src/lib/client-tools/client-tools-capability.ts +++ b/libs/chat/src/lib/client-tools/client-tools-capability.ts @@ -21,6 +21,13 @@ export interface ClientToolsCapability { readonly pending: Signal; /** Record a client tool's result without continuing the run. */ settle?(toolCallId: string, result: ClientToolResult): void; + /** + * Make every result recorded via {@link settle} durable on the server + * WITHOUT continuing the run. No-op for adapters whose settle() is already + * durable. Adapters that buffer locally MUST clear their buffer only on a + * successful write, so a failure degrades to a later flush or submit. + */ + flush?(): void | Promise; /** Return a client tool's result (or error) and continue the run. */ resolve(toolCallId: string, result: ClientToolResult): void; } From 61d187fd22f457cf20cfd4280e8eb744c7d945fb Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 09:51:48 -0300 Subject: [PATCH 04/13] feat(langgraph): batched flush() with submit-drain fallback --- libs/langgraph/src/lib/agent.fn.ts | 39 ++++++- libs/langgraph/src/lib/client-tools.spec.ts | 110 +++++++++++++++++++- libs/langgraph/src/lib/client-tools.ts | 81 +++++++++++++- 3 files changed, 223 insertions(+), 7 deletions(-) diff --git a/libs/langgraph/src/lib/agent.fn.ts b/libs/langgraph/src/lib/agent.fn.ts index fb5b67087..6bb164e1b 100644 --- a/libs/langgraph/src/lib/agent.fn.ts +++ b/libs/langgraph/src/lib/agent.fn.ts @@ -67,7 +67,11 @@ import { createStreamManagerBridge } from './internals/stream-manager.bridge'; import { LANGGRAPH_CLIENT_OPTIONS, resolveClientOptions } from './client/client-options'; import { buildBranchTree } from './internals/branch-tree'; import { extractCitations } from './internals/extract-citations'; -import { createClientToolsCapability, mergeClientTools } from './client-tools'; +import { + createClientToolsCapability, + mergeClientTools, + mergeStagedToolMessages, +} from './client-tools'; import type { ClientToolResultPatch } from './client-tools'; /** @@ -416,6 +420,14 @@ export function agent< // follow-up runs (resolve) without going through the full submit() wrapper. // The catalog is injected into every outbound payload via mergeClientTools() // in the submit wrapper below and in the resolve path inside the capability. + // + // flush() needs a durable write that does NOT start a run. The bridge's + // updateState() silently no-ops when the transport has no updateState, so + // only supply a persist function when the effective transport supports it — + // an omitted transport means the bridge builds a FetchStreamTransport, which + // does. When persistFn is undefined, flush() keeps the buffer and the submit + // wrapper below drains it into the next run instead. + const canPersistToolMessages = !transport || typeof transport.updateState === 'function'; const clientToolsCap = createClientToolsCapability( (payload, opts) => manager.submit(payload, opts), { @@ -424,6 +436,18 @@ export function agent< applyClientResult: (id, patch) => clientResultOverrides.update((m) => new Map(m).set(id, patch)), }, + canPersistToolMessages + ? async (messages) => { + // Throw rather than let the bridge no-op: without a thread there is + // nothing to write to, and flush() must keep the buffer staged. + if (!manager.currentThreadId) { + throw new Error('no threadId for client tool flush'); + } + // No asNode: add_messages appends the ToolMessages and the graph's + // resume point is left untouched, so no run is started. + await manager.updateState({ messages: [...messages] }); + } + : undefined, ); return { @@ -452,7 +476,18 @@ export function agent< // Thread the client-tools catalog into every outbound payload so the // backend middleware can merge them into the model's tool list. Null // payloads (regenerate re-runs, command resumes) are left unchanged. - const payload = mergeClientTools(request.payload, clientToolsCap.catalog()); + // + // Drain any results settled but not yet made durable (flush unavailable + // or a prior flush failed) so they ride along with this run. A null + // payload cannot carry them, so leave the buffer alone in that case + // rather than silently discarding the staged results. + const staged = request.payload === null || request.payload === undefined + ? [] + : clientToolsCap.drainToolMessages(); + const withStaged = staged.length > 0 + ? mergeStagedToolMessages(request.payload, staged) + : request.payload; + const payload = mergeClientTools(withStaged, clientToolsCap.catalog()); return manager.submit(payload, request.options); }, stop: () => manager.stop(), diff --git a/libs/langgraph/src/lib/client-tools.spec.ts b/libs/langgraph/src/lib/client-tools.spec.ts index 7fb3e1349..3d3bb41e0 100644 --- a/libs/langgraph/src/lib/client-tools.spec.ts +++ b/libs/langgraph/src/lib/client-tools.spec.ts @@ -2,8 +2,16 @@ import { describe, it, expect, vi } from 'vitest'; import { signal } from '@angular/core'; import type { ToolCall } from '@threadplane/chat'; -import { createClientToolsCapability, mergeClientTools } from './client-tools'; -import type { ClientToolsStore, SubmitFn } from './client-tools'; +import { + createClientToolsCapability, + mergeClientTools, + mergeStagedToolMessages, +} from './client-tools'; +import type { + ClientToolsStore, + PersistToolMessagesFn, + SubmitFn, +} from './client-tools'; // ─── Fakes ─────────────────────────────────────────────────────────────────── @@ -384,3 +392,101 @@ describe('createClientToolsCapability', () => { expect(result).toBe(humanPayload); }); }); + +// ─── flush — durable write without continuing the run ──────────────────────── + +describe('flush', () => { + const spec = { name: 'get_weather', description: 'w', parameters: {} }; + + function setup(persist?: (m: readonly unknown[]) => Promise) { + const submitFn = vi.fn(async () => undefined); + const store = { + toolCalls: signal([] as readonly ToolCall[]), + isLoading: signal(false), + applyClientResult: () => undefined, + }; + const cap = createClientToolsCapability( + submitFn as unknown as SubmitFn, + store, + persist as unknown as PersistToolMessagesFn | undefined, + ); + cap.setCatalog([spec]); + return { cap, submitFn }; + } + + it('writes all buffered messages in a single persist call', async () => { + const persist = vi.fn(async () => undefined); + const { cap, submitFn } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + cap.settle?.('t2', { ok: true, value: 'b' }); + await cap.flush?.(); + + expect(persist).toHaveBeenCalledTimes(1); + expect((persist as unknown as ReturnType).mock.calls[0][0]).toEqual([ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + { type: 'tool', role: 'tool', tool_call_id: 't2', content: 'b' }, + ]); + expect(submitFn).not.toHaveBeenCalled(); + }); + + it('makes no call when the buffer is empty', async () => { + const persist = vi.fn(async () => undefined); + const { cap } = setup(persist); + await cap.flush?.(); + expect(persist).not.toHaveBeenCalled(); + }); + + it('keeps the buffer when persist fails so a later drain retries', async () => { + const persist = vi.fn(async () => { throw new Error('boom'); }); + const { cap } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + await cap.flush?.(); + + expect(cap.drainToolMessages()).toEqual([ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + ]); + }); + + it('keeps the buffer when no persist function is supplied', async () => { + const { cap } = setup(undefined); + cap.settle?.('t1', { ok: true, value: 'a' }); + await cap.flush?.(); + expect(cap.drainToolMessages()).toHaveLength(1); + }); + + it('drainToolMessages empties the buffer', async () => { + const { cap } = setup(undefined); + cap.settle?.('t1', { ok: true, value: 'a' }); + cap.drainToolMessages(); + expect(cap.drainToolMessages()).toEqual([]); + }); +}); + +// ─── mergeStagedToolMessages helper ────────────────────────────────────────── + +describe('mergeStagedToolMessages', () => { + const staged = [ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + ] as const; + + it('prepends staged messages ahead of the payload messages', () => { + const out = mergeStagedToolMessages({ messages: [{ type: 'human', content: 'hi' }] }, staged); + expect(out).toEqual({ + messages: [ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + { type: 'human', content: 'hi' }, + ], + }); + }); + + it('leaves a null payload unchanged', () => { + expect(mergeStagedToolMessages(null, staged)).toBeNull(); + }); + + it('returns the payload unchanged when nothing is staged', () => { + const payload = { messages: [] }; + expect(mergeStagedToolMessages(payload, [])).toBe(payload); + }); +}); diff --git a/libs/langgraph/src/lib/client-tools.ts b/libs/langgraph/src/lib/client-tools.ts index 61e5c99cf..a815dc5cf 100644 --- a/libs/langgraph/src/lib/client-tools.ts +++ b/libs/langgraph/src/lib/client-tools.ts @@ -72,6 +72,37 @@ export function mergeClientTools( return { ...(payload as Record), client_tools: catalog }; } +/** Wire shape for a settled client-tool result awaiting durability. */ +export interface BufferedToolMessage { + readonly type: 'tool'; + readonly role: 'tool'; + readonly tool_call_id: string; + readonly content: string; +} + +/** Writes settled tool messages into server thread state without starting a run. */ +export type PersistToolMessagesFn = ( + messages: readonly BufferedToolMessage[], +) => Promise; + +/** + * Prepend staged tool messages to a run payload's message list. + * + * Mirrors mergeClientTools: a null payload signals a no-input resume and must + * stay null, so staged messages cannot ride along and are left buffered. + */ +export function mergeStagedToolMessages( + payload: unknown, + staged: readonly BufferedToolMessage[], +): unknown { + if (staged.length === 0) return payload; + if (payload === null || payload === undefined) return payload; + if (typeof payload !== 'object' || Array.isArray(payload)) return payload; + const record = payload as Record; + const existing = Array.isArray(record['messages']) ? record['messages'] : []; + return { ...record, messages: [...staged, ...existing] }; +} + /** * Creates a ClientToolsCapability backed by a LangGraph submit function and * a store of tool-call signals. Extracted into a factory so it can be @@ -88,6 +119,11 @@ export function mergeClientTools( * client tools, so `result` stays undefined on those entries. * - settle(id, result): marks the call as resolved, writes the local result, * and buffers a ToolMessage without issuing a run. + * - flush(): makes the whole buffer durable in ONE persistFn call without + * starting a run — the settlement path for tool groups that never continue. + * The buffer is cleared only on a successful write, so a failure (or an + * absent persistFn) leaves the results staged for the next flush or for the + * drainToolMessages() fallback in the submit wrapper. * - resolve(id, result): settles the result, then issues a NEW run on the SAME * thread by calling submitFn with the full buffered ToolMessage group: * input: { @@ -108,10 +144,15 @@ export function mergeClientTools( export function createClientToolsCapability( submitFn: SubmitFn, store: ClientToolsStore, -): ClientToolsCapability & { catalog: Signal } { + persistFn?: PersistToolMessagesFn, +): ClientToolsCapability & { + catalog: Signal; + drainToolMessages(): BufferedToolMessage[]; +} { const catalog = signal([]); const resolvedIds = signal>(new Set()); - const toolMessageBuffer: Array<{ type: 'tool'; role: 'tool'; tool_call_id: string; content: string }> = []; + const toolMessageBuffer: BufferedToolMessage[] = []; + let flushInFlight: Promise | undefined; const pending = computed(() => { // Client tools are only actionable after the run ends (the backend @@ -160,7 +201,10 @@ export function createClientToolsCapability( toolMessageBuffer.push({ type: 'tool', role: 'tool', tool_call_id: id, content }); } - const capability: ClientToolsCapability & { catalog: Signal } = { + const capability: ClientToolsCapability & { + catalog: Signal; + drainToolMessages(): BufferedToolMessage[]; + } = { catalog, setCatalog(specs: readonly ClientToolSpec[]): void { @@ -173,6 +217,37 @@ export function createClientToolsCapability( settleResult(id, result); }, + /** Remove and return every buffered tool message. */ + drainToolMessages(): BufferedToolMessage[] { + const drained = [...toolMessageBuffer]; + toolMessageBuffer.length = 0; + return drained; + }, + + flush(): Promise { + if (flushInFlight) return flushInFlight; + if (toolMessageBuffer.length === 0) return Promise.resolve(); + if (!persistFn) return Promise.resolve(); + + // Snapshot first: the buffer is cleared ONLY after a successful write, so + // a failure leaves the results staged for the next flush or submit drain. + const batch = [...toolMessageBuffer]; + flushInFlight = persistFn(batch) + .then(() => { + toolMessageBuffer.splice(0, batch.length); + }) + .catch((err: unknown) => { + console.warn( + `Client tool flush failed; ${batch.length} result(s) remain staged for the next run.`, + err, + ); + }) + .finally(() => { + flushInFlight = undefined; + }); + return flushInFlight; + }, + resolve(id: string, result: ClientToolResult): void { settleResult(id, result); // Issue a new run on the same thread. LangGraph's add_messages reducer From 726ebc4694041825d87df7f260ae50bd73b1e86f Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 10:02:58 -0300 Subject: [PATCH 05/13] fix(langgraph): take flush() batch ownership at snapshot and clear staging on thread switch --- libs/langgraph/src/lib/agent.fn.spec.ts | 130 ++++++++++++++++++++ libs/langgraph/src/lib/agent.fn.ts | 10 ++ libs/langgraph/src/lib/client-tools.spec.ts | 97 ++++++++++++++- libs/langgraph/src/lib/client-tools.ts | 45 +++++-- 4 files changed, 271 insertions(+), 11 deletions(-) diff --git a/libs/langgraph/src/lib/agent.fn.spec.ts b/libs/langgraph/src/lib/agent.fn.spec.ts index 69d369803..34e267ba8 100644 --- a/libs/langgraph/src/lib/agent.fn.spec.ts +++ b/libs/langgraph/src/lib/agent.fn.spec.ts @@ -1298,3 +1298,133 @@ describe('computeMessageCheckpoints', () => { expect(computeMessageCheckpoints(history).size).toBe(0); }); }); + +// ── Client-tool staging wiring ─────────────────────────────────────────────── +// The capability is unit-tested in client-tools.spec.ts; these cover the +// agent.fn.ts seam: which persist function is supplied, the null-payload +// drain guard, and discarding staged results on a thread switch. + +describe('agent — client tool staging', () => { + beforeEach(() => TestBed.configureTestingModule({})); + + const SPEC = { name: 'get_weather', description: 'w', parameters: {} }; + + /** MockAgentTransport has no updateState; add one that records its calls. */ + function withUpdateState(transport: MockAgentTransport) { + const updateCalls: Array<{ threadId: string; values: Record }> = []; + (transport as unknown as { + updateState: ( + threadId: string, + values: Record, + signal: AbortSignal, + ) => Promise; + }).updateState = async (threadId, values) => { + updateCalls.push({ threadId, values }); + }; + return updateCalls; + } + + /** The langgraph capability exposes staging members beyond the neutral contract. */ + function staging(ref: { clientTools: unknown }) { + return ref.clientTools as { + setCatalog(specs: unknown[]): void; + settle(id: string, result: { ok: true; value: unknown }): void; + flush(): Promise; + drainToolMessages(): Array<{ tool_call_id: string }>; + }; + } + + it('flush() writes staged tool messages via updateState without starting a run', async () => { + const transport = new MockAgentTransport(); + const updateCalls = withUpdateState(transport); + const ref = withInjectionContext(() => + agent({ apiUrl: '', assistantId: 'a', threadId: 't-1', transport, throttle: false }) + ); + const cap = staging(ref); + cap.setCatalog([SPEC]); + + cap.settle('tc-1', { ok: true, value: 'sunny' }); + await cap.flush(); + + expect(updateCalls).toHaveLength(1); + expect(updateCalls[0]?.threadId).toBe('t-1'); + expect(updateCalls[0]?.values?.['messages']).toEqual([ + { type: 'tool', role: 'tool', tool_call_id: 'tc-1', content: 'sunny' }, + ]); + // A durable write must not issue a run. + expect(transport.streams).toHaveLength(0); + expect(cap.drainToolMessages()).toEqual([]); + }); + + it('flush() keeps the buffer when there is no thread to write to', async () => { + const transport = new MockAgentTransport(); + const updateCalls = withUpdateState(transport); + // No threadId and no run yet → the bridge has no currentThreadId, so the + // persist function must throw rather than let updateState silently no-op. + const ref = withInjectionContext(() => + agent({ apiUrl: '', assistantId: 'a', transport, throttle: false }) + ); + const cap = staging(ref); + cap.setCatalog([SPEC]); + + cap.settle('tc-1', { ok: true, value: 'sunny' }); + await cap.flush(); + + expect(updateCalls).toHaveLength(0); + expect(cap.drainToolMessages().map((m) => m.tool_call_id)).toEqual(['tc-1']); + }); + + it('submit drains staged tool messages ahead of the payload messages', async () => { + // No updateState on the transport → flush() cannot persist, so the staged + // result must ride along with the next ordinary submit instead. + const transport = new MockAgentTransport(); + const ref = withInjectionContext(() => + agent({ apiUrl: '', assistantId: 'a', threadId: 't-1', transport, throttle: false }) + ); + const cap = staging(ref); + cap.setCatalog([SPEC]); + + cap.settle('tc-1', { ok: true, value: 'sunny' }); + await cap.flush(); + ref.submit({ message: 'and tomorrow?' }); + + const payload = transport.streams[0]?.payload as { messages: Array> }; + expect(payload.messages[0]).toMatchObject({ type: 'tool', tool_call_id: 'tc-1' }); + expect(payload.messages[1]).toMatchObject({ type: 'human' }); + // Drained exactly once — a second submit must not re-send it. + expect(cap.drainToolMessages()).toEqual([]); + }); + + it('submit(null) does not drain staged tool messages', async () => { + const transport = new MockAgentTransport(); + const ref = withInjectionContext(() => + agent({ apiUrl: '', assistantId: 'a', threadId: 't-1', transport, throttle: false }) + ); + const cap = staging(ref); + cap.setCatalog([SPEC]); + + cap.settle('tc-1', { ok: true, value: 'sunny' }); + ref.submit(null); + + // A null payload signals a no-input resume and cannot carry messages; + // draining into it would discard the result silently. + expect(transport.streams[0]?.payload).toBeNull(); + expect(cap.drainToolMessages().map((m) => m.tool_call_id)).toEqual(['tc-1']); + }); + + it('switchThread discards staged tool messages', () => { + const transport = new MockAgentTransport(); + const ref = withInjectionContext(() => + agent({ apiUrl: '', assistantId: 'a', threadId: 't-1', transport, throttle: false }) + ); + const cap = staging(ref); + cap.setCatalog([SPEC]); + + cap.settle('tc-1', { ok: true, value: 'sunny' }); + ref.switchThread('t-2'); + + // Carrying it over would prepend a ToolMessage whose tool_call_id matches + // no AIMessage on t-2 — turning one broken thread into two. + expect(cap.drainToolMessages()).toEqual([]); + }); +}); diff --git a/libs/langgraph/src/lib/agent.fn.ts b/libs/langgraph/src/lib/agent.fn.ts index 6bb164e1b..f94136ecc 100644 --- a/libs/langgraph/src/lib/agent.fn.ts +++ b/libs/langgraph/src/lib/agent.fn.ts @@ -193,10 +193,19 @@ export function agent< const custom$ = new BehaviorSubject([]); const hasValue$ = new BehaviorSubject(false); + // Assigned once the client-tools capability exists (further down — the + // capability needs `manager`, which needs these subjects). Called through a + // forward reference so the thread-change seam below stays in one place. + let clearStagedToolMessages: (() => void) | undefined; + function resetDerivedThreadState(): void { status$.next(ResourceStatus.Idle); error$.next(undefined); hasValue$.next(false); + // Staged client-tool results belong to the thread whose AIMessage produced + // their tool_call_ids. Carrying them into a different thread would prepend + // a ToolMessage that matches no tool call there — a 400 on that turn. + clearStagedToolMessages?.(); } // Track hasValue — becomes true once values or messages arrive @@ -449,6 +458,7 @@ export function agent< } : undefined, ); + clearStagedToolMessages = () => clientToolsCap.clearStagedToolMessages(); return { // ── Runtime-neutral surface (AgentWithHistory) ──────────────────────── diff --git a/libs/langgraph/src/lib/client-tools.spec.ts b/libs/langgraph/src/lib/client-tools.spec.ts index 3d3bb41e0..b0aab794f 100644 --- a/libs/langgraph/src/lib/client-tools.spec.ts +++ b/libs/langgraph/src/lib/client-tools.spec.ts @@ -456,10 +456,103 @@ describe('flush', () => { expect(cap.drainToolMessages()).toHaveLength(1); }); - it('drainToolMessages empties the buffer', async () => { + it('empties the buffer after a successful flush', async () => { + const persist = vi.fn(async () => undefined); + const { cap } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + await cap.flush?.(); + + // The central invariant: a successful write must NOT leave the result + // staged, or the next submit would re-send it and the thread would carry + // two ToolMessages for one tool_call_id. + expect(cap.drainToolMessages()).toEqual([]); + }); + + it('drainToolMessages returns the buffer and then empties it', async () => { const { cap } = setup(undefined); cap.settle?.('t1', { ok: true, value: 'a' }); - cap.drainToolMessages(); + // Assert the FIRST drain returns the message: without this the test would + // pass against a drainToolMessages that always returns []. + expect(cap.drainToolMessages()).toEqual([ + { type: 'tool', role: 'tool', tool_call_id: 't1', content: 'a' }, + ]); + expect(cap.drainToolMessages()).toEqual([]); + }); + + // ── concurrency: the buffer has three mutators ────────────────────────────── + // flush() takes ownership of its batch at snapshot time. resolve() and + // drainToolMessages() clear the buffer unconditionally and know nothing about + // an in-flight write, so anything left in the buffer across the await is + // fair game for them. + + it('does not drop or duplicate results when resolve and settle interleave with an in-flight flush', async () => { + let releasePersist!: () => void; + const persist = vi.fn( + () => new Promise((resolve) => { releasePersist = resolve; }), + ); + const { cap, submitFn } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + const flushed = cap.flush?.(); // batch = [t1]; write in flight + + cap.resolve('t2', { ok: true, value: 'b' }); // submits, then clears buffer + cap.settle?.('t3', { ok: true, value: 'c' }); // buffer = [t3] + + releasePersist(); + await flushed; + + // t1 is being written by the flush, so resolve() must not re-send it. + const payload = (submitFn as unknown as ReturnType) + .mock.calls[0][0] as { messages: Array<{ tool_call_id: string }> }; + expect(payload.messages.map((m) => m.tool_call_id)).toEqual(['t2']); + + // t3 was never persisted nor submitted — it must survive for the next drain. + expect(cap.drainToolMessages().map((m) => m.tool_call_id)).toEqual(['t3']); + }); + + it('coalesces overlapping flush calls into a single persist call', async () => { + let releasePersist!: () => void; + const persist = vi.fn( + () => new Promise((resolve) => { releasePersist = resolve; }), + ); + const { cap } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + const first = cap.flush?.(); + const second = cap.flush?.(); + + releasePersist(); + await Promise.all([first, second]); + + expect(persist).toHaveBeenCalledTimes(1); + }); + + // ── clearStagedToolMessages — thread switches must not leak ───────────────── + + it('clearStagedToolMessages discards the buffer', () => { + const { cap } = setup(undefined); + cap.settle?.('t1', { ok: true, value: 'a' }); + cap.clearStagedToolMessages(); + expect(cap.drainToolMessages()).toEqual([]); + }); + + it('does not re-stage a failed batch that was cleared while in flight', async () => { + let rejectPersist!: (err: Error) => void; + const persist = vi.fn( + () => new Promise((_resolve, reject) => { rejectPersist = reject; }), + ); + const { cap } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + const flushed = cap.flush?.(); + // Thread switched away while the write was in flight. + cap.clearStagedToolMessages(); + rejectPersist(new Error('boom')); + await flushed; + + // Re-staging here would prepend the old thread's ToolMessage onto the NEW + // thread's next payload, where its tool_call_id matches no AIMessage. expect(cap.drainToolMessages()).toEqual([]); }); }); diff --git a/libs/langgraph/src/lib/client-tools.ts b/libs/langgraph/src/lib/client-tools.ts index a815dc5cf..651d66397 100644 --- a/libs/langgraph/src/lib/client-tools.ts +++ b/libs/langgraph/src/lib/client-tools.ts @@ -121,9 +121,11 @@ export function mergeStagedToolMessages( * and buffers a ToolMessage without issuing a run. * - flush(): makes the whole buffer durable in ONE persistFn call without * starting a run — the settlement path for tool groups that never continue. - * The buffer is cleared only on a successful write, so a failure (or an - * absent persistFn) leaves the results staged for the next flush or for the - * drainToolMessages() fallback in the submit wrapper. + * The batch leaves the buffer at snapshot time and is re-staged only if the + * write fails, so a failure (or an absent persistFn) still degrades to the + * next flush or to the drainToolMessages() fallback in the submit wrapper, + * while a concurrent resolve()/drain can never re-send an in-flight batch. + * - clearStagedToolMessages(): discards the buffer on a thread switch. * - resolve(id, result): settles the result, then issues a NEW run on the SAME * thread by calling submitFn with the full buffered ToolMessage group: * input: { @@ -148,11 +150,15 @@ export function createClientToolsCapability( ): ClientToolsCapability & { catalog: Signal; drainToolMessages(): BufferedToolMessage[]; + clearStagedToolMessages(): void; } { const catalog = signal([]); const resolvedIds = signal>(new Set()); const toolMessageBuffer: BufferedToolMessage[] = []; let flushInFlight: Promise | undefined; + // Bumped whenever the buffer is discarded, so an in-flight flush can tell + // whether its batch still belongs to the current thread. + let bufferGeneration = 0; const pending = computed(() => { // Client tools are only actionable after the run ends (the backend @@ -204,6 +210,7 @@ export function createClientToolsCapability( const capability: ClientToolsCapability & { catalog: Signal; drainToolMessages(): BufferedToolMessage[]; + clearStagedToolMessages(): void; } = { catalog, @@ -224,19 +231,39 @@ export function createClientToolsCapability( return drained; }, + /** + * Discard everything staged. Called when the active thread changes: a + * ToolMessage only makes sense against the thread whose AIMessage produced + * its tool_call_id, so carrying it over would poison the new thread. + */ + clearStagedToolMessages(): void { + toolMessageBuffer.length = 0; + // Invalidate any in-flight flush so its failure path cannot re-stage the + // old thread's messages into the new thread's buffer. + bufferGeneration += 1; + }, + flush(): Promise { if (flushInFlight) return flushInFlight; if (toolMessageBuffer.length === 0) return Promise.resolve(); if (!persistFn) return Promise.resolve(); - // Snapshot first: the buffer is cleared ONLY after a successful write, so - // a failure leaves the results staged for the next flush or submit drain. - const batch = [...toolMessageBuffer]; + // Take ownership of the batch NOW. resolve() and drainToolMessages() both + // clear the buffer unconditionally and know nothing about an in-flight + // write, so leaving the batch in place across the await would let them + // re-send what this write already covers (a duplicate ToolMessage for one + // tool_call_id) and let the completion splice remove the wrong elements + // (dropping a result that was never persisted). + const batch = toolMessageBuffer.splice(0, toolMessageBuffer.length); + const generation = bufferGeneration; flushInFlight = persistFn(batch) - .then(() => { - toolMessageBuffer.splice(0, batch.length); - }) .catch((err: unknown) => { + // Re-stage at the FRONT so ordering is preserved for the next drain — + // unless the buffer was cleared meanwhile (thread switch), in which + // case these results belong to a thread we have left. + if (generation === bufferGeneration) { + toolMessageBuffer.unshift(...batch); + } console.warn( `Client tool flush failed; ${batch.length} result(s) remain staged for the next run.`, err, From 646fe8ae116522a13b0070e7cf74cec959383ced Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 10:13:46 -0300 Subject: [PATCH 06/13] fix(chat): flush settled client-tool groups and stop discarding limited results --- .../client-tools-coordinator.spec.ts | 165 +++++++++++++++++- .../client-tools/client-tools-coordinator.ts | 49 +++++- 2 files changed, 210 insertions(+), 4 deletions(-) diff --git a/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts b/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts index 33901282e..756816463 100644 --- a/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts +++ b/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts @@ -33,15 +33,17 @@ class FakeAskComponent {} function makeFakeCapability() { const pending = signal([]); const settle = vi.fn<[string, ClientToolResult], void>(); + const flush = vi.fn<[], void>(); const resolve = vi.fn<[string, ClientToolResult], void>(); const setCatalog = vi.fn<[readonly unknown[]], void>(); const capability: ClientToolsCapability = { setCatalog, pending, settle, + flush, resolve, }; - return { pending, settle, resolve, setCatalog, capability }; + return { pending, settle, flush, resolve, setCatalog, capability }; } function makeFakeCapabilityWithoutSettle() { @@ -317,6 +319,31 @@ describe('createClientToolsCoordinator()', () => { expect(resolve).not.toHaveBeenCalled(); }); + it('flushes a fully-terminal group so results reach the server', () => { + const registry = tools({ + terminal_card: view( + 'Show terminal card', + z.object({ city: z.string() }), + FakeViewComponent as never, + { followUp: false }, + ), + }); + const { pending, settle, resolve, flush, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + pending.set([{ id: 'v1', name: 'terminal_card', args: { city: 'LA' }, status: 'running' }]); + TestBed.flushEffects(); + + expect(settle).toHaveBeenCalledWith('v1', { ok: true, value: { shown: true } }); + expect(flush).toHaveBeenCalledTimes(1); + expect(resolve).not.toHaveBeenCalled(); + }); + it('falls back to resolve when followUp:false cannot be honored without settle', () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); const registry = tools({ @@ -521,6 +548,142 @@ describe('createClientToolsCoordinator()', () => { error.mockRestore(); }); + it('settles blocked calls with a limit error and preserves real ask results', () => { + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const registry = tools({ + confirm: ask('Confirm', z.object({ q: z.string() }), FakeAskComponent as never), + }); + const { pending, settle, resolve, flush, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry, { + continuationPolicy: { maxTurns: 1 }, + }); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + // Turn 1 consumes the single allowed continuation. + pending.set([{ id: 'a1', name: 'confirm', args: { q: 'x' }, status: 'running' }]); + TestBed.flushEffects(); + coordinator.handleRenderEvent(agent, { + type: 'result', + elementKey: 'confirm', + value: { confirmed: true }, + } as never); + + settle.mockClear(); + resolve.mockClear(); + flush.mockClear(); + + // Turn 2 exceeds maxTurns: the user's answer must still be recorded. + pending.set([{ id: 'a2', name: 'confirm', args: { q: 'y' }, status: 'running' }]); + TestBed.flushEffects(); + coordinator.handleRenderEvent(agent, { + type: 'result', + elementKey: 'confirm', + value: { confirmed: false }, + } as never); + + expect(settle).toHaveBeenCalledWith('a2', { ok: true, value: { confirmed: false } }); + expect(flush).toHaveBeenCalled(); + expect(resolve).not.toHaveBeenCalled(); + error.mockRestore(); + }); + + it('settles a blocked function tool exactly once across effect re-runs', async () => { + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const handler = vi.fn(async () => 'again'); + const registry = tools({ + loop: action('Loop', z.object({}), handler), + }); + const { pending, settle, resolve, flush, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry, { + continuationPolicy: { maxTurns: 1 }, + }); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + // Turn 1 consumes the single allowed continuation. + pending.set([{ id: 'c1', name: 'loop', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + expect(handler).toHaveBeenCalledOnce(); + settle.mockClear(); + resolve.mockClear(); + flush.mockClear(); + handler.mockClear(); + + // Turn 2 is blocked. Re-emitting the same pending call must not re-settle it: + // the predicate that records the block runs on every effect pass. + const blocked: ToolCall = { id: 'c2', name: 'loop', args: {}, status: 'complete' }; + pending.set([blocked]); + TestBed.flushEffects(); + await drainMicrotasks(); + pending.set([{ ...blocked }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + expect(settle).toHaveBeenCalledTimes(1); + expect(settle).toHaveBeenCalledWith('c2', { + ok: false, + error: 'client tool continuation limit reached; loop was not executed', + }); + expect(resolve).not.toHaveBeenCalled(); + expect(handler).not.toHaveBeenCalled(); + error.mockRestore(); + }); + + it('settles a blocked call once even when a later call reforms the group', async () => { + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const handler = vi.fn(async () => 'again'); + const registry = tools({ loop: action('Loop', z.object({}), handler) }); + const { pending, settle, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry, { + continuationPolicy: { maxTurns: 1 }, + }); + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + pending.set([{ id: 'c1', name: 'loop', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + settle.mockClear(); + + // c2 is blocked and settled with a limit error. + pending.set([{ id: 'c2', name: 'loop', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + // c2 is STILL pending when a new call joins, which reforms the group with + // empty settle bookkeeping. A further effect pass must not re-settle c2. + const reformed: readonly ToolCall[] = [ + { id: 'c2', name: 'loop', args: {}, status: 'complete' }, + { id: 'c9', name: 'loop', args: {}, status: 'complete' }, + ]; + pending.set(reformed); + TestBed.flushEffects(); + await drainMicrotasks(); + pending.set(reformed.map((tc) => ({ ...tc }))); + TestBed.flushEffects(); + await drainMicrotasks(); + error.mockRestore(); + + const limitError = { + ok: false, + error: 'client tool continuation limit reached; loop was not executed', + }; + expect(settle.mock.calls).toEqual([ + ['c2', limitError], + ['c9', limitError], + ]); + }); + it('handleRenderEvent() resolves pending ask tool call by elementKey (tool name)', () => { const { pending, resolve, capability } = makeFakeCapability(); const agent = makeFakeAgent(capability); diff --git a/libs/chat/src/lib/client-tools/client-tools-coordinator.ts b/libs/chat/src/lib/client-tools/client-tools-coordinator.ts index 9c86b2977..17e0198fd 100644 --- a/libs/chat/src/lib/client-tools/client-tools-coordinator.ts +++ b/libs/chat/src/lib/client-tools/client-tools-coordinator.ts @@ -70,6 +70,12 @@ export function createClientToolsCoordinator( ): ClientToolsCoordinator { const viewRegistry = views(viewComponents(registry)); const ackedViews = new Set(); + // Tool calls already settled with a continuation-limit result. Tracked outside + // the pending group because a blocked call can outlive the group it was + // blocked in: a later call joining `pending` reforms the group with empty + // settle bookkeeping, and the executor effect re-runs the predicate for every + // still-pending call. + const blockedIds = new Set(); let currentGroup: PendingToolGroup | undefined; let currentUserTurnKey = ''; let continuationTurns = 0; @@ -146,6 +152,18 @@ export function createClientToolsCoordinator( ); } + function flushSettledResults(cap: ClientToolsCapability): void { + if (!cap.flush) { + console.warn( + 'Client tool group settled with no follow-up, but the agent capability does not implement flush(); results may not reach the server.', + ); + return; + } + void Promise.resolve(cap.flush()).catch((err: unknown) => { + console.error('Client tool flush failed', err); + }); + } + function settleClientToolCall( cap: ClientToolsCapability, agent: Agent, @@ -153,7 +171,18 @@ export function createClientToolsCoordinator( result: ClientToolResult, ): void { const group = groupFor(agent, cap, tc); - if (!group.allowed) return; + // Over the continuation limit: still record the result so the server never + // keeps an unanswered tool call, but never continue the run. + if (!group.allowed) { + if (group.settledIds.has(tc.id) || blockedIds.has(tc.id)) return; + group.settledIds.add(tc.id); + blockedIds.add(tc.id); + if (cap.settle) { + cap.settle(tc.id, result); + flushSettledResults(cap); + } + return; + } if (group.settledIds.has(tc.id)) return; group.settledIds.add(tc.id); @@ -172,7 +201,12 @@ export function createClientToolsCoordinator( } cap.settle(tc.id, result); - if (groupComplete) currentGroup = undefined; + if (groupComplete) { + // Terminal group: nothing will continue the run, so make the settled + // results durable ourselves or the server keeps an unanswered tool call. + flushSettledResults(cap); + currentGroup = undefined; + } } return { @@ -183,7 +217,16 @@ export function createClientToolsCoordinator( cap.setCatalog(toClientToolSpecs(registry)); startClientToolExecutor(agent, registry, { executionGuard: options.executionGuard, - shouldExecuteToolCall: (tc) => shouldHandleClientToolCall(agent, cap, tc), + shouldExecuteToolCall: (tc) => { + if (shouldHandleClientToolCall(agent, cap, tc)) return true; + // Blocked before executing: record why, so the model sees a reason + // instead of an unanswered tool call. + settleClientToolCall(cap, agent, tc, { + ok: false, + error: `client tool continuation limit reached; ${tc.name} was not executed`, + }); + return false; + }, settleToolCall: (tc, result) => settleClientToolCall(cap, agent, tc, result), }); // function tools // Auto-ack `view` tools: they render but produce no user value. From 27c71d6d4b9aaa902ead941f8239a487a3de9b3d Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 10:24:16 -0300 Subject: [PATCH 07/13] fix(chat): settle aborted client tools and wrap agent.stop once --- .../client-tool-execution-guard.ts | 8 + .../client-tools/client-tool-executor.spec.ts | 204 +++++++++++++++++- .../lib/client-tools/client-tool-executor.ts | 70 ++++-- libs/chat/src/lib/client-tools/index.ts | 1 + 4 files changed, 264 insertions(+), 19 deletions(-) diff --git a/libs/chat/src/lib/client-tools/client-tool-execution-guard.ts b/libs/chat/src/lib/client-tools/client-tool-execution-guard.ts index 94d8f42d3..630248c2d 100644 --- a/libs/chat/src/lib/client-tools/client-tool-execution-guard.ts +++ b/libs/chat/src/lib/client-tools/client-tool-execution-guard.ts @@ -46,6 +46,14 @@ export function defaultInterruptedClientToolResult(toolCallId: string): ClientTo }; } +/** Result recorded when the user stops a run while a client tool is running. */ +export function cancelledClientToolResult(toolCallId: string): ClientToolResult { + return { + ok: false, + error: `client tool execution cancelled before completion: ${toolCallId}`, + }; +} + /** Default fail-closed result when the execution guard itself cannot be reached. */ export function clientToolGuardFailureResult(toolCallId: string, error: unknown): ClientToolResult { const message = error instanceof Error ? error.message : String(error); diff --git a/libs/chat/src/lib/client-tools/client-tool-executor.spec.ts b/libs/chat/src/lib/client-tools/client-tool-executor.spec.ts index ef801e527..8365cd7bd 100644 --- a/libs/chat/src/lib/client-tools/client-tool-executor.spec.ts +++ b/libs/chat/src/lib/client-tools/client-tool-executor.spec.ts @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT import { describe, it, expect, vi, beforeEach } from 'vitest'; import { TestBed } from '@angular/core/testing'; -import { signal } from '@angular/core'; +import { computed, signal } from '@angular/core'; import { z } from 'zod/v4'; import { action, view, tools } from './tools'; import { startClientToolExecutor } from './client-tool-executor'; @@ -39,6 +39,26 @@ function makeFakeCapability() { return { pending, resolve, capability }; } +/** + * Capability that mirrors the adapters' real `pending` contract: `resolve()` + * marks the id resolved and `pending` drops resolved calls. Needed to prove a + * settled (incl. cancelled) call is never re-dispatched on a later effect pass. + */ +function makeResolvingCapability() { + const raw = signal([]); + const resolvedIds = signal>(new Set()); + const pending = computed(() => raw().filter((tc) => !resolvedIds().has(tc.id))); + const resolve = vi.fn<[string, ClientToolResult], void>((id) => { + resolvedIds.update((s) => new Set(s).add(id)); + }); + const capability: ClientToolsCapability = { + setCatalog: vi.fn(), + pending, + resolve, + }; + return { raw, resolve, capability }; +} + function makeFakeAgent(capability: ClientToolsCapability): Agent { return { messages: signal([]), @@ -235,7 +255,7 @@ describe('startClientToolExecutor()', () => { expect(seen[0].aborted).toBe(false); }); - it('aborts in-flight function tools on stop and does not resolve them', async () => { + it('aborts in-flight function tools on stop and settles them as cancelled', async () => { let complete!: (value: string) => void; const completion = new Promise((resolve) => { complete = resolve; @@ -264,7 +284,81 @@ describe('startClientToolExecutor()', () => { complete('late result'); await drainMicrotasks(); - expect(resolve).not.toHaveBeenCalled(); + // The server thread must never hold a client tool call without a result: + // an aborted call settles with a cancelled error rather than dangling. + expect(resolve).toHaveBeenCalledOnce(); + expect(resolve.mock.calls[0][0]).toBe('slow-1'); + expect(resolve.mock.calls[0][1].ok).toBe(false); + expect((resolve.mock.calls[0][1] as { error: string }).error).toContain('cancelled'); + }); + + it('settles an aborted handler so it cannot re-execute', async () => { + const settled: Array<[string, ClientToolResult]> = []; + let release!: () => void; + const registry = tools({ + slow: action('Slow', z.object({}), async () => { + await new Promise((r) => { + release = r; + }); + return 'done'; + }), + }); + const { pending, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry, { + settleToolCall: (tc, result) => settled.push([tc.id, result]), + }); + }); + + pending.set([{ id: 'slow-1', name: 'slow', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + release(); + await drainMicrotasks(); + + expect(settled).toHaveLength(1); + expect(settled[0][0]).toBe('slow-1'); + expect(settled[0][1].ok).toBe(false); + }); + + it('does not re-dispatch an aborted client tool on a later effect pass', async () => { + const handler = vi.fn(async () => { + await new Promise((r) => { + release = r; + }); + return 'done'; + }); + let release!: () => void; + const registry = tools({ + slow: action('Slow', z.object({}), handler), + }); + const { raw, resolve, capability } = makeResolvingCapability(); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + }); + + raw.set([{ id: 'slow-2', name: 'slow', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + release(); + await drainMicrotasks(); + + // The next run re-emits the same tool call list; the cancelled call is now + // resolved, so it must not be dispatched to the handler a second time. + raw.set([{ id: 'slow-2', name: 'slow', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + expect(handler).toHaveBeenCalledOnce(); + expect(resolve).toHaveBeenCalledOnce(); }); it('aborts in-flight function tools when the injection context is destroyed', async () => { @@ -444,7 +538,7 @@ describe('startClientToolExecutor()', () => { expect(resolve).toHaveBeenCalledWith('read-1', { ok: true, value: 'cached' }); }); - it('does not execute or resolve when stopped before a delayed claim resolves', async () => { + it('does not execute but still settles when stopped before a delayed claim resolves', async () => { let resolveClaim!: (value: 'claimed') => void; const claim = new Promise<'claimed'>((resolve) => { resolveClaim = resolve; @@ -471,7 +565,107 @@ describe('startClientToolExecutor()', () => { await drainMicrotasks(); expect(handler).not.toHaveBeenCalled(); - expect(resolve).not.toHaveBeenCalled(); + expect(resolve).toHaveBeenCalledOnce(); + expect(resolve.mock.calls[0][1].ok).toBe(false); + expect((resolve.mock.calls[0][1] as { error: string }).error).toContain('cancelled'); + }); + + it('records the cancelled result when stopped after claiming', async () => { + let release!: () => void; + const handler = vi.fn(async () => { + await new Promise((r) => { + release = r; + }); + return 'charged'; + }); + const registry = tools({ + charge: action('Charge a card', z.object({}), handler), + }); + const store = makeGuardStore('claimed'); + const { pending, resolve, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry, { executionGuard: makeGuard(store) }); + }); + + pending.set([{ id: 'charge-7', name: 'charge', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + release(); + await drainMicrotasks(8); + + // The store must not be left at 'executing' — a later reload would then + // fail closed with a misleading "interrupted" message. + expect(store.record).toHaveBeenCalledOnce(); + expect(store.record.mock.calls[0][1].ok).toBe(false); + expect(resolve).toHaveBeenCalledOnce(); + expect(resolve.mock.calls[0][1].ok).toBe(false); + }); + + it('wraps agent.stop once across repeated executor starts', () => { + const registry = tools({ noop: action('n', z.object({}), async () => 'x') }); + const { capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const original = agent.stop; + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + startClientToolExecutor(agent, registry); + }); + + expect(agent.stop).not.toBe(original); + const afterFirstWrap = agent.stop; + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + }); + expect(agent.stop).toBe(afterFirstWrap); + }); + + it('restores agent.stop once every executor is destroyed', () => { + const registry = tools({ noop: action('n', z.object({}), async () => 'x') }); + const { capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const original = agent.stop; + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + startClientToolExecutor(agent, registry); + }); + expect(agent.stop).not.toBe(original); + + TestBed.resetTestingModule(); + + expect(agent.stop).toBe(original); + }); + + it('aborts every live executor on the same agent when stop is called', async () => { + const seen: AbortSignal[] = []; + const registry = tools({ + slow: action('slow', z.object({}), async (_args, context) => { + seen.push(context.signal); + return new Promise(() => undefined); + }), + }); + const { pending, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + // A second coordinator wired to the same long-lived (root-provided) agent. + startClientToolExecutor(agent, registry); + }); + + pending.set([{ id: 'a1', name: 'slow', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + + expect(seen).toHaveLength(2); + expect(seen.map((s) => s.aborted)).toEqual([true, true]); }); it('ignores view/ask tool calls (only function tools are auto-executed)', async () => { diff --git a/libs/chat/src/lib/client-tools/client-tool-executor.ts b/libs/chat/src/lib/client-tools/client-tool-executor.ts index 61d2bc07c..cf117556b 100644 --- a/libs/chat/src/lib/client-tools/client-tool-executor.ts +++ b/libs/chat/src/lib/client-tools/client-tool-executor.ts @@ -6,6 +6,7 @@ import type { ClientToolRegistry, AnyFunctionToolDef } from './tool-def'; import type { ClientToolResult } from './client-tools-capability'; import { executeFunctionTool } from './execute'; import { + cancelledClientToolResult, clientToolGuardFailureResult, defaultInterruptedClientToolResult, shouldClaimBeforeExecute, @@ -21,6 +22,16 @@ export interface ClientToolExecutorOptions { readonly shouldExecuteToolCall?: (toolCall: ToolCall) => boolean; } +/** Live stop() patch per agent: every executor's abort, plus the original stop. */ +interface AgentStopPatch { + readonly aborts: Set<() => void>; + readonly originalStop: Agent['stop']; + readonly boundStop: () => Promise; +} + +/** Agents whose stop() this module has already wrapped. */ +const patchedAgents = new WeakMap(); + /** * Watches the agent's pending client tool calls and auto-runs FUNCTION tools, * resolving each with its result. View/ask (component) tools are handled by the @@ -42,11 +53,32 @@ export function startClientToolExecutor( } }; - const originalStop = agent.stop.bind(agent); - agent.stop = async (): Promise => { - abortAll(); - await originalStop(); - }; + // The stop button lives in chat-input, which has no coordinator reference, so + // wrapping agent.stop is the only interception seam. Wrap at most once per + // agent, fan the stop out to EVERY live executor, and restore the original + // once the last one is destroyed — agents often outlive the components that + // start executors, so an unbounded stack of wrappers would leak. + let patch = patchedAgents.get(agent); + if (!patch) { + const originalStop = agent.stop; + const boundStop = originalStop.bind(agent); + const aborts = new Set<() => void>(); + patch = { aborts, originalStop, boundStop }; + patchedAgents.set(agent, patch); + agent.stop = async (): Promise => { + for (const abort of aborts) abort(); + await boundStop(); + }; + } + const registration = patch; + registration.aborts.add(abortAll); + destroyRef.onDestroy(() => { + registration.aborts.delete(abortAll); + if (registration.aborts.size === 0 && patchedAgents.get(agent) === registration) { + agent.stop = registration.originalStop; + patchedAgents.delete(agent); + } + }); destroyRef.onDestroy(abortAll); effect(() => { @@ -88,9 +120,13 @@ async function runFunctionTool(input: { }): Promise { const { def, toolCall, rawArgs, toolCallId, controller, executionGuard, settleToolCall } = input; const signal = controller.signal; + // An aborted call MUST still settle: leaving it unsettled keeps it out of + // `resolvedIds`, so it stays pending and is re-dispatched after the next run + // (re-running a side-effecting handler the user explicitly stopped) and + // leaves the server thread holding a tool call with no tool result. if (!executionGuard || !shouldClaimBeforeExecute(def)) { const result = await executeFunctionTool(def, rawArgs, { signal }); - if (!signal.aborted) settleToolCall(toolCall, result); + settleToolCall(toolCall, signal.aborted ? cancelledClientToolResult(toolCallId) : result); return; } @@ -102,18 +138,20 @@ async function runFunctionTool(input: { if (!signal.aborted) settleToolCall(toolCall, clientToolGuardFailureResult(toolCallId, err)); return; } - if (signal.aborted) return; + if (signal.aborted) { + settleToolCall(toolCall, cancelledClientToolResult(toolCallId)); + return; + } if (claim === 'claimed') { const result = await executeFunctionTool(def, rawArgs, { signal }); - if (signal.aborted) return; + const finalResult = signal.aborted ? cancelledClientToolResult(toolCallId) : result; await recordOrResolveGuardFailure( executionGuard, key, - result, + finalResult, toolCall, toolCallId, - signal, settleToolCall, ); return; @@ -133,25 +171,29 @@ async function runFunctionTool(input: { result, toolCall, toolCallId, - signal, settleToolCall, ); } +/** + * Record the final result then settle. Deliberately abort-agnostic: an aborted + * execution must still write its (cancelled) result, otherwise the guard store + * stays at `executing` forever and a later reload fails closed with a + * misleading "interrupted" message. + */ async function recordOrResolveGuardFailure( executionGuard: ClientToolExecutionGuard, key: ClientToolExecutionKey, result: ClientToolResult, toolCall: ToolCall, toolCallId: string, - signal: AbortSignal, settleToolCall: (toolCall: ToolCall, result: ClientToolResult) => void, ): Promise { try { await executionGuard.store.record(key, result); } catch (err) { - if (!signal.aborted) settleToolCall(toolCall, clientToolGuardFailureResult(toolCallId, err)); + settleToolCall(toolCall, clientToolGuardFailureResult(toolCallId, err)); return; } - if (!signal.aborted) settleToolCall(toolCall, result); + settleToolCall(toolCall, result); } diff --git a/libs/chat/src/lib/client-tools/index.ts b/libs/chat/src/lib/client-tools/index.ts index 7102ec026..b209bde49 100644 --- a/libs/chat/src/lib/client-tools/index.ts +++ b/libs/chat/src/lib/client-tools/index.ts @@ -27,6 +27,7 @@ export type { ClientToolExecutorOptions } from './client-tool-executor'; export { createClientToolsCoordinator, toClientToolSpecs } from './client-tools-coordinator'; export type { ClientToolsCoordinator, ClientToolsCoordinatorOptions } from './client-tools-coordinator'; export { + cancelledClientToolResult, clientToolGuardFailureResult, defaultInterruptedClientToolResult, shouldClaimBeforeExecute, From 3f089a2dd8de387b2a232db71d28479be18b4282 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 10:29:41 -0300 Subject: [PATCH 08/13] fix(chat): flush blocked client-tool groups once on completion Flushing per blocked call stranded every batch after the first: adapter flush() implementations coalesce concurrent calls by returning the in-flight promise, so only the first batch was ever snapshotted. Gate the blocked-group flush on group completion, mirroring the terminal path. Drop the blockedIds guard: both shipped adapters mark a call resolved inside settle() so pending() drops it immediately, meaning a settled call can never be re-presented to the executor effect. The hazard it guarded was an artifact of a test double whose settle() left calls pending forever; the fakes now mirror adapter behavior instead. Warn rather than silently discard when a blocked call cannot be recorded because the capability implements no settle(). --- .../client-tools-coordinator.spec.ts | 145 +++++++++++++++--- .../client-tools/client-tools-coordinator.ts | 35 ++--- 2 files changed, 141 insertions(+), 39 deletions(-) diff --git a/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts b/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts index 756816463..cc034fbbb 100644 --- a/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts +++ b/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT import { describe, it, expect, vi, beforeEach } from 'vitest'; import { TestBed } from '@angular/core/testing'; -import { signal } from '@angular/core'; +import { computed, signal } from '@angular/core'; import { z } from 'zod/v4'; import { action, view, ask, tools } from './tools'; import { toClientToolSpecs, createClientToolsCoordinator } from './client-tools-coordinator'; @@ -30,15 +30,29 @@ class FakeAskComponent {} // ── factory helpers ─────────────────────────────────────────────────────────── +/** Both shipped adapters mark a call resolved inside settle()/resolve() so that + * `pending()` drops it immediately — a settled call can never be re-presented + * to the executor effect. The fakes below mirror that, otherwise they invite + * guards against hazards the real adapters cannot produce. Tests drive the raw + * list; the capability sees the filtered view. */ +function pendingView( + raw: ReturnType>, + resolvedIds: ReturnType>>, +) { + return computed(() => raw().filter((tc) => !resolvedIds().has(tc.id))); +} + function makeFakeCapability() { const pending = signal([]); - const settle = vi.fn<[string, ClientToolResult], void>(); + const resolvedIds = signal>(new Set()); + const drop = (id: string): void => resolvedIds.update((s) => new Set(s).add(id)); + const settle = vi.fn<[string, ClientToolResult], void>((id) => drop(id)); const flush = vi.fn<[], void>(); - const resolve = vi.fn<[string, ClientToolResult], void>(); + const resolve = vi.fn<[string, ClientToolResult], void>((id) => drop(id)); const setCatalog = vi.fn<[readonly unknown[]], void>(); const capability: ClientToolsCapability = { setCatalog, - pending, + pending: pendingView(pending, resolvedIds), settle, flush, resolve, @@ -48,11 +62,14 @@ function makeFakeCapability() { function makeFakeCapabilityWithoutSettle() { const pending = signal([]); - const resolve = vi.fn<[string, ClientToolResult], void>(); + const resolvedIds = signal>(new Set()); + const resolve = vi.fn<[string, ClientToolResult], void>((id) => + resolvedIds.update((s) => new Set(s).add(id)), + ); const setCatalog = vi.fn<[readonly unknown[]], void>(); const capability: ClientToolsCapability = { setCatalog, - pending, + pending: pendingView(pending, resolvedIds), resolve, }; return { pending, resolve, setCatalog, capability }; @@ -294,7 +311,7 @@ describe('createClientToolsCoordinator()', () => { expect(resolve).toHaveBeenCalledWith('f1', { ok: true, value: { temp: 72, city: 'SF' } }); }); - it('settles a fully-terminal group without resolving', () => { + it('settles a fully-terminal group and flushes once, without resolving', () => { const registry = tools({ terminal_card: view( 'Show terminal card', @@ -303,7 +320,7 @@ describe('createClientToolsCoordinator()', () => { { followUp: false }, ), }); - const { pending, settle, resolve, capability } = makeFakeCapability(); + const { pending, settle, resolve, flush, capability } = makeFakeCapability(); const agent = makeFakeAgent(capability); const coordinator = createClientToolsCoordinator(registry); @@ -316,17 +333,22 @@ describe('createClientToolsCoordinator()', () => { expect(settle).toHaveBeenCalledOnce(); expect(settle).toHaveBeenCalledWith('v1', { ok: true, value: { shown: true } }); + // Nothing continues the run, so the coordinator must make the results durable. + expect(flush).toHaveBeenCalledTimes(1); expect(resolve).not.toHaveBeenCalled(); }); - it('flushes a fully-terminal group so results reach the server', () => { + it('flushes a multi-call terminal group exactly once', () => { const registry = tools({ - terminal_card: view( - 'Show terminal card', - z.object({ city: z.string() }), - FakeViewComponent as never, - { followUp: false }, - ), + card_a: view('Card A', z.object({ city: z.string() }), FakeViewComponent as never, { + followUp: false, + }), + card_b: view('Card B', z.object({ city: z.string() }), FakeViewComponent as never, { + followUp: false, + }), + card_c: view('Card C', z.object({ city: z.string() }), FakeViewComponent as never, { + followUp: false, + }), }); const { pending, settle, resolve, flush, capability } = makeFakeCapability(); const agent = makeFakeAgent(capability); @@ -336,10 +358,16 @@ describe('createClientToolsCoordinator()', () => { coordinator.connect(agent); }); - pending.set([{ id: 'v1', name: 'terminal_card', args: { city: 'LA' }, status: 'running' }]); + pending.set([ + { id: 't1', name: 'card_a', args: { city: 'LA' }, status: 'running' }, + { id: 't2', name: 'card_b', args: { city: 'SF' }, status: 'running' }, + { id: 't3', name: 'card_c', args: { city: 'NY' }, status: 'running' }, + ]); TestBed.flushEffects(); - expect(settle).toHaveBeenCalledWith('v1', { ok: true, value: { shown: true } }); + expect(settle.mock.calls.map((c) => c[0])).toEqual(['t1', 't2', 't3']); + // One flush for the whole group: adapters coalesce concurrent flushes, so a + // per-call flush would strand every batch after the first. expect(flush).toHaveBeenCalledTimes(1); expect(resolve).not.toHaveBeenCalled(); }); @@ -586,11 +614,85 @@ describe('createClientToolsCoordinator()', () => { } as never); expect(settle).toHaveBeenCalledWith('a2', { ok: true, value: { confirmed: false } }); - expect(flush).toHaveBeenCalled(); + expect(flush).toHaveBeenCalledTimes(1); expect(resolve).not.toHaveBeenCalled(); error.mockRestore(); }); + it('flushes a two-call blocked group once, after both calls settle', async () => { + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const registry = tools({ + loop_a: action('Loop A', z.object({}), async () => 'a'), + loop_b: action('Loop B', z.object({}), async () => 'b'), + }); + const { pending, settle, resolve, flush, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry, { + continuationPolicy: { maxTurns: 1 }, + }); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + // Turn 1 consumes the single allowed continuation. + pending.set([{ id: 'a1', name: 'loop_a', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + settle.mockClear(); + resolve.mockClear(); + flush.mockClear(); + + // Turn 2 trips the limit with TWO calls. Both must be settled, and the + // group must flush exactly once — adapters coalesce concurrent flushes, so + // a per-call flush strands b2's batch and leaves it unanswered on reload. + pending.set([ + { id: 'b1', name: 'loop_a', args: {}, status: 'complete' }, + { id: 'b2', name: 'loop_b', args: {}, status: 'complete' }, + ]); + TestBed.flushEffects(); + await drainMicrotasks(); + + expect(settle.mock.calls.map((c) => c[0])).toEqual(['b1', 'b2']); + expect(flush).toHaveBeenCalledTimes(1); + expect(resolve).not.toHaveBeenCalled(); + error.mockRestore(); + }); + + it('warns instead of silently discarding a blocked call when settle() is missing', async () => { + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const registry = tools({ + loop: action('Loop', z.object({}), async () => 'again'), + }); + const { pending, resolve, capability } = makeFakeCapabilityWithoutSettle(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry, { + continuationPolicy: { maxTurns: 1 }, + }); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + pending.set([{ id: 'c1', name: 'loop', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + resolve.mockClear(); + + pending.set([{ id: 'c2', name: 'loop', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + // Cannot record the result without settle(), and must not continue the run — + // but the operator gets told, rather than the result vanishing silently. + expect(resolve).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledOnce(); + warn.mockRestore(); + error.mockRestore(); + }); + it('settles a blocked function tool exactly once across effect re-runs', async () => { const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); const handler = vi.fn(async () => 'again'); @@ -638,7 +740,7 @@ describe('createClientToolsCoordinator()', () => { error.mockRestore(); }); - it('settles a blocked call once even when a later call reforms the group', async () => { + it('does not re-settle a blocked call when a later call reforms the group', async () => { const error = vi.spyOn(console, 'error').mockImplementation(() => undefined); const handler = vi.fn(async () => 'again'); const registry = tools({ loop: action('Loop', z.object({}), handler) }); @@ -660,8 +762,9 @@ describe('createClientToolsCoordinator()', () => { TestBed.flushEffects(); await drainMicrotasks(); - // c2 is STILL pending when a new call joins, which reforms the group with - // empty settle bookkeeping. A further effect pass must not re-settle c2. + // A new call joins while c2 is still in the raw list. The group reforms with + // empty settle bookkeeping, but settle() already dropped c2 from pending(), + // so the effect never sees it again and it is not re-settled. const reformed: readonly ToolCall[] = [ { id: 'c2', name: 'loop', args: {}, status: 'complete' }, { id: 'c9', name: 'loop', args: {}, status: 'complete' }, diff --git a/libs/chat/src/lib/client-tools/client-tools-coordinator.ts b/libs/chat/src/lib/client-tools/client-tools-coordinator.ts index 17e0198fd..3ff3eaa02 100644 --- a/libs/chat/src/lib/client-tools/client-tools-coordinator.ts +++ b/libs/chat/src/lib/client-tools/client-tools-coordinator.ts @@ -70,12 +70,6 @@ export function createClientToolsCoordinator( ): ClientToolsCoordinator { const viewRegistry = views(viewComponents(registry)); const ackedViews = new Set(); - // Tool calls already settled with a continuation-limit result. Tracked outside - // the pending group because a blocked call can outlive the group it was - // blocked in: a later call joining `pending` reforms the group with empty - // settle bookkeeping, and the executor effect re-runs the predicate for every - // still-pending call. - const blockedIds = new Set(); let currentGroup: PendingToolGroup | undefined; let currentUserTurnKey = ''; let continuationTurns = 0; @@ -155,7 +149,7 @@ export function createClientToolsCoordinator( function flushSettledResults(cap: ClientToolsCapability): void { if (!cap.flush) { console.warn( - 'Client tool group settled with no follow-up, but the agent capability does not implement flush(); results may not reach the server.', + 'Client tool results were settled with no follow-up run, but the agent capability does not implement flush(); results may not reach the server.', ); return; } @@ -171,22 +165,27 @@ export function createClientToolsCoordinator( result: ClientToolResult, ): void { const group = groupFor(agent, cap, tc); + if (group.settledIds.has(tc.id)) return; + group.settledIds.add(tc.id); + + const groupComplete = Array.from(group.ids).every((id) => group.settledIds.has(id)); + // Over the continuation limit: still record the result so the server never - // keeps an unanswered tool call, but never continue the run. + // keeps an unanswered tool call, but never continue the run. Keep the group + // as `currentGroup` so repeated effect passes over the same calls short- + // circuit on `settledIds` above. if (!group.allowed) { - if (group.settledIds.has(tc.id) || blockedIds.has(tc.id)) return; - group.settledIds.add(tc.id); - blockedIds.add(tc.id); - if (cap.settle) { - cap.settle(tc.id, result); - flushSettledResults(cap); + if (!cap.settle) { + warnMissingSettle(tc); + return; } + cap.settle(tc.id, result); + // Flush ONCE, when the last blocked call settles. Adapters coalesce + // concurrent flushes (returning the in-flight promise), so flushing per + // call would strand every batch after the first. + if (groupComplete) flushSettledResults(cap); return; } - if (group.settledIds.has(tc.id)) return; - group.settledIds.add(tc.id); - - const groupComplete = Array.from(group.ids).every((id) => group.settledIds.has(id)); if (!cap.settle) { if (group.ids.size > 1 || registry[tc.name]?.followUp === false) warnMissingSettle(tc); cap.resolve(tc.id, result); From 3e31911626d8f7e7f85ae99235a413868361a7dd Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 10:32:46 -0300 Subject: [PATCH 09/13] fix(middleware): enforce tenant isolation in the client tool execution store --- .../postgres-client-tool-execution-store.ts | 19 ++-- ...stgres-client-tool-execution-store.spec.ts | 87 +++++++++++++++++-- 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/libs/middleware/src/langgraph/postgres-client-tool-execution-store.ts b/libs/middleware/src/langgraph/postgres-client-tool-execution-store.ts index 8bff28261..d26ddea40 100644 --- a/libs/middleware/src/langgraph/postgres-client-tool-execution-store.ts +++ b/libs/middleware/src/langgraph/postgres-client-tool-execution-store.ts @@ -14,18 +14,19 @@ export type PostgresTaggedSql = ( export const THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA = ` CREATE TABLE IF NOT EXISTS threadplane_client_tool_executions ( - tenant_id text, + tenant_id text NOT NULL DEFAULT '', thread_id text NOT NULL, tool_call_id text NOT NULL, status text NOT NULL, result jsonb, created_at timestamptz NOT NULL DEFAULT now(), updated_at timestamptz NOT NULL DEFAULT now(), - PRIMARY KEY (thread_id, tool_call_id) + PRIMARY KEY (tenant_id, thread_id, tool_call_id) ); `; export interface PostgresClientToolExecutionStoreOptions { + /** Tenant scope for every read and write. Defaults to `''` (the single-tenant scope). */ readonly tenantId?: string | null; } @@ -34,7 +35,9 @@ export function createPostgresClientToolExecutionStore( sql: PostgresTaggedSql, opts: PostgresClientToolExecutionStoreOptions = {}, ): ClientToolExecutionStore { - const tenantId = opts.tenantId ?? null; + // `tenant_id` participates in the primary key, so a missing tenant collapses to the + // empty scope rather than NULL (a nullable column cannot be part of a Postgres key). + const tenantId = opts.tenantId ?? ''; return { async claim(key: ClientToolExecutionKey): Promise<'claimed' | ClientToolExecutionRecord> { @@ -42,7 +45,7 @@ export function createPostgresClientToolExecutionStore( INSERT INTO threadplane_client_tool_executions (tenant_id, thread_id, tool_call_id, status) VALUES (${tenantId}, ${key.threadId}, ${key.toolCallId}, 'executing') - ON CONFLICT (thread_id, tool_call_id) DO NOTHING + ON CONFLICT (tenant_id, thread_id, tool_call_id) DO NOTHING RETURNING status, result `; if (inserted.length > 0) return 'claimed'; @@ -50,7 +53,8 @@ export function createPostgresClientToolExecutionStore( const existing = await sql` SELECT status, result FROM threadplane_client_tool_executions - WHERE thread_id = ${key.threadId} + WHERE tenant_id = ${tenantId} + AND thread_id = ${key.threadId} AND tool_call_id = ${key.toolCallId} LIMIT 1 `; @@ -62,7 +66,7 @@ export function createPostgresClientToolExecutionStore( INSERT INTO threadplane_client_tool_executions (tenant_id, thread_id, tool_call_id, status, result) VALUES (${tenantId}, ${key.threadId}, ${key.toolCallId}, 'done', ${JSON.stringify(result)}::jsonb) - ON CONFLICT (thread_id, tool_call_id) DO UPDATE + ON CONFLICT (tenant_id, thread_id, tool_call_id) DO UPDATE SET status = 'done', result = CASE WHEN threadplane_client_tool_executions.status = 'done' @@ -81,7 +85,8 @@ export function createPostgresClientToolExecutionStore( const rows = await sql` SELECT tool_call_id, status, result FROM threadplane_client_tool_executions - WHERE thread_id = ${threadId} + WHERE tenant_id = ${tenantId} + AND thread_id = ${threadId} AND tool_call_id = ANY(${[...toolCallIds]}) `; const out: Record = {}; diff --git a/libs/middleware/src/postgres-client-tool-execution-store.spec.ts b/libs/middleware/src/postgres-client-tool-execution-store.spec.ts index ec21ba862..5cb888b2e 100644 --- a/libs/middleware/src/postgres-client-tool-execution-store.spec.ts +++ b/libs/middleware/src/postgres-client-tool-execution-store.spec.ts @@ -18,11 +18,13 @@ function makeSql(rows: unknown[][]): { sql: PostgresTaggedSql; queries: string[] } describe('THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA', () => { - it('creates the client-tool execution table with first-ship single-tenant primary key', () => { + it('creates the client-tool execution table with a tenant-scoped primary key', () => { expect(THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA).toContain('CREATE TABLE'); expect(THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA).toContain('threadplane_client_tool_executions'); - expect(THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA).toContain('tenant_id'); - expect(THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA).toContain('PRIMARY KEY (thread_id, tool_call_id)'); + expect(THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA).toContain("tenant_id text NOT NULL DEFAULT ''"); + expect(THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA).toContain( + 'PRIMARY KEY (tenant_id, thread_id, tool_call_id)', + ); }); }); @@ -33,7 +35,7 @@ describe('createPostgresClientToolExecutionStore', () => { await expect(store.claim({ threadId: 'thread-1', toolCallId: 'call-1' })).resolves.toBe('claimed'); - expect(queries[0]).toContain('ON CONFLICT (thread_id, tool_call_id) DO NOTHING'); + expect(queries[0]).toContain('ON CONFLICT (tenant_id, thread_id, tool_call_id) DO NOTHING'); expect(values[0]).toEqual(['tenant-1', 'thread-1', 'call-1']); }); @@ -57,9 +59,9 @@ describe('createPostgresClientToolExecutionStore', () => { await store.record({ threadId: 'thread-1', toolCallId: 'call-1' }, result); - expect(queries[0]).toContain('ON CONFLICT (thread_id, tool_call_id) DO UPDATE'); + expect(queries[0]).toContain('ON CONFLICT (tenant_id, thread_id, tool_call_id) DO UPDATE'); expect(queries[0]).toContain('WHEN threadplane_client_tool_executions.status ='); - expect(values[0]).toEqual([null, 'thread-1', 'call-1', JSON.stringify(result)]); + expect(values[0]).toEqual(['', 'thread-1', 'call-1', JSON.stringify(result)]); }); it('looks up records by requested tool_call_id', async () => { @@ -77,6 +79,77 @@ describe('createPostgresClientToolExecutionStore', () => { }); expect(queries[0]).toContain('tool_call_id = ANY'); - expect(values[0]).toEqual(['thread-1', ['call-1', 'call-2']]); + expect(values[0]).toEqual(['', 'thread-1', ['call-1', 'call-2']]); + }); + + it('scopes lookup by tenant', async () => { + const { sql, queries, values } = makeSql([[]]); + const store = createPostgresClientToolExecutionStore(sql, { tenantId: 'tenant-a' }); + + await store.lookup('thread-1', ['call-1']); + + expect(queries[0]).toContain('tenant_id'); + expect(values[0]).toContain('tenant-a'); + }); + + it('scopes claim by tenant', async () => { + const { sql, queries, values } = makeSql([[], []]); + const store = createPostgresClientToolExecutionStore(sql, { tenantId: 'tenant-a' }); + + await store.claim({ threadId: 'thread-1', toolCallId: 'call-1' }); + + // The follow-up SELECT after a no-op INSERT must be tenant-scoped too. + expect(queries[1]).toContain('tenant_id'); + expect(values[1]).toContain('tenant-a'); + }); + + it('scopes record by tenant', async () => { + const { sql, queries } = makeSql([[]]); + const store = createPostgresClientToolExecutionStore(sql, { tenantId: 'tenant-a' }); + + await store.record({ threadId: 'thread-1', toolCallId: 'call-1' }, { ok: true, value: 1 }); + + expect(queries[0]).toContain('ON CONFLICT (tenant_id, thread_id, tool_call_id) DO UPDATE'); + }); + + it('claims independently for a different tenant on the same thread and tool call', async () => { + // Tenant A claims, then records; tenant B's INSERT does not conflict because the + // primary key is widened with tenant_id, so its own claim returns 'claimed'. + const { sql, queries, values } = makeSql([ + [{ status: 'executing', result: null }], + [], + [{ status: 'executing', result: null }], + ]); + const storeA = createPostgresClientToolExecutionStore(sql, { tenantId: 'tenant-a' }); + const storeB = createPostgresClientToolExecutionStore(sql, { tenantId: 'tenant-b' }); + const key = { threadId: 'thread-1', toolCallId: 'call-1' }; + + await expect(storeA.claim(key)).resolves.toBe('claimed'); + await storeA.record(key, { ok: true, value: 'a' }); + await expect(storeB.claim(key)).resolves.toBe('claimed'); + + expect(queries[0]).toContain('ON CONFLICT (tenant_id, thread_id, tool_call_id) DO NOTHING'); + expect(values[0]).toEqual(['tenant-a', 'thread-1', 'call-1']); + expect(values[1]).toEqual(['tenant-a', 'thread-1', 'call-1', JSON.stringify({ ok: true, value: 'a' })]); + expect(values[2]).toEqual(['tenant-b', 'thread-1', 'call-1']); + }); + + it('dedupes a same-tenant claim after a record round trip', async () => { + const done = { ok: true as const, value: 'first' }; + const { sql, values } = makeSql([ + [{ status: 'executing', result: null }], + [], + [], + [{ status: 'done', result: done }], + ]); + const store = createPostgresClientToolExecutionStore(sql, { tenantId: 'tenant-a' }); + const key = { threadId: 'thread-1', toolCallId: 'call-1' }; + + await expect(store.claim(key)).resolves.toBe('claimed'); + await store.record(key, done); + // Second claim conflicts, so the tenant-scoped SELECT returns the recorded result. + await expect(store.claim(key)).resolves.toEqual({ status: 'done', result: done }); + + expect(values[3]).toEqual(['tenant-a', 'thread-1', 'call-1']); }); }); From de743f0ca2d3cdb9c9560150afdbb33238af76ec Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 10:45:45 -0300 Subject: [PATCH 10/13] fix(chat): never continue the run when settling a cancelled client tool --- .../client-tools/client-tool-executor.spec.ts | 242 +++++++++++++++--- .../lib/client-tools/client-tool-executor.ts | 171 ++++++++++--- .../client-tools-coordinator.spec.ts | 67 ++++- .../client-tools/client-tools-coordinator.ts | 35 ++- .../chat/chat.component.client-tools.spec.ts | 6 +- libs/chat/src/public-api.ts | 1 + 6 files changed, 441 insertions(+), 81 deletions(-) diff --git a/libs/chat/src/lib/client-tools/client-tool-executor.spec.ts b/libs/chat/src/lib/client-tools/client-tool-executor.spec.ts index 8365cd7bd..b723017c1 100644 --- a/libs/chat/src/lib/client-tools/client-tool-executor.spec.ts +++ b/libs/chat/src/lib/client-tools/client-tool-executor.spec.ts @@ -16,8 +16,12 @@ import type { ToolCall } from '../agent/tool-call'; // ── helpers ────────────────────────────────────────────────────────────────── -/** Drain the microtask queue a handful of times to let Promise chains settle. */ -async function drainMicrotasks(rounds = 4): Promise { +/** + * Drain the microtask queue a handful of times to let Promise chains settle. + * The executor races each handler against its abort signal, so a settlement is + * several ticks deep — keep this comfortably above the longest chain. + */ +async function drainMicrotasks(rounds = 12): Promise { for (let i = 0; i < rounds; i++) { await Promise.resolve(); } @@ -30,33 +34,45 @@ class FakeComponent {} function makeFakeCapability() { const pending = signal([]); + // `resolve` is "record AND continue" — both adapters submit a new run from + // it, so a call to `resolve` IS a run submission. const resolve = vi.fn<[string, ClientToolResult], void>(); + const settle = vi.fn<[string, ClientToolResult], void>(); + const flush = vi.fn<[], void>(); const capability: ClientToolsCapability = { setCatalog: vi.fn(), pending, + settle, + flush, resolve, }; - return { pending, resolve, capability }; + return { pending, resolve, settle, flush, capability }; } /** - * Capability that mirrors the adapters' real `pending` contract: `resolve()` - * marks the id resolved and `pending` drops resolved calls. Needed to prove a - * settled (incl. cancelled) call is never re-dispatched on a later effect pass. + * Capability that mirrors the adapters' real `pending` contract: BOTH `settle()` + * and `resolve()` route through a `settleResult` that marks the id resolved, and + * `pending` drops resolved calls. Needed to prove a settled (incl. cancelled) + * call is never re-dispatched on a later effect pass. */ function makeResolvingCapability() { const raw = signal([]); const resolvedIds = signal>(new Set()); const pending = computed(() => raw().filter((tc) => !resolvedIds().has(tc.id))); - const resolve = vi.fn<[string, ClientToolResult], void>((id) => { + const markResolved = (id: string): void => { resolvedIds.update((s) => new Set(s).add(id)); - }); + }; + const settle = vi.fn<[string, ClientToolResult], void>((id) => markResolved(id)); + const flush = vi.fn<[], void>(); + const resolve = vi.fn<[string, ClientToolResult], void>((id) => markResolved(id)); const capability: ClientToolsCapability = { setCatalog: vi.fn(), pending, + settle, + flush, resolve, }; - return { raw, resolve, capability }; + return { raw, resolve, settle, flush, capability }; } function makeFakeAgent(capability: ClientToolsCapability): Agent { @@ -255,7 +271,7 @@ describe('startClientToolExecutor()', () => { expect(seen[0].aborted).toBe(false); }); - it('aborts in-flight function tools on stop and settles them as cancelled', async () => { + it('aborts in-flight function tools on stop and settles them without continuing the run', async () => { let complete!: (value: string) => void; const completion = new Promise((resolve) => { complete = resolve; @@ -267,7 +283,7 @@ describe('startClientToolExecutor()', () => { return completion; }), }); - const { pending, resolve, capability } = makeFakeCapability(); + const { pending, resolve, settle, flush, capability } = makeFakeCapability(); const agent = makeFakeAgent(capability); TestBed.runInInjectionContext(() => { @@ -284,16 +300,47 @@ describe('startClientToolExecutor()', () => { complete('late result'); await drainMicrotasks(); - // The server thread must never hold a client tool call without a result: - // an aborted call settles with a cancelled error rather than dangling. - expect(resolve).toHaveBeenCalledOnce(); - expect(resolve.mock.calls[0][0]).toBe('slow-1'); - expect(resolve.mock.calls[0][1].ok).toBe(false); - expect((resolve.mock.calls[0][1] as { error: string }).error).toContain('cancelled'); + // The server thread must never hold a client tool call without a result, so + // an aborted call settles rather than dangling — but it must settle through + // settle()+flush(), NEVER resolve(): resolve submits a new run, which would + // undo the stop the user just asked for. + expect(resolve).not.toHaveBeenCalled(); + expect(settle).toHaveBeenCalledOnce(); + expect(settle.mock.calls[0][0]).toBe('slow-1'); + expect(settle.mock.calls[0][1].ok).toBe(false); + expect((settle.mock.calls[0][1] as { error: string }).error).toContain('cancelled'); + expect(flush).toHaveBeenCalled(); + }); + + it('submits no run when the user stops a client tool mid-flight', async () => { + const submittedRuns: string[] = []; + const registry = tools({ + slow: action('slow', z.object({}), async () => new Promise(() => undefined)), + }); + const { pending, resolve, capability } = makeFakeCapability(); + resolve.mockImplementation((id) => { + submittedRuns.push(id); + }); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + }); + + pending.set([{ id: 'slow-run', name: 'slow', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + await drainMicrotasks(8); + + expect(submittedRuns).toEqual([]); + expect(agent.submit).not.toHaveBeenCalled(); }); - it('settles an aborted handler so it cannot re-execute', async () => { - const settled: Array<[string, ClientToolResult]> = []; + it('settles an aborted handler through the non-continuing channel so it cannot re-execute', async () => { + const continued: Array<[string, ClientToolResult]> = []; + const cancelled: Array<[string, ClientToolResult]> = []; let release!: () => void; const registry = tools({ slow: action('Slow', z.object({}), async () => { @@ -308,7 +355,8 @@ describe('startClientToolExecutor()', () => { TestBed.runInInjectionContext(() => { startClientToolExecutor(agent, registry, { - settleToolCall: (tc, result) => settled.push([tc.id, result]), + settleToolCall: (tc, result) => continued.push([tc.id, result]), + settleWithoutContinuing: (tc, result) => cancelled.push([tc.id, result]), }); }); @@ -320,9 +368,70 @@ describe('startClientToolExecutor()', () => { release(); await drainMicrotasks(); - expect(settled).toHaveLength(1); - expect(settled[0][0]).toBe('slow-1'); - expect(settled[0][1].ok).toBe(false); + expect(continued).toEqual([]); + expect(cancelled).toHaveLength(1); + expect(cancelled[0][0]).toBe('slow-1'); + expect(cancelled[0][1].ok).toBe(false); + }); + + it('settles on abort without waiting for a handler that ignores the signal', async () => { + const cancelled: Array<[string, ClientToolResult]> = []; + const registry = tools({ + // Never settles, and never looks at context.signal — the common case, + // since honoring the signal is opt-in. + stubborn: action('Stubborn', z.object({}), () => new Promise(() => undefined)), + }); + const { pending, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry, { + settleWithoutContinuing: (tc, result) => cancelled.push([tc.id, result]), + }); + }); + + pending.set([{ id: 'stubborn-1', name: 'stubborn', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + await drainMicrotasks(); + + expect(cancelled).toHaveLength(1); + expect(cancelled[0][0]).toBe('stubborn-1'); + expect(cancelled[0][1].ok).toBe(false); + }); + + it('does not double-settle when a handler finishes after the abort settled it', async () => { + let release!: (value: string) => void; + const registry = tools({ + slow: action('Slow', z.object({}), async () => { + return new Promise((r) => { + release = r; + }); + }), + }); + const { pending, resolve, settle, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + }); + + pending.set([{ id: 'slow-3', name: 'slow', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + await drainMicrotasks(); + expect(settle).toHaveBeenCalledOnce(); + + // The real result arrives long after the cancelled settlement. + release('late real result'); + await drainMicrotasks(8); + + expect(settle).toHaveBeenCalledOnce(); + expect(resolve).not.toHaveBeenCalled(); }); it('does not re-dispatch an aborted client tool on a later effect pass', async () => { @@ -336,7 +445,7 @@ describe('startClientToolExecutor()', () => { const registry = tools({ slow: action('Slow', z.object({}), handler), }); - const { raw, resolve, capability } = makeResolvingCapability(); + const { raw, resolve, settle, capability } = makeResolvingCapability(); const agent = makeFakeAgent(capability); TestBed.runInInjectionContext(() => { @@ -352,13 +461,15 @@ describe('startClientToolExecutor()', () => { await drainMicrotasks(); // The next run re-emits the same tool call list; the cancelled call is now - // resolved, so it must not be dispatched to the handler a second time. + // settled (and so in resolvedIds), so it must not be dispatched to the + // handler a second time. raw.set([{ id: 'slow-2', name: 'slow', args: {}, status: 'complete' }]); TestBed.flushEffects(); await drainMicrotasks(); expect(handler).toHaveBeenCalledOnce(); - expect(resolve).toHaveBeenCalledOnce(); + expect(settle).toHaveBeenCalledOnce(); + expect(resolve).not.toHaveBeenCalled(); }); it('aborts in-flight function tools when the injection context is destroyed', async () => { @@ -385,6 +496,31 @@ describe('startClientToolExecutor()', () => { expect(seen[0].aborted).toBe(true); }); + it('settles on teardown without submitting a run mid-destroy', async () => { + const registry = tools({ + slow: action('slow', z.object({}), async () => new Promise(() => undefined)), + }); + const { pending, resolve, settle, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry); + }); + + pending.set([{ id: 'slow-4', name: 'slow', args: {}, status: 'complete' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + TestBed.resetTestingModule(); + await drainMicrotasks(); + + // Destroying the chat with a tool in flight must not submit a run. + expect(resolve).not.toHaveBeenCalled(); + expect(agent.submit).not.toHaveBeenCalled(); + expect(settle).toHaveBeenCalledOnce(); + expect(settle.mock.calls[0][1].ok).toBe(false); + }); + it('claims before executing guarded function tools and records before resolving', async () => { const order: string[] = []; const handler = vi.fn(async () => { @@ -538,7 +674,7 @@ describe('startClientToolExecutor()', () => { expect(resolve).toHaveBeenCalledWith('read-1', { ok: true, value: 'cached' }); }); - it('does not execute but still settles when stopped before a delayed claim resolves', async () => { + it('records and settles without continuing when stopped before a delayed claim resolves', async () => { let resolveClaim!: (value: 'claimed') => void; const claim = new Promise<'claimed'>((resolve) => { resolveClaim = resolve; @@ -549,7 +685,7 @@ describe('startClientToolExecutor()', () => { }); const store = makeGuardStore('claimed'); store.claim.mockReturnValue(claim); - const { pending, resolve, capability } = makeFakeCapability(); + const { pending, resolve, settle, capability } = makeFakeCapability(); const agent = makeFakeAgent(capability); TestBed.runInInjectionContext(() => { @@ -562,12 +698,49 @@ describe('startClientToolExecutor()', () => { await agent.stop(); resolveClaim('claimed'); + await drainMicrotasks(8); + + expect(handler).not.toHaveBeenCalled(); + // The claim succeeded, so the durable row is now 'executing'. It must be + // recorded or it stays that way forever and a later reload fails closed + // with a misleading "interrupted" message. + expect(store.record).toHaveBeenCalledOnce(); + expect(store.record.mock.calls[0][1].ok).toBe(false); + expect(resolve).not.toHaveBeenCalled(); + expect(settle).toHaveBeenCalledOnce(); + expect((settle.mock.calls[0][1] as { error: string }).error).toContain('cancelled'); + }); + + it('settles without continuing when the claim rejects after a stop', async () => { + let rejectClaim!: (err: Error) => void; + const claim = new Promise<'claimed'>((_res, rej) => { + rejectClaim = rej; + }); + const handler = vi.fn(async () => 'late'); + const registry = tools({ + charge: action('Charge a card', z.object({}), handler), + }); + const store = makeGuardStore('claimed'); + store.claim.mockReturnValue(claim); + const { pending, resolve, settle, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + + TestBed.runInInjectionContext(() => { + startClientToolExecutor(agent, registry, { executionGuard: makeGuard(store) }); + }); + + pending.set([{ id: 'charge-8', name: 'charge', args: {}, status: 'complete' }]); + TestBed.flushEffects(); await drainMicrotasks(); + await agent.stop(); + rejectClaim(new Error('store unavailable')); + await drainMicrotasks(8); + expect(handler).not.toHaveBeenCalled(); - expect(resolve).toHaveBeenCalledOnce(); - expect(resolve.mock.calls[0][1].ok).toBe(false); - expect((resolve.mock.calls[0][1] as { error: string }).error).toContain('cancelled'); + expect(resolve).not.toHaveBeenCalled(); + expect(settle).toHaveBeenCalledOnce(); + expect(settle.mock.calls[0][1].ok).toBe(false); }); it('records the cancelled result when stopped after claiming', async () => { @@ -582,7 +755,7 @@ describe('startClientToolExecutor()', () => { charge: action('Charge a card', z.object({}), handler), }); const store = makeGuardStore('claimed'); - const { pending, resolve, capability } = makeFakeCapability(); + const { pending, resolve, settle, capability } = makeFakeCapability(); const agent = makeFakeAgent(capability); TestBed.runInInjectionContext(() => { @@ -601,8 +774,9 @@ describe('startClientToolExecutor()', () => { // fail closed with a misleading "interrupted" message. expect(store.record).toHaveBeenCalledOnce(); expect(store.record.mock.calls[0][1].ok).toBe(false); - expect(resolve).toHaveBeenCalledOnce(); - expect(resolve.mock.calls[0][1].ok).toBe(false); + expect(resolve).not.toHaveBeenCalled(); + expect(settle).toHaveBeenCalledOnce(); + expect(settle.mock.calls[0][1].ok).toBe(false); }); it('wraps agent.stop once across repeated executor starts', () => { diff --git a/libs/chat/src/lib/client-tools/client-tool-executor.ts b/libs/chat/src/lib/client-tools/client-tool-executor.ts index cf117556b..434d28598 100644 --- a/libs/chat/src/lib/client-tools/client-tool-executor.ts +++ b/libs/chat/src/lib/client-tools/client-tool-executor.ts @@ -19,6 +19,8 @@ import { export interface ClientToolExecutorOptions { readonly executionGuard?: ClientToolExecutionGuard; readonly settleToolCall?: (toolCall: ToolCall, result: ClientToolResult) => void; + /** Settlement for calls that must NOT continue the run (user abort, teardown). */ + readonly settleWithoutContinuing?: (toolCall: ToolCall, result: ClientToolResult) => void; readonly shouldExecuteToolCall?: (toolCall: ToolCall) => boolean; } @@ -26,7 +28,7 @@ export interface ClientToolExecutorOptions { interface AgentStopPatch { readonly aborts: Set<() => void>; readonly originalStop: Agent['stop']; - readonly boundStop: () => Promise; + readonly wrapper: Agent['stop']; } /** Agents whose stop() this module has already wrapped. */ @@ -63,24 +65,50 @@ export function startClientToolExecutor( const originalStop = agent.stop; const boundStop = originalStop.bind(agent); const aborts = new Set<() => void>(); - patch = { aborts, originalStop, boundStop }; - patchedAgents.set(agent, patch); - agent.stop = async (): Promise => { + const wrapper = async (): Promise => { for (const abort of aborts) abort(); await boundStop(); }; + patch = { aborts, originalStop, wrapper }; + patchedAgents.set(agent, patch); + agent.stop = wrapper; } const registration = patch; registration.aborts.add(abortAll); destroyRef.onDestroy(() => { registration.aborts.delete(abortAll); if (registration.aborts.size === 0 && patchedAgents.get(agent) === registration) { - agent.stop = registration.originalStop; + // Only un-patch if OUR wrapper is still installed; something else may + // have replaced agent.stop since, and clobbering it would be worse. + if (agent.stop === registration.wrapper) agent.stop = registration.originalStop; patchedAgents.delete(agent); } }); destroyRef.onDestroy(abortAll); + const settleToolCall = + options.settleToolCall ?? ((toolCall, result) => cap.resolve(toolCall.id, result)); + + // `resolve()` contractually means "record AND continue" — both adapters + // submit a new run from it. A cancelled call must therefore NEVER go through + // it, or clicking Stop would immediately start another run and the model + // could re-call the very tool it was stopped on. settle() + flush() is the + // durable, run-free equivalent. If the capability has no settle(), we leave + // the call dangling on purpose: a dangling tool call is repaired on the next + // user turn, whereas resurrecting a stopped run is not repairable at all. + const settleWithoutContinuing = + options.settleWithoutContinuing ?? + ((toolCall: ToolCall, result: ClientToolResult) => { + if (!cap.settle) { + console.warn( + `Client tool "${toolCall.name}" was cancelled but the agent capability does not implement settle(); the result cannot be recorded without starting a run.`, + ); + return; + } + cap.settle(toolCall.id, result); + void Promise.resolve(cap.flush?.()).catch(() => undefined); + }); + effect(() => { for (const tc of cap.pending()) { const def = registry[tc.name]; @@ -101,7 +129,8 @@ export function startClientToolExecutor( toolCallId: tc.id, controller, executionGuard: options.executionGuard, - settleToolCall: options.settleToolCall ?? ((toolCall, result) => cap.resolve(toolCall.id, result)), + settleToolCall, + settleWithoutContinuing, }).finally(() => { inFlight.delete(tc.id); }); @@ -109,6 +138,44 @@ export function startClientToolExecutor( }); } +/** Outcome of racing a handler against its abort signal. */ +interface FunctionToolOutcome { + readonly aborted: boolean; + /** Present only when `aborted` is false. */ + readonly result?: ClientToolResult; +} + +/** + * Settle the moment the signal fires instead of waiting for the handler. + * + * Honoring `context.signal` is opt-in, and most handlers ignore it, so awaiting + * the handler would leave a stopped call unsettled — dangling on the server + * thread and pinned in `inFlight` — until it happens to finish, or forever. The + * handler's late real result is then discarded: the id is already settled and + * in the adapter's `resolvedIds`, so it can neither double-settle nor + * re-dispatch. `executeFunctionTool` normalizes throws, so the losing promise + * never rejects. + */ +async function raceAbort( + execution: Promise, + signal: AbortSignal, +): Promise { + if (signal.aborted) return { aborted: true }; + let onAbort: () => void = () => undefined; + const aborted = new Promise((resolve) => { + onAbort = () => resolve({ aborted: true }); + signal.addEventListener('abort', onAbort, { once: true }); + }); + try { + return await Promise.race([ + execution.then((result) => ({ aborted: signal.aborted, result })), + aborted, + ]); + } finally { + signal.removeEventListener('abort', onAbort); + } +} + async function runFunctionTool(input: { readonly def: AnyFunctionToolDef; readonly toolCall: ToolCall; @@ -117,16 +184,43 @@ async function runFunctionTool(input: { readonly controller: AbortController; readonly executionGuard?: ClientToolExecutionGuard; readonly settleToolCall: (toolCall: ToolCall, result: ClientToolResult) => void; + readonly settleWithoutContinuing: (toolCall: ToolCall, result: ClientToolResult) => void; }): Promise { - const { def, toolCall, rawArgs, toolCallId, controller, executionGuard, settleToolCall } = input; + const { + def, + toolCall, + rawArgs, + toolCallId, + controller, + executionGuard, + settleToolCall, + settleWithoutContinuing, + } = input; const signal = controller.signal; + // An aborted call MUST still settle: leaving it unsettled keeps it out of // `resolvedIds`, so it stays pending and is re-dispatched after the next run // (re-running a side-effecting handler the user explicitly stopped) and - // leaves the server thread holding a tool call with no tool result. + // leaves the server thread holding a tool call with no tool result. It must + // settle through the NON-continuing channel — see `settleWithoutContinuing`. + let settled = false; + /** Settle and continue the run. Only for outcomes the user did not cancel. */ + const settleAndContinue = (result: ClientToolResult): void => { + if (settled) return; + settled = true; + settleToolCall(toolCall, result); + }; + /** Settle without continuing the run. Abort and teardown paths only. */ + const settleCancelled = (result: ClientToolResult = cancelledClientToolResult(toolCallId)): void => { + if (settled) return; + settled = true; + settleWithoutContinuing(toolCall, result); + }; + if (!executionGuard || !shouldClaimBeforeExecute(def)) { - const result = await executeFunctionTool(def, rawArgs, { signal }); - settleToolCall(toolCall, signal.aborted ? cancelledClientToolResult(toolCallId) : result); + const outcome = await raceAbort(executeFunctionTool(def, rawArgs, { signal }), signal); + if (outcome.aborted) settleCancelled(); + else settleAndContinue(outcome.result as ClientToolResult); return; } @@ -135,65 +229,70 @@ async function runFunctionTool(input: { try { claim = await executionGuard.store.claim(key); } catch (err) { - if (!signal.aborted) settleToolCall(toolCall, clientToolGuardFailureResult(toolCallId, err)); + // A claim that rejects after the user stopped must still settle, but must + // never continue the run. + if (signal.aborted) settleCancelled(); + else settleAndContinue(clientToolGuardFailureResult(toolCallId, err)); return; } + if (signal.aborted) { - settleToolCall(toolCall, cancelledClientToolResult(toolCallId)); + // Aborted while the claim RPC was in flight. If the claim succeeded, the + // durable row is now `executing`; record the cancelled result or it stays + // that way forever and a later reload fails closed as "interrupted". + if (claim === 'claimed') { + await recordThenSettle(executionGuard, key, cancelledClientToolResult(toolCallId), toolCallId, settleCancelled); + } else { + settleCancelled(); + } return; } if (claim === 'claimed') { - const result = await executeFunctionTool(def, rawArgs, { signal }); - const finalResult = signal.aborted ? cancelledClientToolResult(toolCallId) : result; - await recordOrResolveGuardFailure( + const outcome = await raceAbort(executeFunctionTool(def, rawArgs, { signal }), signal); + const result = outcome.aborted + ? cancelledClientToolResult(toolCallId) + : (outcome.result as ClientToolResult); + await recordThenSettle( executionGuard, key, - finalResult, - toolCall, + result, toolCallId, - settleToolCall, + outcome.aborted ? settleCancelled : settleAndContinue, ); return; } if (claim.status === 'done') { - settleToolCall(toolCall, claim.result); + settleAndContinue(claim.result); return; } const result = claim.status === 'failed' && claim.result ? claim.result : defaultInterruptedClientToolResult(toolCallId); - await recordOrResolveGuardFailure( - executionGuard, - key, - result, - toolCall, - toolCallId, - settleToolCall, - ); + await recordThenSettle(executionGuard, key, result, toolCallId, settleAndContinue); } /** - * Record the final result then settle. Deliberately abort-agnostic: an aborted - * execution must still write its (cancelled) result, otherwise the guard store - * stays at `executing` forever and a later reload fails closed with a - * misleading "interrupted" message. + * Record the final result then settle through `settle`. Deliberately + * abort-agnostic: an aborted execution must still write its (cancelled) result, + * otherwise the guard store stays at `executing` forever and a later reload + * fails closed with a misleading "interrupted" message. The caller picks the + * settlement channel, so a cancelled result never continues the run. */ -async function recordOrResolveGuardFailure( +async function recordThenSettle( executionGuard: ClientToolExecutionGuard, key: ClientToolExecutionKey, result: ClientToolResult, - toolCall: ToolCall, toolCallId: string, - settleToolCall: (toolCall: ToolCall, result: ClientToolResult) => void, + settle: (result: ClientToolResult) => void, ): Promise { try { await executionGuard.store.record(key, result); } catch (err) { - settleToolCall(toolCall, clientToolGuardFailureResult(toolCallId, err)); + settle(clientToolGuardFailureResult(toolCallId, err)); return; } - settleToolCall(toolCall, result); + settle(result); } diff --git a/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts b/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts index cc034fbbb..fb0e0dafc 100644 --- a/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts +++ b/libs/chat/src/lib/client-tools/client-tools-coordinator.spec.ts @@ -17,8 +17,12 @@ import type { ToolCall } from '../agent/tool-call'; // ── helpers ────────────────────────────────────────────────────────────────── -/** Drain the microtask queue a handful of times to let Promise chains settle. */ -async function drainMicrotasks(rounds = 4): Promise { +/** + * Drain the microtask queue a handful of times to let Promise chains settle. + * The executor races each handler against its abort signal, so a settlement is + * several ticks deep — keep this comfortably above the longest chain. + */ +async function drainMicrotasks(rounds = 12): Promise { for (let i = 0; i < rounds; i++) { await Promise.resolve(); } @@ -277,6 +281,65 @@ describe('createClientToolsCoordinator()', () => { expect(resolve).toHaveBeenCalledWith('f2', { ok: true, value: 'B:LA' }); }); + it('settles cancelled function tools without submitting a follow-up run', async () => { + const registry = tools({ + slow: action('Slow', z.object({}), async () => new Promise(() => undefined)), + }); + const { pending, settle, flush, resolve, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + pending.set([{ id: 'c1', name: 'slow', args: {}, status: 'running' }]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + await drainMicrotasks(); + + // A single-call group with default followUp would normally resolve(), which + // submits a new run. A cancelled call must never take that path. + expect(resolve).not.toHaveBeenCalled(); + expect(agent.submit).not.toHaveBeenCalled(); + expect(settle).toHaveBeenCalledOnce(); + expect(settle.mock.calls[0][0]).toBe('c1'); + expect(settle.mock.calls[0][1].ok).toBe(false); + expect(flush).toHaveBeenCalledOnce(); + }); + + it('flushes a cancelled two-call group exactly once', async () => { + const registry = tools({ + slow_a: action('Slow A', z.object({}), async () => new Promise(() => undefined)), + slow_b: action('Slow B', z.object({}), async () => new Promise(() => undefined)), + }); + const { pending, settle, flush, resolve, capability } = makeFakeCapability(); + const agent = makeFakeAgent(capability); + const coordinator = createClientToolsCoordinator(registry); + + TestBed.runInInjectionContext(() => { + coordinator.connect(agent); + }); + + pending.set([ + { id: 'c1', name: 'slow_a', args: {}, status: 'running' }, + { id: 'c2', name: 'slow_b', args: {}, status: 'running' }, + ]); + TestBed.flushEffects(); + await drainMicrotasks(); + + await agent.stop(); + await drainMicrotasks(); + + expect(resolve).not.toHaveBeenCalled(); + expect(settle).toHaveBeenCalledTimes(2); + // Adapters coalesce concurrent flushes, so a per-call flush would strand + // every batch after the first: flush ONCE, when the last call settles. + expect(flush).toHaveBeenCalledOnce(); + }); + it('settles terminal tools and flushes once when a mixed group completes', async () => { const registry = tools({ terminal_card: view( diff --git a/libs/chat/src/lib/client-tools/client-tools-coordinator.ts b/libs/chat/src/lib/client-tools/client-tools-coordinator.ts index 3ff3eaa02..963271e51 100644 --- a/libs/chat/src/lib/client-tools/client-tools-coordinator.ts +++ b/libs/chat/src/lib/client-tools/client-tools-coordinator.ts @@ -146,6 +146,13 @@ export function createClientToolsCoordinator( ); } + /** No settle() and the run must not continue — the result cannot be recorded. */ + function warnUnrecordableWithoutSettle(tc: ToolCall): void { + console.warn( + `Client tool "${tc.name}" must not continue the run, but the agent capability does not implement settle(); the result cannot be recorded without starting a run.`, + ); + } + function flushSettledResults(cap: ClientToolsCapability): void { if (!cap.flush) { console.warn( @@ -163,6 +170,7 @@ export function createClientToolsCoordinator( agent: Agent, tc: ToolCall, result: ClientToolResult, + mayContinue = true, ): void { const group = groupFor(agent, cap, tc); if (group.settledIds.has(tc.id)) return; @@ -170,20 +178,27 @@ export function createClientToolsCoordinator( const groupComplete = Array.from(group.ids).every((id) => group.settledIds.has(id)); - // Over the continuation limit: still record the result so the server never - // keeps an unanswered tool call, but never continue the run. Keep the group - // as `currentGroup` so repeated effect passes over the same calls short- - // circuit on `settledIds` above. - if (!group.allowed) { + // Over the continuation limit, or cancelled by the user (stop / teardown): + // still record the result so the server never keeps an unanswered tool + // call, but never continue the run — `resolve()` submits a new run, which + // for a cancelled call would undo the very stop the user asked for. + if (!group.allowed || !mayContinue) { if (!cap.settle) { - warnMissingSettle(tc); + warnUnrecordableWithoutSettle(tc); return; } cap.settle(tc.id, result); - // Flush ONCE, when the last blocked call settles. Adapters coalesce + // Flush ONCE, when the last call in the group settles. Adapters coalesce // concurrent flushes (returning the in-flight promise), so flushing per // call would strand every batch after the first. - if (groupComplete) flushSettledResults(cap); + if (groupComplete) { + flushSettledResults(cap); + // A limit-blocked group must stay `currentGroup` so repeated effect + // passes over the same never-executed calls keep short-circuiting on + // `settledIds`. Cancelled calls leave `pending()` once settled, so + // their group can be retired normally. + if (group.allowed) currentGroup = undefined; + } return; } if (!cap.settle) { @@ -227,6 +242,10 @@ export function createClientToolsCoordinator( return false; }, settleToolCall: (tc, result) => settleClientToolCall(cap, agent, tc, result), + // Cancelled calls reuse the same group bookkeeping (so the flush-once- + // per-group property holds) but are forced down the settle+flush path. + settleWithoutContinuing: (tc, result) => + settleClientToolCall(cap, agent, tc, result, false), }); // function tools // Auto-ack `view` tools: they render but produce no user value. effect(() => { diff --git a/libs/chat/src/lib/compositions/chat/chat.component.client-tools.spec.ts b/libs/chat/src/lib/compositions/chat/chat.component.client-tools.spec.ts index d50d17f85..ebad2f558 100644 --- a/libs/chat/src/lib/compositions/chat/chat.component.client-tools.spec.ts +++ b/libs/chat/src/lib/compositions/chat/chat.component.client-tools.spec.ts @@ -118,7 +118,11 @@ const clientToolRegistry = tools({ ), }); -async function drainMicrotasks(rounds = 4): Promise { +/** + * The executor races each handler against its abort signal, so a settlement is + * several ticks deep — keep this comfortably above the longest chain. + */ +async function drainMicrotasks(rounds = 12): Promise { for (let i = 0; i < rounds; i++) await Promise.resolve(); } diff --git a/libs/chat/src/public-api.ts b/libs/chat/src/public-api.ts index 129692bf9..790a78f98 100644 --- a/libs/chat/src/public-api.ts +++ b/libs/chat/src/public-api.ts @@ -273,6 +273,7 @@ export { validateArgs, executeFunctionTool } from './lib/client-tools/execute'; export { startClientToolExecutor } from './lib/client-tools/client-tool-executor'; export type { ClientToolExecutorOptions } from './lib/client-tools/client-tool-executor'; export { + cancelledClientToolResult, clientToolGuardFailureResult, defaultInterruptedClientToolResult, shouldClaimBeforeExecute, From 91cd7ff5568af5f9ca89701b01f05d52da40b6b1 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 10:50:04 -0300 Subject: [PATCH 11/13] feat(examples): add terminal client tool to the chat demo --- .../chat/angular/src/app/client-tools.spec.ts | 9 ++ examples/chat/angular/src/app/client-tools.ts | 11 ++ .../src/app/trip-summary-card.component.ts | 136 ++++++++++++++++++ examples/chat/python/src/graph.py | 5 +- 4 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 examples/chat/angular/src/app/trip-summary-card.component.ts diff --git a/examples/chat/angular/src/app/client-tools.spec.ts b/examples/chat/angular/src/app/client-tools.spec.ts index 1d12359d7..99d205c8e 100644 --- a/examples/chat/angular/src/app/client-tools.spec.ts +++ b/examples/chat/angular/src/app/client-tools.spec.ts @@ -15,4 +15,13 @@ describe('itineraryClientTools (langgraph demo)', () => { expect(names).toContain('clear_day'); expect(names).not.toContain('get_itinerary'); }); + + it('declares show_trip_summary as a terminal view tool (followUp: false)', () => { + const registry = TestBed.runInInjectionContext(() => itineraryClientTools()); + const summary = (registry as Record) + .show_trip_summary; + expect(summary).toBeDefined(); + expect(summary.kind).toBe('view'); + expect(summary.followUp).toBe(false); + }); }); diff --git a/examples/chat/angular/src/app/client-tools.ts b/examples/chat/angular/src/app/client-tools.ts index 3c8a0f065..fc0c93dcf 100644 --- a/examples/chat/angular/src/app/client-tools.ts +++ b/examples/chat/angular/src/app/client-tools.ts @@ -6,6 +6,7 @@ import { ItineraryStore } from './itinerary-store'; import { GeocodingService } from './geocoding.service'; import { DayCardComponent, DAY_CARD_SCHEMA } from './day-card.component'; import { ClearDayConfirmComponent } from './clear-day-confirm.component'; +import { TripSummaryCardComponent, TRIP_SUMMARY_SCHEMA } from './trip-summary-card.component'; /** Schema for the `clear_day` ask tool — exported in case consumers want to * derive types from it (e.g. `ViewProps`). */ @@ -65,5 +66,15 @@ export function itineraryClientTools(): ClientToolRegistry { DAY_CARD_SCHEMA, DayCardComponent, ), + // The demo's TERMINAL client tool. `followUp: false` means the tool result + // is recorded without forcing a continuation run — the turn ends when the + // card mounts. The result is still flushed to the durable thread, so the + // next user message doesn't hit a dangling-tool-call provider error. + show_trip_summary: view( + 'Show a final trip summary card recapping every day of the itinerary. Call this last — it ends the turn and needs no follow-up.', + TRIP_SUMMARY_SCHEMA, + TripSummaryCardComponent, + { followUp: false }, + ), }); } diff --git a/examples/chat/angular/src/app/trip-summary-card.component.ts b/examples/chat/angular/src/app/trip-summary-card.component.ts new file mode 100644 index 000000000..7286f2703 --- /dev/null +++ b/examples/chat/angular/src/app/trip-summary-card.component.ts @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: MIT +import { ChangeDetectionStrategy, Component, computed, input } from '@angular/core'; +import type { ViewProps } from '@threadplane/chat'; +import { z } from 'zod/v4'; + +/** + * Schema for the `show_trip_summary` view tool — co-located with the component + * so the inputs and the schema shape can be kept in sync at a glance. + * `client-tools.ts` imports this schema to pass to + * `view(…, TRIP_SUMMARY_SCHEMA, …, { followUp: false })`. + */ +export const TRIP_SUMMARY_SCHEMA = z.object({ + title: z.string(), + days: z.array( + z.object({ + day: z.number().int().min(1), + places: z.array(z.string()), + }), + ), + note: z.string().optional(), +}); + +/** Input types derived directly from the `show_trip_summary` schema — + * guarantees this component stays compatible with the view() check at + * compile time. */ +type Inputs = ViewProps; + +/** + * A frontend-owned view rendered for the `show_trip_summary` client tool. + * + * This is the demo's TERMINAL client tool: it is declared with + * `followUp: false`, so mounting this card acknowledges the tool call and the + * turn ENDS — there is no follow-up model turn. The tool result is still + * written back to the durable thread (the LangGraph adapter flushes the + * buffered `ToolMessage` even though no continuation run is started), which is + * what keeps the next user message from hitting a provider 400 over an + * `AIMessage(tool_calls=[…])` with no matching `ToolMessage`. + */ +@Component({ + selector: 'app-trip-summary-card', + standalone: true, + changeDetection: ChangeDetectionStrategy.OnPush, + template: ` +
+
+

{{ title() }}

+

{{ dayCount() }} days · {{ stopCount() }} stops

+
+
    + @for (d of days(); track d.day) { +
  1. + Day {{ d.day }} + {{ d.places.join(' → ') || 'No stops' }} +
    2. + } @empty { +
  2. Nothing planned yet
    3. + } +
+ @if (note()) { +

{{ note() }}

+ } +

Trip summary — end of turn

+
+ `, + styles: [ + ` + .tsc { + border: 1px solid var(--tplane-chat-separator, #e5e7eb); + border-radius: var(--tplane-chat-radius-card, 12px); + background: var(--tplane-chat-surface-alt, transparent); + color: var(--tplane-chat-text, inherit); + font-family: var(--tplane-chat-font-family, inherit); + padding: 16px; + max-width: 360px; + } + .tsc__head { + margin-bottom: 12px; + } + .tsc__title { + margin: 0; + font-size: 1rem; + font-weight: 600; + } + .tsc__meta { + margin: 4px 0 0; + font-size: var(--tplane-chat-font-size-sm, 0.8125rem); + color: var(--tplane-chat-text-muted, inherit); + } + .tsc__days { + list-style: none; + margin: 0; + padding: 0; + display: flex; + flex-direction: column; + gap: 8px; + } + .tsc__day { + display: flex; + flex-direction: column; + gap: 2px; + } + .tsc__day-label { + font-size: var(--tplane-chat-font-size-sm, 0.8125rem); + font-weight: 600; + color: var(--tplane-chat-primary, inherit); + } + .tsc__day-places { + opacity: 0.9; + } + .tsc__day--empty { + opacity: 0.5; + } + .tsc__note { + margin: 12px 0 0; + opacity: 0.9; + } + .tsc__end { + margin: 12px 0 0; + padding-top: 8px; + border-top: 1px solid var(--tplane-chat-separator, #e5e7eb); + font-size: var(--tplane-chat-font-size-sm, 0.8125rem); + color: var(--tplane-chat-text-muted, inherit); + } + `, + ], +}) +export class TripSummaryCardComponent { + readonly title = input.required(); + readonly days = input([]); + readonly note = input(undefined); + + protected readonly dayCount = computed(() => this.days().length); + protected readonly stopCount = computed(() => + this.days().reduce((n, d) => n + (d.places?.length ?? 0), 0), + ); +} diff --git a/examples/chat/python/src/graph.py b/examples/chat/python/src/graph.py index b694f995e..ff4a938d4 100644 --- a/examples/chat/python/src/graph.py +++ b/examples/chat/python/src/graph.py @@ -189,7 +189,10 @@ async def generate_title(state: "State", config: RunnableConfig) -> dict: "days, and POPULATE the itinerary by calling `add_stop` for each recommendation " "(then `day_card` to recap a day). Revise with `move_stop`/`reorder_stop`/`clear_day`. " "Do NOT just describe the plan in prose — call the tools so the map and panel update. " - "Only add stops that are not already present." + "Only add stops that are not already present. " + "When the user asks for a recap/summary of the whole trip, finish by calling " + "`show_trip_summary` with every day — it is terminal, so make it the LAST tool " + "call of the turn and add no prose after it." ) # Reasoning-capable model prefixes. We only attach the ``reasoning`` From 6c25a186634299563bf0259ee760284c82089cee Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 10:50:57 -0300 Subject: [PATCH 12/13] docs: regenerate API docs for client-tool flush surface Co-Authored-By: Claude Opus 5 --- .../content/docs/chat/api/api-docs.json | 31 +++++++++++++++++++ .../content/docs/middleware/api/api-docs.json | 4 +-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/apps/website/content/docs/chat/api/api-docs.json b/apps/website/content/docs/chat/api/api-docs.json index 79895d5e7..c3cf53adf 100644 --- a/apps/website/content/docs/chat/api/api-docs.json +++ b/apps/website/content/docs/chat/api/api-docs.json @@ -6952,6 +6952,12 @@ "description": "", "optional": true }, + { + "name": "settleWithoutContinuing", + "type": "(toolCall: ToolCall, result: ClientToolResult) => void", + "description": "Settlement for calls that must NOT continue the run (user abort, teardown).", + "optional": true + }, { "name": "shouldExecuteToolCall", "type": "(toolCall: ToolCall) => boolean", @@ -7024,6 +7030,12 @@ } ], "methods": [ + { + "name": "flush", + "signature": "flush(): void | Promise", + "description": "Make every result recorded via settle durable on the server\nWITHOUT continuing the run. No-op for adapters whose settle() is already\ndurable. Adapters that buffer locally MUST clear their buffer only on a\nsuccessful write, so a failure degrades to a later flush or submit.", + "params": [] + }, { "name": "resolve", "signature": "resolve(toolCallId: string, result: ClientToolResult): void", @@ -8698,6 +8710,25 @@ }, "examples": [] }, + { + "name": "cancelledClientToolResult", + "kind": "function", + "description": "Result recorded when the user stops a run while a client tool is running.", + "signature": "cancelledClientToolResult(toolCallId: string): ClientToolResult", + "params": [ + { + "name": "toolCallId", + "type": "string", + "description": "", + "optional": false + } + ], + "returns": { + "type": "ClientToolResult", + "description": "" + }, + "examples": [] + }, { "name": "citationSourceVisual", "kind": "function", diff --git a/apps/website/content/docs/middleware/api/api-docs.json b/apps/website/content/docs/middleware/api/api-docs.json index 4bcf673da..3df551fab 100644 --- a/apps/website/content/docs/middleware/api/api-docs.json +++ b/apps/website/content/docs/middleware/api/api-docs.json @@ -381,7 +381,7 @@ { "name": "tenantId", "type": "string | null", - "description": "", + "description": "Tenant scope for every read and write. Defaults to `''` (the single-tenant scope).", "optional": true } ], @@ -472,7 +472,7 @@ "name": "THREADPLANE_CLIENT_TOOL_EXECUTIONS_SCHEMA", "kind": "const", "description": "", - "signature": "\"\\nCREATE TABLE IF NOT EXISTS threadplane_client_tool_executions (\\n tenant_id text,\\n thread_id text NOT NULL,\\n tool_call_id text NOT NULL,\\n status text NOT NULL,\\n result jsonb,\\n created_at timestamptz NOT NULL DEFAULT now(),\\n updated_at timestamptz NOT NULL DEFAULT now(),\\n PRIMARY KEY (thread_id, tool_call_id)\\n);\\n\"", + "signature": "\"\\nCREATE TABLE IF NOT EXISTS threadplane_client_tool_executions (\\n tenant_id text NOT NULL DEFAULT '',\\n thread_id text NOT NULL,\\n tool_call_id text NOT NULL,\\n status text NOT NULL,\\n result jsonb,\\n created_at timestamptz NOT NULL DEFAULT now(),\\n updated_at timestamptz NOT NULL DEFAULT now(),\\n PRIMARY KEY (tenant_id, thread_id, tool_call_id)\\n);\\n\"", "examples": [] }, { From 92384464eb9ea40af4d6f2340511d1ba92ca2f21 Mon Sep 17 00:00:00 2001 From: Brian Love Date: Fri, 7 Aug 2026 11:20:36 -0300 Subject: [PATCH 13/13] fix(langgraph): chain concurrent flushes and drop stale-thread client tool results --- libs/langgraph/src/lib/agent.fn.spec.ts | 74 ++++++++++ libs/langgraph/src/lib/agent.fn.ts | 18 ++- libs/langgraph/src/lib/client-tools.spec.ts | 28 ++++ libs/langgraph/src/lib/client-tools.ts | 148 +++++++++++++++----- 4 files changed, 226 insertions(+), 42 deletions(-) diff --git a/libs/langgraph/src/lib/agent.fn.spec.ts b/libs/langgraph/src/lib/agent.fn.spec.ts index 34e267ba8..24ca4a279 100644 --- a/libs/langgraph/src/lib/agent.fn.spec.ts +++ b/libs/langgraph/src/lib/agent.fn.spec.ts @@ -1428,3 +1428,77 @@ describe('agent — client tool staging', () => { expect(cap.drainToolMessages()).toEqual([]); }); }); + +// ── Stale-thread guard ─────────────────────────────────────────────────────── +// A tool handler still in flight when the user clicks another thread settles +// AFTER the switch. Its ToolMessage belongs to the thread whose AIMessage +// produced the tool_call_id, so it must never reach the new thread. + +describe('agent — client tool results settled after a thread switch', () => { + beforeEach(() => TestBed.configureTestingModule({})); + + const SPEC = { name: 'get_weather', description: 'w', parameters: {} }; + + function staging(ref: { clientTools: unknown }) { + return ref.clientTools as { + setCatalog(specs: unknown[]): void; + settle(id: string, result: { ok: true; value: unknown }): void; + flush(): Promise; + drainToolMessages(): Array<{ tool_call_id: string }>; + }; + } + + /** Agent on t-1 whose transcript holds one pending client tool call. */ + async function agentWithPendingToolCall(transport: MockAgentTransport) { + const ref = withInjectionContext(() => + agent({ apiUrl: '', assistantId: 'a', threadId: 't-1', transport, throttle: false }) + ); + const cap = staging(ref); + cap.setCatalog([SPEC]); + ref.submit({ message: 'weather?' }); + transport.emit([{ + type: 'messages', + messages: [{ + id: 'ai-1', type: 'ai', content: '', + tool_calls: [{ id: 'tc-1', name: 'get_weather', args: {} }], + }], + }]); + transport.close(); + await new Promise(r => setTimeout(r, 30)); + return { ref, cap }; + } + + it('drops a result settled after a thread switch instead of writing it to the new thread', async () => { + const transport = new MockAgentTransport(); + const updateCalls: Array<{ threadId: string; values: Record }> = []; + (transport as unknown as { + updateState: (t: string, v: Record, s: AbortSignal) => Promise; + }).updateState = async (threadId, values) => { updateCalls.push({ threadId, values }); }; + + const { ref, cap } = await agentWithPendingToolCall(transport); + ref.switchThread('t-2'); + cap.settle('tc-1', { ok: true, value: 'sunny' }); + await cap.flush(); + + // Writing here would give t-2 a ToolMessage matching no AIMessage → 400. + expect(updateCalls).toHaveLength(0); + expect(cap.drainToolMessages()).toEqual([]); + }); + + it('does not drain a result settled after a thread switch into the new thread submit', async () => { + const transport = new MockAgentTransport(); + const { ref, cap } = await agentWithPendingToolCall(transport); + const streamsBefore = transport.streams.length; + + ref.switchThread('t-2'); + cap.settle('tc-1', { ok: true, value: 'sunny' }); + ref.submit({ message: 'hello on the new thread' }); + + const payload = transport.streams[streamsBefore]?.payload as { + messages: Array>; + }; + expect(payload.messages).toHaveLength(1); + expect(payload.messages[0]).toMatchObject({ type: 'human' }); + expect(cap.drainToolMessages()).toEqual([]); + }); +}); diff --git a/libs/langgraph/src/lib/agent.fn.ts b/libs/langgraph/src/lib/agent.fn.ts index f94136ecc..2bea4ffdd 100644 --- a/libs/langgraph/src/lib/agent.fn.ts +++ b/libs/langgraph/src/lib/agent.fn.ts @@ -193,10 +193,11 @@ export function agent< const custom$ = new BehaviorSubject([]); const hasValue$ = new BehaviorSubject(false); - // Assigned once the client-tools capability exists (further down — the - // capability needs `manager`, which needs these subjects). Called through a - // forward reference so the thread-change seam below stays in one place. - let clearStagedToolMessages: (() => void) | undefined; + // Forward reference. The client-tools capability is built much further down + // (it needs `manager`, which needs these subjects), but the thread-change + // seam lives here. A holder keeps the binding itself a `const` while its + // member is filled in later. + const clientToolStaging: { clear?: () => void } = {}; function resetDerivedThreadState(): void { status$.next(ResourceStatus.Idle); @@ -205,7 +206,9 @@ export function agent< // Staged client-tool results belong to the thread whose AIMessage produced // their tool_call_ids. Carrying them into a different thread would prepend // a ToolMessage that matches no tool call there — a 400 on that turn. - clearStagedToolMessages?.(); + // Runs BEFORE manager.switchThread resets the store, so the capability can + // still read the outgoing thread's tool calls. + clientToolStaging.clear?.(); } // Track hasValue — becomes true once values or messages arrive @@ -457,8 +460,11 @@ export function agent< await manager.updateState({ messages: [...messages] }); } : undefined, + // Stamps each staged result with the thread it was settled on, so a write + // can never land on a thread the user has since moved to. + () => manager.currentThreadId, ); - clearStagedToolMessages = () => clientToolsCap.clearStagedToolMessages(); + clientToolStaging.clear = () => clientToolsCap.clearStagedToolMessages(); return { // ── Runtime-neutral surface (AgentWithHistory) ──────────────────────── diff --git a/libs/langgraph/src/lib/client-tools.spec.ts b/libs/langgraph/src/lib/client-tools.spec.ts index b0aab794f..c13e7dad2 100644 --- a/libs/langgraph/src/lib/client-tools.spec.ts +++ b/libs/langgraph/src/lib/client-tools.spec.ts @@ -511,6 +511,34 @@ describe('flush', () => { expect(cap.drainToolMessages().map((m) => m.tool_call_id)).toEqual(['t3']); }); + it('persists a batch staged while an earlier flush is still in flight', async () => { + // The abort path in startClientToolExecutor flushes once PER settled call, + // so two adjacent flushes are routine. The second must not be swallowed by + // the in-flight guard — its batch was staged after the first took its + // snapshot, so returning the first promise would leave it unwritten. + const releases: Array<() => void> = []; + const persist = vi.fn( + () => new Promise((resolve) => { releases.push(resolve); }), + ); + const { cap } = setup(persist); + + cap.settle?.('t1', { ok: true, value: 'a' }); + const first = cap.flush?.(); + cap.settle?.('t2', { ok: true, value: 'b' }); + const second = cap.flush?.(); + + releases[0](); + for (let i = 0; i < 50 && releases.length < 2; i++) await Promise.resolve(); + releases[1]?.(); + await Promise.all([first, second]); + + const persisted = (persist as unknown as ReturnType).mock.calls + .flatMap((call) => (call[0] as Array<{ tool_call_id: string }>) + .map((m) => m.tool_call_id)); + expect(persisted).toEqual(['t1', 't2']); + expect(cap.drainToolMessages()).toEqual([]); + }); + it('coalesces overlapping flush calls into a single persist call', async () => { let releasePersist!: () => void; const persist = vi.fn( diff --git a/libs/langgraph/src/lib/client-tools.ts b/libs/langgraph/src/lib/client-tools.ts index 651d66397..0bacdacf8 100644 --- a/libs/langgraph/src/lib/client-tools.ts +++ b/libs/langgraph/src/lib/client-tools.ts @@ -85,6 +85,18 @@ export type PersistToolMessagesFn = ( messages: readonly BufferedToolMessage[], ) => Promise; +/** Reads the thread a write would currently land on. */ +export type CurrentThreadIdFn = () => string | null; + +/** + * A buffered tool message plus the thread it was settled on. The stamp is + * internal bookkeeping and never reaches the wire — only `message` is sent. + */ +interface StagedToolMessage { + readonly threadId: string | null; + readonly message: BufferedToolMessage; +} + /** * Prepend staged tool messages to a run payload's message list. * @@ -147,6 +159,7 @@ export function createClientToolsCapability( submitFn: SubmitFn, store: ClientToolsStore, persistFn?: PersistToolMessagesFn, + currentThreadIdFn?: CurrentThreadIdFn, ): ClientToolsCapability & { catalog: Signal; drainToolMessages(): BufferedToolMessage[]; @@ -154,11 +167,16 @@ export function createClientToolsCapability( } { const catalog = signal([]); const resolvedIds = signal>(new Set()); - const toolMessageBuffer: BufferedToolMessage[] = []; + const toolMessageBuffer: StagedToolMessage[] = []; let flushInFlight: Promise | undefined; // Bumped whenever the buffer is discarded, so an in-flight flush can tell // whether its batch still belongs to the current thread. let bufferGeneration = 0; + // Tool calls belonging to threads we have left. A handler still running when + // the user switches threads settles AFTER the switch, by which point both the + // store and the current thread id already describe the NEW thread — the id is + // the only durable way left to recognise the result as stale. + const retiredToolCallIds = new Set(); const pending = computed(() => { // Client tools are only actionable after the run ends (the backend @@ -199,12 +217,85 @@ export function createClientToolsCapability( ? safeStringify(value) : `Error: ${error}`; + // The tool call belongs to a thread the user has already left, so there is + // nowhere valid to send this result: the current thread has no matching + // tool call, and the old thread is no longer the write target. + if (retiredToolCallIds.has(id)) { + console.warn( + `Discarding client tool result for ${id}: its thread is no longer active.`, + ); + return; + } + // Message shape: both `type` and `role` are set for compatibility — // the LangGraph server's add_messages coercion reads `role` (Python // side), while the bridge's local optimistic-message path reads `type` // (via toMessage's normalizeMessageType). This mirrors the human-message // shape used in buildSubmitUpdate (agent.fn.ts line 732). - toolMessageBuffer.push({ type: 'tool', role: 'tool', tool_call_id: id, content }); + toolMessageBuffer.push({ + threadId: currentThreadIdFn?.() ?? null, + message: { type: 'tool', role: 'tool', tool_call_id: id, content }, + }); + } + + /** + * Remove every staged entry, returning only those still valid for the thread + * a write would land on right now. An entry is stale when it was stamped with + * a different thread — dropped rather than misdelivered. + * + * A null on either side means "thread not tracked yet" (no threadId option + * and no run has reported one); those are kept, since there is no evidence of + * a switch and dropping them would lose results on untracked transports. + */ + function takeStagedForCurrentThread(): StagedToolMessage[] { + const current = currentThreadIdFn?.() ?? null; + const taken = toolMessageBuffer.splice(0, toolMessageBuffer.length); + return taken.filter((entry) => { + const stale = + entry.threadId !== null && current !== null && entry.threadId !== current; + if (stale) { + console.warn( + `Discarding a client tool result staged for thread ${entry.threadId}; ` + + `the active thread is now ${current}.`, + ); + } + return !stale; + }); + } + + /** + * Persist one batch. Takes ownership of the buffer at snapshot time: + * resolve() and drainToolMessages() clear the buffer unconditionally and know + * nothing about an in-flight write, so anything left staged across the await + * could be re-sent (a duplicate ToolMessage for one tool_call_id) or removed + * by the wrong index (dropping a result that was never persisted). + */ + function runFlush(): Promise { + if (!persistFn) return Promise.resolve(); + const staged = takeStagedForCurrentThread(); + if (staged.length === 0) return Promise.resolve(); + + const generation = bufferGeneration; + const batch = staged.map((entry) => entry.message); + const inFlight = persistFn(batch) + .catch((err: unknown) => { + // Re-stage at the FRONT so ordering is preserved for the next drain — + // unless the buffer was cleared meanwhile (thread switch), in which + // case these results belong to a thread we have left. + if (generation === bufferGeneration) { + toolMessageBuffer.unshift(...staged); + } + console.warn( + `Client tool flush failed; ${batch.length} result(s) remain staged for the next run.`, + err, + ); + }) + .finally(() => { + // Only clear if no later flush has already claimed the slot. + if (flushInFlight === inFlight) flushInFlight = undefined; + }); + flushInFlight = inFlight; + return inFlight; } const capability: ClientToolsCapability & { @@ -224,11 +315,9 @@ export function createClientToolsCapability( settleResult(id, result); }, - /** Remove and return every buffered tool message. */ + /** Remove and return every buffered tool message valid for this thread. */ drainToolMessages(): BufferedToolMessage[] { - const drained = [...toolMessageBuffer]; - toolMessageBuffer.length = 0; - return drained; + return takeStagedForCurrentThread().map((entry) => entry.message); }, /** @@ -237,6 +326,10 @@ export function createClientToolsCapability( * its tool_call_id, so carrying it over would poison the new thread. */ clearStagedToolMessages(): void { + // Retire the outgoing thread's tool calls so a handler that settles after + // the switch is recognised as stale. Read the store BEFORE it resets — + // agent.fn.ts calls this ahead of manager.switchThread for that reason. + for (const toolCall of store.toolCalls()) retiredToolCallIds.add(toolCall.id); toolMessageBuffer.length = 0; // Invalidate any in-flight flush so its failure path cannot re-stage the // old thread's messages into the new thread's buffer. @@ -244,35 +337,19 @@ export function createClientToolsCapability( }, flush(): Promise { - if (flushInFlight) return flushInFlight; - if (toolMessageBuffer.length === 0) return Promise.resolve(); if (!persistFn) return Promise.resolve(); - - // Take ownership of the batch NOW. resolve() and drainToolMessages() both - // clear the buffer unconditionally and know nothing about an in-flight - // write, so leaving the batch in place across the await would let them - // re-send what this write already covers (a duplicate ToolMessage for one - // tool_call_id) and let the completion splice remove the wrong elements - // (dropping a result that was never persisted). - const batch = toolMessageBuffer.splice(0, toolMessageBuffer.length); - const generation = bufferGeneration; - flushInFlight = persistFn(batch) - .catch((err: unknown) => { - // Re-stage at the FRONT so ordering is preserved for the next drain — - // unless the buffer was cleared meanwhile (thread switch), in which - // case these results belong to a thread we have left. - if (generation === bufferGeneration) { - toolMessageBuffer.unshift(...batch); - } - console.warn( - `Client tool flush failed; ${batch.length} result(s) remain staged for the next run.`, - err, - ); - }) - .finally(() => { - flushInFlight = undefined; - }); - return flushInFlight; + if (flushInFlight) { + // Chain rather than short-circuit. The caller's batch may have been + // staged AFTER the in-flight write took its snapshot, so returning that + // promise would resolve without ever persisting it — which is exactly + // what happens when an abort fires one flush per settled call. The + // chain terminates because runFlush() returns immediately once the + // buffer is empty. + const chained = flushInFlight.then(() => runFlush()); + flushInFlight = chained; + return chained; + } + return runFlush(); }, resolve(id: string, result: ClientToolResult): void { @@ -281,10 +358,9 @@ export function createClientToolsCapability( // appends the ToolMessages to the thread state. `client_tools` is // included so the model sees the full tool catalog on the continuation. const toolPayload = { - messages: [...toolMessageBuffer], + messages: takeStagedForCurrentThread().map((entry) => entry.message), client_tools: catalog(), }; - toolMessageBuffer.length = 0; void submitFn(toolPayload); },