fix: preserve symlinks during atomic file writes (#16743)#16980
Merged
Conversation
os.replace(tmp, path) replaces the symlink itself with a regular file, breaking users who symlink config.yaml, SOUL.md, or .env from ~/.hermes/ to a dotfiles repo or managed profile package. Fix: resolve symlinks via os.path.realpath() before os.replace(), so the real file is overwritten in-place while the symlink survives. Fixed in 7 files covering all os.replace call sites: - utils.py (atomic_json_write, atomic_yaml_write — fixes save_config) - hermes_cli/config.py (env sanitizer, save_env_value, remove_env_value) - tools/skill_manager_tool.py (_atomic_write_text — SOUL.md writes) - tools/memory_tool.py (memory file writes) - tools/skills_sync.py (manifest writes) - cron/jobs.py (job state + output file writes) - agent/shell_hooks.py (hook file writes) Fixes #16743
Extract the islink/realpath guard from the 16743 fix into a single atomic_replace() helper in utils.py, then migrate every os.replace() call site in the codebase to use it. The original PR #16777 correctly identified and fixed the bug, but only patched 9 of ~24 call sites. The same bug class (managed deployments that symlink state files silently losing the link on every write) still existed at auth.json, sessions file, gateway config, env_loader, webhook subscriptions, debug store, model catalog, pairing, google OAuth, nous rate guard, and more. Rather than add another 10+ copies of the same three-line guard, consolidate into atomic_replace(tmp, target) which: - resolves symlinks via os.path.realpath before os.replace - returns the resolved real path so callers can re-apply permissions - is a drop-in replacement for os.replace at the use sites Changes: - utils.py: new atomic_replace() helper + atomic_json_write / atomic_yaml_write now call it instead of inlining the guard - 16 files: all os.replace() call sites migrated to atomic_replace() - agent/{google_oauth, nous_rate_guard, shell_hooks}.py - cron/jobs.py - gateway/{pairing, session, platforms/telegram}.py - hermes_cli/{auth, config, debug, env_loader, model_catalog, webhook}.py - tools/{memory_tool, skill_manager_tool, skills_sync}.py Tests: tests/test_atomic_replace_symlinks.py pins the invariant for atomic_replace + atomic_json_write + atomic_yaml_write, covers plain files, first-time creates, broken symlinks, and permission preservation. Refs #16743 Builds on #16777 by @vominh1919.
096a3da to
4ad210c
Compare
1 task
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
3 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.
Atomic writes no longer detach symlinks from their tracked targets. Managed deployments that symlink
~/.hermes/config.yaml,SOUL.md,auth.json,.env, sessions, cron state, etc. to a git-tracked profile package or dotfiles repo now stay linked through every write path.Builds on #16777 by @vominh1919.
Changes
atomic_replace(tmp, target)helper that resolves symlinks throughos.path.realpathbeforeos.replace.atomic_json_write/atomic_yaml_writecall it instead of inlining the guard.os.replace()call site in the codebase migrated toatomic_replace(). fix: preserve symlinks during atomic file writes (#16743) #16777 fixed 9 sites; this PR widens the same fix to the 10+ sibling sites the original missed:google_oauth.py,nous_rate_guard.py,shell_hooks.pyjobs.pypairing.py,session.py,platforms/telegram.pyauth.py,config.py,debug.py,env_loader.py,model_catalog.py,webhook.pymemory_tool.py,skill_manager_tool.py,skills_sync.pyZero bare
os.replace()calls remain in the codebase outside the helper itself.Root cause
os.replace(tmp, target)atomically swapstmpinto place attarget. Whentargetis a symlink, the symlink itself is replaced with a regular file, detaching the user's source-of-truth silently. The helper resolves throughrealpathfirst so the real file is overwritten in-place while the symlink survives.Validation
save_configsave_env_valuetests/test_atomic_replace_symlinks.py: 8 new tests covering the helper,atomic_json_write,atomic_yaml_write, permission preservation, and the broken-symlink edge case — all pass.save_config+save_env_valueagainst a symlinked HERMES_HOME → symlinks survive, tracked source files updated in place.Fixes #16743
Supersedes #16777 (vominh1919's commit cherry-picked, authorship preserved).