supercode_harness/permissions/

approval.rs

//! P5-1 (COMPOSABLE-HARNESS-DESIGN.md §2 module 10, §2.10): approval policy
//! plumbing — the POLICY + session-scoped CACHE + a non-interactive decision
//! path. The INTERACTIVE ask-UI itself is the `tui` module (P5 row 4, not
//! this unit) — [`PermissionsApprovalHandler`] is the seam a CLI/TUI/SDK
//! embedder implements to plug an interactive (or scripted/headless) prompt
//! into `crate::agent::Agent`'s tool-dispatch gate, mirroring the existing
//! `crate::reduce::summarize::SpanSummarizer`/`crate::session_title::SessionTitler`
//! "installing one alone changes nothing, the `Config` gate is what turns it
//! on" pattern (`Agent::set_span_summarizer`/`Agent::set_session_titler`).

use std::collections::HashSet;
use std::path::{Path, PathBuf};
use std::sync::Mutex;

use super::rules::Decision;

/// What a [`PermissionsApprovalHandler`] decides for one `Ask`-tier request.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ApprovalOutcome {
    /// Refuse this one call.
    Deny,
    /// Allow this one call only.
    Allow,
    /// Allow this call AND cache the decision for the rest of this agent's
    /// session — CC's "don't ask again" / oc's "always" reply (cc§4, oc§4
    /// "Ask/approve flow"). Subsequent calls whose
    /// [`ApprovalCache::key`] matches skip the handler entirely.
    AllowForSession,
}

/// One `Ask`-tier request handed to a [`PermissionsApprovalHandler`] — enough
/// context for an interactive prompt (or a scripted policy) to render a
/// decision without needing back-references into `Agent`'s private state.
#[derive(Debug, Clone)]
pub struct ApprovalRequest<'a> {
    /// The tool being called (`"bash"`, `"write_file"`, …).
    pub tool: &'a str,
    /// The canonicalized command text (bash-family tools) or resolved path
    /// (file tools), if this call has one — `None` for a tool with no
    /// richer subject (e.g. `update_plan`).
    pub subject: Option<&'a str>,
    /// The raw, model-supplied arguments (for a handler that wants to show
    /// the user the exact call, not just the canonical summary).
    pub raw_args: &'a serde_json::Value,
}

/// The non-interactive decision seam a CLI/TUI/SDK embedder implements. The
/// engine (`crate::agent::Agent`'s gate) calls [`Self::ask`] ONLY when the
/// rule engine has already resolved a call to [`Decision::Ask`] — `Deny`
/// short-circuits before ever reaching a handler (a hard floor, never
/// consulted), and `Allow` never needs one. No handler installed (the
/// default) denies every `Ask` — fail-closed, the same posture
/// `Config::approval_handler`'s doc comment already documents for the
/// pre-P5-1 gate ("absent handler denies, so an OnRequest/Untrusted policy
/// is fail-closed" — agent.rs).
pub trait PermissionsApprovalHandler: Send + Sync {
    /// Render a decision for `req`. Implementations that need to block on
    /// user input (a real TUI prompt) do so here; a scripted/headless
    /// implementation (tests, CI, an `--auto-approve` flag) returns
    /// immediately.
    fn ask(&self, req: &ApprovalRequest) -> ApprovalOutcome;
}

/// Session-scoped "approve for session" decision cache (§2.10). Keyed by
/// [`Self::key`] — `(tool, subject)` — so a repeated identical call (the
/// SAME canonical command, or the same path) skips re-prompting for the rest
/// of this agent's lifetime, exactly like CC's "don't ask again"/oc's
/// "always" (cc§4, oc§4). Cheap and unconditional to construct — an agent
/// that never enables `capabilities.permissions` simply never populates or
/// consults it (§1.13-style "zero cost when off").
///
/// BP-10 (catalog row "Session approval caching (\"don't ask again\")",
/// semantics "Approvals persisted per session/project/**prefix**"): the
/// cache can be BACKED BY A FILE, so a grant outlives the process the way
/// CC's per-project "don't ask again" and cx's saved prefix rules both do.
/// [`Self::new`] (no store) is the pre-BP-10 in-memory cache, unchanged and
/// still the default for any embedder that never asks for persistence.
#[derive(Debug, Default)]
pub struct ApprovalCache {
    granted: Mutex<HashSet<String>>,
    /// BP-10: the JSON file this cache loads from and writes back to.
    /// `None` = in-memory only (the pre-BP-10 behavior).
    store: Option<PathBuf>,
}

