feat(core): add configurable default timeout for foreground shell commands - #6628
Conversation
|
Thanks for the PR! Re-running triage on the latest commit. Template looks good ✓ — all required sections present with real content. Problem: Real and observed. Issue #5838 includes a screenshot showing a foreground command timing out at the hardcoded 2-minute mark with no way to extend it. The PR fixes exactly that. Direction: Aligned. Configurable timeouts are a standard quality-of-life feature for CLI agents. Users who run long builds, test suites, or data pipelines hit the 2-minute wall regularly. Claude Code has the same concept. This is clearly within scope. Size: 72 production logic lines (config plumbing + shell timeout resolution) vs. 180 test lines + 7 doc/schema lines. Not a large PR. Core paths are touched ( Approach: Minimal and focused. Three-line precedence chain ( Moving on to code review. 🔍 中文说明感谢贡献！在当前 commit 上重新执行 triage。 模板完整 ✓ — 所有必要章节均有实际内容。 问题： 真实且已观测。Issue #5838 包含截图，显示前台命令在硬编码的 2 分钟处超时且无法延长。PR 精确修复了此问题。 方向： 对齐。可配置超时是 CLI agent 的标准体验改善功能。运行长构建、测试套件或数据管道的用户经常碰到 2 分钟上限。Claude Code 也有类似概念。明确在范围内。 规模： 72 行生产逻辑（config 管道 + shell 超时解析）vs. 180 行测试 + 7 行文档/schema。不是大 PR。触及核心路径但改动小且范围清晰。 方案： 最小且聚焦。三行优先级链、Config 构造函数中的完善输入校验、以及 timeout=0 时长运行提示的边缘修复。无范围蔓延、无顺手重构。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
| showInDialog: false, | ||
| }, | ||
| defaultTimeoutMs: { | ||
| type: 'number', |
There was a problem hiding this comment.
[Critical] Missing minimum: 0 on the schema — negative values crash shell commands
The defaultTimeoutMs entry has no minimum constraint. A user setting defaultTimeoutMs: -1 passes validation, flows to AbortSignal.timeout(-1), and throws an uncaught RangeError, breaking every foreground shell command. Every other numeric timeout setting in this file (idleTimeoutMs, visionBridgeTimeoutMs, toolIdleTimeoutMs) has minimum/maximum bounds.
Consider also adding maximum: 600000 to match the per-call ceiling (currently the settings path bypasses it).
| type: 'number', | |
| defaultTimeoutMs: { | |
| type: 'number', | |
| minimum: 0, | |
| maximum: 600000, |
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/core/src/tools/shell.ts:2078 |
No debug logging at the timeout precedence resolution point. The three-tier fallback (per-call → configured → built-in) is entirely silent, making it hard to diagnose unexpected timeout behavior. | Add debugLogger.debug('resolved foreground shell timeout', { perCallTimeout: this.params.timeout ?? null, configuredDefault: this.config.getShellDefaultTimeoutMs() ?? null, effectiveTimeout }) after the resolution. |
packages/cli/src/config/config.ts:2167 |
No test verifies the settings.tools.shell.defaultTimeoutMs → Config.shellDefaultTimeoutMs wiring in loadCliConfig. If the mapping is accidentally dropped, the feature silently stops working. |
Add a loadCliConfig test case that passes { tools: { shell: { defaultTimeoutMs: 300000 } } } and asserts config.getShellDefaultTimeoutMs() === 300000. (Note: this is a pre-existing gap — sibling settings also lack wiring tests.) |
packages/cli/src/config/settingsSchema.ts:2238 |
Schema declares type: 'number' but the sibling visionBridgeTimeoutMs uses type: 'integer'. Fractional ms values (e.g., 1500.5) pass validation for defaultTimeoutMs but would be rejected for visionBridgeTimeoutMs. |
Change to type: 'integer' for consistency with visionBridgeTimeoutMs. |
— qwen3.7-max via Qwen Code /review
…6628) Addresses reviewer findings on tools.shell.defaultTimeoutMs: - settingsSchema.ts: add minimum/maximum bounds (matching the sibling tools.computerUse.idleTimeoutMs entry) so a negative value like -1 can no longer pass settings validation and reach AbortSignal.timeout(-1), which throws a RangeError on Node. Regenerated the vscode-ide-companion JSON schema via `npm run generate:settings-schema`. - shell.ts: only compute/append the long-run "consider backgrounding" hint when the effective timeout is > 0. Previously longRunThresholdFor(0) floored to 1000ms, so a configured default of 0 (timeout disabled) still fired the hint on any command over 1s - the opposite of the setting's intent. - shell.ts: corrected the precedence comment; a per-call timeout of 0 or less is rejected by validateToolParamValues, so only a configured default of 0 disables the timeout, not "a value of 0 at any level". - shell.ts: the tool description's timeout note is now built from config.getShellDefaultTimeoutMs() at ShellTool construction time instead of hard-coding "120000ms (2 minutes)", so it no longer goes stale once a custom default is configured. Extended shell.test.ts and settingsSchema.test.ts accordingly.
| ShellTool.Name, | ||
| ToolDisplayNames.SHELL, | ||
| getShellToolDescription(), | ||
| getShellToolDescription(config), |
There was a problem hiding this comment.
[Critical] getShellToolDescription(config) now calls config.getShellDefaultTimeoutMs(), but packages/core/src/tools/toAutoClassifierInput.test.ts constructs new ShellTool(minimalConfig(...)) where minimalConfig only provides getTargetDir and getModelInvocableCommandsExecutor — missing getShellDefaultTimeoutMs. This crashes the test with TypeError: config.getShellDefaultTimeoutMs is not a function. The CI failure in Test (ubuntu-latest, Node 22.x) is likely caused by this.
Fix: add getShellDefaultTimeoutMs: () => undefined to the minimalConfig helper in toAutoClassifierInput.test.ts:
function minimalConfig(over: Partial<Record<string, unknown>> = {}): Config {
return {
getTargetDir: () => '/Users/test/project',
getModelInvocableCommandsExecutor: () => undefined,
getShellDefaultTimeoutMs: () => undefined,
...over,
} as unknown as Config;
}— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] Test regression: coreToolScheduler.test.ts and toAutoClassifierInput.test.ts crash
The PR changed getShellToolDescription() to getShellToolDescription(config), which calls config.getShellDefaultTimeoutMs() during ShellTool construction. Two test files construct ShellTool (or subclass) with incomplete mock Configs that lack getShellDefaultTimeoutMs, causing TypeError: config.getShellDefaultTimeoutMs is not a function at construction time:
packages/core/src/core/coreToolScheduler.test.ts:13249—new TestShellTool({} as Config)(empty mock)packages/core/src/tools/toAutoClassifierInput.test.ts:114—new ShellTool(minimalConfig(...))(minimal mock)
Both need getShellDefaultTimeoutMs: () => undefined added to their mock Config objects.
| - The command argument is required. | ||
| - You can specify an optional timeout in milliseconds (up to 600000ms / 10 minutes). If not specified, commands will timeout after 120000ms (2 minutes). | ||
| - You can specify an optional timeout in milliseconds (up to 600000ms / 10 minutes). ${timeoutUsageNote} | ||
| - It is very helpful if you write a clear, concise description of what this command does in 5-10 words. |
There was a problem hiding this comment.
[Suggestion] When defaultTimeoutMs is configured above 600000, this line produces contradictory text: "up to 600000ms / 10 minutes" sits next to "commands will timeout after 900000ms (the configured default)". The model may try per-call values above 600000 (rejected by validateToolParamValues) or make wrong backgrounding decisions.
Consider capping the schema maximum at 600000 to align with the per-call ceiling, or making the "up to" text dynamic when the configured default exceeds 600000.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Test regression: toAutoClassifierInput.test.ts and coreToolScheduler.test.ts crash at construction time
ShellTool constructor now calls getShellToolDescription(config) which dereferences config.getShellDefaultTimeoutMs(). Two test files construct ShellTool with incomplete mock configs that lack this method:
packages/core/src/tools/toAutoClassifierInput.test.ts:114—new ShellTool(minimalConfig({ getTargetDir: () => '/cwd' }))whereminimalConfigonly providesgetTargetDirandgetModelInvocableCommandsExecutorpackages/core/src/core/coreToolScheduler.test.ts:13247—new TestShellTool({} as Config)where the empty object has nogetShellDefaultTimeoutMs
Both fail with TypeError: config.getShellDefaultTimeoutMs is not a function. This is causing CI's Test (ubuntu-latest, Node 22.x) check to fail.
Fix: add getShellDefaultTimeoutMs: () => undefined to the mock config objects in both test files.
— qwen3.7-max via Qwen Code /review
| // the built-in default. A configured default of 0 disables the | ||
| // timeout. A per-call timeout must be positive — `timeout <= 0` is | ||
| // rejected by `validateToolParamValues` before we ever get here. | ||
| const effectiveTimeout = |
There was a problem hiding this comment.
[Critical] Negative defaultTimeoutMs from a hand-edited settings.json bypasses schema validation and crashes every foreground shell command
The schema declares minimum: 0 / maximum: 2147483647, but validateSettingValue only runs on the write path (settings dialog, /config command, workspace-settings API). loadSettings() parses settings.json from disk without running schema bound checks. A hand-edited file with "defaultTimeoutMs": -1 flows through loadCliConfig → Config.shellDefaultTimeoutMs (stored raw, no guard in constructor) → getShellDefaultTimeoutMs() → this resolution point.
Since -1 is truthy, effectiveTimeout becomes -1, the if (effectiveTimeout) guard passes, and AbortSignal.timeout(-1) throws a synchronous RangeError. Every foreground shell command in the session fails with an opaque error ("The value of 'delay' is out of range") that gives no indication the cause is a misconfigured setting.
The PR's own regression test (shell.test.ts ~line 2789) pins this exact crash path but asserts the guard lives "upstream in the settings schema" — that guard is not active on the file-load path.
| const effectiveTimeout = | |
| const configuredDefault = this.config.getShellDefaultTimeoutMs(); | |
| const safeDefault = | |
| configuredDefault !== undefined && | |
| Number.isFinite(configuredDefault) && | |
| configuredDefault >= 0 | |
| ? configuredDefault | |
| : undefined; | |
| const effectiveTimeout = | |
| this.params.timeout ?? safeDefault ?? DEFAULT_FOREGROUND_TIMEOUT_MS; |
— qwen3.7-max via Qwen Code /review
| defaultTimeoutMs: { | ||
| type: 'number', | ||
| label: 'Default Command Timeout (ms)', | ||
| category: 'Tools', |
There was a problem hiding this comment.
[Critical] requiresRestart: false is incorrect — Config.shellDefaultTimeoutMs is private readonly with no setter, so mid-session changes have no effect
Config.shellDefaultTimeoutMs (packages/core/src/config/config.ts:1745) is declared private readonly and assigned only in the constructor. Settings changes at runtime do not rebuild Config. The sibling visionBridgeTimeoutMs at line 1284 has identical read-once semantics and correctly uses requiresRestart: true with an explanatory comment: "Read once in the Config constructor with no setter, so a mid-session change only takes effect on restart."
Users who change this setting via the settings dialog will see no effect until they restart, but the UI won't prompt for a restart because requiresRestart: false.
| category: 'Tools', | |
| requiresRestart: true, |
— qwen3.7-max via Qwen Code /review
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/29159541631)._ |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| description: 'Show color in shell output.', | ||
| showInDialog: false, | ||
| }, | ||
| defaultTimeoutMs: { |
There was a problem hiding this comment.
[Suggestion] Use type: 'integer' instead of type: 'number' to match the existing visionBridgeTimeoutMs schema (line 1284) and the per-call validator at shell.ts:4943 which enforces Number.isInteger(params.timeout). A fractional value like 0.5 currently passes schema validation and the runtime sanitizer, then reaches AbortSignal.timeout(0.5) — Node coerces to ~0ms, timing out every command immediately. Also add !Number.isInteger(configuredDefaultTimeoutMs) to sanitizeConfiguredDefaultTimeoutMs for defense-in-depth.
| defaultTimeoutMs: { | |
| type: 'integer', |
— qwen3.7-max via Qwen Code /review
| configuredDefaultTimeoutMs < 0 || | ||
| configuredDefaultTimeoutMs > MAX_FOREGROUND_TIMEOUT_MS | ||
| ) { | ||
| return undefined; |
There was a problem hiding this comment.
[Suggestion] When the sanitizer discards an out-of-range value (negative, non-finite, or above 600000), it does so silently with no log or diagnostic. A user who hand-edits settings.json to an invalid value (e.g., -1 or 900000) has no way to discover why their configured default is being ignored. Consider adding a debugLogger.warn(...) before the return undefined, mirroring the logging pattern already used elsewhere in this file (e.g., lines 258, 766, 815):
if (
configuredDefaultTimeoutMs === undefined ||
!Number.isFinite(configuredDefaultTimeoutMs) ||
configuredDefaultTimeoutMs < 0 ||
configuredDefaultTimeoutMs > MAX_FOREGROUND_TIMEOUT_MS
) {
if (configuredDefaultTimeoutMs !== undefined) {
debugLogger.warn(
`tools.shell.defaultTimeoutMs=${configuredDefaultTimeoutMs} is out of range [0, ${MAX_FOREGROUND_TIMEOUT_MS}]; falling back to built-in default`,
);
}
return undefined;
}— qwen3.7-max via Qwen Code /review
db5dfdf to
844d40e
Compare
Address review on QwenLM#6628: - Add getShellDefaultTimeoutMs to mock configs in coreToolScheduler.test.ts and toAutoClassifierInput.test.ts (ShellTool construction now reads it). - Add minimum: 0 / maximum: 600000 to the defaultTimeoutMs setting so a negative value can't reach AbortSignal.timeout(); regenerate schema.
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
| params.truncateToolOutputLines ?? DEFAULT_TRUNCATE_TOOL_OUTPUT_LINES; | ||
| this.toolOutputBatchBudget = | ||
| params.toolOutputBatchBudget ?? DEFAULT_TOOL_OUTPUT_BATCH_BUDGET; | ||
| this.shellDefaultTimeoutMs = params.shellDefaultTimeoutMs; |
There was a problem hiding this comment.
[Critical] Missing runtime validation of shellDefaultTimeoutMs. This value flows from a hand-edited settings.json through to AbortSignal.timeout(effectiveTimeout) at shell.ts:2100 with no type or range check. The sibling visionBridgeTimeoutMs (lines 2134–2140) applies defensive validation (integer check, bounds, fallback to undefined), but this field is assigned verbatim.
A negative value like -1 is truthy and passes the if (effectiveTimeout) guard at shell.ts:2100, reaching AbortSignal.timeout(-1) which throws RangeError — crashing every foreground shell command. Non-numeric values (strings, NaN, Infinity) similarly crash. The schema's minimum/maximum constraints are enforced at write time by validateSettingValue(), but not re-validated at load time, so a hand-edited settings.json bypasses them entirely.
| this.shellDefaultTimeoutMs = params.shellDefaultTimeoutMs; | |
| this.shellDefaultTimeoutMs = | |
| params.shellDefaultTimeoutMs !== undefined && | |
| Number.isInteger(params.shellDefaultTimeoutMs) && | |
| params.shellDefaultTimeoutMs >= 0 && | |
| params.shellDefaultTimeoutMs <= 600_000 | |
| ? params.shellDefaultTimeoutMs | |
| : undefined; |
— qwen3.7-max via Qwen Code /review
| const effectiveTimeout = | ||
| this.params.timeout ?? DEFAULT_FOREGROUND_TIMEOUT_MS; | ||
| this.params.timeout ?? | ||
| this.config.getShellDefaultTimeoutMs() ?? |
There was a problem hiding this comment.
[Suggestion] When effectiveTimeout is 0 (timeout disabled via defaultTimeoutMs: 0), longRunThresholdFor(0) returns Math.max(1000, Math.floor(0/2)) = 1000ms. Every foreground command running longer than 1 second receives a "consider backgrounding this command" hint, even though the user explicitly disabled timeouts. This creates noisy, misleading output.
Fix: suppress the long-run hint when timeout is disabled:
const longRunThreshold = effectiveTimeout
? longRunThresholdFor(effectiveTimeout)
: Infinity;— qwen3.7-max via Qwen Code /review
|
|
||
| // Precedence: an explicit per-call `timeout` wins; otherwise fall back | ||
| // to the configured `tools.shell.defaultTimeoutMs` setting; otherwise | ||
| // the built-in default. A value of 0 at any level disables the timeout. |
There was a problem hiding this comment.
[Suggestion] This comment claims "A value of 0 at any level disables the timeout," but the per-call timeout parameter validation at shell.ts:4897 rejects timeout <= 0 with 'Timeout must be a positive number.' Zero is only valid at the settings/default level, not at the per-call level.
| // the built-in default. A value of 0 at any level disables the timeout. | |
| // Precedence: an explicit per-call `timeout` wins; otherwise fall back | |
| // to the configured `tools.shell.defaultTimeoutMs` setting; otherwise | |
| // the built-in default. A configured default of 0 disables the timeout; | |
| // the per-call `timeout` param must be positive (validated upstream). |
— qwen3.7-max via Qwen Code /review
…mands Foreground shell commands started by the agent time out after a hardcoded 120s (DEFAULT_FOREGROUND_TIMEOUT_MS). A per-call `timeout` param can raise that for a single command, but there is no way to change the default for a project or session, so users repeatedly watch long-running commands fail at the 2-minute mark. Add a `tools.shell.defaultTimeoutMs` setting that feeds the existing timeout resolution. Precedence is now: per-call `timeout` param > setting > built-in default. When the setting is unset, behavior is unchanged; a value of 0 disables the timeout, matching the existing per-call semantics. Fixes QwenLM#5838
Address review on QwenLM#6628: - Add getShellDefaultTimeoutMs to mock configs in coreToolScheduler.test.ts and toAutoClassifierInput.test.ts (ShellTool construction now reads it). - Add minimum: 0 / maximum: 600000 to the defaultTimeoutMs setting so a negative value can't reach AbortSignal.timeout(); regenerate schema.
- shell.ts: debug-log the resolved foreground timeout (per-call vs configured default vs built-in) for observability - settingsSchema.ts: use type 'integer' for tools.shell.defaultTimeoutMs to match sibling visionBridgeTimeoutMs; regenerate settings.schema.json - config.test.ts: add loadCliConfig test asserting tools.shell.defaultTimeoutMs maps to Config.getShellDefaultTimeoutMs()
Address review on the configurable foreground shell timeout: - Config: validate shellDefaultTimeoutMs at construction, mirroring visionBridgeTimeoutMs, but allow 0 (disables the timeout). Negative, fractional, or out-of-range values now coerce to undefined instead of reaching AbortSignal.timeout() via a hand-edited settings.json that bypasses schema validation. - settingsSchema: mark tools.shell.defaultTimeoutMs requiresRestart, since Config.shellDefaultTimeoutMs is private readonly with no setter, so a mid-session change cannot take effect. - shell: when the timeout is disabled (effectiveTimeout === 0), suppress the long-run backgrounding hint instead of firing it on every command over ~1s via the longRunThresholdFor floor. - shell: correct the precedence comment; 0 disables only at the settings/default level, as the per-call timeout param rejects <= 0. Add coverage for negative/fractional coercion to the built-in default and for 0 disabling the timeout without emitting the spurious hint.
844d40e to
e562409
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| }, | ||
| defaultTimeoutMs: { | ||
| type: 'integer', | ||
| minimum: 0, |
There was a problem hiding this comment.
[Suggestion] Schema/runtime max mismatch: maximum: 600000 here, but the Config constructor (packages/core/src/config/config.ts:2047) accepts up to 2_147_483_647 (~24.8 days). A hand-edited settings.json bypasses the schema and reaches the runtime validator, which would accept e.g. 3600000 (1 hour). This contradicts the per-call timeout cap of 600000ms and the schema's stated intent.
Consider aligning the runtime validator's upper bound with the schema's maximum: 600000, or documenting that hand-edited settings are intentionally allowed higher and updating the test in packages/core/src/config/config.test.ts that asserts 2_147_483_647 is accepted.
— qwen3.7-max via Qwen Code /review
| DEFAULT_FOREGROUND_TIMEOUT_MS; | ||
| debugLogger.debug('resolved foreground shell timeout', { | ||
| perCallTimeout: this.params.timeout ?? null, | ||
| configuredDefault: this.config.getShellDefaultTimeoutMs() ?? null, |
There was a problem hiding this comment.
[Suggestion] this.config.getShellDefaultTimeoutMs() is called twice — once in the effectiveTimeout nullish-coalescing chain and again here for the debug log. While the getter is trivial (a private readonly field read), capturing it in a local eliminates the redundant call and guarantees the log reflects exactly the value used in the resolution.
| configuredDefault: this.config.getShellDefaultTimeoutMs() ?? null, | |
| const configuredDefault = this.config.getShellDefaultTimeoutMs(); | |
| const effectiveTimeout = | |
| this.params.timeout ?? | |
| configuredDefault ?? | |
| DEFAULT_FOREGROUND_TIMEOUT_MS; | |
| debugLogger.debug('resolved foreground shell timeout', { | |
| perCallTimeout: this.params.timeout ?? null, | |
| configuredDefault: configuredDefault ?? null, | |
| effectiveTimeout, | |
| }); |
— qwen3.7-max via Qwen Code /review
| const promise = invocation.execute(mockAbortSignal); | ||
| // Well past the 1000ms floor that would otherwise trip the hint. | ||
| await vi.advanceTimersByTimeAsync(120_000); | ||
| resolveShellExecution({ output: 'listening', exitCode: 0 }); |
There was a problem hiding this comment.
[Suggestion] This test asserts the long-run hint suppression via "foreground command ran for" and "is_background: true", but does NOT assert suppression of the timeout warning text "about to time out" (which lives at FOREGROUND_TIMEOUT_WARNING in shell.ts). When effectiveTimeout === 0, the warning is correctly suppressed because timeoutSignalStartedAt stays null, but a future change could break that guard without this test catching it.
| resolveShellExecution({ output: 'listening', exitCode: 0 }); | |
| expect(result.llmContent).not.toContain('foreground command ran for'); | |
| expect(result.llmContent).not.toContain('is_background: true'); | |
| expect(result.llmContent).not.toContain('about to time out'); |
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
All 5 prior Critical findings are fixed at this commit (minimum:0 added, test mocks updated, runtime validation added, requiresRestart corrected to true, type changed to integer). CI 30/30 green.
One terminal-only note: PR description claims "Fixes #5838", but the issue reporter explicitly said they were not asking for a configurable default timeout — they wanted to extend the timeout of an already-running process. The issue discussion narrowed to a Ctrl+B near-timeout hint (claimed by another contributor). Consider removing "Fixes #5838" to avoid auto-closing an issue whose narrowed scope remains unaddressed.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| this.params.timeout ?? | ||
| this.config.getShellDefaultTimeoutMs() ?? | ||
| DEFAULT_FOREGROUND_TIMEOUT_MS; | ||
| debugLogger.debug('resolved foreground shell timeout', { |
There was a problem hiding this comment.
[Suggestion] The effectiveTimeout === 0 (disabled) invariant is distributed across ~4 sites in this file (here, wasTimeout at ~2531/2696, and the long-run hint suppression at ~2651) with no structural link between them. A future maintainer adding a new consumer of effectiveTimeout (e.g., progress bar, telemetry) will likely find one guard but miss the others, silently breaking the disabled-timeout case.
Consider extracting a small helper like const isTimeoutDisabled = (ms: number) => ms === 0; and using it at all sites, or adding a cross-reference comment here pointing to the other guard sites.
— qwen3.7-max via Qwen Code /review
✅ Maintainer local verification — build + real tests all greenI built this PR from source in an isolated worktree and ran the real test/lint/typecheck gates. Everything passes, and I confirmed the new tests genuinely gate the new behavior (they fail when the PR code is reverted). Posting as a merge reference. Environment: PR HEAD What I ran
Before / After — the tests actually gate the featureTo confirm the new tests aren't no-ops, I overlaid
Behavior confirmed
Minor notes (non-blocking)
Verdict: LGTM. Builds clean, all real tests pass, tests demonstrably gate the behavior, no collateral breakage, default behavior unchanged. 中文说明（点击展开）✅ 维护者本地验证 —— 构建 + 真实测试全部通过我在隔离的 worktree 中从源码构建了本 PR，并运行了真实的测试 / lint / 类型检查。全部通过，并且确认新增测试确实是在为新行为把关（把 PR 代码回退后这些测试会失败）。作为合并参考发出。 环境： PR HEAD 运行的检查
Before / After —— 测试确实在为功能把关为确认新增测试不是空断言，我把 已确认的行为
次要说明（不阻塞合并）
结论：LGTM。 构建干净、真实测试全部通过、测试确实为行为把关、无连带破坏、默认行为不变。 Verified locally from source on an isolated worktree; screenshots are faithful renders of the actual test output. |
|
@qwen-code /triage |
Code ReviewIndependent proposal (before reading the diff): I would have added a Comparison: The PR matches this approach exactly — and goes further with thorough input validation in the Config constructor (rejecting values that would break No critical blockers found. The implementation is clean and correct:
No AGENTS.md violations. No over-abstraction. No code in the wrong package. No duplication. Test ResultsAll targeted test suites pass:
The Real-Scenario TestingAttempted tmux E2E test with 中文说明代码审查独立方案（读 diff 前）： 我会添加 对比： PR 完全匹配此方案——并且在 Config 构造函数中增加了完善的输入校验（拒绝会破坏 未发现关键阻断问题。实现干净正确：
无 AGENTS.md 违规。无过度抽象。无包错误。无重复代码。 测试结果所有目标测试套件通过：shell 255/255、config 6/6、scheduler 258/258、classifier 13/13、CLI config 1/1。 真实场景测试尝试使用 — Qwen Code · qwen3.7-max |
|
This PR does exactly what it says — adds a configurable default timeout for foreground shell commands — and does it well. Going back to my independent proposal: the PR matches it and adds meaningful extras (Config constructor validation against The problem is real (#5838 with screenshot evidence), the solution is minimal (3-line precedence chain, ~72 production lines total), and the test coverage is thorough (5 new precedence tests + 6 config validation tests + all existing tests still green at 255/258/13/1). The maintainer verified locally and approved. No reservations. This is ready to ship. 中文说明此 PR 完全实现了其声明——为前台 shell 命令添加可配置的默认超时——并且做得很好。 回到我的独立方案：PR 完全匹配并增加了有意义的额外内容（Config 构造函数对 问题真实（#5838 有截图证据），方案最小（3 行优先级链，约 72 行生产代码），测试覆盖完善（5 个新优先级测试 + 6 个 config 校验测试 + 所有既有测试仍通过 255/258/13/1）。维护者已本地验证并批准。 无顾虑。可以合入。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
4 participants
What this PR does
Adds a
tools.shell.defaultTimeoutMssetting that sets the default timeout for foreground shell commands the agent runs. Resolution precedence becomes: an explicit per-calltimeouton the shell tool takes priority, then this setting, then the built-in default. When the setting is unset, behavior is identical to today (120000 ms / 2 minutes); setting it to0disables the timeout, matching the existing per-call semantics.Why it's needed
Foreground commands currently time out after a hardcoded 2 minutes. A per-call
timeoutcan raise that for a single command, but there is no project- or session-level way to change the default, so users repeatedly watch legitimately long-running commands fail at the 2-minute mark and have to retry. A default-timeout setting lets a user who knows their workflow runs long raise the ceiling once. (Requested in #5838.)Reviewer Test Plan
How to verify
Unit tests pin the resolution precedence in
packages/core/src/tools/shell.test.ts(describeforeground timeout resolution (issue #5838)): a per-calltimeoutoverrides the setting; with no per-call value the setting is used; with neither, the built-in 120000 ms default is used; a setting of0arms no timeout signal at all.Run:
npx vitest run src/tools/shell.test.ts -t "issue #5838"inpackages/core→ 4 passing. Manually: set"tools": { "shell": { "defaultTimeoutMs": 600000 } }in settings and run a command that takes ~3 minutes — it now completes instead of timing out at 2 minutes.Evidence (Before & After)
N/A — no user-visible TUI change; behavior is exercised by unit tests (described above).
Tested on
Risk & Scope
0(disabled) default timeout lets a hung foreground command run longer before it is reclaimed — same tradeoff the existing per-calltimeoutalready allows. Default behavior is unchanged when the setting is absent.Linked Issues
Fixes #5838
中文说明
这个 PR 做了什么
新增
tools.shell.defaultTimeoutMs设置，用于配置 agent 运行的前台 shell 命令的默认超时时间。解析优先级为：shell 工具上显式的单次调用timeout参数最高，其次是该设置，最后是内置默认值。设置未配置时，行为与当前完全一致（120000 毫秒 / 2 分钟）；设为0则禁用超时，与现有的单次调用语义一致。为什么需要
前台命令目前在硬编码的 2 分钟后超时。单次调用的
timeout只能为单条命令临时提高上限，但没有办法在项目或会话级别修改默认值，因此用户会反复看到本应长时间运行的命令在 2 分钟处失败并被迫重试。默认超时设置让清楚自己工作流较慢的用户一次性提高上限。（见 #5838。）复核测试计划
packages/core/src/tools/shell.test.ts中的单元测试（describeforeground timeout resolution (issue #5838)）固定了解析优先级：单次调用timeout覆盖设置；无单次调用值时使用设置；两者都无时使用内置的 120000 毫秒默认值；设置为0时完全不启用超时信号。在
packages/core运行：npx vitest run src/tools/shell.test.ts -t "issue #5838"→ 4 个测试通过。手动验证：在设置中写入"tools": { "shell": { "defaultTimeoutMs": 600000 } }，运行一个约 3 分钟的命令 —— 现在会正常完成，而不是在 2 分钟处超时。风险与范围
0（禁用）的默认超时会让卡住的前台命令运行更久才被回收 —— 这与现有的单次调用timeout的取舍相同。设置未配置时默认行为不变。Fixes #5838