fix(labels): state:needs-human means a human could merge it right now

Ported from heavy-duty/box#137 (heavy-duty/box#136) so the three repos'
reconcilers stay byte-identical. The state machine here was byte-identical to
box's before this change and remains so after -- only the scope:* taxonomy
differs, correctly.

decide_state() derived state from three inputs -- draft flag, requested
reviewers, submitted reviews -- and read NOTHING about mergeability or checks.
With the `if requested "$HUMAN"` short-circuit at the top of its precedence,
the label was sticky: once the maintainer was requested, a PR read
state:needs-human through conflicts, through red CI, through a force-push that
staled every approval.

In this repo the SECOND half is the live one: three PRs sit at
state:needs-human simultaneously with nothing saying which to merge first, and
they will conflict through CHANGELOG.md the moment one lands. The stickiness
has not bitten here yet only because nothing has conflicted -- the code carried
it identically, so the first merge would have reproduced box's situation.

The rule the label now keeps: state:needs-human means a human could merge this
RIGHT NOW, so anything making that false outranks the request that put it
there.

  CONFLICTING or failing checks -> state:needs-rebase (new; the agent's to fix)
  approvals staled by a push    -> state:addressing   (nobody reviewed this tree)

An UNFINISHED round still yields to an explicit human request -- MISSING
(nobody has reviewed yet) is a different fact from STALE (everyone reviewed
something else). UNKNOWN mergeability is NOT treated as unmergeable: GitHub
reports it for about a minute after every merge, and flapping every open PR
through needs-rebase on each merge would be worse than the bug. A failed read
degrades to the same "do not know" value.

Also adds merge-next -- the label this repo needs most today, since a correct
needs-human still does not say which of three ready PRs to merge first. Queue
order is intent, so the reconciler never sets it, only CLEARS it.

Fixtures 19 -> 29. DRY_RUN against this repo changes NOTHING, which is the
correct result: every open PR here is currently mergeable, so the new
precedence is a no-op on a healthy board and fires only when something is
actually wrong. npm test 623 passed.

Closes #127

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dan-claude-bot 2026-07-20 15:28:50 +00:00
parent 2307746437
commit b063e6bc42
4 changed files with 195 additions and 8 deletions

View file

@ -31,7 +31,7 @@ set -euo pipefail
HUMAN="${HUMAN_REVIEWER:-danmt}" HUMAN="${HUMAN_REVIEWER:-danmt}"
BOTS=(claude-bot-andresmgsl codex-bot-andresmgsl grok-bot-andresmgsl) 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)) STALE_AFTER=$((48 * 3600))
log() { printf 'labels: %s\n' "$*"; } 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 # HEAD_SHA the PR's current head commit
# REQUESTED newline-separated logins with a review currently requested # REQUESTED newline-separated logins with a review currently requested
# REVIEWS_JSON JSON array of submitted (non-PENDING) reviews # 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"; } 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 decide_state() { # → the one state:* label this PR should carry
if [ "$DRAFT" = true ]; then echo state:building; return; fi 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 # state:needs-human means ONE thing: a human could merge this right now.
if requested "$HUMAN"; then echo state:needs-human; return; fi # 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="" local b v verdicts=""
for b in "${BOTS[@]}"; do for b in "${BOTS[@]}"; do
if requested "$b"; then echo state:bots-reviewing; return; fi if requested "$b"; then echo state:bots-reviewing; return; fi
done done
for b in "${BOTS[@]}"; do for b in "${BOTS[@]}"; do
v="$(bot_verdict "$b")" 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" verdicts="$verdicts $v"
done 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 case "$verdicts" in
# FEEDBACK = a comment with no verdict → the agent owes the round-reply. # 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*) echo state:addressing; return ;;
*BLOCK* | *FEEDBACK* | *STALE*) echo state:addressing; return ;;
esac esac
# the bots all approve — but if the human's standing word is # the bots all approve — but if the human's standing word is
# changes-requested (and nobody re-requested them yet), the agent owes # 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: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: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: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 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) stale|B60205|No activity for 48h — needs a poke (sweep-managed)
blocked|6A737D|Waiting on another PR or issue to land first blocked|6A737D|Waiting on another PR or issue to land first
release|0E8A16|Release flow and version/packaging work 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
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 ---- # ---- stale: real activity only, and blocked is legitimately quiet ----
last_activity="$( last_activity="$(
{ {
@ -216,6 +262,21 @@ main() {
# PENDING reviews are unsubmitted drafts in someone's browser — not a verdict # PENDING reviews are unsubmitted drafts in someone's browser — not a verdict
REVIEWS_JSON="$(gh api --paginate "repos/$REPO/pulls/$n/reviews" --jq '.[]' \ REVIEWS_JSON="$(gh api --paginate "repos/$REPO/pulls/$n/reviews" --jq '.[]' \
| jq -s '[.[] | select(.state != "PENDING")]')" | 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" reconcile_pr "$n"
) || log "#$n: reconcile failed — continuing with the remaining PRs" ) || log "#$n: reconcile failed — continuing with the remaining PRs"
done done

View file

@ -7,6 +7,47 @@ actually cutting it, and this file starts there.
## Unreleased ## Unreleased
### Fixed
- **`state:needs-human` no longer appears on PRs a human cannot merge**
(#127, heavy-duty/box#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.
In this repo the *second* half is the live one: three PRs currently sit at
`state:needs-human` simultaneously, with nothing saying which to merge first
— and they will conflict through `CHANGELOG.md` the moment one lands. The
stickiness has not bitten here yet only because nothing has conflicted; the
code carried it identically, so the first merge would have reproduced box's
situation exactly.
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.
Also adds `merge-next` — the label this repo needs most today, since a
correct `needs-human` still does not say *which* of three ready PRs to merge
first. Queue order is intent, so the reconciler never sets it; it only
**clears** it once the PR stops being mergeable-by-a-human. Ported from
heavy-duty/box#137 so the three repos' reconcilers stay byte-identical;
fixtures 19 → 29.
## 0.1.1 — 2026-07-19 ## 0.1.1 — 2026-07-19
### Fixed ### Fixed

View file

@ -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: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: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: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 `bots-reviewing` and `addressing` are deliberately distinct: staleness in the
first means *poke the bots*, staleness in the second means *the agent dropped first means *poke the bots*, staleness in the second means *the agent dropped
the ball*. Collapsing them loses exactly the information a sweep needs. 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) ## Cross-cutting (PRs and issues)
| Label | Color | Meaning | | 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. | | `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. | | `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. | | `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) ## 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: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: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: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 "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 "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 "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 gh label create "release" --color 0E8A16 --description "Release flow and version/packaging work" --force

View file

@ -146,5 +146,61 @@ REQUESTED="$HUMAN"
expect "live human request suppresses re-request" not-needed "$(human_request_needed && echo needed || echo not-needed)" expect "live human request suppresses re-request" not-needed "$(human_request_needed && echo needed || echo not-needed)"
REQUESTED="" 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" printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail"
[ "$fail" -eq 0 ] [ "$fail" -eq 0 ]