Merge pull request #1440 from corbitsdev/cl-10696-add-typed-parent-approval-for-pending-worker-denied-calls

fix(permissions): add typed parent approval

Author: TheGreatAxios

src/agent/tools.ts

@@ -87,6 +87,7 @@ import {
   createResumeAgentTool,
   createInterruptAgentTool,
   createSendInputTool,
+  createApproveWorkerCallTool,
 } from "../subagent/lifecycle-tools.js";
 import { createManageTasksRunner } from "./tasks.js";
 import { createSpillingBackgroundShellExitNotifier } from "./background-shell-tool.js";
@@ -618,6 +619,11 @@ export async function createAgentToolset(
         createResumeAgentTool({ sessions: fleetSessions, fleetRecords }),
         createInterruptAgentTool({ sessions: fleetSessions, fleetRecords }),
         createSendInputTool({ sessions: fleetSessions, fleetRecords }),
+        createApproveWorkerCallTool({
+          sessions: fleetSessions,
+          fleetRecords,
+          permissionGate: args.permissionGate,
+        }),
       );
       // Exec-primary opt-in only; TUI primary and nested orchestrators
       // collect via mailbox mail, so wait_agents stays unmounted there.

src/permission/reactor-authorize.test.ts

@@ -234,6 +234,33 @@ test("nested orchestrator wait_agents allows with only a spawn_agent grant", asy
   ).toBe("allow");
 });
 
+test("nested parent approval tool passes the worker authorization seam", async () => {
+  const policy = gate({ interactive: true, auto: false });
+  const authorize = createWorkerAuthorize(policy);
+  expect(
+    (
+      await authorize(
+        "tool:approve_worker_call",
+        "invoke",
+        namedCall("approve_worker_call", {
+          target: "child-session",
+          question_id: "child-question",
+          request_id: "child-request",
+        }),
+      )
+    ).effect,
+  ).toBe("allow");
+  expect(
+    (
+      await authorize(
+        "tool:write_file",
+        "invoke",
+        namedCall("write_file", { path: "ordinary.txt", content: "data" }),
+      )
+    ).effect,
+  ).toBe("deny");
+});
+
 test("worker spawn_agent still needs a parent grant", async () => {
   const policy = gate({ interactive: true, auto: false });
   expect(

src/permission/reactor-authorize.ts

@@ -23,7 +23,6 @@ import type { AuthorizeVerdict, GateVerdict, PermissionGate } from "./gate.js";
 import type { PermissionRequest } from "./types.js";
 import {
   createDeniedCallEnvelope,
-  fingerprintDeniedCall,
   formatWorkerDenyWithGrantId,
   getProcessWorkerGrantStore,
   type WorkerGrantStore,
@@ -164,17 +163,6 @@ function denyWorkerCallWithEnvelope(
   };
 }
 
-/** Mutex key covering the envelope match: concurrent identical worker calls
- * serialize through precheck-to-consume so one envelope allows exactly once. */
-function workerGrantKey(
-  sessionId: string,
-  call: ToolCallType,
-  cwd: string,
-): string {
-  const identity = workerCallIdentity(sessionId, call, cwd);
-  return `${sessionId}\n${fingerprintDeniedCall(identity.canonicalTool, identity.args, identity.cwd)}`;
-}
-
 async function authorizeWorkerCall(
   gate: PermissionGate,
   call: ToolCallType,
@@ -187,11 +175,13 @@ async function authorizeWorkerCall(
   if (sessionId === undefined)
     return authorizeWorkerCallInner(gate, call, grantOptions, workerCwd);
   const store = grantOptions?.store ?? getProcessWorkerGrantStore();
-  return store.runExclusive(workerGrantKey(sessionId, call, workerCwd), () =>
-    authorizeWorkerCallInner(gate, call, grantOptions, workerCwd, {
-      sessionId,
-      store,
-    }),
+  return store.runExclusiveIdentity(
+    workerCallIdentity(sessionId, call, workerCwd),
+    () =>
+      authorizeWorkerCallInner(gate, call, grantOptions, workerCwd, {
+        sessionId,
+        store,
+      }),
   );
 }
 
@@ -203,10 +193,10 @@ async function authorizeWorkerCallInner(
   grant?: { sessionId: string; store: WorkerGrantStore },
 ): Promise<{ effect: "allow" } | { effect: "deny"; reason: string }> {
   if (grant !== undefined) {
-    const precheck = grant.store.precheck(
-      workerCallIdentity(grant.sessionId, call, workerCwd),
-    );
+    const identity = workerCallIdentity(grant.sessionId, call, workerCwd);
+    const precheck = grant.store.precheck(identity);
     if (!precheck.ok) return { effect: "deny", reason: precheck.blocker };
+    if (grant.store.hasAuthorizedRetry(identity)) return { effect: "allow" };
   }
   const verdict = await gate.authorizeCall(call);
   // Authorize never consumes: it only permits the call to proceed to the
@@ -248,11 +238,13 @@ async function executionVerdictWorkerCall(
   if (sessionId === undefined)
     return executionVerdictWorkerCallInner(gate, call, grantOptions, workerCwd);
   const store = grantOptions?.store ?? getProcessWorkerGrantStore();
-  return store.runExclusive(workerGrantKey(sessionId, call, workerCwd), () =>
-    executionVerdictWorkerCallInner(gate, call, grantOptions, workerCwd, {
-      sessionId,
-      store,
-    }),
+  return store.runExclusiveIdentity(
+    workerCallIdentity(sessionId, call, workerCwd),
+    () =>
+      executionVerdictWorkerCallInner(gate, call, grantOptions, workerCwd, {
+        sessionId,
+        store,
+      }),
   );
 }
 
@@ -264,10 +256,11 @@ async function executionVerdictWorkerCallInner(
   grant?: { sessionId: string; store: WorkerGrantStore },
 ): Promise<AuthorizeVerdict> {
   if (grant !== undefined) {
-    const precheck = grant.store.precheck(
-      workerCallIdentity(grant.sessionId, call, workerCwd),
-    );
+    const identity = workerCallIdentity(grant.sessionId, call, workerCwd);
+    const precheck = grant.store.precheck(identity);
     if (!precheck.ok) return { effect: "deny", reason: precheck.blocker };
+    if (grant.store.consumeAuthorizedRetry(identity) !== undefined)
+      return { effect: "allow" };
   }
   const verdict = await gate.executionVerdict(call);
   // Sole consumption point: the execution backstop spends the envelope when

src/permission/worker-grant-flow.test.ts

@@ -186,7 +186,7 @@ describe("worker grant-request flow: deny → parent replay grant → one retry"
     expect(store.peek(requestId)?.status).toBe("pending");
   });
 
-  test("send_input text answers the ask but grants nothing", () => {
+  test("send_input text answers an unbound ask but grants nothing", () => {
     const sessions = createSubAgentSessionStore();
     const session = sessions.start({
       id: "fleet-1",
@@ -221,9 +221,8 @@ describe("worker grant-request flow: deny → parent replay grant → one retry"
         },
       }),
     ).toBe(true);
-    expect(sessions.peekAsk(session.id)?.deniedCall?.requestId).toBe(
-      envelope.requestId,
-    );
+    // A request ID quoted in prose is not an authority-bearing binding.
+    expect(sessions.peekAsk(session.id)?.deniedCall).toBeUndefined();
 
     const echo = JSON.stringify({
       requestId: envelope.requestId,

src/permission/worker-grant.test.ts

@@ -45,12 +45,15 @@ describe("fingerprintDeniedCall", () => {
     expect(a).toBe(b);
   });
 
-  test("differs across args and tool (cwd binds at precheck, not in the hash)", () => {
+  test("differs across args, tool, and cwd", () => {
     const base = fingerprintDeniedCall("run_shell", ARGS, CWD);
     expect(
       fingerprintDeniedCall("run_shell", { command: "npm run evil" }, CWD),
     ).not.toBe(base);
     expect(fingerprintDeniedCall("read_file", ARGS, CWD)).not.toBe(base);
+    expect(fingerprintDeniedCall("run_shell", ARGS, "/elsewhere")).not.toBe(
+      base,
+    );
     expect(
       fingerprintDeniedCall("read_file", { path: "a.md" }, "/repo") ===
         fingerprintDeniedCall("read_file", { path: "b.md" }, "/repo"),
@@ -283,19 +286,32 @@ describe("WorkerGrantStore lifecycle", () => {
     expect(store.invalidateSession("worker-1", "again")).toBe(0);
   });
 
-  test("attachToAsk stamps the questionId and keeps pending", () => {
+  test("attachToAsk binds only an explicit requestId and keeps it pending", () => {
     const store = new WorkerGrantStore();
     const envelope = store.register(createDeniedCallEnvelope(descriptor()));
     // Reads pin the clock: fixture now predates wall time, so an unpinned
     // read would sweep the envelope as expired.
-    expect(store.attachToAsk("worker-1", "ask-7", undefined, 1_000_001)).toBe(
-      envelope,
-    );
-    expect(envelope.questionId).toBe("ask-7");
+    expect(
+      store.attachToAsk("worker-1", "ask-generic", undefined, 1_000_001),
+    ).toBeUndefined();
+    expect(envelope.questionId).toBeUndefined();
     expect(envelope.status).toBe("pending");
     expect(store.peek(envelope.requestId)).toBe(envelope);
     expect(
-      store.attachToAsk("worker-9", "ask-8", undefined, 1_000_001),
+      store.attachToAsk("worker-1", "ask-7", envelope.requestId, 1_000_001),
+    ).toBe(envelope);
+    expect(envelope.questionId).toBe("ask-7");
+    expect(
+      store.attachToAsk(
+        "worker-1",
+        "ask-rebound",
+        envelope.requestId,
+        1_000_001,
+      ),
+    ).toBeUndefined();
+    expect(envelope.questionId).toBe("ask-7");
+    expect(
+      store.attachToAsk("worker-9", "ask-8", envelope.requestId, 1_000_001),
     ).toBeUndefined();
     expect(envelope.audit.map((event) => event.event)).toEqual([
       "denied",
@@ -329,12 +345,12 @@ describe("WorkerGrantStore lifecycle", () => {
       expect(store.peek(envelopeB.requestId)?.questionId).toBe("ask-B");
     });
 
-    test("unnamed ask keeps the legacy first-pending bind", () => {
+    test("unnamed ask does not bind a pending denial", () => {
       const { store, envelopeA, envelopeB } = twoDenies();
-      expect(store.attachToAsk("worker-1", "ask-x", undefined, 1_000_001)).toBe(
-        envelopeA,
-      );
-      expect(envelopeA.questionId).toBe("ask-x");
+      expect(
+        store.attachToAsk("worker-1", "ask-x", undefined, 1_000_001),
+      ).toBeUndefined();
+      expect(envelopeA.questionId).toBeUndefined();
       expect(envelopeB.questionId).toBeUndefined();
     });
 
@@ -370,7 +386,7 @@ describe("WorkerGrantStore lifecycle", () => {
   test("audit trail orders deny → ask → consume", () => {
     const store = new WorkerGrantStore();
     const envelope = store.register(createDeniedCallEnvelope(descriptor()));
-    store.attachToAsk("worker-1", "ask-1", undefined, 1_000_001);
+    store.attachToAsk("worker-1", "ask-1", envelope.requestId, 1_000_001);
     store.consumeOnAllow({
       sessionId: "worker-1",
       canonicalTool: "run_shell",
@@ -385,6 +401,25 @@ describe("WorkerGrantStore lifecycle", () => {
     ]);
   });
 
+  test("revokeAuthorizedRetry returns authorized to pending and audits revoked", () => {
+    const store = new WorkerGrantStore();
+    const envelope = store.register(createDeniedCallEnvelope(descriptor()));
+    expect(store.authorizeRetry(envelope.requestId, 1_000_001)).toBe(true);
+    store.revokeAuthorizedRetry(envelope.requestId);
+    expect(envelope.status).toBe("pending");
+    expect(envelope.audit.map((event) => event.event)).toEqual([
+      "denied",
+      "authorized",
+      "revoked",
+    ]);
+    store.revokeAuthorizedRetry(envelope.requestId);
+    expect(envelope.audit.map((event) => event.event)).toEqual([
+      "denied",
+      "authorized",
+      "revoked",
+    ]);
+  });
+
   test("no text API: state moves only on typed identities, never prose", () => {
     const store = new WorkerGrantStore();
     const envelope = store.register(createDeniedCallEnvelope(descriptor()));