ci: optimize checks for stacked pull requests (#12507)
## 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 <noreply@paperclip.ing>
This commit is contained in:
Dotta
GitHub
parent
d7ff895a9a
commit
39b8ee2960
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in New Issue