Skip to content

test(cron): handle Windows path and permission semantics - #60888

Draft
rerdi92 wants to merge 1 commit into
NousResearch:mainfrom
rerdi92:fix/windows-python-test-portability
Draft

test(cron): handle Windows path and permission semantics#60888
rerdi92 wants to merge 1 commit into
NousResearch:mainfrom
rerdi92:fix/windows-python-test-portability

Conversation

@rerdi92

@rerdi92 rerdi92 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Make cron/config permission tests portable on Windows by keeping strict POSIX mode-bit assertions on POSIX only.
  • Keep Windows coverage meaningful by still asserting that sensitive files/directories are created.
  • Make the ~ workdir normalization test patch Windows-native home variables (USERPROFILE, HOMEDRIVE, HOMEPATH) in addition to HOME.

Root cause

Two tests encoded POSIX-only assumptions:

  1. Windows/NTFS does not round-trip chmod(0600/0700) through stat().st_mode as strict POSIX mode bits.
  2. Path("~").expanduser() on Windows uses USERPROFILE/HOMEDRIVE+HOMEPATH, not HOME alone.

Test plan

  • Reproduced the Windows failures before the patch:
    • tests/cron/test_file_permissions.py strict 0600/0700 assertions failed as 0666/0777.
    • tests/cron/test_cron_workdir.py::TestNormalizeWorkdir::test_tilde_expands resolved to C:\Users\82109 instead of patched HOME.
  • Focused verification:
python -m pytest tests/cron/test_file_permissions.py tests/cron/test_cron_workdir.py -q

29 passed
  • Syntax/checks:
python -m py_compile tests/cron/test_file_permissions.py tests/cron/test_cron_workdir.py
git diff --check

Scope note

This is intentionally separate from the cron profile-scoped jobs storage fix in #60882.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/cron Cron scheduler and job management platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows P3 Low — cosmetic, nice to have labels Jul 8, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment (token read-only)

PR 60888 adds unit tests for Windows path and permission semantics in cron. Test improvements for cross-platform compatibility. Well-scoped (2 files, 32 additions, 19 deletions). No security issues or debug artifacts detected.

LGTM - awaiting maintainer approval.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Windows portability coverage. The premise remains valid on current main: tests/cron/test_file_permissions.py:37-118 still asserts POSIX 0600/0700 mode bits unconditionally, while tests/cron/test_cron_workdir.py:48-52 only patches HOME before cron.jobs._normalize_workdir() calls Path.expanduser() at cron/jobs.py:823.

The patch matches existing repository precedent: tests/hermes_cli/test_atomic_json_write.py:162-166 limits durable mode-bit assertions to platforms that support them, and tests/tools/test_checkpoint_manager.py:57-60 patches USERPROFILE while clearing HOMEDRIVE and HOMEPATH for deterministic home expansion.

The PR is behind current main, but the two changed test regions remain applicable; salvage should be mechanical.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 10, 2026
@LceAn

LceAn commented Jul 13, 2026

Copy link
Copy Markdown

Independent confirmation: I reproduced the same two failures and arrived at the same fix independently on a different Windows setup — Python 3.13.14 (the original report in #60243 was 3.11.15), so the issue and this fix hold across Python versions.

For the workdir test, the USERPROFILE + HOMEDRIVE/HOMEPATH patch makes it pass (not skip) on Windows under 3.13, exercising the ~ expansion for real rather than gating it out.

One thing worth flagging for reviewers: this is the cron instance of a recurring HOME / POSIX-assumption pattern that also shows up in:

The assert_secure_mode() helper here is actually the nicer of the approaches taken — it keeps the existence assertion on Windows rather than skipping wholesale, which the 0o600 backup test (#59433) currently does with a plain skipif. Worth aligning on a shared helper across these if the maintainers want one pattern for the whole class of bug.

Not opening a competing PR — this one covers it. Posting only to add the cross-version verification and the cross-issue context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants