fix(source-control): fail closed when PR commit walk hits API cap

[kyle-sexton]

Fixes #2387.

Compares walked commits against the PR commits field and raises when GitHub's pull-commits endpoint returns fewer rows than the PR reports (250-commit ceiling).

Tests: test_babysit_gh.FetchPullRequestCommitsTests.

Related

• source-control:babysit-prs: the signature walk inherits the PR-commits endpoint's 250-commit cap, making its own 'may only ever over-report' invariant false above 250 #2387 — babysit-prs 250-commit cap fail-closed

[cursor]

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.

[claude]

Claude finished @kyle-sexton's task in 1m 45s —— View job

---

Security Review — PR #2400

• Gather context (PR diff, changed files)
• Review babysit_gh.py changes for security issues
• Review plugin.json version bump
• Review test file changes
• Post final findings

Scope: reviewed only the 3 files changed in this PR (babysit_gh.py, plugin.json, test_babysit_gh.py) against origin/main...HEAD.

No security issues found.

This is a well-scoped fail-closed fix. Summary of what I checked and why it's safe:

• Command construction is not injectable. The new call — gh_json(["api", f"repos/{repo}/pulls/{number}", "-q", ".commits"]) — is invoked via subprocess with an argv list (["gh", *args], no shell=True; see _run_gh), so the f-string interpolation can't break out of its argument position. repo also can't smuggle shell metacharacters or extra CLI flags into the gh invocation regardless, since every call path validates it through parse_repo_number/parse_repo (GITHUB_OWNER_RE / GITHUB_REPOSITORY_RE, both anchored allow-lists with no /, \, #, or whitespace) before it ever reaches fetch_pull_request_commits. number is a plain int.
• The new truncation check can't be spoofed to fail open. expected comes from GitHub's own server-computed .commits field on the PR resource — it's not client- or attacker-suppliable — so a PR author can't understate it to mask a truncated walk. The comparison (expected > walked) correctly maps to "the pull-commits endpoint returned fewer rows than GitHub says exist" with no off-by-one gap.
• The raise genuinely fails closed. evaluate_required_signatures in babysit_merge.py catches RuntimeError from this function and turns it into a blocking "could not be read -- held" finding rather than swallowing it — I traced this caller (the only caller) to confirm the exception isn't caught-and-ignored anywhere upstream, which would have silently reintroduced the fail-open behavior this PR is trying to close.
• No workflow, permissions, or dependency-pin changes — plugin.json is a version bump only.

Nothing here falls under logic/auth/injection/secrets-handling concerns; per-scope instructions I'm not re-flagging supply-chain/permissions/trigger-pattern territory that zizmor already covers statically (not applicable here anyway — no workflow files changed).

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31575087134
• Failure class: other
• Last SDK result: (no execution file was produced)

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

• rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
• a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31575087104
• Failure class: other
• Last SDK result: (no execution file was produced)

Re-run the job to retry the review. A new push re-triggers this lane only if the caller's pull_request triggers include synchronize (the canonical caller omits it).
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).

[kyle-sexton]

Superseded by #2395 (same #2387 fix on the babysit-prs branch).