emitter/bundle: record executable-intent explicitly, not via stat() - #158
Open
KNambiarDJsc wants to merge 2 commits into
Open
KNambiarDJsc wants to merge 2 commits into
KNambiarDJsc wants to merge 2 commits into
Conversation
added 2 commits
September 22, 2026 11:20
…-isolation fix huggingface#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 — huggingface#155. repo2rlenv migrate pr-diff <dir> [--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.
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 huggingface#130, confirmed to fail identically on unmodified main
This branch has not been deployed
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
1 participant
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
Implements the design you approved on #130: "recording executable intent explicitly makes sense for a separate PR. Please keep legacy bundle hashes unchanged, reject invalid or duplicate asset paths, and retain the POSIX chmod checks. Tests should cover old and new bundles on both platforms, including tampered manifests."
Root cause, precisely:
bundle_hash()(write side) always got a file's mode from the in-memoryTaskFile.executableflag — always correct.inspect_bundle()(verify/resume side) instead re-derived it fromitem.stat(). That round-trips on POSIX (chmod→stat), but NTFS has no POSIX execute bit for regular files at all, sostat()reports0o666unconditionally on Windows — not sometimes wrong, structurally incapable of carrying this information. Every bundle's integrity check was unconditionallyFalse(or raised) there.What this does:
tracked_files) and its executable subset (executable_files) intask.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 ofstat(). On POSIX it additionally still cross-checks the real mode against it — unchanged strictness, a realchmodtamper is still caught exactly as today.tracked_filesmarks 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 (test_legacy_bundle_hash_is_unchanged).tracked_filesandexecutable_filesare validated through the samerelative_asset_pathshape check every other asset uses, duplicates are rejected, andexecutable_filesmust be a subset oftracked_files.A real gap I found mid-implementation, not anticipated in the original proposal: the quality loop (
quality/loop/artifacts.py) appends files directly to an already-written bundle directory, then re-stampsbundle_hashby callinginspect_bundle()again. My first pass broke this — it treated any file not in the manifest as "must be non-executable," which raised on real executable files added this way. Fixed by tracking the complete originally-emitted file set (not just the executable subset), so anything added later by other tooling falls back to today's exact stat()-only leniency, unaffected either way. Caught this by running the full suite, not just this module's own tests — see test plan.Test plan
tests/test_task_bundle.py— 12 new tests: golden legacy-hash value, legacy bundle still round-trips, tracked/executable manifest recorded correctly, POSIX chmod-tamper still caught (both for tracked files and for a manifest-tamper-only case), Windows-equivalent proof (realsys.platformforced towin32, every file's real mode forced to0o666, integrity still holds), content-tamper still caught even under simulated Windows, invalid/duplicateexecutable_filesentries rejected, and the quality-loop append-and-restamp pattern still worksmain→ 180 failed / 18 errors with this change, full suitetest_task_bundle.pynatively — everything passes excepttest_symlink_rejected, which fails identically on unmodifiedmain(confirmed viagit stash) — the separately-tracked symlink-privilege gap from Track native Windows controller and artifact portability #130, unrelated to this change and out of scope hereruff check ./ruff format --check .— cleanScope note
This PR is the file-modes item only. Symlinks (real product site:
execution/hub_asset_alias.py:100, plus several test-fixture cases) remain the one item from #130's "file modes, symlinks, and process cleanup" list still untouched — happy to pick that up next if useful.