feat(items): agent body writes are version-checked by default, phase 1: count, warn, replace (PLAN-3555) (#2054)

Phase 1 of the ruling that agent content writes carry the version the agent
read. Nothing is refused yet; phase 2 (428 at the agent doors) follows a week
of these counts.

- server: `replace` request member; pad_content_writes_unguarded_total
  {door,kind}; warnings.unguarded_content_write on an agent door's unguarded
  content write; a seq update_conflict on a content write (applier route
  included) carries content_write, changed_at, changed_by_kind and a
  "do not resend your body" message.
- CLI: `pad item update --replace` (refused with a token or without a body);
  a stderr warning on an unguarded body write; the merge line on conflict.
- MCP 0.86 (additive): pad_item.update `replace`; the content-conflict hint
  on both transports; the update description leads with read, edit, write
  with the seq.
- Docs and prompts: SKILL.md (both copies), instructions.md, api.md,
  mcp.md, deployment.md, README, update help, plan and onboard playbooks.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS
This commit is contained in:
xarmian
2026-10-11 05:29:53 -04:00
committed by GitHub
parent 3e8bf36f3d
commit bc329fb7f4
30 changed files with 769 additions and 41 deletions
+2 -2
View File
@@ -402,7 +402,7 @@ directory for `claude-code`, and an `[mcp_servers.pad]` table in
project-scoped, it's install-on-request only — `--all` and `pad mcp status` cover
the per-user clients (including Codex) and skip it.
**Tool catalog (v0.85)** — ten resource × action tools plus `pad_set_workspace`. Undeclared input keys are rejected with a structured error rather than silently dropped, and field values are typed against the collection schema on the server:
**Tool catalog (v0.86)** — ten resource × action tools plus `pad_set_workspace`. Undeclared input keys are rejected with a structured error rather than silently dropped, and field values are typed against the collection schema on the server:
| Tool | Actions |
|---|---|
@@ -426,7 +426,7 @@ Plus resources at `pad://workspaces`, `pad://workspace/{ws}/dashboard`,
and `pad://_meta/version`.
**Stability contract.** The catalog is versioned
(`tool_surface_version: "0.85"`) and advertised in the initialize handshake and at
(`tool_surface_version: "0.86"`) and advertised in the initialize handshake and at
`pad://_meta/version`, so external agents can pin against it; the per-version
changelog lives in [`internal/mcp/version.go`](internal/mcp/version.go). Errors
come back as structured envelopes (`{error: {code, message, hint, ...}}`) from a
+41 -2
View File
@@ -1086,6 +1086,7 @@ func updateCmd() *cobra.Command {
expectedUpdatedAt string
expectedSeq int64
overwritePendingEdits bool
replaceBody bool
clearAssignedUser bool
clearAgentRole bool
clearParent bool
@@ -1111,11 +1112,19 @@ duration.
--stdin REPLACES the body with what it reads, and a blank read (empty or
whitespace-only) is refused before anything is sent, because it is what a lost
heredoc looks like. To empty a body on purpose, use --clear-content.`,
heredoc looks like. To empty a body on purpose, use --clear-content.
A body write replaces whatever is stored, so guard it with the version you
read: --expected-seq <seq from pad item show --agent>. A conflict means the item
changed after your read. Re-read it and apply your edit to the CURRENT body;
resending yours would undo the other change. --replace overwrites on purpose.
A body write with neither prints a warning, and a future release refuses it
(PLAN-3555).`,
Example: ` pad item update TASK-5 --status done
pad item update TASK-5 --status done --comment "Fixed the login bug"
pad item update PLAN-2 --status active --priority high
pad item update DOC-3 --stdin < updated-doc.md
pad item update DOC-3 --stdin --expected-seq 41 < updated-doc.md
pad item update DOC-3 --stdin --replace < replacement.md
pad item update DOC-3 --clear-content`,
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
@@ -1160,6 +1169,17 @@ heredoc looks like. To empty a body on purpose, use --clear-content.`,
if clearContent && (useStdin || cmd.Flags().Changed("content")) {
return fmt.Errorf("--clear-content conflicts with --content and --stdin; drop one")
}
// PLAN-3555: --replace says "no version token, on purpose". With a
// token it contradicts itself, and without a body it has nothing to
// replace; both are refused before anything is sent.
if replaceBody {
if cmd.Flags().Changed("expected-seq") || expectedUpdatedAt != "" {
return fmt.Errorf("--replace conflicts with --expected-seq and --expected-updated-at: a token guards the write, --replace says it is deliberately unguarded; drop one")
}
if !useStdin && !clearContent && content == "" {
return fmt.Errorf("--replace applies to a body write: pass it with --content, --stdin or --clear-content")
}
}
var stdinBody string
if useStdin {
b, err := readStdinBody("update")
@@ -1238,6 +1258,7 @@ heredoc looks like. To empty a body on purpose, use --clear-content.`,
if overwritePendingEdits {
input.OverwritePendingEdits = true
}
input.Replace = replaceBody
// Build a FIELD-LEVEL patch carrying ONLY the keys this command
// changes (TASK-2022 / IDEA-1480). The server shallow-merges it
@@ -1472,6 +1493,7 @@ heredoc looks like. To empty a body on purpose, use --clear-content.`,
// create path (codex round 2).
warnUndeclaredFields(updated)
warnContentPendingFlush(updated)
warnUnguardedContentWrite(&input)
if formatFlag == "json" {
return cli.PrintJSON(updated)
@@ -1518,6 +1540,7 @@ heredoc looks like. To empty a body on purpose, use --clear-content.`,
cmd.Flags().BoolVar(&force, "force", false, "override the open-children guard (allow marking the item terminal even if children are non-terminal)")
cmd.Flags().StringVar(&expectedUpdatedAt, "expected-updated-at", "", "optimistic concurrency: RFC3339 updated_at you last read; the update is rejected with a conflict (exit non-zero) if the item changed since")
cmd.Flags().BoolVar(&overwritePendingEdits, "overwrite-pending-edits", false, "with a version token and new content: replace unflushed edits a browser tab holds for this item instead of being refused with content_pending_flush (BUG-3133)")
cmd.Flags().BoolVar(&replaceBody, "replace", false, "with --content, --stdin or --clear-content: overwrite the body on purpose, whatever changed since you read it (no version token). A body write with neither this nor --expected-seq is warned about now and will be refused in a future release (PLAN-3555)")
cmd.Flags().Int64Var(&expectedSeq, "expected-seq", 0, "optimistic concurrency (preferred): the `seq` you last read; rejected with a conflict if the item changed since. Unlike --expected-updated-at this distinguishes two writes inside the same second (BUG-3037)")
// UPDATE ONLY, and deliberately asymmetric with `item create` — do NOT
// "complete" the pair by adding these there (IDEA-2584 ruling). Clearing
@@ -4513,6 +4536,22 @@ func warnContentPendingFlush(item *models.Item) {
"stored form may end up differing slightly from what you sent.")
}
// warnUnguardedContentWrite tells the caller, on stderr, that its body write
// carried no version token and no --replace (PLAN-3555 phase 1). Decided from
// what this command SENT, not from the server's warning, so it also prints
// against a server that predates the warning. It names no seq: the one this
// command fetched just before the write would guard only the milliseconds
// between them, which is the false safety the plan rules out.
func warnUnguardedContentWrite(input *models.ItemUpdate) {
if input.Content == nil || input.ExpectedSeq != nil || input.ExpectedUpdatedAt != "" || input.Replace {
return
}
fmt.Fprintln(os.Stderr, "warning: unguarded content write: this replaced the body without checking that "+
"it is still the one you read, so anything written since then is gone (version history keeps it). "+
"Pass --expected-seq <the seq from your pad item show>, or --replace to overwrite deliberately. "+
"A future release refuses an unguarded body write.")
}
// warnPrunedPendingEdits tells the caller, on stderr, that a direct content
// write deleted unflushed edits a browser tab had left in the op-log (BUG-3133).
// Those edits existed nowhere else, so this is the only notice anyone gets.
+82
View File
@@ -0,0 +1,82 @@
package main
import (
"net/http"
"strings"
"testing"
)
// PLAN-3555 phase 1 at the CLI: --replace reaches the server, is refused
// before any request when it contradicts a token or has no body, and an
// unguarded body write prints the warning while a guarded or deliberate one
// does not.
func TestPLAN3555_ItemUpdateReplace(t *testing.T) {
run := func(args ...string) (stderr string, err error, reqs *[]recordedRequest) {
t.Helper()
reqs = recordingServer(t)
cmd := updateCmd()
cmd.SetArgs(append([]string{"TASK-5"}, args...))
stderr = captureStderr(t, func() {
_ = captureStdout(t, func() { err = cmd.Execute() })
})
return stderr, err, reqs
}
t.Run("unguarded body write warns", func(t *testing.T) {
stderr, err, reqs := run("--content", "x")
if err != nil {
t.Fatal(err)
}
if !strings.Contains(stderr, "unguarded content write") {
t.Fatalf("no warning on stderr: %q", stderr)
}
if p := writesOf(*reqs, http.MethodPatch); len(p) != 1 || p[0].body["replace"] != nil {
t.Fatalf("an unguarded write must not send replace: %+v", *reqs)
}
})
t.Run("--replace is sent and silences the warning", func(t *testing.T) {
stderr, err, reqs := run("--content", "x", "--replace")
if err != nil {
t.Fatal(err)
}
if strings.Contains(stderr, "unguarded content write") {
t.Fatalf("a deliberate replace was warned about: %q", stderr)
}
if p := writesOf(*reqs, http.MethodPatch); len(p) != 1 || p[0].body["replace"] != true {
t.Fatalf("--replace did not reach the PATCH: %+v", *reqs)
}
})
t.Run("--expected-seq silences the warning", func(t *testing.T) {
stderr, err, _ := run("--content", "x", "--expected-seq", "3")
if err != nil {
t.Fatal(err)
}
if strings.Contains(stderr, "unguarded content write") {
t.Fatalf("a guarded write was warned about: %q", stderr)
}
})
t.Run("a field-only write does not warn", func(t *testing.T) {
stderr, err, _ := run("--status", "done")
if err != nil {
t.Fatal(err)
}
if strings.Contains(stderr, "unguarded content write") {
t.Fatalf("a field-only write was warned about: %q", stderr)
}
})
for name, args := range map[string][]string{
"with --expected-seq": {"--content", "x", "--replace", "--expected-seq", "3"},
"with --expected-updated-at": {"--content", "x", "--replace", "--expected-updated-at", "2026-10-11T00:00:00Z"},
"without a body": {"--status", "done", "--replace"},
} {
t.Run("refused "+name, func(t *testing.T) {
_, err, reqs := run(args...)
if err == nil || !strings.Contains(err.Error(), "--replace") {
t.Fatalf("want a --replace refusal, got %v", err)
}
if len(*reqs) != 0 {
t.Fatalf("a refused flag combination reached the server: %d request(s)", len(*reqs))
}
})
}
}
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
+1
View File
@@ -28,6 +28,7 @@ REST API at `/api/v1/`. Key endpoints:
- **A convention's reserved `convention` copy follows its ordinary metadata keys** (TASK-2257). A convention stores its metadata twice: the ordinary fields `trigger`, `scope` / `surfaces`, `enforcement` / `priority`, `category` and `commands` (what agents obey; bootstrap and `--field` filters read them) and the reserved `convention` key (what `item.convention` prefers). When an update CHANGES one of those ordinary keys on an item that HAS a reserved copy, the server rebuilds the copy from the merged fields in the same transaction, after the carry check above: `surfaces` from `surfaces`, else `[scope]`; `enforcement` from `enforcement`, else `priority`; untouched keys keep their stored value. An item without a reserved copy is left as it was. The one new refusal is deliberate: a write whose merged values would leave the copy empty answers 400 `validation_error` and writes nothing. Before this, `pad item update --field trigger=X` left `item.convention` on the old trigger
- **`refuse_undeclared_fields: true` is a strict opt-in on item update** (BUG-3156): a write that would store a field key the collection does not declare is REFUSED with 400 `validation_error`, and nothing is written, instead of being accepted and named in `warnings.undeclared_fields`. It refuses EXACTLY the keys that warning would have named: on a `fields_patch` write the patched keys only; on a full `fields` write the whole blob, stray stored keys included. It is additive and omitted by default, so a write without it is unchanged. **Skew:** a server that predates it ignores the member, which is the accept-and-warn, so a strict caller confirms by the ABSENCE of the warning rather than trusting a 200. `pad item bulk-update` and the remote bulk-update dispatcher set it; `pad item update` / `pad_item.update` do not
- **A content write carrying a version token is REFUSED while a tab holds unflushed edits** (BUG-3133): 409 `content_pending_flush`, lifted by `overwrite_pending_edits: true`. The token guards the row, and those edits live only in the op-log until a tab flushes, so re-reading does not clear it. A tokenless content write still replaces them (edits the server has STORED; typing it has not received yet is the next bullet, and refuses tokenless writes too); with no tab open that deletes them from the op-log, and the response names how many in `warnings.pruned_pending_edits` (additive, omitempty; the CLI prints it to stderr). Rows a schema rebuild SET ASIDE (BUG-3244) refuse EVERY content write, token or not, with the same code plus `details.set_aside_rows`, because no tab will ever store them; `overwrite_pending_edits` deletes them in the write's own transaction and counts them into `warnings.pruned_pending_edits`. A collab-snapshot flush is exempt and leaves them alone. A writer that cannot carry a token asks for the same refusal with `refuse_pending_edits: true` (BUG-3230 U0): no row version is compared, and `overwrite_pending_edits` still lifts it. The web item pane's raw-markdown saves send it, because their debounced saves overlap and their unload flush is a teardown write (BUG-3080). Web-only: not on the CLI or the MCP catalog, so no tool-surface bump, and a server that predates it ignores it and replaces
- **Unguarded writes are counted, and an agent door's unguarded content write is flagged** (PLAN-3555 phase 1). An update that carries neither `expected_seq` / `expected_updated_at` nor `replace: true` is counted in `pad_content_writes_unguarded_total{door,kind}`. `door` is `web` (a browser session), `cli` (the CLI, including local stdio MCP, which runs it), `api` (any other token caller), `mcp_remote` or `chatgpt`. `kind` is `content` or `fields`. Append-only updates (notes, decisions, a bare comment), built-in updates and collab-snapshot flushes are not counted. Such a write with `content` from any door but `web` answers 200 with an additive, `omitempty` `warnings.unguarded_content_write: true`. Nothing is refused in this phase; phase 2 answers 428 for the agent doors. `replace: true` is the deliberate overwrite. It changes no outcome now, a token makes it moot, and it is separate from `overwrite_pending_edits`. A seq `update_conflict` on a content write (the applier route included) gains additive `details.content_write: true`, `changed_at` and `changed_by_kind` (`user`/`agent`, the row's last writer), and a message naming both seqs that says not to resend the body. A field-only conflict and a timestamp conflict are unchanged
- **A content write routed to an open tab is REFUSED when that tab holds typing the server has not stored** (BUG-3542): 409 `content_not_applied` with `details.apply_reason: "unconfirmed_edits"` and `content_landed: false`, token or not (Dave's ruling). The check above reads the op-log when the PATCH arrives, so typing still on the wire is invisible to it; the designated applier tab knows (its BUG-3523 barrier has not confirmed every send) and answers `applier_refuse` instead of applying, and the server does not re-elect another tab. Other fields in the request land, as on every `content_not_applied`. Wait a few seconds, re-read and resend; `overwrite_pending_edits: true` sends the request unguarded, and is the only way to replace that typing. A tab that predates the refusal ignores the request's `guarded` flag and applies, as before
- **Write responses may also carry `warnings.dropped_fields`** (TASK-2878, widened by BUG-3079) — schema-declared keys the write DISCARDED. The general case is an injected schema DEFAULT that fails the same `validateFieldType` a caller-supplied value takes: it used to be assigned and skipped past that check, so the SAME BYTES were refused when supplied and stored when injected, decided only by who put them there — a `status` retyped to `multi_select` whose scalar default survived answered 400 to `{"status":"open"}` and 201 to `{}`, storing `{"status":"open"}`. It is now dropped and named here. The relation case keeps its own later pass, because "is this a string" and "does this string name a live, visible item" are different questions and only the first is answerable in the DB-free validator. Dropped rather than refused because nobody in the request typed it — EXCEPT on a `required` field, where dropping leaves it absent and the write is refused as required, with a message naming the default as the cause. Additive and `omitempty`
- **A create in a system Conventions or Playbooks collection may ADD a library word to that collection's `trigger` / `scope` options** (BUG-3446). The `blank` template seeds one trigger and one scope, so every triggered library convention used to be refused there, on every door (CLI and MCP `library activate`, the web Activate button, the onboard playbook's own B3 create). A value the convention/playbook LIBRARY itself uses (`collections.LibraryOptionVocabulary`, computed from the library) is now appended to the options through `UpdateCollection` with its concurrency token, ahead of validation, and reported in an additive `omitempty` `warnings.options_added` (keyed by field; the CLI prints it to stderr) and in the created item's activity metadata (`options_added`, `options_added_collection`), which the item timeline renders. Any other value is refused as before, so the options grow only by library words. An editor's create may trigger it (lead ruling). The artifact import door (`POST /workspaces/{ws}/import-artifact`) does not: it keeps clearing an unlisted select value with a warning. The create is validated against the widened schema before the widening is stored, so a create refused by validation changes nothing; the schema write is still its own transaction, so a refusal after it (the plan cap at insert, a lost unique race, or a schema edit by someone else landing between the dry run and the write) leaves the library word listed and logged but unreported
+1
View File
@@ -523,6 +523,7 @@ Alert on these instead:
| `pad_collab_resume_force_refreshes_total` | Resumes refused with `force_refresh`, by `reason`: `pruned` (the item's whole op-log is gone), `behind_min` (rows the tab still needed were pruned), `restored` (a version restore since its seed). The tab rebuilds from the server. An **upper bound** on offline edits handed back to the user, since the client hands back only when it held unsent edits (TASK-3501) |
| `pad_collab_overflow_closes_total` | Collab peers the server closed because the op bus had to drop an op for them: their subscriber buffer was full, typically a stalled or very slow socket (TASK-1273). Each reconnects and replays the gap from the op-log, so nothing is lost; expect near zero, and a steady count means peers that cannot keep up |
| `pad_collab_append_failures_total` | Batches of collab sync frames whose op-log append failed (the `collab: append op-log` error). The frames are still broadcast, so the edit exists only in the open tabs until one of them persists it, and if the originating tab closes first, a later SyncStep2 may be its only stored copy, which leaves the item reading clean (TASK-3574). Expect zero. Any nonzero week is the ruled trigger for building the SyncStep2 coverage check |
| `pad_content_writes_unguarded_total` | Item updates that carried neither a version token (`expected_seq` / `expected_updated_at`) nor `replace`, by `door` (`web`, `cli` (local stdio MCP included), `api`, `mcp_remote`, `chatgpt`) and `kind` (`content`, `fields`). Append-only updates, built-in updates and collab-snapshot flushes are not counted (PLAN-3555 phase 1). This is the traffic phase 2 would refuse at the agent doors, and the per-kind split decides whether field writes join it |
| `pad_session_presence_failures_total` | Presence operations failing — **read the `op` label**, the risks differ and run in opposite directions: `register`/`renew` may under-report (a live session unlisted and untargetable), `deregister` may over-report (a dead session left listed, and a push aimed at it reaches nobody), `list` returns a 503, `prune` is benign. A failure means the operation reported an error — Redis can fail a pipeline after applying it, so the write may have landed anyway |
+2 -2
View File
File diff suppressed because one or more lines are too long
+26 -6
View File
@@ -1867,8 +1867,21 @@ type UpdateConflictDetails struct {
ExpectedSeq *int64 `json:"expected_seq,omitempty"`
ActualSeq *int64 `json:"actual_seq,omitempty"`
ConflictType string `json:"conflict_type,omitempty"`
// PLAN-3555: set on a refused BODY write. They must be declared here to
// survive: the stdio marker re-marshals this struct, not the server's
// details.
ContentWrite bool `json:"content_write,omitempty"`
ChangedAt string `json:"changed_at,omitempty"`
ChangedByKind string `json:"changed_by_kind,omitempty"`
}
// UpdateConflictContentHint is the recovery guidance for an update_conflict on
// a body write (PLAN-3555): re-read and merge, never resend. Duplicated in
// internal/mcp (UpdateConflictContentHint); a test asserts the two match.
const UpdateConflictContentHint = "Do not resend your body: it would undo the change you missed. " +
"Re-read the item, apply your edit to the CURRENT content, and send that with expected_seq set to the seq you just read."
// AsUpdateConflict returns the parsed conflict details when this APIError
// carries them, or nil otherwise. Returns nil for any error other than
// "update_conflict" so callers can branch cleanly.
@@ -1892,13 +1905,15 @@ func (e *APIError) AsUpdateConflict() *UpdateConflictDetails {
// falling through to a generic server_error.
// 2. Human-readable lines: the message plus the expected/actual timestamps.
func WriteUpdateConflictError(w io.Writer, apiErr *APIError, uc *UpdateConflictDetails) {
envelope := map[string]any{
"error": map[string]any{
"code": apiErr.Code,
"message": apiErr.Message,
"details": uc,
},
errBody := map[string]any{
"code": apiErr.Code,
"message": apiErr.Message,
"details": uc,
}
if uc.ContentWrite {
errBody["hint"] = UpdateConflictContentHint
}
envelope := map[string]any{"error": errBody}
if data, err := json.Marshal(envelope); err == nil {
fmt.Fprintln(w, StructuredErrorMarker+string(data))
}
@@ -1913,6 +1928,11 @@ func WriteUpdateConflictError(w io.Writer, apiErr *APIError, uc *UpdateConflictD
}
if uc.ActualSeq != nil {
fmt.Fprintf(w, " actual seq: %d\n", *uc.ActualSeq)
if uc.ContentWrite {
// PLAN-3555: the server's message already says not to resend.
fmt.Fprintf(w, "Re-read the item (pad item show), apply your edit to the current body, and update with --expected-seq %d.\n", *uc.ActualSeq)
return
}
fmt.Fprintf(w, "Re-read the item (pad item show) and retry with --expected-seq %d.\n", *uc.ActualSeq)
return
}
@@ -173,7 +173,7 @@ Browse it the same way as conventions — ` + "`pad library list --type playbook
- Read the library body.
- Activate via ` + "`pad library activate \"<title>\"`" + ` (CLI) or ` + "`pad_library`" + ` with ` + "`action: activate, title: \"<title>\"`" + ` (MCP).
- If it needs project-specific tweaks (the seeded ` + "`ship`" + ` references ` + "`make install`" + ` — change it if the project uses ` + "`npm run build`" + ` instead), activate AND THEN immediately edit the playbook body via ` + "`pad item update <PLAYB-ref> --stdin`" + ` with the rewritten content.
- If it needs project-specific tweaks (the seeded ` + "`ship`" + ` references ` + "`make install`" + ` — change it if the project uses ` + "`npm run build`" + ` instead), activate AND THEN immediately edit the playbook body via ` + "`pad item update <PLAYB-ref> --stdin --expected-seq <seq from your read>`" + ` with the rewritten content.
- If it doesn't fit at all, skip.
### B6. Bring in existing open work, or seed a first item
@@ -216,7 +216,7 @@ For each seeded convention, READ ITS BODY. Ask:
- Does the rule make sense for this project? (Some seeded conventions assume specific tooling.)
- Are the commands in the body the real ones for this project? (The library version says "run the test suite"; the seeded version might say "run ` + "`make test`" + `"; the user's project might actually use ` + "`go test -race ./...`" + ` or ` + "`npm test`" + `.)
Rewrite the body via ` + "`pad item update <CONVE-ref> --stdin`" + ` (or MCP). Delete conventions that don't apply via ` + "`pad item delete`" + `.
Rewrite the body via ` + "`pad item update <CONVE-ref> --stdin --expected-seq <seq from your read>`" + ` (or MCP ` + "`pad_item`" + ` update with ` + "`expected_seq`" + `). Delete conventions that don't apply via ` + "`pad item delete`" + `.
For software workspaces, also consider proposing the **"Independent AI code review"**
library convention if a different-model review tool is available — same check as build
@@ -33,7 +33,7 @@ nothing gets written until the user confirms.
Detect which mode the user invoked by inspecting the first positional:
- **Looks like a ref** (matches ` + "`^[A-Z][A-Z0-9]*-\\d+$`" + ` and resolves to a real item in the workspace) → **elaborate mode**. Load the item with ` + "`pad item show <ref> --agent`" + `, ask the user what they want to expand or clarify, and update the item via ` + "`pad item update <ref> --stdin`" + ` with the agreed-upon body.
- **Looks like a ref** (matches ` + "`^[A-Z][A-Z0-9]*-\\d+$`" + ` and resolves to a real item in the workspace) → **elaborate mode**. Load the item with ` + "`pad item show <ref> --agent`" + `, ask the user what they want to expand or clarify, and update the item via ` + "`pad item update <ref> --stdin --expected-seq <seq from that read>`" + ` with the agreed-upon body. If that conflicts, someone edited the item meanwhile: re-read it and fold the agreed changes into the current body rather than resending yours.
- **Otherwise** → **create-new mode**. Run the full Conversation flow below to design and create a fresh plan.
Quick-action prompts that invoke this playbook with a ref + title and trailing intent — e.g. ` + "`plan <REF> \"<title>\" — outline goals, deliverables, and timeline`" + ` (shortcut form ` + "`/pad plan …`" + ` in Claude Code) — are the elaborate-mode entry point; the freeform text after the title is conversational context, not extra positional args.
+16 -4
View File
@@ -301,6 +301,9 @@ var padItemSchemaParams = []ParamDef{
// to catch. `seq` is bumped on every mutation of the row, is returned on
// every item AND every item summary, and is an integer.
{Name: "overwrite_pending_edits", Type: "bool", Description: "update: true replaces a tab's unsaved edits instead of refusing token-guarded content (content_pending_flush)."},
// PLAN-3555: the deliberate-overwrite opt-out. Phase 1 changes no outcome;
// phase 2 refuses a body write that carries neither this nor a token.
{Name: "replace", Type: "bool", Description: "update: true overwrites the content on purpose, whatever changed since you read it (no version token). A content write with neither this nor expected_seq is flagged warnings.unguarded_content_write now and will be refused later. Conflicts with expected_seq."},
{Name: "expected_seq", Type: "number", Description: "Optimistic-concurrency guard for action=update, PREFERRED over expected_updated_at. The `seq` you last read; the update is rejected with code=update_conflict if the item changed since. Unlike expected_updated_at it distinguishes two writes inside the same second. Optional."},
// ── Notes / decisions ──
@@ -330,13 +333,22 @@ Actions:
update — Update an item by ref.
Required: ref. At least one mutable field.
Optional: title, status, priority, content, role, assign, parent, comment, tags,
field, fields, expected_seq, expected_updated_at, overwrite_pending_edits.
field, fields, expected_seq, expected_updated_at, overwrite_pending_edits,
replace.
Same placement rules as create. Field updates MERGE server-side: only
the keys you set change. Pass expected_seq (the seq you last read, on
every item and every list summary) to fail with code=update_conflict
if the item changed since; the details carry actual_seq to retry with.
PREFER IT over expected_updated_at, which cannot tell two writes inside
one second apart.
if the item changed since; the details carry actual_seq. PREFER IT over
expected_updated_at, which cannot tell two writes inside one second
apart.
A 'content' write REPLACES the whole body, so send expected_seq with
it: read (get), edit that body, write it back with the seq you read.
On update_conflict with details.content_write, do NOT resend your
body (it would undo the change you missed): re-read, apply your edit
to the CURRENT content, and send that with the new seq. To overwrite
on purpose, send replace=true instead of a seq. A content write with
neither is flagged warnings.unguarded_content_write, and a later
release refuses it.
Writing 'content' while a browser tab has the item open goes to that
live document: the response echoes what you SENT with
warnings.content_outcome="applied_pending_flush" (it describes the
+1 -1
View File
@@ -1697,7 +1697,7 @@ func TestFieldConflictProperty_SourcesDerivedFromTheDeclaredSchema(t *testing.T)
// expected_seq joins expected_updated_at here for the same reason
// (BUG-3037): it is a concurrency TOKEN, not a field write, so the
// field-conflict machinery has nothing to say about it.
"expected_updated_at": true, "expected_seq": true, "overwrite_pending_edits": true, "force": true, "allow_draft": true,
"expected_updated_at": true, "expected_seq": true, "overwrite_pending_edits": true, "replace": true, "force": true, "allow_draft": true,
"clear_parent": true, "clear_assigned_user": true, "clear_agent_role": true,
"artifact": true, "raw_args": true,
+20
View File
@@ -436,6 +436,26 @@ func (d *HTTPHandlerDispatcher) dispatchItemUpdate(
if hasSeq {
payload["expected_seq"] = seq
}
// PLAN-3555: the deliberate-overwrite opt-out, forwarded only when true.
// With a token it contradicts itself, refused here as the CLI refuses
// --replace with --expected-seq, so both transports answer the same.
if b, ok := input["replace"].(bool); ok && b {
if hasSeq || input["expected_updated_at"] != nil {
return NewErrorResult(ErrorPayload{
Code: ErrValidationFailed,
Message: "replace conflicts with expected_seq and expected_updated_at: a token guards the write, replace says it is deliberately unguarded",
Hint: "Send expected_seq to guard the write, or replace=true to overwrite on purpose, not both.",
}), nil
}
if _, hasContent := payload["content"]; !hasContent {
return NewErrorResult(ErrorPayload{
Code: ErrValidationFailed,
Message: "replace applies to a content write",
Hint: "Send replace=true together with content, or drop it.",
}), nil
}
payload["replace"] = true
}
// BUG-3133: lifts the content_pending_flush refusal. Forwarded only when
// true, like force, so a caller that never sends it sends nothing new.
if b, ok := input["overwrite_pending_edits"].(bool); ok && b {
+20
View File
@@ -1014,6 +1014,9 @@ func classifyHTTPStatusKind(
hint = ContentSetAsideHint
}
}
if upstream.Code == string(ErrUpdateConflict) && contentWriteIn(upstream.Details) {
hint = UpdateConflictContentHint
}
if upstream.Code == string(ErrCommentHasReplies) {
// Retrying refuses identically until the replies are gone.
hint = CommentHasRepliesHint
@@ -1388,6 +1391,13 @@ const LeaseHeldHint = "Another holder (details.holder) has claimed this item unt
const ContentPendingFlushHint = "Re-reading will not clear this: the stored item is unchanged until the open editor saves its edits. " +
"Wait for that and re-read, or resend with overwrite_pending_edits=true to replace them."
// UpdateConflictContentHint replaces the generic re-read-and-retry hint on an
// update_conflict whose details carry content_write (PLAN-3555): a resent body
// would undo the change the caller missed. Duplicated in internal/cli, whose
// marker carries it on stdio; a test asserts the two match.
const UpdateConflictContentHint = "Do not resend your body: it would undo the change you missed. " +
"Re-read the item, apply your edit to the CURRENT content, and send that with expected_seq set to the seq you just read."
// ContentSetAsideHint replaces ContentPendingFlushHint when the refusal is
// about edits a collab schema-version rebuild set aside (BUG-3244,
// details.set_aside_rows > 0). Waiting and re-reading cannot clear those: no
@@ -1396,6 +1406,16 @@ const ContentPendingFlushHint = "Re-reading will not clear this: the stored item
const ContentSetAsideHint = "Re-reading will not clear this, and neither will waiting or opening the item: these edits were set aside by an editor upgrade and no editor will write them back. " +
"Read them with pad item set-aside <ref>, then resend with overwrite_pending_edits=true to discard them."
// contentWriteIn reads content_write from an update_conflict's details
// (PLAN-3555).
func contentWriteIn(details json.RawMessage) bool {
var d struct {
ContentWrite bool `json:"content_write"`
}
_ = json.Unmarshal(details, &d)
return d.ContentWrite
}
// setAsideRowsIn reads set_aside_rows from a refusal's details (BUG-3244).
func setAsideRowsIn(details json.RawMessage) int {
if len(details) == 0 {
File diff suppressed because one or more lines are too long
+150
View File
@@ -0,0 +1,150 @@
package mcp
import (
"bytes"
"context"
"encoding/json"
"strings"
"testing"
"github.com/PerpetualSoftware/pad/internal/cli"
"github.com/PerpetualSoftware/pad/internal/models"
)
// PLAN-3555 phase 1 at the MCP doors: the unguarded-write warning reaches the
// agent, replace reaches the server and is refused where it contradicts
// itself, and a content conflict carries the do-not-resend hint on both
// transports.
func (f *parentFixture) dispatchResult(t *testing.T, input map[string]any) *CallToolResult {
t.Helper()
d := &HTTPHandlerDispatcher{
Handler: f.srv,
UserResolver: func(context.Context) *models.User { return f.owner },
}
res, err := d.Dispatch(WithDispatchInput(context.Background(), input), []string{"item", "update"}, nil)
if err != nil {
t.Fatalf("Dispatch: %v", err)
}
return res
}
func unguardedFlag(t *testing.T, res *CallToolResult) bool {
t.Helper()
if res.IsError {
t.Fatalf("update refused: %s", textOf(res))
}
m, ok := res.StructuredContent.(map[string]any)
if !ok {
t.Fatalf("structured content is %T", res.StructuredContent)
}
w, _ := m["warnings"].(map[string]any)
flagged, _ := w["unguarded_content_write"].(bool)
return flagged
}
func TestPLAN3555_MCPReplaceAndWarning(t *testing.T) {
f := newParentFixture(t)
base := func() map[string]any {
return map[string]any{"workspace": f.workspace.Slug, "ref": f.child.Slug}
}
in := base()
in["content"] = "unguarded"
if !unguardedFlag(t, f.dispatchResult(t, in)) {
t.Fatal("an unguarded content update did not carry warnings.unguarded_content_write")
}
in = base()
in["content"] = "deliberate"
in["replace"] = true
if unguardedFlag(t, f.dispatchResult(t, in)) {
t.Fatal("replace=true was flagged unguarded: it did not reach the server")
}
cur, err := f.store.GetItem(f.child.ID)
if err != nil {
t.Fatal(err)
}
in = base()
in["content"] = "contradiction"
in["replace"] = true
in["expected_seq"] = float64(cur.Seq)
if msg := f.dispatchExpectingError(t, []string{"item", "update"}, in); !strings.Contains(msg, "replace conflicts with expected_seq") {
t.Fatalf("replace with a token: %s", msg)
}
in = base()
in["title"] = "No body"
in["replace"] = true
if msg := f.dispatchExpectingError(t, []string{"item", "update"}, in); !strings.Contains(msg, "replace applies to a content write") {
t.Fatalf("replace without content: %s", msg)
}
}
func TestPLAN3555_MCPContentConflictHint(t *testing.T) {
f := newParentFixture(t)
read, err := f.store.GetItem(f.child.ID)
if err != nil {
t.Fatal(err)
}
f.dispatch(t, []string{"item", "update"}, map[string]any{"workspace": f.workspace.Slug, "ref": f.child.Slug, "content": "theirs", "replace": true})
hint := func(extra map[string]any) (string, map[string]any) {
t.Helper()
in := map[string]any{"workspace": f.workspace.Slug, "ref": f.child.Slug, "expected_seq": float64(read.Seq)}
for k, v := range extra {
in[k] = v
}
res := f.dispatchResult(t, in)
env, ok := res.StructuredContent.(ErrorEnvelope)
if !res.IsError || !ok || env.Error.Code != ErrUpdateConflict {
t.Fatalf("want update_conflict, got %s", textOf(res))
}
var d map[string]any
_ = json.Unmarshal(env.Error.Details, &d)
return env.Error.Hint, d
}
h, d := hint(map[string]any{"content": "mine"})
if h != UpdateConflictContentHint || d["content_write"] != true {
t.Fatalf("content conflict: hint %q details %v", h, d)
}
if h, _ := hint(map[string]any{"title": "Mine"}); h == UpdateConflictContentHint {
t.Fatal("a field-only conflict got the content hint")
}
}
// Stdio half, through the REAL CLI writer: the marker carries the hint and the
// new details, which the CLI re-marshals from its own struct.
func TestPLAN3555_StdioContentConflict(t *testing.T) {
seqs := []int64{4, 7}
apiErr := &cli.APIError{Code: "update_conflict", Message: "TASK-2 changed since you read it (seq 4 → 7)."}
uc := &cli.UpdateConflictDetails{
Ref: "TASK-2", ExpectedSeq: &seqs[0], ActualSeq: &seqs[1], ConflictType: "seq",
ContentWrite: true, ChangedAt: "2026-10-11T07:00:00Z", ChangedByKind: "agent",
}
var stderr bytes.Buffer
cli.WriteUpdateConflictError(&stderr, apiErr, uc)
res := extractStructuredCLIError(stderr.String())
if res == nil {
t.Fatal("stdio classifier did not recognise the marker")
}
env := res.StructuredContent.(ErrorEnvelope)
if env.Error.Hint != UpdateConflictContentHint {
t.Fatalf("stdio hint = %q", env.Error.Hint)
}
var d map[string]any
_ = json.Unmarshal(env.Error.Details, &d)
if d["content_write"] != true || d["changed_at"] != "2026-10-11T07:00:00Z" || d["changed_by_kind"] != "agent" {
t.Fatalf("stdio details = %v", d)
}
if !strings.Contains(stderr.String(), "apply your edit to the current body") {
t.Fatalf("human lines lack the merge instruction:\n%s", stderr.String())
}
}
func TestPLAN3555_CLIAndMCPAgreeOnContentConflictHint(t *testing.T) {
if cli.UpdateConflictContentHint != UpdateConflictContentHint {
t.Errorf("hints differ:\n cli %q\n mcp %q", cli.UpdateConflictContentHint, UpdateConflictContentHint)
}
}
+17 -1
View File
@@ -1738,6 +1738,22 @@ const CmdhelpVersion = "0.1"
// caller cannot read embeds nothing; a failed fetch stays non-fatal,
// as on stdio. Stdio is unchanged.
//
// 0.86 — PLAN-3555 phase 1. ADDITIVE bump on the v0.53 grounds: a new
// param and new response members, and no call that was accepted is
// refused. `pad_item.action=update` gains `replace` (bool), the
// deliberate-overwrite opt-out for a content write that carries no
// version token. It is refused with `expected_seq` or
// `expected_updated_at`, and without `content`, on both transports. A
// content write with neither `replace` nor a token now carries
// `warnings.unguarded_content_write: true` (omitempty) on both
// transports; a later release refuses it (phase 2, a BEHAVIOR bump). A
// seq `update_conflict` on a content write gains additive details
// (`content_write`, `changed_at`, `changed_by_kind`), a message naming
// both seqs that says not to resend the body, and the hint
// UpdateConflictContentHint on BOTH transports: stdio carried no hint
// for this code before, and the remote one said "retry". The code and
// every other conflict are unchanged.
//
// 0.85 — BUG-3544. BEHAVIOR bump on the v0.58 / v0.45 grounds (an
// error that read as transient now says what it is; no name, enum or
// param moved): `pad_workspace.action=invite` on an instance with no
@@ -2033,7 +2049,7 @@ const CmdhelpVersion = "0.1"
// this surface can receive it; the entry exists so a future action does
// not collapse it to permission_denied. When an action that can reach
// it is added, that addition is the contract change and owns the bump.
const ToolSurfaceVersion = "0.85"
const ToolSurfaceVersion = "0.86"
// MetaVersionURI is the canonical URI of the queryable version document.
// Lives outside the pad://workspace/{ws}/... namespace because it's a
+14
View File
@@ -477,6 +477,13 @@ type Metrics struct {
// detail the log line carries.
ContentWritesSupersededTotal prometheus.Counter
// ContentWritesUnguardedTotal counts item updates that carried neither a
// version token nor replace, by door (web, cli, api, mcp_remote, chatgpt)
// and kind (content, fields) (PLAN-3555 phase 1). Append-only updates are
// not counted. Phase 2 refuses the agent doors' unguarded content writes;
// this is the traffic it would refuse.
ContentWritesUnguardedTotal *prometheus.CounterVec
// CollabWatermarkStampsTotal counts browser-tab watermark stamps that
// ADVANCED the flush watermark (BUG-3124 unit B), by what the advance
// covered (TASK-3541 step 0): "content" when it covered at least one
@@ -653,6 +660,11 @@ func New() *Metrics {
Help: "Item content writes refused because the same browser tab had already applied a newer one (BUG-3080).",
})
contentWritesUnguardedTotal := prometheus.NewCounterVec(prometheus.CounterOpts{
Name: "pad_content_writes_unguarded_total",
Help: "Item updates with neither a version token nor replace, by door and kind (PLAN-3555).",
}, []string{"door", "kind"})
collabWatermarkStampsTotal := prometheus.NewCounterVec(prometheus.CounterOpts{
Name: "pad_collab_watermark_stamps_total",
Help: "Browser-tab watermark stamps that advanced the flush watermark, by whether the advance covered content-bearing op-log rows (content) or none (view_only) (TASK-3541).",
@@ -898,6 +910,7 @@ func New() *Metrics {
mcpHTTPRequestsTotal,
mcpActiveSessions,
contentWritesSupersededTotal,
contentWritesUnguardedTotal,
collabWatermarkStampsTotal,
materializeGiveUpsTotal,
dbDeadlockRetriesTotal,
@@ -958,6 +971,7 @@ func New() *Metrics {
MCPActiveSessions: mcpActiveSessions,
ContentWritesSupersededTotal: contentWritesSupersededTotal,
CollabWatermarkStampsTotal: collabWatermarkStampsTotal,
ContentWritesUnguardedTotal: contentWritesUnguardedTotal,
MaterializeGiveUpsTotal: materializeGiveUpsTotal,
DBDeadlockRetriesTotal: dbDeadlockRetriesTotal,
AppWebhookDeliveriesTotal: appWebhookDeliveriesTotal,
+16
View File
@@ -133,6 +133,12 @@ type ItemWriteWarnings struct {
// nowhere else. A write without a version token, or with
// overwrite_pending_edits, still does that; this names how many it destroyed.
PrunedPendingEdits int `json:"pruned_pending_edits,omitempty"`
// UnguardedContentWrite marks a content write from an agent door (anything
// but a browser session) that carried neither a version token nor replace
// (PLAN-3555 phase 1). It replaced whatever was stored since the caller
// read the item. Phase 2 refuses such a write; send expected_seq (the seq
// you read) or replace: true.
UnguardedContentWrite bool `json:"unguarded_content_write,omitempty"`
// NotUnique describes each value a move or copy DROPPED because the
// destination collection declares that field unique and another item
@@ -1504,6 +1510,16 @@ type ItemUpdate struct {
// ignores the member, which is the old replace-and-prune.
RefusePendingEdits bool `json:"refuse_pending_edits,omitempty"`
// Replace is the deliberate-overwrite opt-out (PLAN-3555): the caller means
// to replace the body whatever was written since it read it, so the write
// carries no version token on purpose. It changes no outcome in phase 1; it
// keeps the write out of the unguarded count and its warning, and in phase 2
// it is what an agent door sends instead of expected_seq. Distinct from
// OverwritePendingEdits, which lifts the unflushed-edits refusal: a
// deliberate replace over a tab's pending edits needs both. A version token
// makes it moot.
Replace bool `json:"replace,omitempty"`
// ClientWrite orders ONE browser tab's content writes to an item
// (BUG-3080). The pane stamps every content PATCH with a random id minted
// per page load and a counter that only rises, and the server refuses a
+70
View File
@@ -0,0 +1,70 @@
package server
import (
"net/http"
"github.com/PerpetualSoftware/pad/internal/models"
)
// PLAN-3555 phase 1: agent content writes become version-checked by default.
// In this phase nothing is refused. An unguarded write (no expected_seq, no
// expected_updated_at, no replace) is counted by door and kind, so phase 2's
// server-side refusal (428) is decided on measured traffic, and a content write
// from a door other than the web is told so in warnings.unguarded_content_write.
// Doors a write can arrive through, bounded for a metric label.
const (
writeDoorWeb = "web" // a browser session (cookie)
writeDoorCLI = "cli" // the pad CLI, including local stdio MCP, which runs it
writeDoorAPI = "api" // a token-authenticated caller that is not the CLI
writeDoorMCPRemote = "mcp_remote" // remote MCP over /mcp
writeDoorChatGPT = "chatgpt" // the ChatGPT catalog
)
// Kinds of item update, by what the write can clobber.
const (
writeKindContent = "content" // the body
writeKindFields = "fields" // title, fields, tags, parent, assignment and other row members
)
// writeDoor names the door r arrived through. The ChatGPT surface is checked
// before remote MCP because it is one; the CLI before the generic token check
// because it authenticates with one.
func writeDoor(r *http.Request) string {
if isChatGPTSurface(r) {
return writeDoorChatGPT
}
if _, ok := remoteMCPCaller(r); ok {
return writeDoorMCPRemote
}
if r.Header.Get("X-Pad-Client") == "cli" {
return writeDoorCLI
}
if r.Header.Get("Authorization") != "" || isAPITokenAuth(r) {
return writeDoorAPI
}
return writeDoorWeb
}
// updateWriteKind classifies an item update. An update that only appends
// (implementation notes, decisions, a comment) or carries nothing a later
// write could have changed returns "": the ruling exempts append-only
// operations, because they cannot clobber anything.
func updateWriteKind(in *models.ItemUpdate) string {
if in.Content != nil {
return writeKindContent
}
if in.Title != nil || in.Fields != nil || in.FieldsPatch != nil || in.Tags != nil ||
in.Pinned != nil || in.SortOrder != nil || in.ParentID != nil ||
in.AssignedUserID != nil || in.AgentRoleID != nil || in.ClearAssignedUser ||
in.ClearAgentRole || in.GitHubPR != nil || in.ClearGitHubPR {
return writeKindFields
}
return ""
}
// updateIsUnguarded reports whether an update carries neither a version token
// nor the deliberate-overwrite opt-out.
func updateIsUnguarded(in *models.ItemUpdate) bool {
return in.ExpectedSeq == nil && in.ExpectedUpdatedAt == "" && !in.Replace
}
+20 -4
View File
@@ -2210,6 +2210,21 @@ func (s *Server) updateItem(w http.ResponseWriter, r *http.Request, b *builtinIt
}
}
// PLAN-3555 phase 1: count the unguarded update by door and kind, and mark
// an agent door's unguarded content write for the response. A built-in
// update is the server's own write and a collab-snapshot flush is the tab
// writing its document; neither is a caller's read-then-write.
var unguardedContentWrite bool
if b == nil && !collabSnapshot && updateIsUnguarded(&input) {
if kind := updateWriteKind(&input); kind != "" {
door := writeDoor(r)
if s.metrics != nil {
s.metrics.ContentWritesUnguardedTotal.WithLabelValues(door, kind).Inc()
}
unguardedContentWrite = kind == writeKindContent && door != writeDoorWeb
}
}
// 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
@@ -2792,11 +2807,12 @@ func (s *Server) updateItem(w http.ResponseWriter, r *http.Request, b *builtinIt
prunedPendingEdits += discardedSetAside
// Advisory, post-write, same as create (BUG-2850).
if len(undeclaredFields) > 0 || len(droppedDefaults) > 0 || contentAppliedPendingFlush || prunedPendingEdits > 0 {
if len(undeclaredFields) > 0 || len(droppedDefaults) > 0 || contentAppliedPendingFlush || prunedPendingEdits > 0 || unguardedContentWrite {
warnings := &models.ItemWriteWarnings{
UndeclaredFields: undeclaredFields,
DroppedFields: droppedDefaults,
PrunedPendingEdits: prunedPendingEdits,
UndeclaredFields: undeclaredFields,
DroppedFields: droppedDefaults,
PrunedPendingEdits: prunedPendingEdits,
UnguardedContentWrite: unguardedContentWrite,
}
if contentAppliedPendingFlush {
warnings.ContentOutcome = contentOutcomeAppliedPendingFlush
@@ -498,6 +498,9 @@ func (s *Server) writeTypedItemRefusal(w http.ResponseWriter, item *models.Item,
return true
}
if conflict, ok := asUpdateConflictError(err); ok {
// The applier path's row write carries no content (it travels to the
// tab instead), but the request is a body write all the same.
conflict.ContentWrite = true
writeUpdateConflictError(w, itemRefOrSlug(*item), conflict)
return true
}
@@ -329,7 +329,7 @@ func writeUpdateConflictError(w http.ResponseWriter, ref string, conflict *store
// `actual_updated_at` would re-send the weak token that cannot see the race
// it just lost.
if conflict.ExpectedSeq != nil {
writeUpdateConflictSeqEnvelope(w, ref, *conflict.ExpectedSeq, conflict.ActualSeq)
writeUpdateConflictSeqEnvelope(w, ref, conflict)
return
}
writeUpdateConflictEnvelope(w, ref, conflict.ExpectedUpdatedAt, conflict.ActualUpdatedAt)
@@ -340,7 +340,13 @@ func writeUpdateConflictError(w http.ResponseWriter, ref string, conflict *store
// same code, same message — only the two details keys differ, so a client that
// branches on `code` needs no change and one that reads the details learns
// which token to retry with.
func writeUpdateConflictSeqEnvelope(w http.ResponseWriter, ref string, expectedSeq int64, actualSeq *int64) {
//
// PLAN-3555: a refused CONTENT write gets a message that leads to re-read and
// merge, not to a blind resend (the resend would undo the change the caller
// missed), plus additive details: content_write, changed_at and
// changed_by_kind.
func writeUpdateConflictSeqEnvelope(w http.ResponseWriter, ref string, conflict *store.UpdateConflictError) {
expectedSeq, actualSeq := *conflict.ExpectedSeq, conflict.ActualSeq
details := map[string]any{
"ref": ref,
"expected_seq": expectedSeq,
@@ -351,12 +357,21 @@ func writeUpdateConflictSeqEnvelope(w http.ResponseWriter, ref string, expectedS
if actualSeq != nil {
details["actual_seq"] = *actualSeq
}
message := fmt.Sprintf("%s was modified by another writer since you last read it; re-read and retry.", ref)
if conflict.ContentWrite {
details["content_write"] = true
if !conflict.ActualUpdatedAt.IsZero() {
details["changed_at"] = conflict.ActualUpdatedAt.UTC().Format(time.RFC3339)
}
if conflict.LastModifiedBy != "" {
details["changed_by_kind"] = conflict.LastModifiedBy
}
message = contentConflictMessage(ref, expectedSeq, actualSeq)
}
writeJSON(w, http.StatusConflict, map[string]any{
"error": map[string]any{
"code": "update_conflict",
"message": fmt.Sprintf(
"%s was modified by another writer since you last read it; re-read and retry.",
ref),
"code": "update_conflict",
"message": message,
"details": details,
},
})
@@ -678,3 +693,15 @@ func extractFieldString(fieldsJSON, key string) string {
s, _ := m[key].(string)
return s
}
// contentConflictMessage is the update_conflict message for a refused body
// write (PLAN-3555). It names both seqs, says why resending is wrong, and
// gives the merge loop.
func contentConflictMessage(ref string, expectedSeq int64, actualSeq *int64) string {
if actualSeq == nil {
return fmt.Sprintf("%s changed since you read it (seq %d). Do not resend your body: it would undo that change. "+
"Re-read the item, apply your change to the CURRENT body, and send that with the seq you just read.", ref, expectedSeq)
}
return fmt.Sprintf("%s changed since you read it (seq %d → %d). Do not resend your body: it would undo that change. "+
"Re-read the item, apply your change to the CURRENT body, and send that with expected_seq %d.", ref, expectedSeq, *actualSeq, *actualSeq)
}
@@ -0,0 +1,198 @@
package server
import (
"encoding/json"
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/PerpetualSoftware/pad/internal/metrics"
dto "github.com/prometheus/client_model/go"
)
// PLAN-3555 phase 1: every unguarded item update is counted by door and kind,
// a guarded or deliberate one is not, an append-only one is not, and only an
// agent door's unguarded CONTENT write is told so in the response.
func TestPLAN3555_UnguardedWritesCountedByDoorAndKind(t *testing.T) {
srv := testServer(t)
m := metrics.New()
srv.SetMetrics(m)
slug := createWSWithCollections(t, srv)
item := createTaskWithFields(t, srv, slug, "Item", `{"status":"open"}`)
path := "/api/v1/workspaces/" + slug + "/items/" + item.Slug
cli := map[string]string{"X-Pad-Client": "cli"}
count := func(door, kind string) float64 {
t.Helper()
var out dto.Metric
if err := m.ContentWritesUnguardedTotal.WithLabelValues(door, kind).Write(&out); err != nil {
t.Fatal(err)
}
return out.GetCounter().GetValue()
}
patch := func(headers map[string]string, body map[string]any) bool {
t.Helper()
rr := doRequestWithHeaders(srv, "PATCH", path, body, headers)
if rr.Code != http.StatusOK {
t.Fatalf("PATCH %v: %d %s", body, rr.Code, rr.Body.String())
}
var got struct {
Seq int64 `json:"seq"`
Warnings map[string]any `json:"warnings"`
}
_ = json.Unmarshal(rr.Body.Bytes(), &got)
flagged, _ := got.Warnings["unguarded_content_write"].(bool)
return flagged
}
seq := func() int64 {
t.Helper()
it, err := srv.store.GetItem(item.ID)
if err != nil {
t.Fatal(err)
}
return it.Seq
}
type want struct{ cliContent, cliFields, webContent float64 }
check := func(step string, w want) {
t.Helper()
if a, b, c := count("cli", "content"), count("cli", "fields"), count("web", "content"); a != w.cliContent || b != w.cliFields || c != w.webContent {
t.Fatalf("%s: cli/content=%v cli/fields=%v web/content=%v, want %v/%v/%v", step, a, b, c, w.cliContent, w.cliFields, w.webContent)
}
}
if !patch(cli, map[string]any{"content": "one"}) {
t.Fatal("an unguarded CLI content write must carry warnings.unguarded_content_write")
}
check("unguarded CLI content", want{1, 0, 0})
if patch(cli, map[string]any{"content": "two", "expected_seq": seq()}) {
t.Fatal("a token-guarded content write was flagged unguarded")
}
check("seq-guarded CLI content", want{1, 0, 0})
if patch(cli, map[string]any{"content": "three", "replace": true}) {
t.Fatal("a replace content write was flagged unguarded")
}
check("replace CLI content", want{1, 0, 0})
patch(cli, map[string]any{"append_implementation_note": map[string]any{"summary": "a note"}})
check("append-only CLI write", want{1, 0, 0})
if patch(nil, map[string]any{"content": "four"}) {
t.Fatal("a browser write was flagged: the warning is for agent doors only")
}
check("unguarded web content", want{1, 0, 1})
// Last: a rename moves the slug the path above is built from.
if patch(cli, map[string]any{"title": "Renamed"}) {
t.Fatal("a field-only write was flagged as an unguarded CONTENT write")
}
check("unguarded CLI title", want{1, 1, 1})
}
// PLAN-3555: a seq conflict on a BODY write says not to resend the body and
// carries content_write, changed_at and changed_by_kind; the same conflict on
// a field-only write keeps the old answer.
func TestPLAN3555_ContentConflictSaysDoNotResend(t *testing.T) {
srv := testServer(t)
slug := createWSWithCollections(t, srv)
item := createTaskWithFields(t, srv, slug, "Item", `{"status":"open"}`)
path := "/api/v1/workspaces/" + slug + "/items/" + item.Slug
read, err := srv.store.GetItem(item.ID)
if err != nil {
t.Fatal(err)
}
// Someone else's write lands after our read, from an agent.
if rr := doRequestWithHeaders(srv, "PATCH", path, map[string]any{"content": "theirs"}, map[string]string{"X-Pad-Agent": "other"}); rr.Code != http.StatusOK {
t.Fatalf("premise write: %d %s", rr.Code, rr.Body.String())
}
now, _ := srv.store.GetItem(item.ID)
refuse := func(body map[string]any) (string, map[string]any) {
t.Helper()
rr := doRequest(srv, "PATCH", path, body)
if rr.Code != http.StatusConflict {
t.Fatalf("stale seq must conflict; got %d %s", rr.Code, rr.Body.String())
}
var env struct {
Error struct {
Code string `json:"code"`
Message string `json:"message"`
Details map[string]any `json:"details"`
} `json:"error"`
}
_ = json.Unmarshal(rr.Body.Bytes(), &env)
if env.Error.Code != "update_conflict" {
t.Fatalf("code = %q", env.Error.Code)
}
return env.Error.Message, env.Error.Details
}
msg, d := refuse(map[string]any{"content": "mine", "expected_seq": read.Seq})
if d["content_write"] != true || d["changed_by_kind"] != "agent" || d["changed_at"] == nil {
t.Fatalf("content conflict details = %v", d)
}
if !strings.Contains(msg, "Do not resend your body") || !strings.Contains(msg, "→") {
t.Fatalf("content conflict message = %q", msg)
}
if got := int64(d["actual_seq"].(float64)); got != now.Seq {
t.Fatalf("actual_seq = %d, want %d", got, now.Seq)
}
msg, d = refuse(map[string]any{"title": "Mine", "expected_seq": read.Seq})
if _, ok := d["content_write"]; ok || strings.Contains(msg, "resend") {
t.Fatalf("a field-only conflict took the content answer: %q %v", msg, d)
}
}
// PLAN-3555: on the APPLIER path the row write carries no content (it travels
// to the tab), so the store cannot know the request was a body write; the
// route says so. Without that, a stale content write over an open tab would
// get the field-conflict answer.
func TestPLAN3555_ApplierPathContentConflictIsAContentConflict(t *testing.T) {
srv := testServerWithCollab(t)
ts := httptest.NewServer(srv)
t.Cleanup(ts.Close)
slug := createWSWithCollections(t, srv)
item := createTaskWithFields(t, srv, slug, "Item", `{"status":"open"}`)
conn, resp, err := dialCollab(t, ts.URL, item.ID, nil, "")
if err != nil {
status := ""
if resp != nil {
status = resp.Status
}
t.Fatalf("dialCollab: %v (%s)", err, status)
}
stop := applierEcho(t, conn)
t.Cleanup(stop)
waitForApplierPath(t, srv, slug, item.Slug, item.ID)
read, err := srv.store.GetItem(item.ID)
if err != nil {
t.Fatal(err)
}
path := "/api/v1/workspaces/" + slug + "/items/" + item.Slug
rr := doRequest(srv, "PATCH", path, map[string]any{"expected_seq": read.Seq, "content": "first"})
var ok struct {
Warnings map[string]any `json:"warnings"`
}
_ = json.Unmarshal(rr.Body.Bytes(), &ok)
if rr.Code != http.StatusOK || ok.Warnings["content_outcome"] != contentOutcomeAppliedPendingFlush {
t.Fatalf("premise: the first write must take the applier path; got %d %s", rr.Code, rr.Body.String())
}
rr = doRequest(srv, "PATCH", path, map[string]any{"expected_seq": read.Seq, "content": "second, stale"})
if rr.Code != http.StatusConflict {
t.Fatalf("stale seq: %d %s", rr.Code, rr.Body.String())
}
var env struct {
Error struct {
Message string `json:"message"`
Details map[string]any `json:"details"`
} `json:"error"`
}
_ = json.Unmarshal(rr.Body.Bytes(), &env)
if env.Error.Details["content_write"] != true || !strings.Contains(env.Error.Message, "Do not resend your body") {
t.Fatalf("applier-path content conflict took the field answer: %s", rr.Body.String())
}
}
+11
View File
@@ -2239,6 +2239,13 @@ type UpdateConflictError struct {
// caller sent a token they did not.
ExpectedSeq *int64
ActualSeq *int64
// ContentWrite is set when the refused update carried a body (PLAN-3555),
// and LastModifiedBy is the kind of writer (user/agent) whose change the
// caller missed. The conflict answer uses them to say "do not resend your
// body", which would undo that change, instead of a bare "retry".
ContentWrite bool
LastModifiedBy string
}
func (e *UpdateConflictError) Error() string {
@@ -2788,6 +2795,8 @@ func (s *Store) updateItemWithParentLinkOnce(
ActualUpdatedAt: existing.UpdatedAt,
ExpectedSeq: input.ExpectedSeq,
ActualSeq: &actual,
ContentWrite: input.Content != nil,
LastModifiedBy: existing.LastModifiedBy,
}
}
}
@@ -2810,6 +2819,8 @@ func (s *Store) updateItemWithParentLinkOnce(
ItemID: id,
ExpectedUpdatedAt: input.ExpectedUpdatedAt,
ActualUpdatedAt: existing.UpdatedAt,
ContentWrite: input.Content != nil,
LastModifiedBy: existing.LastModifiedBy,
}
}
}
+3 -1
View File
@@ -116,6 +116,8 @@ Interpret the user's intent and route to the appropriate action. Here are common
**Updating:** `pad item update <ref> --status X --comment "..."` — **always** include `--comment` on status changes to explain *why*. The audit trail is the whole point. Same pattern for priority/role/assign changes.
**Editing a body:** `--stdin` / `--content` REPLACE the whole body, so guard the write with the version you read. Read with `pad item show <ref> --agent` (it carries `seq` and `content`), edit that text, then write it back with `pad item update <ref> --stdin --expected-seq <that seq>`. A conflict means someone changed the item after your read. Re-read it and re-apply your edit to the CURRENT body; never resend yours, because that would undo their change. `--replace` overwrites on purpose. An unguarded body write prints a warning now, and a future release refuses it.
**Working with attachments:** items reference attachments as `![alt](pad-attachment:<uuid>)` (images) or `[label](pad-attachment:<uuid>)` (files). To inspect or read bytes, always use `pad attachment {list|show|view|upload|download}`. `view <uuid>` writes the bytes to a temp file and prints the path — compose with `IMG=$(pad attachment view <uuid>)`, then open it with whatever's available on the platform (`open "$IMG"` on macOS, `xdg-open "$IMG"` on Linux) or just read/describe the file directly.
**Hard rule for agents:** NEVER read directly from `~/.pad/attachments/<storage_key>`. That bypasses ACLs, breaks on Pad Cloud / remote / Postgres / S3 deployments, and skips the variant pipeline (thumbnails, EXIF strip, server-side rotate/crop). Always go through the CLI.
@@ -186,7 +188,7 @@ All commands accepting an item reference take issue IDs (e.g. `TASK-5`, `BUG-8`)
pad item create <collection> "title" [--status X] [--priority X] [--parent REF] [--role X] [--assign X] [--field key=value] [--content "..." | --stdin]
pad item list [collection] [--status X] [--role X] [--assign X] [--parent REF] [--all] [--field key=value]
pad item show TASK-5 [--agent] # --agent: title, ref, status, fields AND body in one read. (--format markdown is the body only, for an edit round-trip with update --stdin; empty for a title-only item)
pad item update TASK-5 [--status X] [--role X] [--assign X] [--comment "..."] [--stdin | --clear-content]
pad item update TASK-5 [--status X] [--role X] [--assign X] [--comment "..."] [--stdin | --clear-content] [--expected-seq N | --replace]
pad item delete TASK-5
pad item search "query"
pad item comment TASK-5 "..." [--reply-to <comment-id>]
+3 -1
View File
@@ -133,6 +133,8 @@ Interpret the user's intent and route to the appropriate action. Here are common
**Updating:** `pad item update <ref> --status X --comment "..."` — **always** include `--comment` on status changes to explain *why*. The audit trail is the whole point. Same pattern for priority/role/assign changes.
**Editing a body:** `--stdin` / `--content` REPLACE the whole body, so guard the write with the version you read. Read with `pad item show <ref> --agent` (it carries `seq` and `content`), edit that text, then write it back with `pad item update <ref> --stdin --expected-seq <that seq>`. A conflict means someone changed the item after your read. Re-read it and re-apply your edit to the CURRENT body; never resend yours, because that would undo their change. `--replace` overwrites on purpose. An unguarded body write prints a warning now, and a future release refuses it.
**Claiming work before you start:** when you are about to work an item (not just discuss it), run `pad item claim <ref>` first (`pad_item action=claim` via MCP). If it fails with `lease_held`, another agent is on it: pick something else and don't retry in a loop. Release it with `pad item release <ref>` when you finish or get blocked. A lease expires on its own (15m by default: pass `--ttl 2h` for longer work, or re-claim to extend it), and `pad project next` / `ready` leave out items another holder has leased. The lease is held under this session's agent name, so two sessions on one account only get distinct leases when each is registered under its own name (`pad session register --agent <name>`).
**Working with attachments:** items reference attachments as `![alt](pad-attachment:<uuid>)` (images) or `[label](pad-attachment:<uuid>)` (files). To inspect or read bytes, always use `pad attachment {list|show|view|upload|download}`. `view <uuid>` writes the bytes to a temp file and prints the path — compose with `IMG=$(pad attachment view <uuid>)`, then open it with whatever the platform has (`open "$IMG"` on macOS, `xdg-open "$IMG"` on Linux) or just read/describe the file directly.
@@ -201,7 +203,7 @@ All commands accepting an item reference take issue IDs (e.g. `TASK-5`, `BUG-8`)
pad item create <collection> "title" [--status X] [--priority X] [--parent REF] [--role X] [--assign X] [--field key=value] [--content "..." | --stdin]
pad item list [collection] [--status X] [--role X] [--assign X] [--parent REF] [--all] [--field key=value]
pad item show TASK-5 [--agent] # --agent: title, ref, status, fields AND body in one read. (--format markdown is the body only, for an edit round-trip with update --stdin; empty for a title-only item)
pad item update TASK-5 [--status X] [--role X] [--assign X] [--comment "..."] [--stdin | --clear-content]
pad item update TASK-5 [--status X] [--role X] [--assign X] [--comment "..."] [--stdin | --clear-content] [--expected-seq N | --replace]
pad item delete TASK-5
pad item search "query"
pad item comment TASK-5 "..." [--reply-to <comment-id>]