fix(mcp): reject unknown facade repo selectors

[tiendungdev]

Summary

• reject repository-selector-like facade fields only when their normalized form is not consumed by the selected legacy handler
• validate top-level and every facade container, including unadvertised/cold-domain containers
• return structured invalid_argument responses with an options.repo suggestion when that selector is supported
• preserve existing consumed compatibility fields and keep validation on the single handleFacade path
• cover the literal change.detect source.repo_path reproduction from change.detect silently ignores source.repo_path and analyzes the active repository #549

This deliberately narrows the silent-fallback surface but does not claim to close every options.repo consumption gap. The canonical selector and the generic consumption-probe residual remain follow-up work.

Verification

• focused TestFacadeRepositoryValidation suite
• race-enabled facade repository validation, diff scope, rejection, and input tests
• go vet ./internal/mcp
• git diff --check
• branch merged upstream/main without force-push

Refs #549

[zzet]

Thanks for taking this on — the diagnosis in #549 was precise and the structured error you produce is exactly the shape the issue asked for. The problem is that the enforcement is considerably wider than the bug, and it leaves the original failure mode reachable through three other doors.

Everything below is A/B verified by driving handleFacade against real setupTestServer registrations on this branch (9e11785a) and on main (de6ffb03) with identical inputs.

What works

change.detect and review.run with source.repo_path now return exactly the right thing:

{"error_code":"invalid_argument",
 "message":"unknown field \"source.repo_path\"; use options.repo to select a repository",
 "data":{"field":"source.repo_path","container":"source","suggested_field":"options.repo"}}

CI is green, go test -race ./internal/mcp is clean, golangci-lint reports 0 issues.

1. The rule rejects calls that work today

facade_tools.go:1481:

if !facadeRepositorySelectorLike(field) &&
    (containerName != "source" || facadeCompatibilitySourceField(field)) { continue }

The containerName != "source" clause means every unadvertised field in the source container is rejected, checked against an 11-entry hand-written whitelist — not just repository selectors. Same input, both branches:

call	main	this branch
change.detect {"source":{"base":"HEAD"}}	accepted	invalid_argument
change.detect {"source":{"base_ref":"HEAD"}}	accepted, lowers to legacy base_ref	invalid_argument
change.detect {"source":{"repo":"x"}}	accepted, lowers to legacy repo	invalid_argument
change.detect {"output":{"repo":"x"}}	accepted, lowers to legacy repo	invalid_argument
review.run {"source":{"base_ref":"HEAD"}}	accepted, lowers to legacy base_ref	invalid_argument

The first row is the example in docs/mcp-facade-v1.md:314 verbatim. (In fairness, base was already a silent no-op on change.detect — detect_changes declares base_ref, not base — so the doc example was wrong before this PR. But it now fails loudly, and the doc is not updated in this branch.)

Rows 2-5 are different: those fields were consumed by the legacy handler and now hard-error.

An exhaustive sweep over every registered operation — for each closed advertised container, every field the legacy tool declares, checking "rejected by the new rule AND still lowered by normalizeFacadeArguments into a declared legacy field" — finds 135 rejected-but-consumed (operation, container, field) rows. 51 are repository-selector-like, which is the intent. The other 84 are collateral, spanning 17 change.* / review.* operations: source.path/start_line/end_line on change.ranges, source.lens/risk_gate on change.contract, source.keep on change.simulate, source.prior_review on review.critique, and so on. 41 operations lose a working repository selector via source.repo / output.repo.

Aggravating factor: the static tools/list schema that every MCP client actually sees declares source / options / output as {"type":"object","properties":{},"additionalProperties":true} (the freeObject helper, facade_tools.go:97-99). This change enforces the closed per-operation capabilities(detail:"schema") contract instead, which a client only obtains through a separate round-trip. So a client conforming to the advertised schema now gets rejected.

2. #549's failure mode is still reachable

Identical results on main and on this branch:

change.detect  {"operation":"detect","repo_path":"<repo B>"}            -> ACCEPTED, diffs the ACTIVE repo
change.detect  {"operation":"detect","context":{"repo_path":"<repo B>"}} -> ACCEPTED, diffs the ACTIVE repo
change.detect  {"operation":"detect","guard":{"repo_path":"<repo B>"}}   -> ACCEPTED, diffs the ACTIVE repo

Two gaps cause this:

• validateFacadeContainerFields iterates facadeContainerKeys only. Nothing enforces the published schema's top-level additionalProperties:false, and normalizeFacadeArguments (facade_tools.go:1708) copies every non-container top-level key straight into the legacy args.
• if !advertised { continue } skips any container the operation's schema omits — but normalizeFacadeArguments merges all six containers unconditionally. change.detect publishes only operation / options / source / output, so arguments, context and guard are skipped entirely.

That second point also means the comment at facade_tools.go:1461 — "production registrations always carry their full schema" — does not hold: real registrations routinely omit containers.

AC-5 is affected by the same gap. The cold-domain diff-backed operations publish every legacy field under arguments, so options and source are unadvertised and get no validation at all:

pr.risk  {"source":{"repo_path":"C:\\other"}}          -> ACCEPTED, normalized = map[repo_path:...] (no `repo` key)
pr.list  {"context":{"repository":"..."}}              -> ACCEPTED
publish_review.post {"source":{"repo_path":"..."}}     -> ACCEPTED   (this one is a write)

Same for pr.impact, pr.conflicts, pr.reviewers, pr.triage, remember.risk_ack — all of which reach diffRepoScope.

3. ~100x cost on every facade call

validateFacadeInput now calls s.facadeCapability(spec, true) unconditionally and uncached — before it even knows whether any container is present — and it runs twice per request (facade_tools.go:541 in handleFacade, then again at :959 inside invokeFacadeSpec).

handleFacade(change.detect) with a captured no-op legacy handler, so this is pure facade plumbing (Apple M1 Pro):

main:  1,468 ns/op    1,304 B/op     17 allocs/op
PR:  149,424 ns/op  305,887 B/op  1,040 allocs/op

An alloc_space profile puts 71% of it in facadeToolDefinition -> facadeCanonicalOperationNames -> facadeOperationSpecs -> addFacadeGroup, re-materialising the immutable 186-spec operation table. facadePublicCapabilitySchema and facadeCompleteRequiredSelectors each call it, and validation runs twice, so that's four full rebuilds per tool call. The json.Marshal + SHA-256 that compute the schema_hash the validator discards are only ~2%.

Absolute latency (~0.3 ms) is not user-perceptible against MCP round-trip times, so this isn't a correctness risk — but it's a 100x regression on the server's hottest path, over data the code documents as immutable.

4. Test coverage

facadeContainerValidationSchema() at the bottom of the new test file is a hand-rolled schema, and TestFacadeRejectsUnknownNestedFieldsBeforeLegacyDispatch derives its schema from a 5-field stub tool. So none of the four new tests asserts against a schema the server actually publishes — which is why the 84 collateral rejections pass CI.

Two smaller notes:

• The added t.TempDir() assertion in diff_repo_scope_test.go passes on main too. It's fine as coverage, but it isn't a guard for this change.
• TestFacadeForwardsSupportedRepositorySelectors uses a fake handler, so it can't distinguish a tracked repo from an untracked path. The PR description's "absolute untracked-path resolution" coverage claim is stronger than what the test shows.

Suggested direction

1. Reject only what isn't consumed. Probe normalizeFacadeArguments once for the field: if it lowers into a legacy field the selected handler declares, accept it (a deprecation note is fine). Reject only the unconsumed spellings — repo_path, repository, repository_path. Dropping the containerName != "source" catch-all removes all 84 collateral rejections while keeping the fix.
2. Extend the check outward, not inward. Enforce the top level, and treat an unadvertised container as having no legal fields rather than all of them — that closes context / guard / top-level repo_path and the pr.* / publish_review.post family in one go.
3. Memoize. A package-level sync.Once / sync.Map over facadeToolDefinition and facadeOperationSpecs() removes ~71% of the new cost with no behaviour change. Better still, add a facadeContainerSchema(spec) that returns just the container properties and skips request_shape, json.Marshal and the hash. Hoisting validation so it runs once per request rather than in both handleFacade and invokeFacadeSpec halves what's left.
4. Test against the real registry. One table test over setupTestServer's registered specs asserting the published schema — plus the end-to-end "active repo A, requested tracked repo B" case from AC-4 — would have caught items 1 and 2.

Smaller cleanups while you're in there:

• if capability := s.facadeCapability(spec, true); capability != nil — facadeCapability always returns a non-nil map, so the guard is dead. The input_schema type assertion below it is the real check.
• The double-negative predicate is hard to verify by reading; the positive form ("reject when selector-like, or when in source and not a known compat alias") is clearer.
• validFields := sortedFacadeMapKeys(allowed) allocates and sorts for every container even when no error is produced — worth moving inside the error branch.
• validateFacadeSelector sets data.field to the container name while the new code sets it to the dotted path; worth aligning.

Steps 1 and 2 together would close #549 properly and remove the regressions, which is why I'd rather see those than merge this and follow up.

[tiendungdev]

Reworked the validation direction in ad11aab. It now rejects only repository-like fields whose normalized form is not consumed by the selected real legacy handler, checks top-level plus every facade container (including unadvertised containers/cold domains), preserves consumed compatibility fields, and validates once in handleFacade. The hot path no longer constructs capability/schema/hash data. Tests now use setupTestServer and the real registry for the bypass and compatibility cases. Focused facade/diff scope tests and go vet ./internal/mcp pass. A full internal/mcp run also exposed existing Windows/platform failures; the one facade regression it identified (explore options.repo) was fixed and its focused regression test passes.

[zzet]

Round 2 clears both blockers from my last review. I verified that by measurement rather than by reading the diff — A/B on ad11aaba vs main d4801638, driven through the real registry via setupTestServer.

What's fixed

Over-rejection is gone. Sweep of every operation × 7 locations (top level + 6 containers) × the 4 selector spellings, plus every property of each legacy tool's declared input schema — 27,321 rows per side:

• rejected on PR but accepted and consumed on main: 0
• non-selector fields newly refused: 0 (commit 1's 84 collateral rows)
• all 12 named casualties now behave exactly as on main: change.detect {"source":{"base":"HEAD"}}, source.base_ref on change.detect and review.run, change.detect source.repo / output.repo, change.ranges path/start_line/end_line, change.contract lens/risk_gate, change.simulate keep, review.critique prior_review

Probing actual consumption instead of whitelisting a schema was the right call.

The ~100x hot path is gone. Same benchmark (handleFacade change.detect, no-op legacy handler), three trees:

tree	ns/op	B/op	allocs/op
main d4801638	1,438	1,304	17
9e11785a (commit 1)	811,196	318,639	1,283
ad11aaba (commit 2)	1,637	1,416	18

Residual is +1 alloc / 112 B per request (the locations append at facade_tools.go:1445) plus one normalizeFacadeArguments probe per selector-like field. Microsecond-scale — fine.

The three previously-open doors are closed (top-level repo_path, context.repo_path, guard.repo_path), and so are the cold arguments-only operations (pr.*, publish_review.post). Fail-open selector rows drop from 4,774 to 127.

Dropping the duplicate validateFacadeInput from invokeFacadeSpec is safe. One caller only, handleFacade validates first on the same spec, and input / req.GetArguments() are the same underlying map. Confirmed by mutation: moving the call to after dispatch turns three pre-existing tests red.

Gates on the head: build, go vet, go test -race ./internal/mcp (3,924 tests, 213 s), golangci-lint 0 issues. The three new tests are genuine guards — each goes red against main or against commit 1, which I don't take for granted.

Two changes before merge

1. The literal #549 reproduction has no test row

The rejection table covers change.detect at top level / context / guard, and source.repo_path on pr.risk and publish_review.post — but not change.detect + source.repo_path, which is the shape the issue actually reports.

That gap bites. I mutated validateFacadeRepositoryFields to skip the source container for change/review: the exact reported bug comes back, and go test ./internal/mcp still exits 0. Please add the row to TestFacadeRepositoryValidationRejectsUnconsumedPathsFromRealRegistry:

{
	name: "change detect source repo path", facade: "change", operation: "detect",
	input: map[string]any{"source": map[string]any{"repo_path": `C:\work\other-repo`}}, wantField: "source.repo_path",
},

While you're in there: nothing asserts Data["suggested_field"] == "options.repo", which is the second half of the issue's acceptance criterion 2.

2. Please change Fixes #549 to Refs #549

The guard doesn't cover the canonical selector. options.repo is unconditionally exempt at facade_tools.go:1456, making it the one spelling never checked — and it is silently dropped on roughly 100 of ~157 available operations. Measured end-to-end: explore(operation:"outline", options:{repo:"/not/tracked"}) and change(operation:"impact", options:{repo:"/not/tracked"}) both return output byte-identical to the no-selector control, with isError=false. A caller who follows the advice in your own error message lands back in the original failure mode.

The exemption comment says "Some facade middleware consumes it before the legacy handler does." I went looking for that consumer and couldn't find one — every repo reader is either a legacy handler or the shared scope layer, and both read the already-normalized map. What the exemption actually protects is TestFacadeExplorePathLowersToExplicitRepoSelector, which uses a synthetic registry whose stub doesn't declare repo.

Both this and the read.symbols case below are pre-existing on main, not regressions introduced here — this PR strictly shrinks the hole. But acceptance criterion 3 ("never falls back to the active repo when any repository-selector-like field was supplied but not consumed") isn't met, so the issue shouldn't auto-close. I'm happy to merge the narrower fix on its own merits.

One follow-up worth its own issue

facadeFieldConsumed returns true if any lowered probe key is declared, rather than the key the probed field actually lowered into. read.symbols shows the consequence: normalizeFacadeAliases sets include_source unconditionally (facade_tools.go:1852) and batch_symbols declares it, so the probe reports "consumed" for every field name — all 28 selector slots accepted, the guard entirely inert on that operation. Harm today is low (batch_symbols has no repo parameter and resolves ids by exact lookup), but the mechanism is generic: any future operation whose normalizer injects a declared key silently disables the guard there. Testing the probed field's own lowered form fixes it.

Investigated and dismissed — please don't chase these

• top-level repo on read.file flips accept → reject, but main accepted-and-ignored it, so no working capability is lost
• repo_path: null / "" now rejected — the field name is unconsumable regardless of value, and a null selector conveys no repository
• the explore path → repo alias overriding options.repo — pre-existing, and test-locked as intended behavior
• docs drift — capabilities already advertises additionalProperties:false, so every new rejection is predicted by a published schema. One sentence in docs/mcp-facade-v1.md noting that repository-selector-like fields are refused when unconsumed would still be welcome
• the Verification block in the PR description names one test commit 2 deleted (TestFacadeForwardsSupportedRepositorySelectors); the unanchored first command does still exercise all three replacements

Thanks for the rework — the consumption probe is a much better shape than the whitelist, and the measured result backs that up.

[tiendungdev]

Addressed the round-2 review on head 6bc147e.

• Added the literal change.detect plus source.repo_path row against the real registry.
• The row also asserts suggested_field equals options.repo.
• Merged current upstream/main without force-push and resolved the single facade_tools.go conflict semantically: retained one validation pass and the consumption-based repository guard, while incorporating the new input-shape helpers from main.
• Updated the PR body from Fixes change.detect silently ignores source.repo_path and analyzes the active repository #549 to Refs change.detect silently ignores source.repo_path and analyzes the active repository #549 and removed the over-strong closure and coverage claims.

Verification: focused repository-validator suite passed; the race-enabled facade validation and diff-scope set passed; go vet ./internal/mcp and git diff --check passed.

The canonical options.repo consumption gap and the generic probe-key residual remain explicitly out of this PR; #549 stays open.

[zzet]

Request changes: the literal change.detect/source.repo_path case is now fixed, the focused/full internal/mcp tests pass, all GitHub checks are green, and the branch merges cleanly with current main. However, the repository-selection boundary is not yet safe to merge. I reproduced an end-to-end wrong-repository write on this exact head: edit.file received options.repo pointing elsewhere, returned success, and changed the relative target in the active repository. The canonical selector must fail closed when the chosen operation cannot consume it, malformed selector values must be rejected, and the remediation path must match each operation's published schema. Please keep #549 open until the full selector invariant is covered.

[zzet]

Blocking — fail closed for an unconsumed canonical selector. This unconditional exemption lets options.repo pass even when the selected legacy operation has no repo field. I reproduced it through the real facade on this head: edit.file with options.repo pointing outside the active repository returned success and changed the active repository's relative target from before to after. Please exempt this field only when a shared scope layer or the selected handler demonstrably consumes it, require a non-empty string, and add an end-to-end mutating-facade regression.

[zzet]

The suggested field must come from the public operation schema. Cold domains such as pr.risk and publish_review.post expose the repository selector as arguments.repo, not options.repo. I verified that pr.risk publishes arguments and no options, while this error returns suggested_field: options.repo. Derive the public path from the capability/schema mapping and add assertions for both cold-domain operations.

[zzet]

Require a candidate-caused normalized field. This returns true when any normalized key is declared, even if it is unrelated to the probed selector. read.symbols injects include_source=true, so a context.repo_path probe is incorrectly considered consumed. Compare the normalized operation-only baseline with the candidate probe and require a newly produced declared key/value attributable to the selector.

[tiendungdev]

Addressed the repository-selection blockers in 06be468.

• Removed the unconditional options.repo exemption. A repository-like field is now accepted only when its probe produces a candidate-caused, legacy-declared normalized key relative to the operation-only baseline.
• Canonical public selectors are validated as non-empty strings before dispatch.
• suggested_field is derived from the selected operation's published schema, so change.detect reports options.repo while pr.risk and publish_review.post report arguments.repo.
• Added the read.symbols injected-baseline regression and an end-to-end edit.file test that proves an unconsumed options.repo is rejected before the active repository file can be written.
• Updated the synthetic explore fixture to declare the real legacy repo field, preserving the existing path-over-repo precedence without adding a production exemption.

Verification passed:

• go test ./internal/mcp -run TestFacade -count=1 -timeout=240s
• go test -race ./internal/mcp -run TestFacade(RepositoryValidation|EditFileRejectsUnconsumedRepositoryBeforeWrite|ExplorePathLowersToExplicitRepoSelector) -count=1 -timeout=240s
• go vet ./internal/mcp
• git diff --check

[tiendungdev]

CI failure diagnosed and addressed in 5389756.

Root cause:

• The failing GitHub merge commit used base 28f24d4, immediately after fix(mcp): guide unindexed symbol renames #574 introduced internal/indexer/extract_source.go.
• That base did not yet contain fix(mcp): harden unindexed rename recovery #580 / fbd49d6, which restores the owner-indexer extraction implementation required by ExtractSource.
• Consequently build, lint, test, benchmark, ONNX, and skill-drift jobs all converged on the same compile error: idx.extractFileCtx undefined. This was not caused by the facade validation patch.
• The govulncheck job additionally hit an independent setup-go module-cache extraction collision.

I merged current upstream/main, including #580. The merge was conflict-free and the PR diff against current main remains limited to the four facade-scope files.

Local verification on the merged head:

• go build ./cmd/gortex/ — pass
• go test ./internal/mcp -run TestFacade -count=1 -timeout=300s — pass
• go test ./internal/indexer -run "Test.*ExtractSource|TestExtract" -count=1 -timeout=300s — pass
• go vet ./internal/mcp ./internal/indexer — pass
• git diff upstream/main...HEAD --check — pass

The focused Windows race command spent the outer 904-second budget in compile/instrumentation and was inconclusive; it emitted no test failure. The new Linux CI run will provide the clean platform signal.