refactor: the author escalates — drop the agreement-body heuristic
Maintainer direction: body-parsing agreement was a guess, and the machine must not guess. COMMENTED is now unconditionally a non-verdict; the judgment that a comment-only reviewer's round passed belongs to the PR AUTHOR, who escalates by requesting the human's review — an explicit request is a fact, and it is the machine's top-precedence input. Auto-request survives only for the no-judgment case: three formal head-current approvals. CONTRIBUTING and LABELS.md state the handoff; fixtures updated (14 transitions, including author-escalation and the three-formal-approvals path). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
08d099cce2
commit
9d3fed8901
4 changed files with 51 additions and 55 deletions
46
.github/scripts/labels-reconcile.sh
vendored
46
.github/scripts/labels-reconcile.sh
vendored
|
|
@ -10,13 +10,15 @@ set -euo pipefail
|
||||||
# own mark every tick.
|
# own mark every tick.
|
||||||
#
|
#
|
||||||
# The verdict contract (CONTRIBUTING.md): reviews end in approve or
|
# The verdict contract (CONTRIBUTING.md): reviews end in approve or
|
||||||
# request-changes. Reality check (#85 round 1): at least one live bot posts
|
# request-changes. Some live bots are comment-only and post agreement as a
|
||||||
# its agreement as a COMMENTED review and can never formally approve, which
|
# COMMENTED review — a non-verdict this machine refuses to guess about (body
|
||||||
# would park every fully-agreed PR in state:addressing forever. So COMMENTED
|
# parsing is a heuristic, and a wrong guess promotes an unapproved PR). The
|
||||||
# reviews whose body carries a durable agreement signal count as approval —
|
# judgment call belongs to the PR AUTHOR, who reads the round and escalates
|
||||||
# the workaround the state machine owes the fleet until every bot speaks the
|
# by requesting the human's review — an explicit request is a fact, and it is
|
||||||
# formal contract. Any verdict that counts toward needs-human must be bound
|
# the one this machine trusts (see decide_state's top precedence). The
|
||||||
# to the CURRENT head SHA: GitHub keeps approvals alive across pushes, and a
|
# machine auto-requests the human only in the no-judgment-needed case: three
|
||||||
|
# formal head-current approvals. Any approval that counts must be bound to
|
||||||
|
# the CURRENT head SHA: GitHub keeps approvals alive across pushes, and a
|
||||||
# stale approval must never promote unreviewed code to the human.
|
# stale approval must never promote unreviewed code to the human.
|
||||||
#
|
#
|
||||||
# DRY_RUN=1 narrates every mutation instead of performing it (how this script
|
# DRY_RUN=1 narrates every mutation instead of performing it (how this script
|
||||||
|
|
@ -48,22 +50,14 @@ run() { # every mutation goes through here — DRY_RUN=1 logs instead of doing
|
||||||
|
|
||||||
requested() { grep -qxF "$1" <<<"$REQUESTED"; }
|
requested() { grep -qxF "$1" <<<"$REQUESTED"; }
|
||||||
|
|
||||||
agreement_signal() { # $1 = review body → 0 when it carries a durable agreement
|
|
||||||
# the signals the live bots actually emit: grok "**Verdict: Approve**",
|
|
||||||
# codex "Verdict: I agree with everything", claude "✅ … I agree with
|
|
||||||
# everything". Conservative on purpose: "I agree with most" is NOT a match.
|
|
||||||
grep -qiE 'verdict:? ?\**approve|i agree with everything|^✅' <<<"$1"
|
|
||||||
}
|
|
||||||
|
|
||||||
bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK
|
bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK
|
||||||
local review state commit body
|
local review state commit
|
||||||
review="$(jq -c --arg u "$1" \
|
review="$(jq -c --arg u "$1" \
|
||||||
'[.[] | select(.user.login == $u)] | sort_by(.submitted_at) | last // empty' \
|
'[.[] | select(.user.login == $u)] | sort_by(.submitted_at) | last // empty' \
|
||||||
<<<"$REVIEWS_JSON")"
|
<<<"$REVIEWS_JSON")"
|
||||||
if [ -z "$review" ]; then echo MISSING; return; fi
|
if [ -z "$review" ]; then echo MISSING; return; fi
|
||||||
state="$(jq -r '.state' <<<"$review")"
|
state="$(jq -r '.state' <<<"$review")"
|
||||||
commit="$(jq -r '.commit_id' <<<"$review")"
|
commit="$(jq -r '.commit_id' <<<"$review")"
|
||||||
body="$(jq -r '.body // ""' <<<"$review")"
|
|
||||||
case "$state" in
|
case "$state" in
|
||||||
CHANGES_REQUESTED)
|
CHANGES_REQUESTED)
|
||||||
# blocks at ANY head — GitHub's own semantic: only a newer review
|
# blocks at ANY head — GitHub's own semantic: only a newer review
|
||||||
|
|
@ -71,13 +65,11 @@ bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK
|
||||||
echo BLOCK ;;
|
echo BLOCK ;;
|
||||||
APPROVED)
|
APPROVED)
|
||||||
if [ "$commit" = "$HEAD_SHA" ]; then echo APPROVE; else echo STALE; fi ;;
|
if [ "$commit" = "$HEAD_SHA" ]; then echo APPROVE; else echo STALE; fi ;;
|
||||||
COMMENTED)
|
*)
|
||||||
if agreement_signal "$body"; then
|
# COMMENTED and anything else: a non-verdict. The machine does not
|
||||||
if [ "$commit" = "$HEAD_SHA" ]; then echo APPROVE; else echo STALE; fi
|
# read bodies — if the comment is really an agreement, the AUTHOR
|
||||||
else
|
# says so by requesting the human's review.
|
||||||
echo FEEDBACK
|
echo FEEDBACK ;;
|
||||||
fi ;;
|
|
||||||
*) echo FEEDBACK ;;
|
|
||||||
esac
|
esac
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -143,9 +135,11 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch
|
||||||
|
|
||||||
desired="$(decide_state)"
|
desired="$(decide_state)"
|
||||||
|
|
||||||
# encode the runbook's last step: the round passed → the human is asked,
|
# encode the runbook's last step for the no-judgment case: three formal
|
||||||
# once. The guard (never requested, never reviewed) makes it idempotent —
|
# head-current approvals → the human is asked, once. The guard (never
|
||||||
# and the shared concurrency group in labels.yml makes it race-free.
|
# requested, never reviewed) makes it idempotent — and the shared
|
||||||
|
# concurrency group in labels.yml makes it race-free. With a comment-only
|
||||||
|
# bot on the panel this path stays cold and the AUTHOR requests the human.
|
||||||
if [ "$desired" = state:needs-human ] && ! requested "$HUMAN" \
|
if [ "$desired" = state:needs-human ] && ! requested "$HUMAN" \
|
||||||
&& [ -z "$(jq -r --arg u "$HUMAN" '[.[] | select(.user.login == $u)] | last | .state // empty' <<<"$REVIEWS_JSON")" ]; then
|
&& [ -z "$(jq -r --arg u "$HUMAN" '[.[] | select(.user.login == $u)] | last | .state // empty' <<<"$REVIEWS_JSON")" ]; then
|
||||||
run gh api "repos/$REPO/pulls/$n/requested_reviewers" -f "reviewers[]=$HUMAN" --silent
|
run gh api "repos/$REPO/pulls/$n/requested_reviewers" -f "reviewers[]=$HUMAN" --silent
|
||||||
|
|
|
||||||
|
|
@ -28,13 +28,16 @@ labels tell you where everything is without opening anything.
|
||||||
them at their discretion; anything blocking — including a question that
|
them at their discretion; anything blocking — including a question that
|
||||||
gates the verdict — is **request changes**, saying what unblocks it. The
|
gates the verdict — is **request changes**, saying what unblocks it. The
|
||||||
reconciler treats a comment-only review as not-approved, so commenting
|
reconciler treats a comment-only review as not-approved, so commenting
|
||||||
without a verdict only stalls the PR. (Transitional workaround: until
|
without a verdict only stalls the PR. The machine never reads review
|
||||||
every bot speaks the formal contract, the reconciler counts a COMMENTED
|
bodies: when a comment-only reviewer's line is really an agreement, that
|
||||||
review whose body carries a durable agreement signal — "Verdict: Approve",
|
judgment belongs to the **author** — escalate by requesting the
|
||||||
"I agree with everything", a leading ✅ — as an approval, bound to the
|
maintainer's review (step 6), and the reconciler flips the label on that
|
||||||
current head SHA.)
|
request, because an explicit request is a fact it can trust.
|
||||||
6. **When all three approve**, the final review goes to the maintainer — the
|
6. **When the round passes, the author hands the PR to the maintainer** by
|
||||||
labels workflow requests it automatically.
|
requesting their review — that request is what flips `state:needs-human`.
|
||||||
|
With three formal head-current approvals the labels workflow requests it
|
||||||
|
automatically; when part of the panel is comment-only, reading their
|
||||||
|
agreement is the author's judgment, so the author makes the request.
|
||||||
7. **Checks must be green**: `shellcheck` and `bash test/cli.sh` locally
|
7. **Checks must be green**: `shellcheck` and `bash test/cli.sh` locally
|
||||||
mirror what CI runs; the multi-user rehearsal runs in CI on a real Incus.
|
mirror what CI runs; the multi-user rehearsal runs in CI on a real Incus.
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -17,7 +17,7 @@ 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 | all three bots approve | merged — or changes requested, which cycles back to `state:addressing` |
|
| `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` |
|
||||||
|
|
||||||
`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
|
||||||
|
|
|
||||||
|
|
@ -1,10 +1,10 @@
|
||||||
#!/usr/bin/env bash
|
#!/usr/bin/env bash
|
||||||
set -euo pipefail
|
set -euo pipefail
|
||||||
|
|
||||||
# Fixture tests for the labels-reconcile state machine — the transitions #85's
|
# Fixture tests for the labels-reconcile state machine: a comment is a
|
||||||
# review demanded proof of: comment-only agreement closes the gate, a stale
|
# non-verdict whatever its body says (the AUTHOR escalates by requesting the
|
||||||
# approval does not promote unreviewed code, a comment without a verdict parks
|
# human), a stale approval does not promote unreviewed code, and an explicit
|
||||||
# the PR on the agent, and an explicit human request outranks everything.
|
# human request outranks everything.
|
||||||
# Dependency-free beyond jq; no network, no daemon — pure decide_state.
|
# Dependency-free beyond jq; no network, no daemon — pure decide_state.
|
||||||
|
|
||||||
cd "$(dirname "$0")/.."
|
cd "$(dirname "$0")/.."
|
||||||
|
|
@ -49,12 +49,23 @@ REQUESTED="" REVIEWS_JSON="$(reviews \
|
||||||
"$(rev "$BOT2" APPROVED head1 "" t2)")"
|
"$(rev "$BOT2" APPROVED head1 "" t2)")"
|
||||||
expect "missing bot review means bots-reviewing" state:bots-reviewing "$(decide_state)"
|
expect "missing bot review means bots-reviewing" state:bots-reviewing "$(decide_state)"
|
||||||
|
|
||||||
# -- comment-only agreement closes the gate (the #85 blocker) -----------------
|
# -- a comment is a non-verdict, agreement body or not: the author escalates --
|
||||||
REVIEWS_JSON="$(reviews \
|
REVIEWS_JSON="$(reviews \
|
||||||
"$(rev "$BOT1" COMMENTED head1 "✅ **Reviewed — I agree with everything.**" t1)" \
|
"$(rev "$BOT1" COMMENTED head1 "✅ **Reviewed — I agree with everything.**" t1)" \
|
||||||
"$(rev "$BOT2" APPROVED head1 "Verdict: I agree with everything and have no additional feedback." t2)" \
|
"$(rev "$BOT2" APPROVED head1 "" t2)" \
|
||||||
"$(rev "$BOT3" COMMENTED head1 "**Verdict: Approve** — I agree with this as-is." t3)")"
|
"$(rev "$BOT3" APPROVED head1 "" t3)")"
|
||||||
expect "comment-only agreement counts as approval" state:needs-human "$(decide_state)"
|
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)"
|
||||||
|
|
||||||
# -- a comment WITHOUT a verdict parks the PR on the agent --------------------
|
# -- a comment WITHOUT a verdict parks the PR on the agent --------------------
|
||||||
REVIEWS_JSON="$(reviews \
|
REVIEWS_JSON="$(reviews \
|
||||||
|
|
@ -87,7 +98,7 @@ REVIEWS_JSON="$(reviews \
|
||||||
"$(rev "$BOT1" CHANGES_REQUESTED head1 "blockers" t1)" \
|
"$(rev "$BOT1" CHANGES_REQUESTED head1 "blockers" t1)" \
|
||||||
"$(rev "$BOT1" APPROVED head1 "" t2)" \
|
"$(rev "$BOT1" APPROVED head1 "" t2)" \
|
||||||
"$(rev "$BOT2" APPROVED head1 "" t3)" \
|
"$(rev "$BOT2" APPROVED head1 "" t3)" \
|
||||||
"$(rev "$BOT3" COMMENTED head1 "Verdict: Approve" t4)")"
|
"$(rev "$BOT3" APPROVED head1 "" t4)")"
|
||||||
expect "later approval supersedes earlier block" state:needs-human "$(decide_state)"
|
expect "later approval supersedes earlier block" state:needs-human "$(decide_state)"
|
||||||
|
|
||||||
# -- an explicit human request outranks the bot rounds ------------------------
|
# -- an explicit human request outranks the bot rounds ------------------------
|
||||||
|
|
@ -108,17 +119,5 @@ REQUESTED="$HUMAN"
|
||||||
expect "re-requested human is needs-human again" state:needs-human "$(decide_state)"
|
expect "re-requested human is needs-human again" state:needs-human "$(decide_state)"
|
||||||
REQUESTED=""
|
REQUESTED=""
|
||||||
|
|
||||||
# -- agreement_signal is conservative -----------------------------------------
|
|
||||||
if agreement_signal "I agree with most; feedback below"; then
|
|
||||||
fail=$((fail + 1)); echo "FAIL: 'agree with most' must NOT be agreement"
|
|
||||||
else
|
|
||||||
pass=$((pass + 1))
|
|
||||||
fi
|
|
||||||
if agreement_signal "**Verdict: Request changes** — blockers listed below."; then
|
|
||||||
fail=$((fail + 1)); echo "FAIL: 'Verdict: Request changes' must NOT be agreement"
|
|
||||||
else
|
|
||||||
pass=$((pass + 1))
|
|
||||||
fi
|
|
||||||
|
|
||||||
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