Skip to content

Make bash foreground timeout configurable#3018

Open
GTC2080 wants to merge 2 commits into
esengine:main-v2from
GTC2080:GTC/issue-2982-bash-timeout
Open

Make bash foreground timeout configurable#3018
GTC2080 wants to merge 2 commits into
esengine:main-v2from
GTC2080:GTC/issue-2982-bash-timeout

Conversation

@GTC2080
Copy link
Copy Markdown
Contributor

@GTC2080 GTC2080 commented Jun 4, 2026

Fixes #2982

Summary

  • Keep the historical 120s foreground bash safety cap when tools.bash_timeout_seconds is omitted.
  • Use *int config plumbing so explicit tools.bash_timeout_seconds = 0 remains an opt-in unlimited setting, while positive values set a custom cap.
  • Preserve cancellation/process-tree cleanup and background job behavior, and update config docs plus built-in testing guidance.

Validation

  • D:\Go\go\bin\go.exe test ./internal/config -run 'TestBashTimeoutSeconds|TestRenderTOMLRoundTrips' -count=1 -v
  • D:\Go\go\bin\go.exe test ./internal/tool/builtin -run 'TestBash(ForegroundTimeoutConfig|ExplicitZeroTimeoutDoesNotCapForeground|CancelReturnsPromptly)|TestWorkspacePassesBashTimeout' -count=1 -v
  • D:\Go\go\bin\go.exe test ./... -count=1 with isolated HOME, USERPROFILE, XDG_CONFIG_HOME, APPDATA, and LOCALAPPDATA
  • D:\Go\go\bin\go.exe vet ./...
  • pnpm --dir desktop/frontend install --frozen-lockfile
  • pnpm --dir desktop/frontend build
  • D:\Go\go\bin\go.exe test ./... -count=1 in desktop/
  • git diff --check
  • Performance: go test ./internal/tool/builtin -run '^$' -bench 'BenchmarkBashForegroundTimeout' -benchmem -count=5
    • Explicit zero path: 2.747-3.784 ns/op, 0 B/op, 0 allocs/op
    • Configured 120s cap path: 270.5-363.7 ns/op, 272 B/op, 4 allocs/op

No UI changes, so screenshots are not applicable.

@github-actions github-actions Bot added the v2 Go rewrite (1.x) — main-v2 branch, active development label Jun 4, 2026
@esengine
Copy link
Copy Markdown
Owner

esengine commented Jun 4, 2026

Thanks — making the foreground bash timeout configurable is genuinely useful, and the implementation (config plumbing, context.WithTimeoutCause, the negative-normalize guard, the sleep/kill tests) is clean. I built it and ran the new tests locally; the mechanism works.

One change before this can land: please keep 120s as the default rather than 0 (unlimited).

Today every foreground bash command is capped at 120s, which is the safety net that lets the agent recover from a runaway command (an infinite loop, a blocking read) on its own. This PR makes bash_timeout_seconds = 0 the default, so out of the box that protection is gone — a wedged foreground command now hangs the turn until the session context cancels. TestBashZeroTimeoutDoesNotCapForeground bakes that in. Configurable: yes. New default of "no cap": that's a separate, riskier change I'd rather not ship by default.

The wrinkle is that a plain int defaulting to its zero value can't distinguish "user didn't set it → 120" from "user explicitly wants unlimited → 0". Two clean ways to resolve it:

  • Pointer *int (toml:"bash_timeout_seconds"): nil → 120 default, explicit 0 → unlimited, N → N. BashTimeoutSeconds() resolves nil to 120.
  • Sentinel: default the field to 120 in config.Default(), treat -1 (or any negative) as "unlimited", keep 0… — messier; I'd go with the pointer.

So: default 120s, config overrides it, and explicit 0/unlimited stays available for anyone who wants it. Update the rendered default + README line to 120 and flip TestBashZeroTimeoutDoesNotCapForeground to assert against an explicit 0 (the default path should now assert a 120s cap). Ping me and I'll merge.

@GTC2080
Copy link
Copy Markdown
Contributor Author

GTC2080 commented Jun 4, 2026

Updated in e0ba2a5.

Changes made per review:

  • tools.bash_timeout_seconds is now *int.
  • omitted config now resolves to the historical 120s foreground cap.
  • explicit bash_timeout_seconds = 0 remains the opt-in unlimited mode.
  • docs/rendered config now show 120 by default.
  • tests now cover nil/default 120, explicit in-memory zero, TOML-parsed explicit zero, negative fallback, and the explicit-zero foreground execution path.

Validation rerun:

  • targeted config and bash timeout tests
  • full root go test ./... -count=1 with isolated user config env
  • go vet ./...
  • desktop frontend install/build
  • desktop go test ./... -count=1
  • benchmark rerun for explicit-zero and configured-cap paths

GTC2080 added 2 commits June 4, 2026 21:38
Keep the historical 120s foreground bash cap when tools.bash_timeout_seconds is omitted, while preserving explicit 0 as the opt-in unlimited setting.
@GTC2080 GTC2080 force-pushed the GTC/issue-2982-bash-timeout branch from e0ba2a5 to 1d36378 Compare June 4, 2026 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Go rewrite (1.x) — main-v2 branch, active development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: 沙箱限时2分钟机制没有必要

2 participants