[rominf]

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. — N/A, test-only.
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. — N/A.
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. — N/A, no message changes.

Summary

rocm unpacks downloaded archives in four places: the Lemonade embeddable (tar.gz and zip, in-process), the TheRock SDK tarball and the uv bootstrap (system tar), and the ComfyUI source (tar crate). This adds a property test for each, judged on the real filesystem.

Generator. Archives are written byte-by-byte (so tar::Builder cannot refuse the names) from an alphabet weighted toward .., . and empty components, mixed / and \ separators, absolute names, drive and UNC prefixes, NUL and non-UTF-8 bytes, long names, symlinks and hard links pointing out of the root, entries written through an earlier symlink, and setuid/setgid/world-writable modes.

Oracle. It does not re-derive the extractor's path checks. It snapshots everything around the extraction root before and after, and fails on anything created, modified or removed outside it, or any file sharing an inode with one outside. For extractors that promise more, it also walks the extracted tree without following links and fails on symlinks that resolve outside the root and on setuid/setgid or world-writable entries.

Properties that hold (regression guards):

Extractor	Contract checked	Cases
Lemonade tar.gz	nothing outside the root; no outward links; safe modes	512
Lemonade zip	same	512
Lemonade full install (prepare_embeddable_with)	same, and a failed install never leaves the version marker	256
TheRock SDK tarball (system tar)	nothing outside the root; safe modes	256
uv archive (system tar)	nothing outside the root; safe modes (two properties)	256 each

These found no defect in the extractors themselves.

Ignored reproducers, tied to #522. The same properties found defects in the steps that run after extraction. Those are filed as #522, and the tests that pin them are #[ignore]d with (#522) in the reason, so the fix for #522 only has to un-ignore them:

• ComfyUI: a top-level symlink in the source archive becomes the install's source/, and the next install write lands outside the app root (three tests, including the shrunk one-entry case).
• ComfyUI: world-writable modes from the archive are kept.
• uv: the binary lookup follows a uv symlink out of staging and chmods the target; two -> . links make the lookup effectively never terminate.

Each still fails on this base when run with --ignored.

Is the harness able to fail?

• Calibration. Two tests run the same property against deliberately unsafe extractors: one that joins raw names (catches traversal), and one that sanitises names but writes through planted links (catches the link-then-write shape only). Both must be caught.
• Generator reach. Every property reports, and asserts a 5% floor for, how often each dangerous shape occurred. With the default seed, over 256 cases: .. 62%, absolute 37%, backslash 77%, outward symlink 73%, write-through-link 18%, hard link 16%, special mode 89%, long name 18%, non-UTF-8 20%. The zip archives as actually stored by the zip writer: traversal/absolute names in 79%, symlinks in 78%. The full Lemonade install reaches a completed install in 53 of 256 hostile archives (20%), so the copy and marker steps are exercised, not only the rejection path.
• Mutation testing, each red against the property and restored afterwards:
  • Lemonade: honour .. in archive_entry_destination — the tar property fails (created outside the root).
  • Lemonade: mask modes with 0o7777 instead of 0o755 — the tar, zip and install properties all fail (setuid and world-writable entries).
  • uv: pass -P (--absolute-names) to the system tar — both uv properties fail; the calibration tests stay green.

Determinism, runtime, platforms

• The properties run from a fixed seed, so the reach floors cannot fail on seed luck and a failure reproduces as-is. PROPTEST_RNG_SEED picks another seed; PROPTEST_CASES raises the case count for a longer run.
• Runtime: about 4 s for the rocm-core group, 2–7 s for Lemonade, and 3 s for TheRock on a loaded machine.
• Unix-only (#[cfg(all(test, unix))]): the oracle reads mode bits, inode numbers and symlinks. Windows (bsdtar, Expand-Archive, drive-letter separators) is not exercised.
• Under plain cargo test, GNU tar prints its own rejections of the hostile names to stderr. nextest captures them.
• No proptest-regressions files: the runner does not persist failures, because the fixed seed already replays every case.

Implementation notes

• The generator and oracle live once in crates/rocm-core/src/archive_props.rs. apps/rocm and engines/lemonade include that file through #[path] instead of adding a test-support crate or a cross-crate dev-dependency, so no new crate edge is introduced.
• Dev-dependencies only: proptest uses rocm-core's existing spec ({ version = "1", default-features = false, features = ["std"] }), and flate2, tar and tempfile are already in the workspace. MANIFEST.md and the third-party notices are unchanged (cargo xtask manifest --check and cargo xtask tpn --check pass).

AGENTS.md §3

This is a test-only change: it changes no CLI output, exit code, file layout or runtime selection, so no Gherkin scenario is needed.

Verification

• cargo test --locked -p rocm-core -p rocm -p rocm-engine-lemonade: exit 0 (rocm-core 476 passed, 2 ignored; rocm 989 passed, 5 ignored; lemonade 123 passed)
• cargo clippy --locked --workspace --all-targets -- -D warnings: exit 0
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings: exit 0
• cargo fmt --all --check, cargo xtask manifest --check, cargo xtask tpn --check, cargo xtask check-crate-edges: exit 0

Since the review (6cb4386…8cab6f95)

• Fence. Every case now runs at the bottom of a "fence" of 49 nested directories, deeper than any ../link chain the generator can build. Absolute names and link targets are re-rooted inside it, and the snapshot covers the whole fence. The harness can therefore no longer touch real host directories.
• Benign round trip. Each extractor (uv, TheRock, Lemonade tar and zip, ComfyUI) also gets a round trip that requires every file to land under its destination. Dropping -C from uv's or TheRock's tar now fails it.
• Modes. The fixture sets its own modes, and world-write is judged only while the umask clears it. Under euid 0 the system-tar properties skip mode checks and print why. All properties pass under umask 022, 000 and 077, and as namespace root.
• ComfyUI. Active escape and round-trip properties over archives without the top-level symlink Post-extraction steps follow symlinks out of the staging directory (ComfyUI source, uv bootstrap) #522 is about.
• Runtime. The fence makes each case slower locally: about 20 s for TheRock + ComfyUI on this overlay filesystem.

The reach and mutation figures earlier in this description predate these changes. The PR conversation has the current figures.

[jussielo-amd]

Really nice piece of test engineering. The shared generator/oracle in archive_props.rs judges extractors on the real filesystem instead of re-deriving their path logic, and the calibration tests (catching a naive join-based extractor and a sanitize-but-follow-planted-links one) give good confidence the harness can actually catch what it claims to catch. I ran the full suite plus clippy/fmt/manifest/crate-edges locally and everything is green, and I also ran the #[ignore]d tests directly — both uv ones and all four ComfyUI ones fail exactly as described, so the regression pins against #522 are real, not aspirational.

No blocking issues. One thing worth a follow-up (not for this PR): Windows extraction paths (bsdtar, Expand-Archive) aren't exercised by any of this, which the PR description already calls out honestly — just flagging so it doesn't get lost.

Nice work on the mutation-testing writeup and reach-floor asserts too — that's the kind of detail that makes a property test suite trustworthy rather than decorative.

[siloteemu]

🔴 Automated review · pr-review-watcher · dc37664

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
Adds archive-extraction property tests and pins the post-extraction defects from #522 with ignored tests. It does not fix #522 and does not claim to close it.

Blocking

• The oracle only watches base, so writes beyond it go unseen, and the hostile generator can make real changes on the host — crates/rocm-core/src/archive_props.rs:434-450, :697-741, :847-859, :96-104, :69-85

  • What the code does. run_case snapshots only layout.base (snapshot_outside(&layout.base, …)). The generator does emit names that reach past it: up to four .. components after a /, @OUT/ or l/ prefix, and links to ../../outside and /.
  • Missed escape, confirmed by a run. I removed .arg("-C").arg(target_dir) from extract_archive in uv.rs. Both uv_archive_extraction_* properties stayed green, and the hostile entries were written into crates/rocm-core/ in the source checkout. For example, C:\x, ..\..\x and an l/ tree with mode-0644 directories appeared there.
  • Host change, confirmed by a run. With PROPTEST_RNG_SEED=1, which the PR documents as the way to pick another seed, the active test generator_catches_a_traversing_extractor changed the mode of the directory two levels above TMPDIR to 0644. I ran with TMPDIR nested in a scratch tree so I could observe it. With the default TMPDIR=/tmp that path is /. The default seed caused no such change in my run.
  • Absolute names. naive_unpack does destination.join(raw), and for a raw name starting with / that join resolves to the real filesystem root. Run as root (for example in a CI container), the calibration writes there. This part is read from the code, not run.
  • The description says otherwise. It states that the oracle "snapshots everything around the extraction root before and after, and fails on anything created, modified or removed outside it". The run above shows that does not hold.
  • Confidence 100 · architectural
  • Fix:
    • Nest the extraction root deep enough inside the tempdir that no generated .. chain or link can leave it, and snapshot the whole tempdir.
    • Make the naive calibration extractors re-root absolute names under the tempdir rather than at /.
    • Add a benign-archive check that expected content lands under dest. That catches a missing -C regardless of where the stray writes go.

• The mode checks depend on the runner's umask and uid, not on the code under test — crates/rocm-core/src/archive_props.rs:352-370, :497; apps/rocm/src/therock.rs (safe_modes: true); crates/rocm-core/src/uv.rs (uv_archive_extraction_leaves_no_setuid_or_world_writable_entry); engines/lemonade/src/install.rs (FULL)

  • umask 0, confirmed by a run. Layout::new creates dest with create_dir_all, and inspect_tree mode-checks the root. Under umask 0, every property with safe_modes failed: the uv modes test, the TheRock test, and all three Lemonade properties. The Lemonade failure names the harness's own directory: "/tmp/archive-props-…/dest is world-writable (40777)". That happens even though Lemonade masks modes correctly.
  • Running as root, not run. extract_archive and extract_tarball only run tar -xf … -C …. GNU tar preserves archived permissions by default when run as root, so the uv and TheRock "safe modes" properties would fail there with no change to the code. This part is from documented GNU tar behaviour; I could not run it, because user namespaces are unavailable on this host. #522 also lists root as not verified.
  • What is actually being checked. These tests check the environment's tar, umask and uid, not code in this repo. The description's table lists "safe modes" as a contract the system-tar paths hold.
  • Confidence 100 for umask, 70 for root · logic
  • Fix:
    • Have Layout::new set explicit modes on the directories it creates.
    • For the system-tar lanes, either make production pass --no-same-permissions and --no-same-owner, or skip the mode check when running as root and say why. Then narrow the description to match.

Non-blocking

• The ComfyUI unpack gets no active coverage — apps/rocm/src/comfyui.rs (all four tests in archive_properties are #[ignore]).
  • CI runs none of these tests, and I found no lane that runs --ignored. If #522 is fixed, or a test starts failing for a different reason, nothing will notice.
  • source_unpack_leaves_no_setuid_or_world_writable_entry fails for two independent reasons (the symlink and the modes), so it cannot be un-ignored when only one of them is fixed.
  • Confidence 85 · logic
  • Fix: add an active property for the parts that hold today, for example a generator without top-level symlinks plus a benign_entries round trip.
• base/victim cannot be reached, but the comment says an escape would touch it — crates/rocm-core/src/archive_props.rs:343, :363, :551
  • No component, prefix or link target in the generator produces victim, so its inode check and its snapshot entry can never fire.
  • Confidence 85 · mechanical
  • Fix: add victim and ../victim to the alphabet and the hard-link targets, or drop the fixture and correct the comment.
• benign_entries claims an "every extractor must accept" direction that only one test uses — crates/rocm-core/src/archive_props.rs:186-188
  • Only the Lemonade install property draws from it. For the tar, zip, uv and TheRock properties, an extractor that rejects everything still passes. I confirmed this for Lemonade tar and zip with a reject-all mutation: the properties stayed green, and older unit tests caught the tar case.
  • Confidence 85 · mechanical
  • Fix: use it in each property, or narrow the doc comment.

Decisions for the author

• Unix-only coverage leaves out the Windows extraction path — tradeoff
  • The Lemonade zip extractor is the production path on Windows, which is a supported platform. On Unix the generator's C:, \\srv\share and ..\..\x names are just ordinary bytes.
  • The description discloses the gap, and the oracle's mode, inode and symlink reads are what make it Unix-only.
  • An escapes-only cfg(windows) variant would cover the platform where those names actually mean something.
• #[path] includes the module across crates instead of using a test-support crate — tradeoff
  • There is precedent for this (apps/rocm/tests/build_metadata.rs), and it adds no crate edge.
  • The cost: three ../../../ paths, engines/lemonade reaching into rocm-core's source tree, and a blanket #![allow(dead_code)] that would hide future dead helpers in any of the three crates that include it.

Positive signals

• The calibration tests run the same property against a traversing extractor and a link-following one, and every property asserts per-shape reach floors. This guards against a property that passes only because the generator never produced the dangerous shapes.
• uv_binary_lookup_terminates_on_a_symlink_loop runs the lookup on a worker thread with a 20 s recv_timeout. Un-ignoring it fails the test instead of hanging CI.

Deployment notes

None

What this covered

• What was read. Every file in the change, in full: archive_props.rs, the new test modules in comfyui.rs, therock.rs, uv.rs and install.rs, the #[path] includes, and the manifest and lockfile changes. Also the production extractors and post-extraction steps those tests exercise.
• Commits. 830f3799…dc37664f. The branch's merge of the base was checked for conflict markers.
• Ran. Targeted cargo test for rocm-core, rocm-engine-lemonade and rocm; the calibration tests under nine seeds; all properties under umask 0; mutations to uv -C and to Lemonade tar and zip reject-all.
• Did not run:
  • Root-uid runs: no user namespaces on this host.
  • Workspace-wide test and clippy: the touched crates were tested instead.
  • Separate design-question workers for Step 7: the design questions were answered from the code passes' cross-file results.
  • Prior-changes pass: the history pass covered the touched regions, but review comments on earlier PRs were not read.
• CI. At the head commit, "E2E tests (MI300X)" was still queued. "Sphinx docs build (-W)", "Skill checks (skillscope)" and "Regenerate manifests" were skipped and reported nothing. All other checks reported success.
• Discussion. Not reconciled against any discussion on the PR.

[rominf]

Addressing the automated review on dc37664f, in 6cb43867…8cab6f95.

Oracle reach / host safety (blocking). Reproduced first: with the old layout, PROPTEST_RNG_SEED=1 chmodded the directory above a nested TMPDIR to 0644. The fix:

• Every case now runs at the bottom of a fence of 49 nested directories, deeper than any ../link chain the generator can build (the bound is spelled out in NESTING).
• Absolute names and link targets are re-rooted at the case's fake root, and the snapshot covers the whole fence.

The result: with seeds 1–9, as a normal user and under unshare -r, a nested scratch tree is unchanged. Each extractor also gets a benign round trip that requires every file to land under its destination, and dropping -C from uv's or TheRock's tar now fails it. The escape properties still don't see that case, because those writes land in the working directory.

Mode checks depend on the runner (blocking). Reproduced both: under umask 0 five properties failed, and under unshare -r the uv-modes and TheRock properties failed. The fix:

• Layout now sets explicit modes.
• World-write is judged only while the umask clears it.
• Under euid 0 the system-tar properties don't judge modes, and print why.
• Which checks run for a given umask and uid is a pure function with a table test.

Everything passes under umask 022, 000 and 077, and as namespace root. The Lemonade 0o7777/0o777 mask mutants are still caught.

Non-blocking:

• ComfyUI: new active escape and round-trip properties over archives without the top-level symlink Post-extraction steps follow symlinks out of the staging directory (ComfyUI source, uv bootstrap) #522 is about. The ignored modes test now fails for the modes alone.
• victim unreachable: victim is now in the alphabet and ../victim in the link targets. A naive extractor reaches it in 7 of 256 cases.
• The benign_entries claim: now used by every extractor through a shared run_round_trip. Reject-all on Lemonade tar or zip now fails.

Decisions:

• Windows: agreed, it's a gap. The oracle reads modes, inodes and symlinks, so a Windows variant would be an escapes-only oracle over the Expand-Archive/bsdtar paths. That fits better as a follow-up with a Windows lane to run it than landing untested here.
• #[path] include: kept. It adds no crate edge, with precedent in apps/rocm/tests/build_metadata.rs. The allow(dead_code) is an inner attribute of this one file, so it can only hide dead helpers in archive_props.rs itself.

All six #522 reproducers still fail under --ignored, on this branch and on a merge with current main. The touched crates pass on that merge, so no rebase was needed.

Verified locally: fmt, both clippy invocations, rocm, rocm-core and lemonade tests, xtask, and a Windows-target cargo check with -D warnings.

Left for later, non-blocking:

• PROPTEST_CASES=abc prints the minimum-cases message instead of a parse error.
• The combination of the umask/uid table into pass/fail has no test of its own.
• The unreadable-symlink branch is untested.

Stale: approved 0e78aa4; head is now 8cab6f9. The automated review's findings were addressed in 6cb4386…8cab6f95: a fence that keeps hostile archives inside the watched tree, runner-independent mode checks, and active ComfyUI coverage. A fresh look is needed on the current head.

[rominf]

Rebased onto main 0573d7fb; the head is now 3190c9a7. #562 also changed the proptest comment in apps/rocm/Cargo.toml. Main now declares proptest there itself, so this PR adds only tempfile to the existing [dev-dependencies] table. 3190c9a7 merges the table this branch used to add into main's, so there is one table. Every other line of the diff is unchanged.

Verified locally, all exiting 0:

• fmt
• tests for rocm --bin (1043), rocm-core (605), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e
• both clippy invocations
• a Windows-target cargo check -D warnings

[jussielo-amd]

I read this PR's full diff against the merge-base with origin/main (cfacd7a..3190c9a) and re-verified it against both prior review rounds rather than reviewing cold.

Prior rounds: jussielo-amd's hands-on review at 0e78aa4 approved the design outright (shared generator/oracle, calibration tests, mutation-testing writeup), flagging only the disclosed Windows-coverage gap as a non-blocking follow-up. The automated pr-review-watcher review at dc37664 found two real blocking issues, which I independently re-verified against the current code rather than trusting the dismissal: (1) the oracle only snapshotted layout.base, so a hostile ../absolute name or an extractor missing -C could write to the real host or just outside the watched directory unnoticed, and (2) the "safe modes" properties were actually asserting the runner's umask/uid rather than the extractor's behavior. Both are fixed at current HEAD. run_case now snapshots layout.top, the whole bottom-nested Fence (archive_props.rs:890-905, :437-471), sized so no .. chain or link target the generator can build (NESTING, archive_props.rs:67-81) reaches past it; absolute names and link targets are re-rooted inside the fence (archive_props.rs:298-319). Mode judgment now goes through mode_checks(Modes, umask, euid), read from /proc/self/status, skipping setuid/world-writable checks under euid 0 and judging world-writable only while the umask clears it (archive_props.rs:978-1030), backed by a table-driven unit test (mode_checks_follow_the_umask_and_the_uid, archive_props.rs:1314-1339). The three non-blocking items from that review are addressed too: victim/../victim are now reachable in the generator (archive_props.rs:119, 166), every extractor (uv, TheRock, ComfyUI, Lemonade tar+zip) now has a run_round_trip/benign_entries check that a reject-all extractor would fail (uv.rs:1367, therock.rs:1233, comfyui.rs:3221, install.rs:2047/2054), and ComfyUI now has two active (non-#[ignore]d) properties for what holds today, alongside the three still correctly #[ignore]d and tied to #522 (comfyui.rs:3207-3265).

I also ran a full review pass (code-review skill, high effort) and manually verified its findings rather than taking them at face value. None of its findings are production-code issues — the entire diff is #[cfg(test)]-only (lib.rs/main.rs additions at crates/rocm-core/src/lib.rs:30-31 and apps/rocm/src/main.rs:5-9 are just the #[path] test-module includes) — and none meet this repo's bar for a blocker. The one worth a closer look: entries_without_top_level_symlink (comfyui.rs:3120-3135) shifts every entry name one level down under a/ to keep #522's "write through a nested link" shape in play, but doesn't compensate the fixed single-.. literals in link_target() (archive_props.rs:164-166, e.g. ../outside), so a symlink that would have escaped at the original nesting depth now resolves one level short, inside dest. The syntactic reach-floor counter (Reach::symlink_out, archive_props.rs:794-801) only checks whether a symlink target contains ../is absolute, not whether it actually escaped, so this wouldn't show up as a floor failure; the always-escaping OUTSIDE_TOKEN-based absolute targets in the same strategy still exercise real escapes, so the property isn't defeated, just slightly less precise than the comment claims for that one shape. Worth a follow-up, not a blocker. The rest are minor: ESCAPES_ONLY is defined identically in both comfyui.rs:3199 and uv.rs:1352; CaseOutcome's tree.shared_inodes is always drained into escapes before being returned (archive_props.rs:949-950), so a consumer reading it off CaseOutcome.tree will always find it empty despite the field's own doc comment; and process_umask_and_euid (archive_props.rs:980) re-derives "is this euid 0" via /proc/self/status parsing instead of the existing running_as_root() in crates/rocm-core/src/openmpi.rs:177 (it still needs its own umask read, so this would only partially simplify).

Locally I ran cargo test --locked -p rocm-core -p rocm -p rocm-engine-lemonade (green, including the uv and rocm-core archive-property suites), cargo clippy --locked -p rocm-core -p rocm -p rocm-engine-lemonade --all-targets -- -D warnings (clean), cargo fmt --all --check (clean), and cargo xtask check-crate-edges (clean). CI on the PR is green across the board at this head commit, with only the MI300X GPU lane still pending.

This is a test-only change with no CLI-observable behavior, so no Gherkin scenario applies, and the checklist in the PR body is accurate. Approving.

[rominf]

Follow-ups are in #660:

• Moved symlinks: they get a compensating ../ on relative targets, so ../outside escapes again at the shifted depth. A test checks that the resolved target is unchanged by the move.
• ESCAPES_ONLY: defined once now.
• The tree doc: it now says shared_inodes is always moved into escapes.
• running_as_root(): not used. archive_props.rs is #[path]-included into crates where crate::openmpi doesn't exist, and the umask read from /proc is still needed.