Skip to content

fix(tool): honor timeouts in ScriptedTool - #1944

Merged
chaliy merged 1 commit into
mainfrom
2026-06-05-propose-fix-for-scriptedtool-timeout_ms-issue
Jun 5, 2026
Merged

chaliy merged 1 commit into
mainfrom
2026-06-05-propose-fix-for-scriptedtool-timeout_ms-issue

Conversation

@chaliy

@chaliy chaliy commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • ToolRequest::timeout_ms is part of the public tool contract and must abort per-call orchestrations; ScriptedTool previously ignored it, allowing long-running callbacks/commands to bypass orchestrator limits.

Description

  • Enforce timeout_ms in ScriptedTool::run_request_with_stream by wrapping the execution future in tokio::time::timeout so the whole orchestration (including streaming and callbacks) is aborted on expiry.
  • Reuse the shared timeout helper by making timeout_response available crate-wide so timeouts return exit code 124 and error = "timeout" consistently with BashTool.
  • Preserve streaming behavior and status callbacks while applying the global timeout.
  • Add regression tests in crates/bashkit/src/scripted_tool/execute.rs that verify execute and execute_with_status honor timeout_ms using a slow async callback.

Testing

  • Ran cargo fmt --check which succeeded.
  • Ran targeted tests with the scripted_tool feature: cargo test -p bashkit --features scripted_tool test_scripted_tool_execute -- --nocapture, and the added timeout tests passed (2 passed, 0 failed).
  • Performed git diff --check (no whitespace/errors reported).

Codex Task

Copilot AI review requested due to automatic review settings June 5, 2026 19:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a contract gap in ScriptedTool: it now honors ToolRequest::timeout_ms by enforcing a per-call timeout that aborts the entire orchestration (including tool callbacks and streaming). It also centralizes timeout response formatting so ScriptedTool and BashTool return consistent timeout results (exit code 124, error = "timeout"), and adds regression tests to prevent regressions.

Changes:

  • Enforce timeout_ms in ScriptedTool::run_request_with_stream via tokio::time::timeout.
  • Make the shared timeout_response helper available crate-wide for consistent timeout responses.
  • Add async regression tests proving execute / execute_with_status respect timeout_ms.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/bashkit/src/tool.rs Exposes timeout_response as pub(crate) so other crate modules can reuse the standardized timeout ToolResponse.
crates/bashkit/src/scripted_tool/execute.rs Wraps ScriptedTool execution in a per-call timeout and adds tests verifying timeouts abort slow async callbacks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 5, 2026 •

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit de06e7d Commit Preview URL Jun 05 2026, 08:01 PM

@chaliy
chaliy merged commit cd89e36 into main Jun 5, 2026
36 checks passed
@chaliy
chaliy deleted the 2026-06-05-propose-fix-for-scriptedtool-timeout_ms-issue branch June 5, 2026 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants