mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
fix(acp): use modes for edit auto-approval
This commit is contained in:
parent
f70e0b85dd
commit
0292398604
5 changed files with 165 additions and 87 deletions
|
|
@ -117,6 +117,7 @@ def make_tool_progress_cb(
|
|||
loop: asyncio.AbstractEventLoop,
|
||||
tool_call_ids: Dict[str, Deque[str]],
|
||||
tool_call_meta: Dict[str, Dict[str, Any]],
|
||||
edit_approval_policy_getter: Callable[[], tuple[str, str | None]] | None = None,
|
||||
) -> Callable:
|
||||
"""Create a ``tool_progress_callback`` for AIAgent.
|
||||
|
||||
|
|
@ -162,7 +163,20 @@ def make_tool_progress_cb(
|
|||
logger.debug("Failed to capture ACP edit snapshot for %s", name, exc_info=True)
|
||||
tool_call_meta[tc_id] = {"args": args, "snapshot": snapshot}
|
||||
|
||||
update = build_tool_start(tc_id, name, args)
|
||||
edit_diff = None
|
||||
if name in {"write_file", "patch"} and edit_approval_policy_getter is not None:
|
||||
try:
|
||||
from acp_adapter.edit_approval import build_edit_proposal, should_auto_approve_edit
|
||||
|
||||
proposal = build_edit_proposal(name, args)
|
||||
if proposal is not None:
|
||||
policy, cwd = edit_approval_policy_getter()
|
||||
if should_auto_approve_edit(proposal, policy, cwd):
|
||||
edit_diff = proposal
|
||||
except Exception:
|
||||
logger.debug("Failed to prepare auto-approved ACP edit diff for %s", name, exc_info=True)
|
||||
|
||||
update = build_tool_start(tc_id, name, args, edit_diff=edit_diff)
|
||||
_send_update(conn, session_id, loop, update)
|
||||
|
||||
return _tool_progress
|
||||
|
|
|
|||
|
|
@ -45,10 +45,10 @@ from acp.schema import (
|
|||
SetSessionModeResponse,
|
||||
ResourceContentBlock,
|
||||
SessionCapabilities,
|
||||
SessionConfigOptionSelect,
|
||||
SessionConfigSelectOption,
|
||||
SessionForkCapabilities,
|
||||
SessionListCapabilities,
|
||||
SessionMode,
|
||||
SessionModeState,
|
||||
SessionModelState,
|
||||
SessionResumeCapabilities,
|
||||
SessionInfo,
|
||||
|
|
@ -499,6 +499,17 @@ class HermesACPAgent(acp.Agent):
|
|||
|
||||
_EDIT_APPROVAL_POLICY_CONFIG_ID = "edit_approval_policy"
|
||||
_EDIT_APPROVAL_POLICY_DEFAULT = "ask"
|
||||
_MODE_DEFAULT = "default"
|
||||
_MODE_ACCEPT_EDITS = "accept_edits"
|
||||
_MODE_DONT_ASK = "dont_ask"
|
||||
_MODE_TO_EDIT_APPROVAL_POLICY = {
|
||||
_MODE_DEFAULT: "ask",
|
||||
_MODE_ACCEPT_EDITS: "workspace_session",
|
||||
_MODE_DONT_ASK: "session",
|
||||
}
|
||||
_EDIT_APPROVAL_POLICY_TO_MODE = {
|
||||
value: key for key, value in _MODE_TO_EDIT_APPROVAL_POLICY.items()
|
||||
}
|
||||
|
||||
def __init__(self, session_manager: SessionManager | None = None):
|
||||
super().__init__()
|
||||
|
|
@ -513,47 +524,43 @@ class HermesACPAgent(acp.Agent):
|
|||
logger.info("ACP client connected")
|
||||
|
||||
|
||||
def _session_config_options(self, state: SessionState) -> list[Any]:
|
||||
values = getattr(state, "config_options", None)
|
||||
if not isinstance(values, dict):
|
||||
values = {}
|
||||
current = str(values.get(self._EDIT_APPROVAL_POLICY_CONFIG_ID) or self._EDIT_APPROVAL_POLICY_DEFAULT)
|
||||
allowed = {"ask", "workspace_session", "session"}
|
||||
if current not in allowed:
|
||||
current = self._EDIT_APPROVAL_POLICY_DEFAULT
|
||||
return [
|
||||
SessionConfigOptionSelect(
|
||||
id=self._EDIT_APPROVAL_POLICY_CONFIG_ID,
|
||||
name="Edit approvals",
|
||||
description="Control ACP edit approvals for this session.",
|
||||
category="permissions",
|
||||
type="select",
|
||||
current_value=current,
|
||||
options=[
|
||||
SessionConfigSelectOption(
|
||||
value="ask",
|
||||
name="Ask before edits",
|
||||
description="Require approval for every file edit.",
|
||||
),
|
||||
SessionConfigSelectOption(
|
||||
value="workspace_session",
|
||||
name="Auto-allow workspace edits",
|
||||
description="Allow workspace and /tmp edits for this session; still asks for sensitive paths.",
|
||||
),
|
||||
SessionConfigSelectOption(
|
||||
value="session",
|
||||
name="Auto-allow all edits this session",
|
||||
description="Allow file edits for this session except sensitive paths.",
|
||||
),
|
||||
],
|
||||
)
|
||||
]
|
||||
def _session_modes(self, state: SessionState) -> SessionModeState:
|
||||
"""Return ACP session modes while preserving Zed's separate model picker.
|
||||
|
||||
Zed renders ``config_options`` in the prominent selector slot where the
|
||||
model picker was visible. Claude/Codex expose policy-like controls as ACP
|
||||
modes, which coexist with the model picker, so Hermes maps edit approval
|
||||
policy onto modes instead of advertising config options.
|
||||
"""
|
||||
|
||||
current = str(getattr(state, "mode", "") or self._MODE_DEFAULT)
|
||||
if current not in self._MODE_TO_EDIT_APPROVAL_POLICY:
|
||||
current = self._MODE_DEFAULT
|
||||
return SessionModeState(
|
||||
current_mode_id=current,
|
||||
available_modes=[
|
||||
SessionMode(
|
||||
id=self._MODE_DEFAULT,
|
||||
name="Default",
|
||||
description="Ask before edits.",
|
||||
),
|
||||
SessionMode(
|
||||
id=self._MODE_ACCEPT_EDITS,
|
||||
name="Accept Edits",
|
||||
description="Auto-allow workspace and /tmp edits; still asks for sensitive paths.",
|
||||
),
|
||||
SessionMode(
|
||||
id=self._MODE_DONT_ASK,
|
||||
name="Don't Ask",
|
||||
description="Auto-allow file edits for this session except sensitive paths.",
|
||||
),
|
||||
],
|
||||
)
|
||||
|
||||
def _edit_approval_policy_for_state(self, state: SessionState) -> tuple[str, str | None]:
|
||||
values = getattr(state, "config_options", None)
|
||||
if not isinstance(values, dict):
|
||||
values = {}
|
||||
return str(values.get(self._EDIT_APPROVAL_POLICY_CONFIG_ID) or self._EDIT_APPROVAL_POLICY_DEFAULT), state.cwd
|
||||
mode = str(getattr(state, "mode", "") or self._MODE_DEFAULT)
|
||||
policy = self._MODE_TO_EDIT_APPROVAL_POLICY.get(mode, self._EDIT_APPROVAL_POLICY_DEFAULT)
|
||||
return policy, state.cwd
|
||||
|
||||
@staticmethod
|
||||
def _encode_model_choice(provider: str | None, model: str | None) -> str:
|
||||
|
|
@ -1040,7 +1047,7 @@ class HermesACPAgent(acp.Agent):
|
|||
return NewSessionResponse(
|
||||
session_id=state.session_id,
|
||||
models=self._build_model_state(state),
|
||||
config_options=self._session_config_options(state),
|
||||
modes=self._session_modes(state),
|
||||
)
|
||||
|
||||
async def load_session(
|
||||
|
|
@ -1084,7 +1091,7 @@ class HermesACPAgent(acp.Agent):
|
|||
self._schedule_usage_update(state)
|
||||
return LoadSessionResponse(
|
||||
models=self._build_model_state(state),
|
||||
config_options=self._session_config_options(state),
|
||||
modes=self._session_modes(state),
|
||||
)
|
||||
|
||||
async def resume_session(
|
||||
|
|
@ -1116,7 +1123,7 @@ class HermesACPAgent(acp.Agent):
|
|||
self._schedule_usage_update(state)
|
||||
return ResumeSessionResponse(
|
||||
models=self._build_model_state(state),
|
||||
config_options=self._session_config_options(state),
|
||||
modes=self._session_modes(state),
|
||||
)
|
||||
|
||||
async def cancel(self, session_id: str, **kwargs: Any) -> None:
|
||||
|
|
@ -1150,7 +1157,7 @@ class HermesACPAgent(acp.Agent):
|
|||
return ForkSessionResponse(
|
||||
session_id=new_id,
|
||||
models=self._build_model_state(state) if state is not None else None,
|
||||
config_options=self._session_config_options(state) if state is not None else None,
|
||||
modes=self._session_modes(state) if state is not None else None,
|
||||
)
|
||||
|
||||
async def list_sessions(
|
||||
|
|
@ -1307,7 +1314,14 @@ class HermesACPAgent(acp.Agent):
|
|||
streamed_message = False
|
||||
|
||||
if conn:
|
||||
tool_progress_cb = make_tool_progress_cb(conn, session_id, loop, tool_call_ids, tool_call_meta)
|
||||
tool_progress_cb = make_tool_progress_cb(
|
||||
conn,
|
||||
session_id,
|
||||
loop,
|
||||
tool_call_ids,
|
||||
tool_call_meta,
|
||||
edit_approval_policy_getter=lambda: self._edit_approval_policy_for_state(state),
|
||||
)
|
||||
reasoning_cb = make_thinking_cb(conn, session_id, loop)
|
||||
step_cb = make_step_cb(conn, session_id, loop, tool_call_ids, tool_call_meta)
|
||||
message_cb = make_message_cb(conn, session_id, loop)
|
||||
|
|
@ -1849,9 +1863,12 @@ class HermesACPAgent(acp.Agent):
|
|||
if state is None:
|
||||
logger.warning("Session %s: mode switch requested for missing session", session_id)
|
||||
return None
|
||||
setattr(state, "mode", mode_id)
|
||||
normalized_mode = str(mode_id or "").strip()
|
||||
if normalized_mode not in self._MODE_TO_EDIT_APPROVAL_POLICY:
|
||||
normalized_mode = self._MODE_DEFAULT
|
||||
setattr(state, "mode", normalized_mode)
|
||||
self.session_manager.save_session(session_id)
|
||||
logger.info("Session %s: mode switched to %s", session_id, mode_id)
|
||||
logger.info("Session %s: mode switched to %s", session_id, normalized_mode)
|
||||
return SetSessionModeResponse()
|
||||
|
||||
async def set_config_option(
|
||||
|
|
@ -1863,11 +1880,15 @@ class HermesACPAgent(acp.Agent):
|
|||
logger.warning("Session %s: config update requested for missing session", session_id)
|
||||
return None
|
||||
|
||||
options = getattr(state, "config_options", None)
|
||||
if not isinstance(options, dict):
|
||||
options = {}
|
||||
options[str(config_id)] = value
|
||||
setattr(state, "config_options", options)
|
||||
if str(config_id) == self._EDIT_APPROVAL_POLICY_CONFIG_ID:
|
||||
mode = self._EDIT_APPROVAL_POLICY_TO_MODE.get(str(value), self._MODE_DEFAULT)
|
||||
setattr(state, "mode", mode)
|
||||
else:
|
||||
options = getattr(state, "config_options", None)
|
||||
if not isinstance(options, dict):
|
||||
options = {}
|
||||
options[str(config_id)] = value
|
||||
setattr(state, "config_options", options)
|
||||
self.session_manager.save_session(session_id)
|
||||
logger.info("Session %s: config option %s updated", session_id, config_id)
|
||||
return SetSessionConfigOptionResponse(config_options=self._session_config_options(state))
|
||||
return SetSessionConfigOptionResponse(config_options=[])
|
||||
|
|
|
|||
|
|
@ -928,6 +928,8 @@ def build_tool_start(
|
|||
tool_call_id: str,
|
||||
tool_name: str,
|
||||
arguments: Dict[str, Any],
|
||||
*,
|
||||
edit_diff: Any = None,
|
||||
) -> ToolCallStart:
|
||||
"""Create a ToolCallStart event for the given hermes tool invocation."""
|
||||
kind = get_tool_kind(tool_name)
|
||||
|
|
@ -935,16 +937,34 @@ def build_tool_start(
|
|||
locations = extract_locations(arguments)
|
||||
|
||||
if tool_name == "patch":
|
||||
mode = arguments.get("mode", "replace")
|
||||
path = arguments.get("path") or "patch input"
|
||||
content = [_text(f"Preparing {mode} edit for {path}. Approval prompt shows the diff.")]
|
||||
if edit_diff is not None:
|
||||
content = [
|
||||
acp.tool_diff_content(
|
||||
path=edit_diff.path,
|
||||
old_text=edit_diff.old_text,
|
||||
new_text=edit_diff.new_text,
|
||||
)
|
||||
]
|
||||
else:
|
||||
mode = arguments.get("mode", "replace")
|
||||
path = arguments.get("path") or "patch input"
|
||||
content = [_text(f"Preparing {mode} edit for {path}. Approval prompt shows the diff.")]
|
||||
return acp.start_tool_call(
|
||||
tool_call_id, title, kind=kind, content=content, locations=locations,
|
||||
)
|
||||
|
||||
if tool_name == "write_file":
|
||||
path = arguments.get("path", "")
|
||||
content = [_text(f"Preparing write to {path}. Approval prompt shows the diff." if path else "Preparing file write. Approval prompt shows the diff.")]
|
||||
if edit_diff is not None:
|
||||
content = [
|
||||
acp.tool_diff_content(
|
||||
path=edit_diff.path,
|
||||
old_text=edit_diff.old_text,
|
||||
new_text=edit_diff.new_text,
|
||||
)
|
||||
]
|
||||
else:
|
||||
path = arguments.get("path", "")
|
||||
content = [_text(f"Preparing write to {path}. Approval prompt shows the diff." if path else "Preparing file write. Approval prompt shows the diff.")]
|
||||
return acp.start_tool_call(
|
||||
tool_call_id, title, kind=kind, content=content, locations=locations,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ from acp.schema import (
|
|||
PromptResponse,
|
||||
ResumeSessionResponse,
|
||||
SessionModelState,
|
||||
SessionModeState,
|
||||
SetSessionConfigOptionResponse,
|
||||
SetSessionModelResponse,
|
||||
SetSessionModeResponse,
|
||||
|
|
@ -52,31 +53,34 @@ def agent(mock_manager):
|
|||
"""HermesACPAgent backed by a mock session manager."""
|
||||
return HermesACPAgent(session_manager=mock_manager)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_new_session_includes_edit_approval_config_option(self, agent):
|
||||
resp = await agent.new_session(cwd="/tmp")
|
||||
|
||||
assert resp.config_options
|
||||
option = resp.config_options[0]
|
||||
assert option.id == "edit_approval_policy"
|
||||
assert option.current_value == "ask"
|
||||
assert {choice.value for choice in option.options} == {
|
||||
"ask",
|
||||
"workspace_session",
|
||||
"session",
|
||||
}
|
||||
@pytest.mark.asyncio
|
||||
async def test_new_session_exposes_edit_approvals_as_modes_not_config_options(agent):
|
||||
resp = await agent.new_session(cwd="/tmp")
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_config_option_persists_edit_approval_policy(self, agent):
|
||||
resp = await agent.new_session(cwd="/tmp")
|
||||
update = await agent.set_config_option(
|
||||
"edit_approval_policy",
|
||||
resp.session_id,
|
||||
"workspace_session",
|
||||
)
|
||||
assert resp.config_options is None
|
||||
assert isinstance(resp.modes, SessionModeState)
|
||||
assert resp.modes.current_mode_id == "default"
|
||||
assert [(mode.id, mode.name) for mode in resp.modes.available_modes] == [
|
||||
("default", "Default"),
|
||||
("accept_edits", "Accept Edits"),
|
||||
("dont_ask", "Don't Ask"),
|
||||
]
|
||||
|
||||
assert isinstance(update, SetSessionConfigOptionResponse)
|
||||
assert update.config_options[0].current_value == "workspace_session"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_set_config_option_persists_edit_approval_policy_without_advertising_config(agent):
|
||||
resp = await agent.new_session(cwd="/tmp")
|
||||
update = await agent.set_config_option(
|
||||
"edit_approval_policy",
|
||||
resp.session_id,
|
||||
"workspace_session",
|
||||
)
|
||||
state = agent.session_manager.get_session(resp.session_id)
|
||||
|
||||
assert isinstance(update, SetSessionConfigOptionResponse)
|
||||
assert update.config_options == []
|
||||
assert getattr(state, "mode", None) == "accept_edits"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -891,11 +895,11 @@ class TestSessionConfiguration:
|
|||
@pytest.mark.asyncio
|
||||
async def test_set_session_mode_returns_response(self, agent):
|
||||
new_resp = await agent.new_session(cwd="/tmp")
|
||||
resp = await agent.set_session_mode(mode_id="chat", session_id=new_resp.session_id)
|
||||
resp = await agent.set_session_mode(mode_id="accept_edits", session_id=new_resp.session_id)
|
||||
state = agent.session_manager.get_session(new_resp.session_id)
|
||||
|
||||
assert isinstance(resp, SetSessionModeResponse)
|
||||
assert getattr(state, "mode", None) == "chat"
|
||||
assert getattr(state, "mode", None) == "accept_edits"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_router_accepts_stable_session_config_methods(self, agent):
|
||||
|
|
@ -904,7 +908,7 @@ class TestSessionConfiguration:
|
|||
|
||||
mode_result = await router(
|
||||
"session/set_mode",
|
||||
{"modeId": "chat", "sessionId": new_resp.session_id},
|
||||
{"modeId": "accept_edits", "sessionId": new_resp.session_id},
|
||||
False,
|
||||
)
|
||||
config_result = await router(
|
||||
|
|
@ -918,8 +922,7 @@ class TestSessionConfiguration:
|
|||
)
|
||||
|
||||
assert mode_result == {}
|
||||
assert config_result["configOptions"]
|
||||
assert config_result["configOptions"][0]["id"] == "edit_approval_policy"
|
||||
assert config_result["configOptions"] == []
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_router_accepts_unstable_model_switch_when_enabled(self, agent):
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
import pytest
|
||||
|
||||
from acp_adapter.edit_approval import EditProposal
|
||||
from acp_adapter.tools import (
|
||||
TOOL_KIND_MAP,
|
||||
build_tool_complete,
|
||||
|
|
@ -174,6 +175,25 @@ class TestBuildToolStart:
|
|||
assert "Approval prompt shows the diff" in item.content.text
|
||||
assert "new_file.py" in item.content.text
|
||||
|
||||
def test_auto_approved_edit_start_shows_diff_content(self):
|
||||
"""Auto-approved edit starts need the diff because no approval card exists."""
|
||||
args = {"path": "/tmp/acp.txt", "old_string": "old", "new_string": "new"}
|
||||
result = build_tool_start(
|
||||
"tc-auto-edit",
|
||||
"patch",
|
||||
args,
|
||||
edit_diff=EditProposal("patch", "/tmp/acp.txt", "old\n", "new\n", args),
|
||||
)
|
||||
|
||||
assert isinstance(result, ToolCallStart)
|
||||
assert result.kind == "edit"
|
||||
assert len(result.content) == 1
|
||||
item = result.content[0]
|
||||
assert isinstance(item, FileEditToolCallContent)
|
||||
assert item.path == "/tmp/acp.txt"
|
||||
assert item.old_text == "old\n"
|
||||
assert item.new_text == "new\n"
|
||||
|
||||
def test_build_tool_start_for_terminal(self):
|
||||
"""terminal should produce text content with the command."""
|
||||
args = {"command": "ls -la /tmp"}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue