fix(shell): rewrite rc files atomically on uninstall - #3585
Merged
Conversation
`wt config shell uninstall` finished its rc rewrite with `fs::write`, which truncates in place. A crash, a full disk, or a lost power cable between the truncate and the write left `~/.bashrc` empty or half-written. The new content now goes to a temp file beside the target and is renamed over it, so the rc file is either entirely the old version or entirely the new one. The target is resolved through symlinks first, so a dotfile manager's link survives, and its mode is copied onto the temp file before the rename.
worktrunk-bot
left a comment
worktrunk-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the atomic-rewrite change and traced the failure paths — it holds up. The temp-file-plus-rename mirrors write_approvals_file, and the two additions user rc files actually need are both correct: dunce::canonicalize resolves the symlink so the rename lands on the real file (link survives), and the target's mode is copied onto the temp before the fsync. Every failure point — temp creation in a read-only dir, write_all/sync_all, persist — drops the NamedTempFile and leaves the rc file exactly as it was, which the three Unix-gated tests exercise against the specific alternative each is meant to catch. The fs::remove_file sites elsewhere in this file are separate wholly-managed-file removals; this diff neither touches nor reaches them.
No actionable findings. Not auto-approving, though: per this repo's review policy, changes on the rc-rewrite / data-safety surface in src/commands/configure_shell.rs are held for your call rather than rubber-stamped by the bot — flagging that this is clean rather than approving it.
`write_rc` and `RC_AFTER_UNINSTALL` serve only the `#[cfg(unix)]` rewrite tests, so the Windows build saw them as dead code and `-D warnings` failed it.
The local gate runs one platform, so an item used only behind `#[cfg(unix)]` looks live to it and dead to `test (windows)`. Rewriting the predicate to `cfg(any())` reproduces that evaluation without a cross-compiler.
The rule (gate the item like its uses) is checkable by reading, so the reproduction command was buying nothing, and its `sed -i ''` was macOS-only syntax that rewrote tracked source.
max-sixty
added a commit
that referenced
this pull request
Jul 25, 2026
…3591) `fs::write` truncates in place, so a crash, a full disk, or a lost power cable between the truncate and the write leaves the file empty or half-written. #3585 fixed that for the rc file `wt config shell uninstall` rewrites and left the rest of the class open, pending a decision on whether every site should share one writer. It should, so this is that writer plus the sites it covers. ## Sites Four were truncating a file worktrunk can't put back: - `config/user/persistence.rs` — `UserConfig::save` reads the user's whole `config.toml`, merges, and writes it back. The highest-traffic of these, and the one not listed in #3585's follow-ups. Its lock is on `config.toml.lock`, a separate file, so the rename doesn't disturb it. - `commands/config/update.rs` — the migrated config `wt config update` writes. - `commands/config/plugins.rs` — the Claude `settings.json` the statusline setup merges `statusLine` into. Everything else in that file is the user's. - `commands/configure_shell.rs` — the rc file, shell wrapper, and fish completion `wt config shell install` creates. A half-written wrapper is one the user's shell sources at startup. Two already did this correctly, each in its own way: `write_approvals_file` and the uninstall rewrite from #3585. Both now call the shared function, so the rule is one place rather than a convention every site reimplements. `cache::write_json` and the `-vv` diagnostic report keep the plain write. A torn write there costs a recompute, not the user's data, and an fsync per cache entry isn't worth paying for. ## Why not a crate `atomic-write-file` is the serious candidate: same-directory temp, Windows and WASI support, and the only one that carries ownership across, which this helper can't. But it replaces a symlink rather than writing through it ("if the path of an `AtomicWriteFile` is a symlink to another file, the symlink is replaced"), so the canonicalize step stays either way, and it costs `nix` plus `rand`. `atomicwrites` does neither mode nor symlinks and creates a temp subdirectory per write. `tempfile` is already a runtime dependency and does the hard part, which left about ten lines. The comparison is in the function's docstring so it isn't re-litigated. ## Behavior changes A newly *created* file lands at `0600` rather than the `0644` a truncating write leaves under a default umask. An existing target's mode is copied across, but a fresh temp file is `0600` and there is no non-racy way to read the umask from std. Everything written here is read by its own owner (`.bashrc`, `wt.fish`, `config.toml`, `settings.json`), it fails safe, and `approvals.toml` has shipped this way since it started using a temp file. Creating the temp file needs a writable parent directory, which a truncating write did not, so a read-only directory holding a writable file now fails with the file left as it was. ## Testing The three uninstall tests from #3585 now exercise the shared function: mode preserved with no temp file left behind, the rewrite following a symlink into a different directory, and the rc file intact when the write fails. Two new unit tests cover what those don't — the create-then-replace round trip leaving no stray, and the `0600` a created file gets. Also driven through the real CLI rather than only tests. `wt config update` against a `0644` `config.toml` symlinked into a dotfiles directory rewrote the real file and left the link, the mode, and the directory as they were. `wt config shell install` into a fresh `$HOME` produced the `0600` rc file, wrapper, and completion above. > _This was written by Claude Code on behalf of max_ Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
max-sixty
added a commit
that referenced
this pull request
Jul 25, 2026
Eleven commits landed on `main` while #3590 sat in CI, so they ship in v0.69.2 with no changelog entry. Most are user-facing, and three are data-loss fixes — exactly the drift the release skill's step-12 check exists to catch. The tag is held until this lands. Entries added, most-impactful first: - **#3589** — two ways shell integration deleted user data on a substring guess: an rc line that only *quotes* the init command, and a user's own `conf.d/wt.fish` deleted outright by `install`'s legacy cleanup. Plus forge detection moving from `host.contains("github")` to label-wise matching. - **#3585 + #3591** — truncate-in-place rc writes replaced by write-temp-then-rename, then generalized to every user-file write. - **#3578** — despite its `docs(step):` subject this carries three behavior fixes, including `wt step push` succeeding mid-rebase and moving the target branch onto a half-replayed history. - **#3588 + #3587** — conflict markers can no longer reach a commit via `wt step relocate --commit`, and `wt merge` refuses in its own name. - **#3554** — OpenCode marker writes could land in another session's worktree (thanks @4i3n6, who both reported and fixed it). Not documented, per convention: #3574 (doc comments only, no runtime or docs-site surface) and the three dependency bumps. Also corrects `.github/CLAUDE.md`, which lists three required status checks; there are four — `fast-checks` is required too, confirmed against the branch-protection API. Entries verified against the diffs by subagent. Two corrections came back and are folded in: the #3589 entry originally promised "two of them deleted" and described only one, and the #3578 entry mis-quoted the success line as `✓ Pushed to main` when it carries a commit count. > _This was written by Claude Code on behalf of max_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wt config shell uninstallended its rc rewrite withfs::write, which truncates in place. A crash, a full disk, or a lost power cable between the truncate and the write left~/.bashrc,~/.zshrc, or a PowerShell profile empty or half-written, taking every line the user ever added to it. That is the silent loss CLAUDE.md's Data Safety section rules out.The new content now goes to a temp file created beside the target and renamed over it, the same shape
write_approvals_filealready uses. The rc file ends up either entirely the old version or entirely the new one.Two details a bare
NamedTempFile::new()pluspersist()gets wrong:~/.zshrcinto a repo or a synced folder.fs::writefollowed the link and updated the real file; a rename would replace the link itself with a regular file and silently detach the user's setup. The target is resolved first, so the link survives.0600, an rc file is typically0644. The target's mode is copied onto the temp file before the rename.Ownership is not preserved: the replacement belongs to the running user, so a root-run uninstall against another user's rc file would change its owner. Restoring it needs a
chown, which no current dependency exposes.Why only uninstall
The other user-file writes in this module do not have the same exposure:
configure_shell_fileappends to an existing rc withOpenOptions::append. A crash there can leave a partial integration line, but it never touches what was already in the file. Converting it to a full-file rewrite would introduce the truncate window this PR is removing.configure_wrapper_fileand the fish completion write replace files whose entire content worktrunk generates, andwt config shell installregenerates them.Uninstall is the only place worktrunk truncates a file whose other contents belong to the user.
Behavior change
Creating the temp file needs write access to the target's directory, which a truncating write did not. Uninstall now fails where a read-only directory holds a writable rc file, leaving the rc file as it was.
Testing
Three unit tests, Unix-gated since mode and symlink semantics differ on Windows: mode preserved with no temp file left behind, the rewrite following a symlink into a different directory, and the rc file intact when the write fails. Each was checked against the alternative it is meant to catch. The failure test fails under the old
fs::write; the mode and symlink tests fail under a naiveNamedTempFile::new()pluspersist(). I also ran the real CLI against a fake home whose.bashrcis a 0644 file behind a symlink, and confirmed the link, the mode, and the surrounding lines all survived.Their shared helper and expected-output const are
#[cfg(unix)]for the same reason, which the first push got wrong: used only behind#[cfg(unix)], they read as live to the local gate and as dead code totest (windows)under-D warnings.tests/CLAUDE.mdnow records that blind spot, since the gate runs one platform and structurally cannot catch this class.Same bug class elsewhere, not fixed here
Two writes outside this module read a user-authored file, modify it, and write it back with a truncating
fs::write:src/commands/config/update.rs:114rewrites the user'swt.tomlor user config with migrated content.src/commands/config/plugins.rs:144mergesstatusLineinto the user's Claudesettings.json.Fixing those wants a prior decision on whether every site,
write_approvals_fileincluded, should go through one shared writer. That is worth its own review rather than a rider on this one.