diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d69a245..00d5002 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,6 +24,10 @@ env: CI: "1" jobs: + windows: + name: Windows compatibility + uses: ./.github/workflows/windows.yml + lint: name: Lint (ruff) runs-on: ubuntu-latest @@ -160,7 +164,7 @@ jobs: build: name: Build sdist + wheel runs-on: ubuntu-latest - needs: [lint, test, owned-contracts, tasksmith-runtimes] + needs: [lint, test, owned-contracts, tasksmith-runtimes, windows] steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1f908b1..04abeb6 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -31,6 +31,12 @@ concurrency: cancel-in-progress: false jobs: + windows: + name: Windows compatibility + uses: ./.github/workflows/windows.yml + with: + ref: ${{ github.event.release.tag_name || inputs.tag }} + # Sanity gate — run the full test suite on the release tag before we publish. test: name: Tests on release tag (py${{ matrix.python-version }}) @@ -75,7 +81,7 @@ jobs: build: name: Build sdist + wheel runs-on: ubuntu-latest - needs: [test, tasksmith-runtimes] + needs: [test, tasksmith-runtimes, windows] outputs: version: ${{ steps.read-version.outputs.version }} steps: diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml new file mode 100644 index 0000000..1917b48 --- /dev/null +++ b/.github/workflows/windows.yml @@ -0,0 +1,44 @@ +name: Windows compatibility + +on: + workflow_call: + inputs: + ref: + description: Commit or release tag to validate + required: false + type: string + +permissions: + contents: read + +jobs: + wheel: + name: Windows wheel (py${{ matrix.python-version }}) + runs-on: windows-latest + strategy: + fail-fast: false + matrix: + python-version: ["3.12", "3.13", "3.14"] + env: + PYTHON_DOTENV_DISABLED: "1" + NO_COLOR: "1" + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + ref: ${{ inputs.ref }} + - uses: astral-sh/setup-uv@20cfd1bf945f4377ade1205e4dbc17946fc9a30d # v10.0.1 + with: + python-version: ${{ matrix.python-version }} + enable-cache: false + - run: uv build + - name: Install base wheel in isolation + run: | + uv venv "$env:RUNNER_TEMP/cli-check" + $wheel = (Get-ChildItem dist/*.whl).FullName + uv pip install --python "$env:RUNNER_TEMP/cli-check/Scripts/python.exe" $wheel + - name: Check console entrypoint, discovery and native task validation + run: '& "$env:RUNNER_TEMP/cli-check/Scripts/python.exe" tests/check_cli_install.py' + - name: Check real Windows process locks + run: | + uv pip install --python "$env:RUNNER_TEMP/cli-check/Scripts/python.exe" pytest + & "$env:RUNNER_TEMP/cli-check/Scripts/python.exe" -m pytest -q tests/test_locking.py tests/test_posix_only_imports.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 97531bd..ab6e50b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,6 +75,7 @@ Closes #N - **lint** — `uv run ruff check .` + `uv run ruff format --check .` - **test** — `uv run pytest -q` against Python 3.12, 3.13, 3.14 (matrix) +- **Windows** — base-only wheel CLI smoke checks and real process-lock contention on Python 3.12, 3.13, 3.14; the same gate runs before publication. Full controller portability remains separate work. - **build** — `uv build` produces sdist + wheel, smoke-installs the wheel, checks `repo2rlenv --version` A green CI is the floor for merge — green plus at least one approving review is the ceiling. diff --git a/README.md b/README.md index 9587487..f163270 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,10 @@ and repair emitted tasks; exporting a task alone does not establish its quality. Requires **Python 3.12+** and Git. This example generates PR-diff tasks without building a container: +Windows CI covers CLI startup, recipe discovery, native task emission and static +validation. Use Linux, macOS or WSL for Tasksmith, research-recipe generation and +the quality controller; their full native Windows execution is not yet supported. + ```bash pip install repo2rlenv diff --git a/docs/quickstart.md b/docs/quickstart.md index c2f39a3..6bcc50e 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -21,6 +21,12 @@ hf auth login ## Install +Requires Python 3.12+. Windows CI checks the installed CLI, recipe discovery, +native task emission and static validation. Run Tasksmith, research-recipe +generation and the quality controller on Linux, macOS or WSL: those controllers +still depend on POSIX artifact permissions and process cleanup. Remote sandboxes +run Linux; choosing a cloud provider does not remove these host requirements. + ```bash pip install repo2rlenv # from PyPI # or: diff --git a/src/repo2rlenv/locking.py b/src/repo2rlenv/locking.py new file mode 100644 index 0000000..93b3356 --- /dev/null +++ b/src/repo2rlenv/locking.py @@ -0,0 +1,38 @@ +"""Process locks for controller receipts and shared worker checkouts.""" + +from __future__ import annotations + +import errno +import sys +import time +from typing import IO + + +def lock_file(handle: IO, *, blocking: bool = False) -> None: + """Exclusively lock an open file until it closes, without deleting its path. + + POSIX retains flock semantics, including interoperability with older + controllers. Windows locks byte zero; the region may extend beyond EOF, + so an empty lock file needs no write. All callers must keep the same path + and close their handle on success, failure or cancellation. + """ + if sys.platform != "win32": + import fcntl + + fcntl.flock(handle, fcntl.LOCK_EX | (0 if blocking else fcntl.LOCK_NB)) + return + + import msvcrt + + handle.seek(0) + while True: + try: + msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) + return + except OSError as error: + if error.errno not in {errno.EACCES, errno.EAGAIN, errno.EDEADLK}: + raise + if not blocking: + raise BlockingIOError(errno.EAGAIN, "Another process owns this lock") from error + # LK_LOCK gives up after ten retries; match flock's indefinite wait. + time.sleep(0.1) diff --git a/src/repo2rlenv/pipelines/recipes/history/worker.py b/src/repo2rlenv/pipelines/recipes/history/worker.py index b250014..a68eec2 100644 --- a/src/repo2rlenv/pipelines/recipes/history/worker.py +++ b/src/repo2rlenv/pipelines/recipes/history/worker.py @@ -3,7 +3,6 @@ from __future__ import annotations import argparse -import fcntl import hashlib import io import json @@ -16,6 +15,7 @@ from repo2rlenv.execution.lifecycle import save_record from repo2rlenv.execution.python_repository import bootstrap_snapshot, test_image +from repo2rlenv.locking import lock_file from repo2rlenv.pipelines.recipes.history.selection import check_entities, within from repo2rlenv.pipelines.recipes.history.test_suite import stage_tests from repo2rlenv.quality.python_evidence import test_excerpts @@ -131,7 +131,7 @@ def prepare(config: dict, destination: Path) -> dict: root = Path("/work/history") / hashlib.sha256(repo.url.encode()).hexdigest()[:16] root.parent.mkdir(parents=True, exist_ok=True) with root.with_suffix(".lock").open("a") as lock: - fcntl.flock(lock, fcntl.LOCK_EX) + lock_file(lock, blocking=True) if not root.exists(): temporary = root.with_name(root.name + "-" + uuid.uuid4().hex) try: diff --git a/src/repo2rlenv/quality/loop/runner.py b/src/repo2rlenv/quality/loop/runner.py index 9e8ca61..fa03484 100644 --- a/src/repo2rlenv/quality/loop/runner.py +++ b/src/repo2rlenv/quality/loop/runner.py @@ -2,7 +2,6 @@ from __future__ import annotations -import fcntl import hashlib import json import re @@ -15,6 +14,7 @@ from repo2rlenv.campaigns.budget import BudgetExceeded, BudgetLedger from repo2rlenv.campaigns.events import EventJournal, ProgressEvent from repo2rlenv.execution.lifecycle import save_record +from repo2rlenv.locking import lock_file from repo2rlenv.quality.loop.artifacts import ( EXPECTED_PASSES_CONTRACT, apply_repair, @@ -641,7 +641,7 @@ def run( self.directory.mkdir(parents=True, exist_ok=True) with (self.directory / ".lock").open("a") as lock: try: - fcntl.flock(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) + lock_file(lock) except BlockingIOError as exc: raise RuntimeError("Another controller owns this quality run") from exc try: diff --git a/src/repo2rlenv/tasksmith/batch.py b/src/repo2rlenv/tasksmith/batch.py index 9d074dc..68b9891 100644 --- a/src/repo2rlenv/tasksmith/batch.py +++ b/src/repo2rlenv/tasksmith/batch.py @@ -3,7 +3,6 @@ from __future__ import annotations import argparse -import fcntl import hashlib import json import os @@ -23,6 +22,7 @@ from repo2rlenv.campaigns.budget import BudgetLedger from repo2rlenv.execution.artifacts import check_runtime_wheel from repo2rlenv.execution.lifecycle import save_record +from repo2rlenv.locking import lock_file from repo2rlenv.quality.loop.artifacts import digest from repo2rlenv.quality.loop.client import RunBudget from repo2rlenv.quality.loop.models import LoopResult, ProbeManifest @@ -370,7 +370,7 @@ def run_batch( directory, campaign, wheel = directory.resolve(), campaign.resolve(), wheel.resolve() directory.mkdir(parents=True, exist_ok=True) with (directory / ".lock").open("a") as lock: - fcntl.flock(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) + lock_file(lock) prior = [verified_result(path) for path in plan.prior_verified] if len({item["url"] for item in prior}) != len(prior): raise ValueError("Prior verified results contain duplicate PRs") diff --git a/src/repo2rlenv/tasksmith/runner.py b/src/repo2rlenv/tasksmith/runner.py index caa0806..7490b6e 100644 --- a/src/repo2rlenv/tasksmith/runner.py +++ b/src/repo2rlenv/tasksmith/runner.py @@ -3,7 +3,6 @@ from __future__ import annotations import asyncio -import fcntl import hashlib import json import shlex @@ -27,6 +26,7 @@ save_record, stop_worker, ) +from repo2rlenv.locking import lock_file from repo2rlenv.quality.loop.artifacts import task_identity from repo2rlenv.quality.loop.client import RunBudget from repo2rlenv.quality.loop.models import ProbeManifest @@ -765,7 +765,7 @@ def run( raise ValueError("stop-after must be within the frozen panel size") self.directory.mkdir(parents=True, exist_ok=True) with (self.directory / ".lock").open("a") as lock: - fcntl.flock(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) + lock_file(lock) return self._run(panel, limit, generation_run, reuse_evidence, source_records) def _run(self, panel, limit, generation_run, reuse_evidence=False, source_records=None): diff --git a/tests/check_cli_install.py b/tests/check_cli_install.py new file mode 100644 index 0000000..ba8a5ae --- /dev/null +++ b/tests/check_cli_install.py @@ -0,0 +1,75 @@ +"""Smoke-test an installed CLI from outside the checkout, without cloud calls.""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import sysconfig +from importlib.metadata import version +from pathlib import Path +from tempfile import TemporaryDirectory + +from repo2rlenv.emitter.harbor import HarborTask, write_harbor_task +from repo2rlenv.ui import console + + +def check() -> dict: + executable = Path(sysconfig.get_path("scripts")) / ( + "repo2rlenv.exe" if sys.platform == "win32" else "repo2rlenv" + ) + with TemporaryDirectory(prefix="repo2rlenv CLI ") as directory: + root = Path(directory) + + def cli(*args: str, expected: int = 0) -> str: + result = subprocess.run( + [str(executable), *args], + cwd=root, + env={**os.environ, "PYTHON_DOTENV_DISABLED": "1", "PYTHONUTF8": "1"}, + capture_output=True, + encoding="utf-8", + timeout=60, + check=False, + ) + assert result.returncode == expected, result.stderr + result.stdout + return result.stdout + + assert version("repo2rlenv") in cli("--version") + assert "generate" in cli("--help") + for command in ( + "generate", + "validate", + "push", + "pull", + "tasksmith", + "quality", + "pipelines", + ): + assert "usage:" in cli(command, "--help") + listing = json.loads(cli("pipelines", "list", "--json")) + recipes = [item for item in listing["recipes"] if item["implemented"]] + for recipe in recipes: + detail = json.loads( + cli("pipelines", "describe", recipe["pipeline"], "--recipe", recipe["id"], "--json") + ) + assert detail["id"] == recipe["id"] + for command in ("tasksmith", "quality"): + error = json.loads(cli(command, "show", "missing.json", "--json", expected=2)) + assert set(error) == {"error", "message"} + task = HarborTask( + name="unicode-task", + org="smoke", + description="Handle café input", + instruction="# Task\n\nPreserve café and 日本語 in the output.\n", + oracle_diff="--- a/x.py\n+++ b/x.py\n@@ -1 +1 @@\n-1\n+2\n", + repo2env={"pipeline": "pr_diff", "pipeline_version": "0.1.0", "repo": "smoke/demo"}, + ) + output = write_harbor_task(task, root / "task output") + assert (output / "instruction.md").read_text(encoding="utf-8") == task.instruction + cli("validate", str(root / "task output"), "--deep") + return {"status": "passed", "version": version("repo2rlenv"), "recipes": len(recipes)} + + +if __name__ == "__main__": + console.json(check()) diff --git a/tests/test_locking.py b/tests/test_locking.py new file mode 100644 index 0000000..304720d --- /dev/null +++ b/tests/test_locking.py @@ -0,0 +1,113 @@ +"""Exercise real OS locks between independent processes, including on Windows.""" + +from __future__ import annotations + +import subprocess +import sys +from concurrent.futures import ThreadPoolExecutor +from contextlib import contextmanager +from pathlib import Path + +import pytest + +from repo2rlenv.locking import lock_file + +_OWNER = """ +import sys +from repo2rlenv.locking import lock_file +with open(sys.argv[1], 'a') as handle: + lock_file(handle) + sys.stdout.write('locked\\n') + sys.stdout.flush() + sys.stdin.read() +""" + + +def _line(process: subprocess.Popen) -> str: + with ThreadPoolExecutor(max_workers=1) as reader: + ready = reader.submit(process.stdout.readline) + try: + return ready.result(timeout=15) + except BaseException: + process.kill() + raise + + +@contextmanager +def _owner(path: Path, code: str = _OWNER): + process = subprocess.Popen( + [sys.executable, "-c", code, str(path)], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + try: + # A bounded handshake avoids a hanging test if the child cannot import. + assert _line(process) == "locked\n" + yield process + finally: + if process.poll() is None: + process.kill() + process.communicate(timeout=15) + + +def test_nonblocking_lock_and_release(tmp_path): + path = tmp_path / "controller.lock" + with _owner(path) as owner: + with path.open("a") as handle, pytest.raises(BlockingIOError): + lock_file(handle) + owner.communicate(timeout=15) + assert owner.returncode == 0 + with path.open("a") as handle: + lock_file(handle) + assert path.read_bytes() == b"" # An empty lock file needs no mutation. + + +def test_lock_released_after_process_death(tmp_path): + path = tmp_path / "controller.lock" + with _owner(path) as owner: + owner.kill() + owner.communicate(timeout=15) + with path.open("a") as handle: + lock_file(handle) + + +def test_blocking_lock_waits_for_owner(tmp_path): + path = tmp_path / "checkout.lock" + contender_code = _OWNER.replace("lock_file(handle)", "lock_file(handle, blocking=True)") + contender_code = contender_code.replace( + " lock_file(handle, blocking=True)", + " sys.stdout.write('attempting\\n')\n" + " sys.stdout.flush()\n" + " lock_file(handle, blocking=True)", + ) + contender_code = contender_code.replace(" sys.stdin.read()", "") + with _owner(path) as owner: + contender = subprocess.Popen( + [sys.executable, "-c", contender_code, str(path)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + try: + assert _line(contender) == "attempting\n" + with pytest.raises(subprocess.TimeoutExpired): + contender.communicate(timeout=0.5) + owner.communicate(timeout=15) + stdout, stderr = contender.communicate(timeout=15) + assert contender.returncode == 0, stderr + assert stdout == "locked\n" + finally: + if contender.poll() is None: + contender.kill() + contender.communicate(timeout=15) + + +@pytest.mark.skipif(sys.platform == "win32", reason="Interoperability with earlier POSIX releases") +def test_interoperates_with_existing_flock(tmp_path): + path = tmp_path / "controller.lock" + code = _OWNER.replace("from repo2rlenv.locking import lock_file", "import fcntl") + code = code.replace("lock_file(handle)", "fcntl.flock(handle, fcntl.LOCK_EX)") + with _owner(path), path.open("a") as handle, pytest.raises(BlockingIOError): + lock_file(handle) diff --git a/tests/test_posix_only_imports.py b/tests/test_posix_only_imports.py new file mode 100644 index 0000000..5d5d081 --- /dev/null +++ b/tests/test_posix_only_imports.py @@ -0,0 +1,51 @@ +"""No POSIX-only stdlib module is imported unconditionally at module level. + +`fcntl` (and pwd/grp/termios/tty/pty/posix/resource/crypt/nis/spwd) don't +exist on Windows. An unconditional `import fcntl` at the top of a module +crashes with `ModuleNotFoundError` the moment anything imports that module — +even just to register an argparse subcommand, never to call anything +POSIX-specific. `cli.py`'s dispatcher eagerly imports every subsystem just to +build `--help`, so one such import anywhere in that chain took down +`repo2rlenv --version` entirely (#128). This static check complements the +installed-wheel Windows CI checks and keeps new top-level imports explicit +about the platforms they need. + +Guard the import instead: `if sys.platform != "win32": import fcntl`. +""" + +from __future__ import annotations + +import ast +from collections.abc import Iterator +from pathlib import Path + +import repo2rlenv + +SRC = Path(repo2rlenv.__file__).parent + +_POSIX_ONLY = frozenset( + {"fcntl", "pwd", "grp", "termios", "tty", "pty", "posix", "resource", "crypt", "nis", "spwd"} +) + + +def _unconditional_posix_only_imports() -> Iterator[str]: + for path in sorted(SRC.rglob("*.py")): + tree = ast.parse(path.read_text(encoding="utf-8")) + # Only module.body (top level): an import inside an `if` guard, e.g. + # `if sys.platform != "win32": import fcntl`, is intentionally exempt. + for node in tree.body: + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name in _POSIX_ONLY: + where = f"{path.relative_to(SRC).as_posix()}:{node.lineno}" + yield f"{where} (import {alias.name})" + elif isinstance(node, ast.ImportFrom) and node.module in _POSIX_ONLY: + where = f"{path.relative_to(SRC).as_posix()}:{node.lineno}" + yield f"{where} (from {node.module} import ...)" + + +def test_no_unconditional_posix_only_imports(): + offenders = list(_unconditional_posix_only_imports()) + assert offenders == [], ( + f'guard with `if sys.platform != "win32":` before importing: {offenders}' + )