Skip to content

Commit 453a262

Browse files
committed
fix: handle workspace folder changes in ql-mcp extension
- Invalidate cached environment when workspace folders change so the next server start picks up updated folder list - getEffectiveVersion() always returns a concrete string (never undefined) so VS Code has a reliable baseline for version comparison - Add multi-root workspace integration tests verifying environment correctness (CODEQL_MCP_WORKSPACE_FOLDERS includes/excludes folders) - Add unit test for workspace folder change handler behavior - Add unit test for always-defined version string - Add multiRoot test profile with 4 workspace folders
1 parent ecb2acc commit 453a262

9 files changed

Lines changed: 265 additions & 212 deletions

File tree

extensions/vscode/esbuild.config.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ const testSuiteConfig = {
4141
'test/suite/mcp-resource-e2e.integration.test.ts',
4242
'test/suite/mcp-server.integration.test.ts',
4343
'test/suite/mcp-tool-e2e.integration.test.ts',
44+
'test/suite/workspace-folder-change.integration.test.ts',
4445
'test/suite/workspace-scenario.integration.test.ts',
4546
],
4647
outdir: 'dist/test/suite',

extensions/vscode/src/extension.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -81,19 +81,21 @@ export async function activate(
8181
context.subscriptions.push(
8282
vscode.workspace.onDidChangeConfiguration((e) => {
8383
if (e.affectsConfiguration('codeql-mcp')) {
84-
logger.info('Configuration changed — rebuilding environment');
84+
logger.info('Configuration changed — requesting MCP server restart');
8585
envBuilder.invalidate();
86-
mcpProvider.fireDidChange();
86+
mcpProvider.requestRestart();
8787
}
8888
}),
8989
);
9090

91-
// Re-scan when workspace folders change
91+
// Invalidate cached environment when workspace folders change.
92+
// VS Code itself manages MCP server lifecycle when roots change
93+
// (stopping and restarting the server as needed). We just clear
94+
// the cached env so the next server start picks up updated folders.
9295
context.subscriptions.push(
9396
vscode.workspace.onDidChangeWorkspaceFolders(() => {
94-
logger.info('Workspace folders changed — refreshing watchers');
97+
logger.info('Workspace folders changed — invalidating environment cache');
9598
envBuilder.invalidate();
96-
mcpProvider.fireDidChange();
9799
}),
98100
);
99101

extensions/vscode/src/server/mcp-provider.ts

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,9 @@ export class McpProvider
2121
readonly onDidChangeMcpServerDefinitions = this._onDidChange.event;
2222

2323
/**
24-
* Monotonically increasing counter, bumped each time `fireDidChange()` is
25-
* called. Appended to the version string returned by
26-
* `provideMcpServerDefinitions()` so that VS Code sees a genuinely new
27-
* server definition and restarts the MCP server instead of only stopping it.
24+
* Monotonically increasing counter, bumped by `requestRestart()`.
25+
* Appended to the version string so that VS Code sees a genuinely new
26+
* server definition and triggers a stop → restart cycle.
2827
*/
2928
private _revision = 0;
3029

@@ -37,9 +36,34 @@ export class McpProvider
3736
this.push(this._onDidChange);
3837
}
3938

40-
/** Notify VS Code that the MCP server definitions have changed. */
39+
/**
40+
* Soft notification: tell VS Code that definitions may have changed.
41+
*
42+
* Does NOT bump the version, so VS Code will re-query
43+
* `provideMcpServerDefinitions()` / `resolveMcpServerDefinition()` but
44+
* will NOT restart the server. Use for lightweight updates (file watcher
45+
* events, extension changes, background install completion) where the
46+
* running server can continue with its current environment.
47+
*/
4148
fireDidChange(): void {
49+
this._onDidChange.fire();
50+
}
51+
52+
/**
53+
* Request that VS Code restart the MCP server with a fresh environment.
54+
*
55+
* Bumps the internal revision counter so that the next call to
56+
* `provideMcpServerDefinitions()` returns a definition with a different
57+
* `version` string. VS Code compares the new version to the running
58+
* server's version and, seeing a change, triggers a stop → start cycle.
59+
*
60+
* Use for changes that require a server restart (configuration changes).
61+
*/
62+
requestRestart(): void {
4263
this._revision++;
64+
this.logger.info(
65+
`Requesting ql-mcp restart (revision ${this._revision})...`,
66+
);
4367
this._onDidChange.fire();
4468
}
4569

