fix: address unresolved PR review comments

- Fix deletion-only hunk misclassification in line-level granularity by
  adding hunksParsed flag to DiffFileEntry; parseGitDiffOutput sets it
  when @@ headers are seen, and diffSarifByCommits uses it to distinguish
  "no hunk info" from "deletion-only" diffs
- Precompute normalized diff paths once before the results loop, removing
  the unused diffPathMatchesSarifUri wrapper
- Migrate all params_test.go from t.TempDir() to project-local .tmp/
- Add regression tests for deletion-only diffs in unit and handler tests

Author: data-douser

client/internal/testing/params_test.go

@@ -39,7 +39,7 @@ func projectTmpDir(t *testing.T, name string) string {
 
 func TestBuildToolParams_TestConfig(t *testing.T) {
 	// Create a temp test fixture with test-config.json
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "test-config")
 	testDir := filepath.Join(dir, "tools", "my_tool", "my_test")
 	os.MkdirAll(filepath.Join(testDir, "before"), 0o755)
 	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
@@ -56,7 +56,7 @@ func TestBuildToolParams_TestConfig(t *testing.T) {
 }
 
 func TestBuildToolParams_MonitoringStateParams(t *testing.T) {
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "monitoring-state")
 	testDir := filepath.Join(dir, "tools", "codeql_lsp_completion", "basic")
 	os.MkdirAll(filepath.Join(testDir, "before"), 0o755)
 	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
@@ -77,7 +77,7 @@ func TestBuildToolParams_MonitoringStateParams(t *testing.T) {
 }
 
 func TestBuildToolParams_ValidateCodeqlQuery(t *testing.T) {
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "validate-query")
 	testDir := filepath.Join(dir, "tools", "validate_codeql_query", "syntax_validation")
 	os.MkdirAll(filepath.Join(testDir, "before"), 0o755)
 	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
@@ -97,7 +97,7 @@ func TestBuildToolParams_ValidateCodeqlQuery(t *testing.T) {
 }
 
 func TestBuildToolParams_ResolveQueries_UsesDirectoryKey(t *testing.T) {
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "resolve-queries")
 	testDir := filepath.Join(dir, "tools", "codeql_resolve_queries", "resolve_queries")
 	os.MkdirAll(filepath.Join(testDir, "before"), 0o755)
 	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
@@ -116,7 +116,7 @@ func TestBuildToolParams_ResolveQueries_UsesDirectoryKey(t *testing.T) {
 }
 
 func TestBuildToolParams_ResolveLanguages(t *testing.T) {
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "resolve-languages")
 	testDir := filepath.Join(dir, "tools", "codeql_resolve_languages", "list_languages")
 	os.MkdirAll(filepath.Join(testDir, "before"), 0o755)
 	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
@@ -134,7 +134,7 @@ func TestBuildToolParams_ResolveLanguages(t *testing.T) {
 }
 
 func TestBuildToolParams_UnknownTool(t *testing.T) {
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "unknown-tool")
 	testDir := filepath.Join(dir, "tools", "unknown_tool_xyz", "test1")
 	os.MkdirAll(filepath.Join(testDir, "before"), 0o755)
 	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
@@ -148,7 +148,7 @@ func TestBuildToolParams_UnknownTool(t *testing.T) {
 }
 
 func TestFindFilesByExt(t *testing.T) {
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "find-files")
 	os.WriteFile(filepath.Join(dir, "a.ql"), []byte(""), 0o600)
 	os.WriteFile(filepath.Join(dir, "b.ql"), []byte(""), 0o600)
 	os.WriteFile(filepath.Join(dir, "c.txt"), []byte(""), 0o600)
@@ -189,7 +189,7 @@ func TestIsSARIFTool(t *testing.T) {
 }
 
 func TestBuildToolParams_SARIFToolWithConfig(t *testing.T) {
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "sarif-tool-config")
 	testDir := filepath.Join(dir, "tools", "sarif_extract_rule", "extract_sql_injection")
 	beforeDir := filepath.Join(testDir, "before")
 	os.MkdirAll(beforeDir, 0o755)
@@ -221,7 +221,7 @@ func TestBuildToolParams_SARIFToolWithConfig(t *testing.T) {
 }
 
 func TestBuildToolParams_SARIFCompareAlertsWithConfig(t *testing.T) {
-	dir := t.TempDir()
+	dir := projectTmpDir(t, "sarif-compare-alerts")
 	testDir := filepath.Join(dir, "tools", "sarif_compare_alerts", "sink_overlap")
 	beforeDir := filepath.Join(testDir, "before")
 	os.MkdirAll(beforeDir, 0o755)

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

@@ -189516,9 +189516,6 @@ function findOverlappingAlerts(resultsA, ruleA, resultsB, ruleB, mode = "sink")
   }
   return overlaps;
 }
-function diffPathMatchesSarifUri(diffPath, sarifUri) {
-  return urisMatch(diffPath, sarifUri);
-}
 function lineInHunks(line, hunks) {
   for (const hunk of hunks) {
     const hunkEnd = hunk.startLine + Math.max(hunk.lineCount - 1, 0);
@@ -189532,6 +189529,10 @@ function diffSarifByCommits(sarif, diffFiles, refRange, granularity = "file") {
   const results = sarif.runs[0]?.results ?? [];
   const newResults = [];
   const preExistingResults = [];
+  const normalizedDiffEntries = diffFiles.map((df) => ({
+    entry: df,
+    normalized: normalizeUri(df.path)
+  }));
   for (let i = 0; i < results.length; i++) {
     const result = results[i];
     const primaryLoc = result.locations?.[0]?.physicalLocation;
@@ -189545,19 +189546,26 @@ function diffSarifByCommits(sarif, diffFiles, refRange, granularity = "file") {
       continue;
     }
     const startLine = primaryLoc?.region?.startLine;
-    const matchingDiff = diffFiles.find((df) => diffPathMatchesSarifUri(df.path, uri));
+    const normalizedUri = normalizeUri(uri);
+    let matchingDiff;
+    for (const { entry, normalized } of normalizedDiffEntries) {
+      if (normalized === normalizedUri || normalized.endsWith(normalizedUri) || normalizedUri.endsWith(normalized)) {
+        matchingDiff = entry;
+        break;
+      }
+    }
     let isNew = false;
     if (matchingDiff) {
       if (granularity === "file") {
         isNew = true;
       } else if (startLine !== void 0 && matchingDiff.hunks.length > 0) {
         isNew = lineInHunks(startLine, matchingDiff.hunks);
-      } else if (matchingDiff.hunks.length === 0) {
+      } else if (matchingDiff.hunks.length === 0 && !matchingDiff.hunksParsed) {
         isNew = true;
       }
     }
     const classified = {
-      file: matchingDiff ? matchingDiff.path : normalizeUri(uri),
+      file: matchingDiff ? matchingDiff.path : normalizedUri,
       line: startLine,
       resultIndex: i,
       ruleId: result.ruleId
@@ -201173,6 +201181,7 @@ function parseGitDiffOutput(diffOutput) {
     if (currentFile && line.startsWith("@@")) {
       const match = line.match(/@@ [^ ]+ \+(\d+)(?:,(\d+))? @@/);
       if (match) {
+        currentFile.hunksParsed = true;
         const startLine = parseInt(match[1], 10);
         const lineCount = match[2] !== void 0 ? parseInt(match[2], 10) : 1;
         if (lineCount > 0) {

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

@@ -89,8 +89,14 @@ export interface SarifDiffResult {
 
 /** A file changed in a git diff, with optional line ranges. */
 export interface DiffFileEntry {
-  /** Changed line ranges (hunks). Empty array means file-level only. */
+  /** Changed line ranges (hunks). Only includes hunks with lineCount > 0 (additions). */
   hunks: Array<{ startLine: number; lineCount: number }>;
+  /**
+   * Whether hunk headers were parsed for this file. When true and hunks is empty,
+   * the diff contained only deletions (no new lines). When false/undefined and
+   * hunks is empty, no hunk information was available (file-level match only).
+   */
+  hunksParsed?: boolean;
   /** File path relative to the repository root. */
   path: string;
 }
@@ -863,18 +869,6 @@ export function findOverlappingAlerts(
 // SARIF-to-git-diff correlation
 // ---------------------------------------------------------------------------
 
-/**
- * Check whether a SARIF URI matches a diff file path.
- *
- * SARIF URIs may be absolute (`file:///…`) or relative (`src/db.js`),
- * while git diff paths are always relative to the repo root (e.g. `src/db.js`).
- * We normalize both and use suffix matching so that cross-environment
- * comparisons work (e.g. CI vs local).
- */
-function diffPathMatchesSarifUri(diffPath: string, sarifUri: string): boolean {
-  return urisMatch(diffPath, sarifUri);
-}
-
 /**
  * Check whether a line number falls within any of a file's changed hunks.
  */
@@ -912,6 +906,13 @@ export function diffSarifByCommits(
   const newResults: ClassifiedResult[] = [];
   const preExistingResults: ClassifiedResult[] = [];
 
+  // Precompute normalized diff paths once to avoid re-normalizing on every
+  // result iteration (O(results) normalize calls instead of O(results × diffFiles)).
+  const normalizedDiffEntries = diffFiles.map(df => ({
+    entry: df,
+    normalized: normalizeUri(df.path),
+  }));
+
   for (let i = 0; i < results.length; i++) {
     const result = results[i];
     const primaryLoc = result.locations?.[0]?.physicalLocation;
@@ -928,25 +929,34 @@ export function diffSarifByCommits(
     }
 
     const startLine = primaryLoc?.region?.startLine;
-
-    // Find matching diff file
-    const matchingDiff = diffFiles.find(df => diffPathMatchesSarifUri(df.path, uri));
+    const normalizedUri = normalizeUri(uri);
+
+    // Find matching diff file using precomputed normalized paths
+    let matchingDiff: DiffFileEntry | undefined;
+    for (const { entry, normalized } of normalizedDiffEntries) {
+      if (normalized === normalizedUri || normalized.endsWith(normalizedUri) || normalizedUri.endsWith(normalized)) {
+        matchingDiff = entry;
+        break;
+      }
+    }
 
     let isNew = false;
     if (matchingDiff) {
       if (granularity === 'file') {
         isNew = true;
       } else if (startLine !== undefined && matchingDiff.hunks.length > 0) {
         isNew = lineInHunks(startLine, matchingDiff.hunks);
-      } else if (matchingDiff.hunks.length === 0) {
-        // No hunk info available — treat as file-level match
+      } else if (matchingDiff.hunks.length === 0 && !matchingDiff.hunksParsed) {
+        // No hunk info available at all — treat as file-level match
         isNew = true;
       }
-      // else: line granularity requested but no startLine on the result → pre-existing
+      // else: line granularity but hunksParsed=true with empty hunks means
+      // deletion-only diff (no new lines) → pre-existing.
+      // Or: line granularity requested but no startLine on the result → pre-existing.
     }
 
     const classified: ClassifiedResult = {
-      file: matchingDiff ? matchingDiff.path : normalizeUri(uri),
+      file: matchingDiff ? matchingDiff.path : normalizedUri,
       line: startLine,
       resultIndex: i,
       ruleId: result.ruleId,

server/src/lib/sarif-utils.ts

@@ -347,6 +347,7 @@ function parseGitDiffOutput(diffOutput: string): DiffFileEntry[] {
     if (currentFile && line.startsWith('@@')) {
       const match = line.match(/@@ [^ ]+ \+(\d+)(?:,(\d+))? @@/);
       if (match) {
+        currentFile.hunksParsed = true;
         const startLine = parseInt(match[1], 10);
         const lineCount = match[2] !== undefined ? parseInt(match[2], 10) : 1;
         if (lineCount > 0) {

server/src/tools/sarif-tools.ts

@@ -1596,15 +1596,41 @@ describe('diffSarifByCommits', () => {
       expect(result.newResults.filter(r => r.file === 'src/db.js')).toHaveLength(1);
     });
 
-    it('should treat files with empty hunks as file-level match in line mode', () => {
+    it('should treat files with no hunks parsed as file-level match in line mode', () => {
       const sarif = createDiffTestSarif();
       const diffFiles: DiffFileEntry[] = [
-        { path: 'src/db.js', hunks: [] },
+        { path: 'src/db.js', hunks: [], hunksParsed: false },
+      ];
+
+      const result = diffSarifByCommits(sarif, diffFiles, 'main..HEAD', 'line');
+
+      // No hunk info at all → falls back to file-level
+      expect(result.newResults.filter(r => r.file === 'src/db.js')).toHaveLength(1);
+    });
+
+    it('should classify results as pre-existing for deletion-only diffs in line mode', () => {
+      const sarif = createDiffTestSarif();
+      // hunksParsed=true but hunks=[] means all hunks had lineCount=0 (deletion-only)
+      const diffFiles: DiffFileEntry[] = [
+        { path: 'src/db.js', hunks: [], hunksParsed: true },
       ];
 
       const result = diffSarifByCommits(sarif, diffFiles, 'main..HEAD', 'line');
 
-      // No hunk info → falls back to file-level
+      // Deletion-only file: no new lines → results should be pre-existing
+      expect(result.newResults.filter(r => r.file === 'src/db.js')).toHaveLength(0);
+      expect(result.preExistingResults.filter(r => r.file === 'src/db.js')).toHaveLength(1);
+    });
+
+    it('should classify results as new for deletion-only diffs in file mode', () => {
+      const sarif = createDiffTestSarif();
+      // In file mode, any matching file is "new" regardless of hunk details
+      const diffFiles: DiffFileEntry[] = [
+        { path: 'src/db.js', hunks: [], hunksParsed: true },
+      ];
+
+      const result = diffSarifByCommits(sarif, diffFiles, 'main..HEAD', 'file');
+
       expect(result.newResults.filter(r => r.file === 'src/db.js')).toHaveLength(1);
     });
 

server/test/src/lib/sarif-utils.test.ts

@@ -653,6 +653,32 @@ describe('SARIF Tools', () => {
         const newFiles = parsed.newResults.map((r: any) => r.file);
         expect(newFiles).toContain('src/db.js');
       });
+
+      it('should classify results as pre-existing for deletion-only diffs in line mode', async () => {
+        // Deletion-only hunk: @@ -10,3 +10,0 @@ means 3 lines removed, 0 added
+        mockExecuteCLICommand.mockResolvedValue({
+          success: true,
+          stdout: [
+            'diff --git a/src/db.js b/src/db.js',
+            '--- a/src/db.js',
+            '+++ b/src/db.js',
+            '@@ -10,3 +10,0 @@',
+          ].join('\n'),
+          stderr: '',
+        });
+
+        const result = await handlers.sarif_diff_by_commits({
+          sarifPath: testSarifPath,
+          refRange: 'main..HEAD',
+          granularity: 'line',
+        });
+        const parsed = JSON.parse(result.content[0].text);
+
+        expect(parsed.granularity).toBe('line');
+        // src/db.js had only deletions — no new lines, so result at line 42 is pre-existing
+        const newInDb = parsed.newResults.filter((r: any) => r.file === 'src/db.js');
+        expect(newInDb).toHaveLength(0);
+      });
     });
   });
 });