Repository navigation
pr ready --required-only: a guard that cannot run, on the path that merges - #3227
Merged
Merged
Conversation
The units fan-out flagged prcheck.rs and I had not checked it. Checking
found a guard that is wrong on every real input.
`merged_recently` asks the API for CLOSED pull requests and keeps the
MERGED ones, then calls read_is_complete(rows.len(), page) -- the merged
count against the closed page size. `read_is_complete` is
`returned < limit`, "the page was not full so the read saw everything",
and closed is a superset of merged.
Measured on gHashTag/t27:
per_page closed returned merged guard page full
30 30 29 COMPLETE YES
60 60 59 COMPLETE YES
90 90 88 COMPLETE YES
The page was full every time and the guard said complete every time. It
can only say otherwise when EVERY closed pull request on the page is
merged: one unmerged row anywhere is enough to silence it forever.
Fixed by asking the completeness question of the read the PAGE bounded.
The request now returns `number<TAB>merged?` for every closed row,
read_is_complete sees the closed count, and the merged filter is applied
afterwards. Reverting the guard to the filtered count no longer compiles,
because `rows` does not exist at that point.
The first test for the merged filter rebuilt it inline from the same TSV
and a mutant removing the filter from production passed it. Extracted to
`merged_numbers`; the test calls it now. A test that reimplements the
thing it tests is a second copy agreeing with itself.
And the count of ratio prints was itself undercounted: 16 by the first
regex, 49 by a wider one, because the first required a bare `{}` and
skipped every `{named}` interpolation -- which this crate uses
everywhere. Counting the population of a units defect, in the pass about
units, with a matcher that had a dead half.
SKILL 583.
Refs #3195
The ratio sweep's strongest survivor is not a malformed number. It is a
merge.
`tri pr ready --required-only --wait` printed `{p} of {total}` where p
counts required check RUNS still going on the head commit and total was
req.len() -- required context NAMES from repository SETTINGS, which never
look at the commit.
The consequence is not the arithmetic. required_pending returns None for
an empty ruleset and the caller bails, so total >= 1 on every path that
reaches the print, which makes this arm unreachable in that mode:
} else if total == 0 {
// An empty list is not "finished" -- it is "not started".
quiet += 1;
if quiet >= 4 { break; }
}
That guard exists because a pull request was once merged while ten checks
were still running; its rationale is four hundred lines up in the same
file. Under --required-only it could not execute at all, and `quiet` --
incremented only inside it -- was never incremented in that mode.
So: run the flag seconds after a push, before any required context has
posted a run. p = 0, honestly, because nothing required is running.
total = 4, from settings. Control falls to `else { break }` on the FIRST
poll, the final read repeats the zero, and the verdict reads safe to
merge into `gh pr merge --squash`. The wait exits before the checks it
exists to wait for exist.
required_posted counts how many required NAMES have posted a run on this
commit. Same query as the numerator, so it can be zero -- which revives
the not-started arm -- and the arm now covers the partial case too,
because zero pending out of three posted says nothing about the fourth.
req.len() is still printed as its own clause: "3 of 3 posted are still
running; the ruleset requires 4".
The claim arrived with two wrong supporting details and the adversarial
pass dropped both: rings-rust.yml does set a `name:`, so three workflows
emit a `build` check run rather than four, and `build` is not required,
so those duplicates can never reach the numerator. p > total is a latent
property of a row-counting function, not a state this repository is in
today. The finding survives on the dead guard, not on the arithmetic.
The loop is I/O-bound, so all four unit tests stay green when either half
is reverted; both are pinned structurally. Eleventh pass running that the
wiring outlived the function, and the first where that wiring ends in
`gh pr merge`.
SKILL 586.
Refs #3195
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
# Conflicts: # .claude/skills/ci-gates/SKILL.md
Contributor
PR DashboardGenerated at: 2026-09-05 02:46:23 UTC
Summary
Seal Status
|
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
# Conflicts: # .claude/skills/ci-gates/SKILL.md # cli/tri/src/prcheck.rs
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-09-05 03:09:22 UTC
Summary
Seal Status
|
This was referenced Sep 5, 2026
Contributor
PR DashboardGenerated at: 2026-09-05 03:37:23 UTC
Summary
Seal Status
|
# Conflicts: # .claude/skills/ci-gates/SKILL.md
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
This was referenced Sep 5, 2026
fix(rust): lower Zig's undefined to Default::default() (+2) — NOT auto-merged, needs your call
#3223
Closed
Contributor
PR DashboardGenerated at: 2026-09-05 04:10:14 UTC
Summary
Seal Status
|
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
This was referenced Sep 5, 2026
# Conflicts: # .claude/skills/ci-gates/SKILL.md
This was referenced Sep 5, 2026
Contributor
PR DashboardGenerated at: 2026-09-05 05:19:05 UTC
Summary
Seal Status
|
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-09-05 05:52:34 UTC
Summary
Seal Status
|
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-09-05 06:09:16 UTC
Summary
Seal Status
|
This was referenced Sep 5, 2026
# Conflicts: # .claude/skills/ci-gates/SKILL.md
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-09-05 06:44:02 UTC
Summary
Seal Status
|
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-09-05 07:16:36 UTC
Summary
Seal Status
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ratio sweep's strongest survivor is not a malformed number. It is a merge.
tri pr ready --required-only --waitprinted{p} of {total}wherepcounts required check runsstill going on the head commit, and
totalwasreq.len()— required context names read fromrepository settings, which never look at the commit.
The consequence is not the arithmetic
required_pendingreturnsNonefor an empty ruleset and the caller bails, sototal >= 1on everypath that reaches the print — which makes this arm unreachable in that mode:
That guard exists because a pull request was once merged while ten checks were still running — its
rationale sits four hundred lines up in the same file. Under
--required-onlyit could not execute atall, and
quiet, incremented only inside it, was never incremented in that mode.So: run the flag seconds after a push, before any required context has posted a run.
p = 0— honestly, nothing required is running.total = 4— from settings.else { break }on the first poll.safe to mergeintogh pr merge --squash.The wait exits before the checks it exists to wait for exist.
Fix: the denominator comes from the same read as the numerator
required_postedcounts how many required names have posted a run on this commit. Same query asp, so it can be zero — which revives the not-started arm — and the arm now covers the partial casetoo, because zero pending out of three posted says nothing about the fourth.
req.len()is still printed, as its own clause: "3 of 3 posted are still running; the rulesetrequires 4" is the state a reader has to be able to see.
What the adversarial pass corrected in its own finding
The claim arrived with two supporting details that were wrong, and the verifier dropped both:
rings-rust.ymldoes set aname:, so three workflows emit abuildcheck run rather than four —and
buildis not a required context, so those duplicates can never reach the numerator.p > totalis a latent property of a row-counting function, not a state this repository is intoday. The finding survives on the dead guard, not on the arithmetic — and saying which of the two
carried it is the difference between a report and a rumour.
Verification
The loop is I/O-bound, so all four unit tests on
required_posted/required_pendingstay green wheneither half is reverted. Both are pinned structurally, and the structural test also asserts the arm is
reached before anything that merges.
req.len()required_postedalways returns the full ruleset size749 crate tests. SKILL 586. Refs #3195