Address PR review comments

Author: data-douser

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

@@ -1,7 +1,7 @@
 import { execFile } from 'child_process';
 import { access, readdir, readFile } from 'fs/promises';
 import { constants } from 'fs';
-import { dirname, join } from 'path';
+import { join } from 'path';
 import { DisposableObject } from '../common/disposable';
 import type { Logger } from '../common/logger';
 
@@ -16,6 +16,14 @@ const KNOWN_LOCATIONS = [
   `${process.env.HOME}/codeql/codeql`,
 ];
 
+/**
+ * Known directory names for the vscode-codeql extension's global storage.
+ * VS Code lowercases the extension ID on some platforms/versions, but the
+ * publisher casing (`GitHub`) may also appear on disk. Both are checked to
+ * ensure discovery works on case-sensitive filesystems.
+ */
+const VSCODE_CODEQL_STORAGE_DIR_NAMES = ['github.vscode-codeql', 'GitHub.vscode-codeql'];
+
 /**
  * Resolves the absolute path to the CodeQL CLI binary.
  *
@@ -34,7 +42,7 @@ export class CliResolver extends DisposableObject {
 
   constructor(
     private readonly logger: Logger,
-    private readonly vsCodeCodeqlStoragePath?: string,
+    private readonly vsCodeGlobalStorageRoot?: string,
   ) {
     super();
   }
@@ -120,20 +128,15 @@ export class CliResolver extends DisposableObject {
    * fall back to scanning for the highest-numbered `distribution*` folder.
    */
   private async resolveFromVsCodeDistribution(): Promise<string | undefined> {
-    if (!this.vsCodeCodeqlStoragePath) return undefined;
+    if (!this.vsCodeGlobalStorageRoot) return undefined;
 
-    const parent = dirname(this.vsCodeCodeqlStoragePath);
     // VS Code stores the extension directory as either 'GitHub.vscode-codeql'
     // (original publisher casing) or 'github.vscode-codeql' (lowercased by VS Code
     // on some platforms/versions). Probe both to ensure discovery works on
     // case-sensitive filesystems.
-    const candidatePaths = [
-      ...new Set([
-        this.vsCodeCodeqlStoragePath,
-        join(parent, 'github.vscode-codeql'),
-        join(parent, 'GitHub.vscode-codeql'),
-      ]),
-    ];
+    const candidatePaths = VSCODE_CODEQL_STORAGE_DIR_NAMES.map(dirName =>
+      join(this.vsCodeGlobalStorageRoot!, dirName),
+    );
 
     for (const storagePath of candidatePaths) {
       try {
@@ -185,10 +188,10 @@ export class CliResolver extends DisposableObject {
       const entries = await readdir(storagePath, { withFileTypes: true });
 
       const distDirs = entries
-        .filter(e => e.isDirectory() && /^distribution\d*$/.test(e.name))
+        .filter(e => e.isDirectory() && /^distribution\d+$/.test(e.name))
         .map(e => ({
           name: e.name,
-          num: parseInt(e.name.replace('distribution', '') || '0', 10),
+          num: parseInt(e.name.replace('distribution', ''), 10),
         }))
         .sort((a, b) => b.num - a.num);
 

extensions/vscode/src/extension.ts

@@ -25,7 +25,7 @@ export async function activate(
 
   // --- Core components ---
   const storagePaths = new StoragePaths(context);
-  const cliResolver = new CliResolver(logger, storagePaths.getCodeqlGlobalStoragePath());
+  const cliResolver = new CliResolver(logger, storagePaths.getGlobalStorageRoot());
   const serverManager = new ServerManager(context, logger);
   const packInstaller = new PackInstaller(cliResolver, serverManager, logger);
   const envBuilder = new EnvironmentBuilder(

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

@@ -168,6 +168,7 @@ describe('CliResolver', () => {
   });
 
   describe('vscode-codeql distribution discovery', () => {
+    const globalStorageRoot = '/mock/globalStorage';
     const storagePath = '/mock/globalStorage/github.vscode-codeql';
     const binaryName = process.platform === 'win32' ? 'codeql.exe' : 'codeql';
 
@@ -203,7 +204,7 @@ describe('CliResolver', () => {
     });
 
     it('should resolve from distribution.json hint', async () => {
-      resolver = new CliResolver(logger, storagePath);
+      resolver = new CliResolver(logger, globalStorageRoot);
 
       // distribution.json exists with folderIndex=3
       vi.mocked(readFile).mockResolvedValueOnce(
@@ -225,7 +226,7 @@ describe('CliResolver', () => {
     });
 
     it('should fall back to directory scan when distribution.json is missing', async () => {
-      resolver = new CliResolver(logger, storagePath);
+      resolver = new CliResolver(logger, globalStorageRoot);
 
       // distribution.json read fails
       vi.mocked(readFile).mockRejectedValueOnce(new Error('ENOENT'));
@@ -251,7 +252,7 @@ describe('CliResolver', () => {
     });
 
     it('should scan directories sorted by descending number', async () => {
-      resolver = new CliResolver(logger, storagePath);
+      resolver = new CliResolver(logger, globalStorageRoot);
 
       vi.mocked(readFile).mockRejectedValueOnce(new Error('ENOENT'));
 
@@ -283,7 +284,7 @@ describe('CliResolver', () => {
     });
 
     it('should skip distribution directories without a valid binary', async () => {
-      resolver = new CliResolver(logger, storagePath);
+      resolver = new CliResolver(logger, globalStorageRoot);
 
       vi.mocked(readFile).mockRejectedValueOnce(new Error('ENOENT'));
 
@@ -305,7 +306,7 @@ describe('CliResolver', () => {
     });
 
     it('should handle distribution.json with invalid JSON gracefully', async () => {
-      resolver = new CliResolver(logger, storagePath);
+      resolver = new CliResolver(logger, globalStorageRoot);
 
       // Return non-JSON content
       vi.mocked(readFile).mockResolvedValueOnce('not-valid-json');
@@ -325,11 +326,12 @@ describe('CliResolver', () => {
     });
 
     it('should discover CLI from alternate casing of storage path', async () => {
-      // Simulate the caller providing 'GitHub.vscode-codeql' (StoragePaths publisher casing)
-      // but the actual directory on disk is 'github.vscode-codeql' (lowercased by VS Code).
+      // The resolver receives the globalStorageRoot and probes both casings:
+      //   github.vscode-codeql (lowercase) and GitHub.vscode-codeql (publisher casing).
+      // Simulate only the lowercase variant existing on disk.
       const lowercaseExtStorage = '/mock/globalStorage/github.vscode-codeql';
       const publisherCaseExtStorage = '/mock/globalStorage/GitHub.vscode-codeql';
-      resolver = new CliResolver(logger, publisherCaseExtStorage);
+      resolver = new CliResolver(logger, globalStorageRoot);
 
       // distribution.json at the publisher-cased path throws; the lowercase path returns valid JSON
       vi.mocked(readFile).mockImplementation((path: any) => {
@@ -350,7 +352,7 @@ describe('CliResolver', () => {
     });
 
     it('should handle distribution.json without folderIndex property', async () => {
-      resolver = new CliResolver(logger, storagePath);
+      resolver = new CliResolver(logger, globalStorageRoot);
 
       vi.mocked(readFile).mockResolvedValueOnce(
         JSON.stringify({ release: { name: 'v2.24.2' } }),

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

@@ -38175,7 +38175,7 @@ __export(cli_executor_exports, {
   validateCommandExists: () => validateCommandExists
 });
 import { execFile } from "child_process";
-import { existsSync as existsSync2, readdirSync, readFileSync as readFileSync2 } from "fs";
+import { existsSync as existsSync2, readdirSync, readFileSync as readFileSync2, statSync } from "fs";
 import { basename, delimiter as delimiter4, dirname as dirname2, isAbsolute as isAbsolute2, join as join4 } from "path";
 import { homedir } from "os";
 import { promisify } from "util";
@@ -38223,6 +38223,13 @@ function discoverVsCodeCodeQLDistribution(candidateStorageRoots) {
   }
   return void 0;
 }
+function isRegularFile(filePath) {
+  try {
+    return statSync(filePath).isFile();
+  } catch {
+    return false;
+  }
+}
 function discoverFromDistributionJson(codeqlStorage) {
   try {
     const jsonPath = join4(codeqlStorage, "distribution.json");
@@ -38236,7 +38243,7 @@ function discoverFromDistributionJson(codeqlStorage) {
       "codeql",
       CODEQL_BINARY_NAME
     );
-    if (existsSync2(binaryPath)) {
+    if (isRegularFile(binaryPath)) {
       logger.debug(`Discovered CLI via distribution.json (folderIndex=${data.folderIndex})`);
       return binaryPath;
     }
@@ -38247,9 +38254,9 @@ function discoverFromDistributionJson(codeqlStorage) {
 function discoverFromDistributionScan(codeqlStorage) {
   try {
     const entries = readdirSync(codeqlStorage, { withFileTypes: true });
-    const distDirs = entries.filter((e) => e.isDirectory() && /^distribution\d*$/.test(e.name)).map((e) => ({
+    const distDirs = entries.filter((e) => e.isDirectory() && /^distribution\d+$/.test(e.name)).map((e) => ({
       name: e.name,
-      num: parseInt(e.name.replace("distribution", "") || "0", 10)
+      num: parseInt(e.name.replace("distribution", ""), 10)
     })).sort((a, b) => b.num - a.num);
     for (const dir of distDirs) {
       const binaryPath = join4(
@@ -38258,7 +38265,7 @@ function discoverFromDistributionScan(codeqlStorage) {
         "codeql",
         CODEQL_BINARY_NAME
       );
-      if (existsSync2(binaryPath)) {
+      if (isRegularFile(binaryPath)) {
         logger.debug(`Discovered CLI via distribution scan: ${dir.name}`);
         return binaryPath;
       }
@@ -38267,13 +38274,13 @@ function discoverFromDistributionScan(codeqlStorage) {
   }
   return void 0;
 }
-function resolveCodeQLBinary() {
+function resolveCodeQLBinary(candidateStorageRoots) {
   if (resolvedBinaryResult !== void 0) {
     return resolvedBinaryResult;
   }
   const envPath = process.env.CODEQL_PATH;
   if (!envPath) {
-    const discovered = discoverVsCodeCodeQLDistribution();
+    const discovered = discoverVsCodeCodeQLDistribution(candidateStorageRoots);
     if (discovered) {
       resolvedCodeQLDir = dirname2(discovered);
       resolvedBinaryResult = "codeql";
@@ -60648,7 +60655,7 @@ var codeqlGenerateQueryHelpTool = {
 };
 
 // src/tools/codeql/list-databases.ts
-import { existsSync as existsSync6, readdirSync as readdirSync3, readFileSync as readFileSync5, statSync as statSync2 } from "fs";
+import { existsSync as existsSync6, readdirSync as readdirSync3, readFileSync as readFileSync5, statSync as statSync3 } from "fs";
 import { join as join8 } from "path";
 
 // src/lib/discovery-config.ts
@@ -60712,7 +60719,7 @@ async function discoverDatabases(baseDirs, language) {
     for (const entry of entries) {
       const entryPath = join8(baseDir, entry);
       try {
-        if (!statSync2(entryPath).isDirectory()) {
+        if (!statSync3(entryPath).isDirectory()) {
           continue;
         }
       } catch {
@@ -60801,7 +60808,7 @@ function registerListDatabasesTool(server) {
 }
 
 // src/tools/codeql/list-mrva-run-results.ts
-import { existsSync as existsSync7, readdirSync as readdirSync4, readFileSync as readFileSync6, statSync as statSync3 } from "fs";
+import { existsSync as existsSync7, readdirSync as readdirSync4, readFileSync as readFileSync6, statSync as statSync4 } from "fs";
 import { join as join9 } from "path";
 init_logger();
 var NUMERIC_DIR_PATTERN = /^\d+$/;
@@ -60821,7 +60828,7 @@ async function discoverMrvaRunResults(resultsDirs, runId) {
     for (const entry of entries) {
       const entryPath = join9(dir, entry);
       try {
-        if (!statSync3(entryPath).isDirectory()) {
+        if (!statSync4(entryPath).isDirectory()) {
           continue;
         }
       } catch {
@@ -60866,7 +60873,7 @@ function discoverRepoResults(runPath) {
     }
     const ownerPath = join9(runPath, ownerEntry);
     try {
-      if (!statSync3(ownerPath).isDirectory()) {
+      if (!statSync4(ownerPath).isDirectory()) {
         continue;
       }
     } catch {
@@ -60881,7 +60888,7 @@ function discoverRepoResults(runPath) {
     for (const repoEntry of repoEntries) {
       const repoPath = join9(ownerPath, repoEntry);
       try {
-        if (!statSync3(repoPath).isDirectory()) {
+        if (!statSync4(repoPath).isDirectory()) {
           continue;
         }
       } catch {
@@ -60988,7 +60995,7 @@ function registerListMrvaRunResultsTool(server) {
 }
 
 // src/tools/codeql/list-query-run-results.ts
-import { existsSync as existsSync8, readdirSync as readdirSync5, readFileSync as readFileSync7, statSync as statSync4 } from "fs";
+import { existsSync as existsSync8, readdirSync as readdirSync5, readFileSync as readFileSync7, statSync as statSync5 } from "fs";
 import { join as join10 } from "path";
 init_logger();
 var QUERY_RUN_DIR_PATTERN = /^(.+\.ql)-(.+)$/;
@@ -61037,7 +61044,7 @@ async function discoverQueryRunResults(resultsDirs, filter) {
     for (const entry of entries) {
       const entryPath = join10(dir, entry);
       try {
-        if (!statSync4(entryPath).isDirectory()) {
+        if (!statSync5(entryPath).isDirectory()) {
           continue;
         }
       } catch {
@@ -62091,7 +62098,7 @@ function registerQuickEvaluateTool(server) {
 
 // src/tools/codeql/read-database-source.ts
 var import_adm_zip = __toESM(require_adm_zip(), 1);
-import { existsSync as existsSync11, readdirSync as readdirSync6, readFileSync as readFileSync10, statSync as statSync5 } from "fs";
+import { existsSync as existsSync11, readdirSync as readdirSync6, readFileSync as readFileSync10, statSync as statSync6 } from "fs";
 import { join as join14, resolve as resolve7 } from "path";
 import { fileURLToPath as fileURLToPath2 } from "url";
 init_logger();
@@ -62110,7 +62117,7 @@ function toFilesystemPath(uri) {
 function* walkDirectory(dir, base = dir) {
   for (const entry of readdirSync6(dir)) {
     const fullPath = join14(dir, entry);
-    if (statSync5(fullPath).isDirectory()) {
+    if (statSync6(fullPath).isDirectory()) {
       yield* walkDirectory(fullPath, base);
     } else {
       yield fullPath.slice(base.length).replace(/\\/g, "/").replace(/^\//, "");

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

@@ -3,7 +3,7 @@
  */
 
 import { execFile } from 'child_process';
-import { existsSync, readdirSync, readFileSync } from 'fs';
+import { existsSync, readdirSync, readFileSync, statSync } from 'fs';
 import { basename, delimiter, dirname, isAbsolute, join } from 'path';
 import { homedir } from 'os';
 import { promisify } from 'util';
@@ -200,6 +200,20 @@ export function discoverVsCodeCodeQLDistribution(candidateStorageRoots?: string[
   return undefined;
 }
 
+/**
+ * Check whether `filePath` refers to an existing regular file.
+ *
+ * Unlike `existsSync`, this rejects directories and other non-file entries
+ * so that a directory named `codeql` is not mistakenly treated as a binary.
+ */
+function isRegularFile(filePath: string): boolean {
+  try {
+    return statSync(filePath).isFile();
+  } catch {
+    return false;
+  }
+}
+
 /**
  * Read `distribution.json` and validate the binary at the indicated path.
  */
@@ -218,7 +232,7 @@ function discoverFromDistributionJson(codeqlStorage: string): string | undefined
       'codeql',
       CODEQL_BINARY_NAME,
     );
-    if (existsSync(binaryPath)) {
+    if (isRegularFile(binaryPath)) {
       logger.debug(`Discovered CLI via distribution.json (folderIndex=${data.folderIndex})`);
       return binaryPath;
     }
@@ -237,10 +251,10 @@ function discoverFromDistributionScan(codeqlStorage: string): string | undefined
     const entries = readdirSync(codeqlStorage, { withFileTypes: true });
 
     const distDirs = entries
-      .filter(e => e.isDirectory() && /^distribution\d*$/.test(e.name))
+      .filter(e => e.isDirectory() && /^distribution\d+$/.test(e.name))
       .map(e => ({
         name: e.name,
-        num: parseInt(e.name.replace('distribution', '') || '0', 10),
+        num: parseInt(e.name.replace('distribution', ''), 10),
       }))
       .sort((a, b) => b.num - a.num);
 
@@ -251,7 +265,7 @@ function discoverFromDistributionScan(codeqlStorage: string): string | undefined
         'codeql',
         CODEQL_BINARY_NAME,
       );
-      if (existsSync(binaryPath)) {
+      if (isRegularFile(binaryPath)) {
         logger.debug(`Discovered CLI via distribution scan: ${dir.name}`);
         return binaryPath;
       }
@@ -276,7 +290,7 @@ function discoverFromDistributionScan(codeqlStorage: string): string | undefined
  * The resolved value is cached for the lifetime of the process. Call this once
  * at startup; subsequent calls are a no-op and return the cached value.
  */
-export function resolveCodeQLBinary(): string {
+export function resolveCodeQLBinary(candidateStorageRoots?: string[]): string {
   // Short-circuit if already resolved
   if (resolvedBinaryResult !== undefined) {
     return resolvedBinaryResult;
@@ -286,7 +300,7 @@ export function resolveCodeQLBinary(): string {
 
   if (!envPath) {
     // Try auto-discovery of the vscode-codeql managed distribution
-    const discovered = discoverVsCodeCodeQLDistribution();
+    const discovered = discoverVsCodeCodeQLDistribution(candidateStorageRoots);
     if (discovered) {
       resolvedCodeQLDir = dirname(discovered);
       resolvedBinaryResult = 'codeql';