fix(git-merge): no auto-flip from approved to declined; cached approval is sticky
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 <noreply@anthropic.com> Co-Authored-By: OpenAI Codex <noreply@openai.com>
This commit is contained in:
Mikael Hugo
parent
d38e5ea092
commit
5d3c204006
2 changed files with 238 additions and 30 deletions
|
|
@ -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<string, Promise<GitCommandDecision>>;
|
||||
recentOnceApprovals: Map<string, number>;
|
||||
}
|
||||
|
||||
const SAFE_GIT_DEFAULTS: Required<SafeGitConfig> = {
|
||||
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<string>,
|
||||
sessionBlockedActions: Set<string>,
|
||||
gateState: SafeGitGateState,
|
||||
): Promise<GitCommandDecision> {
|
||||
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<string>,
|
||||
sessionBlockedActions: Set<string>,
|
||||
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<string>();
|
||||
const sessionBlockedActions = new Set<string>();
|
||||
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);
|
||||
}
|
||||
|
|
|
|||
136
src/resources/extensions/sf/tests/safe-git-merge-gate.test.ts
Normal file
136
src/resources/extensions/sf/tests/safe-git-merge-gate.test.ts
Normal file
|
|
@ -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<string, unknown>, ctx: Record<string, unknown>) => Promise<ToolCallResult>;
|
||||
type Choice = string | undefined | Promise<string | undefined>;
|
||||
|
||||
const ALLOW_ONCE = "✅ Allow this command once";
|
||||
const DECLINE_ONCE = "⏭️ Decline this time (ask again later)";
|
||||
|
||||
function installGuardrails(): { toolCall: Handler } {
|
||||
const handlers = new Map<string, Handler[]>();
|
||||
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<string, unknown> {
|
||||
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<string | undefined>((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"));
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue