feat(storage): add rocm storage to see and reclaim disk space

[rominf]

Summary

Adds rocm storage: see what ROCm CLI is using on disk, and reclaim what it no longer needs.

rocm storage [report]                                          # default verb
rocm storage remove-old-installs [--keep N] [--dry-run] [--yes]
rocm storage remove-downloads    [--dry-run] [--yes]

Root cause

Every SDK install that resolves a new version creates a fresh multi-gigabyte tree and nothing ever removes the old ones. Both the wheel and tarball paths always fold the version into the runtime key, so accumulation is unbounded, and rocm update --apply goes through the same path — simply keeping up to date is how users hit this. The only pruning that existed anywhere was for log files and dash demo sessions. There was also no way to see what was consuming space.

Design

Mirrors RuntimesCommand/ServicesCommand, including the Option<Subcommand> shape so the bare verb is the read-only report. Named in plain English per the UX guidelines — gc and prune are jargon, and the leaf verbs say "installs" rather than "runtimes". remove-old-runtimes and remove-downloaded-files exist as aliases.

Removal delegates to the existing uninstall_runtime per selected key, so config and marker cleanup stays in one place. remove-downloads reuses UninstallPlan/remove_path, and both mutating verbs keep the interactive_terminal() + confirm_uninstall() gate.

Selection is split in two so the policy is testable without a filesystem:

• select_runtimes_to_remove — pure. Groups by channel/format/family, orders by install time, and force-keeps the active install, the previous one (rollback depends on it), the configured default, and anything named by the active-install marker. Refuses read_only and imported_from.
• build_prune_plan — adds the filesystem guards: runtime_install_root_is_protected first, then should_remove_runtime_install_root, which enforces the in-tree manifest check so we only ever delete trees we wrote.

Every skip is printed under "Left alone:" with its reason.

Sizing is a non-following iterative walk: an unreadable subtree renders as "N or more (part unreadable)" and a missing root as "unknown size" — a report never fails because one path could not be read.

Shared caches (uv, Hugging Face) are reported and labelled, never deleted. Model weights get no delete verb at all in this pass.

Verified behaviour

Against a fixture of six accumulated installs of one family, with config.json
written exactly as activate_runtime writes it — that is, active_runtime_key,
previous_runtime_key and the family-wide default_runtime_id all set,
which is the state every real machine is in:

$ rocm storage remove-old-installs --dry-run
2 install(s) would be removed, freeing about 196.1 KiB:
  - release-wheel-gfx110x-all-7-10-0 version=7.10.0 ...
  - release-wheel-gfx110x-all-7-11-0 version=7.11.0 ...

Left alone:
  - release-wheel-gfx110x-all-7-12-0: one of the most recent installs kept for this GPU family
  - release-wheel-gfx110x-all-7-13-0: one of the most recent installs kept for this GPU family
  - release-wheel-gfx110x-all-7-14-0: the rollback target for `rocm runtimes rollback`
  - release-wheel-gfx110x-all-7-15-0: in use right now

Re-running without --dry-run --yes removes exactly those two folders and their
registry entries; the other four are untouched.

An earlier revision of this PR omitted default_runtime_id from this fixture. That
mattered: runtime_id carries no version, so the default matched the whole family
and held all of it, and the command removed nothing on any real machine. Fixed, with
a regression test carrying default_runtime_id and active_runtime_key for the same
family.

A cache root that is a symlink is reported as left alone rather than followed;
deleting through a relocated cache is verified not to happen.

Without --yes outside a terminal both mutating verbs refuse and exit 1,
removing nothing.

Two changes outside the new module

• treat_as_natural_language gates clap on a hard-coded verb list, so rocm storage --help was silently swallowed by the freeform planner until storage was registered. This is a footgun for any future top-level verb.
• The startup update check can provision a managed Python, so asking what was using disk space downloaded more of it. storage is now excluded alongside update/bootstrap/completions.

format_bytes also gained KiB/MiB tiers — it was dead code with only a GiB tier, so a 39 MB cache printed as 39496209 bytes. Note the repo has a near-duplicate format_bytes_for_user; worth collapsing separately.

For maintainer decision

1. --keep 2 default (current plus one rollback target) is my judgement from the rollback feature, not a tuned value.
2. Should rocm update --apply offer to prune what it superseded? Update is the accumulation engine. I deliberately did not touch that path.
3. Retention grouping is by channel/format/family ordered by install time, not parsed version — so the same version in two formats is two groups.
4. remove-downloads scope currently includes the signed metadata cache under cache_dir/therock. If that should survive, it is a one-line filter.
5. Engine environments under data_dir/engines are also large and still have no removal path — arguably a follow-up.

Tests

Retention grouping per family; all four force-keeps, each checked for case-insensitive matching; adopted/imported/read-only never selected; prune refusing an install whose in-tree manifest is missing; report rendering; tolerant sizing of a missing folder; and the downloads plan never touching model files.

Added in review: a family-wide default_runtime_id alongside an active key for the same family (the real-machine state, and the regression test for the inert---keep bug); an unambiguous default still holding its install; both symlinked-root cases (downloads plan and sizing); the deletion path end to end; and the confirmation gate. storage classification is covered in both assistant verb allowlists.

cargo test --workspace --all-targets shows two failures in proc_lifecycle that are unrelated to this PR — they reproduce on an unmodified main on the same host. They are tracked in #168, with #169 open as the proposed fix; neither has landed. Everything else passes; cargo fmt --all --check, both clippy -D warnings invocations, and cargo xtask manifest --check are clean.

Non-goals

Three things surfaced in review are deliberately left out of this PR and tracked separately:

• Derive the assistant verb allowlists from the clap command tree #188 — derive the four hand-maintained assistant verb allowlists from the clap command tree. This PR classifies storage in all of them, but does not restructure them.
• runtime_install_root_is_protected is a denylist, not an allowlist #189 — runtime_install_root_is_protected is a denylist with real gaps. Unreachable today behind the in-tree manifest check, but worth inverting.
• format_bytes_for_user prints KB/MB/GB for base-1024 math #190 — format_bytes_for_user prints KB/MB/GB for base-1024 arithmetic. It reaches unrelated call sites, so changing its output belongs in its own change.

Not verified: Windows behaviour for the two symlink fixes (NTFS junction semantics differ; worth a look in the Windows lane), and whether uv's hardlink mode makes the report double-count blocks between ROCm installs and the uv cache.

Fixes #161

[volen-silo]

Reviewed this one harder than usual given it permanently deletes multi-gigabyte trees. The safety machinery is genuinely well built — I tried to find a way to make it delete a runtime it promised to keep and could not. Selection is correctly split into a pure policy plus filesystem guards, the confirmation gate holds, and the ownership check is strong.

Two blocking issues though, and they pull in opposite directions: remove-old-installs currently deletes nothing on any real machine, and remove-downloads can delete something it shouldn't.

---

Blocking

1. remove-old-installs is inert on every real installation — it doesn't fix #161

apps/rocm/src/storage.rs:222 holds an install when manifest.runtime_id matches config.default_runtime_id. But runtime_id carries no version and no format — apps/rocm/src/therock.rs:942 and :1067:

runtime_id: format!("therock-{}:{}", channel.as_str(), resolution.family),

So every install of a given channel+family shares one runtime_id. And activate_runtime sets it unconditionally (apps/rocm/src/main.rs:5810):

config.default_runtime_id = Some(manifest.runtime_id.clone());

Installing or updating activates, so default_runtime_id is set on every real machine — and the Default hold then matches every install in the family. --keep becomes a no-op, including --keep 0. Against a fixture with six accumulated installs and a config.json written exactly as activate_runtime writes it:

$ rocm storage remove-old-installs --dry-run
Nothing would be removed.
Left alone:
  - release-wheel-gfx110x-all-7-10-0: the configured default
  - release-wheel-gfx110x-all-7-11-0: the configured default
  - release-wheel-gfx110x-all-7-12-0: the configured default
  - release-wheel-gfx110x-all-7-13-0: the configured default
  - release-wheel-gfx110x-all-7-14-0: the rollback target for `rocm runtimes rollback`
  - release-wheel-gfx110x-all-7-15-0: in use right now

Deleting only default_runtime_id from that same fixture and re-running removes 2 installs.

The reason this isn't caught: the "Verified behaviour" fixture in the PR description omits default_runtime_id, a state no real installation is ever in. And the single test that sets it (storage.rs:962) uses "therock-release:gfx120X-all" — a different family from the manifests under test — so the Default hold never fires in the suite. That's how 8 green tests and 21/21 green CI coexist with a feature that does nothing. This is precisely the rocm update --apply accumulation scenario the PR's own Root cause section describes.

Suggested fix: scope the Default hold to the install the default actually resolves to, reusing current_runtime_manifest / default_runtime_id_matches (main.rs:6844-6874), which already handles the "matches exactly one" case — or drop the Default hold entirely, since HoldReason::Active already covers the resolved install. Please add a regression test whose RetentionInputs carry default_runtime_id and active_runtime_key for the same family.

2. remove-downloads deletes through a symlinked cache root

apps/rocm/src/storage.rs:684 uses Path::exists, which follows symlinks, and :691 calls read_dir on the walk root, which also follows. Child entries are handled correctly — :698-704 uses symlink_metadata and refuses to descend into symlinked directories — but the two roots are never symlink_metadata'd. The entries collected are real files reached through the link, so remove_path deletes them for real.

Reproduced end to end with the victim directory entirely under /tmp:

$ ln -s /tmp/victim <cache_dir>/therock
$ rocm storage remove-downloads --yes
2 downloaded file(s) removed
# both files gone from /tmp/victim

The trigger is relocating the ROCm cache to a bigger disk with a symlink — a normal thing for exactly the user who is running a disk-space command. Dry-run does not protect them: it prints the paths through the link, so they look like ordinary cache paths and nothing appears wrong.

Fix: symlink_metadata each root and skip with a "Left alone:" reason if it's a symlink, or canonicalize and require containment under paths.cache_dir.

---

Worth fixing before merge

3. storage is missing from two more verb allowlists. chat_rocm_command_action_from_args (main.rs:9077-9226) has no "storage" arm, so the catch-all at :9224 bails; ensure_rocm_command_is_read_only (apps/rocmd/src/lib.rs:2366-2395) likewise. So an assistant tool call for storage report is rejected even though it's purely read-only. Fail-closed, so not a security hole — but this PR is itself the evidence for the footgun it flags: there are four parallel hand-maintained verb lists and it updated two.

4. measure_path reports a symlinked directory as ~17 bytes, marked complete. storage.rs:60-72: symlink_metadata on the walk root → is_dir() false → returns {bytes: <link length>, complete: true}. Meanwhile PathUsage::measure (:124) and build_report (:380) gate on path.exists(), which does follow — so the two halves disagree. A 2.9 MiB HF cache reports as 17 bytes with no "(part unreadable)" caveat. This hits exactly the shared caches the feature exists to surface, and feeds the "freeing about X" estimate via :610. Use fs::metadata for the root specifically; the descent at :87-94 is correct as written.

5. "removed X (N GiB)" prints even when the folder wasn't removed. storage.rs:801-810 discards RuntimeUninstallResult::removed_install_root. When uninstall_runtime's own re-evaluation returns Ok(false) the install root stays, but the registry entry is deleted unconditionally (main.rs:5902-5910) and the call still returns Ok. The user is told N GiB was freed; the tree remains, is now invisible to rocm storage report, and is un-prunable forever because its registry record is gone. The sibling command already handles this — RuntimesCommand::Uninstall at main.rs:5521-5529 prints folder_removed: no.

6. A corrupt config.json silently disables all four force-keeps. storage.rs:780 uses RocmCliConfig::load(&paths).unwrap_or_default(). load returns Ok(default()) only when the file is absent; an unparseable file returns Err, which unwrap_or_default() collapses into "nothing active, nothing previous, no default" — with no warning. ConfigCommand uses ? and surfaces the error (main.rs:7033). This is a fail-open on the only destructive command here, and it becomes the dominant residual risk once #1 is fixed.

7. Neither destructive verb records an audit event. record_cli_audit_event appears ~30× in main.rs including "runtime_uninstall" at :5540, but storage.rs has zero calls — calling crate::uninstall_runtime directly bypasses the dispatch arm that logs. Bulk-deleting several multi-GB installs leaves no trace while deleting one via rocm runtimes uninstall does.

Non-blocking

• The case-insensitivity test only covers one of four force-keeps (storage.rs:957-963) — only active_runtime_key differs in case. Replacing eq_ignore_ascii_case with == on the Previous, Default and Marker guards leaves all 8 tests passing.
• No test covers the deletion path or the safety gates. All 8 stop at plan building; nothing exercises storage() (:778-831), approved() (:768-776), remove_path, or uninstall_runtime. Both #2 and #4 would have been caught. I verified the gates by hand — but nothing stops them regressing.
• treat_as_natural_language: I cross-checked all 30 clap Command variants plus the comfy/models aliases against STRUCTURED (main.rs:16281-16320) — the list is currently complete, storage was the only casualty. But the guard test at main.rs:20381 is itself a hardcoded 26-entry array, so the next forgotten verb is also forgotten in the test. Iterating Cli::command().get_subcommands() + get_visible_aliases() would make the whole class CI-visible, and extended to the other two lists would have caught #3.
• --keep is unvalidated (main.rs:649-650, no value_parser range). --keep 0 is accepted and intentional; today #1 makes it harmless, but once #1 is fixed it becomes a "delete everything not currently active" button behind a single y/N.
• Retention orders by install time, not version (storage.rs:277-282), so a deliberate downgrade makes the older version "most recent" and --keep 1 deletes the newer one. Defensible, but undocumented in --help and the README. Same for (channel, format, family) grouping meaning a user who's tried nightly keeps --keep × N.
• TOCTOU: holds are computed at storage.rs:792 and never re-checked after approved() blocks on stdin. The ownership guard is re-checked inside uninstall_runtime; the holds aren't. Activating a runtime in another terminal while the prompt waits lets it be deleted. Cheap fix: re-run unconditional_hold per entry immediately before deleting.
• runtime_install_root_is_protected (crates/rocm-core/src/runtime.rs:365-393) is a denylist returning false for $HOME exactly, and for /home, /mnt, /srv, /media; the Windows list at :375 hardcodes C:. Not reachable today thanks to the manifest check, but this is the belt-and-braces guard storage.rs:581 newly leans on.
• format_bytes_for_user (main.rs:13544) is live via main.rs:13409 and prints KB/MB/GB for base-1024 math while the newly-live format_bytes prints KiB/MiB/GiB. Confirmed format_bytes had zero call sites at the merge base, so "it was dead code" is accurate. Minor: {:.1} prints 1_048_575 as "1024.0 KiB".
• Small accuracy note: the PR says the proc_lifecycle failures are "fixed by #169", but #169 is still open — reads as landed.

Verified clean

• The confirmation gate holds. Non-interactive without --yes → refuses, exit 1, nothing deleted. echo y | → still refused (interactive_terminal() requires both stdin and stdout to be TTYs). --dry-run --yes → exit 0, nothing deleted; dry_run short-circuits at :794/:816 before approved() is called, so there's no precedence bug. confirm_uninstall() (main.rs:15040-15049) accepts only exact y/yes.
• Reported == executed. :792 → 793 → 801 and :814 → 815 → 823 print from and iterate the same plan binding — one plan-builder per verb, no separate dry-run computation that could drift.
• All five guards in unconditional_hold (:216-234) are load-bearing — disabling any one makes an existing test fail. Held entries are extracted before grouping and sorting (:263-272), so a tie in installed_at_unix_ms can't leak the active install into the removal set. Empty registry, single install, a default naming a nonexistent key, and negative/non-numeric --keep all behave.
• The ownership guard is strong. local_runtime_manifest_matches (main.rs:5991-6003) requires an in-tree .rocm-cli-runtime.json whose runtime_key, runtime_id and install_root all match, with install_root compared via paths_equivalent against the directory it was found in — so a copied or moved tree carrying a stale manifest does not look owned. This is what keeps the $HOME gap above unreachable.
• Symlink semantics in the deletion path itself are correct — remove_path (main.rs:15105-15119) branches on is_symlink(), and both walks refuse to descend into child symlinks. Issues #2 and #4 are specifically about the roots.
• measure_path is genuinely iterative (explicit stack, :76) with saturating_add throughout — no recursion or overflow risk.
• Locally: cargo fmt --all --check clean; cargo clippy --locked --workspace --all-targets -- -D warnings clean; cargo test -p rocm --bin rocm storage:: 8/8; cargo xtask manifest --check clean. cargo test --workspace --all-targets shows only the two known proc_lifecycle failures. Leak scan clean; license header present; README accurate against the implemented surface.

Not verified: Windows behaviour (NTFS junctions vs. the symlink semantics in #2/#4 — worth confirming #2 specifically in the Windows lane), cargo xtask tpn --check (no cargo-about locally; CI's equivalent is green), and whether uv's hardlink mode makes measure_path double-count blocks across "ROCm installs" and "uv package cache".

All destructive testing used ROCM_CLI_*_DIR pointed at throwaway /tmp fixtures; no real install was touched.

[rominf]

Thanks — this was an unusually useful review. Both blocking issues were real; I reproduced each one before touching anything, and confirmed the same reproduction behaves differently afterwards. All of it is in e941f83.

1. remove-old-installs inert on every real machine — fixed.

Reproduced first, with your fixture: six accumulated installs of one family and a config.json written exactly as activate_runtime writes it. Nothing would be removed., all six held, four of them as "the configured default", and --keep 0 equally inert. Your diagnosis was exactly right.

The Default hold now only fires when default_runtime_id resolves to exactly one install — the same "matches exactly one" rule current_runtime_manifest already applies. When it is ambiguous it holds nothing, and the install genuinely in use is still held as Active, which is your second suggestion in effect. I kept the hold rather than dropping it so an unambiguous default still reports the most useful reason.

Same fixture after the fix removes 2 installs, holding 7-14-0 as the rollback target and 7-15-0 as in use. Verified for real, not just --dry-run: the two folders and their registry entries are gone, the other four untouched.

Regression test is keep_still_applies_when_the_default_id_covers_a_whole_family — RetentionInputs carrying default_runtime_id and active_runtime_key for the same family, asserting both --keep 2 and --keep 0, plus an_unambiguous_default_still_holds_its_install for the other half of the rule.

2. remove-downloads deleting through a symlinked cache root — fixed.

Reproduced end to end under /tmp with ROCM_CLI_*_DIR: ln -s the victim directory in as <cache>/therock, remove-downloads --yes, both files really gone. Your point that dry-run does not protect the user here is the part that made this worth prioritising.

Both walk roots are now symlink_metadata'd and reported as <path> is a link to somewhere else, so its contents are left alone. I chose skip-and-explain over canonicalise-and-contain because it tells the user what happened. Same repro now removes nothing and leaves both files in place. Covered by downloads_plan_refuses_to_reach_through_a_symlinked_cache_root.

3. storage missing from two more allowlists — fixed. Added to both chat_rocm_command_action_from_args and ensure_rocm_command_is_read_only: report (and the bare verb) read-only, the two remove-* verbs through approval with --yes forced. Tested on both sides. On the underlying footgun — four hand-maintained lists where the guard test is itself a hardcoded array — I agree, but deriving them from Cli::command().get_subcommands() is its own change; opened #188.

4. measure_path reporting a symlinked directory as ~17 bytes — fixed. Root now uses fs::metadata, descent unchanged as you suggested, so the two halves agree with the Path::exists gates. Verified: a 2.9 MiB linked cache reported as 17 bytes before, 2.9 MiB after. Test also asserts child symlinks are still not followed, so the original intent is not lost.

5. "removed X (N GiB)" when the folder was not removed — fixed. removed_install_root is no longer discarded. On None it says the registry entry went but the folder stayed, that nothing was freed, and names the path so it is still actionable — the tree is no longer silently orphaned.

6. Corrupt config.json silently disabling all four force-keeps — fixed. unwrap_or_default() replaced with ?. load already returns the default for an absent file, so the only thing this was swallowing was a genuinely unparseable one, which is exactly when a destructive command should stop.

7. No audit event — fixed. Both destructive verbs now call record_cli_audit_event; the install one records folder_removed so the log distinguishes #5's two outcomes. Verified in the audit log on the real removal run.

Non-blocking:

• Case-insensitivity now covered on all four force-keeps (all_force_keeps_compare_case_insensitively) — replacing any one eq_ignore_ascii_case with == fails it.
• Deletion path and gates now tested: the removal end to end, both symlink-root cases, and the --yes requirement outside a terminal.
• TOCTOU: holds are re-checked per entry immediately before deleting, as you suggested.
• --keep ordering by install time and the per-(channel, format, family) grouping are now documented in both --help and the README, including that a downgrade makes the older version the "most recent" install.
• format_bytes no longer prints 1024.0 KiB. The format_bytes_for_user unit mismatch reaches unrelated call sites, so that is format_bytes_for_user prints KB/MB/GB for base-1024 math #190.
• runtime_install_root_is_protected as a denylist: agreed, and it matters more now that this command leans on it. Inverting it is runtime_install_root_is_protected is a denylist, not an allowlist #189.
• --keep 0 left accepted and still gated by the same holds plus the y/N, now that it is no longer a no-op. Happy to add a floor if you would rather.

PR description corrected on both counts: the "Verified behaviour" fixture now includes default_runtime_id and shows the real-machine six-install case, with a note on why the old fixture hid the bug; and the proc_lifecycle line no longer implies #169 landed — #168 is open, #169 is open, and I confirmed those two failures reproduce on an unmodified main on the same host.

Verification. cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings, and cargo xtask manifest --check all clean. cargo test --workspace --all-targets --no-fail-fast: only the two known proc_lifecycle failures, nothing else.

Still not verified by me: Windows behaviour for the two symlink fixes — the Windows lane is green, but it does not exercise a junction pointing at the cache root specifically, so your note stands; and whether uv's hardlink mode makes the report double-count blocks. Both are called out as non-goals in the description.

CI: everything green except E2E tests (GPU), which is not this change. It fails one second in having executed zero steps — the self-hosted runner is never picked up — and the same zero-step failure is on the latest main run. I re-ran it twice (the run is on attempt 3) with the identical result. That job is continue-on-error: true, so the CI run's overall conclusion is success; it needs a runner, not a code fix. windows-build-and-test and both Strix Halo lanes are green.

Leaving all of this for you to confirm rather than marking it resolved.

[volen-silo]

Re-reviewed at e941f83. All seven blocking items are genuinely fixed. I checked each against the code rather than the description, and re-ran the verification locally rather than taking it on trust.

Confirmed fixed

1. resolved_default_runtime_key is the right rule, and it is the same one — default_runtime_id_matches + [manifest] => Some(*manifest) at main.rs:6861-6865. Keeping the hold for the unambiguous case rather than dropping it is the better of the two options I offered. keep_still_applies_when_the_default_id_covers_a_whole_family asserts both --keep 2 and --keep 0, and an_unambiguous_default_still_holds_its_install pins the other half.
2. Both roots symlink_metadata'd, skip-and-explain rather than canonicalise — agreed, the reason line is worth more here than containment. Entries below the root are unchanged, which was already correct.
3. storage is in all four. I re-checked the two you didn't touch this round (STRUCTURED at main.rs:16322, the guard array at :20462) — complete.
4. Root on fs::metadata, descent unchanged. DirEntry::metadata() does not follow, so the "child symlinks are still not followed" assertion is load-bearing and correct.
5. / 6. / 7. As described. RocmCliConfig::load(&paths)? also matches the rest of the tree — ? is the dominant pattern here and unwrap_or_default() the exception, so this is a consistency fix as well as a safety one.

Verification I reproduced

cargo fmt --all --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo xtask manifest --check clean. cargo test --workspace --all-targets --no-fail-fast: only proc_lifecycle::tests::tree_stop_waits_for_descendants and tree_forced_kill_reaches_sigterm_ignoring_descendant, nothing else; 15/15 in storage::. #168 and #169 are both open, so the corrected description reads right. The GPU lane here started and completed at the same second with zero steps on self-hosted, linux, amd-gpu; the same job on main's latest CI run does the same thing. Not this change.

Three small things, none blocking

1. The measure_path fix leaks into the downloads estimate. A symlink inside the cache is collected as an action — build_downloads_plan only refuses to descend into child symlinks — and render_downloads_plan now sizes it with the root-following measure_path:

1 downloaded file(s) would be removed, freeing about 8.0 KiB:
  - ROCm archive: <cache>/therock/linked-archive

The link is a few bytes and the 8 KiB behind it stays; remove_path unlinks correctly, so the number is the only thing wrong. Display-only, no data loss, but it is wrong in the optimistic direction.

2. The plan and the run can disagree now that --keep 0 is a real button. Holds are re-checked per entry against the current manifest set, so a removal can make the default resolve uniquely and hold the next entry. Two installs of one family, no active or previous, default_runtime_id naming the family, --keep 0:

PLANNED=["bee", "cee"]
DELETED bee
SKIPPED cee : the configured default

It fails safe and says so out loud, and it needs a config state activate_runtime does not produce on its own — I would leave it. But it is the dry-run/actual divergence class from #2, so worth knowing it is there.

3. matches_ignore_case(default_key, key) in unconditional_hold can no longer differ from == — default_key is a clone of some manifest's own runtime_key. Swapping it for default_key == Some(key) leaves all 15 storage tests green, so "replacing any one eq_ignore_ascii_case with == fails it" is off by that one site. The four inputs are all genuinely covered — the eq_ignore_ascii_case inside resolved_default_runtime_key is what the Default case exercises — so the test does what its name says. The remaining comparison is harmless defensive code.

While you're here

rocm storage has no e2e scenario at all, while uninstall has several in install_lifecycle.feature. A single rocm storage report scenario would pin the freeform-parser footgun this PR had to fix — that one is invisible to unit tests by construction.

On --keep 0: leave it as is. The holds plus the y/N are the right gate, and a floor would make "delete everything I am not using" unreachable for no safety gain.

Nothing blocking left from my side. My CHANGES_REQUESTED is stale — happy to clear it.