fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) by xarmian · Pull Request #1166 · PerpetualSoftware/pad

xarmian · 2026-08-20T02:52:35Z

Closes BUG-2627 (part 2, the last part) and BUG-2675, on one ToolSurfaceVersion bump as ruled.

What this closes

BUG-2627 is a family: --field implementation_notes=<json> stored the entries as a JSON-encoded STRING, which no reader can decode. Part 3 (shipped in de96cce) stopped the next pad item note destroying such a value; part 1 repaired the one affected row. Part 2 closes the door that mints it.

Refused server-side in fields_patch, not at the CLI as the item's scope line proposed. The deviation was recorded and reasoned on the trail before implementing: the CLI is one of three clients, and all three lower a user field-setter into the same key — pad item update --field (cmd_item.go), the MCP field param on remote (dispatch_http_advanced.go), and stdio by shelling out to that CLI. One gate closes all three. Both call sites were read, and the CLI's lowering is pinned by a test rather than left as an assumption — a fix that reaches one transport is the shape v0.16 shipped and v0.17 had to finish a version later.

BUG-2675 rides along: the part-3 refusal reached MCP agents as server_error — not our fault, and not transient — so an agent could reasonably retry a failure that is deterministic forever. New closed-set code stored_state_unreadable, emitted on both transports (HTTP classifies the sentinel directly; stdio via a pad-structured-error/v1: marker the CLI now writes for its own local refusal — the first marker generated without an upstream APIError).

What is deliberately NOT closed, and why

Both of these are decisions, not gaps, and both are stated where an agent will meet them rather than only here:

Item CREATE stays open. Its full-fields door is shared with Pad's own writers — pad item note / decide / github link send one, and so does convention activation via BuildConventionItemFields → ItemCreate. Closing it would break the writers the gate exists to protect. Tracked with the rest of that surface in BUG-2685.
github_pr is exempt on update. This one changed during review and is the most interesting thing in the PR. pad github link needs a local git checkout and the gh CLI, so it is excluded from remote MCP by name, and noRemoteEquivalent told remote agents to use item update --field github_pr=... instead. For that audience the patch door is not a bypass of the writer — it is the writer. Refusing it deleted a documented capability and answered with a message naming a command they cannot run. It still refuses on move/copy, where BUG-2674's argument applies instead (an override reintroduces the key the migration just dropped).

Round 4 then found that documented workaround does not work: a field value is stored as a string on every surface, so the PR data lands double-encoded — the BUG-2627 shape one key over. Filed as BUG-2696 with three candidate fixes; not folded in, because the narrowest changes how every field value is typed. The exemption stands regardless (refusing leaves remote agents with strictly less than a broken door), but every artifact that pointed agents at it now says it is broken rather than advertising it.

Codex review: 11 rounds

Rounds 1-10 found something; round 11 was CLEAN. The ones worth reading:

Round	Finding	Why it mattered
1	The refusal named `pad item note` unconditionally	On an already-broken row that command refuses too, so the caller was routed in a circle. The message now inspects the stored value and tells the truth.
2	The message's readability check and the guard it describes were two different decodes	`[1]` passed mine and failed the guard. Replaced with `models.StructuredFieldIsAppendable`, which asks the guard, plus an agreement table over 15 shapes × 2 keys.
3	P1 — `github_pr` refusal broke the only remote path	See above.
3	`fields = "null"` panicked `pad item note`	Nil map from `parseMutableItemFields`; pre-existing, reproduced, fixed, pinned by a test that fails on a panic.
4	The documented remote workaround doesn't work	BUG-2696.
5	I fixed four artifacts pointing at that workaround and missed the fifth	`noRemoteEquivalent`'s own text — the message the agent actually receives, and the one Codex had quoted at me in round 3.
7-8	The move and copy refusals classified as `server_error` on stdio	Carried over from v0.22, surfaced because this bump documents the refusals as agreeing across transports. Three real messages now in a parity table driven through both real classifiers.

Verification

Gate matrix (stated in full, including what didn't apply):

go build ./... — clean
make lint — 0 issues
go test ./... — 25 packages ok, 0 failures
make test-pg — 25 packages ok, 0 failures, exit 0. Run against 359adb94, not the tip: the only commit since is 5bee9c9e, whose diff is comment-only in internal/items/overrides.go (verified by filtering the diff for non-comment lines, not asserted). CI's Go (PostgreSQL) job is the per-tip evidence.
web checks — N/A, no web/ files changed
Codex — CLEAN at round 11

Mutation matrix. 20 mutations across the unit, counted from the run log rather than remembered. 19 were killed by an assertion. The 20th — removing sort.Strings — SURVIVED at the server level, because with two keys and Go's randomized map iteration my order assertion passes half the time; the sort's real guard is the three-key unit test in internal/items (which does fail unsorted), and the server-side assertion is now documented as an echo rather than the guard. One other mutation was rewritten mid-run because its first form was killed by a COMPILE error, which says nothing about the test.

Highlights of what was mutated: gate absent; gate too broad; gate applied to the full-fields door (which would break every system writer); remedy table pointing at a command that can't write the key; message not naming the key; unsorted output; the stdio code missing from the allow-list; the append classifier reverted; the CLI marker dropped; the CLI call site dropped (the unit test stayed green — that's why the dispatcher-level tests exist); the append guard removed; the readability predicate reverted to its round-2 form; the github_pr exemption removed; the nil-map guard removed; each new stderr pattern removed.

Compat posture

Stated deliberately, same reading as v0.17 and v0.22: today's callers passing such a setter get a 400 where they previously got a write that made the entries unreadable everywhere and disabled note / decide on that item until the row was repaired. Relying on the old behaviour is relying on a defect.

stored_state_unreadable is additive for consumers that switch on code; a client that pattern-matched server_error to detect this condition stops matching — that client was retrying a permanent failure.

Version number: 0.23, taken as the next free number relative to main (which is at 0.22). PR #1159 is still open and stale at 0.22, so it has to rebase regardless; whoever merges second takes the next free number. Worth a re-check at merge time rather than trusting this line.

https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…try-hostile error code (BUG-2627 part 2, BUG-2675) Part 2 of BUG-2627 closes the door that mints the defect parts 1 and 3 dealt with: `--field implementation_notes=<json>` stored the entries as a JSON-ENCODED STRING, which is invisible to every reader and — since part 3's guard — disables `pad item note` on that item until the row is repaired. Refused SERVER-SIDE in `fields_patch`, not at the CLI as the item's scope line proposed. The deviation is deliberate and recorded on the trail: the CLI is one of three clients, and all three lower a user field-setter into the same key (`pad item update --field` at cmd_item.go, the MCP `field` param via dispatch_http_advanced.go on remote, and stdio by shelling out to that CLI). One gate closes all three; a CLI-only refusal would have left remote MCP writing the key. Both call sites were read, and the CLI's lowering is now pinned by a test rather than left as an assumption. Scope, stated because it is deliberate: this closes UPDATE only. The full `fields` blob stays open because that door is SHARED — `pad item note` / `decide` / `github link` send one, and so does convention activation via BuildConventionItemFields -> ItemCreate. Closing it would break the system writers the gate exists to protect. Item create therefore remains a mint site, tracked with the rest of that surface in BUG-2685. The refusal message is per-key: implementation_notes -> `pad item note`, decision_log -> `pad item decide`, github_pr -> the GitHub link flow, and `convention` refuses WITHOUT naming a command, because none writes it. PATTE-135 wants a remedy that works in the failing state; a single "use pad item note" line would have been wrong for three of the four keys. BUG-2675 rides along on one ToolSurfaceVersion bump, as ruled. The append refusal from part 3 reached MCP agents as `server_error` — not our fault, and not transient, so agents could reasonably retry a failure that is deterministic forever. New closed-set code `stored_state_unreadable`, emitted on BOTH transports: HTTP classifies the sentinel error directly, stdio via a `pad-structured-error/v1:` marker the CLI now writes for its own local refusal (the first marker generated without an upstream APIError). v0.16-then-v0.17 is what a one-transport fix costs. Also here: - items.ReservedOverrideKeys -> ReservedFieldKeysIn. The second caller passes a patch, not an override map, and the old doc comment said fields_patch was an open exposure — true until this commit. - `Extract* returns nil for THREE reasons` -> FOUR. The comment listed four; the count was corrected everywhere except the code. - Consumer-read artifacts updated where the claim is ACTED on, not only where it is documented: instructions.md (incl. a "do not retry this code" section), the catalog `field` param description, `pad item update --help`, README. Gates: build · make lint · go test ./... · make test-pg · Codex. Eleven-mutation matrix run against the new tests; every one killed by an assertion (two were rewritten after killing by compile error / surviving, which proves nothing). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…dable; name the MCP-facing code (Codex round 1) Three findings from the pre-push review, all real: P2 — the refusal named `pad item note` unconditionally, but on an item whose stored value is ALREADY undecodable that command refuses too (part 3's guard). The caller was routed in a circle: field write refused -> run the note -> refused -> back again. That is exactly the failure PATTE-135 exists to prevent, and my own trail had reasoned the remedy was safe on the strength of the HEALTHY case only. The message now inspects the item's stored value and, when the key is unparseable, says so and points at the one action that works in that state (inspection), noting that the repair needs a full `fields` write no CLI flag exposes. P2 — two doc claims were false where an actor reads them. The catalog said reserved keys are refused "on every action that accepts field", which includes CREATE, and create is deliberately NOT gated; and both the catalog and instructions.md named `validation_error` (the HTTP code) where an MCP client actually receives `validation_failed`. Both corrected, and the create exception is now stated rather than implied by omission — an agent that reads only "refused on update" will otherwise assume create is fine, which is how a hole gets used. nit — the destructive-downstream sentence claimed every reserved key becomes unreadable and trips an append guard. True only for the two append-backed keys; github_pr and convention are simply overwritten. The clause is now per-key, because a confident wrong explanation is worse than a vague right one. Two more mutations run against the new branch: always-readable (the circular remedy returns) and never-readable (the working remedy disappears) — both killed by assertions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

… hint parity (Codex round 2) Five findings, all real. P2 — the message's readability check and the guard it describes were two different decodes. Mine unmarshalled into []json.RawMessage; the guard uses []ItemImplementationNote. A stored `[1]` passed mine and fails the guard, so the message would again have prescribed a command that refuses — the same circularity round 1 caught, through a narrower door. Replaced with models.StructuredFieldIsAppendable, which ASKS the guard rather than re-deriving it, plus an agreement test over 12 shapes x 2 keys that compares the predicate against the real Append* helpers. Verified by restoring the RawMessage version: the table catches it on `[1]`. P2 — stdio lost the new code's hint. Remote MCP told the agent retrying is pointless and how to inspect; stdio got the code with an empty hint, because the CLI's marker envelope carried none and the classifier parsed none. Both fixed, with the hint hoisted into paired constants (the same duplication StructuredErrorMarker already uses) and the test comparing the two TRANSPORTS' envelopes rather than either against a literal. P2 — doc text was still false for `convention`: the catalog, the instructions and `--help` all said reserved keys are maintained by note/decide/the GitHub flow, which is true of three of the four. Each key now names its own writer, and `convention` names library activation. Also dropped the `malformed_override` advertisement — that is the SERVER's code; an MCP client sees validation_failed for both refusals. nit — the classification test called structuredAppendErrorResult directly, so deleting either dispatcher call site left it green. Added dispatcher-level tests driving the real server + store, asserting the code, the hint, and that the item's stored fields are byte-identical afterwards. Mutation-verified by reverting the note call site. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

