Address PR review feedback: fix Close() kill, remove enableAnnotationTools setting, record skipped results, fix schema default, clarify changelog

Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/a90a53a5-b2ad-4775-8b61-f11d16b33749

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

Author: Copilot

CHANGELOG.md

@@ -18,7 +18,7 @@ _Changes on `main` since the latest tagged release that have not yet been includ
 
 ### Highlights
 
-- **Annotation, audit, cache, and SARIF tools are now always enabled** — Removed the `ENABLE_ANNOTATION_TOOLS` opt-in gate; all annotation, audit, query result cache, and SARIF analysis tools are registered by default. The `ENABLE_ANNOTATION_TOOLS` environment variable is still respected (set to `false` to disable) but defaults to `true`. ([#223](https://github.com/advanced-security/codeql-development-mcp-server/pull/223))
+- **Annotation, audit, cache, and SARIF tools are now always enabled** — Removed the `ENABLE_ANNOTATION_TOOLS` opt-in gate; all annotation, audit, query result cache, and SARIF analysis tools are registered by default. The `ENABLE_ANNOTATION_TOOLS` environment variable no longer controls tool availability; when set to `false`, it only disables the related auto-caching behaviour in result processing. ([#223](https://github.com/advanced-security/codeql-development-mcp-server/pull/223))
 - **Go-based `ql-mcp-client` rewrite** — Replaced the Node.js `ql-mcp-client.js` integration test runner with a Go CLI (`gh-ql-mcp-client`) built with Cobra and `mcp-go`. Adds `list tools/prompts/resources` commands and assertion-based integration test validation. ([#223](https://github.com/advanced-security/codeql-development-mcp-server/pull/223))
 - **Persistent MRVA workflow state and caching** — Introduced a new `SqliteStore` backend plus annotation, audit, and query result cache tools to support the next phase of MCP-assisted CodeQL development and `seclab-taskflow-agent` integration. ([#169](https://github.com/advanced-security/codeql-development-mcp-server/pull/169))
 - **Rust language support** — Added first-class Rust support with `PrintAST`, `PrintCFG`, `CallGraphFrom`, `CallGraphTo`, and `CallGraphFromTo` queries, bringing the total supported languages to 10. ([#195](https://github.com/advanced-security/codeql-development-mcp-server/pull/195))

client/Makefile

@@ -12,7 +12,7 @@ export CGO_ENABLED = 0
 # Build flags
 LDFLAGS := -s -w
 
-.PHONY: all build test test-unit test-integration lint clean install build-all
+.PHONY: all build test test-unit test-integration test-integration-fast lint clean install build-all
 
 all: lint test build
 
@@ -27,8 +27,12 @@ test: test-unit test-integration
 test-unit:
 	go test ./...
 
-## test-integration: Build binary and run integration tests via run-integration-tests.sh
+## test-integration: Build binary and run integration tests (including codeql_pack_install)
 test-integration: build
+	scripts/run-integration-tests.sh
+
+## test-integration-fast: Build binary and run integration tests, skipping pack installation
+test-integration-fast: build
 	scripts/run-integration-tests.sh --no-install-packs
 
 ## test-verbose: Run all unit tests with verbose output

client/internal/mcp/client.go

@@ -10,9 +10,21 @@ import (
 	"time"
 
 	mcpclient "github.com/mark3labs/mcp-go/client"
+	"github.com/mark3labs/mcp-go/client/transport"
 	"github.com/mark3labs/mcp-go/mcp"
 )
 
+// innerClient is satisfied by *mcpclient.Client and allows the Close() timeout
+// path to be tested without a live MCP server.
+type innerClient interface {
+	Initialize(context.Context, mcp.InitializeRequest) (*mcp.InitializeResult, error)
+	Close() error
+	CallTool(context.Context, mcp.CallToolRequest) (*mcp.CallToolResult, error)
+	ListTools(context.Context, mcp.ListToolsRequest) (*mcp.ListToolsResult, error)
+	ListPrompts(context.Context, mcp.ListPromptsRequest) (*mcp.ListPromptsResult, error)
+	ListResources(context.Context, mcp.ListResourcesRequest) (*mcp.ListResourcesResult, error)
+}
+
 const (
 	// ModeStdio spawns the MCP server as a child process.
 	ModeStdio = "stdio"
@@ -40,8 +52,9 @@ type Config struct {
 
 // Client wraps an MCP client with convenience methods for tool calls.
 type Client struct {
-	inner  *mcpclient.Client
-	config Config
+	inner     innerClient
+	config    Config
+	serverCmd *exec.Cmd // stdio mode only; retained so Close() can force-kill on timeout
 }
 
 // NewClient creates a new MCP client with the given config but does not connect.
@@ -67,14 +80,27 @@ func (c *Client) connectStdio(ctx context.Context) error {
 		return fmt.Errorf("cannot locate MCP server: %w", err)
 	}
 
-	client, err := mcpclient.NewStdioMCPClient(
+	// Capture the spawned exec.Cmd so Close() can force-kill the process on timeout.
+	var spawnedCmd *exec.Cmd
+	cmdFunc := func(spawnCtx context.Context, command string, env []string, args []string) (*exec.Cmd, error) {
+		cmd := exec.CommandContext(spawnCtx, command, args...)
+		if len(env) > 0 {
+			cmd.Env = append(os.Environ(), env...)
+		}
+		spawnedCmd = cmd
+		return cmd, nil
+	}
+
+	client, err := mcpclient.NewStdioMCPClientWithOptions(
 		"node", nil,
-		serverPath,
+		[]string{serverPath},
+		transport.WithCommandFunc(cmdFunc),
 	)
 	if err != nil {
 		return fmt.Errorf("failed to create stdio MCP client: %w", err)
 	}
 	c.inner = client
+	c.serverCmd = spawnedCmd
 
 	initCtx, cancel := context.WithTimeout(ctx, ConnectTimeout)
 	defer cancel()
@@ -135,8 +161,8 @@ func (c *Client) Close() error {
 
 	// For stdio transport, closing stdin signals the server to exit.
 	// However, the Node.js server may not exit immediately, so we
-	// run Close in a goroutine with a timeout and kill the process
-	// if it doesn't exit within 3 seconds.
+	// run Close in a goroutine with a timeout and force-kill the
+	// subprocess if it doesn't exit within 3 seconds.
 	done := make(chan error, 1)
 	go func() {
 		done <- c.inner.Close()
@@ -146,8 +172,10 @@ func (c *Client) Close() error {
 	case err := <-done:
 		return err
 	case <-time.After(3 * time.Second):
-		// Close didn't complete in time; the process is likely stuck.
-		// This is expected with Node.js stdio servers.
+		// Close didn't complete in time; force-kill the subprocess.
+		if c.serverCmd != nil && c.serverCmd.Process != nil {
+			_ = c.serverCmd.Process.Kill()
+		}
 		return fmt.Errorf("%s", CloseTimeoutErr)
 	}
 }

client/internal/mcp/client_test.go

@@ -1,10 +1,46 @@
 package mcp
 
 import (
+	"context"
+	"os/exec"
+	"strings"
 	"testing"
 	"time"
+
+	"github.com/mark3labs/mcp-go/mcp"
 )
 
+// hangCloser is a stub innerClient whose Close() blocks until released.
+// It satisfies the innerClient interface to allow timeout-path testing.
+type hangCloser struct {
+	released chan struct{}
+}
+
+func (h *hangCloser) Close() error {
+	<-h.released
+	return nil
+}
+
+func (h *hangCloser) Initialize(_ context.Context, _ mcp.InitializeRequest) (*mcp.InitializeResult, error) {
+	return nil, nil
+}
+
+func (h *hangCloser) CallTool(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) {
+	return nil, nil
+}
+
+func (h *hangCloser) ListTools(_ context.Context, _ mcp.ListToolsRequest) (*mcp.ListToolsResult, error) {
+	return nil, nil
+}
+
+func (h *hangCloser) ListPrompts(_ context.Context, _ mcp.ListPromptsRequest) (*mcp.ListPromptsResult, error) {
+	return nil, nil
+}
+
+func (h *hangCloser) ListResources(_ context.Context, _ mcp.ListResourcesRequest) (*mcp.ListResourcesResult, error) {
+	return nil, nil
+}
+
 func TestTimeoutForTool_CodeQLTools(t *testing.T) {
 	codeqlTools := []string{
 		"codeql_query_run",
@@ -82,6 +118,61 @@ func TestClose_TimeoutReturnsError(t *testing.T) {
 	}
 }
 
+func TestClose_KillsSubprocessOnTimeout(t *testing.T) {
+	t.Parallel()
+
+	// Start a long-running subprocess to simulate a stuck server.
+	proc := exec.Command("sleep", "60")
+	if err := proc.Start(); err != nil {
+		t.Skipf("cannot start subprocess for test: %v", err)
+	}
+
+	hang := &hangCloser{released: make(chan struct{})}
+
+	c := &Client{
+		inner:     hang,
+		serverCmd: proc,
+	}
+
+	start := time.Now()
+	err := c.Close()
+	elapsed := time.Since(start)
+
+	// Release the hanging closer goroutine (cleanup).
+	close(hang.released)
+
+	if err == nil {
+		t.Fatal("Close() should return a timeout error, got nil")
+	}
+	if !strings.Contains(err.Error(), "timed out") {
+		t.Errorf("Close() error = %q; want a message containing 'timed out'", err.Error())
+	}
+
+	// Elapsed time should be roughly the 3-second timeout window.
+	if elapsed > 5*time.Second {
+		t.Errorf("Close() took %v, expected ~3s", elapsed)
+	}
+
+	// proc.Wait() should return quickly if the process was killed.
+	// Use a channel with timeout to avoid hanging the test if Kill() failed.
+	waitCh := make(chan error, 1)
+	go func() {
+		waitCh <- proc.Wait()
+	}()
+
+	select {
+	case waitErr := <-waitCh:
+		// Process exited (possibly as a zombie until Wait() is called).
+		// A killed process returns an error from Wait().
+		if waitErr == nil {
+			t.Error("subprocess exited with success; expected it to be killed (non-zero exit)")
+		}
+	case <-time.After(2 * time.Second):
+		_ = proc.Process.Kill()
+		t.Error("subprocess did not exit after Close() timeout; expected force-kill")
+	}
+}
+
 func TestConstants(t *testing.T) {
 	if DefaultTimeout != 60*time.Second {
 		t.Errorf("DefaultTimeout = %v, want 60s", DefaultTimeout)

client/internal/testing/runner.go

@@ -144,12 +144,14 @@ 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)
+		r.recordResult(toolName, "", false, "skipped: --no-install-packs", 0)
 		return
 	}
 
 	// Deprecated monitoring/session tools — skip entirely
 	if isDeprecatedTool(toolName) {
 		fmt.Printf("\n  %s (skipped: deprecated)\n", toolName)
+		r.recordResult(toolName, "", false, "skipped: deprecated", 0)
 		return
 	}
 

client/internal/testing/runner_test.go

@@ -129,29 +129,38 @@ func TestRunnerWithMockCaller(t *testing.T) {
 func TestRunnerNoInstallPacks(t *testing.T) {
 	caller := newMockCaller()
 
-	// Create a valid repo root with an empty fixtures directory
+	// Create a repo root with a codeql_pack_install fixture directory
 	dir := t.TempDir()
 	testsDir := filepath.Join(dir, "client", "integration-tests", "primitives", "tools")
-	os.MkdirAll(testsDir, 0o755)
+	// Create the fixture dir so the runner encounters it and records a skip
+	os.MkdirAll(filepath.Join(testsDir, "codeql_pack_install"), 0o755)
 
 	opts := RunnerOptions{
 		RepoRoot:       dir,
 		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
+	// codeql_pack_install should be skipped — one skipped result recorded
 	allPassed, results := runner.Run()
 	if !allPassed {
-		t.Error("expected allPassed=true when pack install is skipped")
+		t.Error("expected allPassed=true when only skipped results")
 	}
-	if len(results) != 0 {
-		t.Errorf("expected 0 results (skipped), got %d", len(results))
+	if len(results) != 1 {
+		t.Fatalf("expected 1 skipped result, got %d", len(results))
+	}
+	if results[0].ToolName != "codeql_pack_install" {
+		t.Errorf("expected skipped result for codeql_pack_install, got %q", results[0].ToolName)
+	}
+	if results[0].Error != "skipped: --no-install-packs" {
+		t.Errorf("unexpected skip reason: %q", results[0].Error)
+	}
+	if results[0].Passed {
+		t.Error("skipped result should have Passed=false")
 	}
 }
 

extensions/vscode/package.json

@@ -95,11 +95,6 @@
           "default": true,
           "markdownDescription": "Copy CodeQL databases from the `GitHub.vscode-codeql` extension storage into a managed directory, removing query-server lock files so the MCP server CLI can operate without contention. Disable to use databases in-place (may fail when the CodeQL query server is running)."
         },
-        "codeql-mcp.enableAnnotationTools": {
-          "type": "boolean",
-          "default": true,
-          "markdownDescription": "Enable annotation, audit, and query results caching tools. When enabled, the MCP server registers `annotation_*`, `audit_*`, and `query_results_cache_*` tools. Disable to reduce the tool surface if these capabilities are not needed."
-        },
         "codeql-mcp.serverArgs": {
           "type": "array",
           "items": {

extensions/vscode/test/suite/mcp-tool-e2e.integration.test.ts

@@ -271,9 +271,10 @@ suite('MCP Server Tool Integration Tests', () => {
 });
 
 /**
- * Integration tests for annotation and audit tools.
- * These tests spawn a separate server instance with ENABLE_ANNOTATION_TOOLS=true
- * to validate opt-in annotation and MRVA audit tool functionality.
+ * Integration tests for annotation, audit, cache, and SARIF tools.
+ * These tests spawn a separate server instance to validate annotation
+ * and MRVA audit tool functionality. All these tools are always registered
+ * by default (no ENABLE_ANNOTATION_TOOLS opt-in required).
  */
 suite('MCP Annotation & Audit Tool Integration Tests', () => {
   let client: Client;
@@ -295,7 +296,6 @@ suite('MCP Annotation & Audit Tool Integration Tests', () => {
     const env: Record<string, string> = {
       ...process.env as Record<string, string>,
       TRANSPORT_MODE: 'stdio',
-      ENABLE_ANNOTATION_TOOLS: 'true',
       CODEQL_DATABASES_BASE_DIRS: path.join(fixtureStorage, 'databases'),
       CODEQL_QUERY_RUN_RESULTS_DIRS: path.join(fixtureStorage, 'queries'),
       CODEQL_MRVA_RUN_RESULTS_DIRS: path.join(fixtureStorage, 'variant-analyses'),
@@ -310,7 +310,7 @@ suite('MCP Annotation & Audit Tool Integration Tests', () => {
 
     client = new Client({ name: 'annotation-test', version: '1.0.0' });
     await client.connect(transport);
-    console.log('[mcp-annotation-e2e] Connected to MCP server with annotation tools enabled');
+    console.log('[mcp-annotation-e2e] Connected to MCP server');
   });
 
   suiteTeardown(async function () {
@@ -319,7 +319,7 @@ suite('MCP Annotation & Audit Tool Integration Tests', () => {
     try { if (transport) await transport.close(); } catch { /* best-effort */ }
   });
 
-  test('Annotation tools should be available when ENABLE_ANNOTATION_TOOLS=true', async function () {
+  test('Annotation, audit, cache, and SARIF tools should always be available', async function () {
     this.timeout(15_000);
 
     const response = await client.listTools();
@@ -333,19 +333,19 @@ suite('MCP Annotation & Audit Tool Integration Tests', () => {
     assert.ok(toolNames.includes('annotation_delete'), 'Should include annotation_delete');
     assert.ok(toolNames.includes('annotation_search'), 'Should include annotation_search');
 
-    // Layer 2: audit tools (gated by same flag)
+    // Layer 2: audit tools
     assert.ok(toolNames.includes('audit_store_findings'), 'Should include audit_store_findings');
     assert.ok(toolNames.includes('audit_list_findings'), 'Should include audit_list_findings');
     assert.ok(toolNames.includes('audit_add_notes'), 'Should include audit_add_notes');
     assert.ok(toolNames.includes('audit_clear_repo'), 'Should include audit_clear_repo');
 
-    // Layer 3: query results cache tools (gated by same flag)
+    // Layer 3: query results cache tools
     assert.ok(toolNames.includes('query_results_cache_lookup'), 'Should include query_results_cache_lookup');
     assert.ok(toolNames.includes('query_results_cache_retrieve'), 'Should include query_results_cache_retrieve');
     assert.ok(toolNames.includes('query_results_cache_clear'), 'Should include query_results_cache_clear');
     assert.ok(toolNames.includes('query_results_cache_compare'), 'Should include query_results_cache_compare');
 
-    // Layer 4: SARIF analysis tools (gated by same flag)
+    // Layer 4: SARIF analysis tools
     assert.ok(toolNames.includes('sarif_extract_rule'), 'Should include sarif_extract_rule');
     assert.ok(toolNames.includes('sarif_list_rules'), 'Should include sarif_list_rules');
     assert.ok(toolNames.includes('sarif_rule_to_markdown'), 'Should include sarif_rule_to_markdown');

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

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