fix(mcp): create-target DX — explicit folder_id, ambiguity guard, strict args

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
+  `<id>_<name>.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.

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")
+	}
+}

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)
 	}

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,15 +487,15 @@ 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")
 }
 
 // CreateEmptyConv creates an empty Corezoid object of the given conv_type
 // ("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,22 +521,26 @@ 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 {
 			if firstOp, ok := opsArray[0].(map[string]interface{}); ok {
 				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 {

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

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()

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 <id>_<name>.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) {

plugins/corezoid/mcp-server/mcp_utils.go

@@ -78,13 +78,24 @@ func relativeToCwd(p string) (string, error) {
 }
 
 // resolveFolderIDFromDir looks for a file matching <id>_<name>.folder.json or
-// <id>_<name>.stage.json in the given directory and returns the numeric id.
-func resolveFolderIDFromDir(dir string) (int, error) {
+// <id>_<name>.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 <id>_<name>.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 <id>_<name>.folder.json file found in '%s'; cannot determine folder ID", dir)
 }
 
 // intArg extracts an integer argument from args map.