Split out of the review of the #302 fix series, deferred to keep that PR focused.
Problem
In the check_run.rerequested handler, we resolve the pull request via:
context.payload.check_run.pull_requests[0]
check_run.pull_requests[] can legitimately contain more than one entry when a single commit is the head of several pull requests. Index 0 has no guaranteed relationship to the check run that was actually re-run, so we may evaluate the wrong PR.
This matters because different PRs can have different base branches, and the base determines the merge base and therefore the set of commits DCO evaluates. A commit set that passes against one base can fail against another, so we can publish a verdict that is simply wrong for the check the user clicked.
Why it is deferred rather than urgent
Several pull requests sharing a head SHA is uncommon in practice, and the handler itself only started working at all as part of the #305 fix (the re-run button had never worked). So this is a latent correctness gap rather than an active regression.
Suggested fix
Persist the originating pull request number in external_id when creating the check run, then read it back on re-request instead of guessing. For pre-existing check runs that have no external_id and an ambiguous pull_requests array, publish a diagnostic result rather than silently picking the first entry.
Note this touches every check we create, which is why it was kept out of the #302 series.
Related
Split out of the review of the #302 fix series, deferred to keep that PR focused.
Problem
In the
check_run.rerequestedhandler, we resolve the pull request via:check_run.pull_requests[]can legitimately contain more than one entry when a single commit is the head of several pull requests. Index0has no guaranteed relationship to the check run that was actually re-run, so we may evaluate the wrong PR.This matters because different PRs can have different base branches, and the base determines the merge base and therefore the set of commits DCO evaluates. A commit set that passes against one base can fail against another, so we can publish a verdict that is simply wrong for the check the user clicked.
Why it is deferred rather than urgent
Several pull requests sharing a head SHA is uncommon in practice, and the handler itself only started working at all as part of the #305 fix (the re-run button had never worked). So this is a latent correctness gap rather than an active regression.
Suggested fix
Persist the originating pull request number in
external_idwhen creating the check run, then read it back on re-request instead of guessing. For pre-existing check runs that have noexternal_idand an ambiguouspull_requestsarray, publish a diagnostic result rather than silently picking the first entry.Note this touches every check we create, which is why it was kept out of the #302 series.
Related