fix(skills_hub): use atomic writes for lock.json and taps.json

[vominh1919]

Problem

tools/skills_hub.py has two save() methods that write JSON state files non-atomically using write_text():

1. HubLockFile.save() (line ~2565): writes skills/.hub/lock.json — tracks installed skills, trust levels, and hashes
2. TapsManager.save() (line ~2632): writes taps.json — tracks custom skill source taps

If the process is killed mid-write (OOM, signal, power loss), the file is truncated/corrupted and all installed skill state is lost.

Fix

Replace write_text() with the atomic write pattern used elsewhere in the codebase (e.g. memory_tool.py:441, mcp_oauth.py:161, skill_manager_tool.py:306):

1. tempfile.mkstemp(dir=parent_dir) — create temp file in same directory (same filesystem for atomic rename)
2. os.write(fd, payload.encode()) — write full JSON payload
3. os.replace(tmp, path) — atomic rename on POSIX
4. except BaseException: os.unlink(tmp); raise — cleanup on failure

[teknium1]

Thanks for the focused crash-safety fix. The underlying problem still exists on current main: HubLockFile.save() and TapsManager.save() directly call write_text() at tools/skills_hub.py:3406 and tools/skills_hub.py:3481.

Problems

• The added direct os.replace() bypasses utils.atomic_replace(), whose documented purpose is to preserve symlinks and handle EXDEV/EBUSY fallback (utils.py:91-136). Please use atomic_json_write() instead; it also flushes and fsyncs before publishing (utils.py:139-208).
• The named state files have two additional direct initialization writes in ensure_hub_dirs() at tools/skills_hub.py:3540 and tools/skills_hub.py:3544.
• The PR adds no regression coverage; current manager tests cover ordinary save/load behavior only (tests/tools/test_skills_hub.py:1311-1457).

Suggested changes

• Route both save() methods and the initialization writes through atomic_json_write().
• Add manager-level tests covering failure preservation and symlink-safe atomic publication.

Automated hermes-sweeper review.

[teknium1]

Please use utils.atomic_json_write(self.path, data) rather than calling os.replace directly. The shared helper fsyncs staged data and publishes through atomic_replace, which preserves symlink targets and handles EXDEV/EBUSY fallback (utils.py:91-208).

[GottZ]

This was generated by AI during triage.

Summary

Three PRs address the persistence-corruption risk in lock.json and taps.json: #16440 and #29699 add local tempfile-and-replace implementations, while #32233 routes the same two save methods through the repository’s established atomic_json_write() helper. None of the diffs adds the requested failure-preservation or symlink-safety tests, and none covers the additional initialization writes identified in review; #29699 also does not modify the index-cache path claimed in its body.

Related pull requests

• #16440 related — (+19/-2) — keep open, but revise before merge: the diff makes both save methods atomic against interrupted writes, yet directly calls os.replace() instead of the symlink-, metadata-, and fallback-aware atomic_json_write() path; its contributor review also identifies uncovered initialization writes and missing regression tests.
• #29699 duplicate — (+36/-2) — duplicate and not preferred: the diff covers the same two save methods as #16440 with another local tempfile implementation, but does not change any index-cache writer despite the PR body’s broader claim. Despite the keep_open review on #29699, the complete diff shows no distinct implementation advantage over a corrected #16440 and retains the same direct-os.replace() concern.
• #32233 [closed] duplicate — (+3/-4) — closed duplicate, relevant as the preferred reference implementation: unlike #16440 and #29699, its diff correctly reuses atomic_json_write() for both save methods, but it still omits the additional initialization sites and regression coverage requested in the review of #16440.

Duplicates

#16440, #29699, and #32233 substantially duplicate the same lock.json and taps.json save-method fix; #29699’s claimed index-cache scope is not present in its diff.

Suggested consolidation

Update and merge #16440 by adopting the concise atomic_json_write() approach demonstrated by closed #32233, covering the additional initialization writes, and adding failure-preservation and symlink-safe publication tests. Then close #29699 as a duplicate despite its keep_open review—the diff duplicates #16440’s two save-method changes, uses the same non-preferred direct replacement strategy, and does not implement its advertised cache changes; keep #32233 closed as a duplicate/reference implementation.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup16440 ["PRs duplicating each other"]
        P16440["PR #16440 (open)"]
        P29699["PR #29699 (open)"]
        P32233["PR #32233 (closed)"]
    end
    class P16440 open
    class P29699 open
    class P32233 closed
    class P16440 target
    click P16440 "https://github.com/NousResearch/hermes-agent/pull/16440"
    click P29699 "https://github.com/NousResearch/hermes-agent/pull/29699"
    click P32233 "https://github.com/NousResearch/hermes-agent/pull/32233"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 5 kB of PR diffs, 4 kB of issue/PR text, 3 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.