Skip to content

fix(paths): preserve symlinks in atomic_write - #108

Closed
joy13975 wants to merge 4 commits into
aannoo:mainfrom
joy13975:fix/atomic-write-preserve-symlink
Closed

fix(paths): preserve symlinks in atomic_write#108
joy13975 wants to merge 4 commits into
aannoo:mainfrom
joy13975:fix/atomic-write-preserve-symlink

Conversation

@joy13975

@joy13975 joy13975 commented Aug 17, 2026

Copy link
Copy Markdown

The problem

atomic_write_io replaces its target with tempfile + rename. A rename onto a symlink replaces the link with a regular file. So when a user symlinks a config file into a dotfiles repo, an hcom hook install silently detaches it: hcom's edit lands in a brand-new local file, the repo copy stops receiving updates, and the two diverge with no error and no warning.

This is not hypothetical - it is how I hit it. My ~/.claude/settings.json and ~/.codex/config.toml are symlinks into a config repo that is the single source of truth for my machine setup.

Reproduction against 0.7.25:

$ ln -s "$R/repo/settings.json" "$R/home/.claude/settings.json"
$ ls -l "$R/home/.claude/settings.json"
lrwxr-xr-x  ... settings.json -> .../repo/settings.json

$ HCOM_DIR="$R/home/.hcom" hcom hooks add claude
Added Claude hooks  (.../home/.claude/settings.json)

$ ls -l "$R/home/.claude/settings.json"
-rw-------  4905  settings.json          # link is gone
$ grep -c hcom "$R/repo/settings.json"
0                                        # repo copy never got the hooks

The fix: opt-in, not a change to the shared default

My first attempt made the shared primitive follow symlinks for every caller. That was wrong, and I'm glad I checked before you read it: atomic_write is used both by user-owned config writers (which want link preservation) and by hcom's own internal state files - the flag counters in paths.rs, the pidfile in pidtrack.rs, the update flags in update.rs, the relay pidfile in relay/worker.rs, the device id in relay/mod.rs. Those live under ~/.hcom and gain nothing from following a link; making them follow one only hands them a redirection surface they didn't have before.

So the split is explicit:

  • atomic_write / atomic_write_io - unchanged behavior: rename onto the literal path. Every internal state writer keeps its pre-PR redirection immunity by construction, with no edits at those call sites, and any future internal writer reaching for the default gets the safe behavior for free.
  • atomic_write_following_symlinks / _io - new, named opt-in that resolves the link chain first. Wired only to the writers that legitimately need dotfiles-link preservation: the config.rs env/config.toml wrappers, the hooks/*.rs modules, and the cursor/copilot preprocessing writers.

Both funnel through one write_atomically so the pieces below apply to every atomic write, not just the symlink-following path.

Details worth noting in resolve_write_target:

  • Relative link destinations resolve against the link's own directory; multi-hop chains mixing absolute and relative hops are covered by a test.
  • A dangling link resolves to its missing target deliberately - that is the "linked into a checkout that has not created the file yet" case, where creating the target is what the user meant.
  • Only ErrorKind::NotFound from symlink_metadata falls through to "write as given". Every other stat error propagates, so an EACCES on the final component can't be misread as "not a symlink" and quietly clobber a link.
  • The chain is bounded at 8 hops, so a cycle returns InvalidInput rather than spinning.
  • Resolution is advisory, not a security boundary. The doc comment names the resolve-then-rename TOCTOU explicitly and records the O_DIRECTORY-fd + relative-persist remedy if a stronger guarantee is ever wanted. Under hcom's per-user threat model, anyone able to plant a symlink in the config directory can already write the config file directly.

preserve_target_mode fixes a second, separate bug in the same primitive: an in-place atomic write over an existing regular file used to reset it to the tempfile's 0600. Writing through a link into a repo turned a 0644 tracked file into 0600 - which happened to my own config repo before this was fixed. Now an existing regular destination keeps its mode, while a brand-new file still lands private at 0600.

Testing

24 tests in the paths module pass, cargo clippy --all-targets -- -D warnings is clean.

The behavioral tests were each proven to bite by reverting the specific hunk in place and observing the exact failure, then restoring:

Test Reverted-state failure
test_atomic_write_io_does_not_follow_symlink link survived and the target was written through
test_resolve_write_target_propagates_non_notfound_stat_error got Ok where Err is required
test_atomic_write_io_preserves_existing_file_mode left: 384 (0o600), right: 420 (0o644)
test_atomic_write_following_symlinks_writes_through_and_keeps_link same mode mismatch

Coverage also includes absolute, relative, multi-hop and dangling links, cycle rejection, plain-file replacement, and that a new file is still created 0600.

End to end against the real binary and trigger, same setup as the reproduction above:

released 0.7.25:  symlink=NO   hcom-hooks-in-repo-file=NO
this branch:      symlink=YES  hcom-hooks-in-repo-file=YES

The unrelated model key in the repo-side file was preserved, and all 13 hook events landed in it. I then ran the real thing: hcom hooks add claude and hcom hooks add codex against my live symlinked config, and both symlinks survived with the hook entries landing in the repo files.

Known gap

Symlink resolution is only exercised on unix. I have no Windows host, and creating a file symlink on Windows needs elevation or Developer Mode, so a #[cfg(windows)] test would be unverifiable decoration rather than coverage. The logic is platform-agnostic std::fs, and the cfg(windows) persist path is untouched. Flagging it rather than pretending it is covered - the Windows CI jobs here exercise the no-follow default, not the following path.

Note on the local suite

cargo test on my machine has 17 failures in hooks::gemini::tests. They reproduce identically on unmodified main at 79ebde1, and the root cause is machine-local: my gemini-cli is 0.19.2 and try_setup_gemini_hooks requires 0.26.0, so it returns VersionUnsupported before reaching anything this branch changes. Unrelated to this PR. Otherwise 2168 pass.

joy13975 and others added 3 commits August 17, 2026 22:19
atomic_write_io replaces its target with tempfile + rename. A rename onto
a symlink replaces the LINK with a regular file, so any config file the
user has symlinked into a dotfiles repo gets silently detached: hcom's
edit lands in a new local file, the repo copy stops receiving updates,
and the two diverge with no error and no warning.

This hits every config writer that goes through atomic_write, which is
all of them - claude settings.json, codex config.toml/hooks.json/rules,
gemini settings + policy, copilot, cursor, kimi, antigravity - so fix it
once in the primitive rather than at ~20 call sites.

resolve_write_target follows the symlink chain before the write, so the
rename lands on the file the user pointed at and the link survives.
Relative link destinations resolve against the link's own directory. A
dangling link resolves to its missing target on purpose: that is the
"linked into a checkout that has not created the file yet" case, where
creating the target is what was meant. The chain is bounded at 8 hops so
a symlink cycle fails loudly instead of spinning.

Following a link the user created honors their intent and does not widen
exposure: anyone able to plant a symlink in the config directory can
already write the config file directly.

Tests cover writing through an absolute link, a relative link, a dangling
link, the cycle rejection, and that a plain file is still replaced in
place. The three symlink tests fail without the fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Make atomic_write[_io] no-follow by default (redirection-immune), so
  internal state files (flag counter, pidfile, update flags, relay
  pid/device-id) can no longer be redirected by a pre-planted symlink.
- Scope symlink-following to an explicit opt-in variant
  (atomic_write_following_symlinks[_io]) used only by user-config/dotfile
  writers (config, all hooks, cursor/copilot preprocessing) that need
  dotfiles-link preservation.
- Regression test pins that the default primitive destroys a planted link
  rather than writing through it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- resolve_write_target: propagate all non-NotFound stat errors instead of collapsing them into the write-as-given fallback (fail-loud; no silent symlink clobber)
- write_atomically: preserve an existing regular target file mode so an atomic write no longer silently resets it to the tempfile 0600
- resolve_write_target: fail-loud expect on the unreachable parent()==None arm; reword partial-resolution comment; document resolve-then-rename TOCTOU as advisory
- tests: multi-hop chain, mode-preservation (follow + no-follow), new-file 0600, non-NotFound stat propagation

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@joy13975
joy13975 force-pushed the fix/atomic-write-preserve-symlink branch from 84bf965 to 8562178 Compare August 17, 2026 16:26
cargo fmt --all -- --check failed in CI on the longer call name.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@joy13975
joy13975 force-pushed the fix/atomic-write-preserve-symlink branch from 8562178 to c8656b6 Compare August 17, 2026 16:34
@joy13975 joy13975 closed this Aug 17, 2026
@joy13975
joy13975 deleted the fix/atomic-write-preserve-symlink branch August 17, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant