From 212d59e7c68dae558d725af29fec2e9d53be0fdc Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 13 Aug 2026 16:21:10 -0400 Subject: [PATCH] fix(cli,server): make agent attribution actually happen (BUG-2542) (#1088) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- cmd/pad/notes.go | 28 ++- internal/cli/agent_identity.go | 86 ++++++++ internal/cli/agent_identity_test.go | 167 ++++++++++++++ internal/cli/client.go | 9 +- internal/server/handlers_artifact_import.go | 19 +- internal/server/handlers_item_links.go | 10 + internal/server/handlers_item_versions.go | 9 +- internal/server/handlers_items.go | 28 ++- .../server/handlers_items_attribution_test.go | 206 ++++++++++++++++++ plugin/skills/pad/SKILL.md | 2 +- skills/pad/SKILL.md | 2 +- 11 files changed, 541 insertions(+), 25 deletions(-) create mode 100644 internal/cli/agent_identity.go create mode 100644 internal/cli/agent_identity_test.go create mode 100644 internal/server/handlers_items_attribution_test.go diff --git a/cmd/pad/notes.go b/cmd/pad/notes.go index 879dd5df..9305cdb3 100644 --- a/cmd/pad/notes.go +++ b/cmd/pad/notes.go @@ -49,7 +49,13 @@ func noteCmd() *cobra.Command { Summary: strings.TrimSpace(args[1]), Details: body, CreatedAt: time.Now().UTC().Format(time.RFC3339), - CreatedBy: "user", + // The server never parses this entry — it lives inside the + // item's fields JSON — so the client is the last party that + // could know who wrote it. Hardcoding "user" here made every + // agent note claim a human wrote it (BUG-2542); leaving it + // empty would have left it authorless, since nothing + // downstream fills it in. Self-declared, like the header. + CreatedBy: cli.ActorKind(), } fields, err := models.AppendImplementationNote(item.Fields, entry) if err != nil { @@ -57,9 +63,9 @@ func noteCmd() *cobra.Command { } updated, err := client.UpdateItem(ws, item.Slug, models.ItemUpdate{ - Fields: &fields, - LastModifiedBy: "user", - Source: "cli", + Fields: &fields, + // LastModifiedBy left empty — server-stamped (BUG-2542). + Source: "cli", }) if err != nil { return err @@ -115,7 +121,13 @@ func decideCmd() *cobra.Command { Decision: strings.TrimSpace(args[1]), Rationale: body, CreatedAt: time.Now().UTC().Format(time.RFC3339), - CreatedBy: "user", + // The server never parses this entry — it lives inside the + // item's fields JSON — so the client is the last party that + // could know who wrote it. Hardcoding "user" here made every + // agent note claim a human wrote it (BUG-2542); leaving it + // empty would have left it authorless, since nothing + // downstream fills it in. Self-declared, like the header. + CreatedBy: cli.ActorKind(), } fields, err := models.AppendDecisionLogEntry(item.Fields, entry) if err != nil { @@ -123,9 +135,9 @@ func decideCmd() *cobra.Command { } updated, err := client.UpdateItem(ws, item.Slug, models.ItemUpdate{ - Fields: &fields, - LastModifiedBy: "user", - Source: "cli", + Fields: &fields, + // LastModifiedBy left empty — server-stamped (BUG-2542). + Source: "cli", }) if err != nil { return err diff --git a/internal/cli/agent_identity.go b/internal/cli/agent_identity.go new file mode 100644 index 00000000..acb613a4 --- /dev/null +++ b/internal/cli/agent_identity.go @@ -0,0 +1,86 @@ +package cli + +import "os" + +// ResolveAgentName decides what goes in the X-Pad-Agent header, which is the +// ONLY signal the server has for attributing a write to an agent rather than a +// human (server.actorFromRequest). Before BUG-2542 the header was populated +// from exactly one place — `agent_name` in .pad.toml — and nothing else, so any +// workspace that had not opted in recorded every agent write as a human one. +// The embedded skill meanwhile promised that `created_by: agent` was automatic. +// The contract was the thing that was wrong; this makes it true instead. +// +// Precedence, most explicit first: +// +// 1. `agent_name` in .pad.toml — a deliberate per-workspace choice. +// 2. $PAD_AGENT — the runtime-agnostic override. Any harness we have not +// taught this function about can set it and be attributed correctly. +// 3. A detected agent runtime, below. +// +// WHAT THIS IS NOT. The header is client-supplied and self-declared, so it +// records honesty, not identity: +// +// - an agent that omits it is indistinguishable from the human whose +// credentials it is using; +// - a human running `! pad ...` inside an agent's terminal inherits that +// terminal's environment and will be attributed to the agent. +// +// So this is not a basis for machine-verifiable human-approval provenance. A +// grant that has to be provable needs a channel the agent cannot author at all. +// BUG-2542's trail has the incident that made the distinction concrete: an +// agent's relay of a human's words was recorded indistinguishably from the +// human having typed them. +func ResolveAgentName() string { + if pt, _ := LoadPadToml(); pt != nil && pt.AgentName != "" { + return pt.AgentName + } + if name := os.Getenv("PAD_AGENT"); name != "" { + return name + } + return detectAgentRuntime() +} + +// agentRuntimeEnv maps an environment variable to the agent name recorded when +// it is set to a non-empty value. +// +// Only entries VERIFIED against a live session of that runtime belong here. +// Claude Code exports CLAUDECODE=1 to child processes (confirmed by reading the +// environment of a `pad` subprocess inside one). Other harnesses — Cursor, +// Windsurf, Aider, Codex — very likely have their own markers, but guessing at +// variable names would put unverified claims in a shipped binary and produce +// silent misattribution when wrong in either direction. They should set +// $PAD_AGENT until someone confirms a signature and adds it here with the same +// standard of evidence. +var agentRuntimeEnv = []struct { + env string + name string +}{ + {env: "CLAUDECODE", name: "claude-code"}, +} + +func detectAgentRuntime() string { + for _, r := range agentRuntimeEnv { + if os.Getenv(r.env) != "" { + return r.name + } + } + return "" +} + +// ActorKind is the writer role — "agent" or "user" — as this process can best +// describe itself. Same self-declared signal as ResolveAgentName, just reduced +// to the enum the created_by / last_modified_by fields hold. +// +// Use it ONLY where the client is the last party that could know: structured +// entries embedded in an item's fields JSON (implementation notes, decision +// log) are written whole by the CLI and the server never parses them for +// attribution, so nobody downstream can fill this in. Everywhere else, leave +// attribution empty and let the server stamp it from the request — an explicit +// value suppresses that stamp, which is how the CLI used to record every agent +// note as a human one (BUG-2542). +func ActorKind() string { + if ResolveAgentName() != "" { + return "agent" + } + return "user" +} diff --git a/internal/cli/agent_identity_test.go b/internal/cli/agent_identity_test.go new file mode 100644 index 00000000..93fb2c40 --- /dev/null +++ b/internal/cli/agent_identity_test.go @@ -0,0 +1,167 @@ +package cli + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" +) + +// BUG-2542. X-Pad-Agent is the only signal the server has for attributing a +// write to an agent, and it used to be populated from .pad.toml's agent_name +// and nothing else — so an agent session in a workspace that had not opted in +// was recorded as the human whose credentials it used. +// +// Each case pins one rung of the precedence AND the negative that makes it +// meaningful: a plain human shell with no markers must still resolve to "", +// because a resolver that returned a name unconditionally would satisfy every +// positive case here while attributing Dave's own commands to an agent. +func TestResolveAgentName(t *testing.T) { + for _, tc := range []struct { + name string + padToml string + env map[string]string + want string + }{ + { + name: "plain human shell resolves to nothing", + want: "", + }, + { + name: "detected runtime is used when nothing more explicit exists", + env: map[string]string{"CLAUDECODE": "1"}, + want: "claude-code", + }, + { + name: "PAD_AGENT overrides a detected runtime", + env: map[string]string{"CLAUDECODE": "1", "PAD_AGENT": "some-other-harness"}, + want: "some-other-harness", + }, + { + name: "pad.toml wins over both", + padToml: "workspace = \"w\"\nagent_name = \"wren\"\n", + env: map[string]string{"CLAUDECODE": "1", "PAD_AGENT": "some-other-harness"}, + want: "wren", + }, + { + name: "an empty marker is not a marker", + env: map[string]string{"CLAUDECODE": "", "PAD_AGENT": ""}, + want: "", + }, + } { + t.Run(tc.name, func(t *testing.T) { + // Run from a scratch cwd so a .pad.toml higher up the real tree + // (this repo has one) cannot leak into the no-toml cases. + dir := t.TempDir() + if tc.padToml != "" { + if err := os.WriteFile(filepath.Join(dir, ".pad.toml"), []byte(tc.padToml), 0o600); err != nil { + t.Fatalf("write .pad.toml: %v", err) + } + } + chdir(t, dir) + + // Clear every variable the resolver consults, then set this case's. + for _, k := range []string{"PAD_AGENT", "CLAUDECODE"} { + t.Setenv(k, "") + os.Unsetenv(k) + } + for k, v := range tc.env { + t.Setenv(k, v) + } + + if got := ResolveAgentName(); got != tc.want { + t.Errorf("ResolveAgentName() = %q, want %q", got, tc.want) + } + }) + } +} + +func chdir(t *testing.T, dir string) { + t.Helper() + prev, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + if err := os.Chdir(dir); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { _ = os.Chdir(prev) }) +} + +// The resolver is only half the path: its value has to reach the wire as +// X-Pad-Agent, which is the header the server keys attribution on. The +// server-side tests inject that header directly, so without this the +// resolver could be correct and the client still send nothing (which is +// exactly the pre-BUG-2542 state). +func TestClientSendsResolvedAgentHeader(t *testing.T) { + for _, tc := range []struct { + name string + env map[string]string + want string + }{ + {name: "agent session sends the header", env: map[string]string{"CLAUDECODE": "1"}, want: "claude-code"}, + {name: "human shell sends none", env: nil, want: ""}, + } { + t.Run(tc.name, func(t *testing.T) { + chdir(t, t.TempDir()) + for _, k := range []string{"PAD_AGENT", "CLAUDECODE"} { + t.Setenv(k, "") + os.Unsetenv(k) + } + for k, v := range tc.env { + t.Setenv(k, v) + } + + var got string + var seen bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got, seen = r.Header.Get("X-Pad-Agent"), true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"status":"ok"}`)) + })) + defer srv.Close() + + if err := NewClientFromURL(srv.URL).Health(); err != nil { + t.Fatalf("health: %v", err) + } + if !seen { + t.Fatal("server never saw the request") + } + if got != tc.want { + t.Errorf("X-Pad-Agent = %q, want %q", got, tc.want) + } + }) + } +} + +// ActorKind backs the one place the client must self-describe: structured +// entries inside an item's fields JSON, which the server never parses. An +// earlier revision of the BUG-2542 fix removed the CLI's hardcoded "user" +// there on the theory that the server would stamp it — it does not, and the +// entries would have been written authorless. +func TestActorKind(t *testing.T) { + for _, tc := range []struct { + name string + env map[string]string + want string + }{ + {name: "agent session", env: map[string]string{"CLAUDECODE": "1"}, want: "agent"}, + {name: "explicit PAD_AGENT", env: map[string]string{"PAD_AGENT": "some-harness"}, want: "agent"}, + {name: "human shell", env: nil, want: "user"}, + } { + t.Run(tc.name, func(t *testing.T) { + chdir(t, t.TempDir()) + for _, k := range []string{"PAD_AGENT", "CLAUDECODE"} { + t.Setenv(k, "") + os.Unsetenv(k) + } + for k, v := range tc.env { + t.Setenv(k, v) + } + if got := ActorKind(); got != tc.want { + t.Errorf("ActorKind() = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/cli/client.go b/internal/cli/client.go index 90ac7cb6..da0983e2 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -69,10 +69,11 @@ func NewClientFromURL(baseURL string) *Client { } } - // Auto-load agent name from .pad.toml if available - if pt, _ := LoadPadToml(); pt != nil && pt.AgentName != "" { - c.agentName = pt.AgentName - } + // Resolve the agent identity that becomes X-Pad-Agent. Used to read + // .pad.toml's agent_name and nothing else, which meant any workspace that + // had not opted in recorded agent writes as human ones (BUG-2542). See + // ResolveAgentName for the precedence and for what this signal cannot do. + c.agentName = ResolveAgentName() return c } diff --git a/internal/server/handlers_artifact_import.go b/internal/server/handlers_artifact_import.go index 8b137b4b..bf816378 100644 --- a/internal/server/handlers_artifact_import.go +++ b/internal/server/handlers_artifact_import.go @@ -140,12 +140,19 @@ func (s *Server) handleImportArtifact(w http.ResponseWriter, r *http.Request) { Content: body, Fields: string(fieldsJSON), } - // Attribution: a normal agent/api create. Source is left blank here so - // createItemChecked stamps it from the request auth context (cli/web), - // matching every other create path. - if u := currentUser(r); u != nil { - input.CreatedBy = u.ID - } + // Attribution: a normal agent/api create. CreatedBy and Source are both + // left blank so createItemChecked stamps them from the request auth + // context, matching every other create path. + // + // This used to set CreatedBy to the user's UUID, which is the wrong + // DOMAIN for the field, not just the wrong value: created_by holds the + // role — "user" or "agent" — and consumers compare it to those literals + // (CommentThread.svelte, TimelineVersionCard.svelte). An imported item + // therefore matched neither and rendered as neither. Found while fixing + // BUG-2542; it also would have defeated that fix here, since a non-empty + // CreatedBy suppresses the actor stamp. The user's identity is already + // carried by the items.created_by_user_id column, which no create path + // currently populates — separate gap, not widened into this change. // Enforce the workspace item-count limit (workspace-scoped), identical to // handleCreateItem. Writes the 403 plan_limit_exceeded response itself when diff --git a/internal/server/handlers_item_links.go b/internal/server/handlers_item_links.go index e633ea52..cc7f4d5b 100644 --- a/internal/server/handlers_item_links.go +++ b/internal/server/handlers_item_links.go @@ -158,6 +158,16 @@ func (s *Server) handleCreateItemLink(w http.ResponseWriter, r *http.Request) { return } + // Non-parent links (blocks / blocked-by / relates / implements) fall + // through to here. The CLI doesn't send created_by, so the store defaulted + // it to "user" and an agent's `pad item block` recorded a human. The parent + // branch above already passes the actor to SetParentLink; this mirrors it. + // An explicit body value still wins, same as everywhere else (BUG-2542). + if input.CreatedBy == "" { + linkActor, _ := actorFromRequest(r) + input.CreatedBy = linkActor + } + link, err := s.store.CreateItemLink(workspaceID, input, item.ID) if err != nil { if strings.Contains(err.Error(), "UNIQUE constraint") { diff --git a/internal/server/handlers_item_versions.go b/internal/server/handlers_item_versions.go index 7d147ee6..42eaa61c 100644 --- a/internal/server/handlers_item_versions.go +++ b/internal/server/handlers_item_versions.go @@ -152,13 +152,18 @@ func (s *Server) handleRestoreItemVersion(w http.ResponseWriter, r *http.Request // client lazy-seeds from the restored content; the boundary rejects any // in-flight pre-restore snapshot. This replaces the earlier // applier/epoch/watermark routing: prune+reseed needs none of it. + // Stamp the writer from the request rather than hardcoding user/web: a + // restore driven by an agent used to record itself as a human web edit + // (BUG-2542). Source falls back to "web" for cookie-authenticated callers, + // which is what actorFromRequest already returns for them. + restoreActor, restoreSource := actorFromRequest(r) content := targetVersion.Content summary := "Restored from version " + targetVersion.CreatedAt.Format("Jan 2, 2006 3:04 PM") input := models.ItemUpdate{ Content: &content, ChangeSummary: summary, - LastModifiedBy: "user", - Source: "web", + LastModifiedBy: restoreActor, + Source: restoreSource, // A restore must always leave an undo point + a version bracketing the // content it moves items.content back to, even on a repeat restore within // the version-throttle window (VersionThrottleInterval = 1h). diff --git a/internal/server/handlers_items.go b/internal/server/handlers_items.go index e4b918b0..0f0ffc0d 100644 --- a/internal/server/handlers_items.go +++ b/internal/server/handlers_items.go @@ -708,9 +708,21 @@ func (s *Server) createItemChecked(r *http.Request, workspaceID string, coll *mo // on. // If a client explicitly sent a source in the body (e.g. an agent // marking itself as 'skill'), respect it. - if input.Source == "" { - _, src := actorFromRequest(r) - input.Source = src + if input.Source == "" || input.CreatedBy == "" { + actor, src := actorFromRequest(r) + if input.Source == "" { + input.Source = src + } + // BUG-2542: the actor half used to be discarded here, so every item + // created through this path fell through to store.CreateItem's + // `created_by = "user"` default — even for an agent that DID send + // X-Pad-Agent. Comments have always stamped it (handlers_comments.go); + // item creation silently did not, which made the skill's "items you + // create will have created_by: agent" contract false on its own terms. + // Same shape as Source: an explicit body value wins. + if input.CreatedBy == "" { + input.CreatedBy = actor + } } item, err := s.store.CreateItem(workspaceID, coll.ID, input) @@ -1286,6 +1298,16 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) { input.VersionSource = "collab-snapshot" } + // BUG-2542: stamp the writer on single-item updates. Bulk ops already do + // this (handlers_items_bulk.go), but this path did not, so an agent's + // PATCH left last_modified_by at store.UpdateItem's "user" default and an + // item edited only by agents read as human-edited. An explicit body value + // wins, matching Source/CreatedBy on the create path. + if input.LastModifiedBy == "" { + updateActor, _ := actorFromRequest(r) + input.LastModifiedBy = updateActor + } + // Server-side gate: reject collab-snapshot PATCHes whose // op_log_cursor is below MIN(item_yjs_updates.id). Such a // cursor proves the flushing tab's Y.Doc was built on op-log diff --git a/internal/server/handlers_items_attribution_test.go b/internal/server/handlers_items_attribution_test.go new file mode 100644 index 00000000..c0c6504c --- /dev/null +++ b/internal/server/handlers_items_attribution_test.go @@ -0,0 +1,206 @@ +package server + +import ( + "bytes" + "encoding/json" + "io" + "net/http/httptest" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-2542. Two write paths took the actor from actorFromRequest and dropped +// it: item CREATE discarded it entirely (keeping only source), and the +// single-item PATCH never set LastModifiedBy at all. Both then fell through to +// the store's `"user"` defaults, so an item created and edited exclusively by +// an agent — one that DID send X-Pad-Agent — read back as human-authored. +// +// Every case runs twice, agent and human, because the bug's signature is that +// the two are indistinguishable. A test that only asserted the agent leg would +// pass against a server that hardcoded "agent", which is the same defect +// pointing the other way. +func TestItemAttribution_AgentVsHuman(t *testing.T) { + for _, tc := range []struct { + name string + agent string // X-Pad-Agent value; empty = human + want string + }{ + {name: "agent write", agent: "claude-code", want: "agent"}, + {name: "human write (control)", agent: "", want: "user"}, + } { + t.Run(tc.name, func(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + + created := doAttributionRequest(t, srv, tc.agent, "POST", + "/api/v1/workspaces/"+ws+"/collections/tasks/items", + map[string]any{"title": "attribution probe"}) + var item models.Item + decodeAttributionBody(t, created, &item) + + if item.CreatedBy != tc.want { + t.Errorf("created_by = %q, want %q (X-Pad-Agent=%q)", item.CreatedBy, tc.want, tc.agent) + } + // Source is pre-existing behaviour and must not regress. Both legs + // authenticate the same way here (no Authorization header, no API + // token), so actorFromRequest returns "web" for both — assert the + // value, not merely that something was written. + if item.Source != "web" { + t.Errorf("source = %q, want %q", item.Source, "web") + } + + // The updater is deliberately the OTHER kind of writer. Patching + // with the same one proves nothing: insertItemTx seeds + // last_modified_by FROM created_by, so a same-writer update leaves + // the expected value in place whether or not the PATCH stamps + // anything — verified by reverting the update stamp alone, which + // that version of this test passed. The cross leg is the only + // shape where the update stamp is the sole mechanism that can + // produce the result. + crossAgent, crossWant := "", "user" + if tc.agent == "" { + crossAgent, crossWant = "claude-code", "agent" + } + updated := doAttributionRequest(t, srv, crossAgent, "PATCH", + "/api/v1/workspaces/"+ws+"/items/"+item.Slug, + map[string]any{"title": "attribution probe (edited)"}) + var after models.Item + decodeAttributionBody(t, updated, &after) + + if after.LastModifiedBy != crossWant { + t.Errorf("last_modified_by after a %s edit = %q, want %q (creator was %q)", + crossWant, after.LastModifiedBy, crossWant, tc.want) + } + // created_by must NOT be rewritten by whoever edited it. + if after.CreatedBy != tc.want { + t.Errorf("created_by after update = %q, want %q — an edit must not restamp the creator", after.CreatedBy, tc.want) + } + }) + } +} + +// An explicit body value still wins, so a caller that knows better than the +// header (an agent recording a write it made on a human's behalf, say) is not +// overridden by it. This is the contract the Source field already had. +func TestItemAttribution_ExplicitBodyValueWins(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + + rr := doAttributionRequest(t, srv, "claude-code", "POST", + "/api/v1/workspaces/"+ws+"/collections/tasks/items", + map[string]any{"title": "explicit", "created_by": "user"}) + var item models.Item + decodeAttributionBody(t, rr, &item) + + if item.CreatedBy != "user" { + t.Errorf("created_by = %q, want %q — an explicit body value must beat the header", item.CreatedBy, "user") + } +} + +func doAttributionRequest(t *testing.T, srv *Server, agent, method, path string, body any) *httptest.ResponseRecorder { + t.Helper() + var r io.Reader + if body != nil { + data, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal body: %v", err) + } + r = bytes.NewReader(data) + } + req := httptest.NewRequest(method, path, r) + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + if agent != "" { + req.Header.Set("X-Pad-Agent", agent) + } + req.RemoteAddr = "192.0.2.1:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + if rr.Code != 200 && rr.Code != 201 { + t.Fatalf("%s %s = %d: %s", method, path, rr.Code, rr.Body.String()) + } + return rr +} + +func decodeAttributionBody(t *testing.T, rr *httptest.ResponseRecorder, out any) { + t.Helper() + if err := json.Unmarshal(rr.Body.Bytes(), out); err != nil { + t.Fatalf("decode response: %v (body: %s)", err, rr.Body.String()) + } +} + +func createTestWorkspaceViaAPI(t *testing.T, srv *Server) string { + t.Helper() + rr := doRequest(srv, "POST", "/api/v1/workspaces", map[string]any{ + "name": "Attribution", "slug": "attribution", "template": "startup", + }) + if rr.Code != 200 && rr.Code != 201 { + t.Fatalf("create workspace = %d: %s", rr.Code, rr.Body.String()) + } + var ws struct { + Slug string `json:"slug"` + } + decodeAttributionBody(t, rr, &ws) + if ws.Slug == "" { + t.Fatal("workspace create returned no slug") + } + return ws.Slug +} + +// Non-parent links (blocks / blocked-by / relates) went through a different +// store call than parent links and never carried the actor, so an agent's +// `pad item block` recorded a human. Parent links were already correct, so the +// human leg here is the control that proves the assertion isn't just reading a +// hardcoded default. +func TestItemLinkAttribution_AgentVsHuman(t *testing.T) { + for _, tc := range []struct { + name string + agent string + want string + }{ + {name: "agent link", agent: "claude-code", want: "agent"}, + {name: "human link (control)", agent: "", want: "user"}, + } { + t.Run(tc.name, func(t *testing.T) { + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + + mk := func(title string) models.Item { + rr := doAttributionRequest(t, srv, tc.agent, "POST", + "/api/v1/workspaces/"+ws+"/collections/tasks/items", + map[string]any{"title": title}) + var it models.Item + decodeAttributionBody(t, rr, &it) + return it + } + src := mk("link source") + + // Every non-parent type in models.ItemLinkType*, not just + // `blocks`. They share routing, so one case would arguably do — + // but "shared routing makes the rest fine" is exactly the + // assumption a table costs nothing to stop assuming. It earned + // that immediately: the first version of this list included + // `blocked-by`, which is CLI surface sugar that inverts + // source/target into a `blocks` row, not a stored link type. The + // API rejects it with 400, on BOTH writer legs — which is also + // how you can tell that failure apart from an attribution one. + for _, linkType := range []string{"blocks", "related", "implements", "supersedes", "split_from"} { + t.Run(linkType, func(t *testing.T) { + target := mk("link target " + linkType) + rr := doAttributionRequest(t, srv, tc.agent, "POST", + "/api/v1/workspaces/"+ws+"/items/"+src.Slug+"/links", + map[string]any{"target_id": target.ID, "link_type": linkType}) + var link models.ItemLink + decodeAttributionBody(t, rr, &link) + + if link.CreatedBy != tc.want { + t.Errorf("%s link created_by = %q, want %q (X-Pad-Agent=%q)", + linkType, link.CreatedBy, tc.want, tc.agent) + } + }) + } + }) + } +} diff --git a/plugin/skills/pad/SKILL.md b/plugin/skills/pad/SKILL.md index e2d4c492..641af38a 100644 --- a/plugin/skills/pad/SKILL.md +++ b/plugin/skills/pad/SKILL.md @@ -314,7 +314,7 @@ See the **Onboarding** entry under Natural Language Routing above — it branche 5. **Be conversational.** You're not a command executor. You're a project partner. 6. **Reference existing items.** Use `[[Item Title]]` links in content to connect items. 7. **Keep it practical.** Size each item so it's a single meaningful unit of work — what "meaningful" means depends on the workspace (one branch/PR for code, one interview round for hiring, one research question for research). Ideas should be actionable. Docs should be concise. Check the workspace's conventions for domain-specific sizing rules. -8. **Attribution matters.** Items you create will have `created_by: agent` and `source: cli` automatically. +8. **Attribution matters.** Items and comments you create are stamped `created_by: agent` and `source: cli` automatically — but the agent half only works if the CLI can tell it is being run by an agent. It detects Claude Code on its own; under any other harness, set `PAD_AGENT=` in the environment (or `agent_name` in `.pad.toml`) or your writes will be recorded as the human whose credentials you are using. Note this is self-declared, not proof: it makes the trail honest, it does not make it verifiable, so never treat `created_by` on a comment as evidence that a human said something. 9. **Follow project conventions.** Always load and follow active conventions before performing work. They are project-specific rules that override your defaults. When a role is active, load both role-specific and global conventions. 10. **Learn and teach.** When the user corrects your behavior or teaches you a project-specific rule, offer to save it as a convention: "Should I save this as a project convention so future agents follow it too?" Use `pad item create convention "Title" --field trigger= --field scope= --field priority=should --stdin` with an appropriate trigger inferred from the context. If the correction is role-specific, add `--field role=`. 11. **Role context is per-conversation.** If roles exist, ask which role the user is working as on first invocation. Remember it for the session. Auto-filter queries and suggest assignments accordingly. Never block on role — if the user says "no role" or the workspace has no roles, work normally. diff --git a/skills/pad/SKILL.md b/skills/pad/SKILL.md index b41de4f1..f2fa8b5e 100644 --- a/skills/pad/SKILL.md +++ b/skills/pad/SKILL.md @@ -308,7 +308,7 @@ Run the **onboard** invokable playbook — see the **Onboarding** entry under Na 5. **Be conversational.** You're not a command executor. You're a project partner. 6. **Reference existing items.** Use `[[Item Title]]` links in content to connect items. 7. **Keep it practical.** Size each item so it's a single meaningful unit of work — what "meaningful" means depends on the workspace (one branch/PR for code, one interview round for hiring, one research question for research). Ideas should be actionable. Docs should be concise. Check the workspace's conventions for domain-specific sizing rules. -8. **Attribution matters.** Items you create will have `created_by: agent` and `source: cli` automatically. +8. **Attribution matters.** Items and comments you create are stamped `created_by: agent` and `source: cli` automatically — but the agent half only works if the CLI can tell it is being run by an agent. It detects Claude Code on its own; under any other harness, set `PAD_AGENT=` in the environment (or `agent_name` in `.pad.toml`) or your writes will be recorded as the human whose credentials you are using. Note this is self-declared, not proof: it makes the trail honest, it does not make it verifiable, so never treat `created_by` on a comment as evidence that a human said something. 9. **Follow project conventions.** Always load and follow active conventions before performing work. They are project-specific rules that override your defaults. When a role is active, load both role-specific and global conventions. 10. **Learn and teach.** When the user corrects your behavior or teaches you a project-specific rule, offer to save it as a convention: "Should I save this as a project convention so future agents follow it too?" Use `pad item create convention "Title" --field trigger= --field scope= --field priority=should --stdin` with an appropriate trigger inferred from the context. If the correction is role-specific, add `--field role=`. 11. **Role context is per-conversation.** If roles exist, ask which role the user is working as on first invocation. Remember it for the session. Auto-filter queries and suggest assignments accordingly. Never block on role — if the user says "no role" or the workspace has no roles, work normally.