From 0160f6a8831cfb3eacc4ff08b6ca486db95a961b Mon Sep 17 00:00:00 2001 From: codex-bot-andresmgsl Date: Mon, 24 Aug 2026 12:38:03 +0000 Subject: [PATCH 1/5] fix: read live review requests from each forge --- lib/forge-forgejo.sh | 9 +++++ lib/forge-github.sh | 6 ++++ test/forge-backends.test.sh | 70 +++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+) diff --git a/lib/forge-forgejo.sh b/lib/forge-forgejo.sh index cfdf887..1b05275 100644 --- a/lib/forge-forgejo.sh +++ b/lib/forge-forgejo.sh @@ -452,6 +452,15 @@ forge_pr_view() { }' } +# forge_pr_review_requests — 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). diff --git a/lib/forge-github.sh b/lib/forge-github.sh index c2e268e..7a088ed 100644 --- a/lib/forge-github.sh +++ b/lib/forge-github.sh @@ -101,6 +101,12 @@ forge_pr_view() { gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup } +# forge_pr_review_requests — 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' diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index fd2c1c7..8349708 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -581,6 +581,76 @@ 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 approver who was never re-requested is not requested" 0 "" \ + eq "" forge_pr_review_requests 96 +check "supersedes: a merged PR whose requested_reviewers still lists the whole panel 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 login whose only row is REQUEST_REVIEW 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() { + case "$1 $2" in + "api repos/o/r/pulls/55") + jq -r '.requested_reviewers[].login' <<'JSON' +{"requested_reviewers":[{"login":"glm-bot"},{"login":"claude-bot"}]} +JSON + ;; + *) return 1 ;; + esac +} +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 -- 2.45.2 From 6f5ce8f99407341f6dffc384d26b84b8ed0c7e9c Mon Sep 17 00:00:00 2001 From: codex-bot-andresmgsl Date: Mon, 24 Aug 2026 12:42:11 +0000 Subject: [PATCH 2/5] fix: drive round state from live review requests --- actions/labels-reconcile/labels-reconcile.sh | 37 +------ test/labels-reconcile.test.sh | 110 ++++++++----------- 2 files changed, 52 insertions(+), 95 deletions(-) diff --git a/actions/labels-reconcile/labels-reconcile.sh b/actions/labels-reconcile/labels-reconcile.sh index e154f13..bb792c4 100755 --- a/actions/labels-reconcile/labels-reconcile.sh +++ b/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 — 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 - # 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")")" + # Read AFTER the reviews: review_filter_probe captures REVIEWS_JSON at + # this boundary. The request set itself comes from the backend's exact + # live representation rather than being derived from verdicts (#238). + 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 diff --git a/test/labels-reconcile.test.sh b/test/labels-reconcile.test.sh index aaf18e8..c759891 100755 --- a/test/labels-reconcile.test.sh +++ b/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 -# are not submitted verdicts (#235): they must leave all three logins -# outstanding, so an opening round stays with the panel rather than falling -# through to the builder as three comment-only answers. +# Forgejo materializes each live request as a REQUEST_REVIEW row. The backend +# returns those three logins directly; the rows are not submitted verdicts +# (#235), so an opening round stays with the panel rather than falling through +# 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 -# boundary proves REQUEST_REVIEW never reaches the grader; the COMMENT and +# bot_verdict (#235). Capturing REVIEWS_JSON at the review-request read that +# 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 -# them, so a failure there printed FAIL, was left out of the totals, and the -# suite still exited 0 (@codex-reviewer-andresmgsl #4780 item 2). Anything -# appended below this line is ungated — so nothing goes below it. +# middle until #188, so a later failure printed FAIL, was left out of the +# totals, and the suite still exited 0 (@codex-reviewer-andresmgsl #4780 item +# 2). Anything 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 ] -- 2.45.2 From f05e83a562c3c083dfb4683fb772fa3caf7a79a9 Mon Sep 17 00:00:00 2001 From: codex-bot-andresmgsl Date: Mon, 24 Aug 2026 12:43:33 +0000 Subject: [PATCH 3/5] docs: record live review request fix --- changelog.d/238.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/238.md diff --git a/changelog.d/238.md b/changelog.d/238.md new file mode 100644 index 0000000..26522ef --- /dev/null +++ b/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). -- 2.45.2 From 22865aba5430f5a4263a33efba1f4c8a34838bd5 Mon Sep 17 00:00:00 2001 From: codex-bot-andresmgsl Date: Mon, 24 Aug 2026 12:46:42 +0000 Subject: [PATCH 4/5] test: preserve superseded request question names --- test/forge-backends.test.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index 8349708..e34bfc1 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -598,9 +598,9 @@ FAKE_REVIEWS='[ {"user":{"login":"claude-bot"},"state":"REQUEST_CHANGES","commit_id":"old","submitted_at":"2026-08-22T23:03:39Z"} ]' review_requests_stub -check "supersedes: a stale approver who was never re-requested is not requested" 0 "" \ +check "supersedes 'a stale approval still owes a verdict': one never re-requested is not requested" 0 "" \ eq "" forge_pr_review_requests 96 -check "supersedes: a merged PR whose requested_reviewers still lists the whole panel yields nobody" 0 "" \ +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: @@ -629,7 +629,7 @@ check "supersedes: a reviewer re-requested while holding REQUEST_CHANGES is requ 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 login whose only row is REQUEST_REVIEW is requested" 0 "" \ +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 -- 2.45.2 From f0f39076187dbaf827b09397422fd60095a3b4c8 Mon Sep 17 00:00:00 2001 From: codex-bot-andresmgsl Date: Mon, 24 Aug 2026 14:42:36 +0000 Subject: [PATCH 5/5] test: exercise GitHub review-request selector --- test/forge-backends.test.sh | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index e34bfc1..ad1bd65 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -600,6 +600,16 @@ FAKE_REVIEWS='[ 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 @@ -637,14 +647,21 @@ check "supersedes 'a Forgejo request row is not an answer and leaves the login o . "$ROOT/lib/forge-github.sh" # shellcheck disable=SC2317 # invoked indirectly, by forge_api gh() { - case "$1 $2" in - "api repos/o/r/pulls/55") - jq -r '.requested_reviewers[].login' <<'JSON' + [ "$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 - ;; - *) return 1 ;; - esac } check "github returns requested_reviewers from the PR object unchanged" 0 "" \ eq $'claude-bot\nglm-bot' forge_pr_review_requests 55 -- 2.45.2