-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(nodejs): plumb AbortSignal through ToolInvocation (#1433) #1701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -475,6 +475,15 @@ export interface ToolInvocation { | |
| toolCallId: string; | ||
| toolName: string; | ||
| arguments: unknown; | ||
| /** | ||
| * An `AbortSignal` that aborts when `session.abort()` or | ||
| * `session.cancelToolCall(toolCallId)` is invoked while this handler is | ||
| * in flight. Handlers may opt in to cooperative cancellation by forwarding | ||
| * it to abortable APIs (`fetch(url, { signal })`, `child_process.spawn`, | ||
| * etc.) or by checking `signal.aborted`. Handlers that ignore it continue | ||
| * to run to completion, preserving existing behavior. | ||
| */ | ||
| signal: AbortSignal; | ||
|
Comment on lines
475
to
+486
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping |
||
| /** W3C Trace Context traceparent from the CLI's execute_tool span. */ | ||
| traceparent?: string; | ||
| /** W3C Trace Context tracestate from the CLI's execute_tool span. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| models: | ||
| - claude-sonnet-4.5 | ||
| conversations: | ||
| - messages: | ||
| - role: system | ||
| content: ${system} | ||
| - role: user | ||
| content: Use slow_analysis with value 'test_cancel'. Wait for the result. | ||
| - role: assistant | ||
| content: I'll call the slow_analysis tool with the value 'test_cancel' and wait for it to complete. | ||
| - role: assistant | ||
| tool_calls: | ||
| - id: toolcall_0 | ||
| type: function | ||
| function: | ||
| name: report_intent | ||
| arguments: '{"intent":"Running slow analysis test"}' | ||
| - role: assistant | ||
| tool_calls: | ||
| - id: toolcall_1 | ||
| type: function | ||
| function: | ||
| name: slow_analysis | ||
| arguments: '{"value":"test_cancel"}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be sending
session.abortbefore calling_abortInFlightToolCalls.If tools get aborted first, their
AbortSignalwill fire, and their promises will be rejected. This sends back a tool call result to the runtime. If the runtime receives this before the session abort message, it will treat it as an error and might even start a new agent invocation to process the failed tool call.I know that as the code is structured right now, that doesn't actually happen because there are no awaits in between lines 1228 and 1229. But that means it's working just by chance, and could stop working if anything about this code is refactored in the future (e.g., awaiting something between those two lines).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: this would cease to be an concern after updating the flow to match #1701 (comment)