fix(gateway): resolve max_iterations from config.yaml directly instead of the env round-trip - #64298
fix(gateway): resolve max_iterations from config.yaml directly instead of the env round-trip#64298Soju06 wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
This PR removes the HERMES_MAX_ITERATIONS env var round-trip and reads agent.max_turns directly from config.yaml.
Looks Good
- _read_config_max_iterations is a clean, focused helper.
- Handles both agent.max_turns and legacy root-level max_turns for backward compat.
- Reduces unnecessary env var surface.
No Issues Found
Reviewed by Hermes Agent
5667442 to
000589f
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the remaining config/env resolution paths. The premise is still present on current main: gateway/run.py:1394-1405 serializes a nested null as "None", and it does not read legacy root-level max_turns.
Problems
gateway/run.py:1611-1614is a separate bootstrap bridge that the PR leaves unchanged. It still writesstr(_agent_cfg["max_turns"]), soagent.max_turns: nullpoisons the process environment with"None". The new startup call at PR linegateway/run.py:6882invokes_resolve_gateway_max_iterations()without reloading.env; it therefore resolves 90 rather than a valid.envfallback and logs the wrong budget. A later normal turn reloads.env, so startup and turn resolution disagree.
Suggested changes
- Route the bootstrap assignment through the new validated helper, including legacy root-level
max_turnshandling. - Add an import/bootstrap regression using
tests/gateway/test_config_env_bridge_authority.py:agent.max_turns: nullplus.env HERMES_MAX_ITERATIONS=120should preserve120.
Automated hermes-sweeper review.
| @@ -6843,7 +6882,7 @@ async def start(self) -> bool: | |||
| # config.yaml → env bridge did the right thing at a glance (instead | |||
There was a problem hiding this comment.
This resolver does not reload .env, but the unchanged bootstrap bridge at gateway/run.py:1611-1614 still writes "None" for agent.max_turns: null. Therefore this startup log resolves 90 even when .env contains a valid fallback such as 120; a later normal turn reloads .env and differs. Route the bootstrap bridge through the validated helper and cover the import/startup path.
000589f to
c7b0ae9
Compare
c7b0ae9 to
6447d4d
Compare
|
Good catch — the import-time bootstrap bridge was indeed left on the raw str() path. Changes: the bootstrap assignment now routes through _bridge_max_turns_from_config(), which validates via _read_config_max_iterations(), so agent.max_turns: null no longer exports the literal "None" and legacy root-level max_turns is bridged at import time as well. With the import-time env no longer poisoned, the startup call to _resolve_gateway_max_iterations() falls back to the .env value when config yields nothing, matching per-turn resolution — startup and turn now log the same budget. Added the requested regression in tests/gateway/test_config_env_bridge_authority.py: agent.max_turns: null with .env HERMES_MAX_ITERATIONS=120 preserves 120 across gateway.run import, plus a legacy root-level max_turns bridge test. Both bridge/reload test files pass (16 tests). |
|
@teknium1 Gentle ping — all points from the review here have been addressed (summary in the comment above), the branch is rebased on current main, and CI is green. Ready for another look whenever convenient. |
6447d4d to
6c7be58
Compare
|
@teknium1 Re-review request: rebased over the #69423 salvage — the new |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Five PRs address the gateway max-iteration precedence bug: #17306 and #18230 add config-first resolution at agent creation, #18764 establishes the broader config-authoritative bridge, #43407 centralizes runtime/startup/API resolution, and #64298 closes the remaining null, legacy-root, and startup-consistency gaps.
Related pull requests
- #17306 [closed]
related— (+58/-3) — superseded: Adds a config-first resolver for the two native gateway creation paths, but current main already preserves agent.max_turns across dotenv reloads; it remains relevant as an earlier implementation of the same root-cause fix. - #18230 [closed]
related— (+82/-6) — duplicate/superseded: Implements substantially the same config-first resolver as #17306 and also applies it to the API server; it remains relevant because it documents API-server coverage, but the underlying behavior is already on main. - #18764 [merged]
related— (+276/-24) — merged foundation: Makes config.yaml authoritative over stale .env values for max_turns and several related settings, removes setup's duplicate HERMES_MAX_ITERATIONS write, and adds bridge regressions; #64298 addresses residual max-turns edge cases left by this reference implementation. - #43407 [closed]
related— (+121/-27) — superseded: Centralizes config-authoritative resolution across startup, native gateway, and API-server paths and reloads dotenv before resolving per-turn budgets; it remains relevant as the direct precursor to the current-main implementation, while #64298 covers additional null and legacy-root cases. - #64298
related— (+160/-17) — merge: Replaces the fragile env round-trip with validated direct config resolution, preserves a valid env fallback for agent.max_turns: null, honors legacy root-level max_turns, and aligns startup logging with per-turn resolution. The contributor keep_open review on #64298 identified an unhandled bootstrap bridge, and the current diff explicitly routes that bridge through the shared validated loader and adds the requested import-time regressions.
Duplicates
#17306 and #18230 are near-duplicates of the config-first agent-construction fix; #43407 overlaps their resolution strategy and the runtime-authority work already established by merged #18764. #64298 is not a pure duplicate because it adds null handling, legacy root-level compatibility, and startup/per-turn consistency.
Suggested consolidation
Merge #64298 as the focused completion of the merged #18764 foundation: its current diff addresses the bootstrap objection from the keep_open review and covers the remaining reproducible edge cases. Keep #17306, #18230, and #43407 closed as superseded/duplicate implementations; no reopening or separate merge is warranted.
Cross-PR triage: Reviewed 5 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 49 kB of PR diffs, 9 kB of issue/PR text, 7 kB of discussion (11 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
6c7be58 to
5d658c2
Compare
…d of the env round-trip The gateway bridged config.yaml agent.max_turns into HERMES_MAX_ITERATIONS and read the env var back. That round-trip let a stale ~/.hermes/.env value win in several cases: agent.max_turns: null poisoned the env with the literal string "None" (collapsing to the hardcoded 90 default), legacy root-level max_turns was invisible to the bridge, and the startup budget log read the raw env var. Add _read_config_max_iterations / _resolve_gateway_max_iterations so config is consulted directly (env-var expansion and managed-scope overlay preserved, fail-open) with the env var as fallback only when config omits the key. _current_max_iterations becomes a thin wrapper, so all per-turn call sites and the API-server adapter pick this up unchanged. The import-time bootstrap bridge routes through the same validated helper, so a null key never exports "None" and startup and per-turn resolution agree.
5d658c2 to
5ae5cb7
Compare
Problem
The gateway resolves the per-turn iteration cap by bridging config.yaml
agent.max_turnsintoHERMES_MAX_ITERATIONSand then reading the env var back. That round-trip has three holes that let a stale~/.hermes/.envvalue (e.g.HERMES_MAX_ITERATIONS=90written by an older setup flow) silently win over the user's configured budget, reintroducing 90/90 iteration-exhaustion on long turns:agent.max_turns: nullin config.yaml makes_bridge_max_turns_from_configwrite the literal string"None"into the env ("max_turns" in agent_cfgis true), and_current_max_iterationsthen swallows theint()failure and returns the hardcoded default 90 — ignoring a perfectly valid.envfallback.max_turns(whichhermes_cli.config._normalize_max_turns_configexplicitly supports and migrates intoagent.max_turns) is invisible to the gateway bridge, so those configs run at whatever stale value.envholds.Change
gateway/run.py:_read_config_max_iterations(home): loads config.yaml (env-var expansion + managed-scope overlay, both fail-open, exactly as the bridge did), returnsint(agent.max_turns)when present, falls back to legacy root-levelmax_turns(agent.max_turnswins when both are set), and returnsNonefor missing/null/malformed values instead of propagating garbage into the environment._bridge_max_turns_from_confignow delegates to it and only writes the env var for a valid integer._resolve_gateway_max_iterations(default=90, *, reload_runtime_env=False): optionally refreshes the runtime env (rotated credentials), then prefers the config value directly — syncing the env var for subprocess consumers — and only consultsHERMES_MAX_ITERATIONSwhen config omits the key (with an int-parse guard)._current_max_iterationsbecomes a thin wrapper over_resolve_gateway_max_iterations(reload_runtime_env=True), so all existing per-turn call sites (native gateway turns and the API-server adapter) pick up config-authoritative resolution without signature changes.scripts/release.py: contributor-attribution entry.Correctness notes
agent.max_turnsis applied inside_read_config_max_iterations, so direct config reads cannot bypass a managed pin._reload_runtime_env_preserving_config_authoritystill skips the global.envreload and only re-bridges config.agent.max_turns(non-integer) now falls back to the env var / default rather than crashing the turn or poisoning the env.gateway.run._current_max_iterationskeep working since the symbol and signature are unchanged.Tests
tests/gateway/test_runtime_env_reload_config_authority.py: new tests for (a) config winning over a stale.envafter a runtime env reload, (b) legacy root-levelmax_turns, (c)agent.max_turns: nullfalling back to the env value; the existing_current_max_iterationsreload test now pins a config-less hermes home so it keeps exercising the env-fallback path.tests/gateway/test_runtime_env_reload_config_authority.py,tests/gateway/test_cached_agent_max_iterations.py(9 passed),tests/gateway/test_api_server.py+tests/scripts(201 passed).tests/gatewayrun: 9101 passed; the remaining failures (telegram/slack/feishu/path-completion modules, unrelated to this change) reproduce identically on a cleanmaincheckout in the same environment.Measured impact
Reproduced the failure mode end-to-end: with
config.yaml agent.max_turns: 500and a stale.env HERMES_MAX_ITERATIONS=90, per-turn resolution previously could run at 90; it now resolves 500 and re-syncs the env var to 500.🤖 Generated with Claude Code