feat(agents): allow per-invocation timeout override on shell/http tools #113

Merged
CoreRasurae merged 1 commits from feature/m1-shell-timeout-override into master 2026-08-05 12:40:43 +00:00
Member

Summary

Adds an optional per-invocation timeout argument to the shell and http_request tools so an LLM driving them can raise the execution limit for a single long-running call — instead of being told to "increase the timeout config value", which it cannot do at runtime. The model previously threaded timeout N into the call, corrupting the command.

  • New immutable TimeoutPolicy value object resolves the effective timeout.
  • Generic max_timeout config field (default 120s) bounds any override; an optional shell-only shell_max_timeout refines the ceiling for shell alone and is never consulted by http_request.
  • Absent the argument, behaviour is byte-for-byte unchanged. Invalid overrides are rejected before any subprocess is spawned / request is sent. Timeout errors now direct a retry with a larger timeout argument.

Specification / ADR

  • docs/adr/ADR-2030-tool-calling-spec-extensions.md D-9 (accepted revision).
  • Actor Configuration Standard §4.5 + §21.1 → Version 1.2.0 (adds the generic max_timeout field).

Testing

  • Behave: features/shell_timeout_override.feature — 32 scenarios (shell + http, including one proving shell_max_timeout does not leak into http_request). Full suite green (2943 scenarios, 0 failed).
  • Robot: robot/shell_timeout_override.robot — 4 real-subprocess integration tests.
  • ASV: benchmarks/timeout_policy_benchmark.py.
  • Coverage 96.7% (new files: timeout_policy.py 100%, llm_tools.py 100%, tool.py 98%). lint / format / typecheck / security / dead_code all green.

Closes #111

## Summary Adds an optional per-invocation `timeout` argument to the `shell` and `http_request` tools so an LLM driving them can raise the execution limit for a single long-running call — instead of being told to "increase the timeout config value", which it cannot do at runtime. The model previously threaded `timeout N` into the call, corrupting the command. - New immutable `TimeoutPolicy` value object resolves the effective timeout. - Generic `max_timeout` config field (default 120s) bounds any override; an optional shell-only `shell_max_timeout` refines the ceiling for `shell` alone and is **never** consulted by `http_request`. - Absent the argument, behaviour is byte-for-byte unchanged. Invalid overrides are rejected before any subprocess is spawned / request is sent. Timeout errors now direct a retry with a larger `timeout` argument. ## Specification / ADR - `docs/adr/ADR-2030-tool-calling-spec-extensions.md` **D-9** (accepted revision). - Actor Configuration Standard §4.5 + §21.1 → **Version 1.2.0** (adds the generic `max_timeout` field). ## Testing - **Behave:** `features/shell_timeout_override.feature` — 32 scenarios (shell + http, including one proving `shell_max_timeout` does not leak into `http_request`). Full suite green (2943 scenarios, 0 failed). - **Robot:** `robot/shell_timeout_override.robot` — 4 real-subprocess integration tests. - **ASV:** `benchmarks/timeout_policy_benchmark.py`. - Coverage **96.7%** (new files: `timeout_policy.py` 100%, `llm_tools.py` 100%, `tool.py` 98%). lint / format / typecheck / security / dead_code all green. Closes #111
CoreRasurae added this to the v2.1.0 milestone 2026-08-04 23:01:34 +00:00
CoreRasurae added the
Type
Feature
label 2026-08-04 23:01:35 +00:00
hurui200320 approved these changes 2026-08-05 05:51:56 +00:00
hurui200320 left a comment
Member

