Merge pull request #109 from dan-claude-bot/fix/restore-confirm

fix: box restore asks before it destroys, in its own words (#105)
This commit is contained in:
Daniel Marin 2026-07-19 21:25:59 +01:00 committed by GitHub
commit 9ea50d21e2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 219 additions and 16 deletions

View file

@ -34,6 +34,43 @@ which records not just what changed but what each drill run proved.
### Fixed
- **`box restore` asks before it destroys — and the confirmation prompt is
now the row's, not rm's** (#105) — `restore` and `rm` both irreversibly
discard user state, and only one of them asked. The table gave `restore`
the preconditions `box,arg2`: the instance is ours, a snapshot name is
present, go. So `box restore work stale-label` silently threw away
everything done in the box since that snapshot, with no prompt, no
`--force`, and no way to take it back — a warning in `--help` is not a
gate. It has been that way since the verb shipped, and it is about to
become routine rather than rare (heavy-duty/rig#62's pristine snapshot),
which is the wrong time to still be relying on the operator typing the
right label. The reason it stayed ungated is worth recording, because it
is the actual bug: `confirm` was already a precondition token, but the
dispatch line hardcoded the *words* — `confirm "delete $inst and all its
snapshots"` — so the one-token fix would have gated restore behind a
prompt offering to DELETE the box the operator was trying to rescue. A
gate that names the wrong act is worse than no gate; it is how people
learn to answer `y` without reading. So the prompt moved into the table
as a seventh field, each row saying what it is about to do in its own
words, and `restore` now asks to "roll `<box>` back to snapshot
`<label>` and discard everything in the box since it was taken" — naming
the label, because picking the wrong one is the whole risk. `rm`'s
wording is unchanged and pinned verbatim by a test, since rewording the
one verb that already worked would be a regression shipped as a
refactor. A row marked `confirm` with no words is now a hard internal
error rather than a blank question. `--force` and the no-TTY refusal come
free — `confirm()` already had both. The one automated caller had to
consent explicitly: `drill/multiuser.sh` drives restore unattended on real
Incus and now passes `--force`, which is the rehearsal proving the gate
rather than working around it — the CI run of this very PR failed there
first, which is the shape a gate is supposed to have. Coverage went from two
argument-validation checks that never reached dispatch to the destructive
path itself, driven against a fake incus: refusing leaves the call log
empty, `--force` produces exactly one `incus snapshot restore`. Not
changed, deliberately: `restore` still does not require the box stopped
(#105 makes that case separately and it deserves its own call), and
`--help` now says plainly that a rollback of a running box is
crash-consistent, because these snapshots are stateless.
- **`box-firewall` could hand a UFW host the no-UFW firewall, ~2% of the
time** (#102) — filed as an intermittent test flake (`test/cli.sh`'s
fresh-UFW block going four-assertions-red on an unmodified `main`,

View file

@ -320,7 +320,10 @@ box new --name feature --from work/authed # clone the authed state into a new
`--from` copies the whole box (agent login, git creds, clones and all) while
preserving isolation. You can also `box new --name x --from work` to clone
a box's live state, or roll a box back with `box restore work authed`.
a box's live state, or roll a box back with `box restore work authed` — which
asks first, since a rollback discards everything since the snapshot (`--force`
skips the prompt, and scripts must pass it: with no terminal to ask on, box
refuses rather than assuming yes).
Forgotten what you called a checkpoint? `box info work` prints the box's
snapshot labels and the `--from` line to clone one.
@ -388,7 +391,8 @@ box shell <box> # enter as the template's user
box exec <box> -- <cmd...> # run a command in the box
box tmux <box> [session] # attach/create a tmux session — survives disconnects
box snapshot <box> [label] # checkpoint (label defaults to manual-<epoch>)
box restore <box> <snap> # roll back to a snapshot
box restore <box> <snap> [--force]
# roll back to a snapshot — destructive, asks first
box export <box> [<file>] [--instance-only]
# one portable file (snapshots incl.) — survives rm & host
box import <file> [--name <box>]

66
bin/box
View file

@ -35,7 +35,7 @@ box_tier() {
# ---------------------------------------------------------------------------
# The command table.
#
# verb ^ synopsis args ^ preconditions ^ one-line summary ^ action ^ ok message
# verb ^ synopsis args ^ preconditions ^ one-line summary ^ action ^ ok message ^ confirm prompt
#
# Fields are ^-separated because a synopsis may contain '|' ([--vm|--container]).
#
@ -44,7 +44,8 @@ box_tier() {
# isn't tagged user.box=1 (or the legacy user.claudebox=1) — the boundary, enforced, not assumed
# arg2 a second positional is required
# stopped the box must not be running
# confirm destructive: prompt unless --force
# confirm destructive: prompt unless --force. The row supplies the words
# (last field) — see 'confirm prompt' below.
#
# action:
# incus:<subcommand> run `incus <subcommand> <instance> [rest...]`
@ -52,6 +53,19 @@ box_tier() {
#
# ok message: printed on success; {} = the box, {1} = the second positional.
#
# confirm prompt: the words the 'confirm' precondition asks with, phrased as
# the act ("delete X", "roll X back to Y") — confirm() wraps it into
# "box: <prompt>? this cannot be undone." and into the no-TTY refusal
# ("refusing to <prompt> without --force"). It is a per-row field and not a
# shared string on purpose: the prompt was hardcoded to rm's wording once, and
# the whole reason 'restore' shipped ungated for four releases is that adding
# the token to its row would have asked the operator to confirm DELETING the
# box they were trying to rescue (#105). A gate that names the wrong act is
# worse than no gate — it teaches people to answer 'y' without reading. Same
# substitutions as the ok message, except {} is the RESOLVED instance: the
# prompt names the thing about to be destroyed, so under --remote it should
# say 'lab:work', not 'work'.
#
# Adding a thin verb is one row. If a request can't be expressed as a row and
# doesn't enforce a box invariant, it is incus's job, not ours — that is
# what `box incus` is for.
@ -64,13 +78,13 @@ CMDS=(
"exec^<box> -- <cmd...>^box^Run a command inside a box^fn:cmd_exec^"
"tmux^<box> [<session>]^box^Attach or create a tmux session in a box — survives disconnects^fn:cmd_tmux^"
"snapshot^<box> [<label>]^box^Checkpoint a box (label defaults to manual-<epoch>)^fn:cmd_snapshot^"
"restore^<box> <snapshot>^box,arg2^Roll a box back to one of its snapshots^incus:snapshot restore^restored {} to {1}"
"restore^<box> <snapshot> [--force]^box,arg2,confirm^Roll a box back to one of its snapshots — irreversible, and it asks first^incus:snapshot restore^restored {} to {1}^roll {} back to snapshot '{1}' and discard everything in the box since it was taken"
"export^<box> [<file>] [--instance-only]^box^Export a stopped box to one portable file — it survives 'box rm' and this host^fn:cmd_export^"
"import^<file> [--name <box>]^^Mint a box from an exported file, re-stamped onto this host's stack^fn:cmd_import^"
"rename^<box> <new-name>^box,arg2,stopped^Rename a box (it must be stopped first)^incus:rename^renamed {} to {1}"
"down^<box>^box^Stop a box, keeping its state ('start' resumes it)^incus:stop^stopped {}"
"start^<box>^box^Start a stopped box^incus:start^started {}"
"rm^<box> [--force]^box,confirm^Delete a box and its snapshots — irreversible, and it asks first^incus:delete -f^removed {}"
"rm^<box> [--force]^box,confirm^Delete a box and its snapshots — irreversible, and it asks first^incus:delete -f^removed {}^delete {} and all its snapshots"
"expose^<box> <port> [<host-port>] | --list | --remove <port>^box^Forward a box port to the host's loopback — see a dev server^fn:cmd_expose^"
"incus^<box> -- <args...>^box^Escape hatch: run any incus command against a box^fn:cmd_incus^"
"doctor^[--fix | --pin-dns]^^Is this host fit to mint boxes? Diagnose the daemon, network, DNS, isolation^fn:cmd_doctor^"
@ -92,12 +106,12 @@ is_command() { cmd_row "$1" >/dev/null 2>&1; }
# locals matter here: dispatch holds $pre/$action/$ok, and field() is called from
# error paths inside it — a global read would clobber the row being dispatched.
field() {
local r f_syn f_pre f_sum f_act f_ok
local r f_syn f_pre f_sum f_act f_ok f_cnf
r="$(cmd_row "$1")" || return 1
IFS='^' read -r _ f_syn f_pre f_sum f_act f_ok <<<"$r"
IFS='^' read -r _ f_syn f_pre f_sum f_act f_ok f_cnf <<<"$r"
case "$2" in
syn) echo "$f_syn" ;; pre) echo "$f_pre" ;; sum) echo "$f_sum" ;;
act) echo "$f_act" ;; ok) echo "$f_ok" ;;
act) echo "$f_act" ;; ok) echo "$f_ok" ;; cnf) echo "$f_cnf" ;;
esac
}
synopsis_of() { local s; s="$(field "$1" syn)"; echo "box $1${s:+ $s}"; }
@ -147,7 +161,7 @@ COMMANDS
EOF
local r v sum
for r in "${CMDS[@]}"; do
IFS='^' read -r v _ _ sum _ _ <<<"$r"
IFS='^' read -r v _ _ sum _ _ _ <<<"$r"
printf ' %-13s %s\n' "$v" "$sum"
done
cat <<'EOF'
@ -161,7 +175,8 @@ OPTIONS
dev/test only. Default where /dev/kvm is absent.
--json Emit Incus JSON instead of a table (list, info)
--instance-only Export the live state only, no snapshots (export)
--force, -f Delete without asking (rm); overwrite the file (export)
--force, -f Destroy without asking (rm, restore); overwrite the
file (export)
--remote <r> Act on Incus remote <r> (any)
--help, -h Help; after a command, help for that command
--version, -V Print the box version
@ -358,6 +373,16 @@ Roll a box back to one of its snapshots, in place. Anything in the box since
that snapshot is lost. 'box info <box>' lists the labels.
box restore work authed
box restore work authed --force
Destructive, so it asks first — naming the snapshot it is rolling back to,
because the whole risk is picking the wrong label. --force (-f) skips the
prompt; with no TTY to ask on it refuses rather than assuming yes.
box does not require the box to be stopped for this. Snapshots here are
stateless — no live memory is captured — so a rollback is crash-consistent:
the box comes back the way a machine comes back from losing power. 'box down
<box>' first if that matters.
EOF
;;
export) cat <<'EOF'
@ -784,6 +809,15 @@ need_arg2() {
fi
}
# Row templates: {} -> $2, {1} -> the second positional. The caller picks what
# {} means because the two uses differ on purpose — the ok message reports on
# the box the operator NAMED, the confirm prompt names the RESOLVED instance,
# because a prompt about to destroy something should say which machine.
fill() { # $1 = template, $2 = what {} stands for
local t="${1//\{\}/$2}"
printf '%s\n' "${t//\{1\}/${args[1]:-}}"
}
confirm() { # $1 = prompt. --force, or a TTY to ask on, or we refuse.
if [ "$force" -eq 1 ]; then return 0; fi
[ -t 0 ] || usage_error "refusing to $1 without --force (no terminal to confirm on)"
@ -1908,12 +1942,19 @@ cmd_expose() {
# --- dispatch: driven by the table, not by a hand-written case --------------
row="$(cmd_row "$cmd")" || unknown_command "$cmd"
IFS='^' read -r _ _ pre _ action ok <<<"$row"
IFS='^' read -r _ _ pre _ action ok cnf <<<"$row"
case ",$pre," in *,box,*) need_name; inst="$(resolve_box "${args[0]}")" ;; esac
case ",$pre," in *,arg2,*) need_arg2 ;; esac
case ",$pre," in *,stopped,*) require_stopped "$inst" "${args[0]}" ;; esac
case ",$pre," in *,confirm,*) confirm "delete $inst and all its snapshots" ;; esac
# The prompt comes from the ROW, never from here. A shared string can only be
# right for one verb, and it was rm's — which is why 'restore' could not be
# gated by adding the token alone (#105). A row with 'confirm' and no words is
# a table bug, and it dies as one rather than asking a blank question.
case ",$pre," in *,confirm,*)
[ -n "$cnf" ] || die "internal: '$cmd' is marked confirm but its row carries no prompt"
confirm "$(fill "$cnf" "$inst")" ;;
esac
case "$action" in
fn:*)
@ -1925,8 +1966,7 @@ case "$action" in
# shellcheck disable=SC2086
incus $sub "$inst" "${args[@]:1}"
if [ -n "$ok" ]; then
msg="${ok//\{\}/${args[0]}}"; msg="${msg//\{1\}/${args[1]:-}}"
echo "box: $msg"
echo "box: $(fill "$ok" "${args[0]}")"
fi
;;
esac

View file

@ -206,7 +206,13 @@ as_u "$U1" box info mine 2>/dev/null | grep -qF "$(boxnet_pfx)" \
&& ok "(g) box info shows a boxnet ($(boxnet_pfx)x) address — placed on the hardened network" \
|| no "(g) mine has no boxnet address in box info"
as_u "$U1" box snapshot mine s1 >/dev/null 2>&1 && ok "(b) box snapshot mine s1" || no "(b) snapshot refused"
as_u "$U1" box restore mine s1 >/dev/null 2>&1 && ok "(b) box restore mine s1 (the incus 6 'snapshot restore' spelling)" || no "(b) restore failed"
# --force, and it is the assertion as much as the fix: restore is destructive
# and now asks (#105), so an unattended drill MUST consent explicitly. Without
# the flag this line fails — correctly — because confirm() refuses rather than
# assuming yes when there is no terminal to ask on. Adding --force here is not
# working around the gate; it is the drill proving the gate is real, and that
# the documented non-interactive path through it works on real Incus.
as_u "$U1" box restore mine s1 --force >/dev/null 2>&1 && ok "(b) box restore mine s1 --force (the incus 6 'snapshot restore' spelling, through the #105 gate)" || no "(b) restore failed"
as_u "$U1" box new --name c1 --from mine/s1 >/dev/null 2>&1 && ok "(b) box new --from mine/s1 — the clone workflow" || no "(b) clone failed"
# c1 stays alive through phase g: it is the distinctly-NAMED sibling the
# enumeration probe needs (both users' primaries are 'mine' by design of d).

View file

@ -776,6 +776,122 @@ check "box exports BOX_TIER to the doctor" 0 "" \
# does not exist and the verb was broken for everyone until #74's rehearsal hit it.
check "restore: dispatches 'incus snapshot restore'" 0 "" \
grep -qF '^incus:snapshot restore^' "$ROOT/bin/box"
# ---------------------------------------------------------------------------
# The confirm gate (#105) — DRIVEN, not grepped.
#
# Until #105 the only coverage restore had was the two argument-validation
# checks above: neither ever reached dispatch, so the verb spent four releases
# handing a running box straight to 'incus snapshot restore' with no prompt
# and no --force, and nothing in this suite could have noticed. Both halves of
# the gate are now exercised against a fake incus that logs what it was asked
# to do — refusing must leave the log EMPTY (an assertion about an absence is
# the only way to prove a gate held), and --force must produce the restore.
#
# Stdin is closed on every run on purpose: confirm() branches on '[ -t 0 ]',
# and a suite run from a terminal would otherwise inherit one and sit there
# waiting for a human to type 'y'.
# ---------------------------------------------------------------------------
CSHIM="$(mktemp -d)"; CWORK="$(mktemp -d)"
cat > "$CSHIM/incus" <<'SHIM'
#!/usr/bin/env bash
# Fake incus for the destructive-path drive. Logs every call, and answers the
# one probe resolve_box makes so a box called 'work' exists and is ours.
[ -n "${FAKE_INCUS_LOG:-}" ] && printf 'incus %s\n' "$*" >> "$FAKE_INCUS_LOG"
case "$*" in
"config get work user.box") echo 1 ;;
"config get "*) exit 1 ;;
esac
exit 0
SHIM
chmod +x "$CSHIM/incus"
runbox() { # runbox <logfile> <args...> — the real box, shimmed, no TTY
local log="$1" rc; shift
: > "$log"
# Output is kept in <log>.out as well as replayed, so a check can assert on
# what the run PRINTED after the fact — check() swallows the output of a run
# it passes, and the "the prompt does not say 'delete'" assertion is exactly
# that: a claim about text from a run that already passed on its exit code.
env FAKE_INCUS_LOG="$log" PATH="$CSHIM:$PATH" "$BOX" "$@" </dev/null >"$log.out" 2>&1
rc=$?
cat "$log.out"
return "$rc"
}
# --- restore: the gate refuses, and nothing is destroyed --------------------
RLOG="$CWORK/restore.log"
check "restore: refuses without --force when there is no terminal (#105)" \
2 "refusing to roll work back to snapshot 'authed'" \
runbox "$RLOG" restore work authed
# The exact no-TTY wording, pinned. This is the regression test for the CI
# failure this PR produced: the multi-user rehearsal drives restore unattended
# on real Incus, took this refusal, and recorded '(b) restore failed' — a
# 40-minute job catching what a 15-second suite should have. box refuses
# rather than assuming consent, and it says which of the two ways out applies.
check "restore: ...and the refusal names the missing terminal, not a bad usage (#105)" \
2 "no terminal to confirm on" \
runbox "$CWORK/r-tty.log" restore work authed
# The load-bearing assertion: the refusal actually PREVENTED the rollback.
# 'grep -q' on an absence, so an empty log passes and a logged restore fails.
check "restore: ...and the refusal reached incus with no restore (#105)" 1 "" \
grep -qF 'snapshot restore' "$RLOG"
# The prompt must name the SNAPSHOT and the loss, not rm's wording. This is
# the entire point of making the prompt row-driven: adding the 'confirm' token
# alone would have asked the operator to confirm deleting the box.
check "restore: the prompt names what is lost, not a deletion (#105)" \
2 "discard everything in the box since it was taken" \
runbox "$CWORK/r2.log" restore work authed
check "restore: the prompt does NOT offer to delete the box (#105)" 1 "" \
grep -qF 'delete work' "$CWORK/r2.log.out"
# --- restore: --force is the way through, and it still restores -------------
FLOG="$CWORK/force.log"
check "restore --force: skips the prompt and restores (#105)" 0 "restored work to authed" \
runbox "$FLOG" restore work authed --force
check "restore --force: ...and incus was really asked for the rollback (#105)" 0 "" \
grep -qF 'incus snapshot restore work authed' "$FLOG"
# --- rm: its wording is unchanged, and its gate still holds -----------------
# #105 moved the prompt out of the dispatch line and into the rows. rm's text
# was the string that lived there, so it is pinned verbatim: a refactor that
# rewords the ONE verb that already asked correctly is a regression.
MLOG="$CWORK/rm.log"
check "rm: still refuses without --force, in its own words (#105 refactor)" \
2 "refusing to delete work and all its snapshots" \
runbox "$MLOG" rm work
check "rm: ...and nothing was deleted" 1 "" grep -qF 'delete' "$MLOG"
check "rm --force: still deletes" 0 "removed work" runbox "$CWORK/rmf.log" rm work --force
check "rm --force: ...via 'incus delete -f'" 0 "" \
grep -qF 'incus delete -f work' "$CWORK/rmf.log"
# --- the table invariant: a confirm row must carry its own words ------------
# Fail-closed on the shape itself, so a future 'confirm' row cannot ship with
# an empty prompt field and inherit whatever the dispatch happens to say.
# shellcheck disable=SC2016 # $3/$7/$1 are awk's fields, not the shell's
check "table: every 'confirm' row supplies a prompt (#105)" 0 "" \
awk -F'^' '
/^CMDS=\(/ { in_t = 1; next }
in_t && /^\)/ { exit }
in_t && /^ "/ && $3 ~ /(^|,)confirm(,|$)/ {
seen = 1
if (NF < 7) { print "row for " $1 " is marked confirm with no prompt field"; bad = 1; next }
p = $7; sub(/"$/, "", p)
if (p == "") { print "row for " $1 " has an empty confirm prompt"; bad = 1 }
}
END { if (!seen) { print "no confirm rows found — the pin is not reading the table"; bad = 1 }
exit (bad ? 1 : 0) }
' "$ROOT/bin/box"
# shellcheck disable=SC2016 # the $-strings are literals in the target file
check "dispatch: the confirm prompt comes from the row, not a constant (#105)" 0 "" \
grep -qF 'confirm "$(fill "$cnf" "$inst")"' "$ROOT/bin/box"
# The rehearsal drives restore unattended on real Incus, so it must consent
# EXPLICITLY — the gate is only real if the one automated caller had to change.
# Pinned here because the rehearsal itself needs a daemon and this suite has none.
check "rehearsal: the unattended restore passes --force (#105)" 0 "" \
grep -qF 'box restore mine s1 --force' "$ROOT/drill/multiuser.sh"
rm -rf "$CSHIM" "$CWORK"
# ---------------------------------------------------------------------------
# export / import (#70) — a box's state that survives the box and the host.
# Usage errors and the pure pre-incus refusals are DRIVEN; every daemon-gated