fix(sdk): honor canUseTool timeout + copy CLI chunks into SDK bundle#326
Merged
Conversation
Ported from QwenLM#4491. The SDK already honored `options.timeout.canUseTool` for its own local permission callback, but never forwarded it to the CLI: the CLI's permission control request used a hardcoded default, so a long-running `canUseTool` handler could be timed out by the CLI before the SDK's own timeout elapsed. - SDK Query now sends `timeout.canUseTool` in the `initialize` control request. - The CLI's SystemController reads it on initialize, validates it (finite, > 0, <= 10min cap to avoid setTimeout 32-bit overflow), and stores it on the control context. - PermissionController uses the context value (falling back to a 60s default) instead of the unconditional default when issuing the can_use_tool request. - types/protocol gain the `timeout` field on CLIControlInitializeRequest. Tests: SDK Query.test asserts the timeout is sent on initialize (and omitted when unset). 55 Query tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Ported from QwenLM#4541. Both SDK CLI-bundling scripts now copy an adjacent `chunks/` directory (via a shared `copyOptionalDir` helper that also covers vendor/ and locales/). If the CLI bundle is ever code-split, its chunk files must ship in the SDK package or the bundled CLI fails at runtime on missing imports. Our bundle is currently single-file, so this is a guarded no-op today — included for parity and to remove the latent footgun. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
WalkthroughThis PR adds optional timeout configuration for permission control requests. The SDK can now specify a ChangesPermission Control Timeout Configuration
Build Script Artifact Consolidation
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/cli/src/nonInteractive/control/ControlContext.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/cli/src/nonInteractive/control/controllers/permissionController.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. packages/cli/src/nonInteractive/control/controllers/systemController.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
There was a problem hiding this comment.
QA Audit — PR #326 | fix(sdk): honor canUseTool timeout + copy CLI chunks into SDK bundle
VERDICT: PASS
CI Status
- Lint: in_progress
- CodeQL: in_progress
(Terminal verdict will be issued once checks complete.)
Diff Review
Two clean upstream ports touching the SDK CLI-bundling scripts and the CLI control plane:
-
canUseTooltimeout propagation — SDKQuerysendstimeout.canUseToolininitialize.SystemControllervalidates it (finite, > 0, ≤ 600 s cap) and stores onControlContext.PermissionControllerreads the context value with a 60 s fallback. The wire contract is declared inCLIControlInitializeRequestin both CLI types and SDK protocol types. -
chunks/bundle copy — Both bundling scripts (bundle-cli.js,bundle-cli-from-npm.js) gain acopyOptionalDirhelper and now copychunks/alongsidevendor//locales/. Currently a no-op (single-file bundle, no code-splitting), but removes the latent footgun if splitting is ever enabled.
Observations
- CLAWPATCH: unavailable for this repo; review is diff-based.
- CRITICAL: None.
- HIGH: None.
- MEDIUM: The deliberate skip of upstream's
permissionController.test.ts/systemController.test.tsis noted and acceptable — no controller-test harness exists on our side, and the wire propagation is covered by the 55 SDKQuery.testassertions. CLI controller unit tests are tracked as a follow-up. - LOW: None.
— Quinn, QA Engineer — holding for CI completion
|
Submitted |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/nonInteractive/control/controllers/systemController.ts`:
- Around line 95-103: The initialize logic only assigns
this.context.sdkCanUseToolTimeoutMs when payload.timeout?.canUseTool is a valid
number <= MAX_CAN_USE_TOOL_TIMEOUT_MS, which leaves stale values or silently
drops oversized values; change the block around canUseToolTimeout so that on
re-initialize you always reset this.context.sdkCanUseToolTimeoutMs (clear it
when omitted/invalid or non-positive), and when canUseToolTimeout is a finite
number clamp it to MAX_CAN_USE_TOOL_TIMEOUT_MS if it exceeds the max before
assigning; reference the symbols canUseToolTimeout, MAX_CAN_USE_TOOL_TIMEOUT_MS
and this.context.sdkCanUseToolTimeoutMs when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93f92dc2-c633-4ce0-9c39-f7ec23a1f15a
📒 Files selected for processing (9)
packages/cli/src/nonInteractive/control/ControlContext.tspackages/cli/src/nonInteractive/control/controllers/permissionController.tspackages/cli/src/nonInteractive/control/controllers/systemController.tspackages/cli/src/nonInteractive/types.tspackages/sdk-typescript/scripts/bundle-cli-from-npm.jspackages/sdk-typescript/scripts/bundle-cli.jspackages/sdk-typescript/src/query/Query.tspackages/sdk-typescript/src/types/protocol.tspackages/sdk-typescript/test/unit/Query.test.ts
Comment on lines
+95
to
+103
| const canUseToolTimeout = payload.timeout?.canUseTool; | ||
| if ( | ||
| typeof canUseToolTimeout === 'number' && | ||
| Number.isFinite(canUseToolTimeout) && | ||
| canUseToolTimeout > 0 && | ||
| canUseToolTimeout <= MAX_CAN_USE_TOOL_TIMEOUT_MS | ||
| ) { | ||
| this.context.sdkCanUseToolTimeoutMs = canUseToolTimeout; | ||
| } |
There was a problem hiding this comment.
Reset and clamp canUseTool timeout during initialize.
Line 95-Line 103 only updates context on “already-valid and <= max” input. On re-initialize, an omitted/invalid value can leave a stale prior timeout; values above max are dropped instead of capped.
Suggested fix
- const canUseToolTimeout = payload.timeout?.canUseTool;
- if (
- typeof canUseToolTimeout === 'number' &&
- Number.isFinite(canUseToolTimeout) &&
- canUseToolTimeout > 0 &&
- canUseToolTimeout <= MAX_CAN_USE_TOOL_TIMEOUT_MS
- ) {
- this.context.sdkCanUseToolTimeoutMs = canUseToolTimeout;
- }
+ const canUseToolTimeout = payload.timeout?.canUseTool;
+ // Re-initialize should not retain stale timeout values
+ this.context.sdkCanUseToolTimeoutMs = undefined;
+ if (
+ typeof canUseToolTimeout === 'number' &&
+ Number.isFinite(canUseToolTimeout) &&
+ canUseToolTimeout > 0
+ ) {
+ this.context.sdkCanUseToolTimeoutMs = Math.min(
+ canUseToolTimeout,
+ MAX_CAN_USE_TOOL_TIMEOUT_MS,
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/nonInteractive/control/controllers/systemController.ts`
around lines 95 - 103, The initialize logic only assigns
this.context.sdkCanUseToolTimeoutMs when payload.timeout?.canUseTool is a valid
number <= MAX_CAN_USE_TOOL_TIMEOUT_MS, which leaves stale values or silently
drops oversized values; change the block around canUseToolTimeout so that on
re-initialize you always reset this.context.sdkCanUseToolTimeoutMs (clear it
when omitted/invalid or non-positive), and when canUseToolTimeout is a finite
number clamp it to MAX_CAN_USE_TOOL_TIMEOUT_MS if it exceeds the max before
assigning; reference the symbols canUseToolTimeout, MAX_CAN_USE_TOOL_TIMEOUT_MS
and this.context.sdkCanUseToolTimeoutMs when making the change.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bounded SDK fixes ported from upstream, both relevant to the SDK control plane / packaging that the fleet's
ProtoSdkExecutordepends on.1. Honor
canUseTooltimeout in CLI control requests (ports QwenLM#4491)The SDK already honored
options.timeout.canUseToolfor its own local permission callback, but never forwarded it to the CLI. The CLI's permission (can_use_tool) control request used a hardcoded default, so a long-runningcanUseToolhandler could be timed out by the CLI before the SDK's own configured timeout elapsed.Querynow sendstimeout.canUseToolin theinitializecontrol request.SystemControllerreads it on initialize, validates it (finite, > 0, ≤ 10 min cap to avoidsetTimeout32-bit overflow), and stores it on the control context.PermissionControlleruses the context value (falling back to a 60s default) instead of the unconditional default.timeoutfield added toCLIControlInitializeRequestin both the CLI and SDK protocol types.Tests: SDK
Query.testasserts the timeout is sent on initialize and omitted when unset — 55 Query tests pass.2. Copy CLI
chunks/into the SDK package bundle (ports QwenLM#4541)Both SDK CLI-bundling scripts now copy an adjacent
chunks/directory (via a sharedcopyOptionalDirhelper that also coversvendor/andlocales/). If the CLI bundle is ever code-split, its chunk files must ship in the SDK package or the bundled CLI fails at runtime on missing imports.Our bundle is currently single-file (
esbuildwith nosplitting), so this is a guarded no-op today — included for upstream parity and to remove the latent footgun if splitting is ever enabled. It rides along in this PR at no extra release cost.Validation
tsc (CLI + SDK) clean, eslint clean,
node --checkon both scripts, 55 SDK Query tests pass.Deliberately skipped
I did not port upstream's new
permissionController.test.ts/systemController.test.ts(186 + 207 lines) — they assume upstream's controller mock shapes and we have no existing controller-test harness. The wire propagation (the part most likely to regress) is covered by the SDKQuery.test. CLI-side controller unit tests could be a small follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores