diff --git a/src/agent/director.test.ts b/src/agent/director.test.ts index 7e1b6061a..641949187 100644 --- a/src/agent/director.test.ts +++ b/src/agent/director.test.ts @@ -426,6 +426,195 @@ describe("ChatDirector inference-error recovery (CL-6910)", () => { ); expect(stale).toEqual([]); }); + + function keeperManageTasksTurn(): ReactorInboundEvent { + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [ + { + type: "tool_call", + id: "manage-tasks", + name: "manage_tasks", + arguments: { + action: "create", + tasks: [{ id: "t1", title: "work", status: "doing" }], + }, + }, + ], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + } + + function keeperManageTasksPlusSubmitTurn(): ReactorInboundEvent { + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [ + { + type: "tool_call", + id: "manage-tasks", + name: "manage_tasks", + arguments: { + action: "create", + tasks: [{ id: "t1", title: "work", status: "doing" }], + }, + }, + { + type: "tool_call", + id: "submit-1", + name: "submit_output", + arguments: { step: "step-1" }, + }, + ], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + } + + function keeperTextTurn(): ReactorInboundEvent { + return { + type: "inference.done", + turn: { + role: "assistant", + model: "test", + timestamp: 0, + content: [{ type: "text", text: "all set" }], + }, + usage: { input: 0, output: 0 }, + source: "test", + } as unknown as ReactorInboundEvent; + } + + function keeperTasksChanged(actions: ReactorAction[]): ReactorAction[] { + return actions.filter( + (a) => + a.type === "emit" && + (a as { eventType?: string }).eventType === CHAT_TASKS_CHANGED_EVENT, + ); + } + + // CL-7992 K1: every coordinator rail consulted on the turn boundary + // rethrows instead of degrading — a throwing rail rejects decide() and + // the next turn carries no stale task-change notifications. + test.each(["isActive", "currentStepIsGate", "currentStepId"] as const)( + "a throwing %s rail rejects the inference turn without leaking task-change emits", + async (rail) => { + const failure = new Error(`${rail} exploded`); + const coordinator = { + isActive: () => { + if (rail === "isActive") throw failure; + return true; + }, + currentStepIsGate: () => { + if (rail === "currentStepIsGate") throw failure; + return false; + }, + currentStepId: () => { + if (rail === "currentStepId") throw failure; + return null; + }, + directive: () => null, + handleToolDone: () => false, + } as unknown as WorkflowCoordinator; + const director = createChatDirector("system", [], {}); + director.setWorkflowCoordinator(coordinator); + const capabilities = makeCapabilities(); + + // The step-id rail only runs past a terminal base action, so it + // throws on a text turn; the earlier rails throw on a manage_tasks + // turn after it queues its task-change notification. + const triggering = + rail === "currentStepId" ? keeperTextTurn() : keeperManageTasksTurn(); + await expect( + director.decide(triggering, mockState, capabilities), + ).rejects.toBe(failure); + + director.setWorkflowCoordinator(undefined); + const actions = actionsArray( + await director.decide(keeperTextTurn(), mockState, capabilities), + ); + expect(keeperTasksChanged(actions)).toEqual([]); + }, + ); + + // CL-7992 K2: a mid-turn failure outside the coordinator still rejects + // the turn, but already-queued notifications survive for the next turn. + test("a non-coordinator mid-turn failure preserves queued task-change emits for the next turn", async () => { + const failure = new Error("tool execution exploded"); + const director = createChatDirector("system", [], {}); + const capabilities = makeCapabilities(); + const executing: ReactorCapabilities = { + ...capabilities, + executeTools: (): ReactorAction => { + throw failure; + }, + }; + + await expect( + director.decide(keeperManageTasksTurn(), mockState, executing), + ).rejects.toBe(failure); + + const actions = actionsArray( + await director.decide(keeperTextTurn(), mockState, capabilities), + ); + expect(keeperTasksChanged(actions)).toHaveLength(1); + }); + + // CL-7992 K2 variant: a throwing handleToolDone degrades (it takes no + // rethrow parameter) instead of marking the turn stale, so a later + // non-coordinator failure still preserves the queued notifications. + test("a throwing handleToolDone degrades without dropping preserved task-change emits", async () => { + let handleToolDoneSeen = false; + const coordinator = { + isActive: () => false, + currentStepIsGate: () => false, + currentStepId: () => null, + directive: () => null, + handleToolDone: (): boolean => { + handleToolDoneSeen = true; + throw new Error("coordinator completion exploded"); + }, + } as unknown as WorkflowCoordinator; + const director = createChatDirector("system", [], {}); + director.setWorkflowCoordinator(coordinator); + const capabilities = makeCapabilities(); + const executing: ReactorCapabilities = { + ...capabilities, + executeTools: (): ReactorAction => { + throw new Error("tool execution exploded"); + }, + }; + + await expect( + director.decide(keeperManageTasksPlusSubmitTurn(), mockState, executing), + ).rejects.toThrow("tool execution exploded"); + + const brokenState = { + get turns(): never { + throw new Error("history unavailable"); + }, + } as unknown as ReactorState; + await expect( + director.decide(toolDoneEvent("submit-1"), brokenState, capabilities), + ).rejects.toThrow("history unavailable"); + expect(handleToolDoneSeen).toBe(true); + + director.setWorkflowCoordinator(undefined); + const actions = actionsArray( + await director.decide(keeperTextTurn(), mockState, capabilities), + ); + expect(keeperTasksChanged(actions)).toHaveLength(1); + }); }); // CL-7973: the director's live source id (which stamps retry decisions so a diff --git a/src/exec/runner.ts b/src/exec/runner.ts index 07e607eb5..233bd8808 100644 --- a/src/exec/runner.ts +++ b/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 { // 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 { 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 | 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((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); } diff --git a/src/permission/denial-memory.test.ts b/src/permission/denial-memory.test.ts new file mode 100644 index 000000000..1b2a83343 --- /dev/null +++ b/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(); + }); +}); diff --git a/src/permission/denial-memory.ts b/src/permission/denial-memory.ts new file mode 100644 index 000000000..b92c58bc0 --- /dev/null +++ b/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, + toolName: string, +): Record { + const out: Record = {}; + 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(); + + 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(); + } +} diff --git a/src/permission/gate.ts b/src/permission/gate.ts index 121a67c5c..fc2ac9e12 100644 --- a/src/permission/gate.ts +++ b/src/permission/gate.ts @@ -44,6 +44,7 @@ import { type RootsProvider, } from "./worktree-roots.js"; import { OPERATOR_DECLINED_PREFIX } from "./decline-markers.js"; +import { DenialMemory, stableRequestId } from "./denial-memory.js"; import { getSubAgentIdentity } from "../subagent/identity-context.js"; import { PRODUCT_MUTATION_TOOLS } from "../agent/product-mutation-tools.js"; @@ -592,6 +593,9 @@ export function createPermissionGate( ), ); } + // A new grant can cover a previously-denied request — drop cached denies + // so the retry re-evaluates against the live approvals. + denialMemory.clear(); }; // An auto-mode (or non-interactive-unavailable) decision settles the @@ -627,6 +631,13 @@ export function createPermissionGate( { name: string; arguments: string; verdict: AuthorizeVerdict } >(); + // Same-turn denial memory: stable fingerprints of headless and + // operator-declined denies so a retry with a fresh tool_call.id returns the + // identical cached reason instead of re-evaluating. Cleared by reset() and + // by every state change that can flip a deny to an allow (new grants, + // re-seeded approvals, auto/skip toggles, provider-identity switches). + const denialMemory = new DenialMemory(); + // Non-blocking policy decision for one tool call: everything the gate owns — // tier pre-filter, auto rules, pre-grant guards, grants, headless denial — // resolved WITHOUT waiting on an operator. `ask` carries the fully-built @@ -671,6 +682,17 @@ export function createPermissionGate( const subAgentIdentity = getSubAgentIdentity(); const effectiveCwd = subAgentIdentity?.cwd ?? resolvedCwd; + // A same-turn retry of an already-denied request (fresh tool_call.id, same + // stable fingerprint) returns the identical cached reason instead of + // re-evaluating and re-logging. This check runs before the path-escape + // deny below: the decision is the same deny either way, but the reason + // text is the originally recorded one, not a freshly computed escape + // reason. + const stableId = stableRequestId(call, effectiveCwd, rootsProvider); + const cachedDenial = denialMemory.isDenied(stableId); + if (cachedDenial !== undefined) + return { kind: "deny", reason: cachedDenial }; + // Path-escape will hard-reject these at execution unless skipPermissions // (already returned allow above). Deny here rather than showing Accept for // a call that cannot succeed. Match the plugin stack: workers sandbox @@ -838,12 +860,11 @@ export function createPermissionGate( askRule ?? "non-interactive", "deny", ); - return { - kind: "deny", - reason: anySecret - ? `${request.action} references a sensitive path and requires operator approval, which is unavailable in a non-interactive run.` - : `${request.action} requires operator approval, which is unavailable in a non-interactive run. Re-run with --dangerously-skip-permissions to bypass, or narrow the action.`, - }; + const reason = anySecret + ? `${request.action} references a sensitive path and requires operator approval, which is unavailable in a non-interactive run.` + : `${request.action} requires operator approval, which is unavailable in a non-interactive run. Re-run with --dangerously-skip-permissions to bypass, or narrow the action.`; + denialMemory.record(stableId, reason); + return { kind: "deny", reason }; } // Secret-path shell must never mint a stored grant — even an exact match @@ -885,10 +906,9 @@ export function createPermissionGate( if (!interactive || requestApproval === undefined) { recordAutoDecision(request.tool, "non-interactive", "deny"); - return { - kind: "deny", - reason: `${request.action} requires operator approval, which is unavailable in a non-interactive run. Re-run with --dangerously-skip-permissions to bypass, or narrow the action.`, - }; + const reason = `${request.action} requires operator approval, which is unavailable in a non-interactive run. Re-run with --dangerously-skip-permissions to bypass, or narrow the action.`; + denialMemory.record(stableId, reason); + return { kind: "deny", reason }; } return { kind: "ask", request, anySecret: false, segmentCount: 0 }; @@ -942,6 +962,20 @@ export function createPermissionGate( return outcome; }; + // Operator-decline reason for a decided-ask request and its outcome. Shared + // by evaluate() (middleware) and resolveSuspended() (reactor) so a cached + // decline reads identically whichever path recorded it. + const declineReason = ( + request: PermissionRequest, + outcome: ApprovalOutcome | undefined, + ): string => { + const suffix = + outcome?.message !== undefined && outcome.message.length > 0 + ? ` — ${outcome.message}` + : ""; + return `${OPERATOR_DECLINED_PREFIX}${request.action} (${request.subject})${suffix}`; + }; + // Middleware path: blocking evaluation used by tool-runner consumers whose // calls never pass through the reactor (sub-agents). When the gate is // reactor-gated, gateToolCall uses executionVerdict instead of evaluate() so @@ -953,14 +987,27 @@ export function createPermissionGate( return { allowed: false, reason: decision.reason }; const outcome = await resolveInteractiveAsk(decision); if (outcome === undefined || !outcome.allow) { - const suffix = - outcome?.message !== undefined && outcome.message.length > 0 - ? ` — ${outcome.message}` - : ""; - return { - allowed: false, - reason: `${OPERATOR_DECLINED_PREFIX}${decision.request.action} (${decision.request.subject})${suffix}`, - }; + const reason = declineReason(decision.request, outcome); + // Cache operator declines so a same-turn retry (fresh tool_call.id, + // same stable fingerprint) denies with the identical reason instead of + // re-asking. Timeouts, aborts, and missing outcomes are never cached — + // the operator made no decision, so the retry must ask again (mirrors + // resolveSuspended). + if ( + outcome !== undefined && + !outcome.allow && + classifyOutcome(outcome) === "deny" + ) { + denialMemory.record( + stableRequestId( + call, + getSubAgentIdentity()?.cwd ?? resolvedCwd, + rootsProvider, + ), + reason, + ); + } + return { allowed: false, reason }; } return { allowed: true }; }; @@ -1021,20 +1068,40 @@ export function createPermissionGate( request.tool === "run_shell" && commandReferencesSensitivePath(request.subject, request.cwd) !== undefined; - return resolveInteractiveAsk( - { - kind: "ask", - request, - anySecret, - segmentCount: - request.tool === "run_shell" - ? splitChainedCommand(request.subject).filter( - (s) => !isShellCommentOnly(s), - ).length - : 0, - }, - stillCurrent, - ); + const decision = { + kind: "ask" as const, + request, + anySecret, + segmentCount: + request.tool === "run_shell" + ? splitChainedCommand(request.subject).filter( + (s) => !isShellCommentOnly(s), + ).length + : 0, + }; + return (async () => { + const outcome = await resolveInteractiveAsk(decision, stillCurrent); + // Cache operator declines so a same-turn reactor retry (fresh + // tool_call.id, same stable fingerprint) denies with the identical + // reason instead of re-prompting. Timeouts, aborts, and missing outcomes + // are never cached — the operator made no decision, so the retry must + // ask again. Invalidation (grant-mint/mode/identity clears) is unchanged. + if ( + outcome !== undefined && + !outcome.allow && + classifyOutcome(outcome) === "deny" + ) { + denialMemory.record( + stableRequestId( + { id: "", name: request.tool, arguments: request.arguments ?? {} }, + request.cwd ?? getSubAgentIdentity()?.cwd ?? resolvedCwd, + rootsProvider, + ), + declineReason(request, outcome), + ); + } + return outcome; + })(); }; const reset = (): void => { @@ -1044,6 +1111,7 @@ export function createPermissionGate( } sessionGrants.length = 0; authorizedByCallId.clear(); + denialMemory.clear(); }; const sameApproval = (a: Approval, b: Approval): boolean => @@ -1069,6 +1137,9 @@ export function createPermissionGate( const setSeededApprovals = (seeded: readonly Approval[]): void => { approvals.length = 0; approvals.push(...seeded, ...sessionGrants); + // Re-seeded approvals can cover previously-denied requests — cached + // denies must re-evaluate instead of serving stale reasons. + denialMemory.clear(); }; const registerMcpClient = (client: MCPClient): void => { @@ -1093,13 +1164,19 @@ export function createPermissionGate( getAuto: () => auto === true, setAuto: (value: boolean) => { auto = value; + // Mode changes can flip denies to allows — cached denies re-evaluate. + denialMemory.clear(); }, getSkipPermissions: () => skipPermissions, setSkipPermissions: (value: boolean) => { skipPermissions = value; + // Mode changes can flip denies to allows — cached denies re-evaluate. + denialMemory.clear(); }, setProviderIdentity: (nextProviderName: string, nextModel: string) => { activeProviderModel = `${nextProviderName}:${nextModel}`; + // Provider-model grants key off this identity — cached denies re-evaluate. + denialMemory.clear(); }, registerMcpClient, unregisterMcpServer, diff --git a/src/permission/permission.test.ts b/src/permission/permission.test.ts index d8d27d885..caf153155 100644 --- a/src/permission/permission.test.ts +++ b/src/permission/permission.test.ts @@ -27,6 +27,7 @@ import { isAutoAllowedShellCall, } from "./classify.js"; import { createPermissionGate } from "./gate.js"; +import { APPROVAL_TIMEOUT_RESULT_TEXT } from "./decline-markers.js"; import { createMcpToolPermissionRegistry, registerMcpClientTools, @@ -39,7 +40,7 @@ import { createPathRestriction, resolveWorkspacePath, } from "./path-restriction.js"; -import type { Approval, PermissionRequest } from "./types.js"; +import type { Approval, ApprovalOutcome, PermissionRequest } from "./types.js"; import { initTemporaryGitRepo } from "../../tests/helpers/temporary-git-repo.js"; import { secretGuardPlugin } from "../plugins/secret-guard-plugin.js"; import { pathEscapePlugin } from "../plugins/path-escape-plugin.js"; @@ -2286,6 +2287,204 @@ describe("createPermissionGate", () => { expect("reason" in verdict && verdict.reason.length > 0).toBe(true); }); + // CL-8002: the reactor retries a denied ask-tier call with a fresh + // tool_call.id. The retry must deny with the identical cached reason + // instead of re-evaluating, or the loop never settles. + test("headless denies a same-URL web_fetch retry with the identical reason", async () => { + const gate = createPermissionGate({ + approvals: [], + interactive: false, + skipPermissions: false, + reactorGated: false, + }); + const fetch = (id: string, url: string) => + gate.evaluate({ + id, + name: "web_fetch", + arguments: { url, format: "markdown" }, + }); + const first = await fetch("call_0", "https://example.com/docs"); + const retry = await fetch("call_1", "https://example.com/docs"); + if (first.allowed || retry.allowed) + throw new Error("expected both web_fetch calls denied"); + expect(retry.reason).toBe(first.reason); + }); + + // CL-8002: the reactor path suspends an ask-tier call, resolves the operator + // decline via resolveSuspended, then retries same-turn with a fresh + // tool_call.id. The retry must deny with the identical cached reason (the + // same text the middleware path records) and the operator is asked once. + test("reactor-path decline is cached: fresh-id retry denies without re-asking", async () => { + let asked = 0; + const gate = createPermissionGate({ + approvals: [], + interactive: true, + skipPermissions: false, + reactorGated: true, + requestApproval: async () => { + asked++; + return { allow: false }; + }, + }); + const args = { url: "https://example.com/docs", format: "markdown" }; + const first = await gate.authorizeCall({ + id: "call_0", + name: "web_fetch", + arguments: args, + }); + if (first.effect !== "ask") + throw new Error("expected the first call to suspend for approval"); + const outcome = await gate.resolveSuspended(first.request); + expect(outcome?.allow).toBe(false); + expect(asked).toBe(1); + const retry = await gate.authorizeCall({ + id: "call_1", + name: "web_fetch", + arguments: args, + }); + if (retry.effect !== "deny") + throw new Error("expected the retry denied from denial memory"); + let middlewareAsked = 0; + const middleware = createPermissionGate({ + approvals: [], + interactive: true, + skipPermissions: false, + reactorGated: false, + requestApproval: async () => { + middlewareAsked++; + return { allow: false }; + }, + }); + const verdict = await middleware.evaluate({ + id: "call_0", + name: "web_fetch", + arguments: args, + }); + if (verdict.allowed) + throw new Error("expected the middleware call declined"); + expect(middlewareAsked).toBe(1); + expect(retry.reason).toBe(verdict.reason); + const retryAgain = await gate.authorizeCall({ + id: "call_2", + name: "web_fetch", + arguments: args, + }); + if (retryAgain.effect !== "deny") + throw new Error("expected the second retry denied"); + expect(retryAgain.reason).toBe(retry.reason); + expect(asked).toBe(1); + }); + + // CL-8002: a reactor-path timeout is not an operator decision, so + // resolveSuspended must not cache it — the retry re-asks the operator. + test("reactor-path timeout is not cached: retry re-asks", async () => { + let asked = 0; + const gate = createPermissionGate({ + approvals: [], + interactive: true, + skipPermissions: false, + reactorGated: true, + requestApproval: async () => { + asked++; + return { allow: false, message: APPROVAL_TIMEOUT_RESULT_TEXT }; + }, + }); + const args = { url: "https://example.com/docs", format: "markdown" }; + const first = await gate.authorizeCall({ + id: "call_0", + name: "web_fetch", + arguments: args, + }); + if (first.effect !== "ask") + throw new Error("expected the first call to suspend for approval"); + const outcome = await gate.resolveSuspended(first.request); + expect(outcome?.allow).toBe(false); + expect(asked).toBe(1); + const retry = await gate.authorizeCall({ + id: "call_1", + name: "web_fetch", + arguments: args, + }); + if (retry.effect !== "ask") + throw new Error("expected the retry to re-ask after a timeout"); + expect(asked).toBe(1); + }); + + // The middleware path must mirror the reactor-path guard above: only a real + // operator decline populates denial memory. Timeouts, aborts, and missing + // outcomes are never cached — the operator made no decision, so a same-turn + // retry with a fresh tool_call.id must re-ask instead of denying from cache. + test("middleware-path timeout/abort/missing outcomes are not cached: retry re-asks", async () => { + const args = { url: "https://example.com/docs", format: "markdown" }; + const outcomes: { name: string; outcome: ApprovalOutcome | undefined }[] = [ + { + name: "timeout", + outcome: { allow: false, message: APPROVAL_TIMEOUT_RESULT_TEXT }, + }, + { + name: "abort", + outcome: { + allow: false, + message: "tool no longer running; permission request denied", + }, + }, + { name: "missing", outcome: undefined }, + ]; + for (const { name, outcome } of outcomes) { + let asked = 0; + const gate = createPermissionGate({ + approvals: [], + interactive: true, + skipPermissions: false, + reactorGated: false, + requestApproval: async () => { + asked++; + return outcome as ApprovalOutcome; + }, + }); + const first = await gate.evaluate({ + id: "call_0", + name: "web_fetch", + arguments: args, + }); + if (first.allowed) + throw new Error(`expected the first ${name} call denied`); + const retry = await gate.evaluate({ + id: "call_1", + name: "web_fetch", + arguments: args, + }); + if (retry.allowed) + throw new Error(`expected the ${name} retry denied after re-asking`); + expect(asked).toBe(2); + } + }); + + // CL-8002: distinct URLs deny independently, and reset() clears the denial + // memory so the next turn re-denies cleanly with no stale state. + test("headless denies distinct web_fetch URLs independently; reset clears denials", async () => { + const gate = createPermissionGate({ + approvals: [], + interactive: false, + skipPermissions: false, + reactorGated: false, + }); + const fetch = (id: string, url: string) => + gate.evaluate({ + id, + name: "web_fetch", + arguments: { url, format: "markdown" }, + }); + const first = await fetch("call_0", "https://example.com/first"); + const other = await fetch("call_1", "https://example.com/second"); + if (first.allowed || other.allowed) + throw new Error("expected both web_fetch calls denied"); + gate.reset(); + const again = await fetch("call_2", "https://example.com/first"); + if (again.allowed) throw new Error("expected the re-fetch denied"); + expect(again.reason).toBe(first.reason); + }); + // SECURITY: headless with requestApproval present but interactive=false must // still deny — interactive=false is the authoritative headless signal, not the // absence of the callback. diff --git a/src/permission/project-approvals-trust.test.ts b/src/permission/project-approvals-trust.test.ts index cbcdce418..9c2dbd9d7 100644 --- a/src/permission/project-approvals-trust.test.ts +++ b/src/permission/project-approvals-trust.test.ts @@ -303,6 +303,10 @@ describe("CL-7782: project approvals require grant trust", () => { ) ).allowed, ).toBe(false); - expect(asked).toEqual(["run_shell:npm test", "run_shell:npm test"]); + // CL-8002: the second evaluate is a same-turn retry of the already-declined + // request (same stable fingerprint), so the gate returns the cached decline + // instead of re-prompting. Both requests are still denied — nothing here + // auto-allows — but the operator is asked only once. + expect(asked).toEqual(["run_shell:npm test"]); }); }); diff --git a/src/session/approval-resume.test.ts b/src/session/approval-resume.test.ts index 558e9c442..899ddfacb 100644 --- a/src/session/approval-resume.test.ts +++ b/src/session/approval-resume.test.ts @@ -649,6 +649,16 @@ describe("approval resume retry re-await", () => { expect(delivered).toHaveLength(1); }); + test("distinct correlations gate and deliver independently", async () => { + const { resume, delivered, resolveSuspended } = retryHarness({ + allow: true, + }); + expect(await resume.handle(suspension("corr-A", "echo alpha"))).toBe(true); + expect(await resume.handle(suspension("corr-B", "echo beta"))).toBe(true); + expect(resolveSuspended).toHaveBeenCalledTimes(2); + expect(delivered).toHaveLength(2); + }); + test("retry without acceptance still gates", async () => { const delivered: InboundMessage[] = []; const deliver = mock((message: InboundMessage): void => { @@ -702,4 +712,34 @@ describe("approval resume retry re-await", () => { expect(resolveSuspended).toHaveBeenCalledTimes(1); expect(delivered).toHaveLength(1); }); + + // CL-7992 K3: concurrent duplicates share one in-flight rejection (one + // gate, zero deliveries), then a retry re-gates and delivers exactly + // once, and later retries memoize. + test("a shared rejection re-gates once, delivers once, then memoizes", async () => { + const gate = Promise.withResolvers<{ allow: boolean }>(); + const { resume, delivered, resolveSuspended } = retryHarness({ + allow: true, + }); + resolveSuspended.mockImplementationOnce(() => gate.promise); + const failure = new Error("gate exploded"); + const first = resume.handle(suspension("corr-A", "echo alpha")); + const second = resume.handle(suspension("corr-A", "echo alpha")); + for (let i = 0; i < 10; i++) await Promise.resolve(); + expect(resolveSuspended).toHaveBeenCalledTimes(1); + gate.reject(failure); + await expect(first).rejects.toBe(failure); + await expect(second).rejects.toBe(failure); + expect(resolveSuspended).toHaveBeenCalledTimes(1); + expect(delivered).toHaveLength(0); + + expect(await resume.handle(suspension("corr-A", "echo alpha"))).toBe(true); + expect(resolveSuspended).toHaveBeenCalledTimes(2); + expect(delivered).toHaveLength(1); + expect(decisionBody(onlyDecision(delivered)).outcome).toBe("approved"); + + expect(await resume.handle(suspension("corr-A", "echo alpha"))).toBe(true); + expect(resolveSuspended).toHaveBeenCalledTimes(2); + expect(delivered).toHaveLength(1); + }); }); diff --git a/src/session/approval-resume.ts b/src/session/approval-resume.ts index ab3c0a300..075a21d6c 100644 --- a/src/session/approval-resume.ts +++ b/src/session/approval-resume.ts @@ -256,7 +256,10 @@ export function createApprovalResume(args: { handle: (result) => { if (result.type !== "suspended") return Promise.resolve(false); const { correlationId } = result; - if (handedOver.has(correlationId)) return Promise.resolve(true); + if (handedOver.has(correlationId)) { + logger.warn`duplicate approval resume ignored correlation=${correlationId}`; + return Promise.resolve(true); + } const ongoing = inflight.get(correlationId); if (ongoing !== undefined) return ongoing; const task = settleSuspended(result); diff --git a/src/tools/web-fetch.test.ts b/src/tools/web-fetch.test.ts index c2a51e54f..ac10d78e5 100644 --- a/src/tools/web-fetch.test.ts +++ b/src/tools/web-fetch.test.ts @@ -6,6 +6,7 @@ import { createExaMCPWebFetchTool, createWebFetchTool, runWebFetch, + webFetchDefinition, MAX_FETCH_BYTES, } from "./web-fetch.js"; @@ -126,6 +127,33 @@ describe("runWebFetch", () => { }); }); +describe("webFetchDefinition", () => { + test("routes hosts covered by a connected MCP server through tool_search", () => { + expect(webFetchDefinition.description).toContain( + "covered by a connected MCP server", + ); + expect(webFetchDefinition.description).toContain("via tool_search"); + }); + + test("extracts the item identifier from the pasted URL host-plus-path", () => { + expect(webFetchDefinition.description).toContain( + "extracting the item identifier from the host-plus-path", + ); + }); + + test("scopes web_fetch to public refs, not authenticated app surfaces", () => { + expect(webFetchDefinition.description).toContain( + "not authenticated app surfaces", + ); + }); + + test("allows web_fetch when no covering MCP, MCP fails, or page is public", () => { + expect(webFetchDefinition.description).toContain( + "the MCP call fails, or the page is genuinely public", + ); + }); +}); + describe("createExaMCPWebFetchTool", () => { function createTool(call: MCPClient["call"]) { return createExaMCPWebFetchTool({ diff --git a/src/tools/web-fetch.ts b/src/tools/web-fetch.ts index 7f3e38d47..8a23c1a22 100644 --- a/src/tools/web-fetch.ts +++ b/src/tools/web-fetch.ts @@ -32,7 +32,7 @@ type WebFetchFormat = "text" | "markdown" | "html"; export const webFetchDefinition: ToolDefinition = { name: "web_fetch", description: - "Fetch a web page over HTTP(S) and return its content. Uses built-in Exa MCP by default, or a direct in-process fetch when that built-in is disabled or overridden. Converts HTML to markdown by default. Use for documentation, articles, and other external references.", + "Fetch a web page over HTTP(S) and return its content. Uses built-in Exa MCP by default, or a direct in-process fetch when that built-in is disabled or overridden. Converts HTML to markdown by default. Use for documentation, articles, and other external references. If a pasted URL belongs to a host covered by a connected MCP server, route it through that server via tool_search first, extracting the item identifier from the host-plus-path. web_fetch is for public docs, articles, and external references, not authenticated app surfaces. Use web_fetch when no covering MCP server exists, the MCP call fails, or the page is genuinely public.", inputSchema: { type: "object", properties: {