Address latest PR review feedback

Author: data-douser

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

@@ -183829,12 +183829,13 @@ var SqliteStore = class _SqliteStore {
         truncated
       };
     } catch {
-      const fallback = this.getCacheContentSubset(cacheKey2, { maxLines: options.maxResults ?? 100 });
+      const FALLBACK_MAX_LINES = 500;
+      const fallback = this.getCacheContentSubset(cacheKey2, { maxLines: FALLBACK_MAX_LINES });
       if (!fallback) return null;
       return {
         content: fallback.content,
-        totalResults: fallback.totalLines,
-        returnedResults: fallback.returnedLines,
+        totalResults: 0,
+        returnedResults: 0,
         truncated: fallback.truncated
       };
     }
@@ -193434,7 +193435,7 @@ function registerAnnotationGetTool(server) {
     "annotation_get",
     "Retrieve a single annotation by its numeric ID.",
     {
-      id: external_exports.number().describe("The annotation ID.")
+      id: external_exports.number().int().positive().describe("The annotation ID (positive integer primary key).")
     },
     async ({ id }) => {
       const store = sessionDataManager.getStore();
@@ -193475,7 +193476,7 @@ function registerAnnotationUpdateTool(server) {
     "annotation_update",
     "Update the content, label, or metadata of an existing annotation.",
     {
-      id: external_exports.number().describe("The annotation ID to update."),
+      id: external_exports.number().int().positive().describe("The annotation ID to update (positive integer primary key)."),
       content: external_exports.string().optional().describe("New content (replaces existing)."),
       label: external_exports.string().optional().describe("New label (replaces existing)."),
       metadata: external_exports.string().optional().describe("New JSON-encoded metadata (replaces existing).")
@@ -193560,7 +193561,7 @@ function registerAuditStoreFindingsTool(server) {
       repo: external_exports.string().describe("Repository name."),
       findings: external_exports.array(external_exports.object({
         sourceLocation: external_exports.string().describe("File path of the finding."),
-        line: external_exports.number().describe("Line number of the finding."),
+        line: external_exports.number().int().min(1).describe("Line number of the finding (integer >= 1)."),
         sourceType: external_exports.string().describe('Type of the source (e.g. "RemoteFlowSource").'),
         description: external_exports.string().optional().describe("Human-readable description.")
       })).describe("Array of findings to store.")
@@ -193640,7 +193641,7 @@ function registerAuditAddNotesTool(server) {
       owner: external_exports.string().describe("Repository owner."),
       repo: external_exports.string().describe("Repository name."),
       sourceLocation: external_exports.string().describe("File path of the finding."),
-      line: external_exports.number().describe("Line number of the finding."),
+      line: external_exports.number().int().min(1).describe("Line number of the finding (integer >= 1)."),
       notes: external_exports.string().describe("Notes to append.")
     },
     async ({ owner, repo, sourceLocation, line, notes }) => {
@@ -193781,7 +193782,7 @@ function registerQueryResultsCacheRetrieveTool(server) {
               totalResults: subset2.totalResults,
               returnedResults: subset2.returnedResults,
               truncated: subset2.truncated,
-              results: parsedResults
+              sarifSubset: parsedResults
             }, null, 2)
           }]
         };

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

@@ -36,8 +36,9 @@ export type BuiltInEvaluator = keyof typeof BUILT_IN_EVALUATORS;
 /**
  * In-memory cache for extracted query metadata, keyed by file path.
  * Stores the file modification time to invalidate when the file changes.
- * Bounded to {@link METADATA_CACHE_MAX} entries; oldest entries are evicted
- * when the limit is reached (Map iteration order = insertion order).
+ * Bounded to {@link METADATA_CACHE_MAX} entries; least-recently-used entries
+ * (by access) are evicted when the limit is reached. Entries are refreshed
+ * on cache hits via delete+set, so Map iteration order reflects LRU state.
  */
 const METADATA_CACHE_MAX = 256;
 const metadataCache = new Map<string, { mtime: number; metadata: QueryMetadata }>();

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

@@ -772,12 +772,15 @@ export class SqliteStore {
         truncated,
       };
     } catch {
-      const fallback = this.getCacheContentSubset(cacheKey, { maxLines: options.maxResults ?? 100 });
+      // Cached content is not valid SARIF JSON; fall back to line-oriented
+      // retrieval with a dedicated line limit.
+      const FALLBACK_MAX_LINES = 500;
+      const fallback = this.getCacheContentSubset(cacheKey, { maxLines: FALLBACK_MAX_LINES });
       if (!fallback) return null;
       return {
         content: fallback.content,
-        totalResults: fallback.totalLines,
-        returnedResults: fallback.returnedLines,
+        totalResults: 0,
+        returnedResults: 0,
         truncated: fallback.truncated,
       };
     }

server/src/lib/sqlite-store.ts

@@ -67,7 +67,7 @@ function registerAnnotationGetTool(server: McpServer): void {
     'annotation_get',
     'Retrieve a single annotation by its numeric ID.',
     {
-      id: z.number().describe('The annotation ID.'),
+      id: z.number().int().positive().describe('The annotation ID (positive integer primary key).'),
     },
     async ({ id }) => {
       const store = sessionDataManager.getStore();
@@ -118,7 +118,7 @@ function registerAnnotationUpdateTool(server: McpServer): void {
     'annotation_update',
     'Update the content, label, or metadata of an existing annotation.',
     {
-      id: z.number().describe('The annotation ID to update.'),
+      id: z.number().int().positive().describe('The annotation ID to update (positive integer primary key).'),
       content: z.string().optional().describe('New content (replaces existing).'),
       label: z.string().optional().describe('New label (replaces existing).'),
       metadata: z.string().optional().describe('New JSON-encoded metadata (replaces existing).'),

server/src/tools/annotation-tools.ts

@@ -57,7 +57,7 @@ function registerAuditStoreFindingsTool(server: McpServer): void {
       repo: z.string().describe('Repository name.'),
       findings: z.array(z.object({
         sourceLocation: z.string().describe('File path of the finding.'),
-        line: z.number().describe('Line number of the finding.'),
+        line: z.number().int().min(1).describe('Line number of the finding (integer >= 1).'),
         sourceType: z.string().describe('Type of the source (e.g. "RemoteFlowSource").'),
         description: z.string().optional().describe('Human-readable description.'),
       })).describe('Array of findings to store.'),
@@ -152,7 +152,7 @@ function registerAuditAddNotesTool(server: McpServer): void {
       owner: z.string().describe('Repository owner.'),
       repo: z.string().describe('Repository name.'),
       sourceLocation: z.string().describe('File path of the finding.'),
-      line: z.number().describe('Line number of the finding.'),
+      line: z.number().int().min(1).describe('Line number of the finding (integer >= 1).'),
       notes: z.string().describe('Notes to append.'),
     },
     async ({ owner, repo, sourceLocation, line, notes }) => {

server/src/tools/audit-tools.ts

@@ -136,7 +136,7 @@ function registerQueryResultsCacheRetrieveTool(server: McpServer): void {
               totalResults: subset.totalResults,
               returnedResults: subset.returnedResults,
               truncated: subset.truncated,
-              results: parsedResults,
+              sarifSubset: parsedResults,
             }, null, 2),
           }],
         };

server/src/tools/cache-tools.ts

@@ -445,5 +445,27 @@ describe('SqliteStore', () => {
       expect(cleared).toBe(2);
       expect(store.listCacheEntries()).toHaveLength(0);
     });
+
+    it('should fall back to line-based retrieval for non-JSON SARIF content', () => {
+      const nonJsonContent = 'This is not valid JSON\nLine 2\nLine 3\nLine 4\nLine 5';
+      store.putCacheEntry({
+        cacheKey: 'bad-sarif',
+        queryName: 'Q',
+        queryPath: '/q.ql',
+        databasePath: '/db',
+        language: 'javascript',
+        codeqlVersion: '2.25.0',
+        outputFormat: 'sarif-latest',
+        resultContent: nonJsonContent,
+      });
+
+      const subset = store.getCacheSarifSubset('bad-sarif', {});
+      expect(subset).not.toBeNull();
+      // Fallback should report 0 for result counts since it's not valid SARIF
+      expect(subset!.totalResults).toBe(0);
+      expect(subset!.returnedResults).toBe(0);
+      // Content should still be returned (line-based fallback)
+      expect(subset!.content).toContain('This is not valid JSON');
+    });
   });
 });