Address review feedback for PR #244

Author: data-douser

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

@@ -247,6 +247,7 @@ export class PackInstaller extends DisposableObject {
   ): Promise<void> {
     const qlRoot = this.getQlpackRoot();
     let successCount = 0;
+    let skippedCount = 0;
 
     for (const lang of languages) {
       const packDir = join(qlRoot, 'ql', lang, 'tools', 'src');
@@ -257,6 +258,7 @@ export class PackInstaller extends DisposableObject {
         await access(packDir, constants.R_OK);
       } catch {
         this.logger.debug(`Pack directory not found, skipping: ${packDir}`);
+        skippedCount++;
         continue;
       }
 
@@ -271,8 +273,10 @@ export class PackInstaller extends DisposableObject {
         );
       }
     }
+    const attemptCount = languages.length - skippedCount;
+    const skippedSuffix = skippedCount > 0 ? `, ${skippedCount} skipped` : '';
     this.logger.info(
-      `Bundled pack install complete: ${successCount}/${languages.length} languages succeeded.`,
+      `Bundled pack install complete: ${successCount}/${attemptCount} languages succeeded${skippedSuffix}.`,
     );
   }
 

extensions/vscode/test/server/pack-installer.test.ts

@@ -456,6 +456,48 @@ describe('PackInstaller', () => {
       );
     });
 
+    it('should report attemptCount (not languages.length) when packs are skipped', async () => {
+      cliResolver.getCliVersion.mockReturnValue('2.25.1');
+      serverManager.getExtensionVersion.mockReturnValue('2.25.1');
+
+      // Only javascript pack dir exists; python and go are missing
+      vi.mocked(access).mockImplementation((path: any) => {
+        if (String(path).includes('/javascript/')) return Promise.resolve(undefined as any);
+        return Promise.reject(new Error('ENOENT'));
+      });
+
+      await installer.installAll({ languages: ['javascript', 'python', 'go'] });
+
+      // Summary should say 1/1 (succeeded/attempted), not 1/3
+      const summaryCall = logger.info.mock.calls.find(
+        (call: any[]) => typeof call[0] === 'string' && call[0].includes('Bundled pack install complete'),
+      );
+      expect(summaryCall).toBeDefined();
+      const summaryMsg = summaryCall![0] as string;
+      expect(summaryMsg).toContain('1/1');
+      expect(summaryMsg).not.toContain('1/3');
+    });
+
+    it('should mention skipped count in summary when packs are missing', async () => {
+      cliResolver.getCliVersion.mockReturnValue('2.25.1');
+      serverManager.getExtensionVersion.mockReturnValue('2.25.1');
+
+      // Only javascript pack dir exists; python and go are missing
+      vi.mocked(access).mockImplementation((path: any) => {
+        if (String(path).includes('/javascript/')) return Promise.resolve(undefined as any);
+        return Promise.reject(new Error('ENOENT'));
+      });
+
+      await installer.installAll({ languages: ['javascript', 'python', 'go'] });
+
+      const summaryCall = logger.info.mock.calls.find(
+        (call: any[]) => typeof call[0] === 'string' && call[0].includes('Bundled pack install complete'),
+      );
+      expect(summaryCall).toBeDefined();
+      const summaryMsg = summaryCall![0] as string;
+      expect(summaryMsg).toMatch(/2 skipped/);
+    });
+
     it('should use download terminology in pack download logs', async () => {
       cliResolver.getCliVersion.mockReturnValue('2.24.1');
       serverManager.getExtensionVersion.mockReturnValue('2.25.1-next.1');

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

@@ -196876,10 +196876,13 @@ var MAX_FILE_SIZE_BYTES = 5 * 1024 * 1024;
 var MAX_FILES_TRAVERSED = 1e4;
 var MAX_CONTEXT_LINES = 50;
 var MAX_MAX_RESULTS = 1e4;
-var SKIP_DIRS2 = getScanExcludeDirs();
+function getSkipDirs() {
+  return getScanExcludeDirs();
+}
 function collectFiles(paths, extensions, fileCount) {
   const files = [];
   const visitedDirs = /* @__PURE__ */ new Set();
+  const skipDirs = getSkipDirs();
   function walk(p) {
     if (fileCount.value >= MAX_FILES_TRAVERSED) return;
     let stat;
@@ -196895,7 +196898,7 @@ function collectFiles(paths, extensions, fileCount) {
       }
       fileCount.value++;
     } else if (stat.isDirectory()) {
-      if (SKIP_DIRS2.has(basename9(p))) return;
+      if (skipDirs.has(basename9(p))) return;
       let realPath;
       try {
         realPath = realpathSync2(p);
@@ -198322,7 +198325,9 @@ init_package_paths();
 var MAX_FILE_COMPLETIONS = 50;
 var MAX_SCAN_DEPTH = 8;
 var CACHE_TTL_MS = 5e3;
-var SKIP_DIRS3 = getScanExcludeDirs();
+function getSkipDirs2() {
+  return getScanExcludeDirs();
+}
 var scanCache = /* @__PURE__ */ new Map();
 function getCachedResults(cacheKey2) {
   const entry = scanCache.get(cacheKey2);
@@ -198338,8 +198343,9 @@ function completeLanguage(value) {
   const lower = (value || "").toLowerCase();
   return [...SUPPORTED_LANGUAGES].filter((lang) => lang.startsWith(lower));
 }
-async function findFilesByExtension(dir, baseDir, extensions, maxDepth, results) {
+async function findFilesByExtension(dir, baseDir, extensions, maxDepth, results, skipDirs) {
   if (maxDepth <= 0 || results.length >= MAX_FILE_COMPLETIONS) return;
+  const excluded = skipDirs ?? getSkipDirs2();
   let entries;
   try {
     entries = await readdir(dir, { withFileTypes: true });
@@ -198350,10 +198356,10 @@ async function findFilesByExtension(dir, baseDir, extensions, maxDepth, results)
     if (results.length >= MAX_FILE_COMPLETIONS) break;
     const fullPath = join22(dir, entry.name);
     if (entry.isDirectory()) {
-      if (SKIP_DIRS3.has(entry.name)) {
+      if (excluded.has(entry.name)) {
         continue;
       }
-      await findFilesByExtension(fullPath, baseDir, extensions, maxDepth - 1, results);
+      await findFilesByExtension(fullPath, baseDir, extensions, maxDepth - 1, results, excluded);
     } else if (entry.isFile()) {
       const lower = entry.name.toLowerCase();
       if (extensions.some((ext) => lower.endsWith(ext))) {
@@ -198463,7 +198469,7 @@ async function findDatabaseDirs(dir, _baseDir, maxDepth, results) {
   }
   for (const entry of entries) {
     if (results.length >= MAX_FILE_COMPLETIONS) break;
-    if (entry.isDirectory() && !SKIP_DIRS3.has(entry.name)) {
+    if (entry.isDirectory() && !getSkipDirs2().has(entry.name)) {
       await findDatabaseDirs(join22(dir, entry.name), _baseDir, maxDepth - 1, results);
     }
   }
@@ -198490,7 +198496,7 @@ async function completePackRoot(value) {
       }
       for (const entry of entries) {
         if (results.length >= MAX_FILE_COMPLETIONS) break;
-        if (entry.isDirectory() && !SKIP_DIRS3.has(entry.name)) {
+        if (entry.isDirectory() && !getSkipDirs2().has(entry.name)) {
           await scan(join22(dir, entry.name), depth - 1);
         }
       }

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

@@ -38,9 +38,11 @@ const CACHE_TTL_MS = 5_000;
 /**
  * Directories to skip during recursive workspace scans.
  * Uses the shared, configurable exclusion list from scan-exclude.ts.
- * Computed once per module load; the env var is read at that time.
+ * Resolved at traversal time so that env var changes take effect.
  */
-const SKIP_DIRS: Set<string> = getScanExcludeDirs();
+function getSkipDirs(): Set<string> {
+  return getScanExcludeDirs();
+}
 
 /** Cached scan results keyed by a workspace+type identifier. */
 interface CacheEntry {
@@ -96,9 +98,12 @@ async function findFilesByExtension(
   extensions: string[],
   maxDepth: number,
   results: string[],
+  skipDirs?: Set<string>,
 ): Promise<void> {
   if (maxDepth <= 0 || results.length >= MAX_FILE_COMPLETIONS) return;
 
+  const excluded = skipDirs ?? getSkipDirs();
+
   let entries;
   try {
     entries = await readdir(dir, { withFileTypes: true });
@@ -113,10 +118,10 @@ async function findFilesByExtension(
 
     if (entry.isDirectory()) {
       // Skip common non-CodeQL directories
-      if (SKIP_DIRS.has(entry.name)) {
+      if (excluded.has(entry.name)) {
         continue;
       }
-      await findFilesByExtension(fullPath, baseDir, extensions, maxDepth - 1, results);
+      await findFilesByExtension(fullPath, baseDir, extensions, maxDepth - 1, results, excluded);
     } else if (entry.isFile()) {
       const lower = entry.name.toLowerCase();
       if (extensions.some(ext => lower.endsWith(ext))) {
@@ -284,7 +289,7 @@ async function findDatabaseDirs(
 
   for (const entry of entries) {
     if (results.length >= MAX_FILE_COMPLETIONS) break;
-    if (entry.isDirectory() && !SKIP_DIRS.has(entry.name)) {
+    if (entry.isDirectory() && !getSkipDirs().has(entry.name)) {
       await findDatabaseDirs(join(dir, entry.name), _baseDir, maxDepth - 1, results);
     }
   }
@@ -322,7 +327,7 @@ export async function completePackRoot(value: string): Promise<string[]> {
 
       for (const entry of entries) {
         if (results.length >= MAX_FILE_COMPLETIONS) break;
-        if (entry.isDirectory() && !SKIP_DIRS.has(entry.name)) {
+        if (entry.isDirectory() && !getSkipDirs().has(entry.name)) {
           await scan(join(dir, entry.name), depth - 1);
         }
       }

server/src/prompts/prompt-completions.ts

@@ -34,8 +34,11 @@ const MAX_MAX_RESULTS = 10_000;
 /**
  * Directory names to skip during traversal.
  * Uses the shared, configurable exclusion list from scan-exclude.ts.
+ * Resolved at traversal time so that env var changes take effect.
  */
-const SKIP_DIRS: Set<string> = getScanExcludeDirs();
+function getSkipDirs(): Set<string> {
+  return getScanExcludeDirs();
+}
 
 // ---------------------------------------------------------------------------
 // Types
@@ -73,6 +76,7 @@ function collectFiles(
 ): string[] {
   const files: string[] = [];
   const visitedDirs = new Set<string>();
+  const skipDirs = getSkipDirs();
 
   function walk(p: string): void {
     if (fileCount.value >= MAX_FILES_TRAVERSED) return;
@@ -95,7 +99,7 @@ function collectFiles(
       fileCount.value++;
     } else if (stat.isDirectory()) {
       // Skip well-known directories that mirror source or contain deps
-      if (SKIP_DIRS.has(basename(p))) return;
+      if (skipDirs.has(basename(p))) return;
 
       // Track visited directories by real path to prevent cycles
       let realPath: string;

server/src/tools/codeql/search-ql-code.ts

@@ -631,6 +631,27 @@ describe('completeQueryPath — directory filtering', () => {
     const result = await completeQueryPath('');
     expect(result).toEqual([]);
   });
+
+  it('should respect CODEQL_MCP_SCAN_EXCLUDE_DIRS changes after module load', async () => {
+    // Custom directory that is NOT in the default exclusion list
+    const customDir = join(tmpDir, 'custom-build');
+    mkdirSync(customDir, { recursive: true });
+    writeFileSync(join(customDir, 'Query.ql'), '');
+    writeFileSync(join(tmpDir, 'TopLevel.ql'), '');
+
+    // Without exclusion: custom-build should be found
+    vi.stubEnv('CODEQL_MCP_SCAN_EXCLUDE_DIRS', '');
+    clearCompletionCache();
+    const before = await completeQueryPath('');
+    expect(before).toContain(join('custom-build', 'Query.ql'));
+
+    // Now exclude custom-build via env var — should be respected
+    vi.stubEnv('CODEQL_MCP_SCAN_EXCLUDE_DIRS', 'custom-build');
+    clearCompletionCache();
+    const after = await completeQueryPath('');
+    expect(after).not.toContain(join('custom-build', 'Query.ql'));
+    expect(after).toContain('TopLevel.ql');
+  });
 });
 
 // ─────────────────────────────────────────────────────────────────────────────

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

@@ -2,7 +2,7 @@
  * Tests for search-ql-code tool
  */
 
-import { afterEach, describe, expect, it } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import { mkdirSync, writeFileSync } from 'fs';
 import { join } from 'path';
 import { searchQlCode } from '../../../../src/tools/codeql/search-ql-code';
@@ -337,4 +337,35 @@ describe('searchQlCode', () => {
       expect(result.results[0].filePath).toContain('lib');
     });
   });
+
+  describe('dynamic scan exclude dirs', () => {
+    beforeEach(() => {
+      vi.stubEnv('CODEQL_MCP_SCAN_EXCLUDE_DIRS', '');
+    });
+
+    afterEach(() => {
+      vi.unstubAllEnvs();
+    });
+
+    it('should respect CODEQL_MCP_SCAN_EXCLUDE_DIRS changes after module load', async () => {
+      tempDir = createTestTempDir('search-ql-code');
+      const srcDir = join(tempDir, 'src');
+      const customDir = join(tempDir, 'custom-build');
+      mkdirSync(srcDir, { recursive: true });
+      mkdirSync(customDir, { recursive: true });
+      writeFileSync(join(srcDir, 'Query.qll'), 'findme\n');
+      writeFileSync(join(customDir, 'Query.qll'), 'findme\n');
+
+      // Without exclusion: custom-build should be found
+      vi.stubEnv('CODEQL_MCP_SCAN_EXCLUDE_DIRS', '');
+      const before = await searchQlCode({ pattern: 'findme', paths: [tempDir] });
+      expect(before.totalMatches).toBe(2);
+
+      // Now exclude custom-build via env var — should be respected
+      vi.stubEnv('CODEQL_MCP_SCAN_EXCLUDE_DIRS', 'custom-build');
+      const after = await searchQlCode({ pattern: 'findme', paths: [tempDir] });
+      expect(after.totalMatches).toBe(1);
+      expect(after.results[0].filePath).toContain('src');
+    });
+  });
 });