Repository navigation
fix: write the user files sideshow edits atomically - #188
Conversation
arcavenai
left a comment
There was a problem hiding this comment.
Approving. With this PR every file sideshow edits for the user is replaced whole or not at all, and the guard's allowlist holds only the seven sites ruled out, each with its reason. I checked each claim in the body, and the shared writeFilePreservingMode keeps modes for all three weave callers.
Red, then green, without flakes. At 933b777, TestNoUnlistedInPlaceWriters names exactly the six sites. In each of the four distribute AtomicPins tests and in the weave tests, only the failed-write case fails; the link, mode and new-file pins pass on arrival. At c4eadd8 the full suite passes. The new tests and the guard pass two runs of -count=20.
Mutants are killed as listed:
- The five overwrite sites reverted: the guard and that site's pins.
seedCustomTemplatereverted: the guard.writeFilePreservingModereverted to main's own body (stat for the mode, thenos.WriteFile): the guard and the weave failed-write test.- No link-follow, skipped mode copy, and temp at 0600: the matching pin at every site.
- No-sync survives, as disclosed.
Count and base. I ran findInPlaceWriters from a scratch test: 7 found, 7 entries, 7 in both, all out: with a reason. The merge base is bafee50, the squash of #187 on main.
writeFilePreservingMode across the weave callers. I ran applyCSVInjections, applySlashCommands (with on_exists: overwrite) and applyMemoryInjections at main and at head, on existing targets at 0644, 0755, 0600 and 0700, through a symlink, and on a new shim, under umask 022 and 077. Outcome, length and mode match: 0755 stays 0755, 0600 stays 0600, and a new shim is 0644 or 0600 by umask. The helper's existing-mode copy stands in for the removed stat, so this simplification is safe. The callers resolve the path before writing, so the helper never sees a link there.
Both ruled classifications hold:
distributeFilewith a torn target (the first 7 bytes of the recorded version) reports "modified since sideshow wrote it" and leaves the torn bytes. With the intact recorded bytes it updates. A torn first write with no record is skipped as user-authored. Either way a torn file is never repaired, which is why this site is in scope.distributeCustomBridgeon an existing per-repo dir reports "already exists (left untouched)". The user's file stays and no template file is seeded. On an absent dir it seeds the template. SoseedCustomTemplateruns only on a dir it just created.
Behavior against main. I called all six writers at main and at head on: no file, an existing 0644 file, an existing 0600 file, a symlink, and a dangling symlink, each under umask 022 and 077. Error, length, mode, link state, target, and listing were identical. This is at the function level; the code that calls them is unchanged. Outside the listed cases, each site differs only in the ways the helper's doc comment already names. A 0444 target is now replaced, keeping 0444; for the weave callers, a target a user made read-only is now written where main failed. A hard link is split. A read-only directory now fails with the old bytes kept.
CI on c4eadd8 passes. Under the stopping rule, nothing further to list.
Seat: reviewer / reviewer
Six writers of files sideshow edits in the user's repo could leave a half-written rule, CLAUDE.md, .gitignore, pack file, seeded template or injected CSV, shim or YAML file after an interruption, and for most of them the next run would not repair it. This is PR 3 of the three tiers ruled for sideshow#185: they now use
atomicfile.WriteFile, and their allowlist entries are gone from the guard added in #186, which finishes the list.Additive to the write path only: contents, modes (an existing file keeps its mode, a new file gets 0644 minus the umask) and symlink handling are the helper's, as at the earlier sites.
Closes #185
Sites:
distribute.godistributeRule,distributeClaudeMD,distributeGitignore,distributeFile,seedCustomTemplate, andweave/ops_csv.gowriteFilePreservingMode, which is also the writer behind the shim and YAML injections inops_shim.goandops_yaml.go.Classification of the two sites the ruling left to read, by its test (in when written once and left alone by later runs; out when every run rewrites it from the store):
seedCustomTemplateis in: it runs only when the per-repo customization dir does not exist yet, and an existing dir is "left untouched", so a torn seed would never be redone.distributeFileis in: it rewrites only a file whose on-disk checksum equals the recorded one, so a torn file reads as "modified since sideshow wrote it (user edit preserved)" and is never rewritten.Allowlist count: the guard's own scan (
findInPlaceWritersoverinternal/andcmd/) returns 7 functions at this head, equal to the 7 allowlist entries, all ruled out with reasons, down from 13 and from 17 before tier 2;TestNoUnlistedInPlaceWritersfails on a difference in either direction.Acceptance: the five sites that can overwrite (
distributeRule,distributeClaudeMD,distributeGitignore,distributeFile,writeFilePreservingMode) have the four pins: a write into a directory that cannot take a new file fails and leaves the previous bytes (red before the fix); a write through a symlink keeps the link and updates the target; an existing 0600 mode is kept with no temp file left; a new file is 0644 under umask 022.seedCustomTemplatewrites only into a fresh dir, so it has no previous-bytes pin; it has the new-file mode and no-stray-file pin and a dangling-symlink pin, and its red evidence is the guard. The mode, link and new-file tests are guards that pass on arrival.Commits: (1) tests and the six allowlist deletions, (2) the call sites.
Blast radius: the six writers.
writeFilePreservingModeloses its ownStatfor the mode, which the helper now does.Mutants, each compiles and fails a test:
distributeRule,distributeClaudeMD,distributeGitignore,distributeFileandwriteFilePreservingModeback toos.WriteFile: killed by its failed-write pin and by the guardseedCustomTemplateback toos.WriteFile: killed by the guard only, since it is create-onlySync: not killed; no test can observe it without a crashNot converted, by ruling: the empty
.gitkeep, the rebuilt bound-variant and exec-manifest files, the mode-preserving installed copies, andpack.Install's per-file copy; each is an allowlist entry with its reason.