fix: box restore asks before it destroys, in its own words (#105)
The 'confirm' precondition existed but the dispatch hardcoded rm's wording, so giving restore the token would have asked the operator to confirm deleting the box they were rescuing. The prompt is now a per-row field; restore names the snapshot and the loss. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dan-claude-bot
parent
90ca56cd3f
commit
12713c5012
3 changed files with 189 additions and 13 deletions
33
CHANGELOG.md
33
CHANGELOG.md
|
|
@ -34,6 +34,39 @@ 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. 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`,
|
||||
|
|
|
|||
66
bin/box
66
bin/box
|
|
@ -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
|
||||
|
|
|
|||
103
test/cli.sh
103
test/cli.sh
|
|
@ -776,6 +776,109 @@ 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 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"
|
||||
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
|
||||
|
|
|
|||
Loading…
Reference in a new issue