mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 11:10:45 +00:00
fix(gateway): make repeated approval resolves idempotent
This commit is contained in:
@@ -33,6 +33,7 @@ export type ExecApprovalRecord<TPayload = ExecApprovalRequestPayload> = {
|
||||
requestedByClientId?: string | null;
|
||||
resolvedAtMs?: number;
|
||||
decision?: ExecApprovalDecision;
|
||||
consumedDecision?: ExecApprovalDecision;
|
||||
resolvedBy?: string | null;
|
||||
};
|
||||
|
||||
@@ -181,6 +182,7 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
|
||||
}
|
||||
// One-time approvals must be consumed atomically so the same runId
|
||||
// cannot be replayed during the resolved-entry grace window.
|
||||
record.consumedDecision = record.decision;
|
||||
record.decision = undefined;
|
||||
return true;
|
||||
}
|
||||
@@ -194,7 +196,10 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
|
||||
return entry?.promise ?? null;
|
||||
}
|
||||
|
||||
lookupPendingId(input: string): ExecApprovalIdLookupResult {
|
||||
lookupApprovalId(
|
||||
input: string,
|
||||
opts: { includeResolved?: boolean } = {},
|
||||
): ExecApprovalIdLookupResult {
|
||||
const normalized = input.trim();
|
||||
if (!normalized) {
|
||||
return { kind: "none" };
|
||||
@@ -202,7 +207,7 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
|
||||
|
||||
const exact = this.pending.get(normalized);
|
||||
if (exact) {
|
||||
return exact.record.resolvedAtMs === undefined
|
||||
return opts.includeResolved || exact.record.resolvedAtMs === undefined
|
||||
? { kind: "exact", id: normalized }
|
||||
: { kind: "none" };
|
||||
}
|
||||
@@ -210,7 +215,7 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
|
||||
const lowerPrefix = normalizeLowercaseStringOrEmpty(normalized);
|
||||
const matches: string[] = [];
|
||||
for (const [id, entry] of this.pending.entries()) {
|
||||
if (entry.record.resolvedAtMs !== undefined) {
|
||||
if (!opts.includeResolved && entry.record.resolvedAtMs !== undefined) {
|
||||
continue;
|
||||
}
|
||||
if (normalizeLowercaseStringOrEmpty(id).startsWith(lowerPrefix)) {
|
||||
@@ -226,4 +231,8 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
|
||||
}
|
||||
return { kind: "none" };
|
||||
}
|
||||
|
||||
lookupPendingId(input: string): ExecApprovalIdLookupResult {
|
||||
return this.lookupApprovalId(input);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -11,8 +11,19 @@ import type { GatewayClient, GatewayRequestContext, RespondFn } from "./types.js
|
||||
|
||||
export const APPROVAL_NOT_FOUND_DETAILS = {
|
||||
reason: ErrorCodes.APPROVAL_NOT_FOUND,
|
||||
remediation: "Re-request the action; pending approvals are cleared after expiry or restart.",
|
||||
} as const;
|
||||
|
||||
const APPROVAL_ALREADY_RESOLVED_DETAILS = {
|
||||
reason: "APPROVAL_ALREADY_RESOLVED",
|
||||
} as const;
|
||||
|
||||
function resolveRecordedApprovalDecision<TPayload>(
|
||||
record: ExecApprovalRecord<TPayload>,
|
||||
): ExecApprovalDecision | undefined {
|
||||
return record.decision ?? record.consumedDecision;
|
||||
}
|
||||
|
||||
type PendingApprovalLookupError =
|
||||
| "missing"
|
||||
| {
|
||||
@@ -97,6 +108,37 @@ export function resolvePendingApprovalRecord<TPayload>(params: {
|
||||
return { ok: true, approvalId: resolvedId.id, snapshot };
|
||||
}
|
||||
|
||||
function resolveResolvedApprovalRecord<TPayload>(params: {
|
||||
manager: ExecApprovalManager<TPayload>;
|
||||
inputId: string;
|
||||
exposeAmbiguousPrefixError?: boolean;
|
||||
}):
|
||||
| {
|
||||
ok: true;
|
||||
approvalId: string;
|
||||
snapshot: ExecApprovalRecord<TPayload>;
|
||||
}
|
||||
| {
|
||||
ok: false;
|
||||
response: PendingApprovalLookupError;
|
||||
} {
|
||||
const resolvedId = params.manager.lookupApprovalId(params.inputId, { includeResolved: true });
|
||||
if (resolvedId.kind !== "exact" && resolvedId.kind !== "prefix") {
|
||||
return {
|
||||
ok: false,
|
||||
response: resolvePendingApprovalLookupError({
|
||||
resolvedId,
|
||||
exposeAmbiguousPrefixError: params.exposeAmbiguousPrefixError,
|
||||
}),
|
||||
};
|
||||
}
|
||||
const snapshot = params.manager.getSnapshot(resolvedId.id);
|
||||
if (!snapshot || snapshot.resolvedAtMs === undefined) {
|
||||
return { ok: false, response: "missing" };
|
||||
}
|
||||
return { ok: true, approvalId: resolvedId.id, snapshot };
|
||||
}
|
||||
|
||||
export function respondPendingApprovalLookupError(params: {
|
||||
respond: RespondFn;
|
||||
response: PendingApprovalLookupError;
|
||||
@@ -259,6 +301,25 @@ export async function handleApprovalResolve<TPayload, TResolvedEvent extends obj
|
||||
exposeAmbiguousPrefixError: params.exposeAmbiguousPrefixError,
|
||||
});
|
||||
if (!resolved.ok) {
|
||||
const resolvedRepeat = resolveResolvedApprovalRecord({
|
||||
manager: params.manager,
|
||||
inputId: params.inputId,
|
||||
exposeAmbiguousPrefixError: params.exposeAmbiguousPrefixError,
|
||||
});
|
||||
if (resolvedRepeat.ok) {
|
||||
if (resolveRecordedApprovalDecision(resolvedRepeat.snapshot) === params.decision) {
|
||||
params.respond(true, { ok: true }, undefined);
|
||||
return;
|
||||
}
|
||||
params.respond(
|
||||
false,
|
||||
undefined,
|
||||
errorShape(ErrorCodes.INVALID_REQUEST, "approval already resolved", {
|
||||
details: APPROVAL_ALREADY_RESOLVED_DETAILS,
|
||||
}),
|
||||
);
|
||||
return;
|
||||
}
|
||||
respondPendingApprovalLookupError({ respond: params.respond, response: resolved.response });
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -752,7 +752,7 @@ describe("exec approval handlers", () => {
|
||||
},
|
||||
hasExecApprovalClients: () => true,
|
||||
};
|
||||
return { handlers, broadcasts, respond, context };
|
||||
return { manager, handlers, broadcasts, respond, context };
|
||||
}
|
||||
|
||||
function createForwardingExecApprovalFixture(opts?: {
|
||||
@@ -1011,6 +1011,62 @@ describe("exec approval handlers", () => {
|
||||
expect(broadcasts.some((entry) => entry.event === "exec.approval.resolved")).toBe(true);
|
||||
});
|
||||
|
||||
it("treats duplicate same-decision exec resolves as idempotent during grace", async () => {
|
||||
const { manager, handlers, broadcasts, respond, context } = createExecApprovalFixture();
|
||||
|
||||
const requestPromise = requestExecApproval({
|
||||
handlers,
|
||||
respond,
|
||||
context,
|
||||
params: { id: "approval-repeat-1", twoPhase: true },
|
||||
});
|
||||
|
||||
const firstResolveRespond = vi.fn();
|
||||
await resolveExecApproval({
|
||||
handlers,
|
||||
id: "approval-repeat-1",
|
||||
respond: firstResolveRespond,
|
||||
context,
|
||||
});
|
||||
await requestPromise;
|
||||
expect(manager.consumeAllowOnce("approval-repeat-1")).toBe(true);
|
||||
|
||||
const resolvedBroadcastCount = broadcasts.filter(
|
||||
(entry) => entry.event === "exec.approval.resolved",
|
||||
).length;
|
||||
|
||||
const repeatResolveRespond = vi.fn();
|
||||
await resolveExecApproval({
|
||||
handlers,
|
||||
id: "approval-repeat-1",
|
||||
respond: repeatResolveRespond,
|
||||
context,
|
||||
});
|
||||
|
||||
const conflictingResolveRespond = vi.fn();
|
||||
await resolveExecApproval({
|
||||
handlers,
|
||||
id: "approval-repeat-1",
|
||||
decision: "deny",
|
||||
respond: conflictingResolveRespond,
|
||||
context,
|
||||
});
|
||||
|
||||
expect(firstResolveRespond).toHaveBeenCalledWith(true, { ok: true }, undefined);
|
||||
expect(repeatResolveRespond).toHaveBeenCalledWith(true, { ok: true }, undefined);
|
||||
expect(broadcasts.filter((entry) => entry.event === "exec.approval.resolved")).toHaveLength(
|
||||
resolvedBroadcastCount,
|
||||
);
|
||||
expect(conflictingResolveRespond).toHaveBeenCalledWith(
|
||||
false,
|
||||
undefined,
|
||||
expect.objectContaining({
|
||||
message: "approval already resolved",
|
||||
details: expect.objectContaining({ reason: "APPROVAL_ALREADY_RESOLVED" }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects allow-always when the request ask mode is always", async () => {
|
||||
const { handlers, broadcasts, respond, context } = createExecApprovalFixture();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user