More fixes for PR review feedback

Author: data-douser

client/go.mod

@@ -1,6 +1,6 @@
 module github.com/advanced-security/codeql-development-mcp-server/client
 
-go 1.25.8
+go 1.25.6
 
 require (
 	github.com/mark3labs/mcp-go v0.47.0

client/internal/testing/params.go

@@ -150,7 +150,7 @@ func buildToolParams(repoRoot, toolName, testCase, testDir string) (map[string]a
 		params["query"] = filepath.Join(beforeDir, qlFiles[0])
 
 	case "codeql_resolve_queries":
-		params["path"] = beforeDir
+		params["directory"] = beforeDir
 
 	case "codeql_resolve_tests":
 		params["tests"] = []string{beforeDir}

client/internal/testing/params_test.go

@@ -65,6 +65,25 @@ func TestBuildToolParams_ValidateCodeqlQuery(t *testing.T) {
 	}
 }
 
+func TestBuildToolParams_ResolveQueries_UsesDirectoryKey(t *testing.T) {
+	dir := t.TempDir()
+	testDir := filepath.Join(dir, "tools", "codeql_resolve_queries", "resolve_queries")
+	os.MkdirAll(filepath.Join(testDir, "before"), 0o755)
+	os.MkdirAll(filepath.Join(testDir, "after"), 0o755)
+
+	params, err := buildToolParams(dir, "codeql_resolve_queries", "resolve_queries", testDir)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// The server schema expects "directory", not "path"
+	if _, ok := params["directory"]; !ok {
+		t.Errorf("expected params to contain 'directory' key, got keys: %v", params)
+	}
+	if _, ok := params["path"]; ok {
+		t.Errorf("params should NOT contain 'path' key for codeql_resolve_queries; use 'directory' instead")
+	}
+}
+
 func TestBuildToolParams_ResolveLanguages(t *testing.T) {
 	dir := t.TempDir()
 	testDir := filepath.Join(dir, "tools", "codeql_resolve_languages", "list_languages")

client/internal/testing/runner.go

@@ -95,8 +95,10 @@ func (r *Runner) Run() (bool, []TestResult) {
 
 	entries, err := os.ReadDir(testsDir)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "No integration tests directory found: %v\n", err)
-		return true, nil
+		errMsg := fmt.Sprintf("integration tests directory not found: %v", err)
+		fmt.Fprintf(os.Stderr, "Error: %s\n", errMsg)
+		r.recordResult("(fixtures)", "", false, errMsg, 0)
+		return r.printSummary(), r.results
 	}
 
 	var toolDirs []string

client/internal/testing/runner_test.go

@@ -101,8 +101,13 @@ func TestTruncate(t *testing.T) {
 func TestRunnerWithMockCaller(t *testing.T) {
 	caller := newMockCaller()
 
+	// Create a valid repo root with an empty fixtures directory
+	dir := t.TempDir()
+	testsDir := filepath.Join(dir, "client", "integration-tests", "primitives", "tools")
+	os.MkdirAll(testsDir, 0o755)
+
 	opts := RunnerOptions{
-		RepoRoot:    "/nonexistent",
+		RepoRoot:    dir,
 		FilterTools: []string{"nonexistent_tool"},
 	}
 
@@ -111,9 +116,8 @@ func TestRunnerWithMockCaller(t *testing.T) {
 		t.Fatal("NewRunner returned nil")
 	}
 
-	// Running with nonexistent directory should not panic
+	// Running with no matching tool dirs = all passed (vacuously true)
 	allPassed, results := runner.Run()
-	// No tests found = all passed (vacuously true)
 	if !allPassed {
 		t.Error("expected allPassed=true when no tests found")
 	}
@@ -125,8 +129,13 @@ func TestRunnerWithMockCaller(t *testing.T) {
 func TestRunnerNoInstallPacks(t *testing.T) {
 	caller := newMockCaller()
 
+	// Create a valid repo root with an empty fixtures directory
+	dir := t.TempDir()
+	testsDir := filepath.Join(dir, "client", "integration-tests", "primitives", "tools")
+	os.MkdirAll(testsDir, 0o755)
+
 	opts := RunnerOptions{
-		RepoRoot:       "/nonexistent",
+		RepoRoot:       dir,
 		NoInstallPacks: true,
 		FilterTools:    []string{"codeql_pack_install"},
 	}
@@ -295,6 +304,28 @@ func TestValidateAssertions_MultipleBlocks(t *testing.T) {
 	}
 }
 
+func TestRunnerMissingFixturesDirFails(t *testing.T) {
+	caller := newMockCaller()
+
+	// Point to a repo root that exists but has no client/integration-tests/primitives/tools
+	dir := t.TempDir()
+
+	opts := RunnerOptions{
+		RepoRoot: dir,
+	}
+
+	runner := NewRunner(caller, opts)
+	allPassed, results := runner.Run()
+
+	// A missing fixtures directory must be treated as a hard failure
+	if allPassed {
+		t.Error("expected allPassed=false when fixtures directory is missing")
+	}
+	if len(results) == 0 {
+		t.Error("expected at least one failure result for missing fixtures directory")
+	}
+}
+
 func TestRunnerAssertionFailure(t *testing.T) {
 	caller := newMockCaller()
 	caller.results["mock_tool"] = mockResult{

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

@@ -190235,7 +190235,7 @@ var MonitoringConfigSchema = external_exports.object({
   enableMonitoringTools: external_exports.boolean().default(false),
   // Opt-in: session_* tools disabled by default for end-users
   enableAnnotationTools: external_exports.boolean().default(false)
-  // Opt-in: annotation_* and audit_* tools disabled by default
+  // Controls auto-caching behavior in result-processor; tools are always registered
 });
 
 // src/lib/session-data-manager.ts

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

@@ -1,8 +1,7 @@
 /**
  * Annotation Tools — general-purpose notes and bookmarks on any entity.
  *
- * Enabled by default. Can be disabled via ENABLE_ANNOTATION_TOOLS=false.
- * Uses the shared SqliteStore from the session data manager.
+ * Always registered. Uses the shared SqliteStore from the session data manager.
  */
 
 import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';

server/src/tools/annotation-tools.ts

@@ -196,7 +196,7 @@ export const MonitoringConfigSchema = z.object({
   archiveCompletedSessions: z.boolean().default(true),
   enableRecommendations: z.boolean().default(true),
   enableMonitoringTools: z.boolean().default(false), // Opt-in: session_* tools disabled by default for end-users
-  enableAnnotationTools: z.boolean().default(false), // Opt-in: annotation_* and audit_* tools disabled by default
+  enableAnnotationTools: z.boolean().default(false), // Controls auto-caching behavior in result-processor; tools are always registered
 });
 
 export type MonitoringConfig = z.infer<typeof MonitoringConfigSchema>;

server/src/types/monitoring.ts

@@ -59,6 +59,35 @@ describe('Annotation Tools', () => {
       expect(mockServer.tool).toHaveBeenCalledTimes(6);
     });
 
+    it('should register all 6 annotation tools even when enableAnnotationTools is false', () => {
+      vi.spyOn(sessionDataManager, 'getConfig').mockReturnValue({
+        storageLocation: testStorageDir,
+        autoTrackSessions: true,
+        retentionDays: 90,
+        includeCallParameters: true,
+        includeCallResults: true,
+        maxActiveSessionsPerQuery: 3,
+        scoringFrequency: 'per_call',
+        archiveCompletedSessions: true,
+        enableAnnotationTools: false,
+        enableRecommendations: true,
+        enableMonitoringTools: false,
+      });
+
+      registerAnnotationTools(mockServer);
+
+      const toolCalls = (mockServer.tool as any).mock.calls;
+      const toolNames = toolCalls.map((call: any) => call[0]);
+
+      expect(toolNames).toContain('annotation_create');
+      expect(toolNames).toContain('annotation_delete');
+      expect(toolNames).toContain('annotation_get');
+      expect(toolNames).toContain('annotation_list');
+      expect(toolNames).toContain('annotation_search');
+      expect(toolNames).toContain('annotation_update');
+      expect(mockServer.tool).toHaveBeenCalledTimes(6);
+    });
+
     describe('tool schema validation', () => {
       beforeEach(() => {
         vi.spyOn(sessionDataManager, 'getConfig').mockReturnValue({