diff --git a/clients/terminal/tui/events.py b/clients/terminal/tui/events.py index e21e4f8..bff7729 100644 --- a/clients/terminal/tui/events.py +++ b/clients/terminal/tui/events.py @@ -37,13 +37,3 @@ self.name = name self.args = args super().__init__() - - -class PermissionRequest(Message): - """Tool call requires destructive-operation confirmation (component fallback).""" - - def __init__(self, tool: str, args: dict, message: str) -> None: - self.tool = tool - self.args = args - self.message = message - super().__init__() diff --git a/clients/terminal/tui/permissions.py b/clients/terminal/tui/permissions.py deleted file mode 100644 index 7250e55..0000000 --- a/clients/terminal/tui/permissions.py +++ /dev/null @@ -1,124 +0,0 @@ -"""Permission engine for destructive tool operations.""" - -from __future__ import annotations - -import fnmatch -import json -from dataclasses import dataclass -from pathlib import Path -from typing import Callable - - -@dataclass -class PermissionRule: - """A rule that decides whether a tool call needs confirmation.""" - - tool: str - action: str | None = None # filesystem action, terminal command pattern, etc. - pattern: str | None = None # glob pattern for paths/commands - message: str = "" - - -# Default destructive patterns. -DEFAULT_RULES: list[PermissionRule] = [ - PermissionRule(tool="filesystem", action="delete", message="Delete file/directory"), - PermissionRule(tool="filesystem", action="move", message="Move/overwrite file"), - PermissionRule(tool="filesystem", action="write", message="Overwrite existing file"), - PermissionRule(tool="terminal", pattern="rm *", message="Remove files/directories"), - PermissionRule(tool="terminal", pattern="*format*", message="Format operation"), - PermissionRule(tool="terminal", pattern="*drop*", message="Drop database/table"), - PermissionRule(tool="code_exec", message="Execute arbitrary code"), - PermissionRule(tool="ssh_exec", message="Execute remote command"), - PermissionRule(tool="shell", action="run", message="Local shell command"), -] - - -PermissionCallback = Callable[[bool], None] - - -class PermissionEngine: - """Check whether a tool call requires user confirmation.""" - - def __init__(self, store_path: Path | None = None, rules: list[PermissionRule] | None = None) -> None: - self._rules = rules or list(DEFAULT_RULES) - self._store_path = store_path or (Path.home() / ".navi_code" / "permissions.json") - self._always_allow: set[str] = set() - self._always_deny: set[str] = set() - self._load() - - def _load(self) -> None: - if not self._store_path.exists(): - return - try: - data = json.loads(self._store_path.read_text()) - self._always_allow = set(data.get("allow", [])) - self._always_deny = set(data.get("deny", [])) - except Exception: - self._always_allow = set() - self._always_deny = set() - - def _save(self) -> None: - self._store_path.parent.mkdir(parents=True, exist_ok=True) - data = {"allow": sorted(self._always_allow), "deny": sorted(self._always_deny)} - self._store_path.write_text(json.dumps(data, indent=2)) - - def check(self, tool: str, args: dict) -> PermissionRule | None: - """Return matching rule if confirmation is needed, else None. - - Always-allow and always-deny entries both bypass the confirmation - dialog. Callers that need to actively reject always-deny matches can - use :meth:`is_always_deny`. - """ - rule_key = self._rule_key(tool, args) - if rule_key in self._always_allow or rule_key in self._always_deny: - return None - - for rule in self._rules: - if rule.tool != tool: - continue - if rule.action is not None: - if args.get("action") != rule.action: - continue - if rule.pattern is not None: - target = self.extract_target(tool, args) - if target is None or not fnmatch.fnmatch(target, rule.pattern): - continue - return rule - return None - - def is_always_deny(self, tool: str, args: dict) -> bool: - """Return True if this tool call was permanently denied by the user.""" - return self._rule_key(tool, args) in self._always_deny - - def extract_target(self, tool: str, args: dict) -> str: - """Public helper for extracting the human-readable target of a tool call.""" - return self._extract_target(tool, args) - - def set_always_allow(self, tool: str, args: dict) -> None: - self._always_allow.add(self._rule_key(tool, args)) - self._always_deny.discard(self._rule_key(tool, args)) - self._save() - - def set_always_deny(self, tool: str, args: dict) -> None: - self._always_deny.add(self._rule_key(tool, args)) - self._always_allow.discard(self._rule_key(tool, args)) - self._save() - - def _rule_key(self, tool: str, args: dict) -> str: - action = args.get("action", "") - target = self.extract_target(tool, args) - if target: - return f"{tool}:{action}:{target}" - return f"{tool}:{action}" - - @staticmethod - def _extract_target(tool: str, args: dict) -> str: - if tool == "filesystem": - return args.get("path", "") or args.get("destination", "") - if tool == "terminal": - return args.get("command", "") or args.get("action", "") - if tool == "code_exec": - return args.get("language", "") or args.get("code", "")[:40] - if tool == "ssh_exec": - return args.get("host", "") or args.get("command", "") - return "" diff --git a/clients/terminal/tui/screens/permission_dialog.py b/clients/terminal/tui/screens/permission_dialog.py deleted file mode 100644 index 1d95024..0000000 --- a/clients/terminal/tui/screens/permission_dialog.py +++ /dev/null @@ -1,80 +0,0 @@ -"""Modal permission dialog for destructive tool operations.""" - -from __future__ import annotations - -from textual.app import ComposeResult -from textual.containers import Grid, Horizontal -from textual.screen import ModalScreen -from textual.widgets import Button, Static - - -class PermissionDialogScreen(ModalScreen[str | None]): - """Ask user whether to allow a potentially destructive tool call. - - Returns: - "allow_once" | "allow_always" | "deny_once" | "deny_always" | None (dismissed) - """ - - DEFAULT_CSS = """ - PermissionDialogScreen { align: center middle; } - PermissionDialogScreen > Grid { - grid-size: 1; - grid-gutter: 1 2; - padding: 1 2; - border: thick $tui-error; - background: $tui-surface; - width: 60; - height: auto; - } - PermissionDialogScreen > Grid > Static { - width: 100%; - color: $tui-text; - } - PermissionDialogScreen .tool-name { - color: $tui-error; - text-style: bold; - } - PermissionDialogScreen .details { - color: $tui-text-dim; - } - PermissionDialogScreen .buttons { height: auto; } - PermissionDialogScreen Button { - margin: 0 1; - } - """ - - def __init__( - self, - tool: str, - action: str, - target: str, - details: str, - ) -> None: - super().__init__() - self._tool = tool - self._action = action - self._target = target - self._details = details - - def compose(self) -> ComposeResult: - with Grid(): - yield Static("Permission required", classes="tool-name") - yield Static(f"Tool: {self._tool}", classes="details") - if self._action: - yield Static(f"Action: {self._action}", classes="details") - if self._target: - yield Static(f"Target: {self._target}", classes="details") - if self._details: - yield Static(self._details, classes="details") - with Horizontal(classes="buttons"): - yield Button("Allow once", id="allow_once", variant="primary") - yield Button("Always allow", id="allow_always", variant="primary") - yield Button("Deny", id="deny_once", variant="error") - yield Button("Always deny", id="deny_always", variant="error") - - def on_button_pressed(self, event: Button.Pressed) -> None: - self.dismiss(event.button.id) - - def on_key(self, event) -> None: - if event.key == "escape": - self.dismiss("deny_once") diff --git a/clients/terminal/tui/tui_app.py b/clients/terminal/tui/tui_app.py index 460e7f7..e3db0ab 100644 --- a/clients/terminal/tui/tui_app.py +++ b/clients/terminal/tui/tui_app.py @@ -14,18 +14,15 @@ from clients.terminal.tui.context import TuiContext from clients.terminal.tui.events import ( ConnectionStatusChanged, - PermissionRequest, UserSubmitted, WsEvent, ) from clients.terminal.tui.file_refs import FileRefResolver -from clients.terminal.tui.permissions import PermissionEngine, PermissionRule from clients.terminal.tui.shell_runner import run_shell_command from clients.terminal.tui.settings import get_tui_settings from clients.terminal.tui.themes import ThemeRegistry, set_active_theme from clients.terminal.tui.commands.registry import get_registry from clients.terminal.tui.screens.command_palette import CommandPaletteScreen -from clients.terminal.tui.screens.permission_dialog import PermissionDialogScreen from clients.terminal.tui.widgets import ChatPanel, InputBox, StatusBar, StatusPanel, TodoPanel from clients.terminal.tui.ws_bridge import WsBridge @@ -70,8 +67,6 @@ cwd=cwd or Path.cwd().resolve(), ) self._bridge: WsBridge | None = None - self._permission_engine = PermissionEngine() - self._pending_permission: PermissionRequest | None = None self._requested_session_id = session_id self._requested_profile_id = profile_id self._force_new_session = new_session @@ -245,54 +240,10 @@ ) def _run_shell_command(self, text: str) -> None: - command = text[1:].strip() - args = {"action": "run", "command": command} - if self._permission_engine.is_always_deny("shell", args): - self._chat_panel.handle_ws_event( - {"type": "error", "message": f"Shell command denied by policy: {command}"} - ) - return - if self._permission_engine.check("shell", args) is None: - self.run_worker(self._shell_worker(text)) - return - self._confirm_shell_command(text) - - def _confirm_shell_command(self, text: str) -> None: - command = text[1:].strip() - - def on_decision(choice: str | None) -> None: - if choice == "allow_once": - self.run_worker(self._shell_worker(text)) - elif choice == "allow_always": - self._permission_engine.set_always_allow( - "shell", {"action": "run", "command": command} - ) - self.run_worker(self._shell_worker(text)) - elif choice == "deny_once": - self._chat_panel.handle_ws_event( - {"type": "error", "message": f"Shell command cancelled: {command}"} - ) - elif choice == "deny_always": - self._permission_engine.set_always_deny( - "shell", {"action": "run", "command": command} - ) - self._chat_panel.handle_ws_event( - {"type": "error", "message": f"Shell command cancelled: {command}"} - ) - else: - self._chat_panel.handle_ws_event( - {"type": "error", "message": f"Shell command cancelled: {command}"} - ) - - self.push_screen( - PermissionDialogScreen( - tool="shell", - action="run", - target=command, - details="Local shell command", - ), - callback=on_decision, - ) + # User-typed `!cmd` — the user explicitly invoked this, so no + # confirmation gate. (Agent-initiated tool calls are gated on the + # backend; see docs/permissions.md — not yet implemented.) + self.run_worker(self._shell_worker(text)) async def _shell_worker(self, text: str) -> None: result = run_shell_command(text) @@ -445,76 +396,8 @@ tool = payload.get("tool", "") self._status_bar.activity_label(f"running {tool}" if tool else "running tool") - if msg_type == "tool_started": - tool = payload.get("tool", "") - args = payload.get("args") or {} - if self._permission_engine.is_always_deny(tool, args): - self._deny_tool(tool, args) - return - rule = self._permission_engine.check(tool, args) - if rule is not None: - self._show_permission_dialog(payload, rule) - return self._chat_panel.handle_ws_event(payload) - def _show_permission_dialog(self, payload: dict, rule) -> None: - tool = payload.get("tool", "?") - args = payload.get("args") or {} - action = args.get("action", "") - target = self._permission_engine.extract_target(tool, args) - - def on_decision(choice: str | None) -> None: - if choice == "allow_once": - self._chat_panel.handle_ws_event(payload) - elif choice == "allow_always": - self._permission_engine.set_always_allow(tool, args) - self._chat_panel.handle_ws_event(payload) - elif choice == "deny_once": - self._deny_tool(tool, args) - elif choice == "deny_always": - self._permission_engine.set_always_deny(tool, args) - self._deny_tool(tool, args) - else: - # Dismissed / escape — treat as deny once. - self._deny_tool(tool, args) - - self.push_screen( - PermissionDialogScreen( - tool=tool, - action=action, - target=target, - details=rule.message, - ), - callback=on_decision, - ) - - def _deny_tool(self, tool: str, args: dict) -> None: - # Render a synthetic tool result so the user sees the denial, then stop - # the session because the backend is already executing the tool and the - # TUI cannot inject a result into the running tool-call loop. - self._chat_panel.handle_ws_event( - { - "type": "tool_call", - "tool": tool, - "args": args, - "success": False, - "result": "permission denied by user", - } - ) - if self._ctx.session_id: - self.run_worker(self._stop_session_worker(self._ctx.session_id)) - - async def _stop_session_worker(self, session_id: str) -> None: - try: - api.stop_session(session_id) - except Exception as exc: - self._chat_panel.handle_ws_event( - {"type": "error", "message": f"Failed to stop session: {exc}"} - ) - if self._bridge: - await self._bridge.stop() - self._status_panel.set_connection(False, "permission denied") - def on_connection_status_changed(self, event: ConnectionStatusChanged) -> None: self._status_panel.set_connection(event.connected, event.detail) # A dropped connection mid-turn will never deliver stream_end, so stop @@ -523,14 +406,6 @@ self._status_bar.activity_stop() self._status_bar.elapsed_stop() - def on_permission_request(self, event: PermissionRequest) -> None: - """Manual PermissionRequest event (fallback from components).""" - tool = event.tool - args = event.args - payload = {"type": "tool_started", "tool": tool, "args": args} - rule = PermissionRule(tool=tool, message=event.message) - self._show_permission_dialog(payload, rule) - def action_command_palette(self) -> None: registry = get_registry() diff --git a/docs/index.md b/docs/index.md index da4e937..593d8a8 100644 --- a/docs/index.md +++ b/docs/index.md @@ -38,6 +38,7 @@ | [`memory.md`](memory.md) | Long-term memory — facts, extraction, search | | [`eval_system.md`](eval_system.md) | Eval system — open scale, weekly trend | | [`android-client.md`](android-client.md) | Android WebView client — architecture, build/deploy | +| [`permissions.md`](permissions.md) | Permission gate — authoritative backend confirmation for destructive tool calls (**design only, not yet implemented**) | | [`auth.md`](auth.md) | Auth: multi-user OAuth via gnexus-auth, or disable with `NAVI_AUTH_ENABLED=false` | | [`config.md`](config.md) | All environment variables with types and defaults | | [`api.md`](api.md) | REST API endpoints + full WebSocket event schemas and sequences | diff --git a/docs/navi_code_cli.md b/docs/navi_code_cli.md index a5ade75..b2f93fd 100644 --- a/docs/navi_code_cli.md +++ b/docs/navi_code_cli.md @@ -108,10 +108,13 @@ ### Диалоги и палитра - **Command palette** (`Ctrl+P`) — поиск команд по имени. -- **Permission dialog** — запрос подтверждения на запуск инструмента: allow once/always, deny once/always. - **Sessions picker** — модальный выбор сессии. - **Theme picker** — выбор темы с live-preview. +> Подтверждение деструктивных tool-вызовов (permission gate) — отдельная +> серверная механика, описана в [`permissions.md`](permissions.md) +> (спроектирована, не реализована). Клиентского диалога пока нет. + ### Hotkeys | Комбинация | Действие | @@ -178,8 +181,8 @@ - `tui/widgets/` — `ChatPanel`, `InputBox`, `CommandHints`, `StatusBar`, `StatusPanel`, `TodoList` (+ `ActivityIndicator`, `ElapsedTime`). - `tui/renderers/` — рендереры карточек по типам событий (сообщения, markdown, рассуждения, инструменты, планирование, субагент, todo, summary, turn_meta, diff, artifact, error); диспетчеризация через `RendererRegistry`. - `tui/commands/` — slash-команды (`builtin.py`: `/help`, `/new`, `/sessions`, `/switch`, `/profile`, `/export`, `/themes`, `/mouse`, `/thinking`, `/compact`, `/quit`). -- `tui/screens/` — модальные экраны: `command_palette`, `sessions_picker`, `theme_picker`, `permission_dialog`. -- `tui/permissions.py`, `tui/shell_runner.py`, `tui/file_refs.py`, `tui/duration.py`, `tui/context.py`, `tui/events.py` — диалоги разрешений, `!`shell, `@`file-refs, таймеры, контекст, вспомогательные события. +- `tui/screens/` — модальные экраны: `command_palette`, `sessions_picker`, `theme_picker`. +- `tui/shell_runner.py`, `tui/file_refs.py`, `tui/duration.py`, `tui/context.py`, `tui/events.py` — `!`shell, `@`file-refs, таймеры, контекст, вспомогательные события. - `tui/settings.py` — persisted TUI config (`~/.navi_code/tui.json`). - `tui/themes.py` — палитры тем. diff --git a/docs/permissions.md b/docs/permissions.md new file mode 100644 index 0000000..d2831d5 --- /dev/null +++ b/docs/permissions.md @@ -0,0 +1,309 @@ +# Permission Gate (destructive-action confirmation) + +> **Status: design only — not yet implemented.** This document specifies the +> authoritative backend permission gate. The previous client-side mechanism +> (terminal-TUI `PermissionEngine` intercepting `tool_started`) was **removed** +> — it was a race: the backend executed the tool before the client dialog +> appeared, so "deny" could not un-execute the action. This design replaces it +> from zero. See the "Why the old mechanism was removed" section below. + +## Goal + +An **authoritative, client-agnostic** confirmation gate for destructive agent +tool calls. The tool **does not execute** until the user decides. The decision +lives on the backend; every client (terminal TUI, webclient, android) renders +the same `permission_request` event and posts the decision to the same endpoint. + +Applies to **all profiles** via a single global rules config (a destructive +operation is destructive regardless of which profile triggers it — e.g. writing +to system directories is equally undesirable from `navi_code`, `secretary`, or +any other profile). + +## Why the old mechanism was removed + +The old gate lived in `clients/terminal/tui/permissions.py` and fired on the +`tool_started` WebSocket event. But `agent.py` `_execute_tools_with_sink` emits +`ToolStarted` and **immediately starts the tool task** on the backend in +parallel: + +```python +yield ToolStarted(...) # event travels to the client +tool_task = asyncio.create_task( # tool is ALREADY running on the backend + _run_with_sentinel(...) +) +``` + +By the time the client showed the confirmation dialog, the destructive action +(file delete, `rm`, etc.) had already completed. `_deny_tool` could only render +a synthetic result and stop the session — it could not un-execute the tool. The +gate was therefore theatrical for agent-initiated calls. It also existed only in +the terminal TUI (webclient/android had nothing), and the system-prompt +"Strict Confirmation" nudge was an unreliable duplicate that competed with the +gate (double-asking) and was frequently ignored by local models. + +The removal was a clean slate: `permissions.py`, `screens/permission_dialog.py`, +the `PermissionRequest` TUI event, the `_deny_tool`/`_show_permission_dialog`/ +`_confirm_shell_command` wiring, the `!cmd` shell gate, and the "Strict +Confirmation" prompt rule were all deleted. This doc specifies the real +replacement. + +## Architecture + +``` +┌───────────────┐ tool_call ┌────────────────────────────┐ +│ Agent loop │──────────────▶│ PermissionEngine.check() │ +│ (_execute_ │ │ (rules + per-user policy) │ +│ tools_with_ │ └─────────────┬──────────────┘ +│ sink) │ │ rule matches? +│ │◀───────────────────────────┤ +│ await │ yield PermissionRequest │ yes +│ Future │─────────────┐ │ +│ (poll stop) │ ▼ │ +└──────┬────────┘ ┌──────────────────┐ │ + │ │ WebSocket event │ │ no → execute tool normally + │ │ permission_request│ │ + │ └────────┬─────────┘ │ + │ ▼ │ + │ ┌──────────────────┐ │ + │ │ Client dialog │ │ + │ │ (TUI/web/android)│ │ + │ └────────┬─────────┘ │ + │ │ decision │ + │ ▼ │ + │ POST /sessions/{id}/permissions/{request_id} + │ │ │ + │ ▼ │ + │ ┌──────────────────┐ │ + │◀───────────│ PermissionRegistry│◀───┘ + │ resolve() │ (Future by id) │ + ▼ └──────────────────┘ + allow → execute tool + deny → synthetic ToolResult("permission denied by user") to agent +``` + +## Components + +### 1. `navi/core/permissions.py` — backend engine + +Port of the old client `PermissionEngine`, adapted for server-side use and +multi-user persistence. + +```python +@dataclass +class PermissionRule: + tool: str + action: str | None = None # filesystem action, etc. + pattern: str | None = None # glob for paths / command prefixes + message: str = "" # human-readable reason shown in the dialog + +class PermissionEngine: + def __init__(self, rules: list[PermissionRule], policy_store): ... + def check(self, tool: str, args: dict) -> PermissionRule | None: ... + def is_always_allow(self, tool: str, args: dict, user_id: str) -> bool: ... + def is_always_deny(self, tool: str, args: dict, user_id: str) -> bool: ... + def set_always_allow(self, tool, args, user_id) -> None: ... # persist + def set_always_deny(self, tool, args, user_id) -> None: ... # persist + def extract_target(self, tool: str, args: dict) -> str: ... + def rule_key(self, tool: str, args: dict) -> str: ... +``` + +- `check` returns the matching rule **unless** the `(user_id, rule_key)` is in + `always_allow` or `always_deny` (both bypass the dialog; `is_always_deny` is + available to actively reject). +- `extract_target` / `rule_key` — same shape as before (path / command / host / + code-prefix). + +### 2. Global rules config file + +Rules are **global** (all profiles), loaded from a standalone JSON config file. +Path configurable via `settings` (e.g. `NAVI_PERMISSIONS_RULES`), default +`permissions.d/rules.json` — mirroring the `mcp_servers.d/` directory idiom. + +Schema (defaults shown — same set the old client used): + +```json +{ + "rules": [ + {"tool": "filesystem", "action": "delete", "message": "Delete file/directory"}, + {"tool": "filesystem", "action": "move", "message": "Move/overwrite file"}, + {"tool": "filesystem", "action": "write", "message": "Overwrite existing file"}, + {"tool": "terminal", "pattern": "rm *", "message": "Remove files/directories"}, + {"tool": "terminal", "pattern": "*format*", "message": "Format operation"}, + {"tool": "terminal", "pattern": "*drop*", "message": "Drop database/table"}, + {"tool": "code_exec", "message": "Execute arbitrary code"}, + {"tool": "ssh_exec", "message": "Execute remote command"} + ] +} +``` + +Notes: +- The old `"shell"` rule is dropped — user-typed `!cmd` is no longer gated (the + user typed it knowingly; the gate is for **agent-initiated** tool calls only). +- File is loaded once at startup into `PermissionEngine`; a `reload_tools`-style + hot reload is a nice-to-have, not required for v1. +- If the file is absent, the defaults above are used (engine ships with them). + +### 3. Per-user policy persistence (PostgreSQL) + +`always_allow` / `always_deny` decisions persist per user. + +Table: `user_permission_policies` + +| Column | Type | Description | +|---|---|---| +| `user_id` | TEXT | Owner of the decision | +| `rule_key` | TEXT | `tool:action:target` (engine rule_key) | +| `decision` | TEXT | `allow` \| `deny` | +| `created_at` | TIMESTAMPTZ | | +| `updated_at` | TIMESTAMPTZ | | + +Primary key `(user_id, rule_key)`. Postgres is the primary DB (sqlite fallback +only). A `PolicyStore` class wraps this; `PermissionEngine` depends on it. + +When auth is disabled (`NAVI_AUTH_ENABLED=false`), `user_id` is `anonymous` — +policy still works but is shared across the single-user install. + +### 4. `PermissionRegistry` — pending requests + +Holds `asyncio.Future`s keyed by `(session_id, request_id)` so the WS endpoint +can resolve a decision back to the blocked agent loop. + +```python +class PermissionRegistry: + def register(self, session_id, request_id) -> asyncio.Future: ... + async def await_decision(self, session_id, request_id, stop_event) -> str: ... + def resolve(self, session_id, request_id, decision: str) -> bool: ... + def cancel(self, session_id, request_id) -> None: ... +``` + +- `await_decision` **polls `stop_event`** with a short timeout (like the existing + sink poll in `_execute_tools_with_sink`) so cooperative-stop still works — a + stop while waiting is treated as `deny_once` and the request is cancelled. +- Lives in the app container, injected into `Agent`. +- One pending request per tool at a time (the loop is sequential), but the + registry is id-keyed so concurrent requests are supported if they ever occur. + +### 5. Event `PermissionRequest` (`navi/core/events.py`) + +Emitted by the gate **before** tool execution. Forwarded to WS clients. + +```python +@dataclass +class PermissionRequest(AgentEvent): + request_id: str + session_id: str + tool: str + action: str + target: str + message: str + args: dict + + def to_wire(self) -> dict: + return {"type": "permission_request", "request_id": ..., "session_id": ..., + "tool": ..., "action": ..., "target": ..., "message": ..., "args": ...} +``` + +### 6. Gate in `agent.py` `_execute_tools_with_sink` + +Inserted **before** `tool_task = asyncio.create_task(...)`, per tool call: + +```python +rule = self._permission_engine.check(tc.name, tc.arguments) +if rule is not None and not self._permission_engine.is_always_allow(tc.name, tc.arguments, user_id): + if self._permission_engine.is_always_deny(tc.name, tc.arguments, user_id): + # skip without a dialog — already permanently denied + yield _denied_tool_result(tc) + continue + request_id = str(uuid4()) + fut = self._permission_registry.register(session.id, request_id) + yield PermissionRequest(request_id, session.id, tool=tc.name, + action=tc.arguments.get("action", ""), + target=self._permission_engine.extract_target(tc.name, tc.arguments), + message=rule.message, args=tc.arguments) + decision = await self._permission_registry.await_decision(session.id, request_id, stop_event) + if decision in ("deny_once", "deny_always"): + if decision == "deny_always": + self._permission_engine.set_always_deny(tc.name, tc.arguments, user_id) + yield _denied_tool_result(tc) # agent sees "permission denied by user" + continue + if decision == "allow_always": + self._permission_engine.set_always_allow(tc.name, tc.arguments, user_id) + # allow_once / allow_always → fall through to execute +# ...existing tool_task execution unchanged +``` + +`_denied_tool_result(tc)` yields a synthetic `ToolResult(success=False, +"permission denied by user")` so the agent loop stays coherent and the model +can adapt (try an alternative, ask the user differently, or give up) instead of +hanging. + +### 7. REST endpoint + +`POST /api/sessions/{session_id}/permissions/{request_id}` + +```json +{"decision": "allow_once" | "allow_always" | "deny_once" | "deny_always"} +``` + +- Auth: session owner or admin (`check_session_access`). +- Resolves the Future via `PermissionRegistry.resolve`. +- Returns `200` on success, `404` if the request_id is unknown (already + resolved / cancelled / never existed), `403` if not the session owner. + +### 8. Clients + +Render `permission_request` and POST the decision. UI built fresh in each phase +(the old `PermissionDialogScreen` was deleted). + +- **terminal TUI** — new modal screen with the four choices; on selection POST + to the endpoint. No more `tool_started` interception. +- **webclient** — new `PermissionRequestDialog.vue` + WS handler + POST. +- **android** — WebView, inherits the webclient dialog. + +## What the agent sees / prompt impact + +The gate is **invisible to the agent except on deny**: a denied tool call +returns a normal `ToolResult` with `"permission denied by user"`. The model +treats it like any failed tool call. + +The "Strict Confirmation" prompt rule is **removed** (done with the cleanup): +the gate is authoritative, and a text-level nudge would cause double-asking and +be ignored by local models anyway. No prompt change is needed for the gate +itself — the deny result is enough signal. (Optional: a one-line note "the +system confirms destructive actions with the user; don't ask in text yourself" +if models start asking in text.) + +## Edge cases & decisions + +- **Sub-agents**: gated too. Sub-agent tool execution goes through the same + `_execute_tools_with_sink` / executor, so the gate applies. A sub-agent + deleting a file is equally destructive. Cost: a sub-agent spawn may surface + confirmation dialogs to the user; acceptable and correct. +- **`current_user_id`**: read from the ContextVar (set by the WS handler / + `run_ephemeral`) for policy persistence. Sub-agents inherit the parent user. +- **Timeout**: no automatic timeout — the request waits until the user decides + or stops the session (stop → `deny_once` + cancel). This matches "wait for the + user". A configurable timeout is a possible later addition. +- **Auth disabled**: `user_id = "anonymous"`; policy is shared. +- **Reload**: rules file loaded at startup; hot-reload is nice-to-have, not v1. +- **Rule ordering / specificity**: first matching rule wins (same as old + engine). `always_*` overrides any rule match. + +## Phasing + +- **Phase 1 — backend + terminal TUI**: `navi/core/permissions.py` (engine + + `PolicyStore` + postgres table + migration), `PermissionRegistry`, event, + endpoint, gate in `_execute_tools_with_sink`, global rules config file, + sub-agent coverage, terminal-TUI dialog. Tests. +- **Phase 2 — webclient**: `PermissionRequestDialog.vue` + WS handler + POST. +- **Phase 3 — android**: verify the webclient dialog renders in the WebView + (thin client usually inherits). + +## Open questions for implementation + +- Exact rules file path / `settings` key name (proposed + `NAVI_PERMISSIONS_RULES` → `permissions.d/rules.json`). +- Whether `always_allow/deny` decisions need an admin UI to review/revoke, or + the JSON policy file + a manual DB edit suffices for v1. +- Whether to emit a `permission_decision` event back to *other* connected + clients of the same session (multi-tab), so a second tab doesn't also prompt. \ No newline at end of file diff --git a/docs/profiles.md b/docs/profiles.md index f8a31ac..59b9204 100644 --- a/docs/profiles.md +++ b/docs/profiles.md @@ -162,7 +162,7 @@ - **Excluded:** `share_file`, `content_publish`, `ssh_exec`, `gmail`, `image_view`, `mcp__navi-web`. - **Planning:** Phase 1 and Phase 3 enabled, Phase 2 disabled to reduce latency. Phase 1 classifies the request as `MODE: observe | act`. - **Bounded autonomy:** `scope_boundary_enabled` and `observe_skips_plan_enabled` are both `true` — the agent stays within the requested scope (does not climb to sibling projects or execute discovered milestone/TODO docs) and `observe` requests (look/read/explain) skip the execution plan and just answer. Flip both off to reproduce the legacy "free flight" behavior. -- **Safety:** the system prompt asks Navi to confirm destructive operations (`rm`, overwrites) before executing them. +- **Safety:** an authoritative backend permission gate for destructive tool calls is specified in [`permissions.md`](permissions.md) (designed, not yet implemented). The old prompt-level "confirm before destructive ops" nudge and the client-side permission dialog were removed. Use it with `NAVI_DEFAULT_PROFILE_ID=navi_code` so `POST /sessions` without a `profile_id` creates a `navi_code` session automatically. See [`docs/navi_code.md`](navi_code.md) for the full local-terminal setup. diff --git a/docs/testing.md b/docs/testing.md index b2c95b1..07bbc41 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -75,8 +75,6 @@ ├── test_tui_sessions_panel.py ├── test_tui_settings.py ├── test_tui_themes.py - ├── test_permissions.py - ├── test_permission_dialog.py ├── test_shell_runner.py ├── test_file_refs.py └── test_diff_artifact_renderers.py diff --git a/navi/profiles/developer/system_prompt.txt b/navi/profiles/developer/system_prompt.txt index a29668b..0473d45 100644 --- a/navi/profiles/developer/system_prompt.txt +++ b/navi/profiles/developer/system_prompt.txt @@ -60,7 +60,6 @@ --- ## Safety Rules -- **Strict Confirmation**: Before any destructive or irreversible operation (e.g., deleting files or directories, running `rm`, overwriting an existing file wholesale with `write`, formatting disks, dropping database tables, force-pushing, etc.), you MUST explicitly ask the user for confirmation unless they have already approved that specific action in the current conversation. Routine edits via `edit`/`edit_lines` are not destructive and don't require confirmation. - Always double-check file paths before executing destructive terminal commands. ## Git discipline diff --git a/navi/profiles/navi_code/system_prompt.txt b/navi/profiles/navi_code/system_prompt.txt index d7fd26f..34a61f6 100644 --- a/navi/profiles/navi_code/system_prompt.txt +++ b/navi/profiles/navi_code/system_prompt.txt @@ -60,7 +60,6 @@ --- ## Safety Rules -- **Strict Confirmation**: Before any destructive or irreversible operation (e.g., deleting files or directories, running `rm`, overwriting an existing file wholesale with `write`, formatting disks, dropping database tables, force-pushing, etc.), you MUST explicitly ask the user for confirmation unless they have already approved that specific action in the current conversation. Routine edits via `edit`/`edit_lines` are not destructive and don't require confirmation. - Always double-check file paths before executing destructive terminal commands. ## Git discipline diff --git a/navi/profiles/tool_developer/system_prompt.txt b/navi/profiles/tool_developer/system_prompt.txt index 98a6a0c..62007ed 100644 --- a/navi/profiles/tool_developer/system_prompt.txt +++ b/navi/profiles/tool_developer/system_prompt.txt @@ -185,7 +185,6 @@ - Read the function or block you're changing plus a few lines of context, not the whole module. ## Safety Rules -- **Strict Confirmation**: Before any destructive or irreversible operation (e.g., deleting files or directories, running `rm`, overwriting an existing file wholesale with `write`, dropping database tables, force-pushing, etc.), you MUST explicitly ask the user for confirmation unless they have already approved that specific action in the current conversation. Routine edits via `edit`/`edit_lines` are not destructive and don't require confirmation. - Always double-check file paths before executing destructive terminal commands. ## Git discipline diff --git a/tests/clients/test_permission_dialog.py b/tests/clients/test_permission_dialog.py deleted file mode 100644 index 43b2604..0000000 --- a/tests/clients/test_permission_dialog.py +++ /dev/null @@ -1,51 +0,0 @@ -"""Tests for the inline permission dialog.""" - -from __future__ import annotations - -import pytest - -from clients.terminal.tui.screens.permission_dialog import PermissionDialogScreen -from clients.terminal.tui.events import WsEvent -from clients.terminal.tui.tui_app import NaviCodeTui - - -@pytest.mark.anyio -async def test_tool_started_triggers_permission_dialog() -> None: - """A destructive tool_started event opens the permission dialog.""" - async with NaviCodeTui(new_session=True).run_test() as pilot: - await pilot.pause() - chat = pilot.app.query_one("ChatPanel") - assert chat is not None - pilot.app.post_message(WsEvent({"type": "tool_started", "tool": "filesystem", "args": {"action": "delete", "path": "/tmp/x"}})) - await pilot.pause() - assert pilot.app.screen_stack[-1].__class__.__name__ == "PermissionDialogScreen" - - -@pytest.mark.anyio -async def test_allow_once_passes_tool_to_chat() -> None: - async with NaviCodeTui(new_session=True).run_test() as pilot: - await pilot.pause() - chat = pilot.app.query_one("ChatPanel") - assert chat is not None - pilot.app.post_message(WsEvent({"type": "tool_started", "tool": "filesystem", "args": {"action": "delete", "path": "/tmp/x"}})) - await pilot.pause() - dialog = pilot.app.screen_stack[-1] - assert isinstance(dialog, PermissionDialogScreen) - await pilot.click("#allow_once") - await pilot.pause() - assert any(item.kind == "tool_started" for item in chat._model.items) - - -@pytest.mark.anyio -async def test_deny_once_adds_synthetic_tool_call() -> None: - async with NaviCodeTui(new_session=True).run_test() as pilot: - await pilot.pause() - chat = pilot.app.query_one("ChatPanel") - assert chat is not None - pilot.app.post_message(WsEvent({"type": "tool_started", "tool": "filesystem", "args": {"action": "delete", "path": "/tmp/x"}})) - await pilot.pause() - dialog = pilot.app.screen_stack[-1] - assert isinstance(dialog, PermissionDialogScreen) - await pilot.click("#deny_once") - await pilot.pause() - assert any(item.kind == "tool_call" for item in chat._model.items) diff --git a/tests/clients/test_permissions.py b/tests/clients/test_permissions.py deleted file mode 100644 index e6c3711..0000000 --- a/tests/clients/test_permissions.py +++ /dev/null @@ -1,53 +0,0 @@ -"""Tests for the permission engine.""" - -from __future__ import annotations - -from pathlib import Path - -from clients.terminal.tui.permissions import PermissionEngine - - -def test_default_rules_match_destructive_filesystem() -> None: - engine = PermissionEngine(store_path=Path("/dev/null")) - assert engine.check("filesystem", {"action": "delete", "path": "x.txt"}) is not None - assert engine.check("filesystem", {"action": "read", "path": "x.txt"}) is None - - -def test_default_rules_match_terminal_rm() -> None: - engine = PermissionEngine(store_path=Path("/dev/null")) - assert engine.check("terminal", {"command": "rm -rf /tmp/x"}) is not None - assert engine.check("terminal", {"command": "ls /tmp"}) is None - - -def test_default_rules_match_code_exec_and_ssh_exec() -> None: - engine = PermissionEngine(store_path=Path("/dev/null")) - assert engine.check("code_exec", {"language": "python", "code": "print(1)"}) is not None - assert engine.check("ssh_exec", {"host": "server", "command": "uptime"}) is not None - - -def test_default_rules_match_shell_command() -> None: - engine = PermissionEngine(store_path=Path("/dev/null")) - assert engine.check("shell", {"action": "run", "command": "ls"}) is not None - - -def test_always_allow_bypasses_confirmation(tmp_path: Path) -> None: - store = tmp_path / "permissions.json" - engine = PermissionEngine(store_path=store) - engine.set_always_allow("terminal", {"command": "rm -rf /tmp/x"}) - assert engine.check("terminal", {"command": "rm -rf /tmp/x"}) is None - - -def test_always_deny_is_detected_without_rule(tmp_path: Path) -> None: - store = tmp_path / "permissions.json" - engine = PermissionEngine(store_path=store) - engine.set_always_deny("terminal", {"command": "rm -rf /tmp/x"}) - assert engine.is_always_deny("terminal", {"command": "rm -rf /tmp/x"}) is True - assert engine.check("terminal", {"command": "rm -rf /tmp/x"}) is None - - -def test_extract_target_for_tools() -> None: - engine = PermissionEngine(store_path=Path("/dev/null")) - assert engine.extract_target("filesystem", {"path": "/tmp/x"}) == "/tmp/x" - assert engine.extract_target("filesystem", {"destination": "/tmp/y"}) == "/tmp/y" - assert engine.extract_target("terminal", {"command": "ls"}) == "ls" - assert engine.extract_target("ssh_exec", {"host": "h1"}) == "h1"