PR Review: !113 (Ticket #111)

Verdict: Approve

The implementation cleanly realizes ADR-2030 D-9 and the Actor Configuration Standard §4.5 / §21.1 (Version 1.2.0) changes. The TimeoutPolicy value object is a good abstraction, validation happens before any subprocess/request, the timeout control argument is never concatenated into the shell command, and the error messages now point the model at the actionable per-call lever. Tests (Behave, Robot, ASV) are comprehensive and the commit/branch hygiene matches the ticket metadata. Only minor gaps/note-level items remain.

Critical Issues

None

Major Issues

None

Minor Issues

  1. Missing regression scenario for the original bug shape (timeout + positional args)

    • File: features/shell_timeout_override.feature
    • The original failure trace in #111 was the model injecting timeout N into the positional args list, which got concatenated onto the command line. The code now correctly reads args.get("timeout") separately from args["args"], but there is no Behave/Robot scenario that supplies both a per-call timeout and a positional args list and asserts the output is uncorrupted. Adding one scenario would close the regression loop explicitly.
  2. TimeoutPolicy module docstring is slightly stale

    • File: src/cleveractors/agents/timeout_policy.py, lines 1–9
    • The docstring says the policy "backs the shell tool's per-invocation timeout argument today … and is reusable by any future tool." Since _http_request_tool also uses TimeoutPolicy today, the wording implies only shell consumes it. Rephrase to mention both current consumers or drop the "today / future" framing.
  3. Invalid timeout ceilings are validated lazily

    • File: src/cleveractors/agents/tool.py, lines 325–362
    • An invalid max_timeout or shell_max_timeout is only detected when the first timeout-honouring tool is invoked, not at ToolAgent construction. For faster failure of misconfigured agents, consider validating these config fields in __init__ (or in the policy builders called from __init__).
  4. Coverage sits between two documented thresholds

    • The PR reports 96.7% coverage. This passes the noxfile.py constant (COVERAGE_THRESHOLD = 96.5), but CONTRIBUTING.md states the project-specific enforced merge gate is 97%. Please confirm the actual nox -s coverage_report output; if it is below 97%, add targeted tests to meet the documented gate (the project may also need to reconcile the noxfile constant with CONTRIBUTING.md).

Nits

  1. ShellTimeoutLib exposes a raw ValueError on bad timeout strings

    • File: robot/ShellTimeoutLib.py, line 36
    • float(timeout) will raise ValueError if the Robot test passes a non-numeric string. Wrapping this in an ExecutionError (or validating and raising AssertionError) would give clearer Robot failure output.
  2. Empty benchmark teardown

    • File: benchmarks/timeout_policy_benchmark.py, lines 32–33
    • The teardown method is a no-op. It is harmless, but can be removed or annotated with a comment if state cleanup is truly unnecessary.

Summary

Solid, well-scoped change. The per-invocation timeout argument is implemented correctly for both shell and http_request, the ceiling logic (max_timeout / shell_max_timeout) is clean and properly separated, and the schema/documentation/spec updates are all in place. The only substantive follow-ups are adding the timeout+args regression scenario and clarifying the coverage threshold situation; neither blocks approval.

## PR Review: !113 (Ticket #111) ### Verdict: Approve The implementation cleanly realizes ADR-2030 D-9 and the Actor Configuration Standard §4.5 / §21.1 (Version 1.2.0) changes. The `TimeoutPolicy` value object is a good abstraction, validation happens before any subprocess/request, the `timeout` control argument is never concatenated into the shell command, and the error messages now point the model at the actionable per-call lever. Tests (Behave, Robot, ASV) are comprehensive and the commit/branch hygiene matches the ticket metadata. Only minor gaps/note-level items remain. ### Critical Issues None ### Major Issues None ### Minor Issues 1. **Missing regression scenario for the original bug shape (`timeout` + positional `args`)** - **File:** `features/shell_timeout_override.feature` - The original failure trace in #111 was the model injecting `timeout N` into the positional `args` list, which got concatenated onto the command line. The code now correctly reads `args.get("timeout")` separately from `args["args"]`, but there is no Behave/Robot scenario that supplies **both** a per-call `timeout` and a positional `args` list and asserts the output is uncorrupted. Adding one scenario would close the regression loop explicitly. 2. **`TimeoutPolicy` module docstring is slightly stale** - **File:** `src/cleveractors/agents/timeout_policy.py`, lines 1–9 - The docstring says the policy "backs the `shell` tool's per-invocation `timeout` argument today … and is reusable by any future tool." Since `_http_request_tool` also uses `TimeoutPolicy` today, the wording implies only `shell` consumes it. Rephrase to mention both current consumers or drop the "today / future" framing. 3. **Invalid timeout ceilings are validated lazily** - **File:** `src/cleveractors/agents/tool.py`, lines 325–362 - An invalid `max_timeout` or `shell_max_timeout` is only detected when the first timeout-honouring tool is invoked, not at `ToolAgent` construction. For faster failure of misconfigured agents, consider validating these config fields in `__init__` (or in the policy builders called from `__init__`). 4. **Coverage sits between two documented thresholds** - The PR reports **96.7%** coverage. This passes the `noxfile.py` constant (`COVERAGE_THRESHOLD = 96.5`), but `CONTRIBUTING.md` states the project-specific enforced merge gate is **97%**. Please confirm the actual `nox -s coverage_report` output; if it is below 97%, add targeted tests to meet the documented gate (the project may also need to reconcile the noxfile constant with `CONTRIBUTING.md`). ### Nits 1. **`ShellTimeoutLib` exposes a raw `ValueError` on bad timeout strings** - **File:** `robot/ShellTimeoutLib.py`, line 36 - `float(timeout)` will raise `ValueError` if the Robot test passes a non-numeric string. Wrapping this in an `ExecutionError` (or validating and raising `AssertionError`) would give clearer Robot failure output. 2. **Empty benchmark `teardown`** - **File:** `benchmarks/timeout_policy_benchmark.py`, lines 32–33 - The `teardown` method is a no-op. It is harmless, but can be removed or annotated with a comment if state cleanup is truly unnecessary. ### Summary Solid, well-scoped change. The per-invocation `timeout` argument is implemented correctly for both `shell` and `http_request`, the ceiling logic (`max_timeout` / `shell_max_timeout`) is clean and properly separated, and the schema/documentation/spec updates are all in place. The only substantive follow-ups are adding the `timeout`+`args` regression scenario and clarifying the coverage threshold situation; neither blocks approval.
Author
Member

In the review issuecomment 321954, Minor, Issue 4, the coverage is a non-issue, since the 96.5% threshold rounds to the 97% threshold in the documentation and it is meant to be that way. The current coverage of 96.7% is within the specified range.

In the review issuecomment 321954, Minor, Issue 4, the coverage is a non-issue, since the 96.5% threshold rounds to the 97% threshold in the documentation and it is meant to be that way. The current coverage of 96.7% is within the specified range.
Author
Member

Thanks for the review, @hurui200320! Addressed the Minor issues; Nit 1 fixed, Nit 2 kept as-is (see below). Confirmed all four against §4.5/ADR-2030 before applying — none conflict with the spec.

Minor 1 (missing timeout+args regression scenario) — Added. Two new Behave scenarios in features/shell_timeout_override.feature reproduce the original bug shape directly: a positional args list containing the literal tokens "timeout"/"99" alongside a real per-call timeout control argument, asserting the output is exactly the harmless concatenated text ("marker timeout 99") and that the override still functions (a sleep 2 succeeds under timeout=4 even with args present). Also added a matching Robot regression test.

Minor 2 (stale TimeoutPolicy docstring) — Fixed. Now states it backs the per-invocation timeout argument on both shell and http_request today, dropping the "today / future" framing.

Minor 3 (lazy ceiling validation) — Fixed. ToolAgent.__init__ now eagerly builds both timeout policies and converts a ValueError into AgentCreationError, so a misconfigured tools_max_timeout/shell_max_timeout fails at construction, not on first call. As a consequence, the two call sites (_execute_shell_command, _http_request_tool) no longer need their own try/except ValueError — the ceiling is guaranteed valid by the time they run, per ADR-2030 D-9 (updated in place with a bullet documenting this). The two existing "invalid ceiling" scenarios now assert AgentCreationError at construction time instead of ExecutionError at call time.

Minor 4 (coverage 96.5% vs 97%) — No code change; per @CoreRasurae's earlier reply this is an intentional pre-existing project-wide threshold choice, outside this ticket's scope (not part of the Actor Configuration Standard or ADR-2030). Independently verified: a coverage run scoped to the feature-relevant BDD files shows timeout_policy.py and llm_tools.py at 100%, and none of the changed/added lines in tool.py appear in that scoped run's missing-lines list — the new eager-validation code is fully exercised. Also confirmed the full 2945-scenario suite still passes with 0 failures after these changes (no regressions from touching the shared ToolAgent.__init__ path).

Nit 1 (ShellTimeoutLib raw ValueError) — Fixed. Added a _parse_timeout helper that raises a clear AssertionError on a non-numeric timeout string; added a Robot test case (Non Numeric Timeout Argument Fails With A Clear Message) exercising it.

Nit 2 (empty benchmark teardown) — Left as-is. It matches the established setup/time_*/teardown lifecycle convention used by the other benchmarks in this repo (e.g. hello_benchmark.py), so removing it would make this benchmark inconsistent with house style for near-zero benefit.

Ran nox -s lint / format --check / typecheck / dead_code / security_scan (all green), the full Behave suite (146 features, 2945 scenarios, 0 failed), and the Robot integration suite for this feature (5/5 passed) after applying the above.

Thanks for the review, @hurui200320! Addressed the Minor issues; Nit 1 fixed, Nit 2 kept as-is (see below). Confirmed all four against §4.5/ADR-2030 before applying — none conflict with the spec. **Minor 1 (missing `timeout`+`args` regression scenario)** — Added. Two new Behave scenarios in `features/shell_timeout_override.feature` reproduce the original bug shape directly: a positional `args` list containing the literal tokens `"timeout"`/`"99"` alongside a real per-call `timeout` control argument, asserting the output is exactly the harmless concatenated text (`"marker timeout 99"`) and that the override still functions (a `sleep 2` succeeds under `timeout=4` even with `args` present). Also added a matching Robot regression test. **Minor 2 (stale `TimeoutPolicy` docstring)** — Fixed. Now states it backs the per-invocation `timeout` argument on *both* `shell` and `http_request` today, dropping the "today / future" framing. **Minor 3 (lazy ceiling validation)** — Fixed. `ToolAgent.__init__` now eagerly builds both timeout policies and converts a `ValueError` into `AgentCreationError`, so a misconfigured `tools_max_timeout`/`shell_max_timeout` fails at construction, not on first call. As a consequence, the two call sites (`_execute_shell_command`, `_http_request_tool`) no longer need their own `try/except ValueError` — the ceiling is guaranteed valid by the time they run, per ADR-2030 D-9 (updated in place with a bullet documenting this). The two existing "invalid ceiling" scenarios now assert `AgentCreationError` at construction time instead of `ExecutionError` at call time. **Minor 4 (coverage 96.5% vs 97%)** — No code change; per @CoreRasurae's earlier reply this is an intentional pre-existing project-wide threshold choice, outside this ticket's scope (not part of the Actor Configuration Standard or ADR-2030). Independently verified: a coverage run scoped to the feature-relevant BDD files shows `timeout_policy.py` and `llm_tools.py` at 100%, and none of the changed/added lines in `tool.py` appear in that scoped run's missing-lines list — the new eager-validation code is fully exercised. Also confirmed the full 2945-scenario suite still passes with 0 failures after these changes (no regressions from touching the shared `ToolAgent.__init__` path). **Nit 1 (`ShellTimeoutLib` raw `ValueError`)** — Fixed. Added a `_parse_timeout` helper that raises a clear `AssertionError` on a non-numeric `timeout` string; added a Robot test case (`Non Numeric Timeout Argument Fails With A Clear Message`) exercising it. **Nit 2 (empty benchmark `teardown`)** — Left as-is. It matches the established `setup`/`time_*`/`teardown` lifecycle convention used by the other benchmarks in this repo (e.g. `hello_benchmark.py`), so removing it would make this benchmark inconsistent with house style for near-zero benefit. Ran `nox -s lint / format --check / typecheck / dead_code / security_scan` (all green), the full Behave suite (146 features, 2945 scenarios, 0 failed), and the Robot integration suite for this feature (5/5 passed) after applying the above.
CoreRasurae force-pushed feature/m1-shell-timeout-override from e3b675b7d2 to f19714a3da 2026-08-05 10:27:18 +00:00 Compare
CoreRasurae added 1 commit 2026-08-05 12:13:21 +00:00
feat(agents): allow per-invocation timeout override on shell/http tools
CI / lint (pull_request) Successful in 54s
CI / typecheck (pull_request) Successful in 1m28s
CI / security (pull_request) Successful in 1m27s
CI / quality (pull_request) Successful in 53s
CI / build (pull_request) Successful in 1m47s
CI / integration_tests (pull_request) Successful in 4m51s
CI / benchmark (pull_request) Has been cancelled
CI / unit_tests (pull_request) Successful in 6m0s
CI / coverage (pull_request) Successful in 6m9s
CI / status-check (pull_request) Successful in 13s
CI / lint (push) Successful in 55s
CI / typecheck (push) Successful in 1m41s
CI / quality (push) Successful in 1m44s
CI / security (push) Successful in 2m5s
CI / build (push) Successful in 50s
CI / integration_tests (push) Successful in 3m32s
CI / unit_tests (push) Successful in 4m59s
CI / coverage (push) Failing after 16m5s
CI / benchmark (push) Failing after 19m26s
CI / status-check (push) Failing after 4s
0133520555
The tool agent `timeout` config field (Actor Configuration Standard §4.5,
default 1s) was a static per-agent value, and the shell/http_request timeout
errors told the model to "increase the timeout config value" — an action an
LLM driving the tool cannot take at runtime. The model would instead thread
`timeout N` into the call, corrupting the command.

Both tools now accept an optional per-call `timeout` argument, resolved by a
new immutable `TimeoutPolicy` value object and bounded by a generic
`tools_max_timeout` config field (default 120s). An optional shell-only
`shell_max_timeout` refines the ceiling for `shell` alone and is never
consulted by `http_request`. Absent the argument, behaviour is byte-for-byte
unchanged. Invalid overrides are rejected before any subprocess is spawned or
request is sent; the timeout errors now direct a retry with a larger `timeout`
argument.

Implements ADR-2030 D-9 (accepted revision) and Actor Configuration Standard
§4.5 / §21.1 (Version 1.2.0).

ISSUES CLOSED: #111
CoreRasurae force-pushed feature/m1-shell-timeout-override from f19714a3da to 0133520555 2026-08-05 12:13:21 +00:00 Compare
CoreRasurae merged commit 0133520555 into master 2026-08-05 12:40:43 +00:00
CoreRasurae deleted branch feature/m1-shell-timeout-override 2026-08-05 12:40:56 +00:00
Sign in to join this conversation.
No Reviewers
No Label
Type
Feature
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: cleveragents/cleveractors-core#113