… no nil-map panic (Codex round 3) P1 — the gate refused `github_pr`, and that was wrong. My model was "system writers use the full fields blob, user setters use fields_patch", which holds for three of the four reserved keys and fails for this one: `pad github link` needs a local git checkout and the `gh` CLI, so it is excluded from remote MCP BY NAME, and internal/mcp/dispatch_http.go's noRemoteEquivalent map tells remote agents in so many words to use `item update --field github_pr=...` instead. For that audience the patch door is not a bypass of the writer — it IS the writer. So the refusal deleted a documented capability from remote agents, and answered with a message naming a command they cannot run: the same circular remedy round 1 caught, aimed this time at the people the gate was meant to help. items.PatchRefusedFieldKeysIn now exempts the key and records the rule being applied — refuse a raw write where a real writer exists — rather than the list it produces. Whether remote agents should get a proper PR-link action, so the key can be closed too, is a product question and is left as one. P2 — the hint told agents to read the bad value with `pad_item action=get`. They cannot: stripDuplicatedFieldsKeys removes implementation_notes and decision_log from every MCP response's fields blob, and the top-level arrays come from the extractor, which returns nil for exactly this shape. The value is invisible on the whole surface. The hint now says so and routes to a human, who can read it with `pad item show --format json`. P2 — `fields` holding a literal `null` unmarshals into a NIL map with no error, and both Append* helpers assign into what they get back, so `pad item note` PANICKED ("assignment to entry in nil map") instead of appending. Reproduced, fixed in parseMutableItemFields, and pinned by a test that fails on a panic rather than taking the process down. An absent blob and a null blob mean the same thing to every caller. Pre-existing, but it sits in the function family this bug is about and the message was about to recommend the command that panics. nit — README claimed a "closed eight-code taxonomy" (17 codes, and I had just added one) and read as if create lowers into fields_patch. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…p promising a broken workaround (Codex round 4) P1 — round 3 exempted `github_pr` from the update gate on the strength of noRemoteEquivalent's documented workaround. That workaround does not work: ingestFieldKVP (remote) and parseFieldFlag (CLI, and so stdio) both store a `field` value as a STRING, so the PR data lands double-encoded and no link appears — the BUG-2627 shape one key over. Filed as BUG-2696 with the three candidate fixes; NOT folded in, because the narrowest of them changes how every field value is typed. The exemption stands regardless: refusing would leave remote agents with strictly less than a broken door. What changes is what we may PROMISE. The catalog, instructions.md, version.go and README said "this is how you link a PR"; they now say the door is open and broken, and to hand PR linking to a human. Advertising a capability that isn't there is the failure mode this whole unit keeps circling. P2 — StructuredFieldIsAppendable returned TRUE when the whole fields blob was unparseable, on the reasoning that a broken outer blob is a different problem. True of the cause, irrelevant to the caller: the Append* helpers bail on that same parse, so the message again named a command that fails. It now returns false, which is simply the honest answer to the question asked, and the agreement table grew a malformed-outer-blob leg — the gap that let the disagreement through. P2 — the message claimed a raw field write always stores something Pad cannot read back. That holds for the CLI and MCP (a `--field` value is typed by schema lookup and these keys are in no schema) but not for a direct REST caller sending a valid array, who is refused for ownership reasons alone. Reworded to say both parts. nit — a misplaced parenthetical in the README read as if item CREATE lowers into fields_patch. It does not; it sends the full blob. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…round; classify an unparseable blob as retry-hostile (Codex round 5) P1 — I corrected four artifacts that pointed agents at the github_pr field write and missed the fifth: noRemoteEquivalent's own text, which IS the message a remote agent receives when it calls `github link`, and which Codex had quoted at me in round 3 to establish the workaround existed. The nearest artifact to the actor was the one I did not open. Both entries now say there is no working remote path and name BUG-2696, with a test pinning the negative so a future edit cannot quietly reinstate the advice while the write is still broken. P2 — a fields blob that will not parse at all produced a bare parse error, so `note` / `decide` reached agents as `server_error`: transient- looking, and therefore retried, for a failure that is as deterministic as the per-key one BUG-2675 exists for. Both Append* helpers now wrap that parse failure in ErrStructuredFieldUnreadable, which both transports already classify, and the malformed-blob test asserts the sentinel rather than just an error. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…is unreadable (Codex round 6 nit) Round 5 widened stored_state_unreadable to cover a fields blob that fails to parse outright, which made half of its own hint false: MCP's normalization strips a broken structured KEY (so `get` hides it), but leaves an unparseable BLOB as a raw string (so `get` shows it). The hint and instructions.md asserted the first case for both. Now stated per layer, in the two paired constants and the instructions. The reason it is worth the words rather than being cut: an agent told 'you cannot see this' does not look, and would have missed a value that was in fact right there in the response it already had. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…n stdio too (Codex round 7) P2 — carried over from v0.22, surfaced because THIS bump documents the two reserved-key refusals as agreeing across transports. The move/copy message ("Field(s) reserved for system metadata and not settable here") matched none of the stdio validation patterns, so the same deterministic 400 arrived as validation_failed on remote and server_error on stdio — and server_error reads as transient, so an agent retries a refusal that can never pass. One pattern added, plus a test that drives both real classifiers with the real server message text for both refusals, so a reworded message that stops matching fails here rather than in the field. nit — the github_pr exemption is UPDATE-only; move and copy still refuse it, because there the argument is BUG-2674's (an override reintroduces the key the migration just dropped), not this one's. The catalog and instructions said "not refused" without that qualifier. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…sifier (Codex round 8) P2 — round 7 fixed the MOVE wording; the copy path words the same class of refusal differently ("Destination collection has no field(s): ..."), so it kept arriving as server_error on stdio and validation_failed on remote. Third message in one family, and the round-7 test used the move text for every case, which is why it missed this. The parity table now carries all three real messages plus a control leg using one the pattern list already covered — without it the table could pass by matching everything. Recorded in the pattern list's comment rather than left implicit: matching prose is a stopgap, the structural fix is the pad-structured-error/v1 marker that carries the code instead of inferring it, and until a refusal emits one, this test is where a new wording has to be added. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…round 9 nit) The copy legs carried `validation_error` where the handlers actually emit `malformed_override` and `invalid_override`. The 400 branch ignores the body code today, so the test passed either way — which is exactly why the fixture mattered: it was quietly recording a wrong contract, and a future code-aware classifier would regress against a table that agrees with it. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…ound 10 nit) The catalog said the server's own code (validation_error / malformed_override) appears in the MCP message. It does not: the 400 branch emits code=validation_failed with a fixed "Validation failed." message and the server's text in the HINT, discarding the finer-grained code. Reworded to say what an agent actually receives, and to say that telling the two refusals apart means reading the message. Also carried the update-only qualifier on the github_pr exemption into the README, matching the catalog and instructions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…ead ruling) The lead's ruling on the github_pr reversal: make the REASON what the code says, so the next key added to reserved metadata is evaluated against 'does this audience have a real writer?' rather than pattern-matched onto a list that happened to be wrong for one key. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

…22 -> 0.24 (0.22/0.23 were taken by PerpetualSoftware#1165/PerpetualSoftware#1166) Mechanical drift absorption, maintainer-side: main took 0.22 (BUG-2674) and 0.23 (BUG-2627 part 2) while this PR was in review, so its fields- object + strict-validation bump becomes 0.24. Conflicts resolved in favor of main's newer refusal semantics; the PR's fields sentence and strict-validation line are grafted into the current docs. CLAUDE.md's MCP section (stale at 0.21 on main) gains brief 0.22/0.23 entries pointing at version.go. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt

xarmian added 12 commits August 20, 2026 00:49

xarmian marked this pull request as ready for review August 20, 2026 03:17

xarmian merged commit 449ac10 into main Aug 20, 2026
7 checks passed

xarmian deleted the fix/bug-2627-reserved-fields-patch branch August 20, 2026 03:17

xarmian mentioned this pull request Aug 20, 2026

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

Merged

5 tasks

Navigation Menu

Uh oh!

fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) - #1166

fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675)#1166
xarmian merged 12 commits into
mainfrom
fix/bug-2627-reserved-fields-patch

xarmian commented Aug 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

xarmian commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this closes

What is deliberately NOT closed, and why

Codex review: 11 rounds

Verification

Compat posture

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

xarmian commented Aug 20, 2026 •

edited

Loading