mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
fix(cli): item edit refuses a stale body, guards its save with expected_seq, and keeps a recovery file on failure (BUG-3035) (#1418)
* fix(cli): item edit refuses a stale seed, guards the save with expected_seq, and never loses the edit (BUG-3035) Claude-Session: https://claude.ai/code/session_01X4uYKs5ZPF28eCWfm8VPvE * docs(cli): BUG-3035 prose sweep — edit refuses by default, the warning is the forced path's Claude-Session: https://claude.ai/code/session_01X4uYKs5ZPF28eCWfm8VPvE * fix(cli): BUG-3035 codex round 1 — help states the unflushed-typing gap (BUG-3133); a nonzero editor exit aborts and says so Claude-Session: https://claude.ai/code/session_01X4uYKs5ZPF28eCWfm8VPvE
This commit is contained in:
@@ -258,7 +258,9 @@ pad item unblock <source> <target>
|
||||
pad collection list # List collections
|
||||
pad collection create "Name" --fields "key:type[:opts]; ..." # compact DSL for simple schemas
|
||||
pad collection create "Name" --schema '<json>' # full CollectionSchema (terminal_options, defaults, computed, relations)
|
||||
pad item edit <ref> # Open in $EDITOR
|
||||
pad item edit <ref> [--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
|
||||
|
||||
@@ -498,7 +498,7 @@ pad item open <ref> Open item in web UI
|
||||
pad item update <ref> Update item fields
|
||||
pad item delete <ref> Delete item
|
||||
pad item move <ref> <collection> Move item between collections
|
||||
pad item edit <ref> Open item in $EDITOR
|
||||
pad item edit <ref> [--force] Open item in $EDITOR (guarded save; see --help)
|
||||
pad item search "query" Full-text search across all items
|
||||
pad item comment <ref> "text" Add comment to an item
|
||||
pad item comments <ref> View item comments
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
}
|
||||
+160
-12
@@ -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 <ref>",
|
||||
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.
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user