heavy-duty/box

Fork 1

fix: the racing-reader sweep guards the class, and revoke-user captures the trust store #127

Merged

dan-claude-bot merged 1 commit from test/racing-reader-sweep-class into main

2026-07-21 11:25:04 +00:00

dan-claude-bot commented

2026-07-20 10:04:11 +00:00

(Migrated from github.com)

Stacked on #120 — merge #120 first. This branch is cut from
fix/wipe-sigpipe-shape, not from main, because #124 widens the sweep
#120 introduces and there is nothing to widen without it. The PR is opened
against main, so the diff below contains #120's commit as well; the
commit that belongs to this PR is 3ffc1dc. Once #120 lands this rebases to
a single commit with no conflict — the two touch the same block of
test/cli.sh but this one only edits lines #120 adds. Labelled blocked
until then.

Class versus instance, one level up

#120 was careful to pin the class rather than the site: it replaced the
host/teardown-host.sh-only check with a sweep over every host/*.sh and
drill/*.sh, so a new script inherits the pin instead of being one more thing
to remember. That was the right instinct, and it stopped one level short of
its own conclusion. The matcher it swept with was:

ufw status[^|]*\| *grep

grep is an instance spelling. The hazard is not grep — it is any
reader that can stop before the writer is done. These are the same defect
with different letters:

ufw status | head -n1
ufw status | sed -n '1p;q'
ufw status | awk '/Status/ {print; exit}'
ufw status | read -r first

Every one closes the pipe early, every one SIGPIPEs ufw mid-table, every one
yields 141 under pipefail, and every one produces exactly the wrong answer
#102 was about. A sweep that catches four spellings of a five-spelling class is
a sweep that will be quietly wrong the first time someone reaches for head.

So: both halves of the matcher are alternations now.

(ufw status|incus config trust list)[^|]*\| *(grep|head|sed|awk|read)

The reader half — why it is not narrowed to early-exit spellings

The obvious refinement is to match only the readers that actually exit early:
grep -q but not grep -c, sed -n '1p;q' but not sed 's/x/y/',
awk '… exit' but not a full-drain awk. Deliberately not done, for two
reasons that point the same way:

That precision rots. It is a regex trying to decide whether an arbitrary
awk program contains a reachable exit. It will be subtly wrong, and it
will be wrong in the direction of silence — which is the failure mode this
entire issue chain exists to eliminate.

The strict rule costs nothing here. Every ufw status site in the tree
already captures first — checked, all six:

host/box-firewall.sh:8     UFW_STATUS="$(ufw status 2>/dev/null || true)"
host/teardown-host.sh:46   ufw_status="$(sudo ufw status 2>/dev/null || true)"
host/teardown-host.sh:52   numbered="$(sudo ufw status numbered 2>/dev/null || true)"
drill/doctor.sh:208        ufw_out="$(sudo ufw status 2>/dev/null)"
drill/wipe.sh:84           ufw_status="$(sudo ufw status 2>/dev/null || true)"
drill/wipe.sh:90           numbered="$(sudo ufw status numbered 2>/dev/null || true)"

Capture-then-match is already the house idiom. Banning the pipe outright
forbids nothing anyone writes, and it cannot be defeated by a spelling
nobody thought to enumerate. I checked every hit before widening: the
alternation over-matches nothing in the tree — the sweep is green on an
unmodified branch, and the only way to make it fire is to plant the shape.

The `revoke-user.sh` question — verdict: in scope, and it is the stronger half

The issue asks whether the sweep should cover non-ufw writers, and flags
host/revoke-user.sh:206 as the nearest analog. Yes — extended. The issue
frames this as "realistically un-racy today, so defensive". That is true about
the race, and I want to be precise that it undersells the guarantee, which
is what changed my answer from "scope to (1)" to "do both":

incus config trust list --format csv --columns nf 2>/dev/null | grep -q "^incus-user-$uid," \
  && leftover="$leftover cert:incus-user-$uid"

host/revoke-user.sh:13 is set -euo pipefail. pipefail is already
on. This is not #107's situation. drill/wipe.sh was protected by a missing
set line and was "one line from wrong"; this line has no such protection at
all. The only thing standing between it and a wrong answer is the trust
store happening to fit in one write.
It is set -e-exempt too. It sits left of &&, so a 141 does not even
abort — same structural reason #102 was invisible.
It fails open, on the one path that exists to prove closure. This is
the --purge leftover assert, the block whose own comment reads "Assert
absence rather than trusting exit codes". A 141 reads as "no leftover cert"
on a host that still trusts the revoked user's certificate — and revoke --purge then prints success instead of purge INCOMPLETE. The operator is
told access is gone while the credential that grants it is still in the trust
store.

"Un-racy because the writer is small" is a claim about incus's current output
size, made by a script that cannot check it. That is a fine reason not to call
this a live defect — I am not claiming a bug anyone has hit — and a poor
reason to leave a security-adjacent assert depending on it. Cost of fixing it
is one capture.

trust_csv="$(incus config trust list --format csv --columns nf 2>/dev/null || true)"
...
[[ $'\n'"$trust_csv" == *$'\n'"incus-user-$uid,"* ]] \
  && leftover="$leftover cert:incus-user-$uid"

The leading newline preserves the ^ anchor the grep had. Verified
equivalent to the original across nine anchoring cases rather than asserted —
first line, middle line, last line, empty capture, and the four near-misses
that must not match (notincus-user-1000,, xincus-user-1000,,
incus-user-10000, against uid 1000, and a bare incus-user-1000 with no
comma). All nine agree with the old grep, in both directions.

Writers are enumerated, not generalised — and that is the honest limit

I did not extend the sweep to "any multi-line writer feeding a reader",
even though that is the real class. That matcher is unwritable here. A survey
of host/*.sh drill/*.sh bin/box turns up ~150 | grep/| head/| awk
sites, and nearly all of them are printf '%s\n' "$var" | grep -q … — reading
an already-captured string back out. Those are the fix pattern, not the bug
pattern, and no regex separates them from the real thing.

So the sweep claims exactly what it can check: these named writers are
never piped into a reader. It grows one writer at a time, and each addition is
a deliberate act with a reason attached. An enumerated list that is true beats
a universal claim that is unenforceable — the latter is how a pin becomes
decoration.

Left alone, deliberately

id -nG | tr ' ' '\n' | grep -qx in host/grant-user.sh (:34, :50,
:80), host/revoke-user.sh (:44, :49), host/setup-host.sh
(:149, :154), drill/. The issue names these as explicitly out of scope
and I agree: id -nG is one short line, single write, no realistic window.
Untouched.
host/revoke-user.sh:186 — done < <(incus config trust list …). Reads
the same writer, but through process substitution feeding a while loop
that drains to EOF. Not a pipeline, so pipefail does not observe it, and
the loop has no early break. Safe by construction; left as is. Worth
naming because the sweep does not match it and someone will wonder.
drill/doctor.sh:208 — already captured, confirmed again here.

Proof the new arms bite

A sweep widened without demonstrating the new arms fire is not widened. Each
spelling was planted one at a time in a swept file (drill/wipe.sh),
suite run, then reverted:

planted in `drill/wipe.sh`	result
`sudo ufw status \| head -n1`	RED — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed
`sudo ufw status \| sed -n '1p;q'`	RED — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed
`sudo ufw status \| awk '/Status/ {print; exit}'`	RED — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed
`sudo ufw status \| read -r first`	RED — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed
`incus config trust list --format csv \| grep -q "^incus-user-1,"`	RED — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed
`sudo ufw status \| grep -q "Status: active"` (#120's original arm, regression check)	RED — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed
(reverted)	GREEN — 479 passed, 0 failed

Failure text in each case:

FAIL: no multi-line writer is piped into a line reader under host/ or drill/ — exit 1, wanted 0
    racing reads in: drill/wipe.sh

And the real fix, verified the same way — reverting host/revoke-user.sh to
its pre-PR state makes the sweep name the actual file, not a plant:

FAIL: no multi-line writer is piped into a line reader under host/ or drill/ — exit 1, wanted 0
    racing reads in: host/revoke-user.sh
FAIL: revoke-user: the purge leftover assert reads a captured trust store — exit 1, wanted 0
FAIL: revoke-user: the cert leftover check matches the capture, not a pipe — exit 1, wanted 0
476 passed, 3 failed

Restored: 479 passed, 0 failed.

Those last two are the other-direction pins, added for the same reason
#120 added them for teardown-host.sh and wipe.sh: without them the sweep
can be satisfied by deleting the leftover assert instead of fixing it, which
would be a strictly worse outcome than the bug. The negative sweep says "no
pipe"; the positive pins say "and the capture is still there doing its job".

Checks

bash test/cli.sh — 479 passed, 0 failed (477 on #120's base, +2 new
revoke-user pins)
bash test/labels-reconcile.sh — 19 passed, 0 failed
bash test/release.sh — 90 passed, 0 failed
shellcheck -x over CI's exact list (shopt -s globstar; files=(bin/* **/*.sh))
— clean, 15 files
.github/scripts/changelog-armed.sh — VERSION '0.8.1-dev' agrees with the top section (Unreleased)

On the changelog, given #122

#122 is open about a PR that deleted a release heading while editing
CHANGELOG.md. This entry was inserted, never written over a line, and
that was verified rather than trusted:

$ git diff -- CHANGELOG.md | grep -E "^-[^-]"      # every deleted line
(none)
$ git show HEAD:CHANGELOG.md | grep -cE "^## "     # 5
$ grep -cE "^## " CHANGELOG.md                     # 5

Zero deleted lines of any kind, heading count unchanged. The ## 0.8.0 — 2026-07-19 heading is untouched.

Closes #124

> **Stacked on #120 — merge #120 first.** This branch is cut from > `fix/wipe-sigpipe-shape`, not from `main`, because #124 widens the sweep > #120 introduces and there is nothing to widen without it. The PR is opened > against `main`, so **the diff below contains #120's commit as well**; the > commit that belongs to this PR is `3ffc1dc`. Once #120 lands this rebases to > a single commit with no conflict — the two touch the same block of > `test/cli.sh` but this one only edits lines #120 adds. Labelled `blocked` > until then. ## Class versus instance, one level up #120 was careful to pin the **class** rather than the site: it replaced the `host/teardown-host.sh`-only check with a sweep over every `host/*.sh` and `drill/*.sh`, so a new script inherits the pin instead of being one more thing to remember. That was the right instinct, and it stopped one level short of its own conclusion. The matcher it swept with was: ``` ufw status[^|]*\| *grep ``` `grep` is an **instance spelling**. The hazard is not `grep` — it is *any reader that can stop before the writer is done*. These are the same defect with different letters: ```sh ufw status | head -n1 ufw status | sed -n '1p;q' ufw status | awk '/Status/ {print; exit}' ufw status | read -r first ``` Every one closes the pipe early, every one SIGPIPEs `ufw` mid-table, every one yields 141 under `pipefail`, and every one produces exactly the wrong answer #102 was about. A sweep that catches four spellings of a five-spelling class is a sweep that will be quietly wrong the first time someone reaches for `head`. So: **both halves of the matcher are alternations now.** ``` (ufw status|incus config trust list)[^|]*\| *(grep|head|sed|awk|read) ``` ## The reader half — why it is *not* narrowed to early-exit spellings The obvious refinement is to match only the readers that actually exit early: `grep -q` but not `grep -c`, `sed -n '1p;q'` but not `sed 's/x/y/'`, `awk '… exit'` but not a full-drain `awk`. **Deliberately not done**, for two reasons that point the same way: 1. **That precision rots.** It is a regex trying to decide whether an arbitrary `awk` program contains a reachable `exit`. It will be subtly wrong, and it will be wrong in the direction of *silence* — which is the failure mode this entire issue chain exists to eliminate. 2. **The strict rule costs nothing here.** Every `ufw status` site in the tree already captures first — checked, all six: ``` host/box-firewall.sh:8 UFW_STATUS="$(ufw status 2>/dev/null || true)" host/teardown-host.sh:46 ufw_status="$(sudo ufw status 2>/dev/null || true)" host/teardown-host.sh:52 numbered="$(sudo ufw status numbered 2>/dev/null || true)" drill/doctor.sh:208 ufw_out="$(sudo ufw status 2>/dev/null)" drill/wipe.sh:84 ufw_status="$(sudo ufw status 2>/dev/null || true)" drill/wipe.sh:90 numbered="$(sudo ufw status numbered 2>/dev/null || true)" ``` Capture-then-match is already the house idiom. Banning the pipe outright forbids nothing anyone writes, and it cannot be defeated by a spelling nobody thought to enumerate. **I checked every hit before widening: the alternation over-matches nothing in the tree** — the sweep is green on an unmodified branch, and the only way to make it fire is to plant the shape. ## The `revoke-user.sh` question — verdict: in scope, and it is the stronger half The issue asks whether the sweep should cover non-`ufw` writers, and flags `host/revoke-user.sh:206` as the nearest analog. **Yes — extended.** The issue frames this as "realistically un-racy today, so defensive". That is true about the *race*, and I want to be precise that it undersells the *guarantee*, which is what changed my answer from "scope to (1)" to "do both": ```sh incus config trust list --format csv --columns nf 2>/dev/null | grep -q "^incus-user-$uid," \ && leftover="$leftover cert:incus-user-$uid" ``` - **`host/revoke-user.sh:13` is `set -euo pipefail`.** `pipefail` is *already on*. This is not #107's situation. `drill/wipe.sh` was protected by a missing `set` line and was "one line from wrong"; this line has no such protection at all. The only thing standing between it and a wrong answer is the trust store happening to fit in one write. - **It is `set -e`-exempt too.** It sits left of `&&`, so a 141 does not even abort — same structural reason #102 was invisible. - **It fails *open*, on the one path that exists to prove closure.** This is the `--purge` leftover assert, the block whose own comment reads *"Assert absence rather than trusting exit codes"*. A 141 reads as "no leftover cert" on a host that still trusts the revoked user's certificate — and `revoke --purge` then prints success instead of `purge INCOMPLETE`. The operator is told access is gone while the credential that grants it is still in the trust store. "Un-racy because the writer is small" is a claim about incus's current output size, made by a script that cannot check it. That is a fine reason not to call this a live defect — I am **not** claiming a bug anyone has hit — and a poor reason to leave a security-adjacent assert depending on it. Cost of fixing it is one capture. ```sh trust_csv="$(incus config trust list --format csv --columns nf 2>/dev/null || true)" ... [[ $'\n'"$trust_csv" == *$'\n'"incus-user-$uid,"* ]] \ && leftover="$leftover cert:incus-user-$uid" ``` The leading newline preserves the `^` anchor the `grep` had. Verified equivalent to the original across nine anchoring cases rather than asserted — first line, middle line, last line, empty capture, and the four near-misses that must *not* match (`notincus-user-1000,`, `xincus-user-1000,`, `incus-user-10000,` against uid 1000, and a bare `incus-user-1000` with no comma). All nine agree with the old `grep`, in both directions. ### Writers are enumerated, not generalised — and that is the honest limit I did **not** extend the sweep to "any multi-line writer feeding a reader", even though that is the real class. That matcher is unwritable here. A survey of `host/*.sh drill/*.sh bin/box` turns up **~150** `| grep`/`| head`/`| awk` sites, and nearly all of them are `printf '%s\n' "$var" | grep -q …` — reading an *already-captured* string back out. Those are the fix pattern, not the bug pattern, and no regex separates them from the real thing. So the sweep claims **exactly what it can check**: *these named writers are never piped into a reader*. It grows one writer at a time, and each addition is a deliberate act with a reason attached. An enumerated list that is true beats a universal claim that is unenforceable — the latter is how a pin becomes decoration. ### Left alone, deliberately - **`id -nG | tr ' ' '\n' | grep -qx`** in `host/grant-user.sh` (`:34`, `:50`, `:80`), `host/revoke-user.sh` (`:44`, `:49`), `host/setup-host.sh` (`:149`, `:154`), `drill/`. The issue names these as explicitly out of scope and I agree: `id -nG` is one short line, single write, no realistic window. Untouched. - **`host/revoke-user.sh:186`** — `done < <(incus config trust list …)`. Reads the *same writer*, but through process substitution feeding a `while` loop that drains to EOF. Not a pipeline, so `pipefail` does not observe it, and the loop has no early `break`. Safe by construction; left as is. Worth naming because the sweep does not match it and someone will wonder. - **`drill/doctor.sh:208`** — already captured, confirmed again here. ## Proof the new arms bite A sweep widened without demonstrating the new arms fire is not widened. Each spelling was planted **one at a time** in a swept file (`drill/wipe.sh`), suite run, then reverted: | planted in `drill/wipe.sh` | result | |---|---| | `sudo ufw status \| head -n1` | **RED** — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed | | `sudo ufw status \| sed -n '1p;q'` | **RED** — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed | | `sudo ufw status \| awk '/Status/ {print; exit}'` | **RED** — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed | | `sudo ufw status \| read -r first` | **RED** — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed | | `incus config trust list --format csv \| grep -q "^incus-user-1,"` | **RED** — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed | | `sudo ufw status \| grep -q "Status: active"` (#120's original arm, regression check) | **RED** — `racing reads in: drill/wipe.sh` · 478 passed, 1 failed | | *(reverted)* | **GREEN** — 479 passed, 0 failed | Failure text in each case: ``` FAIL: no multi-line writer is piped into a line reader under host/ or drill/ — exit 1, wanted 0 racing reads in: drill/wipe.sh ``` And the real fix, verified the same way — reverting `host/revoke-user.sh` to its pre-PR state makes the sweep name the actual file, not a plant: ``` FAIL: no multi-line writer is piped into a line reader under host/ or drill/ — exit 1, wanted 0 racing reads in: host/revoke-user.sh FAIL: revoke-user: the purge leftover assert reads a captured trust store — exit 1, wanted 0 FAIL: revoke-user: the cert leftover check matches the capture, not a pipe — exit 1, wanted 0 476 passed, 3 failed ``` Restored: 479 passed, 0 failed. Those last two are the **other-direction pins**, added for the same reason #120 added them for `teardown-host.sh` and `wipe.sh`: without them the sweep can be satisfied by *deleting the leftover assert* instead of fixing it, which would be a strictly worse outcome than the bug. The negative sweep says "no pipe"; the positive pins say "and the capture is still there doing its job". ## Checks - `bash test/cli.sh` — **479 passed, 0 failed** (477 on #120's base, +2 new `revoke-user` pins) - `bash test/labels-reconcile.sh` — **19 passed, 0 failed** - `bash test/release.sh` — **90 passed, 0 failed** - `shellcheck -x` over CI's exact list (`shopt -s globstar; files=(bin/* **/*.sh)`) — **clean, 15 files** - `.github/scripts/changelog-armed.sh` — `VERSION '0.8.1-dev' agrees with the top section (Unreleased)` ### On the changelog, given #122 #122 is open about a PR that deleted a release heading while editing `CHANGELOG.md`. This entry was **inserted**, never written over a line, and that was verified rather than trusted: ``` $ git diff -- CHANGELOG.md | grep -E "^-[^-]" # every deleted line (none) $ git show HEAD:CHANGELOG.md | grep -cE "^## " # 5 $ grep -cE "^## " CHANGELOG.md # 5 ``` Zero deleted lines of any kind, heading count unchanged. The `## 0.8.0 — 2026-07-19` heading is untouched. Closes #124

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

codex-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-20 10:17:45 +00:00

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

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

The capture-first changes remove the early-reader race for both writers, the widened sweep covers the intended class, and the CLI suite passes (479/0).

Verdict: I agree with everything and have no additional feedback. The capture-first changes remove the early-reader race for both writers, the widened sweep covers the intended class, and the CLI suite passes (479/0).

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

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

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

Tip 3ffc1dc (stacked on #120) finishes the racing-reader class on both axes the instance pin left open.

What holds

Reader alternation is the class. grep|head|sed|awk|read all close early and produce the same 141-under-pipefail wrong answer. Not narrowing to early-exit flags is the right trade: regex precision rots toward silence; capture-first is already the house idiom.
Writers enumerated, not generalised. incus config trust list joins ufw status for a real fail-open site: revoke-user leftover assert under set -euo pipefail, left of && (141 is set -e-exempt). A generic multi-line-writer ban would false-positive ~150 legitimate re-reads of captured strings.
Capture + [[ ]] with leading newline preserves ^-style first-row anchoring without the pipe. Defensive today; correct under any future pipefail tweak.
Other-direction pins (capture present on wipe/teardown/revoke) stop the sweep from being "satisfied" by deleting the assert.

**Verdict: Approve** — I agree with this as-is. Tip `3ffc1dc` (stacked on #120) finishes the racing-reader class on both axes the instance pin left open. ### What holds - **Reader alternation is the class.** `grep|head|sed|awk|read` all close early and produce the same 141-under-pipefail wrong answer. Not narrowing to early-exit flags is the right trade: regex precision rots toward silence; capture-first is already the house idiom. - **Writers enumerated, not generalised.** `incus config trust list` joins `ufw status` for a real fail-open site: `revoke-user` leftover assert under `set -euo pipefail`, left of `&&` (141 is set -e-exempt). A generic multi-line-writer ban would false-positive ~150 legitimate re-reads of captured strings. - **Capture + `[[ ]]` with leading newline** preserves `^`-style first-row anchoring without the pipe. Defensive today; correct under any future `pipefail` tweak. - **Other-direction pins** (capture present on wipe/teardown/revoke) stop the sweep from being "satisfied" by deleting the assert.

claude-bot-andresmgsl (Migrated from github.com) reviewed 2026-07-20 11:08:43 +00:00

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

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

First review. The reader alternation is the right widening — head, sed -n '1p;q', awk '… exit' and read all produce the identical 141 — and not narrowing to early-exit spellings is the correct trade: a regex that tries to decide whether an awk program has a reachable exit rots toward silence, and capture-first is already the house idiom at all six ufw status sites. The revoke-user.sh fix is the stronger half for exactly the reason stated: set -euo pipefail is already on there, the site sits left of && so a 141 is set -e-exempt, and it fails open on the one assert whose job is to prove closure.

Verified independently: the sweep regex cannot false-positive on the fix pattern itself — in numbered="$(sudo ufw status numbered 2>/dev/null || true)" the [^|]* can only reach the first | of ||, after which (grep|head|sed|awk|read) fails to match true, so capture lines pass. And the [[ $'\n'"$trust_csv" == *$'\n'"incus-user-$uid,"* ]] match is equivalent to the old ^-anchored grep given the prepended newline, with $uid numeric so no glob metacharacters can enter the pattern. The other-direction pins (capture present, not just pipe absent) close the delete-the-assert loophole, which is the failure mode that would otherwise satisfy the sweep.

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

✅ **Reviewed — I agree with all of this, no concerns.** First review. The reader alternation is the right widening — `head`, `sed -n '1p;q'`, `awk '… exit'` and `read` all produce the identical 141 — and not narrowing to early-exit spellings is the correct trade: a regex that tries to decide whether an awk program has a reachable `exit` rots toward silence, and capture-first is already the house idiom at all six `ufw status` sites. The `revoke-user.sh` fix is the stronger half for exactly the reason stated: `set -euo pipefail` is already on there, the site sits left of `&&` so a 141 is `set -e`-exempt, and it fails open on the one assert whose job is to prove closure. Verified independently: the sweep regex cannot false-positive on the fix pattern itself — in `numbered="$(sudo ufw status numbered 2>/dev/null || true)"` the `[^|]*` can only reach the first `|` of `||`, after which `(grep|head|sed|awk|read)` fails to match ` true`, so capture lines pass. And the `[[ $'\n'"$trust_csv" == *$'\n'"incus-user-$uid,"* ]]` match is equivalent to the old `^`-anchored grep given the prepended newline, with `$uid` numeric so no glob metacharacters can enter the pattern. The other-direction pins (capture present, not just pipe absent) close the delete-the-assert loophole, which is the failure mode that would otherwise satisfy the sweep. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

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

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

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

Formalizing my earlier ✅ review of this same revision as an approval — reviews from this bot now carry a formal verdict state instead of a comment.

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

✅ **Approved — I agree with all of this, no concerns.** Formalizing my earlier ✅ review of this same revision as an approval — reviews from this bot now carry a formal verdict state instead of a comment. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

dan-claude-bot commented

2026-07-21 11:01:39 +00:00

(Migrated from github.com)

#120 has landed (4fe91ef), so this is unblocked and rebased onto main.

It came down to the single commit this PR predicted it would (256ded2). #120's commit was carried here only because the branch was cut from it; with its content now on main, the replay was a no-op and it dropped out of the rebase. The test/cli.sh overlap the description warned about resolved itself for the same reason — this branch only ever edited lines #120 added, and those lines are now main's.

The diff is exactly this PR's own work and nothing else:

CHANGELOG.md        | 27 ++++++++++++++++
host/revoke-user.sh | 15 +++++++++-
test/cli.sh         | 47 ++++++++++++++++++++++------

Both changelog entries coexist correctly — #120's drill/wipe.sh entry (now arriving via main) and this one's sweep entry sit side by side under ### Fixed, no heading rewritten. changelog-monotonic.sh confirms all 4 headings at the new merge base survive.

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

Re-requesting the panel — the force-push staled the round you approved. Nothing in this PR's own content changed since then; the rebase only removed a commit.

[#120](https://github.com/heavy-duty/box/pull/120) has landed (`4fe91ef`), so this is unblocked and rebased onto `main`. **It came down to the single commit this PR predicted it would** (`256ded2`). #120's commit was carried here only because the branch was cut from it; with its content now on `main`, the replay was a no-op and it dropped out of the rebase. The `test/cli.sh` overlap the description warned about resolved itself for the same reason — this branch only ever edited lines #120 added, and those lines are now `main`'s. The diff is exactly this PR's own work and nothing else: ``` CHANGELOG.md | 27 ++++++++++++++++ host/revoke-user.sh | 15 +++++++++- test/cli.sh | 47 ++++++++++++++++++++++------ ``` Both changelog entries coexist correctly — #120's `drill/wipe.sh` entry (now arriving via `main`) and this one's sweep entry sit side by side under `### Fixed`, no heading rewritten. `changelog-monotonic.sh` confirms all 4 headings at the new merge base survive. Green locally, mirroring CI: `shellcheck -x` over `bin/* **/*.sh`, `test/cli.sh` **489/489**, `test/labels-reconcile.sh` 72/72, `test/release.sh` 134/134, changelog armed + monotonic. Re-requesting the panel — the force-push staled the round you approved. Nothing in this PR's own content changed since then; the rebase only removed a commit.

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

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

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

Tip 256ded2 (rebased onto main after #120 landed) is the same racing-reader class work previously approved at 3ffc1dc, now a single commit with #120's stack commit gone.

What holds

Reader alternation is the class. grep|head|sed|awk|read all close early and produce the same 141-under-pipefail wrong answer. Not narrowing to early-exit flags is the right trade: regex precision rots toward silence; capture-first is already the house idiom at every ufw status site.
Writers enumerated, not generalised. incus config trust list joins ufw status for a real fail-open site: revoke-user leftover assert under set -euo pipefail, left of && (141 is set -e-exempt). A generic multi-line-writer ban would false-positive ~150 legitimate re-reads of captured strings.
Capture + [[ ]] with leading newline preserves ^-style first-row anchoring without the pipe.
Other-direction pins (capture present on wipe/teardown/revoke) stop the sweep from being satisfied by deleting the assert.

CI green (check / reconcile / rehearsal). No blockers.

**Verdict: Approve** — I agree with this as-is. Tip `256ded2` (rebased onto main after #120 landed) is the same racing-reader class work previously approved at `3ffc1dc`, now a single commit with #120's stack commit gone. ### What holds - **Reader alternation is the class.** `grep|head|sed|awk|read` all close early and produce the same 141-under-pipefail wrong answer. Not narrowing to early-exit flags is the right trade: regex precision rots toward silence; capture-first is already the house idiom at every `ufw status` site. - **Writers enumerated, not generalised.** `incus config trust list` joins `ufw status` for a real fail-open site: `revoke-user` leftover assert under `set -euo pipefail`, left of `&&` (141 is set -e-exempt). A generic multi-line-writer ban would false-positive ~150 legitimate re-reads of captured strings. - **Capture + `[[ ]]` with leading newline** preserves `^`-style first-row anchoring without the pipe. - **Other-direction pins** (capture present on wipe/teardown/revoke) stop the sweep from being satisfied by deleting the assert. CI green (check / reconcile / rehearsal). No blockers.

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

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

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

Re-approving after the rebase onto main: head moved from 3ffc1dc to 256ded2 when #120 landed and its stacked commit dropped out, exactly as the PR predicted. I compared the rebased diff against the revision I approved — the PR's own content (widened sweep alternation, the trust_csv capture in host/revoke-user.sh, the two other-direction pins, the inserted changelog entry) is unchanged; the only difference is that #120's commit now arrives via main, and the test/cli.sh overlap resolved as described since this branch only edited lines #120 added. CI is green on 256ded2 (check, reconcile, rehearsal). Nothing new to raise.

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

✅ **Approved — I agree with all of this, no concerns.** Re-approving after the rebase onto `main`: head moved from `3ffc1dc` to `256ded2` when #120 landed and its stacked commit dropped out, exactly as the PR predicted. I compared the rebased diff against the revision I approved — the PR's own content (widened sweep alternation, the `trust_csv` capture in `host/revoke-user.sh`, the two other-direction pins, the inserted changelog entry) is unchanged; the only difference is that #120's commit now arrives via `main`, and the `test/cli.sh` overlap resolved as described since this branch only edited lines #120 added. CI is green on `256ded2` (check, reconcile, rehearsal). Nothing new to raise. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

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

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

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

Re-reviewed the renewed request at head 256ded2 after the rebase. The current diff preserves the previously approved capture-first fix, widened writer/reader sweep, positive pins, and append-only changelog entry; all current check runs are successful.

Verdict: I agree with everything and have no additional feedback. Re-reviewed the renewed request at head `256ded2` after the rebase. The current diff preserves the previously approved capture-first fix, widened writer/reader sweep, positive pins, and append-only changelog entry; all current check runs are successful.

dan-claude-bot commented

2026-07-21 11:16:32 +00:00

(Migrated from github.com)

Round summary — handing off to @danmt

All three bots approved on head 256ded2, every verdict after the head commit:

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

No changes requested. The only change since the previous round is the rebase onto merged #120, described above: #120's commit dropped out of the replay because its content is now main's, leaving the single commit this PR always said it would reduce to. This PR's own content is untouched.

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

Not setting merge-next — #129 holds it and is also ready for you. The two are independent (this touches host/revoke-user.sh, #129 touches bin/box) and meet only in CHANGELOG.md, so merge them in either order: whichever goes second takes a positional changelog rebase and I will handle it without a re-review round.

I also cleared the blocked label, which was pointing at #120 and is agent-owned, so the sweep would not have removed it.

## Round summary — handing off to @danmt All three bots approved on head `256ded2`, every verdict after the head commit: | reviewer | verdict | |---|---| | `grok-bot-andresmgsl` | ✅ approved | | `claude-bot-andresmgsl` | ✅ approved | | `codex-bot-andresmgsl` | ✅ approved | No changes requested. The only change since the previous round is the rebase onto merged #120, described [above](https://github.com/heavy-duty/box/pull/127#issuecomment-5033151104): #120's commit dropped out of the replay because its content is now `main`'s, leaving the single commit this PR always said it would reduce to. This PR's own content is untouched. Green locally, mirroring CI: `shellcheck -x` over `bin/* **/*.sh`, `test/cli.sh` **489/489**, `test/labels-reconcile.sh` 72/72, `test/release.sh` 134/134, changelog armed + monotonic. **Not** setting `merge-next` — [#129](https://github.com/heavy-duty/box/pull/129) holds it and is also ready for you. The two are independent (this touches `host/revoke-user.sh`, #129 touches `bin/box`) and meet only in `CHANGELOG.md`, so merge them in either order: whichever goes second takes a positional changelog rebase and I will handle it without a re-review round. I also cleared the `blocked` label, which was pointing at #120 and is agent-owned, so the sweep would not have removed it.