fix(approval): settle retried and resumed approval decisions

Author: TheGreatAxios

src/exec/runner.ts

@@ -132,7 +132,13 @@ import { ID_PREFIX, LOG_NAMESPACE_ROOT } from "../branding.js";
 import type { ReactorEmittedEvent } from "@intx/inference";
 import { setAgentSourceUnlessClosed } from "../tui/agent-source-sync.js";
 import { ensureFreshInferenceSource } from "../subagent/refresh-inference-source.js";
-import { getToolApprovalBudget } from "../tui/tool-execution-watchdog.js";
+import {
+  MAX_TOOL_APPROVAL_PAUSE_MS,
+  getToolApprovalBudget,
+} from "../tui/tool-execution-watchdog.js";
+import { createApprovalDeliverer } from "../tui/approval-delivery.js";
+import { createCorrelationAcceptance } from "../tui/correlation-acceptance.js";
+import { APPROVAL_TIMEOUT_RESULT_TEXT } from "../permission/decline-markers.js";
 import { WorkflowHost } from "../workflows/host.js";
 
 const logger = getLogger([LOG_NAMESPACE_ROOT, "exec"]);
@@ -968,10 +974,20 @@ export async function runExec(config: Config): Promise<ExecResult> {
     // Consume-once gate for the compaction continuation emit: a replayed
     // duplicate of an already-answered emission must not re-deliver.
     const continuationGate = createContinuationGate();
+    // Approval-delivery parity with the TUI: the resume routes its deliver
+    // through the shared acceptance-tracked deliverer, and the stream sink
+    // below feeds acceptances so a delivered decision settles instead of
+    // timing out.
+    const approvalAcceptance = createCorrelationAcceptance();
+    const approvalDeliverer = createApprovalDeliverer({
+      deliverToAgent: (message) => activeAgent.deliver(message),
+      acceptance: approvalAcceptance,
+    });
     // Cycles persist to the context store only on inference.done; the recorder
     // keeps the in-flight cycle's text so an errored or aborted turn leaves
     // its partial output in partial.jsonl instead of vanishing.
     const sink = (event: ReactorEmittedEvent): void => {
+      approvalAcceptance.observe(event);
       // Chat-director reactor events (replacing the former onTasksChange /
       // onActivateTools closures). Exec mode has no live task panel or task
       // stdout output today (unlike the TUI's chrome zone) — debug logging
@@ -1062,6 +1078,10 @@ export async function runExec(config: Config): Promise<ExecResult> {
         resolveParkedCallId: (correlationId) =>
           resolveParkedCallIdFromStore(activeStorage, correlationId),
         gate: permissionGate,
+        deliver: async (message, stillCurrent) => {
+          if (!stillCurrent()) return;
+          await approvalDeliverer.deliver(message);
+        },
       }).handle(sendResult);
       sendCompleted = true;
       runError = runSink.getRunError();
@@ -1307,9 +1327,28 @@ async function promptPermission(
   });
   stderr.write(`  [${scopes.length + 1}] Deny\n`);
   const rl = createInterface({ input, output: stderr });
+  let timer: ReturnType<typeof setTimeout> | undefined;
   try {
-    const line = (await rl.question("Choice: ")).trim();
-    const n = Number(line);
+    // The reactor bounds an approval suspension (default 1h); the prompt must
+    // settle below that so the gate answers instead of the suspension timing
+    // out. The deadline reuses the tool-budget approval-pause ceiling.
+    const answer = rl.question("Choice: ");
+    const expired = new Promise<undefined>((resolve) => {
+      timer = setTimeout(() => resolve(undefined), MAX_TOOL_APPROVAL_PAUSE_MS);
+    });
+    const line = await Promise.race([answer, expired]);
+    if (line === undefined) {
+      // The deadline won: rl.close() in the finally below does not settle a
+      // pending question, so abandon the loser explicitly. It can never
+      // resolve after close; the attached settlement keeps it contained (no
+      // floating promise) if a future runtime ever settles it.
+      void answer.then(
+        () => undefined,
+        () => undefined,
+      );
+      return { allow: false, message: APPROVAL_TIMEOUT_RESULT_TEXT };
+    }
+    const n = Number(line.trim());
     if (!Number.isInteger(n) || n < 1 || n > scopes.length) {
       return { allow: false };
     }
@@ -1322,6 +1361,7 @@ async function promptPermission(
       ...(chosen.pattern !== null ? { persist: chosen } : {}),
     };
   } finally {
+    if (timer !== undefined) clearTimeout(timer);
     rl.close();
     if (pauseToken !== undefined) budget?.resume(pauseToken);
   }

