fix(exec): honor node approval floors before auto-review

joshavant · vincentkoc · jesse-merhi · jesse-merhi · commit 49ab52894a57 · 2026-05-30T00:04:06.000+10:00
Co-authored-by: Vincent Koc &lt;25068+vincentkoc@users.noreply.github.com&gt;
Co-authored-by: jesse-merhi &lt;79823012+jesse-merhi@users.noreply.github.com&gt;
diff --git a/src/agents/bash-tools.exec-host-node-phases.ts b/src/agents/bash-tools.exec-host-node-phases.ts
@@ -48,6 +48,8 @@ type NodeApprovalAnalysis = {
   analysisOk: boolean;
   allowlistSatisfied: boolean;
   durableApprovalSatisfied: boolean;
+  nodeSecurity?: ExecSecurity;
+  nodeAsk?: ExecAsk;
   inlineEvalHit: InterpreterInlineEvalHit | null;
   requiresSecurityAuditSuppressionApproval: boolean;
   autoReviewArgv?: string[];
@@ -314,6 +316,8 @@ export async function analyzeNodeApprovalRequirement(params: {
   let analysisOk = baseAllowlistEval.analysisOk;
   let allowlistSatisfied = false;
   let durableApprovalSatisfied = false;
+  let nodeSecurity: ExecSecurity | undefined;
+  let nodeAsk: ExecAsk | undefined;
   const inlineEvalHit =
     params.request.strictInlineEval === true
       ? detectPolicyInlineEval(baseAllowlistEval.segments)
@@ -349,6 +353,8 @@ export async function analyzeNodeApprovalRequirement(params: {
           agentId: params.request.agentId,
           overrides: { security: "full" },
         });
+        nodeSecurity = resolved.agent.security;
+        nodeAsk = resolved.agent.ask;
         // Allowlist-only precheck; safe bins are node-local and may diverge.
         const allowlistEval = evaluateShellAllowlist({
           command: params.request.command,
@@ -376,6 +382,8 @@ export async function analyzeNodeApprovalRequirement(params: {
     analysisOk,
     allowlistSatisfied,
     durableApprovalSatisfied,
+    nodeSecurity,
+    nodeAsk,
     inlineEvalHit,
     requiresSecurityAuditSuppressionApproval,
     autoReviewArgv:
diff --git a/src/agents/bash-tools.exec-host-node.test.ts b/src/agents/bash-tools.exec-host-node.test.ts
@@ -38,6 +38,18 @@ const evaluateShellAllowlistMock = vi.hoisted(() =>
     segmentAllowlistEntries: [],
   })),
 );
+const resolveExecApprovalsFromFileMock = vi.hoisted(() =>
+  vi.fn(() => ({
+    allowlist: [],
+    file: { version: 1, agents: {} },
+    agent: {
+      security: "full",
+      ask: "off",
+      askFallback: "deny",
+      autoAllowSkills: false,
+    },
+  })),
+);
 const requiresExecApprovalMock = vi.hoisted(() => vi.fn(() => true));
 const resolveExecHostApprovalContextMock = vi.hoisted(() =>
   vi.fn(() => ({
@@ -93,10 +105,7 @@ vi.mock("../infra/exec-approvals.js", () => ({
   hasDurableExecApproval: vi.fn(() => false),
   requiresExecApproval: requiresExecApprovalMock,
   resolveExecApprovalAllowedDecisions: vi.fn(() => ["allow-once", "allow-always", "deny"]),
-  resolveExecApprovalsFromFile: vi.fn(() => ({
-    allowlist: [],
-    file: { version: 1, agents: {} },
-  })),
+  resolveExecApprovalsFromFile: resolveExecApprovalsFromFileMock,
 }));
 
 vi.mock("../infra/command-analysis/inline-eval.js", () => ({
@@ -275,6 +284,17 @@ describe("executeNodeHostCommand", () => {
       segments: [{ resolution: null, argv: ["bun", "./script.ts"] }],
       segmentAllowlistEntries: [],
     });
+    resolveExecApprovalsFromFileMock.mockReset();
+    resolveExecApprovalsFromFileMock.mockReturnValue({
+      allowlist: [],
+      file: { version: 1, agents: {} },
+      agent: {
+        security: "full",
+        ask: "off",
+        askFallback: "deny",
+        autoAllowSkills: false,
+      },
+    });
     requiresExecApprovalMock.mockReset();
     requiresExecApprovalMock.mockReturnValue(true);
     resolveExecHostApprovalContextMock.mockReset();
@@ -460,6 +480,94 @@ describe("executeNodeHostCommand", () => {
     expect(warnings.join("\n")).toContain("needs a person");
   });
 
+  it.each([
+    {
+      name: "ask always",
+      nodeSecurity: "full",
+      nodeAsk: "always",
+    },
+    {
+      name: "deny security",
+      nodeSecurity: "deny",
+      nodeAsk: "off",
+    },
+  ] as const)(
+    "requests human approval when node policy has $name floor",
+    async ({ nodeSecurity, nodeAsk }) => {
+      const autoReviewer = vi.fn<ExecAutoReviewer>(async () => ({
+        decision: "allow-once",
+        risk: "low",
+        rationale: "test reviewer would allow it",
+      }));
+      resolveExecHostApprovalContextMock.mockReturnValue({
+        approvals: { allowlist: [], file: { version: 1, agents: {} } },
+        hostSecurity: "allowlist",
+        hostAsk: "on-miss",
+        askFallback: "deny",
+      });
+      resolveExecApprovalsFromFileMock.mockReturnValue({
+        allowlist: [],
+        file: { version: 1, agents: {} },
+        agent: {
+          security: nodeSecurity,
+          ask: nodeAsk,
+          askFallback: "deny",
+          autoAllowSkills: false,
+        },
+      });
+      callGatewayToolMock.mockImplementation(
+        async (method: string, _options: unknown, params: MockNodeInvokeParams | undefined) => {
+          if (method === "exec.approvals.node.get") {
+            return { file: { version: 1, agents: {} } };
+          }
+          if (method === "exec.approval.resolve") {
+            return { payload: {} };
+          }
+          if (method !== "node.invoke") {
+            throw new Error(`unexpected gateway method: ${method}`);
+          }
+          if (params?.command === "system.run.prepare") {
+            return { payload: { plan: preparedPlan } };
+          }
+          if (params?.command === "system.run") {
+            return {
+              payload: {
+                success: true,
+                stdout: "ok",
+                stderr: "",
+                exitCode: 0,
+                timedOut: false,
+              },
+            };
+          }
+          throw new Error(`unexpected node invoke command: ${String(params?.command)}`);
+        },
+      );
+
+      const result = await executeNodeHostCommand({
+        command: "bun ./script.ts",
+        workdir: "/tmp/work",
+        env: {},
+        security: "allowlist",
+        ask: "on-miss",
+        autoReview: true,
+        autoReviewer,
+        defaultTimeoutSec: 30,
+        approvalRunningNoticeMs: 0,
+        warnings: [],
+        agentId: "requested-agent",
+        sessionKey: "requested-session",
+      });
+
+      expect(result.details?.status).toBe("approval-pending");
+      expect(autoReviewer).not.toHaveBeenCalled();
+      expect(createAndRegisterDefaultExecApprovalRequestMock).toHaveBeenCalledTimes(1);
+      expect(
+        callGatewayToolMock.mock.calls.some(([method]) => method === "exec.approval.resolve"),
+      ).toBe(false);
+    },
+  );
+
   it("auto-reviews strict inline-eval commands before asking a human", async () => {
     const autoReviewer = vi.fn<ExecAutoReviewer>(async () => ({
       decision: "allow-once",
diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts
@@ -56,6 +56,29 @@ function resolveNodeAutoReviewReason(params: {
   return "approval-required";
 }
 
+function execSecurityFloorRank(security: ExecSecurity): number {
+  switch (security) {
+    case "full":
+      return 0;
+    case "allowlist":
+      return 1;
+    case "deny":
+      return 2;
+  }
+}
+
+function nodePolicyBlocksAutoReview(params: {
+  hostSecurity: ExecSecurity;
+  nodeSecurity?: ExecSecurity;
+  nodeAsk?: "off" | "on-miss" | "always";
+}): boolean {
+  return (
+    params.nodeAsk === "always" ||
+    (params.nodeSecurity !== undefined &&
+      execSecurityFloorRank(params.nodeSecurity) > execSecurityFloorRank(params.hostSecurity))
+  );
+}
+
 export async function executeNodeHostCommand(
   params: ExecuteNodeHostCommandParams,
 ): Promise<AgentToolResult<ExecToolDetails>> {
@@ -88,6 +111,8 @@ export async function executeNodeHostCommand(
     analysisOk,
     allowlistSatisfied,
     durableApprovalSatisfied,
+    nodeSecurity,
+    nodeAsk,
     inlineEvalHit,
     requiresSecurityAuditSuppressionApproval,
     autoReviewArgv,
@@ -142,6 +167,7 @@ export async function executeNodeHostCommand(
     if (
       params.autoReview === true &&
       hostAsk !== "always" &&
+      !nodePolicyBlocksAutoReview({ hostSecurity, nodeSecurity, nodeAsk }) &&
       !requiresSecurityAuditSuppressionApproval
     ) {
       const reviewer = params.autoReviewer ?? defaultExecAutoReviewer;
