Address review round 4: empty string edge cases, process.env guards, log clarity

- BQRS file param: check `file !== undefined` instead of truthiness,
  validate with `trim()` to catch whitespace-only strings
- bqrs_interpret database: always delete key from options regardless of
  value; fail fast on empty/whitespace-only string with clear error
- environment-builder: check `process.env` for inherited values instead
  of the freshly-constructed `env` object (which is always empty)
- result-processor: log "result count unknown" instead of "0 results"
  when resultCount is null
- Added tests: empty string file, whitespace file, empty database param,
  process.env ENABLE_ANNOTATION_TOOLS preservation

Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/f9deedbc-dbc0-4432-9d7b-775d1a9f624f

Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>

Author: Copilot

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

@@ -151,17 +151,18 @@ export class EnvironmentBuilder extends DisposableObject {
     // The setting controls ENABLE_ANNOTATION_TOOLS and defaults
     // MONITORING_STORAGE_LOCATION to the scratch directory so tools work
     // out-of-the-box without manual env var configuration.
-    // Only set these when not already defined, to avoid overriding values
-    // inherited from a parent process or earlier env setup.
+    // Respect values inherited from the extension host process environment;
+    // only apply defaults when not already defined there. The additionalEnv
+    // block below still overrides everything for advanced users.
     const enableAnnotations = config.get<boolean>('enableAnnotationTools', true);
-    if (!('ENABLE_ANNOTATION_TOOLS' in env)) {
+    if (typeof process.env.ENABLE_ANNOTATION_TOOLS === 'string') {
+      env.ENABLE_ANNOTATION_TOOLS = process.env.ENABLE_ANNOTATION_TOOLS;
+    } else {
       env.ENABLE_ANNOTATION_TOOLS = enableAnnotations ? 'true' : 'false';
     }
-    if (
-      enableAnnotations &&
-      env.CODEQL_MCP_SCRATCH_DIR &&
-      !('MONITORING_STORAGE_LOCATION' in env)
-    ) {
+    if (typeof process.env.MONITORING_STORAGE_LOCATION === 'string') {
+      env.MONITORING_STORAGE_LOCATION = process.env.MONITORING_STORAGE_LOCATION;
+    } else if (enableAnnotations && env.CODEQL_MCP_SCRATCH_DIR) {
       env.MONITORING_STORAGE_LOCATION = env.CODEQL_MCP_SCRATCH_DIR;
     }
 

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

@@ -281,33 +281,26 @@ describe('EnvironmentBuilder', () => {
   it('should not overwrite MONITORING_STORAGE_LOCATION if already set in parent env', async () => {
     const vscode = await import('vscode');
     const origFolders = vscode.workspace.workspaceFolders;
-    const originalGetConfig = vscode.workspace.getConfiguration;
+    const origMonLoc = process.env.MONITORING_STORAGE_LOCATION;
 
     try {
       (vscode.workspace.workspaceFolders as any) = [
         { uri: { fsPath: '/mock/workspace' }, name: 'ws', index: 0 },
       ];
       // Simulate parent process env with MONITORING_STORAGE_LOCATION already set
-      vscode.workspace.getConfiguration = () => ({
-        get: (_key: string, defaultVal?: any) => {
-          if (_key === 'additionalEnv') return { MONITORING_STORAGE_LOCATION: '/custom/storage/path' };
-          if (_key === 'additionalDatabaseDirs') return [];
-          if (_key === 'additionalQueryRunResultsDirs') return [];
-          if (_key === 'additionalMrvaRunResultsDirs') return [];
-          return defaultVal;
-        },
-        has: () => false,
-        inspect: () => undefined as any,
-        update: () => Promise.resolve(),
-      }) as any;
+      process.env.MONITORING_STORAGE_LOCATION = '/custom/storage/path';
 
       builder.invalidate();
       const env = await builder.build();
-      // additionalEnv should override the default MONITORING_STORAGE_LOCATION
+      // process.env value should be preserved
       expect(env.MONITORING_STORAGE_LOCATION).toBe('/custom/storage/path');
     } finally {
       (vscode.workspace.workspaceFolders as any) = origFolders;
-      vscode.workspace.getConfiguration = originalGetConfig;
+      if (origMonLoc === undefined) {
+        delete process.env.MONITORING_STORAGE_LOCATION;
+      } else {
+        process.env.MONITORING_STORAGE_LOCATION = origMonLoc;
+      }
     }
   });
 
@@ -380,4 +373,23 @@ describe('EnvironmentBuilder', () => {
       vscode.workspace.getConfiguration = originalGetConfig;
     }
   });
+
+  it('should preserve ENABLE_ANNOTATION_TOOLS from parent process environment', async () => {
+    const origValue = process.env.ENABLE_ANNOTATION_TOOLS;
+
+    try {
+      process.env.ENABLE_ANNOTATION_TOOLS = 'false';
+
+      builder.invalidate();
+      const env = await builder.build();
+      // Inherited process.env value should be preserved
+      expect(env.ENABLE_ANNOTATION_TOOLS).toBe('false');
+    } finally {
+      if (origValue === undefined) {
+        delete process.env.ENABLE_ANNOTATION_TOOLS;
+      } else {
+        process.env.ENABLE_ANNOTATION_TOOLS = origValue;
+      }
+    }
+  });
 });

server/src/lib/cli-tool-registry.ts

@@ -198,8 +198,10 @@ export function registerCLITool(server: McpServer, definition: CLIToolDefinition
           positionalArgs = [...positionalArgs, ...files as string[]];
         }
 
-        // Handle file parameter as positional argument for BQRS tools
-        if (file && name.startsWith('codeql_bqrs_')) {
+        // Handle file parameter as positional argument for BQRS tools.
+        // Check for key presence (not truthiness) so that empty strings
+        // are caught by the validation below rather than silently skipped.
+        if (file !== undefined && name.startsWith('codeql_bqrs_')) {
           // Defensive coercion: handle file value that is an actual array
           // or a JSON-encoded array string (e.g. '["/path/to/file.bqrs"]')
           let cleanFile = Array.isArray(file) ? (file.length > 0 ? String(file[0]) : '') : String(file);
@@ -213,7 +215,7 @@ export function registerCLITool(server: McpServer, definition: CLIToolDefinition
               }
             } catch { /* not valid JSON — use as-is */ }
           }
-          if (!cleanFile) {
+          if (!cleanFile.trim()) {
             throw new Error('The "file" parameter for BQRS tools must be a non-empty string path to a .bqrs file.');
           }
           positionalArgs = [...positionalArgs, cleanFile];
@@ -399,11 +401,19 @@ export function registerCLITool(server: McpServer, definition: CLIToolDefinition
             break;
           }
 
-          case 'codeql_bqrs_interpret':
+          case 'codeql_bqrs_interpret': {
             // Map 'database' to '--source-archive' and '--source-location-prefix'
-            // for codeql bqrs interpret (only when not explicitly provided)
-            if (options.database) {
-              const dbPath = resolveDatabasePath(options.database as string);
+            // for codeql bqrs interpret (only when not explicitly provided).
+            // Always delete the synthetic 'database' key to prevent it from
+            // being forwarded as an unsupported CLI flag.
+            const dbValue = options.database;
+            delete options.database;
+            if (dbValue !== undefined) {
+              const dbStr = String(dbValue).trim();
+              if (!dbStr) {
+                throw new Error('The "database" parameter must be a non-empty path to a CodeQL database.');
+              }
+              const dbPath = resolveDatabasePath(dbStr);
               if (!options['source-archive']) {
                 const srcZipPath = join(dbPath, 'src.zip');
                 const srcDirPath = join(dbPath, 'src');
@@ -412,7 +422,6 @@ export function registerCLITool(server: McpServer, definition: CLIToolDefinition
                 } else if (existsSync(srcDirPath)) {
                   options['source-archive'] = srcDirPath;
                 } else {
-                  delete options.database;
                   throw new Error(
                     `CodeQL database at "${dbPath}" does not contain a source archive (expected "src.zip" file or "src" directory).`,
                   );
@@ -432,9 +441,9 @@ export function registerCLITool(server: McpServer, definition: CLIToolDefinition
                   // codeql-database.yml missing or unparseable — skip prefix resolution
                 }
               }
-              delete options.database;
             }
             break;
+          }
 
           case 'codeql_query_compile':
           case 'codeql_resolve_metadata':

server/src/lib/result-processor.ts

@@ -382,7 +382,7 @@ export function cacheDatabaseAnalyzeResults(
       resultCount,
     });
 
-    logger.info(`Cached database-analyze results with key: ${cacheKey} (${resultCount ?? 0} results)`);
+    logger.info(`Cached database-analyze results with key: ${cacheKey} (${resultCount != null ? `${resultCount} results` : 'result count unknown'})`);
   } catch (err) {
     logger.error('Failed to cache database-analyze results:', err);
   }

server/test/src/lib/cli-tool-registry.test.ts

@@ -1301,4 +1301,70 @@ describe('registerCLITool handler behavior', () => {
       rmSync(tmpDir, { recursive: true, force: true });
     }
   });
+
+  it('should return error when file parameter is an empty string for BQRS tools', async () => {
+    const definition: CLIToolDefinition = {
+      name: 'codeql_bqrs_info',
+      description: 'BQRS info',
+      command: 'codeql',
+      subcommand: 'bqrs info',
+      inputSchema: {
+        file: z.string()
+      }
+    };
+
+    registerCLITool(mockServer, definition);
+
+    const handler = (mockServer.tool as ReturnType<typeof vi.fn>).mock.calls[0][3];
+
+    const result = await handler({ file: '' });
+    expect(result.isError).toBe(true);
+    expect(result.content[0].text).toContain('The "file" parameter for BQRS tools must be a non-empty string path to a .bqrs file.');
+  });
+
+  it('should return error when file parameter is a whitespace-only string for BQRS tools', async () => {
+    const definition: CLIToolDefinition = {
+      name: 'codeql_bqrs_decode',
+      description: 'Decode BQRS',
+      command: 'codeql',
+      subcommand: 'bqrs decode',
+      inputSchema: {
+        file: z.string()
+      }
+    };
+
+    registerCLITool(mockServer, definition);
+
+    const handler = (mockServer.tool as ReturnType<typeof vi.fn>).mock.calls[0][3];
+
+    const result = await handler({ file: '   ' });
+    expect(result.isError).toBe(true);
+    expect(result.content[0].text).toContain('The "file" parameter for BQRS tools must be a non-empty string path to a .bqrs file.');
+  });
+
+  it('should return error when database parameter is an empty string for bqrs_interpret', async () => {
+    const definition: CLIToolDefinition = {
+      name: 'codeql_bqrs_interpret',
+      description: 'Interpret BQRS',
+      command: 'codeql',
+      subcommand: 'bqrs interpret',
+      inputSchema: {
+        file: z.string(),
+        database: z.string().optional(),
+        format: z.string().optional()
+      }
+    };
+
+    registerCLITool(mockServer, definition);
+
+    const handler = (mockServer.tool as ReturnType<typeof vi.fn>).mock.calls[0][3];
+
+    const result = await handler({
+      file: '/path/to/results.bqrs',
+      database: '',
+      format: 'sarif-latest'
+    });
+    expect(result.isError).toBe(true);
+    expect(result.content[0].text).toContain('The "database" parameter must be a non-empty path to a CodeQL database.');
+  });
 });