src/permission/denial-memory.test.ts

@@ -0,0 +1,134 @@
+import { describe, expect, test } from "bun:test";
+import type { ToolCall } from "@intx/types/runtime";
+
+import { DenialMemory, stableRequestId } from "./denial-memory.js";
+
+const fetchCall = (id: string, url: string): ToolCall => ({
+  id,
+  name: "web_fetch",
+  arguments: { url, format: "markdown" },
+});
+
+describe("stableRequestId", () => {
+  test("same URL across distinct call ids shares one fingerprint", () => {
+    expect(
+      stableRequestId(fetchCall("call_0", "https://example.com/docs"), "/work"),
+    ).toBe(
+      stableRequestId(fetchCall("call_1", "https://example.com/docs"), "/work"),
+    );
+  });
+
+  test("different URLs have independent fingerprints", () => {
+    expect(
+      stableRequestId(fetchCall("call_0", "https://example.com/docs"), "/work"),
+    ).not.toBe(
+      stableRequestId(
+        fetchCall("call_1", "https://example.com/other"),
+        "/work",
+      ),
+    );
+  });
+
+  test("scheme/host case and trailing slashes normalize to one fingerprint", () => {
+    expect(
+      stableRequestId(fetchCall("call_0", "https://example.com/docs"), "/work"),
+    ).toBe(
+      stableRequestId(
+        fetchCall("call_1", "HTTPS://EXAMPLE.COM/docs/"),
+        "/work",
+      ),
+    );
+  });
+
+  // URL paths and queries are case-sensitive (RFC 3986): /Docs and /docs are
+  // distinct resources and must fingerprint distinctly.
+  test("case-distinct paths and queries have independent fingerprints", () => {
+    expect(
+      stableRequestId(fetchCall("call_0", "https://example.com/Docs"), "/work"),
+    ).not.toBe(
+      stableRequestId(fetchCall("call_1", "https://example.com/docs"), "/work"),
+    );
+    expect(
+      stableRequestId(
+        fetchCall("call_0", "https://example.com/search?q=ABC"),
+        "/work",
+      ),
+    ).not.toBe(
+      stableRequestId(
+        fetchCall("call_1", "https://example.com/search?q=abc"),
+        "/work",
+      ),
+    );
+  });
+
+  test("tool name participates in the fingerprint", () => {
+    const args = { url: "https://example.com/docs" };
+    expect(
+      stableRequestId({ id: "a", name: "web_fetch", arguments: args }, "/work"),
+    ).not.toBe(
+      stableRequestId(
+        { id: "a", name: "web_search", arguments: args },
+        "/work",
+      ),
+    );
+  });
+
+  test("same shell command across distinct call ids shares one fingerprint", () => {
+    const shellCall = (id: string, command: string): ToolCall => ({
+      id,
+      name: "run_shell",
+      arguments: { command },
+    });
+    expect(
+      stableRequestId(shellCall("call_0", "echo alpha && echo beta"), "/work"),
+    ).toBe(
+      stableRequestId(shellCall("call_1", "echo alpha && echo beta"), "/work"),
+    );
+    expect(
+      stableRequestId(shellCall("call_2", "echo alpha && echo beta"), "/work"),
+    ).not.toBe(stableRequestId(shellCall("call_3", "echo gamma"), "/work"));
+  });
+});
+
+describe("DenialMemory", () => {
+  test("a same-fingerprint retry returns the cached reason", () => {
+    const memory = new DenialMemory();
+    const first = stableRequestId(
+      fetchCall("call_0", "https://example.com/docs"),
+      "/work",
+    );
+    const retry = stableRequestId(
+      fetchCall("call_1", "https://example.com/docs"),
+      "/work",
+    );
+    expect(memory.isDenied(first)).toBeUndefined();
+    memory.record(first, "denied: needs approval");
+    expect(memory.isDenied(retry)).toBe("denied: needs approval");
+  });
+
+  test("distinct fingerprints are independent", () => {
+    const memory = new DenialMemory();
+    memory.record(
+      stableRequestId(fetchCall("call_0", "https://example.com/a"), "/work"),
+      "denied: needs approval",
+    );
+    expect(
+      memory.isDenied(
+        stableRequestId(fetchCall("call_1", "https://example.com/b"), "/work"),
+      ),
+    ).toBeUndefined();
+  });
+
+  test("first recorded reason wins; clear forgets every denial", () => {
+    const memory = new DenialMemory();
+    const stableId = stableRequestId(
+      fetchCall("call_0", "https://example.com/docs"),
+      "/work",
+    );
+    memory.record(stableId, "first");
+    memory.record(stableId, "second");
+    expect(memory.isDenied(stableId)).toBe("first");
+    memory.clear();
+    expect(memory.isDenied(stableId)).toBeUndefined();
+  });
+});

