fix: write the four remaining settings writers atomically, with an in-place writer guard

[arcaven]

Four more writers of the user's Claude Code settings files still replaced the file in place, so an interruption part-way could leave a half-written file that every settings reader refuses to parse. This is PR 1 of the three tiers ruled for sideshow#185: those four now use atomicfile.WriteFile, and a guard test makes the next in-place writer fail the build instead of waiting for a reviewer to find it.

The guard parses every non-test file under internal/ and cmd/, resolves each file's own import names for os and io/ioutil (so a renamed or dot import is followed), and fails on any reference, called or not, to WriteFile, Create or Truncate there, on any os.OpenFile whose flag is not os.O_RDONLY or 0, and on package-level initializers as well as function bodies, unless the function is on an allowlist. Each entry carries a one-line reason. The list starts as everything not yet converted, plus the ruled-out sites. It also fails on a stale entry, so each later tier PR must delete its own entries and the list can only shrink.

What it does not see, also in its doc comment: a write through another package (syscall, x/sys, a vendored writer), a shell-out to cp or tee, a write through an *os.File or *os.Root opened elsewhere (the open is what it flags; a *os.Root received as a parameter is not), and the wiring of the three report blocks in TestNoUnlistedInPlaceWriters, which no test checks (checkAllowlist, which computes what they report, is tested on its own), and a writer reached by reflection or a plugin. It flags os.OpenRoot and os.OpenInRoot as references, since an *os.Root has its own WriteFile, Create and OpenFile. Two read-only forms are flagged although harmless and appear nowhere in the tree: a flag held in a const ro = os.O_RDONLY, and a bare O_RDONLY under a dot import. os.CreateTemp and os.Rename are not in-place writes and are not flagged.

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.

Refs: sideshow#185

Sites: enable/activate.go writeSettingsJSON, foreign/suppress.go writeSettingsObject, adopt/adopt.go writeAgentKey, permissions/permissions.go ClaudeSettings.Save.

Acceptance: at each site, 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. The last three are guards that pass on arrival. The guard test was red before the fix, naming exactly the four sites. After review round 1 the detector self-test covers the nine forms the reviewer probed, plus a read-only control and a local function named WriteFile; those cases were added with the new detector, and the reviewer's probe of the old detector is their red evidence.

Commits: (1) tests and guard, (2) the four call sites.

Blast radius: the four writers; the guard reads source and writes nothing.

Mutants, each compiles and fails a test:

• each of the four sites back to os.WriteFile: killed by its failed-write test and by the guard
• helper does not follow links: killed by the four link tests
• helper skips the mode copy for an existing file: killed by the four mode tests
• helper creates the temp at 0600: killed by the four new-file tests
• guard stops following dot imports, renamed imports, ioutil, package-level initializers os.Truncate, os.OpenRoot or os.OpenInRoot, treats any literal or any selector flag as read-only, or starts scanning test files: killed by the detector self-test, which has one case per form
• guard drops the unlisted, stale or empty-reason check: killed by TestCheckAllowlist_FiresOnEachProblem
• skipping the helper's Sync: not killed; no test can observe it without a crash

Remaining for #185: tier 2 (state files) and tier 3 (user files); the allowlist above names them.

Query at the head: the guard's own scan, findInPlaceWriters over internal/ and cmd/, returns 17 functions, the same 17 as the allowlist (4 pending tier 2, 6 pending tier 3, 7 out with reasons); the four converted sites no longer appear. TestNoUnlistedInPlaceWriters fails on any difference in either direction.

Each documented false positive (a const-held O_RDONLY flag, a bare O_RDONLY under a dot import) is flagged, and an allowlist entry with a reason satisfies it; TestFindInPlaceWriters_FalsePositivesCanBeAllowlisted shows both. The guard's limits sit under a "Known limits" heading in its comment.

[arcavenai]

Requesting changes on one point: the guard misses common ways to write the same in-place call, and the body does not name them as limits. A guard that reads as "the next in-place writer fails the build" is worth having only if a reader knows which writers it cannot see. The four conversions themselves check out.

Blocking: the guard's blind spots are silent. findInPlaceWriters matches a call whose function is the selector os.WriteFile, os.Create, or os.OpenFile with an O_TRUNC selector inside the flag argument, spelled with the identifier os, inside a FuncDecl body. I ran it over a temp tree with one function per form. It flagged only the plain os.WriteFile control and missed all of these:

• alias: w := os.WriteFile; w(...)
• method value: run(os.WriteFile)
• ioutil.WriteFile
• O_TRUNC through a variable: fl := os.O_WRONLY|os.O_CREATE|os.O_TRUNC; os.OpenFile(p, fl, 0o644)
• dot import: import . "os", then WriteFile(...)
• renamed import: import osx "os", then osx.WriteFile(...)
• package-level func literal: var F = func() { os.WriteFile(...) }
• os.OpenFile(p, os.O_WRONLY|os.O_CREATE, ...) with no O_TRUNC, then a write (overwrites in place without truncating)
• os.Truncate

None of these forms exists in non-test code at this head; my grep found none. So nothing slips through today; this is about the claim. Could the guard either catch the first five (resolve the import name for os and io/ioutil, flag a non-call reference to the writers, flag any OpenFile whose flag is not a constant expression, and walk package-level var initializers), or say in its doc comment and in the PR body which forms it does not see? Either resolves this.

Verified, no change needed:

• Red at 50c9f90: the guard names exactly the four sites, and each site's failed-write test fails; every other package passes. Green at d4c382e.
• Flakiness: the new tests and the guard, -count=20, twice, all pass.
• Mutants from the body: each site reverted to os.WriteFile is killed by its failed-write test and the guard; no link-follow (12 failures), skipped mode copy (7), temp at 0600 (5), guard dropping Create, dropping O_TRUNC, and scanning tests are each killed. No-sync survives, as the body says. A no-op guard (empty result) is killed by both the stale check and the detector self-test.
• Allowlist: my own grep of non-test internal/ and cmd/ for the three forms finds the same 15 functions as the list, plus the four converted sites at main. The out-reasons match the code: .gitkeep content is empty, the bound-variant and exec-manifest files are re-rendered from the store on every enable, and the source-mode copies are rewritten from their source on every sync or materialize. foreign/enables.go:140 opens with O_WRONLY only, writes nothing, and closes at once; it is a writability check, not a writer.
• Behavior against main: I called each of the four writers at main and at head over no file, an existing 0644 file, an existing 0600 file, a symlink, and a dangling symlink, under umask 022 and 077. Error, content length, mode, link state, target, and directory listing are identical.
• CI on d4c382e: all checks pass.

Not blocking, for the record:

• Deleting the stale-entry check, or the empty-reason check, from TestNoUnlistedInPlaceWriters survives; nothing tests those two checks. The "list can only shrink" promise rests on the stale check, so a small self-test with a fake allowlist entry would hold it.
• Two differences from main outside the listed cases. They come from the helper, so they are not new with this PR. An existing settings file at 0444 in a writable directory was refused on main and is now replaced, keeping 0444. A settings file that is a hard link is now split: the settings path gets the new bytes and the other name keeps the old. A sentence in the helper's doc comment would cover both.

Seat: reviewer / reviewer

[arcavenai]

Requesting changes on one remaining gap. The guard now catches every form from round 1 and every new evasion I tried except one: an os.Root writer. The doc comment does not list it, and it is a stdlib write path in the Go version this module targets. Closing it, or naming it, keeps the guard's stated limits true. Everything else checks out.

Blocking: os.Root writes pass and are not in the documented limits. go.mod says go 1.25.4, and os.Root has WriteFile, Create and OpenFile in 1.25. I ran findInPlaceWriters over one function per form at 5469a33, and all three of these were missed:

• r, _ := os.OpenRoot("."); _ = r.WriteFile("x", nil, 0o644)
• r, _ := os.OpenRoot("."); _, _ = r.Create("x")
• r, _ := os.OpenRoot("."); _, _ = r.OpenFile("x", os.O_WRONLY|os.O_TRUNC, 0)

The doc comment's limits name other packages, shell-outs, an *os.File opened elsewhere, and reflection or plugins. A *os.Root is none of those. Could the detector flag OpenRoot the way it flags OpenFile (as a reference, so a read-only root needs an entry with a reason), or could the comment name os.Root as a limit? Either resolves this. No os.Root use exists in non-test code today.

Verified, no change needed:

• All nine round-1 forms are flagged: alias, method value, ioutil.WriteFile, flag through a variable, dot import, renamed import, package-level func literal, O_WRONLY|O_CREATE without O_TRUNC, and os.Truncate.

• The new evasions are flagged:

  • a writer held in a struct field, in a local map, or in a package-level map;
  • a write(fn) helper given os.WriteFile;
  • a flag computed by a function call;
  • an open through a type F = *os.File alias;
  • a method value on a renamed import;
  • a parenthesized (os.WriteFile)(...);
  • a dot-imported io/ioutil;
  • O_RDONLY|O_TRUNC;
  • a method, and init.

  A Truncate or WriteAt on an *os.File passed in is missed, which the comment documents.

• Read-only patterns: os.Open, os.ReadFile, os.Stat, OpenFile with os.O_RDONLY, and the same through a renamed import are not flagged. Two read-only forms are flagged (a const ro = os.O_RDONLY flag, and a bare O_RDONLY under a dot import). Neither appears in the repo, so no entry is forced today.

• Allowlist: my own grep of non-test internal/ and cmd/ for os/ioutil WriteFile, Create, OpenFile and Truncate finds 17 functions, the same 17 as the list. The two new reasons match the code: CanWriteSettings opens O_WRONLY and closes without writing, and createTempBeside opens O_RDWR|O_CREATE|O_EXCL, a new file only.

• Mutants, each killed:

  • the builder's list: dot, renamed, ioutil, package var, Truncate, literal and selector flag read as read-only, and scanning tests;
  • the round-1 survivors, deleting the stale or the empty-reason append;
  • my additions: deleting the unlisted append, dropping Create, dropping OpenFile, read-only always true, no read-only skip, flagging calls only, and the var key without its name;
  • the four sites back to os.WriteFile, no link-follow, skipped mode copy, and temp at 0600.

• The production code at the four sites is unchanged since d4c382e (empty diff). The full suite passes. The guard tests pass with -count=20. CI on 5469a33 passes.

Not blocking: turning off the three t.Errorf blocks in TestNoUnlistedInPlaceWriters survives, since nothing is unlisted or stale at head and TestCheckAllowlist_FiresOnEachProblem tests the function, not that wiring. The new doc paragraph on read-only files and hard links matches what I measured in round 1.

Seat: reviewer / reviewer

[arcavenai]

Approving. The four settings writers are atomic, and the guard now sees every write form I could construct through os or io/ioutil, os.Root included. Its doc comment states what it cannot see, so a reader can trust both the pass and the limit.

Round-2 gap, closed. I re-ran the probe at fbbcafb with one function per form. These are flagged:

• os.OpenRoot then r.WriteFile, r.Create, or r.OpenFile with O_WRONLY|O_TRUNC;
• the same through a renamed import (osx.OpenRoot), a dot import, an alias (o := os.OpenRoot), and a package-level var r, _ = os.OpenRoot(".");
• os.OpenInRoot.

A *os.Root passed in as a parameter and written through is not flagged. That is the same shape as the documented *os.File limit, since the open is what gets flagged.

No regressions. In the same run:

• all nine round-1 forms are flagged;
• the round-2 evasions are flagged: struct field, local map, package-level map, a write(fn) helper, a flag from a function call, the *os.File type alias, a method value on a renamed import, a parenthesized call, dot io/ioutil, O_RDONLY|O_TRUNC, a method, and init;
• these stay unflagged: os.Open, os.ReadFile, os.Stat, and OpenFile with O_RDONLY, plain and renamed.

Also verified:

• The two read-only false positives are named in the doc comment and in the body.
• Mutants dropping OpenRoot or OpenInRoot from writerNames are killed by the detector self-test. Dot, renamed, stale and empty-reason are still killed.
• The production code and the allowlist are unchanged from 5469a33. The only change is guard_test.go.
• The full suite passes, and CI on fbbcafb passes.

Optional, carried from round 2: turning off the three t.Errorf report blocks in TestNoUnlistedInPlaceWriters still goes unnoticed, since nothing is unlisted or stale at head. Fine to leave.

Seat: reviewer / reviewer

[arcavenai]

Requesting changes on two lines of doc comment. The new "Known limits" list is what a future maintainer reads to decide whether a green guard means a write is safe. Two of the gaps I measured are not in it.

Blocking: two measured gaps are missing from "Known limits."

1. A *os.Root passed in. func RootParam(r *os.Root) { _ = r.WriteFile("x", nil, 0) } is not flagged (round-3 probe, and the detector code is unchanged since). The list names "a write through an *os.File opened elsewhere (the open is what it flags)", but a *os.Root is a different type with its own WriteFile, Create and OpenFile. Could that bullet say "an *os.File or *os.Root opened elsewhere"?
2. The report wiring in TestNoUnlistedInPlaceWriters. Turning off any of its three if len(...) > 0 { t.Errorf(...) } blocks still passes the suite, because nothing is unlisted or stale at head, and checkAllowlist is tested on its own. A line saying so, or a test that runs the same reporting on a fixture, closes it.

f.Truncate and f.WriteAt on a passed-in *os.File are covered by the existing *os.File bullet, so nothing is needed there.

Verified at 44d615e:

• Count: I ran findInPlaceWriters over internal/ and cmd/ from a scratch test. It returns 17, equal to the allowlist's 17: 4 pending tier 2, 6 pending tier 3, and 7 out. This matches the body.

• TestFindInPlaceWriters_FalsePositivesCanBeAllowlisted goes through the real checkAllowlist. Each mutant is killed by this test, among others:

  • ignoring an allowlist entry for unlisted;
  • always reporting entries as stale;
  • a detector that returns nothing;
  • treating a const flag as read-only.

  Accepting a blank reason is killed by TestCheckAllowlist_FiresOnEachProblem.

• No regression: the detector code (scanFile, writerRef, readOnlyFlag, checkAllowlist, writerNames) is byte-identical to fbbcafb, and this commit changes only the comment and adds a test. Production code and the allowlist are unchanged.

• The full suite passes locally, and CI on 44d615e passes.

Seat: reviewer / reviewer

[arcavenai]

Approving. Both gaps from round 4 are now in the guard's "Known limits" and in the body, so a reader can see what a green guard does and does not cover.

• The diff from 44d615e is comments only. Every added or removed line in guard_test.go is a // comment, so the detector, the tests, the production code and the allowlist are unchanged.
• The *os.Root bullet matches the round-3 probe: os.OpenRoot and os.OpenInRoot are flagged, and a *os.Root received as a parameter and written through is not.
• The report-wiring note matches the round-2 and round-4 mutants: turning off any of the three t.Errorf blocks in TestNoUnlistedInPlaceWriters survives, and checkAllowlist is killed by its own test.
• The body carries both items.
• CI on 4f8f51a passes.

My round-3 and round-4 measurements still hold here, since no code changed: the count is 17 against an allowlist of 17, the false-positive allowlist path is exercised, and the mutants are killed.

Seat: reviewer / reviewer