From b23c0dbc6f9feef66283fa1dc7bdd16dacb4c3f6 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 3 Sep 2026 01:41:11 +0000 Subject: [PATCH] fix(mcp): apply the null and hierarchy guards to promoted keys too (BUG-2850) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 4, and both findings are the same defect in my own round-3 fix: ORDERING. The null guard and the hierarchy-key guard sat BELOW the branch that promotes status/priority/category/parent/role/assign/tags onto dedicated params, so every promoted key walked around both. - `fields: {"tags": null}` reached the promoted branch and became a silent no-op, instead of the refusal the null rule documents. - `fields: {"parent": 42}` was accepted here and dropped later by the handler — the same silent-drop shape this whole bug is about, reintroduced by a guard I added to prevent a different instance of it. Both guards now run before that branch, so they apply to every key. A guard a whole class of keys bypasses is not a guard. Pinned separately from the generic-path cases, because the generic-path tests passed throughout: they never exercised a promoted key, which is exactly why the hole survived round 3. Reverting the hoist fails the new test. Well-formed promoted keys still promote — tested, so the hoist did not break promotion while closing the bypass. Gates: gofmt clean, go vet clean, go test ./... 29 packages ok. Claude-Session: https://claude.ai/code/session_011Q4b1iHtJtSyMs7BA2ySxo --- internal/mcp/catalog_item_fields.go | 62 +++++++++++++----------- internal/mcp/catalog_item_fields_test.go | 49 +++++++++++++++++++ 2 files changed, 83 insertions(+), 28 deletions(-) diff --git a/internal/mcp/catalog_item_fields.go b/internal/mcp/catalog_item_fields.go index 21440cea..82737a06 100644 --- a/internal/mcp/catalog_item_fields.go +++ b/internal/mcp/catalog_item_fields.go @@ -173,6 +173,40 @@ 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] { + 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)) + } + } + if padItemPromotedFieldKeys[k] { existing, has := out[k] if has { @@ -191,34 +225,6 @@ func reshapeItemFields(prefix string, input map[string]any) (map[string]any, *mc out[k] = v continue } - // 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] { - 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)) - } - } - - // 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)) - } - // 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 diff --git a/internal/mcp/catalog_item_fields_test.go b/internal/mcp/catalog_item_fields_test.go index 74f951b8..5125bb19 100644 --- a/internal/mcp/catalog_item_fields_test.go +++ b/internal/mcp/catalog_item_fields_test.go @@ -473,3 +473,52 @@ func TestPadItemUpdate_StringHierarchyKeyStillAccepted(t *testing.T) { 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") + } +}