Fixes for PR review feedback

Author: data-douser

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

@@ -182939,7 +182939,6 @@ async function resolveQueryPath(params, logger2) {
 
 // src/lib/result-processor.ts
 init_cli_executor();
-init_cli_executor();
 import { basename as basename4, dirname as dirname4 } from "path";
 import { mkdirSync as mkdirSync7, readFileSync as readFileSync6 } from "fs";
 import { createHash as createHash2 } from "crypto";
@@ -182965,6 +182964,8 @@ async function extractQueryMetadata(queryPath) {
       mtime = fstatSync(fd).mtimeMs;
       const cached2 = metadataCache.get(queryPath);
       if (cached2 && cached2.mtime === mtime) {
+        metadataCache.delete(queryPath);
+        metadataCache.set(queryPath, cached2);
         return cached2.metadata;
       }
       queryContent = readFileSync4(fd, "utf-8");
@@ -183402,8 +183403,13 @@ var SqliteStore = class _SqliteStore {
   /**
    * Write the in-memory database to disk.
    *
-   * Uses write-to-temp + atomic rename so a crash mid-write cannot
-   * corrupt the database file.
+   * On platforms where `renameSync` can atomically replace the destination
+   * (for example, POSIX filesystems and Windows when the target is not
+   * locked), this uses a write-to-temp + atomic rename pattern so a crash
+   * mid-write cannot corrupt the existing database file. On some Windows
+   * configurations where `renameSync` fails because the destination is
+   * locked, we fall back to a direct overwrite, which is best-effort only
+   * and not fully crash-safe.
    */
   flush() {
     if (this.flushTimer) {
@@ -183447,6 +183453,10 @@ var SqliteStore = class _SqliteStore {
    * Close the database (and flush remaining changes).
    */
   close() {
+    if (this.flushTimer) {
+      globalThis.clearTimeout(this.flushTimer);
+      this.flushTimer = null;
+    }
     this.flushIfDirty();
     this.db?.close();
     this.db = null;
@@ -183582,11 +183592,11 @@ var SqliteStore = class _SqliteStore {
       sql += " WHERE " + conditions.join(" AND ");
     }
     sql += " ORDER BY updated_at DESC";
-    if (filter?.limit) {
+    if (filter?.limit !== void 0) {
       sql += " LIMIT $limit";
       params.$limit = filter.limit;
     }
-    if (filter?.offset) {
+    if (filter?.offset !== void 0) {
       sql += " OFFSET $offset";
       params.$offset = filter.offset;
     }
@@ -193731,11 +193741,11 @@ function registerQueryResultsCacheRetrieveTool(server) {
     "Retrieve cached query results with optional subset selection. Supports line ranges (for graphtext/CSV) and SARIF result indices and file filtering to return only the relevant portion.",
     {
       cacheKey: external_exports.string().describe("The cache key of the result to retrieve."),
-      lineRange: external_exports.tuple([external_exports.number(), external_exports.number()]).optional().describe("Line range [start, end] (1-indexed, inclusive). For graphtext/CSV output only."),
-      resultIndices: external_exports.tuple([external_exports.number(), external_exports.number()]).optional().describe("SARIF result index range [start, end] (0-indexed, inclusive). For SARIF output only."),
+      lineRange: external_exports.tuple([external_exports.number().int().min(1), external_exports.number().int().min(1)]).refine(([start, end]) => start <= end, { message: "lineRange start must be <= end" }).optional().describe("Line range [start, end] (1-indexed, inclusive). For graphtext/CSV output only."),
+      resultIndices: external_exports.tuple([external_exports.number().int().min(0), external_exports.number().int().min(0)]).refine(([start, end]) => start <= end, { message: "resultIndices start must be <= end" }).optional().describe("SARIF result index range [start, end] (0-indexed, inclusive). For SARIF output only."),
       fileFilter: external_exports.string().optional().describe("For SARIF: only include results whose file path contains this string."),
-      maxLines: external_exports.number().optional().describe("Maximum number of lines to return for line-based formats (default: 500)."),
-      maxResults: external_exports.number().optional().describe("Maximum number of SARIF results to return (default: 100).")
+      maxLines: external_exports.number().int().positive().optional().describe("Maximum number of lines to return for line-based formats (default: 500)."),
+      maxResults: external_exports.number().int().positive().optional().describe("Maximum number of SARIF results to return (default: 100).")
     },
     async ({ cacheKey: cacheKey2, lineRange, resultIndices, fileFilter, maxLines, maxResults }) => {
       const store = sessionDataManager.getStore();

server/src/lib/query-results-evaluator.ts

@@ -56,6 +56,9 @@ export async function extractQueryMetadata(queryPath: string): Promise<QueryMeta
       mtime = fstatSync(fd).mtimeMs;
       const cached = metadataCache.get(queryPath);
       if (cached && cached.mtime === mtime) {
+        // Refresh position in Map to implement true LRU behavior.
+        metadataCache.delete(queryPath);
+        metadataCache.set(queryPath, cached);
         return cached.metadata;
       }
       queryContent = readFileSync(fd, 'utf-8');

server/src/lib/result-processor.ts

@@ -9,8 +9,7 @@
 import { basename, dirname } from 'path';
 import { mkdirSync, readFileSync } from 'fs';
 import { createHash } from 'crypto';
-import { executeCodeQLCommand, CLIExecutionResult } from './cli-executor';
-import { getActualCodeqlVersion } from './cli-executor';
+import { CLIExecutionResult, executeCodeQLCommand, getActualCodeqlVersion } from './cli-executor';
 import { evaluateQueryResults, extractQueryMetadata, QueryEvaluationResult } from './query-results-evaluator';
 import { resolveQueryPath } from './query-resolver';
 import { sessionDataManager } from './session-data-manager';

server/src/lib/sqlite-store.ts

@@ -252,8 +252,13 @@ export class SqliteStore {
   /**
    * Write the in-memory database to disk.
    *
-   * Uses write-to-temp + atomic rename so a crash mid-write cannot
-   * corrupt the database file.
+   * On platforms where `renameSync` can atomically replace the destination
+   * (for example, POSIX filesystems and Windows when the target is not
+   * locked), this uses a write-to-temp + atomic rename pattern so a crash
+   * mid-write cannot corrupt the existing database file. On some Windows
+   * configurations where `renameSync` fails because the destination is
+   * locked, we fall back to a direct overwrite, which is best-effort only
+   * and not fully crash-safe.
    */
   flush(): void {
     if (this.flushTimer) {
@@ -299,6 +304,10 @@ export class SqliteStore {
    * Close the database (and flush remaining changes).
    */
   close(): void {
+    if (this.flushTimer) {
+      globalThis.clearTimeout(this.flushTimer);
+      this.flushTimer = null;
+    }
     this.flushIfDirty();
     this.db?.close();
     this.db = null;
@@ -458,11 +467,11 @@ export class SqliteStore {
     }
     sql += ' ORDER BY updated_at DESC';
 
-    if (filter?.limit) {
+    if (filter?.limit !== undefined) {
       sql += ' LIMIT $limit';
       params.$limit = filter.limit;
     }
-    if (filter?.offset) {
+    if (filter?.offset !== undefined) {
       sql += ' OFFSET $offset';
       params.$offset = filter.offset;
     }

server/src/tools/cache-tools.ts

@@ -83,11 +83,19 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {
     'Retrieve cached query results with optional subset selection. Supports line ranges (for graphtext/CSV) and SARIF result indices and file filtering to return only the relevant portion.',
     {
       cacheKey: z.string().describe('The cache key of the result to retrieve.'),
-      lineRange: z.tuple([z.number(), z.number()]).optional().describe('Line range [start, end] (1-indexed, inclusive). For graphtext/CSV output only.'),
-      resultIndices: z.tuple([z.number(), z.number()]).optional().describe('SARIF result index range [start, end] (0-indexed, inclusive). For SARIF output only.'),
+      lineRange: z
+        .tuple([z.number().int().min(1), z.number().int().min(1)])
+        .refine(([start, end]) => start <= end, { message: 'lineRange start must be <= end' })
+        .optional()
+        .describe('Line range [start, end] (1-indexed, inclusive). For graphtext/CSV output only.'),
+      resultIndices: z
+        .tuple([z.number().int().min(0), z.number().int().min(0)])
+        .refine(([start, end]) => start <= end, { message: 'resultIndices start must be <= end' })
+        .optional()
+        .describe('SARIF result index range [start, end] (0-indexed, inclusive). For SARIF output only.'),
       fileFilter: z.string().optional().describe('For SARIF: only include results whose file path contains this string.'),
-      maxLines: z.number().optional().describe('Maximum number of lines to return for line-based formats (default: 500).'),
-      maxResults: z.number().optional().describe('Maximum number of SARIF results to return (default: 100).'),
+      maxLines: z.number().int().positive().optional().describe('Maximum number of lines to return for line-based formats (default: 500).'),
+      maxResults: z.number().int().positive().optional().describe('Maximum number of SARIF results to return (default: 100).'),
     },
     async ({ cacheKey, lineRange, resultIndices, fileFilter, maxLines, maxResults }) => {
       const store = sessionDataManager.getStore();