Merge pull request #128 from dan-claude-bot/fix/labels-mergeability-aware
fix(labels): `state:needs-human` means a human could merge it right now
This commit is contained in:
commit
5f9f09a70a
4 changed files with 438 additions and 13 deletions
154
.github/scripts/labels-reconcile.sh
vendored
154
.github/scripts/labels-reconcile.sh
vendored
|
|
@ -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,10 +46,80 @@ 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"; }
|
||||||
|
|
||||||
|
checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE
|
||||||
|
# The rollup mixes two node types with two different closed enums: CheckRun
|
||||||
|
# carries `conclusion` (CheckConclusionState), StatusContext carries `state`
|
||||||
|
# (StatusState). Rather than list the outcomes that block — the version that
|
||||||
|
# shipped in this PR's first round listed four, and ERROR, CANCELLED and
|
||||||
|
# STALE fell through its `else` into SUCCESS — this lists the outcomes that
|
||||||
|
# DON'T, and treats everything else as blocking.
|
||||||
|
#
|
||||||
|
# That direction is the point. An outcome we do not recognise is one we
|
||||||
|
# cannot certify as mergeable, and certifying the unrecognised as green is
|
||||||
|
# the exact shape of #136. The cost of being wrong is symmetric in form and
|
||||||
|
# not in consequence: a false FAILURE parks the PR on the agent, who looks;
|
||||||
|
# a false SUCCESS invites a human to merge a tree that will not merge.
|
||||||
|
jq -r '
|
||||||
|
# NEUTRAL and SKIPPED satisfy branch protection — a skipped required check
|
||||||
|
# is not a failed one, and path-filtered jobs skip constantly here.
|
||||||
|
["SUCCESS", "NEUTRAL", "SKIPPED"] as $passing
|
||||||
|
# "" covers a StatusContext still reported with no state at all.
|
||||||
|
| ["", "PENDING", "IN_PROGRESS", "QUEUED", "WAITING", "REQUESTED", "EXPECTED"] as $waiting
|
||||||
|
|
||||||
|
# A re-run does not evict the run it superseded — the rollup keeps both.
|
||||||
|
# This PR proved it: its own tip carried a CANCELLED `scope` (15:19:39)
|
||||||
|
# beside the SUCCESS `scope` (15:19:45) that replaced it, same workflow.
|
||||||
|
# Once CANCELLED blocks, judging every entry would strand this very PR in
|
||||||
|
# needs-rebase forever, so collapse each context to its newest entry first.
|
||||||
|
# Key on workflow + name because a bare job name is only unique within its
|
||||||
|
# workflow.
|
||||||
|
#
|
||||||
|
# Dating a run is the subtle part, and getting it wrong restores the bug.
|
||||||
|
# A run still in flight has no completion, but `gh` does not omit the
|
||||||
|
# field: its Go struct marshals the zero time as "0001-01-01T00:00:00Z",
|
||||||
|
# which is a string, so `//` will not fall through it. Ordering on
|
||||||
|
# completion therefore sorted the LIVE re-run to the bottom and let `last`
|
||||||
|
# pick the very run it superseded — reporting the old SUCCESS while a
|
||||||
|
# replacement was still running, which is #136 again.
|
||||||
|
#
|
||||||
|
# So: date a run by when it BEGAN, discarding both spellings of absent
|
||||||
|
# (null, and the zero sentinel) and falling back only if it never recorded
|
||||||
|
# a beginning. NOT by the newest stamp of any kind: `max` compares the
|
||||||
|
# completion of a finished run against the start of a live one, which are
|
||||||
|
# different quantities and not an ordering on runs. A run cancelled by the
|
||||||
|
# concurrency group does not stop the instant its replacement starts — the
|
||||||
|
# runner has to wind down — so predecessor.completedAt > successor.startedAt
|
||||||
|
# is the ordinary case, and `max` dated the dead predecessor newer than the
|
||||||
|
# live run that replaced it, narrowing both failures above without closing
|
||||||
|
# them. The list is already in preference order, so `first` IS that rule.
|
||||||
|
#
|
||||||
|
# An entry that carries no usable timestamp at all sorts LAST rather than
|
||||||
|
# first — something we cannot date is most likely the thing just created,
|
||||||
|
# and treating it as newest keeps an undateable in-flight run from being
|
||||||
|
# discarded in favour of a stale success. Every ambiguity resolves toward
|
||||||
|
# "not settled".
|
||||||
|
| [ (.statusCheckRollup // [])[]
|
||||||
|
| { ctx: [.workflowName // "", .name // .context // ""],
|
||||||
|
at: ([.startedAt, .createdAt, .completedAt]
|
||||||
|
| map(select(type == "string" and . != ""
|
||||||
|
and (startswith("0001-01-01") | not)))
|
||||||
|
| first // ""),
|
||||||
|
outcome: ((.conclusion // .state // "") | ascii_upcase) } ]
|
||||||
|
| group_by(.ctx)
|
||||||
|
| map(sort_by([(.at == ""), .at]) | last | .outcome) as $latest
|
||||||
|
|
||||||
|
| if ($latest | length) == 0 then "NONE"
|
||||||
|
elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE"
|
||||||
|
elif (($latest - $passing) | length) > 0 then "PENDING"
|
||||||
|
else "SUCCESS" end'
|
||||||
|
}
|
||||||
|
|
||||||
bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK
|
bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK
|
||||||
local review state commit
|
local review state commit
|
||||||
review="$(jq -c --arg u "$1" \
|
review="$(jq -c --arg u "$1" \
|
||||||
|
|
@ -85,22 +155,60 @@ 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 —
|
||||||
local b v verdicts=""
|
# 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 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
|
||||||
|
# Collect the WHOLE round before applying any precedence. Deciding inside
|
||||||
|
# the loop let BOTS order pick the winner: a MISSING returned immediately,
|
||||||
|
# so a STALE belonging to a later bot was never even read, and the mixed
|
||||||
|
# round (one approval staled by a push, another bot yet to review) came out
|
||||||
|
# needs-human — the #136 headline shape, with zero reviews bound to the head.
|
||||||
for b in "${BOTS[@]}"; do
|
for b in "${BOTS[@]}"; do
|
||||||
v="$(bot_verdict "$b")"
|
verdicts="$verdicts $(bot_verdict "$b")"
|
||||||
if [ "$v" = MISSING ]; then echo state:bots-reviewing; return; fi
|
|
||||||
verdicts="$verdicts $v"
|
|
||||||
done
|
done
|
||||||
|
case "$verdicts" in
|
||||||
|
# STALE = a verdict for an older head. Unlike MISSING, this outranks the
|
||||||
|
# human request: every approval it covers 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.
|
||||||
|
# Checked before MISSING because "unfinished" must not swallow "and also
|
||||||
|
# stale": a round that is both is a push that outran the re-requests, not
|
||||||
|
# a maintainer deliberately claiming the PR early.
|
||||||
|
*STALE*) echo state:addressing; return ;;
|
||||||
|
esac
|
||||||
|
case "$verdicts" in
|
||||||
|
# No verdict at all from some bot, and nothing staled. An explicit human
|
||||||
|
# request still outranks an unfinished round — a maintainer pulling a PR
|
||||||
|
# to themselves early is a deliberate act, and the original precedence.
|
||||||
|
*MISSING*)
|
||||||
|
if requested "$HUMAN"; then echo state:needs-human; return; fi
|
||||||
|
echo state:bots-reviewing; 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 +233,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-human|8250DF|All bots approve — waiting on the human reviewer
|
state:needs-rebase|B60205|Does not merge — conflicts or failing checks; the agent owes a fix
|
||||||
|
state:needs-human|8250DF|Mergeable, green, 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 +284,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 +338,16 @@ 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="$(checks_state <<<"$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
|
||||||
|
|
|
||||||
86
CHANGELOG.md
86
CHANGELOG.md
|
|
@ -7,6 +7,92 @@ 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). Precedence is applied to the round as a whole, after every
|
||||||
|
verdict is collected: deciding inside the loop let the order of `BOTS` pick
|
||||||
|
the answer, so a round that was *both* unfinished and staled returned on the
|
||||||
|
`MISSING` before any later bot's `STALE` was read — and came out
|
||||||
|
`needs-human` over a head nobody had reviewed, the original bug wearing a
|
||||||
|
different hat.
|
||||||
|
|
||||||
|
Whether a check blocks is judged by listing the outcomes that *don't* —
|
||||||
|
`SUCCESS`, `NEUTRAL`, `SKIPPED`, and the pending set — rather than the
|
||||||
|
outcomes that do. The rollup mixes two closed enums (`CheckRun.conclusion`
|
||||||
|
and `StatusContext.state`), and an outcome the list forgets is one the label
|
||||||
|
cannot certify as mergeable: `ERROR`, `CANCELLED` and `STALE` all read as
|
||||||
|
green under an allow-list of failures. The costs are not symmetric — a false
|
||||||
|
failure parks the PR on the agent, who looks; a false success invites a human
|
||||||
|
to merge a tree that will not merge. Superseded runs are dropped first, each
|
||||||
|
context collapsing to its newest entry: a re-run does not evict the run it
|
||||||
|
replaced, and the rollup keeps both. That shape is live on this board — this
|
||||||
|
PR's own tip carried two `scope` and two `reconcile` entries — and on
|
||||||
|
heavy-duty/box#137's tip the superseded half was `CANCELLED`, so once
|
||||||
|
`CANCELLED` blocks, judging every entry rather than the newest would strand
|
||||||
|
every re-run PR in `needs-rebase`.
|
||||||
|
|
||||||
|
Dating a run turned out to be the subtle half, and getting it wrong restored
|
||||||
|
the bug. A run still in flight has no completion, but `gh` does not omit the
|
||||||
|
field — its Go struct marshals the zero time as `"0001-01-01T00:00:00Z"`, a
|
||||||
|
string, which jq's `//` will not fall through. Ordering on completion
|
||||||
|
therefore sorted the *live* re-run below every finished one and let the
|
||||||
|
collapse discard it, judging the very run it superseded: a green context with
|
||||||
|
a replacement mid-flight read `SUCCESS` — the original bug restored, pointing
|
||||||
|
a human at a disabled merge button — and a `CANCELLED` original whose
|
||||||
|
replacement was still running read `FAILURE`, the flap the collapse exists to
|
||||||
|
prevent. So a run is dated by when it **began**, with both spellings of
|
||||||
|
absent discarded (`null`, and the zero sentinel) and a fallback only for a
|
||||||
|
run that never recorded a beginning — not by the newest stamp of any kind,
|
||||||
|
which compares the completion of a finished run against the start of a live
|
||||||
|
one. Those are different quantities, and a run cancelled by the concurrency
|
||||||
|
group does not stop the instant its replacement starts: the runner winds
|
||||||
|
down, so a predecessor routinely finishes *after* its successor began, and
|
||||||
|
dating by "newest stamp" let the dead run out-rank the live one that
|
||||||
|
replaced it. An entry carrying no usable timestamp at all sorts **last**
|
||||||
|
rather than first: something undateable is most likely the thing just
|
||||||
|
created, and every ambiguity here resolves toward "not settled" rather than
|
||||||
|
toward a stale success.
|
||||||
|
|
||||||
|
`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; both
|
||||||
|
live shapes, the mixed round, the in-flight run superseding a finished one —
|
||||||
|
in both spellings of an absent completion, in both directions, and across the
|
||||||
|
wind-down window where the two overlap — and the whole check-outcome enum are
|
||||||
|
pinned in `test/labels-reconcile.sh` (fixtures 19 → 51).
|
||||||
|
|
||||||
## 0.1.1 — 2026-07-19
|
## 0.1.1 — 2026-07-19
|
||||||
|
|
||||||
### Fixed
|
### Fixed
|
||||||
|
|
|
||||||
33
LABELS.md
33
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: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-human" --color 8250DF --description "All bots approve — waiting on the human reviewer" --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 "Mergeable, green, 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
|
||||||
|
|
|
||||||
|
|
@ -146,5 +146,183 @@ 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)"
|
||||||
|
|
||||||
|
# -- ...and a round that is BOTH unfinished and staled is still the agent's.
|
||||||
|
# Deciding inside the bot loop made this depend on BOTS order: the MISSING
|
||||||
|
# returned before any later bot's STALE was read, so the mixed round came
|
||||||
|
# out needs-human with nothing bound to the head. Pinned at both ends of
|
||||||
|
# the array, because the whole failure was one of ordering.
|
||||||
|
MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="$HUMAN"
|
||||||
|
REVIEWS_JSON="$(reviews \
|
||||||
|
"$(rev "$BOT1" APPROVED oldhead "" t1)" \
|
||||||
|
"$(rev "$BOT2" APPROVED oldhead "" t2)")"
|
||||||
|
expect "stale approvals + a bot yet to review is addressing, not needs-human" \
|
||||||
|
state:addressing "$(decide_state)"
|
||||||
|
REVIEWS_JSON="$(reviews "$(rev "$BOT3" APPROVED oldhead "" t3)")"
|
||||||
|
expect "...and the same when the stale verdict is the LAST bot in BOTS" \
|
||||||
|
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)"
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# checks_state: the rollup classifier. It lived inline in main() for the first
|
||||||
|
# round of this PR, which is why nothing here caught it calling ERROR,
|
||||||
|
# CANCELLED and STALE green. Extracted so the enum can be pinned down.
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
rollup() { jq -n --argjson c "$1" '{statusCheckRollup: $c}'; }
|
||||||
|
run_() { jq -n --arg n "$1" --arg o "$2" --arg t "${3:-2026-07-20T15:00:00Z}" \
|
||||||
|
'{__typename:"CheckRun", workflowName:"ci", name:$n, conclusion:$o, completedAt:$t}'; }
|
||||||
|
ctx_() { jq -n --arg n "$1" --arg s "$2" --arg t "${3:-2026-07-20T15:00:00Z}" \
|
||||||
|
'{__typename:"StatusContext", context:$n, state:$s, createdAt:$t}'; }
|
||||||
|
|
||||||
|
expect "no checks at all is NONE" NONE "$(rollup '[]' | checks_state)"
|
||||||
|
expect "all green is SUCCESS" SUCCESS \
|
||||||
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)"
|
||||||
|
expect "a queued run is PENDING" PENDING \
|
||||||
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b QUEUED)]" | checks_state)"
|
||||||
|
expect "a plain failure is FAILURE" FAILURE \
|
||||||
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b FAILURE)]" | checks_state)"
|
||||||
|
|
||||||
|
# -- the round-1 gap: outcomes that are neither success nor pending, and that
|
||||||
|
# leave a required check unsatisfied. All three reached the old `else`.
|
||||||
|
expect "a commit status ERROR blocks" FAILURE \
|
||||||
|
"$(rollup "[$(run_ a SUCCESS),$(ctx_ lint ERROR)]" | checks_state)"
|
||||||
|
expect "a CANCELLED run blocks" FAILURE \
|
||||||
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b CANCELLED)]" | checks_state)"
|
||||||
|
expect "a STALE run blocks" FAILURE \
|
||||||
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b STALE)]" | checks_state)"
|
||||||
|
expect "an outcome the enum does not know blocks, it does not pass" FAILURE \
|
||||||
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b SOME_FUTURE_STATE)]" | checks_state)"
|
||||||
|
|
||||||
|
# -- NEUTRAL and SKIPPED satisfy branch protection; path-filtered jobs skip
|
||||||
|
# constantly, and calling that red would park every PR on the agent.
|
||||||
|
expect "NEUTRAL and SKIPPED are not failures" SUCCESS \
|
||||||
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b NEUTRAL),$(run_ c SKIPPED)]" | checks_state)"
|
||||||
|
|
||||||
|
# -- latest-wins. The rollup keeps superseded runs, so this PR's own tip
|
||||||
|
# carried a CANCELLED `scope` beside the SUCCESS `scope` that replaced it.
|
||||||
|
# Without collapsing, making CANCELLED block would strand it forever.
|
||||||
|
expect "a re-run supersedes the cancelled original" SUCCESS \
|
||||||
|
"$(rollup "[$(run_ scope CANCELLED 2026-07-20T15:19:39Z),\
|
||||||
|
$(run_ scope SUCCESS 2026-07-20T15:19:45Z)]" | checks_state)"
|
||||||
|
expect "...and the reverse order is not a re-run passing, it is one failing" FAILURE \
|
||||||
|
"$(rollup "[$(run_ scope SUCCESS 2026-07-20T15:19:39Z),\
|
||||||
|
$(run_ scope CANCELLED 2026-07-20T15:19:45Z)]" | checks_state)"
|
||||||
|
# same job name in a different workflow is a different context, not a re-run
|
||||||
|
expect "same name in another workflow does not supersede" FAILURE \
|
||||||
|
"$(rollup "[$(jq -n '{__typename:"CheckRun",workflowName:"labels",name:"scope",conclusion:"FAILURE",completedAt:"2026-07-20T15:00:00Z"}'),\
|
||||||
|
$(run_ scope SUCCESS 2026-07-20T15:19:45Z)]" | checks_state)"
|
||||||
|
|
||||||
|
# -- a run still IN FLIGHT. `run_()` cannot express this: it always carries a
|
||||||
|
# real completedAt, which is exactly why the supersede rule shipped dating
|
||||||
|
# runs by completion and nothing caught it. Both spellings of "no
|
||||||
|
# completion" are pinned, because `gh` emits the zero sentinel (a string,
|
||||||
|
# which `//` does not fall through) while the API emits null.
|
||||||
|
inflight_() { jq -n --arg n "$1" --arg t "$2" --arg c "${3:-0001-01-01T00:00:00Z}" \
|
||||||
|
'{__typename:"CheckRun", workflowName:"ci", name:$n, status:"IN_PROGRESS",
|
||||||
|
conclusion:"", startedAt:$t, completedAt:(if $c == "null" then null else $c end)}'; }
|
||||||
|
|
||||||
|
expect "a re-run in flight beats the success it superseded (zero sentinel)" PENDING \
|
||||||
|
"$(rollup "[$(run_ build SUCCESS 2026-07-20T15:00:00Z),\
|
||||||
|
$(inflight_ build 2026-07-20T15:10:00Z)]" | checks_state)"
|
||||||
|
expect "...and the same when the absent completion is null" PENDING \
|
||||||
|
"$(rollup "[$(run_ build SUCCESS 2026-07-20T15:00:00Z),\
|
||||||
|
$(inflight_ build 2026-07-20T15:10:00Z null)]" | checks_state)"
|
||||||
|
expect "a replacement in flight for a CANCELLED run is pending, not failed" PENDING \
|
||||||
|
"$(rollup "[$(run_ build CANCELLED 2026-07-20T15:00:00Z),\
|
||||||
|
$(inflight_ build 2026-07-20T15:10:00Z)]" | checks_state)"
|
||||||
|
# an entry carrying no usable timestamp is treated as newest, not oldest —
|
||||||
|
# ambiguity resolves toward "not settled" rather than toward a stale success.
|
||||||
|
# Guarded by the sort tiebreak rather than the dating expression: reverting
|
||||||
|
# only `at:` leaves this passing, so the two changes are separately pinned.
|
||||||
|
expect "an undateable in-flight run is not discarded for a stale success" PENDING \
|
||||||
|
"$(rollup "[$(run_ build SUCCESS 2026-07-20T15:00:00Z),\
|
||||||
|
$(jq -n '{__typename:"CheckRun",workflowName:"ci",name:"build",conclusion:"",startedAt:null,completedAt:null}')]" \
|
||||||
|
| checks_state)"
|
||||||
|
# ...and the reverse direction, which stops "in flight sorts last" being
|
||||||
|
# widened into "in flight always wins": a run that FINISHED after an earlier
|
||||||
|
# in-flight entry is the newer word, and the context is settled.
|
||||||
|
expect "a finished re-run supersedes an earlier in-flight run" SUCCESS \
|
||||||
|
"$(rollup "[$(inflight_ build 2026-07-20T15:19:00Z),\
|
||||||
|
$(run_ build SUCCESS 2026-07-20T15:19:45Z)]" | checks_state)"
|
||||||
|
|
||||||
|
# -- the wind-down window. A predecessor cancelled by the concurrency group
|
||||||
|
# does not stop the instant its replacement starts, so its completion
|
||||||
|
# routinely lands AFTER the successor's start — on box's aa5a6ba the
|
||||||
|
# replacement started 15:19:38 and the run it cancelled finished 15:19:51.
|
||||||
|
# Dating by "newest stamp of any kind" compares the dead run's completion
|
||||||
|
# against the live run's start, which is not an ordering on runs, and the
|
||||||
|
# predecessor wins. Every fixture above spaces completion before start, so
|
||||||
|
# none of them can see it. run_() cannot express the overlap either — it
|
||||||
|
# carries no startedAt — hence the explicit payloads.
|
||||||
|
overlap_() { jq -n --arg n "$1" --arg o "$2" --arg s "$3" --arg c "$4" \
|
||||||
|
'{__typename:"CheckRun", workflowName:"ci", name:$n, conclusion:$o,
|
||||||
|
startedAt:$s, completedAt:$c}'; }
|
||||||
|
expect "a predecessor finishing after its replacement started is still older (CANCELLED)" PENDING \
|
||||||
|
"$(rollup "[$(overlap_ scope CANCELLED 2026-07-20T15:19:00Z 2026-07-20T15:19:51Z),\
|
||||||
|
$(inflight_ scope 2026-07-20T15:19:38Z)]" | checks_state)"
|
||||||
|
expect "...and the same when it finished green — mid-flight is not mergeable" PENDING \
|
||||||
|
"$(rollup "[$(overlap_ build SUCCESS 2026-07-20T15:19:00Z 2026-07-20T15:19:51Z),\
|
||||||
|
$(inflight_ build 2026-07-20T15:19:38Z)]" | checks_state)"
|
||||||
|
|
||||||
|
# -- the classifier feeds the state machine: a cancelled required check must
|
||||||
|
# take the PR off the human's plate, which is the whole point of #136.
|
||||||
|
DRAFT=false HEAD_SHA=head1 REQUESTED="$HUMAN" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE
|
||||||
|
CHECKS="$(rollup "[$(run_ a SUCCESS),$(run_ b CANCELLED)]" | checks_state)"
|
||||||
|
expect "a cancelled check reaches decide_state as needs-rebase" state:needs-rebase "$(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 ]
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue