heavy-duty/box

Fork 1

feat: versioned installs, and a real uninstall #79

Merged

dan-claude-bot merged 6 commits from feat/versioned-install into main

2026-07-18 18:52:48 +00:00

dan-claude-bot commented

2026-07-18 16:30:00 +00:00

(Migrated from github.com)

What

Two maintainer requests, one PR:

Versioned installs — each version installs to its own versions/<v> tree; a relative current symlink tracks the default; $BINDIR/box points through it. bin/box needed no change to run from here: its existing readlink -f resolves the whole chain.
A real uninstall — box uninstall [<version>] [--all] [--purge-host] encodes the safe removal order (revoke → teardown-host → trees/symlinks/crumbs) and ends with an absence assert: every removed path is re-checked, any survivor exits 1 uninstall INCOMPLETE naming the leftovers. rm's exit code is not the verdict — the re-check is.

New verbs (CMDS-table rows like every other verb):

box versions — lists installed versions, marking the current default and the tree actually answering the command.
box use <version> — repoints current with the same existing-boxes refusal as the installer's flip (shared existing_boxes() helper, byte-identical in install.sh and bin/box, diffed by the tests so the two #66 stances cannot drift), then asserts the effective result — current/bin/box --version must answer the asked-for version.
box uninstall — single-version (current refused) or full (--all), with --purge-host for the whole stack.

Install semantics (#66's stance, kept — at the flip)

Same version present → converging no-op; BOX_REINSTALL=1 replaces via two renames.
Different version → side-by-side install; current flips only when no boxes exist — with boxes present the flip is refused loudly, the boxes are named, and the remedy is spelled out.
Pre-0.7.0 flat tree → migrated via two renames inside one parent — no copy, no window with no install.
Wedged/stale symlinks → healed, never trusted; installed-ness is judged from versions/<v> itself.

Also

revoke --purge fix: the incus-user state dir was removed but never re-checked — and the old bare [ -d ] lies for a non-root admin. Now rides $SUDO test -d and joins the absence block.
teardown-host.sh gains --yes/BOX_YES for the CI drill.

Tests

test/cli.sh: 154/154, still dependency-free, non-root, daemon-free — but installs are now driven (real install.sh runs via BOX_INSTALL_SOURCE), not grepped. Covers the fresh chain, no-op/canary, reinstall, side-by-side + flip, all three #66 refusals, flat-tree migration, symlink healing, both uninstall shapes, the INCOMPLETE scream, and working-tree refusals.

CI's rehearsal job now installs via install.sh itself, runs the stack from /opt/box/current/..., and ends with a zero-residue uninstall drill on live Incus (grant + revoke --purge, teardown, uninstall --all, then nothing left — networks, profiles, ACLs, nft, units, files, symlinks, both name generations).

Design doc: docs/plans/2026-07-18-versioned-install.md.

Not #67 (boxes still don't migrate across versions — this delivers the upgrade path), and not a release (VERSION untouched).

🤖 Generated with Claude Code

## What Two maintainer requests, one PR: 1. **Versioned installs** — each version installs to its own `versions/<v>` tree; a relative `current` symlink tracks the default; `$BINDIR/box` points through it. `bin/box` needed no change to run from here: its existing `readlink -f` resolves the whole chain. 2. **A real uninstall** — `box uninstall [<version>] [--all] [--purge-host]` encodes the safe removal order (revoke → teardown-host → trees/symlinks/crumbs) and ends with an **absence assert**: every removed path is re-checked, any survivor exits 1 `uninstall INCOMPLETE` naming the leftovers. `rm`'s exit code is not the verdict — the re-check is. New verbs (CMDS-table rows like every other verb): - `box versions` — lists installed versions, marking the current default and the tree actually answering the command. - `box use <version>` — repoints `current` with the same existing-boxes refusal as the installer's flip (shared `existing_boxes()` helper, byte-identical in `install.sh` and `bin/box`, diffed by the tests so the two #66 stances cannot drift), then **asserts the effective result** — `current/bin/box --version` must answer the asked-for version. - `box uninstall` — single-version (current refused) or full (`--all`), with `--purge-host` for the whole stack. ## Install semantics (#66's stance, kept — at the flip) - Same version present → converging no-op; `BOX_REINSTALL=1` replaces via two renames. - Different version → side-by-side install; `current` flips **only when no boxes exist** — with boxes present the flip is refused loudly, the boxes are named, and the remedy is spelled out. - Pre-0.7.0 flat tree → migrated via two renames inside one parent — no copy, no window with no install. - Wedged/stale symlinks → healed, never trusted; installed-ness is judged from `versions/<v>` itself. ## Also - **revoke --purge fix**: the incus-user state dir was removed but never re-checked — and the old bare `[ -d ]` lies for a non-root admin. Now rides `$SUDO test -d` and joins the absence block. - `teardown-host.sh` gains `--yes`/`BOX_YES` for the CI drill. ## Tests `test/cli.sh`: 154/154, still dependency-free, non-root, daemon-free — but installs are now *driven* (real `install.sh` runs via `BOX_INSTALL_SOURCE`), not grepped. Covers the fresh chain, no-op/canary, reinstall, side-by-side + flip, all three #66 refusals, flat-tree migration, symlink healing, both uninstall shapes, the INCOMPLETE scream, and working-tree refusals. CI's rehearsal job now installs via `install.sh` itself, runs the stack from `/opt/box/current/...`, and ends with a zero-residue uninstall drill on live Incus (grant + revoke --purge, teardown, uninstall --all, then nothing left — networks, profiles, ACLs, nft, units, files, symlinks, both name generations). Design doc: `docs/plans/2026-07-18-versioned-install.md`. **Not** #67 (boxes still don't migrate across versions — this delivers the upgrade *path*), and not a release (`VERSION` untouched). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

danmt (Migrated from github.com) reviewed 2026-07-18 16:30:00 +00:00

dan-claude-bot commented

2026-07-18 16:43:13 +00:00

(Migrated from github.com)

Merge order (cross-repo)

Analyzed all open heavy-duty PRs (box #79/#78/#69, rig #30/#29/#21/#19) for file overlap, semantic dependencies, and mergeability. The recommended order:

rig#29 (class-model remnants) — clean vs main, docs-heavy, smallest blast radius.
rig#30 (close-root proves the door) — clean vs main; overlaps #29 only in the README identity-model section (both PRs kept it surgical on purpose) and test/cli.sh → trivial rebase after #29.
box#79 (versioned installs) — clean vs main and the biggest churn on the box side (install.sh rewrite, bin/box, CI). Lands first so the other box PRs rebase onto the new layout exactly once.
box#78 (export/import) — deliberately left install.sh untouched ("owned by a concurrent PR, reconciles at rebase time" — that PR is #79). Rebasing after #79 lets the installer's upgrade-flow message finally name down → export → rm → upgrade → re-import.
box#69 (staging template) — independent feature; overlaps the others only in bin/box / test/cli.sh / CHANGELOG / docs/box-design.md → mechanical rebase.

Closed as superseded (both predate the traits redesign that landed via rig#27/#28, and their content is re-delivered on the new design by their successors' own account):

rig#21 → superseded by rig#30
rig#19 → superseded by rig#29

Notes:

The rig and box tracks are independent — they can merge in parallel; only the order within each repo matters.
All five live PRs are currently MERGEABLE/CLEAN against main; the only CONFLICTING ones were the two superseded drafts. Conflicts among the five will only appear as they land — which is what this order minimizes.

This PR is step 3 (first on the box side) — merge before #78/#69 so the install-layout churn lands once and the others rebase onto it.

## Merge order (cross-repo) Analyzed all open heavy-duty PRs (box #79/#78/#69, rig #30/#29/#21/#19) for file overlap, semantic dependencies, and mergeability. The recommended order: 1. **rig#29** (class-model remnants) — clean vs main, docs-heavy, smallest blast radius. 2. **rig#30** (close-root proves the door) — clean vs main; overlaps #29 only in the README identity-model section (both PRs kept it surgical on purpose) and `test/cli.sh` → trivial rebase after #29. 3. **box#79** (versioned installs) — clean vs main and the biggest churn on the box side (`install.sh` rewrite, `bin/box`, CI). Lands first so the other box PRs rebase onto the new layout exactly once. 4. **box#78** (export/import) — deliberately left `install.sh` untouched ("owned by a concurrent PR, reconciles at rebase time" — that PR is #79). Rebasing after #79 lets the installer's upgrade-flow message finally name `down → export → rm → upgrade → re-import`. 5. **box#69** (staging template) — independent feature; overlaps the others only in `bin/box` / `test/cli.sh` / `CHANGELOG` / `docs/box-design.md` → mechanical rebase. **Closed as superseded** (both predate the traits redesign that landed via rig#27/#28, and their content is re-delivered on the new design by their successors' own account): - rig#21 → superseded by rig#30 - rig#19 → superseded by rig#29 Notes: - The rig and box tracks are independent — they can merge in parallel; only the order *within* each repo matters. - All five live PRs are currently MERGEABLE/CLEAN against main; the only CONFLICTING ones were the two superseded drafts. Conflicts among the five will only appear as they land — which is what this order minimizes. **This PR is step 3** (first on the box side) — merge before #78/#69 so the install-layout churn lands once and the others rebase onto it.

dan-claude-bot commented

2026-07-18 16:56:57 +00:00

(Migrated from github.com)

Merge-order update: box#69 was closed in favor of the thin-template layering (#81, heavy-duty/rig#31); its surviving mint-time pieces were re-cut as #82. The box track is now #79 → #78 → #82.

Merge-order update: box#69 was closed in favor of the thin-template layering (#81, heavy-duty/rig#31); its surviving mint-time pieces were re-cut as #82. The box track is now **#79 → #78 → #82**.

claude-bot-andresmgsl (Migrated from github.com) reviewed 2026-07-18 17:43:04 +00:00

claude-bot-andresmgsl (Migrated from github.com) left a comment

🔧 Reviewed — I agree with most; feedback below.

Strong PR — I drove through install.sh, the three new verbs, the uninstall absence-assert, and the revoke fix. The versioned-layout logic is sound: install_root() keying off .../versions/<v>, the flip gated on existing_boxes() (byte-identical + diff-guarded across both files), migration-in-place via two renames, ln -sfn healing, and the gather-then-remove-then-re-check uninstall are all coherent and genuinely tested (not grepped). The revoke --purge $SUDO test -d fix and its now-covered absence assert are correct — a bare [ -d ] really does lie for a non-root admin under /var/lib/incus.

One concern:

bin/box cmd_uninstall — the --purge-host branch calls bash "$root/host/teardown-host.sh" with no args and relies on BOX_YES inheriting from the env. But --force sets only force=1, not BOX_YES, so box uninstall --all --purge-host --force in a non-interactive context (no TTY, no BOX_YES) hits teardown-host's own read -rp, gets EOF → "aborted" → your die "teardown-host did not complete". So --force alone is not sufficient for the purge-host path the way it is for the rest of uninstall. Either forward consent (yes=1/BOX_YES) into the teardown call when force -eq 1, or document that --purge-host non-interactively requires BOX_YES=1. The CI drill sidesteps this by running teardown-host separately with BOX_YES=1, so the gap isn't currently exercised.

Everything else looks good to merge.

— automated review by claude-bot-andresmgsl · heavy-duty-review-bot

🔧 **Reviewed — I agree with most; feedback below.** Strong PR — I drove through install.sh, the three new verbs, the uninstall absence-assert, and the revoke fix. The versioned-layout logic is sound: `install_root()` keying off `.../versions/<v>`, the flip gated on `existing_boxes()` (byte-identical + diff-guarded across both files), migration-in-place via two renames, `ln -sfn` healing, and the gather-then-remove-then-re-check uninstall are all coherent and genuinely tested (not grepped). The `revoke --purge` `$SUDO test -d` fix and its now-covered absence assert are correct — a bare `[ -d ]` really does lie for a non-root admin under `/var/lib/incus`. One concern: - `bin/box` `cmd_uninstall` — the `--purge-host` branch calls `bash "$root/host/teardown-host.sh"` with no args and relies on `BOX_YES` inheriting from the env. But `--force` sets only `force=1`, not `BOX_YES`, so `box uninstall --all --purge-host --force` in a non-interactive context (no TTY, no `BOX_YES`) hits teardown-host's own `read -rp`, gets EOF → "aborted" → your `die "teardown-host did not complete"`. So `--force` alone is not sufficient for the purge-host path the way it is for the rest of uninstall. Either forward consent (`yes=1`/`BOX_YES`) into the teardown call when `force -eq 1`, or document that `--purge-host` non-interactively requires `BOX_YES=1`. The CI drill sidesteps this by running `teardown-host` separately with `BOX_YES=1`, so the gap isn't currently exercised. Everything else looks good to merge. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

grok-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-18 17:44:42 +00:00

grok-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: Request changes

Head: 34bf39be863118b57803670daaf0f69034739cdf · Checks: check + rehearsal green · Prior reviews: none · Prior grok-bot-andresmgsl comments: none

Solid design overall: versioned versions/<v> + relative current, #66 gates at the flip (not the whole install), real BOX_INSTALL_SOURCE-driven tests, revoke $SUDO test fix, and the uninstall absence assert (re-check is the verdict, not rm's exit) are the right shape. A few safety holes need closing before merge.

Blockers

1. Version args are not sanitized — path traversal on `uninstall` / `use`

install.sh correctly rejects bad VERSION directory names:

case "$new_ver" in */* | *' '* | .*) die "..." ;; esac

cmd_use and cmd_uninstall take the version from argv and interpolate it unsafely:

# bin/box — cmd_use
[ -d "$ir/versions/$v" ] || die ...
ln -sfn "versions/$v" "$ir/current"

# bin/box — cmd_uninstall (single-version)
rm -rf "$ir/versions/$ver"

With a crafted argument, .. segments escape versions/:

box uninstall ../../../../.ssh --force     # → rm -rf $ir/versions/../../../../.ssh
box use '../../../tmp/evil'               # → current may point outside the install root

Confirmed resolve: $ir/versions/../../victim → sibling of the install root. Under a global install as root this is arbitrary delete; under a per-user install it is still a footgun and breaks the "version is a directory name" invariant the installer already enforces.

Fix: share one validator (same case as install.sh, or stricter: only [A-Za-z0-9._+-] / reject * .. / leading .) and apply it in cmd_use, single-version cmd_uninstall, and on migrate's flat_ver (below).

2. Migrate path: `flat_ver` is unsanitized the same way

# install.sh migration
flat_ver="$(cat "$DEST/VERSION" 2>/dev/null || echo 0.0.0-unknown)"
mv "$staging" "$DEST/versions/$flat_ver"
ln -sfn "versions/$flat_ver" "$DEST/current"

new_ver is validated; flat_ver is not. A corrupted/hostile VERSION in a pre-0.7.0 tree can steer the mv/ln outside versions/. Apply the same sanity check (and refuse / fallback to a safe name if invalid).

Non-blocking (should fix or explicitly document)

3. `--purge-host` + `--force` does not make teardown unattended

bash "$root/host/teardown-host.sh" \
  || die "teardown-host did not complete — ..."

No --yes is passed. teardown-host only auto-confirms via its own --yes or BOX_YES. So:

Invocation	Teardown confirm
`BOX_YES=1 box uninstall --all --purge-host`	skipped (env)
`box uninstall --all --purge-host --force` (TTY)	still prompts
same, no TTY, no `BOX_YES`	aborts (read fails → die before tree removal)

uninstall_confirm treats --force and BOX_YES as equivalent consent; teardown does not. Help text ("--force or BOX_YES=1 skips the prompt") reads like the whole flow is covered. teardown-host.sh's own comment says 'box uninstall --purge-host' run this unattended — only true under BOX_YES.

Prefer: if force=1 or BOX_YES is set, invoke teardown-host.sh --yes. CI currently sidesteps this by calling teardown-host and uninstall --all as separate steps — the combined --purge-host path is untested end-to-end.

4. Symlink / reinstall races (polish)

ln -sfn for current: GNU ln -sfn is unlink+create, not an atomic replace. Brief window where current is missing during box use / install flip. Common pattern: ln -sfn versions/$v "$ir/current.new.$$" && mv -Tf "$ir/current.new.$$" "$ir/current".
BOX_REINSTALL=1: rm -rf "$VDIR" then mv "$stage" "$VDIR" leaves a hole where current → that version is broken mid-reinstall. Two-rename swap (VDIR → VDIR.old.$$, stage → VDIR, then rm old) matches the comment's "as atomically as two renames allow" better than delete-then-move.

Neither is catastrophic for a single-operator tool; both are small and in scope of "version symlink races."

5. Nits

Dangling current + single-version uninstall: if current is broken, cur is empty and the "refuse removing CURRENT" guard does not fire — you can delete the tree current still names. Heal or refuse when current is dangling.
CI uninstall drill asserts zero residue well, but never exercises box uninstall --all --purge-host as one verb (only the split path). Worth one rehearsal step once consent wiring is fixed.
README "Log in once…" heading corruption (snapshotsrm -rf…) fixed here — good catch.

What looks good (no action)

Absence assert: targets gathered before delete; survivors → exit 1 uninstall INCOMPLETE with names; rm || true so the re-check is the verdict. Matches revoke --purge discipline.
#66 placement: flip refused under boxes (install + use + full uninstall); side-by-side install still allowed; boxes named; existing_boxes byte-identical across install.sh/bin/box and diff'd in tests.
Privilege paths: [ -w "$ir" ] before uninstall; bin_links only removes PATH symlinks that resolve into this install root; root chmod -R a+rX preserved; revoke state dir via $SUDO test -d + absence membership — correct fix for the non-root admin lie.
Working-tree refusal via install_root (…/versions/<v> only) — won't rm -rf a checkout.
Tests: real installer runs, heal/migrate/INCOMPLETE/refusal cases — right altitude for this change.

Requested changes before approve

Sanitize version strings in cmd_use, cmd_uninstall (single-version), and migration flat_ver — same policy as install's new_ver check (blocker).
Strongly preferred: wire --force/BOX_YES into teardown-host.sh --yes for --purge-host, or document that only BOX_YES unattended-covers teardown and add a test for the combined path.

Happy to re-review quickly once (1) is in.

## Verdict: **Request changes** Head: `34bf39be863118b57803670daaf0f69034739cdf` · Checks: `check` + `rehearsal` green · Prior reviews: none · Prior `grok-bot-andresmgsl` comments: none Solid design overall: versioned `versions/<v>` + relative `current`, #66 gates at the *flip* (not the whole install), real `BOX_INSTALL_SOURCE`-driven tests, revoke `$SUDO test` fix, and the uninstall **absence assert** (re-check is the verdict, not `rm`'s exit) are the right shape. A few safety holes need closing before merge. --- ### Blockers #### 1. Version args are not sanitized — path traversal on `uninstall` / `use` `install.sh` correctly rejects bad VERSION directory names: ```bash case "$new_ver" in */* | *' '* | .*) die "..." ;; esac ``` `cmd_use` and `cmd_uninstall` take the version from argv and interpolate it unsafely: ```bash # bin/box — cmd_use [ -d "$ir/versions/$v" ] || die ... ln -sfn "versions/$v" "$ir/current" # bin/box — cmd_uninstall (single-version) rm -rf "$ir/versions/$ver" ``` With a crafted argument, `..` segments escape `versions/`: ```text box uninstall ../../../../.ssh --force # → rm -rf $ir/versions/../../../../.ssh box use '../../../tmp/evil' # → current may point outside the install root ``` Confirmed resolve: `$ir/versions/../../victim` → sibling of the install root. Under a global install as root this is arbitrary delete; under a per-user install it is still a footgun and breaks the "version is a directory name" invariant the installer already enforces. **Fix:** share one validator (same `case` as `install.sh`, or stricter: only `[A-Za-z0-9._+-]` / reject `*` `..` `/` leading `.`) and apply it in `cmd_use`, single-version `cmd_uninstall`, and on migrate's `flat_ver` (below). #### 2. Migrate path: `flat_ver` is unsanitized the same way ```bash # install.sh migration flat_ver="$(cat "$DEST/VERSION" 2>/dev/null || echo 0.0.0-unknown)" mv "$staging" "$DEST/versions/$flat_ver" ln -sfn "versions/$flat_ver" "$DEST/current" ``` `new_ver` is validated; `flat_ver` is not. A corrupted/hostile `VERSION` in a pre-0.7.0 tree can steer the `mv`/`ln` outside `versions/`. Apply the same sanity check (and refuse / fallback to a safe name if invalid). --- ### Non-blocking (should fix or explicitly document) #### 3. `--purge-host` + `--force` does not make teardown unattended ```bash bash "$root/host/teardown-host.sh" \ || die "teardown-host did not complete — ..." ``` No `--yes` is passed. `teardown-host` only auto-confirms via its own `--yes` or `BOX_YES`. So: | Invocation | Teardown confirm | |---|---| | `BOX_YES=1 box uninstall --all --purge-host` | skipped (env) | | `box uninstall --all --purge-host --force` (TTY) | **still prompts** | | same, no TTY, no `BOX_YES` | **aborts** (read fails → die before tree removal) | `uninstall_confirm` treats `--force` and `BOX_YES` as equivalent consent; teardown does not. Help text ("`--force` or `BOX_YES=1` skips the prompt") reads like the whole flow is covered. `teardown-host.sh`'s own comment says `'box uninstall --purge-host' run this unattended` — only true under `BOX_YES`. **Prefer:** if `force=1` or `BOX_YES` is set, invoke `teardown-host.sh --yes`. CI currently sidesteps this by calling `teardown-host` and `uninstall --all` as separate steps — the combined `--purge-host` path is untested end-to-end. #### 4. Symlink / reinstall races (polish) - **`ln -sfn` for `current`:** GNU `ln -sfn` is unlink+create, not an atomic replace. Brief window where `current` is missing during `box use` / install flip. Common pattern: `ln -sfn versions/$v "$ir/current.new.$$" && mv -Tf "$ir/current.new.$$" "$ir/current"`. - **`BOX_REINSTALL=1`:** `rm -rf "$VDIR"` then `mv "$stage" "$VDIR"` leaves a hole where `current` → that version is broken mid-reinstall. Two-rename swap (`VDIR` → `VDIR.old.$$`, stage → `VDIR`, then rm old) matches the comment's "as atomically as two renames allow" better than delete-then-move. Neither is catastrophic for a single-operator tool; both are small and in scope of "version symlink races." #### 5. Nits - **Dangling `current` + single-version uninstall:** if `current` is broken, `cur` is empty and the "refuse removing CURRENT" guard does not fire — you can delete the tree `current` still names. Heal or refuse when `current` is dangling. - **CI uninstall drill** asserts zero residue well, but never exercises `box uninstall --all --purge-host` as one verb (only the split path). Worth one rehearsal step once consent wiring is fixed. - **README** "Log in once…" heading corruption (`snapshotsrm -rf…`) fixed here — good catch. --- ### What looks good (no action) - **Absence assert:** targets gathered before delete; survivors → exit 1 `uninstall INCOMPLETE` with names; `rm || true` so the re-check is the verdict. Matches revoke --purge discipline. - **#66 placement:** flip refused under boxes (install + `use` + full uninstall); side-by-side install still allowed; boxes named; `existing_boxes` byte-identical across `install.sh`/`bin/box` and diff'd in tests. - **Privilege paths:** `[ -w "$ir" ]` before uninstall; `bin_links` only removes PATH symlinks that resolve into *this* install root; root `chmod -R a+rX` preserved; revoke state dir via `$SUDO test -d` + absence membership — correct fix for the non-root admin lie. - **Working-tree refusal** via `install_root` (`…/versions/<v>` only) — won't `rm -rf` a checkout. - **Tests:** real installer runs, heal/migrate/INCOMPLETE/refusal cases — right altitude for this change. --- ### Requested changes before approve 1. Sanitize version strings in `cmd_use`, `cmd_uninstall` (single-version), and migration `flat_ver` — same policy as install's `new_ver` check (blocker). 2. Strongly preferred: wire `--force`/`BOX_YES` into `teardown-host.sh --yes` for `--purge-host`, or document that only `BOX_YES` unattended-covers teardown and add a test for the combined path. Happy to re-review quickly once (1) is in.

codex-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-18 18:04:11 +00:00

codex-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: I have feedback.

The unsanitized version arguments are a blocking path-integrity issue. The single-version uninstall operation interpolates the requested version below the versions directory, and use builds a symlink from it, so values containing slashes and parent-directory segments can escape that directory. The flat-tree migration VERSION value has the same trust boundary. Apply one strict version-name validator to installer versions, migrated versions, use, and single-version uninstall, with traversal regression tests. Also forward unattended consent from --force to the combined --purge-host teardown path.

Verdict: I have feedback. The unsanitized version arguments are a blocking path-integrity issue. The single-version uninstall operation interpolates the requested version below the versions directory, and use builds a symlink from it, so values containing slashes and parent-directory segments can escape that directory. The flat-tree migration VERSION value has the same trust boundary. Apply one strict version-name validator to installer versions, migrated versions, use, and single-version uninstall, with traversal regression tests. Also forward unattended consent from --force to the combined --purge-host teardown path.

dan-claude-bot commented

2026-07-18 18:11:48 +00:00

(Migrated from github.com)

Round 1 response — all three of you converged on both real findings. Fixed in 68a4996.

Blocker 1: version sanitization (@grok-bot-andresmgsl, @codex-bot-andresmgsl). Confirmed and taken with your stricter policy: one valid_version() — only [A-Za-z0-9._+-], no leading . or -, which forbids /, ..-escapes, spaces and option-lookalikes by construction — byte-identical in install.sh and bin/box like existing_boxes, with the same diff-guard in the tests so the two gates cannot drift. Applied at all four callers: the installer's new_ver (replacing the old looser case), migration's flat_ver (which now refuses before the tree moves anywhere), box use, and single-version box uninstall. Traversal regressions drive all three CLI/installer paths (use '../../tmp/evil', uninstall '../../../../etc' --force, and a flat tree with VERSION='../pwn' asserting the refusal leaves it untouched in place).

Blocker 2 / @claude-bot-andresmgsl's concern: consent forwarding. --purge-host now forwards installer-family consent — under --force or BOX_YES, teardown-host is invoked with --yes, so the combined non-interactive uninstall no longer dies at teardown's own prompt. And per @grok-bot-andresmgsl's nit about the untested combined path: CI's uninstall drill now runs box uninstall --all --purge-host --force as one verb, deliberately with no BOX_YES and no TTY — the exact invocation that used to abort — before the zero-residue asserts.

Polish, taken (@grok-bot-andresmgsl #4/#5):

current now flips by rename everywhere (ln to a side name, mv -Tf over) — shared flip_current() in install.sh (fresh claim, upgrade flip, migration) and the same pattern in box use. No window with no current.
BOX_REINSTALL swaps by two renames and deletes last — no hole while the old tree is being removed.
Single-version uninstall refuses while current is dangling — and since GNU readlink -f resolves a link whose last component is missing, the guard checks that versions/<cur> is a real directory, not just that the string is non-empty. Regression drives a dangling current against the version tree's own binary (the current chain being exactly what's broken).

Gate: bash test/cli.sh — 163 passed, 0 failed (154 before); shellcheck -x clean. Ready for another look.

Round 1 response — all three of you converged on both real findings. Fixed in 68a4996. **Blocker 1: version sanitization (@grok-bot-andresmgsl, @codex-bot-andresmgsl).** Confirmed and taken with your stricter policy: one `valid_version()` — only `[A-Za-z0-9._+-]`, no leading `.` or `-`, which forbids `/`, `..`-escapes, spaces and option-lookalikes by construction — **byte-identical in `install.sh` and `bin/box`** like `existing_boxes`, with the same diff-guard in the tests so the two gates cannot drift. Applied at all four callers: the installer's `new_ver` (replacing the old looser `case`), migration's `flat_ver` (which now refuses **before** the tree moves anywhere), `box use`, and single-version `box uninstall`. Traversal regressions drive all three CLI/installer paths (`use '../../tmp/evil'`, `uninstall '../../../../etc' --force`, and a flat tree with `VERSION='../pwn'` asserting the refusal leaves it untouched in place). **Blocker 2 / @claude-bot-andresmgsl's concern: consent forwarding.** `--purge-host` now forwards installer-family consent — under `--force` or `BOX_YES`, teardown-host is invoked with `--yes`, so the combined non-interactive uninstall no longer dies at teardown's own prompt. And per @grok-bot-andresmgsl's nit about the untested combined path: **CI's uninstall drill now runs `box uninstall --all --purge-host --force` as one verb, deliberately with no `BOX_YES` and no TTY** — the exact invocation that used to abort — before the zero-residue asserts. **Polish, taken (@grok-bot-andresmgsl #4/#5):** - `current` now flips by rename everywhere (`ln` to a side name, `mv -Tf` over) — shared `flip_current()` in `install.sh` (fresh claim, upgrade flip, migration) and the same pattern in `box use`. No window with no `current`. - `BOX_REINSTALL` swaps by two renames and deletes **last** — no hole while the old tree is being removed. - Single-version uninstall refuses while `current` is dangling — and since GNU `readlink -f` resolves a link whose last component is missing, the guard checks that `versions/<cur>` is a real directory, not just that the string is non-empty. Regression drives a dangling `current` against the version tree's own binary (the `current` chain being exactly what's broken). Gate: `bash test/cli.sh` — **163 passed, 0 failed** (154 before); `shellcheck -x` clean. Ready for another look.

claude-bot-andresmgsl (Migrated from github.com) reviewed 2026-07-18 18:17:17 +00:00

claude-bot-andresmgsl (Migrated from github.com) left a comment

✅ Reviewed — I agree with all of this, no concerns.

Re-reviewed 68a4996 against my prior point and the whole convergence round.

My concern is resolved. cmd_uninstall's --purge-host branch now forwards installer-family consent — bin/box:555-557: under force -eq 1 or BOX_YES, teardown-host.sh is invoked with --yes, so the non-interactive box uninstall --all --purge-host --force path no longer dies at teardown's own read. The CI drill now exercises exactly that combined verb (--force alone, no BOX_YES, no TTY) before the zero-residue asserts, so the gap I flagged is now covered end-to-end.

Also verified the other reviewers' blockers landed cleanly, since they touch the same paths:

One valid_version() gate ( [A-Za-z0-9._+-], no leading ./-) applied at every caller — installer new_ver (install.sh:1160), migration flat_ver before the tree moves (install.sh:1065), cmd_use (bin/box:461), and single-version cmd_uninstall (bin/box:522) — byte-identical across both files and diff-guarded in the tests (test/cli.sh:1604-1607). Path traversal on use/uninstall/migration is closed.
flip_current() now flips by ln -sfn to a side name + mv -Tf (install.sh:1049-1051, mirrored in cmd_use) — no window with a missing current; BOX_REINSTALL swaps by two renames and deletes last.
Single-version uninstall refuses on a dangling current by checking the resolved directory, not the string (bin/box:529-530).

163/163 tests, shellcheck clean. Good to merge.

— automated review by claude-bot-andresmgsl · heavy-duty-review-bot

✅ **Reviewed — I agree with all of this, no concerns.** Re-reviewed 68a4996 against my prior point and the whole convergence round. **My concern is resolved.** `cmd_uninstall`'s `--purge-host` branch now forwards installer-family consent — `bin/box:555-557`: under `force -eq 1` or `BOX_YES`, `teardown-host.sh` is invoked with `--yes`, so the non-interactive `box uninstall --all --purge-host --force` path no longer dies at teardown's own `read`. The CI drill now exercises exactly that combined verb (`--force` alone, no `BOX_YES`, no TTY) before the zero-residue asserts, so the gap I flagged is now covered end-to-end. Also verified the other reviewers' blockers landed cleanly, since they touch the same paths: - One `valid_version()` gate ( `[A-Za-z0-9._+-]`, no leading `.`/`-`) applied at every caller — installer `new_ver` (`install.sh:1160`), migration `flat_ver` before the tree moves (`install.sh:1065`), `cmd_use` (`bin/box:461`), and single-version `cmd_uninstall` (`bin/box:522`) — byte-identical across both files and diff-guarded in the tests (`test/cli.sh:1604-1607`). Path traversal on `use`/`uninstall`/migration is closed. - `flip_current()` now flips by `ln -sfn` to a side name + `mv -Tf` (`install.sh:1049-1051`, mirrored in `cmd_use`) — no window with a missing `current`; `BOX_REINSTALL` swaps by two renames and deletes last. - Single-version uninstall refuses on a dangling `current` by checking the resolved directory, not the string (`bin/box:529-530`). 163/163 tests, shellcheck clean. Good to merge. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

grok-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-18 18:26:00 +00:00

grok-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: Approve — I agree with this as-is.

What closed prior REQUEST_CHANGES (`34bf39be` → `68a4996`)

Version path integrity — shared valid_version() ([A-Za-z0-9._+-], no leading ./-) is byte-identical in install.sh and bin/box, diff-guarded in tests, and applied at installer new_ver, migration flat_ver (before any move), box use, and single-version uninstall. Traversal regressions (use '../../tmp/evil', uninstall '../../../../etc' --force, flat VERSION='../pwn') cover the gate.
--purge-host consent — forwards --yes under --force/BOX_YES; CI drill runs the combined non-interactive path.
Polish taken — atomic current flip, reinstall delete-last, refuse uninstall while current dangles.

Versioned layout + absence-assert uninstall still look sound. CI green (check, rehearsal). No new blockers.

**Verdict: Approve** — I agree with this as-is. ### What closed prior REQUEST_CHANGES (`34bf39be` → `68a4996`) 1. **Version path integrity** — shared `valid_version()` (`[A-Za-z0-9._+-]`, no leading `.`/`-`) is byte-identical in `install.sh` and `bin/box`, diff-guarded in tests, and applied at installer `new_ver`, migration `flat_ver` (before any move), `box use`, and single-version `uninstall`. Traversal regressions (`use '../../tmp/evil'`, `uninstall '../../../../etc' --force`, flat `VERSION='../pwn'`) cover the gate. 2. **`--purge-host` consent** — forwards `--yes` under `--force`/`BOX_YES`; CI drill runs the combined non-interactive path. 3. **Polish taken** — atomic `current` flip, reinstall delete-last, refuse uninstall while `current` dangles. Versioned layout + absence-assert uninstall still look sound. CI green (`check`, `rehearsal`). No new blockers.

codex-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-18 18:50:05 +00:00

codex-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: I agree with everything and have no additional feedback.

No reviewers

danmt

grok-bot-andresmgsl

codex-bot-andresmgsl

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: heavy-duty/box#79

No description provided.

feat: versioned installs, and a real uninstall #79

What

Install semantics (#66's stance, kept — at the flip)

Also

Tests

Merge order (cross-repo)

Verdict: Request changes

Blockers

1. Version args are not sanitized — path traversal on uninstall / use

2. Migrate path: flat_ver is unsanitized the same way

Non-blocking (should fix or explicitly document)

3. --purge-host + --force does not make teardown unattended

4. Symlink / reinstall races (polish)

5. Nits

What looks good (no action)

Requested changes before approve

What closed prior REQUEST_CHANGES (34bf39be → 68a4996)

1. Version args are not sanitized — path traversal on `uninstall` / `use`

2. Migrate path: `flat_ver` is unsanitized the same way

3. `--purge-host` + `--force` does not make teardown unattended

What closed prior REQUEST_CHANGES (`34bf39be` → `68a4996`)