mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
xarmian
212d59e7c6
* fix(cli,server): make agent attribution actually happen (BUG-2542)
Agent CLI writes were recorded as the human whose credentials they used.
Three independent defects, each verified by reading the path AND by
probing a live instance — the item deliberately held the mechanism open,
so none of this is inherited.
1. THE HEADER WAS NEVER SENT. actorFromRequest sets actor="agent" on one
signal: the X-Pad-Agent header. The only code that sets it took the
value from `agent_name` in .pad.toml and nowhere else — no
environment detection, no session detection. This repo's .pad.toml
has only `workspace`, so the header has never been sent from here and
every agent write has looked human. ResolveAgentName now resolves
.pad.toml → $PAD_AGENT → detected runtime.
2. ITEM CREATE DISCARDED THE ACTOR. createItemChecked called
actorFromRequest and kept only the source (`_, src :=`), never
setting input.CreatedBy, so store.CreateItem fell through to its
"user" default — even for an agent that DID send the header.
Comments have always stamped it correctly; item creation silently did
not, which made the skill's own contract false on its own terms.
3. SINGLE-ITEM PATCH NEVER STAMPED LastModifiedBy. Bulk ops do
(handlers_items_bulk.go); the single-item path did not, so an item
edited only by agents read as human-edited.
Only entries VERIFIED against a live session belong in the runtime
detection table, so it has exactly one: Claude Code exports CLAUDECODE=1
to child processes, confirmed by reading a pad subprocess's environment
inside one. Guessing at Cursor/Windsurf/Aider variable names would put
unverified claims in a shipped binary and misattribute silently when
wrong; those set $PAD_AGENT until someone confirms a signature.
WHAT THIS DOES NOT DO, stated in the code and the skill rather than left
for someone to assume: the header is client-supplied and self-declared.
An agent that omits it is indistinguishable from the human it borrows
credentials from, and a human running `! pad ...` inside an agent's
terminal inherits that environment and is attributed to the agent. This
makes the trail HONEST, not VERIFIED — it is not a basis for
machine-verifiable human-approval provenance, which needs a channel the
agent cannot author at all. The incident behind this item is exactly
that distinction: an agent's relay of a human's words was recorded
indistinguishably from the human typing them.
Contract corrected in both skill copies, since the item's first question
was which of contract and behavior was wrong. It was the contract: it
promised automatic agent attribution that only ever applied to
workspaces that had opted in.
Tests, each mutation-tested against its own defect reverted alone:
- TestResolveAgentName — precedence plus the negative that makes it mean
something: a plain human shell must still resolve to "". Fails 2/5
reverted.
- TestItemAttribution_AgentVsHuman — agent and human legs for create,
update and the create-stamp-survives-edit invariant. Fails on the
create stamp reverted; fails 2/2 on the update stamp reverted.
The update leg deliberately uses the OTHER writer: insertItemTx seeds
last_modified_by FROM created_by, so a same-writer edit passes whether
or not the PATCH stamps anything — the first version of this test did
exactly that and passed its own counterfactual. Caught only because
each fix was reverted separately.
- TestItemAttribution_ExplicitBodyValueWins — an explicit body value
still beats the header.
End-to-end on a live instance through the real CLI, no .pad.toml opt-in:
agent session → created_by/last_modified_by/comment all `agent`; same
binary with CLAUDECODE stripped → all `user`.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* fix(server): artifact import wrote a UUID into created_by (BUG-2542)
Found by Codex while reviewing the attribution fix. handleImportArtifact
set `input.CreatedBy = u.ID`, which is the wrong DOMAIN for the field
rather than merely the wrong value: created_by holds the role — "user"
or "agent" — and consumers compare it against those literals
(CommentThread.svelte, TimelineVersionCard.svelte). An imported item
matched neither and rendered as neither.
It also would have defeated the fix in the parent commit at this path: a
non-empty CreatedBy suppresses the actor stamp, so imports would have
kept a UUID while every other create path started recording the actor.
The line contradicted the comment directly above it, which said Source
was being left blank precisely so createItemChecked could stamp it "like
every other create path". Now both fields are left blank and stamped
together.
The user's identity has its own home — the items.created_by_user_id
column — which no create path currently populates. That is a separate
gap and is not widened into this change.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* fix: close the remaining attribution bypasses Codex found (BUG-2542)
Review found no P1s and three P2 families beyond the artifact-import bug
already fixed in 6fac5dec. Two are closed here; two are deliberately not,
and the reasons matter more than the diff.
CLOSED — paths that asserted "user" and so SUPPRESSED the new stamp,
which made them worse after the parent commit rather than merely stale:
- cmd/pad/notes.go sent CreatedBy/LastModifiedBy = "user" from the
CLIENT on all four note/decision writes. An explicit body value beats
the header by design, so every agent note claimed a human wrote it,
and would have kept claiming it. The client shouldn't assert an
attribution it cannot know; all four now leave it to the server.
- handlers_item_versions.go hardcoded LastModifiedBy "user" / Source
"web" on restore, so an agent-driven restore recorded itself as a
human web edit. Now stamped from the request.
Also closed Codex's nit that the tests injected X-Pad-Agent directly and
never proved the resolver reaches the wire — TestClientSendsResolvedAgentHeader
runs the real client against an httptest server and asserts the header,
with a human-shell leg asserting its ABSENCE. Fails when the client wiring
is reverted. And the Source assertion now pins "web" rather than
merely non-empty.
NOT CLOSED, on purpose:
- Collab flush. An agent PATCH stamps `agent`, then the browser's later
?source=collab-snapshot PATCH stamps `user`. Codex reads that as lost
attribution; I'm not convinced it's wrong — the browser really is the
writer of that flush, and the agent's edit is already recorded on the
PATCH that carried it. Deciding whose name belongs on a
human-flushed doc containing agent edits is a semantics call about
what last_modified_by MEANS, not a bug I should settle inside a fix
commit. Filed rather than guessed.
- Move paths don't touch last_modified_by at all. That predates this
change and is the same question (is a move an edit?), so it goes with
the above.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* fix(cli): note/decision entries self-declare instead of going authorless
Self-caught regression from the previous commit, found by checking the
thing I changed rather than assuming it behaved like its neighbours.
I removed the CLI's hardcoded CreatedBy: "user" from note and decision
entries on the reasoning that applies to every OTHER write in that file:
an explicit value suppresses the server's stamp, so the client should
stay quiet and let the request context decide. That reasoning does not
reach these two. The entries live INSIDE the item's fields JSON, which
the server stores as an opaque blob and never parses for attribution —
so nothing downstream fills the gap, and blanking it would have written
authorless notes. Worse than the bug I was fixing: "user" was at least
right half the time.
They now carry cli.ActorKind() — the same self-declared signal as the
header, reduced to the user/agent enum the field holds. Its doc says
plainly that this is the ONE place a client should assert attribution,
and why, so the next person doesn't generalise it back the wrong way.
The item-level LastModifiedBy in the same functions stays server-stamped;
that half of the previous commit was right.
TestActorKind covers agent, explicit PAD_AGENT, and human-shell legs.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* fix(server): stamp the actor on non-parent item links (BUG-2542)
Last P2 from the review. Parent links pass the actor to SetParentLink;
every other link type (blocks / blocked-by / relates / implements) goes
through CreateItemLink, which the CLI calls without created_by, so the
store defaulted it to "user" and an agent's `pad item block` recorded a
human. Same one-line shape as the create path, explicit body value still
wins.
TestItemLinkAttribution_AgentVsHuman covers both writers; fails on the
agent leg when the stamp is reverted, control passes either way.
That closes every actor-dropping path the review found except the two
filed as IDEA-2549 (collab flush, move), which are semantics questions
about what last_modified_by means rather than defects — Codex agrees the
deferral holds if the field means content author, and flags that they
become real follow-ups if we decide it means sender-of-write.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* test(server): table-drive every non-parent link type (BUG-2542)
Codex nit: the link regression only covered `blocks`, and "shared routing
makes the other types fine" was doing the work. It cost nothing to stop
assuming, and the table earned itself on the first run — my initial list
included `blocked-by`, which is CLI surface sugar that inverts
source/target into a `blocks` row rather than a stored link type. The API
rejects it with a 400, on BOTH writer legs, which is also how that failure
reads differently from an attribution one.
Now covers blocks / related / implements / supersedes / split_from
against both writers.
One precision fix owed on 06938079's message: it says "THE HEADER WAS
NEVER SENT". Not true in general — a workspace with agent_name in
.pad.toml did send it, which is exactly how I probed the behaviour before
fixing it. Accurate version: the header was absent for anything that had
not opted in, which is every workspace I can see, including this repo's.
The body of that commit says it correctly; the headline overstates.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* style: gofmt notes.go after the attribution edit (BUG-2542)
Removing the hardcoded LastModifiedBy from the two ItemUpdate literals
left the surviving fields aligned to a column that no longer had a
member, so gofmt disagreed and CI's golangci-lint failed the Go job in
42s.
The real fault is upstream of the whitespace: my gates line for #1088
read "go test ./... green · Codex to CLEAN" and lint was simply not in
it. The omission in the report and the failure in CI are the same fact —
I reported a matrix that did not include the axis that broke. `make lint`
runs the pinned suite CI runs and takes seconds; it belongs in every
report I make, alongside test and build.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag