feat(scripts): pairwise PR file-set collision instrument (closes #2072) - #2073
Conversation
…instrument Closes #2072. batch-merge-and-resolve.md requires a pairwise collision check across open PRs and CLAUDE.md's merge-order section requires deriving each PR's file set and intersecting them, and neither had a committed instrument -- so the check was run by hand every time, which is deterministic-tools.md's stated bar for building the tool. Sibling of scripts/pr-sweep.py, sharing its argument shape and its habit of reporting what it examined. The two answer different questions: pr-sweep asks which PRs are stalled, a property of each PR; this asks which PRs collide, a property of the set.
Round 1 of self-review reproduced two cases where a broken fetch printed a
PASSED control banner, reported "mergeable in any order", and exited 0 under
--strict -- defeating the one property this script exists to guarantee.
1. The completeness guard tested for an EMPTY file set, not a COMPLETE
one, so a PR reporting 3 changed files and returning 1 was accepted
and the missing file was the colliding one. Now compares the derived
count against the API's own totalCount.
2. hasNextPage with a null endCursor entered remaining_files with no
cursor, whose `while cursor:` never ran, silently dropping every
remaining file. Now raises.
3. A missing `gh` raised FileNotFoundError, which escaped main() and
exited 1 -- the "collision found" verdict. Now exits 2, matching
check-pr-fully-clean.py, with a top-level guard so no exception can
exit 1.
4. GraphQL reports a renamed file by its NEW path only, so a PR renaming
foo.yml and a PR editing foo.yml derived disjoint sets while
conflicting at merge time. previous_filename is folded in from REST,
for renaming PRs only.
5. The pullRequests connection caps first: at 100, so "raise --limit" was
unfollowable advice above that. The PR connection is now paginated.
Also: the negative control now drives the real completeness guard in both
directions rather than only the pure classifier (the classifier was never
where failures lived), and its banner no longer claims to cover fetching;
JSON emits PRs as a list of objects rather than dicts keyed by stringified
ints; a wholly underivable sweep no longer prints "(no open PRs to
compare)"; MAX_FILE_PAGES is a named constant since it bounds the
configurable --files-per-page.
Prose corrections in the same round: "a file-set intersection is a superset
of the textual conflicts" was false (renames refute it); "identical file
sets merge cleanly because both sides carry the same content" conflated set
equality with content equality, in the harmful direction of closing a PR as
a duplicate; a forward reference and two now-stale downstream claims in
batch-merge-and-resolve.md; and "deriving the same live set" was false,
since the draft default is deliberately inverted from pr-sweep.py's.
Tests: 65 -> 91, covering the fetch and pagination layer that previously had
none.
|
Working on this --- paws off until I'm done. |
…ine semicolon The local semantic-line-breaks.py joins the two clauses onto one line and reports no change needed; CI's check-new-line-breaks rejects exactly that shape. Filed the instrument disagreement as ai-config#2085; this splits the sentence so both agree.
…ion loop
Two holes found reviewing my own round-1 fixes rather than reported:
1. The RENAMED test ran over page one's nodes only, and remaining_files
returned just paths -- so a PR whose rename fell past the first page
skipped the REST lookup and lost its pre-rename path, reinstating the
blind spot for large PRs exactly where it is hardest to notice.
remaining_files now reports whether it saw a rename.
2. The new PR-pagination loop terminated only on hasNextPage going false,
with no runaway guard, unlike the file loop beside it. A hung sweep
reports nothing at all, which is worse than one that fails.
Completeness is deliberately checked BEFORE the rename fold: a pre-rename
path is an addition totalCount never counted, so comparing after the fold
could let a short set make up its shortfall with rename aliases.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_cc65315b-f690-4a65-9fe4-a76fa3b83d22) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…alse-clean Round 2 of adversarial self-review, against 71bf6f9. Three findings reopened the one property this script exists to guarantee -- that a broken or incomplete fetch cannot read as "no collision". 1. FILES_QUERY never selected `changeType`, so the page-two rename fold added in 71bf6f9 could not fire: `saw_rename` was permanently False past the first file page. The fix existed and never executed. 2. The test certifying that fix passed against a fixture that INVENTED `changeType`, a field the real query did not request. This is fixtures-are-not-evidence exactly -- the fixture was named after real output and vouched for a shape it did not have. Fixed by selecting the field, and pinned by new checks asserting every field the fixtures inject is actually selected by the query they claim to imitate, so the fixture cannot drift ahead of the query again. 3. `fetch` counted raw nodes rather than distinct PR numbers, so a connection re-serving a page inflated the count past `totalCount` and set `ok` on a sweep that had seen 100 distinct PRs out of 250 -- exiting 0 while the headline said "examined 100 of 250". Now keyed by PR number, so the runaway guard catches it loudly instead. Also from that round: - `run_gh` could return None. `text=True` decodes with the platform encoding, and a byte cp1252 cannot decode leaves stdout=None with returncode 0, so the return-code test passed and callers got None, crashing outside SweepError and killing the whole sweep rather than reporting one PR underivable. Now decodes UTF-8 explicitly and treats a None stdout as an error. This is the same defect ai-config#2086 is currently removing from pr-sweep.py. - `-F owner=`/`-F name=` applied gh's magic type conversion, so a repo named `owner/2024` was rejected by the String! variable. Now `-f`. - A null or number-less PR node crashed; it is now reported as incompleteness, since skipping it would shrink the population silently. - The null-cursor end-to-end test used total=200, so the completeness guard fired first and the test passed with the guard it names removed. Now total=1, and mutation-tested: reverting the guard turns it red. - Off-by-one in the MAX_FILE_PAGES comment (101 files, not 100). - pr-status-all/SKILL.md now lists the instrument beside its sibling. - CLAUDE.md leads with the script rather than the hand method, and says the hand method misses renames. Both critical fixes are mutation-tested: reverting FILES_QUERY's changeType selection, or the null-cursor guard, each turns the suite red. Tests: 92 -> 103.
Adversarial self-review round 2 found three critical defects at
|
2026-08-24 06:30 UTC |
|
Claude finished review — View run Code Review — PR #2073Scope of this round: The PR's most recent Claude review (posted 2026-08-24T06:09:47Z, commit What changed:
Also bundled: Verification performed:
No correctness bugs, no CLAUDE.md violations, and no hallucinated APIs/fields found in this incremental commit. The fixes are genuine (not cosmetic) and each is backed by a new test that would fail if the fix were reverted, per the commit's own mutation-testing claim, which I found plausible on inspection of the guard logic (e.g. VerdictReady for merge Reviewed commit: 03c3eba Reviewed commit: 03c3eba |
Merge order: no constraint, derived rather than recalledRe-derived at head The second one is new since the earlier review, and this PR created it: addressing a review finding added an entry to A shared file is not a conflict, so I ran the merge rather than stopping at the intersection. Negative control first, since a zero matrix is otherwise indistinguishable from a detector that never ran: Both share a file in disjoint regions: #2079 edits Per the boundary this PR's own script prints, that is a statement about collisions and not about dependencies. Neither PR asserts anything the other makes true, so far as I can tell. One aside worth recording, since it landed on the exact defect under discussion. The first attempt at the merge test above crashed with the cp1252 bug this PR fixes --- my throwaway script used That is the same failure #2086 removes from |
1 participant
Closes #2072.
Adds
scripts/pr-overlap.py, a sibling ofscripts/pr-sweep.pythat answers the other set-level question: which pairs of open PRs share a file.shared/workflow/batch-merge-and-resolve.mdrequires a pairwise collision check across open PRs, andCLAUDE.md's merge-order section requires deriving each PR's file set and intersecting them rather than recalling what each PR is "about". Neither had a committed instrument, so the check was run by hand every time.Reproduces the issue's cited figures exactly
d-morrison/altdocd-morrison/rmeIt also reproduces the substantive findings: the four altdoc PRs with identical file sets (#105, #108, #111, #112) surface as one duplicate cluster, and rme's
_sec_linreg_mle_est.qmdshows as edited by four separate PRs with #1091 the only independently mergeable one.Where the prototype's approach turned out to be wrong
Three things, each of which the committed version does differently.
Its negative control could not fail. The prototype's control was
f and (f & f) == f, which in Python reduces tobool(f)for every set --- exactly the "perfect impostor"batch-merge-and-resolve.mddescribes, which "runs the real command, against real refs, and returns exactly the clean result a working detector would". It validated nothing about the collision detector. The committed control runs the real pair classifier over fixtures known to collide, to be identical, and to be disjoint, plus the real completeness guard in both directions, and refuses to examine any repo if one is misreported. A test asserts the control fails against a deliberately broken classifier, which is the property the prototype's version lacked.A
ghfailure silently shrank the population. The prototype caught the error per PR, printed a line, and dropped that PR from the comparison --- so the examined count quietly fell and the run still exited 0. An underivable file set is now reported by name, the pairs it would have participated in are counted as not examined, and the run exits 2.It could not see a rename. Both the prototype's
gh pr diff --name-onlyand GraphQL report a renamed file by its new path only, so a PR renamingfoo.ymland a PR still editingfoo.ymlderive disjoint sets and are reported mergeable in any order, while the merge produces a rename/modify conflict. The committed version folds the pre-rename path back in from REST, for renaming PRs only.Design
pullRequestsconnection (GraphQL capsfirst:at 100).fail-fast.mdandfully-clean.md's account of collapsing "could not run" into "found something":0ran/advisory,1a real collision under--strict,2could not answer --- and2is never suppressed by the absence of--strict.Tests
scripts/test_pr_overlap.py, 91 checks, mirroringscripts/test_pr_sweep.py's structure (plain Python,check(name, condition), exit 1 on failure) and wired intovalidate.ymlbeside it. Only the tests run in CI, for the same reason the sibling gives: the script needs an authenticatedgh.Two rounds of adversarial self-review ran against the diff. Round 1 returned "Needs more work" with reproductions of two cases where a broken fetch printed a PASSED control banner, reported "mergeable in any order", and exited 0 under
--strict; both are fixed ine88f2eecand both now have regression tests. Round 1 also caught four false claims in the prose, including "a file-set intersection is a superset of the textual conflicts" (renames refute it) and "identical file sets merge cleanly because both sides carry the same content" (set equality is not content equality, and the error pointed in the harmful direction of closing a PR as a duplicate).Merge order
Derived with the script itself rather than recalled:
This PR collides with #2074 and #2079 on
CLAUDE.mdonly, in different sections (this one appends a sentence to the merge-order section). Whichever merges second resolves a one-file conflict; no ordering between them changes the result. Per the boundary the script prints, that is a statement about collisions and not about dependencies --- I am not aware of a dependency in either direction.Boundary
File-set intersection finds collisions. It cannot see a dependency, where one PR asserts something another makes true, since those PRs' file sets never overlap.
CLAUDE.md's merge-order section already says so; the script prints it on every run rather than letting its own silence imply otherwise.Note
Low Risk
New read-only reporting script plus docs and CI tests; it never mutates PRs or secrets. Failure modes are designed to fail closed (exit 2) rather than report a false all-clear.
Overview
Adds
scripts/pr-overlap.py, a read-only sibling ofpr-sweep.pythat derives every open PR's file set and reports which pairs collide — the checkbatch-merge-and-resolve.mdrequired but previously ran by hand.It classifies pairs as identical (duplicate clusters), overlap (needs a merge order), or disjoint, and never treats an empty diff as "mergeable in any order." Drafts are included by default (the inverse of
pr-sweep.py), because a draft collides at merge time the same way a ready PR does. Incomplete fetches, truncated PR lists, andghfailures exit 2 rather than looking like a clean queue. Renames fold in the pre-rename path from REST so a rename/modify pair is not reported disjoint.A negative control that can actually fail runs before any repo is examined. Offline tests (
test_pr_overlap.py) are wired intovalidate.yml. Docs inCLAUDE.md,batch-merge-and-resolve.md, andderive-dont-enumerate.mdpoint at the script and restate its boundary: intersection finds collisions, not dependencies.Reviewed by Cursor Bugbot for commit 71bf6f9. Bugbot is set up for automated code reviews on this repo. Configure here.