feat(agents): inherit tool timeout ceilings from parent LLM agent #141
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#140 Ephemeral tool-call ToolAgent instances do not inherit
tools_max_timeout/shell_max_timeout from the parent LLM agent
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!141
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m1-inherit-tool-timeout-ceilings"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
type: llmagent'stools_max_timeout/shell_max_timeoutconfig now bounds the per-invocationtimeouttool-call argument forshell/http_requestcalls that agent dispatches internally, exactly as it already does for a directly-configuredtype: toolagent.timeoutitself already inherited correctly and is untouched.ToolAgent-construction sites inLLMAgent._execute_tool_loopnow share one_build_ephemeral_tool_config()helper (Template/Factory Method) instead of three near-identical inline dict literals, closing the drift that lettools_max_timeout/shell_max_timeoutdiverge fromtimeoutin the first place.docs/adr/ADR-2030-tool-calling-spec-extensions.mdwith D-10 anddocs/index.md§4.4 (Version 1.6.0, §21.1 Revision History) per the spec-revision procedure, since this makes the two fields usable/discoverable on atype: llmagent for the first time.timeouttool-call argument mechanism (ADR-2030 D-9,TimeoutPolicy) is unaffected: a model-supplied override still takes precedence over the (now possibly inherited) default/ceiling for that one call.Closes #140
Test plan
nox -s lint/nox -s format -- --check— greennox -s typecheck— green (0 errors)nox -s security_scan/nox -s dead_code— green, no findingsnox -s unit_tests— 154 features, 3089 scenarios, 0 failed (6 new scenarios infeatures/llm_agent_tool_loop.feature)nox -s coverage_report— 96.9% (threshold 96.5%); no new uncovered linesnox -s integration_tests— 364 Robot tests, 0 failed (2 new tests inrobot/llm_tool_calling.robot)nox -s benchmark_regression -- --quick— BENCHMARKS NOT SIGNIFICANTLY CHANGED (expected: config-threading only, no perf-sensitive path touched)tools_max_timeout/shell_max_timeoutfrom the parent LLM agent9eb8378d6449e7e263a0PR Review: !141 (Ticket #140)
Verdict: Approve
The implementation correctly threads
tools_max_timeoutandshell_max_timeoutfrom a parenttype: llmagent into the ephemeralToolAgentinstances it constructs per dispatched tool call, satisfying the functional requirements of #140. The code is clean, well-documented, and the spec/ADR updates are consistent. No critical or major issues were found.Critical Issues
None
Major Issues
None
Minor Issues
Missing Behave coverage for the post-loop stuck-model synthesis dispatch site
features/llm_agent_tool_loop.feature_build_ephemeral_tool_config()helper, the functional risk is low, but the requirement coverage is incomplete.tools_max_timeoutceiling is honored there.Invalid ceiling test only exercises
tools_max_timeout: 0features/llm_agent_tool_loop.featuretools_max_timeoutorshell_max_timeoutmust raiseAgentCreationErrorat ephemeral-agent construction. The new scenario only teststools_max_timeout: 0; it does not cover non-numeric, negative, non-finite, orshell_max_timeoutinvalid values.shell_max_timeoutcases.Robot integration tests do not exercise inherited
shell_max_timeoutrobot/llm_tool_calling.robottools_max_timeoutinheritance but does not include a case forshell_max_timeoutoverriding the shell-only ceiling.shell_max_timeouton the LLM agent and verifies a per-calltimeoutabovetools_max_timeoutbut belowshell_max_timeoutis accepted for ashellcall.Nits
ToolAgentdefaultssrc/cleveractors/agents/llm.py_build_ephemeral_tool_config()hardcodes120.0andNoneas fallbacks. These matchToolAgent.__init__defaults today, but if those defaults ever change, this helper could silently drift out of sync.ToolAgentdefaults via module constants or a shared helper to make the coupling explicit. (This is consistent with the existingtimeout: 1pattern, so purely a nit.)Summary
This is a focused, well-scoped change. Extracting the ephemeral-tool-agent config construction into
_build_ephemeral_tool_config()removes the drift that allowed the timeout ceilings to diverge, and the ADR/spec updates follow the established process. The implementation is functionally correct and backward-compatible. Addressing the minor test-coverage gaps would make the PR fully satisfy the acceptance criteria, but the current state is safe to approve.49e7e263a0cb48550f16@hurui200320 Thanks for the review — all three Minor items and the Nit are addressed in
cb48550(force-pushed to this branch). Details below, checked againstdocs/index.md§4.4/§4.5, ADR-2030 D-9/D-10, and issue #140's acceptance criteria before applying.Minor 1 — Missing Behave coverage for the post-loop stuck-model synthesis dispatch site
Done. Added a scenario ("A post-loop stuck-model synthesis tool dispatch also honors an inherited tools_max_timeout ceiling") in
features/llm_agent_tool_loop.featurethat drives the loop to the third_build_ephemeral_tool_config()call site (tool_config_s, the post-loop synthesis dispatch) with ashellcall carrying atimeoutabove the built-in default but within an inheritedtools_max_timeout: 300, and asserts it succeeds. This now gives Behave coverage across all three dispatch sites named in #140's acceptance criteria (normal round — already covered; budget-exhaustion synthesis — already covered; post-loop synthesis — new).Minor 2 — Invalid ceiling test only exercised tools_max_timeout: 0
Done. Added three scenarios alongside the existing
tools_max_timeout: 0case:tools_max_timeout(-5)tools_max_timeout(not_a_number)tools_max_timeout(nan)shell_max_timeout(-5, with a validtools_max_timeout)All four assert
AgentCreationErrorat ephemeral-agent construction, matching #140's "non-numeric, zero, negative, or non-finite... for either tools_max_timeout or shell_max_timeout" criterion. (Implementation note: the negative cases reuse the existing integer-typed Given steps rather than new step definitions, since-5is representable there directly — no behavioral difference, just avoided adding redundant step functions.)Minor 3 — Robot integration tests do not exercise inherited shell_max_timeout
Done. Added
LLM Agent's shell_max_timeout Raises The Shell-Only Ceiling For A Dispatched Shell Call (Ticket 140)torobot/llm_tool_calling.robot, using theshell_max_timeoutparameter already wired intoCreate Executor With Shell Tool Calling Agent(robot/ToolCallingTestLib.py). It configurestools_max_timeout=2/shell_max_timeout=100and issues a per-calltimeout=50(above the generic ceiling, below the shell-only one) — exactly the case you suggested.Nit — Hardcoded fallback values mirror ToolAgent defaults
Done. Extracted
DEFAULT_TOOLS_MAX_TIMEOUT = 120.0andDEFAULT_SHELL_MAX_TIMEOUT = Noneas module-level constants insrc/cleveractors/agents/tool.py, and bothToolAgent.__init__andLLMAgent._build_ephemeral_tool_config()now read from them instead of each hardcoding the literals independently. This closes the exact drift risk you flagged — the two call sites can no longer diverge silently if the default ever changes.Not changed
Nothing from your review was skipped or rejected — all four items were verified as correct against the spec/ADR/issue before being applied, and all are now in the diff.
Verification
nox -s lint/format --check— greennox -s typecheck— 0 errorsnox -s unit_tests— 3094 scenarios passed (5 new, all exercising this change), 0 failednox -s coverage_report— 96.9% (unchanged from the prior report; no new uncovered lines introduced)nox -s integration_tests— 365 Robot tests passed (was 364), 0 failednox -s dead_code/nox -s security_scan— cleanReady for another look whenever convenient.
cb48550f16d1f17c0b20