Skip to content

Commit statuses are posted to the PR merge commit, and patch coverage is diffed against the PR head — the two commits look swapped #103

Description

@YannickSchroerLN

Summary

On pull_request events the action works with two different commits, and I believe each is used where the other belongs:

  1. codecov/project and codecov/patch are posted to the merge commit (refs/pull/N/merge). The pull request's head commit receives no status at all, so nothing that reads a pull request — gh, branch protection, a bot — can see the result.
  2. Patch coverage is computed from a diff numbered against the head commit, while the coverage report it is intersected with is normally produced from a checkout of the merge commit. Once the base branch has moved, the two disagree about line numbers, and the action reports uncovered lines that the author never wrote.

Both are visible by reading the source, and both affect the pattern the README documents rather than any unusual setup. They are independent and can be split into two issues if you prefer — I have reported them together because the fix for each is "use the other commit", and seeing them side by side is what made the cause obvious.

Source is quoted from 0.4.0 (5f3f449cf9e909fd63513a48f27c57f090a5f321). Both code paths are unchanged since 0.3.7, which is the version I observed this on.

1. Statuses land on a commit nothing reads

createCommitStatus targets this.context.sha:

// src/utils/github-client.ts (0.4.0)
  async createCommitStatus(
    context: string,
    state: "success" | "failure" | "pending",
    description: string,
    targetUrl?: string,
  ): Promise<void> {
    const { owner, repo } = this.context.repo;
    const sha = this.context.sha;

reached from src/index.ts through statusReporter.reportStatus("codecov/patch", …). On a pull_request event this.context.sha is the merge commit, not the pull request's own commit. From GitHub's Events that trigger workflows:

GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

The effect is easy to miss, because the status is created successfully and the log says so — ✅ Reported status 'codecov/patch': failure - …. What actually happens:

  • GET /repos/{owner}/{repo}/commits/{head_sha}/statuses returns an empty array. The statuses exist only under refs/pull/N/merge.
  • gh pr view <n> --json statusCheckRollup does not list codecov/patch or codecov/project at all.
  • The merge ref is rebuilt whenever either branch moves, so the status is left on a commit the pull request no longer points to.

This appears to contradict the README, which offers these statuses as an enforcement mechanism:

This creates two status checks (codecov/project and codecov/patch) that:

  • Appear on commits and PRs
  • Can be required via branch protection rules

and, under Status Checks:

  • Can be used in branch protection rules to require coverage thresholds

Branch protection and rulesets evaluate required checks on the pull request's head commit, so a required codecov/patch can never be satisfied — it is never present on the commit that is examined.

Suggested fix. Prefer the head sha where the event provides one:

const sha = this.context.payload.pull_request?.head?.sha ?? this.context.sha;

0.4.0 already reads that value in getPullRequestCommitRefs() (added in #100 for the base comparison), so the same helper could serve here.

2. Patch coverage is diffed against a different tree than the coverage was measured on

The diff comes from the pull request endpoint:

// src/utils/github-client.ts (0.4.0)
    const { data } = await this.octokit.rest.pulls.get({
      owner,
      repo,
      pull_number: prNumber,
      mediaType: {
        format: "diff",
      },
    });

Its added-line numbers are relative to the head commit. PatchAnalyzer.analyzePatchCoverage then intersects those numbers with line numbers taken from the coverage report.

The coverage report, however, normally describes the merge commit. actions/checkout defaults to GITHUB_REF, which on pull_request is refs/pull/N/merge; the same documentation page states:

Because actions/checkout uses GITHUB_REF by default, it checks out the merge branch. Your CI tests run against the merged result, not just the head branch alone.

The README's own quickstart uses a bare actions/checkout@v4, so this is the default arrangement, not an exotic one. The two sides are then numbered against different versions of the same file whenever the base branch has moved since the branch point and has touched a file the pull request also touches. The offset is however many lines the base branch inserted or removed above the changed region — and if that region happens to be executable code with no coverage in the report, it is reported as the author's uncovered lines.

Symptom, as experienced: patch coverage fails, naming lines the author did not write; merging the base branch into the branch makes the failure disappear, with no test added and the author's own code byte-for-byte unchanged.

The mechanism is readable in the two code paths above, and it is straightforward to confirm in a repository of your own:

  1. Take a pull request whose branch is a few commits behind the base branch, where the base branch has modified a file that the pull request also modifies.
  2. Compare the coverage report's own view of that file with the file at the pull request's head commit — for Cobertura, the highest <line number="…"> under that <class> against the head file's line count. If they differ, every added-line number handed to PatchAnalyzer for that file is offset by the difference.
  3. Merge the base branch into the branch and re-run. The reported missing lines change without any test being written.

#80 in 0.4.0 (skipping zero-hit comment and blank lines) does not cover this: in my case the misattributed lines were executable statements, not comments.

Codecov's own CLI has a related report, codecov/codecov-cli#564: tests run on the merged state, comparison made against a stale point.

Suggested fix. Number the diff against the tree the coverage was measured on — the merge commit — by comparing it with its first parent:

const { data: mc } = await octokit.rest.repos.getCommit({ owner, repo, ref: context.sha });
// mc.parents[0] is the base branch tip the merge commit was built on
const { data } = await octokit.rest.repos.compareCommitsWithBasehead({
  owner, repo,
  basehead: `${mc.parents[0].sha}...${context.sha}`,
  mediaType: { format: "diff" },
});

falling back to the current behaviour when context.sha is not a two-parent commit. If you would rather not change the default, an input such as patch-diff-base: auto | merge | head would let users opt in — though auto looks safe, since a merge-commit checkout is what actions/checkout produces by default.

Environment

  • Action: getsentry/coverage-action@0.3.7; both code paths verified unchanged in 0.4.0
  • Trigger: pull_request, Linux runners
  • Coverage: Cobertura XML, consumed by the action from a directory of downloaded artifacts
  • Inputs: target-project, target-patch, fail-ci-if-error: true, enable-tests: false, post-pr-comment: false

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

    jaredTrigger the Jared agent to work on stuff

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions