[brettheap]

Closes #158

What was wrong

rename-lane publishes the Rule 3 RENAMED from … stamp into the handoff after proving the splice, and it did that with a direct redirect: cat -- "$rl_htmpd/out" > "$rl_hfile_new" (lanes-edit.sh:14383 at f87fd5a). The redirect truncates the handoff before the first stamped byte is written. If the writer is killed inside that cat, or the machine loses power there, the handoff is left cut short, and every later read takes the short file for the real one. The existing EXIT/INT/TERM rollback (rename_undo) never reaches that state, because a trap does not run on SIGKILL or a power loss.

The fix

The stamped bytes now go to an exclusive mktemp file beside the handoff, in the physical directory it resolves to (cd -P). cp -p gives that file the handoff's mode, and mv -f replaces the handoff in one rename. A reader sees the handoff whole before the stamp or whole after it. These are publish-handoff's mechanics (lanes-edit.sh:16285–16288 at f87fd5a), inlined because that verb takes the writer lock this write already holds. cleanup removes the temporary file on every handled exit. A file a crash leaves behind is named <handoff>.rename.XXXXXX, which no reader of *.md takes for a handoff.

Unchanged: the splice proof, the symlink refusals (a leaf link is still refused before the first byte, so replacing the leaf rather than writing through it is the same file), the mutex, and the rollback bookkeeping (RL_H_OLD/RL_H_NEW and the snapshot).

This makes the stamp atomic, not the rename. As #158 asks, the broader crash recovery of the four moves is not claimed; it is now tracked as #224. A crash between two of them still leaves them apart. rename_undo's own restores (log, handoff, alias table, register) are still in-place redirects. The register is written that way on purpose, because of the symlink hazard in the file header.

Two side effects, both shared with publish-handoff:

• In the in-place shape (a handoff not named …-lane-<old>.md), the handoff's directory must now be writable as well as the file. The moved shape already needed that for its mv. In a read-only directory the rename now refuses with exit 5 ("no temporary file could be made beside…") and rolls back whole, where f87fd5a stamped the file in place and exited 0. A test pins this.
• A hard link to the handoff is no longer written through.

Review fix round

The independent review's verdict was FIX:

