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

[sVIKs]

The full lesson of the "process landed in production with a misleading Stage-is-immutable error" incident: every way a create can go to the wrong place — or fail without saying why — is closed.

1. Explicit folder_id on create-process / create-state-diagram / create-folder

An integer folder_id always wins; otherwise the target resolves from the local <id>_<name>.folder.json/.stage.json marker as before.

2. Ambiguity guard

A directory containing MORE than one marker is an error naming all candidates — not a silent first pick (markers sorted production before develop, which is exactly how creates went to the immutable stage):

ambiguous target: directory '.' contains 2 folder/stage markers
(681527_production.stage.json, 681528_develop.stage.json) — pass folder_id explicitly …

3. The result reports the resolved target

Process 'x' created in Corezoid folder #685226 (explicit folder_id / resolved from local marker …) — a wrong destination is visible immediately.

4. Strict arguments on every tool

The dispatcher rejects undeclared arguments (unknown argument(s) bogus_arg … (accepted: folder_id, folder_path, process_name)). Unknown keys used to be dropped silently, letting create-process{folder_id: N} fall back to directory resolution and create somewhere else entirely. A registry-wide test asserts every tool declares a schema.

5. Create failures carry the server's reason

CreateEmptyConv returned a bare 0 and the real reason ("Stage is immutable", access denied, …) went only to mcp.log. Now:

Error: failed to create process 'x' in folder #685227 (explicit folder_id): API op error: Stage is immutable

6. CLI boolean coercion (schema-driven)

CLI args arrive as strings while handlers type-assert booleans — deploy-stage apply=true silently ran as a dry-run, set-stage-immutable immutable=true could never work. runCLI now coerces "true"/"false" for schema-boolean args only (string args like confirm stay strings). The env-based folder_id default is also narrowed to tools whose schema REQUIRES it (it used to be injected even into login).

Live verification (sandbox project 685225)

• create with folder_id into dev → lands there, reports "explicit folder_id";
• create into the immutable stage → the real reason in the tool result;
• poisoned dir with two markers → immediate refusal naming both;
• unknown arg → rejected with the accepted-list.

Tests

go build, go test, go test -race green: ambiguity guard, explicit-id-wins, marker reporting, folder_id-to-the-wire (mock server), unknown-arg rejection, every-tool-has-schema, server-reason surfacing, CLI boolean coercion.

🤖 Generated with Claude Code

[gh-corezoid]

AI Review

Fixes a long-running production incident where create-process silently ignored a passed folder_id, resolved the wrong stage from an ambiguous multi-marker directory, and surfaced only as a misleading "Stage is immutable" server error. The PR adds explicit folder_id support to all three create tools, turns multi-marker ambiguity into a named error, and enforces strict argument validation across every MCP tool call.

Checklist

Check	Result
U1 — Conventional commit format	✅ pass
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (main)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	⚠️ warning (see below)
C1 — Manifest files version-synced	⏭️ skip (no manifest files changed)
C2 — Node IDs in .conv.json valid	⏭️ skip
C3 — No hardcoded env values in .conv.json	⏭️ skip
C4 — extra fields have matching extra_type	⏭️ skip
C5 — README updated for new skills	⏭️ skip

U5 detail: go build ./... — clean. go test ./... — all tests green on the PR branch (ok convctl 6.723s). Same suite on base main also fully green (ok convctl 10.015s). No regressions introduced by this PR.

Issues found

warning (U6) — Unknown-arg rejection is a silent breaking change with no CHANGELOG entry.
Before this PR, passing an undeclared argument to any MCP tool was a no-op — the key was silently dropped and the call proceeded. After this PR, the same call returns an error. Any external integration, script, or saved prompt that passes extra keys (e.g. a folder_id to push-process, or any arg to login) will start failing with no prior notice. The PR intentionally defers the CHANGELOG entry until #62 lands, which is a reasonable choice, but the behavior change should be explicitly noted under [Unreleased] before release so operators can audit their workflows.

warning (U6) — Ambiguity guard changes behavior for existing multi-marker directories.
Previously a directory with two marker files silently targeted the first sorted match (the production stage in the incident). Now it errors immediately. This is the correct fix, but users who happened to have multi-marker directories and whose create-* calls were accidentally working against the first marker will now see a failure that previously wasn't there. Worth a one-liner in the release notes so operators can diagnose it quickly if it surfaces.

warning (design) — []string type assertion for required fields is fragile.
In tool_args_validation.go the required-arg detection uses:

if req, ok := schema["required"].([]string); ok {

This assertion succeeds only for Go-literal []string values. Any future toolRegistry entry whose required list is populated from a JSON decode (which produces []interface{}) will silently return false and mark no args as required, disabling the CLI env-default guard for that tool. Today every registry entry uses Go literals, so there is no bug — but the pattern is a trap for future contributors. Consider a helper that accepts []interface{} with an inner cast, or a comment explaining the constraint.

---

This review was generated automatically. A human maintainer should still make the merge decision.

[gh-corezoid]

AI Review

Re-review after synchronize: the third commit surfaces the server's actual error reason from CreateEmptyConv (e.g. "Stage is immutable") all the way into the tool result, and adds schema-driven CLI boolean coercion so deploy-stage apply=true and set-stage-immutable immutable=true finally work from the CLI. The first two commits are unchanged from the initial review.

Checklist

Check	Result
U1 — Conventional commit format	⚠️ warning (see below)
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (main)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	⚠️ warning (see below)
C1 — Manifest files version-synced	⏭️ skip (no manifest files changed)
C2 — Node IDs in .conv.json valid	⏭️ skip
C3 — No hardcoded env values in .conv.json	⏭️ skip
C4 — extra fields have matching extra_type	⏭️ skip
C5 — README updated for new skills	⏭️ skip

U5 detail: go build ./... — clean. go test ./... — all tests green on the PR branch (ok convctl 6.297s). Same suite on base main fully green (ok convctl 11.190s). No regressions.

Issues found

warning (U1, new — introduced by commit 3) — The third commit's subject line is 72 characters:

fix(mcp): create tools surface the server's reason; CLI coerces booleans

The convention cap is 70. Two characters over — not blocking, easy to address if you rebase before merge.

warning (U6, unresolved from initial review) — Unknown-arg rejection is a silent breaking change with no CHANGELOG entry. Any external integration, script, or saved prompt that passes extra keys to a tool will start failing after this PR merges. The new commit does not add a [Unreleased] entry. Worth a one-liner before release so operators can audit.

warning (U6, unresolved from initial review) — Ambiguity guard changes behavior for existing multi-marker directories. Users whose create-* calls previously silently targeted the first sorted marker (production) will now see an error. Deserves a release-note callout for the same reason.

warning (design, unresolved from initial review) — []string type assertion for required fields in tool_args_validation.go:

if req, ok := schema["required"].([]string); ok {

This succeeds only for Go-literal []string. Any future registry entry whose required list comes from a JSON decode ([]interface{}) would silently mark no args as required, disabling the env-default guard for that tool. No current bug — but a trap for future contributors. A helper or a code comment explaining the constraint would prevent confusion.

---

This review was generated automatically. A human maintainer should still make the merge decision.

[gh-corezoid]

AI Review

Re-review after synchronize: commit 4 (fix(mcp): harden strict-args and CLI coercion per adversarial review) adds robustness to the CLI boolean coercion (accepting True/1/yes, refusing unrecognisable values with an error), routes unknown-argument rejections through the normal analytics path, and replaces the tautological TestUnknownArgsError_EveryToolHasSchema test with one that genuinely verifies the built allowed set against each schema's declared properties.

Checklist

Check	Result
U1 — Conventional commit format	⚠️ warning (see below)
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (main)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	⚠️ warning (see below)
C1 — Manifest files version-synced	⏭️ skip (no manifest files changed)
C2 — Node IDs in .conv.json valid	⏭️ skip
C3 — No hardcoded env values in .conv.json	⏭️ skip
C4 — extra fields have matching extra_type	⏭️ skip
C5 — README updated for new skills	⏭️ skip

U5 detail: go build ./... — clean on PR branch. go test ./... — ok convctl 6.630s on PR branch; ok convctl 9.698s on base main. No regressions introduced by this PR.

Fixed since last review ✅

TestUnknownArgsError_EveryToolHasSchema is now a real safety net. The previous version checked only that toolAllowedArgs[tool.Name] existed in the map — always true since buildToolAllowedArgs inserts a key for every tool, so a malformed InputSchema (e.g. properties missing or wrong type) would pass the test with an empty allowed set, causing every argument of that tool to be rejected at runtime. The new version compares the built set against the schema's declared properties key-by-key and asserts equal lengths. A malformed schema is now caught at test time rather than silently failing at runtime. This resolves the most dangerous half of the previous design concern.

Issues found

warning (U1, unresolved from previous review) — Commit 3's subject line is still 72 characters:

fix(mcp): create tools surface the server's reason; CLI coerces booleans

The convention cap is 70. Two characters over — not blocking.

warning (U6, unresolved from previous review) — No [Unreleased] CHANGELOG entry for the unknown-argument rejection behavior change. Any external integration or saved prompt that passed extra keys to a tool will start failing silently after this PR merges. A one-liner under [Unreleased] before release gives operators the signal they need to audit their workflows.

warning (U6, unresolved from previous review) — No release-note callout for the ambiguity guard. Users whose create-* calls previously worked against the first sorted marker (and happened to be correct, or happened to be wrong and didn't notice) will now see an error with no prior notice. Worth a sentence in the release notes so they can diagnose it quickly.

warning (design, partially mitigated) — The schema["required"].([]string) type assertion in tool_args_validation.go:51 is still present:

if req, ok := schema["required"].([]string); ok {

The improved test now covers properties/allowed (the critical path for unknown-arg rejection), but not the required field parsing (used only by toolRequiresArg to scope the CLI folder_id env-default). If a future registry entry populates required from a JSON decode — which yields []interface{} — the assertion silently returns false and CLI env-default injection stops working for that tool. No current bug; no current test catches it. A helper that accepts both []string and []interface{} (or a code comment noting the constraint) would close the trap for future contributors.

---

This review was generated automatically. A human maintainer should still make the merge decision.

[sVIKs]

Review findings addressed:

• U1 72-char subject — commit reworded to fix(mcp): surface create errors; coerce CLI booleans (52 chars); tree unchanged.
• Missing [Unreleased] notes — CHANGELOG now documents both behaviour changes operators must know about: undeclared tool arguments are rejected (previously silently dropped), and ambiguous multi-marker directories refuse creates instead of silently picking the first marker (which could be production). The explicit folder_id feature is noted alongside.
• \[\]string assertion trap — extracted schemaRequiredList() accepting both a Go-literal \[\]string and a decoded-JSON \[\]interface{}, with a test covering both shapes plus nil.

Heads-up: the [Unreleased] section may need a trivial merge with #62's once either lands — happy to rebase whichever goes second.

[gh-corezoid]

AI Review

Re-review after synchronize: commit 5 (docs(changelog): note breaking arg/marker checks; parse required) adds a documented [Unreleased] CHANGELOG section covering both breaking behaviour changes, and replaces the bare []string type assertion for schema required lists with a schemaRequiredList helper that accepts both []string (Go literal) and []interface{} (JSON-decoded) shapes — with a test. Commit 3's subject line was also shortened from 72 to 52 characters. All four findings from the previous review are now resolved.

Checklist

Check	Result
U1 — Conventional commit format	✅ pass
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (main)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	✅ pass
C1 — Manifest files version-synced	⏭️ skip (no manifest files changed)
C2 — Node IDs in .conv.json valid	⏭️ skip
C3 — No hardcoded env values in .conv.json	⏭️ skip
C4 — extra fields have matching extra_type	⏭️ skip
C5 — README updated for new skills	⏭️ skip

U1 detail: All five commit subjects are within 70 characters (64 / 68 / 52 / 64 / 66). Valid types (fix, docs), all scoped, lowercase after colon, no trailing period, no WIP/bare commits.

U5 detail: go build ./... — clean on PR branch. go test ./... — ok convctl 5.809s on PR branch; ok convctl 8.707s on base main. No regressions introduced by this PR.

Fixed since last review ✅

U1 — Commit 3 subject trimmed. Previously flagged as 72 characters (fix(mcp): create tools surface the server's reason; CLI coerces booleans). Now 52 characters: fix(mcp): surface create errors; coerce CLI booleans. Within the 70-char cap.

U6/CHANGELOG — Breaking changes documented. CHANGELOG.md now carries a [Unreleased] section calling out both operator-visible behaviour changes: unknown tool arguments are now rejected with an error, and multi-marker directories now refuse creates instead of silently targeting the first sorted match. Both entries include the action required of integrators.

Design — schemaRequiredList closes the []interface{} trap. buildToolAllowedArgs now calls schemaRequiredList(schema["required"]) instead of a bare .([]string) assertion. The helper handles []string (Go-literal registry entries today) and []interface{} (JSON-decoded entries in future), and TestSchemaRequiredList_BothShapes verifies both shapes. Future contributors adding a registry entry loaded from a file will no longer silently lose the CLI env-default guard for that tool.

Issues found

No issues found.

---

This review was generated automatically. A human maintainer should still make the merge decision.

[gh-corezoid]

AI Review

Re-review after synchronize: the branch was rebased onto the updated main (PR #62 — feat/node-auto-layout — now merged). The five commits are content-identical to the previous review round; no new logic was added. A key side-effect of the rebase: the layout-process tool introduced by PR #62 is now also subject to the strict-arg validation added by this PR — the TestUnknownArgsError_EveryToolHasSchema safety net confirmed its schema is properly declared and all its arguments pass validation.

Checklist

Check	Result
U1 — Conventional commit format	✅ pass
U2 — No leaked credentials	✅ pass
U3 — No merge commits	✅ pass
U4 — PR targets correct base branch (main)	✅ pass
U5 — Build & tests (Go)	✅ pass
U6 — Architectural & design consequences	✅ pass
C1 — Manifest files version-synced	⏭️ skip (no manifest files changed)
C2 — Node IDs in .conv.json valid	⏭️ skip
C3 — No hardcoded env values in .conv.json	⏭️ skip
C4 — extra fields have matching extra_type	⏭️ skip
C5 — README updated for new skills	⏭️ skip

U1 detail: All five commit subjects are within the 70-character cap (64 / 63 / 52 / 69 / 64 chars). Valid types (fix, docs), all scoped, lowercase after colon, no trailing period, no WIP or bare commits. None of the commits touch a version manifest, so no "bump to" suffix is required.

U5 detail: go build ./... — clean on PR branch. go test ./... — first full run produced one transient failure in TestMCPProtocol_AuthRequired_NoCredentials (open ./tool_args_validation.go: no such file or directory from within the test's internal go build . subprocess); the test passes in isolation and on every subsequent run (ok convctl 46.713s on re-run with -count=1; ok convctl 233.705s on base main). Assessed as a cold-cache environment flake, not a regression introduced by this PR — the file exists in the package directory and the test consistently passes otherwise.

U6 / schema safety-net: TestUnknownArgsError_EveryToolHasSchema passed against the rebased branch, which now includes the layout-process tool from PR #62. All registry entries — old and new — have parseable properties maps; the strict-arg rejection will work correctly for every tool currently in the registry.

Fixed since last review ✅

All four findings from the previous review were resolved in commit 5 (reviewed 2026-07-11). This re-review found no regressions and no new issues introduced by the rebase.

Issues found

No issues found.

---

This review was generated automatically. A human maintainer should still make the merge decision.