fix(tools): key read tracking canonically and record the version a write wrote (#524, #528)

CHANGELOG.md

@@ -71,6 +71,33 @@
   descriptor. A file replaced during the read keeps the version read. One
   rewritten in place is read again, and one that never settles records no hash.
   Applies to `read_file` (windowed and pattern search) and `read_symbol`.
+- **A write records the version it wrote.** After a successful write, plumb
+  refreshed the session's read record by stat'ing and hashing the path again.
+  The per-path lock excludes only plumb's own writers, so a process outside
+  plumb that wrote the file in that gap had its content recorded as this
+  session's version, and the session's next write passed "changed since you
+  read it" and the same-mtime `expected_mtime` check over a change it never
+  saw. Writes now record the hash of the bytes plumb wrote and the mtime of the
+  file it wrote them to, taken from the closed staged file just before the
+  rename publishes it. `rename_file`, which writes no bytes, records the version
+  it moved, read from the source before the move. Applies to every write tool,
+  `undo_edit`, and the `fail_on_new_errors` rollbacks. `edit_file`'s reply had
+  the same gap: its `mtime:` line re-read the path after the post-write
+  diagnostics wait, so an outside write in that wait handed the caller an
+  `expected_mtime` that let its next write through. The reply now prints the
+  version plumb wrote, with or without `apply_partial` (issue #528).
+- **A file read through one spelling and written through another keeps its read
+  record.** Read tracking keyed a read on the path as spelled, while the write
+  lock, write tracking and undo resolve symlinks and fold case where the volume
+  does. A file read through a symlinked parent, macOS `/tmp` versus
+  `/private/tmp`, or a case variant, and then written through another spelling,
+  had no read record at the write: the "changed since you read it" guard let the
+  write overwrite a peer's change, and strict mode refused the edit as unread.
+  Reads are now keyed the way writes are, in memory and in the persisted
+  session state. Rows saved by an older daemon are re-keyed when they are
+  restored after a restart; where one collides with a row this version saved,
+  the newer row wins even if a tool such as `cp -p` moved the file's mtime
+  backwards (issue #524).
 - **Contested-pin messages no longer recommend `session_id` as the fix.** The
   contested-pin note in `session_start`, the boundary and re-pin refusals, and
   the `git`, `run_task` and `undo_edit` refusals told agents sharing a

internal/cli/conn_persist_alias_test.go

@@ -0,0 +1,59 @@
+package cli
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/plumbkit/plumb/internal/config"
+	"github.com/plumbkit/plumb/internal/sessionstate"
+)
+
+// TestPersist_SpelledReadRowRehydratesUnderEverySpelling: a daemon predating
+// issue #524 persisted a read under the path as the agent spelled it. After a
+// restart that row must answer for the file under any spelling, or strict mode
+// refuses an edit through the canonical path ("has not been read") and the
+// default staleness guard fails open over a peer's change.
+func TestPersist_SpelledReadRowRehydratesUnderEverySpelling(t *testing.T) {
+	t.Setenv("XDG_DATA_HOME", t.TempDir())
+	store := config.NewStore(config.Defaults())
+	ss, err := sessionstate.Open()
+	if err != nil {
+		t.Fatalf("sessionstate.Open: %v", err)
+	}
+	defer ss.Close()
+
+	root := freshTempDir(t)
+	mustGitDir(t, root)
+	if err := os.MkdirAll(filepath.Join(root, "real"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(filepath.Join(root, "real"), filepath.Join(root, "alias")); err != nil {
+		t.Fatal(err)
+	}
+	realPath := filepath.Join(root, "real", "a.go")
+	if err := os.WriteFile(realPath, []byte("package a\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	mtime := time.Unix(1_700_000_000, 444)
+
+	// An older daemon's row: keyed by the alias spelling, written straight to
+	// the store as that daemon's persist sink would have.
+	before := newPersistSession(t, store, ss, "proxyX")
+	before.attachWorkspace(context.Background(), "file://"+root)
+	ws := before.view().acquiredRoot
+	if err := ss.UpsertRead("proxyX", ws, filepath.Join(root, "alias", "a.go"), mtime, "sha-a"); err != nil {
+		t.Fatal(err)
+	}
+	before.close()
+
+	after := newPersistSession(t, store, ss, "proxyX")
+	after.attachWorkspace(context.Background(), "file://"+root)
+	for _, p := range []string{realPath, filepath.Join(root, "alias", "a.go")} {
+		if got := after.readTracker.Mtime(p); !got.Equal(mtime) {
+			t.Fatalf("rehydrated Mtime(%s) = %v, want %v — a spelled row must answer for every spelling", p, got, mtime)
+		}
+	}
+}

internal/tools/copy_file.go

@@ -111,10 +111,11 @@ func (t *CopyFile) Execute(ctx context.Context, raw json.RawMessage) (string, er
 	if err != nil {
 		return "", err
 	}
-	if _, err := safeWrite(to, data, perm); err != nil {
+	res, err := safeWrite(to, data, perm)
+	if err != nil {
 		return "", fmt.Errorf("copy_file: writing destination: %w", err)
 	}
-	t.copyFilePostWrite(ctx, to)
+	t.copyFilePostWrite(ctx, to, res.written)
 	return fmt.Sprintf("copied %s → %s (%d bytes)", from, to, len(data)), nil
 }
 
@@ -162,11 +163,11 @@ func copyFilePreconditions(ctx context.Context, deps WriteDeps, from, to string,
 	return data, info.Mode().Perm(), nil
 }
 
-func (t *CopyFile) copyFilePostWrite(ctx context.Context, to string) {
+func (t *CopyFile) copyFilePostWrite(ctx context.Context, to string, written fileSnapshot) {
 	if err := notifyLSP(ctx, t.deps.Client, to, protocol.FileCreated); err != nil {
 		slog.Warn("copy_file: LSP create-notify failed", "path", to, "err", err)
 	}
 	invalidateCache(t.deps.Cache, "file://"+to)
 	t.deps.notifyTopology(to)
-	t.deps.recordWritten(ctx, to)
+	t.deps.recordWritten(ctx, to, written)
 }

internal/tools/edit_apply.go

@@ -80,13 +80,15 @@ func applyWorkspaceEditDetailed(we *protocol.WorkspaceEdit, onApplied func([]wor
 	}
 
 	var modified []string
-	for _, p := range plans {
-		if _, err := safeWrite(p.path, p.after, p.mode); err != nil {
+	for i, p := range plans {
+		res, err := safeWrite(p.path, p.after, p.mode)
+		if err != nil {
 			if rbErr := rollbackWorkspaceEdit(plans, modified); rbErr != nil {
 				return modified, plans, fmt.Errorf("writing %s: %w; rollback failed: %w", p.path, err, rbErr)
 			}
 			return modified, plans, fmt.Errorf("writing %s: %w", p.path, err)
 		}
+		plans[i].written = res.written
 		modified = append(modified, p.path)
 	}
 	if onApplied != nil {
@@ -100,6 +102,8 @@ type workspaceEditPlan struct {
 	before []byte
 	after  []byte
 	mode   os.FileMode
+	// written is the version the apply published, set once the write lands.
+	written fileSnapshot
 }
 
 // workspaceEditTarget is one file's share of a WorkspaceEdit, resolved to a

internal/tools/edit_file_apply.go

@@ -54,7 +54,7 @@ func (t *EditFile) editFileApply(ctx context.Context, path string, a editFileArg
 			}
 		}
 		invalidateCache(t.deps.Cache, uri)
-		t.deps.recordWritten(ctx, path)
+		t.deps.recordWritten(ctx, path, result.written)
 		t.deps.recordUndo(ctx, path, before, content, true, "edit_file")
 
 		// Still inside the per-path lock taken in Execute: the write, the
@@ -67,12 +67,19 @@ func (t *EditFile) editFileApply(ctx context.Context, path string, a editFileArg
 				before: before, existedBefore: true, wrote: content, diag: diag,
 			})
 		}
-		return t.formatEditFileSuccess(path, attempt, a.Edits, before, content, notes, diag), nil
+		return t.formatEditFileSuccess(path, attempt, a.Edits, before, content, notes, diag, result.written), nil
 	}
 	return "", fmt.Errorf("edit_file: failed after %d attempts: %w", maxEditRetries, lastErr)
 }
 
-func (t *EditFile) formatEditFileSuccess(path string, attempt int, edits []strEdit, before, content string, notes []string, diag postWriteDiagResult) string {
+// formatEditFileSuccess renders the reply. Its mtime line is written — the
+// version this edit published and recordWritten recorded — never a re-stat of
+// the path: this runs after the post-write diagnostics wait (seconds, with
+// await_diagnostics), and an outside writer landing in it would otherwise hand
+// the caller ITS mtime. Passed back as expected_mtime, that matched the file,
+// and changedAtSameMtime could not second-guess it (the recorded read is at
+// plumb's mtime), so the next write went over a change it never saw (#528).
+func (t *EditFile) formatEditFileSuccess(path string, attempt int, edits []strEdit, before, content string, notes []string, diag postWriteDiagResult, written fileSnapshot) string {
 	noun := "edit"
 	if len(edits) > 1 {
 		noun = "edits"
@@ -86,8 +93,8 @@ func (t *EditFile) formatEditFileSuccess(path string, attempt int, edits []strEd
 	if attempt > 1 {
 		fmt.Fprintf(&sb, " (succeeded on attempt %d)", attempt)
 	}
-	if info, err := os.Stat(path); err == nil {
-		fmt.Fprintf(&sb, "\nmtime: %s", info.ModTime().Format(time.RFC3339Nano))
+	if !written.mtime.IsZero() {
+		fmt.Fprintf(&sb, "\nmtime: %s", written.mtime.Format(time.RFC3339Nano))
 	}
 	for _, n := range notes {
 		fmt.Fprintf(&sb, "\n%s", n)

internal/tools/edit_file_partial.go

@@ -34,14 +34,19 @@ func (t *EditFile) executePartial(
 	baseline := t.deps.capturePreWriteBaseline(ctx, uri)
 	results, res, original, content, writeErr := t.tryEditPartial(ctx, path, edits)
 	applied := countApplied(results)
-	var sb strings.Builder
-	sb.WriteString(t.formatPartialHeader(path, original, content, applied, len(edits), writeErr))
-	sb.WriteString(formatPartialEditsResults(results))
+	var post strings.Builder
 	if writeErr == nil && applied > 0 {
-		_ = res
-		t.executePartialPostWrite(ctx, path, uri, original, content, awaitFresh, &sb, baseline)
+		t.executePartialPostWrite(ctx, path, uri, original, content, res.written, awaitFresh, &post, baseline)
 		t.deps.recordUndo(ctx, path, original, content, true, "edit_file")
 	}
+	// The header is rendered after the post-write pipeline, as edit_file's
+	// ordinary reply is: that pipeline is where an outside write can land, so
+	// both replies face the same window and one regression test pins both to
+	// the written version (#528). The output order is unchanged.
+	var sb strings.Builder
+	sb.WriteString(t.formatPartialHeader(path, original, content, applied, len(edits), writeErr, res.written))
+	sb.WriteString(formatPartialEditsResults(results))
+	sb.WriteString(post.String())
 	return sb.String()
 }
 
@@ -55,26 +60,28 @@ func countApplied(results []partialEditResult) int {
 	return n
 }
 
-func (t *EditFile) formatPartialHeader(path, original, content string, applied, total int, writeErr error) string {
+func (t *EditFile) formatPartialHeader(path, original, content string, applied, total int, writeErr error, written fileSnapshot) string {
 	switch {
 	case writeErr != nil:
 		return fmt.Sprintf("partial apply: write failed after %d successful edit(s): %v\n\n", applied, writeErr)
 	case applied == 0:
 		return "partial apply: all edits failed — file not modified\n\n"
 	default:
-		return t.formatPartialAppliedHeader(path, original, content, applied, total)
+		return t.formatPartialAppliedHeader(path, original, content, applied, total, written)
 	}
 }
 
 // formatPartialAppliedHeader renders the header for the case where at least one
-// edit landed and the write succeeded: the count, the fresh mtime, a line-change
-// summary, and (when enabled) the diff.
-func (t *EditFile) formatPartialAppliedHeader(path, original, content string, applied, total int) string {
+// edit landed and the write succeeded: the count, the written version's mtime, a
+// line-change summary, and (when enabled) the diff. The mtime is the version the
+// write published, not a re-stat of the path, for the reason formatEditFileSuccess
+// gives (#528).
+func (t *EditFile) formatPartialAppliedHeader(path, original, content string, applied, total int, written fileSnapshot) string {
 	var sb strings.Builder
 	fmt.Fprintf(&sb, "partial apply: applied %d of %d edit(s) to %s (%d bytes)\n",
 		applied, total, path, len(content))
-	if info, err := os.Stat(path); err == nil {
-		fmt.Fprintf(&sb, "mtime: %s\n", info.ModTime().Format(time.RFC3339Nano))
+	if !written.mtime.IsZero() {
+		fmt.Fprintf(&sb, "mtime: %s\n", written.mtime.Format(time.RFC3339Nano))
 	}
 	if s := summariseLineChanges(original, content); s != "" {
 		fmt.Fprintf(&sb, "%s\n", s)
@@ -108,7 +115,7 @@ func formatPartialEditsResults(results []partialEditResult) string {
 	return sb.String()
 }
 
-func (t *EditFile) executePartialPostWrite(ctx context.Context, path, uri, before, content string, awaitFresh bool, sb *strings.Builder, baseline *diagBaseline) {
+func (t *EditFile) executePartialPostWrite(ctx context.Context, path, uri, before, content string, written fileSnapshot, awaitFresh bool, sb *strings.Builder, baseline *diagBaseline) {
 	notifyFailed := false
 	if err := notifyLSP(ctx, t.deps.Client, path, protocol.FileChanged); err != nil {
 		notifyFailed = true
@@ -121,7 +128,7 @@ func (t *EditFile) executePartialPostWrite(ctx context.Context, path, uri, befor
 		}
 	}
 	invalidateCache(t.deps.Cache, uri)
-	t.deps.recordWritten(ctx, path)
+	t.deps.recordWritten(ctx, path, written)
 	// apply_partial cannot request fail_on_new_errors (the preconditions refuse
 	// the combination), so this path only ever reports.
 	opt := postWriteDiagOpts{awaitFresh: awaitFresh, structured: awaitFresh, lspNotifyFailed: notifyFailed}

internal/tools/fail_on_new_errors.go

@@ -141,24 +141,26 @@ func (d WriteDeps) revertWrite(ctx context.Context, req rollbackRequest) (holds
 		if err := os.Remove(req.path); err != nil && !os.IsNotExist(err) {
 			return "the content this call wrote (the file plumb created is still there)", fmt.Errorf("removing %q: %w", req.path, err)
 		}
-		d.notifyReverted(ctx, req.path, req.uri, protocol.FileDeleted)
+		d.notifyReverted(ctx, req.path, req.uri, protocol.FileDeleted, fileSnapshot{})
 		return "", nil
 	}
 	perm := os.FileMode(0o644)
 	if info, statErr := os.Stat(req.path); statErr == nil && info.Mode().Perm() != 0 {
 		perm = info.Mode().Perm()
 	}
-	if _, err := safeWrite(req.path, []byte(req.before), perm); err != nil {
+	res, err := safeWrite(req.path, []byte(req.before), perm)
+	if err != nil {
 		return "the content this call wrote (the restore itself failed)", fmt.Errorf("restoring %q: %w", req.path, err)
 	}
-	d.notifyReverted(ctx, req.path, req.uri, protocol.FileChanged)
+	d.notifyReverted(ctx, req.path, req.uri, protocol.FileChanged, res.written)
 	return "", nil
 }
 
 // notifyReverted mirrors the post-write notification, so the language server,
 // the symbol cache, the topology index and this session's own read/write state
-// all see the restored content rather than the reverted one.
-func (d WriteDeps) notifyReverted(ctx context.Context, path, uri string, ct protocol.FileChangeType) {
+// all see the restored content rather than the reverted one. restored is the
+// version the restoring write published (unused for a deletion).
+func (d WriteDeps) notifyReverted(ctx context.Context, path, uri string, ct protocol.FileChangeType, restored fileSnapshot) {
 	if err := notifyLSP(ctx, d.Client, path, ct); err != nil {
 		slog.Warn("fail_on_new_errors: LSP notification after rollback failed", "path", path, "err", err)
 	}
@@ -169,7 +171,7 @@ func (d WriteDeps) notifyReverted(ctx context.Context, path, uri string, ct prot
 	}
 	invalidateCache(d.Cache, uri)
 	if ct != protocol.FileDeleted {
-		d.recordWritten(ctx, path)
+		d.recordWritten(ctx, path, restored)
 	}
 	d.notifyTopology(path)
 }

internal/tools/file_write_helpers.go

@@ -231,6 +231,10 @@ type writeResult struct {
 	// the temp file. Used as a reference to detect whether the target was
 	// modified by a third party after we started but before our rename landed.
 	tempWrittenAt time.Time
+	// written is the version this write published (stagedSnapshot): the hash of
+	// the bytes written and the closed staged file's mtime, which the rename
+	// carries to the target. recordWritten records it rather than re-reading the path.
+	written fileSnapshot
 }
 
 // safeWrite writes data to path using temp-file-then-atomic-rename.
@@ -308,6 +312,10 @@ func safeWrite(path string, data []byte, perm os.FileMode) (writeResult, error)
 		_ = os.Remove(tmpPath)
 		return res, fmt.Errorf("closing temp file: %w", err)
 	}
+	if res.written, err = stagedSnapshot(tmpPath, data); err != nil {
+		_ = os.Remove(tmpPath)
+		return res, fmt.Errorf("stat temp file: %w", err)
+	}
 
 	res.tempWrittenAt = time.Now()
 
@@ -359,6 +367,10 @@ func safeWriteSibling(path string, data []byte, perm os.FileMode, modTimeBefore
 		_ = os.Remove(sibling)
 		return res, fmt.Errorf("closing sibling temp file: %w", err)
 	}
+	if res.written, err = stagedSnapshot(sibling, data); err != nil {
+		_ = os.Remove(sibling)
+		return res, fmt.Errorf("stat sibling temp file: %w", err)
+	}
 	res.tempWrittenAt = time.Now()
 
 	if err := os.Rename(sibling, path); err != nil {

internal/tools/find_replace.go

@@ -317,7 +317,7 @@ func (t *findReplaceTool) findReplaceProcessFile(ctx context.Context, path strin
 		unlock()
 		return fmt.Errorf("find_replace: %q has uncommitted changes; review and commit first, or pass dirty_ok: true to proceed", path)
 	}
-	_, writeErr := safeWrite(path, newData, 0o644)
+	res, writeErr := safeWrite(path, newData, 0o644)
 	unlock()
 	if writeErr != nil {
 		return fmt.Errorf("find_replace: writing %s: %w", path, writeErr)
@@ -326,7 +326,7 @@ func (t *findReplaceTool) findReplaceProcessFile(ctx context.Context, path strin
 		slog.Warn("find_replace: LSP notification failed", "path", path, "err", err)
 	}
 	invalidateCache(t.deps.Cache, "file://"+path)
-	t.deps.recordWritten(ctx, path)
+	t.deps.recordWritten(ctx, path, res.written)
 	return nil
 }
 

internal/tools/move_symbol.go

@@ -269,7 +269,7 @@ func (t *MoveSymbol) applyMove(ctx, lspCtx context.Context, waited time.Duration
 			return
 		}
 		for _, p := range plans {
-			deps.recordWritten(ctx, p.path)
+			deps.recordWritten(ctx, p.path, p.written)
 			deps.recordUndo(ctx, p.path, string(p.before), string(p.after), p.existedBefore, "move_symbol")
 		}
 	}
@@ -540,6 +540,8 @@ type movePlan struct {
 	after         []byte
 	mode          os.FileMode
 	existedBefore bool
+	// written is the version the move published, set once the write lands.
+	written fileSnapshot
 }
 
 // applyMovePlans writes each plan in order and rolls every prior write back on a
@@ -550,13 +552,15 @@ type movePlan struct {
 // also directly unit-testable.
 func applyMovePlans(plans []movePlan, onApplied func()) ([]string, error) {
 	var written []movePlan
-	for _, p := range plans {
-		if _, err := safeWrite(p.path, p.after, p.mode); err != nil {
+	for i, p := range plans {
+		res, err := safeWrite(p.path, p.after, p.mode)
+		if err != nil {
 			if rbErr := rollbackMove(written); rbErr != nil {
 				return nil, fmt.Errorf("writing %s: %w; rollback failed: %w", p.path, err, rbErr)
 			}
 			return nil, fmt.Errorf("writing %s: %w", p.path, err)
 		}
+		plans[i].written = res.written
 		written = append(written, p)
 	}
 	if onApplied != nil {