• MEDIUM: rl_htmp was never set at top level, so cleanup (which runs on every exit) deleted whatever path that name inherited from the environment. With rl_htmp=<file> exported, a read-only canon-lane deleted the file. pbh_tmp had the same hole on main, and so did TMPD, which cleanup removes with rm -rf, so an exported TMPD=<dir> deleted the whole directory (managed-projection reads this checkout's row as the writers do, lane-trees tells an unlistable trees directory from no inventory, and the manual's seven-column row is qualified (#157 items 4a, 4c, 4d, 4e) #213's review found TMPD too).
  • Audit of every path the exit and signal traps remove, through cleanup and the two helpers it calls, rename_undo and release_lock:
    • pbh_tmp, rl_htmp, TMPD: not set at top level; now set empty.
    • SE_CACHE_FILE: first assigned only at the state-event cache (line 3090), so a signal between the trap and that line removed the inherited path. Now set empty.
    • RL_SNAP and the undo's RL_LOG_NEW, RL_H_NEW, RL_ALIAS, armed by RL_ACTIVE: already set at lines 1148–1151.
    • LANES_IDX_DIR, LANES_WT_IDX_DIR: already set at lines 1083 and 1089.
    • LOCK, armed by LOCK_HELD: already set at lines 1077 and 1091.
    • index_nudge removes nothing, and these are the only three traps in the file.
  • All four of the unset names now start empty before trap cleanup EXIT. The environment test exports a victim path under each of the nine names, with the arming flag beside it where there is one.
• LOW, docs: skills/handoff/SKILL.md still called rename's stamp "an explicit exception to this atomic publication contract". Lines 218–219 now say the stamp is atomically replaced and the four-file rename still is not crash-atomic (rename-lane: the four-file rename is still not crash-atomic, deferred from #158 #224). Checked with git merge-tree --write-tree in a private clone at 66a866e: clean against Handoff skill: resolve lanes-edit.sh on PATH first, correct the comments that still promised write (c) without a workstation, and refuse the object-log and register-line writes for a lane that has never had a session recorded #207, against Handoff skill: commit the handoff in the checkout it was published in, by its own path, and refuse foreign staged entries and in-progress operations (#197) #199, against both together, and against every other open PR (26). The same stale claim in docs/README-lanes.md:~4202–4206 is inside lane openRepoTools-3's fenced region and is recorded in rename-lane: the four-file rename is still not crash-atomic, deferred from #158 #224 instead.
• Follow-up: rename-lane: the four-file rename is still not crash-atomic, deferred from #158 #224 tracks the four-file crash recovery Harden legacy lane rename handoff publication against process crashes #158 set aside. It also records the reviewer's pre-existing finding (also on main) that a rolled-back moved handoff comes back at the default umask mode (0640 becomes 0644).
• INFO: the read-only-directory case now has a test.

Functions and line ranges edited (at this head)

lanes-edit.sh:

• Top level, lines 1204–1214: a comment and TMPD=""; pbh_tmp=""; rl_htmp=""; SE_CACHE_FILE="", placed directly above cleanup() and before trap cleanup EXIT.
• cleanup(), line 1217: one line added that removes rl_htmp, rename-lane's temporary stamp file, before rename_undo.
• The rename-lane dispatcher arm, step (d), the handoff stamp's publication: lines 14395–14419 replace the one-line redirect at old line 14383. That covers the comment, the cd -P resolution, mktemp, cp -p + cat + mv -f, and the two new refusals, both exit 5 like the write refusal they sit beside.

skills/handoff/SKILL.md: lines 218–219 only.

Nothing else in the file is touched. That keeps clear of commit_push (#194), :12621/:16616 (#157), retire_rows/archive_rows (#138), and lines ~13000–13350 and ~16940–16960.

Tests

New file tests/test_rename_publication_atomic_158.py. It runs a real rename-lane in test_lane_start_claude_current's sandbox: a bare origin, a workspace clone, a row and a log from a real lane-start --no-launch, and a handoff seeded at the path the row names. There is no network and no real lane data. The first three tests below run for both handoff shapes, one moved and then stamped and one stamped in place: 20 cases in all. Faults are injected by cat and mv stubs first on PATH. Each stub passes every other call through to the real utility.

• test_rename_stamp_is_the_same_splice_and_keeps_the_handoffs_mode: the normal path. The stamp is at line 5, and taking it back out gives the original bytes. The mode stays 0640, no stray file is left, and the committed blob equals the file.
• test_rename_writer_killed_around_the_stamp_leaves_the_handoff_whole[crash-mid-write|kill-before-replace]: SIGKILL of the writer inside the stamp's write, and between the write and the replace. The handoff holds the original bytes, whole.
• test_rename_replace_refused_or_interrupted_rolls_back_and_leaves_no_temporary[fail-replace|term-before-replace]: a failing replace (exit 5) and a SIGTERM between write and replace. The existing rollback restores every file, HEAD is unchanged, and no temporary file is left. On the signal path that last part is cleanup's doing.
• test_rename_in_place_handoff_in_a_read_only_directory_refuses_and_rolls_back: exit 5, "no temporary file could be made beside", a full rollback and no temporary file left. Skipped when run as root.
• test_cleanup_removes_no_path_a_variable_inherits_from_the_environment[…]: 9 cases, one for each of rl_htmp, pbh_tmp, TMPD, SE_CACHE_FILE, RL_SNAP, LANES_IDX_DIR, LANES_WT_IDX_DIR, rename_undo (RL_ACTIVE=1 with RL_LOG_NEW/RL_H_NEW/RL_ALIAS) and release_lock (LOCK_HELD=1 with LOCK). Each exports a victim path under that name, and the victim must survive a read-only canon-lane. The cases for names that were already set are guards, so they pass on main too. SE_CACHE_FILE's gap is a signal before its assignment, which a read-only command cannot reach, so its case also passes on main.

Every run used the shared lock, flock -w 1800 ${TMPDIR:-/tmp}/openrepotools-pytest.lock …, with python3 -m pytest tests/test_rename_publication_atomic_158.py -q -p no:cacheprovider. The f87fd5a and fcfd1b4 runs used a git archive copy of that commit holding this test file.

First round (lock taken 18:59:38Z):

• At f87fd5a: 8 failed, 2 passed. Only the normal path passes. Both crash-mid-write cases leave the handoff as its first two lines (assert b'# Session h...ne: repoZ-1\n' == b'# Session h...-\nHistory.\n'), and the other three fault cases fail on "the fault never fired".
• At fcfd1b4: 10 passed.

This fix round (lock taken 20:05:55Z):

• At fcfd1b4, the previous head, with -k inherits_from_the_environment: 3 failed, 6 passed. rl_htmp, pbh_tmp and TMPD deleted the victim.
• At f87fd5a, with -k "read_only_directory or inherits_from_the_environment": 3 failed, 7 passed. The read-only case exits 0 there, and pbh_tmp and TMPD delete the victim. rl_htmp does not exist on main.
• At 66a866e, the whole file: 20 passed in 37.52s.

bash -n lanes-edit.sh passes. The hygiene text rules for non-ASCII after $name and for a case inside $( ) find nothing in the changed lines. No new exit code is used. CI's Linux tests job is the suite of record.

🤖 Generated with Claude Code

[sourcery-ai]

Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 22 hours and 59 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

[sourcery-ai]

Reviewer's Guide

The PR hardens Rule 3 handoff publication by staging the stamped content beside the resolved handoff, preserving its mode, and atomically replacing it with one rename; cleanup and focused end-to-end fault tests cover normal, crash, signal, and replacement-failure paths without claiming broader rename crash recovery.

Sequence diagram for atomic Rule 3 handoff publication

sequenceDiagram
    participant RenameLane
    participant Handoff
    participant TempFile
    participant Reader

    RenameLane->>Handoff: cd -P resolve parent directory
    RenameLane->>TempFile: mktemp <handoff>.rename.XXXXXX
    RenameLane->>TempFile: cp -p handoff
    RenameLane->>TempFile: cat stamped output > temp file
    RenameLane->>Handoff: mv -f temp file to handoff
    Reader->>Handoff: read handoff
    Handoff-->>Reader: whole original or whole stamped file
    alt write or replace fails
        RenameLane->>TempFile: rm -f temporary file
        RenameLane->>RenameLane: die with exit 5 and rollback
    end

Loading

Flow diagram for crash-safe handoff stamp replacement

flowchart TD
    A[Prove Rule 3 splice] --> B[Resolve physical handoff directory with cd -P]
    B --> C[Create exclusive adjacent temporary file]
    C --> D[Copy handoff mode with cp -p]
    D --> E[Write stamped bytes to temporary file]
    E --> F{mv -f succeeds}
    F -->|yes| G[Temporary file becomes handoff in one rename]
    F -->|no| H[Remove temporary file]
    H --> I[Rollback and exit 5]
    E -. writer killed or power loss .-> J[Original handoff remains whole]
    G --> K[Cleanup removes temporary path reference]

Loading

File-Level Changes

Change	Details	Files
Publishes the Rule 3 handoff stamp through a same-directory temporary file and a single atomic rename instead of redirecting directly into the handoff.	Resolves the handoff’s physical directory with cd -P and creates an exclusive <handoff>.rename.XXXXXX temporary file beside it. Copies the original mode, writes the stamped bytes to the temporary file, and replaces the handoff with mv -f; failures remove the temporary and abort the rename. Cleans up the temporary path on handled exits while preserving the documented limitation that only stamp publication is atomic, not the broader four-move rename.	lanes-edit.sh
Adds end-to-end fault-injection coverage for whole-file handoff publication across both rename-lane handoff shapes.	Verifies the normal stamp preserves the splice, file mode, committed content, and leaves no temporary files. Simulates SIGKILL during writing and before replacement, asserting the original handoff remains complete. Simulates replacement failure and SIGTERM, asserting existing rollback, unchanged HEAD, and temporary-file cleanup.	tests/test_rename_publication_atomic_158.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#158	Publish the Rule 3 handoff stamp through an exclusive temporary file in the resolved handoff directory, then atomically replace the handoff so a crash during writing cannot leave partial handoff bytes.	✅
#158	Retain the existing splice proof, symlink handling, writer mutex, and rollback bookkeeping, without claiming that this makes the full four-file rename crash-atomic.	✅
#158	Add sandboxed tests for normal publication, write/replace interruptions and failures, handoff integrity, and existing rollback behavior.	✅

Possibly linked issues

• Harden legacy lane rename handoff publication against process crashes #158: The PR implements the requested atomic stamp replacement and adds sandboxed crash, interruption, and rollback tests.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud