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

[atlas-from-plumb]

Fixes #524
Fixes #528

Two holes in the session's read record, the state that the "changed since you read it" guard, strict mode and the same-mtime expected_mtime check rely on.

#524: a read recorded under one spelling was invisible to a write through another

Defect. Suppose a file is read through one spelling, a peer then changes it, and the session writes it through another spelling. The spellings can differ by a symlinked parent, by macOS /tmp versus /private/tmp, or by case on a case-insensitive volume. write_file's default guard let the overwrite through and discarded the peer's change. Strict mode refused the same flow with "has not been read".

Cause. ReadTracker keyed entries on filepath.Clean of the path as spelled. The write lock, WriteTracker and the undo store key on lockPathKey, which resolves symlinks and folds case where the volume folds it. So at the write the lookup missed, and changedSinceSessionRead reads "never read" as "not stale". The persisted read_tracking.path rows had the same spelling dependence.

Fix. Record, Mtime, recorded, Hydrate and the persist sink all key on lockPathKey. lockPathKey touches the filesystem, so it is resolved outside the tracker lock. Hydrate re-keys every row, so rows persisted by an older daemon under spelled paths come back canonical. When two spellings of one file collide, the later read wins whatever order the store returns them in: that is the newest version the session is known to have seen. internal/cli needs no production change, because both rehydration paths (rehydrateReads, rehydrateReadsForAgent) and the shard seeding go through Hydrate. Its only change is one new test file.

#528: after a write, plumb recorded whatever the path held by then

Defect. If an outside process writes the file between plumb's rename and the post-write bookkeeping, its content is recorded as this session's version. The session's next write then passes both guards over a change it never saw.

Cause. WriteDeps.recordWritten ran stat and hashed the path again after the write. The per-path lock excludes only plumb's own writers. WriteTracker.Record also ran stat on the path, so read_file's concurrent-edit note and file_status's last writer shared the hole.

Fix. safeWrite now returns writeResult.written, built by a new stagedSnapshot. It holds the SHA-256 of the bytes written and the mtime from an fstat of the staged temp file, taken after its last write and fsync and before the rename. A rename moves the inode with its mtime, so this is the target's version at the moment the rename lands. An fstat failure fails the write before the rename. recordWritten(ctx, path, v fileSnapshot) records v into both trackers and no longer touches the path.

Every caller passes the version it wrote:

• write_file
• edit_file, including apply_partial
• find_replace
• copy_file
• transaction_apply, including its verified fail_on_new_errors rollback
• rename_symbol and move_symbol, through their plans
• the symbol-edit tools
• undo_edit
• the fail_on_new_errors revert

Where plumb holds no bytes: rename_file. It moves an inode, so before the rename it takes a snapshot of the source through one descriptor (readSnapshot) and records that version. A later change at the destination differs from it, so the guards refuse rather than trust it. If the source can't be read, the version is unknown. recordWritten then records the write but invents no read state: a zero-mtime record would make every later write look stale.

Verification

• Regression tests first, red on origin/main for the stated reason:
  • Read tracking keys on the spelled path; an alias spelling makes the 'changed since you read it' guard fail open #524, internal/tools/read_tracker_alias_test.go. The guard fails open in both directions (read through the alias and write through the real path, and the reverse). The case-variant spelling also fails open, and strict mode says "has not been read". Hydrating a spelled row misses the canonical path.
  • Read tracking keys on the spelled path; an alias spelling makes the 'changed since you read it' guard fail open #524, internal/cli/conn_persist_alias_test.go. A pre-fix spelled read_tracking row fails to rehydrate for the canonical path after a restart. Confirmed red with read_tracker.go reverted.
  • recordWritten re-reads the path after a write; an outside writer's content is recorded as the session's #528, internal/tools/write_record_race_test.go. An outsider write lands inside the window through the adapter hook or LSP notification, which run before the bookkeeping. It uses two modes, mtime advanced and mtime preserved, the latter the cp -p/same-tick case where only the SHA can tell. The next unguarded write_file was not refused for write_file, edit_file, transaction_apply or rename_file.
• Positive controls, green:
  • The session's own writes alternating spellings are never flagged. This test was also red on main, which refused the write through the alias.
  • Strict mode accepts a read made through an alias and still refuses after a peer change.
  • The own-write chain write → edit (strict, no re-read) → transaction → rename → overwrite records exactly the on-disk version at each step.
  • An unreadable-source rename records the write but no read state.
  • read_file's concurrent-edit note still fires after an outsider.
• CI-faithful run: GOWORK=off GOTMPDIR=$PWD/.testcache go test ./internal/tools/... ./internal/cli/... ./internal/sessionstate/... ./internal/paths/... -count=1 passes.
• Race: go test -race over the affected internal/tools tests, and the Persist|Shard|Repin|LogicalAgent tests in internal/cli, passes.
• Lint and checks: golangci-lint run ./internal/tools/... ./internal/cli/... (v2.13.2) reports 0 issues. make check-size check-brief check-changelog passes.
• Mutation testing: 16 of 16 hand-applied mutants were KILLED. Each mutant compiled, the focused test set passed first on the unmutated tree, and the tree was restored and confirmed clean afterwards. Plumb's mutation_test was refused for over 30 minutes because another agent's run held the daemon's mutation slot, so these were applied by a script on the committed tree.
  • Read tracking keys on the spelled path; an alias spelling makes the 'changed since you read it' guard fail open #524:
    • Record, Mtime and recorded each keyed on the spelling, three mutants.
    • Hydrate kept the spelled key.
    • Hydrate collision: the last row wins.
  • recordWritten re-reads the path after a write; an outside writer's content is recorded as the session's #528:
    • recordWritten re-reads the path for the read tracker.
    • recordWritten runs stat on the path for the write tracker.
    • The zero-version guard is dropped.
    • stagedSnapshot hashes the wrong bytes.
    • stagedSnapshot's mtime is off by 1 ns.
    • rename_file re-reads the destination.
    • transaction_apply drops the written version.
    • edit_file and write_file re-read the path, one mutant each.
  • Two first-draft mutants did not compile (an unused variable). They were rewritten, then killed, and never counted as kills while invalid.
  • Not covered by a test: the cross-device safeWriteSibling branch, which needs a second filesystem, and the fstat failure of a staged temp file.

Review round 1 (folded in)

• B1 (recordWritten re-reads the path after a write; an outside writer's content is recorded as the session's #528, blocking): edit_file's reply mtime.
  • The mtime: line was a fresh os.Stat of the path, taken after the post-write diagnostics wait.
  • An outside write in that wait gave the agent an expected_mtime that let its next write clobber the change.
  • The reply and apply_partial's header now print writeResult.written, the version that was also recorded. apply_partial's header is now rendered after its post-write pipeline, as the ordinary reply is, so one test pins both. Output order is unchanged.
  • No other tool's reply prints a post-write mtime or SHA. The remaining current-mtime lines are in refusals and file_status, which report the file's present state by design.
  • Tests: write_reply_version_test.go. An outsider lands before the reply, the reply shows plumb's mtime, and a write guarded by it is refused, for both replies and both outsider modes. The positive control checks the reply mtime round-trips and a peer write afterwards is refused.
• N1 (Read tracking keys on the spelled path; an alias spelling makes the 'changed since you read it' guard fail open #524): the old-row tie-break in Hydrate.
  • A row stored under its canonical key now outranks a spelled row, which only an older daemon writes. File mtime decides only between two rows of one kind.
  • cp -p and rsync -t move the mtime backwards, and the stale row used to win.
  • Test: TestReadTracker_HydrateCanonicalRowOutranksSpelledRow.
• N2 (recordWritten re-reads the path after a write; an outside writer's content is recorded as the session's #528): when the staged file's mtime is read.
  • stagedSnapshot now calls Lstat on the staged name after Close and before the rename, instead of fstat on the open descriptor. Some filesystems (SMB, drvfs) stamp the mtime at close.
• Before and after:
  • The two new regression tests were red against the pre-review code, in the edit_file mtime-advances and hydrate cases. The apply_partial case was not red then, because its header was rendered before any hook could run; that is why the header now renders later.
  • After the fixes, the CI-faithful go test over tools, cli, sessionstate and paths passes, as do -race, the integration-tagged tools tests, lint (0 issues) and make check-size check-brief check-changelog.
• Mutation testing with plumb's mutation_test: 6 of 6 mutants were killed.
  • B1: edit_file reply re-stats the path, and apply_partial header re-stats the path. The header mutant first SURVIVED, which led to the render-order change, and was then killed.
  • N1: the canonical rank is dropped, and the last row wins among rows of one kind.
  • N2: the staged mtime is off by 1 ns.

🤖 Generated with Claude Code

[golimpio]

Two independent review rounds. Round 1's blocker (edit_file's reply mtime re-read the path) is fixed, and round 2 found nothing blocking. Every fix is proven by mutation.

[golimpio]

Re-approve after updating with main (#541).