From 277152ba087b30e3608f5c20b890ea3e515cdcc3 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Wed, 19 Aug 2026 10:07:32 +0200 Subject: [PATCH 1/4] mcp: reject unknown tool options at dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dispatch read only the keys it knew, so an unknown option vanished silently — read_file(line_range: ...) returned the full file at maximum token cost with no signal to self-correct (#597). Close every structured schema that never took a position, publish that in tools/list, and enforce it before the handler runs: unknown keys error immediately, naming them and the valid options. GORTEX_TOOL_ARG_GUARD=warn appends an _ignored_options rider instead; =off restores the old behavior. Facade names stay exempt — their compat wrapper accepts legacy shapes by design. --- internal/mcp/arg_schema_guard.go | 109 ++++++++++++++ internal/mcp/arg_schema_guard_test.go | 198 +++++++++++++++++++++++++ internal/mcp/server.go | 14 ++ internal/mcp/tools_list_budget_test.go | 13 +- 4 files changed, 332 insertions(+), 2 deletions(-) create mode 100644 internal/mcp/arg_schema_guard.go create mode 100644 internal/mcp/arg_schema_guard_test.go diff --git a/internal/mcp/arg_schema_guard.go b/internal/mcp/arg_schema_guard.go new file mode 100644 index 000000000..bffa989aa --- /dev/null +++ b/internal/mcp/arg_schema_guard.go @@ -0,0 +1,109 @@ +package mcp + +import ( + "context" + "encoding/json" + "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 honor what the +// published schema says: on a closed schema an unknown key is an immediate +// tool error naming the key and the valid options, BEFORE the handler runs +// — the expensive wrong answer is never produced. + +// toolArgGuardEnv dials enforcement for one run: unset (or anything +// unrecognised) rejects — the schema's own contract; "warn" runs the +// handler and appends an _ignored_options rider; "off"/"0"/"false"/"none" +// restores the pre-guard behavior. +const toolArgGuardEnv = "GORTEX_TOOL_ARG_GUARD" + +// toolArgGuardKeys extracts a tool's declared top-level option names and +// whether its schema closes itself. Only an explicit +// additionalProperties:false closes a schema — one left open, explicitly +// or by JSON-Schema's permissive default, is honored in that direction +// too: no enforcement. +func toolArgGuardKeys(tool mcp.Tool) (map[string]struct{}, bool) { + if tool.RawInputSchema != nil { + var s struct { + Properties map[string]json.RawMessage `json:"properties"` + AdditionalProperties json.RawMessage `json:"additionalProperties"` + } + if err := json.Unmarshal(tool.RawInputSchema, &s); err != nil { + return nil, false + } + if strings.TrimSpace(string(s.AdditionalProperties)) != "false" { + return nil, false + } + keys := make(map[string]struct{}, len(s.Properties)) + for k := range s.Properties { + keys[k] = struct{}{} + } + return keys, true + } + 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 enforces 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 "off", "0", "false", "none": + return handler(ctx, req) + } + var unknown []string + for k := range req.GetArguments() { + if _, ok := allowed[k]; !ok { + unknown = append(unknown, k) + } + } + if len(unknown) == 0 { + return handler(ctx, req) + } + sort.Strings(unknown) + if mode == "warn" { + res, err := handler(ctx, req) + if err == nil && res != nil { + res.Content = append(res.Content, mcp.NewTextContent(fmt.Sprintf( + "_ignored_options: %s — not options of %s; %s", + strings.Join(unknown, ", "), name, validGloss))) + } + return res, err + } + 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, strings.Join(unknown, ", "), validGloss)), nil + } +} diff --git a/internal/mcp/arg_schema_guard_test.go b/internal/mcp/arg_schema_guard_test.go new file mode 100644 index 000000000..2a82e3fdc --- /dev/null +++ b/internal/mcp/arg_schema_guard_test.go @@ -0,0 +1,198 @@ +package mcp + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// #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 rejects at dispatch, BEFORE the +// handler runs, naming the unknown keys and the valid ones. + +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() +} + +func TestToolArgGuard_UnknownKeyRejectedBeforeHandler(t *testing.T) { + tool, calls := guardFixture(t) + res := callGuarded(t, tool, calls, map[string]any{ + "offset": "10", + "line_range": []any{120, 160}, + }) + assert.True(t, res.IsError, "an unknown option must reject the call, not vanish") + assert.Zero(t, *calls, "the handler must never run — that is the whole point: 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") +} + +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 (the facade envelopes) are enforced exactly as authored: +// additionalProperties:false rejects, true stays open. +func TestToolArgGuard_RawSchemaHonorsAuthoredContract(t *testing.T) { + closed := mcp.NewToolWithRawSchema("closed_tool", "d", json.RawMessage( + `{"type":"object","properties":{"operation":{"type":"string"}},"additionalProperties":false}`)) + open := mcp.NewToolWithRawSchema("open_tool", "d", json.RawMessage( + `{"type":"object","properties":{"operation":{"type":"string"}},"additionalProperties":true}`)) + + calls := 0 + res := callGuarded(t, closed, &calls, map[string]any{"operaton": "file"}) + assert.True(t, res.IsError, "typo'd key on a closed raw schema must reject") + assert.Zero(t, calls) + + res = callGuarded(t, open, &calls, map[string]any{"extra": true}) + assert.False(t, res.IsError, "an open raw schema accepts extras by contract") + assert.Equal(t, 1, calls) +} + +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") +} + +func TestToolArgGuard_OffModeDisables(t *testing.T) { + t.Setenv(toolArgGuardEnv, "off") + 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") +} + +// The issue's measured shape, end to end through real MCP frames on the +// full tool surface: the exact read_file(line_range: ...) call that used +// to return the whole file silently now refuses before the read, and the +// corrected call still works against the real handler. +func TestReadFileRejectsUnknownWindowOptionE2E(t *testing.T) { + t.Setenv("GORTEX_TOOLS", "full") // legacy names are session-gated off the default surface + srv, _ := setupTestServer(t) + ctx := WithSessionID(context.Background(), "arg_guard_e2e") + 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)) + + call := func(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 + } + + res := call(2, "read_file", map[string]any{"path": "main.go", "line_range": []any{120, 160}}) + require.True(t, res.IsError, "line_range must refuse, 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 = call(3, "read_file", map[string]any{"path": "main.go"}) + require.False(t, res.IsError, guardResultText(res)) +} diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 590282827..420202587 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -3110,6 +3110,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. @@ -3117,6 +3128,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_list_budget_test.go b/internal/mcp/tools_list_budget_test.go index 3c529397a..4fd6a54db 100644 --- a/internal/mcp/tools_list_budget_test.go +++ b/internal/mcp/tools_list_budget_test.go @@ -53,7 +53,12 @@ const ( // follow-up reader). Measured cost after the addition: 27883 bytes — // the ceiling keeps ~300 bytes of slack, so any further description // creep still fails loudly. -const agentPresetByteCeiling = 28200 +// +// Re-based 28200 → 29050 when every structured schema began publishing +// `additionalProperties:false` (#597) — ~27 bytes per tool so the +// contract states what dispatch now enforces. Measured after the stamp: +// 28743 bytes; ~300 bytes of slack again. +const agentPresetByteCeiling = 29050 // localizationPresetByteCeiling is the hard budget for the diet // localization preset (the `localization` instruction profile's tool @@ -66,7 +71,11 @@ const agentPresetByteCeiling = 28200 // 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 → 21250 alongside the #597 `additionalProperties:false` +// stamp (~27 bytes per tool): measured 20922 bytes, restoring ~300 +// bytes of slack the stamp had eaten. +const localizationPresetByteCeiling = 21250 // TestToolsListByteCeilings is the permanent measurement gate: it prints the // cold tools/list byte cost of every preset and asserts the agent preset From 26e9b620edc598248891c4a8f134b864bf39e571 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Thu, 20 Aug 2026 13:17:08 +0200 Subject: [PATCH 2/4] mcp: warn on unknown tool options by default, reject opt-in Review follow-up on the dispatch arg guard. Rejecting by default broke callers that work today: first-party surfaces inject undeclared keys into arbitrary tools (the CLI pins format into every legacy-surface frame, gortex call sets it for every non-facade tool, the HTTP bridge merges ?format=), and generic layers honor max_bytes / max_tokens / fields on every list-shaped response without any schema declaring them. - Default is now warn: the handler runs and the result carries an _ignored_options rider naming the unknown keys and the valid options. GORTEX_TOOL_ARG_GUARD=reject restores the hard refusal; the off vocabulary aligns with the repo's boolean-env idiom (0/false/off/no). - Response-shaping keys (format, fields, max_bytes, max_tokens, cursor) are exempt in both modes - dispatch demonstrably honors them, so flagging them is noise at best and a broken CLI at worst. - The rider skips error results and is mirrored into structuredContent when the result carries a structured map - Content alone is invisible to structured readers. - read_file declares its handler-honored max_chars; the subagent hook stops passing compact to smart_context (never a declared or honored option - the call worked only because unknown keys used to vanish). - The RawInputSchema branch is gone: 0 shipped tools use raw schemas and the #597 stamp closes only structured ones, so it guarded a population of zero. - End-to-end pins: the CLI's format-pinned frames for audit_health / verify_change / get_test_targets survive guarded dispatch untouched, and the #597 read_file(line_range) shape still refuses under reject while warning under the default. --- internal/hooks/subagent.go | 8 +- internal/mcp/arg_schema_guard.go | 99 ++++++---- internal/mcp/arg_schema_guard_test.go | 256 ++++++++++++++++++++------ internal/mcp/tools_coding.go | 1 + 4 files changed, 269 insertions(+), 95 deletions(-) diff --git a/internal/hooks/subagent.go b/internal/hooks/subagent.go index 75aafeec7..184cf6c19 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 index bffa989aa..af163cd96 100644 --- a/internal/mcp/arg_schema_guard.go +++ b/internal/mcp/arg_schema_guard.go @@ -2,7 +2,6 @@ package mcp import ( "context" - "encoding/json" "fmt" "os" "sort" @@ -15,39 +14,50 @@ import ( // #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 honor what the -// published schema says: on a closed schema an unknown key is an immediate -// tool error naming the key and the valid options, BEFORE the handler runs -// — the expensive wrong answer is never produced. +// 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) rejects — the schema's own contract; "warn" runs the -// handler and appends an _ignored_options rider; "off"/"0"/"false"/"none" -// restores the pre-guard behavior. +// 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": {}, +} + // toolArgGuardKeys extracts a tool's declared top-level option names and // whether its schema closes itself. Only an explicit -// additionalProperties:false closes a schema — one left open, explicitly -// or by JSON-Schema's permissive default, is honored in that direction -// too: no enforcement. +// 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 { - var s struct { - Properties map[string]json.RawMessage `json:"properties"` - AdditionalProperties json.RawMessage `json:"additionalProperties"` - } - if err := json.Unmarshal(tool.RawInputSchema, &s); err != nil { - return nil, false - } - if strings.TrimSpace(string(s.AdditionalProperties)) != "false" { - return nil, false - } - keys := make(map[string]struct{}, len(s.Properties)) - for k := range s.Properties { - keys[k] = struct{}{} - } - return keys, true + return nil, false } allowExtra, ok := tool.InputSchema.AdditionalProperties.(bool) if !ok || allowExtra { @@ -60,7 +70,7 @@ func toolArgGuardKeys(tool mcp.Tool) (map[string]struct{}, bool) { return keys, true } -// wrapToolArgGuard enforces a closed schema's key set at dispatch. Open +// 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) @@ -80,11 +90,14 @@ func wrapToolArgGuard(tool mcp.Tool, handler server.ToolHandlerFunc) server.Tool return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { mode := strings.ToLower(strings.TrimSpace(os.Getenv(toolArgGuardEnv))) switch mode { - case "off", "0", "false", "none": + 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) } @@ -93,17 +106,27 @@ func wrapToolArgGuard(tool mcp.Tool, handler server.ToolHandlerFunc) server.Tool return handler(ctx, req) } sort.Strings(unknown) - if mode == "warn" { - res, err := handler(ctx, req) - if err == nil && res != nil { - res.Content = append(res.Content, mcp.NewTextContent(fmt.Sprintf( - "_ignored_options: %s — not options of %s; %s", - strings.Join(unknown, ", "), name, validGloss))) + 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, strings.Join(unknown, ", "), 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. + if err == nil && res != nil && !res.IsError { + ignored := strings.Join(unknown, ", ") + res.Content = append(res.Content, mcp.NewTextContent(fmt.Sprintf( + "_ignored_options: %s — not options of %s; %s", + ignored, name, validGloss))) + if sc, ok := res.StructuredContent.(map[string]any); ok { + if _, taken := sc["_ignored_options"]; !taken { + sc["_ignored_options"] = ignored + } } - return res, err } - 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, strings.Join(unknown, ", "), validGloss)), nil + return res, err } } diff --git a/internal/mcp/arg_schema_guard_test.go b/internal/mcp/arg_schema_guard_test.go index 2a82e3fdc..dcc0c7a2d 100644 --- a/internal/mcp/arg_schema_guard_test.go +++ b/internal/mcp/arg_schema_guard_test.go @@ -3,6 +3,7 @@ package mcp import ( "context" "encoding/json" + "fmt" "strings" "testing" @@ -14,8 +15,10 @@ import ( // #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 rejects at dispatch, BEFORE the -// handler runs, naming the unknown keys and the valid ones. +// 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() @@ -59,20 +62,105 @@ func guardResultText(res *mcp.CallToolResult) string { return sb.String() } -func TestToolArgGuard_UnknownKeyRejectedBeforeHandler(t *testing.T) { +// 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.True(t, res.IsError, "an unknown option must reject the call, not vanish") - assert.Zero(t, *calls, "the handler must never run — that is the whole point: the expensive wrong answer is never produced") + 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}) @@ -97,22 +185,20 @@ func TestToolArgGuard_ExplicitOpenSchemaUnenforced(t *testing.T) { assert.Equal(t, 1, *calls) } -// Raw-schema tools (the facade envelopes) are enforced exactly as authored: -// additionalProperties:false rejects, true stays open. -func TestToolArgGuard_RawSchemaHonorsAuthoredContract(t *testing.T) { +// 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}`)) - open := mcp.NewToolWithRawSchema("open_tool", "d", json.RawMessage( - `{"type":"object","properties":{"operation":{"type":"string"}},"additionalProperties":true}`)) calls := 0 res := callGuarded(t, closed, &calls, map[string]any{"operaton": "file"}) - assert.True(t, res.IsError, "typo'd key on a closed raw schema must reject") - assert.Zero(t, calls) - - res = callGuarded(t, open, &calls, map[string]any{"extra": true}) - assert.False(t, res.IsError, "an open raw schema accepts extras by contract") + 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) { @@ -124,13 +210,19 @@ func TestToolArgGuard_WarnModeCallsHandlerWithRider(t *testing.T) { 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) { - t.Setenv(toolArgGuardEnv, "off") - 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") + 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 @@ -154,45 +246,101 @@ func TestPrepareToolStampsClosedSchema(t *testing.T) { "an unset structured schema is published closed, matching enforcement") } -// The issue's measured shape, end to end through real MCP frames on the -// full tool surface: the exact read_file(line_range: ...) call that used -// to return the whole file silently now refuses before the read, and the -// corrected call still works against the real handler. -func TestReadFileRejectsUnknownWindowOptionE2E(t *testing.T) { - t.Setenv("GORTEX_TOOLS", "full") // legacy names are session-gated off the default surface - srv, _ := setupTestServer(t) - ctx := WithSessionID(context.Background(), "arg_guard_e2e") +// 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 +} - call := func(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 - } +// 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") - res := call(2, "read_file", map[string]any{"path": "main.go", "line_range": []any{120, 160}}) - require.True(t, res.IsError, "line_range must refuse, not return the full file: %s", guardResultText(res)) + 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 = call(3, "read_file", map[string]any{"path": "main.go"}) + 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) + } +} + +// 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/tools_coding.go b/internal/mcp/tools_coding.go index 7aa464684..19694eb2c 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, From daacb52e7dccd9f3c4d0ded0da2e09d44ae30f00 Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:39:58 +0200 Subject: [PATCH 3/4] mcp: attach the warn rider after the response decorators --- docs/mcp.md | 2 + internal/mcp/arg_schema_guard.go | 108 +++++++++++++++++++++++--- internal/mcp/arg_schema_guard_test.go | 65 ++++++++++++++++ internal/mcp/overlay.go | 12 +++ 4 files changed, 177 insertions(+), 10 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 7a1be69b8..8ddd052fa 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/mcp/arg_schema_guard.go b/internal/mcp/arg_schema_guard.go index af163cd96..3576b287d 100644 --- a/internal/mcp/arg_schema_guard.go +++ b/internal/mcp/arg_schema_guard.go @@ -47,6 +47,91 @@ var toolArgShapingKeys = map[string]struct{}{ "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 @@ -106,25 +191,28 @@ func wrapToolArgGuard(tool mcp.Tool, handler server.ToolHandlerFunc) server.Tool 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, strings.Join(unknown, ", "), validGloss)), nil + 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. + // 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 { - ignored := strings.Join(unknown, ", ") - res.Content = append(res.Content, mcp.NewTextContent(fmt.Sprintf( - "_ignored_options: %s — not options of %s; %s", - ignored, name, validGloss))) - if sc, ok := res.StructuredContent.(map[string]any); ok { - if _, taken := sc["_ignored_options"]; !taken { - sc["_ignored_options"] = ignored - } + 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 index dcc0c7a2d..87d5505fd 100644 --- a/internal/mcp/arg_schema_guard_test.go +++ b/internal/mcp/arg_schema_guard_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "fmt" + "os" + "path/filepath" "strings" "testing" @@ -331,6 +333,69 @@ func TestCLIShapedFramesSurviveGuardE2E(t *testing.T) { } } +// 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 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. diff --git a/internal/mcp/overlay.go b/internal/mcp/overlay.go index c01fb401f..25673b2ee 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 { From 67ad30df4f24ccb8ab48897eda21f228f3efd89d Mon Sep 17 00:00:00 2001 From: pbednarcik <29061766+pbednarcik@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:47:00 +0200 Subject: [PATCH 4/4] mcp: pin warming-path rider survival too --- internal/mcp/arg_schema_guard_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/internal/mcp/arg_schema_guard_test.go b/internal/mcp/arg_schema_guard_test.go index 87d5505fd..822a2aa68 100644 --- a/internal/mcp/arg_schema_guard_test.go +++ b/internal/mcp/arg_schema_guard_test.go @@ -12,6 +12,7 @@ import ( "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 @@ -371,6 +372,29 @@ func helper() {} 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) {