From 98573bf59f74fc5ad0d1d4856890c012a41d8d11 Mon Sep 17 00:00:00 2001 From: chh-ay Date: Sun, 26 Jul 2026 16:21:42 +0700 Subject: [PATCH] chore(ci): run the performance gate only when a change can move it The matched zero-regression benchmark occupies the single self-hosted runner for up to 90 minutes and every other pull request queues behind it, yet it ran for changeset-only and docs-only diffs. Classify performance-relevant paths the way `docs_required` already classifies documentation, and gate the job on the result. Unknown, mixed, and empty change sets keep taking the expensive lane: a false negative ships a regression, a false positive costs one benchmark run. `unit-coverage` installed Chromium, Firefox and WebKit with system dependencies, but its only browser consumer is the `bench:verify` render smoke, which launches Chromium alone. Also close two holes found while gating: - The job now refuses fork pull requests. It is the only non-ephemeral runner in the workflow, and a persistent host exposes its filesystem, caches and network position regardless of a read-only token. Required CI fails such a pull request rather than accepting the skip, so a fork cannot alter `bench/results/render-baseline.json` and be validated against its own baseline after merge. - `preflight` joins the job's `needs`, because the `needs` context exposes direct dependencies only and the condition would otherwise evaluate against an undefined context and skip on every run. Contract tests cover the trigger lists, the dependency requirement for any job reading another job's outputs, the fork rejection, and the absence of `pull_request_target`. --- .github/workflows/ci.yml | 51 ++++++++++++++++++++++++-- package.json | 2 +- scripts/ci-paths.test.ts | 48 +++++++++++++++++++++++- scripts/ci-paths.ts | 38 ++++++++++++++++++- scripts/toolchain-contract.test.ts | 35 +++++++++++++++++- scripts/workflow-contract.test.ts | 59 ++++++++++++++++++++++++++++++ 6 files changed, 225 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3c0a108b..cdf32460 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,6 +30,7 @@ jobs: timeout-minutes: 20 outputs: docs_required: ${{ steps.paths.outputs.docs_required }} + perf_required: ${{ steps.paths.outputs.perf_required }} steps: - name: Checkout uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 @@ -113,8 +114,11 @@ jobs: run: bun install --frozen-lockfile - name: Install coverage toolchain run: cargo install cargo-llvm-cov --version "$CARGO_LLVM_COV_VERSION" --locked - - name: Install benchmark browser - run: bun run browser:install + # `bench:verify` runs `render-driver.ts --smoke`, which launches Chromium and + # nothing else. Installing Firefox and WebKit here downloads two engines plus + # their system dependencies on every run for no consumer. + - name: Install Chromium for the render smoke + run: bun run browser:install:chromium - name: Download canonical tarballs uses: actions/download-artifact@634f93cb2916e3fdff6788551b99b062d0335ce0 # v5 with: @@ -407,7 +411,27 @@ jobs: controlled-performance: name: Matched Zero-Regression Performance - needs: artifact-build + # `preflight` is in `needs` because the `if:` below reads its output. The + # `needs` context exposes DIRECT dependencies only, so omitting it would make + # the expression resolve against an undefined context and skip this job on + # every run. Execution order is unchanged: `artifact-build` already needs + # `preflight`. + needs: [preflight, artifact-build] + # Two independent conditions, both required. + # + # 1. Cost: this job occupies a single self-hosted runner for up to 90 minutes + # and every other pull request queues behind it, so it runs only when the + # diff could move a measurement. `requiresPerformanceRun` defaults to true. + # 2. Security: this is the only NON-ephemeral runner in the workflow. It must + # never check out and execute code from a fork — a persistent host exposes + # its filesystem, caches, and network position regardless of the read-only + # token. Fork pull requests skip it; the measurement is taken instead by the + # `push` run on `develop` after merge, which is same-repository by + # construction. + if: >- + needs.preflight.outputs.perf_required == 'true' && + (github.event_name == 'push' || + github.event.pull_request.head.repo.full_name == github.repository) runs-on: sheetwrite-perf-i9-12900h-cachyos timeout-minutes: 90 steps: @@ -482,6 +506,8 @@ jobs: BROWSER: ${{ needs.browser-smoke.result }} PERFORMANCE: ${{ needs.controlled-performance.result }} DOCS_REQUIRED: ${{ needs.preflight.outputs.docs_required }} + PERF_REQUIRED: ${{ needs.preflight.outputs.perf_required }} + PERF_FROM_FORK: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository }} run: | test "$PREFLIGHT" = success test "$UNIT_COVERAGE" = success @@ -489,7 +515,6 @@ jobs: test "$PACKED" = success test "$BUNDLERS" = success test "$SIZE" = success - test "$PERFORMANCE" = success if [ "$DOCS_REQUIRED" = "true" ]; then test "$DOCS" = success test "$BROWSER" = success @@ -498,3 +523,21 @@ jobs: test "$DOCS" = skipped test "$BROWSER" = skipped fi + # The self-hosted benchmark never runs fork code (see + # `controlled-performance`). That protects the host but must not become a + # way to merge perf-sensitive changes without evidence: a fork could edit + # `bench/results/render-baseline.json` and, if we deferred to a post-merge + # run, be validated against its own altered baseline. So a fork pull + # request is allowed through only when the diff needs no measurement. + if [ "$PERF_REQUIRED" != "true" ]; then + test "$PERF_REQUIRED" = "false" + test "$PERFORMANCE" = skipped + elif [ "$PERF_FROM_FORK" = "true" ]; then + echo "This pull request changes performance-sensitive paths, but the" >&2 + echo "matched zero-regression benchmark does not run fork code on the" >&2 + echo "self-hosted runner. A maintainer must replay these commits on a" >&2 + echo "same-repository branch so the gate runs before merge." >&2 + exit 1 + else + test "$PERFORMANCE" = success + fi diff --git a/package.json b/package.json index e32c9d96..e5a23e3a 100644 --- a/package.json +++ b/package.json @@ -65,7 +65,7 @@ "test:browser": "playwright test --config test/browser/playwright.config.ts", "browser:install:chromium": "playwright install --with-deps chromium", "test:browser:portability": "playwright test --config test/browser/playwright.config.ts --grep @portability", - "test:tooling": "bun test scripts/toolchain-contract.test.ts scripts/workspace-tooling.test.ts scripts/dependency-audit.test.ts scripts/verify-clean-build.test.ts scripts/size-report.test.ts scripts/release-artifacts.test.ts scripts/consumer-lock.test.ts scripts/release-verify.test.ts scripts/release-workflow.test.ts scripts/workflow-contract.test.ts scripts/public-api.test.ts scripts/coverage-check.test.ts scripts/sheetwrite-code-hovers.test.ts scripts/normalize-sitemap.test.ts", + "test:tooling": "bun test scripts/toolchain-contract.test.ts scripts/workspace-tooling.test.ts scripts/dependency-audit.test.ts scripts/verify-clean-build.test.ts scripts/size-report.test.ts scripts/release-artifacts.test.ts scripts/consumer-lock.test.ts scripts/release-verify.test.ts scripts/release-workflow.test.ts scripts/workflow-contract.test.ts scripts/ci-paths.test.ts scripts/public-api.test.ts scripts/coverage-check.test.ts scripts/sheetwrite-code-hovers.test.ts scripts/normalize-sitemap.test.ts", "test:coverage:ts": "bun scripts/coverage.ts ts", "test:coverage:rust": "bun scripts/coverage.ts rust", "test:coverage": "bun run test:coverage:ts && bun run test:coverage:rust", diff --git a/scripts/ci-paths.test.ts b/scripts/ci-paths.test.ts index 17b0c6fb..5dfb50af 100644 --- a/scripts/ci-paths.test.ts +++ b/scripts/ci-paths.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { requiresDocumentationBuild } from "./ci-paths.js"; +import { requiresDocumentationBuild, requiresPerformanceRun } from "./ci-paths.js"; describe("documentation CI path classification", () => { it("runs for documentation, public API, package, and browser-contract changes", () => { @@ -40,3 +40,49 @@ describe("documentation CI path classification", () => { expect(requiresDocumentationBuild([])).toBeTrue(); }); }); + +describe("performance CI path classification", () => { + it("runs for engine, renderer, harness, manifest, and workflow changes", () => { + for (const path of [ + "packages/core/src/canvas-paint.ts", + "packages/core/src/store/data-engine.ts", + "packages/wasm/src/query.rs", + "packages/wasm/Cargo.toml", + "packages/react/src/index.ts", + "bench/src/render-driver.ts", + // The gate compares fresh samples against this reference, so a change to it + // must re-run the benchmark rather than skip it. + "bench/results/render-baseline.json", + "package.json", + "bun.lock", + ".github/workflows/ci.yml", + "scripts/ci-paths.ts", + ]) { + expect(requiresPerformanceRun([path]), path).toBeTrue(); + } + }); + + it("skips only paths proven unable to move a measurement", () => { + expect( + requiresPerformanceRun([ + ".changeset/release-note.md", + ".github/ISSUE_TEMPLATE/bug-report.yml", + "README.md", + "SECURITY.md", + "docs/src/routes/index.tsx", + "examples/vanilla/index.html", + "packages/core/test/grid.test.ts", + "packages/core/README.md", + "bench/test/render-gate.test.ts", + "scripts/release-workflow.test.ts", + "test/browser/vanilla-workbench.spec.ts", + ]), + ).toBeFalse(); + }); + + it("takes the expensive lane for mixed, unknown, and empty change sets", () => { + expect(requiresPerformanceRun([".changeset/note.md", "packages/wasm/src/sheet.rs"])).toBeTrue(); + expect(requiresPerformanceRun(["new-subsystem/config.json"])).toBeTrue(); + expect(requiresPerformanceRun([])).toBeTrue(); + }); +}); diff --git a/scripts/ci-paths.ts b/scripts/ci-paths.ts index 4a56553e..1909992c 100644 --- a/scripts/ci-paths.ts +++ b/scripts/ci-paths.ts @@ -21,6 +21,40 @@ export function requiresDocumentationBuild(paths: readonly string[]): boolean { ); } +/** + * Paths proven unable to change measured runtime performance. Everything else — + * every `src` tree, manifest, lockfile, and workflow — selects the expensive lane. + */ +const PERFORMANCE_INDEPENDENT_PATHS = [ + /^\.changeset\//, + /^\.github\/ISSUE_TEMPLATE\//, + /^\.github\/(?:CODEOWNERS|dependabot\.yml)$/, + /^(?:CONTRIBUTING|LICENSE|README|SECURITY|SUPPORT)\.md$/, + /^docs\//, + /^examples\//, + /^packages\/[^/]+\/(?:test|tests)\//, + /^packages\/[^/]+\/README\.md$/, + // `bench/test/` only. `bench/results/render-baseline.json` is the reference the + // zero-regression gate compares against (`ci.yml` check step), so a change there + // must run the benchmark rather than skip it. + /^bench\/test\//, + /^scripts\/[^/]+\.test\.ts$/, + /^test\//, +] as const; + +/** + * Returns true unless every changed path is proven independent from measured + * performance. The matched zero-regression benchmark occupies a single + * self-hosted runner for up to 90 minutes, so it is skipped when the change + * cannot move it — but unknown paths, mixed sets, and empty sets deliberately + * take the expensive lane: a false negative ships a regression, a false positive + * costs one benchmark run. + */ +export function requiresPerformanceRun(paths: readonly string[]): boolean { + if (paths.length === 0) return true; + return paths.some((path) => !PERFORMANCE_INDEPENDENT_PATHS.some((pattern) => pattern.test(path))); +} + function changedPaths(baseSha: string): readonly string[] { if (!/^[0-9a-f]{40}$/.test(baseSha)) { throw new Error("BASE_SHA must be a full lowercase commit SHA"); @@ -44,5 +78,7 @@ function changedPaths(baseSha: string): readonly string[] { if (import.meta.main) { const baseSha = process.env.BASE_SHA; if (!baseSha) throw new Error("BASE_SHA is required"); - console.log(`docs_required=${requiresDocumentationBuild(changedPaths(baseSha))}`); + const paths = changedPaths(baseSha); + console.log(`docs_required=${requiresDocumentationBuild(paths)}`); + console.log(`perf_required=${requiresPerformanceRun(paths)}`); } diff --git a/scripts/toolchain-contract.test.ts b/scripts/toolchain-contract.test.ts index 74f4389e..d1189410 100644 --- a/scripts/toolchain-contract.test.ts +++ b/scripts/toolchain-contract.test.ts @@ -162,7 +162,16 @@ describe("contributor and CI toolchain contract", () => { const jobs = parsedWorkflow.jobs ?? {}; const controlled = jobs["controlled-performance"]; expect(controlled?.["runs-on"]).toBe("sheetwrite-perf-i9-12900h-cachyos"); - expect(controlled?.needs).toBe("artifact-build"); + // `preflight` is required in `needs` because the job's `if:` reads + // `needs.preflight.outputs.perf_required`; the `needs` context exposes direct + // dependencies only, so omitting it would skip the gate on every run. + expect(controlled?.needs).toEqual(["preflight", "artifact-build"]); + const condition = String(controlled?.if ?? "").replace(/\s+/g, " "); + expect(condition).toContain("needs.preflight.outputs.perf_required == 'true'"); + // The only non-ephemeral runner must never execute fork code. + expect(condition).toContain( + "github.event.pull_request.head.repo.full_name == github.repository", + ); const commands = controlled?.steps?.flatMap((step) => (step.run ? [step.run] : [])).join("\n"); expect(commands).toContain("--rounds 10"); expect(commands).toContain("src/check.ts"); @@ -206,6 +215,30 @@ describe("contributor and CI toolchain contract", () => { expect(requiredCommand).toContain('test "$BROWSER" = skipped'); }); + it("gates the performance benchmark without letting fork pull requests bypass it", () => { + const jobs = parsedWorkflow.jobs ?? {}; + const preflight = jobs.preflight; + expect(preflight?.outputs?.perf_required).toBe("$" + "{{ steps.paths.outputs.perf_required }}"); + + const requiredJob = jobs.required; + const requiredCommand = requiredJob?.steps?.find((step) => + step.run?.includes('test "$PREFLIGHT" = success'), + )?.run; + + // A diff that needs no measurement may skip, and the skip must be exact. + expect(requiredCommand).toContain('if [ "$PERF_REQUIRED" != "true" ]'); + expect(requiredCommand).toContain('test "$PERFORMANCE" = skipped'); + expect(requiredCommand).toContain('test "$PERFORMANCE" = success'); + + // A fork pull request touching performance-sensitive paths must FAIL, not + // pass on a skip. Deferring to a post-merge run would let a fork alter + // `bench/results/render-baseline.json` and be validated against its own + // baseline after the merge already happened. + expect(JSON.stringify(requiredJob)).toContain("PERF_FROM_FORK"); + expect(requiredCommand).toContain('elif [ "$PERF_FROM_FORK" = "true" ]'); + expect(requiredCommand).toContain("exit 1"); + }); + it("shares a source-keyed Rust compilation cache across build and test jobs", () => { const jobs = parsedWorkflow.jobs ?? {}; for (const jobName of ["artifact-build", "unit-coverage"]) { diff --git a/scripts/workflow-contract.test.ts b/scripts/workflow-contract.test.ts index 6beb1988..3c842b53 100644 --- a/scripts/workflow-contract.test.ts +++ b/scripts/workflow-contract.test.ts @@ -121,6 +121,65 @@ describe("CI and release workflow contracts", () => { expect(changesetStatus?.if).toBeUndefined(); }); + it("never grants a workflow write-scoped secrets against untrusted pull-request code", () => { + // `pull_request_target` runs with the base repository's token and secrets while + // checking out attacker-controllable head code. It is the trigger behind a + // well-known class of supply-chain compromises and has no use in this repo. + for (const [name, workflow] of Object.entries(workflows())) { + expect(workflow.on?.pull_request_target, `${name} pull_request_target`).toBeUndefined(); + } + }); + + it("never executes fork pull-request code on a non-ephemeral runner", () => { + // GitHub-hosted runners are destroyed after each job; a self-hosted runner is + // a persistent machine whose filesystem, caches, and network position survive. + // Checking out fork code there is a host-compromise vector that a read-only + // token does not mitigate, so every self-hosted job must be gated to + // same-repository events. + for (const [workflowName, workflow] of Object.entries(workflows())) { + for (const [jobName, job] of Object.entries(workflow.jobs)) { + const runsOn = job["runs-on"]; + const hosted = typeof runsOn === "string" && /^(?:ubuntu|windows|macos)-/.test(runsOn); + if (hosted) continue; + const condition = typeof job.if === "string" ? job.if : ""; + expect( + condition.replace(/\s+/g, " "), + `${workflowName}.${jobName} runs on ${String(runsOn)} and must reject fork pull requests`, + ).toContain("github.event.pull_request.head.repo.full_name == github.repository"); + } + } + }); + + it("pins the CI trigger branches so release branches are an explicit decision", () => { + const triggers = workflows().ci.on; + expect((triggers?.push as { branches?: string[] } | undefined)?.branches).toEqual(["develop"]); + expect((triggers?.pull_request as { branches?: string[] } | undefined)?.branches).toEqual([ + "develop", + ]); + }); + + it("declares every job whose condition reads another job's outputs as a direct dependency", () => { + // `needs` exposes DIRECT dependencies only. A job that reads + // `needs..outputs` without listing `` evaluates against an + // undefined context, so the condition is always false and the job silently + // never runs. + for (const [workflowName, workflow] of Object.entries(workflows())) { + for (const [jobName, job] of Object.entries(workflow.jobs)) { + const condition = typeof job.if === "string" ? job.if : ""; + const referenced = [...condition.matchAll(/needs\.([A-Za-z0-9_-]+)\.outputs/g)].map( + (match) => match[1]!, + ); + if (referenced.length === 0) continue; + const needs = job.needs === undefined ? [] : [job.needs].flat(); + for (const dependency of referenced) { + expect(needs, `${workflowName}.${jobName} reads needs.${dependency}.outputs`).toContain( + dependency, + ); + } + } + } + }); + it("keeps each workflow's required toolchain versions in parity", () => { const parsed = workflows(); for (const [name, workflow] of Object.entries(parsed)) {