TDD improvements for ql-mcp-client

Author: data-douser

client/cmd/root_test.go

@@ -5,18 +5,31 @@ import (
 	"testing"
 )
 
-func TestRootCommand_Help(t *testing.T) {
-	cmd := rootCmd
+// executeRootCmd executes rootCmd with the given args and returns stdout.
+// It resets the command's output and args before and after each call
+// to avoid leaking state between tests.
+func executeRootCmd(args []string) (string, error) {
 	buf := new(bytes.Buffer)
-	cmd.SetOut(buf)
-	cmd.SetArgs([]string{"--help"})
+	rootCmd.SetOut(buf)
+	rootCmd.SetErr(buf)
+	rootCmd.SetArgs(args)
+
+	err := rootCmd.Execute()
+
+	// Reset to default so subsequent tests are not affected
+	rootCmd.SetOut(nil)
+	rootCmd.SetErr(nil)
+	rootCmd.SetArgs(nil)
+
+	return buf.String(), err
+}
 
-	err := cmd.Execute()
+func TestRootCommand_Help(t *testing.T) {
+	output, err := executeRootCmd([]string{"--help"})
 	if err != nil {
 		t.Fatalf("root --help failed: %v", err)
 	}
 
-	output := buf.String()
 	if output == "" {
 		t.Fatal("expected help output, got empty string")
 	}
@@ -55,9 +68,8 @@ func TestRootCommand_Version(t *testing.T) {
 }
 
 func TestRootCommand_DefaultFlags(t *testing.T) {
-	// Reset flags to defaults
-	rootCmd.SetArgs([]string{})
-	_ = rootCmd.Execute()
+	// Reset flags to defaults by executing with no args
+	_, _ = executeRootCmd([]string{})
 
 	if MCPMode() != "http" {
 		t.Errorf("expected default mode %q, got %q", "http", MCPMode())
@@ -74,27 +86,15 @@ func TestRootCommand_DefaultFlags(t *testing.T) {
 }
 
 func TestCodeScanningCommand_InHelp(t *testing.T) {
-	cmd := rootCmd
-	buf := new(bytes.Buffer)
-	cmd.SetOut(buf)
-	cmd.SetArgs([]string{"--help"})
-
-	_ = cmd.Execute()
-	output := buf.String()
+	output, _ := executeRootCmd([]string{"--help"})
 
 	if !bytes.Contains([]byte(output), []byte("code-scanning")) {
 		t.Error("root help should list code-scanning subcommand")
 	}
 }
 
 func TestSarifCommand_InHelp(t *testing.T) {
-	cmd := rootCmd
-	buf := new(bytes.Buffer)
-	cmd.SetOut(buf)
-	cmd.SetArgs([]string{"--help"})
-
-	_ = cmd.Execute()
-	output := buf.String()
+	output, _ := executeRootCmd([]string{"--help"})
 
 	if !bytes.Contains([]byte(output), []byte("sarif")) {
 		t.Error("root help should list sarif subcommand")

client/internal/github/client.go

@@ -13,7 +13,8 @@ import (
 
 // Client wraps the go-gh REST client for Code Scanning API calls.
 type Client struct {
-	rest *ghapi.RESTClient
+	rest      *ghapi.RESTClient
+	sarifRest *ghapi.RESTClient // lazily initialized; uses Accept: application/sarif+json
 }
 
 // NewClient creates a new GitHub API client using gh auth credentials.
@@ -30,6 +31,25 @@ func NewClient() (*Client, error) {
 	return &Client{rest: rest}, nil
 }
 
+// sarifClient returns or lazily creates a REST client with Accept: application/sarif+json.
+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": "2026-03-10",
+		},
+	}
+	client, err := ghapi.NewRESTClient(sarifOpts)
+	if err != nil {
+		return nil, fmt.Errorf("create SARIF client: %w", err)
+	}
+	c.sarifRest = client
+	return c.sarifRest, nil
+}
+
 // ListAnalysesOptions configures the list analyses request.
 type ListAnalysesOptions struct {
 	Owner     string
@@ -71,20 +91,13 @@ func (c *Client) ListAnalyses(opts ListAnalysesOptions) ([]Analysis, error) {
 func (c *Client) GetAnalysisSARIF(owner, repo string, analysisID int) ([]byte, error) {
 	path := fmt.Sprintf("repos/%s/%s/code-scanning/analyses/%d", owner, repo, analysisID)
 
-	// Need a separate client with SARIF Accept header
-	sarifOpts := ghapi.ClientOptions{
-		Headers: map[string]string{
-			"Accept":               "application/sarif+json",
-			"X-GitHub-Api-Version": "2026-03-10",
-		},
-	}
-	sarifClient, err := ghapi.NewRESTClient(sarifOpts)
+	sc, err := c.sarifClient()
 	if err != nil {
-		return nil, fmt.Errorf("create SARIF client: %w", err)
+		return nil, err
 	}
 
 	var sarif json.RawMessage
-	if err := sarifClient.Do("GET", path, nil, &sarif); err != nil {
+	if err := sc.Do("GET", path, nil, &sarif); err != nil {
 		return nil, fmt.Errorf("get analysis SARIF: %w", err)
 	}
 	return sarif, nil

client/internal/github/client_test.go

@@ -176,3 +176,13 @@ func TestIntToStr(t *testing.T) {
 		t.Errorf("intToStr(42) = %q, want %q", intToStr(42), "42")
 	}
 }
+
+func TestClient_SarifClientCached(t *testing.T) {
+	// Verify that the sarifRest field starts nil and would be set by sarifClient().
+	// We cannot call sarifClient() without real gh auth, so we verify the struct
+	// field starts nil (no eager init) and would be reused once set.
+	c := &Client{rest: nil}
+	if c.sarifRest != nil {
+		t.Error("sarifRest should be nil on a freshly constructed Client")
+	}
+}

package-lock.json

@@ -200402,7 +200402,8 @@ function registerSarifTools(server) {
   registerSarifStoreTool(server);
   logger.info("Registered SARIF analysis tools");
 }
-function loadSarif(sarifPath, cacheKey2, inlineContent) {
+function loadSarif(opts) {
+  const { cacheKey: cacheKey2, inlineContent, sarifPath } = opts;
   if (!sarifPath && !cacheKey2 && !inlineContent) {
     return { error: "Either sarifPath or cacheKey is required." };
   }
@@ -200453,7 +200454,7 @@ function registerSarifExtractRuleTool(server) {
       sarifPath: external_exports.string().optional().describe("Path to the SARIF file.")
     },
     async ({ sarifPath, cacheKey: cacheKey2, ruleId }) => {
-      const loaded = loadSarif(sarifPath, cacheKey2);
+      const loaded = loadSarif({ sarifPath, cacheKey: cacheKey2 });
       if (loaded.error) {
         return { content: [{ type: "text", text: loaded.error }] };
       }
@@ -200490,7 +200491,7 @@ function registerSarifListRulesTool(server) {
       sarifPath: external_exports.string().optional().describe("Path to the SARIF file.")
     },
     async ({ sarifPath, cacheKey: cacheKey2 }) => {
-      const loaded = loadSarif(sarifPath, cacheKey2);
+      const loaded = loadSarif({ sarifPath, cacheKey: cacheKey2 });
       if (loaded.error) {
         return { content: [{ type: "text", text: loaded.error }] };
       }
@@ -200518,7 +200519,7 @@ function registerSarifRuleToMarkdownTool(server) {
       sarifPath: external_exports.string().optional().describe("Path to the SARIF file.")
     },
     async ({ sarifPath, cacheKey: cacheKey2, ruleId }) => {
-      const loaded = loadSarif(sarifPath, cacheKey2);
+      const loaded = loadSarif({ sarifPath, cacheKey: cacheKey2 });
       if (loaded.error) {
         return { content: [{ type: "text", text: loaded.error }] };
       }
@@ -200556,11 +200557,11 @@ function registerSarifCompareAlertsTool(server) {
       overlapMode: external_exports.enum(["sink", "source", "any-location", "full-path", "fingerprint"]).optional().default("sink").describe('Comparison mode: "sink" (primary locations), "source" (first dataflow step), "any-location" (all locations), "full-path" (structural path similarity), "fingerprint" (partialFingerprints match, falls back to full-path).')
     },
     async ({ alertA, alertB, overlapMode }) => {
-      const loadedA = loadSarif(alertA.sarifPath, alertA.cacheKey);
+      const loadedA = loadSarif({ sarifPath: alertA.sarifPath, cacheKey: alertA.cacheKey });
       if (loadedA.error) {
         return { content: [{ type: "text", text: `Alert A: ${loadedA.error}` }] };
       }
-      const loadedB = loadSarif(alertB.sarifPath, alertB.cacheKey);
+      const loadedB = loadSarif({ sarifPath: alertB.sarifPath, cacheKey: alertB.cacheKey });
       if (loadedB.error) {
         return { content: [{ type: "text", text: `Alert B: ${loadedB.error}` }] };
       }
@@ -200621,11 +200622,11 @@ function registerSarifDiffRunsTool(server) {
       sarifPathB: external_exports.string().optional().describe("Path to the second (comparison) SARIF file.")
     },
     async ({ sarifPathA, sarifPathB, cacheKeyA, cacheKeyB, labelA, labelB }) => {
-      const loadedA = loadSarif(sarifPathA, cacheKeyA);
+      const loadedA = loadSarif({ sarifPath: sarifPathA, cacheKey: cacheKeyA });
       if (loadedA.error) {
         return { content: [{ type: "text", text: `Run A: ${loadedA.error}` }] };
       }
-      const loadedB = loadSarif(sarifPathB, cacheKeyB);
+      const loadedB = loadSarif({ sarifPath: sarifPathB, cacheKey: cacheKeyB });
       if (loadedB.error) {
         return { content: [{ type: "text", text: `Run B: ${loadedB.error}` }] };
       }
@@ -200666,7 +200667,7 @@ function registerSarifStoreTool(server) {
       } else {
         content = sarifContent;
       }
-      const loaded = loadSarif(void 0, void 0, content);
+      const loaded = loadSarif({ inlineContent: content });
       if (loaded.error) {
         return { content: [{ type: "text", text: loaded.error }] };
       }
@@ -200717,11 +200718,11 @@ function registerSarifDeduplicateRulesTool(server) {
       sarifPathB: external_exports.string().optional().describe("Path to the second SARIF file.")
     },
     async ({ sarifPathA, sarifPathB, cacheKeyA, cacheKeyB, overlapThreshold }) => {
-      const loadedA = loadSarif(sarifPathA, cacheKeyA);
+      const loadedA = loadSarif({ sarifPath: sarifPathA, cacheKey: cacheKeyA });
       if (loadedA.error) {
         return { content: [{ type: "text", text: `SARIF A: ${loadedA.error}` }] };
       }
-      const loadedB = loadSarif(sarifPathB, cacheKeyB);
+      const loadedB = loadSarif({ sarifPath: sarifPathB, cacheKey: cacheKeyB });
       if (loadedB.error) {
         return { content: [{ type: "text", text: `SARIF B: ${loadedB.error}` }] };
       }
@@ -200738,28 +200739,31 @@ function registerSarifDeduplicateRulesTool(server) {
           const ruleObjA = extractedA.runs[0]?.tool.driver.rules?.[0] ?? { id: rA.ruleId };
           const ruleObjB = extractedB.runs[0]?.tool.driver.rules?.[0] ?? { id: rB.ruleId };
           const overlaps = findOverlappingAlerts(resultsA, ruleObjA, resultsB, ruleObjB, "full-path");
-          let fingerprintMatches = 0;
-          for (const rResultA of resultsA) {
+          const matchedAIndices = /* @__PURE__ */ new Set();
+          for (let ai = 0; ai < resultsA.length; ai++) {
             for (const rResultB of resultsB) {
-              const fpResult = computeFingerprintOverlap(rResultA, rResultB);
+              const fpResult = computeFingerprintOverlap(resultsA[ai], rResultB);
               if (fpResult.fingerprintMatch) {
-                fingerprintMatches++;
+                matchedAIndices.add(ai);
+                break;
               }
             }
           }
-          const matchedAlerts = Math.max(overlaps.length, fingerprintMatches);
-          const totalUnique = resultsA.length + resultsB.length - matchedAlerts;
-          const overlapScore = totalUnique > 0 ? matchedAlerts / totalUnique : 0;
+          const matchedAlerts = Math.max(overlaps.length, matchedAIndices.size);
+          const minResults = Math.min(resultsA.length, resultsB.length);
+          const cappedMatched = Math.min(matchedAlerts, minResults);
+          const totalUnique = resultsA.length + resultsB.length - cappedMatched;
+          const overlapScore = totalUnique > 0 ? cappedMatched / totalUnique : 0;
           if (overlapScore >= (overlapThreshold ?? 0.8)) {
             duplicateGroups.push({
-              matchedAlerts,
+              matchedAlerts: cappedMatched,
               overlapScore: Math.round(overlapScore * 1e3) / 1e3,
               ruleIdA: rA.ruleId,
               ruleIdB: rB.ruleId,
               totalA: resultsA.length,
               totalB: resultsB.length,
-              unmatchedA: resultsA.length - matchedAlerts,
-              unmatchedB: resultsB.length - matchedAlerts
+              unmatchedA: Math.max(0, resultsA.length - cappedMatched),
+              unmatchedB: Math.max(0, resultsB.length - cappedMatched)
             });
           }
         }