diff --git a/acp_adapter/permissions.py b/acp_adapter/permissions.py index b10b2a169e..72ff7dbfac 100644 --- a/acp_adapter/permissions.py +++ b/acp_adapter/permissions.py @@ -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, ) diff --git a/cli.py b/cli.py index c04b61a808..6ba6e8ab73 100644 --- a/cli.py +++ b/cli.py @@ -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"] diff --git a/tests/acp/test_permissions.py b/tests/acp/test_permissions.py index 3d8576d3e2..8ea5ff45ff 100644 --- a/tests/acp/test_permissions.py +++ b/tests/acp/test_permissions.py @@ -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"), diff --git a/tests/cli/test_cli_approval_ui.py b/tests/cli/test_cli_approval_ui.py index b12112017f..081a94bac8 100644 --- a/tests/cli/test_cli_approval_ui.py +++ b/tests/cli/test_cli_approval_ui.py @@ -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( + "", + "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() diff --git a/tests/tools/test_file_write_safety.py b/tests/tools/test_file_write_safety.py index 514aba33fb..fc92b55b09 100644 --- a/tests/tools/test_file_write_safety.py +++ b/tests/tools/test_file_write_safety.py @@ -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"]) diff --git a/tools/approval.py b/tools/approval.py index 1ecda55ec4..ac5f4c3ee7 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -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" diff --git a/tools/file_tools.py b/tools/file_tools.py index 2270fb57f6..2365f335e5 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -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"}: