fix(mcp): stop advertising code_execution while enable_code_execution is off - #1261
Merged
Conversation
… 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>
…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>
Deploying mcpproxy-docs with
|
| Latest commit: |
931646a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://acb18d05.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-1236-hide-disabled-code.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 34675287129 --repo smart-mcp-proxy/mcpproxy-go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
With
enable_code_execution: false(the default),tools/liston/mcpstill advertisedcode_execution. Only the call handler refused it. Clients select tools from the list, not from descriptions (which harnesses routinely truncate), so agents kept calling it and burning a round trip per session before falling back to another path.Closes #1236
Root cause
buildCodeExecutionTool(internal/server/mcp_routing.go) returned a"Code Execution (Disabled)"stub — a registered tool whose handler always refused — whenever the flag was off. Both routing-mode surfaces (/mcpin the defaultretrieve_toolsmode,/mcp/call,/mcp/code) appended it unconditionally. The stub dates from PR #339, which treated a descriptive refusal as better than an unknown-tool error; in practice a disabled capability that is still advertised is indistinguishable from an available one.A second, smaller problem:
RefreshCallToolModeToolsranSetToolson everyconfig.reloaded, so every unrelated config edit pushednotifications/tools/list_changedto every connected session on the default/mcpsurface.Fix
buildCodeExecutionToolreturns nothing while the flag is off. The default surface (registerToolsinmcp.go) now registers through the same builder, so there is one definition of the tool.RefreshCodeExecutionAvailability(driven byconfig.reloaded) applies a runtime flip on every surface that carries the tool:RefreshCallToolModeToolsgains the same fingerprint guardRefreshCodeExecModeToolsalready had; both guards are seeded at init so the first reload does not re-announce an unchanged set);AddToolson enable,DeleteToolson disable — and only when its state actually differs from the flag (mcp-go'sAddToolsnotifies even for an empty batch).notifications/tools/list_changedper surface; an unrelated reload emits none.mcp_code_execution.gois untouched: REST / CLI /CallToolDirectcallers still get the "enable it" message. An MCPtools/callfrom a stale list now gets mcp-go's unknown-tool error (documented).docs/features/routing-modes.md,docs/features/code-execution.md,docs/configuration.md,docs/code_execution/troubleshooting.mdno longer say the tool "appears but returns an error" or that a restart is needed.Golden tests (deliberate update)
Three frozen tool-surface gates exist. The frozen baselines (
testdata/toolslist_goldens/pre099/*.json,testdata/tools_list_prefeature.golden.json) were captured with the flag off and therefore contain the stub — they are untouched. The current goldenstoolslist_goldens/retrieve_tools_mode.jsonandcode_execution_mode.jsonwere regenerated viaMCPPROXY_WRITE_TOOLSLIST_GOLDENS; the only delta is the removedcode_executionentry (verified withjqdiff).default_server.jsonis unchanged (that surface never carried the stub). The two enumerated-delta gates (toolsListAllowedRemovalsintoolslist_snapshot_test.go,wantRemovedinmcp_menu_surface_test.go) now allow exactly that one removal per surface and still fail on any other addition, removal, or schema change.assertCodeExecutionDeltawas deleted because it only ever compared the disabled stub; the live tool'sscriptparam / non-requiredcoderemain pinned byTestCodeExecutionRegistrations_ScriptParamon all three surfaces.Testing
Failing tests were written first (
internal/server/code_execution_hotreload_test.go, rewritten from the F16 stub tests):TestBuildCodeExecutionTool_FollowsCurrentConfig— disabled → no tool; enabled → live tool; back off → none.TestRoutingModeSurfaces_OmitDisabledCodeExecution— neither routing-mode builder lists it while disabled.TestRefreshCodeExecutionAvailability_TogglesAdvertisedTool— hot enable adds it on all three surfaces, hot disable withdraws it.TestRefreshCodeExecutionAvailability_EmitsListChangedOnlyOnAFlip— registers an initialized session per surface and asserts exactly onelist_changedper flip and zero on unchanged reloads (this one also caught the pre-existing per-reload churn).TestRefreshCodeExecutionAvailability_PreservesOtherTools— the in-place add/remove touches nothing else.TestDisabledCodeExecutionNotRegistered,TestCodeExecutionDisabled_NotRegisteredButScriptCallStillExplained, the "wording matches" subtest ofTestCodeExecution_DisabledGateCoversEveryDispatch.Verification run locally:
go build ./...andgo build -tags server ./cmd/mcpproxygo test -race -count=1 ./internal/server/(with the CI-skipregex) — okgo test -race -count=1 ./internal/httpapi/ ./internal/runtime/ ./internal/config/— okgolangci-lint run --config .github/.golangci.yml ./internal/server/...— 0 issuesopencode(github-copilot/gpt-6-astra) in three file-scoped chunks: production code CLEAN, test/golden gates CLEAN, docs FINDINGS (1 — the "cached list still gets the enable hint" claim was wrong; fixed in the second commit), re-review CLEAN.Not run here:
./scripts/test-api-e2e.sh(CI).🤖 Generated with Claude Code