diff --git a/.changes/pr-pipeline/fix-arm-on-head-approval-6a1f3c92.md b/.changes/pr-pipeline/fix-arm-on-head-approval-6a1f3c92.md new file mode 100644 index 0000000000..a37603116d --- /dev/null +++ b/.changes/pr-pipeline/fix-arm-on-head-approval-6a1f3c92.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- enable-auto-merge no longer holds a lanes App pull request on a hold path once a person on the trusted-actor list approved its current head, the same `pathLifters` rule `ci-status` lifts that hold by; its other checks then decide. An approval of an older head, an untrusted reviewer's approval, or a newer request for changes lifts nothing, and a hold-path list or file listing that could not be read, or one too long to check, still holds (`.github/scripts/pr-merge/enable-auto-merge.mjs`). The gate's risk score lifts the same paths' `high-path:` reasons on that approval, so such a pull request now arms end to end (source-control's `path_lifters`). diff --git a/.changes/source-control/fix-arm-on-head-approval-3d7e9b14.md b/.changes/source-control/fix-arm-on-head-approval-3d7e9b14.md new file mode 100644 index 0000000000..b9b983e8f2 --- /dev/null +++ b/.changes/source-control/fix-arm-on-head-approval-3d7e9b14.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- The pr-merge lane's risk score (`babysit_verify_evidence.py evidence --lane-reviews`) drops a configured or floor high path (`high-path:`) once a person on the trusted-actor list approved the pull request's current head, by the same rule `ci-status` lifts the hold path with (`babysit_merge_policy.path_lifters`, held equal to `merge-holds.mjs` `pathLifters`). An approval of an older head, an untrusted approver, a newer request for changes or a dismissal lifts nothing, and so does a review list or trusted-actor list that does not read. An aliasing path, symlink, submodule or mode change, and every cost, trust-gate, manifest, file-cap and review-lane hold, still hold. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 6adf5bdf50..a55fd53ae2 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -24,8 +24,10 @@ // wrote its Hold: line can add one. The rule is merge-holds.pathHold, which // ci-status also runs on every lanes App pull request event and merge // group, so a push after arming is held without waiting for this job. A -// config that does not read arms nothing and labels nothing; the next run -// reads it again. +// trusted person's approving review of the head lifts it, as ci-status +// lifts it (merge-holds.pathLifters); it lifts no listing or check that +// could not run. A config that does not read arms nothing and labels +// nothing; the next run reads it again. // - Its head changes no cost path, unless its body links the owner's cost // decision: merge-holds.costHold and costDecision, which ci-status runs too. // - No person turned its auto-merge off: the newest auto-merge-disabled event @@ -139,7 +141,7 @@ // label (default .base/.work-item-tracker.json) // RESOLVED_CONFIG the resolve-config output, for the hold paths // TRUSTED_ACTORS the base trusted-actors.json, for the owner of a cost -// decision (default .base/.github/standards/trusted-actors/trusted-actors.json) +// decision and the approvers who lift a hold path (default .base/.github/standards/trusted-actors/trusted-actors.json) // EVENT_NAME the run's event; only READER_EVENTS read verdicts and arm // POSTMERGE_RESULT this run's babysit_postmerge.py report, for lane-room // POSTMERGE_CODE_SHA the commit whose code ran that report's check (empty @@ -168,7 +170,7 @@ 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, pathHoldKind, personWrote, vetoers } from "./merge-holds.mjs"; +import { BODY_HOLD, costDecision, costHold, humansOf, LANE_HOLD, pathHoldKind, pathLifters, personWrote, vetoers } from "./merge-holds.mjs"; export { BODY_HOLD, HUMAN_VETO } from "./merge-holds.mjs"; @@ -204,7 +206,7 @@ export const PULLS_QUERY = `query($owner: String!, $name: String!, $after: Strin // One candidate's discussion: the newest 100 of each, and wholeDiscussion // reads the rest of any list that has more. const COMMENT_FIELDS = "databaseId author { __typename login } body createdAt updatedAt lastEditedAt authorAssociation"; -const REVIEW_FIELDS = "author { __typename login } body submittedAt authorAssociation"; +const REVIEW_FIELDS = "author { __typename login ... on User { databaseId } } state commit { oid } body submittedAt authorAssociation"; const THREAD_COMMENT_FIELDS = "author { __typename login } body createdAt authorAssociation"; const THREAD_FIELDS = `id comments(last: 100) { totalCount nodes { ${THREAD_COMMENT_FIELDS} } }`; export const DETAIL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { @@ -403,19 +405,29 @@ export async function wholeDiscussion(github, variables, detail) { return { ...read, reviewThreads: { ...read.reviewThreads, nodes: threads } }; } +// One DETAIL_QUERY comment or review as a merge-holds item (fromRest's shape). +const itemOf = (item) => ({ + login: item?.author?.login, + userId: item?.author?.databaseId, + state: item?.state, + commitId: item?.commit?.oid, + isUser: item?.author?.__typename === "User", + association: item?.authorAssociation, + at: String(item?.submittedAt ?? item?.createdAt ?? ""), + body: String(item?.body ?? ""), +}); + +// The trusted people (`humans`) whose newest deciding review of `head` in +// `detail` approves it, as ci-status reads them (merge-holds.pathLifters). +const headLifters = (detail, humans, head) => pathLifters((detail?.reviews?.nodes ?? []).map(itemOf), humans, head); + // Why the discussion `detail` (DETAIL_QUERY's pullRequest) holds it, or null. export function discussionHold(detail) { const threads = detail?.reviewThreads; if (![detail?.comments, detail?.reviews, threads].every(whole) || !threads.nodes.every((thread) => whole(thread?.comments))) { return "its comments could not all be read"; } - const items = [...detail.comments.nodes, ...detail.reviews.nodes, ...threads.nodes.flatMap((thread) => thread.comments.nodes)].map((item) => ({ - login: item?.author?.login, - isUser: item?.author?.__typename === "User", - association: item?.authorAssociation, - at: String(item?.submittedAt ?? item?.createdAt ?? ""), - body: String(item?.body ?? ""), - })); + const items = [...detail.comments.nodes, ...detail.reviews.nodes, ...threads.nodes.flatMap((thread) => thread.comments.nodes)].map(itemOf); const vetoes = vetoers(items); return vetoes.length ? `${vetoes.join(", ")} asked not to merge it` : null; } @@ -948,10 +960,11 @@ export function decide(pull, { bot, humanGated, holdPaths = null }, detail) { if (detail.filesCapped === true) { return { action: armed ? "disarm" : "none", reason: FILES_CAPPED_REASON, clears: "person" }; } - // A hold path a person clears; a listing or check that could not run is - // a lane's, which the next run reads again. + // A hold path a person clears, lifted by a trusted approval of the head + // (`lifters`) as ci-status lifts it; a listing or check that could not run + // is a lane's, which the next run reads again. const pathHeld = pathHoldKind(detail.files, holdPaths); - if (pathHeld !== null) { + if (pathHeld !== null && !(pathHeld.kind === "hold-path" && detail.lifters?.length > 0)) { return { action: armed ? "disarm" : "none", reason: pathHeld.reason, clears: pathHeld.kind === "hold-path" ? "person" : "lane" }; } const costHeld = costHold(detail.files, detail.costDecision); @@ -1187,8 +1200,9 @@ export async function main({ env = process.env, github, verify, room, sleep = (m if (detail !== null) { detail = await wholeDiscussion(github, variables, detail); const { paths: files, capped } = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); - const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, trustedHumans(env), pull.headRefOid) : null; - detail = { ...detail, files, filesCapped: capped, costDecision: decision }; + const humans = trustedHumans(env); + const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, humans, pull.headRefOid) : null; + detail = { ...detail, files, filesCapped: capped, costDecision: decision, lifters: headLifters(detail, humans, pull.headRefOid) }; } } 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 2a1ad59afd..6db87849bc 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -1173,6 +1173,51 @@ test("a pull request whose head now changes a hold path is not armed, or is disa ]); }); +// ci-status lifts a hold path on a trusted person's approving review of the +// head (merge-holds.pathLifters); arming reads the same rule. Kyle (id 5) is +// on the trusted-actor list, id 9 is not. +const reviewed = (state, { id = 5, login = "kyle-sexton", oid = HEAD, at = "2026-10-26T16:41:00Z" } = {}) => ({ + author: { __typename: "User", login, databaseId: id }, + state, + commit: { oid }, + body: "", + submittedAt: at, + authorAssociation: "OWNER", +}); +function trustedKyle() { + const dir = mkdtempSync(join(tmpdir(), "enable-auto-merge-")); + writeFileSync(join(dir, "actors.json"), JSON.stringify({ actors: [{ kind: "human", login: "kyle-sexton", id: 5 }] })); + return { TRUSTED_ACTORS: join(dir, "actors.json") }; +} +const HELD = [{ filename: "docs/held.md" }]; + +test("a hold path a trusted person approved at the head is armed, and an armed one stays armed", async () => { + const approved = { reviews: [reviewed("APPROVED")] }; + const { writes } = await run([pull(50, { files: HELD, discussion: approved }), pull(51, { files: HELD, discussion: approved, autoMergeRequest: {} })], trustedKyle()); + assert.deepEqual(writes, [{ mutation: "arm", id: "PR_50", head: HEAD }]); +}); + +test("a hold path stays held without a trusted approval of the head", async () => { + const { writes } = await run( + [ + pull(52, { files: HELD, discussion: { reviews: [reviewed("APPROVED", { oid: "c".repeat(40) })] } }), + pull(53, { files: HELD, discussion: { reviews: [reviewed("APPROVED", { id: 9, login: "someone" })] } }), + pull(54, { files: HELD, discussion: { reviews: [reviewed("APPROVED"), reviewed("CHANGES_REQUESTED", { at: "2026-10-26T17:20:00Z" })] } }), + pull(55, { files: HELD, discussion: { reviews: [reviewed("APPROVED", { id: 9, login: "someone" })] }, autoMergeRequest: {} }), + ], + trustedKyle(), + ); + assert.deepEqual(writes, [{ mutation: "disarm", id: "PR_55" }]); +}); + +test("an approval lifts no unread hold-path check and no capped file listing", () => { + const options = { bot: LANES.login, humanGated: "needs-human", holdPaths: [] }; + const armed = pull(1, { autoMergeRequest: {} }); + const approved = { ...detail(), lifters: ["kyle-sexton"], verify: PASSED }; + assert.deepEqual(decide(armed, options, { ...approved, files: null }), { action: "disarm", reason: "its changed files could not all be read", clears: "lane" }); + assert.deepEqual(decide(armed, options, { ...approved, files: ["docs/a.md"], filesCapped: true }), { action: "disarm", reason: FILES_CAPPED_REASON, clears: "person" }); +}); + // A manifest is no hold path: the gate's reader scores it (`evidence // --lane-reviews`, babysit_risk manifest-review-required), so a manifest // change the gate reviews is not armed, or is disarmed, and one it does not, @@ -1740,7 +1785,7 @@ def passes(self, number, head, pr=None): argv = sys.argv[1:] pr = lambda repo, number: {"number": number, "head": {"sha": argv[-1]}} -with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "evidence", passes), mock.patch.object(policy, "pull_request", pr), mock.patch.object(ve, "lane_risk_refusal", lambda *args: None), mock.patch.object(policy, "read_config_at", lambda repo, sha: policy.PipelineConfig(sha, "C2", "pr-merge", "agent[bot]", None, (), "c2-mechanical")), mock.patch.object(policy, "with_default_branch", lambda repo, base, *rest: base), mock.patch.object(babysit_merge, "read_merge_queue", lambda repo, number: {"readError": None, "failedEjections": 0}): +with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "evidence", passes), mock.patch.object(policy, "pull_request", pr), mock.patch.object(ve, "lane_risk_refusal", lambda *args, **kwargs: None), mock.patch.object(policy, "read_config_at", lambda repo, sha: policy.PipelineConfig(sha, "C2", "pr-merge", "agent[bot]", None, (), "c2-mechanical")), mock.patch.object(policy, "with_default_branch", lambda repo, base, *rest: base), mock.patch.object(babysit_merge, "read_merge_queue", lambda repo, number: {"readError": None, "failedEjections": 0}): sys.exit(ve.main(argv)) `, ); @@ -1825,7 +1870,7 @@ def passes(self, number, head, pr=None): argv = sys.argv[1:] pr = lambda repo, number: {"number": number, "head": {"sha": argv[-1]}} -with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "_read_listing", listing), mock.patch.object(ve.Reader, "_evidence", passes), mock.patch.object(policy, "pull_request", pr), mock.patch.object(ve, "lane_risk_refusal", lambda *args: None), mock.patch.object(policy, "read_config_at", lambda repo, sha: policy.PipelineConfig(sha, "C2", "pr-merge", "agent[bot]", None, (), "c2-mechanical")), mock.patch.object(policy, "with_default_branch", lambda repo, base, *rest: base), mock.patch.object(babysit_merge, "read_merge_queue", lambda repo, number: {"readError": None, "failedEjections": 0}): +with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "_read_listing", listing), mock.patch.object(ve.Reader, "_evidence", passes), mock.patch.object(policy, "pull_request", pr), mock.patch.object(ve, "lane_risk_refusal", lambda *args, **kwargs: None), mock.patch.object(policy, "read_config_at", lambda repo, sha: policy.PipelineConfig(sha, "C2", "pr-merge", "agent[bot]", None, (), "c2-mechanical")), mock.patch.object(policy, "with_default_branch", lambda repo, base, *rest: base), mock.patch.object(babysit_merge, "read_merge_queue", lambda repo, number: {"readError": None, "failedEjections": 0}): sys.exit(ve.main(argv)) `, ); @@ -2408,7 +2453,7 @@ with mock.patch.object(ve.Reader, "link_refusal", lambda self, n: None), \\ mock.patch.object(policy, "read_config_at", lambda repo, sha: config), \\ mock.patch.object(policy, "with_default_branch", lambda repo, base, *rest: base), \\ mock.patch.object(policy, "pull_request", lambda repo, n: pr), \\ - mock.patch.object(ve, "lane_risk_refusal", lambda *args: None), \\ + mock.patch.object(ve, "lane_risk_refusal", lambda *args, **kwargs: None), \\ mock.patch.object(babysit_merge, "read_merge_queue", lambda repo, n: {"readError": None, "failedEjections": COUNTS[str(n)]}), \\ mock.patch.object(babysit_merge, "_failed_merge_group_runs", lambda repo, branch, n: []): sys.exit(ve.main(sys.argv[1:])) diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 89bcafae0c..43353a5533 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1174,7 +1174,7 @@ collaborator) whose newest comment, review or inline comment with text says "do gate's veto rule; a later comment lifts it), and whose head changes no path in the lane's `merge.diff-check.denied-paths` or on the [path floor](#merge-authority) (`merge-holds.mjs` `HOLD_PATH_FLOOR`, the floor's `trust` and `gate` globs, equal to `babysit_risk.HIGH_FLOOR`), read on the files the head changes at each -run, and whose +run, unless a person on the trusted-actor list approved that head as `ci-status` reads it, and whose issue link and verifier verdict at that head pr-merge's gate would take: `babysit_verify_evidence.py evidence --lane-link` refuses nothing, so the link is one the gate's [eligibility](#the-pr-merge-lane) rule takes (one closing issue in this repository, or, with none, @@ -1318,7 +1318,14 @@ request event and in the merge queue, where a red group removes the pull request adds a hold path or such a widening fails `ci-status` on its own head. A person on the trusted-actor list (as main has it) lifts either by approving that head: their newest review of it that approves, requests changes or was dismissed must approve. Then re-run `ci-status`, since a review does not. The approval lifts no -body hold or veto, and arming still skips the pull request, so that person merges it. +body hold or veto. `enable-auto-merge` reads the same approval (`pathLifters`) and stops holding +the pull request on the hold path, and the gate's risk score (`evidence --lane-reviews`, +`lane_risk_refusal`) drops its `high-path:` reasons for a path on `babysit_risk.HIGH_FLOOR` or in +`merge.diff-check.denied-paths` on the same approval (`babysit_merge_policy.path_lifters`, which +`test_merge_holds_sync.py` holds equal to `pathLifters`), so its other checks decide. The approval +lifts no aliasing path, symlink, submodule or mode change (`high-path:**`), cost, trust-gate, +manifest, file-cap or review-lane hold, and a review list or trusted-actor list that does not read +lifts nothing. Both also hold a pull request that changes one of the merge gate's cost paths (`babysit_risk.COST_PATHS`: Pulumi, Terraform and Bicep files). An approval does not lift that hold. Only the owner's cost decision does, read as the gate's 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 38102c451d..a35f3571a1 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 @@ -2509,6 +2509,34 @@ def _denied_path_refusal( return f"denied-path:{','.join(hits)}" if hits else None +def path_lifters( + reviews: Sequence[Mapping[str, Any]], humans: Collection[int], head: str +) -> list[str]: + """The logins of the trusted people (`humans` ids) whose newest deciding + review (approved, changes requested or dismissed) of `head` among the REST + `reviews` approves it, sorted: merge-holds.mjs `pathLifters` over + `fromRest`, which test_merge_holds_sync.py holds equal.""" + newest: dict[int, tuple[str, Mapping[str, Any]]] = {} + for row in reviews: + user = json_object(row.get("user")) + uid = user.get("id") + if user.get("type") != "User" or type(uid) is not int or uid not in humans: + continue + if row.get("commit_id") != head: + continue + if row.get("state") not in ("APPROVED", "CHANGES_REQUESTED", "DISMISSED"): + continue + at = row.get("submitted_at") + at = str((row.get("created_at") if at is None else at) or "") + if uid not in newest or at >= newest[uid][0]: + newest[uid] = (at, row) + return sorted( + str(json_object(row.get("user")).get("login") or "") + for _, row in newest.values() + if row.get("state") == "APPROVED" + ) + + AUTHOR_NOT_OWNER = "author-not-owner" # `trusted_actors` by (repo, ref), read once a run. _TRUSTED_IDS: dict[tuple[str, ...], tuple[tuple[int, ...], tuple[int, ...]]] = {} 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 f008e2f237..20a9d67c5f 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 @@ -144,6 +144,7 @@ import babysit_run_cache as run_cache from babysit_classify import is_dependency_author from babysit_gh import ( + fetch_paginated_api, flatten_paginated_items, gh_http_status, gh_timeout_seconds, @@ -1896,6 +1897,8 @@ def lane_risk_refusal( pr: dict[str, Any], config: merge_policy.PipelineConfig, issue: int | None, + *, + humans: Callable[[], frozenset[int]] = frozenset, ) -> tuple[str, bool] | None: """Why pr-merge's gate would hold the pull request on its risk score, and whether only a person clears it (`babysit_risk.person_reasons`), or @@ -1906,7 +1909,11 @@ def lane_risk_refusal( or other non-regular mode, a high path, the manifest review, the trust gate and the cost review all hold, the cost review lifted only by the owner's decision on the pull request or its linked `issue` - (`merge_policy.cost_decision`). A read that fails holds.""" + (`merge_policy.cost_decision`). A read that fails holds. A configured or + floor high path is lifted, as ci-status lifts it, by a trusted person's + approval of the head (`merge_policy.path_lifters` over the pull request's + reviews and `humans`, the trusted list's people); an aliasing path keeps + its hold, and a review or trusted list that does not read lifts nothing.""" # Imported here: babysit_merge imports this module. import babysit_merge # noqa: PLC0415 import babysit_risk # noqa: PLC0415 @@ -1953,6 +1960,23 @@ def lane_risk_refusal( except Exception as exc: # noqa: BLE001 - any doubt holds, as the gate's return f"unreadable: {type(exc).__name__}: {exc}", False held = babysit_risk.holding(risk) + paths = [ + r + for r in held + if r.code.startswith("high-path:") and r.code != babysit_risk.UNSAFE_PATH_CODE + ] + if paths: + head = str(json_object(pr.get("head")).get("sha") or "") + try: + reviews = fetch_paginated_api( + f"repos/{repo}/pulls/{number}/reviews?per_page=100", + f"{repo}#{number} reviews", + ) + lifted = bool(merge_policy.path_lifters(reviews, humans(), head)) + except Exception: # noqa: BLE001 - an unread approval lifts nothing + lifted = False + if lifted: + held = [r for r in held if r not in paths] text = "; ".join(f"{r.code}: {r.detail}" for r in held) return (text, bool(babysit_risk.person_reasons(held))) if text else None @@ -2766,6 +2790,7 @@ def _evidence_report(args: argparse.Namespace, reader: Reader) -> dict[str, Any] json_object(pr), config, reader.linked_issue.get(args.number), + humans=reader._trusted_humans, ) ) link["reviews"], link["reviewsPerson"] = held or (None, 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 2bb74f7e4f..2fe7579f8c 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 @@ -101,10 +101,19 @@ def held_by( files: tuple[ChangedFile, ...] = (DOC,), evidence: object = None, config: policy.PipelineConfig = CONFIG, + reviews: object = (), + humans: frozenset[int] = frozenset(), ) -> tuple[str, bool] | None: evidence = lanes() if evidence is None else evidence effect = evidence if isinstance(evidence, Exception) else None + read = reviews if isinstance(reviews, Exception) else None with ( + mock.patch.object( + ve, + "fetch_paginated_api", + side_effect=read, + return_value=None if read else list(reviews), + ), mock.patch.object(policy, "changed_files", return_value=(files, True)), mock.patch.object(policy, "with_contents", side_effect=lambda repo, pr, f: f), mock.patch.object(policy, "pulumi_dirs_at", return_value=()), @@ -117,7 +126,68 @@ def held_by( return_value=None if effect else (evidence, []), ), ): - return ve.lane_risk_refusal(REPO, 41, PR, config, 7) + return ve.lane_risk_refusal(REPO, 41, PR, config, 7, humans=lambda: humans) + + +# A path on the floor (babysit_risk.HIGH_FLOOR), held as `high-path:` until a +# trusted person approves the head, as ci-status lifts it (merge-holds.mjs +# pathLifters). Kyle (id 5) is trusted, id 9 is not. +FLOORED = dataclasses.replace(DOC, path=".github/actions/resolve-config/resolve.mjs") +COST = dataclasses.replace(DOC, path="infra/main.tf") +TRUSTED = frozenset({5}) + + +def review( + state: str, *, user: int = 5, sha: str = HEAD, at: str = "2026-10-26T16:41:00Z" +) -> dict[str, object]: + return { + "user": {"id": user, "login": f"user{user}", "type": "User"}, + "state": state, + "commit_id": sha, + "submitted_at": at, + } + + +class ATrustedApprovalLiftsAHighPath(unittest.TestCase): + def test_a_floor_path_approved_at_the_head_holds_nothing(self) -> None: + self.assertIsNone( + held_by((DOC, FLOORED), reviews=[review("APPROVED")], humans=TRUSTED) + ) + + def test_a_floor_path_holds_for_a_person_without_that_approval(self) -> None: + later = "2026-10-26T17:20:00Z" + cases = { + "no review": [], + "an older head": [review("APPROVED", sha="c" * 40)], + "an untrusted approver": [review("APPROVED", user=9)], + "changes requested since": [ + review("APPROVED"), + review("CHANGES_REQUESTED", at=later), + ], + "the review list did not read": RuntimeError("reviews: HTTP 502"), + } + for name, reviews in cases.items(): + with self.subTest(name): + held = held_by((DOC, FLOORED), reviews=reviews, humans=TRUSTED) + self.assertEqual( + held, + ( + "high-path:.github/actions/resolve-config/**: " + ".github/actions/resolve-config/resolve.mjs", + True, + ), + ) + # The trusted list that did not read lifts nothing either. + self.assertIsNotNone(held_by((DOC, FLOORED), reviews=[review("APPROVED")])) + + def test_the_approval_lifts_no_unsafe_path_or_cost_hold(self) -> None: + approved = [review("APPROVED")] + unsafe = held_by((FLOORED, SYMLINK), reviews=approved, humans=TRUSTED) + cost = held_by((FLOORED, COST), reviews=approved, humans=TRUSTED) + self.assertEqual(unsafe and unsafe[0].split(":", 2)[:2], ["high-path", "**"]) + self.assertNotIn("resolve-config", str(unsafe)) + self.assertEqual(str(cost and cost[0]).split(":")[0], "cost-review-required") + self.assertNotIn("high-path", str(cost)) class LaneRiskRefusal(unittest.TestCase): 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 6dc6dd0052..127cdc8f71 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 @@ -28,6 +28,9 @@ pull request as `#` (`namesPull`, `names_pull`), and both carry it to the same heads: the decided head, or a later one with the same net change against the base branch (`costMarker`, `sameNetChange`). +- The high-path lift: `pathLifters` over `fromRest` and + `babysit_merge_policy.path_lifters` name the same trusted approvers of the + head on the same REST review lists. """ from __future__ import annotations @@ -409,6 +412,70 @@ def test_both_hold_the_same_trusted_actor_list_changes(self) -> None: self.assertIn(False, held) +NODE_LIFTERS = """ +import { fromRest, pathLifters } from %s; +const [cases, humans, head] = JSON.parse(process.argv[1]); +console.log(JSON.stringify(cases.map((rows) => pathLifters(rows.map(fromRest), new Set(humans), head)))); +""" +_HEAD = "a" * 40 + + +def _review( + state: str, + user: int = 1, + sha: str = _HEAD, + at: str = "2026-10-26T16:41:00Z", + kind: str = "User", +) -> dict[str, Any]: + return { + "user": {"id": user, "login": f"u{user}", "type": kind}, + "state": state, + "commit_id": sha, + "submitted_at": at, + } + + +_LATER = "2026-10-26T17:20:00Z" +# REST review lists, kyle (id 1) and id 3 trusted, at head _HEAD. +LIFTER_CASES = { + "approved at the head": [_review("APPROVED")], + "approved at another head": [_review("APPROVED", sha="b" * 40)], + "an untrusted approver": [_review("APPROVED", user=2)], + "a bot account": [_review("APPROVED", kind="Bot")], + "changes requested since": [_review("APPROVED"), _review("CHANGES_REQUESTED", at=_LATER)], + "dismissed since": [_review("APPROVED"), _review("DISMISSED", at=_LATER)], + "approved after changes requested": [_review("CHANGES_REQUESTED"), _review("APPROVED", at=_LATER)], + "a comment after the approval": [_review("APPROVED"), _review("COMMENTED", at=_LATER)], + "two people, one blocks": [_review("APPROVED", user=3), _review("APPROVED"), _review("CHANGES_REQUESTED", at=_LATER)], + "no reviews": [], +} # fmt: skip + + +@unittest.skipUnless(shutil.which("node"), "node not installed") +class TheHighPathLiftIsTheGates(unittest.TestCase): + def test_both_name_the_same_lifters(self) -> None: + script = NODE_LIFTERS % json.dumps(MODULE.as_uri()) + proc = subprocess.run( + [ + "node", + "--input-type=module", + "-e", + script, + json.dumps([list(LIFTER_CASES.values()), [1, 3], _HEAD]), + ], + capture_output=True, + text=True, + check=True, + ) + answers = json.loads(proc.stdout) + for (name, rows), js in zip(LIFTER_CASES.items(), answers, strict=True): + with self.subTest(name): + self.assertEqual(policy.path_lifters(rows, {1, 3}, _HEAD), js) + self.assertIn([], answers) + self.assertIn(["u1"], answers) + self.assertIn(["u3"], answers) + + REPO = "o/r" HEAD = "0123456789abcdef0123456789abcdef01234567" BASE = "89abcdef0123456789abcdef0123456789abcdef"