Files
coder/coderd/x/chatd/chattool/mcpworkspace_test.go
T
Kyle Carberry 58f70b4488 fix(coderd/x/chatd): sanitize workspace MCP tool names (#26928)
## Summary

Workspace MCP tools (servers a workspace declares in `.mcp.json`) take
their model-facing name from the server key joined with the tool name as
`serverName__toolName`. That name reached the model **unsanitized**, so
a server or tool name containing a character outside
`^[a-zA-Z0-9_-]{1,128}$` (for example `@`) produced an invalid tool
name. Anthropic and Bedrock reject the whole request with `HTTP 400`:

```
tools.N.custom.name: String should match pattern '^[a-zA-Z0-9_-]{1,128}$'
```

which fails the entire turn, not just the one tool. The remote MCP path
(`mcpclient`) and the AI Gateway path (`aibridge/mcp`) already sanitize;
the workspace path did not.

Alternative to #26853 (thanks @ibdafna for the report and repro).

## Fix

Sanitize and length-cap the **model-facing** name, and keep the original
`serverName__toolName` as a `routingName` the workspace agent uses to
reach the original server and tool. `NewWorkspaceMCPTools` builds a
whole set and disambiguates names that collide after sanitization (for
example server keys `foo.bar` and `foo_bar` both exposing `echo`) so
every tool stays addressable in the model's name-keyed dispatch map.
Names already within the allowed set are unchanged, so there is no
behavior change for valid names.

The sanitizer is local to `coderd/x/chatd/chattool`; the fix does
**not** touch the `aibridge` package or the remote MCP client.

### Changes
- `coderd/x/chatd/chattool/mcpworkspace.go`: local provider-safe
sanitizer + length cap, `routingName` for the agent proxy, and
`NewWorkspaceMCPTools` for set-level collision disambiguation.
- `coderd/x/chatd/chatd.go`: build the pinned workspace tool set via
`NewWorkspaceMCPTools`.

## Why sanitize here (not at `.mcp.json` / agent parse)?

The agent uses `serverName__toolName` to route to the real downstream
server (it splits on `__` and calls the original tool name), so
sanitizing at parse time would break routing or merely relocate the
original->sanitized mapping. Sanitization is also a provider constraint
the agent has no knowledge of, and coderd/agent version skew means
coderd must sanitize at its own boundary regardless. The model-facing
boundary in chatd is the right place.

## Test plan
- `@` in a name is sanitized for the model while the original routes to
the agent; a valid name is unchanged; an over-length name is truncated;
colliding names in a set are disambiguated while each still routes to
its own original name.
- `go build`, `go vet`, `golangci-lint`, and `go test
./coderd/x/chatd/chattool/...` pass locally.

<details>
<summary>Design notes / decision log</summary>

**Constraint that drives the design.** The tool name is both the
identifier shown to the model (and the key the model layer dispatches
tool calls by) and, for the workspace path, the string the agent splits
on `__` to route back to the original server and tool. Those roles
conflict once sanitization changes the name, so the name is sanitized
for the model while the unsanitized form is kept as `routingName`.

**Options considered.**
1. **Chosen:** sanitize in the workspace path only, with helpers local
to `chattool`. Smallest blast radius; no new cross-package dependency.
This matches the shape of the other MCP paths (`mcpclient` keeps
`originalName` + `configID`) without sharing code.
2. Sanitize at `.mcp.json` parse time or in the agent. Rejected: breaks
routing (the agent needs the original name), pushes a provider concern
into the agent, and coderd must still defend its own boundary because
the agent and coderd version independently. Tool names also come from
the downstream server at list time, not from `.mcp.json`, so parsing
cannot fully validate them.
3. Extract a shared sanitize/truncate/dedupe helper into `aibridge/mcp`
and adopt it in `mcpclient` too (so the remote path also gains collision
disambiguation). This DRYs all paths, but it grows chatd's coupling to
the `aibridge` subsystem and expands scope/behavior/tests in the remote
path for what is a workspace-path bug. Left out deliberately to keep
this change minimal and self-contained; it can be a separate refactor.
4. Sanitize once at the provider serialization boundary (chat loop). The
only truly generic spot, but the model dispatches by name, so it needs a
reverse (sanitized -> original) mapping and set-wide collision handling
in the model layer. Larger, riskier change.

**Notes.**
- The workspace path defines its own sanitizer (`[^a-zA-Z0-9_-]` -> `_`)
and a `maxModelToolNameLen = 64` constant that mirrors the strictest
provider limit (OpenAI 64, Bedrock 128), rather than importing
`aibridge/mcp`, so it carries no new dependency.
- The set builder sorts before assigning suffixes so disambiguation is
stable across turns.

</details>

---

_Opened by Coder Agents on behalf of @kylecarbs. Alternative to #26853._
2026-07-01 20:34:25 +02:00

281 lines
8.4 KiB
Go

package chattool_test
import (
"context"
"net/http"
"strings"
"sync/atomic"
"testing"
"charm.land/fantasy"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/coder/coder/v2/coderd/x/chatd/chattool"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/codersdk/workspacesdk"
)
// fakeAgentConn implements just enough of workspacesdk.AgentConn
// for testing CallMCPTool.
type fakeAgentConn struct {
workspacesdk.AgentConn
callMCPToolFunc func(ctx context.Context, req workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error)
}
func (f *fakeAgentConn) CallMCPTool(ctx context.Context, req workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
return f.callMCPToolFunc(ctx, req)
}
func TestWorkspaceMCPTool_InvalidateOn404(t *testing.T) {
t.Parallel()
t.Run("404ErrorInvalidatesCache", func(t *testing.T) {
t.Parallel()
var invalidated atomic.Bool
tool := chattool.NewWorkspaceMCPTool(
workspacesdk.MCPToolInfo{
Name: "test__echo",
Description: "test tool",
},
func(ctx context.Context) (workspacesdk.AgentConn, error) {
return &fakeAgentConn{
callMCPToolFunc: func(_ context.Context, _ workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
return workspacesdk.CallMCPToolResponse{}, codersdk.NewError(
http.StatusNotFound,
codersdk.Response{
Message: "MCP tool call failed.",
Detail: `unknown MCP server: "test"`,
},
)
},
}, nil
},
func() { invalidated.Store(true) },
)
resp, err := tool.Run(context.Background(), fantasy.ToolCall{})
require.NoError(t, err)
assert.True(t, resp.IsError, "response should be an error")
assert.True(t, invalidated.Load(),
"invalidateCache should fire on 404")
})
t.Run("Non404DoesNotInvalidate", func(t *testing.T) {
t.Parallel()
var invalidated atomic.Bool
tool := chattool.NewWorkspaceMCPTool(
workspacesdk.MCPToolInfo{
Name: "test__echo",
Description: "test tool",
},
func(ctx context.Context) (workspacesdk.AgentConn, error) {
return &fakeAgentConn{
callMCPToolFunc: func(_ context.Context, _ workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
return workspacesdk.CallMCPToolResponse{}, codersdk.NewError(
http.StatusBadGateway,
codersdk.Response{
Message: "Bad Gateway",
},
)
},
}, nil
},
func() { invalidated.Store(true) },
)
resp, err := tool.Run(context.Background(), fantasy.ToolCall{})
require.NoError(t, err)
assert.True(t, resp.IsError)
assert.False(t, invalidated.Load(),
"invalidateCache should NOT fire on non-404 error")
})
t.Run("ToolLevelErrorNoInvalidation", func(t *testing.T) {
t.Parallel()
var invalidated atomic.Bool
tool := chattool.NewWorkspaceMCPTool(
workspacesdk.MCPToolInfo{
Name: "test__echo",
Description: "test tool",
},
func(ctx context.Context) (workspacesdk.AgentConn, error) {
return &fakeAgentConn{
callMCPToolFunc: func(_ context.Context, _ workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
return workspacesdk.CallMCPToolResponse{
IsError: true,
Content: []workspacesdk.MCPToolContent{
{Type: "text", Text: "tool error"},
},
}, nil
},
}, nil
},
func() { invalidated.Store(true) },
)
resp, err := tool.Run(context.Background(), fantasy.ToolCall{})
require.NoError(t, err)
assert.True(t, resp.IsError)
assert.False(t, invalidated.Load(),
"invalidateCache should NOT fire on tool-level error (HTTP 200)")
})
t.Run("NilInvalidateCallbackSafe", func(t *testing.T) {
t.Parallel()
tool := chattool.NewWorkspaceMCPTool(
workspacesdk.MCPToolInfo{
Name: "test__echo",
Description: "test tool",
},
func(ctx context.Context) (workspacesdk.AgentConn, error) {
return &fakeAgentConn{
callMCPToolFunc: func(_ context.Context, _ workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
return workspacesdk.CallMCPToolResponse{}, codersdk.NewError(
http.StatusNotFound,
codersdk.Response{
Message: "MCP tool call failed.",
Detail: `unknown MCP server: "test"`,
},
)
},
}, nil
},
nil,
)
// Should not panic.
resp, err := tool.Run(context.Background(), fantasy.ToolCall{})
require.NoError(t, err)
assert.True(t, resp.IsError)
})
}
func TestWorkspaceMCPTool_SanitizesModelNameKeepsRoutingName(t *testing.T) {
t.Parallel()
t.Run("InvalidCharsSanitizedForModelOriginalForRouting", func(t *testing.T) {
t.Parallel()
var gotToolName string
tool := chattool.NewWorkspaceMCPTool(
workspacesdk.MCPToolInfo{
// "@" is outside the provider's allowed tool-name set; the
// model must never see it or the whole request is rejected.
Name: "weather@home__get_forecast",
Description: "test tool",
},
func(_ context.Context) (workspacesdk.AgentConn, error) {
return &fakeAgentConn{
callMCPToolFunc: func(_ context.Context, req workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
gotToolName = req.ToolName
return workspacesdk.CallMCPToolResponse{
Content: []workspacesdk.MCPToolContent{{Type: "text", Text: "ok"}},
}, nil
},
}, nil
},
nil,
)
// The model-facing name is sanitized to the provider-safe set.
assert.Equal(t, "weather_home__get_forecast", tool.Info().Name)
resp, err := tool.Run(context.Background(), fantasy.ToolCall{})
require.NoError(t, err)
assert.False(t, resp.IsError)
// The agent receives the original name so it can route the call to
// the correct server and original tool.
assert.Equal(t, "weather@home__get_forecast", gotToolName)
})
t.Run("ValidNameUnchanged", func(t *testing.T) {
t.Parallel()
tool := chattool.NewWorkspaceMCPTool(
workspacesdk.MCPToolInfo{
Name: "github__create_issue",
Description: "test tool",
},
func(_ context.Context) (workspacesdk.AgentConn, error) {
return &fakeAgentConn{
callMCPToolFunc: func(_ context.Context, _ workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
return workspacesdk.CallMCPToolResponse{}, nil
},
}, nil
},
nil,
)
// A name already within the allowed set is left untouched.
assert.Equal(t, "github__create_issue", tool.Info().Name)
})
t.Run("LongNameTruncatedForModel", func(t *testing.T) {
t.Parallel()
// A name longer than the provider limit is truncated. "srv__" plus a
// 64-char tool name exceeds the 64-char cap.
longName := "srv__" + strings.Repeat("a", 64)
tool := chattool.NewWorkspaceMCPTool(
workspacesdk.MCPToolInfo{
Name: longName,
Description: "test tool",
},
func(_ context.Context) (workspacesdk.AgentConn, error) {
return &fakeAgentConn{
callMCPToolFunc: func(_ context.Context, _ workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
return workspacesdk.CallMCPToolResponse{}, nil
},
}, nil
},
nil,
)
// The model-facing name is capped at the strictest provider limit.
assert.LessOrEqual(t, len(tool.Info().Name), 64)
})
}
func TestNewWorkspaceMCPTools_DisambiguatesCollidingNames(t *testing.T) {
t.Parallel()
var routed []string
getConn := func(_ context.Context) (workspacesdk.AgentConn, error) {
return &fakeAgentConn{
callMCPToolFunc: func(_ context.Context, req workspacesdk.CallMCPToolRequest) (workspacesdk.CallMCPToolResponse, error) {
routed = append(routed, req.ToolName)
return workspacesdk.CallMCPToolResponse{}, nil
},
}, nil
}
// Both names sanitize to "foo_bar__echo"; the set builder must keep them
// distinct for the model while routing each to its own original name.
infos := []workspacesdk.MCPToolInfo{
{Name: "foo.bar__echo"},
{Name: "foo_bar__echo"},
}
tools := chattool.NewWorkspaceMCPTools(infos, getConn, nil)
require.Len(t, tools, 2)
names := []string{tools[0].Info().Name, tools[1].Info().Name}
assert.NotEqual(t, names[0], names[1],
"colliding model-facing names must be disambiguated")
assert.ElementsMatch(t,
[]string{"foo_bar__echo", "foo_bar__echo_2"}, names)
// Each tool routes to its own original (unsanitized) name.
for _, tl := range tools {
_, err := tl.Run(context.Background(), fantasy.ToolCall{})
require.NoError(t, err)
}
assert.ElementsMatch(t,
[]string{"foo.bar__echo", "foo_bar__echo"}, routed)
}