Skip to content

Commit f021633

Browse files
openperfshakkernerd
openperf
authored and
shakkernerd
committed
fix(agents): drop stale exec approval followups after session rebind
Exec approval followups were dispatched by sessionKey only. When /new or /reset rotates the sessionId under that key while an approval is pending, the resolved followup landed in the new session, surfacing stale approval output (or 'Exec denied' / continuation text) in a fresh conversation. Capture the session UUID active when the approval is requested and drop the followup once the key has been rebound to a different sessionId: - agent-run followups: carry the expected id on the agent request and drop it at the gateway as an early preflight, before the handler touches the rebound session (session-store write, chat/agent run + active-run registration, dedupe, accepted ack) — not just before model dispatch. Covers elevated and non-elevated. - denied / direct fallback followups: resolve the key's current sessionId from the session store and drop before the channel send. Fixes #59349.
1 parent 67dc805 commit f021633

15 files changed

Lines changed: 408 additions & 1 deletion

‎apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,7 @@ public struct AgentParams: Codable, Sendable {
765765
public let bootstrapcontextrunkind: AnyCodable?
766766
public let acpturnsource: String?
767767
public let internalruntimehandoffid: String?
768+
public let execapprovalfollowupexpectedsessionid: String?
768769
public let internalevents: [[String: AnyCodable]]?
769770
public let inputprovenance: [String: AnyCodable]?
770771
public let suppresspromptpersistence: Bool?
@@ -806,6 +807,7 @@ public struct AgentParams: Codable, Sendable {
806807
bootstrapcontextrunkind: AnyCodable?,
807808
acpturnsource: String?,
808809
internalruntimehandoffid: String?,
810+
execapprovalfollowupexpectedsessionid: String?,
809811
internalevents: [[String: AnyCodable]]?,
810812
inputprovenance: [String: AnyCodable]?,
811813
suppresspromptpersistence: Bool?,
@@ -846,6 +848,7 @@ public struct AgentParams: Codable, Sendable {
846848
self.bootstrapcontextrunkind = bootstrapcontextrunkind
847849
self.acpturnsource = acpturnsource
848850
self.internalruntimehandoffid = internalruntimehandoffid
851+
self.execapprovalfollowupexpectedsessionid = execapprovalfollowupexpectedsessionid
849852
self.internalevents = internalevents
850853
self.inputprovenance = inputprovenance
851854
self.suppresspromptpersistence = suppresspromptpersistence
@@ -888,6 +891,7 @@ public struct AgentParams: Codable, Sendable {
888891
case bootstrapcontextrunkind = "bootstrapContextRunKind"
889892
case acpturnsource = "acpTurnSource"
890893
case internalruntimehandoffid = "internalRuntimeHandoffId"
894+
case execapprovalfollowupexpectedsessionid = "execApprovalFollowupExpectedSessionId"
891895
case internalevents = "internalEvents"
892896
case inputprovenance = "inputProvenance"
893897
case suppresspromptpersistence = "suppressPromptPersistence"

‎packages/gateway-protocol/src/schema/agent.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ export const AgentParamsSchema = Type.Object(
219219
),
220220
acpTurnSource: Type.Optional(Type.Literal("manual_spawn")),
221221
internalRuntimeHandoffId: Type.Optional(NonEmptyString),
222+
execApprovalFollowupExpectedSessionId: Type.Optional(NonEmptyString),
222223
internalEvents: Type.Optional(Type.Array(AgentInternalEventSchema)),
223224
inputProvenance: Type.Optional(InputProvenanceSchema),
224225
suppressPromptPersistence: Type.Optional(Type.Boolean()),

‎src/agents/agent-tools.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -824,6 +824,8 @@ export function createOpenClawCodingTools(options?: {
824824
allowBackground,
825825
scopeKey,
826826
sessionKey: options?.sessionKey,
827+
sessionId: options?.sessionId,
828+
sessionStore: options?.config?.session?.store,
827829
mainKey: options?.config?.session?.mainKey,
828830
sessionScope: options?.config?.session?.scope,
829831
eventRouting: resolveEventSessionRoutingPolicy({

‎src/agents/bash-tools.exec-approval-followup-state.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,21 @@ export function consumeExecApprovalFollowupRuntimeHandoff(params: {
169169
return cloneExecApprovalFollowupRuntimeHandoff(entry);
170170
}
171171

172+
/**
173+
* A persisted exec-approval followup is stale when the session key it targeted
174+
* has since been rebound to a different session id (via `/new` or `/reset`).
175+
* Delivering it would leak the old approval result into the new session, so the
176+
* gateway drops the followup instead of resuming the rebound session.
177+
*/
178+
export function isExecApprovalFollowupSessionRebound(params: {
179+
expectedSessionId?: string;
180+
resolvedSessionId?: string;
181+
}): boolean {
182+
const expected = normalizeOptionalString(params.expectedSessionId);
183+
const resolved = normalizeOptionalString(params.resolvedSessionId);
184+
return Boolean(expected && resolved && expected !== resolved);
185+
}
186+
172187
/** Clear exec approval follow-up handoffs between tests. */
173188
export function resetExecApprovalFollowupRuntimeHandoffsForTests(): void {
174189
execApprovalFollowupRuntimeHandoffs.clear();

‎src/agents/bash-tools.exec-approval-followup.test.ts‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,52 @@ vi.mock("../infra/outbound/message.js", () => ({
1313
sendMessage: vi.fn(async () => ({ ok: true })),
1414
}));
1515

16+
import fs from "node:fs";
17+
import os from "node:os";
18+
import path from "node:path";
19+
import {
20+
closeSqliteSessionStoreDatabase,
21+
replaceSqliteSessionStore,
22+
} from "../config/sessions/store-sqlite.js";
23+
import { clearSessionStoreCacheForTest } from "../config/sessions/store.js";
24+
import type { SessionEntry } from "../config/sessions/types.js";
1625
import { sendMessage } from "../infra/outbound/message.js";
1726
import {
1827
buildExecApprovalFollowupPrompt,
1928
sendExecApprovalFollowup,
2029
} from "./bash-tools.exec-approval-followup.js";
2130
import { callGatewayTool } from "./tools/gateway.js";
2231

32+
const tempStoreDirs: string[] = [];
33+
const tempStorePaths: string[] = [];
34+
35+
// Seed the same SQLite-backed session store path the runtime reads; mocking this
36+
// boundary would hide stale-session regressions in shared workers.
37+
function writeTempSessionStore(entries: Record<string, { sessionId: string }>): string {
38+
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "exec-approval-followup-store-"));
39+
tempStoreDirs.push(dir);
40+
const storePath = path.join(dir, "sessions.json");
41+
tempStorePaths.push(storePath);
42+
replaceSqliteSessionStore(storePath, entries as Record<string, SessionEntry>);
43+
clearSessionStoreCacheForTest();
44+
return storePath;
45+
}
46+
2347
afterEach(() => {
2448
vi.resetAllMocks();
49+
clearSessionStoreCacheForTest();
50+
while (tempStorePaths.length > 0) {
51+
const storePath = tempStorePaths.pop();
52+
if (storePath) {
53+
closeSqliteSessionStoreDatabase(storePath);
54+
}
55+
}
56+
while (tempStoreDirs.length > 0) {
57+
const dir = tempStoreDirs.pop();
58+
if (dir) {
59+
fs.rmSync(dir, { recursive: true, force: true });
60+
}
61+
}
2562
});
2663

2764
function requireRecord(value: unknown, label: string): Record<string, unknown> {
@@ -118,6 +155,93 @@ describe("exec approval followup", () => {
118155
expect(sendMessage).not.toHaveBeenCalled();
119156
});
120157

158+
it("forwards the approval-time session id so the gateway can drop stale followups", async () => {
159+
await sendExecApprovalFollowup({
160+
approvalId: "req-pin-59349",
161+
sessionKey: "agent:main:main",
162+
expectedSessionId: "session-original",
163+
resultText: "Exec completed: echo ok",
164+
});
165+
166+
expectGatewayAgentFollowup({
167+
sessionKey: "agent:main:main",
168+
execApprovalFollowupExpectedSessionId: "session-original",
169+
});
170+
});
171+
172+
it("omits the expected session id when none was captured", async () => {
173+
await sendExecApprovalFollowup({
174+
approvalId: "req-no-pin",
175+
sessionKey: "agent:main:main",
176+
resultText: "Exec completed: echo ok",
177+
});
178+
179+
const params = expectGatewayAgentFollowup({ sessionKey: "agent:main:main" });
180+
expect(params).not.toHaveProperty("execApprovalFollowupExpectedSessionId");
181+
});
182+
183+
it("drops a denied direct followup when the session key was rebound by /new or /reset", async () => {
184+
const sessionStore = writeTempSessionStore({
185+
"agent:main:main": { sessionId: "session-after-reset" },
186+
});
187+
188+
const result = await sendExecApprovalFollowup({
189+
approvalId: "req-denied-rebound",
190+
sessionKey: "agent:main:main",
191+
expectedSessionId: "session-original",
192+
sessionStore,
193+
direct: true,
194+
turnSourceChannel: "telegram",
195+
turnSourceTo: "-100123",
196+
resultText: "Exec denied (gateway id=req-denied-rebound, user-denied): uname -a",
197+
});
198+
199+
expect(result).toBe(false);
200+
expect(sendMessage).not.toHaveBeenCalled();
201+
expect(callGatewayTool).not.toHaveBeenCalled();
202+
});
203+
204+
it("delivers a denied direct followup when the key still resolves to the approval-time session", async () => {
205+
const sessionStore = writeTempSessionStore({
206+
"agent:main:main": { sessionId: "session-original" },
207+
});
208+
209+
await sendExecApprovalFollowup({
210+
approvalId: "req-denied-same",
211+
sessionKey: "agent:main:main",
212+
expectedSessionId: "session-original",
213+
sessionStore,
214+
direct: true,
215+
turnSourceChannel: "telegram",
216+
turnSourceTo: "-100123",
217+
resultText: "Exec denied (gateway id=req-denied-same, user-denied): uname -a",
218+
});
219+
220+
expect(sendMessage).toHaveBeenCalled();
221+
expect(callGatewayTool).not.toHaveBeenCalled();
222+
});
223+
224+
it("drops a non-denied direct fallback when the session key was rebound", async () => {
225+
const sessionStore = writeTempSessionStore({
226+
"agent:main:main": { sessionId: "session-after-reset" },
227+
});
228+
229+
const result = await sendExecApprovalFollowup({
230+
approvalId: "req-finished-rebound",
231+
sessionKey: "agent:main:main",
232+
expectedSessionId: "session-original",
233+
sessionStore,
234+
direct: true,
235+
turnSourceChannel: "telegram",
236+
turnSourceTo: "-100123",
237+
resultText: "Exec finished (gateway id=req-finished-rebound, code 0)\nok",
238+
});
239+
240+
expect(result).toBe(false);
241+
expect(sendMessage).not.toHaveBeenCalled();
242+
expect(callGatewayTool).not.toHaveBeenCalled();
243+
});
244+
121245
it("routes denied followups through the originating main session", async () => {
122246
await sendExecApprovalFollowup({
123247
approvalId: "req-denied-main",

‎src/agents/bash-tools.exec-approval-followup.ts‎

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,21 @@ import {
77
normalizeLowercaseStringOrEmpty,
88
normalizeOptionalString,
99
} from "@openclaw/normalization-core/string-coerce";
10+
import { resolveStorePath } from "../config/sessions/paths.js";
11+
import { loadSessionStore } from "../config/sessions/store-load.js";
1012
import {
1113
resolveExternalBestEffortDeliveryTarget,
1214
type ExternalBestEffortDeliveryTarget,
1315
} from "../infra/outbound/best-effort-delivery.js";
1416
import { sendMessage } from "../infra/outbound/message.js";
17+
import { createSubsystemLogger } from "../logging/subsystem.js";
18+
import { resolveAgentIdFromSessionKey } from "../routing/session-key.js";
1519
import { isCronSessionKey, isSubagentSessionKey } from "../sessions/session-key-utils.js";
1620
import { isGatewayMessageChannel, normalizeMessageChannel } from "../utils/message-channel.js";
17-
import { buildExecApprovalFollowupIdempotencyKey } from "./bash-tools.exec-approval-followup-state.js";
21+
import {
22+
buildExecApprovalFollowupIdempotencyKey,
23+
isExecApprovalFollowupSessionRebound,
24+
} from "./bash-tools.exec-approval-followup-state.js";
1825
import { sanitizeUserFacingText } from "./embedded-agent-helpers/sanitize-user-facing-text.js";
1926
import {
2027
formatExecDeniedUserMessage,
@@ -23,9 +30,17 @@ import {
2330
} from "./exec-approval-result.js";
2431
import { callGatewayTool } from "./tools/gateway.js";
2532

33+
const log = createSubsystemLogger("agents/exec-approval-followup");
34+
2635
type ExecApprovalFollowupParams = {
2736
approvalId: string;
2837
sessionKey?: string;
38+
/** Session UUID active when the approval was requested. Carried to the gateway
39+
* so a followup whose session key was rebound by /new or /reset is dropped. */
40+
expectedSessionId?: string;
41+
/** `session.store` template, used by the direct/denied path to resolve the
42+
* key's current sessionId and drop a rebound followup before sending. */
43+
sessionStore?: string;
2944
turnSourceChannel?: string;
3045
turnSourceTo?: string;
3146
turnSourceAccountId?: string;
@@ -91,6 +106,41 @@ function shouldSuppressExecDeniedFollowup(sessionKey: string | undefined): boole
91106
return isSubagentSessionKey(sessionKey) || isCronSessionKey(sessionKey);
92107
}
93108

109+
/**
110+
* Direct/denied followups bypass the gateway agent dispatch, so the gateway
111+
* rebind guard never sees them. Resolve the session key's current sessionId
112+
* from the on-disk store and report whether it was rebound away from the
113+
* approval-time session by `/new` or `/reset` (#59349). Failure to resolve is
114+
* treated as "not rebound" so a real result is never suppressed by accident.
115+
*/
116+
function isExecApprovalFollowupDirectDeliveryStale(params: {
117+
sessionKey: string | undefined;
118+
expectedSessionId: string | undefined;
119+
sessionStore: string | undefined;
120+
}): boolean {
121+
const sessionKey = normalizeOptionalString(params.sessionKey);
122+
const expectedSessionId = normalizeOptionalString(params.expectedSessionId);
123+
if (!sessionKey || !expectedSessionId) {
124+
return false;
125+
}
126+
try {
127+
const storePath = resolveStorePath(normalizeOptionalString(params.sessionStore), {
128+
agentId: resolveAgentIdFromSessionKey(sessionKey),
129+
});
130+
const resolvedSessionId = normalizeOptionalString(
131+
loadSessionStore(storePath)?.[sessionKey]?.sessionId,
132+
);
133+
return isExecApprovalFollowupSessionRebound({ expectedSessionId, resolvedSessionId });
134+
} catch (err) {
135+
// Fail open: if the session store can't be resolved we deliver rather than
136+
// risk dropping a real followup, but log it so this rare path is observable.
137+
log.debug(
138+
`exec approval followup session-rebind check skipped for ${sessionKey}; delivering: ${formatUnknownError(err)}`,
139+
);
140+
return false;
141+
}
142+
}
143+
94144
function formatDirectExecApprovalFollowupText(
95145
resultText: string,
96146
opts: { allowDenied?: boolean } = {},
@@ -199,6 +249,7 @@ function canDirectSendDeniedFollowup(sessionError: unknown): boolean {
199249
function buildAgentFollowupArgs(params: {
200250
approvalId: string;
201251
sessionKey: string;
252+
expectedSessionId?: string;
202253
resultText: string;
203254
deliveryTarget: ExternalBestEffortDeliveryTarget;
204255
sessionOnlyOriginChannel?: string;
@@ -240,6 +291,9 @@ function buildAgentFollowupArgs(params: {
240291
buildExecApprovalFollowupIdempotencyKey({
241292
approvalId: params.approvalId,
242293
}),
294+
...(params.expectedSessionId
295+
? { execApprovalFollowupExpectedSessionId: params.expectedSessionId }
296+
: {}),
243297
...(params.internalRuntimeHandoffId
244298
? { internalRuntimeHandoffId: params.internalRuntimeHandoffId }
245299
: {}),
@@ -310,6 +364,7 @@ export async function sendExecApprovalFollowup(
310364
const agentArgs = buildAgentFollowupArgs({
311365
approvalId: params.approvalId,
312366
sessionKey,
367+
expectedSessionId: params.expectedSessionId,
313368
resultText,
314369
deliveryTarget,
315370
sessionOnlyOriginChannel,
@@ -341,6 +396,18 @@ export async function sendExecApprovalFollowup(
341396
}
342397

343398
if (isDenied) {
399+
if (
400+
isExecApprovalFollowupDirectDeliveryStale({
401+
sessionKey,
402+
expectedSessionId: params.expectedSessionId,
403+
sessionStore: params.sessionStore,
404+
})
405+
) {
406+
log.info(
407+
`Dropping stale denied exec approval followup ${params.approvalId}: session ${sessionKey ?? ""} was rebound before the approval resolved`,
408+
);
409+
return false;
410+
}
344411
if (
345412
await sendDirectFollowupFallback({
346413
approvalId: params.approvalId,
@@ -358,6 +425,19 @@ export async function sendExecApprovalFollowup(
358425
return false;
359426
}
360427

428+
if (
429+
isExecApprovalFollowupDirectDeliveryStale({
430+
sessionKey,
431+
expectedSessionId: params.expectedSessionId,
432+
sessionStore: params.sessionStore,
433+
})
434+
) {
435+
log.info(
436+
`Dropping stale exec approval followup ${params.approvalId} direct fallback: session ${sessionKey ?? ""} was rebound before the approval resolved`,
437+
);
438+
return false;
439+
}
440+
361441
if (
362442
await sendDirectFollowupFallback({
363443
approvalId: params.approvalId,

‎src/agents/bash-tools.exec-host-gateway.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,10 @@ export type ProcessGatewayAllowlistParams = {
8282
trigger?: string;
8383
agentId?: string;
8484
sessionKey?: string;
85+
/** Session UUID active when the approval was requested; pins the followup. */
86+
sessionId?: string;
87+
/** Session-store template, so the direct/denied followup can detect a rebind. */
88+
sessionStore?: string;
8589
bashElevated?: ExecElevatedDefaults;
8690
turnSourceChannel?: string;
8791
turnSourceTo?: string;
@@ -702,6 +706,8 @@ export async function processGatewayAllowlist(
702706
const followupTarget = buildExecApprovalFollowupTarget({
703707
approvalId,
704708
sessionKey: params.notifySessionKey ?? params.sessionKey,
709+
expectedSessionId: params.sessionId,
710+
sessionStore: params.sessionStore,
705711
bashElevated: params.bashElevated,
706712
turnSourceChannel: params.turnSourceChannel,
707713
turnSourceTo: params.turnSourceTo,

‎src/agents/bash-tools.exec-host-node.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,8 @@ export async function executeNodeHostCommand(
306306
const followupTarget = execHostShared.buildExecApprovalFollowupTarget({
307307
approvalId,
308308
sessionKey: params.notifySessionKey ?? params.sessionKey,
309+
expectedSessionId: params.sessionId,
310+
sessionStore: params.sessionStore,
309311
bashElevated: params.bashElevated,
310312
turnSourceChannel: params.turnSourceChannel,
311313
turnSourceTo: params.turnSourceTo,

0 commit comments

Comments
 (0)