mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-22 18:43:45 +00:00
xarmian
438cb6180a
* fix(mcp): flexible JSON shapes on item create + clearer field surface (BUG-1431, BUG-1432)
BUG-1432 root cause (real): models.ItemCreate.Tags is a Go string, so
the default unmarshaler rejected the natural JSON-array shape every
agent sends (`tags: ["foo","bar"]` → "cannot unmarshal array into Go
struct field ItemCreate.tags of type string", HTTP 400). On Postgres
the alternative — passing `tags: "foo,bar"` per the catalog's old
"Comma-separated tags" description — landed as a non-JSON value in
the JSONB column and surfaced as a generic HTTP 500. SQLite's TEXT
column silently accepted the corrupt value, which is why local repros
didn't show it.
Codex's independent investigation called out the asymmetry: ItemUpdate
already had a flexible UnmarshalJSON for `fields`/`tags` per BUG-1144,
but ItemCreate didn't. This PR mirrors that flexibility on the create
path and aligns the MCP surface description with reality.
BUG-1431 root cause (real, not the misdiagnosis the agent reported):
the dispatcher's `parseFieldKVP` only accepted the CLI-style array-of-
"key=value" shape, rejecting the JSON-native `field: {key: value}` map
shape with "expected array or string, got map[string]interface {}".
Agents naturally try the map shape and got a non-actionable error;
that drove the BUG-1409 agent to mis-blame status placement. Empirical
repro confirmed that `status` actually works in both top-level AND
inside-fields positions today (Tests 1, 4 in the investigation); the
real surface problem was the missing map shape on `field`.
Changes:
- internal/models/item.go: add UnmarshalJSON to ItemCreate mirroring
ItemUpdate's BUG-1144 pattern. Accepts `fields` as object or
JSON-encoded string; `tags` as array or JSON-encoded string; either
field absent / null leaves Go zero value. Wrong shapes surface
ErrInvalidFieldsType / ErrInvalidTagsType (existing sentinels) so
agents see clean domain errors instead of "Go struct field" leaks.
- internal/mcp/dispatch_http.go: parseFieldKVP now accepts
map[string]any in addition to the existing array/string shapes. Map
shape preserves non-string values verbatim (e.g. number from a typed
flag), matching the array path's existing pass-through for non-string
entries.
- internal/mcp/catalog_item.go: update `tags` description from
"Comma-separated tags" (wrong on both SQLite and Postgres) to
"Tags as a JSON array of strings, e.g. [\"v1\",\"frontend\"]". Update
`field` description to clarify it's the escape hatch for
SCHEMA-DECLARED custom fields, name the dedicated top-level params
agents should reach for instead (status/priority/category/parent/
role/assign/tags), and note the new map-shape acceptance. Tool-level
prose updated to match.
Tests:
- TestItemCreateUnmarshalFlexFields (mirror of
TestItemUpdateUnmarshalFlexFields): 9 cases covering array/string/
null/absent/wrong-shape tags + object/string/array fields, plus a
smoke test that other fields decode normally alongside the new
flex paths.
- TestParseFieldKVP_Variants: extended with 3 new map-shape cases
(basic map, empty-key-skipped, non-string-value preserved).
End-to-end verification: 5 input shapes via curl against the live
handler. Pre-fix `tags: ["foo","bar"]` returned HTTP 400; post-fix
returns HTTP 201 with `tags="[\"foo\",\"bar\"]"` in the column.
`tags: {x:1}` (wrong shape) now returns a clean
domain-level 400 instead of leaked Go internals. Existing back-compat
paths (JSON-encoded string forms) preserved.
Related: PR #546 (BUG-1430 rate limit) addressed the original 500
cascade that drove the agent's specific misdiagnoses in BUG-1409.
* fix(mcp): forward tags array on update + drop unsupported map-shape doc per Codex review (round 1)
Codex round 1 caught two issues:
[P1] dispatch_http_advanced.go's PATCH builder filtered on `string`
only when forwarding `tags`, so a schema-conforming
`pad_item.update tags: ["a","b"]` was silently dropped. Now forwards
verbatim like mapItemCreate does — the handler's ItemUpdate
flex-unmarshaler (BUG-1144) normalizes any shape downstream.
Regression test added.
[P2] The `field` description claimed `{key: value}` map shape was
accepted, but the schema Type stays `array<string>` so schema-following
clients won't send the map shape. parseFieldKVP's map-shape handling
(added in the previous commit) stays as defensive parsing for clients
that ignore the schema, but the description no longer promises a shape
the published schema doesn't advertise. Tool-level prose updated to
match.
* fix(mcp): revert speculative parseFieldKVP map-shape support per Codex review (round 2)
Codex round 2 [P2] pointed out the map-shape parseFieldKVP support
added in the first commit is dead code in practice:
1. The advertised schema for `field` is `array<string>` — no
schema-conforming client sends a map.
2. `BuildCLIArgs` rejects map-shaped repeatable flags before they
reach the HTTP dispatcher.
3. Even if a map did reach the dispatcher, `hasFieldChanges`
doesn't recognize map shapes as field changes — `pad_item.update
field: {effort: "l"}` would skip the merge and PATCH without
`fields`.
Either completing the support (fix hasFieldChanges + BuildCLIArgs +
ItemUpdate Unmarshal) OR reverting was the right call. Reverting
keeps the surface consistent with the schema and removes the
unreachable code; future agents who want to override fields can use
the documented `["key=value"]` array shape.
BUG-1431's functional fix lands as the catalog description tightening
(the empirical repro confirmed `status` placement already works in
both forms; the agent's misdiagnosis was rooted in unclear docs, not
broken code). BUG-1432's flexible JSON unmarshal on ItemCreate stays
— that's the real fix verified by the live-handler repro.
* fix(mcp): preserve empty-string tags no-op + table-driven test per Codex review (round 3)
Codex round 3 [P2] caught a regression introduced in round 1's fix: by
switching the tags forwarding guard from \`v.(string) && v != ""\` to
\`v != nil\` to support array shapes, the empty-string filter for tags
on update was lost. \`pad_item.update tags: ""\` would now forward an
empty string to ItemUpdate, which treats it as an explicit
empty-string write — corrupting the JSON/JSONB tags column (500 on
Postgres).
Fix: type-switch on tags. Empty string skips (matches pre-fix
behaviour); arrays (including empty array \`[]\`, the legitimate
"clear tags" case) and non-empty strings forward.
Tests: the single-shape array test is replaced with a table-driven
TestDispatchItemUpdate_TagsForwarding covering array, empty array,
empty string (no-op), and comma-separated back-compat. Each case
asserts the tags key's presence/absence and shape in the PATCH body.