fix: Windows portability for os.chown and git-apply CRLF hermeticity - #136
Merged
Merged
Conversation
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.
Collaborator
|
Thanks, tested and merged. Reproduced the autocrlf issue and confirmed the fix. Keeping #130 open for the remaining Windows work. |
3 tasks
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two of the mechanical root causes from the #130 Windows-portability triage (#2 and #4):
#2 —
os.chowndoesn't exist on Windows.grade.py's two call sites now guard onsys.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 callvalidate_submission()directly run on a Windows host. Four test filesmonkeypatch.setattr("os.chown", ...)to neutralize the call during unit tests; switched toraising=Falseso the patch itself doesn'tAttributeErroron a platform where the attribute never existed.#4 —
git apply --reverseisn't hermetic against the caller's git config.reverse_source()andreverse_crlf_patch()both rungit applyagainst plain directories (not git repositories), so without an explicit override they silently inherit the caller's global git config. On a machine withcore.autocrlf=true— the Git-for-Windows installer default —git applywas rewriting LF to CRLF while applying, corrupting the exact-byte reversal both functions promise and breaking the dedicatedreverse_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=falseexplicitly.While tracking that down I also found two
write_text()calls doing the same kind of damage independently ofgit 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 withwrite_text(), whose newline translation rewrites\nto the host line separator on the way in. Switched both towrite_bytes().Also skips
test_verifier_editor_backups.py's"fifo"parametrize case onwin32:os.mkfifohas 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 --checkon all touched files — clean.os.chown/os.mkfifo/ CRLF-patch-apply failures — none; the only matches left are my own guard code and the one intentional skip.uv sync --group dev --all-extras --frozen+uv run --all-extras pytest -q, matching CI): 1871 passed, 0 failed — including thefifoand symlink cases this skip doesn't touch on that platform.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.