fix(mcp): accept a fields object on pad_item create/update; reject undeclared input keys (#1066)

[b4rk13]

What does this PR do?

Implements the contract agreed in #1066: the fields object becomes a real write form, and the silent-drop mechanism behind it is removed for good.

Closes #1066

Half 1 — fields object alias (option 2). pad_item create/update now accept a fields OBJECT — the same shape reads return since the BUG-991 normalization — and fold it into the existing paths at the catalog layer (internal/mcp/catalog_item_fields.go), so both transports get it: keys with a dedicated top-level param (status, priority, category, parent, role, assign, tags) promote onto that param; every other key merges into the field: ["key=value"] path, where server-side schema validation applies unchanged (an unknown custom field still fails with validation_failed rather than defaulting).

Half 2 — strict input validation (option 1). The fan-out handler now rejects any top-level key outside the tool's declared schema, across all ten catalog tools, with a structured validation_failed naming the offending key(s). This is the generalized fix: the original bug's mechanism was "accepted, never mapped by BuildCLIArgs, dropped, PATCH still runs", and that path is now closed for every future undeclared-param variant, not just fields.

Your two constraints, as built:

• Refuse on ambiguity: the same key in fields AND at the top level (or in the field array) with conflicting values returns a structured error and dispatches nothing. Equal duplicates are unambiguous and collapse to one write.
• Single ToolSurfaceVersion bump 0.21 → 0.22 covering both halves, per your follow-up correction after fix: bound item history and stop resolving bodies nobody reads (BUG-2608) #1147 shipped 0.21.

Two decisions worth your eye:

1. Compat carve-out. Strict rejection would have broken the documented v0.16 remote-transport clear form (assigned_user_id: "" / agent_role_id: ""), which is undeprecated and deliberately not schema-declared. Those two keys stay accepted for pad_item via an explicit, commented allowlist (compatAcceptedInputKeys). Happy to drop the carve-out and break that form instead if you'd rather the bump carry it.
2. fields on non-writer actions refuses loudly. Declaring the param at tool level would otherwise let fields flow to, say, action=list and be dropped by BuildCLIArgs — recreating per-action the exact silent no-op this PR removes. Every action except create/update wraps in an explicit refusal pointing at create/update.

Values in fields must be scalars (nested objects/arrays/nulls are refused with the key named); tags is the exception and promotes its native array.

Docs updated in lockstep (the TASK-2005 drift guards enforce the version strings): internal/mcp/instructions.md, README.md, CLAUDE.md, and the authoritative changelog in internal/mcp/version.go.

How to test

1. go test ./internal/mcp/ -run 'PadItem.*Fields|UndeclaredKey|CompatIDParams' -count=1 — the new suite. TestPadItemUpdate_FieldsObjectApplies is the MCP: pad_item create/update silently ignores valid fields.status / fields.priority writes (no error, updated_at still bumps) #1066 repro inverted: it was written first and failed on the pre-change code with "nothing dispatched — fields write was dropped", which reproduces the reported behaviour at the boundary both transports share.
2. Live repro from the issue, after this change: pad_item update ref=TASK-x fields={"status":"done","priority":"critical"} → both values apply; fields={"status":"done"} + top-level status="cancelled" → structured refusal, nothing written; statuss="done" → validation_failed: unknown parameter(s): statuss.
3. go test ./internal/mcp/ -count=1 — full package.

Checklist

• make build passes (go build ./...; web unchanged)
• make test passes — with the caveat that the Windows dev box has a pre-existing red baseline (the same credential/HOME + exec//bin/sh + chmod failures cnYui hit in fix(cli): wrap comment not-found errors with item context #910); captured on clean main before the change and identical after, with internal/mcp contributing no new failures
• New features have tests (TDD — written first, watched fail)
• TypeScript types updated — n/a, no API shape change outside the MCP catalog
• CLI help text updated — n/a, no new CLI command

🤖 Generated with Claude Code

[xarmian]

Thank you for this — it's a model implementation of the #1066 contract. The refuse-on-ambiguity discipline, the deterministic key ordering, rejectFieldsParam preventing the silent no-op from reappearing per-action, and especially the paper-trail comment on the compat carve-out are all exactly the standard we want this codebase to hold. Review ran locally (mcp package green) plus an adversarial tool pass; two bugs to fix, one scope note, one intent question.

Bug 1 (blocking): the strict gate breaks pad_item action=export with an agent-supplied output. rejectUndeclaredKeys runs in the fan-out handler before any action handler, and output is neither schema-declared nor in compatAcceptedInputKeys — but actionItemExport deliberately overrides out["output"] = "-" (catalog_item.go:784) because agents send it. Those calls now fail with "unknown parameter(s): output" before the override can run. The existing TestPadItemExport_OverridesAgentOutput doesn't catch this because it calls actionItemExport directly, bypassing the gate. Fix is your choice — add output to the pad_item compat map with the same style of paper-trail comment, or schema-declare it — but please also add a test that exercises the real fan-out dispatch path so the gate and the handlers are tested together.

Bug 2: empty-string key generates a malformed entry. fields: {"": "v"} passes the =-in-key check and produces field: ["=v"]. Reject empty keys alongside the existing check.

Scope note (document or extend, your call): non-scalar custom-field values (multi_select arrays, JSON fields) are refused by stringifyFieldValue — loudly, which fits the file's disposition, but it means the read shape isn't fully round-trippable for those field types, which was the PR's own motivation. Fine to ship as a named limitation (one line in the file header + the tool description), with array/JSON encoding as a follow-up — just make the limitation discoverable rather than discovered.

Intent question: if a collection declares a custom field literally named one of the seven promoted keys (role, status, …), fields: {role: "x"} promotes to the reserved MCP param instead of writing the custom field — a silent redirect. Reading your merge-contract comment I believe this is an accepted trade-off (the catalog layer can't see collection schemas), but please confirm and, if so, add it to the merge-contract comment so the next reader doesn't re-derive it.

With bug 1 + 2 fixed this is mergeable from my side. Appreciate the care that went into this — the "please don't auto-close #879" style of coordination across your two PRs is noticed and welcome.

[b4rk13]

All four addressed in 2e833042.

Bug 1 (export output) — fixed via the compat map, with the paper-trail comment style you called out: actionItemExport's own doc says agents send the key, so rejecting it at the gate contradicted the handler's reason to exist. The new TestPadItemExport_AgentOutputPassesStrictGate drives the real fan-out dispatch path — written first, and it failed with "nothing dispatched — the gate rejected a key the handler exists to override", which reproduces exactly the break you described. It also asserts the override still lands (--output -, agent path absent).

One deliberate non-fix while I was in there: the analogous file key on action=import stays rejected. The schema steers agents to artifact, actionItemImport overwrites any supplied file anyway, and the rejection hint lists the declared params — so a loud error is the correct steer rather than a compat entry. Say the word if you'd rather have it accepted for symmetry.

Bug 2 (empty key) — fixed; fields: {"": "v"} now refuses with "fields: keys cannot be empty" before the = check, test-pinned.

Scope note — documented as a named limitation: the merge-contract comment carries a KNOWN LIMITATION block (non-scalar field types — multi_select, json — refuse loudly rather than round-trip), and the fields param description says the same in one line. Array/JSON encoding onto the field path noted as the follow-up.

Intent question — confirmed: the promoted-key shadowing is an accepted trade-off. This layer can't see collection schemas, the dedicated params already shadow such fields everywhere on the tool, and the field: ["key=value"] path has the same collision. Now written into the merge-contract comment so the next reader doesn't re-derive it.

internal/mcp suite is at the same pre-existing-failure baseline as before the change (the Windows set), lint clean.

[xarmian]

Pushed a maintainer commit (59646ee) rather than bouncing this back to you: while this sat in review, main took ToolSurfaceVersion 0.22 (#1165) and 0.23 (#1166), so your bump is now 0.24. The merge keeps all your code; conflicts were only in the docs/changelog, resolved in favor of main's newer text with your fields-object and strict-validation entries renumbered on top. No action needed from your side — review of the round-2 changes continues on the updated head.

[xarmian]

Round-2 re-review clean; version renumbered to 0.24 maintainer-side (0.22/0.23 were taken by #1165/#1166 while this was in review); CI 7/7 green on the renumbered head.

[xarmian]

Merged, thank you. Strict input validation is the real prize here: every future undeclared-param typo now fails loudly across all eleven tools instead of returning success while doing nothing, which retires an entire bug class, and the compat carve-out map with a paper trail per entry is how that kind of gate stays honest over time. The fields object alias fixes the trap every agent fell into (reads return a shape that writes silently dropped). One heads-up on adjacent territory: the github_pr field-setter path you'll see referenced in the catalog docs is broken independently of this PR (stored double-encoded, tracked internally as BUG-2696) — pre-existing, found during an internal unit this week, not something your change introduced or needs to address.