From 080c56289863d45bd7cf76325e178273fafa2357 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 28 Sep 2026 00:37:26 -0700 Subject: [PATCH] perf(ci): diff against the merge commit's first parent so PR checkouts can be shallow (#23562) Every changed-path gate asked git for `--merge-base "$BASE_SHA" "$HEAD_SHA"`, which needs the event payload's base SHA to be in the local graph. That is the only reason two jobs cloned all 8127 refs' history. On a pull_request checkout HEAD is already the merge commit, so its first parent is the base side and no merge base has to be computed. config/scripts/git-pull-request-diff-base.mjs resolved that for the two Node gates; the workflow's inline gates now use the same helper through a small CLI rather than open-coding it. code_paths gates all 22 jobs, so its checkout is charged to the start of every one of them: measured 20.7s to 1.6s, keeping blob:none because its sparse tree is ~7 files and leaves no blobs to refetch. Static analysis drops the filter instead, since populating all 30,226 files makes blob:none force a second promisor fetch: 23s to ~11s. Verified on a real merge ref. At depth 50 the old and new forms produce identical changed-file sets. At depth 2 the new form still works and the old one fails with `fatal: bad object`, which is the failure a stale base would have caused once the checkout stopped being complete. Also drops the dead resolveBase + merge-base prelude in the changed-code gate, whose result resolvePullRequestDiffBase already discarded on every PR. --- .github/workflows/pr.yml | 52 +++++++++++-------- config/scripts/check-changed-code-quality.mjs | 19 ++++--- .../check-root-directory-entries.test.mjs | 8 ++- config/scripts/git-pull-request-diff-base.mjs | 13 +++++ ...alization-extraction-change-scope.test.mjs | 11 ++-- ...orcad-terminal-smoke-change-scope.test.mjs | 8 +-- config/scripts/pr-code-change-scope.test.mjs | 6 ++- config/scripts/pr-e2e-gate-contract.test.mjs | 4 +- 8 files changed, 80 insertions(+), 41 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index be443ced225..ec123b052e6 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -59,12 +59,15 @@ jobs: - 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 + # Why depth 50 and not 0: every diff below resolves to HEAD^1, so only the merge commit + # and a little slack are needed. Fetching all 8127 refs' commit graph cost ~20s here and + # is charged to the start of all 22 jobs, since they all need this one. + # Why blob:none stays: the sparse tree below is ~7 files, so there are no blobs to + # materialize and no promisor refetch. Measured 9.5s -> 1.6s against 20.7s today. + fetch-depth: 50 filter: blob:none sparse-checkout: | + /config/scripts/git-pull-request-diff-base.mjs /package.json /README.md /docs/readme/ @@ -79,8 +82,9 @@ jobs: - name: Reject new root-level files and folders env: BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} - run: node .github/scripts/check-root-directory-entries.mjs "$BASE_SHA" "$HEAD_SHA" + run: | + DIFF_BASE="$(node config/scripts/git-pull-request-diff-base.mjs "$BASE_SHA")" + node .github/scripts/check-root-directory-entries.mjs "$DIFF_BASE" HEAD # The full Git index retains link targets outside the sparse working tree. - name: Check README local links @@ -99,12 +103,15 @@ jobs: id: filter env: BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | set -euo pipefail + # Why HEAD^1 and not --merge-base: HEAD is the pull request merge commit, so its first + # parent is the base side already. Computing a merge base instead would require the + # payload base SHA to be in the graph, which is what forced a full-history checkout. + DIFF_BASE="$(node config/scripts/git-pull-request-diff-base.mjs "$BASE_SHA")" # 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")" + CHANGED="$(git diff --name-only --no-renames --diff-filter=ACDMR "$DIFF_BASE" HEAD)" echo "Changed paths:" printf '%s\n' "$CHANGED" printf '%s\n' "$CHANGED" | node config/scripts/pr-code-change-scope.mjs | tee -a "$GITHUB_OUTPUT" @@ -116,8 +123,8 @@ jobs: run: | set -euo pipefail BASE="${{ github.event.pull_request.base.sha }}" - HEAD="${{ github.event.pull_request.head.sha }}" - CHANGED="$(git diff --name-only --diff-filter=AMCR --merge-base "$BASE" "$HEAD")" + DIFF_BASE="$(node config/scripts/git-pull-request-diff-base.mjs "$BASE")" + CHANGED="$(git diff --name-only --diff-filter=AMCR "$DIFF_BASE" HEAD)" # Source routes are executable contracts so a test can prove exact # authorities, exclusions, and sentinels without evaluating workflow shell. TEST_FILES_JSON="$(printf '%s\n' "$CHANGED" | node config/scripts/pr-e2e-source-routing.mjs)" @@ -131,7 +138,7 @@ jobs: # trigger on IME source rather than on a spec name in some route's list. NATIVE_IME_SOURCE_CHANGED="$(printf '%s\n' "$CHANGED" | node config/scripts/pr-e2e-source-routing.mjs --native-ime-source)" echo "native_ime_source_changed=$NATIVE_IME_SOURCE_CHANGED" >> "$GITHUB_OUTPUT" - WSL_CHANGED="$(git diff --name-only --no-renames --diff-filter=ACDMR --merge-base "$BASE" "$HEAD")" + WSL_CHANGED="$(git diff --name-only --no-renames --diff-filter=ACDMR "$DIFF_BASE" HEAD)" WSL_SOURCE_CHANGED="$(printf '%s\n' "$WSL_CHANGED" | node config/scripts/pr-e2e-source-routing.mjs --wsl-source)" echo "wsl_source_changed=$WSL_SOURCE_CHANGED" >> "$GITHUB_OUTPUT" echo "Native IME source changed: $NATIVE_IME_SOURCE_CHANGED" @@ -154,11 +161,12 @@ jobs: - 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 + # Why depth 50: the gates below diff against HEAD^1, so no merge base is computed and + # the payload base SHA need not be in the graph. + # Why no blob:none here, unlike code_paths: this job checks out all 30,226 files, and + # the filter then forces a second promisor fetch of nearly every blob. Measured 23s + # blobless against ~11s without it. + fetch-depth: 50 persist-credentials: false # Why two guarded installs: the mixed root+mobile store entry is 537 MB against @@ -228,9 +236,9 @@ jobs: - name: Check VM runtime rollback compatibility env: BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - if git diff --quiet --merge-base "$BASE_SHA" "$HEAD_SHA" -- \ + DIFF_BASE="$(node config/scripts/git-pull-request-diff-base.mjs "$BASE_SHA")" + if git diff --quiet "$DIFF_BASE" HEAD -- \ src/shared/ephemeral-vm-runtime-store.ts \ src/shared/ephemeral-vm-runtime-feature-store.ts \ src/shared/ephemeral-vm-runtime-rollback-projection.ts \ @@ -262,10 +270,10 @@ jobs: background: true env: BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | # Detection failures run the full check; renames retain the removed input path. - if git diff --name-only --no-renames -z --merge-base "$BASE_SHA" "$HEAD_SHA" > "$RUNNER_TEMP/localization-changes" && + DIFF_BASE="$(node config/scripts/git-pull-request-diff-base.mjs "$BASE_SHA")" + if git diff --name-only --no-renames -z "$DIFF_BASE" HEAD > "$RUNNER_TEMP/localization-changes" && scope="$(node config/scripts/localization-extraction-change-scope.mjs "$RUNNER_TEMP/localization-changes")" && [ "$scope" = false ]; then echo "Localization extraction inputs are unchanged." else @@ -278,11 +286,11 @@ jobs: - name: Boot orcad and round-trip a terminal env: BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} ORCA_BACKGROUND_LAUNCH: '1' run: | # Detection failures run the smoke; renames retain the removed input path. - if git diff --name-only --no-renames -z --merge-base "$BASE_SHA" "$HEAD_SHA" > "$RUNNER_TEMP/orcad-smoke-changes" && + DIFF_BASE="$(node config/scripts/git-pull-request-diff-base.mjs "$BASE_SHA")" + if git diff --name-only --no-renames -z "$DIFF_BASE" HEAD > "$RUNNER_TEMP/orcad-smoke-changes" && scope="$(node config/scripts/orcad-terminal-smoke-change-scope.mjs "$RUNNER_TEMP/orcad-smoke-changes")" && [ "$scope" = false ]; then echo "Orcad terminal smoke inputs are unchanged." else diff --git a/config/scripts/check-changed-code-quality.mjs b/config/scripts/check-changed-code-quality.mjs index a49ad1ddb51..71a512ad407 100644 --- a/config/scripts/check-changed-code-quality.mjs +++ b/config/scripts/check-changed-code-quality.mjs @@ -136,9 +136,14 @@ function resolveBase(root, requestedBase) { } export function collectAddedLineRanges(root, requestedBase) { - const base = resolveBase(root, requestedBase) - const mergeBase = runGit(root, ['merge-base', base, 'HEAD']).trim() - const comparisonBase = resolvePullRequestDiffBase(root, mergeBase) + // On a pull_request checkout HEAD is the merge commit, so its first parent is the base side and + // no merge base has to be computed. Resolving it first is what lets CI checkout shallowly: the + // payload base SHA can lag HEAD^1 by any number of commits and need not be in the graph at all. + // Off that ref (local runs) the requested base is an arbitrary branch tip, so the merge base is + // still what isolates this branch's own lines. + const comparisonBase = + resolvePullRequestDiffBase(root, null) ?? + runGit(root, ['merge-base', resolveBase(root, requestedBase), 'HEAD']).trim() const changedFiles = splitNullDelimited( runGit(root, ['diff', '--name-only', '-z', '--diff-filter=ACMRTUB', comparisonBase, '--']) ) @@ -174,7 +179,7 @@ export function collectAddedLineRanges(root, requestedBase) { const lineCount = readFileSync(absolutePath, 'utf8').split(/\r?\n/).length rangesByFile.set(file, [{ start: 1, end: lineCount }]) } - return { base, comparisonBase, rangesByFile } + return { comparisonBase, rangesByFile } } function parseOxlintOutput(stdout, label) { @@ -414,10 +419,12 @@ export function main( root = process.cwd(), requestedBase = process.argv.slice(2).find((argument) => argument !== '--') ) { - const { base, comparisonBase, rangesByFile } = collectAddedLineRanges(root, requestedBase) + const { comparisonBase, rangesByFile } = collectAddedLineRanges(root, requestedBase) const files = [...rangesByFile.keys()] if (files.length === 0) { - console.log(`Changed-code quality gate: no changed JavaScript or TypeScript since ${base}.`) + console.log( + `Changed-code quality gate: no changed JavaScript or TypeScript since ${comparisonBase.slice(0, 12)}.` + ) return 0 } diff --git a/config/scripts/check-root-directory-entries.test.mjs b/config/scripts/check-root-directory-entries.test.mjs index f606d5623fd..6de07a053c7 100644 --- a/config/scripts/check-root-directory-entries.test.mjs +++ b/config/scripts/check-root-directory-entries.test.mjs @@ -228,8 +228,14 @@ describe('root directory guard', () => { expect(guardJob.if).toBeUndefined() expect(guardStep.if).toBeUndefined() - expect(guardJob.steps[0].with['fetch-depth']).toBe(0) + // Why >= 2 rather than 0: the guard compares the merge commit's first parent against the + // merged tree, so it needs both parents present but no history beyond them. Depth 1 would + // leave HEAD^1 unreachable and the guard would fail closed on every run. + expect(guardJob.steps[0].with['fetch-depth']).toBeGreaterThanOrEqual(2) expect(guardStep.run).toContain('node .github/scripts/check-root-directory-entries.mjs') + // The base side must come from the merge ref, not the event payload, or a shallow checkout + // cannot resolve it. + expect(guardStep.run).toContain('git-pull-request-diff-base.mjs') expect(workflow.jobs.verify.needs).toContain('code_paths') }) }) diff --git a/config/scripts/git-pull-request-diff-base.mjs b/config/scripts/git-pull-request-diff-base.mjs index 3e2021a1eb1..0dd5b825ef7 100644 --- a/config/scripts/git-pull-request-diff-base.mjs +++ b/config/scripts/git-pull-request-diff-base.mjs @@ -1,5 +1,6 @@ import { execFileSync } from 'node:child_process' import process from 'node:process' +import { pathToFileURL } from 'node:url' export function selectPullRequestDiffBase(requestedBase, headParents, eventName) { if (eventName === 'pull_request' && headParents.length >= 2) { @@ -21,3 +22,15 @@ export function resolvePullRequestDiffBase( .split(/\s+/) return selectPullRequestDiffBase(requestedBase, headParents, eventName) } + +// Why a CLI: the workflow's inline gates used `git diff --merge-base "$BASE_SHA" "$HEAD_SHA"`, +// which needs the payload base to be reachable and so forced a full-history checkout. Printing +// the resolved base lets those steps diff against `HEAD^1` with the same logic the Node gates +// already use, instead of each one open-coding it. +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + const resolved = resolvePullRequestDiffBase(process.cwd(), process.argv[2]) + if (!resolved) { + throw new Error('No diff base: pass the pull request base SHA, or run on a merge ref.') + } + process.stdout.write(`${resolved}\n`) +} diff --git a/config/scripts/localization-extraction-change-scope.test.mjs b/config/scripts/localization-extraction-change-scope.test.mjs index eb3aae13493..8852965ad30 100644 --- a/config/scripts/localization-extraction-change-scope.test.mjs +++ b/config/scripts/localization-extraction-change-scope.test.mjs @@ -43,12 +43,13 @@ it('preserves deleted and renamed inputs and falls back to extraction on detecti (candidate) => candidate.name === 'Verify localization extraction' ) expect(step.env).toEqual({ - BASE_SHA: '${{ github.event.pull_request.base.sha }}', - HEAD_SHA: '${{ github.event.pull_request.head.sha }}' + BASE_SHA: '${{ github.event.pull_request.base.sha }}' }) - expect(step.run).toContain( - 'git diff --name-only --no-renames -z --merge-base "$BASE_SHA" "$HEAD_SHA"' - ) + // The base side comes from the merge ref's first parent, so the gate needs no merge base and + // works on a shallow checkout. The payload head SHA is no longer read: HEAD is the merged tree + // the gate is actually deciding about. + expect(step.run).toContain('node config/scripts/git-pull-request-diff-base.mjs "$BASE_SHA"') + expect(step.run).toContain('git diff --name-only --no-renames -z "$DIFF_BASE" HEAD') expect(step.run).not.toContain('--diff-filter') expect(step.run).toContain('&& [ "$scope" = false ]; then') expect(step.run).toMatch(/else\s+pnpm run verify:localization-extraction\s+fi/) diff --git a/config/scripts/orcad-terminal-smoke-change-scope.test.mjs b/config/scripts/orcad-terminal-smoke-change-scope.test.mjs index f08a12848fd..e0e978800fc 100644 --- a/config/scripts/orcad-terminal-smoke-change-scope.test.mjs +++ b/config/scripts/orcad-terminal-smoke-change-scope.test.mjs @@ -97,12 +97,12 @@ it('only skips the unchanged smoke after a successful diff and dependency analys ) expect(step.env).toEqual({ BASE_SHA: '${{ github.event.pull_request.base.sha }}', - HEAD_SHA: '${{ github.event.pull_request.head.sha }}', ORCA_BACKGROUND_LAUNCH: '1' }) - expect(step.run).toContain( - 'git diff --name-only --no-renames -z --merge-base "$BASE_SHA" "$HEAD_SHA"' - ) + // See the localization gate: HEAD^1 removes the merge-base computation, so a shallow checkout + // is enough and the payload head SHA is unused. + expect(step.run).toContain('node config/scripts/git-pull-request-diff-base.mjs "$BASE_SHA"') + expect(step.run).toContain('git diff --name-only --no-renames -z "$DIFF_BASE" HEAD') expect(step.run).not.toContain('--diff-filter') expect(step.run).toContain('&& [ "$scope" = false ]; then') expect(step.run).toMatch(/else\s+pnpm run smoke:orcad-terminal\s+fi/) diff --git a/config/scripts/pr-code-change-scope.test.mjs b/config/scripts/pr-code-change-scope.test.mjs index 5f2823b6e44..c23ea0e1140 100644 --- a/config/scripts/pr-code-change-scope.test.mjs +++ b/config/scripts/pr-code-change-scope.test.mjs @@ -530,7 +530,11 @@ describe('PR Checks skip wiring', () => { ) expect(classify.run).toContain('--diff-filter=ACDMR') expect(classify.run).toContain('--no-renames') - expect(classify.run).toContain('--merge-base "$BASE_SHA" "$HEAD_SHA"') + // HEAD is the merge commit, so HEAD^1 is the base side and no merge base is computed. + // That is what lets this job check out shallowly, which every other job waits on. + expect(classify.run).toContain('node config/scripts/git-pull-request-diff-base.mjs "$BASE_SHA"') + expect(classify.run).toContain('"$DIFF_BASE" HEAD') + expect(classify.run).not.toContain('--merge-base "$') expect(classify.run).toContain('node config/scripts/pr-code-change-scope.mjs') expect(classify.run).toContain('tee -a "$GITHUB_OUTPUT"') expect(prWorkflow.jobs.code_paths.outputs.should_run).toBe( diff --git a/config/scripts/pr-e2e-gate-contract.test.mjs b/config/scripts/pr-e2e-gate-contract.test.mjs index 518cf62b930..4bcb1b4f1af 100644 --- a/config/scripts/pr-e2e-gate-contract.test.mjs +++ b/config/scripts/pr-e2e-gate-contract.test.mjs @@ -238,7 +238,7 @@ describe('PR E2E gate contract', () => { }) it('scopes detection to the PR range so base drift cannot false-trigger', () => { - expect(filterStep.run).toContain('--merge-base "$BASE" "$HEAD"') + expect(filterStep.run).toMatch(/diff-base\.mjs "\$BASE"[\s\S]*"\$DIFF_BASE" HEAD/) expect(filterStep.run).toContain('set -euo pipefail') }) @@ -444,7 +444,7 @@ describe('PR E2E gate contract', () => { }) it('scopes the VM rollback oracle to the PR range and recipe schema authorities', () => { - expect(rollbackStep.run).toContain('--merge-base "$BASE_SHA" "$HEAD_SHA"') + expect(rollbackStep.run).toMatch(/diff-base\.mjs "\$BASE_SHA"[\s\S]*"\$DIFF_BASE" HEAD --/) expect(rollbackStep.run).toContain('src/shared/ephemeral-vm-recipes.ts') expect(rollbackStep.run).toContain('src/shared/orca-yaml-hook-types.ts') expect(selectPrE2eSpecs(['src/shared/ephemeral-vm-recipes.ts'])).toEqual([