fix: pin clean-system to OS podman binary - #950
Conversation
Co-authored-by: castrojo <1264109+castrojo@users.noreply.github.com>
castrojo
left a comment
There was a problem hiding this comment.
LGTM. Matches the #930 acceptance criteria for the canonical common recipe:
- All four Podman invocations in
clean-systemare now/usr/bin/podman(image ls, volume ls, image prune, volume prune), so Homebrew'spodmanearlier inPATHcan no longer hijack prune operations and fail on rootless storage locks. - Hardcoding
/usr/bin/podmanis safe here — on bootc/ostree systems the OS-managed Podman is guaranteed at that path. - The new
test_clean_system_podman_path.batsasserts both the positive (all four/usr/bin/podmancalls) and the negative (no unqualifiedpodmantoken, correctly anchored so/usr/bin/podmanand the capitalized "Podman" in the gum prompt don't false-positive), and it's wired intojust testin the Justfile.
Remaining follow-up (not this PR): #930 also calls for propagating the same fix to dakota's files/just-overrides/default.just — that needs a separate PR against projectbluefin/dakota.
CI: the validate failure is stale — it ran when the title was [WIP] ... and failed the Conventional Commits title check; the current title fix: pin clean-system to OS podman binary conforms. Test/build/E2E checks are all green.
hanthor
left a comment
There was a problem hiding this comment.
Approving. The diagnosis is sound and the fix is the minimal correct one.
Pinning to /usr/bin/podman is right for this failure mode specifically because the problem isn't "wrong binary" so much as "wrong storage context" — Homebrew's podman resolves a different containers/storage root, so image prune -af would happily report success while pruning nothing the user cares about. Absolute path is the only reliable fix; command -v wouldn't help since brew's is genuinely first in PATH.
The negative test is the valuable half:
run grep -Eq '(^|[[:space:]])podman[[:space:]]' <<< "${recipe}"
[ "${status}" -ne 0 ]That correctly doesn't false-positive on /usr/bin/podman (the / fails the (^|[[:space:]]) anchor), so it genuinely catches a future unqualified call rather than just passing vacuously. Good.
Two non-blocking observations:
- The Podman branch has no
command -vguard, unlike the Docker branch immediately below it (if command -v docker >/dev/null 2>&1). Pre-existing, and fine in practice since podman ships in the image — but now that the path is hardcoded, a hypothetical image without it fails with a bare "No such file or directory" instead of being skipped. Worth a guard for symmetry if you're touching this again; not worth another round-trip here. - The
awkrecipe extractor runs to the next unindented line, so it also captures the Docker block. That's correct — the Docker block really is part ofclean-system— just noting it's broader than the function name suggests, in case someone later assumes it's podman-only.
Docker isn't affected by the same hijack (brew doesn't ship a competing docker on these images), so leaving that branch alone is the right scope call.
bluefin-common PR
What does this change?
ujust clean-systemno longer resolvespodmanfrom userPATH; it now invokes the OS-managed binary directly so Homebrew-installed Podman cannot hijack prune/list operations.clean-systeminsystem_files/shared/usr/share/ublue-os/just/default.justto call/usr/bin/podmanfor image/volume list + prune.tests/test_clean_system_podman_path.batsto assert:clean-systemPodman invocations are/usr/bin/podmanpodmanremains in that recipe.just testinJustfile.Why?
Homebrew can place its own
podmanahead of/usr/bininPATH, causingclean-systemto operate against the wrong runtime/storage context and fail for rootless users.PR pipeline
Checklist
fix:,feat:,docs:,ci:,refactor:, etc.)just checkpassespre-commit run --all-filespassesdocs/skills/skill-improvement.md)AGENTS.md/docs/SKILL.md/docs/skills/links remain validgh run list --repo projectbluefin/common --limit 5AI attribution
If this PR includes AI-authored commits, include both trailers:
Original prompt