From cdd4ec64b7a3b68b877521f197c7525632e7f34c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 12 Oct 2026 14:39:51 -0700 Subject: [PATCH 1/2] ci: scope pull request checks to what the diff can affect A `changes` job classifies the pull request's files against explicit allowlists (.github/scripts/ci-changes.sh, replacing desktop-live-changes.sh): - content posts, author profiles and their public assets: lint, test, build - apps/docs: lint, test - apps/desktop: lint, test, desktop-live - agent guidance (.agents, .claude, .cursor, AGENTS.md, CLAUDE.md, root README): lint Any other file, any .github change, an empty diff, or a diff that cannot be worked out runs everything; pushes and dispatches always run everything. The live desktop suite keeps its own skip list, now including the content asset folders it previously missed. `ci` accepts a skip only when `changes` set that job's flag to exactly `false`, so a failed or skipped `changes`, or a lost output, fails it. --- .github/scripts/ci-changes.sh | 64 ++++++++ .github/scripts/desktop-live-changes.sh | 24 --- .github/workflows/checks.yml | 86 ++++++---- .github/workflows/ci.yml | 5 +- scripts/ci-changes.test.ts | 207 ++++++++++++++++++++++++ scripts/desktop-live-changes.test.ts | 78 --------- 6 files changed, 326 insertions(+), 138 deletions(-) create mode 100755 .github/scripts/ci-changes.sh delete mode 100755 .github/scripts/desktop-live-changes.sh create mode 100644 scripts/ci-changes.test.ts delete mode 100644 scripts/desktop-live-changes.test.ts diff --git a/.github/scripts/ci-changes.sh b/.github/scripts/ci-changes.sh new file mode 100755 index 00000000000..75264dce1c5 --- /dev/null +++ b/.github/scripts/ci-changes.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# Prints which checks a pull request's changes can affect, as `=true|false` lines for +# $GITHUB_OUTPUT. A flag is `false` only when every changed file is on an allowlist whose jobs +# fully cover it. A file on no allowlist sets every flag but `desktop_live` (which keeps its own +# skip list) to `true`; a change under .github/, an empty diff, or any failure to work out the +# diff sets all of them. +# +# full integration and e2e: any file not allowlisted below +# test the Vitest shards: content, docs app, and desktop changes, plus `full` +# build the app's `next build`: content changes, plus `full` +# desktop_live the live Electron suite: any file outside its own skip list +# +# lint always runs and is not gated. +# +# Usage: ci-changes.sh +set -u + +# Posts, author profiles, and the public assets only posts reference. The test shards read them +# (lib/content/og-image.test.ts) and `next build` prerenders every post with its MDX components, +# which check:library-content compiles but never renders. Components under content/ are code. +content='^apps/sim/content/([^/]+/)*[^/]+\.mdx?$|^apps/sim/content/authors/[^/]+\.json$|^apps/sim/public/(authors|blog|changelog|library)/' +# Its own workspace tests, its type-check and docs-manifest audit in lint, and apps/sim tests +# that read its pages (tools/netsuite/netsuite.test.ts). +docs='^apps/docs/' +# Nothing outside the Electron app imports it; the desktop bridge and protocol packages are +# consumed by apps/sim and stay unlisted. +desktop='^apps/desktop/' +# Agent guidance, read only by the lint job's audits. +guidance='^(\.agents|\.claude|\.cursor)/|^([^/]+/)*(AGENTS|CLAUDE)\.md$|^README\.md$' +# The live desktop suite drives apps/desktop against the app, so it skips more than the rest. +desktop_live_skip="$guidance"'|^(apps/docs/|apps/pii/|apps/sim/content/|apps/sim/public/(authors|blog|changelog|library)/|packages/(python-sdk|ts-sdk)/)|\.mdx?$|(^|/)LICENSE$' + +emit() { + printf 'full=%s\ntest=%s\nbuild=%s\ndesktop_live=%s\n' "$1" "$2" "$3" "$4" +} +run_all() { + emit true true true true + echo "Running every check: $1" >&2 + exit 0 +} +matches() { printf '%s\n' "$names" | grep -qE "$1"; } +outside() { printf '%s\n' "$names" | grep -qvE "$1"; } + +base=${1:-} +[ -n "$base" ] || run_all 'no base commit' +git fetch --quiet --depth=1 origin "$base" || run_all "could not fetch $base" +names=$(git diff --no-renames --name-only "$base" HEAD) || run_all "could not diff against $base" +[ -n "$names" ] || run_all 'no changed files listed' +if matches '^\.github/'; then run_all 'a CI file changed'; fi + +desktop_live=false +if outside "$desktop_live_skip"; then desktop_live=true; fi +if outside "$content|$docs|$desktop|$guidance"; then + emit true true true "$desktop_live" + unlisted=$(printf '%s\n' "$names" | grep -vE "$content|$docs|$desktop|$guidance" | head -n 1) + echo "Running every check (desktop_live=$desktop_live): $unlisted is not on an allowlist" >&2 + exit 0 +fi + +test=false build=false +if matches "$content|$docs|$desktop"; then test=true; fi +if matches "$content"; then build=true; fi +emit false "$test" "$build" "$desktop_live" +echo "Scoped checks: test=$test build=$build desktop_live=$desktop_live" >&2 diff --git a/.github/scripts/desktop-live-changes.sh b/.github/scripts/desktop-live-changes.sh deleted file mode 100755 index d42cc2b2e70..00000000000 --- a/.github/scripts/desktop-live-changes.sh +++ /dev/null @@ -1,24 +0,0 @@ -#!/usr/bin/env bash -# Prints `changed=false` only when every file a pull request changes is clearly unrelated to the -# live desktop suite, and `changed=true` otherwise, including when the diff cannot be worked out. -# -# Usage: desktop-live-changes.sh -set -u - -unrelated='^(apps/docs/|apps/pii/|apps/sim/content/|packages/(python-sdk|ts-sdk)/)|\.mdx?$|(^|/)LICENSE$' - -base=${1:-} -run() { - echo "changed=true" - echo "Running the live desktop suite: $1" >&2 - exit 0 -} -[ -n "$base" ] || run 'no base commit' -git fetch --quiet --depth=1 origin "$base" || run "could not fetch $base" -names=$(git diff --no-renames --name-only "$base" HEAD) || run "could not diff against $base" -[ -n "$names" ] || run 'no changed files listed' -if printf '%s\n' "$names" | grep -qvE "$unrelated"; then - run 'a change may affect it' -fi -echo "changed=false" -echo 'Skipping the live desktop suite: every change is unrelated to it' >&2 diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index ccddacf8261..cc8fb468876 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -8,6 +8,37 @@ permissions: contents: read jobs: + # Which checks a pull request's changes can affect (.github/scripts/ci-changes.sh). A job is + # skipped only when the script prints `false` for its flag; an unset flag, and every push or + # dispatch (the deploy chain's checks), runs it. Every gated job needs this one, so its failure + # skips them and fails `ci` below. + changes: + name: changes + runs-on: &runner-2vcpu ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }} + timeout-minutes: 5 + outputs: + full: ${{ github.event_name != 'pull_request' || steps.classify.outputs.full != 'false' }} + test: ${{ github.event_name != 'pull_request' || steps.classify.outputs.test != 'false' }} + build: ${{ github.event_name != 'pull_request' || steps.classify.outputs.build != 'false' }} + desktop_live: ${{ github.event_name != 'pull_request' || steps.classify.outputs.desktop_live != 'false' }} + steps: + - name: Checkout code (mirror) + if: github.event_name == 'pull_request' && !github.event.pull_request.head.repo.fork + uses: useblacksmith/checkout@25227e61ff9dafe400e22fa487b673eac4e4409a # v1.8.1 + with: + fetch-depth: 2 + - name: Checkout code + if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v6 + with: + fetch-depth: 2 + - name: Classify the pull request's changes + id: classify + if: github.event_name == 'pull_request' + env: + BASE: ${{ github.event.pull_request.base.sha }} + run: bash .github/scripts/ci-changes.sh "$BASE" >> "$GITHUB_OUTPUT" + integration: # Runs the real-infrastructure test layer: every `*.integration.ts` in packages/db and # apps/sim, discovered by glob (`vitest run --mode integration`), against the database each @@ -19,6 +50,8 @@ jobs: # shard runs its files one at a time against its own database. Files run serially, so a shard # barely uses more than one core: 4 vCPU is enough. name: integration (${{ matrix.provision }}, ${{ matrix.shard }}/4) + needs: changes + if: needs.changes.outputs.full == 'true' runs-on: &runner-4vcpu ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-4vcpu-ubuntu-2404' || 'ubuntu-latest' }} timeout-minutes: 20 strategy: @@ -143,6 +176,8 @@ jobs: # self-hosted suites fit on 4. e2e: name: e2e (${{ matrix.group }}) + needs: changes + if: needs.changes.outputs.full == 'true' runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && matrix.runner || 'ubuntu-latest' }} timeout-minutes: ${{ matrix.group == 'mobile' && 40 || 20 }} strategy: @@ -217,39 +252,12 @@ jobs: if-no-files-found: ignore retention-days: 7 - # Pull requests skip the live desktop suite only when every change is clearly unrelated to the - # app it drives (docs, other apps, published content). Anything else, and any failure to work - # out the diff, runs it: a pull request that skipped it wrongly would first fail on staging. - desktop-changes: - name: desktop-changes - runs-on: &runner-2vcpu ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }} - timeout-minutes: 5 - outputs: - changed: ${{ github.event_name != 'pull_request' || steps.diff.outputs.changed != 'false' }} - steps: - - name: Checkout code (mirror) - if: github.event_name == 'pull_request' && !github.event.pull_request.head.repo.fork - uses: useblacksmith/checkout@25227e61ff9dafe400e22fa487b673eac4e4409a # v1.8.1 - with: - fetch-depth: 2 - - name: Checkout code - if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v6 - with: - fetch-depth: 2 - - name: Diff against the pull request's base - id: diff - if: github.event_name == 'pull_request' - env: - BASE: ${{ github.event.pull_request.base.sha }} - run: bash .github/scripts/desktop-live-changes.sh "$BASE" >> "$GITHUB_OUTPUT" - # Desktop tools in the real Electron app against a local app, on its own runner: the # Electron app, the dev app and its realtime server together outgrow the e2e runner. desktop-live: name: desktop-live - needs: desktop-changes - if: needs.desktop-changes.outputs.changed == 'true' + needs: changes + if: needs.changes.outputs.desktop_live == 'true' runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-16vcpu-ubuntu-2404' || 'ubuntu-latest' }} timeout-minutes: 30 services: @@ -592,6 +600,8 @@ jobs: # task hash, so a shard only ever replays its own result. test: name: test (${{ matrix.shard }}/2) + needs: changes + if: needs.changes.outputs.test == 'true' runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-8vcpu-ubuntu-2404' || 'ubuntu-latest' }} timeout-minutes: 15 strategy: @@ -660,6 +670,8 @@ jobs: # not the native Turbopack workers that dominate, so it cannot prevent this. build: name: build + needs: changes + if: needs.changes.outputs.build == 'true' runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-16vcpu-ubuntu-2404' || 'linux-x64-8-core' }} # Build durations crossed 15 minutes as the app grew (10m02 on Jul 29 AM, # 14m44 after the folders/desktop/library merges, then two straight @@ -713,13 +725,14 @@ jobs: run: bunx turbo run build --filter=@sim/app # One status for every check above: the single check to require on a branch ruleset, so adding, - # sharding or renaming a job never means editing the ruleset. Skipped is a pass (the desktop live - # suite skips on pull requests that cannot affect it); a failure or a cancellation is not. + # sharding or renaming a job never means editing the ruleset. A skip passes only for a gated job + # whose `changes` flag is exactly `false`; any other skip (a failed or skipped `changes`, a lost + # output, an ungated job) fails, as does a failure or a cancellation. # `always()`, not `!cancelled()`: a skipped job satisfies a required check, so a gate that skips # on a cancelled run would report a cancelled head commit as passing. ci: name: ci - needs: [integration, e2e, desktop-changes, desktop-live, lint, test, build] + needs: [changes, integration, e2e, desktop-live, lint, test, build] if: ${{ always() }} runs-on: *runner-2vcpu timeout-minutes: 5 @@ -728,7 +741,14 @@ jobs: env: RESULTS: ${{ toJSON(needs) }} run: | - failed="$(jq -r 'to_entries[] | select(.value.result != "success" and .value.result != "skipped") | "\(.key): \(.value.result)"' <<< "$RESULTS")" + failed="$(jq -r ' + {"integration": "full", "e2e": "full", "test": "test", "build": "build", "desktop-live": "desktop_live"} as $gate + | (.changes.outputs // {}) as $flags + | to_entries[] + | select(.value.result != "success") + | select(.value.result != "skipped" or $gate[.key] == null or $flags[$gate[.key]] != "false") + | "\(.key): \(.value.result)" + ' <<< "$RESULTS")" if [ -n "$failed" ]; then echo "::error::Checks did not pass:" echo "$failed" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3d94c3fd75f..29b48038285 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,9 +18,8 @@ name: ci on: push: branches: [main, staging, dev] - # No path filter: `checks / ci` is the required status check, and a pull request whose run - # never starts never reports it, so it could not merge. Docs-only pull requests are rare enough - # that running CI on them costs nothing measurable. + # No path filter: a pull request whose run never starts never reports `checks / ci`. The + # `changes` job in checks.yml scopes the jobs to what the diff can affect instead. pull_request: branches: [main, staging, dev] diff --git a/scripts/ci-changes.test.ts b/scripts/ci-changes.test.ts new file mode 100644 index 00000000000..71ade83d077 --- /dev/null +++ b/scripts/ci-changes.test.ts @@ -0,0 +1,207 @@ +import { spawnSync } from 'node:child_process' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' +import { afterEach, describe, expect, it } from 'vitest' + +const script = fileURLToPath(new URL('../.github/scripts/ci-changes.sh', import.meta.url)) +const fixtures: string[] = [] + +type Flags = Record<'full' | 'test' | 'build' | 'desktop_live', boolean> + +const EVERYTHING: Flags = { full: true, test: true, build: true, desktop_live: true } + +function git(cwd: string, ...args: string[]) { + const result = spawnSync('git', args, { cwd, encoding: 'utf8' }) + if (result.status !== 0) throw new Error(`git ${args.join(' ')}: ${result.stderr}`) + return result.stdout.trim() +} + +function write(repo: string, file: string, content: string) { + mkdirSync(dirname(join(repo, file)), { recursive: true }) + writeFileSync(join(repo, file), content) +} + +/** Builds a clone whose base commit holds `apps/sim/x.ts` and whose origin is a local bare repo. */ +function fixture() { + const root = mkdtempSync(join(tmpdir(), 'ci-changes-')) + fixtures.push(root) + const origin = join(root, 'origin.git') + const repo = join(root, 'repo') + git(root, 'init', '--quiet', '--bare', origin) + git(root, 'init', '--quiet', repo) + git(repo, 'config', 'user.email', 'test@example.com') + git(repo, 'config', 'user.name', 'Test') + git(repo, 'config', 'commit.gpgsign', 'false') + git(repo, 'remote', 'add', 'origin', origin) + write(repo, 'apps/sim/x.ts', 'export const x = 1\n'.repeat(20)) + git(repo, 'add', '-A') + git(repo, 'commit', '--quiet', '-m', 'base') + const base = git(repo, 'rev-parse', 'HEAD') + git(repo, 'push', '--quiet', 'origin', 'HEAD:refs/heads/base') + return { repo, base } +} + +function classify(repo: string, base: string): Flags { + const result = spawnSync('bash', [script, base], { cwd: repo, encoding: 'utf8' }) + expect(result.status, result.stderr).toBe(0) + const lines = result.stdout.trim().split('\n') + const flags = Object.fromEntries(lines.map((line) => line.split('='))) + expect(Object.keys(flags).sort()).toEqual(['build', 'desktop_live', 'full', 'test']) + return { + full: flags.full === 'true', + test: flags.test === 'true', + build: flags.build === 'true', + desktop_live: flags.desktop_live === 'true', + } +} + +/** Commits `files` on top of the base, as a pull request changing exactly those paths. */ +function classifyFiles(files: string[]): Flags { + const { repo, base } = fixture() + for (const file of files) write(repo, file, `${file}\n`) + git(repo, 'add', '-A') + git(repo, 'commit', '--quiet', '-m', 'change') + return classify(repo, base) +} + +afterEach(() => { + for (const root of fixtures.splice(0)) rmSync(root, { recursive: true, force: true }) +}) + +describe('CI change classifier', () => { + it.each<{ name: string; files: string[]; expected: Flags }>([ + { + name: '#8931 library post and its cover', + files: [ + 'apps/sim/content/library/best-ai-agents-web-scraping-market-research/index.mdx', + 'apps/sim/public/library/best-ai-agents-web-scraping-market-research/cover.jpg', + ], + expected: { full: false, test: true, build: true, desktop_live: false }, + }, + { + name: '#8927 changelog review note', + files: ['apps/sim/content/changelog/nebius-models-for-agents/production.md'], + expected: { full: false, test: true, build: true, desktop_live: false }, + }, + { + name: '#8924 changelog post with a docs page and a screenshot', + files: [ + 'apps/docs/content/docs/workflows/blocks/agent.mdx', + 'apps/sim/content/changelog/editorial-standard.md', + 'apps/sim/content/changelog/nebius-models-for-agents/index.mdx', + 'apps/sim/content/changelog/nebius-models-for-agents/production.md', + 'apps/sim/public/changelog/nebius-model-picker-v1.jpg', + ], + expected: { full: false, test: true, build: true, desktop_live: false }, + }, + { + name: 'an author profile', + files: ['apps/sim/content/authors/waleed.json', 'apps/sim/public/authors/waleed.jpg'], + expected: { full: false, test: true, build: true, desktop_live: false }, + }, + { + name: '#8767 docs app', + files: ['apps/docs/app/llms-full.txt/route.ts'], + expected: { full: false, test: true, build: false, desktop_live: false }, + }, + { + name: '#8855 desktop app', + files: ['apps/desktop/e2e/local-files.spec.ts', 'apps/desktop/src/main/menu.ts'], + expected: { full: false, test: true, build: false, desktop_live: true }, + }, + { + name: 'agent guidance only (from #8887)', + files: [ + '.agents/skills/add-block/SKILL.md', + '.claude/rules/sim-testing.md', + '.cursor/rules/sim-testing.mdc', + 'CLAUDE.md', + 'apps/sim/CLAUDE.md', + 'apps/sim/stores/AGENTS.md', + ], + expected: { full: false, test: false, build: false, desktop_live: false }, + }, + { + name: '#8897 docs page with app code', + files: [ + 'apps/docs/content/docs/platform/enterprise/whitelabeling.mdx', + 'apps/sim/app/_shell/application-loading.tsx', + ], + expected: EVERYTHING, + }, + { + name: '#8806 library post with a lockfile', + files: [ + 'apps/sim/content/library/best-ai-agent-marketplaces-template-libraries/index.mdx', + 'apps/sim/public/library/best-ai-agent-marketplaces-template-libraries/cover.jpg', + 'bun.lock', + ], + expected: EVERYTHING, + }, + { + name: '#8937 CI script with app code', + files: ['.github/scripts/http-e2e.sh', 'apps/sim/scripts/test-mobile-e2e.ts'], + expected: EVERYTHING, + }, + { + name: 'a Markdown file under .github', + files: ['.github/CONTRIBUTING.md'], + expected: EVERYTHING, + }, + { + name: '#8772 PII service, outside the live desktop suite', + files: ['apps/pii/server.py'], + expected: { full: true, test: true, build: true, desktop_live: false }, + }, + { + name: 'a component beside a blog post', + files: ['apps/sim/content/blog/v0-5/components.tsx'], + expected: { full: true, test: true, build: true, desktop_live: false }, + }, + { + name: 'a public asset outside the content folders', + files: ['apps/sim/public/landing/hero.png'], + expected: EVERYTHING, + }, + { + name: 'a README that is a test fixture', + files: ['apps/sim/tools/pitchbook/__fixtures__/README.md'], + expected: { full: true, test: true, build: true, desktop_live: false }, + }, + { + name: 'the desktop bridge package, which apps/sim imports', + files: ['packages/desktop-bridge/src/index.ts'], + expected: EVERYTHING, + }, + ])('$name', ({ files, expected }) => { + expect(classifyFiles(files)).toEqual(expected) + }) + + it('runs everything when a file moves from app code into an allowlisted path', () => { + const { repo, base } = fixture() + mkdirSync(join(repo, 'apps/docs'), { recursive: true }) + git(repo, 'mv', 'apps/sim/x.ts', 'apps/docs/x.ts') + git(repo, 'commit', '--quiet', '-m', 'move') + + expect(classify(repo, base)).toEqual(EVERYTHING) + }) + + it('runs everything when the diff is empty', () => { + const { repo, base } = fixture() + git(repo, 'commit', '--quiet', '--allow-empty', '-m', 'empty') + + expect(classify(repo, base)).toEqual(EVERYTHING) + }) + + it('runs everything when the base commit cannot be fetched', () => { + const { repo } = fixture() + expect(classify(repo, '0'.repeat(40))).toEqual(EVERYTHING) + }) + + it('runs everything when no base commit is given', () => { + const { repo } = fixture() + expect(classify(repo, '')).toEqual(EVERYTHING) + }) +}) diff --git a/scripts/desktop-live-changes.test.ts b/scripts/desktop-live-changes.test.ts deleted file mode 100644 index 611414a9bfa..00000000000 --- a/scripts/desktop-live-changes.test.ts +++ /dev/null @@ -1,78 +0,0 @@ -import { spawnSync } from 'node:child_process' -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' -import { tmpdir } from 'node:os' -import { dirname, join } from 'node:path' -import { fileURLToPath } from 'node:url' -import { afterEach, describe, expect, it } from 'vitest' - -const script = fileURLToPath(new URL('../.github/scripts/desktop-live-changes.sh', import.meta.url)) -const fixtures: string[] = [] - -function git(cwd: string, ...args: string[]) { - const result = spawnSync('git', args, { cwd, encoding: 'utf8' }) - if (result.status !== 0) throw new Error(`git ${args.join(' ')}: ${result.stderr}`) - return result.stdout.trim() -} - -function write(repo: string, file: string, content: string) { - mkdirSync(dirname(join(repo, file)), { recursive: true }) - writeFileSync(join(repo, file), content) -} - -/** Builds a clone whose base commit holds `apps/sim/x.ts` and whose origin is a local bare repo. */ -function fixture() { - const root = mkdtempSync(join(tmpdir(), 'desktop-live-changes-')) - fixtures.push(root) - const origin = join(root, 'origin.git') - const repo = join(root, 'repo') - git(root, 'init', '--quiet', '--bare', origin) - git(root, 'init', '--quiet', repo) - git(repo, 'config', 'user.email', 'test@example.com') - git(repo, 'config', 'user.name', 'Test') - git(repo, 'config', 'commit.gpgsign', 'false') - git(repo, 'remote', 'add', 'origin', origin) - write(repo, 'apps/sim/x.ts', 'export const x = 1\n'.repeat(20)) - git(repo, 'add', '-A') - git(repo, 'commit', '--quiet', '-m', 'base') - const base = git(repo, 'rev-parse', 'HEAD') - git(repo, 'push', '--quiet', 'origin', 'HEAD:refs/heads/base') - return { repo, base } -} - -function detect(repo: string, base: string) { - return spawnSync('bash', [script, base], { cwd: repo, encoding: 'utf8' }) -} - -afterEach(() => { - for (const root of fixtures.splice(0)) rmSync(root, { recursive: true, force: true }) -}) - -describe('desktop live change gate', () => { - it('runs the suite when a live file moves into a skipped path', () => { - const { repo, base } = fixture() - mkdirSync(join(repo, 'apps/docs'), { recursive: true }) - git(repo, 'mv', 'apps/sim/x.ts', 'apps/docs/x.ts') - git(repo, 'commit', '--quiet', '-m', 'move') - - const result = detect(repo, base) - expect(result.status, result.stderr).toBe(0) - expect(result.stdout.trim()).toBe('changed=true') - }) - - it('skips the suite when every change is in a skipped path', () => { - const { repo, base } = fixture() - write(repo, 'apps/docs/guide.mdx', '# Guide\n') - git(repo, 'add', '-A') - git(repo, 'commit', '--quiet', '-m', 'docs') - - const result = detect(repo, base) - expect(result.status, result.stderr).toBe(0) - expect(result.stdout.trim()).toBe('changed=false') - }) - - it('runs the suite when the base commit cannot be fetched', () => { - const { repo } = fixture() - const result = detect(repo, '0'.repeat(40)) - expect(result.stdout.trim()).toBe('changed=true') - }) -}) From 6cd28a230479e98c5ed0277871dee3692daca11b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 12 Oct 2026 15:14:44 -0700 Subject: [PATCH 2/2] ci: move the ci gate into a tested script and reject non-boolean change flags The ci aggregator's skip rules now live in .github/scripts/ci-gate.sh, read from NEEDS, with table-driven tests covering every guard. The change-classifier test helper fails on any flag value other than exactly true or false. --- .github/scripts/ci-gate.sh | 39 ++++++++ .github/workflows/checks.yml | 27 ++---- scripts/ci-changes.test.ts | 2 + scripts/ci-gate.test.ts | 182 +++++++++++++++++++++++++++++++++++ 4 files changed, 231 insertions(+), 19 deletions(-) create mode 100755 .github/scripts/ci-gate.sh create mode 100644 scripts/ci-gate.test.ts diff --git a/.github/scripts/ci-gate.sh b/.github/scripts/ci-gate.sh new file mode 100755 index 00000000000..c6dea95a416 --- /dev/null +++ b/.github/scripts/ci-gate.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash +# The `ci` job's verdict: the single required check, so it must never pass a run whose checks did +# not all pass. Reads the job's `needs` context (`toJSON(needs)`) from $NEEDS and exits non-zero, +# naming each offending job, unless every job succeeded or was skipped as `changes` intended. +# +# A skip passes only for a gated job whose `changes` flag is exactly `false`: +# +# integration, e2e full +# test test +# build build +# desktop-live desktop_live +# +# Every other skip fails: an ungated job (lint, `changes` itself), a gated job whose flag is +# `true`, missing, or anything else (a failed or skipped `changes` leaves no outputs). A failure +# or a cancellation always fails. +# +# Usage: NEEDS='' ci-gate.sh +set -euo pipefail + +needs=${NEEDS:-} +if ! jq -e 'type == "object"' <<< "$needs" > /dev/null 2>&1; then + echo "::error::The needs context is not a JSON object: '$needs'" + exit 1 +fi + +failed="$(jq -r ' + {"integration": "full", "e2e": "full", "test": "test", "build": "build", "desktop-live": "desktop_live"} as $gate + | (.changes.outputs // {}) as $flags + | to_entries[] + | select(.value.result != "success") + | select(.value.result != "skipped" or $gate[.key] == null or $flags[$gate[.key]] != "false") + | "\(.key): \(.value.result)" +' <<< "$needs")" +if [ -n "$failed" ]; then + echo "::error::Checks did not pass:" + echo "$failed" + exit 1 +fi +echo "All checks passed." diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index cc8fb468876..747808c42e9 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -725,9 +725,10 @@ jobs: run: bunx turbo run build --filter=@sim/app # One status for every check above: the single check to require on a branch ruleset, so adding, - # sharding or renaming a job never means editing the ruleset. A skip passes only for a gated job - # whose `changes` flag is exactly `false`; any other skip (a failed or skipped `changes`, a lost - # output, an ungated job) fails, as does a failure or a cancellation. + # sharding or renaming a job never means editing the ruleset. .github/scripts/ci-gate.sh decides: + # a skip passes only for a gated job whose `changes` flag is exactly `false`; any other skip (a + # failed or skipped `changes`, a lost output, an ungated job) fails, as does a failure or a + # cancellation. A failed checkout fails the gate too. # `always()`, not `!cancelled()`: a skipped job satisfies a required check, so a gate that skips # on a cancelled run would report a cancelled head commit as passing. ci: @@ -737,21 +738,9 @@ jobs: runs-on: *runner-2vcpu timeout-minutes: 5 steps: + - *checkout-mirror + - *checkout-plain - name: Require every check to pass env: - RESULTS: ${{ toJSON(needs) }} - run: | - failed="$(jq -r ' - {"integration": "full", "e2e": "full", "test": "test", "build": "build", "desktop-live": "desktop_live"} as $gate - | (.changes.outputs // {}) as $flags - | to_entries[] - | select(.value.result != "success") - | select(.value.result != "skipped" or $gate[.key] == null or $flags[$gate[.key]] != "false") - | "\(.key): \(.value.result)" - ' <<< "$RESULTS")" - if [ -n "$failed" ]; then - echo "::error::Checks did not pass:" - echo "$failed" - exit 1 - fi - echo "All checks passed." + NEEDS: ${{ toJSON(needs) }} + run: bash .github/scripts/ci-gate.sh diff --git a/scripts/ci-changes.test.ts b/scripts/ci-changes.test.ts index 71ade83d077..ccafa4d6ba9 100644 --- a/scripts/ci-changes.test.ts +++ b/scripts/ci-changes.test.ts @@ -47,7 +47,9 @@ function classify(repo: string, base: string): Flags { const result = spawnSync('bash', [script, base], { cwd: repo, encoding: 'utf8' }) expect(result.status, result.stderr).toBe(0) const lines = result.stdout.trim().split('\n') + for (const line of lines) expect(line).toMatch(/^[a-z_]+=(true|false)$/) const flags = Object.fromEntries(lines.map((line) => line.split('='))) + expect(lines).toHaveLength(4) expect(Object.keys(flags).sort()).toEqual(['build', 'desktop_live', 'full', 'test']) return { full: flags.full === 'true', diff --git a/scripts/ci-gate.test.ts b/scripts/ci-gate.test.ts new file mode 100644 index 00000000000..19ab78f6b8d --- /dev/null +++ b/scripts/ci-gate.test.ts @@ -0,0 +1,182 @@ +import { spawnSync } from 'node:child_process' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' + +const script = fileURLToPath(new URL('../.github/scripts/ci-gate.sh', import.meta.url)) + +type Result = 'success' | 'failure' | 'cancelled' | 'skipped' +type Job = 'changes' | 'integration' | 'e2e' | 'desktop-live' | 'lint' | 'test' | 'build' +type Results = Partial> + +const GATED: Job[] = ['integration', 'e2e', 'desktop-live', 'test', 'build'] +const ALL_FALSE = { full: 'false', test: 'false', build: 'false', desktop_live: 'false' } + +/** + * The `needs` context GitHub gives the `ci` job: every job succeeded and `changes` set `outputs`, + * with `results` overriding individual jobs. + */ +function needs(outputs: unknown, results: Results = {}) { + const jobs: Job[] = ['changes', ...GATED, 'lint'] + return Object.fromEntries( + jobs.map((job) => [ + job, + { + result: results[job] ?? 'success', + outputs: job === 'changes' ? outputs : {}, + }, + ]) + ) +} + +const skipGated: Results = Object.fromEntries(GATED.map((job) => [job, 'skipped' as const])) + +interface FailureCase { + name: string + outputs: unknown + results: Results + reported: string[] +} + +/** Runs the gate; `reported` holds the `: ` lines it failed on. */ +function gate(context: string) { + const result = spawnSync('bash', [script], { + encoding: 'utf8', + env: { PATH: process.env.PATH, NEEDS: context }, + }) + const reported = result.stdout + .split('\n') + .filter((line) => /^[a-z0-9-]+: [a-z]+$/.test(line)) + .sort() + return { status: result.status, stdout: result.stdout, reported } +} + +describe('CI gate', () => { + it('passes when every job succeeded', () => { + const { status, stdout } = gate(JSON.stringify(needs(ALL_FALSE))) + expect(status).toBe(0) + expect(stdout).toBe('All checks passed.\n') + }) + + it('passes gated jobs skipped because their flag is exactly false', () => { + const { status, stdout } = gate(JSON.stringify(needs(ALL_FALSE, skipGated))) + expect(status).toBe(0) + expect(stdout).toBe('All checks passed.\n') + }) + + it.each([ + { + name: 'a skip whose flag is true', + outputs: { ...ALL_FALSE, build: 'true' }, + results: skipGated, + reported: ['build: skipped'], + }, + { + name: 'a skip whose flag is missing', + outputs: { full: 'false', test: 'false', desktop_live: 'false' }, + results: skipGated, + reported: ['build: skipped'], + }, + { + name: 'a skip whose flag is a misspelling', + outputs: { ...ALL_FALSE, full: 'flase' }, + results: skipGated, + reported: ['e2e: skipped', 'integration: skipped'], + }, + { + name: 'a skip whose flag is differently cased', + outputs: { ...ALL_FALSE, test: 'False' }, + results: skipGated, + reported: ['test: skipped'], + }, + { + name: 'a skip whose flag is empty', + outputs: { ...ALL_FALSE, desktop_live: '' }, + results: skipGated, + reported: ['desktop-live: skipped'], + }, + { + name: 'a skip whose flag is a boolean, not the string GitHub sets', + outputs: { ...ALL_FALSE, build: false }, + results: skipGated, + reported: ['build: skipped'], + }, + { + name: 'skips with no changes outputs', + outputs: {}, + results: skipGated, + reported: [ + 'build: skipped', + 'desktop-live: skipped', + 'e2e: skipped', + 'integration: skipped', + 'test: skipped', + ], + }, + { + name: 'skips with a null changes outputs', + outputs: null, + results: skipGated, + reported: [ + 'build: skipped', + 'desktop-live: skipped', + 'e2e: skipped', + 'integration: skipped', + 'test: skipped', + ], + }, + { + name: 'a failed changes job', + outputs: {}, + results: { ...skipGated, changes: 'failure' }, + reported: [ + 'build: skipped', + 'changes: failure', + 'desktop-live: skipped', + 'e2e: skipped', + 'integration: skipped', + 'test: skipped', + ], + }, + { + name: 'a skipped changes job, even with flags that would allow the skips', + outputs: ALL_FALSE, + results: { ...skipGated, changes: 'skipped' }, + reported: ['changes: skipped'], + }, + { + name: 'a failed job whose flag is false', + outputs: ALL_FALSE, + results: { test: 'failure' }, + reported: ['test: failure'], + }, + { + name: 'a failed ungated job', + outputs: ALL_FALSE, + results: { lint: 'failure' }, + reported: ['lint: failure'], + }, + { + name: 'a cancelled job whose flag is false', + outputs: ALL_FALSE, + results: { e2e: 'cancelled' }, + reported: ['e2e: cancelled'], + }, + { + name: 'a skipped lint, which is never gated', + outputs: ALL_FALSE, + results: { ...skipGated, lint: 'skipped' }, + reported: ['lint: skipped'], + }, + ])('fails on $name', ({ outputs, results, reported }) => { + const result = gate(JSON.stringify(needs(outputs, results))) + expect(result.status, result.stdout).toBe(1) + expect(result.stdout).toContain('::error::Checks did not pass:') + expect(result.reported).toEqual(reported) + }) + + it.each(['', 'not json', '[]', 'null'])('fails on a needs context of %j', (context) => { + const { status, stdout } = gate(context) + expect(status).toBe(1) + expect(stdout).toContain('::error::The needs context is not a JSON object') + }) +})