mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 13:28:57 +00:00
xarmian
847ee73327
* fix(cli): lift assigned_user_id / agent_role_id onto their columns (BUG-2583)
`pad item update TASK-9 --field assigned_user_id=<uuid>` wrote the pair
into the item's FIELDS JSON BLOB while the column stayed stale, and then
printed "Updated TASK-9". Two defects in one line: a success message for
a write that did nothing the caller asked for, and a blob key shadowing a
real column's name, so the CLI surface diverged from store/HTTP/MCP
truth. The empty-string case was the same defect wearing a worse hat —
it was the only route an agent had to unassign an item.
Blast radius beyond the CLI: local stdio MCP (`pad mcp serve` — Claude
Desktop, Cursor, Windsurf) dispatches through ExecDispatcher, which
shells out to this CLI. So TASK-2571's fix reached the remote /mcp
transport only, and the transport most agents actually use still could
not unassign. This closes that half.
`cmd/pad/cmd_item.go` now lifts `columnFieldKeys` out of the --field map
onto the column pointers, on CREATE and UPDATE both, mirroring
internal/mcp/dispatch_http.go's liftFieldsToColumns — including its
INVARIANT, which is the part that matters: only keys with defined
clear-to-NULL semantics for "" belong in the list, and `tags` never does
(an empty write corrupts a JSONB column rather than clearing it). A test
fails if anyone adds it.
Two compat changes, ruled separately by the lead:
Q1 non-empty values move to the COLUMN and stop writing the blob key.
Accepted: relying on the old behaviour is relying on a shadowing
defect.
Q2 empty values clear the column. Falls out of the lift, inheriting
BUG-2566's store semantics.
`agent_role_id` gets identical treatment. Existing stray blob keys are
left alone per the ruling — this stops minting new ones; a sweep would
be its own change.
Precedence is explicit and tested: `--assign` / `--role` win over a
lifted --field value, matching liftFieldsToColumns' "caller-supplied
top-level values win". It is delivered by the ORDER of two blocks in the
command, which is exactly the kind of thing that gets reordered by
accident, so there is a test whose only job is to fail when it does.
A non-string --field value is deliberately NOT lifted: a collection that
genuinely declares a field with one of these names makes parseFieldFlag
return a typed value, which cannot address a column. It stays in the
blob — today's behaviour and the only lossless option.
ToolSurfaceVersion 0.16 -> 0.17, and v0.16's transport-scope paragraph
now points forward rather than claiming a limitation that no longer
holds. Behaviour-only bump again, same grounds as v0.16 and v0.9. The
CLI's own marker, CmdhelpVersion, deliberately does NOT move: its
contract is flag/arg SCHEMAS, and no flag or argument changed shape.
instructions.md — the text agents receive at handshake — drops the
"remote only" caveat it carried since TASK-2571. That file is the reason
this PR exists in the shape it does: it is the artifact the actor reads,
and it was the one place the previous PR overclaimed.
VERIFIED LIVE against a running server, with a negative control, because
the claim is about a transport rather than a function:
legs, fixed binary
--field assigned_user_id= -> column CLEARED, blob clean
--field assigned_user_id=<uuid> -> column SET, blob clean
--field agent_role_id= / <uuid> -> same, sibling column untouched
stdio MCP tools/call pad_item
action=update field=["assigned_user_id="]
-> column CLEARED, blob clean
control, PRE-FIX binary, same server + same item + same JSON-RPC bytes
-> column UNCHANGED, blob polluted
with {"assigned_user_id":""}
Six unit tests in cmd/pad/item_column_fields_test.go, four mutations each
failing only its own test (no lift; drop non-strings; flip the
lift/assign precedence; add `tags` to the list). One assertion was
rewritten after mutation testing showed it was VACUOUS: `len(fields_patch)
!= 0` passes whether the key is absent or present-and-empty, so it now
asserts key PRESENCE — confirmed by mutating `omitempty` off the model
field and watching the old form stay green. The redundant `len(patch) > 0`
guard that assertion was meant to cover is gone too; `omitempty` already
does that job, and a guard that reads as load-bearing while doing nothing
is worse than no guard.
go test ./cmd/pad ./internal/mcp — pass. gofmt clean.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* test(cli): cover the create half of the column lift (BUG-2583)
Codex came back CLEAN, but the review reminded me I'd changed `item
create` and only tested it through `liftColumnFields` directly — no test
asserted what create actually puts on the wire. That's the weaker half to
leave uncovered, not the stronger one: on update a wrong write contradicts
a visible prior value, while on create the column-named key is simply
baked into the blob at birth with nothing to contradict it.
The assertion has to parse rather than index, because ItemCreate.Fields is
a JSON-encoded STRING and not a nested object — a body["fields"]["…"]
lookup would have been vacuous in a way that looks fine.
Mutation-tested like the rest: neutralizing the create-side lift fails
this test and only this test.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* docs(mcp): say WHICH form of the unassign works on which transport (codex round 2)
Codex round 2, and it is the same class of defect as the previous PR's
round 2 — an overclaim in the artifact agents actually read. My
instructions.md said "works on BOTH transports" of two forms that do not
behave the same:
field: ["assigned_user_id="] clears on BOTH transports
assigned_user_id: "" clears on REMOTE ONLY
The direct params are not declared in pad_item's schema. They reach the
remote mapper only by riding the verbatim input map; on stdio,
BuildCLIArgs drops unknown keys, so the call does nothing.
VERIFIED, not accepted on the reviewer's word, and the verification
corrected my own first reading. My initial probe appeared to show the
stdio call CORRUPTING the fields blob — but that blob key was leftover
state from the earlier pre-fix control leg, not something the probe
wrote. Re-run against a freshly created item, the two forms separate
cleanly:
before assigned=b6786b13... fields={priority,status}
after assigned_user_id:"" assigned=b6786b13... fields={priority,status} (clean no-op)
after field:["assigned_user_id="] assigned=None fields={priority,status} (cleared)
So the stdio behaviour of the direct param is a DROP, not a corruption —
worth stating precisely, because "it corrupts the blob" would have sent
the next reader hunting a bug that isn't there. (Identity-doc rule: a
guessed mechanism stated as the reason is a claim, not a hedge.)
instructions.md now leads with the form that works everywhere and names
the remote-only limitation of the other; version.go and CLAUDE.md say the
same. IDEA-2584 — declare the params properly — is the fix that would
collapse this distinction, and is now cited from all three.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* fix(cli): don't lift a field the collection actually DECLARES (codex round 3)
Nothing reserves `assigned_user_id` or `agent_role_id` as field names, so a
collection may legally declare a field with one of those keys. For that
collection `--field assigned_user_id=foo` means the DECLARED field — and
the lift I just added would redirect it to the assignment column while
dropping the value the user set. Two wrongs from one line: the intended
write vanishes and an unintended one happens.
liftColumnFields is now schema-aware and never lifts a declared key. Cheap
to do here because both call sites already fetch the collection schema for
parseFieldFlag. The check is PER-KEY — an undeclared sibling still lifts,
so one collision doesn't disable the feature — and a schema-fetch failure
degrades toward lifting, matching how the rest of --field handling degrades.
This makes the CLI deliberately STRICTER than the MCP dispatcher it
otherwise mirrors. liftFieldsToColumns has the identical collision and
can't make the same check as written: it builds its fields map straight
from the tool input without fetching a schema. Filed as IDEA-2587 rather
than fixed here, because closing it costs a round-trip on a hot path while
the CLI fix was free — and recorded so the divergence is KNOWN, in the safe
direction, rather than something a later reader "fixes" by loosening the
CLI to match.
The old non-string branch stays as belt-and-braces: parseFieldFlag only
returns a non-string for a declared field, which the new check already
catches, but if that stops being true a non-string still can't address a
column.
Mutation-tested: ignoring the schema declaration fails the new test and
only that test.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd