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..abd766a0e66 --- /dev/null +++ b/.changes/pr-pipeline/fix-kill-switch-deny-ends-neutral-3b7e91c4.md @@ -0,0 +1,51 @@ +--- +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. + +- 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. 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 + 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. The same label also carries + 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. + +- 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/`, `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 + 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..900f7f7814c --- /dev/null +++ b/.changes/source-control/fix-kill-switch-deny-ends-neutral-d729d8dd.md @@ -0,0 +1,24 @@ +--- +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, and babysit-prs' own scripts directory. test_merge_holds_sync.py + 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 + dismissed approval still carry nothing. 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/actions/resolve-config/callers.test.mjs b/.github/actions/resolve-config/callers.test.mjs index 81d05515b46..96c73e5b683 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. @@ -6850,3 +6853,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/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 900380333ca..d9df6810e15 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -166,16 +166,52 @@ // 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, 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, + HOLD_LINE, + isApp, + labelHistory, + laneHold, + openReviewHold, + openSignedCommitsHold, + PAIRED_MS, + personSince, + reviewEntries, + sinceParam, + spoken, +} from "../pr-unblock/gate-source.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"; @@ -603,6 +639,114 @@ 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; +} + +// 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, 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, 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`; + } + 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"], + // 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"], + "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. + // 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 69dd8ed1022..1c5e33ea4ae 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -838,17 +838,52 @@ 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 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/**", "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` +// 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 +975,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 +1018,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 { @@ -1309,6 +1353,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) { @@ -1365,7 +1452,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/.github/scripts/pr-merge/merge-job.test.mjs b/.github/scripts/pr-merge/merge-job.test.mjs index 5a81d33b6e9..a5a46fb765a 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. 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/intake-triage.yml b/.github/workflows/intake-triage.yml index d77fc497c81..b34e463036b 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: @@ -1336,6 +1343,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..97b4e4af39c 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 @@ -1248,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 @@ -1282,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 }} @@ -1305,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 }} @@ -1354,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 @@ -1897,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: @@ -2012,6 +2016,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 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..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 @@ -61,6 +61,8 @@ ) from babysit_risk import ( FLOOR, + HIGH_FLOOR, + REVIEW_FLOOR, ChangedFile, glob_match, is_trusted_actors_path, @@ -2696,6 +2698,60 @@ 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/**", + "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/**", +) + + +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 +2761,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 +2913,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 +3013,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..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 @@ -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) @@ -1328,3 +1363,173 @@ 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_.-]+)") +# 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"(? 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): + # 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 script in in_git: + 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_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: + 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)