diff --git a/docs/mcp.md b/docs/mcp.md index 7a1be69b..8ddd052f 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -161,6 +161,8 @@ A spec whose first token isn't a known preset (`search_symbols,find_files,…`) **Prompt-injection screening.** Every tool call is screened by middleware that scans arguments and result text for injection patterns. On a hit it attaches a non-blocking `_meta.gortex_security` advisory — the call still succeeds and the result body is never mutated. Disable with `GORTEX_MCP_SANITIZE=0`. +**Unknown-option guard.** Tools published with a closed schema (`additionalProperties: false`) enforce it at dispatch. By default an unknown option still executes the call and the result carries an `_ignored_options` rider naming the unknown keys and the valid ones — the self-correct signal for a mistyped or hallucinated option (#597). `GORTEX_TOOL_ARG_GUARD=reject` upgrades that to a refusal before the handler runs; `GORTEX_TOOL_ARG_GUARD=0` / `false` / `off` / `no` disables enforcement. Response-shaping keys generic layers honor on any tool (`format`, `fields`, `max_bytes`, `max_tokens`, `cursor`) are always accepted, and facade tools are exempt — their compatibility wrappers deliberately take legacy call shapes. + ## Core navigation | Tool | Description | diff --git a/internal/hooks/subagent.go b/internal/hooks/subagent.go index 75aafeec..184cf6c1 100644 --- a/internal/hooks/subagent.go +++ b/internal/hooks/subagent.go @@ -85,11 +85,13 @@ const gortexToolGuidance = "### MUST use Gortex MCP tools instead of Read/Grep/G "4. Call `capabilities` only when an operation's exact fields are unknown.\n" // renderTaskContext calls smart_context with the subagent task text and -// returns a compacted body. Falls back to empty on any error. +// returns a capped body. Falls back to empty on any error. Only declared +// options ride the call — smart_context has no compact option, and an +// undeclared key would draw the dispatch arg guard's rider into the +// briefing text. func renderTaskContext(port int, task string) string { raw := callServerTool(port, "smart_context", map[string]any{ - "task": task, - "compact": true, + "task": task, }) raw = strings.TrimSpace(raw) if raw == "" { diff --git a/internal/mcp/arg_schema_guard.go b/internal/mcp/arg_schema_guard.go new file mode 100644 index 00000000..3576b287 --- /dev/null +++ b/internal/mcp/arg_schema_guard.go @@ -0,0 +1,220 @@ +package mcp + +import ( + "context" + "fmt" + "os" + "sort" + "strings" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/mark3labs/mcp-go/server" +) + +// #597: request decoding reads the keys it knows and unknown keys simply +// vanished — read_file(line_range: …) returned the full file, at maximum +// token cost, for a call that asked for a 40-line window, and the caller +// got no signal to self-correct. The guard makes dispatch surface what the +// published schema says: on a closed schema an unknown key appends an +// _ignored_options rider to the result by default, and under the reject +// opt-in it is an immediate tool error naming the key and the valid +// options, BEFORE the handler runs. + +// toolArgGuardEnv dials enforcement for one run: unset (or anything +// unrecognised) warns — the handler runs and the result carries an +// _ignored_options rider; "reject" refuses the call before the handler, +// naming the unknown keys and the valid options; "0"/"false"/"off"/"no" +// restores the pre-guard behavior. Warn is the default because first-party +// surfaces inject undeclared keys into arbitrary tools (see +// toolArgShapingKeys) and third-party clients grew up against open +// schemas — reject-by-default would break calls that work today. +const toolArgGuardEnv = "GORTEX_TOOL_ARG_GUARD" + +// toolArgShapingKeys are response-shaping options first-party surfaces +// inject into ANY tool call and generic layers honor without a per-tool +// declaration: the CLI pins format into every legacy-surface frame +// (buildToolCallFrameWithDefault), gortex call sets it for every non-facade +// tool, the HTTP bridge merges ?format= into any tool's args, and +// effectiveBudget / applyFieldsFilter read max_bytes / max_tokens / fields +// on every list-shaped response. The guard treats them as declared +// everywhere — warning on a key dispatch demonstrably honors would be +// noise, and rejecting it broke the CLI outright. +var toolArgShapingKeys = map[string]struct{}{ + "format": {}, + "fields": {}, + "max_bytes": {}, + "max_tokens": {}, + "cursor": {}, +} + +// The echoed unknown-key list is caller-controlled text: bound it in count +// and per-key length so the rider stays a nudge and the reject error stays +// an error, whatever the caller sent. +const ( + toolArgGuardMaxEchoedKeys = 5 + toolArgGuardMaxKeyRunes = 80 +) + +// toolArgGuardEcho renders the capped unknown-key list shared by the rider +// and the reject error: at most toolArgGuardMaxEchoedKeys keys, each cut at +// toolArgGuardMaxKeyRunes runes, with an overflow tail naming the rest. +func toolArgGuardEcho(unknown []string) string { + echo := make([]string, 0, len(unknown)+1) + for i, k := range unknown { + if i == toolArgGuardMaxEchoedKeys { + echo = append(echo, fmt.Sprintf("(+%d more)", len(unknown)-toolArgGuardMaxEchoedKeys)) + break + } + if r := []rune(k); len(r) > toolArgGuardMaxKeyRunes { + k = string(r[:toolArgGuardMaxKeyRunes]) + "…" + } + echo = append(echo, k) + } + return strings.Join(echo, ", ") +} + +// argGuardPendingRider carries one warn verdict from the guard (which runs +// deep inside the handler chain) out to the dispatch wrapper, which attaches +// it AFTER the warming / freshness decorators: both rebuild the text result +// from Content[0] (rebuildTextResult), so a rider block appended mid-chain +// would be silently dropped exactly when the freshness rider fires — a file +// drifted on disk mid-edit, the case the guard exists for. +type argGuardPendingRider struct { + text string // the rider content block + ignored string // capped key list, mirrored into StructuredContent +} + +type argGuardRiderSlotKey struct{} + +// withArgGuardRiderSlot arms the deferred rider attach for one dispatch. +func withArgGuardRiderSlot(ctx context.Context) context.Context { + return context.WithValue(ctx, argGuardRiderSlotKey{}, &argGuardPendingRider{}) +} + +func pendingArgGuardRider(ctx context.Context) *argGuardPendingRider { + p, _ := ctx.Value(argGuardRiderSlotKey{}).(*argGuardPendingRider) + return p +} + +// attachRiderToResult appends the rider content block and mirrors the capped +// key list into a structured payload that carries one. Error results never +// gain the rider: their error text stays clean. +func attachRiderToResult(res *mcp.CallToolResult, text, ignored string) { + if res == nil || res.IsError || text == "" { + return + } + res.Content = append(res.Content, mcp.NewTextContent(text)) + if sc, ok := res.StructuredContent.(map[string]any); ok { + if _, taken := sc["_ignored_options"]; !taken { + sc["_ignored_options"] = ignored + } + } +} + +// attachPendingArgGuardRider lands the deferred rider at the end of the +// dispatch chain. It runs after the injection screen (sanitize wraps the +// handler, not the decorators), so the rider — whose key names are caller +// text — is screened here with the same detector; an existing security +// notice is never overwritten. +func (s *Server) attachPendingArgGuardRider(ctx context.Context, res *mcp.CallToolResult) *mcp.CallToolResult { + pending := pendingArgGuardRider(ctx) + if pending == nil || pending.text == "" { + return res + } + attachRiderToResult(res, pending.text, pending.ignored) + if s.sanitizeInjection && res != nil && !res.IsError { + if hits := detectInjection(pending.text); len(hits) > 0 { + if res.Meta == nil || res.Meta.AdditionalFields["gortex_security"] == nil { + annotateSecurityMeta(res, nil, hits) + } + } + } + return res +} + +// toolArgGuardKeys extracts a tool's declared top-level option names and +// whether its schema closes itself. Only an explicit +// additionalProperties:false on a structured schema closes it — one left +// open, explicitly or by JSON-Schema's permissive default, is honored in +// that direction too: no enforcement. Raw schemas are out of scope: no +// shipped tool uses one (the #597 stamp in prepareTool closes only +// structured schemas), so parsing raw JSON per registration would guard a +// population of zero. +func toolArgGuardKeys(tool mcp.Tool) (map[string]struct{}, bool) { + if tool.RawInputSchema != nil { + return nil, false + } + allowExtra, ok := tool.InputSchema.AdditionalProperties.(bool) + if !ok || allowExtra { + return nil, false + } + keys := make(map[string]struct{}, len(tool.InputSchema.Properties)) + for k := range tool.InputSchema.Properties { + keys[k] = struct{}{} + } + return keys, true +} + +// wrapToolArgGuard surfaces a closed schema's key set at dispatch. Open +// schemas pass through untouched. +func wrapToolArgGuard(tool mcp.Tool, handler server.ToolHandlerFunc) server.ToolHandlerFunc { + allowed, closed := toolArgGuardKeys(tool) + if !closed { + return handler + } + valid := make([]string, 0, len(allowed)) + for k := range allowed { + valid = append(valid, k) + } + sort.Strings(valid) + validGloss := "valid options: " + strings.Join(valid, ", ") + if len(valid) == 0 { + validGloss = "this tool takes no options" + } + name := tool.Name + return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + mode := strings.ToLower(strings.TrimSpace(os.Getenv(toolArgGuardEnv))) + switch mode { + case "0", "false", "off", "no": + return handler(ctx, req) + } + var unknown []string + for k := range req.GetArguments() { + if _, shaping := toolArgShapingKeys[k]; shaping { + continue + } + if _, ok := allowed[k]; !ok { + unknown = append(unknown, k) + } + } + if len(unknown) == 0 { + return handler(ctx, req) + } + sort.Strings(unknown) + ignored := toolArgGuardEcho(unknown) + if mode == "reject" { + return mcp.NewToolResultError(fmt.Sprintf( + "%s does not accept option(s): %s; %s. The call was not executed — resend it with declared options only.", + name, ignored, validGloss)), nil + } + res, err := handler(ctx, req) + // The rider is a nudge on a successful result only: an error result + // keeps its error text clean, and structured readers get the same + // nudge mirrored into the structured payload — Content alone is + // invisible to them. When the dispatch wrapper armed the deferred + // slot, the rider is recorded there and attached after the + // warming / freshness decorators — appending it here would hand it + // to rebuildTextResult to drop. A slot-less call (direct handler + // invocation) attaches inline, where no decorator runs. + if err == nil && res != nil && !res.IsError { + text := fmt.Sprintf("_ignored_options: %s — not options of %s; %s", + ignored, name, validGloss) + if slot := pendingArgGuardRider(ctx); slot != nil { + slot.text, slot.ignored = text, ignored + } else { + attachRiderToResult(res, text, ignored) + } + } + return res, err + } +} diff --git a/internal/mcp/arg_schema_guard_test.go b/internal/mcp/arg_schema_guard_test.go new file mode 100644 index 00000000..822a2aa6 --- /dev/null +++ b/internal/mcp/arg_schema_guard_test.go @@ -0,0 +1,435 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +// #597: tool schemas promise additionalProperties:false but dispatch read +// only the keys it knew — an unknown option (line_range on read_file) +// vanished silently and the caller paid the full un-windowed response with +// no signal to self-correct. The guard warns by default: the handler still +// runs and an _ignored_options rider names the unknown keys and the valid +// ones. GORTEX_TOOL_ARG_GUARD=reject upgrades that to a refusal BEFORE the +// handler runs. + +func guardFixture(t *testing.T) (mcp.Tool, *int) { + t.Helper() + tool := mcp.NewTool("probe_tool", + mcp.WithString("offset", mcp.Description("window start")), + mcp.WithNumber("limit", mcp.Description("window size")), + ) + // NewTool leaves additionalProperties unset; prepareTool closes it. + // The guard-unit fixture models the post-registration state. + tool.InputSchema.AdditionalProperties = false + calls := 0 + return tool, &calls +} + +func guardHandler(calls *int) func(context.Context, mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + *calls++ + return mcp.NewToolResultText("ok"), nil + } +} + +func callGuarded(t *testing.T, tool mcp.Tool, calls *int, args map[string]any) *mcp.CallToolResult { + t.Helper() + wrapped := wrapToolArgGuard(tool, guardHandler(calls)) + req := mcp.CallToolRequest{} + req.Params.Name = tool.Name + req.Params.Arguments = args + res, err := wrapped(context.Background(), req) + require.NoError(t, err) + require.NotNil(t, res) + return res +} + +func guardResultText(res *mcp.CallToolResult) string { + var sb strings.Builder + for _, c := range res.Content { + if tc, ok := c.(mcp.TextContent); ok { + sb.WriteString(tc.Text) + } + } + return sb.String() +} + +// The default is warn: first-party surfaces inject undeclared keys into +// arbitrary tools (the CLI pins format into every legacy frame, the HTTP +// bridge merges ?format=), so rejecting by default breaks callers that work +// today. The handler still runs; the rider gives the agent its signal to +// self-correct. +func TestToolArgGuard_DefaultWarnsAndExecutes(t *testing.T) { + tool, calls := guardFixture(t) + res := callGuarded(t, tool, calls, map[string]any{ + "offset": "10", + "line_range": []any{120, 160}, + }) + assert.False(t, res.IsError, "the default must not refuse the call") + assert.Equal(t, 1, *calls, "the handler runs under the warn default") + text := guardResultText(res) + assert.Contains(t, text, "line_range", "the rider names the unknown key") + assert.Contains(t, text, "offset", "the rider lists the valid keys") + assert.Contains(t, text, "limit", "the rider lists the valid keys") +} + +func TestToolArgGuard_RejectOptInRefusesBeforeHandler(t *testing.T) { + t.Setenv(toolArgGuardEnv, "reject") + tool, calls := guardFixture(t) + res := callGuarded(t, tool, calls, map[string]any{ + "offset": "10", + "line_range": []any{120, 160}, + }) + assert.True(t, res.IsError, "reject mode refuses the call") + assert.Zero(t, *calls, "the handler must never run — the expensive wrong answer is never produced") + text := guardResultText(res) + assert.Contains(t, text, "line_range", "the error names the unknown key") + assert.Contains(t, text, "offset", "the error lists the valid keys") + assert.Contains(t, text, "limit", "the error lists the valid keys") +} + +// Response-shaping keys are injected by first-party surfaces into ANY tool +// call and honored by generic layers dispatch shares (the CLI's format pin, +// the HTTP bridge's ?format= merge, effectiveBudget's max_bytes / max_tokens, +// applyFieldsFilter's fields). They are declared-everywhere by decree: no +// warn rider, and even reject mode passes them through. +func TestToolArgGuard_ResponseShapingKeysExempt(t *testing.T) { + t.Setenv(toolArgGuardEnv, "reject") + tool, calls := guardFixture(t) + res := callGuarded(t, tool, calls, map[string]any{ + "offset": "10", + "format": "json", + "fields": "a,b", + "max_bytes": 1024, + "max_tokens": 200, + "cursor": "abc", + }) + assert.False(t, res.IsError, "shaping keys must survive even reject mode: %s", guardResultText(res)) + assert.Equal(t, 1, *calls) + assert.NotContains(t, guardResultText(res), "_ignored_options", "no rider for keys dispatch honors") +} + +// The rider is a nudge on a SUCCESSFUL result. A result that is already an +// error keeps its error text clean — appending "you also passed an unknown +// option" to a failure helps nobody and pollutes error matching. +func TestToolArgGuard_WarnRiderSkipsErrorResults(t *testing.T) { + t.Setenv(toolArgGuardEnv, "warn") + tool := mcp.NewTool("probe_tool", mcp.WithString("offset", mcp.Description("window start"))) + tool.InputSchema.AdditionalProperties = false + wrapped := wrapToolArgGuard(tool, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return mcp.NewToolResultError("boom"), nil + }) + req := mcp.CallToolRequest{} + req.Params.Name = tool.Name + req.Params.Arguments = map[string]any{"line_range": []any{1, 2}} + res, err := wrapped(context.Background(), req) + require.NoError(t, err) + require.True(t, res.IsError) + assert.NotContains(t, guardResultText(res), "_ignored_options", + "an error result must not gain the rider") +} + +// Structured-content readers never see Content, so a rider that lives only +// there is invisible to them. When the result carries a structured map the +// rider is mirrored in under _ignored_options. +func TestToolArgGuard_WarnRiderMirrorsIntoStructuredContent(t *testing.T) { + tool := mcp.NewTool("probe_tool", mcp.WithString("offset", mcp.Description("window start"))) + tool.InputSchema.AdditionalProperties = false + wrapped := wrapToolArgGuard(tool, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + res := mcp.NewToolResultText("ok") + res.StructuredContent = map[string]any{"answer": 42} + return res, nil + }) + req := mcp.CallToolRequest{} + req.Params.Name = tool.Name + req.Params.Arguments = map[string]any{"line_range": []any{1, 2}} + res, err := wrapped(context.Background(), req) + require.NoError(t, err) + require.False(t, res.IsError) + sc, ok := res.StructuredContent.(map[string]any) + require.True(t, ok) + assert.Equal(t, 42, sc["answer"], "existing structured payload is untouched") + assert.Contains(t, sc, "_ignored_options", "the rider must be visible to structured readers") + assert.Contains(t, fmt.Sprint(sc["_ignored_options"]), "line_range") +} + +func TestToolArgGuard_ValidKeysPassThrough(t *testing.T) { + tool, calls := guardFixture(t) + res := callGuarded(t, tool, calls, map[string]any{"offset": "10", "limit": 40}) + assert.False(t, res.IsError) + assert.Equal(t, 1, *calls) +} + +func TestToolArgGuard_NilArgumentsPassThrough(t *testing.T) { + tool, calls := guardFixture(t) + res := callGuarded(t, tool, calls, nil) + assert.False(t, res.IsError) + assert.Equal(t, 1, *calls) +} + +// A tool that explicitly opens its top-level schema opts out of the guard — +// the contract is whatever the schema says, in both directions. +func TestToolArgGuard_ExplicitOpenSchemaUnenforced(t *testing.T) { + tool, calls := guardFixture(t) + tool.InputSchema.AdditionalProperties = true + res := callGuarded(t, tool, calls, map[string]any{"anything": 1}) + assert.False(t, res.IsError) + assert.Equal(t, 1, *calls) +} + +// Raw-schema tools are outside the guard: no shipped tool uses one (the +// #597 stamp closes only structured schemas), so parsing raw JSON at +// registration would be dead surface. Authored closed or open, the call +// passes through untouched. +func TestToolArgGuard_RawSchemasNotGuarded(t *testing.T) { + t.Setenv(toolArgGuardEnv, "reject") + closed := mcp.NewToolWithRawSchema("closed_tool", "d", json.RawMessage( + `{"type":"object","properties":{"operation":{"type":"string"}},"additionalProperties":false}`)) + + calls := 0 + res := callGuarded(t, closed, &calls, map[string]any{"operaton": "file"}) + assert.False(t, res.IsError, "raw schemas are not enforced by this guard") + assert.Equal(t, 1, calls) + assert.NotContains(t, guardResultText(res), "_ignored_options") +} + +func TestToolArgGuard_WarnModeCallsHandlerWithRider(t *testing.T) { + t.Setenv(toolArgGuardEnv, "warn") + tool, calls := guardFixture(t) + res := callGuarded(t, tool, calls, map[string]any{"line_range": []any{1, 2}}) + assert.False(t, res.IsError) + assert.Equal(t, 1, *calls, "warn mode still runs the handler") + assert.Contains(t, guardResultText(res), "line_range", "the rider names what was ignored") +} + +// The off vocabulary matches the repo's boolean-env idiom (parse_gate.go): +// 0 / false / off / no. +func TestToolArgGuard_OffModeDisables(t *testing.T) { + for _, mode := range []string{"off", "0", "false", "no"} { + t.Run(mode, func(t *testing.T) { + t.Setenv(toolArgGuardEnv, mode) + tool, calls := guardFixture(t) + res := callGuarded(t, tool, calls, map[string]any{"line_range": []any{1, 2}}) + assert.False(t, res.IsError) + assert.Equal(t, 1, *calls) + assert.NotContains(t, guardResultText(res), "line_range") + }) + } +} + +// prepareTool closes any structured schema that never took a position, so +// the published contract says what dispatch now enforces. Raw schemas are +// left exactly as authored. +func TestPrepareToolStampsClosedSchema(t *testing.T) { + srv, _ := setupTestServer(t) + tool := mcp.NewTool("stamp_probe_tool", mcp.WithString("offset", mcp.Description("window start"))) + require.Nil(t, tool.InputSchema.AdditionalProperties) + srv.prepareTool(&tool, guardHandler(new(int))) + + out, err := json.Marshal(tool) + require.NoError(t, err) + var m struct { + InputSchema struct { + AdditionalProperties any `json:"additionalProperties"` + } `json:"inputSchema"` + } + require.NoError(t, json.Unmarshal(out, &m)) + assert.Equal(t, false, m.InputSchema.AdditionalProperties, + "an unset structured schema is published closed, matching enforcement") +} + +// guardE2ECall drives one tools/call frame through the real MCP dispatch +// path (initialize + HandleMessage), returning the decoded result. +func guardE2ECall(t *testing.T, srv *Server, ctx context.Context, id int, name string, arguments map[string]any) *mcp.CallToolResult { + t.Helper() + frame, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", + "id": id, + "method": "tools/call", + "params": map[string]any{"name": name, "arguments": arguments}, + }) + require.NoError(t, err) + reply := srv.MCPServer().HandleMessage(ctx, frame) + require.NotNil(t, reply) + raw, err := json.Marshal(reply) + require.NoError(t, err) + var envelope struct { + Error any `json:"error"` + Result *mcp.CallToolResult `json:"result"` + } + require.NoError(t, json.Unmarshal(raw, &envelope)) + require.Nil(t, envelope.Error, "protocol error: %v", envelope.Error) + require.NotNil(t, envelope.Result) + return envelope.Result +} + +func guardE2ESession(t *testing.T, srv *Server, session string) context.Context { + t.Helper() + ctx := WithSessionID(context.Background(), session) + initFrame := []byte(`{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18","capabilities":{},"clientInfo":{"name":"integration-harness","version":"1.0"}}}`) + require.NotNil(t, srv.MCPServer().HandleMessage(ctx, initFrame)) + return ctx +} + +// The issue's measured shape, end to end through real MCP frames on the +// full tool surface: under the reject opt-in the exact +// read_file(line_range: ...) call that used to return the whole file +// silently refuses before the read; the corrected call still works. Under +// the warn default the same call executes and carries the rider instead. +func TestReadFileUnknownWindowOptionE2E(t *testing.T) { + t.Setenv("GORTEX_TOOLS", "full") // legacy names are session-gated off the default surface + srv, _ := setupTestServer(t) + ctx := guardE2ESession(t, srv, "arg_guard_e2e") + + t.Setenv(toolArgGuardEnv, "reject") + res := guardE2ECall(t, srv, ctx, 2, "read_file", map[string]any{"path": "main.go", "line_range": []any{120, 160}}) + require.True(t, res.IsError, "line_range must refuse under reject, not return the full file: %s", guardResultText(res)) + require.Contains(t, guardResultText(res), "line_range") + require.NotContains(t, guardResultText(res), "func helper", "no file content may ride an arg-guard refusal") + + res = guardE2ECall(t, srv, ctx, 3, "read_file", map[string]any{"path": "main.go"}) + require.False(t, res.IsError, guardResultText(res)) + + t.Setenv(toolArgGuardEnv, "") + res = guardE2ECall(t, srv, ctx, 4, "read_file", map[string]any{"path": "main.go", "line_range": []any{120, 160}}) + require.False(t, res.IsError, "the warn default executes the call: %s", guardResultText(res)) + require.Contains(t, guardResultText(res), "_ignored_options", "the rider gives the self-correct signal") + require.Contains(t, guardResultText(res), "line_range") +} + +// The CLI's legacy-surface frames pin format:"json" into every tool call +// (buildToolCallFrameWithDefault), and the HTTP bridge merges ?format= the +// same way. Those frames must keep working against the guarded dispatch +// path: no refusal, no rider — format is a response-shaping key generic +// layers honor. +func TestCLIShapedFramesSurviveGuardE2E(t *testing.T) { + t.Setenv("GORTEX_TOOLS", "full") + srv, _ := setupTestServer(t) + ctx := guardE2ESession(t, srv, "arg_guard_cli_e2e") + + calls := []struct { + name string + args map[string]any + }{ + {"audit_health", map[string]any{"format": "json"}}, + {"verify_change", map[string]any{"format": "json", "changes": `[{"symbol_id":"main.go::helper","new_signature":"func helper(n int)"}]`}}, + {"get_test_targets", map[string]any{"format": "json", "ids": "main.go::helper"}}, + } + for i, c := range calls { + res := guardE2ECall(t, srv, ctx, 10+i, c.name, c.args) + require.False(t, res.IsError, "%s with the CLI's format pin must not error: %s", c.name, guardResultText(res)) + require.NotContains(t, guardResultText(res), "does not accept option", c.name) + require.NotContains(t, guardResultText(res), "_ignored_options", "%s must not warn on the format pin", c.name) + } +} + +// The warn rider must survive the response decorators. +// decorateResultWithWarming and decorateResultWithFreshness both end in +// rebuildTextResult, which rebuilds the result from Content[0] and drops +// every other block — so a rider appended mid-chain vanishes exactly when +// the freshness rider fires: a file drifted on disk mid-edit, the #597 +// poster child. This drives the real dispatch path with a drifted file and +// pins that BOTH signals arrive. +func TestWarnRiderSurvivesFreshnessRebuildE2E(t *testing.T) { + t.Setenv("GORTEX_TOOLS", "full") + srv, dir := setupTestServer(t) + ctx := guardE2ESession(t, srv, "arg_guard_drift_e2e") + t.Setenv(toolArgGuardEnv, "") + + // Drift the file on disk after indexing — the normal mid-edit state. + require.NoError(t, os.WriteFile(filepath.Join(dir, "main.go"), []byte(`package main + +type Config struct { + Port int + Host string +} + +func main() { + helper() +} + +func helper() {} +`), 0o644)) + + res := guardE2ECall(t, srv, ctx, 2, "read_file", map[string]any{"path": "main.go", "line_range": []any{1, 2}}) + require.False(t, res.IsError, guardResultText(res)) + text := guardResultText(res) + require.Contains(t, text, "freshness", + "fixture sanity: the drifted file must fire the freshness rider") + assert.Contains(t, text, "_ignored_options", + "the warn rider must survive the freshness rebuild") + assert.Contains(t, text, "line_range") +} + +// The warming decorator is the second rebuild seam: a graph tool called +// mid-warmup gets its result rebuilt around the `warming` envelope, and the +// rider must survive that rebuild exactly as it survives freshness. +func TestWarnRiderSurvivesWarmingRebuildE2E(t *testing.T) { + t.Setenv("GORTEX_TOOLS", "full") + srv, _ := setupTestServer(t) + // Put the server mid-warmup the way the daemon does: a published + // readiness phase that is not yet ready. + srv.readinessBroadcaster = newReadinessBroadcaster(&fakeSpecificSender{}, zap.NewNop()) + srv.readinessBroadcaster.publish(map[string]any{"phase": "parallel_parse", "ready": false}) + ctx := guardE2ESession(t, srv, "arg_guard_warming_e2e") + t.Setenv(toolArgGuardEnv, "") + + res := guardE2ECall(t, srv, ctx, 2, "read_file", map[string]any{"path": "main.go", "line_range": []any{1, 2}}) + require.False(t, res.IsError, guardResultText(res)) + text := guardResultText(res) + require.Contains(t, text, "warming", + "fixture sanity: a mid-warmup read must carry the warming envelope") + assert.Contains(t, text, "_ignored_options", + "the warn rider must survive the warming rebuild") + assert.Contains(t, text, "line_range") +} + +// The echoed unknown-key list is caller-controlled, so the rider and the +// reject error bound it in both count and per-key length. +func TestGuardEchoedKeyListIsCapped(t *testing.T) { + tool, calls := guardFixture(t) + long := strings.Repeat("k", 300) + args := map[string]any{ + "aaa": 1, "bbb": 1, "ccc": 1, "ddd": 1, "eee": 1, "fff": 1, "ggg": 1, + long: 1, + } + + t.Setenv(toolArgGuardEnv, "") + res := callGuarded(t, tool, calls, args) + text := guardResultText(res) + assert.Contains(t, text, "_ignored_options") + assert.Contains(t, text, "more)", "overflow keys collapse to a (+N more) tail") + assert.NotContains(t, text, long, "an oversized key is truncated, never echoed whole") + + t.Setenv(toolArgGuardEnv, "reject") + res = callGuarded(t, tool, calls, args) + require.True(t, res.IsError) + text = guardResultText(res) + assert.Contains(t, text, "more)") + assert.NotContains(t, text, long) +} + +// Handler-honored keys must be declared: read_file reads max_chars +// (capReadFileContent) — an undeclared honored option is exactly the +// #597 shape in reverse, a working key the schema calls unknown. +func TestReadFileDeclaresMaxChars(t *testing.T) { + t.Setenv("GORTEX_TOOLS", "full") + srv, _ := setupTestServer(t) + ctx := guardE2ESession(t, srv, "arg_guard_max_chars") + + t.Setenv(toolArgGuardEnv, "reject") + res := guardE2ECall(t, srv, ctx, 2, "read_file", map[string]any{"path": "main.go", "max_chars": 32}) + require.False(t, res.IsError, "max_chars is handler-honored and must be declared: %s", guardResultText(res)) + require.NotContains(t, guardResultText(res), "_ignored_options") +} diff --git a/internal/mcp/overlay.go b/internal/mcp/overlay.go index c01fb401..25673b2e 100644 --- a/internal/mcp/overlay.go +++ b/internal/mcp/overlay.go @@ -136,6 +136,10 @@ func (s *Server) wrapToolHandlerMode(h mcpserver.ToolHandlerFunc, injectOverlay // forwarding derive child contexts, so the pointer survives both paths; // idempotence prevents nested preparation from resetting the budget. ctx = withLocalizationFileRequestBudget(ctx) + // Arm the arg guard's deferred-rider slot: the guard runs inside the + // handler chain, but its warn rider must attach AFTER the decorators + // below (see attachPendingArgGuardRider). + ctx = withArgGuardRiderSlot(ctx) if injectOverlay { var err error ctx, _, err = s.prepareOverlayRequest(ctx) @@ -191,6 +195,14 @@ func (s *Server) wrapToolHandlerMode(h mcpserver.ToolHandlerFunc, injectOverlay res = s.decorateListResultWithFreshness(res) } } + // The arg guard's warn rider lands here — after the warming and + // freshness decorators, both of which rebuild the text result from + // Content[0] and would drop a rider block attached any earlier. This + // is what keeps the unknown-option signal alive on the case it + // exists for: a drifted file mid-edit carrying both riders. + if hErr == nil { + res = s.attachPendingArgGuardRider(ctx, res) + } // Capture large successful responses into the session ring so // the post-filter tools can re-cut them without re-querying. if hErr == nil { diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 867cd621..36c5ed81 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -3126,6 +3126,17 @@ func (s *Server) prepareTool(tool *mcp.Tool, handler server.ToolHandlerFunc) ser // the full semantics live once in the server instructions legend. Runs // before the split so deferred tools carry the compact schema too. compactSharedToolParams(tool) + // #597: dispatch reads only the keys it knows, so an unknown option + // used to vanish silently — the caller paid the full un-windowed + // response with no signal to self-correct. Close any structured schema + // that never took a position so the published contract states what the + // guard below enforces. Facade names are exempt: their compatibility + // wrapper deliberately accepts legacy call shapes the facade schema + // does not declare, and their options envelopes are open by design. + guarded := !isFacadeToolName(tool.Name) + if guarded && tool.RawInputSchema == nil && tool.InputSchema.AdditionalProperties == nil { + tool.InputSchema.AdditionalProperties = false + } // Capture the finished schema plus the unwrapped legacy implementation // before lazy routing. Reused facade names receive a compatibility wrapper // that keeps their old call shape outside facade-v1 sessions. @@ -3133,6 +3144,9 @@ func (s *Server) prepareTool(tool *mcp.Tool, handler server.ToolHandlerFunc) ser s.facades.capture(*tool, handler) handler = s.wrapLegacyFacade(tool.Name, handler) } + if guarded { + handler = wrapToolArgGuard(*tool, handler) + } return handler } diff --git a/internal/mcp/tools_coding.go b/internal/mcp/tools_coding.go index 7aa46468..19694eb2 100644 --- a/internal/mcp/tools_coding.go +++ b/internal/mcp/tools_coding.go @@ -146,6 +146,7 @@ func (s *Server) registerCodingTools() { mcp.WithString("fidelity_globs", mcp.Description(fidelityGlobsParamDescription)), mcp.WithNumber("max_lines", mcp.Description("When the file exceeds this many lines, collapse runs of leaf statements inside function bodies into `… N lines elided …` markers while keeping declarations and the control-flow skeleton. Falls back to a plain head cut for non-code files. Omit or 0 to disable.")), mcp.WithNumber("max_bytes", mcp.Description("Cap the marshaled response at this many bytes; truncation flag rides on the response. Omit for no cap.")), + mcp.WithNumber("max_chars", mcp.Description("Cap the returned content at this many encoded bytes, cut at a valid UTF-8 boundary; a truncation note rides on the response. Omit or 0 to disable.")), mcp.WithString("if_none_match", mcp.Description("ETag from a previous response — returns not_modified if content unchanged")), ), s.handleReadFile, diff --git a/internal/mcp/tools_list_budget_test.go b/internal/mcp/tools_list_budget_test.go index 659c595e..7c675398 100644 --- a/internal/mcp/tools_list_budget_test.go +++ b/internal/mcp/tools_list_budget_test.go @@ -39,8 +39,15 @@ func serializeToolsList(t *testing.T, preset, mode string) (int, []string) { // Pre-diet baselines measured on this test harness (the same NewServer path // the gate uses), so "strictly smaller than today" is a real regression // assertion rather than a moving target. +// +// core re-based 95060 → 96500: the #597 `additionalProperties:false` +// stamp (~27 bytes across every closed core tool) plus read_file's +// max_chars declaration landed on top of main's receipt/idempotency +// growth, which had already eaten most of the diet slack. Measured after +// the stamp: 96168 bytes. The assertion still bites on description +// creep; the stamp is contract, not creep. const ( - corePresetBaselineBytes = 95060 + corePresetBaselineBytes = 96500 fullPresetBaselineBytes = 289808 ) @@ -66,7 +73,13 @@ const ( // The `mutation_id` idempotency key on the same two floor tools then took // another 148 bytes (28527 → 28675), sharing one blurb constant for the same // reason. The ceiling still holds; the remaining slack is ~175 bytes. -const agentPresetByteCeiling = 28850 +// +// Re-based 28850 → 29700 when every structured schema began publishing +// `additionalProperties:false` (#597) — ~27 bytes per tool so the +// contract states what dispatch now surfaces — and read_file declared its +// handler-honored max_chars option. Measured after both, on top of the +// receipt/idempotency growth: 29313 bytes; ~390 bytes of slack. +const agentPresetByteCeiling = 29700 // localizationPresetByteCeiling is the hard budget for the diet // localization preset (the `localization` instruction profile's tool @@ -79,7 +92,12 @@ const agentPresetByteCeiling = 28850 // session without a multi-symbol read). Measured after the growth: // 20432 bytes — still ~27% under the agent floor, with slack so any // further description creep fails loudly. -const localizationPresetByteCeiling = 21000 +// +// Re-based 21000 → 21350 alongside the #597 `additionalProperties:false` +// stamp (~27 bytes per tool) and read_file's max_chars declaration: +// measured 21020 bytes, restoring ~300 bytes of slack the stamp had +// eaten. +const localizationPresetByteCeiling = 21350 // TestToolsListByteCeilings is the permanent measurement gate: it prints the // cold tools/list byte cost of every preset and asserts the agent preset