mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-04 00:53:39 +00:00
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.
This commit is contained in:
@@ -765,6 +765,7 @@ public struct AgentParams: Codable, Sendable {
|
||||
public let bootstrapcontextrunkind: AnyCodable?
|
||||
public let acpturnsource: String?
|
||||
public let internalruntimehandoffid: String?
|
||||
public let execapprovalfollowupexpectedsessionid: String?
|
||||
public let internalevents: [[String: AnyCodable]]?
|
||||
public let inputprovenance: [String: AnyCodable]?
|
||||
public let suppresspromptpersistence: Bool?
|
||||
@@ -806,6 +807,7 @@ public struct AgentParams: Codable, Sendable {
|
||||
bootstrapcontextrunkind: AnyCodable?,
|
||||
acpturnsource: String?,
|
||||
internalruntimehandoffid: String?,
|
||||
execapprovalfollowupexpectedsessionid: String?,
|
||||
internalevents: [[String: AnyCodable]]?,
|
||||
inputprovenance: [String: AnyCodable]?,
|
||||
suppresspromptpersistence: Bool?,
|
||||
@@ -846,6 +848,7 @@ public struct AgentParams: Codable, Sendable {
|
||||
self.bootstrapcontextrunkind = bootstrapcontextrunkind
|
||||
self.acpturnsource = acpturnsource
|
||||
self.internalruntimehandoffid = internalruntimehandoffid
|
||||
self.execapprovalfollowupexpectedsessionid = execapprovalfollowupexpectedsessionid
|
||||
self.internalevents = internalevents
|
||||
self.inputprovenance = inputprovenance
|
||||
self.suppresspromptpersistence = suppresspromptpersistence
|
||||
@@ -888,6 +891,7 @@ public struct AgentParams: Codable, Sendable {
|
||||
case bootstrapcontextrunkind = "bootstrapContextRunKind"
|
||||
case acpturnsource = "acpTurnSource"
|
||||
case internalruntimehandoffid = "internalRuntimeHandoffId"
|
||||
case execapprovalfollowupexpectedsessionid = "execApprovalFollowupExpectedSessionId"
|
||||
case internalevents = "internalEvents"
|
||||
case inputprovenance = "inputProvenance"
|
||||
case suppresspromptpersistence = "suppressPromptPersistence"
|
||||
|
||||
@@ -219,6 +219,7 @@ export const AgentParamsSchema = Type.Object(
|
||||
),
|
||||
acpTurnSource: Type.Optional(Type.Literal("manual_spawn")),
|
||||
internalRuntimeHandoffId: Type.Optional(NonEmptyString),
|
||||
execApprovalFollowupExpectedSessionId: Type.Optional(NonEmptyString),
|
||||
internalEvents: Type.Optional(Type.Array(AgentInternalEventSchema)),
|
||||
inputProvenance: Type.Optional(InputProvenanceSchema),
|
||||
suppressPromptPersistence: Type.Optional(Type.Boolean()),
|
||||
|
||||
@@ -824,6 +824,8 @@ export function createOpenClawCodingTools(options?: {
|
||||
allowBackground,
|
||||
scopeKey,
|
||||
sessionKey: options?.sessionKey,
|
||||
sessionId: options?.sessionId,
|
||||
sessionStore: options?.config?.session?.store,
|
||||
mainKey: options?.config?.session?.mainKey,
|
||||
sessionScope: options?.config?.session?.scope,
|
||||
eventRouting: resolveEventSessionRoutingPolicy({
|
||||
|
||||
@@ -169,6 +169,21 @@ export function consumeExecApprovalFollowupRuntimeHandoff(params: {
|
||||
return cloneExecApprovalFollowupRuntimeHandoff(entry);
|
||||
}
|
||||
|
||||
/**
|
||||
* A persisted exec-approval followup is stale when the session key it targeted
|
||||
* has since been rebound to a different session id (via `/new` or `/reset`).
|
||||
* Delivering it would leak the old approval result into the new session, so the
|
||||
* gateway drops the followup instead of resuming the rebound session.
|
||||
*/
|
||||
export function isExecApprovalFollowupSessionRebound(params: {
|
||||
expectedSessionId?: string;
|
||||
resolvedSessionId?: string;
|
||||
}): boolean {
|
||||
const expected = normalizeOptionalString(params.expectedSessionId);
|
||||
const resolved = normalizeOptionalString(params.resolvedSessionId);
|
||||
return Boolean(expected && resolved && expected !== resolved);
|
||||
}
|
||||
|
||||
/** Clear exec approval follow-up handoffs between tests. */
|
||||
export function resetExecApprovalFollowupRuntimeHandoffsForTests(): void {
|
||||
execApprovalFollowupRuntimeHandoffs.clear();
|
||||
|
||||
@@ -13,6 +13,15 @@ vi.mock("../infra/outbound/message.js", () => ({
|
||||
sendMessage: vi.fn(async () => ({ ok: true })),
|
||||
}));
|
||||
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import {
|
||||
closeSqliteSessionStoreDatabase,
|
||||
replaceSqliteSessionStore,
|
||||
} from "../config/sessions/store-sqlite.js";
|
||||
import { clearSessionStoreCacheForTest } from "../config/sessions/store.js";
|
||||
import type { SessionEntry } from "../config/sessions/types.js";
|
||||
import { sendMessage } from "../infra/outbound/message.js";
|
||||
import {
|
||||
buildExecApprovalFollowupPrompt,
|
||||
@@ -20,8 +29,36 @@ import {
|
||||
} from "./bash-tools.exec-approval-followup.js";
|
||||
import { callGatewayTool } from "./tools/gateway.js";
|
||||
|
||||
const tempStoreDirs: string[] = [];
|
||||
const tempStorePaths: string[] = [];
|
||||
|
||||
// Seed the same SQLite-backed session store path the runtime reads; mocking this
|
||||
// boundary would hide stale-session regressions in shared workers.
|
||||
function writeTempSessionStore(entries: Record<string, { sessionId: string }>): string {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "exec-approval-followup-store-"));
|
||||
tempStoreDirs.push(dir);
|
||||
const storePath = path.join(dir, "sessions.json");
|
||||
tempStorePaths.push(storePath);
|
||||
replaceSqliteSessionStore(storePath, entries as Record<string, SessionEntry>);
|
||||
clearSessionStoreCacheForTest();
|
||||
return storePath;
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
vi.resetAllMocks();
|
||||
clearSessionStoreCacheForTest();
|
||||
while (tempStorePaths.length > 0) {
|
||||
const storePath = tempStorePaths.pop();
|
||||
if (storePath) {
|
||||
closeSqliteSessionStoreDatabase(storePath);
|
||||
}
|
||||
}
|
||||
while (tempStoreDirs.length > 0) {
|
||||
const dir = tempStoreDirs.pop();
|
||||
if (dir) {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
function requireRecord(value: unknown, label: string): Record<string, unknown> {
|
||||
@@ -118,6 +155,93 @@ describe("exec approval followup", () => {
|
||||
expect(sendMessage).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("forwards the approval-time session id so the gateway can drop stale followups", async () => {
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-pin-59349",
|
||||
sessionKey: "agent:main:main",
|
||||
expectedSessionId: "session-original",
|
||||
resultText: "Exec completed: echo ok",
|
||||
});
|
||||
|
||||
expectGatewayAgentFollowup({
|
||||
sessionKey: "agent:main:main",
|
||||
execApprovalFollowupExpectedSessionId: "session-original",
|
||||
});
|
||||
});
|
||||
|
||||
it("omits the expected session id when none was captured", async () => {
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-no-pin",
|
||||
sessionKey: "agent:main:main",
|
||||
resultText: "Exec completed: echo ok",
|
||||
});
|
||||
|
||||
const params = expectGatewayAgentFollowup({ sessionKey: "agent:main:main" });
|
||||
expect(params).not.toHaveProperty("execApprovalFollowupExpectedSessionId");
|
||||
});
|
||||
|
||||
it("drops a denied direct followup when the session key was rebound by /new or /reset", async () => {
|
||||
const sessionStore = writeTempSessionStore({
|
||||
"agent:main:main": { sessionId: "session-after-reset" },
|
||||
});
|
||||
|
||||
const result = await sendExecApprovalFollowup({
|
||||
approvalId: "req-denied-rebound",
|
||||
sessionKey: "agent:main:main",
|
||||
expectedSessionId: "session-original",
|
||||
sessionStore,
|
||||
direct: true,
|
||||
turnSourceChannel: "telegram",
|
||||
turnSourceTo: "-100123",
|
||||
resultText: "Exec denied (gateway id=req-denied-rebound, user-denied): uname -a",
|
||||
});
|
||||
|
||||
expect(result).toBe(false);
|
||||
expect(sendMessage).not.toHaveBeenCalled();
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("delivers a denied direct followup when the key still resolves to the approval-time session", async () => {
|
||||
const sessionStore = writeTempSessionStore({
|
||||
"agent:main:main": { sessionId: "session-original" },
|
||||
});
|
||||
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-denied-same",
|
||||
sessionKey: "agent:main:main",
|
||||
expectedSessionId: "session-original",
|
||||
sessionStore,
|
||||
direct: true,
|
||||
turnSourceChannel: "telegram",
|
||||
turnSourceTo: "-100123",
|
||||
resultText: "Exec denied (gateway id=req-denied-same, user-denied): uname -a",
|
||||
});
|
||||
|
||||
expect(sendMessage).toHaveBeenCalled();
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("drops a non-denied direct fallback when the session key was rebound", async () => {
|
||||
const sessionStore = writeTempSessionStore({
|
||||
"agent:main:main": { sessionId: "session-after-reset" },
|
||||
});
|
||||
|
||||
const result = await sendExecApprovalFollowup({
|
||||
approvalId: "req-finished-rebound",
|
||||
sessionKey: "agent:main:main",
|
||||
expectedSessionId: "session-original",
|
||||
sessionStore,
|
||||
direct: true,
|
||||
turnSourceChannel: "telegram",
|
||||
turnSourceTo: "-100123",
|
||||
resultText: "Exec finished (gateway id=req-finished-rebound, code 0)\nok",
|
||||
});
|
||||
|
||||
expect(result).toBe(false);
|
||||
expect(sendMessage).not.toHaveBeenCalled();
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("routes denied followups through the originating main session", async () => {
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-denied-main",
|
||||
|
||||
@@ -7,14 +7,21 @@ import {
|
||||
normalizeLowercaseStringOrEmpty,
|
||||
normalizeOptionalString,
|
||||
} from "@openclaw/normalization-core/string-coerce";
|
||||
import { resolveStorePath } from "../config/sessions/paths.js";
|
||||
import { loadSessionStore } from "../config/sessions/store-load.js";
|
||||
import {
|
||||
resolveExternalBestEffortDeliveryTarget,
|
||||
type ExternalBestEffortDeliveryTarget,
|
||||
} from "../infra/outbound/best-effort-delivery.js";
|
||||
import { sendMessage } from "../infra/outbound/message.js";
|
||||
import { createSubsystemLogger } from "../logging/subsystem.js";
|
||||
import { resolveAgentIdFromSessionKey } from "../routing/session-key.js";
|
||||
import { isCronSessionKey, isSubagentSessionKey } from "../sessions/session-key-utils.js";
|
||||
import { isGatewayMessageChannel, normalizeMessageChannel } from "../utils/message-channel.js";
|
||||
import { buildExecApprovalFollowupIdempotencyKey } from "./bash-tools.exec-approval-followup-state.js";
|
||||
import {
|
||||
buildExecApprovalFollowupIdempotencyKey,
|
||||
isExecApprovalFollowupSessionRebound,
|
||||
} from "./bash-tools.exec-approval-followup-state.js";
|
||||
import { sanitizeUserFacingText } from "./embedded-agent-helpers/sanitize-user-facing-text.js";
|
||||
import {
|
||||
formatExecDeniedUserMessage,
|
||||
@@ -23,9 +30,17 @@ import {
|
||||
} from "./exec-approval-result.js";
|
||||
import { callGatewayTool } from "./tools/gateway.js";
|
||||
|
||||
const log = createSubsystemLogger("agents/exec-approval-followup");
|
||||
|
||||
type ExecApprovalFollowupParams = {
|
||||
approvalId: string;
|
||||
sessionKey?: string;
|
||||
/** Session UUID active when the approval was requested. Carried to the gateway
|
||||
* so a followup whose session key was rebound by /new or /reset is dropped. */
|
||||
expectedSessionId?: string;
|
||||
/** `session.store` template, used by the direct/denied path to resolve the
|
||||
* key's current sessionId and drop a rebound followup before sending. */
|
||||
sessionStore?: string;
|
||||
turnSourceChannel?: string;
|
||||
turnSourceTo?: string;
|
||||
turnSourceAccountId?: string;
|
||||
@@ -91,6 +106,41 @@ function shouldSuppressExecDeniedFollowup(sessionKey: string | undefined): boole
|
||||
return isSubagentSessionKey(sessionKey) || isCronSessionKey(sessionKey);
|
||||
}
|
||||
|
||||
/**
|
||||
* Direct/denied followups bypass the gateway agent dispatch, so the gateway
|
||||
* rebind guard never sees them. Resolve the session key's current sessionId
|
||||
* from the on-disk store and report whether it was rebound away from the
|
||||
* approval-time session by `/new` or `/reset` (#59349). Failure to resolve is
|
||||
* treated as "not rebound" so a real result is never suppressed by accident.
|
||||
*/
|
||||
function isExecApprovalFollowupDirectDeliveryStale(params: {
|
||||
sessionKey: string | undefined;
|
||||
expectedSessionId: string | undefined;
|
||||
sessionStore: string | undefined;
|
||||
}): boolean {
|
||||
const sessionKey = normalizeOptionalString(params.sessionKey);
|
||||
const expectedSessionId = normalizeOptionalString(params.expectedSessionId);
|
||||
if (!sessionKey || !expectedSessionId) {
|
||||
return false;
|
||||
}
|
||||
try {
|
||||
const storePath = resolveStorePath(normalizeOptionalString(params.sessionStore), {
|
||||
agentId: resolveAgentIdFromSessionKey(sessionKey),
|
||||
});
|
||||
const resolvedSessionId = normalizeOptionalString(
|
||||
loadSessionStore(storePath)?.[sessionKey]?.sessionId,
|
||||
);
|
||||
return isExecApprovalFollowupSessionRebound({ expectedSessionId, resolvedSessionId });
|
||||
} catch (err) {
|
||||
// Fail open: if the session store can't be resolved we deliver rather than
|
||||
// risk dropping a real followup, but log it so this rare path is observable.
|
||||
log.debug(
|
||||
`exec approval followup session-rebind check skipped for ${sessionKey}; delivering: ${formatUnknownError(err)}`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
function formatDirectExecApprovalFollowupText(
|
||||
resultText: string,
|
||||
opts: { allowDenied?: boolean } = {},
|
||||
@@ -199,6 +249,7 @@ function canDirectSendDeniedFollowup(sessionError: unknown): boolean {
|
||||
function buildAgentFollowupArgs(params: {
|
||||
approvalId: string;
|
||||
sessionKey: string;
|
||||
expectedSessionId?: string;
|
||||
resultText: string;
|
||||
deliveryTarget: ExternalBestEffortDeliveryTarget;
|
||||
sessionOnlyOriginChannel?: string;
|
||||
@@ -240,6 +291,9 @@ function buildAgentFollowupArgs(params: {
|
||||
buildExecApprovalFollowupIdempotencyKey({
|
||||
approvalId: params.approvalId,
|
||||
}),
|
||||
...(params.expectedSessionId
|
||||
? { execApprovalFollowupExpectedSessionId: params.expectedSessionId }
|
||||
: {}),
|
||||
...(params.internalRuntimeHandoffId
|
||||
? { internalRuntimeHandoffId: params.internalRuntimeHandoffId }
|
||||
: {}),
|
||||
@@ -310,6 +364,7 @@ export async function sendExecApprovalFollowup(
|
||||
const agentArgs = buildAgentFollowupArgs({
|
||||
approvalId: params.approvalId,
|
||||
sessionKey,
|
||||
expectedSessionId: params.expectedSessionId,
|
||||
resultText,
|
||||
deliveryTarget,
|
||||
sessionOnlyOriginChannel,
|
||||
@@ -341,6 +396,18 @@ export async function sendExecApprovalFollowup(
|
||||
}
|
||||
|
||||
if (isDenied) {
|
||||
if (
|
||||
isExecApprovalFollowupDirectDeliveryStale({
|
||||
sessionKey,
|
||||
expectedSessionId: params.expectedSessionId,
|
||||
sessionStore: params.sessionStore,
|
||||
})
|
||||
) {
|
||||
log.info(
|
||||
`Dropping stale denied exec approval followup ${params.approvalId}: session ${sessionKey ?? ""} was rebound before the approval resolved`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
if (
|
||||
await sendDirectFollowupFallback({
|
||||
approvalId: params.approvalId,
|
||||
@@ -358,6 +425,19 @@ export async function sendExecApprovalFollowup(
|
||||
return false;
|
||||
}
|
||||
|
||||
if (
|
||||
isExecApprovalFollowupDirectDeliveryStale({
|
||||
sessionKey,
|
||||
expectedSessionId: params.expectedSessionId,
|
||||
sessionStore: params.sessionStore,
|
||||
})
|
||||
) {
|
||||
log.info(
|
||||
`Dropping stale exec approval followup ${params.approvalId} direct fallback: session ${sessionKey ?? ""} was rebound before the approval resolved`,
|
||||
);
|
||||
return false;
|
||||
}
|
||||
|
||||
if (
|
||||
await sendDirectFollowupFallback({
|
||||
approvalId: params.approvalId,
|
||||
|
||||
@@ -82,6 +82,10 @@ export type ProcessGatewayAllowlistParams = {
|
||||
trigger?: string;
|
||||
agentId?: string;
|
||||
sessionKey?: string;
|
||||
/** Session UUID active when the approval was requested; pins the followup. */
|
||||
sessionId?: string;
|
||||
/** Session-store template, so the direct/denied followup can detect a rebind. */
|
||||
sessionStore?: string;
|
||||
bashElevated?: ExecElevatedDefaults;
|
||||
turnSourceChannel?: string;
|
||||
turnSourceTo?: string;
|
||||
@@ -702,6 +706,8 @@ export async function processGatewayAllowlist(
|
||||
const followupTarget = buildExecApprovalFollowupTarget({
|
||||
approvalId,
|
||||
sessionKey: params.notifySessionKey ?? params.sessionKey,
|
||||
expectedSessionId: params.sessionId,
|
||||
sessionStore: params.sessionStore,
|
||||
bashElevated: params.bashElevated,
|
||||
turnSourceChannel: params.turnSourceChannel,
|
||||
turnSourceTo: params.turnSourceTo,
|
||||
|
||||
@@ -306,6 +306,8 @@ export async function executeNodeHostCommand(
|
||||
const followupTarget = execHostShared.buildExecApprovalFollowupTarget({
|
||||
approvalId,
|
||||
sessionKey: params.notifySessionKey ?? params.sessionKey,
|
||||
expectedSessionId: params.sessionId,
|
||||
sessionStore: params.sessionStore,
|
||||
bashElevated: params.bashElevated,
|
||||
turnSourceChannel: params.turnSourceChannel,
|
||||
turnSourceTo: params.turnSourceTo,
|
||||
|
||||
@@ -16,6 +16,10 @@ export type ExecuteNodeHostCommandParams = {
|
||||
requestedNode?: string;
|
||||
boundNode?: string;
|
||||
sessionKey?: string;
|
||||
/** Session UUID active when the approval was requested; pins the followup. */
|
||||
sessionId?: string;
|
||||
/** Session-store template, so the direct/denied followup can detect a rebind. */
|
||||
sessionStore?: string;
|
||||
bashElevated?: ExecElevatedDefaults;
|
||||
turnSourceChannel?: string;
|
||||
turnSourceTo?: string;
|
||||
|
||||
@@ -7,6 +7,7 @@ import { MAX_DATE_TIMESTAMP_MS } from "@openclaw/normalization-core/number-coerc
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
consumeExecApprovalFollowupRuntimeHandoff,
|
||||
isExecApprovalFollowupSessionRebound,
|
||||
registerExecApprovalFollowupRuntimeHandoff,
|
||||
resetExecApprovalFollowupRuntimeHandoffsForTests,
|
||||
} from "./bash-tools.exec-approval-followup-state.js";
|
||||
@@ -133,6 +134,7 @@ describe("sendExecApprovalFollowupResult", () => {
|
||||
internalRuntimeHandoffId?: string;
|
||||
idempotencyKey?: string;
|
||||
execApprovalFollowupToken?: string;
|
||||
expectedSessionId?: string;
|
||||
bashElevated?: unknown;
|
||||
}
|
||||
| undefined {
|
||||
@@ -141,6 +143,7 @@ describe("sendExecApprovalFollowupResult", () => {
|
||||
internalRuntimeHandoffId?: string;
|
||||
idempotencyKey?: string;
|
||||
execApprovalFollowupToken?: string;
|
||||
expectedSessionId?: string;
|
||||
bashElevated?: unknown;
|
||||
}
|
||||
| undefined;
|
||||
@@ -304,6 +307,53 @@ describe("sendExecApprovalFollowupResult", () => {
|
||||
expect(call).not.toHaveProperty("idempotencyKey");
|
||||
expect(call).not.toHaveProperty("bashElevated");
|
||||
});
|
||||
|
||||
it("forwards the approval-time session id to the followup dispatch (non-elevated)", async () => {
|
||||
sendExecApprovalFollowup.mockResolvedValue(true);
|
||||
|
||||
await sendExecApprovalFollowupResult(
|
||||
{
|
||||
approvalId: "approval-session-pin-59349",
|
||||
sessionKey: "agent:main:telegram:direct:123",
|
||||
expectedSessionId: "session-original",
|
||||
turnSourceChannel: "telegram",
|
||||
},
|
||||
"Exec finished",
|
||||
{ sendExecApprovalFollowup, logWarn },
|
||||
);
|
||||
|
||||
expect(firstExecApprovalFollowupCall()?.expectedSessionId).toBe("session-original");
|
||||
});
|
||||
});
|
||||
|
||||
describe("isExecApprovalFollowupSessionRebound", () => {
|
||||
it("flags a rebound session when the resolved id differs from the approval-time id", () => {
|
||||
expect(
|
||||
isExecApprovalFollowupSessionRebound({
|
||||
expectedSessionId: "session-original",
|
||||
resolvedSessionId: "session-after-reset",
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("keeps the followup when the session id is unchanged", () => {
|
||||
expect(
|
||||
isExecApprovalFollowupSessionRebound({
|
||||
expectedSessionId: "session-original",
|
||||
resolvedSessionId: "session-original",
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("does not drop when either session id is missing", () => {
|
||||
expect(isExecApprovalFollowupSessionRebound({ resolvedSessionId: "session-after-reset" })).toBe(
|
||||
false,
|
||||
);
|
||||
expect(isExecApprovalFollowupSessionRebound({ expectedSessionId: "session-original" })).toBe(
|
||||
false,
|
||||
);
|
||||
expect(isExecApprovalFollowupSessionRebound({})).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("resolveExecHostApprovalContext", () => {
|
||||
|
||||
@@ -101,6 +101,12 @@ export type RegisteredExecApprovalRequestContext = {
|
||||
export type ExecApprovalFollowupTarget = {
|
||||
approvalId: string;
|
||||
sessionKey?: string;
|
||||
/** Session UUID active when the approval was requested. Lets the followup be
|
||||
* dropped if `/new` or `/reset` rebinds the session key to a new session. */
|
||||
expectedSessionId?: string;
|
||||
/** Session-store template, so the direct/denied path can resolve the key's
|
||||
* current sessionId and drop a rebound followup before sending. */
|
||||
sessionStore?: string;
|
||||
turnSourceChannel?: string;
|
||||
turnSourceTo?: string;
|
||||
turnSourceAccountId?: string;
|
||||
@@ -352,6 +358,8 @@ export function buildExecApprovalFollowupTarget(
|
||||
return {
|
||||
approvalId: params.approvalId,
|
||||
sessionKey: params.sessionKey,
|
||||
expectedSessionId: params.expectedSessionId,
|
||||
sessionStore: params.sessionStore,
|
||||
turnSourceChannel: params.turnSourceChannel,
|
||||
turnSourceTo: params.turnSourceTo,
|
||||
turnSourceAccountId: params.turnSourceAccountId,
|
||||
@@ -457,6 +465,8 @@ export async function sendExecApprovalFollowupResult(
|
||||
await send({
|
||||
approvalId: target.approvalId,
|
||||
sessionKey: target.sessionKey,
|
||||
expectedSessionId: target.expectedSessionId,
|
||||
sessionStore: target.sessionStore,
|
||||
turnSourceChannel: target.turnSourceChannel,
|
||||
turnSourceTo: target.turnSourceTo,
|
||||
turnSourceAccountId: target.turnSourceAccountId,
|
||||
|
||||
@@ -50,6 +50,14 @@ export type ExecToolDefaults = {
|
||||
allowBackground?: boolean;
|
||||
scopeKey?: string;
|
||||
sessionKey?: string;
|
||||
/** Ephemeral session UUID active when this exec tool was built. Regenerated
|
||||
* on `/new` and `/reset`, so it pins exec-approval followups to the original
|
||||
* session instance and lets stale followups drop after a session rebind. */
|
||||
sessionId?: string;
|
||||
/** `session.store` template from the runtime config. Lets the direct/denied
|
||||
* exec approval followup path resolve the session key's current sessionId and
|
||||
* drop the followup when the key was rebound by `/new` or `/reset`. */
|
||||
sessionStore?: string;
|
||||
/** `session.mainKey` from the runtime config; passed through into
|
||||
* runExecProcess so background-exit notifications can remap cron-run
|
||||
* session keys to the agent's main queue without an ambient config load. */
|
||||
|
||||
@@ -1758,6 +1758,8 @@ export function createExecTool(
|
||||
requestedNode: params.node?.trim(),
|
||||
boundNode: defaults?.node?.trim(),
|
||||
sessionKey: defaults?.sessionKey,
|
||||
sessionId: defaults?.sessionId,
|
||||
sessionStore: defaults?.sessionStore,
|
||||
bashElevated: elevatedDefaults,
|
||||
turnSourceChannel: defaults?.messageProvider,
|
||||
turnSourceTo: defaults?.currentChannelId,
|
||||
@@ -1806,6 +1808,8 @@ export function createExecTool(
|
||||
trigger: defaults?.trigger,
|
||||
agentId,
|
||||
sessionKey: defaults?.sessionKey,
|
||||
sessionId: defaults?.sessionId,
|
||||
sessionStore: defaults?.sessionStore,
|
||||
bashElevated: elevatedDefaults,
|
||||
turnSourceChannel: defaults?.messageProvider,
|
||||
turnSourceTo: defaults?.currentChannelId,
|
||||
|
||||
@@ -3005,6 +3005,63 @@ describe("gateway agent handler", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("drops a stale exec approval followup at preflight without touching the rebound session (#59349)", async () => {
|
||||
const bashElevated = {
|
||||
enabled: true,
|
||||
allowed: true,
|
||||
defaultLevel: "on" as const,
|
||||
};
|
||||
const registration = registerExecApprovalFollowupRuntimeHandoff({
|
||||
approvalId: "req-rebound-followup",
|
||||
sessionKey: "agent:main:telegram:direct:123",
|
||||
bashElevated,
|
||||
});
|
||||
if (!registration) {
|
||||
throw new Error("expected runtime handoff id");
|
||||
}
|
||||
// Session was rebound by /new or /reset: current sessionId differs from the
|
||||
// approval-time sessionId carried on the request.
|
||||
mockMainSessionEntry({
|
||||
sessionId: "current-session-after-reset",
|
||||
lastChannel: "telegram",
|
||||
lastTo: "123",
|
||||
});
|
||||
const context = makeContext();
|
||||
const updateSessionStoreCallsBefore = mocks.updateSessionStore.mock.calls.length;
|
||||
const agentCommandCallsBefore = mocks.agentCommand.mock.calls.length;
|
||||
|
||||
const respond = await invokeAgent(
|
||||
{
|
||||
message: "exec followup",
|
||||
sessionKey: "agent:main:telegram:direct:123",
|
||||
channel: "telegram",
|
||||
idempotencyKey: registration.idempotencyKey,
|
||||
internalRuntimeHandoffId: registration.handoffId,
|
||||
execApprovalFollowupExpectedSessionId: "approval-time-session-id",
|
||||
},
|
||||
{
|
||||
reqId: "exec-followup-rebound-drop",
|
||||
client: backendGatewayClient(),
|
||||
context,
|
||||
flushDispatch: false,
|
||||
},
|
||||
);
|
||||
|
||||
expect(mockCallArg(respond, 0, 1)).toMatchObject({
|
||||
runId: registration.idempotencyKey,
|
||||
status: "ok",
|
||||
summary: expect.stringContaining("exec approval followup dropped"),
|
||||
});
|
||||
expect(mocks.updateSessionStore.mock.calls.length).toBe(updateSessionStoreCallsBefore);
|
||||
expect(mocks.agentCommand.mock.calls.length).toBe(agentCommandCallsBefore);
|
||||
const dedupeEntry = context.dedupe.get("agent:exec-approval-followup:req-rebound-followup");
|
||||
expect(dedupeEntry?.ok).toBe(true);
|
||||
expect(dedupeEntry?.payload).toMatchObject({
|
||||
status: "ok",
|
||||
summary: expect.stringContaining("exec approval followup dropped"),
|
||||
});
|
||||
});
|
||||
|
||||
it("does not honor caller-supplied exec approval runtime handoff ids without registry state", async () => {
|
||||
mockMainSessionEntry({
|
||||
sessionId: "existing-session-id",
|
||||
|
||||
@@ -28,6 +28,7 @@ import { listAgentIds, resolveDefaultAgentId } from "../../agents/agent-scope.js
|
||||
import { resolveTrustedGroupId } from "../../agents/agent-tools.policy.js";
|
||||
import {
|
||||
consumeExecApprovalFollowupRuntimeHandoff,
|
||||
isExecApprovalFollowupSessionRebound,
|
||||
parseExecApprovalFollowupApprovalId,
|
||||
} from "../../agents/bash-tools.exec-approval-followup-state.js";
|
||||
import { clearAllCliSessions } from "../../agents/cli-session.js";
|
||||
@@ -1061,6 +1062,7 @@ export const agentHandlers: GatewayRequestHandlers = {
|
||||
bootstrapContextRunKind?: "default" | "heartbeat" | "cron";
|
||||
acpTurnSource?: "manual_spawn";
|
||||
internalRuntimeHandoffId?: string;
|
||||
execApprovalFollowupExpectedSessionId?: string;
|
||||
internalEvents?: AgentInternalEvent[];
|
||||
suppressPromptPersistence?: boolean;
|
||||
sessionEffects?: "visible" | "internal";
|
||||
@@ -1300,6 +1302,44 @@ export const agentHandlers: GatewayRequestHandlers = {
|
||||
agentId = inferredAgentId;
|
||||
}
|
||||
}
|
||||
// Drop an exec-approval followup whose session key was rebound by /new or
|
||||
// /reset while the approval was pending, before the handler touches the
|
||||
// rebound session (store write, run registration, dedupe, accepted ack).
|
||||
if (execApprovalFollowupApprovalId && requestedSessionKeyRaw) {
|
||||
const expectedSessionId = normalizeOptionalString(
|
||||
request.execApprovalFollowupExpectedSessionId,
|
||||
);
|
||||
let currentSessionId: string | undefined;
|
||||
try {
|
||||
currentSessionId = normalizeOptionalString(
|
||||
loadSessionEntry(requestedSessionKeyRaw).entry?.sessionId,
|
||||
);
|
||||
} catch {
|
||||
currentSessionId = undefined;
|
||||
}
|
||||
if (
|
||||
isExecApprovalFollowupSessionRebound({
|
||||
expectedSessionId,
|
||||
resolvedSessionId: currentSessionId,
|
||||
})
|
||||
) {
|
||||
context.logGateway.info(
|
||||
`Dropping stale exec approval followup ${execApprovalFollowupApprovalId}: session ${requestedSessionKeyRaw} rebound (expected ${expectedSessionId}, current ${currentSessionId}) before the approval resolved`,
|
||||
);
|
||||
const droppedPayload = {
|
||||
runId,
|
||||
status: "ok" as const,
|
||||
summary: "exec approval followup dropped: session was reset before the approval resolved",
|
||||
};
|
||||
setGatewayDedupeEntries({
|
||||
dedupe: context.dedupe,
|
||||
keys: agentDedupeKeys,
|
||||
entry: { ts: Date.now(), ok: true, payload: droppedPayload },
|
||||
});
|
||||
respond(true, droppedPayload, undefined, { runId });
|
||||
return;
|
||||
}
|
||||
}
|
||||
const requestedSessionId = normalizeOptionalString(request.sessionId);
|
||||
let requestedSessionKey =
|
||||
requestedSessionKeyRaw ??
|
||||
|
||||
Reference in New Issue
Block a user