fix(mcp): close the remaining doors to a silent wrong-repository target - #585
Merged
Conversation
The repository guard matched four field names, so a caller who wrote `options.workspace`, `options.root`, `options.cwd`, `options.repo_root` or `options.worktree` still had it silently dropped and the operation ran against the active repository. On edit.file that was 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. Inventing a name does not make the intent less clear, so it must not make the failure quieter. The class now covers the spellings a caller may reasonably reach for. Membership is by name, but refusal is still decided per operation by the consumption probe, so an operation that genuinely reads `workspace` or `root` keeps receiving it. `path` and `scope` stay out: on this surface they name a file or a working-tree scope far more often than a repository, and promoting them refuses working calls to buy nothing. Measured over every reachable operation x 7 locations x the whole 342-name facade vocabulary, 378,094 rows, against the previous behaviour: rows consumed before and refused now 0 silently dropped before, refused now 10,353 consumed rows preserved 7,547 Fields outside this class that an operation does not consume are still forwarded and ignored. Closing that needs an enumeration of every server-side reader — handler, response layer, and facade middleware all read the normalized arguments — and an incomplete one refuses working calls: `output.format`, `options.new_user_task` and `options.fields` are each honoured by a reader no handler schema mentions. The specification now records that limit rather than implying a guarantee that does not hold.
…abulary `match` and `replacement` are the edit facade's caller-facing names for the legacy `old_string` / `new_string` pair, and the lowering rewrote them on every facade. `edit_memory` declares `replacement` itself, so remember.edit_memory's replacement text was renamed into a field that handler does not declare: the memory was edited with no replacement, and the call reported success. The public schema advertises `arguments.replacement` for that operation, so this was the server ignoring something it had promised to honour. Translate only on the facade that publishes the friendly pair. Elsewhere `match` and `replacement` are a handler's own vocabulary and must arrive unchanged. A test pins the predicate to the facade definitions so the two cannot drift apart. Found by sweeping the whole facade vocabulary against every operation and asking, for each field, whether the value reached the handler under a name the handler declares. These seven rows — replacement at every location on remember.edit_memory — were the only ones where the answer was no while the schema said yes.
…selector-coverage * 'main' of github.com:zzet/gortex: perf(mcp): presize the physical-evidence read from the observed size test(mcp): pin the physical-evidence confinement wiring and non-blocking open fix(mcp): judge the physical-evidence secret gate on the hashed buffer fix(codex): bound the `codex --version` probe with a deadline fix(churn): stop spawning a process per file to test path existence fix(mcp): harden physical read evidence fix(mcp): stabilize physical read evidence fix(mcp): keep agent tool list under budget feat(mcp): add physical file read evidence
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 #583. That PR fixed the cost and the wording of the repository-selector guard; this one closes the two ways a caller's stated target could still be silently discarded.
1. The guard matched four names, so the same bug had other doors
#553 refused
repo,repo_path,repositoryandrepository_pathwhen the selected operation could not consume them. A caller who wroteoptions.workspace,options.root,options.cwd,options.repo_rootoroptions.worktreestill had it silently dropped and the operation ran against the active repository. 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.Inventing a name does not make the intent less clear, so it must not make the failure quieter. The class now covers the spellings a caller may reasonably reach for:
repo,repo_path,repository,repository_path,repo_root,repository_root,repoPath,repo-path,repo_dir,root,cwd,dir,worktree,work_tree,base_repo,workspace,project.Membership is by name, but refusal is still decided per operation by the consumption probe, so an operation that genuinely reads
workspaceorrootkeeps receiving it.pathandscopestay out and a test pins that: on this surface they name a file or a working-tree scope far more often than a repository, and promoting them refuses working calls to buy nothing.2. The edit facade's aliases were eating another handler's vocabulary
matchandreplacementare 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. Only the facade that publishes the friendly pair translates it now, with a test pinning the predicate to the facade definitions so the two cannot drift.Measured
Every reachable operation × 7 locations (top level + 6 containers) × the whole 342-name facade vocabulary — 378,094 rows — against the previous behaviour:
The nine-door
edit.filewrite probe refuses every spelling at every location with the target file byte-identical;options.workspace, which wrote to the active repository before, is now refused.Tests
Both mechanisms are mutation-verified — narrowing the class back to four names turns
TestFacadeRefusesInventedRepositorySelectorsBeforeWriteandTestFacadeInventedSelectorRefusalIsActionablered; translating the aliases everywhere again turnsTestFacadeEditMemoryReceivesItsOwnReplacementVocabularyred. Plus drift pins for the alias predicate against the facade definitions and for the deliberate exclusion ofpath/scope.Verification
go test ./internal/mcpon the merge with current main — passgo test -race ./internal/mcp— pass (127 s, before the merge commit)golangci-lint run ./internal/mcp/...— 0 issuesgo 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 and measured it — it refused working calls in exactly those places, trading a silent-drop bug for a broken feature.docs/mcp-facade-v1.mdnow states the limit instead of implying a guarantee that does not hold.Refs #549