fix(approval): stop the CLI and ACP offering a scope the protected gate discards
The protected agent-instruction gate grants one operation and persists nothing, but only the TUI/desktop and Runs transports were taught that. The prompt_toolkit panel, the input() fallback, and the ACP editor menu still rendered "Allow for session", so a user editing SOUL.md tapped it, got re-prompted on the next write, and read the gate as broken. Thread allow_session through prompt_dangerous_approval so a caller that re-asks every time collapses every surface to once/deny, and cover the producer-to-transport contract end to end.
This commit is contained in:
committed by
brooklyn!
parent
04154a37d3
commit
165d1849e2
@@ -39,13 +39,19 @@ def _permission_option_supports_kind(kind: str) -> bool:
|
||||
|
||||
|
||||
def _build_permission_options(
|
||||
*, allow_permanent: bool, smart_denied: bool = False,
|
||||
*, allow_permanent: bool, allow_session: bool = True,
|
||||
smart_denied: bool = False,
|
||||
) -> list[PermissionOption]:
|
||||
"""Return ACP options that match Hermes approval semantics."""
|
||||
# A gate that re-asks every time (allow_session=False, e.g. protected
|
||||
# agent-instruction writes) collapses to the same two options as a
|
||||
# Smart DENY override — the editor must not offer a scope Hermes
|
||||
# discards, or every subsequent write re-prompts (#81887).
|
||||
once_only = smart_denied or not allow_session
|
||||
options = [PermissionOption(
|
||||
option_id="allow_once", kind="allow_once", name="Allow once",
|
||||
)]
|
||||
if not smart_denied:
|
||||
if not once_only:
|
||||
options.append(PermissionOption(
|
||||
option_id="allow_session",
|
||||
# ACP has no session-scoped kind, so use the closest persistent
|
||||
@@ -53,7 +59,7 @@ def _build_permission_options(
|
||||
kind="allow_always",
|
||||
name="Allow for session",
|
||||
))
|
||||
if allow_permanent and not smart_denied:
|
||||
if allow_permanent and not once_only:
|
||||
options.append(
|
||||
PermissionOption(
|
||||
option_id="allow_always",
|
||||
@@ -62,7 +68,7 @@ def _build_permission_options(
|
||||
),
|
||||
)
|
||||
options.append(PermissionOption(option_id="deny", kind="reject_once", name="Deny"))
|
||||
if not smart_denied and _permission_option_supports_kind("reject_always"):
|
||||
if not once_only and _permission_option_supports_kind("reject_always"):
|
||||
options.append(
|
||||
PermissionOption(
|
||||
option_id="deny_always",
|
||||
@@ -132,6 +138,7 @@ def make_approval_callback(
|
||||
description: str,
|
||||
*,
|
||||
allow_permanent: bool = True,
|
||||
allow_session: bool = True,
|
||||
smart_denied: bool = False,
|
||||
**_: object,
|
||||
) -> str:
|
||||
@@ -139,6 +146,7 @@ def make_approval_callback(
|
||||
|
||||
options = _build_permission_options(
|
||||
allow_permanent=allow_permanent,
|
||||
allow_session=allow_session,
|
||||
smart_denied=smart_denied,
|
||||
)
|
||||
|
||||
|
||||
@@ -15702,14 +15702,16 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
|
||||
|
||||
def _approval_callback(self, command: str, description: str,
|
||||
*, allow_permanent: bool = True,
|
||||
allow_session: bool = True,
|
||||
smart_denied: bool = False) -> str:
|
||||
"""
|
||||
Prompt for dangerous command approval through the prompt_toolkit UI.
|
||||
|
||||
Called from the agent thread. Shows a selection UI similar to clarify
|
||||
with choices: once / session / always / deny. Smart DENY owner
|
||||
overrides show only once / deny. When allow_permanent is False for
|
||||
another reason (for example tirith), only 'always' is hidden.
|
||||
overrides show only once / deny, as do gates that re-ask every time
|
||||
(allow_session=False). When allow_permanent is False for another
|
||||
reason (for example tirith), only 'always' is hidden.
|
||||
Long commands also get a 'view' option so the full command can be
|
||||
expanded before deciding.
|
||||
|
||||
@@ -15729,6 +15731,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
|
||||
"choices": self._approval_choices(
|
||||
command,
|
||||
allow_permanent=allow_permanent,
|
||||
allow_session=allow_session,
|
||||
smart_denied=smart_denied,
|
||||
),
|
||||
"selected": 0,
|
||||
@@ -15780,9 +15783,10 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
|
||||
return "timeout"
|
||||
|
||||
def _approval_choices(self, command: str, *, allow_permanent: bool = True,
|
||||
allow_session: bool = True,
|
||||
smart_denied: bool = False) -> list[str]:
|
||||
"""Return approval choices for a dangerous command prompt."""
|
||||
if smart_denied:
|
||||
if smart_denied or not allow_session:
|
||||
choices = ["once", "deny"]
|
||||
else:
|
||||
choices = ["once", "session", "always", "deny"] if allow_permanent else ["once", "session", "deny"]
|
||||
|
||||
@@ -23,6 +23,7 @@ def _invoke_callback(
|
||||
outcome,
|
||||
*,
|
||||
allow_permanent=True,
|
||||
allow_session=True,
|
||||
smart_denied=False,
|
||||
timeout=60.0,
|
||||
use_prompt_path=False,
|
||||
@@ -46,6 +47,7 @@ def _invoke_callback(
|
||||
"rm -rf /",
|
||||
"dangerous command",
|
||||
allow_permanent=allow_permanent,
|
||||
allow_session=allow_session,
|
||||
smart_denied=smart_denied,
|
||||
approval_callback=cb,
|
||||
)
|
||||
@@ -54,6 +56,7 @@ def _invoke_callback(
|
||||
"rm -rf /",
|
||||
"dangerous command",
|
||||
allow_permanent=allow_permanent,
|
||||
allow_session=allow_session,
|
||||
smart_denied=smart_denied,
|
||||
)
|
||||
|
||||
@@ -96,6 +99,21 @@ class TestApprovalBridge:
|
||||
"deny_always",
|
||||
]
|
||||
|
||||
def test_session_less_gate_offers_only_once_and_deny(self):
|
||||
"""allow_session=False collapses the editor menu to once/deny.
|
||||
|
||||
Hermes discards any scope broader than one operation for the
|
||||
protected agent-instruction gate, so an editor that renders
|
||||
"Allow for session" would re-prompt on the next write (#81887).
|
||||
"""
|
||||
_, kwargs, _, _, _ = _invoke_callback(
|
||||
AllowedOutcome(option_id="allow_once", outcome="selected"),
|
||||
allow_permanent=False,
|
||||
allow_session=False,
|
||||
)
|
||||
|
||||
assert [option.option_id for option in kwargs["options"]] == ["allow_once", "deny"]
|
||||
|
||||
def test_tool_call_ids_are_unique(self):
|
||||
_, first_kwargs, _, _, _ = _invoke_callback(
|
||||
AllowedOutcome(option_id="allow_once", outcome="selected"),
|
||||
|
||||
@@ -93,6 +93,38 @@ class TestCliApprovalUi:
|
||||
thread.join(timeout=2)
|
||||
assert result["value"] == "deny"
|
||||
|
||||
def test_session_less_gate_offers_only_once_and_deny(self):
|
||||
"""A gate that re-asks every time must not advertise a session scope.
|
||||
|
||||
The protected agent-instruction gate (tools/file_tools.py) grants one
|
||||
operation and persists nothing, so offering "session" here makes every
|
||||
later write re-prompt and reads as a broken gate (#81887).
|
||||
"""
|
||||
cli = _make_cli_stub()
|
||||
result = {}
|
||||
|
||||
def _run_callback():
|
||||
result["value"] = cli._approval_callback(
|
||||
"<write to AGENTS.md>",
|
||||
"protected agent-instruction file",
|
||||
allow_permanent=False,
|
||||
allow_session=False,
|
||||
)
|
||||
|
||||
thread = threading.Thread(target=_run_callback, daemon=True)
|
||||
thread.start()
|
||||
|
||||
deadline = time.time() + 2
|
||||
while cli._approval_state is None and time.time() < deadline:
|
||||
time.sleep(0.01)
|
||||
|
||||
assert cli._approval_state is not None
|
||||
assert cli._approval_state["choices"] == ["once", "deny"]
|
||||
|
||||
cli._approval_state["response_queue"].put("once")
|
||||
thread.join(timeout=2)
|
||||
assert result["value"] == "once"
|
||||
|
||||
|
||||
def test_sudo_prompt_restores_existing_draft_after_response(self):
|
||||
cli = _make_cli_stub()
|
||||
|
||||
@@ -463,6 +463,19 @@ class TestProtectedInstructionFiles:
|
||||
self._write(target, "second")
|
||||
assert len(approvals["calls"]) == 2
|
||||
|
||||
def test_cli_prompt_is_told_no_scope_persists(self, tmp_path, approvals):
|
||||
"""The prompt must not advertise a scope this gate discards.
|
||||
|
||||
Since nothing is persisted, a rendered "session"/"always" option
|
||||
re-prompts on the very next write and reads as a broken gate
|
||||
(#81887).
|
||||
"""
|
||||
approvals["answer"] = "once"
|
||||
self._write(tmp_path / "SOUL.md")
|
||||
call = approvals["calls"][0]
|
||||
assert call["allow_session"] is False
|
||||
assert call["allow_permanent"] is False
|
||||
|
||||
def test_regular_file_never_prompts(self, tmp_path, approvals):
|
||||
res = self._write(tmp_path / "notes.md", "hello")
|
||||
assert not res.get("error"), res
|
||||
@@ -644,6 +657,35 @@ class TestProtectedInstructionFiles:
|
||||
finally:
|
||||
A.reset_current_session_key(token)
|
||||
|
||||
def test_gateway_payload_renders_only_once_and_deny(self, tmp_path):
|
||||
"""End-to-end: what this gate emits, a TUI/desktop client can render.
|
||||
|
||||
The transport used to derive its button set from ``allow_permanent``
|
||||
alone, so it re-added a "session" scope the gate refuses to persist —
|
||||
users tapped it and got re-prompted on every write (#81887). Asserting
|
||||
the two layers together is what catches that drift.
|
||||
"""
|
||||
import tools.approval as A
|
||||
from tui_gateway.server import _approval_request_payload
|
||||
|
||||
session_key = "protected-files-payload-session"
|
||||
token = A.set_current_session_key(session_key)
|
||||
rendered = {}
|
||||
try:
|
||||
def notify(approval_data):
|
||||
rendered.update(_approval_request_payload(approval_data))
|
||||
A.resolve_gateway_approval(session_key, "once")
|
||||
|
||||
A.register_gateway_notify(session_key, notify)
|
||||
try:
|
||||
self._write(tmp_path / "SOUL.md", "gateway approved")
|
||||
finally:
|
||||
A.unregister_gateway_notify(session_key)
|
||||
finally:
|
||||
A.reset_current_session_key(token)
|
||||
|
||||
assert rendered["choices"] == ["once", "deny"]
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
pytest.main([__file__, "-v"])
|
||||
|
||||
+22
-7
@@ -2965,20 +2965,27 @@ def prompt_dangerous_approval(command: str, description: str,
|
||||
timeout_seconds: int | None = None,
|
||||
allow_permanent: bool = True,
|
||||
approval_callback=None,
|
||||
*, smart_denied: bool = False) -> str:
|
||||
*, allow_session: bool = True,
|
||||
smart_denied: bool = False) -> str:
|
||||
"""Prompt the user to approve a dangerous command (CLI only).
|
||||
|
||||
Args:
|
||||
allow_permanent: When False, hide the [a]lways option (used when
|
||||
tirith warnings are present, since broad permanent allowlisting
|
||||
is inappropriate for content-level security findings).
|
||||
allow_session: When False, hide the [s]ession option too — the
|
||||
caller grants one operation and re-asks next time (the
|
||||
protected agent-instruction gate in ``tools/file_tools.py``).
|
||||
Offering a scope the caller discards makes every subsequent
|
||||
write re-prompt and reads as a broken gate (#81887).
|
||||
smart_denied: When True, this is an owner override of a Smart DENY.
|
||||
Offer only one-operation approval or denial.
|
||||
approval_callback: Optional callback registered by the CLI for
|
||||
prompt_toolkit integration. Signature:
|
||||
(command, description, *, allow_permanent=True,
|
||||
smart_denied=False) -> str. Legacy callback signatures remain
|
||||
supported when ``smart_denied`` is false.
|
||||
allow_session=True, smart_denied=False) -> str. Legacy callback
|
||||
signatures remain supported while both keywords hold their
|
||||
defaults.
|
||||
|
||||
Returns: 'once', 'session', 'always', 'deny', or 'timeout'.
|
||||
'timeout' means the prompt expired without a user response — the
|
||||
@@ -2999,6 +3006,7 @@ def prompt_dangerous_approval(command: str, description: str,
|
||||
timeout_seconds,
|
||||
allow_permanent,
|
||||
approval_callback,
|
||||
allow_session=allow_session,
|
||||
smart_denied=smart_denied,
|
||||
)
|
||||
|
||||
@@ -3007,7 +3015,8 @@ def _prompt_dangerous_approval_inner(command: str, description: str,
|
||||
timeout_seconds: int,
|
||||
allow_permanent: bool = True,
|
||||
approval_callback=None,
|
||||
*, smart_denied: bool = False) -> str:
|
||||
*, allow_session: bool = True,
|
||||
smart_denied: bool = False) -> str:
|
||||
# Redact secrets before any user-visible rendering. The original
|
||||
# `command` is still what executes after approval; only the displayed
|
||||
# copy is scrubbed. Reuses the same redaction module used for memory
|
||||
@@ -3016,9 +3025,15 @@ def _prompt_dangerous_approval_inner(command: str, description: str,
|
||||
display_command = redact_sensitive_text(command)
|
||||
display_description = redact_sensitive_text(description)
|
||||
|
||||
# Smart DENY and a session-less gate both reduce the menu to
|
||||
# once/deny; the rendered strings are the same either way.
|
||||
once_only = smart_denied or not allow_session
|
||||
|
||||
if approval_callback is not None:
|
||||
try:
|
||||
callback_kwargs = {"allow_permanent": allow_permanent}
|
||||
if not allow_session:
|
||||
callback_kwargs["allow_session"] = False
|
||||
if smart_denied:
|
||||
callback_kwargs["smart_denied"] = True
|
||||
return approval_callback(
|
||||
@@ -3065,7 +3080,7 @@ def _prompt_dangerous_approval_inner(command: str, description: str,
|
||||
print(f" {t('approval.dangerous_header', description=display_description)}")
|
||||
print(f" {display_command}")
|
||||
print()
|
||||
if smart_denied:
|
||||
if once_only:
|
||||
print(t("approval.choose_smart_deny"))
|
||||
elif allow_permanent:
|
||||
print(t("approval.choose_long"))
|
||||
@@ -3078,7 +3093,7 @@ def _prompt_dangerous_approval_inner(command: str, description: str,
|
||||
|
||||
def get_input():
|
||||
try:
|
||||
if smart_denied:
|
||||
if once_only:
|
||||
prompt = t("approval.prompt_smart_deny")
|
||||
else:
|
||||
prompt = t("approval.prompt_long") if allow_permanent else t("approval.prompt_short")
|
||||
@@ -3098,7 +3113,7 @@ def _prompt_dangerous_approval_inner(command: str, description: str,
|
||||
return "timeout"
|
||||
|
||||
choice = result["choice"]
|
||||
if smart_denied:
|
||||
if once_only:
|
||||
choice_map = {
|
||||
**{
|
||||
value: "once"
|
||||
|
||||
@@ -916,6 +916,7 @@ def _request_protected_instruction_approval(
|
||||
choice = _approval.prompt_dangerous_approval(
|
||||
display, description,
|
||||
allow_permanent=False,
|
||||
allow_session=False,
|
||||
approval_callback=callback,
|
||||
)
|
||||
if choice in {"once", "session", "always"}:
|
||||
|
||||
Reference in New Issue
Block a user