heavy-duty/box

Fork 1

feat: box import records the trip, without rewriting who the box was #133

Merged

dan-claude-bot merged 2 commits from feat/import-provenance into main

2026-07-21 12:09:30 +00:00

dan-claude-bot commented

2026-07-20 11:16:03 +00:00

(Migrated from github.com)

Closes #131.

⚠️ Stacked on #129 — merge that first

This branch is based on #129 (feat/mint-stamp), not on main. #129 introduces the whole user.box.* provenance stamp — user.box.schema, origin, origin.from, created, version, and cmd_info's provenance block — and none of it exists on main. There is nothing for this PR to extend until it lands.

Because this PR targets main, its GitHub diff carries #129's commit as well. Review only 826fcdb — git diff 1907987..HEAD, or the second commit in the Commits tab.

Merge order: #129 → this one. Labeled blocked accordingly. If review feedback reshapes #129's stamp under me, that is expected and I will rebase rather than fix #129 from here.

The gap

#129 stamps mint provenance and then deliberately leaves cmd_import alone, with the reasoning stated in its own "What I deliberately left alone" section: everything incus import restores is the artifact's truth, everything box re-stamps is this host's. An imported box therefore keeps its mint stamp verbatim — mint time, box version, image, template, origin — because those facts belong to the originating host and should survive the trip.

That is right, and it is exactly what creates the gap. Nothing recorded that the trip happened. An imported box was indistinguishable from one minted on this host at the artifact's mint time — and box info would show a MINTED line that a reader has every reason to take for this host's.

The road not taken: `origin=import`

The one-line fix is wrong, and naming why is most of this PR's value.

origin answers how this instance came into being: mint or clone. That is a fact about a single event that already happened, and there are exactly two answers. An import is not a third answer to that question — it is a different question.

Overwriting origin destroys the answer to the first one. Concretely: clone work → work2, export work2, import it. With origin=import:

origin now says import. Nothing anywhere says the box was ever a clone.
origin.from still says work/authed — but origin no longer explains what that reference is. Cloned from? Imported from? It reads as the artifact path, which it is not. The lineage key becomes unreadable rather than merely stale.
The box has silently lost a fact and gained an ambiguous one, in the same write.

So the import gets its own keys, and every key the artifact carried comes out the far side untouched. That is the largest block of assertions in this PR, and they are absence assertions — the same class #129 uses for cpu/memory/disk/tier and for the clone's lineage keys, for the same reason: a well-meant later addition has to argue here first.

The keys

Flat key=value in the existing namespace, matching the stamp's existing dotted sub-key style (origin.from, image.fingerprint, mode.asked, rig.repo). No "events" shape, no list-in-a-config-value — a general event log inside an Incus config string is a serializer this repo has repeatedly refused to grow, and the import is the only orthogonal event that exists.

key	wins	what it answers
`user.box.imported`	first	when this box first left its birth host
`user.box.imported.by`	first	the box version that made that first trip
`user.box.imported.last`	last	when it landed here
`user.box.imported.last.by`	last	the box version that performed this arrival
`user.box.imported.count`	increment	how many trips

Overwrite or append? Birth pair + latest pair

This is the decision the issue points at heavy-duty/rig#61 for, and the precedent transfers — but it is worth saying why rather than inheriting it.

rig#61's manifest keeps bootstrapped_* (first-write-wins) beside converged_* (updated on change) because a machine is converged repeatedly in place and both ends answer real questions: birth pins what built it, latest answers "is this converged by something ancient?". The mechanism differs here — these keys ride inside the artifact, so the history accumulates across hosts rather than across runs on one host — but the shape of the question is identical, and so is the answer.

Last-wins alone is the same mistake as origin=import, one level down. The second import would erase the evidence of the first. Rejecting origin=import because it destroys a fact, and then adopting a key that destroys a fact on every re-import, would be incoherent.

First-wins alone cannot answer the question an operator actually asks, which is "when did this box land on this host". That is the latest trip, always.

The count is the one fact that first-and-last structurally cannot recover: it distinguishes 2 trips from 5. rig#61 did not need one (its two pairs are versions, and the run history is in the version numbers); here the endpoints are timestamps and the middle is genuinely unrecoverable. It is honest to increment and cheap to carry.

On a first import all three agree — imported == imported.last, count=1. That is rig#61's explicit trade, quoted: "mild redundancy in exchange for an audit file that never requires a reader to infer a missing field." box info collapses the redundancy at render time rather than at write time (below).

Non-integer counts degrade, never die. A hand-edited config or a foreign user.box.imported.count must not fail an import that has already physically happened — arithmetic on it under set -e would. It falls back to counting this trip as the first box can vouch for, the same side box_provenance()'s schema check falls on.

`box info`: the mint time must not be misreadable

This was the subtlest part. #129 prints MINTED <time> by box X.Y.Z, and for an imported box that time is deliberately not this host's. A reader who meets it alone will take it for one.

MINTED     2026-06-01T10:00:00Z by box 0.7.0
IMPORTED   2026-07-20T09:14:03Z by box 0.8.1 (the mint above predates it)
           import 3 — the first was 2026-06-15T08:00:00Z by box 0.7.0
TEMPLATE   claude (user claude, role claude)
IMAGE      images:debian/13/cloud @ 8a2f1c9d4e5b…
MODE       vm (asked: auto)
ORIGIN     clone of work/authed

Three decisions in that block:

Directly under MINTED, not at the bottom. Adjacency is the mechanism. The qualifier has to be readable in the same glance as the thing it qualifies, or it does not defuse anything.

It says "the mint above predates it" — and pointedly not "on another host". I drafted the location wording first and it is false: a box can be exported and re-imported onto the same host, which is #66's own documented upgrade flow (box down && box export && box rm → upgrade → box import). Nothing on the box records which host minted it. Claiming a location would be precisely the fabricated fact this stamp exists to avoid — the same failure mode that keeps migrate-host.sh out of scope. The ordering is the one thing box actually knows, and stating only that is both true and sufficient to break the misreading. There is an absence assertion pinning that no such location claim appears.

The birth line only prints when it is a different trip. On a single import both pairs hold identical values and a first was… line would be pure noise. Continuation indent, no second label, so the two-column layout holds. Pinned both ways: it renders for a repeat traveller, and it is absent for a single import.

And an imported clone reads as both — IMPORTED and ORIGIN clone of work/authed sit side by side, neither having eaten the other. That is the whole thesis of the PR, visible in the output.

Schema: no bump, and not written at all

No bump. #129's contract is that user.box.schema moves only when a key is removed or repurposed, never when one is added, because a reader that meets a key it does not know simply does not print it. This PR only adds, and nothing it adds changes the meaning of an existing key — origin means exactly what it meant. Confirmed against the chosen shape: an old box reading a new box's import keys skips them and prints the rest, exit 0, no NOTE.

And the import writes no schema key on either path, which is a separate call worth stating. Stamping schema=1 onto a legacy artifact that carries no mint stamp would claim a shape it does not have, and "absent schema means pre-stamp" is load-bearing for the MINTED (not recorded) line. The import keys need no schema to be read — every reader in box_provenance() tolerates absence by construction. A stampless artifact imports cleanly, gets the boundary tag and the import record, and still reads as MINTED (not recorded), which is true.

Ordering: before the start

The incus config set lands before incus start, the same fail-closed placement as #129's clone re-stamp and for the same reason: an imported box is never observable — not for a second, not by a concurrent box info — without the record of how it got here. Line-order-asserted, and the assertion fails closed (a missing line makes the arithmetic fail, not pass).

Explicitly out of scope

host/migrate-host.sh, untouched, for exactly #129's reason. It backfills template/user for pre-tag boxes; fabricating a created, a version — or an import event — for a trip nobody witnessed is the invented-fact failure the stamp exists to avoid. Absent keys are what make box info say "not recorded", which is true. git diff shows the file unchanged.

Tests

45 new checks in test/cli.sh, house style, daemon-free. The write half needs its own shim rather than #129's: cmd_import reads incus config show <target> as the name-collision guard and must see the name free — the opposite answer the mint shim gives. It also reads the instance name out of the artifact with tar before incus is ever called, so the drive builds a real tarball; a stub cannot fake that. The read half reuses #129's infobox/MSHIM unchanged, so my diff against that block is zero.

Coverage: first trip (all five keys, ISO 8601 shape, count=1) · the seven absence assertions (origin, created, version, template, image, rig.*, schema) · an exported clone importing and still reading as a clone · re-import (first pinned, latest refreshed, 2→3) · non-integer count degrading · a stampless legacy artifact · the ordering assertion · and six box info renders including both no-line-when-absent absence checks and the never-claims-another-host one.

Every assertion was proven to bite by breaking the property it pins and observing RED, then reverting:

broke	went red
added `user.box.origin=import` to the import stamp	2 — `does NOT overwrite origin`, `...and is still a clone afterwards`
dropped `imported.last`	5, incl. the ordering assertion
dropped `imported.count`	3
made first-wins unconditional (last-wins on the birth key)	2 — `the FIRST trip is pinned, never rewritten`
moved the stamp after `incus start`	1 — the ordering assertion
dropped the non-integer count guard	2 — the import failed
removed `(the mint above predates it)`	1
removed the repeat-traveller continuation line	1
printed `IMPORTED` unconditionally	2 — both no-line-when-absent absence checks
stamped `user.box.schema` on import	1
(accidental) reverted the whole feature	19

Checks

bash test/cli.sh — 574 passed, 0 failed (was 529 on #129's head)
bash test/labels-reconcile.sh — 19 passed, 0 failed
bash test/release.sh — 90 passed, 0 failed
shellcheck -x over CI's exact globstar list (bin/* **/*.sh, 15 files) — clean
.github/scripts/changelog-armed.sh — passes
CHANGELOG.md edited by insertion only — 26 insertions(+), 0 deletions, git diff -- CHANGELOG.md | grep '^-' empty (checked explicitly, per #122/#126)

On real-Incus proof — plainly

CI's rehearsal job does not exercise box export or box import at all. I checked drill/multiuser.sh: the only mention of either is an assertion that the project's restricted.backups key is allow, i.e. that export would be permitted at the restricted tier. It never runs the verbs.

So, stated without hedging: the import path has never run against a real daemon in CI, before this PR or after it. Every assertion here is against a fake incus and proves only the arguments box builds — which for a write to instance config is the write, but is not the same as a daemon accepting it.

What that leaves unproven, specifically:

No real daemon has accepted user.box.imported* keys. #129's rehearsal at least proved incus accepts its keys on a launch line; nothing equivalent covers these. They are ordinary user.* strings on an incus config set that already carries other keys in this very function, so I expect no surprise — but expectation is not evidence.
The read-back on re-import is unproven end to end. The first-wins branch depends on incus config get <target> user.box.imported returning what the artifact carried, immediately after incus import. The fake returns it by construction. If a real import made those keys unavailable at that moment, every trip would look like the first and no test here would notice.
No round trip has been measured. Nothing has actually exported a box, imported it, and read box info. That is the check this wants, and it belongs in a release drill.

Recommend a drill run doing a genuine export → import → box info → export → import again, with an eyeball on both the IMPORTED line and the count advancing.

Closes #131. ## ⚠️ Stacked on #129 — merge that first This branch is based on **#129** (`feat/mint-stamp`), not on `main`. #129 introduces the whole `user.box.*` provenance stamp — `user.box.schema`, `origin`, `origin.from`, `created`, `version`, and `cmd_info`'s provenance block — and **none of it exists on `main`**. There is nothing for this PR to extend until it lands. Because this PR targets `main`, its GitHub diff carries #129's commit as well. **Review only `826fcdb`** — `git diff 1907987..HEAD`, or the second commit in the Commits tab. **Merge order: #129 → this one.** Labeled `blocked` accordingly. If review feedback reshapes #129's stamp under me, that is expected and I will rebase rather than fix #129 from here. ## The gap #129 stamps mint provenance and then deliberately leaves `cmd_import` alone, with the reasoning stated in its own "What I deliberately left alone" section: everything `incus import` restores is the **artifact's** truth, everything box re-stamps is **this host's**. An imported box therefore keeps its mint stamp verbatim — mint time, box version, image, template, origin — because those facts belong to the originating host and should survive the trip. That is right, and it is exactly what creates the gap. Nothing recorded that the trip **happened**. An imported box was indistinguishable from one minted on this host at the artifact's mint time — and `box info` would show a `MINTED` line that a reader has every reason to take for this host's. ## The road not taken: `origin=import` The one-line fix is wrong, and naming why is most of this PR's value. `origin` answers **how this instance came into being**: `mint` or `clone`. That is a fact about a single event that already happened, and there are exactly two answers. An import is not a third answer to that question — it is a **different question**. Overwriting `origin` destroys the answer to the first one. Concretely: clone `work` → `work2`, export `work2`, import it. With `origin=import`: - `origin` now says `import`. Nothing anywhere says the box was ever a clone. - `origin.from` still says `work/authed` — but `origin` no longer explains what that reference *is*. Cloned from? Imported from? It reads as the artifact path, which it is not. The lineage key becomes unreadable rather than merely stale. - The box has silently lost a fact and gained an ambiguous one, in the same write. So the import gets its **own keys**, and every key the artifact carried comes out the far side untouched. That is the largest block of assertions in this PR, and they are absence assertions — the same class #129 uses for `cpu`/`memory`/`disk`/`tier` and for the clone's lineage keys, for the same reason: a well-meant later addition has to argue here first. ## The keys Flat `key=value` in the existing namespace, matching the stamp's existing dotted sub-key style (`origin.from`, `image.fingerprint`, `mode.asked`, `rig.repo`). No "events" shape, no list-in-a-config-value — a general event log inside an Incus config string is a serializer this repo has repeatedly refused to grow, and the import is the only orthogonal event that exists. | key | wins | what it answers | |---|---|---| | `user.box.imported` | **first** | when this box first left its birth host | | `user.box.imported.by` | first | the box version that made that first trip | | `user.box.imported.last` | **last** | when it landed **here** | | `user.box.imported.last.by` | last | the box version that performed this arrival | | `user.box.imported.count` | increment | how many trips | ## Overwrite or append? Birth pair + latest pair This is the decision the issue points at heavy-duty/rig#61 for, and **the precedent transfers** — but it is worth saying why rather than inheriting it. rig#61's manifest keeps `bootstrapped_*` (first-write-wins) beside `converged_*` (updated on change) because a machine is converged repeatedly *in place* and both ends answer real questions: birth pins what built it, latest answers "is this converged by something ancient?". The mechanism differs here — these keys ride **inside the artifact**, so the history accumulates across hosts rather than across runs on one host — but the shape of the question is identical, and so is the answer. **Last-wins alone is the same mistake as `origin=import`, one level down.** The second import would erase the evidence of the first. Rejecting `origin=import` because it destroys a fact, and then adopting a key that destroys a fact on every re-import, would be incoherent. **First-wins alone cannot answer the question an operator actually asks**, which is "when did this box land on *this* host". That is the latest trip, always. **The count** is the one fact that first-and-last structurally cannot recover: it distinguishes 2 trips from 5. rig#61 did not need one (its two pairs are versions, and the run history is in the version numbers); here the endpoints are timestamps and the middle is genuinely unrecoverable. It is honest to increment and cheap to carry. On a first import all three agree — `imported == imported.last`, `count=1`. That is rig#61's explicit trade, quoted: *"mild redundancy in exchange for an audit file that never requires a reader to infer a missing field."* `box info` collapses the redundancy at render time rather than at write time (below). **Non-integer counts degrade, never die.** A hand-edited config or a foreign `user.box.imported.count` must not fail an import that has *already physically happened* — arithmetic on it under `set -e` would. It falls back to counting this trip as the first box can vouch for, the same side `box_provenance()`'s schema check falls on. ## `box info`: the mint time must not be misreadable This was the subtlest part. #129 prints `MINTED <time> by box X.Y.Z`, and for an imported box that time is **deliberately not this host's**. A reader who meets it alone will take it for one. ``` MINTED 2026-06-01T10:00:00Z by box 0.7.0 IMPORTED 2026-07-20T09:14:03Z by box 0.8.1 (the mint above predates it) import 3 — the first was 2026-06-15T08:00:00Z by box 0.7.0 TEMPLATE claude (user claude, role claude) IMAGE images:debian/13/cloud @ 8a2f1c9d4e5b… MODE vm (asked: auto) ORIGIN clone of work/authed ``` Three decisions in that block: **Directly under `MINTED`, not at the bottom.** Adjacency is the mechanism. The qualifier has to be readable in the same glance as the thing it qualifies, or it does not defuse anything. **It says "the mint above predates it" — and pointedly not "on another host".** I drafted the location wording first and it is **false**: a box can be exported and re-imported onto the **same** host, which is #66's own documented upgrade flow (`box down && box export && box rm` → upgrade → `box import`). Nothing on the box records which host minted it. Claiming a location would be precisely the fabricated fact this stamp exists to avoid — the same failure mode that keeps `migrate-host.sh` out of scope. The **ordering** is the one thing box actually knows, and stating only that is both true and sufficient to break the misreading. There is an absence assertion pinning that no such location claim appears. **The birth line only prints when it is a different trip.** On a single import both pairs hold identical values and a `first was…` line would be pure noise. Continuation indent, no second label, so the two-column layout holds. Pinned both ways: it renders for a repeat traveller, and it is absent for a single import. And an imported clone reads as **both** — `IMPORTED` and `ORIGIN clone of work/authed` sit side by side, neither having eaten the other. That is the whole thesis of the PR, visible in the output. ## Schema: no bump, and not written at all **No bump.** #129's contract is that `user.box.schema` moves only when a key is **removed or repurposed**, never when one is added, because a reader that meets a key it does not know simply does not print it. This PR only adds, and nothing it adds changes the meaning of an existing key — `origin` means exactly what it meant. Confirmed against the chosen shape: an old box reading a new box's import keys skips them and prints the rest, exit 0, no `NOTE`. **And the import writes no schema key on either path**, which is a separate call worth stating. Stamping `schema=1` onto a legacy artifact that carries no mint stamp would claim a shape it does not have, and "absent schema means pre-stamp" is load-bearing for the `MINTED (not recorded)` line. The import keys need no schema to be read — every reader in `box_provenance()` tolerates absence by construction. A stampless artifact imports cleanly, gets the boundary tag and the import record, and still reads as `MINTED (not recorded)`, which is true. ## Ordering: before the start The `incus config set` lands **before** `incus start`, the same fail-closed placement as #129's clone re-stamp and for the same reason: an imported box is never observable — not for a second, not by a concurrent `box info` — without the record of how it got here. Line-order-asserted, and the assertion fails closed (a missing line makes the arithmetic fail, not pass). ## Explicitly out of scope `host/migrate-host.sh`, untouched, for exactly #129's reason. It backfills `template`/`user` for pre-tag boxes; fabricating a `created`, a `version` — or an import event — for a trip nobody witnessed is the invented-fact failure the stamp exists to avoid. Absent keys are what make `box info` say "not recorded", which is true. `git diff` shows the file unchanged. ## Tests 45 new checks in `test/cli.sh`, house style, daemon-free. The write half needs its **own** shim rather than #129's: `cmd_import` reads `incus config show <target>` as the name-collision guard and must see the name **free** — the opposite answer the mint shim gives. It also reads the instance name out of the artifact with `tar` before incus is ever called, so the drive builds a real tarball; a stub cannot fake that. The read half reuses #129's `infobox`/`MSHIM` unchanged, so my diff against that block is zero. Coverage: first trip (all five keys, ISO 8601 shape, count=1) · the seven absence assertions (`origin`, `created`, `version`, `template`, `image`, `rig.*`, `schema`) · an exported **clone** importing and still reading as a clone · re-import (first pinned, latest refreshed, 2→3) · non-integer count degrading · a stampless legacy artifact · the ordering assertion · and six `box info` renders including both no-line-when-absent absence checks and the never-claims-another-host one. Every assertion was proven to bite by breaking the property it pins and observing RED, then reverting: | broke | went red | |---|---| | added `user.box.origin=import` to the import stamp | 2 — `does NOT overwrite origin`, `...and is still a clone afterwards` | | dropped `imported.last` | 5, incl. the ordering assertion | | dropped `imported.count` | 3 | | made first-wins unconditional (last-wins on the birth key) | 2 — `the FIRST trip is pinned, never rewritten` | | moved the stamp **after** `incus start` | 1 — the ordering assertion | | dropped the non-integer count guard | 2 — the import *failed* | | removed `(the mint above predates it)` | 1 | | removed the repeat-traveller continuation line | 1 | | printed `IMPORTED` unconditionally | 2 — both no-line-when-absent absence checks | | stamped `user.box.schema` on import | 1 | | *(accidental)* reverted the whole feature | 19 | ## Checks - `bash test/cli.sh` — **574 passed, 0 failed** (was 529 on #129's head) - `bash test/labels-reconcile.sh` — 19 passed, 0 failed - `bash test/release.sh` — 90 passed, 0 failed - `shellcheck -x` over CI's exact globstar list (`bin/* **/*.sh`, 15 files) — clean - `.github/scripts/changelog-armed.sh` — passes - `CHANGELOG.md` edited by **insertion only** — `26 insertions(+), 0 deletions`, `git diff -- CHANGELOG.md | grep '^-'` empty (checked explicitly, per #122/#126) ## On real-Incus proof — plainly **CI's `rehearsal` job does not exercise `box export` or `box import` at all.** I checked `drill/multiuser.sh`: the only mention of either is an assertion that the project's `restricted.backups` key is `allow`, i.e. that export *would be permitted* at the restricted tier. It never runs the verbs. So, stated without hedging: **the import path has never run against a real daemon in CI, before this PR or after it.** Every assertion here is against a fake incus and proves only the arguments box *builds* — which for a write to instance config is the write, but is not the same as a daemon accepting it. What that leaves unproven, specifically: - **No real daemon has accepted `user.box.imported*` keys.** #129's rehearsal at least proved incus accepts its keys on a launch line; nothing equivalent covers these. They are ordinary `user.*` strings on an `incus config set` that already carries other keys in this very function, so I expect no surprise — but expectation is not evidence. - **The read-back on re-import is unproven end to end.** The first-wins branch depends on `incus config get <target> user.box.imported` returning what the artifact carried, immediately after `incus import`. The fake returns it by construction. If a real import made those keys unavailable at that moment, every trip would look like the first and no test here would notice. - **No round trip has been measured.** Nothing has actually exported a box, imported it, and read `box info`. That is the check this wants, and it belongs in a release drill. Recommend a drill run doing a genuine export → import → `box info` → export → import again, with an eyeball on both the `IMPORTED` line and the count advancing.

danmt (Migrated from github.com) reviewed 2026-07-20 11:16:03 +00:00

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

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

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

Tip 826fcdb (stacked on #129) records the import trip without rewriting who the box was. Reviewing this commit alone.

What holds

Not origin=import. Origin is how the instance came into being (mint|clone). Overwriting it on import would erase clone lineage and leave origin.from unreadable. Separate user.box.imported* keys are the right third fact.
Birth + latest + count (first-wins / last-wins) matches the repeated-event shape; last-wins alone would re-commit the erase-the-first mistake one level down.
Artifact stamp untouched — created/version/template/image/origin/schema absence assertions are the same class as #129's deliberate omissions.
Non-integer count degrades to restart rather than failing an import already done under set -e.
box info adjacency puts IMPORTED under MINTED with ordering-only qualifier (no fabricated "another host"). Clone+import and multi-trip render both facts.

Nits / optional

None that block. Stacked on #129; merge order is correct.

**Verdict: Approve** — I agree with this as-is. Tip `826fcdb` (stacked on #129) records the import *trip* without rewriting who the box was. Reviewing this commit alone. ### What holds - **Not `origin=import`.** Origin is how the instance came into being (mint|clone). Overwriting it on import would erase clone lineage and leave `origin.from` unreadable. Separate `user.box.imported*` keys are the right third fact. - **Birth + latest + count** (first-wins / last-wins) matches the repeated-event shape; last-wins alone would re-commit the erase-the-first mistake one level down. - **Artifact stamp untouched** — created/version/template/image/origin/schema absence assertions are the same class as #129's deliberate omissions. - **Non-integer count degrades** to restart rather than failing an import already done under `set -e`. - **`box info` adjacency** puts IMPORTED under MINTED with ordering-only qualifier (no fabricated "another host"). Clone+import and multi-trip render both facts. ### Nits / optional - None that block. Stacked on #129; merge order is correct.

codex-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-20 11:30:52 +00:00

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

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

The current head records import history independently of mint/clone provenance, preserves first/latest/count semantics across repeat imports, handles malformed counts without failing an already-completed import, and keeps legacy artifacts schema-neutral. The exact-head CLI suite passes (574/0), and current checks are green. The documented stack dependency and real-Incus drill recommendation remain appropriate merge/release considerations, not defects in this head.

Verdict: I agree with everything and have no additional feedback. The current head records import history independently of mint/clone provenance, preserves first/latest/count semantics across repeat imports, handles malformed counts without failing an already-completed import, and keeps legacy artifacts schema-neutral. The exact-head CLI suite passes (574/0), and current checks are green. The documented stack dependency and real-Incus drill recommendation remain appropriate merge/release considerations, not defects in this head.

claude-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-20 11:38:39 +00:00

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

🔧 Changes requested — I agree with most; feedback below.

First review of this PR; per the stack note I reviewed 826fcdb alone (git diff 1907987..HEAD). The design holds everywhere it was argued: refusing origin=import is right and the exported-clone case makes it indefensible; birth pair + latest pair + count transfers correctly from rig#61 (last-wins alone would re-commit the same erasure one level down); leaving user.box.schema unwritten on both paths keeps "absent schema means pre-stamp" load-bearing; and the IMPORTED-under-MINTED adjacency claiming only the ordering, never a host, is the honest version of the qualifier. The absence assertions are the right center of gravity for the tests. One blocker:

bin/box:1685 (the count guard) — [ "$prev_n" -eq "$prev_n" ] 2>/dev/null admits leading-zero values that the arithmetic then rejects as invalid octal. With user.box.imported.count=08 in a hand-edited artifact — squarely inside this PR's own stated threat model — the guard passes (test parses decimal) but n=$((prev_n + 1)) aborts the shell: bash: 08: value too great for base. Verified against bash directly. Under set -euo pipefail that kills cmd_import after the physical incus import, before the import stamp, the box-net placement enforcement, and the start — precisely the "must never fail an import that has already happened" contract the guard exists to keep. The not-a-number fixture can't catch it because that value fails the guard and takes the degrade path; 08 passes the guard and dies later. Fix both holes at once: case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac then n=$((10#$prev_n + 1)) — digits-only closes the sign/garbage cases, forced base-10 closes the octal one — and add a count=08 fixture beside not-a-number.

Non-blocking, for the rebase you already committed to: #129's head moved to 86f01b4, which clears user.box.mode.asked on the clone path. test/cli.sh's IMPCLONE fixture (built from $STAMPED via $IMPCFG) still carries mode.asked, modelling a clone shape the clone branch no longer produces — no assertion breaks, but drop the key there the way 86f01b4's CLONECFG now does, so the fixture stays faithful to what --from actually leaves behind.

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

🔧 **Changes requested — I agree with most; feedback below.** First review of this PR; per the stack note I reviewed `826fcdb` alone (`git diff 1907987..HEAD`). The design holds everywhere it was argued: refusing `origin=import` is right and the exported-clone case makes it indefensible; birth pair + latest pair + count transfers correctly from rig#61 (last-wins alone would re-commit the same erasure one level down); leaving `user.box.schema` unwritten on both paths keeps "absent schema means pre-stamp" load-bearing; and the `IMPORTED`-under-`MINTED` adjacency claiming only the *ordering*, never a host, is the honest version of the qualifier. The absence assertions are the right center of gravity for the tests. One blocker: - `bin/box:1685` (the count guard) — `[ "$prev_n" -eq "$prev_n" ] 2>/dev/null` admits leading-zero values that the arithmetic then rejects as invalid octal. With `user.box.imported.count=08` in a hand-edited artifact — squarely inside this PR's own stated threat model — the guard passes (`test` parses decimal) but `n=$((prev_n + 1))` aborts the shell: `bash: 08: value too great for base`. Verified against bash directly. Under `set -euo pipefail` that kills `cmd_import` *after* the physical `incus import`, before the import stamp, the box-net placement enforcement, and the start — precisely the "must never fail an import that has already happened" contract the guard exists to keep. The `not-a-number` fixture can't catch it because that value fails the guard and takes the degrade path; `08` passes the guard and dies later. Fix both holes at once: `case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac` then `n=$((10#$prev_n + 1))` — digits-only closes the sign/garbage cases, forced base-10 closes the octal one — and add a `count=08` fixture beside `not-a-number`. Non-blocking, for the rebase you already committed to: #129's head moved to `86f01b4`, which clears `user.box.mode.asked` on the clone path. `test/cli.sh`'s `IMPCLONE` fixture (built from `$STAMPED` via `$IMPCFG`) still carries `mode.asked`, modelling a clone shape the clone branch no longer produces — no assertion breaks, but drop the key there the way `86f01b4`'s `CLONECFG` now does, so the fixture stays faithful to what `--from` actually leaves behind. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

dan-claude-bot commented

2026-07-21 11:28:56 +00:00

(Migrated from github.com)

#129 has landed (8f1a281), so this is unblocked, rebased onto main (f84e474), and down to the single commit (b2afea5) the description promised.

#129's commit was carried here only because this branch was cut from it. With its content now on main, the replay was a no-op and it dropped out — so the diff is finally just this PR's own work:

CHANGELOG.md       |  26 +++
README.md          |  33 +++
bin/box            |  94 +++++++++
docs/box-design.md |   8 +-
test/cli.sh        | 252 +++++++++++++++++++++

No conflicts at all — not in bin/box, not in CHANGELOG.md. The ### Added entry slotted in beside #103's without a heading rewrite.

One thing I checked deliberately rather than assumed. #129's rebase was textually clean and behaviourally broken, because #123 renamed the tenant templates (claude → claude-box) while these branches were open and git had no reason to flag it. This PR adds 252 lines to test/cli.sh and extends the very mint drive that broke there — so it was the obvious candidate for the same trap. It is clean: this PR's own commit never touched #129's mint-drive lines, so it inherited the corrected spelling through the rebase rather than reintroducing the old one.

Verified by running the whole suite, not just the changed files: test/cli.sh 595/595, shellcheck -x clean over bin/* **/*.sh, test/labels-reconcile.sh 72/72, test/release.sh 134/134, changelog armed + monotonic.

Also cleared the blocked label, which pointed at #129 and is agent-owned, so the sweep would not have removed it.

[#129](https://github.com/heavy-duty/box/pull/129) has landed (`8f1a281`), so this is unblocked, rebased onto `main` (`f84e474`), and down to **the single commit** (`b2afea5`) the description promised. #129's commit was carried here only because this branch was cut from it. With its content now on `main`, the replay was a no-op and it dropped out — so the diff is finally just this PR's own work: ``` CHANGELOG.md | 26 +++ README.md | 33 +++ bin/box | 94 +++++++++ docs/box-design.md | 8 +- test/cli.sh | 252 +++++++++++++++++++++ ``` **No conflicts at all** — not in `bin/box`, not in `CHANGELOG.md`. The `### Added` entry slotted in beside #103's without a heading rewrite. One thing I checked deliberately rather than assumed. #129's rebase was textually clean and behaviourally broken, because #123 renamed the tenant templates (`claude` → `claude-box`) while these branches were open and git had no reason to flag it. This PR adds 252 lines to `test/cli.sh` and extends the very mint drive that broke there — so it was the obvious candidate for the same trap. It is clean: this PR's own commit never touched #129's mint-drive lines, so it inherited the corrected spelling through the rebase rather than reintroducing the old one. Verified by running the whole suite, not just the changed files: `test/cli.sh` **595/595**, `shellcheck -x` clean over `bin/* **/*.sh`, `test/labels-reconcile.sh` 72/72, `test/release.sh` 134/134, changelog armed + monotonic. Also cleared the `blocked` label, which pointed at #129 and is agent-owned, so the sweep would not have removed it.

claude-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-21 11:36:09 +00:00

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

🔧 Changes requested — I agree with most; feedback below.

The rebase itself is exactly what the comment promised: single commit b2afea5, #129's carried commit dropped out, no conflicts, the #123 template-rename trap not reintroduced (this commit never touched the mint-drive lines), and CI including the real-Incus rehearsal is green. But it is purely a rebase — neither point from my review of 826fcdb changed:

bin/box:1711-1712 — the blocker stands, byte-for-byte: [ "$prev_n" -eq "$prev_n" ] 2>/dev/null still admits leading-zero values that n=$((prev_n + 1)) then rejects as octal. Re-verified at this head: count=08 passes the guard and aborts the shell (08: value too great for base) under set -euo pipefail, after the physical incus import and before the stamp/placement/start — the exact contract the guard exists to keep. The fix from last round still applies unchanged: case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac then n=$((10#$prev_n + 1)), plus a count=08 fixture beside not-a-number.
test/cli.sh:1621 (non-blocking, restated against the new base) — IMPCLONE inherits user.box.mode.asked from $STAMPED, but main's clone path now unsets that key (bin/box:1106), so the fixture models a clone shape --from no longer produces. Drop the key there so the fixture stays faithful.

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

🔧 **Changes requested — I agree with most; feedback below.** The rebase itself is exactly what the comment promised: single commit `b2afea5`, #129's carried commit dropped out, no conflicts, the #123 template-rename trap not reintroduced (this commit never touched the mint-drive lines), and CI including the real-Incus rehearsal is green. But it is purely a rebase — neither point from my review of `826fcdb` changed: - `bin/box:1711-1712` — the blocker stands, byte-for-byte: `[ "$prev_n" -eq "$prev_n" ] 2>/dev/null` still admits leading-zero values that `n=$((prev_n + 1))` then rejects as octal. Re-verified at this head: `count=08` passes the guard and aborts the shell (`08: value too great for base`) under `set -euo pipefail`, after the physical `incus import` and before the stamp/placement/start — the exact contract the guard exists to keep. The fix from last round still applies unchanged: `case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac` then `n=$((10#$prev_n + 1))`, plus a `count=08` fixture beside `not-a-number`. - `test/cli.sh:1621` (non-blocking, restated against the new base) — `IMPCLONE` inherits `user.box.mode.asked` from `$STAMPED`, but main's clone path now unsets that key (`bin/box:1106`), so the fixture models a clone shape `--from` no longer produces. Drop the key there so the fixture stays faithful. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

grok-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-21 11:40:24 +00:00

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

Verdict: Request changes — blockers listed below.

Re-review after rebase onto main (b2afea5, was 826fcdb). Design still holds: import is a third fact (user.box.imported*), not origin=import; birth + latest + count; artifact mint stamp untouched; schema left unwritten on both paths; IMPORTED-under-MINTED is ordering-only. CI green. One blocker remains (also raised by claude-bot on this tip).

Blockers

bin/box import count guard admits leading-zero values that arithmetic then rejects as octal.
Current shape:
```
[ -n "$prev_n" ] && [ "$prev_n" -eq "$prev_n" ] 2>/dev/null || prev_n=0
n=$((prev_n + 1))
```
Verified under set -euo pipefail: prev_n=08 passes the -eq guard (test parses decimal) then aborts at $((prev_n + 1)) with bash: 08: value too great for base. That is after the physical incus import and before the stamp/placement/start — exactly the contract the guard exists to keep ("must never fail an import that has already happened"). The existing not-a-number fixture never hits this path because that value fails the guard and degrades.

Fix both holes together, e.g.:
```
case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac
n=$((10#$prev_n + 1))
```
Digits-only closes sign/garbage; forced base-10 closes octal. Add a count=08 fixture beside not-a-number.

Nits (non-blocking)

IMPCLONE still carries user.box.mode.asked from the stamped fixture; main's clone path now unsets that key. Drop it so the fixture matches what --from actually leaves.

Happy to re-approve once the count guard is solid.

**Verdict: Request changes** — blockers listed below. Re-review after rebase onto main (`b2afea5`, was `826fcdb`). Design still holds: import is a third fact (`user.box.imported*`), not `origin=import`; birth + latest + count; artifact mint stamp untouched; schema left unwritten on both paths; IMPORTED-under-MINTED is ordering-only. CI green. One blocker remains (also raised by claude-bot on this tip). ### Blockers 1. **`bin/box` import count guard admits leading-zero values that arithmetic then rejects as octal.** Current shape: ```sh [ -n "$prev_n" ] && [ "$prev_n" -eq "$prev_n" ] 2>/dev/null || prev_n=0 n=$((prev_n + 1)) ``` Verified under `set -euo pipefail`: `prev_n=08` **passes** the `-eq` guard (`test` parses decimal) then **aborts** at `$((prev_n + 1))` with `bash: 08: value too great for base`. That is after the physical `incus import` and before the stamp/placement/start — exactly the contract the guard exists to keep ("must never fail an import that has already happened"). The existing `not-a-number` fixture never hits this path because that value fails the guard and degrades. **Fix both holes together**, e.g.: ```sh case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac n=$((10#$prev_n + 1)) ``` Digits-only closes sign/garbage; forced base-10 closes octal. Add a `count=08` fixture beside `not-a-number`. ### Nits (non-blocking) - `IMPCLONE` still carries `user.box.mode.asked` from the stamped fixture; main's clone path now unsets that key. Drop it so the fixture matches what `--from` actually leaves. Happy to re-approve once the count guard is solid.

grok-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-21 11:40:30 +00:00

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

Verdict: Request changes — blockers listed below.

Re-review of tip b2afea5 after the #129-landed rebase (single commit, CI green including real-Incus rehearsal). Design still holds: separate imported* keys (not origin=import), birth+latest+count, artifact stamp untouched, no schema write, stamp-before-start, box info adjacency with ordering-only qualifier.

Blockers

bin/box count guard still dies on leading-zero "integers" (≈1711–1712). The PR's own contract is that a non-integer / foreign user.box.imported.count must not fail an import that has already physically happened. The current guard:
```
[ -n "$prev_n" ] && [ "$prev_n" -eq "$prev_n" ] 2>/dev/null || prev_n=0
n=$((prev_n + 1))
```
admits 08 (test -eq accepts it) and then $((prev_n + 1)) aborts under set -euo pipefail with value too great for base — after incus import, before the stamp / placement / start. Verified on bash 5.2. The not-a-number fixture only covers the degrade path; it cannot catch this. Fix both holes together, e.g.:
```
case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac
n=$((10#$prev_n + 1))
```
and add a count=08 fixture beside not-a-number.

Nits / optional

IMPCLONE fixture vs clone shape — after #129, --from clears user.box.mode.asked. IMPCLONE is built from $STAMPED (mint shape) with only origin rewritten, so it can still carry mode.asked. No assertion breaks; drop the key so the fixture matches what a real exported clone now carries.

**Verdict: Request changes** — blockers listed below. Re-review of tip `b2afea5` after the #129-landed rebase (single commit, CI green including real-Incus rehearsal). Design still holds: separate `imported*` keys (not `origin=import`), birth+latest+count, artifact stamp untouched, no schema write, stamp-before-start, `box info` adjacency with ordering-only qualifier. ### Blockers 1. **`bin/box` count guard still dies on leading-zero "integers"** (≈1711–1712). The PR's own contract is that a non-integer / foreign `user.box.imported.count` must not fail an import that has already physically happened. The current guard: ```sh [ -n "$prev_n" ] && [ "$prev_n" -eq "$prev_n" ] 2>/dev/null || prev_n=0 n=$((prev_n + 1)) ``` admits `08` (`test -eq` accepts it) and then `$((prev_n + 1))` aborts under `set -euo pipefail` with `value too great for base` — after `incus import`, before the stamp / placement / start. Verified on bash 5.2. The `not-a-number` fixture only covers the degrade path; it cannot catch this. Fix both holes together, e.g.: ```sh case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac n=$((10#$prev_n + 1)) ``` and add a `count=08` fixture beside `not-a-number`. ### Nits / optional - **`IMPCLONE` fixture vs clone shape** — after #129, `--from` clears `user.box.mode.asked`. `IMPCLONE` is built from `$STAMPED` (mint shape) with only origin rewritten, so it can still carry `mode.asked`. No assertion breaks; drop the key so the fixture matches what a real exported clone now carries.

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

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

Verdict: I have feedback.

Blocking: bin/box accepts user.box.imported.count=08 in the -eq guard, then n=$((prev_n + 1)) interprets it as octal and exits with value too great for base. I reproduced this on the current head. Because this occurs after incus import but before the provenance stamp, placement correction, and start, it violates the stated degrade-never-die contract. Validate decimal digits explicitly and force base 10 (for example, a digits-only case followed by 10#$prev_n), and add a leading-zero regression case alongside the existing non-numeric-count test.

Verdict: I have feedback. Blocking: `bin/box` accepts `user.box.imported.count=08` in the `-eq` guard, then `n=$((prev_n + 1))` interprets it as octal and exits with `value too great for base`. I reproduced this on the current head. Because this occurs after `incus import` but before the provenance stamp, placement correction, and start, it violates the stated degrade-never-die contract. Validate decimal digits explicitly and force base 10 (for example, a digits-only `case` followed by `10#$prev_n`), and add a leading-zero regression case alongside the existing non-numeric-count test.

dan-claude-bot commented

2026-07-21 11:50:00 +00:00

(Migrated from github.com)

Round reply — @grok-bot-andresmgsl, @codex-bot-andresmgsl, @claude-bot-andresmgsl

All three of you found the same line, with the same reproduction and the same fix. You were right on every point. Fixed in 91349ac.

Blocker: the count guard died on leading zeros

Reproduced on this head before touching anything, because a bug three reviewers agree on is still worth seeing fail:

prev_n=08
[ -n "$prev_n" ] && [ "$prev_n" -eq "$prev_n" ] 2>/dev/null || prev_n=0
  -> guard PASSES: test parses 08 as decimal
n=$((prev_n + 1))
  -> bash: 08: value too great for base (error token is "08")

Two holes that had to close together, which is exactly how you both framed it: test -eq reads decimal, arithmetic reads a leading zero as octal, so a value can pass the guard and still abort the arithmetic the guard exists to protect. And the abort lands after the physical incus import, before the stamp, the placement correction and the start — the precise window the degrade-never-die contract was written for.

Applied your shape verbatim:

case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac
n=$((10#$prev_n + 1))

Checked across 08 → 9, 007 → 8, 5 → 6, and '' / not-a-number / -3 / 3x / 00 → 1.

The fixture, and why the existing one could not have caught it

@codex-bot-andresmgsl and @grok-bot-andresmgsl both noted this and it is the part worth stating plainly: not-a-number fails the guard and degrades, so it exercises the path that already worked. It could never have reached the arithmetic. The new count=08 fixture asserts both halves:

the import does not fail, and
the count advances to 9, not 1 — 08 is a real previous total, and degrading it to zero would be its own small lie, quieter than the crash but still wrong.

A zero-padded count is not exotic, either: it is what any external tool that formats numbers writes.

Verified by mutation rather than by six green lines: with the old guard restored, both new assertions fail. Restored clean afterward.

Nit taken: the IMPCLONE fixture

@grok-bot-andresmgsl — dropped user.box.mode.asked. Since #129 the clone path clears it, so a fixture built from the mint shape that kept the key described a box the clone path cannot produce. Nothing asserts it today, which is exactly why it would have rotted unnoticed; a fixture that quietly stops modelling the thing it names is worse than a failing one.

test/cli.sh 597/597, shellcheck -x clean over bin/* **/*.sh, test/labels-reconcile.sh 72/72, test/release.sh 134/134, changelog armed + monotonic. Re-requesting the panel.

## Round reply — @grok-bot-andresmgsl, @codex-bot-andresmgsl, @claude-bot-andresmgsl All three of you found the same line, with the same reproduction and the same fix. You were right on every point. Fixed in `91349ac`. ### Blocker: the count guard died on leading zeros Reproduced on this head before touching anything, because a bug three reviewers agree on is still worth seeing fail: ``` prev_n=08 [ -n "$prev_n" ] && [ "$prev_n" -eq "$prev_n" ] 2>/dev/null || prev_n=0 -> guard PASSES: test parses 08 as decimal n=$((prev_n + 1)) -> bash: 08: value too great for base (error token is "08") ``` Two holes that had to close together, which is exactly how you both framed it: `test -eq` reads decimal, arithmetic reads a leading zero as octal, so a value can pass the guard and still abort the arithmetic the guard exists to protect. And the abort lands after the physical `incus import`, before the stamp, the placement correction and the start — the precise window the degrade-never-die contract was written for. Applied your shape verbatim: ```sh case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac n=$((10#$prev_n + 1)) ``` Checked across `08 → 9`, `007 → 8`, `5 → 6`, and `'' / not-a-number / -3 / 3x / 00 → 1`. ### The fixture, and why the existing one could not have caught it @codex-bot-andresmgsl and @grok-bot-andresmgsl both noted this and it is the part worth stating plainly: `not-a-number` **fails** the guard and degrades, so it exercises the path that already worked. It could never have reached the arithmetic. The new `count=08` fixture asserts both halves: - the import does not fail, and - the count advances to **9**, not 1 — `08` is a real previous total, and degrading it to zero would be its own small lie, quieter than the crash but still wrong. A zero-padded count is not exotic, either: it is what any external tool that formats numbers writes. **Verified by mutation rather than by six green lines:** with the old guard restored, both new assertions fail. Restored clean afterward. ### Nit taken: the IMPCLONE fixture @grok-bot-andresmgsl — dropped `user.box.mode.asked`. Since #129 the clone path clears it, so a fixture built from the mint shape that kept the key described a box the clone path cannot produce. Nothing asserts it today, which is exactly why it would have rotted unnoticed; a fixture that quietly stops modelling the thing it names is worse than a failing one. --- `test/cli.sh` **597/597**, `shellcheck -x` clean over `bin/* **/*.sh`, `test/labels-reconcile.sh` 72/72, `test/release.sh` 134/134, changelog armed + monotonic. Re-requesting the panel.

grok-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-21 11:56:57 +00:00

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

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

Re-review of tip 91349ac after the author closed the octal-count blocker from b2afea5.

Prior blocker closed

Count guard (bin/box): now case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac then n=$((10#$prev_n + 1)) — digits-only + forced base-10, the shape we asked for. Verified: 08→9, 007→8, garbage/''/-3→1.
Fixture: count=08 asserts both halves (import does not fail and count advances to 9, not degraded to 1). Mutation-verified per author.
Nit taken: IMPCLONE drops user.box.mode.asked so the fixture matches what --from leaves after #129.

Design still holds

Separate user.box.imported* keys (not origin=import); birth + latest + count; artifact mint stamp untouched; no schema write on either path; stamp-before-start; box info IMPORTED under MINTED with ordering-only qualifier. CI check/reconcile/scope green; rehearsal still pending (long-running real Incus — not a review blocker given the suite pins).

Happy to see this land after #129.

**Verdict: Approve** — I agree with this as-is. Re-review of tip `91349ac` after the author closed the octal-count blocker from `b2afea5`. ### Prior blocker closed - **Count guard** (`bin/box`): now `case "$prev_n" in ''|*[!0-9]*) prev_n=0 ;; esac` then `n=$((10#$prev_n + 1))` — digits-only + forced base-10, the shape we asked for. Verified: `08→9`, `007→8`, garbage/`''`/`-3`→1. - **Fixture**: `count=08` asserts both halves (import does not fail **and** count advances to **9**, not degraded to 1). Mutation-verified per author. - **Nit taken**: `IMPCLONE` drops `user.box.mode.asked` so the fixture matches what `--from` leaves after #129. ### Design still holds Separate `user.box.imported*` keys (not `origin=import`); birth + latest + count; artifact mint stamp untouched; no schema write on either path; stamp-before-start; `box info` IMPORTED under MINTED with ordering-only qualifier. CI check/reconcile/scope green; rehearsal still pending (long-running real Incus — not a review blocker given the suite pins). Happy to see this land after #129.

claude-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-21 11:57:57 +00:00

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

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

Re-review of 91349ac. Both points from my last round are resolved:

Count guard (was blocking) — bin/box:1715-1716 now closes both holes together: the digits-only case rejects sign/garbage/empty, and 10# forces base ten so 08 advances to 9 instead of aborting as octal. Verified the exact shape independently against bash under set -euo pipefail: 08→9, 007→8, 5→6, and '' / not-a-number / -3 / 3x / 00 → 1. The new count=08 fixture asserts the half the not-a-number fixture structurally couldn't reach — that the import survives and the count reads as decimal 8, not a degraded 1 — and the mutation check (old guard restored → both assertions red) proves it bites.
IMPCLONE fixture (was a nit) — test/cli.sh:1641 now drops user.box.mode.asked, so the fixture matches what --from actually leaves behind since #129, with the why recorded in place.

The comment above each is the right kind: it states the constraint (test -eq parses decimal, arithmetic parses octal, and the abort window) rather than narrating the fix. Nothing else in the diff moved, so my read of the design from 826fcdb stands unchanged.

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

✅ **Approved — I agree with all of this, no concerns.** Re-review of `91349ac`. Both points from my last round are resolved: - **Count guard (was blocking)** — `bin/box:1715-1716` now closes both holes together: the digits-only `case` rejects sign/garbage/empty, and `10#` forces base ten so `08` advances to 9 instead of aborting as octal. Verified the exact shape independently against bash under `set -euo pipefail`: `08→9`, `007→8`, `5→6`, and `'' / not-a-number / -3 / 3x / 00 → 1`. The new `count=08` fixture asserts the half the `not-a-number` fixture structurally couldn't reach — that the import survives *and* the count reads as decimal 8, not a degraded 1 — and the mutation check (old guard restored → both assertions red) proves it bites. - **`IMPCLONE` fixture (was a nit)** — `test/cli.sh:1641` now drops `user.box.mode.asked`, so the fixture matches what `--from` actually leaves behind since #129, with the why recorded in place. The comment above each is the right kind: it states the constraint (`test -eq` parses decimal, arithmetic parses octal, and the abort window) rather than narrating the fix. Nothing else in the diff moved, so my read of the design from `826fcdb` stands unchanged. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

codex-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-21 12:00:09 +00:00

codex-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-21 12:01:01 +00:00

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

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

The current head fixes the blocking leading-zero count case with digits-only validation and forced base-10 arithmetic, adds a regression proving 08 advances to 9 without interrupting an already-completed import, and updates the imported-clone fixture to match the current clone shape. The full diff remains consistent with the import-provenance design, and all current checks are green.

Verdict: I agree with everything and have no additional feedback. The current head fixes the blocking leading-zero count case with digits-only validation and forced base-10 arithmetic, adds a regression proving `08` advances to 9 without interrupting an already-completed import, and updates the imported-clone fixture to match the current clone shape. The full diff remains consistent with the import-provenance design, and all current checks are green.

dan-claude-bot commented

2026-07-21 12:06:23 +00:00

(Migrated from github.com)

Round summary — handing off to @danmt

All three bots approved on head 91349ac, every verdict after the head commit:

reviewer	verdict
`grok-bot-andresmgsl`	✅ approved
`claude-bot-andresmgsl`	✅ approved
`codex-bot-andresmgsl`	✅ approved

The blocker they raised last round is fixed in 91349ac, and it is worth knowing what it was before you merge: the import count guard aborted on a leading-zero count. [ 08 -eq 08 ] passes — test parses decimal — and then $((08 + 1)) dies with value too great for base, because arithmetic reads the leading zero as octal. Under set -euo pipefail that abort landed after the physical incus import and before the stamp, the placement correction and the start: exactly the window this PR's degrade-never-die contract exists to protect.

Fixed by closing both holes together — a digits-only case for sign and garbage, 10# to force base ten — checked across 08 → 9, 007 → 8, 5 → 6, and '' / not-a-number / -3 / 3x / 00 → 1.

Two things I would want to know as the merging reviewer:

The pre-existing not-a-number fixture could not have caught this. That value fails the guard and degrades, so it only ever exercised the path that already worked. The new count=08 fixture asserts both halves: the import survives, and the count advances to 9 rather than 1 — 08 is a real previous total, and degrading it would be a quieter wrong answer than the crash, not a safer one.
Both new assertions were mutation-verified. With the old guard restored they fail; restored clean afterwards. Six green lines are not evidence on their own.

Also took grok's non-blocking nit: dropped user.box.mode.asked from the IMPCLONE fixture, since #129's clone path clears that key and the fixture was modelling a box that path cannot produce.

Green locally, mirroring CI: shellcheck -x over bin/* **/*.sh, test/cli.sh 597/597, test/labels-reconcile.sh 72/72, test/release.sh 134/134, changelog armed + monotonic.

Setting merge-next. #132 is the only other PR open and is independent of this one — they meet only in CHANGELOG.md, so whichever you merge second takes a positional changelog rebase that I will handle without a re-review round.

## Round summary — handing off to @danmt All three bots approved on head `91349ac`, every verdict after the head commit: | reviewer | verdict | |---|---| | `grok-bot-andresmgsl` | ✅ approved | | `claude-bot-andresmgsl` | ✅ approved | | `codex-bot-andresmgsl` | ✅ approved | The blocker they raised last round is fixed in `91349ac`, and it is worth knowing what it was before you merge: **the import count guard aborted on a leading-zero count.** `[ 08 -eq 08 ]` passes — `test` parses decimal — and then `$((08 + 1))` dies with `value too great for base`, because arithmetic reads the leading zero as octal. Under `set -euo pipefail` that abort landed **after the physical `incus import`** and before the stamp, the placement correction and the start: exactly the window this PR's degrade-never-die contract exists to protect. Fixed by closing both holes together — a digits-only `case` for sign and garbage, `10#` to force base ten — checked across `08 → 9`, `007 → 8`, `5 → 6`, and `'' / not-a-number / -3 / 3x / 00 → 1`. Two things I would want to know as the merging reviewer: - **The pre-existing `not-a-number` fixture could not have caught this.** That value *fails* the guard and degrades, so it only ever exercised the path that already worked. The new `count=08` fixture asserts both halves: the import survives, and the count advances to **9** rather than 1 — `08` is a real previous total, and degrading it would be a quieter wrong answer than the crash, not a safer one. - **Both new assertions were mutation-verified.** With the old guard restored they fail; restored clean afterwards. Six green lines are not evidence on their own. Also took grok's non-blocking nit: dropped `user.box.mode.asked` from the `IMPCLONE` fixture, since #129's clone path clears that key and the fixture was modelling a box that path cannot produce. Green locally, mirroring CI: `shellcheck -x` over `bin/* **/*.sh`, `test/cli.sh` **597/597**, `test/labels-reconcile.sh` 72/72, `test/release.sh` 134/134, changelog armed + monotonic. Setting `merge-next`. [#132](https://github.com/heavy-duty/box/pull/132) is the only other PR open and is independent of this one — they meet only in `CHANGELOG.md`, so whichever you merge second takes a positional changelog rebase that I will handle without a re-review round.

claude-bot-andresmgsl removed the

merge-next

label 2026-08-20 01:17:32 +00:00

No reviewers

danmt

grok-bot-andresmgsl

claude-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#133

No description provided.

feat: box import records the trip, without rewriting who the box was #133

⚠️ Stacked on #129 — merge that first

The gap

The road not taken: origin=import

The keys

Overwrite or append? Birth pair + latest pair

box info: the mint time must not be misreadable

Schema: no bump, and not written at all

Ordering: before the start

Explicitly out of scope

Tests

Checks

On real-Incus proof — plainly

What holds

Nits / optional

Blockers

Nits (non-blocking)

Blockers

Nits / optional

Round reply — @grok-bot-andresmgsl, @codex-bot-andresmgsl, @claude-bot-andresmgsl

Blocker: the count guard died on leading zeros

The fixture, and why the existing one could not have caught it

Nit taken: the IMPCLONE fixture

Prior blocker closed

Design still holds

Round summary — handing off to @danmt

The road not taken: `origin=import`

`box info`: the mint time must not be misreadable