impl ApprovalCache {
    /// A fresh, empty cache.
    pub fn new() -> Self {
        ApprovalCache::default()
    }

    /// BP-10: a cache backed by `store` — every grant already recorded
    /// there is honored immediately (so a grant made in an earlier process
    /// is not re-asked), and every new grant is written back.
    ///
    /// **Reversible, by construction.** The store is the whole state: a
    /// [`Self::clear`] (or simply deleting the file) drops every grant and
    /// the next matching call reaches the handler again. There is no
    /// second copy anywhere and no in-config residue to also undo.
    ///
    /// A store that cannot be read (absent, unreadable, corrupt) starts
    /// EMPTY rather than failing: a lost grant costs one extra prompt,
    /// which is the fail-closed direction — the same reasoning
    /// [`Self::approve`]'s poisoned-lock branch already documents. A store
    /// that cannot be WRITTEN degrades to in-memory for the run (the grant
    /// still holds for this session, it just is not remembered).
    pub fn persistent(store: impl Into<PathBuf>) -> Self {
        let store = store.into();
        let granted = load_store(&store);
        ApprovalCache {
            granted: Mutex::new(granted),
            store: Some(store),
        }
    }

    /// BP-10: the file this cache is backed by, if any.
    pub fn store_path(&self) -> Option<&Path> {
        self.store.as_deref()
    }

    /// BP-10: forget every grant — in memory AND on disk. The next
    /// matching call re-asks. This is the "reversible" half of
    /// [`Self::persistent`]; a missing store file is not an error (there
    /// was nothing to forget).
    pub fn clear(&self) {
        if let Ok(mut g) = self.granted.lock() {
            g.clear();
        }
        if let Some(store) = &self.store {
            let _ = std::fs::remove_file(store);
        }
    }

    /// The cache key for a `(tool, subject)` pair — no hashing, so it
    /// stays legible-ish in logs/debug output (see `length_prefixed`'s
    /// doc comment for D-3's length-prefixed encoding, which keeps this
    /// readable while still being provably unambiguous); a session cache
    /// has no untrusted-input DoS surface a hash would need to guard
    /// against (bounded by how many distinct calls one session can make).
    ///
    /// **Only use this directly for a request that HAS a `subject`** (bash's
    /// `command`, a file tool's resolved `path`, `apply_patch`'s `patch`).
    /// For the general case — including a request with NO subject — use
    /// [`Self::key_for_request`], which falls back to this exact function
    /// when `subject` is `Some` (so every existing bash/file-tool caller
    /// is unaffected) but does something different when it's `None` — see
    /// that method's doc comment for why (F2, Fable-5 adversarial review).
    pub fn key(tool: &str, subject: Option<&str>) -> String {
        match subject {
            Some(s) => length_prefixed(&[tool, s]),
            None => length_prefixed(&[tool]),
        }
    }

    /// F2 (Fable-5 adversarial review — HIGH, "'allow for session'
    /// over-grants tool-wide for subject-less tools"): the cache key
    /// `resolve_ask` actually uses, for ANY request shape.
    ///
    /// `ApprovalRequest::subject` is `command.or(path).or(patch)`
    /// (`Agent::permissions_gate_denial`) — `None` for every MCP tool call
    /// and any tool whose interesting content lives in richer JSON args
    /// rather than a single command/path/patch string (e.g.
    /// `mcp_db_query {"sql": "…"}`). Before this fix, [`Self::key`] alone
    /// collapsed a subject-less request down to the bare tool name, so an
    /// `AllowForSession` granted for ONE call's args
    /// (`{"sql":"SELECT 1"}`) silently auto-allowed EVERY later call to
    /// that tool regardless of args (`{"sql":"DROP TABLE users"}`) — an
    /// over-grant the user never saw, let alone approved.
    ///
    /// The fix: when there's no `subject`, fold a canonical digest of
    /// `raw_args` into the key too, so a session grant only ever
    /// auto-allows the exact SAME args again — a call with different args
    /// still reaches the handler. "Canonical" here means
    /// `canonical_json_string`'s recursively-key-sorted rendering, NOT
    /// `Value`'s own `Display`/`to_string()` — this crate's own build
    /// happens to render `Value`'s keys already-sorted (`serde_json`'s
    /// default `Map` backing is a `BTreeMap` unless some dependency's
    /// build pulls in the `preserve_order` feature and Cargo's feature
    /// resolver unifies it into this target too), but a SECURITY-relevant
    /// cache key has no business depending on an indirect, easily-
    /// disturbed fact like that — so this re-sorts explicitly and is
    /// correct regardless.
    ///
    /// When `subject` IS `Some` (bash/file tools/`apply_patch`), this is
    /// byte-identical to [`Self::key`] — those callers' session-grant
    /// breadth is completely unchanged.
    ///
    /// D-3 (Fable-5 delta review — LOW hardening): the `None` branch used
    /// to join `tool` and the args digest with a bare `\u{0}args:`
    /// separator that wasn't itself length-guarded — see
    /// `length_prefixed`'s doc comment for the exact collision the
    /// review proved constructible against [`Self::key`]'s `Some` branch,
    /// and why length-prefixing every component (rather than trusting an
    /// unlengthed separator no caller-controlled byte could ever
    /// reproduce) closes it for good.
    pub fn key_for_request(req: &ApprovalRequest) -> String {
        match req.subject {
            Some(s) => Self::key(req.tool, Some(s)),
            None => length_prefixed(&[req.tool, "args", &canonical_json_string(req.raw_args)]),
        }
    }

    /// Has `key` previously been granted "for session"?
    pub fn is_approved(&self, key: &str) -> bool {
        self.granted
            .lock()
            .map(|g| g.contains(key))
            .unwrap_or(false)
    }

    /// Record `key` as approved for the rest of this session. A poisoned
    /// lock (a prior panic while held) is treated as "cache unavailable" —
    /// silently drops the grant rather than panicking the caller; the next
    /// identical call simply re-prompts, which is the fail-closed direction
    /// (a lost cache entry costs an extra prompt, never a skipped one).
    pub fn approve(&self, key: &str) {
        let snapshot = match self.granted.lock() {
            Ok(mut g) => {
                g.insert(key.to_string());
                // BP-10: write the whole set, not an append — the file IS
                // the state, so a truncated/partial write is recovered by
                // the next grant rather than accumulating a diff nobody
                // can replay.
                let mut all: Vec<String> = g.iter().cloned().collect();
                all.sort();
                all
            }
            Err(_) => return,
        };
        if let Some(store) = &self.store {
            save_store(store, &snapshot);
        }
    }
}

/// BP-10: read a persisted grant set. Any failure (absent file, unreadable,
/// not a JSON string array) yields an EMPTY set — see
/// [`ApprovalCache::persistent`]'s doc comment for why that is the
/// fail-closed direction.
fn load_store(path: &Path) -> HashSet<String> {
    let Ok(text) = std::fs::read_to_string(path) else {
        return HashSet::new();
    };
    serde_json::from_str::<Vec<String>>(&text)
        .map(|v| v.into_iter().collect())
        .unwrap_or_default()
}

/// BP-10: write the grant set back. A failure is silent-but-degrading (the
/// run keeps its in-memory grants, they are simply not remembered) rather
/// than a panic in a tool-dispatch path.
fn save_store(path: &Path, keys: &[String]) {
    if let Some(parent) = path.parent() {
        let _ = std::fs::create_dir_all(parent);
    }
    if let Ok(text) = serde_json::to_string(keys) {
        let _ = std::fs::write(path, text);
    }
}

/// BP-10: the DEFAULT per-project approval store for `cwd` —
/// `$SUPERCODE_HOME/approvals/<project_tag>.json`. Transcribes
/// `crate::checkpoint`'s own `default_shadow_root` method exactly (same
/// `$SUPERCODE_HOME` resolver, same `crate::checkpoint::project_tag`), so a
/// remembered approval sits beside the session's other per-project records
/// instead of inventing a third layout.
///
/// Per PROJECT, not per session: cc§4 records "don't ask again" per
/// project+command and cx saves prefix rules the same way — a grant a user
/// gave once should not evaporate because they started a new session in the
/// same repo. It is still scoped: another project never sees it.
pub fn default_approval_store(cwd: &Path) -> PathBuf {
    crate::agent::global_instructions_dir()
        .join("approvals")
        .join(format!("{}.json", crate::checkpoint::project_tag(cwd)))
}

