mcp: surface unknown tool options at dispatch

[pbednarcik]

Addresses #597.

Problem

Request decoding reads the keys it knows and unknown keys simply vanish. The measured shape from the issue: an agent passed line_range: [120, 160] to read_file three times — a plausible guess for an option that doesn't exist — and got the full file back each time, silently. Maximum token cost on the heaviest tool in the surface, for a call that explicitly asked for a 40-line window, with no signal to self-correct.

Why reject, not the rider I proposed first

The issue ranked a warning rider (option 2) as the natural first step. Running agents against my own index changed my mind: the rider arrives attached to the full-cost response — the tokens are already spent, and the correction only helps the next call. An immediate error costs a few dozen tokens, names the bad key, and the agent fixes the very next call; the expensive wrong answer is never produced. So this PR makes dispatch honor the schema's own contract by default, with the rider and the old behavior both one env var away:

• default — unknown key on a closed schema → tool error, before the handler runs: read_file does not accept option(s): line_range; valid options: …
• GORTEX_TOOL_ARG_GUARD=warn — run the handler, append an _ignored_options rider (option 2 as described in the issue)
• GORTEX_TOOL_ARG_GUARD=off — pre-guard behavior

Change

• prepareTool closes any structured schema that never took a position (additionalProperties unset → false), so tools/list publishes exactly what dispatch enforces. Tools that opt into open args (AdditionalProperties(true)) keep them — enforcement follows the schema in both directions, raw schemas as authored.
• wrapToolArgGuard enforces at the same seam as the existing overlay/telemetry middleware, so both the daemon-dispatched path and the in-process HTTP path get it, and lazy-promoted tools inherit it.
• Facade names are exempt: their compatibility wrapper deliberately accepts legacy call shapes the facade schema doesn't declare, and the facade options envelopes are open by design. Per-operation key enforcement inside those envelopes would be the follow-up — that one's a design conversation, not a patch.
• tools/list byte ceilings re-based (+~27 bytes per tool for the published stamp), following the constants' own re-base convention.

Tests

Nine pins: rejection happens before the handler runs (the handler-never-ran assertion is the point of the whole change), valid/nil args pass, explicit-open structured and raw schemas stay unenforced, closed raw schemas reject typo'd keys, warn mode runs the handler and names what it ignored, off mode restores old behavior, prepareTool publishes the closed schema — and the issue's exact shape end to end through real MCP frames: read_file(line_range: [120,160]) refuses before the read, the corrected call works. Full suite matches my Windows baseline.

[zzet]

Requesting changes. The blanket additionalProperties:false stamp closes 152 of 173 registered tools and hard-rejects keys gortex's own callers inject. Verified on 277152b against b55b9a0.

1. format injection → refusal on 54/152 closed tools

Three first-party injectors add format to arbitrary tools' args:

• cmd/gortex/cli_daemon.go:95 — buildToolCallFrameWithDefault pins format:"json" into every legacy-surface CLI frame (pinJSONDefault = tools != facade-v1; CLI verbs request core).
• cmd/gortex/call.go:117 — gortex call <non-facade tool> sets argObj["format"] = callFormat (default json). Only the facade branch is exempt.
• internal/server/handler.go:449 — merges ?format= / body format into any tool. docs/server.md:32 documents this for "any MCP tool".

Broken:

• gortex audit → audit_health
• gortex affected → get_test_targets (affected.go:149)
• gortex tools list|receipt → tool_profile (declares only ["tool"]). The receipt reports AdvertisedTools:0 / no_surface_mounted against a healthy daemon, no error surfaced.
• gortex edit — 9/13 subcommands via edit.go:67: verify_change, get_edit_plan, preview_edit, simulate_chain, batch_edit, edit_file, edit_symbol, rename_symbol, get_test_targets
• gortex call edit_file --arg path=... — the invocation printed in docs/cli.md:250
• POST /v1/tools/read_file?format=gcx and 53 other tools

audit_health does not accept option(s): format; valid options: project, repo, scope, workspace

2. Handler-honored keys that no schema declares

• read_file reads max_chars (tools_fileops.go:1408, honored via capReadFileContent) but doesn't declare it. TestReadFilePhysicalEvidenceMaxCharsRetainsTruncationContract asserts that contract and stays green. Its description also says Composable with format:"gcx" while declaring no format.
• budget.go:481 effectiveBudget reads max_bytes / max_tokens generically for every tool (budget.go:55: "wired onto every list-shaped tool"); applyFieldsFilter does the same for fields. 104/152 closed tools don't declare max_bytes, 148/152 don't declare fields. search_text{max_bytes} and graph_query{fields} now hard-error.

3. Gortex's own hook self-degrades silently

internal/hooks/subagent.go renderTaskContext calls smart_context{task, compact:true}. smart_context declares no compact. Both hook transports collapse isError to "", so the ### Relevant Symbols block drops out of every subagent briefing with nothing logged.

4. The suite cannot see any of this

Full suite is green on the head (internal/mcp, internal/server, internal/server/hub, cmd/gortex). Nothing crosses the guarded path with a caller-built arg map:

• internal/mcp/server_test.go:69 findAndCallHandler dispatches through a hand-maintained handler map, bypassing prepareTool.
• cmd/gortex tests stub the relay seam (edit_test.go:40, tools_cmd_test.go:38, analyze_test.go:31).
• cli_daemon_test.go:13 only asserts the pin exists, using search_symbols / graph_stats — both declare format.
• internal/server/handler_test.go:204's ?format=gcx test registers its fixture via bare mcpserver.AddTool.

5. Guard is inert where #597 was observed

docs/mcp.md:114: every connection with a non-empty clientInfo.name defaults to facade-v1 — the 21 tools exempted at server.go:3120. Second cause: s.facades.capture(*tool, handler) at :3128 stores the pre-guard closure facade dispatch invokes. Measured: read(operation:"file", target:{file:"main.go"}, line_range:"1-3") returns the whole file, isError=false, no rider. So the fix doesn't fire where the bug was reported, and does fire on the CLI, which it breaks.

Premise

Schemas do not declare additionalProperties:false on main — this PR's own TestPrepareToolStampsClosedSchema asserts require.Nil(...) before the stamp. The change mints the contract and enforces it in one step rather than honoring an existing one, so "clients that work by accident" understates the break.

Suggested path

Default to warn; make reject opt-in (GORTEX_TOOL_ARG_GUARD=reject). That clears every item above and matches this issue's own ranking of option 2 as the first step.

If reject-by-default is wanted:

1. Teach the three injectors to consult the published schema, or exempt a fixed response-shaping key set (format, fields, max_bytes, max_tokens, cursor) inside wrapToolArgGuard.
2. Declare max_chars on read_file and compact on smart_context.
3. Add an integration test piping buildToolCallFrame(...) output for audit_health / verify_change / get_test_targets through srv.MCPServer().HandleMessage, asserting isError=false. The harness at arg_schema_guard_test.go:161 already exists.
4. A lint test asserting no shipped tool reads an undeclared key prevents recurrence.

Correct as-is

The middleware seam is the right one: the guard survives lazy promotion (tool stamped before lazy.Register), covers control tools and both dispatch paths. reconcileToolParams (overlay.go:124) runs before the guard and deletes resolved alias keys, so typo recovery still works. checkToolGate runs first, so hidden tools leak no schema. The e2e test is not vacuous — the fixture's main.go really contains func helper(). Byte-ceiling re-base verified against main: agent had 8 bytes of slack (28192/28200) → 28743/29050; loc 20545 → 20922; core/full still under their pre-diet baselines.

Minor: warn appends only to Content, so it is invisible to structuredContent readers, and it also fires on error results. The off-vocabulary {off,0,false,none} diverges from the repo's {0,false,off,no} (parse_gate.go:97). The RawInputSchema branch is unreachable — 0/173 live tools use raw schemas — and its test comment about "the facade envelopes" is wrong; facades use structured schemas.

[pbednarcik]

Reworked in 26e9b62 (plus a merge of main in ff76686 that stacks the two ceiling re-bases).

Took the suggested path: warn is the default, GORTEX_TOOL_ARG_GUARD=reject opts into the refusal. On top of that:

