feat: label automation — state reconciler, path-scoped labeler, and CONTRIBUTING #34
No reviewers
Labels
No labels
attention
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-ruling
needs-triage
offsite
post-merge
ready
release
scope:bootstrap
scope:coolify
scope:db
scope:docs
scope:drill
scope:installer
scope:labels
scope:platform
scope:runner
scope:users
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/rig#34
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 rig. 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 current head has three blocking correctness issues:
In
.github/scripts/labels-reconcile.sh,all_approvedrequires every bot's latest review state to beAPPROVED. At least one configured bot reports agreement using aCOMMENTEDreview, so a successful bot round can never transition tostate:needs-humanor requestdanmt. Define and consume a verdict contract that matches the bots' actual review output (or require all bots to submit formal approvals).Bind each accepted bot verdict to the current head SHA. GitHub may retain an old approval after a new commit, and the documented flow re-requests only bots that did not approve; without checking the review's
commit_id(or an equivalent post-head timestamp), unreviewed code can be promoted to human review.In
.github/workflows/labels.yml, cron/dispatch runs uselabels-cronwhile PR events uselabels-<number>, but everyreconcilejob sweeps every open PR. Those runs can mutate the same PR concurrently and both pass the one-time human-request guard. Serialize reconcile jobs under one shared concurrency group, or restrict full-repository reconciliation to schedule/dispatch.The two current checks pass, but they do not exercise these workflow-state transitions. Please add focused fixture-driven tests for comment-only agreement, approval followed by a new head commit, and overlapping trigger behavior.
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
27fb934. All four of my Round-1 points are genuinely resolved in the diff, and this repo'slabeler.ymlis tailored to rig's own paths (scope:bootstrap/users/runner/coolify/db/installer) — no box/cast copy-paste.agreement_signal()+ theCOMMENTEDarm ofbot_verdict()map a durable agreement body toAPPROVE, so the gate tostate:needs-humancan close. Conservative — "I agree with most" and "Verdict: Request changes" are asserted NOT to match.commit_id == HEAD_SHA; an older-head approval resolvesSTALE→state:addressing, never a promotion.CHANGES_REQUESTEDblocks at any head.reconcileunder one sharedconcurrency.group: labels-reconcile(cancel-in-progress: false); the two triggers serialize and the human-request-once guard can't double-fire.scopekeeps its per-PR group.The new
test/labels-reconcile.shproves the disputed transitions (including codex's request for approval-then-new-head and comment-only fixtures) and is wired into CI. Nothing further from me.— automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot
Verdict: I agree with everything and have no additional feedback.
Re-reviewed at
27fb934. The current head resolves the earlier verdict-contract, head-SHA binding, and reconcile-concurrency concerns, and adds focused state-machine fixtures. Both current checks pass.Verdict: Approve — I agree with this as-is.
Re-reviewed at
27fb934. Same fix set as cast#98/box#85; all three Round-1 blockers and both nits are in the diff. CI green (check + db-integration).Blockers cleared
agreement_signal()+ COMMENTED→APPROVE mapping; conservative negatives covered by fixtures.commit_id == HEAD_SHA; STALE parks instate:addressing; CHANGES_REQUESTED blocks at any head.labels-reconcilegroup serializes full-repo sweeps.Nits: dispatch-only bootstrap claim, tolerate missing labels, per-PR subshell.
test/labels-reconcile.shcovers the disputed transitions.labeler.ymlis rig-scoped (bootstrap/users/runner/coolify/db/installer). 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
03e5deca— the maintainer-directed "author escalates" refactor. I agree with removing the body-parsing heuristic outright rather than hardening it: a machine guessing agreement from prose is the wrong-guess-promotes-unapproved-code risk the head-SHA binding exists to prevent.Verified in the diff:
agreement_signal()and theCOMMENTED→APPROVEarm removed;bot_verdict()returnsFEEDBACKforCOMMENTEDand any non-formal state. No body reads.decide_state()keeps human-request as top precedence, so the author's explicit re-request flipsstate:needs-human; auto-request survives only for three formal head-current approvals and stays cold while a comment-only bot is on the panel.labels-reconcileconcurrency group, dispatch-only bootstrap, and per-PR subshell resilience all retained — my Round-1 points stay resolved.labeler.ymlremains rig-scoped.agreement_signalnegative tests correctly dropped with the function.Confirmed byte-identical to
box#85/cast#98. Nothing further from me.— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: I agree with everything and have no additional feedback.
Re-reviewed at
03e5dec. The design change removes review-body parsing, keeps formal approvals bound to the current head, and makes an explicit human review request the authoritative escalation signal. The 14 state-machine fixtures pass, both current checks are green, and the earlier concurrency concern remains resolved by the shared reconcile group.