From 39b8ee2960541d14b380f95365deecba6723d9bd Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sat, 29 Aug 2026 10:26:38 -0500 Subject: [PATCH] ci: optimize checks for stacked pull requests (#12507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip uses GitHub Actions to protect changes before they enter `master`. > - GitHub evaluates every pull request in a native stack against the stack base. > - The current workflow therefore starts the complete CI matrix for every layer in a stack. > - A large stack can queue many copies of the same integrated verification and delay every pull request. > - GitHub provides stack position and base metadata so workflows can select merge-relevant layers. > - This pull request keeps policy and required check names on every layer, but runs full CI only for ordinary pull requests, the top layer, and the lowest unmerged layer. > - The benefit is much lower CI load without weakening the required-check contract. ## Linked Issues or Issue Description **What existing behavior does this improve?** The trusted pull request workflow currently runs every test, build, canary, and E2E lane for every pull request in a native stack. **Subsystem affected** GitHub Actions pull request verification. **Current behavior** A stack with 61 pull requests can start 61 complete CI matrices after a cascading rebase. **Proposed behavior** Run the always-on policy job and stable required-check aggregators for every layer. Run the complete verification matrix only for ordinary pull requests, the top stack layer, and the lowest unmerged stack layer. **Reason and benefit** The top layer verifies the integrated stack. The lowest unmerged layer verifies the current merge candidate. Middle layers keep branch-protection checks without consuming the complete runner matrix. **Breaking changes** Middle stack layers no longer run the complete CI matrix. Their `ci / verify` and `ci / e2e` checks still require the policy job to pass and require every expensive lane to be intentionally skipped. ## What Changed - Add a fail-safe stack scope decision to the trusted PR runner gate. - Run typecheck, general tests, build, serialized tests, canary, and E2E shards only for ordinary, top, and lowest-unmerged pull requests. - Preserve the required `ci / verify` and `ci / e2e` names on every layer. - Make the required aggregators distinguish valid middle-layer skips from failures or missing scope decisions. - Add regression coverage for ordinary, top, bottom, middle, and malformed stack metadata. ## Verification - `node --test scripts/__tests__/e2e-shard.test.mjs` — 11 tests passed. - `actionlint .github/workflows/pr-trusted.yml .github/workflows/pr.yml` — passed. - `git diff --check origin/master...HEAD` — passed. - The caller remains pinned to the current trusted workflow. A separate activation change must advance the immutable SHA after this pull request lands. ## Risks - Incorrect stack classification could skip important jobs. Missing or malformed stack metadata defaults to full CI. - Middle-layer required checks depend on the policy job and verify that all expensive jobs have the `skipped` result. - The reusable workflow change does not become active until the immutable caller SHA advances in a separate change. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex with GPT-5. The exact deployment suffix and context window are not exposed. The model used reasoning, repository tools, code execution, Git, and GitHub API access. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes: #` / `Refs: #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .github/workflows/pr-trusted.yml | 81 ++++++++++++++-- scripts/__tests__/e2e-shard.test.mjs | 137 +++++++++++++++++++++++---- 2 files changed, 196 insertions(+), 22 deletions(-) diff --git a/.github/workflows/pr-trusted.yml b/.github/workflows/pr-trusted.yml index a2e34b4cd1..5bc040f992 100644 --- a/.github/workflows/pr-trusted.yml +++ b/.github/workflows/pr-trusted.yml @@ -22,6 +22,7 @@ jobs: timeout-minutes: 5 outputs: runner: ${{ steps.route.outputs.runner }} + full_ci: ${{ steps.scope.outputs.full_ci }} steps: - name: Validate PR identity and select runner @@ -217,6 +218,41 @@ jobs: echo "runner=$aws_runner" >> "$GITHUB_OUTPUT" echo '::notice title=AWS CI routing::Using an ephemeral RunsOn Fleet runner' + - name: Select stacked PR CI scope + id: scope + shell: bash + env: + STACK_JSON: ${{ toJSON(github.event.pull_request.stack) }} + PR_BASE_REF: ${{ github.event.pull_request.base.ref }} + run: | + set -euo pipefail + + full_ci='true' + reason='ordinary pull request' + + if jq -e 'type == "object"' <<< "$STACK_JSON" >/dev/null 2>&1; then + stack_position="$(jq -r '.position // empty' <<< "$STACK_JSON")" + stack_size="$(jq -r '.size // empty' <<< "$STACK_JSON")" + stack_base_ref="$(jq -r '.base.ref // empty' <<< "$STACK_JSON")" + + if [[ ! "$stack_position" =~ ^[1-9][0-9]*$ ]] || + [[ ! "$stack_size" =~ ^[1-9][0-9]*$ ]] || + (( stack_position > stack_size )) || + [[ -z "$stack_base_ref" ]]; then + reason='malformed stack metadata; defaulting to full CI' + elif (( stack_position == stack_size )); then + reason='top pull request in stack' + elif [[ "$stack_base_ref" == "$PR_BASE_REF" ]]; then + reason='lowest unmerged pull request in stack' + else + full_ci='false' + reason='middle pull request in stack' + fi + fi + + echo "full_ci=$full_ci" >> "$GITHUB_OUTPUT" + echo "::notice title=Stacked PR CI scope::$reason; full_ci=$full_ci" + policy: needs: [gate] runs-on: ${{ needs.gate.outputs.runner }} @@ -323,6 +359,7 @@ jobs: typecheck_release_registry: name: Typecheck + Release Registry needs: [gate, policy] + if: ${{ needs.gate.outputs.full_ci == 'true' }} runs-on: ${{ needs.gate.outputs.runner }} timeout-minutes: 20 @@ -362,6 +399,7 @@ jobs: general_tests: name: General tests (${{ matrix.group_label }}) needs: [gate, policy] + if: ${{ needs.gate.outputs.full_ci == 'true' }} runs-on: ${{ needs.gate.outputs.runner }} timeout-minutes: 20 strategy: @@ -452,24 +490,41 @@ jobs: # Preserve the legacy required-check name while the underlying work runs in parallel. name: verify if: ${{ always() }} - needs: [gate, typecheck_release_registry, general_tests, build] + needs: [gate, policy, typecheck_release_registry, general_tests, build] runs-on: ${{ needs.gate.outputs.runner }} timeout-minutes: 5 steps: - name: Fail if any split verify lane failed env: + FULL_CI: ${{ needs.gate.outputs.full_ci }} + POLICY_RESULT: ${{ needs.policy.result }} TYPECHECK_RELEASE_REGISTRY_RESULT: ${{ needs.typecheck_release_registry.result }} GENERAL_TESTS_RESULT: ${{ needs.general_tests.result }} BUILD_RESULT: ${{ needs.build.result }} run: | - test "$TYPECHECK_RELEASE_REGISTRY_RESULT" = "success" - test "$GENERAL_TESTS_RESULT" = "success" - test "$BUILD_RESULT" = "success" + test "$POLICY_RESULT" = "success" + case "$FULL_CI" in + true) + test "$TYPECHECK_RELEASE_REGISTRY_RESULT" = "success" + test "$GENERAL_TESTS_RESULT" = "success" + test "$BUILD_RESULT" = "success" + ;; + false) + test "$TYPECHECK_RELEASE_REGISTRY_RESULT" = "skipped" + test "$GENERAL_TESTS_RESULT" = "skipped" + test "$BUILD_RESULT" = "skipped" + ;; + *) + echo "Invalid full_ci decision: $FULL_CI" >&2 + exit 1 + ;; + esac build: name: Build needs: [gate, policy] + if: ${{ needs.gate.outputs.full_ci == 'true' }} runs-on: ${{ needs.gate.outputs.runner }} timeout-minutes: 20 @@ -509,6 +564,7 @@ jobs: verify_serialized_server: name: Verify serialized server suites (${{ matrix.shard_label }}) needs: [gate, policy] + if: ${{ needs.gate.outputs.full_ci == 'true' }} runs-on: ${{ needs.gate.outputs.runner }} timeout-minutes: 20 strategy: @@ -570,6 +626,7 @@ jobs: canary_dry_run: name: Canary Dry Run needs: [gate, policy] + if: ${{ needs.gate.outputs.full_ci == 'true' }} runs-on: ${{ needs.gate.outputs.runner }} timeout-minutes: 20 @@ -627,6 +684,7 @@ jobs: e2e_shards: name: e2e shard (${{ matrix.shard_label }}) needs: [gate, policy] + if: ${{ needs.gate.outputs.full_ci == 'true' }} runs-on: ${{ needs.gate.outputs.runner }} timeout-minutes: 30 strategy: @@ -728,12 +786,23 @@ jobs: # across the matrix above (same pattern as the `verify` aggregate). name: e2e if: ${{ always() }} - needs: [gate, e2e_shards] + needs: [gate, policy, e2e_shards] runs-on: ${{ needs.gate.outputs.runner }} timeout-minutes: 5 steps: - name: Fail if any e2e shard failed env: + FULL_CI: ${{ needs.gate.outputs.full_ci }} + POLICY_RESULT: ${{ needs.policy.result }} E2E_SHARDS_RESULT: ${{ needs.e2e_shards.result }} - run: test "$E2E_SHARDS_RESULT" = "success" + run: | + test "$POLICY_RESULT" = "success" + case "$FULL_CI" in + true) test "$E2E_SHARDS_RESULT" = "success" ;; + false) test "$E2E_SHARDS_RESULT" = "skipped" ;; + *) + echo "Invalid full_ci decision: $FULL_CI" >&2 + exit 1 + ;; + esac diff --git a/scripts/__tests__/e2e-shard.test.mjs b/scripts/__tests__/e2e-shard.test.mjs index 9b80a99edf..e1074ea683 100644 --- a/scripts/__tests__/e2e-shard.test.mjs +++ b/scripts/__tests__/e2e-shard.test.mjs @@ -1,6 +1,7 @@ import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; -import { readFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; import test from "node:test"; @@ -39,6 +40,58 @@ function readPinnedTrustedPrWorkflow() { return result.stdout; } +function readWorkflowJobs(workflow) { + const jobs = new Map(); + let current = null; + for (const line of workflow.split("\n")) { + const header = /^ {2}([A-Za-z0-9_-]+):\s*$/.exec(line); + if (header) { + current = header[1]; + jobs.set(current, []); + continue; + } + if (current && /^\S/.test(line)) current = null; + if (current) jobs.get(current).push(line); + } + for (const [id, lines] of jobs) jobs.set(id, lines.join("\n")); + return jobs; +} + +function runStackScope(stack, prBaseRef) { + const workflow = readFileSync(trustedPrWorkflow, "utf8"); + const match = workflow.match( + / - name: Select stacked PR CI scope[\s\S]*? run: \|\n([\s\S]*?)\n\n policy:/, + ); + assert.ok(match, "trusted workflow must define the stacked PR scope script"); + const script = match[1] + .split("\n") + .map((line) => line.replace(/^ {10}/, "")) + .join("\n"); + const scratch = mkdtempSync(path.join(tmpdir(), "paperclip-stack-scope-")); + const output = path.join(scratch, "github-output"); + + try { + const result = spawnSync("bash", ["-c", script], { + encoding: "utf8", + env: { + ...process.env, + GITHUB_OUTPUT: output, + PR_BASE_REF: prBaseRef, + STACK_JSON: JSON.stringify(stack), + }, + }); + assert.equal(result.status, 0, result.stderr); + return Object.fromEntries( + readFileSync(output, "utf8") + .trim() + .split("\n") + .map((line) => line.split("=")), + ); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } +} + test("the e2e shards form a complete, non-overlapping partition", () => { const specs = listE2eSpecs(); assert.ok(specs.length > 0, "expected a non-empty e2e spec set"); @@ -115,19 +168,7 @@ test("the trusted PR workflow keeps a stable aggregate check named e2e over the // as `e2e shard (n/3)`, so the aggregate job below is what keeps the // required-check contract intact — same pattern as the `verify` aggregate. const workflow = readPinnedTrustedPrWorkflow(); - const jobs = new Map(); - let current = null; - for (const line of workflow.split("\n")) { - const header = /^ {2}([A-Za-z0-9_-]+):\s*$/.exec(line); - if (header) { - current = header[1]; - jobs.set(current, []); - continue; - } - if (current && /^\S/.test(line)) current = null; - if (current) jobs.get(current).push(line); - } - for (const [id, lines] of jobs) jobs.set(id, lines.join("\n")); + const jobs = readWorkflowJobs(workflow); const aggregate = jobs.get("e2e"); assert.ok(aggregate, "pr-trusted.yml must define an `e2e` job to satisfy branch protection"); @@ -135,8 +176,8 @@ test("the trusted PR workflow keeps a stable aggregate check named e2e over the assert.match(aggregate, /^ {4}if: \$\{\{ always\(\) \}\}$/m, "the aggregate must run even when a shard fails"); assert.match( aggregate, - /^ {4}needs: \[gate, e2e_shards\]$/m, - "the aggregate must depend on the runner gate and shard matrix", + /^ {4}needs: \[gate, (?:policy, )?e2e_shards\]$/m, + "the aggregate must depend on the runner gate, optional policy gate, and shard matrix", ); assert.match( aggregate, @@ -168,6 +209,70 @@ test("the trusted PR workflow keeps a stable aggregate check named e2e over the } }); +test("the trusted PR workflow limits full CI to merge-relevant stack layers", () => { + const workflow = readFileSync(trustedPrWorkflow, "utf8"); + const jobs = readWorkflowJobs(workflow); + const gate = jobs.get("gate"); + + assert.match(gate, /^ {6}full_ci: \$\{\{ steps\.scope\.outputs\.full_ci \}\}$/m); + assert.match(gate, /STACK_JSON: \$\{\{ toJSON\(github\.event\.pull_request\.stack\) \}\}/); + assert.match(gate, /stack_position == stack_size/); + assert.match(gate, /"\$stack_base_ref" == "\$PR_BASE_REF"/); + + for (const jobId of [ + "typecheck_release_registry", + "general_tests", + "build", + "verify_serialized_server", + "canary_dry_run", + "e2e_shards", + ]) { + assert.match( + jobs.get(jobId), + /^ {4}if: \$\{\{ needs\.gate\.outputs\.full_ci == 'true' \}\}$/m, + `${jobId} must run only when the gate selects full CI`, + ); + } + + assert.doesNotMatch( + jobs.get("policy"), + /needs\.gate\.outputs\.full_ci/, + "the policy job must run on every PR layer", + ); + + const verify = jobs.get("verify"); + assert.match(verify, /^ {4}needs: \[gate, policy, typecheck_release_registry, general_tests, build\]$/m); + assert.match(verify, /POLICY_RESULT: \$\{\{ needs\.policy\.result \}\}/); + assert.match(verify, /test "\$TYPECHECK_RELEASE_REGISTRY_RESULT" = "skipped"/); + assert.match(verify, /test "\$GENERAL_TESTS_RESULT" = "skipped"/); + assert.match(verify, /test "\$BUILD_RESULT" = "skipped"/); + + const e2e = jobs.get("e2e"); + assert.match(e2e, /^ {4}needs: \[gate, policy, e2e_shards\]$/m); + assert.match(e2e, /POLICY_RESULT: \$\{\{ needs\.policy\.result \}\}/); + assert.match(e2e, /false\) test "\$E2E_SHARDS_RESULT" = "skipped"/); +}); + +test("the stacked PR scope selector runs full CI only where intended", () => { + assert.equal(runStackScope(null, "master").full_ci, "true"); + assert.equal( + runStackScope({ position: 11, size: 11, base: { ref: "master" } }, "stack-10").full_ci, + "true", + ); + assert.equal( + runStackScope({ position: 1, size: 11, base: { ref: "master" } }, "master").full_ci, + "true", + ); + assert.equal( + runStackScope({ position: 6, size: 11, base: { ref: "master" } }, "stack-5").full_ci, + "false", + ); + assert.equal( + runStackScope({ position: "invalid", size: 11, base: { ref: "master" } }, "stack-5").full_ci, + "true", + ); +}); + test("the trusted PR workflow passes the shard's spec filter to Playwright without a literal --", () => { // `pnpm run test:e2e -- $specs` forwards the literal separator to Playwright, // so the specs after it are not applied as file filters.