From 4c595eb94517175850abf7a0b98d66205dc1cec3 Mon Sep 17 00:00:00 2001
From: dan-claude-bot <dan-claude-bot@users.noreply.github.com>
Date: Tue, 21 Jul 2026 11:40:37 +0000
Subject: [PATCH] fix: offer the pristine rollback only when the mark actually
 exists
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both reviewers landed on the same line independently, and they are right.
cmd_new's hook-failure path offered `box restore $name pristine`
unconditionally, but three mints reach that line with no pristine mark, all
by this stack's own design: a `dir` pool (skipped, no CoW),
BOX_SNAPSHOT_PRISTINE=0, and a create incus refused (warned, mint continued).
On a `dir` host that is EVERY hook failure — so the operator standing at the
one moment the message exists for copy-pastes a restore that errors.

It could not simply be read off the return status. The never-fatal contract
means snapshot_mark returns 0 on all four paths on purpose — taken, skipped,
knob-disabled, refused — so `if snapshot_mark ...` cannot distinguish a mark
that exists from one that does not, and making it distinguish would put the
mint's success at the mercy of a checkpoint, which is what that contract
exists to prevent.

So the fact is recorded rather than inferred: `marks` accumulates the labels
snapshot_mark actually created, and `mark_taken <label>` answers the only
question a rollback offer may ask. Per-label, so one mark never answers for
another.

This is the same refusal the message one line earlier already makes when it
declines to invent a `bootstrapped` it never watched — a promise the disk
cannot deliver is the failure mode #130 was built to refuse. It just was not
applied to the restore offer sitting under it.

Pinned per path rather than once, because the three no-mark paths fail
differently and a single case would let the other two regress silently: five
drives over snapshot_mark + mark_taken (created / dir-skip / knob / refused /
no bleed between labels) and a static check that the call site is gated.
Verified by mutation — dropping the recording, and un-gating the offer, each
fail the suite rather than passing quietly.
---
 bin/box     | 24 +++++++++++++++++++++++-
 test/cli.sh | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/bin/box b/bin/box
index 09a279a..92d8d78 100755
--- a/bin/box
+++ b/bin/box
@@ -9,6 +9,12 @@ root="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")/.." && pwd)"
 remote=""; mode="auto"; name=""; from=""; template=""; force=0; json=0; want_help=0
 cpu=""; memory=""; disk=""; instance_only=0
 inst=""   # the resolved Incus instance, set by the 'box' precondition
+# Labels snapshot_mark actually CREATED this run, space-separated. Every path
+# through snapshot_mark returns 0 on purpose — the never-fatal contract — so
+# the exit status cannot tell a mark that was taken from one that was skipped
+# (dir pool, knob=0) or refused (incus said no). Anything that offers the
+# operator a mark must ask this, not assume the call happened.
+marks=""
 
 die() { echo "box: $*" >&2; exit 1; }         # 1 = it went wrong
 usage_error() { echo "box: $*" >&2; echo "try 'box help'." >&2; exit 2; }  # 2 = you asked wrong
@@ -1216,9 +1222,15 @@ snapshot_mark() {
     echo "box: the box is fine; it just has no $label mark. The mint continues." >&2
     return 0
   fi
+  marks="$marks $label"
   echo "box: roll back to it any time with:  box restore $name $label"
 }
 
+# Did THIS run actually create <label>? The only honest source for a message
+# that offers a rollback: see 'marks' above for why the return status cannot
+# answer it.
+mark_taken() { case " $marks " in *" $1 "*) return 0 ;; *) return 1 ;; esac; }
+
 # Take the 'pristine' snapshot, or say loudly why not. Default ON: the value
 # of this mark only exists if it is already there on the bad day, and nobody
 # takes it by hand at the one moment it is true. The escape hatch is the
@@ -1557,7 +1569,17 @@ cmd_new() {
         echo "  and a by-hand re-run happens in a shell it does not watch. Take it yourself once" >&2
         echo "  the role converges, at the moment it is true:" >&2
         echo "  box snapshot $name bootstrapped" >&2
-        echo "box: 'box restore $name pristine' is still there if you would rather start the role over." >&2
+        # Only offer the rollback that EXISTS. Three mints reach this line
+        # with no pristine mark, all by this stack's own design: a 'dir' pool
+        # (skipped), BOX_SNAPSHOT_PRISTINE=0, and a refused create (warned,
+        # mint continued). On a 'dir' host that is EVERY hook failure, and an
+        # operator standing at the one moment this message exists for would
+        # copy-paste a restore that errors. Offering a mark the disk does not
+        # have is the same lie this PR refuses one message earlier, when it
+        # declines to invent a 'bootstrapped' it never watched.
+        if mark_taken pristine; then
+          echo "box: 'box restore $name pristine' is still there if you would rather start the role over." >&2
+        fi
         die "the tenant role did not converge — the box is incomplete, so refusing to call it ready"
       fi
       # The hook RAN and box WATCHED it succeed — so right here the box is
diff --git a/test/cli.sh b/test/cli.sh
index a5a9a3c..ccfff68 100644
--- a/test/cli.sh
+++ b/test/cli.sh
@@ -698,7 +698,7 @@ check "bootstrapped: exactly one auto-mark policy — 'incus snapshot create' tw
 
 # The policy half, DRIVEN. Same stub shape as pris() above, one label over.
 BOOTFN="$(mktemp)"
-awk '/^storage_driver\(\) \{/,/^\}/;/^snapshot_mark\(\) \{/,/^\}/;/^snapshot_bootstrapped\(\) \{/,/^\}/' \
+awk '/^storage_driver\(\) \{/,/^\}/;/^snapshot_mark\(\) \{/,/^\}/;/^mark_taken\(\)/;/^snapshot_bootstrapped\(\) \{/,/^\}/' \
   "$ROOT/bin/box" > "$BOOTFN"
 check "bootstrapped: the functions extracted from bin/box (guards the awk)" 0 "snapshot_bootstrapped" cat "$BOOTFN"
 check "bootstrapped: the extracted functions are valid bash" 0 "" bash -n "$BOOTFN"
@@ -755,6 +755,58 @@ check "bootstrapped: the opt-out is a per-label knob — PRISTINE=0 does not sil
   boot_took_mark btrfs inst-x BOX_SNAPSHOT_PRISTINE=0
 check "bootstrapped: a failed snapshot warns and returns 0 (never fails a good mint)" 0 "WARNING" \
   boot btrfs fail-x
+
+# --- only offer a rollback that EXISTS -------------------------------------
+# The never-fatal contract means snapshot_mark returns 0 whether it took the
+# mark, skipped it, or was refused — so the exit status cannot answer "is
+# there something to restore?" and any message offering one must ask 'marks'.
+# Driven per path rather than asserted once: the three no-mark paths fail
+# differently and a single case would let the other two regress silently.
+took() { # took <driver> <label> [VAR=VAL...] — did THIS run create the mark?
+  local driver="$1" label="$2"; shift 2
+  # shellcheck disable=SC2016  # the body is the stub's source, expanded by the
+  # inner bash from the environment 'env' sets up — never by this shell.
+  env "$@" DRIVER="$driver" LABEL="$label" BOOTFN="$BOOTFN" bash -c '
+    incus() {
+      case "$*" in
+        "profile device get box-net root pool") printf "boxpool\n" ;;
+        "storage show boxpool")
+          [ "$DRIVER" = none ] && return 1
+          printf "name: boxpool\ndriver: %s\n" "$DRIVER" ;;
+        "storage list --format csv")
+          [ "$DRIVER" = none ] && return 1
+          printf "boxpool,%s,,0,CREATED\n" "$DRIVER" ;;
+        "snapshot create fail-x "*) return 1 ;;
+        "snapshot create "*) printf "STUB: snapshot created\n" ;;
+        *) return 1 ;;
+      esac
+    }
+    marks=""
+    . "$BOOTFN"
+    snapshot_mark "$INSTANCE_X" boxname "$LABEL" "${ENABLED:-1}" "some state" >/dev/null 2>&1
+    mark_taken "$LABEL" && echo TAKEN || echo ABSENT
+  ' 2>&1
+}
+check "rollback: a mark that WAS created is remembered" 0 "TAKEN" \
+  took btrfs pristine INSTANCE_X=inst-x
+check "rollback: a 'dir' skip is NOT remembered (no CoW, no mark)" 0 "ABSENT" \
+  took dir pristine INSTANCE_X=inst-x
+check "rollback: the opt-out knob is NOT remembered" 0 "ABSENT" \
+  took btrfs pristine INSTANCE_X=inst-x ENABLED=0
+check "rollback: a REFUSED create is not remembered (incus said no)" 0 "ABSENT" \
+  took btrfs pristine INSTANCE_X=fail-x
+# One label's mark must not answer for another's.
+check "rollback: marks do not bleed between labels" 0 "ABSENT" \
+  bash -c 'marks=" bootstrapped "; . "'"$BOOTFN"'"; mark_taken pristine && echo TAKEN || echo ABSENT'
+# The call site itself, pinned statically: the hook-failure message offers the
+# restore only under the guard. On a 'dir' host EVERY hook failure reaches this
+# line with no pristine mark, so an unconditional offer is a copy-pasteable
+# command that errors at the one moment the operator is standing there.
+# shellcheck disable=SC2016  # the $-strings are literals in the target file
+check "rollback: the hook-failure restore offer is GATED on the mark existing" 0 "" bash -c '
+  awk "/^cmd_new\(\) \{/,/^\}/" "'"$ROOT"'/bin/box" \
+    | grep -B12 "box restore \$name pristine. is still there" \
+    | grep -q "if mark_taken pristine; then"'
 rm -f "$BOOTFN"
 
 # The label is one-directional and the docs must say so: its PRESENCE means the