diff --git a/.changes/pr-pipeline/feat-review-lane-holds-3c9e1a52.md b/.changes/pr-pipeline/feat-review-lane-holds-3c9e1a52.md new file mode 100644 index 0000000000..f4d660928b --- /dev/null +++ b/.changes/pr-pipeline/feat-review-lane-holds-3c9e1a52.md @@ -0,0 +1,14 @@ +--- +bump: minor +--- + +### Added + +- Both review lanes hold a pull request for a person: `pr-review-hosted.yml` and `pr-review-security-hosted.yml` call ci-workflows' [`pr-review-cli.yml`](https://github.com/melodic-software/ci-workflows/blob/9aca743c4ed9aaf7870288a36586c3994e9321e4/.github/workflows/pr-review-cli.yml) v0.64.1 with `hold-label: needs-human`, and a changed file under `.github/standards/trusted-actors/**`, `.github/actions/check-trusted-trigger/**`, `**/CODEOWNERS` or `**/.gitmodules` holds as `trust-gate` whatever the review says ([Review-lane holds](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/pr-pipeline/README.md#review-lane-holds)). +- ci-status requires both review lanes on the head of a ready pull request: the newest `pull_request` run of each caller must have a successful status job whose `Apply the hold` step ran. A pending, failed, skipped or summary-mode review fails with the cause named; a lane the kill switch stopped passes; Dependabot, the release App and the standards-sync App are exempt by author id unless their PR changes a hold path. +- ci-status fails when a review-lane hold was lifted by anyone but a person on the base's trusted-actor list: the first removal of the human-gated label after the newest hold comment or lane-added label event decides. +- `pr-feedback-dispatch.yml`'s `rerun-ci-status` job re-runs the head's ci-status job when either review lane completes on an open same-repository pull request, after polling at most 10 minutes for a ci-status already in progress. + +### Changed + +- `fix-ci` dispatch keys on the head's `ci-lanes` commit status instead of ci-status, so a hold, a pending review or a contract failure no longer summons it. diff --git a/.github/actions/resolve-config/callers.test.mjs b/.github/actions/resolve-config/callers.test.mjs index 322ae6937a..ca98ef74a1 100644 --- a/.github/actions/resolve-config/callers.test.mjs +++ b/.github/actions/resolve-config/callers.test.mjs @@ -506,7 +506,9 @@ test("pr-unblock.yml: fix-ci is reported neutral when fix-ci-context skips, and conclusion: calls[0].conclusion.toUpperCase(), title: calls[0].output.title, }, - ].map((run) => ({ __typename: "CheckRun", checkSuite: { app: { databaseId: 15368 } }, ...run })), + ] + .map((run) => ({ __typename: "CheckRun", checkSuite: { app: { databaseId: 15368 } }, ...run })) + .concat({ __typename: "StatusContext", context: "ci-lanes", state: "FAILURE", creator: { login: "github-actions[bot]" } }), pageInfo: { hasNextPage: false }, }, }, @@ -3351,7 +3353,9 @@ test("report job: fix-ci-dispatch reads the neutral not-assessed post as the hea oid: head, statusCheckRollup: { contexts: { - nodes: [{ name: "ci-status", status: "COMPLETED", conclusion: "FAILURE" }, ...runs].map((run) => ({ __typename: "CheckRun", checkSuite: { app: { databaseId: 15368 } }, ...run })), + nodes: [{ name: "ci-status", status: "COMPLETED", conclusion: "FAILURE" }, ...runs] + .map((run) => ({ __typename: "CheckRun", checkSuite: { app: { databaseId: 15368 } }, ...run })) + .concat({ __typename: "StatusContext", context: "ci-lanes", state: "FAILURE", creator: { login: "github-actions[bot]" } }), pageInfo: { hasNextPage: false }, }, }, @@ -4276,7 +4280,9 @@ test("pr-unblock.yml: a fix-ci round with no posted check posts fix-ci failure o conclusion: decision.report.conclusion.toUpperCase(), title: decision.report.output.title, })), - ].map((run) => ({ __typename: "CheckRun", checkSuite: { app: { databaseId: 15368 } }, ...run })), + ] + .map((run) => ({ __typename: "CheckRun", checkSuite: { app: { databaseId: 15368 } }, ...run })) + .concat({ __typename: "StatusContext", context: "ci-lanes", state: "FAILURE", creator: { login: "github-actions[bot]" } }), pageInfo: { hasNextPage: false }, }, }, diff --git a/.github/actions/resolve-config/dispatch-limits.test.mjs b/.github/actions/resolve-config/dispatch-limits.test.mjs index 63a6481578..5bacb0c860 100644 --- a/.github/actions/resolve-config/dispatch-limits.test.mjs +++ b/.github/actions/resolve-config/dispatch-limits.test.mjs @@ -43,10 +43,11 @@ function baseWith(text) { // The lanes App bot's user id (.github/standards/trusted-actors/trusted-actors.json). const LANES_APP = 337595936; -// Lanes App PRs whose head's ci-status failed, as fix-ci-dispatch.mjs lists them. +// Lanes App PRs whose head's lanes failed, as fix-ci-dispatch.mjs lists them. const ciFixPull = (number) => { const head = String(number).padStart(40, "c"); const ciStatus = { __typename: "CheckRun", name: "ci-status", status: "COMPLETED", conclusion: "FAILURE", checkSuite: { app: { databaseId: 15368 } } }; + const ciLanes = { __typename: "StatusContext", context: "ci-lanes", state: "FAILURE", creator: { login: "github-actions[bot]" } }; return { number, isDraft: false, @@ -55,7 +56,7 @@ const ciFixPull = (number) => { headRefOid: head, author: { databaseId: LANES_APP }, labels: { nodes: [] }, - commits: { nodes: [{ commit: { oid: head, statusCheckRollup: { contexts: { nodes: [ciStatus] } } } }] }, + commits: { nodes: [{ commit: { oid: head, statusCheckRollup: { contexts: { nodes: [ciStatus, ciLanes] } } } }] }, }; }; diff --git a/.github/scripts/pr-unblock/dispatch.test.mjs b/.github/scripts/pr-unblock/dispatch.test.mjs index 24f3209830..9f90184931 100644 --- a/.github/scripts/pr-unblock/dispatch.test.mjs +++ b/.github/scripts/pr-unblock/dispatch.test.mjs @@ -551,7 +551,7 @@ test("main records a review-dependency dispatch beside the conflict dispatches", assert.equal(output, "run-ids=9130 9131\n"); }); -// A lanes App PR whose head's ci-status failed, as fix-ci-dispatch.mjs lists it. +// A lanes App PR whose head's lanes failed, as fix-ci-dispatch.mjs lists it. const ciFixPull = (number) => { const head = String(number).padStart(40, "c"); return { @@ -569,7 +569,10 @@ const ciFixPull = (number) => { oid: head, statusCheckRollup: { contexts: { - nodes: [{ __typename: "CheckRun", name: "ci-status", status: "COMPLETED", conclusion: "FAILURE", checkSuite: { app: { databaseId: 15368 } } }], + nodes: [ + { __typename: "CheckRun", name: "ci-status", status: "COMPLETED", conclusion: "FAILURE", checkSuite: { app: { databaseId: 15368 } } }, + { __typename: "StatusContext", context: "ci-lanes", state: "FAILURE", creator: { login: "github-actions[bot]" } }, + ], }, }, }, diff --git a/.github/scripts/pr-unblock/fix-ci-dispatch.mjs b/.github/scripts/pr-unblock/fix-ci-dispatch.mjs index be1d733167..e978702469 100644 --- a/.github/scripts/pr-unblock/fix-ci-dispatch.mjs +++ b/.github/scripts/pr-unblock/fix-ci-dispatch.mjs @@ -1,7 +1,9 @@ // Starts the pr-unblock lane's fix-ci activity for each open pull request the // lanes App opened, or a trusted actor opened (an App's at once, a person's -// once idle; the base config's `adoption` section), whose head's ci-status -// failed (or whose pr-test-windows run failed), from the same +// once idle; the base config's `adoption` section), whose head's lanes failed +// (the `ci-lanes` commit status ci-status records, not ci-status itself, which +// a review-lane hold or a pending review also fails) or whose pr-test-windows +// run failed, from the same // dispatcher run // as dispatch.mjs (pr-unblock-dispatch.yml), so the trigger gate and the // broker check it as they check a resolve-conflicts dispatch. @@ -13,7 +15,7 @@ // when it has no pr-unblock / fix-ci check run from the GitHub Actions app, // or when every such run failed and there are at most `fix-ci-restarts` of // them; at most `fix-ci-per-run` per dispatcher run, lowest PR number first. -// A head whose ci-status passed but whose pr-test-windows run failed is a +// A head whose lanes passed but whose pr-test-windows run failed is a // candidate too. A head is skipped while the lost-run reconciler still re-runs // its failed run (infra-failure.mjs reconcilerOwns: an infra-only failure for // its backoff re-runs, any other for one re-run at attempt 1, inside the @@ -29,7 +31,7 @@ export const DEFAULTS = Object.freeze({ workflow: "pr-unblock.yml", activity: "fix-ci", checkName: "pr-unblock / fix-ci", - // The run whose failure counts: pr-require-checks when ci-status failed, + // The run whose failure counts: pr-require-checks when its lanes failed, // else the advisory pr-test-windows, which ci-status does not aggregate but // the merge gate holds on. requiredWorkflow: ".github/workflows/pr-require-checks.yml", @@ -76,6 +78,7 @@ export const LIST_QUERY = `query($owner: String!, $name: String!, $base: String! nodes { __typename ... on CheckRun { name status conclusion title checkSuite { app { databaseId } } } + ... on StatusContext { context state creator { login } } } pageInfo { hasNextPage } } @@ -92,7 +95,7 @@ export const LIST_QUERY = `query($owner: String!, $name: String!, $base: String! // The check runs the GitHub Actions app posted on the PR's head. function headChecks(node, actionsAppId) { const commit = node.commits?.nodes?.[0]?.commit; - // A cut-off list could hide ci-status or an earlier fix-ci run. + // A cut-off list could hide ci-lanes or an earlier fix-ci run. if (!commit || commit.oid !== node.headRefOid || commit.statusCheckRollup?.contexts?.pageInfo?.hasNextPage) { return null; } @@ -113,13 +116,22 @@ function ownerIdle(node, idleHours, now) { return commit?.oid === node.headRefOid && Date.parse(node.updatedAt) <= since && Date.parse(commit.committedDate) <= since; } -const ciStatusFailed = (checks) => checks.some((run) => run.name === "ci-status" && run.status === "COMPLETED" && run.conclusion === "FAILURE"); +// The head's `ci-lanes` commit status when github-actions[bot] wrote it, as +// ci-status's aggregate step does: the lanes' verdict alone. ci-status itself +// also fails on a hold label, a pending or missing review and the PR +// contract, none of which a CI fix mends. +function lanesState(node) { + const contexts = node.commits?.nodes?.[0]?.commit?.statusCheckRollup?.contexts?.nodes ?? []; + const status = contexts.find((context) => context?.__typename === "StatusContext" && context.context === "ci-lanes"); + return status?.creator?.login === "github-actions[bot]" ? status.state : null; +} + +const lanesFailed = (node) => ["FAILURE", "ERROR"].includes(lanesState(node)); -// ci-status passed and another check failed: an advisory lane's, if any. -const advisoryFailed = (checks, checkName) => - !ciStatusFailed(checks) && - checks.some((run) => run.name === "ci-status" && run.status === "COMPLETED" && run.conclusion === "SUCCESS") && - checks.some((run) => run.name !== checkName && run.status === "COMPLETED" && run.conclusion === "FAILURE"); +// The lanes passed and another check failed: an advisory lane's, if any. +const advisoryFailed = (node, checks, checkName) => + lanesState(node) === "SUCCESS" && + checks.some((run) => run.name !== checkName && run.name !== "ci-status" && run.status === "COMPLETED" && run.conclusion === "FAILURE"); // Why a listed PR is not dispatched, or "" when it is a candidate. export function skipReason(node, { base, appSenderId, actionsAppId, checkName, holdLabels, restarts, adoption = DEFAULTS.adoption, now = Date.now() }) { @@ -149,9 +161,9 @@ export function skipReason(node, { base, appSenderId, actionsAppId, checkName, h if (!checks) { return "head commit or its checks not listed whole"; } - const advisory = advisoryFailed(checks, checkName); - if (!ciStatusFailed(checks) && !advisory) { - return "ci-status has not failed"; + const advisory = advisoryFailed(node, checks, checkName); + if (!lanesFailed(node) && !advisory) { + return "ci-lanes has not failed"; } // node.requiredRun, the head's tested pr-require-checks run (pr-test-windows // when only an advisory check failed) and its latest jobs, is attached by @@ -159,7 +171,7 @@ export function skipReason(node, { base, appSenderId, actionsAppId, checkName, h const required = node.requiredRun; if (required !== undefined) { if (advisory && required.run?.conclusion !== "failure") { - return "ci-status passed and no pr-test-windows run failed"; + return "ci-lanes passed and no pr-test-windows run failed"; } const owned = reconcilerOwns(required, now); if (owned !== "") { @@ -246,7 +258,7 @@ export async function dispatchCiFixes({ } // A failure an outside outage explains, or a first failure, is the // lost-run reconciler's to re-run before a model run fixes it. - const path = ciStatusFailed(headChecks(node, actionsAppId)) ? DEFAULTS.requiredWorkflow : DEFAULTS.advisoryWorkflow; + const path = lanesFailed(node) ? DEFAULTS.requiredWorkflow : DEFAULTS.advisoryWorkflow; let requiredRun; try { requiredRun = await api.readRequiredRun(node.headRefOid, path); diff --git a/.github/scripts/pr-unblock/fix-ci-dispatch.test.mjs b/.github/scripts/pr-unblock/fix-ci-dispatch.test.mjs index 732cc3895d..c3c332f144 100644 --- a/.github/scripts/pr-unblock/fix-ci-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/fix-ci-dispatch.test.mjs @@ -37,8 +37,11 @@ const check = (name, conclusion, extra = {}) => ({ ...extra, }); -// A lanes App PR whose head's ci-status failed and has no fix-ci run yet. -function pr(number, { checks = [check("ci-status", "FAILURE")], ...overrides } = {}) { +// The head's ci-lanes commit status, as ci-status's aggregate step writes it. +const lanes = (state, creator = "github-actions[bot]") => ({ __typename: "StatusContext", context: "ci-lanes", state, creator: { login: creator } }); + +// A lanes App PR whose head's lanes failed and has no fix-ci run yet. +function pr(number, { checks = [lanes("FAILURE")], ...overrides } = {}) { const head = String(number).padStart(40, "a"); return { number, @@ -136,19 +139,19 @@ test("other authors, drafts, forks, other bases and held PRs are left alone", as assert.deepEqual(result.dispatched, []); }); -test("a head whose ci-status passed, is pending, is missing or came from another app is left alone", async () => { +test("a head whose ci-lanes passed, is pending, is missing or came from another writer is left alone", async () => { const { result } = await run({ pages: [ [ - pr(1, { checks: [check("ci-status", "SUCCESS")] }), - pr(2, { checks: [check("ci-status", null)] }), + pr(1, { checks: [lanes("SUCCESS")] }), + pr(2, { checks: [lanes("PENDING")] }), pr(3, { checks: [] }), - pr(4, { checks: [check("ci-status", "FAILURE", { checkSuite: { app: { databaseId: 999 } } })] }), + pr(4, { checks: [lanes("FAILURE", "someone")] }), pr(5, { commits: { nodes: [] } }), // More checks than the page holds could hide an earlier fix-ci run. pr(6, { commits: { - nodes: [{ commit: { oid: "6".padStart(40, "a"), statusCheckRollup: { contexts: { nodes: [check("ci-status", "FAILURE")], pageInfo: { hasNextPage: true } } } } }], + nodes: [{ commit: { oid: "6".padStart(40, "a"), statusCheckRollup: { contexts: { nodes: [lanes("FAILURE")], pageInfo: { hasNextPage: true } } } } }], }, }), ], @@ -159,7 +162,7 @@ test("a head whose ci-status passed, is pending, is missing or came from another test("a head that already has a fix-ci run is skipped, unless every run failed within the restart count", async () => { const fix = (conclusion, extra) => check("pr-unblock / fix-ci", conclusion, extra); - const failed = check("ci-status", "FAILURE"); + const failed = lanes("FAILURE"); const { result } = await run({ pages: [ [ @@ -183,7 +186,7 @@ test("a head that already has a fix-ci run is skipped, unless every run failed w test("a restart count of 0 never dispatches a head again", async () => { const { result } = await run( - { pages: [[pr(4, { checks: [check("ci-status", "FAILURE"), check("pr-unblock / fix-ci", "FAILURE")] })]] }, + { pages: [[pr(4, { checks: [lanes("FAILURE"), check("pr-unblock / fix-ci", "FAILURE")] })]] }, { restarts: 0 }, ); assert.deepEqual(result.dispatched, []); @@ -367,7 +370,7 @@ test("the adapter sends one query naming the PR, its last commit's check rollup assert.equal(requests[0].url, "https://api.example/graphql"); assert.equal(requests[0].body.query, LIST_QUERY); assert.deepEqual(requests[0].body.variables, { owner: "octo", name: "repo", base: "main", after: null }); - for (const field of ["rateLimit", "cost", "statusCheckRollup", "commits(last: 1)", "databaseId", "... on User { databaseId }", "updatedAt", "committedDate", "body"]) { + for (const field of ["rateLimit", "cost", "statusCheckRollup", "commits(last: 1)", "databaseId", "... on User { databaseId }", "updatedAt", "committedDate", "body", "... on StatusContext { context state creator { login } }"]) { assert.ok(LIST_QUERY.includes(field), field); } }); @@ -455,7 +458,7 @@ test("a real failure at attempt 1 is the reconciler's to re-run once; at attempt // failing check, such as a macOS test_tunnel flake. test("a failed advisory pr-test-windows run with ci-status green is fix-ci's once its one re-run failed", async () => { const head = (number) => String(number).padStart(40, "a"); - const advisory = [check("ci-status", "SUCCESS"), check("test-macos-planning-shared", "FAILURE")]; + const advisory = [lanes("SUCCESS"), check("test-macos-planning-shared", "FAILURE")]; const requiredRuns = new Map([ [`${head(5)} ${ADVISORY}`, { run: tested(1), jobs: MIXED_JOBS }], [`${head(6)} ${ADVISORY}`, { run: tested(2), jobs: MIXED_JOBS }], @@ -468,13 +471,37 @@ test("a failed advisory pr-test-windows run with ci-status green is fix-ci's onc ); assert.deepEqual(api.calls.paths, [ADVISORY, ADVISORY, ADVISORY]); assert.ok(lines.includes("skip fix-ci #5: first failure, re-run job re-runs it once")); - assert.ok(lines.includes("skip fix-ci #7: ci-status passed and no pr-test-windows run failed")); + assert.ok(lines.includes("skip fix-ci #7: ci-lanes passed and no pr-test-windows run failed")); }); test("a green ci-status with only a failed fix-ci run, or no failure, is left alone", async () => { const { api, result } = await run({ - pages: [[pr(5, { checks: [check("ci-status", "SUCCESS"), check("pr-unblock / fix-ci", "FAILURE")] }), pr(6, { checks: [check("ci-status", "SUCCESS"), check("lint", "SUCCESS")] })]], + pages: [[pr(5, { checks: [lanes("SUCCESS"), check("pr-unblock / fix-ci", "FAILURE")] }), pr(6, { checks: [lanes("SUCCESS"), check("lint", "SUCCESS")] })]], + }); + assert.deepEqual(result.dispatched, []); + assert.deepEqual(api.calls.paths, []); +}); + +// ci-status also fails while a review lane is pending, missing or holding the +// PR, with no label on it; none of that is a CI fix. Only ci-lanes counts. +test("a red ci-status over green or absent ci-lanes summons no fix-ci", async () => { + const red = check("ci-status", "FAILURE"); + const { api, lines, result } = await run({ + pages: [[pr(1, { checks: [red, lanes("SUCCESS")] }), pr(2, { checks: [red] }), pr(3, { checks: [red, lanes("PENDING")] })]], }); assert.deepEqual(result.dispatched, []); assert.deepEqual(api.calls.paths, []); + assert.equal(skipReason(pr(1, { checks: [red, lanes("SUCCESS")] }), { base: "main", appSenderId: APP, actionsAppId: ACTIONS, checkName: "pr-unblock / fix-ci", holdLabels: [], restarts: 1, now: NOW }), "ci-lanes has not failed"); + assert.ok(!lines.some((line) => line.includes("fix-ci #")), lines.join("\n")); +}); + +test("a ci-lanes failure or error from github-actions[bot] is dispatched, whatever ci-status says", async () => { + const { api, result } = await run({ + pages: [[pr(1, { checks: [check("ci-status", "SUCCESS"), lanes("FAILURE")] }), pr(2, { checks: [lanes("ERROR")] }), pr(3, { checks: [lanes("FAILURE", "octocat")] })]], + }, { maxDispatch: 5 }); + assert.deepEqual( + result.dispatched.map((d) => d.number), + [1, 2], + ); + assert.deepEqual(api.calls.paths, [REQUIRED, REQUIRED]); }); diff --git a/.github/scripts/pr-unblock/workflow-gates.test.mjs b/.github/scripts/pr-unblock/workflow-gates.test.mjs index dcb41a065d..46fa99d43e 100644 --- a/.github/scripts/pr-unblock/workflow-gates.test.mjs +++ b/.github/scripts/pr-unblock/workflow-gates.test.mjs @@ -145,6 +145,38 @@ test("pr-feedback-dispatch.yml posts verifier notes only for a pr-verify run of } }); +// Every same-repository PR's completed review re-runs its head's ci-status, +// whatever the branch: ci-status needs both review lanes on the head. +test("pr-feedback-dispatch.yml re-runs ci-status for a completed review run of a same-repository PR only", () => { + const gate = jobIf("pr-feedback-dispatch.yml", "rerun-ci-status"); + const repo = "melodic-software/claude-code-plugins"; + const runs = (overrides = {}) => + evaluate(gate, { + "github.ref": "refs/heads/main", + "github.repository": repo, + "github.event.workflow_run.path": ".github/workflows/pr-review-hosted.yml", + "github.event.workflow_run.event": "pull_request", + "github.event.workflow_run.conclusion": "success", + "github.event.workflow_run.repository.full_name": repo, + "github.event.workflow_run.head_repository.full_name": repo, + ...overrides, + }); + assert.equal(runs(), true); + assert.equal(runs({ "github.event.workflow_run.path": ".github/workflows/pr-review-security-hosted.yml" }), true); + for (const conclusion of ["failure", "cancelled", "timed_out"]) { + assert.equal(runs({ "github.event.workflow_run.conclusion": conclusion }), true, conclusion); + } + // A draft's review run is skipped: it reviewed nothing. + assert.equal(runs({ "github.event.workflow_run.conclusion": "skipped" }), false); + assert.equal(runs({ "github.event.workflow_run.path": ".github/workflows/pr-verify.yml" }), false); + assert.equal(runs({ "github.event.workflow_run.event": "workflow_dispatch" }), false); + assert.equal(runs({ "github.event.workflow_run.event": "push" }), false); + assert.equal(runs({ "github.event.workflow_run.head_repository.full_name": "fork/repo" }), false); + assert.equal(runs({ "github.ref": "refs/heads/feat/x" }), false); + // Not gated on a lanes App branch: a person's PR needs its ci-status re-run too. + assert.doesNotMatch(gate, /head_branch/); +}); + const UNBLOCK = "pr-unblock.yml"; const JOBS = ["address-feedback-select", "address-feedback-not-run", "address-feedback", "address-feedback-subscription", "apply-feedback-patch", "publish-feedback"]; diff --git a/.github/workflows/pr-feedback-dispatch.yml b/.github/workflows/pr-feedback-dispatch.yml index 94ec865197..af16996899 100644 --- a/.github/workflows/pr-feedback-dispatch.yml +++ b/.github/workflows/pr-feedback-dispatch.yml @@ -24,6 +24,10 @@ name: pr-feedback-dispatch # that head as its upstream, so the gate and the broker check the same kind of # run either way. # +# A completed review run on any same-repository PR also starts the +# rerun-ci-status job, which re-runs the head's ci-status so it reads the +# finished review (see that job). +# # Every completed review run at a head starts this file, so a run leaves a # record for the next one at that head in the Actions cache # (dispatch-record.mjs): a head already handled for good is skipped before @@ -256,3 +260,102 @@ jobs: with: phase: end tokens: github-token ${{ github.token }} + + # ci-status's `Require both review lanes on this head` step reads the + # review runs when it runs, and a review usually finishes after it. So + # when either review caller completes on a pull request of this + # repository, this job re-runs the ci-status job of the newest + # pr-require-checks run on that head, which reads both lanes and the live + # labels again. Every PR's review starts it, not only a lanes App PR's. + # A run GitHub skipped (a draft) reviewed nothing, and a fork's review + # never runs, so neither starts it. A head the PR has moved past, or a + # closed PR, is left alone. A ci-status job not started yet reads the + # reviews when it starts, so it is left alone too; one in progress is + # polled every 60 s for at most 10 minutes, then re-run. No checkout, no + # model: the token can only read runs and the PR and re-run a job. + # Pinned by scripts/ci-rerun-ci-status.test.sh, which runs this body. + rerun-ci-status: + if: >- + github.ref == 'refs/heads/main' + && (github.event.workflow_run.path == '.github/workflows/pr-review-hosted.yml' + || github.event.workflow_run.path == '.github/workflows/pr-review-security-hosted.yml') + && github.event.workflow_run.event == 'pull_request' + && github.event.workflow_run.conclusion != 'skipped' + && github.event.workflow_run.repository.full_name == github.repository + && github.event.workflow_run.head_repository.full_name == github.repository + runs-on: ubuntu-24.04 + timeout-minutes: 15 + permissions: + actions: write # list pr-require-checks runs and jobs; re-run the ci-status job + pull-requests: read # the PR's state and live head + steps: + - name: Re-run ci-status on the reviewed head + env: + GH_TOKEN: ${{ github.token }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number }} + POLL_SECONDS: '60' + POLL_LIMIT_SECONDS: '600' + run: | + if [[ ! $HEAD_SHA =~ ^[0-9a-f]{40}$ ]]; then + echo "::error::the review run's head '$HEAD_SHA' is not a commit SHA." + exit 1 + fi + if [[ ! $PR_NUMBER =~ ^[1-9][0-9]{0,9}$ ]]; then + echo "::notice::the review run names no pull request; nothing to re-run." + exit 0 + fi + read_api() { + local attempt body + for attempt in 1 2 3; do + if body=$(gh api "$2"); then + printf '%s\n' "$body" + return 0 + fi + [ "$attempt" -lt 3 ] || { echo "::error::could not read $1; re-run this run." >&2; return 1; } + sleep $((attempt * 5)) + done + } + pr=$(read_api "#$PR_NUMBER" "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER") || exit 1 + if [ "$(jq -r '"\(.state) \(.head.sha)"' <<<"$pr")" != "open $HEAD_SHA" ]; then + echo "#$PR_NUMBER is closed or no longer at $HEAD_SHA; nothing to re-run." + exit 0 + fi + waited=0 + while :; do + runs=$(read_api "the pr-require-checks runs on $HEAD_SHA" \ + "repos/$GITHUB_REPOSITORY/actions/workflows/pr-require-checks.yml/runs?head_sha=$HEAD_SHA&event=pull_request&per_page=100") || exit 1 + run=$(jq -c --arg sha "$HEAD_SHA" '[.workflow_runs[] + | select(.path == ".github/workflows/pr-require-checks.yml" and .head_sha == $sha and .event == "pull_request")] + | max_by(.id) // empty' <<<"$runs") + if [ -z "$run" ]; then + echo "No pr-require-checks run on $HEAD_SHA yet; its ci-status reads the reviews when it runs." + exit 0 + fi + id=$(jq -r .id <<<"$run") + jobs=$(read_api "the jobs of run $id" "repos/$GITHUB_REPOSITORY/actions/runs/$id/jobs?filter=latest&per_page=100") || exit 1 + job=$(jq -c '[.jobs[] | select(.name == "ci-status")] | max_by(.id) // empty' <<<"$jobs") + if [ "$(jq -r .status <<<"$run")" = completed ]; then + break + fi + case $(jq -r '.status // "absent"' <<<"${job:-null}") in + in_progress | completed) ;; + *) + echo "ci-status of run $id has not started; it reads the reviews when it runs." + exit 0 + ;; + esac + if [ "$waited" -ge "$POLL_LIMIT_SECONDS" ]; then + echo "::error::run $id on $HEAD_SHA has not finished after ${waited}s; re-run its ci-status job once it does." + exit 1 + fi + sleep "$POLL_SECONDS" + waited=$((waited + POLL_SECONDS)) + done + job_id=$(jq -r '.id // empty' <<<"${job:-null}") + if [[ ! $job_id =~ ^[1-9][0-9]{0,14}$ ]]; then + echo "::error::run $id on $HEAD_SHA has no ci-status job to re-run." + exit 1 + fi + gh api -X POST "repos/$GITHUB_REPOSITORY/actions/jobs/$job_id/rerun" >/dev/null + echo "Re-ran ci-status (job $job_id) of run $id on $HEAD_SHA." diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index e295a0f306..0c0ee8ddc0 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3051,8 +3051,10 @@ jobs: # the worst case of the pinned composites' waits: two 60 s rate-limit waits # (`rate-limit-wait-seconds` on check-contract and aggregate-results), 45 s # of own-gate retries, 7 s of status-write retries and the 90 s - # `rerun-wait-seconds` wait, about four and a half minutes. - timeout-minutes: 5 + # `rerun-wait-seconds` wait, about four and a half minutes, plus up to + # 15 s of retry sleeps for each of the hold-lift step's three reads and + # the review-lane step's four, under two minutes. + timeout-minutes: 7 permissions: # `yield-to-full-run` lists this workflow's runs on the head SHA, and # `rerun-contract-only-siblings` re-runs the red contract-only ones once @@ -3200,9 +3202,12 @@ jobs: # outdates. The reason is the newest review-lane hold comment's text: a # `` +comment when the review returns a hold, or when a changed file (or a renamed file's old path) +matches a hold path, which holds as `trust-gate` whatever the review says: + +- `.github/standards/trusted-actors/**` +- `.github/actions/check-trusted-trigger/**` +- `**/CODEOWNERS` +- `**/.gitmodules` + +No ruleset requires a review check (github-iac ADR 0017); ci-status, the one required check, enforces +them on every `pull_request` run, contract-only runs included: + +- **The label holds.** ci-status fails while the PR carries the human-gated label (`needs-human` + unless the base's `.work-item-tracker.json` names another). Removing it approves. +- **Only a trusted person lifts a review-lane hold.** A hold is the newest hold comment or the + newest `labeled` event of the label by `github-actions[bot]` or `claude[bot]`, whichever is later, + so deleting the comment does not lift it. The first removal of the label after it must be by a `human` of the base's + trusted-actor list; a later removal is of a later label, such as an escalation + `clear-escalations` lifts. A bot's or anyone else's lift, or none recorded, fails. To recover, a + trusted person adds the label back, which starts a new hold, and a trusted person removes it. +- **Both lanes review the head.** On a ready PR, the newest `pull_request` run of each caller on the + head must have a successful status job (`pr-review / claude-review-status`, + `pr-review-security / security-review`) whose `Apply the hold` step ran. A summary-mode run or + one with no hold label does not count. A pending, failed, cancelled or skipped review fails with + the cause named. A lane the kill switch stopped passes (no merge freeze), but a kill-switch job + that failed does not. A review skips a fork and a bot outside the caller's + `allowed-bot-sender-ids`, so their PRs stay red until a person on the trusted-actor list pushes a + commit to the head. Dependabot, the release App and the standards-sync App are exempt by numeric + author id, except that their PR fails when a changed file (or a renamed file's old path) matches + a hold path, since no lane reviews it to hold it; a person merges it. A draft is exempt. + +A review usually finishes after ci-status has read it, so `pr-feedback-dispatch.yml`'s +`rerun-ci-status` job re-runs the ci-status job of the newest `pr-require-checks` run on the head +whenever either review caller completes (not when GitHub skipped it) on an open same-repository PR +still at that head. A ci-status not started yet is left to read the reviews itself; one in progress +is polled every 60 seconds for at most 10 minutes first. The merge-group run re-checks the labels. +`fix-ci` keys on `ci-lanes`, so a hold or a pending review never summons it. + ## Trust-root paths A lane's token grant and the rules it is judged by come from these default-branch paths, so a @@ -1840,7 +1885,7 @@ check, the security-review lane's findings and every other gate still apply. Dep its own tier in pr-merge ([Dependabot pull requests](#dependabot-pull-requests)), and is never dispatched `fix-ci`. -An adopted PR whose `ci-status` failed is dispatched `fix-ci` like a lanes App PR, under the same +An adopted PR whose lanes failed (`ci-lanes`) is dispatched `fix-ci` like a lanes App PR, under the same `limits.fix-ci-per-run`, `limits.fix-ci-restarts` and `loop.fix-ci-rounds`. The run still needs the author on the trusted-actor list, as every pr-unblock run does. A pushed fix commit updates the PR and its head, so the next fix waits another `idle-hours`: a person's PR gets at most one fix diff --git a/scripts/ci-hold-human-gated.test.sh b/scripts/ci-hold-human-gated.test.sh index 9e861110ab..acac62c00d 100755 --- a/scripts/ci-hold-human-gated.test.sh +++ b/scripts/ci-hold-human-gated.test.sh @@ -46,6 +46,11 @@ if [[ "$step" == *"continue-on-error"* ]]; then else ok "step fails the job on its exit code" fi +if [[ "$step" == *$'\n id: human_gated\n'* ]]; then + ok "step carries the id the hold-lift step reads its outputs by" +else + fail "step lost its human_gated id" +fi if [[ "$step" == *"BASE_SHA: \${{ github.event.pull_request.base.sha }}"* ]]; then ok "step reads the tracker config at the base commit, never the head" else @@ -112,8 +117,10 @@ OTHER_BOT="$(hold "$LANE" $'A comment with no hold marker.\nclaude-lane: claude- expect() { local label="$1" want_rc="$2" want_calls="$3" out rc calls : >"$TMP_ROOT/log" + : >"$TMP_ROOT/output" out="$(PATH="$TMP_ROOT/bin:$PATH" STUB_LOG="$TMP_ROOT/log" STUB_LABELS="$4" STUB_FAILS="$5" PR_NUMBER=6419 \ - GITHUB_REPOSITORY=o/r RUNNER_TEMP="$TMP_ROOT" BASE_SHA="${BASE_SHA-$BASE}" bash -e "$TMP_ROOT/body.sh" 2>&1)" && rc=0 || rc=$? + GITHUB_REPOSITORY=o/r RUNNER_TEMP="$TMP_ROOT" GITHUB_OUTPUT="$TMP_ROOT/output" BASE_SHA="${BASE_SHA-$BASE}" \ + bash -e "$TMP_ROOT/body.sh" 2>&1)" && rc=0 || rc=$? calls=$(grep -c '/labels' "$TMP_ROOT/log") if [[ "$rc" -ne "$want_rc" ]]; then fail "$label: expected rc=$want_rc got rc=$rc out='$out'" @@ -136,10 +143,20 @@ APPROVE="Removing the label approves this pull request; ci-status re-runs on the expect "an unlabeled pull request passes" 0 1 "enhancement,do-not-merge" 0 "#6419 carries no 'needs-human' label." if grep -q '/comments' "$TMP_ROOT/log"; then fail "comments are read only when the label is on"; else ok "comments are read only when the label is on"; fi +if [[ "$(cat "$TMP_ROOT/output")" == $'label=needs-human\nheld=false' ]]; then + ok "an unlabeled pull request outputs the label and held=false" +else + fail "an unlabeled pull request's outputs: '$(cat "$TMP_ROOT/output")'" +fi expect "a pull request with no labels passes" 0 1 "" 0 expect "a label that only contains the name does not hold" 0 1 "needs-human-later,not-needs-human" 0 expect "needs-human with no hold comment fails without a reason" 1 1 "ci,needs-human" 0 \ "::error::#6419 carries 'needs-human'. $APPROVE" +if [[ "$(cat "$TMP_ROOT/output")" == "label=needs-human" ]]; then + ok "a held pull request outputs the label and no held=false" +else + fail "a held pull request's outputs: '$(cat "$TMP_ROOT/output")'" +fi export STUB_COMMENTS="[$OLD,$NEW,$SPOOF,$NO_FOOTER,$OTHER_BOT]" expect "the newest lane hold comment gives the reason, without its footer" 1 1 "needs-human" 0 \ @@ -158,9 +175,19 @@ unset STUB_COMMENTS STUB_COMMENTS_FAIL expect "a transient label read failure is retried" 1 2 "needs-human" 1 expect "an unreadable label list fails closed" 1 3 "" 3 "::error::could not read the labels of #6419" +if grep -q '^held=' "$TMP_ROOT/output"; then + fail "an unreadable label list output held" +else + ok "an unreadable label list outputs no held" +fi export STUB_TRACKER="$CUSTOM" expect "the base tracker's human-gated label holds" 1 1 "owner-call" 0 "::error::#6419 carries 'owner-call'. $APPROVE" +if grep -qxF 'label=owner-call' "$TMP_ROOT/output"; then + ok "the output names the base tracker's label" +else + fail "the output does not name the base tracker's label: '$(cat "$TMP_ROOT/output")'" +fi expect "needs-human does not hold when the tracker names another label" 0 1 "needs-human" 0 "#6419 carries no 'owner-call' label." export STUB_TRACKER='{not json' expect "an invalid tracker config fails closed" 1 0 "" 0 "::error::cannot read the human-gated role label" diff --git a/scripts/ci-hold-lifted-by.test.sh b/scripts/ci-hold-lifted-by.test.sh new file mode 100755 index 0000000000..2966b441e7 --- /dev/null +++ b/scripts/ci-hold-lifted-by.test.sh @@ -0,0 +1,199 @@ +#!/usr/bin/env bash +# Runs the `Fail a review-lane hold an untrusted account lifted` step body of +# .github/workflows/pr-require-checks.yml's ci-status under the shell Actions +# gives a step with no `shell:` (bash -e), against stub `gh` and `sleep`. +# +# Once a review lane has held a pull request, ci-status passes with the +# human-gated label off only when the first removal of that label after the +# newest hold comment was made by a `human` of the base commit's trusted-actor +# list. It fails closed when it cannot tell. +# test-scope: .github/workflows/pr-require-checks.yml +set -uo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +WORKFLOW="$ROOT/.github/workflows/pr-require-checks.yml" + +# shellcheck source=lib/test-harness.sh +. "$ROOT/scripts/lib/test-harness.sh" + +TMP_ROOT="$(mktemp -d)" +trap 'rm -rf "$TMP_ROOT"' EXIT + +NAME='Fail a review-lane hold an untrusted account lifted' +step="$(awk -v name=" - name: $NAME" '$0 == name { on = 1; print; next } + on && /^ - / { exit } + on { print }' "$WORKFLOW")" +awk '/^ run: \|$/ { on = 1; next } + on && /^ / { print substr($0, 11); next } + on && /^[[:blank:]]*$/ { print ""; next } + on { exit }' <<<"$step" >"$TMP_ROOT/body.sh" + +if [[ ! -s "$TMP_ROOT/body.sh" ]]; then + fail "no '$NAME' step with a run: | body in $WORKFLOW" + test_harness::report + exit 1 +fi +if [[ "$step" == *"if: \${{ !cancelled() && github.event_name == 'pull_request' && steps.human_gated.outputs.held == 'false' }}"* ]]; then + ok "step runs only once the human-gated step read the label off" +else + fail "step lost its condition on the human-gated step's held output" +fi +if [[ "$step" == *"continue-on-error"* ]]; then + fail "step must not continue on error: its exit code is the hold" +else + ok "step fails the job on its exit code" +fi +# shellcheck disable=SC2016 # the ${{ }} text is workflow source, not shell +for env in 'BASE_SHA: ${{ github.event.pull_request.base.sha }}' 'HUMAN_GATED: ${{ steps.human_gated.outputs.label }}'; do + if [[ "$step" == *"$env"* ]]; then ok "step env carries $env"; else fail "step env lacks $env"; fi +done +order="$(grep -nE "^ ci-status:$|^ - name: (Fail a pull request carrying the human-gated label|$NAME|Aggregate lane results)$" "$WORKFLOW" | cut -d: -f1 | paste -sd' ')" +read -r job_line gated_line lift_line agg_line <<<"$order" +if [[ -n "${agg_line:-}" && "$job_line" -lt "$gated_line" && "$gated_line" -lt "$lift_line" && "$lift_line" -lt "$agg_line" ]]; then + ok "step sits in ci-status after the human-gated step, before the aggregate" +else + fail "step is not inside ci-status between the human-gated step and the aggregate (lines: $order)" +fi + +# The stub logs its arguments. A comments read fails when STUB_COMMENTS_FAIL is +# set, else applies the --jq filter to the STUB_COMMENTS array, as gh does per +# page. A contents read of the trusted-actor list fails its first +# STUB_ACTORS_FAILS calls, then prints STUB_ACTORS. An events read fails when +# STUB_EVENTS_FAIL is set, else prints STUB_EVENTS (pages concatenated, as +# --paginate prints them). +mkdir -p "$TMP_ROOT/bin" +cat >"$TMP_ROOT/bin/gh" <<'EOF' +#!/usr/bin/env bash +printf '%s\n' "$*" >>"$STUB_LOG" +case "$*" in +*/comments*) + [[ -z "${STUB_COMMENTS_FAIL:-}" ]] || exit 1 + while [[ $# -gt 0 && "$1" != --jq ]]; do shift; done + jq -r "$2" <<<"${STUB_COMMENTS:-[]}" + ;; +*/contents/.github/standards/trusted-actors/trusted-actors.json*) + [[ "$(grep -c '/contents/' "$STUB_LOG")" -gt "${STUB_ACTORS_FAILS:-0}" ]] || { echo 'gh: Server Error (HTTP 502)' >&2; exit 1; } + printf '%s\n' "$STUB_ACTORS" + ;; +*/events*) + [[ -z "${STUB_EVENTS_FAIL:-}" ]] || exit 1 + printf '%s\n' "${STUB_EVENTS:-[]}" + ;; +*) exit 1 ;; +esac +EOF +printf '#!/usr/bin/env bash\n' >"$TMP_ROOT/bin/sleep" +chmod +x "$TMP_ROOT/bin/gh" "$TMP_ROOT/bin/sleep" + +BASE=89abcdef0123456789abcdef0123456789abcdef +ACTORS='{"version":1,"actors":[{"id":153232337,"login":"kyle-sexton","kind":"human"},{"id":337595936,"login":"melodic-automation-lanes[bot]","kind":"bot"}]}' +LANE='{"login":"github-actions[bot]","type":"Bot"}' +comment() { jq -cn --arg body "$2" --arg at "$3" --argjson user "$1" '{user: $user, body: $body, created_at: $at}'; } +HOLD="$(comment "$LANE" $'\nHeld.\n\nclaude-lane: claude-review' 2026-10-23T10:00:00Z)" +OLD_HOLD="$(comment "$LANE" $'\nHeld.\nclaude-lane: claude-security-review' 2026-10-22T10:00:00Z)" +SPOOF="$(comment '{"login":"someone","type":"User"}' $'\nclaude-lane: claude-review' 2026-10-23T12:00:00Z)" +NO_FOOTER="$(comment "$LANE" $'\nNo footer.' 2026-10-23T12:00:00Z)" +# unlabeled