fix(labels): state:needs-human means a human could merge it right now
#137
4 changed files with 193 additions and 8 deletions
75
.github/scripts/labels-reconcile.sh
vendored
75
.github/scripts/labels-reconcile.sh
vendored
|
|
@ -31,7 +31,7 @@ set -euo pipefail
|
|||
|
||||
HUMAN="${HUMAN_REVIEWER:-danmt}"
|
||||
BOTS=(claude-bot-andresmgsl codex-bot-andresmgsl grok-bot-andresmgsl)
|
||||
STATES=(state:building state:bots-reviewing state:addressing state:needs-human)
|
||||
STATES=(state:building state:needs-rebase state:bots-reviewing state:addressing state:needs-human)
|
||||
STALE_AFTER=$((48 * 3600))
|
||||
|
||||
log() { printf 'labels: %s\n' "$*"; }
|
||||
|
|
@ -46,6 +46,8 @@ run() { # every mutation goes through here — DRY_RUN=1 logs instead of doing
|
|||
# HEAD_SHA the PR's current head commit
|
||||
# REQUESTED newline-separated logins with a review currently requested
|
||||
# REVIEWS_JSON JSON array of submitted (non-PENDING) reviews
|
||||
# MERGEABLE MERGEABLE | CONFLICTING | UNKNOWN (GitHub's own verdict)
|
||||
# CHECKS SUCCESS | FAILURE | PENDING | NONE (the check rollup)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
requested() { grep -qxF "$1" <<<"$REQUESTED"; }
|
||||
|
|
@ -85,22 +87,52 @@ human_request_needed() { # 0 when needs-human requires a FRESH human request
|
|||
|
||||
decide_state() { # → the one state:* label this PR should carry
|
||||
if [ "$DRAFT" = true ]; then echo state:building; return; fi
|
||||
# an explicit human request outranks the bot rounds — it is the final
|
||||
# gate, and a maintainer pulling a PR to themselves early counts too
|
||||
if requested "$HUMAN"; then echo state:needs-human; return; fi
|
||||
|
||||
# state:needs-human means ONE thing: a human could merge this right now.
|
||||
# Anything that makes that false outranks the request that put it there —
|
||||
# otherwise the board invites a merge that cannot or must not happen, and
|
||||
# nothing else on the page contradicts it (#136).
|
||||
#
|
||||
# A conflicted or red branch is the agent's to fix, not the human's to
|
||||
# merge. UNKNOWN is deliberately NOT treated as unmergeable: GitHub reports
|
||||
# it for a minute after every merge while it recomputes, and flapping every
|
||||
# open PR through needs-rebase on each merge would be worse than the bug.
|
||||
# An unknown mergeability simply does not trigger this arm; the next sweep
|
||||
# sees the settled value.
|
||||
# Both default to the "do not know" value: an unset global (older fixture,
|
||||
# a failed fetch) must never invent a verdict it did not read.
|
||||
case "${MERGEABLE:-UNKNOWN}" in CONFLICTING) echo state:needs-rebase; return ;; esac
|
||||
case "${CHECKS:-NONE}" in FAILURE) echo state:needs-rebase; return ;; esac
|
||||
|
||||
local b v verdicts=""
|
||||
for b in "${BOTS[@]}"; do
|
||||
if requested "$b"; then echo state:bots-reviewing; return; fi
|
||||
done
|
||||
for b in "${BOTS[@]}"; do
|
||||
v="$(bot_verdict "$b")"
|
||||
if [ "$v" = MISSING ]; then echo state:bots-reviewing; return; fi
|
||||
if [ "$v" = MISSING ]; then
|
||||
# No verdict at all from this bot. An explicit human request still
|
||||
# outranks an unfinished bot round — a maintainer pulling a PR to
|
||||
# themselves early is a deliberate act, and the original precedence.
|
||||
if requested "$HUMAN"; then echo state:needs-human; return; fi
|
||||
echo state:bots-reviewing; return
|
||||
fi
|
||||
verdicts="$verdicts $v"
|
||||
done
|
||||
case "$verdicts" in
|
||||
# STALE = a verdict for an older head. Unlike MISSING, this outranks the
|
||||
# human request: every approval was invalidated by a push, so NOBODY has
|
||||
# reviewed this tree. Handing that to the human is the #136 case where
|
||||
# everything reads green — mergeable, CI passing, "waiting on the human" —
|
||||
# over code no reviewer has seen. The agent owes a re-request.
|
||||
*STALE*) echo state:addressing; return ;;
|
||||
esac
|
||||
# an explicit human request outranks the remaining bot outcomes — it is the
|
||||
# final gate, and a maintainer pulling a PR to themselves early counts too
|
||||
if requested "$HUMAN"; then echo state:needs-human; return; fi
|
||||
case "$verdicts" in
|
||||
# FEEDBACK = a comment with no verdict → the agent owes the round-reply.
|
||||
# STALE = a verdict for an older head → the agent owes a re-request.
|
||||
*BLOCK* | *FEEDBACK* | *STALE*) echo state:addressing; return ;;
|
||||
*BLOCK* | *FEEDBACK*) echo state:addressing; return ;;
|
||||
esac
|
||||
# the bots all approve — but if the human's standing word is
|
||||
# changes-requested (and nobody re-requested them yet), the agent owes
|
||||
|
|
@ -125,7 +157,9 @@ bootstrap_labels() { # dispatch-only: ~20 upserts is too chatty for every cron t
|
|||
state:building|FBCA04|PR is a draft — the coding agent is still building
|
||||
state:bots-reviewing|1D76DB|Waiting on the bot reviewers to finish the round
|
||||
state:addressing|D93F0B|All bots reviewed — coding agent owes the single reply + fixes
|
||||
state:needs-rebase|B60205|Does not merge — conflicts or failing checks; the agent owes a fix
|
||||
state:needs-human|8250DF|All bots approve — waiting on the human reviewer
|
||||
merge-next|0E8A16|Head of the merge queue — merge this one next (set by hand/agent, cleared here)
|
||||
stale|B60205|No activity for 48h — needs a poke (sweep-managed)
|
||||
blocked|6A737D|Waiting on another PR or issue to land first
|
||||
release|0E8A16|Release flow and version/packaging work
|
||||
|
|
@ -174,6 +208,18 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch
|
|||
fi
|
||||
fi
|
||||
|
||||
# ---- merge-next: cleared, never set ----------------------------------
|
||||
# Queue order is INTENT — which PR should land first is a judgement about
|
||||
# conflicts and dependencies that GitHub knows nothing about, so the
|
||||
# reconciler must not guess it (LABELS.md's rule for `blocked`/`release`).
|
||||
# What it CAN do is stop the label going stale the way needs-human did:
|
||||
# the moment the PR is no longer the thing a human should merge next, the
|
||||
# claim is removed. Setting it stays with whoever owns the queue.
|
||||
if has_label merge-next && [ "$desired" != state:needs-human ]; then
|
||||
run gh issue edit "$n" -R "$REPO" --remove-label merge-next >/dev/null
|
||||
log "#$n: cleared merge-next (state is $desired, not mergeable-by-a-human)"
|
||||
fi
|
||||
|
||||
# ---- stale: real activity only, and blocked is legitimately quiet ----
|
||||
last_activity="$(
|
||||
{
|
||||
|
|
@ -216,6 +262,21 @@ main() {
|
|||
# PENDING reviews are unsubmitted drafts in someone's browser — not a verdict
|
||||
REVIEWS_JSON="$(gh api --paginate "repos/$REPO/pulls/$n/reviews" --jq '.[]' \
|
||||
| jq -s '[.[] | select(.state != "PENDING")]')"
|
||||
# mergeability + the check rollup, the two facts the state machine was
|
||||
# blind to (#136). `gh pr view` rather than the REST PR object: the API's
|
||||
# `mergeable` is a tri-state boolean that GitHub computes lazily, while
|
||||
# this returns the same MERGEABLE/CONFLICTING/UNKNOWN string the UI shows.
|
||||
# Failure to read them is NOT fatal and NOT treated as broken — an API
|
||||
# hiccup must never flap every PR into needs-rebase, so both degrade to
|
||||
# the "do not know" value that triggers nothing.
|
||||
GH_VIEW="$(gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup 2>/dev/null || echo '{}')"
|
||||
MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")"
|
||||
CHECKS="$(jq -r '
|
||||
(.statusCheckRollup // []) as $c
|
||||
| if ($c | length) == 0 then "NONE"
|
||||
elif ($c | map(.conclusion // .state // "") | any(. == "FAILURE" or . == "TIMED_OUT" or . == "STARTUP_FAILURE" or . == "ACTION_REQUIRED")) then "FAILURE"
|
||||
elif ($c | map(.conclusion // .state // "") | any(. == "" or . == "PENDING" or . == "IN_PROGRESS" or . == "QUEUED")) then "PENDING"
|
||||
else "SUCCESS" end' <<<"$GH_VIEW")"
|
||||
reconcile_pr "$n"
|
||||
) || log "#$n: reconcile failed — continuing with the remaining PRs"
|
||||
done
|
||||
|
|
|
|||
39
CHANGELOG.md
39
CHANGELOG.md
|
|
@ -61,6 +61,45 @@ which records not just what changed but what each drill run proved.
|
|||
|
||||
### Fixed
|
||||
|
||||
- **`state:needs-human` no longer appears on PRs a human cannot merge** (#136)
|
||||
— `decide_state()` derived state from three inputs (draft flag, requested
|
||||
reviewers, submitted reviews) and read *nothing* about mergeability or
|
||||
checks. Combined with the `if requested "$HUMAN"` short-circuit at the top of
|
||||
its precedence, the label was **sticky**: once the maintainer was requested,
|
||||
the PR read `state:needs-human` through conflicts, through red CI, through a
|
||||
force-push that staled every approval. Nothing demoted it.
|
||||
|
||||
Observed twice in one afternoon on this repo, in two different shapes. Three
|
||||
PRs sat at `state:needs-human` while `CONFLICTING` for hours — the board
|
||||
inviting a merge GitHub had already disabled. And #119, after a rebase, read
|
||||
`MERGEABLE`, four green checks, `state:needs-human` — with **zero** reviews
|
||||
bound to its head. Every visible signal said *merge me* over a tree no
|
||||
reviewer had seen, and unlike the conflict case, nothing on the page
|
||||
contradicted it.
|
||||
|
||||
The rule the label now keeps is that **`state:needs-human` means a human
|
||||
could merge this right now**, so anything making that false outranks the
|
||||
request that put it there. A `CONFLICTING` branch or a failing check is the
|
||||
agent's to fix: new `state:needs-rebase`. Approvals staled by a push mean
|
||||
nobody reviewed this tree: `state:addressing`, because the agent owes a
|
||||
re-request. An *unfinished* round still yields to an explicit human request —
|
||||
a maintainer pulling a PR to themselves early is deliberate, and `MISSING`
|
||||
(nobody has reviewed yet) is a different fact from `STALE` (everyone reviewed
|
||||
something else).
|
||||
|
||||
`UNKNOWN` mergeability is deliberately not treated as unmergeable: GitHub
|
||||
reports it for about a minute after every merge while it recomputes, and
|
||||
flapping every open PR through `needs-rebase` on each merge would be worse
|
||||
than the bug. A failed read of either fact degrades to the same "do not know"
|
||||
value for the same reason — an API hiccup must not relabel the board.
|
||||
|
||||
Also adds `merge-next`, because a correct `needs-human` still does not say
|
||||
*which* PR to merge first, and order matters when they conflict through
|
||||
`CHANGELOG.md`. Queue order is intent, so the reconciler never sets it — it
|
||||
only **clears** it the moment the PR stops being mergeable-by-a-human, which
|
||||
is precisely the staleness that made `needs-human` untrustworthy. Both live
|
||||
shapes are pinned in `test/labels-reconcile.sh` (19 fixtures → 29).
|
||||
|
||||
- **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) —
|
||||
`globstar` makes `**` descend into subdirectories, but a glob still does
|
||||
not *match* a dot-prefixed name, so `**/` never entered `.github/`. The
|
||||
|
|
|
|||
31
LABELS.md
31
LABELS.md
|
|
@ -17,12 +17,38 @@ single reply, and a human takes the final review.
|
|||
| `state:building` | `#FBCA04` | the coding agent, still building | PR opened as draft | marked ready + bot reviews requested |
|
||||
| `state:bots-reviewing` | `#1D76DB` | the reviewer bots to finish the round | ready with reviews requested, or fixes pushed and reviews re-requested | all three bots have reviewed the round |
|
||||
| `state:addressing` | `#D93F0B` | the coding agent to reply and push fixes | all bots reviewed the round, not all approved | the single round-reply is posted and fixes pushed |
|
||||
| `state:needs-human` | `#8250DF` | the human reviewer | the human review is requested — by the author when the round passes, or automatically on three formal head-current approvals | merged — or changes requested, which cycles back to `state:addressing` |
|
||||
| `state:needs-rebase` | `#B60205` | the coding agent to rebase or fix | the branch does not merge — GitHub says `CONFLICTING`, or a check has failed | it merges cleanly and checks are green again |
|
||||
| `state:needs-human` | `#8250DF` | the human reviewer | the PR **could be merged right now**: mergeable, checks not failing, three formal head-current approvals — and the human review is requested | merged — or changes requested, which cycles back to `state:addressing` |
|
||||
|
||||
`bots-reviewing` and `addressing` are deliberately distinct: staleness in the
|
||||
first means *poke the bots*, staleness in the second means *the agent dropped
|
||||
the ball*. Collapsing them loses exactly the information a sweep needs.
|
||||
|
||||
**`state:needs-human` means one thing: a human could merge this right now.**
|
||||
Anything that makes that false outranks the review request that put it there,
|
||||
because the label is the only signal a maintainer scanning the board (or a
|
||||
phone) actually reads — and a label that says "your turn" on an unmergeable PR
|
||||
is worse than no label at all. Two things therefore take precedence over an
|
||||
explicit human request:
|
||||
|
||||
- **it does not merge** — `CONFLICTING`, or a failing check → `state:needs-rebase`
|
||||
- **nobody reviewed *this* head** — every approval staled by a push → `state:addressing`,
|
||||
because the agent owes a re-request
|
||||
|
||||
The second is the more dangerous of the two: with a conflict, GitHub at least
|
||||
disables the merge button, while a staled-approval PR reads green, mergeable
|
||||
and "waiting on the human" over code no reviewer has seen.
|
||||
|
||||
`UNKNOWN` mergeability is deliberately **not** treated as unmergeable. GitHub
|
||||
reports it for about a minute after every merge while it recomputes, and
|
||||
flapping every open PR through `needs-rebase` on each merge would be worse than
|
||||
the bug this precedence fixes.
|
||||
|
||||
An *unfinished* round still yields to an explicit human request — a maintainer
|
||||
pulling a PR to themselves early is a deliberate act. `MISSING` (nobody has
|
||||
reviewed yet) and `STALE` (everyone reviewed something else) are different
|
||||
facts and are treated differently.
|
||||
|
||||
## Cross-cutting (PRs and issues)
|
||||
|
||||
| Label | Color | Meaning |
|
||||
|
|
@ -30,6 +56,7 @@ the ball*. Collapsing them loses exactly the information a sweep needs.
|
|||
| `stale` | `#B60205` | No activity for 48h. Sweep-managed, never hand-applied. `state:building` + `stale` is precisely a forgotten draft. |
|
||||
| `blocked` | `#6A737D` | Waiting on another PR or issue to land first. Quiet *legitimately* — the staleness sweep skips it. |
|
||||
| `release` | `#0E8A16` | Release flow, versioning, and packaging work. |
|
||||
| `merge-next` | `#0E8A16` | Head of the merge queue — **merge this one next**. Queue order is *intent* (which PR lands first, given how they conflict), so the reconciler never sets it: you or the agent maintaining the queue do. The reconciler only **clears** it, the moment the PR stops being something a human could merge — so it cannot go stale the way `state:needs-human` did. |
|
||||
|
||||
## Scope — which surface? (PRs and issues, any number)
|
||||
|
||||
|
|
@ -69,7 +96,9 @@ missing label idempotently. To create them by hand (needs push access):
|
|||
gh label create "state:building" --color FBCA04 --description "PR is a draft — the coding agent is still building" --force
|
||||
gh label create "state:bots-reviewing" --color 1D76DB --description "Waiting on the bot reviewers to finish the round" --force
|
||||
gh label create "state:addressing" --color D93F0B --description "All bots reviewed — coding agent owes the single reply + fixes" --force
|
||||
gh label create "state:needs-rebase" --color B60205 --description "Does not merge — conflicts or failing checks; the agent owes a fix" --force
|
||||
gh label create "state:needs-human" --color 8250DF --description "All bots approve — waiting on the human reviewer" --force
|
||||
gh label create "merge-next" --color 0E8A16 --description "Head of the merge queue — merge this one next (set by hand/agent, cleared here)" --force
|
||||
gh label create "stale" --color B60205 --description "No activity for 48h — needs a poke (sweep-managed)" --force
|
||||
gh label create "blocked" --color 6A737D --description "Waiting on another PR or issue to land first" --force
|
||||
gh label create "release" --color 0E8A16 --description "Release flow and version/packaging work" --force
|
||||
|
|
|
|||
|
|
@ -146,5 +146,61 @@ REQUESTED="$HUMAN"
|
|||
expect "live human request suppresses re-request" not-needed "$(human_request_needed && echo needed || echo not-needed)"
|
||||
REQUESTED=""
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #136: state:needs-human must mean "a human could merge this RIGHT NOW".
|
||||
# Both cases below were observed live in this repo on 2026-07-20, and both
|
||||
# showed state:needs-human while being unmergeable in different ways.
|
||||
# ---------------------------------------------------------------------------
|
||||
ALL_APPROVE="$(reviews \
|
||||
"$(rev "$BOT1" APPROVED head1 "" t1)" \
|
||||
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
||||
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
||||
|
||||
# -- flavour 1: not mergeable. The merge button is disabled, yet the board
|
||||
# said "your turn" on #119/#120/#127 for hours.
|
||||
DRAFT=false HEAD_SHA=head1 REQUESTED="" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=CONFLICTING CHECKS=SUCCESS
|
||||
expect "a CONFLICTING PR is needs-rebase, not needs-human" state:needs-rebase "$(decide_state)"
|
||||
REQUESTED="$HUMAN"
|
||||
expect "...even with the human explicitly requested" state:needs-rebase "$(decide_state)"
|
||||
|
||||
# -- red CI is the same claim: not something a human should merge.
|
||||
REQUESTED="" MERGEABLE=MERGEABLE CHECKS=FAILURE
|
||||
expect "a red PR is needs-rebase" state:needs-rebase "$(decide_state)"
|
||||
REQUESTED="$HUMAN"
|
||||
expect "...and a human request does not override red CI" state:needs-rebase "$(decide_state)"
|
||||
|
||||
# -- UNKNOWN is NOT unmergeable. GitHub reports it for ~a minute after every
|
||||
# merge while it recomputes; treating it as broken would flap every open PR
|
||||
# into needs-rebase on each merge — worse than the bug being fixed.
|
||||
REQUESTED="" MERGEABLE=UNKNOWN CHECKS=PENDING
|
||||
expect "UNKNOWN mergeability does not trigger needs-rebase" state:needs-human "$(decide_state)"
|
||||
|
||||
# -- flavour 2 (the dangerous one): mergeable, green, human requested, and
|
||||
# NOBODY has reviewed this head. Observed on #119 after a rebase: every
|
||||
# signal read "merge me" and nothing on the page contradicted it.
|
||||
MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="$HUMAN"
|
||||
REVIEWS_JSON="$(reviews \
|
||||
"$(rev "$BOT1" APPROVED oldhead "" t1)" \
|
||||
"$(rev "$BOT2" APPROVED oldhead "" t2)" \
|
||||
"$(rev "$BOT3" APPROVED oldhead "" t3)")"
|
||||
expect "stale approvals outrank the human request (nobody reviewed this tree)" state:addressing "$(decide_state)"
|
||||
|
||||
# -- but an UNFINISHED round still yields to an explicit human request: a
|
||||
# maintainer pulling a PR to themselves early is deliberate, and was the
|
||||
# original precedence. MISSING differs from STALE — nobody has reviewed
|
||||
# YET, versus everyone reviewed something else.
|
||||
REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")"
|
||||
expect "an unfinished round still yields to an explicit human request" state:needs-human "$(decide_state)"
|
||||
REQUESTED=""
|
||||
expect "...and without that request it is still bots-reviewing" state:bots-reviewing "$(decide_state)"
|
||||
|
||||
# -- the happy path survives all of the above.
|
||||
REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED=""
|
||||
expect "mergeable + green + three head-current approvals is needs-human" state:needs-human "$(decide_state)"
|
||||
# -- and a draft outranks everything, including a conflict.
|
||||
DRAFT=true MERGEABLE=CONFLICTING
|
||||
expect "a draft is building even when conflicted" state:building "$(decide_state)"
|
||||
DRAFT=false MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" REVIEWS_JSON='[]'
|
||||
|
||||
printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail"
|
||||
[ "$fail" -eq 0 ]
|
||||
|
|
|
|||
Loading…
Reference in a new issue