fix(runtime): compare paths by the folder they name, not by their text

[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. — no rows reference this area.
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. — N/A, no new subcommand.
• 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. — see "On the Gherkin requirement" below.

Fixes #490

Summary

runtime_install_root_is_protected decides whether a folder may be handed to fs::remove_dir_all. It compared path text, and off Windows the normalization applied is a no-op, so /etc/ and /etc were different folders. Nothing resolved . or ...

Measured against the real gate on main, ensure_runtime_install_root_is_safe_to_remove accepted /etc/, //etc, /./etc, /etc/., /usr/ and $HOME/../../etc, refusing only the exact spelling /etc. The guard's own comment says it exists for "a hand-edited or corrupted registry entry [pointing] install_root at a protected system location" — it failed at precisely that. A trailing slash needs no adversary; a dirname, a shell variable or a hand-edit produces one.

Both comparisons now resolve ., .., repeated separators and a trailing separator before comparing.

Three further consumers are fixed by the same change: chat_install_prefix_is_system, which gates an install prefix arriving as model-generated JSON; rocmd's system_prefix_requires_ack, which had stopped demanding acknowledgement for a .. path that escaped home; and the prune planner at apps/rocm/src/storage.rs:666, which is automatically correct post-fix because it delegates to runtime_install_root_is_protected.

Risk: medium. It changes a comparison used by several call sites. Every caller was enumerated and reviewed; the only behaviour changes are the intended ones. The failure mode of getting this wrong is severe, which is why it is pinned by properties rather than examples and why the review notes below are spelled out.

Non-obvious decisions

Resolution is lexical, not canonicalize. These paths frequently do not exist yet — they come from a registry entry — so canonicalize has nothing to resolve.

It is therefore not symlink-safe, and the code now says so. Because the home exemption is consulted first, a user-owned symlink is enough: with $HOME/link -> /etc, $HOME/link/child is lexically inside home, so the guard reports it removable while the kernel lands in /etc. This hole is identical before and after this PR. An earlier draft of this change claimed a symlink "can only land somewhere the lexical answer did not already call safe" — that is false, it was the argument for the whole design, and it is corrected in the comments rather than left for the next reader to inherit.

Folding \ to / stays gated on Windows. Off Windows a backslash is an ordinary filename byte. An unconditional fold plus the new .. resolution would rewrite a real directory named ..\tmp inside /usr into /usr/../tmp and walk it out of the protected root — turning this fix into a new bypass. Raw-text comparison tolerated the unconditional fold because equality never removed components; resolving .. does. The generator alphabet includes ..\tmp so the property suite holds that line.

Platform parameterization. The comparison helpers now take a platform instead of reading runtime_is_windows(), so Windows path rules are exercised from a Linux host. This covers the helpers only — the branch selecting the Windows protected-root list still reads runtime_is_windows() and remains Windows-lane-only. Parameterizing also required the containment test to compare bytes rather than &path[..n], which splits a non-ASCII path mid-character and panics.

Windows trade, stated deliberately. Direct respellings (C:/Windows/, c:/windows, C:/Windows//System32) are now caught. Conversely C:\Windows\..\ProgramData — previously refused only because the text walk matched C:\Windows as a literal ancestor — now correctly resolves to C:/ProgramData and is accepted, because that path is not on the list. The three-entry Windows list omits the drive root, C:/Users and C:/ProgramData and hardcodes drive C: and English names. That is an incomplete policy which was previously masked by the ancestor-walk bug; widening it is a separate change and is not attempted here.

Test plan

Properties, not more examples — the defect's nature was that the examples pinned the spellings somebody thought of. The reference oracle is derived from POSIX semantics rather than restated from the implementation, so it does not inherit the implementation's mistakes.

Verified against the real gate, before and after:

spelling	before	after
/etc/, //etc, /./etc, /etc/., /usr/	accepted for deletion	refused
//, /../../.., /etc/..	accepted / unprotected	refused
$HOME/../../etc	accepted	refused
/usr/..\tmp (one real directory)	refused	refused
$HOME/rocm, $HOME/rocm/../rocm, $HOME/a\b	removable	removable

That last row matters as much as the rest: a guard that protects everything is useless.

• cargo test --workspace --all-targets — 3195 passed, 0 failed (32 suites), exit 0
• cargo clippy --locked --workspace --all-targets -- -D warnings — exit 0
• cargo fmt --all --check — exit 0
• The property suite was confirmed to catch the backslash case by temporarily reinstating it: two properties fail with minimal input /bin/..\tmp.
• Generator reach was measured rather than assumed: ~45% of draws contain .., ~31% a . component, ~15% //, ~30% a trailing separator, reaching several hundred distinct resolved locations across all protected roots.

On the Gherkin requirement

AGENTS.md asks for a scenario when observable CLI behaviour changes, and it does: rocm runtimes uninstall on an entry spelled /etc/ now refuses where it previously deleted. No scenario is added, deliberately. To exercise that end-to-end a scenario would have to plant a registry entry pointing at a real protected system path on a CI runner — and if the guard ever regresses, the test deletes it. The failure mode of the test is the catastrophe it tests for. The behaviour is covered at the unit level instead, including a regression test on the gate itself in apps/rocm.

Follow-ups deliberately not in this PR

• The symlink-through-home hole described above.
• Widening the Windows protected-root list.
• A relative path or ~ is not protected (pre-existing and unchanged); nothing absolutizes before the guard runs, and two consumers take model-generated JSON.

[jussielo-amd]

Reviewed the path-comparison fix in crates/rocm-core/src/runtime.rs plus its two other consumers. Hand-traced the lexical resolver against the respelling cases named in the PR body (/etc/, $HOME/../../etc, C:\Windows\..\ProgramData, etc.) and found no mismatches. Ran cargo test -p rocm-core, clippy, fmt, and xtask manifest --check/check-crate-edges locally — all clean, consistent with the green CI run.

A few non-blocking nits for a follow-up, not required for this PR:

• The PR body's list of "fixed consumers" misses a fourth caller, apps/rocm/src/storage.rs:628 (the prune planner) — it's automatically correct post-fix since it delegates to runtime_install_root_is_protected, just undocumented.
• rocmd's system_prefix_requires_ack is named as a fixed consumer, but its only test (install_sdk_rejects_system_prefix_without_ack) uses an already-canonical path, so it doesn't actually exercise the respelling bug this PR fixes. Worth a regression test with a respelled path.
• The new respelling tests are #[cfg(unix)]-only; the public gate's Windows branch (as opposed to the platform-parameterized helpers) isn't exercised end-to-end on the Windows CI lane. An adjacent existing test already shows the cfg!(windows)-at-runtime pattern to do this cheaply.
• delete_guard_properties's reference oracle unconditionally assumes absolute input — harmless today since all generator seeds are absolute, but worth a comment/assertion pinning that assumption so a future generator change can't silently desync oracle from production.
• The two explicitly-disclosed deferred gaps (symlink-through-home bypass, incomplete Windows protected-root list) are only noted in prose/comments — might be worth a tracked follow-up issue for each.

Approving — solid fix, well-tested, clearly reasoned PR description.

[rominf]

Thanks for the careful review — and apologies for the timing: I force-pushed a rebase after your approval, which I should have checked for first. Nothing in the reviewed change was modified. What happened:

main moved five commits ahead and the PR went to mergeable: false / dirty. The rebase from 9d261bd8 onto 01e7625b conflicted only in the two generated files, Cargo.lock and MANIFEST.md; apps/rocm/src/main.rs auto-merged. I resolved both by taking main's version and regenerating (cargo metadata, cargo xtask manifest) rather than hand-merging, so the only delta from your reviewed tree is the three new dependency rows that the proptest dev-dependency adds.

Diff between the commit you approved (fdc38faf) and the new head (347aa397) is the rebase and those regenerated files only — crates/rocm-core/src/runtime.rs and apps/rocm/src/main.rs are byte-identical to what you read.

Re-verified after the rebase, since a conflict-free rebase is not proof of a working build:

• cargo test --workspace --all-targets — exit 0, 3267 passed, 0 failed
• cargo clippy --locked --workspace --all-targets -- -D warnings — exit 0
• cargo fmt --all --check — exit 0
• cargo xtask manifest --check / cargo xtask tpn --check — exit 0

On your nits, all fair:

1. Fourth consumer undocumented. Correct — storage.rs:628 delegates to the guard and is automatically fixed. I'll add it to the PR body.
2. system_prefix_requires_ack isn't actually exercised by a respelled path. This is the sharpest one: naming it as a fixed consumer while its only test uses an already-canonical path means the claim rests on reading, not on a test. I'd rather add that regression test than leave the claim unbacked.
3. Respelling tests are cfg(unix)-only. Agreed, and thank you for pointing at the existing cfg!(windows)-at-runtime pattern — the public gate's Windows branch is exactly the part that is otherwise only reachable on the Windows lane.
4. Oracle assumes absolute input. Agreed, and worth more than a comment: the oracle desyncing silently from production is the specific failure this suite exists to prevent. An assertion is better than a note.
5. Deferred gaps should be tracked issues, not prose. Agreed — the symlink-through-home bypass in particular is a live hole that a reader of the comment could easily assume is covered.

Let me know whether you would prefer 2–4 in this PR or as an immediate follow-up. My instinct is that 2 and 4 belong here, since both are about this PR's claims being backed by tests rather than by prose, while 3 is a genuine addition that could go either way. I will file issues for 5 regardless.

[rominf]

Pushed c104da01 on top of your approval — a separate commit rather than an amend, so the delta is visible. Test and assertion additions only; zero production lines changed (the diff has no deletions at all).

Nit 2 — system_prefix_requires_ack now actually exercised. You were right that naming it as a fixed consumer while its only test used an already-canonical path left the claim resting on reading. install_sdk_rejects_system_prefix_reached_by_escaping_home builds the prefix from the real home dir plus /../../usr — the exact shape the bug let through. Confirmed red against the pre-fix runtime.rs: unwrap_err() on an Ok value: [..., "--prefix", "<home>/../../usr", "--approve-replacing-active-default"], i.e. the old code waved the escaping prefix through without requiring the ack.

Nit 3 — Windows lane now reaches the public gate. the_public_gate_refuses_every_spelling_of_a_protected_root_on_its_own_host follows the cfg!(windows)-at-runtime pattern you pointed at (the one ensure_runtime_install_root_rejects_protected_system_path already uses) rather than a new mechanism. On Windows it drives the gate against respelled C:/Windows, C:/Program Files and C:/Program Files (x86) (trailing separator, mixed case, doubled separator); elsewhere it asserts the Unix answer.

Since cargo check --target x86_64-pc-windows-msvc can't run here (ring/aws-lc-sys need lib.exe), the Windows arm was type-checked by copying the pure-Rust functions into a scratch crate and checking against the MSVC target — and the check was proved live by planting a deliberate type error in the protected-roots array and confirming the compiler rejected it at that line. Runtime behaviour on Windows is still first exercised by the windows-build-and-test lane.

Nit 4 — oracle assumption pinned with an assertion, not a comment, at the top of the property suite's lexically_resolved reference oracle. An oracle silently desyncing from production is precisely what that suite exists to prevent, so a note would not have been enough.

Nit 1 (fourth consumer) is in the PR description now. Nit 5 is filed: #494 for the symlink-through-home bypass and #495 for the narrow Windows protected-root list.

One thing worth flagging since you may see it locally: xtask's workflow_contract::tests::apu_preflight_gates_on_the_pool_the_engine_uses fails in my container, and it is not related to this PR — it reproduces on unmodified main at 01e7625b. The panic is out.status.code() returning None, which means the pwsh child was killed by a signal rather than exiting non-zero; it only appears here under memory pressure. CI is the authority on it.

Verification after the new commit: 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 exit 0, cargo xtask manifest --check exit 0, and both new tests plus all five delete_guard_properties properties pass.

Re-approval is yours to give or withhold — happy to split any of this into a follow-up if you would rather keep the approved diff untouched.