Skip to content

Commit fe3f4e3

Browse files
Dumbrisclaude
andauthored
fix(mcp): stop advertising code_execution while enable_code_execution is off (#1261)
* fix(mcp): stop advertising code_execution while enable_code_execution is off A disabled code_execution was still listed in tools/list on every surface that carries it (/mcp in the default retrieve_tools routing mode, /mcp/call, /mcp/code) as a "Code Execution (Disabled)" stub whose handler refused every call. Clients pick tools from the list, not from descriptions, so agents kept calling it and burning a round trip per session before falling back (#1236). - buildCodeExecutionTool returns nothing when the flag is off; the default surface now registers through the same builder (one definition). - RefreshCodeExecutionAvailability adds/removes the tool in place on the default surface (AddTools / DeleteTools) and rebuilds the routing-mode surfaces through content-guarded refreshers, so a runtime flip emits notifications/tools/list_changed exactly once per surface and an unrelated config reload emits nothing. The call-tool surface gains the same fingerprint guard the code-exec surface already had; both guards are seeded at init so the first reload does not re-announce an unchanged set. - The handler-level gate still refuses a call that arrives by name (REST, CallToolDirect, a cached listing) with the same wording. Goldens: the frozen retrieve_tools_mode / code_execution_mode goldens were captured with the flag off and carried the stub; they are regenerated with that single entry removed, and the enumerated-delta gates (toolslist snapshot + menu surface) now allow exactly that one removal per surface. The pre099 / pre-feature baselines are untouched. Closes #1236 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(code-execution): stale MCP tool lists get unknown-tool, not the enable hint Review round 1 (astra): an MCP tools/call for the withdrawn name is refused by mcp-go as an unknown tool; only REST/CLI callers reach the handler gate and its "enable it" message. Also drop the restart step from the "tool not found in tools list" troubleshooting entry — the flag is hot-reloaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 9fca30a commit fe3f4e3

13 files changed

Lines changed: 359 additions & 305 deletions

‎docs/code_execution/troubleshooting.md‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,12 @@ Common issues, error messages, and solutions for the `code_execution` tool.
3131
Error: code_execution is disabled in configuration. Set 'enable_code_execution: true' in config file.
3232
```
3333

34-
**Cause**: The `code_execution` feature is disabled by default for security.
34+
**Cause**: The `code_execution` feature is disabled by default for security. While it is disabled the tool is not listed in `tools/list`, so an MCP client will normally not see it at all; this error appears when a call reaches the handler by name anyway (the REST API or the CLI). An MCP client that calls the name from a stale tool list gets an unknown-tool error instead.
3535

3636
**Solution**:
3737
1. Edit your configuration file (`~/.mcpproxy/mcp_config.json`)
3838
2. Add or update: `"enable_code_execution": true`
39-
3. Restart mcpproxy: `pkill mcpproxy && mcpproxy serve`
39+
3. The flag is hot-reloaded: the running proxy picks up the change, advertises the tool, and sends `notifications/tools/list_changed` to connected sessions. No restart is needed; clients that do not honor `list_changed` need to reconnect to see the tool.
4040

4141
**Example Configuration**:
4242
```json
@@ -56,13 +56,12 @@ Error: code_execution is disabled in configuration. Set 'enable_code_execution:
5656

5757
**Symptom**: LLM agent lists available tools but `code_execution` is missing.
5858

59-
**Cause**: Feature is not enabled or server didn't restart after configuration change.
59+
**Cause**: Feature is not enabled — a disabled `code_execution` is deliberately absent from `tools/list` — or the client is holding a tool list from before the flag was enabled.
6060

6161
**Solution**:
6262
1. Verify configuration has `"enable_code_execution": true`
63-
2. Restart mcpproxy server
64-
3. Reconnect LLM client
65-
4. List tools again
63+
2. The change is hot-reloaded and announced with `notifications/tools/list_changed`; if the client does not honor that notification, reconnect it
64+
3. List tools again
6665

6766
**Verification**:
6867
```bash

‎docs/configuration.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1264,7 +1264,7 @@ See [Tool Quarantine](features/tool-quarantine.md) for complete details.
12641264

12651265
| Field | Type | Default | Description |
12661266
|-------|------|---------|-------------|
1267-
| `enable_code_execution` | boolean | `false` | Enable JavaScript/TypeScript code execution tool (disabled by default for security) |
1267+
| `enable_code_execution` | boolean | `false` | Enable JavaScript/TypeScript code execution tool (disabled by default for security). While `false` the `code_execution` tool is not advertised in `tools/list`. Hot-reloaded: a flip adds/removes the tool and sends `notifications/tools/list_changed` |
12681268
| `code_execution_timeout_ms` | integer | `120000` | Default timeout in milliseconds (1-600000, max 10 minutes) |
12691269
| `code_execution_max_tool_calls` | integer | `0` | Maximum tool calls per execution (0 = unlimited) |
12701270
| `code_execution_pool_size` | integer | `10` | Number of JavaScript VM instances in pool (1-100) |

‎docs/features/code-execution.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ Enable code execution in your config:
3737

3838
| Option | Type | Default | Description |
3939
|--------|------|---------|-------------|
40-
| `enable_code_execution` | boolean | `false` | Enable the code execution tool |
40+
| `enable_code_execution` | boolean | `false` | Enable the code execution tool. While `false` the tool is not listed in `tools/list`; a flip is hot-reloaded and announced with `notifications/tools/list_changed` |
4141
| `code_execution_timeout_ms` | integer | `120000` | Execution timeout (2 minutes) |
4242
| `code_execution_max_tool_calls` | integer | `0` | Max tool calls (0 = unlimited) |
4343
| `code_execution_pool_size` | integer | `10` | Number of VM instances to pool |

‎docs/features/routing-modes.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ Code execution mode is designed for multi-step orchestration workflows. Instead
165165
- Complex conditional logic that would require many round-trips in other modes
166166
- Data transformation pipelines (fetch from one tool, transform, send to another)
167167

168-
**Note:** Code execution must be enabled in config (`"enable_code_execution": true`). If disabled, the `code_execution` tool appears but returns an error message directing the user to enable it.
168+
**Note:** Code execution must be enabled in config (`"enable_code_execution": true`). While it is disabled, the `code_execution` tool is not listed in `tools/list` on any endpoint, so clients that pick tools from the list never attempt it. An MCP `tools/call` for the name is refused as an unknown tool; a call that reaches the handler through the REST API or the CLI is refused with an error directing the user to enable it. The flag is hot-reloadable: flipping it adds or removes the tool on the live surfaces and sends `notifications/tools/list_changed` to connected sessions, no restart required.
169169

170170
## Choosing the Right Mode
171171

‎internal/server/code_execution_hotreload_test.go‎

Lines changed: 192 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
package server
22

33
import (
4+
"context"
45
"testing"
56

7+
"github.com/mark3labs/mcp-go/mcp"
68
mcpserver "github.com/mark3labs/mcp-go/server"
79
"github.com/stretchr/testify/assert"
810
"github.com/stretchr/testify/require"
@@ -11,18 +13,18 @@ import (
1113
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
1214
)
1315

14-
// UX audit F16: Settings advertises enable_code_execution as an
15-
// instantly-applied field, but the tool surfaces built their code_execution
16-
// entry — the live tool, or a "disabled" stub whose handler refuses every call
17-
// unconditionally — exactly once, at construction. Flipping the flag on
18-
// therefore left the stub in place on /mcp, so `code_execution` kept answering
19-
// "Code execution is disabled" from a brand-new MCP session until a restart,
20-
// even though the handler-level gate (mcp_code_execution.go) had already gone
21-
// live. These tests pin the rebuild seam that makes the promise true.
22-
23-
func isDisabledCodeExecutionStub(tool mcpserver.ServerTool) bool {
24-
return tool.Tool.Annotations.Title == "Code Execution (Disabled)"
25-
}
16+
// Issue #1236: with enable_code_execution=false (the default) the tool surfaces
17+
// used to advertise a "disabled" code_execution stub whose handler refused every
18+
// call. Clients that pick tools from tools/list rather than from descriptions —
19+
// most of them — kept calling it and burning a round trip per attempt. The
20+
// contract is now: disabled → NOT advertised on any surface; a runtime toggle
21+
// (config hot-reload) re-advertises or withdraws the tool and emits
22+
// notifications/tools/list_changed so caching clients refetch. These tests pin
23+
// both halves.
24+
//
25+
// UX audit F16 (the prior shape of this file) established that the builder
26+
// resolves the flag on every call and that RefreshCodeExecutionAvailability is
27+
// the seam config.reloaded drives; those guarantees carry over unchanged.
2628

2729
func codeExecutionToolFrom(tools []mcpserver.ServerTool) *mcpserver.ServerTool {
2830
for i := range tools {
@@ -33,107 +35,223 @@ func codeExecutionToolFrom(tools []mcpserver.ServerTool) *mcpserver.ServerTool {
3335
return nil
3436
}
3537

36-
// TestBuildCodeExecutionTool_FollowsCurrentConfig: the builder must resolve the
37-
// flag on every call, not capture it. currentConfig() falls back to p.config
38-
// when no runtime is attached, so mutating that snapshot models a hot reload.
38+
// advertisedCodeExecution returns the registered code_execution entry on a
39+
// server, or nil when the surface does not list it.
40+
func advertisedCodeExecution(s *mcpserver.MCPServer) *mcpserver.ServerTool {
41+
tool, ok := s.ListTools()["code_execution"]
42+
if !ok {
43+
return nil
44+
}
45+
return tool
46+
}
47+
48+
// notifyingSession is an initialized mcp-go ClientSession whose notification
49+
// channel is captured, so a test can observe what the server pushed to a
50+
// connected client. It stands in for a real Streamable HTTP session.
51+
type notifyingSession struct {
52+
id string
53+
ch chan mcp.JSONRPCNotification
54+
}
55+
56+
func newNotifyingSession(id string) *notifyingSession {
57+
return &notifyingSession{id: id, ch: make(chan mcp.JSONRPCNotification, 16)}
58+
}
59+
60+
func (s *notifyingSession) Initialize() {}
61+
func (s *notifyingSession) Initialized() bool { return true }
62+
func (s *notifyingSession) NotificationChannel() chan<- mcp.JSONRPCNotification { return s.ch }
63+
func (s *notifyingSession) SessionID() string { return s.id }
64+
65+
// drainListChanged counts the notifications/tools/list_changed pushes queued
66+
// on the session and empties the channel.
67+
func (s *notifyingSession) drainListChanged() int {
68+
n := 0
69+
for {
70+
select {
71+
case notif := <-s.ch:
72+
if notif.Method == mcp.MethodNotificationToolsListChanged {
73+
n++
74+
}
75+
default:
76+
return n
77+
}
78+
}
79+
}
80+
81+
// newHotReloadProxy builds a proxy with all three built-in-carrying surfaces
82+
// registered the way production does it — the routing-mode servers through
83+
// initRoutingModeServers (which also seeds their content guards), the default
84+
// surface the way registerTools assembles it — plus one initialized session
85+
// per surface so list_changed pushes are observable.
86+
func newHotReloadProxy(t *testing.T, cfg *config.Config) (*MCPProxyServer, map[string]*notifyingSession) {
87+
t.Helper()
88+
p := &MCPProxyServer{
89+
config: cfg,
90+
logger: zap.NewNop(),
91+
server: mcpserver.NewMCPServer("test", "0.0.0", mcpserver.WithToolCapabilities(true)),
92+
}
93+
p.initRoutingModeServers()
94+
require.NotNil(t, p.callToolServer)
95+
require.NotNil(t, p.codeExecServer)
96+
// The default surface is assembled by registerTools; the call-tool set is
97+
// a faithful stand-in (same built-ins, same code_execution gating).
98+
for _, st := range p.buildCallToolModeTools() {
99+
p.server.AddTool(st.Tool, st.Handler)
100+
}
101+
102+
sessions := map[string]*notifyingSession{}
103+
for name, s := range map[string]*mcpserver.MCPServer{
104+
"call_tool": p.callToolServer,
105+
"code_exec": p.codeExecServer,
106+
"default": p.server,
107+
} {
108+
sess := newNotifyingSession("sess-" + name)
109+
require.NoError(t, s.RegisterSession(context.Background(), sess))
110+
sess.drainListChanged() // registration-time noise, if any
111+
sessions[name] = sess
112+
}
113+
return p, sessions
114+
}
115+
116+
// TestBuildCodeExecutionTool_FollowsCurrentConfig: the builder resolves the
117+
// flag on every call, and a disabled flag yields NO tool — not a stub.
118+
// currentConfig() falls back to p.config when no runtime is attached, so
119+
// mutating that snapshot models a hot reload.
39120
func TestBuildCodeExecutionTool_FollowsCurrentConfig(t *testing.T) {
40121
cfg := &config.Config{EnableCodeExecution: false}
41122
p := &MCPProxyServer{config: cfg}
42123

43-
disabled := p.buildCodeExecutionTool()
44-
require.Len(t, disabled, 1)
45-
assert.True(t, isDisabledCodeExecutionStub(disabled[0]), "expected the disabled stub while the flag is off")
124+
assert.Empty(t, p.buildCodeExecutionTool(),
125+
"issue #1236: a disabled code_execution must not be advertised at all")
46126

47127
cfg.EnableCodeExecution = true
48128
enabled := p.buildCodeExecutionTool()
49129
require.Len(t, enabled, 1)
50-
assert.False(t, isDisabledCodeExecutionStub(enabled[0]), "the live tool must appear once the flag is on")
130+
assert.Equal(t, "code_execution", enabled[0].Tool.Name)
51131
assert.Equal(t, codeExecutionToolDescription, enabled[0].Tool.Description)
132+
assert.Equal(t, "Code Execution", enabled[0].Tool.Annotations.Title)
52133

53134
cfg.EnableCodeExecution = false
54-
assert.True(t, isDisabledCodeExecutionStub(p.buildCodeExecutionTool()[0]),
55-
"turning the flag back off must restore the stub")
135+
assert.Empty(t, p.buildCodeExecutionTool(),
136+
"turning the flag back off must withdraw the tool again")
56137
}
57138

58-
// TestRefreshCodeExecutionAvailability_SwapsAdvertisedTool: the call-tool
59-
// surface (/mcp in the default retrieve_tools routing mode) and the code-exec
60-
// surface must both re-advertise code_execution after a config reload.
61-
func TestRefreshCodeExecutionAvailability_SwapsAdvertisedTool(t *testing.T) {
62-
cfg := &config.Config{EnableCodeExecution: false}
63-
p := &MCPProxyServer{
64-
config: cfg,
65-
logger: zap.NewNop(),
66-
callToolServer: mcpserver.NewMCPServer("test", "0.0.0"),
67-
codeExecServer: mcpserver.NewMCPServer("test", "0.0.0"),
68-
server: mcpserver.NewMCPServer("test", "0.0.0"),
69-
}
139+
// TestRoutingModeSurfaces_OmitDisabledCodeExecution: neither routing-mode
140+
// builder may carry code_execution while the flag is off. The retrieve_tools
141+
// surface is what /mcp serves in the default routing mode, so this is the
142+
// listing the issue reporter saw.
143+
func TestRoutingModeSurfaces_OmitDisabledCodeExecution(t *testing.T) {
144+
p := &MCPProxyServer{config: &config.Config{EnableCodeExecution: false}, logger: zap.NewNop()}
70145

71-
for _, st := range p.buildCallToolModeTools() {
72-
p.callToolServer.AddTool(st.Tool, st.Handler)
73-
}
74-
for _, st := range p.buildCodeExecModeTools() {
75-
p.codeExecServer.AddTool(st.Tool, st.Handler)
76-
}
146+
assert.Nil(t, codeExecutionToolFrom(p.buildCallToolModeTools()),
147+
"retrieve_tools mode (/mcp default) must not list a disabled code_execution")
148+
assert.Nil(t, codeExecutionToolFrom(p.buildCodeExecModeTools()),
149+
"code_execution mode must not list a disabled code_execution")
77150

78-
advertised := func(s *mcpserver.MCPServer) *mcpserver.ServerTool {
79-
tool, ok := s.ListTools()["code_execution"]
80-
if !ok {
81-
return nil
82-
}
83-
return tool
84-
}
151+
p.config.EnableCodeExecution = true
152+
assert.NotNil(t, codeExecutionToolFrom(p.buildCallToolModeTools()))
153+
assert.NotNil(t, codeExecutionToolFrom(p.buildCodeExecModeTools()))
154+
}
155+
156+
// TestRefreshCodeExecutionAvailability_TogglesAdvertisedTool: a hot enable
157+
// adds code_execution to every surface that carries it and a hot disable
158+
// withdraws it again — including the default surface, which is updated in
159+
// place (AddTools / DeleteTools) rather than rebuilt.
160+
func TestRefreshCodeExecutionAvailability_TogglesAdvertisedTool(t *testing.T) {
161+
cfg := &config.Config{EnableCodeExecution: false}
162+
p, _ := newHotReloadProxy(t, cfg)
85163

86-
require.NotNil(t, advertised(p.callToolServer))
87-
assert.True(t, isDisabledCodeExecutionStub(*advertised(p.callToolServer)))
88-
assert.True(t, isDisabledCodeExecutionStub(*advertised(p.codeExecServer)))
164+
require.Nil(t, advertisedCodeExecution(p.callToolServer), "precondition: disabled at startup")
165+
require.Nil(t, advertisedCodeExecution(p.codeExecServer))
166+
require.Nil(t, advertisedCodeExecution(p.server))
89167

90168
// The operator flips the toggle; ApplyConfig swaps the live snapshot and
91169
// emits config.reloaded, which drives this call.
92170
cfg.EnableCodeExecution = true
93171
p.RefreshCodeExecutionAvailability()
94172

95-
require.NotNil(t, advertised(p.callToolServer), "code_execution must still be advertised")
96-
assert.False(t, isDisabledCodeExecutionStub(*advertised(p.callToolServer)),
97-
"the call-tool surface must serve the live tool after a hot enable")
98-
assert.False(t, isDisabledCodeExecutionStub(*advertised(p.codeExecServer)),
99-
"the code-exec surface must serve the live tool after a hot enable")
173+
for name, s := range map[string]*mcpserver.MCPServer{
174+
"call_tool": p.callToolServer, "code_exec": p.codeExecServer, "default": p.server,
175+
} {
176+
tool := advertisedCodeExecution(s)
177+
require.NotNil(t, tool, "%s surface must advertise code_execution after a hot enable", name)
178+
assert.Equal(t, codeExecutionToolDescription, tool.Tool.Description,
179+
"%s surface must serve the live tool, never a stub", name)
180+
}
181+
182+
// And back off again: the tool disappears from every surface.
183+
cfg.EnableCodeExecution = false
184+
p.RefreshCodeExecutionAvailability()
185+
186+
assert.Nil(t, advertisedCodeExecution(p.callToolServer),
187+
"turning the flag off must withdraw code_execution from the retrieve_tools surface")
188+
assert.Nil(t, advertisedCodeExecution(p.codeExecServer),
189+
"turning the flag off must withdraw code_execution from the code-exec surface")
190+
assert.Nil(t, advertisedCodeExecution(p.server),
191+
"turning the flag off must withdraw code_execution from the default surface")
192+
}
193+
194+
// TestRefreshCodeExecutionAvailability_EmitsListChangedOnlyOnAFlip: a runtime
195+
// toggle must push notifications/tools/list_changed to connected sessions on
196+
// every affected surface (the issue's second expectation), and an unrelated
197+
// config reload — which also drives this refresh — must push nothing, or every
198+
// config edit looks to a client exactly like the tool set changing.
199+
func TestRefreshCodeExecutionAvailability_EmitsListChangedOnlyOnAFlip(t *testing.T) {
200+
cfg := &config.Config{EnableCodeExecution: false}
201+
p, sessions := newHotReloadProxy(t, cfg)
100202

101-
// The default/stdio surface is updated in place with AddTools, which must
102-
// not disturb the tools registerTools put there.
103-
require.NotNil(t, advertised(p.server))
104-
assert.False(t, isDisabledCodeExecutionStub(*advertised(p.server)))
203+
// Unrelated reload: same flag value, nothing to announce.
204+
p.RefreshCodeExecutionAvailability()
205+
p.RefreshCodeExecutionAvailability()
206+
for name, sess := range sessions {
207+
assert.Zero(t, sess.drainListChanged(),
208+
"%s surface: a reload that changes nothing must not emit list_changed", name)
209+
}
105210

106-
// And back off again.
211+
// Enable: exactly one list_changed per surface.
212+
cfg.EnableCodeExecution = true
213+
p.RefreshCodeExecutionAvailability()
214+
for name, sess := range sessions {
215+
assert.Equal(t, 1, sess.drainListChanged(),
216+
"%s surface: a hot enable must emit exactly one list_changed", name)
217+
}
218+
219+
// Steady state again: silence.
220+
p.RefreshCodeExecutionAvailability()
221+
for name, sess := range sessions {
222+
assert.Zero(t, sess.drainListChanged(),
223+
"%s surface: a reload after the flip must not re-announce it", name)
224+
}
225+
226+
// Disable: exactly one list_changed per surface.
107227
cfg.EnableCodeExecution = false
108228
p.RefreshCodeExecutionAvailability()
109-
assert.True(t, isDisabledCodeExecutionStub(*advertised(p.callToolServer)),
110-
"turning the flag off must restore the refusing stub")
229+
for name, sess := range sessions {
230+
assert.Equal(t, 1, sess.drainListChanged(),
231+
"%s surface: a hot disable must emit exactly one list_changed", name)
232+
}
111233
}
112234

113235
// TestRefreshCodeExecutionAvailability_PreservesOtherTools: the default surface
114-
// is refreshed with AddTools rather than SetTools precisely because its tool set
115-
// is assembled once by registerTools; SetTools would drop everything else.
236+
// is refreshed in place precisely because its tool set is assembled once by
237+
// registerTools; a rebuild would drop everything else. Neither the add nor the
238+
// remove may disturb the other registrations.
116239
func TestRefreshCodeExecutionAvailability_PreservesOtherTools(t *testing.T) {
117240
cfg := &config.Config{EnableCodeExecution: false}
118-
p := &MCPProxyServer{
119-
config: cfg,
120-
logger: zap.NewNop(),
121-
server: mcpserver.NewMCPServer("test", "0.0.0"),
122-
}
241+
p, _ := newHotReloadProxy(t, cfg)
123242

124-
callToolTools := p.buildCallToolModeTools()
125-
for _, st := range callToolTools {
126-
p.server.AddTool(st.Tool, st.Handler)
127-
}
128243
before := len(p.server.ListTools())
129244
require.Greater(t, before, 1)
130245

131246
cfg.EnableCodeExecution = true
132247
p.RefreshCodeExecutionAvailability()
248+
assert.Len(t, p.server.ListTools(), before+1,
249+
"a hot enable adds code_execution and nothing else")
133250

251+
cfg.EnableCodeExecution = false
252+
p.RefreshCodeExecutionAvailability()
134253
assert.Len(t, p.server.ListTools(), before,
135-
"refreshing code_execution must not add or drop any other tool")
136-
assert.NotNil(t, codeExecutionToolFrom(callToolTools), "sanity: the builder emits code_execution")
254+
"a hot disable removes code_execution and nothing else")
137255
}
138256

139257
// TestRefreshCodeExecutionAvailability_NilSafe: the refresh runs from the shared

0 commit comments

Comments
 (0)