Merge pull request #1037 from corbitsdev/cl-7950-unify-permission-verdict-path-and-single-owner-shell-policy

Unify permission verdict path with a mode-invariant catastrophic deny

Author: TheGreatAxios

docs/ARCHITECTURE.md

@@ -364,9 +364,9 @@ Tool middleware applied over `createPosixTools`, in this order:
 tool call
   → pathEscapePlugin      (resolve + sandbox paths)
     → secretGuardPlugin   (hard-deny path-keyed secret files)
-      → authzPlugin       (deny catastrophic commands)
-        → permissionPlugin (tiered operator approval)
-          → verifyPlugin   (post-write/edit verification)
+      → permissionPlugin  (tiered operator approval; hard-denies catastrophic
+                           shell commands at the top of its verdict path)
+        → verifyPlugin   (post-write/edit verification)
             → actual tool execution
 ```
 
@@ -377,7 +377,7 @@ tool call
 - **Evidence archive** (`evidence-archive-search-plugin.ts`, `evidence-archive-path-guard.ts`) — Primary-session compaction evidence is a first-class search/read surface on `search_files` / `read_file` / `grep` via `archive:///` refs. Dump paths (`evidence-archive/`, `tool-output/archive-*`) stay blocked so the on-disk sidecar is not the retrieval API. Blob keys reject `/` so they cannot nest under `tool-output`.
 - **Tool-output URI** (`tool-output-uri-plugin.ts`) — Normalizes mistaken `read_file` blob URIs to `tool-output:///id` (corbits-only; interchange stays unpatched).
 - **Secret Guard** (`secret-guard-plugin.ts`) — Hard-denies path-keyed tool calls (`read_file`, `write_file`, …) that would put a sensitive file into (or write it from) the model context. Runs before the permission plugin, so the path-arg deny holds even under `--dangerously-skip-permissions`. Shell commands that _reference_ a sensitive path (tokenized so `cat .env`, `bun --env-file=.env run …`, and quote/env-assignment forms are detected) are not hard-denied here: they require operator approval via the permission gate, and auto mode forces an ask through the auto-shell policy (`sensitive-path` rule). Once the operator approves, the command runs. Shell detection is best-effort: token matching defeats quoting and env-assignment/redirection forms but not dynamic path construction (variable indirection, `printf` assembly). Tool-result secret scrub still redacts credential-shaped output.
-- **Authorization** (`run-shell-authz.ts`, wired by `authz-plugin.ts`) — Denies catastrophic shell command patterns by regex, and hard-blocks shell `find`, head-position `rg`, and recursive `grep -r` (they can walk huge trees and OOM the host). Bounded `grep`/`search_files` tools remain practical alternatives (timeout + output caps); the patterns match those three command shapes only — an `ls -R`, `fd`, or scripted `os.walk` is just as unbounded and is not caught, so the block message tells the model not to substitute one. The permission gate’s shell auto-allow path consults the same policy so it never pre-approves a command authz would reject.
+- **Authorization** (`run-shell-authz.ts`, enforced by the permission gate) — Denies catastrophic shell command patterns by regex, and hard-blocks shell `find`, head-position `rg`, and recursive `grep -r` (they can walk huge trees and OOM the host). Bounded `grep`/`search_files` tools remain practical alternatives (timeout + output caps); the patterns match those three command shapes only — an `ls -R`, `fd`, or scripted `os.walk` is just as unbounded and is not caught, so the block message tells the model not to substitute one. The gate hard-denies these at the top of its verdict path — before auto-allow, prompting, grants, and skipPermissions — so no mode or stored grant can admit them.
 - **Permission** (`permission-plugin.ts`) — Delegates consequential calls to the permission gate.
 - **Shell Guard** (`shell-guard-plugin.ts`) — Corbits Code-only replacement for stock `run_shell` (interchange stays unpatched): no built-in default timeout (optional per-call or `settings.shell.timeoutMs`; `maxTimeoutMs` clamps only a resolved timeout), 512KB display cap with head+tail retention (the process keeps running when the cap is hit), process-group kill on timeout, abort, and plugin dispose (live children tracked in the plugin and reaped by `posixTools.dispose`), and `background: true` — the call returns a `shell_id` at once (registry in `src/shell/background-shell.ts`), the process group keeps running past the turn, completion is delivered on a later turn via `buildShellBackgroundMessage`, and `shell_collect` retrieves or cancels (schema advertised by `advertiseShellGuardTimeout`; evaluated by the permission chain at start time like any shell call). Also applies a 10s wall-clock budget to `grep`/`search_files`. Ripgrep detached spawns are not tracked.
 - **Read File Guard** (`read-file-guard-plugin.ts`) — Corbits Code-only short-circuit for `read_file` on real filesystem paths and configured `tool-output://` URIs (interchange stays unpatched): streaming reads that never decode the whole file in one pass, caps model-facing output at 50KB, defaults to 2000 lines, truncates long lines with recovery hints, samples the first chunk to reject binary, and stops at an 8MB scan ceiling. Emits `offset` continuation notices so the model can page without losing file or spill content on disk.

docs/IMPLEMENTATION.md

@@ -121,10 +121,9 @@ src/
     evidence-archive-path-guard.ts     Block dump-path reads of the archive sidecar
     tool-output-uri-plugin.ts  Normalize read_file tool-output URIs
     secret-guard-plugin.ts     Hard-deny path-keyed secret files
-    authz-plugin.ts            Catastrophic command blocking (thin wrapper)
-    permission-plugin.ts       Tiered operator approval
+    permission-plugin.ts       Tiered operator approval (owns catastrophic shell deny)
   shell/
-    run-shell-authz.ts         Shared run_shell deny policy (authz + permission)
+    run-shell-authz.ts         Shared run_shell deny policy (gate-enforced)
     background-shell.ts        Background run_shell registry (start/collect/cancel/disposeAll)
     verify-plugin.ts           Write/edit verification (per-path lock)
     file-mutation-lock.ts      Serialize mutations per file for verify

src/agent/codex-read-raw-file.ts

@@ -3,7 +3,7 @@
  *
  * `applyOp` calls this directly from outside the posixTools middleware chain
  * — no ToolPlugin ever sees `op.path` here, unlike the write leg which still
- * goes through the full pathEscapePlugin / secretGuardPlugin / authzPlugin /
+ * goes through the full pathEscapePlugin / secretGuardPlugin /
  * permissionPlugin stack (see buildCorePosixToolPlugins in
  * posix-tool-plugins.ts). `requireRelativePath` in codex-apply-patch.ts only
  * rejects absolute paths — it does nothing about `../` traversal — so this

src/agent/posix-tool-plugins.test.ts

@@ -7,6 +7,7 @@ import { createPosixTools, composeMiddleware } from "@intx/tools-posix";
 import type { ToolPlugin } from "@intx/tools-posix";
 import type { ToolCall, ToolResult } from "@intx/types/runtime";
 import { createPermissionGate } from "../permission/gate.js";
+import { BLOCKED_BY_POLICY_PREFIX } from "../permission/decline-markers.js";
 import { buildCorePosixToolPlugins } from "./posix-tool-plugins.js";
 import {
   createCompositeBlobReader,
@@ -688,4 +689,52 @@ describe("buildCorePosixToolPlugins", () => {
     expect(content).not.toContain(straddlingSecret);
     expect(content).not.toMatch(/AKIA[0-9A-Z]*/);
   });
+
+  test("catastrophic shell stays denied with secret-guard ahead of the gate in skipPermissions mode (CL-7950)", async () => {
+    // The pass-through hard-deny plugin folded into the gate verdict path:
+    // with no separate enforcement plugin left in the chain, the gate itself
+    // must deny catastrophic shell even when skipPermissions auto-allows
+    // everything else, and secret-guard must still sit ahead of it.
+    const cwd = await mkdtemp(join(tmpdir(), "cl7950-fold-"));
+    try {
+      const gate = createPermissionGate({
+        approvals: [],
+        interactive: false,
+        skipPermissions: true,
+        reactorGated: false,
+        cwd,
+      });
+      const plugins = buildCorePosixToolPlugins({ cwd, permissionGate: gate });
+      const secretGuardIndex = findMiddlewareIndex(
+        plugins,
+        "Access to sensitive file blocked by policy",
+      );
+      const permissionIndex = findMiddlewareIndex(plugins, "gateToolCall");
+      expect(secretGuardIndex).toBeGreaterThanOrEqual(0);
+      expect(permissionIndex).toBeGreaterThanOrEqual(0);
+      expect(secretGuardIndex).toBeLessThan(permissionIndex);
+
+      const composed = composeMiddleware(
+        plugins
+          .map((plugin) => plugin.middleware)
+          .filter((mw): mw is NonNullable<typeof mw> => mw !== undefined),
+        async (call) => ({ callId: call.id, content: "reached terminal" }),
+      );
+      const signal = new AbortController().signal;
+      const blocked = await composed(
+        { id: "c1", name: "run_shell", arguments: { command: "sudo reboot" } },
+        signal,
+      );
+      expect(blocked.isError).toBe(true);
+      expect(String(blocked.content)).toContain(BLOCKED_BY_POLICY_PREFIX);
+      const allowed = await composed(
+        { id: "c2", name: "run_shell", arguments: { command: "echo hi" } },
+        signal,
+      );
+      expect(allowed.isError).not.toBe(true);
+      expect(String(allowed.content)).toContain("hi");
+    } finally {
+      await rm(cwd, { recursive: true, force: true });
+    }
+  });
 });

src/agent/posix-tool-plugins.ts

@@ -5,7 +5,6 @@ import { evidenceArchivePathGuardPlugin } from "../plugins/evidence-archive-path
 import { evidenceArchiveSearchPlugin } from "../plugins/evidence-archive-search-plugin.js";
 import { deleteFilePlugin } from "../plugins/delete-file-plugin.js";
 import { secretGuardPlugin } from "../plugins/secret-guard-plugin.js";
-import { authzPlugin } from "../plugins/authz-plugin.js";
 import { permissionPlugin } from "../plugins/permission-plugin.js";
 import { verifyPlugin } from "../plugins/verify-plugin.js";
 import { editFileDiagnosticsPlugin } from "../plugins/edit-file-diagnostics-plugin.js";
@@ -96,8 +95,8 @@ export function buildCorePosixToolPlugins(
   // Pre-gate sandboxes honor yolo mode so outside-workspace path tools and shell
   // cwd are not hard-denied after the gate already auto-allows. Pass a live
   // getter so `/yolo` mid-session unlocks (or re-enforces) bounds without
-  // rebuilding the plugin stack. Secret-guard and authz still hard-deny
-  // regardless.
+  // rebuilding the plugin stack. Secret-guard and the gate's
+  // catastrophic-shell check still hard-deny regardless.
   const allowOutside = (): boolean => permissionGate.getSkipPermissions();
   // One shared workspace-roots provider for every bound in this stack, so
   // pathEscape and delete_file admit the same registered sibling worktrees.
@@ -120,7 +119,6 @@ export function buildCorePosixToolPlugins(
     deleteFilePlugin(cwd, { allowOutside, rootsProvider }),
     toolOutputUriPlugin(),
     secretGuardPlugin(),
-    authzPlugin(),
     permissionPlugin(permissionGate),
     shellGuardPlugin(cwd, shellTimeout, shellEnv, {
       allowOutsideCwd: allowOutside,

src/permission/authz-grants.ts

@@ -97,12 +97,14 @@ export interface EvaluateApprovalsInput {
   workspace: GrantWorkspace;
 }
 
-// Grant-store evaluation via @intx/authz. Filters provider-model and cwd via
-// grantScopeMatches, then asks evaluateGrants for the highest-specificity
-// allow among package-compatible grants. Exact-escaped grants are checked with
-// matchesPattern (equality after unescape) first so a stored exact command is
-// never lost.
-export async function evaluateApprovals(
+// Grant-evaluation owner for the live decide() path: the shell per-segment
+// checks and the path-arg check inside decide() resolve coverage through this
+// function. The queued-request reconciliation path (isRequestCoveredByApprovals
+// in gate.ts) matches inline against the same scope helper and pattern
+// matcher instead of calling here, so keep the two in sync when changing
+// matching semantics. Fail-closed throughout:
+// unknown tools, unknown runners, and empty grant lists all refuse.
+export async function approvalCoversSubject(
   input: EvaluateApprovalsInput,
 ): Promise<boolean> {
   const {
@@ -135,3 +137,14 @@ export async function evaluateApprovals(
   const decision = await evaluateGrants(grants, subject, tool);
   return decision.effect === "allow";
 }
+
+// Grant-store evaluation via @intx/authz. Filters provider-model and cwd via
+// grantScopeMatches, then asks evaluateGrants for the highest-specificity
+// allow among package-compatible grants. Exact-escaped grants are checked with
+// matchesPattern (equality after unescape) first so a stored exact command is
+// never lost.
+export async function evaluateApprovals(
+  input: EvaluateApprovalsInput,
+): Promise<boolean> {
+  return approvalCoversSubject(input);
+}

src/permission/gate.test.ts

@@ -32,8 +32,8 @@ const shellCall = (command: string): ToolCall => ({
 //
 // The shell-authz hard-deny cases are not independently reachable through
 // reconciliation today — evaluate() already denies and returns before such a
-// request is ever queued (see the block-reason check ahead of the per-request
-// loop), so a queued entry has always already cleared this guard. They stay
+// request is ever queued (see the block-reason check at the top of the
+// verdict path), so a queued entry has always already cleared this guard. They stay
 // in preGrantGuardReason and this table anyway as drift-resistance: if a
 // future refactor ever let a hard-denied command reach the queue, this still
 // catches it.

src/permission/gate.ts

@@ -28,7 +28,7 @@ import {
 import { runShellAuthzBlockReason } from "../shell/run-shell-authz.js";
 import { matchesPattern, escapeGlobLiteral } from "./matcher.js";
 import {
-  evaluateApprovals,
+  approvalCoversSubject,
   grantScopeMatches,
   type GrantWorkspace,
 } from "./authz-grants.js";
@@ -645,6 +645,24 @@ export function createPermissionGate(
       };
 
   const decide = async (call: ToolCall): Promise<GateDecision> => {
+    // Catastrophic shell commands are hard-denied here, at the top of the
+    // single verdict path every entry (evaluate, authorizeCall,
+    // executionVerdict) flows through — this is the owning enforcement point
+    // for the run-shell-authz classification. The verdict is invariant across
+    // modes: auto, headless, and skipPermissions never allow these commands.
+    // Judged against the full command string, not per split segment, so a
+    // stage that only reads bounded, already-piped data (e.g.
+    // `git show sha:path | rg -n foo`) is not denied in isolation when the
+    // full pipeline is exempt. This runs before every grant shortcut — a
+    // stored grant must never admit a hard-denied command (see
+    // preGrantGuardReason).
+    if (call.name === "run_shell") {
+      const command = String(call.arguments.command ?? "");
+      const blockReason = runShellAuthzBlockReason(command);
+      if (blockReason !== undefined) {
+        return { kind: "deny", reason: blockReason };
+      }
+    }
     if (skipPermissions) return { kind: "allow" };
     // Sub-agent tool calls run under ALS identity (identity-context.ts). The
     // process cwd is the worktree (or session when no identity is set); every
@@ -751,20 +769,8 @@ export function createPermissionGate(
         );
         if (segments.length === 0) continue;
 
-        // A command authz would hard-deny at execution is stricter than "ask":
-        // the gate must deny the call outright rather than show an Accept
-        // button for a command that can never actually run. Judged against the
-        // full command string with the same predicate authz enforces at
-        // execution time — not per split segment — so a stage that only reads
-        // bounded, already-piped data (e.g. `git show sha:path | rg -n foo`)
-        // is not denied in isolation when the full pipeline is exempt. This
-        // must run before the exact-full-command grant shortcut below — a
-        // stored grant must never let a hard-denied command skip straight
-        // past the check that would otherwise deny it (see preGrantGuardReason).
-        const blockReason = runShellAuthzBlockReason(fullCommand);
-        if (blockReason !== undefined) {
-          return { kind: "deny", reason: blockReason };
-        }
+        // Catastrophic commands were already hard-denied at the top of the
+        // verdict path before any grant shortcut could admit them.
 
         let needsOperator = false;
         let anySecret = false;
@@ -790,7 +796,7 @@ export function createPermissionGate(
             // so. Matching semantics are untouched; this only annotates the ask.
             if (
               mismatchNotice === undefined &&
-              (await evaluateApprovals({
+              (await approvalCoversSubject({
                 tool: request.tool,
                 subject: segment,
                 approvals,
@@ -804,7 +810,7 @@ export function createPermissionGate(
             continue;
           }
           if (
-            await evaluateApprovals({
+            await approvalCoversSubject({
               tool: request.tool,
               subject: segment,
               approvals,
@@ -865,7 +871,7 @@ export function createPermissionGate(
 
       // Path-arg tools already drop to ask via callTargetsRestricted; grants
       // match on the path subject the same as before.
-      const alreadyApproved = await evaluateApprovals({
+      const alreadyApproved = await approvalCoversSubject({
         tool: request.tool,
         subject: request.subject,
         approvals,