Skip to content

Resolve PR by head SHA in coverage-comment workflow#847

Merged
mjp41 merged 1 commit into
microsoft:mainfrom
mjp41:coverage-ci-fix
May 8, 2026
Merged

Resolve PR by head SHA in coverage-comment workflow#847
mjp41 merged 1 commit into
microsoft:mainfrom
mjp41:coverage-ci-fix

Conversation

@mjp41
Copy link
Copy Markdown
Member

@mjp41 mjp41 commented May 8, 2026

workflow_run.pull_requests[] is always empty for PRs whose head ref lives in a fork. By GitHub design, the default-branch workflow_run handler (which holds the write token) does not receive cross-repo PR linkage. The first PR through the new coverage workflow happened to come from a fork, so the comment was posted to the tracking issue instead of the PR.

Replace the bash step with a github-script step:

  • reads github.event via an environment variable, immune to payload-quoting issues;
  • first tries workflow_run.pull_requests[0] for the same-repo branch case;
  • falls back to GET /repos/{owner}/{repo}/commits/{sha}/pulls (the "pull requests associated with commit" API), which is the only way to recover a PR number from a fork workflow_run payload;
  • prefers an open PR but tolerates a closed one if there's a squash-merge race between the build and this comment job;
  • falls through to the tracking-issue path only when neither source produces a PR.

The downstream "Comment on PR" step is unchanged — it consumes the same steps.pr.outputs.pr it always did.

Two issues with the previous "Resolve PR number" step:

1. Inline `${{ toJson(github.event) }}` interpolation inside `'...'`
   shell quoting could be broken by any single quote in the payload
   (e.g. an apostrophe in a commit message), producing the
   notorious `syntax error near unexpected token "else"`.

2. `workflow_run.pull_requests[]` is *always empty* for PRs whose
   head ref lives in a fork. By GitHub design, the default-branch
   `workflow_run` handler (which holds the write token) does not
   receive cross-repo PR linkage. The first PR through the new
   coverage workflow happened to come from a fork, so the comment
   was posted to the tracking issue instead of the PR.

Replace the bash step with a github-script step:

  - reads `github.event` via an environment variable, immune to
    payload-quoting issues;
  - first tries `workflow_run.pull_requests[0]` for the same-repo
    branch case;
  - falls back to
    `GET /repos/{owner}/{repo}/commits/{sha}/pulls` (the
    "pull requests associated with commit" API), which is the only
    way to recover a PR number from a fork `workflow_run` payload;
  - prefers an open PR but tolerates a closed one if there's a
    squash-merge race between the build and this comment job;
  - falls through to the tracking-issue path only when neither
    source produces a PR.

The downstream "Comment on PR" step is unchanged — it consumes the
same `steps.pr.outputs.pr` it always did.
@mjp41 mjp41 merged commit 6e0989c into microsoft:main May 8, 2026
195 checks passed
@mjp41 mjp41 deleted the coverage-ci-fix branch May 8, 2026 08:24
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

Coverage report (cross-platform merged)

Lines covered (src/snmalloc/**): 2788 / 3178 (87.73%)

Merged line coverage is the per-line union across all platforms. Region coverage is reported per-platform only; no cross-platform region total is computed.

Per-directory breakdown

Directory Lines covered Lines executable %
src/snmalloc/mitigations 8 18 44.44%
src/snmalloc/stl 2 4 50.00%
src/snmalloc/override 94 156 60.26%
src/snmalloc/pal 330 449 73.50%
src/snmalloc/ds_core 340 399 85.21%
src/snmalloc/aal 51 59 86.44%
src/snmalloc/global 264 303 87.13%
src/snmalloc/ds 328 357 91.88%
src/snmalloc/backend_helpers 336 356 94.38%
src/snmalloc/mem 837 872 95.99%
src/snmalloc/ds_aal 108 112 96.43%
src/snmalloc/backend 90 93 96.77%
Per-platform contributions (advisory)
Platform Lines covered Lines executable Lines % Regions covered Regions executable Regions %
freebsd-14 4177 4680 89.25% 3782 5609 67.43%
linux-self-host-shim-checks 4261 4736 89.97% 3881 5909 65.68%
linux-self-host-shim-checks-selfhost 1640 2323 70.60% 1498 2911 51.46%
macos-14 4213 4664 90.33% 3841 5628 68.25%
windows-2022 4119 4684 87.94% 3774 5590 67.51%

mjp41 added a commit that referenced this pull request May 10, 2026
The SHA-based fallback added in #847 (resolve PR by head SHA when
`workflow_run.pull_requests[]` is empty) handles fork PRs correctly,
but it also fires for `schedule` and `push` runs on `main`. The
nightly's head SHA is the merge commit of whichever PR last landed,
and `listPullRequestsAssociatedWithCommit` returns that PR — so the
nightly coverage report ended up posted as a comment on the just-
merged (now closed) PR instead of on the tracking issue.

Two changes, both narrowing the fallback:

  1. Only run the SHA fallback when
     `workflow_run.event === 'pull_request'`. `schedule`/`push`
     runs on the default branch always belong on the tracking
     issue and have no business looking up a PR by SHA.

  2. Require the matched PR to be in the `open` state. The
     previous "fall back to any PR if only closed ones match
     (e.g. squash-merge race)" is undesirable: if the PR has
     already merged, posting a fresh coverage comment on it is
     noise, not information. If a contributor really wanted the
     comment on a merged PR they can re-run the workflow.

Together these mean the SHA fallback only ever fires for an open
fork PR, which is the only case it was ever meant to handle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant