diff --git a/CHANGELOG.md b/CHANGELOG.md index e147e33..554133c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # Changelog +## [Unreleased] + +- **Breaking / behaviour**: tool calls now REJECT undeclared arguments with an + error naming the unknown keys and the accepted list (previously unknown keys + were silently dropped — a call could quietly act on the wrong object). + Integrations passing stray keys must remove them. +- **Breaking / behaviour**: `create-process` / `create-state-diagram` / + `create-folder` refuse a working directory that contains MORE than one + `_.folder/stage.json` marker instead of silently picking the first + one (which could target a production stage). Pass the new explicit + `folder_id` argument or run from the specific folder's directory. +- Feature: create tools accept an explicit `folder_id` and report the resolved + target ("created in Corezoid folder #N (explicit folder_id / resolved from + marker X)") in the result. + ## [2.9.0] - Feat: `push-process` now runs `lint-process` before deploying and blocks on issues that would break the deploy or its callers (broken node links, old-format nodes, RPC paths without reply, nodes missing a default `go`, sub-30s timers, literal reply values); advisory findings are shown but do not block, and `force=true` overrides. Advisory findings from `lint-process` are also surfaced back on a successful push instead of silently swallowed. diff --git a/plugins/corezoid/mcp-server/create_target_dx_test.go b/plugins/corezoid/mcp-server/create_target_dx_test.go new file mode 100644 index 0000000..93dabb0 --- /dev/null +++ b/plugins/corezoid/mcp-server/create_target_dx_test.go @@ -0,0 +1,127 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// ---- ambiguous local markers -------------------------------------------------- + +func TestResolveFolderIDFromDir_AmbiguousMarkersRejected(t *testing.T) { + dir := t.TempDir() + for _, name := range []string{"681527_production.stage.json", "681528_develop.stage.json"} { + if err := os.WriteFile(filepath.Join(dir, name), []byte("{}"), 0644); err != nil { + t.Fatal(err) + } + } + _, _, err := resolveFolderIDFromDir(dir) + if err == nil { + t.Fatal("two markers in one directory must be an error, not a silent first pick") + } + if !strings.Contains(err.Error(), "ambiguous") || !strings.Contains(err.Error(), "folder_id") { + t.Errorf("error must say the target is ambiguous and suggest folder_id, got: %v", err) + } +} + +// ---- explicit folder_id ------------------------------------------------------- + +func TestResolveCreateTarget_ExplicitFolderIDWins(t *testing.T) { + dir := t.TempDir() // no marker files at all — explicit id must not need them + id, how, err := resolveCreateTarget(map[string]interface{}{"folder_id": float64(685228)}, dir) + if err != nil || id != 685228 { + t.Fatalf("got (%d, %v), want (685228, nil)", id, err) + } + if !strings.Contains(how, "explicit") { + t.Errorf("resolution description must say the id was explicit, got: %q", how) + } +} + +func TestResolveCreateTarget_ReportsMarker(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "42_dev.folder.json"), []byte("{}"), 0644); err != nil { + t.Fatal(err) + } + id, how, err := resolveCreateTarget(map[string]interface{}{}, dir) + if err != nil || id != 42 { + t.Fatalf("got (%d, %v), want (42, nil)", id, err) + } + if !strings.Contains(how, "42_dev.folder.json") { + t.Errorf("resolution description must name the marker file, got: %q", how) + } +} + +// ---- folder_id reaches the wire ------------------------------------------------ + +func TestCreateEmptyConv_SendsGivenFolderID(t *testing.T) { + var gotFolderID interface{} + _, e := mockAPIServer(t, func(ops []map[string]interface{}) interface{} { + gotFolderID = ops[0]["folder_id"] + return map[string]interface{}{ + "request_proc": "ok", + "ops": []interface{}{map[string]interface{}{"proc": "ok", "obj_id": float64(777)}}, + } + }) + if id, err := e.CreateEmptyConv(685228, "t", "", "process"); id != 777 || err != nil { + t.Fatalf("CreateEmptyConv returned (%d, %v), want (777, nil)", id, err) + } + if got, ok := gotFolderID.(float64); !ok || int(got) != 685228 { + t.Errorf("folder_id on the wire = %v, want 685228", gotFolderID) + } +} + +// ---- the server's reason reaches the tool result ------------------------------- + +// The server explains WHY a create failed ("Stage is immutable", access +// denied, ...). Burying that in mcp.log while the tool said only "failed to +// create" cost real field-debugging time — the reason must be in the result. +func TestCreateEmptyConv_SurfacesServerReason(t *testing.T) { + _, e := mockAPIServer(t, func(ops []map[string]interface{}) interface{} { + return map[string]interface{}{"request_proc": "ok", "ops": []interface{}{ + map[string]interface{}{"proc": "error", "description": "Stage is immutable"}}} + }) + id, err := e.CreateEmptyConv(685227, "t", "", "process") + if id != 0 || err == nil { + t.Fatalf("expected (0, err), got (%d, %v)", id, err) + } + if !strings.Contains(err.Error(), "Stage is immutable") { + t.Errorf("error must carry the server's reason, got: %v", err) + } +} + +// ---- CLI boolean coercion ------------------------------------------------------- + +// CLI args arrive as strings, but handlers type-assert booleans — before the +// coercion, `deploy-stage apply=true` silently ran as a dry-run. +func TestCoerceCLIArgs_Booleans(t *testing.T) { + args := map[string]interface{}{ + "apply": "true", // boolean in deploy-stage's schema + "confirm": "true", // string in the schema — must stay a string + "company_id": "c1", // untouched + } + if err := coerceCLIArgs("deploy-stage", args); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if args["apply"] != true { + t.Errorf(`apply = %v (%T), want true (bool)`, args["apply"], args["apply"]) + } + if args["confirm"] != "true" { + t.Errorf(`confirm = %v, must stay the string "true"`, args["confirm"]) + } + if args["company_id"] != "c1" { + t.Errorf("company_id changed: %v", args["company_id"]) + } +} + +// Unparseable boolean strings must fail loudly — a boolean the handler cannot +// read is exactly how `apply=True` degraded to a silent dry-run. +func TestCoerceCLIArgs_CaseAndErrors(t *testing.T) { + args := map[string]interface{}{"apply": "True"} + if err := coerceCLIArgs("deploy-stage", args); err != nil || args["apply"] != true { + t.Fatalf("mixed-case True must coerce, got err=%v apply=%v", err, args["apply"]) + } + if err := coerceCLIArgs("deploy-stage", map[string]interface{}{"apply": "yep"}); err == nil { + t.Fatal("unparseable boolean must be an error, not a silent string") + } +} diff --git a/plugins/corezoid/mcp-server/executor_mock_test.go b/plugins/corezoid/mcp-server/executor_mock_test.go index 781ed30..1c77e6a 100644 --- a/plugins/corezoid/mcp-server/executor_mock_test.go +++ b/plugins/corezoid/mcp-server/executor_mock_test.go @@ -208,7 +208,7 @@ func TestCreateEmptyProcess_Error(t *testing.T) { return map[string]interface{}{"request_proc": "fail"} }) - id := e.CreateEmptyProcess(1, "test", "") + id, _ := e.CreateEmptyProcess(1, "test", "") if id != 0 { t.Errorf("expected 0 on error, got %d", id) } diff --git a/plugins/corezoid/mcp-server/executor_process.go b/plugins/corezoid/mcp-server/executor_process.go index d87abdb..2714cc4 100644 --- a/plugins/corezoid/mcp-server/executor_process.go +++ b/plugins/corezoid/mcp-server/executor_process.go @@ -257,10 +257,12 @@ func (validator *Executor) ProcessJSON(filePath, jsonContent string) (newProcess if validator.ProcessID == 0 { validator.NewProc = true title, _ := processDataOfAI["title"].(string) - validator.ProcessID = validator.CreateEmptyProcess(0, title, "") + validator.ProcessID, err = validator.CreateEmptyProcess(0, title, "") if validator.ProcessID == 0 { - err = fmt.Errorf("failed to create process") - return nil, err + if err == nil { + err = fmt.Errorf("failed to create process") + } + return nil, fmt.Errorf("failed to create process: %v", err) } } else { oldProcessData, err := validator.GetProcessByID(validator.ProcessID) @@ -485,7 +487,7 @@ func (v *Executor) GetProcessByID(id int) (rsp map[string]any, err error) { return nil, fmt.Errorf("failed to get process: no response from server") } -func (v *Executor) CreateEmptyProcess(folderID int, title, desc string) int { +func (v *Executor) CreateEmptyProcess(folderID int, title, desc string) (int, error) { return v.CreateEmptyConv(folderID, title, desc, "process") } @@ -493,7 +495,7 @@ func (v *Executor) CreateEmptyProcess(folderID int, title, desc string) int { // ("process" for a regular process, "state" for a state diagram). // CreateEmptyProcess is preserved as a backward-compatible wrapper that // defaults to conv_type "process". -func (v *Executor) CreateEmptyConv(folderID int, title, desc, convType string) int { +func (v *Executor) CreateEmptyConv(folderID int, title, desc, convType string) (int, error) { if title == "" { title = time.Now().String() } @@ -519,8 +521,12 @@ func (v *Executor) CreateEmptyConv(folderID int, title, desc, convType string) i } response, err := v.req("create_process", ops) if err != nil { + // The op error carries the server's actual reason ("Stage is immutable", + // "Folder not found", access denied, ...). Return it — burying it in the + // log while the tool reports a bare "failed to create" cost real + // debugging time in the field. logger.Error("Failed to create empty process: %v", err) - return 0 + return 0, err } if response != nil && response["ops"] != nil { if opsArray, ok := response["ops"].([]interface{}); ok && len(opsArray) > 0 { @@ -528,13 +534,13 @@ func (v *Executor) CreateEmptyConv(folderID int, title, desc, convType string) i if objID, ok := firstOp["obj_id"].(float64); ok { v.ProcessID = int(objID) logger.Debug("Empty process created: %d", v.ProcessID) - return v.ProcessID + return v.ProcessID, nil } } } } logger.Error("Failed to create empty process") - return 0 + return 0, fmt.Errorf("create returned no obj_id") } func (v *Executor) SetParams(params []interface{}) error { diff --git a/plugins/corezoid/mcp-server/main.go b/plugins/corezoid/mcp-server/main.go index 517f651..ba1d929 100644 --- a/plugins/corezoid/mcp-server/main.go +++ b/plugins/corezoid/mcp-server/main.go @@ -205,8 +205,16 @@ func runCLI(toolName string, rawArgs []string) { k, v, _ := strings.Cut(a, "=") args[k] = v } - // Apply env-based defaults so the tool works with zero arguments. - if _, ok := args["folder_id"]; !ok && stageID != 0 { + if cerr := coerceCLIArgs(toolName, args); cerr != nil { + fmt.Println("Error:", cerr) + os.Exit(1) + } + // Apply env-based defaults so folder tools work with zero arguments — + // but only where the schema REQUIRES folder_id (pull-folder & friends). + // Injecting it blindly into every call passed a junk argument to tools + // like login, and would silently redirect create-process away from its + // documented directory-based target resolution. + if _, ok := args["folder_id"]; !ok && stageID != 0 && toolRequiresArg(toolName, "folder_id") { args["folder_id"] = stageID } // CLI mode runs to completion or until the user kills the process; we diff --git a/plugins/corezoid/mcp-server/mcp_handlers.go b/plugins/corezoid/mcp-server/mcp_handlers.go index 8be9499..95f820d 100644 --- a/plugins/corezoid/mcp-server/mcp_handlers.go +++ b/plugins/corezoid/mcp-server/mcp_handlers.go @@ -154,7 +154,17 @@ func handleToolCall(ctx context.Context, name string, args map[string]interface{ } start := time.Now() - result, isError = h(ctx, args) + // Reject arguments the tool does not declare. Unknown keys used to be + // silently dropped, which let a call like create-process{folder_id: N} + // quietly fall back to directory-based target resolution and create the + // process somewhere else entirely. The rejection flows through the same + // result/analytics path as any other tool error so it is visible in + // telemetry. + if msg := unknownArgsError(name, args); msg != "" { + result, isError = msg, true + } else { + result, isError = h(ctx, args) + } if analyticsEnabled.Load() { apiURLv, _, _, _, _ := authSnapshot() diff --git a/plugins/corezoid/mcp-server/mcp_handlers_process.go b/plugins/corezoid/mcp-server/mcp_handlers_process.go index 6dec4db..9beb112 100644 --- a/plugins/corezoid/mcp-server/mcp_handlers_process.go +++ b/plugins/corezoid/mcp-server/mcp_handlers_process.go @@ -455,15 +455,21 @@ func createConv(ctx context.Context, args map[string]interface{}, convType strin return "Error: " + err.Error(), true } - folderID, err := resolveFolderIDFromDir(folderPath) + // An explicit folder_id always wins; otherwise the target is resolved + // from the local directory's _.folder.json marker. Either way + // the resolved target is reported back so a wrong destination is visible + // immediately instead of surfacing as a confusing server error. + folderID, resolvedFrom, err := resolveCreateTarget(args, folderPath) if err != nil { return fmt.Sprintf("Error resolving folder ID: %v", err), true } v := NewValidator(ctx, 0) - processID := v.CreateEmptyConv(folderID, processName, "", convType) + processID, cerr := v.CreateEmptyConv(folderID, processName, "", convType) if processID == 0 { - return fmt.Sprintf("Error: failed to create %s '%s'", convType, processName), true + // Pass the server's reason through: "Stage is immutable" in the tool + // result is actionable, "failed to create" alone is not. + return fmt.Sprintf("Error: failed to create %s '%s' in folder #%d (%s): %v", convType, processName, folderID, resolvedFrom, cerr), true } procInfo1, err := v.ExportProcess() @@ -492,7 +498,27 @@ func createConv(ctx context.Context, args map[string]interface{}, convType strin if convType == "state" { label = "State diagram" } - return fmt.Sprintf("%s '%s' created and saved to %s", label, processName, filePath), false + return fmt.Sprintf("%s '%s' created in Corezoid folder #%d (%s) and saved to %s", + label, processName, folderID, resolvedFrom, filePath), false +} + +// resolveCreateTarget picks the Corezoid folder a create lands in: an explicit +// integer folder_id argument if given, else the local directory's marker file. +// The second return value describes HOW the target was chosen, for the tool's +// result message. +func resolveCreateTarget(args map[string]interface{}, dir string) (int, string, error) { + if raw, ok := args["folder_id"]; ok { + id, err := intArg(args, "folder_id") + if err != nil { + return 0, "", fmt.Errorf("invalid folder_id %v: %v", raw, err) + } + return id, "explicit folder_id", nil + } + id, marker, err := resolveFolderIDFromDir(dir) + if err != nil { + return 0, "", err + } + return id, fmt.Sprintf("resolved from local marker %s in '%s'", marker, dir), nil } // handleCreateFolder creates a new folder under the given parent, mirrors it @@ -505,7 +531,7 @@ func handleCreateFolder(ctx context.Context, args map[string]interface{}) (strin return "Error: " + err.Error(), true } - parentFolderID, err := resolveFolderIDFromDir(parentPath) + parentFolderID, parentResolvedFrom, err := resolveCreateTarget(args, parentPath) if err != nil { return fmt.Sprintf("Error resolving parent folder ID: %v", err), true } @@ -547,9 +573,11 @@ func handleCreateFolder(ctx context.Context, args map[string]interface{}) (strin return fmt.Sprintf("Error writing folder file: %v", err), true } - return fmt.Sprintf("Folder '%s' created and saved to %s", folderName, filePath), false + return fmt.Sprintf("Folder '%s' created in Corezoid folder #%d (%s) and saved to %s", + folderName, parentFolderID, parentResolvedFrom, filePath), false } + // handleShowFolder returns metadata for a single folder (title, obj_type, // parent). Used to introspect folders without writing anything to disk. func handleShowFolder(ctx context.Context, args map[string]interface{}) (string, bool) { diff --git a/plugins/corezoid/mcp-server/mcp_utils.go b/plugins/corezoid/mcp-server/mcp_utils.go index d77e13a..3f5d360 100644 --- a/plugins/corezoid/mcp-server/mcp_utils.go +++ b/plugins/corezoid/mcp-server/mcp_utils.go @@ -78,13 +78,24 @@ func relativeToCwd(p string) (string, error) { } // resolveFolderIDFromDir looks for a file matching _.folder.json or -// _.stage.json in the given directory and returns the numeric id. -func resolveFolderIDFromDir(dir string) (int, error) { +// _.stage.json in the given directory and returns the numeric id +// together with the marker file name it was resolved from. +// +// When the directory contains MORE than one marker, the target is ambiguous +// and we refuse to guess: silently picking the first match once sent creates +// into a production stage (the marker files sorted production before develop) +// where the only symptom was a misleading "Stage is immutable" error. +func resolveFolderIDFromDir(dir string) (int, string, error) { entries, err := os.ReadDir(dir) if err != nil { - return 0, fmt.Errorf("failed to read directory '%s': %v", dir, err) + return 0, "", fmt.Errorf("failed to read directory '%s': %v", dir, err) } reFolderFile := regexp.MustCompile(`^(\d+)_.*\.(folder|stage)\.json$`) + type match struct { + id int + name string + } + var found []match for _, e := range entries { if e.IsDir() { continue @@ -97,9 +108,20 @@ func resolveFolderIDFromDir(dir string) (int, error) { if err != nil { continue } - return id, nil + found = append(found, match{id: id, name: e.Name()}) + } + switch len(found) { + case 0: + return 0, "", fmt.Errorf("no _.folder.json file found in '%s'; cannot determine folder ID — pass folder_id explicitly", dir) + case 1: + return found[0].id, found[0].name, nil + default: + names := make([]string, len(found)) + for i, f := range found { + names[i] = f.name + } + return 0, "", fmt.Errorf("ambiguous target: directory '%s' contains %d folder/stage markers (%s) — pass folder_id explicitly or run from the specific folder's directory", dir, len(found), strings.Join(names, ", ")) } - return 0, fmt.Errorf("no _.folder.json file found in '%s'; cannot determine folder ID", dir) } // intArg extracts an integer argument from args map. diff --git a/plugins/corezoid/mcp-server/mcp_utils_test.go b/plugins/corezoid/mcp-server/mcp_utils_test.go index b8f81f4..a0fdb41 100644 --- a/plugins/corezoid/mcp-server/mcp_utils_test.go +++ b/plugins/corezoid/mcp-server/mcp_utils_test.go @@ -143,10 +143,13 @@ func TestResolveFolderIDFromDir_Found(t *testing.T) { if err := os.WriteFile(filepath.Join(dir, "12345_my-stage.stage.json"), []byte("{}"), 0644); err != nil { t.Fatal(err) } - id, err := resolveFolderIDFromDir(dir) + id, marker, err := resolveFolderIDFromDir(dir) if err != nil || id != 12345 { t.Errorf("got (%d, %v), want (12345, nil)", id, err) } + if marker != "12345_my-stage.stage.json" { + t.Errorf("marker = %q, want the matched file name", marker) + } } func TestResolveFolderIDFromDir_FolderJSON(t *testing.T) { @@ -154,22 +157,25 @@ func TestResolveFolderIDFromDir_FolderJSON(t *testing.T) { if err := os.WriteFile(filepath.Join(dir, "999_my-folder.folder.json"), []byte("{}"), 0644); err != nil { t.Fatal(err) } - id, err := resolveFolderIDFromDir(dir) + id, marker, err := resolveFolderIDFromDir(dir) if err != nil || id != 999 { t.Errorf("got (%d, %v), want (999, nil)", id, err) } + if marker != "999_my-folder.folder.json" { + t.Errorf("marker = %q, want the matched file name", marker) + } } func TestResolveFolderIDFromDir_NotFound(t *testing.T) { dir := t.TempDir() - _, err := resolveFolderIDFromDir(dir) + _, _, err := resolveFolderIDFromDir(dir) if err == nil { t.Error("expected error when no folder/stage json present, got nil") } } func TestResolveFolderIDFromDir_BadDir(t *testing.T) { - _, err := resolveFolderIDFromDir("/nonexistent_dir_xyz_abc") + _, _, err := resolveFolderIDFromDir("/nonexistent_dir_xyz_abc") if err == nil { t.Error("expected error for non-existent directory, got nil") } diff --git a/plugins/corezoid/mcp-server/tool_args_validation.go b/plugins/corezoid/mcp-server/tool_args_validation.go new file mode 100644 index 0000000..7ba3360 --- /dev/null +++ b/plugins/corezoid/mcp-server/tool_args_validation.go @@ -0,0 +1,140 @@ +package main + +import ( + "fmt" + "sort" + "strings" + "sync" +) + +// toolAllowedArgs maps tool name → the set of argument names its InputSchema +// declares. Built once from toolRegistry — the single source of truth for +// tool definitions. +var ( + toolAllowedArgsOnce sync.Once + toolAllowedArgs map[string]map[string]bool + toolRequiredArgs map[string]map[string]bool + toolArgTypes map[string]map[string]string +) + +func buildToolAllowedArgs() { + toolAllowedArgs = make(map[string]map[string]bool, len(toolRegistry)) + toolRequiredArgs = make(map[string]map[string]bool, len(toolRegistry)) + toolArgTypes = make(map[string]map[string]string, len(toolRegistry)) + for _, t := range toolRegistry { + allowed := make(map[string]bool) + required := make(map[string]bool) + argTypes := make(map[string]string) + if schema, ok := t.InputSchema.(map[string]interface{}); ok { + if props, ok := schema["properties"].(map[string]interface{}); ok { + for k, p := range props { + allowed[k] = true + if pm, ok := p.(map[string]interface{}); ok { + if typ, ok := pm["type"].(string); ok { + argTypes[k] = typ + } + } + } + } + for _, k := range schemaRequiredList(schema["required"]) { + required[k] = true + } + } + toolAllowedArgs[t.Name] = allowed + toolRequiredArgs[t.Name] = required + toolArgTypes[t.Name] = argTypes + } +} + +// coerceCLIArgs converts CLI-supplied string values to the type the tool's +// InputSchema declares. CLI args always arrive as strings ("apply=true"), but +// handlers type-assert booleans (`args["apply"].(bool)`) — so before this, +// boolean flags passed on the CLI were silently ignored: deploy-stage +// apply=true ran as a dry-run. Integers are left alone (intArg already parses +// strings); only booleans need the conversion. +func coerceCLIArgs(tool string, args map[string]interface{}) error { + toolAllowedArgsOnce.Do(buildToolAllowedArgs) + for k, v := range args { + s, ok := v.(string) + if !ok { + continue + } + if toolArgTypes[tool][k] == "boolean" { + switch strings.ToLower(s) { + case "true", "1", "yes": + args[k] = true + case "false", "0", "no": + args[k] = false + default: + // A boolean flag the handler cannot read is exactly how + // `apply=True` used to degrade to a silent dry-run — refuse + // loudly instead of guessing. + return fmt.Errorf("argument %s of %s is boolean; got %q (use true/false)", k, tool, s) + } + } + } + return nil +} + +// schemaRequiredList extracts a schema's "required" list whether it is a +// Go-literal []string (the registry today) or a decoded-JSON []interface{} +// (a future registry entry loaded from a file) — a bare []string assertion +// would silently treat the latter as "nothing required" and disable the CLI +// env-default for that tool. +func schemaRequiredList(v interface{}) []string { + switch req := v.(type) { + case []string: + return req + case []interface{}: + out := make([]string, 0, len(req)) + for _, item := range req { + if s, ok := item.(string); ok { + out = append(out, s) + } + } + return out + } + return nil +} + +// toolRequiresArg reports whether the tool's InputSchema marks the argument +// as required. Used by the CLI to decide when an env-based default is safe +// to inject. +func toolRequiresArg(tool, arg string) bool { + toolAllowedArgsOnce.Do(buildToolAllowedArgs) + return toolRequiredArgs[tool][arg] +} + +// unknownArgsError returns a non-empty error message when args contains keys +// the tool's InputSchema does not declare. Unknown arguments used to be +// silently ignored; that let calls fall back to defaults and act on the wrong +// object with no warning (e.g. create-process{folder_id: N} creating the +// process in a directory-resolved folder instead of folder N). +func unknownArgsError(tool string, args map[string]interface{}) string { + toolAllowedArgsOnce.Do(buildToolAllowedArgs) + allowed, ok := toolAllowedArgs[tool] + if !ok { + return "" // unknown tool is reported by the dispatcher itself + } + var unknown []string + for k := range args { + if !allowed[k] { + unknown = append(unknown, k) + } + } + if len(unknown) == 0 { + return "" + } + sort.Strings(unknown) + accepted := make([]string, 0, len(allowed)) + for k := range allowed { + accepted = append(accepted, k) + } + sort.Strings(accepted) + acceptedDesc := "no arguments" + if len(accepted) > 0 { + acceptedDesc = strings.Join(accepted, ", ") + } + return fmt.Sprintf("Error: unknown argument(s) %s for tool %s (accepted: %s)", + strings.Join(unknown, ", "), tool, acceptedDesc) +} diff --git a/plugins/corezoid/mcp-server/tool_args_validation_test.go b/plugins/corezoid/mcp-server/tool_args_validation_test.go new file mode 100644 index 0000000..d97008e --- /dev/null +++ b/plugins/corezoid/mcp-server/tool_args_validation_test.go @@ -0,0 +1,74 @@ +package main + +import ( + "context" + "strings" + "testing" +) + +// ---- strict argument validation --------------------------------------------- + +func TestHandleToolCall_UnknownArgumentRejected(t *testing.T) { + resetGlobals(t) + withAuthLock(func() { + apiToken = "test-token" + accountURL = "https://account.example" + stageID = 1 + }) + result, isErr := handleToolCall(context.Background(), "create-process", map[string]interface{}{ + "process_name": "x", + "bogus_arg": 1, + }) + if !isErr { + t.Fatal("expected isError=true for an unknown argument") + } + if !strings.Contains(result, "bogus_arg") || !strings.Contains(result, "accepted:") { + t.Errorf("error must name the unknown argument and list accepted ones, got: %s", result) + } +} + +func TestUnknownArgsError_KnownArgsPass(t *testing.T) { + if msg := unknownArgsError("create-process", map[string]interface{}{ + "process_name": "x", "folder_path": ".", "folder_id": 42, + }); msg != "" { + t.Errorf("declared arguments must pass validation, got: %s", msg) + } +} + +func TestUnknownArgsError_EveryToolHasSchema(t *testing.T) { + // Guard: every registry entry must expose a parseable properties map. An + // InputSchema that isn't map[string]interface{} (or lacks "properties") + // yields an EMPTY allowed set — the validator would then reject every + // argument of that tool at runtime. So assert against the schema itself: + // if the schema declares properties, the built set must contain them all. + toolAllowedArgsOnce.Do(buildToolAllowedArgs) + for _, tool := range toolRegistry { + allowed := toolAllowedArgs[tool.Name] + schema, ok := tool.InputSchema.(map[string]interface{}) + if !ok { + t.Errorf("tool %s: InputSchema is %T, not map[string]interface{} — all its args would be rejected", tool.Name, tool.InputSchema) + continue + } + props, _ := schema["properties"].(map[string]interface{}) + for k := range props { + if !allowed[k] { + t.Errorf("tool %s: declared property %q missing from allowed set", tool.Name, k) + } + } + if len(props) != len(allowed) { + t.Errorf("tool %s: %d declared properties but %d allowed args", tool.Name, len(props), len(allowed)) + } + } +} + +func TestSchemaRequiredList_BothShapes(t *testing.T) { + if got := schemaRequiredList([]string{"a", "b"}); len(got) != 2 { + t.Errorf("[]string shape: got %v", got) + } + if got := schemaRequiredList([]interface{}{"a", "b"}); len(got) != 2 { + t.Errorf("[]interface{} shape (decoded JSON): got %v", got) + } + if got := schemaRequiredList(nil); got != nil { + t.Errorf("nil: got %v", got) + } +} diff --git a/plugins/corezoid/mcp-server/tools_registry.go b/plugins/corezoid/mcp-server/tools_registry.go index f460ab1..d976bf0 100644 --- a/plugins/corezoid/mcp-server/tools_registry.go +++ b/plugins/corezoid/mcp-server/tools_registry.go @@ -231,6 +231,10 @@ var toolRegistry = []mcpTool{ InputSchema: map[string]interface{}{ "type": "object", "properties": map[string]interface{}{ + "folder_id": map[string]interface{}{ + "type": "integer", + "description": "Explicit Corezoid folder/stage ID to create in; overrides folder_path resolution", + }, "folder_path": map[string]interface{}{ "type": "string", "description": "Relative path to the folder directory. Omit to use the current directory.", @@ -249,6 +253,10 @@ var toolRegistry = []mcpTool{ InputSchema: map[string]interface{}{ "type": "object", "properties": map[string]interface{}{ + "folder_id": map[string]interface{}{ + "type": "integer", + "description": "Explicit Corezoid folder/stage ID to create in; overrides folder_path resolution", + }, "folder_path": map[string]interface{}{ "type": "string", "description": "Relative path to the folder directory. Omit to use the current directory.", @@ -267,6 +275,10 @@ var toolRegistry = []mcpTool{ InputSchema: map[string]interface{}{ "type": "object", "properties": map[string]interface{}{ + "folder_id": map[string]interface{}{ + "type": "integer", + "description": "Explicit Corezoid folder/stage ID to create the folder in; overrides parent_path resolution", + }, "parent_path": map[string]interface{}{ "type": "string", "description": "Relative path to the parent folder directory. Omit to use the current directory.",