heavy-duty/rig

Fork 1

refactor: one drill record per version, in drills/ #104

Merged

dan-claude-bot merged 3 commits from refactor/drills-per-version into main

2026-07-21 17:41:34 +00:00

dan-claude-bot commented

2026-07-21 16:37:21 +00:00

(Migrated from github.com)

Drill records move from sections inside drill/RUNS.md to one file per version: drills/<version>.md.

Why — it deletes the bug class, not just tidies

The guard shipped in the previous PR had to parse headings: em-dash field matching, an optional — DATE tail, whole-version comparison so X.Y.Z-rc1 could not satisfy X.Y.Z, avoiding \x escapes because CI runs mawk not gawk, and a non-blank body rule.

Every one of those existed only because records shared a file. And that complexity produced two real defects in review, both caught by all three reviewers:

sed '/./,$!d' treated a whitespace-only body as content — . matches a space — so a heading followed by one tab satisfied the gate. An evidence-free release for the price of an invisible character, on the check whose entire job is to demand evidence.
Heading grammar drifted between siblings: cast accepted ## Release drill — 0.2.0 stray words, box did not.

One file per version makes nearly all of it unrepresentable. 0.9.0.md and 0.9.0-rc1.md are simply different files, so whole-version matching is the filesystem's problem rather than a comparison anyone can get wrong. There is no heading left to parse, so there is no grammar left to drift.

What survives is the one rule that was never really about parsing: a file of only whitespace is not a record. [ -f ] is happy with it, so existence alone is a claim touch can defeat.

The guard is now:

record="$drills/$ver.md"
if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"; then ... fi

Executable logic dropped by roughly two thirds; the file barely shrank because the comments now carry the history of why the complexity is gone.

`drills/`, not `.drills/`

Deliberate. A dot-directory is invisible to a glob without dotglob, which is exactly the blind spot behind box#116/#118 and rig#70 — a sweep that reads green because it never descended into the directory holding the thing it was meant to check. Evidence a sweep cannot see is evidence that goes missing quietly.

The framing correction that rides along

The docs previously described the drill as one orchestrated stack run, implying the three repos must be drilled together in sequence. That over-constrains it and is now corrected everywhere:

The three drills are independent — any order, any schedule, separate sittings.
What makes that safe is that every drill pins the same fixed set of candidate refs, so each measures the combination that will ship rather than whatever main is that afternoon.
Pinning, not sequencing, is what dissolves the box↔rig recursion. box and rig are mutually recursive — rig builds the host that runs box, box's seed calls rig back to converge the guest — but candidate refs are static identifiers that exist as soon as the release branches do, long before any drill runs. A runtime cycle becomes independent tests against one fixed pair.
Each repo drills a different thing: box asserts the isolation contract, rig asserts convergence, cast asserts promotion. Three exercises over a shared substrate — which is exactly why records are per-repo.
If a defect appears only in the combination: patch, re-drill, re-record. The releases converge; they need not be right in one pass.

Substrate-before-guest is framed as how you run a single drill, not an inter-repo ordering rule. Tests pin the corrected claims, including a negative that fails if fixed-order language returns.

Verification

Per-repo results are in the commit; all suites green, all guards pass on their own tree, shellcheck sweeps clean with the rewritten script covered.

Hand-verified in a mktemp dir, not just via fixtures: a bare-version tree with no drills/ exits 1 with the full unblock message; one with a real record exits 0.

Note on the open release PR

It is currently red on drill-recorded — correctly, since no drill has been run. This PR changes where the record goes, not whether one is required, so it stays red until the drill lands. It carries blocker:drill-pending.

Drill records move from sections inside `drill/RUNS.md` to **one file per version**: `drills/<version>.md`. ## Why — it deletes the bug class, not just tidies The guard shipped in the previous PR had to parse headings: em-dash field matching, an optional ` — DATE` tail, whole-version comparison so `X.Y.Z-rc1` could not satisfy `X.Y.Z`, avoiding `\x` escapes because CI runs mawk not gawk, and a non-blank body rule. **Every one of those existed only because records shared a file.** And that complexity produced two real defects in review, both caught by all three reviewers: - `sed '/./,$!d'` treated a whitespace-only body as content — `.` matches a space — so a heading followed by one tab satisfied the gate. An evidence-free release for the price of an invisible character, on the check whose entire job is to demand evidence. - Heading grammar drifted between siblings: cast accepted `## Release drill — 0.2.0 stray words`, box did not. One file per version makes nearly all of it **unrepresentable**. `0.9.0.md` and `0.9.0-rc1.md` are simply different files, so whole-version matching is the filesystem's problem rather than a comparison anyone can get wrong. There is no heading left to parse, so there is no grammar left to drift. What survives is the one rule that was never really about parsing: **a file of only whitespace is not a record**. `[ -f ]` is happy with it, so existence alone is a claim `touch` can defeat. The guard is now: ```sh record="$drills/$ver.md" if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"; then ... fi ``` Executable logic dropped by roughly two thirds; the file barely shrank because the comments now carry the history of why the complexity is gone. ## `drills/`, not `.drills/` Deliberate. A dot-directory is invisible to a glob without `dotglob`, which is exactly the blind spot behind box#116/#118 and rig#70 — a sweep that reads green because it never descended into the directory holding the thing it was meant to check. Evidence a sweep cannot see is evidence that goes missing quietly. ## The framing correction that rides along The docs previously described the drill as **one orchestrated stack run**, implying the three repos must be drilled together in sequence. That over-constrains it and is now corrected everywhere: - **The three drills are independent** — any order, any schedule, separate sittings. - What makes that safe is that every drill **pins the same fixed set of candidate refs**, so each measures the combination that will ship rather than whatever `main` is that afternoon. - **Pinning, not sequencing, is what dissolves the box↔rig recursion.** box and rig *are* mutually recursive — rig builds the host that runs box, box's seed calls rig back to converge the guest — but candidate refs are static identifiers that exist as soon as the release branches do, long before any drill runs. A runtime cycle becomes independent tests against one fixed pair. - Each repo drills a **different thing**: box asserts the isolation contract, rig asserts convergence, cast asserts promotion. Three exercises over a shared substrate — which is exactly why records are per-repo. - If a defect appears only in the combination: patch, re-drill, re-record. The releases converge; they need not be right in one pass. Substrate-before-guest is framed as how you run a single drill, not an inter-repo ordering rule. Tests pin the corrected claims, including a **negative** that fails if fixed-order language returns. ## Verification Per-repo results are in the commit; all suites green, all guards pass on their own tree, shellcheck sweeps clean with the rewritten script covered. Hand-verified in a `mktemp` dir, not just via fixtures: a bare-version tree with **no** `drills/` exits 1 with the full unblock message; one with a real record exits 0. ## Note on the open release PR It is currently red on `drill-recorded` — correctly, since no drill has been run. This PR changes *where* the record goes, not whether one is required, so it stays red until the drill lands. It carries `blocker:drill-pending`.

danmt (Migrated from github.com) reviewed 2026-07-21 16:37:21 +00:00

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

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

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

First review. Tip moves drill evidence from section-in-drill/RUNS.md to one file per version at drills/<version>.md.

What holds

Deletes the bug class that produced the prior-round defects: no heading grammar → no em-dash field drift; whole-version match is the filesystem (0.9.0.md vs 0.9.0-rc1.md).
Surviving non-blank rule is correct: [ ! -f ] || ! grep -q '[^[:space:]]' — verified spaces/tabs-only file fails; content passes; missing file fails; -dev vacuous with no dir.
drills/ not .drills/ — correct lesson from #118 / shellcheck globstar blind spot.
Keeps drill/RUNS.md as harness log distinct from release evidence — docs and failure text say so.
Record-not-pass / waiver path still honest; tests pin empty, whitespace-only, rc↔bare, missing dir, missing VERSION, README, CI wiring.

No blockers.

Tip 11dfeb2. Strongest fixture set of the three (blank body, rc both ways, no drills dir, README pins).

**Verdict: Approve** — I agree with this as-is. First review. Tip moves drill evidence from section-in-`drill/RUNS.md` to **one file per version** at `drills/<version>.md`. ### What holds - **Deletes the bug class** that produced the prior-round defects: no heading grammar → no em-dash field drift; whole-version match is the filesystem (`0.9.0.md` vs `0.9.0-rc1.md`). - **Surviving non-blank rule is correct:** `[ ! -f ] || ! grep -q '[^[:space:]]'` — verified spaces/tabs-only file fails; content passes; missing file fails; `-dev` vacuous with no dir. - **`drills/` not `.drills/`** — correct lesson from #118 / shellcheck globstar blind spot. - **Keeps `drill/RUNS.md` as harness log** distinct from release evidence — docs and failure text say so. - **Record-not-pass** / waiver path still honest; tests pin empty, whitespace-only, rc↔bare, missing dir, missing VERSION, README, CI wiring. No blockers. Tip `11dfeb2`. Strongest fixture set of the three (blank body, rc both ways, no drills dir, README pins).

dan-claude-bot commented

2026-07-21 16:43:07 +00:00

(Migrated from github.com)

Pushed a second commit after a real defect surfaced the moment the gate met a release branch.

What went wrong

The gate PR's self-test asserted the guard always passes on our own tree:

check "drill-recorded: THIS tree passes its own guard" 0 "" \
  bash "$DRILLED" "$ROOT/drills" "$ROOT/VERSION"

On a -dev tree the guard is vacuous, so it passed and looked fine. On a ceremony tree it cannot pass until a human has run the drill and written the record — which is the entire point of the gate.

Consequences, both real:

test/release.sh became un-greenable on every release branch before its drill. An author preparing a release could not get a green suite locally, through no fault of their own.
CI reported it as a release-flow tests failure rather than as the drill gate doing its job. The signal was misattributed — you would go looking for a broken test.

Observed live: rebasing the release PR onto the gated main turned it red on release-flow tests, not on drill-recorded. That is what sent me looking.

This is the same shape as #146 — a check coupled to the repo's real VERSION that behaves one way on -dev and another on the ceremony tree, so it only misbehaves during a release. I reintroduced it in the very PR that fixed the last instance.

The fix

Assert the property that is actually true: the guard's verdict is correct for this tree, not that it always passes.

-dev tree → vacuous, must pass
ceremony tree with its record → must pass
ceremony tree without a record → must be refused, and say so

Green in all three states, and still red if the guard ever disagrees with the tree in front of it — which is the thing worth testing.

The sibling "default arguments, as CI runs it" check got the same treatment: it now asserts the defaults reach the same verdict as the explicit call, rather than a hard-coded 0. What that test is for is proving the defaults are the paths this repo uses, and comparing verdicts says that without re-importing the bug.

Verified in both states, not just the current one

Each suite was run on its -dev tree and again on a simulated full ceremony tree — bare version and stamped changelog, no drill record:

repo	`-dev` tree	ceremony tree, no record
box	173 passed, 0 failed	173 passed, 0 failed
rig	134 passed, 0 failed	134 passed, 0 failed
cast	78 passed (release suite)	78 passed

And the gate still refuses the undrilled ceremony tree — that assertion is now what the suite checks, rather than something the suite trips over.

Full sweeps re-run: box and rig shellcheck clean under CI's exact globstar/dotglob invocation; cast npm test 784, biome clean.

Pushed a second commit after a real defect surfaced the moment the gate met a release branch. ## What went wrong The gate PR's self-test asserted the guard **always passes on our own tree**: ``` check "drill-recorded: THIS tree passes its own guard" 0 "" \ bash "$DRILLED" "$ROOT/drills" "$ROOT/VERSION" ``` On a `-dev` tree the guard is vacuous, so it passed and looked fine. On a **ceremony tree it cannot pass** until a human has run the drill and written the record — which is the entire point of the gate. Consequences, both real: 1. `test/release.sh` became **un-greenable on every release branch** before its drill. An author preparing a release could not get a green suite locally, through no fault of their own. 2. CI reported it as a **`release-flow tests` failure** rather than as the drill gate doing its job. The signal was misattributed — you would go looking for a broken test. Observed live: rebasing the release PR onto the gated `main` turned it red on `release-flow tests`, not on `drill-recorded`. That is what sent me looking. This is the same shape as #146 — a check coupled to the repo's real `VERSION` that behaves one way on `-dev` and another on the ceremony tree, so it only misbehaves during a release. I reintroduced it in the very PR that fixed the last instance. ## The fix Assert the property that is actually true: **the guard's verdict is correct for this tree**, not that it always passes. - `-dev` tree → vacuous, must pass - ceremony tree **with** its record → must pass - ceremony tree **without** a record → must be *refused*, and say so Green in all three states, and still red if the guard ever disagrees with the tree in front of it — which is the thing worth testing. The sibling "default arguments, as CI runs it" check got the same treatment: it now asserts the defaults reach the **same verdict** as the explicit call, rather than a hard-coded 0. What that test is for is proving the defaults are the paths this repo uses, and comparing verdicts says that without re-importing the bug. ## Verified in both states, not just the current one Each suite was run on its `-dev` tree and again on a **simulated full ceremony tree** — bare version *and* stamped changelog, no drill record: | repo | `-dev` tree | ceremony tree, no record | |---|---|---| | box | 173 passed, 0 failed | **173 passed, 0 failed** | | rig | 134 passed, 0 failed | **134 passed, 0 failed** | | cast | 78 passed (release suite) | **78 passed** | And the gate still refuses the undrilled ceremony tree — that assertion is now what the suite checks, rather than something the suite trips over. Full sweeps re-run: box and rig shellcheck clean under CI's exact globstar/dotglob invocation; cast `npm test` 784, biome clean.

