Address PR #169 review: FTS4 search, FD leak fix, Windows renameSync fallback, JSON.parse fallback

Copilot · data-douser · data-douser · commit 0e52b34ac6d2 · 2026-03-27T14:31:54.000-06:00
Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/7ff5a5d7-9ac1-4964-8b54-e233d7253301 Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>
diff --git a/server/dist/codeql-development-mcp-server.js b/server/dist/codeql-development-mcp-server.js
@@ -184855,9 +184855,8 @@ import { createHash as createHash2 } from "crypto";
 // src/lib/query-results-evaluator.ts
 init_cli_executor();
 init_logger();
-import { fstatSync, openSync, readFileSync as readFileSync4, writeFileSync } from "fs";
+import { closeSync, fstatSync, mkdirSync as mkdirSync4, openSync, readFileSync as readFileSync4, writeFileSync } from "fs";
 import { dirname as dirname3, isAbsolute as isAbsolute3 } from "path";
-import { mkdirSync as mkdirSync4 } from "fs";
 var BUILT_IN_EVALUATORS = {
   "json-decode": "JSON format decoder for query results",
   "csv-decode": "CSV format decoder for query results",
@@ -184868,12 +184867,18 @@ var metadataCache = /* @__PURE__ */ new Map();
 async function extractQueryMetadata(queryPath) {
   try {
     const fd = openSync(queryPath, "r");
-    const mtime = fstatSync(fd).mtimeMs;
-    const cached2 = metadataCache.get(queryPath);
-    if (cached2 && cached2.mtime === mtime) {
-      return cached2.metadata;
+    let queryContent;
+    let mtime;
+    try {
+      mtime = fstatSync(fd).mtimeMs;
+      const cached2 = metadataCache.get(queryPath);
+      if (cached2 && cached2.mtime === mtime) {
+        return cached2.metadata;
+      }
+      queryContent = readFileSync4(fd, "utf-8");
+    } finally {
+      closeSync(fd);
     }
-    const queryContent = readFileSync4(fd, "utf-8");
     const metadata = {};
     const kindMatch = queryContent.match(/@kind\s+([^\s]+)/);
     if (kindMatch) metadata.kind = kindMatch[1];
@@ -185134,7 +185139,7 @@ import { randomUUID as randomUUID2 } from "crypto";
 // src/lib/sqlite-store.ts
 var import_sql_asm = __toESM(require_sql_asm(), 1);
 init_logger();
-import { mkdirSync as mkdirSync5, readFileSync as readFileSync5, renameSync, writeFileSync as writeFileSync2 } from "fs";
+import { mkdirSync as mkdirSync5, readFileSync as readFileSync5, renameSync, unlinkSync, writeFileSync as writeFileSync2 } from "fs";
 import { join as join8 } from "path";
 var SqliteStore = class _SqliteStore {
   db = null;
@@ -185202,6 +185207,32 @@ var SqliteStore = class _SqliteStore {
       CREATE INDEX IF NOT EXISTS idx_annotations_category_entity
         ON annotations (category, entity_key);
     `);
+    this.exec(`
+      CREATE VIRTUAL TABLE IF NOT EXISTS annotations_fts
+        USING fts4(tokenize=unicode61, content, label, metadata);
+    `);
+    this.exec(`
+      CREATE TRIGGER IF NOT EXISTS annotations_ai
+        AFTER INSERT ON annotations BEGIN
+          INSERT INTO annotations_fts(rowid, content, label, metadata)
+            VALUES (new.id, new.content, new.label, new.metadata);
+        END;
+    `);
+    this.exec(`
+      CREATE TRIGGER IF NOT EXISTS annotations_ad
+        AFTER DELETE ON annotations BEGIN
+          DELETE FROM annotations_fts WHERE rowid = old.id;
+        END;
+    `);
+    this.exec(`
+      CREATE TRIGGER IF NOT EXISTS annotations_au
+        AFTER UPDATE ON annotations BEGIN
+          DELETE FROM annotations_fts WHERE rowid = old.id;
+          INSERT INTO annotations_fts(rowid, content, label, metadata)
+            VALUES (new.id, new.content, new.label, new.metadata);
+        END;
+    `);
+    this.backfillAnnotationsFts();
     this.exec(`
       CREATE TABLE IF NOT EXISTS query_result_cache (
         id INTEGER PRIMARY KEY AUTOINCREMENT,
@@ -185250,6 +185281,24 @@ var SqliteStore = class _SqliteStore {
     }
     this.dirty = true;
   }
+  /**
+   * Backfill the FTS index for any annotations rows that were inserted before
+   * the FTS table existed (schema migration).  Compares row counts and rebuilds
+   * the entire FTS index when there is a mismatch.
+   */
+  backfillAnnotationsFts() {
+    const db = this.ensureDb();
+    const ftsCountResult = db.exec("SELECT COUNT(*) FROM annotations_fts");
+    const annCountResult = db.exec("SELECT COUNT(*) FROM annotations");
+    const ftsCount = ftsCountResult[0]?.values[0][0] ?? 0;
+    const annCount = annCountResult[0]?.values[0][0] ?? 0;
+    if (ftsCount < annCount) {
+      db.run("DELETE FROM annotations_fts");
+      db.run(
+        "INSERT INTO annotations_fts(rowid, content, label, metadata) SELECT id, content, label, metadata FROM annotations"
+      );
+    }
+  }
   /**
    * Get the number of rows modified by the last INSERT/UPDATE/DELETE.
    */
@@ -185274,7 +185323,15 @@ var SqliteStore = class _SqliteStore {
     const buffer = Buffer.from(data);
     const tmpPath = this.dbPath + ".tmp";
     writeFileSync2(tmpPath, buffer);
-    renameSync(tmpPath, this.dbPath);
+    try {
+      renameSync(tmpPath, this.dbPath);
+    } catch {
+      writeFileSync2(this.dbPath, buffer);
+      try {
+        unlinkSync(tmpPath);
+      } catch {
+      }
+    }
     this.dirty = false;
   }
   /**
@@ -185425,8 +185482,8 @@ var SqliteStore = class _SqliteStore {
       params.$entity_key_prefix = filter.entityKeyPrefix + "%";
     }
     if (filter?.search) {
-      conditions.push("(content LIKE $search OR metadata LIKE $search OR label LIKE $search)");
-      params.$search = "%" + filter.search + "%";
+      conditions.push("id IN (SELECT rowid FROM annotations_fts WHERE annotations_fts MATCH $search)");
+      params.$search = filter.search;
     }
     let sql = "SELECT * FROM annotations";
     if (conditions.length > 0) {
@@ -192094,7 +192151,7 @@ var codeqlResolveTestsTool = {
 };
 
 // src/tools/codeql/search-ql-code.ts
-import { closeSync, createReadStream as createReadStream3, fstatSync as fstatSync2, lstatSync, openSync as openSync2, readdirSync as readdirSync8, readFileSync as readFileSync12, realpathSync } from "fs";
+import { closeSync as closeSync2, createReadStream as createReadStream3, fstatSync as fstatSync2, lstatSync, openSync as openSync2, readdirSync as readdirSync8, readFileSync as readFileSync12, realpathSync } from "fs";
 import { basename as basename8, extname as extname2, join as join19, resolve as resolve9 } from "path";
 import { createInterface as createInterface3 } from "readline";
 init_logger();
@@ -192160,7 +192217,7 @@ async function searchFile(filePath, regex, contextLines, maxCollect) {
     size = fstatSync2(fd).size;
   } catch {
     try {
-      closeSync(fd);
+      closeSync2(fd);
     } catch {
     }
     return { matches: [], totalCount: 0 };
@@ -192173,13 +192230,13 @@ async function searchFile(filePath, regex, contextLines, maxCollect) {
     content = readFileSync12(fd, "utf-8");
   } catch {
     try {
-      closeSync(fd);
+      closeSync2(fd);
     } catch {
     }
     return { matches: [], totalCount: 0 };
   }
   try {
-    closeSync(fd);
+    closeSync2(fd);
   } catch {
   }
   const lines = content.replace(/\r\n/g, "\n").split("\n");
@@ -192227,7 +192284,7 @@ async function searchFileStreaming(filePath, regex, contextLines, maxCollect, fd
   } catch {
     if (fd !== void 0) {
       try {
-        closeSync(fd);
+        closeSync2(fd);
       } catch {
       }
     }
@@ -195606,14 +195663,25 @@ function registerQueryResultsCacheRetrieveTool(server) {
         if (!subset2) {
           return { content: [{ type: "text", text: `Cached content not available for key: ${cacheKey2}` }] };
         }
+        let parsedResults;
+        try {
+          parsedResults = JSON.parse(subset2.content);
+        } catch {
+          return {
+            content: [{
+              type: "text",
+              text: subset2.content
+            }]
+          };
+        }
         return {
           content: [{
             type: "text",
             text: JSON.stringify({
               totalResults: subset2.totalResults,
               returnedResults: subset2.returnedResults,
               truncated: subset2.truncated,
-              results: JSON.parse(subset2.content)
+              results: parsedResults
             }, null, 2)
           }]
         };
diff --git a/server/dist/codeql-development-mcp-server.js.map b/server/dist/codeql-development-mcp-server.js.map
diff --git a/server/src/lib/query-results-evaluator.ts b/server/src/lib/query-results-evaluator.ts
@@ -4,9 +4,8 @@
 
 import { executeCodeQLCommand } from './cli-executor';
 import { logger } from '../utils/logger';
-import { fstatSync, openSync, readFileSync, writeFileSync } from 'fs';
+import { closeSync, fstatSync, mkdirSync, openSync, readFileSync, writeFileSync } from 'fs';
 import { dirname, isAbsolute } from 'path';
-import { mkdirSync } from 'fs';
 
 export interface QueryEvaluationResult {
   success: boolean;
@@ -51,13 +50,18 @@ export async function extractQueryMetadata(queryPath: string): Promise<QueryMeta
   try {
     // Open once, then fstat + read via the fd to avoid TOCTOU race (CWE-367).
     const fd = openSync(queryPath, 'r');
-    const mtime = fstatSync(fd).mtimeMs;
-    const cached = metadataCache.get(queryPath);
-    if (cached && cached.mtime === mtime) {
-      return cached.metadata;
+    let queryContent: string;
+    let mtime: number;
+    try {
+      mtime = fstatSync(fd).mtimeMs;
+      const cached = metadataCache.get(queryPath);
+      if (cached && cached.mtime === mtime) {
+        return cached.metadata;
+      }
+      queryContent = readFileSync(fd, 'utf-8');
+    } finally {
+      closeSync(fd);
     }
-
-    const queryContent = readFileSync(fd, 'utf-8');
     const metadata: QueryMetadata = {};
 
     // Extract metadata from comments using regex patterns
diff --git a/server/src/lib/sqlite-store.ts b/server/src/lib/sqlite-store.ts
@@ -13,7 +13,7 @@
 
 import initSqlJs from 'sql.js/dist/sql-asm.js';
 import type { Database as SqlJsDatabase } from 'sql.js';
-import { mkdirSync, readFileSync, renameSync, writeFileSync } from 'fs';
+import { mkdirSync, readFileSync, renameSync, unlinkSync, writeFileSync } from 'fs';
 import { join } from 'path';
 import { logger } from '../utils/logger';
 
@@ -129,6 +129,41 @@ export class SqliteStore {
         ON annotations (category, entity_key);
     `);
 
+    // FTS4 virtual table for full-text search over annotation text fields.
+    // The unicode61 tokenizer provides case-insensitive, accent-insensitive matching.
+    this.exec(`
+      CREATE VIRTUAL TABLE IF NOT EXISTS annotations_fts
+        USING fts4(tokenize=unicode61, content, label, metadata);
+    `);
+
+    // Triggers keep the FTS index in sync with the annotations table.
+    this.exec(`
+      CREATE TRIGGER IF NOT EXISTS annotations_ai
+        AFTER INSERT ON annotations BEGIN
+          INSERT INTO annotations_fts(rowid, content, label, metadata)
+            VALUES (new.id, new.content, new.label, new.metadata);
+        END;
+    `);
+
+    this.exec(`
+      CREATE TRIGGER IF NOT EXISTS annotations_ad
+        AFTER DELETE ON annotations BEGIN
+          DELETE FROM annotations_fts WHERE rowid = old.id;
+        END;
+    `);
+
+    this.exec(`
+      CREATE TRIGGER IF NOT EXISTS annotations_au
+        AFTER UPDATE ON annotations BEGIN
+          DELETE FROM annotations_fts WHERE rowid = old.id;
+          INSERT INTO annotations_fts(rowid, content, label, metadata)
+            VALUES (new.id, new.content, new.label, new.metadata);
+        END;
+    `);
+
+    // Migration: backfill FTS for any existing rows that pre-date the FTS table.
+    this.backfillAnnotationsFts();
+
     this.exec(`
       CREATE TABLE IF NOT EXISTS query_result_cache (
         id INTEGER PRIMARY KEY AUTOINCREMENT,
@@ -184,6 +219,27 @@ export class SqliteStore {
     this.dirty = true;
   }
 
+  /**
+   * Backfill the FTS index for any annotations rows that were inserted before
+   * the FTS table existed (schema migration).  Compares row counts and rebuilds
+   * the entire FTS index when there is a mismatch.
+   */
+  private backfillAnnotationsFts(): void {
+    const db = this.ensureDb();
+    const ftsCountResult = db.exec('SELECT COUNT(*) FROM annotations_fts');
+    const annCountResult = db.exec('SELECT COUNT(*) FROM annotations');
+    const ftsCount = (ftsCountResult[0]?.values[0][0] as number) ?? 0;
+    const annCount = (annCountResult[0]?.values[0][0] as number) ?? 0;
+
+    if (ftsCount < annCount) {
+      // Clear and fully rebuild to avoid duplicate entries.
+      db.run('DELETE FROM annotations_fts');
+      db.run(
+        'INSERT INTO annotations_fts(rowid, content, label, metadata) SELECT id, content, label, metadata FROM annotations',
+      );
+    }
+  }
+
   /**
    * Get the number of rows modified by the last INSERT/UPDATE/DELETE.
    */
@@ -209,7 +265,14 @@ export class SqliteStore {
     const buffer = Buffer.from(data);
     const tmpPath = this.dbPath + '.tmp';
     writeFileSync(tmpPath, buffer);
-    renameSync(tmpPath, this.dbPath);
+    try {
+      renameSync(tmpPath, this.dbPath);
+    } catch {
+      // On some Windows configurations renameSync can fail if the destination
+      // is locked; fall back to a direct overwrite and clean up the temp file.
+      writeFileSync(this.dbPath, buffer);
+      try { unlinkSync(tmpPath); } catch { /* ignore cleanup failure */ }
+    }
     this.dirty = false;
   }
 
@@ -383,8 +446,10 @@ export class SqliteStore {
       params.$entity_key_prefix = filter.entityKeyPrefix + '%';
     }
     if (filter?.search) {
-      conditions.push('(content LIKE $search OR metadata LIKE $search OR label LIKE $search)');
-      params.$search = '%' + filter.search + '%';
+      // Use the FTS4 index for efficient, case-insensitive full-text search
+      // across content, label, and metadata fields.
+      conditions.push('id IN (SELECT rowid FROM annotations_fts WHERE annotations_fts MATCH $search)');
+      params.$search = filter.search;
     }
 
     let sql = 'SELECT * FROM annotations';
diff --git a/server/src/tools/cache-tools.ts b/server/src/tools/cache-tools.ts
@@ -109,14 +109,26 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {
         if (!subset) {
           return { content: [{ type: 'text' as const, text: `Cached content not available for key: ${cacheKey}` }] };
         }
+        let parsedResults: unknown;
+        try {
+          parsedResults = JSON.parse(subset.content);
+        } catch {
+          // getCacheSarifSubset fell back to plain-text content; return it as-is.
+          return {
+            content: [{
+              type: 'text' as const,
+              text: subset.content,
+            }],
+          };
+        }
         return {
           content: [{
             type: 'text' as const,
             text: JSON.stringify({
               totalResults: subset.totalResults,
               returnedResults: subset.returnedResults,
               truncated: subset.truncated,
-              results: JSON.parse(subset.content),
+              results: parsedResults,
             }, null, 2),
           }],
         };
diff --git a/server/test/src/lib/sqlite-store.test.ts b/server/test/src/lib/sqlite-store.test.ts
@@ -118,7 +118,7 @@ describe('SqliteStore', () => {
       expect(results[0].content).toBe('yes');
     });
 
-    it('should search annotations by content', () => {
+    it('should full-text search annotations across content, label, and metadata', () => {
       store.createAnnotation('note', 'a', 'Found a vulnerability in auth');
       store.createAnnotation('note', 'b', 'No issues here');
       store.createAnnotation('note', 'c', null, null, '{"detail":"vulnerability in parser"}');
@@ -127,6 +127,18 @@ describe('SqliteStore', () => {
       expect(results).toHaveLength(2);
     });
 
+    it('should perform case-insensitive full-text search', () => {
+      store.createAnnotation('note', 'x', 'SQL injection risk');
+      store.createAnnotation('note', 'y', 'No problems');
+
+      const lower = store.listAnnotations({ search: 'sql' });
+      const upper = store.listAnnotations({ search: 'SQL' });
+      const mixed = store.listAnnotations({ search: 'Sql' });
+      expect(lower).toHaveLength(1);
+      expect(upper).toHaveLength(1);
+      expect(mixed).toHaveLength(1);
+    });
+
     it('should support limit and offset', () => {
       for (let i = 0; i < 10; i++) {
         store.createAnnotation('note', `key-${i}`, `content ${i}`);
