From 3ed293a72db95e290e582aca5d69c4ffd4de1b7a Mon Sep 17 00:00:00 2001 From: JSONbored Date: Wed, 29 Jul 2026 03:26:20 -0700 Subject: [PATCH] fix(ci): hold the Codecov verdict until every upload has landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `notify.after_n_builds` fires on the Nth upload to ARRIVE, and arrival order does not match importance. validate-code's rees / control-plane / engine reports land in ~2 minutes; validate-tests is still producing the backend lcov that covers most of src/**. So codecov/patch posted a conclusion computed from a partial report — on #9799 it went pass, then fail, while validate-tests was still pending, with Codecov's own comment printing `backend | BASE 1 | HEAD 0`. That is not cosmetic: the gate closes a contributor PR on any red check, codecov/patch included, so a transient partial-report red can close a PR that has no defect. `require_ci_to_pass` gates the final verdict but does not suppress the interim status, and `wait_for_ci` (already on by default) did not prevent it either. Raising the count cannot fix this — the reasoning preserved on `after_n_builds` explains why a floor above the guaranteed minimum leaves codecov/patch permanently un-posted on PRs that produce fewer uploads — because a count cannot express "wait for the backend upload specifically". `manual_trigger` can: Codecov sends nothing until `codecovcli send-notifications` runs, and the new codecov-notify job runs it once, after every uploading job has finished. The notifier is deliberately unable to fail a PR. It is not in `validate`'s needs, and every failure path — a missing pipx, a failed install, a rejected notification — warns and exits 0. If it never runs, no status posts, the check set stays incomplete and the gate waits: it cannot merge unreviewed and cannot false-close, which is the whole point. `!cancelled()` rather than `always()` so a run superseded by a newer push does not notify on a stale sha, and the job skips entirely when neither upload job ran, since notifying on a commit Codecov has no reports for is an error rather than a verdict. Closes #9801 --- .github/workflows/ci.yml | 57 ++++++++++++++ codecov.yml | 34 +++++++-- test/unit/ci-codecov-notify.test.ts | 114 ++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+), 8 deletions(-) create mode 100644 test/unit/ci-codecov-notify.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f865988618..699a24ccc3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1253,3 +1253,60 @@ jobs: echo "::error title=CI::A required CI job failed or was cancelled." echo "Job results: ${{ join(needs.*.result, ', ') }}" exit 1 + + # Codecov sends nothing until it is told the uploads are done (codecov.yml's notify.manual_trigger). + # This job is the one place that can truthfully say "that's all of them": it needs every uploading job + # (validate-code carries rees / control-plane / engine, validate-tests carries backend), so it cannot + # run before they have finished. + # + # Before this existed, notify.after_n_builds fired the verdict on the first upload to ARRIVE, and + # arrival order does not match importance -- validate-code finishes in ~2 minutes while validate-tests + # is still producing the backend lcov. codecov/patch therefore posted a conclusion computed from a + # partial report (#9801). + # + # Deliberately NOT in `validate`'s needs, and deliberately non-fatal. The bug being fixed is a phantom + # red closing a good PR, so the fix must not add a new way to do that: a Codecov delivery hiccup must + # never fail the required check. If the trigger does not run, no status posts, the gate sees an + # incomplete check set and waits -- it cannot merge unreviewed and cannot false-close. + codecov-notify: + name: codecov-notify + needs: [changes, validate-code, validate-tests] + # Skip only when neither uploading job ran: a fully path-filtered PR uploads nothing, and asking + # Codecov to notify on a commit it has no reports for is an error rather than a verdict. Uses + # !cancelled() rather than always() so a run superseded by a newer push does not notify on a stale + # sha -- the replacement run posts the real verdict. + if: ${{ !cancelled() && (needs.validate-code.result != 'skipped' || needs.validate-tests.result != 'skipped') }} + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + steps: + # Every failure path below is swallowed on purpose. A red check from THIS job would be closed by + # the gate, which is precisely the bug being fixed -- the notifier must never be able to fail a PR. + # Worst case is that no verdict posts, leaving the check set incomplete, and the gate waits. + - name: Release the Codecov verdict + env: + # Empty on fork PRs, where Actions withholds secrets. That matches the tokenless upload steps + # in validate-code / validate-tests, so pass --token only when there is one to pass. + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + # MUST match the uploads' override_commit exactly (head sha on a PR, github.sha on push) or the + # notification targets a commit that has no reports attached to it. + COMMIT_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} + SLUG: ${{ github.repository }} + # Pinned like every action SHA in this file -- the CLI reaches the same Codecov API the uploads do. + CODECOV_CLI_VERSION: "11.3.1" + run: | + if ! pipx install "codecov-cli==${CODECOV_CLI_VERSION}"; then + echo "::warning title=Codecov::could not install codecov-cli==${CODECOV_CLI_VERSION}; no verdict will post for ${COMMIT_SHA}." + exit 0 + fi + args=(--commit-sha "$COMMIT_SHA" --slug "$SLUG" --git-service github --fail-on-error) + # `[ -n "$X" ] && args+=(...)` would abort the step under `bash -e` when the token is absent. + if [ -n "$CODECOV_TOKEN" ]; then + args+=(--token "$CODECOV_TOKEN") + fi + # --fail-on-error makes the CLI exit non-zero so a real failure is visible in the log; catching + # it keeps that visibility without ever turning this check red. + if ! codecovcli send-notifications "${args[@]}"; then + echo "::warning title=Codecov::send-notifications failed for ${COMMIT_SHA} -- codecov/patch will not post for this run." + fi diff --git a/codecov.yml b/codecov.yml index 1aa928209a..dae605f425 100644 --- a/codecov.yml +++ b/codecov.yml @@ -11,14 +11,32 @@ codecov: # Don't post a verdict until the CI run that produced the report has finished. require_ci_to_pass: true notify: - # The count that ALWAYS lands whenever validate-tests runs: since the 2026-07-24 unsharding, the - # backend suite produces ONE whole-suite lcov upload (flags: backend) instead of the former 3 shard - # uploads this threshold was sized for. It must stay 1, not 2, even though review-enrichment / - # control-plane add their own flag uploads on PRs that touch them -- those uploads only exist on such - # PRs, and a floor above the guaranteed minimum would leave codecov/patch permanently un-posted on - # every other PR. The narrow premature-window this reintroduces (a rees/control-plane upload landing - # and posting before the ~25-minute backend run finishes) self-corrects on the backend upload, and - # require_ci_to_pass above still holds the final verdict to the run's own conclusion. + # Hold every status and comment until CI says the uploads are done, rather than guessing from a + # count (#9801). `after_n_builds` fires on the Nth upload to ARRIVE, and arrival order does not + # match importance: validate-code's rees / control-plane / engine reports land in ~2 minutes while + # validate-tests is still running the backend lcov that covers most of src/**. So the verdict was + # computed from a partial report -- codecov/patch went pass, then fail, while validate-tests was + # still pending, with Codecov's own comment printing `backend | BASE 1 | HEAD 0`. + # + # That is not cosmetic here: the gate closes a contributor PR on ANY red check, codecov/patch + # included, so a transient partial-report red can close a PR that has no defect. + # + # Raising the count cannot fix it -- see the reasoning preserved on after_n_builds below -- because + # a count cannot express "wait for the backend upload specifically". manual_trigger can: Codecov + # sends nothing until `codecovcli send-notifications` runs, and the codecov-notify job in + # .github/workflows/ci.yml runs that once, after every uploading job has finished. + # + # Failure mode if the trigger never runs: codecov/patch simply never posts, so the gate sees an + # incomplete check set and waits. That is fail-safe -- it cannot merge unreviewed and cannot + # false-close -- which is the whole point of the change. + manual_trigger: true + # Inert while manual_trigger is true, and kept deliberately so removing manual_trigger restores the + # correct value rather than the default. The count that ALWAYS lands whenever validate-tests runs: + # since the 2026-07-24 unsharding, the backend suite produces ONE whole-suite lcov upload + # (flags: backend) instead of the former 3 shard uploads this threshold was sized for. It must stay + # 1, not 2, even though review-enrichment / control-plane add their own flag uploads on PRs that + # touch them -- those uploads only exist on such PRs, and a floor above the guaranteed minimum would + # leave codecov/patch permanently un-posted on every other PR. after_n_builds: 1 coverage: diff --git a/test/unit/ci-codecov-notify.test.ts b/test/unit/ci-codecov-notify.test.ts new file mode 100644 index 0000000000..f196f2e59f --- /dev/null +++ b/test/unit/ci-codecov-notify.test.ts @@ -0,0 +1,114 @@ +import { readFileSync } from "node:fs"; +import { parse } from "yaml"; +import { describe, expect, it } from "vitest"; + +function readYaml(path: string): Record { + return record(parse(readFileSync(path, "utf8")), path); +} + +function record(value: unknown, label: string): Record { + if (!value || typeof value !== "object" || Array.isArray(value)) { + throw new Error(`${label} must be an object`); + } + return value as Record; +} + +function steps(job: Record): Array> { + return (job.steps as Array>) ?? []; +} + +// Regression guard for #9801: codecov/patch posted a red conclusion computed from a PARTIAL report. +// notify.after_n_builds fires on the Nth upload to ARRIVE, and arrival order does not match importance -- +// validate-code's rees/control-plane/engine reports land in ~2 minutes while validate-tests is still +// producing the backend lcov that covers most of src/**. Codecov's own comment printed +// `backend | BASE 1 | HEAD 0` while it was posting a failure. Because the gate closes a contributor PR on +// ANY red check, codecov/patch included, that transient red could close a PR with no defect. +// +// The fix holds all notifications (notify.manual_trigger) until the codecov-notify job releases them once +// every uploading job has finished. These assertions pin the parts that are load-bearing and silent when +// broken -- a wrong commit sha or a resurrected count would fail open, not loudly. +describe("codecov notifications wait for every upload (#9801)", () => { + const workflow = readYaml(".github/workflows/ci.yml"); + const jobs = record(workflow.jobs, "workflow.jobs"); + const codecov = readYaml("codecov.yml"); + + it("codecov.yml holds every status until the CLI releases it", () => { + const notify = record(record(codecov.codecov, "codecov").notify, "codecov.notify"); + expect(notify.manual_trigger).toBe(true); + // Kept deliberately so dropping manual_trigger restores the correct floor rather than the default. + expect(notify.after_n_builds).toBe(1); + }); + + it("codecov-notify waits on every job that uploads a report", () => { + const job = record(jobs["codecov-notify"], "jobs.codecov-notify"); + // validate-code carries rees/control-plane/engine; validate-tests carries backend. Missing either + // reintroduces the partial-report verdict this job exists to prevent. + expect(job.needs).toEqual(["changes", "validate-code", "validate-tests"]); + }); + + it("codecov-notify still runs when an upload job fails, but not on a superseded run", () => { + const condition = String(record(jobs["codecov-notify"], "jobs.codecov-notify").if); + // A failed validate-tests must still release the verdict, otherwise codecov/patch never posts at all. + expect(condition).toContain("!cancelled()"); + // always() would notify on a sha the replacement run has already superseded. + expect(condition).not.toContain("always()"); + // A fully path-filtered PR uploads nothing; notifying on a commit Codecov has no reports for errors. + expect(condition).toContain("needs.validate-code.result != 'skipped'"); + expect(condition).toContain("needs.validate-tests.result != 'skipped'"); + }); + + it("codecov-notify is NOT a dependency of the required validate check", () => { + // The bug being fixed is a phantom red closing a good PR. Wiring the notifier into the required + // check would let a Codecov delivery hiccup do exactly that. + expect(record(jobs.validate, "jobs.validate").needs).not.toContain("codecov-notify"); + }); + + it("a failed send-notifications warns instead of failing the job", () => { + const run = String(steps(record(jobs["codecov-notify"], "jobs.codecov-notify")).at(-1)?.run ?? ""); + // The CLI exits non-zero (--fail-on-error) so a real failure is visible, and the `if !` converts it + // to a warning. Without the guard the step would go red and re-create the false-close class. + expect(run).toContain("--fail-on-error"); + expect(run).toContain("if ! codecovcli send-notifications"); + expect(run).toContain("::warning title=Codecov::"); + }); + + it("the token flag is only added when a token exists (fork PRs run without secrets)", () => { + const run = String(steps(record(jobs["codecov-notify"], "jobs.codecov-notify")).at(-1)?.run ?? ""); + // `[ -n "$X" ] && args+=(...)` aborts the whole step under `bash -e` when the token is empty, which + // is exactly the fork-PR case -- it must stay an if-block. + expect(run).toContain('if [ -n "$CODECOV_TOKEN" ]; then'); + expect(run).not.toMatch(/\[ -n "\$CODECOV_TOKEN" \] &&/); + }); + + it("codecov-notify targets the same commit the uploads used", () => { + const notifyStep = steps(record(jobs["codecov-notify"], "jobs.codecov-notify")).at(-1); + const sha = String(record(notifyStep?.env, "codecov-notify.env").COMMIT_SHA); + // The uploads all pass override_commit: head sha on a PR, github.sha on push. A mismatch here points + // the notification at a commit with no reports attached, so nothing ever posts. + const expected = "${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}"; + expect(sha).toBe(expected); + + const uploadOverrides = steps(record(jobs["validate-tests"], "jobs.validate-tests")) + .filter((step) => String(step.name ?? "").startsWith("Upload coverage to Codecov") && !String(step.name).includes("fork")) + .map((step) => String(record(step.with, "step.with").override_commit)); + expect(uploadOverrides.length).toBeGreaterThan(0); + for (const override of uploadOverrides) expect(override).toBe(expected); + }); + + it("the Codecov CLI is pinned like every other third-party dependency in the workflow", () => { + const env = record(steps(record(jobs["codecov-notify"], "jobs.codecov-notify")).at(-1)?.env, "codecov-notify.env"); + expect(String(env.CODECOV_CLI_VERSION)).toMatch(/^\d+\.\d+\.\d+$/); + }); + + it("no step in codecov-notify can turn the check red", () => { + // The gate closes a contributor PR on ANY red check, so a failed notifier would re-create the very + // bug it fixes. Every failure path -- install included -- has to be swallowed. + const job = record(jobs["codecov-notify"], "jobs.codecov-notify"); + const runs = steps(job).map((step) => String(step.run ?? "")); + expect(runs).toHaveLength(1); + // A bare `pipx install` would fail the step (and the check) when the runner image lacks the CLI. + expect(runs[0]).toContain("if ! pipx install"); + expect(runs[0]).toContain("exit 0"); + for (const step of steps(job)) expect(step.uses).toBeUndefined(); + }); +});