From 8483930cdbb82272e87e06516f48ff2ca2cbae38 Mon Sep 17 00:00:00 2001 From: Slava Goltser Date: Fri, 19 Jun 2026 23:08:37 -0400 Subject: [PATCH] fix(acp): add tests asserting cwd and mcpServers are always passed to session/load (#1593) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add unit tests in acp-agent.test.ts that verify initializeResumedSession() always calls loadSession and unstable_resumeSession with { sessionId, cwd, mcpServers } — even when mcpServers is an empty array. Some strict ACP providers (e.g., Devin CLI) return 'Invalid params' if any of these fields are omitted. Also adds a docstring above initializeResumedSession() documenting this requirement so future refactors don't accidentally drop params. Closes #1593 --- .../server/agent/providers/acp-agent.test.ts | 171 ++++++++++++++++++ .../src/server/agent/providers/acp-agent.ts | 7 + 2 files changed, 178 insertions(+) diff --git a/packages/server/src/server/agent/providers/acp-agent.test.ts b/packages/server/src/server/agent/providers/acp-agent.test.ts index f5d2701f8..14b88f986 100644 --- a/packages/server/src/server/agent/providers/acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/acp-agent.test.ts @@ -2167,3 +2167,174 @@ describe("ACPAgentClient probe cleanup", () => { expect(child.stderr.destroyed).toBe(true); }); }); + +describe("ACP session/load invariant — cwd and mcpServers always passed", () => { + test("loadSession is always called with sessionId, cwd, and mcpServers even when mcpServers is empty", async () => { + const loadSession = vi.fn().mockResolvedValue({ + sessionId: "session-1", + modes: null, + models: null, + configOptions: [], + }); + + class TestSession extends ACPAgentSession { + protected override async spawnProcess(): Promise { + return { + child: createProbeChildStub(), + connection: { + prompt: vi.fn(), + loadSession, + }, + initialize: { agentCapabilities: { loadSession: true } }, + } as unknown as SpawnedACPProcess; + } + } + + const session = new TestSession( + { + provider: "claude-acp", + cwd: "/tmp/paseo-acp-test", + }, + { + provider: "claude-acp", + logger: createTestLogger(), + defaultCommand: ["claude", "--acp"], + defaultModes: [], + capabilities: { + supportsStreaming: true, + supportsSessionPersistence: true, + supportsDynamicModes: true, + supportsMcpServers: true, + supportsReasoningStream: true, + supportsToolInvocations: true, + }, + }, + ); + // Provide the persistence handle that initializeResumedSession requires + (session as unknown as { initialHandle: unknown }).initialHandle = { + sessionId: "session-1", + provider: "claude-acp", + }; + + await session.initializeResumedSession(); + + expect(loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/tmp/paseo-acp-test", + mcpServers: [], + }); + }); + + test("loadSession is always called with mcpServers even when supportsMcpServers is false", async () => { + const loadSession = vi.fn().mockResolvedValue({ + sessionId: "session-1", + modes: null, + models: null, + configOptions: [], + }); + + class TestSession extends ACPAgentSession { + protected override async spawnProcess(): Promise { + return { + child: createProbeChildStub(), + connection: { + prompt: vi.fn(), + loadSession, + }, + initialize: { agentCapabilities: { loadSession: true } }, + } as unknown as SpawnedACPProcess; + } + } + + const session = new TestSession( + { + provider: "claude-acp", + cwd: "/tmp/paseo-acp-test", + }, + { + provider: "claude-acp", + logger: createTestLogger(), + defaultCommand: ["claude", "--acp"], + defaultModes: [], + capabilities: { + supportsStreaming: true, + supportsSessionPersistence: true, + supportsDynamicModes: true, + supportsMcpServers: false, + supportsReasoningStream: true, + supportsToolInvocations: true, + }, + }, + ); + (session as unknown as { initialHandle: unknown }).initialHandle = { + sessionId: "session-1", + provider: "claude-acp", + }; + + await session.initializeResumedSession(); + + // Even with supportsMcpServers=false, mcpServers: [] must still be passed + expect(loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/tmp/paseo-acp-test", + mcpServers: [], + }); + }); + + test("unstable_resumeSession is always called with sessionId, cwd, and mcpServers", async () => { + const unstableResumeSession = vi.fn().mockResolvedValue({ + sessionId: "session-1", + modes: null, + models: null, + configOptions: [], + }); + + class TestSession extends ACPAgentSession { + protected override async spawnProcess(): Promise { + return { + child: createProbeChildStub(), + connection: { + prompt: vi.fn(), + unstable_resumeSession: unstableResumeSession, + }, + initialize: { + agentCapabilities: { sessionCapabilities: { resume: {} } }, + }, + } as unknown as SpawnedACPProcess; + } + } + + const session = new TestSession( + { + provider: "claude-acp", + cwd: "/tmp/paseo-acp-test", + }, + { + provider: "claude-acp", + logger: createTestLogger(), + defaultCommand: ["claude", "--acp"], + defaultModes: [], + capabilities: { + supportsStreaming: true, + supportsSessionPersistence: true, + supportsDynamicModes: true, + supportsMcpServers: true, + supportsReasoningStream: true, + supportsToolInvocations: true, + }, + }, + ); + (session as unknown as { initialHandle: unknown }).initialHandle = { + sessionId: "session-1", + provider: "claude-acp", + }; + + await session.initializeResumedSession(); + + expect(unstableResumeSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/tmp/paseo-acp-test", + mcpServers: [], + }); + }); +}); diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index 662acf300..1abd0a5a2 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -1074,6 +1074,13 @@ export class ACPAgentSession implements AgentSession, ACPClient { await this.applyConfiguredOverrides(); } + /** + * IMPORTANT: Some ACP providers (e.g., Devin CLI) require all three params + * (sessionId, cwd, mcpServers) to be present in session/load or + * unstable_resumeSession — even when mcpServers is an empty array — and + * return "Invalid params" if any are omitted. Never drop cwd or mcpServers + * from these calls regardless of capabilities. + */ async initializeResumedSession(): Promise { const handle = this.initialHandle; if (!handle) {