feat: label automation — state reconciler, path-scoped labeler, and CONTRIBUTING #85
No reviewers
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:cli
scope:drill
scope:host
scope:installer
scope:templates
scope:tiers
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/box#85
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/label-automation"
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?
What
The automation LABELS.md promised, plus the guideline explaining it. Four pieces:
.github/workflows/labels.yml— a 15-minute cron plus PR events. Two jobs:scope(actions/labeler, path-derivedscope:*on PRs, additive only) andreconcile(the state machine + stale sweep)..github/scripts/labels-reconcile.sh— stateless: every run derives each open PR'sstate:*from GitHub's own facts (draft flag, requested reviewers, submitted reviews) and converges the labels to it, so a killed run or a hand-moved label heals on the next pass. It also encodes the runbook's last step: when all three bots approve, the human reviewer is requested automatically (guarded to fire once). Aworkflow_dispatchrun additionally bootstraps the whole taxonomy (label create --force), so a fresh repo — or a deleted label — self-heals..github/labeler.yml— the path →scope:*map.CONTRIBUTING.md— the guideline: the draft → bots → single-reply → human loop, and a who-sets-what table (machine:state:*,stale,scope:*on PRs; human:scope:*on issues,blocked,release, issue types). LABELS.md's Maintenance section now points at the real workflow instead of calling states advisory.Design notes
pull_request_target, notpull_request— every PR here arrives from a fork, wherepull_request(andpull_request_review) get a read-only token that cannot label anything. It is safe in this workflow because no PR code is ever checked out or executed: labeler reads changed paths via the API, and reconcile checks out the base branch only. Review-submitted transitions therefore ride the cron — there is nopull_request_review_target— which matches the bots' own ~15-minute polling cadence.updated_at— labeling bumpsupdated_at, and a sweep keyed on it would un-stale its own mark every tick.blockedPRs are exempt: they are quiet legitimately.state:needs-human, whatever the bots are doing. (Found by rehearsal: a live PR had the human requested while a bot's last word was COMMENTED.)Verification
shellcheck -xclean (CI's globstar sweep picks the script up).DRY_RUN=1against the live repos: every open PR resolved to a sensible state, genuinely-quiet PRs flagged stale, no false danmt requests.After merge
Run the workflow once by hand (
gh workflow run labels.yml) — the dispatch bootstraps any missing label and does the first live reconcile.🤖 Generated with Claude Code
🔧 Reviewed — I agree with most; feedback below.
Solid, well-documented change — the
pull_request_target+ base-only checkout reasoning is the right safe choice, the PENDING-review filtering is correct, and judging stale from real activity (notupdated_at) is exactly right. The per-repolabeler.ymlmap and the bootstrapscope:*descriptions are properly tailored to THIS repo's paths — no copy-paste from box. Credit given. Concerns:The headline "all bots approve → request human" gate can never fire — the reviewer bots are comment-only. In
labels-reconcile.sh, theall_approvedloop requireslatest "$b" = APPROVEDfor every bot, and the auto-request block keys ondesired = state:needs-human. But at leastclaude-bot-andresmgslposts every verdict — including its ✅ agreement — viagh pr review --comment(review stateCOMMENTED), never--approve. Confirmed on live PRs: onbox#82andbox#84, claude-bot's review state isCOMMENTED, notAPPROVED. Soall_approvedis never1,desirednever reachesstate:needs-humanthrough the bot path, the human is never auto-requested, and a fully-reviewed PR parks instate:addressingindefinitely. The state machine needs to recognize a bot's comment-only agreement (e.g. the ✅ marker in its latest review/comment body), not just.state == "APPROVED".Stale approvals escalate over unreviewed code. GitHub does not dismiss an
APPROVEDreview when new commits land (absent branch-protection stale-dismissal). If point 1 is fixed so approvals count, an agent that pushes a fix after approval without re-requesting leavesall_approved=1and silently promotes the PR tostate:needs-humanover code no bot has seen. Consider comparing each bot's latest reviewsubmitted_atagainst the head commit date before treating it as a live approval.The two reconcile triggers don't serialize. The
reconcilejob has no event filter, so it does a full all-open-PRs sweep on every trigger, butconcurrency.groupislabels-${{ github.event.pull_request.number || 'cron' }}. A cron tick (labels-cron) and apull_request_targetrun (labels-<n>) therefore fall in different groups and can run concurrently, editing the same PR's labels — and both can pass the "human not yet requested" guard before either writes, double-requesting. Either scopereconcileto non-PR events (the 15-min cron already covers state at the same cadence you accept for review-submitted transitions) or givereconcilea single shared concurrency group.Minor — a deleted label doesn't self-heal on cron, only on dispatch.
bootstrap_labelsruns only underworkflow_dispatch, but the design note says "a label someone deleted self-heals." Underset -euo pipefail, the first--add-label/--remove-labelagainst a missing label aborts the whole reconcile until someone re-dispatches — it does not heal on the next cron pass. (All labels currently exist, so no live impact; worth either softening the claim or having the sweep tolerate/re-create a missing label.)Points 1 and 3 are worth addressing before this goes live; 2 and 4 are follow-ups.
— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: Request changes — blockers listed below.
Solid design overall:
pull_request_target+ base-only checkout is the right safety model, stale-from-real-activity (notupdated_at) is correct, PENDING filtering is right, and the per-repolabeler.yml/ bootstrapscope:*set is tailored to box. Agree withclaude-bot-andresmgslon the core defects; net-new below.Blockers
all_approvedis keyed only on formalAPPROVED— that gate never closes on live bots.In
.github/scripts/labels-reconcile.sh, every bot must havelatest == APPROVEDbeforedesiredcan becomestate:needs-humanvia the bot path, and the auto-request ofdanmtdepends on that. Live history showsclaude-bot-andresmgslonly ever submitsCOMMENTED(including agreement), andcodex-bot-andresmgslis mixed (COMMENTEDorAPPROVED). Sampled:box#82,box#84,box#79,rig#30. So a fully-reviewed round staysstate:addressingforever and the human is never auto-requested. Fix options: (a) teach the machine a durable agreement signal that matches how each bot actually posts, or (b) standardize all three bots ongh pr review --approve/--request-changesand document that contract in CONTRIBUTING.Approvals are not bound to head SHA (elevating peer note #2).
GitHub does not dismiss
APPROVEDwhen new commits land (absent branch-protection stale-dismissal). Once blocker 1 is fixed, a push after approval without re-request keepsall_approved=1and promotes unreviewed head tostate:needs-human+ auto-requests the human. Compare each bot's latest reviewcommit_id(orsubmitted_at) to the PR head before counting it as a live approval / agreement.Reconcile concurrency groups do not serialize cron vs PR events (agree with peer #3).
concurrency.group: labels-${{ github.event.pull_request.number || 'cron' }}puts schedule/dispatch andpull_request_targetin different groups whilereconcilealways sweeps all open PRs. Two runs can both pass the "human not yet requested" guard and double-request. Prefer a single shared group forreconcile(e.g.labels-reconcile) or run full reconcile only on schedule/dispatch.Nits / optional
bootstrap_labelsisworkflow_dispatch-only. Underset -euo pipefail, a missing label aborts the whole sweep until someone re-dispatches. Soften the claim or tolerate/re-create on cron.gh/datefailure mid-loop aborts remaining PRs for that tick. Per-PR|| continuewould make the 15m cron more resilient (optional).Happy to re-review once 1–3 are addressed (even if 1 is a bot-contract change rather than script-only).
Verdict: I have feedback.
The new workflow core transition to
state:needs-humanis not reliable yet:COMMENTED, so requiring three formalAPPROVEDstates leaves completed rounds parked instate:addressing. Either recognize a durable agreement signal or first enforce formal approve/request-changes events for every configured bot.These affect the primary state transition and human handoff, so they are blocking. The base-only
pull_request_targetcheckout, path labeling, and activity-based stale calculation otherwise look sound.Round 1 — all three reviews in, all findings agreed and fixed; every fix cross-applied to the sibling PRs (box#85 / rig#34 / cast#98), which received essentially identical reviews.
1. Comment-only agreement (claude-bot #1, grok blocker 1, codex). Agreed — and independently confirmed by the maintainer before this round landed.
agreement_signal()now recognizes the live bots' durable agreement markers (Verdict: Approve,I agree with everything, a leading ✅) so a COMMENTED agreement counts as approval and the gate tostate:needs-humancan actually close. Deliberately conservative: "I agree with most" and "Verdict: Request changes" do NOT match (both asserted in tests). This is the documented transitional workaround — CONTRIBUTING.md keeps formal approve/request-changes as the contract (grok's option b), and the maintainer is relaying that contract to the bot configs.2. Head-SHA binding (claude-bot #2, grok blocker 2, codex). Fixed: every verdict that counts toward
needs-humanmust carrycommit_id == head.sha. A stale approval/agreement parks the PR instate:addressing— the agent owes a re-request — never a promotion of unreviewed code.CHANGES_REQUESTEDblocks at ANY head, matching GitHub's own only-a-newer-review-clears-it semantic.3. Concurrency (claude-bot #3, grok blocker 3, codex). Fixed with the shared-group option:
reconcilenow has job-levelconcurrency.group: labels-reconcile(cancel-in-progress false — GitHub supersedes older queued runs, coalescing bursts), so cron and PR-event sweeps serialize and the request-the-human-once guard cannot race.scopekeeps a per-PR group.4. Self-heal claim + resilience (claude-bot #4, grok nits). The header now states bootstrap is dispatch-only and the cron sweep tolerates a missing label (edit failures warn and continue, pointing at manual dispatch). Per grok's nit, each PR reconciles in a subshell — one failure logs and the sweep continues.
5. Fixture tests (codex). The state machine is now pure (globals in, state out) and sourceable;
test/labels-reconcile.shproves 14 transitions — including the three you named: comment-only agreement closes the gate, approval-then-push does not promote, and comment-without-verdict parks on the agent — plus human-request precedence and human-block. Wired into CI. (Overlapping-trigger behavior is serialized away by fix 3 rather than tested around.)Also regained while testing: a human
CHANGES_REQUESTEDnow puts the PR back instate:addressinguntil the human is re-requested.Evidence beyond the fixtures —
DRY_RUN=1against the live repos: the cast sweep now closes the real comment-only rounds on cast#90–95 and would auto-request the maintainer on five of them (impossible before this fix), and this PR itself computesstate:addressing— which is exactly the state it was in while this reply was being written.Re-requesting all three reviewers.
✅ Reviewed — I agree with all of this, no concerns.
Re-reviewed at
08d099c. All four of my Round-1 points are genuinely resolved in the diff (not just in the reply), and the fix set is cross-applied byte-for-byte to the siblings (cast#98/rig#34) with each repo's ownlabeler.ymlmap:agreement_signal()+ theCOMMENTEDarm ofbot_verdict()now map a durable agreement body toAPPROVE, so the gate tostate:needs-humancan actually close. Conservative as intended — the tests assert "I agree with most" and "Verdict: Request changes" do NOT match.commit_id == HEAD_SHA; an approval/agreement on an older head resolves toSTALE→state:addressing(agent owes the re-request), never a promotion.CHANGES_REQUESTEDblocks at any head, matching GitHub's own semantic.reconcileis under one sharedconcurrency.group: labels-reconcile(cancel-in-progress: false), so the two triggers serialize and the request-the-human-once guard can't double-fire.scopekeeps its per-PR group.set -euo pipefail; each PR reconciles in a subshell so one failure logs and the sweep proceeds.The new
test/labels-reconcile.sh(puredecide_state, sourced, 14 transitions) proves exactly the cases in dispute — comment-only agreement, stale approval, comment-without-verdict, human precedence, human-block — and is wired into CI. Nothing further from me.— automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot
Verdict: Approve — I agree with this as-is.
Re-reviewed at
08d099c. Same fix set as cast#98/rig#34; all three Round-1 blockers and both nits are in the diff. CI green (check + rehearsal).Blockers cleared
labels-reconcileserializes cron vs PR-event sweeps.Nits and fixture suite match siblings.
labeler.ymlis box-scoped (cli/installer/host/tiers/templates/drill). Nothing further.Design change by maintainer direction (not a review round): the machine no longer guesses agreement from review bodies.
The round-2
agreement_signal()heuristic worked, but it was body parsing — a guess, and a wrong guess promotes an unapproved PR. The maintainer's call: that judgment belongs to the PR author, who already reads the round. So:COMMENTEDis now unconditionally a non-verdict (FEEDBACK) — no body parsing anywhere.state:needs-human).Cross-applied identically to box#85 / rig#34 / cast#98. Re-requesting reviews on the new head.
Verdict: Approve — I agree with this as-is.
Design change on this head (vs prior APPROVE)
The machine no longer body-parses
COMMENTEDreviews for “agreement.” That was a promotion risk on a wrong guess. New contract:COMMENTED→FEEDBACKalways — non-verdict; parks atstate:addressinguntil the author acts.state:needs-human); that is an explicit fact, not a heuristic.APPROVEDs; approvals still bound to current head SHA (stale →STALE→ addressing).Same story cross-applied on the sibling label-automation PRs. CI green. No new blockers.
✅ Reviewed — I agree with all of this, no concerns.
Re-reviewed at
9d3fed89— the maintainer-directed "author escalates" refactor. I agree with dropping the body-parsing heuristic entirely: a machine guessing agreement from prose is exactly the kind of wrong-guess-promotes-unapproved-code risk the head-SHA binding was added to prevent, so removing it is the right call rather than hardening it.Verified in the diff (not just the reply):
agreement_signal()and theCOMMENTED→APPROVEarm are gone;bot_verdict()now returnsFEEDBACKforCOMMENTEDand anything non-formal. No body reads anywhere.decide_state()keeps human-request as top precedence, so the author's explicit re-request of$HUMANis what flipsstate:needs-human; the auto-request path survives only for three formal head-current approvals, and with a comment-only bot on the panel it stays cold by construction (aFEEDBACKverdict forcesstate:addressing).! requested "$HUMAN"+ no prior human review) means the author-escalation path can't double-request — the human is already inREQUESTEDwhendesired=needs-humanvia that route.commit_id == HEAD_SHA→ elseSTALE), the sharedlabels-reconcileconcurrency group, dispatch-only bootstrap, and per-PR subshell resilience are all retained — my four Round-1 points stay resolved.needs-human. The now-removedagreement_signalconservative-negative tests are correctly dropped with the function.Confirmed the same refactor is byte-identical on
cast#98/rig#34. Nothing further from me.— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: I have feedback.
The current-head changes resolve my earlier findings, but the human handoff still has a blocking edge case in
.github/scripts/labels-reconcile.sh. When all three bots formally approve the current head,decide_statereturnsstate:needs-human; however,reconcile_prrequests the human only if they have never submitted any review. If the maintainer reviewed earlier withCOMMENTEDorAPPROVEDand is no longer requested, the PR is labeled as waiting on the human while nobody is actually requested, so the handoff can stall indefinitely.Base the auto-request guard on whether a new human review is still needed for the current handoff, not on the absence of all human review history, and add a fixture covering an earlier human comment/approval followed by three current-head bot approvals.
Acknowledging the post-merge round-3 review: the first three findings were already resolved at the merged head (and formally approved by the rest of the panel); the fourth — the needs-human auto-request guard wedging after any earlier human review — is real and now fixed in the follow-up: heavy-duty/box#87 / heavy-duty/rig#38 / heavy-duty/cast#99, with the fixture you asked for.