Skip to content

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

Description

@kyle-sexton

Found by the independent verifier on #2316 (verdict posted at pull/2316#issuecomment-5263278393). Not a defect #2316 introduced — an upstream API ceiling it inherited — but it makes a stated invariant false at the top end.

The invariant

plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py:483 asserts, in the docstring for fetch_pull_request_commits:

the caller enforcing a signature rule may only ever over-report

That is the right posture, and #2316 implements it carefully everywhere it controls: a missing or non-boolean verification block reads verified=false, reason="unreadable" rather than being skipped, and a fetch failure holds with "could not be read" rather than fabricating unsigned.

Where it stops holding

The fetch is:

rows = fetch_paginated_api(
    f"repos/{repo}/pulls/{number}/commits?per_page=100",
    f"{repo}#{number} commits",
)

per_page=100 with --paginate --slurp correctly walks Link headers, so the >30 truncation class is genuinely closed. But GitHub documents GET /repos/{owner}/{repo}/pulls/{n}/commits as returning a maximum of 250 commits, directing callers past that to the List commits endpoint. Pagination does not lift that ceiling.

So on a PR with more than 250 commits, an unsigned commit at position 251+ is silently dropped, and the wrapper reports a clean signature walk that never happened. That is under-reporting — the one direction the docstring rules out.

A git grep -n 250 across skills/babysit-prs at 5a393008 returns nothing, so the cap is not acknowledged anywhere in the module.

Severity: low, and worth being precise about why

Blast radius is reporting only. GitHub still refuses the merge server-side on an unsigned commit, so the failure mode is a wrong operator-facing message on a >250-commit PR, not an unsafe merge. The guarded wrapper would report ready; the actual merge would then fail at the API with the real reason.

It is also strictly an improvement on what preceded it: requireSignatures was computed and never enforced at all (#2265), so this goes from zero coverage to 250 commits of coverage.

Suggested fix

Detect the ceiling rather than assume it is unreachable. fetch_pull_request_commits can compare the returned row count against the PR's commits field (already available from gh api repos/{o}/{r}/pulls/{n}) and, when the walk is short, emit a hold in the same shape as the existing unreadable path:

base branch requires signed commits and the commit list exceeded the API's
250-commit cap (walked N of M) -- held

That preserves the over-report-only invariant by failing closed on the one input the endpoint cannot answer, and it reuses the unreadable-style blocker rather than inventing a new state. Alternatively, fall back to GET /repos/{o}/{r}/commits for the over-cap case — heavier, and probably not worth it for a PR shape this repo has never produced.

Either way the docstring should name the ceiling, so the next reader does not inherit the same false assumption.

Verification note

The verifier's other four items on #2316 were CONFIRMED, including an executed revert-proof: reverting only the enforcement hunk moved the two relevant test modules from Ran 91 tests ... OK to FAILED (failures=2, errors=5), and reverting the mergeStateStatus string hunk as well added an eighth failure — so that line is pinned by its own test rather than riding on the enforcement one.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions