mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
ci: stop docs-only commits from starting the skill-roundtrip matrix
A cancelled Skill update round trip on a README merge painted main red because push to main had no path filter. Share the PR path list on push, and skip expensive PR Checks when every changed file is docs.
This commit is contained in:
@@ -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" \
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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')
|
||||
}
|
||||
@@ -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"')
|
||||
})
|
||||
})
|
||||
@@ -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}"`)
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user