From c7634fed41aeaaa2d1741cad56e5defb6022deb2 Mon Sep 17 00:00:00 2001 From: KNambiarDJsc Date: Wed, 16 Sep 2026 11:07:25 +0530 Subject: [PATCH] fix: Windows portability for os.chown and git-apply CRLF hermeticity Two mechanical fixes from the #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 #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. --- .../pipelines/recipes/swe_smith/grade.py | 11 +++++++++-- src/repo2rlenv/tasksmith/source_patch.py | 5 ++++- src/repo2rlenv/tasksmith/worker.py | 14 +++++++++++--- tests/test_repository_source_collection.py | 4 +++- tests/test_tasksmith_added_source.py | 3 ++- tests/test_tasksmith_verifier_selection.py | 3 ++- tests/test_verifier_editor_backups.py | 19 +++++++++++++++++-- 7 files changed, 48 insertions(+), 11 deletions(-) diff --git a/src/repo2rlenv/pipelines/recipes/swe_smith/grade.py b/src/repo2rlenv/pipelines/recipes/swe_smith/grade.py index 03a345b6..a803a55b 100644 --- a/src/repo2rlenv/pipelines/recipes/swe_smith/grade.py +++ b/src/repo2rlenv/pipelines/recipes/swe_smith/grade.py @@ -95,7 +95,13 @@ def validate_submission(workspace: Path, contract: dict) -> None: ): raise ValueError("Non-Python source assets must remain unchanged") if relative not in backups: - os.chown(path, 0, 0) + # os.chown doesn't exist on Windows. This module only ever runs for + # real inside the Linux verifier container (every other path here + # is an absolute container path); the guard exists so the tests + # that call validate_submission() directly can run on a Windows + # host without touching real container-only ownership semantics. + if sys.platform != "win32": + os.chown(path, 0, 0) path.chmod(0o644) # Only the verifier's collected copy is cleaned. The learner workspace and # archived submission retain their bytes. Validate every path first so a @@ -126,7 +132,8 @@ def main() -> None: return with tempfile.TemporaryDirectory(prefix="r2e-grade-") as temporary: working = Path(temporary) - os.chown(working, 1001, 1001) + if sys.platform != "win32": # os.chown doesn't exist on Windows; see above + os.chown(working, 1001, 1001) working.chmod(0o700) report = working / "results.xml" command = [ diff --git a/src/repo2rlenv/tasksmith/source_patch.py b/src/repo2rlenv/tasksmith/source_patch.py index 84130842..51b34761 100644 --- a/src/repo2rlenv/tasksmith/source_patch.py +++ b/src/repo2rlenv/tasksmith/source_patch.py @@ -84,7 +84,10 @@ def reverse_crlf_patch(root: Path, patch: Path) -> bool: adapted_path = directory / "source.diff" adapted_path.write_bytes(adapted) result = subprocess.run( - ["git", "apply", "--reverse", str(adapted_path.resolve())], + # core.autocrlf=false: `staged` isn't a git repository, so `git apply` + # would otherwise fall back to the caller's global config and risk + # re-normalizing the exact CRLF bytes this fallback just reconstructed. + ["git", "-c", "core.autocrlf=false", "apply", "--reverse", str(adapted_path.resolve())], cwd=staged, capture_output=True, text=True, diff --git a/src/repo2rlenv/tasksmith/worker.py b/src/repo2rlenv/tasksmith/worker.py index 9899811a..2705350f 100644 --- a/src/repo2rlenv/tasksmith/worker.py +++ b/src/repo2rlenv/tasksmith/worker.py @@ -211,9 +211,14 @@ def reverse_source(source: dict, base: Path, output: Path) -> tuple[Path, tuple[ path.parent.mkdir(parents=True, exist_ok=True) shutil.copyfile(base / relative, path) patch = output / "source.diff" - patch.write_text(source["source_diff"]) + patch.write_bytes(source["source_diff"].encode()) try: - run(["git", "apply", "--reverse", str(patch)], cwd=defective) + # -c core.autocrlf=false: `defective` isn't a git repository, so without + # an explicit override `git apply` falls back to the caller's global + # config. On a machine with the (very common, Git-for-Windows-default) + # `core.autocrlf=true`, that silently rewrites line endings while + # applying, corrupting the exact-byte reversal this function promises. + run(["git", "-c", "core.autocrlf=false", "apply", "--reverse", str(patch)], cwd=defective) except ValueError: if not reverse_crlf_patch(defective, patch): raise @@ -277,7 +282,10 @@ def construct(source: dict, profile: Profile, design: Design, ready: dict, outpu "Private supplements require an existing tests/ directory and an unused tasksmith_behavior.py path" ) local = output / "tasksmith_behavior.py" - local.write_text(design.additional_tests) + # write_bytes, not write_text: this file is re-read as raw bytes below and + # shipped verbatim into the bundle; write_text's newline translation would + # rewrite LF to the host's line separator (CRLF on Windows) on the way in. + local.write_bytes(design.additional_tests.encode()) additions[extra] = local options.test_selectors.append(extra) if "tests" not in options.test_paths: diff --git a/tests/test_repository_source_collection.py b/tests/test_repository_source_collection.py index e99487c2..f4c17912 100644 --- a/tests/test_repository_source_collection.py +++ b/tests/test_repository_source_collection.py @@ -25,7 +25,9 @@ def source_tree(tmp_path, monkeypatch): path = base / name path.parent.mkdir(parents=True, exist_ok=True) path.write_text(content) - monkeypatch.setattr("os.chown", lambda *args: None) + # raising=False: os.chown doesn't exist on Windows, unlike POSIX where + # this test's whole point is to neutralize it. + monkeypatch.setattr("os.chown", lambda *args: None, raising=False) return base diff --git a/tests/test_tasksmith_added_source.py b/tests/test_tasksmith_added_source.py index d0981e47..78b8e20e 100644 --- a/tests/test_tasksmith_added_source.py +++ b/tests/test_tasksmith_added_source.py @@ -72,7 +72,8 @@ def test_added_modules_are_absent_and_new_helpers_can_be_submitted(tmp_path, mon assert Task(task).config.artifacts[0].source == "/workspace/lib" contract = json.loads((task / "tests/contract.json").read_text()) workspace = task / "environment/source" - monkeypatch.setattr("os.chown", lambda *args: None) + # raising=False: os.chown doesn't exist on Windows. + monkeypatch.setattr("os.chown", lambda *args: None, raising=False) # Missing new APIs are left to behavioral assertions, not a collection exception. validate_submission(workspace, contract) (workspace / "lib/helper.py").write_text("value = 1\n") diff --git a/tests/test_tasksmith_verifier_selection.py b/tests/test_tasksmith_verifier_selection.py index a0773067..ffc16c6a 100644 --- a/tests/test_tasksmith_verifier_selection.py +++ b/tests/test_tasksmith_verifier_selection.py @@ -161,7 +161,8 @@ def test_image(image, options, output, **kwargs): monkeypatch.setattr(worker, "reverse_source", lambda *args: (defective, [])) monkeypatch.setattr(worker, "test_image", test_image) monkeypatch.setattr(worker, "test_excerpts", lambda *args, **kwargs: []) - monkeypatch.setattr("os.chown", lambda *args: None) + # raising=False: os.chown doesn't exist on Windows. + monkeypatch.setattr("os.chown", lambda *args: None, raising=False) output = tmp_path / "construct" output.mkdir() result = worker.construct( diff --git a/tests/test_verifier_editor_backups.py b/tests/test_verifier_editor_backups.py index cb3f2756..de7f15c6 100644 --- a/tests/test_verifier_editor_backups.py +++ b/tests/test_verifier_editor_backups.py @@ -3,6 +3,7 @@ import hashlib import os import shutil +import sys import pytest @@ -14,7 +15,8 @@ def submission(tmp_path, monkeypatch): workspace = tmp_path / "private" (workspace / "src").mkdir(parents=True) (workspace / "src/model.py").write_text("value = 1\n") - monkeypatch.setattr("os.chown", lambda *args: None) + # raising=False: os.chown doesn't exist on Windows. + monkeypatch.setattr("os.chown", lambda *args: None, raising=False) contract = {"submitted_files": ["src/model.py"], "submitted_roots": ["src"]} return workspace, contract @@ -48,7 +50,20 @@ def test_unknown_non_python_additions_still_reject_without_cleaning(submission, assert backup.read_text() == "previous\n" -@pytest.mark.parametrize("kind", ["symlink", "dangling_symlink", "oversized", "fifo"]) +@pytest.mark.parametrize( + "kind", + [ + "symlink", + "dangling_symlink", + "oversized", + pytest.param( + "fifo", + marks=pytest.mark.skipif( + sys.platform == "win32", reason="os.mkfifo doesn't exist on Windows" + ), + ), + ], +) def test_backup_paths_keep_regular_file_and_symlink_guards(submission, tmp_path, kind): workspace, contract = submission backup = workspace / "src/model.py.bak"