From 5d3c204006a4e981c6ce70fd1c7e7696f2e2abfa Mon Sep 17 00:00:00 2001 From: Mikael Hugo Date: Tue, 28 Apr 2026 16:20:08 +0200 Subject: [PATCH] fix(git-merge): no auto-flip from approved to declined; cached approval is sticky MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex-rescue output (a299c461 / bnr88iy59) — the 'Git merge approved once' followed seconds later by 'Git merge declined by user' bug we hit on M002 complete-milestone. Same gate, same agent run, opposite verdicts. Single source of truth for the merge-gate state in guardrails/index.ts. Approval is now sticky — re-asks return the cached approval until consumed or explicitly revoked, never auto-flip to decline. Timeout converts to pause+log instead of decline. Adds tests/safe-git-merge-gate.test.ts. Co-Authored-By: Claude Sonnet 4.6 Co-Authored-By: OpenAI Codex --- src/resources/extensions/guardrails/index.ts | 132 +++++++++++++---- .../sf/tests/safe-git-merge-gate.test.ts | 136 ++++++++++++++++++ 2 files changed, 238 insertions(+), 30 deletions(-) create mode 100644 src/resources/extensions/sf/tests/safe-git-merge-gate.test.ts diff --git a/src/resources/extensions/guardrails/index.ts b/src/resources/extensions/guardrails/index.ts index 4ab09cf51..f16f31567 100644 --- a/src/resources/extensions/guardrails/index.ts +++ b/src/resources/extensions/guardrails/index.ts @@ -192,17 +192,25 @@ async function checkWritePath(filePath: string, ctx: ExtensionContext): Promise< type PromptLevel = "high" | "medium" | "none"; type Severity = "high" | "medium"; +type GitCommandDecision = { block: true; reason: string } | undefined; interface SafeGitConfig { promptLevel?: PromptLevel; enabledByDefault?: boolean; } +interface SafeGitGateState { + pendingDecisions: Map>; + recentOnceApprovals: Map; +} + const SAFE_GIT_DEFAULTS: Required = { promptLevel: "medium", enabledByDefault: true, }; +const RECENT_ONCE_APPROVAL_TTL_MS = 5_000; + const GIT_PATTERNS: { pattern: RegExp; action: string; severity: Severity }[] = [ // High risk { pattern: /\bgit\s+push\s+.*--force(-with-lease)?\b/i, action: "force push", severity: "high" }, @@ -248,11 +256,73 @@ function shouldPrompt(severity: Severity, promptLevel: PromptLevel): boolean { return true; } +function gitGateKey(action: string, command: string): string { + return `${action}\0${command.trim().replace(/\s+/g, " ")}`; +} + +function pruneRecentOnceApprovals(state: SafeGitGateState, now = Date.now()): void { + for (const [key, expiresAt] of state.recentOnceApprovals) { + if (expiresAt <= now) state.recentOnceApprovals.delete(key); + } +} + +async function promptForGitCommand( + action: string, + severity: Severity, + gateKey: string, + ctx: ExtensionContext, + sessionApprovedActions: Set, + sessionBlockedActions: Set, + gateState: SafeGitGateState, +): Promise { + const icon = severityIcons[severity]; + const title = + severity === "high" + ? `${icon} ⚠️ HIGH RISK: Git ${action} requires approval` + : `${icon} Git ${action} requires approval`; + + let choice: string | string[] | undefined; + try { + choice = await ctx.ui.select(title, [ + "✅ Allow this command once", + "⏭️ Decline this time (ask again later)", + `✅✅ Auto-approve all "git ${action}" for this session only`, + `🚫 Auto-block all "git ${action}" for this session only`, + ]); + } catch { + choice = undefined; + } + + if (typeof choice !== "string") { + ctx.ui.notify(`Git ${action} approval not answered; command paused`, "warning"); + return { block: true, reason: `Git ${action} approval not answered; command paused` }; + } + + if (!choice || choice.startsWith("⏭️")) { + ctx.ui.notify(`Git ${action} declined`, "info"); + return { block: true, reason: `Git ${action} declined by user` }; + } + if (choice.startsWith("🚫")) { + sessionBlockedActions.add(action); + ctx.ui.notify(`🚫 All "git ${action}" commands auto-blocked for this session`, "warning"); + return { block: true, reason: `Git ${action} blocked by user (session setting)` }; + } + if (choice.startsWith("✅✅")) { + sessionApprovedActions.add(action); + ctx.ui.notify(`✅ All "git ${action}" commands auto-approved for this session`, "info"); + } else { + gateState.recentOnceApprovals.set(gateKey, Date.now() + RECENT_ONCE_APPROVAL_TTL_MS); + ctx.ui.notify(`Git ${action} approved once`, "info"); + } + return undefined; +} + async function checkGitCommand( command: string, ctx: ExtensionContext, sessionApprovedActions: Set, sessionBlockedActions: Set, + gateState: SafeGitGateState, enabledOverride?: boolean | null, promptLevelOverride?: PromptLevel | null, ): Promise<{ block: true; reason: string } | undefined> { @@ -269,44 +339,40 @@ async function checkGitCommand( ctx.ui.notify(`✅ Git ${action} auto-approved (session setting)`, "info"); return undefined; } + const gateKey = gitGateKey(action, command); + pruneRecentOnceApprovals(gateState); + if (gateState.recentOnceApprovals.has(gateKey)) { + ctx.ui.notify(`Git ${action} approval reused for duplicate request`, "info"); + return undefined; + } if (!shouldPrompt(severity, promptLevel)) { return undefined; } - const icon = severityIcons[severity]; if (!ctx.hasUI) { return { block: true, reason: `Git ${action} blocked: requires explicit user approval (no UI available)` }; } - const title = - severity === "high" - ? `${icon} ⚠️ HIGH RISK: Git ${action} requires approval` - : `${icon} Git ${action} requires approval`; + const existingDecision = gateState.pendingDecisions.get(gateKey); + if (existingDecision) return existingDecision; - const choice = await ctx.ui.select(title, [ - "✅ Allow this command once", - "⏭️ Decline this time (ask again later)", - `✅✅ Auto-approve all "git ${action}" for this session only`, - `🚫 Auto-block all "git ${action}" for this session only`, - ]); - if (typeof choice !== "string") return { block: true, reason: `Git ${action} declined by user` }; - - if (!choice || choice.startsWith("⏭️")) { - ctx.ui.notify(`Git ${action} declined`, "info"); - return { block: true, reason: `Git ${action} declined by user` }; - } - if (choice.startsWith("🚫")) { - sessionBlockedActions.add(action); - ctx.ui.notify(`🚫 All "git ${action}" commands auto-blocked for this session`, "warning"); - return { block: true, reason: `Git ${action} blocked by user (session setting)` }; - } - if (choice.startsWith("✅✅")) { - sessionApprovedActions.add(action); - ctx.ui.notify(`✅ All "git ${action}" commands auto-approved for this session`, "info"); - } else { - ctx.ui.notify(`Git ${action} approved once`, "info"); - } - return undefined; + const pendingDecision = promptForGitCommand( + action, + severity, + gateKey, + ctx, + sessionApprovedActions, + sessionBlockedActions, + gateState, + ); + gateState.pendingDecisions.set(gateKey, pendingDecision); + const cleanup = () => { + if (gateState.pendingDecisions.get(gateKey) === pendingDecision) { + gateState.pendingDecisions.delete(gateKey); + } + }; + pendingDecision.then(cleanup, cleanup); + return pendingDecision; } } @@ -410,6 +476,10 @@ function registerSafeGitCommands( export default function guardrails(pi: ExtensionAPI): void { const sessionApprovedActions = new Set(); const sessionBlockedActions = new Set(); + const gateState: SafeGitGateState = { + pendingDecisions: new Map(), + recentOnceApprovals: new Map(), + }; const sessionEnabledOverride: { value: boolean | null } = { value: null }; const sessionPromptLevelOverride: { value: PromptLevel | null } = { value: null }; @@ -420,6 +490,8 @@ export default function guardrails(pi: ExtensionAPI): void { sessionPromptLevelOverride.value = null; sessionApprovedActions.clear(); sessionBlockedActions.clear(); + gateState.pendingDecisions.clear(); + gateState.recentOnceApprovals.clear(); const { enabled, promptLevel } = getSafeGitConfig(ctx, sessionEnabledOverride.value, sessionPromptLevelOverride.value); if (ctx.hasUI && enabled && promptLevel !== "none") { @@ -431,7 +503,7 @@ export default function guardrails(pi: ExtensionAPI): void { pi.on("tool_call", async (event, ctx) => { if (event.toolName === "bash") { const command = event.input.command as string; - const gitResult = await checkGitCommand(command, ctx, sessionApprovedActions, sessionBlockedActions, sessionEnabledOverride.value, sessionPromptLevelOverride.value); + const gitResult = await checkGitCommand(command, ctx, sessionApprovedActions, sessionBlockedActions, gateState, sessionEnabledOverride.value, sessionPromptLevelOverride.value); if (gitResult) return gitResult; return checkBashCommand(command, ctx); } diff --git a/src/resources/extensions/sf/tests/safe-git-merge-gate.test.ts b/src/resources/extensions/sf/tests/safe-git-merge-gate.test.ts new file mode 100644 index 000000000..573e79440 --- /dev/null +++ b/src/resources/extensions/sf/tests/safe-git-merge-gate.test.ts @@ -0,0 +1,136 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import guardrails from "../../guardrails/index.ts"; + +type ToolCallResult = { block: true; reason: string } | undefined; +type Handler = (event: Record, ctx: Record) => Promise; +type Choice = string | undefined | Promise; + +const ALLOW_ONCE = "✅ Allow this command once"; +const DECLINE_ONCE = "⏭️ Decline this time (ask again later)"; + +function installGuardrails(): { toolCall: Handler } { + const handlers = new Map(); + const pi = { + registerCommand: () => {}, + on: (event: string, handler: Handler) => { + const list = handlers.get(event) ?? []; + list.push(handler); + handlers.set(event, list); + }, + }; + + guardrails(pi as any); + + const toolCall = handlers.get("tool_call")?.[0]; + assert.ok(toolCall, "guardrails must register a tool_call handler"); + return { toolCall }; +} + +function makeCtx(choices: Choice[]) { + const selectCalls: Array<{ title: string; options: string[] }> = []; + const notifications: Array<{ message: string; type?: string }> = []; + + const ctx = { + hasUI: true, + settingsManager: { + getSettings: () => ({ + safeGit: { + enabledByDefault: true, + promptLevel: "medium", + }, + }), + }, + ui: { + select: async (title: string, options: string[]) => { + selectCalls.push({ title, options }); + return choices.length > 0 ? await choices.shift() : undefined; + }, + confirm: async () => true, + input: async () => undefined, + notify: (message: string, type?: string) => { + notifications.push({ message, type }); + }, + onTerminalInput: () => () => {}, + setStatus: () => {}, + setWorkingMessage: () => {}, + setWidget: () => {}, + }, + }; + + return { ctx, selectCalls, notifications }; +} + +function mergeEvent(command: string): Record { + return { + type: "tool_call", + toolName: "bash", + toolCallId: `tool-${command}`, + input: { command }, + }; +} + +test("safe-git merge: once approval re-asks a later command without auto-declining no-answer", async () => { + const { toolCall } = installGuardrails(); + const { ctx, selectCalls, notifications } = makeCtx([ALLOW_ONCE, undefined]); + + const approved = await toolCall(mergeEvent("git merge feature-a"), ctx); + assert.equal(approved, undefined); + + const unanswered = await toolCall(mergeEvent("git merge feature-b"), ctx); + assert.equal(selectCalls.length, 2, "a different once-approved merge should be asked again"); + assert.equal(unanswered?.block, true); + assert.equal(unanswered?.reason, "Git merge approval not answered; command paused"); + assert.doesNotMatch(unanswered?.reason ?? "", /declined by user/); + assert.ok(notifications.some((entry) => entry.message === "Git merge approved once")); + assert.ok(notifications.some((entry) => entry.message === "Git merge approval not answered; command paused")); +}); + +test("safe-git merge: duplicate in-flight asks share the same approval", async () => { + const { toolCall } = installGuardrails(); + let resolveChoice: (choice: string | undefined) => void = () => {}; + const choice = new Promise((resolve) => { + resolveChoice = resolve; + }); + const { ctx, selectCalls, notifications } = makeCtx([choice]); + + const first = toolCall(mergeEvent("git merge feature-race"), ctx); + const second = toolCall(mergeEvent("git merge feature-race"), ctx); + + assert.equal(selectCalls.length, 1, "concurrent duplicate gate should show only one prompt"); + resolveChoice(ALLOW_ONCE); + + const [firstResult, secondResult] = await Promise.all([first, second]); + assert.equal(firstResult, undefined); + assert.equal(secondResult, undefined); + assert.equal(selectCalls.length, 1); + assert.equal( + notifications.filter((entry) => entry.message === "Git merge approved once").length, + 1, + ); + assert.equal(notifications.some((entry) => entry.message.includes("declined by user")), false); +}); + +test("safe-git merge: quick duplicate after once approval uses cached approval", async () => { + const { toolCall } = installGuardrails(); + const { ctx, selectCalls, notifications } = makeCtx([ALLOW_ONCE]); + + const first = await toolCall(mergeEvent("git merge feature-cache"), ctx); + const second = await toolCall(mergeEvent("git merge feature-cache"), ctx); + + assert.equal(first, undefined); + assert.equal(second, undefined); + assert.equal(selectCalls.length, 1, "cached duplicate approval should not re-open the gate"); + assert.ok(notifications.some((entry) => entry.message === "Git merge approval reused for duplicate request")); +}); + +test("safe-git merge: explicit manual decline still blocks as declined by user", async () => { + const { toolCall } = installGuardrails(); + const { ctx, notifications } = makeCtx([DECLINE_ONCE]); + + const declined = await toolCall(mergeEvent("git merge feature-decline"), ctx); + + assert.equal(declined?.block, true); + assert.equal(declined?.reason, "Git merge declined by user"); + assert.ok(notifications.some((entry) => entry.message === "Git merge declined")); +});