diff --git a/internal/mcp/diff_repo_scope_test.go b/internal/mcp/diff_repo_scope_test.go index 734f1316..c50239b0 100644 --- a/internal/mcp/diff_repo_scope_test.go +++ b/internal/mcp/diff_repo_scope_test.go @@ -62,11 +62,13 @@ func TestDiffRepoScope(t *testing.T) { require.Equal(t, repoA, root) require.Equal(t, "repo-a", prefix) - // An unknown selector resolves nothing — the caller errors instead of - // falling back to an unrelated repo. - root, prefix = srv.diffRepoScope(ctx, "nope") - require.Empty(t, root) - require.Empty(t, prefix) + // Unknown prefix and absolute path selectors both resolve nothing — the + // caller errors instead of falling back to an unrelated active repo. + for _, selector := range []string{"nope", t.TempDir()} { + root, prefix = srv.diffRepoScope(ctx, selector) + require.Empty(t, root) + require.Empty(t, prefix) + } // No selector + multiple tracked repos + no session binding: ambiguous. root, prefix = srv.diffRepoScope(ctx, "") diff --git a/internal/mcp/facade_container_validation_test.go b/internal/mcp/facade_container_validation_test.go new file mode 100644 index 00000000..a01b288d --- /dev/null +++ b/internal/mcp/facade_container_validation_test.go @@ -0,0 +1,207 @@ +package mcp + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + mcpgo "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" +) + +func TestFacadeRepositoryValidationRejectsUnconsumedPathsFromRealRegistry(t *testing.T) { + srv, _ := setupTestServer(t) + + for _, test := range []struct { + name string + facade string + operation string + input map[string]any + wantField string + wantSuggested string + }{ + { + 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", wantSuggested: "options.repo", + }, + { + name: "top-level repo path", facade: "change", operation: "detect", + input: map[string]any{"repo_path": `C:\work\other-repo`}, wantField: "repo_path", + }, + { + name: "context repo path", facade: "change", operation: "detect", + input: map[string]any{"context": map[string]any{"repo_path": `C:\work\other-repo`}}, wantField: "context.repo_path", + }, + { + name: "guard repo path", facade: "change", operation: "detect", + input: map[string]any{"guard": map[string]any{"repo_path": `C:\work\other-repo`}}, wantField: "guard.repo_path", + }, + { + name: "cold pr source repo path", facade: "pr", operation: "risk", + input: map[string]any{"source": map[string]any{"repo_path": `C:\work\other-repo`}}, wantField: "source.repo_path", + wantSuggested: "arguments.repo", + }, + { + name: "cold pr context repository", facade: "pr", operation: "list", + input: map[string]any{"context": map[string]any{"repository": "other-repo"}}, wantField: "context.repository", + }, + { + name: "external write source repo path", facade: "publish_review", operation: "post", + input: map[string]any{"source": map[string]any{"repo_path": `C:\work\other-repo`}}, wantField: "source.repo_path", + wantSuggested: "arguments.repo", + }, + { + name: "read symbols injected baseline", facade: "read", operation: "symbols", + input: map[string]any{"context": map[string]any{"repo_path": `C:\work\other-repo`}}, wantField: "context.repo_path", + }, + } { + t.Run(test.name, func(t *testing.T) { + spec, ok := srv.facades.operation(test.facade, test.operation) + require.True(t, ok, "real registry must contain %s.%s", test.facade, test.operation) + test.input["operation"] = test.operation + + result := srv.validateFacadeInput(spec, test.input) + require.NotNil(t, result) + require.True(t, result.IsError) + var structured StructuredError + require.NoError(t, json.Unmarshal([]byte(toolResultText(result)), &structured)) + require.Equal(t, ErrCodeInvalidArgument, structured.ErrorCode) + require.Equal(t, test.wantField, structured.Data["field"]) + if test.wantSuggested != "" { + require.Equal(t, test.wantSuggested, structured.Data["suggested_field"]) + } + }) + } +} + +func TestFacadeRepositoryValidationPreservesConsumedCompatibilityFields(t *testing.T) { + srv, _ := setupTestServer(t) + + for _, test := range []struct { + name string + facade string + operation string + container string + fields map[string]any + wantLowered []string + }{ + { + name: "change source base ref and repo", facade: "change", operation: "detect", container: "source", + fields: map[string]any{"base_ref": "HEAD", "repo": "tracked-repo"}, wantLowered: []string{"base_ref", "repo"}, + }, + { + name: "change output repo", facade: "change", operation: "detect", container: "output", + fields: map[string]any{"repo": "tracked-repo"}, wantLowered: []string{"repo"}, + }, + { + name: "review source base ref", facade: "review", operation: "run", container: "source", + fields: map[string]any{"base_ref": "HEAD"}, wantLowered: []string{"base_ref"}, + }, + { + name: "change ranges source fields", facade: "change", operation: "ranges", container: "source", + fields: map[string]any{"path": "main.go", "start_line": 1, "end_line": 2}, wantLowered: []string{"path", "start_line", "end_line"}, + }, + { + name: "change contract source fields", facade: "change", operation: "contract", container: "source", + fields: map[string]any{"lens": "api", "risk_gate": "strict"}, wantLowered: []string{"lens", "risk_gate"}, + }, + { + name: "change simulate source keep", facade: "change", operation: "simulate", container: "source", + fields: map[string]any{"keep": true}, wantLowered: []string{"keep"}, + }, + { + name: "review critique prior review", facade: "review", operation: "critique", container: "source", + fields: map[string]any{"prior_review": "review text"}, wantLowered: []string{"prior_review"}, + }, + } { + t.Run(test.name, func(t *testing.T) { + spec, ok := srv.facades.operation(test.facade, test.operation) + require.True(t, ok, "real registry must contain %s.%s", test.facade, test.operation) + input := map[string]any{"operation": test.operation, test.container: test.fields} + + require.Nil(t, srv.validateFacadeInput(spec, input)) + normalized := normalizeFacadeArguments(spec, input) + for _, field := range test.wantLowered { + _, exists := normalized[field] + require.True(t, exists, "%s must survive normalization", field) + require.True(t, srv.legacyDeclaresField(spec.Legacy, field), "%s must be consumed by %s", field, spec.Legacy) + } + }) + } +} + +func TestFacadeRepositoryValidationLeavesNonSelectorCompatibilityAlone(t *testing.T) { + srv, _ := setupTestServer(t) + spec, ok := srv.facades.operation("change", "detect") + require.True(t, ok) + require.Nil(t, srv.validateFacadeInput(spec, map[string]any{ + "operation": "detect", + "source": map[string]any{"base": "HEAD", "mystery": true}, + })) +} + +func TestFacadeRepositoryValidationRejectsInvalidCanonicalSelectors(t *testing.T) { + srv, _ := setupTestServer(t) + + for _, test := range []struct { + name string + facade string + operation string + input map[string]any + wantField string + }{ + { + name: "empty common selector", facade: "change", operation: "detect", + input: map[string]any{"options": map[string]any{"repo": " "}}, wantField: "options.repo", + }, + { + name: "non-string common selector", facade: "change", operation: "detect", + input: map[string]any{"options": map[string]any{"repo": 42}}, wantField: "options.repo", + }, + { + name: "empty cold selector", facade: "pr", operation: "risk", + input: map[string]any{"arguments": map[string]any{"repo": ""}}, wantField: "arguments.repo", + }, + } { + t.Run(test.name, func(t *testing.T) { + spec, ok := srv.facades.operation(test.facade, test.operation) + require.True(t, ok) + test.input["operation"] = test.operation + + result := srv.validateFacadeInput(spec, test.input) + require.NotNil(t, result) + require.True(t, result.IsError) + var structured StructuredError + require.NoError(t, json.Unmarshal([]byte(toolResultText(result)), &structured)) + require.Equal(t, test.wantField, structured.Data["field"]) + require.Equal(t, "non-empty string", structured.Data["expected_type"]) + }) + } +} + +func TestFacadeEditFileRejectsUnconsumedRepositoryBeforeWrite(t *testing.T) { + srv, root := setupTestServer(t) + target := filepath.Join(root, "main.go") + before, err := os.ReadFile(target) + require.NoError(t, err) + + 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", + "options": map[string]any{"repo": t.TempDir()}, + } + result, err := srv.handleFacade(context.Background(), "edit", req) + require.NoError(t, err) + require.True(t, result.IsError) + + after, err := os.ReadFile(target) + require.NoError(t, err) + require.Equal(t, before, after) +} diff --git a/internal/mcp/facade_localization_scope_test.go b/internal/mcp/facade_localization_scope_test.go index ee5c567c..ffcf7d1c 100644 --- a/internal/mcp/facade_localization_scope_test.go +++ b/internal/mcp/facade_localization_scope_test.go @@ -14,7 +14,11 @@ import ( func TestFacadeExplorePathLowersToExplicitRepoSelector(t *testing.T) { registry := newFacadeRegistry() - registry.capture(mcpgo.NewTool("explore", mcpgo.WithString("task", mcpgo.Required())), func(_ context.Context, req mcpgo.CallToolRequest) (*mcpgo.CallToolResult, error) { + registry.capture(mcpgo.NewTool( + "explore", + mcpgo.WithString("task", mcpgo.Required()), + mcpgo.WithString("repo"), + ), func(_ context.Context, req mcpgo.CallToolRequest) (*mcpgo.CallToolResult, error) { return mcpgo.NewToolResultJSON(req.GetArguments()) }) server := &Server{facades: registry, localization: newLocalizationTerminalState()} diff --git a/internal/mcp/facade_tools.go b/internal/mcp/facade_tools.go index 3b8d3e64..6dfc4f33 100644 --- a/internal/mcp/facade_tools.go +++ b/internal/mcp/facade_tools.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "encoding/json" "fmt" + "reflect" "slices" "sort" "strings" @@ -982,10 +983,6 @@ func (s *Server) invokeFacadeSpec(ctx context.Context, req mcpgo.CallToolRequest Data: map[string]any{"facade": spec.Facade, "operation": spec.Operation, "legacy_tool": spec.Legacy}, }), nil } - if invalid := s.validateFacadeInput(spec, req.GetArguments()); invalid != nil { - outcome = facadeOutcomeInvalidArgument - return invalid, nil - } normalized := normalizeFacadeArguments(spec, req.GetArguments()) if targetErr := normalizeFacadeChangeTargets(spec, req.GetArguments(), normalized); targetErr != nil { outcome = facadeOutcomeInvalidArgument @@ -1457,7 +1454,7 @@ func (s *Server) facadeAcceptedTopLevelFields(spec facadeOperationSpec) []string } func (s *Server) validateFacadeInput(spec facadeOperationSpec, input map[string]any) *mcpgo.CallToolResult { - for _, field := range []string{"arguments", "options", "source", "context", "guard", "output"} { + for _, field := range facadeContainerKeys { value, present := input[field] if !present || value == nil { continue @@ -1489,6 +1486,9 @@ func (s *Server) validateFacadeInput(spec facadeOperationSpec, input map[string] }) } } + if invalid := s.validateFacadeRepositoryFields(spec, input); invalid != nil { + return invalid + } for _, field := range []string{"target", "to"} { if raw, present := input[field]; present && raw != nil { if invalid := validateFacadeSelector(field, raw); invalid != nil { @@ -1528,6 +1528,110 @@ 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 { + locations := append([]string{""}, facadeContainerKeys...) + for _, containerName := range locations { + fields := input + if containerName != "" { + var ok bool + fields, ok = input[containerName].(map[string]any) + if !ok { + continue + } + } + for _, field := range sortedFacadeMapKeys(fields) { + if !facadeRepositorySelectorLike(field) { + 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 + } + message := fmt.Sprintf("unknown field %q", path) + if canonicalPath != "" { + data["suggested_field"] = canonicalPath + message += fmt.Sprintf("; use %s to select a repository", canonicalPath) + } + return NewStructuredErrorResult(StructuredError{ + ErrorCode: ErrCodeInvalidArgument, + Message: message, + Data: data, + }) + } + } + return nil +} + +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} + if containerName == "" { + probe[field] = value + } else { + probe[containerName] = map[string]any{field: value} + } + for lowered, candidateValue := range normalizeFacadeArguments(spec, probe) { + if _, fixed := spec.Fixed[lowered]; fixed || !s.legacyDeclaresField(spec.Legacy, lowered) { + continue + } + baselineValue, existed := baseline[lowered] + if !existed || !reflect.DeepEqual(baselineValue, candidateValue) { + return true + } + } + return false +} + +func (s *Server) facadePublicRepositoryField(spec facadeOperationSpec) string { + capability := s.facadeCapability(spec, true) + schema, _ := capability["input_schema"].(map[string]any) + properties, _ := schema["properties"].(map[string]any) + if _, ok := properties["repo"]; ok { + return "repo" + } + for _, container := range facadeContainerKeys { + containerSchema, _ := properties[container].(map[string]any) + containerProperties, _ := containerSchema["properties"].(map[string]any) + if _, ok := containerProperties["repo"]; ok { + return container + ".repo" + } + } + return "" +} + +func facadeRepositorySelectorLike(field string) bool { + switch strings.ToLower(strings.TrimSpace(field)) { + case "repo", "repo_path", "repository", "repository_path": + return true + default: + return false + } +} + func validateFacadeSelector(field string, raw any) *mcpgo.CallToolResult { target, ok := raw.(map[string]any) if !ok {