Address PR review comments

Author: data-douser

extensions/vscode/package.json

@@ -54,7 +54,7 @@
             "type": "string"
           },
           "default": [],
-          "description": "Additional directories to search for CodeQL databases. Appended to CODEQL_DATABASES_BASE_DIRS alongside the vscode-codeql storage paths."
+          "description": "Additional directories to search for CodeQL databases. Appended to CODEQL_DATABASES_BASE_DIRS. When copyDatabases is enabled (default), they are appended alongside the managed databases directory; when copyDatabases is disabled, they are appended alongside the original vscode-codeql database storage directories."
         },
         "codeql-mcp.additionalEnv": {
           "type": "object",

extensions/vscode/src/bridge/database-copier.ts

@@ -1,4 +1,5 @@
-import { cpSync, existsSync, mkdirSync, readdirSync, rmSync, statSync, unlinkSync } from 'fs';
+import { existsSync } from 'fs';
+import { cp, mkdir, readdir, rm, stat, unlink } from 'fs/promises';
 import { join } from 'path';
 import type { Logger } from '../common/logger';
 
@@ -29,8 +30,8 @@ export class DatabaseCopier {
    * @returns The list of database paths in the managed destination that
    *          are ready for use (absolute paths).
    */
-  syncAll(sourceDirs: string[]): string[] {
-    mkdirSync(this.destinationBase, { recursive: true });
+  async syncAll(sourceDirs: string[]): Promise<string[]> {
+    await mkdir(this.destinationBase, { recursive: true });
 
     const copied: string[] = [];
 
@@ -41,24 +42,26 @@ export class DatabaseCopier {
 
       let entries: string[];
       try {
-        entries = readdirSync(sourceDir);
+        entries = await readdir(sourceDir);
       } catch {
         continue;
       }
 
       for (const entry of entries) {
         const srcDbPath = join(sourceDir, entry);
-        if (!isCodeQLDatabase(srcDbPath)) {
+        if (!(await isCodeQLDatabase(srcDbPath))) {
           continue;
         }
 
         const destDbPath = join(this.destinationBase, entry);
 
-        if (this.needsCopy(srcDbPath, destDbPath)) {
-          this.copyDatabase(srcDbPath, destDbPath);
+        if (await this.needsCopy(srcDbPath, destDbPath)) {
+          await this.copyDatabase(srcDbPath, destDbPath);
         }
 
-        copied.push(destDbPath);
+        if (await isCodeQLDatabase(destDbPath)) {
+          copied.push(destDbPath);
+        }
       }
     }
 
@@ -69,16 +72,16 @@ export class DatabaseCopier {
    * Copy a single database directory, then strip any `.lock` files that
    * the CodeQL query server may have left behind.
    */
-  private copyDatabase(src: string, dest: string): void {
+  private async copyDatabase(src: string, dest: string): Promise<void> {
     this.logger.info(`Copying database ${src} → ${dest}`);
     try {
       // Remove stale destination if present
       if (existsSync(dest)) {
-        rmSync(dest, { recursive: true, force: true });
+        await rm(dest, { recursive: true, force: true });
       }
 
-      cpSync(src, dest, { recursive: true });
-      removeLockFiles(dest);
+      await cp(src, dest, { recursive: true });
+      await removeLockFiles(dest);
       this.logger.info(`Database copied successfully: ${dest}`);
     } catch (err) {
       this.logger.error(
@@ -91,16 +94,16 @@ export class DatabaseCopier {
    * A copy is needed when the destination does not exist, or the source
    * `codeql-database.yml` is newer than the destination's.
    */
-  private needsCopy(src: string, dest: string): boolean {
+  private async needsCopy(src: string, dest: string): Promise<boolean> {
     const destYml = join(dest, 'codeql-database.yml');
     if (!existsSync(destYml)) {
       return true;
     }
 
     const srcYml = join(src, 'codeql-database.yml');
     try {
-      const srcMtime = statSync(srcYml).mtimeMs;
-      const destMtime = statSync(destYml).mtimeMs;
+      const srcMtime = (await stat(srcYml)).mtimeMs;
+      const destMtime = (await stat(destYml)).mtimeMs;
       return srcMtime > destMtime;
     } catch {
       // If stat fails, re-copy to be safe
@@ -110,9 +113,9 @@ export class DatabaseCopier {
 }
 
 /** Check whether a directory looks like a CodeQL database. */
-function isCodeQLDatabase(dirPath: string): boolean {
+async function isCodeQLDatabase(dirPath: string): Promise<boolean> {
   try {
-    return statSync(dirPath).isDirectory() && existsSync(join(dirPath, 'codeql-database.yml'));
+    return (await stat(dirPath)).isDirectory() && existsSync(join(dirPath, 'codeql-database.yml'));
   } catch {
     return false;
   }
@@ -123,22 +126,22 @@ function isCodeQLDatabase(dirPath: string): boolean {
  * These are empty sentinel files created by the CodeQL query server in
  * `<dataset>/default/cache/.lock`.
  */
-function removeLockFiles(dir: string): void {
+async function removeLockFiles(dir: string): Promise<void> {
   let entries: string[];
   try {
-    entries = readdirSync(dir);
+    entries = await readdir(dir);
   } catch {
     return;
   }
 
   for (const entry of entries) {
     const fullPath = join(dir, entry);
     try {
-      const stat = statSync(fullPath);
-      if (stat.isDirectory()) {
-        removeLockFiles(fullPath);
+      const st = await stat(fullPath);
+      if (st.isDirectory()) {
+        await removeLockFiles(fullPath);
       } else if (entry === '.lock') {
-        unlinkSync(fullPath);
+        await unlink(fullPath);
       }
     } catch {
       // Best-effort removal

extensions/vscode/src/bridge/environment-builder.ts

@@ -1,5 +1,5 @@
 import * as vscode from 'vscode';
-import { join } from 'path';
+import { delimiter, join } from 'path';
 import { DisposableObject } from '../common/disposable';
 import type { Logger } from '../common/logger';
 import type { CliResolver } from '../codeql/cli-resolver';
@@ -89,7 +89,7 @@ export class EnvironmentBuilder extends DisposableObject {
       }
     }
 
-    env.CODEQL_ADDITIONAL_PACKS = additionalPaths.join(':');
+    env.CODEQL_ADDITIONAL_PACKS = additionalPaths.join(delimiter);
 
     // Database discovery directories for list_codeql_databases
     // Includes: global storage, workspace storage, and user-configured dirs
@@ -104,24 +104,24 @@ export class EnvironmentBuilder extends DisposableObject {
     if (copyEnabled) {
       const managedDir = this.storagePaths.getManagedDatabaseStoragePath();
       const copier = this.copierFactory(managedDir, this.logger);
-      copier.syncAll(sourceDirs);
+      await copier.syncAll(sourceDirs);
       dbDirs = [managedDir, ...userDbDirs];
     } else {
       dbDirs = [...sourceDirs, ...userDbDirs];
     }
-    env.CODEQL_DATABASES_BASE_DIRS = dbDirs.join(':');
+    env.CODEQL_DATABASES_BASE_DIRS = dbDirs.join(delimiter);
 
     // MRVA run results directory for variant analysis discovery
     const mrvaDirs = [this.storagePaths.getVariantAnalysisStoragePath()];
     const userMrvaDirs = config.get<string[]>('additionalMrvaRunResultsDirs', []);
     mrvaDirs.push(...userMrvaDirs);
-    env.CODEQL_MRVA_RUN_RESULTS_DIRS = mrvaDirs.join(':');
+    env.CODEQL_MRVA_RUN_RESULTS_DIRS = mrvaDirs.join(delimiter);
 
     // Query run results directory for query history discovery
     const queryDirs = [this.storagePaths.getQueryStoragePath()];
     const userQueryDirs = config.get<string[]>('additionalQueryRunResultsDirs', []);
     queryDirs.push(...userQueryDirs);
-    env.CODEQL_QUERY_RUN_RESULTS_DIRS = queryDirs.join(':');
+    env.CODEQL_QUERY_RUN_RESULTS_DIRS = queryDirs.join(delimiter);
 
     // User-configured additional environment variables
     const additionalEnv = config.get<Record<string, string>>('additionalEnv', {});

extensions/vscode/test/bridge/database-copier.test.ts

@@ -61,35 +61,35 @@ describe('DatabaseCopier', () => {
     expect(copier).toBeDefined();
   });
 
-  it('should copy a database to the destination', () => {
+  it('should copy a database to the destination', async () => {
     createFakeDatabase(sourceDir, 'my-db');
 
     const copier = new DatabaseCopier(destDir, logger);
-    const results = copier.syncAll([sourceDir]);
+    const results = await copier.syncAll([sourceDir]);
 
     expect(results).toHaveLength(1);
     expect(results[0]).toBe(join(destDir, 'my-db'));
     expect(existsSync(join(destDir, 'my-db', 'codeql-database.yml'))).toBe(true);
   });
 
-  it('should create the destination directory if it does not exist', () => {
+  it('should create the destination directory if it does not exist', async () => {
     createFakeDatabase(sourceDir, 'db-1');
 
     const copier = new DatabaseCopier(destDir, logger);
-    copier.syncAll([sourceDir]);
+    await copier.syncAll([sourceDir]);
 
     expect(existsSync(destDir)).toBe(true);
   });
 
-  it('should remove .lock files from the copy', () => {
+  it('should remove .lock files from the copy', async () => {
     createFakeDatabase(sourceDir, 'locked-db', { withLock: true });
 
     // Verify lock exists in source
     const srcLock = join(sourceDir, 'locked-db', 'db-javascript', 'default', 'cache', '.lock');
     expect(existsSync(srcLock)).toBe(true);
 
     const copier = new DatabaseCopier(destDir, logger);
-    copier.syncAll([sourceDir]);
+    await copier.syncAll([sourceDir]);
 
     // Lock file should NOT exist in the copy
     const destLock = join(destDir, 'locked-db', 'db-javascript', 'default', 'cache', '.lock');
@@ -100,94 +100,100 @@ describe('DatabaseCopier', () => {
     expect(existsSync(join(destDir, 'locked-db', 'db-javascript', 'default', 'cache'))).toBe(true);
   });
 
-  it('should not re-copy a database that has not changed', () => {
+  it('should not re-copy a database that has not changed', async () => {
     createFakeDatabase(sourceDir, 'stable-db');
 
     const copier = new DatabaseCopier(destDir, logger);
-    copier.syncAll([sourceDir]);
+    await copier.syncAll([sourceDir]);
     expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('Copying database'));
 
     logger.info.mockClear();
-    copier.syncAll([sourceDir]);
+    await copier.syncAll([sourceDir]);
 
     // Second call should NOT log a copy (database unchanged)
     expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining('Copying database'));
   });
 
-  it('should re-copy a database when source is newer', () => {
+  it('should re-copy a database when source is newer', async () => {
     createFakeDatabase(sourceDir, 'updated-db');
 
     const copier = new DatabaseCopier(destDir, logger);
-    copier.syncAll([sourceDir]);
+    await copier.syncAll([sourceDir]);
 
     // Advance the source codeql-database.yml mtime into the future
     const srcYml = join(sourceDir, 'updated-db', 'codeql-database.yml');
     const future = new Date(Date.now() + 10_000);
     utimesSync(srcYml, future, future);
 
     logger.info.mockClear();
-    copier.syncAll([sourceDir]);
+    await copier.syncAll([sourceDir]);
 
     expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('Copying database'));
   });
 
-  it('should handle multiple source directories', () => {
+  it('should handle multiple source directories', async () => {
     const sourceDir2 = join(tmpDir, 'source2');
     mkdirSync(sourceDir2, { recursive: true });
 
     createFakeDatabase(sourceDir, 'db-a');
     createFakeDatabase(sourceDir2, 'db-b');
 
     const copier = new DatabaseCopier(destDir, logger);
-    const results = copier.syncAll([sourceDir, sourceDir2]);
+    const results = await copier.syncAll([sourceDir, sourceDir2]);
 
     expect(results).toHaveLength(2);
     expect(existsSync(join(destDir, 'db-a', 'codeql-database.yml'))).toBe(true);
     expect(existsSync(join(destDir, 'db-b', 'codeql-database.yml'))).toBe(true);
   });
 
-  it('should skip non-existent source directories', () => {
+  it('should skip non-existent source directories', async () => {
     const copier = new DatabaseCopier(destDir, logger);
-    const results = copier.syncAll(['/nonexistent/path']);
+    const results = await copier.syncAll(['/nonexistent/path']);
 
     expect(results).toHaveLength(0);
   });
 
-  it('should skip directories that are not CodeQL databases', () => {
+  it('should skip directories that are not CodeQL databases', async () => {
     // Create a regular directory (no codeql-database.yml)
     const notADb = join(sourceDir, 'not-a-db');
     mkdirSync(notADb, { recursive: true });
     writeFileSync(join(notADb, 'README.md'), '# Not a database');
 
     const copier = new DatabaseCopier(destDir, logger);
-    const results = copier.syncAll([sourceDir]);
+    const results = await copier.syncAll([sourceDir]);
 
     expect(results).toHaveLength(0);
   });
 
-  it('should not modify the source .lock files', () => {
+  it('should not modify the source .lock files', async () => {
     createFakeDatabase(sourceDir, 'locked-db', { withLock: true });
     const srcLock = join(sourceDir, 'locked-db', 'db-javascript', 'default', 'cache', '.lock');
 
     const copier = new DatabaseCopier(destDir, logger);
-    copier.syncAll([sourceDir]);
+    await copier.syncAll([sourceDir]);
 
     // Source lock file must remain untouched
     expect(existsSync(srcLock)).toBe(true);
   });
 
-  it('should log an error when copy fails', () => {
+  it('should log an error and exclude database when copy fails', async () => {
     createFakeDatabase(sourceDir, 'bad-db');
 
-    // Make destination read-only to cause a copy failure
-    mkdirSync(destDir, { recursive: true });
-    const blockerPath = join(destDir, 'bad-db');
-    writeFileSync(blockerPath, 'I am a file, not a directory');
+    // Make the destination base directory read-only to prevent cp from writing
+    mkdirSync(destDir, { mode: 0o444, recursive: true });
 
     const copier = new DatabaseCopier(destDir, logger);
-    // rmSync on a file should succeed, then cpSync should work — but
-    // this tests the error path if something goes truly wrong.
-    // Actually, since rmSync handles files, let's just verify no throw:
-    expect(() => copier.syncAll([sourceDir])).not.toThrow();
+    const results = await copier.syncAll([sourceDir]);
+
+    // Restore permissions for cleanup
+    const { chmod } = await import('fs/promises');
+    await chmod(destDir, 0o755);
+
+    // Should have logged the error
+    expect(logger.error).toHaveBeenCalledWith(
+      expect.stringContaining('Failed to copy database'),
+    );
+    // Should NOT include the failed database in results
+    expect(results).toHaveLength(0);
   });
 });

extensions/vscode/test/bridge/environment-builder.test.ts

@@ -1,4 +1,5 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { delimiter } from 'path';
 
 import { EnvironmentBuilder } from '../../src/bridge/environment-builder';
 import type { DatabaseCopierFactory } from '../../src/bridge/environment-builder';
@@ -49,7 +50,7 @@ function createMockLogger() {
 }
 
 function createMockCopierFactory(): { factory: DatabaseCopierFactory; syncAll: ReturnType<typeof vi.fn> } {
-  const syncAll = vi.fn().mockReturnValue([]);
+  const syncAll = vi.fn().mockResolvedValue([]);
   const factory: DatabaseCopierFactory = () => ({ syncAll } as any);
   return { factory, syncAll };
 }
@@ -199,7 +200,7 @@ describe('EnvironmentBuilder', () => {
     builder.invalidate();
     const env = await builder.build();
     expect(env.CODEQL_DATABASES_BASE_DIRS).toBe(
-      '/mock/global-storage/GitHub.vscode-codeql:/mock/workspace-storage/ws-123/GitHub.vscode-codeql',
+      ['/mock/global-storage/GitHub.vscode-codeql', '/mock/workspace-storage/ws-123/GitHub.vscode-codeql'].join(delimiter),
     );
 
     vscode.workspace.getConfiguration = originalGetConfig;