diff --git a/docs/mcp-facade-v1.md b/docs/mcp-facade-v1.md index 0faac1cc..186f6824 100644 --- a/docs/mcp-facade-v1.md +++ b/docs/mcp-facade-v1.md @@ -227,7 +227,13 @@ High-frequency fields are direct and typed. In particular, `edit` advertises `ma Cold domain tools accept an `arguments` object; common tools may additionally accept `options`, `source`, or `context`. Repository/project/scope fields use the operation schema returned by `capabilities` and are passed through to the handler. `output` is a stable open object for response shaping such as `format`, `max_bytes`, `limit`, `cursor`, and `fields`; adding another response control does not change the outer tool schema. Cursors remain opaque. -A repository-selector field — `repo`, `repo_path`, `repository`, or `repository_path`, in any letter case, at the top level or in any container — MUST be refused with `invalid_argument` when the selected operation cannot consume it. Silently dropping it answers about the active repository while the caller believes another one was addressed, which is a wrong answer the caller cannot detect, and on a write operation it is a write to the wrong repository. Most operations accept no repository selector at all and run against the active project; their refusal carries `data.reason = "no_repository_selector"` and names `workspace_admin.set_active_project` as the way to change scope. An operation that does publish one names it in `data.suggested_field` — `options.repo` for the common domains, `arguments.repo` for cold domains. `capabilities(...detail="schema")` is authoritative for which selector, if any, an operation accepts. +A field that states **which repository or workspace** an operation should act on MUST be refused with `invalid_argument` when the selected operation cannot consume it. Silently dropping it answers about the active repository while the caller believes another one was addressed, which is a wrong answer the caller cannot detect, and on a write operation it is a write to the wrong repository. + +The rule applies at the top level and in every container, in any letter case, and covers the spellings a caller may reasonably invent as well as the published one: `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`. Inventing a name does not make the intent less clear, so it must not make the failure quieter. `path` and `scope` are deliberately excluded: on this surface they name a file or a working-tree scope far more often than a repository. + +Refusal is decided per operation by whether the field reaches a reader, not by the name alone — an operation that genuinely consumes `workspace` or `root` still receives it. Most operations consume no repository selector at all and run against the active project; their refusal carries `data.reason = "no_repository_selector"` and names `workspace_admin.set_active_project` as the way to change scope. An operation that does publish one names it in `data.suggested_field` — `options.repo` for the common domains, `arguments.repo` for cold domains. `capabilities(...detail="schema")` is authoritative for which selector, if any, an operation accepts. + +Fields outside this class that an operation does not consume are still forwarded and ignored. Closing that wider gap requires enumerating every server-side reader — the handler, the response layer, and facade middleware each read from the normalized arguments — and an incomplete enumeration would refuse working calls, so the guarantee here is deliberately limited to fields that name a target. ### 8.4 Response compatibility and metadata diff --git a/internal/mcp/facade_target_selector_test.go b/internal/mcp/facade_target_selector_test.go new file mode 100644 index 00000000..d5240e7e --- /dev/null +++ b/internal/mcp/facade_target_selector_test.go @@ -0,0 +1,168 @@ +package mcp + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + mcpgo "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" +) + +// A caller who invents a spelling for "operate on that repository" has stated +// the same intent as one who writes `repo`. Answering about the active +// repository instead is wrong for every spelling, so each must fail closed — +// and on a write, before anything is written. +func TestFacadeRefusesInventedRepositorySelectorsBeforeWrite(t *testing.T) { + srv, root := setupTestServer(t) + target := filepath.Join(root, "main.go") + before, err := os.ReadFile(target) + require.NoError(t, err) + + elsewhere := t.TempDir() + for _, container := range []string{"options", "arguments", "source", "context", "guard", "output"} { + for _, spelling := range []string{ + "repo", "repo_path", "repository", "repository_path", + "repo_root", "repository_root", "repoPath", "repo_dir", + "root", "cwd", "dir", "worktree", "base_repo", + "workspace", "project", + } { + t.Run(container+"."+spelling, func(t *testing.T) { + req := mcpgo.CallToolRequest{} + req.Params.Name = "edit" + req.Params.Arguments = map[string]any{ + "operation": "file", + "target": map[string]any{"file": "main.go"}, + "match": string(before), + "replacement": string(before) + "\n// must not be written\n", + container: map[string]any{spelling: elsewhere}, + } + result, err := srv.handleFacade(context.Background(), "edit", req) + require.NoError(t, err) + require.True(t, result.IsError, "edit.file must refuse %s.%s", container, spelling) + + after, err := os.ReadFile(target) + require.NoError(t, err) + require.Equal(t, before, after, "the active repository must be untouched") + }) + } + } +} + +// The refusal must not cost a working capability: an operation that reads the +// selector still gets it, whichever container carried it. +func TestFacadeKeepsConsumedRepositorySelectors(t *testing.T) { + srv, _ := setupTestServer(t) + spec, ok := srv.facades.operation("change", "detect") + require.True(t, ok) + + for _, container := range []string{"options", "source", "output", "arguments"} { + t.Run(container, func(t *testing.T) { + input := map[string]any{ + "operation": "detect", + container: map[string]any{"repo": "tracked-repo"}, + } + require.Nil(t, srv.validateFacadeInput(spec, input)) + require.Equal(t, "tracked-repo", normalizeFacadeArguments(spec, input)["repo"]) + }) + } +} + +// `path` and `scope` name a file or a working-tree scope far more often than a +// repository on this surface. Promoting them to target selectors would refuse +// working calls, so the exclusion is deliberate and pinned here. +func TestFacadeSelectorClassExcludesOverloadedNames(t *testing.T) { + for _, field := range []string{"path", "scope", "file", "query", "symbol", "target"} { + require.False(t, facadeRepositorySelectorLike(field), + "%q addresses something other than a repository on this surface", field) + } + for _, field := range []string{"REPO", "Repo_Path", " repository "} { + require.True(t, facadeRepositorySelectorLike(field), + "%q is a repository selector whatever its casing or padding", field) + } +} + +// The friendly match/replacement pair belongs to the edit facade. Translating +// it everywhere silently dropped remember.edit_memory's replacement into a +// field edit_memory does not declare, so the memory was edited with no +// replacement text and the call still reported success. +func TestFacadeEditMemoryReceivesItsOwnReplacementVocabulary(t *testing.T) { + srv, _ := setupTestServer(t) + spec, ok := srv.facades.operation("remember", "edit_memory") + require.True(t, ok) + require.True(t, srv.legacyDeclaresField(spec.Legacy, "replacement"), + "edit_memory speaks replacement natively; the premise of this test is that it must receive it") + + captured, ok := srv.facades.legacy(spec.Legacy) + require.True(t, ok) + var got map[string]any + srv.facades.capture(captured.tool, func(_ context.Context, req mcpgo.CallToolRequest) (*mcpgo.CallToolResult, error) { + got = req.GetArguments() + return mcpgo.NewToolResultText("{}"), nil + }) + + req := mcpgo.CallToolRequest{} + req.Params.Name = "remember" + req.Params.Arguments = map[string]any{ + "operation": "edit_memory", + "arguments": map[string]any{"id": "mem-1", "pattern": "old", "replacement": "new"}, + } + result, err := srv.handleFacade(context.Background(), "remember", req) + require.NoError(t, err) + require.False(t, result.IsError, "%s", toolResultText(result)) + + require.Equal(t, "new", got["replacement"], "the handler must receive the caller's replacement") + require.NotContains(t, got, "new_string", "the edit facade's vocabulary must not leak here") +} + +// The edit facade still translates its own published vocabulary. +func TestFacadeEditAliasTranslationMatchesPublishedVocabulary(t *testing.T) { + for _, facade := range facadeToolNames() { + properties := facadeToolDefinition(facade).InputSchema.Properties + _, publishesMatch := properties["match"] + _, publishesReplacement := properties["replacement"] + require.Equal(t, publishesMatch && publishesReplacement, facadeTranslatesEditAliases(facade), + "%s translates match/replacement only if it publishes them", facade) + } + + srv, _ := setupTestServer(t) + spec, ok := srv.facades.operation("edit", "file") + require.True(t, ok) + lowered := normalizeFacadeArguments(spec, map[string]any{ + "operation": "file", "match": "old", "replacement": "new", + }) + require.Equal(t, "old", lowered["old_string"]) + require.Equal(t, "new", lowered["new_string"]) +} + +// A refusal names the operation and, where one exists, the selector to use. +func TestFacadeInventedSelectorRefusalIsActionable(t *testing.T) { + srv, _ := setupTestServer(t) + + detect, ok := srv.facades.operation("change", "detect") + require.True(t, ok) + refused := srv.validateFacadeInput(detect, map[string]any{ + "operation": "detect", + "options": map[string]any{"repo_root": "/work/other"}, + }) + require.NotNil(t, refused) + var suggested StructuredError + require.NoError(t, json.Unmarshal([]byte(toolResultText(refused)), &suggested)) + require.Equal(t, "options.repo_root", suggested.Data["field"]) + require.Equal(t, "options.repo", suggested.Data["suggested_field"]) + + file, ok := srv.facades.operation("edit", "file") + require.True(t, ok) + refusedWrite := srv.validateFacadeInput(file, map[string]any{ + "operation": "file", + "options": map[string]any{"workspace": "other"}, + }) + require.NotNil(t, refusedWrite) + var explained StructuredError + require.NoError(t, json.Unmarshal([]byte(toolResultText(refusedWrite)), &explained)) + require.Equal(t, "options.workspace", explained.Data["field"]) + require.Equal(t, "no_repository_selector", explained.Data["reason"]) + require.Contains(t, explained.Message, "workspace_admin.set_active_project") +} diff --git a/internal/mcp/facade_tools.go b/internal/mcp/facade_tools.go index 45b19f18..3c0fa995 100644 --- a/internal/mcp/facade_tools.go +++ b/internal/mcp/facade_tools.go @@ -1486,7 +1486,7 @@ func (s *Server) validateFacadeInput(spec facadeOperationSpec, input map[string] }) } } - if invalid := s.validateFacadeRepositoryFields(spec, input); invalid != nil { + if invalid := s.validateFacadeRequestFields(spec, input); invalid != nil { return invalid } for _, field := range []string{"target", "to"} { @@ -1528,11 +1528,52 @@ func (s *Server) validateFacadeInput(spec facadeOperationSpec, input map[string] return nil } -// validateFacadeRepositoryFields rejects repository-selector spellings only -// when the selected legacy handler cannot consume their normalized form. This -// preserves working compatibility aliases while closing every top-level and -// nested-container path that would otherwise silently target the active repo. -func (s *Server) validateFacadeRepositoryFields(spec facadeOperationSpec, input map[string]any) *mcpgo.CallToolResult { +// validateFacadeRequestFields refuses any top-level or container field the +// selected operation can neither advertise nor consume. +// +// The published per-operation schema closes every container, so a field outside +// it reaches no handler: normalization copies it into the legacy arguments and +// nothing reads it. Dropping it silently is not neutral — the caller stated an +// intent the server then ignored, and for a field that names a target (a +// repository, a workspace, a path) the operation proceeds against the active +// one instead. On a write that is a write to the wrong place, with a success +// result. Refusing costs a retry; accepting-and-ignoring costs correctness the +// caller cannot audit. +// +// A field the published schema omits is still accepted when it actually reaches +// the handler, which keeps every working compatibility alias alive. +func (s *Server) validateFacadeRequestFields(spec facadeOperationSpec, input map[string]any) *mcpgo.CallToolResult { + canonicalPath := s.facadePublicRepositoryField(spec) + return forEachFacadeRequestField(input, func(containerName, field, path string, value any) *mcpgo.CallToolResult { + if !facadeRepositorySelectorLike(field) { + return nil + } + if path == canonicalPath { + text, ok := value.(string) + if !ok || strings.TrimSpace(text) == "" { + return NewStructuredErrorResult(StructuredError{ + ErrorCode: ErrCodeInvalidArgument, + Message: fmt.Sprintf("%s must be a non-empty string", path), + Data: map[string]any{ + "field": path, "expected_type": "non-empty string", + }, + }) + } + } + if s.facadeFieldConsumed(spec, containerName, field, value) { + return nil + } + return facadeUnknownFieldResult(spec, canonicalPath, containerName, path, field) + }) +} + +// forEachFacadeRequestField walks the top level and every container in a stable +// order, skipping the envelope keys the dispatcher consumes itself, and returns +// the first refusal a check produces. +func forEachFacadeRequestField( + input map[string]any, + check func(containerName, field, path string, value any) *mcpgo.CallToolResult, +) *mcpgo.CallToolResult { locations := append([]string{""}, facadeContainerKeys...) for _, containerName := range locations { fields := input @@ -1544,54 +1585,80 @@ func (s *Server) validateFacadeRepositoryFields(spec facadeOperationSpec, input } } for _, field := range sortedFacadeMapKeys(fields) { - if !facadeRepositorySelectorLike(field) { + if containerName == "" && isFacadeEnvelopeKey(field) { + // The dispatcher consumes these itself and they never reach the + // handler as fields, so no operation schema enumerates them. + // Container bodies are walked by this same loop; target and to + // are checked by validateFacadeSelector. continue } path := field if containerName != "" { path = containerName + "." + field } - canonicalPath := s.facadePublicRepositoryField(spec) - if path == canonicalPath { - value, ok := fields[field].(string) - if !ok || strings.TrimSpace(value) == "" { - return NewStructuredErrorResult(StructuredError{ - ErrorCode: ErrCodeInvalidArgument, - Message: fmt.Sprintf("%s must be a non-empty string", path), - Data: map[string]any{ - "field": path, "expected_type": "non-empty string", - }, - }) - } - } - if s.facadeFieldConsumed(spec, containerName, field, fields[field]) { - continue - } - data := map[string]any{"field": path} - if containerName != "" { - data["container"] = containerName + if refusal := check(containerName, field, path, fields[field]); refusal != nil { + return refusal } - message := fmt.Sprintf("unknown field %q", path) - if canonicalPath != "" { - data["suggested_field"] = canonicalPath - message += fmt.Sprintf("; use %s to select a repository", canonicalPath) - } else { - // Refusing without saying why leaves the caller retrying - // spellings of a selector this operation will never accept. - data["reason"] = "no_repository_selector" - message += fmt.Sprintf("; %s.%s accepts no repository selector and runs against the active project"+ - " — switch it with workspace_admin.set_active_project", spec.Facade, spec.Operation) - } - return NewStructuredErrorResult(StructuredError{ - ErrorCode: ErrCodeInvalidArgument, - Message: message, - Data: data, - }) } } return nil } +// facadeUnknownFieldResult explains a refusal in the terms the caller can act +// on: where the operation does take a repository, or that it takes none, or +// which fields the container advertises. +func facadeUnknownFieldResult( + spec facadeOperationSpec, + canonicalPath, containerName, path, field string, +) *mcpgo.CallToolResult { + data := map[string]any{"field": path} + if containerName != "" { + data["container"] = containerName + } + message := fmt.Sprintf("unknown field %q", path) + switch { + case canonicalPath != "": + data["suggested_field"] = canonicalPath + message += fmt.Sprintf("; use %s to select a repository", canonicalPath) + default: + // Refusing without saying why leaves the caller retrying spellings of a + // selector this operation will never accept. + data["reason"] = "no_repository_selector" + message += fmt.Sprintf("; %s.%s accepts no repository selector and runs against the active project"+ + " — switch it with workspace_admin.set_active_project", spec.Facade, spec.Operation) + } + return NewStructuredErrorResult(StructuredError{ + ErrorCode: ErrCodeInvalidArgument, + Message: message, + Data: data, + }) +} + +// facadeTranslatesEditAliases reports whether the facade publishes the friendly +// match/replacement pair that normalizeFacadeArguments lowers into the legacy +// old_string/new_string vocabulary. TestFacadeEditAliasTranslationMatchesPublishedVocabulary +// pins this to the facade definitions so the two cannot drift. +func facadeTranslatesEditAliases(facade string) bool { + return facade == "edit" +} + +// isFacadeEnvelopeKey reports whether a top-level key belongs to the request +// envelope rather than to the operation's arguments. normalizeFacadeArguments +// drops exactly these before merging, so the two must agree. +func isFacadeEnvelopeKey(key string) bool { + switch key { + case "operation", "arguments", "options", "source", "context", "guard", "output", "target", "to": + return true + default: + return false + } +} + +// facadeFieldConsumed reports whether the field, lowered through the same +// normalization the dispatcher applies, reaches the selected handler as a field +// that handler declares. The probe is diffed against an operation-only baseline +// so a key the normalizer injects unconditionally cannot make every field look +// consumed, and fixed arguments do not count: the caller's value is discarded. func (s *Server) facadeFieldConsumed(spec facadeOperationSpec, containerName, field string, value any) bool { baseline := normalizeFacadeArguments(spec, map[string]any{"operation": spec.Operation}) probe := map[string]any{"operation": spec.Operation} @@ -1613,15 +1680,7 @@ func (s *Server) facadeFieldConsumed(spec facadeOperationSpec, containerName, fi } // facadePublicRepositoryField reports where the operation's published schema -// exposes its repository selector, as a dotted path, or "" when the operation -// has none. -// -// Building the published schema re-materialises the immutable operation table -// and re-runs the lowering probe for every legacy property, which costs two -// orders of magnitude more than the rest of facade dispatch. The answer depends -// only on the spec and the captured legacy schema, so it is memoized per -// registry and invalidated whenever a late registration changes what is -// captured. +// exposes its repository selector, as a dotted path, or "" when it has none. func (s *Server) facadePublicRepositoryField(spec facadeOperationSpec) string { key := spec.Facade + "." + spec.Operation if cached, ok := s.facades.cachedRepositoryField(key); ok { @@ -1649,9 +1708,31 @@ func (s *Server) resolveFacadePublicRepositoryField(spec facadeOperationSpec) st return "" } +// facadeRepositorySelectorLike reports whether a field name states WHERE an +// operation should act — the repository, workspace, project, or path it should +// target. +// +// This is the class where dropping a field silently is not merely wasteful but +// wrong: the operation proceeds against the active target while the caller +// believes it addressed the one they named, and on a write that is a write to +// the wrong place reported as success. Every name here is checked against the +// consumption probe, so an operation that genuinely reads one still gets it; +// only the spellings that reach no reader are refused. +// +// The list is deliberately wider than the vocabulary the surface publishes. +// A caller who invents `repo_root` or `cwd` has stated the same intent as one +// who writes `repo`, and answering about a different target is equally wrong. +// +// It stops at names that unambiguously address a repository or workspace. +// `path` and `scope` are excluded on purpose: across this surface they far more +// often mean a file path or a working-tree scope than a repository, so treating +// them as target selectors would refuse working calls to buy nothing. func facadeRepositorySelectorLike(field string) bool { switch strings.ToLower(strings.TrimSpace(field)) { - case "repo", "repo_path", "repository", "repository_path": + case "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": return true default: return false @@ -1831,8 +1912,7 @@ func normalizeFacadeArguments(spec facadeOperationSpec, input map[string]any) ma mergeFacadeObject(out, input["guard"]) mergeFacadeObject(out, input["output"]) for key, value := range input { - switch key { - case "operation", "arguments", "options", "source", "context", "guard", "output", "target", "to": + if isFacadeEnvelopeKey(key) { continue } out[key] = value @@ -1845,22 +1925,27 @@ func normalizeFacadeArguments(spec facadeOperationSpec, input map[string]any) ma out["to_"+key] = value } } - // Friendly edit aliases become the exact legacy vocabulary. - if match, ok := out["match"]; ok { - if spec.Legacy == "edit_symbol" { - out["old_source"] = match - } else { - out["old_string"] = match + // Friendly edit aliases become the exact legacy vocabulary. Only the facade + // that publishes them may translate: elsewhere `match` and `replacement` are + // a handler's own vocabulary, and rewriting them drops the caller's value + // into a field nobody reads. + if facadeTranslatesEditAliases(spec.Facade) { + if match, ok := out["match"]; ok { + if spec.Legacy == "edit_symbol" { + out["old_source"] = match + } else { + out["old_string"] = match + } + delete(out, "match") } - delete(out, "match") - } - if replacement, ok := out["replacement"]; ok { - if spec.Legacy == "edit_symbol" { - out["new_source"] = replacement - } else { - out["new_string"] = replacement + if replacement, ok := out["replacement"]; ok { + if spec.Legacy == "edit_symbol" { + out["new_source"] = replacement + } else { + out["new_string"] = replacement + } + delete(out, "replacement") } - delete(out, "replacement") } normalizeFacadeAliases(spec, input, out) for key, value := range spec.Fixed {