refactor(agents): delete duplicate node approval lifecycle (#115418)

src/agents/bash-tools.exec-host-node-followup.ts

@@ -1,81 +0,0 @@
-import type { InterpreterInlineEvalHit } from "../infra/command-analysis/inline-eval.js";
-import type { ExecSecurity } from "../infra/exec-approvals.js";
-import type { ExecAutoReviewInput } from "../infra/exec-auto-review.js";
-import type { sendExecApprovalFollowupResult } from "./bash-tools.exec-host-shared.js";
-
-type SendExecApprovalFollowupResult = typeof sendExecApprovalFollowupResult;
-
-function execSecurityFloorRank(security: ExecSecurity): number {
-  switch (security) {
-    case "full":
-      return 0;
-    case "allowlist":
-      return 1;
-    case "deny":
-      return 2;
-  }
-  throw new Error("Unsupported exec security floor");
-}
-
-export function nodePolicyBlocksAutoReview(params: {
-  hostSecurity: ExecSecurity;
-  nodeApprovalPolicyKnown: boolean;
-  nodeSecurity?: ExecSecurity;
-  nodeAsk?: "off" | "on-miss" | "always";
-}): boolean {
-  // Remote policy may be stricter; local auto-review cannot bypass that floor.
-  return (
-    !params.nodeApprovalPolicyKnown ||
-    params.nodeAsk === "always" ||
-    (params.nodeSecurity !== undefined &&
-      execSecurityFloorRank(params.nodeSecurity) > execSecurityFloorRank(params.hostSecurity))
-  );
-}
-
-export function resolveNodeAutoReviewReason(params: {
-  inlineEvalHit: InterpreterInlineEvalHit | null;
-  hostSecurity: ExecSecurity;
-  analysisOk: boolean;
-  allowlistSatisfied: boolean;
-  durableApprovalSatisfied: boolean;
-}): ExecAutoReviewInput["reason"] {
-  if (params.inlineEvalHit !== null) {
-    return "strict-inline-eval";
-  }
-  if (
-    params.hostSecurity === "allowlist" &&
-    (!params.analysisOk || !params.allowlistSatisfied) &&
-    !params.durableApprovalSatisfied
-  ) {
-    return "allowlist-miss";
-  }
-  return "approval-required";
-}
-
-export function createNodeApprovalRequestFailureFollowup(params: {
-  send: SendExecApprovalFollowupResult;
-  target: Parameters<SendExecApprovalFollowupResult>[0];
-  nodeId: string;
-  approvalId: string;
-  command: string;
-  signal?: AbortSignal;
-}): () => Promise<void> {
-  const message = `Exec denied (node=${params.nodeId} id=${params.approvalId}, approval-request-failed): ${params.command}`;
-
-  return async () => {
-    if (params.signal?.aborted) {
-      return;
-    }
-    try {
-      await params.send(params.target, message);
-    } catch {
-      if (!params.signal?.aborted) {
-        try {
-          await params.send(params.target, message);
-        } catch {
-          // The delivery owner already records failures; detached work must settle.
-        }
-      }
-    }
-  };
-}

src/agents/bash-tools.exec-host-node.cancellation.test.ts

@@ -29,6 +29,10 @@ vi.mock("../infra/exec-approvals.js", () => ({
     const priority: Record<ExecAsk, number> = { off: 0, "on-miss": 1, always: 2 };
     return priority[left] >= priority[right] ? left : right;
   },
+  minSecurity: (left: ExecSecurity, right: ExecSecurity) => {
+    const priority: Record<ExecSecurity, number> = { deny: 0, allowlist: 1, full: 2 };
+    return priority[left] <= priority[right] ? left : right;
+  },
   requiresExecApproval: mocks.requiresExecApproval,
   resolveExecApprovalAllowedDecisions: vi.fn(() => ["allow-once", "allow-always", "deny"]),
   resolveExecApprovalUnavailableDecisions: vi.fn(() => []),

src/agents/bash-tools.exec-host-node.ts

@@ -9,6 +9,7 @@ import {
   type ExecAsk,
   type ExecSecurity,
   maxAsk,
+  minSecurity,
   requiresExecApproval,
   resolveExecApprovalAllowedDecisions,
   resolveExecApprovalUnavailableDecisions,
@@ -24,11 +25,6 @@ import {
   isExecApprovalRunAbortedError,
   registerExecApprovalRequestForHostOrThrow,
 } from "./bash-tools.exec-approval-request.js";
-import {
-  createNodeApprovalRequestFailureFollowup,
-  nodePolicyBlocksAutoReview,
-  resolveNodeAutoReviewReason,
-} from "./bash-tools.exec-host-node-followup.js";
 import {
   analyzeNodeApprovalRequirement,
   buildNodeSystemRunInvoke,
@@ -318,15 +314,13 @@ export async function executeNodeHostCommand(
   let inlineFallbackPolicy: NodeGatewayPolicyCheckpoint | undefined;
   if (requiresAsk) {
     const autoReviewHasBoundCommand = analysisOk && autoReviewArgv !== undefined;
+    // Remote policy may be stricter; local auto-review cannot bypass that floor.
     const autoReviewBlockedByNodePolicy =
       params.autoReview === true &&
       hostAsk !== "always" &&
-      nodePolicyBlocksAutoReview({
-        hostSecurity,
-        nodeApprovalPolicyKnown,
-        nodeSecurity,
-        nodeAsk,
-      });
+      (!nodeApprovalPolicyKnown ||
+        nodeAsk === "always" ||
+        (nodeSecurity !== undefined && minSecurity(hostSecurity, nodeSecurity) !== hostSecurity));
     let autoReviewRequiresHumanApproval =
       autoReviewBlockedByNodePolicy ||
       (params.autoReview === true && hostAsk !== "always" && !autoReviewHasBoundCommand) ||
@@ -339,19 +333,21 @@ export async function executeNodeHostCommand(
       !requiresSecurityAuditSuppressionApproval
     ) {
       const reviewer = params.autoReviewer ?? defaultExecAutoReviewer;
+      const autoReviewReason =
+        inlineEvalHit !== null
+          ? "strict-inline-eval"
+          : hostSecurity === "allowlist" &&
+              (!analysisOk || !allowlistSatisfied) &&
+              !durableApprovalSatisfied
+            ? "allowlist-miss"
+            : "approval-required";
       const pendingDecision = resolveExecAutoReviewDecision(reviewer, {
         command: prepared.rawCommand,
         argv: autoReviewArgv,
         cwd: prepared.cwd,
         envKeys: Object.keys(params.requestedEnv ?? {}).toSorted(),
         host: "node",
-        reason: resolveNodeAutoReviewReason({
-          inlineEvalHit,
-          hostSecurity,
-          analysisOk,
-          allowlistSatisfied,
-          durableApprovalSatisfied,
-        }),
+        reason: autoReviewReason,
         analysis: {
           parsed: analysisOk,
           allowlistMatched: allowlistSatisfied,
@@ -484,14 +480,14 @@ export async function executeNodeHostCommand(
           turnSourceAccountId: params.turnSourceAccountId,
           turnSourceThreadId: params.turnSourceThreadId,
         });
-        const sendApprovalRequestFailedFollowup = createNodeApprovalRequestFailureFollowup({
-          send: execHostShared.sendExecApprovalFollowupResult,
-          target: followupTarget,
-          nodeId: target.nodeId,
-          approvalId,
-          command: params.command,
-          signal: params.signal,
-        });
+        const sendApprovalRequestFailedFollowup = async (): Promise<void> => {
+          if (!params.signal?.aborted) {
+            await execHostShared.sendExecApprovalFollowupResult(
+              followupTarget,
+              `Exec denied (node=${target.nodeId} id=${approvalId}, approval-request-failed): ${params.command}`,
+            );
+          }
+        };
         let nodeInvocationStarted = false;
         let nodeInvocationCompleted = false;