Address review feedback for PR #260

data-douser · data-douser · commit 32bfd06bad5d · 2026-04-20T09:53:05.000-06:00
diff --git a/.github/actions/setup-codeql-environment/action.yml b/.github/actions/setup-codeql-environment/action.yml
@@ -29,7 +29,7 @@ inputs:
   go-version:
     description: 'Go version to install'
     required: false
-    default: '1.25'
+    default: '1.25.0'
   dotnet-version:
     description: '.NET version to install'
     required: false
diff --git a/client/Makefile b/client/Makefile
@@ -51,7 +51,7 @@ test-coverage:
 	go tool cover -html=coverage.out -o coverage.html
 
 ## lint: Run linters
-lint:
+lint: tidy
 	@if command -v golangci-lint > /dev/null 2>&1; then \
 		golangci-lint run ./...; \
 	else \
diff --git a/client/README.md b/client/README.md
@@ -31,12 +31,12 @@ gh ql-mcp-client <command> [flags]
 
 ### Global Flags
 
-| Flag       | Default     | Description                           |
-| ---------- | ----------- | ------------------------------------- |
-| `--mode`   | `stdio`     | MCP server transport (`stdio`/`http`) |
-| `--host`   | `localhost` | MCP server host (http mode)           |
-| `--port`   | `3000`      | MCP server port (http mode)           |
-| `--format` | `text`      | Output format (`text`/`json`)         |
+| Flag       | Default     | Description                              |
+| ---------- | ----------- | ---------------------------------------- |
+| `--mode`   | `stdio`     | MCP server transport (`stdio`/`http`)    |
+| `--host`   | `localhost` | MCP server host (http mode)              |
+| `--port`   | `3000`      | MCP server port (http mode)              |
+| `--format` | `text`      | Output format (`text`/`json`/`markdown`) |
 
 Transport is configured via CLI flags. The CLI does not currently read `MCP_MODE`.
 
diff --git a/client/go.mod b/client/go.mod
@@ -1,6 +1,6 @@
 module github.com/advanced-security/codeql-development-mcp-server/client
 
