From a053815807685c9c71d142e06fe05c9967dffca1 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:33:52 -0400 Subject: [PATCH 1/3] fix(source-control): union required contexts across composing rulesets `branch_rules` assigned `requiredContexts` inside the loop over `repos/{repo}/rules/branches/{branch}`, so each `required_status_checks` rule overwrote the previous one and only the last ruleset's contexts survived. That endpoint returns one such rule PER RULESET -- the single-rule shape classic branch protection always produced, and rulesets do not. On this repository the two rulesets governing `main` collapsed to the one returned last, dropping three of four required contexts from `effectiveRules` and from the unmet-required blocker. Accumulate into a set and report it sorted, so every ruleset's contexts survive, a context two rulesets both require is reported once, and the order is stable regardless of the order the API returns rulesets in. Entries carrying no `context` are dropped rather than sorted as `None`. `base_is_unprotected` needs no change and is now honest: it derives from the union, which is empty only when no ruleset requires anything. Under the overwrite it hung on whichever ruleset came last, so a trailing rule with an empty context list would have flipped it and silently retired the hold on a non-self-authored PR onto an unprotected base. Co-Authored-By: Claude Opus 5 (1M context) --- .../babysit-prs/scripts/babysit_merge.py | 26 ++- .../tests/test_babysit_merge_branch_rules.py | 157 ++++++++++++++++++ 2 files changed, 178 insertions(+), 5 deletions(-) create mode 100644 plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py 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 9256362b4..db9d7dd20 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -232,7 +232,18 @@ def repository_default_branch(repo: str) -> str | None: def branch_rules(repo: str, branch: str) -> dict[str, object]: - """Summarize the effective merge-governing rules for the base branch.""" + """Summarize the effective merge-governing rules for the base branch. + + `requiredContexts` is the UNION of every rule's contexts, deduped and + sorted. Rulesets compose: `/rules/branches/{branch}` returns one + `required_status_checks` rule per ruleset governing the branch, so the + single-rule assumption that held under classic branch protection does not + hold here -- keeping only one rule's list drops every other ruleset's + required contexts from both `effectiveRules` and the unmet-required + blocker. Two rulesets may legitimately require the same context, hence the + dedupe; the sort makes the reported set stable across runs regardless of + the order the API returns rulesets in. + """ summary: dict[str, object] = { "requiredContexts": [], "requiredApprovingReviews": 0, @@ -247,6 +258,7 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: # Rules are advisory context; a read failure must never fail the run. summary["error"] = f"could not read branch rules: {exc}" return summary + required_contexts: set[str] = set() for rule in cast(list[Any], rules) if isinstance(rules, list) else []: if not isinstance(rule, dict): continue @@ -257,11 +269,14 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: cast(dict[str, Any], raw_params) if isinstance(raw_params, dict) else {} ) if rtype == "required_status_checks": - summary["requiredContexts"] = [ - cast(dict[str, Any], c).get("context") + # A context-less entry is dropped rather than carried: it names no + # check to reconcile, and a None would sort-crash the union and + # surface downstream as a literal "None" required context. + required_contexts.update( + str(cast(dict[str, Any], c)["context"]) for c in params.get("required_status_checks", []) - if isinstance(c, dict) - ] + if isinstance(c, dict) and cast(dict[str, Any], c).get("context") + ) elif rtype == "pull_request": summary["requiredApprovingReviews"] = params.get( "required_approving_review_count", 0 @@ -275,6 +290,7 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: summary["requireLinearHistory"] = True elif rtype == "merge_queue": summary["mergeQueueRequired"] = True + summary["requiredContexts"] = sorted(required_contexts) return summary diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py new file mode 100644 index 000000000..c8673db1a --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py @@ -0,0 +1,157 @@ +"""Ruleset composition in `babysit_merge.branch_rules`. + +`repos/{repo}/rules/branches/{branch}` returns one `required_status_checks` +rule PER RULESET governing the branch, not one rule overall -- the shape +classic branch protection never produced. These tests pin the union: every +ruleset's contexts survive, a context required by two rulesets is reported +once, and a rule carrying an empty context list cannot erase the contexts an +earlier rule established (which would flip `baseUnprotected` and silently drop +the unprotected-base hold on a non-self-authored PR). + +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 +PR_NUMBER = 2157 +CI_GATE = ["pr-title / pr-title", "do-not-merge / do-not-merge", "ci-status"] +SECURITY_GATE = ["security-review / security-review"] + + +def _status_checks_rule(contexts: list[str]) -> dict[str, Any]: + return { + "type": "required_status_checks", + "parameters": { + "required_status_checks": [{"context": c} for c in contexts] + }, + } + + +# A base whose `pull_request` rule requires zero approving reviews, as this +# repository's does: `baseUnprotected` then hangs entirely on the context union. +NO_REQUIRED_REVIEWS = { + "type": "pull_request", + "parameters": {"required_approving_review_count": 0}, +} + + +def _rules(*rules: dict[str, Any]) -> list[dict[str, Any]]: + return list(rules) + + +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": "someone-else"}, + "url": "https://example/pr", + "title": "t", + "labels": [], + "statusCheckRollup": [], + "closingIssuesReferences": [], + } + pr.update(overrides) + return pr + + +class BranchRulesUnionsEveryRulesetsContexts(unittest.TestCase): + def _branch_rules(self, rules: list[dict[str, Any]]) -> dict[str, object]: + with mock.patch.object(merge, "gh_json", return_value=rules): + return merge.branch_rules("owner/repo", "main") + + def test_contexts_from_every_ruleset_survive(self) -> None: + summary = self._branch_rules( + _rules( + NO_REQUIRED_REVIEWS, + _status_checks_rule(CI_GATE), + _status_checks_rule(SECURITY_GATE), + ) + ) + self.assertEqual( + summary["requiredContexts"], sorted(CI_GATE + SECURITY_GATE) + ) + + def test_a_context_required_by_two_rulesets_is_reported_once(self) -> None: + summary = self._branch_rules( + _rules( + _status_checks_rule(["ci-status", "pr-title / pr-title"]), + _status_checks_rule(["ci-status"]), + ) + ) + self.assertEqual( + summary["requiredContexts"], ["ci-status", "pr-title / pr-title"] + ) + + def test_a_context_less_entry_is_dropped(self) -> None: + summary = self._branch_rules( + _rules( + { + "type": "required_status_checks", + "parameters": { + "required_status_checks": [{"integration_id": 1}] + }, + } + ) + ) + self.assertEqual(summary["requiredContexts"], []) + + +class AnEmptyLaterRuleCannotUnprotectTheBase(unittest.TestCase): + """The union keeps `baseUnprotected` honest, and with it the merge hold.""" + + def _evaluate(self, rules: list[dict[str, Any]]) -> dict[str, Any]: + def gh_json(args: list[str]) -> Any: + if args[:2] == ["pr", "view"]: + return _pr() + if args[0] == "api" and "/rules/branches/" in args[1]: + return rules + if args[0] == "api": # repository metadata (default-branch read) + return {"name": "main"} + 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=[]), + ): + return merge.evaluate( + "owner/repo", PR_NUMBER, None, {"owner"}, frozenset(), False, False, + ) + + def test_empty_trailing_rule_leaves_the_base_protected(self) -> None: + result = self._evaluate( + _rules( + NO_REQUIRED_REVIEWS, + _status_checks_rule(["ci-status"]), + _status_checks_rule([]), + ) + ) + self.assertFalse(result["baseUnprotected"]) + self.assertEqual( + [b for b in result["blockers"] if "unprotected" in b], [] + ) + + def test_no_context_anywhere_still_reports_an_unprotected_base(self) -> None: + result = self._evaluate(_rules(NO_REQUIRED_REVIEWS, _status_checks_rule([]))) + self.assertTrue(result["baseUnprotected"]) + self.assertTrue([b for b in result["blockers"] if "unprotected" in b]) + + +if __name__ == "__main__": + unittest.main() From 474238018dfc161e00d7e2c643e7525207d45313 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:35:44 -0400 Subject: [PATCH 2/3] fix(source-control): fold pull_request rules across rulesets too `branch_rules` folded `required_status_checks` across every ruleset but still assigned the `pull_request` rule inside the same loop, so a second such rule would overwrite the first. Not observed misreporting: exactly one `pull_request` rule governs the branch today. Nothing prevents a second, and `requiredApprovingReviews` feeds both `base_is_unprotected` and the "needs N approving review(s)" blocker, so a ruleset requiring two approvals returned before one requiring zero would have reported zero. Fold `requiredApprovingReviews` with max and `requireThreadResolution` with OR. That is the fail-closed direction whatever GitHub's own composition rule turns out to be, which is why it needs no appeal to one: max/OR can only over-report, holding a PR for a human, where last-wins can under-report and release one. The count is a behaviour change on a multi-ruleset base; `requireThreadResolution` is report-only, never consumed as a blocker, since the gate holds on unresolved threads unconditionally. Bump source-control 0.51.4 -> 0.51.5 with its CHANGELOG entry, per the plugin's convention of versioning each behaviour fix. Co-Authored-By: Claude Opus 5 (1M context) --- .../source-control/.claude-plugin/plugin.json | 2 +- plugins/source-control/CHANGELOG.md | 29 ++++++++++ .../babysit-prs/scripts/babysit_merge.py | 40 +++++++++----- .../tests/test_babysit_merge_branch_rules.py | 55 ++++++++++++++++--- 4 files changed, 104 insertions(+), 22 deletions(-) diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index 506dd596f..b70e384d5 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.51.7", + "version": "0.51.8", "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), /babysit-loop (the loop-lane merge lane: a standing or drain loop that invokes babysit-prs per cycle, configured through repo-scoped babysit_loop_* keys on the layered source-control.md seam, with merge authority human-only until the target repo's tracked config adopts the lane, a gate-proven C2-mechanical baseline once adopted, and standing merge-rung raises binding from the team-tracked layer only — with one named exception, where an invocation line explicitly typing both the autopilot tier keyword and the dedicated raise argument --merge c3-this-run widens that single invocation's merge authority up to C3 behind a fresh independent frontier-tier resolver, while C4-structural and C5-untrusted-provenance stay unconditionally human-merge), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention merged across its config layers and the babysit-prs config, or apply — interview the repo and write the convention config to a chosen layer), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep — never --abort). The commit-subject / PR-title convention is configurable via a source-control.md config written by a re-runnable setup skill, layered across a ~/.claude user-global file, the tracked team file, and a gitignored .claude/source-control.local.md personal overlay merged per key; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index d1787e7ea..cfbeb7b06 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,35 @@ 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.51.8] + +### Fixed + +- **`babysit_merge` no longer drops required status contexts when more than one ruleset governs the + base branch.** `branch_rules` assigned `requiredContexts` inside its loop over + `repos/{repo}/rules/branches/{branch}`, but that endpoint returns one rule of a given type PER + RULESET, so each `required_status_checks` rule overwrote the previous one and only the last + survived. On a branch governed by two rulesets this reported one of four required contexts, + under-reporting `effectiveRules` and the "required checks not satisfied" blocker. The single-rule + assumption was correct under classic branch protection, which has exactly one such rule, and does + not hold under rulesets. Contexts are now unioned across all rules, deduped (two rulesets may + legitimately require the same context) and sorted (stable regardless of the order rulesets are + returned in). An entry carrying no `context` is dropped rather than surfacing as a literal `None` + required context. Not a merge-safety hole: the gate refuses independently on `mergeStateStatus`, + which GitHub computes from all required checks. Its one safety-adjacent effect ran in the + over-holding direction — `baseUnprotected` is true when the context list is empty, which under + the bug meant "the LAST status-checks rule is empty" and now means "ALL of them are", a subset — + so the bug produced a false hold on a superset of cases and never retired one. Latent on this + repository, where neither ruleset carries an empty context list. +- **`pull_request` rules are folded across rulesets too.** Same assign-in-loop shape, same + function. `requiredApprovingReviews` now takes the max and `requireThreadResolution` the OR — the + fail-closed direction whatever GitHub's own composition rule is, since max/OR can only + over-report and hold a PR for a human, where last-wins can under-report and release one. This + one could lose a blocker outright: a trailing rule with `required_approving_review_count: 0` + erased an earlier ruleset's requirement and dropped the "needs N approving review(s)" hold. Not + observed — one such rule governs the branch today. The count fold is a behaviour change; the + boolean is report-only, never consumed as a blocker. + ## [0.51.7] ### Fixed 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 db9d7dd20..d105c83b7 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -234,15 +234,20 @@ def repository_default_branch(repo: str) -> str | None: def branch_rules(repo: str, branch: str) -> dict[str, object]: """Summarize the effective merge-governing rules for the base branch. - `requiredContexts` is the UNION of every rule's contexts, deduped and - sorted. Rulesets compose: `/rules/branches/{branch}` returns one - `required_status_checks` rule per ruleset governing the branch, so the - single-rule assumption that held under classic branch protection does not - hold here -- keeping only one rule's list drops every other ruleset's - required contexts from both `effectiveRules` and the unmet-required - blocker. Two rulesets may legitimately require the same context, hence the - dedupe; the sort makes the reported set stable across runs regardless of - the order the API returns rulesets in. + Rulesets COMPOSE: `/rules/branches/{branch}` returns one rule of a given + type PER RULESET governing the branch, so the single-rule assumption that + held under classic branch protection does not hold here. Every repeatable + rule is therefore folded across all rules rather than assigned from one: + + * `requiredContexts` is the union, deduped and sorted -- keeping a single + rule's list drops every other ruleset's contexts from both + `effectiveRules` and the unmet-required blocker. Two rulesets may + legitimately require the same context, hence the dedupe; the sort makes + the reported set stable regardless of the order rulesets are returned in. + * `requiredApprovingReviews` takes the max and `requireThreadResolution` + the OR. That is the fail-closed direction whatever GitHub's own + composition rule turns out to be: max/OR can only ever over-report, which + holds a PR for a human, where last-wins can under-report and release one. """ summary: dict[str, object] = { "requiredContexts": [], @@ -259,6 +264,8 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: summary["error"] = f"could not read branch rules: {exc}" return summary required_contexts: set[str] = set() + required_reviews = 0 + require_thread_resolution = False for rule in cast(list[Any], rules) if isinstance(rules, list) else []: if not isinstance(rule, dict): continue @@ -278,11 +285,16 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: if isinstance(c, dict) and cast(dict[str, Any], c).get("context") ) elif rtype == "pull_request": - summary["requiredApprovingReviews"] = params.get( - "required_approving_review_count", 0 + # An uninterpretable-but-present count reads as one review, never + # as zero: reading it as zero would be the one fail-OPEN step in a + # fold whose whole argument is that it can only ever over-report. + count = params.get("required_approving_review_count", 0) + required_reviews = max( + required_reviews, + count if isinstance(count, int) else (1 if count else 0), ) - summary["requireThreadResolution"] = params.get( - "required_review_thread_resolution", False + require_thread_resolution = require_thread_resolution or bool( + params.get("required_review_thread_resolution", False) ) elif rtype == "required_signatures": summary["requireSignatures"] = True @@ -291,6 +303,8 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: elif rtype == "merge_queue": summary["mergeQueueRequired"] = True summary["requiredContexts"] = sorted(required_contexts) + summary["requiredApprovingReviews"] = required_reviews + summary["requireThreadResolution"] = require_thread_resolution return summary diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py index c8673db1a..9da4f071d 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py @@ -1,12 +1,13 @@ """Ruleset composition in `babysit_merge.branch_rules`. -`repos/{repo}/rules/branches/{branch}` returns one `required_status_checks` -rule PER RULESET governing the branch, not one rule overall -- the shape -classic branch protection never produced. These tests pin the union: every -ruleset's contexts survive, a context required by two rulesets is reported -once, and a rule carrying an empty context list cannot erase the contexts an -earlier rule established (which would flip `baseUnprotected` and silently drop -the unprotected-base hold on a non-self-authored PR). +`repos/{repo}/rules/branches/{branch}` returns one rule of a given type PER +RULESET governing the branch, not one rule overall -- the shape classic branch +protection never produced. These tests pin the fold: every ruleset's contexts +survive, a context required by two rulesets is reported once, and a rule +carrying an empty context list cannot erase the contexts an earlier rule +established (which would flip `baseUnprotected` and silently drop the +unprotected-base hold on a non-self-authored PR). `pull_request` rules compose +the same way, folded max/OR so the summary can only ever over-report. Network is stubbed by monkeypatching `babysit_merge`'s gh seams; no real gh process is spawned. @@ -113,8 +114,46 @@ def test_a_context_less_entry_is_dropped(self) -> None: self.assertEqual(summary["requiredContexts"], []) +class PullRequestRulesFoldFailClosed(unittest.TestCase): + """`pull_request` composes too; max/OR can only ever hold more, never less.""" + + def _branch_rules(self, rules: list[dict[str, Any]]) -> dict[str, object]: + with mock.patch.object(merge, "gh_json", return_value=rules): + return merge.branch_rules("owner/repo", "main") + + def test_the_strictest_approval_count_wins(self) -> None: + summary = self._branch_rules( + _rules( + { + "type": "pull_request", + "parameters": {"required_approving_review_count": 2}, + }, + NO_REQUIRED_REVIEWS, + ) + ) + self.assertEqual(summary["requiredApprovingReviews"], 2) + + def test_thread_resolution_required_by_any_ruleset_survives(self) -> None: + summary = self._branch_rules( + _rules( + { + "type": "pull_request", + "parameters": {"required_review_thread_resolution": True}, + }, + NO_REQUIRED_REVIEWS, + ) + ) + self.assertTrue(summary["requireThreadResolution"]) + + class AnEmptyLaterRuleCannotUnprotectTheBase(unittest.TestCase): - """The union keeps `baseUnprotected` honest, and with it the merge hold.""" + """The union keeps `baseUnprotected` honest, and with it the merge hold. + + Only the first case regresses. `test_no_context_anywhere_...` passes against + the unfixed code too, by design: it is the over-correction guard, pinning + that a genuinely context-less base still reports unprotected. Do not count + it among the regression tests. + """ def _evaluate(self, rules: list[dict[str, Any]]) -> dict[str, Any]: def gh_json(args: list[str]) -> Any: From 1c17be55645a7d63473d638868582c64d649f7ae Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 11 Aug 2026 18:53:22 -0400 Subject: [PATCH 3/3] fix(source-control): count an unreadable review requirement as one, not zero The previous guard read `count if isinstance(count, int) else (1 if count else 0)`, so every FALSY non-int -- `None`, `""`, `0.0`, `[]`, `{}` -- still collapsed to zero. That is the same fail-open the guard was added to close, only narrower, and `None` is the realistic case: a ruleset payload carrying `required_approving_review_count` with a null value. The comment above it asserted an invariant the code did not deliver. Absence and unreadability are now distinguished, which is the distinction the previous two attempts blurred. No key means the rule states no review requirement, which is genuinely zero. A key holding anything unreadable as a count means a requirement IS stated and its size is unknown, so it counts as one and holds the PR for a human. `int()` normalizes the value so a `bool` -- an `int` subclass -- cannot leak into the summary as `True`. Pinned by tests over all five falsy non-ints plus the absent and bool cases, so the fold's only-ever-over-report guarantee is now checked rather than asserted in a comment. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/source-control/CHANGELOG.md | 6 ++- .../babysit-prs/scripts/babysit_merge.py | 20 +++++---- .../tests/test_babysit_merge_branch_rules.py | 41 +++++++++++++++++++ 3 files changed, 58 insertions(+), 9 deletions(-) diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index cfbeb7b06..093571dd6 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -30,7 +30,11 @@ All notable changes to the `source-control` plugin are documented here. Format f one could lose a blocker outright: a trailing rule with `required_approving_review_count: 0` erased an earlier ruleset's requirement and dropped the "needs N approving review(s)" hold. Not observed — one such rule governs the branch today. The count fold is a behaviour change; the - boolean is report-only, never consumed as a blocker. + boolean is report-only, never consumed as a blocker. The count also distinguishes an ABSENT + `required_approving_review_count` (the rule requires no reviews — zero) from one present but + unreadable (`null`, `""`, `0.0`, `[]`, `{}` — a requirement is stated and its size is unknown, so + it counts as one). Collapsing a falsy non-int to zero would be the single fail-open step in a + fold whose guarantee is that it may only ever over-report. ## [0.51.7] 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 d105c83b7..5dc860890 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -285,14 +285,18 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: if isinstance(c, dict) and cast(dict[str, Any], c).get("context") ) elif rtype == "pull_request": - # An uninterpretable-but-present count reads as one review, never - # as zero: reading it as zero would be the one fail-OPEN step in a - # fold whose whole argument is that it can only ever over-report. - count = params.get("required_approving_review_count", 0) - required_reviews = max( - required_reviews, - count if isinstance(count, int) else (1 if count else 0), - ) + # Absence and unreadability are different facts. No key means the + # rule requires no reviews, which is 0. A key holding anything this + # code cannot read as a count -- null, "", 0.0, [], {} -- means a + # requirement IS stated and its size is unknown, so it counts as + # one: a falsy non-int must not collapse to 0, which would be the + # single fail-OPEN step in a fold that may only ever over-report. + if "required_approving_review_count" not in params: + count = 0 + else: + raw = params["required_approving_review_count"] + count = int(raw) if isinstance(raw, int) else 1 + required_reviews = max(required_reviews, count) require_thread_resolution = require_thread_resolution or bool( params.get("required_review_thread_resolution", False) ) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py index 9da4f071d..7176ac951 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py @@ -145,6 +145,47 @@ def test_thread_resolution_required_by_any_ruleset_survives(self) -> None: ) self.assertTrue(summary["requireThreadResolution"]) + def test_an_unreadable_count_counts_as_one_not_zero(self) -> None: + """Every FALSY non-int too -- the shape that keeps re-opening this hole. + + `None` is the realistic one: a ruleset payload carrying the key with a + null value. A truthiness test reads all of these as zero, which is the + fail-open this fold exists to prevent. + """ + for raw in (None, "", 0.0, [], {}, "two", object()): + with self.subTest(raw=raw): + summary = self._branch_rules( + _rules( + { + "type": "pull_request", + "parameters": { + "required_approving_review_count": raw + }, + } + ) + ) + self.assertEqual(summary["requiredApprovingReviews"], 1) + + def test_an_absent_count_is_zero_not_one(self) -> None: + """Absence is readable: the rule states no review requirement.""" + summary = self._branch_rules( + _rules({"type": "pull_request", "parameters": {}}) + ) + self.assertEqual(summary["requiredApprovingReviews"], 0) + + def test_a_readable_count_is_reported_as_an_int(self) -> None: + """`bool` is an `int` subclass; it must not leak into the summary.""" + summary = self._branch_rules( + _rules( + { + "type": "pull_request", + "parameters": {"required_approving_review_count": True}, + } + ) + ) + self.assertIs(type(summary["requiredApprovingReviews"]), int) + self.assertEqual(summary["requiredApprovingReviews"], 1) + class AnEmptyLaterRuleCannotUnprotectTheBase(unittest.TestCase): """The union keeps `baseUnprotected` honest, and with it the merge hold.