diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 4717da652fc..48b545ef3ea 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -16,8 +16,46 @@ permissions: contents: read jobs: + # Why: a README/docs-only PR used to start the full matrix (32 test shards, + # two package jobs, typecheck, git compat, xterm, shell contracts). Path + # filters on `on.pull_request` would drop the `verify` check entirely; this + # detector keeps verify as the required aggregate and skips the expensive jobs. + code_paths: + name: detect code-relevant changes + runs-on: ubuntu-latest + outputs: + should_run: ${{ steps.filter.outputs.should_run }} + steps: + - name: Checkout + uses: actions/checkout@v6 + with: + # Why blob:none: full history is needed for the merge-base diff, but historical + # file contents are not. Blobs are ~89% of this repo's pack, and Git fetches the + # few this job actually reads on demand. + fetch-depth: 0 + filter: blob:none + persist-credentials: false + + - name: Classify changed paths + id: filter + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + # Why --no-renames: name-only rename detection can report only the destination. + # A code file moved under docs/ must still expose its code-side deletion. + CHANGED="$(git diff --name-only --no-renames --diff-filter=ACDMR --merge-base "$BASE_SHA" "$HEAD_SHA")" + echo "Changed paths:" + printf '%s\n' "$CHANGED" + SHOULD_RUN="$(printf '%s\n' "$CHANGED" | node config/scripts/pr-code-change-scope.mjs)" + echo "should_run=$SHOULD_RUN" >> "$GITHUB_OUTPUT" + echo "should_run=$SHOULD_RUN" + static_analysis: name: static analysis + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest steps: @@ -140,6 +178,8 @@ jobs: run: node .github/scripts/check-root-directory-entries.mjs "$BASE_SHA" "$HEAD_SHA" typecheck: + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest steps: @@ -166,6 +206,8 @@ jobs: git_compatibility: name: Git compatibility + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest steps: @@ -231,6 +273,8 @@ jobs: xterm_patch_sync: name: xterm patch sync + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest steps: @@ -259,6 +303,8 @@ jobs: shell_contracts: name: shell contracts + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest # Why: this job's cost is almost entirely package download, and a stalled mirror has # no wall-clock bound of its own. A successful run finishes in ~4.5 minutes, so this @@ -372,6 +418,8 @@ jobs: test: name: tests node ${{ matrix.node }} ${{ matrix.shard }}/${{ matrix.shard_total }} + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest strategy: fail-fast: false @@ -416,6 +464,8 @@ jobs: cross-version-wire: name: cross-version wire compatibility + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest steps: @@ -443,6 +493,8 @@ jobs: managed_hook_node18: name: managed hooks on Node 18 + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest steps: @@ -466,6 +518,8 @@ jobs: package: name: package + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: ubuntu-latest steps: @@ -527,6 +581,8 @@ jobs: package_windows: name: package (windows) + needs: [code_paths] + if: needs.code_paths.outputs.should_run == 'true' runs-on: windows-2022 timeout-minutes: 30 @@ -592,8 +648,9 @@ jobs: # release runs retain full-suite coverage. e2e-paths: name: detect changed e2e specs + needs: [code_paths] runs-on: ubuntu-latest - if: github.event.pull_request.draft != true + if: github.event.pull_request.draft != true && needs.code_paths.outputs.should_run == 'true' # Why: detector only needs to read the checkout; do not inherit repo defaults. permissions: contents: read @@ -673,6 +730,7 @@ jobs: verify: if: always() needs: + - code_paths - static_analysis - root_directory_guard - typecheck @@ -696,6 +754,8 @@ jobs: # checked outside the loop or it would excuse the jobs above. - name: Require successful checks env: + CODE_PATHS: ${{ needs.code_paths.result }} + SHOULD_RUN: ${{ needs.code_paths.outputs.should_run }} STATIC_ANALYSIS: ${{ needs.static_analysis.result }} ROOT_DIRECTORY_GUARD: ${{ needs.root_directory_guard.result }} TYPECHECK: ${{ needs.typecheck.result }} @@ -707,7 +767,33 @@ jobs: PACKAGE: ${{ needs.package.result }} PACKAGE_WINDOWS: ${{ needs.package_windows.result }} run: | + if [ "$CODE_PATHS" != "success" ]; then + exit 1 + fi + if [ "$SHOULD_RUN" != "true" ]; then + echo "Docs-only change; expensive PR checks skipped." + if [ "$ROOT_DIRECTORY_GUARD" != "success" ]; then + exit 1 + fi + for result in \ + "$STATIC_ANALYSIS" \ + "$TYPECHECK" \ + "$GIT_COMPATIBILITY" \ + "$XTERM_PATCH_SYNC" \ + "$SHELL_CONTRACTS" \ + "$TEST" \ + "$MANAGED_HOOK_NODE18" \ + "$PACKAGE" \ + "$PACKAGE_WINDOWS"; do + if [ "$result" != "skipped" ]; then + exit 1 + fi + done + exit 0 + fi + # Require success when the PR has code-relevant changes for result in \ + "$CODE_PATHS" \ "$STATIC_ANALYSIS" \ "$ROOT_DIRECTORY_GUARD" \ "$TYPECHECK" \ diff --git a/.github/workflows/skill-update-roundtrip.yml b/.github/workflows/skill-update-roundtrip.yml index 6c9418e1f64..71fcf264f69 100644 --- a/.github/workflows/skill-update-roundtrip.yml +++ b/.github/workflows/skill-update-roundtrip.yml @@ -1,18 +1,26 @@ name: Skill update round trip +# Why the same paths on push as pull_request: GitHub evaluates each event's +# filters independently. The PR already skipped README-only changes, but the +# unfiltered `push` to main still started the 13-job matrix. Cancelling that +# run marks the default-branch tip failed, so the repo shows a red X. on: pull_request: - paths: + paths: &skill-roundtrip-paths - 'skills/**' - 'resources/skills/**' - 'config/scripts/verify-skill-update-roundtrip.mjs' + - 'config/scripts/skill-update-roundtrip-workflow.test.mjs' - 'src/main/skills/skill-freshness-eligibility.ts' - 'src/shared/skill-freshness.ts' - '.github/workflows/skill-update-roundtrip.yml' + # Why unfiltered: GitHub ignores `paths` on merge_group. We do not run a merge + # queue today; if one is added, gate the matrix on a path job first. merge_group: push: branches: - main + paths: *skill-roundtrip-paths jobs: roundtrip: diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs new file mode 100644 index 00000000000..d19eee52454 --- /dev/null +++ b/config/scripts/pr-code-change-scope.mjs @@ -0,0 +1,41 @@ +import { readFileSync } from 'node:fs' +import process from 'node:process' +import { pathToFileURL } from 'node:url' + +const DOCS_ONLY_FILES = new Set([ + 'README.md', + 'LICENSE', + 'AGENTS.md', + 'CLAUDE.md', + 'Agents.md', + 'Claude.md', + '.github/CONTRIBUTING.md', + '.github/pull_request_template.md', + '.github/CODEOWNERS' +]) + +const DOCS_ONLY_PREFIXES = ['docs/', '.github/ISSUE_TEMPLATE/'] + +export function isDocsOnlyPath(file) { + if (DOCS_ONLY_FILES.has(file)) { + return true + } + if (DOCS_ONLY_PREFIXES.some((prefix) => file.startsWith(prefix))) { + return true + } + return /^README\.[^/]+\.md$/.test(file) +} + +export function shouldRunPrChecks(changedFiles) { + // Why empty-run: a silent empty diff is more likely a detector bug than a + // genuine no-op PR, so fail closed and keep the expensive jobs. + if (changedFiles.length === 0) { + return true + } + return changedFiles.some((file) => !isDocsOnlyPath(file)) +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + const files = readFileSync(0, 'utf8').split('\n').filter(Boolean) + process.stdout.write(shouldRunPrChecks(files) ? 'true\n' : 'false\n') +} diff --git a/config/scripts/pr-code-change-scope.test.mjs b/config/scripts/pr-code-change-scope.test.mjs new file mode 100644 index 00000000000..f71d6c42934 --- /dev/null +++ b/config/scripts/pr-code-change-scope.test.mjs @@ -0,0 +1,107 @@ +import { readFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' +import { parse } from 'yaml' +import { isDocsOnlyPath, shouldRunPrChecks } from './pr-code-change-scope.mjs' + +const projectDir = resolve(import.meta.dirname, '../..') +const prWorkflow = parse(readFileSync(join(projectDir, '.github/workflows/pr.yml'), 'utf8')) + +const gatedIf = "needs.code_paths.outputs.should_run == 'true'" +const expensiveJobs = [ + 'static_analysis', + 'typecheck', + 'git_compatibility', + 'xterm_patch_sync', + 'shell_contracts', + 'test', + 'cross-version-wire', + 'managed_hook_node18', + 'package', + 'package_windows' +] + +describe('docs-only path classification', () => { + it('treats the WeChat README PR files as docs-only', () => { + expect( + shouldRunPrChecks([ + 'README.md', + 'docs/assets/wechat-qr-group8.jpg', + 'docs/readme/README.zh-CN.md' + ]) + ).toBe(false) + }) + + it('skips root instruction files and GitHub markdown templates', () => { + expect(isDocsOnlyPath('AGENTS.md')).toBe(true) + expect(isDocsOnlyPath('CLAUDE.md')).toBe(true) + expect(isDocsOnlyPath('LICENSE')).toBe(true) + expect(isDocsOnlyPath('.github/CONTRIBUTING.md')).toBe(true) + expect(isDocsOnlyPath('.github/pull_request_template.md')).toBe(true) + expect(isDocsOnlyPath('.github/ISSUE_TEMPLATE/bug_report.yml')).toBe(true) + expect(isDocsOnlyPath('.github/CODEOWNERS')).toBe(true) + }) + + it('still runs PR Checks for product markdown and CI', () => { + expect(isDocsOnlyPath('skills/computer-use/SKILL.md')).toBe(false) + expect(isDocsOnlyPath('skill-guides/orca-cli.md')).toBe(false) + expect(isDocsOnlyPath('.github/workflows/pr.yml')).toBe(false) + expect(isDocsOnlyPath('src/main/index.ts')).toBe(false) + expect(isDocsOnlyPath('config/scripts/pr-code-change-scope.mjs')).toBe(false) + expect(shouldRunPrChecks(['README.md', 'src/main/index.ts'])).toBe(true) + }) + + it('runs PR Checks when the diff is empty rather than skipping by accident', () => { + expect(shouldRunPrChecks([])).toBe(true) + }) +}) + +describe('PR Checks docs-only skip wiring', () => { + it('classifies the PR range with a tested script and expands renames', () => { + const classify = prWorkflow.jobs.code_paths.steps.find( + (step) => step.name === 'Classify changed paths' + ) + expect(classify.run).toContain('--diff-filter=ACDMR') + expect(classify.run).toContain('--no-renames') + expect(classify.run).toContain('--merge-base "$BASE_SHA" "$HEAD_SHA"') + expect(classify.run).toContain('node config/scripts/pr-code-change-scope.mjs') + expect(prWorkflow.jobs.code_paths.outputs.should_run).toBe( + '${{ steps.filter.outputs.should_run }}' + ) + }) + + it('keeps the cheap root-directory guard on docs-only PRs', () => { + expect(prWorkflow.jobs.root_directory_guard.if).toBeUndefined() + expect(prWorkflow.jobs.root_directory_guard.needs).toBeUndefined() + }) + + it('skips expensive jobs unless the detector says the PR has code', () => { + for (const jobName of expensiveJobs) { + expect(prWorkflow.jobs[jobName].needs, jobName).toEqual(['code_paths']) + expect(prWorkflow.jobs[jobName].if, jobName).toBe(gatedIf) + } + }) + + it('skips e2e detection on docs-only PRs without dropping the draft gate', () => { + expect(prWorkflow.jobs['e2e-paths'].needs).toEqual(['code_paths']) + expect(prWorkflow.jobs['e2e-paths'].if).toBe( + "github.event.pull_request.draft != true && needs.code_paths.outputs.should_run == 'true'" + ) + }) + + it('lets verify pass when expensive jobs are skipped for docs-only PRs', () => { + const verifyStep = prWorkflow.jobs.verify.steps.find( + (step) => step.name === 'Require successful checks' + ) + expect(prWorkflow.jobs.verify.needs[0]).toBe('code_paths') + expect(verifyStep.env.SHOULD_RUN).toBe('${{ needs.code_paths.outputs.should_run }}') + expect(verifyStep.run).toContain('"$SHOULD_RUN" != "true"') + const docsOnlyBranch = verifyStep.run.slice( + 0, + verifyStep.run.indexOf('# Require success when the PR has code-relevant changes') + ) + expect(docsOnlyBranch).toContain('if [ "$result" != "skipped" ]') + expect(docsOnlyBranch).not.toContain('"$result" != "success"') + expect(verifyStep.run).toContain('"$ROOT_DIRECTORY_GUARD" != "success"') + }) +}) diff --git a/config/scripts/pr-e2e-gate-contract.test.mjs b/config/scripts/pr-e2e-gate-contract.test.mjs index 46de7b39fcc..92faae907a1 100644 --- a/config/scripts/pr-e2e-gate-contract.test.mjs +++ b/config/scripts/pr-e2e-gate-contract.test.mjs @@ -53,11 +53,17 @@ describe('PR E2E gate contract', () => { // job without adding it to the strict loop fails here instead of silently // leaving that job unenforced. This is what caught GIT_COMPATIBILITY and // SHELL_CONTRACTS being absent from an earlier hardcoded list. - const strictLoop = verifyStep.run.slice(0, verifyStep.run.indexOf('done')) + // Why lastIndexOf: the docs-only branch has its own loop that allows skipped. + const successMarker = '# Require success when the PR has code-relevant changes' + const successLoop = verifyStep.run.slice( + verifyStep.run.indexOf(successMarker), + verifyStep.run.lastIndexOf('done') + ) + expect(successLoop.length).toBeGreaterThan(0) for (const job of prWorkflow.jobs.verify.needs) { const envVar = job.toUpperCase() expect(verifyStep.env[envVar]).toBe(`\${{ needs.${job}.result }}`) - expect(strictLoop).toContain(`"$${envVar}"`) + expect(successLoop).toContain(`"$${envVar}"`) } }) diff --git a/config/scripts/pr-workflow-parallelism.test.mjs b/config/scripts/pr-workflow-parallelism.test.mjs index 5b9ab226502..59ea62b1fc2 100644 --- a/config/scripts/pr-workflow-parallelism.test.mjs +++ b/config/scripts/pr-workflow-parallelism.test.mjs @@ -334,6 +334,7 @@ describe('PR workflow parallelism', () => { it('keeps verify as the aggregate required check', () => { expect(workflow.jobs.verify.needs).toEqual([ + 'code_paths', 'static_analysis', 'root_directory_guard', 'typecheck', diff --git a/config/scripts/skill-update-roundtrip-workflow.test.mjs b/config/scripts/skill-update-roundtrip-workflow.test.mjs new file mode 100644 index 00000000000..f7bea277ed9 --- /dev/null +++ b/config/scripts/skill-update-roundtrip-workflow.test.mjs @@ -0,0 +1,35 @@ +import { readFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' +import { parse } from 'yaml' + +const projectDir = resolve(import.meta.dirname, '../..') +const workflow = parse( + readFileSync(join(projectDir, '.github/workflows/skill-update-roundtrip.yml'), 'utf8') +) + +const expectedPaths = [ + 'skills/**', + 'resources/skills/**', + 'config/scripts/verify-skill-update-roundtrip.mjs', + 'config/scripts/skill-update-roundtrip-workflow.test.mjs', + 'src/main/skills/skill-freshness-eligibility.ts', + 'src/shared/skill-freshness.ts', + '.github/workflows/skill-update-roundtrip.yml' +] + +describe('skill-update-roundtrip workflow triggers', () => { + it('uses the same path filter on push to main as on pull_request', () => { + // Why: GitHub evaluates each event independently. An unfiltered `push` to + // main started the 13-job matrix on README-only merges; cancelling that + // run paints the default-branch tip red. + expect(workflow.on.pull_request.paths).toEqual(expectedPaths) + expect(workflow.on.push.branches).toEqual(['main']) + expect(workflow.on.push.paths).toEqual(workflow.on.pull_request.paths) + }) + + it('does not claim a path filter on merge_group that GitHub would ignore', () => { + expect(workflow.on.merge_group).toBeDefined() + expect(workflow.on.merge_group?.paths).toBeUndefined() + }) +})