diff --git a/CLAUDE.md b/CLAUDE.md index 60f58a824..d3bbde764 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -258,7 +258,9 @@ pad item unblock pad collection list # List collections pad collection create "Name" --fields "key:type[:opts]; ..." # compact DSL for simple schemas pad collection create "Name" --schema '' # full CollectionSchema (terminal_options, defaults, computed, relations) -pad item edit # Open in $EDITOR +pad item edit [--force] # Open in $EDITOR. The save is guarded by the seq it was seeded from (a conflict or failed + # save writes your text to a recovery file and prints its path); refuses a stale body + # (content_state) unless --force (BUG-3035) pad workspace init [--template X] # Create workspace pad agent install [tool] # Install /pad skill for AI tools # Workspace onboarding: run `/pad onboard` from an agent session inside the diff --git a/README.md b/README.md index 45416d801..84caab495 100644 --- a/README.md +++ b/README.md @@ -498,7 +498,7 @@ pad item open Open item in web UI pad item update Update item fields pad item delete Delete item pad item move Move item between collections -pad item edit Open item in $EDITOR +pad item edit [--force] Open item in $EDITOR (guarded save; see --help) pad item search "query" Full-text search across all items pad item comment "text" Add comment to an item pad item comments View item comments diff --git a/cmd/pad/bug3035_edit_guard_test.go b/cmd/pad/bug3035_edit_guard_test.go new file mode 100644 index 000000000..85307fd79 --- /dev/null +++ b/cmd/pad/bug3035_edit_guard_test.go @@ -0,0 +1,322 @@ +package main + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3035: `pad item edit` is a read-modify-write over the whole body. It now +// (1) refuses to open the editor on a stale-body read unless --force, (2) sends +// the seq that seeded the editor as expected_seq, (3) re-reads before saving and +// refuses if the body went stale meanwhile, and (4) preserves the edited text in +// a recovery file whenever the save does not land. + +const editSentinel = "EDITOR-RAN-HERE" + +// editFixture is a REAL item's shape — ref parts, seq, updated_at and fields — +// because a stub missing what real data always has hides the branch real use +// takes (a field-less, ref-less stub already hid one bug on this surface, #1416). +func editFixture(content, contentState string, seq int64) map[string]any { + it := map[string]any{ + "id": "i1", "slug": "fix-the-thing", "title": "Fix the thing", + "collection_slug": "tasks", "collection_prefix": "TASK", "item_number": 5, + "ref": "TASK-5", "fields": `{"status":"open","priority":"high"}`, + "content": content, "seq": seq, "updated_at": "2026-09-21T22:53:52Z", + } + if contentState != "" { + it["content_state"] = contentState + } + return it +} + +type editStub struct { + mu sync.Mutex + gets []map[string]any // one per GET, last one repeats + nGet int + patches []map[string]any + // patchStatus: 0/200 answers the item; 409 answers a seq conflict; other + // codes answer a plain error envelope. + patchStatus int +} + +func (s *editStub) handler(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() + if r.Method == http.MethodPatch { + raw, _ := io.ReadAll(r.Body) + var body map[string]any + _ = json.Unmarshal(raw, &body) + s.patches = append(s.patches, body) + switch s.patchStatus { + case 0, http.StatusOK: + c, _ := body["content"].(string) + _ = json.NewEncoder(w).Encode(editFixture(c, "", 42)) + case http.StatusConflict: + w.WriteHeader(http.StatusConflict) + _ = json.NewEncoder(w).Encode(map[string]any{"error": map[string]any{ + "code": "update_conflict", "message": "Item was modified by another writer", + "details": map[string]any{"ref": "TASK-5", "expected_seq": 41, "actual_seq": 43, "conflict_type": "seq"}, + }}) + default: + w.WriteHeader(s.patchStatus) + _ = json.NewEncoder(w).Encode(map[string]any{"error": map[string]any{ + "code": "internal_error", "message": "boom"}}) + } + return + } + i := s.nGet + if i >= len(s.gets) { + i = len(s.gets) - 1 + } + s.nGet++ + _ = json.NewEncoder(w).Encode(s.gets[i]) +} + +// runEdit runs `pad item edit TASK-5 [extra...]` against the stub with a fake +// editor that announces itself on stderr and appends a line. It returns stdout, +// stderr, the command error, and the directory recovery files land in. +func runEdit(t *testing.T, stub *editStub, extra ...string) (string, string, error, string) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(stub.handler)) + t.Cleanup(srv.Close) + setTempHomeMain(t) + t.Setenv("PAD_URL", srv.URL) + t.Setenv("PAD_TOKEN", "pad_testtoken") + + tmp := t.TempDir() + t.Setenv("TMPDIR", tmp) + + editorPath := filepath.Join(t.TempDir(), "fake-editor") + script := "#!/bin/sh\necho " + editSentinel + " >&2\necho edited >> \"$1\"\n" + if err := os.WriteFile(editorPath, []byte(script), 0o755); err != nil { + t.Fatalf("write fake editor: %v", err) + } + t.Setenv("EDITOR", editorPath) + + origWS, origFormat := workspaceFlag, formatFlag + t.Cleanup(func() { workspaceFlag, formatFlag = origWS, origFormat }) + workspaceFlag, formatFlag = "ws", "" + + cmd := editCmd() + cmd.SetArgs(append([]string{"TASK-5"}, extra...)) + var execErr error + var stdout string + stderr := captureStderr(t, func() { + stdout = captureStdout(t, func() { execErr = cmd.Execute() }) + }) + return stdout, stderr, execErr, tmp +} + +func recoveryFiles(t *testing.T, dir string) []string { + t.Helper() + m, err := filepath.Glob(filepath.Join(dir, "pad-edit-*.md")) + if err != nil { + t.Fatal(err) + } + return m +} + +func TestEditRefusesStaleSeed(t *testing.T) { + stub := &editStub{gets: []map[string]any{ + editFixture("the previous content", models.ContentOutcomeAppliedPendingFlush, 41)}} + _, stderr, err, _ := runEdit(t, stub) + if err == nil { + t.Fatal("a stale-body read must refuse without --force") + } + if strings.Contains(stderr, editSentinel) { + t.Fatal("the editor opened on a body the server reported stale; the refusal must come BEFORE it") + } + if len(stub.patches) != 0 { + t.Fatalf("a refused edit sent %d PATCH(es)", len(stub.patches)) + } + for _, want := range []string{"TASK-5", "browser tab", "--force", "after the tab that caused it has closed"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal must name %q: %v", want, err) + } + } +} + +// CONTROL for the refusal: a current row opens the editor and saves. Without +// this leg a command that refused EVERYTHING would pass the test above. +func TestEditCurrentRowSavesWithTheSeedSeq(t *testing.T) { + stub := &editStub{gets: []map[string]any{editFixture("body", "", 41)}} + stdout, stderr, err, tmp := runEdit(t, stub) + if err != nil { + t.Fatalf("edit: %v\n%s", err, stderr) + } + if !strings.Contains(stderr, editSentinel) { + t.Fatal("the editor never ran") + } + if len(stub.patches) != 1 { + t.Fatalf("want exactly one PATCH, got %d", len(stub.patches)) + } + p := stub.patches[0] + if p["content"] != "body"+"edited\n" { + t.Fatalf("PATCH content = %q", p["content"]) + } + if p["expected_seq"] != float64(41) { + t.Fatalf("the save must carry the seq that SEEDED the editor (41); got %v", p["expected_seq"]) + } + if _, has := p["expected_updated_at"]; has { + t.Fatalf("with a seq available the weak token must not be sent too: %v", p) + } + if stub.nGet != 2 { + t.Fatalf("want the seed read plus one pre-save re-read (2 GETs), got %d", stub.nGet) + } + if !strings.Contains(stdout, `Updated TASK-5 "Fix the thing"`) { + t.Fatalf("success line: %q", stdout) + } + if f := recoveryFiles(t, tmp); len(f) != 0 { + t.Fatalf("a successful save must not leave a recovery file: %v", f) + } +} + +func TestEditFallsBackToUpdatedAtWhenNoSeq(t *testing.T) { + stub := &editStub{gets: []map[string]any{editFixture("body", "", 0)}} + if _, stderr, err, _ := runEdit(t, stub); err != nil { + t.Fatalf("edit: %v\n%s", err, stderr) + } + p := stub.patches[0] + if _, has := p["expected_seq"]; has { + t.Fatalf("a seq the server never issued (0) must not be sent: %v", p) + } + if p["expected_updated_at"] != "2026-09-21T22:53:52Z" { + t.Fatalf("with no seq the save must still be guarded by updated_at; got %v", p["expected_updated_at"]) + } +} + +func TestEditForceOpensAStaleSeedAndStillSendsTheToken(t *testing.T) { + stub := &editStub{gets: []map[string]any{ + editFixture("the previous content", models.ContentOutcomeAppliedPendingFlush, 41)}} + _, stderr, err, _ := runEdit(t, stub, "--force") + if err != nil { + t.Fatalf("edit --force: %v\n%s", err, stderr) + } + if len(stub.patches) != 1 || stub.patches[0]["expected_seq"] != float64(41) { + t.Fatalf("--force skips the stale refusal, not the concurrency guard: %v", stub.patches) + } +} + +func TestEditConflictPreservesTheEditedText(t *testing.T) { + stub := &editStub{gets: []map[string]any{editFixture("body", "", 41)}, patchStatus: http.StatusConflict} + _, stderr, err, tmp := runEdit(t, stub) + if err == nil { + t.Fatal("a 409 must fail the command") + } + files := recoveryFiles(t, tmp) + if len(files) != 1 { + t.Fatalf("want one recovery file, got %v (stderr: %s)", files, stderr) + } + got, _ := os.ReadFile(files[0]) + if string(got) != "body"+"edited\n" { + t.Fatalf("the recovery file must hold the EDITED text byte for byte; got %q", got) + } + if !strings.Contains(err.Error(), files[0]) { + t.Errorf("the returned error must carry the path, for a caller that discards stderr: %v", err) + } + pathAt := strings.Index(stderr, files[0]) + markerAt := strings.Index(stderr, "pad-structured-error/v1:") + if pathAt < 0 || markerAt < 0 { + t.Fatalf("stderr must carry both the path and the structured conflict:\n%s", stderr) + } + if pathAt > markerAt { + t.Errorf("the recovery path must print BEFORE the conflict block:\n%s", stderr) + } +} + +func TestEditAnyFailedSavePreservesTheEditedText(t *testing.T) { + stub := &editStub{gets: []map[string]any{editFixture("body", "", 41)}, patchStatus: http.StatusInternalServerError} + _, _, err, tmp := runEdit(t, stub) + if err == nil { + t.Fatal("a 500 must fail the command") + } + if files := recoveryFiles(t, tmp); len(files) != 1 { + t.Fatalf("a non-conflict failure loses the same text; want one recovery file, got %v", files) + } +} + +// A tab starts typing while $EDITOR is open: the op-log moves, the row's seq +// does not, so only the re-read can see it. +func TestEditRefusesWhenTheBodyWentStaleWhileEditing(t *testing.T) { + stub := &editStub{gets: []map[string]any{ + editFixture("body", "", 41), + editFixture("body", models.ContentOutcomeAppliedPendingFlush, 41), + }} + _, stderr, err, tmp := runEdit(t, stub) + if err == nil { + t.Fatal("a body that went stale during editing must refuse the save") + } + if !strings.Contains(stderr, editSentinel) { + t.Fatal("premise: the editor must have run (the seed read was current)") + } + if len(stub.patches) != 0 { + t.Fatalf("the save went out anyway: %v", stub.patches) + } + if files := recoveryFiles(t, tmp); len(files) != 1 { + t.Fatalf("the refused save must preserve the edit; got %v", files) + } +} + +func TestEditNoChangesSendsNothing(t *testing.T) { + stub := &editStub{gets: []map[string]any{editFixture("body", "", 41)}} + // An editor that saves the file untouched. + noop := filepath.Join(t.TempDir(), "noop-editor") + if err := os.WriteFile(noop, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + srv := httptest.NewServer(http.HandlerFunc(stub.handler)) + t.Cleanup(srv.Close) + setTempHomeMain(t) + t.Setenv("PAD_URL", srv.URL) + t.Setenv("PAD_TOKEN", "pad_testtoken") + t.Setenv("EDITOR", noop) + origWS := workspaceFlag + t.Cleanup(func() { workspaceFlag = origWS }) + workspaceFlag = "ws" + cmd := editCmd() + cmd.SetArgs([]string{"TASK-5"}) + var err error + out := captureStdout(t, func() { err = cmd.Execute() }) + if err != nil || !strings.Contains(out, "No changes.") || len(stub.patches) != 0 { + t.Fatalf("an unchanged edit must say so and send nothing: err=%v out=%q patches=%d", err, out, len(stub.patches)) + } +} + +// A nonzero editor exit aborts (vim's :cq), even after the file was written: +// nothing is sent, and the error says nothing was saved (codex round 1 P2 — +// dispositioned as abort semantics, not a lost save). +func TestEditNonzeroEditorExitAbortsWithoutSaving(t *testing.T) { + stub := &editStub{gets: []map[string]any{editFixture("body", "", 41)}} + abort := filepath.Join(t.TempDir(), "abort-editor") + if err := os.WriteFile(abort, []byte("#!/bin/sh\necho edited >> \"$1\"\nexit 1\n"), 0o755); err != nil { + t.Fatal(err) + } + srv := httptest.NewServer(http.HandlerFunc(stub.handler)) + t.Cleanup(srv.Close) + setTempHomeMain(t) + t.Setenv("PAD_URL", srv.URL) + t.Setenv("PAD_TOKEN", "pad_testtoken") + t.Setenv("EDITOR", abort) + origWS := workspaceFlag + t.Cleanup(func() { workspaceFlag = origWS }) + workspaceFlag = "ws" + cmd := editCmd() + cmd.SetArgs([]string{"TASK-5"}) + var err error + _ = captureStderr(t, func() { _ = captureStdout(t, func() { err = cmd.Execute() }) }) + if err == nil || !strings.Contains(err.Error(), "nothing was saved") { + t.Fatalf("an aborted edit must fail and say nothing was saved; got %v", err) + } + if stub.nGet != 1 || len(stub.patches) != 0 { + t.Fatalf("premise and conclusion: the seed read happened (%d GETs) and nothing was sent (%d PATCHes)", stub.nGet, len(stub.patches)) + } +} diff --git a/cmd/pad/cmd_item.go b/cmd/pad/cmd_item.go index 6176da8a3..4db0c51d5 100644 --- a/cmd/pad/cmd_item.go +++ b/cmd/pad/cmd_item.go @@ -12,6 +12,7 @@ import ( "sort" "strconv" "strings" + "time" "github.com/fatih/color" "github.com/spf13/cobra" @@ -3592,14 +3593,33 @@ func searchCmd() *cobra.Command { // --- playbook --- func editCmd() *cobra.Command { - return &cobra.Command{ + var force bool + cmd := &cobra.Command{ Use: "edit ", Short: "Open an item's content in $EDITOR", Long: `Open an item's rich content in your default editor. After editing and saving, the content is updated in Pad. Items can be referenced by issue ID (e.g. TASK-5) or slug. -Set EDITOR or VISUAL env var to choose your editor (default: vi).`, +Set EDITOR or VISUAL env var to choose your editor (default: vi). + +The save is guarded: it is refused if the stored item was written since +the editor opened. If it is refused, or the save fails for any other +reason, your edited text is written to a recovery file and its path is +printed. Exiting the editor with a nonzero status (e.g. vim's :cq) aborts +without saving, as git does. + +One gap remains: the guard sees writes to the STORED item, and a browser +tab's unsaved typing is not one until the tab writes it back. The command +checks for such typing when the editor opens and again just before saving, +but typing that starts in the moment between that last check and the save +can still be overwritten. + +Refuses to open when the stored body is behind the item's live +collaborative document (an editor in a browser tab holds edits not yet +written back), because saving would replace those edits. Open the item +in a browser tab so it flushes, then retry — or pass --force to edit the +stored body anyway.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { client, cfg := getClient() @@ -3611,13 +3631,26 @@ Set EDITOR or VISUAL env var to choose your editor (default: vi).`, return err } + // BUG-3035: refuse, don't just warn. The seed would be a body that + // predates edits which exist, and the save replaces the WHOLE body. + // The concurrency token below cannot catch this: those edits live in + // the op-log, not the row, so the row's seq never moved for them. + if item.ContentState == models.ContentOutcomeAppliedPendingFlush && !force { + return staleEditRefusal(item) + } + // BEFORE the editor opens, not after: this is the only point at which - // the warning can still change what the user does (BUG-3033). + // the warning can still change what the user does (BUG-3033). Reached + // only under --force now. warnStaleEditSeed(item) edited, err := cli.OpenInEditor(cfg, item.Content, ".md") if err != nil { - return err + // A nonzero editor exit is the conventional ABORT (vim's :cq, + // honoured by git the same way), so nothing is sent and no + // recovery file is written. Say so, rather than leaving the + // reader to guess whether anything was saved. + return fmt.Errorf("%w — nothing was saved", err) } if edited == item.Content { @@ -3625,11 +3658,37 @@ Set EDITOR or VISUAL env var to choose your editor (default: vi).`, return nil } - updated, err := client.UpdateItem(ws, slug, models.ItemUpdate{ - Content: &edited, - }) + // A tab that starts typing while $EDITOR is open appends to the + // op-log without touching the row, so the seq token cannot see it + // either. Re-read and refuse if that happened. This narrows the + // window to the gap between this read and the PATCH; it does not + // close it (BUG-3035). + if !force { + latest, rerr := client.GetItem(ws, slug) + if rerr != nil { + return saveEditRecovery(item, edited, rerr) + } + if latest.ContentState == models.ContentOutcomeAppliedPendingFlush { + return saveEditRecovery(item, edited, staleEditRefusal(latest)) + } + } + + input := models.ItemUpdate{Content: &edited} + // The token is the seq that SEEDED the editor, so any row write + // since — another CLI, an API caller, a tab's flush — is refused + // rather than overwritten. seq is never 0 on a live row; a 0 means + // the server did not report one, and the weaker timestamp is still + // better than an unguarded replace of the whole body. + if item.Seq >= 1 { + seq := item.Seq + input.ExpectedSeq = &seq + } else if !item.UpdatedAt.IsZero() { + input.ExpectedUpdatedAt = item.UpdatedAt.UTC().Format(time.RFC3339) + } + + updated, err := client.UpdateItem(ws, slug, input) if err != nil { - return err + return saveEditRecovery(item, edited, err) } warnUndeclaredFields(updated) @@ -3644,6 +3703,96 @@ Set EDITOR or VISUAL env var to choose your editor (default: vi).`, return nil }, } + cmd.Flags().BoolVar(&force, "force", false, "edit even when the stored body is behind the live collaborative document (replaces edits held only there)") + return cmd +} + +// staleEditRefusal is the error `pad item edit` returns instead of opening the +// editor on a body the server reports as behind its live collaborative document +// (BUG-3035). +// +// It names --force as well as the tab, and says why --force is not merely a +// footgun: the flush that would clear this state is not guaranteed to happen +// (BUG-3000), and the state can read pending when nothing is (BUG-3124). Without +// the escape hatch either of those would leave the item permanently uneditable +// from the CLI. +func staleEditRefusal(item *models.Item) error { + name := cli.ItemRef(*item) + if name == "" { + name = item.Slug + } + return fmt.Errorf("refusing to edit %s: its stored content is behind its live collaborative "+ + "document — an editor holds edits that have not been written back yet, and saving would "+ + "replace them with a version derived from a state before them. Open the item in a browser "+ + "tab so those edits are written back, then retry. If no tab holds unsaved work (this state "+ + "can persist after the tab that caused it has closed), pass --force to edit the stored "+ + "content anyway", name) +} + +// saveEditRecovery writes the text the user edited to a file and returns an +// error that carries both the original cause and the file's path (BUG-3035). +// +// Without it a refused or failed save lost the edit outright: OpenInEditor +// removes its temp file on return, so the only copy was this process's memory. +// It covers EVERY failed save, not only a conflict — a network error loses the +// same text. +// +// The path is printed to stderr BEFORE the cause is rendered, so a long +// structured conflict block cannot push it off a short terminal, and it is also +// in the returned error so a caller that discards stderr still gets it. +func saveEditRecovery(item *models.Item, edited string, cause error) error { + name := cli.ItemRef(*item) + if name == "" { + name = item.Slug + } + f, ferr := os.CreateTemp("", "pad-edit-"+sanitizeRecoveryName(name)+"-*.md") + if ferr == nil { + _, ferr = f.WriteString(edited) + if cerr := f.Close(); ferr == nil { + ferr = cerr + } + } + if ferr != nil { + // Nothing else can hold the text now; say so plainly rather than + // pointing at a path that may be empty. + if f != nil { + _ = os.Remove(f.Name()) + } + renderEditSaveError(cause) + return fmt.Errorf("save of %s failed and your edit could NOT be written to a recovery file (%v): %w", name, ferr, cause) + } + path := f.Name() + fmt.Fprintf(os.Stderr, "Your edit was NOT saved to %s. It is preserved in %s\n", name, path) + renderEditSaveError(cause) + return fmt.Errorf("save of %s failed; your edit is preserved in %s: %w", name, path, cause) +} + +// renderEditSaveError prints the structured conflict block for an +// optimistic-concurrency refusal, the same two-track shape `pad item update` +// uses (TASK-2022), so the MCP stdio classifier and a human read one payload. +func renderEditSaveError(err error) { + if apiErr, ok := err.(*cli.APIError); ok { + if uc := apiErr.AsUpdateConflict(); uc != nil { + cli.WriteUpdateConflictError(os.Stderr, apiErr, uc) + } + } +} + +// sanitizeRecoveryName keeps a recovery file name to characters that are safe +// in a path component on every platform; a slug or ref never needs more. +func sanitizeRecoveryName(s string) string { + var b strings.Builder + for _, r := range s { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' { + b.WriteRune(r) + } else { + b.WriteRune('_') + } + } + if b.Len() == 0 { + return "item" + } + return b.String() } // --- utility --- @@ -4017,10 +4166,9 @@ func warnStaleDerivedText(what string, refs []string, howToSee string) { // version derived from a state before them, and neither the person editing nor // the person whose tab holds them sees it happen. // -// What this line does NOT do is stop that: it warns and proceeds. Whether the -// command should refuse, or offer to merge, is a behaviour decision with its own -// item (BUG-3035) rather than something to settle inside a sweep for missing -// markers. +// Since BUG-3035 the command REFUSES on this state by default (staleEditRefusal), +// so this line is reached only under --force: it is the forced path's reminder of +// what the user chose to overwrite. // // It is printed BEFORE the editor is launched. Printed afterwards it would be // read, at best, next to a "Updated TASK-5" line — after the damage. diff --git a/cmd/pad/cmd_playbook_stale_body_test.go b/cmd/pad/cmd_playbook_stale_body_test.go index b99b68ff6..3274814a2 100644 --- a/cmd/pad/cmd_playbook_stale_body_test.go +++ b/cmd/pad/cmd_playbook_stale_body_test.go @@ -181,12 +181,13 @@ func TestWarnPlaybookBodyStaleGating(t *testing.T) { // the one whose consequence is not merely a confusing read. // // `pad item edit` is a read-modify-write over the WHOLE body: it seeds $EDITOR -// from the row and PATCHes the entire edited text back with no concurrency -// token. When the row is behind the live document, saving replaces edits that -// exist and are durable with a version derived from a state before them. The -// warning does not prevent that — BUG-3035 owns the refuse/prompt/diff decision — -// so what this test guards is that the signal exists, fires only for the defined -// value, and says the thing that distinguishes it from an ordinary stale read. +// from the row and PATCHes the entire edited text back. When the row is behind +// the live document, saving replaces edits that exist and are durable with a +// version derived from a state before them. Since BUG-3035 the command refuses +// that state unless --force (bug3035_edit_guard_test.go), so this warning is the +// forced path's signal; what this test guards is that the signal exists, fires +// only for the defined value, and says the thing that distinguishes it from an +// ordinary stale read. func TestWarnStaleEditSeedGating(t *testing.T) { cases := []struct { name string @@ -297,8 +298,15 @@ func TestEditWarnsBeforeLaunchingTheEditor(t *testing.T) { t.Cleanup(func() { workspaceFlag, formatFlag = origWS, origFormat }) workspaceFlag, formatFlag = "ws", "" + // --force on the stale leg: since BUG-3035 a stale read REFUSES by + // default (TestEditRefusesStaleSeed), so the warning is now the + // forced path's signal, and this test keeps guarding its ordering. + editArgs := []string{"TASK-5"} + if tc.wantWarning { + editArgs = append(editArgs, "--force") + } cmd := editCmd() - cmd.SetArgs([]string{"TASK-5"}) + cmd.SetArgs(editArgs) stderr := captureStderr(t, func() { _ = captureStdout(t, func() { if err := cmd.Execute(); err != nil { diff --git a/internal/server/bug3035_applier_seq_test.go b/internal/server/bug3035_applier_seq_test.go new file mode 100644 index 000000000..92df1515c --- /dev/null +++ b/internal/server/bug3035_applier_seq_test.go @@ -0,0 +1,89 @@ +package server + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) + +// BUG-3035: `pad item edit` now round-trips the `seq` that seeded the editor as +// `expected_seq`, and the whole design rests on one server claim — that the seq +// guard refuses on the APPLIER path too, BEFORE the content reaches the live +// document. That is the path the bug is about (an open tab), and the only prior +// pin of refuse-before-apply used the weak `expected_updated_at` token +// (TestBUG2840HalfA_RefusedPatchLeavesTheDocumentUntouched). +// +// Two legs against one room. The ACCEPT leg is not decoration: it proves this +// room really routes a content PATCH through the applier (the 200 carries +// content_outcome=applied_pending_flush), so the refusal leg's 409 is a statement +// about that path rather than about a direct write that happened to refuse. +func TestBUG3035_StaleSeqRefusesOnTheApplierPathBeforeApplying(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) + + seeded, err := srv.store.GetItem(item.ID) + if err != nil { + t.Fatalf("GetItem: %v", err) + } + if seeded.Seq < 1 { + t.Fatalf("a live row must carry seq >= 1; got %d", seeded.Seq) + } + + // ACCEPT leg: the seq the editor was seeded with is current, so the save lands + // — through the applier. + rr := doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/items/"+item.Slug, + map[string]interface{}{"expected_seq": seeded.Seq, "content": "first save"}) + if rr.Code != http.StatusOK { + t.Fatalf("current seq must be accepted; got %d: %s", rr.Code, rr.Body.String()) + } + var ok struct { + Warnings map[string]any `json:"warnings"` + } + _ = json.Unmarshal(rr.Body.Bytes(), &ok) + if outcome, _ := ok.Warnings["content_outcome"].(string); outcome != contentOutcomeAppliedPendingFlush { + t.Fatalf("the accepted save did not take the applier path (content_outcome=%q), so the "+ + "refusal below would say nothing about it", outcome) + } + + // REFUSE leg: a second editor seeded from the SAME read saves after the first. + // Its seq is now stale. + beforeOps := countOpLog(t, srv, item.ID) + rr = doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/items/"+item.Slug, + map[string]interface{}{"expected_seq": seeded.Seq, "content": "second save, seeded stale"}) + if rr.Code != http.StatusConflict { + t.Fatalf("a stale seq on the applier path must be refused 409; got %d: %s", rr.Code, rr.Body.String()) + } + var refusal struct { + Error struct { + Code string `json:"code"` + Details map[string]any `json:"details"` + } `json:"error"` + } + _ = json.Unmarshal(rr.Body.Bytes(), &refusal) + if refusal.Error.Code != "update_conflict" || refusal.Error.Details["conflict_type"] != "seq" { + t.Fatalf("want update_conflict on the seq token; got %s", rr.Body.String()) + } + // Nothing reached the document: op-log rows unchanged is what that reduces to + // on a dumb relay (see the BUG-2840 test for why the server cannot read the + // document itself). + if afterOps := countOpLog(t, srv, item.ID); afterOps != beforeOps { + t.Fatalf("the refused save left %d op-log row(s) behind; a tab joining later would replay "+ + "the content the caller was told was refused", afterOps-beforeOps) + } +}