-
-
Notifications
You must be signed in to change notification settings - Fork 860
fix(acp): add tests asserting cwd and mcpServers are always passed to session/load (#1593) #1624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<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: [], | ||
| }); | ||
| }); | ||
| }); | ||
|
Comment on lines
+2180
to
+2340
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The same 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! |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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 ofinitialHandle. 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 invokeinitializeResumedSessionthrough 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)