From d775a4d86c3753bee1fd37b3ec364949f6e64b25 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 05:25:46 -0400 Subject: [PATCH 01/11] feat(source-control): babysit autopilot merge tier, gate-enforced and disabled by default Add the #476 autopilot merge tier to babysit-prs: a second bot account (author != approver) runs a genuine review pass and submits an approving review only when clean, after which the pinned merge gate merges only when every criterion holds. Criteria are enforced deterministically in babysit_merge.py behind the fail-closed --autopilot-merge-tier umbrella flag: - required checks green incl. the review workflow (mergeStateStatus CLEAN; ruleset never bypassed) and head pinned by --expected-head as always; - issue-linked (a closing-issue reference); - authored by a configured pipeline lane (--lane-logins); - no human CHANGES_REQUESTED / blocking comment / unresolved review thread; - no configured do-not-merge label (--block-labels); - a distinct-bot approving review on the live head (--approver-bot-logins; author != approver, head SHA unchanged since review). Any criterion failing falls back to today's behavior: the PR is reported on the human merge-ready list. The umbrella flag refuses (exit 3) unless all three parameter sets are supplied, and every criterion predicate is reused from the shared babysit_classify module rather than re-implemented. Absent the flag the merge gate is byte-for-byte its prior self, so worker/autopilot's existing gate-proven merges are unchanged. The tier exists only while the new babysit_autopilot_merge_tier userConfig (boolean, default off) is enabled; enabling it and any later gate-off flip is a separate, announced operator step. safety.md's "Never do automatically: merge" contract is updated deliberately to codify the tier and its criteria. Tests: new test_babysit_merge.py covers each criterion with a passing and a fall-back fixture plus the tier-absent no-network invariant; guard and skill-contract tests cover the fail-closed CLI and the tier prose. Co-authored-by: Claude Opus 4.8 --- .../source-control/.claude-plugin/plugin.json | 26 +- plugins/source-control/CHANGELOG.md | 24 ++ .../skills/babysit-prs/SKILL.md | 40 +++ .../skills/babysit-prs/reference/safety.md | 32 +- .../babysit-prs/scripts/babysit_merge.py | 298 +++++++++++++++++- .../skills/babysit-prs/scripts/engine.test.sh | 4 + .../scripts/tests/test_babysit_merge.py | 239 ++++++++++++++ .../babysit-prs/scripts/tests/test_guards.py | 32 ++ .../scripts/tests/test_skill_contract.py | 25 ++ 9 files changed, 716 insertions(+), 4 deletions(-) create mode 100644 plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index dce220504..6e88ee49d 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "source-control", - "version": "0.13.1", + "version": "0.14.0", "description": "Git and GitHub delivery workflow: /commit (Conventional Commits + Co-Authored-By trailer via safe heredoc mechanics), /pull-request (prep, create, CI monitoring, review-comment triage, merge, CI-log fetch), /babysit-prs (self-pacing fleet loop — safe by default; opt-in worker/autopilot tiers add gate-checked merge and thread resolution behind a deterministic Python engine), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention and babysit-prs config, or apply — interview the repo and write the tracked convention config), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep — never --abort). The commit-subject / PR-title convention is configurable per repo via a tracked .claude/source-control.md config written by a re-runnable setup skill; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", @@ -51,6 +51,30 @@ "title": "Babysit merge method", "description": "Merge method for gate-proven merges: merge, squash, or rebase. Absent: repo convention, then squash." }, + "babysit_autopilot_merge_tier": { + "type": "boolean", + "title": "Babysit autopilot merge tier", + "description": "Enable the #476 autopilot merge tier: a distinct bot account submits a genuine approving review, then the gate merges only when every criterion holds (issue-linked, lane-authored, no do-not-merge label, distinct-bot approval on the live head, no human blocking comment). Ships DISABLED; a deliberate operator opt-in. Requires babysit_lane_logins, babysit_approver_bot_logins, and babysit_merge_block_labels to be set. Absent/false: the tier does not exist and PRs go to the human merge-ready list.", + "default": false + }, + "babysit_lane_logins": { + "type": "string", + "multiple": true, + "title": "Babysit pipeline lane logins", + "description": "Author logins recognized as pipeline lanes for the autopilot merge tier's lane-authored criterion. Absent: the tier (when enabled) refuses fail-closed." + }, + "babysit_approver_bot_logins": { + "type": "string", + "multiple": true, + "title": "Babysit approver bot logins", + "description": "Bot logins whose approving review satisfies the autopilot merge tier's author != approver criterion. Absent: the tier (when enabled) refuses fail-closed." + }, + "babysit_merge_block_labels": { + "type": "string", + "multiple": true, + "title": "Babysit merge block labels", + "description": "Labels that veto an autopilot-merge-tier merge, e.g. do-not-merge. Absent: the tier (when enabled) refuses fail-closed." + }, "babysit_review_trigger_phrase": { "type": "string", "title": "Babysit review trigger phrase", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 40b051a80..2bd8e03c4 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,30 @@ All notable changes to the `source-control` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.14.0] + +### Added + +- **`babysit-prs` autopilot merge tier (#476), shipped DISABLED behind an explicit operator + flag.** At day-scale throughput, human approve-and-merge is the pipeline bottleneck. The new + tier lets the fleet satisfy the branch ruleset instead of bypassing it: a second bot account + (author ≠ approver) runs a genuine review pass through the review plugin and submits an + approving review only when clean, after which the pinned merge gate merges **only when every + criterion holds** — required checks green including the review workflow (`mergeStateStatus` + CLEAN, ruleset untouched), issue-linked, authored by a configured pipeline lane, no human + `CHANGES_REQUESTED` / blocking comment / unresolved thread, no configured do-not-merge label, + and a distinct-bot approval on the live head (head SHA unchanged since review). Any criterion + failing falls back to today's behavior: the PR is reported on the human merge-ready list. The + gate flag `--autopilot-merge-tier` is **fail-closed** — it refuses unless `--lane-logins`, + `--approver-bot-logins`, and `--block-labels` are all supplied — and every criterion predicate + is reused from the shared `babysit_classify` module rather than re-implemented. The tier exists + only while `babysit_autopilot_merge_tier` is enabled (new boolean userConfig, default off); + enabling it and any later gate-off flip is a separate, announced operator step. New userConfig: + `babysit_autopilot_merge_tier`, `babysit_lane_logins`, `babysit_approver_bot_logins`, + `babysit_merge_block_labels`. Absent the flag the merge gate is byte-for-byte its prior self, so + worker/autopilot's existing gate-proven merges are unchanged. `safety.md`'s "Never do + automatically: merge" contract is updated deliberately to codify the tier and its criteria. + ## [0.13.1] ### Fixed diff --git a/plugins/source-control/skills/babysit-prs/SKILL.md b/plugins/source-control/skills/babysit-prs/SKILL.md index 88eb6fb30..c089a2d9b 100644 --- a/plugins/source-control/skills/babysit-prs/SKILL.md +++ b/plugins/source-control/skills/babysit-prs/SKILL.md @@ -176,6 +176,42 @@ it does **not** widen the owner allowlist, and it does not gain force-push, `--a settings powers — those still escalate. Run it looped: `/loop 15m /source-control:babysit-prs autopilot`. +## Autopilot merge tier (#476) + +A deliberate, config-gated escalation of autopilot's merge authority for a pipeline running at +day-scale, where human approve-and-merge is the throughput bottleneck. It **ships DISABLED** and +exists only while the operator sets `babysit_autopilot_merge_tier`; enabling that flag, and any +later gate-off flip, is a separate, loudly-announced operator step, never a default. Without the +flag every merge decision is exactly today's — the tier's whole surface is dormant and PRs that +are otherwise ready are reported on the human merge-ready list. + +Without the tier, autopilot can merge a PR only once *something else* has produced the approving +review the base ruleset requires; the tier lets the fleet produce that approval itself, safely. +When enabled, per candidate PR autopilot: + +1. Runs a **genuine review pass** through the review plugin under a **second bot account** whose + login is one of `babysit_approver_bot_logins` (author ≠ approver), and submits an **approving + review only when that review is clean**. This is a real review, never a rubber stamp; a review + that finds anything blocking is posted as findings and the PR is not merged. + +2. After the approval lands on the current head, runs the pinned merge gate with the tier flags — + `source-control-babysit-merge owner/repo#N --allowed-owners --self-logins + @me, --merge --expected-head --autopilot-merge-tier + --lane-logins --approver-bot-logins --block-labels + `. The gate merges **only when every criterion holds**, each enforced + deterministically: required checks green including the review workflow (`mergeStateStatus` + CLEAN, ruleset never bypassed); issue-linked (a closing-issue reference); authored by a + configured pipeline lane; no human `CHANGES_REQUESTED` / blocking comment / unresolved thread; + no configured do-not-merge label; and a distinct-bot approval (author ≠ approver) submitted + against the live head (head SHA unchanged since review, pinned by `--expected-head`). + +Any criterion failing falls back to reporting the PR on the human merge-ready list — the tier +never routes around the gate. The gate is **fail-closed**: `--autopilot-merge-tier` refuses (exit +`3`) unless all three of `--lane-logins`, `--approver-bot-logins`, and `--block-labels` are +non-empty, so an under-configured tier merges nothing. The criteria predicates are reused from the +shared classifier (`babysit_classify`), not re-implemented. The ruleset itself stays unchanged; +the bot review is what makes it a genuine gate rather than a bypass. + ## Guarded mutations: deterministic gates, agent judgment The two mutation gates are invoked ONLY by their bare wrapper names (they resolve their own @@ -288,6 +324,10 @@ tier authority. | `babysit_intended_write_identity` | `${user_config.babysit_intended_write_identity}` | `--intended-write-identity` (snapshot) | attribution-drift check dormant | | `babysit_default_tier` | `${user_config.babysit_default_tier}` | prose only — tier of explicit bare invocations | `safe` | | `babysit_merge_method` | `${user_config.babysit_merge_method}` | `--method` (merge wrapper) | repo convention, then squash | +| `babysit_autopilot_merge_tier` | `${user_config.babysit_autopilot_merge_tier}` | prose only — gates whether the tier's `--autopilot-merge-tier` merge flags are wired at all | `false` (tier disabled; PRs go to the human merge-ready list) | +| `babysit_lane_logins` | `${user_config.babysit_lane_logins}` | `--lane-logins` (merge wrapper, autopilot merge tier) | tier refuses fail-closed when enabled | +| `babysit_approver_bot_logins` | `${user_config.babysit_approver_bot_logins}` | `--approver-bot-logins` (merge wrapper, autopilot merge tier) | tier refuses fail-closed when enabled | +| `babysit_merge_block_labels` | `${user_config.babysit_merge_block_labels}` | `--block-labels` (merge wrapper, autopilot merge tier) | tier refuses fail-closed when enabled | | `babysit_review_trigger_phrase` | `${user_config.babysit_review_trigger_phrase}` | `--trigger-phrase` (snapshot, request_review) | review-trigger module dormant | | `babysit_review_bot_logins` | `${user_config.babysit_review_bot_logins}` | `--review-bot-logins` (snapshot, request_review) | review-trigger module dormant | | `babysit_review_gate_context` | `${user_config.babysit_review_gate_context}` | `--review-gate-context` (snapshot) | gate treated as absent | diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index c8f6a52a8..a32b4d925 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -134,6 +134,13 @@ auto-mode safety classifier and blocks the call before the wrapper runs. to merge on an unprotected repository — zero required reviews AND zero required status contexts — when the PR author is not one of ``, absent `--allow-unprotected`. Both overrides are human decisions, never passed autonomously. +- The merge wrapper's `--autopilot-merge-tier` flag layers the #476 tier criteria (issue-linked, + lane-authored, no blocking label, a distinct-bot approval on the live head, no human blocking + comment) onto the base gate. It is **fail-closed**: the umbrella flag refuses (exit `3`) unless + `--lane-logins`, `--approver-bot-logins`, and `--block-labels` are all non-empty, and supplying + any of those three without the umbrella is a usage error (exit `2`). Absent the flag the gate is + exactly its prior self, so worker/autopilot's existing gate-proven merges are unchanged. This + tier is only ever wired when `babysit_autopilot_merge_tier` is enabled. - The resolve wrapper's mutating forms are `--autonomous --resolve` (worker tier, constrained by the pre-push-outdated rule in `orchestration.md`) and `--resolve --include-human` (autopilot's addressed-thread widening). @@ -221,7 +228,30 @@ as done and re-running the gate. ## Never Do Automatically - Merge in default (safe) mode, or merge through any path other than the pinned merge wrapper's - gate. + gate. Worker and autopilot merge only a PR that gate proves 100% ready. +- Generate an approving review to satisfy a required-review ruleset, or merge on a review the + fleet produced itself — **except** under the autopilot merge tier (#476), a deliberate, + config-gated opt-in that ships **DISABLED**. It engages only when the operator sets + `babysit_autopilot_merge_tier`; enabling that flag, and any later gate-off flip, is a + separate, loudly-announced operator step, never a default and never a side effect of another + change. When the tier is enabled, a second bot account (author ≠ approver) runs a **genuine** + review pass and submits an approving review **only when it is clean**, and the pinned merge + wrapper's `--autopilot-merge-tier` gate then merges **only when every criterion holds**, each + enforced deterministically: + - required checks green, including the review workflow, with the base ruleset satisfied + (`mergeStateStatus` CLEAN — the ruleset itself is never bypassed); + - the PR is issue-linked (carries a closing-issue reference); + - the PR is authored by a configured pipeline lane; + - no human `CHANGES_REQUESTED`, no human blocking comment, no unresolved review thread; + - no configured do-not-merge label is present; + - the approving review is by a **distinct bot identity** (author ≠ approver) and was + submitted against the **live head** (head SHA unchanged since review), pinned as always by + `--expected-head`. + + Any criterion failing falls back to today's behavior — the PR is reported on the human + merge-ready list. The tier never routes around the gate and never rubber-stamps: the bot + review is a real review pass, and the ruleset stays meaningful. Absent the enable flag this + tier does not exist and the first bullet governs unchanged. - Enable auto-merge. - Force-push. - Rebase or force-update a PR branch as freshness maintenance. diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 3316df338..969eb0ebd 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -20,6 +20,14 @@ - A PR on an unprotected base (zero required reviews AND zero required contexts) authored by someone other than a configured self login is held unless `--allow-unprotected` is passed: on such a base `CLEAN` proves nothing. +- The #476 autopilot merge tier (`--autopilot-merge-tier`) layers five extra + criteria on top of the base gate -- issue-linked, lane-authored, no blocking + label, a distinct-bot approving review on the live head (author != approver via + bot identity, unchanged since review), and no human blocking comment. It is + fail-closed: the umbrella flag refuses to run unless `--lane-logins`, + `--approver-bot-logins`, and `--block-labels` are all non-empty. Any criterion + failing is just another blocker, so the caller falls back to the human + merge-ready list. Absent the flag the gate is byte-for-byte its prior self. Readiness is gated on GitHub's own `mergeStateStatus == CLEAN` (which integrates required checks, up-to-date, approvals, and conversation resolution) plus @@ -37,11 +45,23 @@ import argparse import json import re +from dataclasses import dataclass from typing import Any, cast from babysit_checks import check_identity_key, classify_checks -from babysit_classify import is_dependency_author, is_self_login, normalize_self_logins +from babysit_classify import ( + actor_kind, + has_blocking_severity, + has_blocking_text, + is_bot, + is_dependency_author, + is_self_login, + normalize_login_set, + normalize_self_logins, +) from babysit_gh import ( + fetch_issue_comments, + fetch_pull_request_reviews, fetch_review_threads, gh_capture, gh_json, @@ -58,6 +78,31 @@ READY_MERGE_STATES = {"CLEAN", "HAS_HOOKS"} +@dataclass(frozen=True) +class AutopilotMergeTierConfig: + """The extra criteria the #476 autopilot merge tier gates on, over the base + readiness gate that every tier already shares. + + Present only when the caller passes `--autopilot-merge-tier`; absent (None) + the gate behaves exactly as it always has, so worker/autopilot's existing + gate-proven merges are unchanged. Every field is caller-supplied and the + umbrella flag refuses to run with any of the three required sets empty, so + the tier is fail-closed: it can never merge without knowing which authors are + pipeline lanes, which login the distinct bot approver posts under, and which + labels veto a merge. + """ + + lane_logins: frozenset[str] + approver_bot_logins: frozenset[str] + block_labels: frozenset[str] + + +def parse_csv_set(raw: str | None) -> set[str]: + if not raw: + return set() + return {part.strip() for part in raw.split(",") if part.strip()} + + def split_owner(repo: str) -> str: return repo.split("/", 1)[0] @@ -145,6 +190,155 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: return summary +def find_distinct_bot_approval( + reviews: list[dict[str, Any]], + author_login: str | None, + head: str | None, + approver_bot_logins: frozenset[str], +) -> dict[str, Any] | None: + """The most recent APPROVED review by a distinct bot identity on the live head. + + Enforces two #476 criteria at once: author != approver (via bot identity) and + head SHA unchanged since review. An approval is eligible only when its author + is a bot (structural `[bot]`/`Bot` type, or a caller-named approver login), is + not the PR author (normalized login compare), and was submitted against the + exact live head commit — a stale approval left on a since-superseded commit is + not "unchanged since review". Reviews arrive oldest-first; the last eligible + one wins so a re-approval on the current head is honored. + """ + author_norm = normalize_login_set([author_login] if author_login else []) + match: dict[str, Any] | None = None + for review in reviews: + if str(review.get("state") or "") != "APPROVED": + continue + review_author = review.get("author") + login = ( + review_author.get("login") + if is_json_object(review_author) + else review_author + ) + typename = ( + review_author.get("__typename") if is_json_object(review_author) else None + ) + if normalize_login_set([login]) & author_norm: + continue # same identity as the PR author -- not a distinct approver + if not is_bot(login, typename, approver_bot_logins): + continue + commit = review.get("commit") + commit_oid = commit.get("oid") if is_json_object(commit) else None + if not (head and commit_oid and str(commit_oid) == str(head)): + continue # approval is on a superseded commit -- head moved since review + match = review + return match + + +def evaluate_autopilot_tier( + repo: str, + number: int, + head: str | None, + author_login: str | None, + labels: list[Any], + closing_issues: list[Any], + tier: AutopilotMergeTierConfig, +) -> tuple[list[str], dict[str, Any]]: + """Evaluate the #476 tier criteria that ride on top of the base gate. + + Returns the tier's own blockers plus a self-documenting per-criterion record. + Every predicate is imported from the shared classifier (`babysit_classify`) so + the tier never re-implements authorship, bot, or blocking-text detection. Any + criterion failing simply adds a blocker; the caller falls back to reporting the + PR on the human merge-ready list, never routing around the gate. + """ + blockers: list[str] = [] + + issue_linked = bool(closing_issues) + if not issue_linked: + blockers.append( + "not issue-linked -- no closing-issue reference (autopilot merge tier)" + ) + + label_names = {str(name).casefold() for name in labels if name} + blocking_labels = sorted( + label + for label in tier.block_labels + if label.casefold() in label_names + ) + if blocking_labels: + blockers.append( + "blocked by label(s) " + ", ".join(repr(b) for b in blocking_labels) + ) + + lane_authored = bool( + normalize_login_set([author_login] if author_login else []) + & normalize_login_set(tier.lane_logins) + ) + if not lane_authored: + blockers.append( + f"author {author_login!r} is not a configured pipeline lane " + "(autopilot merge tier requires a lane-authored PR)" + ) + + reviews = fetch_pull_request_reviews(repo, number) + approval = find_distinct_bot_approval( + reviews, author_login, head, tier.approver_bot_logins + ) + if approval is None: + blockers.append( + "no distinct-bot approving review on the live head " + "(need author != approver via bot identity, approval unchanged since head)" + ) + + # A human "do not merge"/blocking comment that is not a formal + # CHANGES_REQUESTED and not an unresolved inline thread (both already gated + # above) still halts the tier. Reuse the shared blocking-text/severity + # predicates over every human-authored issue comment and review summary. + human_blocking: list[str] = [] + corpus: list[dict[str, Any]] = list(fetch_issue_comments(repo, number)) + corpus.extend(reviews) + for item in corpus: + if actor_kind(item) != "human": + continue + body = str(item.get("body") or "") + if has_blocking_text(body) or has_blocking_severity(body): + login = item.get("author") + login = login.get("login") if is_json_object(login) else login + human_blocking.append(str(login or "unknown")) + if human_blocking: + blockers.append( + "human blocking comment(s) from " + + ", ".join(sorted(set(human_blocking))) + + " -- resolve before an autopilot merge" + ) + + tier_result = { + "enabled": True, + "issueLinked": issue_linked, + "closingIssues": [ + c.get("number") if is_json_object(c) else c for c in closing_issues + ], + "laneAuthored": lane_authored, + "blockingLabels": blocking_labels, + "distinctBotApproval": ( + { + "author": ( + approval.get("author", {}).get("login") + if is_json_object(approval.get("author")) + else None + ), + "commit": ( + approval.get("commit", {}).get("oid") + if is_json_object(approval.get("commit")) + else None + ), + } + if approval + else None + ), + "humanBlockingComments": sorted(set(human_blocking)), + } + return blockers, tier_result + + def evaluate( repo: str, number: int, @@ -153,6 +347,7 @@ def evaluate( self_logins: frozenset[str], allow_dependency: bool, allow_unprotected: bool, + tier: AutopilotMergeTierConfig | None = None, ) -> dict[str, Any]: owner = split_owner(repo) pr_data = gh_json( @@ -164,7 +359,8 @@ def evaluate( repo, "--json", "state,isDraft,mergeable,mergeStateStatus,reviewDecision," - "headRefOid,baseRefName,author,url,title,labels,statusCheckRollup", + "headRefOid,baseRefName,author,url,title,labels,statusCheckRollup," + "closingIssuesReferences", ] ) pr = cast(dict[str, Any], pr_data) if isinstance(pr_data, dict) else {} @@ -292,9 +488,21 @@ def evaluate( "login -- held (pass --allow-unprotected to override)" ) + closing_issues = cast( + list[Any], pr.get("closingIssuesReferences") or [] + ) + tier_result: dict[str, Any] = {"enabled": False} + if tier is not None: + tier_blockers, tier_result = evaluate_autopilot_tier( + repo, number, str(head) if head else None, author_login, labels, + closing_issues, tier, + ) + blockers.extend(tier_blockers) + ready = not blockers return { "pr": f"{repo}#{number}", + "autopilotMergeTier": tier_result, "url": pr.get("url"), "title": pr.get("title"), "author": author_login, @@ -402,6 +610,38 @@ def main() -> int: "the TOCTOU guard that pins the vetted head SHA" ), ) + parser.add_argument( + "--autopilot-merge-tier", + action="store_true", + help=( + "gate on the #476 autopilot-merge-tier criteria in addition to the " + "base readiness gate: issue-linked, lane-authored, no blocking label, " + "a distinct-bot approving review on the live head, and no human " + "blocking comment. Fail-closed: requires --lane-logins, " + "--approver-bot-logins, and --block-labels to be non-empty" + ), + ) + parser.add_argument( + "--lane-logins", + default=None, + help="comma-separated pipeline lane author logins (autopilot merge tier)", + ) + parser.add_argument( + "--approver-bot-logins", + default=None, + help=( + "comma-separated bot logins whose approving review satisfies the " + "author != approver criterion (autopilot merge tier)" + ), + ) + parser.add_argument( + "--block-labels", + default=None, + help=( + "comma-separated labels that veto a tier merge, e.g. do-not-merge " + "(autopilot merge tier)" + ), + ) args = parser.parse_args() allowed = parse_allowed_owners(args.allowed_owners) @@ -454,6 +694,59 @@ def main() -> int: ) return 3 + # Build the autopilot-merge-tier config before any network access, failing + # closed on a partial configuration: the tier's whole point is that the three + # sets are all supplied deliberately, so an umbrella flag with any of them + # empty is a refusal, never a merge on an under-specified tier. + tier: AutopilotMergeTierConfig | None = None + if args.autopilot_merge_tier: + lane = parse_csv_set(args.lane_logins) + approver = parse_csv_set(args.approver_bot_logins) + block = parse_csv_set(args.block_labels) + missing = [ + name + for name, value in ( + ("--lane-logins", lane), + ("--approver-bot-logins", approver), + ("--block-labels", block), + ) + if not value + ] + if missing: + print( + json.dumps( + { + "pr": args.pr, + "error": ( + "--autopilot-merge-tier requires non-empty " + + ", ".join(missing) + + "; refusing to run the tier under-specified" + ), + } + ) + ) + return 3 + tier = AutopilotMergeTierConfig( + lane_logins=frozenset(lane), + approver_bot_logins=frozenset(approver), + block_labels=frozenset(block), + ) + elif any( + (args.lane_logins, args.approver_bot_logins, args.block_labels) + ): + print( + json.dumps( + { + "pr": args.pr, + "error": ( + "--lane-logins / --approver-bot-logins / --block-labels " + "are only meaningful with --autopilot-merge-tier" + ), + } + ) + ) + return 2 + # Resolve self logins only after every argument-shape refusal above: '@me' # resolution is a network call, and the guard's contract is that malformed # input is rejected before any network access. @@ -478,6 +771,7 @@ def main() -> int: self_logins, args.allow_dependency, args.allow_unprotected, + tier, ) except (RuntimeError, ValueError, json.JSONDecodeError) as exc: # Surface any gh/parse failure as JSON rather than a traceback. diff --git a/plugins/source-control/skills/babysit-prs/scripts/engine.test.sh b/plugins/source-control/skills/babysit-prs/scripts/engine.test.sh index f73a9b2f4..59cffcf53 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/engine.test.sh +++ b/plugins/source-control/skills/babysit-prs/scripts/engine.test.sh @@ -65,5 +65,9 @@ check_exit "merge wrapper reaches fail-closed CLI (no allowlist)" 3 \ bash "$MERGE_WRAPPER" "owner/repo#1" check_exit "resolve wrapper reaches fail-closed CLI (no allowlist)" 3 \ bash "$RESOLVE_WRAPPER" "owner/repo#1" +# The autopilot merge tier is fail-closed at the wrapper: the umbrella flag +# without its three required sets refuses before any network access. +check_exit "merge wrapper rejects --autopilot-merge-tier without required sets" 3 \ + bash "$MERGE_WRAPPER" "owner/repo#1" --allowed-owners owner --autopilot-merge-tier exit "$FAILED" diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py new file mode 100644 index 000000000..0174a0cf6 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -0,0 +1,239 @@ +"""Autopilot-merge-tier (#476) criteria evaluated in `babysit_merge`. + +The base readiness gate already blocks on state/mergeability/checks/approval/ +threads; those paths are covered by the subprocess guard tests. This module +covers the five criteria the #476 tier layers on top -- issue-linked, +lane-authored, no blocking label, a distinct-bot approval on the live head, and +no human blocking comment -- each with a passing and a fall-back fixture, plus +the invariant that an absent tier makes zero tier-specific network calls. + +Network is stubbed by monkeypatching `babysit_merge`'s gh seams; no real gh +process is spawned. +""" + +from __future__ import annotations + +import pathlib +import sys +import unittest +from typing import Any +from unittest import mock + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) + +import babysit_merge as merge + +HEAD = "a" * 40 +STALE = "b" * 40 +LANE = "lane-bot" +APPROVER = "approver-bot" + +TIER = merge.AutopilotMergeTierConfig( + lane_logins=frozenset({LANE}), + approver_bot_logins=frozenset({APPROVER}), + block_labels=frozenset({"do-not-merge"}), +) + + +def _pr(**overrides: Any) -> dict[str, Any]: + pr: dict[str, Any] = { + "state": "OPEN", + "isDraft": False, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": "APPROVED", + "headRefOid": HEAD, + "baseRefName": "main", + "author": {"login": LANE}, + "url": "https://example/pr", + "title": "t", + "labels": [], + "statusCheckRollup": [], + "closingIssuesReferences": [{"number": 476}], + } + pr.update(overrides) + return pr + + +# A branch rule requiring one approving review and no required status contexts: +# the base is protected (so no unprotected hold) and needs reviewDecision APPROVED. +RULES = [{"type": "pull_request", "parameters": {"required_approving_review_count": 1}}] + + +def _approval(login: str, oid: str, typename: str = "Bot", state: str = "APPROVED") -> dict[str, Any]: + return { + "state": state, + "author": {"login": login, "__typename": typename, "is_bot": typename == "Bot"}, + "commit": {"oid": oid}, + "body": "", + } + + +CLEAN_APPROVAL = [_approval(f"{APPROVER}[bot]", HEAD)] + + +class TierEvaluateHarness(unittest.TestCase): + """Run `evaluate` with the gh seams stubbed and the tier engaged.""" + + def _evaluate( + self, + pr: dict[str, Any], + *, + reviews: list[dict[str, Any]] | None = None, + issue_comments: list[dict[str, Any]] | None = None, + tier: merge.AutopilotMergeTierConfig | None = TIER, + ) -> dict[str, Any]: + def gh_json(args: list[str]) -> Any: + if args[:2] == ["pr", "view"]: + return pr + if args[0] == "api": # branch rules + return RULES + raise AssertionError(f"unexpected gh_json call: {args}") + + with ( + mock.patch.object(merge, "gh_json", side_effect=gh_json), + mock.patch.object(merge, "fetch_review_threads", return_value=[]), + mock.patch.object( + merge, "fetch_pull_request_reviews", + return_value=(CLEAN_APPROVAL if reviews is None else reviews), + ) as reviews_mock, + mock.patch.object( + merge, "fetch_issue_comments", + return_value=(issue_comments or []), + ) as comments_mock, + ): + result = merge.evaluate( + "owner/repo", 476, HEAD, {"owner"}, frozenset(), False, False, tier, + ) + result["_reviews_called"] = reviews_mock.called + result["_comments_called"] = comments_mock.called + return result + + +class TierPassesWhenEveryCriterionHolds(TierEvaluateHarness): + def test_all_criteria_met_is_ready(self) -> None: + result = self._evaluate(_pr()) + self.assertTrue(result["ready"], result["blockers"]) + tier = result["autopilotMergeTier"] + self.assertTrue(tier["enabled"]) + self.assertTrue(tier["issueLinked"]) + self.assertTrue(tier["laneAuthored"]) + self.assertEqual(tier["blockingLabels"], []) + self.assertEqual(tier["distinctBotApproval"]["author"], f"{APPROVER}[bot]") + self.assertEqual(tier["humanBlockingComments"], []) + + +class TierFallsBackPerCriterion(TierEvaluateHarness): + def test_not_issue_linked_blocks(self) -> None: + result = self._evaluate(_pr(closingIssuesReferences=[])) + self.assertFalse(result["ready"]) + self.assertFalse(result["autopilotMergeTier"]["issueLinked"]) + self.assertTrue(any("issue-linked" in b for b in result["blockers"])) + + def test_blocking_label_blocks(self) -> None: + result = self._evaluate(_pr(labels=[{"name": "do-not-merge"}])) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["blockingLabels"], ["do-not-merge"] + ) + self.assertTrue(any("do-not-merge" in b for b in result["blockers"])) + + def test_label_match_is_case_insensitive(self) -> None: + result = self._evaluate(_pr(labels=[{"name": "Do-Not-Merge"}])) + self.assertEqual( + result["autopilotMergeTier"]["blockingLabels"], ["do-not-merge"] + ) + + def test_non_lane_author_blocks(self) -> None: + result = self._evaluate(_pr(author={"login": "outsider"})) + self.assertFalse(result["ready"]) + self.assertFalse(result["autopilotMergeTier"]["laneAuthored"]) + self.assertTrue(any("pipeline lane" in b for b in result["blockers"])) + + def test_same_identity_approval_is_not_distinct(self) -> None: + # Author and approver are the same login: author != approver fails. + result = self._evaluate( + _pr(author={"login": f"{APPROVER}[bot]"}), + reviews=[_approval(f"{APPROVER}[bot]", HEAD)], + ) + self.assertIsNone(result["autopilotMergeTier"]["distinctBotApproval"]) + self.assertTrue(any("author != approver" in b for b in result["blockers"])) + + def test_human_approval_is_not_a_bot_approver(self) -> None: + result = self._evaluate( + _pr(), reviews=[_approval("maintainer", HEAD, typename="User")] + ) + self.assertIsNone(result["autopilotMergeTier"]["distinctBotApproval"]) + self.assertTrue(any("distinct-bot" in b for b in result["blockers"])) + + def test_stale_approval_off_head_blocks(self) -> None: + # An approval left on a superseded commit is not "unchanged since review". + result = self._evaluate( + _pr(), reviews=[_approval(f"{APPROVER}[bot]", STALE)] + ) + self.assertIsNone(result["autopilotMergeTier"]["distinctBotApproval"]) + self.assertTrue(any("distinct-bot" in b for b in result["blockers"])) + + def test_no_approving_review_blocks(self) -> None: + result = self._evaluate(_pr(), reviews=[]) + self.assertIsNone(result["autopilotMergeTier"]["distinctBotApproval"]) + + def test_human_blocking_comment_blocks(self) -> None: + comment = { + "author": {"login": "maintainer", "__typename": "User", "is_bot": False}, + "body": "Please do not merge, this is a blocking regression.", + } + result = self._evaluate(_pr(), issue_comments=[comment]) + self.assertFalse(result["ready"]) + self.assertIn("maintainer", result["autopilotMergeTier"]["humanBlockingComments"]) + self.assertTrue(any("human blocking comment" in b for b in result["blockers"])) + + def test_bot_comment_with_blocking_prose_does_not_block(self) -> None: + # A bot review body carrying blocking-looking prose is not a human stop. + comment = { + "author": {"login": "some-bot[bot]", "__typename": "Bot", "is_bot": True}, + "body": "This is a blocking regression must fix.", + } + result = self._evaluate(_pr(), issue_comments=[comment]) + self.assertEqual(result["autopilotMergeTier"]["humanBlockingComments"], []) + self.assertTrue(result["ready"], result["blockers"]) + + +class TierAbsentIsInert(TierEvaluateHarness): + def test_no_tier_makes_no_tier_network_calls(self) -> None: + result = self._evaluate(_pr(), tier=None) + self.assertFalse(result["autopilotMergeTier"]["enabled"]) + self.assertFalse(result["_reviews_called"]) + self.assertFalse(result["_comments_called"]) + self.assertTrue(result["ready"], result["blockers"]) + + +class DistinctBotApprovalUnit(unittest.TestCase): + def test_last_eligible_approval_on_head_wins(self) -> None: + reviews = [ + _approval(f"{APPROVER}[bot]", STALE), + _approval(f"{APPROVER}[bot]", HEAD), + ] + match = merge.find_distinct_bot_approval( + reviews, LANE, HEAD, frozenset({APPROVER}) + ) + self.assertIsNotNone(match) + self.assertEqual(match["commit"]["oid"], HEAD) + + def test_approver_matched_by_configured_login_without_bot_suffix(self) -> None: + # A bot account whose review author carries no [bot] suffix and no Bot + # typename is still an approver when named in approver_bot_logins. + reviews = [_approval(APPROVER, HEAD, typename="User")] + match = merge.find_distinct_bot_approval( + reviews, LANE, HEAD, frozenset({APPROVER}) + ) + self.assertIsNotNone(match) + + def test_no_match_when_head_none(self) -> None: + self.assertIsNone( + merge.find_distinct_bot_approval(CLEAN_APPROVAL, LANE, None, frozenset()) + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py index 0008cd7be..a4dd5062c 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py @@ -68,6 +68,38 @@ def test_malformed_ref_is_usage_error_exit_2(self): code, _ = run(MERGE, "not-a-ref", "--allowed-owners", "owner") self.assertEqual(code, 2) + def test_autopilot_tier_without_required_sets_refuses_exit_3(self): + # The tier's fail-closed core: the umbrella flag alone, with none of its + # three required sets, refuses before any network access. + code, payload = run( + MERGE, "owner/repo#1", "--allowed-owners", "owner", + "--autopilot-merge-tier", + ) + self.assertEqual(code, 3) + error = payload.get("error", "") + for flag in ("--lane-logins", "--approver-bot-logins", "--block-labels"): + self.assertIn(flag, error) + + def test_autopilot_tier_partial_config_refuses_exit_3(self): + # Two of three supplied still refuses, naming only the missing set. + code, payload = run( + MERGE, "owner/repo#1", "--allowed-owners", "owner", + "--autopilot-merge-tier", "--lane-logins", "lane", + "--approver-bot-logins", "bot", + ) + self.assertEqual(code, 3) + self.assertIn("--block-labels", payload.get("error", "")) + + def test_tier_params_without_umbrella_are_usage_error_exit_2(self): + # The parameter sets are meaningless without the umbrella flag; supplying + # them alone is a usage error, never a silent no-op. + code, payload = run( + MERGE, "owner/repo#1", "--allowed-owners", "owner", + "--lane-logins", "lane", + ) + self.assertEqual(code, 2) + self.assertIn("--autopilot-merge-tier", payload.get("error", "")) + class ResolveGuardFailsClosed(unittest.TestCase): def test_absent_allowlist_refuses_exit_3(self): diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py index ba9d2c5cb..694593480 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py @@ -123,6 +123,31 @@ def test_zero_blocker_draft_always_uses_a_worker(self) -> None: self.assertIn("a genuinely in-progress draft stays draft", paragraph) self.assertIn("reported and escalated with the reason", paragraph) + def test_autopilot_merge_tier_ships_disabled_and_fail_closed(self) -> None: + intro = _paragraph_containing(self.skill_text, "config-gated escalation") + self.assertIn("ships DISABLED", intro) + self.assertIn("separate, loudly-announced operator step", intro) + + review = _paragraph_containing(self.skill_text, "genuine review pass") + self.assertIn("second bot account", review) + self.assertIn("author ≠ approver", review) + self.assertIn("only when that review is clean", review) + + gate = _paragraph_containing(self.skill_text, "runs the pinned merge gate") + for criterion in ( + "issue-linked", + "pipeline lane", + "do-not-merge label", + "distinct-bot approval", + "head SHA unchanged since review", + "review workflow", + ): + self.assertIn(criterion, gate) + + fallback = _paragraph_containing(self.skill_text, "Any criterion failing falls back") + self.assertIn("never routes around the gate", fallback) + self.assertIn("fail-closed", fallback) + def test_full_queue_and_draft_contract_remains_explicit(self) -> None: autopilot = _paragraph_containing(self.skill_text, '"Every PR" means every PR') drafts = _paragraph_containing(self.skill_text, "**Draft PRs** are in scope") From da239d82c7162d421424f420d2e83d2e83a73112 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 05:40:52 -0400 Subject: [PATCH 02/11] fix(source-control): keep babysit SKILL.md under the 500-line skill-quality cap The #476 tier subsection duplicated the criteria that reference/safety.md already codifies and pushed SKILL.md to 530 lines, over the skill-quality hard cap of 500. Condense the subsection to an operational summary (disabled by default, the review-pass-then-gated-merge flow, fail-closed, fallback) and point to safety.md as the single home for the criteria and the safety-contract rationale, removing the duplication. Retarget the skill-contract test at the condensed prose and add a safety.md guard for the criteria that moved there. Co-authored-by: Claude Opus 4.8 --- .../skills/babysit-prs/SKILL.md | 35 +----------------- .../scripts/tests/test_skill_contract.py | 37 +++++++++++-------- 2 files changed, 24 insertions(+), 48 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/SKILL.md b/plugins/source-control/skills/babysit-prs/SKILL.md index c089a2d9b..1e54beecb 100644 --- a/plugins/source-control/skills/babysit-prs/SKILL.md +++ b/plugins/source-control/skills/babysit-prs/SKILL.md @@ -178,39 +178,8 @@ settings powers — those still escalate. Run it looped: ## Autopilot merge tier (#476) -A deliberate, config-gated escalation of autopilot's merge authority for a pipeline running at -day-scale, where human approve-and-merge is the throughput bottleneck. It **ships DISABLED** and -exists only while the operator sets `babysit_autopilot_merge_tier`; enabling that flag, and any -later gate-off flip, is a separate, loudly-announced operator step, never a default. Without the -flag every merge decision is exactly today's — the tier's whole surface is dormant and PRs that -are otherwise ready are reported on the human merge-ready list. - -Without the tier, autopilot can merge a PR only once *something else* has produced the approving -review the base ruleset requires; the tier lets the fleet produce that approval itself, safely. -When enabled, per candidate PR autopilot: - -1. Runs a **genuine review pass** through the review plugin under a **second bot account** whose - login is one of `babysit_approver_bot_logins` (author ≠ approver), and submits an **approving - review only when that review is clean**. This is a real review, never a rubber stamp; a review - that finds anything blocking is posted as findings and the PR is not merged. - -2. After the approval lands on the current head, runs the pinned merge gate with the tier flags — - `source-control-babysit-merge owner/repo#N --allowed-owners --self-logins - @me, --merge --expected-head --autopilot-merge-tier - --lane-logins --approver-bot-logins --block-labels - `. The gate merges **only when every criterion holds**, each enforced - deterministically: required checks green including the review workflow (`mergeStateStatus` - CLEAN, ruleset never bypassed); issue-linked (a closing-issue reference); authored by a - configured pipeline lane; no human `CHANGES_REQUESTED` / blocking comment / unresolved thread; - no configured do-not-merge label; and a distinct-bot approval (author ≠ approver) submitted - against the live head (head SHA unchanged since review, pinned by `--expected-head`). - -Any criterion failing falls back to reporting the PR on the human merge-ready list — the tier -never routes around the gate. The gate is **fail-closed**: `--autopilot-merge-tier` refuses (exit -`3`) unless all three of `--lane-logins`, `--approver-bot-logins`, and `--block-labels` are -non-empty, so an under-configured tier merges nothing. The criteria predicates are reused from the -shared classifier (`babysit_classify`), not re-implemented. The ruleset itself stays unchanged; -the bot review is what makes it a genuine gate rather than a bypass. +A config-gated escalation of autopilot's merge authority, **shipped DISABLED** and active only while the operator sets `babysit_autopilot_merge_tier` (enabling it, and the later gate-off flip, are separate announced steps; without it every merge decision is exactly today's). When enabled, per candidate PR autopilot runs a **genuine review pass** under a **second bot account** (author ≠ approver) that submits an approving review **only when clean**, then runs the pinned merge gate with the tier flags (`--autopilot-merge-tier --lane-logins --approver-bot-logins --block-labels `) added to `--merge --expected-head `. +That gate merges **only when every criterion holds** — the criteria and the safety-contract rationale are codified in [reference/safety.md](reference/safety.md). It is **fail-closed** (the umbrella flag refuses unless all three parameter sets are supplied; predicates reused from the shared `babysit_classify` module), and any criterion failing falls back to the human merge-ready list — the tier never routes around the gate. ## Guarded mutations: deterministic gates, agent judgment diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py index 694593480..0571d5f80 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py @@ -124,29 +124,36 @@ def test_zero_blocker_draft_always_uses_a_worker(self) -> None: self.assertIn("reported and escalated with the reason", paragraph) def test_autopilot_merge_tier_ships_disabled_and_fail_closed(self) -> None: - intro = _paragraph_containing(self.skill_text, "config-gated escalation") - self.assertIn("ships DISABLED", intro) - self.assertIn("separate, loudly-announced operator step", intro) - - review = _paragraph_containing(self.skill_text, "genuine review pass") - self.assertIn("second bot account", review) - self.assertIn("author ≠ approver", review) - self.assertIn("only when that review is clean", review) + para = _paragraph_containing(self.skill_text, "config-gated escalation") + for marker in ( + "shipped DISABLED", + "separate announced steps", + "genuine review pass", + "second bot account", + "author ≠ approver", + "only when clean", + "--autopilot-merge-tier", + "fail-closed", + "never routes around the gate", + "reference/safety.md", + ): + with self.subTest(marker=marker): + self.assertIn(marker, para) - gate = _paragraph_containing(self.skill_text, "runs the pinned merge gate") + def test_safety_md_codifies_the_tier_criteria(self) -> None: + safety = (SKILL.parent / "reference" / "safety.md").read_text(encoding="utf-8") + self.assertIn("ships **DISABLED**", safety) + self.assertIn("babysit_autopilot_merge_tier", safety) for criterion in ( "issue-linked", "pipeline lane", "do-not-merge label", - "distinct-bot approval", + "distinct bot identity", "head SHA unchanged since review", "review workflow", ): - self.assertIn(criterion, gate) - - fallback = _paragraph_containing(self.skill_text, "Any criterion failing falls back") - self.assertIn("never routes around the gate", fallback) - self.assertIn("fail-closed", fallback) + with self.subTest(criterion=criterion): + self.assertIn(criterion, safety) def test_full_queue_and_draft_contract_remains_explicit(self) -> None: autopilot = _paragraph_containing(self.skill_text, '"Every PR" means every PR') From fddd27cf500542adff94b6d7aca1c9d797ac8585 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 05:50:50 -0400 Subject: [PATCH 03/11] fix(source-control): harden babysit autopilot-merge-tier human-veto and approval checks Address three P1 review findings on the #476 tier, all fail-closed: - Ignore superseded bot approvals: collapse reviews to each actor's latest decisive state (via babysit_feedback.latest_reviews_by_author, decisive_only=True) before accepting a tier approval, so an approver bot that approved then requested changes on the same head no longer counts even when a separate approval keeps the base reviewDecision APPROVED. - Recognize explicit do-not-merge comments: has_blocking_text carries no do-not-merge pattern, so a plain human "please do not merge" veto escaped the "no human blocking comment" criterion. Add a bounded merge-veto regex to the tier's human-comment scan. - Scan inline review comments: a human inline veto on a since-resolved thread saw neither the base unresolved-thread gate nor the tier scan. Include the paginated pull-request review comments (fetch_pull_request_review_comments) with normalized author data in the human-blocking corpus. Each fix reuses an existing shared seam; new fixtures cover all three. Co-authored-by: Claude Opus 4.8 --- .../babysit-prs/scripts/babysit_merge.py | 30 ++++++++++- .../scripts/tests/test_babysit_merge.py | 54 ++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 969eb0ebd..14fbb1b81 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -59,17 +59,27 @@ normalize_login_set, normalize_self_logins, ) +from babysit_feedback import latest_reviews_by_author from babysit_gh import ( fetch_issue_comments, + fetch_pull_request_review_comments, fetch_pull_request_reviews, fetch_review_threads, gh_capture, gh_json, + normalized_rest_author, parse_repo_number, resolve_authors, ) from babysit_util import MIN_HEAD_SHA_PREFIX_LENGTH, configure_stdio, is_json_object +# A plain human "do not merge" veto that is neither a formal CHANGES_REQUESTED +# review nor the configured label: the shared blocking-text predicate does not +# carry a do-not-merge pattern, so the tier's "no human blocking comment" +# criterion matches it here. Bounded to the merge-veto sense (do/don't/do-not +# merge) so ordinary prose does not false-block. +HUMAN_MERGE_VETO_RE = re.compile(r"\bdo(?:n['’]?t| not|-not)[\s-]*merge\b", re.I) + EXPECTED_HEAD_RE = re.compile(rf"^[0-9a-fA-F]{{{MIN_HEAD_SHA_PREFIX_LENGTH},64}}$") # GitHub's own fixed enum contract. MergeStateStatus values meaning "mergeable, @@ -279,8 +289,13 @@ def evaluate_autopilot_tier( ) reviews = fetch_pull_request_reviews(repo, number) + # Collapse to each actor's latest decisive review before accepting a tier + # approval: a bot that approved and then submitted CHANGES_REQUESTED (or had + # its approval dismissed) on the same head must no longer count as the + # approver, even when another approval keeps the base reviewDecision APPROVED. + decisive_reviews = latest_reviews_by_author({"reviews": reviews}, decisive_only=True) approval = find_distinct_bot_approval( - reviews, author_login, head, tier.approver_bot_logins + decisive_reviews, author_login, head, tier.approver_bot_logins ) if approval is None: blockers.append( @@ -295,11 +310,22 @@ def evaluate_autopilot_tier( human_blocking: list[str] = [] corpus: list[dict[str, Any]] = list(fetch_issue_comments(repo, number)) corpus.extend(reviews) + # Inline review-thread comments are neither issue comments nor review + # summaries; a human veto left inline whose thread is later resolved would + # otherwise escape both the base unresolved-thread gate and this scan. + corpus.extend( + {"author": normalized_rest_author(row), "body": row.get("body")} + for row in fetch_pull_request_review_comments(repo, number) + ) for item in corpus: if actor_kind(item) != "human": continue body = str(item.get("body") or "") - if has_blocking_text(body) or has_blocking_severity(body): + if ( + has_blocking_text(body) + or has_blocking_severity(body) + or HUMAN_MERGE_VETO_RE.search(body) + ): login = item.get("author") login = login.get("login") if is_json_object(login) else login human_blocking.append(str(login or "unknown")) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 0174a0cf6..af041ec9f 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -60,11 +60,18 @@ def _pr(**overrides: Any) -> dict[str, Any]: RULES = [{"type": "pull_request", "parameters": {"required_approving_review_count": 1}}] -def _approval(login: str, oid: str, typename: str = "Bot", state: str = "APPROVED") -> dict[str, Any]: +def _approval( + login: str, + oid: str, + typename: str = "Bot", + state: str = "APPROVED", + submitted_at: str = "2026-01-01T00:00:00Z", +) -> dict[str, Any]: return { "state": state, "author": {"login": login, "__typename": typename, "is_bot": typename == "Bot"}, "commit": {"oid": oid}, + "submittedAt": submitted_at, "body": "", } @@ -81,6 +88,7 @@ def _evaluate( *, reviews: list[dict[str, Any]] | None = None, issue_comments: list[dict[str, Any]] | None = None, + review_comments: list[dict[str, Any]] | None = None, tier: merge.AutopilotMergeTierConfig | None = TIER, ) -> dict[str, Any]: def gh_json(args: list[str]) -> Any: @@ -101,12 +109,17 @@ def gh_json(args: list[str]) -> Any: merge, "fetch_issue_comments", return_value=(issue_comments or []), ) as comments_mock, + mock.patch.object( + merge, "fetch_pull_request_review_comments", + return_value=(review_comments or []), + ) as review_comments_mock, ): result = merge.evaluate( "owner/repo", 476, HEAD, {"owner"}, frozenset(), False, False, tier, ) result["_reviews_called"] = reviews_mock.called result["_comments_called"] = comments_mock.called + result["_review_comments_called"] = review_comments_mock.called return result @@ -188,6 +201,44 @@ def test_human_blocking_comment_blocks(self) -> None: self.assertIn("maintainer", result["autopilotMergeTier"]["humanBlockingComments"]) self.assertTrue(any("human blocking comment" in b for b in result["blockers"])) + def test_superseded_bot_approval_is_ignored(self) -> None: + # The approver bot approved, then requested changes on the same head; a + # human approval keeps the base reviewDecision APPROVED. The bot's latest + # decisive state is CHANGES_REQUESTED, so it is not a valid tier approver. + result = self._evaluate( + _pr(), + reviews=[ + _approval(f"{APPROVER}[bot]", HEAD, submitted_at="2026-01-01T00:00:00Z"), + _approval( + f"{APPROVER}[bot]", HEAD, state="CHANGES_REQUESTED", + submitted_at="2026-01-02T00:00:00Z", + ), + ], + ) + self.assertIsNone(result["autopilotMergeTier"]["distinctBotApproval"]) + self.assertFalse(result["ready"]) + + def test_plain_do_not_merge_comment_blocks(self) -> None: + comment = { + "author": {"login": "maintainer", "__typename": "User", "is_bot": False}, + "body": "Please do not merge this PR yet.", + } + result = self._evaluate(_pr(), issue_comments=[comment]) + self.assertIn("maintainer", result["autopilotMergeTier"]["humanBlockingComments"]) + self.assertFalse(result["ready"]) + + def test_inline_review_comment_veto_blocks(self) -> None: + # A human inline review comment on a since-resolved thread escapes the + # base unresolved-thread gate; the tier still catches it. + row = { + "user": {"login": "maintainer", "type": "User"}, + "body": "This is a blocking regression.", + } + result = self._evaluate(_pr(), review_comments=[row]) + self.assertTrue(result["_review_comments_called"]) + self.assertIn("maintainer", result["autopilotMergeTier"]["humanBlockingComments"]) + self.assertFalse(result["ready"]) + def test_bot_comment_with_blocking_prose_does_not_block(self) -> None: # A bot review body carrying blocking-looking prose is not a human stop. comment = { @@ -205,6 +256,7 @@ def test_no_tier_makes_no_tier_network_calls(self) -> None: self.assertFalse(result["autopilotMergeTier"]["enabled"]) self.assertFalse(result["_reviews_called"]) self.assertFalse(result["_comments_called"]) + self.assertFalse(result["_review_comments_called"]) self.assertTrue(result["ready"], result["blockers"]) From 9912da47b70f48f54a7d5e28a6d0cc9b88fb15c4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 05:57:06 -0400 Subject: [PATCH 04/11] fix(source-control): restrict babysit tier approval to the configured approver bot is_bot() accepts any [bot]/Bot-typed login before consulting approver_bot_logins, so an unrelated installed GitHub App's APPROVED review satisfied the tier's author != approver criterion, defeating the babysit_approver_bot_logins boundary. find_distinct_bot_approval now also requires normalized membership in the configured approver set, so only the designated bot's approval authorizes a tier merge. Fail-closed: an empty approver set matches nothing (already refused by the umbrella flag). Test: test_unconfigured_bot_approval_is_rejected. Co-authored-by: Claude Opus 4.8 --- .../skills/babysit-prs/scripts/babysit_merge.py | 6 ++++++ .../babysit-prs/scripts/tests/test_babysit_merge.py | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 14fbb1b81..67963fa8f 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -217,6 +217,7 @@ def find_distinct_bot_approval( one wins so a re-approval on the current head is honored. """ author_norm = normalize_login_set([author_login] if author_login else []) + approver_norm = normalize_login_set(approver_bot_logins) match: dict[str, Any] | None = None for review in reviews: if str(review.get("state") or "") != "APPROVED": @@ -234,6 +235,11 @@ def find_distinct_bot_approval( continue # same identity as the PR author -- not a distinct approver if not is_bot(login, typename, approver_bot_logins): continue + # A bot, but it must be the configured approver identity: `is_bot` accepts + # any `[bot]`/Bot-typed login, so without this an arbitrary installed + # App's approval would authorize a tier merge past the configured boundary. + if not (normalize_login_set([login]) & approver_norm): + continue commit = review.get("commit") commit_oid = commit.get("oid") if is_json_object(commit) else None if not (head and commit_oid and str(commit_oid) == str(head)): diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index af041ec9f..5f9cc8511 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -191,6 +191,15 @@ def test_no_approving_review_blocks(self) -> None: result = self._evaluate(_pr(), reviews=[]) self.assertIsNone(result["autopilotMergeTier"]["distinctBotApproval"]) + def test_unconfigured_bot_approval_is_rejected(self) -> None: + # An unrelated installed App's [bot] approval must not satisfy the tier: + # being a bot is not enough, it must be the configured approver identity. + result = self._evaluate( + _pr(), reviews=[_approval("random-app[bot]", HEAD)] + ) + self.assertIsNone(result["autopilotMergeTier"]["distinctBotApproval"]) + self.assertFalse(result["ready"]) + def test_human_blocking_comment_blocks(self) -> None: comment = { "author": {"login": "maintainer", "__typename": "User", "is_bot": False}, From a12e770f5a1362c89f4f3ab0db28c9cdf70a9fed Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 06:12:49 -0400 Subject: [PATCH 05/11] feat(source-control): add decision-default veto criterion to the babysit merge tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The triage lane records a defaulted (maintainer-vetoable) decision only as a 'Decision defaulted: X — veto before merge' issue comment, which a deterministic merge gate cannot see (Codex P1 on #641). Add a tier criterion: the PR's linked issue must carry no unratified decision-default marker. A marker is ratified only by a human maintainer (authorAssociation OWNER/MEMBER) comment strictly after it; reactions are not consulted (the reactions API carries no author association, so a reaction cannot be attributed to a maintainer, and using the operator's self-logins would let pipeline automation clear its own veto — the same #450 attribution-drift hazard). Fail closed: an unratified marker, or a linked issue whose comments cannot be fetched, holds the PR for the human merge-ready list. Marker matching is deliberately loose (over-matching only holds more for the human). Reuses fetch_issue_comments and the shared actor_kind predicate; criterion codified in safety.md. Pass, fall-back, and fetch-error fixtures added. Co-authored-by: Claude Opus 4.8 --- plugins/source-control/CHANGELOG.md | 2 + .../skills/babysit-prs/reference/safety.md | 5 ++ .../babysit-prs/scripts/babysit_merge.py | 84 ++++++++++++++++++ .../scripts/tests/test_babysit_merge.py | 86 ++++++++++++++++++- .../scripts/tests/test_skill_contract.py | 1 + 5 files changed, 175 insertions(+), 3 deletions(-) diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 854eb1db2..b5d7d0dc4 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -15,6 +15,8 @@ All notable changes to the `source-control` plugin are documented here. Format f criterion holds** — required checks green including the review workflow (`mergeStateStatus` CLEAN, ruleset untouched), issue-linked, authored by a configured pipeline lane, no human `CHANGES_REQUESTED` / blocking comment / unresolved thread, no configured do-not-merge label, + no unratified `Decision defaulted` marker on the linked issue (the triage lane's maintainer + veto window, which a maintainer ratifies by comment before the default rides into a merge), and a distinct-bot approval on the live head (head SHA unchanged since review). Any criterion failing falls back to today's behavior: the PR is reported on the human merge-ready list. The gate flag `--autopilot-merge-tier` is **fail-closed** — it refuses unless `--lane-logins`, diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index a32b4d925..edd2fbfae 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -244,6 +244,11 @@ as done and re-running the gate. - the PR is authored by a configured pipeline lane; - no human `CHANGES_REQUESTED`, no human blocking comment, no unresolved review thread; - no configured do-not-merge label is present; + - the PR's linked issue carries no unratified `Decision defaulted` marker — the triage lane + records a defaulted (maintainer-vetoable) decision only as a `Decision defaulted: X — veto + before merge` issue comment, invisible to the gate, so the default rides into an autopilot + merge only once a maintainer has ratified it (a human `OWNER`/`MEMBER` comment after the + marker); an unratified marker, or an issue whose comments cannot be read, holds the PR; - the approving review is by a **distinct bot identity** (author ≠ approver) and was submitted against the **live head** (head SHA unchanged since review), pinned as always by `--expected-head`. diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 67963fa8f..df6774360 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -248,6 +248,84 @@ def find_distinct_bot_approval( return match +DECISION_DEFAULT_MARKER_RE = re.compile(r"decision[ -]defaulted", re.I) +# GitHub author associations that identify a maintainer able to exercise the +# "veto before merge" window: the operators the ratification signal must come +# from. COLLABORATOR is deliberately excluded -- it is granted per-repo push +# access, not the maintainer role that owns the veto. +RATIFYING_ASSOCIATIONS = frozenset({"OWNER", "MEMBER"}) + + +def _decision_default_ratified(comments: list[dict[str, Any]], marker_ts: str) -> bool: + """True when a human maintainer commented strictly after the marker. + + Reactions are deliberately not consulted: the reactions API carries no + author association, so a reaction cannot be attributed to a maintainer, and + attributing it via the operator's own self-logins would let pipeline + automation posting under that identity clear its own veto (the #450 + attribution-drift hazard). The fail-closed reading holds the marker until a + maintainer clears it with a comment. + """ + for comment in comments: + if str(comment.get("createdAt") or "") <= marker_ts: + continue + if actor_kind(comment) != "human": + continue + association = str(comment.get("authorAssociation") or "").upper() + if association in RATIFYING_ASSOCIATIONS: + return True + return False + + +def evaluate_decision_default_veto( + repo: str, closing_issues: list[Any] +) -> tuple[list[str], list[int]]: + """Hold when a linked issue carries an unratified 'Decision defaulted' marker. + + The triage lane records a defaulted (maintainer-vetoable) decision only as a + `Decision defaulted: X -- veto before merge` issue comment, which a + deterministic merge gate cannot see; the default may ride into an autopilot + merge only once a maintainer has ratified it. Marker matching is deliberately + loose (over-matching merely holds more for the human). Fail closed: a + comment-fetch failure holds the PR for the human list rather than merging on + an unverifiable issue. + """ + blockers: list[str] = [] + held: list[int] = [] + for ref in closing_issues: + number = ref.get("number") if is_json_object(ref) else ref + try: + issue_number = int(number) + except (TypeError, ValueError): + continue + try: + comments = fetch_issue_comments(repo, issue_number) + except (RuntimeError, ValueError, json.JSONDecodeError) as exc: + blockers.append( + f"could not verify the decision-default veto on #{issue_number} " + f"({type(exc).__name__}) -- holding for the human merge-ready list" + ) + held.append(issue_number) + continue + marker_timestamps = [ + str(c.get("createdAt") or "") + for c in comments + if is_json_object(c) + and DECISION_DEFAULT_MARKER_RE.search(str(c.get("body") or "")) + ] + if not marker_timestamps: + continue + if _decision_default_ratified(comments, max(marker_timestamps)): + continue + blockers.append( + f"linked issue #{issue_number} carries an unratified 'Decision " + "defaulted' marker -- a maintainer must ratify or veto before an " + "autopilot merge" + ) + held.append(issue_number) + return blockers, held + + def evaluate_autopilot_tier( repo: str, number: int, @@ -342,6 +420,11 @@ def evaluate_autopilot_tier( + " -- resolve before an autopilot merge" ) + veto_blockers, decision_default_held = evaluate_decision_default_veto( + repo, closing_issues + ) + blockers.extend(veto_blockers) + tier_result = { "enabled": True, "issueLinked": issue_linked, @@ -367,6 +450,7 @@ def evaluate_autopilot_tier( else None ), "humanBlockingComments": sorted(set(human_blocking)), + "decisionDefaultHeldIssues": decision_default_held, } return blockers, tier_result diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 5f9cc8511..6da4d4a4f 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -27,6 +27,32 @@ STALE = "b" * 40 LANE = "lane-bot" APPROVER = "approver-bot" +PR_NUMBER = 476 +LINKED_ISSUE = 999 # distinct from the PR so the two comment fetches are separable + + +def _comment( + login: str, + body: str = "", + *, + typename: str = "User", + association: str = "NONE", + created_at: str = "2026-01-01T00:00:00Z", +) -> dict[str, Any]: + return { + "author": {"login": login, "__typename": typename, "is_bot": typename == "Bot"}, + "authorAssociation": association, + "body": body, + "createdAt": created_at, + } + + +DECISION_MARKER = _comment( + "triage-bot[bot]", + "Decision defaulted: use squash — veto before merge", + typename="Bot", + created_at="2026-01-01T00:00:00Z", +) TIER = merge.AutopilotMergeTierConfig( lane_logins=frozenset({LANE}), @@ -49,7 +75,7 @@ def _pr(**overrides: Any) -> dict[str, Any]: "title": "t", "labels": [], "statusCheckRollup": [], - "closingIssuesReferences": [{"number": 476}], + "closingIssuesReferences": [{"number": LINKED_ISSUE}], } pr.update(overrides) return pr @@ -89,6 +115,8 @@ def _evaluate( reviews: list[dict[str, Any]] | None = None, issue_comments: list[dict[str, Any]] | None = None, review_comments: list[dict[str, Any]] | None = None, + linked_issue_comments: list[dict[str, Any]] | None = None, + linked_issue_error: bool = False, tier: merge.AutopilotMergeTierConfig | None = TIER, ) -> dict[str, Any]: def gh_json(args: list[str]) -> Any: @@ -98,6 +126,15 @@ def gh_json(args: list[str]) -> Any: return RULES raise AssertionError(f"unexpected gh_json call: {args}") + def issue_comments_side_effect(repo: str, n: int) -> list[dict[str, Any]]: + # The linked-issue fetch (decision-default veto) is separable from the + # PR's own comment fetch (human-blocking scan) by issue number. + if n == LINKED_ISSUE: + if linked_issue_error: + raise RuntimeError("simulated comment-fetch failure") + return linked_issue_comments or [] + return issue_comments or [] + with ( mock.patch.object(merge, "gh_json", side_effect=gh_json), mock.patch.object(merge, "fetch_review_threads", return_value=[]), @@ -107,7 +144,7 @@ def gh_json(args: list[str]) -> Any: ) as reviews_mock, mock.patch.object( merge, "fetch_issue_comments", - return_value=(issue_comments or []), + side_effect=issue_comments_side_effect, ) as comments_mock, mock.patch.object( merge, "fetch_pull_request_review_comments", @@ -115,7 +152,7 @@ def gh_json(args: list[str]) -> Any: ) as review_comments_mock, ): result = merge.evaluate( - "owner/repo", 476, HEAD, {"owner"}, frozenset(), False, False, tier, + "owner/repo", PR_NUMBER, HEAD, {"owner"}, frozenset(), False, False, tier, ) result["_reviews_called"] = reviews_mock.called result["_comments_called"] = comments_mock.called @@ -134,6 +171,21 @@ def test_all_criteria_met_is_ready(self) -> None: self.assertEqual(tier["blockingLabels"], []) self.assertEqual(tier["distinctBotApproval"]["author"], f"{APPROVER}[bot]") self.assertEqual(tier["humanBlockingComments"], []) + self.assertEqual(tier["decisionDefaultHeldIssues"], []) + + def test_ratified_decision_default_marker_passes(self) -> None: + # A maintainer (OWNER) comment strictly after the marker ratifies it. + ratify = _comment( + "maintainer", "Ratified — proceed.", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, ratify] + ) + self.assertTrue(result["ready"], result["blockers"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [] + ) class TierFallsBackPerCriterion(TierEvaluateHarness): @@ -248,6 +300,34 @@ def test_inline_review_comment_veto_blocks(self) -> None: self.assertIn("maintainer", result["autopilotMergeTier"]["humanBlockingComments"]) self.assertFalse(result["ready"]) + def test_unratified_decision_default_marker_blocks(self) -> None: + result = self._evaluate(_pr(), linked_issue_comments=[DECISION_MARKER]) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_ISSUE] + ) + self.assertTrue(any("Decision defaulted" in b for b in result["blockers"])) + + def test_non_maintainer_comment_does_not_ratify(self) -> None: + # A later comment from a non-maintainer is not the veto-holder's ratification. + later = _comment( + "drive-by", "looks fine to me", association="NONE", + created_at="2026-02-01T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, later] + ) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_ISSUE] + ) + + def test_decision_default_fetch_error_holds(self) -> None: + # Fail closed: a comment-fetch failure holds the PR for the human list. + result = self._evaluate(_pr(), linked_issue_error=True) + self.assertFalse(result["ready"]) + self.assertTrue(any("could not verify" in b for b in result["blockers"])) + def test_bot_comment_with_blocking_prose_does_not_block(self) -> None: # A bot review body carrying blocking-looking prose is not a human stop. comment = { diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py index 0571d5f80..ea08c26f1 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_skill_contract.py @@ -151,6 +151,7 @@ def test_safety_md_codifies_the_tier_criteria(self) -> None: "distinct bot identity", "head SHA unchanged since review", "review workflow", + "Decision defaulted", ): with self.subTest(criterion=criterion): self.assertIn(criterion, safety) From d3c9bbf38bdd8d98233d5d7db19ecb72747e6c8a Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 06:31:36 -0400 Subject: [PATCH 06/11] fix(source-control): require an explicit ratification signal to clear the decision-default veto MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decision-default veto treated ANY later OWNER/MEMBER comment as ratifying the marker, so an unrelated "thanks" would clear the maintainer veto (Codex P1 on #665). Ratification now requires an explicit signal in the maintainer's comment: a closed, whole-word token set (ratify/ratified, approve/approved, confirm/confirmed), and not a withheld-approval negation (reusing the shared NON_APPROVAL_RE). Matching is strict/fail-closed — an unrelated comment, a signal that predates the marker, or a negated approval do not clear, and only over-hold to the human list. Token set documented in safety.md next to the marker contract. Fixtures added: explicit-signal clears (existing), unrelated-comment does not, signal-before-marker does not, negated-approval does not. Co-authored-by: Claude Opus 4.8 --- .../skills/babysit-prs/reference/safety.md | 8 +++- .../babysit-prs/scripts/babysit_merge.py | 27 ++++++++++-- .../scripts/tests/test_babysit_merge.py | 41 +++++++++++++++++++ 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index edd2fbfae..43ef30288 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -247,8 +247,12 @@ as done and re-running the gate. - the PR's linked issue carries no unratified `Decision defaulted` marker — the triage lane records a defaulted (maintainer-vetoable) decision only as a `Decision defaulted: X — veto before merge` issue comment, invisible to the gate, so the default rides into an autopilot - merge only once a maintainer has ratified it (a human `OWNER`/`MEMBER` comment after the - marker); an unratified marker, or an issue whose comments cannot be read, holds the PR; + merge only once a maintainer has **ratified** it: a human `OWNER`/`MEMBER` comment posted + after the marker carrying an explicit ratification signal — a closed, whole-word token set + (`ratify`/`ratified`, `approve`/`approved`, `confirm`/`confirmed`), and not a + withheld-approval negation (`not approved`, `cannot approve`). Matching is strict and + fail-closed: an unrelated maintainer comment, a signal appearing before the marker, an + unratified marker, or an issue whose comments cannot be read all hold the PR; - the approving review is by a **distinct bot identity** (author ≠ approver) and was submitted against the **live head** (head SHA unchanged since review), pinned as always by `--expected-head`. diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index df6774360..9fe7f0472 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -50,6 +50,7 @@ from babysit_checks import check_identity_key, classify_checks from babysit_classify import ( + NON_APPROVAL_RE, actor_kind, has_blocking_severity, has_blocking_text, @@ -254,17 +255,30 @@ def find_distinct_bot_approval( # from. COLLABORATOR is deliberately excluded -- it is granted per-repo push # access, not the maintainer role that owns the veto. RATIFYING_ASSOCIATIONS = frozenset({"OWNER", "MEMBER"}) +# A maintainer clears the veto only with an explicit ratification signal -- a +# small, closed, whole-word token set -- not merely any later comment (an +# unrelated "thanks" must not ratify). Matching is strict/fail-closed: a comment +# without a signal (or carrying a withheld-approval negation) does not clear, so +# an ambiguous maintainer comment over-holds to the human list. Keep this set in +# sync with the contract documented in reference/safety.md. +RATIFICATION_SIGNAL_RE = re.compile( + r"\b(?:ratif(?:y|ied)|approved?|confirmed?)\b", re.I +) def _decision_default_ratified(comments: list[dict[str, Any]], marker_ts: str) -> bool: - """True when a human maintainer commented strictly after the marker. + """True when a human maintainer explicitly ratified strictly after the marker. - Reactions are deliberately not consulted: the reactions API carries no + Ratification requires an explicit signal (`RATIFICATION_SIGNAL_RE`) in a human + maintainer's comment posted after the marker -- an unrelated later comment + ("thanks", a status question) does not clear the veto, and a withheld-approval + negation (`NON_APPROVAL_RE`) never ratifies even when it contains an approval + token. Reactions are deliberately not consulted: the reactions API carries no author association, so a reaction cannot be attributed to a maintainer, and attributing it via the operator's own self-logins would let pipeline automation posting under that identity clear its own veto (the #450 attribution-drift hazard). The fail-closed reading holds the marker until a - maintainer clears it with a comment. + maintainer clears it with an explicit ratification comment. """ for comment in comments: if str(comment.get("createdAt") or "") <= marker_ts: @@ -272,7 +286,12 @@ def _decision_default_ratified(comments: list[dict[str, Any]], marker_ts: str) - if actor_kind(comment) != "human": continue association = str(comment.get("authorAssociation") or "").upper() - if association in RATIFYING_ASSOCIATIONS: + if association not in RATIFYING_ASSOCIATIONS: + continue + body = str(comment.get("body") or "") + if NON_APPROVAL_RE.search(body): + continue # a withheld/negated approval is not a ratification + if RATIFICATION_SIGNAL_RE.search(body): return True return False diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 6da4d4a4f..5c9de3cfc 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -328,6 +328,47 @@ def test_decision_default_fetch_error_holds(self) -> None: self.assertFalse(result["ready"]) self.assertTrue(any("could not verify" in b for b in result["blockers"])) + def test_unrelated_maintainer_comment_does_not_ratify(self) -> None: + # A later maintainer comment with no explicit ratification signal (a bare + # "thanks") must not clear the veto. + chatter = _comment( + "maintainer", "thanks, nice work here", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, chatter] + ) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_ISSUE] + ) + + def test_ratification_signal_before_marker_does_not_clear(self) -> None: + # A ratification signal that predates the marker is not a ratification of it. + early = _comment( + "maintainer", "approved", association="OWNER", + created_at="2025-12-01T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[early, DECISION_MARKER] + ) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_ISSUE] + ) + + def test_negated_approval_does_not_ratify(self) -> None: + # A withheld/negated approval must not clear the veto even though it + # contains an approval token. + negated = _comment( + "maintainer", "not approved yet — hold this", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, negated] + ) + self.assertFalse(result["ready"]) + def test_bot_comment_with_blocking_prose_does_not_block(self) -> None: # A bot review body carrying blocking-looking prose is not a human stop. comment = { From a9bb74d94456d3009f1cb56d9ee91b12f98a0d44 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 06:45:17 -0400 Subject: [PATCH 07/11] fix(source-control): read decision-default veto comments from the referenced issue's own repo The decision-default veto fetched the linked issue's comments from the PR's repository using only the issue number, so a PR closing an issue in ANOTHER repo would read a same-numbered issue in the wrong repo and miss its marker (Codex P1 on #665). Each `closingIssuesReferences` entry carries its own repository; the scan now reads from `owner/name` (falling back to the PR repo), and the held list reports canonical `owner/repo#n` refs. Fixture added: test_cross_repo_linked_issue_read_from_its_own_repo. Co-authored-by: Claude Opus 4.8 --- .../babysit-prs/scripts/babysit_merge.py | 50 +++++++++++++------ .../scripts/tests/test_babysit_merge.py | 35 +++++++++++-- 2 files changed, 67 insertions(+), 18 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 9fe7f0472..ceb111fdd 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -296,35 +296,58 @@ def _decision_default_ratified(comments: list[dict[str, Any]], marker_ts: str) - return False +def _ref_repo(ref: dict[str, Any]) -> str | None: + """`owner/name` of a closing-issue reference's own repository, if present. + + A PR may close an issue in a different repository; the reference carries that + repository, so the veto scan must read comments from it rather than assuming + the PR's repo (where a same-numbered issue could carry no marker). + """ + repository = ref.get("repository") + if not is_json_object(repository): + return None + name = repository.get("name") + owner = repository.get("owner") + login = owner.get("login") if is_json_object(owner) else None + return f"{login}/{name}" if login and name else None + + def evaluate_decision_default_veto( repo: str, closing_issues: list[Any] -) -> tuple[list[str], list[int]]: +) -> tuple[list[str], list[str]]: """Hold when a linked issue carries an unratified 'Decision defaulted' marker. The triage lane records a defaulted (maintainer-vetoable) decision only as a `Decision defaulted: X -- veto before merge` issue comment, which a deterministic merge gate cannot see; the default may ride into an autopilot - merge only once a maintainer has ratified it. Marker matching is deliberately - loose (over-matching merely holds more for the human). Fail closed: a - comment-fetch failure holds the PR for the human list rather than merging on - an unverifiable issue. + merge only once a maintainer has ratified it. Each linked issue is read from + its own repository (a PR may close an issue in another repo). Marker matching + is deliberately loose (over-matching merely holds more for the human). Fail + closed: a comment-fetch failure holds the PR for the human list rather than + merging on an unverifiable issue. """ blockers: list[str] = [] - held: list[int] = [] + held: list[str] = [] for ref in closing_issues: - number = ref.get("number") if is_json_object(ref) else ref + if is_json_object(ref): + number = ref.get("number") + issue_repo = _ref_repo(ref) or repo + else: + number = ref + issue_repo = repo try: issue_number = int(number) except (TypeError, ValueError): continue + target = f"{issue_repo}#{issue_number}" try: - comments = fetch_issue_comments(repo, issue_number) + comments = fetch_issue_comments(issue_repo, issue_number) except (RuntimeError, ValueError, json.JSONDecodeError) as exc: blockers.append( - f"could not verify the decision-default veto on #{issue_number} " + f"could not verify the decision-default veto on {target} " f"({type(exc).__name__}) -- holding for the human merge-ready list" ) - held.append(issue_number) + held.append(target) continue marker_timestamps = [ str(c.get("createdAt") or "") @@ -337,11 +360,10 @@ def evaluate_decision_default_veto( if _decision_default_ratified(comments, max(marker_timestamps)): continue blockers.append( - f"linked issue #{issue_number} carries an unratified 'Decision " - "defaulted' marker -- a maintainer must ratify or veto before an " - "autopilot merge" + f"linked issue {target} carries an unratified 'Decision defaulted' " + "marker -- a maintainer must ratify or veto before an autopilot merge" ) - held.append(issue_number) + held.append(target) return blockers, held diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 5c9de3cfc..b9a51959c 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -29,6 +29,7 @@ APPROVER = "approver-bot" PR_NUMBER = 476 LINKED_ISSUE = 999 # distinct from the PR so the two comment fetches are separable +LINKED_REF = f"owner/repo#{LINKED_ISSUE}" def _comment( @@ -157,6 +158,7 @@ def issue_comments_side_effect(repo: str, n: int) -> list[dict[str, Any]]: result["_reviews_called"] = reviews_mock.called result["_comments_called"] = comments_mock.called result["_review_comments_called"] = review_comments_mock.called + result["_comments_calls"] = list(comments_mock.call_args_list) return result @@ -304,7 +306,7 @@ def test_unratified_decision_default_marker_blocks(self) -> None: result = self._evaluate(_pr(), linked_issue_comments=[DECISION_MARKER]) self.assertFalse(result["ready"]) self.assertEqual( - result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_ISSUE] + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_REF] ) self.assertTrue(any("Decision defaulted" in b for b in result["blockers"])) @@ -319,7 +321,7 @@ def test_non_maintainer_comment_does_not_ratify(self) -> None: ) self.assertFalse(result["ready"]) self.assertEqual( - result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_ISSUE] + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_REF] ) def test_decision_default_fetch_error_holds(self) -> None: @@ -328,6 +330,31 @@ def test_decision_default_fetch_error_holds(self) -> None: self.assertFalse(result["ready"]) self.assertTrue(any("could not verify" in b for b in result["blockers"])) + def test_cross_repo_linked_issue_read_from_its_own_repo(self) -> None: + # A PR closing an issue in another repo must have the veto read from that + # repo, not the PR's repo where a same-numbered issue could lack the marker. + cross = _pr( + closingIssuesReferences=[ + { + "number": LINKED_ISSUE, + "repository": { + "name": "other-repo", + "owner": {"login": "other-owner"}, + }, + } + ] + ) + result = self._evaluate(cross, linked_issue_comments=[DECISION_MARKER]) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], + [f"other-owner/other-repo#{LINKED_ISSUE}"], + ) + self.assertIn( + mock.call("other-owner/other-repo", LINKED_ISSUE), + result["_comments_calls"], + ) + def test_unrelated_maintainer_comment_does_not_ratify(self) -> None: # A later maintainer comment with no explicit ratification signal (a bare # "thanks") must not clear the veto. @@ -340,7 +367,7 @@ def test_unrelated_maintainer_comment_does_not_ratify(self) -> None: ) self.assertFalse(result["ready"]) self.assertEqual( - result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_ISSUE] + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_REF] ) def test_ratification_signal_before_marker_does_not_clear(self) -> None: @@ -354,7 +381,7 @@ def test_ratification_signal_before_marker_does_not_clear(self) -> None: ) self.assertFalse(result["ready"]) self.assertEqual( - result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_ISSUE] + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_REF] ) def test_negated_approval_does_not_ratify(self) -> None: From 871a9e8d7d2167ff29bc7c282f48d06234c25d2c Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 07:26:43 -0400 Subject: [PATCH 08/11] fix(source-control): honor later maintainer revocations of a ratified decision default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_decision_default_ratified` returned True on the first ratifying maintainer comment after the marker, so a LATER maintainer comment withdrawing that approval ("not approved", "do not merge") was never examined and the veto was permanently cleared — the tier could autopilot-merge despite a subsequently expressed veto. The linked issue's comments are also outside the PR-level human-blocking corpus, so nothing else caught the withdrawal (Codex P1 on #665, thread PRRT_kwDOTCGFQM6SN2wi). Now every human-maintainer comment after the marker is scanned and the latest decisive signal wins: an explicit ratification token ratifies, while a revocation reusing the shared veto vocabulary (`NON_APPROVAL_RE` / `HUMAN_MERGE_VETO_RE`) re-holds. Revocation is tested first so a mixed-signal comment reads as a revoke, and ratification clears only when strictly newer than every revocation, so a same-timestamp tie holds — fail-closed throughout. Fixtures added: ratified-then-revoked holds, revoked-then-ratified clears, non-decisive later comment leaves a prior ratification standing, same-timestamp ambiguity holds. safety.md's ratification contract now states latest-decisive- signal-wins. Co-authored-by: Claude Opus 4.8 --- .../skills/babysit-prs/reference/safety.md | 10 ++- .../babysit-prs/scripts/babysit_merge.py | 47 +++++++----- .../scripts/tests/test_babysit_merge.py | 76 +++++++++++++++++++ 3 files changed, 112 insertions(+), 21 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index 43ef30288..55c4723af 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -250,9 +250,13 @@ as done and re-running the gate. merge only once a maintainer has **ratified** it: a human `OWNER`/`MEMBER` comment posted after the marker carrying an explicit ratification signal — a closed, whole-word token set (`ratify`/`ratified`, `approve`/`approved`, `confirm`/`confirmed`), and not a - withheld-approval negation (`not approved`, `cannot approve`). Matching is strict and - fail-closed: an unrelated maintainer comment, a signal appearing before the marker, an - unratified marker, or an issue whose comments cannot be read all hold the PR; + withheld-approval negation (`not approved`, `cannot approve`). All maintainer comments + after the marker are scanned and the **latest decisive signal wins**: a ratification token + ratifies, while a revocation reusing the veto vocabulary (`not approved`, `do not merge`) + re-holds, so a maintainer who ratifies and then revokes holds the PR. Matching is strict and + fail-closed: an unrelated maintainer comment, a signal appearing before the marker, a + ratify/revoke tie at the same timestamp, an unratified marker, or an issue whose comments + cannot be read all hold the PR; - the approving review is by a **distinct bot identity** (author ≠ approver) and was submitted against the **live head** (head SHA unchanged since review), pinned as always by `--expected-head`. diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index ceb111fdd..e7440f967 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -267,21 +267,32 @@ def find_distinct_bot_approval( def _decision_default_ratified(comments: list[dict[str, Any]], marker_ts: str) -> bool: - """True when a human maintainer explicitly ratified strictly after the marker. - - Ratification requires an explicit signal (`RATIFICATION_SIGNAL_RE`) in a human - maintainer's comment posted after the marker -- an unrelated later comment - ("thanks", a status question) does not clear the veto, and a withheld-approval - negation (`NON_APPROVAL_RE`) never ratifies even when it contains an approval - token. Reactions are deliberately not consulted: the reactions API carries no - author association, so a reaction cannot be attributed to a maintainer, and - attributing it via the operator's own self-logins would let pipeline - automation posting under that identity clear its own veto (the #450 - attribution-drift hazard). The fail-closed reading holds the marker until a - maintainer clears it with an explicit ratification comment. + """True when a maintainer's latest decisive comment after the marker ratifies. + + Every human-maintainer comment posted strictly after the marker is scanned and + the latest *decisive* signal wins: a single early ratification no longer + settles the question, so a maintainer who ratifies and then revokes ("not + approved", "do not merge") re-holds the PR for the human list. A comment is + decisive when it carries either an explicit ratification signal + (`RATIFICATION_SIGNAL_RE`) or an explicit revocation signal reusing the shared + veto vocabulary (`NON_APPROVAL_RE`, or `HUMAN_MERGE_VETO_RE`). Revocation is + tested first, so a comment mixing both reads as a revoke (fail closed). An + unrelated later comment ("thanks", a status question) is non-decisive and + leaves any prior decisive signal standing. + + Ratification clears the veto only when a ratifying comment is strictly newer + than every revoking one, so a ratify/revoke tie at the same timestamp -- like a + bare marker with no decisive comment -- holds. Reactions are deliberately not + consulted: the reactions API carries no author association, so a reaction + cannot be attributed to a maintainer, and attributing it via the operator's own + self-logins would let pipeline automation posting under that identity clear its + own veto (the #450 attribution-drift hazard). """ + latest_ratify = "" + latest_revoke = "" for comment in comments: - if str(comment.get("createdAt") or "") <= marker_ts: + created_at = str(comment.get("createdAt") or "") + if created_at <= marker_ts: continue if actor_kind(comment) != "human": continue @@ -289,11 +300,11 @@ def _decision_default_ratified(comments: list[dict[str, Any]], marker_ts: str) - if association not in RATIFYING_ASSOCIATIONS: continue body = str(comment.get("body") or "") - if NON_APPROVAL_RE.search(body): - continue # a withheld/negated approval is not a ratification - if RATIFICATION_SIGNAL_RE.search(body): - return True - return False + if NON_APPROVAL_RE.search(body) or HUMAN_MERGE_VETO_RE.search(body): + latest_revoke = max(latest_revoke, created_at) + elif RATIFICATION_SIGNAL_RE.search(body): + latest_ratify = max(latest_ratify, created_at) + return bool(latest_ratify) and latest_ratify > latest_revoke def _ref_repo(ref: dict[str, Any]) -> str | None: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index b9a51959c..1c723dbf4 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -396,6 +396,82 @@ def test_negated_approval_does_not_ratify(self) -> None: ) self.assertFalse(result["ready"]) + def test_ratified_then_revoked_holds(self) -> None: + # The latest decisive maintainer signal wins: a later revocation ("do not + # merge") re-holds a marker an earlier comment had ratified. + ratify = _comment( + "maintainer", "Ratified — proceed.", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + revoke = _comment( + "maintainer", "Actually, do not merge — hold this.", association="OWNER", + created_at="2026-02-02T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, ratify, revoke] + ) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_REF] + ) + + def test_revoked_then_ratified_clears(self) -> None: + # Latest wins in the other direction: a ratification newer than an earlier + # revocation clears the veto. + revoke = _comment( + "maintainer", "not approved yet — hold this", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + ratify = _comment( + "maintainer", "Re-reviewed — ratified, proceed.", association="OWNER", + created_at="2026-02-02T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, revoke, ratify] + ) + self.assertTrue(result["ready"], result["blockers"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [] + ) + + def test_non_decisive_comment_after_ratification_leaves_it_standing(self) -> None: + # A non-decisive later comment (a bare "thanks") does not disturb a prior + # ratification. + ratify = _comment( + "maintainer", "Ratified — proceed.", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + chatter = _comment( + "maintainer", "thanks, nice work here", association="OWNER", + created_at="2026-02-02T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, ratify, chatter] + ) + self.assertTrue(result["ready"], result["blockers"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [] + ) + + def test_same_timestamp_ratify_and_revoke_holds(self) -> None: + # Ambiguity is fail-closed: a ratify/revoke tie at the same timestamp holds + # (ratification must be strictly newer than every revocation to clear). + ratify = _comment( + "maintainer", "Ratified — proceed.", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + revoke = _comment( + "maintainer", "do not merge", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, ratify, revoke] + ) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_REF] + ) + def test_bot_comment_with_blocking_prose_does_not_block(self) -> None: # A bot review body carrying blocking-looking prose is not a human stop. comment = { From c404522a8872a950f0ddcfc6fa04c06bb4af43c5 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 08:08:32 -0400 Subject: [PATCH 09/11] fix(source-control): classify configured approver/lane logins as bots in the tier veto and ratification scans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An approver configured via `--approver-bot-logins` (or a pipeline lane login) that GitHub reports as account type `User` without a `[bot]` suffix passed `find_distinct_bot_approval` — which classifies via `is_bot`, honoring the configured login over the non-`Bot` typename — but was read as HUMAN by the tier's veto and ratification scans, which classify via `actor_kind`. Root cause: `actor_kind` returned "human" on a `User` typename / `is_bot: False` before ever consulting `extra_bot_logins`, so threading the config in was provably inert (a populated config still returned "human"). That defeats the field's own documented purpose — "accounts whose metadata misreports them as users" — and disagrees with `is_bot`, which already ignores any non-`Bot` typename. Consequence: the configured approver's own clean review body could count as a human blocking comment against the very approval it provides, and a configured automation identity could ratify a decision-default marker — automation clearing its own veto, the #450 attribution-drift hazard the ratification design explicitly excludes (Codex P2 on #665, thread PRRT_kwDOTCGFQM6SOgye). Fix, fail-closed: `actor_kind` now lets an explicitly configured `extra_bot_logins` identity outrank a `User` typename, matching `is_bot` and honoring the field's contract; default-config callers are unchanged. The tier threads a single automation config (`approver_bot_logins | lane_logins`) into both the human-blocking scan and the decision-default ratification scan via a new `AutopilotMergeTierConfig.automation_actor_config` property, so a configured identity is always automation for those scans — never a human veto, never a ratifier. Fixtures added: classifier-level precedence contract; a configured `User`-type approver's blocking-looking review body does not block; a configured `User`-type lane account with an OWNER association does not ratify a decision default; unconfigured humans unchanged. Co-authored-by: Claude Opus 4.8 --- .../babysit-prs/scripts/babysit_classify.py | 14 ++++++- .../babysit-prs/scripts/babysit_merge.py | 42 ++++++++++++++++--- .../scripts/tests/test_babysit_classify.py | 20 +++++++++ .../scripts/tests/test_babysit_merge.py | 34 +++++++++++++++ 4 files changed, 102 insertions(+), 8 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py index 2fe07bc31..2a18c1832 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py @@ -212,18 +212,28 @@ def is_bot( def actor_kind( item: dict[str, Any], config: FeedbackConfig = DEFAULT_FEEDBACK_CONFIG ) -> str: - """Classify actors from authoritative type metadata, then exact fallbacks.""" + """Classify actors from authoritative type metadata, then exact fallbacks. + + An explicitly configured `extra_bot_logins` identity outranks a `User` + typename: a bot account whose metadata misreports it as a user is the one + case that field exists for, so the operator's declaration wins over the + structural signal, matching `is_bot`, which already ignores any non-`Bot` + typename. An unconfigured caller (the empty default) never reaches this + override and classifies from structure alone. + """ author = item.get("author") + login = author_login(item).casefold() if is_json_object(author): typename = str(author.get("__typename") or "") if typename == "Bot" or author.get("is_bot") is True: return "bot" + if config.extra_bot_logins and is_bot(login, None, config.extra_bot_logins): + return "bot" if ( typename in {"Mannequin", "Organization", "User"} or author.get("is_bot") is False ): return "human" - login = author_login(item).casefold() return "bot" if is_bot(login, None, config.extra_bot_logins) else "human" diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index e7440f967..319f28737 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -50,7 +50,9 @@ from babysit_checks import check_identity_key, classify_checks from babysit_classify import ( + DEFAULT_FEEDBACK_CONFIG, NON_APPROVAL_RE, + FeedbackConfig, actor_kind, has_blocking_severity, has_blocking_text, @@ -107,6 +109,23 @@ class AutopilotMergeTierConfig: approver_bot_logins: frozenset[str] block_labels: frozenset[str] + @property + def automation_actor_config(self) -> FeedbackConfig: + """Classify every configured pipeline identity as a bot for the veto and + ratification scans. + + A lane or approver account GitHub misreports as a `User` (no `[bot]` + suffix) passes the structural `is_bot` fallback in + `find_distinct_bot_approval` but would otherwise read as a human here: a + configured identity is *always* automation for these scans -- never a + human merge veto, never a maintainer ratifying its own decision-default + marker (the #450 attribution-drift hazard the ratification design + excludes). + """ + return FeedbackConfig( + extra_bot_logins=self.approver_bot_logins | self.lane_logins + ) + def parse_csv_set(raw: str | None) -> set[str]: if not raw: @@ -266,7 +285,11 @@ def find_distinct_bot_approval( ) -def _decision_default_ratified(comments: list[dict[str, Any]], marker_ts: str) -> bool: +def _decision_default_ratified( + comments: list[dict[str, Any]], + marker_ts: str, + config: FeedbackConfig = DEFAULT_FEEDBACK_CONFIG, +) -> bool: """True when a maintainer's latest decisive comment after the marker ratifies. Every human-maintainer comment posted strictly after the marker is scanned and @@ -294,7 +317,7 @@ def _decision_default_ratified(comments: list[dict[str, Any]], marker_ts: str) - created_at = str(comment.get("createdAt") or "") if created_at <= marker_ts: continue - if actor_kind(comment) != "human": + if actor_kind(comment, config) != "human": continue association = str(comment.get("authorAssociation") or "").upper() if association not in RATIFYING_ASSOCIATIONS: @@ -324,7 +347,9 @@ def _ref_repo(ref: dict[str, Any]) -> str | None: def evaluate_decision_default_veto( - repo: str, closing_issues: list[Any] + repo: str, + closing_issues: list[Any], + config: FeedbackConfig = DEFAULT_FEEDBACK_CONFIG, ) -> tuple[list[str], list[str]]: """Hold when a linked issue carries an unratified 'Decision defaulted' marker. @@ -368,7 +393,7 @@ def evaluate_decision_default_veto( ] if not marker_timestamps: continue - if _decision_default_ratified(comments, max(marker_timestamps)): + if _decision_default_ratified(comments, max(marker_timestamps), config): continue blockers.append( f"linked issue {target} carries an unratified 'Decision defaulted' " @@ -453,8 +478,13 @@ def evaluate_autopilot_tier( {"author": normalized_rest_author(row), "body": row.get("body")} for row in fetch_pull_request_review_comments(repo, number) ) + # A configured approver/lane account GitHub misreports as a `User` classifies + # as a bot here, so its clean review body ("no blocking issues") no longer + # self-blocks the very approval `find_distinct_bot_approval` accepted -- + # extending the existing "a bot's blocking-looking prose is not a human stop" + # rule to configured bots that lack a `[bot]` suffix. for item in corpus: - if actor_kind(item) != "human": + if actor_kind(item, tier.automation_actor_config) != "human": continue body = str(item.get("body") or "") if ( @@ -473,7 +503,7 @@ def evaluate_autopilot_tier( ) veto_blockers, decision_default_held = evaluate_decision_default_veto( - repo, closing_issues + repo, closing_issues, tier.automation_actor_config ) blockers.extend(veto_blockers) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py index 38989501c..72625bc46 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_classify.py @@ -41,6 +41,26 @@ def test_non_string_login_without_bot_typename_is_not_a_bot(self) -> None: self.assertFalse(bc.is_bot(None, "User")) +class ActorKindTests(unittest.TestCase): + def test_configured_login_outranks_user_typename(self) -> None: + # A bot account GitHub misreports as a `User` (no `[bot]` suffix) is a bot + # when named in extra_bot_logins: the operator's declaration wins over the + # structural signal, matching `is_bot`. This is the one case the field + # documents itself for, so `actor_kind` must not settle it as human first. + config = bc.FeedbackConfig(extra_bot_logins=frozenset({"svc-account"})) + item = {"author": {"login": "svc-account", "__typename": "User", "is_bot": False}} + self.assertEqual(bc.actor_kind(item, config), "bot") + + def test_unconfigured_user_is_human(self) -> None: + config = bc.FeedbackConfig(extra_bot_logins=frozenset({"svc-account"})) + item = {"author": {"login": "maintainer", "__typename": "User", "is_bot": False}} + self.assertEqual(bc.actor_kind(item, config), "human") + + def test_default_empty_config_relies_on_structure(self) -> None: + item = {"author": {"login": "svc-account", "__typename": "User", "is_bot": False}} + self.assertEqual(bc.actor_kind(item), "human") + + class SelfLoginTests(unittest.TestCase): def test_normalize_casefolds_and_keeps_bot_suffix(self) -> None: # The self set is matched against a raw author login; stripping [bot] diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 1c723dbf4..52617e931 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -482,6 +482,40 @@ def test_bot_comment_with_blocking_prose_does_not_block(self) -> None: self.assertEqual(result["autopilotMergeTier"]["humanBlockingComments"], []) self.assertTrue(result["ready"], result["blockers"]) + def test_configured_user_approver_body_is_not_a_human_block(self) -> None: + # The configured distinct-bot approver GitHub reports as a `User` (no + # [bot] suffix): its own approving review body carrying blocking-looking + # prose must not count as a human stop against the very approval it + # provides. The same review satisfies find_distinct_bot_approval and no + # longer self-blocks the merge. + review = _approval(APPROVER, HEAD, typename="User") + review["body"] = ( + "Approved. This change resolves the blocking regression " + "from the linked issue." + ) + result = self._evaluate(_pr(), reviews=[review]) + self.assertTrue(result["ready"], result["blockers"]) + self.assertEqual(result["autopilotMergeTier"]["humanBlockingComments"], []) + self.assertIsNotNone(result["autopilotMergeTier"]["distinctBotApproval"]) + + def test_configured_user_lane_does_not_ratify_decision_default(self) -> None: + # A pipeline lane account GitHub reports as a `User` is automation for the + # ratification scan: its "ratified" comment after the marker must not + # clear the decision-default veto (the #450 hazard of automation ratifying + # its own default). The OWNER association proves the bot classification -- + # not the association -- holds it. + ratify = _comment( + LANE, "Ratified — proceed.", typename="User", association="OWNER", + created_at="2026-02-01T00:00:00Z", + ) + result = self._evaluate( + _pr(), linked_issue_comments=[DECISION_MARKER, ratify] + ) + self.assertFalse(result["ready"]) + self.assertEqual( + result["autopilotMergeTier"]["decisionDefaultHeldIssues"], [LINKED_REF] + ) + class TierAbsentIsInert(TierEvaluateHarness): def test_no_tier_makes_no_tier_network_calls(self) -> None: From ab9d9e567bea8343f841b0fda70cf3e13efe1ecc Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 12:51:14 -0400 Subject: [PATCH 10/11] test(source-control): assert not-ready on case-insensitive do-not-merge label match --- .../skills/babysit-prs/scripts/tests/test_babysit_merge.py | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 52617e931..91cd1db6b 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -207,6 +207,7 @@ def test_blocking_label_blocks(self) -> None: def test_label_match_is_case_insensitive(self) -> None: result = self._evaluate(_pr(labels=[{"name": "Do-Not-Merge"}])) + self.assertFalse(result["ready"]) self.assertEqual( result["autopilotMergeTier"]["blockingLabels"], ["do-not-merge"] ) From b3bedc92e1eb22e623367d99ad7d5c6b308f5ad6 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 20 Jul 2026 14:09:38 -0400 Subject: [PATCH 11/11] fix(source-control): reject approver-bot approvals that report blocking findings The #476 autopilot merge tier accepted a distinct-bot APPROVED review on identity/head alone. The human-blocking corpus scan deliberately skips bot-authored items and GitHub can still return reviewDecision=APPROVED, so an approver bot that approved while its own body reported a P1/CRITICAL/blocking regression sailed through the gate -- a fail-open in a merge gate. Severity-scan the accepted approval's own body: a structured high-severity finding (CRITICAL/IMPORTANT surviving negation redaction, or a P0-P3 severity badge / bracketed [P0-P3] marker) invalidates it as a tier approval. Treated as no approval, not as a human blocker, and a since-superseded earlier clean approval is not honored past the latest blocking verdict. Only the shared classifier's structured severity vocabulary is scanned -- prose words ("blocking"/"regression") are not, so a clean approval describing the fix it signs off stays accepted. This is the autopilot-tier answer to the open #621 question of whether formal APPROVED-state reviews should be severity-scanned. --- .../babysit-prs/scripts/babysit_merge.py | 45 ++++++++- .../scripts/tests/test_babysit_merge.py | 96 +++++++++++++++++++ 2 files changed, 139 insertions(+), 2 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 319f28737..d0c978f28 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -23,7 +23,8 @@ - The #476 autopilot merge tier (`--autopilot-merge-tier`) layers five extra criteria on top of the base gate -- issue-linked, lane-authored, no blocking label, a distinct-bot approving review on the live head (author != approver via - bot identity, unchanged since review), and no human blocking comment. It is + bot identity, unchanged since review, no blocking finding in its own body), and + no human blocking comment. It is fail-closed: the umbrella flag refuses to run unless `--lane-logins`, `--approver-bot-logins`, and `--block-labels` are all non-empty. Any criterion failing is just another blocker, so the caller falls back to the human @@ -52,6 +53,8 @@ from babysit_classify import ( DEFAULT_FEEDBACK_CONFIG, NON_APPROVAL_RE, + SEVERITY_BADGE_RE, + SEVERITY_PLAIN_RE, FeedbackConfig, actor_kind, has_blocking_severity, @@ -220,6 +223,29 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: return summary +def approval_reports_blocking(body: str) -> bool: + """True when an approving review's own body reports a live blocking finding. + + A distinct-bot approval can ratify the live head while its body raises a + structured high-severity finding; the human-blocking corpus scan deliberately + skips bot-authored items and GitHub can still return `reviewDecision=APPROVED`, + so without this check an approve-with-blocking-findings verdict would merge. + Only the shared classifier's *structured* severity vocabulary counts -- a + CRITICAL/IMPORTANT marker surviving negation redaction (`has_blocking_severity`), + or a P0-P3 severity badge / bracketed `[P0-P3]` marker. Prose severity words + (`has_blocking_text`'s "blocking"/"regression"/"must fix") are intentionally not + scanned: a clean approval routinely describes the fix it signs off ("resolves + the blocking regression"), so keying on prose would over-hold legitimate + approvals. This is the autopilot-tier answer to the open #621 question of + whether formal APPROVED-state reviews should be severity-scanned. + """ + return ( + has_blocking_severity(body) + or bool(SEVERITY_BADGE_RE.search(body)) + or bool(SEVERITY_PLAIN_RE.search(body)) + ) + + def find_distinct_bot_approval( reviews: list[dict[str, Any]], author_login: str | None, @@ -458,7 +484,22 @@ def evaluate_autopilot_tier( approval = find_distinct_bot_approval( decisive_reviews, author_login, head, tier.approver_bot_logins ) - if approval is None: + if approval is not None and approval_reports_blocking( + str(approval.get("body") or "") + ): + # The latest distinct-bot approval ratifies the live head but its own body + # raises a structured high-severity finding. An approve-with-blocking- + # findings verdict is not a clean tier approval, so it counts as no + # approval (not a human blocker) -- a since-superseded earlier clean + # approval must not be honored past the latest blocking verdict. See #621. + blockers.append( + "distinct-bot approving review reports blocking findings in its body " + "(CRITICAL/IMPORTANT or a P0-P3 severity marker) -- an " + "approve-with-blocking-findings verdict is not a clean tier approval, " + "so it is treated as no approval" + ) + approval = None + elif approval is None: blockers.append( "no distinct-bot approving review on the live head " "(need author != approver via bot identity, approval unchanged since head)" diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 91cd1db6b..68d3c6375 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -518,6 +518,79 @@ def test_configured_user_lane_does_not_ratify_decision_default(self) -> None: ) +class TierApprovalBodySeverityGate(TierEvaluateHarness): + """The accepted approver's own review body is severity-scanned (#621). + + A distinct-bot approval can ratify the live head while its body reports a live + blocking finding; the human-blocking corpus scan skips bot-authored items and + reviewDecision can stay APPROVED, so an approve-with-blocking-findings verdict + would merge. The tier invalidates such an approval -- treated as no approval, + never as a human block -- keying only on the shared classifier's structured + severity vocabulary (CRITICAL/IMPORTANT, P0-P3 badge, bracketed [P0-P3]). + """ + + def _approval_body(self, body: str) -> dict[str, Any]: + review = _approval(f"{APPROVER}[bot]", HEAD) + review["body"] = body + return review + + def _assert_rejected(self, result: dict[str, Any]) -> None: + self.assertIsNone(result["autopilotMergeTier"]["distinctBotApproval"]) + self.assertFalse(result["ready"]) + # Treated as no approval, not a human blocker. + self.assertEqual(result["autopilotMergeTier"]["humanBlockingComments"], []) + self.assertTrue( + any("blocking findings in its body" in b for b in result["blockers"]), + result["blockers"], + ) + + def test_approval_with_shields_p1_badge_body_rejected(self) -> None: + # A Codex-style shields.io P1 badge in the approval body is a structured + # blocking finding; the approve-with-P1-body verdict is not clean (#621). + review = self._approval_body( + "Approved overall. " + "![P1 Badge](https://img.shields.io/badge/P1-orange?style=flat) " + "this introduces an authorization-bypass regression." + ) + self._assert_rejected(self._evaluate(_pr(), reviews=[review])) + + def test_approval_with_bracketed_p1_body_rejected(self) -> None: + review = self._approval_body("Approved. [P1] auth bypass introduced here.") + self._assert_rejected(self._evaluate(_pr(), reviews=[review])) + + def test_approval_with_critical_body_rejected(self) -> None: + review = self._approval_body("Approved. CRITICAL: authorization bypass.") + self._assert_rejected(self._evaluate(_pr(), reviews=[review])) + + def test_approval_with_important_body_rejected(self) -> None: + review = self._approval_body("Approving. IMPORTANT: unhandled error path.") + self._assert_rejected(self._evaluate(_pr(), reviews=[review])) + + def test_clean_approval_body_accepted(self) -> None: + review = self._approval_body("Approved. LGTM, no concerns.") + result = self._evaluate(_pr(), reviews=[review]) + self.assertTrue(result["ready"], result["blockers"]) + self.assertIsNotNone(result["autopilotMergeTier"]["distinctBotApproval"]) + + def test_negated_severity_approval_body_accepted(self) -> None: + # Negation redaction: a body stating the ABSENCE of high-severity findings + # is a clean approval, not a live one. + review = self._approval_body("Approved. No CRITICAL or IMPORTANT findings.") + result = self._evaluate(_pr(), reviews=[review]) + self.assertTrue(result["ready"], result["blockers"]) + self.assertIsNotNone(result["autopilotMergeTier"]["distinctBotApproval"]) + + def test_lowercase_severity_prose_approval_accepted(self) -> None: + # Lowercase "critical"/"important" are ordinary prose, not structured + # severity labels: a clean approval describing a fix stays accepted. + review = self._approval_body( + "Approved. This resolves the blocking regression on the critical path." + ) + result = self._evaluate(_pr(), reviews=[review]) + self.assertTrue(result["ready"], result["blockers"]) + self.assertIsNotNone(result["autopilotMergeTier"]["distinctBotApproval"]) + + class TierAbsentIsInert(TierEvaluateHarness): def test_no_tier_makes_no_tier_network_calls(self) -> None: result = self._evaluate(_pr(), tier=None) @@ -555,5 +628,28 @@ def test_no_match_when_head_none(self) -> None: ) +class ApprovalReportsBlockingUnit(unittest.TestCase): + """The structured-severity scan of an approval body (#621).""" + + def test_structured_markers_are_blocking(self) -> None: + for body in ( + "CRITICAL: bug", + "IMPORTANT: bug", + "see [P0] here", + "see [P1] here", + "![P1](https://img.shields.io/badge/P1-orange)", + ): + self.assertTrue(merge.approval_reports_blocking(body), body) + + def test_clean_and_prose_are_not_blocking(self) -> None: + for body in ( + "", + "Approved. LGTM.", + "No CRITICAL or IMPORTANT findings.", + "resolves the blocking regression on the critical path", + ): + self.assertFalse(merge.approval_reports_blocking(body), body) + + if __name__ == "__main__": unittest.main()