From 07ba85a069b342b62e90d4d139c3bcbfdc2999c8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 27 Jan 2026 16:05:44 +0000 Subject: [PATCH 1/3] Initial plan From e505b5ba49c142f1c868d6e9281bf424bc2f10b0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 27 Jan 2026 16:11:55 +0000 Subject: [PATCH 2/3] Fix default mode to routed and add non-existent tool tests Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- internal/cmd/root.go | 8 +- .../server/nonexistent_tool_logging_test.go | 223 ++++++++++++++++++ 2 files changed, 227 insertions(+), 4 deletions(-) create mode 100644 internal/server/nonexistent_tool_logging_test.go diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 6640293..90e91b3 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -254,10 +254,10 @@ func run(cmd *cobra.Command, args []string) error { log.Println("Parallel server launching enabled (default)") } - // Determine mode (default to unified if neither flag is set) - mode := "unified" - if routedMode { - mode = "routed" + // Determine mode (default to routed if neither flag is set) + mode := "routed" + if unifiedMode { + mode = "unified" } debugLog.Printf("Server mode: %s, DIFC enabled: %v", mode, cfg.EnableDIFC) diff --git a/internal/server/nonexistent_tool_logging_test.go b/internal/server/nonexistent_tool_logging_test.go new file mode 100644 index 0000000..a094642 --- /dev/null +++ b/internal/server/nonexistent_tool_logging_test.go @@ -0,0 +1,223 @@ +package server + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/githubnext/gh-aw-mcpg/internal/config" + "github.com/githubnext/gh-aw-mcpg/internal/logger" +) + +// TestNonExistentToolCallLogging_RoutedMode verifies that when a non-existent tool +// is called via routed mode, an appropriate error is returned and logged. +func TestNonExistentToolCallLogging_RoutedMode(t *testing.T) { + // Initialize logger to capture log output + logger.InitFileLogger("/tmp", "test-nonexistent-tool.log") + defer logger.CloseGlobalLogger() + + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "safeoutputs": {Command: "echo", Args: []string{}}, + }, + } + + ctx := context.Background() + us, err := NewUnified(ctx, cfg) + require.NoError(t, err, "Failed to create unified server") + defer us.Close() + + // Create HTTP server in routed mode + httpServer := CreateHTTPServerForRoutedMode("127.0.0.1:0", us, "") + + // First, initialize the session with initialize request + initReq := map[string]interface{}{ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": map[string]interface{}{ + "protocolVersion": "2024-11-05", + "capabilities": map[string]interface{}{}, + "clientInfo": map[string]interface{}{ + "name": "test-client", + "version": "1.0.0", + }, + }, + } + + bodyBytes, _ := json.Marshal(initReq) + req := httptest.NewRequest("POST", "/mcp/safeoutputs", bytes.NewReader(bodyBytes)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + req.Header.Set("Authorization", "test-session-123") + + rr := httptest.NewRecorder() + httpServer.Handler.ServeHTTP(rr, req) + + t.Logf("Initialize response: %s", rr.Body.String()) + + // Now test calling a non-existent tool "foobar" + reqBody := map[string]interface{}{ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": map[string]interface{}{ + "name": "foobar", // Non-existent tool + "arguments": map[string]interface{}{}, + }, + } + + bodyBytes, err = json.Marshal(reqBody) + require.NoError(t, err) + + req = httptest.NewRequest("POST", "/mcp/safeoutputs", bytes.NewReader(bodyBytes)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + req.Header.Set("Authorization", "test-session-123") + + rr = httptest.NewRecorder() + httpServer.Handler.ServeHTTP(rr, req) + + resp := rr.Result() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + t.Logf("Response status: %d", resp.StatusCode) + t.Logf("Response body: %s", string(body)) + + // Parse SSE response using existing helper + jsonrpcResp := parseSSEResponse(t, bytes.NewReader(body)) + + // Verify an error is returned + errObj, hasError := jsonrpcResp["error"] + require.True(t, hasError, "Expected error for non-existent tool call") + + errorMap, ok := errObj.(map[string]interface{}) + require.True(t, ok, "Error should be a map") + + errorMsg := errorMap["message"].(string) + t.Logf("Error code: %v", errorMap["code"]) + t.Logf("Error message: %s", errorMsg) + + // Check that error message mentions the tool doesn't exist or is unknown + // The SDK or gateway should return an appropriate error + errorMsgLower := strings.ToLower(errorMsg) + assert.True(t, + strings.Contains(errorMsgLower, "tool") || + strings.Contains(errorMsgLower, "not found") || + strings.Contains(errorMsgLower, "unknown") || + strings.Contains(errorMsgLower, "foobar") || + strings.Contains(errorMsgLower, "handler") || + strings.Contains(errorMsgLower, "no tool"), + "Error message should indicate tool not found or unknown tool. Got: %s", errorMsg) +} + +// TestNonExistentToolCallLogging_UnifiedMode verifies that when a non-existent tool +// is called via unified mode with the correct prefix format, an appropriate error is returned. +func TestNonExistentToolCallLogging_UnifiedMode(t *testing.T) { + // Initialize logger to capture log output + logger.InitFileLogger("/tmp", "test-nonexistent-tool-unified.log") + defer logger.CloseGlobalLogger() + + cfg := &config.Config{ + Servers: map[string]*config.ServerConfig{ + "safeoutputs": {Command: "echo", Args: []string{}}, + }, + } + + ctx := context.Background() + us, err := NewUnified(ctx, cfg) + require.NoError(t, err, "Failed to create unified server") + defer us.Close() + + // Create HTTP server in unified mode + httpServer := CreateHTTPServerForMCP("127.0.0.1:0", us, "") + + // First, initialize the session + initReq := map[string]interface{}{ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": map[string]interface{}{ + "protocolVersion": "2024-11-05", + "capabilities": map[string]interface{}{}, + "clientInfo": map[string]interface{}{ + "name": "test-client", + "version": "1.0.0", + }, + }, + } + + bodyBytes, _ := json.Marshal(initReq) + req := httptest.NewRequest("POST", "/mcp", bytes.NewReader(bodyBytes)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + req.Header.Set("Authorization", "test-session-456") + + rr := httptest.NewRecorder() + httpServer.Handler.ServeHTTP(rr, req) + + t.Logf("Initialize response: %s", rr.Body.String()) + + // In unified mode, tool names should have the backend prefix + // Try calling "safeoutputs___foobar" which doesn't exist + reqBody := map[string]interface{}{ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": map[string]interface{}{ + "name": "safeoutputs___foobar", // Non-existent tool with prefix + "arguments": map[string]interface{}{}, + }, + } + + bodyBytes, err = json.Marshal(reqBody) + require.NoError(t, err) + + req = httptest.NewRequest("POST", "/mcp", bytes.NewReader(bodyBytes)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + req.Header.Set("Authorization", "test-session-456") + + rr = httptest.NewRecorder() + httpServer.Handler.ServeHTTP(rr, req) + + resp := rr.Result() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + t.Logf("Response status: %d", resp.StatusCode) + t.Logf("Response body: %s", string(body)) + + // Parse SSE response using existing helper + jsonrpcResp := parseSSEResponse(t, bytes.NewReader(body)) + + // Verify an error is returned + errObj, hasError := jsonrpcResp["error"] + require.True(t, hasError, "Expected error for non-existent tool call") + + errorMap, ok := errObj.(map[string]interface{}) + require.True(t, ok, "Error should be a map") + + errorMsg := errorMap["message"].(string) + t.Logf("Error code: %v", errorMap["code"]) + t.Logf("Error message: %s", errorMsg) + + // Check that error message indicates tool not found + errorMsgLower := strings.ToLower(errorMsg) + assert.True(t, + strings.Contains(errorMsgLower, "tool") || + strings.Contains(errorMsgLower, "not found") || + strings.Contains(errorMsgLower, "unknown") || + strings.Contains(errorMsgLower, "foobar") || + strings.Contains(errorMsgLower, "handler") || + strings.Contains(errorMsgLower, "no tool"), + "Error message should indicate tool not found. Got: %s", errorMsg) +} From d259b3f9a25f5d0f5bf128feac39cc87563c333b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 27 Jan 2026 16:14:04 +0000 Subject: [PATCH 3/3] Investigation complete - all requirements met Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- .../server/nonexistent_tool_logging_test.go | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/internal/server/nonexistent_tool_logging_test.go b/internal/server/nonexistent_tool_logging_test.go index a094642..c72cbc3 100644 --- a/internal/server/nonexistent_tool_logging_test.go +++ b/internal/server/nonexistent_tool_logging_test.go @@ -60,7 +60,7 @@ func TestNonExistentToolCallLogging_RoutedMode(t *testing.T) { rr := httptest.NewRecorder() httpServer.Handler.ServeHTTP(rr, req) - + t.Logf("Initialize response: %s", rr.Body.String()) // Now test calling a non-existent tool "foobar" @@ -98,10 +98,10 @@ func TestNonExistentToolCallLogging_RoutedMode(t *testing.T) { // Verify an error is returned errObj, hasError := jsonrpcResp["error"] require.True(t, hasError, "Expected error for non-existent tool call") - + errorMap, ok := errObj.(map[string]interface{}) require.True(t, ok, "Error should be a map") - + errorMsg := errorMap["message"].(string) t.Logf("Error code: %v", errorMap["code"]) t.Logf("Error message: %s", errorMsg) @@ -110,12 +110,12 @@ func TestNonExistentToolCallLogging_RoutedMode(t *testing.T) { // The SDK or gateway should return an appropriate error errorMsgLower := strings.ToLower(errorMsg) assert.True(t, - strings.Contains(errorMsgLower, "tool") || - strings.Contains(errorMsgLower, "not found") || - strings.Contains(errorMsgLower, "unknown") || - strings.Contains(errorMsgLower, "foobar") || - strings.Contains(errorMsgLower, "handler") || - strings.Contains(errorMsgLower, "no tool"), + strings.Contains(errorMsgLower, "tool") || + strings.Contains(errorMsgLower, "not found") || + strings.Contains(errorMsgLower, "unknown") || + strings.Contains(errorMsgLower, "foobar") || + strings.Contains(errorMsgLower, "handler") || + strings.Contains(errorMsgLower, "no tool"), "Error message should indicate tool not found or unknown tool. Got: %s", errorMsg) } @@ -163,7 +163,7 @@ func TestNonExistentToolCallLogging_UnifiedMode(t *testing.T) { rr := httptest.NewRecorder() httpServer.Handler.ServeHTTP(rr, req) - + t.Logf("Initialize response: %s", rr.Body.String()) // In unified mode, tool names should have the backend prefix @@ -202,10 +202,10 @@ func TestNonExistentToolCallLogging_UnifiedMode(t *testing.T) { // Verify an error is returned errObj, hasError := jsonrpcResp["error"] require.True(t, hasError, "Expected error for non-existent tool call") - + errorMap, ok := errObj.(map[string]interface{}) require.True(t, ok, "Error should be a map") - + errorMsg := errorMap["message"].(string) t.Logf("Error code: %v", errorMap["code"]) t.Logf("Error message: %s", errorMsg) @@ -213,11 +213,11 @@ func TestNonExistentToolCallLogging_UnifiedMode(t *testing.T) { // Check that error message indicates tool not found errorMsgLower := strings.ToLower(errorMsg) assert.True(t, - strings.Contains(errorMsgLower, "tool") || - strings.Contains(errorMsgLower, "not found") || - strings.Contains(errorMsgLower, "unknown") || - strings.Contains(errorMsgLower, "foobar") || - strings.Contains(errorMsgLower, "handler") || - strings.Contains(errorMsgLower, "no tool"), + strings.Contains(errorMsgLower, "tool") || + strings.Contains(errorMsgLower, "not found") || + strings.Contains(errorMsgLower, "unknown") || + strings.Contains(errorMsgLower, "foobar") || + strings.Contains(errorMsgLower, "handler") || + strings.Contains(errorMsgLower, "no tool"), "Error message should indicate tool not found. Got: %s", errorMsg) }