refactor: promote atomic_write_text to a shared util; migrate vanilla tmp+replace sites (BE-4353)

[mattmillerai]

ELI-5

Several places in the CLI write a file "safely" the same way: write to a temporary file, then rename it over the real file so a Ctrl-C mid-write can never leave a half-written or empty file. That same ~10-line snippet was copy-pasted into five (really six) different modules, and one of the copies had a small bug — it leaked the temp file if something failed between writing and renaming. This PR pulls that pattern into one shared helper, comfy_cli.file_utils.atomic_write_text(...), and points the vanilla copies at it (fixing the leak in the process). The one specially-hardened copy that protects secrets is left alone.

What changed

New shared helper in comfy_cli/file_utils.py:

atomic_write_text(path, content, *, fsync=False)

tmp-in-same-dir + os.replace, cleans up the tmp file and re-raises on any failure, creates parent dirs, and optionally fsyncs the tmp contents before the rename (best-effort, matching prior per-site behavior).

Migrated call sites:

Site	Notes
skills.write_manifest	Fixes an existing tmp-leak bug — the old inline version had no failure cleanup, so an exception between write_text and os.replace leaked the tmp file.
skills._atomic_write_text	Removed; all four skill writers (_write_claude_skill, _write_cursor_rule, _upsert_agents_md_block ×2) now call the shared helper.
cql/loader.write_object_info_cache	Keeps its best-effort try/except OSError wrapper around the call; its own tmp-cleanup is now handled by the helper.
jobs_state.write	Uses fsync=True, keeping its locking.file_lock wrapper. Drops the now-redundant secrets.token_hex tmp suffix (writes are already serialized per-path by the lock).
command/workflow._atomic_write_text	Removed; a sixth identical copy the ticket's finder did not list — migrated for completeness (see judgment call below).

Deliberately untouched: auth/store._write_all — a security-hardened superset (O_CREAT|O_EXCL with 0o600 at open time, 0o700 parent dir, fsync) protecting secret material. Folding it into a generic helper would dilute those permission guarantees for zero benefit; the ticket explicitly excludes it.

tmp-name normalization (the riskiest change)

write_manifest and jobs_state previously replaced the suffix (manifest.json → manifest.<pid>.tmp); the shared helper appends (manifest.json.<pid>.tmp). This is harmless — tmp names are ephemeral and renamed away immediately — and importantly both old and new names end in .tmp, so neither matches the *.json glob that command/jobs.py uses to list job state files. Verified: no job-listing glob or watcher can observe the tmp under either scheme.

Tests

• New unit tests for atomic_write_text in tests/comfy_cli/test_file_utils.py: creates file + parents, overwrites existing, fsync=True path, and tmp-cleanup + destination-untouched on a failed rename.
• Full suite green: 2773 passed, 37 skipped. ruff check / ruff format --check clean on all changed files. (The 15 pre-existing UP038 ruff findings in untouched files are on main already, unrelated to this change.)

Judgment call

The ticket enumerated five duplicate sites and migrated three (+ optional jobs_state). While auditing I found a sixth identical _atomic_write_text in command/workflow.py that the finder overlooked (not a deliberate exclusion like auth/store). Since the ticket's whole purpose is to consolidate these vanilla duplicates, I migrated it too. It is semantically identical; the only difference is the shared helper adds a parent.mkdir(parents=True, exist_ok=True), which is a harmless no-op at both of its call sites (both already write into pre-existing directories). Flagging it here since it's beyond the ticket's literal list.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 16 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d30872cb-fa11-4c16-ad50-37c4cb0c3c39

📥 Commits

Reviewing files that changed from the base of the PR and between 92e70fe and 35bcfc3.

📒 Files selected for processing (4)

• comfy_cli/command/workflow.py
• comfy_cli/cql/loader.py
• comfy_cli/file_utils.py
• tests/comfy_cli/test_file_utils.py

📝 Walkthrough

Walkthrough

Changes

The PR adds shared atomic text-file persistence with optional durability syncing and failure cleanup. Workflow, cache, job-state, manifest, skill, and rule writers now use it, while tests cover writing, overwriting, cleanup, uniqueness, fsync, and symlink protection.

Atomic File Persistence

Layer / File(s)	Summary
Shared atomic write utility and validation comfy_cli/file_utils.py, tests/comfy_cli/test_file_utils.py	Adds atomic_write_text with temporary-file replacement, optional fsync, cleanup, and tests for its persistence and failure behavior.
Persistence call-site migration comfy_cli/command/workflow.py, comfy_cli/cql/loader.py, comfy_cli/jobs_state.py, comfy_cli/skills/__init__.py	Replaces duplicated local atomic-write implementations with the shared utility across workflow, cache, job-state, manifest, and skill-related writers.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-4353-atomic-json-write-util

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-4353-atomic-json-write-util

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 5 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	2
🟢 Low	1
⚪ Nit	1

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@comfy_cli/file_utils.py`:
- Around line 39-52: Update the atomic-write flow around tempfile.mkstemp and
os.replace to set the temporary file’s mode before renaming: reuse the existing
destination’s permission bits when the destination exists, otherwise apply the
platform’s umask-derived default. Ensure this occurs before os.replace so the
final file preserves expected shared read access.

In `@tests/comfy_cli/test_file_utils.py`:
- Around line 107-112: Strengthen test_atomic_write_text_fsync_true_still_writes
by spying on the relevant os.fsync/os.fdopen operations, following the existing
mkstemp spy pattern in test_atomic_write_text_tmp_name_is_unique_per_write.
Assert fsync is invoked for both the temporary file descriptor and the parent
directory while preserving the existing content and temporary-file cleanup
assertions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b6dcbc15-2421-453c-9cbb-8465a49c9258

📥 Commits

Reviewing files that changed from the base of the PR and between 85b62da and 92e70fe.

📒 Files selected for processing (6)

• comfy_cli/command/workflow.py
• comfy_cli/cql/loader.py
• comfy_cli/file_utils.py
• comfy_cli/jobs_state.py
• comfy_cli/skills/__init__.py
• tests/comfy_cli/test_file_utils.py

[mattmillerai]

Fixed in e8ae317: the mode-preservation test now sets 0o700 (owner-execute — a bit mkstemp's hardcoded 0600 lacks, with no group/other bits) instead of 0o640. Group-read still trips py/overly-permissive-file, and the test only needs a non-0600 mode to prove restoration happens, so this keeps the exact regression guard while clearing the CodeQL alert.

[bigcat88]

Needs a rebase — my fault for the timing, and it's a two-line resolution. I verified the substance while I was here so this is worth doing promptly.

The conflict: comfy_cli/command/workflow.py, import block only. I merged #623 a few minutes ago, which added

from comfy_cli.http import ResponseTooLarge as _ResponseTooLarge

right where this branch adds

from comfy_cli.file_utils import atomic_write_text

Keep both — they're unrelated additions. Nothing else conflicts; file_utils.py itself auto-merged. (GitHub still shows this MERGEABLE because it hasn't recomputed against the new main yet; a local test-merge is what surfaced it.) ruff check / ruff format --diff are clean on the branch as-is under the CI-pinned 0.15.15.

Both of your riskier claims check out, verified against main:

1. The tmp-leak is real, and the helper fixes it. main's write_manifest goes tmp.write_text(...) → os.replace(tmp, path) with no try/cleanup. Forcing a failure between the two:

   main's pattern:        tmp still on disk?  True -> manifest.15812.tmp
                          destination intact? {'old': True}
   
   atomic_write_text:     tmp leftovers: NONE  <-- cleaned up
                          destination intact? {'old': True}   (and the error is re-raised)
   

   Both keep the destination intact; only the shared helper cleans up after itself.

2. The tmp-name normalization is safe. command/jobs.py globs *.json at lines 194 and 876, and neither naming scheme is visible to it:

   abc.json              matches *.json ? True
   abc.json.1234.tmp     matches *.json ? False    (new, appended)
   abc.1234.tmp          matches *.json ? False    (old, with_suffix replaced)
   

   So no job listing or watcher can observe the tmp under either scheme, as you said.

Leaving auth/store._write_all alone is right — O_CREAT|O_EXCL with 0o600 at open time plus a 0o700 parent is a superset whose guarantees a generic helper would dilute. And migrating the sixth copy in command/workflow.py is the correct call given the ticket's whole purpose; the added parent.mkdir(parents=True, exist_ok=True) is a genuine no-op at both of its call sites.

Ping me once it's rebased and I'll finish the review and merge.

[bigcat88]

Approving. The mode-restoration logic is the part of this that could quietly go wrong, so I checked it against main on real files rather than only through the unit tests, and then tortured the atomicity guarantee directly.

Mode parity with main — the subtle part

mkstemp hardcodes 0600 and os.replace carries it onto the destination, so without the restore this would have silently tightened every migrated writer on its first write. Ran each real writer end-to-end in an isolated HOME (umask 022), on both branches:

writer	main	this branch
jobs_state.write → <id>.json	0644	0644
skills.write_manifest	0644	0644
cql.loader.write_object_info_cache	0644	0644
jobs state directory	0700	0700

Plus, directly: a fresh file lands at 0644 under umask 022, and a pre-existing 0640 destination stays 0640 across the write. No stray .tmp in any of those directories afterwards.

Atomicity, exercised rather than asserted

Six concurrent processes writing a 400 KB payload to one destination in a loop, with a reader hammering read_text() + json.loads() throughout: 21,489 clean reads, 0 torn reads, no tmp leftovers.

Then a real mid-write SIGKILL — with the child printing a readiness marker after its imports and payload construction, so the kill lands inside the write rather than during interpreter startup (my first attempt at this was vacuous for exactly that reason): 20/20 kills left a parseable destination. 18 of them had already flipped to the new content, and 13 stranded a tmp file — which is how I know the kills really were landing mid-write.

Tests are load-bearing

Mutated the helper three ways; each was caught by exactly the right test and nothing else:

mutation	caught by
drop os.chmod(tmp_name, dest_mode)	preserves_existing_mode + new_file_uses_umask_default (2 failed)
drop the tmp cleanup in the except	cleans_up_tmp_on_failure
make both fsync calls no-ops	fsync_true_still_writes (the spy counts the fds — good call spying rather than trusting the flag)

Claims I checked independently

• auth/store._write_all really is untouched — git diff origin/main on that file is empty, and it still opens O_CREAT|O_EXCL at 0600 into a 0700 parent with a 0600 chmod after. Same for download_state.py. The exclusion is honoured, not just described.
• The tmp-name change can't be observed by the job listing. Both globs in command/jobs.py (lines 245 and 982) are *.json, and the new name is <id>.json.<rand>.tmp — ends in .tmp, so no match. The .lock sibling is excluded for the same reason. Your "verified: no job-listing glob can observe the tmp" holds under the new scheme.
• The sixth copy in command/workflow.py is semantically identical, and the added parent.mkdir is a no-op at both call sites (set_slot_cmd writes next to an existing file; vary_cmd already mkdirs out itself).

Two nits, neither blocking

1. os.umask(0) / os.umask(umask) is process-global and not thread-safe. Any thread that creates a file inside that two-syscall window gets a 0 umask, i.e. a world-writable file. Unreachable today — every writer here runs on the main thread, the watcher is a separate process, and the telemetry workers don't touch the filesystem — so I'd just note it in the comment rather than restructure. It's the kind of thing that becomes true later without anyone noticing.

2. Stranded tmp files are now unbounded rather than bounded by pid reuse. The old <name>.<pid>.tmp meant a later run with the same pid overwrote the corpse; mkstemp gives a fresh name every attempt, so each crash mid-write leaves a distinct file forever. I measured 13 survivors from 20 kills. Nothing prunes the jobs directory (no unlink/retention logic in jobs_state.py or command/jobs.py), and jobs ls globs *.json so they're invisible while they pile up. Cheap fix if you want it: sweep *.tmp older than a day alongside the existing stale-watcher reap in _gather_local_state_files. Worth it in exchange for O_EXCL + no symlink following, which is a real security improvement over Path.write_text on a predictable name — I'd just not leave the litter unmentioned.

Suite

Full pytest . on your branch merged with current main: 3779 passed, 31 skipped. ruff check / ruff format --diff clean at the CI-pinned 0.15.15.

Note for sequencing: #661 stacks on this branch, so it'll retarget to main and probably need a rebase once this squash-merges.