Skip to content

fix(acp): honor configured max turns - #70592

Draft
lxy271713 wants to merge 1 commit into
NousResearch:mainfrom
lxy271713:codex/hermes-acp-max-turns
Draft

fix(acp): honor configured max turns#70592
lxy271713 wants to merge 1 commit into
NousResearch:mainfrom
lxy271713:codex/hermes-acp-max-turns

Conversation

@lxy271713

Copy link
Copy Markdown

What does this PR do?

ACP sessions now honor the existing agent.max_turns setting when constructing
AIAgent. Previously, SessionManager._make_agent() omitted max_iterations,
so ACP always used the agent constructor default even when a profile configured
a smaller turn limit.

The setting is forwarded only when it is a positive integer. Missing or invalid
values remain omitted, preserving the existing AIAgent default and avoiding a
second copy of that default in the ACP adapter.

This is intentionally separate from #64045 and the other open ACP toolset PRs;
it does not change ACP tool selection.

#67696 proposes a shared resolver with additional unlimited/string spellings but
does not currently update ACP. This patch targets main's positive-integer
contract; if #67696 lands first, the ACP read can switch to that resolver during
rebase instead of keeping the local validation.

Related Issue

No linked issue. Related but non-overlapping: #64045 configures the ACP tool
surface, and #67696 proposes broader turn-limit parsing. This PR only propagates
the existing turn limit into ACP agent construction.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Security fix
  • Documentation update
  • Tests (adding or improving test coverage)
  • Refactor (no behavior change)
  • New skill (bundled or hub)

Changes Made

  • Read agent.max_turns in acp_adapter/session.py.
  • Pass valid positive integers to AIAgent as max_iterations.
  • Preserve the existing constructor default for missing, boolean, string, zero,
    negative, or otherwise invalid values.
  • Add regression coverage for the configured and fallback paths.

How to Test

  1. Run TMPDIR=<non-sensitive-path> uv run --isolated --frozen --extra acp --extra dev pytest tests/acp tests/acp_adapter -q.
  2. Confirm all 338 tests pass.
  3. Run uv run --isolated --frozen --extra acp --extra dev ruff check acp_adapter/session.py tests/acp/test_session.py.

Checklist

Code

  • I've read the Contributing Guide.
  • My commit message follows Conventional Commits.
  • I searched existing PRs and kept this change separate from the open ACP toolset work.
  • My PR contains only changes related to this fix.
  • I've run pytest tests/ -q across the entire repository.
  • I've added tests for my changes.
  • I've tested on macOS 26.5.1.

Documentation & Housekeeping

  • Documentation update: N/A; this uses the existing agent.max_turns setting.
  • cli-config.yaml.example: N/A; no config key was added or changed.
  • CONTRIBUTING.md / AGENTS.md: N/A; no architecture or workflow changed.
  • Cross-platform impact considered: the change is platform-neutral config propagation.
  • Tool descriptions/schemas: N/A; no tool behavior changed.

Screenshots / Logs

Not applicable. The regression is covered by constructor-argument assertions and
the complete ACP/ACP-adapter test suites.

@alt-glitch alt-glitch added type/bug Something isn't working P4 Best-effort: we will get to it when we get to it (no commitment) comp/acp Agent Communication Protocol adapter area/config Config system, migrations, profiles sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 24, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the ACP configuration propagation fix. The source change addresses a current-main omission: acp_adapter/session.py:623-651 constructs AIAgent without max_iterations, despite agent.max_turns defaulting to 500 in hermes_cli/config_defaults.py:32.

Problems

  • The test hunk is anchored at tests/acp/test_session.py:439 from the PR base, but current main's file ends at line 329 after 39975613b13b418e0eceda178434d7be90ad4f91; GitHub reports the PR as conflicting.

Suggested changes

  • Recreate the parameterized regression coverage in the current test-file layout while preserving the positive-integer and invalid-value cases.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/acp Agent Communication Protocol adapter P4 Best-effort: we will get to it when we get to it (no commitment) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants