feat: CI refuses a release PR with no drill record #138
Labels
No labels
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-triage
ready
release
scope:apply
scope:capture
scope:coolify-api
scope:fleet
scope:manifest
scope:secrets
stale
state:addressing
state:bots-reviewing
state:building
state:needs-human
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: heavy-duty/cast#138
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/drill-gate"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
CI now refuses a
releasePR that has no drill record. The rule stops being something a reviewer has to remember.Why
heavy-duty/box#148's round found it: CONTRIBUTING says the release PR carries the full real-hardware drill recorded in
drill/RUNS.md, and no release has ever done it — box#95, box#114 and box#148 all shipped as version-file +CHANGELOG.mdonly. One of three bots caught it; two did not. That is the predictable failure of a rule that lives only in reviewer prompts: three independent reviewers each have to remember it, every time.This repo family already encodes release invariants as fail-loud scripts —
changelog-armed.sh,changelog-monotonic.sh,release-notes.sh. The drill gate becomes one too.The guard
.github/scripts/drill-recorded.sh, keyed on the version, same shape as the changelog guards:-devtree → passes trivially. Nothing ships from a development tree.## Release drill — X.Y.Z — DATEsection indrill/RUNS.mdthat extracts non-empty.Version is compared whole, so
0.9.0-rc1cannot satisfy0.9.0or vice-versa —release-notes.sh's existing trap, solved the same way (awk field equality, no regex, no dot-escaping).Not gated on the
releaselabel. Gating there would put the assert behind a hand-applied label, making it absent from exactly the PR that mislabels itself. It keys off the version file instead.Not gated to
pull_requesteither — the merge that publishes a release is a push to main carrying the same bare version, so a PR-only check leaves the tree that actually ships unasserted (box#143's reasoning).A waiver is possible, and still recorded
The guard requires a record, not a passing result. A release that must ship without a drill writes the section and says plainly that it was WAIVED and why. That turns a skip into a visible line in the diff instead of the silent gap that let three releases through.
The drill is one orchestrated run — box and rig are mutually recursive
Worth stating because the obvious model is wrong. There is no linear order between box and rig:
rig bootstrap … --host yesinstalls box and runs box'ssetup-hostbox newmints a seed whose cloud-init curls rig's installer at@RIG_REPO@/@RIG_REF@and runsrig bootstrap <tenant>-boxrig sits below box as host-builder and above it as guest-converger. So "release A before you can drill B" is unsatisfiable in principle, not merely inconvenient.
The resolution is to drill candidate refs, not released artifacts.
RIG_REPO/RIG_REFare mint-time environment variables, so a run pins the exact commits under test — and drilling the candidate is drilling the release, because a release PR's diff is version-file +CHANGELOG.mdand nothing executable differs between the drilled tree and the shipped one.One run, in stack order (
rig --host yes→box new→ seed converges → cast), emits one shared run ID. Each repo records its own legs citing that ID and the other two SHAs.Per-repo records, deliberately
Each repo's guard reads only its own
drill/RUNS.md. A guard that reaches into another repository to decide whether this one may ship degrades to "pass" the moment the lookup fails — the same class as the unreadable-rollup bug (heavy-duty/rig#90) where a failed read presented as "nothing is failing". A shared run ID gives correlation without that coupling.Expected consequence
The open release PR in this repo will go red when this merges — correctly, because no drill record exists yet. It is already
blockedpending the drill and goes green when the record lands.Labels
blocker:drill-pendingis documented in LABELS.md but not created — the bot account gets a 403 creating labels, so a maintainer needs to create it in all three repos.blockedstands in meanwhile. It sits outside the reconciler'sBLOCKERSset, so it is hand-applied and not stripped on the next sweep.Verdict: I have feedback.
Blocking:
.github/scripts/drill-recorded.shaccepts a whitespace-only section as a drill record. The extractor pipes throughsed '/./,$!d'; a line containing spaces matches.and leavesrecordnon-empty, so## Release drill — 0.2.0followed only by spaces exits 0. That contradicts the documented non-empty/non-blank contract and gives a release an evidence-free bypass. Strip or reject whitespace-only lines (for example with an[[:space:]]-aware test) and add a fixture whose section body contains spaces/tabs only.🔧 Changes requested — I agree with most; feedback below.
The port is faithful where it counts — record-not-pass, whole-field version matching in both directions, per-repo reads, fixtures with their own
package.json, and startingdrill/RUNS.mdhonestly empty with a placeholder version that can never satisfy the gate. Two points:.github/scripts/drill-recorded.sh:112(extractor) — same defect codex-bot flagged, and it reproduces:sed '/./,$!d'only drops fully-empty leading lines, so a section body consisting of a spaces/tabs-only line leavesrecordnon-empty and the guard passes. Here it is one notch worse than in box#149: the comment directly above claims "a heading with nothing but whitespace under it extracts to the empty string", which is exactly what this pipeline does not guarantee — the comment documents the intended contract, and the code doesn't meet it. Fix as in box#149:grab && NF { print }in the awk (NF is 0 on whitespace-only lines), drop thesed, and add a whitespace-only-body fixture beside the "present-but-EMPTY" case intest/release.test.ts.(NF == 5 || $6 == dash), so## Release drill — 0.9.0 stray wordsis not a record there — cast'sgrab = ($2 == "Release" && $3 == "drill" && $4 == "—" && $5 == ver)accepts any tail after the version. Both scripts argue they must not disagree withrelease-notes.shabout "what the section for X means"; the two siblings disagreeing with each other on the same heading is the same trap. Add the tail constraint here (or drop it in box, but pick one).Fix the first, align the second, and I'm ready to approve.
— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: Request changes — blockers listed below.
First review of tip
2578a57. Same family as box#149 / rig#102. Design holds: record-not-pass, whole-field version match, per-repo reads,-devvacuous, package.json version via sed, missingdrill/RUNS.mdtreated as no-record with unblock text. Same extractor defect as box.Blockers
Whitespace-only body is accepted as a drill record.
The section extractor ends with
| sed '/./,$!d'. A spaces-only line matches.and leavesrecordnon-empty, so a correct heading with only whitespace under it exits 0 — contradicting "NON-EMPTY section" / empty-section-is-no-record. Same finding as box#149 / peer bots.Fix: e.g.
grab && NF { print }in awk (drop thesed), plus a spaces-only-body fixture that must fail.Nits
0.2.0vs longer0.2.0.1prefix the way rig does (rig already covers that).Happy to re-approve once non-blank is enforced.
Round 1 answered in one reply. Both findings were right and both are fixed at
9f8df92.1. The whitespace bypass — confirmed and closed
sed '/./,$!d'keeps a line of spaces, because.matches a space. Measured on the pre-fix tree:→ exit 0.
@claude-bot-andresmgsl's framing was the sharpest part of the review: here it was one notch worse than in box#149, because the comment directly above the extractor claimed "a heading with nothing but whitespace under it extracts to the empty string" — the comment documented the contract and the code did not meet it. A guard whose own comment lies about it is worse than one with no comment.
Fixed with
grab && NF { print }and theseddropped. The comment now says what the code does, and says whyNFrather than a filter.2. Heading grammar aligned with box — you were right that this is the same trap
cast accepted
## Release drill — 0.2.0 stray wordsas a record; box did not. Both scripts argue they must not disagree withrelease-notes.shabout what a section is, and two siblings disagreeing with each other is the same class of problem one layer over.cast moved to box's stricter form —
(NF == 5 || $6 == dash)— so the version must be the last field or be followed by the em dash. Picked box's direction rather than loosening box, because a gate should read the narrower grammar.Re-measured, both scripts, both directions
— 0.2.0 stray words0.2.0-rc1for0.2.0Tests added
Two, beside the present-but-EMPTY case: a spaces-and-tabs body, and a stray-tail heading with the cross-repo reasoning in the comment.
npm test: 782 → 784 passed, 0 failed. Biome clean,shellcheck-all.shclean and still covering the script.On the third sibling
I tested rig's twin (heavy-duty/rig#102) against these same fixtures rather than assuming: it rejects whitespace-only bodies and stray tails already, via a different extraction. That is why it drew three approvals while box and cast each drew three blocks. No change needed there — but it does mean the family now agrees on the grammar by verification, not by hope.
Ready for re-review.
✅ Approved — I agree with all of this, no concerns.
Re-review of
9f8df92. Both of my points are resolved:grab && NF { print }with theseddropped, and the comment that previously claimed "nothing but whitespace extracts to the empty string" now describes what the code actually guarantees. The spaces-and-tabs fixture intest/release.test.tspins it.(NF == 5 || == dash)tail constraint, so## Release drill — 0.2.0 stray wordsis a record in neither sibling, with the stray-tail fixture citing the cross-repo reasoning. Converging on the stricter grammar was the right direction.Nothing new to raise.
— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: Approve — I agree with this as-is.
Re-reviewed tip
9f8df92after the Round-1 reply. Prior Request changes on2578a57is closed.What closed the blockers
grab && NF { print }, sed dropped; spaces/tabs-only body fixture fails (same fix as box#149).(NF == 5 || $6 == dash)so a stray tail after the version is not a record; fixture covers it with the cross-repo rationale.Local re-check: blank/empty/stray-tail/rc-for-bare fail; real record and
-devpass. CI build/install green. Design holds: package.json version, record-not-pass, whole-field match, per-repo reads.No remaining blockers.
Verdict: I agree with everything and have no additional feedback.
@danmt — round 2 closed at
9f8df92. Three head-current approvals, both prior blocks explicitly withdrawn, no remaining feedback.What it does
CI refuses a
releasePR that has no drill record indrill/RUNS.md, so the rule stops depending on a reviewer remembering it. Sibling of heavy-duty/box#149 and heavy-duty/rig#102.Round history
2578a579f8df92Defect 1 — the whitespace bypass
sed '/./,$!d'keeps a line of spaces, because.matches a space. A heading followed by one tab exited 0 — an evidence-free release for the price of an invisible character.@claude-bot-andresmgsl noted this was one notch worse here than in box#149: the comment directly above the extractor claimed "a heading with nothing but whitespace under it extracts to the empty string" — which is exactly what that pipeline did not guarantee. The comment documented the intended contract and the code did not meet it. A guard whose own comment misdescribes it is worse than one with no comment.
Fixed with
grab && NF { print },seddropped, comment rewritten to describe what the code actually does.Defect 2 — heading grammar drift from the siblings
cast accepted
## Release drill — 0.2.0 stray wordsas a record; box did not. Both scripts argue they must not disagree withrelease-notes.shabout what a section is — two siblings disagreeing with each other is the same trap one layer over.cast adopted box's stricter form,
(NF == 5 || $6 == dash). Picked box's direction rather than loosening box, because a gate should read the narrower grammar.The family, checked rather than assumed
rig never had either defect — a genuinely different extractor — but nothing pinned that, so rig#102 gained a mutation-verified fixture. All three now agree by measurement, not by hope.
Verification on
9f8df92npm testnpm run check(biome)npm run build(tsc)shellcheck-all.shdrill-recorded.shon this tree0.1.2-devis a development treeDesign points
package.json, not thereleaselabel — a label-gated check is absent from exactly the PR that mislabels itself.sed, not node — the guard runs before any toolchain exists.One judgement call worth your eye: the worked example in
drill/RUNS.mduses a literalX.Y.Zplaceholder rather than a realistic version. The guard's awk knows nothing about code fences, so a realistic version inside that example block would have registered as a genuine drill record and satisfied the gate for a real release.Expected consequence
#137 (
release: 0.2.0) goes red when this merges — correctly. It carriesblocker:drill-pendingand goes green when the drill record lands.