src/permission/denial-memory.ts

@@ -0,0 +1,78 @@
+// Stable denial fingerprint for the permission gate's denial memory.
+//
+// The reactor retries a denied ask-tier call with a fresh tool_call.id; the
+// fingerprint must be stable across those retries (same tool + same normalized
+// arguments) so the second decide() returns the identical cached reason instead
+// of re-evaluating and re-logging. call.id and correlationId never participate.
+
+import type { ToolCall } from "@intx/types/runtime";
+
+import { normalizePathArguments } from "../plugins/path-escape-plugin.js";
+import { splitChainedCommand } from "./command.js";
+import type { RootsProvider } from "./worktree-roots.js";
+
+const NULL_SEPARATOR = "\0";
+
+// URL paths and queries are case-sensitive (RFC 3986): only the scheme and
+// host normalize. The path, query, and fragment keep their case so distinct
+// resources fingerprint distinctly.
+function normalizeUrl(value: unknown): unknown {
+  if (typeof value !== "string") return value;
+  const trimmed = value.trim();
+  const match = /^(https?:\/\/)([^/?#]*)([\s\S]*)$/i.exec(trimmed);
+  if (match === null) return value;
+  const scheme = match[1] ?? "";
+  const authority = match[2] ?? "";
+  const rest = match[3] ?? "";
+  // Userinfo (rarely present) is case-sensitive; only the host lowercases.
+  const at = authority.lastIndexOf("@");
+  const userinfo = at === -1 ? "" : authority.slice(0, at + 1);
+  const host = (at === -1 ? authority : authority.slice(at + 1)).toLowerCase();
+  return `${scheme.toLowerCase()}${userinfo}${host}${rest}`.replace(/\/+$/, "");
+}
+
+function normalizeArguments(
+  args: Record<string, unknown>,
+  toolName: string,
+): Record<string, unknown> {
+  const out: Record<string, unknown> = {};
+  for (const [key, value] of Object.entries(args)) {
+    if (key === "url") {
+      out[key] = normalizeUrl(value);
+    } else if (toolName === "run_shell" && key === "command") {
+      out[key] = typeof value === "string" ? splitChainedCommand(value) : value;
+    } else {
+      out[key] = value;
+    }
+  }
+  return out;
+}
+
+export function stableRequestId(
+  call: ToolCall,
+  cwd: string = process.cwd(),
+  rootsProvider: RootsProvider = () => [],
+): string {
+  const identity = normalizePathArguments(call.arguments, cwd, rootsProvider);
+  return (
+    call.name +
+    NULL_SEPARATOR +
+    JSON.stringify(normalizeArguments(identity, call.name))
+  );
+}
+
+export class DenialMemory {
+  private readonly reasons = new Map<string, string>();
+
+  record(stableId: string, reason: string): void {
+    if (!this.reasons.has(stableId)) this.reasons.set(stableId, reason);
+  }
+
+  isDenied(stableId: string): string | undefined {
+    return this.reasons.get(stableId);
+  }
+
+  clear(): void {
+    this.reasons.clear();
+  }
+}