Skip to content

Commit ff44039

Browse files
fix: ignore stale approval resolve errors (#98394)
* fix(ui): scope approval decision results Co-authored-by: haruaiclone-droid <[email protected]> * docs(changelog): credit approval race fix --------- Co-authored-by: Peter Steinberger <[email protected]> Co-authored-by: haruaiclone-droid <[email protected]>
1 parent 978b425 commit ff44039

4 files changed

Lines changed: 248 additions & 26 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ Docs: https://docs.openclaw.ai
1515

1616
### Fixes
1717

18+
- **Control UI approval prompts:** keep stale resolve failures and busy-state cleanup from leaking across newer approvals or Gateway reconnects. (#98394) Thanks @haruaiclone-droid.
1819
- **Agent empty replies:** surface a visible failure when a completed interactive turn has no deliverable reply, including queued follow-ups, while preserving explicit silence, pending continuations, and committed side effects. (#100456) Thanks @mushuiyu886.
1920
- **Child process output safety:** prevent stdout/stderr pipe failures from crashing agent exec sessions, local TUI shell commands, and bounded process execution. (#100407, #100406, #100410) Thanks @cxbAsDev.
2021
- **Background refresh isolation:** keep remote skill-bin refreshes running when one node fails, and contain periodic subagent-sweeper failures without hiding errors from direct callers. (#100393, #100390) Thanks @cxbAsDev.

ui/src/app/overlays.test.ts

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
// Control UI tests cover application-owned overlay races.
2+
import { describe, expect, it, vi } from "vitest";
3+
import type { GatewayBrowserClient, GatewayEventFrame } from "../api/gateway.ts";
4+
import type { ApplicationGateway, ApplicationGatewaySnapshot } from "./gateway.ts";
5+
import { createApplicationOverlays } from "./overlays.ts";
6+
7+
type RequestFn = (method: string, params?: unknown) => Promise<unknown>;
8+
9+
function deferred<T = unknown>() {
10+
let resolve!: (value: T | PromiseLike<T>) => void;
11+
let reject!: (reason?: unknown) => void;
12+
const promise = new Promise<T>((resolvePromise, rejectPromise) => {
13+
resolve = resolvePromise;
14+
reject = rejectPromise;
15+
});
16+
return { promise, reject, resolve };
17+
}
18+
19+
function approval(id: string, createdAtMs: number) {
20+
return {
21+
id,
22+
createdAtMs,
23+
expiresAtMs: Date.now() + 60_000,
24+
request: { command: `echo ${id}` },
25+
};
26+
}
27+
28+
function createGatewayHarness(initialClient: GatewayBrowserClient) {
29+
let snapshot: ApplicationGatewaySnapshot = {
30+
assistantAgentId: "main",
31+
client: initialClient,
32+
connected: true,
33+
hello: null,
34+
lastError: null,
35+
lastErrorCode: null,
36+
sessionKey: "main",
37+
};
38+
const snapshotListeners = new Set<(next: ApplicationGatewaySnapshot) => void>();
39+
const eventListeners = new Set<(event: GatewayEventFrame) => void>();
40+
const gateway = {
41+
get snapshot() {
42+
return snapshot;
43+
},
44+
connection: { gatewayUrl: "ws://gateway.test", password: "", token: "" },
45+
eventLog: [],
46+
connect() {},
47+
setSessionKey() {},
48+
start() {},
49+
stop() {},
50+
subscribe(listener: (next: ApplicationGatewaySnapshot) => void) {
51+
snapshotListeners.add(listener);
52+
return () => snapshotListeners.delete(listener);
53+
},
54+
subscribeEventLog() {
55+
return () => {};
56+
},
57+
subscribeEvents(listener: (event: GatewayEventFrame) => void) {
58+
eventListeners.add(listener);
59+
return () => eventListeners.delete(listener);
60+
},
61+
} satisfies ApplicationGateway;
62+
return {
63+
emitApproval(id: string, createdAtMs: number) {
64+
const event: GatewayEventFrame = {
65+
event: "exec.approval.requested",
66+
payload: approval(id, createdAtMs),
67+
type: "event",
68+
};
69+
for (const listener of eventListeners) {
70+
listener(event);
71+
}
72+
},
73+
gateway,
74+
update(next: Partial<ApplicationGatewaySnapshot>) {
75+
snapshot = { ...snapshot, ...next };
76+
for (const listener of snapshotListeners) {
77+
listener(snapshot);
78+
}
79+
},
80+
};
81+
}
82+
83+
function client(request: RequestFn): GatewayBrowserClient {
84+
return { request } as unknown as GatewayBrowserClient;
85+
}
86+
87+
describe("application approval overlays", () => {
88+
it("does not attach an older resolve failure to a newer approval", async () => {
89+
const resolveAttempt = deferred();
90+
const request = vi.fn<RequestFn>((method) =>
91+
method.endsWith(".list") ? Promise.resolve([]) : resolveAttempt.promise,
92+
);
93+
const harness = createGatewayHarness(client(request));
94+
const overlays = createApplicationOverlays(harness.gateway);
95+
96+
harness.emitApproval("approval-active", 1_000);
97+
const decision = overlays.decideApproval("allow-once");
98+
harness.emitApproval("approval-newer", 2_000);
99+
resolveAttempt.reject(new Error("gateway unavailable"));
100+
await decision;
101+
102+
expect(overlays.snapshot.approvalQueue.map((entry) => entry.id)).toEqual([
103+
"approval-newer",
104+
"approval-active",
105+
]);
106+
expect(overlays.snapshot.approvalError).toBeNull();
107+
expect(overlays.snapshot.approvalBusy).toBe(false);
108+
overlays.dispose();
109+
});
110+
111+
it("does not release a new client's busy state when an old resolve settles", async () => {
112+
const oldResolve = deferred();
113+
const oldRequest = vi.fn<RequestFn>((method) =>
114+
method.endsWith(".list") ? Promise.resolve([]) : oldResolve.promise,
115+
);
116+
const harness = createGatewayHarness(client(oldRequest));
117+
const overlays = createApplicationOverlays(harness.gateway);
118+
119+
harness.emitApproval("approval-old", 1_000);
120+
const oldDecision = overlays.decideApproval("allow-once");
121+
harness.update({ client: null, connected: false });
122+
123+
const newResolve = deferred();
124+
const newClient = client((method) =>
125+
method.endsWith(".list") ? Promise.resolve([]) : newResolve.promise,
126+
);
127+
harness.update({ client: newClient, connected: true });
128+
await Promise.resolve();
129+
harness.emitApproval("approval-new", 2_000);
130+
const newDecision = overlays.decideApproval("deny");
131+
expect(overlays.snapshot.approvalBusy).toBe(true);
132+
133+
oldResolve.reject(new Error("gateway client stopped"));
134+
await oldDecision;
135+
expect(overlays.snapshot.approvalBusy).toBe(true);
136+
expect(overlays.snapshot.approvalError).toBeNull();
137+
138+
newResolve.resolve({ ok: true });
139+
await newDecision;
140+
expect(overlays.snapshot.approvalBusy).toBe(false);
141+
expect(overlays.snapshot.approvalQueue).toEqual([]);
142+
overlays.dispose();
143+
});
144+
});

ui/src/app/overlays.ts

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,7 @@ export function createApplicationOverlays(gateway: ApplicationGateway): Applicat
216216
let updateVerificationGeneration = 0;
217217
let updateVerificationTimer: ReturnType<typeof globalThis.setTimeout> | null = null;
218218
let devicePairPendingCountGeneration = 0;
219+
let approvalDecision: { client: NonNullable<typeof activeClient>; id: string } | null = null;
219220
const devicePairSetupState: DevicePairSetupState & { pendingCount: number } = {
220221
client: gateway.snapshot.client,
221222
connected: gateway.snapshot.connected,
@@ -253,6 +254,12 @@ export function createApplicationOverlays(gateway: ApplicationGateway): Applicat
253254
};
254255
promptState.execApprovalExpired = publish;
255256

257+
const isCurrentClient = (client: NonNullable<typeof activeClient>) =>
258+
!disposed &&
259+
activeClient === client &&
260+
gateway.snapshot.client === client &&
261+
gateway.snapshot.connected;
262+
256263
const refreshDevicePairPendingCount = async () => {
257264
const client = gateway.snapshot.client;
258265
if (
@@ -285,12 +292,7 @@ export function createApplicationOverlays(gateway: ApplicationGateway): Applicat
285292

286293
const refreshApprovals = async (client: NonNullable<typeof activeClient>) => {
287294
const applied = await refreshPendingApprovalQueue(promptState, {
288-
isCurrentClient: (requestClient) =>
289-
!disposed &&
290-
requestClient === client &&
291-
activeClient === client &&
292-
gateway.snapshot.client === client &&
293-
gateway.snapshot.connected,
295+
isCurrentClient: (requestClient) => requestClient === client && isCurrentClient(client),
294296
});
295297
if (applied && !disposed) {
296298
publish();
@@ -411,6 +413,7 @@ export function createApplicationOverlays(gateway: ApplicationGateway): Applicat
411413
devicePairSetupState.client = next.client;
412414
devicePairSetupState.connected = next.connected;
413415
if (previousClient !== next.client || !next.connected) {
416+
approvalDecision = null;
414417
devicePairPendingCountGeneration += 1;
415418
closeDevicePairSetupState(devicePairSetupState);
416419
devicePairSetupState.pendingCount = 0;
@@ -571,47 +574,40 @@ export function createApplicationOverlays(gateway: ApplicationGateway): Applicat
571574
}
572575
promptState.execApprovalBusy = true;
573576
promptState.execApprovalError = null;
577+
const operation = { client, id: active.id };
578+
approvalDecision = operation;
574579
publish();
575580
try {
576581
const method =
577582
active.kind === "plugin" ? "plugin.approval.resolve" : "exec.approval.resolve";
578583
await client.request(method, { id: active.id, decision });
579-
if (
580-
disposed ||
581-
activeClient !== client ||
582-
gateway.snapshot.client !== client ||
583-
!gateway.snapshot.connected
584-
) {
584+
if (!isCurrentClient(client)) {
585585
return;
586586
}
587587
dismissExecApprovalPrompt(promptState, active.id);
588588
} catch (error) {
589589
if (isStaleApprovalResolutionError(error)) {
590-
if (
591-
disposed ||
592-
activeClient !== client ||
593-
gateway.snapshot.client !== client ||
594-
!gateway.snapshot.connected
595-
) {
590+
if (!isCurrentClient(client)) {
596591
return;
597592
}
598593
dismissExecApprovalPrompt(promptState, active.id);
599594
const currentClient = activeClient;
600-
if (
601-
currentClient &&
602-
gateway.snapshot.client === currentClient &&
603-
gateway.snapshot.connected
604-
) {
595+
if (currentClient && isCurrentClient(currentClient)) {
605596
await refreshApprovals(currentClient);
606597
}
607598
return;
608599
}
609-
if (promptState.execApprovalQueue.some((entry) => entry.id === active.id)) {
600+
if (isCurrentClient(client) && promptState.execApprovalQueue[0]?.id === active.id) {
610601
promptState.execApprovalError = `Approval failed: ${error instanceof Error ? error.message : String(error)}`;
611602
}
612603
} finally {
613-
promptState.execApprovalBusy = false;
614-
publish();
604+
// Reconnect can admit a new decision while this request is still settling.
605+
// Only the operation that owns the busy state may release it.
606+
if (approvalDecision === operation) {
607+
approvalDecision = null;
608+
promptState.execApprovalBusy = false;
609+
publish();
610+
}
615611
}
616612
},
617613
async openDevicePairSetup() {
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Control UI E2E tests cover approval queue behavior through the Gateway WebSocket.
2+
import { chromium, type Browser, type Page } from "playwright";
3+
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
4+
import {
5+
canRunPlaywrightChromium,
6+
installMockGateway,
7+
resolvePlaywrightChromiumExecutablePath,
8+
startControlUiE2eServer,
9+
type ControlUiE2eServer,
10+
} from "../test-helpers/control-ui-e2e.ts";
11+
12+
const chromiumExecutablePath = resolvePlaywrightChromiumExecutablePath(chromium.executablePath());
13+
const chromiumAvailable = canRunPlaywrightChromium(chromiumExecutablePath);
14+
const allowMissingChromium = process.env.OPENCLAW_UI_E2E_ALLOW_MISSING_CHROMIUM === "1";
15+
const describeControlUiE2e = chromiumAvailable || !allowMissingChromium ? describe : describe.skip;
16+
17+
let browser: Browser | undefined;
18+
let page: Page | undefined;
19+
let server: ControlUiE2eServer | undefined;
20+
21+
function approval(id: string, command: string, createdAtMs: number) {
22+
return {
23+
id,
24+
createdAtMs,
25+
expiresAtMs: Date.now() + 60_000,
26+
request: { command },
27+
};
28+
}
29+
30+
describeControlUiE2e("Control UI approval flow", () => {
31+
beforeAll(async () => {
32+
server = await startControlUiE2eServer();
33+
});
34+
35+
afterEach(async () => {
36+
await page
37+
?.context()
38+
.close()
39+
.catch(() => {});
40+
await browser?.close().catch(() => {});
41+
page = undefined;
42+
browser = undefined;
43+
});
44+
45+
afterAll(async () => {
46+
await server?.close();
47+
});
48+
49+
it("keeps an older resolve failure off the newly active approval", async () => {
50+
browser = await chromium.launch({ executablePath: chromiumExecutablePath });
51+
const context = await browser.newContext({ viewport: { height: 800, width: 1200 } });
52+
const currentPage = await context.newPage();
53+
page = currentPage;
54+
const gateway = await installMockGateway(currentPage);
55+
56+
await currentPage.goto(`${server?.baseUrl ?? ""}chat`);
57+
await gateway.waitForRequest("sessions.list");
58+
await gateway.deferNext("exec.approval.resolve");
59+
await gateway.emitGatewayEvent(
60+
"exec.approval.requested",
61+
approval("approval-active", "echo active", 1_000),
62+
);
63+
await currentPage.getByText("echo active", { exact: true }).waitFor();
64+
await currentPage.getByRole("button", { name: "Allow once" }).click();
65+
66+
await gateway.emitGatewayEvent(
67+
"exec.approval.requested",
68+
approval("approval-newer", "echo newer", 2_000),
69+
);
70+
await currentPage.getByText("echo newer", { exact: true }).waitFor();
71+
await gateway.rejectDeferred("exec.approval.resolve", {
72+
code: "UNAVAILABLE",
73+
message: "gateway unavailable",
74+
});
75+
76+
await expect.poll(() => currentPage.locator(".exec-approval-error").count()).toBe(0);
77+
await expect
78+
.poll(() => currentPage.getByRole("button", { name: "Deny" }).isEnabled())
79+
.toBe(true);
80+
});
81+
});

0 commit comments

Comments
 (0)