mirror of
https://github.com/agentclientprotocol/claude-agent-acp.git
synced 2026-10-27 04:30:59 +00:00
fix: recreate resumed sessions when any session option changes (#1097)
A warm `session/resume` or `session/load` currently ignores changed `maxTurns` and `maxBudgetUsd`: the session fingerprint compares only cwd and MCP servers, so the SDK query retains its old options. A client can request a tighter limit and receive success while the old limit remains active. Include these two SDK execution limits in the session fingerprint. Adding, changing or removing a limit recreates the query with the supplied options and the same session ID. Identical limits reuse the existing query; object key order does not trigger recreation. Budget accounting across recreated SDK processes remains the client's responsibility. Validation: - Four regression cases failed before the fix; six new cases cover both methods, both limits, tightening/removal, and unchanged reuse. - `npm run check` and `npm run build` passed. - Focused suites: 551 passed, 9 skipped. - Full suite with `--maxWorkers=2`: 1,240 passed, 27 skipped. An initial unrestricted run hit one existing provider-switch test's 5s timeout and macOS `EMFILE` watcher errors; bounding workers passed without changing tests or timeouts. Prepared while integrating typed runtime limits in BinaryBourbon/fountain#1732. No live SDK budget-enforcement claim is made by these adapter tests. --------- Signed-off-by: Jake Gaylor <jhgaylor@gmail.com> Co-authored-by: Ben Brandt <benjamin.j.brandt@gmail.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
135
src/acp-agent.ts
135
src/acp-agent.ts
@@ -1208,25 +1208,138 @@ function normalizeSkills(skills: Options["skills"]): Options["skills"] {
|
||||
return Array.isArray(skills) ? [...new Set(skills)].sort() : skills;
|
||||
}
|
||||
|
||||
/** Whether a changed `_meta.claudeCode.options` key on session/load or
|
||||
* session/resume must rebuild the Query process. Almost every option is read
|
||||
* only when the process starts, so the default is `true`. Every key of the
|
||||
* SDK's `Options` must be classified: an SDK bump that adds or removes an
|
||||
* option fails to compile until it is decided here. */
|
||||
const OPTION_REBUILDS_SESSION = {
|
||||
additionalDirectories: true,
|
||||
agentProgressSummaries: true,
|
||||
agents: true,
|
||||
allowDangerouslySkipPermissions: true,
|
||||
allowedTools: true,
|
||||
betas: true,
|
||||
debug: true,
|
||||
debugFile: true,
|
||||
disallowedTools: true,
|
||||
effort: true,
|
||||
enableFileCheckpointing: true,
|
||||
env: true,
|
||||
executableArgs: true,
|
||||
extraArgs: true,
|
||||
fallbackModel: true,
|
||||
forwardSubagentText: true,
|
||||
includeHookEvents: true,
|
||||
managedSettings: true,
|
||||
maxBudgetUsd: true,
|
||||
maxThinkingTokens: true,
|
||||
maxTurns: true,
|
||||
mcpServers: true,
|
||||
model: true,
|
||||
outputFormat: true,
|
||||
pathToClaudeCodeExecutable: true,
|
||||
permissionPrompts: true,
|
||||
permissionPromptToolName: true,
|
||||
persistSession: true,
|
||||
perTaskStopAffordance: true,
|
||||
planModeInstructions: true,
|
||||
pluginDelivery: true,
|
||||
plugins: true,
|
||||
projectConfigRoot: true,
|
||||
promptSuggestions: true,
|
||||
sandbox: true,
|
||||
sessionStoreFlush: true,
|
||||
settings: true,
|
||||
settingSources: true,
|
||||
skills: true,
|
||||
strictMcpConfig: true,
|
||||
supportedDialogKinds: true,
|
||||
systemPrompt: true,
|
||||
taskBudget: true,
|
||||
thinking: true,
|
||||
toolAliases: true,
|
||||
toolConfig: true,
|
||||
tools: true,
|
||||
verbatimPrompts: true,
|
||||
// Managed or ignored by the adapter.
|
||||
agent: false,
|
||||
cwd: false,
|
||||
executable: false,
|
||||
includePartialMessages: false,
|
||||
permissionMode: false,
|
||||
// Per-call controls for attaching to the stored conversation.
|
||||
continue: false,
|
||||
forkSession: false,
|
||||
resume: false,
|
||||
resumeDropsTurn: false,
|
||||
resumeSessionAt: false,
|
||||
sessionId: false,
|
||||
// Callbacks and live objects: they cannot cross JSON-RPC and have no stable serialized form.
|
||||
abortController: false,
|
||||
canUseTool: false,
|
||||
hooks: false,
|
||||
onElicitation: false,
|
||||
onUserDialog: false,
|
||||
sessionStore: false,
|
||||
spawnClaudeCodeProcess: false,
|
||||
stderr: false,
|
||||
// No effect on an already-created session.
|
||||
loadTimeoutMs: false,
|
||||
title: false,
|
||||
} as const satisfies Record<keyof Options, boolean>;
|
||||
|
||||
/** A JSON value with object keys sorted, so key order never changes a
|
||||
* fingerprint. Functions and class instances (hook callbacks, abort
|
||||
* controllers, in-process MCP servers) cannot cross JSON-RPC and have no
|
||||
* stable serialized form, so they are left out. */
|
||||
function canonicalJson(value: unknown): unknown {
|
||||
if (typeof value === "function") return undefined;
|
||||
if (value === null || typeof value !== "object") return value;
|
||||
if (Array.isArray(value)) return value.map(canonicalJson);
|
||||
const proto = Object.getPrototypeOf(value);
|
||||
if (proto !== Object.prototype && proto !== null) return undefined;
|
||||
return Object.fromEntries(
|
||||
Object.keys(value)
|
||||
.sort()
|
||||
.map((key) => [key, canonicalJson((value as Record<string, unknown>)[key])])
|
||||
.filter(([, entry]) => entry !== undefined),
|
||||
);
|
||||
}
|
||||
|
||||
/** Compute a stable fingerprint of the session-defining params so we can
|
||||
* detect when a loadSession/resumeSession call requires tearing down and
|
||||
* recreating the underlying Query process. MCP servers are sorted by name,
|
||||
* and skills are normalized as a set, so ordering differences don't trigger
|
||||
* unnecessary recreations. */
|
||||
function computeSessionFingerprint(params: {
|
||||
* recreating the underlying Query process. Every option classified in
|
||||
* {@link OPTION_REBUILDS_SESSION} is covered, so a warm resume never silently
|
||||
* keeps stale values. MCP servers are sorted by name, and skills are
|
||||
* normalized as a set, so ordering differences don't trigger unnecessary
|
||||
* recreations. */
|
||||
export function computeSessionFingerprint(params: {
|
||||
cwd: string;
|
||||
mcpServers?: NewSessionRequest["mcpServers"];
|
||||
additionalDirectories?: NewSessionRequest["additionalDirectories"];
|
||||
_meta?: NewSessionRequest["_meta"];
|
||||
}): string {
|
||||
const servers = [...(params.mcpServers ?? [])].sort((a, b) => a.name.localeCompare(b.name));
|
||||
const skills = normalizeSkills(
|
||||
(params._meta as NewSessionMeta | undefined)?.claudeCode?.options?.skills,
|
||||
const meta = params._meta as (NewSessionMeta & Record<string, unknown>) | undefined;
|
||||
const options: Record<string, unknown> = Object.fromEntries(
|
||||
Object.entries(meta?.claudeCode?.options ?? {}).filter(
|
||||
([key]) => (OPTION_REBUILDS_SESSION as Record<string, boolean>)[key] === true,
|
||||
),
|
||||
);
|
||||
options.skills = normalizeSkills(options.skills as Options["skills"]);
|
||||
return JSON.stringify(
|
||||
canonicalJson({
|
||||
cwd: params.cwd,
|
||||
mcpServers: servers,
|
||||
additionalDirectories: params.additionalDirectories,
|
||||
additionalRoots: meta?.additionalRoots,
|
||||
systemPrompt: meta?.systemPrompt,
|
||||
disableBuiltInTools: meta?.disableBuiltInTools,
|
||||
emitRawSDKMessages: meta?.claudeCode?.emitRawSDKMessages,
|
||||
options,
|
||||
}),
|
||||
);
|
||||
return JSON.stringify({
|
||||
cwd: params.cwd,
|
||||
mcpServers: servers,
|
||||
...(skills !== undefined && { skills }),
|
||||
});
|
||||
}
|
||||
|
||||
export type SDKMessageFilter = {
|
||||
|
||||
@@ -37,6 +37,7 @@ import {
|
||||
stripLocalCommandMetadata,
|
||||
ClaudeAcpAgent,
|
||||
claudeCliPath,
|
||||
computeSessionFingerprint,
|
||||
streamEventToAcpNotifications,
|
||||
messageIdForGrouping,
|
||||
buildConfigOptions,
|
||||
@@ -9419,11 +9420,15 @@ describe("getOrCreateSession param change detection", () => {
|
||||
cwd?: string;
|
||||
mcpServers?: { name: string }[];
|
||||
skills?: Options["skills"];
|
||||
options?: Options;
|
||||
} = {},
|
||||
) {
|
||||
const cwd = opts.cwd ?? "/test";
|
||||
const mcpServers = (opts.mcpServers ?? []) as any[];
|
||||
const skills = Array.isArray(opts.skills) ? [...new Set(opts.skills)].sort() : opts.skills;
|
||||
const options = {
|
||||
...opts.options,
|
||||
...(opts.skills !== undefined && { skills: opts.skills }),
|
||||
};
|
||||
function* empty() {}
|
||||
const gen = Object.assign(empty(), {
|
||||
interrupt: vi.fn(),
|
||||
@@ -9436,10 +9441,10 @@ describe("getOrCreateSession param change detection", () => {
|
||||
cancelled: false,
|
||||
titles: new SessionTitles(agent, sessionId),
|
||||
cwd,
|
||||
sessionFingerprint: JSON.stringify({
|
||||
sessionFingerprint: computeSessionFingerprint({
|
||||
cwd,
|
||||
mcpServers: [...mcpServers].sort((a: any, b: any) => a.name.localeCompare(b.name)),
|
||||
...(skills !== undefined && { skills }),
|
||||
mcpServers,
|
||||
_meta: { claudeCode: { options } },
|
||||
}),
|
||||
modes: { currentModeId: "default", availableModes: [] },
|
||||
models: { currentModelId: "default", availableModels: [] },
|
||||
@@ -9623,9 +9628,54 @@ describe("getOrCreateSession param change detection", () => {
|
||||
expect(session.settingsManager.dispose).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("ignores unrelated Claude options when computing the fingerprint", async () => {
|
||||
it("tears down the existing session when any other Claude option changes", async () => {
|
||||
const agent = createMockAgent();
|
||||
const session = injectSession(agent, "s1", {
|
||||
cwd: "/project",
|
||||
options: { env: { CUSTOM_ENV: "original" } },
|
||||
});
|
||||
const createSessionSpy = vi
|
||||
.spyOn(agent as any, "createSession")
|
||||
.mockRejectedValue(new Error("mock"));
|
||||
|
||||
await expect(
|
||||
agent.resumeSession({
|
||||
sessionId: "s1",
|
||||
cwd: "/project",
|
||||
mcpServers: [],
|
||||
_meta: { claudeCode: { options: { env: { CUSTOM_ENV: "changed" } } } },
|
||||
}),
|
||||
).rejects.toThrow("mock");
|
||||
|
||||
expect(session.settingsManager.dispose).toHaveBeenCalled();
|
||||
expect(agent.sessions["s1"]).toBeUndefined();
|
||||
expect(createSessionSpy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("tears down the existing session when additionalDirectories change", async () => {
|
||||
const agent = createMockAgent();
|
||||
const session = injectSession(agent, "s1", { cwd: "/project" });
|
||||
vi.spyOn(agent as any, "createSession").mockRejectedValue(new Error("mock"));
|
||||
|
||||
await expect(
|
||||
agent.resumeSession({
|
||||
sessionId: "s1",
|
||||
cwd: "/project",
|
||||
mcpServers: [],
|
||||
additionalDirectories: ["/other"],
|
||||
}),
|
||||
).rejects.toThrow("mock");
|
||||
|
||||
expect(session.settingsManager.dispose).toHaveBeenCalled();
|
||||
expect(agent.sessions["s1"]).toBeUndefined();
|
||||
});
|
||||
|
||||
it("ignores option key order, per-call resume controls and creation-only options", async () => {
|
||||
const agent = createMockAgent();
|
||||
const session = injectSession(agent, "s1", {
|
||||
cwd: "/project",
|
||||
options: { maxTurns: 10, env: { A: "1", B: "2" } },
|
||||
});
|
||||
|
||||
await agent.resumeSession({
|
||||
sessionId: "s1",
|
||||
@@ -9633,7 +9683,17 @@ describe("getOrCreateSession param change detection", () => {
|
||||
mcpServers: [],
|
||||
_meta: {
|
||||
claudeCode: {
|
||||
options: { env: { CUSTOM_ENV: "changed" } },
|
||||
options: {
|
||||
env: { B: "2", A: "1" },
|
||||
maxTurns: 10,
|
||||
resume: "s1",
|
||||
sessionId: "s1",
|
||||
forkSession: false,
|
||||
resumeSessionAt: "msg-1",
|
||||
resumeDropsTurn: "msg-1",
|
||||
title: "Renamed",
|
||||
abortController: new AbortController(),
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
@@ -99,6 +99,48 @@ describe("createSession options merging", () => {
|
||||
agent = new ClaudeAcpAgent(createMockClient());
|
||||
});
|
||||
|
||||
for (const method of ["resumeSession", "loadSession"] as const) {
|
||||
for (const limit of ["maxTurns", "maxBudgetUsd"] as const) {
|
||||
it(`${method} applies added, tightened and removed ${limit} to the SDK query`, async () => {
|
||||
const cwd = process.cwd();
|
||||
const { sessionId } = await agent.newSession({ cwd, mcpServers: [] });
|
||||
|
||||
for (const value of [10, 1, undefined]) {
|
||||
const previousOptions = capturedOptions;
|
||||
await agent[method]({
|
||||
sessionId,
|
||||
cwd,
|
||||
mcpServers: [],
|
||||
_meta: { claudeCode: { options: { [limit]: value } } },
|
||||
});
|
||||
|
||||
expect(capturedOptions).not.toBe(previousOptions);
|
||||
expect(capturedOptions?.[limit]).toBe(value);
|
||||
expect(capturedOptions?.resume).toBe(sessionId);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
it(`${method} reuses the SDK query when execution limits are unchanged`, async () => {
|
||||
const cwd = process.cwd();
|
||||
const { sessionId } = await agent.newSession({
|
||||
cwd,
|
||||
mcpServers: [],
|
||||
_meta: { claudeCode: { options: { maxTurns: 10, maxBudgetUsd: 1 } } },
|
||||
});
|
||||
const previousOptions = capturedOptions;
|
||||
|
||||
await agent[method]({
|
||||
sessionId,
|
||||
cwd,
|
||||
mcpServers: [],
|
||||
_meta: { claudeCode: { options: { maxBudgetUsd: 1, maxTurns: 10 } } },
|
||||
});
|
||||
|
||||
expect(capturedOptions).toBe(previousOptions);
|
||||
});
|
||||
}
|
||||
|
||||
describe("allowDangerouslySkipPermissions", () => {
|
||||
it("requests bypass capability by default and mirrors it in the mode catalog", async () => {
|
||||
const response = await agent.newSession({ cwd: process.cwd(), mcpServers: [] });
|
||||
|
||||
Reference in New Issue
Block a user