fix(chat): built-in tools allowlist no longer filters user-configured MCP servers#41
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ld invariant Rename stale "Tools" allowlist references in comments to BuiltinTools after the field rename, drop an orphaned SendMessage comment fragment, and document that mcpToolFilter must be rebuilt per turn (never cached) to avoid stale cfgSessions pointers after ReconcileMCPServers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Summary
Fixes a bug diagnosed via live debugging: wiz's built-in-tools allowlist (
cfg.Tools, used by nib-desktop to trim the prompt for small/CPU models to a fixed 9-tool set) was silently filtering out tools from every user-configured MCP server too — not just built-ins. A user could add a fully working remote MCP server (verified independently viacurlandnib mcp test, returning 9 real tools), and none of them would ever reach the model, with no error shown anywhere — defeating the entire point of adding one.types.Config.Tools→types.Config.BuiltinTools(clean rename, no deprecated alias — this field is young enough that a break is low-risk).cogito.WithMCPToolFilter) now bypasses the allowlist for any tool call arriving on a session sourced fromSession.cfgClients(a user-configured MCP server), using the*mcp.ClientSessionidentity the filter callback already receives but previously discarded. Session-identity-based, not name-based, so a coincidentally-named built-in tool can't spoof a bypass.add_mcp_server,list_mcp_servers, etc.) still respect the allowlist exactly as before — an explicit design decision confirmed during brainstorming ("we want to have a minimum set + the ones that the user configures via MCP"), not an oversight.Design spec:
docs/superpowers/specs/2026-07-01-builtin-tools-allowlist-scoping.md(local-only, gitignored, not part of this diff)Implementation plan:
docs/superpowers/plans/2026-07-01-builtin-tools-allowlist-scoping.md(same)This is phase 1 of 2. nib-desktop needs a mechanical 2-line follow-up (
internal/agent/agent.go:130,155-156, the only file referencing the renamed field) once this merges and itsgithub.com/mudler/nibdependency bumps — tracked as a separate piece of work, not part of this PR.Test plan
go build ./...,go vet ./...,go test ./...all clean across every packagefunc(_ *mcp.ClientSession, name string) bool { return s.toolEnabled(name) }), proving this closes the exact gap found during live debuggingtoolEnabledcall sites outside the MCP filter (native tool gates, the 10-tool self-config loop) are untouched —TestSelfConfigToolDefinitionsCountstill asserts exactly 10mcpToolsFromTransport→filter(session, tool.Name)) to confirm the session pointers genuinely match by identity, not just in the isolated unit test🤖 Generated with Claude Code