fix(mcp): cut the facade repository-selector cost and explain its refusals - #583
Merged
Conversation
Resolving an operation's published repository selector builds its whole public capability schema, which re-materialises the immutable operation table and re-probes every legacy property. That ran on every request carrying a repository selector — including options.repo, the canonical one — putting two orders of magnitude more work on the hottest path than the rest of facade dispatch. Measured with handleFacade over a stubbed legacy handler (M1 Pro): change.detect, no selector 1,488 ns 1,128 B 17 allocs change.detect + options.repo 89,707 ns 160,884 B 659 allocs read.file + options.repo 118,789 ns 174,940 B 832 allocs alloc_space put 62% of it in facadeCanonicalOperationNames -> facadeOperationSpecs -> addFacadeGroup, plus 16% in regexp compilation. The answer depends only on the operation spec and the captured legacy schema, so memoize it on the registry and drop the memo whenever capture changes the captured set — optional handlers register after NewServer, so a pre-registration answer must not survive. change.detect + options.repo 6,437 ns 2,200 B 27 allocs read.file + options.repo 12,668 ns 2,096 B 33 allocs
Refusing an unconsumable repository selector is right — dropping it silently answers about the active repository while the caller believes another one was addressed, and on a write it writes to the wrong place. But most operations publish no repository selector at all, and those refusals said only `unknown field "options.repo"`, which reads as a typo and invites the caller to retry other spellings of a selector that will never be accepted. Name the operation, say it accepts no selector, and point at the way to change scope instead. The refusal now carries reason=no_repository_selector so a caller can branch on it without parsing prose; operations that do publish a selector keep naming it in suggested_field. Also fix the request envelope in the facade specification: its example passed options.repo to read.file, which read.file does not accept and the server now refuses. Every field in the replacement is published by read.file, and the surrounding text states the refusal rule and that capabilities remains authoritative for which selector an operation takes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #553, which closed the silent wrong-repository fallback behind #549. Four commits: two fix what that merge shipped with, two close the residual it left open.
1. The published-schema lookup ran per request
facadePublicRepositoryFieldbuilds the operation's whole public capability schema, which re-materialises the immutable operation table and re-probes every legacy property. It ran on every call carrying a repository selector — includingoptions.repo, the canonical one. Memoized on the registry and dropped whenevercapturechanges the captured set, since optional handlers register afterNewServerand a pre-registration answer must not survive.change.detect, no selectorchange.detect+options.reporead.file+options.repoalloc_spaceattributed 62% of the removed cost tofacadeCanonicalOperationNames→facadeOperationSpecs→addFacadeGroup, plus 16% to regexp compilation.2. Refusals on operations with no selector said nothing useful
Most operations publish no repository selector; those refusals read
unknown field "options.repo", which looks like a typo and invites retrying spellings that can never be accepted. The refusal now names the operation, points atworkspace_admin.set_active_project, and carriesreason: "no_repository_selector". The facade specification's own §8.1 envelope passedoptions.repotoread.file, whichread.filedoes not accept and the server now refuses — replaced with an envelope whose every fieldread.filepublishes.3. The guard matched four names, so the same bug had other doors
options.workspace,options.root,options.cwd,options.repo_root,options.worktreeand friends were still silently dropped, and onedit.filethat is a write to the wrong repository reported as success — the same failure the four-name guard was added to close, reachable by writing the intent a different way. The class now covers the spellings a caller may reasonably reach for. Membership is by name; refusal is still decided per operation by the consumption probe, so an operation that genuinely readsworkspaceorrootkeeps receiving it.pathandscopestay out — on this surface they name a file or a working-tree scope far more often than a repository.4. The edit facade's aliases were eating another handler's vocabulary
match/replacementare theeditfacade's caller-facing names for the legacyold_string/new_stringpair, and the lowering rewrote them on every facade.edit_memorydeclaresreplacementitself, soremember.edit_memory's replacement text was renamed into a field that handler does not declare: the memory was edited with no replacement text and the call reported success, while the published schema advertisedarguments.replacement. Now only the facade that publishes the pair translates it.Measured
Every reachable operation × 7 locations (top level + 6 containers) × the whole 342-name facade vocabulary — 378,094 rows — against merged main:
The nine-door
edit.filewrite probe refuses every spelling at every location with the target file unchanged;options.workspace— the door this PR opened with — is now closed.Tests
Five guards, each mutation-verified to go red when its mechanism is removed: the allocation bound on the selector path, memo invalidation on late registration, the
no_repository_selectorguidance, refusal of every invented spelling before a write, andremember.edit_memoryreceiving its own vocabulary. Plus drift pins: the alias predicate against the facade definitions, and the deliberate exclusion ofpath/scope.Verification
go test -race ./internal/mcp— pass (127 s)golangci-lint run ./internal/mcp/...— 0 issuesgo vet ./internal/mcp,go build ./cmd/gortex/,git diff --check— passKnown limit, recorded rather than implied
Fields outside the target-selector class that an operation does not consume are still forwarded and ignored. Refusing those generically needs an enumeration of every server-side reader, and the handler, the response layer and facade middleware each read the normalized arguments:
output.format,options.new_user_taskandoptions.fieldsare each honoured by a reader no handler schema mentions. I built that generic rule, measured it, and it refused working calls in exactly those places — an incomplete enumeration trades a silent-drop bug for a broken feature. The specification now states the limit instead of implying a guarantee that does not hold.Refs #549