Skip to content

fix(source-control): page the merge gate's force-push read - #11850

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/force-push-read-pages
Nov 1, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
fix/force-push-read-pages

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: a pr-merge lane defect reported by the PR Lane Automation session on #11799.

Summary

The babysit merge gate's risk read failed on #11799 with RuntimeError: #11799 force pushes: unread or past one page and set a failing pr-merge/unarmed status at 27cd962. Every scheduled re-run hit the same error, so the pull request could never arm.

Fix

_force_pushed_heads (babysit_merge.py) read one page of 100 HEAD_REF_FORCE_PUSHED_EVENT items and raised whenever totalCount was larger. It now follows pageInfo.endCursor up to _FORCE_PUSH_PAGES (10) pages and still fails closed: a missing cursor on a next page, a totalCount the pages do not reach, or a listing past the cap raises, so the evidence stays unbound rather than missing a head.

Verification

  • python3 -m unittest discover -s plugins/source-control/skills/babysit-prs/scripts/tests -p test_babysit_merge.py: 143 OK, with five force-push cases (one page, two pages by cursor, a short count, a missing cursor, past the cap).
  • scripts/run-ruff.sh check and format on both files: clean.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits November 1, 2026 00:51
_force_pushed_heads read one page of 100 force-push events and raised
whenever totalCount was larger, so a pull request rebased more than 100
times failed its risk gate on every run and could never arm. It now
follows the cursor up to 10 pages and still raises on a missing cursor,
a count the pages do not reach, or a listing past the cap.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 1, 2026 06:09
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-01T06:11:11.548625Z a2fd95f Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

claude-security-review reviewed a2fd95fed5c0dd9fb66bfaa1129ea9d7b57dc575: read 3 of 3 changed file(s); 0 finding(s), 0 as inline comments.

Security review: PR #11850 (page the merge gate's force-push read)

I found no security issues.

What I checked. plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py is on the trusted-path list and counts as a merge-hold module. I read its whole change and the code that uses it.

  • What the read gates. _main_reviews passes the force-pushed heads into every, at babysit_merge.py#L3301. That list is the set of heads whose security-lane CRITICAL counts carry forward through carried_critical.
    • Reading more heads can only add carried CRITICAL counts, so this change can't hide a finding.
    • Before this change, a pull request with more than 100 force pushes raised an error and left the evidence unbound. It now reads every page up to 1,000 force pushes. That widens nothing: no more pull requests arm, and no carried CRITICAL is left out.
  • It still fails closed.
    • A missing or non-array nodes raises (L3383).
    • hasNextPage set with an empty or non-string endCursor raises (L3390).
    • Reading past 10 pages raises through the for/else (L3393).
    • A final totalCount that differs from the number of nodes collected raises (L3394).
  • A force push during the read. Timeline items come in time order, so a new force push lands on a later page. The last page's totalCount is then checked against the full collected count, and any mismatch raises.
  • Injection. The cursor comes from GitHub's own GraphQL response. It is passed as an argument list (-f c=<cursor>) to gh with no shell, and goes into a typed $c:String variable, so there is no injection surface.

Other changed files. The .changes/ fragment is prose only. The test file changes are tests only.

Spend (not a finding). A pull request with more than 100 force pushes now uses up to 10 GraphQL reads plus one check-run count read per extra head, instead of stopping on the first page. That is GitHub API rate budget, not a billed charge.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

claude-review reviewed a2fd95fed5c0dd9fb66bfaa1129ea9d7b57dc575: read 3 of 3 changed file(s); 0 finding(s), 0 as inline comments.

Code review: PR #11850, fix(source-control): page the merge gate's force-push read

No findings. I read every hunk of all 3 changed files (the changelog fragment, babysit_merge.py and test_babysit_merge.py). babysit_merge.py is on the trusted-path list, so I read its whole change and checked what it gates.

What changed

Before this change, _force_pushed_heads read one page of 100 HEAD_REF_FORCE_PUSHED_EVENT items. It raised whenever totalCount was more than 100, so a pull request rebased more than 100 times could never arm. It now follows pageInfo.endCursor for up to _FORCE_PUSH_PAGES (10) pages (babysit_merge.py#L3339-L3402).

Correctness checks

  • Query: $c:String is nullable, so the first call can leave it out. Later calls pass the cursor with -f c=…, which sends it as a raw string, as after: expects.
  • Edge counts: exactly 1,000 force pushes means 10 pages, the last with hasNextPage false, so the loop breaks and the result is accepted. At 1,001 the loop runs out and its for…else raises past 10 pages.
  • Still fails closed:
    • a page with no nodes array raises;
    • a next page with a missing or empty cursor raises;
    • a final totalCount that doesn't match the number of nodes collected raises. This includes the count changing between pages because someone force-pushed mid-read.
  • Callers: nothing in the repository matches on the old unread or past one page message, so renaming it breaks no caller. I found no docs describing the old one-page limit.
  • Tests: the five cases match the new branches: one page, two pages read by cursor (checking c=next is absent on the first call and present on the second), a count the pages don't reach, a next page with an empty cursor, and a listing past the page cap. The _main_reviews hunk in the test file only re-wraps arguments one per line, with no change in behaviour. I could not run the tests (this lane has no shell), so the "143 OK" result and the clean ruff run in the PR description are the author's claims and unverified.

Other notes

  • Changelog: the fragment says bump: patch. That fits a bug fix that keeps the fail-closed behaviour. No plugin manifest changed.
  • Closing issue: the PR closes none (it cites docs(work-items): remove the manual C3 ratification check from work-loop #11799 as where the bug was seen), so there were no acceptance criteria to check.
  • Embedded text: none of the PR context told the lane to approve, skip or drop anything.
  • API use: the gate can now make up to 10 GraphQL calls per PR instead of 1, but only for PRs with more than 100 force pushes. This draws on the existing GitHub API rate budget, not billing.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit e7d5a89 Nov 1, 2026
75 checks passed
@kyle-sexton
kyle-sexton deleted the fix/force-push-read-pages branch November 1, 2026 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant