-
Notifications
You must be signed in to change notification settings - Fork 162
feat(shell): adaptive timeouts and a wait-for-condition primitive - #3402
feat(shell): adaptive timeouts and a wait-for-condition primitive #3402kovtcharov-amd wants to merge 4 commits into
Conversation
...it primitive A shell command was killed at 30 seconds no matter what it was, so a test suite, a build or an install never finished — and while `timeout` was already a parameter the model could raise, nothing told it how long the command it was about to run should take, so it almost never did. The timeout now comes from the command itself: test runners get 900s, builds and installs 1800s, VCS and network calls 300s, everything else the unchanged 30s. The applied value and its class come back in the result, and an explicit `timeout=` still wins. Out-of-range values are refused rather than clamped, so a command is never killed at a limit its caller did not choose. `wait_for_condition` replaces sleep-and-recheck: it polls a predicate against a monotonic deadline inside one call, so waiting for a server to answer or a build to land costs one agent step instead of one per check. It is bounded (600s ceiling, 5-60s poll interval), gated behind the same confirmation and guardrails as the command it polls with, charged once against the shell rate limit, and interrupted by the agent's cancel signal.
The tool registry takes a tool's description from its docstring — @tool's `description=` and `parameters=` kwargs are swallowed and ignored — and the non-native prompt path renders only the first line of it. The class defaults were in the ignored kwarg, so the model would never have seen them. They now lead the docstring, with a test that fails if a class default drifts out of that line. Also documents both tools in the mixin spec and the flagship's tool surface.
...eturns A 5-second timeout took over two minutes and never returned: `subprocess.run` kills only the process it launched, then re-enters `communicate()` with no timeout, so a surviving grandchild holding the pipes blocks the call for as long as it lives. With 30-minute build timeouts now reachable, that hang would have held the agent loop for an hour instead of three minutes. The executor now drives Popen itself and kills the whole tree on expiry — `taskkill /T` on Windows, the process group on POSIX — then reads the buffered output with a short grace. Verified live: the same command that hung returns in 5.1s with its partial output intact. That partial output is also truncated to 10K now, like the success path always was. A command killed at 30 minutes has printed far more than one killed at 30 seconds; the live run returned 806KB straight into the model's context.
The flagship shows the model a subset of its registry per turn, chosen by bundle. A tool in no bundle is registered but unreachable, so wait_for_condition joins the shell bundle in both the chat and full profiles. Also repoints the granted-CLI argv tests at Popen, which is what the executor drives now — the behaviour they pin (a granted binary never goes through cmd.exe) is unchanged.
|
Verdict: Request changes This gives shell commands a timeout that matches what they are (test runs get 15 minutes, builds 30, network calls 5, everything else 30 seconds), makes a timed-out command actually die instead of hanging on a surviving grandchild, and adds a What needs fixing before merge:
Real-world evidenceThe automated evidence harness failed to run on this PR ( 🔍 Technical details🔴 Critical
🟡 ImportantCancel is not observed during a long The
The non-
Same gap in the docs: 🟢 Minor
Strengths
|
The agent could not run anything that takes minutes. A shell command was killed at 30 seconds no matter what it was, so a test suite, a build or an install never finished — and a timed-out command on Windows did not even stop: a verified 5-second timeout took over two minutes and never returned, because the kill missed the grandchild still holding the pipes. Waiting was just as bad: with no way to wait for a condition, the agent slept and re-checked, which is what a user sees as spinning and what burns the loop's step budget. Now the timeout comes from what the command is — 900s for test runners, 1800s for builds and installs, 300s for git/network calls, 30s for everything else — the kill actually fires, and
wait_for_conditionwaits for a server to answer or a file to appear in one step instead of one per check.Closes #3382.
Needs a rebase once #3394 (bypass mode) and #3380 (persistent shell) land — all three touch
shell_tools.py. The diff there was kept as small as the change allows; the class table lives in its own module.🔍 Technical details
src/gaia/agents/tools/command_timeouts.py— four named classes, one default each, plus the classifier. Wrappers are transparent (python -m pytest,uv run pytest,npx jest), and a pipeline takes its longest segment.timeout=still wins; out of range is refused, not clamped, so a command is never killed at a limit its caller did not choose.@tool'sdescription=/parameters=kwargs are swallowed and ignored, and the non-native prompt path renders only the first line of__doc__. A test fails if a class default drifts out of that line.Popenand kills the process tree on expiry (taskkill /T/ process group), then reads the buffered output with a grace period. Partial output is truncated to 10K like the success path — the live run returned 806KB before that.wait_for_conditionis confirmation-gated, runs its predicate throughrun_shell_command(same allowlist, refused before the prompt), is charged once against the shell rate limit rather than once per probe, and waits on the agent's cancel event so Stop lands immediately.Test plan
python -m pytest tests/unit/test_shell_adaptive_timeouts.py tests/unit/test_shell_guardrails.py tests/unit/test_shell_output_encoding.py tests/unit/test_skill_binary_grants.py -q— 376 pass. Covers each timeout class end-to-end tosubprocess, the wait primitive's deadline expiry, and the three already-true behaviours (applied timeout returned,timed_outflag, partial output on timeout).python -m pytest hub/agents/gaia/python/tests/test_full_tool_bundles.py -q— the new tool is in the shell bundle, so the flagship's per-turn selector can surface it.python util/lint.py --all— clean.gaia eval agent --category tool_selectionon Gemma-4-E4B — judged pass rate unchanged vs thegemma-4-e4b-d71cd914baseline (75% → 75%), avg score 8.4 → 8.7.known_path_readwent FAIL → PASS.smart_discoveryfailed for an environment reason, not this change: the agent called no tools and refused over a path from another worktree left in this box's memory store. The elapsed-time flags are a loaded dev box, not the change.rag_quality + context_retention + tool_selectioncheck — Eval gate fails identically on main: tool_selection scenarios die in 3s and claude's stderr is swallowed #3341 tracks it failing identically on main.