Skip to content

fix(claude-sdk): keep SDK loop alive when end_turn / react fails so the model can retry - #158

Closed
claudiusthebot wants to merge 1 commit into
mainfrom
fix/preserve-loop-on-send-failure
Closed

fix(claude-sdk): keep SDK loop alive when end_turn / react fails so the model can retry#158
claudiusthebot wants to merge 1 commit into
mainfrom
fix/preserve-loop-on-send-failure

Conversation

@claudiusthebot

Copy link
Copy Markdown
Collaborator

Summary

The PostToolBatch hook used to terminate the SDK query loop the moment it saw a turn-terminator tool call in the batch β€” with zero check on whether the call actually succeeded. When the terminator's delivery failed (e.g. Telegram rejected end_turn for "Message too long", invalid chat_id, network blip), the loop exited as if delivery had worked. The model saw the error in its tool result but had no model turn left to react.

End result: the user saw nothing and the turn was silently dropped.

Canonical incident

2026-05-13 13:11Z (Pandario reply 226264). I tried to deliver an end-of-turn of 4326 chars. Telegram's cap is 4096. Bridge returned {ok: false, error: \"Message too long (4326 chars, max 4096).\"}. Hook fired regardless. Turn ended. User pinged the bot 24 min later asking what happened.

Fix

  • New helper isFailedToolResponse(response: unknown): boolean that parses the bridge response in any of its three observed shapes (raw {ok: false} object, JSON string, MCP content envelope [{type:\"text\", text:\"<JSON>\"}]) and returns true iff failure is affirmatively detected.

  • turnTerminatorHook now consults the terminator's tool_response. If failure is detected, returns { continue: true } and logs that the loop is being preserved β€” model receives the error in the next assistant turn and can retry / message the user about the problem.

  • Conservative default preserved: any unrecognizable / missing / parse-failed response keeps the current terminate-on-success perf path. Same happy-path latency, no regression for tools that don't return ok-shaped JSON.

Test plan

  • 7 new PostToolBatch turn-terminator hook > preserves SDK loop on failed terminator cases:
    • end_turn fail as raw {ok:false} object β†’ loop alive
    • end_turn fail as JSON string β†’ loop alive
    • end_turn fail wrapped in MCP [{type, text}] envelope β†’ loop alive
    • react (strict terminator) fail β†’ loop alive
    • non-terminator send fail β†’ pass-through unchanged (hook never fires for it)
    • end_turn success β†’ still terminates (no regression)
    • end_turn with missing / garbage response β†’ still terminates (conservative default)
  • 12 new unit tests for isFailedToolResponse covering every input shape:
    • Raw object {ok:false} / {ok:true}
    • JSON string \"{...}\" for both
    • MCP envelope for both
    • null / undefined / \"\"
    • Strings without \"ok\" (cheap skip)
    • Unparseable strings containing \"ok\"
    • Objects without ok field
    • Nested arrays of content blocks
  • npm run typecheck clean
  • npm run lint β€” no new warnings
  • npm test β€” 2002/2015 pass, 1 pre-existing flake in package.functional.test.ts unrelated (same flake on main when tests run on a host where Talon daemon is already live β€” status returns "running" instead of "stopped").

πŸ€– Generated with Claude Code

…he model can retry

PostToolBatch hook used to terminate the SDK query loop the moment it saw a
turn-terminator tool call in the batch, with zero check on whether the call
actually succeeded. When the terminator's delivery failed (e.g. Telegram
rejected `end_turn` for "Message too long", invalid chat_id, network blip),
the loop still exited as if delivery succeeded β€” the model saw the error in
its tool result but had no model turn left to react. End result: the user
saw nothing and the turn was silently dropped.

Concrete incident (2026-05-13 13:11Z Pandario reply 226264): an end-of-turn
delivery of a 4326-char message hit Telegram's 4096 cap, the bridge returned
`{ok: false, error: "Message too long..."}`, hook fired regardless, turn
ended. Dylan had to ping the bot to find out anything had happened.

Fix:
  - New helper `isFailedToolResponse(response)` parses the bridge response
    (raw `{ok: false}`, JSON string, or MCP content envelope `[{type, text}]`
    around the JSON) and returns true iff failure is affirmatively detected.
  - PostToolBatch hook now consults the terminator's `tool_response`. If
    failure is detected, returns `{ continue: true }` and logs that the loop
    is being preserved β€” model receives the error and can retry / message
    the user about the problem.
  - Conservative default preserved: any unrecognizable / missing / parse-
    failed response keeps the current terminate-on-success perf path. Same
    happy-path latency, no regression for tools that don't return ok-shaped
    JSON.

Tests:
  - 7 new PostToolBatch hook cases (end_turn fail object / JSON string / MCP
    envelope, react fail, non-terminator-send pass-through, success still
    terminates, missing / garbage response still terminates).
  - 12 new unit tests for `isFailedToolResponse` covering every shape combo.
  - 2015 total tests, 1 pre-existing flake in `package.functional.test.ts`
    unrelated (same flake on main when running tests on a host where Talon
    is already live β€” verified).
@claudiusthebot

Copy link
Copy Markdown
Collaborator Author

Superseded by a cleaner approach β€” see follow-up PR. The content-sniffing hook in this PR is fragile (frontend coupling, schema drift, false positives). Replacing with: make end_turn / react (strict-terminator) tools throw new ToolDeliveryError(...) on bridge {ok:false}, then handle PostToolUseFailure via a per-session failed-id set that PostToolBatch consults. SDK's native error pipeline, no string matching, frontend-agnostic.

dylanneve1 added a commit that referenced this pull request May 19, 2026
… SDK's native error pipeline (#159)

When a turn-terminator tool (`end_turn`, strict `react`) failed to deliver
(e.g. Telegram rejected `end_turn` for "Message too long", invalid chat_id,
network blip), the PostToolBatch hook terminated the SDK loop anyway β€” the
model saw the error in its tool result but had no turn left to react. End
result: silent dropped turn, user sees nothing.

Canonical incident (2026-05-13 13:11Z Pandario reply 226264): end-of-turn
delivery of a 4326-char message hit Telegram's 4096 cap, bridge returned
`{ok: false, error: "Message too long..."}`, hook fired regardless, turn
silently ended.

Supersedes #158 (content-sniffing approach was fragile β€” frontend-coupled,
schema-drift-vulnerable, false-positive-prone on responses that happened
to contain `"ok":false` substrings).

This PR uses the SDK's NATIVE error pipeline instead of inspecting bodies.

Implementation:

1. `end_turn.execute` and `react.execute` THROW when the bridge returns
   `{ok: false}` instead of returning the failure object silently. A new
   `throwIfFailed` helper wraps the bridge result and raises a typed
   `Error("<tool> delivery failed: <bridge error>")`. The "what counts
   as a failure" decision now lives in the tool implementation, where the
   contract is owned.

2. The SDK observes the throw and fires `PostToolUseFailure` with a typed
   `{tool_name, tool_input, tool_use_id, error, is_interrupt}` payload β€”
   no string sniffing, no `unknown` parsing.

3. New `PostToolUseFailure` hook records the failed `tool_use_id` in a
   per-session `Set<string>`. Ignores interrupts (`is_interrupt: true`)
   and non-terminator failures (e.g. `send`).

4. `PostToolBatch` hook now consults the Set β€” if the terminator's
   `tool_use_id` was flagged, it deletes the flag and returns
   `{continue: true}` to keep the SDK loop alive. Otherwise terminates
   as before (perf win from PR #122 preserved on the happy path).

5. The two hooks share state via closure β€” `buildTurnTerminatorHooks()`
   creates a fresh Set per `buildSdkOptions()` call, so concurrent chat
   sessions stay isolated.

Frontend-agnostic by design: any frontend whose tools throw on delivery
failure gets the same recovery behaviour. No bridge envelope shape is
baked into the SDK options layer.

Tests:
  - 9 new `PostToolUseFailure + PostToolBatch coordination` cases
    (terminator failure preserves loop, success terminates, interrupt
    ignored, non-terminator failure ignored, soft-react `end_turn:false`
    ignored, defensive non-failure events, flag-consumed-on-match,
    per-session isolation).
  - 8 new messaging-tools cases for `end_turn` / `react` throw behaviour
    (text path throws on {ok:false}, buttons path throws, generic
    message when error field missing, success path unchanged, react
    strict + soft both throw, react strips end_turn param).
  - All 33 existing PostToolBatch hook tests still pass.
  - 2001/2014 vitest pass β€” same pre-existing `package.functional` flake
    as PR #157 (irrelevant: running tests on a host where Talon daemon is
    already live).
  - typecheck clean, prettier clean, no new lint warnings.

Co-authored-by: Dylan Neve <dylan.neve@intel.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