Address PR review feedback

Author: data-douser

client/README.md

@@ -39,6 +39,7 @@ gh ql-mcp-client <command> [flags]
 | `--format` | `text`      | Output format (`text`/`json`)         |
 
 Transport is configured via CLI flags. The CLI does not currently read `MCP_MODE`.
+
 ### Commands
 
 #### `code-scanning` (alias: `cs`)

client/cmd/integration_tests.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"time"
 
 	mcpclient "github.com/advanced-security/codeql-development-mcp-server/client/internal/mcp"
 	itesting "github.com/advanced-security/codeql-development-mcp-server/client/internal/testing"
@@ -37,16 +38,23 @@ func init() {
 	f.StringVar(&integrationTestsFlags.tools, "tools", "", "Comma-separated list of tool names to test")
 	f.StringVar(&integrationTestsFlags.tests, "tests", "", "Comma-separated list of test case names to run")
 	f.BoolVar(&integrationTestsFlags.noInstall, "no-install-packs", false, "Skip CodeQL pack installation")
-	f.IntVar(&integrationTestsFlags.timeout, "timeout", 30, "Per-tool-call timeout in seconds")
+	f.IntVar(&integrationTestsFlags.timeout, "timeout", 0, "Per-tool-call timeout in seconds (0 = use server defaults)")
 }
 
 // mcpToolCaller adapts the MCP client to the ToolCaller interface.
 type mcpToolCaller struct {
-	client *mcpclient.Client
+	client  *mcpclient.Client
+	timeout time.Duration
 }
 
 func (c *mcpToolCaller) CallToolRaw(name string, params map[string]any) ([]itesting.ContentBlock, bool, error) {
-	result, err := c.client.CallTool(context.Background(), name, params)
+	ctx := context.Background()
+	if c.timeout > 0 {
+		var cancel context.CancelFunc
+		ctx, cancel = context.WithTimeout(ctx, c.timeout)
+		defer cancel()
+	}
+	result, err := c.client.CallTool(ctx, name, params)
 	if err != nil {
 		return nil, false, err
 	}
@@ -115,10 +123,14 @@ func runIntegrationTests(cmd *cobra.Command, _ []string) error {
 	}
 
 	// Create and run the test runner
-	runner := itesting.NewRunner(&mcpToolCaller{client: client}, itesting.RunnerOptions{
-		RepoRoot:    repoRoot,
-		FilterTools: filterTools,
-		FilterTests: filterTests,
+	runner := itesting.NewRunner(&mcpToolCaller{
+		client:  client,
+		timeout: time.Duration(integrationTestsFlags.timeout) * time.Second,
+	}, itesting.RunnerOptions{
+		RepoRoot:       repoRoot,
+		FilterTools:    filterTools,
+		FilterTests:    filterTests,
+		NoInstallPacks: integrationTestsFlags.noInstall,
 	})
 
 	allPassed, _ := runner.Run()

client/internal/testing/params.go

@@ -28,7 +28,11 @@ func buildToolParams(repoRoot, toolName, testCase, testDir string) (map[string]a
 		if err := json.Unmarshal(configData, &config); err != nil {
 			return nil, fmt.Errorf("parse test-config.json: %w", err)
 		}
-		return config.Arguments, nil
+		// SARIF tools need derived path injection from before/ even when
+		// test-config.json provides base params — continue to tool-specific logic.
+		if !isSARIFTool(toolName) {
+			return config.Arguments, nil
+		}
 	}
 
 	// Check monitoring-state.json for embedded parameters
@@ -317,6 +321,12 @@ func buildToolParams(repoRoot, toolName, testCase, testDir string) (map[string]a
 	return params, nil
 }
 
+// isSARIFTool returns true for tools that need SARIF file path injection
+// from the before/ directory even when test-config.json provides base params.
+func isSARIFTool(name string) bool {
+	return strings.HasPrefix(name, "sarif_")
+}
+
 // findFilesByExt returns filenames in dir matching the given extension.
 func findFilesByExt(dir, ext string) []string {
 	entries, err := os.ReadDir(dir)

client/internal/testing/params_test.go

@@ -116,3 +116,95 @@ func TestFindFilesByExt(t *testing.T) {
 		t.Errorf("found %d .txt files, want 1", len(txtFiles))
 	}
 }
+
+func TestIsSARIFTool(t *testing.T) {
+	tests := []struct {
+		name string
+		want bool
+	}{
+		{"sarif_extract_rule", true},
+		{"sarif_list_rules", true},
+		{"sarif_compare_alerts", true},
+		{"sarif_diff_runs", true},
+		{"sarif_rule_to_markdown", true},
+		{"codeql_query_run", false},
+		{"validate_codeql_query", false},
+		{"codeql_pack_install", false},
+	}
+	for _, tt := range tests {
+		if got := isSARIFTool(tt.name); got != tt.want {
+			t.Errorf("isSARIFTool(%q) = %v, want %v", tt.name, got, tt.want)
+		}
+	}
+}
+
+func TestBuildToolParams_SARIFToolWithConfig(t *testing.T) {
+	dir := t.TempDir()
+	testDir := filepath.Join(dir, "tools", "sarif_extract_rule", "extract_sql_injection")
+	beforeDir := filepath.Join(testDir, "before")
+	os.MkdirAll(beforeDir, 0o755)
+	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
+
+	// Write test-config.json with ruleId but no sarifPath
+	os.WriteFile(filepath.Join(testDir, "test-config.json"),
+		[]byte(`{"toolName":"sarif_extract_rule","arguments":{"ruleId":"js/sql-injection"}}`), 0o600)
+
+	// Write a SARIF file in before/
+	os.WriteFile(filepath.Join(beforeDir, "test-input.sarif"),
+		[]byte(`{"version":"2.1.0"}`), 0o600)
+
+	params, err := buildToolParams(dir, "sarif_extract_rule", "extract_sql_injection", testDir)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// Should have sarifPath injected from before/
+	sarifPath, ok := params["sarifPath"].(string)
+	if !ok || sarifPath == "" {
+		t.Error("expected sarifPath to be injected from before/ directory")
+	}
+
+	// Should still have ruleId from config
+	if params["ruleId"] != "js/sql-injection" {
+		t.Errorf("params[ruleId] = %v, want js/sql-injection", params["ruleId"])
+	}
+}
+
+func TestBuildToolParams_SARIFCompareAlertsWithConfig(t *testing.T) {
+	dir := t.TempDir()
+	testDir := filepath.Join(dir, "tools", "sarif_compare_alerts", "sink_overlap")
+	beforeDir := filepath.Join(testDir, "before")
+	os.MkdirAll(beforeDir, 0o755)
+	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
+
+	// Write test-config.json with alertA/alertB but no sarifPath
+	os.WriteFile(filepath.Join(testDir, "test-config.json"),
+		[]byte(`{"toolName":"sarif_compare_alerts","arguments":{"alertA":{"ruleId":"r1","resultIndex":0},"alertB":{"ruleId":"r2","resultIndex":0},"overlapMode":"sink"}}`), 0o600)
+
+	// Write a SARIF file in before/
+	os.WriteFile(filepath.Join(beforeDir, "test-input.sarif"),
+		[]byte(`{"version":"2.1.0"}`), 0o600)
+
+	params, err := buildToolParams(dir, "sarif_compare_alerts", "sink_overlap", testDir)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// alertA should have sarifPath injected
+	alertA, ok := params["alertA"].(map[string]any)
+	if !ok {
+		t.Fatal("expected alertA in params")
+	}
+	if alertA["sarifPath"] == nil || alertA["sarifPath"] == "" {
+		t.Error("expected sarifPath injected into alertA")
+	}
+
+	// alertB should have sarifPath injected
+	alertB, ok := params["alertB"].(map[string]any)
+	if !ok {
+		t.Fatal("expected alertB in params")
+	}
+	if alertB["sarifPath"] == nil || alertB["sarifPath"] == "" {
+		t.Error("expected sarifPath injected into alertB")
+	}
+}

client/internal/testing/runner.go

@@ -38,9 +38,10 @@ type TestResult struct {
 
 // RunnerOptions configures the integration test runner.
 type RunnerOptions struct {
-	FilterTests []string
-	FilterTools []string
-	RepoRoot    string
+	FilterTests    []string
+	FilterTools    []string
+	NoInstallPacks bool
+	RepoRoot       string
 }
 
 // Runner discovers and executes integration tests.
@@ -54,7 +55,7 @@ type Runner struct {
 
 // NewRunner creates a new integration test runner.
 func NewRunner(caller ToolCaller, opts RunnerOptions) *Runner {
-	tmpBase := filepath.Join(opts.RepoRoot, ".tmp")
+	tmpBase := filepath.Join(opts.RepoRoot, "server", ".tmp")
 	return &Runner{
 		caller:  caller,
 		options: opts,
@@ -124,6 +125,12 @@ func (r *Runner) Run() (bool, []TestResult) {
 }
 
 func (r *Runner) runToolTests(toolName, testsDir string) {
+	// Skip codeql_pack_install when --no-install-packs is set
+	if r.options.NoInstallPacks && toolName == "codeql_pack_install" {
+		fmt.Printf("\n  %s (skipped: --no-install-packs)\n", toolName)
+		return
+	}
+
 	// Deprecated monitoring/session tools — skip entirely
 	if isDeprecatedTool(toolName) {
 		fmt.Printf("\n  %s (skipped: deprecated)\n", toolName)
@@ -227,6 +234,14 @@ func (r *Runner) runSingleTest(toolName, testCase, toolDir string) {
 		return
 	}
 
+	// Validate non-empty response content — catches broken tools that
+	// return success with empty results.
+	if len(content) == 0 {
+		r.recordResult(toolName, testCase, false, "tool returned no content blocks", elapsed)
+		fmt.Printf("    FAIL %s (no content) [%.1fs]\n", testCase, elapsed.Seconds())
+		return
+	}
+
 	r.recordResult(toolName, testCase, true, "", elapsed)
 	fmt.Printf("    PASS %s [%.1fs]\n", testCase, elapsed.Seconds())
 }

client/internal/testing/runner_test.go

@@ -1,6 +1,8 @@
 package testing
 
 import (
+	"os"
+	"path/filepath"
 	"testing"
 )
 
@@ -119,3 +121,72 @@ func TestRunnerWithMockCaller(t *testing.T) {
 		t.Errorf("expected 0 results, got %d", len(results))
 	}
 }
+
+func TestRunnerNoInstallPacks(t *testing.T) {
+	caller := newMockCaller()
+
+	opts := RunnerOptions{
+		RepoRoot:       "/nonexistent",
+		NoInstallPacks: true,
+		FilterTools:    []string{"codeql_pack_install"},
+	}
+
+	runner := NewRunner(caller, opts)
+	if runner == nil {
+		t.Fatal("NewRunner returned nil")
+	}
+
+	// codeql_pack_install should be skipped — no results
+	allPassed, results := runner.Run()
+	if !allPassed {
+		t.Error("expected allPassed=true when pack install is skipped")
+	}
+	if len(results) != 0 {
+		t.Errorf("expected 0 results (skipped), got %d", len(results))
+	}
+}
+
+func TestRunnerEmptyContentFails(t *testing.T) {
+	caller := newMockCaller()
+	// Return empty content blocks for mock_tool
+	caller.results["mock_tool"] = mockResult{
+		content: []ContentBlock{},
+		isError: false,
+		err:     nil,
+	}
+
+	// Create a minimal test fixture directory
+	dir := t.TempDir()
+	testsDir := filepath.Join(dir, "client", "integration-tests", "primitives", "tools")
+	toolDir := filepath.Join(testsDir, "mock_tool", "basic")
+	os.MkdirAll(filepath.Join(toolDir, "before"), 0o755)
+	os.MkdirAll(filepath.Join(toolDir, "after"), 0o755)
+	os.WriteFile(filepath.Join(toolDir, "test-config.json"),
+		[]byte(`{"toolName":"mock_tool","arguments":{"key":"value"}}`), 0o600)
+
+	opts := RunnerOptions{
+		RepoRoot: dir,
+	}
+
+	runner := NewRunner(caller, opts)
+	allPassed, results := runner.Run()
+
+	if allPassed {
+		t.Error("expected allPassed=false when tool returns empty content")
+	}
+	if len(results) == 0 {
+		t.Fatal("expected at least one result")
+	}
+	for _, r := range results {
+		if r.ToolName == "mock_tool" && r.TestName == "basic" {
+			if r.Passed {
+				t.Error("expected mock_tool/basic to fail with empty content")
+			}
+			if r.Error != "tool returned no content blocks" {
+				t.Errorf("unexpected error: %q", r.Error)
+			}
+			return
+		}
+	}
+	t.Error("mock_tool/basic result not found")
+}