fix(config): route every user-file write through one atomic writer - #3591
Merged
Conversation
`fs::write` truncates in place, so a crash between the truncate and the write left the file empty or half-written. Four sites did that over files worktrunk can't put back: the user's `config.toml` on every `UserConfig::save`, the migrated config `wt config update` writes, the Claude `settings.json` the statusline setup merges into, and the rc file, shell wrapper, and fish completion `wt config shell install` creates. `utils::write_atomically` now owns all of them, along with the two sites that already did this correctly on their own — `write_approvals_file` and the uninstall rewrite from #3585 — so the rule is one function rather than a convention each site reimplements. It resolves symlinks before the rename (dotfile managers link `~/.zshrc` into a repo), keeps the temp file beside the target, and carries the target's mode across. A newly created file lands at 0600 rather than the 0644 a truncating write leaves under a default umask: a fresh temp file is 0600 and there is no non-racy way to read the umask. Everything written here is read by its own owner, and `approvals.toml` already behaved this way. The cache and the -vv diagnostic report keep the plain write — a torn write there costs a recompute, not the user's data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the consolidation carefully — it's clean, and I couldn't find a correctness gap. Verification notes:
- Coverage is complete. Every remaining production
fs::writeinsrc/commands/andsrc/config/is a test fixture; the only non-test plain writes left are the two you exclude on purpose —cache::write_json(src/cache.rs, already documented as plain) and the-vvdiagnostic report.codex.rsand the Gemini path write no files, so there was no fourth installer to route. - No leftover dead code. After both helpers collapse into
write_atomically,save_parentis still live (thecreate_dir_allinApprovals::save_to), andio/Write/tempfileare still used elsewhere inconfigure_shell.rs— no unused-import fallout for-D warnings. - Docstring cross-references resolve.
crate::cache::write_jsonexists and does keep the plain write, so the "regenerable content" carve-out is accurate. - The empty-parent guard (
Some("")→.), the canonicalize-then-temp-beside-target symlink handling, and the pre-sync mode copy all look right, and the dangling-symlink note matches whatcanonicalizefailure falls back to.
One process note, not a code change: this touches worktrunk's data-safety surface — configure_shell.rs (which holds the fs::remove_file cleanup calls and the uninstall rewrite) plus the config.toml / approvals.toml writers. Per the repo's own review policy that surface isn't mine to green-light, so I'm leaving this as a comment rather than an approval. Since you authored it, that call is already yours.
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
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.
fs::writetruncates 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 filewt config shell uninstallrewrites 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::savereads the user's wholeconfig.toml, merges, and writes it back. The highest-traffic of these, and the one not listed in fix(shell): rewrite rc files atomically on uninstall #3585's follow-ups. Its lock is onconfig.toml.lock, a separate file, so the rename doesn't disturb it.commands/config/update.rs— the migrated configwt config updatewrites.commands/config/plugins.rs— the Claudesettings.jsonthe statusline setup mergesstatusLineinto. Everything else in that file is the user's.commands/configure_shell.rs— the rc file, shell wrapper, and fish completionwt config shell installcreates. 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_fileand 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_jsonand the-vvdiagnostic 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-fileis 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 anAtomicWriteFileis a symlink to another file, the symlink is replaced"), so the canonicalize step stays either way, and it costsnixplusrand.atomicwritesdoes neither mode nor symlinks and creates a temp subdirectory per write.tempfileis 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
0600rather than the0644a truncating write leaves under a default umask. An existing target's mode is copied across, but a fresh temp file is0600and 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, andapprovals.tomlhas 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
0600a created file gets.Also driven through the real CLI rather than only tests.
wt config updateagainst a0644config.tomlsymlinked into a dotfiles directory rewrote the real file and left the link, the mode, and the directory as they were.wt config shell installinto a fresh$HOMEproduced the0600rc file, wrapper, and completion above.