• format, fields, max_bytes, max_tokens, cursor are exempt in both modes. Dispatch honors them through the injectors and generic layers you listed, so flagging them is noise at best and the broken CLI at worst. Your three CLI shapes are pinned end to end: format-pinned frames for audit_health, verify_change and get_test_targets go through HandleMessage and come back isError=false with no rider.
• read_file now declares max_chars. For smart_context I went the other way: the handler never reads compact, so declaring it would publish an option that does nothing. The hook stops sending it instead (internal/hooks/subagent.go) - the call only ever worked because unknown keys used to vanish, which is this PR's bug in miniature.
• The rider skips error results and is mirrored into structuredContent when the result carries a structured map, so structured readers see it too.
• Off vocabulary is now 0/false/off/no, matching parse_gate.
• The RawInputSchema branch is gone along with its wrong test comment - raw schemas are simply out of the guard's scope.

Not done: the lint test asserting no shipped tool reads an undeclared key. That needs a per-handler key inventory and felt like its own change; with the shaping set exempt and the two known gaps declared or removed, the known instances are covered. Can file it as a follow-up issue if you want it tracked.

One thing this still does not change: facade dispatch invokes the pre-guard closure, so the guard remains inert on facade-v1 sessions where #597 was originally observed. The facade envelope validates its own operation vocabulary, but its options envelope stays open by design - if you want the guard inside the facade lane too, I would rather do that as a follow-up with its own tests than widen this PR further.

Retitled the PR to match: it surfaces unknown options by default now, rejecting only on opt-in.

[zzet]

Re-reviewed at 26e9b620. The rework lands: all four blockers from the last pass are fixed, and I re-verified each rather than taking them on trust. One new defect, and it's in the mechanism this PR exists for.

Verified clean this round — don't re-litigate

• The warn default can't be bypassed behind your back. mcp-go v0.58.0 does ship server-side additionalProperties enforcement (WithInputSchemaValidation, plus WithStrictInputSchemaDefault which does your stamp for you) — gortex enables neither, zero hits repo-wide. So stamping the schema closed genuinely cannot hard-reject while the guard says warn.
• The CLI/HTTP shapes work. format exemption verified through the injectors; the CLI reads res.Content[0].Text only, so an appended rider can't corrupt JSON decoding either.
• All 13 callServerTool hook sites cross-checked against declared schemas — clean after the subagent.go fix. (precompact.go:154 get_symbol_history{compact} is fine, it's declared.)
• Ordering is right. reconcileToolParams and checkToolGate both run before the guard, so typo recovery still works and the "valid options" gloss can't leak a gated tool's schema. Sanitize wraps the guard, so echoed key names pass the injection screen.
• Byte re-base is honest. Measured main → PR: agent 28675→29313 (ceiling 29700), loc 20556→21020, core 94931→96168, full 249727→254357. That's ≈27 B/tool of stamp, not description creep.
• I ran the lint test you deferred, ad hoc: regex over every req.Get*/Require* inside each handle* body, mapped to its tool via the mcp.NewTool("x", …), s.handleX registration. 0 closed tools read a key their own schema doesn't declare. That supports your claim that the known instances are covered and reject mode is safe now. One caveat it can't see: keys read in shared helpers. resolveScopeForRequest reads ref on behalf of 8 tools that don't declare it (audit_health, find_clones, find_files, get_communities, get_processes, get_recent_changes, run_inspections, search_text) — but reconcileToolParams alias-rewrites ref→repo (levenshtein 2) before the guard sees it, so no rider fires. That rewrite is a pre-existing bug on main, not yours.

Blocker: the warn rider is silently eaten by the freshness and warmup decorators

The guard appends _ignored_options as a second text content block from inside the handler. decorateResultWithWarming (warmup_fastpath.go) and decorateResultWithFreshness / decorateListResultWithFreshness (freshness_rider.go) both end in rebuildTextResult, which builds a fresh single-block result from Content[0] and drops everything else — other blocks and StructuredContent. Those decorators run after the guard in wrapToolHandlerMode.

Reproduced directly, same call twice, only difference is whether the file drifted on disk:

NO-DRIFT  rider=true
DRIFT     freshness=true  rider=false  blocks=1

So read_file(line_range: …) against a file that changed on disk since indexing — the normal state mid-edit — returns the full un-windowed response with no signal at all. That is exactly #597, on this PR's poster-child tool. Same for get_symbol_source, get_symbol, get_file_summary, get_editing_context, and the list tools search_symbols / find_usages / smart_context / get_callers, plus every graph tool during daemon warmup.

The fix is small — the repo already has a seam that survives: maybeAttachMomentumNote (internal/mcp/overlay.go:203) appends after the rebuilds. Either move the rider there, or embed _ignored_options into the JSON body the way freshness does. Worth a test that drifts a file and asserts the rider survives the decorator chain; nothing covers that today, which is why this passed green.

Minor, non-blocking

1. GORTEX_TOOL_ARG_GUARD is documented nowhere. docs/mcp.md documents GORTEX_TOOLS, GORTEX_LAZY_TOOLS and GORTEX_OVERLAY_IDLE_TTL inline; a switch that changes dispatch semantics should join them.
2. The stamp publishes a contract slightly stricter than reality. Among the 153 closed tools, format is undeclared on 55, max_bytes on 105, max_tokens on 141, cursor on 148, fields on 149 — and all five are still accepted. Sharpest instance: read_file's own description says "Composable with format:"gcx"" while its schema now forbids format. Nothing breaks at runtime, but models and schema-aware clients are being steered away from documented, working options.
3. The unknown-key list echoed into the rider/error is uncapped in both count and length.

On scope

Agreed on the facade lane as a follow-up, and thanks for stating plainly that the guard stays inert on facade-v1 — that's the default surface for named MCP clients, so it's worth being explicit that merging this does not close #597 where it was observed. Same for the declared-vs-read lint test: file it, and feel free to seed it with the sweep above, including the shared-helper caveat that makes resolveScope's ref invisible to a body-only scan.

Fix the rider ordering and the docs line and I'm happy to merge.

[pbednarcik]

Both required items are in, plus one of the minors and the two follow-up filings.

Rider ordering — fixed at the seam you pointed at

The guard no longer appends mid-chain. It records the warn verdict in a request-scoped slot (withArgGuardRiderSlot, armed by the dispatch wrapper), and the wrapper attaches the rider after decorateResultWithWarming and both freshness decorators — i.e. after every rebuildTextResult — right before the response-ring capture, with momentum still last. A slot-less invocation (a handler called outside the dispatch wrapper) falls back to the old inline append, where no decorator runs.

Two properties from your verified-clean list survive by construction:

• The injection screen still covers the echoed key names. Sanitize wraps the handler, so a rider attached after the decorators would bypass scanResult. The attach step therefore runs detectInjection over the rider text itself and annotates _meta.gortex_security on a hit (never overwriting an existing notice). Same detector, same advisory shape.
• CLI framing is unaffected — the rider is still a second content block; Content[0] stays the canonical payload.

Both rebuild seams are pinned by tests that failed red against the pre-fix code with exactly your fingerprint (decorated block present, rider gone, one content block):

• TestWarnRiderSurvivesFreshnessRebuildE2E — real MCP frames, drifts the fixture file on disk after indexing, asserts one result carrying both freshness and _ignored_options. This is the test you asked for.
• TestWarnRiderSurvivesWarmingRebuildE2E — same call against a server whose readiness broadcaster reports mid-warmup, asserts warming and the rider coexist.

Docs

GORTEX_TOOL_ARG_GUARD is documented in docs/mcp.md, next to the prompt-injection screening paragraph: warn default, reject opt-in, off vocabulary, the shaping-key exemption, and the facade exemption.

Minor 3 — taken here

The echoed unknown-key list is now capped in both the rider and the reject error: at most 5 keys, each cut at 80 runes, with a (+N more) tail (toolArgGuardEcho, pinned by TestGuardEchoedKeyListIsCapped). Caller-controlled text stays bounded, which also shrinks what the attach-time screen has to reason about.

Scope follow-ups — filed

• mcp: lint test for closed tools reading undeclared argument keys #635 — the declared-vs-read lint test, seeded with your sweep (0/153 on this tip), the resolveScopeForRequest shared-helper caveat with the eight tool names, and the ref → repo alias-rewrite that currently masks it. Minor 2 (the stamp publishing a stricter contract than dispatch honors, sharpest on read_file's own format prose) is folded in there with your per-key counts, since it's the same declared-vs-honored contract question.
• mcp: extend the unknown-option guard to the facade surface #636 — the facade-lane guard, stating explicitly that merging this PR does not close MCP tools silently swallow unknown option keys the schemas promise to reject #597 on facade-v1, where it was observed.

Full internal/mcp is green on my Windows box except one pre-existing symlink-permission name that fails on clean main too.