Address PR #235 review feedback

Address PR #235 review comments for codeql_query_compile DIL persistence:

- Reorder path imports alphabetically (basename, delimiter, dirname, ...)
- Update logDir description to match pattern used by query-run, test-run,
  and database-analyze (mentions CODEQL_QUERY_LOG_DIR and default path)
- Defer compile log directory creation to post-execution so failed
  compilations do not leave empty directories behind
- Make DIL file test hermetic by controlling CODEQL_QUERY_LOG_DIR via a
  test-scoped temp directory and restoring it in a finally block
- Add test verifying no empty log directory on compilation failure
- Add test verifying logDir description matches other CLI tools

Author: data-douser

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

@@ -32676,9 +32676,8 @@ var require_side_channel_list = __commonJS({
           }
         },
         "delete": function(key) {
-          var root = $o && $o.next;
           var deletedNode = listDelete($o, key);
-          if (deletedNode && root && root === deletedNode) {
+          if (deletedNode && $o && !$o.next) {
             $o = void 0;
           }
           return !!deletedNode;
@@ -34321,9 +34320,9 @@ var require_parse = __commonJS({
       var limit = options.parameterLimit === Infinity ? void 0 : options.parameterLimit;
       var parts = cleanStr.split(
         options.delimiter,
-        options.throwOnLimitExceeded ? limit + 1 : limit
+        options.throwOnLimitExceeded && typeof limit !== "undefined" ? limit + 1 : limit
       );
-      if (options.throwOnLimitExceeded && parts.length > limit) {
+      if (options.throwOnLimitExceeded && typeof limit !== "undefined" && parts.length > limit) {
         throw new RangeError("Parameter limit exceeded. Only " + limit + " parameter" + (limit === 1 ? "" : "s") + " allowed.");
       }
       var skipIndex = -1;
@@ -38371,10 +38370,8 @@ var require_content_disposition = __commonJS({
     "use strict";
     module.exports = contentDisposition;
     module.exports.parse = parse4;
-    var basename11 = __require("path").basename;
+    var utf8Decoder = new TextDecoder("utf-8");
     var ENCODE_URL_ATTR_CHAR_REGEXP = /[\x00-\x20"'()*,/:;<=>?@[\\\]{}\x7f]/g;
-    var HEX_ESCAPE_REGEXP = /%[0-9A-Fa-f]{2}/;
-    var HEX_ESCAPE_REPLACE_REGEXP = /%([0-9A-Fa-f]{2})/g;
     var NON_LATIN1_REGEXP = /[^\x20-\x7e\xa0-\xff]/g;
     var QESC_REGEXP = /\\([\u0000-\u007f])/g;
     var QUOTE_REGEXP = /([\\"])/g;
@@ -38410,7 +38407,7 @@ var require_content_disposition = __commonJS({
       var isQuotedString = TEXT_REGEXP.test(name);
       var fallbackName = typeof fallback !== "string" ? fallback && getlatin1(name) : basename11(fallback);
       var hasFallback = typeof fallbackName === "string" && fallbackName !== name;
-      if (hasFallback || !isQuotedString || HEX_ESCAPE_REGEXP.test(name)) {
+      if (hasFallback || !isQuotedString || hasHexEscape(name)) {
         params["filename*"] = name;
       }
       if (isQuotedString || hasFallback) {
@@ -38437,26 +38434,32 @@ var require_content_disposition = __commonJS({
       return string3;
     }
     function decodefield(str2) {
-      var match = EXT_VALUE_REGEXP.exec(str2);
+      const match = EXT_VALUE_REGEXP.exec(str2);
       if (!match) {
         throw new TypeError("invalid extended field value");
       }
-      var charset = match[1].toLowerCase();
-      var encoded = match[2];
-      var value;
-      var binary2 = encoded.replace(HEX_ESCAPE_REPLACE_REGEXP, pdecode);
+      const charset = match[1].toLowerCase();
+      const encoded = match[2];
       switch (charset) {
-        case "iso-8859-1":
-          value = getlatin1(binary2);
-          break;
+        case "iso-8859-1": {
+          const binary2 = decodeHexEscapes(encoded);
+          return getlatin1(binary2);
+        }
         case "utf-8":
-        case "utf8":
-          value = Buffer.from(binary2, "binary").toString("utf8");
-          break;
-        default:
-          throw new TypeError("unsupported charset in extended field");
+        case "utf8": {
+          try {
+            return decodeURIComponent(encoded);
+          } catch {
+            const binary2 = decodeHexEscapes(encoded);
+            const bytes = new Uint8Array(binary2.length);
+            for (let idx = 0; idx < binary2.length; idx++) {
+              bytes[idx] = binary2.charCodeAt(idx);
+            }
+            return utf8Decoder.decode(bytes);
+          }
+        }
       }
-      return value;
+      throw new TypeError("unsupported charset in extended field");
     }
     function getlatin1(val) {
       return String(val).replace(NON_LATIN1_REGEXP, "?");
@@ -38506,9 +38509,6 @@ var require_content_disposition = __commonJS({
       }
       return new ContentDisposition(type2, params);
     }
-    function pdecode(str2, hex) {
-      return String.fromCharCode(parseInt(hex, 16));
-    }
     function pencode(char) {
       return "%" + String(char).charCodeAt(0).toString(16).toUpperCase();
     }
@@ -38525,6 +38525,51 @@ var require_content_disposition = __commonJS({
       this.type = type2;
       this.parameters = parameters;
     }
+    function basename11(path3) {
+      const normalized = path3.replaceAll("\\", "/");
+      let end = normalized.length;
+      while (end > 0 && normalized[end - 1] === "/") {
+        end--;
+      }
+      if (end === 0) {
+        return "";
+      }
+      let start = end - 1;
+      while (start >= 0 && normalized[start] !== "/") {
+        start--;
+      }
+      return normalized.slice(start + 1, end);
+    }
+    function isHexDigit(char) {
+      const code = char.charCodeAt(0);
+      return code >= 48 && code <= 57 || // 0-9
+      code >= 65 && code <= 70 || // A-F
+      code >= 97 && code <= 102;
+    }
+    function hasHexEscape(str2) {
+      const maxIndex = str2.length - 3;
+      let lastIndex = -1;
+      while ((lastIndex = str2.indexOf("%", lastIndex + 1)) !== -1 && lastIndex <= maxIndex) {
+        if (isHexDigit(str2[lastIndex + 1]) && isHexDigit(str2[lastIndex + 2])) {
+          return true;
+        }
+      }
+      return false;
+    }
+    function decodeHexEscapes(str2) {
+      const firstEscape = str2.indexOf("%");
+      if (firstEscape === -1) return str2;
+      let result = str2.slice(0, firstEscape);
+      for (let idx = firstEscape; idx < str2.length; idx++) {
+        if (str2[idx] === "%" && idx + 2 < str2.length && isHexDigit(str2[idx + 1]) && isHexDigit(str2[idx + 2])) {
+          result += String.fromCharCode(Number.parseInt(str2[idx + 1] + str2[idx + 2], 16));
+          idx += 2;
+        } else {
+          result += str2[idx];
+        }
+      }
+      return result;
+    }
   }
 });
 
@@ -40392,7 +40437,7 @@ var require_main = __commonJS({
             lastError = e;
           }
         }
-        _log(`injecting env (${keysCount}) from ${shortPaths.join(",")} ${dim(`// tip: ${_getRandomTip()}`)}`);
+        _log(`injected env (${keysCount}) from ${shortPaths.join(",")} ${dim(`// tip: ${_getRandomTip()}`)}`);
       }
       if (lastError) {
         return { parsed: parsedAll, error: lastError };
@@ -190955,7 +191000,7 @@ function cacheDatabaseAnalyzeResults(params, logger2) {
 // src/lib/cli-tool-registry.ts
 init_package_paths();
 import { existsSync as existsSync6, mkdirSync as mkdirSync8, realpathSync, rmSync, writeFileSync as writeFileSync4 } from "fs";
-import { delimiter as delimiter5, dirname as dirname5, basename as basename5, isAbsolute as isAbsolute4, join as join10, resolve as resolve4 } from "path";
+import { basename as basename5, delimiter as delimiter5, dirname as dirname5, isAbsolute as isAbsolute4, join as join10, resolve as resolve4 } from "path";
 
 // ../node_modules/js-yaml/dist/js-yaml.mjs
 function isNothing(subject) {
@@ -193921,14 +193966,11 @@ function registerCLITool(server, definition) {
             mkdirSync8(outputDir, { recursive: true });
           }
         }
-        let compileLogDir;
+        let effectiveDumpDilEnabled = false;
         if (name === "codeql_query_compile") {
           const pendingArgs = Array.isArray(options.additionalArgs) ? options.additionalArgs : [];
           const effectiveDumpDilDisabled = options["dump-dil"] === false || pendingArgs.includes("--no-dump-dil");
-          if (!effectiveDumpDilDisabled) {
-            compileLogDir = getOrCreateLogDirectory(customLogDir);
-            logger.info(`Using log directory for ${name}: ${compileLogDir}`);
-          }
+          effectiveDumpDilEnabled = !effectiveDumpDilDisabled;
         }
         const rawAdditionalArgs = Array.isArray(options.additionalArgs) ? options.additionalArgs : [];
         delete options.additionalArgs;
@@ -194026,8 +194068,10 @@ function registerCLITool(server, definition) {
           cacheDatabaseAnalyzeResults({ ...params, database: resolvedDb, output: options.output, format: options.format }, logger);
         }
         let dilFilePath;
-        if (name === "codeql_query_compile" && result.success && compileLogDir && result.stdout) {
+        if (name === "codeql_query_compile" && result.success && effectiveDumpDilEnabled && result.stdout) {
           try {
+            const compileLogDir = getOrCreateLogDirectory(customLogDir);
+            logger.info(`Using log directory for ${name}: ${compileLogDir}`);
             const queryBaseName = query ? basename5(query, ".ql") : "query";
             dilFilePath = join10(compileLogDir, `${queryBaseName}.dil`);
             writeFileSync4(dilFilePath, result.stdout, "utf8");
@@ -196107,7 +196151,7 @@ var codeqlQueryCompileTool = {
     database: external_exports.string().optional().describe("Path to the CodeQL database"),
     "dump-dil": external_exports.boolean().optional().describe("Print the optimized DIL intermediate representation to standard output while compiling. Enabled by default; pass false or --no-dump-dil to disable."),
     library: external_exports.string().optional().describe("Path to query library"),
-    logDir: external_exports.string().optional().describe("Directory to write the .dil file. If not provided, a unique log directory is created automatically."),
+    logDir: external_exports.string().optional().describe("Custom directory for compilation DIL output (overrides CODEQL_QUERY_LOG_DIR environment variable). If not provided, uses CODEQL_QUERY_LOG_DIR or defaults to .tmp/query-logs/<unique-id>"),
     output: external_exports.string().optional().describe("Output file path"),
     warnings: external_exports.enum(["hide", "show", "error"]).optional().describe("How to handle compilation warnings"),
     verbose: external_exports.boolean().optional().describe("Enable verbose output"),

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

@@ -13,7 +13,7 @@ import { resolveQueryPath } from './query-resolver';
 import { cacheDatabaseAnalyzeResults, processQueryRunResults } from './result-processor';
 import { getUserWorkspaceDir, packageRootDir } from '../utils/package-paths';
 import { existsSync, mkdirSync, realpathSync, rmSync, writeFileSync } from 'fs';
-import { delimiter, dirname, basename, isAbsolute, join, resolve } from 'path';
+import { basename, delimiter, dirname, isAbsolute, join, resolve } from 'path';
 import * as yaml from 'js-yaml';
 import { createProjectTempDir } from '../utils/temp-dir';
 
@@ -556,20 +556,18 @@ export function registerCLITool(server: McpServer, definition: CLIToolDefinition
           }
         }
 
-        // Set up log directory for compile runs to persist DIL output.
-        // Compute an effective "dump-dil enabled" flag that accounts for
-        // both `dump-dil: false` and `--no-dump-dil` in `additionalArgs`.
-        let compileLogDir: string | undefined;
+        // Compute an effective "dump-dil enabled" flag for codeql_query_compile
+        // that accounts for both `dump-dil: false` and `--no-dump-dil` in
+        // `additionalArgs`.  The log directory is created lazily post-success
+        // to avoid leaving empty directories behind on compilation failures.
+        let effectiveDumpDilEnabled = false;
         if (name === 'codeql_query_compile') {
           const pendingArgs = Array.isArray(options.additionalArgs)
             ? options.additionalArgs as string[]
             : [];
           const effectiveDumpDilDisabled = options['dump-dil'] === false
             || pendingArgs.includes('--no-dump-dil');
-          if (!effectiveDumpDilDisabled) {
-            compileLogDir = getOrCreateLogDirectory(customLogDir as string | undefined);
-            logger.info(`Using log directory for ${name}: ${compileLogDir}`);
-          }
+          effectiveDumpDilEnabled = !effectiveDumpDilDisabled;
         }
 
         // Extract additionalArgs from options so they are passed as raw CLI
@@ -733,10 +731,14 @@ export function registerCLITool(server: McpServer, definition: CLIToolDefinition
           cacheDatabaseAnalyzeResults({ ...params, database: resolvedDb, output: options.output, format: options.format }, logger);
         }
 
-        // Post-execution: persist DIL output to a .dil file for codeql_query_compile
+        // Post-execution: persist DIL output to a .dil file for codeql_query_compile.
+        // The log directory is created lazily here (only on success with output)
+        // to avoid leaving empty directories behind on compilation failures.
         let dilFilePath: string | undefined;
-        if (name === 'codeql_query_compile' && result.success && compileLogDir && result.stdout) {
+        if (name === 'codeql_query_compile' && result.success && effectiveDumpDilEnabled && result.stdout) {
           try {
+            const compileLogDir = getOrCreateLogDirectory(customLogDir as string | undefined);
+            logger.info(`Using log directory for ${name}: ${compileLogDir}`);
             const queryBaseName = query
               ? basename(query as string, '.ql')
               : 'query';

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

@@ -17,7 +17,7 @@ export const codeqlQueryCompileTool: CLIToolDefinition = {
       .describe('Print the optimized DIL intermediate representation to standard output while compiling. Enabled by default; pass false or --no-dump-dil to disable.'),
     library: z.string().optional().describe('Path to query library'),
     logDir: z.string().optional()
-      .describe('Directory to write the .dil file. If not provided, a unique log directory is created automatically.'),
+      .describe('Custom directory for compilation DIL output (overrides CODEQL_QUERY_LOG_DIR environment variable). If not provided, uses CODEQL_QUERY_LOG_DIR or defaults to .tmp/query-logs/<unique-id>'),
     output: z.string().optional().describe('Output file path'),
     warnings: z.enum(['hide', 'show', 'error']).optional()
       .describe('How to handle compilation warnings'),

server/src/tools/codeql/query-compile.ts

@@ -4,7 +4,7 @@
 
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'fs';
-import { dirname, join } from 'path';
+import { join } from 'path';
 import { McpServer } from '@modelcontextprotocol/sdk/server/mcp';
 import { z } from 'zod';
 import {
@@ -690,25 +690,36 @@ describe('registerCLITool handler behavior', () => {
       success: true
     });
 
-    const result = await handler({ query: '/path/to/MyQuery.ql' });
+    const codeqlQueryLogDir = createTestTempDir('codeql-query-log-dir');
+    const originalCodeqlQueryLogDir = process.env.CODEQL_QUERY_LOG_DIR;
+    process.env.CODEQL_QUERY_LOG_DIR = codeqlQueryLogDir;
 
-    // Response should contain the DIL file path
-    expect(result.content[0].text).toContain('DIL file:');
-    expect(result.content[0].text).toContain('MyQuery.dil');
-
-    // Extract the DIL file path from the response
-    const match = result.content[0].text.match(/DIL file: (.+)/);
-    expect(match).not.toBeNull();
-    const dilFilePath = match![1];
-
-    // Verify the .dil file was created with the correct content
-    expect(existsSync(dilFilePath)).toBe(true);
-    const fileContent = readFileSync(dilFilePath, 'utf8');
-    expect(fileContent).toBe(dilContent);
+    try {
+      const result = await handler({ query: '/path/to/MyQuery.ql' });
+
+      // Response should contain the DIL file path
+      expect(result.content[0].text).toContain('DIL file:');
+      expect(result.content[0].text).toContain('MyQuery.dil');
+
+      // Extract the DIL file path from the response
+      const match = result.content[0].text.match(/DIL file: (.+)/);
+      expect(match).not.toBeNull();
+      const dilFilePath = match![1];
+
+      // Verify the .dil file was created with the correct content
+      expect(existsSync(dilFilePath)).toBe(true);
+      expect(dilFilePath.startsWith(`${codeqlQueryLogDir}/`) || dilFilePath.startsWith(`${codeqlQueryLogDir}\\`)).toBe(true);
+      const fileContent = readFileSync(dilFilePath, 'utf8');
+      expect(fileContent).toBe(dilContent);
+    } finally {
+      if (originalCodeqlQueryLogDir === undefined) {
+        delete process.env.CODEQL_QUERY_LOG_DIR;
+      } else {
+        process.env.CODEQL_QUERY_LOG_DIR = originalCodeqlQueryLogDir;
+      }
 
-    // Clean up generated .dil file and its containing log directory
-    const dilDir = dirname(dilFilePath);
-    rmSync(dilDir, { recursive: true, force: true });
+      rmSync(codeqlQueryLogDir, { recursive: true, force: true });
+    }
   });
 
   it('should not save DIL file when dump-dil is explicitly false for codeql_query_compile', async () => {
@@ -740,6 +751,57 @@ describe('registerCLITool handler behavior', () => {
     expect(result.content[0].text).not.toContain('DIL file:');
   });
 
+  it('should not leave empty log directory when codeql_query_compile fails', async () => {
+    const definition: CLIToolDefinition = {
+      name: 'codeql_query_compile',
+      description: 'Compile query',
+      command: 'codeql',
+      subcommand: 'query compile',
+      inputSchema: {
+        query: z.string(),
+        'dump-dil': z.boolean().optional(),
+        additionalArgs: z.array(z.string()).optional()
+      }
+    };
+
+    registerCLITool(mockServer, definition);
+
+    const handler = (mockServer.registerTool as ReturnType<typeof vi.fn>).mock.calls[0][2];
+
+    // Simulate compilation failure
+    executeCodeQLCommand.mockResolvedValueOnce({
+      stdout: '',
+      stderr: 'Compilation error: syntax error',
+      success: false,
+      exitCode: 1
+    });
+
+    const codeqlQueryLogDir = createTestTempDir('codeql-query-log-dir');
+    const originalCodeqlQueryLogDir = process.env.CODEQL_QUERY_LOG_DIR;
+    process.env.CODEQL_QUERY_LOG_DIR = codeqlQueryLogDir;
+
+    try {
+      const result = await handler({ query: '/path/to/MyQuery.ql' });
+
+      // Should report failure
+      expect(result.isError).toBe(true);
+
+      // No new subdirectories should be created inside the log dir
+      // since DIL persistence is deferred to post-success
+      const { readdirSync } = await import('fs');
+      const entries = readdirSync(codeqlQueryLogDir);
+      expect(entries).toHaveLength(0);
+    } finally {
+      if (originalCodeqlQueryLogDir === undefined) {
+        delete process.env.CODEQL_QUERY_LOG_DIR;
+      } else {
+        process.env.CODEQL_QUERY_LOG_DIR = originalCodeqlQueryLogDir;
+      }
+
+      rmSync(codeqlQueryLogDir, { recursive: true, force: true });
+    }
+  });
+
   it('should not save DIL file when --no-dump-dil is in additionalArgs for codeql_query_compile', async () => {
     const definition: CLIToolDefinition = {
       name: 'codeql_query_compile',