From 5e52983db0198352bef883d9fc1e3130786d64f3 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:54:10 -0400 Subject: [PATCH 1/7] fix(ci): end a kill-switch deny neutral, not failed The lane breaker turns CLAUDE_LANES_REPO_DISABLED on after a queued run has read its variables, so the run's own kill-switch check passes and only the token broker's live read stops it, failing the run red. 12 runs since 2026-10-24 (pr-merge 4, intake-triage 5, work-resolve-issue 3) ended that way. request-lane-token takes a kill-switch-ok input (token mode only): a 403 kill-switch ends green with a notice, no token and skip-reason kill-switch. pr-merge's merge and enable-auto-merge jobs and intake-triage's apply-verdict and apply-refinement jobs opt in; every step there that uses the token already runs only when it is set, and refine-select now skips on apply-verdict's new stopped output. work-resolve-issue stays red: work-unblock re-fires its issue from the failed mint step. Co-Authored-By: Claude Opus 5.5 --- ...-kill-switch-deny-ends-neutral-3b7e91c4.md | 16 +++++ .github/actions/request-lane-token/README.md | 6 +- .github/actions/request-lane-token/action.yml | 31 +++++++++- .../request-lane-token.test.mjs | 62 +++++++++++++++++++ .github/workflows/intake-triage.yml | 10 ++- .github/workflows/pr-merge.yml | 6 +- 6 files changed, 125 insertions(+), 6 deletions(-) create mode 100644 .changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md diff --git a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md new file mode 100644 index 00000000000..d56d89dd1b6 --- /dev/null +++ b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md @@ -0,0 +1,16 @@ +--- +bump: patch +--- + +### Fixed + +- A pr-merge or intake-triage run that the token broker refuses with reason `kill-switch` no longer + ends red. The lane breaker turns the repository kill switch on after a queued run has read its + variables, so the run's own kill-switch check passes and only the broker's live read stops it. + `request-lane-token` takes a new `kill-switch-ok` input (`token` mode only): on that 403 it ends + green with a `lane-token-stopped` notice, no token and `skip-reason` `kill-switch`. Every step in + pr-merge's `merge` and `enable-auto-merge` jobs and intake-triage's `apply-verdict` and + `apply-refinement` jobs that uses the token already runs only when the token is set, and + intake-triage's `refine-select` now skips on apply-verdict's new `stopped` output. Any other + denial, and every caller without the input, stays red; work-resolve-issue stays red because + work-unblock re-fires its issue from the failed mint step. `version` stays 2. diff --git a/.github/actions/request-lane-token/README.md b/.github/actions/request-lane-token/README.md index fd8055184cd..46e4295c724 100644 --- a/.github/actions/request-lane-token/README.md +++ b/.github/actions/request-lane-token/README.md @@ -24,6 +24,7 @@ run, and mints nothing ([Check mode](#check-mode)). | `dispatcher-run-id` | `token` mode: the `pr-unblock-dispatch.yml` run that dispatched this one; sent as `dispatcher_run_id` when set. The broker needs it to trust a run sent by `github-actions[bot]` | | `effect`, `contents`, `pull-requests`, `issues`, `actions` | `token` mode: this job's own `resolve-config` outputs, compared with the broker's answer. `actions` is `read` or empty; empty expects no `actions` permission | | `superseded-ok` | `true` ends green, with a `lane-token-superseded` warning and no token, on a 403 whose reason is `workflow-modified` (a newer copy of the caller workflow on the default branch, whose run does the work). Default `false` | +| `kill-switch-ok` | `token` mode: `true` ends green, with a `lane-token-stopped` notice, no token and `skip-reason` `kill-switch`, on a 403 whose reason is `kill-switch` (a kill switch turned on after the run read its variables, a designed stop). Steps that use the token must gate on `token`, and jobs that need this one on `skip-reason`. Default `false` | | `pr-workflow-change-ok` | `token` mode with `pr-number`: `true` ends green, with no token and `skip-reason` `workflow-modified`, on a 403 `workflow-modified` naming a workflow file the PR itself changes ([below](#a-workflow-file-the-pr-changes)). Default `false` | | `head-sha`, `default-branch`, `github-token` | Required with `pr-workflow-change-ok`: the gated head, the default branch, and a token that reads the repository's contents, for the compare | @@ -35,7 +36,7 @@ run, and mints nothing ([Check mode](#check-mode)). | `expires-at` | The expiry the broker returned | | `mint-id` | `-`, the broker's audit key for this mint | | `failure-reason` | The one-line reason the step failed with (`lane-token-denied: ...`); empty on a success | -| `skip-reason` | `workflow-modified` when `pr-workflow-change-ok` skipped; empty otherwise | +| `skip-reason` | `workflow-modified` when `pr-workflow-change-ok` skipped, `kill-switch` when `kill-switch-ok` did; empty otherwise | ## What it does @@ -66,7 +67,8 @@ run, and mints nothing ([Check mode](#check-mode)). With `superseded-ok: true`, a 403 with reason `workflow-modified` warns and ends green instead, setting no output, so steps gated on the `token` output skip. With `pr-workflow-change-ok: true`, such a 403 for a workflow file the PR changes ends green with `skip-reason` set - ([below](#a-workflow-file-the-pr-changes)). + ([below](#a-workflow-file-the-pr-changes)). With `kill-switch-ok: true`, a 403 with reason + `kill-switch` ends green with a notice and `skip-reason` `kill-switch`, setting no token. 5. On 200, masks the token, then compares the broker's `effect`, `permissions`, `lane`, `activity` and `repository_ids` with this job's values (`pull-requests` is `pull_requests` in the API). Any difference revokes the token and fails red with `effect-mismatch`. That happens when the PR's diff --git a/.github/actions/request-lane-token/action.yml b/.github/actions/request-lane-token/action.yml index c3d35d45b2d..5f88249a727 100644 --- a/.github/actions/request-lane-token/action.yml +++ b/.github/actions/request-lane-token/action.yml @@ -7,7 +7,8 @@ description: > revoking the token, unless the broker's effect, permissions, lane, activity and repository equal what this job resolved; with `pr-workflow-change-ok` a `workflow-modified` refusal of a workflow file the PR changes skips instead - of failing. In `check` mode it asks the + of failing, and with `kill-switch-ok` a `kill-switch` refusal skips with a + notice. In `check` mode it asks the broker's check endpoint, which mints nothing, whether a read lane may run, and fails red on anything but an allow. Contract: docs/conventions/pr-pipeline/pr-run-activity.md, run job step 6 and the @@ -88,6 +89,17 @@ inputs: work. Any other denial still fails red. Default `false`. required: false default: "false" + kill-switch-ok: + description: >- + `true` ends the step green, with a notice, no token and the + `skip-reason` output `kill-switch`, when the broker answers 403 with + reason `kill-switch`: a kill switch turned on after this run read its + variables, a designed stop. Every step that uses the token must run + only when the `token` output is set, and every job that depends on this + one must skip on `skip-reason`. Any other denial still fails red. + `token` mode only. Default `false`. + required: false + default: "false" pr-workflow-change-ok: description: >- `true` ends the step green, with no token and the `skip-reason` output @@ -129,7 +141,8 @@ outputs: value: ${{ steps.request.outputs.failure-reason }} skip-reason: description: >- - `workflow-modified` when `pr-workflow-change-ok` skipped the activity; empty otherwise. + `workflow-modified` when `pr-workflow-change-ok` skipped the activity, + `kill-switch` when `kill-switch-ok` did; empty otherwise. value: ${{ steps.request.outputs.skip-reason }} runs: @@ -154,6 +167,7 @@ runs: ISSUES: ${{ inputs.issues }} ACTIONS: ${{ inputs.actions }} SUPERSEDED_OK: ${{ inputs.superseded-ok }} + KILL_SWITCH_OK: ${{ inputs.kill-switch-ok }} PR_WORKFLOW_CHANGE_OK: ${{ inputs.pr-workflow-change-ok }} HEAD_SHA: ${{ inputs.head-sha }} DEFAULT_BRANCH: ${{ inputs.default-branch }} @@ -180,6 +194,10 @@ runs: [[ $LANE =~ $name && $ACTIVITY =~ $name ]] || fail "the lane and activity must be kebab-case names" SUPERSEDED_OK=${SUPERSEDED_OK:-false} [[ $SUPERSEDED_OK == true || $SUPERSEDED_OK == false ]] || fail "superseded-ok must be true or false" + KILL_SWITCH_OK=${KILL_SWITCH_OK:-false} + [[ $KILL_SWITCH_OK == true || $KILL_SWITCH_OK == false ]] || fail "kill-switch-ok must be true or false" + # A check allow is the exit code itself, so a check may never end green on a refusal. + [[ $KILL_SWITCH_OK == false || $MODE == token ]] || fail "kill-switch-ok needs token mode" PR_WORKFLOW_CHANGE_OK=${PR_WORKFLOW_CHANGE_OK:-false} [[ $PR_WORKFLOW_CHANGE_OK == true || $PR_WORKFLOW_CHANGE_OK == false ]] || fail "pr-workflow-change-ok must be true or false" @@ -334,6 +352,15 @@ runs: printf -- '- %s\n' "$msg" >>"$GITHUB_STEP_SUMMARY" exit 0 fi + # The broker reads the kill switches live; this run's vars were read + # when it was queued, so a switch turned on since reaches it here. + if [[ $KILL_SWITCH_OK == true && $status == 403 && $reason == kill-switch ]]; then + msg="lane-token-stopped: the broker refused the token because a lanes kill switch is on, a designed stop; this run takes no token and skips the rest" + echo "::notice::$msg" + printf -- '- %s\n' "$msg" >>"$GITHUB_STEP_SUMMARY" + echo "skip-reason=kill-switch" >>"$GITHUB_OUTPUT" + exit 0 + fi # The refusal does not say whether the PR or the default branch # changed the file, so the skip needs GitHub's own three-dot compare # at the gated head to list it; any doubt stays red. diff --git a/.github/actions/request-lane-token/request-lane-token.test.mjs b/.github/actions/request-lane-token/request-lane-token.test.mjs index a34dee91f1c..b29b5e6ee70 100644 --- a/.github/actions/request-lane-token/request-lane-token.test.mjs +++ b/.github/actions/request-lane-token/request-lane-token.test.mjs @@ -397,6 +397,68 @@ test("without superseded-ok, a 403 workflow-modified still fails red", () => { ); }); +// The 403 pr-merge run 40747727299 logged when the breaker turned the +// repository kill switch on after the run had read its variables. +const KILLED = { + status: "403", + body: { reason: "kill-switch", detail: 'not-false: the repository CLAUDE_LANES_REPO_DISABLED is set and not exactly "false"' }, +}; +const KILLED_ERROR = + 'lane-token-denied: the broker answered 403 with reason kill-switch: not-false: the repository CLAUDE_LANES_REPO_DISABLED is set and not exactly "false"'; + +test("with kill-switch-ok, a 403 kill-switch ends green with a notice, skip-reason kill-switch and no token", () => { + const run = request({ ...MERGE, KILL_SWITCH_OK: "true" }, KILLED); + assert.equal(run.status, 0, run.stdout); + assert.match(run.stdout, /^::notice::lane-token-stopped: the broker refused the token because a lanes kill switch is on/m); + assert.doesNotMatch(run.stdout, /::error::/); + assert.equal(run.output, "skip-reason=kill-switch\n"); + assert.equal(run.auth.length, 1); +}); + +test("with kill-switch-ok, any other denial reason still fails red", () => { + const run = request({ ...MERGE, KILL_SWITCH_OK: "true" }, { status: "403", body: { reason: "already-minted" } }); + assert.equal(run.status, 1); + assert.equal(run.output, "failure-reason=lane-token-denied: the broker answered 403 with reason already-minted\n"); +}); + +test("with kill-switch-ok, a kill-switch reason on a status other than 403 still fails red", () => { + const run = request({ ...MERGE, KILL_SWITCH_OK: "true" }, { ...KILLED, status: "500" }); + assert.equal(run.status, 1); + assert.doesNotMatch(run.output, /skip-reason/); +}); + +test("without kill-switch-ok, a 403 kill-switch still fails red as it did in run 40747727299", () => { + const run = request(MERGE, KILLED); + assert.equal(run.status, 1); + assert.match(run.stdout, new RegExp(`^::error::${KILLED_ERROR.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}$`, "m")); + assert.equal(run.output, `failure-reason=${KILLED_ERROR}\n`); +}); + +test("with kill-switch-ok, an allow still mints the token unchanged", () => { + const run = request({ ...MERGE, KILL_SWITCH_OK: "true" }); + assert.equal(run.status, 0, run.stdout); + assert.match(run.output, /^token=ghs_lane$/m); + assert.match(run.output, /^mint-id=5550001-1$/m); + assert.doesNotMatch(run.output, /skip-reason/); +}); + +test("kill-switch-ok in check mode fails before the broker request", () => { + const run = request( + { MODE: "check", EFFECT: "", CONTENTS: "", PULL_REQUESTS: "", ISSUES: "", PR_NUMBER: "", KILL_SWITCH_OK: "true" }, + KILLED, + ); + assert.equal(run.status, 1); + assert.match(run.stdout, /::error::kill-switch-ok needs token mode/); + assert.equal(run.body, null); +}); + +test("a kill-switch-ok other than true or false fails before the broker request", () => { + const run = request({ ...MERGE, KILL_SWITCH_OK: "yes" }, KILLED); + assert.equal(run.status, 1); + assert.match(run.stdout, /::error::kill-switch-ok must be true or false/); + assert.equal(run.body, null); +}); + // #7732: the refusal fix-docs got on #7717, a PR that changes a workflow file. // The detail's shape is the broker's, as SUPERSEDED above records it. const HEAD = "a".repeat(40); diff --git a/.github/workflows/intake-triage.yml b/.github/workflows/intake-triage.yml index 732c81f2b6a..c7c2ef27a25 100644 --- a/.github/workflows/intake-triage.yml +++ b/.github/workflows/intake-triage.yml @@ -420,6 +420,9 @@ jobs: permissions: contents: read # check out main id-token: write # prove to the token broker which workflow this is + outputs: + # `kill-switch` when the broker refused the token for a kill switch. + stopped: ${{ steps.lane-token.outputs.skip-reason }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -476,6 +479,9 @@ jobs: # Swaps this job's GitHub identity proof for a short-lived lanes App # token for this repository only, with the resolved grant (issues: # write, contents: read, pull-requests: read). + # A kill switch turned on after this run read its variables is refused + # `kill-switch`: the step ends green with a notice and no token, the + # write below skips, and refine-select skips on the `stopped` output. - name: Request the lane token id: lane-token if: steps.config.outcome == 'success' && steps.config.outputs.applies == 'true' @@ -490,6 +496,7 @@ jobs: contents: ${{ steps.config.outputs.contents }} pull-requests: ${{ steps.config.outputs.pull-requests }} issues: ${{ steps.config.outputs.issues }} + kill-switch-ok: true - name: Write the verdict if: steps.lane-token.outputs.token != '' @@ -546,7 +553,7 @@ jobs: # in a separate marked comment. Rules: .github/scripts/intake-triage/refine.mjs. refine-select: needs: [born-triaged, apply-verdict] - if: ${{ !cancelled() && needs.apply-verdict.result == 'success' }} + if: ${{ !cancelled() && needs.apply-verdict.result == 'success' && needs.apply-verdict.outputs.stopped == '' }} runs-on: ubuntu-24.04 timeout-minutes: 10 concurrency: @@ -1322,6 +1329,7 @@ jobs: contents: ${{ steps.config.outputs.contents }} pull-requests: ${{ steps.config.outputs.pull-requests }} issues: ${{ steps.config.outputs.issues }} + kill-switch-ok: true - name: Post the refinement id: apply diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 1812c4d6fa6..1c1a8dcc665 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -936,7 +936,9 @@ jobs: # run that a change to this file on main superseded is refused # `workflow-modified`; it ends green with a warning and no token, so the # steps below skip and no stale failed `merge` check lands on a PR. The - # newer run checks every PR. + # newer run checks every PR. A kill switch turned on after this run read + # its variables is refused `kill-switch`; it ends green with a notice and + # no token the same way, and the next scheduled run checks every PR. - name: Request the lane token id: lane-token if: steps.config.outcome == 'success' @@ -952,6 +954,7 @@ jobs: issues: ${{ steps.config.outputs.issues }} actions: ${{ steps.config.outputs.actions }} superseded-ok: true + kill-switch-ok: true # Candidates: open, non-draft PRs into main that the lanes App # (merge.agent-identity on main) or the owner account opened, plus @@ -2012,6 +2015,7 @@ jobs: pull-requests: ${{ steps.config.outputs.pull-requests }} issues: ${{ steps.config.outputs.issues }} superseded-ok: true + kill-switch-ok: true # The verdict reader of a scheduled or hand-started run ties untied # FAILs as the gate job does: the head-ref map and the carried run From fb1759db827060e1e99d3c641909fd2feaf56b3a Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 16:06:29 -0400 Subject: [PATCH 2/7] test(ci): pin that a kill-switch stop runs no guarded step callers.test.mjs walks every workflow job whose lane-token step sets kill-switch-ok: each later step has an if:, a step that reads the token requires it set, and a job that needs the stopped job's success and reads none of its outputs skips on its skip output. It fails when refine-select's stopped clause is removed. Four pr-merge merge steps that relied on an earlier token-gated step now name the token gate themselves, which changes no run. Co-Authored-By: Claude Opus 5.5 --- .../actions/resolve-config/callers.test.mjs | 50 ++++++++++++++++++- .github/workflows/pr-merge.yml | 8 +-- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/.github/actions/resolve-config/callers.test.mjs b/.github/actions/resolve-config/callers.test.mjs index cd1a9ade4f0..bec58dfcce0 100644 --- a/.github/actions/resolve-config/callers.test.mjs +++ b/.github/actions/resolve-config/callers.test.mjs @@ -497,8 +497,11 @@ test("intake-triage.yml: a thin issue's refinement runs its model with no write assert.equal(job.concurrency["cancel-in-progress"], false); } - assert.equal(evaluate(select.if, { "needs.apply-verdict.result": "success" }), true); - assert.equal(evaluate(select.if, { "needs.apply-verdict.result": "failure" }), false); + const selects = (result, stopped) => + evaluate(select.if, { "needs.apply-verdict.result": result, "needs.apply-verdict.outputs.stopped": stopped }); + assert.equal(selects("success", ""), true); + assert.equal(selects("failure", ""), false); + assert.equal(selects("success", "kill-switch"), false, "a kill-switch stop of apply-verdict refines nothing"); assert.equal(evaluate(model.if, { "needs.refine-select.outputs.refine": "true" }), true); assert.equal(evaluate(model.if, { "needs.refine-select.outputs.refine": "false" }), false); // The subscription leg runs only after the key leg handed off or found no key. @@ -6809,3 +6812,46 @@ test("every workflow npm ci reads npm-fetch.npmrc from the job's checkout", () = // and one each in ten other workflows. assert.ok(covered >= 15, `only ${covered} installing jobs found`); }); + +// request-lane-token's kill-switch-ok ends a kill-switch refusal green with no +// token, so nothing after it may run on success() alone: every later step is +// gated, a step that reads the token runs only when it is set, and a job that +// needs the stopped job's success and reads none of its outputs also needs its +// skip output empty (an output only a token step sets already stops it). +test("every kill-switch-ok caller runs no token step and no dependent job on a kill-switch stop", () => { + const callers = []; + for (const name of readdirSync(new URL("../../workflows/", import.meta.url)).filter((f) => /\.ya?ml$/.test(f))) { + const { jobs } = workflow(name); + for (const [id, job] of Object.entries(jobs ?? {})) { + const steps = job.steps ?? []; + const at = steps.findIndex((step) => step.with?.["kill-switch-ok"] === true); + if (at === -1) continue; + callers.push(`${name}:${id}`); + const tokenId = steps[at].id; + assert.ok(tokenId, `${name}:${id}: the token step has an id`); + const tokenRef = `steps.${tokenId}.outputs.token`; + for (const step of steps.slice(at + 1)) { + const where = `${name}:${id}: "${step.name}"`; + assert.equal(typeof step.if, "string", `${where} has an if:, so it never runs on success() alone`); + if (JSON.stringify({ env: step.env, with: step.with, run: step.run }).includes(tokenRef)) { + assert.ok(step.if.includes(`${tokenRef} != ''`), `${where} reads the token only when it is set`); + } + } + const [skip] = Object.entries(job.outputs ?? {}).find(([, value]) => value.includes(`steps.${tokenId}.outputs.skip-reason`)) ?? []; + for (const [otherId, other] of Object.entries(jobs)) { + const needs = [other.needs ?? []].flat(); + const cond = String(other.if ?? ""); + if (!needs.includes(id) || !cond.includes(`needs.${id}.result == 'success'`)) continue; + if (cond.replaceAll(`needs.${id}.outputs.${skip} `, "").includes(`needs.${id}.outputs.`)) continue; + assert.ok(skip, `${name}:${id} exposes the token step's skip-reason for ${otherId}`); + assert.ok(cond.includes(`needs.${id}.outputs.${skip} == ''`), `${name}:${otherId} skips on ${id}'s ${skip}`); + } + } + } + assert.deepEqual(callers.sort(), [ + "intake-triage.yml:apply-refinement", + "intake-triage.yml:apply-verdict", + "pr-merge.yml:enable-auto-merge", + "pr-merge.yml:merge", + ]); +}); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 1c1a8dcc665..bfcce3d84ac 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1251,7 +1251,7 @@ jobs: # here and are reported as a write failure (exit 4); the # post-merge-dispatch job starts those. Live mode only. - name: Record a failed lane merge - if: steps.mode.outputs.live == 'true' && steps.post-merge.outputs.code == '1' + if: steps.lane-token.outputs.token != '' && steps.mode.outputs.live == 'true' && steps.post-merge.outputs.code == '1' env: GH_TOKEN: ${{ steps.lane-token.outputs.token }} API_BUDGET_TOKEN: lanes-app @@ -1285,7 +1285,7 @@ jobs: # request at its current head is marked ready here: one comments read # per such draft. - name: Re-ready a draft a whole-review request left - if: steps.mode.outputs.report-only == 'false' && steps.candidates.outcome == 'success' + if: steps.lane-token.outputs.token != '' && steps.mode.outputs.report-only == 'false' && steps.candidates.outcome == 'success' continue-on-error: true env: GH_TOKEN: ${{ steps.lane-token.outputs.token }} @@ -1308,7 +1308,7 @@ jobs: # saved for the gate's first candidate. Any error gates every # candidate. Rules: babysit_merge_skip.py. - name: Skip refusals whose inputs have not changed - if: steps.candidates.outcome == 'success' + if: steps.lane-token.outputs.token != '' && steps.candidates.outcome == 'success' continue-on-error: true env: GH_TOKEN: ${{ steps.lane-token.outputs.token }} @@ -1357,7 +1357,7 @@ jobs: # back to the capped rotation, never deferred again. - name: Run the merge gate on each candidate id: gate - if: steps.candidates.outcome == 'success' + if: steps.lane-token.outputs.token != '' && steps.candidates.outcome == 'success' env: GH_TOKEN: ${{ steps.lane-token.outputs.token }} # The verdict reader's Actions calls, the review-evidence reads From 327371a343fbddd4168ad3234f86b04fe6f8d9d3 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 16:11:14 -0400 Subject: [PATCH 3/7] fix(pr-merge): keep a head a pr-unblock lane stopped out of the queue ci-status's merge-queue run fails a head whose newest pr-unblock/* status by github-actions[bot] is failure or error, but only after every lane leg ran, and the queue then ejected the pull request. enable-auto-merge already refuses the other ci-status holds (labels, body and member holds, hold and cost paths, the verifier verdict, the kill switch) before it arms; this was the one it lacked. merge-holds.mjs gains unblockStops and readStatuses, the rule as ci-status states it. enable-auto-merge reads the head's statuses with the discussion and leaves a stopped head, or one whose statuses do not read, unarmed (disarming an armed one), with a run-summary line for each. The merge-job pins follow the explicit token gates. Co-Authored-By: Claude Opus 5.5 --- ...-kill-switch-deny-ends-neutral-3b7e91c4.md | 7 +++ .../scripts/pr-merge/enable-auto-merge.mjs | 45 ++++++++++++- .../pr-merge/enable-auto-merge.test.mjs | 63 ++++++++++++++++++- .github/scripts/pr-merge/merge-holds.mjs | 43 +++++++++++++ .github/scripts/pr-merge/merge-job.test.mjs | 4 +- 5 files changed, 156 insertions(+), 6 deletions(-) diff --git a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md index d56d89dd1b6..54267dcb486 100644 --- a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md +++ b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md @@ -14,3 +14,10 @@ bump: patch intake-triage's `refine-select` now skips on apply-verdict's new `stopped` output. Any other denial, and every caller without the input, stays red; work-resolve-issue stays red because work-unblock re-fires its issue from the failed mint step. `version` stays 2. + +- pr-merge's `enable-auto-merge` no longer queues a lanes App pull request whose head a pr-unblock + lane stopped. ci-status's merge-queue run fails a head whose newest `pr-unblock/*` status by + github-actions[bot] is `failure` or `error`, but only after every lane leg ran, and the queue then + ejected it. The rule now lives in `merge-holds.mjs` (`unblockStops`, `readStatuses`): a stopped + head, or one whose statuses do not read, is left unarmed (an armed one is disarmed), and each + such skip gets a line in the run summary. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index a629534e44c..a84bbb1f899 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -175,7 +175,20 @@ import { createGitHub, errorLabel, GitHubError } from "../../actions/check-trust import { rateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; import { readHoldPaths } from "../work-resolve-issue/report-outcome.mjs"; -import { BODY_HOLD, costDecision, costHold, humansOf, LANE_HOLD, liftersOf, pathHoldKind, pathLifters, personWrote, vetoers } from "./merge-holds.mjs"; +import { + BODY_HOLD, + costDecision, + costHold, + humansOf, + LANE_HOLD, + liftersOf, + pathHoldKind, + pathLifters, + personWrote, + readStatuses, + unblockStops, + vetoers, +} from "./merge-holds.mjs"; export { BODY_HOLD, HUMAN_VETO } from "./merge-holds.mjs"; @@ -984,6 +997,15 @@ export function rotated(pulls, runNumber) { return [...pulls.slice(start), ...pulls.slice(0, start)]; } +// Why a head's pr-unblock statuses hold it (merge-holds.unblockStops), or +// null: anything but a read list of stops holds it. +export function unblockHold(stops) { + if (!Array.isArray(stops)) { + return "its head's pr-unblock statuses could not be read"; + } + return stops.length === 0 ? null : `a pr-unblock lane stopped its head (${stops.map(({ context, state }) => `${context} is ${state}`).join(", ")}), which ci-status fails in the merge queue`; +} + // What to do with one listed pull request: { action: arm | disarm | none, reason }. // `detail` is its discussion; undefined means not read yet, and an arm or a // keep then waits on it (action `read`). Its `verify` is the gate's verifier @@ -1023,6 +1045,12 @@ export function decide(pull, { bot, humanGated, holdPaths = null }, detail) { if (detail.filesCapped === true) { return { action: armed ? "disarm" : "none", reason: FILES_CAPPED_REASON, clears: "person" }; } + // ci-status's merge-queue run fails a head a pr-unblock lane stopped, so it + // is never queued; a status list that did not read holds it too. + const stopped = unblockHold(detail.unblockStops); + if (stopped !== null) { + return { action: armed ? "disarm" : "none", reason: stopped, clears: "lane" }; + } // A hold path a person clears, lifted by a trusted person's approval of the // head or of an earlier head carried over base merges (`lifters`) as // ci-status lifts it; a listing or check that could not run @@ -1193,6 +1221,8 @@ export async function main({ env = process.env, github, actions, verify, room, s let networkArms = 0; let unexplainedArms = 0; const skippedArms = []; + // Each pull request a pr-unblock stop on its head kept out of the queue, for the summary. + const unblockSkips = []; if (holdPaths === null) { console.log("::warning title=enable-auto-merge::the resolved config's hold paths did not read, so nothing is armed this run; the next run reads them again"); } @@ -1316,7 +1346,13 @@ export async function main({ env = process.env, github, actions, verify, room, s if (!lifters.length && Array.isArray(holdPaths) && pathHoldKind(files, holdPaths)?.kind === "hold-path") { lifters = (await liftersOf(github, repo[0], pull.number, pull.headRefOid, humans)).lifters; } - detail = { ...detail, files, filesCapped: capped, costDecision: decision, lifters }; + let stops = null; + try { + stops = unblockStops(await readStatuses(github, `${repo[1]}/${repo[2]}`, pull.headRefOid)); + } catch (error) { + console.log(`enable-auto-merge: #${pull.number} head statuses did not read: ${describe(error)}`); + } + detail = { ...detail, files, filesCapped: capped, costDecision: decision, lifters, unblockStops: stops }; } } catch (error) { // A read that fails decides nothing and writes nothing. @@ -1327,6 +1363,9 @@ export async function main({ env = process.env, github, actions, verify, room, s widen && (detail === null || unmarked) ? { action: "none", reason: labelHold, clears: undefined, comment: undefined, withdraw: undefined, partial: undefined, fold: null } : decide(judged, { bot: slug, humanGated, holdPaths }, detail)); + if (detail && reason === unblockHold(detail.unblockStops)) { + unblockSkips.push(`#${pull.number} ${action === "disarm" ? "disarmed" : "not queued"}: ${escapeCommand(reason)}`); + } } // Past the budget, under serve with an answers file, an unarmed pull // request is answered only from a kept refusal, which reads no more @@ -1549,7 +1588,7 @@ export async function main({ env = process.env, github, actions, verify, room, s console.log(`${GUARD_ERROR}enable-auto-merge: ${escapeCommand(stopped.message)}; no pull request is read, noted or armed after it this run, and each armed lanes App pull request is disarmed`); } // Each verifier read's cost too, so a run's spend reads per pull request. - const summary = [...lines.map(([, text]) => text), ...(verify.costs ?? []).map(costLine)]; + const summary = [...lines.map(([, text]) => text), ...unblockSkips, ...(verify.costs ?? []).map(costLine)]; if (summary.length > 0 && env.GITHUB_STEP_SUMMARY) { try { appendFileSync(env.GITHUB_STEP_SUMMARY, summary.map((text) => `enable-auto-merge: ${text}\n`).join("")); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index e77db26972a..c9eb2e483d4 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -27,6 +27,7 @@ import { UNLABELED_QUERY, decide, describe, + unblockHold, liftedByPerson, READER_EVENTS, personReason, @@ -54,7 +55,7 @@ import { } from "./enable-auto-merge.mjs"; import { GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { createRateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; -import { COMMENT_EDITED_QUERY, LANE_HOLD, PULL_OPENED_QUERY, VERIFY_PATH, personWrote, verdictHold } from "./merge-holds.mjs"; +import { COMMENT_EDITED_QUERY, LANE_HOLD, PULL_OPENED_QUERY, VERIFY_PATH, personWrote, unblockStops, verdictHold } from "./merge-holds.mjs"; import { VERIFY_PATH as RESOLVER_VERIFY_PATH } from "../pr-verify/resolve-pr.mjs"; import { HOLD_LINE, decide as decideOutcome } from "../work-resolve-issue/report-outcome.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; @@ -94,6 +95,8 @@ function detail({ comments = [], reviews = [], inline = [], disabledBy = null } reviews: page(reviews), reviewThreads: page(inline.length ? [{ comments: page(inline) }] : []), timelineItems: { nodes: disabledBy ? [{ actor: disabledBy }] : [] }, + // The head's pr-unblock stops (merge-holds.unblockStops): none. + unblockStops: [], }; } const said = (login, body, at, association = "OWNER", typename = "User") => ({ @@ -143,6 +146,16 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note const files = pulls.find((candidate) => candidate.number === Number(listing[1]))?.files ?? [{ filename: "plugins/work-items/README.md" }]; return files.slice((Number(listing[2]) - 1) * 100, Number(listing[2]) * 100); } + // The head's commit statuses: a pull request's `statuses`, none by + // default; `statuses: "error"` fails the read. + const statusPage = /^\/repos\/melodic-software\/claude-code-plugins\/commits\/([0-9a-f]{40})\/statuses\?per_page=100&page=1$/.exec(path); + if (method === "GET" && statusPage) { + const statuses = pulls.find((candidate) => candidate.headRefOid === statusPage[1])?.statuses ?? []; + if (statuses === "error") { + throw new Error("GET statuses failed with status 502"); + } + return statuses; + } // The cost decision reads (merge-holds.costDecision): the owner's comment // 7 on pull request 40's closing issue, and each pull request closes its own // number plus 1000. @@ -2947,3 +2960,51 @@ test("serve asks for a kept answer only for an unarmed pull request, and never o assert.deepEqual(timeouts, [VERIFY_TIMEOUT_MS, VERIFY_TIMEOUT_MS, VERIFY_TIMEOUT_MS, CACHE_ONLY_TIMEOUT_MS]); assert.deepEqual(spawned[0].slice(spawned[0].indexOf("--answers-out"), spawned[0].indexOf("--answers-out") + 6), ["--answers-out", "/tmp/out.json", "--answers-mode", "serve", "--answers-in", "/tmp/in.json"]); }); + +// ci-status's merge-queue run fails a head whose newest pr-unblock/* status by +// github-actions[bot] is failure or error (pr-require-checks.yml "Fail a head a +// pr-unblock lane stopped"), so enable-auto-merge never queues one. +const BOT = { login: "github-actions[bot]" }; +const unblock = (context, state, creator = BOT, description = "stopped") => ({ context, state, creator, description }); + +test("unblockStops keeps each pr-unblock context's newest github-actions status that fails", () => { + assert.deepEqual( + unblockStops([ + unblock("pr-unblock/fix-ci", "failure", BOT, "fix-ci\tstopped"), + unblock("pr-unblock/fix-ci", "success"), + unblock("pr-unblock/address-feedback", "success"), + unblock("pr-unblock/address-feedback", "failure"), + unblock("pr-unblock/resolve-conflicts", "error"), + unblock("pr-unblock/fix-docs", "failure", { login: "someone" }), + unblock("ci-lanes", "failure"), + ]), + [ + { context: "pr-unblock/fix-ci", state: "failure", description: "fix-ci stopped" }, + { context: "pr-unblock/resolve-conflicts", state: "error", description: "stopped" }, + ], + ); + assert.throws(() => unblockStops(null), /not a list/); +}); + +test("a head a pr-unblock lane stopped is not queued and is named in the summary; an unheld one still arms", async () => { + const summary = join(tempDir("enable-auto-merge-unblock-"), "summary.md"); + writeFileSync(summary, ""); + const stopped = pull(1, { verdict: PASSED, statuses: [unblock("pr-unblock/fix-ci", "failure")] }); + const { writes, lines } = await run([stopped], { GITHUB_STEP_SUMMARY: summary }); + assert.deepEqual(writes, []); + const reason = "a pr-unblock lane stopped its head (pr-unblock/fix-ci is failure), which ci-status fails in the merge queue"; + assert.ok(lines.some((line) => String(line).includes(reason)), lines.join("\n")); + assert.equal(readFileSync(summary, "utf8"), `enable-auto-merge: #1 not queued: ${reason}\n`); + + const cleared = pull(1, { verdict: PASSED, statuses: [unblock("pr-unblock/fix-ci", "success"), unblock("pr-unblock/fix-ci", "failure")] }); + assert.equal((await run([cleared])).writes[0]?.mutation, "arm"); + assert.equal((await run([pull(1, { verdict: PASSED })])).writes[0]?.mutation, "arm"); +}); + +test("a head whose statuses do not read is not queued, and an armed one is disarmed", async () => { + assert.deepEqual((await run([pull(1, { verdict: PASSED, statuses: "error" })])).writes, []); + const armed = pull(1, { verdict: PASSED, statuses: "error", autoMergeRequest: { enabledAt: NOW } }); + assert.equal((await run([armed])).writes[0]?.mutation, "disarm"); + assert.equal(unblockHold(null), "its head's pr-unblock statuses could not be read"); + assert.equal(unblockHold([]), null); +}); diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 69dd8ed1022..c5a720dbc34 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -1309,6 +1309,49 @@ export function changeOf(line) { return { path: line.filename, previous: typeof previous === "string" && previous ? previous : null, status: typeof line.status === "string" ? line.status : null }; } +// A HEAD A PR-UNBLOCK LANE STOPPED: each `pr-unblock/*` context whose newest +// status by github-actions[bot] is `failure` or `error`, as +// { context, state, description }. `statuses` are a commit's REST statuses, +// newest first as GitHub lists them, so a later `success` on the head +// supersedes a failure, and another account's status neither holds nor +// clears. ci-status's pr-unblock step and enable-auto-merge both judge a head +// by this; a list that is not an array throws. +export function unblockStops(statuses) { + if (!Array.isArray(statuses)) { + throw new Error("the head's statuses are not a list"); + } + const seen = new Set(); + const stops = []; + for (const status of statuses) { + const context = status?.context; + if (typeof context !== "string" || !context.startsWith("pr-unblock/") || status?.creator?.login !== "github-actions[bot]" || seen.has(context)) { + continue; + } + seen.add(context); + if (status.state === "failure" || status.state === "error") { + stops.push({ context, state: status.state, description: String(status.description ?? "").replaceAll(/[\t\r\n]/g, " ") }); + } + } + return stops; +} + +// Every REST status of `head`, 100 a page, newest first. A page that is not a +// list, or more than 10 pages, throws. +export async function readStatuses(github, repo, head) { + const statuses = []; + for (let page = 1; page <= 10; page += 1) { + const rows = await github("GET", `/repos/${repo}/commits/${head}/statuses?per_page=100&page=${page}`); + if (!Array.isArray(rows)) { + throw new Error("the head's statuses did not read"); + } + statuses.push(...rows); + if (rows.length < 100) { + return statuses; + } + } + throw new Error("more than 1000 statuses on the head"); +} + // Whether a person wrote the body of pull request `number` (personWrote); a // read that fails counts as a person. async function bodyByPerson(github, repo, number) { diff --git a/.github/scripts/pr-merge/merge-job.test.mjs b/.github/scripts/pr-merge/merge-job.test.mjs index d11399a3f00..e918490948e 100644 --- a/.github/scripts/pr-merge/merge-job.test.mjs +++ b/.github/scripts/pr-merge/merge-job.test.mjs @@ -906,7 +906,7 @@ test("a failed lane merge reruns the check with --remediate under the lane token assert.ok(check >= 0 && record === check + 1, "the record step follows the check"); assert.ok(record < names.indexOf("Run the merge gate on each candidate")); const block = step("Record a failed lane merge"); - assert.match(block, /\n {8}if: steps\.mode\.outputs\.live == 'true' && steps\.post-merge\.outputs\.code == '1'\n/); + assert.match(block, /\n {8}if: steps\.lane-token\.outputs\.token != '' && steps\.mode\.outputs\.live == 'true' && steps\.post-merge\.outputs\.code == '1'\n/); assert.match(block, /\n {10}GH_TOKEN: \$\{\{ steps\.lane-token\.outputs\.token \}\}\n/); assert.match(runScript("Check earlier lane merges"), /^echo "code=\$code" >>"\$GITHUB_OUTPUT"$/m); }); @@ -1247,7 +1247,7 @@ test("the skip step runs just before the gate and its record between the carry a assert.equal(at("Upload the verdict reads for the next run"), at("Keep the refusals the next run may skip") + 1); const filter = step("Skip refusals whose inputs have not changed"); assert.match(filter, /\n {8}continue-on-error: true\n/); - assert.match(filter, /\n {8}if: steps\.candidates\.outcome == 'success'\n/); + assert.match(filter, /\n {8}if: steps\.lane-token\.outputs\.token != '' && steps\.candidates\.outcome == 'success'\n/); assert.match(filter, /GH_TOKEN: \$\{\{ steps\.lane-token\.outputs\.token \}\}/); assert.match(filter, /BABYSIT_VERIFY_GH_TOKEN: \$\{\{ github\.token \}\}/); // The listing it reads is saved where the gate's first candidate looks. From 8f1af503eb6e98ca0421b4582388f3d9b75a78f5 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 16:52:07 -0400 Subject: [PATCH 4/7] fix(pr-merge): lift a hold-path escalation on a trusted approval of the head The lane's needs-human refusal for a hold path says a person must remove the label, and ci-status says an approving review of the head by a trusted actor lifts the hold, yet the owner still removed the label by hand after approving (#10665, #10738, #10745). On a reader run, a labeled lanes App pull request whose newest lane refusal at its head names only hold paths is read on when a trusted person approved in its discussion. When merge-holds' lifters (an approval bound to the head, or one carried over base merges) lift the hold path and no cost hold stands, the lane deletes the label with its own token, which re-runs ci-status at the head, and judges the pull request as unlabeled. Any other reason, head, reviewer or a dismissed approval leaves the label on. Co-Authored-By: Claude Opus 5.5 --- ...-kill-switch-deny-ends-neutral-3b7e91c4.md | 7 +++ .../scripts/pr-merge/enable-auto-merge.mjs | 54 ++++++++++++++++++- .../pr-merge/enable-auto-merge.test.mjs | 52 ++++++++++++++---- 3 files changed, 102 insertions(+), 11 deletions(-) diff --git a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md index 54267dcb486..a82e983ee9d 100644 --- a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md +++ b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md @@ -21,3 +21,10 @@ bump: patch ejected it. The rule now lives in `merge-holds.mjs` (`unblockStops`, `readStatuses`): a stopped head, or one whose statuses do not read, is left unarmed (an armed one is disarmed), and each such skip gets a line in the run summary. + +- pr-merge's `enable-auto-merge` takes `needs-human` off a lanes App pull request itself when the + lane's newest refusal at the head names only hold paths and a person on the trusted-actor list + approved that head (merge-holds `pathLifters`, or `liftersOf` over base merges), the release the + refusal and ci-status already name. Removing the label re-runs ci-status at the head. A cost, + risk or verifier reason, a refusal at another head, an approval of another head, a dismissed + approval, an untrusted reviewer or a report-only run lifts nothing. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index a84bbb1f899..cb5bde8da49 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -166,6 +166,12 @@ // which reads in full and logs how a kept answer // compares (VERIFY_CACHE_PREFIX) // GITHUB_API_URL +// +// A reader run takes the human-gated label off a pull request whose newest +// refusal at its head names only hold paths once a trusted person approved +// that head (approvalLift), which re-runs ci-status there; every other +// person's refusal keeps waiting for a person to remove the label. + import { spawnSync } from "node:child_process"; import { appendFileSync, readFileSync } from "node:fs"; import process from "node:process"; @@ -616,6 +622,32 @@ function personReasons(body, head, label) { return open >= 0 && refusalBody(true, reasons.join("\n"), head, label, null, partial) === body ? reasons : null; } +// A person's refusal at `head` whose every reason is a hold path +// (merge-holds.pathHoldKind's "it changes , a hold path"): the one +// escalation a trusted person's approval of the head lifts, so the lane may +// take its label off. A cost, risk or verifier reason, a label a person set +// with no refusal, or a comment at another head is not one. +const HOLD_PATH_LINE = /^it changes .+, a hold path$/; +export function holdPathOnly(body, head, label) { + const reasons = personReasons(body, head, label); + return reasons !== null && reasons.length > 0 && reasons.every((line) => HOLD_PATH_LINE.test(line)); +} + +// Whether the lane lifts `label` off `pull`: its newest refusal at the head +// names only hold paths, the changed files still hold on nothing but a hold +// path (no cost hold, a listing read whole), and `detail.lifters` holds a +// trusted person whose approval binds the head (merge-holds pathLifters, or +// liftersOf over base merges). +export function approvalLift(pull, detail, { bot, label, holdPaths }) { + if (!holdPathOnly(priorRefusal(detail?.comments?.nodes, bot)?.body, pull.headRefOid, label)) { + return false; + } + if (!Array.isArray(holdPaths) || detail.filesCapped === true || pathHoldKind(detail.files, holdPaths)?.kind !== "hold-path") { + return false; + } + return costHold(detail.files, detail.costDecision) === null && Array.isArray(detail.lifters) && detail.lifters.length > 0; +} + // A reason as the comment's fenced block carries it. const fenced = (reason) => String(reason).replaceAll(/[\u0000-\u0008\u000b-\u001f\u007f]/g, "?").replaceAll("```", "'''"); @@ -1317,7 +1349,7 @@ export async function main({ env = process.env, github, actions, verify, room, s // verifier's answer (isPartial): a reader run with time left reads it // once, judged as if unlabeled, to widen the comment. It never arms. const labelHold = reason; - const widen = reads && stopped === null && held === null && pull.autoMergeRequest == null && reason === `it carries ${humanGated}` && now() <= deadline && roomHold(pull) === null; + let widen = reads && stopped === null && held === null && pull.autoMergeRequest == null && reason === `it carries ${humanGated}` && now() <= deadline && roomHold(pull) === null; // A widening read finding no partial comment reads nothing past the discussion. let unmarked = false; const judged = widen ? { ...pull, labels: unlabeledAs(pull.labels, humanGated) } : pull; @@ -1332,7 +1364,11 @@ export async function main({ env = process.env, github, actions, verify, room, s detail = (await github("POST", "/graphql", { query: DETAIL_QUERY, variables }))?.data?.repository?.pullRequest ?? null; if (detail !== null) { detail = await wholeDiscussion(github, variables, detail); - unmarked = widen && !isPartial(priorRefusal(detail?.comments?.nodes, slug)?.body, pull.headRefOid); + const standing = priorRefusal(detail?.comments?.nodes, slug)?.body; + // A hold-path refusal with a trusted person's approval in the + // discussion reads on, for approvalLift below. + const approvedBy = (detail.reviews?.nodes ?? []).map(itemOf).some((item) => item.isUser && trustedHumans(env).has(item.userId) && item.state === "APPROVED"); + unmarked = widen && !isPartial(standing, pull.headRefOid) && !(approvedBy && holdPathOnly(standing, pull.headRefOid, humanGated)); } if (detail !== null && !unmarked) { const { paths: files, capped } = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); @@ -1353,6 +1389,20 @@ export async function main({ env = process.env, github, actions, verify, room, s console.log(`enable-auto-merge: #${pull.number} head statuses did not read: ${describe(error)}`); } detail = { ...detail, files, filesCapped: capped, costDecision: decision, lifters, unblockStops: stops }; + // A hold-path escalation a trusted person's approval of the head + // lifts: the lane takes its label off, which re-runs ci-status at + // the head (pr-require-checks.yml runs on unlabeled), and judges + // it as unlabeled from here. + if (widen && approvalLift(pull, detail, { bot: slug, label: humanGated, holdPaths })) { + if (write) { + await github("DELETE", `/repos/${repo[0]}/issues/${pull.number}/labels/${encodeURIComponent(humanGated)}`); + } + console.log(`enable-auto-merge: #${pull.number} ${write ? "lifted" : "would lift (report-only)"} ${escapeCommand(humanGated)}: ${detail.lifters.join(", ")} approved its head, which lifts its hold path`); + widen = false; + // Off in this run's view too, so a person's reason the verifier + // read adds puts the label back (refusalWrites). + pull.labels = unlabeledAs(pull.labels, humanGated); + } } } catch (error) { // A read that fails decides nothing and writes nothing. diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index c9eb2e483d4..c72527f23b3 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -212,6 +212,11 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note } return {}; } + // The lane lifting the human-gated label (approvalLift). + if (method === "DELETE" && /^\/repos\/melodic-software\/claude-code-plugins\/issues\/[0-9]+\/labels\/[^/]+$/.test(path)) { + notes.push({ method, path }); + return []; + } assert.equal(`${method} ${path}`, "POST /graphql"); if (body.query === PULL_OPENED_QUERY) { const nodes = [{ number: body.variables.number + 1000, repository: { nameWithOwner: REPO } }]; @@ -687,19 +692,17 @@ test("a labeled pull request whose standing refusal lacks the verifier's answer const after = await run([held({ verdict: RISK_HELD, discussion: { comments: [lanesSaid(personNote(BOTH))] } })]); assert.deepEqual(after.verified, []); assert.deepEqual(after.notes, []); - // A verdict that would arm it, with the hold path lifted, never arms a labeled one. - // Its answer came and names no person's reason: the mark comes off in - // place, so the next run reads no verdict. + // A hold-path-only refusal a trusted person's approval of the head lifts: + // the lane takes the label off and judges it unlabeled, so a passing + // verdict arms it (approvalLift; the label tests below cover each refusal). const lifted = await run([held({ verdict: PASSED, discussion: { reviews: [reviewed("APPROVED")], comments: [lanesSaid(personNote(HOLD_PATH_REASON, true))] } })], trustedKyle()); assert.deepEqual(lifted.verified, [2]); - assert.deepEqual(lifted.writes, []); - assert.deepEqual(lifted.notes, [{ method: "PATCH", path: `${ISSUES}/comments/77`, body: { body: personNote(HOLD_PATH_REASON) } }]); - const laneVerdict = await run([held({ verdict: MISSING, discussion: { reviews: [reviewed("APPROVED")], comments: [lanesSaid(personNote(HOLD_PATH_REASON, true))] } })], trustedKyle()); - assert.deepEqual(laneVerdict.notes, [{ method: "PATCH", path: `${ISSUES}/comments/77`, body: { body: personNote(HOLD_PATH_REASON) } }]); - const next = await run([held({ verdict: PASSED, discussion: { reviews: [reviewed("APPROVED")], comments: [lanesSaid(personNote(HOLD_PATH_REASON))] } })], trustedKyle()); + assert.deepEqual(lifted.notes, [{ method: "DELETE", path: `${ISSUES}/2/labels/needs-human` }]); + assert.equal(lifted.writes.at(-1)?.mutation, "arm"); + // With no partial comment and no approval, only the discussion is read: no file listing. + const next = await run([held({ verdict: PASSED, discussion: { comments: [lanesSaid(personNote(HOLD_PATH_REASON))] } })], trustedKyle()); assert.deepEqual(next.verified, []); assert.deepEqual(next.notes, []); - // With no partial comment, only the discussion is read: no file listing. const listed = (calls) => calls.filter(({ path }) => /\/pulls\/2\/files\?/.test(path)).length; assert.equal(listed(next.calls), 0); // A held lane spends no read on it, partial or not. @@ -3008,3 +3011,34 @@ test("a head whose statuses do not read is not queued, and an armed one is disar assert.equal(unblockHold(null), "its head's pr-unblock statuses could not be read"); assert.equal(unblockHold([]), null); }); + +// The owner's approval of the head is the documented release of a hold-path +// escalation, so the lane takes its needs-human label off itself; nothing +// else does, and the approval control is the one ci-status applies. +test("a hold-path escalation lifts its label only on a trusted person's approval of the head", async () => { + const LIFT = { method: "DELETE", path: `${ISSUES}/2/labels/needs-human` }; + const held = (discussion, files = HELD, overrides = {}) => pull(2, { files, labels: labels("needs-human"), verdict: PASSED, discussion, ...overrides }); + const escalated = [lanesSaid(personNote(HOLD_PATH_REASON))]; + const lifted = await run([held({ reviews: [reviewed("APPROVED")], comments: escalated })], trustedKyle()); + assert.deepEqual(lifted.notes, [LIFT]); + assert.equal(lifted.writes.at(-1)?.mutation, "arm"); + + const refused = { + "an approval of another head": held({ reviews: [reviewed("APPROVED", { oid: "e".repeat(40) })], comments: escalated }), + "an approval later dismissed": held({ reviews: [reviewed("APPROVED"), reviewed("DISMISSED", { at: "2026-10-26T17:00:00Z" })], comments: escalated }), + "an approval by an account off the trusted-actor list": held({ reviews: [reviewed("APPROVED", { id: 9, login: "someone" })], comments: escalated }), + "a cost escalation": held({ reviews: [reviewed("APPROVED")], comments: [lanesSaid(personNote(COST_REASON))] }, COST_FILES), + "a hold path beside a risk hold": held({ reviews: [reviewed("APPROVED")], comments: [lanesSaid(personNote(BOTH))] }), + "a label with no lane escalation": held({ reviews: [reviewed("APPROVED")], comments: [] }), + "an escalation at an earlier head": held({ reviews: [reviewed("APPROVED")], comments: [lanesSaid(personNote(HOLD_PATH_REASON).replaceAll(HEAD, "e".repeat(40)))] }), + }; + for (const [why, pr] of Object.entries(refused)) { + const result = await run([pr], trustedKyle()); + assert.ok(!result.notes.some(({ method }) => method === "DELETE"), `${why} lifts nothing`); + assert.ok(!result.writes.some(({ mutation }) => mutation === "arm"), `${why} arms nothing`); + } + + const reportOnly = await run([held({ reviews: [reviewed("APPROVED")], comments: escalated })], { ...trustedKyle(), REPORT_ONLY: "true" }); + assert.deepEqual(reportOnly.notes.filter(({ method }) => method === "DELETE"), []); + assert.ok(reportOnly.lines.some((line) => String(line).includes("would lift (report-only) needs-human")), reportOnly.lines.join("\n")); +}); From 37e56e19b26c630c81b59264ab1543f8a4d48112 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 17:25:47 -0400 Subject: [PATCH 5/7] fix(pr-merge): carry a hold-path approval over pushes that touch no gated code The owner's decision Q10 of 2026-10-28 (#10759): a person's approval of a hold-path head stays valid at a later head when the diff between them touches no hold-path file. approvalCarry and its Python twin approval_carry take that as a second carry path beside the base-merge one, read from the compare of the approved head to the head. That diff must also touch no file a hold path loads: the enforcement floor, and CARRY_REFUSES for the workflows, actions and scripts hold-path workflows run, the trusted-actor list and the pipeline config. test_merge_holds_sync.py derives that closure and fails on any member outside the floor and CARRY_REFUSES. A compare not read whole, a head not ahead, a dismissal the push did not make, an untrusted or dismissed approval still carry nothing. Co-Authored-By: Claude Opus 5.5 --- ...-kill-switch-deny-ends-neutral-3b7e91c4.md | 11 ++ ...-kill-switch-deny-ends-neutral-d729d8dd.md | 21 +++ .../scripts/pr-merge/enable-auto-merge.mjs | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 69 ++++++-- .github/scripts/pr-merge/merge-holds.test.mjs | 96 ++++++++++- .../scripts/babysit_merge_policy.py | 99 +++++++++-- .../scripts/babysit_verify_evidence.py | 4 +- .../tests/test_babysit_lane_reviews.py | 4 +- .../scripts/tests/test_merge_holds_sync.py | 160 ++++++++++++++++++ 9 files changed, 433 insertions(+), 33 deletions(-) create mode 100644 .changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md diff --git a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md index a82e983ee9d..b4730fec843 100644 --- a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md +++ b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md @@ -28,3 +28,14 @@ bump: patch refusal and ci-status already name. Removing the label re-runs ci-status at the head. A cost, risk or verifier reason, a refusal at another head, an approval of another head, a dismissed approval, an untrusted reviewer or a report-only run lifts nothing. + +- A trusted person's approval of a hold-path head now also carries to a later head when the diff + between the two touches no hold path and no file a hold path loads (the owner's decision Q10 of + 2026-10-28, #10759), in `merge-holds.mjs` `approvalCarry` and its twin + `babysit_merge_policy.approval_carry`. The diff is read with the hold-path rules plus the + enforcement floor and `CARRY_REFUSES` (workflows, actions, `.github/scripts`, `.github/standards`, + the pipeline config's directory, `scripts/` and `lib/`). Any edit to such a file, a test-only + or comment-only edit included, needs a new approval, and so do a compare not read whole and a + head that is not ahead of the approved one. `test_merge_holds_sync.py` derives every file the + hold paths load and fails on one outside the floor and `CARRY_REFUSES`. The control is still a + person's approval. diff --git a/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md b/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md new file mode 100644 index 00000000000..b0d5613342d --- /dev/null +++ b/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md @@ -0,0 +1,21 @@ +--- +bump: patch +--- + +### Fixed + +- carry a hold-path approval over pushes that touch no gated code + + The owner's decision Q10 of 2026-10-28 (#10759): a person's approval of + a hold-path head stays valid at a later head when the diff between them + touches no hold-path file. approvalCarry and its Python twin + approval_carry take that as a second carry path beside the base-merge + one, read from the compare of the approved head to the head. + + That diff must also touch no file a hold path loads: the enforcement + floor, and CARRY_REFUSES for the workflows, actions and scripts + hold-path workflows run, the trusted-actor list and the pipeline + config. test_merge_holds_sync.py derives that closure and fails on any + member outside the floor and CARRY_REFUSES. A compare not read whole, a + head not ahead, a dismissal the push did not make, an untrusted or + dismissed approval still carry nothing. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index cb5bde8da49..f76eb32dd05 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -1380,7 +1380,7 @@ export async function main({ env = process.env, github, actions, verify, room, s // it, so a base merge after a person's approval does not send the // pull request back to them. if (!lifters.length && Array.isArray(holdPaths) && pathHoldKind(files, holdPaths)?.kind === "hold-path") { - lifters = (await liftersOf(github, repo[0], pull.number, pull.headRefOid, humans)).lifters; + lifters = (await liftersOf(github, repo[0], pull.number, pull.headRefOid, humans, holdPaths)).lifters; } let stops = null; try { diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index c5a720dbc34..74a2677d64f 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -838,17 +838,49 @@ function sameModes(merges, trees) { } // Whether an approval of `from` carries to the head `now` was read at -// (babysit_merge_policy.approval_carry): every commit since is a merge of the -// base (baseMergesOnly), and the net change is the same (sameNetChange) or -// the merges brought exactly the base's changes (exactBaseCarry over -// `merges`, `based` and `trees`, null when not read). An approval GitHub -// dismissed as stale (autoDismissed) passes its `dismissal` commit, which -// must be one of the commits pushed since `from`, so the push the carry -// checks dismissed it. -export function approvalCarry(then, now, from, inBase, merges, based, dismissal = null, trees = null) { - if (!baseMergesOnly(then, now, from, new Set(inBase))) return false; - if (dismissal !== null && !newCommits(then, now).some((c) => c?.sha === dismissal)) return false; - return sameNetChange(then, now) || exactBaseCarry(merges, based, trees); +// (babysit_merge_policy.approval_carry), by either of two paths: +// - every commit since is a merge of the base (baseMergesOnly), and the net +// change is the same (sameNetChange) or the merges brought exactly the +// base's changes (exactBaseCarry over `merges`, `based` and `trees`, null +// when not read); +// - the owner's decision Q10 of 2026-10-28: `merges`, the compare from...H', +// touches no hold path (ungatedSince, with `holdPaths` the configured +// globs; undefined, as a caller that read none passes, skips this path). +// An approval GitHub dismissed as stale (autoDismissed) passes its +// `dismissal` commit, which must be one of the commits pushed since `from`, +// so the push the carry checks dismissed it. +export function approvalCarry(then, now, from, inBase, merges, based, dismissal = null, trees = null, holdPaths = undefined) { + const pushed = (commits) => dismissal === null || (commits ?? []).some((c) => c?.sha === dismissal); + const merged = baseMergesOnly(then, now, from, new Set(inBase)) && pushed(newCommits(then, now)) && (sameNetChange(then, now) || exactBaseCarry(merges, based, trees)); + return merged || (holdPaths !== undefined && ungatedSince(merges, holdPaths) && pushed(listedCommits(merges))); +} + +// The code a hold-path file loads that is no hold path itself: every action, +// script and workflow a hold-path workflow runs or imports, the lint and test +// scripts ci-status runs included (test_merge_holds_sync.py derives that +// closure and fails on a member these globs and HOLD_PATH_CLASSES miss). +// Holding them for a person would hold most pull requests, so a push that +// touches one only stops an approval carrying (ungatedSince); the review +// lanes still judge it. babysit_merge_policy.CARRY_REFUSES is the same list. +// The trusted-actor list and the pipeline config are read too: a widening the +// approver never saw is lifted by the same approval (actorsHold). +export const CARRY_REFUSES = Object.freeze([".github/workflows/**", ".github/actions/**", ".github/scripts/**", ".github/standards/**", "docs/conventions/pr-pipeline/**", "scripts/**", "lib/**"]); + +// Whether the compare from...H' `merges` touches no hold path and no file a +// hold path loads, old paths of renames included: pathHold with `holdPaths` +// and HOLD_PATH_FLOOR, REVIEW_FLOOR's enforcement globs (the floored code the +// gate holds for a security review, which a person's approval never +// covered) and CARRY_REFUSES. H' must be ahead of `from` with nothing behind, +// its commit list read whole, and fewer than the compare API's 300 files +// listed, each named. Any edit to such a file, a test or a comment included, +// needs a new approval. +export function ungatedSince(merges, holdPaths) { + const files = merges?.files; + if (merges?.status !== "ahead" || merges.behind_by !== 0 || listedCommits(merges) === null || !Array.isArray(files) || files.length >= 300) return false; + if (!files.every((f) => typeof f?.filename === "string")) return false; + const paths = files.flatMap((f) => [f.filename, ...(typeof f.previous_filename === "string" ? [f.previous_filename] : [])]); + const loaded = [...REVIEW_FLOOR, ...CARRY_REFUSES]; + return Array.isArray(holdPaths) && pathHold(paths, holdPaths) === null && !paths.some((path) => loaded.some((glob) => matchesHoldPath(path, glob))); } // The reviews GitHub dismissed as stale, from the pull request's issue @@ -940,7 +972,7 @@ export function liftsOf(reviews, humans, dismissed = new Map()) { // the recursive trees of H' and M' (exactBaseCarry). A read that fails lifts nothing (a timeline that does not // read restores no dismissed approval): the caller keeps the exact-head // approvals it read (pathLifters). -export async function liftersOf(github, repo, number, head, humans) { +export async function liftersOf(github, repo, number, head, humans, holdPaths = undefined) { try { const pull = await github("GET", `/repos/${repo}/pulls/${number}`); const base = pull?.base?.ref; @@ -983,7 +1015,16 @@ export async function liftersOf(github, repo, number, head, humans) { [merges, based] = [await compare(`${sha}...${head}`), await compare(`${from}...${to}`)]; trees = [await tree(head), await tree(to)]; } - if (approvalCarry(then, now, sha, inBase, merges, based, dismissal, trees)) lifters.add(login); + if (approvalCarry(then, now, sha, inBase, merges, based, dismissal, trees)) { + lifters.add(login); + continue; + } + // Q10's path reads from...H' when the base-merge path did not; a read + // that fails carries nothing by it. + if (holdPaths !== undefined && merges === null) { + merges = await compare(`${sha}...${head}`).catch(() => null); + } + if (holdPaths !== undefined && approvalCarry(then, now, sha, inBase, merges, based, dismissal, trees, holdPaths)) lifters.add(login); } return { lifters: [...lifters].sort() }; } catch { @@ -1408,7 +1449,7 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi const held = pathHold(files, holdPaths); let lifted = { lifters: [] }; if (!approved.length && ((held && Array.isArray(holdPaths)) || widened) && /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repo)) { - lifted = await liftersOf(github, repo, Number(number), head, humans); + lifted = await liftersOf(github, repo, Number(number), head, humans, holdPaths); } const lifters = approved.length ? approved : lifted.lifters; const reasons = holds(body, items, files, holdPaths, lifters, decision, byPerson, widened); diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 7414596c3f5..7aae9f5a750 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -20,7 +20,7 @@ import { join } from "node:path"; import { test } from "node:test"; import * as holdsModule from "./merge-holds.mjs"; -import { BODY_WRITERS_QUERY, COMMENT_EDITED_QUERY, HOLD_PATH_CLASSES, HOLD_PATH_FLOOR, PULL_OPENED_QUERY, REVIEW_FLOOR, baseMergeCarry, baseMergesOnly, changeOf, mergedParents, costDecision, costHold, costNotices, decidedHead, evidenceHold, laneChange, linkHold, main, pathHold, sameNetChange, unranHead } from "./merge-holds.mjs"; +import { BODY_WRITERS_QUERY, CARRY_REFUSES, COMMENT_EDITED_QUERY, approvalCarry, ungatedSince, HOLD_PATH_CLASSES, HOLD_PATH_FLOOR, PULL_OPENED_QUERY, REVIEW_FLOOR, baseMergeCarry, baseMergesOnly, changeOf, mergedParents, costDecision, costHold, costNotices, decidedHead, evidenceHold, laneChange, linkHold, main, pathHold, sameNetChange, unranHead } from "./merge-holds.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; const REPO = "owner/repo"; @@ -962,6 +962,20 @@ const BASE_DOC = { filename: "docs/base.md", status: "modified", patch: "@@ -1 + const brought = (files = [BASE_DOC], extra = {}) => ({ status: "ahead", behind_by: 0, files, commits: [{ sha: MAIN }, { sha: HEAD }], total_commits: 2, ...extra }); const baseRange = (files = [BASE_DOC], extra = {}) => ({ status: "ahead", behind_by: 0, files, commits: [{ sha: MB_HEAD }], total_commits: 1, ...extra }); const ranges = (merges = brought(), based = baseRange()) => ({ [`${OLD}...${HEAD}`]: merges, [`${MB_OLD}...${MB_HEAD}`]: based }); +// The owner's decision Q10 of 2026-10-28: an approval of OLD carries to HEAD +// when the diff OLD...HEAD touches no hold path, whatever the commits since +// are. Each case below fails the base-merge carry, but what OLD...HEAD +// brought (its `across`: docs/base.md alone) touches no hold path and no +// file a hold path loads, so the approval carries. +const UNGATED_CARRIES = new Set([ + "a base that changed a file the merges did not bring", + "a crafted merge whose second parent the base does not hold", + "a history longer than the compare lists", + "a merge after which the base holds one of the approved head's files", + "a merge of a branch other than the base", + "a merge whose parents are not commit objects", + "a non-merge commit after the approval", +]); // A recursive tree answer: each entry [path, mode]. const treeOf = (entries, extra = {}) => ({ truncated: false, tree: entries.map(([path, mode = "100644"]) => ({ path, mode, type: "blob", sha: "b".repeat(40) })), ...extra }); const FILES_AT = [["docs/base.md"], [".github/actions/collect-base-activity/collect-base-activity.test.mjs"], [".github/actions/resolve-config/live-config.test.mjs"], ["scripts/helper.sh"], ["scripts/lib.mjs"]]; @@ -1087,6 +1101,11 @@ test("a label removal lifts no hold path", async () => { })) { const world = liftWorld(options); const run = await lifting(world); + if (UNGATED_CARRIES.has(name)) { + assert.equal(run.code, 0, `${name}: ${run.out}`); + assert.match(run.out, /which lifts its hold path/, name); + continue; + } assert.equal(run.code, 1, `${name}: ${run.out}`); assert.match(run.out, HOLD_REASON, name); assert.ok(!world.reads.some((path) => /\/(events|activity)\?/.test(path) || path.includes("/contents/.work-item")), `${name}: ${world.reads.join(" ")}`); @@ -1132,6 +1151,11 @@ test("a dismissal with a message, by a person, or not by the push the carry chec "a merge that also edits a file": { timeline: [STALE], compares: { [OLD]: atOld([HELD_PATCH, HELPER]), [HEAD]: merged([HELD_PATCH, { ...HELPER, patch: "+evil" }]) }, across: ranges(brought([BASE_DOC, { ...HELPER, patch: "+evil" }])) }, })) { const run = await lifting(liftWorld({ reviews: [DISMISSED_OLD], ...options })); + if (UNGATED_CARRIES.has(name)) { + assert.equal(run.code, 0, `${name}: ${run.out}`); + assert.match(run.out, /which lifts its hold path/, name); + continue; + } assert.equal(run.code, 1, `${name}: ${run.out}`); assert.match(run.out, HOLD_REASON, name); } @@ -1193,6 +1217,11 @@ test("a merge that adds anything beyond the base's changes, or a range not read "a dismissal with a message": { ...REPLAY, reviews: [DISMISSED_OLD], timeline: [stale({ dismissal_message: "stale" })] }, })) { const run = await lifting(liftWorld(options)); + if (UNGATED_CARRIES.has(name)) { + assert.equal(run.code, 0, `${name}: ${run.out}`); + assert.match(run.out, /which lifts its hold path/, name); + continue; + } assert.equal(run.code, 1, `${name}: ${run.out}`); assert.match(run.out, HOLD_REASON, name); } @@ -1228,12 +1257,13 @@ test("no adversarial history, approval or reviewer carries a lift to the head", compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: OTHER, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: OTHER }] }], total_commits: 3 } }, }, // A crafted merge commit (itself a merge of base commits) as the second parent. - "a merge whose second parent the base does not hold": { + "a crafted merge whose second parent the base does not hold": { compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: OTHER, parents: [{ sha: MAIN }, { sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: OTHER }] }], total_commits: 3 } }, }, "a merge whose parents are not commit objects": { compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [OLD, MAIN] }], total_commits: 2 } } }, "a force push that rewrites the approved head": { compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: "6".repeat(40), parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: "6".repeat(40) }, { sha: MAIN }] }], total_commits: 2 } }, + across: ranges(brought([BASE_DOC], { status: "diverged", behind_by: 1 })), }, "a history longer than the compare lists": { compares: { [OLD]: atOld(), [HEAD]: { ...merged([HELD_PATCH]), total_commits: 251 } } }, "a head in a fork": { fork: true }, @@ -1252,6 +1282,11 @@ test("no adversarial history, approval or reviewer carries a lift to the head", }; for (const [name, options] of Object.entries(cases)) { const run = await lifting(liftWorld(options)); + if (UNGATED_CARRIES.has(name)) { + assert.equal(run.code, 0, `${name}: ${run.out}`); + assert.match(run.out, /which lifts its hold path/, name); + continue; + } assert.equal(run.code, 1, `${name}: ${run.out}`); assert.match(run.out, HOLD_REASON, name); } @@ -1290,6 +1325,11 @@ test("the exact carry reads file modes from the trees, and a non-default base ca "an unreadable repository": { defaultBranch: null }, })) { const run = await lifting(liftWorld(options)); + if (UNGATED_CARRIES.has(name)) { + assert.equal(run.code, 0, `${name}: ${run.out}`); + assert.match(run.out, /which lifts its hold path/, name); + continue; + } assert.equal(run.code, 1, `${name}: ${run.out}`); assert.match(run.out, HOLD_REASON, name); } @@ -1363,3 +1403,55 @@ test("exactBaseCarry, approvalCarry and autoDismissed read their inputs strictly assert.deepEqual([...autoDismissed([STALE, stale({ dismissal_message: "x" })])], []); assert.deepEqual([...autoDismissed("not a list")], []); }); + +// The owner's decision Q10 of 2026-10-28, verbatim in #10759: "go w/ +// recommendations". An approval of H0 stays valid at H1 when the diff H0...H1 +// touches no hold-path file; a push that touches one needs a new approval, +// a test-only or comment-only edit included. The peer review of #10737 adds +// the files hold paths load (REVIEW_FLOOR, CARRY_REFUSES). +const H0 = "a".repeat(40); +const H1 = "b".repeat(40); +const pushed = (files, extra = {}) => ({ status: "ahead", behind_by: 0, files: files.map((filename) => ({ filename, status: "modified", patch: "+x" })), commits: [{ sha: H1 }], total_commits: 1, ...extra }); +const carriesQ10 = (merges, holdPaths = [], dismissal = null) => approvalCarry(null, null, H0, [], merges, null, dismissal, null, holdPaths); + +test("Q10: an approval carries over a push that touches no hold path", () => { + assert.equal(carriesQ10(pushed(["docs/a.md", "plugins/work-items/README.md"])), true); + assert.equal(carriesQ10(pushed(["docs/a.md"]), [], H1), true, "a stale dismissal by the push it checks"); + // No hold paths read: this path does not run, and the base-merge path alone decides. + assert.equal(approvalCarry(null, null, H0, [], pushed(["docs/a.md"]), null, null, null), false); +}); + +test("Q10: a push that touches any hold path, a configured one or a file a hold path loads refuses", () => { + for (const [name, files, holdPaths] of [ + ["a gated file", [".github/scripts/pr-merge/merge-holds.mjs"], []], + ["a test-only edit to a gated file's directory", [".github/scripts/pr-merge/merge-holds.test.mjs"], []], + ["a comment-only edit to a gated file", ["plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py"], []], + ["a rename off a gated file", ["docs/moved.md"], []], + ["a configured hold path", ["docs/conventions/held.md"], ["docs/conventions/**"]], + ["a helper a gated file imports", ["plugins/source-control/skills/babysit-prs/scripts/babysit_util.py"], []], + ["a script ci-status runs", ["scripts/check-stale-base-overlap.sh"], []], + ["an action a hold-path workflow uses", [".github/actions/report-check-run/report-check-run.mjs"], []], + ["a workflow", [".github/workflows/pr-review-hosted.yml"], []], + ]) { + const merges = pushed(files); + if (name === "a rename off a gated file") merges.files[0].previous_filename = ".github/actions/request-lane-token/action.yml"; + assert.equal(carriesQ10(merges, holdPaths), false, name); + assert.equal(ungatedSince(merges, holdPaths), false, name); + } + assert.ok(CARRY_REFUSES.includes("scripts/**")); +}); + +test("Q10: a compare not read whole, not ahead, or with a dismissal the push did not make refuses", () => { + const many = Array.from({ length: 300 }, (_, i) => `docs/${i}.md`); + for (const [name, merges, dismissal] of [ + ["an unreadable compare", null, null], + ["300 files listed", pushed(many), null], + ["a commit list cut short", pushed(["docs/a.md"], { total_commits: 251 }), null], + ["a diverged head", pushed(["docs/a.md"], { status: "diverged", behind_by: 1 }), null], + ["a file with no name", { ...pushed([]), files: [{ status: "modified" }] }, null], + ["a stale dismissal by another commit", pushed(["docs/a.md"]), "c".repeat(40)], + ]) { + assert.equal(carriesQ10(merges, [], dismissal), false, name); + } + assert.equal(carriesQ10(pushed(["docs/a.md"]), null), false, "unread hold paths"); +}); diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index 2239c9858e5..12516b25d3c 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -61,6 +61,8 @@ ) from babysit_risk import ( FLOOR, + HIGH_FLOOR, + REVIEW_FLOOR, ChangedFile, glob_match, is_trusted_actors_path, @@ -2696,6 +2698,50 @@ def modes(tree: Any) -> dict[str, str] | None: return True +# The code a hold-path file loads that is no hold path itself (merge-holds.mjs +# CARRY_REFUSES, the same list): a push that touches one stops an approval +# carrying (`ungated_since`); test_merge_holds_sync.py derives that closure. +CARRY_REFUSES: tuple[str, ...] = ( + ".github/workflows/**", + ".github/actions/**", + ".github/scripts/**", + ".github/standards/**", + "docs/conventions/pr-pipeline/**", + "scripts/**", + "lib/**", +) + + +def ungated_since(merges: Any, hold_paths: Sequence[str] | None) -> bool: + """Whether the compare start...H' `merges` touches no hold path and no + file a hold path loads, old paths of renames included (merge-holds.mjs + `ungatedSince`): `hold_paths` and HIGH_FLOOR, REVIEW_FLOOR and + CARRY_REFUSES. H' ahead with nothing behind, its commit list read whole + and fewer than the compare API's 300 files listed, each named.""" + compare = json_object(merges) + files = compare.get("files") + if ( + hold_paths is None + or compare.get("status") != "ahead" + or compare.get("behind_by") != 0 + or _listed_commits(compare) is None + or not is_json_array(files) + or len(files) >= 300 + ): + return False + paths: list[str] = [] + for f in files: + name = json_object(f).get("filename") + if not isinstance(name, str): + return False + paths.append(name) + previous = json_object(f).get("previous_filename") + if isinstance(previous, str): + paths.append(previous) + globs = (*HIGH_FLOOR, *hold_paths, *REVIEW_FLOOR, *CARRY_REFUSES) + return not any(glob_match(g, p) for g in globs for p in paths) + + def approval_carry( then: Any, now: Any, @@ -2705,22 +2751,34 @@ def approval_carry( based: Any, dismissal: str | None = None, trees: Any = None, + hold_paths: Sequence[str] | None = None, ) -> bool: """Whether an approval of `start` carries to the head `now` was read at - (merge-holds.mjs `approvalCarry`): every commit since is a merge of the - base (`base_merges_only`), and the net change is the same - (`same_net_change`) or the merges brought exactly the base's changes - (`exact_base_carry` over `merges`, `based` and `trees`, None when not - read). An approval GitHub dismissed as stale (`auto_dismissed`) passes - its `dismissal` commit, which must be one of the commits pushed since - `start`.""" - if not base_merges_only(then, now, start, in_base): - return False - if dismissal is not None and not any( - json_object(c).get("sha") == dismissal for c in _new_commits(then, now) or () - ): - return False - return same_net_change(then, now) or exact_base_carry(merges, based, trees) + (merge-holds.mjs `approvalCarry`), by either of two paths: every commit + since is a merge of the base (`base_merges_only`), and the net change is + the same (`same_net_change`) or the merges brought exactly the base's + changes (`exact_base_carry` over `merges`, `based` and `trees`, None when + not read); or, the owner's decision Q10 of 2026-10-28, `merges` (the + compare start...H') touches no hold path (`ungated_since`; None + `hold_paths` skips this path). An approval GitHub dismissed as stale + (`auto_dismissed`) passes its `dismissal` commit, which must be one of + the commits pushed since `start`.""" + + def pushed(commits: Any) -> bool: + return dismissal is None or any( + json_object(c).get("sha") == dismissal for c in commits or () + ) + + merged = ( + base_merges_only(then, now, start, in_base) + and pushed(_new_commits(then, now)) + and (same_net_change(then, now) or exact_base_carry(merges, based, trees)) + ) + return merged or ( + hold_paths is not None + and ungated_since(merges, hold_paths) + and pushed(_listed_commits(json_object(merges))) + ) def _review_key(value: Any) -> str | None: @@ -2845,6 +2903,7 @@ def carried_lifters( pr: Mapping[str, Any], reviews: Sequence[Any], humans: Collection[int], + hold_paths: Sequence[str] | None = None, ) -> list[str]: """The trusted people whose approval (`lifts_of` over the REST `reviews`) is of the pull request's head or carries to it over merges of its base @@ -2944,6 +3003,18 @@ def tree(commit: str) -> Any: trees = [tree(sha), tree(end)] if approval_carry(then, now, lifted, in_base, merges, based, dismissal, trees): lifters.add(login) + continue + # Q10's path reads start...H' when the base-merge path did not; a + # read that fails carries nothing by it. + if hold_paths is not None and merges is None: + try: + merges = compare(f"{lifted}...{sha}") + except PolicyReadError: + merges = None + if hold_paths is not None and approval_carry( + then, now, lifted, in_base, merges, based, dismissal, trees, hold_paths + ): + lifters.add(login) except PolicyReadError: return [] return sorted(lifters) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index c39cfbeb5ce..cfb70b8e028 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -2470,7 +2470,9 @@ def lane_risk_refusal( # An approval of an earlier head carries over merges that bring # only the base's changes. lifted = bool(merge_policy.path_lifters(reviews, trusted, head)) or bool( - merge_policy.carried_lifters(repo, pr, reviews, trusted) + merge_policy.carried_lifters( + repo, pr, reviews, trusted, list(config.denied_paths) + ) ) except Exception: # noqa: BLE001 - an unread approval lifts nothing lifted = False diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py index 5a5f19a3ef3..5748e2e1575 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py @@ -574,7 +574,9 @@ def test_the_risk_hold_reads_the_carried_lift_on_its_high_paths(self) -> None: ) as lift: self.assertIsNone(held_by((DOC, FLOORED), reviews=[], humans=TRUSTED)) self.assertEqual(lift.call_args.args[0], REPO) - self.assertEqual(len(lift.call_args.args), 4) + # The configured hold paths, for the owner's decision Q10 carry. + self.assertEqual(len(lift.call_args.args), 5) + self.assertIsInstance(lift.call_args.args[4], list) with mock.patch.object(policy, "carried_lifters", return_value=[]): self.assertIsNotNone(held_by((DOC, FLOORED), reviews=[], humans=TRUSTED)) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index 92744f41ae0..aa86861fa5d 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py @@ -1328,3 +1328,163 @@ def test_both_readers_carry_the_same_lifts(self) -> None: if __name__ == "__main__": unittest.main() + + +# The owner's decision Q10 of 2026-10-28 ("go w/ recommendations"): an +# approval of H0 carries to H1 when the diff H0...H1 touches no hold path and +# no file a hold path loads. Each case: (changed files, configured hold paths, +# compare overrides). +NODE_Q10 = """ +import { CARRY_REFUSES, ungatedSince } from %s; +let text = ""; +for await (const chunk of process.stdin) text += chunk; +const cases = JSON.parse(text); +console.log(JSON.stringify([CARRY_REFUSES, cases.map(([merges, holdPaths]) => ungatedSince(merges, holdPaths))])); +""" +_H1 = "b" * 40 + + +def _pushed(files: list[str], **extra: Any) -> dict[str, Any]: + return { + "status": "ahead", + "behind_by": 0, + "files": [{"filename": f, "status": "modified", "patch": "+x"} for f in files], + "commits": [{"sha": _H1}], + "total_commits": 1, + } | extra + + +_SCRIPTS_DIR = "plugins/source-control/skills/babysit-prs/scripts" +Q10_CASES = { + "a docs-only push": ([_pushed(["docs/a.md", "plugins/work-items/README.md"]), []], True), + "a gated file": ([_pushed([".github/scripts/pr-merge/merge-holds.mjs"]), []], False), + "a test-only edit beside a gated file": ([_pushed([".github/scripts/pr-merge/merge-holds.test.mjs"]), []], False), + "a comment-only edit to a gated file": ([_pushed([f"{_SCRIPTS_DIR}/babysit_merge_policy.py"]), []], False), + "a helper a gated file imports": ([_pushed([f"{_SCRIPTS_DIR}/babysit_util.py"]), []], False), + "a helper babysit_postmerge.py imports": ([_pushed([f"{_SCRIPTS_DIR}/babysit_gh.py"]), []], False), + "a script ci-status runs": ([_pushed(["scripts/check-stale-base-overlap.sh"]), []], False), + "an action a hold-path workflow uses": ([_pushed([".github/actions/report-check-run/report-check-run.mjs"]), []], False), + "a trusted-actor list edit": ([_pushed([".github/standards/trusted-actors/trusted-actors.json"]), []], False), + "a configured hold path": ([_pushed(["docs/conventions/held.md"]), ["docs/conventions/**"]], False), + "a rename off a gated file": ([_pushed(["docs/moved.md"]) | {"files": [{"filename": "docs/moved.md", "status": "renamed", "previous_filename": ".github/actions/request-lane-token/action.yml"}]}, []], False), + "300 files": ([_pushed([f"docs/{i}.md" for i in range(300)]), []], False), + "a cut commit list": ([_pushed(["docs/a.md"], total_commits=251), []], False), + "a diverged head": ([_pushed(["docs/a.md"], status="diverged", behind_by=1), []], False), + "an unread compare": ([None, []], False), + "unread hold paths": ([_pushed(["docs/a.md"]), None], False), +} # fmt: skip + + +@unittest.skipUnless(shutil.which("node"), "node not installed") +class TheUngatedCarryIsTheGates(unittest.TestCase): + def test_both_carry_over_the_same_pushes(self) -> None: + proc = subprocess.run( + ["node", "--input-type=module", "-e", NODE_Q10 % json.dumps(MODULE.as_uri())], + input=json.dumps([args for args, _ in Q10_CASES.values()]), + capture_output=True, + text=True, + check=True, + ) + refuses, answers = json.loads(proc.stdout) + self.assertEqual(tuple(refuses), policy.CARRY_REFUSES) + for (name, ((merges, hold_paths), expected)), js in zip( + Q10_CASES.items(), answers, strict=True + ): + with self.subTest(name): + self.assertEqual(js, expected) + self.assertEqual(policy.ungated_since(merges, hold_paths), expected) + self.assertEqual( + policy.approval_carry( + None, None, _OLD, set(), merges, None, None, None, hold_paths + ), + expected, + ) + # A stale dismissal must be by a commit the push made. + docs = _pushed(["docs/a.md"]) + self.assertTrue(policy.approval_carry(None, None, _OLD, set(), docs, None, _H1, None, [])) + self.assertFalse(policy.approval_carry(None, None, _OLD, set(), docs, None, "c" * 40, None, [])) + # No hold paths read: only the base-merge path decides. + self.assertFalse(policy.approval_carry(None, None, _OLD, set(), docs, None)) + + +# Every local file a hold-path file loads: the floor entries' JS closure and +# the Python it runs, and each hold-path workflow's local actions (every +# file of the action) and the scripts its steps name, with their closures. +USES = re.compile(r"uses:\s*\./(?:\.base/)?(\.github/actions/[A-Za-z0-9_.-]+)") +RUNS = re.compile( + r"(?:\.base/)?((?:\.github/scripts|\.github/actions|scripts|lib)/[A-Za-z0-9_./-]+\.(?:mjs|js|sh|bash|py))" +) + + +def loaded_closure() -> list[str]: + workflows = sorted( + g for g in risk.FLOOR if g.startswith(".github/workflows/") and (ROOT / g).is_file() + ) + files: set[str] = set(workflows) + entries: set[str] = set(FLOOR_ENTRIES) | set(ROUTE_ENTRIES) + for workflow in workflows: + text = (ROOT / workflow).read_text(encoding="utf-8") + for action in USES.findall(text): + # The action's own files; its tests and fixtures run in CI, not + # when a workflow uses it. + for f in (ROOT / action).iterdir(): + if f.is_file() and ".test." not in f.name: + name = f.relative_to(ROOT).as_posix() + files.add(name) + if f.suffix == ".mjs": + entries.add(name) + for script in RUNS.findall(text): + if (ROOT / script).is_file(): + files.add(script) + if script.endswith(".mjs"): + entries.add(script) + # import_closure's walk, where a module that reads a whole directory + # (`new URL("../../workflows/", import.meta.url)`) stands for a file in it. + js: set[str] = set() + todo = [ROOT / e for e in entries if e.endswith(".mjs")] + while todo: + file = todo.pop().resolve() + # A spec that names no file beside the module (a repository-root + # path in a string) loads nothing from here. + if not file.exists(): + continue + name = file.relative_to(ROOT).as_posix() + if name in js: + continue + if file.is_dir(): + js.add(f"{name}/any.yml" if name != "." else "any.yml") + continue + js.add(name) + todo.extend(file.parent / spec for spec in IMPORT.findall(file.read_text(encoding="utf-8"))) + # The Python those modules and the workflows run, as python_closure + # walks it, from the scripts that exist (a module's string can name an + # example path). + run = [ROOT / p for n in sorted(files | js) if n.endswith((".mjs", ".yml")) and (ROOT / n).is_file() for p in PY_RUN.findall((ROOT / n).read_text(encoding="utf-8")) if (ROOT / p).is_file()] + py: set[str] = set() + todo = [SCRIPTS / "babysit_risk.py", *run] + while todo: + file = todo.pop() + name = file.relative_to(ROOT).as_posix() + if name in py: + continue + py.add(name) + for node in ast.walk(ast.parse(file.read_text(encoding="utf-8"))): + names = [a.name for a in node.names] if isinstance(node, ast.Import) else [node.module] if isinstance(node, ast.ImportFrom) and not node.level and node.module else [] + todo.extend(file.parent / f"{n}.py" for n in names if (file.parent / f"{n}.py").is_file()) + return sorted(files | js | py) + + +class TheCarryRefusesWhatHoldPathsLoad(unittest.TestCase): + def test_every_file_a_hold_path_loads_stops_the_carry(self) -> None: + files = loaded_closure() + # The peer review's own example: babysit_postmerge.py's imports. + for name in ("babysit_merge_policy.py", "babysit_gh.py", "babysit_util.py"): + self.assertIn(f"{_SCRIPTS_DIR}/{name}", files) + self.assertIn(".github/workflows/pr-require-checks.yml", files) + for path in files: + with self.subTest(path): + self.assertTrue( + any(risk.glob_match(g, path) for g in (*risk.FLOOR, *policy.CARRY_REFUSES)), + f"{path} is loaded by a hold path but neither held nor in CARRY_REFUSES", + ) + self.assertFalse(policy.ungated_since(_pushed([path]), []), path) From 45cd73e296e60d5fee1b1b243334c96d04dcc184 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:21:50 -0400 Subject: [PATCH 6/7] fix(pr-merge): lift only a needs-human label that stands for the lane's own refusal The lane's approval lift deleted needs-human after looking only at its own newest refusal. The same label also carries holds that live only in it: a pr-unblock escalation adds no labeled event when the label is already on, and a review-lane hold, a signed-commits hold or a person's label sit under it too. liftBlocker now reads the issue timeline, review comments and reviews with gate-source's readers and keeps the label unless its newest labeled event is the lanes App's, pairs with enable-auto-merge's refusal, and nothing else stands behind it since. The head is read again just before the delete. The closure test now matches bare and variable-prefixed Python invocations, which found babysit_merge_prefilter.py, babysit_merge_skip.py and two scripts ci-status runs outside CARRY_REFUSES; both twins now refuse the carry on them. A new test pins HOLD_PATH_FLOOR to HIGH_FLOOR. Co-Authored-By: Claude Opus 5.5 --- ...-kill-switch-deny-ends-neutral-3b7e91c4.md | 11 +- ...-kill-switch-deny-ends-neutral-d729d8dd.md | 6 +- .../scripts/pr-merge/enable-auto-merge.mjs | 104 +++++++++++++++++- .../pr-merge/enable-auto-merge.test.mjs | 74 ++++++++++++- .github/scripts/pr-merge/merge-holds.mjs | 2 +- .../scripts/babysit_merge_policy.py | 3 + .../scripts/tests/test_merge_holds_sync.py | 43 +++++++- 7 files changed, 228 insertions(+), 15 deletions(-) diff --git a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md index b4730fec843..36056e7b3f1 100644 --- a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md +++ b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md @@ -27,14 +27,21 @@ bump: patch approved that head (merge-holds `pathLifters`, or `liftersOf` over base merges), the release the refusal and ci-status already name. Removing the label re-runs ci-status at the head. A cost, risk or verifier reason, a refusal at another head, an approval of another head, a dismissed - approval, an untrusted reviewer or a report-only run lifts nothing. + approval, an untrusted reviewer or a report-only run lifts nothing. The same label also carries + holds that live only in it, so the label stays unless it stands for that refusal alone: the + newest `labeled` event is the lanes App's and pairs with the refusal comment; no review-lane or + check-signed-commits hold stands; the lanes App wrote no pr-unblock escalation or other marked + comment since; no person commented, replied on a review thread, reviewed (an approval aside) or + labeled since; and the head read again just before the delete is the one judged. A failed read + keeps it. It never removes a label a person applied or one another escalation stands behind. - A trusted person's approval of a hold-path head now also carries to a later head when the diff between the two touches no hold path and no file a hold path loads (the owner's decision Q10 of 2026-10-28, #10759), in `merge-holds.mjs` `approvalCarry` and its twin `babysit_merge_policy.approval_carry`. The diff is read with the hold-path rules plus the enforcement floor and `CARRY_REFUSES` (workflows, actions, `.github/scripts`, `.github/standards`, - the pipeline config's directory, `scripts/` and `lib/`). Any edit to such a file, a test-only + the pipeline config's directory, `scripts/`, `lib/`, babysit-prs' scripts and the two plugin + scripts ci-status runs). Any edit to such a file, a test-only or comment-only edit included, needs a new approval, and so do a compare not read whole and a head that is not ahead of the approved one. `test_merge_holds_sync.py` derives every file the hold paths load and fails on one outside the floor and `CARRY_REFUSES`. The control is still a diff --git a/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md b/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md index b0d5613342d..3ba899cc784 100644 --- a/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md +++ b/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md @@ -15,7 +15,9 @@ bump: patch That diff must also touch no file a hold path loads: the enforcement floor, and CARRY_REFUSES for the workflows, actions and scripts hold-path workflows run, the trusted-actor list and the pipeline - config. test_merge_holds_sync.py derives that closure and fails on any - member outside the floor and CARRY_REFUSES. A compare not read whole, a + config, and babysit-prs' own scripts directory. test_merge_holds_sync.py + derives that closure, including scripts a workflow runs by a bare or + variable-prefixed path, and fails on any member outside the floor and + CARRY_REFUSES. A compare not read whole, a head not ahead, a dismissal the push did not make, an untrusted or dismissed approval still carry nothing. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index f76eb32dd05..0bf6f05f22b 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -169,17 +169,31 @@ // // A reader run takes the human-gated label off a pull request whose newest // refusal at its head names only hold paths once a trusted person approved -// that head (approvalLift), which re-runs ci-status there; every other -// person's refusal keeps waiting for a person to remove the label. +// that head (approvalLift), which re-runs ci-status there, but only while the +// label stands for that refusal alone (liftBlocker): never a label a person +// set or one another escalation or hold stands behind. Every other person's +// refusal keeps waiting for a person to remove the label. import { spawnSync } from "node:child_process"; import { appendFileSync, readFileSync } from "node:fs"; import process from "node:process"; import { pathToFileURL } from "node:url"; -import { createGitHub, errorLabel, GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; +import { createGitHub, errorLabel, GitHubError, paginate } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { rateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; +import { + lines as bodyLines, + foreignMarker, + isApp, + labelHistory, + openReviewHold, + openSignedCommitsHold, + personSince, + reviewEntries, + sinceParam, + spoken, +} from "../pr-unblock/gate-source.mjs"; import { readHoldPaths } from "../work-resolve-issue/report-outcome.mjs"; import { BODY_HOLD, @@ -648,6 +662,68 @@ export function approvalLift(pull, detail, { bot, label, holdPaths }) { return costHold(detail.files, detail.costDecision) === null && Array.isArray(detail.lifters) && detail.lifters.length > 0; } +// Whether `label` stands for enable-auto-merge's refusal alone, so the lane +// may take it off: null when it does, else why it stays. The same label also +// carries holds that live only in the label, so it stays unless all hold: +// - the newest `labeled` event is the lanes App's (`login`, the REST login), +// and the App's marked comment written last at or before it is a person's +// refusal (pr-merge's escalation and refusal markers, nothing else); +// - no review-lane or check-signed-commits hold comment stands since the +// label was last removed (gate-source's readers, as clear-escalations reads +// them); +// - no lanes App comment written or edited since the label carries any marker +// but the refusal's: a pr-unblock escalation adds no label event; +// - no person commented, replied on a review thread, reviewed or labeled +// since, a trusted approval (`humans`) aside. +// A time that does not parse counts as since, which keeps the label. +const REFUSAL_LINES = (line) => line === ATTEND_QUEUE_MARKER || line === REFUSAL_MARKER || /^$/.test(line); +export function liftBlocker({ timeline, reviewComments = [], reviews = [], label, login, humans }) { + const { labeled, since, lifted } = labelHistory(timeline, label); + if (!isApp(labeled?.actor, login)) { + return `${label} was last added by ${labeled?.actor?.login ?? "no one on the timeline"}, not the lanes App`; + } + const appComments = timeline.filter((item) => item.event === "commented" && isApp(item.user, login)); + const marked = (item) => bodyLines(item.body).filter((line) => line.includes("", ``, "- Reason: `security-finding`"].join("\n"); + const claude = { login: "claude[bot]", type: "Bot" }; + const actions = { login: "github-actions[bot]", type: "Bot" }; + const cases = { + "a label a person added": [{ timeline: [note, labelEvent(KYLE, "2026-10-23T10:00:01Z"), approval] }, "was last added by kyle-sexton, not the lanes App"], + "a label a pr-unblock escalation set before the refusal": [ + { timeline: [restSaid(APP_USER, escalation, "2026-10-22T09:00:00Z"), labelEvent(APP_USER, "2026-10-22T09:00:01Z"), { ...note, created_at: "2026-10-23T10:00:00Z" }, approval] }, + "does not pair with enable-auto-merge's refusal", + ], + "a pr-unblock escalation in the same second as the refusal": [ + { timeline: [restSaid(APP_USER, escalation, "2026-10-23T10:00:00Z"), note, label, approval] }, + "does not pair with enable-auto-merge's refusal", + ], + "a pr-unblock escalation since the label": [{ timeline: [note, label, restSaid(APP_USER, escalation, after), approval] }, "carries another marker: `, after), approval] }, "a review lane holds it"], + "a review lane's hold the approval came after": [{ timeline: [restSaid(claude, ``, "2026-10-22T09:00:00Z"), note, label, approval] }, "a review lane holds it"], + "check-signed-commits' hold": [{ timeline: [note, label, restSaid(actions, "\nA lane commit is unverified.", after), approval] }, "check-signed-commits holds it"], + "a person's comment since the label": [{ timeline: [note, label, restSaid(KYLE, "Wait for the design call.", after), approval] }, "kyle-sexton spoke or labeled after needs-human went on"], + "a person's label event since the label": [{ timeline: [note, label, labelEvent(KYLE, after, { name: "question" }), approval] }, "kyle-sexton spoke or labeled after needs-human went on"], + "a person's reply on a review thread": [{ threadComments: [{ user: KYLE, created_at: after, updated_at: after }] }, "kyle-sexton spoke or labeled after needs-human went on"], + "a person's review that is not an approval": [{ restReviews: [{ user: KYLE, state: "COMMENTED", submitted_at: after }] }, "kyle-sexton spoke or labeled after needs-human went on"], + "a head that moved before the delete": [{ freshHead: "e".repeat(40) }, `its head moved to ${"e".repeat(40)}`], + "a timeline that does not read": [{ timeline: undefined }, "the label's history did not read"], + }; + for (const [why, [overrides, reason]] of Object.entries(cases)) { + const result = await run([held(overrides)], trustedKyle()); + assert.ok(!result.notes.some(({ method }) => method === "DELETE"), `${why} lifts nothing`); + assert.ok(!result.writes.some(({ mutation }) => mutation === "arm"), `${why} arms nothing`); + assert.ok(result.lines.some((line) => String(line).startsWith("enable-auto-merge: #2 keeps needs-human: ") && String(line).includes(reason)), `${why}: ${result.lines.join("\n")}`); + } + // The approval itself, the refusal edited after the label and a bot's + // comment are not a reason to keep it. + const edited = { ...note, updated_at: after }; + const lifted = await run([held({ timeline: [edited, label, approval, restSaid(claude, "Review done, no findings.", after)], restReviews: [{ user: KYLE, state: "APPROVED", submitted_at: after }] })], trustedKyle()); + assert.deepEqual(lifted.notes, [{ method: "DELETE", path: `${ISSUES}/2/labels/needs-human` }]); +}); diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 74a2677d64f..a6c903fe9d6 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -864,7 +864,7 @@ export function approvalCarry(then, now, from, inBase, merges, based, dismissal // lanes still judge it. babysit_merge_policy.CARRY_REFUSES is the same list. // The trusted-actor list and the pipeline config are read too: a widening the // approver never saw is lifted by the same approval (actorsHold). -export const CARRY_REFUSES = Object.freeze([".github/workflows/**", ".github/actions/**", ".github/scripts/**", ".github/standards/**", "docs/conventions/pr-pipeline/**", "scripts/**", "lib/**"]); +export const CARRY_REFUSES = Object.freeze([".github/workflows/**", ".github/actions/**", ".github/scripts/**", ".github/standards/**", "docs/conventions/pr-pipeline/**", "scripts/**", "lib/**", "plugins/source-control/skills/babysit-prs/scripts/**", "plugins/performance/scripts/**", "plugins/harness-ops/skills/inventory/scripts/**"]); // Whether the compare from...H' `merges` touches no hold path and no file a // hold path loads, old paths of renames included: pathHold with `holdPaths` diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index 12516b25d3c..b8e3206dcd3 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -2709,6 +2709,9 @@ def modes(tree: Any) -> dict[str, str] | None: "docs/conventions/pr-pipeline/**", "scripts/**", "lib/**", + "plugins/source-control/skills/babysit-prs/scripts/**", + "plugins/performance/scripts/**", + "plugins/harness-ops/skills/inventory/scripts/**", ) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index aa86861fa5d..be736fac8cd 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py @@ -114,7 +114,24 @@ def import_closure(entries: tuple[str, ...]) -> list[str]: # JS module runs (enable-auto-merge.mjs's VERIFY_SCRIPT), with every module of # babysit-prs/scripts/ they import. SCRIPTS = pathlib.Path(risk.__file__).resolve().parent -PY_RUN = re.compile(r"""["'`][^"'`\s]*?(plugins/[^"'`\s]+\.py)["'`]""") +# A script path in a quoted string, or a bare one a `python`/`python3` command +# runs, with or without a `.base/` checkout prefix. +PY_RUN = re.compile( + r"""["'`][^"'`\s]*?(plugins/[^"'`\s]+\.py)["'`]""" + r"""|\bpython3?\s+(?:-\S+\s+)*(?:\.base/)?(plugins/[^"'`\s;)]+\.py)""" +) +# A script a shell step names through a variable (`scripts=` then +# `"$scripts/.py"`), resolved from the same file's assignments. +PY_VAR = re.compile(r"""\$\{?([A-Za-z_]\w*)\}?/([A-Za-z0-9_.-]+\.py)""") +PY_DIR = re.compile(r"""^\s*([A-Za-z_]\w*)=["']?(?:\.base/)?(plugins/[^"'\s]+)""", re.M) + + +def py_runs(text: str) -> list[str]: + dirs = dict(PY_DIR.findall(text)) + return [ + *(quoted or bare for quoted, bare in PY_RUN.findall(text)), + *(f"{dirs[var]}/{name}" for var, name in PY_VAR.findall(text) if var in dirs), + ] def python_closure(js_files: list[str]) -> list[str]: @@ -122,8 +139,10 @@ def python_closure(js_files: list[str]) -> list[str]: # A floor entry for a script not in this tree yet runs nothing here. floored = {g for g in risk.FLOOR if g.endswith(".mjs") and (ROOT / g).is_file()} for name in sorted(floored | set(js_files)): - for path in PY_RUN.findall((ROOT / name).read_text(encoding="utf-8")): - todo.append(ROOT / path) + # Quoted paths only: a floored module's strings, not its prose. + for path, _ in PY_RUN.findall((ROOT / name).read_text(encoding="utf-8")): + if path: + todo.append(ROOT / path) seen: set[str] = set() while todo: file = todo.pop() @@ -260,6 +279,22 @@ def test_the_floor_holds_the_same_paths_on_both_sides(self) -> None: self.assertIn(False, answer["configured"]) self.assertNotEqual(answer["held"], answer["configured"]) + def test_the_person_floor_is_the_same_list(self) -> None: + # HOLD_PATH_FLOOR (merge-holds.mjs) is babysit_risk.HIGH_FLOOR, entry + # for entry and in order: the trust and gate globs a person clears. + script = ( + "import { HOLD_PATH_FLOOR } from %s; console.log(JSON.stringify(HOLD_PATH_FLOOR));" + % json.dumps(MODULE.as_uri()) + ) + proc = subprocess.run( + ["node", "--input-type=module", "-e", script], + capture_output=True, + text=True, + check=True, + ) + self.assertEqual(tuple(json.loads(proc.stdout)), risk.HIGH_FLOOR) + self.assertEqual(len(risk.HIGH_FLOOR), 32) + def test_the_floor_holds_what_its_actions_load(self) -> None: files = import_closure(FLOOR_ENTRIES) self.assertIn(".github/actions/check-kill-switch/check-kill-switch.mjs", files) @@ -1459,7 +1494,7 @@ def loaded_closure() -> list[str]: # The Python those modules and the workflows run, as python_closure # walks it, from the scripts that exist (a module's string can name an # example path). - run = [ROOT / p for n in sorted(files | js) if n.endswith((".mjs", ".yml")) and (ROOT / n).is_file() for p in PY_RUN.findall((ROOT / n).read_text(encoding="utf-8")) if (ROOT / p).is_file()] + run = [ROOT / p for n in sorted(files | js) if n.endswith((".mjs", ".yml")) and (ROOT / n).is_file() for p in py_runs((ROOT / n).read_text(encoding="utf-8")) if (ROOT / p).is_file()] py: set[str] = set() todo = [SCRIPTS / "babysit_risk.py", *run] while todo: From 58c861047402c62d16c02c2e31a06ac83a7d4975 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 19:45:56 -0400 Subject: [PATCH 7/7] fix(pr-merge): keep the label under a head hold and a bot's unlabel, and read statuses on the job token liftBlocker now keeps needs-human when a review lane's hold names the head (or does not parse), whenever it was written, counts only a person's removal of the label as lifting earlier holds, and requires the refusal comment within PAIRED_MS of the App's label, so a label put back with no new comment stays for a person. The lanes App holds no commit-statuses permission, so enable-auto-merge reads the head's pr-unblock statuses on the job token and its job gains statuses: read. The closure test now takes every tracked script a hold-path workflow names in any directory; the plugin script directories ci-status runs from join CARRY_REFUSES in both copies. Co-Authored-By: Claude Opus 5.5 --- ...-kill-switch-deny-ends-neutral-3b7e91c4.md | 15 +++--- ...-kill-switch-deny-ends-neutral-d729d8dd.md | 3 +- .../scripts/pr-merge/enable-auto-merge.mjs | 50 ++++++++++++++----- .../pr-merge/enable-auto-merge.test.mjs | 33 ++++++++++-- .github/scripts/pr-merge/merge-holds.mjs | 13 +++-- .github/scripts/pr-merge/workflow.test.mjs | 5 +- .github/scripts/pr-unblock/gate-source.mjs | 2 +- .github/workflows/pr-merge.yml | 1 + .../scripts/babysit_merge_policy.py | 7 +++ .../scripts/tests/test_merge_holds_sync.py | 14 +++++- 10 files changed, 112 insertions(+), 31 deletions(-) diff --git a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md index 36056e7b3f1..abd766a0e66 100644 --- a/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md +++ b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md @@ -20,7 +20,8 @@ bump: patch github-actions[bot] is `failure` or `error`, but only after every lane leg ran, and the queue then ejected it. The rule now lives in `merge-holds.mjs` (`unblockStops`, `readStatuses`): a stopped head, or one whose statuses do not read, is left unarmed (an armed one is disarmed), and each - such skip gets a line in the run summary. + such skip gets a line in the run summary. The statuses are read on the job token, whose job now + holds `statuses: read`, because the lanes App holds no commit-statuses permission. - pr-merge's `enable-auto-merge` takes `needs-human` off a lanes App pull request itself when the lane's newest refusal at the head names only hold paths and a person on the trusted-actor list @@ -28,9 +29,11 @@ bump: patch refusal and ci-status already name. Removing the label re-runs ci-status at the head. A cost, risk or verifier reason, a refusal at another head, an approval of another head, a dismissed approval, an untrusted reviewer or a report-only run lifts nothing. The same label also carries - holds that live only in it, so the label stays unless it stands for that refusal alone: the - newest `labeled` event is the lanes App's and pairs with the refusal comment; no review-lane or - check-signed-commits hold stands; the lanes App wrote no pr-unblock escalation or other marked + holds that live only in it, so the label stays unless it stands for that refusal alone: no + review lane's hold names the head (or fails to parse); the newest `labeled` event is the lanes + App's and comes within a minute of the refusal comment, so a label put back with no new comment + stays; no review-lane or check-signed-commits hold stands since a person last removed the label + (a bot's removal lifts nothing); the lanes App wrote no pr-unblock escalation or other marked comment since; no person commented, replied on a review thread, reviewed (an approval aside) or labeled since; and the head read again just before the delete is the one judged. A failed read keeps it. It never removes a label a person applied or one another escalation stands behind. @@ -40,8 +43,8 @@ bump: patch 2026-10-28, #10759), in `merge-holds.mjs` `approvalCarry` and its twin `babysit_merge_policy.approval_carry`. The diff is read with the hold-path rules plus the enforcement floor and `CARRY_REFUSES` (workflows, actions, `.github/scripts`, `.github/standards`, - the pipeline config's directory, `scripts/`, `lib/`, babysit-prs' scripts and the two plugin - scripts ci-status runs). Any edit to such a file, a test-only + the pipeline config's directory, `scripts/`, `lib/`, babysit-prs' scripts and every plugin + script directory a hold-path workflow runs a script from). Any edit to such a file, a test-only or comment-only edit included, needs a new approval, and so do a compare not read whole and a head that is not ahead of the approved one. `test_merge_holds_sync.py` derives every file the hold paths load and fails on one outside the floor and `CARRY_REFUSES`. The control is still a diff --git a/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md b/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md index 3ba899cc784..900f7f7814c 100644 --- a/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md +++ b/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md @@ -16,7 +16,8 @@ bump: patch floor, and CARRY_REFUSES for the workflows, actions and scripts hold-path workflows run, the trusted-actor list and the pipeline config, and babysit-prs' own scripts directory. test_merge_holds_sync.py - derives that closure, including scripts a workflow runs by a bare or + derives that closure, including every tracked script a hold-path + workflow names in any directory and scripts run by a bare or variable-prefixed path, and fails on any member outside the floor and CARRY_REFUSES. A compare not read whole, a head not ahead, a dismissal the push did not make, an untrusted or diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 12f45849f8a..d9df6810e15 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -185,10 +185,13 @@ import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; import { lines as bodyLines, foreignMarker, + HOLD_LINE, isApp, labelHistory, + laneHold, openReviewHold, openSignedCommitsHold, + PAIRED_MS, personSince, reviewEntries, sinceParam, @@ -665,20 +668,37 @@ export function approvalLift(pull, detail, { bot, label, holdPaths }) { // Whether `label` stands for enable-auto-merge's refusal alone, so the lane // may take it off: null when it does, else why it stays. The same label also // carries holds that live only in the label, so it stays unless all hold: +// - no review lane's hold names the head `head` (or fails to parse); // - the newest `labeled` event is the lanes App's (`login`, the REST login), -// and the App's marked comment written last at or before it is a person's -// refusal (pr-merge's escalation and refusal markers, nothing else); -// - no review-lane or check-signed-commits hold comment stands since the -// label was last removed (gate-source's readers, as clear-escalations reads -// them); +// and the App's marked comment written last at or before it, within +// PAIRED_MS, is a person's refusal (pr-merge's escalation and refusal +// markers); +// - no review-lane or check-signed-commits hold comment stands since a +// person last removed the label (gate-source's readers, as clear-escalations +// reads them); // - no lanes App comment written or edited since the label carries any marker // but the refusal's: a pr-unblock escalation adds no label event; // - no person commented, replied on a review thread, reviewed or labeled // since, a trusted approval (`humans`) aside. // A time that does not parse counts as since, which keeps the label. const REFUSAL_LINES = (line) => line === ATTEND_QUEUE_MARKER || line === REFUSAL_MARKER || /^$/.test(line); -export function liftBlocker({ timeline, reviewComments = [], reviews = [], label, login, humans }) { - const { labeled, since, lifted } = labelHistory(timeline, label); +export function liftBlocker({ timeline, reviewComments = [], reviews = [], label, login, humans, head }) { + const { labeled, since } = labelHistory(timeline, label); + // Only a person's removal lifts a hold (liftedByPerson's rule): a bot's + // unlabel, which the lane then puts back, lifts nothing. + const lifted = Math.max( + -Infinity, + ...timeline + .filter((item) => item.event === "unlabeled" && item.actor?.type === "User" && String(item.label?.name).toLowerCase() === String(label).toLowerCase()) + .map((item) => Date.parse(item.created_at)) + .filter((time) => time <= since), + ); + // A review lane's hold on this head keeps it whenever it was written, and + // one whose marker does not parse keeps it too (gateSource's head check). + const holdMarkers = timeline.map(laneHold).filter((marker) => marker !== undefined); + if (holdMarkers.some((marker) => (HOLD_LINE.exec(marker)?.[1] ?? head) === head)) { + return `a review lane holds head ${head}, or its hold does not parse`; + } if (!isApp(labeled?.actor, login)) { return `${label} was last added by ${labeled?.actor?.login ?? "no one on the timeline"}, not the lanes App`; } @@ -689,7 +709,10 @@ export function liftBlocker({ timeline, reviewComments = [], reviews = [], label .filter(({ markers, at }) => markers.length > 0 && at > -Infinity) .sort((a, b) => a.at - b.at); const paired = before.at(-1); - if (paired === undefined || before.filter(({ at }) => at === paired.at).length > 1 || !paired.markers.includes(REFUSAL_MARKER) || !paired.markers.includes(ATTEND_QUEUE_MARKER)) { + // The refusal comes within PAIRED_MS before the label (refusalWrites posts + // the comment, then the label). A label put back under a standing comment + // with no new one does not pair, so it stays for a person. + if (paired === undefined || before.filter(({ at }) => at === paired.at).length > 1 || !paired.markers.includes(REFUSAL_MARKER) || !paired.markers.includes(ATTEND_QUEUE_MARKER) || !(since - paired.at <= PAIRED_MS)) { return `${label} does not pair with enable-auto-merge's refusal`; } if (openReviewHold(timeline, lifted) !== undefined) { @@ -1263,8 +1286,9 @@ export async function main({ env = process.env, github, actions, verify, room, s latch ??= rateLimitLatch; github = withRetry(github ?? createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined, onResponse, latch }), { sleep, random }); // The lane runs, jobs and annotations a cost decision's decided head is - // read from, on the job token (actions: read, checks: read), as the - // verifier reads take it; the lanes App token holds neither. + // read from, and the head's pr-unblock statuses, on the job token + // (actions: read, checks: read, statuses: read), as the verifier reads + // take it; the lanes App token holds none of them. actions = actions ?? (!injected && env.BABYSIT_VERIFY_GH_TOKEN @@ -1460,7 +1484,9 @@ export async function main({ env = process.env, github, actions, verify, room, s } let stops = null; try { - stops = unblockStops(await readStatuses(github, `${repo[1]}/${repo[2]}`, pull.headRefOid)); + // On the job token (statuses: read): the lanes App holds no + // commit-statuses permission. + stops = unblockStops(await readStatuses(actions, `${repo[1]}/${repo[2]}`, pull.headRefOid)); } catch (error) { console.log(`enable-auto-merge: #${pull.number} head statuses did not read: ${describe(error)}`); } @@ -1476,7 +1502,7 @@ export async function main({ env = process.env, github, actions, verify, room, s if (lift) { let kept; try { - kept = await liftBlocked(github, repo[0], pull.number, { label: humanGated, login: env.LANES_BOT_LOGIN, humans }); + kept = await liftBlocked(github, repo[0], pull.number, { label: humanGated, login: env.LANES_BOT_LOGIN, humans, head: pull.headRefOid }); if (kept === null && write) { const fresh = await github("GET", `/repos/${repo[0]}/pulls/${pull.number}`); kept = fresh?.head?.sha === pull.headRefOid ? null : `its head moved to ${fresh?.head?.sha ?? "an unread head"}`; diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 251da58da2f..7290ec8c942 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -306,7 +306,8 @@ async function run(pulls, env = {}, options = {}, lane = ROOM) { const log = console.log; console.log = (line) => lines.push(line); try { - const code = await main({ env: { ...ENV, GITHUB_OUTPUT: output, ...env }, github, verify, room: room(lane), sleep: async (ms) => slept.push(ms), random: options.random, now: options.now ?? (() => Date.parse(NOW)) }); + const actions = options.jobToken ? options.jobToken(github) : undefined; + const code = await main({ env: { ...ENV, GITHUB_OUTPUT: output, ...env }, github, actions, verify, room: room(lane), sleep: async (ms) => slept.push(ms), random: options.random, now: options.now ?? (() => Date.parse(NOW)) }); // The `pr-merge/unarmed` statuses the run hands the status job. const line = readFileSync(output, "utf8").split("\n").find((text) => text.startsWith(`${UNARMED_OUTPUT}=`)); const statuses = line === undefined ? [] : JSON.parse(line.slice(UNARMED_OUTPUT.length + 1)); @@ -3028,6 +3029,15 @@ test("a head a pr-unblock lane stopped is not queued and is named in the summary assert.equal((await run([pull(1, { verdict: PASSED })])).writes[0]?.mutation, "arm"); }); +// The lanes App holds no commit-statuses permission, so the head's statuses +// are read on the job token (statuses: read): a lanes token that cannot read +// them stops nothing when the job token can. +test("the head's statuses are read on the job token, not the lanes App token", async () => { + const jobToken = (github) => async (method, path, ...rest) => (path.includes("/statuses?") ? [] : github(method, path, ...rest)); + const { writes } = await run([pull(1, { verdict: PASSED, statuses: "error" })], {}, { jobToken }); + assert.equal(writes[0]?.mutation, "arm"); +}); + test("a head whose statuses do not read is not queued, and an armed one is disarmed", async () => { assert.deepEqual((await run([pull(1, { verdict: PASSED, statuses: "error" })])).writes, []); const armed = pull(1, { verdict: PASSED, statuses: "error", autoMergeRequest: { enabledAt: NOW } }); @@ -3076,6 +3086,7 @@ test("an approved hold-path escalation keeps its label while anything else stand const held = (overrides) => pull(2, { files: HELD, labels: labels("needs-human"), verdict: PASSED, discussion: { reviews: [reviewed("APPROVED")], comments: [lanesSaid(personNote(HOLD_PATH_REASON))] }, timeline: liftTimeline(), ...overrides }); const [note, label, approval] = liftTimeline(); const after = "2026-10-24T09:00:00Z"; + const OLD = "e".repeat(40); const escalation = ["", ``, "- Reason: `security-finding`"].join("\n"); const claude = { login: "claude[bot]", type: "Bot" }; const actions = { login: "github-actions[bot]", type: "Bot" }; @@ -3090,8 +3101,21 @@ test("an approved hold-path escalation keeps its label while anything else stand "does not pair with enable-auto-merge's refusal", ], "a pr-unblock escalation since the label": [{ timeline: [note, label, restSaid(APP_USER, escalation, after), approval] }, "carries another marker: `, after), approval] }, "a review lane holds it"], - "a review lane's hold the approval came after": [{ timeline: [restSaid(claude, ``, "2026-10-22T09:00:00Z"), note, label, approval] }, "a review lane holds it"], + "a review lane's hold on an earlier head": [{ timeline: [note, label, restSaid(claude, ``, after), approval] }, "a review lane holds it"], + "a review lane's hold the approval came after": [{ timeline: [restSaid(claude, ``, "2026-10-22T09:00:00Z"), note, label, approval] }, "a review lane holds it"], + // The second security review's probe: a hold on this head, a bot's + // unlabel, the lane putting the label back, then the approval. + "a hold on the head under a bot's unlabel and the lane's relabel": [ + { timeline: [note, label, restSaid(claude, ``, after), labelEvent(actions, "2026-10-24T09:01:00Z", { event: "unlabeled" }), labelEvent(APP_USER, "2026-10-24T09:02:00Z"), approval] }, + `a review lane holds head ${HEAD}`, + ], + "a hold whose marker does not parse": [{ timeline: [note, label, restSaid(claude, "", after), approval] }, "or its hold does not parse"], + // A bot's unlabel lifts no earlier hold, even under a newly paired label. + "an earlier head's hold under a bot's unlabel": [ + { timeline: [restSaid(APP_USER, personNote(HOLD_PATH_REASON), "2026-10-22T08:00:00Z"), labelEvent(APP_USER, "2026-10-22T08:00:01Z"), restSaid(claude, ``, "2026-10-22T09:00:00Z"), labelEvent(actions, "2026-10-22T09:30:00Z", { event: "unlabeled" }), note, label, approval] }, + "a review lane holds it", + ], + "a refusal posted longer than PAIRED_MS before the label": [{ timeline: [note, labelEvent(APP_USER, "2026-10-23T10:01:01Z"), approval] }, "does not pair with enable-auto-merge's refusal"], "check-signed-commits' hold": [{ timeline: [note, label, restSaid(actions, "\nA lane commit is unverified.", after), approval] }, "check-signed-commits holds it"], "a person's comment since the label": [{ timeline: [note, label, restSaid(KYLE, "Wait for the design call.", after), approval] }, "kyle-sexton spoke or labeled after needs-human went on"], "a person's label event since the label": [{ timeline: [note, label, labelEvent(KYLE, after, { name: "question" }), approval] }, "kyle-sexton spoke or labeled after needs-human went on"], @@ -3108,6 +3132,9 @@ test("an approved hold-path escalation keeps its label while anything else stand } // The approval itself, the refusal edited after the label and a bot's // comment are not a reason to keep it. + // A person who removed the label lifted an earlier head's hold under it. + const personLifted = await run([held({ timeline: [restSaid(claude, ``, "2026-10-22T09:00:00Z"), labelEvent(KYLE, "2026-10-22T09:30:00Z", { event: "unlabeled" }), note, label, approval] })], trustedKyle()); + assert.deepEqual(personLifted.notes, [{ method: "DELETE", path: `${ISSUES}/2/labels/needs-human` }]); const edited = { ...note, updated_at: after }; const lifted = await run([held({ timeline: [edited, label, approval, restSaid(claude, "Review done, no findings.", after)], restReviews: [{ user: KYLE, state: "APPROVED", submitted_at: after }] })], trustedKyle()); assert.deepEqual(lifted.notes, [{ method: "DELETE", path: `${ISSUES}/2/labels/needs-human` }]); diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index a6c903fe9d6..1c5e33ea4ae 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -855,16 +855,19 @@ export function approvalCarry(then, now, from, inBase, merges, based, dismissal return merged || (holdPaths !== undefined && ungatedSince(merges, holdPaths) && pushed(listedCommits(merges))); } -// The code a hold-path file loads that is no hold path itself: every action, -// script and workflow a hold-path workflow runs or imports, the lint and test -// scripts ci-status runs included (test_merge_holds_sync.py derives that -// closure and fails on a member these globs and HOLD_PATH_CLASSES miss). +// The code a hold-path file loads that is no hold path itself: every file of +// each local action a hold-path workflow uses, every tracked script it names +// by path in any directory (from the head checkout or `.base/`, the lint and +// test scripts ci-status runs included), and the JS and Python those import +// (test_merge_holds_sync.py derives that closure and fails on a member these +// globs and HOLD_PATH_CLASSES miss; a shell script's own `source` is not +// walked). // Holding them for a person would hold most pull requests, so a push that // touches one only stops an approval carrying (ungatedSince); the review // lanes still judge it. babysit_merge_policy.CARRY_REFUSES is the same list. // The trusted-actor list and the pipeline config are read too: a widening the // approver never saw is lifted by the same approval (actorsHold). -export const CARRY_REFUSES = Object.freeze([".github/workflows/**", ".github/actions/**", ".github/scripts/**", ".github/standards/**", "docs/conventions/pr-pipeline/**", "scripts/**", "lib/**", "plugins/source-control/skills/babysit-prs/scripts/**", "plugins/performance/scripts/**", "plugins/harness-ops/skills/inventory/scripts/**"]); +export const CARRY_REFUSES = Object.freeze([".github/workflows/**", ".github/actions/**", ".github/scripts/**", ".github/standards/**", "docs/conventions/pr-pipeline/**", "scripts/**", "lib/**", "plugins/source-control/skills/babysit-prs/scripts/**", "plugins/performance/scripts/**", "plugins/harness-ops/skills/inventory/scripts/**", "plugins/ai-briefing/skills/generate/scripts/**", "plugins/harness-config/skills/audit-permission-state/scripts/**", "plugins/knowledge/skills/course-digest/scripts/**", "plugins/knowledge/skills/video-digest/scripts/**", "plugins/skill-quality/scripts/**", "plugins/source-control/scripts/**", "plugins/testing/skills/check-visual-parity/scripts/**"]); // Whether the compare from...H' `merges` touches no hold path and no file a // hold path loads, old paths of renames included: pathHold with `holdPaths` diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 993a2f49e4a..309f18d8a1f 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -780,7 +780,10 @@ test("enable-auto-merge asks the broker for its own activity's merge token", () const block = /\n {4}permissions:\n((?: {6}[a-z-]+: [a-z]+.*\n)+)/.exec(arm)[1]; assert.deepEqual( [...block.matchAll(/ {6}([a-z-]+): ([a-z]+)/g)].map((m) => `${m[1]}: ${m[2]}`), - ["contents: read", "actions: read", "pull-requests: read", "checks: read", "issues: read", "id-token: write"], + // statuses: read for the head's pr-unblock statuses: GitHub lists that + // endpoint under the "Commit statuses" read permission, which the lanes + // App does not hold. + ["contents: read", "actions: read", "pull-requests: read", "checks: read", "issues: read", "statuses: read", "id-token: write"], ); // The verifier evidence is read as the gate reads it: the job's // GITHUB_TOKEN for the Actions reads, with the run's head-ref map and diff --git a/.github/scripts/pr-unblock/gate-source.mjs b/.github/scripts/pr-unblock/gate-source.mjs index bb948279e77..d8c4cadcca8 100644 --- a/.github/scripts/pr-unblock/gate-source.mjs +++ b/.github/scripts/pr-unblock/gate-source.mjs @@ -38,7 +38,7 @@ export const CLEARED = /^$/; // A review lane's hold comment opens a line with this marker. const REVIEW_HOLD = "$/; +export const HOLD_LINE = /^$/; const LANE_FOOTER = /^`?claude-lane: (?:claude-review|claude-security-review)`?$/; // check-signed-commits' escalation comment opens with this line; its report // job posts it with GITHUB_TOKEN, as github-actions[bot]. diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index bfcce3d84ac..97b4e4af39c 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1900,6 +1900,7 @@ jobs: pull-requests: read # the lane's merges and the merge queue, for the lane's room checks: read # the review lanes' annotations, as the gate binds their evidence issues: read # the demotion records the lane's room re-checks + statuses: read # the head's pr-unblock statuses (the lanes App holds no statuses permission) id-token: write # prove to the token broker which workflow this is # The pr-merge/unarmed statuses for the unarmed-status job to post. outputs: diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index b8e3206dcd3..26714eceb02 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -2712,6 +2712,13 @@ def modes(tree: Any) -> dict[str, str] | None: "plugins/source-control/skills/babysit-prs/scripts/**", "plugins/performance/scripts/**", "plugins/harness-ops/skills/inventory/scripts/**", + "plugins/ai-briefing/skills/generate/scripts/**", + "plugins/harness-config/skills/audit-permission-state/scripts/**", + "plugins/knowledge/skills/course-digest/scripts/**", + "plugins/knowledge/skills/video-digest/scripts/**", + "plugins/skill-quality/scripts/**", + "plugins/source-control/scripts/**", + "plugins/testing/skills/check-visual-parity/scripts/**", ) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index be736fac8cd..7b1084c7ec2 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py @@ -1446,17 +1446,27 @@ def test_both_carry_over_the_same_pushes(self) -> None: # the Python it runs, and each hold-path workflow's local actions (every # file of the action) and the scripts its steps name, with their closures. USES = re.compile(r"uses:\s*\./(?:\.base/)?(\.github/actions/[A-Za-z0-9_.-]+)") +# Any repository path to a script a workflow names, from the head checkout or +# `.base/`, whatever its directory: only a tracked file counts. RUNS = re.compile( - r"(?:\.base/)?((?:\.github/scripts|\.github/actions|scripts|lib)/[A-Za-z0-9_./-]+\.(?:mjs|js|sh|bash|py))" + r"(? set[str]: + proc = subprocess.run( + ["git", "-C", str(ROOT), "ls-files"], capture_output=True, text=True, check=True + ) + return set(proc.stdout.splitlines()) + + def loaded_closure() -> list[str]: workflows = sorted( g for g in risk.FLOOR if g.startswith(".github/workflows/") and (ROOT / g).is_file() ) files: set[str] = set(workflows) entries: set[str] = set(FLOOR_ENTRIES) | set(ROUTE_ENTRIES) + in_git = tracked() for workflow in workflows: text = (ROOT / workflow).read_text(encoding="utf-8") for action in USES.findall(text): @@ -1469,7 +1479,7 @@ def loaded_closure() -> list[str]: if f.suffix == ".mjs": entries.add(name) for script in RUNS.findall(text): - if (ROOT / script).is_file(): + if script in in_git: files.add(script) if script.endswith(".mjs"): entries.add(script)