Skip to content

Commit a6f3222

Browse files
lpcoxlpcox
authored
Fix gateway default mode to routed (#492)
Gateway was defaulting to unified mode when no `--routed` or `--unified` flag was specified, contradicting documentation and `run.sh` behavior. ## Changes - **Fixed default mode**: Changed `internal/cmd/root.go` to default to routed mode instead of unified ```go // Before: defaulted to unified mode := "unified" if routedMode { mode = "routed" } // After: defaults to routed mode := "routed" if unifiedMode { mode = "unified" } ``` - **Added tests**: Verified non-existent tool call handling in both modes (`nonexistent_tool_logging_test.go`) ## Tool Name Behavior Confirmed and tested tool name exposure differs by mode: | Mode | Endpoint | Tool Name Format | Example | |------|----------|------------------|---------| | Routed (default) | `/mcp/{serverID}` | No prefix | `"create-issue"` | | Unified | `/mcp` | With prefix | `"safeoutputs___create-issue"` | Non-existent tool calls properly return JSON-RPC errors and are logged via existing infrastructure. > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `nonexistent.local` > - Triggering command: `/tmp/go-build98642035/b269/launcher.test /tmp/go-build98642035/b269/launcher.test -test.testlogfile=/tmp/go-build98642035/b269/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo --local x_amd64/vet user.email pVvZn2gawien` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build98642035/b278/mcp.test /tmp/go-build98642035/b278/mcp.test -test.testlogfile=/tmp/go-build98642035/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/c-test.run=TestNonExistentToolCallLogging jXVyRLCVa x_amd64/vet pull.rebase boring/sig` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/githubnext/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
2 parents 2cd9101 + d259b3f commit a6f3222

2 files changed

Lines changed: 227 additions & 4 deletions

File tree

‎internal/cmd/root.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -254,10 +254,10 @@ func run(cmd *cobra.Command, args []string) error {
254254
log.Println("Parallel server launching enabled (default)")
255255
}
256256

257-
// Determine mode (default to unified if neither flag is set)
258-
mode := "unified"
259-
if routedMode {
260-
mode = "routed"
257+
// Determine mode (default to routed if neither flag is set)
258+
mode := "routed"
259+
if unifiedMode {
260+
mode = "unified"
261261
}
262262

263263
debugLog.Printf("Server mode: %s, DIFC enabled: %v", mode, cfg.EnableDIFC)

‎internal/server/nonexistent_tool_logging_test.go‎

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
package server
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"encoding/json"
7+
"io"
8+
"net/http/httptest"
9+
"strings"
10+
"testing"
11+
12+
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
14+
15+
"github.com/githubnext/gh-aw-mcpg/internal/config"
16+
"github.com/githubnext/gh-aw-mcpg/internal/logger"
17+
)
18+
19+
// TestNonExistentToolCallLogging_RoutedMode verifies that when a non-existent tool
20+
// is called via routed mode, an appropriate error is returned and logged.
21+
func TestNonExistentToolCallLogging_RoutedMode(t *testing.T) {
22+
// Initialize logger to capture log output
23+
logger.InitFileLogger("/tmp", "test-nonexistent-tool.log")
24+
defer logger.CloseGlobalLogger()
25+
26+
cfg := &config.Config{
27+
Servers: map[string]*config.ServerConfig{
28+
"safeoutputs": {Command: "echo", Args: []string{}},
29+
},
30+
}
31+
32+
ctx := context.Background()
33+
us, err := NewUnified(ctx, cfg)
34+
require.NoError(t, err, "Failed to create unified server")
35+
defer us.Close()
36+
37+
// Create HTTP server in routed mode
38+
httpServer := CreateHTTPServerForRoutedMode("127.0.0.1:0", us, "")
39+
40+
// First, initialize the session with initialize request
41+
initReq := map[string]interface{}{
42+
"jsonrpc": "2.0",
43+
"id": 1,
44+
"method": "initialize",
45+
"params": map[string]interface{}{
46+
"protocolVersion": "2024-11-05",
47+
"capabilities": map[string]interface{}{},
48+
"clientInfo": map[string]interface{}{
49+
"name": "test-client",
50+
"version": "1.0.0",
51+
},
52+
},
53+
}
54+
55+
bodyBytes, _ := json.Marshal(initReq)
56+
req := httptest.NewRequest("POST", "/mcp/safeoutputs", bytes.NewReader(bodyBytes))
57+
req.Header.Set("Content-Type", "application/json")
58+
req.Header.Set("Accept", "application/json, text/event-stream")
59+
req.Header.Set("Authorization", "test-session-123")
60+
61+
rr := httptest.NewRecorder()
62+
httpServer.Handler.ServeHTTP(rr, req)
63+
64+
t.Logf("Initialize response: %s", rr.Body.String())
65+
66+
// Now test calling a non-existent tool "foobar"
67+
reqBody := map[string]interface{}{
68+
"jsonrpc": "2.0",
69+
"id": 2,
70+
"method": "tools/call",
71+
"params": map[string]interface{}{
72+
"name": "foobar", // Non-existent tool
73+
"arguments": map[string]interface{}{},
74+
},
75+
}
76+
77+
bodyBytes, err = json.Marshal(reqBody)
78+
require.NoError(t, err)
79+
80+
req = httptest.NewRequest("POST", "/mcp/safeoutputs", bytes.NewReader(bodyBytes))
81+
req.Header.Set("Content-Type", "application/json")
82+
req.Header.Set("Accept", "application/json, text/event-stream")
83+
req.Header.Set("Authorization", "test-session-123")
84+
85+
rr = httptest.NewRecorder()
86+
httpServer.Handler.ServeHTTP(rr, req)
87+
88+
resp := rr.Result()
89+
body, err := io.ReadAll(resp.Body)
90+
require.NoError(t, err)
91+
92+
t.Logf("Response status: %d", resp.StatusCode)
93+
t.Logf("Response body: %s", string(body))
94+
95+
// Parse SSE response using existing helper
96+
jsonrpcResp := parseSSEResponse(t, bytes.NewReader(body))
97+
98+
// Verify an error is returned
99+
errObj, hasError := jsonrpcResp["error"]
100+
require.True(t, hasError, "Expected error for non-existent tool call")
101+
102+
errorMap, ok := errObj.(map[string]interface{})
103+
require.True(t, ok, "Error should be a map")
104+
105+
errorMsg := errorMap["message"].(string)
106+
t.Logf("Error code: %v", errorMap["code"])
107+
t.Logf("Error message: %s", errorMsg)
108+
109+
// Check that error message mentions the tool doesn't exist or is unknown
110+
// The SDK or gateway should return an appropriate error
111+
errorMsgLower := strings.ToLower(errorMsg)
112+
assert.True(t,
113+
strings.Contains(errorMsgLower, "tool") ||
114+
strings.Contains(errorMsgLower, "not found") ||
115+
strings.Contains(errorMsgLower, "unknown") ||
116+
strings.Contains(errorMsgLower, "foobar") ||
117+
strings.Contains(errorMsgLower, "handler") ||
118+
strings.Contains(errorMsgLower, "no tool"),
119+
"Error message should indicate tool not found or unknown tool. Got: %s", errorMsg)
120+
}
121+
122+
// TestNonExistentToolCallLogging_UnifiedMode verifies that when a non-existent tool
123+
// is called via unified mode with the correct prefix format, an appropriate error is returned.
124+
func TestNonExistentToolCallLogging_UnifiedMode(t *testing.T) {
125+
// Initialize logger to capture log output
126+
logger.InitFileLogger("/tmp", "test-nonexistent-tool-unified.log")
127+
defer logger.CloseGlobalLogger()
128+
129+
cfg := &config.Config{
130+
Servers: map[string]*config.ServerConfig{
131+
"safeoutputs": {Command: "echo", Args: []string{}},
132+
},
133+
}
134+
135+
ctx := context.Background()
136+
us, err := NewUnified(ctx, cfg)
137+
require.NoError(t, err, "Failed to create unified server")
138+
defer us.Close()
139+
140+
// Create HTTP server in unified mode
141+
httpServer := CreateHTTPServerForMCP("127.0.0.1:0", us, "")
142+
143+
// First, initialize the session
144+
initReq := map[string]interface{}{
145+
"jsonrpc": "2.0",
146+
"id": 1,
147+
"method": "initialize",
148+
"params": map[string]interface{}{
149+
"protocolVersion": "2024-11-05",
150+
"capabilities": map[string]interface{}{},
151+
"clientInfo": map[string]interface{}{
152+
"name": "test-client",
153+
"version": "1.0.0",
154+
},
155+
},
156+
}
157+
158+
bodyBytes, _ := json.Marshal(initReq)
159+
req := httptest.NewRequest("POST", "/mcp", bytes.NewReader(bodyBytes))
160+
req.Header.Set("Content-Type", "application/json")
161+
req.Header.Set("Accept", "application/json, text/event-stream")
162+
req.Header.Set("Authorization", "test-session-456")
163+
164+
rr := httptest.NewRecorder()
165+
httpServer.Handler.ServeHTTP(rr, req)
166+
167+
t.Logf("Initialize response: %s", rr.Body.String())
168+
169+
// In unified mode, tool names should have the backend prefix
170+
// Try calling "safeoutputs___foobar" which doesn't exist
171+
reqBody := map[string]interface{}{
172+
"jsonrpc": "2.0",
173+
"id": 2,
174+
"method": "tools/call",
175+
"params": map[string]interface{}{
176+
"name": "safeoutputs___foobar", // Non-existent tool with prefix
177+
"arguments": map[string]interface{}{},
178+
},
179+
}
180+
181+
bodyBytes, err = json.Marshal(reqBody)
182+
require.NoError(t, err)
183+
184+
req = httptest.NewRequest("POST", "/mcp", bytes.NewReader(bodyBytes))
185+
req.Header.Set("Content-Type", "application/json")
186+
req.Header.Set("Accept", "application/json, text/event-stream")
187+
req.Header.Set("Authorization", "test-session-456")
188+
189+
rr = httptest.NewRecorder()
190+
httpServer.Handler.ServeHTTP(rr, req)
191+
192+
resp := rr.Result()
193+
body, err := io.ReadAll(resp.Body)
194+
require.NoError(t, err)
195+
196+
t.Logf("Response status: %d", resp.StatusCode)
197+
t.Logf("Response body: %s", string(body))
198+
199+
// Parse SSE response using existing helper
200+
jsonrpcResp := parseSSEResponse(t, bytes.NewReader(body))
201+
202+
// Verify an error is returned
203+
errObj, hasError := jsonrpcResp["error"]
204+
require.True(t, hasError, "Expected error for non-existent tool call")
205+
206+
errorMap, ok := errObj.(map[string]interface{})
207+
require.True(t, ok, "Error should be a map")
208+
209+
errorMsg := errorMap["message"].(string)
210+
t.Logf("Error code: %v", errorMap["code"])
211+
t.Logf("Error message: %s", errorMsg)
212+
213+
// Check that error message indicates tool not found
214+
errorMsgLower := strings.ToLower(errorMsg)
215+
assert.True(t,
216+
strings.Contains(errorMsgLower, "tool") ||
217+
strings.Contains(errorMsgLower, "not found") ||
218+
strings.Contains(errorMsgLower, "unknown") ||
219+
strings.Contains(errorMsgLower, "foobar") ||
220+
strings.Contains(errorMsgLower, "handler") ||
221+
strings.Contains(errorMsgLower, "no tool"),
222+
"Error message should indicate tool not found. Got: %s", errorMsg)
223+
}

0 commit comments

Comments
 (0)