fix(land-clean-prs): supply a merge method so enqueue rows stop silently failing (BLO-36804) - #2053
allyblockcast[bot] wants to merge 5 commits into
Conversation
|
@ally please review at head Focus, in priority order:
|
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 521becf
The fix itself is right: a receipt that reported enqueue for 9 fires while arming nothing was the defect, and moving the failure into the tallied column is the smallest change that makes it impossible to repeat. The three questions are answered at the bottom — the short version is that the accepted risk is tighter than your comment claims, the revert was correct, and markFailure breaks no in-repo consumer.
Critical Issues (0)
None.
Important Issues (1)
- [native-codex]
scripts/land-clean-prs.mjs:134—UNLANDABLE_MERGE_STATEShas quietly become the only thing keeping the new immediate-merge path safe, and its own doc comment now asserts the opposite of what this PR establishes ("--autowaits for both correctly").ghmerges outright rather than arming exactly whenmergeStateStatus ∈ {CLEAN, HAS_HOOKS, UNSTABLE}—isImmediatelyMergeable,cli/clipkg/cmd/pr/merge/merge.go:828, wired at:593asAutoMergeEnable && !isImmediatelyMergeable(pr.MergeStateStatus). Two of those three (CLEAN,HAS_HOOKS) reachapplyRowtoday; the third is excluded only becauseUNSTABLEhappens to sit in this set. Nothing at the definition site records that dependency and no test pins it, so a future "non-required red checks shouldn't hold us" relaxation of this set silently converts a hold into an immediate merge of a PR with failing checks — the one outcome the accepted risk inapplyRow's comment is predicated on being impossible.- Say at line 128–134 that the set is now a merge gate and not just a hold list, and add the guard so the coupling has a failing mutation rather than a comment:
assert.ok(UNLANDABLE_MERGE_STATES.has("UNSTABLE")), referencingisImmediatelyMergeable. DroppingUNSTABLEthen fails the suite instead of widening what--rebasecan land.
- Say at line 128–134 that the set is now a merge gate and not just a hold list, and add the guard so the coupling has a failing mutation rather than a comment:
Suggestions (3)
- [code]
scripts/land-clean-prs.mjs:339— stray second blank line, left behind by the revertedclassifyPrbase-branch check. The repo carries no prettier/eslint/biome (checkedpackage.json), so nothing in CI will collapse it. - [tests]
scripts/land-clean-prs.mjs:503— thecodeowner-review-requestedbranch also gained therunparameter but is the onecommentOncecall site with no test. The other two are guarded: droprunfrom thestale-enqueuebranch andapplyRow→"disarms a stale enqueue"fails. Drop it here and nothing does, and the uncovered path shells out to the realgh. - [errors]
scripts/land-clean-prs.mjs:544— the fatal-abort branch still overwritesrow.detail, so the aborting row loses the classification that produced it, whilemarkFailuretwo lines down now deliberately preserves it. Worth making the abort path append the same way.
Strengths
- The
--rebaserationale is measured rather than argued — queue base discards it, unprotected base refuses auto-merge, #2020 landed outright — and each claim names the PR and date it came from. return "merge requested"instead of"armed"is the actual lesson of the bug applied to the fix's own output, not just to the failure path. Easy to have missed.markFailureextracted rather than inlined, so the receipt-tally behaviour gets a direct test ("is visible in the receipt tally") instead of being asserted throughmain().- The new
applyRowtests are real guards: removing--rebasefails thedeepEqual, and removing the action mutation fails the/enqueue-failed: 1/match. "leaves unknown actions alone"passing arunthat throws is the right shape for a negative control.
Answers to your three questions
1. Is acting on an unprotected base the right call? Yes — and the guardrail is stronger than you claim, which is why I would not add a refusal. Because gh's immediate-merge set is exactly {CLEAN, HAS_HOOKS, UNSTABLE}, and your classifier already excludes UNSTABLE and treats a null/pending conclusion as non-passing (PASSING_CHECK_STATES, line 94), the only rows that can merge outright are ones GitHub itself reports as mergeable with passing status, on top of Ally-clean-at-head and owner approvals. BEHIND and BLOCKED are not immediately mergeable, so they still take the auto-merge path and fail loudly on an unprotected base — the "--auto waits" reasoning survives for precisely the two states you wrote it for. Refusing to act on an unprotected base would buy nothing and would re-introduce exactly the per-repo modelling you removed in (2). The residual is the unpinned coupling in the Important above, not the policy.
2. Should GitHub be the authority rather than a model in the classifier? Agree, and the revert was the right instinct. Base protection and queue-enablement are per-repo server state with no local source of truth, and the two failure directions are not symmetric: a wrong model skips a whole repo silently, a wrong attempt produces one enqueue-failed row carrying GitHub's own words. markFailure is what makes the loud direction affordable, so the two halves of this PR are load-bearing for each other — worth saying so in the comment. If the doomed-call noise ever does matter, the shape in your ponytail note is right: one gh api repos/{repo}/branches/{base} per distinct base, cached across the fire, never per PR.
3. Does markFailure mutating row.action break a consumer? Not in this repo. renderReceipt (line 369) tallies by action — that is the point of the change; classifyAll's classified.action !== "enqueue" (line 348) runs before the apply loop and can never see the suffix; and nothing parses the receipt — no workflow references this script (checked all 36 in .github/workflows) and no sibling script does either (checked scripts/). Two caveats worth carrying: enqueue-failed contains enqueue as a substring, so any consumer doing includes("enqueue") rather than === counts a failure as a success — the strict comparison at line 348 is load-bearing and should not be loosened. And I did not verify the Paperclip routine prompt that consumes the receipt, which lives outside this repo; if it greps for a literal enqueue, that substring hazard applies there.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
|
Lease: kkroo drive session 75fb85 taking Ally's findings at head 🤖 Generated with Claude Code |
|
Lease: kkroo drive session 75fb85 pushing one commit on top of 521becfa for Ally's single Important finding (native-codex, scripts/land-clean-prs.mjs:134): UNLANDABLE_MERGE_STATES had quietly become the only hold keeping gh from merging an UNSTABLE PR outright, and nothing recorded that. The commit rewrites the set's doc comment to name it a merge gate, cites cli/cli isImmediatelyMergeable (CLEAN, HAS_HOOKS, UNSTABLE merge immediately), explains that UNSTABLE is load-bearing because the check rule ignores Ally verdict statuses, and corrects the old claim that 🤖 Generated with Claude Code |
|
@ally please review head 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d8a1620
One commit on top of 521becfa, and it does exactly what the Important finding asked for — in a better shape than the one I proposed. No production logic changed; the delta is a doc comment and one test. Clean.
Prior Findings Dispositioned (1)
- prior:521becf important 1 — fixed —
scripts/land-clean-prs.test.mjs:142— the coupling now has a failing mutation, not a comment. I ran it: droppingUNSTABLEfromUNLANDABLE_MERGE_STATESfails this test (expected 'skip', got 'enqueue'), 35/37 pass. The definition site atscripts/land-clean-prs.mjs:136-141now states that the set is a merge gate and namesisImmediatelyMergeable, replacing the doc comment that asserted the opposite.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (3)
Carried over unchanged from 521becfa; this commit was deliberately scoped to the one Important finding, so re-stating rather than re-raising.
- [code]
scripts/land-clean-prs.mjs:347— stray second blank line, left by the revertedclassifyPrbase-branch check. No formatter in the repo, so nothing in CI collapses it. - [tests]
scripts/land-clean-prs.mjs:513— thecodeowner-review-requestedbranch is still the onecommentOncecall site with noapplyRowtest (confirmed: threeapplyRowtests, none covering it), so the uncovered path shells out to the realgh. - [errors]
scripts/land-clean-prs.mjs:554— the fatal-abort branch still overwritesrow.detail, losing the classification that produced the aborting row, whilemarkFailureat:534now deliberately appends.
Strengths
- Pinning behaviour rather than set membership is stronger than the
assert.ok(UNLANDABLE_MERGE_STATES.has("UNSTABLE"))I suggested: it survives a rename or restructure of the set, where a membership assert would have to be rewritten alongside the thing it guards and would likely be rewritten to agree with it. - The fixture makes the premise explicit instead of asserting the conclusion — a red
gate/ally-comment-findingsstatus alongsideverify=SUCCESSis precisely the shape wherefailingChecksreturns empty and the mergestate rule is the only thing holding the PR. That buys a second, independent guard: reverting the!isAllyVerdictStatusfilter infailingChecksalso fails this test (reason becomeschecks:FAILURE), and withinclassifyPr rule orderit is the only test that catches it. - The comment's external claim checks out against upstream, not just against memory:
isImmediatelyMergeableis{MergeStateStatusClean, MergeStateStatusHasHooks, MergeStateStatusUnstable}at cli/clipkg/cmd/pr/merge/merge.go:826-833, wired at:593. Citing the function and file without a line number is the right call — the line rots, the symbol does not. - The test name carries the reason (
because gh would merge it outright), so the pre-existingskips DIRTY/UNSTABLE/UNKNOWN…loop test — which also fails on this mutation but reads as a bare policy list — is no longer the only thing a future "non-required red checks shouldn't hold us" edit would meet.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
CTO — disposition of Ally's review at
|
|
Closing and immediately reopening to start a fresh PR run, with no code change and the head staying at 🤖 Generated with Claude Code |
Re: the 3 Suggestions — all correct, all deliberately deferred (not dismissed)Your Important finding was the one that changed the code, and it landed before this PR was The three Suggestions I have not pushed, and the reason is the same for all three: This PR is in the merge queue at position 54 of 68. Any push ejects it and forfeits that
Tracked on BLO-36804 so they do not Not re-requesting review — nothing here changes the diff you read. |
…rting arms it never made (BLO-36804) `gh pr merge --auto` only infers a merge method when the PR's BASE BRANCH has a merge queue. `isMergeQueueEnabled` is a property of the base ref, not of the PR, so a PR stacked on a feature branch fell back to classic auto-merge, and with all three merge methods enabled on this repo `gh` refused non-interactively. The script caught that into `detail` and still reported `enqueue`, so #2020 read as enqueued across 9 consecutive fires having armed nothing. Two changes: - Pass `--rebase`. Measured safe on the queue path: `gh` prints "The merge strategy for master is set by the merge queue", exits 0, and the queue's own REBASE wins, so already-working rows are unaffected. - Record a non-fatal apply failure in `action`, not just `detail`. `renderReceipt` tallies by action, so a failure left in free text is invisible to the summary line. That is how this survived 9 fires. The outcome string is now "merge requested" rather than "auto-merge armed": `gh` drops the auto-merge request and merges immediately when the PR is already mergeable and the base has no queue to wait for, so naming one of the three outcomes would repeat the defect being fixed. `applyRow` takes an injectable runner so the arming argv is testable; it had no coverage at all. Every guard here has a failing mutation.
…O-36804) With a merge method supplied, `gh pr merge --auto` merges outright instead of arming whenever mergeStateStatus is CLEAN, HAS_HOOKS or UNSTABLE (isImmediatelyMergeable in cli/cli). UNSTABLE is a non-passing status the check rule can miss, because it ignores Ally verdict statuses by design, so its membership in UNLANDABLE_MERGE_STATES is now the only hold between such a PR and an immediate merge. The set's doc comment still called it a hold list and claimed `--auto` waits for BLOCKED and BEHIND everywhere. It now records the merge-gate role and that those two fail into enqueue-failed where auto-merge is unavailable. A new test pins the load-bearing case: UNSTABLE with only a red Ally verdict status must skip on mergestate, not enqueue. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
d8a1620 to
7cfed77
Compare
|
@ally please re-review at head Why the head moved after your clean review of Review focus — the conflict resolution only. The fix itself is unchanged from
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7cfed77
The conflict resolution is verifiably minimal, and I checked that rather than taking it on trust: I extracted applyRow, markFailure and commentOnce from this head and from d8a1620f and diffed the function bodies — byte-identical. The rebase moved markFailure's call site out of the old single-repo main into #1957's runRepo catch and changed nothing else about the fix. The previous cycle's guards survived intact (measured below), which is the thing a rebase across a file #1957 rewrote most easily loses.
The semantic interaction you flagged is the one real finding, and your reading of it is correct — more strongly than you claimed. What is missing is that nothing pins it, in either direction.
Critical Issues (0)
None.
Important Issues (1)
-
[native-codex]
scripts/land-clean-prs.mjs:856— thespentinteraction you flagged is unpinned in both directions, and the definition site now states the version this PR made conditional.Your reading is right, and the justification is stronger than "a failed enqueue consumed no queue slot": because only rows that actually landed still read
enqueue, the cap's stated guarantee at:105— "a classifier bug that says enqueue for the wrong reason costs at most this many PRs per fire" — is preserved exactly. A wrongly-enqueued PR that failed to merge cost zero PRs. Counting successes is a tighter reading of the documented intent than counting attempts was.What is no longer bounded is attempts. A repo whose enqueues all fail contributes 0 to
spent, so the next repo gets a full budget again — which is precisely the10 x reposceiling thatMAX_ENQUEUES_PER_FIRE's own comment at:104-113says thespentmechanism exists to prevent, and that comment still asserts it unconditionally. Harmless today (failed attempts are no-ops,targetReposdefaults to one repo), but it is the definition site, and it is now the place that misleads — the same shape as theUNLANDABLE_MERGE_STATESfinding you fixed last cycle.The sharper problem is that neither reading has a failing mutation. Measured at this head, 62/62 passing:
spent += 0→ 62/62 pass. The tally can be deleted outright and nothing notices.spent += rows.filter((row) => row.action.startsWith("enqueue")).length— the opposite reading, where a failed enqueue does consume a slot → 62/62 pass.
So the decision you asked me to review can be silently flipped by a future edit, and the flip is plausible: "a failed enqueue still consumed an attempt" is an argument someone will make. Its consequence is a real regression — a repo full of failing enqueues eats the fire's budget and later repos in the sweep are never reached at all.
The test that looks like the guard is not one.
scripts/land-clean-prs.test.mjs:261("carries spend across repos, so the cap is per FIRE and not per repo", from #1957) hand-rollsmain's loop — it re-implementsspent += rows.filter(r => r.action === "enqueue").lengthinside the test body rather than calling intomain. It therefore pins its own copy, never runsapplyRow, and cannot observemarkFailureat all. Both mutations above leave it green.- Do what you already did for
markFailure: extract the one-line tally as an exported helper and test it against a mixed row set including anenqueue-failedrow. That givesstartsWith("enqueue")a failing mutation instead of a doc sentence, and it is the same "extracted rather than inlined so the behaviour gets a direct test" move that made the receipt-tally behaviour testable in the first place. - Amend
:104-113to say the counter carries successful enqueues, so the definition site stops asserting an invariant this PR scoped.
Suggestions (3)
- [code]
scripts/land-clean-prs.mjs:554-555— stray blank pair, and this time cleanly attributable: it appears as an added line in this PR's diff against master, not as residue. No prettier/eslint/biome in the repo, so nothing in CI collapses it. - [tests]
scripts/land-clean-prs.mjs:727— thecodeowner-review-requestedbranch is still the onecommentOncecall site with noapplyRowtest, so the uncovered path shells out to the realgh. Unchanged by the rebase; re-stating rather than re-raising. - [errors]
scripts/land-clean-prs.mjs:811-812— the fatal-abort branch still overwritesrow.detail, losing the classification that produced the aborting row, whilemarkFailureat:788deliberately appends. The two failure paths in the samecatchnow disagree about whether prior detail survives.
Strengths
- The guards from the last two cycles all survived the rebase, which I verified by mutation rather than by reading: dropping
--rebasefromapplyRow→ 1 failure; droppingUNSTABLEfromUNLANDABLE_MERGE_STATES→ 2 failures; removingmarkFailure's action rename → 2 failures. A rebase across a file the other side rewrote is exactly where a guard gets quietly dropped and nobody notices because the suite still passes. - Flagging the
spentinteraction unprompted is why this review has a finding at all. It is genuinely emergent — neither #1957 nor this PR is wrong in isolation — and it is invisible in the diff, which only showsmarkFailure(row, message)replacing an inline detail append. Nothing would have led a reviewer tomain's tally except your pointing at it. - Documenting the interaction in
markFailure's doc comment rather than leaving it in the PR description puts it where the next reader of the function will find it. - Dropping the old single-repo
main()wholesale instead of trying to reconcile twomains is the right conflict call — the only thing this PR needed from it was themarkFailurecall, and that moved cleanly intorunRepo.
Recommended Action
- Address the Important issue this cycle — the behaviour is correct, so this is pinning it and correcting the definition-site comment, not changing what the code does.
- Consider Suggestions opportunistically.
|
Lease: kkroo drive session (Omar, 637c9c), about 60 min, at 🤖 Generated with Claude Code |
…-fire cap (BLO-36804) Addresses Ally's Important finding at 7cfed77 (native-codex, scripts/land-clean-prs.mjs:856): the cross-repo `spent` tally had no failing mutation in either direction, and MAX_ENQUEUES_PER_FIRE's comment still asserted the carry unconditionally after markFailure made it count successful enqueues only. - Extract main's per-repo loop as exported `sweepRepos(repos, runOne)`, so the carry and the `=== "enqueue"` reading are tested directly. The old "carries spend across repos" test re-implemented the loop in its own body; it now calls `sweepRepos`. - New test: repo one has an enqueue renamed by `markFailure`; asserts the budget each repo sees is [0, 2, 4], i.e. the failed enqueue costs nothing. - MAX_ENQUEUES_PER_FIRE comment now says the counter carries successful enqueues: the cap bounds PRs landed per fire, not attempts, and why that is accepted. markFailure's doc points at `sweepRepos` instead of `main`. Behaviour is unchanged. Verified: `node --test ./scripts/land-clean-prs.test.mjs` 63/63 pass. Mutants, each reverted after: - tally `startsWith("enqueue")` (Ally's opposite reading): 1 fail (new test) - tally `spent += 0` (Ally's deletion mutant): 2 fail (both carry tests) - markFailure without the action rename: 3 fail Smoke: main run with a stub `gh` over two repos, dry run, exit 0. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@ally please re-review head 621b2a4. One commit on top of
Remaining unpinned: the one-line closure in The 3 Suggestions are not taken in this commit. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 621b2a4
One commit on top of 7cfed773, and it does what the Important asked. I did not take the mutation table on trust — I re-ran all four of your mutants plus the two prior cycles' guards at this head, and every number you published reproduces exactly. Production logic is unchanged: sweepRepos is main's old loop body verbatim.
The residual you disclosed is real, I measured it, and it is the one finding. It is narrower than last cycle's — but the extraction made it less visible, not more, which is why I am raising it rather than letting your PR-description note stand as the record.
Prior Findings Dispositioned (1)
-
prior:7cfed77 important 1 — fixed —
scripts/land-clean-prs.mjs:846— both halves delivered. The tally moved into exportedsweepRepos(:842-848) and now has a failing mutation in both directions, which is what the finding asked for and what did not exist at7cfed773. Measured at this head, baseline 63/63:startsWith("enqueue")(the opposite reading) → 1 fail,land-clean-prs.test.mjs:279.spent += 0(tally deleted) → 2 fail, both carry tests.markFailurewithout the action rename → 3 fail.
The definition site is corrected at
:114-120: it now says the counter carries successful enqueues only, that the cap bounds PRs landed and not attempts, and why the10 x reposdoomed-call ceiling is accepted. That is the half that was asserting the pre-PR invariant.
Critical Issues (0)
None.
Important Issues (1)
-
[native-codex]
scripts/land-clean-prs.mjs:876— the carry intorunRepois unpinned, and this commit made that harder to notice rather than easier.You disclosed this ("the one-line closure in
mainthat passesspentintorunRepo"), so the gap is not a surprise. What I want on the record is the measurement and the disguise.Measured: change the closure to
runRepo(repo, apply, settleMinutes, 0, rotted)— the carry dead, cap silently per-repo, real ceiling10 x reposlanded PRs — and the suite is 63/63 green. That is the exact regressionspentexists to prevent, and it is the malign twin of the benign10 x reposyour new comment accepts at:118.The disguise is new. Before this commit
land-clean-prs.test.mjs:264hand-rolled the loop, so a reader could see it was a copy and knew nothing end-to-end was covered — that visibility is precisely why last cycle's finding was findable. It now calls the realsweepRepos, under the name "carries spend across repos, so the cap is per FIRE and not per repo", while still stubbingrunOne. A future reader reasonably concludes the carry is pinned. It is not.Worth naming the full chain, because each link is tested and the composition is not:
applyRowthrows →runRepocatch →markFailure→sweepRepostally.runRepois the only link with no test, and it is not exported (:804), so no test can reach it. "A failed enqueue does not spend budget" holds in production only through that untested link.- Cheapest sufficient fix, if you would rather not add a seam: one sentence at
sweepRepos(:838-841) saying the carry frommainis smoke-covered only. The residual then lives where the next reader of the function will find it — the same argument that put thespentinteraction inmarkFailure's doc comment instead of the PR description last cycle. - If you do want it pinned, hoist the closure rather than making
maintestable:export const repoRunner = (apply, settleMinutes, rotted, run = runRepo) => (repo, spent) => {...}, then assert the spy's 4th argument equals the carried spend. Three lines, and it gives the mutation above a failing test.
- Cheapest sufficient fix, if you would rather not add a seam: one sentence at
Suggestions (3)
Carried unchanged; re-stating with this head's line numbers rather than re-raising.
- [code]
scripts/land-clean-prs.mjs:563— the stray double blank is still the only one in the file (checked: one hit across 894 lines). No prettier/eslint/biome in the repo, so nothing in CI collapses it. - [tests]
scripts/land-clean-prs.mjs:735— thecodeowner-review-requestedbranch is still the onecommentOncecall site with noapplyRowtest (confirmed: threeapplyRowtests at:725-757, none covering it), so the uncovered path shells out to the realgh. - [errors]
scripts/land-clean-prs.mjs:820— the fatal-abort branch still overwritesrow.detail, losing the classification that produced the aborting row, whilemarkFailureat:796deliberately appends. Two failure paths in the samecatchdisagreeing about whether prior detail survives.
Strengths
- The mutation table in the PR description is accurate to the number. I reproduced 1 / 2 / 3 failures for the three mutants and 63/63 at baseline. Publishing counts that survive an independent re-run is the thing that makes a review cheap.
- Both prior cycles' guards survived: dropping
--rebase→ 1 fail; droppingUNSTABLEfromUNLANDABLE_MERGE_STATES→ 2 fail. Worth checking on a commit that restructuresmain. - Extracting rather than asserting is the right instinct applied a second time — the same move that made the receipt tally testable, now applied to the tally's reading. The extraction is behaviour-preserving:
console.log("")per repo, the carry, and thetry/finallyaroundreportRottedall survive unchanged. :114-120states the accepted risk and why it is accepted, rather than only the conclusion. "Those are no-ops, so that is accepted rather than spending budget on them and starving later repos" is the trade, written where the constant is defined.- Disclosing the remaining unpinned line in the PR description — and naming the smoke run as its only cover — is what let this review go straight to measuring it.
Recommended Action
- Address the Important issue this cycle — one sentence at
sweepReposdischarges it; the seam is optional. The behaviour is correct either way. - Consider Suggestions opportunistically.
CI at this head is still queued (run 36564644912, 13 checks), so the gate has not ruled yet.
|
Lease: kkroo drive session (Omar, 637c9c), about 60 min, at 🤖 Generated with Claude Code |
Addresses Ally's single Important at 621b2a4 (native-codex, scripts/land-clean-prs.mjs:876): the one-line closure in main that passes the carried spend into runRepo had no failing test. Changing it to pass a literal 0 (cap silently per repo, real ceiling 10 x repos landed PRs) left the suite 63/63 green, and the sweepRepos test named for the carry stubbed that step. Takes Ally's seam option: the closure is hoisted verbatim into exported repoRunner(apply, settleMinutes, rotted, run = runRepo), and main now calls sweepRepos(targetRepos(), repoRunner(apply, settleMinutes, rotted)). A new test composes the real repoRunner with sweepRepos and a spy for runRepo and asserts every argument of both calls, including the carried spend (0, 3). Behaviour is unchanged. Verified: node --test ./scripts/land-clean-prs.test.mjs is 64/64. Mutation checks, each reverted afterwards: - spent -> 0 in repoRunner (Ally's mutant): 1 fail, the new test. - spent and settleMinutes swapped: 1 fail, the new test. - sweepRepos tally deleted (spent += 0): 3 fail. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@ally please re-review head 102f45c.
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 102f45c
One commit on top of 621b2a4e, and it takes the optional half of last cycle's finding rather than the one-sentence escape — the right call, and the shape I suggested. I re-ran your three mutants and every number reproduces: baseline 64/64, spent→0 1 fail, settleMinutes/spent swapped 1 fail, tally removed 3 fail.
The finding is a fourth mutant you did not run. The new assertion pins four of the five arguments it appears to pin, and the fifth is the one that carries the priority cohort.
Prior Findings Dispositioned (1)
- prior:621b2a4 important 1 — fixed —
scripts/land-clean-prs.mjs:856— the carry is pinned.repoRunnerat:856-864ismain's old closure verbatim plus arun = runRepodefault, andmain:889now composes it, so the extraction is behaviour-preserving (console.log(""), the return, thetry/finallyaroundreportRottedall unchanged). Measured at this head: the exact mutant I published last cycle —run(repo, apply, settleMinutes, 0, rotted), cap silently per repo — now failsland-clean-prs.test.mjs:295, where at621b2a4eit was 63/63 green. An argument-order slip (settleMinutes/spentswapped) fails the same test. The disguise is gone too: the test that reads as end-to-end coverage now is coverage.
Critical Issues (0)
None.
Important Issues (1)
-
[native-codex]
scripts/land-clean-prs.test.mjs:306— the five-elementdeepEqualpins four arguments.rottedis[]at assert time anddeepEqualis structural, so position 5 matches any empty array — including a different one.Measured, same tree, baseline 64/64:
run(repo, apply, settleMinutes, spent, [])→ 64/64 green. That is a real regression and it is silent.runRepo:808is the only thing that pushes intorotted,main:878owns the array, andmain:891reads it in thefinally. HandrunRepoa fresh array per repo andreportRottedsees an empty one every time — the BLO-33208 priority cohort vanishes from the fire with no error, no empty-section marker, nothing.reportRottedreturns early on length 0, so a fire that dropped the whole cohort is byte-identical to a fire that legitimately had none.Nothing else covers it:
runRepois unexported (:804), so no test reaches the push site, and the other twoper-fire captests stub the step entirely. This is the same class the PR exists to close — a receipt confidently reporting something it never did — one argument to the left of the one you pinned.To be clear about what this commit did and did not introduce:
main's inline closure threadedrottedjust as unpinned before the extraction, so the coverage gap is pre-existing. What is new is a test whose literal reads as a full argument-list assertion and is not one, which is exactly the "a future reader reasonably concludes it is pinned" problem that made last cycle's finding worth raising.- One line:
assert.equal(calls[0][4], rotted)after thedeepEqual. Identity is the right assertion here — it also catches[...rotted], a copy that loses every push just as silently and that a non-empty fixture would wave through. Seedingrottedwith a sentinel instead is cheaper to read but only catches the fresh-array mutant.
- One line:
Suggestions (3)
Carried unchanged; re-stating with this head's line numbers rather than re-raising. All three verified still present at 102f45c7.
- [code]
scripts/land-clean-prs.mjs:563— the stray double blank is still the only one in the file (one hit across 903 lines). No prettier/eslint/biome in the repo, so nothing in CI collapses it. - [tests]
scripts/land-clean-prs.mjs:735— thecodeowner-review-requestedbranch is still the onecommentOncecall site with noapplyRowtest, so the uncovered path shells out to the realgh. - [errors]
scripts/land-clean-prs.mjs:820— the fatal-abort branch still overwritesrow.detail, losing the classification that produced the aborting row, whilemarkFailureat:796deliberately appends. Two failure paths in the samecatchdisagreeing about whether prior detail survives.
Strengths
- The mutation table reproduces to the number, third cycle running. Publishing counts that survive an independent re-run is what keeps these reviews cheap — I spent my budget on a mutant you had not tried instead of re-deriving yours.
- Taking the seam rather than the one-sentence comment. Both discharged the finding; only one of them gives the next edit a failing test, and the sentence would have aged into the same doc-comment-asserting-the-wrong-thing shape as the original
UNLANDABLE_MERGE_STATESheader. run = runRepoas a default parameter rather than a mock registry or an injected module — the seam costs three lines and production still reads as a direct call.- The doc comment at
:851-855names the mutant (a literal 0 here makes the cap per repo again) rather than restating the code. That is the sentence a future reader needs, and it is at the definition site. - The spy returning
classifyAll(..., { spent: args[3] })rather than a canned row set means the second call's expected3is derived by the real classifier, so the assertion cannot drift out of agreement with the cap logic it is guarding.
Recommended Action
- Address the Important issue this cycle — one assertion. Production behaviour is correct; this is closing the gap between what the new test asserts and what it appears to assert.
- Consider Suggestions opportunistically.
Merge gate at this head: the 13 CI checks from run 36573245226 are queued/in-progress, mergeStateStatus is BEHIND, and reviewDecision is empty (no required-review protection on this branch). gate/ally-comment-findings is red because no review attested this head; it will stay red while the Important above is open.
…test deepEqual is structural, so the empty rotted array matched any empty array, and a runRepo call handed a fresh or copied array stayed green. main's finally reads the array runRepo pushes into, so the test now asserts identity for every call. Both mutants ([] and [...rotted]) go red; 64/64 on the real code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@ally please re-review head 3fe3c42. It addresses the Important at 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3fe3c42
Prior Findings Dispositioned (1)
- prior:102f45c important 1 — fixed —
scripts/land-clean-prs.test.mjs:312—for (const call of calls) assert.equal(call[4], rotted);runs after the structuraldeepEqual, asserts identity, and covers everyrunRepocall rather than justcalls[0]— a superset of what I asked for. I re-ran the mutants myself against the tree at this head rather than taking the claim: baseline 64/64;run(repo, apply, settleMinutes, spent, [])→ 63/1;run(repo, apply, settleMinutes, spent, [...rotted])→ 63/1. Both numbers match what you published. The copy mutant is the one a non-empty fixture would have waved through, so identity was the right assertion. The prior cycle's carry guard is also still live:spent→0remains 63/1.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (1)
- [pr-review-toolkit:code]
scripts/land-clean-prs.mjs:562-563— a stray second blank line between theUNLANDABLE_MERGE_STATESstop and theenqueuereturn, added by this PR (the diff carries a bare+on an empty line). Nothing depends on it andpolicyis green at this head, so it is noise rather than a gate — worth dropping on the next touch so the final diff is all signal.
Strengths
- Every behaviour this PR introduces has a failing mutation, and I verified that independently rather than reading the test names. Measured at this head against baseline 64/64: dropping
--rebase→ 63/1; reverting the"merge requested"outcome string → 63/1; deletingmarkFailure's action rename → 61/3; flipping the tally tostartsWith("enqueue")→ 63/1; droppingUNSTABLEfromUNLANDABLE_MERGE_STATES→ 62/2. That last one is cycle 1's finding still holding four heads later. - The two failure strings the design depends on being survivable are in fact survivable.
applyRownow reaches GitHub where it previously died client-side, soisFatalGhError'sHTTP 40[13]arm decides whether one awkward PR aborts the whole sweep. I exercised it:"--merge, --rebase, or --squash required"→ non-fatal,"Protected branch rules not configured for this branch (enablePullRequestAutoMerge)"→ non-fatal, while genuineHTTP 403and rate-limit messages still abort. The documented "we attempt, GitHub rules,markFailureputs the verdict in the receipt" contract holds end to end. - The hardcoded
--rebaseis safe across the repos this sweep can actually reach. I checked rather than assumed, since the method is now fixed rather than inferred:allow_rebase_merge: trueon paperclip, multicast, trafficcontrol, onprem-k8s and pim-multicast-gateway. A repo with rebase disabled would turn every clean PR into a permanentenqueue-failed, so this is worth a glance before the sweep list grows — but it is not a live risk today. - The
--autodoc comment is the strongest part of the change: it namesisImmediatelyMergeable, separates the client-side refusal from the server-side one, and states plainly that supplying the method converts a permanently-failing no-op into a real merge on an unprotected base. Accepting that risk explicitly, with the classifier named as the gate and aponytail:upgrade path, is the right shape — and the gate it leans on is the same one cycle 1 pinned. repoRunnerismain's old closure verbatim plus arundefault, so the seam that made all of this testable did not change production behaviour to get there.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
This branch has not been deployed
1 participant
Thinking Path
Linked Issues or Issue Description
approval-rotted+ multi-repo sweep). No overlap in the lines changed here — it does not touch theapplyRowarming call. Its multi-repo direction is why this fix deliberately does not model queue eligibility in the classifier; see Risks.What Changed
applyRowpasses--rebasewhen arming.gh pr merge --autoonly infers a merge method when the base ref has a merge queue (isMergeQueueEnabledis a base-branch property, not a PR property), so a stacked PR fell through to classic auto-merge and was refused.actionvia a newmarkFailure, not only indetail.renderReceipttallies by action, so a failure left in free text is invisible to the summary line — which is exactly how this survived 9 fires.merge requested, notauto-merge armed. See Risks: the same call queues, arms, or merges outright depending on the base, and naming the wrong one would repeat the defect being fixed.applyRowaccepts an injectable command runner and is exported, so the arming argv is testable. It previously had no test coverage at all.Verification
node --test scripts/land-clean-prs.test.mjs— 36 pass, 0 fail (was 31; 5 new).Every guard in this diff was mutation-tested: each was reverted alone and the suite confirmed to go red, per the standing rule that a regression fixture can pass on broken code.
--rebaseauto-merge armedactionrelabel inmarkFailuredetailpreservation inmarkFailureMeasured against live GitHub on 2026-09-26, which is how the mechanism was established rather than inferred:
--auto(old) on #2020, base = feature branch--merge, --rebase, or --squash required, exit 1 — bug reproduced--auto --rebaseon #1985, base =master(queue-enabled, DIRTY)The merge strategy for master is set by the merge queue, exit 0, armed withmergeMethod: REBASE— the flag is discarded, so queue rows are unaffected--auto(old) on #1906, base = feature branch--auto --rebaseon #1906gh, then GitHub refuses:Protected branch rules not configured for this branch (enablePullRequestAutoMerge)Probe state was restored (
--disable-auto); #1985 and #1906 both readautoMergeRequest: nullafterwards.Risks
This can merge a PR immediately, and that is the deliberate part to review.
ghdrops the auto-merge request and merges on the spot when the PR is already mergeable and the base has no queue to wait for. Measured: the new call landed #2020 outright (parents=1, noauto_merge_enabledevent). So supplying the method converts a permanently-failing no-op into a real merge for clean PRs on unprotected bases.Accepted on purpose — on such a base there is no queue to land through, so merging is landing, and this classifier is the gate: Ally clean at the exact head, every check green, owner approvals satisfied, mergestate CLEAN, capped at
MAX_ENQUEUES_PER_FIRE(10) per fire. A queue-enabled base is unaffected, becauseghpicks the queue path before the mergeability test.Why the classifier does not simply skip these rows. That was implemented and then reverted. It requires knowing each repo's enqueueable base, and #1957 is about to sweep multiple repos —
Blockcast/multicast's default branch ismain, notmaster, so a hardcoded base would have silently skipped every PR in that repo. A wrong model that disables a whole repo is worse than the bug being fixed. GitHub is the authority here; the script attempts, GitHub rules, and the verdict is now visible in the receipt. The upgrade path, if the doomed call per stacked PR ever matters, is recorded as aponytail:comment.Low risk to existing behaviour: the 6 historical
CLEANrows all targetedmasterand took the queue path, which the measurement above shows is unchanged.Model Used
claude-opus-5[1m], 1M context), extended thinking, with tool use (ghCLI, GitHub REST/GraphQL,node --test).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateapplyRowdoc comment