diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 38de0b9..4a225fd 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -10,13 +10,15 @@ set -euo pipefail # own mark every tick. # # The verdict contract (CONTRIBUTING.md): reviews end in approve or -# request-changes. Reality check (#85 round 1): at least one live bot posts -# its agreement as a COMMENTED review and can never formally approve, which -# would park every fully-agreed PR in state:addressing forever. So COMMENTED -# reviews whose body carries a durable agreement signal count as approval — -# the workaround the state machine owes the fleet until every bot speaks the -# formal contract. Any verdict that counts toward needs-human must be bound -# to the CURRENT head SHA: GitHub keeps approvals alive across pushes, and a +# request-changes. Some live bots are comment-only and post agreement as a +# COMMENTED review — a non-verdict this machine refuses to guess about (body +# parsing is a heuristic, and a wrong guess promotes an unapproved PR). The +# judgment call belongs to the PR AUTHOR, who reads the round and escalates +# by requesting the human's review — an explicit request is a fact, and it is +# the one this machine trusts (see decide_state's top precedence). The +# 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. # # 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"; } -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 - local review state commit body + local review state commit review="$(jq -c --arg u "$1" \ '[.[] | select(.user.login == $u)] | sort_by(.submitted_at) | last // empty' \ <<<"$REVIEWS_JSON")" if [ -z "$review" ]; then echo MISSING; return; fi state="$(jq -r '.state' <<<"$review")" commit="$(jq -r '.commit_id' <<<"$review")" - body="$(jq -r '.body // ""' <<<"$review")" case "$state" in CHANGES_REQUESTED) # 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 ;; APPROVED) if [ "$commit" = "$HEAD_SHA" ]; then echo APPROVE; else echo STALE; fi ;; - COMMENTED) - if agreement_signal "$body"; then - if [ "$commit" = "$HEAD_SHA" ]; then echo APPROVE; else echo STALE; fi - else - echo FEEDBACK - fi ;; - *) echo FEEDBACK ;; + *) + # COMMENTED and anything else: a non-verdict. The machine does not + # read bodies — if the comment is really an agreement, the AUTHOR + # says so by requesting the human's review. + echo FEEDBACK ;; esac } @@ -143,9 +135,11 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch desired="$(decide_state)" - # encode the runbook's last step: the round passed → the human is asked, - # once. The guard (never requested, never reviewed) makes it idempotent — - # and the shared concurrency group in labels.yml makes it race-free. + # encode the runbook's last step for the no-judgment case: three formal + # head-current approvals → the human is asked, once. The guard (never + # 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" \ && [ -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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f080fdf..2013c37 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,13 +28,16 @@ labels tell you where everything is without opening anything. them at their discretion; anything blocking — including a question that gates the verdict — is **request changes**, saying what unblocks it. The reconciler treats a comment-only review as not-approved, so commenting - without a verdict only stalls the PR. (Transitional workaround: until - every bot speaks the formal contract, the reconciler counts a COMMENTED - review whose body carries a durable agreement signal — "Verdict: Approve", - "I agree with everything", a leading ✅ — as an approval, bound to the - current head SHA.) -6. **When all three approve**, the final review goes to the maintainer — the - labels workflow requests it automatically. + without a verdict only stalls the PR. The machine never reads review + bodies: when a comment-only reviewer's line is really an agreement, that + judgment belongs to the **author** — escalate by requesting the + maintainer's review (step 6), and the reconciler flips the label on that + request, because an explicit request is a fact it can trust. +6. **When the round passes, the author hands the PR to the maintainer** by + 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 mirror what CI runs; the multi-user rehearsal runs in CI on a real Incus. diff --git a/LABELS.md b/LABELS.md index 24e78b6..612b38c 100644 --- a/LABELS.md +++ b/LABELS.md @@ -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:bots-reviewing` | `#1D76DB` | the reviewer bots to finish the round | ready with reviews requested, or fixes pushed and reviews re-requested | all three bots have reviewed the round | | `state:addressing` | `#D93F0B` | the coding agent to reply and push fixes | all bots reviewed the round, not all approved | the single round-reply is posted and fixes pushed | -| `state:needs-human` | `#8250DF` | the human reviewer | 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 first means *poke the bots*, staleness in the second means *the agent dropped diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 13a5e5f..643546b 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -1,10 +1,10 @@ #!/usr/bin/env bash set -euo pipefail -# Fixture tests for the labels-reconcile state machine — the transitions #85's -# review demanded proof of: comment-only agreement closes the gate, a stale -# approval does not promote unreviewed code, a comment without a verdict parks -# the PR on the agent, and an explicit human request outranks everything. +# 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. # Dependency-free beyond jq; no network, no daemon — pure decide_state. cd "$(dirname "$0")/.." @@ -49,12 +49,23 @@ REQUESTED="" REVIEWS_JSON="$(reviews \ "$(rev "$BOT2" APPROVED head1 "" t2)")" 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 \ "$(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 "$BOT3" COMMENTED head1 "**Verdict: Approve** — I agree with this as-is." t3)")" -expect "comment-only agreement counts as approval" state:needs-human "$(decide_state)" + "$(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)" # -- a comment WITHOUT a verdict parks the PR on the agent -------------------- REVIEWS_JSON="$(reviews \ @@ -87,7 +98,7 @@ REVIEWS_JSON="$(reviews \ "$(rev "$BOT1" CHANGES_REQUESTED head1 "blockers" t1)" \ "$(rev "$BOT1" APPROVED head1 "" t2)" \ "$(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)" # -- 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)" 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" [ "$fail" -eq 0 ]