diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index b70e384d5..c92e06c42 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.8", + "version": "0.51.9", "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 140a3220c..25c98f401 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,39 @@ 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.9] + +### 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. 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.8] ### 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 9256362b4..5dc860890 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,23 @@ 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. + + 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": [], "requiredApprovingReviews": 0, @@ -247,6 +263,9 @@ 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() + 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 @@ -257,17 +276,29 @@ 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) - ] - elif rtype == "pull_request": - summary["requiredApprovingReviews"] = params.get( - "required_approving_review_count", 0 + if isinstance(c, dict) and cast(dict[str, Any], c).get("context") ) - summary["requireThreadResolution"] = params.get( - "required_review_thread_resolution", False + elif rtype == "pull_request": + # 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) ) elif rtype == "required_signatures": summary["requireSignatures"] = True @@ -275,6 +306,9 @@ 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) + 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 new file mode 100644 index 000000000..7176ac951 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_branch_rules.py @@ -0,0 +1,237 @@ +"""Ruleset composition in `babysit_merge.branch_rules`. + +`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. +""" + +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 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"]) + + 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. + + 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: + 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()