Skip to content

feat(acp): advertise thought-level session config - #48444

Open
jakepresent wants to merge 1 commit into
NousResearch:mainfrom
jakepresent:feat/acp-thought-level-config
Open

feat(acp): advertise thought-level session config#48444
jakepresent wants to merge 1 commit into
NousResearch:mainfrom
jakepresent:feat/acp-thought-level-config

Conversation

@jakepresent

@jakepresent jakepresent commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • advertise ACP's standardized thought_level session config option from hermes acp
  • map session/set_config_option for thought_level into AIAgent.reasoning_config
  • return updated configOptions so ACP clients can refresh the selected value

Why

ACP v1 defines configOptions with category: thought_level for reasoning-level selectors. Hermes already has internal reasoning_config, but the ACP adapter did not advertise or apply that surface, so clients could not provide a real reasoning-effort dropdown.

Related work

Tests

  • python -m pytest tests/acp_adapter/test_acp_commands.py::test_acp_new_session_advertises_thought_level_config_option tests/acp_adapter/test_acp_commands.py::test_acp_set_thought_level_updates_agent_reasoning_config tests/acp_adapter/test_acp_commands.py::test_acp_set_thought_level_none_disables_reasoning -q -o 'addopts='
  • python -m pytest tests/acp_adapter -q -o 'addopts='

Live probe

Ran the ACP adapter from this worktree over stdio and verified session/new returns configOptions[thought_level] with current medium, and session/set_config_option to high returns the updated current value.

Expose ACP's standardized thought_level configOptions selector from hermes acp
and map session/set_config_option updates to AIAgent.reasoning_config.

This lets ACP clients render a real reasoning-effort dropdown generically while
preserving the existing session modes for edit-approval policy. Tests cover the
advertised selector and both high/none reasoning updates.
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/acp Agent Communication Protocol adapter labels Jun 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: #30290 (fix(acp): honor agent reasoning config) and #25364 (reasoning-level selector request). Different mechanism — #30290 honors the existing internal reasoning_config, while this PR advertises the standardized ACP thought_level config option to clients and maps set_config_option back into reasoning_config. Complementary, not duplicate.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

✅ Looks Good

  • Clean feature implementation: Advertises ACP's thought_level session config option and maps it to internal reasoning_config
  • Well-scoped: Only 2 files (server.py + test), focused on a single concern
  • Good test coverage: 3 new tests covering:
    • new_session returns the config option with correct default (medium)
    • set_config_option correctly maps high to reasoning_config = {"enabled": True, "effort": "high"}
    • none correctly sets {"enabled": False}
  • Proper return value: set_config_option returns updated config_options so ACP clients can refresh — not ignored
  • Follows existing patterns: _thought_level_current_value() mirrors the existing _edit_approval_policy_for_state() pattern
  • No debug artifacts or secrets

Minor Note

  • The parse_reasoning_effort import inside the elif block is fine (lazy import to avoid circular dependency)

Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused ACP contribution. Current main confirms the feature gap: acp_adapter/server.py:1124-1131 exposes only models/modes, while set_config_option() returns no option state at acp_adapter/server.py:2045-2065.

Problems

  • acp_adapter/server.py:537-540 documents that Zed renders config_options in the model-picker slot. Commit 029239860 intentionally moved edit approval from config options to modes to retain that picker. The added config_options responses would restore the same conflict.
  • The new live-agent assignment is not durable. _persist() stores session metadata without reasoning_config (acp_adapter/session.py:424-449), _restore() recreates the agent without it (acp_adapter/session.py:544-552), and model switching replaces the agent (acp_adapter/server.py:2010-2017).
  • Forks are also incomplete: fork_session() returns models/modes only (acp_adapter/server.py:1243-1247) and creates a fresh agent without carrying reasoning state (acp_adapter/session.py:252-264).

Suggested changes

  • Preserve the Zed model-picker behavior while choosing the ACP representation for thought level.
  • Make the setting durable across restore, model switch, and fork, with tests for each lifecycle path.

This is an automated hermes-sweeper review.

Comment thread acp_adapter/server.py
session_id=state.session_id,
models=self._build_model_state(state),
modes=self._session_modes(state),
config_options=self._session_config_options(state),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main deliberately avoids config_options because Zed renders them in the model-picker slot (acp_adapter/server.py:537-540; commit 029239860). Adding this response field would reintroduce that picker regression; please use a representation that preserves model selection or gate it by verified client behavior.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have 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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants