cleveragents/cleveractors-core

Fork 0

fix(agents): honor tool_max_rounds precedence and document it #147

Merged

CoreRasurae merged 1 commit from bugfix/m1-tool-max-rounds-precedence into master

2026-08-24 15:40:09 +00:00

CoreRasurae commented

2026-08-24 13:13:14 +00:00

Member

Summary

LLMAgent._execute_tool_loop() resolved its round limit via config.get("tool_max_rounds") or os.environ.get("TOOL_MAX_ROUNDS", "20"), so an explicitly configured falsy tool_max_rounds: 0 was discarded instead of being clamped to 1, contradicting the precedence CHANGELOG.md already documented (issue #59 entry).
tool_max_rounds is now validated and stored at LLMAgent.__init__() time, consistent with sibling config fields, distinguishing "absent from config" from "present but falsy". Precedence: explicit config value (including 0) > TOOL_MAX_ROUNDS env var (only consulted when config is absent) > default of 20.
ADR-2031 revised in place with new D-8, formally documenting tool_max_rounds in Actor Configuration Standard §4.4 (Version 1.7.0).
docs/guides/reasoning-aware-llm-agents.md updated with concrete precedence examples.
Removed @tdd_expected_fail from the @tdd_issue_144 regression scenario now that the fix is in place; added two new scenarios covering explicit-config-over-env-var precedence and env-var-fallback-when-absent; adjusted two pre-existing scenarios whose bad-value source moved from the loop to agent construction.

Test plan

nox -s unit_tests — 154 features / 3100 scenarios / 14350 steps passed
nox -s integration_tests — 365 Robot tests passed
nox -s e2e_tests — passed
nox -s coverage_report — 96.9% (threshold 96.5%)
nox -s security_scan, nox -s dead_code, nox -s complexity — all passed
nox -s docs, nox -s build — passed
nox -s benchmark / benchmark_regression — not evaluated (informational-only ASV sessions, per maintainer instruction)

Closes #144

## Summary - `LLMAgent._execute_tool_loop()` resolved its round limit via `config.get("tool_max_rounds") or os.environ.get("TOOL_MAX_ROUNDS", "20")`, so an explicitly configured falsy `tool_max_rounds: 0` was discarded instead of being clamped to 1, contradicting the precedence CHANGELOG.md already documented (issue #59 entry). - `tool_max_rounds` is now validated and stored at `LLMAgent.__init__()` time, consistent with sibling config fields, distinguishing "absent from config" from "present but falsy". Precedence: explicit config value (including 0) > `TOOL_MAX_ROUNDS` env var (only consulted when config is absent) > default of 20. - ADR-2031 revised in place with new **D-8**, formally documenting `tool_max_rounds` in Actor Configuration Standard §4.4 (Version 1.7.0). - `docs/guides/reasoning-aware-llm-agents.md` updated with concrete precedence examples. - Removed `@tdd_expected_fail` from the `@tdd_issue_144` regression scenario now that the fix is in place; added two new scenarios covering explicit-config-over-env-var precedence and env-var-fallback-when-absent; adjusted two pre-existing scenarios whose bad-value source moved from the loop to agent construction. ## Test plan - [x] `nox -s unit_tests` — 154 features / 3100 scenarios / 14350 steps passed - [x] `nox -s integration_tests` — 365 Robot tests passed - [x] `nox -s e2e_tests` — passed - [x] `nox -s coverage_report` — 96.9% (threshold 96.5%) - [x] `nox -s security_scan`, `nox -s dead_code`, `nox -s complexity` — all passed - [x] `nox -s docs`, `nox -s build` — passed - [ ] `nox -s benchmark` / `benchmark_regression` — not evaluated (informational-only ASV sessions, per maintainer instruction) Closes #144

CoreRasurae added this to the v2.1.0 milestone

2026-08-24 13:13:14 +00:00

CoreRasurae added 1 commit

2026-08-24 13:13:15 +00:00

fix(agents): honor tool_max_rounds precedence and document it

CI / lint (pull_request) Successful in 1m12s

Details

CI / typecheck (pull_request) Successful in 1m21s

Details

CI / security (pull_request) Successful in 2m11s

Details

CI / quality (pull_request) Successful in 1m18s

Details

CI / build (pull_request) Successful in 1m44s

Details

CI / integration_tests (pull_request) Successful in 2m34s

Details

CI / unit_tests (pull_request) Successful in 4m51s

Details

CI / coverage (pull_request) Successful in 5m42s

Details

CI / status-check (pull_request) Successful in 1s

Details

CI / benchmark (pull_request) Failing after 26m42s

Details

4b9d0c5ad5

LLMAgent._execute_tool_loop() resolved its round limit via
config.get("tool_max_rounds") or os.environ.get("TOOL_MAX_ROUNDS", "20"),
so an agent explicitly configured with tool_max_rounds: 0 had that value
discarded by Python's `or` truthiness check and silently ran with the
TOOL_MAX_ROUNDS/default limit instead, contradicting the config/env/clamp
precedence CHANGELOG.md already documented publicly (issue #59 entry).

tool_max_rounds is now validated and stored at LLMAgent.__init__() time,
consistent with sibling config fields (token_budget_percent,
pruning_threshold, max_retries), distinguishing "absent from config" from
"present but falsy". Precedence: an explicit per-agent value (including 0)
always wins, clamped to a minimum of 1; TOOL_MAX_ROUNDS is consulted only
when tool_max_rounds is absent from config; the default of 20 applies only
when both are absent. A non-integer TOOL_MAX_ROUNDS env var still raises
ConfigurationError from within _execute_tool_loop(), the one remaining
path that parses the value at call time rather than construction time.

The governing ADR-2031 is revised in place with a new D-8 formally
introducing tool_max_rounds as an Actor Configuration Standard §4.4
LLM-agent configuration field (Version 1.7.0), since its precedence
contract was already a public commitment via CHANGELOG.md. The
reasoning-aware-llm-agents guide is updated with concrete examples of
the corrected precedence.

Two pre-existing scenarios exercising the old parse-inside-the-loop
behavior are adjusted for the new construction-time validation: the
config-supplied bad-value case now fails at LLMAgent construction
(features/llm_agent_tool_calling.feature), and the "_ToolLoopError raised
before any ainvoke" case is re-targeted at a malformed TOOL_MAX_ROUNDS
env var, the only remaining way to trigger that path
(features/llm_agent_tool_loop.feature). Two new scenarios cover explicit
config precedence over the env var and env-var fallback when config is
absent.

ISSUES CLOSED: #144

CoreRasurae added the

Type

Bug

label

2026-08-24 13:13:47 +00:00

CoreRasurae added a new dependency

2026-08-24 13:14:23 +00:00

#144 tool_max_rounds config silently ignored when falsy, and undocumented in the LLM agent specification

hurui200320 approved these changes

2026-08-24 13:39:49 +00:00

hurui200320 left a comment

PR Review: !147 (Ticket #144)

Verdict: Approve

The implementation correctly fixes the tool_max_rounds: 0 precedence bug, moves validation to LLMAgent.__init__() consistently with sibling fields, and updates the specification (ADR-2031 D-8, docs/index.md v1.7.0), user guide, and BDD scenarios. Code logic and test structure are sound. Only minor issues remain.

Critical Issues

None.

Major Issues

None.

Minor Issues

Coverage is reported at 96.9%, below the documented 97% merge gate. noxfile.py currently enforces 96.5%, so the PR passes CI, but CONTRIBUTING.md and issue #144's acceptance criteria both require ≥97%. This is a pre-existing threshold/documentation inconsistency; consider adding targeted tests or aligning the threshold docs before it becomes a blocking gate.
Inconsistent float coercion between config and env var paths. src/cleveractors/agents/llm.py:403 uses int(_raw_max_rounds) for config values (silently truncates floats like 2.7 to 2), while the env-var path at line 1081 uses int(str(_raw_max_rounds)) (rejects non-integer strings/floats). Since the field is specified as integer, consider making the two paths consistent.
Missing test coverage for env-var clamping edge cases. No scenario verifies that TOOL_MAX_ROUNDS=0 or a negative env var is clamped to 1, or that an invalid env var is still rejected. These are covered by code inspection but not by dedicated BDD scenarios.

Nits

_raw_max_rounds is reused to hold the clamped integer value (src/cleveractors/agents/llm.py:400-408), which shadows the original raw input. Using a separate variable (e.g., _clamped_max_rounds) would improve readability.
Error message for env-var parse failure still refers to tool_max_rounds (src/cleveractors/agents/llm.py:1084) rather than TOOL_MAX_ROUNDS, which may confuse users debugging an environment-only issue.

Summary

Solid, focused bugfix that closes #144. The precedence contract is now correctly implemented and normatively documented. The remaining items are minor and should not block merge.

## PR Review: !147 (Ticket #144) ### Verdict: Approve The implementation correctly fixes the `tool_max_rounds: 0` precedence bug, moves validation to `LLMAgent.__init__()` consistently with sibling fields, and updates the specification (ADR-2031 D-8, docs/index.md v1.7.0), user guide, and BDD scenarios. Code logic and test structure are sound. Only minor issues remain. ### Critical Issues None. ### Major Issues None. ### Minor Issues - **Coverage is reported at 96.9%, below the documented 97% merge gate.** `noxfile.py` currently enforces 96.5%, so the PR passes CI, but `CONTRIBUTING.md` and issue #144's acceptance criteria both require ≥97%. This is a pre-existing threshold/documentation inconsistency; consider adding targeted tests or aligning the threshold docs before it becomes a blocking gate. - **Inconsistent float coercion between config and env var paths.** `src/cleveractors/agents/llm.py:403` uses `int(_raw_max_rounds)` for config values (silently truncates floats like `2.7` to `2`), while the env-var path at line 1081 uses `int(str(_raw_max_rounds))` (rejects non-integer strings/floats). Since the field is specified as integer, consider making the two paths consistent. - **Missing test coverage for env-var clamping edge cases.** No scenario verifies that `TOOL_MAX_ROUNDS=0` or a negative env var is clamped to 1, or that an invalid env var is still rejected. These are covered by code inspection but not by dedicated BDD scenarios. ### Nits - **`_raw_max_rounds` is reused to hold the clamped integer value** (`src/cleveractors/agents/llm.py:400-408`), which shadows the original raw input. Using a separate variable (e.g., `_clamped_max_rounds`) would improve readability. - **Error message for env-var parse failure still refers to `tool_max_rounds`** (`src/cleveractors/agents/llm.py:1084`) rather than `TOOL_MAX_ROUNDS`, which may confuse users debugging an environment-only issue. ### Summary Solid, focused bugfix that closes #144. The precedence contract is now correctly implemented and normatively documented. The remaining items are minor and should not block merge.

CoreRasurae commented

2026-08-24 14:25:03 +00:00

Author

Member

Minor issues 1, is not relevant, we have a noxfile threshold at 96.5% while having a 97% in the specification, but the 97% is the rounded value, which is also obtainable from a 96.5% real coverage.

CoreRasurae force-pushed bugfix/m1-tool-max-rounds-precedence from 4b9d0c5ad5

CI / lint (pull_request) Successful in 1m12s

Details

CI / typecheck (pull_request) Successful in 1m21s

Details

CI / security (pull_request) Successful in 2m11s

Details

CI / quality (pull_request) Successful in 1m18s

Details

CI / build (pull_request) Successful in 1m44s

Details

CI / integration_tests (pull_request) Successful in 2m34s

Details

CI / unit_tests (pull_request) Successful in 4m51s

Details

CI / coverage (pull_request) Successful in 5m42s

Details

CI / status-check (pull_request) Successful in 1s

Details

CI / benchmark (pull_request) Failing after 26m42s

Details

to bf09e4facc

CI / lint (pull_request) Successful in 48s

Details

CI / security (pull_request) Has started running

Details

CI / typecheck (pull_request) Successful in 1m22s

Details

CI / quality (pull_request) Has started running

Details

CI / unit_tests (pull_request) Has been cancelled

Details

CI / integration_tests (pull_request) Has been cancelled

Details

CI / coverage (pull_request) Has been cancelled

Details

CI / build (pull_request) Has been cancelled

Details

CI / benchmark (pull_request) Has been cancelled

Details

CI / status-check (pull_request) Has been cancelled

Details

2026-08-24 14:46:42 +00:00

Compare

CoreRasurae commented

2026-08-24 14:47:12 +00:00

Author

Member

@hurui200320 thanks for the review. Response to each item below (verified against docs/index.md §4.4 / ADR-2031 D-8 before acting on any of them). Pushed as an amended commit (bf09e4f, was 4b9d0c5) since the branch hadn't been merged yet.

Minor Issues

1. Coverage 96.9% vs the documented 97% gate — not changed.
Already addressed in my earlier comment on this PR: noxfile.py's actual COVERAGE_THRESHOLD constant is 96.5, and 96.9% real coverage rounds to the "97%" figure quoted in CONTRIBUTING.md/issue #144's acceptance criteria. This is a pre-existing display/rounding inconsistency between the docs and the constant, not something introduced by this PR, so I've left it as-is rather than adding tests just to move a number that already satisfies the real gate.

2. Inconsistent float coercion between config and env-var paths — fixed.
LLMAgent.__init__ now validates tool_max_rounds with isinstance(_raw_max_rounds, int) instead of int(_raw_max_rounds), matching the pattern already used by the sibling field pruning_threshold. A float such as 2.7 is now rejected with ConfigurationError on the config path exactly as it already was on the TOOL_MAX_ROUNDS env-var path (where int(str(x)) rejects a float string), instead of being silently truncated to 2. docs/index.md §4.4 documents the field's type as integer, so this brings the implementation in line with the existing spec rather than requiring any spec change.

3. Missing test coverage for env-var clamping edge cases — fixed, and extended to the config path too.
Added to features/llm_agent_tool_loop.feature:

TOOL_MAX_ROUNDS=0 clamped to 1 round
TOOL_MAX_ROUNDS=-5 clamped to 1 round

For symmetry (both the config field and the env var implement the same clamp-to-1 contract per §4.4/D-8), I also added:

an explicit tool_max_rounds: -3 config value clamped to 1 round (llm_agent_tool_loop.feature)
an explicit tool_max_rounds: 2.7 config value rejected with ConfigurationError (llm_agent_tool_calling.feature), covering the isinstance fix from item 2 above

Nits

1. _raw_max_rounds reused to hold the clamped value — fixed.
The clamped result is now stored in its own _clamped_max_rounds variable; _raw_max_rounds no longer changes meaning partway through the block.

2. Env-var parse-failure error message naming tool_max_rounds — fixed.
The ConfigurationError raised from the TOOL_MAX_ROUNDS-parsing branch in _execute_tool_loop() now says "TOOL_MAX_ROUNDS must be an integer, got ...", naming the actual source of the bad value.

Verification

nox -s unit_tests (154 features / 3104 scenarios / 0 failures), lint, typecheck, security_scan, and dead_code all green. Ran a scoped nox -s coverage_report -- features/llm_agent_tool_loop.feature features/llm_agent_tool_calling.feature and confirmed every changed line (the __init__ validation block and the TOOL_MAX_ROUNDS resolution block in _execute_tool_loop) is exercised — none appear in slipcover's missing-lines list.

No ADR or docs/index.md changes were needed for this round — the field's documented type (integer) didn't change, only the implementation's fidelity to it.

@hurui200320 thanks for the review. Response to each item below (verified against `docs/index.md` §4.4 / ADR-2031 D-8 before acting on any of them). Pushed as an amended commit (`bf09e4f`, was `4b9d0c5`) since the branch hadn't been merged yet. ### Minor Issues **1. Coverage 96.9% vs the documented 97% gate — not changed.** Already addressed in my earlier comment on this PR: `noxfile.py`'s actual `COVERAGE_THRESHOLD` constant is `96.5`, and 96.9% real coverage rounds to the "97%" figure quoted in `CONTRIBUTING.md`/issue #144's acceptance criteria. This is a pre-existing display/rounding inconsistency between the docs and the constant, not something introduced by this PR, so I've left it as-is rather than adding tests just to move a number that already satisfies the real gate. **2. Inconsistent float coercion between config and env-var paths — fixed.** `LLMAgent.__init__` now validates `tool_max_rounds` with `isinstance(_raw_max_rounds, int)` instead of `int(_raw_max_rounds)`, matching the pattern already used by the sibling field `pruning_threshold`. A float such as `2.7` is now rejected with `ConfigurationError` on the config path exactly as it already was on the `TOOL_MAX_ROUNDS` env-var path (where `int(str(x))` rejects a float string), instead of being silently truncated to `2`. `docs/index.md` §4.4 documents the field's type as `integer`, so this brings the implementation in line with the existing spec rather than requiring any spec change. **3. Missing test coverage for env-var clamping edge cases — fixed, and extended to the config path too.** Added to `features/llm_agent_tool_loop.feature`: - `TOOL_MAX_ROUNDS=0` clamped to 1 round - `TOOL_MAX_ROUNDS=-5` clamped to 1 round For symmetry (both the config field and the env var implement the same clamp-to-1 contract per §4.4/D-8), I also added: - an explicit `tool_max_rounds: -3` config value clamped to 1 round (`llm_agent_tool_loop.feature`) - an explicit `tool_max_rounds: 2.7` config value rejected with `ConfigurationError` (`llm_agent_tool_calling.feature`), covering the isinstance fix from item 2 above ### Nits **1. `_raw_max_rounds` reused to hold the clamped value — fixed.** The clamped result is now stored in its own `_clamped_max_rounds` variable; `_raw_max_rounds` no longer changes meaning partway through the block. **2. Env-var parse-failure error message naming `tool_max_rounds` — fixed.** The `ConfigurationError` raised from the `TOOL_MAX_ROUNDS`-parsing branch in `_execute_tool_loop()` now says `"TOOL_MAX_ROUNDS must be an integer, got ..."`, naming the actual source of the bad value. ### Verification `nox -s unit_tests` (154 features / 3104 scenarios / 0 failures), `lint`, `typecheck`, `security_scan`, and `dead_code` all green. Ran a scoped `nox -s coverage_report -- features/llm_agent_tool_loop.feature features/llm_agent_tool_calling.feature` and confirmed every changed line (the `__init__` validation block and the `TOOL_MAX_ROUNDS` resolution block in `_execute_tool_loop`) is exercised — none appear in slipcover's missing-lines list. No ADR or `docs/index.md` changes were needed for this round — the field's documented type (`integer`) didn't change, only the implementation's fidelity to it.

CoreRasurae force-pushed bugfix/m1-tool-max-rounds-precedence from bf09e4facc

CI / lint (pull_request) Successful in 48s

Details

CI / security (pull_request) Has started running

Details

CI / typecheck (pull_request) Successful in 1m22s

Details

CI / quality (pull_request) Has started running

Details

CI / unit_tests (pull_request) Has been cancelled

Details

CI / integration_tests (pull_request) Has been cancelled