mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-27 04:47:03 -06:00
fix(approvals): bind decisions to lifecycle instances
This commit is contained in:
@@ -251,6 +251,7 @@ export const ApprovalChannelReviewerSchema = closedObject({
|
||||
|
||||
export const ApprovalResolveParamsSchema = closedObject({
|
||||
id: ApprovalRecordCommonFields.id,
|
||||
instanceId: Type.Optional(NonEmptyString),
|
||||
kind: ApprovalKindSchema,
|
||||
decision: ApprovalDecisionSchema,
|
||||
reviewer: Type.Optional(ApprovalChannelReviewerSchema),
|
||||
|
||||
@@ -313,6 +313,7 @@ export const ExecApprovalRequestParamsSchema = closedObject({
|
||||
/** Reviewer decision payload for one pending exec approval. */
|
||||
export const ExecApprovalResolveParamsSchema = closedObject({
|
||||
id: NonEmptyString,
|
||||
instanceId: Type.Optional(NonEmptyString),
|
||||
decision: NonEmptyString,
|
||||
reviewer: Type.Optional(ApprovalChannelReviewerSchema),
|
||||
});
|
||||
|
||||
@@ -56,6 +56,7 @@ export const PluginApprovalRequestParamsSchema = closedObject({
|
||||
/** Reviewer decision payload resolving one pending plugin approval request. */
|
||||
export const PluginApprovalResolveParamsSchema = closedObject({
|
||||
id: NonEmptyString,
|
||||
instanceId: Type.Optional(NonEmptyString),
|
||||
decision: NonEmptyString,
|
||||
reviewer: Type.Optional(ApprovalChannelReviewerSchema),
|
||||
});
|
||||
|
||||
@@ -82,6 +82,7 @@ type ExecApprovalResolutionSource = "operator" | "auto-review";
|
||||
|
||||
export type ExecApprovalRecord<TPayload = ExecApprovalRequestPayload> = {
|
||||
id: string;
|
||||
instanceId: string;
|
||||
request: TPayload;
|
||||
createdAtMs: number;
|
||||
expiresAtMs: number;
|
||||
@@ -267,6 +268,7 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
|
||||
const resolvedId = hasExplicitId ? id : randomUUID();
|
||||
const record: ExecApprovalRecord<TPayload> = {
|
||||
id: resolvedId,
|
||||
instanceId: randomUUID(),
|
||||
request,
|
||||
createdAtMs: now,
|
||||
expiresAtMs,
|
||||
|
||||
@@ -125,6 +125,7 @@ describe("sanitizeSystemRunParamsForForwarding", () => {
|
||||
const effectiveBindingArgv = bindingArgv ?? commandArgv ?? [command];
|
||||
return {
|
||||
id: "approval-1",
|
||||
instanceId: "approval-instance-1",
|
||||
request: {
|
||||
host: "node",
|
||||
nodeId: "node-1",
|
||||
|
||||
@@ -127,6 +127,7 @@ export function listVisiblePendingApprovalRequests<TPayload>(params: {
|
||||
}): Array<{
|
||||
approvalKind?: ChannelApprovalKind;
|
||||
id: string;
|
||||
instanceId: string;
|
||||
request: TPayload;
|
||||
createdAtMs: number;
|
||||
expiresAtMs: number;
|
||||
@@ -140,8 +141,8 @@ export function listVisiblePendingApprovalRequests<TPayload>(params: {
|
||||
...(params.cfg ? { cfg: params.cfg } : {}),
|
||||
}),
|
||||
)
|
||||
.map(({ id, request, createdAtMs, expiresAtMs }) => {
|
||||
const approval = { id, request, createdAtMs, expiresAtMs };
|
||||
.map(({ id, instanceId, request, createdAtMs, expiresAtMs }) => {
|
||||
const approval = { id, instanceId, request, createdAtMs, expiresAtMs };
|
||||
return params.approvalKind
|
||||
? Object.assign(approval, { approvalKind: params.approvalKind })
|
||||
: approval;
|
||||
|
||||
@@ -1286,6 +1286,34 @@ describe("handlePendingApprovalRequest", () => {
|
||||
expect(manager.getSnapshot(record.id)?.decision).toBeUndefined();
|
||||
});
|
||||
|
||||
it("rejects a decision bound to an older approval instance", async () => {
|
||||
const manager = new ExecApprovalManager();
|
||||
const record = manager.create({ command: "echo replacement" }, 60_000, "approval-reused");
|
||||
void manager.register(record, 60_000);
|
||||
const respond = vi.fn();
|
||||
|
||||
await handleApprovalResolve({
|
||||
approvalKind: "exec",
|
||||
manager,
|
||||
inputId: record.id,
|
||||
instanceId: "older-instance",
|
||||
decision: "allow-once",
|
||||
respond,
|
||||
context: {
|
||||
broadcast: vi.fn(),
|
||||
broadcastToConnIds: vi.fn(),
|
||||
} as unknown as GatewayRequestContext,
|
||||
client: null,
|
||||
});
|
||||
|
||||
expect(respond).toHaveBeenCalledWith(
|
||||
false,
|
||||
undefined,
|
||||
expect.objectContaining({ message: "unknown or expired approval id" }),
|
||||
);
|
||||
expect(manager.getSnapshot(record.id)?.decision).toBeUndefined();
|
||||
});
|
||||
|
||||
it("does not wait on decisions for approvals hidden from the caller", async () => {
|
||||
const manager = new ExecApprovalManager();
|
||||
const record = manager.create(
|
||||
|
||||
@@ -58,6 +58,7 @@ type RequestedApprovalEvent<
|
||||
> = {
|
||||
approvalKind?: TKind;
|
||||
id: string;
|
||||
instanceId?: string;
|
||||
request: TPayload;
|
||||
createdAtMs: number;
|
||||
expiresAtMs: number;
|
||||
@@ -75,6 +76,7 @@ type ApprovalRequestDeliveryRoute = "approval-client" | "forwarder" | "turn-sour
|
||||
|
||||
type ApprovalResolveParams = {
|
||||
id: string;
|
||||
instanceId?: string;
|
||||
decision: string;
|
||||
reviewer?: ApprovalChannelReviewer;
|
||||
};
|
||||
@@ -153,6 +155,7 @@ export function buildRequestedApprovalEvent<
|
||||
return {
|
||||
...(approvalKind ? { approvalKind } : {}),
|
||||
id: record.id,
|
||||
instanceId: record.instanceId,
|
||||
request: record.request,
|
||||
createdAtMs: record.createdAtMs,
|
||||
expiresAtMs: record.expiresAtMs,
|
||||
@@ -167,6 +170,7 @@ export function resolveApprovalDecisionParams<TParams extends ApprovalResolvePar
|
||||
respond: RespondFn;
|
||||
}): {
|
||||
inputId: string;
|
||||
instanceId?: string;
|
||||
decision: ExecApprovalDecision;
|
||||
reviewer?: ApprovalChannelReviewer;
|
||||
} | null {
|
||||
@@ -180,6 +184,7 @@ export function resolveApprovalDecisionParams<TParams extends ApprovalResolvePar
|
||||
}
|
||||
return {
|
||||
inputId: rawParams.id,
|
||||
...(rawParams.instanceId ? { instanceId: rawParams.instanceId } : {}),
|
||||
decision: rawParams.decision,
|
||||
...(rawParams.reviewer ? { reviewer: rawParams.reviewer } : {}),
|
||||
};
|
||||
@@ -484,6 +489,7 @@ export async function handleApprovalResolve<
|
||||
approvalKind: ChannelApprovalKind;
|
||||
manager: ExecApprovalManager<TPayload>;
|
||||
inputId: string;
|
||||
instanceId?: string;
|
||||
decision: ExecApprovalDecision;
|
||||
respond: RespondFn;
|
||||
context: GatewayRequestContext;
|
||||
@@ -560,6 +566,11 @@ export async function handleApprovalResolve<
|
||||
return;
|
||||
}
|
||||
|
||||
if (params.instanceId && resolved.snapshot.instanceId !== params.instanceId) {
|
||||
respondUnknownOrExpiredApproval(params.respond);
|
||||
return;
|
||||
}
|
||||
|
||||
const validationError = params.validateDecision?.(resolved.snapshot);
|
||||
if (validationError) {
|
||||
params.respond(
|
||||
|
||||
@@ -435,7 +435,7 @@ export function createApprovalHandlers(
|
||||
? params.execApprovalManager.getLiveSnapshot(record.id)
|
||||
: record.kind === "plugin"
|
||||
? params.pluginApprovalManager.getLiveSnapshot(record.id)
|
||||
: undefined;
|
||||
: params.systemAgentApprovalManager?.getLiveSnapshot(record.id);
|
||||
if (resolveParams?.reviewer && (!custody || !liveRecord || !custody.authorizes(liveRecord))) {
|
||||
respondApprovalNotFound(respond);
|
||||
return;
|
||||
@@ -454,6 +454,10 @@ export function createApprovalHandlers(
|
||||
respond(true, { applied: false, approval }, undefined);
|
||||
return;
|
||||
}
|
||||
if (resolveParams?.instanceId && liveRecord?.instanceId !== resolveParams.instanceId) {
|
||||
respondApprovalNotFound(respond);
|
||||
return;
|
||||
}
|
||||
const resolver = custody
|
||||
? ({ kind: "channel", id: custody.resolverId } as const)
|
||||
: resolveApprovalResolver(client);
|
||||
|
||||
@@ -457,7 +457,7 @@ export function createExecApprovalHandlers(
|
||||
if (!decisionPromise) {
|
||||
return;
|
||||
}
|
||||
const requestEvent: ExecApprovalRequest = buildRequestedApprovalEvent(record, "exec");
|
||||
const requestEvent = buildRequestedApprovalEvent(record, "exec");
|
||||
const forwardRequest = opts?.forwarder?.handleRequested.bind(opts.forwarder);
|
||||
const iosPushRequest = opts?.iosPushDelivery?.handleRequested?.bind(opts.iosPushDelivery);
|
||||
await handlePendingApprovalRequest({
|
||||
@@ -522,12 +522,13 @@ export function createExecApprovalHandlers(
|
||||
if (!resolveParams) {
|
||||
return;
|
||||
}
|
||||
const { inputId, decision, reviewer } = resolveParams;
|
||||
const { inputId, instanceId, decision, reviewer } = resolveParams;
|
||||
let autoReviewResolution = false;
|
||||
await handleApprovalResolve({
|
||||
approvalKind: "exec",
|
||||
manager,
|
||||
inputId,
|
||||
instanceId,
|
||||
decision,
|
||||
respond,
|
||||
context,
|
||||
|
||||
@@ -310,11 +310,12 @@ export function createPluginApprovalHandlers(
|
||||
if (!resolveParams) {
|
||||
return;
|
||||
}
|
||||
const { inputId, decision, reviewer } = resolveParams;
|
||||
const { inputId, instanceId, decision, reviewer } = resolveParams;
|
||||
await handleApprovalResolve({
|
||||
approvalKind: "plugin",
|
||||
manager,
|
||||
inputId,
|
||||
instanceId,
|
||||
decision,
|
||||
respond,
|
||||
context,
|
||||
|
||||
@@ -219,6 +219,7 @@ export type ExecApprovalRequest = {
|
||||
/** Descriptive wire metadata; readers derive it from the payload when absent. */
|
||||
approvalKind?: "exec";
|
||||
id: string;
|
||||
instanceId?: string;
|
||||
request: ExecApprovalRequestPayload;
|
||||
createdAtMs: number;
|
||||
expiresAtMs: number;
|
||||
|
||||
@@ -23,6 +23,8 @@ export type ExecApprovalDecision = "allow-once" | "allow-always" | "deny";
|
||||
|
||||
export type ExecApprovalRequest = {
|
||||
id: string;
|
||||
/** Gateway-owned lifecycle identity. Older event producers omit it. */
|
||||
instanceId?: string;
|
||||
kind: "exec" | "plugin" | "system-agent";
|
||||
request: ExecApprovalRequestPayload;
|
||||
pluginTitle?: string;
|
||||
@@ -124,6 +126,7 @@ function parseExecApprovalRequested(payload: unknown): ExecApprovalRequest | nul
|
||||
}
|
||||
return {
|
||||
id,
|
||||
instanceId: normalizeOptionalString(payload.instanceId) ?? undefined,
|
||||
kind: "exec",
|
||||
request: {
|
||||
command,
|
||||
@@ -192,6 +195,7 @@ function parsePluginApprovalRequested(payload: unknown): ExecApprovalRequest | n
|
||||
|
||||
return {
|
||||
id,
|
||||
instanceId: normalizeOptionalString(payload.instanceId) ?? undefined,
|
||||
kind: "plugin",
|
||||
request: {
|
||||
command: title,
|
||||
@@ -225,6 +229,7 @@ function parseSystemAgentApprovalRequested(payload: unknown): ExecApprovalReques
|
||||
}
|
||||
return {
|
||||
id,
|
||||
instanceId: normalizeOptionalString(payload.instanceId) ?? undefined,
|
||||
kind: "system-agent",
|
||||
request: {
|
||||
command,
|
||||
@@ -263,13 +268,18 @@ export async function resolveApprovalRequest(
|
||||
if (approval.kind === "system-agent") {
|
||||
await client.request("approval.resolve", {
|
||||
id: approval.id,
|
||||
...(approval.instanceId ? { instanceId: approval.instanceId } : {}),
|
||||
kind: "system-agent",
|
||||
decision,
|
||||
});
|
||||
return;
|
||||
}
|
||||
const method = approval.kind === "plugin" ? "plugin.approval.resolve" : "exec.approval.resolve";
|
||||
await client.request(method, { id: approval.id, decision });
|
||||
await client.request(method, {
|
||||
id: approval.id,
|
||||
...(approval.instanceId ? { instanceId: approval.instanceId } : {}),
|
||||
decision,
|
||||
});
|
||||
}
|
||||
|
||||
function pruneExecApprovalQueue(queue: ExecApprovalRequest[]): ExecApprovalRequest[] {
|
||||
|
||||
@@ -22,9 +22,10 @@ export function deferred<T = unknown>() {
|
||||
return { promise, reject, resolve };
|
||||
}
|
||||
|
||||
export function approval(id: string, createdAtMs: number) {
|
||||
export function approval(id: string, createdAtMs: number, instanceId = `${id}:${createdAtMs}`) {
|
||||
return {
|
||||
id,
|
||||
instanceId,
|
||||
createdAtMs,
|
||||
expiresAtMs: Date.now() + 60_000,
|
||||
request: { command: `echo ${id}` },
|
||||
@@ -79,10 +80,10 @@ export function createGatewayHarness(
|
||||
listener(frame);
|
||||
}
|
||||
},
|
||||
emitApproval(id: string, createdAtMs: number) {
|
||||
emitApproval(id: string, createdAtMs: number, instanceId?: string) {
|
||||
const event: GatewayEventFrame = {
|
||||
event: "exec.approval.requested",
|
||||
payload: approval(id, createdAtMs),
|
||||
payload: approval(id, createdAtMs, instanceId),
|
||||
type: "event",
|
||||
};
|
||||
for (const listener of eventListeners) {
|
||||
@@ -104,6 +105,7 @@ export function createGatewayHarness(
|
||||
event: "openclaw.approval.requested",
|
||||
payload: {
|
||||
id,
|
||||
instanceId: `${id}:${createdAtMs}`,
|
||||
createdAtMs,
|
||||
expiresAtMs: Date.now() + 60_000,
|
||||
request: {
|
||||
|
||||
@@ -8,7 +8,7 @@ import {
|
||||
} from "./overlays-access.test-support.ts";
|
||||
import { createApplicationOverlays } from "./overlays.ts";
|
||||
|
||||
function approvalRace(id: string, replacementCreatedAtMs: number) {
|
||||
function approvalRace(id: string, replacementInstanceId: string) {
|
||||
const resolveAttempt = deferred();
|
||||
const request = vi.fn<RequestFn>((method) =>
|
||||
method.endsWith(".list") ? Promise.resolve([]) : resolveAttempt.promise,
|
||||
@@ -18,7 +18,7 @@ function approvalRace(id: string, replacementCreatedAtMs: number) {
|
||||
harness.emitApproval(id, 1_000);
|
||||
const original = overlays.snapshot.approvalQueue[0];
|
||||
const decision = overlays.decideApproval("allow-once");
|
||||
harness.emitApproval(id, replacementCreatedAtMs);
|
||||
harness.emitApproval(id, 1_000, replacementInstanceId);
|
||||
return { decision, original, overlays, request, resolveAttempt };
|
||||
}
|
||||
|
||||
@@ -27,7 +27,7 @@ describe("application approval replacement races", () => {
|
||||
it(`keeps a refreshed approval owned by its pending decision after ${outcome}`, async () => {
|
||||
const { decision, original, overlays, resolveAttempt } = approvalRace(
|
||||
"approval-refreshed",
|
||||
1_000,
|
||||
"approval-refreshed:1000",
|
||||
);
|
||||
|
||||
expect(overlays.snapshot.approvalQueue[0]).not.toBe(original);
|
||||
@@ -69,7 +69,7 @@ describe("application approval replacement races", () => {
|
||||
async ({ error, stale }) => {
|
||||
const { decision, overlays, request, resolveAttempt } = approvalRace(
|
||||
"approval-reused",
|
||||
2_000,
|
||||
"replacement-instance",
|
||||
);
|
||||
const listRequestCount = request.mock.calls.filter(([method]) =>
|
||||
method.endsWith(".list"),
|
||||
@@ -83,7 +83,7 @@ describe("application approval replacement races", () => {
|
||||
await decision;
|
||||
|
||||
expect(overlays.snapshot.approvalQueue).toEqual([
|
||||
expect.objectContaining({ createdAtMs: 2_000, id: "approval-reused" }),
|
||||
expect.objectContaining({ instanceId: "replacement-instance", id: "approval-reused" }),
|
||||
]);
|
||||
expect(overlays.snapshot.approvalErrors).toEqual(new Map());
|
||||
if (stale) {
|
||||
|
||||
@@ -616,14 +616,16 @@ export function createApplicationOverlays(
|
||||
operation.grantGeneration === approvalGrantGeneration &&
|
||||
readGatewayOperatorAccess(gateway.snapshot).canGrantApprovals &&
|
||||
isCurrentClient(operation.client);
|
||||
// A refresh can reconstruct the same approval, while a reused id
|
||||
// with a different creation time denotes a new protected request.
|
||||
// The Gateway owns this lifecycle token. Approval ids are reusable, so
|
||||
// an older decision must never settle or annotate a replacement record.
|
||||
const isCurrentApproval = () =>
|
||||
promptState.execApprovalQueue.some(
|
||||
(entry) =>
|
||||
entry.id === active.id &&
|
||||
entry.kind === active.kind &&
|
||||
entry.createdAtMs === active.createdAtMs,
|
||||
(active.instanceId
|
||||
? entry.instanceId === active.instanceId
|
||||
: !entry.instanceId && entry.createdAtMs === active.createdAtMs),
|
||||
);
|
||||
publish();
|
||||
try {
|
||||
|
||||
@@ -17,9 +17,16 @@ const activeSessionKey = "agent:main:main";
|
||||
const captureUiProof = process.env.OPENCLAW_CAPTURE_UI_PROOF === "1";
|
||||
const proofDir = path.join(process.cwd(), ".artifacts", "control-ui-e2e", "approval-flow");
|
||||
|
||||
function approval(id: string, command: string, createdAtMs: number, sessionKey = activeSessionKey) {
|
||||
function approval(
|
||||
id: string,
|
||||
command: string,
|
||||
createdAtMs: number,
|
||||
sessionKey = activeSessionKey,
|
||||
instanceId = `${id}:${createdAtMs}`,
|
||||
) {
|
||||
return {
|
||||
id,
|
||||
instanceId,
|
||||
createdAtMs,
|
||||
expiresAtMs: Date.now() + 60_000,
|
||||
request: { command, agentId: "main", sessionKey },
|
||||
@@ -120,7 +127,13 @@ suite.define(() => {
|
||||
.toBe(0);
|
||||
await gateway.emitGatewayEvent(
|
||||
"exec.approval.requested",
|
||||
approval("approval-reused", "echo replacement approval", 2_000),
|
||||
approval(
|
||||
"approval-reused",
|
||||
"echo replacement approval",
|
||||
1_000,
|
||||
activeSessionKey,
|
||||
"replacement-instance",
|
||||
),
|
||||
);
|
||||
const replacement = currentPage.getByText("echo replacement approval", { exact: true });
|
||||
await replacement.waitFor();
|
||||
|
||||
@@ -242,6 +242,7 @@ describe("AppSidebar session attention", () => {
|
||||
it("shows approval attention ahead of a run error", async () => {
|
||||
const approval = {
|
||||
id: "approval-1",
|
||||
instanceId: "approval-instance-1",
|
||||
kind: "exec",
|
||||
request: { command: "git status", sessionKey },
|
||||
createdAtMs: Date.now(),
|
||||
@@ -266,6 +267,7 @@ describe("AppSidebar session attention", () => {
|
||||
const mainKey = "agent:main:main";
|
||||
const approval = {
|
||||
id: "approval-main",
|
||||
instanceId: "approval-main-instance",
|
||||
kind: "exec",
|
||||
request: { command: "git status", sessionKey: mainKey },
|
||||
createdAtMs: Date.now(),
|
||||
@@ -387,6 +389,7 @@ describe("AppSidebar session attention", () => {
|
||||
]);
|
||||
const approval = {
|
||||
id: "approval-child",
|
||||
instanceId: "approval-child-instance",
|
||||
kind: "exec",
|
||||
request: { command: "git status", sessionKey: childKey },
|
||||
createdAtMs: Date.now(),
|
||||
|
||||
Reference in New Issue
Block a user