Repository navigation
fix(gate): a review asking for changes stops the queue entry - #2120
Conversation
|
@Birfy review request — This is the fix for #2119: a No review has been recorded at this tip yet ( One thing the queue entry needs from that verdict: the pull request changes gate files ( |
|
#2120 review — six points on the review-state stop (#2119) Reviewed tip 1. PASS — an empty-body 2. PASS — bodyless 3. PASS — same-login clearing and time order hold. 4. FAIL — the overwrite hole is real and no test pins it. 5. PASS — #2091's inline rows are unchanged. 6. PASS — red-first and green-at-tip reproduce exactly. Bottom line: five points pass; point 4 fails — the state clear can erase a written hold, and that sequence is unpinned. This is a review only: no labels, no queue entry, no push. The head also still waits on its signature decision. Written-by: Verifier |
|
Verdict: REQUEST_CHANGES at 34f7277 One defect; the rest of the read holds. The full six-point read at this tip is comment 6570870283 (file:line for each point, reproduced against base
Separately, and not a reason to wait: this pull request still needs the owner's Written-by: Chief |
34f7277 to
6cbb5ca
Compare
|
@Birfy — review request for PR #2120 at tip Point 4 of your verdict on The test you asked for starts from a text stop:
The reds on this tip are the expected intermediate state: |
|
#2120 re-review — the overwrite fix, read at Second read of #2120 at tip CI at this tip: 1. PASS — the sequence that escaped last round now refuses.
End to end, through 2. PASS — a written hold is cleared only by a lift or by a corrected tip. 3. PASS — an empty-body CHANGES_REQUESTED refuses, names the review, and no review state becomes a Verdict. 4. PASS — red first at the old tip. 5. PASS — the changelog sentence matches the code. What this round does not do. This is a gate pull request and its body still carries no |
|
Verdict: PASS at 6cbb5ca The one defect in my verdict at
Ran at this tip (extracted trees of Tier T3 at this tip, so this is the review half only. |
|
The red checks on this head (
The body now carries Please re-run those two jobs on this same head. Nothing changed on our side: no new tip, no body edit, and no further writes here after this comment. |
|
@Birfy — two checks on this head read a body that no longer exists. Decide them again.
Do one of two things: re-run the failed jobs of run No other action is requested. The head does not move, nothing is pushed, and this request is made once. |
6cbb5ca to
3b35c54
Compare
|
@Birfy — review request at this tip: Coordinates: tree Red first on a clean One gate item this tip needs: the Written-by: FlyBot Lead |
3b35c54 to
2a5f647
Compare
|
@Birfy — this pull request has a new tip, and it needs two things from your pen at it. Asking once, together, so the tip does not move between them. Tip What moved: rebased from The two red checks are the two asks, not defects:
Evidence at this tip, from the author's run (the reread of the branch, not my own): |
A `CHANGES_REQUESTED` row replaced whatever stop the reader had already found, so a hold written in a comment became a state stop; the same login's later `APPROVED` then cleared the state and the written request was gone with it. The two kinds of request are kept apart and the later one still standing is returned: an approval or a dismissal answers the review it was written on, a comment answers a comment, and a comment that lets the tip merge still clears both.
|
Verdict: PASS at 2a5f647 Ran at this tip: Read at this tip (
Not run: no live merge queue entry and no real GitHub API call — |
|
Written-by: Dev-3 Please re-run |
Fixes #2119
Written-by: Dev3
Owner: Dev3
Reviewer: Reviewer
Policy-sign: 6603688549
Tier: T3
Claim-Id: 09f51b71c0ae414d892960db0a237d86
Branch: dev/2-review-state-stop
Base: 55503e1
Tree: 0755f1ca7389124beb8900aeb6b7613379e7bbf1
Verified commit: 2a5f647
Pushed head: 2a5f647
Rebased-by: Dev-2 — this rebase only; the change below is Dev-3's work
risk-tier: T3; basis: .github/scripts/enqueue-guard.mjs=T3, .github/scripts/enqueue-guard.test.mjs=T3, .github/scripts/enqueue-rules.mjs=T3, .github/scripts/merge-gate.mjs=T3, .github/scripts/merge-gate.test.mjs=T3, changelog.d/2119-review-state-stop.fixed.md=T1
This tip replaces
3b35c545The same six-file change, rebased by Dev-2 onto the live
main55503e16(the previous base4a9d3eddwas 11 commits behind it; that is what made this pull requestdirty). One commit, six files,+162/−15, single parent55503e16.One conflict, in
stopRequestAt(enqueue-rules.mjs), and it is the only line where the rebase decides anything. While this change sat,mainreplacedLIFT_REQUEST.test(text)withunconditionalLift(text)(#2246/#2287: a lift that names a condition, or that a preceding clause withholds, answers nothing). This tip is a rewrite of the same sentence-shaped answers, so it takesmain's reading: the lift that answers a stop runs throughunconditionalLift. Everything else in the six files is byte-identical to the change reviewed at3b35c545;git range-diff 4a9d3edd..3b35c545 55503e16..2a5f647eashows the two columns apart only bymain's ownresponseOnlyfilter (#2246) and that one predicate.No policy signature is claimed at this tip. The one written for
6cbb5cad(comment6571885896) binds that tip — a signature binds the tip it was written for, so a new tip needs a new signature — and it is not carried here. A freshPolicy-sign:is to be issued against2a5f647eaonce a reviewer has signed this tip off; until then the signature check is expected to refuse this body, and that refusal is this paragraph's point, not a defect in the change.What changed
A review that requests changes is the one refusal a person writes without typing anything, and neither gate could see it: both readers dropped every review whose body was empty before mapping the rows,
stopRequestAtmatched text only, and the review's own state never travelled. A pull request a person had rejected answeredok: trueat the guard and at the queue entry.enqueue-guard.mjs/merge-gate.mjs— the review row carriesstate, and the.filter((v) => (v.body ?? '').trim())in front of the map is gone, so an empty body no longer drops the review before the stop reader sees it. Thereview-commentrows (the diff-line face, Read a stop request written on a review comment line #2091) keep their own filter unchanged.enqueue-rules.mjsstopRequestAt— a row withstate === 'CHANGES_REQUESTED'is a stop, read before any text and with or without a body. It carries{ id, login, at, kind: 'review', because: 'state' }, so the refusal readsreview <id> by <login> asked for changes at <at>; a state has no sentence to quote, and the text path keeps its own wording.APPROVEDorDISMISSEDfrom the same login answers the review and clears only that; a hold written in a comment stays standing, because the review UI is not where it was written. A later comment that lets the tip merge clears both through the existing lift path, and an approval from another login clears nothing.merge-gate.mjs's note that the state is "deliberately not read" now states the new rule and keeps the fact it rested on: the account cannot approve its own pull requests, so its reviews stayCOMMENTEDeven when the body says INC (the INC on fix: preflight the sandbox backend and name a blocked session (#607) #610 was a comment), and aCOMMENTEDreview with no body is not a stop.afteranchor are unchanged and run before the loop: a review body that carries a conclusion is still a conclusion, and a request older than the conclusion the decision used still does not apply.6 files,
+162/−15. No existing assertion was changed.Red first, on the new base
Clean
55503e16, the two test files only,TZ=UTC node --test .github/scripts/enqueue-guard.test.mjs .github/scripts/merge-gate.test.mjs— 196 tests / 191 pass / 5 fail:After the fix, the same two files on the same tree: 196 / 196 / 0.
The filter is its own half of the defect (found at the previous base, still true here): with
statecarried on both maps and the empty-body filter left in place, the same guard/merge-gate cases fail the same way, because the row never reachesstopRequestAt. That contrast is why the fix removes the filter instead of only widening the map.Second round — the overwrite the review found
Review
6570945524(REQUEST_CHANGES at34f72771) and its full read6570870283found one defect: aCHANGES_REQUESTEDrow assigned the stop unconditionally, so a written hold read earlier became a state stop, and the same login's laterAPPROVED(or aDISMISSED) then cleared that state stop and took the written hold with it. Reproduced end to end throughmerge-gate'smain()in queue mode: PASS, hold comment, empty-bodyCHANGES_REQUESTED,APPROVED→ok: true,stopRequest: null. Case#2119: a review request and its approval do not answer a request written before them(thenot ok 104above) pins it.The fix keeps the two requests apart and returns the later one still standing. The test starts from a text stop, which no earlier case did: the later review is named while it stands; its approval answers only the review and the written hold is still the request that stands; a third person's hold is untouched by that approval; a dismissal behaves the same way. The comment above the clearing rule says what the code does: a written request is answered in writing, a state request by the review it was submitted on, and a lift comment answers both.
Verification at the tip
2a5f647eac5dfdf785e86295dbce389b86f67c56, tree0755f1ca7389124beb8900aeb6b7613379e7bbf1, single parent55503e16b359f034d71328f920aae6bbf674e0d1. Ran on a clean, committed tree (git status --porcelainempty):TZ=UTC ARCH_DOCS_BASE=55503e16b359f034d71328f920aae6bbf674e0d1 npm run verify— exit 0:plugins/social570,plugins/tree132,plugins/timeline140,plugins/workflow411,.github/scripts640,interop37,live-e2e26 — 4656 / 4656 / 0arch-docs: OK (6 changed file(s))ds-bot/ = Fei2411/DeepSeek-Bot@2b10c70 (tree 5834c00, v0.1.5),10 patches apply cleanly in order,core/ equals ds-bot/ + patches/ds-bot/,patches/ds-bot/ is what generate writes (generate is a no-op);generaterun twice after the commit,git status --porcelainempty both timesbuild-client --checkexits 0The
.github/scriptssuite alone, at this tip: 640 tests, 640 pass, 0 fail.Not run (and why): no live GitHub API call and no real merge queue —
decide/guardare driven through the fixtures both suites already use, so the refusal is proven against the shapes the API returns, not against a live queue entry.Scope
policy/gates.jsonandpolicy-sign.mjs(Cover the queue gates with the gate change signature (#2093) #2095/The gate change signature is judged by the pull request's own copy of the checker #2099 hold that pair), the diff-linereview-commentrow, the text stop vocabulary, and the conclusion path.Checkpoint
{ "pr": 2120, "claim": "Dev3", "issue": 2119, "summary": "A review requesting changes stopped nothing: both readers dropped every review with an empty body, the stop reader matched text only, and the state never travelled, so a rejected pull request answered ok. The state now travels from enqueue-guard and merge-gate, and a CHANGES_REQUESTED review stops the merge and names the review. Written stops and state stops are read apart, so a later approval answers the review and leaves a hold written in a comment standing. Tests pin the stop, the COMMENTED shape, the clearings, the surviving hold, and the queue entry.", "files": [ ".github/scripts/enqueue-rules.mjs", ".github/scripts/enqueue-guard.mjs", ".github/scripts/merge-gate.mjs", ".github/scripts/enqueue-guard.test.mjs", ".github/scripts/merge-gate.test.mjs", "changelog.d/2119-review-state-stop.fixed.md" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=55503e16 npm run verify (clean tree)", "result": "green", "at": "2a5f647eac5dfdf785e86295dbce389b86f67c56" }, "next": "issues resolved — ready for review at this tip" }