refactor: finish the atomic-writer consolidation and document the write policy

[mattmillerai]

STACKED — merging lands on matt/be-4353-atomic-json-write-util (owned by @mattmillerai, PR #595), NOT main. Review/merge #595 first; GitHub retargets this PR to main automatically once it lands.

ELI-5

The CLI writes a bunch of small files — a job's status, a lock file listing pushed assets, a couple of caches. Writing a file isn't instant, so if the CLI is killed halfway through you can end up with a half-written file that nothing can read. The fix everyone uses is: write to a scratch file first, then rename it over the real one in a single step that can't be interrupted. Ten places in this codebase had each written that same dance by hand.

#595 built one shared helper and moved five of them onto it. This PR moves the last three, and — more importantly — writes down the rule for which ones are allowed to use the shared helper at all. Two writers deliberately stay hand-written because they can contain passwords or signed URLs, and they need stricter file permissions than the shared helper gives. Now there's a note in the code explaining exactly why, so the next person doing a cleanup pass doesn't "helpfully" consolidate a secrets file into the generic path.

What changed

New helper. atomic_write_bytes(path, data, *, fsync=False) joins atomic_write_text. Both delegate to one private _atomic_write that operates on bytes; the text variant encodes UTF-8 and forwards. No mode= parameter — that was considered and rejected, because a writer whose payload needs 0600 needs more than a mode (an O_EXCL open at that mode, a 0700 parent, an fsync), and that's clearer spelled out at its own call site.

Three migrations.

Writer	Now	Note
command/project.py _write_assets_lock	atomic_write_text(..., fsync=True)	Net gain: tmp-file cleanup on failure, which the inline version lacked. Dropped the docstring sentence admitting it re-implemented the jobs_state pattern.
command/outdated.py _save_cache	atomic_write_text(..., fsync=False)	Stays inside the existing best-effort try/except OSError: pass; the helper's mkdir replaces the explicit path.parent.mkdir.
command/templates.py _persist_cache	atomic_write_bytes(..., fsync=False)	Keeps its best-effort wrapper and docstring.

The write policy, as a comment block adjacent to the helpers in file_utils.py: four tiers (secrets → bespoke 0600; secret-adjacent state → bespoke 0600; cross-process state → fsync=True; regenerable caches → fsync=False), each naming its members, plus the two recorded invariants (why JobState files are umask-mode, and why nobody fsyncs the parent directory after os.replace). auth/store._write_all and download_state.write_path each gained a one-line pointer at that note so the next dedup sweep doesn't re-flag them.

Permissions

No writer's permissions change, with one deliberate exception called out in the ticket and in the code: the templates gallery cache. Its old inline tempfile.mkstemp left it at mode 0600 incidentally (mkstemp hardcodes 0600 and os.replace carries that onto the destination); the shared helper restores the umask-derived mode instead. That relaxation is intended — the payload is the public template-gallery index, not user data. There's a test pinning the new mode.

Judgment calls

This is stacked on #595, not a rebase-and-land of it. The ticket's precondition said to rebase #595 onto main, resolve the conflict, and land it first. I resolved the conflict but could not land it (merging is not mine to do), and rebasing would have required a force-push. So instead I pushed the conflict resolution to #595's branch as an ordinary merge commit — a fast-forward, no history rewritten — which took #595 from CONFLICTING to MERGEABLE, and stacked this work on top. That also keeps this PR's diff to just my own commit rather than double-counting all of main. The conflict itself was one import block in command/workflow.py where both sides had added an import; resolved by keeping both.

The shared implementation writes in binary mode. atomic_write_text now encodes UTF-8 itself and the shared _atomic_write opens "wb", where it previously opened "w", encoding="utf-8". On Windows that drops the automatic \n → \r\n translation, so files written by all seven text call sites now get LF line endings there instead of CRLF. This is safe and I'd argue more correct: every reader of these files goes through read_text() or json.loads(), both of which normalize newlines, so the round-trip is unchanged; no test asserts exact bytes for any of them; and it makes the two variants byte-identical in behavior. Flagging it explicitly because I can't exercise Windows locally — the windows-latest CI leg covers it.

Two existing tests were rewritten, not deleted. Both asserted the old implementation, not behavior, and could not survive the migration:

• test_save_cache_pins_the_write_encoding spied on Path.write_text to check encoding="utf-8" was named. The helper doesn't use Path.write_text. Rewritten to assert the same contract one layer down and rather more directly — that the bytes reaching the filesystem are the payload encoded UTF-8 by us, so the host locale never gets a vote.
• test_readonly_cache_dir_still_serves_fetched_data patched templates_cmd.tempfile.mkstemp to force a write failure; templates.py no longer imports tempfile at module scope. Repointed at file_utils.tempfile.mkstemp, which keeps the test end-to-end through the real _persist_cache → atomic_write_bytes path rather than stubbing the helper out.

Negative-claim falsification: not triggered. This diff denies no capability — it adds no not supported / unavailable / STOP strings, no throw/deny dead-end, and flips no test to assert a dead-end. It is a pure consolidation: every migrated writer keeps its existing success and failure behavior, and one (_write_assets_lock) gains cleanup it previously lacked.

Testing

• pytest: 3756 passed, 37 skipped (full suite).
• ruff check . and ruff format --check .: clean, run against v0.15.15, the version pinned in .pre-commit-config.yaml. Worth knowing for anyone reproducing locally: a plain uv sync resolves ruff 0.12.7, which reports 17 pre-existing errors and 1 format diff on main untouched by this PR — version skew, not real findings.
• New atomic_write_bytes unit tests mirroring the text-variant cases: create-with-parents, overwrite, verbatim payload (non-UTF-8 bytes and bare LFs survive), fsync path, cleanup-with-destination-untouched on a failed rename, and the umask-default mode that the templates cache now relies on.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bd0744de-f61f-4b14-b31f-aa7305a1bd6a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 10 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	4
🟢 Low	5

Panel: 8/8 reviewers contributed findings.

---

comfy_cli/file_utils.py:116 — 🟠 High — Switching the shared implementation from os.fdopen(fd, "w", encoding="utf-8") to "wb" drops text-mode newline translation, so on Windows every atomic_write_text caller now emits bare LF where it previously emitted CRLF. Callers that read an existing file with universal newlines and rewrite it whole (e.g. the comfy_cli/skills writers for AGENTS.md/CLAUDE.md) will silently convert CRLF files to LF; either keep a newline-translating text path for atomic_write_text or document the deliberate change and update the bytes-variant test comment that still claims "no platform newline translation" distinguishes the twin. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, gemini-3.1-pro edge-case).

comfy_cli/command/project.py:289 — 🟡 Medium — The removed inline writer let an os.fsync failure propagate before os.replace, aborting the push so the lock was never published; atomic_write_text swallows fsync errors (except OSError: pass) and renames anyway. An EIO/ENOSPC writeback failure now silently overwrites a known-good asset lock with possibly-undurable content and reports success, contradicting the tier-3 durability rationale in the comment right above. Consider a strict-fsync option for tier-3 callers. Raised by 4 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, gemini-3.1-pro adversarial, claude-opus-5-thinking-max edge-case).

comfy_cli/file_utils.py:136 — 🟡 Medium — The mode is re-applied with os.chmod(tmp_name, dest_mode) by path after the fd is closed, and os.chmod follows symlinks. In a world-writable destination directory an attacker who observes the mkstemp name can unlink it and drop a symlink in that window, chmod-ing an arbitrary file owned by the invoking user — which defeats the "non-symlink-following fd" claim in the comment above. Use os.fchmod(f.fileno(), dest_mode) inside the with block instead, which also makes the mode durable before the fsync. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, kimi-k2.7-code edge-case).

comfy_cli/file_utils.py:132 — 🟡 Medium — umask = os.umask(0); os.umask(umask) is a non-atomic read-modify-write of process-global state, and it isn't in a try/finally — a KeyboardInterrupt between the two calls leaves the umask at 0 for the rest of the process, and any concurrent thread creating a file in that window (telemetry flush in tracking.py, the OAuth callback server, launch stream redirectors) gets 0666/0777. Wrap the probe in try/finally at minimum, or avoid mutating the umask by deriving the mode from the tmp file that mkstemp already created. Raised by 4 of 8 reviewers (gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max adversarial, kimi-k2.7-code adversarial, kimi-k2.7-code edge-case).

comfy_cli/command/templates.py:148 — 🟡 Medium — The gallery cache moves from mkstemp's fixed 0600 to a umask-derived mode, so under a permissive umask (0002/0000, common in containers and CI images) index.json becomes group- or world-writable. _load_gallery feeds the parsed rows into template resolution and into _enforce_spend_gate's paid-node detection with no integrity check, so a local user could strip those signals and weaken the credit-spend consent gate. The comment's rationale ("the payload is the public template-gallery index") addresses confidentiality but not tampering. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial).

comfy_cli/file_utils.py:116 — 🟢 Low — If os.fdopen(fd, "wb") itself raises, the fd from mkstemp is leaked: the cleanup path only unlinks the tmp file and never closes the raw fd. Guard the fdopen call so the descriptor is closed on that failure. Raised by 1 of 8 reviewers (kimi-k2.7-code edge-case).

comfy_cli/command/templates.py:145 — 🟢 Low — The comment claims the helper "restores the umask-derived mode", but _atomic_write only falls back to 0666 & ~umask when os.stat(path) fails; when the destination already exists it reuses the destination's current mode. Cache files already on disk at 0600 from the old mkstemp path therefore stay 0600 forever, so the intended relaxation applies only to fresh installs — and test_atomic_write_bytes_new_file_uses_umask_default covers only the fresh-file case. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).

comfy_cli/file_utils.py:130 — 🟢 Low — Preferring the destination's existing mode means the helper faithfully re-applies whatever bits are already there, including group/world-writable and setuid/setgid/sticky bits that stat.S_IMODE preserves. A file pre-created by a local user in a shared directory stays attacker-writable across every subsequent atomic rewrite instead of being healed, and os.stat follows symlinks so the mode can be sourced from an unrelated file. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, kimi-k2.7-code adversarial).

tests/comfy_cli/command/test_templates.py:548 — 🟢 Low — monkeypatch.setattr(file_utils.tempfile, "mkstemp", ...) mutates the attribute on the shared stdlib tempfile module object rather than on file_utils, so mkstemp raises OSError process-wide for the entire runner.invoke. The test then passes even if _persist_cache/atomic_write_bytes is never reached, masking a regression in the best-effort write path it is meant to pin. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).

tests/comfy_cli/command/test_node_deps.py:834 — 🟢 Low — The new assertion reduces to X.encode("utf-8") == X.encode("utf-8") for a pure-ASCII X (json.dumps escapes café under the default ensure_ascii=True), so it holds under any ASCII-compatible codec and cannot detect the locale-picked text-mode write the docstring says it guards — weaker than the encoding == "utf-8" assertion it replaces. Compare against a payload with a literal non-ASCII byte (e.g. ensure_ascii=False) or assert the encode step directly. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).

(Inline comments could not be anchored to the diff; listed above instead.)

[bigcat88]

Your base, #595, is now merged — I reviewed it in depth and landed it (3779 passed, mode parity with main verified on every migrated writer, atomicity exercised under 6 concurrent writers and 20 mid-write SIGKILLs).

That squash-merge is exactly what has flipped this PR to CONFLICTING: GitHub retargeted it to main, and the squashed commit no longer matches the merge-commit history this branch was built on. Not your fault and nothing wrong with the change — it just needs a rebase onto current main before I can review it.

Two things from the #595 review that are worth folding into this PR while you're in there, since it's the one that writes down the policy:

• os.umask(0) / os.umask(umask) in _atomic_write is process-global and not thread-safe — a thread creating a file inside that two-syscall window gets a 0 umask. Unreachable today (all writers are on the main thread; the watcher is a separate process), so a note in the comment is enough. Your write-policy block is the natural home for it.
• Stranded tmp files are now unbounded rather than bounded by pid reuse. mkstemp gives a fresh name per attempt, so every crash mid-write leaves a distinct corpse; I measured 13 survivors from 20 kills. Nothing prunes the jobs directory and jobs ls globs *.json, so they accumulate invisibly. A *.tmp sweep alongside the stale-watcher reap in _gather_local_state_files would close it.

Ping me once it's rebased and I'll pick it straight up — the atomic_write_bytes + write-policy work reads well from the description.

[mattmillerai]

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

• BE-6392 — Harden mode handling for tier-4 atomic_write_* cache files against local tampering — filed as agent-spike (premise unverified)

The following carry agent-spike instead of agent-ok because their reachability claim was not backed by evidence (BE-5378) — the claim is investigated before any code is written, and "the premise does not hold" is a valid, successful outcome:

• Harden mode handling for tier-4 atomic_write_ cache files against local tampering* — no reachability block in the proposal