fix(source-control): fail closed when PR commit walk hits 250 cap - #2510
fix(source-control): fail closed when PR commit walk hits 250 cap#2510kyle-sexton wants to merge 1 commit into
Conversation
Fixes #2387. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ebb62fc5
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **`fetch_pull_request_commits` fails closed when GitHub's 250-commit API cap truncates the walk | ||
| (#2387).** Compares the walked count to the PR's `commits` field and raises when the endpoint | ||
| cannot return the full list, preserving the over-report-only invariant for signature enforcement. |
There was a problem hiding this comment.
This behavior was already introduced by commit 95d95a2 together with the 0.53.6 manifest bump, and the parent changelog correctly records it under that release. Moving the note here leaves 0.53.6 as an empty section and incorrectly tells users that 0.53.12 introduced the fix even though this commit changes no implementation; retain the historical note under 0.53.6 and describe the actual reason for the 0.53.12 release separately, if a new release is needed.
Useful? React with 👍 / 👎.
|
Claude finished @kyle-sexton's task in 1m 29s —— View job Security review — PR #2510Skip gate: this PR's diff has no security-relevant surface.
The actual fail-closed logic in No security issues found. Per the skip-gate in
|
|
Last security-reviewed head: |
|
Claude finished @kyle-sexton's task in 2m 11s —— View job CI Code Review
Scope: This PR touches only Finding (posted inline):
No correctness issues with the code itself — this lane found no functional code changes in scope (security-relevant logic already shipped and is out of scope for this changelog/version housekeeping PR). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -43,12 +51,6 @@ All notable changes to the `source-control` plugin are documented here. Format f | |||
|
|
|||
| ## [0.53.6] | |||
There was a problem hiding this comment.
This edit leaves ## [0.53.6] as an empty header (no entries beneath it, followed immediately by ## [0.53.5]). Elsewhere in this file, versions with nothing to document are omitted entirely rather than left as bare headers — e.g. 0.53.10 doesn't appear between 0.53.9 and 0.53.11. For consistency with that convention, consider deleting the now-empty ## [0.53.6] line along with the entry it used to hold, rather than leaving an orphan header.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Closing as duplicate: the |
…ckRollup running-check pitfall (#2513) ## Summary Lands the two PR-lifecycle facts #1436 recorded from driving #1393/#1316/#1318/#1322 to merge, in the document that was prescribing the expensive path: - **Stale-branch recovery now defaults to merge-forward.** `monitor.md`'s mergeable pre-check (§3.1 step 1) and stale-branch recovery rule (§3.2) both prescribed "force-push with lease" — but auto-mode permission classifiers commonly deny force-push, and the recorded consequence was a fresh branch + fresh PR + closing the old one per rebase (the #1315 → #1377 → #1393 churn, with every review thread re-opened on the successor). Merging the default branch *into* the PR branch resolves staleness and pushes **fast-forward** — no force-push — and under a squash-only default branch the merge commits collapse to one commit on merge, so linear-history requirements stay satisfied. Verified in the issue's own record: #1393 landed that way and #1318 was merge-forwarded five times without needing a new branch. Rebase stays available as the exception for projects requiring a linear PR branch where force-push is actually permitted. - **`statusCheckRollup` reports a running check as `conclusion: ""` (empty string), not `null`.** The complement-shaped filter (`conclusion != null and != "SUCCESS"`) therefore counts every in-progress check as a failure — the exact misreport in the issue (two "failing" checks that were simply still running). The multi-PR scan section (§3.0.6, the one place this skill reads `statusCheckRollup`) now documents the pitfall with value-positive jq selectors for "failed" and "still running". Version `0.53.11` → `0.53.14` (patch; `0.53.12`/`0.53.13` are claimed by in-flight PRs #2450/#2453/#2483/#2510 and #2469 — skipping past them per the #1746 collision pattern). ## Test plan - `npx markdownlint-cli2@0.23.2` on both edited markdown files — 0 issues. - Docs-only change to skill reference text; no scripts or hooks touched. The jq forms added are the ones from the issue, verified against `gh pr view --json statusCheckRollup` semantics. ## Related Fixes #1436 Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…ckRollup running-check pitfall (#2513) ## Summary Lands the two PR-lifecycle facts #1436 recorded from driving #1393/#1316/#1318/#1322 to merge, in the document that was prescribing the expensive path: - **Stale-branch recovery now defaults to merge-forward.** `monitor.md`'s mergeable pre-check (§3.1 step 1) and stale-branch recovery rule (§3.2) both prescribed "force-push with lease" — but auto-mode permission classifiers commonly deny force-push, and the recorded consequence was a fresh branch + fresh PR + closing the old one per rebase (the #1315 → #1377 → #1393 churn, with every review thread re-opened on the successor). Merging the default branch *into* the PR branch resolves staleness and pushes **fast-forward** — no force-push — and under a squash-only default branch the merge commits collapse to one commit on merge, so linear-history requirements stay satisfied. Verified in the issue's own record: #1393 landed that way and #1318 was merge-forwarded five times without needing a new branch. Rebase stays available as the exception for projects requiring a linear PR branch where force-push is actually permitted. - **`statusCheckRollup` reports a running check as `conclusion: ""` (empty string), not `null`.** The complement-shaped filter (`conclusion != null and != "SUCCESS"`) therefore counts every in-progress check as a failure — the exact misreport in the issue (two "failing" checks that were simply still running). The multi-PR scan section (§3.0.6, the one place this skill reads `statusCheckRollup`) now documents the pitfall with value-positive jq selectors for "failed" and "still running". Version `0.53.11` → `0.53.14` (patch; `0.53.12`/`0.53.13` are claimed by in-flight PRs #2450/#2453/#2483/#2510 and #2469 — skipping past them per the #1746 collision pattern). ## Test plan - `npx markdownlint-cli2@0.23.2` on both edited markdown files — 0 issues. - Docs-only change to skill reference text; no scripts or hooks touched. The jq forms added are the ones from the issue, verified against `gh pr view --json statusCheckRollup` semantics. ## Related Fixes #1436 Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
2 participants
Fixes #2387
Summary
fetch_pull_request_commitsnow compares the walked count to the PR'scommitsfield and raises when GitHub's 250-commit API cap truncates the list, preserving the over-report-only invariant for signature enforcement.Test plan
python3 -m pytest plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py -k pull_commits_capRelated
None.