diff --git a/actions/labels-reconcile/labels-reconcile.sh b/actions/labels-reconcile/labels-reconcile.sh index 9e4ff13..a8e2c26 100755 --- a/actions/labels-reconcile/labels-reconcile.sh +++ b/actions/labels-reconcile/labels-reconcile.sh @@ -187,6 +187,35 @@ set_required_bots() { # the PR author is recused by construction requested() { grep -qxF "$1" <<<"$REQUESTED"; } +# outstanding_requests — 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 @@ -752,10 +781,13 @@ main() { HEAD_SHA="$(jq -r '.head.sha' <<<"$PR_JSON")" BASE_SHA="$(jq -r '.base.sha' <<<"$PR_JSON")" LABELS="$(jq -r '.labels[].name' <<<"$PR_JSON")" - REQUESTED="$(jq -r '.requested_reviewers[].login' <<<"$PR_JSON")" # PENDING reviews are unsubmitted drafts in someone's browser — not a verdict REVIEWS_JSON="$(forge_api --paginate "repos/$REPO/pulls/$n/reviews" --jq '.[]' \ | jq -s '[.[] | select(.state != "PENDING")]')" + # Read AFTER the reviews, because the raw field is not portable: Forgejo + # never clears it, so it is intersected with who still owes a verdict on + # this head (#188 term 4). A no-op on GitHub, which clears it itself. + REQUESTED="$(outstanding_requests "$(jq -r '.requested_reviewers[].login' <<<"$PR_JSON")")" # 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 diff --git a/changelog.d/188.md b/changelog.d/188.md index a3770d4..ff19014 100644 --- a/changelog.d/188.md +++ b/changelog.d/188.md @@ -28,6 +28,10 @@ other's parameter: `per_page=100` reads 30 items on Forgejo and `limit=100` reads 30 on GitHub, both HTTP 200. No call site names one (#188). +- Outstanding review requests are derived from the reviews on the current head + rather than from `requested_reviewers`, which Forgejo never clears — read + raw there, a PR would sit at `state:bots-reviewing` forever (#188). + ### Fixed - `labels-reconcile` and `labels-scope` no longer exit 0 on a Forgejo diff --git a/docs/CONSUMERS.md b/docs/CONSUMERS.md index eaac5c1..672f09f 100644 --- a/docs/CONSUMERS.md +++ b/docs/CONSUMERS.md @@ -396,6 +396,13 @@ never before it and never through mixed refs. Both actor lists are whitespace-separated. `triage-actors` names the identities allowed to mint issues without the sweep applying `needs-triage`. Label rows use exactly `name|color|description`; blank lines are ignored and extra pipes are refused. + +**Every account in `panel=` must be able to read the repository.** Requesting a +review from someone without read access is refused by the forge, not silently +dropped — on Forgejo with `422 Reviewer can't read`, naming the account +(#188). On a public repo this is satisfied already; on a **private** consumer +it is a real failure mode when a panel member is not on the collaborator +list, and the sweep will report it rather than sweep blind. There are no comment lines: every non-blank line must be the `panel=` setting, the `triage-actors=` setting, or a label row, so `#`-prefixed prose is a parse failure, not a comment (rig #13's conversion found this the hard diff --git a/test/labels-reconcile.test.sh b/test/labels-reconcile.test.sh index 20e45e3..50e0448 100755 --- a/test/labels-reconcile.test.sh +++ b/test/labels-reconcile.test.sh @@ -970,3 +970,43 @@ for ev in schedule pull_request_target; do done printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ] + +# --------------------------------------------------------------------------- +# 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")" + +# 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 "")"