/// BP-10: the [`ApprovalCache`] a fresh `crate::agent::Agent` should carry,
/// given a resolved config. `capabilities.permissions.approvals.persist`
/// (`Config::permissions_approvals_persist`, `false` by default) is the ONE
/// gate: off returns the pre-BP-10 in-memory cache without touching the
/// filesystem at all.
pub fn cache_for_config(config: &crate::Config) -> ApprovalCache {
    if !config.permissions_approvals_persist {
        return ApprovalCache::new();
    }
    let store = config
        .permissions_approval_store
        .clone()
        .unwrap_or_else(|| default_approval_store(&config.cwd));
    ApprovalCache::persistent(store)
}

/// D-3 (Fable-5 delta review — LOW hardening, "unlengthed separator can
/// collide two distinct (tool, subject, args) triples"): join `parts` into
/// one unambiguous string by prefixing EACH component with its own byte
/// length (netstring-style: `"<decimal-length>:<bytes>"`, repeated back to
/// back, no trailing separator) — every [`ApprovalCache`] key this module
/// produces (both [`ApprovalCache::key`]'s `Some`/`None` branches and
/// [`ApprovalCache::key_for_request`]'s args-digest branch) is built from
/// this ONE function, so the whole key space shares one encoding rather
/// than two ad-hoc ones that could disagree.
///
/// **Why this actually closes the collision** (the review's own repro:
/// `ApprovalCache::key("bash", Some("x\0args:{}"))` rendered
/// byte-identical to `key_for_request` on a tool literally named
/// `"bash:x"` with no subject and empty args — both collapsed to
/// `"bash:x\0args:{}"` under the old bare `:`/`\u{0}` separators, which
/// weren't guarded against a component embedding those exact bytes
/// itself). A decoder here would parse strictly left to right: read
/// decimal digits up to the first `:` to learn a component's TRUE length,
/// then consume exactly that many bytes as its content, with no scanning
/// for a delimiter INSIDE that content — so no byte sequence a component
/// carries (a colon, a NUL, digits, anything) can ever be mistaken for a
/// length prefix or a boundary. And because the prefix is always the
/// REAL length of what follows (computed here via `p.len()`, never a
/// value a caller can pick independently of the content), the encoding's
/// TOTAL byte length is pinned to its true component count and their true
/// lengths — which is what additionally rules out a `(tool, subject)`
/// pair (2 components) ever colliding with a `(tool, "args", digest)`
/// triple (3 components): every extra component contributes at least 2
/// more bytes (`"0:"` at minimum), so encodings built from a different
/// number of parts can never even have equal total length, let alone
/// equal bytes. Deterministic (a pure function of `parts`) and
/// order-independent in the one sense that matters here — the args digest
/// fed in as one already-canonicalized (recursively key-sorted, F2)
/// string, so two calls with the same args in a different JSON key order
/// still produce the same component and thus the same key.
fn length_prefixed(parts: &[&str]) -> String {
    let mut out = String::new();
    for p in parts {
        out.push_str(&p.len().to_string());
        out.push(':');
        out.push_str(p);
    }
    out
}

/// F2: a canonical (recursively key-sorted) rendering of `value` — see
/// [`ApprovalCache::key_for_request`]'s doc comment for why this doesn't
/// just lean on `serde_json::Value`'s own `Display`. Rebuilding every
/// object from an already-sorted `BTreeMap` and re-serializing is correct
/// regardless of whether `serde_json`'s `Map` is itself `BTreeMap`- or
/// insertion-order (`indexmap`)-backed in a given build: either way, the
/// values get inserted into the output `Value::Object` in sorted order,
/// so `to_string()` renders them in that order.
fn canonical_json_string(value: &serde_json::Value) -> String {
    fn sorted(value: &serde_json::Value) -> serde_json::Value {
        match value {
            serde_json::Value::Object(map) => {
                let ordered: std::collections::BTreeMap<&String, &serde_json::Value> =
                    map.iter().collect();
                serde_json::Value::Object(
                    ordered
                        .into_iter()
                        .map(|(k, v)| (k.clone(), sorted(v)))
                        .collect(),
                )
            }
            serde_json::Value::Array(items) => {
                serde_json::Value::Array(items.iter().map(sorted).collect())
            }
            other => other.clone(),
        }
    }
    sorted(value).to_string()
}

/// Resolve one `Ask`-tier request against the cache + an optional handler:
/// cache hit → `true` (no handler call); no handler → `false` (fail-closed);
/// handler `Deny`/`Allow`/`AllowForSession` → `false`/`true`/`true`
/// (recording the grant in `cache` for the last case). This is the single
/// call site `crate::agent::Agent`'s gate uses, factored out so it's unit-
/// testable without a full `Agent`.
pub fn resolve_ask(
    cache: &ApprovalCache,
    handler: Option<&dyn PermissionsApprovalHandler>,
    req: &ApprovalRequest,
) -> bool {
    // F2 (Fable-5 adversarial review — HIGH): was `ApprovalCache::key`
    // alone, which collapses to the bare tool name for a subject-less
    // request — see `ApprovalCache::key_for_request`'s doc comment for
    // the over-grant that let a caller's `AllowForSession` on one MCP
    // call's args silently auto-allow a later, DIFFERENT call to the
    // same tool.
    let key = ApprovalCache::key_for_request(req);
    if cache.is_approved(&key) {
        return true;
    }
    match handler {
        None => false,
        Some(h) => match h.ask(req) {
            ApprovalOutcome::Deny => false,
            ApprovalOutcome::Allow => true,
            ApprovalOutcome::AllowForSession => {
                cache.approve(&key);
                true
            }
        },
    }
}

/// Convenience: fold a [`Decision`] into the boolean "may this call proceed"
/// the tool-dispatch gate needs, given a `resolve_ask`-style callback for the
/// `Ask` case. `Deny` never reaches `ask_fn` (hard floor); `Allow` never
/// needs it either.
pub fn decision_to_approved(decision: Decision, ask_fn: impl FnOnce() -> bool) -> bool {
    match decision {
        Decision::Deny => false,
        Decision::Allow => true,
        Decision::Ask => ask_fn(),
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    struct AlwaysAllow;
    impl PermissionsApprovalHandler for AlwaysAllow {
        fn ask(&self, _req: &ApprovalRequest) -> ApprovalOutcome {
            ApprovalOutcome::Allow
        }
    }
    struct AlwaysDeny;
    impl PermissionsApprovalHandler for AlwaysDeny {
        fn ask(&self, _req: &ApprovalRequest) -> ApprovalOutcome {
            ApprovalOutcome::Deny
        }
    }
    struct AlwaysAllowForSession;
    impl PermissionsApprovalHandler for AlwaysAllowForSession {
        fn ask(&self, _req: &ApprovalRequest) -> ApprovalOutcome {
            ApprovalOutcome::AllowForSession
        }
    }

    fn req<'a>(
        tool: &'a str,
        subject: Option<&'a str>,
        args: &'a serde_json::Value,
    ) -> ApprovalRequest<'a> {
        ApprovalRequest {
            tool,
            subject,
            raw_args: args,
        }
    }

    #[test]
    fn no_handler_denies_fail_closed() {
        let cache = ApprovalCache::new();
        let args = serde_json::json!({});
        assert!(!resolve_ask(
            &cache,
            None,
            &req("bash", Some("rm -rf /"), &args)
        ));
    }

    #[test]
    fn handler_deny_is_denied_and_not_cached() {
        let cache = ApprovalCache::new();
        let args = serde_json::json!({});
        let h = AlwaysDeny;
        assert!(!resolve_ask(
            &cache,
            Some(&h),
            &req("bash", Some("ls"), &args)
        ));
        assert!(!cache.is_approved(&ApprovalCache::key("bash", Some("ls"))));
    }

    #[test]
    fn handler_allow_once_is_not_cached() {
        let cache = ApprovalCache::new();
        let args = serde_json::json!({});
        let h = AlwaysAllow;
        assert!(resolve_ask(
            &cache,
            Some(&h),
            &req("bash", Some("ls"), &args)
        ));
        assert!(!cache.is_approved(&ApprovalCache::key("bash", Some("ls"))));
    }

    #[test]
    fn allow_for_session_is_cached_and_skips_handler_next_time() {
        let cache = ApprovalCache::new();
        let args = serde_json::json!({});
        let h = AlwaysAllowForSession;
        assert!(resolve_ask(
            &cache,
            Some(&h),
            &req("bash", Some("ls -la"), &args)
        ));
        assert!(cache.is_approved(&ApprovalCache::key("bash", Some("ls -la"))));
        // Second call: even a deny-everything handler is never consulted,
        // because the cache short-circuits first.
        let deny = AlwaysDeny;
        assert!(resolve_ask(
            &cache,
            Some(&deny),
            &req("bash", Some("ls -la"), &args)
        ));
    }

    #[test]
    fn cache_key_distinguishes_subjects() {
        let cache = ApprovalCache::new();
        cache.approve(&ApprovalCache::key("bash", Some("ls -la")));
        assert!(cache.is_approved(&ApprovalCache::key("bash", Some("ls -la"))));
        assert!(!cache.is_approved(&ApprovalCache::key("bash", Some("rm -rf /"))));
        assert!(!cache.is_approved(&ApprovalCache::key("write_file", Some("ls -la"))));
    }

    #[test]
    fn decision_to_approved_short_circuits_deny_and_allow() {
        assert!(!decision_to_approved(Decision::Deny, || panic!(
            "must not call"
        )));
        assert!(decision_to_approved(Decision::Allow, || panic!(
            "must not call"
        )));
        assert!(decision_to_approved(Decision::Ask, || true));
        assert!(!decision_to_approved(Decision::Ask, || false));
    }

    // ---- F2 (Fable-5 adversarial review — HIGH): "allow for session"
    // must be per-args, not per-tool, for a subject-less request ----

    /// THE regression test for the HIGH finding: an `AllowForSession` on
    /// `mcp_db_query {"sql":"SELECT 1"}` must NOT auto-allow
    /// `{"sql":"DROP TABLE users"}` — before this fix, both collapsed to
    /// the same bare-tool-name cache key.
    #[test]
    fn allow_for_session_on_a_subject_less_tool_does_not_leak_to_different_args() {
        let cache = ApprovalCache::new();
        let handler = AlwaysAllowForSession;
        let safe_args = serde_json::json!({"sql": "SELECT 1"});
        let safe = req("mcp_db_query", None, &safe_args);
        assert!(resolve_ask(&cache, Some(&handler), &safe));
        // Same tool, same args again: cache hit, no handler needed (would
        // still return true even without a handler if this weren't
        // cached — assert the handler path directly below instead).
        assert!(resolve_ask(&cache, Some(&handler), &safe));

        // Same tool, DIFFERENT args: must NOT be auto-allowed by the
        // grant above — prove it by using `AlwaysDeny` as the handler
        // here, so a pass is only possible if the cache did NOT
        // short-circuit (i.e. the handler really was consulted and really
        // denied).
        let deny = AlwaysDeny;
        let dangerous_args = serde_json::json!({"sql": "DROP TABLE users"});
        let dangerous = req("mcp_db_query", None, &dangerous_args);
        assert!(
            !resolve_ask(&cache, Some(&deny), &dangerous),
            "a session grant for one arg set must not leak to a different one"
        );
    }

    #[test]
    fn key_for_request_is_per_args_when_subject_is_none() {
        let a = serde_json::json!({"sql": "SELECT 1"});
        let b = serde_json::json!({"sql": "DROP TABLE users"});
        let key_a = ApprovalCache::key_for_request(&req("mcp_db_query", None, &a));
        let key_b = ApprovalCache::key_for_request(&req("mcp_db_query", None, &b));
        assert_ne!(key_a, key_b);

        // The SAME args, keys given in a different order, must produce
        // the SAME key (a call is "the same call" regardless of the
        // model's own JSON key ordering).
        let a_reordered = serde_json::json!({
            "extra": "same",
            "sql": "SELECT 1",
        });
        let a2 = serde_json::json!({
            "sql": "SELECT 1",
            "extra": "same",
        });
        let key_a_reordered =
            ApprovalCache::key_for_request(&req("mcp_db_query", None, &a_reordered));
        let key_a2 = ApprovalCache::key_for_request(&req("mcp_db_query", None, &a2));
        assert_eq!(key_a_reordered, key_a2);
    }

    #[test]
    fn key_for_request_is_unchanged_for_bash_and_file_tools() {
        // Every existing subject-carrying caller (bash's `command`, a
        // file tool's `path`, `apply_patch`'s `patch`) must see BYTE-
        // IDENTICAL key behavior — `key_for_request` must not widen OR
        // narrow their existing session-grant breadth.
        let args = serde_json::json!({"command": "git status"});
        let r = req("bash", Some("git status"), &args);
        assert_eq!(
            ApprovalCache::key_for_request(&r),
            ApprovalCache::key("bash", Some("git status"))
        );
    }

    /// The review's own worked example, end to end: a bash `AllowForSession`
    /// for `git status` must not leak to `git push` — proves `key_for_request`
    /// didn't accidentally change bash's existing exact-subject-match
    /// behavior while fixing the subject-less case.
    #[test]
    fn bash_allow_for_session_still_does_not_leak_to_a_different_command() {
        let cache = ApprovalCache::new();
        let handler = AlwaysAllowForSession;
        let status_args = serde_json::json!({"command": "git status"});
        assert!(resolve_ask(
            &cache,
            Some(&handler),
            &req("bash", Some("git status"), &status_args)
        ));

        let deny = AlwaysDeny;
        let push_args = serde_json::json!({"command": "git push"});
        assert!(!resolve_ask(
            &cache,
            Some(&deny),
            &req("bash", Some("git push"), &push_args)
        ));
    }

    #[test]
    fn canonical_json_string_sorts_nested_objects_and_arrays() {
        let a = serde_json::json!({"z": 1, "a": {"y": 2, "b": 3}, "list": [{"n": 2, "m": 1}]});
        let b = serde_json::json!({"a": {"b": 3, "y": 2}, "z": 1, "list": [{"m": 1, "n": 2}]});
        assert_eq!(canonical_json_string(&a), canonical_json_string(&b));
    }

    // ---- D-3 (Fable-5 delta review — LOW hardening): no separator
    // injection can collide two distinct (tool, subject, args) triples ----

    /// THE regression test for the review's own repro: `bash` with a
    /// subject that embeds the OLD raw separator bytes
    /// (`"x\0args:{}"`) must no longer render the same key as a
    /// subject-less call to a tool literally named `"bash:x"` with empty
    /// args — under the pre-D-3 bare `:`/`\u{0}` joins, both collapsed to
    /// `"bash:x\0args:{}"`. Fail-on-revert: reverting `length_prefixed`
    /// back to the old `format!("{tool}:{s}")` / `format!("{}\u{0}args:{}",
    /// ...)` pair makes this fail.
    #[test]
    fn key_collision_probe_bash_subject_vs_colon_named_tool_now_distinct() {
        let empty_args = serde_json::json!({});

        let subject_key = ApprovalCache::key("bash", Some("x\0args:{}"));

        let colon_named_tool_req = req("bash:x", None, &empty_args);
        let args_digest_key = ApprovalCache::key_for_request(&colon_named_tool_req);

        assert_ne!(
            subject_key, args_digest_key,
            "a subject embedding the old separator bytes must not collide with an \
             unrelated colon-named tool's subject-less key"
        );
    }

    /// The same probe via [`ApprovalCache::key_for_request`] end to end
    /// (not just the raw [`ApprovalCache::key`] building block), proving
    /// the two DIFFERENT call shapes (`bash` with a subject vs. a
    /// subject-less `"bash:x"`) never share a cache entry: granting one
    /// "for session" must not silently cover the other.
    #[test]
    fn key_collision_probe_does_not_let_one_grant_cover_the_other() {
        let cache = ApprovalCache::new();
        let empty_args = serde_json::json!({});

        let bash_with_tricky_subject = req("bash", Some("x\0args:{}"), &empty_args);
        cache.approve(&ApprovalCache::key_for_request(&bash_with_tricky_subject));

        let colon_named_tool_req = req("bash:x", None, &empty_args);
        assert!(
            !cache.is_approved(&ApprovalCache::key_for_request(&colon_named_tool_req)),
            "granting the tricky-subject bash call must not also grant the \
             unrelated colon-named, subject-less tool call"
        );
    }

    /// `length_prefixed` itself, directly: differing arity (2 parts vs
    /// 3 parts) must never collide even when a component is crafted to
    /// mimic the other encoding's bytes.
    #[test]
    fn length_prefixed_distinguishes_differing_arity_and_embedded_separators() {
        assert_ne!(
            length_prefixed(&["bash", "x\0args:{}"]),
            length_prefixed(&["bash:x", "args", "{}"])
        );
        // Same components, called out explicitly: embedding a `:` or a
        // NUL byte inside a component doesn't change how many bytes that
        // component's own length prefix claims, so it can't be mistaken
        // for a boundary.
        assert_ne!(
            length_prefixed(&["a:b", "c"]),
            length_prefixed(&["a", "b:c"])
        );
    }
}