forked from heavy-duty/ceremony
fix: drive round state from live review requests
This commit is contained in:
parent
0160f6a883
commit
6f5ce8f994
2 changed files with 52 additions and 95 deletions
|
|
@ -282,35 +282,6 @@ set_required_bots() { # the PR author is recused by construction
|
||||||
|
|
||||||
requested() { grep -qxF "$1" <<<"$REQUESTED"; }
|
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
|
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
|
# 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
|
# leaves behind — distinct from a present-but-empty rollup, which honestly
|
||||||
|
|
@ -1070,10 +1041,10 @@ main() {
|
||||||
or .state == "REQUEST_CHANGES"
|
or .state == "REQUEST_CHANGES"
|
||||||
or .state == "COMMENTED"
|
or .state == "COMMENTED"
|
||||||
or .state == "COMMENT")]')"
|
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
|
# 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
|
# 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
|
# `mergeable` is a tri-state boolean that GitHub computes lazily, while
|
||||||
|
|
|
||||||
|
|
@ -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
|
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
|
# The blocker:unrequested quiescence inputs (#236 D2). Every fixture below
|
||||||
# inherits a readable, settled world — a head commit an hour before this
|
# 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
|
# sweep's clock — so the cases written before #236 assert exactly what they
|
||||||
|
|
@ -172,20 +185,39 @@ $BOT2
|
||||||
$BOT3" REVIEWS_JSON='[]'
|
$BOT3" REVIEWS_JSON='[]'
|
||||||
expect "requested bots mean bots-reviewing" state:bots-reviewing "$(decide_state)"
|
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 \
|
REVIEWS_JSON="$(reviews \
|
||||||
"$(rev "$BOT1" REQUEST_REVIEW "" "" t1)" \
|
"$(rev "$BOT1" REQUEST_REVIEW "" "" t1)" \
|
||||||
"$(rev "$BOT2" REQUEST_REVIEW "" "" t2)" \
|
"$(rev "$BOT2" REQUEST_REVIEW "" "" t2)" \
|
||||||
"$(rev "$BOT3" REQUEST_REVIEW "" "" t3)")"
|
"$(rev "$BOT3" REQUEST_REVIEW "" "" t3)")"
|
||||||
REQUESTED="$(outstanding_requests "$BOT1
|
REQUESTED="$BOT1
|
||||||
$BOT2
|
$BOT2
|
||||||
$BOT3" 2>"$RTMP/request-round-log")"
|
$BOT3"
|
||||||
expect "three Forgejo request rows keep the opening round with the panel" \
|
expect "three Forgejo request rows keep the opening round with the panel" \
|
||||||
state:bots-reviewing "$(round_state)"
|
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 ---------------------------
|
# -- a bot that never reviewed keeps the round open ---------------------------
|
||||||
# With a live request that is the bots' ball; with NO request outstanding it
|
# 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.
|
# 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)"
|
yes "$(grep -q 'label edit FAILED' <<<"$sf_out" && echo yes || echo no)"
|
||||||
|
|
||||||
# Drive the ingestion expression through main(), independently of
|
# 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
|
# APPROVED controls prove both gradeable Forgejo states and rows generally
|
||||||
# survive the filter.
|
# survive the filter.
|
||||||
review_filter_probe() {
|
review_filter_probe() {
|
||||||
|
|
@ -1671,8 +1703,12 @@ review_filter_probe() {
|
||||||
*) printf '[]\n' ;;
|
*) printf '[]\n' ;;
|
||||||
esac
|
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
|
# shellcheck disable=SC2317 # main invokes the probe override indirectly
|
||||||
outstanding_requests() {
|
forge_pr_review_requests() {
|
||||||
printf '%s\n' "$REVIEWS_JSON" >"$RTMP/gradeable-reviews.json"
|
printf '%s\n' "$REVIEWS_JSON" >"$RTMP/gradeable-reviews.json"
|
||||||
}
|
}
|
||||||
main >/dev/null
|
main >/dev/null
|
||||||
|
|
@ -1682,59 +1718,9 @@ review_filter_probe
|
||||||
expect "REQUEST_REVIEW is removed before REVIEWS_JSON reaches the grader" \
|
expect "REQUEST_REVIEW is removed before REVIEWS_JSON reaches the grader" \
|
||||||
COMMENT,APPROVED "$(jq -r 'map(.state) | join(",")' "$RTMP/gradeable-reviews.json")"
|
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
|
# 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"
|
printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail"
|
||||||
[ "$fail" -eq 0 ]
|
[ "$fail" -eq 0 ]
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue