fix: address review feedback — sarifClient race, dedup formula comment, handler tests

1. Fix data race in sarifClient() by replacing nil-check with sync.Once
2. Add clarifying comment on overlap scoring formula in sarif_deduplicate_rules
3. Add 3 handler-level tests for sarif_deduplicate_rules (overlap detection,
   no-overlap, missing input error)

Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/7aefb02d-b415-49ed-87da-f03d9f4a2b3d

Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>

Author: Copilot

client/internal/github/client.go

@@ -7,14 +7,17 @@ import (
 	"fmt"
 	"net/url"
 	"strconv"
+	"sync"
 
 	ghapi "github.com/cli/go-gh/v2/pkg/api"
 )
 
 // Client wraps the go-gh REST client for Code Scanning API calls.
 type Client struct {
-	rest      *ghapi.RESTClient
-	sarifRest *ghapi.RESTClient // lazily initialized; uses Accept: application/sarif+json
+	rest         *ghapi.RESTClient
+	sarifRest    *ghapi.RESTClient // lazily initialized; uses Accept: application/sarif+json
+	sarifOnce    sync.Once
+	sarifInitErr error
 }
 
 // NewClient creates a new GitHub API client using gh auth credentials.
@@ -32,22 +35,21 @@ func NewClient() (*Client, error) {
 }
 
 // sarifClient returns or lazily creates a REST client with Accept: application/sarif+json.
+// Uses sync.Once to prevent data races from concurrent goroutines.
 func (c *Client) sarifClient() (*ghapi.RESTClient, error) {
-	if c.sarifRest != nil {
-		return c.sarifRest, nil
-	}
-	sarifOpts := ghapi.ClientOptions{
-		Headers: map[string]string{
-			"Accept":               "application/sarif+json",
-			"X-GitHub-Api-Version": "2022-11-28",
-		},
-	}
-	client, err := ghapi.NewRESTClient(sarifOpts)
-	if err != nil {
-		return nil, fmt.Errorf("create SARIF client: %w", err)
-	}
-	c.sarifRest = client
-	return c.sarifRest, nil
+	c.sarifOnce.Do(func() {
+		sarifOpts := ghapi.ClientOptions{
+			Headers: map[string]string{
+				"Accept":               "application/sarif+json",
+				"X-GitHub-Api-Version": "2022-11-28",
+			},
+		}
+		c.sarifRest, c.sarifInitErr = ghapi.NewRESTClient(sarifOpts)
+		if c.sarifInitErr != nil {
+			c.sarifInitErr = fmt.Errorf("create SARIF client: %w", c.sarifInitErr)
+		}
+	})
+	return c.sarifRest, c.sarifInitErr
 }
 
 // ListAnalysesOptions configures the list analyses request.

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

@@ -495,6 +495,11 @@ function registerSarifDeduplicateRulesTool(server: McpServer): void {
             }
           }
 
+          // Overlap scoring: We use the higher of two matching strategies:
+          // 1. Location-based: `overlaps.length` from full-path structural comparison
+          // 2. Fingerprint-based: `matchedAIndices.size` from partialFingerprints
+          // The score is Jaccard-like: matchedAlerts / (totalA + totalB - matchedAlerts).
+          // We cap matchedAlerts at min(totalA, totalB) so unmatched counts stay non-negative.
           const matchedAlerts = Math.max(overlaps.length, matchedAIndices.size);
           const minResults = Math.min(resultsA.length, resultsB.length);
           // Cap matched alerts at the smaller set size to avoid negative unmatched counts

server/src/tools/sarif-tools.ts

@@ -402,5 +402,106 @@ describe('SARIF Tools', () => {
         expect(result.content[0].text).toContain('Invalid SARIF');
       });
     });
+
+    describe('sarif_deduplicate_rules', () => {
+      it('should detect duplicate rules across two SARIF files with overlapping results', async () => {
+        // Create SARIF B with same rule and matching codeFlow locations to A.
+        // Both results must have codeFlows for full-path overlap to detect them.
+        const sarifB = {
+          version: '2.1.0',
+          runs: [{
+            tool: {
+              driver: {
+                name: 'CodeQL',
+                version: '2.25.1',
+                rules: [
+                  { id: 'js/sql-injection-v2', shortDescription: { text: 'SQL injection (v2)' } },
+                ],
+              },
+            },
+            results: [
+              {
+                ruleId: 'js/sql-injection-v2',
+                ruleIndex: 0,
+                message: { text: 'SQL injection from user input.' },
+                locations: [{ physicalLocation: { artifactLocation: { uri: 'src/db.js' }, region: { startLine: 42, startColumn: 5, endColumn: 38 } } }],
+                codeFlows: [{ threadFlows: [{ locations: [
+                  { location: { physicalLocation: { artifactLocation: { uri: 'src/handler.js' }, region: { startLine: 10 } }, message: { text: 'req.query.id' } } },
+                  { location: { physicalLocation: { artifactLocation: { uri: 'src/db.js' }, region: { startLine: 42 } }, message: { text: 'query(...)' } } },
+                ] }] }],
+              },
+            ],
+          }],
+        };
+        const pathB = join(testStorageDir, 'dedup-b.sarif');
+        writeFileSync(pathB, JSON.stringify(sarifB));
+
+        // Use a low threshold since only the codeFlow result will match (1 of 2 in A)
+        const result = await handlers.sarif_deduplicate_rules({
+          sarifPathA: testSarifPath,
+          sarifPathB: pathB,
+          overlapThreshold: 0.3,
+        });
+        const parsed = JSON.parse(result.content[0].text);
+
+        expect(parsed.summary.totalRulesA).toBe(2);
+        expect(parsed.summary.totalRulesB).toBe(1);
+        expect(parsed.duplicateGroups.length).toBeGreaterThan(0);
+
+        // The js/sql-injection (A) vs js/sql-injection-v2 (B) pair should be detected
+        const sqlPair = parsed.duplicateGroups.find(
+          (g: any) => g.ruleIdA === 'js/sql-injection' && g.ruleIdB === 'js/sql-injection-v2',
+        );
+        expect(sqlPair).toBeDefined();
+        expect(sqlPair.overlapScore).toBeGreaterThan(0);
+        expect(sqlPair.matchedAlerts).toBeGreaterThan(0);
+        expect(sqlPair.totalA).toBe(2);
+        expect(sqlPair.totalB).toBe(1);
+      });
+
+      it('should return empty groups when no rules overlap', async () => {
+        // Create SARIF B with completely different rule and locations
+        const sarifB = {
+          version: '2.1.0',
+          runs: [{
+            tool: {
+              driver: {
+                name: 'CodeQL',
+                version: '2.25.1',
+                rules: [
+                  { id: 'py/command-injection', shortDescription: { text: 'Command injection' } },
+                ],
+              },
+            },
+            results: [
+              {
+                ruleId: 'py/command-injection',
+                ruleIndex: 0,
+                message: { text: 'Command injection.' },
+                locations: [{ physicalLocation: { artifactLocation: { uri: 'src/run.py' }, region: { startLine: 100, startColumn: 1, endColumn: 20 } } }],
+              },
+            ],
+          }],
+        };
+        const pathB = join(testStorageDir, 'dedup-none.sarif');
+        writeFileSync(pathB, JSON.stringify(sarifB));
+
+        const result = await handlers.sarif_deduplicate_rules({
+          sarifPathA: testSarifPath,
+          sarifPathB: pathB,
+        });
+        const parsed = JSON.parse(result.content[0].text);
+
+        expect(parsed.summary.overlapThreshold).toBe(0.8);
+        expect(parsed.duplicateGroups).toHaveLength(0);
+      });
+
+      it('should return error when sarifPathA is missing', async () => {
+        const result = await handlers.sarif_deduplicate_rules({
+          sarifPathB: testSarifPath,
+        });
+        expect(result.content[0].text).toContain('Either sarifPath or cacheKey is required');
+      });
+    });
   });
 });