Avoid clobbering concurrently created rc files

[max-sixty]

Shell installation checked that a user rc file was absent before persisting an atomic temp file with overwrite semantics. A file created in that gap could therefore be replaced.

This adds write_new_atomically, backed by tempfile::persist_noclobber, and uses it only for missing user rc files. A concurrent creator now wins and Worktrunk fails safely for a rerun. A dangling rc symlink is also preserved and gets an actionable error explaining how to restore its target or remove the link. Existing rc files retain the small advisory append lock; the project guidance records why broader locks and extra prechecks do not close races with editors or dotfile managers, and identifies the remaining last-writer-wins merge into another tool's settings.json.

The change also replaces the custom one-syscall append helper with standard Write::write_all. The helper could report a partial append but could not prevent one, so removing it cuts 99 lines of special handling without weakening the no-truncation recovery boundary.

Validated with the full pre-merge hook: formatting, clippy, documentation, doctests, and 4,712 tests passed (1 skipped). After the review follow-ups, the focused rc-creation, concurrent-creator, dangling- and live-symlink, and no-clobber helper tests pass locally.

This was written by Codex on behalf of @max-sixty

[worktrunk-bot]

One verified behavior change, and a scope note on the new guidance.

write_new_atomically treats a dangling symlink as an existing file, where write_atomically replaced it. configure_shell_file gates the create branch on path.exists(), which follows symlinks and reports false for a link pointing nowhere. dunce::canonicalize then has nothing to resolve, so target stays the link path, and persist_noclobber (renameat2(RENAME_NOREPLACE), falling back to link(2)) sees the link itself and returns EEXIST. Checked on this branch by calling both functions against a dangling link in a tempdir:

noclobber on dangling symlink: Err(Os { code: 17, kind: AlreadyExists, message: "File exists" })
overwrite  on dangling symlink: Ok(())

So a user whose ~/.zshrc is a stow/chezmoi link into a store that isn't materialized now gets Failed to write to ~/.zshrc: File exists (os error 17) from shell install, against a path that reads as absent, with no way forward short of removing the link by hand. It fails safe, so nothing is lost — but it's the same dotfile-manager setup the Symlinks bullet on write_atomically is about, and that bullet's closing sentence ("A dangling link has nothing to resolve to, so it is replaced by a regular file rather than followed to the missing path it names") now describes only the overwrite = true half of the function it sits above. Suggestion inline records it on write_new_atomically. If the failure is the behavior you want, the message is the other half of the fix: File exists on a path the user can't see reads as a bug rather than a race they can rerun through.

The new boundary bullet leaves out Claude Code's settings.json. It closes with "Worktrunk-owned wrappers and completions use last-writer-wins semantics", which accounts for wrappers, completions, and rc files — but not the one remaining file that is neither Worktrunk-owned nor an rc file. configure_claude_statusline in src/commands/config/plugins.rs reads that file (or starts from an empty map when settings_path.exists() is false), merges statusLine, and hands the result to write_atomically — the same observed-absent-then-overwrite shape this PR fixes for rc files, with a read-modify-write window on top. Either answer seems defensible; since the bullet's job is to record where the boundary sits, a reader following it today would conclude every non-Worktrunk-owned file is covered.

Not approving on my own judgment rather than a written rule: the dangling-link case is a real change on the shell-install path, and whether to keep failing there is your call. CI is green on 4a551ea apart from affected tests (macos, advisory), still running.

[worktrunk-bot]

Both prior findings are addressed: the dangling-symlink divergence now fails with an actionable message and a Unix regression test that asserts the link survives, and the boundary bullet names the settings.json merge. Two smaller things, both inline.

The remaining gap is the sibling branch of the same AlreadyExists. The dangling link gets a tailored message; the concurrent-creator case — the one this PR exists to create — still surfaces as Failed to write to ~/.zshrc: File exists (os error 17). That's the PR's own user-facing output for its headline behavior, and the user is told nothing about the rerun the design depends on.

The CLAUDE.md bullet dropped Removal still rewrites those rc files whole (uninstall_previewed_lines), which is why the write_atomically spec still names them, and rc files also left the parenthetical inventory of what goes through write_atomically. But uninstall_previewed_lines still calls write_atomically(path, &new_content) on the rc file, and the spec's first paragraph still opens "rc files and shell wrappers the user's shell sources at startup". So the guidance and the spec now disagree about rc files, and the sentence that existed to keep them aligned is the one that went. The new concurrency bullet mentions "rc-file uninstall" but never says it is a write_atomically caller.

Holding approval per this repo's .claude/skills/running-tend/references/review-pr.md → Data-Loss Surface: Hold for Human Review: the diff edits src/commands/configure_shell.rs, which holds three fs::remove_file call sites (legacy-wrapper and stranded-Nushell cleanup), and the surface the change governs is rc files — named in that section as work a user can't get back. That section also says to request review from @max-sixty, which is a no-op here since you authored it. Nothing in the diff widens what can be deleted; the hold is on the file and surface, not on a defect.

CI on 436b9430e: fast-checks, lint, feature-check, affected tests (linux, advisory) and pre-commit.ci green; the three test legs and code-coverage still running.

[worktrunk-bot]

Both prior findings are addressed at 8e45b8848: the concurrent-creator branch now has its own message, and the uninstall_previewed_lines pointer is back in the Data Safety bullet (still accurate — uninstall_previewed_lines calls write_atomically(path, &new_content)). One new finding on the extracted helper, inline.

format_new_rc_error uses path.is_symlink() to mean dangling, but that predicate is also true for a live link. The only way to reach the arm at all is the race this PR exists to handle, and a dotfile manager is a realistic concurrent creator: chezmoi apply or stow landing ~/.zshrc as a symlink between the path.exists() check and persist_noclobber produces AlreadyExists on a link that resolves fine, and the user is told to "restore its target or remove the link". Following that advice deletes a working dotfile link. !path.exists() pins the arm to dangling; a live link then falls through to "another process created it first; rerun to append", which is both true and actionable, since a rerun takes the append branch.

Verified on this branch

Path::exists follows the link and Path::is_symlink does not, so the two cases are only separable together:

"live":     exists=true  is_symlink=true
"dangling": exists=false is_symlink=true
hard-link onto live symlink: Err(AlreadyExists)

The last line is the link(2) fallback persist_noclobber takes when renameat2(RENAME_NOREPLACE) is unavailable — a live symlink reaches the same AlreadyExists a dangling one does. test_configure_shell_reports_dangling_symlink still passes under the added condition, since its link is dangling.

Nothing else: dropping write_append_buffer for write_all is a net gain (write_all finishes a short write instead of turning it into an error), the removed test went with the helper it exercised, and all three arms of the new helper are covered.

Holding approval per this repo's .claude/skills/running-tend/references/review-pr.md → Data-Loss Surface: Hold for Human Review: the diff edits src/commands/configure_shell.rs, which holds three production fs::remove_file sites, and the surface it governs is rc files. Nothing in the diff widens what can be deleted — the hold is on the file and surface. That section also says to request review from @max-sixty, a no-op here since you authored it.

CI on 8e45b8848: fast-checks, lint, feature-check, code-coverage, affected tests (linux, advisory) and pre-commit.ci green; the three test legs, the macOS/Windows advisory legs, and codecov/patch not yet reported.

[worktrunk-bot]

The format_new_rc_error finding is resolved at ef573338e: path.is_symlink() && !path.exists() pins the dangling arm correctly (is_symlink reads symlink_metadata, exists follows the link, so a live link now falls through to "another process created it first; rerun to append" — which is the accurate advice for it), and test_new_rc_error_reports_live_symlink_as_concurrent_creator covers the newly-separated branch while test_configure_shell_reports_dangling_symlink still exercises the other. Nothing further from me on the diff.

Not approving, same reason as the prior two rounds and not a defect: this repo's .claude/skills/running-tend/references/review-pr.md → Data-Loss Surface: Hold for Human Review holds on a source change in a file carrying fs::remove_file sites, which src/commands/configure_shell.rs does (legacy-wrapper and stranded-Nushell cleanup). Nothing in the diff widens what can be deleted.

CI on ef573338e: fast-checks, lint, feature-check, gate, affected tests (linux, advisory) and pre-commit.ci green; the three test legs, code-coverage, the macOS/Windows advisory legs, and codecov/patch not yet reported.