fix(oneshot): honor --max-turns / agent.max_turns in hermes -z - #61152
fix(oneshot): honor --max-turns / agent.max_turns in hermes -z#61152OmarB97 wants to merge 2 commits into
Conversation
Related: part of the "oneshot ( |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the missing max_iterations propagation: current main does construct oneshot AIAgent without that argument at hermes_cli/oneshot.py:387-411, so the configuration half of the bug is real.
Problems
- Blocking:
--max-turnsis only registered on thechatsubparser (hermes_cli/_parser.py:353-359); the top-level parser that owns-z/--oneshot(hermes_cli/_parser.py:102-114) does not accept it. The new forwarding athermes_cli/main.py:12502/:14600therefore never receives a CLI value forhermes -z ... --max-turns N. Add the top-level registration and a realparse_args(["-z", "hi", "--max-turns", "5"])regression test. The cross-referenced #61566 contains that parser wiring. - The new resolver is passed
load_config()output (hermes_cli/oneshot.py:358), which is already default-merged and normalizes rootmax_turnsintoagent.max_turns(hermes_cli/config.py:6937-6951). The raw-dict legacy/environment tests do not cover that production flow.
Suggested changes
- Retain the
AIAgent(max_iterations=...)propagation, add the parser wiring, and align the resolver tests/documentation with the actual loaded-config data flow.
Automated hermes-sweeper review.
| @@ -12499,6 +12499,7 @@ def _try_termux_fast_cli_launch() -> bool: | |||
| provider=getattr(args, "provider", None), | |||
| toolsets=getattr(args, "toolsets", None), | |||
| usage_file=getattr(args, "usage_file", None), | |||
| max_turns=getattr(args, "max_turns", None), | |||
There was a problem hiding this comment.
args.max_turns cannot be populated for hermes -z: current --max-turns is registered only on the chat subparser (hermes_cli/_parser.py:353-359), not the top-level parser that owns -z. Please add top-level parser support and a parser-level -z ... --max-turns regression test.
|
Addressed the review feedback in
Verification: 75 targeted tests passed; Ruff and |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Three PRs address the max-turns path: #4314 exposes the existing setting for chat, while #61152 and #61566 extend it to oneshot; only #61152 also resolves the configured iteration budget instead of handling the explicit flag alone.
Related pull requests
- #4314 [merged]
related— (+8/-0) — merged reference implementation: #4314 remains relevant because it added the chat-subcommand --max-turns parser wiring and forwarded the value into the interactive CLI, but it did not touch the hermes -z oneshot path. - #61152
related— (+230/-1) — canonical fix: #61152 registers --max-turns at the top level, forwards it through both oneshot dispatch sites, resolves the CLI value over normalized agent.max_turns configuration, and passes the result to AIAgent(max_iterations=...), with parser, propagation, and config-derived regression coverage. The keep_open review on #61152 identified missing top-level parser wiring and production-flow coverage; the current diff addresses both points. - #61566
duplicate— (+74/-1) — duplicate and incomplete relative to #61152: it wires the explicit flag through the same parser and oneshot call sites, but leaves unflagged runs at AIAgent's default and therefore still bypasses agent.max_turns. Despite the keep_open review on #61566, its own review documents this gap and points to #61152 as the broader implementation.
Duplicates
#61566 substantially duplicates the explicit --max-turns parser and oneshot propagation already present in #61152; #61152 is the functional superset. #4314 overlaps only in the chat-side flag exposure and is a merged reference, not a duplicate of the oneshot fix.
Suggested consolidation
Merge #61152 as the canonical oneshot fix because its current diff addresses the blocking keep_open review by adding top-level parsing and real config-to-AIAgent coverage, while preserving the chat behavior introduced by merged #4314. Close #61566 as a duplicate of #61152: its explicit-flag-only implementation does not fix the configured-budget half of the root cause.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup61152 ["PRs duplicating each other"]
P61152["PR #61152 (open)"]
P61566["PR #61566 (open)"]
end
class P61152 open
class P61566 open
class P61152 target
click P61152 "https://github.com/NousResearch/hermes-agent/pull/61152"
click P61566 "https://github.com/NousResearch/hermes-agent/pull/61566"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).
Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 21 kB of PR diffs, 5 kB of issue/PR text, 5 kB of discussion (6 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
hermes -z built its AIAgent without a max_iterations argument, so oneshot runs silently used the AIAgent built-in default and ignored the profile's configured agent.max_turns, the --max-turns flag, and HERMES_MAX_ITERATIONS. Every other agent path (interactive CLI, gateway) already resolves the per-turn budget from that precedence chain; oneshot was the outlier. The practical damage: long-running 'hermes -z' workers (e.g. automated dispatch harnesses) were capped below their configured budget and cut off mid-task, with no per-invocation lever to raise the ceiling even though the --max-turns flag already existed and was wired for interactive/--tui runs. - run_oneshot() / _run_agent() take a max_turns argument. - New _resolve_max_iterations() mirrors the interactive precedence: --max-turns > agent.max_turns > legacy root max_turns > HERMES_MAX_ITERATIONS > 90. - The resolved value is passed to AIAgent(max_iterations=...). - Both hermes_cli.main oneshot call sites forward the existing --max-turns. Tests: resolver precedence, run_oneshot threading, and an end-to-end check that the resolved cap reaches the AIAgent constructor through the real _run_agent path. Co-Authored-By: Claude Code <noreply@anthropic.com>
49f702c to
5384606
Compare
Problem
hermes -z(oneshot) built itsAIAgentwithout amax_iterationsargument, so it silently fell back to theAIAgentbuilt-in default (90) and ignored:agent.max_turnsinconfig.yaml,--max-turnsflag (which already existed and worked for interactive /--tui),HERMES_MAX_ITERATIONS.Every other agent path — interactive
HermesCLIand the gateway — resolves the per-turn tool-iteration budget from that precedence chain. Oneshot was the sole outlier, so a profile that setsagent.max_turns: 120still ran-zat 90.The practical damage: long-running
hermes -zworkers (automated dispatch harnesses that drive-zin a subprocess) were capped below their configured budget and cut off mid-task, with no per-invocation lever to raise the ceiling — even though--max-turnswas already a documented flag.Fix
run_oneshot()and_run_agent()accept amax_turnsargument._resolve_max_iterations(cfg, cli_max_turns)mirrors the interactive precedence exactly:--max-turns>agent.max_turns> legacy rootmax_turns>HERMES_MAX_ITERATIONS> 90.AIAgent(max_iterations=...).hermes_cli.mainoneshot call sites forward the existing--max-turnsflag (andusage_file, already present).No new flag, no signature break for existing callers (
max_turnsdefaults toNone→ same behavior unless set, except that a configuredagent.max_turnsis now honored — which is the intended fix).Behavior change to note
A profile with
agent.max_turnsset will now have that value applied to-zruns (previously ignored, pinned at 90). This makes oneshot consistent with interactive and gateway sessions. Runs with no config/flag/env still default to 90.Tests
tests/hermes_cli/test_oneshot_max_turns.py:_resolve_max_iterationsprecedence (flag > config > legacy > env > default; non-numeric/zero fall-through).run_oneshotthreadsmax_turnsinto_run_agent._run_agent(onlyAIAgent+ the network-touching provider resolver stubbed): asserts the resolved cap reachesAIAgent(max_iterations=...)— 250 via flag, 120 via config.Also updated the two
test_tui_resume_flow.pycall-signature assertions to include the new kwarg.