Skip to content

Commit 258def1

Browse files
committed
fix: restart MCP server on vscode workspace folder changes
When workspace folders were added or removed, fireDidChange() notified VS Code that the MCP server definitions changed, causing it to stop the running server. However, because the returned McpStdioServerDefinition had the same version as before, VS Code did not restart the server. Add a monotonically increasing revision counter to McpProvider that increments on each fireDidChange() call and is appended to the version string (e.g. 2.25.1.1, 2.25.1.2). This signals VS Code that the definition has genuinely changed and triggers a server restart with the updated environment instead of only stopping the server. - Add _revision counter and getEffectiveVersion() to McpProvider - Add 4 unit tests for version-bumping behaviour - Add integration test suite for workspace folder change scenarios
1 parent 1faad96 commit 258def1

File tree

3 files changed

+308
-1
lines changed

3 files changed

+308
-1
lines changed

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

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,14 @@ export class McpProvider
2020
private readonly _onDidChange = new vscode.EventEmitter<void>();
2121
readonly onDidChangeMcpServerDefinitions = this._onDidChange.event;
2222

23+
/**
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.
28+
*/
29+
private _revision = 0;
30+
2331
constructor(
2432
private readonly serverManager: ServerManager,
2533
private readonly envBuilder: EnvironmentBuilder,
@@ -31,6 +39,7 @@ export class McpProvider
3139

3240
/** Notify VS Code that the MCP server definitions have changed. */
3341
fireDidChange(): void {
42+
this._revision++;
3443
this._onDidChange.fire();
3544
}
3645

@@ -40,7 +49,7 @@ export class McpProvider
4049
const command = this.serverManager.getCommand();
4150
const args = this.serverManager.getArgs();
4251
const env = await this.envBuilder.build();
43-
const version = this.serverManager.getVersion();
52+
const version = this.getEffectiveVersion();
4453

4554
this.logger.info(
4655
`Providing MCP server definition: ${command} ${args.join(' ')}`,
@@ -66,4 +75,26 @@ export class McpProvider
6675
server.env = env;
6776
return server;
6877
}
78+
79+
/**
80+
* Computes the version string for the MCP server definition.
81+
*
82+
* Before any `fireDidChange()` call, returns the raw version from
83+
* `serverManager.getVersion()` (preserving the original behaviour).
84+
*
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.
90+
*/
91+
private getEffectiveVersion(): string | undefined {
92+
if (this._revision === 0) {
93+
return this.serverManager.getVersion();
94+
}
95+
const base =
96+
this.serverManager.getVersion() ??
97+
this.serverManager.getExtensionVersion();
98+
return `${base}.${this._revision}`;
99+
}
69100
}

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ function createMockServerManager() {
77
return {
88
getCommand: vi.fn().mockReturnValue('npx'),
99
getArgs: vi.fn().mockReturnValue(['-y', 'codeql-development-mcp-server']),
10+
getExtensionVersion: vi.fn().mockReturnValue('2.25.1'),
1011
getVersion: vi.fn().mockReturnValue(undefined),
1112
getDescription: vi.fn().mockReturnValue('npx -y codeql-development-mcp-server'),
1213
getInstallDir: vi.fn().mockReturnValue('/mock/install'),
@@ -120,6 +121,52 @@ describe('McpProvider', () => {
120121
expect(definitions![0].version).toBe('2.20.0');
121122
});
122123

124+
it('should produce a different version after fireDidChange to trigger server restart', async () => {
125+
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
126+
const before = await provider.provideMcpServerDefinitions(token as any);
127+
const versionBefore = before![0].version;
128+
129+
provider.fireDidChange();
130+
const after = await provider.provideMcpServerDefinitions(token as any);
131+
const versionAfter = after![0].version;
132+
133+
expect(versionAfter).toBeDefined();
134+
expect(versionAfter).not.toBe(versionBefore);
135+
});
136+
137+
it('should produce distinct versions after successive fireDidChange calls', async () => {
138+
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
139+
140+
provider.fireDidChange();
141+
const defs1 = await provider.provideMcpServerDefinitions(token as any);
142+
143+
provider.fireDidChange();
144+
const defs2 = await provider.provideMcpServerDefinitions(token as any);
145+
146+
expect(defs1![0].version).not.toBe(defs2![0].version);
147+
});
148+
149+
it('should append revision to pinned version after fireDidChange', async () => {
150+
serverManager.getVersion.mockReturnValue('2.20.0');
151+
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
152+
153+
provider.fireDidChange();
154+
const definitions = await provider.provideMcpServerDefinitions(token as any);
155+
156+
expect(definitions![0].version).toMatch(/^2\.20\.0\.\d+$/);
157+
});
158+
159+
it('should append revision to extension version after fireDidChange when version is latest', async () => {
160+
serverManager.getVersion.mockReturnValue(undefined);
161+
serverManager.getExtensionVersion.mockReturnValue('2.25.1');
162+
const token = { isCancellationRequested: false, onCancellationRequested: vi.fn() };
163+
164+
provider.fireDidChange();
165+
const definitions = await provider.provideMcpServerDefinitions(token as any);
166+
167+
expect(definitions![0].version).toMatch(/^2\.25\.1\.\d+$/);
168+
});
169+
123170
it('should resolve definition by refreshing environment', async () => {
124171
const updatedEnv = { CODEQL_PATH: '/new/path', TRANSPORT_MODE: 'stdio' };
125172
envBuilder.build.mockResolvedValue(updatedEnv);
Lines changed: 229 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,229 @@
1+
/**
2+
* Integration tests for workspace folder change handling.
3+
*
4+
* These run inside the Extension Development Host with the REAL VS Code API.
5+
* They verify that the MCP server definition version changes when workspace
6+
* folders are added or removed, which signals VS Code to restart the server
7+
* with the updated environment rather than simply stopping it.
8+
*
9+
* Bug: Prior to the fix, fireDidChange() notified VS Code that the MCP server
10+
* definitions changed, causing it to stop the running server. However, because
11+
* the returned McpStdioServerDefinition had the same version as before, VS Code
12+
* did not restart the server — it only stopped it.
13+
*/
14+
15+
import * as assert from 'assert';
16+
import * as fs from 'fs';
17+
import * as os from 'os';
18+
import * as path from 'path';
19+
import * as vscode from 'vscode';
20+
21+
const EXTENSION_ID = 'advanced-security.vscode-codeql-development-mcp-server';
22+
23+
suite('Workspace Folder Change Tests', () => {
24+
let api: any;
25+
26+
suiteSetup(async () => {
27+
const ext = vscode.extensions.getExtension(EXTENSION_ID);
28+
assert.ok(ext, `Extension ${EXTENSION_ID} not found`);
29+
api = ext.isActive ? ext.exports : await ext.activate();
30+
});
31+
32+
test('MCP definition version should change after workspace folder is added', async function () {
33+
const provider = api.mcpProvider;
34+
if (!provider) {
35+
this.skip();
36+
return;
37+
}
38+
39+
const token: vscode.CancellationToken = {
40+
isCancellationRequested: false,
41+
onCancellationRequested: () => ({ dispose: () => {} }),
42+
};
43+
44+
// Get initial definitions
45+
const beforeDefs = await provider.provideMcpServerDefinitions(token);
46+
assert.ok(beforeDefs && beforeDefs.length >= 1, 'Should provide at least one definition');
47+
const versionBefore = beforeDefs[0].version;
48+
49+
// Create a real temporary directory to add as a workspace folder
50+
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ql-mcp-test-'));
51+
52+
// Listen for the onDidChangeMcpServerDefinitions event.
53+
// Mocha's test timeout (60 s) handles the failure case.
54+
const changePromise = new Promise<void>((resolve) => {
55+
const disposable = provider.onDidChangeMcpServerDefinitions(() => {
56+
disposable.dispose();
57+
resolve();
58+
});
59+
});
60+
61+
// Add the temporary folder to the workspace
62+
const folders = vscode.workspace.workspaceFolders ?? [];
63+
const added = vscode.workspace.updateWorkspaceFolders(
64+
folders.length,
65+
0,
66+
{ uri: vscode.Uri.file(tempDir) },
67+
);
68+
69+
if (!added) {
70+
// Clean up and skip if updateWorkspaceFolders is not supported in this profile
71+
fs.rmdirSync(tempDir);
72+
this.skip();
73+
return;
74+
}
75+
76+
try {
77+
// Wait for the MCP definition change event
78+
await changePromise;
79+
80+
// Get definitions after the workspace folder change
81+
const afterDefs = await provider.provideMcpServerDefinitions(token);
82+
assert.ok(afterDefs && afterDefs.length >= 1, 'Should still provide definitions after folder change');
83+
const versionAfter = afterDefs[0].version;
84+
85+
// The version MUST be different so VS Code knows to restart the server
86+
assert.notStrictEqual(
87+
versionAfter,
88+
versionBefore,
89+
'MCP server definition version must change after workspace folder update ' +
90+
'to signal VS Code to restart the server instead of only stopping it',
91+
);
92+
} finally {
93+
// Cleanup: remove the added folder and temp directory
94+
const updatedFolders = vscode.workspace.workspaceFolders ?? [];
95+
const idx = updatedFolders.findIndex((f) => f.uri.fsPath === tempDir);
96+
if (idx >= 0) {
97+
vscode.workspace.updateWorkspaceFolders(idx, 1);
98+
}
99+
try {
100+
fs.rmdirSync(tempDir);
101+
} catch {
102+
// Best-effort cleanup
103+
}
104+
}
105+
});
106+
107+
test('MCP definition version should change after workspace folder is removed', async function () {
108+
const provider = api.mcpProvider;
109+
const folders = vscode.workspace.workspaceFolders;
110+
if (!provider || !folders || folders.length < 2) {
111+
// Need at least 2 folders to remove one without closing the workspace
112+
this.skip();
113+
return;
114+
}
115+
116+
const token: vscode.CancellationToken = {
117+
isCancellationRequested: false,
118+
onCancellationRequested: () => ({ dispose: () => {} }),
119+
};
120+
121+
// Get initial definitions
122+
const beforeDefs = await provider.provideMcpServerDefinitions(token);
123+
assert.ok(beforeDefs && beforeDefs.length >= 1, 'Should provide at least one definition');
124+
const versionBefore = beforeDefs[0].version;
125+
126+
// Remember the last folder so we can re-add it after removal
127+
const lastFolder = folders[folders.length - 1];
128+
const removedUri = lastFolder.uri;
129+
130+
// Listen for the onDidChangeMcpServerDefinitions event.
131+
// Mocha's test timeout (60 s) handles the failure case.
132+
const changePromise = new Promise<void>((resolve) => {
133+
const disposable = provider.onDidChangeMcpServerDefinitions(() => {
134+
disposable.dispose();
135+
resolve();
136+
});
137+
});
138+
139+
// Remove the last workspace folder
140+
const removed = vscode.workspace.updateWorkspaceFolders(folders.length - 1, 1);
141+
if (!removed) {
142+
this.skip();
143+
return;
144+
}
145+
146+
try {
147+
// Wait for the MCP definition change event
148+
await changePromise;
149+
150+
// Get definitions after the workspace folder change
151+
const afterDefs = await provider.provideMcpServerDefinitions(token);
152+
assert.ok(afterDefs && afterDefs.length >= 1, 'Should still provide definitions after folder removal');
153+
const versionAfter = afterDefs[0].version;
154+
155+
// The version MUST be different so VS Code knows to restart the server
156+
assert.notStrictEqual(
157+
versionAfter,
158+
versionBefore,
159+
'MCP server definition version must change after workspace folder removal ' +
160+
'to signal VS Code to restart the server instead of only stopping it',
161+
);
162+
} finally {
163+
// Cleanup: re-add the removed folder
164+
const currentFolders = vscode.workspace.workspaceFolders ?? [];
165+
vscode.workspace.updateWorkspaceFolders(
166+
currentFolders.length,
167+
0,
168+
{ uri: removedUri },
169+
);
170+
}
171+
});
172+
173+
test('Environment should reflect updated workspace folders after change', async function () {
174+
const envBuilder = api.environmentBuilder;
175+
const folders = vscode.workspace.workspaceFolders;
176+
if (!envBuilder || !folders || folders.length === 0) {
177+
this.skip();
178+
return;
179+
}
180+
181+
// Create a real temporary directory
182+
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ql-mcp-env-test-'));
183+
184+
// Listen for workspace folder changes through the VS Code API
185+
const changePromise = new Promise<void>((resolve) => {
186+
const disposable = vscode.workspace.onDidChangeWorkspaceFolders(() => {
187+
disposable.dispose();
188+
resolve();
189+
});
190+
});
191+
192+
// Add the temporary folder
193+
const added = vscode.workspace.updateWorkspaceFolders(
194+
folders.length,
195+
0,
196+
{ uri: vscode.Uri.file(tempDir) },
197+
);
198+
199+
if (!added) {
200+
fs.rmdirSync(tempDir);
201+
this.skip();
202+
return;
203+
}
204+
205+
try {
206+
await changePromise;
207+
208+
// invalidate() was called by the event handler, so build() returns fresh env
209+
const env = await envBuilder.build();
210+
assert.ok(env.CODEQL_MCP_WORKSPACE_FOLDERS, 'CODEQL_MCP_WORKSPACE_FOLDERS should be set');
211+
assert.ok(
212+
env.CODEQL_MCP_WORKSPACE_FOLDERS.includes(tempDir),
213+
`CODEQL_MCP_WORKSPACE_FOLDERS should include the newly added folder: ${env.CODEQL_MCP_WORKSPACE_FOLDERS}`,
214+
);
215+
} finally {
216+
// Cleanup
217+
const updatedFolders = vscode.workspace.workspaceFolders ?? [];
218+
const idx = updatedFolders.findIndex((f) => f.uri.fsPath === tempDir);
219+
if (idx >= 0) {
220+
vscode.workspace.updateWorkspaceFolders(idx, 1);
221+
}
222+
try {
223+
fs.rmdirSync(tempDir);
224+
} catch {
225+
// Best-effort cleanup
226+
}
227+
}
228+
});
229+
});

0 commit comments

Comments
 (0)