Skip to content

fix(tool-executor): thread the executed tool call into the hook context - #91

Merged
LukasParke merged 4 commits into
mainfrom
fix/thread-tool-call-into-execute-context
Aug 3, 2026
Merged

fix(tool-executor): thread the executed tool call into the hook context#91
LukasParke merged 4 commits into
mainfrom
fix/thread-tool-call-into-execute-context

Conversation

@w0nche0l

Copy link
Copy Markdown
Contributor

Problem

TurnContext.toolCall is part of the tool-facing contract ("The specific tool call being executed"), but only the non-streaming tool-orchestrator path ever populated it. The streaming ModelResult loop builds its per-round turn context as { numberOfTurns } only, and buildExecuteCtx received the toolCall as a parameter at every call site without merging it into the execute context — so execute / onToolCalled hooks saw context.toolCall === undefined on the code path most consumers actually run.

Found deterministically by an OpenRouter router e2e (DEV-763): a HITL tool keys suspensions on context.toolCall.callId and failed on every streaming run with its contract-violation guard.

Fix

buildExecuteCtx now threads the executed call:

  • a caller-provided turnContext.toolCall wins (the orchestrator's is richer — it carries status);
  • otherwise the executed ParsedToolCall is converted back to the wire-shaped FunctionCallItem (id/callId = the wire call id, parsed arguments re-serialized) and fills the gap;
  • the onResponseReceived path threads nothing — only the function_call_output item is in scope there, and fabricating call arguments would lie to the hook. A caller-provided turn-context toolCall still flows through.

Tests

Two regressions in hitl-tool.test.ts: a streaming-shaped turn context (numberOfTurns only) delivers the synthesized FunctionCallItem to onToolCalled, and a caller-provided turn-context toolCall is preferred untouched. Full unit suite: 645 tests green, typecheck + biome clean.

Downstream

openrouter-web pins @openrouter/agent@0.7.2 with an existing patch file; once this lands and releases, the monorepo can pick it up with a version bump (or extend its patch in the interim).

Made with Cursor

TurnContext declares toolCall as part of the tool-facing contract, but
only the non-streaming orchestrator populated it — the streaming
ModelResult loop builds its turn context with numberOfTurns only, so
execute/onToolCalled hooks saw context.toolCall === undefined on the
path most consumers actually run. Every executor path has the call in
hand, so buildExecuteCtx now threads it: a caller-provided turn-context
toolCall wins (the orchestrator's carries status), and the executed
call fills the gap otherwise. The onResponseReceived path only has the
function_call_output item in scope, so it threads nothing rather than
fabricating arguments.

Found by an OpenRouter router e2e: a HITL tool keyed suspensions on
context.toolCall.callId and failed deterministically under streaming.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread package.json Outdated
Comment on lines +28 to +31
},
"workspaces": [
"packages/*"
]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is this necessary? i don't think it is, we should remove it.

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.

What do you mean? There is another package? This is a monorepo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, exactly, i think an agent added this while it was trying to debug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed these changes!

@w0nche0l
w0nche0l requested a review from LukasParke July 31, 2026 20:41
w0nche0l and others added 3 commits August 3, 2026 14:49
pnpm reads workspace config from pnpm-workspace.yaml (which already
declares packages/*); the package.json workspaces field was a leftover
from a stray bun install and is ignored by pnpm. Addresses review
feedback.

Co-authored-by: Cursor <cursoragent@cursor.com>
Artifact of the same stray bun install as the workspaces field removed
in the previous commit — this is a pnpm repo (pnpm-lock.yaml is the
lockfile); nothing in the repo or CI uses bun.

Co-authored-by: Cursor <cursoragent@cursor.com>
- Gitignore bun.lock so local bun runs don't reintroduce the stray
  lockfile (upstream already dropped it and the workspaces field).
- Add a patch changeset for @openrouter/agent so the fix releases.
- toFunctionCallItem: always JSON.stringify arguments — every executor
  entry point parses the wire string first, so a string value is a parsed
  string-schema input, and passing it through unquoted would put invalid
  JSON on the synthesized item. Document the byte-fidelity caveat.
- Tests: cover the regular-execute path through the dispatcher, and pin
  that onResponseReceived deliberately gets no synthesized toolCall.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasParke
LukasParke merged commit 231fb65 into main Aug 3, 2026
5 checks passed
@LukasParke
LukasParke deleted the fix/thread-tool-call-into-execute-context branch August 3, 2026 19:15
LukasParke added a commit that referenced this pull request Aug 3, 2026
#74 (loopKey) and #91 (toolCall in hook context) landed on main. Three
conflicts, all resolved as unions:

- tool-executor.ts: both sides changed buildExecuteCtx's call sites — this
  branch added `extras`, main added `toolCall`. The signature had already
  auto-merged to accept both; the three call sites now pass both.
- doom-loop.ts / doom-loop.test.ts: main's loopKey field-array guard (the
  empty-list and all-fields-absent fallbacks) arrived wholesale; this
  branch had nothing there.

Two fixes the merge made necessary:

- tool-types.ts: this branch carried a pre-#74 `ToolLoopKey` without the
  `readonly string[]` member, so `tool-wrapper.ts` failed to typecheck
  against the field-array form main now produces. Restored main's union.
- model-result.ts crossed the fan-out ceiling again (16, limit 15) once the
  merge landed. `normalizeInputToArray` now comes from
  `conversation-state.js`, which already imported it and is the natural
  owner of conversation-input normalization. God files back to 0.

Also carries a review fix (Devin, tool-context.ts): NEVER_ABORT_SIGNAL was
one process-wide signal shared by every context built without cancellation
sources. A tool body subscribing to it leaked a listener for the process
lifetime, since the signal never fires and nothing removes them. Replaced
with a per-context factory; two tests pin it, mutation-verified.

Verified: 62 agent + 10 mcp test files pass, typecheck and lint clean,
gate reports God files 0 -> 0 and complex functions at baseline.
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.

2 participants