Summary
Only the first page of the commits of a pull request was checked. The commits after it were
never validated, and the pull request passed on the strength of the first page alone.
Cause
extractCommits requests payload.pull_request.commits_url with no page size:
const { body } = await got.get(prCommitsUrl, { responseType: "json" });
That endpoint is paginated and returns 30 commits by default. The Link header naming the
next page is ignored, so commit 31 onwards is not read. Nothing in the log says so: the group
of checked commit messages simply ends, and a pull request whose only offending commit is past
the boundary is reported as clean.
Observed behaviour
On a scratch pull request with 106 commits, the 104th carrying the message
invalid message on the second page (javier-godoy/test-repo#22), the check was green
before the fix. Only the first 30 messages were read, and the invalid one is not among them.
Why it matters
It fails open, and it is invisible: no warning, no count, nothing that distinguishes "all the
commits are valid" from "the commits I looked at are valid". Branches long enough to cross the
boundary are exactly the ones most likely to have collected a WIP or a malformed message on
the way.
Possible resolutions
- Follow the
Link header of the response until there is no next page. This needs nothing
else: the same request that reads the first page reads the rest, authenticated or not.
- Read the commits through the API client of the toolkit, which follows the pages with
paginate and asks for 100 at a time. The client cannot be constructed without a token, so
this route is available only if the request is authenticated. That is a separate decision,
and it is a prerequisite for this route alone — not for fixing the page limit.
- Ask for
per_page=100 without following the pages. Cheaper, and enough for almost every
pull request, but it moves the silent boundary rather than removing it.
(1) and (2) both remove the boundary and differ only in what they depend on. (3) is a stopgap,
and it should warn when it sees a rel="next" link rather than stay silent.
Note that this issue is about the page limit alone: how many commits are read. Whether the
request is authenticated is a separate matter, tracked in #8, and it decides only which of the
routes above is available — (1) works either way.
Summary
Only the first page of the commits of a pull request was checked. The commits after it were
never validated, and the pull request passed on the strength of the first page alone.
Cause
extractCommitsrequestspayload.pull_request.commits_urlwith no page size:That endpoint is paginated and returns 30 commits by default. The
Linkheader naming thenext page is ignored, so commit 31 onwards is not read. Nothing in the log says so: the group
of checked commit messages simply ends, and a pull request whose only offending commit is past
the boundary is reported as clean.
Observed behaviour
On a scratch pull request with 106 commits, the 104th carrying the message
invalid message on the second page(javier-godoy/test-repo#22), the check was greenbefore the fix. Only the first 30 messages were read, and the invalid one is not among them.
Why it matters
It fails open, and it is invisible: no warning, no count, nothing that distinguishes "all the
commits are valid" from "the commits I looked at are valid". Branches long enough to cross the
boundary are exactly the ones most likely to have collected a WIP or a malformed message on
the way.
Possible resolutions
Linkheader of the response until there is no next page. This needs nothingelse: the same request that reads the first page reads the rest, authenticated or not.
paginateand asks for 100 at a time. The client cannot be constructed without a token, sothis route is available only if the request is authenticated. That is a separate decision,
and it is a prerequisite for this route alone — not for fixing the page limit.
per_page=100without following the pages. Cheaper, and enough for almost everypull request, but it moves the silent boundary rather than removing it.
(1) and (2) both remove the boundary and differ only in what they depend on. (3) is a stopgap,
and it should warn when it sees a
rel="next"link rather than stay silent.Note that this issue is about the page limit alone: how many commits are read. Whether the
request is authenticated is a separate matter, tracked in #8, and it decides only which of the
routes above is available — (1) works either way.