Skip to content

fix(chat): never leave a client tool call unanswered on the server - #807

Merged
blove merged 13 commits into
mainfrom
blove/client-tool-followup-fix
Aug 7, 2026
Merged

fix(chat): never leave a client tool call unanswered on the server#807
blove merged 13 commits into
mainfrom
blove/client-tool-followup-fix

Conversation

@blove

@blove blove commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes five defects found reviewing the shipped client-tool continuation stack (#782#805). Four violate one invariant the architecture depended on but never stated:

The server thread must never hold a client tool call without a corresponding tool result.

When violated, the thread holds an AIMessage(tool_calls=[…]) with no ToolMessage, and most providers reject that history with a 400 on the next user turn.

The central change

ClientToolsCapability gains flush(), making the three settlement verbs distinct:

Verb Meaning
settle(id, result) Record locally. Never continues.
flush() New. Make everything staged durable server-side. Never continues.
resolve(id, result) Record + continue, in one run.

AG-UI's flush() is a no-op — its settle() already calls addMessage. LangGraph's batches the buffer into one threads.updateState write (no asNode, so add_messages appends and the resume point is untouched), falling back to draining into the next submit() when the transport has no updateState.

The five defects

  1. followUp: false corrupted LangGraph threads. When every tool in a group was terminal, resolve() was never called — and it was the only thing that drained the buffer. Results never reached the server. AG-UI was immune, which is what made the asymmetry easy to miss.
  2. Aborted tools re-executed. The abort path returned without settling, so the call stayed in pending() and was re-dispatched after the next run — a second execution of a side-effecting handler the user explicitly stopped.
  3. The max-turns guard discarded results. An ask tool's user-supplied answer was thrown away and the calls left unsettled. Now real results are preserved and only never-executed tools get a limit error.
  4. agent.stop wrappers stacked without bound. Now wrapped once per agent via a registry and restored on destroy.
  5. Postgres tenant_id was written but never filtered on. Now NOT NULL DEFAULT '', in the primary key and every WHERE/conflict target.

Behavior changes worth calling out

  • 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 hold the invariant, but both are observable.

Schema change

threadplane_client_tool_executions gains tenant_id in its primary key. No migration tooling — the table is recreated. Authorized explicitly; no backward compatibility required.

Verification

Live, against a real LLM and a real LangGraph backend — the decisive test, because the failure is a provider rejection of persisted server state that no unit test can reproduce:

  1. Model called the terminal show_trip_summary tool → no follow-up run fired, and GET /threads/{id}/state showed the ToolMessage present. Unanswered tool calls: none.
  2. Reloaded, sent another message → normal reply, no 400. This is the scenario that was broken.
  3. Zero console errors; zero tool-call errors in the backend log.

Automated: 1102 passing in chat, 294 in langgraph, 206 in ag-ui, 46 in middleware, 103 in examples-chat-angular. Lint errors: 0 across all five. API docs regenerated for the new public surface (flush, settleWithoutContinuing, cancelledClientToolResult).

Review notes

Each batch was adversarially reviewed, and the reviews caught four defects that the plan and the implementations both missed — worth knowing since three came from following the plan literally:

  • flush() originally left its batch in the buffer across the await, but two other mutators clear that buffer unconditionally — it could destroy an unpersisted result and double-send a persisted one.
  • The plan put the flush inside the per-call blocked branch, so an N-call group fired N flushes and LangGraph's in-flight short-circuit stranded all but the first.
  • Settling on abort via the default channel (cap.resolve) meant Stop submitted a run. Fixed with a settle-only channel that can never fall back to resolve.
  • Re-staging a failed flush after a thread switch would inject the old thread's messages into the new one. Fixed with a buffer generation guard.

Known follow-up (not fixed here)

When a custom transport lacks updateState, persistFn is undefined and flush() resolves with no diagnostic — the coordinator can't distinguish that from a real write. The submit-drain fallback still covers it, so the residual exposure is a reload before the user's next message. Closing it properly needs a signal on the capability contract.

Spec: docs/superpowers/specs/2026-08-07-client-tool-continuation-fixes-design.md
Plan: docs/superpowers/plans/2026-08-07-client-tool-continuation-fixes.md

🤖 Generated with Claude Code

blove and others added 12 commits August 7, 2026 09:40
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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().
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 7, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
threadplane Ready Ready Preview Aug 7, 2026 2:24pm

Request Review

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @blove's task in 0s —— View job


I'll analyze this and get back to you.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated approval: this PR received an intelligent (AI) code review. See the review comments on this PR.

@blove

blove commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Update — three blocking issues from final cross-batch review, fixed in 92384464

1. Lint error (CI-blocking). prefer-const on a forward reference in agent.fn.ts. Replaced the let with a holder object so the binding is genuinely const.

Worth flagging for anyone using the repo's usual pre-merge check: grep -cE ' error ' silently returns 0 on nx output, because ANSI color codes sit between the spaces around error. That is how this reached CI despite a local gate reporting zero errors across five projects. Strip ANSI first:

npx nx lint <lib> --skip-nx-cache 2>&1 | perl -pe 's/\e\[[0-9;]*m//g' | grep -cE '^\s+[0-9]+:[0-9]+\s+error\s'

2. flush() stranded concurrent batches. if (flushInFlight) return flushInFlight handed the second caller a promise that did not cover their batch. Safe via the coordinator (flush is once-per-group), but startClientToolExecutor is public and its default abort path flushes per call — so Stop on a 2-call group persisted only the first result. Now chained rather than short-circuited; the returned promise resolves only after the caller's own batch is written.

3. A tool settling after a thread switch wrote its ToolMessage to the wrong thread. Nothing aborts in-flight handlers on switchThread, and persistFn resolved the thread id at write time. Thread B silently received a ToolMessage matching no AIMessage → permanent 400 on B's next turn.

Note the originally-proposed fix (stamp the thread id at settle time) does not work: setThreadId is synchronous, so by settle time the stamp already reads the new thread. The shipped fix retires the outgoing thread's tool-call ids before the store resets and refuses to buffer a retired id, with thread stamping kept as defense-in-depth. Both are covered by tests using the real agent() + MockAgentTransport; the flush test failed before the fix with the exact wrong-thread write.

Behavior change worth review

Switching away from a thread and back retires that thread's ids, so a handler still in flight across the round trip has its result discarded rather than delivered. Deliberate — the result is stale either way — but it goes beyond the reported bug.

Verification

297 passing in langgraph (+3), 206 ag-ui, plus chat and middleware green. 0 lint errors in all four under the ANSI-safe check. Builds succeed.

Follow-ups deliberately not fixed here

persistFn-undefined resolves with no diagnostic; submit()'s drain is destructive if the run fails; clearStagedToolMessages does not clear resolvedIds; an unanswered ask leaves its group staged until the next submit; the Postgres PK change needs an ALTER TABLE note before the store's first release.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @blove's task in 0s —— View job


I'll analyze this and get back to you.

@blove
blove merged commit d3aa695 into main Aug 7, 2026
67 checks passed
@blove
blove deleted the blove/client-tool-followup-fix branch August 7, 2026 14:58
blove added a commit that referenced this pull request Aug 7, 2026
…act (#808)

Client-tools guide gains app-facing docs for followUp:false terminal
tools, idempotent + the execution guard, AbortSignal on handlers, and
the continuation-turn cap — none of which were documented after #807.

Adapter guide gains the settle/flush/resolve contract, the invariant it
protects, and the three implementation rules each learned from a real
defect (batch ownership at snapshot, chain don't short-circuit, discard
staged results on thread switch).

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant