From 22f7c734db0e3b9029be0fd0bd8dec620b3e7a4f Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 00:02:25 -0700 Subject: [PATCH 1/5] refactor: promote atomic_write_text to a shared util; migrate vanilla tmp+replace sites (BE-4353) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidate the duplicated "write via tmp file + os.replace" pattern into a single shared helper `comfy_cli.file_utils.atomic_write_text(path, content, *, fsync=False)` and migrate the vanilla copies onto it: - skills.write_manifest — also FIXES an existing tmp-file leak (an exception between the write and the rename previously left the tmp behind). - skills._atomic_write_text — removed; all four skill writers now use the shared helper. - cql/loader.write_object_info_cache — keeps its best-effort try/except OSError wrapper around the call. - jobs_state.write — uses fsync=True, keeping its locking.file_lock wrapper. - command/workflow._atomic_write_text — removed; a sixth identical copy the ticket's finder overlooked, migrated for completeness. auth/store._write_all is deliberately left untouched: it is a security-hardened superset (O_EXCL + 0o600 at open, 0o700 parent dir, fsync) protecting secrets. --- comfy_cli/command/workflow.py | 21 ++----------- comfy_cli/cql/loader.py | 12 +++----- comfy_cli/file_utils.py | 38 ++++++++++++++++++++++++ comfy_cli/jobs_state.py | 17 ++--------- comfy_cli/skills/__init__.py | 31 +++++--------------- tests/comfy_cli/test_file_utils.py | 47 ++++++++++++++++++++++++++++++ 6 files changed, 102 insertions(+), 64 deletions(-) diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py index 383651d7..9db14de8 100644 --- a/comfy_cli/command/workflow.py +++ b/comfy_cli/command/workflow.py @@ -23,6 +23,7 @@ import typer from comfy_cli import tracking +from comfy_cli.file_utils import atomic_write_text from comfy_cli.output import get_renderer, rprint app = typer.Typer(no_args_is_help=True, help="Slot-based editing of frontend-format ComfyUI workflows.") @@ -115,22 +116,6 @@ def _get_graph(input_path: str | None, host: str | None, port: int | None, on_st raise typer.Exit(code=1) from e -def _atomic_write_text(path: Path, content: str) -> None: - """Write via tmp + rename so SIGINT mid-write can't leave a half-written file.""" - import os - - tmp = path.with_suffix(path.suffix + f".{os.getpid()}.tmp") - try: - tmp.write_text(content, encoding="utf-8") - os.replace(tmp, path) - except Exception: - try: - tmp.unlink() - except OSError: - pass - raise - - def _parse_value(raw: str) -> Any: """Parse a CLI-supplied value as JSON; fall back to the literal string.""" try: @@ -275,7 +260,7 @@ def set_slot_cmd( sys.stdout.write("\n") return - _atomic_write_text(p, serialized) + atomic_write_text(p, serialized) payload = { "workflow": str(p), @@ -372,7 +357,7 @@ def vary_cmd( out.mkdir(parents=True, exist_ok=True) for i, wf in enumerate(workflows): target = out / f"{p.stem}_{i:03d}.json" - _atomic_write_text(target, json.dumps(wf, indent=2)) + atomic_write_text(target, json.dumps(wf, indent=2)) written.append(str(target)) else: import sys diff --git a/comfy_cli/cql/loader.py b/comfy_cli/cql/loader.py index f432882e..0f366de2 100644 --- a/comfy_cli/cql/loader.py +++ b/comfy_cli/cql/loader.py @@ -30,6 +30,7 @@ from comfy_cli.cql._net import is_loopback_host from comfy_cli.cql.errors import CQLRuntimeError +from comfy_cli.file_utils import atomic_write_text from comfy_cli.http import NoRedirectHandler # Cap raw bytes read from disk or the network. Real `object_info` dumps are a @@ -310,16 +311,11 @@ def write_object_info_cache(host_key: str, data: dict[str, Any]) -> None: """ path = object_info_cache_path(host_key) try: - path.parent.mkdir(parents=True, exist_ok=True) - tmp = path.with_suffix(path.suffix + f".{os.getpid()}.tmp") - tmp.write_text(json.dumps(data), encoding="utf-8") - os.replace(tmp, path) + atomic_write_text(path, json.dumps(data)) except OSError: # A cache we can't write is not worth failing the command over. - try: - tmp.unlink() # type: ignore[possibly-undefined] - except (OSError, NameError, UnboundLocalError): - pass + # atomic_write_text already cleaned up its own tmp file on failure. + pass def read_object_info_cache(host_key: str) -> dict[str, Any] | None: diff --git a/comfy_cli/file_utils.py b/comfy_cli/file_utils.py index 3a84027c..ff79c7aa 100644 --- a/comfy_cli/file_utils.py +++ b/comfy_cli/file_utils.py @@ -13,6 +13,44 @@ from comfy_cli import constants, ui +def atomic_write_text(path: pathlib.Path, content: str, *, fsync: bool = False) -> None: + """Atomically write ``content`` to ``path`` via a sibling tmp file + ``os.replace``. + + The write goes to a per-process tmp file in the same directory (so the rename + stays on one filesystem and is atomic), which is then renamed over ``path``. + A SIGINT or crash mid-write therefore never leaves a half-written or empty + file at the destination — readers see either the old contents or the new. + On any failure the tmp file is cleaned up and the exception re-raised. + + Args: + path: destination file. Parent directories are created if missing. + content: text to write (UTF-8). + fsync: if True, flush the tmp file's contents to disk before the rename + (durability against power loss, at the cost of a sync). Best-effort: + a failing fsync is ignored, matching the prior per-site behavior. + """ + path.parent.mkdir(parents=True, exist_ok=True) + tmp = path.with_suffix(path.suffix + f".{os.getpid()}.tmp") + try: + tmp.write_text(content, encoding="utf-8") + if fsync: + try: + fd = os.open(str(tmp), os.O_RDONLY) + try: + os.fsync(fd) + finally: + os.close(fd) + except OSError: + pass + os.replace(tmp, path) + except Exception: + try: + tmp.unlink() + except OSError: + pass + raise + + class DownloadException(Exception): pass diff --git a/comfy_cli/jobs_state.py b/comfy_cli/jobs_state.py index 2758125d..1b83549e 100644 --- a/comfy_cli/jobs_state.py +++ b/comfy_cli/jobs_state.py @@ -39,15 +39,14 @@ from __future__ import annotations import json -import os import re -import secrets as _secrets from dataclasses import asdict, dataclass, field from datetime import datetime, timezone from pathlib import Path from typing import Any from comfy_cli import constants, locking +from comfy_cli.file_utils import atomic_write_text from comfy_cli.utils import get_os TERMINAL_STATUSES = frozenset({"completed", "error", "cancelled"}) @@ -123,18 +122,8 @@ def write(state: JobState) -> Path | None: # Lock per-file so a watcher and a foreground update can't tear each # other's writes. with locking.file_lock(path.with_suffix(".lock")): - tmp = path.with_suffix(f".{os.getpid()}.{_secrets.token_hex(4)}.tmp") - tmp.write_text(json.dumps(state.to_dict(), indent=2, default=str), encoding="utf-8") - # fsync for durability before atomic rename - try: - fd = os.open(str(tmp), os.O_RDONLY) - try: - os.fsync(fd) - finally: - os.close(fd) - except OSError: - pass - os.replace(tmp, path) + # fsync=True: durability against power loss before the atomic rename. + atomic_write_text(path, json.dumps(state.to_dict(), indent=2, default=str), fsync=True) return path diff --git a/comfy_cli/skills/__init__.py b/comfy_cli/skills/__init__.py index 78f22c42..9297e72c 100644 --- a/comfy_cli/skills/__init__.py +++ b/comfy_cli/skills/__init__.py @@ -36,6 +36,8 @@ from pathlib import Path from typing import Literal +from comfy_cli.file_utils import atomic_write_text + # Where the bundled skills live. Each tuple is (skill_name, package_subdir). # ``skill_name`` is the public identifier used in AGENTS.md fences and as the # subdir name in installed targets. ``package_subdir`` is the local resource @@ -199,11 +201,7 @@ def read_manifest() -> dict: def write_manifest(manifest: dict) -> None: """Atomically write the manifest (tmp + rename so a SIGINT can't corrupt it).""" - path = manifest_path() - path.parent.mkdir(parents=True, exist_ok=True) - tmp = path.with_suffix(f".{os.getpid()}.tmp") - tmp.write_text(json.dumps(manifest, indent=2), encoding="utf-8") - os.replace(tmp, path) + atomic_write_text(manifest_path(), json.dumps(manifest, indent=2)) def _sha256(text: str) -> str: @@ -577,24 +575,9 @@ def _backup_if_user_edited(path: Path, expected_content: str) -> Path | None: return bak -def _atomic_write_text(path: Path, content: str) -> None: - """Write via tmp + rename so a SIGINT mid-write can't leave the file empty.""" - path.parent.mkdir(parents=True, exist_ok=True) - tmp = path.with_suffix(path.suffix + f".{os.getpid()}.tmp") - try: - tmp.write_text(content, encoding="utf-8") - os.replace(tmp, path) - except Exception: - try: - tmp.unlink() - except OSError: - pass - raise - - def _write_claude_skill(path: Path, content: str) -> None: _backup_if_user_edited(path, content) - _atomic_write_text(path, content) + atomic_write_text(path, content) def _cursor_description_for(skill_name: str) -> str: @@ -610,14 +593,14 @@ def _write_cursor_rule(path: Path, content: str, *, skill_name: str) -> None: body = _strip_frontmatter(content) rule = f'---\ndescription: {_cursor_description_for(skill_name)}\nglobs: "**/*"\nalwaysApply: false\n---\n\n{body}' _backup_if_user_edited(path, rule) - _atomic_write_text(path, rule) + atomic_write_text(path, rule) def _upsert_agents_md_block(path: Path, content: str, *, skill_name: str) -> None: start, end = _agents_fence(skill_name) block = f"\n{start}\n{content}\n{end}\n" if not path.exists(): - _atomic_write_text(path, block.lstrip("\n")) + atomic_write_text(path, block.lstrip("\n")) return existing = path.read_text(encoding="utf-8") if start in existing and end in existing: @@ -626,7 +609,7 @@ def _upsert_agents_md_block(path: Path, content: str, *, skill_name: str) -> Non new = before.rstrip() + "\n\n" + block.lstrip("\n") + after.lstrip("\n") else: new = existing.rstrip() + "\n" + block - _atomic_write_text(path, new) + atomic_write_text(path, new) def _remove_agents_md_block(path: Path, *, skill_name: str) -> bool: diff --git a/tests/comfy_cli/test_file_utils.py b/tests/comfy_cli/test_file_utils.py index dfeed4ea..2af987ed 100644 --- a/tests/comfy_cli/test_file_utils.py +++ b/tests/comfy_cli/test_file_utils.py @@ -1,6 +1,9 @@ import zipfile +import pytest + from comfy_cli import file_utils +from comfy_cli.file_utils import atomic_write_text def test_zip_files_respects_comfyignore(tmp_path, monkeypatch): @@ -81,3 +84,47 @@ def test_zip_files_without_git_falls_back_to_walk(tmp_path, monkeypatch): assert "file.txt" in names assert "node.zip" not in names + + +def test_atomic_write_text_creates_file_and_parents(tmp_path): + target = tmp_path / "sub" / "dir" / "out.json" + atomic_write_text(target, '{"a": 1}') + + assert target.read_text(encoding="utf-8") == '{"a": 1}' + # No stray tmp files left behind. + assert list(target.parent.glob("*.tmp")) == [] + + +def test_atomic_write_text_overwrites_existing(tmp_path): + target = tmp_path / "out.txt" + target.write_text("old", encoding="utf-8") + + atomic_write_text(target, "new") + + assert target.read_text(encoding="utf-8") == "new" + + +def test_atomic_write_text_fsync_true_still_writes(tmp_path): + target = tmp_path / "durable.txt" + atomic_write_text(target, "durable", fsync=True) + + assert target.read_text(encoding="utf-8") == "durable" + assert list(target.parent.glob("*.tmp")) == [] + + +def test_atomic_write_text_cleans_up_tmp_on_failure(tmp_path, monkeypatch): + target = tmp_path / "out.txt" + target.write_text("original", encoding="utf-8") + + def boom(src, dst): + raise OSError("replace failed") + + # Fail at the rename step, after the tmp file has been written. + monkeypatch.setattr(file_utils.os, "replace", boom) + + with pytest.raises(OSError): + atomic_write_text(target, "new content") + + # The tmp file is cleaned up and the destination is untouched. + assert list(target.parent.glob("*.tmp")) == [] + assert target.read_text(encoding="utf-8") == "original" From 92e70fee05500cc80ba6ef0e424f2abc4e55a2c3 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 02:46:41 -0700 Subject: [PATCH 2/5] fix(file_utils): harden atomic_write_text per cursor-review (BE-4353) Rewrite the tmp-file creation to use tempfile.mkstemp in the destination directory, addressing the consolidated cursor-review panel findings: - High: drop the PID-only tmp name that let concurrent writers collide on ..tmp; mkstemp gives a unique name per write. - Medium (CWE-377): mkstemp opens with O_CREAT|O_EXCL and doesn't follow symlinks, so a pre-planted symlink can't redirect the write. - Medium: fsync now uses the O_RDWR fd from mkstemp, so os.fsync no longer silently no-ops on Windows (FlushFileBuffers needs write access). - Low: except BaseException so a KeyboardInterrupt mid-write still cleans up the tmp file, per the docstring. - Nit: fsync the parent directory after os.replace so the rename itself is durable against power loss. Add tests for unique-per-write tmp names and no-symlink-following. Co-Authored-By: Claude Opus 4.8 --- comfy_cli/file_utils.py | 42 +++++++++++++++++++++++------- tests/comfy_cli/test_file_utils.py | 39 +++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 10 deletions(-) diff --git a/comfy_cli/file_utils.py b/comfy_cli/file_utils.py index ff79c7aa..fbe957c4 100644 --- a/comfy_cli/file_utils.py +++ b/comfy_cli/file_utils.py @@ -2,6 +2,7 @@ import os import pathlib import subprocess +import tempfile import time import zipfile from http import HTTPStatus @@ -16,36 +17,57 @@ def atomic_write_text(path: pathlib.Path, content: str, *, fsync: bool = False) -> None: """Atomically write ``content`` to ``path`` via a sibling tmp file + ``os.replace``. - The write goes to a per-process tmp file in the same directory (so the rename + The write goes to a uniquely-named tmp file in the same directory (so the rename stays on one filesystem and is atomic), which is then renamed over ``path``. A SIGINT or crash mid-write therefore never leaves a half-written or empty file at the destination — readers see either the old contents or the new. On any failure the tmp file is cleaned up and the exception re-raised. + The tmp file is created with ``tempfile.mkstemp`` (``O_CREAT | O_EXCL``, no + symlink following) in the destination directory, so concurrent writers never + collide on a shared name and a pre-planted symlink can't redirect the write + (CWE-377). + Args: path: destination file. Parent directories are created if missing. content: text to write (UTF-8). fsync: if True, flush the tmp file's contents to disk before the rename - (durability against power loss, at the cost of a sync). Best-effort: + and fsync the destination directory afterwards so both the data and + the rename survive power loss, at the cost of a sync. Best-effort: a failing fsync is ignored, matching the prior per-site behavior. """ path.parent.mkdir(parents=True, exist_ok=True) - tmp = path.with_suffix(path.suffix + f".{os.getpid()}.tmp") + # mkstemp gives a unique, O_EXCL, non-symlink-following fd opened O_RDWR in the + # destination directory — same filesystem, so the os.replace below is atomic. + fd, tmp_name = tempfile.mkstemp(dir=str(path.parent), prefix=path.name + ".", suffix=".tmp") try: - tmp.write_text(content, encoding="utf-8") + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(content) + if fsync: + f.flush() + try: + os.fsync(f.fileno()) # O_RDWR fd, so fsync works on Windows too + except OSError: + pass + os.replace(tmp_name, path) if fsync: + # Also fsync the parent directory so the rename itself is durable. try: - fd = os.open(str(tmp), os.O_RDONLY) + dir_fd = os.open(str(path.parent), os.O_RDONLY) try: - os.fsync(fd) + os.fsync(dir_fd) + except OSError: + pass finally: - os.close(fd) + os.close(dir_fd) except OSError: + # e.g. Windows can't open a directory for fsync; best-effort. pass - os.replace(tmp, path) - except Exception: + except BaseException: + # BaseException (not just Exception) so a KeyboardInterrupt mid-write + # still cleans up the tmp file, per the docstring. try: - tmp.unlink() + os.unlink(tmp_name) except OSError: pass raise diff --git a/tests/comfy_cli/test_file_utils.py b/tests/comfy_cli/test_file_utils.py index 2af987ed..f1e239a5 100644 --- a/tests/comfy_cli/test_file_utils.py +++ b/tests/comfy_cli/test_file_utils.py @@ -128,3 +128,42 @@ def boom(src, dst): # The tmp file is cleaned up and the destination is untouched. assert list(target.parent.glob("*.tmp")) == [] assert target.read_text(encoding="utf-8") == "original" + + +def test_atomic_write_text_tmp_name_is_unique_per_write(tmp_path, monkeypatch): + # Two writes from the "same" pid must not collide on a shared tmp path. + target = tmp_path / "out.txt" + seen = [] + real_mkstemp = file_utils.tempfile.mkstemp + + def spy(*args, **kwargs): + fd, name = real_mkstemp(*args, **kwargs) + seen.append(name) + return fd, name + + monkeypatch.setattr(file_utils.tempfile, "mkstemp", spy) + monkeypatch.setattr(file_utils.os, "getpid", lambda: 4242) + + atomic_write_text(target, "a") + atomic_write_text(target, "b") + + assert len(seen) == 2 + assert seen[0] != seen[1] + assert target.read_text(encoding="utf-8") == "b" + + +def test_atomic_write_text_does_not_follow_symlinked_tmp(tmp_path): + # A pre-planted symlink at a predictable tmp path must not redirect the write. + target = tmp_path / "out.txt" + victim = tmp_path / "victim.txt" + victim.write_text("do-not-touch", encoding="utf-8") + # The old scheme used "..tmp"; plant a symlink there. + import os as _os + + decoy = tmp_path / f"out.txt.{_os.getpid()}.tmp" + decoy.symlink_to(victim) + + atomic_write_text(target, "new") + + assert target.read_text(encoding="utf-8") == "new" + assert victim.read_text(encoding="utf-8") == "do-not-touch" From 8fe46139a4452afaafa21eb9fdc0329b7d4e76cc Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 03:14:32 -0700 Subject: [PATCH 3/5] fix(file_utils): preserve destination mode in atomic_write_text + assert fsync (BE-4353) CodeRabbit review follow-ups: - mkstemp hardcodes the tmp file to 0600 and os.replace carries that mode onto the destination, so a first atomic write silently stripped group/other read from shared outputs. Restore the existing destination's mode, else the umask-derived default for a new file, before the rename. This matches the pre-migration write_text() behavior of the migrated call sites. - Strengthen the fsync test to spy on os.fsync and assert it's actually invoked on the tmp fd (and the parent dir fd off Windows), plus add mode-preservation tests. Co-Authored-By: Claude Opus 4.8 --- comfy_cli/file_utils.py | 17 ++++++++++ tests/comfy_cli/test_file_utils.py | 50 +++++++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/comfy_cli/file_utils.py b/comfy_cli/file_utils.py index fbe957c4..dfac7131 100644 --- a/comfy_cli/file_utils.py +++ b/comfy_cli/file_utils.py @@ -1,6 +1,7 @@ import json import os import pathlib +import stat import subprocess import tempfile import time @@ -49,6 +50,22 @@ def atomic_write_text(path: pathlib.Path, content: str, *, fsync: bool = False) os.fsync(f.fileno()) # O_RDWR fd, so fsync works on Windows too except OSError: pass + # mkstemp hardcodes the tmp file to 0600, and os.replace carries that mode + # onto the destination — so without this a first atomic write would quietly + # strip the group/other read access a shared output is meant to have. Restore + # the intended mode before the rename: reuse the existing destination's bits, + # else fall back to the umask-derived default (0666 & ~umask) for a new file. + try: + dest_mode = stat.S_IMODE(os.stat(path).st_mode) + except OSError: + umask = os.umask(0) + os.umask(umask) + dest_mode = 0o666 & ~umask + try: + os.chmod(tmp_name, dest_mode) + except OSError: + # Windows / filesystems without POSIX perms: best-effort, matching fsync. + pass os.replace(tmp_name, path) if fsync: # Also fsync the parent directory so the rename itself is durable. diff --git a/tests/comfy_cli/test_file_utils.py b/tests/comfy_cli/test_file_utils.py index f1e239a5..408ef01b 100644 --- a/tests/comfy_cli/test_file_utils.py +++ b/tests/comfy_cli/test_file_utils.py @@ -1,3 +1,6 @@ +import os +import stat +import sys import zipfile import pytest @@ -104,12 +107,57 @@ def test_atomic_write_text_overwrites_existing(tmp_path): assert target.read_text(encoding="utf-8") == "new" -def test_atomic_write_text_fsync_true_still_writes(tmp_path): +def test_atomic_write_text_fsync_true_still_writes(tmp_path, monkeypatch): target = tmp_path / "durable.txt" + + # Spy on os.fsync so we assert it's actually invoked, not silently no-op'd + # (the class of bug flagged for the Windows O_RDONLY case). Delegate to the + # real fsync so durability behavior is preserved. + real_fsync = file_utils.os.fsync + synced_fds = [] + + def spy_fsync(fd): + synced_fds.append(fd) + return real_fsync(fd) + + monkeypatch.setattr(file_utils.os, "fsync", spy_fsync) + atomic_write_text(target, "durable", fsync=True) assert target.read_text(encoding="utf-8") == "durable" assert list(target.parent.glob("*.tmp")) == [] + # The tmp file fd is always fsynced (O_RDWR, works cross-platform). On POSIX + # the parent directory fd is fsynced too; Windows can't open a dir for fsync + # and best-effort skips it, so only require the extra sync off Windows. + if sys.platform == "win32": + assert len(synced_fds) >= 1 + else: + assert len(synced_fds) == 2 + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX file-mode semantics") +def test_atomic_write_text_preserves_existing_mode(tmp_path): + # A first atomic write must not strip the destination's group/other read bits. + target = tmp_path / "shared.json" + target.write_text("old", encoding="utf-8") + os.chmod(target, 0o644) + + atomic_write_text(target, "new") + + assert stat.S_IMODE(os.stat(target).st_mode) == 0o644 + assert target.read_text(encoding="utf-8") == "new" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX file-mode semantics") +def test_atomic_write_text_new_file_uses_umask_default(tmp_path): + # A new destination gets the umask-derived default, not mkstemp's hardcoded 0600. + old_umask = os.umask(0o022) + try: + target = tmp_path / "fresh.json" + atomic_write_text(target, "data") + assert stat.S_IMODE(os.stat(target).st_mode) == (0o666 & ~0o022) + finally: + os.umask(old_umask) def test_atomic_write_text_cleans_up_tmp_on_failure(tmp_path, monkeypatch): From c5e4d4e42b8039042e7e3592371c3e23da6ea4c4 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 03:26:25 -0700 Subject: [PATCH 4/5] test: use 0o640 not 0o644 in mode-preservation test (BE-4353) Resolves the CodeQL py/overly-permissive-file (high) alert flagged on the atomic-write mode-preservation test. The test only needs a non-0600 mode to prove mkstemp's hardcoded 0600 does not clobber the destination mode; 0o640 keeps the group-read regression guard without granting world read. Co-Authored-By: Claude Opus 4.8 --- tests/comfy_cli/test_file_utils.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/comfy_cli/test_file_utils.py b/tests/comfy_cli/test_file_utils.py index 408ef01b..08f70c5f 100644 --- a/tests/comfy_cli/test_file_utils.py +++ b/tests/comfy_cli/test_file_utils.py @@ -137,14 +137,16 @@ def spy_fsync(fd): @pytest.mark.skipif(sys.platform == "win32", reason="POSIX file-mode semantics") def test_atomic_write_text_preserves_existing_mode(tmp_path): - # A first atomic write must not strip the destination's group/other read bits. + # A first atomic write must not strip the destination's group read bit + # (mkstemp hardcodes 0600, so without mode restoration this would regress to + # owner-only). Uses 0o640 rather than 0o644 to avoid granting world read. target = tmp_path / "shared.json" target.write_text("old", encoding="utf-8") - os.chmod(target, 0o644) + os.chmod(target, 0o640) atomic_write_text(target, "new") - assert stat.S_IMODE(os.stat(target).st_mode) == 0o644 + assert stat.S_IMODE(os.stat(target).st_mode) == 0o640 assert target.read_text(encoding="utf-8") == "new" From e8ae317ec0959baf4adf0e8ea528057e28d8ba48 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 03:37:53 -0700 Subject: [PATCH 5/5] test: use owner-exec 0o700 to satisfy CodeQL in mode-preservation test (BE-4353) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior 0o640 (world→group) still trips py/overly-permissive-file because group-read is flagged too. The test only needs a non-0600 mode to prove mkstemp's hardcoded 0600 doesn't clobber the destination mode, so use owner-execute (0o700) — a bit 0600 lacks, with no group/other bits — keeping the required CodeQL check green. Co-Authored-By: Claude Opus 4.8 --- tests/comfy_cli/test_file_utils.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/comfy_cli/test_file_utils.py b/tests/comfy_cli/test_file_utils.py index 08f70c5f..0011f5cf 100644 --- a/tests/comfy_cli/test_file_utils.py +++ b/tests/comfy_cli/test_file_utils.py @@ -137,16 +137,18 @@ def spy_fsync(fd): @pytest.mark.skipif(sys.platform == "win32", reason="POSIX file-mode semantics") def test_atomic_write_text_preserves_existing_mode(tmp_path): - # A first atomic write must not strip the destination's group read bit - # (mkstemp hardcodes 0600, so without mode restoration this would regress to - # owner-only). Uses 0o640 rather than 0o644 to avoid granting world read. + # A first atomic write must not clobber the destination's mode down to mkstemp's + # hardcoded 0600. We only need a non-0600 mode to prove restoration happens, so + # use owner-execute (0o700) — a bit mkstemp's 0600 lacks — rather than a + # group/other-readable mode. That keeps this clear of the py/overly-permissive-file + # scanner while still exercising the exact "restore bits beyond 0600" path. target = tmp_path / "shared.json" target.write_text("old", encoding="utf-8") - os.chmod(target, 0o640) + os.chmod(target, 0o700) atomic_write_text(target, "new") - assert stat.S_IMODE(os.stat(target).st_mode) == 0o640 + assert stat.S_IMODE(os.stat(target).st_mode) == 0o700 assert target.read_text(encoding="utf-8") == "new"