fix(acp): add tests asserting cwd and mcpServers are always passed to session/load (#1593)#1624
fix(acp): add tests asserting cwd and mcpServers are always passed to session/load (#1593)#1624theslava wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| class TestSession extends ACPAgentSession { | ||
| protected override async spawnProcess(): Promise<SpawnedACPProcess> { | ||
| 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<SpawnedACPProcess> { | ||
| 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<SpawnedACPProcess> { | ||
| 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: [], | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The same TestSession subclass (with an identical spawnProcess() override that returns a stub child + connection) is defined and re-instantiated in each of the three tests. Per the project's test discipline guide, copy-paste duplication for setup mechanics belongs in a shared helper. Extracting a factory such as makeTestSession(connectionStub, capabilities?) would cut each test body in half, make the session-config variation explicit, and ensure all three tests break or pass together if SpawnedACPProcess's shape changes.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| (session as unknown as { initialHandle: unknown }).initialHandle = { | ||
| sessionId: "session-1", | ||
| provider: "claude-acp", | ||
| }; |
There was a problem hiding this comment.
(session as unknown as { initialHandle: unknown }).initialHandle = ... bypasses TypeScript access control to set what is presumably a private or protected field. The rules flag "assert private fields" as a smell because the test now depends on the internal name and type of initialHandle. If that field gets renamed, moved to a constructor parameter, or replaced by a factory method, these tests silently stop compiling or start asserting the wrong thing. A cleaner setup would invoke initializeResumedSession through a higher-level entry point that already sets the persistence handle, or expose a typed constructor overload/factory for tests.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
theslava
force-pushed
the
fix/acp-session-load-invariant
branch
2 times, most recently
from
8ac5c4b to
2144f4f
Compare
… session/load (getpaseo#1593) 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 getpaseo#1593
- Extract shared makeTestSession() factory to eliminate duplicated TestSession class definitions across all three tests (concern getpaseo#1) - Pass handle through typed constructor option instead of casting private initialHandle field (concern getpaseo#2) - Add missing type imports for AgentCapabilityFlags and AgentPersistenceHandle
theslava
force-pushed
the
fix/acp-session-load-invariant
branch
from
2144f4f to
59180ec
Compare
1 participant
Linked issue
Closes #1593
Type of change
What does this PR do
Adds unit tests in
acp-agent.test.tsthat verifyinitializeResumedSession()always callsloadSessionandunstable_resumeSessionwith{ sessionId, cwd, mcpServers }— even whenmcpServersis 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.How did you verify it
npx vitest run packages/server/src/server/agent/providers/acp-agent.test.ts --bail=1— 59 tests passednpm run typecheck— passednpm run lint— passednpm run format— ranRisk surface
Tests only. No behavioral change — the existing code already passes all three params. This guards against a future refactor that might accidentally drop
cwdormcpServers.Checklist
npm run typecheckpassesnpm run lintpassesnpm run formatran (Biome)