@@ -79,22 +103,24 @@ export class McpProvider
79103
/**
80104
* Computes the version string for the MCP server definition.
81105
*
82-
* Before any `fireDidChange()` call, returns the raw version from
83-
* `serverManager.getVersion()` (preserving the original behaviour).
106+
* Always returns a concrete string so that VS Code has a reliable
107+
* baseline for version comparison. When `serverManager.getVersion()`
108+
* returns `undefined` (the "latest" / unpinned case), the extension
109+
* version is used as the base instead.
84110
*
85-
* After one or more `fireDidChange()` calls, appends a `.N` revision
86-
* suffix so that the version is always different from the previous one.
87-
* This is essential because VS Code uses the version to decide whether to
88-
* restart a running server: if the version is unchanged, VS Code may stop
89-
* the server without restarting it.
111+
* After one or more `requestRestart()` calls, a `.N` revision suffix
112+
* is appended so that the version is always different from the
113+
* previous one. VS Code uses the version to decide whether to
114+
* restart a running server: a changed version triggers a stop → start
115+
* cycle.
90116
*/
91-
private getEffectiveVersion(): string | undefined {
92-
if (this._revision === 0) {
93-
return this.serverManager.getVersion();
94-
}
117+
private getEffectiveVersion(): string {
95118
const base =
96119
this.serverManager.getVersion() ??
97120
this.serverManager.getExtensionVersion();
121+
if (this._revision === 0) {
122+
return base;
123+
}
98124
return `${base}.${this._revision}`;
99125
}
100126
}

extensions/vscode/test/extension.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ vi.mock('../src/server/mcp-provider', () => ({
5050
resolveMcpServerDefinition: vi.fn().mockResolvedValue(undefined),
5151
onDidChangeMcpServerDefinitions: vi.fn(),
5252
fireDidChange: vi.fn(),
53+
requestRestart: vi.fn(),
5354
dispose: vi.fn(),
5455
};
5556
}),
@@ -102,6 +103,8 @@ vi.mock('../src/bridge/environment-builder', () => ({
102103

103104
import { activate, deactivate } from '../src/extension';
104105
import * as vscode from 'vscode';
106+
import { McpProvider } from '../src/server/mcp-provider';
107+
import { EnvironmentBuilder } from '../src/bridge/environment-builder';
105108

106109
function createMockContext(): vscode.ExtensionContext {
107110
return {
@@ -168,4 +171,36 @@ describe('Extension', () => {
168171
it('should deactivate even if activate was never called', () => {
169172
expect(() => deactivate()).not.toThrow();
170173
});
174+
175+
it('should invalidate env cache on workspace folder changes', async () => {
176+
// Capture the workspace folder change callback
177+
let workspaceFolderChangeCallback: Function | undefined;
178+
const vscodeMock = vi.mocked(vscode);
179+
vscodeMock.workspace.onDidChangeWorkspaceFolders = vi.fn().mockImplementation(
180+
(cb: Function) => { workspaceFolderChangeCallback = cb; return { dispose: vi.fn() }; },
181+
);
182+
183+
await activate(ctx);
184+
185+
// The extension should have registered a workspace folder change handler
186+
expect(workspaceFolderChangeCallback).toBeDefined();
187+
188+
// Get the mock instances created during activation
189+
const mcpProviderInstance = vi.mocked(McpProvider).mock.results[0]?.value;
190+
const envBuilderInstance = vi.mocked(EnvironmentBuilder).mock.results[0]?.value;
191+
192+
// Reset call counts from activation
193+
mcpProviderInstance.fireDidChange.mockClear();
194+
mcpProviderInstance.requestRestart.mockClear();
195+
envBuilderInstance.invalidate.mockClear();
196+
197+
// Simulate workspace folder change
198+
workspaceFolderChangeCallback!();
199+
200+
// Cache invalidated immediately; no VS Code notification.
201+
// VS Code manages MCP server lifecycle (stop/restart) when roots change.
202+
expect(envBuilderInstance.invalidate).toHaveBeenCalledTimes(1);
203+
expect(mcpProviderInstance.fireDidChange).not.toHaveBeenCalled();
204+
expect(mcpProviderInstance.requestRestart).not.toHaveBeenCalled();
205+
});
171206
});

extensions/vscode/test/fixtures/multi-root-workspace/folder-c/.gitkeep

Whitespace-only changes.

extensions/vscode/test/fixtures/multi-root-workspace/folder-d/.gitkeep

Whitespace-only changes.
Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,20 @@
11
{
22
"folders": [
3-
{ "path": "folder-a" },
4-
{ "path": "folder-b" }
3+
{
4+
"path": "folder-a"
5+
},
6+
{
7+
"path": "folder-b"
8+
},
9+
{
10+
"path": "folder-c"
11+
},
12+
{
13+
"path": "folder-d"
14+
},
15+
{
16+
"name": "ql-mcp-seq2-DMXgqX",
17+
"path": "../../../../../../../../../../private/var/folders/m_/6l74bbjj5wzf7vgl0mxmgwzm0000gn/T/ql-mcp-seq2-DMXgqX"
18+
}
519
]
620
}

extensions/vscode/test/server/mcp-provider.test.ts

Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,22 @@ describe('McpProvider', () => {
112112
expect(definitions).toHaveLength(1);
113113
});
114114

115+
it('should always provide a defined version string even when serverVersion is latest', async () => {
116+
// When serverManager.getVersion() returns undefined ("latest" mode),
117+
// the definition must still carry a concrete version string so that
118+
// VS Code has a baseline for version comparison. An undefined initial
119+
// version prevents VS Code from detecting changes after requestRestart().
120+
serverManager.getVersion.mockReturnValue(undefined);
121+
serverManager.getExtensionVersion.mockReturnValue('2.25.1');
122+
123+
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
124+
const definitions = await provider.provideMcpServerDefinitions(token as any);
125+
126+
expect(definitions![0].version).toBeDefined();
127+
expect(typeof definitions![0].version).toBe('string');
128+
expect(definitions![0].version!.length).toBeGreaterThan(0);
129+
});
130+
115131
it('should pass version from serverManager when pinned', async () => {
116132
serverManager.getVersion.mockReturnValue('2.20.0');
117133

@@ -121,52 +137,72 @@ describe('McpProvider', () => {
121137
expect(definitions![0].version).toBe('2.20.0');
122138
});
123139

124-
it('should produce a different version after fireDidChange to trigger server restart', async () => {
140+
it('should not change version on fireDidChange (soft signal)', async () => {
125141
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
126142
const before = await provider.provideMcpServerDefinitions(token as any);
127143
const versionBefore = before![0].version;
128144

129145
provider.fireDidChange();
130146
const after = await provider.provideMcpServerDefinitions(token as any);
147+
148+
expect(after![0].version).toBe(versionBefore);
149+
});
150+
151+
it('should produce a different version after requestRestart', async () => {
152+
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
153+
const before = await provider.provideMcpServerDefinitions(token as any);
154+
const versionBefore = before![0].version;
155+
156+
provider.requestRestart();
157+
const after = await provider.provideMcpServerDefinitions(token as any);
131158
const versionAfter = after![0].version;
132159

133160
expect(versionAfter).toBeDefined();
134161
expect(versionAfter).not.toBe(versionBefore);
135162
});
136163

137-
it('should produce distinct versions after successive fireDidChange calls', async () => {
164+
it('should produce distinct versions after successive requestRestart calls', async () => {
138165
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
139166

140-
provider.fireDidChange();
167+
provider.requestRestart();
141168
const defs1 = await provider.provideMcpServerDefinitions(token as any);
142169

143-
provider.fireDidChange();
170+
provider.requestRestart();
144171
const defs2 = await provider.provideMcpServerDefinitions(token as any);
145172

146173
expect(defs1![0].version).not.toBe(defs2![0].version);
147174
});
148175

149-
it('should append revision to pinned version after fireDidChange', async () => {
176+
it('should append revision to pinned version after requestRestart', async () => {
150177
serverManager.getVersion.mockReturnValue('2.20.0');
151178
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
152179

153-
provider.fireDidChange();
180+
provider.requestRestart();
154181
const definitions = await provider.provideMcpServerDefinitions(token as any);
155182

156183
expect(definitions![0].version).toMatch(/^2\.20\.0\.\d+$/);
157184
});
158185

159-
it('should append revision to extension version after fireDidChange when version is latest', async () => {
186+
it('should append revision to extension version after requestRestart when version is latest', async () => {
160187
serverManager.getVersion.mockReturnValue(undefined);
161188
serverManager.getExtensionVersion.mockReturnValue('2.25.1');
162189
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
163190

164-
provider.fireDidChange();
191+
provider.requestRestart();
165192
const definitions = await provider.provideMcpServerDefinitions(token as any);
166193

167194
expect(definitions![0].version).toMatch(/^2\.25\.1\.\d+$/);
168195
});
169196

197+
it('should fire change event when requestRestart is called', () => {
198+
const listener = vi.fn();
199+
provider.onDidChangeMcpServerDefinitions(listener);
200+
201+
provider.requestRestart();
202+
203+
expect(listener).toHaveBeenCalledTimes(1);
204+
});
205+
170206
it('should resolve definition by refreshing environment', async () => {
171207
const updatedEnv = { CODEQL_PATH: '/new/path', TRANSPORT_MODE: 'stdio' };
172208
envBuilder.build.mockResolvedValue(updatedEnv);

0 commit comments

Comments
 (0)