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: 8 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: f037db5a-a3b5-4d61-948e-7c544eec0d08

📥 Commits

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

📒 Files selected for processing (2)

• 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.