diff --git a/CLAUDE.md b/CLAUDE.md index 34141f850..2bd228661 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,6 +76,7 @@ REST API at `/api/v1/`. Key endpoints: - `GET/POST /workspaces/{ws}/collections` — collection CRUD - `GET/POST /workspaces/{ws}/collections/{coll}/items` — item CRUD - `GET/PATCH/DELETE /workspaces/{ws}/items/{slug}` — item by slug + - **Write responses (create/update) may carry `warnings.undeclared_fields`** (BUG-2850) — field keys stored in the item's `fields` blob that the collection's schema does not declare. They are ACCEPTED, not refused: a census found 168 live values under 14 such keys, and refusing them would break read-modify-write on items nobody edited wrongly. The element is additive and `omitempty`, so a clean write is byte-identical to before; system-written metadata (`implementation_notes`, `decision_log`, `github_pr`, `convention`) is excluded. The CLI prints the same list to **stderr**, never stdout, so `--format json` stays parseable - `POST /workspaces/{ws}/items/{slug}/copy/preflight` — cross-workspace copy dry run: what would carry / drop / need a value, plus the full warning set. Read-only and safe to call repeatedly (PLAN-2357) - `POST /workspaces/{ws}/items/{slug}/copy` — cross-workspace copy; with `archive_source` it is the move. Same request shape as the preflight. **Never retry it automatically** — there is no idempotency key, so a retry duplicates the item - `GET /workspaces/{ws}/dashboard` — computed project overview (active items, plans, attention, blockers) diff --git a/README.md b/README.md index f780bee9d..98caf031d 100644 --- a/README.md +++ b/README.md @@ -388,7 +388,7 @@ directory for `claude-code`, and an `[mcp_servers.pad]` table in project-scoped, it's install-on-request only — `--all` and `pad mcp status` cover the per-user clients (including Codex) and skip it. -**Tool catalog (v0.26)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. Undeclared input keys are rejected with a structured error rather than silently dropped. `pad_item` create/update accept field values as a `fields` object (the same shape reads return) as an equivalent to the dedicated params / `field: ["key=value"]`. `pad_item.list` accepts `unparented: true` (mutually exclusive with `parent`) to select items with no parent or implements relationship, and is summary-shaped by default on both transports (`full: true` opts into complete content bodies): +**Tool catalog (v0.27)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. Undeclared input keys are rejected with a structured error rather than silently dropped. `pad_item` create/update accept field values as a `fields` object (the same shape reads return) as an equivalent to the dedicated params / `field: ["key=value"]`, and its values keep their JSON types where the transport can carry them. Field values are typed against the collection schema server-side, so a declared number or json field is writable from the remote transport (which sends every value as a string). Keys the schema does not declare are stored and NAMED back in `warnings.undeclared_fields`. One key supplied through two doors is adjudicated once: differing values are refused, equal ones collapse, and two names for the same target — `parent`/`plan`, `assign`/`assigned_user_id`, `role`/`agent_role_id` — are refused even when the values match. `pad_item.list` accepts `unparented: true` (mutually exclusive with `parent`) to select items with no parent or implements relationship, and is summary-shaped by default on both transports (`full: true` opts into complete content bodies): | Tool | Actions | |---|---| @@ -417,7 +417,7 @@ initialize handshake under `capabilities.experimental.padCmdhelp` and `pad://_meta/version`): - `cmdhelp_version: "0.1"` — CLI help-tree contract (used at dispatch time) -- `tool_surface_version: "0.26"` — MCP tool catalog contract (v0.5 added `pad_library`; v0.6 `pad_item.backlinks`; v0.7 `pad_item` `export`/`import`; v0.8 `pad_workspace` `deleted`/`restore`; v0.9 made `pad_item.list` summary-shaped by default with a default+max result cap; v0.10 enforced the draft-playbook gate server-side on `pad_playbook.run` with an `allow_draft` escape hatch; v0.11 added the read-only `pad_attachment` tool (`list`/`show`); v0.12 added `pad_project.activity` (agent-accessible non-streaming activity feed); v0.13 added `pad_project` `ready`/`stale` (agent-oriented backlog + attention queries); v0.14 added `pad_item` `history` + optimistic concurrency (TASK-2022); v0.15 added the `pad_item.list` `unparented` parameter (TASK-2096); v0.16 made an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an agent can finally unassign an item (TASK-2571); v0.17 carried that to the LOCAL STDIO transport by teaching the CLI to lift those keys onto their columns instead of into the fields blob (BUG-2583); v0.18 added `clear_assigned_user` / `clear_agent_role` booleans — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` flags on `pad item update` (IDEA-2584); v0.19 added a `clear_parent` boolean — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` flag on `pad item update` (BUG-2078); v0.20 gave every tool an explicit annotation block derived from the catalog’s read-only knowledge — fully-read-only tools advertise `readOnlyHint: true` / `destructiveHint: false`, all-additive-write tools (`pad_workspace`, `pad_library`) drop `destructiveHint`, overwrite/delete-capable tools stay conservatively destructive, `openWorldHint: false` everywhere — replacing mcp-go’s defaults that marked every tool destructive (BUG-2302), and made `pad_item.list` summary-shaped on the remote HTTP transport too, with a declared `full` boolean as the opt-in for complete bodies on both transports (BUG-2305); v0.21 bounded `pad_item.history`, which was unbounded on every surface — `limit` now covers it (default 50, max 300, the NEWEST N; no `offset`, because reverse-patch storage makes only a newest-end window cheap), applied in the catalog action so it lands on both transports, and summary mode now asks the server to skip patch resolution rather than resolving bodies the dispatcher discards (BUG-2608); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces — and on a connection with an explicit workspace allow-list, could not then see them (a wildcard `all_current_workspaces` connection could, which is why the consent mismatch rather than the invisibility is the defect); the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps` (IDEA-2756); see `internal/mcp/version.go` for the full changelog) +- `tool_surface_version: "0.27"` — MCP tool catalog contract (v0.5 added `pad_library`; v0.6 `pad_item.backlinks`; v0.7 `pad_item` `export`/`import`; v0.8 `pad_workspace` `deleted`/`restore`; v0.9 made `pad_item.list` summary-shaped by default with a default+max result cap; v0.10 enforced the draft-playbook gate server-side on `pad_playbook.run` with an `allow_draft` escape hatch; v0.11 added the read-only `pad_attachment` tool (`list`/`show`); v0.12 added `pad_project.activity` (agent-accessible non-streaming activity feed); v0.13 added `pad_project` `ready`/`stale` (agent-oriented backlog + attention queries); v0.14 added `pad_item` `history` + optimistic concurrency (TASK-2022); v0.15 added the `pad_item.list` `unparented` parameter (TASK-2096); v0.16 made an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an agent can finally unassign an item (TASK-2571); v0.17 carried that to the LOCAL STDIO transport by teaching the CLI to lift those keys onto their columns instead of into the fields blob (BUG-2583); v0.18 added `clear_assigned_user` / `clear_agent_role` booleans — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` flags on `pad item update` (IDEA-2584); v0.19 added a `clear_parent` boolean — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` flag on `pad item update` (BUG-2078); v0.20 gave every tool an explicit annotation block derived from the catalog’s read-only knowledge — fully-read-only tools advertise `readOnlyHint: true` / `destructiveHint: false`, all-additive-write tools (`pad_workspace`, `pad_library`) drop `destructiveHint`, overwrite/delete-capable tools stay conservatively destructive, `openWorldHint: false` everywhere — replacing mcp-go’s defaults that marked every tool destructive (BUG-2302), and made `pad_item.list` summary-shaped on the remote HTTP transport too, with a declared `full` boolean as the opt-in for complete bodies on both transports (BUG-2305); v0.21 bounded `pad_item.history`, which was unbounded on every surface — `limit` now covers it (default 50, max 300, the NEWEST N; no `offset`, because reverse-patch storage makes only a newest-end window cheap), applied in the catalog action so it lands on both transports, and summary mode now asks the server to skip patch resolution rather than resolving bodies the dispatcher discards (BUG-2608); v0.22 stopped `pad_item.move` destroying an item’s system metadata — implementation notes, decision log, linked PR and convention data now survive a move, any field the destination schema has no home for is REPORTED in the move’s activity entry rather than vanishing, and a `field` setter naming one of those reserved keys is refused with `malformed_override` instead of writing it (BUG-2674); v0.23 closed the same door on the ordinary update — a `field` setter naming `implementation_notes`, `decision_log` or `convention` is now refused on every transport at once (`validation_error` on HTTP, surfaced to MCP clients as `validation_failed`); the one gate covers the CLI, remote MCP and stdio MCP at once because all three lower a `field` setter into the same `fields_patch`; `github_pr` is deliberately exempt ON UPDATE (move and copy still refuse it), since `pad github link` cannot run on remote MCP and refusing it would leave those agents with no door at all (that door is itself broken — BUG-2696); item CREATE stays open, deliberately, because its full-`fields` payload is shared with Pad’s own writers. v0.23 also added the retry-hostile `stored_state_unreadable` error code so an agent told its target item’s stored data is unreadable stops instead of retrying a permanent failure (BUG-2627 / BUG-2675); v0.24 made the `pad_item` `fields` object a real write form on create/update — reads return `fields` as a native object, and writing that shape back was a silent no-op (accepted, never mapped, dropped while the PATCH still bumped `updated_at`) — merging it into the same path as `field`/the dedicated params with conflicting duplicate keys refused, and made input validation strict across all catalog tools: undeclared top-level keys now fail with a structured error instead of being silently dropped (#1066); v0.25 made `pad_library.activate` resolve its DESTINATION collection from the target’s declared artifact kind (SPEC-5 collection traits) rather than the literal `conventions` / `playbooks` slugs, so activating into a workspace that renamed either collection lands correctly instead of failing not-found with the collection sitting right there (BUG-2702); a lookup ERROR is now surfaced rather than silently falling back to the canonical slug, because falling back on an error means writing to a slug nothing was confirmed about (TASK-2657); v0.26 made `pad_workspace.create` REFUSE with a 403 when the calling OAuth connection's grant has `may_create_workspaces=false` — that checkbox previously gated only the post-creation auto-add, so a connection whose user declined it could still create workspaces — and on a connection with an explicit workspace allow-list, could not then see them (a wildcard `all_current_workspaces` connection could, which is why the consent mismatch rather than the invisibility is the defect); the same gate covers `POST /workspaces/import`, which mints a workspace through a second door. There is deliberately no escape-hatch parameter: the gate expresses the USER's consent decision, so only the user can lift it — by re-authorizing, or by enabling the flag on the existing connection at `/console/connected-apps` (IDEA-2756); v0.27 typed field values server-side so a declared number/json field is writable from the remote transport at all, carried the `fields` object with its JSON types intact, named undeclared keys back in `warnings.undeclared_fields` (accepted rather than refused — a census of 1012 items found 14 such keys across 168 live values, so refusing would have broken read-modify-write on items nobody had edited wrongly), and replaced the accreted per-site conflict guards with ONE check over a canonical view of every source; that check refuses several ambiguities v0.26 resolved silently, chiefly two names for one target in a single call (`parent`/`plan`, `assign`/`assigned_user_id`, `role`/`agent_role_id`), refused even when the values match because the names address one thing through incomparable vocabularies and the two doors resolved them differently (BUG-2850); see `internal/mcp/version.go` for the full changelog) External agents pin against these so a future rename doesn't break them silently. Errors come back as structured envelopes (`{error: {code, diff --git a/cmd/pad/cmd_item.go b/cmd/pad/cmd_item.go index 42dda4fc3..bb5681fac 100644 --- a/cmd/pad/cmd_item.go +++ b/cmd/pad/cmd_item.go @@ -327,6 +327,12 @@ Run with --help-collections to see available collections and their status values return err } + // Before the JSON early-return, and outside the ref branch below: + // the warning goes to STDERR, so it reaches a caller piping stdout + // into a parser — which is exactly the caller most likely to have + // sent a mistyped key and least likely to notice (codex round 2). + warnUndeclaredFields(item) + if formatFlag == "json" { return cli.PrintJSON(item) } @@ -1253,6 +1259,10 @@ Examples: return err } + // Stderr, before the JSON early-return — see the note on the + // create path (codex round 2). + warnUndeclaredFields(updated) + if formatFlag == "json" { return cli.PrintJSON(updated) } @@ -3238,6 +3248,8 @@ Set EDITOR or VISUAL env var to choose your editor (default: vi).`, return err } + warnUndeclaredFields(updated) + ref := cli.ItemRef(*updated) if ref != "" { fmt.Printf("Updated %s %q\n", ref, updated.Title) @@ -3541,3 +3553,20 @@ func starredCmd() *cobra.Command { return cmd } + +// warnUndeclaredFields prints one line to STDERR naming field keys the +// collection's schema does not declare (BUG-2850). +// +// Stderr, never stdout: `pad item create --format json` output is piped into +// scripts, and a warning on stdout would corrupt the JSON they parse. The +// keys are stored either way — this is the trace that makes a typo findable, +// since once written a mistyped key and a deliberate extra field look the +// same. +func warnUndeclaredFields(item *models.Item) { + if item == nil || item.Warnings == nil || len(item.Warnings.UndeclaredFields) == 0 { + return + } + fmt.Fprintf(os.Stderr, "warning: %s not declared by this collection's schema — stored as-is: %s\n", + pluralize(len(item.Warnings.UndeclaredFields), "field", "fields"), + strings.Join(item.Warnings.UndeclaredFields, ", ")) +} diff --git a/cmd/pad/item_same_name_duplicate_precedence_test.go b/cmd/pad/item_same_name_duplicate_precedence_test.go new file mode 100644 index 000000000..131aca33f --- /dev/null +++ b/cmd/pad/item_same_name_duplicate_precedence_test.go @@ -0,0 +1,35 @@ +package main + +// BUG-2850, codex round 7 boundary + the lead's condition on it. +// +// checkHierarchyAliasAmbiguity refuses parent+plan — two NAMES for one target, +// which a caller can collide without knowing. It deliberately does NOT refuse +// a SAME-NAME duplicate (`--status A --field status=B`): those are visibly +// duplicates and both doors resolve them identically, so refusing would be new +// policy rather than a defect fix. +// +// "Both doors resolve them identically" is the load-bearing half of that +// argument, and until now nothing enforced it. This is the stdio door's half; +// internal/mcp/dispatch_http_same_name_duplicate_test.go is the remote door's, +// asserting the same outcome through mapItemUpdate. If either door's overlay +// order is ever reordered, one of the two fails and the boundary gets +// re-examined instead of silently becoming untrue. +// +// The resolution is `--field` wins: cmd_item.go applies the named flags into +// the patch first, then overlays the --field pairs (the same order +// dispatch_http_advanced.go uses). Asserted, not assumed — a test that only +// checked "one of them won" would pass on a build where the doors disagreed. + +import "testing" + +func TestItemUpdate_SameNameDuplicateResolvesFieldWins(t *testing.T) { + body := captureUpdateBody(t, "TASK-9", "--status", "open", "--field", "status=done") + + fp := fieldsPatchOf(t, body) + if fp == nil { + t.Fatal("expected a fields_patch") + } + if got := fp["status"]; got != "done" { + t.Fatalf("status = %v, want %q — the --field entry overlays the named flag on this door", got, "done") + } +} diff --git a/internal/items/coerce.go b/internal/items/coerce.go new file mode 100644 index 000000000..6f60267b0 --- /dev/null +++ b/internal/items/coerce.go @@ -0,0 +1,136 @@ +package items + +import ( + "encoding/json" + "math" + "sort" + "strconv" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// CoerceFields converts STRING field values to the type the collection schema +// declares, so every write door stores the same native type for the same input +// (BUG-2850). +// +// The doors did not agree. The CLI has coerced by schema type since BUG-1125 +// (cmd/pad/cmd_item.go::parseFieldFlag), and local stdio MCP inherits that by +// shelling out to the binary — but the remote /mcp transport builds its field +// map in ingestFieldKVP, which does `dst[key] = val` unconditionally, so every +// value arrives as a string. validateFieldType then correctly refuses a string +// for a declared number/json field, and the net effect was that an MCP agent on +// that transport could not write those fields AT ALL: every attempt a 400. +// Typing belongs to the server, keyed on the schema, so the doors cannot drift +// again. +// +// WHAT THIS DELIBERATELY DOES NOT DO: +// +// - It does not report errors. A value that will not coerce is left as the +// string and handed to the validator, which already produces the right +// message ("field %q must be a number"). Coercion never invents an error +// path, which is also why it cannot turn a currently-PASSING write into a +// failure — the only inputs whose behaviour changes are ones that are +// 400ing today. +// - It does not touch non-string values. A caller already sending 42 keeps +// sending 42; this is not a re-typing pass over well-formed input. +// - It does not touch keys the schema does not declare. Those are stored as +// given, silently, which is the OTHER half of BUG-2850 — see the decision +// point below. +// +// It returns a new map rather than mutating in place: two of the call sites +// re-marshal the map they pass, and a function that quietly rewrote their input +// would change what gets stored from behind a name that does not say so. +func CoerceFields(fields map[string]any, schema models.CollectionSchema) map[string]any { + if len(fields) == 0 { + return fields + } + byKey := make(map[string]models.FieldDef, len(schema.Fields)) + for i := range schema.Fields { + byKey[schema.Fields[i].Key] = schema.Fields[i] + } + + out := make(map[string]any, len(fields)) + for k, v := range fields { + def, declared := byKey[k] + if !declared { + // DECISION POINT — BUG-2850 undeclared-key disposition (with Dave: + // refuse / warn / keep). Today's behaviour is KEEP, which is what + // silently stores materials_cost="42" on a collection that never + // declared it. Refuse becomes a returned issue here; warn becomes a + // collected key. Deliberately isolated to this branch so the choice + // drops in without touching the coercion above it. + out[k] = v + continue + } + out[k] = coerceValue(def, v) + } + return out +} + +// coerceValue converts one string value to its declared type, or returns it +// unchanged when it is not a string or will not parse. +func coerceValue(def models.FieldDef, v any) any { + s, ok := v.(string) + if !ok { + return v + } + switch def.Type { + case "json", "multi_select": + var parsed any + if err := json.Unmarshal([]byte(s), &parsed); err == nil { + return parsed + } + case "number": + if f, err := strconv.ParseFloat(s, 64); err == nil { + // Reject NaN / ±Inf rather than storing them: encoding/json cannot + // marshal either, and the downstream json.Marshal(fields) error is + // ignored, so a non-finite float silently drops the ENTIRE fields + // payload. Falling through leaves the string for the validator, + // which says "must be a number" — the same reasoning as the CLI's + // guard in parseFieldFlag (BUG-1125). + if !math.IsNaN(f) && !math.IsInf(f, 0) { + return f + } + } + case "checkbox": + if b, err := strconv.ParseBool(s); err == nil { + return b + } + } + // text, url, select, date, relation — a string is already the right type. + // Anything that did not parse above also lands here, on purpose. + return s +} + +// UndeclaredFieldKeys returns the keys of fields the schema does not declare, +// sorted, excluding system-written metadata (BUG-2850). +// +// These keys are stored, not refused — see ItemWriteWarnings for why. The +// caller reports them so a typo leaves a trace: once written, a mistyped key +// and a deliberate extra field look identical, and the reporter's case was an +// agent that could not tell its own data had gone somewhere unintended. +// +// Reserved keys are excluded via models.IsReservedItemField rather than a +// second list here — that set exists precisely so callers ask instead of +// re-listing, and its own doc comment records what re-listing cost last time. +func UndeclaredFieldKeys(fields map[string]any, schema models.CollectionSchema) []string { + if len(fields) == 0 { + return nil + } + declared := make(map[string]struct{}, len(schema.Fields)) + for i := range schema.Fields { + declared[schema.Fields[i].Key] = struct{}{} + } + var out []string + for k := range fields { + if _, ok := declared[k]; ok { + continue + } + if models.IsReservedItemField(k) { + continue + } + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/internal/items/coerce_test.go b/internal/items/coerce_test.go new file mode 100644 index 000000000..f55558899 --- /dev/null +++ b/internal/items/coerce_test.go @@ -0,0 +1,173 @@ +package items + +import ( + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// The schema every case below is coerced against. +func coerceSchema() models.CollectionSchema { + return models.CollectionSchema{Fields: []models.FieldDef{ + {Key: "cost", Type: "number"}, + {Key: "spec", Type: "json"}, + {Key: "tags", Type: "multi_select"}, + {Key: "done", Type: "checkbox"}, + {Key: "note", Type: "text"}, + {Key: "due", Type: "date"}, + }} +} + +// The defect this exists for: the remote /mcp door builds its field map with +// `dst[key] = val`, so a declared number field arrives as the STRING "42" and +// validateFieldType refuses it — the field is unwritable on that transport +// (BUG-2850). Every assertion here is about the native TYPE that reaches the +// store, because "the write succeeded" is what the CLI door already did. +func TestCoerceFieldsTypesDeclaredStrings(t *testing.T) { + in := map[string]any{ + "cost": "42", + "spec": `[{"name":"a"}]`, + "tags": `["x","y"]`, + "done": "true", + } + out := CoerceFields(in, coerceSchema()) + + if got, ok := out["cost"].(float64); !ok || got != 42 { + t.Fatalf("cost: want float64(42), got %[1]T(%[1]v)", out["cost"]) + } + if _, ok := out["spec"].([]any); !ok { + t.Fatalf("spec: want []any, got %[1]T(%[1]v)", out["spec"]) + } + if _, ok := out["tags"].([]any); !ok { + t.Fatalf("tags: want []any, got %[1]T(%[1]v)", out["tags"]) + } + if got, ok := out["done"].(bool); !ok || !got { + t.Fatalf("done: want bool(true), got %[1]T(%[1]v)", out["done"]) + } +} + +// Types that are already strings must stay strings. A "coerce everything that +// parses" implementation would turn a text field holding "42" into a number and +// corrupt data that was never broken — this is the guard against fixing the bug +// by over-reaching. +func TestCoerceFieldsLeavesStringTypedFieldsAlone(t *testing.T) { + out := CoerceFields(map[string]any{ + "note": "42", + "due": "2026-09-02", + }, coerceSchema()) + + if got, ok := out["note"].(string); !ok || got != "42" { + t.Fatalf("note: want string(\"42\"), got %[1]T(%[1]v)", out["note"]) + } + if got, ok := out["due"].(string); !ok || got != "2026-09-02" { + t.Fatalf("due: want the date string, got %[1]T(%[1]v)", out["due"]) + } +} + +// A value that will not parse is handed to the validator UNCHANGED, so the +// existing "must be a number" error still fires. Coercion must not invent an +// error path, and must not swallow a bad value into something plausible. +func TestCoerceFieldsLeavesUnparseableValuesForTheValidator(t *testing.T) { + out := CoerceFields(map[string]any{ + "cost": "not-a-number", + "spec": "{definitely not json", + }, coerceSchema()) + + if got, ok := out["cost"].(string); !ok || got != "not-a-number" { + t.Fatalf("cost: want the original string, got %[1]T(%[1]v)", out["cost"]) + } + if err := ValidateFields(out, coerceSchema()); err == nil { + t.Fatal("expected the validator to still refuse the un-coercible value") + } +} + +// NaN and ±Inf parse as floats and then cannot be marshalled: encoding/json +// fails, the downstream json.Marshal(fields) error is ignored, and the ENTIRE +// fields payload is silently dropped. They must fall through as strings so the +// validator refuses them loudly instead. +func TestCoerceFieldsRefusesNonFiniteNumbers(t *testing.T) { + for _, raw := range []string{"NaN", "Inf", "-Inf", "+Inf"} { + out := CoerceFields(map[string]any{"cost": raw}, coerceSchema()) + if f, ok := out["cost"].(float64); ok { + t.Fatalf("%q was coerced to float64(%v); non-finite values must stay strings", raw, f) + } + } +} + +// Non-string values pass through untouched. This is not a re-typing pass over +// well-formed input — a caller already sending 42 keeps sending 42, and an +// int must not become a float64 behind their back. +func TestCoerceFieldsPassesNonStringsThrough(t *testing.T) { + in := map[string]any{"cost": 42, "spec": []any{"already", "parsed"}, "done": true} + out := CoerceFields(in, coerceSchema()) + + if got, ok := out["cost"].(int); !ok || got != 42 { + t.Fatalf("cost: want int(42) untouched, got %[1]T(%[1]v)", out["cost"]) + } + if _, ok := out["spec"].([]any); !ok { + t.Fatalf("spec: want []any untouched, got %T", out["spec"]) + } + if got, ok := out["done"].(bool); !ok || !got { + t.Fatalf("done: want bool(true) untouched, got %[1]T(%[1]v)", out["done"]) + } +} + +// Keys the schema does not declare are left exactly as they arrived. This is +// TODAY'S behaviour and the other half of BUG-2850 — the disposition (refuse / +// warn / keep) is a product decision still open. The test pins what the code +// does so the decision, when it lands, is a deliberate change to a stated +// behaviour rather than a silent one. +func TestCoerceFieldsLeavesUndeclaredKeysUntouched(t *testing.T) { + out := CoerceFields(map[string]any{"materials_cost": "42"}, coerceSchema()) + + if got, ok := out["materials_cost"].(string); !ok || got != "42" { + t.Fatalf("undeclared key: want the string untouched, got %[1]T(%[1]v)", out["materials_cost"]) + } +} + +// CoerceFields returns a NEW map. Two call sites re-marshal the map they pass +// in, so a function that quietly rewrote its argument would change what gets +// stored from behind a name that does not say so. +func TestCoerceFieldsDoesNotMutateItsInput(t *testing.T) { + in := map[string]any{"cost": "42"} + _ = CoerceFields(in, coerceSchema()) + + if got, ok := in["cost"].(string); !ok || got != "42" { + t.Fatalf("input was mutated: cost is now %[1]T(%[1]v)", in["cost"]) + } +} + +// Reserved keys are system-written metadata, not a user's stray field, so +// naming them as "undeclared" on every write that carries one would be noise +// the reader learns to ignore (BUG-2850). +// +// Excluded via models.IsReservedItemField rather than a second list here: that +// set exists so callers ask instead of re-listing, and its doc comment records +// what re-listing cost the last time someone did it. +func TestUndeclaredFieldKeysExcludesReservedMetadata(t *testing.T) { + got := UndeclaredFieldKeys(map[string]any{ + "implementation_notes": "written by pad item note", + "decision_log": "written by pad item decide", + "github_pr": "written by pad github link", + "convention": "system", + "materials_cost": 42, + }, coerceSchema()) + + if len(got) != 1 || got[0] != "materials_cost" { + t.Fatalf("undeclared = %v, want only [materials_cost] — reserved metadata must not be reported", got) + } +} + +// Declared keys are never reported, and the result is sorted so the warning +// text is stable across runs (map iteration order is not). +func TestUndeclaredFieldKeysIsSortedAndSkipsDeclared(t *testing.T) { + got := UndeclaredFieldKeys(map[string]any{ + "cost": 1, + "zebra": 1, + "alpha": 1, + }, coerceSchema()) + + if len(got) != 2 || got[0] != "alpha" || got[1] != "zebra" { + t.Fatalf("undeclared = %v, want [alpha zebra] sorted, with the declared 'cost' absent", got) + } +} diff --git a/internal/mcp/catalog_item.go b/internal/mcp/catalog_item.go index ac33d495b..30044c71c 100644 --- a/internal/mcp/catalog_item.go +++ b/internal/mcp/catalog_item.go @@ -222,7 +222,7 @@ var padItemSchemaParams = []ParamDef{ // return `fields` as a native object, so writing that same shape // back is what agents naturally do — it used to be silently // dropped. See catalog_item_fields.go for the merge contract. - {Name: "fields", Type: "object", Description: "Field values as one OBJECT (e.g. {\"status\":\"done\",\"effort\":\"l\"}) — the same shape reads return. Only for: create, update. Merges into the same path as `field`/the dedicated params; a key given here AND at the top level (or in `field`) with a DIFFERENT value is REFUSED, not silently resolved. Values must be scalars (tags may be an array) — non-scalar field types (multi_select, json) are refused, not written; pass those via a supported path until array/JSON encoding lands."}, + {Name: "fields", Type: "object", Description: "Field values as one OBJECT (e.g. {\"status\":\"done\",\"effort\":\"l\"}) — the same shape reads return. Only for: create, update. Merges into the same path as `field`/the dedicated params; a key given here AND at the top level (or in `field`) with a DIFFERENT value is REFUSED, not silently resolved. Values keep their JSON type: a number stays a number, and an object or array is written as-is to multi_select / json fields (BUG-2850). Two exceptions: `null` is refused (omit the key to leave a field unchanged), and parent/plan must be a string ref. Structured values require the REMOTE transport — the local stdio server shells out to the CLI, whose --field key=value encoding cannot carry them, and refuses with a message naming the transport."}, // ── List / starred ── {Name: "all", Type: "bool", Description: "Include archived/done items in list responses. Optional for: list, starred."}, diff --git a/internal/mcp/catalog_item_fields.go b/internal/mcp/catalog_item_fields.go index 02654e944..f4a8a988d 100644 --- a/internal/mcp/catalog_item_fields.go +++ b/internal/mcp/catalog_item_fields.go @@ -63,6 +63,480 @@ import ( // top-level param it promotes onto. Mirrors the "the dispatcher rolls // those into the fields JSON automatically" list in the `field` param // description — keep the two in sync. +// hierarchyPseudoFieldKeys are `fields` keys the SERVER reads as parent-link +// directives rather than as ordinary field values (extractParentLink, in +// internal/server/handlers_items.go). They take a string ref and nothing else. +// +// `parent` is also in padItemPromotedFieldKeys below and gets a scalar check +// there; `plan` is not, which is how a structured value reached the server +// once BUG-2850 stopped refusing structures. Both are listed here so the +// guard does not depend on which other set a key happens to belong to. +var hierarchyPseudoFieldKeys = map[string]bool{ + "parent": true, + "plan": true, +} + +// hierarchyAliasKeys is the same set in the order extractParentLink reads it +// (handlers_items.go: `for _, key := range []string{"parent", "plan"}`, no +// early exit, so the LATER key wins). Kept as a slice for deterministic error +// text when one alias conflicts with the other. +var hierarchyAliasKeys = []string{"parent", "plan"} + +// identityRefFieldKeys are promoted keys whose value NAMES something (a user, +// a role) rather than being a value in its own right. They take a string and +// nothing else — see the refusal in reshapeItemFields for why a number is +// worse than useless here. +var identityRefFieldKeys = map[string]bool{ + "assign": true, + "role": true, +} + +// ─── ONE CANONICAL VIEW, ONE CONFLICT CHECK (BUG-2850, lead ruling after +// codex round 13) ─────────────────────────────────────────────────────────── +// +// Rounds 5, 7, 11, 12 and 13 each found a defect in the PREVIOUS round's fix, +// and every one was the same shape: conflict handling had accreted a separate +// guard at each site that noticed a problem — the generic path, the promoted +// block, the alias check, the compat-ID block, the canonicalization predicate +// — and each guard only covered the sources its author happened to think +// about. Round 13's finding is the proof: `assign` and `assigned_user_id` are +// two names for one target, exactly like `parent`/`plan`, and NO guard +// compared them, because the alias guard knew only about hierarchy and the +// compat guard knew only about same-name collisions. +// +// So the guards are replaced by one pass that resolves EVERY source to a +// canonical key and refuses on the resulting map. A new alias pair is now one +// line in fieldAliasGroups rather than a sixth guard with its own edges. + +// fieldAliasGroups maps a key to the canonical name of the thing it WRITES. +// Members of a group are different names for one target, so two of them in +// one call is ambiguous by construction — their value spaces are not even +// comparable (`assign` takes a slug or email, `assigned_user_id` a UUID), +// which is why co-occurrence refuses rather than trying to decide equality. +var fieldAliasGroups = map[string]string{ + "parent": "parent", + "plan": "parent", + "assign": "assign", + "assigned_user_id": "assign", + "role": "role", + "agent_role_id": "role", +} + +func canonicalFieldKey(k string) string { + if g, ok := fieldAliasGroups[k]; ok { + return g + } + return k +} + +// fieldContribution is one source offering one value for one canonical key. +type fieldContribution struct { + key string // as the caller wrote it + source string // human-readable, for the refusal + value string // normalized for comparison; "" when unstringifiable + nested bool // a structure, with no key=value encoding + raw any // the value as supplied, for comparing two structures + + // nonCanonical marks a `field` array entry written in something other + // than its canonical `key=value` form (padding around either half). It + // matters because the two doors then receive DIFFERENT writes. + nonCanonical bool + + // topLevel marks a contribution that arrived as a top-level param rather + // than through `field` or `fields`. It is what the compat-ID exception + // actually turns on — see the gate below. + topLevel bool +} + +// topLevelConflictKeys are the top-level params that can collide with a +// `fields` entry. Built from the three sets so adding a key to any of them +// cannot leave this pass behind. +func topLevelConflictKeys() []string { + seen := map[string]bool{} + var out []string + for _, set := range []map[string]bool{padItemPromotedFieldKeys, compatIDFieldKeys} { + for k := range set { + if !seen[k] { + seen[k] = true + out = append(out, k) + } + } + } + for k := range hierarchyPseudoFieldKeys { + if !seen[k] { + seen[k] = true + out = append(out, k) + } + } + sort.Strings(out) + return out +} + +// detectFieldConflicts builds the canonical map and refuses on it. It is the +// ONLY place a conflict is decided; the emission code below runs knowing the +// input is unambiguous. +// +// SCOPE: it runs from BOTH action entry points (create and update) on every +// call, whether or not a `fields` object is present. It used to run only from +// inside reshapeItemFields, which returns early without one — codex round 14 +// found the alias pair that slipped through that gap, and the body below +// carries the detail. +// +// Two halves, with different reach: +// +// - ALIAS collisions (parent/plan, assign/assigned_user_id, +// role/agent_role_id) are adjudicated ALWAYS, through any combination of +// sources, and refuse even when the values match. Two names for one +// target address it through different vocabularies — a slug and a UUID — +// so "equal" is not a question this layer can answer. +// +// - SAME-NAME collisions are adjudicated only when something will +// arbitrate them, which is a PER-KEY question: does the `fields` object +// carry THIS key (rounds 17 and 19 were each a per-request predicate +// standing in for it). Otherwise the doors resolve it themselves and the +// round-7 exemption applies — the array overlays the named param, +// identically on both, so last-write-wins is a resolution the caller can +// predict. Pinned per door by the SameNameDuplicate tests. +// +// The exemption is NARROWER than "same name". It holds only for keys the CLI +// can express: the v0.16 compat IDs have no flag behind them, so BuildCLIArgs +// drops the top-level value while HTTP reads it, and those refuse instead +// (round 15) — but only when a top-level compat value is actually present, +// which is the asymmetry's source (round 20). A padded entry is outside it +// too, since the doors then receive different writes (round 16). +// +// If you are about to add a condition here, say its QUANTIFIER out loud +// first. Per-key or per-request, this key class or all of them: rounds 15, +// 16, 17, 19, 20 and 21 were each that question answered by assumption. +func detectFieldConflicts(prefix string, input map[string]any) *mcp.CallToolResult { + // Parsed here rather than passed in, so this pass sees the same inputs + // whether or not a `fields` object exists (codex round 14). It used to + // run only from inside reshapeItemFields, which returns early without + // `fields` — so an ALIAS pair arriving through the top level and the + // `field` array alone slipped past it, and the doors diverged: + // `assigned_user_id:"B"` with `field:["assign=dave"]` applies the compat + // ID over HTTP while stdio drops it and sends only the generic field. + // + // Round 7 had already built exactly this always-run guard, for the + // hierarchy pair only (checkHierarchyAliasAmbiguity, now deleted). So the + // restructure that was supposed to end guard accretion had itself left + // TWO alias mechanisms with different reach. One is what the ruling + // asked for; this is it. + obj, _ := input["fields"].(map[string]any) // nil when absent; shape errors belong to reshapeItemFields + // NOTE: there is deliberately no `fieldsPresent` here any more. Both + // gates below ask the per-KEY question — is THIS key carried by the + // object — and rounds 17 and 19 were each a per-request predicate + // standing in for it. The variable's absence is the fix's shape: if a + // future gate wants "does the request have a fields object", that is + // almost certainly the same mistake a third time. + // The INDEX is deliberately discarded here — this pass walks the raw + // entries so two entries naming one key stay two contributions (round 18). + fieldEntries, _, errRes := parseFieldArray(prefix, input["field"]) + if errRes != nil { + return nil // the caller parses for real and owns this error surface + } + + groups := map[string][]fieldContribution{} + add := func(canonical string, c fieldContribution) { + groups[canonical] = append(groups[canonical], c) + } + + objKeys := make([]string, 0, len(obj)) + for k := range obj { + objKeys = append(objKeys, k) + } + sort.Strings(objKeys) // deterministic refusal text across runs + for _, k := range objKeys { + sv, err := stringifyFieldValue(obj[k]) + // COMPARED TRIMMED, EMITTED RAW (codex round 19). Entry values are + // trimmed for comparison because ingestFieldKVP trims them, so + // comparing a trimmed entry against an untrimmed `fields` value was + // apples to oranges: `fields:{"note":" x "}` with `field:["note= x "]` + // read as " x " vs "x" and refused, though both doors write " x ". + // Only `value` (the comparison key) is trimmed; `raw` keeps the + // original, and the canonical re-emission still carries the untrimmed + // value to the wire. + add(canonicalFieldKey(k), fieldContribution{ + key: k, source: "fields." + k, value: strings.TrimSpace(sv), nested: err != nil, raw: obj[k], + }) + } + + // ONE CONTRIBUTION PER ENTRY, not per indexed key (codex round 18). + // + // parseFieldArray indexes by NORMALIZED key, so two entries naming one + // key collapse to a single index slot — and iterating that index made + // this pass's own input lossy. `field:["effort=l", " effort=l"]` arrived + // as ONE contribution, fell under the len < 2 early exit, and passed + // unchecked: HTTP trims both to `effort` while stdio writes `effort` AND + // a junk `" effort"`. The pass claims to adjudicate one canonical key + // offered by multiple sources; two array entries ARE multiple sources, + // and it could not see them. + // + // Walking the raw entries keeps the multiplicity, so the existing rules + // apply unchanged: equal canonical duplicates collapse, and a padded twin + // is refused by the non-canonical check like any other. + for _, entry := range fieldEntries { + k, v, ok := strings.Cut(entry, "=") + if !ok { + continue // no '=' — the CLI owns that error surface, not this pass + } + key := strings.TrimSpace(k) + val := strings.TrimSpace(v) + add(canonicalFieldKey(key), fieldContribution{ + key: key, source: "the field array entry " + strconv.Quote(entry), value: val, raw: val, + // Whether the entry as WRITTEN matches its canonical form. The + // index is normalized so a padded entry can be recognized at all; + // this remembers that the doors will not receive it identically. + nonCanonical: entry != key+"="+val, + }) + } + + for _, k := range topLevelConflictKeys() { + v, present := input[k] + if !present || !topLevelValueProvided(k, v) { + continue + } + sv, err := stringifyFieldValue(v) + add(canonicalFieldKey(k), fieldContribution{ + key: k, source: "the top-level " + k + " param", value: sv, nested: err != nil, raw: v, topLevel: true, + }) + } + + canonicals := make([]string, 0, len(groups)) + for k := range groups { + canonicals = append(canonicals, k) + } + sort.Strings(canonicals) + + for _, canonical := range canonicals { + contribs := groups[canonical] + if len(contribs) < 2 { + continue + } + // ALIAS COLLISION: two different NAMES for one target. Refused even + // when the values look equal — the names address the same thing + // through different vocabularies, so "equal" is not a question this + // layer can answer. + for i := range contribs { + if contribs[i].key == contribs[0].key { + continue + } + a, b := contribs[0], contribs[i] + return errStructured(prefix, fmt.Errorf( + "%s conflicts with %s — %q and %q are two names for the same thing and the doors resolve them differently; pass one of them", + a.source, b.source, a.key, b.key)) + } + // SAME NAME, different sources: equal collapses, differing refuses. + // + // Gated on `fields` being present, because that is this merge's + // scope. A top-level param colliding with a `field:[]` entry and no + // `fields` object keeps its documented last-write-wins resolution — + // the round-7 boundary the lead confirmed, pinned on both doors by + // the SameNameDuplicate tests. Alias collisions above are NOT gated: + // last-write-wins is only defensible when both sources name the same + // key, and two names for one target have incomparable value spaces. + // EQUALITY ONLY LICENSES A COLLAPSE WHEN BOTH DOORS RECEIVE THE SAME + // WRITE — and this runs BEFORE the exemption below, deliberately + // (codex round 16). + // + // The conflict index is normalized, so `field:["k = A"]` compares + // EQUAL to a top-level `k:"A"` and the pair was accepted while the + // entry stayed padded on the wire. HTTP trims it and writes `k`; the + // CLI does not, and writes a junk `"k "` key instead. The + // normalization that lets the collision be SEEN is exactly what made + // accepting it wrong. + // + // It sits above the exemption because padding breaks the exemption's + // own premise — that both doors resolve the duplicate identically — + // for EVERY key class, not just the compat IDs. Round 15 was this + // same mistake (a premise verified for declared params, generalized + // to keys it did not hold for); putting this check below the + // exemption would have repeated it one round later, and my first + // draft did exactly that. + // + // Only when nothing will canonicalize the entry, i.e. no `fields` + // object — with one present, reshapeItemFields re-emits it + // canonically and equality is safe again. + // + // Deliberately NOT extended to a padded entry standing ALONE with no + // colliding param: that is BUG-2870, ruled out of this PR's scope, + // and it changes what every CLI caller receives. Here the caller has + // supplied one key twice and one of the forms is malformed, which is + // a narrower and locally-answerable question. + // WHETHER THIS KEY GETS CANONICALIZED IS A PER-KEY QUESTION, not a + // per-request one (codex round 17). + // + // Round 16 gated this on `!fieldsPresent`, reasoning that with a + // `fields` object present reshapeItemFields re-emits the entry + // canonically. True — for keys that are IN that object. With + // `fields:{}`, or a `fields` carrying some OTHER key, nothing + // canonicalizes `field:["status = done"]` and it reaches the doors + // padded exactly as it does with no `fields` at all. + // + // Third round running that I generalized a property verified on one + // subset to the whole: round 15 (a premise true of declared params, + // applied to the compat IDs), round 16's first draft (a check placed + // below the exemption so it covered one key class), and now a + // per-key property read as per-request. The predicate is now the + // actual question — will anything canonicalize THIS key. + canonicalized := false + for _, c := range contribs { + if _, inObj := obj[c.key]; inObj { + canonicalized = true + break + } + } + if !canonicalized { + for i := 1; i < len(contribs); i++ { + a, b := contribs[0], contribs[i] + if a.nonCanonical || b.nonCanonical { + return errStructured(prefix, fmt.Errorf( + "%s conflicts with %s — the field entry is not in canonical key=value form, so the transports would write different keys; remove the padding or pass only one of them", + a.source, b.source)) + } + } + } + + // ...and the exemption holds only where the doors PROVABLY agree, + // which is not everywhere (codex round 15). + // + // For a schema-declared param the CLI has a real flag, so stdio + // receives BOTH forms (`--status open --field status=done`) and its + // overlay order resolves them exactly as the HTTP mapper does — the + // premise the round-7 boundary rests on, pinned per door. + // + // The v0.16 compat IDs are the exception, and being undeclared is + // precisely why: BuildCLIArgs emits the CLI's real flags, and there + // is no flag behind `assigned_user_id`, so the top-level value is + // DROPPED and stdio sees only the field entry — while HTTP reads the + // top-level param. Same call, two different people assigned, with no + // `fields` object anywhere. Last-write-wins cannot be the answer when + // the two doors do not receive the same writes. + // + // I generalised the round-7 premise from the params I had verified to + // the two whose whole nature is being unverifiable that way. This is + // the narrowing. + // PER-KEY HERE TOO (codex round 19). Round 17 made the padded gate + // per-key and left this one per-request — the same mistake in the + // sibling gate, one round later. With `fields:{"other":"x"}` and + // `field:["effort=l","effort=s"]`, `effort` is not in the object, + // nothing arbitrates it but the doors themselves, and both keep the + // last entry — so refusing it was a false refusal on a call that + // resolves deterministically. + // + // `canonicalized` is exactly the right question and is already + // computed above: is THIS key carried by the `fields` object. + // THE COMPAT EXCEPTION TURNS ON A TOP-LEVEL VALUE BEING PRESENT, not + // on the key being a compat one (codex round 20). + // + // Round 15's reason was specific: a top-level compat param has no CLI + // flag, so BuildCLIArgs DROPS it while HTTP reads it, and the doors + // receive different writes. That asymmetry exists only for the + // top-level form. Two `field` entries naming a compat key carry no + // such thing — both doors keep the last and lift the same column — so + // refusing them was a false refusal, the reason applied past the + // source it was verified on. + compatTopLevel := false + for _, c := range contribs { + if c.topLevel && compatIDFieldKeys[c.key] { + compatTopLevel = true + break + } + } + if !canonicalized && !compatTopLevel { + continue + } + for i := 1; i < len(contribs); i++ { + a, b := contribs[0], contribs[i] + if a.nested && b.nested { + // TWO STRUCTURES: equal ones collapse like any other equal + // duplicate (codex round 14). Refusing them unconditionally + // was a regression the restructure introduced — `tags:["a"]` + // plus `fields:{"tags":["a"]}` is one unambiguous value, and + // scalarEqual had always collapsed it before. + if !scalarEqual(a.raw, b.raw) { + return errStructured(prefix, fmt.Errorf( + "%s conflicts with %s — pass one of them, or the same value in both", a.source, b.source)) + } + continue + } + if a.nested || b.nested { + return errStructured(prefix, fmt.Errorf( + "%s conflicts with %s — one key cannot be both a structured value and a string", a.source, b.source)) + } + if a.value != b.value { + return errStructured(prefix, fmt.Errorf( + "%s conflicts with %s (%s vs %s) — pass one of them, or the same value in both", + a.source, b.source, a.value, b.value)) + } + } + } + return nil +} + +// topLevelValueProvided reports whether a top-level param VALUE counts as +// supplied for the duplicate checks below (codex round 12). +// +// An empty string does not. That is the convention every schema-declared +// string on this tool follows — promotedParamValue treats "" as absent, the +// CLI's `status != ""` guards do, and `assign: ""` is documented as inert — +// so a client that zero-fills its optional params was being REFUSED for +// asking one question, on every promoted key at once. +// +// Round 10 fixed exactly this for the hierarchy keys and I never asked +// whether the same reasoning covered their siblings; it did, for all six. +// CONVE-18: the reviewer names an instance, the fix owes the population. The +// probe that drove all eight keys is what turned one named key into six — +// and, below, into the two that are NOT part of the class. +// +// THE COMPAT ID KEYS ARE EXCLUDED. For `assigned_user_id` / `agent_role_id` +// an empty string is not absence — it is a CLEAR to NULL, the deliberate +// v0.16 semantics (see dispatch_http_advanced.go, which forwards "" verbatim +// for exactly these two). So a blank there IS an effective directive and DOES +// conflict with a non-empty `fields` value, the same way `field:["parent="]` +// does. Applying the round-12 finding uniformly would have discarded a clear +// in favour of the `fields` value — a spurious refusal traded for a silent +// wrong write, which is the worse half of the trade. +// +// Both call sites consult this, including the compat block that the carve-out +// is FOR. That is not decoration: when only the promoted block called it, the +// carve-out was unreachable and a mutant deleting it survived every test. +func topLevelValueProvided(key string, v any) bool { + // NIL IS ABSENCE ON EVERY DOOR AND FOR EVERY KEY (codex round 21). + // + // Checked BEFORE the compat carve-out, which returned true + // unconditionally and so counted `assigned_user_id: null` as a supplied + // value. Nothing writes a nil: the HTTP mapper's `.(string)` assertion + // fails and drops it, and BuildCLIArgs has no flag value to emit. So a + // nil beside a `fields` entry for the same key was a false refusal on a + // call both doors resolve to the `fields` value. + // + // The finding named the compat pair; the probe found all five top-level + // keys behaving identically, because the non-compat path fell through to + // `return true` as well. Fixing only the named pair would have left + // `status: null` refusing — the same instance-versus-population trap this + // unit keeps setting. + if v == nil { + return false + } + if compatIDFieldKeys[key] { + return true // "" is a clear here, not an absence + } + if str, isString := v.(string); isString && str == "" { + return false + } + return true +} + +// compatIDFieldKeys are the v0.16 remote-transport compat params. They are +// deliberately never schema-declared (see version.go), which is exactly why +// they need naming here: nothing else in this file knows they can arrive at +// the top level. +var compatIDFieldKeys = map[string]bool{ + "assigned_user_id": true, + "agent_role_id": true, +} + var padItemPromotedFieldKeys = map[string]bool{ "status": true, "priority": true, @@ -78,6 +552,12 @@ var padItemPromotedFieldKeys = map[string]bool{ // into the dedicated-param + `field`-array paths, then dispatch as // before. Calls without `fields` are byte-for-byte unchanged. func actionItemCreate(ctx context.Context, input map[string]any, env ActionEnv) (*mcp.CallToolResult, error) { + // ONE conflict decision, over the canonical view of every source, run + // whether or not a `fields` object is present (lead ruling after round + // 13; reach corrected after round 14). + if errRes := detectFieldConflicts("pad_item.create", input); errRes != nil { + return errRes, nil + } out, errRes := reshapeItemFields("pad_item.create", input) if errRes != nil { return errRes, nil @@ -86,6 +566,12 @@ func actionItemCreate(ctx context.Context, input map[string]any, env ActionEnv) } func actionItemUpdate(ctx context.Context, input map[string]any, env ActionEnv) (*mcp.CallToolResult, error) { + // ONE conflict decision, over the canonical view of every source, run + // whether or not a `fields` object is present (lead ruling after round + // 13; reach corrected after round 14). + if errRes := detectFieldConflicts("pad_item.update", input); errRes != nil { + return errRes, nil + } out, errRes := reshapeItemFields("pad_item.update", input) if errRes != nil { return errRes, nil @@ -139,6 +625,58 @@ func reshapeItemFields(prefix string, input map[string]any) (map[string]any, *mc return nil, errRes } + // The ORIGINAL top-level hierarchy params, snapshotted before the loop + // starts writing promoted keys into `out` (codex round 9). + // + // The alias check below reads this rather than `out`. Reading `out` made + // the guard work by ACCIDENT for a pair arriving wholly inside `fields`: + // keys are processed in sorted order, so `parent` was promoted into + // out["parent"] and `plan` then collided with it. Right answer, wrong + // mechanism — it says "conflicts with the top-level parent param" to a + // caller who passed no such param, and it would evaporate the day + // `parent` left padItemPromotedFieldKeys. The fields-vs-fields case is + // now checked directly against `obj`, and this snapshot keeps the + // top-level message honest. + origHierarchyParams := map[string]any{} + for _, alias := range hierarchyAliasKeys { + v, ok := input[alias] + if !ok { + continue + } + // AN EMPTY STRING IS NOT PROVIDED (codex round 10). Every declared + // string param on this tool follows that convention — it is why + // promotedParamValue treats "" as absent and why `assign: ""` is + // deliberately inert. Counting it here made `parent: ""` collide with + // a perfectly good `fields.plan`, refusing a call that asks for one + // hierarchy directive and passes the other as a padded-out zero + // value, which is exactly what a client that fills every declared + // optional param does. + // + // NOT the same as an empty value in the `field` ARRAY or in `fields`: + // `field:["parent="]` and `fields:{"parent":""}` are the documented + // CLEAR signal (BUG-2013 / BUG-2078), so they are semantically + // effective and stay conflicts. The asymmetry is real and load- + // bearing: one is a param left blank, the other is an explicit + // instruction that happens to look like one. + if !topLevelValueProvided(alias, v) { + continue + } + origHierarchyParams[alias] = v + } + + // The same values with their JSON types intact (BUG-2850). + fieldsNative := map[string]any{} + + // Promoted keys whose array entry is an equal duplicate and is therefore + // removed from `field` before dispatch, so the value is written once + // through its dedicated param (codex round 6). + dropFieldKeys := map[string]bool{} + + // Generic keys whose array entry was a PADDED equal duplicate: the raw + // entry is dropped and re-emitted in canonical `key=value` form, so the + // doors that do not trim write the key the caller meant (codex round 7). + reEmitFields := map[string]string{} + // Deterministic processing (and error ordering) across runs. keys := make([]string, 0, len(obj)) for k := range obj { @@ -157,14 +695,114 @@ func reshapeItemFields(prefix string, input map[string]any) (map[string]any, *mc if strings.Contains(k, "=") { return nil, errStructured(prefix, fmt.Errorf("fields.%q: field keys cannot contain '='", k)) } + // THESE TWO GUARDS RUN BEFORE THE PROMOTED BRANCH, and that ordering + // is the fix, not a detail (codex round 4). They were below it, so + // `tags: null` and `parent: 42` reached the promoted path and skipped + // both: the null was converted into a silent no-op, and a numeric + // parent was dropped later by the handler. A guard that a whole class + // of keys walks around is not a guard. + // NULL stays refused (BUG-2850 lifts objects and arrays, not this). + // A null in a fields map has no agreed meaning — "store JSON null" and + // "clear this field" are both readable from it, and Pad already has an + // explicit clear vocabulary (clear_parent, clear_assigned_user). Giving + // null a silent meaning here would be inventing semantics inside a bug + // fix; if a clear-by-null is ever wanted it should be ruled and named. + if v == nil { + return nil, errStructured(prefix, fmt.Errorf( + "fields.%s: null has no defined write semantics — omit the key to leave it unchanged", k)) + } + + // HIERARCHY PSEUDO-KEYS TAKE A STRING REF, ALWAYS (BUG-2850, codex + // round 3). `plan` is not in padItemPromotedFieldKeys, so before this + // it fell to the generic path and — once structures stopped being + // refused — a `fields:{"plan":{…}}` reached the server natively. There + // `extractParentLink` reads any PRESENT non-string plan/parent as a + // hierarchy directive, drops the key, and on update CLEARS the item's + // existing parent link. So lifting the nested refusal quietly opened a + // path where a malformed value silently detaches an item from its + // parent. A structure has no meaning here at all: the only value these + // keys take is a ref. + if hierarchyPseudoFieldKeys[k] { + // AN EMPTY HIERARCHY VALUE HERE IS A SILENT NO-OP, so it is + // refused rather than accepted (codex round 11). + // + // `fields:{"parent":""}` promotes onto the top-level `parent` + // param, where BOTH doors then treat empty as NOT PROVIDED — + // promotedParamValue on the remote side, the `parentRef != ""` + // guard in cmd_item.go on the CLI side. So the call reported + // success and detached nothing. That is the exact failure mode + // this bug exists to remove, and it is worse here than elsewhere + // because the caller's intent (detach) is unambiguous. + // + // Refused, not silently promoted to a clear: giving this door + // clear semantics is a decision about what `fields` MEANS, and + // v0.19 already made clear_parent the canonical detach precisely + // so that an empty string would not have to carry it. Inventing + // the semantics inside a bug fix is what the `null` refusal above + // declines to do, for the same reason. The message names both + // working forms so the caller is not merely blocked. + if str, isString := v.(string); isString && str == "" { + return nil, errStructured(prefix, fmt.Errorf( + "fields.%s: an empty value here is silently ignored, not a detach — use clear_parent: true, or field: [\"%s=\"] if you want the raw fields_patch form", k, k)) + } + if _, isString := v.(string); !isString { + return nil, errStructured(prefix, fmt.Errorf( + "fields.%s must be a string ref (e.g. %q) — a %T here would be read as a hierarchy directive and could detach the item", k, "PLAN-12", v)) + } + } + + // IDENTITY-REFERENCE KEYS TAKE A STRING, ALWAYS (codex round 9). + // `assign` and `role` name a person or a role — a slug, an email, a + // UUID. A number has no meaning for either, and the two doors + // disagreed about what to do with one: the HTTP dispatcher's + // `rawAssign.(string)` turns it into "" and treats it as NOT + // PROVIDED, silently dropping the write, while stdio emits + // `--assign 123` and the CLI fails loudly on the lookup. Same call, + // one door silent and one door red. + // + // Refused here, at the door-independent layer, rather than taught to + // each dispatcher — that is what stops them drifting again. Note this + // does NOT walk back round 6's decision to accept non-string promoted + // values generally: `priority` may legitimately be a number in a + // custom schema, and create has always passed such values through. + // These two keys are references, not values. + // THE v0.16 COMPAT ID PARAMS CONFLICT LIKE ANY OTHER KEY (codex + // round 11). `assigned_user_id` / `agent_role_id` are accepted at the + // top level as a documented, never-schema-declared compat form, so + // they are invisible to padItemPromotedFieldKeys and took the generic + // path — where the conflict check only looks at the `field` array. + // With `assigned_user_id:"A"` plus `fields:{"assigned_user_id":"B"}` + // the doors then disagreed outright: the remote mapper reads the + // top-level A while stdio emits only `--field assigned_user_id=B`, + // because the top-level form has no CLI flag behind it. One call, + // two different people assigned. + if compatIDFieldKeys[k] { + // Conflicts are already decided; an equal duplicate just collapses + // so exactly one form reaches dispatch. + if existing, has := out[k]; has && topLevelValueProvided(k, existing) { + delete(out, k) + } + } + + if identityRefFieldKeys[k] { + if _, isString := v.(string); !isString { + return nil, errStructured(prefix, fmt.Errorf( + "fields.%s must be a string (a slug, email, or id) — a %T is silently dropped by one transport and rejected by the other", k, v)) + } + } + if padItemPromotedFieldKeys[k] { - existing, has := out[k] - if has { - if !scalarEqual(existing, v) { - return nil, errStructured(prefix, fmt.Errorf( - "fields.%s conflicts with the top-level %s param (%v vs %v) — pass one of them, or the same value in both", k, k, v, existing)) - } - continue // equal duplicate: unambiguous, already applied + // Conflicts are decided; what is left here is EMISSION. An equal + // duplicate in the field array is dropped so the value is written + // once, through the dedicated param. + if _, inArray := fieldByKey[k]; inArray { + dropFieldKeys[k] = true + delete(fieldByKey, k) + } + if existing, has := out[k]; has && !topLevelValueProvided(k, existing) { + delete(out, k) + } else if has { + continue // equal duplicate, already applied } // tags promotes the native array; everything else is scalar. if k != "tags" { @@ -175,14 +813,33 @@ func reshapeItemFields(prefix string, input map[string]any) (map[string]any, *mc out[k] = v continue } + // NATIVE FORM, ALWAYS (BUG-2850). The value goes into fieldsNative + // with its JSON type intact — a number stays a number, an object stays + // an object. Whether a door can USE that depends on the door, which is + // why the string form below is still emitted alongside it rather than + // replaced. + fieldsNative[k] = v + sv, err := stringifyFieldValue(v) if err != nil { - return nil, errStructured(prefix, fmt.Errorf("fields.%s: %s", k, err)) + // A nested value (object/array/null). It has no `key=value` + // encoding, so it cannot join fieldEntries — but it is no longer + // refused here: the native map above carries it for doors that can + // express it, and BuildCLIArgs refuses it for the stdio door that + // cannot (BUG-2850, lifting PR #1159's blanket refusal now that + // server-side coercion exists). A conflict with an existing + // `field` entry is still a conflict: one key cannot be both a + // string and a structure. + // A structure has no key=value encoding; the native map carries it. + continue } - if prev, has := fieldByKey[k]; has { - if prev != sv { - return nil, errStructured(prefix, fmt.Errorf( - "fields.%s conflicts with the field array entry %q (%s vs %s) — pass one of them, or the same value in both", k, k+"="+prev, sv, prev)) + if _, has := fieldByKey[k]; has { + // Known equal (the pass above refused anything else). Re-emit + // canonically when the retained entry is padded, so every door + // writes the key the caller meant. + if hasNonCanonicalFieldEntry(fieldEntries, k, sv) { + dropFieldKeys[k] = true + reEmitFields[k] = sv } continue } @@ -190,12 +847,85 @@ func reshapeItemFields(prefix string, input map[string]any) (map[string]any, *mc fieldByKey[k] = sv } + // Remove the equal-duplicate entries the promoted branch claimed. Matched + // on the NORMALIZED key, the same form parseFieldArray indexed on, so + // `field:[" role=implementer"]` is dropped by the same rule that let it + // be recognized as a duplicate in the first place. + if len(dropFieldKeys) > 0 { + kept := fieldEntries[:0] + for _, e := range fieldEntries { + if key, _, ok := strings.Cut(e, "="); ok && dropFieldKeys[strings.TrimSpace(key)] { + continue + } + kept = append(kept, e) + } + fieldEntries = kept + } + // Canonical re-emissions go on AFTER the filter, or the filter would + // remove them again — they carry the same key it just matched on. + if len(reEmitFields) > 0 { + reKeys := make([]string, 0, len(reEmitFields)) + for k := range reEmitFields { + reKeys = append(reKeys, k) + } + sort.Strings(reKeys) // deterministic arg order across runs + for _, k := range reKeys { + fieldEntries = append(fieldEntries, k+"="+reEmitFields[k]) + } + } if len(fieldEntries) > 0 { out["field"] = fieldEntries + } else { + // An entry set emptied by the drop above must not leave the caller's + // original `field` array in place — that is the value we just decided + // not to send twice. + delete(out, "field") + } + // Emitted under a distinct key so each transport takes what it can use: + // the HTTP mapper prefers these (types intact), BuildCLIArgs consults them + // only to refuse the nested values the CLI cannot express. Both forms + // describe the same input, so a door reading either is correct — they + // differ only in fidelity. + if len(fieldsNative) > 0 { + out[fieldsNativeKey] = fieldsNative } return out, nil } +// fieldsNativeKey is the dispatch-input key carrying the `fields` object with +// JSON types intact (BUG-2850). Deliberately not a name a caller could send: +// it is produced by this merge, never accepted from the wire, and strict input +// validation would reject it as an undeclared param if it were. +const fieldsNativeKey = "__fields_native" + +// hasNonCanonicalFieldEntry reports whether ANY entry for this key is written +// in something other than the canonical `key=value` form. +// +// "Any", not "no canonical entry exists" (codex round 8). The first version +// asked whether a canonical entry was PRESENT and left the array alone if one +// was — so `field:["effort=l", " effort=l"]` kept the padded twin, and the +// doors then disagreed about it: HTTP trims and writes `effort`, the CLI does +// not and writes an undeclared `" effort"`. One canonical entry does not make +// its padded sibling harmless; every entry for the key has to be canonical, +// or the key gets re-emitted once and cleanly. +// +// Collapsing duplicates in that re-emission is correct rather than lossy: +// parseFieldArray already indexes them to a single value, so two entries for +// one key were never two writes. +func hasNonCanonicalFieldEntry(entries []string, key, value string) bool { + want := key + "=" + value + for _, e := range entries { + k, _, ok := strings.Cut(e, "=") + if !ok || strings.TrimSpace(k) != key { + continue + } + if e != want { + return true + } + } + return false +} + // parseFieldArray normalizes an existing `field` param into a []string // plus a key→value index. Entries the CLI would reject anyway (no '=') // are passed through unindexed rather than pre-empting the CLI's own @@ -225,7 +955,23 @@ func parseFieldArray(prefix string, raw any) ([]string, map[string]string, *mcp. } for _, e := range entries { if k, val, ok := strings.Cut(e, "="); ok { - byKey[k] = val + // NORMALIZED THE WAY THE DOOR WILL NORMALIZE (codex round 6). + // ingestFieldKVP (dispatch_http.go) TrimSpaces both halves before + // storing them, so an un-trimmed index here does not describe what + // the remote door is about to write: `field:[" status=cancelled"]` + // indexed under " status" missed every conflict check against + // `fields:{"status":…}` and then silently overrode it. Trimming + // the value closes the mirror-image false refusal, where + // `field:["status= done"]` looked different from "done" and was + // refused as a conflict with a call that agrees. + // + // Only the INDEX is normalized. `entries` stays verbatim so every + // door still parses exactly what the caller sent — the CLI door + // does not trim (cmd/pad/cmd_item.go), and this must not quietly + // change what it receives. The effect is a conflict check that is + // conservative on both doors, which is the correct direction for + // a guard whose disposition is refuse-on-ambiguity. + byKey[strings.TrimSpace(k)] = strings.TrimSpace(val) } } return entries, byKey, nil diff --git a/internal/mcp/catalog_item_fields_test.go b/internal/mcp/catalog_item_fields_test.go index c5984d1de..999c3d579 100644 --- a/internal/mcp/catalog_item_fields_test.go +++ b/internal/mcp/catalog_item_fields_test.go @@ -2,6 +2,7 @@ package mcp import ( "context" + "sort" "strings" "testing" @@ -223,34 +224,100 @@ func TestPadItemUpdate_FieldsNonObjectRefused(t *testing.T) { } } -// TestPadItemUpdate_FieldsNestedOrNullValueRefused: field values are -// scalars; a nested object/array or an explicit null has no defined -// write semantics and is refused with the offending key named. -func TestPadItemUpdate_FieldsNestedOrNullValueRefused(t *testing.T) { - for name, bad := range map[string]any{ +// TestPadItemUpdate_FieldsNullRefused: a null field value still has no +// defined write semantics and is refused with the key named. +// +// BUG-2850 lifted PR #1159's refusal for nested OBJECTS and ARRAYS — see the +// test below — but deliberately NOT for null. "Store JSON null" and "clear +// this field" are both readable from it, and Pad has an explicit clear +// vocabulary already (clear_parent, clear_assigned_user). Giving null a silent +// meaning inside a bug fix would be inventing semantics. +func TestPadItemUpdate_FieldsNullRefused(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{"meta": nil}, + }) + if !isErr { + t.Fatalf("expected refusal, got success: %s", msg) + } + if !strings.Contains(msg, "meta") { + t.Errorf("error should name the offending key: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } +} + +// TestPadItemUpdate_FieldsNestedValuesReachTheDispatcher: a nested object or +// array is no longer refused on the way to the dispatcher (BUG-2850). +// +// PR #1159 refused these in the CATALOG, for every transport, on the grounds +// that they had no defined write semantics. They do now — the merge carries +// them natively and the remote /mcp door writes them, which is what makes a +// json-typed field (a playbook's `arguments`) writable at all. +// +// THIS TEST EXISTS BECAUSE THE FIRST FIX PUT THE REFUSAL IN THE WRONG PLACE. +// It went into BuildCLIArgs, which env.Dispatch runs for BOTH transports, so +// it blocked the remote door too and the native handling was never reached — +// codex round 2 [P1], and a binding I had tested at the component (mapItemCreate +// directly) rather than through dispatch. The refusal now lives in +// ExecDispatcher, the door that actually cannot encode a structure, and this +// asserts that everything else gets through. +func TestPadItemUpdate_FieldsNestedValuesReachTheDispatcher(t *testing.T) { + for name, nested := range map[string]any{ "nested object": map[string]any{"meta": map[string]any{"a": 1}}, "nested array": map[string]any{"meta": []any{"a"}}, - "null value": map[string]any{"meta": nil}, } { t.Run(name, func(t *testing.T) { disp, msg, isErr := dispatchPadItem(t, map[string]any{ "action": "update", "ref": "TASK-5", - "fields": bad, + "fields": nested, }) - if !isErr { - t.Fatalf("expected refusal, got success: %s", msg) - } - if !strings.Contains(msg, "meta") { - t.Errorf("error should name the offending key: %s", msg) + if isErr { + t.Fatalf("nested values must reach the dispatcher since BUG-2850: %s", msg) } - if len(disp.gotPath) != 0 { - t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") } }) } } +// The stdio door's own refusal, at the door (BUG-2850). Named separately from +// the dispatch test above because they are different claims: one is that the +// remote transport is unblocked, the other that the CLI transport still says +// no — and the first fix conflated them. +func TestRefuseStructuredFieldsOverCLI(t *testing.T) { + err := refuseStructuredFieldsOverCLI(map[string]any{ + fieldsNativeKey: map[string]any{ + "scalar": "fine", + "obj": map[string]any{"a": 1}, + "arr": []any{"a"}, + }, + }) + if err == nil { + t.Fatal("expected a refusal for structured values over the CLI transport") + } + for _, want := range []string{"obj", "arr", "stdio"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("refusal should mention %q: %v", want, err) + } + } + // The message must not read as a verdict on the data — the reporter's + // agent rewrote seven playbooks after taking the old one that way. + if strings.Contains(err.Error(), "no defined write semantics") { + t.Errorf("refusal should name the transport limit, not the value: %v", err) + } + // A scalar-only native map is not a refusal. + if err := refuseStructuredFieldsOverCLI(map[string]any{ + fieldsNativeKey: map[string]any{"scalar": "fine"}, + }); err != nil { + t.Errorf("scalars are encodable as key=value; got refusal: %v", err) + } +} + // TestPadItemList_FieldsRefused: `fields` is a create/update writer. On // any other action it would be silently dropped by BuildCLIArgs — the // exact failure mode this contract removes — so it is refused loudly. @@ -361,3 +428,1698 @@ func TestPadItemUpdate_FieldsEmptyKeyRefused(t *testing.T) { t.Errorf("must not dispatch; dispatched %v", disp.gotPath) } } + +// A structured `plan` or `parent` must be refused before dispatch (BUG-2850, +// codex round 3). +// +// These keys are hierarchy DIRECTIVES to the server, not ordinary fields: +// extractParentLink reads any present non-string value as one, drops the key, +// and on update clears the item's existing parent link. Lifting the nested +// refusal opened that path — a malformed value would silently detach an item +// from its parent — so the guard is specific to these keys rather than a +// return to refusing structures everywhere. +func TestPadItemUpdate_StructuredHierarchyKeyRefused(t *testing.T) { + for _, key := range []string{"plan", "parent"} { + t.Run(key, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{key: map[string]any{"id": "PLAN-12"}}, + }) + if !isErr { + t.Fatalf("a structured %s must be refused, got success: %s", key, msg) + } + if !strings.Contains(msg, key) { + t.Errorf("refusal should name the key: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// ...but a string ref still works, so the guard did not re-refuse the normal +// case while closing the structured one. +func TestPadItemUpdate_StringHierarchyKeyStillAccepted(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{"plan": "PLAN-12"}, + }) + if isErr { + t.Fatalf("a string plan ref must still be accepted: %s", msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } +} + +// The guards must apply to PROMOTED keys too (BUG-2850, codex round 4). +// +// `tags`, `parent`, `status` and friends are handled by an earlier branch that +// promotes them onto dedicated top-level params. The null and hierarchy guards +// were written below that branch, so every promoted key walked around both: +// `tags: null` became a silent no-op instead of the documented refusal, and +// `parent: 42` was accepted here and dropped later by the handler. A guard a +// whole class of keys bypasses is not a guard, which is why these cases are +// pinned separately from the generic-path ones above. +func TestPadItemUpdate_GuardsApplyToPromotedKeys(t *testing.T) { + cases := map[string]map[string]any{ + "null tags": {"tags": nil}, + "null status": {"status": nil}, + "numeric parent": {"parent": float64(42)}, + "bool parent": {"parent": true}, + } + for name, fields := range cases { + t.Run(name, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": fields, + }) + if !isErr { + t.Fatalf("expected refusal, got success: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// ...and the promoted keys still work with well-formed values, so hoisting the +// guards did not break promotion itself. +func TestPadItemUpdate_PromotedKeysStillPromote(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{"status": "done", "tags": []any{"a", "b"}}, + }) + if isErr { + t.Fatalf("well-formed promoted keys must still be accepted: %s", msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } +} + +// TestPadItemUpdate_FieldArrayConflictAppliesToPromotedKeys: the +// `fields` vs `field: ["k=v"]` conflict guard has to cover PROMOTED keys +// too (codex round 5, BUG-2850). +// +// TestPadItemUpdate_FieldsConflictInFieldArrayRefused above proves the +// guard for `effort` — a key that takes the GENERIC path, where the +// fieldByKey check lives. Every promoted key (`status`, `parent`, `role`, +// …) returns from the promoted branch before reaching it, so the guard +// that test vouches for was never on their path. That is CONVE-19 in the +// same unit for the fourth round running: the earlier test binds the +// generic path, not the class of keys that skips it. +// +// Unrefused, the ambiguity does not fail closed — it silently picks the +// `field` entry, because the promoted branch writes the top-level param +// (`out["status"]`) while the array stays in `out["field"]`, and both +// mapItemCreate/mapItemUpdate and the CLI overlay `--field` entries AFTER +// the named flags. So `fields:{"status":"done"}` with +// `field:["status=cancelled"]` cancels the item. On `parent` the same +// shape relinks or detaches it. +func TestPadItemUpdate_FieldArrayConflictAppliesToPromotedKeys(t *testing.T) { + cases := map[string]struct { + field []any + fields map[string]any + key string + }{ + "status": {[]any{"status=cancelled"}, map[string]any{"status": "done"}, "status"}, + "parent": {[]any{"parent=PLAN-9"}, map[string]any{"parent": "PLAN-12"}, "parent"}, + "role": {[]any{"role=reviewer"}, map[string]any{"role": "implementer"}, "role"}, + // tags is the structured promoted key: one key cannot be both an + // array and a string, which is a conflict for the same reason. + "tags": {[]any{"tags=a"}, map[string]any{"tags": []any{"b"}}, "tags"}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": tc.field, + "fields": tc.fields, + }) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s", msg) + } + if !strings.Contains(msg, tc.key) { + t.Errorf("error should name the conflicting key %q: %s", tc.key, msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("conflicting call must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// ...and the equal-duplicate half, so the fix refuses ambiguity rather +// than refusing agreement. A promoted key with the SAME value in both +// places is unambiguous and must still apply, exactly once. +func TestPadItemUpdate_FieldArrayEqualDuplicateOnPromotedKeyAllowed(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{"status=done"}, + "fields": map[string]any{"status": "done"}, + }) + if isErr { + t.Fatalf("expected success for equal duplicate, got error: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--status", "done") { + t.Errorf("status lost: %v", disp.gotArgs) + } +} + +// --- codex round 6 --- + +// TestPadItemUpdate_HierarchyAliasConflictRefused: `parent` and `plan` are +// ONE directive, so a conflict between them has to refuse even though the key +// names differ (BUG-2850, codex round 6). +// +// extractParentLink (handlers_items.go) resolves the link with +// `for _, key := range []string{"parent", "plan"}` and no early exit, so when +// both arrive the LATER key wins. Every conflict guard in reshapeItemFields +// matched on the SAME key name, so `fields:{"parent":"PLAN-12"}` with +// `field:["plan=PLAN-9"]` passed every check and then relinked the item to +// PLAN-9. Same alias bypass BUG-2078's round-1 review found on clear_parent, +// reached through a different door — which is why this pins both directions +// and the top-level param as well. +func TestPadItemUpdate_HierarchyAliasConflictRefused(t *testing.T) { + cases := map[string]map[string]any{ + "fields.parent vs field[plan]": { + "field": []any{"plan=PLAN-9"}, + "fields": map[string]any{"parent": "PLAN-12"}, + }, + "fields.plan vs field[parent]": { + "field": []any{"parent=PLAN-9"}, + "fields": map[string]any{"plan": "PLAN-12"}, + }, + "fields.plan vs top-level parent": { + "parent": "PLAN-9", + "fields": map[string]any{"plan": "PLAN-12"}, + }, + // Equal values are refused too: two hierarchy directives in one call + // are ambiguous by construction, and v0.19 already refuses this shape + // for parent + clear_parent "including via the plan alias". + "equal values still refused": { + "field": []any{"plan=PLAN-12"}, + "fields": map[string]any{"parent": "PLAN-12"}, + }, + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + input := map[string]any{"action": "update", "ref": "TASK-5"} + for k, v := range extra { + input[k] = v + } + disp, msg, isErr := dispatchPadItem(t, input) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s", msg) + } + if !strings.Contains(msg, "parent") || !strings.Contains(msg, "plan") { + t.Errorf("error should name both hierarchy keys: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("conflicting call must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// ...and a lone hierarchy key still works, so the alias guard did not make +// the ordinary case unreachable. +func TestPadItemUpdate_LoneHierarchyKeyStillAccepted(t *testing.T) { + for _, key := range []string{"parent", "plan"} { + t.Run(key, func(t *testing.T) { + _, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{key: "PLAN-12"}, + }) + if isErr { + t.Fatalf("a lone %s must still be accepted: %s", key, msg) + } + }) + } +} + +// TestPadItemUpdate_FieldArrayKeysNormalizedForConflicts: the conflict index +// must be normalized the way the DOOR normalizes (BUG-2850, codex round 6). +// +// ingestFieldKVP (dispatch_http.go) TrimSpaces both halves of a `key=value` +// entry before writing it. parseFieldArray indexed the raw halves, so the +// index described something the remote door was never going to write: +// `field:[" status=cancelled"]` sat under " status", missed the guard against +// `fields:{"status":"done"}`, and then won. The mirror-image case is the +// control leg — a value that differs only by padding is the SAME value, and +// refusing it would be refusing a call that agrees with itself. +func TestPadItemUpdate_FieldArrayKeysNormalizedForConflicts(t *testing.T) { + t.Run("padded key still conflicts", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{" status=cancelled"}, + "fields": map[string]any{"status": "done"}, + }) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("conflicting call must not dispatch; dispatched %v", disp.gotPath) + } + }) + t.Run("padded value is not a conflict", func(t *testing.T) { + _, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{"status= done"}, + "fields": map[string]any{"status": "done"}, + }) + if isErr { + t.Fatalf("padding is not a disagreement; expected success, got: %s", msg) + } + }) +} + +// TestPadItemUpdate_EqualPromotedDuplicateAppliesOnce: an equal duplicate on a +// promoted key must be written ONCE, through its dedicated param — the array +// entry has to be removed (BUG-2850, codex round 6). +// +// Leaving it produced two writes of one value by two mechanisms: `role` was +// resolved to agent_role_id AND written as a literal `role` key into the +// fields blob, which no schema declares — so an equal-duplicate call silently +// created an undeclared field and (since dc3fc2d5) a warning naming it. The +// assertion is on the ABSENCE of the --field pair, because that is the half +// that was wrong; a status-code check would pass either way. +func TestPadItemUpdate_EqualPromotedDuplicateAppliesOnce(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{"status=done"}, + "fields": map[string]any{"status": "done"}, + }) + if isErr { + t.Fatalf("equal duplicate must be accepted: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--status", "done") { + t.Errorf("the value must still be applied through its param: %v", disp.gotArgs) + } + if argsContainPair(disp.gotArgs, "--field", "status=done") { + t.Errorf("the duplicate --field entry must be dropped, not sent alongside: %v", disp.gotArgs) + } +} + +// ...and an unrelated `field` entry alongside an equal duplicate survives, so +// the drop removes exactly the duplicate and not the array. +func TestPadItemUpdate_EqualDuplicateDropKeepsOtherFieldEntries(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{"status=done", "effort=l"}, + "fields": map[string]any{"status": "done"}, + }) + if isErr { + t.Fatalf("expected success: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "effort=l") { + t.Errorf("unrelated field entry lost: %v", disp.gotArgs) + } + if argsContainPair(disp.gotArgs, "--field", "status=done") { + t.Errorf("duplicate entry survived: %v", disp.gotArgs) + } +} + +// --- codex round 7 --- + +// TestPadItemUpdate_HierarchyAliasAmbiguityRefusedWithoutFieldsObject: the +// alias guard must fire whichever doors the two hierarchy keys arrive +// through, INCLUDING when no `fields` object is involved at all (BUG-2850, +// codex round 7). +// +// Round 6 put the guard inside reshapeItemFields' per-key loop, and +// reshapeItemFields returns early when `fields` is absent — so +// `field:["parent=A","plan=B"]` walked straight past it and +// extractParentLink's no-early-exit loop applied `plan` while the caller had +// every reason to think `parent` was what they set. A guard a caller can step +// around by moving the same two values into a different param is not a guard, +// which is this unit's recurring finding stated once more. +// +// The pure-`field` form predates BUG-2850, so this closes a pre-existing +// silent mis-write rather than a regression — see the note on +// checkHierarchyAliasAmbiguity for why it is fixed here rather than filed. +func TestPadItemUpdate_HierarchyAliasAmbiguityRefusedWithoutFieldsObject(t *testing.T) { + cases := map[string]map[string]any{ + "both aliases in the field array, no fields object": { + "field": []any{"parent=PLAN-9", "plan=PLAN-12"}, + }, + "both aliases in the field array, unrelated fields object": { + "field": []any{"parent=PLAN-9", "plan=PLAN-12"}, + "fields": map[string]any{"effort": "l"}, + }, + "top-level parent param vs field[plan], no fields object": { + "parent": "PLAN-9", + "field": []any{"plan=PLAN-12"}, + }, + "padded entries still caught": { + "field": []any{" parent=PLAN-9", "plan =PLAN-12"}, + }, + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + input := map[string]any{"action": "update", "ref": "TASK-5"} + for k, v := range extra { + input[k] = v + } + disp, msg, isErr := dispatchPadItem(t, input) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s", msg) + } + if !strings.Contains(msg, "parent") || !strings.Contains(msg, "plan") { + t.Errorf("error should name both hierarchy keys: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("ambiguous call must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// ...and ONE hierarchy key through the array is still an ordinary call. The +// guard refuses ambiguity, not the feature. +func TestPadItemUpdate_SingleHierarchyKeyInFieldArrayStillAccepted(t *testing.T) { + for _, entry := range []string{"parent=PLAN-12", "plan=PLAN-12"} { + t.Run(entry, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{entry}, + }) + if isErr { + t.Fatalf("a lone %q must still be accepted: %s", entry, msg) + } + if !argsContainPair(disp.gotArgs, "--field", entry) { + t.Errorf("entry lost: %v", disp.gotArgs) + } + }) + } +} + +// TestPadItemUpdate_PaddedEqualDuplicateIsCanonicalized: a padded equal +// duplicate must be re-emitted in canonical form, not retained raw +// (BUG-2850, codex round 7). +// +// Round 6 normalized the conflict INDEX so ` effort=l` matches +// `fields:{"effort":"l"}` — correct, and it stopped the padded-key bypass. +// But the raw entry stayed in `field`, and the CLI door does not trim: stdio +// would store an undeclared `" effort"` key and leave `effort` untouched. The +// normalization that made the duplicate visible is what made the retained +// entry wrong, so the fix belongs at the same place. +// +// Asserted on the emitted args rather than on success, because the call +// succeeded before the fix too — it just wrote the wrong key. +func TestPadItemUpdate_PaddedEqualDuplicateIsCanonicalized(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{" effort=l"}, + "fields": map[string]any{"effort": "l"}, + }) + if isErr { + t.Fatalf("padding is not a disagreement; expected success, got: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "effort=l") { + t.Errorf("the entry must be re-emitted canonically: %v", disp.gotArgs) + } + if argsContainPair(disp.gotArgs, "--field", " effort=l") { + t.Errorf("the padded entry must not survive — the CLI door does not trim it: %v", disp.gotArgs) + } + count := 0 + for i := 0; i+1 < len(disp.gotArgs); i++ { + if disp.gotArgs[i] == "--field" { + count++ + } + } + if count != 1 { + t.Errorf("--field emitted %d times, want exactly 1: %v", count, disp.gotArgs) + } +} + +// ...and an already-canonical equal duplicate is left exactly as it was, so +// the re-emission does not churn a well-formed array. +func TestPadItemUpdate_CanonicalEqualDuplicateIsUntouched(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{"effort=l", "cost=3"}, + "fields": map[string]any{"effort": "l"}, + }) + if isErr { + t.Fatalf("expected success: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "effort=l") || + !argsContainPair(disp.gotArgs, "--field", "cost=3") { + t.Errorf("both entries must survive unchanged: %v", disp.gotArgs) + } + count := 0 + for i := 0; i+1 < len(disp.gotArgs); i++ { + if disp.gotArgs[i] == "--field" { + count++ + } + } + if count != 2 { + t.Errorf("--field emitted %d times, want 2: %v", count, disp.gotArgs) + } +} + +// --- codex round 8 --- + +// TestPadItemUpdate_MixedCanonicalAndPaddedDuplicatesCollapse: one canonical +// entry does not make its padded twin harmless (BUG-2850, codex round 8). +// +// Round 7's canonicalization asked whether a canonical entry was PRESENT and +// left the array alone if one was — so `field:["effort=l", " effort=l"]` kept +// the padded sibling, and the doors then disagreed: HTTP trims and writes +// `effort`, the CLI does not and writes an undeclared `" effort"`. Transport +// divergence from a call both doors accept, which is the shape this whole +// unit is about. +// +// The key is now re-emitted ONCE, canonically. Collapsing the pair is not +// lossy: parseFieldArray already indexes them to a single value, so two +// entries for one key were never two writes. +func TestPadItemUpdate_MixedCanonicalAndPaddedDuplicatesCollapse(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{"effort=l", " effort=l"}, + "fields": map[string]any{"effort": "l"}, + }) + if isErr { + t.Fatalf("expected success: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "effort=l") { + t.Errorf("the canonical entry must survive: %v", disp.gotArgs) + } + if argsContainPair(disp.gotArgs, "--field", " effort=l") { + t.Errorf("the padded twin must not survive — stdio would store it as an undeclared %q key: %v", " effort", disp.gotArgs) + } + count := 0 + for i := 0; i+1 < len(disp.gotArgs); i++ { + if disp.gotArgs[i] == "--field" { + count++ + } + } + if count != 1 { + t.Errorf("--field emitted %d times, want exactly 1: %v", count, disp.gotArgs) + } +} + +// --- codex round 9 --- + +// TestPadItemUpdate_BothAliasesInOneFieldsObject: the round-9 P1 was REFUTED — +// this pair was already refused — but it was refused by ACCIDENT, and the +// accident is what this pins against. +// +// Keys are processed in sorted order, so `parent` was promoted into +// out["parent"] and `plan` then collided with it one iteration later. Right +// answer, wrong mechanism: the refusal depended on `parent` sorting before +// `plan` AND on `parent` being a promoted key, and it told the caller their +// value conflicted with "the top-level parent param" when they had passed no +// such param. The check now reads the `fields` object directly. +// +// Both orderings are driven because a map literal's order says nothing about +// iteration order, and the error text is asserted because a refusal that +// names a param the caller never sent is a debugging cost even when the +// verdict is right. +func TestPadItemUpdate_BothAliasesInOneFieldsObject(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{"parent": "PLAN-A", "plan": "PLAN-B"}, + }) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("ambiguous call must not dispatch; dispatched %v", disp.gotPath) + } + if !strings.Contains(msg, "fields.parent") || !strings.Contains(msg, "fields.plan") { + t.Errorf("error should name BOTH as fields keys: %s", msg) + } + if strings.Contains(msg, "top-level") { + t.Errorf("no top-level param was passed; the error must not blame one: %s", msg) + } +} + +// ...and the guard must not depend on `parent` being a promoted key. This is +// the mutation the round-9 finding would have become real through: drop +// `parent` from padItemPromotedFieldKeys and the old out[]-based check goes +// silent, because nothing writes out["parent"] any more. +func TestPadItemUpdate_AliasGuardDoesNotDependOnPromotion(t *testing.T) { + if !padItemPromotedFieldKeys["parent"] { + t.Skip("parent is no longer promoted; this test's premise has changed") + } + // `plan` is NOT a promoted key, so a pair where the non-promoted alias is + // the only one that could have populated `out` exercises the direct check. + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{"plan": "PLAN-B", "parent": "PLAN-A"}, + }) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } +} + +// TestPadItemUpdate_NonStringIdentityRefRefused: `assign` and `role` name a +// person or a role, so a number is refused at the door-independent layer +// (BUG-2850, codex round 9 P2). +// +// The two doors disagreed about a numeric value: the HTTP dispatcher's +// `rawAssign.(string)` turns it into "" and treats it as NOT PROVIDED — +// silently dropping the write — while stdio emits `--assign 123` and the CLI +// fails loudly on the lookup. Same call, one door silent and one red. +// +// This does NOT walk back round 6's decision to accept non-string promoted +// values in general; the control leg below is that decision, still standing. +func TestPadItemUpdate_NonStringIdentityRefRefused(t *testing.T) { + for _, key := range []string{"assign", "role"} { + t.Run(key, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{key: float64(123)}, + }) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s (args %v)", msg, disp.gotArgs) + } + if !strings.Contains(msg, key) { + t.Errorf("error should name the key: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// ...and a numeric value on a NON-identity promoted key is still accepted — +// round 6's fix, which a blanket "promoted keys must be strings" rule would +// have silently undone. `priority` can legitimately be a number in a custom +// schema, and create has always passed such values through. +func TestPadItemUpdate_NonStringPriorityStillAccepted(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{"priority": float64(3)}, + }) + if isErr { + t.Fatalf("a numeric priority must still be accepted: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--priority", "3") { + t.Errorf("priority lost: %v", disp.gotArgs) + } +} + +// ...and a STRING assign/role is untouched, so the refusal is about the type +// and not about the keys. +func TestPadItemUpdate_StringIdentityRefStillAccepted(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{"assign": "dave", "role": "implementer"}, + }) + if isErr { + t.Fatalf("string identity refs must still be accepted: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--assign", "dave") || + !argsContainPair(disp.gotArgs, "--role", "implementer") { + t.Errorf("identity refs lost: %v", disp.gotArgs) + } +} + +// --- codex round 10 --- + +// TestPadItemUpdate_EmptyParentParamIsNotAnAliasConflict: an empty top-level +// `parent` is NOT PROVIDED, so it must not collide with `fields.plan` +// (BUG-2850, codex round 10). +// +// Every declared string param on this tool follows that convention — it is +// why promotedParamValue treats "" as absent and why `assign: ""` is +// deliberately inert. Counting it as a hierarchy directive refused a +// perfectly good call from any client that fills declared optional params +// with their zero value rather than omitting them, which is a common client +// shape and exactly who this would have hit. +func TestPadItemUpdate_EmptyParentParamIsNotAnAliasConflict(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "parent": "", + "fields": map[string]any{"plan": "PLAN-12"}, + }) + if isErr { + t.Fatalf("an empty parent param is not a directive; expected success, got: %s", msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } +} + +// ...and the EFFECTIVE empty forms still conflict, because they are not the +// same thing. `field:["parent="]` and `fields:{"parent":""}` are the +// documented CLEAR signal (BUG-2013 / BUG-2078) — an explicit instruction +// that happens to look like a blank param. If this ever goes green, the +// round-10 fix has been over-applied and a clear-plus-set pair is resolving +// silently instead of refusing. +func TestPadItemUpdate_EmptyClearFormsStillConflictWithTheAlias(t *testing.T) { + // NOTE (round 11): the `fields:{"parent":""}` half of this case moved out + // to TestPadItemUpdate_EmptyHierarchyValueInFieldsRefused, because it now + // refuses for a DIFFERENT reason — an empty hierarchy value inside + // `fields` is refused outright as a silent no-op, before any alias check + // runs. Leaving it here would have kept a passing test whose stated + // reason had become false (CONVE-23). What remains is the case this test + // is actually about: the `field` ARRAY clear, which IS an effective + // directive and must still collide with the alias. + cases := map[string]map[string]any{ + "field array clear vs fields.plan": { + "field": []any{"parent="}, + "fields": map[string]any{"plan": "PLAN-12"}, + }, + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + input := map[string]any{"action": "update", "ref": "TASK-5"} + for k, v := range extra { + input[k] = v + } + disp, msg, isErr := dispatchPadItem(t, input) + if !isErr { + t.Fatalf("an explicit clear is a directive and must still conflict; got success: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// --- codex round 11 --- + +// TestPadItemUpdate_EmptyHierarchyValueInFieldsRefused: an empty hierarchy +// value inside `fields` was a SILENT NO-OP, so it is refused (BUG-2850, +// codex round 11). +// +// `fields:{"parent":""}` promotes onto the top-level `parent` param, where +// both doors treat empty as NOT PROVIDED — promotedParamValue remotely, the +// `parentRef != ""` guard in cmd_item.go on the CLI. The call reported +// success and detached nothing, which is worse here than elsewhere because +// the caller's intent is unambiguous. +// +// Refused rather than promoted to a clear: giving this door clear semantics +// decides what `fields` MEANS, and v0.19 already made clear_parent the +// canonical detach so an empty string would not have to carry it. The +// refusal names both working forms, so the assertion covers that too — a +// refusal that leaves the caller with no route is a worse bug than the no-op. +func TestPadItemUpdate_EmptyHierarchyValueInFieldsRefused(t *testing.T) { + for _, key := range []string{"parent", "plan"} { + t.Run(key, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "fields": map[string]any{key: ""}, + }) + if !isErr { + t.Fatalf("an empty %s must not report success having done nothing; got: %s (args %v)", key, msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + if !strings.Contains(msg, "clear_parent") { + t.Errorf("the refusal must point at the working form: %s", msg) + } + }) + } +} + +// ...and the `field` ARRAY clear still works, because that form does reach +// the server's present-but-empty detach (BUG-2013). The refusal above is +// about the `fields` door only, and this is the leg that proves it did not +// over-reach into a form callers depend on. +func TestPadItemUpdate_FieldArrayClearStillDispatches(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{"parent="}, + }) + if isErr { + t.Fatalf("the field-array clear must still work: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "parent=") { + t.Errorf("clear entry lost: %v", disp.gotArgs) + } +} + +// TestPadItemUpdate_CompatIDConflictRefused: the v0.16 compat ID params +// conflict with `fields` like any other key (BUG-2850, codex round 11). +// +// `assigned_user_id` / `agent_role_id` are accepted at the top level as a +// documented, never-schema-declared compat form — which is exactly why they +// slipped the guard: invisible to padItemPromotedFieldKeys, they took the +// generic path, where the conflict check only consults the `field` array. +// With `assigned_user_id:"A"` plus `fields:{"assigned_user_id":"B"}` the +// doors disagreed outright — the remote mapper reads the top-level A while +// stdio emits only `--field assigned_user_id=B`, since the top-level form has +// no CLI flag behind it. One call, two different people assigned. +func TestPadItemUpdate_CompatIDConflictRefused(t *testing.T) { + for _, key := range []string{"assigned_user_id", "agent_role_id"} { + t.Run(key, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + key: "user-A", + "fields": map[string]any{key: "user-B"}, + }) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s (args %v)", msg, disp.gotArgs) + } + if !strings.Contains(msg, key) { + t.Errorf("error should name the conflicting key: %s", msg) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// ...and an EQUAL compat duplicate collapses to one form rather than being +// refused or sent twice — same disposition as the promoted keys. +func TestPadItemUpdate_CompatIDEqualDuplicateCollapses(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "assigned_user_id": "user-A", + "fields": map[string]any{"assigned_user_id": "user-A"}, + }) + if isErr { + t.Fatalf("an equal duplicate is unambiguous and must be accepted: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "assigned_user_id=user-A") { + t.Errorf("the value must still reach dispatch exactly once: %v", disp.gotArgs) + } +} + +// --- codex round 12 --- + +// TestPadItemUpdate_BlankTopLevelParamDoesNotBlockFields: a zero-filled +// optional param must not refuse the `fields` answer to the same question +// (BUG-2850, codex round 12). +// +// The reviewer named `status`. The probe drove every key that can arrive at +// the top level and all six promoted ones behaved identically, so this is the +// POPULATION rather than the instance (CONVE-18). Round 10 fixed exactly this +// for the hierarchy keys and I did not ask whether the same reasoning covered +// their siblings — it did. +// +// `""` is "not supplied" everywhere else on this surface: promotedParamValue +// treats it as absent, the CLI's `status != ""` guards do, `assign: ""` is +// documented inert. A client that fills declared optional params with their +// zero value was therefore refused on every promoted key at once. +func TestPadItemUpdate_BlankTopLevelParamDoesNotBlockFields(t *testing.T) { + cases := map[string]string{ + "status": "done", + "priority": "high", + "category": "infra", + "parent": "PLAN-12", + "role": "implementer", + "assign": "dave", + } + for key, val := range cases { + t.Run(key, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + key: "", + "fields": map[string]any{key: val}, + }) + if isErr { + t.Fatalf("a blank %s is not a competing value; expected success, got: %s", key, msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } + if !argsContainPair(disp.gotArgs, "--"+key, val) { + t.Errorf("the fields value must reach dispatch: %v", disp.gotArgs) + } + }) + } +} + +// ...and the COMPAT ID KEYS are excluded from that rule, because for them an +// empty string is not absence — it is a CLEAR to NULL, the deliberate v0.16 +// semantics (dispatch_http_advanced.go forwards "" verbatim for exactly these +// two). So a blank there IS an effective directive and still conflicts. +// +// This is the leg that stops the round-12 fix being applied uniformly. Had I +// taken the finding at face value across every top-level key, a clear would +// have been silently discarded in favour of the `fields` value — turning a +// spurious refusal into a silent wrong write, which is the worse trade. +func TestPadItemUpdate_BlankCompatIDIsAClearAndStillConflicts(t *testing.T) { + for _, key := range []string{"assigned_user_id", "agent_role_id"} { + t.Run(key, func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + key: "", + "fields": map[string]any{key: "user-B"}, + }) + if !isErr { + t.Fatalf("a blank %s is a CLEAR, so it conflicts with a set; got success: %s (args %v)", key, msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// --- codex round 13 + the restructure --- + +// TestPadItemUpdate_SemanticAliasPairsRefused: `assign`/`assigned_user_id` +// and `role`/`agent_role_id` are two names for one target, exactly like +// `parent`/`plan` (BUG-2850, codex round 13). +// +// None of the five guards that existed at round 12 compared them: the alias +// guard knew only about hierarchy, the compat guard only about same-name +// collisions. So `assigned_user_id:"B"` with `fields:{"assign":"A"}` was +// accepted and the doors then disagreed — resolveAssignName gives the +// explicit ID precedence over HTTP, while BuildCLIArgs drops the compat ID +// and emits `--assign A`. One call, two different people assigned. +// +// This drove the finding's POPULATION rather than its example: both +// directions of both pairs, plus the both-inside-`fields` and +// field-array-vs-`fields` shapes. +func TestPadItemUpdate_SemanticAliasPairsRefused(t *testing.T) { + cases := map[string]map[string]any{ + "compat id param vs fields alias": { + "assigned_user_id": "user-B", "fields": map[string]any{"assign": "dave"}, + }, + "alias param vs fields compat id": { + "assign": "dave", "fields": map[string]any{"assigned_user_id": "user-B"}, + }, + "role compat id param vs fields alias": { + "agent_role_id": "role-B", "fields": map[string]any{"role": "implementer"}, + }, + "role alias param vs fields compat id": { + "role": "implementer", "fields": map[string]any{"agent_role_id": "role-B"}, + }, + "both aliases inside one fields object": { + "fields": map[string]any{"assign": "dave", "assigned_user_id": "user-B"}, + }, + "field array alias vs fields compat id": { + "field": []any{"assign=dave"}, "fields": map[string]any{"assigned_user_id": "user-B"}, + }, + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + input := map[string]any{"action": "update", "ref": "TASK-5"} + for k, v := range extra { + input[k] = v + } + disp, msg, isErr := dispatchPadItem(t, input) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s (args %v)", msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("ambiguous call must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// TestFieldConflictProperty_AliasGroupsRefuseAcrossEverySourcePair is the +// PROPERTY the case tests above are instances of (lead ruling after round 13). +// +// The case tests are kept as regressions, but they are examples, and every +// round of this unit found the example that nobody had written. The property +// is the thing that generalizes: for EVERY alias group, and EVERY ordered +// pair of distinct member names, offering the two names through any two +// sources is ambiguous and must refuse. It is derived from fieldAliasGroups +// itself, so adding a future alias pair to that map extends this test with +// no edit — which is the whole point of replacing the guards with one map. +func TestFieldConflictProperty_AliasGroupsRefuseAcrossEverySourcePair(t *testing.T) { + // Group the alias map back into its equivalence classes. + classes := map[string][]string{} + for key, canonical := range fieldAliasGroups { + classes[canonical] = append(classes[canonical], key) + } + + // The three ways a value can enter, as functions writing into the input. + sources := map[string]func(in map[string]any, key, val string){ + "top-level param": func(in map[string]any, key, val string) { in[key] = val }, + "field array": func(in map[string]any, key, val string) { in["field"] = []any{key + "=" + val} }, + "fields object": func(in map[string]any, key, val string) { + f, _ := in["fields"].(map[string]any) + if f == nil { + f = map[string]any{} + in["fields"] = f + } + f[key] = val + }, + } + + tried := 0 + for canonical, members := range classes { + sort.Strings(members) + if len(members) < 2 { + t.Fatalf("alias class %q has one member; the map is the source of truth for this property", canonical) + } + for _, a := range members { + for _, b := range members { + if a == b { + continue + } + for sa, writeA := range sources { + for sb, writeB := range sources { + // One `fields` object cannot hold the same key twice, + // and one `field` array write would overwrite itself, + // so skip same-source pairs. + if sa == sb { + continue + } + // NO SKIP for the no-`fields` pairs any more (codex + // round 14). The property was written when the + // canonical pass ran only from inside + // reshapeItemFields, so param-vs-array combinations + // were excluded as out of scope — and that exclusion + // is exactly where the round-14 defect lived. Alias + // collisions now refuse whatever the sources, so the + // property covers every pair. + // + // The round-7 boundary is untouched and is about + // SAME-NAME duplicates, which this property never + // drives: last-write-wins is defensible when both + // sources name one key, and indefensible when two + // names address one target through different + // vocabularies. + // BOTH value shapes, and the EQUAL one is the load- + // bearing half. With differing values the ordinary + // same-canonical-key comparison refuses too, so a + // build that had lost alias detection entirely would + // still pass — verified: deleting the alias branch + // left this property green until this leg existed + // (CONVE-28, on a mutant that survived). + // + // Equal values are the case only alias detection + // catches, and refusing them is the ruling: two names + // address one target through different vocabularies + // (a slug vs a UUID), so "equal" is not a question + // this layer can answer, and matching strings do not + // make it answerable. + for _, values := range []struct{ label, a, b string }{ + {"differing", "value-A", "value-B"}, + {"equal", "same-value", "same-value"}, + } { + name := a + "/" + sa + " vs " + b + "/" + sb + " (" + values.label + ")" + t.Run(name, func(t *testing.T) { + in := map[string]any{"action": "update", "ref": "TASK-5"} + writeA(in, a, values.a) + writeB(in, b, values.b) + disp, msg, isErr := dispatchPadItem(t, in) + if !isErr { + t.Fatalf("two names for %q offered at once must refuse; got success: %s (args %v)", canonical, msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + tried++ + } + } + } + } + } + } + if tried == 0 { + t.Fatal("the property exercised nothing — the source/class enumeration is broken") + } + t.Logf("property held over %d source×alias combinations", tried) +} + +// --- codex round 14 --- + +// TestPadItemUpdate_AliasPairsRefusedWithoutFieldsObject: alias detection must +// reach the no-`fields` case too (BUG-2850, codex round 14). +// +// The round-13 restructure ran its canonical pass from inside +// reshapeItemFields, which returns early without a `fields` object — so an +// alias pair arriving through the top level and the `field` array alone +// slipped past it. Round 7 had already built exactly this always-run guard +// for the hierarchy pair, which is the tell: the restructure that was meant +// to end guard accretion had itself left TWO alias mechanisms with different +// reach. There is now one. +// +// The hierarchy leg is included deliberately, as the CONTROL that used to +// pass: it is the one pair the old always-run guard covered, so a fix that +// merely moved the gap would keep it green while the other two go red. +func TestPadItemUpdate_AliasPairsRefusedWithoutFieldsObject(t *testing.T) { + cases := map[string]map[string]any{ + "assign pair, no fields object": { + "assigned_user_id": "user-B", "field": []any{"assign=dave"}, + }, + "role pair, no fields object": { + "agent_role_id": "role-B", "field": []any{"role=implementer"}, + }, + "hierarchy pair, no fields object (was already covered)": { + "parent": "PLAN-A", "field": []any{"plan=PLAN-B"}, + }, + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + input := map[string]any{"action": "update", "ref": "TASK-5"} + for k, v := range extra { + input[k] = v + } + disp, msg, isErr := dispatchPadItem(t, input) + if !isErr { + t.Fatalf("expected structured refusal, got success: %s (args %v)", msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// TestPadItemUpdate_EqualStructuredDuplicateCollapses: two equal STRUCTURES +// are one unambiguous value, not a conflict (BUG-2850, codex round 14). +// +// A regression the round-13 restructure introduced: the new pass refused +// whenever either side was structured, where scalarEqual had always collapsed +// equal ones. Nothing in the suite caught it — the existing tags tests pass a +// structure on one side only — which is why this pin exists in both +// directions. +func TestPadItemUpdate_EqualStructuredDuplicateCollapses(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "tags": []any{"a", "b"}, + "fields": map[string]any{"tags": []any{"a", "b"}}, + }) + if isErr { + t.Fatalf("equal structured duplicates are one value, not a conflict: %s", msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } +} + +// ...and DIFFERING structures still refuse, so the collapse did not turn into +// "any two structures agree". +func TestPadItemUpdate_DifferingStructuredDuplicateRefused(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "tags": []any{"a"}, + "fields": map[string]any{"tags": []any{"b"}}, + }) + if !isErr { + t.Fatalf("differing structures must still refuse; got success: %s (args %v)", msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } +} + +// TestFieldConflictProperty_SourcesDerivedFromTheDeclaredSchema closes the +// gap the lead named after round 14: the property above enumerates its +// sources by hand, and that hand-written list came from the same head as +// detectFieldConflicts. A property whose input list mirrors the code cannot +// see a source the code forgot — which is exactly how the round-14 defect +// (param-vs-array pairs excluded as "out of scope") survived a property test +// that was supposed to generalize. +// +// So this derives the population from the DOOR'S DECLARED CONTRACT — the live +// pad_item ToolDef's parameter list, which is what agents read and what the +// tool advertises — and asserts that every declared param capable of writing +// a field value is CLASSIFIED by the conflict machinery. Adding a writable +// param to the catalog without teaching detectFieldConflicts about it fails +// here, naming the param, rather than silently creating a source no guard +// visits. +// +// The two lists have genuinely different origins: one is the tool schema, the +// other is the three key sets. That independence is the whole point — if this +// test ever starts deriving its expectations from the same sets it checks, it +// stops being able to fail. +func TestFieldConflictProperty_SourcesDerivedFromTheDeclaredSchema(t *testing.T) { + def := padItemDef(t) + + // Params that carry a field VALUE into an item write. Everything else on + // the tool addresses the call (ref, workspace, action), controls the + // response (limit, format, full), or is a documented non-field flag. + // Each entry carries WHY it cannot collide with a `fields` key. The + // reasons are the point — a bare list would let a future field-writing + // param be silenced by adding one word to it, which is how the round-14 + // exclusion happened in the first place. + notFieldWriters := map[string]bool{ + // Address the call rather than the item's fields. + "action": true, "ref": true, "workspace": true, "collection": true, + "refs": true, // bulk-update's target list + "target": true, // link/unlink's other end + "target_collection": true, // move's destination + "link_type": true, // the relationship kind, not a field + + // Item COLUMNS, not entries in the fields blob. + "title": true, "content": true, "slug": true, "pinned": true, + + // Shape the response or the query. + "limit": true, "offset": true, "full": true, "sort": true, "query": true, + "group_by": true, "all": true, "archived": true, "include_archived": true, + "unparented": true, "since": true, "days": true, "actor": true, + "category_filter": true, "parent_ref": true, + + // Control the write without naming a field value. + "expected_updated_at": true, "force": true, "allow_draft": true, + "clear_parent": true, "clear_assigned_user": true, "clear_agent_role": true, + "artifact": true, "raw_args": true, + + // SYSTEM-METADATA WRITERS — these DO change item state, so the + // exclusion is a real claim rather than a shrug. They write + // implementation_notes / decision_log through their own actions + // (note, decide), and those exact keys are REFUSED through `field` + // and `fields` (BUG-2627 / BUG-2675). So they cannot reach the same + // key by two routes, which is the only thing this pass adjudicates. + "summary": true, "details": true, // action=note + "decision": true, "rationale": true, // action=decide + "message": true, "reply_to": true, // action=comment + "comment": true, // the audit note on update + + // The two SOURCES themselves, not keys within them. + "fields": true, + "field": true, + } + + // What the conflict machinery knows how to classify. + classified := map[string]bool{} + for _, set := range []map[string]bool{padItemPromotedFieldKeys, compatIDFieldKeys, hierarchyPseudoFieldKeys, identityRefFieldKeys} { + for k := range set { + classified[k] = true + } + } + + var unclassified []string + for _, p := range def.Schema.Params { + if notFieldWriters[p.Name] || classified[p.Name] { + continue + } + unclassified = append(unclassified, p.Name) + } + sort.Strings(unclassified) + + if len(unclassified) > 0 { + t.Errorf("declared pad_item params that neither the conflict machinery classifies "+ + "nor this test excludes as non-field-writing: %v\n"+ + "If one of these can write a field value, add it to the appropriate key set so "+ + "detectFieldConflicts visits it — a source no guard visits is the round-14 defect. "+ + "If it cannot, add it to notFieldWriters with that reason.", unclassified) + } + + // The independence check: every classified key must actually be reachable + // through the declared schema OR be a documented undeclared compat form. + // This is what fails if a key set grows a name the door cannot carry. + declared := map[string]bool{} + for _, p := range def.Schema.Params { + declared[p.Name] = true + } + for k := range classified { + if declared[k] { + continue + } + if compatIDFieldKeys[k] { + continue // v0.16 compat: deliberately never schema-declared + } + if k == "plan" { + continue // a fields_patch pseudo-key the server reads; no top-level param + } + t.Errorf("the conflict machinery classifies %q, but the pad_item schema declares no such "+ + "param and it is not a documented undeclared form — either the key set is stale or "+ + "the schema lost a param", k) + } +} + +// --- codex round 15 --- + +// TestPadItemUpdate_CompatIDSameNameCollisionRefusedWithoutFields: the +// round-7 same-name exemption holds only where the doors provably agree, and +// the compat IDs are where they do not (BUG-2850, codex round 15). +// +// For a schema-declared param the CLI has a real flag, so stdio receives BOTH +// forms and its overlay order resolves them exactly as the HTTP mapper does. +// That is the premise the exemption rests on, and it is pinned per door by +// the SameNameDuplicate tests. +// +// `assigned_user_id` / `agent_role_id` are deliberately never +// schema-declared, so BuildCLIArgs — which emits the CLI's real flags — has +// nothing to emit for the top-level form and DROPS it. stdio then sees only +// the field entry while HTTP reads the top-level param: same call, two +// different people assigned, with no `fields` object anywhere. +// +// The control leg is the declared param, which must STILL resolve rather than +// refuse. Without it this test would pass on a build that abandoned the +// round-7 boundary altogether. +func TestPadItemUpdate_CompatIDSameNameCollisionRefusedWithoutFields(t *testing.T) { + for _, key := range []string{"assigned_user_id", "agent_role_id"} { + t.Run(key+" refuses", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + key: "value-A", + "field": []any{key + "=value-B"}, + }) + if !isErr { + t.Fatalf("the doors cannot agree on this; expected refusal, got success: %s (args %v)", msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } + t.Run("declared param still resolves", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "status": "open", + "field": []any{"status=done"}, + }) + if isErr { + t.Fatalf("the round-7 boundary must survive: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "status=done") { + t.Errorf("both forms must still reach the CLI, which resolves them: %v", disp.gotArgs) + } + }) +} + +// --- codex round 16 --- + +// TestPadItemUpdate_PaddedEntryCollidingWithAParamRefused: equality only +// licenses a collapse when both doors receive the SAME write (BUG-2850, +// codex round 16). +// +// The conflict index is normalized — that is what lets a padded entry be +// recognized as a collision at all — so `field:["k = A"]` compared EQUAL to a +// top-level `k:"A"` and the pair was accepted while the entry stayed padded +// on the wire. HTTP trims it and writes `k`; the CLI does not, and writes a +// junk `"k "` key instead. The normalization that made the collision VISIBLE +// is exactly what made accepting it wrong. +// +// Both key classes are driven, because the divergence is not compat-specific: +// a declared param picks up a junk key alongside its correct write, a compat +// ID loses its value entirely. +func TestPadItemUpdate_PaddedEntryCollidingWithAParamRefused(t *testing.T) { + cases := map[string]map[string]any{ + "compat id vs padded entry": { + "assigned_user_id": "user-A", "field": []any{"assigned_user_id = user-A"}, + }, + "declared param vs padded entry": { + "status": "done", "field": []any{"status = done"}, + }, + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + input := map[string]any{"action": "update", "ref": "TASK-5"} + for k, v := range extra { + input[k] = v + } + disp, msg, isErr := dispatchPadItem(t, input) + if !isErr { + t.Fatalf("a padded entry and a param do not reach the doors alike; expected refusal, got success: %s (args %v)", msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + } +} + +// ...and a padded entry standing ALONE is untouched. That is BUG-2870 — ruled +// out of this PR's scope because fixing it changes what every CLI caller +// receives, not just callers who supplied the same key twice. +// +// This leg is the scope boundary made executable: if it ever goes red, the +// round-16 refusal has grown into BUG-2870's territory without a ruling. +func TestPadItemUpdate_PaddedEntryAloneIsUntouched(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "field": []any{"assigned_user_id = user-A"}, + }) + if isErr { + t.Fatalf("a lone padded entry is BUG-2870's business, not this PR's: %s", msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } +} + +// ...and a CANONICAL equal duplicate still collapses, so the refusal is about +// the padding and not about the collision. +func TestPadItemUpdate_CanonicalCompatDuplicateStillCollapses(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", + "ref": "TASK-5", + "assigned_user_id": "user-A", + "field": []any{"assigned_user_id=user-A"}, + }) + if isErr { + t.Fatalf("an equal canonical duplicate is unambiguous: %s", msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } +} + +// --- codex round 17 --- + +// TestPadItemUpdate_PaddedEntryRefusedWhenFieldsDoesNotCoverTheKey: whether an +// entry gets canonicalized is a PER-KEY question (BUG-2850, codex round 17). +// +// Round 16 gated the padded-entry refusal on "no `fields` object", reasoning +// that a `fields` object causes reshapeItemFields to re-emit the entry +// canonically. That holds for keys IN the object. With `fields:{}` or a +// `fields` carrying some other key, nothing canonicalizes +// `field:["status = done"]` and it reaches the doors padded exactly as it +// does with no `fields` at all — HTTP writes `status`, the CLI writes a junk +// `"status "` beside it. +// +// The last leg is the control that makes the distinction real: with the key +// present in `fields`, the entry IS canonicalized, so the call must still +// succeed. A fix that simply refused whenever anything was padded would pass +// the first two legs and fail this one. +func TestPadItemUpdate_PaddedEntryRefusedWhenFieldsDoesNotCoverTheKey(t *testing.T) { + t.Run("fields empty", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "status": "done", "field": []any{"status = done"}, + "fields": map[string]any{}, + }) + if !isErr { + t.Fatalf("nothing canonicalizes this entry; expected refusal, got: %s (args %v)", msg, disp.gotArgs) + } + }) + t.Run("fields carries another key", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "status": "done", "field": []any{"status = done"}, + "fields": map[string]any{"effort": "l"}, + }) + if !isErr { + t.Fatalf("a fields object covering a DIFFERENT key does not canonicalize this one; got: %s (args %v)", msg, disp.gotArgs) + } + }) + t.Run("fields carries the key — canonicalized, so accepted", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "status": "done", "field": []any{"status = done"}, + "fields": map[string]any{"status": "done"}, + }) + if isErr { + t.Fatalf("this key IS canonicalized, so the call must succeed: %s", msg) + } + if argsContainPair(disp.gotArgs, "--field", "status = done") { + t.Errorf("the padded entry must not survive canonicalization: %v", disp.gotArgs) + } + }) +} + +// --- codex round 18 --- + +// TestPadItemUpdate_DuplicateFieldEntriesKeepTheirMultiplicity: two `field` +// entries naming one key are two sources, and the conflict pass must see both +// (BUG-2850, codex round 18). +// +// parseFieldArray indexes by NORMALIZED key, so `["effort=l", " effort=l"]` +// collapsed into a single index slot. Iterating that index made the pass's own +// input lossy: the pair arrived as ONE contribution, fell under the len < 2 +// early exit, and passed unchecked — HTTP trims both to `effort` while stdio +// writes `effort` AND a junk `" effort"`. The pass claims to adjudicate one +// canonical key offered by multiple sources; two array entries ARE multiple +// sources, and it could not see them. +// +// The fix is upstream of the rules rather than another rule: walk the raw +// entries, and the existing non-canonical check applies unchanged. +func TestPadItemUpdate_DuplicateFieldEntriesKeepTheirMultiplicity(t *testing.T) { + t.Run("padded twin is refused", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "field": []any{"effort=l", " effort=l"}, + }) + if !isErr { + t.Fatalf("the doors write different key sets for this; expected refusal, got: %s (args %v)", msg, disp.gotArgs) + } + if len(disp.gotPath) != 0 { + t.Errorf("must not dispatch; dispatched %v", disp.gotPath) + } + }) + + // Two IDENTICAL canonical entries are one unambiguous write stated twice — + // both doors receive the same thing, so this must still succeed. Without + // this leg the fix above could refuse every repeated key and still look + // correct. + t.Run("identical canonical entries still pass", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "field": []any{"effort=l", "effort=l"}, + }) + if isErr { + t.Fatalf("identical entries are not a conflict: %s", msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } + }) + + // ...and two CANONICAL entries for one key with differing values are NOT + // refused — they resolve, last-write-wins, on both doors. + // + // I first wrote this leg asserting a refusal, and the code disagreed. + // The code was right: ingestFieldKVP (HTTP) and the --field loop in + // cmd_item.go (CLI) both do `map[key] = val` in entry order, so each door + // keeps the LAST entry and they agree. That is the round-7 boundary + // exactly — a visible duplicate with a resolution the caller can predict + // — and refusing it here would have contradicted the boundary the lead + // confirmed, on two doors that are pinned to agree. + // + // Kept as a leg rather than deleted, because it is the one that stops a + // future "refuse every repeated key" simplification from looking correct. + t.Run("differing canonical values resolve, not refuse", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "field": []any{"effort=l", "effort=s"}, + }) + if isErr { + t.Fatalf("both doors keep the last entry, so this resolves rather than refusing: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "effort=s") { + t.Errorf("the last entry must reach the door: %v", disp.gotArgs) + } + }) +} + +// --- codex round 19 --- + +// TestPadItemUpdate_UnrelatedFieldsObjectDoesNotForceRefusal: a `fields` +// object carrying some OTHER key must not drag an unrelated duplicate into +// conflict resolution (BUG-2850, codex round 19). +// +// Round 17 made the padded gate per-key and left the SAME-NAME gate +// per-request — the same mistake in the sibling gate, one round later. With +// `fields:{"other":"x"}` and `field:["effort=l","effort=s"]`, `effort` is not +// in the object, nothing arbitrates it but the doors themselves, and both keep +// the last entry. Refusing it was a false refusal on a call that resolves +// deterministically. +// +// The second leg is the control: when the object DOES carry the key it is a +// genuine third source, and differing values must still refuse. +func TestPadItemUpdate_UnrelatedFieldsObjectDoesNotForceRefusal(t *testing.T) { + t.Run("fields carries another key — resolves", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "field": []any{"effort=l", "effort=s"}, + "fields": map[string]any{"other": "x"}, + }) + if isErr { + t.Fatalf("nothing arbitrates effort but the doors, which agree: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "effort=s") { + t.Errorf("the last entry must reach the door: %v", disp.gotArgs) + } + }) + t.Run("fields carries THE key — still refuses", func(t *testing.T) { + _, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "field": []any{"effort=l"}, + "fields": map[string]any{"effort": "s"}, + }) + if !isErr { + t.Fatalf("the object is a genuine third source; differing values must refuse: %s", msg) + } + }) +} + +// TestPadItemUpdate_WhitespacePreservingValuesCompareLikeWithLike: entry +// values are trimmed for COMPARISON because ingestFieldKVP trims them, so +// comparing them against an untrimmed `fields` value was apples to oranges +// (BUG-2850, codex round 19). +// +// `fields:{"note":" x "}` with `field:["note= x "]` read as " x " vs "x" and +// refused, though both doors write " x ". The assertion is on what reaches +// the door, not merely on success: trimming for comparison must not trim what +// is WRITTEN, or the fix would silently retype the caller's value. +func TestPadItemUpdate_WhitespacePreservingValuesCompareLikeWithLike(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "fields": map[string]any{"note": " x "}, + "field": []any{"note= x "}, + }) + if isErr { + t.Fatalf("both doors write \" x \"; this is one value stated twice: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "note= x ") { + t.Errorf("the untrimmed value must survive to the wire; comparison-trimming must not become write-trimming: %v", disp.gotArgs) + } +} + +// ...and the same value through a NON-canonical entry, which forces the +// re-emission path to run. Without this leg the assertion above is +// unreachable for that path: its entry is already canonical, so nothing is +// re-emitted and a mutant that trimmed the EMITTED value survived untouched +// (CONVE-28 — the mutant was faithful, the test could not see it). +// +// Here the KEY is padded, so the entry is re-emitted, and the value it +// carries must still be the untrimmed one the caller sent. +func TestPadItemUpdate_ReEmittedValueKeepsItsWhitespace(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "fields": map[string]any{"note": " x "}, + "field": []any{"note = x "}, + }) + if isErr { + t.Fatalf("padding around the KEY is canonicalized, not a conflict: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", "note= x ") { + t.Errorf("the re-emitted entry must carry the caller's untrimmed VALUE with the canonical key: %v", disp.gotArgs) + } +} + +// ...and genuinely different values are still refused, so trimming for +// comparison did not turn into "any two values agree". +func TestPadItemUpdate_TrimmedComparisonStillCatchesRealDifferences(t *testing.T) { + _, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "fields": map[string]any{"note": " x "}, + "field": []any{"note= y "}, + }) + if !isErr { + t.Fatalf("x and y are different values however they are padded: %s", msg) + } +} + +// --- codex round 20 --- + +// TestPadItemUpdate_CompatExceptionNeedsATopLevelValue: the compat-ID +// exception turns on a TOP-LEVEL value being present, not on the key being a +// compat one (BUG-2850, codex round 20). +// +// Round 15's reason was specific: a top-level compat param has no CLI flag, +// so BuildCLIArgs DROPS it while HTTP reads it, and the doors receive +// different writes. That asymmetry exists only for the top-level form. Two +// `field` entries naming a compat key carry no such thing — both doors keep +// the last and lift the same column — so refusing them applied the reason +// past the source it was verified on. +// +// The second leg is round 15's own case, which must still refuse. Without it +// this test would pass on a build that dropped the compat exception +// altogether, which is the defect round 15 existed to fix. +func TestPadItemUpdate_CompatExceptionNeedsATopLevelValue(t *testing.T) { + for _, key := range []string{"assigned_user_id", "agent_role_id"} { + t.Run(key+": two entries, no top-level — resolves", func(t *testing.T) { + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "field": []any{key + "=A", key + "=B"}, + }) + if isErr { + t.Fatalf("both doors keep the last entry and lift the same column: %s", msg) + } + if !argsContainPair(disp.gotArgs, "--field", key+"=B") { + t.Errorf("the last entry must reach the door: %v", disp.gotArgs) + } + }) + t.Run(key+": top-level present — still refuses", func(t *testing.T) { + _, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + key: "A", + "field": []any{key + "=B"}, + }) + if !isErr { + t.Fatalf("the top-level form is dropped by stdio and read by HTTP; this must refuse: %s", msg) + } + }) + } +} + +// --- codex round 21 --- + +// TestPadItemUpdate_NilTopLevelIsAbsentForEveryKey: a nil top-level value is +// absence on every door and for every key (BUG-2850, codex round 21). +// +// topLevelValueProvided returned true unconditionally for the compat IDs and +// fell through to true for everything else, so a nil counted as a supplied +// value and refused against a `fields` entry for the same key. Nothing writes +// a nil: the HTTP mapper's `.(string)` assertion drops it and BuildCLIArgs has +// no flag value to emit, so both doors resolve to the `fields` value. +// +// The finding named `assigned_user_id` / `agent_role_id`. The probe found all +// five top-level keys behaving identically, so the fix and this pin cover the +// population rather than the instance. +func TestPadItemUpdate_NilTopLevelIsAbsentForEveryKey(t *testing.T) { + for _, key := range []string{"assigned_user_id", "agent_role_id", "status", "priority", "parent"} { + t.Run(key, func(t *testing.T) { + val := "B" + if key == "parent" { + val = "PLAN-12" + } + disp, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + key: nil, + "fields": map[string]any{key: val}, + }) + if isErr { + t.Fatalf("a nil %s is not a competing value; expected success, got: %s", key, msg) + } + if len(disp.gotPath) == 0 { + t.Fatal("expected the update to dispatch") + } + }) + } +} + +// ...and an empty-string compat ID is STILL a clear, so treating nil as +// absence did not swallow the v0.16 semantics that carve-out exists for. +func TestPadItemUpdate_EmptyCompatIDIsStillAClearAfterTheNilFix(t *testing.T) { + _, msg, isErr := dispatchPadItem(t, map[string]any{ + "action": "update", "ref": "TASK-5", + "assigned_user_id": "", + "fields": map[string]any{"assigned_user_id": "user-B"}, + }) + if !isErr { + t.Fatalf("an empty compat ID is a CLEAR and still conflicts with a set: %s", msg) + } +} diff --git a/internal/mcp/dispatch.go b/internal/mcp/dispatch.go index 6d78b7c71..dd7e1e28c 100644 --- a/internal/mcp/dispatch.go +++ b/internal/mcp/dispatch.go @@ -128,6 +128,19 @@ func (d *ExecDispatcher) Dispatch(ctx context.Context, cmdPath []string, cliArgs if d.Binary == "" { return mcp.NewToolResultError("dispatcher: binary path not configured"), nil } + // THIS dispatcher is the stdio door, and it cannot express a structured + // field value: it runs the CLI with `--field key=value` arguments, and a + // nested object or array has no such encoding (BUG-2850). + // + // The check lives HERE, not in BuildCLIArgs, because BuildCLIArgs runs for + // BOTH transports — env.Dispatch calls it before handing off to whichever + // Dispatcher is configured — so a refusal there also blocked the remote + // door, whose whole point in this change is that it CAN carry structures + // (codex round 2 [P1]). Refuse loudly rather than dropping: cliArgs simply + // would not contain the key, which is the silent discard this bug is about. + if err := refuseStructuredFieldsOverCLI(DispatchInputFromContext(ctx)); err != nil { + return validationFailedFromBuildErr(strings.Join(cmdPath, " "), err), nil + } full := append(append([]string{}, cmdPath...), cliArgs...) cmd := exec.CommandContext(ctx, d.Binary, full...) var stdout, stderr strings.Builder @@ -359,6 +372,37 @@ func parseWorkspaceListJSON(body string) ([]WorkspaceHint, error) { // values in rootFlags are skipped. // - `--format json` is appended unless the input explicitly sets a // format flag (e.g. an agent specifically requests markdown). +// +// refuseStructuredFieldsOverCLI reports an error naming any `fields` keys whose +// value is a nested object or array — values the `--field key=value` CLI +// encoding cannot carry (BUG-2850). +// +// The message names the TRANSPORT rather than the value on purpose: the +// reporter's agent read PR #1159's "no defined write semantics" as a verdict +// on its data and rewrote seven playbooks' argument specs. This door cannot +// express the value; Pad can store it. +func refuseStructuredFieldsOverCLI(input map[string]any) error { + native, ok := input[fieldsNativeKey].(map[string]any) + if !ok { + return nil + } + var keys []string + for k, v := range native { + switch v.(type) { + case map[string]any, []any: + keys = append(keys, k) + } + } + if len(keys) == 0 { + return nil + } + sort.Strings(keys) + return fmt.Errorf( + "fields.%s: a structured value cannot be written over the stdio transport, which sends --field key=value; "+ + "use the remote transport, or write the field with the CLI directly", + strings.Join(keys, ", fields.")) +} + func BuildCLIArgs( cmdInfo cmdhelp.Command, input map[string]any, diff --git a/internal/mcp/dispatch_http.go b/internal/mcp/dispatch_http.go index f5f0c9bef..8576b1ca8 100644 --- a/internal/mcp/dispatch_http.go +++ b/internal/mcp/dispatch_http.go @@ -793,6 +793,16 @@ func mapItemCreate(input map[string]any) (method, path string, body []byte, err fields[k] = v } } + // The `fields` object with its JSON types intact, applied LAST so it wins + // over the stringified copy of itself in `field` (BUG-2850). This is the + // only door where those types survive the trip — parseFieldKVP above turns + // every entry into a string, which is what made a number field unwritable + // and an undeclared number land as "42". The two forms carry the same + // input, so last-write-wins here is a fidelity choice, not a precedence + // one; genuine key conflicts were already refused in the catalog merge. + for k, v := range nativeFields(input) { + fields[k] = v + } payload := map[string]any{} for _, key := range []string{"title", "content", "slug"} { @@ -945,6 +955,13 @@ func liftFieldsToColumns(fields, payload map[string]any) { // (single string, []string, []any) into a `key→value` map. Empty / // invalid entries are skipped silently to match the CLI's permissive // behaviour. +// nativeFields returns the `fields` object the catalog merge preserved with +// its JSON types intact, or nil. See fieldsNativeKey (BUG-2850). +func nativeFields(input map[string]any) map[string]any { + m, _ := input[fieldsNativeKey].(map[string]any) + return m +} + func parseFieldKVP(raw any) (map[string]any, error) { out := map[string]any{} switch v := raw.(type) { diff --git a/internal/mcp/dispatch_http_advanced.go b/internal/mcp/dispatch_http_advanced.go index 47d3c657c..70ec9afd5 100644 --- a/internal/mcp/dispatch_http_advanced.go +++ b/internal/mcp/dispatch_http_advanced.go @@ -435,7 +435,7 @@ func (d *HTTPHandlerDispatcher) dispatchItemUpdate( if hasFieldChanges(input) { patch := map[string]any{} for _, key := range []string{"status", "priority", "category", "parent"} { - if v, ok := input[key].(string); ok && v != "" { + if v, ok := promotedParamValue(input[key]); ok { patch[key] = v } } @@ -449,6 +449,13 @@ func (d *HTTPHandlerDispatcher) dispatchItemUpdate( patch[k] = v } } + // The `fields` object with its JSON types intact, applied LAST so it + // wins over the stringified copy of itself in `field` — same reasoning + // as mapItemCreate (BUG-2850). Without this an update through the + // object param stringifies exactly as a create did. + for k, v := range nativeFields(input) { + patch[k] = v + } // Lift recognized column keys (agent_role_id, assigned_user_id) // out of the patch onto the top-level payload so the handler writes // the column instead of stuffing the value inert in the JSON. Same @@ -634,6 +641,42 @@ func (d *HTTPHandlerDispatcher) collectionSchemaShadowsParent( return "", nil } +// promotedParamValue reads one of the promoted item params (status, priority, +// category, parent) off the dispatch input, reporting whether it was actually +// supplied. +// +// A NON-STRING SCALAR COUNTS (codex round 6). Both call sites used to read +// `input[key].(string)`, which was right while these params could only arrive +// as CLI-shaped strings — but reshapeItemFields now promotes `fields:{…}` +// values with their JSON types intact, so `fields:{"priority":3}` arrives as a +// float64. The type assertion dropped it at BOTH sites: hasFieldChanges said +// there was nothing to change, so the dispatcher skipped the fields_patch +// branch entirely and answered SUCCESS having written nothing. A silent +// no-op — the exact failure mode BUG-2850 exists to remove — reintroduced by +// the fix for it, and asymmetric with create, where mapItemCreate has always +// passed non-strings through for the handler to validate. +// +// Empty string stays "not supplied", which is the invariant every other +// declared string param on this tool holds; a non-string is always supplied, +// since no zero value can be confused with absence. The server still +// validates the value against the collection schema — this decides whether it +// is SENT, not whether it is correct. +func promotedParamValue(raw any) (any, bool) { + switch v := raw.(type) { + case nil: + return nil, false + case string: + return v, v != "" + case bool, float64, int, int64, json.Number: + return v, true + default: + // Structures never reach here: reshapeItemFields refuses a non-string + // for the hierarchy keys and stringifies the rest, and a caller + // sending one directly is refused by strict input validation. + return nil, false + } +} + // hasFieldChanges reports whether the input has any value that // should trigger field-merging on update. Mirrors the CLI's check // at cmd/pad/main.go itemUpdateCmd around the `hasFieldChanges` @@ -641,8 +684,16 @@ func (d *HTTPHandlerDispatcher) collectionSchemaShadowsParent( // --content "x"` would do an unnecessary GET-merge-PATCH of // fields, churning the audit log entry for no reason. func hasFieldChanges(input map[string]any) bool { + // The native `fields` map counts as a change on its own (BUG-2850). A + // NESTED-only update — fields:{"spec":[…]} — emits no `field` entry at + // all, because a structure has no key=value encoding, so without this the + // update reports success and writes nothing. That is the silent-drop shape + // this bug is about, arriving through the fix for it. + if len(nativeFields(input)) > 0 { + return true + } for _, key := range []string{"status", "priority", "category", "parent"} { - if v, ok := input[key].(string); ok && v != "" { + if _, ok := promotedParamValue(input[key]); ok { return true } } diff --git a/internal/mcp/dispatch_http_advanced_test.go b/internal/mcp/dispatch_http_advanced_test.go index 584911eca..c1d5d6442 100644 --- a/internal/mcp/dispatch_http_advanced_test.go +++ b/internal/mcp/dispatch_http_advanced_test.go @@ -925,3 +925,108 @@ func (c *requestCapture) ServeHTTP(w http.ResponseWriter, r *http.Request) { } _, _ = w.Write([]byte(`{}`)) } + +// TestDispatchItemUpdate_NonStringPromotedValueIsNotDropped: a promoted param +// that arrives as a NUMBER must reach the PATCH, not be silently discarded +// (BUG-2850, codex round 6). +// +// reshapeItemFields promotes `fields:{"priority":3}` onto the top-level +// `priority` input with its JSON type intact, so the dispatcher sees a +// float64. Both places that read these params asserted `.(string)`: +// hasFieldChanges then reported nothing to change, the whole fields_patch +// branch was skipped, and the call answered SUCCESS having sent no PATCH at +// all — a silent no-op reintroduced by the fix for silent no-ops, and +// asymmetric with create, where mapItemCreate has always passed non-strings +// through. +// +// The assertion is that a PATCH HAPPENED and carries the value with its type. +// Unfixed, requestCount is 0, which is why counting the request matters more +// here than reading the body. +func TestDispatchItemUpdate_NonStringPromotedValueIsNotDropped(t *testing.T) { + captured := newRequestCapture() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5","fields":"{}"}`)) + case http.MethodPatch: + captured.ServeHTTP(w, r) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5"}`)) + default: + t.Fatalf("unexpected method %s", r.Method) + } + }) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "ref": "TASK-5", + // Exactly what reshapeItemFields emits for fields:{"priority":3}: + // the promoted key at the top level, still a JSON number. + "priority": float64(3), + }) + res, err := d.Dispatch(ctx, []string{"item", "update"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + if captured.requestCount != 1 { + t.Fatalf("expected 1 PATCH, got %d — a numeric promoted value was dropped and the update wrote nothing", captured.requestCount) + } + var body map[string]any + if err := json.Unmarshal([]byte(captured.lastBody), &body); err != nil { + t.Fatalf("decode body: %v\n%s", err, captured.lastBody) + } + patch, ok := body["fields_patch"].(map[string]any) + if !ok { + t.Fatalf("fields_patch not an object in body: %v", body) + } + got, isNumber := patch["priority"].(float64) + if !isNumber { + t.Fatalf("priority: want a JSON number, got %[1]T(%[1]v) — patch: %v", patch["priority"], patch) + } + if got != 3 { + t.Errorf("priority = %v, want 3", got) + } +} + +// ...and an empty string still means "not supplied", so widening the type +// check did not turn a no-op param into a write. Same invariant every other +// declared string param on this tool holds. +func TestDispatchItemUpdate_EmptyPromotedStringIsStillNotAChange(t *testing.T) { + captured := newRequestCapture() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5","fields":"{}"}`)) + case http.MethodPatch: + captured.ServeHTTP(w, r) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5"}`)) + default: + t.Fatalf("unexpected method %s", r.Method) + } + }) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "ref": "TASK-5", + "comment": "note only", + "status": "", + }) + res, err := d.Dispatch(ctx, []string{"item", "update"}, nil) + if err != nil || res.IsError { + t.Fatalf("Dispatch err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + var body map[string]any + if err := json.Unmarshal([]byte(captured.lastBody), &body); err != nil { + t.Fatalf("decode body: %v\n%s", err, captured.lastBody) + } + if _, present := body["fields_patch"]; present { + t.Errorf("an empty status must not produce a fields_patch: %v", body) + } +} diff --git a/internal/mcp/dispatch_http_same_name_duplicate_test.go b/internal/mcp/dispatch_http_same_name_duplicate_test.go new file mode 100644 index 000000000..92851431c --- /dev/null +++ b/internal/mcp/dispatch_http_same_name_duplicate_test.go @@ -0,0 +1,64 @@ +package mcp + +// BUG-2850, codex round 7 boundary + the lead's condition on it. +// +// The remote door's half of the same-name-duplicate contract; +// cmd/pad/item_same_name_duplicate_precedence_test.go is the stdio door's. +// Both assert the SAME outcome, which is the point: the round-7 decision not +// to refuse `status` + `field:["status=…"]` rests entirely on the two doors +// resolving it identically, and nothing enforced that until these two tests +// existed. Reorder either overlay and exactly one of them goes red. +// +// parent+plan is the case that IS refused — two names for one target, which a +// caller can collide without knowing. A same-name duplicate is visibly a +// duplicate, so last-write-wins is a resolution the caller can predict. + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +func TestDispatchItemUpdate_SameNameDuplicateResolvesFieldWins(t *testing.T) { + captured := newRequestCapture() + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-5", func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5","fields":"{}"}`)) + case http.MethodPatch: + captured.ServeHTTP(w, r) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ref":"TASK-5"}`)) + default: + t.Fatalf("unexpected method %s", r.Method) + } + }) + + d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "caller"})} + ctx := WithDispatchInput(context.Background(), map[string]any{ + "workspace": "docapp", + "ref": "TASK-5", + "status": "open", + "field": []any{"status=done"}, + }) + res, err := d.Dispatch(ctx, []string{"item", "update"}, nil) + if err != nil || res.IsError { + t.Fatalf("a same-name duplicate must resolve, not refuse: err=%v IsError=%v: %#v", err, res != nil && res.IsError, res) + } + var body map[string]any + if err := json.Unmarshal([]byte(captured.lastBody), &body); err != nil { + t.Fatalf("decode body: %v\n%s", err, captured.lastBody) + } + patch, ok := body["fields_patch"].(map[string]any) + if !ok { + t.Fatalf("fields_patch not an object in body: %v", body) + } + if got := patch["status"]; got != "done" { + t.Fatalf("status = %v, want %q — the field entry overlays the named param on this door too", got, "done") + } +} diff --git a/internal/mcp/dispatch_http_test.go b/internal/mcp/dispatch_http_test.go index ece7b24ae..023a07d41 100644 --- a/internal/mcp/dispatch_http_test.go +++ b/internal/mcp/dispatch_http_test.go @@ -1015,3 +1015,51 @@ func TestHTTPHandlerDispatcher_PassesNonDefaultCollectionSlugThrough(t *testing. "shadow a real collection (BUG-2630)", rec.gotPath, wantPath) } } + +// The remote /mcp door must carry the `fields` object's JSON types through to +// the request body (BUG-2850). +// +// This is the door the bug was reported against. The catalog merge used to +// flatten every value into `field: ["key=value"]` before dispatch, so a +// number arrived as "42" and the server — correctly — refused it for a +// declared number field, making that field unwritable. An UNDECLARED number +// was worse: accepted, and silently stored as a string. +// +// The assertion is on the TYPE in the outgoing body, not on the request +// succeeding, because a string "42" is exactly what the broken version sent. +func TestMapItemCreate_FieldsObjectKeepsJSONTypes(t *testing.T) { + // What the catalog merge produces for fields:{cost:42, spec:{a:[1,2]}}. + // Both forms are present by design — the string copy is what the stdio + // door consumes; this door must prefer the native one. + input := map[string]any{ + "workspace": "ws", "collection": "tasks", "title": "x", + "field": []any{"cost=42"}, + fieldsNativeKey: map[string]any{ + "cost": float64(42), + "spec": map[string]any{"a": []any{float64(1), float64(2)}}, + }, + } + _, _, body, err := mapItemCreate(input) + if err != nil { + t.Fatalf("mapItemCreate: %v", err) + } + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("decode body: %v", err) + } + var fields map[string]any + if err := json.Unmarshal([]byte(payload["fields"].(string)), &fields); err != nil { + t.Fatalf("decode fields: %v", err) + } + + if got, ok := fields["cost"].(float64); !ok || got != 42 { + t.Fatalf("fields.cost = %[1]T(%[1]v), want a JSON number — the native form must win over the stringified copy", fields["cost"]) + } + spec, ok := fields["spec"].(map[string]any) + if !ok { + t.Fatalf("fields.spec = %[1]T(%[1]v), want a JSON object", fields["spec"]) + } + if _, ok := spec["a"].([]any); !ok { + t.Fatalf("fields.spec.a = %[1]T(%[1]v), want a JSON array", spec["a"]) + } +} diff --git a/internal/mcp/instructions.md b/internal/mcp/instructions.md index c40d310ca..e9035ef9c 100644 --- a/internal/mcp/instructions.md +++ b/internal/mcp/instructions.md @@ -6,7 +6,7 @@ Pad is a project tracker for developers and AI agents — issues (TASK, BUG), pl If the user is asking general code questions with no project-management thread, you don't need this server. -## Tool surface (v0.26) +## Tool surface (v0.27) Ten resource × action tools, plus `pad_set_workspace` (which takes a `workspace` slug only — no action enum). Eleven tools total. diff --git a/internal/mcp/version.go b/internal/mcp/version.go index 36ce7c938..735074888 100644 --- a/internal/mcp/version.go +++ b/internal/mcp/version.go @@ -714,7 +714,82 @@ const CmdhelpVersion = "0.1" // That reliance was indistinguishable from a bug in the caller // (the key never did anything), so the break is the fix. Single // bump covers both halves; they are one contract change. -const ToolSurfaceVersion = "0.26" +// +// 0.27 — BUG-2850. Field values are TYPED SERVER-SIDE, the `fields` +// object reaches the remote door with its JSON types intact, and +// the fields/field merge refuses several ambiguities it used to +// resolve silently. +// +// (1) Coercion. A string arriving for a declared number/json field +// is coerced against the collection schema before validation, at +// all eight Validate* call sites. The remote /mcp transport builds +// its field map in ingestFieldKVP, so EVERY value reached the +// server as a string and a declared number or json field was +// unwritable from that transport — a 400 on a correct value in the +// wrong clothes. Uncoercible values are still refused with the +// validator's existing message. +// +// (2) The `fields` OBJECT carries native types. The catalog no +// longer stringifies it; the HTTP mappers apply it last so the +// types survive. The key=value doors (CLI --field, stdio MCP via +// the CLI, remote field:[…]) are string-by-construction and +// unchanged — the fidelity is available wherever the encoding +// carries it, not everywhere. +// +// (3) Undeclared keys are NAMED. Item create/update responses may +// carry `warnings.undeclared_fields`; the CLI prints the same list +// to stderr, never stdout. Additive and omitempty, so a clean +// write is byte-identical to before. Keys are ACCEPTED, not +// refused: a census of 1012 items found 14 undeclared keys across +// 168 live values (priority alone at 127), so refusing would have +// broken read-modify-write on items nobody had edited wrongly. +// +// (4) ONE conflict check over a canonical view of every source. +// The `fields` object, the `field:[]` entries, the promoted named +// params and the v0.16 compat IDs are resolved to a canonical key +// and adjudicated once. What this REFUSES that 0.26 permitted: +// +// - two names for one target in a single call — parent/plan, +// assign/assigned_user_id, role/agent_role_id — refused even +// when the values match, because the names address one thing +// through different vocabularies (a slug and a UUID are not +// comparable), and the doors resolved them differently; +// +// - the same key supplied through the `fields` object and +// another source with DIFFERING values (equal ones collapse to +// one write); +// +// - a non-string `assign` / `role`, which one door silently +// dropped and the other rejected; +// +// - an empty hierarchy value inside `fields`, which promoted +// onto a param both doors read as "not supplied" and so +// reported success having detached nothing. Use clear_parent. +// +// NOT refused, deliberately — but NARROWER than it first reads. A +// SCHEMA-DECLARED param colliding with a `field:[]` entry under the +// same name, with no `fields` object, still resolves: the CLI has a +// real flag for such a param, so stdio receives BOTH forms and its +// overlay order resolves them exactly as the HTTP mapper does. It +// is visibly a duplicate, so last-write-wins is a resolution the +// caller can predict. Pinned per door so they cannot drift apart. +// +// The v0.16 compat IDs are NOT exempt, and being undeclared is +// precisely why. BuildCLIArgs emits the CLI's real flags and there +// is none behind `assigned_user_id`, so the top-level value is +// DROPPED: stdio sees only the field entry while HTTP reads the +// param. Two different people assigned from one call, with no +// `fields` object anywhere. Last-write-wins cannot be the answer +// when the doors do not receive the same writes, so that pair +// refuses. +// +// Bump rationale: (1)-(3) are additive or fix outright breakage, +// but (4) refuses calls 0.26 accepted. Same grounds as 0.26, 0.25, +// 0.16, 0.10 and 0.9 — no tool name, action enum or parameter +// shape changed, and the behaviour did. Every refusal added here +// replaced a call that SUCCEEDED while doing something other than +// what it said, so the break is the fix in each case. +const ToolSurfaceVersion = "0.27" // MetaVersionURI is the canonical URI of the queryable version document. // Lives outside the pad://workspace/{ws}/... namespace because it's a diff --git a/internal/models/item.go b/internal/models/item.go index 2eeaec164..611c6024d 100644 --- a/internal/models/item.go +++ b/internal/models/item.go @@ -58,6 +58,19 @@ var reservedItemFieldKeys = map[string]struct{}{ ItemFieldConvention: {}, } +// ItemWriteWarnings carries advisory notes about a write that succeeded +// (BUG-2850). Nothing here is an error: every field named has been stored. +// +// UndeclaredFields lists keys written into the item's fields blob that the +// collection's schema does not declare. Those keys are ACCEPTED — a census of +// this deployment found 168 live values under 14 such keys, and refusing them +// would have broken read-modify-write on items nobody edited wrongly — but a +// typo and a deliberate extra field are indistinguishable once stored, so the +// write says which keys it did not recognize instead of staying silent. +type ItemWriteWarnings struct { + UndeclaredFields []string `json:"undeclared_fields,omitempty"` +} + // IsReservedItemField reports whether key is system-written metadata rather than // a user-facing schema field. Callers that filter, migrate, or render an item's // fields map should consult this rather than enumerating the constants. @@ -163,6 +176,17 @@ type Item struct { ParentSlug string `json:"parent_slug,omitempty"` ParentCollectionSlug string `json:"parent_collection_slug,omitempty"` + // Warnings is populated on WRITE responses only (create / update), never + // on reads, and is never stored. It carries things the write did that the + // caller might not have meant — today, field keys the collection's schema + // does not declare (BUG-2850). + // + // Additive and omitempty: a client that does not know the key ignores it, + // and a write with nothing to report is byte-identical to before. The + // alternative — wrapping the response as {item, warnings} — would have + // broken every existing parser of the item write response. + Warnings *ItemWriteWarnings `json:"warnings,omitempty"` + // HasChildren is true if this item has child items linked to it. // Populated by enrichment, not stored in the DB. HasChildren bool `json:"has_children,omitempty"` diff --git a/internal/server/handlers_artifact_import.go b/internal/server/handlers_artifact_import.go index 7661ace1a..99cdc9da5 100644 --- a/internal/server/handlers_artifact_import.go +++ b/internal/server/handlers_artifact_import.go @@ -207,6 +207,34 @@ func (s *Server) handleImportArtifact(w http.ResponseWriter, r *http.Request) { return } + // Carry the create's own warnings into the import response (BUG-2850). + // + // CORRECTION. Round 10 removed this merge as unreachable, on the grounds + // that artifact.Decode populates Fields only from FieldKeysForKind, so no + // undeclared key could arrive. That check was real but it was the WRONG + // SIDE of the comparison: UndeclaredFieldKeys compares the field map + // against the DESTINATION COLLECTION'S SCHEMA, not against the artifact + // format's key list. The destination schema is editable, so a canonical + // artifact key can be undeclared THERE while being perfectly legal in the + // artifact. + // + // Reachable and verified (round 11, and pinned by + // TestImportArtifactReportsUndeclaredFieldsAgainstANarrowedSchema): + // narrow the conventions collection's schema to declare only `status`, + // import a convention carrying trigger/scope/priority — all three are + // stored in the blob and UndeclaredFieldKeys names all three, so + // createItemChecked sets item.Warnings and this handler dropped them. + // + // An import is exactly where that matters: the forgiving preprocessing + // above exists because a foreign artifact's vocabulary may not match the + // destination, and a key that survived into the blob unrecognized is the + // same class of news as the values this handler already reports. + if item.Warnings != nil { + for _, key := range item.Warnings.UndeclaredFields { + warnings = append(warnings, fmt.Sprintf("field %q is not declared by the destination collection's schema; stored as-is", key)) + } + } + writeJSON(w, http.StatusCreated, artifactImportResponse{ Ref: item.Ref, Slug: item.Slug, diff --git a/internal/server/handlers_artifact_import_warnings_test.go b/internal/server/handlers_artifact_import_warnings_test.go new file mode 100644 index 000000000..b64996e7b --- /dev/null +++ b/internal/server/handlers_artifact_import_warnings_test.go @@ -0,0 +1,115 @@ +package server + +// BUG-2850: the artifact-import door must report the create's +// undeclared-field warnings. +// +// This PR's claim is that an undeclared key is STORED and NAMED, so a typo +// leaves a trace. createItemChecked computes exactly that; the artifact-import +// handler builds its OWN response shape and was returning only the +// preprocessing warnings. +// +// READ THE SETUP BEFORE CHANGING IT — it is what makes the case reachable, +// and I got this wrong once. Round 10 dismissed the finding as unreachable +// because artifact.Decode populates Fields only from FieldKeysForKind. That +// check was true and irrelevant: UndeclaredFieldKeys compares the field map +// against the DESTINATION COLLECTION'S SCHEMA, not against the artifact +// format's key list. So the reachable shape is not a weird artifact — it is +// an ordinary artifact meeting a NARROWED destination schema, which is what +// this test builds. +// +// The trait is unique per kind (a second collection declaring +// artifact_kind=convention is refused 409), so the destination is always the +// seeded conventions collection — narrowing its schema, not adding a rival +// collection, is the route. + +import ( + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/artifact" +) + +func TestImportArtifactReportsUndeclaredFieldsAgainstANarrowedSchema(t *testing.T) { + srv := testServer(t) + ws := createWSForTest(t, srv) + + // Narrow the destination so trigger/scope/priority — all canonical + // convention artifact fields — are no longer declared there. + rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+ws+"/collections/conventions", map[string]any{ + "schema": `{"fields":[{"key":"status","type":"select","options":["draft","active"]}]}`, + }) + if rr.Code != http.StatusOK { + t.Fatalf("narrow conventions schema: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + + art := artifact.Artifact{ + Kind: artifact.KindConvention, + FormatVersion: artifact.FormatVersion, + Title: "Convention meeting a narrowed destination", + Fields: map[string]any{"status": "active", "trigger": "on-commit", "scope": "all", "priority": "must"}, + Body: "Perfectly legal artifact; the destination just declares less.\n", + } + data, err := artifact.Encode(art) + if err != nil { + t.Fatalf("encode: %v", err) + } + + rr2 := doArtifactRequest(srv, "POST", "/api/v1/workspaces/"+ws+"/import-artifact", data) + if rr2.Code != http.StatusCreated { + t.Fatalf("import: expected 201, got %d: %s", rr2.Code, rr2.Body.String()) + } + + var resp struct { + Ref string `json:"ref"` + Warnings []string `json:"warnings"` + } + if err := json.Unmarshal(rr2.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v\n%s", err, rr2.Body.String()) + } + + joined := strings.Join(resp.Warnings, "\n") + for _, key := range []string{"trigger", "scope", "priority"} { + if !strings.Contains(joined, key) { + t.Errorf("undeclared key %q was stored but not named in the import warnings; got %v", key, resp.Warnings) + } + } +} + +// ...and an import into an INTACT destination reports no undeclared-field +// warning, so the channel does not cry wolf on the ordinary case. This is +// also the control that shows the test above is testing the narrowing and not +// merely the presence of the keys. +func TestImportArtifactAgainstTheDefaultSchemaNamesNothingUndeclared(t *testing.T) { + srv := testServer(t) + ws := createWSForTest(t, srv) + + art := artifact.Artifact{ + Kind: artifact.KindConvention, + FormatVersion: artifact.FormatVersion, + Title: "Convention meeting the seeded destination", + Fields: map[string]any{"status": "active", "trigger": "on-commit", "scope": "all", "priority": "must"}, + Body: "Nothing foreign here.\n", + } + data, err := artifact.Encode(art) + if err != nil { + t.Fatalf("encode: %v", err) + } + + rr := doArtifactRequest(srv, "POST", "/api/v1/workspaces/"+ws+"/import-artifact", data) + if rr.Code != http.StatusCreated { + t.Fatalf("import: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var resp struct { + Warnings []string `json:"warnings"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v\n%s", err, rr.Body.String()) + } + for _, w := range resp.Warnings { + if strings.Contains(w, "not declared") { + t.Errorf("an artifact matching its destination must not produce an undeclared-field warning; got %q", w) + } + } +} diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index f82da53ee..ad33445e8 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -744,9 +744,15 @@ func (e *itemCreateError) Error() string { return e.message } // Returns the created item (Ref/Slug populated by the store) or an // *itemCreateError with a status hint. func (s *Server) createItemChecked(r *http.Request, workspaceID string, coll *models.Collection, schema models.CollectionSchema, input models.ItemCreate, fieldMap map[string]any, parentValue string) (*models.Item, *itemCreateError) { + // Coerce strings to their declared types before validating (BUG-2850). + fieldMap = items.CoerceFields(fieldMap, schema) if err := items.ValidateFields(fieldMap, schema); err != nil { return nil, &itemCreateError{http.StatusBadRequest, "validation_error", err.Error()} } + // Keys the schema does not declare are STORED, not refused — but the write + // says which ones, so a typo leaves a trace (BUG-2850). Computed before the + // store call because fieldMap is what gets written. + undeclared := items.UndeclaredFieldKeys(fieldMap, schema) if err := s.checkUniqueFields(workspaceID, coll.ID, "", schema, fieldMap); err != nil { return nil, &itemCreateError{http.StatusConflict, "conflict", err.Error()} @@ -864,6 +870,12 @@ func (s *Server) createItemChecked(r *http.Request, workspaceID string, coll *mo }) } + // Attach after the write succeeded: these are advisory notes about a + // stored item, not a reason to refuse one (BUG-2850). + if len(undeclared) > 0 { + item.Warnings = &models.ItemWriteWarnings{UndeclaredFields: undeclared} + } + return item, nil } @@ -984,6 +996,9 @@ func writeInvalidItemTitle(w http.ResponseWriter, err error) bool { } func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { + // Keys this write puts in the fields blob that the schema does not + // declare. Stored, not refused; reported on the response (BUG-2850). + var undeclaredFields []string workspaceID, ok := s.getWorkspaceID(w, r) if !ok { return @@ -1184,10 +1199,13 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { return } + // Coerce strings to their declared types before validating (BUG-2850). + fieldMap = items.CoerceFields(fieldMap, schema) if err := items.ValidateFields(fieldMap, schema); err != nil { writeError(w, http.StatusBadRequest, "validation_error", err.Error()) return } + undeclaredFields = items.UndeclaredFieldKeys(fieldMap, schema) if err := s.checkUniqueFields(workspaceID, item.CollectionID, item.ID, schema, fieldMap); err != nil { writeError(w, http.StatusConflict, "conflict", err.Error()) @@ -1340,6 +1358,26 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { return } + // Coerce strings to their declared types before validating (BUG-2850). + patchMap = items.CoerceFields(patchMap, schema) + // Only the PATCHED keys are reported. A stray key already on the item + // is not something this write introduced, and naming it every time + // anyone touches the item would train the reader to ignore the field. + // + // And only keys the patch STORES. A nil value in fields_patch is a + // DELETE — internal/store/items.go removes the key — so reporting it as + // an undeclared field would tell the caller a field was stored that the + // same request just removed (codex round 2). Filtered here rather than + // inside UndeclaredFieldKeys because nil means "store JSON null" on the + // full-fields path, where reporting it IS correct. + stored := make(map[string]any, len(patchMap)) + for k, v := range patchMap { + if v == nil { + continue + } + stored[k] = v + } + undeclaredFields = items.UndeclaredFieldKeys(stored, schema) if err := items.ValidatePartialFields(patchMap, schema); err != nil { writeError(w, http.StatusBadRequest, "validation_error", err.Error()) return @@ -2018,6 +2056,11 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { return } + // Advisory, post-write, same as create (BUG-2850). + if len(undeclaredFields) > 0 { + updated.Warnings = &models.ItemWriteWarnings{UndeclaredFields: undeclaredFields} + } + writeJSON(w, http.StatusOK, updated) } @@ -2267,6 +2310,8 @@ func (s *Server) handleMoveItem(w http.ResponseWriter, r *http.Request) { // `missing_required_fields` code and message shape, because CLI and // web callers key off it; genuinely invalid VALUES get their own // `invalid_fields` code rather than being mislabelled as missing. + // Coerce strings to their declared types before validating (BUG-2850). + result.Fields = items.CoerceFields(result.Fields, items.SchemaForMigratedFields(targetSchema)) if issues := items.ValidateFieldsDetailed(result.Fields, items.SchemaForMigratedFields(targetSchema)); len(issues) > 0 { var missing, invalid []string for _, iss := range issues { diff --git a/internal/server/handlers_items_bulk.go b/internal/server/handlers_items_bulk.go index 73a75ab54..cfb09560c 100644 --- a/internal/server/handlers_items_bulk.go +++ b/internal/server/handlers_items_bulk.go @@ -495,6 +495,8 @@ func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *mode fieldMap[k] = v } + // Coerce strings to their declared types before validating (BUG-2850). + fieldMap = items.CoerceFields(fieldMap, schema) if err := items.ValidateFields(fieldMap, schema); err != nil { return nil, &bulkOpError{message: err.Error(), code: "validation_error"} } @@ -677,6 +679,8 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m // against the TARGET schema — MigrateFields validates migrated // values but an override can smuggle in a value the target schema // doesn't allow (e.g. a status not in the target's options). + // Coerce strings to their declared types before validating (BUG-2850). + result.Fields = items.CoerceFields(result.Fields, items.SchemaForMigratedFields(targetSchema)) if err := items.ValidateFields(result.Fields, items.SchemaForMigratedFields(targetSchema)); err != nil { return nil, &bulkOpError{message: err.Error(), code: "validation_error"} } diff --git a/internal/server/handlers_items_copy_preflight.go b/internal/server/handlers_items_copy_preflight.go index 901a5e785..ca7c863b5 100644 --- a/internal/server/handlers_items_copy_preflight.go +++ b/internal/server/handlers_items_copy_preflight.go @@ -644,6 +644,11 @@ func (s *Server) handleCopyItemPreflight(w http.ResponseWriter, r *http.Request) // ValidateFieldsDetailed injects any remaining schema defaults into // `final` in place, so a key that appears only afterwards has no // origin entry and is reported as "default". + // Coerce strings to their declared types before validating (BUG-2850). + // MUST match the store-side copy (items_cross_workspace_copy.go): the + // preflight exists to PREDICT what the copy does, so a coercion on one + // side only would make it report a field as failing that the copy accepts. + final = items.CoerceFields(final, items.SchemaForMigratedFields(targetSchema)) issues := items.ValidateFieldsDetailed(final, items.SchemaForMigratedFields(targetSchema)) // DR-12's other half: an override whose VALUE is invalid is rejected, diff --git a/internal/server/handlers_items_field_coercion_test.go b/internal/server/handlers_items_field_coercion_test.go new file mode 100644 index 000000000..3311f5759 --- /dev/null +++ b/internal/server/handlers_items_field_coercion_test.go @@ -0,0 +1,717 @@ +package server + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-2850 at the HTTP door — the door that was broken. +// +// The remote /mcp transport builds its field map in ingestFieldKVP with +// `dst[key] = val`, so every value reaches this handler as a STRING. Before +// the fix, validateFieldType then refused a string for a declared number or +// json field and the write 400'd: an MCP agent on that transport could not +// write those fields at all. The CLI and local stdio MCP were unaffected — +// they coerce by schema before the request is built — which is why this +// reproduced only against Pad Cloud. +// +// These assert the stored NATIVE TYPE, not that the request succeeded. A test +// that only checked for 201 would pass on an implementation that stored the +// string, which is the shape the reporter described. +func TestItemFieldsCoercedFromStringsAtTheHTTPDoor(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Coercion Test"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + // A collection whose schema declares the two types the bug made unwritable. + schema := `{"fields":[{"key":"cost","type":"number"},{"key":"spec","type":"json"},{"key":"note","type":"text"}]}` + rr = doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+ws.Slug+"/collections", + map[string]interface{}{"name": "Jobs", "schema": schema}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + + // Exactly what ingestFieldKVP produces: every value a string. + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+coll.Slug+"/items", + map[string]interface{}{ + "title": "from the remote mcp door", + "fields": `{"cost":"42","spec":"[{\"name\":\"a\"}]","note":"42"}`, + }, + map[string]string{"Authorization": "Bearer " + sessionToken}, + ) + if rr.Code != http.StatusCreated { + t.Fatalf("expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var created models.Item + parseJSON(t, rr, &created) + + stored := map[string]any{} + if err := json.Unmarshal([]byte(created.Fields), &stored); err != nil { + t.Fatalf("stored fields are not JSON: %v (%s)", err, created.Fields) + } + + if got, ok := stored["cost"].(float64); !ok || got != 42 { + t.Fatalf("cost: want a JSON number, got %[1]T(%[1]v) — stored blob: %s", stored["cost"], created.Fields) + } + if _, ok := stored["spec"].([]any); !ok { + t.Fatalf("spec: want a JSON array, got %[1]T(%[1]v) — stored blob: %s", stored["spec"], created.Fields) + } + // The text field holding "42" must STAY a string. Fixing the bug by + // coercing anything that parses would silently retype real data. + if got, ok := stored["note"].(string); !ok || got != "42" { + t.Fatalf("note: want the string \"42\" untouched, got %[1]T(%[1]v)", stored["note"]) + } +} + +// The same door on UPDATE, which is a separate call site and would not have +// been covered by the create test above (BUG-2850 wires eight sites). +func TestItemFieldsCoercedFromStringsOnUpdate(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Coercion Update"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + schema := `{"fields":[{"key":"cost","type":"number"}]}` + rr = doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+ws.Slug+"/collections", + map[string]interface{}{"name": "Jobs", "schema": schema}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+coll.Slug+"/items", + map[string]interface{}{"title": "job", "fields": `{}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("create item: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + rr = doRequestWithHeaders(srv, "PATCH", + "/api/v1/workspaces/"+ws.Slug+"/items/"+item.Slug, + map[string]interface{}{"fields": `{"cost":"99.5"}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusOK { + t.Fatalf("update: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var updated models.Item + parseJSON(t, rr, &updated) + + stored := map[string]any{} + if err := json.Unmarshal([]byte(updated.Fields), &stored); err != nil { + t.Fatalf("stored fields are not JSON: %v (%s)", err, updated.Fields) + } + if got, ok := stored["cost"].(float64); !ok || got != 99.5 { + t.Fatalf("cost: want a JSON number, got %[1]T(%[1]v) — stored blob: %s", stored["cost"], updated.Fields) + } +} + +// fields_patch is a THIRD call site (the field-level merge from IDEA-1480), +// separate from the full-fields update above. CONVE-19: wiring is a claim, so +// each site that threads CoerceFields needs a test that fails if that site +// alone is missed — this one caught nothing when written, which is the point: +// it exists so a future edit that drops the call here is red. +func TestItemFieldsCoercedOnFieldsPatch(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Coercion Patch"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + schema := `{"fields":[{"key":"cost","type":"number"},{"key":"note","type":"text"}]}` + rr = doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+ws.Slug+"/collections", + map[string]interface{}{"name": "Jobs", "schema": schema}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+coll.Slug+"/items", + map[string]interface{}{"title": "job", "fields": `{"note":"keep me"}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("create item: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + rr = doRequestWithHeaders(srv, "PATCH", + "/api/v1/workspaces/"+ws.Slug+"/items/"+item.Slug, + map[string]interface{}{"fields_patch": map[string]any{"cost": "7"}}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusOK { + t.Fatalf("fields_patch: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var updated models.Item + parseJSON(t, rr, &updated) + + stored := map[string]any{} + if err := json.Unmarshal([]byte(updated.Fields), &stored); err != nil { + t.Fatalf("stored fields are not JSON: %v (%s)", err, updated.Fields) + } + if got, ok := stored["cost"].(float64); !ok || got != 7 { + t.Fatalf("cost: want a JSON number, got %[1]T(%[1]v) — stored blob: %s", stored["cost"], updated.Fields) + } + // The merge must not have eaten the untouched field. + if got, ok := stored["note"].(string); !ok || got != "keep me" { + t.Fatalf("note: want it preserved by the merge, got %[1]T(%[1]v)", stored["note"]) + } +} + +// A value that will not coerce must still be REFUSED, with the validator's +// existing message. Coercion removes the cases where a correct value arrived +// in the wrong clothes; it must not start accepting wrong values. +func TestUncoercibleFieldValueStillRefused(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Coercion Refusal"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + schema := `{"fields":[{"key":"cost","type":"number"}]}` + rr = doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+ws.Slug+"/collections", + map[string]interface{}{"name": "Jobs", "schema": schema}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+coll.Slug+"/items", + map[string]interface{}{"title": "bad", "fields": `{"cost":"not-a-number"}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for an un-coercible number, got %d: %s", rr.Code, rr.Body.String()) + } +} + +// The preflight and the copy must coerce IDENTICALLY (BUG-2850). +// +// They validate in different PACKAGES — the preflight in +// handlers_items_copy_preflight.go, the copy in +// internal/store/items_cross_workspace_copy.go — which is exactly how they +// would drift unnoticed. The preflight exists to PREDICT what the copy does, +// so coercion on one side only makes it report a field as failing that the +// copy accepts, which is worse than either behaviour alone. +// +// Both files carry a comment saying they must match. This is the test that +// makes that comment more than a wish. +func TestCopyAndPreflightCoerceIdentically(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + mkWS := func(name string) models.Workspace { + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": name}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws %s: expected 201, got %d: %s", name, rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + return ws + } + mkColl := func(ws models.Workspace, name, schema string) models.Collection { + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+ws.Slug+"/collections", + map[string]interface{}{"name": name, "schema": schema}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection %s: expected 201, got %d: %s", name, rr.Code, rr.Body.String()) + } + var c models.Collection + parseJSON(t, rr, &c) + return c + } + + src := mkWS("Coercion Copy Src") + dst := mkWS("Coercion Copy Dst") + schema := `{"fields":[{"key":"cost","type":"number"}]}` + srcColl := mkColl(src, "Jobs", schema) + dstColl := mkColl(dst, "Jobs", schema) + + rr := doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+src.Slug+"/collections/"+srcColl.Slug+"/items", + map[string]interface{}{"title": "job", "fields": `{"cost":1}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("create item: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + // A STRING override for a number field — what the remote MCP door sends. + body := map[string]interface{}{ + "target_workspace": dst.Slug, + "target_collection": dstColl.Slug, + "field_overrides": map[string]any{"cost": "42"}, + } + + pre := doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+src.Slug+"/items/"+item.Slug+"/copy/preflight", + body, map[string]string{"Authorization": "Bearer " + sessionToken}) + cp := doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+src.Slug+"/items/"+item.Slug+"/copy", + body, map[string]string{"Authorization": "Bearer " + sessionToken}) + + // The agreement itself: whatever they do, they must do the same thing. A + // preflight that previews success against a copy that refuses (or the + // reverse) is the failure this guards, independent of which is "right". + preOK := pre.Code >= 200 && pre.Code < 300 + cpOK := cp.Code >= 200 && cp.Code < 300 + if preOK != cpOK { + t.Fatalf("preflight and copy disagreed on a string override for a number field:\n"+ + " preflight: %d %s\n copy: %d %s", + pre.Code, pre.Body.String(), cp.Code, cp.Body.String()) + } + + // And with coercion in place, both accept and the copy stores a NUMBER. + if !cpOK { + t.Fatalf("copy refused a coercible override: %d %s", cp.Code, cp.Body.String()) + } + // The copy answers with a wrapper, not a bare item. + var result struct { + Item *models.Item `json:"item"` + } + parseJSON(t, cp, &result) + if result.Item == nil { + t.Fatalf("copy response carried no item: %s", cp.Body.String()) + } + stored := map[string]any{} + if err := json.Unmarshal([]byte(result.Item.Fields), &stored); err != nil { + t.Fatalf("copied fields are not JSON: %v (%s)", err, result.Item.Fields) + } + if got, ok := stored["cost"].(float64); !ok || got != 42 { + t.Fatalf("copied cost: want a JSON number 42, got %[1]T(%[1]v) — blob: %s", stored["cost"], result.Item.Fields) + } +} + +// The parity pin the ruling asked for: an UNDECLARED number and an UNDECLARED +// object, asserting the stored native type (BUG-2850). +// +// Undeclared keys are accepted — a census found 168 live values under 14 such +// keys in this deployment, and refusing them would have broken read-modify- +// write on items nobody edited wrongly. What changed is that their JSON type +// survives where the encoding carries one. This is the HTTP door, which +// carries it; the key=value doors are string-by-construction and their row is +// the string, asserted in the MCP package. +// +// It also pins the WARNING: the write names the keys it did not recognize, so +// a typo leaves a trace. Both halves in one test because they describe one +// write. +func TestUndeclaredFieldsKeepTheirTypeAndAreNamed(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Undeclared Pin"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + // The schema declares NOTHING but status — every key below is undeclared. + schema := `{"fields":[{"key":"status","type":"text"}]}` + rr = doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+ws.Slug+"/collections", + map[string]interface{}{"name": "Jobs", "schema": schema}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+coll.Slug+"/items", + map[string]interface{}{ + "title": "undeclared pin", + "fields": `{"materials_cost":42,"spec":{"a":[1,2]}}`, + }, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("expected 201 — undeclared keys are ACCEPTED, got %d: %s", rr.Code, rr.Body.String()) + } + var created models.Item + parseJSON(t, rr, &created) + + stored := map[string]any{} + if err := json.Unmarshal([]byte(created.Fields), &stored); err != nil { + t.Fatalf("stored fields are not JSON: %v (%s)", err, created.Fields) + } + if got, ok := stored["materials_cost"].(float64); !ok || got != 42 { + t.Fatalf("undeclared number: want a JSON number, got %[1]T(%[1]v) — blob: %s", stored["materials_cost"], created.Fields) + } + obj, ok := stored["spec"].(map[string]any) + if !ok { + t.Fatalf("undeclared object: want a JSON object, got %[1]T(%[1]v) — blob: %s", stored["spec"], created.Fields) + } + if _, ok := obj["a"].([]any); !ok { + t.Fatalf("undeclared object: nested array lost, got %[1]T(%[1]v)", obj["a"]) + } + + // And the write says which keys it did not recognize. + if created.Warnings == nil { + t.Fatalf("expected warnings naming the undeclared keys, got none: %s", rr.Body.String()) + } + got := created.Warnings.UndeclaredFields + if len(got) != 2 || got[0] != "materials_cost" || got[1] != "spec" { + t.Fatalf("undeclared_fields = %v, want [materials_cost spec] (sorted)", got) + } +} + +// A write with nothing to report carries NO warnings element, so the response +// stays byte-identical to before for every well-formed write (BUG-2850). +func TestNoWarningsWhenEveryFieldIsDeclared(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "No Warnings"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + schema := `{"fields":[{"key":"cost","type":"number"}]}` + rr = doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+ws.Slug+"/collections", + map[string]interface{}{"name": "Jobs", "schema": schema}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+coll.Slug+"/items", + map[string]interface{}{"title": "clean", "fields": `{"cost":"5"}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + if strings.Contains(rr.Body.String(), "warnings") { + t.Fatalf("a clean write must carry no warnings element: %s", rr.Body.String()) + } +} + +// A nil value in fields_patch DELETES the key, so it must not be reported as +// an undeclared field that was stored (BUG-2850, codex round 2). The warning +// would otherwise tell the caller a field exists that the same request removed. +func TestFieldsPatchDeleteIsNotReportedAsUndeclared(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Patch Delete"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + schema := `{"fields":[{"key":"status","type":"text"}]}` + rr = doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+ws.Slug+"/collections", + map[string]interface{}{"name": "Jobs", "schema": schema}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+coll.Slug+"/items", + map[string]interface{}{"title": "job", "fields": `{"stray":"value"}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("create item: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + // Now DELETE that key via fields_patch. + rr = doRequestWithHeaders(srv, "PATCH", + "/api/v1/workspaces/"+ws.Slug+"/items/"+item.Slug, + map[string]interface{}{"fields_patch": map[string]any{"stray": nil}}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusOK { + t.Fatalf("patch delete: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var updated models.Item + parseJSON(t, rr, &updated) + + if updated.Warnings != nil && len(updated.Warnings.UndeclaredFields) > 0 { + t.Fatalf("a DELETE must not be reported as a stored undeclared field: %v", + updated.Warnings.UndeclaredFields) + } + stored := map[string]any{} + if err := json.Unmarshal([]byte(updated.Fields), &stored); err != nil { + t.Fatalf("stored fields are not JSON: %v", err) + } + if _, still := stored["stray"]; still { + t.Fatalf("the key should have been deleted: %s", updated.Fields) + } +} + +// The three remaining coercion call sites, pinned (BUG-2850, lead ruling at +// the day-11 boot). +// +// The PR's claim is "typed on every door". Five of the eight `CoerceFields` +// sites had a test that goes red if that site alone is dropped; move, bulk +// move and bulk update did not, so the claim rested on reading the code. That +// is CONVE-19 and PATTE-128's exact shape, and it is what rounds 2-5 of this +// unit kept finding: wiring is a claim. +// +// WHAT MAKES THE MOVE PINS FAITHFUL — this is the part worth reading before +// trusting them. It would be easy to write a move test that passes for the +// wrong reason, because `migrateValue` already handles a text→number move +// (migrate.go:190). But look at what it returns on success: `value`, the +// ORIGINAL — not the parsed float. So a text field holding "42" lands in a +// number-typed destination as the STRING "42", and it is `CoerceFields` at +// the move site, and only that, which turns it into a JSON number before +// `ValidateFieldsDetailed` sees it. Drop the call and the move 400s with +// `invalid_fields`. The assertion is on the STORED NATIVE TYPE rather than on +// the status code, so an implementation that answered 200 and stored the +// string would still be red. + +// Single-item move: POST /items/{slug}/move (handlers_items.go, the +// SchemaForMigratedFields site). +func TestItemFieldsCoercedOnMove(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Coercion Move"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + // Source declares cost as TEXT, destination as NUMBER. The move migrates + // the value across (text→number is a permitted conversion) but carries the + // string verbatim. + src := createCollectionForCoercion(t, srv, sessionToken, ws.Slug, "Drafts", + `{"fields":[{"key":"cost","type":"text"}]}`) + dst := createCollectionForCoercion(t, srv, sessionToken, ws.Slug, "Jobs", + `{"fields":[{"key":"cost","type":"number"}]}`) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+src.Slug+"/items", + map[string]interface{}{"title": "job", "fields": `{"cost":"42"}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("create item: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/items/"+item.Slug+"/move", + map[string]interface{}{"target_collection": dst.Slug}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusOK { + t.Fatalf("move: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + + assertStoredNumber(t, srv, sessionToken, ws.Slug, item.Slug, "cost", 42) +} + +// Bulk move: POST /items/bulk op=move with a target collection +// (handlers_items_bulk.go, the second SchemaForMigratedFields site). A +// separate function from the single move — bulkMoveCollection — which is why +// the test above says nothing about it. +func TestItemFieldsCoercedOnBulkMove(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Coercion Bulk Move"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + src := createCollectionForCoercion(t, srv, sessionToken, ws.Slug, "Drafts", + `{"fields":[{"key":"cost","type":"text"}]}`) + dst := createCollectionForCoercion(t, srv, sessionToken, ws.Slug, "Jobs", + `{"fields":[{"key":"cost","type":"number"}]}`) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+src.Slug+"/items", + map[string]interface{}{"title": "job", "fields": `{"cost":"42"}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("create item: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/items/bulk", + map[string]interface{}{"op": "move", "ids": []string{item.ID}, "collection": dst.Slug}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + // A bulk op answers 200 and reports per-item failures in the envelope, so + // the status code alone proves nothing — read the envelope. + if rr.Code != http.StatusOK { + t.Fatalf("bulk move: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + assertBulkAllSucceeded(t, rr) + + assertStoredNumber(t, srv, sessionToken, ws.Slug, item.Slug, "cost", 42) +} + +// Bulk update: POST /items/bulk op=set-priority (handlers_items_bulk.go's +// bulkFieldUpdate — a third site, reached by set-priority and by status +// moves). +// +// The change values this site merges are request STRINGS, so it is observable +// only where the schema declares one of those keys as a non-string type. A +// collection declaring `priority` as a number is unusual but entirely legal — +// numeric priorities are a real convention — and it is the honest way to +// exercise this site. With coercion dropped, ValidateFields refuses the string +// and the item lands in the envelope's `failed` list. +func TestItemFieldsCoercedOnBulkFieldUpdate(t *testing.T) { + t.Parallel() + srv := testServer(t) + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "Coercion Bulk Update"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create ws: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws models.Workspace + parseJSON(t, rr, &ws) + + coll := createCollectionForCoercion(t, srv, sessionToken, ws.Slug, "Jobs", + `{"fields":[{"key":"priority","type":"number"}]}`) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/collections/"+coll.Slug+"/items", + map[string]interface{}{"title": "job", "fields": `{}`}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusCreated { + t.Fatalf("create item: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + rr = doRequestWithHeaders(srv, "POST", + "/api/v1/workspaces/"+ws.Slug+"/items/bulk", + map[string]interface{}{"op": "set-priority", "ids": []string{item.ID}, "priority": "3"}, + map[string]string{"Authorization": "Bearer " + sessionToken}) + if rr.Code != http.StatusOK { + t.Fatalf("bulk set-priority: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + assertBulkAllSucceeded(t, rr) + + assertStoredNumber(t, srv, sessionToken, ws.Slug, item.Slug, "priority", 3) +} + +func createCollectionForCoercion(t *testing.T, srv *Server, token, wsSlug, name, schema string) models.Collection { + t.Helper() + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces/"+wsSlug+"/collections", + map[string]interface{}{"name": name, "schema": schema}, token) + if rr.Code != http.StatusCreated { + t.Fatalf("create collection %q: expected 201, got %d: %s", name, rr.Code, rr.Body.String()) + } + var coll models.Collection + parseJSON(t, rr, &coll) + return coll +} + +// assertBulkAllSucceeded fails if any item landed in the envelope's `failed` +// list. Without this a dropped coercion reads as a passing 200. +func assertBulkAllSucceeded(t *testing.T, rr *httptest.ResponseRecorder) { + t.Helper() + var resp struct { + Updated []struct { + Ref string `json:"ref"` + } `json:"updated"` + Failed []struct { + Ref string `json:"ref"` + Error string `json:"error"` + } `json:"failed"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("bulk response is not JSON: %v (%s)", err, rr.Body.String()) + } + if len(resp.Failed) > 0 { + t.Fatalf("bulk op reported %d failure(s): %+v", len(resp.Failed), resp.Failed) + } + if len(resp.Updated) == 0 { + t.Fatalf("bulk op updated nothing: %s", rr.Body.String()) + } +} + +// assertStoredNumber re-reads the item and asserts the field is stored as a +// JSON NUMBER with the expected value. Re-reading rather than trusting the +// mutation's own response body, so the pin covers what was persisted. +func assertStoredNumber(t *testing.T, srv *Server, token, wsSlug, itemSlug, key string, want float64) { + t.Helper() + rr := doRequestWithHeaders(srv, "GET", + "/api/v1/workspaces/"+wsSlug+"/items/"+itemSlug, nil, + map[string]string{"Authorization": "Bearer " + token}) + if rr.Code != http.StatusOK { + t.Fatalf("re-read item: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + var item models.Item + parseJSON(t, rr, &item) + + stored := map[string]any{} + if err := json.Unmarshal([]byte(item.Fields), &stored); err != nil { + t.Fatalf("stored fields are not JSON: %v (%s)", err, item.Fields) + } + got, ok := stored[key].(float64) + if !ok { + t.Fatalf("%s: want a JSON number, got %[2]T(%[2]v) — stored blob: %s", key, stored[key], item.Fields) + } + if got != want { + t.Fatalf("%s: want %v, got %v", key, want, got) + } +} diff --git a/internal/store/items_cross_workspace_copy.go b/internal/store/items_cross_workspace_copy.go index 0ca7eb365..12a7ba94f 100644 --- a/internal/store/items_cross_workspace_copy.go +++ b/internal/store/items_cross_workspace_copy.go @@ -1084,6 +1084,11 @@ func migrateCopyFields(sourceFieldsJSON, sourceSchemaJSON, targetSchemaJSON stri } migrated.Fields[k] = v } + // Coerce strings to their declared types before validating (BUG-2850). + // MUST match the preflight (handlers_items_copy_preflight.go) — see the + // note there; these two live in different PACKAGES, which is exactly how + // they would drift unnoticed. + migrated.Fields = items.CoerceFields(migrated.Fields, items.SchemaForMigratedFields(targetSchema)) if err := items.ValidateFields(migrated.Fields, items.SchemaForMigratedFields(targetSchema)); err != nil { return nil, nil, &FieldValidationError{Err: err} }