fix: verdict contract, head-bound approvals, serialized reconcile — and a testable state machine
Round-1 blockers, all three reviewers concurring:
- COMMENTED agreement now counts: agreement_signal recognizes the live bots'
durable markers (Verdict: Approve / I agree with everything / leading ✅) —
the gate to needs-human can actually close. Formal verdicts remain the
contract (CONTRIBUTING), this is the documented transitional workaround.
- Every counting verdict is bound to the head SHA; a stale approval parks the
PR in addressing (agent owes re-request) instead of promoting unreviewed
code. CHANGES_REQUESTED blocks at any head, per GitHub's own semantic.
- reconcile serializes under ONE job-level concurrency group; scope stays
per-PR. No more cron-vs-event race on the request-the-human-once guard.
- Sweep resilience: per-PR subshell (one failure logs and continues), label
edits warn instead of wedging; the self-heal claim now matches reality
(dispatch-only bootstrap).
- The state machine is extracted pure (globals in, state out) and sourceable:
test/labels-reconcile.sh proves 14 fixture transitions — comment-only
agreement, stale approval, comment-without-verdict, human precedence and
human-block — wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 19:31:00 +00:00
|
|
|
#!/usr/bin/env bash
|
|
|
|
|
set -euo pipefail
|
|
|
|
|
|
2026-07-18 19:51:52 +00:00
|
|
|
# Fixture tests for the labels-reconcile state machine: a comment is a
|
|
|
|
|
# non-verdict whatever its body says (the AUTHOR escalates by requesting the
|
|
|
|
|
# human), a stale approval does not promote unreviewed code, and an explicit
|
|
|
|
|
# human request outranks everything.
|
fix: verdict contract, head-bound approvals, serialized reconcile — and a testable state machine
Round-1 blockers, all three reviewers concurring:
- COMMENTED agreement now counts: agreement_signal recognizes the live bots'
durable markers (Verdict: Approve / I agree with everything / leading ✅) —
the gate to needs-human can actually close. Formal verdicts remain the
contract (CONTRIBUTING), this is the documented transitional workaround.
- Every counting verdict is bound to the head SHA; a stale approval parks the
PR in addressing (agent owes re-request) instead of promoting unreviewed
code. CHANGES_REQUESTED blocks at any head, per GitHub's own semantic.
- reconcile serializes under ONE job-level concurrency group; scope stays
per-PR. No more cron-vs-event race on the request-the-human-once guard.
- Sweep resilience: per-PR subshell (one failure logs and continues), label
edits warn instead of wedging; the self-heal claim now matches reality
(dispatch-only bootstrap).
- The state machine is extracted pure (globals in, state out) and sourceable:
test/labels-reconcile.sh proves 14 fixture transitions — comment-only
agreement, stale approval, comment-without-verdict, human precedence and
human-block — wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 19:31:00 +00:00
|
|
|
# Dependency-free beyond jq; no network, no daemon — pure decide_state.
|
|
|
|
|
|
|
|
|
|
cd "$(dirname "$0")/.."
|
|
|
|
|
# shellcheck source=.github/scripts/labels-reconcile.sh
|
|
|
|
|
. .github/scripts/labels-reconcile.sh
|
|
|
|
|
|
|
|
|
|
# The DRAFT/HEAD_SHA/REQUESTED/REVIEWS_JSON assignments below are the state
|
|
|
|
|
# machine's inputs, consumed inside the sourced decide_state — not unused.
|
|
|
|
|
# shellcheck disable=SC2034
|
|
|
|
|
BOT1="${BOTS[0]}" BOT2="${BOTS[1]}" BOT3="${BOTS[2]}"
|
|
|
|
|
pass=0 fail=0
|
|
|
|
|
|
|
|
|
|
expect() { # $1 = description, $2 = want, $3 = got
|
|
|
|
|
if [ "$2" = "$3" ]; then
|
|
|
|
|
pass=$((pass + 1))
|
|
|
|
|
else
|
|
|
|
|
fail=$((fail + 1))
|
|
|
|
|
printf 'FAIL: %s — want %s, got %s\n' "$1" "$2" "$3"
|
|
|
|
|
fi
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
rev() { # $1=login $2=state $3=commit $4=body $5=submitted_at → one review object
|
|
|
|
|
jq -n --arg u "$1" --arg s "$2" --arg c "$3" --arg b "$4" --arg t "$5" \
|
|
|
|
|
'{user: {login: $u}, state: $s, commit_id: $c, body: $b, submitted_at: $t}'
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
reviews() { jq -s '.' <<<"$*"; } # collect review objects into an array
|
|
|
|
|
|
|
|
|
|
# -- drafts are building, whoever is requested --------------------------------
|
|
|
|
|
DRAFT=true HEAD_SHA=head1 REQUESTED="" REVIEWS_JSON='[]'
|
|
|
|
|
expect "draft PR is building" state:building "$(decide_state)"
|
|
|
|
|
|
|
|
|
|
# -- fresh ready PR with bots requested ---------------------------------------
|
|
|
|
|
DRAFT=false REQUESTED="$BOT1
|
|
|
|
|
$BOT2
|
|
|
|
|
$BOT3" REVIEWS_JSON='[]'
|
|
|
|
|
expect "requested bots mean bots-reviewing" state:bots-reviewing "$(decide_state)"
|
|
|
|
|
|
|
|
|
|
# -- a bot that never reviewed keeps the round open ---------------------------
|
2026-07-20 17:27:06 +00:00
|
|
|
# With a live request that is the bots' ball; with NO request outstanding it
|
|
|
|
|
# is the agent's, because nothing is coming until somebody asks.
|
|
|
|
|
REQUESTED="$BOT3" REVIEWS_JSON="$(reviews \
|
fix: verdict contract, head-bound approvals, serialized reconcile — and a testable state machine
Round-1 blockers, all three reviewers concurring:
- COMMENTED agreement now counts: agreement_signal recognizes the live bots'
durable markers (Verdict: Approve / I agree with everything / leading ✅) —
the gate to needs-human can actually close. Formal verdicts remain the
contract (CONTRIBUTING), this is the documented transitional workaround.
- Every counting verdict is bound to the head SHA; a stale approval parks the
PR in addressing (agent owes re-request) instead of promoting unreviewed
code. CHANGES_REQUESTED blocks at any head, per GitHub's own semantic.
- reconcile serializes under ONE job-level concurrency group; scope stays
per-PR. No more cron-vs-event race on the request-the-human-once guard.
- Sweep resilience: per-PR subshell (one failure logs and continues), label
edits warn instead of wedging; the self-heal claim now matches reality
(dispatch-only bootstrap).
- The state machine is extracted pure (globals in, state out) and sourceable:
test/labels-reconcile.sh proves 14 fixture transitions — comment-only
agreement, stale approval, comment-without-verdict, human precedence and
human-block — wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 19:31:00 +00:00
|
|
|
"$(rev "$BOT1" APPROVED head1 "" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)")"
|
2026-07-20 17:27:06 +00:00
|
|
|
expect "a missing bot WITH a live request is bots-reviewing" state:bots-reviewing "$(decide_state)"
|
|
|
|
|
REQUESTED=""
|
|
|
|
|
expect "...but with nobody asked it is the agent's ball" state:addressing "$(decide_state)"
|
|
|
|
|
expect "...and the blocker names the stall" blocker:unrequested "$(blockers)"
|
fix: verdict contract, head-bound approvals, serialized reconcile — and a testable state machine
Round-1 blockers, all three reviewers concurring:
- COMMENTED agreement now counts: agreement_signal recognizes the live bots'
durable markers (Verdict: Approve / I agree with everything / leading ✅) —
the gate to needs-human can actually close. Formal verdicts remain the
contract (CONTRIBUTING), this is the documented transitional workaround.
- Every counting verdict is bound to the head SHA; a stale approval parks the
PR in addressing (agent owes re-request) instead of promoting unreviewed
code. CHANGES_REQUESTED blocks at any head, per GitHub's own semantic.
- reconcile serializes under ONE job-level concurrency group; scope stays
per-PR. No more cron-vs-event race on the request-the-human-once guard.
- Sweep resilience: per-PR subshell (one failure logs and continues), label
edits warn instead of wedging; the self-heal claim now matches reality
(dispatch-only bootstrap).
- The state machine is extracted pure (globals in, state out) and sourceable:
test/labels-reconcile.sh proves 14 fixture transitions — comment-only
agreement, stale approval, comment-without-verdict, human precedence and
human-block — wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 19:31:00 +00:00
|
|
|
|
2026-07-18 19:51:52 +00:00
|
|
|
# -- a comment is a non-verdict, agreement body or not: the author escalates --
|
fix: verdict contract, head-bound approvals, serialized reconcile — and a testable state machine
Round-1 blockers, all three reviewers concurring:
- COMMENTED agreement now counts: agreement_signal recognizes the live bots'
durable markers (Verdict: Approve / I agree with everything / leading ✅) —
the gate to needs-human can actually close. Formal verdicts remain the
contract (CONTRIBUTING), this is the documented transitional workaround.
- Every counting verdict is bound to the head SHA; a stale approval parks the
PR in addressing (agent owes re-request) instead of promoting unreviewed
code. CHANGES_REQUESTED blocks at any head, per GitHub's own semantic.
- reconcile serializes under ONE job-level concurrency group; scope stays
per-PR. No more cron-vs-event race on the request-the-human-once guard.
- Sweep resilience: per-PR subshell (one failure logs and continues), label
edits warn instead of wedging; the self-heal claim now matches reality
(dispatch-only bootstrap).
- The state machine is extracted pure (globals in, state out) and sourceable:
test/labels-reconcile.sh proves 14 fixture transitions — comment-only
agreement, stale approval, comment-without-verdict, human precedence and
human-block — wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 19:31:00 +00:00
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" COMMENTED head1 "✅ **Reviewed — I agree with everything.**" t1)" \
|
2026-07-18 19:51:52 +00:00
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
|
|
|
|
expect "comment-only agreement still parks on the author" state:addressing "$(decide_state)"
|
|
|
|
|
# ...and the author's escalation — requesting the human — flips it
|
|
|
|
|
REQUESTED="$HUMAN"
|
|
|
|
|
expect "author escalation flips to needs-human" state:needs-human "$(decide_state)"
|
|
|
|
|
REQUESTED=""
|
|
|
|
|
|
|
|
|
|
# -- three formal approvals need no author judgment ---------------------------
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" APPROVED head1 "" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
|
|
|
|
expect "three formal approvals reach needs-human" state:needs-human "$(decide_state)"
|
fix: verdict contract, head-bound approvals, serialized reconcile — and a testable state machine
Round-1 blockers, all three reviewers concurring:
- COMMENTED agreement now counts: agreement_signal recognizes the live bots'
durable markers (Verdict: Approve / I agree with everything / leading ✅) —
the gate to needs-human can actually close. Formal verdicts remain the
contract (CONTRIBUTING), this is the documented transitional workaround.
- Every counting verdict is bound to the head SHA; a stale approval parks the
PR in addressing (agent owes re-request) instead of promoting unreviewed
code. CHANGES_REQUESTED blocks at any head, per GitHub's own semantic.
- reconcile serializes under ONE job-level concurrency group; scope stays
per-PR. No more cron-vs-event race on the request-the-human-once guard.
- Sweep resilience: per-PR subshell (one failure logs and continues), label
edits warn instead of wedging; the self-heal claim now matches reality
(dispatch-only bootstrap).
- The state machine is extracted pure (globals in, state out) and sourceable:
test/labels-reconcile.sh proves 14 fixture transitions — comment-only
agreement, stale approval, comment-without-verdict, human precedence and
human-block — wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 19:31:00 +00:00
|
|
|
|
|
|
|
|
# -- a comment WITHOUT a verdict parks the PR on the agent --------------------
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" COMMENTED head1 "🔧 Reviewed — I agree with most; feedback below." t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
|
|
|
|
expect "comment without verdict is addressing" state:addressing "$(decide_state)"
|
|
|
|
|
|
|
|
|
|
# -- changes requested blocks, at any head ------------------------------------
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" CHANGES_REQUESTED old1 "blockers below" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
|
|
|
|
expect "changes-requested blocks even from an old head" state:addressing "$(decide_state)"
|
|
|
|
|
|
|
|
|
|
# -- a stale approval must not promote unreviewed code ------------------------
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" APPROVED old1 "" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
|
|
|
|
expect "stale approval is addressing (agent owes re-request)" state:addressing "$(decide_state)"
|
|
|
|
|
|
|
|
|
|
# -- a re-requested bot reopens the round even with an old approval on file ---
|
|
|
|
|
REQUESTED="$BOT1"
|
|
|
|
|
expect "re-requested bot means bots-reviewing" state:bots-reviewing "$(decide_state)"
|
|
|
|
|
REQUESTED=""
|
|
|
|
|
|
|
|
|
|
# -- only the LATEST review per bot counts ------------------------------------
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" CHANGES_REQUESTED head1 "blockers" t1)" \
|
|
|
|
|
"$(rev "$BOT1" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t3)" \
|
2026-07-18 19:51:52 +00:00
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t4)")"
|
fix: verdict contract, head-bound approvals, serialized reconcile — and a testable state machine
Round-1 blockers, all three reviewers concurring:
- COMMENTED agreement now counts: agreement_signal recognizes the live bots'
durable markers (Verdict: Approve / I agree with everything / leading ✅) —
the gate to needs-human can actually close. Formal verdicts remain the
contract (CONTRIBUTING), this is the documented transitional workaround.
- Every counting verdict is bound to the head SHA; a stale approval parks the
PR in addressing (agent owes re-request) instead of promoting unreviewed
code. CHANGES_REQUESTED blocks at any head, per GitHub's own semantic.
- reconcile serializes under ONE job-level concurrency group; scope stays
per-PR. No more cron-vs-event race on the request-the-human-once guard.
- Sweep resilience: per-PR subshell (one failure logs and continues), label
edits warn instead of wedging; the self-heal claim now matches reality
(dispatch-only bootstrap).
- The state machine is extracted pure (globals in, state out) and sourceable:
test/labels-reconcile.sh proves 14 fixture transitions — comment-only
agreement, stale approval, comment-without-verdict, human precedence and
human-block — wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 19:31:00 +00:00
|
|
|
expect "later approval supersedes earlier block" state:needs-human "$(decide_state)"
|
|
|
|
|
|
|
|
|
|
# -- an explicit human request outranks the bot rounds ------------------------
|
|
|
|
|
REQUESTED="$HUMAN" REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" COMMENTED head1 "feedback, no verdict" t1)")"
|
|
|
|
|
expect "human requested outranks bots" state:needs-human "$(decide_state)"
|
|
|
|
|
REQUESTED=""
|
|
|
|
|
|
|
|
|
|
# -- human CHANGES_REQUESTED puts the ball back on the agent ------------------
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" APPROVED head1 "" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)" \
|
|
|
|
|
"$(rev "$HUMAN" CHANGES_REQUESTED head1 "not yet" t4)")"
|
|
|
|
|
expect "human block with bots approving is addressing" state:addressing "$(decide_state)"
|
|
|
|
|
# ...and re-requesting the human hands it back to them
|
|
|
|
|
REQUESTED="$HUMAN"
|
|
|
|
|
expect "re-requested human is needs-human again" state:needs-human "$(decide_state)"
|
|
|
|
|
REQUESTED=""
|
|
|
|
|
|
2026-07-18 20:16:55 +00:00
|
|
|
# -- an old human comment must not wedge the handoff (codex, #85 round 3) -----
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$HUMAN" COMMENTED old1 "early thoughts" t0)" \
|
|
|
|
|
"$(rev "$BOT1" APPROVED head1 "" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
|
|
|
|
expect "old human comment + three approvals is needs-human" state:needs-human "$(decide_state)"
|
|
|
|
|
expect "old human comment still needs a fresh request" needed "$(human_request_needed && echo needed || echo not-needed)"
|
|
|
|
|
# ...a stale human APPROVAL likewise needs a re-request for the new head
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$HUMAN" APPROVED old1 "" t0)" \
|
|
|
|
|
"$(rev "$BOT1" APPROVED head1 "" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
|
|
|
|
expect "stale human approval needs a fresh request" needed "$(human_request_needed && echo needed || echo not-needed)"
|
|
|
|
|
# ...a HEAD-CURRENT human approval needs nothing more
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$HUMAN" APPROVED head1 "" t0)" \
|
|
|
|
|
"$(rev "$BOT1" APPROVED head1 "" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
|
|
|
|
expect "head-current human approval needs no request" not-needed "$(human_request_needed && echo needed || echo not-needed)"
|
|
|
|
|
# ...and a live request suppresses re-requesting
|
|
|
|
|
REQUESTED="$HUMAN"
|
|
|
|
|
expect "live human request suppresses re-request" not-needed "$(human_request_needed && echo needed || echo not-needed)"
|
|
|
|
|
REQUESTED=""
|
|
|
|
|
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
# #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
|
2026-07-20 17:27:06 +00:00
|
|
|
# said "your turn" on #119/#120/#127 for hours. The branch fact now rides
|
|
|
|
|
# the blocker axis; the state says whose ball it is, which is the agent's.
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
DRAFT=false HEAD_SHA=head1 REQUESTED="" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=CONFLICTING CHECKS=SUCCESS
|
2026-07-20 17:27:06 +00:00
|
|
|
expect "a CONFLICTING PR is the agent's, not the human's" state:addressing "$(decide_state)"
|
|
|
|
|
expect "...and says WHY on the blocker axis" blocker:conflict "$(blockers)"
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
REQUESTED="$HUMAN"
|
2026-07-20 17:27:06 +00:00
|
|
|
expect "...even with the human explicitly requested" state:addressing "$(decide_state)"
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
|
2026-07-20 17:27:06 +00:00
|
|
|
# -- red CI is the same claim, but NOT the same work: a rebase does not fix a
|
|
|
|
|
# failing test. Collapsing both into one needs-rebase label told the agent
|
|
|
|
|
# to do the wrong thing, which is why the axis split exists.
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
REQUESTED="" MERGEABLE=MERGEABLE CHECKS=FAILURE
|
2026-07-20 17:27:06 +00:00
|
|
|
expect "a red PR is the agent's" state:addressing "$(decide_state)"
|
|
|
|
|
expect "...and is distinguishable from a conflict" blocker:ci-red "$(blockers)"
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
REQUESTED="$HUMAN"
|
2026-07-20 17:27:06 +00:00
|
|
|
expect "...and a human request does not override red CI" state:addressing "$(decide_state)"
|
|
|
|
|
|
|
|
|
|
# -- both at once. The single-axis design could not say this at all: one label
|
|
|
|
|
# had to win, and the loser silently vanished off the board.
|
|
|
|
|
REQUESTED="" MERGEABLE=CONFLICTING CHECKS=FAILURE
|
|
|
|
|
expect "a conflicted AND red PR reports both blockers" "blocker:conflict
|
|
|
|
|
blocker:ci-red" "$(blockers)"
|
|
|
|
|
expect "...and is still just the agent's ball" state:addressing "$(decide_state)"
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
|
|
|
|
|
# -- 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
|
2026-07-20 17:27:06 +00:00
|
|
|
# on each merge — worse than the bug being fixed.
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
REQUESTED="" MERGEABLE=UNKNOWN CHECKS=PENDING
|
2026-07-20 17:27:06 +00:00
|
|
|
expect "UNKNOWN mergeability blocks nothing" state:needs-human "$(decide_state)"
|
|
|
|
|
expect "...and raises no blocker" "" "$(blockers)"
|
|
|
|
|
|
|
|
|
|
# -- blocker:unrequested — the stalled round. Nobody owes an answer because
|
|
|
|
|
# nobody was ever asked, yet the board read "waiting on the bots" until
|
|
|
|
|
# `stale` noticed 48h later.
|
|
|
|
|
MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" REVIEWS_JSON='[]'
|
|
|
|
|
expect "ready, nobody asked, nothing reviewed raises unrequested" blocker:unrequested "$(blockers)"
|
|
|
|
|
# ...the partial case is equally stalled: one verdict in, nobody asked for the rest
|
|
|
|
|
REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")"
|
|
|
|
|
expect "one bot in, none requested is still unrequested" blocker:unrequested "$(blockers)"
|
2026-07-20 17:50:00 +00:00
|
|
|
# ...a STALE round with nobody asked is the same debt, and arguably worse: the
|
|
|
|
|
# page carries approvals that no longer describe the tree. Guarding on
|
|
|
|
|
# MISSING alone let this one through with no blocker at all.
|
|
|
|
|
REVIEWS_JSON="$(reviews \
|
|
|
|
|
"$(rev "$BOT1" APPROVED oldhead "" t1)" \
|
|
|
|
|
"$(rev "$BOT2" APPROVED oldhead "" t2)" \
|
|
|
|
|
"$(rev "$BOT3" APPROVED oldhead "" t3)")"
|
|
|
|
|
expect "a stale round with nobody asked is unrequested too" blocker:unrequested "$(blockers)"
|
|
|
|
|
expect "...and is still the agent's ball" state:addressing "$(decide_state)"
|
2026-07-20 17:27:06 +00:00
|
|
|
# ...but a live request means an answer IS coming
|
2026-07-20 17:50:00 +00:00
|
|
|
REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")"
|
2026-07-20 17:27:06 +00:00
|
|
|
REQUESTED="$BOT2"
|
|
|
|
|
expect "a live bot request is not a stalled round" "" "$(blockers)"
|
|
|
|
|
# ...and a draft is exempt: the bots ignore drafts by design
|
|
|
|
|
DRAFT=true REQUESTED="" REVIEWS_JSON='[]'
|
|
|
|
|
expect "a draft with nobody asked is not stalled" "" "$(blockers)"
|
|
|
|
|
# ...as is an explicit human request — claiming a PR early is deliberate
|
|
|
|
|
DRAFT=false REQUESTED="$HUMAN"
|
|
|
|
|
expect "an early human claim is not a stalled round" "" "$(blockers)"
|
|
|
|
|
REQUESTED="" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE CHECKS=SUCCESS
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
|
|
|
|
|
# -- 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)"
|
|
|
|
|
|
fix(labels): an unrecognised check outcome blocks, and a staled round outranks an unfinished one
Round 2. Two blockers, both real, both closing the same hole this PR
exists to close — a `state:needs-human` that invites a human to merge a
tree that will not merge.
The check-rollup classifier enumerated the outcomes that block and let
the rest fall into `else "SUCCESS"`, so ERROR, CANCELLED and STALE all
read as green. Inverted: it now lists the outcomes that DON'T block —
SUCCESS, NEUTRAL, SKIPPED, plus the pending set — and treats everything
else as blocking. The direction is the point. The rollup mixes two
closed enums (CheckRun.conclusion, StatusContext.state) and an outcome
the list forgets is one we cannot certify as mergeable; the costs are
not symmetric, since a false FAILURE parks the PR on the agent who
looks, while a false SUCCESS is #136 exactly.
Once CANCELLED blocks, superseded runs must be dropped first: a re-run
does not evict the run it replaced, and this PR's own tip carries a
CANCELLED `scope` beside the SUCCESS `scope` that superseded it. Each
context now collapses to its newest entry before anything is judged,
keyed on workflow + job name because a bare job name is only unique
within its workflow. That preserves the re-run case the panel split
over while still blocking a cancelled run that is the newest word.
The classifier also moved out of main() into checks_state(). That is
why no fixture caught this: it was inline in the fetch loop, so the
fixtures could only inject CHECKS= as an already-decided string.
Second, decide_state() returned from inside the bot loop on the first
MISSING, so a STALE belonging to a later bot in BOTS was never read —
a round that was both unfinished and staled came out needs-human over
a head nobody had reviewed. The whole round is now collected before
precedence is applied to it as a unit, STALE before MISSING.
test/labels-reconcile.sh: 29 -> 44 fixtures, pinning the check-outcome
enum, the supersede rule (both orders, plus same name in another
workflow), and the mixed round at both ends of BOTS. All verified
non-vacuous against the round-1 code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:04:16 +00:00
|
|
|
# -- ...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)"
|
|
|
|
|
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
# -- 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=""
|
2026-07-20 17:27:06 +00:00
|
|
|
expect "...and without that request the agent owes the ask" state:addressing "$(decide_state)"
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
|
fix(labels): an unrecognised check outcome blocks, and a staled round outranks an unfinished one
Round 2. Two blockers, both real, both closing the same hole this PR
exists to close — a `state:needs-human` that invites a human to merge a
tree that will not merge.
The check-rollup classifier enumerated the outcomes that block and let
the rest fall into `else "SUCCESS"`, so ERROR, CANCELLED and STALE all
read as green. Inverted: it now lists the outcomes that DON'T block —
SUCCESS, NEUTRAL, SKIPPED, plus the pending set — and treats everything
else as blocking. The direction is the point. The rollup mixes two
closed enums (CheckRun.conclusion, StatusContext.state) and an outcome
the list forgets is one we cannot certify as mergeable; the costs are
not symmetric, since a false FAILURE parks the PR on the agent who
looks, while a false SUCCESS is #136 exactly.
Once CANCELLED blocks, superseded runs must be dropped first: a re-run
does not evict the run it replaced, and this PR's own tip carries a
CANCELLED `scope` beside the SUCCESS `scope` that superseded it. Each
context now collapses to its newest entry before anything is judged,
keyed on workflow + job name because a bare job name is only unique
within its workflow. That preserves the re-run case the panel split
over while still blocking a cancelled run that is the newest word.
The classifier also moved out of main() into checks_state(). That is
why no fixture caught this: it was inline in the fetch loop, so the
fixtures could only inject CHECKS= as an already-decided string.
Second, decide_state() returned from inside the bot loop on the first
MISSING, so a STALE belonging to a later bot in BOTS was never read —
a round that was both unfinished and staled came out needs-human over
a head nobody had reviewed. The whole round is now collected before
precedence is applied to it as a unit, STALE before MISSING.
test/labels-reconcile.sh: 29 -> 44 fixtures, pinning the check-outcome
enum, the supersede rule (both orders, plus same name in another
workflow), and the mixed round at both ends of BOTS. All verified
non-vacuous against the round-1 code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:04:16 +00:00
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
# 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)"
|
2026-07-20 17:36:46 +00:00
|
|
|
# A failed fetch leaves no rollup KEY; a PR with no checks leaves an empty
|
|
|
|
|
# ARRAY. Collapsing the two let an API hiccup read as "nothing is failing" —
|
|
|
|
|
# the same unknown-certified-as-green shape as #136, in the one place that
|
|
|
|
|
# fix did not look. The caller skips an UNREADABLE PR rather than relabelling.
|
|
|
|
|
expect "a failed read is UNREADABLE, not NONE" UNREADABLE "$(echo '{}' | checks_state)"
|
|
|
|
|
expect "...and a real empty rollup is still NONE" NONE \
|
|
|
|
|
"$(echo '{"mergeable":"MERGEABLE","statusCheckRollup":[]}' | checks_state)"
|
fix(labels): an unrecognised check outcome blocks, and a staled round outranks an unfinished one
Round 2. Two blockers, both real, both closing the same hole this PR
exists to close — a `state:needs-human` that invites a human to merge a
tree that will not merge.
The check-rollup classifier enumerated the outcomes that block and let
the rest fall into `else "SUCCESS"`, so ERROR, CANCELLED and STALE all
read as green. Inverted: it now lists the outcomes that DON'T block —
SUCCESS, NEUTRAL, SKIPPED, plus the pending set — and treats everything
else as blocking. The direction is the point. The rollup mixes two
closed enums (CheckRun.conclusion, StatusContext.state) and an outcome
the list forgets is one we cannot certify as mergeable; the costs are
not symmetric, since a false FAILURE parks the PR on the agent who
looks, while a false SUCCESS is #136 exactly.
Once CANCELLED blocks, superseded runs must be dropped first: a re-run
does not evict the run it replaced, and this PR's own tip carries a
CANCELLED `scope` beside the SUCCESS `scope` that superseded it. Each
context now collapses to its newest entry before anything is judged,
keyed on workflow + job name because a bare job name is only unique
within its workflow. That preserves the re-run case the panel split
over while still blocking a cancelled run that is the newest word.
The classifier also moved out of main() into checks_state(). That is
why no fixture caught this: it was inline in the fetch loop, so the
fixtures could only inject CHECKS= as an already-decided string.
Second, decide_state() returned from inside the bot loop on the first
MISSING, so a STALE belonging to a later bot in BOTS was never read —
a round that was both unfinished and staled came out needs-human over
a head nobody had reviewed. The whole round is now collected before
precedence is applied to it as a unit, STALE before MISSING.
test/labels-reconcile.sh: 29 -> 44 fixtures, pinning the check-outcome
enum, the supersede rule (both orders, plus same name in another
workflow), and the mixed round at both ends of BOTS. All verified
non-vacuous against the round-1 code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:04:16 +00:00
|
|
|
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)"
|
|
|
|
|
|
fix(labels): date a check run by the newest stamp it carries, not by its completion
Round 3. @claude-bot-andresmgsl and @codex-bot-andresmgsl independently
caught that the supersede rule added in round 2 inverted itself on the
one shape its fixtures could not express: a re-run still in flight.
A running check does not omit `completedAt`. `gh` marshals the Go zero
time as the STRING "0001-01-01T00:00:00Z", and jq's `//` only falls
through null/false, so the sentinel won the sort key and sorted before
every real timestamp. The live re-run became the OLDEST entry in its
context, `last` discarded it, and the run it superseded was judged
instead — exactly backwards, and wrong in both directions:
green + re-run in flight -> SUCCESS (should be PENDING)
CANCELLED + re-run in flight -> FAILURE (should be PENDING)
The first is #136 restored by the very rule meant to close it: all bots
approve, mergeable, state:needs-human — over a tree whose merge button
branch protection has disabled. It was also a regression from round 1,
which caught it via `any(. == "")`. The second is the re-run flap the
supersede rule exists to prevent, narrowed rather than removed.
Fixed by taking the newest timestamp a run actually carries and
discarding BOTH spellings of absent — null and the zero sentinel —
rather than by reordering the fallbacks. An entry with no usable
timestamp now sorts LAST rather than first, so an undateable in-flight
run is never dropped in favour of a stale success. Every ambiguity
resolves toward "not settled".
The fixtures could not have caught this: the `run_()` helper sets only
`completedAt`, so every supersede fixture was a race between two
FINISHED runs. The helper now expresses an in-flight entry, and the
four new fixtures assert PENDING over both a green and a cancelled
predecessor. 44 -> 48; reverting just the dating expression fails 3 of
the 4.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:18:22 +00:00
|
|
|
# -- 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 —
|
2026-07-20 16:29:59 +00:00
|
|
|
# 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.
|
fix(labels): date a check run by the newest stamp it carries, not by its completion
Round 3. @claude-bot-andresmgsl and @codex-bot-andresmgsl independently
caught that the supersede rule added in round 2 inverted itself on the
one shape its fixtures could not express: a re-run still in flight.
A running check does not omit `completedAt`. `gh` marshals the Go zero
time as the STRING "0001-01-01T00:00:00Z", and jq's `//` only falls
through null/false, so the sentinel won the sort key and sorted before
every real timestamp. The live re-run became the OLDEST entry in its
context, `last` discarded it, and the run it superseded was judged
instead — exactly backwards, and wrong in both directions:
green + re-run in flight -> SUCCESS (should be PENDING)
CANCELLED + re-run in flight -> FAILURE (should be PENDING)
The first is #136 restored by the very rule meant to close it: all bots
approve, mergeable, state:needs-human — over a tree whose merge button
branch protection has disabled. It was also a regression from round 1,
which caught it via `any(. == "")`. The second is the re-run flap the
supersede rule exists to prevent, narrowed rather than removed.
Fixed by taking the newest timestamp a run actually carries and
discarding BOTH spellings of absent — null and the zero sentinel —
rather than by reordering the fallbacks. An entry with no usable
timestamp now sorts LAST rather than first, so an undateable in-flight
run is never dropped in favour of a stale success. Every ambiguity
resolves toward "not settled".
The fixtures could not have caught this: the `run_()` helper sets only
`completedAt`, so every supersede fixture was a race between two
FINISHED runs. The helper now expresses an in-flight entry, and the
four new fixtures assert PENDING over both a green and a cancelled
predecessor. 44 -> 48; reverting just the dating expression fails 3 of
the 4.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:18:22 +00:00
|
|
|
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)"
|
2026-07-20 16:29:59 +00:00
|
|
|
# ...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)"
|
fix(labels): date a check run by the newest stamp it carries, not by its completion
Round 3. @claude-bot-andresmgsl and @codex-bot-andresmgsl independently
caught that the supersede rule added in round 2 inverted itself on the
one shape its fixtures could not express: a re-run still in flight.
A running check does not omit `completedAt`. `gh` marshals the Go zero
time as the STRING "0001-01-01T00:00:00Z", and jq's `//` only falls
through null/false, so the sentinel won the sort key and sorted before
every real timestamp. The live re-run became the OLDEST entry in its
context, `last` discarded it, and the run it superseded was judged
instead — exactly backwards, and wrong in both directions:
green + re-run in flight -> SUCCESS (should be PENDING)
CANCELLED + re-run in flight -> FAILURE (should be PENDING)
The first is #136 restored by the very rule meant to close it: all bots
approve, mergeable, state:needs-human — over a tree whose merge button
branch protection has disabled. It was also a regression from round 1,
which caught it via `any(. == "")`. The second is the re-run flap the
supersede rule exists to prevent, narrowed rather than removed.
Fixed by taking the newest timestamp a run actually carries and
discarding BOTH spellings of absent — null and the zero sentinel —
rather than by reordering the fallbacks. An entry with no usable
timestamp now sorts LAST rather than first, so an undateable in-flight
run is never dropped in favour of a stale success. Every ambiguity
resolves toward "not settled".
The fixtures could not have caught this: the `run_()` helper sets only
`completedAt`, so every supersede fixture was a race between two
FINISHED runs. The helper now expresses an in-flight entry, and the
four new fixtures assert PENDING over both a green and a cancelled
predecessor. 44 -> 48; reverting just the dating expression fails 3 of
the 4.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:18:22 +00:00
|
|
|
|
fix(labels): date a check run by when it BEGAN, not by the newest stamp it carries
Third and final correction to the supersede rule, and the second one
that silently restored #136.
Round 3 replaced "date by completion" with "date by the newest stamp
the run carries". That is still not an ordering on runs: `max` over
[startedAt, createdAt, completedAt] resolves to completedAt for a
FINISHED run and startedAt for a LIVE one — different quantities, so
the comparison was never between like and like.
The consequence is the ordinary concurrency-group path, not an edge
case. A run cancelled by a concurrency group does not stop instantly;
it drains AFTER its replacement has already started, so
predecessor.completedAt > successor.startedAt is the normal shape. On
this PR's own aa5a6ba the window was 13 seconds. Inside it the dead
predecessor out-dated the live run replacing it, and a green
predecessor reported SUCCESS while a re-run was still in flight:
SUCCESS completing 15:30:13, replacement started 15:30:00
max -> SUCCESS (#136: needs-human over a disabled merge button)
first -> PENDING
Fixed with `max` -> `first`. The list is already in preference order,
so `first` IS "date it by when it began" — and a replacement always
begins after the run it replaces, whatever order they finish in. The
sentinel filtering and the undateable-sorts-last tiebreak are
unchanged; this narrows the rule to a quantity that actually orders.
Prescribed independently by claude-bot-andresmgsl and
codex-bot-andresmgsl.
Two fixtures pin the drain window in both colours. 49 -> 51.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:41:49 +00:00
|
|
|
# -- 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)"
|
|
|
|
|
|
fix(labels): an unrecognised check outcome blocks, and a staled round outranks an unfinished one
Round 2. Two blockers, both real, both closing the same hole this PR
exists to close — a `state:needs-human` that invites a human to merge a
tree that will not merge.
The check-rollup classifier enumerated the outcomes that block and let
the rest fall into `else "SUCCESS"`, so ERROR, CANCELLED and STALE all
read as green. Inverted: it now lists the outcomes that DON'T block —
SUCCESS, NEUTRAL, SKIPPED, plus the pending set — and treats everything
else as blocking. The direction is the point. The rollup mixes two
closed enums (CheckRun.conclusion, StatusContext.state) and an outcome
the list forgets is one we cannot certify as mergeable; the costs are
not symmetric, since a false FAILURE parks the PR on the agent who
looks, while a false SUCCESS is #136 exactly.
Once CANCELLED blocks, superseded runs must be dropped first: a re-run
does not evict the run it replaced, and this PR's own tip carries a
CANCELLED `scope` beside the SUCCESS `scope` that superseded it. Each
context now collapses to its newest entry before anything is judged,
keyed on workflow + job name because a bare job name is only unique
within its workflow. That preserves the re-run case the panel split
over while still blocking a cancelled run that is the newest word.
The classifier also moved out of main() into checks_state(). That is
why no fixture caught this: it was inline in the fetch loop, so the
fixtures could only inject CHECKS= as an already-decided string.
Second, decide_state() returned from inside the bot loop on the first
MISSING, so a STALE belonging to a later bot in BOTS was never read —
a round that was both unfinished and staled came out needs-human over
a head nobody had reviewed. The whole round is now collected before
precedence is applied to it as a unit, STALE before MISSING.
test/labels-reconcile.sh: 29 -> 44 fixtures, pinning the check-outcome
enum, the supersede rule (both orders, plus same name in another
workflow), and the mixed round at both ends of BOTS. All verified
non-vacuous against the round-1 code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:04:16 +00:00
|
|
|
# -- 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)"
|
2026-07-20 17:27:06 +00:00
|
|
|
expect "a cancelled check reaches decide_state as the agent's ball" state:addressing "$(decide_state)"
|
|
|
|
|
expect "...via blocker:ci-red, not a conflict" blocker:ci-red "$(blockers)"
|
fix(labels): an unrecognised check outcome blocks, and a staled round outranks an unfinished one
Round 2. Two blockers, both real, both closing the same hole this PR
exists to close — a `state:needs-human` that invites a human to merge a
tree that will not merge.
The check-rollup classifier enumerated the outcomes that block and let
the rest fall into `else "SUCCESS"`, so ERROR, CANCELLED and STALE all
read as green. Inverted: it now lists the outcomes that DON'T block —
SUCCESS, NEUTRAL, SKIPPED, plus the pending set — and treats everything
else as blocking. The direction is the point. The rollup mixes two
closed enums (CheckRun.conclusion, StatusContext.state) and an outcome
the list forgets is one we cannot certify as mergeable; the costs are
not symmetric, since a false FAILURE parks the PR on the agent who
looks, while a false SUCCESS is #136 exactly.
Once CANCELLED blocks, superseded runs must be dropped first: a re-run
does not evict the run it replaced, and this PR's own tip carries a
CANCELLED `scope` beside the SUCCESS `scope` that superseded it. Each
context now collapses to its newest entry before anything is judged,
keyed on workflow + job name because a bare job name is only unique
within its workflow. That preserves the re-run case the panel split
over while still blocking a cancelled run that is the newest word.
The classifier also moved out of main() into checks_state(). That is
why no fixture caught this: it was inline in the fetch loop, so the
fixtures could only inject CHECKS= as an already-decided string.
Second, decide_state() returned from inside the bot loop on the first
MISSING, so a STALE belonging to a later bot in BOTS was never read —
a round that was both unfinished and staled came out needs-human over
a head nobody had reviewed. The whole round is now collected before
precedence is applied to it as a unit, STALE before MISSING.
test/labels-reconcile.sh: 29 -> 44 fixtures, pinning the check-outcome
enum, the supersede rule (both orders, plus same name in another
workflow), and the mixed round at both ends of BOTS. All verified
non-vacuous against the round-1 code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 16:04:16 +00:00
|
|
|
|
fix(labels): state:needs-human means a human could merge it right now
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, 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 saying "merge me" over a tree no reviewer had seen. That second
shape is the dangerous one, because unlike a conflict nothing on the page
contradicts it.
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 -- 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). That distinction is why the two are handled in different arms rather
than collapsed.
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 being fixed. 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 -- precisely the staleness that
made needs-human untrustworthy.
Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures),
including that UNKNOWN does not trigger needs-rebase and that a draft outranks
a conflict. Proven non-vacuous: dropping the mergeability arm fails 4
assertions, dropping the STALE precedence fails 2, restoring returns 29/0.
Closes #136
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 15:18:49 +00:00
|
|
|
# -- 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='[]'
|
|
|
|
|
|
fix(labels): a missing state label must skip the EDIT, not the whole PR
Round 2 review (claude-bot, codex-bot, grok-bot -- all three, independently).
The label pre-flight added in round 1 returned out of reconcile_pr entirely.
Everything below that point is independent of the state:* taxonomy: clearing a
stale merge-next, and the stale sweep. Stranding them meant a merge-next claim
reading "merge this one next" survived on a PR the board had moved to the
agent -- the same false invitation as #136, one scope smaller -- and the
staleness detector went silent. On a cold-start repo, where no state:* label
exists yet, that was EVERY PR.
It was also a regression against main rather than a missed improvement: the old
code failed the edit, logged, and fell through to both blocks. The round-1 fix
turned a per-edit failure into a per-PR abort.
Now skip_edit=true, and control reaches the rest of the function.
Also taken, both from claude-bot and grok-bot: the dead "$desired" term in the
filter loop, and `[ -n "$missing" ] && log` becoming a proper elif rather than
an &&-as-statement under set -e.
Four fixtures now drive reconcile_pr itself, stubbing run/gh -- the first in
this suite to reach past the pure functions, which is exactly why a per-PR
return was invisible to the fixtures that existed. Fixtures 68 -> 72.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 18:09:07 +00:00
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
# reconcile_pr's cold-start path. Everything above tests pure functions, which
|
|
|
|
|
# is exactly why a per-PR `return` in the label pre-flight got through review:
|
|
|
|
|
# the fixtures could not reach it. A missing state:* label must skip the label
|
|
|
|
|
# EDIT only — merge-next clearing and the stale sweep are independent of the
|
|
|
|
|
# taxonomy, and stranding them reintroduced the false-invitation bug (a
|
|
|
|
|
# `merge-next` claim surviving on a PR the board had moved to the agent).
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
reconcile_probe() { # $1 = REPO_LABELS content → the log lines reconcile_pr emits
|
|
|
|
|
(
|
|
|
|
|
REPO_LABELS="$1" REPO=owner/repo NOW="$(date +%s)"
|
|
|
|
|
LABELS="merge-next" # the PR carries a queue claim
|
|
|
|
|
DRAFT=false HEAD_SHA=head1 REQUESTED="" REVIEWS_JSON='[]'
|
|
|
|
|
MERGEABLE=MERGEABLE CHECKS=SUCCESS
|
|
|
|
|
PR_JSON='{"created_at":"2020-01-01T00:00:00Z"}'
|
|
|
|
|
run() { :; } # swallow mutations
|
|
|
|
|
gh() { :; } # no network
|
|
|
|
|
reconcile_pr 777 2>&1
|
|
|
|
|
)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
cold="$(reconcile_probe "merge-next")" # state:* labels absent entirely
|
|
|
|
|
expect "a cold-start repo still clears merge-next" \
|
|
|
|
|
yes "$(grep -q 'cleared merge-next' <<<"$cold" && echo yes || echo no)"
|
|
|
|
|
expect "...and still runs the stale sweep" \
|
|
|
|
|
yes "$(grep -q 'stale (' <<<"$cold" && echo yes || echo no)"
|
|
|
|
|
expect "...while warning that the state label is missing" \
|
|
|
|
|
yes "$(grep -q "state label 'state:addressing' does not exist" <<<"$cold" && echo yes || echo no)"
|
|
|
|
|
|
|
|
|
|
warm="$(reconcile_probe "$(printf 'state:addressing\nmerge-next\nstale\nblocker:unrequested')")"
|
|
|
|
|
expect "a bootstrapped repo converges the state as well" \
|
|
|
|
|
yes "$(grep -q 'state -> state:addressing' <<<"$warm" && echo yes || echo no)"
|
|
|
|
|
|
fix: verdict contract, head-bound approvals, serialized reconcile — and a testable state machine
Round-1 blockers, all three reviewers concurring:
- COMMENTED agreement now counts: agreement_signal recognizes the live bots'
durable markers (Verdict: Approve / I agree with everything / leading ✅) —
the gate to needs-human can actually close. Formal verdicts remain the
contract (CONTRIBUTING), this is the documented transitional workaround.
- Every counting verdict is bound to the head SHA; a stale approval parks the
PR in addressing (agent owes re-request) instead of promoting unreviewed
code. CHANGES_REQUESTED blocks at any head, per GitHub's own semantic.
- reconcile serializes under ONE job-level concurrency group; scope stays
per-PR. No more cron-vs-event race on the request-the-human-once guard.
- Sweep resilience: per-PR subshell (one failure logs and continues), label
edits warn instead of wedging; the self-heal claim now matches reality
(dispatch-only bootstrap).
- The state machine is extracted pure (globals in, state out) and sourceable:
test/labels-reconcile.sh proves 14 fixture transitions — comment-only
agreement, stale approval, comment-without-verdict, human precedence and
human-block — wired into CI.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-18 19:31:00 +00:00
|
|
|
printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail"
|
|
|
|
|
[ "$fail" -eq 0 ]
|