Skip to content

fix: Windows portability for os.chown and git-apply CRLF hermeticity - #136

Merged
adithya-s-k merged 1 commit into
huggingface:mainfrom
KNambiarDJsc:fix/windows-os-chown
Sep 17, 2026
Merged

adithya-s-k merged 1 commit into
huggingface:mainfrom
KNambiarDJsc:fix/windows-os-chown

Conversation

@KNambiarDJsc

Copy link
Copy Markdown
Contributor

Summary

Two of the mechanical root causes from the #130 Windows-portability triage (#2 and #4):

#2os.chown doesn't exist on Windows. grade.py's two call sites now guard on sys.platform. Both only ever run for real inside the Linux verifier container regardless of host OS (every other path in that module is an absolute container path like /workspace, /tests), so the guard changes no production behavior — it only lets the tests that call validate_submission() directly run on a Windows host. Four test files monkeypatch.setattr("os.chown", ...) to neutralize the call during unit tests; switched to raising=False so the patch itself doesn't AttributeError on a platform where the attribute never existed.

#4git apply --reverse isn't hermetic against the caller's git config. reverse_source() and reverse_crlf_patch() both run git apply against plain directories (not git repositories), so without an explicit override they silently inherit the caller's global git config. On a machine with core.autocrlf=true — the Git-for-Windows installer default — git apply was rewriting LF to CRLF while applying, corrupting the exact-byte reversal both functions promise and breaking the dedicated reverse_crlf_patch() fallback, which assumes the applied bytes are exactly what it constructed and verifies that via git blob hashes. Both invocations now pass -c core.autocrlf=false explicitly.

While tracking that down I also found two write_text() calls doing the same kind of damage independently of git apply: the reversed patch file (reverse_source()) and the private verifier test source (construct()) are both read back later as raw bytes, but were written with write_text(), whose newline translation rewrites \n to the host line separator on the way in. Switched both to write_bytes().

Also skips test_verifier_editor_backups.py's "fifo" parametrize case on win32: os.mkfifo has no Windows equivalent at all, so that one case can't run there (not a permissions issue like the neighboring symlink cases, which are left alone — those need your call on whether Developer Mode is a documented prerequisite, per my earlier comment on #130).

Test plan

  • uv run ruff check / ruff format --check on all touched files — clean.
  • Windows 11, full suite before/after: 249 failed, 97 errors → 242 failed, 74 errors (net -30). Grepped the full run for any remaining os.chown / os.mkfifo / CRLF-patch-apply failures — none; the only matches left are my own guard code and the one intentional skip.
  • Linux (WSL Ubuntu, uv sync --group dev --all-extras --frozen + uv run --all-extras pytest -q, matching CI): 1871 passed, 0 failed — including the fifo and symlink cases this skip doesn't touch on that platform.
  • The 7 Windows failures remaining in the files this PR touches are all the already-tracked symlink-privilege issue (WinError 1314, root cause pr_runtime pipeline (v0.3): SWE-bench-style PR mining with sandbox-verified oracles #3 in Track native Windows controller and artifact portability #130) — untouched here, pending your decision on documenting Developer Mode as a prerequisite.

Out of scope

Root cause #1 (emitter/bundle.py's mode-check tied to the bundle's content-addressed integrity hash) and root cause #3 (symlink privilege) — both flagged in my correction comment on #130 as needing your design/product decision, not something to ship unilaterally as a platform guard.

Closes part of #130.

Two mechanical fixes from the huggingface#130 Windows-portability triage:

- os.chown() doesn't exist as an attribute on Windows. grade.py's two call
  sites now guard on sys.platform; both only ever run for real inside the
  Linux verifier container regardless of host OS, so the guard changes no
  production behavior. Four test files monkeypatch os.chown to neutralize
  it during unit tests; switched to raising=False so that patch doesn't
  itself AttributeError on a platform where the attribute never existed.

- reverse_source()'s and reverse_crlf_patch()'s `git apply --reverse`
  invocations run in plain directories, not git repositories, so they
  silently inherit the *caller's global* git config. On a machine with
  core.autocrlf=true (the Git-for-Windows installer default), git apply
  was rewriting LF to CRLF while applying, corrupting the exact-byte
  reversal both functions promise and breaking the dedicated
  reverse_crlf_patch() fallback that assumes the invocation is hermetic.
  Both invocations now pass -c core.autocrlf=false explicitly. Also
  switched two write_text() calls (the reversed patch file, and the
  private verifier test source) to write_bytes(), since both are later
  read back as raw bytes and write_text's own newline translation was an
  independent source of the same corruption on Windows.

Also skips test_verifier_editor_backups.py's "fifo" parametrize case on
win32: os.mkfifo has no Windows equivalent, so the case can't run there
at all (not a permissions issue like the symlink cases nearby, which are
left alone pending the maintainer's call on huggingface#130).

Verified: full suite green on Linux (WSL, uv sync --group dev --all-extras
--frozen); on Windows this eliminates the os.chown/mkfifo AttributeErrors
and the git-apply CRLF corruption from the local suite (249 failed/97
errors -> 242 failed/74 errors), leaving only the already-tracked
symlink-privilege failures untouched.
@adithya-s-k
adithya-s-k merged commit 7077ab9 into huggingface:main Sep 17, 2026
11 checks passed
@adithya-s-k

Copy link
Copy Markdown
Collaborator

Thanks, tested and merged. Reproduced the autocrlf issue and confirmed the fix. Keeping #130 open for the remaining Windows work.

KNambiarDJsc pushed a commit to KNambiarDJsc/Repo2RLEnv that referenced this pull request Sep 17, 2026
test_timeout_terminates_process_group_and_its_owned_containers mocks
both Popen and killpg to exercise execute()'s timeout/cleanup state
machine without touching a real OS. Neither os.killpg nor
signal.SIGKILL exists on Windows, so monkeypatch.setattr's default
raising=True failed before the fake was even installed.

execute() only ever runs for real inside a Linux remote worker
(REPO2RLENV_REMOTE_WORKER=1, killpg needs a POSIX process group), so
this doesn't change production code — it lets the existing coverage
of that logic run from a Windows host too, same shape as the
os.chown guards in huggingface#136.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants