test_file_utils umask tests assert against a umask the code never reads (import-time _UMASK), so they only pass on a 0o022 host

Đang mở Phù hợp với người mới
  • Có gì không ổn?
#1,071 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

@neverbiasu đang làm issue này rồi.

Từ ngày 4/11/2026.

  • #1105 của @neverbiasu — đang mở

Đánh giá

Độ khó
2/5
Thời gian dự kiến
1-3 giờ
Mức phù hợp với người mới
88/100
Loại issue
Lỗi
Độ rõ ràng
Đặc tả rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
python
Lĩnh vực
testing

Hướng nghiên cứu

Đọc tests/comfy_cli/test_file_utils.py và comfy_cli/file_utils.py ở phần ghi nhận _UMASK. Cập nhật hai bài kiểm tra ghi nguyên tử để kiểm soát giá trị mà phần triển khai đọc, đồng thời bổ sung kiểm thử cho một umask thứ hai, chẳng hạn 0o077. Chạy các bài kiểm tra file-utils tập trung; chúng cần xác minh các chế độ tệp dự kiến mà không thay đổi umask của tiến trình.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

bug

Summary

test_atomic_write_text_new_file_uses_umask_default and test_atomic_write_bytes_new_file_uses_umask_default set a umask at runtime and assert the resulting file mode. file_utils captures the umask once at import, so the os.umask(0o022) the tests call has no effect on the code under test. The assertion therefore only holds on a host whose ambient umask already happens to be 0o022, and fails outright on any other.

Observed on a host with umask 0007:

tests/comfy_cli/test_file_utils.py:163: AssertionError
assert 432 == (438 & ~18)
#      ^ 0o660, i.e. 0o666 & ~0o007 (the AMBIENT umask)
#              ^ 0o644, i.e. 0o666 & ~0o022 (the umask the test set)

Both tests fail the same way; test_atomic_write_bytes_… at :313.

Cause

comfy_cli/file_utils.py:98-105:

# The process umask, captured once at import. os has no getter, so reading it
# ...
# overlapping probes can restore each other's zero and strand the umask at 0.
_UMASK = os.umask(0)
os.umask(_UMASK)

Capturing once is deliberate and the comment gives a good reason for it. The tests simply do not account for it: they manipulate the real process umask, which _atomic_write no longer consults.

So this is a test defect, not a product one. The product behaviour the tests mean to pin — "a new destination gets the umask-derived default, not mkstemp's hardcoded 0600" — is correct and worth keeping pinned; the tests just pin it against the wrong knob.

Suggested fix

Patch the captured value rather than the process umask, so the test controls what the code actually reads:

def test_atomic_write_text_new_file_uses_umask_default(tmp_path, monkeypatch):
    monkeypatch.setattr(file_utils, "_UMASK", 0o022)
    target = tmp_path / "fresh.json"
    atomic_write_text(target, "data")
    assert stat.S_IMODE(os.stat(target).st_mode) == (0o666 & ~0o022)

That also removes the os.umask save/restore, which is process-global state the current tests mutate.

A second case worth adding while in there: assert the same for a non-0o022 umask (0o077, so 0o600), which would have caught this — a single hardcoded umask value cannot distinguish "the code honours the umask" from "the host umask matches the literal in the assertion".

Context

Found while verifying that pre-existing failures were unrelated to a converter change. Two other environment-sensitive failures turned up in the same run and are worth separating:

  • Five tests/comfy_cli/command/test_code_search.py::TestCodeSearchCLI cases fail when the suite runs from inside an AI coding tool, because CLAUDECODE / AI_AGENT in the environment flip detect_caller() and code-search then passes agent_origin=True. Fixed in #1072.
  • test_node_deps.py::test_non_pep440_installed_version_is_unknown_not_a_crash reports mismatch where it expects unknown. Not agent-env related (it fails with those vars unset) and I have not diagnosed it; noting it so it is not assumed covered by the other two.

None of these fail in CI, which is why they have survived: CI's environment happens to satisfy every one of the assumptions.

Task Decomposition

Not decomposed: this is a two-test change in one file (swap os.umask(...) for monkeypatch.setattr(file_utils, "_UMASK", ...), plus one extra parametrised umask value). Stated explicitly per the issue-filing convention.

Ngôn ngữ chính
Python
Star
1k
Fork
176
Merge trung bình
1 ngày 11 giờ
Pull request đã merge (30 ngày)
87

Chuẩn bị môi trường

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của Comfy-Org/comfy-cli

Tất cả issue của Comfy-Org/comfy-cli

Issue tương tự

Thêm issue về Python

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.