diff --git a/.changes/pr-pipeline/fix-pr-unblock-stop-status-pushed-head-5c3e9a17.md b/.changes/pr-pipeline/fix-pr-unblock-stop-status-pushed-head-5c3e9a17.md new file mode 100644 index 00000000000..e8652319ad6 --- /dev/null +++ b/.changes/pr-pipeline/fix-pr-unblock-stop-status-pushed-head-5c3e9a17.md @@ -0,0 +1,12 @@ +--- +bump: patch +--- + +### Fixed + +- pr-unblock's `report-stop-status` posts the stop status on the commit the activity's push job + pushed (`apply-conflict-patch`, `apply-feedback-patch`, `apply-dependency-patch`), and on the + select job's head only when that job pushed none. A reply or verdict job that stopped after the + push, such as `publish-feedback` with `feedback-unadjudicated`, put the failing status on the + head before it, so the pull request's head carried none, ci-status passed and pr-merge armed + auto-merge over the stop. diff --git a/.github/actions/resolve-config/callers.test.mjs b/.github/actions/resolve-config/callers.test.mjs index f0826af6d80..72df3f1224a 100644 --- a/.github/actions/resolve-config/callers.test.mjs +++ b/.github/actions/resolve-config/callers.test.mjs @@ -698,13 +698,16 @@ test("pr-unblock.yml: report-stop-status runs when report-escalation does, or af const HEADS = ["resolve-conflicts-select", "address-feedback-select", "review-dependency-select", "fix-ci-context", "sync-shared-copies-select"]; const EMITTERS = ["apply-conflict-patch", "publish-feedback", "apply-dependency-verdict", "apply-patch", "apply-shared-copies"]; const RESOLVERS = ["address-feedback-select", "fix-ci-context"]; - const context = ({ from = "", reason = "", headFrom = "resolve-conflicts-select", sha = head, rungUp = "", rungUpResult = "skipped", rungUpReason = "", rungUpSkip = "", landed = "", skip = "", resolved = "", resolvedResult = "success" } = {}) => ({ + // The push job of each activity that pushes, before its select job's head. + const PUSHERS = { "resolve-conflicts-select": "apply-conflict-patch", "address-feedback-select": "apply-feedback-patch", "review-dependency-select": "apply-dependency-patch" }; + const context = ({ from = "", reason = "", headFrom = "resolve-conflicts-select", sha = head, pushed = "", pushedSha = "", rungUp = "", rungUpResult = "skipped", rungUpReason = "", rungUpSkip = "", landed = "", skip = "", resolved = "", resolvedResult = "success" } = {}) => ({ ...Object.fromEntries(SOURCES.map((id) => [`needs.${id}.outputs.escalation-reason`, id === from ? reason : ""])), ...Object.fromEntries(RESOLVERS.map((id) => [`needs.${id}.result`, id === resolved ? resolvedResult : "skipped"])), ...Object.fromEntries(RESOLVERS.map((id) => [`needs.${id}.outputs.stop-resolved`, id === resolved ? "true" : ""])), ...Object.fromEntries(EMITTERS.map((id) => [`needs.${id}.result`, id === landed ? "success" : "skipped"])), ...Object.fromEntries([...EMITTERS, "apply-patch-rung-up"].map((id) => [`needs.${id}.outputs.skip-reason`, id === landed ? skip : ""])), ...Object.fromEntries(HEADS.map((id) => [`needs.${id}.outputs.head-sha`, id === headFrom ? sha : ""])), + ...Object.fromEntries(Object.values(PUSHERS).map((id) => [`needs.${id}.outputs.pushed-commit`, id === pushed ? pushedSha : ""])), "needs.apply-patch.outputs.rung-up": rungUp, "needs.apply-patch-rung-up.result": rungUpResult, "needs.apply-patch-rung-up.outputs.escalation-reason": rungUpReason, @@ -776,10 +779,18 @@ test("pr-unblock.yml: report-stop-status runs when report-escalation does, or af assert.equal(evaluate(escalation.if, failed()), true); assert.equal(evaluate(escalation.with["escalation-reason"], failed()), "feedback-publish-failed"); assert.equal(stopSkip("feedback-publish-failed"), "lane-stopped", "routed to a failing status"); - // The head is a select job's, never a model call's; the reason is the one - // report-escalation reads. + // The head is the commit the activity's push job pushed, else its select + // job's, never a model call's; the reason is the one report-escalation reads. const post = job.steps.find((step) => step.name === "Post the stop status"); - assert.equal(post.env.HEAD_SHA.trim(), HEADS.map((id) => `needs.${id}.outputs.head-sha`).join(" || ").replace(/^/, "${{ ").replace(/$/, " }}")); + const heads = HEADS.flatMap((id) => (PUSHERS[id] ? [`needs.${PUSHERS[id]}.outputs.pushed-commit`, `needs.${id}.outputs.head-sha`] : [`needs.${id}.outputs.head-sha`])); + assert.equal(post.env.HEAD_SHA.trim(), `\${{ ${heads.join(" || ")} }}`); + const pushedSha = "d".repeat(40); + for (const headFrom of HEADS) { + assert.equal(evaluate(post.env.HEAD_SHA, context({ headFrom })), head, `${headFrom}'s head with no push`); + if (PUSHERS[headFrom]) { + assert.equal(evaluate(post.env.HEAD_SHA, context({ headFrom, pushed: PUSHERS[headFrom], pushedSha })), pushedSha, `${PUSHERS[headFrom]}'s pushed commit over ${headFrom}'s head`); + } + } // report-escalation's reason, except that a rung-up retry whose patch // applied is no stop. const reason = (extra) => evaluate(post.env.ESCALATION_REASON, context(extra)); @@ -2503,9 +2514,10 @@ const PATCH_LANES = [ { "needs.address-feedback-select.outputs.proceed": "true" }, "apply-feedback-patch", ], - // apply-dependency-patch stays out of report-escalation too: a refused - // patch leaves no fix commit, and the verdict job escalates - // dependency-fix-not-landed (apply-dependency-verdict.test.mjs). + // report-escalation takes nothing from apply-dependency-patch either: a + // refused patch leaves no fix commit, and the verdict job escalates + // dependency-fix-not-landed (apply-dependency-verdict.test.mjs). It needs + // the job only to match report-stop-status's needs (NEEDS_ONLY). [ "pr-unblock.yml", "review-dependency", @@ -2564,6 +2576,9 @@ const APPLY_GATE = { // resolve-conflicts's empty patch leaves the conflicts unresolved. // The push jobs whose pushed-commit report-escalation passes its gate. const PUSHED_COMMIT_READERS = new Set(["apply-feedback-patch"]); +// The push jobs report-escalation needs, reading none of their outputs, so its +// needs equal report-stop-status's, which posts on their pushed commit. +const NEEDS_ONLY = new Set(["apply-dependency-patch"]); const EMPTY_RUNS_APPLY = { "fix-ci": true, "fix-docs": false, "address-feedback": false, "review-dependency": false, "resolve-conflicts": true }; const PATCH_OUTPUTS = { "patch-state": "patch-state", "patch-sha256": "patch-sha256", "patch-attempt": "patch-attempt", "reply-attempt": "reply-attempt" }; // The read runner output each push job reads per input: the scan job's alone, @@ -2744,7 +2759,11 @@ for (const [file, firstId, subId, extraNeeds, extraProceed, applyId] of PATCH_LA assert.deepEqual([...new Set(reads)], ["outputs.pushed-commit"], `report-escalation reads only ${id}'s pushed-commit`); continue; } - assert.ok(!escalation.needs.includes(id), `report-escalation does not need ${id}`); + if (id === applyId && NEEDS_ONLY.has(applyId)) { + assert.ok(escalation.needs.includes(id), `report-escalation needs ${id}, as report-stop-status does`); + } else { + assert.ok(!escalation.needs.includes(id), `report-escalation does not need ${id}`); + } assert.deepEqual(reads, [], `report-escalation reads no ${id} output`); } } diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index b7373ce89dc..1d509f0256e 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -8,8 +8,9 @@ // A pull request is armed only when all of these hold: // - The lanes App opened it (merge.agent-identity), into main, from a branch // of this repository, and it is not a draft. -// - It carries no hold: no `do-not-merge` and no human-gated label (the -// tracker's role label, `needs-human` by default), and no body hold: the +// - It carries no hold: no HOLD_LABELS label (`do-not-merge`, +// `pr-verify-failed`, `security: outside-actor`) and no human-gated label +// (the tracker's role label, `needs-human` by default), and no body hold: the // lane's own hold line (merge-holds.mjs LANE_HOLD), or hold text a person // wrote (BODY_HOLD and personWrote). Labels past the listing's 100 are // read whole. diff --git a/.github/scripts/pr-unblock/workflow-gates.test.mjs b/.github/scripts/pr-unblock/workflow-gates.test.mjs index 43bcd22f5cc..ed008767000 100644 --- a/.github/scripts/pr-unblock/workflow-gates.test.mjs +++ b/.github/scripts/pr-unblock/workflow-gates.test.mjs @@ -746,6 +746,44 @@ test("pr-unblock.yml: apply-conflict-patch runs on a clean merge or a conflicted assert.equal(runs("conflicted", "true", "skipped", "", "garbage"), false); }); +// A reply or verdict job can stop after its activity's push job moved the +// head; a failing status on the head before the push leaves the PR's head +// with none, so ci-status passes and pr-merge arms over the stop. +test("pr-unblock.yml: report-stop-status posts on the commit the activity's push job pushed, else its select head", () => { + const step = stepBlock(UNBLOCK, "report-stop-status", "Post the stop status"); + const at = step.indexOf(" HEAD_SHA: >-"); + assert.notEqual(at, -1, "the step folds HEAD_SHA"); + const end = step.findIndex((line, i) => i > at && !line.startsWith(" ")); + const expression = step + .slice(at + 1, end) + .map((line) => line.trim()) + .join(" "); + const activities = [ + ["apply-conflict-patch", "resolve-conflicts-select"], + ["apply-feedback-patch", "address-feedback-select"], + ["apply-dependency-patch", "review-dependency-select"], + [null, "fix-ci-context"], + [null, "sync-shared-copies-select"], + ]; + const block = jobBlock(UNBLOCK, "report-stop-status"); + const needs = block.slice(block.indexOf(" needs:") + 1, block.findIndex((line) => /^ {4}if: /.test(line))).map((line) => line.replace(/^ {6}- /, "")); + const blank = {}; + for (const [push, select] of activities) { + blank[`needs.${select}.outputs.head-sha`] = ""; + if (push) { + blank[`needs.${push}.outputs.pushed-commit`] = ""; + assert.ok(needs.includes(push), `report-stop-status needs ${push}`); + } + } + for (const [push, select] of activities) { + const selected = { ...blank, [`needs.${select}.outputs.head-sha`]: "before" }; + assert.equal(evaluate(expression, selected), "before", `${select} with no push`); + if (push) { + assert.equal(evaluate(expression, { ...selected, [`needs.${push}.outputs.pushed-commit`]: "pushed" }), "pushed", `${push} over ${select}`); + } + } +}); + test("pr-unblock.yml: the resolve-conflicts model calls run only when the select job found a conflicted merge", () => { const { gate, needs } = jobGate(UNBLOCK, "resolve-conflicts"); assert.deepEqual(needs, ["resolve-conflicts-select"]); diff --git a/.github/workflows/pr-unblock.yml b/.github/workflows/pr-unblock.yml index 726fe94a371..af478e8dbd5 100644 --- a/.github/workflows/pr-unblock.yml +++ b/.github/workflows/pr-unblock.yml @@ -3422,6 +3422,7 @@ jobs: - apply-feedback-patch - publish-feedback - review-dependency-select + - apply-dependency-patch - apply-dependency-verdict - fix-ci-context - apply-patch @@ -3475,15 +3476,19 @@ jobs: # One activity runs per dispatch, so one select job's head is set; a # reason is a fixed enum value from a base script or this run's jobs, # which report-escalation.sh rejects unless the routing table lists it. - # No reason: the attempt did not stop, and clear runs instead. + # No reason: the attempt did not stop, and clear runs instead. A commit + # the activity's push job pushed comes before its select head: the + # reply and verdict jobs can stop after that push, and a status on the + # head before it leaves the PR's head with no failing status. - name: Post the stop status env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ inputs.pr-number }} HEAD_SHA: >- - ${{ needs.resolve-conflicts-select.outputs.head-sha || needs.address-feedback-select.outputs.head-sha - || needs.review-dependency-select.outputs.head-sha || needs.fix-ci-context.outputs.head-sha - || needs.sync-shared-copies-select.outputs.head-sha }} + ${{ needs.apply-conflict-patch.outputs.pushed-commit || needs.resolve-conflicts-select.outputs.head-sha + || needs.apply-feedback-patch.outputs.pushed-commit || needs.address-feedback-select.outputs.head-sha + || needs.apply-dependency-patch.outputs.pushed-commit || needs.review-dependency-select.outputs.head-sha + || needs.fix-ci-context.outputs.head-sha || needs.sync-shared-copies-select.outputs.head-sha }} ESCALATION_REASON: >- ${{ needs.resolve-conflicts-select.outputs.escalation-reason || needs.apply-conflict-patch.outputs.escalation-reason @@ -3522,6 +3527,7 @@ jobs: - apply-feedback-patch - publish-feedback - review-dependency-select + - apply-dependency-patch - apply-dependency-verdict - fix-ci-context - apply-patch