Merge pull request 'fix: read live review requests from each forge' (#249) from build/238-review-requests into main

Reviewed-on: heavy-duty/ceremony#249
Reviewed-by: glm-bot-andresmgsl <andres+5@heavyduty.builders>
Reviewed-by: kimi-bot-andresmgsl <andres+4@heavyduty.builders>
Reviewed-by: claude-bot-andresmgsl <andres+1@heavyduty.builders>

actions/labels-reconcile/labels-reconcile.sh

@@ -282,35 +282,6 @@ set_required_bots() { # the PR author is recused by construction
 
 requested() { grep -qxF "$1" <<<"$REQUESTED"; }
 
-# outstanding_requests <requested-logins> — the portable "who still owes a
-# verdict on THIS head" (issue #188, term 4).
-#
-# GitHub clears requested_reviewers when a verdict lands, so on that forge the
-# field already answers this question and the filter below removes nothing.
-# **Forgejo does not clear it.** Measured 2026-08-02: rig!140 listed all three
-# panelists with all three verdicts in, and rig!146 still lists three while
-# MERGED — the field is stale even on a closed PR, so it over-counts forever.
-#
-# Reading it raw on Forgejo pins a PR at state:bots-reviewing for life and
-# stops blocker:unrequested from ever being true: the sweep believes a round
-# is permanently live. So the requested set is intersected with "has not
-# submitted a verdict for the current head", which is derived from
-# /pulls/{n}/reviews — the read that is true on both forges.
-#
-# Pure over REVIEWS_JSON/HEAD_SHA so the fixtures can drive it; a reviewer
-# whose only verdict is STALE still owes one, which is why this asks
-# bot_verdict rather than merely "has any review".
-outstanding_requests() {
-  local login
-  while IFS= read -r login; do
-    [ -n "$login" ] || continue
-    case "$(bot_verdict "$login")" in
-      APPROVE | BLOCK | FEEDBACK) continue ;;
-    esac
-    printf '%s\n' "$login"
-  done <<<"${1-}"
-}
-
 checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE | UNREADABLE
   # UNREADABLE is the absence of the key itself, which is what a failed fetch
   # leaves behind — distinct from a present-but-empty rollup, which honestly
@@ -1070,10 +1041,10 @@ main() {
                                or .state == "REQUEST_CHANGES"
                                or .state == "COMMENTED"
                                or .state == "COMMENT")]')"
-      # Read AFTER the reviews, because the raw field is not portable: Forgejo
+      # Read AFTER the reviews: review_filter_probe captures REVIEWS_JSON at
-      # never clears it, so it is intersected with who still owes a verdict on
+      # this boundary. The request set itself comes from the backend's exact
-      # this head (#188 term 4). A no-op on GitHub, which clears it itself.
+      # live representation rather than being derived from verdicts (#238).
-      REQUESTED="$(outstanding_requests "$(jq -r '.requested_reviewers[].login' <<<"$PR_JSON")")"
+      REQUESTED="$(forge_pr_review_requests "$n")"
       # mergeability + the check rollup, the two facts the state machine was
       # blind to (#136). `gh pr view` rather than the REST PR object: the API's
       # `mergeable` is a tri-state boolean that GitHub computes lazily, while

changelog.d/238.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- Review-round state now reads each forge's live review-request set directly, so stale Forgejo approvals no longer hand an in-progress fix round back to the panel (#238).

lib/forge-forgejo.sh

@@ -452,6 +452,15 @@ forge_pr_view() {
     }'
 }
 
+# forge_pr_review_requests <n> — logins with a live review request.
+# Forgejo review.go deletes REQUEST_REVIEW rows when the reviewer submits any
+# review, so these rows are the exact live set rather than review history (#238).
+forge_pr_review_requests() {
+  local n="${1:?forge_pr_review_requests: number required}"
+  forge_api --paginate "repos/$REPO/pulls/$n/reviews" \
+    --jq '.[] | select(.state == "REQUEST_REVIEW") | .user.login' | sort -u
+}
+
 forge_label_list() { forge_api --paginate "repos/$REPO/labels" --jq '.[].name'; }
 
 # forge_label_create — an UPSERT, matching `gh label create --force` (#4743).

lib/forge-github.sh

@@ -101,6 +101,12 @@ forge_pr_view() {
   gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup
 }
 
+# forge_pr_review_requests <n> — logins with a live review request.
+forge_pr_review_requests() {
+  local n="${1:?forge_pr_review_requests: number required}"
+  forge_api "repos/$REPO/pulls/$n" --jq '.requested_reviewers[].login' | sort -u
+}
+
 # forge_label_list — every label name in the repo.
 forge_label_list() {
   gh label list -R "$REPO" --limit 200 --json name --jq '.[].name'

test/forge-backends.test.sh

@@ -581,6 +581,93 @@ check "...never deriving them from reviews, as forgejo must" 1 "" \
 unset -f gh
 . "$ROOT/lib/forge-forgejo.sh"
 
+# --- forge_pr_review_requests: the forge's live request set -------------
+# A stale verdict is not itself evidence that anybody was re-requested. The
+# backend reads the forge's request representation directly, so the state
+# machine can distinguish the builder's ball from the panel's (#238).
+review_requests_stub() {
+  fake_forge "$FAKE_REVIEWS_N" "$FAKE_REVIEWS"
+}
+
+# crew!96 after the builder pushed: two approvals and one requested change
+# belong to the older head, but no live REQUEST_REVIEW row exists.
+FAKE_REVIEWS_N=3
+FAKE_REVIEWS='[
+  {"user":{"login":"kimi-bot"},"state":"APPROVED","commit_id":"old","submitted_at":"2026-08-22T23:05:01Z"},
+  {"user":{"login":"glm-bot"},"state":"APPROVED","commit_id":"old","submitted_at":"2026-08-22T23:14:45Z"},
+  {"user":{"login":"claude-bot"},"state":"REQUEST_CHANGES","commit_id":"old","submitted_at":"2026-08-22T23:03:39Z"}
+]'
+review_requests_stub
+check "supersedes 'a stale approval still owes a verdict': one never re-requested is not requested" 0 "" \
+  eq "" forge_pr_review_requests 96
+
+# rig!146 after merge: all three panelists submitted, so the reviews endpoint
+# carries no REQUEST_REVIEW row even though requested_reviewers stayed stale.
+FAKE_REVIEWS_N=3
+FAKE_REVIEWS='[
+  {"user":{"login":"kimi-bot"},"state":"APPROVED","commit_id":"merged","submitted_at":"2026-08-02T14:05:01Z"},
+  {"user":{"login":"glm-bot"},"state":"APPROVED","commit_id":"merged","submitted_at":"2026-08-02T14:14:45Z"},
+  {"user":{"login":"claude-bot"},"state":"APPROVED","commit_id":"merged","submitted_at":"2026-08-02T14:03:39Z"}
+]'
+review_requests_stub
+check "supersedes 'the never-cleared forgejo field collapses to who actually owes': a merged PR yields nobody" 0 "" \
+  eq "" forge_pr_review_requests 146
+
+# crew!97's opening request rows. Deliberately unordered with one duplicate:
+# the public contract is sorted unique logins, independent of API row order.
+FAKE_REVIEWS_N=5
+FAKE_REVIEWS='[
+  {"user":{"login":"kimi-bot"},"state":"REQUEST_REVIEW","commit_id":"","submitted_at":"2026-08-22T22:51:12Z"},
+  {"user":{"login":"claude-bot"},"state":"REQUEST_REVIEW","commit_id":"","submitted_at":"2026-08-22T22:51:11Z"},
+  {"user":{"login":"codex-bot"},"state":"REQUEST_REVIEW","commit_id":"","submitted_at":"2026-08-22T22:51:11Z"},
+  {"user":{"login":"glm-bot"},"state":"REQUEST_REVIEW","commit_id":"","submitted_at":"2026-08-22T22:51:12Z"},
+  {"user":{"login":"kimi-bot"},"state":"REQUEST_REVIEW","commit_id":"","submitted_at":"2026-08-22T22:51:13Z"}
+]'
+review_requests_stub
+check "forgejo returns the four opening request logins sorted and deduplicated" 0 "" \
+  eq $'claude-bot\ncodex-bot\nglm-bot\nkimi-bot' forge_pr_review_requests 97
+
+FAKE_REVIEWS_N=2
+FAKE_REVIEWS='[
+  {"user":{"login":"claude-bot"},"state":"REQUEST_CHANGES","commit_id":"old","submitted_at":"2026-08-22T23:03:39Z"},
+  {"user":{"login":"claude-bot"},"state":"REQUEST_REVIEW","commit_id":"","submitted_at":"2026-08-22T23:16:17Z"}
+]'
+review_requests_stub
+check "supersedes: a reviewer re-requested while holding REQUEST_CHANGES is requested" 0 "" \
+  eq "claude-bot" forge_pr_review_requests 97
+
+FAKE_REVIEWS_N=1
+FAKE_REVIEWS='[{"user":{"login":"kimi-bot"},"state":"REQUEST_REVIEW","commit_id":"","submitted_at":"2026-08-22T22:51:12Z"}]'
+review_requests_stub
+check "supersedes 'a Forgejo request row is not an answer and leaves the login outstanding': its login is requested" 0 "" \
+  eq "kimi-bot" forge_pr_review_requests 97
+
+# GitHub already exposes the exact live set on the PR object. Stub only the
+# network boundary and assert the same backend-neutral output contract.
+. "$ROOT/lib/forge-github.sh"
+# shellcheck disable=SC2317 # invoked indirectly, by forge_api
+gh() {
+  [ "$1" = api ] || return 1
+  shift
+  local jqexpr="" endpoint=""
+  while [ $# -gt 0 ]; do
+    case "$1" in
+      --jq) jqexpr="$2"; shift ;;
+      -*) ;;
+      *) [ -n "$endpoint" ] || endpoint="$1" ;;
+    esac
+    shift
+  done
+  [ "$endpoint" = repos/o/r/pulls/55 ] || return 1
+  jq -r "$jqexpr" <<'JSON'
+{"requested_reviewers":[{"login":"glm-bot"},{"login":"claude-bot"}]}
+JSON
+}
+check "github returns requested_reviewers from the PR object unchanged" 0 "" \
+  eq $'claude-bot\nglm-bot' forge_pr_review_requests 55
+unset -f gh
+. "$ROOT/lib/forge-forgejo.sh"
+
 # --- forge_pr_view: newest verdict per context must win ------------------
 # checks_state groups repeated contexts and selects the newest by
 # [.startedAt, .createdAt, .completedAt]. Mapping only {context,state} left

test/labels-reconcile.test.sh

@@ -67,6 +67,19 @@ rev() { # $1=login $2=state $3=commit $4=body $5=submitted_at → one review obj
 
 reviews() { jq -s '.' <<<"$*"; } # collect review objects into an array
 
+# The approximation replaced by #238, retained only to make crew!96's
+# before/after regression explicit. Production reads the forge's live set.
+inferred_requests_before_238() {
+  local login
+  while IFS= read -r login; do
+    [ -n "$login" ] || continue
+    case "$(bot_verdict "$login")" in
+      APPROVE | BLOCK | FEEDBACK) continue ;;
+    esac
+    printf '%s\n' "$login"
+  done <<<"${1-}"
+}
+
 # The blocker:unrequested quiescence inputs (#236 D2). Every fixture below
 # inherits a readable, settled world — a head commit an hour before this
 # sweep's clock — so the cases written before #236 assert exactly what they
@@ -172,20 +185,39 @@ $BOT2
 $BOT3" REVIEWS_JSON='[]'
 expect "requested bots mean bots-reviewing" state:bots-reviewing "$(decide_state)"
 
-# Forgejo materializes each live request as a REQUEST_REVIEW row. Those rows
+# Forgejo materializes each live request as a REQUEST_REVIEW row. The backend
-# are not submitted verdicts (#235): they must leave all three logins
+# returns those three logins directly; the rows are not submitted verdicts
-# outstanding, so an opening round stays with the panel rather than falling
+# (#235), so an opening round stays with the panel rather than falling through
-# through to the builder as three comment-only answers.
+# to the builder as three comment-only answers.
 REVIEWS_JSON="$(reviews \
   "$(rev "$BOT1" REQUEST_REVIEW "" "" t1)" \
   "$(rev "$BOT2" REQUEST_REVIEW "" "" t2)" \
   "$(rev "$BOT3" REQUEST_REVIEW "" "" t3)")"
-REQUESTED="$(outstanding_requests "$BOT1
+REQUESTED="$BOT1
 $BOT2
-$BOT3" 2>"$RTMP/request-round-log")"
+$BOT3"
 expect "three Forgejo request rows keep the opening round with the panel" \
   state:bots-reviewing "$(round_state)"
 
+# crew!96 after a fix push: every submitted verdict belongs to the older head,
+# and the forge carries no live request row. The old intersection mistakes the
+# two stale approvals for requests; the exact empty set correctly gives the
+# incomplete round back to the builder (#238).
+HEAD_SHA=head2
+REVIEWS_JSON="$(reviews \
+  "$(rev "$BOT1" APPROVED head1 "" 2026-08-22T23:05:01Z)" \
+  "$(rev "$BOT2" APPROVED head1 "" 2026-08-22T23:14:45Z)" \
+  "$(rev "$BOT3" REQUEST_CHANGES head1 "" 2026-08-22T23:03:39Z)")"
+REQUESTED="$(inferred_requests_before_238 "$BOT1
+$BOT2
+$BOT3")"
+expect "crew!96 old inferred requests hand the fix round to the panel" \
+  state:bots-reviewing "$(round_state)"
+REQUESTED=""
+expect "crew!96 exact empty requests return the fix round to the builder" \
+  state:addressing "$(round_state)"
+HEAD_SHA=head1
+
 # -- a bot that never reviewed keeps the round open ---------------------------
 #    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.
@@ -1640,8 +1672,8 @@ expect "...naming the attempt that did not happen" \
   yes "$(grep -q 'label edit FAILED' <<<"$sf_out" && echo yes || echo no)"
 
 # Drive the ingestion expression through main(), independently of
-# bot_verdict (#235). Capturing REVIEWS_JSON at the outstanding_requests
+# bot_verdict (#235). Capturing REVIEWS_JSON at the review-request read that
-# boundary proves REQUEST_REVIEW never reaches the grader; the COMMENT and
+# follows it proves REQUEST_REVIEW never reaches the grader; the COMMENT and
 # APPROVED controls prove both gradeable Forgejo states and rows generally
 # survive the filter.
 review_filter_probe() {
@@ -1671,8 +1703,12 @@ review_filter_probe() {
         *) printf '[]\n' ;;
       esac
     }
+    # main normally re-sources the selected backend. This probe already runs
+    # with the GitHub backend selected at suite startup; keeping that selection
+    # stable lets the verb override below observe the post-REVIEWS_JSON boundary.
+    forge_select() { return 0; }
     # shellcheck disable=SC2317 # main invokes the probe override indirectly
-    outstanding_requests() {
+    forge_pr_review_requests() {
       printf '%s\n' "$REVIEWS_JSON" >"$RTMP/gradeable-reviews.json"
     }
     main >/dev/null
@@ -1682,59 +1718,9 @@ review_filter_probe
 expect "REQUEST_REVIEW is removed before REVIEWS_JSON reaches the grader" \
   COMMENT,APPROVED "$(jq -r 'map(.state) | join(",")' "$RTMP/gradeable-reviews.json")"
 
-# ---------------------------------------------------------------------------
-# outstanding_requests — the portable "who still owes a verdict" (#188 term 4)
-#
-# GitHub clears requested_reviewers when a verdict lands; Forgejo never does.
-# Measured 2026-08-02: rig!140 listed all three panelists with all three
-# verdicts in, and rig!146 still lists three while MERGED. Read raw on
-# Forgejo, that pins a PR at state:bots-reviewing for life and stops
-# blocker:unrequested from ever being true.
-# ---------------------------------------------------------------------------
-HEAD_SHA=head1
-REVIEWS_JSON="$(reviews \
-  "$(rev "$BOT1" APPROVED head1 "" 2026-08-01T00:00:00Z)" \
-  "$(rev "$BOT2" CHANGES_REQUESTED head1 "" 2026-08-01T00:00:00Z)" \
-  "$(rev "$BOT3" APPROVED head0 "" 2026-07-01T00:00:00Z)")"
-
-expect "a head-current approval is no longer outstanding" "" \
-  "$(outstanding_requests "$BOT1")"
-expect "a blocking verdict is not outstanding either — it is answered" "" \
-  "$(outstanding_requests "$BOT2")"
-# The one that matters: an approval of an OLDER head is not a verdict on this
-# head, so that reviewer still owes one. Treating STALE as answered would let
-# a stale round read as complete.
-expect "a stale approval still owes a verdict" "$BOT3" \
-  "$(outstanding_requests "$BOT3")"
-expect "a reviewer who never reviewed still owes one" "nobody" \
-  "$(outstanding_requests "nobody")"
-
-REVIEWS_JSON="$(reviews \
-  "$(rev "$BOT1" REQUEST_REVIEW "" "" 2026-08-22T00:46:05Z)")"
-expect "a Forgejo request row is not an answer and leaves the login outstanding" \
-  "$BOT1" "$(outstanding_requests "$BOT1" 2>"$RTMP/request-outstanding-log")"
-REVIEWS_JSON="$(reviews \
-  "$(rev "$BOT1" APPROVED head1 "" 2026-08-01T00:00:00Z)" \
-  "$(rev "$BOT2" CHANGES_REQUESTED head1 "" 2026-08-01T00:00:00Z)" \
-  "$(rev "$BOT3" APPROVED head0 "" 2026-07-01T00:00:00Z)")"
-
-# The Forgejo shape, end to end: the field lists all three long after every
-# verdict landed. Only the stale one may survive the filter.
-expect "the never-cleared forgejo field collapses to who actually owes" \
-  "$BOT3" "$(outstanding_requests "$BOT1
-$BOT2
-$BOT3")"
-
-# The GitHub shape: the field is already accurate, so the filter is a no-op
-# on the set GitHub would have produced (term 5 — behaviour unchanged).
-expect "on a github-shaped field the filter removes nothing" "nobody" \
-  "$(outstanding_requests "nobody")"
-expect "an empty request list stays empty" "" "$(outstanding_requests "")"
-
 # The summary and the gate belong at the TRUE end of the file. They sat in the
-# middle until #188: eight outstanding_requests expects were appended after
+# middle until #188, so a later failure printed FAIL, was left out of the
-# them, so a failure there printed FAIL, was left out of the totals, and the
+# totals, and the suite still exited 0 (@codex-reviewer-andresmgsl #4780 item
-# suite still exited 0 (@codex-reviewer-andresmgsl #4780 item 2). Anything
+# 2). Anything appended below this line is ungated — so nothing goes below it.
-# appended below this line is ungated — so nothing goes below it.
 printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail"
 [ "$fail" -eq 0 ]