claude-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-21 16:43:49 +00:00

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

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

The mechanics are right and match the siblings: the filesystem does the whole-version match, whitespace-is-not-a-record survives as the POSIX-class grep, the no-dir / other-version / empty / whitespace-only / rc↔final cases are all pinned, and deleting rig's drill/RUNS.md is correct since it held no real records and the test asserts it stays gone. One defect:

.github/scripts/drill-recorded.sh:103 — the comment justifying the if/then shape is factually wrong. It claims that under set -e a bare [ -f "$record" ] && recorded=yes "takes the whole script down with its exit status — so a miss would exit 1 here, before the failure message below ever printed". It would not: a command on the left-hand side of && is exempt from set -e, so on a miss the script simply continues with recorded=no and the failure message prints fine. Verified: bash -ec '[ -f /nonexistent ] && r=yes; echo reached' prints reached and exits 0. (The real gotcha is the opposite one — a && b as the last line of a script makes the script's overall exit status 1 on a miss — and it doesn't apply mid-script.) This guard family just spent #149/box#149/cast#138 on exactly this class: a comment asserting a contract the code's semantics don't match. Either correct/drop the rationale, or adopt the siblings' if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record" form so there is no divergence to explain.

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

🔧 **Changes requested — I agree with most; feedback below.** The mechanics are right and match the siblings: the filesystem does the whole-version match, whitespace-is-not-a-record survives as the POSIX-class grep, the no-dir / other-version / empty / whitespace-only / rc↔final cases are all pinned, and deleting rig's drill/RUNS.md is correct since it held no real records and the test asserts it stays gone. One defect: - `.github/scripts/drill-recorded.sh:103` — the comment justifying the if/then shape is factually wrong. It claims that under `set -e` a bare `[ -f "$record" ] && recorded=yes` "takes the whole script down with its exit status — so a miss would exit 1 here, before the failure message below ever printed". It would not: a command on the left-hand side of `&&` is exempt from `set -e`, so on a miss the script simply continues with `recorded=no` and the failure message prints fine. Verified: `bash -ec '[ -f /nonexistent ] && r=yes; echo reached'` prints `reached` and exits 0. (The real gotcha is the opposite one — `a && b` as the *last* line of a script makes the script's overall exit status 1 on a miss — and it doesn't apply mid-script.) This guard family just spent #149/box#149/cast#138 on exactly this class: a comment asserting a contract the code's semantics don't match. Either correct/drop the rationale, or adopt the siblings' `if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"` form so there is no divergence to explain. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

codex-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-21 16:46:40 +00:00

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

Verdict: I have feedback.

.github/scripts/drill-recorded.sh incorrectly says [ -f "$record" ] && recorded=yes would exit under set -e before the later failure message. A failed command on the left side of && is exempt from errexit, so this mid-script statement would continue. Please correct or remove that rationale (or use the siblings' direct conditional) so the guard's executable documentation matches Bash semantics.

Verdict: I have feedback. .github/scripts/drill-recorded.sh incorrectly says `[ -f "$record" ] && recorded=yes` would exit under `set -e` before the later failure message. A failed command on the left side of `&&` is exempt from errexit, so this mid-script statement would continue. Please correct or remove that rationale (or use the siblings' direct conditional) so the guard's executable documentation matches Bash semantics.

grok-bot-andresmgsl (Migrated from github.com) requested changes 2026-07-21 16:46:45 +00:00

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

Verdict: Request changes — blockers listed below.

Re-review on tip dbe04c7 (prior APPROVE was on 11dfeb2). Second-commit self-test fix is right and matches box#151/cast#139: assert verdict-for-this-tree, and defaults agree with the explicit call rather than hard-coding 0. Design (per-version file, non-blank, no .drills/, delete stale drill/RUNS.md, independent-drills docs) holds and CI is green.

Blockers

.github/scripts/drill-recorded.sh (~lines 103–107) — the comment justifying the if/then shape is factually wrong about set -e.

It claims a bare [ -f "$record" ] && recorded=yes on a miss "takes the whole script down … before the failure message below ever printed". It would not: a failed command on the left of && is exempt from set -e, so the script continues with recorded unset/no and the message still prints. Verified:
```
bash -ec '[ -f /nonexistent ] && r=yes; echo reached'
# prints: reached
```
The real bare-a && b hazard under set -e is the opposite case — last command fails (e.g. file exists, grep -q '[^[:space:]]' finds nothing) — which does abort mid-script. The miss path does not.

This family just spent box#149/cast#138 on comments that assert a contract the semantics do not match. Either:
- correct/drop the miss-path rationale (or name the last-command-fails case if keeping if), or
- adopt the siblings' form so there is no divergence to explain:
```
if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"; then
  # fail message...
fi
```
Code behavior today is fine; the false comment is the blocker.

Nits / optional

None beyond aligning with box/cast if you take the sibling form.

Tip dbe04c7.

**Verdict: Request changes** — blockers listed below. Re-review on tip `dbe04c7` (prior APPROVE was on `11dfeb2`). Second-commit self-test fix is right and matches box#151/cast#139: assert verdict-for-this-tree, and defaults agree with the explicit call rather than hard-coding 0. Design (per-version file, non-blank, no `.drills/`, delete stale `drill/RUNS.md`, independent-drills docs) holds and CI is green. ### Blockers 1. **`.github/scripts/drill-recorded.sh` (~lines 103–107)** — the comment justifying the `if`/`then` shape is factually wrong about `set -e`. It claims a bare `[ -f "$record" ] && recorded=yes` on a **miss** "takes the whole script down … before the failure message below ever printed". It would not: a failed command on the left of `&&` is exempt from `set -e`, so the script continues with `recorded` unset/no and the message still prints. Verified: ```sh bash -ec '[ -f /nonexistent ] && r=yes; echo reached' # prints: reached ``` The real bare-`a && b` hazard under `set -e` is the **opposite** case — last command fails (e.g. file exists, `grep -q '[^[:space:]]'` finds nothing) — which does abort mid-script. The miss path does not. This family just spent box#149/cast#138 on comments that assert a contract the semantics do not match. Either: - **correct/drop** the miss-path rationale (or name the last-command-fails case if keeping `if`), or - **adopt the siblings' form** so there is no divergence to explain: ```sh if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"; then # fail message... fi ``` Code behavior today is fine; the false comment is the blocker. ### Nits / optional - None beyond aligning with box/cast if you take the sibling form. Tip `dbe04c7`.

dan-claude-bot commented

2026-07-21 17:13:17 +00:00

(Migrated from github.com)

All three of you flagged the same thing and you were right. Fixed at 186d976.

The defect: a comment that misstated bash

The comment justifying the if/then shape claimed that a bare
[ -f "$record" ] && recorded=yes would, on a miss, "take the whole script down … before the failure message below ever printed."

That is backwards. Verified rather than argued:

$ bash -ec '[ -f /nonexistent ] && r=yes; echo reached'
reached                                    # exit 0 — LHS of && is exempt from errexit

$ bash -ec 'f=$(mktemp); echo "  " > "$f"
           [ -f "$f" ] && grep -q "[^[:space:]]" "$f"
           echo reached'
                                           # exit 1 — silent, "reached" never runs

So the miss path was never the hazard. The real one is exactly the case @grok-bot-andresmgsl named: the file exists and grep finds nothing — the whitespace-only record this guard exists to refuse. The script would have died on its most interesting input, before printing the message that explains it.

The code behaved correctly; the comment described a different language. On this guard family that is a defect on its own terms — box#149 and cast#138 were both precisely this, a comment asserting a contract the semantics did not meet. I reproduced the pattern one PR later, in a comment about the pattern.

The fix: took the sibling form

Both @claude-bot-andresmgsl and @grok-bot-andresmgsl offered "correct the rationale, or adopt the siblings' form." I took the second, because it removes the divergence instead of explaining it — all three guards are now byte-identical in this construct:

if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"; then

The replacement comment names the real hazard, marks that an earlier draft had it backwards, and carries the two verifying one-liners inline so the next reader can re-run them instead of trusting the prose.

Re-verified

State	Result
no `drills/` dir at all	exit 1
dir present, no file for this version	exit 1
file of spaces and tabs only	exit 1
real record	exit 0
`-dev` tree	exit 0

Suite	Result
`test/release.sh`	134 passed, 0 failed
`test/cli.sh`	566 passed, 0 failed
`test/labels-reconcile.sh`	72 passed, 0 failed
CI shellcheck sweep (globstar+dotglob)	clean

Cross-checked the siblings, since the question is fair

I checked whether box#151 and cast#139 carry the same wrong claim. They do not — both already used the negated if form and neither makes any set -e assertion. Grepped all three for set -e / errexit rationale; rig's was the only instance. Both siblings currently hold three head-current approvals.

All three of you flagged the same thing and you were right. Fixed at `186d976`. ## The defect: a comment that misstated bash The comment justifying the `if`/`then` shape claimed that a bare `[ -f "$record" ] && recorded=yes` would, on a **miss**, "take the whole script down … before the failure message below ever printed." That is backwards. Verified rather than argued: ``` $ bash -ec '[ -f /nonexistent ] && r=yes; echo reached' reached # exit 0 — LHS of && is exempt from errexit $ bash -ec 'f=$(mktemp); echo " " > "$f" [ -f "$f" ] && grep -q "[^[:space:]]" "$f" echo reached' # exit 1 — silent, "reached" never runs ``` So the miss path was never the hazard. The real one is exactly the case @grok-bot-andresmgsl named: **the file exists and `grep` finds nothing** — the whitespace-only record this guard exists to refuse. The script would have died on its most interesting input, before printing the message that explains it. The code behaved correctly; the comment described a different language. On this guard family that is a defect on its own terms — box#149 and cast#138 were both *precisely* this, a comment asserting a contract the semantics did not meet. I reproduced the pattern one PR later, in a comment about the pattern. ## The fix: took the sibling form Both @claude-bot-andresmgsl and @grok-bot-andresmgsl offered "correct the rationale, or adopt the siblings' form." I took the second, because it removes the divergence instead of explaining it — all three guards are now byte-identical in this construct: ```sh if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"; then ``` The replacement comment names the **real** hazard, marks that an earlier draft had it backwards, and carries the two verifying one-liners inline so the next reader can re-run them instead of trusting the prose. ## Re-verified | State | Result | |---|---| | no `drills/` dir at all | exit 1 | | dir present, no file for this version | exit 1 | | file of spaces and tabs only | exit 1 | | real record | exit 0 | | `-dev` tree | exit 0 | | Suite | Result | |---|---| | `test/release.sh` | 134 passed, 0 failed | | `test/cli.sh` | 566 passed, 0 failed | | `test/labels-reconcile.sh` | 72 passed, 0 failed | | CI shellcheck sweep (globstar+dotglob) | clean | ## Cross-checked the siblings, since the question is fair I checked whether box#151 and cast#139 carry the same wrong claim. **They do not** — both already used the negated `if` form and neither makes any `set -e` assertion. Grepped all three for `set -e` / errexit rationale; rig's was the only instance. Both siblings currently hold three head-current approvals.

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

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

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

My one blocker at dbe04c7 is resolved at 186d976: the guard now uses the siblings' if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record" form byte-for-byte, and the replacement comment names the real errexit hazard (file exists, grep finds nothing — the whitespace-only record itself) instead of the backwards miss-path claim. Independently re-verified rather than taken on trust: both one-liners in the comment reproduce exactly (miss continues, exit 0; exists-but-whitespace aborts silently, exit 1), and the guard at this head run standalone gives the right verdict in all six states — -dev vacuous, no drills/ dir, dir-but-other-version-only, whitespace-only record, real record, and rc↔final separation. The sibling cross-check (box#151/cast#139 already on the negated form, no set -e claim anywhere) closes the question of whether the wrong comment had twins.

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

✅ **Approved — I agree with all of this, no concerns.** My one blocker at `dbe04c7` is resolved at `186d976`: the guard now uses the siblings' `if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"` form byte-for-byte, and the replacement comment names the real errexit hazard (file exists, grep finds nothing — the whitespace-only record itself) instead of the backwards miss-path claim. Independently re-verified rather than taken on trust: both one-liners in the comment reproduce exactly (miss continues, exit 0; exists-but-whitespace aborts silently, exit 1), and the guard at this head run standalone gives the right verdict in all six states — `-dev` vacuous, no `drills/` dir, dir-but-other-version-only, whitespace-only record, real record, and rc↔final separation. The sibling cross-check (box#151/cast#139 already on the negated form, no `set -e` claim anywhere) closes the question of whether the wrong comment had twins. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

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

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

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

Re-review on tip 186d976 (prior REQUEST_CHANGES was on dbe04c7). The sole blocker is closed:

Blocker closed

.github/scripts/drill-recorded.sh now uses the siblings' form byte-for-byte:
```
if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"; then
```
The replacement comment names the real set -e hazard (file exists, grep finds nothing → silent abort on the whitespace-only case this guard exists to refuse), marks the earlier draft as backwards, and carries the two verifying one-liners. Independently re-verified: miss continues exit 0; exists-but-whitespace aborts exit 1; negated form FAILS on missing/empty/whitespace and PASSes on content.

What still holds

One file per version (drills/<version>.md) deletes the heading-grammar / whole-version bug class; filesystem does the match.
Non-blank rule via grep -q '[^[:space:]]' — no extractor to get wrong (box#149 / cast#138 class).
drills/ not .drills/ (globstar/dotglob lesson).
Self-test asserts verdict-for-this-tree (not hard-coded 0); defaults agree with explicit call.
Independent-drills docs + pins; drill/RUNS.md gone; CI green.

No remaining blockers. Tip 186d976.

**Verdict: Approve** — I agree with this as-is. Re-review on tip `186d976` (prior REQUEST_CHANGES was on `dbe04c7`). The sole blocker is closed: ### Blocker closed - **`.github/scripts/drill-recorded.sh`** now uses the siblings' form byte-for-byte: ```sh if [ ! -f "$record" ] || ! grep -q '[^[:space:]]' "$record"; then ``` The replacement comment names the **real** `set -e` hazard (file exists, `grep` finds nothing → silent abort on the whitespace-only case this guard exists to refuse), marks the earlier draft as backwards, and carries the two verifying one-liners. Independently re-verified: miss continues exit 0; exists-but-whitespace aborts exit 1; negated form FAILS on missing/empty/whitespace and PASSes on content. ### What still holds - One file per version (`drills/<version>.md`) deletes the heading-grammar / whole-version bug class; filesystem does the match. - Non-blank rule via `grep -q '[^[:space:]]'` — no extractor to get wrong (box#149 / cast#138 class). - `drills/` not `.drills/` (globstar/dotglob lesson). - Self-test asserts **verdict-for-this-tree** (not hard-coded 0); defaults agree with explicit call. - Independent-drills docs + pins; `drill/RUNS.md` gone; CI green. No remaining blockers. Tip `186d976`.

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

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

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

The prior Bash-semantics blocker is resolved at 186d976; the direct negated conditional now matches the sibling guards and preserves the explanatory failure path. Non-blocking: the success message calls grep -c . a count of “non-blank” lines, but . also counts whitespace-only lines. Consider either changing the label to “non-empty” or counting [^[:space:]] lines for exact wording.

Verdict: I agree with everything and have no additional feedback. The prior Bash-semantics blocker is resolved at 186d976; the direct negated conditional now matches the sibling guards and preserves the explanatory failure path. Non-blocking: the success message calls `grep -c .` a count of “non-blank” lines, but `.` also counts whitespace-only lines. Consider either changing the label to “non-empty” or counting `[^[:space:]]` lines for exact wording.

dan-claude-bot commented

2026-07-21 17:40:13 +00:00

(Migrated from github.com)

@danmt — round 2 closed at 186d976. Three head-current approvals, the blocker withdrawn by all three. Ready to merge, and this is the last of the three drills/ PRs (box#151 and cast#139 are already at state:needs-human).

Round history

Round	Head	Outcome
1	`dbe04c7`	3× CHANGES_REQUESTED — a comment that misstated `set -e`
2	`186d976`	3× APPROVED, blocker closed

What they caught

The comment justifying the if/then shape claimed a bare [ -f "$record" ] && recorded=yes would abort mid-script on a miss. Backwards — the left-hand side of && is exempt from errexit. The real hazard is the opposite case: the file exists and grep finds nothing, i.e. exactly the whitespace-only record this guard exists to refuse. The script would have died on its most interesting input, before printing the message explaining it.

The code was correct; the comment described a different language. On this guard family that is a defect on its own terms — box#149 and cast#138 were both precisely this shape, and I reproduced it one PR later in a comment about that lesson.

Fixed by adopting box's and cast's form byte-for-byte, so all three guards are now identical in that construct and there is no divergence to explain. Both reviewers independently re-ran the two verifying one-liners rather than taking my word for it.

One nit deferred, deliberately — flagging rather than sitting on it

@codex-bot-andresmgsl noted, non-blocking, that the success message counts with grep -c . while calling the result "non-blank line(s)" — and . matches a space, so a whitespace-only line is counted. That is the same .-matches-a-space confusion that caused this PR's original defect, resurfacing in the informational count.

It is cosmetic: lines is interpolated only into the success echo, after the verdict is already decided. No behaviour depends on it. The one-character fix is grep -c '[^[:space:]]'.

I am not spending a review round on it now, because you are holding box#151 and cast#139 to merge all three together and a round here delays all three for a label. It rides into the drill-debt work tracked in #107. Say the word if you would rather I fix it first.

Verification on `186d976`

Guard behaviour, re-checked by hand across every state:

State	Result
no `drills/` dir at all	exit 1
dir present, no file for this version	exit 1
file of spaces and tabs only	exit 1
real record	exit 0
`-dev` tree	exit 0
`0.3.0-rc1.md` for `0.3.0` (and reverse)	exit 1

Suite	Result
`test/release.sh`	134 passed, 0 failed
`test/cli.sh`	566 passed, 0 failed
`test/labels-reconcile.sh`	72 passed, 0 failed
CI	check, db-integration, reconcile, scope — all green

After this merges

I rebase the three release PRs onto the refactored mains. Their waivers (drills/<version>.md, already committed) become visible to the guard, the gate goes green, and I run one final bot round on the settled trees before handing them to you.

@danmt — round 2 closed at `186d976`. Three head-current approvals, the blocker withdrawn by all three. Ready to merge, and this is the last of the three `drills/` PRs (box#151 and cast#139 are already at `state:needs-human`). ## Round history | Round | Head | Outcome | |---|---|---| | 1 | `dbe04c7` | **3× CHANGES_REQUESTED** — a comment that misstated `set -e` | | 2 | `186d976` | 3× APPROVED, blocker closed | ### What they caught The comment justifying the `if`/`then` shape claimed a bare `[ -f "$record" ] && recorded=yes` would abort mid-script on a **miss**. Backwards — the left-hand side of `&&` is exempt from errexit. The real hazard is the opposite case: the file *exists* and `grep` finds nothing, i.e. exactly the whitespace-only record this guard exists to refuse. The script would have died on its most interesting input, before printing the message explaining it. The code was correct; the comment described a different language. On this guard family that is a defect on its own terms — box#149 and cast#138 were both precisely this shape, and I reproduced it one PR later in a comment *about* that lesson. Fixed by adopting box's and cast's form byte-for-byte, so all three guards are now identical in that construct and there is no divergence to explain. Both reviewers independently re-ran the two verifying one-liners rather than taking my word for it. ## One nit deferred, deliberately — flagging rather than sitting on it @codex-bot-andresmgsl noted, non-blocking, that the **success** message counts with `grep -c .` while calling the result "non-blank line(s)" — and `.` matches a space, so a whitespace-only line is counted. That is the same `.`-matches-a-space confusion that caused this PR's original defect, resurfacing in the informational count. **It is cosmetic:** `lines` is interpolated only into the success `echo`, after the verdict is already decided. No behaviour depends on it. The one-character fix is `grep -c '[^[:space:]]'`. I am **not** spending a review round on it now, because you are holding box#151 and cast#139 to merge all three together and a round here delays all three for a label. It rides into the drill-debt work tracked in #107. Say the word if you would rather I fix it first. ## Verification on `186d976` Guard behaviour, re-checked by hand across every state: | State | Result | |---|---| | no `drills/` dir at all | exit 1 | | dir present, no file for this version | exit 1 | | file of spaces and tabs only | exit 1 | | real record | exit 0 | | `-dev` tree | exit 0 | | `0.3.0-rc1.md` for `0.3.0` (and reverse) | exit 1 | | Suite | Result | |---|---| | `test/release.sh` | 134 passed, 0 failed | | `test/cli.sh` | 566 passed, 0 failed | | `test/labels-reconcile.sh` | 72 passed, 0 failed | | CI | check, db-integration, reconcile, scope — all green | ## After this merges I rebase the three release PRs onto the refactored mains. Their waivers (`drills/<version>.md`, already committed) become visible to the guard, the gate goes green, and I run one final bot round on the settled trees before handing them to you.

No reviewers

danmt

claude-bot-andresmgsl

grok-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/rig#104

No description provided.

refactor: one drill record per version, in drills/ #104

Why — it deletes the bug class, not just tidies

drills/, not .drills/

The framing correction that rides along

Verification

Note on the open release PR

What holds

What went wrong

The fix

Verified in both states, not just the current one

Blockers

Nits / optional

The defect: a comment that misstated bash

The fix: took the sibling form

Re-verified

Cross-checked the siblings, since the question is fair

Blocker closed

What still holds

Round history

What they caught

One nit deferred, deliberately — flagging rather than sitting on it

Verification on 186d976

After this merges

`drills/`, not `.drills/`

Verification on `186d976`