-go 1.25.6
+go 1.25.0
 
 require (
 	github.com/cli/go-gh/v2 v2.13.0
diff --git a/server/dist/codeql-development-mcp-server.js b/server/dist/codeql-development-mcp-server.js
@@ -188630,13 +188630,13 @@ init_logger();
 // src/lib/log-directory-manager.ts
 init_temp_dir();
 import { mkdirSync as mkdirSync3, existsSync as existsSync5 } from "fs";
-import { join as join7, resolve as resolve3, sep, relative } from "path";
+import { join as join7, resolve as resolve3, sep, relative, isAbsolute as isAbsolute3 } from "path";
 import { randomBytes } from "crypto";
 function ensurePathWithinBase(baseDir, targetPath) {
   const absBase = resolve3(baseDir);
   const absTarget = resolve3(targetPath);
   const rel = relative(absBase, absTarget);
-  if (rel === ".." || rel.startsWith(".." + sep)) {
+  if (rel === ".." || rel.startsWith(".." + sep) || isAbsolute3(rel)) {
     throw new Error(`Provided log directory is outside the allowed base directory: ${absBase}`);
   }
   return absTarget;
@@ -188772,7 +188772,7 @@ import { createHash as createHash2 } from "crypto";
 init_cli_executor();
 init_logger();
 import { closeSync, fstatSync, mkdirSync as mkdirSync4, openSync, readFileSync as readFileSync5, writeFileSync } from "fs";
-import { dirname as dirname3, isAbsolute as isAbsolute3 } from "path";
+import { dirname as dirname3, isAbsolute as isAbsolute4 } from "path";
 var BUILT_IN_EVALUATORS = {
   "json-decode": "JSON format decoder for query results",
   "csv-decode": "CSV format decoder for query results",
@@ -189025,7 +189025,7 @@ async function evaluateQueryResults(bqrsPath, queryPath, evaluationFunction, out
       case "mermaid-graph":
         return await evaluateWithMermaidGraph(bqrsPath, queryPath, outputPath);
       default:
-        if (isAbsolute3(evalFunc)) {
+        if (isAbsolute4(evalFunc)) {
           return await evaluateWithCustomScript(bqrsPath, queryPath, evalFunc, outputPath);
         } else {
           return {
@@ -191104,7 +191104,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 { basename as basename5, delimiter as delimiter5, dirname as dirname5, isAbsolute as isAbsolute4, join as join10, resolve as resolve4 } from "path";
+import { basename as basename5, delimiter as delimiter5, dirname as dirname5, isAbsolute as isAbsolute5, join as join10, resolve as resolve4 } from "path";
 
 // ../node_modules/js-yaml/dist/js-yaml.mjs
 function isNothing(subject) {
@@ -193883,12 +193883,12 @@ function registerCLITool(server, definition) {
             if (tests && Array.isArray(tests)) {
               const userDir = getUserWorkspaceDir();
               positionalArgs = [...positionalArgs, ...tests.map(
-                (t) => isAbsolute4(t) ? t : resolve4(userDir, t)
+                (t) => isAbsolute5(t) ? t : resolve4(userDir, t)
               )];
             }
             break;
           case "codeql_query_run": {
-            if (options.database && typeof options.database === "string" && !isAbsolute4(options.database)) {
+            if (options.database && typeof options.database === "string" && !isAbsolute5(options.database)) {
               options.database = resolve4(getUserWorkspaceDir(), options.database);
               logger.info(`Resolved database path to: ${options.database}`);
             }
@@ -194120,7 +194120,7 @@ function registerCLITool(server, definition) {
           let cwd;
           if ((name === "codeql_pack_install" || name === "codeql_pack_ls") && (dir || packDir)) {
             const rawCwd = dir || packDir;
-            cwd = isAbsolute4(rawCwd) ? rawCwd : resolve4(getUserWorkspaceDir(), rawCwd);
+            cwd = isAbsolute5(rawCwd) ? rawCwd : resolve4(getUserWorkspaceDir(), rawCwd);
           }
           const defaultExamplesPath = resolve4(packageRootDir, "ql", "javascript", "examples");
           const additionalPacksPath = process.env.CODEQL_ADDITIONAL_PACKS || (existsSync6(defaultExamplesPath) ? defaultExamplesPath : void 0);
@@ -194196,11 +194196,21 @@ function registerCLITool(server, definition) {
             dilFilePath = void 0;
           }
         }
-        let processedResult = resultProcessor(result, params);
+        let processedResult;
         if (dilFilePath) {
+          const stdoutBytes = Buffer.byteLength(result.stdout ?? "", "utf8");
+          processedResult = `Compiled successfully. DIL output written to file (${stdoutBytes} bytes).`;
+          if (result.stderr) {
+            processedResult += `
+
+Warnings/Info:
+${result.stderr}`;
+          }
           processedResult += `
 
 DIL file: ${dilFilePath}`;
+        } else {
+          processedResult = resultProcessor(result, params);
         }
         return {
           content: [{
@@ -197774,7 +197784,7 @@ import { pathToFileURL as pathToFileURL3 } from "url";
 // src/tools/lsp/lsp-server-helper.ts
 init_server_manager();
 init_logger();
-import { isAbsolute as isAbsolute5, resolve as resolve11 } from "path";
+import { isAbsolute as isAbsolute6, resolve as resolve11 } from "path";
 import { pathToFileURL as pathToFileURL2 } from "url";
 async function getInitializedLanguageServer(opts = {}) {
   const { packageRootDir: pkgRoot, getUserWorkspaceDir: getUserWorkspaceDir2 } = await Promise.resolve().then(() => (init_package_paths(), package_paths_exports));
@@ -197790,7 +197800,7 @@ async function getInitializedLanguageServer(opts = {}) {
   const server = await manager.getLanguageServer(config2);
   let effectiveUri = opts.workspaceUri;
   if (effectiveUri && !effectiveUri.startsWith("file://")) {
-    const absWorkspace = isAbsolute5(effectiveUri) ? effectiveUri : resolve11(getUserWorkspaceDir2(), effectiveUri);
+    const absWorkspace = isAbsolute6(effectiveUri) ? effectiveUri : resolve11(getUserWorkspaceDir2(), effectiveUri);
     effectiveUri = pathToFileURL2(absWorkspace).href;
   }
   effectiveUri = effectiveUri ?? pathToFileURL2(resolve11(pkgRoot, "ql")).href;
@@ -197955,7 +197965,7 @@ function registerLspDiagnosticsTool(server) {
 init_logger();
 init_package_paths();
 import { readFile as readFile3 } from "fs/promises";
-import { isAbsolute as isAbsolute6, resolve as resolve12 } from "path";
+import { isAbsolute as isAbsolute7, resolve as resolve12 } from "path";
 import { pathToFileURL as pathToFileURL4 } from "url";
 async function getInitializedServer(params) {
   return getInitializedLanguageServer({
@@ -197964,7 +197974,7 @@ async function getInitializedServer(params) {
   });
 }
 function prepareDocumentPosition(params) {
-  const absPath = isAbsolute6(params.filePath) ? params.filePath : resolve12(getUserWorkspaceDir(), params.filePath);
+  const absPath = isAbsolute7(params.filePath) ? params.filePath : resolve12(getUserWorkspaceDir(), params.filePath);
   const docUri = pathToFileURL4(absPath).href;
   return { absPath, docUri };
 }
@@ -198394,7 +198404,7 @@ function registerLanguageResources(server) {
 
 // src/prompts/workflow-prompts.ts
 import { access as access2 } from "fs/promises";
-import { basename as basename10, isAbsolute as isAbsolute7, normalize, relative as relative3, resolve as resolve13, sep as sep4 } from "path";
+import { basename as basename10, isAbsolute as isAbsolute8, normalize, relative as relative3, resolve as resolve13, sep as sep4 } from "path";
 import { fileURLToPath as fileURLToPath3 } from "url";
 
 // src/prompts/constants.ts
@@ -198826,11 +198836,11 @@ async function resolvePromptFilePath(filePath, workspaceRoot) {
   }
   const effectiveRoot = workspaceRoot ?? getUserWorkspaceDir();
   const normalizedPath = normalize(effectivePath);
-  const inputWasAbsolute = isAbsolute7(normalizedPath);
+  const inputWasAbsolute = isAbsolute8(normalizedPath);
   const absolutePath = inputWasAbsolute ? normalizedPath : resolve13(effectiveRoot, normalizedPath);
   if (!inputWasAbsolute) {
     const rel = relative3(effectiveRoot, absolutePath);
-    if (rel === ".." || rel.startsWith(`..${sep4}`) || isAbsolute7(rel)) {
+    if (rel === ".." || rel.startsWith(`..${sep4}`) || isAbsolute8(rel)) {
       return {
         blocked: true,
         resolvedPath: "",
diff --git a/server/dist/codeql-development-mcp-server.js.map b/server/dist/codeql-development-mcp-server.js.map
diff --git a/server/ql/go/tools/test/CallGraphFrom/go.mod b/server/ql/go/tools/test/CallGraphFrom/go.mod
@@ -1,3 +1,3 @@
 module languages/go/tools/test/CallGraphFrom
 
-go 1.21.13
+go 1.21
diff --git a/server/ql/go/tools/test/CallGraphFromTo/go.mod b/server/ql/go/tools/test/CallGraphFromTo/go.mod
@@ -1,3 +1,3 @@
 module languages/go/tools/test/CallGraphFromTo
 
-go 1.21.13
+go 1.21
diff --git a/server/ql/go/tools/test/CallGraphTo/go.mod b/server/ql/go/tools/test/CallGraphTo/go.mod
@@ -1,3 +1,3 @@
 module languages/go/tools/test/CallGraphTo
 
-go 1.21.13
+go 1.21
diff --git a/server/src/lib/cli-tool-registry.ts b/server/src/lib/cli-tool-registry.ts
@@ -763,11 +763,21 @@ export function registerCLITool(server: McpServer, definition: CLIToolDefinition
         }
 
         // Process the result
-        let processedResult = resultProcessor(result, params);
-
-        // Append DIL file path to the response for codeql_query_compile
+        let processedResult: string;
         if (dilFilePath) {
+          // DIL output was successfully persisted to a file. Don't echo the
+          // full DIL back through stdout — it can be very large and would
+          // exceed MCP response size budgets / degrade LLM performance.
+          // Surface only a short summary plus the file path; preserve any
+          // stderr (warnings/info) the CLI emitted for diagnostic value.
+          const stdoutBytes = Buffer.byteLength(result.stdout ?? '', 'utf8');
+          processedResult = `Compiled successfully. DIL output written to file (${stdoutBytes} bytes).`;
+          if (result.stderr) {
+            processedResult += `\n\nWarnings/Info:\n${result.stderr}`;
+          }
           processedResult += `\n\nDIL file: ${dilFilePath}`;
+        } else {
+          processedResult = resultProcessor(result, params);
         }
 
         return {
diff --git a/server/src/lib/log-directory-manager.ts b/server/src/lib/log-directory-manager.ts
@@ -3,7 +3,7 @@
  */
 
 import { mkdirSync, existsSync } from 'fs';
-import { join, resolve, sep, relative } from 'path';
+import { join, resolve, sep, relative, isAbsolute } from 'path';
 import { randomBytes } from 'crypto';
 import { getProjectTmpDir } from '../utils/temp-dir';
 
@@ -18,9 +18,12 @@ function ensurePathWithinBase(baseDir: string, targetPath: string): string {
   // This avoids false positives from a simple startsWith() check (e.g. on
   // Windows where drive-letter casing can differ between absBase and absTarget).
   const rel = relative(absBase, absTarget);
-  // If the relative path starts with ".." (or equals ".."), the target is
-  // outside the base directory.
-  if (rel === '..' || rel.startsWith('..' + sep)) {
+  // The target is outside the base directory if either:
+  //   - the relative path starts with ".." (different subtree), or
+  //   - the relative path is itself absolute. On Windows, path.relative()
+  //     returns an absolute path (e.g. "D:\\...") when the two paths live on
+  //     different drives, which would otherwise bypass the ".." check.
+  if (rel === '..' || rel.startsWith('..' + sep) || isAbsolute(rel)) {
     throw new Error(`Provided log directory is outside the allowed base directory: ${absBase}`);
   }
   return absTarget;
diff --git a/server/test/src/lib/cli-tool-registry.test.ts b/server/test/src/lib/cli-tool-registry.test.ts
@@ -724,6 +724,58 @@ describe('registerCLITool handler behavior', () => {
     }
   });
 
+  it('should not echo full DIL stdout back to caller when DIL is persisted', async () => {
+    // Regression: result.stdout from `codeql query compile --dump-dil` can be
+    // very large. When we successfully persist it to a .dil file, the response
+    // should report a short summary + file path rather than the raw DIL text,
+    // so MCP responses stay within size budgets.
+    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];
+
+    const dilContent = 'DIL:\n  predicate#abc123\n    SCAN table\n    SELECT col1, col2';
+    executeCodeQLCommand.mockResolvedValueOnce({
+      stdout: dilContent,
+      stderr: 'compiled MyQuery.ql in 12ms',
+      success: true
+    });
+
+    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' });
+
+      // The DIL file path must still appear so the LLM can read the file.
+      expect(result.content[0].text).toContain('DIL file:');
+      // But the raw DIL stdout must NOT be echoed back inline.
+      expect(result.content[0].text).not.toContain(dilContent);
+      // Stderr (warnings/info) is still surfaced for diagnostic value.
+      expect(result.content[0].text).toContain('compiled MyQuery.ql in 12ms');
+    } 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 dump-dil is explicitly false for codeql_query_compile', async () => {
     const definition: CLIToolDefinition = {
       name: 'codeql_query_compile',
diff --git a/server/test/src/lib/log-directory-manager.test.ts b/server/test/src/lib/log-directory-manager.test.ts
@@ -98,9 +98,11 @@ describe('Log Directory Manager', () => {
     });
 
     it('should accept a valid logDir even when base path is a prefix of another path segment', () => {
-      // Regression: '/tmp/base-dir' must not be treated as inside '/tmp/base'
-      // (i.e. a simple startsWith('/tmp/base') check would accept '/tmp/base-dir').
-      // This is particularly relevant on Windows where drive-letter casing may differ.
+      // Regression: a sibling path like '<repo>/.tmp/base-dir' must not be
+      // treated as inside '<repo>/.tmp/base' (i.e. a simple
+      // startsWith('<repo>/.tmp/base') check would accept '<repo>/.tmp/base-dir').
+      // This is particularly relevant on Windows where drive-letter casing
+      // may differ between the resolved base and target paths.
       const similarPrefix = join(testBaseDir + '-sibling', 'sub');
 
       expect(() => getOrCreateLogDirectory(similarPrefix)).toThrow(

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,3 @@`
`1`	`1`	`module languages/go/tools/test/CallGraphFrom`
`2`	`2`
`3`		`-go 1.21.13`
	`3`	`+go 1.21`