From 01c21f5fe84f7c97c1d4ca1ae31981ee35da430e Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 8 Aug 2026 20:19:52 +0900 Subject: [PATCH] fix(ci): require aggregate check evidence --- .github/workflows/ci.yml | 74 +++++---- .github/workflows/enforce-pr-target.yml | 46 ++++-- tests/ci-workflows.test.ts | 179 +++++++++++++++++---- tests/helpers/enforce-pr-target-harness.ts | 58 +++++-- 4 files changed, 273 insertions(+), 84 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index afb48e72e..01ec5faf4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,7 +1,10 @@ name: Cross-platform CI on: - pull_request: + # Always create the aggregate `ci` check for pull requests. Expensive jobs + # apply the former path allowlist through the `changes` job below, so a + # docs-only PR receives explicit positive evidence instead of no check at all. + pull_request: {} # No base-branch filter on purpose. GitHub matches `branches:` against the # BASE ref, so `[main, dev]` silently excluded stacked child PRs — whose # base is another open PR's head branch, an intentional review workflow per @@ -12,31 +15,14 @@ on: # # An allowlist cannot express "base is another PR's head" — stacked bases # carry contributor prefixes (`fix/`, `feat/`, `agent/`) as readily as - # `codex/`, and contributor stacks need CI most. `paths:` below is the real - # scope gate, same shape as issue-quality-tests.yml. Safe to widen here + # `codex/`, and contributor stacks need CI most. The `changes` job below is + # the real scope gate, using the same allowlist as the push trigger. Safe to + # widen here # because this workflow is `pull_request` (not `pull_request_target`), # declares `contents: read`, and reads no secrets. # # `push:` stays pinned to the integration lines: it gates the release path, # and this trigger already covers review. - paths: - - "src/**" - - "bin/**" - - "tests/**" - - "scripts/**" - - "gui/**" - - "assets/**" - - ".gitattributes" - - ".npmignore" - - "package.json" - - "bun.lock" - - "tsconfig.json" - - "README.md" - - "LICENSE" - - ".github/workflows/ci.yml" - - ".github/workflows/release.yml" - - ".github/workflows/enforce-pr-target.yml" - - ".github/workflows/stale-needs-info.yml" push: branches: [main, preview, dev] paths: @@ -77,8 +63,8 @@ jobs: # A hostile PR can delete the branch and hardcode the self-hosted labels into # `$GITHUB_OUTPUT`, and `runs-on` will honour it. That this job runs on # `ubuntu-latest` changes nothing — the untrusted part is its OUTPUT, not its - # host. `.github/workflows/ci.yml` is in this workflow's `pull_request.paths`, - # so such an edit triggers its own run. + # host. `.github/workflows/ci.yml` is in the `changes` job's `ci` filter, so + # such an edit triggers every expensive verification job. # # What actually keeps untrusted code off a self-hosted runner lives OUTSIDE # this file, where a PR cannot reach it: the fork-PR approval policy @@ -154,6 +140,7 @@ jobs: contents: read pull-requests: read outputs: + ci: ${{ steps.filter.outputs.ci }} gui: ${{ steps.filter.outputs.gui }} packaging: ${{ steps.filter.outputs.packaging }} steps: @@ -180,6 +167,27 @@ jobs: # on this branch", which is the intent. base: ${{ github.ref }} filters: | + # Mirrors the push trigger's path allowlist. Pull requests always + # start the workflow so the aggregate check exists, while these + # paths decide whether the expensive test jobs need to run. + ci: + - 'src/**' + - 'bin/**' + - 'tests/**' + - 'scripts/**' + - 'gui/**' + - 'assets/**' + - '.gitattributes' + - '.npmignore' + - 'package.json' + - 'bun.lock' + - 'tsconfig.json' + - 'README.md' + - 'LICENSE' + - '.github/workflows/ci.yml' + - '.github/workflows/release.yml' + - '.github/workflows/enforce-pr-target.yml' + - '.github/workflows/stale-needs-info.yml' gui: - 'gui/**' # Everything that ends up inside `npm pack`, or that decides what @@ -218,6 +226,8 @@ jobs: # would eat what the sharding saves. test: name: test ${{ matrix.shard }}/4 + needs: changes + if: github.event_name != 'pull_request' || needs.changes.outputs.ci == 'true' runs-on: ubuntu-latest # A quarter of the suite. A shard that needs longer than this is wedged, not # slow — the old 30-minute ceiling was margin for the Windows leg, which no @@ -271,6 +281,8 @@ jobs: # failure is bounded to this job instead of poisoning a general test shard. storage-policy: name: storage policy + needs: changes + if: github.event_name != 'pull_request' || needs.changes.outputs.ci == 'true' runs-on: ubuntu-latest timeout-minutes: 5 steps: @@ -310,6 +322,7 @@ jobs: gates: name: gates needs: changes + if: github.event_name != 'pull_request' || needs.changes.outputs.ci == 'true' runs-on: ubuntu-latest timeout-minutes: 15 steps: @@ -372,6 +385,8 @@ jobs: # platform-independent and already ran once above. platform-macos: name: macos + needs: changes + if: github.event_name != 'pull_request' || needs.changes.outputs.ci == 'true' runs-on: macos-latest # The unsharded control for the sharded Linux lane: the only place the whole # suite runs in one pool, so it is the place that catches what sharding @@ -499,6 +514,8 @@ jobs: # keyring matrix leg may use the persistent self-hosted Windows runner. keyring-smoke: name: keyring ${{ matrix.name }} + needs: changes + if: github.event_name != 'pull_request' || needs.changes.outputs.ci == 'true' runs-on: ${{ matrix.runner }} timeout-minutes: 8 strategy: @@ -616,12 +633,9 @@ jobs: # no branch protection configured today, so nothing has to be re-pointed — but # whoever enables it has one obvious check to require. # - # NOTE for that day: requiring this check also means dropping the - # workflow-level `paths:` filter above, or moving this job to an - # always-triggered workflow. A PR that touches only docs does not trigger this - # workflow at all, so no `ci` check would be created and the PR would sit - # pending forever. That is harmless while nothing is required and a trap - # afterwards. + # Pull requests always trigger this workflow. The `changes` job keeps + # expensive jobs scoped, but this aggregate still records explicit success + # when every producer is deliberately skipped for an out-of-scope docs change. # # `if: always()` is load-bearing. Without it, a failed or skipped dependency # skips this job too — and GitHub reports a skipped job as success, so the gate @@ -663,4 +677,4 @@ jobs: # leg is a gate violation: on push events it is always skipped, and on # dispatch a failed Windows leg already fails the allowlist above. The # old "windows must have run on main/preview" assertion left with the - # condition it policed. \ No newline at end of file + # condition it policed. diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index e032e6cca..0dfa7e12b 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -734,26 +734,44 @@ jobs: !headDrifted && failures.length === 0 ) { - let ciGreen = true; + let ciGreen = false; try { - const { data: checksData } = - await github.rest.checks.listForRef({ + // GitHub Actions' immutable App ID. Name alone is not evidence: + // any installed app can publish a check called `ci`. + const githubActionsAppId = 15368; + const checkRuns = []; + for await (const response of github.paginate.iterator( + github.rest.checks.listForRef, + { owner, repo, ref: pr.head.sha, + app_id: githubActionsAppId, + check_name: "ci", + filter: "latest", per_page: 100 - }); - const ciCheck = (checksData.check_runs ?? []).find( - check => check.name === "ci" + } + )) { + // Octokit's paginator normalizes `{ total_count, check_runs }` + // into an array in `response.data` for every iterator page. + checkRuns.push(...response.data); + } + const ciChecks = checkRuns.filter( + check => + check.name === "ci" && + check.app?.id === githubActionsAppId ); - // No `ci` check means no CI run exists for this head (for - // example a docs-only change): there is nothing to contradict - // the author's claim. A real `ci` check must be completed - // successfully. + // The readiness claim requires positive CI evidence. A missing, + // pending, unsuccessful, foreign, or conflicting aggregate + // check must fail closed. `filter: latest` removes superseded + // rerun attempts; `every` still rejects ambiguous live results. ciGreen = - ciCheck === undefined || - (ciCheck.status === "completed" && - ciCheck.conclusion === "success"); + ciChecks.length > 0 && + ciChecks.every( + check => + check.status === "completed" && + check.conclusion === "success" + ); } catch (error) { core.warning( `Could not list checks for the readiness claim check: ${error.message}` @@ -1328,4 +1346,4 @@ jobs: "All PR quality gates passed and there is no active bot state." ); return; - } \ No newline at end of file + } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index fa6ca5cc7..87b72998c 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -138,13 +138,15 @@ describe("GitHub Actions hardening", () => { expect([...(gate?.needs ?? [])].sort()) .toEqual(Object.keys(ci.jobs ?? {}).filter(name => name !== "ci").sort()); - // macOS is the unsharded control for the sharded Linux lane: it is the only - // place the whole suite runs in one pool. Sharded or conditional, it stops - // being a control. + // macOS is the unsharded control for every CI-relevant change. It may skip + // only when the shared path filter says the entire expensive suite is out of + // scope (for example a docs-site-only PR). const macosSteps = (ci.jobs?.["platform-macos"] as { steps?: { run?: string }[] })?.steps ?? []; expect(macosSteps.some(step => step.run?.includes("bun test --isolate tests"))).toBe(true); expect(macosSteps.some(step => step.run?.includes("--shard"))).toBe(false); - expect(ci.jobs?.["platform-macos"]).not.toHaveProperty("if"); + expect((ci.jobs?.["platform-macos"] as { needs?: string; if?: string })?.needs).toBe("changes"); + expect((ci.jobs?.["platform-macos"] as { if?: string })?.if) + .toBe("github.event_name != 'pull_request' || needs.changes.outputs.ci == 'true'"); // Windows is dispatch-only: it gates nothing, not even the shipping // boundary. The sharded promotion run surfaced ~207 Windows-only failures @@ -225,7 +227,7 @@ describe("GitHub Actions hardening", () => { for (const [path, expectedKeys] of [ // No `branches`: the stacked-base exemption has no enumerable branch list. - [".github/workflows/ci.yml", ["paths"]], + [".github/workflows/ci.yml", []], [".github/workflows/service-lifecycle.yml", ["branches", "paths"]], ] as const) { const workflow = Bun.YAML.parse(await readText(path)) as { @@ -259,6 +261,7 @@ describe("GitHub Actions hardening", () => { push?: { branches?: string[]; paths?: string[] }; pull_request?: { branches?: string[]; paths?: string[] }; }; + jobs?: Record | undefined>; }; expect([...(ci.on?.push?.branches ?? [])].sort()).toEqual(["dev", "main", "preview"]); @@ -274,14 +277,14 @@ describe("GitHub Actions hardening", () => { // Re-adding an allowlist is the regression this pins, and it cannot be // written correctly: stacked bases carry contributor prefixes (`fix/`, // `feat/`, `agent/`) as readily as `codex/`, so any list leaves some stack - // silently unverified. `paths:` below is the scope gate. + // silently unverified. Pull requests also carry no workflow-level path + // filter: every head needs an aggregate `ci` check. expect(ci.on?.pull_request?.branches).toBeUndefined(); + expect(ci.on?.pull_request?.paths).toBeUndefined(); - // The path filter decides whether the job runs at all. Deleting one entry - // deletes nothing visible: the workflow still exists, still lists the right - // branches, and simply never fires for a PR that touches only that surface. - // Round 16 dropped `src/**`, `tests/**`, and both workflow self-references - // one at a time and the suite stayed green each time. Pin the list. + // The push trigger and pull-request `changes` job share one expensive-CI + // allowlist. PRs always create the workflow and aggregate check; this list + // decides whether the costly jobs run. Pin the entire list on both paths. const ciPaths = [ ".gitattributes", ".github/workflows/ci.yml", @@ -301,10 +304,22 @@ describe("GitHub Actions hardening", () => { "tests/**", "tsconfig.json", ]; - expect([...(ci.on?.pull_request?.paths ?? [])].sort()).toEqual(ciPaths); - // Push and pull_request have to cover the same surfaces, or a change lands - // on dev having been checked on one trigger and not the other. expect([...(ci.on?.push?.paths ?? [])].sort()).toEqual(ciPaths); + + const filterStep = (ci.jobs?.changes as { + steps?: { with?: Record }[]; + })?.steps?.find(step => step.with?.filters); + const areaFilters = Bun.YAML.parse(String(filterStep?.with?.filters ?? "")) as { + ci?: string[]; + }; + expect([...(areaFilters.ci ?? [])].sort()).toEqual(ciPaths); + + const scopedCondition = "github.event_name != 'pull_request' || needs.changes.outputs.ci == 'true'"; + for (const jobName of ["test", "storage-policy", "gates", "platform-macos", "keyring-smoke"]) { + const job = ci.jobs?.[jobName] as { needs?: string; if?: string } | undefined; + expect(`${jobName}:${job?.needs}`).toBe(`${jobName}:changes`); + expect(`${jobName}:${job?.if}`).toBe(`${jobName}:${scopedCondition}`); + } }); test("cross-platform CI keeps the GUI lint and build gates", async () => { @@ -371,15 +386,13 @@ describe("GitHub Actions hardening", () => { "src/**", ].sort()); - // A per-job filter can only narrow what the workflow-level filter admits, so - // every packaging pattern that names a real path must also appear in the - // trigger's own path list. Otherwise the workflow never runs for that file - // and the filter entry is decoration. - const triggerPaths = (ci.on as { pull_request?: { paths?: string[] } } | undefined) - ?.pull_request?.paths ?? []; + // Every packaging pattern that names a real path must also appear in the + // shared expensive-CI filter. Otherwise the workflow records a cheap green + // aggregate while silently skipping the packaging verification. + const ciPatterns = (Bun.YAML.parse(filters) as { ci?: string[] }).ci ?? []; for (const pattern of packaging) { if (pattern === "scripts/prepare-package.ts") continue; // covered by scripts/** - expect(`${pattern}:${triggerPaths.includes(pattern)}`).toBe(`${pattern}:true`); + expect(`${pattern}:${ciPatterns.includes(pattern)}`).toBe(`${pattern}:true`); } }); @@ -1845,9 +1858,9 @@ describe("GitHub Actions hardening", () => { expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); }); - test("a head with no ci check at all keeps the CI box (docs-only style PRs)", async () => { - // No CI run exists for this head: there is nothing to contradict the - // author's claim, so the CI box survives. + test("a head with no ci check fails closed for the CI claim", async () => { + // No CI run means the claim has no positive evidence, so the box is + // unticked and the PR stays in draft. const result = await run({ pr: { base: { ref: "dev" }, @@ -1862,15 +1875,18 @@ describe("GitHub Actions hardening", () => { "checks.listForRef", "graphql", "pulls.listReviews", - "issues.addLabels", - "graphql", + "pulls.get", + "pulls.update", "issues.createComment", ])); - expect(callsTo(result, "pulls.update")).toEqual([]); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); const drafts = callsTo(result, "graphql") as [{ query: string }]; - expect(drafts).toHaveLength(2); + expect(drafts).toHaveLength(1); expect(drafts[0]!.query).toContain("reviewThreads"); - expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + expect(lastReadinessCommentBody(result)).toContain( + "GitHub CI is not green on the current head", + ); }); test("a pending ci check cannot attest green", async () => { @@ -1899,6 +1915,111 @@ describe("GitHub Actions hardening", () => { expect(readinessBody).toContain("GitHub CI is not green on the current head"); }); + test("a green ci check on a later checks page attests green", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRunPages: [ + Array.from({ length: 100 }, (_, index) => ({ + name: `decoy-${index}`, + status: "completed", + conclusion: "success", + })), + [{ name: "ci", status: "completed", conclusion: "success" }], + ], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", + "graphql", + "issues.createComment", + ])); + const checkCalls = callsTo(result, "checks.listForRef") as Array<{ + app_id?: number; + check_name?: string; + filter?: string; + }>; + for (const call of checkCalls) { + expect(call.app_id).toBe(15368); + expect(call.check_name).toBe("ci"); + expect(call.filter).toBe("latest"); + } + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }, { query: string }]; + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); + }); + + test("a foreign app check named ci cannot attest green", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ + name: "ci", + status: "completed", + conclusion: "success", + app: { id: 999999 }, + }], + }); + + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(lastReadinessCommentBody(result)).toContain("GitHub CI is not green on the current head"); + }); + + test("conflicting trusted ci checks fail closed regardless of ordering", async () => { + const green = { name: "ci", status: "completed", conclusion: "success" }; + const pending = { name: "ci", status: "in_progress", conclusion: null }; + const failed = { name: "ci", status: "completed", conclusion: "failure" }; + + for (const checkRuns of [[green, pending], [pending, green], [green, failed], [failed, green]]) { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns, + }); + + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(lastReadinessCommentBody(result)).toContain("GitHub CI is not green on the current head"); + } + }); + + test("multiple latest trusted green ci checks are consistent evidence", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [ + { name: "ci", status: "completed", conclusion: "success" }, + { name: "ci", status: "completed", conclusion: "success" }, + ], + }); + + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }, { query: string }]; + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + }); + test("an unresolved Codex thread unchecks the findings box and re-drafts", async () => { const result = await run({ pr: { diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index d555f461f..6bc41c94e 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -173,7 +173,19 @@ export type RunOptions = { * `ci` check so completed-checklist scenarios pass the claim check. * Pass a red/pending/missing set to exercise the claim-check reset paths. */ - checkRuns?: Array<{ name: string; status: string; conclusion: string | null }>; + checkRuns?: Array<{ + name: string; + status: string; + conclusion: string | null; + app?: { id: number } | null; + }>; + /** Page-keyed check-run fixtures for `checks.listForRef` pagination. */ + checkRunPages?: Array>; /** * Review threads `pullRequestReviewThreads` (via GraphQL) reports for the PR. * Each entry is `{ isResolved, author }`; the harness wraps it into the @@ -236,7 +248,7 @@ const DEFAULT_BODY = [ /** The repo's documented "CI passed" check, green by default. */ const DEFAULT_GREEN_CHECKS = [ - { name: "ci", status: "completed", conclusion: "success" }, + { name: "ci", status: "completed", conclusion: "success", app: { id: 15368 } }, ]; const DEFAULT_PR = { @@ -616,6 +628,13 @@ export async function runEnforcePrTarget( (options.openPulls && options.openPulls.length > 0 ? [options.openPulls] : []); const associatedPullRequestPages: unknown[][] = options.associatedPullRequestPages ?? [options.associatedPullRequests ?? [pr]]; + const checkRunPages = (options.checkRunPages ?? [options.checkRuns ?? DEFAULT_GREEN_CHECKS]) + .map(page => page.map(check => ({ + ...check, + // Existing fixtures model trusted GitHub Actions checks unless a test + // explicitly supplies another app or null to exercise provenance. + app: check.app === undefined ? { id: 15368 } : check.app, + }))); const paginatePageCount = Math.max( pages.length, issueEventPages.length, @@ -736,11 +755,13 @@ export async function runEnforcePrTarget( removeLabel: (args: unknown) => respond("issues.removeLabel", args, {}), }, checks: { - listForRef: (args: unknown) => - respond("checks.listForRef", args, { - total_count: (options.checkRuns ?? DEFAULT_GREEN_CHECKS).length, - check_runs: options.checkRuns ?? DEFAULT_GREEN_CHECKS, - }), + listForRef: (args: unknown) => { + const page = Number((args as { page?: number })?.page ?? 1); + return respond("checks.listForRef", args, { + total_count: checkRunPages.reduce((total, rows) => total + rows.length, 0), + check_runs: checkRunPages[page - 1] ?? [], + }); + }, }, repos: { getCollaboratorPermissionLevel: (args: unknown) => @@ -822,9 +843,13 @@ export async function runEnforcePrTarget( paginate = Object.assign( async (fn: (args: unknown) => Promise<{ data: unknown[] }>, params: unknown) => { const collected: unknown[] = []; - for (let page = 1; page <= paginatePageCount; page += 1) { + const pageCount = fn === rest.checks.listForRef ? checkRunPages.length : paginatePageCount; + for (let page = 1; page <= pageCount; page += 1) { const response = await fn({ ...(params as object), page }); - collected.push(...response.data); + const rows = fn === rest.checks.listForRef + ? ((response.data as unknown as { check_runs?: unknown[] }).check_runs ?? []) + : response.data; + collected.push(...rows); } return collected; }, @@ -837,8 +862,19 @@ export async function runEnforcePrTarget( */ iterator: (fn: (args: unknown) => Promise<{ data: unknown[] }>, params: unknown) => ({ async *[Symbol.asyncIterator]() { - for (let page = 1; page <= paginatePageCount; page += 1) { - yield await fn({ ...(params as object), page }); + const pageCount = fn === rest.checks.listForRef ? checkRunPages.length : paginatePageCount; + for (let page = 1; page <= pageCount; page += 1) { + const response = await fn({ ...(params as object), page }); + if (fn !== rest.checks.listForRef) { + yield response; + continue; + } + // Match @octokit/plugin-paginate-rest: list envelopes such as + // `{ total_count, check_runs }` become array-valued page data. + yield { + ...response, + data: (response.data as unknown as { check_runs?: unknown[] }).check_runs ?? [], + }; } }, }),