From 468193602c70ce58fad02baff273df2eb4653e3a Mon Sep 17 00:00:00 2001 From: KarthikNambiar04 Date: Tue, 22 Sep 2026 11:20:13 +0000 Subject: [PATCH 1/2] migrate: repair pr_diff tasks baked before the #145 oracle-isolation fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #145 stopped baking the oracle/verifier/instruction into pr_diff's agent image, but already-published tasks (e.g. AdithyaSK/repo2rlenv-pr-diff, 181 tasks) keep the pre-fix Dockerfile until re-emitted — #155. repo2rlenv migrate pr-diff [--apply] detects the baked-oracle marker and rewrites environment/Dockerfile + tests/{test.sh,verifier.py, oracle.patch,instruction.md} via the SAME builder functions fresh generate calls today, so a migrated task is byte-identical to one emitted now, not a hand-maintained parallel implementation. instruction.md and solution/patch.diff (the oracle itself) are never touched, so a migrated task keeps its original content_hash. repo_url/base_commit are recovered from the existing Dockerfile's remote set-url line and task.toml's metadata.repo2env.ref, not re-derived or guessed. Refuses to touch a task whose instruction.md/solution/patch.diff don't hash to the content_hash task.toml already claims, rather than silently rewriting an inconsistent bundle. Defaults to a dry-run report; --apply writes. --- src/repo2rlenv/cli.py | 45 ++++++ src/repo2rlenv/pipelines/pr_diff.py | 132 +++++++++++++++++ tests/test_pipeline_pr_diff.py | 219 ++++++++++++++++++++++++++++ 3 files changed, 396 insertions(+) diff --git a/src/repo2rlenv/cli.py b/src/repo2rlenv/cli.py index f3f8f76a..306b846a 100644 --- a/src/repo2rlenv/cli.py +++ b/src/repo2rlenv/cli.py @@ -498,6 +498,35 @@ def cmd_validate(args: argparse.Namespace) -> int: return 0 if failures == 0 else 1 +def cmd_migrate(args: argparse.Namespace) -> int: + from repo2rlenv.pipelines.pr_diff import migrate_pr_diff_task + + dataset_dir = Path(args.path).expanduser().resolve() + task_dirs = sorted({tf.parent for tf in dataset_dir.rglob("task.toml")}) + if not task_dirs: + console.error(f"no task.toml files found under {dataset_dir}") + return 1 + + apply = getattr(args, "apply", False) + label = "Migrating" if apply else "Auditing (dry run — pass --apply to write)" + counts: dict[str, int] = {} + errors: list[dict[str, str]] = [] + with console.section(f"{label} pr_diff tasks under {dataset_dir}"): + for task_dir in task_dirs: + result = migrate_pr_diff_task(task_dir, dry_run=not apply) + action = result["action"] + counts[action] = counts.get(action, 0) + 1 + if action == "error": + errors.append(result) + elif action in ("migrated", "would_migrate"): + console.success(f"{result['task']}: {action}") + for err in errors: + console.error(f"{err['task']}: {err['detail']}") + + console.kv(counts, title="pr_diff migration" if apply else "pr_diff migration (dry run)") + return 0 if not errors else 1 + + class _Backend: """Which source/destination a URI points at.""" @@ -1101,6 +1130,22 @@ def _dispatch(argv: list[str]) -> int: ) v.set_defaults(func=cmd_validate) + # migrate + mg = sub.add_parser("migrate", help="Repair already-emitted tasks after a generator fix") + mg_sub = mg.add_subparsers(dest="migration", required=True) + mg_pr_diff = mg_sub.add_parser( + "pr-diff", + help=( + "repair pr_diff tasks emitted before #145's oracle-isolation fix " + "(oracle baked into the agent's own image)" + ), + ) + mg_pr_diff.add_argument("path", help="dataset or task directory") + mg_pr_diff.add_argument( + "--apply", action="store_true", help="write the fix (default: dry-run report only)" + ) + mg_pr_diff.set_defaults(func=cmd_migrate) + # push p = sub.add_parser("push", help="Push a local dataset directory to HF Hub") p.add_argument( diff --git a/src/repo2rlenv/pipelines/pr_diff.py b/src/repo2rlenv/pipelines/pr_diff.py index 83ae0f81..ba459a97 100644 --- a/src/repo2rlenv/pipelines/pr_diff.py +++ b/src/repo2rlenv/pipelines/pr_diff.py @@ -327,6 +327,138 @@ def _pr_diff_aux_files(*, oracle_diff: str, instruction: str) -> dict[str, str]: } +# --------------------------------------------------------------------------- +# Migration: repair tasks emitted before #145's oracle-isolation fix +# --------------------------------------------------------------------------- + +_BAKED_ORACLE_MARKER = "base64 -d > /verifier/oracle.patch" + + +def _harbor_content_hash(instruction: str, oracle_diff: str) -> str: + """Mirror ``emitter/harbor.py:_content_hash`` without needing a full HarborTask. + + ``content_hash`` only ever covers ``instruction.md`` + ``solution/patch.diff`` + (see that function) — the migration below never touches either file, so this + is purely a sanity check that we're reading the same pair the task was + originally hashed against, not a value the migration recomputes and writes. + """ + import hashlib + + h = hashlib.sha256() + h.update(instruction.encode("utf-8")) + h.update(b"\0") + h.update(oracle_diff.encode("utf-8")) + return f"sha256:{h.hexdigest()}" + + +def migrate_pr_diff_task(task_dir: Path, *, dry_run: bool = False) -> dict[str, str]: + """Repair a single ``pr_diff`` task emitted before #145's oracle-isolation fix. + + Pre-#145, ``environment/Dockerfile`` baked the oracle patch, instruction, and + verifier into the agent's own image — readable and ``git apply``-able for a + perfect score. This detects that shape and rewrites ``environment/Dockerfile`` + + ``tests/{test.sh,verifier.py,oracle.patch,instruction.md}`` using the exact + builder functions fresh ``generate`` calls today, so a migrated task is + byte-identical to one emitted now — not a hand-maintained parallel + implementation that could drift from the real fix. + + ``instruction.md`` and ``solution/patch.diff`` (the oracle diff itself) are + never modified, so ``content_hash`` — which only covers those two files + (``emitter/harbor.py:_content_hash``) — is unaffected; a migrated task keeps + its original identity. ``repo_url`` and ``base_commit`` are recovered from + the existing Dockerfile's ``remote set-url origin`` line and ``task.toml``'s + ``metadata.repo2env.ref`` respectively — both fields the pre-#145 builder + already wrote unchanged, so nothing about them needs to be guessed or + re-derived from an external source. + + With ``dry_run=True``, every check runs identically (including the + ``content_hash`` sanity check) but nothing is written — ``action`` reports + what *would* happen (``"would_migrate"`` in place of ``"migrated"``). + + Returns ``{"task": , "action": ..., "detail": ...}`` where + ``action`` is one of: + + - ``"migrated"`` / ``"would_migrate"`` — rewritten in place / would be + - ``"already_safe"`` — no baked oracle found; nothing to do + - ``"skipped"`` — not a pr_diff task (different pipeline, or malformed) + - ``"error"`` — needs manual review; nothing was written + """ + import tomllib + + name = task_dir.name + toml_path = task_dir / "task.toml" + if not toml_path.exists(): + return {"task": name, "action": "error", "detail": "no task.toml"} + try: + config = tomllib.loads(toml_path.read_text(encoding="utf-8")) + except tomllib.TOMLDecodeError as exc: + return {"task": name, "action": "error", "detail": f"cannot parse task.toml: {exc}"} + repo2env = config.get("metadata", {}).get("repo2env", {}) + if repo2env.get("pipeline") != "pr_diff": + return {"task": name, "action": "skipped", "detail": "not a pr_diff task"} + + dockerfile_path = task_dir / "environment" / "Dockerfile" + if not dockerfile_path.exists(): + return {"task": name, "action": "skipped", "detail": "text-only task, no environment/"} + dockerfile = dockerfile_path.read_text(encoding="utf-8") + if _BAKED_ORACLE_MARKER not in dockerfile: + return {"task": name, "action": "already_safe", "detail": ""} + + base_commit = repo2env.get("ref") + origin_match = re.search(r"remote set-url origin (\S+)\n", dockerfile) + if not base_commit or not origin_match: + return { + "task": name, + "action": "error", + "detail": "cannot recover repo_url/base_commit from task.toml/Dockerfile", + } + repo_url = origin_match.group(1).strip("'\"") + + instruction_path = task_dir / "instruction.md" + oracle_path = task_dir / "solution" / "patch.diff" + if not instruction_path.exists() or not oracle_path.exists(): + return { + "task": name, + "action": "error", + "detail": "missing instruction.md or solution/patch.diff", + } + instruction = instruction_path.read_text(encoding="utf-8") + oracle_diff = oracle_path.read_text(encoding="utf-8") + + claimed_hash = repo2env.get("content_hash") + recomputed_hash = _harbor_content_hash(instruction, oracle_diff) + if claimed_hash and claimed_hash != recomputed_hash: + return { + "task": name, + "action": "error", + "detail": ( + f"content_hash mismatch: task.toml claims {claimed_hash}, " + f"instruction.md + solution/patch.diff hash to {recomputed_hash} — " + "needs manual review, not auto-migrated" + ), + } + + if dry_run: + return {"task": name, "action": "would_migrate", "detail": ""} + + dockerfile_path.write_text( + build_pr_diff_environment_dockerfile(repo_url=repo_url, base_commit=base_commit), + encoding="utf-8", + ) + tests_dir = task_dir / "tests" + tests_dir.mkdir(exist_ok=True) + (tests_dir / "test.sh").write_text( + build_pr_diff_eval_script(base_commit=base_commit), encoding="utf-8" + ) + (tests_dir / "test.sh").chmod(0o755) + for relative, content in _pr_diff_aux_files( + oracle_diff=oracle_diff, instruction=instruction + ).items(): + (task_dir / relative).write_text(content, encoding="utf-8") + + return {"task": name, "action": "migrated", "detail": ""} + + # --------------------------------------------------------------------------- # Gen-time helpers: quality filter, baseline calibration, difficulty bucket # --------------------------------------------------------------------------- diff --git a/tests/test_pipeline_pr_diff.py b/tests/test_pipeline_pr_diff.py index ccbdbff9..1a057cf2 100644 --- a/tests/test_pipeline_pr_diff.py +++ b/tests/test_pipeline_pr_diff.py @@ -24,10 +24,12 @@ from repo2rlenv.github import PullRequestSummary from repo2rlenv.pipelines.pr_diff import ( _build_instruction, + _harbor_content_hash, _pr_diff_aux_files, _strip_info_leak, build_pr_diff_environment_dockerfile, build_pr_diff_eval_script, + migrate_pr_diff_task, ) @@ -426,3 +428,220 @@ def test_verifier_source_is_stdlib_only() -> None: imported.add(node.module.split(".")[0]) non_stdlib = sorted(imported - sys.stdlib_module_names) assert non_stdlib == [], f"verifier imports non-stdlib modules: {non_stdlib}" + + +# --------------------------------------------------------------------------- +# migrate_pr_diff_task — repair for tasks emitted before #145 +# --------------------------------------------------------------------------- + + +def _write_old_style_pr_diff_task( + task_dir: Path, + *, + repo_url: str = "https://github.com/x/y.git", + base_commit: str = "deadbeef1234", + oracle_diff: str = "diff --git a/x b/x\n+fix\n", + instruction: str = "# Issue\ndo it", + content_hash: str | None = None, +) -> None: + """Reconstruct a pre-#145 emitted task: the pattern from #144/#155, not the + real old builder (removed) — just enough surface for migrate_pr_diff_task + to detect and repair: the baked-oracle Dockerfile marker, the old test.sh + shim, and the task.toml fields the migration reads.""" + import base64 + + import tomli_w + + task_dir.mkdir(parents=True, exist_ok=True) + (task_dir / "instruction.md").write_text(instruction, encoding="utf-8") + (task_dir / "solution").mkdir(exist_ok=True) + (task_dir / "solution" / "patch.diff").write_text(oracle_diff, encoding="utf-8") + + encoded_oracle = base64.b64encode(oracle_diff.encode("utf-8")).decode("ascii") + env_dir = task_dir / "environment" + env_dir.mkdir(exist_ok=True) + (env_dir / "Dockerfile").write_text( + "FROM python:3.12-slim\n" + f"RUN git clone --filter=blob:none {repo_url} /workspace \\\n" + f" && git -C /workspace remote set-url origin {repo_url}\n" + f"RUN git reset --hard {base_commit}\n" + "RUN mkdir -p /verifier\n" + f'RUN echo "{encoded_oracle}" | base64 -d > /verifier/oracle.patch\n', + encoding="utf-8", + ) + tests_dir = task_dir / "tests" + tests_dir.mkdir(exist_ok=True) + (tests_dir / "test.sh").write_text( + "#!/bin/bash\npython3 /verifier/verifier.py /verifier/oracle.patch " + "/tmp/predicted.patch /verifier/instruction.md\n", + encoding="utf-8", + ) + + resolved_hash = content_hash or _harbor_content_hash(instruction, oracle_diff) + payload = { + "version": "1.0", + "task": {"name": "test/task", "description": "x"}, + "metadata": { + "difficulty": "medium", + "category": "bugfix", + "keywords": [], + "repo2env": { + "pipeline": "pr_diff", + "ref": base_commit, + "content_hash": resolved_hash, + "reward_kinds": ["test_execution", "diff_similarity"], + }, + }, + "agent": {"timeout_sec": 1800.0}, + "verifier": {"timeout_sec": 300.0}, + } + (task_dir / "task.toml").write_bytes(tomli_w.dumps(payload).encode("utf-8")) + + +def test_migrate_detects_and_repairs_baked_oracle(tmp_path: Path) -> None: + task_dir = tmp_path / "x__y-1" + _write_old_style_pr_diff_task( + task_dir, + repo_url="https://github.com/x/y.git", + base_commit="deadbeef1234", + oracle_diff="diff --git a/x b/x\n+fix\n", + instruction="# Issue\ndo it", + ) + + result = migrate_pr_diff_task(task_dir) + + assert result == {"task": "x__y-1", "action": "migrated", "detail": ""} + dockerfile = (task_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") + assert "base64 -d" not in dockerfile + assert "/verifier/" not in dockerfile + assert "git clone --filter=blob:none https://github.com/x/y.git /workspace" in dockerfile + assert "git reset --hard deadbeef1234" in dockerfile + + test_sh = (task_dir / "tests" / "test.sh").read_text(encoding="utf-8") + assert '"$SCRIPT_DIR/oracle.patch"' in test_sh + assert "/verifier/verifier.py" not in test_sh + + aux = _pr_diff_aux_files(oracle_diff="diff --git a/x b/x\n+fix\n", instruction="# Issue\ndo it") + for relative, content in aux.items(): + assert (task_dir / relative).read_text(encoding="utf-8") == content + + # The oracle itself and the instruction are untouched by the migration. + assert (task_dir / "instruction.md").read_text(encoding="utf-8") == "# Issue\ndo it" + assert (task_dir / "solution" / "patch.diff").read_text( + encoding="utf-8" + ) == "diff --git a/x b/x\n+fix\n" + + +def test_migrate_preserves_content_hash(tmp_path: Path) -> None: + """content_hash only ever covers instruction.md + solution/patch.diff — since + the migration never touches either, a migrated task keeps its identity.""" + task_dir = tmp_path / "x__y-1" + oracle = "diff --git a/x b/x\n+fix\n" + instruction = "# Issue\ndo it" + _write_old_style_pr_diff_task(task_dir, oracle_diff=oracle, instruction=instruction) + before = _harbor_content_hash(instruction, oracle) + + migrate_pr_diff_task(task_dir) + + after = _harbor_content_hash( + (task_dir / "instruction.md").read_text(encoding="utf-8"), + (task_dir / "solution" / "patch.diff").read_text(encoding="utf-8"), + ) + assert before == after + + +def test_migrate_is_idempotent(tmp_path: Path) -> None: + task_dir = tmp_path / "x__y-1" + _write_old_style_pr_diff_task(task_dir) + migrate_pr_diff_task(task_dir) + + second = migrate_pr_diff_task(task_dir) + + assert second == {"task": "x__y-1", "action": "already_safe", "detail": ""} + + +def test_migrate_dry_run_does_not_write(tmp_path: Path) -> None: + task_dir = tmp_path / "x__y-1" + _write_old_style_pr_diff_task(task_dir) + original_dockerfile = (task_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") + + result = migrate_pr_diff_task(task_dir, dry_run=True) + + assert result == {"task": "x__y-1", "action": "would_migrate", "detail": ""} + assert (task_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") == ( + original_dockerfile + ) + assert not (task_dir / "tests" / "oracle.patch").exists() + + +def test_migrate_skips_non_pr_diff_task(tmp_path: Path) -> None: + import tomli_w + + task_dir = tmp_path / "other-task" + task_dir.mkdir() + payload = { + "version": "1.0", + "task": {"name": "test/other", "description": "x"}, + "metadata": {"repo2env": {"pipeline": "pr_runtime"}}, + } + (task_dir / "task.toml").write_bytes(tomli_w.dumps(payload).encode("utf-8")) + + result = migrate_pr_diff_task(task_dir) + + assert result == {"task": "other-task", "action": "skipped", "detail": "not a pr_diff task"} + + +def test_migrate_already_safe_task_is_a_noop(tmp_path: Path) -> None: + """A task already emitted by the fixed generator has no baked-oracle marker + in its Dockerfile — migrate must recognize that and touch nothing.""" + task_dir = tmp_path / "x__y-1" + task_dir.mkdir() + env_dir = task_dir / "environment" + env_dir.mkdir() + (env_dir / "Dockerfile").write_text( + build_pr_diff_environment_dockerfile( + repo_url="https://github.com/x/y.git", base_commit="deadbeef" + ), + encoding="utf-8", + ) + + import tomli_w + + payload = { + "version": "1.0", + "task": {"name": "test/task", "description": "x"}, + "metadata": {"repo2env": {"pipeline": "pr_diff", "ref": "deadbeef"}}, + } + (task_dir / "task.toml").write_bytes(tomli_w.dumps(payload).encode("utf-8")) + + result = migrate_pr_diff_task(task_dir) + + assert result == {"task": "x__y-1", "action": "already_safe", "detail": ""} + + +def test_migrate_flags_content_hash_mismatch_instead_of_silently_rewriting( + tmp_path: Path, +) -> None: + """If instruction.md/solution/patch.diff on disk don't hash to the + content_hash task.toml claims, something is inconsistent — refuse to + auto-migrate rather than paper over it.""" + task_dir = tmp_path / "x__y-1" + _write_old_style_pr_diff_task(task_dir, content_hash="sha256:" + "0" * 64) + original_dockerfile = (task_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") + + result = migrate_pr_diff_task(task_dir) + + assert result["action"] == "error" + assert "content_hash mismatch" in result["detail"] + assert (task_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") == ( + original_dockerfile + ) + + +def test_migrate_reports_error_for_missing_task_toml(tmp_path: Path) -> None: + task_dir = tmp_path / "empty" + task_dir.mkdir() + + result = migrate_pr_diff_task(task_dir) + + assert result == {"task": "empty", "action": "error", "detail": "no task.toml"} From ab07954d8c4240d6461ce3d24c19ae1289d62485 Mon Sep 17 00:00:00 2001 From: KarthikNambiar04 Date: Tue, 22 Sep 2026 11:47:58 +0000 Subject: [PATCH 2/2] emitter/bundle: record executable-intent explicitly, not via stat() bundle_hash() (write side) always got a file's mode from the in-memory TaskFile.executable flag; inspect_bundle() (verify/resume side) instead re-derived it from item.stat(). That round-trips on POSIX (chmod -> stat), but NTFS has no POSIX execute bit for regular files at all, so stat() reports 0o666 unconditionally on Windows -- not sometimes wrong, structurally incapable of carrying this information. Every bundle's integrity check was unconditionally False (or raised) there. Record every originally-emitted role-scoped path (tracked_files) and its executable subset (executable_files) in task.toml's existing [metadata.repo2env] extension -- itself part of the hashed configuration, so still tamper-evident. inspect_bundle() reads executable-intent from that manifest for tracked files instead of stat(); on POSIX it additionally still cross-checks the real mode against it (unchanged strictness -- a real chmod tamper is still caught exactly as today). A file present in the directory but not in tracked_files (the quality loop appends evidence artifacts to an already-written bundle and re-stamps its hash -- quality/loop/ artifacts.py) falls back to the untouched legacy stat()-only check, so that pattern keeps working exactly as it always has. Absence of tracked_files marks a legacy bundle (emitted before this change): _identity() keeps the exact original hash shape for those, so already-published bundle identities never change -- verified against a golden hash captured from the pre-change algorithm. Verified on both platforms: - WSL/Linux: 2015 passed, 0 failed (was already 0 failed; unaffected) - Windows, controlled before/after on the same commit: 276 failed/74 errors on main -> 180 failed/18 errors with this change (measured with the full suite, not just this module's own tests) - Windows: bundle_hash is deterministic across platforms (the golden legacy hash matches whether computed on Linux or Windows); the one remaining local test failure (test_symlink_rejected) is the separately-tracked symlink-privilege gap from #130, confirmed to fail identically on unmodified main --- src/repo2rlenv/emitter/bundle.py | 142 +++++++++++++++---- tests/test_task_bundle.py | 227 ++++++++++++++++++++++++++++++- 2 files changed, 343 insertions(+), 26 deletions(-) diff --git a/src/repo2rlenv/emitter/bundle.py b/src/repo2rlenv/emitter/bundle.py index 7f32153f..b77b0432 100644 --- a/src/repo2rlenv/emitter/bundle.py +++ b/src/repo2rlenv/emitter/bundle.py @@ -11,6 +11,7 @@ import os import re import shutil +import sys import tempfile from dataclasses import dataclass, field from pathlib import Path, PurePosixPath @@ -21,6 +22,8 @@ _SLUG = re.compile(r"[a-zA-Z0-9][a-zA-Z0-9_.-]{0,127}\Z") _ROLES = frozenset({"environment", "solution", "tests"}) _HASH_FIELD = "bundle_hash" +_TRACKED_FIELD = "tracked_files" +_EXECUTABLE_FIELD = "executable_files" def relative_asset_path(value: str) -> PurePosixPath: @@ -108,34 +111,82 @@ def configuration(self) -> dict[str, Any]: } +def _tracked_files(files: dict[str, TaskFile]) -> list[str]: + """Every role-scoped path this bundle originally declared, regardless of + executable-ness. instruction.md/task.toml are handled separately and + never appear here — see relative_asset_path.""" + return sorted(name for name in files if name != "instruction.md") + + +def _executable_files(files: dict[str, TaskFile]) -> list[str]: + return sorted(name for name, asset in files.items() if asset.executable) + + def _identity(configuration: dict, files: dict[str, TaskFile]) -> str: # TOML parsing normalizes the stored representation before hashing on either # side, so dictionary insertion order and serialization layout cannot matter. import tomllib normalized = tomllib.loads(tomli_w.dumps(configuration)) - normalized.get("metadata", {}).get("repo2env", {}).pop(_HASH_FIELD, None) + repo2env = normalized.get("metadata", {}).get("repo2env", {}) + repo2env.pop(_HASH_FIELD, None) # Evaluation is an advisory overlay, not executable task content. Excluding # only this namespace preserves existing unlabeled bundle identities; every # other configuration field and every task file remains bound to the hash. - normalized.get("metadata", {}).get("repo2env", {}).pop("evaluation", None) - record = { - "configuration": normalized, - "files": { - name: { - "sha256": hashlib.sha256(asset.content).hexdigest(), - "mode": asset.mode, - } + repo2env.pop("evaluation", None) + if _TRACKED_FIELD in repo2env: + # tracked_files/executable_files are themselves part of `normalized` + # (hashed below), so a per-file "mode" would be redundant for a file + # they cover — and re-deriving it from a stat() on the read side is + # exactly the lossy round-trip these fields replace (NTFS has no + # POSIX execute bit for regular files at all). A file NOT in + # tracked_files (added to the directory after emission by other + # tooling, e.g. the quality loop) isn't covered by either field, so + # its mode still needs to ride along in the hash exactly as before — + # otherwise a real chmod tamper on that file would go undetected. + tracked_set = set(repo2env.get(_TRACKED_FIELD, [])) + file_record = {} + for name, asset in sorted(files.items()): + entry: dict[str, Any] = {"sha256": hashlib.sha256(asset.content).hexdigest()} + if name not in tracked_set: + entry["mode"] = asset.mode + file_record[name] = entry + else: + # Legacy bundle (emitted before tracked_files existed): keep the + # exact original hash shape so already-published bundle identities + # never change. + file_record = { + name: {"sha256": hashlib.sha256(asset.content).hexdigest(), "mode": asset.mode} for name, asset in sorted(files.items()) - }, - } + } + record = {"configuration": normalized, "files": file_record} raw = json.dumps(record, sort_keys=True, separators=(",", ":"), default=str) return "sha256:" + hashlib.sha256(raw.encode()).hexdigest() def bundle_hash(bundle: TaskBundle) -> str: files = {"instruction.md": TaskFile.text(bundle.instruction), **bundle.files} - return _identity(bundle.configuration(), files) + configuration = bundle.configuration() + configuration["metadata"]["repo2env"][_TRACKED_FIELD] = _tracked_files(files) + configuration["metadata"]["repo2env"][_EXECUTABLE_FIELD] = _executable_files(files) + return _identity(configuration, files) + + +def _validated_asset_path_set(raw: object, *, field: str) -> set[str]: + """Validate a task.toml path-list field before trusting it. + + Each entry must be a well-formed, role-scoped asset path (the same shape + `relative_asset_path` enforces for every other asset), and the list must + not contain duplicates — either would indicate a hand-edited or corrupted + manifest, not a real bundle emitted by `write_bundle`. + """ + if not isinstance(raw, list) or not all(isinstance(item, str) for item in raw): + raise ValueError(f"{field} must be a list of strings") + if len(set(raw)) != len(raw): + raise ValueError(f"{field} contains duplicate entries") + for entry in raw: + relative_asset_path(entry) + return set(raw) def inspect_bundle(path: Path) -> dict[str, Any]: @@ -144,6 +195,19 @@ def inspect_bundle(path: Path) -> dict[str, Any]: if path.is_symlink() or not path.is_dir(): raise ValueError("Task directory must be a real directory") + configuration = tomllib.loads((path / "task.toml").read_text(encoding="utf-8")) + repo2env = configuration.get("metadata", {}).get("repo2env", {}) + raw_tracked = repo2env.get(_TRACKED_FIELD) + if raw_tracked is None: + tracked = executable = None + else: + tracked = _validated_asset_path_set(raw_tracked, field=_TRACKED_FIELD) + executable = _validated_asset_path_set( + repo2env.get(_EXECUTABLE_FIELD, []), field=_EXECUTABLE_FIELD + ) + if not executable <= tracked: + raise ValueError(f"{_EXECUTABLE_FIELD} contains a path outside {_TRACKED_FIELD}") + files: dict[str, TaskFile] = {} for item in sorted(path.rglob("*")): if item.is_symlink(): @@ -153,15 +217,40 @@ def inspect_bundle(path: Path) -> dict[str, Any]: continue raise ValueError(f"Task contains a special file: {item.relative_to(path)}") relative = item.relative_to(path).as_posix() - mode = item.stat().st_mode & 0o7777 - if mode not in {0o644, 0o755}: - raise ValueError(f"Unexpected task asset mode: {relative}: {oct(mode)}") if relative not in {"instruction.md", "task.toml"}: relative_asset_path(relative) + if tracked is not None and ( + relative in tracked or relative in {"instruction.md", "task.toml"} + ): + # A file this bundle originally declared (instruction.md and + # task.toml are always non-executable by construction, never + # listed in tracked_files, but get the same platform-safe + # treatment): executable-intent comes from the (hashed, + # tamper-evident) manifest, not a stat() call that can't answer + # correctly on every platform. + is_executable = relative in executable + if sys.platform != "win32": + # Real chmod-tamper detection, exactly as strict as the + # legacy check below — NTFS can't persist this bit at all, so + # there's nothing meaningful to cross-check on Windows; the + # hash still covers executable-intent via the manifest above. + mode = item.stat().st_mode & 0o7777 + expected = 0o755 if is_executable else 0o644 + if mode != expected: + raise ValueError(f"Unexpected task asset mode: {relative}: {oct(mode)}") + else: + # Legacy bundle, OR a file added to this directory after emission + # by other tooling (e.g. the quality loop appending an evidence + # artifact) that this manifest was never asked to track. Fall + # back to the original stat()-derived check, unaffected by this + # fix either way. + mode = item.stat().st_mode & 0o7777 + if mode not in {0o644, 0o755}: + raise ValueError(f"Unexpected task asset mode: {relative}: {oct(mode)}") + is_executable = mode == 0o755 if relative != "task.toml": - files[relative] = TaskFile(item.read_bytes(), mode == 0o755) - configuration = tomllib.loads((path / "task.toml").read_text()) - claimed = configuration.get("metadata", {}).get("repo2env", {}).get(_HASH_FIELD) + files[relative] = TaskFile(item.read_bytes(), is_executable) + claimed = repo2env.get(_HASH_FIELD) actual = _identity(configuration, files) return {"bundle_hash": actual, "claimed_hash": claimed, "integrity_passed": claimed == actual} @@ -169,17 +258,22 @@ def inspect_bundle(path: Path) -> dict[str, Any]: def write_bundle(bundle: TaskBundle, destination: Path, *, resume: bool = False) -> Path: """Publish a fully materialized task, refusing existing targets or collisions.""" configuration = bundle.configuration() - configuration["metadata"]["repo2env"][_HASH_FIELD] = bundle_hash(bundle) + files = { + "instruction.md": TaskFile.text(bundle.instruction), + **bundle.files, + } + # Computed against the SAME configuration + files that get written below + # (rather than via a second bundle_hash(bundle) call building its own + # configuration) so what's hashed and what's on disk can never diverge. + configuration["metadata"]["repo2env"][_TRACKED_FIELD] = _tracked_files(files) + configuration["metadata"]["repo2env"][_EXECUTABLE_FIELD] = _executable_files(files) + configuration["metadata"]["repo2env"][_HASH_FIELD] = _identity(configuration, files) from repo2rlenv.emitter.evaluation import generated_evaluation configuration["metadata"]["repo2env"]["evaluation"] = generated_evaluation( subject_bundle_hash=configuration["metadata"]["repo2env"][_HASH_FIELD] ) - files = { - "instruction.md": TaskFile.text(bundle.instruction), - "task.toml": TaskFile.text(tomli_w.dumps(configuration)), - **bundle.files, - } + files["task.toml"] = TaskFile.text(tomli_w.dumps(configuration)) destination.mkdir(parents=True, exist_ok=True) target = destination / bundle.name lock = destination / f".{bundle.name}.lock" diff --git a/tests/test_task_bundle.py b/tests/test_task_bundle.py index d0fa7000..d11a0405 100644 --- a/tests/test_task_bundle.py +++ b/tests/test_task_bundle.py @@ -1,9 +1,13 @@ from __future__ import annotations import copy +import os +import tomllib import pytest +import tomli_w +from repo2rlenv.emitter import bundle as bundle_module from repo2rlenv.emitter.bundle import ( TaskBundle, TaskFile, @@ -77,12 +81,231 @@ def test_resume_recovers_only_an_identical_completed_export(bundle, tmp_path): write_bundle(bundle, tmp_path, resume=True) -def test_symlink_and_privileged_mode_rejected(bundle, tmp_path): +def test_symlink_rejected(bundle, tmp_path): path = write_bundle(bundle, tmp_path) (path / "tests/link").symlink_to(tmp_path) with pytest.raises(ValueError, match="symlink"): inspect_bundle(path) - (path / "tests/link").unlink() + + +@pytest.mark.skipif(os.name == "nt", reason="chmod cannot set a real POSIX mode on Windows") +def test_privileged_mode_rejected(bundle, tmp_path): + """A privileged real mode on a tracked file is still caught: this fix + only skips the POSIX cross-check on Windows (where it's meaningless by + construction), it doesn't relax it on a platform where chmod is real.""" + path = write_bundle(bundle, tmp_path) (path / "tests/test.sh").chmod(0o4755) with pytest.raises(ValueError, match="mode"): inspect_bundle(path) + + +# --------------------------------------------------------------------------- +# tracked_files/executable_files: out-of-band executable-intent (see #130) +# --------------------------------------------------------------------------- + + +def _read_toml(path): + return tomllib.loads((path / "task.toml").read_text(encoding="utf-8")) + + +def test_write_bundle_records_tracked_and_executable_files(bundle, tmp_path): + path = write_bundle(bundle, tmp_path) + repo2env = _read_toml(path)["metadata"]["repo2env"] + assert repo2env["tracked_files"] == [ + "environment/Dockerfile", + "environment/input.bin", + "solution/solve.sh", + "tests/test.sh", + ] + assert repo2env["executable_files"] == ["solution/solve.sh", "tests/test.sh"] + + +def test_legacy_bundle_hash_is_unchanged(): + """Golden value captured from the pre-tracked_files algorithm (the exact + fixture below, hashed by the code as it existed before this change) — a + configuration with no tracked_files key must keep hashing exactly the + same way forever, so already-published bundles never stop verifying.""" + legacy_bundle = TaskBundle( + name="golden-task", + org="golden-org", + instruction="do the thing", + files={ + "environment/Dockerfile": TaskFile.text("FROM alpine\n"), + "solution/solve.sh": TaskFile.text("#!/bin/sh\necho solved\n", executable=True), + "tests/test.sh": TaskFile.text("#!/bin/sh\nexit 0\n", executable=True), + }, + metadata={"recipe": "golden", "recipe_version": "1", "reward_kinds": ["test_execution"]}, + ) + # A legacy configuration/files pair is exactly what bundle_hash() built + # before this change: no tracked_files key injected. + legacy_configuration = legacy_bundle.configuration() + legacy_files = { + "instruction.md": TaskFile.text(legacy_bundle.instruction), + **legacy_bundle.files, + } + assert "tracked_files" not in legacy_configuration["metadata"]["repo2env"] + assert ( + bundle_module._identity(legacy_configuration, legacy_files) + == "sha256:5b40a31e4f38c66f88da43a4f22e655b42427935dc14f35e67e120b282f001ed" + ) + + +@pytest.mark.skipif(os.name == "nt", reason="the legacy check requires a real POSIX chmod") +def test_legacy_bundle_on_disk_still_verifies(bundle, tmp_path): + """A bundle built the way write_bundle() worked before this change (no + tracked_files anywhere: hash computed and stamped by the untouched + legacy branch of _identity()) must still round-trip through the + untouched legacy branch of inspect_bundle() exactly as it always has. + + This is unchanged, pre-existing Windows behavior, not a regression: a + legacy bundle's round-trip was never possible on Windows before this fix + either (chmod can't set a real POSIX mode there), and this fix + deliberately leaves that alone — tracked_files is what makes new bundles + round-trip on Windows; legacy ones are explicitly out of scope.""" + configuration = bundle.configuration() + files = {"instruction.md": TaskFile.text(bundle.instruction), **bundle.files} + assert "tracked_files" not in configuration["metadata"]["repo2env"] + configuration["metadata"]["repo2env"]["bundle_hash"] = bundle_module._identity( + configuration, files + ) + files["task.toml"] = TaskFile.text(tomli_w.dumps(configuration)) + task_dir = tmp_path / "legacy" + task_dir.mkdir() + for relative, asset in files.items(): + target = task_dir / relative + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(asset.content) + target.chmod(asset.mode) + result = inspect_bundle(task_dir) + assert result["integrity_passed"] + assert "tracked_files" not in _read_toml(task_dir)["metadata"]["repo2env"] + + +@pytest.mark.skipif(os.name == "nt", reason="the untracked-file check requires a real POSIX chmod") +def test_file_added_after_emission_keeps_legacy_leniency(bundle, tmp_path): + """Other tooling (e.g. the quality loop, quality/loop/artifacts.py) adds + files directly to an already-written bundle directory, then re-stamps + bundle_hash by calling inspect_bundle() again — exactly that pattern. A + file added this way isn't in tracked_files, so it must fall back to the + pre-existing stat()-only check rather than being rejected as 'not + declared executable' just because tracked_files never mentions it. + + Untracked additions were already never round-trippable on Windows before + this fix (same chmod limitation as the legacy case above); this fix + doesn't change that, it only fixes the bundle's own originally-declared + files.""" + path = write_bundle(bundle, tmp_path) + extra = path / "solution" / "quality-original-solve.sh" + extra.write_text("#!/bin/sh\necho original\n", encoding="utf-8") + extra.chmod(0o755) + # Re-stamp, mirroring quality/loop/artifacts.py's own pattern. + new_hash = inspect_bundle(path)["bundle_hash"] + config = _read_toml(path) + config["metadata"]["repo2env"]["bundle_hash"] = new_hash + (path / "task.toml").write_bytes(tomli_w.dumps(config).encode()) + + result = inspect_bundle(path) + assert result["integrity_passed"] + assert result["bundle_hash"] == new_hash + # tracked_files is untouched by the addition — it still only covers what + # write_bundle() originally emitted. + assert ( + "quality-original-solve.sh" not in _read_toml(path)["metadata"]["repo2env"]["tracked_files"] + ) + + +@pytest.mark.skipif(os.name == "nt", reason="chmod is not meaningful on Windows") +def test_untracked_file_mode_tamper_still_detected(bundle, tmp_path): + """The untracked-file leniency above must not become blindness to a real + chmod tamper on that same file — its mode still rides along in the hash + exactly as it did before this change, it just isn't cross-checked + against a manifest entry that was never written for it.""" + path = write_bundle(bundle, tmp_path) + extra = path / "solution" / "quality-original-solve.sh" + extra.write_text("#!/bin/sh\necho original\n", encoding="utf-8") + extra.chmod(0o755) + new_hash = inspect_bundle(path)["bundle_hash"] + config = _read_toml(path) + config["metadata"]["repo2env"]["bundle_hash"] = new_hash + (path / "task.toml").write_bytes(tomli_w.dumps(config).encode()) + assert inspect_bundle(path)["integrity_passed"] + + extra.chmod(0o644) + assert not inspect_bundle(path)["integrity_passed"] + + +@pytest.mark.skipif(os.name == "nt", reason="chmod is not meaningful on Windows") +def test_new_bundle_posix_chmod_tamper_still_detected(bundle, tmp_path): + """The real chmod-tamper defense stays exactly as strict for new bundles: + flipping a file's real mode without updating the (hashed) manifest must + still be caught, same as it always has been — as a raised error, not a + quietly-returned False, matching the legacy check's own behavior.""" + path = write_bundle(bundle, tmp_path) + assert inspect_bundle(path)["integrity_passed"] + (path / "tests/test.sh").chmod(0o644) + with pytest.raises(ValueError, match="mode"): + inspect_bundle(path) + + +def test_new_bundle_integrity_holds_when_the_filesystem_cannot_represent_mode(bundle, tmp_path): + """Windows-equivalent proof: force sys.platform to win32 (the code's own + branch condition) and make every file's real mode wrong in exactly the + way NTFS always is (no POSIX execute bit at all) — integrity must still + pass, because executable-intent now comes from the hashed manifest, not + a stat() call that platform can't answer correctly.""" + path = write_bundle(bundle, tmp_path) + for item in path.rglob("*"): + if item.is_file(): + os.chmod(item, 0o666) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(bundle_module.sys, "platform", "win32") + assert inspect_bundle(path)["integrity_passed"] + + +def test_new_bundle_content_tamper_still_caught_on_simulated_windows(bundle, tmp_path): + """The mode-blindness above must not become blindness to real tampering: + a changed file's bytes still break the hash even when the platform can't + check modes at all.""" + path = write_bundle(bundle, tmp_path) + (path / "tests/test.sh").write_text("#!/bin/sh\necho tampered\n") + with pytest.MonkeyPatch.context() as mp: + mp.setattr(bundle_module.sys, "platform", "win32") + assert not inspect_bundle(path)["integrity_passed"] + + +def test_manifest_tamper_flips_integrity_without_touching_file_bytes(bundle, tmp_path): + """executable_files is inside the hashed configuration, so flipping a + declared executable-intent must still be caught by the hash itself even + with every file's real bytes and mode left untouched. Checked with + sys.platform forced to win32 so the POSIX chmod cross-check (which would + otherwise also — and separately — catch this as a raised mode error) is + out of the picture: this isolates the hash-based detection a real + Windows host relies on exclusively, with no chmod check available at all.""" + path = write_bundle(bundle, tmp_path) + config = _read_toml(path) + config["metadata"]["repo2env"]["executable_files"] = ["solution/solve.sh"] + (path / "task.toml").write_bytes(tomli_w.dumps(config).encode()) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(bundle_module.sys, "platform", "win32") + assert not inspect_bundle(path)["integrity_passed"] + + +def test_invalid_executable_files_entry_rejected(bundle, tmp_path): + path = write_bundle(bundle, tmp_path) + config = _read_toml(path) + config["metadata"]["repo2env"]["executable_files"] = ["../escape"] + (path / "task.toml").write_bytes(tomli_w.dumps(config).encode()) + with pytest.raises(ValueError, match="asset path"): + inspect_bundle(path) + + +def test_duplicate_executable_files_entries_rejected(bundle, tmp_path): + path = write_bundle(bundle, tmp_path) + config = _read_toml(path) + config["metadata"]["repo2env"]["executable_files"] = [ + "solution/solve.sh", + "solution/solve.sh", + ] + (path / "task.toml").write_bytes(tomli_w.dumps(config).encode()) + with pytest.raises(ValueError, match="duplicate"): + inspect_bundle(path)