Skip to content

Commit 0bf6946

Browse files
data-douserdata-douser
committed
Server & extension fixes for PR review feedback
1 parent 11ea9ac commit 0bf6946

File tree

12 files changed

+196
-38
lines changed

12 files changed

+196
-38
lines changed

‎extensions/vscode/src/codeql/cli-resolver.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@ export class CliResolver extends DisposableObject {
3434
private cachedVersion: string | undefined;
3535
private resolvePromise: Promise<string | undefined> | null = null;
3636

37+
/**
38+
* Monotonically increasing generation counter, bumped by `invalidateCache()`.
39+
* Used to discard results from in-flight `doResolve()` calls that started
40+
* before the most recent invalidation.
41+
*/
42+
private _generation = 0;
43+
3744
constructor(
3845
private readonly logger: Logger,
3946
private readonly vsCodeCodeqlStoragePath?: string,
@@ -73,12 +80,27 @@ export class CliResolver extends DisposableObject {
7380

7481
/** Internal resolution logic. Called at most once per cache cycle. */
7582
private async doResolve(): Promise<string | undefined> {
83+
const startGeneration = this._generation;
7684
this.logger.debug('Resolving CodeQL CLI path...');
7785

86+
/**
87+
* Check whether the cache was invalidated while an async operation
88+
* was in flight. If so, discard any version that `validateBinary()`
89+
* may have written and bail out immediately.
90+
*/
91+
const isStale = (): boolean => {
92+
if (this._generation !== startGeneration) {
93+
this.cachedVersion = undefined;
94+
return true;
95+
}
96+
return false;
97+
};
98+
7899
// Strategy 1: CODEQL_PATH env var
79100
const envPath = process.env.CODEQL_PATH;
80101
if (envPath) {
81102
const validated = await this.validateBinary(envPath);
103+
if (isStale()) return undefined;
82104
if (validated) {
83105
this.logger.info(`CodeQL CLI found via CODEQL_PATH: ${envPath}`);
84106
this.cachedPath = envPath;
@@ -89,8 +111,10 @@ export class CliResolver extends DisposableObject {
89111

90112
// Strategy 2: which/command -v
91113
const whichPath = await this.resolveFromPath();
114+
if (isStale()) return undefined;
92115
if (whichPath) {
93116
const validated = await this.validateBinary(whichPath);
117+
if (isStale()) return undefined;
94118
if (validated) {
95119
this.logger.info(`CodeQL CLI found on PATH: ${whichPath}`);
96120
this.cachedPath = whichPath;
@@ -101,6 +125,7 @@ export class CliResolver extends DisposableObject {
101125

102126
// Strategy 3: vscode-codeql managed distribution
103127
const distPath = await this.resolveFromVsCodeDistribution();
128+
if (isStale()) return undefined;
104129
if (distPath) {
105130
this.logger.info(`CodeQL CLI found via vscode-codeql distribution: ${distPath}`);
106131
this.cachedPath = distPath;
@@ -110,6 +135,7 @@ export class CliResolver extends DisposableObject {
110135
// Strategy 4: known filesystem locations
111136
for (const location of KNOWN_LOCATIONS) {
112137
const validated = await this.validateBinary(location);
138+
if (isStale()) return undefined;
113139
if (validated) {
114140
this.logger.info(`CodeQL CLI found at known location: ${location}`);
115141
this.cachedPath = location;
@@ -127,6 +153,7 @@ export class CliResolver extends DisposableObject {
127153
this.cachedPath = null;
128154
this.cachedVersion = undefined;
129155
this.resolvePromise = null;
156+
this._generation++;
130157
}
131158

132159
/** Check if a path exists and responds to `--version`. */

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,14 @@ export class McpProvider
4949
this.push(this._onDidChange);
5050
}
5151

52+
override dispose(): void {
53+
if (this._debounceTimer !== undefined) {
54+
globalThis.clearTimeout(this._debounceTimer);
55+
this._debounceTimer = undefined;
56+
}
57+
super.dispose();
58+
}
59+
5260
/**
5361
* Soft notification: tell VS Code that definitions may have changed.
5462
*

‎extensions/vscode/src/server/pack-installer.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ export class PackInstaller extends DisposableObject {
167167
if (downloadEnabled && actualCliVersion && actualCliVersion !== targetCliVersion) {
168168
this.logger.info(
169169
`CodeQL CLI version ${actualCliVersion} differs from VSIX target ${targetCliVersion}. ` +
170-
'Attempting to install compatible tool query packs...',
170+
'Attempting to download compatible tool query packs...',
171171
);
172172
const downloaded = await this.downloadPacksForCliVersion(
173173
codeqlPath, actualCliVersion, languages,

‎extensions/vscode/test/codeql/cli-resolver.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,44 @@ describe('CliResolver', () => {
152152
process.env.CODEQL_PATH = originalEnv;
153153
});
154154

155+
it('should discard in-flight resolution when invalidateCache is called during resolve', async () => {
156+
const originalEnv = process.env.CODEQL_PATH;
157+
process.env.CODEQL_PATH = '/stale/codeql';
158+
159+
// Make access slow — resolve will be in-flight when we invalidate
160+
let accessResolve: (() => void) | undefined;
161+
vi.mocked(access).mockImplementation(() => {
162+
return new Promise<void>(resolve => {
163+
accessResolve = resolve;
164+
});
165+
});
166+
167+
vi.mocked(execFile).mockImplementation(
168+
(_cmd: any, _args: any, callback: any) => {
169+
callback(null, 'CodeQL CLI 2.19.0\n', '');
170+
return {} as any;
171+
},
172+
);
173+
174+
// Start resolution (will block on access)
175+
const resolvePromise = resolver.resolve();
176+
177+
// Invalidate while resolution is in-flight
178+
resolver.invalidateCache();
179+
180+
// Let the in-flight access complete
181+
accessResolve!();
182+
183+
// The stale result should be discarded
184+
const result = await resolvePromise;
185+
expect(result).toBeUndefined();
186+
187+
// After invalidation, cachedPath should still be null (not set by stale resolve)
188+
expect(resolver.getCliVersion()).toBeUndefined();
189+
190+
process.env.CODEQL_PATH = originalEnv;
191+
});
192+
155193
it('should return undefined when no CLI is found', async () => {
156194
const originalEnv = process.env.CODEQL_PATH;
157195
delete process.env.CODEQL_PATH;

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,4 +326,44 @@ describe('McpProvider', () => {
326326
expect(listener).toHaveBeenCalledTimes(1);
327327
});
328328
});
329+
330+
describe('dispose', () => {
331+
beforeEach(() => {
332+
vi.useFakeTimers();
333+
});
334+
335+
afterEach(() => {
336+
vi.useRealTimers();
337+
});
338+
339+
it('should clear pending debounce timer on dispose', () => {
340+
const clearTimeoutSpy = vi.spyOn(globalThis, 'clearTimeout');
341+
342+
// Trigger a debounced fireDidChange so the timer is pending
343+
provider.fireDidChange();
344+
345+
// Dispose the provider — should clear the pending timer
346+
provider.dispose();
347+
348+
expect(clearTimeoutSpy).toHaveBeenCalled();
349+
clearTimeoutSpy.mockRestore();
350+
});
351+
352+
it('should not fire debounced event after dispose', () => {
353+
const listener = vi.fn();
354+
provider.onDidChangeMcpServerDefinitions(listener);
355+
356+
// Schedule a debounced fire
357+
provider.fireDidChange();
358+
359+
// Dispose before the timer fires
360+
provider.dispose();
361+
362+
// Advance time past the debounce delay
363+
vi.advanceTimersByTime(2_000);
364+
365+
// The listener should NOT have been called
366+
expect(listener).not.toHaveBeenCalled();
367+
});
368+
});
329369
});

‎server/dist/codeql-development-mcp-server.js‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -198107,6 +198107,20 @@ import { access as access2 } from "fs/promises";
198107198107
import { basename as basename9, isAbsolute as isAbsolute7, normalize, relative as relative2, resolve as resolve13, sep as sep3 } from "path";
198108198108
import { fileURLToPath as fileURLToPath3 } from "url";
198109198109

198110+
// src/prompts/constants.ts
198111+
var SUPPORTED_LANGUAGES = [
198112+
"actions",
198113+
"cpp",
198114+
"csharp",
198115+
"go",
198116+
"java",
198117+
"javascript",
198118+
"python",
198119+
"ruby",
198120+
"rust",
198121+
"swift"
198122+
];
198123+
198110198124
// src/prompts/prompt-completions.ts
198111198125
import { readdir, readFile as readFile4 } from "fs/promises";
198112198126
import { homedir as homedir2 } from "os";
@@ -198451,18 +198465,6 @@ function processPromptTemplate(template, variables) {
198451198465
// src/prompts/workflow-prompts.ts
198452198466
init_package_paths();
198453198467
init_logger();
198454-
var SUPPORTED_LANGUAGES = [
198455-
"actions",
198456-
"cpp",
198457-
"csharp",
198458-
"go",
198459-
"java",
198460-
"javascript",
198461-
"python",
198462-
"ruby",
198463-
"rust",
198464-
"swift"
198465-
];
198466198468
function markdownInlineCode(value) {
198467198469
const normalized = value.replace(/\r\n|\r|\n/g, " ");
198468198470
let maxRun = 0;
@@ -198796,7 +198798,7 @@ ${content}`
198796198798
}
198797198799
}
198798198800
if (!effectiveLanguage) {
198799-
warnings.push("\u26A0 **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with a `codeql/<lang>-all` dependency.");
198801+
warnings.push("\u26A0 **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with either a `codeql/<lang>-all` or `codeql/<lang>-queries` dependency.");
198800198802
}
198801198803
const derivedName = workshopName || basename9(resolvedQueryPath).replace(/\.(ql|qlref)$/, "").toLowerCase().replace(/[^a-z0-9]+/g, "-") || "codeql-workshop";
198802198804
const contextSection = buildWorkshopContext(
@@ -199025,7 +199027,7 @@ ${content}`
199025199027
}
199026199028
}
199027199029
if (!effectiveLanguage) {
199028-
warnings.push("\u26A0 **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with a `codeql/<lang>-all` dependency.");
199030+
warnings.push("\u26A0 **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with either a `codeql/<lang>-all` or `codeql/<lang>-queries` dependency.");
199029199031
}
199030199032
let resolvedDatabasePath = databasePath;
199031199033
if (databasePath) {
@@ -199081,7 +199083,7 @@ ${content}`
199081199083
}
199082199084
}
199083199085
if (!effectiveLanguage) {
199084-
warnings.push("\u26A0 **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with a `codeql/<lang>-all` dependency.");
199086+
warnings.push("\u26A0 **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with either a `codeql/<lang>-all` or `codeql/<lang>-queries` dependency.");
199085199087
}
199086199088
const contextSection = `## Query to Document
199087199089

@@ -199176,7 +199178,7 @@ ${workspaceUri ? `- **Workspace URI**: ${workspaceUri}
199176199178
}
199177199179
}
199178199180
if (!effectiveLanguage) {
199179-
warnings.push("\u26A0 **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with a `codeql/<lang>-all` dependency.");
199181+
warnings.push("\u26A0 **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with either a `codeql/<lang>-all` or `codeql/<lang>-queries` dependency.");
199180199182
}
199181199183
let resolvedWorkspaceUri = workspaceUri;
199182199184
if (workspaceUri) {

‎server/dist/codeql-development-mcp-server.js.map‎

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎server/src/prompts/constants.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* Shared constants for MCP server workflow prompts and completions.
3+
*
4+
* This module exists to break the circular dependency between
5+
* `workflow-prompts.ts` and `prompt-completions.ts` — both need
6+
* access to the supported-language list, so it lives here instead
7+
* of in either consumer.
8+
*/
9+
10+
/** Supported CodeQL languages for tools queries */
11+
export const SUPPORTED_LANGUAGES = [
12+
'actions',
13+
'cpp',
14+
'csharp',
15+
'go',
16+
'java',
17+
'javascript',
18+
'python',
19+
'ruby',
20+
'rust',
21+
'swift',
22+
] as const;

‎server/src/prompts/prompt-completions.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import { z } from 'zod';
1919
import { getDatabaseBaseDirs } from '../lib/discovery-config';
2020
import { logger } from '../utils/logger';
2121
import { getUserWorkspaceDir } from '../utils/package-paths';
22-
import { SUPPORTED_LANGUAGES } from './workflow-prompts';
22+
import { SUPPORTED_LANGUAGES } from './constants';
2323

2424
// ────────────────────────────────────────────────────────────────────────────
2525
// Completion callbacks

‎server/src/prompts/workflow-prompts.ts‎

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,14 @@ import { z } from 'zod';
1010
import { access } from 'fs/promises';
1111
import { basename, isAbsolute, normalize, relative, resolve, sep } from 'path';
1212
import { fileURLToPath } from 'url';
13+
import { SUPPORTED_LANGUAGES } from './constants';
1314
import { addCompletions, resolveLanguageFromPack } from './prompt-completions';
1415
import { loadPromptTemplate, processPromptTemplate } from './prompt-loader';
1516
import { getUserWorkspaceDir } from '../utils/package-paths';
1617
import { logger } from '../utils/logger';
1718

18-
/** Supported CodeQL languages for tools queries */
19-
export const SUPPORTED_LANGUAGES = [
20-
'actions',
21-
'cpp',
22-
'csharp',
23-
'go',
24-
'java',
25-
'javascript',
26-
'python',
27-
'ruby',
28-
'rust',
29-
'swift'
30-
] as const;
19+
// Re-export for backward compatibility with existing consumers and tests.
20+
export { SUPPORTED_LANGUAGES } from './constants';
3121

3222
// ────────────────────────────────────────────────────────────────────────────
3323
// File-path resolution for prompt parameters
@@ -761,7 +751,7 @@ export function registerWorkflowPrompts(server: McpServer): void {
761751
}
762752
}
763753
if (!effectiveLanguage) {
764-
warnings.push('⚠ **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with a `codeql/<lang>-all` dependency.');
754+
warnings.push('⚠ **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with either a `codeql/<lang>-all` or `codeql/<lang>-queries` dependency.');
765755
}
766756

767757
const derivedName =
@@ -1030,7 +1020,7 @@ export function registerWorkflowPrompts(server: McpServer): void {
10301020
}
10311021
}
10321022
if (!effectiveLanguage) {
1033-
warnings.push('⚠ **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with a `codeql/<lang>-all` dependency.');
1023+
warnings.push('⚠ **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with either a `codeql/<lang>-all` or `codeql/<lang>-queries` dependency.');
10341024
}
10351025

10361026
let resolvedDatabasePath = databasePath;
@@ -1094,7 +1084,7 @@ export function registerWorkflowPrompts(server: McpServer): void {
10941084
}
10951085
}
10961086
if (!effectiveLanguage) {
1097-
warnings.push('⚠ **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with a `codeql/<lang>-all` dependency.');
1087+
warnings.push('⚠ **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with either a `codeql/<lang>-all` or `codeql/<lang>-queries` dependency.');
10981088
}
10991089

11001090
const contextSection = `## Query to Document
@@ -1211,7 +1201,7 @@ ${workspaceUri ? `- **Workspace URI**: ${workspaceUri}
12111201
}
12121202
}
12131203
if (!effectiveLanguage) {
1214-
warnings.push('⚠ **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with a `codeql/<lang>-all` dependency.');
1204+
warnings.push('⚠ **Language could not be auto-derived.** Please provide the `language` parameter or ensure the query is inside a CodeQL pack with either a `codeql/<lang>-all` or `codeql/<lang>-queries` dependency.');
12151205
}
12161206

12171207
let resolvedWorkspaceUri = workspaceUri;

0 commit comments

Comments
 (0)