From 12b28949dfc6c0d68eaae3984f7f54323a322eaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B1=9F=E5=9B=BD=E5=BA=86?= Date: Thu, 30 Jul 2026 17:52:56 +0800 Subject: [PATCH 1/4] feat: add shell command approval flow --- raven/agent/loop/main.py | 47 +++- raven/agent/tools/base.py | 22 +- raven/agent/tools/registry.py | 21 +- raven/agent/tools/shell.py | 128 +++++++++- raven/agent/tools/shell_policy.py | 159 ++++++++++++ raven/cli/tui_commands.py | 18 +- raven/tui_rpc/approval_broker.py | 157 ++++++++++++ raven/tui_rpc/methods/__init__.py | 14 +- raven/tui_rpc/methods/_stubs.py | 11 +- raven/tui_rpc/methods/approval.py | 54 +++++ raven/tui_rpc/spine.py | 31 ++- tests/test_agent_loop_approval.py | 167 +++++++++++++ tests/test_approval_broker.py | 222 +++++++++++++++++ tests/test_cli_tui_commands.py | 31 ++- tests/test_sandbox_unit.py | 8 +- tests/test_shell_approval.py | 227 ++++++++++++++++++ tests/test_tool_registry_execute.py | 36 ++- tests/test_tui_rpc_approval.py | 65 +++++ tests/test_tui_rpc_spine.py | 81 +++++++ tests/test_tui_rpc_stubs.py | 3 +- .../src/__tests__/approvalRoundTrip.test.ts | 179 ++++++++++++++ ui-tui/src/app/createGatewayEventHandler.ts | 24 +- ui-tui/src/app/useInputHandlers.ts | 16 +- ui-tui/src/app/useMainApp.ts | 39 ++- ui-tui/src/components/prompts.tsx | 46 +++- ui-tui/src/demo/gallery.tsx | 5 +- ui-tui/src/gatewayTypes.ts | 23 +- ui-tui/src/lib/approval.ts | 26 ++ ui-tui/src/types.ts | 4 + 29 files changed, 1802 insertions(+), 62 deletions(-) create mode 100644 raven/agent/tools/shell_policy.py create mode 100644 raven/tui_rpc/approval_broker.py create mode 100644 raven/tui_rpc/methods/approval.py create mode 100644 tests/test_agent_loop_approval.py create mode 100644 tests/test_approval_broker.py create mode 100644 tests/test_shell_approval.py create mode 100644 tests/test_tui_rpc_approval.py create mode 100644 ui-tui/src/__tests__/approvalRoundTrip.test.ts create mode 100644 ui-tui/src/lib/approval.ts diff --git a/raven/agent/loop/main.py b/raven/agent/loop/main.py index 5a17f8c1f..5a9a28dcd 100644 --- a/raven/agent/loop/main.py +++ b/raven/agent/loop/main.py @@ -51,6 +51,12 @@ from raven.tracing import semconv, trace from raven.utils.helpers import estimate_prompt_tokens +_ABORTED_ACTION_REPLY = ( + "The operation was not completed, and no alternative method will be attempted. " + "Would you like me to continue with the remaining parts of the task that do not " + "require this operation?" +) + # NOTE: ``raven.context_engine`` is intentionally imported lazily (inside # ``__init__`` and ``_assemble_context_messages``) to break a runtime # import cycle: ``raven.agent.__init__`` eagerly loads AgentLoop, @@ -1584,6 +1590,7 @@ async def _run_agent_loop( continue if response.has_tool_calls: + abort_action = False if on_progress: thought = self._strip_think(response.content) if thought: @@ -1599,7 +1606,7 @@ async def _run_agent_loop( thinking_blocks=response.thinking_blocks, ) - for tool_call in response.tool_calls: + for tool_call_index, tool_call in enumerate(response.tool_calls): tools_used.append(tool_call.name) args_str = json.dumps(tool_call.arguments, ensure_ascii=False) logger.info("Tool call: {}({})", tool_call.name, args_str[:200]) @@ -1618,6 +1625,10 @@ async def _run_agent_loop( "display": _tool.display_call(tool_call.arguments) if _tool else None, }, ) + if tool_call.name == "exec": + exec_tool = self.tools.get("exec") + if isinstance(exec_tool, ExecTool): + exec_tool.set_tool_call_id(tool_call.id) tool_t0 = time.monotonic() result = await self.tools.execute(tool_call.name, tool_call.arguments) duration_ms = int((time.monotonic() - tool_t0) * 1000) @@ -1647,6 +1658,26 @@ async def _run_agent_loop( }, ) messages = self.context.add_tool_result(messages, tool_call.id, tool_call.name, model_text) + if getattr(result, "abort_action", False): + abort_action = True + # A single assistant message may contain several parallel + # tool calls (for example ``rm`` followed by a Python + # fallback). Once policy terminates the action, none of + # the siblings may execute. We must nevertheless append + # one result for every advertised call id: OpenAI-style + # providers reject conversation history containing an + # assistant tool call without its matching tool result. + for skipped_call in response.tool_calls[tool_call_index + 1 :]: + messages = self.context.add_tool_result( + messages, + skipped_call.id, + skipped_call.name, + ( + "Error: Tool call was not executed because a prior safety " + "decision terminated this action." + ), + ) + break # #1b Track consecutive same-tool deterministic failures # (transient errors excluded — a retry would clear those). if _is_hard_tool_failure(model_text): @@ -1657,6 +1688,20 @@ async def _run_agent_loop( else: loop_fail_tool, loop_fail_streak = None, 0 + if abort_action: + # A normal tool result starts another model iteration. That + # is specifically unsafe here: the next plan can translate + # the rejected operation into an equivalent interpreter, + # script, or tool call. Finish the turn in runtime code and + # expose only the non-destructive continuation question. + # Streaming callers need the explicit callback because no + # final model response exists to generate token deltas. + messages = self.context.add_assistant_message(messages, _ABORTED_ACTION_REPLY) + final_content = _ABORTED_ACTION_REPLY + if on_token_delta is not None: + await on_token_delta(_ABORTED_ACTION_REPLY) + break + # #1b Failure-loop break: the same tool failed deterministically # `threshold` times running → append a change-approach nudge to # the last tool result so the model stops repeating a dead call. diff --git a/raven/agent/tools/base.py b/raven/agent/tools/base.py index 5fe740933..fc6c6fb08 100644 --- a/raven/agent/tools/base.py +++ b/raven/agent/tools/base.py @@ -14,10 +14,15 @@ class ToolResult: to a generic preview of it) or this, when the tool wants a cleaner transcript rendering than what it feeds the model. ``display_text`` must be built from the tool's own execution data, not by re-parsing ``model_text``. + ``retryable=False`` suppresses the registry's generic change-approach hint. + ``abort_action=True`` tells the agent loop not to execute sibling calls or + ask the model for another approach. """ model_text: str display_text: str | None = None + retryable: bool = True + abort_action: bool = False class ToolOutput(str): @@ -30,14 +35,25 @@ class ToolOutput(str): artifact, so the boundary has to return something that *is* a str; handing them a :class:`ToolResult` would format its repr into model context and user-facing replies. The agent loop reads ``display_text`` off it to render - the transcript row. + the transcript row and the control flags to enforce terminal tool decisions. """ display_text: str | None - - def __new__(cls, model_text: str, display_text: str | None = None) -> "ToolOutput": + retryable: bool + abort_action: bool + + def __new__( + cls, + model_text: str, + display_text: str | None = None, + *, + retryable: bool = True, + abort_action: bool = False, + ) -> "ToolOutput": out = super().__new__(cls, model_text) out.display_text = display_text + out.retryable = retryable + out.abort_action = abort_action return out diff --git a/raven/agent/tools/registry.py b/raven/agent/tools/registry.py index 061bd84b5..09e52e11a 100644 --- a/raven/agent/tools/registry.py +++ b/raven/agent/tools/registry.py @@ -72,12 +72,29 @@ async def execute(self, name: str, params: dict[str, Any]) -> str: # string (which rides along on ToolOutput). if isinstance(result, ToolResult): model_text, display_text = result.model_text, result.display_text + retryable, abort_action = result.retryable, result.abort_action else: model_text, display_text = str(result), None + retryable, abort_action = True, False if model_text.startswith("Error"): - return ToolOutput(model_text + _hint, display_text) - return ToolOutput(model_text, display_text) + # ``Error:`` describes presentation, not retry semantics. + # Policy-aware tools return explicit control metadata so the + # registry does not accidentally turn a security decision into + # the generic invitation to find an equivalent implementation. + suffix = _hint if retryable else "" + return ToolOutput( + model_text + suffix, + display_text, + retryable=retryable, + abort_action=abort_action, + ) + return ToolOutput( + model_text, + display_text, + retryable=retryable, + abort_action=abort_action, + ) except asyncio.TimeoutError: return f"Error: Tool '{name}' timed out after {ceiling:.0f}s." + _hint except Exception as e: diff --git a/raven/agent/tools/shell.py b/raven/agent/tools/shell.py index 1ad003dac..5183c6e0f 100644 --- a/raven/agent/tools/shell.py +++ b/raven/agent/tools/shell.py @@ -3,13 +3,46 @@ import os import re import shlex +from contextvars import ContextVar +from dataclasses import dataclass, replace +from hashlib import sha256 from pathlib import Path -from typing import Any +from typing import Any, Protocol -from raven.agent.tools.base import Tool +from raven.agent.tools.base import Tool, ToolResult +from raven.agent.tools.shell_policy import CommandDecision, ShellCommandPolicy from raven.sandbox import DirectExecutor, SandboxExecutor +class ApprovalResponder(Protocol): + """Turn-scoped capability that can approve one exact shell command.""" + + async def await_approval( + self, + *, + conversation_id: str, + turn_id: str, + tool_call_id: str, + command: str, + description: str, + ) -> bool: ... + + +@dataclass(frozen=True) +class _ApprovalTurn: + """Approval state isolated by async context for one agent turn. + + ``denied_digests`` suppresses duplicate prompts only within this turn; a + later user turn receives a fresh decision boundary. + """ + + responder: ApprovalResponder | None = None + conversation_id: str = "" + turn_id: str = "" + tool_call_id: str = "" + denied_digests: frozenset[str] = frozenset() + + class ExecTool(Tool): """Tool to execute shell commands.""" @@ -48,10 +81,37 @@ def __init__( # un-sandboxed on the operator's machine — not a product default. if extra_deny_patterns: self.deny_patterns = self.deny_patterns + list(extra_deny_patterns) + self._policy = ShellCommandPolicy(deny_patterns=self.deny_patterns) self.allow_patterns = allow_patterns or [] self.restrict_to_workspace = restrict_to_workspace self.path_append = path_append self._executor: SandboxExecutor = executor if executor is not None else DirectExecutor() + self._approval_turn: ContextVar[_ApprovalTurn] = ContextVar( + "exec_tool_approval_turn", + default=_ApprovalTurn(), + ) + + def start_approval_turn( + self, + responder: ApprovalResponder | None, + *, + conversation_id: str, + turn_id: str, + ) -> None: + """Bind or revoke interactive approval capability for the current turn.""" + + self._approval_turn.set( + _ApprovalTurn( + responder=responder, + conversation_id=conversation_id, + turn_id=turn_id, + ) + ) + + def set_tool_call_id(self, tool_call_id: str) -> None: + """Attach the provider call ID so approval is auditable end to end.""" + + self._approval_turn.set(replace(self._approval_turn.get(), tool_call_id=tool_call_id)) @property def name(self) -> str: @@ -59,6 +119,15 @@ def name(self) -> str: _MAX_TIMEOUT = 600 _MAX_OUTPUT = 10_000 + # This text is model-facing, not the user-facing turn summary. It closes + # the common loophole where a denied ``rm`` is translated into Python, + # Perl, or another shell form that performs the same protected action. + # Runtime enforcement in AgentLoop is still authoritative; the instruction + # keeps traces and any non-AgentLoop registry consumers equally explicit. + _STOP_INSTRUCTION = ( + " Stop this operation immediately. Do not retry it with another command, " + "tool, script, interpreter, or equivalent method." + ) @property def description(self) -> str: @@ -96,20 +165,27 @@ async def execute( working_dir: str | None = None, timeout: int | None = None, **kwargs: Any, - ) -> str: + ) -> str | ToolResult: cwd = working_dir or self.working_dir or os.getcwd() if not self._executor.is_sandboxed: # Non-sandboxed: full guard — deny-list patterns AND workspace restriction. guard_error = self._guard_command(command, cwd) if guard_error: - return guard_error + return self._terminal_error(guard_error) + decision = self._policy.evaluate(command) + if decision is CommandDecision.HARD_DENY: + return self._terminal_error("Error: Command blocked by safety guard (policy evaluation failed)") + if decision is CommandDecision.REQUIRE_APPROVAL: + approval_error = await self._request_approval(command) + if approval_error: + return approval_error elif self.restrict_to_workspace: # Sandboxed: skip the deny-list (microVM provides real isolation), but still # enforce workspace restriction so operator-set boundaries are respected. workspace_error = self._check_workspace_restriction(command, cwd) if workspace_error: - return workspace_error + return self._terminal_error(workspace_error) # Use `is None` check — `timeout or default` would treat timeout=0 as falsy. effective_timeout = min(self.timeout if timeout is None else timeout, self._MAX_TIMEOUT) @@ -133,6 +209,48 @@ async def execute( return f"Error executing command: {str(e)}" return result.as_text(self._MAX_OUTPUT) + async def _request_approval(self, command: str) -> ToolResult | None: + """Request one-shot authority for an exact command, failing closed. + + The responder belongs to the current turn and is installed only for an + Origin with a trusted approval transport. A missing responder therefore + means "cannot approve", not "approval unnecessary". A digest rejected + earlier in the same turn is remembered to avoid prompting repeatedly. + """ + turn = self._approval_turn.get() + digest = sha256(command.encode()).hexdigest() + if digest in turn.denied_digests: + return self._terminal_error("Error: User denied this command earlier in the current turn") + if turn.responder is None or not turn.conversation_id: + return self._terminal_error( + "Error: Command requires user approval, but this turn is not interactive" + ) + approved = await turn.responder.await_approval( + conversation_id=turn.conversation_id, + turn_id=turn.turn_id, + tool_call_id=turn.tool_call_id, + command=command, + description="Delete files using a shell command", + ) + if approved: + return None + self._approval_turn.set( + replace( + turn, + denied_digests=turn.denied_digests | {digest}, + ) + ) + return self._terminal_error("Error: User denied this command or the approval request expired") + + @classmethod + def _terminal_error(cls, message: str) -> ToolResult: + """Return a policy result that the registry and agent loop cannot retry.""" + return ToolResult( + model_text=message + cls._STOP_INSTRUCTION, + retryable=False, + abort_action=True, + ) + def _guard_command(self, command: str, cwd: str) -> str | None: """Best-effort safety guard for potentially destructive commands.""" cmd = command.strip() diff --git a/raven/agent/tools/shell_policy.py b/raven/agent/tools/shell_policy.py new file mode 100644 index 000000000..a2c7ac9c5 --- /dev/null +++ b/raven/agent/tools/shell_policy.py @@ -0,0 +1,159 @@ +"""Classify shell commands before execution. + +The policy deliberately operates on recognizable shell syntax, not on the +eventual effects of arbitrary programs. It identifies command families Raven +can classify reliably, including commands hidden behind common wrappers, while +the runtime sandbox remains responsible for its separate containment boundary. + +Classification order is security-sensitive: hard-denied commands must never be +downgraded into approval requests, and matcher failures fail closed instead of +silently permitting execution. +""" + +from __future__ import annotations + +import re +import shlex +from collections.abc import Callable, Iterator +from enum import StrEnum +from pathlib import PurePath + +ApprovalMatcher = Callable[[str], bool] + +_WRAPPER_OPTIONS_WITH_VALUE = { + "command": frozenset(), + "env": frozenset({"-u", "--unset", "-C", "--chdir", "-S", "--split-string"}), + "sudo": frozenset( + { + "-C", + "--close-from", + "-D", + "--chdir", + "-g", + "--group", + "-h", + "--host", + "-p", + "--prompt", + "-r", + "--role", + "-t", + "--type", + "-T", + "--command-timeout", + "-u", + "--user", + } + ), +} +_ASSIGNMENT = re.compile(r"[A-Za-z_][A-Za-z0-9_]*=.*", re.DOTALL) + + +class CommandDecision(StrEnum): + """Policy outcomes ordered from ordinary execution to terminal rejection.""" + + ALLOW = "allow" + HARD_DENY = "hard_deny" + REQUIRE_APPROVAL = "require_approval" + + +def _command_segments(command: str) -> Iterator[list[str]]: + """Yield compound shell commands as independently classified token lists. + + This conservative lexical split catches deletion in common sequence, + conditional, and pipeline forms without pretending to evaluate expansions + or reproduce the full shell grammar. + """ + + lexer = shlex.shlex(command, posix=True, punctuation_chars=";&|") + lexer.commenters = "" + lexer.whitespace_split = True + segment: list[str] = [] + for token in lexer: + if token and all(char in ";&|" for char in token): + if segment: + yield segment + segment = [] + continue + segment.append(token) + if segment: + yield segment + + +def _matches_delete_command(command: str) -> bool: + """Recognize direct file-deletion commands after wrapper normalization.""" + + for segment in _command_segments(command): + segment = _unwrap_command_wrappers(segment) + if not segment: + continue + executable = PurePath(segment[0]).name + if executable in {"rm", "unlink"}: + return True + if executable == "find" and "-delete" in segment[1:]: + return True + return False + + +def _unwrap_command_wrappers(segment: list[str]) -> list[str]: + """Expose an executable hidden behind assignments, env, sudo, or command. + + Normalizing well-known wrappers prevents trivial approval bypasses. Unknown + executables and option shapes remain untouched rather than being guessed at. + """ + + tokens = list(segment) + while tokens: + while tokens and _ASSIGNMENT.fullmatch(tokens[0]): + tokens.pop(0) + if not tokens: + return tokens + wrapper = PurePath(tokens[0]).name + options_with_value = _WRAPPER_OPTIONS_WITH_VALUE.get(wrapper) + if options_with_value is None: + return tokens + tokens = tokens[1:] + while tokens and tokens[0].startswith("-"): + option = tokens.pop(0) + if option == "--": + break + option_name = option.split("=", 1)[0] + if option_name in options_with_value and "=" not in option and tokens: + tokens.pop(0) + if wrapper == "env": + while tokens and "=" in tokens[0] and not tokens[0].startswith("="): + tokens = tokens[1:] + return tokens + + +class ShellCommandPolicy: + """Apply hard-deny and approval rules in their required precedence order.""" + + def __init__(self, *, deny_patterns: list[str]) -> None: + # Compile once because every direct shell execution crosses this policy. + self._deny_patterns = tuple(re.compile(pattern, re.IGNORECASE) for pattern in deny_patterns) + self._approval_matchers: list[tuple[str, ApprovalMatcher]] = [("delete_command", _matches_delete_command)] + + def register_approval_matcher(self, name: str, matcher: ApprovalMatcher) -> None: + """Extend approval classification with a named command-family matcher.""" + + self._approval_matchers.append((name, matcher)) + + def evaluate(self, command: str) -> CommandDecision: + """Classify a command, reducing authority when a matcher cannot decide.""" + + # Hard deny runs first so an approval matcher can never convert an + # unconditionally forbidden command into an approvable operation. + if any(pattern.search(command) for pattern in self._deny_patterns): + return CommandDecision.HARD_DENY + try: + if any(matcher(command) for _, matcher in self._approval_matchers): + return CommandDecision.REQUIRE_APPROVAL + except Exception: + # Matchers inspect untrusted command text and may be extended later. + # A faulty matcher must close the gate, not bypass it. + return CommandDecision.HARD_DENY + return CommandDecision.ALLOW + + +__all__ = ["ApprovalMatcher", "CommandDecision", "ShellCommandPolicy"] diff --git a/raven/cli/tui_commands.py b/raven/cli/tui_commands.py index 5cd349a95..5fba6879f 100644 --- a/raven/cli/tui_commands.py +++ b/raven/cli/tui_commands.py @@ -487,6 +487,7 @@ async def _run_rpc_server_until_done( """ # Lazy import: keeps tui_commands importable without pulling tui_rpc on # users who never touch the TUI (e.g. CLI-only workflows). + from raven.tui_rpc.approval_broker import ApprovalBroker from raven.tui_rpc.confirm_broker import ConfirmBroker from raven.tui_rpc.dispatcher import Dispatcher from raven.tui_rpc.methods import register_aligned_methods_except_system @@ -518,11 +519,12 @@ async def hello_then_signal(params: dict) -> dict: # which can only happen post-handshake / post-serve. server = RpcServer(dispatcher=dispatcher, sock=conn, auth_token=auth_token) emitter = SubscriptionEmitter(send_frame=server.send_frame) - # ConfirmBroker shares the same send_frame sink; it lets a paused - # cli.dispatch (typer.confirm) emit a confirm.request and await the - # confirm.respond. cancel_all() in the finally fail-safes any - # pending confirm to its default when the connection drops. + # Prompt brokers share the gateway's send_frame sink but retain separate + # semantics. Shell approval is not a conversational confirmation: it binds + # one exact command to one turn, has dual deadlines, and always fails closed + # when the transport disappears. confirm_broker = ConfirmBroker(send_frame=server.send_frame) + approval_broker = ApprovalBroker(send_frame=server.send_frame) # QuestionBroker shares the same send_frame sink: the ask_user tool emits a # clarify.request and awaits clarify.respond, mirroring ConfirmBroker. question_broker = QuestionBroker(send_frame=server.send_frame) @@ -582,6 +584,7 @@ def _agent_loop_factory(): emitter, on_turn_end=turn_module.clear_active, readback_texts=cron_readback, + approval_responder=approval_broker, ) # Subagent result re-injection submits a SUBAGENT-origin turn. agent_loop.subagents.set_submit(turn_scheduler.submit) @@ -614,6 +617,7 @@ def _agent_loop_factory(): dispatcher, emitter=emitter, agent_loop_factory=_agent_loop_factory, + approval_broker=approval_broker, confirm_broker=confirm_broker, question_broker=question_broker, scheduler=turn_scheduler, @@ -664,9 +668,11 @@ async def _start_backend() -> None: await proc_done.wait() return True finally: - # Fail-safe any pending confirm so a paused dispatch's worker thread - # is released when the connection drops. + # Release all pending UI waits before transport teardown. Approval + # cancellation is always denial, preserving fail-closed behavior on a + # disconnect; ordinary confirms retain their configured default. confirm_broker.cancel_all() + approval_broker.cancel_all() if agent_loop is not None and agent_loop.cron_service is not None: try: agent_loop.cron_service.stop() diff --git a/raven/tui_rpc/approval_broker.py b/raven/tui_rpc/approval_broker.py new file mode 100644 index 000000000..0245349fc --- /dev/null +++ b/raven/tui_rpc/approval_broker.py @@ -0,0 +1,157 @@ +"""Runtime-owned approval round-trip for protected shell commands. + +``ExecTool`` reaches this broker only after ``ShellCommandPolicy`` classifies +an exact command as ``REQUIRE_APPROVAL``. The broker mints an ``approval_id``, +emits ``approval.request`` to the TUI, and blocks that tool call until the user +answers or the backend hard limit expires. Approval is deliberately scoped to +that one command and one conversation; there is no "always allow" state. + +Timeouts have two layers: + +* ``visible_timeout_s`` is serialized as ``expires_at``. The TUI owns the + countdown and fail-closes its overlay at that deadline. +* ``hard_timeout_s`` is the backend ceiling. It is slightly longer so a choice + made before the visible deadline can survive event-loop or RPC transport lag. + +The frontend timer is not authoritative cleanup: the process may be suspended, +the socket may disconnect, or a notification may arrive late. Therefore every +backend outcome emits ``approval.closed`` in ``finally``. The frontend matches +the id before clearing, so a delayed close cannot dismiss a newer request. +""" + +from __future__ import annotations + +import asyncio +import hashlib +import time +from collections.abc import Awaitable, Callable +from dataclasses import dataclass +from typing import Any +from uuid import uuid4 + +from loguru import logger + +SendFrame = Callable[[dict[str, Any]], Awaitable[None]] + + +@dataclass +class _PendingApproval: + conversation_id: str + future: asyncio.Future[tuple[bool, str]] + + +class ApprovalBroker: + """Coordinate one-shot TUI approvals without delegating authority to the model.""" + + def __init__( + self, + send_frame: SendFrame, + *, + visible_timeout_s: float = 30.0, + hard_timeout_s: float = 35.0, + ) -> None: + if visible_timeout_s <= 0 or hard_timeout_s < visible_timeout_s: + raise ValueError("approval timeouts must satisfy 0 < visible <= hard") + # The frontend expires first; the extra backend window lets a response + # chosen before the visible deadline survive transport/event-loop lag. + self._send_frame = send_frame + self._visible_timeout_s = visible_timeout_s + self._hard_timeout_s = hard_timeout_s + self._pending: dict[str, _PendingApproval] = {} + + async def await_approval( + self, + *, + conversation_id: str, + turn_id: str, + tool_call_id: str, + command: str, + description: str, + ) -> bool: + """Wait for an approval decision and fail closed on every error path. + + ``True`` means the exact command may execute once. User denial, visible + timeout forwarded by the TUI, backend timeout, connection failure, and + broker cancellation all resolve to ``False``. Exceptions are contained + here because an approval transport failure must never turn into tool + execution or leave the agent loop waiting indefinitely. + """ + approval_id = uuid4().hex + future = asyncio.get_running_loop().create_future() + created_at = time.time() + close_reason = "cancelled" + request_sent = False + self._pending[approval_id] = _PendingApproval( + conversation_id=conversation_id, + future=future, + ) + try: + await self._send_frame( + { + "jsonrpc": "2.0", + "method": "approval.request", + "params": { + "approval_id": approval_id, + "conversation_id": conversation_id, + "turn_id": turn_id, + "tool_call_id": tool_call_id, + "command": command, + "description": description, + "action_digest": hashlib.sha256(command.encode()).hexdigest(), + "created_at": created_at, + "expires_at": created_at + self._visible_timeout_s, + }, + } + ) + request_sent = True + approved, close_reason = await asyncio.wait_for(future, self._hard_timeout_s) + return approved + except TimeoutError: + close_reason = "timeout" + return False + except Exception: + close_reason = "error" + logger.exception("approval_broker: request failed for {}", approval_id) + return False + finally: + self._pending.pop(approval_id, None) + if request_sent: + try: + # Frontend timers are best-effort (a suspended terminal may + # never fire), so every backend outcome closes the overlay. + await self._send_frame( + { + "jsonrpc": "2.0", + "method": "approval.closed", + "params": { + "approval_id": approval_id, + "conversation_id": conversation_id, + "reason": close_reason, + }, + } + ) + except Exception: + logger.exception("approval_broker: close notification failed for {}", approval_id) + + def resolve(self, approval_id: str, choice: str, *, conversation_id: str) -> bool: + """Resolve a live request only when both opaque id and conversation match. + + Returning ``False`` for stale, duplicate, cross-conversation, or invalid + responses makes late UI input harmless and keeps resolution idempotent. + """ + if choice not in {"allow", "deny"}: + return False + pending = self._pending.get(approval_id) + if pending is None or pending.conversation_id != conversation_id or pending.future.done(): + return False + pending.future.set_result((choice == "allow", choice)) + return True + + def cancel_all(self) -> None: + """Fail-close pending approvals during RPC teardown or TUI disconnect.""" + for pending in list(self._pending.values()): + if not pending.future.done(): + pending.future.set_result((False, "cancelled")) + + +__all__ = ["ApprovalBroker", "SendFrame"] diff --git a/raven/tui_rpc/methods/__init__.py b/raven/tui_rpc/methods/__init__.py index 4ab8da23b..3b5035236 100644 --- a/raven/tui_rpc/methods/__init__.py +++ b/raven/tui_rpc/methods/__init__.py @@ -29,6 +29,7 @@ from typing import TYPE_CHECKING from raven.tui_rpc.methods._stubs import register_stub_methods +from raven.tui_rpc.methods.approval import register_approval_methods from raven.tui_rpc.methods.cli_dispatch import register_cli_methods from raven.tui_rpc.methods.commands import register_commands_methods from raven.tui_rpc.methods.config import register_config_methods @@ -45,6 +46,7 @@ if TYPE_CHECKING: from raven.spine.scheduler import Scheduler + from raven.tui_rpc.approval_broker import ApprovalBroker from raven.tui_rpc.confirm_broker import ConfirmBroker from raven.tui_rpc.dispatcher import Dispatcher from raven.tui_rpc.errors import RpcError @@ -58,6 +60,7 @@ def register_aligned_methods( *, emitter: "SubscriptionEmitter | None" = None, agent_loop_factory: "AgentLoopFactory | None" = None, + approval_broker: "ApprovalBroker | None" = None, confirm_broker: "ConfirmBroker | None" = None, question_broker: "QuestionBroker | None" = None, scheduler: "Scheduler | None" = None, @@ -75,13 +78,16 @@ def register_aligned_methods( ``emitter`` is ``None`` the ``turn.*`` group is skipped (the demo runner path that does not own a streaming subscription channel still works without them); ``agent_loop_factory`` is forwarded to the session methods. - ``confirm_broker`` is forwarded to :func:`register_confirm_methods`. + ``confirm_broker`` is forwarded to :func:`register_confirm_methods`; + ``approval_broker`` gates the shell approval response surface so callers + without an interactive broker do not expose an unusable approval method. """ register_system_methods(dispatcher) register_aligned_methods_except_system( dispatcher, emitter=emitter, agent_loop_factory=agent_loop_factory, + approval_broker=approval_broker, confirm_broker=confirm_broker, question_broker=question_broker, scheduler=scheduler, @@ -95,6 +101,7 @@ def register_aligned_methods_except_system( *, emitter: "SubscriptionEmitter | None" = None, agent_loop_factory: "AgentLoopFactory | None" = None, + approval_broker: "ApprovalBroker | None" = None, confirm_broker: "ConfirmBroker | None" = None, question_broker: "QuestionBroker | None" = None, scheduler: "Scheduler | None" = None, @@ -133,6 +140,10 @@ def register_aligned_methods_except_system( # come AFTER register_stub_methods so session.status's real handler # supersedes the (now-removed) hermes-only stub entry. register_slash_routing_methods(dispatcher, confirm_broker=confirm_broker) + # Unlike generic stubs, approval.respond is a capability-bearing endpoint. + # Register it only when this gateway owns an interactive approval broker. + if approval_broker is not None: + register_approval_methods(dispatcher, approval_broker=approval_broker) # turn.{send,subscribe,unsubscribe,cancel}. The handlers # need a SubscriptionEmitter to push streaming events; when the caller # has not built one (demo runner / production path pre-wire) we skip @@ -175,6 +186,7 @@ def register_aligned_methods_except_system( "register_model_methods", "register_slash_routing_methods", "register_turn_methods", + "register_approval_methods", "register_confirm_methods", "register_question_methods", ] diff --git a/raven/tui_rpc/methods/_stubs.py b/raven/tui_rpc/methods/_stubs.py index d35941c4e..cd8b8a5d1 100644 --- a/raven/tui_rpc/methods/_stubs.py +++ b/raven/tui_rpc/methods/_stubs.py @@ -30,8 +30,7 @@ closed loop, no manual reload trigger) * ``reload.env`` — hermes env-file hot-reload (Raven reads env on process start only) -* ``approval.respond`` / ``sudo.respond`` / ``secret.respond`` — hermes - interactive approval flows (Raven v0.1 has no approval UI) +* ``sudo.respond`` / ``secret.respond`` — hermes interactive credential flows * ``image.attach`` — hermes image-paste attachment * ``prompt.submit`` / ``prompt.background`` — hermes "stash this prompt for later" flow; Raven v0.1 only supports inline submit via the chat turn @@ -148,13 +147,7 @@ "reload.env not supported in Raven v0.1", "Restart `raven tui` to pick up environment changes.", ), - # approval / sudo / secret response — hermes interactive approval flows. - # Raven v0.1 has no approval UI; users edit config directly. - ( - "approval.respond", - "approval.respond not supported in Raven v0.1", - "Raven v0.1 has no interactive approval flow.", - ), + # sudo / secret response — hermes interactive credential flows. ( "sudo.respond", "sudo.respond not supported in Raven v0.1", diff --git a/raven/tui_rpc/methods/approval.py b/raven/tui_rpc/methods/approval.py new file mode 100644 index 000000000..fda3dc68a --- /dev/null +++ b/raven/tui_rpc/methods/approval.py @@ -0,0 +1,54 @@ +"""Resolve Raven-owned shell approval requests at the TUI RPC boundary. + +This handler does not classify commands or grant authority by itself. It only +forwards an explicit allow-once or deny response to the broker that owns the +pending request. The opaque approval ID and conversation binding keep stale or +cross-session UI responses from resolving a different request. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from raven.tui_rpc.approval_broker import ApprovalBroker + from raven.tui_rpc.dispatcher import Dispatcher + + +async def approval_respond( + params: dict[str, Any], + *, + approval_broker: "ApprovalBroker", +) -> dict[str, bool]: + """Resolve a pending request and report whether the broker accepted it.""" + + approval_id = str(params.get("approval_id", "")) + # session_id is the canonical TUI wire field. conversation_id remains a + # compatibility fallback, with the broker enforcing the same binding. + conversation_id = str(params.get("session_id") or params.get("conversation_id") or "") + choice = str(params.get("choice", "")) + if not approval_id or not conversation_id: + return {"ok": False} + return { + "ok": approval_broker.resolve( + approval_id, + choice, + conversation_id=conversation_id, + ) + } + + +def register_approval_methods( + dispatcher: "Dispatcher", + *, + approval_broker: "ApprovalBroker", +) -> None: + """Register the broker-backed approval response endpoint.""" + + async def _respond(params: dict[str, Any]) -> dict[str, bool]: + return await approval_respond(params, approval_broker=approval_broker) + + dispatcher.register("approval.respond", _respond) + + +__all__ = ["approval_respond", "register_approval_methods"] diff --git a/raven/tui_rpc/spine.py b/raven/tui_rpc/spine.py index f239180b6..0345cd3a6 100644 --- a/raven/tui_rpc/spine.py +++ b/raven/tui_rpc/spine.py @@ -21,6 +21,7 @@ from raven.agent.spine_runner import AgentTurnRunner from raven.agent.tools.message import MessageTool +from raven.agent.tools.shell import ApprovalResponder, ExecTool from raven.spine import ( Deliverable, EpisodeStart, @@ -70,15 +71,29 @@ def __init__( usages: dict[str, dict[str, Any]], turn_ids: dict[str, str], readback_texts: dict[str, str], + approval_responder: ApprovalResponder | None = None, ) -> None: super().__init__(agent_loop, stream=True) self._emitter = emitter self._usages = usages self._turn_ids = turn_ids self._readback_texts = readback_texts + self._approval_responder = approval_responder async def run(self, req: TurnRequest, emit: Emit, drain: Drain) -> TurnOutcome: cid = _conversation_id(req) + tools = getattr(self._loop, "tools", None) + exec_tool = tools.get("exec") if tools is not None else None + if isinstance(exec_tool, ExecTool): + # Approval capability is rebound for every turn. Only USER origin + # receives the TUI responder; CRON and other background origins can + # share this process but must still fail closed as non-interactive. + # The IDs bind any response to this exact conversation and turn. + exec_tool.start_approval_turn( + self._approval_responder if req.origin is Origin.USER else None, + conversation_id=cid, + turn_id=self._turn_ids.get(cid, ""), + ) # A CRON turn is not a user turn: it runs non-streaming (one reply, not a # token stream) and its reply text is captured for the cron fan-out, which # delivers a cron.delivered event to every session (the cron: @@ -266,6 +281,7 @@ def build_tui( channel: str = "tui", on_turn_end: Callable[[str], None] | None = None, readback_texts: dict[str, str] | None = None, + approval_responder: ApprovalResponder | None = None, user_pool: int = 1, system_pool: int = 1, ) -> tuple[Scheduler, DeliveryHub, dict[str, str], Callable[[], Awaitable[None]]]: @@ -280,7 +296,11 @@ def build_tui( ``readback_texts`` is the cron read-back map (conversation -> reply text): the runner stores a CRON turn's reply there so the cron fan-out can deliver it as a cron.delivered event. Pass the same dict the cron callback reads; defaults to a - private map when cron is not wired (e.g. tests).""" + private map when cron is not wired (e.g. tests). + + ``approval_responder`` is an interactive capability, not a process-wide + permission. The runner binds it only to USER-origin turns and explicitly + revokes it for background origins.""" hub = DeliveryHub() outlet = TuiOutlet(channel, emitter) hub.register(outlet) @@ -289,7 +309,14 @@ def build_tui( if readback_texts is None: readback_texts = {} scheduler = Scheduler( - TuiTurnRunner(agent_loop, emitter, usages, turn_ids, readback_texts), + TuiTurnRunner( + agent_loop, + emitter, + usages, + turn_ids, + readback_texts, + approval_responder=approval_responder, + ), OriginPools(user=user_pool, system=system_pool), _make_tui_sink(hub, outlet, channel, turn_ids, usages, on_turn_end), ) diff --git a/tests/test_agent_loop_approval.py b/tests/test_agent_loop_approval.py new file mode 100644 index 000000000..fc74a4446 --- /dev/null +++ b/tests/test_agent_loop_approval.py @@ -0,0 +1,167 @@ +from __future__ import annotations + +from raven.agent.loop import AgentLoop +from raven.agent.tools.shell import ExecTool +from raven.providers.base import LLMResponse, ToolCallRequest +from raven.sandbox import ExecResult, SandboxExecutor +from raven.spine.message import ChatType, Source +from raven.spine.turn import Origin, TurnRequest + + +class _Provider: + def __init__(self) -> None: + self.responses = [ + LLMResponse( + content="", + tool_calls=[ + ToolCallRequest( + id="call-a", + name="exec", + arguments={"command": "rm file.txt"}, + ) + ], + finish_reason="tool_calls", + ), + LLMResponse(content="The file was deleted.", finish_reason="stop"), + ] + + async def chat_with_retry(self, **kwargs) -> LLMResponse: + return self.responses.pop(0) + + def get_default_model(self) -> str: + return "fake/model" + + +class _ParallelDeleteProvider(_Provider): + def __init__(self) -> None: + self.responses = [ + LLMResponse( + content="", + tool_calls=[ + ToolCallRequest( + id="call-a", + name="exec", + arguments={"command": "rm file.txt"}, + ), + ToolCallRequest( + id="call-b", + name="exec", + arguments={"command": "python3 -c \"import os; os.remove('file.txt')\""}, + ), + ], + finish_reason="tool_calls", + ), + LLMResponse(content="The file was deleted.", finish_reason="stop"), + ] + + +class _Executor(SandboxExecutor): + def __init__(self) -> None: + self.commands: list[str] = [] + + @property + def is_sandboxed(self) -> bool: + return False + + async def exec(self, command: str, **kwargs) -> ExecResult: + self.commands.append(command) + return ExecResult(stdout="ok", stderr="", exit_code=0) + + +class _Responder: + def __init__(self, answer: bool = True) -> None: + self.answer = answer + self.requests: list[dict] = [] + + async def await_approval(self, **request) -> bool: + self.requests.append(request) + return self.answer + + +async def test_agent_loop_binds_tool_call_id_to_approval_request(tmp_path) -> None: + agent = AgentLoop(provider=_Provider(), workspace=tmp_path, model="fake/model") + executor = _Executor() + responder = _Responder() + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-a") + agent.tools.register(tool) + + result, _media = await agent._process_message( + TurnRequest( + origin=Origin.USER, + source=Source( + channel="tui", + chat_id="default", + sender_id="user", + chat_type=ChatType.DM, + ), + text="delete file.txt", + conversation="session-a", + ), + session_key="session-a", + ) + + assert result == "The file was deleted." + assert responder.requests[0]["tool_call_id"] == "call-a" + assert executor.commands == ["rm file.txt"] + + +async def test_agent_loop_stops_after_user_denies_delete(tmp_path) -> None: + provider = _Provider() + agent = AgentLoop(provider=provider, workspace=tmp_path, model="fake/model") + executor = _Executor() + responder = _Responder(answer=False) + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-a") + agent.tools.register(tool) + + result, _media = await agent._process_message( + TurnRequest( + origin=Origin.USER, + source=Source( + channel="tui", + chat_id="default", + sender_id="user", + chat_type=ChatType.DM, + ), + text="delete file.txt", + conversation="session-a", + ), + session_key="session-a", + ) + + assert "no alternative method will be attempted" in result + assert executor.commands == [] + assert len(provider.responses) == 1 + + +async def test_agent_loop_skips_remaining_tool_calls_after_delete_denial(tmp_path) -> None: + provider = _ParallelDeleteProvider() + agent = AgentLoop(provider=provider, workspace=tmp_path, model="fake/model") + executor = _Executor() + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn( + _Responder(answer=False), + conversation_id="session-a", + turn_id="turn-a", + ) + agent.tools.register(tool) + + result, _media = await agent._process_message( + TurnRequest( + origin=Origin.USER, + source=Source( + channel="tui", + chat_id="default", + sender_id="user", + chat_type=ChatType.DM, + ), + text="delete file.txt", + conversation="session-a", + ), + session_key="session-a", + ) + + assert "no alternative method will be attempted" in result + assert executor.commands == [] + assert len(provider.responses) == 1 diff --git a/tests/test_approval_broker.py b/tests/test_approval_broker.py new file mode 100644 index 000000000..cc6aeb778 --- /dev/null +++ b/tests/test_approval_broker.py @@ -0,0 +1,222 @@ +from __future__ import annotations + +import asyncio +import hashlib + +import pytest + +from raven.tui_rpc.approval_broker import ApprovalBroker + + +async def _wait_for_frame(frames: list[dict]) -> dict: + for _ in range(20): + if frames: + return frames[0] + await asyncio.sleep(0) + raise AssertionError("approval request was not emitted") + + +@pytest.mark.parametrize( + ("choice", "expected"), + [("allow", True), ("deny", False)], +) +async def test_response_resolves_matching_request(choice: str, expected: bool) -> None: + frames: list[dict] = [] + + async def send(frame: dict) -> None: + frames.append(frame) + + broker = ApprovalBroker(send) + waiting = asyncio.create_task( + broker.await_approval( + conversation_id="session-a", + turn_id="turn-a", + tool_call_id="call-a", + command="rm file.txt", + description="Delete files", + ) + ) + frame = await _wait_for_frame(frames) + params = frame["params"] + + assert frame["method"] == "approval.request" + assert params["conversation_id"] == "session-a" + assert params["turn_id"] == "turn-a" + assert params["tool_call_id"] == "call-a" + assert params["command"] == "rm file.txt" + assert params["action_digest"] == hashlib.sha256(b"rm file.txt").hexdigest() + assert broker.resolve(params["approval_id"], choice, conversation_id="session-a") is True + assert await waiting is expected + + +async def test_wrong_session_cannot_resolve_request() -> None: + frames: list[dict] = [] + + async def send(frame: dict) -> None: + frames.append(frame) + + broker = ApprovalBroker(send) + waiting = asyncio.create_task( + broker.await_approval( + conversation_id="session-a", + turn_id="turn-a", + tool_call_id="call-a", + command="rm file.txt", + description="Delete files", + ) + ) + params = (await _wait_for_frame(frames))["params"] + + assert broker.resolve(params["approval_id"], "allow", conversation_id="session-b") is False + assert broker.resolve(params["approval_id"], "deny", conversation_id="session-a") is True + assert await waiting is False + + +async def test_duplicate_and_invalid_responses_are_rejected() -> None: + frames: list[dict] = [] + + async def send(frame: dict) -> None: + frames.append(frame) + + broker = ApprovalBroker(send) + waiting = asyncio.create_task( + broker.await_approval( + conversation_id="session-a", + turn_id="turn-a", + tool_call_id="call-a", + command="unlink file.txt", + description="Delete files", + ) + ) + approval_id = (await _wait_for_frame(frames))["params"]["approval_id"] + + assert broker.resolve(approval_id, "always", conversation_id="session-a") is False + assert broker.resolve(approval_id, "allow", conversation_id="session-a") is True + assert broker.resolve(approval_id, "deny", conversation_id="session-a") is False + assert await waiting is True + + +async def test_timeout_denies_and_expires_request() -> None: + frames: list[dict] = [] + + async def send(frame: dict) -> None: + frames.append(frame) + + broker = ApprovalBroker(send, visible_timeout_s=0.01, hard_timeout_s=0.02) + result = await broker.await_approval( + conversation_id="session-a", + turn_id="turn-a", + tool_call_id="call-a", + command="rm file.txt", + description="Delete files", + ) + approval_id = frames[0]["params"]["approval_id"] + + assert result is False + assert frames[1] == { + "jsonrpc": "2.0", + "method": "approval.closed", + "params": { + "approval_id": approval_id, + "conversation_id": "session-a", + "reason": "timeout", + }, + } + assert broker.resolve(approval_id, "allow", conversation_id="session-a") is False + + +async def test_request_exposes_the_shorter_frontend_deadline() -> None: + frames: list[dict] = [] + + async def send(frame: dict) -> None: + frames.append(frame) + + broker = ApprovalBroker(send, visible_timeout_s=30, hard_timeout_s=35) + waiting = asyncio.create_task( + broker.await_approval( + conversation_id="session-a", + turn_id="turn-a", + tool_call_id="call-a", + command="rm file.txt", + description="Delete files", + ) + ) + params = (await _wait_for_frame(frames))["params"] + + assert 29 <= params["expires_at"] - params["created_at"] <= 30 + assert broker.resolve(params["approval_id"], "deny", conversation_id="session-a") is True + assert await waiting is False + assert frames[-1]["method"] == "approval.closed" + assert frames[-1]["params"]["reason"] == "deny" + + +async def test_cancel_all_denies_every_pending_request() -> None: + frames: list[dict] = [] + + async def send(frame: dict) -> None: + frames.append(frame) + + broker = ApprovalBroker(send) + waits = [ + asyncio.create_task( + broker.await_approval( + conversation_id=f"session-{index}", + turn_id=f"turn-{index}", + tool_call_id=f"call-{index}", + command=f"rm file-{index}", + description="Delete files", + ) + ) + for index in range(2) + ] + for _ in range(20): + if len(frames) == 2: + break + await asyncio.sleep(0) + + broker.cancel_all() + + assert await asyncio.gather(*waits) == [False, False] + + +async def test_task_cancellation_expires_request() -> None: + frames: list[dict] = [] + + async def send(frame: dict) -> None: + frames.append(frame) + + broker = ApprovalBroker(send) + waiting = asyncio.create_task( + broker.await_approval( + conversation_id="session-a", + turn_id="turn-a", + tool_call_id="call-a", + command="rm file.txt", + description="Delete files", + ) + ) + approval_id = (await _wait_for_frame(frames))["params"]["approval_id"] + + waiting.cancel() + with pytest.raises(asyncio.CancelledError): + await waiting + + assert broker.resolve(approval_id, "allow", conversation_id="session-a") is False + + +async def test_send_failure_denies_request() -> None: + async def send(frame: dict) -> None: + raise RuntimeError("disconnected") + + broker = ApprovalBroker(send) + + assert ( + await broker.await_approval( + conversation_id="session-a", + turn_id="turn-a", + tool_call_id="call-a", + command="rm file.txt", + description="Delete files", + ) + is False + ) diff --git a/tests/test_cli_tui_commands.py b/tests/test_cli_tui_commands.py index 869634adb..361d4a522 100644 --- a/tests/test_cli_tui_commands.py +++ b/tests/test_cli_tui_commands.py @@ -279,6 +279,11 @@ async def _fake_serve_forever(): lambda **kw: fake_emitter, ) + # Load the lazily imported module before patching its attributes; otherwise + # this fixture can depend on test order through Python's import cache. + from raven.tui_rpc.methods import system as _system_module + + assert _system_module is not None fake_confirm_broker = MagicMock() fake_confirm_broker.cancel_all = MagicMock() monkeypatch.setattr( @@ -286,6 +291,13 @@ async def _fake_serve_forever(): lambda **kw: fake_confirm_broker, ) + fake_approval_broker = MagicMock() + fake_approval_broker.cancel_all = MagicMock() + monkeypatch.setattr( + "raven.tui_rpc.approval_broker.ApprovalBroker", + lambda **kw: fake_approval_broker, + ) + fake_question_broker = MagicMock() monkeypatch.setattr( "raven.tui_rpc.question_broker.QuestionBroker", @@ -310,16 +322,16 @@ async def _fake_system_hello(params): async def _fake_turn_teardown(): pass - monkeypatch.setattr( - "raven.tui_rpc.spine.build_tui", - lambda *a, **kw: (fake_turn_scheduler, fake_turn_hub, fake_turn_ids, _fake_turn_teardown), - ) + fake_build_tui = MagicMock(return_value=(fake_turn_scheduler, fake_turn_hub, fake_turn_ids, _fake_turn_teardown)) + monkeypatch.setattr("raven.tui_rpc.spine.build_tui", fake_build_tui) monkeypatch.setattr("raven.cli._cron_handler.make_on_cron_job", MagicMock()) monkeypatch.setattr("raven.tui_rpc.methods.turn.clear_active", MagicMock()) ctx["fake_server"] = fake_server ctx["fake_confirm_broker"] = fake_confirm_broker + ctx["fake_approval_broker"] = fake_approval_broker + ctx["fake_build_tui"] = fake_build_tui ctx["dispatcher"] = fake_dispatcher return ctx @@ -376,6 +388,17 @@ async def test_rpc_runner_calls_backend_stop_on_exit(rpc_server_deps, monkeypatc assert rpc_server_deps["stop_calls"] == ["stop"], "backend.stop() must be called exactly once in the finally block" +async def test_rpc_runner_wires_and_cancels_approval_broker(rpc_server_deps, monkeypatch) -> None: + await _run_until_done_with_immediate_proc_done(monkeypatch, rpc_server_deps) + + approval_broker = rpc_server_deps["fake_approval_broker"] + assert rpc_server_deps["fake_build_tui"].call_args.kwargs["approval_responder"] is approval_broker + registration = __import__("raven.tui_rpc.methods", fromlist=["register"]) + register_mock = registration.register_aligned_methods_except_system + assert register_mock.call_args.kwargs["approval_broker"] is approval_broker + approval_broker.cancel_all.assert_called_once_with() + + async def test_rpc_runner_stop_called_even_when_serve_raises(rpc_server_deps, monkeypatch) -> None: """``backend.stop()`` must be awaited even when an exception propagates through the try block of ``_run_rpc_server_until_done``, proving the embedded index lock diff --git a/tests/test_sandbox_unit.py b/tests/test_sandbox_unit.py index cefb56ea5..a66561b04 100644 --- a/tests/test_sandbox_unit.py +++ b/tests/test_sandbox_unit.py @@ -285,7 +285,7 @@ async def test_sandboxed_workspace_restriction_enforced(self, tmp_path): restrict_to_workspace=True, ) result = await tool.execute("cat ../../../etc/passwd", working_dir=str(tmp_path)) - assert "blocked" in result + assert "blocked" in result.model_text assert len(executor.calls) == 0 async def test_non_sandboxed_deny_list_runs(self, tmp_path): @@ -295,7 +295,7 @@ async def test_non_sandboxed_deny_list_runs(self, tmp_path): executor = DirectMockExecutor() tool = ExecTool(executor=executor, working_dir=str(tmp_path)) result = await tool.execute("rm -rf /important") - assert "blocked" in result + assert "blocked" in result.model_text assert len(executor.calls) == 0 # Host GUI automation (osascript / `open -a|-b`) is NOT a product default — @@ -320,7 +320,7 @@ async def run(cmd): "open -a Music", "open -b com.apple.Music", ): - assert "blocked" in await run(cmd), f"should block: {cmd}" + assert "blocked" in (await run(cmd)).model_text, f"should block: {cmd}" for cmd in ("open notes.txt", "echo hi", "ls -la"): assert "blocked" not in await run(cmd), f"should allow: {cmd}" @@ -328,7 +328,7 @@ async def run(cmd): # Known accepted collateral: the security-broad ``\bosascript\b`` also # trips when 'osascript' is a mere argument. Pinned so a future narrowing # to command-position is a deliberate change, not an accident. - assert "blocked" in await run("grep osascript /var/log/system.log") + assert "blocked" in (await run("grep osascript /var/log/system.log")).model_text async def test_gui_automation_not_blocked_by_product_default(self, tmp_path): """Product default (no extra_deny_patterns): osascript is NOT blocked — diff --git a/tests/test_shell_approval.py b/tests/test_shell_approval.py new file mode 100644 index 000000000..cb4077450 --- /dev/null +++ b/tests/test_shell_approval.py @@ -0,0 +1,227 @@ +from __future__ import annotations + +import pytest + +from raven.agent.tools.base import ToolResult +from raven.agent.tools.shell import ExecTool +from raven.agent.tools.shell_policy import CommandDecision, ShellCommandPolicy +from raven.sandbox import ExecResult, SandboxExecutor + + +@pytest.fixture +def policy() -> ShellCommandPolicy: + return ShellCommandPolicy( + deny_patterns=[ + r"\brm\s+-[rf]{1,2}\b", + r"\b(mkfs|diskpart)\b", + ] + ) + + +@pytest.mark.parametrize( + "command", + [ + "pwd", + "ls -la", + "echo hello", + "find . -name '*.py'", + "printf 'rm file.txt'", + "echo 'find . -delete'", + ], +) +def test_safe_commands_are_allowed(policy: ShellCommandPolicy, command: str) -> None: + assert policy.evaluate(command) is CommandDecision.ALLOW + + +@pytest.mark.parametrize( + "command", + [ + "rm -r tmp", + "rm -f file.txt", + "rm -rf tmp", + "rm -fr tmp", + "echo ready && rm -rf tmp", + "mkfs /dev/test", + ], +) +def test_hard_denied_commands_cannot_be_approved(policy: ShellCommandPolicy, command: str) -> None: + assert policy.evaluate(command) is CommandDecision.HARD_DENY + + +@pytest.mark.parametrize( + "command", + [ + "rm file.txt", + "rm file1 file2", + "/bin/rm file.txt", + "rm --force file.txt", + "sudo rm file.txt", + "sudo -u root rm file.txt", + "sudo --user=root unlink file.txt", + "command unlink file.txt", + "MODE=test rm file.txt", + "env MODE=test rm file.txt", + "env -u MODE rm file.txt", + "unlink file.txt", + "find ./tmp -delete", + "echo ready && rm file.txt", + "printf done | unlink file.txt", + ], +) +def test_delete_commands_require_approval(policy: ShellCommandPolicy, command: str) -> None: + assert policy.evaluate(command) is CommandDecision.REQUIRE_APPROVAL + + +def test_hard_deny_wins_when_command_also_matches_approval(policy: ShellCommandPolicy) -> None: + assert policy.evaluate("unlink old.txt && rm -rf tmp") is CommandDecision.HARD_DENY + + +def test_matcher_failure_is_fail_closed(policy: ShellCommandPolicy) -> None: + def broken_matcher(command: str) -> bool: + raise RuntimeError("broken") + + policy.register_approval_matcher("broken", broken_matcher) + + assert policy.evaluate("echo harmless") is CommandDecision.HARD_DENY + + +class _RecordingExecutor(SandboxExecutor): + def __init__(self, *, sandboxed: bool) -> None: + self._sandboxed = sandboxed + self.commands: list[str] = [] + + @property + def is_sandboxed(self) -> bool: + return self._sandboxed + + async def exec(self, command: str, **kwargs) -> ExecResult: + self.commands.append(command) + return ExecResult(stdout="ok", stderr="", exit_code=0) + + +class _ApprovalResponder: + def __init__(self, answers: list[bool]) -> None: + self.answers = answers + self.requests: list[dict] = [] + + async def await_approval(self, **request) -> bool: + self.requests.append(request) + return self.answers.pop(0) + + +async def test_direct_delete_executes_once_after_approval(tmp_path) -> None: + executor = _RecordingExecutor(sandboxed=False) + responder = _ApprovalResponder([True]) + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-a") + tool.set_tool_call_id("call-a") + + result = await tool.execute("rm file.txt") + + assert "Exit code: 0" in result + assert executor.commands == ["rm file.txt"] + assert responder.requests == [ + { + "conversation_id": "session-a", + "turn_id": "turn-a", + "tool_call_id": "call-a", + "command": "rm file.txt", + "description": "Delete files using a shell command", + } + ] + + +async def test_direct_delete_without_responder_is_denied(tmp_path) -> None: + executor = _RecordingExecutor(sandboxed=False) + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + + result = await tool.execute("unlink file.txt") + + assert isinstance(result, ToolResult) + assert result.retryable is False + assert result.abort_action is True + assert "requires user approval" in result.model_text + assert "Do not retry" in result.model_text + assert executor.commands == [] + + +async def test_denied_command_is_not_prompted_again_in_same_turn(tmp_path) -> None: + executor = _RecordingExecutor(sandboxed=False) + responder = _ApprovalResponder([False]) + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-a") + + first = await tool.execute("find tmp -delete") + second = await tool.execute("find tmp -delete") + + assert isinstance(first, ToolResult) + assert first.retryable is False + assert first.abort_action is True + assert "denied" in first.model_text.lower() + assert isinstance(second, ToolResult) + assert "denied" in second.model_text.lower() + assert executor.commands == [] + assert len(responder.requests) == 1 + + +async def test_allow_once_does_not_cover_a_second_execution(tmp_path) -> None: + executor = _RecordingExecutor(sandboxed=False) + responder = _ApprovalResponder([True, False]) + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-a") + + first = await tool.execute("rm file.txt") + second = await tool.execute("rm file.txt") + + assert "Exit code: 0" in first + assert isinstance(second, ToolResult) + assert "denied" in second.model_text.lower() + assert executor.commands == ["rm file.txt"] + assert len(responder.requests) == 2 + + +async def test_new_turn_can_prompt_for_a_previously_denied_command(tmp_path) -> None: + executor = _RecordingExecutor(sandboxed=False) + responder = _ApprovalResponder([False, True]) + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-a") + + first = await tool.execute("unlink file.txt") + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-b") + second = await tool.execute("unlink file.txt") + + assert isinstance(first, ToolResult) + assert "denied" in first.model_text.lower() + assert "Exit code: 0" in second + assert executor.commands == ["unlink file.txt"] + assert len(responder.requests) == 2 + + +async def test_hard_denied_command_never_requests_approval(tmp_path) -> None: + executor = _RecordingExecutor(sandboxed=False) + responder = _ApprovalResponder([True]) + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-a") + + result = await tool.execute("rm -rf tmp") + + assert isinstance(result, ToolResult) + assert result.retryable is False + assert result.abort_action is True + assert "blocked" in result.model_text + assert "Do not retry" in result.model_text + assert responder.requests == [] + assert executor.commands == [] + + +async def test_sandboxed_delete_skips_approval_and_deny_policy(tmp_path) -> None: + executor = _RecordingExecutor(sandboxed=True) + responder = _ApprovalResponder([False]) + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + tool.start_approval_turn(responder, conversation_id="session-a", turn_id="turn-a") + + result = await tool.execute("rm -rf tmp") + + assert "Exit code: 0" in result + assert responder.requests == [] + assert executor.commands == ["rm -rf tmp"] diff --git a/tests/test_tool_registry_execute.py b/tests/test_tool_registry_execute.py index da4740215..2322a9503 100644 --- a/tests/test_tool_registry_execute.py +++ b/tests/test_tool_registry_execute.py @@ -18,9 +18,18 @@ class _Split(Tool): - def __init__(self, model_text: str, display_text: str | None) -> None: + def __init__( + self, + model_text: str, + display_text: str | None, + *, + retryable: bool = True, + abort_action: bool = False, + ) -> None: self._model_text = model_text self._display_text = display_text + self._retryable = retryable + self._abort_action = abort_action @property def name(self) -> str: @@ -35,7 +44,12 @@ def parameters(self) -> dict: return {"type": "object", "properties": {}, "required": []} async def execute(self, **kwargs) -> ToolResult: - return ToolResult(model_text=self._model_text, display_text=self._display_text) + return ToolResult( + model_text=self._model_text, + display_text=self._display_text, + retryable=self._retryable, + abort_action=self._abort_action, + ) class _Plain(Tool): @@ -98,6 +112,24 @@ async def test_error_prefixed_tool_result_keeps_hint_and_display(): assert result.display_text == "asked -> nothing" # type: ignore[attr-defined] +@pytest.mark.asyncio +async def test_non_retryable_error_omits_hint_and_preserves_abort_signal(): + reg = _registry( + _Split( + "Error: denied by safety policy", + None, + retryable=False, + abort_action=True, + ) + ) + + result = await reg.execute("split", {}) + + assert "try a different approach" not in result + assert result.retryable is False # type: ignore[attr-defined] + assert result.abort_action is True # type: ignore[attr-defined] + + @pytest.mark.asyncio async def test_missing_tool_still_returns_a_plain_string(): reg = _registry(_Plain()) diff --git a/tests/test_tui_rpc_approval.py b/tests/test_tui_rpc_approval.py new file mode 100644 index 000000000..fdfa75d8c --- /dev/null +++ b/tests/test_tui_rpc_approval.py @@ -0,0 +1,65 @@ +from __future__ import annotations + +import asyncio + +from raven.tui_rpc.approval_broker import ApprovalBroker +from raven.tui_rpc.dispatcher import Dispatcher +from raven.tui_rpc.methods.approval import approval_respond, register_approval_methods + + +async def test_approval_respond_resolves_matching_request() -> None: + frames: list[dict] = [] + + async def send(frame: dict) -> None: + frames.append(frame) + + broker = ApprovalBroker(send) + waiting = asyncio.create_task( + broker.await_approval( + conversation_id="session-a", + turn_id="turn-a", + tool_call_id="call-a", + command="rm file.txt", + description="Delete files", + ) + ) + for _ in range(20): + if frames: + break + await asyncio.sleep(0) + approval_id = frames[0]["params"]["approval_id"] + + result = await approval_respond( + { + "approval_id": approval_id, + "session_id": "session-a", + "choice": "allow", + }, + approval_broker=broker, + ) + + assert result == {"ok": True} + assert await waiting is True + + +async def test_approval_respond_rejects_missing_fields() -> None: + async def send(frame: dict) -> None: + pass + + broker = ApprovalBroker(send) + + assert await approval_respond({}, approval_broker=broker) == {"ok": False} + assert await approval_respond( + {"approval_id": "missing", "session_id": "session-a", "choice": "always"}, + approval_broker=broker, + ) == {"ok": False} + + +def test_register_approval_methods_adds_real_handler() -> None: + async def send(frame: dict) -> None: + pass + + dispatcher = Dispatcher() + register_approval_methods(dispatcher, approval_broker=ApprovalBroker(send)) + + assert "approval.respond" in dispatcher.methods() diff --git a/tests/test_tui_rpc_spine.py b/tests/test_tui_rpc_spine.py index bd769b056..ee82fce6d 100644 --- a/tests/test_tui_rpc_spine.py +++ b/tests/test_tui_rpc_spine.py @@ -1,6 +1,8 @@ from dataclasses import replace from raven.agent.tools.message import MessageTool +from raven.agent.tools.shell import ExecTool +from raven.sandbox import ExecResult, SandboxExecutor from raven.spine import ( ChatType, MediaOut, @@ -70,6 +72,40 @@ async def run_turn( return TurnOutcome(usage=Usage(0, 0, 0), explicit_reply=True) +class _DirectRecordingExecutor(SandboxExecutor): + def __init__(self) -> None: + self.commands: list[str] = [] + + @property + def is_sandboxed(self) -> bool: + return False + + async def exec(self, command: str, **kwargs) -> ExecResult: + self.commands.append(command) + return ExecResult(stdout="ok", stderr="", exit_code=0) + + +class _ApprovalResponder: + def __init__(self, answer: bool) -> None: + self.answer = answer + self.requests: list[dict] = [] + + async def await_approval(self, **request) -> bool: + self.requests.append(request) + return self.answer + + +class _ApprovalRunLoop: + def __init__(self, tool: ExecTool) -> None: + self.tools = {"exec": tool} + self.result = "" + + async def run_turn(self, req, emit, drain, **kwargs) -> TurnOutcome: + self.tools["exec"].set_tool_call_id("call-a") + self.result = await self.tools["exec"].execute("rm file.txt") + return TurnOutcome(usage=Usage(0, 0, 0), explicit_reply=True) + + def _collect(): events: list = [] @@ -111,6 +147,51 @@ async def test_runner_drives_run_turn_and_stashes_rich_usage(): assert outcome.explicit_reply is True +async def test_user_turn_receives_tui_approval_capability(tmp_path): + executor = _DirectRecordingExecutor() + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + loop = _ApprovalRunLoop(tool) + responder = _ApprovalResponder(True) + runner = TuiTurnRunner( + loop, + FakeEmitter(), + {}, + {"tui:c1": "turn-a"}, + {}, + approval_responder=responder, + ) + req = TurnRequest(origin=Origin.USER, source=_src(), text="delete", conversation="tui:c1") + _events, emit = _collect() + + await runner.run(req, emit, lambda: []) + + assert executor.commands == ["rm file.txt"] + assert responder.requests[0]["turn_id"] == "turn-a" + + +async def test_cron_turn_does_not_receive_tui_approval_capability(tmp_path): + executor = _DirectRecordingExecutor() + tool = ExecTool(executor=executor, working_dir=str(tmp_path)) + loop = _ApprovalRunLoop(tool) + responder = _ApprovalResponder(True) + runner = TuiTurnRunner( + loop, + FakeEmitter(), + {}, + {"cron:c1": "turn-a"}, + {}, + approval_responder=responder, + ) + req = TurnRequest(origin=Origin.CRON, source=_src(), text="delete", conversation="cron:c1") + _events, emit = _collect() + + await runner.run(req, emit, lambda: []) + + assert "requires user approval" in loop.result.model_text + assert executor.commands == [] + assert responder.requests == [] + + async def test_runner_emits_eve22_synthetic_tool_complete_when_message_tool_fired(): message_tool = MessageTool() loop = _RunTurnLoop(tools={"message": message_tool}) diff --git a/tests/test_tui_rpc_stubs.py b/tests/test_tui_rpc_stubs.py index d8a2346da..31a22dd27 100644 --- a/tests/test_tui_rpc_stubs.py +++ b/tests/test_tui_rpc_stubs.py @@ -45,8 +45,7 @@ ("skills.reload", "skills.reload not supported", True), # reload.env ("reload.env", "reload.env not supported", True), - # approval / sudo / secret response (3) - ("approval.respond", "approval.respond not supported", True), + # sudo / secret response ("sudo.respond", "sudo.respond not supported", True), ("secret.respond", "secret.respond not supported", True), # NOTE: ``commands.catalog`` was promoted to a real handler in diff --git a/ui-tui/src/__tests__/approvalRoundTrip.test.ts b/ui-tui/src/__tests__/approvalRoundTrip.test.ts new file mode 100644 index 000000000..3f1689040 --- /dev/null +++ b/ui-tui/src/__tests__/approvalRoundTrip.test.ts @@ -0,0 +1,179 @@ +import { render } from 'ink-testing-library' +import React from 'react' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import type { Msg } from '../types.js' + +import { createGatewayEventHandler } from '../app/createGatewayEventHandler.js' +import { getOverlayState, resetOverlayState } from '../app/overlayStore.js' +import { resetTurnState } from '../app/turnStore.js' +import { resetUiState } from '../app/uiStore.js' +import { ApprovalPrompt } from '../components/prompts.js' +import { + APPROVAL_OPTIONS, + approvalRemainingSeconds, + approvalResponseAccepted, + buildApprovalRespond +} from '../lib/approval.js' +import { DEFAULT_THEME } from '../theme.js' + +const ref = (current: T) => ({ current }) + +const buildCtx = (appended: Msg[]) => + ({ + composer: { + dequeue: () => undefined, + queueEditRef: ref(null), + sendQueued: () => undefined, + setInput: () => undefined + }, + gateway: { + gw: { request: () => undefined }, + rpc: async () => null + }, + session: { + STARTUP_RESUME_ID: '', + colsRef: ref(80), + newSession: () => undefined, + resetSession: () => undefined, + resumeById: () => undefined, + setCatalog: () => undefined + }, + submission: { submitRef: ref(() => undefined) }, + system: { bellOnComplete: false, sys: () => undefined }, + transcript: { + appendMessage: (msg: Msg) => appended.push(msg), + panel: () => undefined, + setHistoryItems: () => undefined + }, + voice: { + setProcessing: () => undefined, + setRecording: () => undefined, + setVoiceEnabled: () => undefined + } + }) as any + +describe('approval round-trip', () => { + beforeEach(() => { + resetOverlayState() + resetTurnState() + resetUiState() + }) + + it('stores the approval id from the runtime notification', () => { + const onEvent = createGatewayEventHandler(buildCtx([])) + + onEvent({ + payload: { + approval_id: 'approval-a', + command: 'rm file.txt', + conversation_id: 'session-a', + description: 'Delete files', + expires_at: 1735689630 + }, + session_id: 'session-a', + type: 'approval.request' + } as any) + + expect(getOverlayState().approval).toEqual({ + approvalId: 'approval-a', + command: 'rm file.txt', + conversationId: 'session-a', + description: 'Delete files', + expiresAt: 1735689630000 + }) + }) + + it('clears only the matching approval when runtime closes it', () => { + const onEvent = createGatewayEventHandler(buildCtx([])) + + onEvent({ + payload: { + approval_id: 'approval-a', + command: 'rm file.txt', + conversation_id: 'session-a', + description: 'Delete files', + expires_at: 1735689630 + }, + session_id: 'session-a', + type: 'approval.request' + } as any) + onEvent({ + payload: { + approval_id: 'approval-b', + conversation_id: 'session-a', + reason: 'timeout' + }, + session_id: 'session-a', + type: 'approval.closed' + } as any) + + expect(getOverlayState().approval?.approvalId).toBe('approval-a') + + onEvent({ + payload: { + approval_id: 'approval-a', + conversation_id: 'session-a', + reason: 'timeout' + }, + session_id: 'session-a', + type: 'approval.closed' + } as any) + + expect(getOverlayState().approval).toBeNull() + }) + + it('offers only allow once and deny', () => { + expect(APPROVAL_OPTIONS).toEqual([ + { choice: 'allow', label: 'Allow once' }, + { choice: 'deny', label: 'Deny' } + ]) + }) + + it('builds a response bound to the approval and session', () => { + expect(buildApprovalRespond('approval-a', 'session-a', 'deny')).toEqual({ + approval_id: 'approval-a', + choice: 'deny', + session_id: 'session-a' + }) + }) + + it('accepts only an explicit ok response', () => { + expect(approvalResponseAccepted({ ok: true })).toBe(true) + expect(approvalResponseAccepted({ ok: false })).toBe(false) + expect(approvalResponseAccepted(null)).toBe(false) + }) + + it('derives the visible countdown from the runtime deadline', () => { + expect(approvalRemainingSeconds(31_000, 1_000)).toBe(30) + expect(approvalRemainingSeconds(1_001, 1_000)).toBe(1) + expect(approvalRemainingSeconds(999, 1_000)).toBe(0) + }) + + it('auto-denies and removes the visible choice at the runtime deadline', async () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + const onChoice = vi.fn() + const rendered = render( + React.createElement(ApprovalPrompt, { + onChoice, + req: { + approvalId: 'approval-a', + command: 'rm file.txt', + conversationId: 'session-a', + description: 'Delete files', + expiresAt: 2_000 + }, + t: DEFAULT_THEME + }) + ) + + try { + await vi.advanceTimersByTimeAsync(1_000) + expect(onChoice).toHaveBeenCalledWith('deny') + } finally { + rendered.unmount() + vi.useRealTimers() + } + }) +}) diff --git a/ui-tui/src/app/createGatewayEventHandler.ts b/ui-tui/src/app/createGatewayEventHandler.ts index 0020db345..6bb97bfb1 100644 --- a/ui-tui/src/app/createGatewayEventHandler.ts +++ b/ui-tui/src/app/createGatewayEventHandler.ts @@ -21,7 +21,7 @@ import { rpcErrorMessage } from '../lib/rpc.js' import { topLevelSubagents } from '../lib/subagentTree.js' import { formatToolCall, stripAnsi } from '../lib/text.js' import { applyDelegationStatus, getDelegationState } from './delegationStore.js' -import { patchOverlayState } from './overlayStore.js' +import { getOverlayState, patchOverlayState } from './overlayStore.js' import { turnController } from './turnController.js' import { applySkinTheme, getUiState, patchUiState } from './uiStore.js' @@ -524,11 +524,31 @@ export function createGatewayEventHandler(ctx: GatewayEventHandlerContext): (ev: case 'approval.request': { const description = String(ev.payload.description ?? 'dangerous command') - patchOverlayState({ approval: { command: String(ev.payload.command ?? ''), description } }) + patchOverlayState({ + approval: { + approvalId: String(ev.payload.approval_id ?? ''), + command: String(ev.payload.command ?? ''), + conversationId: String(ev.payload.conversation_id ?? ''), + description, + expiresAt: Number(ev.payload.expires_at) * 1000 + } + }) setStatus('approval needed') return } + case 'approval.closed': { + const approval = getOverlayState().approval + + // Close notifications can be delayed past a subsequent request. Match + // the opaque id so an old backend timeout never clears the new overlay. + if (approval?.approvalId === String(ev.payload.approval_id ?? '')) { + patchOverlayState({ approval: null }) + setStatus(statusFromBusy()) + } + + return + } case 'confirm.request': patchOverlayState({ diff --git a/ui-tui/src/app/useInputHandlers.ts b/ui-tui/src/app/useInputHandlers.ts index b4292ee74..708d8ca5a 100644 --- a/ui-tui/src/app/useInputHandlers.ts +++ b/ui-tui/src/app/useInputHandlers.ts @@ -17,6 +17,7 @@ import type { import type { InputHandlerContext, InputHandlerResult } from './interfaces.js' import { TYPING_IDLE_MS } from '../config/timing.js' +import { buildApprovalRespond } from '../lib/approval.js' import { isAction, isCopyShortcut, isMac, isVoiceToggleKey } from '../lib/platform.js' import { computePrecisionWheelStep, initPrecisionWheel } from '../lib/precisionWheel.js' import { computeWheelStep, initWheelAccelForHost } from '../lib/wheelAccel.js' @@ -138,9 +139,18 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult { } if (overlay.approval) { - return gateway - .rpc('approval.respond', { choice: 'deny', session_id: getUiState().sid }) - .then(r => r && (patchOverlayState({ approval: null }), patchTurnState({ outcome: 'denied' }))) + const approval = overlay.approval + // Ctrl+C is a local fail-closed action. Clear the overlay before awaiting + // RPC so a disconnected backend cannot leave a stale prompt accepting a + // later keypress. The captured ids still let the best-effort response + // resolve a live backend request when the connection is healthy. + patchOverlayState({ approval: null }) + patchTurnState({ outcome: 'denied' }) + + return gateway.rpc( + 'approval.respond', + buildApprovalRespond(approval.approvalId, approval.conversationId, 'deny') + ) } if (overlay.sudo) { diff --git a/ui-tui/src/app/useMainApp.ts b/ui-tui/src/app/useMainApp.ts index 1f68019e5..c3c352920 100644 --- a/ui-tui/src/app/useMainApp.ts +++ b/ui-tui/src/app/useMainApp.ts @@ -8,6 +8,7 @@ import { useStore } from '@nanostores/react' import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import type { + ApprovalRespondResponse, ClarifyRespondResponse, ClipboardPasteResponse, GatewayEvent, @@ -23,6 +24,7 @@ import { fmtCwdBranch, shortCwd } from '../domain/paths.js' import { type GatewayClient } from '../gatewayClientStub.js' import { useGitBranch } from '../hooks/useGitBranch.js' import { useVirtualHistory } from '../hooks/useVirtualHistory.js' +import { approvalResponseAccepted, buildApprovalRespond } from '../lib/approval.js' import { buildConfirmRespond } from '../lib/confirmCountdown.js' import { composerPromptWidth } from '../lib/inputMetrics.js' import { appendTranscriptMessage } from '../lib/messages.js' @@ -740,13 +742,42 @@ export function useMainApp(gw: GatewayClient, rpcClient?: ChatStreamRpcClient) { ) const answerApproval = useCallback( - (choice: string) => - respondWith('approval.respond', { choice, session_id: ui.sid }, () => { + (choice: string) => { + const approval = overlay.approval + + if (!approval) { + return + } + + if (choice === 'deny') { + // Denial changes no host state, so the frontend can commit it locally + // before the RPC round-trip. Approval is different: the overlay remains + // until the backend confirms that the exact request was still live. + // This asymmetry prevents stale UI from granting authority while also + // ensuring a network failure cannot keep a rejected prompt interactive. + patchOverlayState({ approval: null }) + patchTurnState({ outcome: 'denied' }) + patchUiState({ status: 'running…' }) + } + + rpc( + 'approval.respond', + buildApprovalRespond(approval.approvalId, approval.conversationId, choice) + ).then(response => { + if (choice === 'deny') { + return + } + + if (!approvalResponseAccepted(response)) { + return + } + patchOverlayState({ approval: null }) patchTurnState({ outcome: choice === 'deny' ? 'denied' : `approved (${choice})` }) patchUiState({ status: 'running…' }) - }), - [respondWith, ui.sid] + }) + }, + [overlay.approval, rpc] ) const answerSudo = useCallback( diff --git a/ui-tui/src/components/prompts.tsx b/ui-tui/src/components/prompts.tsx index 513157622..fa79c7929 100644 --- a/ui-tui/src/components/prompts.tsx +++ b/ui-tui/src/components/prompts.tsx @@ -9,36 +9,60 @@ import { useEffect, useRef, useState } from 'react' import type { Theme } from '../theme.js' import type { ApprovalReq, ClarifyReq, ConfirmReq } from '../types.js' +import { APPROVAL_OPTIONS, approvalRemainingSeconds } from '../lib/approval.js' import { CONFIRM_COUNTDOWN_SECONDS, tickCountdown } from '../lib/confirmCountdown.js' import { isMac } from '../lib/platform.js' import { TextInput } from './textInput.js' -const OPTS = ['once', 'session', 'always', 'deny'] as const -const LABELS = { always: 'Always allow', deny: 'Deny', once: 'Allow once', session: 'Allow this session' } as const const CMD_PREVIEW_LINES = 10 export function ApprovalPrompt({ onChoice, req, t }: ApprovalPromptProps) { const [sel, setSel] = useState(0) + const [remainingSeconds, setRemainingSeconds] = useState(() => approvalRemainingSeconds(req.expiresAt)) + const expired = useRef(false) + + useEffect(() => { + expired.current = false + + const update = () => { + // The runtime sends an absolute wall-clock deadline rather than a duration. + // Recomputing from that value matters when rendering is delayed or the + // terminal process is briefly suspended: mounting the component must not + // accidentally grant a fresh approval window. The ref makes auto-denial + // edge-triggered while the interval continues to render the 0-second state. + const remaining = approvalRemainingSeconds(req.expiresAt) + setRemainingSeconds(remaining) + if (remaining === 0 && !expired.current) { + expired.current = true + onChoice('deny') + } + } + + update() + const timer = setInterval(update, 250) + + return () => clearInterval(timer) + }, [onChoice, req.approvalId, req.expiresAt]) useInput((ch, key) => { if (key.upArrow && sel > 0) { setSel(s => s - 1) } - if (key.downArrow && sel < OPTS.length - 1) { + if (key.downArrow && sel < APPROVAL_OPTIONS.length - 1) { setSel(s => s + 1) } const n = parseInt(ch, 10) - if (n >= 1 && n <= OPTS.length) { - onChoice(OPTS[n - 1]!) + if (n >= 1 && n <= APPROVAL_OPTIONS.length) { + onChoice(APPROVAL_OPTIONS[n - 1]!.choice) return } if (key.return) { - onChoice(OPTS[sel]!) + onChoice(APPROVAL_OPTIONS[sel]!.choice) } }) @@ -68,16 +92,18 @@ export function ApprovalPrompt({ onChoice, req, t }: ApprovalPromptProps) { - {OPTS.map((o, i) => ( - + {APPROVAL_OPTIONS.map((option, i) => ( + {sel === i ? '▸ ' : ' '} - {i + 1}. {LABELS[o]} + {i + 1}. {option.label} ))} - ↑/↓ select · Enter confirm · 1-4 quick pick · Ctrl+C deny + + ↑/↓ select · Enter confirm · 1-2 quick pick · Ctrl+C deny · expires in {remainingSeconds}s + ) } diff --git a/ui-tui/src/demo/gallery.tsx b/ui-tui/src/demo/gallery.tsx index 0a11636ec..c5804f87f 100644 --- a/ui-tui/src/demo/gallery.tsx +++ b/ui-tui/src/demo/gallery.tsx @@ -36,8 +36,11 @@ if (!process.stdin.isTTY) { // ── Mock data ──────────────────────────────────────────────────────── const approvalReq: ApprovalReq = { + approvalId: 'demo-approval', command: 'rm -rf dist\nnpm ci\nnpm run build', - description: 'run a shell command' + conversationId: 'demo-session', + description: 'run a shell command', + expiresAt: Date.now() + 30_000 } const clarifyChoicesReq: ClarifyReq = { diff --git a/ui-tui/src/gatewayTypes.ts b/ui-tui/src/gatewayTypes.ts index c73e74a6b..612d003bf 100644 --- a/ui-tui/src/gatewayTypes.ts +++ b/ui-tui/src/gatewayTypes.ts @@ -529,7 +529,28 @@ export type GatewayEvent = session_id?: string type: 'clarify.request' } - | { payload: { command: string; description: string }; session_id?: string; type: 'approval.request' } + | { + payload: { + approval_id: string + command: string + conversation_id: string + description: string + expires_at: number + tool_call_id: string + turn_id: string + } + session_id?: string + type: 'approval.request' + } + | { + payload: { + approval_id: string + conversation_id: string + reason: string + } + session_id?: string + type: 'approval.closed' + } | { payload: { request_id: string }; session_id?: string; type: 'sudo.request' } | { payload: { env_var: string; prompt: string; request_id: string }; session_id?: string; type: 'secret.request' } | { payload: { default: boolean; prompt: string; request_id: string }; session_id?: string; type: 'confirm.request' } diff --git a/ui-tui/src/lib/approval.ts b/ui-tui/src/lib/approval.ts new file mode 100644 index 000000000..3e09a1058 --- /dev/null +++ b/ui-tui/src/lib/approval.ts @@ -0,0 +1,26 @@ +// Approval is intentionally narrower than a generic prompt: the user may +// authorize this exact request once or deny it. Centralizing the protocol +// helpers keeps UI call sites from introducing persistent/session authority. +export const APPROVAL_OPTIONS = [ + { choice: 'allow', label: 'Allow once' }, + { choice: 'deny', label: 'Deny' } +] as const + +export const buildApprovalRespond = (approvalId: string, sessionId: string, choice: string) => { + // Echo both opaque identities so a delayed response cannot resolve a newer + // request that happens to display the same command. + return { + approval_id: approvalId, + choice, + session_id: sessionId + } +} + +// Missing or malformed acknowledgements fail closed; only the broker's +// explicit acceptance means that the command was authorized. +export const approvalResponseAccepted = (response: null | { ok?: boolean }) => response?.ok === true + +// The runtime supplies one absolute deadline. Deriving the countdown from it +// avoids drift when event delivery or React rendering is delayed. +export const approvalRemainingSeconds = (expiresAt: number, now = Date.now()) => + Math.max(0, Math.ceil((expiresAt - now) / 1000)) diff --git a/ui-tui/src/types.ts b/ui-tui/src/types.ts index c94ccfc3a..3d0de63b4 100644 --- a/ui-tui/src/types.ts +++ b/ui-tui/src/types.ts @@ -92,8 +92,12 @@ export interface DelegationStatus { } export interface ApprovalReq { + approvalId: string command: string + conversationId: string description: string + // Absolute Unix deadline in milliseconds; the gateway wire value is seconds. + expiresAt: number } export interface ConfirmReq { From deb4f595c7a091f6ac9778d437462f03d8827f26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B1=9F=E5=9B=BD=E5=BA=86?= Date: Thu, 30 Jul 2026 18:26:55 +0800 Subject: [PATCH 2/4] chore: format shell approval code --- raven/agent/tools/shell.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/raven/agent/tools/shell.py b/raven/agent/tools/shell.py index 5183c6e0f..bec11561b 100644 --- a/raven/agent/tools/shell.py +++ b/raven/agent/tools/shell.py @@ -222,9 +222,7 @@ async def _request_approval(self, command: str) -> ToolResult | None: if digest in turn.denied_digests: return self._terminal_error("Error: User denied this command earlier in the current turn") if turn.responder is None or not turn.conversation_id: - return self._terminal_error( - "Error: Command requires user approval, but this turn is not interactive" - ) + return self._terminal_error("Error: Command requires user approval, but this turn is not interactive") approved = await turn.responder.await_approval( conversation_id=turn.conversation_id, turn_id=turn.turn_id, From 72292cfde4cb94b3be21dfef1516eadc8afd73ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B1=9F=E5=9B=BD=E5=BA=86?= Date: Fri, 31 Jul 2026 11:50:34 +0800 Subject: [PATCH 3/4] fix: enforce terminal shell decisions in subagents --- raven/agent/subagent/manager.py | 20 ++++++++- raven/agent/tools/shell.py | 1 - raven/agent/tools/shell_policy.py | 53 ++++++++++++++++++++++-- tests/test_shell_approval.py | 19 +++++++++ tests/test_subagent_manager.py | 68 +++++++++++++++++++++++++++++++ 5 files changed, 155 insertions(+), 6 deletions(-) diff --git a/raven/agent/subagent/manager.py b/raven/agent/subagent/manager.py index a2d519470..1d17e90e7 100644 --- a/raven/agent/subagent/manager.py +++ b/raven/agent/subagent/manager.py @@ -24,6 +24,10 @@ # One hour: a runaway re-injection loop fires fast and trips the limit quickly, # while legitimate spawns spread over time and age out before it bites. _SPAWN_WINDOW_SECONDS = 3600 +_ABORTED_ACTION_RESULT = ( + "The subtask stopped because a safety decision terminated the requested operation. " + "No alternative method was attempted." +) class SubagentManager: @@ -182,6 +186,7 @@ async def _run_subagent_inner( max_iterations = 15 iteration = 0 final_result: str | None = None + final_status = "ok" while iteration < max_iterations: iteration += 1 @@ -220,6 +225,17 @@ async def _run_subagent_inner( "content": wrap_untrusted(result, source=tool_call.name), } ) + if getattr(result, "abort_action", False): + # Subagents must enforce the same terminal safety + # signal as the main loop. Returning to the model + # would let it translate a rejected operation into + # another command or interpreter, while continuing + # this batch would execute already-proposed siblings. + final_result = _ABORTED_ACTION_RESULT + final_status = "error" + break + if final_result is not None: + break else: final_result = response.content break @@ -227,8 +243,8 @@ async def _run_subagent_inner( if final_result is None: final_result = "Task completed but no final response was generated." - logger.info("Subagent [{}] completed successfully", task_id) - await self._announce_result(task_id, label, task, final_result, origin, "ok") + logger.info("Subagent [{}] finished with status {}", task_id, final_status) + await self._announce_result(task_id, label, task, final_result, origin, final_status) except Exception as e: error_msg = f"Error: {str(e)}" diff --git a/raven/agent/tools/shell.py b/raven/agent/tools/shell.py index bec11561b..67cab4546 100644 --- a/raven/agent/tools/shell.py +++ b/raven/agent/tools/shell.py @@ -71,7 +71,6 @@ def __init__( r"\b(mkfs|diskpart)\b", # disk operations r"\bdd\s+if=", # dd r">\s*/dev/sd", # write to disk - r"\b(shutdown|reboot|poweroff)\b", # system power r":\(\)\s*\{.*\};\s*:", # fork bomb ] # Operator-configurable extras (tools.exec.extra_deny_patterns), appended diff --git a/raven/agent/tools/shell_policy.py b/raven/agent/tools/shell_policy.py index a2c7ac9c5..72e32e2d3 100644 --- a/raven/agent/tools/shell_policy.py +++ b/raven/agent/tools/shell_policy.py @@ -23,6 +23,7 @@ _WRAPPER_OPTIONS_WITH_VALUE = { "command": frozenset(), "env": frozenset({"-u", "--unset", "-C", "--chdir", "-S", "--split-string"}), + "nohup": frozenset(), "sudo": frozenset( { "-C", @@ -47,6 +48,10 @@ ), } _ASSIGNMENT = re.compile(r"[A-Za-z_][A-Za-z0-9_]*=.*", re.DOTALL) +_COMMAND_BOUNDARIES = frozenset(";&|\n(){}") +_SHELL_COMMAND_WRAPPERS = frozenset({"bash", "dash", "ksh", "sh", "zsh"}) +_SYSTEM_POWER_COMMANDS = frozenset({"poweroff", "reboot", "shutdown"}) +_MAX_EMBEDDED_SHELL_DEPTH = 4 class CommandDecision(StrEnum): @@ -65,12 +70,15 @@ def _command_segments(command: str) -> Iterator[list[str]]: or reproduce the full shell grammar. """ - lexer = shlex.shlex(command, posix=True, punctuation_chars=";&|") + lexer = shlex.shlex(command, posix=True, punctuation_chars=";&|(){}\n") lexer.commenters = "" + # Newlines must remain visible as command boundaries. Quoted newlines are + # still returned inside their quoted token and therefore do not split it. + lexer.whitespace = " \t\r" lexer.whitespace_split = True segment: list[str] = [] for token in lexer: - if token and all(char in ";&|" for char in token): + if token and all(char in _COMMAND_BOUNDARIES for char in token): if segment: yield segment segment = [] @@ -80,7 +88,18 @@ def _command_segments(command: str) -> Iterator[list[str]]: yield segment -def _matches_delete_command(command: str) -> bool: +def _embedded_shell_command(segment: list[str]) -> str | None: + """Return the command string supplied to a recognized shell ``-c``.""" + + if not segment or PurePath(segment[0]).name not in _SHELL_COMMAND_WRAPPERS: + return None + for index, token in enumerate(segment[1:], start=1): + if token.startswith("-") and not token.startswith("--") and "c" in token[1:] and index + 1 < len(segment): + return segment[index + 1] + return None + + +def _matches_delete_command(command: str, *, _depth: int = 0) -> bool: """Recognize direct file-deletion commands after wrapper normalization.""" for segment in _command_segments(command): @@ -92,6 +111,32 @@ def _matches_delete_command(command: str) -> bool: return True if executable == "find" and "-delete" in segment[1:]: return True + embedded = _embedded_shell_command(segment) + if ( + embedded is not None + and _depth < _MAX_EMBEDDED_SHELL_DEPTH + and _matches_delete_command(embedded, _depth=_depth + 1) + ): + return True + return False + + +def _matches_system_power_command(command: str, *, _depth: int = 0) -> bool: + """Recognize power-control executables without matching argument text.""" + + for segment in _command_segments(command): + segment = _unwrap_command_wrappers(segment) + if not segment: + continue + if PurePath(segment[0]).name in _SYSTEM_POWER_COMMANDS: + return True + embedded = _embedded_shell_command(segment) + if ( + embedded is not None + and _depth < _MAX_EMBEDDED_SHELL_DEPTH + and _matches_system_power_command(embedded, _depth=_depth + 1) + ): + return True return False @@ -147,6 +192,8 @@ def evaluate(self, command: str) -> CommandDecision: if any(pattern.search(command) for pattern in self._deny_patterns): return CommandDecision.HARD_DENY try: + if _matches_system_power_command(command): + return CommandDecision.HARD_DENY if any(matcher(command) for _, matcher in self._approval_matchers): return CommandDecision.REQUIRE_APPROVAL except Exception: diff --git a/tests/test_shell_approval.py b/tests/test_shell_approval.py index cb4077450..cf0ec7190 100644 --- a/tests/test_shell_approval.py +++ b/tests/test_shell_approval.py @@ -27,6 +27,8 @@ def policy() -> ShellCommandPolicy: "find . -name '*.py'", "printf 'rm file.txt'", "echo 'find . -delete'", + "git grep -n shutdown", + "grep -rn reboot /var/log", ], ) def test_safe_commands_are_allowed(policy: ShellCommandPolicy, command: str) -> None: @@ -42,6 +44,9 @@ def test_safe_commands_are_allowed(policy: ShellCommandPolicy, command: str) -> "rm -fr tmp", "echo ready && rm -rf tmp", "mkfs /dev/test", + "shutdown now", + "sudo -n reboot", + 'bash -c "poweroff"', ], ) def test_hard_denied_commands_cannot_be_approved(policy: ShellCommandPolicy, command: str) -> None: @@ -66,6 +71,15 @@ def test_hard_denied_commands_cannot_be_approved(policy: ShellCommandPolicy, com "find ./tmp -delete", "echo ready && rm file.txt", "printf done | unlink file.txt", + "cd /tmp\nrm file.txt", + "cd /tmp\r\nunlink file.txt", + "(rm file.txt)", + "{ rm file.txt; }", + "echo $(rm file.txt)", + "nohup rm file.txt &", + 'bash -c "rm file.txt"', + 'bash --rcfile setup.sh -c "rm file.txt"', + 'sh -lc "find tmp -delete"', ], ) def test_delete_commands_require_approval(policy: ShellCommandPolicy, command: str) -> None: @@ -85,6 +99,11 @@ def broken_matcher(command: str) -> bool: assert policy.evaluate("echo harmless") is CommandDecision.HARD_DENY +@pytest.mark.parametrize("command", ["echo 'unterminated", "echo trailing\\"]) +def test_shell_parse_failure_is_fail_closed(policy: ShellCommandPolicy, command: str) -> None: + assert policy.evaluate(command) is CommandDecision.HARD_DENY + + class _RecordingExecutor(SandboxExecutor): def __init__(self, *, sandboxed: bool) -> None: self._sandboxed = sandboxed diff --git a/tests/test_subagent_manager.py b/tests/test_subagent_manager.py index f9283b72f..1f65a772c 100644 --- a/tests/test_subagent_manager.py +++ b/tests/test_subagent_manager.py @@ -17,6 +17,8 @@ from raven.agent.subagent import manager as manager_mod from raven.agent.subagent.manager import SubagentManager from raven.config.schema import AgentDefaults +from raven.providers.base import LLMResponse, ToolCallRequest +from raven.sandbox import ExecResult, SandboxExecutor class _StubProvider: @@ -32,6 +34,41 @@ async def __aexit__(self, *exc: object) -> bool: return False +class _RecordingExecutor(SandboxExecutor): + def __init__(self) -> None: + self.commands: list[str] = [] + + @property + def is_sandboxed(self) -> bool: + return False + + async def exec(self, command: str, **kwargs) -> ExecResult: + self.commands.append(command) + return ExecResult(stdout="ok", stderr="", exit_code=0) + + +class _DeleteRetryProvider(_StubProvider): + def __init__(self) -> None: + self.responses = [ + LLMResponse( + content="", + tool_calls=[ + ToolCallRequest(id="call-a", name="exec", arguments={"command": "rm file.txt"}), + ToolCallRequest( + id="call-b", + name="exec", + arguments={"command": 'bash -c "rm file.txt"'}, + ), + ], + finish_reason="tool_calls", + ), + LLMResponse(content="Retried through a shell wrapper.", finish_reason="stop"), + ] + + async def chat_with_retry(self, **kwargs) -> LLMResponse: + return self.responses.pop(0) + + async def _settle(predicate, *, tries: int = 2000) -> None: for _ in range(tries): if predicate(): @@ -90,6 +127,37 @@ async def test_gate_of_one_serializes_subagents(monkeypatch): assert peak == 1 +async def test_subagent_stops_after_terminal_shell_decision(monkeypatch, tmp_path): + provider = _DeleteRetryProvider() + manager = SubagentManager(provider=provider, workspace=tmp_path) + executor = _RecordingExecutor() + announcements: list[dict[str, str]] = [] + + monkeypatch.setattr(manager, "_build_subagent_prompt", lambda: "system") + + async def _capture_announcement(task_id, label, task, result, origin, status) -> None: + announcements.append({"result": result, "status": status}) + + monkeypatch.setattr(manager, "_announce_result", _capture_announcement) + + await manager._run_subagent_inner( + "task-a", + "delete file.txt", + "delete", + {"channel": "tui", "chat_id": "default", "session_key": "tui:session-a"}, + executor, + ) + + assert executor.commands == [] + assert len(provider.responses) == 1 + assert announcements == [ + { + "result": manager_mod._ABORTED_ACTION_RESULT, + "status": "error", + } + ] + + @pytest.mark.parametrize("bad", [0, -1]) def test_max_concurrent_subagents_must_be_positive(bad): with pytest.raises(ValidationError): From e8479562e861f843f309f275707db2ae867ece84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=B1=9F=E5=9B=BD=E5=BA=86?= Date: Fri, 31 Jul 2026 16:04:23 +0800 Subject: [PATCH 4/4] fix: cover shell power and deletion variants --- raven/agent/tools/shell_policy.py | 42 +++++++++++++++++++++++++------ tests/test_shell_approval.py | 21 ++++++++++++++++ 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/raven/agent/tools/shell_policy.py b/raven/agent/tools/shell_policy.py index 72e32e2d3..79e379cd7 100644 --- a/raven/agent/tools/shell_policy.py +++ b/raven/agent/tools/shell_policy.py @@ -48,9 +48,11 @@ ), } _ASSIGNMENT = re.compile(r"[A-Za-z_][A-Za-z0-9_]*=.*", re.DOTALL) -_COMMAND_BOUNDARIES = frozenset(";&|\n(){}") +_COMMAND_BOUNDARIES = frozenset(";&|\n(){}`") _SHELL_COMMAND_WRAPPERS = frozenset({"bash", "dash", "ksh", "sh", "zsh"}) -_SYSTEM_POWER_COMMANDS = frozenset({"poweroff", "reboot", "shutdown"}) +_SYSTEM_POWER_COMMANDS = frozenset({"halt", "poweroff", "reboot", "shutdown"}) +_POWER_MULTIPLEXERS = frozenset({"busybox", "init", "loginctl", "systemctl", "telinit"}) +_POWER_MULTIPLEXER_ACTIONS = _SYSTEM_POWER_COMMANDS | {"0", "6"} _MAX_EMBEDDED_SHELL_DEPTH = 4 @@ -70,7 +72,7 @@ def _command_segments(command: str) -> Iterator[list[str]]: or reproduce the full shell grammar. """ - lexer = shlex.shlex(command, posix=True, punctuation_chars=";&|(){}\n") + lexer = shlex.shlex(command, posix=True, punctuation_chars=";&|(){}`\n") lexer.commenters = "" # Newlines must remain visible as command boundaries. Quoted newlines are # still returned inside their quoted token and therefore do not split it. @@ -94,7 +96,14 @@ def _embedded_shell_command(segment: list[str]) -> str | None: if not segment or PurePath(segment[0]).name not in _SHELL_COMMAND_WRAPPERS: return None for index, token in enumerate(segment[1:], start=1): - if token.startswith("-") and not token.startswith("--") and "c" in token[1:] and index + 1 < len(segment): + if not token.startswith("-") or token.startswith("--"): + continue + command_option = token.find("c", 1) + if command_option == -1: + continue + if command_option + 1 < len(token): + return token[command_option + 1 :] + if index + 1 < len(segment): return segment[index + 1] return None @@ -109,8 +118,24 @@ def _matches_delete_command(command: str, *, _depth: int = 0) -> bool: executable = PurePath(segment[0]).name if executable in {"rm", "unlink"}: return True - if executable == "find" and "-delete" in segment[1:]: - return True + if executable == "find": + if "-delete" in segment[1:]: + return True + for index, token in enumerate(segment[1:], start=1): + if token not in {"-exec", "-execdir"}: + continue + executed = _unwrap_command_wrappers(segment[index + 1 :]) + if not executed: + continue + if PurePath(executed[0]).name in {"rm", "unlink"}: + return True + embedded_exec = _embedded_shell_command(executed) + if ( + embedded_exec is not None + and _depth < _MAX_EMBEDDED_SHELL_DEPTH + and _matches_delete_command(embedded_exec, _depth=_depth + 1) + ): + return True embedded = _embedded_shell_command(segment) if ( embedded is not None @@ -128,7 +153,10 @@ def _matches_system_power_command(command: str, *, _depth: int = 0) -> bool: segment = _unwrap_command_wrappers(segment) if not segment: continue - if PurePath(segment[0]).name in _SYSTEM_POWER_COMMANDS: + executable = PurePath(segment[0]).name + if executable in _SYSTEM_POWER_COMMANDS: + return True + if executable in _POWER_MULTIPLEXERS and any(arg in _POWER_MULTIPLEXER_ACTIONS for arg in segment[1:]): return True embedded = _embedded_shell_command(segment) if ( diff --git a/tests/test_shell_approval.py b/tests/test_shell_approval.py index cf0ec7190..430e85665 100644 --- a/tests/test_shell_approval.py +++ b/tests/test_shell_approval.py @@ -29,6 +29,10 @@ def policy() -> ShellCommandPolicy: "echo 'find . -delete'", "git grep -n shutdown", "grep -rn reboot /var/log", + "man shutdown", + "systemctl show reboot.target", + "grep -rn 'systemctl poweroff' docs/", + "bash -lc 'ls'", ], ) def test_safe_commands_are_allowed(policy: ShellCommandPolicy, command: str) -> None: @@ -45,8 +49,19 @@ def test_safe_commands_are_allowed(policy: ShellCommandPolicy, command: str) -> "echo ready && rm -rf tmp", "mkfs /dev/test", "shutdown now", + "halt", "sudo -n reboot", 'bash -c "poweroff"', + "systemctl poweroff", + "systemctl reboot", + "sudo systemctl reboot", + "busybox poweroff", + "loginctl poweroff", + "systemctl -i poweroff", + "init 0", + "init 6", + "telinit 0", + "telinit 6", ], ) def test_hard_denied_commands_cannot_be_approved(policy: ShellCommandPolicy, command: str) -> None: @@ -76,10 +91,16 @@ def test_hard_denied_commands_cannot_be_approved(policy: ShellCommandPolicy, com "(rm file.txt)", "{ rm file.txt; }", "echo $(rm file.txt)", + "echo `rm file.txt`", "nohup rm file.txt &", 'bash -c "rm file.txt"', + "bash -c'rm file.txt'", + 'bash -c"rm file.txt"', 'bash --rcfile setup.sh -c "rm file.txt"', 'sh -lc "find tmp -delete"', + 'find . -name "*.log" -exec rm {} \\;', + 'find . -name "*.log" -execdir unlink {} \\;', + 'find . -exec sh -c "rm \\"$1\\"" _ {} \\;', ], ) def test_delete_commands_require_approval(policy: ShellCommandPolicy, command: str) -> None: