fix: read live review requests from each forge #249
No reviewers
glm-bot-andresmgsl
kimi-bot-andresmgsl
claude-bot-andresmgsl
andres
Labels
No labels
attention
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-ruling
needs-triage
offsite
post-merge
ready
release
scope:docs
scope:guards
scope:labels
scope:release-flow
stale
state:addressing
state:bots-reviewing
state:building
state:needs-human
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: heavy-duty/ceremony#249
Loading…
Reference in a new issue
No description provided.
Delete branch "build/238-review-requests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #238
Acceptance criteria
forge_pr_review_requestson Forgejo returns exactly liveREQUEST_REVIEWlogins, including empty crew!96 and four-login crew!97 shapes..requested_reviewers[].loginfrom the PR object.REQUESTEDyieldsstate:addressingwhile the old inference yieldsstate:bots-reviewing.outstanding_requestshas no remaining production occurrence.Worklog
Add red-first backend tests for both live-request representations.
Implement
forge_pr_review_requestsin both backends and verify 134 backend assertions.Add the crew!96 before/after round-state fixture and preserve the opening-round assertion.
Re-point the review-filter probe and replace the reconciler input with the exact request verb.
Delete
outstanding_requestsand its superseded assertion block without weakening unrelated coverage.Add
changelog.d/238.md.Run targeted tests, full suite, sanctioned chunked shellcheck, and diff hygiene.
Round fix: make the GitHub stub execute the production
--jqexpression.Round fix: make the merged shape explicit; keep the lower-level failure coverage within the fixed 134-assertion contract.
Round fix: run targeted/full verification, push, and answer the round whole.
Test record
Baseline:
bash test/run.sh— all 31 test files passed.Backend RED: 128 passed, 6 failed because
forge_pr_review_requestswas undefined.Backend GREEN at
0160f6a: 134 passed, 0 failed.Probe RED: 259 passed, 1 failed because
main()still called the old inference.Reconciler GREEN at
6f5ce8f: 252 passed, 0 failed; backend remains 134 passed, 0 failed.Pure
round_stateneeded no change: crew!96 isbots-reviewingwith the old inferred set andaddressingwith the exact empty set.Final verification at
22865aba5430f5a4263a33efba1f4c8a34838bd5: 31/31 test files passed; shellcheck linted 64 tracked scripts with exit 0; diff check and worktree clean; local, tracking, and PR heads match.Round log
f0f39076187dbaf827b09397422fd60095a3b4c8: bad-selector mutation 133 passed / 1 failed; restored backend 134 / 0; full suite 31/31 files; sanctioned shellcheck and actionlint exit 0; diff clean; worktree clean; local and remote heads match.Round at
22865aba🔧 addressing round on head
22865aba54⟲ resuming from
22865aba54Round analysis:
ghstub ignores the production--jq: agree. I will make the stub parse and execute the passed jq expression against a PR-object fixture, then prove a bad production expression fails the test.<n>verb and the sweep self-corrects; changing the interface would exceed #238.review_filter_probeneutralizesforge_select, reducing that probe's selection coverage: agree, no change. The existing comment records why this probe must isolate the post-selection review-filter boundary, while selection remains covered elsewhere.I will append these fix steps to the PR Worklog before editing code, complete and push the round as a whole, reply with changes and verification, then signal the final head while still draft and mark it ready with no intervening commit.
Round answered whole for head
22865aba5430f5a4263a33efba1f4c8a34838bd5; fixes are pushed atf0f39076187dbaf827b09397422fd60095a3b4c8.ghstub now parses the production--jqargument and applies it to a complete PR-object fixture. With the production selector mutated to.this_key_does_not_exist[].login, the backend suite now fails at 133 passed / 1 failed; restoring.requested_reviewers[].loginreturns 134 / 0.requested_reviewersfield remained populated while the reviews endpoint carried noREQUEST_REVIEWrow.<n>verb is #238's specified interface, the transient mismatch self-corrects on the next sweep, and changing the interface is outside this issue.review_filter_probeneutralizesforge_select: no code change. That probe intentionally isolates the post-selection review-filter boundary and already documents the neutralization; selection remains covered independently.Verification at
f0f39076187dbaf827b09397422fd60095a3b4c8:bash test/run.sh— 31 test files passed, 0 failed.bash test/forge-backends.test.sh— 134 passed, 0 failed..github/scripts/shellcheck-all.sh— exit 0..github/scripts/actionlint-all.sh— exit 0.git diff --check origin/main...HEAD— clean.📣 round answered at head
f0f3907618Round at
f0f39076Round passed with no written reply.
fix: read live review requests from each forgeto WIP: fix: read live review requests from each forge📣 round answered at head
22865aba54WIP: fix: read live review requests from each forgeto fix: read live review requests from each forge🔎 reviewing head
f0f3907618Round 2 complete. Last round's blocking item — the GitHub backend's
ghstubdiscarding the production
--jq— is fixed and mutation-verified: replacing.requested_reviewers[].loginwith a non-existent key now reds the suite(133/1), as does dropping
| sort -u. Forgejo half re-verified at this head.Full suite 29/31 files here (the two
yq-dependent files fail identically atthe merge base — my box, not the PR); sanctioned shellcheck rc 0; actionlint
absent here. Verdict submitted.
🔎 reviewing head
f0f3907618🔎 reviewing head
f0f3907618git diff --checkcleanApproved.
This PR correctly implements the #238 spec: it adds
forge_pr_review_requeststo both forge backends, deletes theoutstanding_requestsderivation fromactions/labels-reconcile/labels-reconcile.sh, and rewires the two test sites from #235 without weakening them.What I verified on the PR head (
22865aba5430f5a4263a33efba1f4c8a34838bd5):grep -R outstanding_requests --include='*.sh'returns no matches in the tree.test/forge-backends.test.sh: 134 passed, 0 failed. The new assertions cover the four superseded questions: stale-not-re-requested, re-requested-with-standing-block, merged-with-stale-field, and request-row-only, plus crew!97's sorted/deduplicated four-login set.test/labels-reconcile.test.sh: 252 passed, 0 failed. The crew!96 before/after fixture showsstate:bots-reviewingunder the old inferred request set andstate:addressingunder the exact empty set..github/scripts/shellcheck-all.sh: linted 64 tracked scripts with exit 0.What I could not verify locally:
test/run.shsuite: this box lacksnode/npm, so the package-json cases intest/version.test.sh,test/facts.test.sh,test/changelog-armed.test.sh, andtest/drill-recorded.test.shfail with "node is required". These are unrelated to the changed files and CI confirms they pass.Approve — verified, not opined. Head
22865aba54against issue #238.Acceptance criteria — all checked
REQUEST_REVIEWrows from/pulls/{n}/reviews,sort -u, paginated through the completeness-assertingforge_api --paginate(labels-reconcile.test.sh + forge-backends.test.sh). crew!96 payload (two APPROVED + one REQUEST_CHANGES, no rows) yields nothing; crew!97 opening payload (4 logins + a duplicate row) yields the four sorted-unique logins.repos/$REPO/pulls/$n --jq '.requested_reviewers[].login', asserted against a stubbed PR object through theghboundary.grep -c outstanding_requests actions/labels-reconcile/labels-reconcile.sh= 0 — run by me; no other tracked file references it.REQUESTED=assignment; the probe assertion byte-identical with the override re-pointed atforge_pr_review_requests(same post-REVIEWS_JSONboundary; the addedforge_select(){ return 0; }neutralizes main's backend re-source so the override survives — sound, since the probe already runs the GitHub backend); "a Forgejo request row is not an answer…" superseded by name at the verb; the other seven !244 assertions are untouched (verified against the68b304ddiff; main..head reaches7bdae45via changelog-only 246, so the baseline is clean).bash test/run.sh→ 31/31 files; sanctioned.github/scripts/shellcheck-all.sh→ 64 scripts, exit 0;actionlint-all.sh→ exit 0.Verification runs (what I ran)
main's libs → forge-backends 128 passed / 6 failed (verb unbound); labels-reconcile 251 passed / 1 failed (probe boundary), and the crew!96round_statepair green on main — exactly the issue's "red is at the verb, not at round_state".andres) — correct per the model and inert in production:forge_pr_listisstate=open, so round_state never reads a closed PR. Worth knowing, changes nothing.What I could not verify, and what I relied on instead
ghstub (the fleet is Forgejo-only); relied on the stub asserting the exact endpoint and jq expression.Non-blocking nits (builder's discretion)
mainnow fetches the reviews endpoint (Forgejo) / PR object (GitHub) a second time inside the verb afterREVIEWS_JSON/PR_JSON. Spec-mandated shape; cost only.Panel note: on
panel=per #224, minus the author — this is an on-panel verdict, not advisory.Request changes — one blocking item, in the GitHub backend's test rather than in its code.
The production change is right, and I verified the premise it rests on against this instance rather than taking it from the issue: on
heavy-duty/ceremony!244(merged)requested_reviewersstill lists all three panelists while/pulls/244/reviewscarries zeroREQUEST_REVIEWrows — the rig!146 shape, live. On this PR right now the same endpoint carries exactly threeREQUEST_REVIEWrows for the three requested logins. Rows appear on request and vanish on submit, soforge_pr_review_requestsreads the predicaterequested()actually needs.Blocking
The GitHub backend's test does not exercise the GitHub backend's
jq, so the criterion it is meant to settle is unsettled.test/forge-backends.test.sh:639-648'sghstub matches on"$1 $2"and hardcodesjq -r '.requested_reviewers[].login'over its fixture, discarding the--jqexpressionforge_apipasses it. The assertion therefore tests the stub's own expression, notlib/forge-github.sh:106's.Measured, at this head — I replaced the production expression with a path that cannot exist:
Green on a backend that returns nothing for every PR. That is the acceptance criterion "the same verb on the GitHub backend returns
.requested_reviewers[].loginunchanged, asserted against a stubbed PR object" — the stub is a stubbed result, not a stubbed PR object, and nothing asserts the read. The Forgejo half has no such hole: mutatingselect(.state == "REQUEST_REVIEW")to"PENDING"reds three checks, and dropping| sort -ureds one. It is only the GitHub twin that is unguarded — and that is the twin no runner on this fleet ever exercises, so this test is its whole defence. An emptyREQUESTEDon every PR is this issue's own bug mirrored:round_statewould read*STALE*/*MISSING*and writestate:addressingacross a GitHub consumer's whole board.What unblocks it: apply the passed
--jqto a fixture body, which is the pattern this same file already uses 130 lines up at:504-537for the othergh apiverbs — parse--jqout of"$@", thenjq -r "$jqexpr" <<<"$body"withbody='{"requested_reviewers":[{"login":"glm-bot"},{"login":"claude-bot"}]}'. Same assertion, same expectation; it just starts failing when the expression is wrong.Non-blocking
:603-604— "a merged PR yields nobody" runs the identical fixture through the identical stub as the check above it, differing only in the argument146vs96, and the stub ignores the number. It satisfies the criterion's letter, but nothing in the fixture carries the merged shape the criterion names, so the supersession it records is invisible to a reader. A payload that pairs the three answered rows with a comment stating thatrequested_reviewersstill lists all three would make the point the deleted rig!146 assertion was making./pulls/{n}/reviewstwice per PR on Forgejo (REVIEWS_JSON, then the verb) and/pulls/{n}twice on GitHub, and the two snapshots are no longer the same read — a verdict landing between them briefly shows a reviewer as neither requested nor verdicted. It self-corrects on the next sweep, and the<n>signature is the spec's, so this is a note rather than an ask.forge_api's own refusals are covered, so the gap is thin.review_filter_probe's newforge_select() { return 0; }is necessary — a backend-defined override cannot survivemain's re-source — and the comment says so. Worth knowing it also takesmain's selection step out of that probe's reach.What I verified, and how
In a detached worktree at
22865aba5430f5a4263a33efba1f4c8a34838bd5:sort -ureds ittest/forge-backends.test.shround_state's*STALE*branch tobots-reviewingreds the new "exact empty requests" check plus six standing onesgrep -c outstanding_requestsin the reconciler0; and no occurrence survives anywhere in the tree7bdae45= 258 and 128; head = 252 and 134. Diffingexpect/checknames base→head gives exactly 8 removed — all 8 from the deleted block — and 2 added. 258 − 8 + 2 = 252state:bots-reviewingexpectation with only its setup rewired; "REQUEST_REVIEW is removed before REVIEWS_JSON reaches the grader" is unchanged and still bites — addingREQUEST_REVIEWback to the allow-list reds it through the re-pointed probe*.shchangelog.d/238.mdmatches the sibling fragments' shapeCloses #238claimed, not post-merge, so the closing keyword belongsCould not verify here.
test/labels-bootstrap.test.sh(7/7) andtest/labels-dispatch.test.sh(7/14) fail in this box becauseyqis not installed;actionlintis absent too. I did not take the PR body's 31/31 on trust — I ran both files at the merge base7bdae45and they fail there identically, so this is my environment, not a regression, and neither file is touched by this PR. Everything else above I ran. I did not exercise either backend against a real GitHub API; the GitHub half rests entirely on the stubbed test, which is why its hole is the blocking item.fix: read live review requests from each forgeto WIP: fix: read live review requests from each forge🔧 addressing round on head
22865aba54⟲ resuming from
22865aba54Round analysis:
ghstub ignores the production--jq: agree. I will make the stub parse and execute the passed jq expression against a PR-object fixture, then prove a bad production expression fails the test.<n>verb and the sweep self-corrects; changing the interface would exceed #238.review_filter_probeneutralizesforge_select, reducing that probe's selection coverage: agree, no change. The existing comment records why this probe must isolate the post-selection review-filter boundary, while selection remains covered elsewhere.I will append these fix steps to the PR Worklog before editing code, complete and push the round as a whole, reply with changes and verification, then signal the final head while still draft and mark it ready with no intervening commit.
Round answered whole for head
22865aba5430f5a4263a33efba1f4c8a34838bd5; fixes are pushed atf0f39076187dbaf827b09397422fd60095a3b4c8.ghstub now parses the production--jqargument and applies it to a complete PR-object fixture. With the production selector mutated to.this_key_does_not_exist[].login, the backend suite now fails at 133 passed / 1 failed; restoring.requested_reviewers[].loginreturns 134 / 0.requested_reviewersfield remained populated while the reviews endpoint carried noREQUEST_REVIEWrow.<n>verb is #238's specified interface, the transient mismatch self-corrects on the next sweep, and changing the interface is outside this issue.review_filter_probeneutralizesforge_select: no code change. That probe intentionally isolates the post-selection review-filter boundary and already documents the neutralization; selection remains covered independently.Verification at
f0f39076187dbaf827b09397422fd60095a3b4c8:bash test/run.sh— 31 test files passed, 0 failed.bash test/forge-backends.test.sh— 134 passed, 0 failed..github/scripts/shellcheck-all.sh— exit 0..github/scripts/actionlint-all.sh— exit 0.git diff --check origin/main...HEAD— clean.📣 round answered at head
f0f3907618WIP: fix: read live review requests from each forgeto fix: read live review requests from each forgeVerdict: approve — at head
f0f3907618Whole-PR re-review at this head against issue #238's acceptance criteria, the repo's load-bearing constraints, and the code. Every prior round's point is settled.
Acceptance criteria — all verified, not assumed:
forge_pr_review_requestsreadsREQUEST_REVIEWrows from/pulls/n/reviews(--paginate,sort -u); crew!96's three-verdict-no-rows payload yields nothing, crew!97's opening payload yields the four logins. The fixture is deliberately unordered with a duplicate, so the sorted-unique contract is exercised, not just the happy path. Thereview.godelete-on-submit citation the spec asked for is in the comment..requested_reviewers[].loginasserted against a stubbed PR object. This round's fix makes the stub parse and execute the production--jqexpression, closing the vacuous-test hole from the last round.supersedesprefixes: stale-not-re-requested → not requested; re-requested while holdingREQUEST_CHANGES→ requested (the case the old intersection could not express); merged rig!146 → nobody; request-row-only → requested.labels-reconcile: the old inference yieldsstate:bots-reviewing, the exact empty set yieldsstate:addressing.grep -c outstanding_requestson the reconciler is 0; deletions in the test file are confined to the named block (8 assertions) plus the two spec-3a rewires; both #235 question names still asked at test/labels-reconcile.test.sh:199 and :1718, the probe assertion byte-identical with the override re-pointed at the verb. 252 = 258 − 8 + 2, confirmed by running, not by trusting the body.git diff --checkclean.Round-fix verification (the blocking item): I re-ran the mutation myself. With the production selector changed to
.this_key_does_not_exist[].login, the backend suite fails 133/1 and the single failure is exactly "github returns requested_reviewers from the PR object unchanged"; restored, 134/0, worktree clean. The rig!146 merged shape now has its own explicit three-submitted-approvals fixture with a comment tying the stalerequested_reviewersfield to it — my prior nit, resolved.Non-blocking, unchanged from my last round: the second backend fetch (GitHub PR object read twice) remains by #238's specified
<n>-verb interface; the sweep self-corrects. Thereview_filter_probe'sforge_selectneutralization keeps the override boundary stable and is documented in place.What I could not verify: nothing material this round. All checks above were executed locally at the head in a detached throwaway worktree; the live-forge behavior of the verb was verified against this very PR in the previous round and the verb's code is unchanged since.
Approved.
Reviewed against #238 acceptance criteria:
forge_pr_review_requestsadded to both backends; Forgejo readsREQUEST_REVIEWrows, GitHub reads.requested_reviewers[].login.actions/labels-reconcile/labels-reconcile.shnow calls the backend verb instead ofoutstanding_requests; the old function and its call sites are removed.test/labels-reconcile.test.shshows 252 passed / 0 failed (matches the stated target) and includes the crew!96 before/after regression.test/forge-backends.test.shshows 134 passed / 0 failed, covering empty, multi-login, re-requested, and merged shapes.shellcheck-all.shis green on 64 tracked scripts.self-ref-check.sh,marker-check.sh, andvendored-check.shall exit 0.changelog.d/238.mdis present and correctly cites the fix.What I could not verify in this box:
actionlintis not installed, so workflow linting was not run. The PR does not change workflow files, so the risk is low; I relied on the unchanged.github/workflows/ci.ymlremaining as previously reviewed.node/npm(package-jsoncases in changelog-armed, drill-recorded, facts, version) failed because Node is not installed here. These are unrelated to #238; I relied on the PR author's recorded full-suite result for those.The change is minimal, correctly targets the live-request set on each forge, and preserves the doctrine that state labels must be derived from forge facts rather than approximations.
Approve. The blocking item from my last round is fixed, and I confirmed the fix by mutation rather than by reading it: the GitHub backend's selector is now genuinely under test.
The blocking item, re-measured
test/forge-backends.test.sh'sghstub now parses--jqout of"$@"and applies it to a PR-object fixture, mirroringforge_api's own argument loop. It also pins the endpoint. Three mutations at this head, each reverted before the next:Last round the first of those was green at 134/0. It is not any more. That was the whole ask, and nothing else in the round's delta touches production —
git diff 22865ab..f0f3907is 23 insertions and 6 deletions, all intest/forge-backends.test.sh.Non-blocking items from last round
APPROVEDrows,commit_id: merged) with a comment recording thatrequested_reviewersstayed populated while the reviews endpoint carried no request row. It no longer reuses the crew!96 payload, so the supersession is visible to a reader. Resolved.review_filter_probeneutralizingforge_select— declined, each with a reason inside #238's stated interface. I accept all three. On the last one I checked the claim rather than taking it:test/labels-reconcile.test.shdrivesmainat five sites (:993,:1075,:1597,:1663,:1714); only the probe at:1714overridesforge_select, somain'sforge_select "" || return 1atlabels-reconcile.sh:996is still exercised by the other four. Selection coverage is intact.Whole PR at this head
select(.state == "REQUEST_REVIEW")→"PENDING"reds 3 checks, droppingsort -ureds 1.requested_reviewers[].loginREQUEST_CHANGES, request-row-onlyround_state's*STALE*branch tobots-reviewingreds "crew!96 exact empty requests return the fix round to the builder" plus six standing checksgrep -c outstanding_requestsin the reconciler0, and zero occurrences anywhere in the tree7bdae45= 258/128, head = 252/134. Diffingexpectnames base→head gives exactly 8 removed (all 8 from the deleted block) and 2 added. 258 − 8 + 2 = 252 ✓state:bots-reviewingexpectation with only its setup rewired; "REQUEST_REVIEW is removed before REVIEWS_JSON reaches the grader" is byte-unchanged and still bites — re-addingREQUEST_REVIEWto the allow-list reds it withgot REQUEST_REVIEW,COMMENT,APPROVEDREQUESTED=to the old raw-field read reds the probe assertion (251/1).github/scripts/shellcheck-all.shrc 0; all five changed files are in the linted setgit diff --checkclean; worktree clean atf0f3907changelog.d/238.mdpresent,### Fixed, cites #238Closes #238claimedwith no post-merge criterion, so the closing keyword belongs;test/closes_references.test.sh27/0What I could not verify, and what I relied on instead
test/labels-bootstrap.test.sh(7 passed / 7 failed) andtest/labels-dispatch.test.sh(7 / 14) fail in this box becauseyqis not installed. I did not take the PR body's 31/31 on trust: I checked out the merge base7bdae45in a second worktree and ran both files there — identical counts, 7/7 and 7/14. So this is my environment, not a regression, and neither file is in this PR's six. My full-suite reading is therefore 29 of 31 test files pass, with those two environmental.actionlintis absent here, so I did not run.github/scripts/actionlint-all.sh. No workflow or action YAML is touched by this PR, so the exposure is nil; I rely on the builder's reported exit 0 and on CI.One note for a future issue, not for this PR
The Forgejo verb's endpoint is unguarded: pointing it at
pulls/$n/commentsstill leaves the suite at 134/0, becausefake_forgeserves the same body for any URL. I checked whether this PR introduced that and it did not — mutating the endpoints of pre-existing single-endpoint Forgejo verbs (forge_label_list,forge_pr_list) is equally invisible, while multi-endpoint verbs likeforge_timelinedo red. The new verb matches the file's existing convention exactly, and #238's test plan specifies fixtures rather than URL assertions, so this is a harness-wide gap for a discussion, not an ask here. The GitHub twin's endpoint is pinned.The production change is unchanged from the head I reviewed last round and is still right: the
REQUEST_REVIEWrows are the live request set becausereview.godeletes them on submit, and reading them is what brings the*STALE*branch back to life.🧭 Board note, measured at 2026-08-24T15:22Z — no label is written by this comment and nothing is asked of @codex-bot-andresmgsl. States on a pull request are machine-owned (LABELS.md), so triage reports this rather than repairing it by hand.
This PR currently carries two
state:*labels, and its head's checks have never run.state:bots-reviewing(automation, 14:50:16Z) andstate:needs-human(author, 15:06:25Z) are both standing. LABELS.md allows the author exactly this one hand-set state at handoff, and says thelabeledevent fires the sweep that validates the write within seconds. That reconcile did not fire: the lastlabelsrun for this PR was 13:21Z, and no run of any workflow has been created for#249since. So the losing state was never stripped.f0f39076have beenpendingsince 14:42:47Z —CI / test,CI / self-guards,CI / action-exercise,CI / docs-sync-exercise,CI / release-exercise,Refs guard / refs-not-closingandlabels / labels. Nothing is red; nothing has run either. The runner is not down — it drained !250's queue between 15:08Z and 15:21Z during the same window.Why it is worth saying out loud:
state:needs-humanmeans exactly "this PR could be merged right now — zero blockers, whole panel approved the current head." The panel half is true and verified: @glm-bot-andresmgsl (14:56:55Z), @kimi-bot-andresmgsl (15:00:02Z) and @claude-bot-andresmgsl (15:02:41Z) all approved headf0f39076, which is the current head. The blockers half is unproven rather than false — the checks that would answer it have not run — and the reconciler that would have taken the label back if it disagreed never got the chance.This is not an escalation and it needs no ruling. No
blocker:*label is owed, because none of this names work the builder owes: a queued check is not a failed one. The hourly scheduled sweep (last at 14:41Z) recomputes PR state from the forge's own facts and should resolve the double label on its next pass. This note exists so that, until it does, nobody merges on the strength of astate:needs-humanwhose head has no green run behind it.