From 412b922db274276cb6163399a8657a05a920b6d8 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 4 Oct 2026 03:30:39 +0000 Subject: [PATCH 1/3] fix(source-control): hold a CLEAN babysit head that is behind a loose base Under loose required status checks GitHub reports a behind head CLEAN, and the gate compared the head against its base only on BLOCKED, so it could squash-merge a behind head. The merge gate now compares an otherwise-ready head against the live base on a base with neither strict checks nor a merge queue, and holds it while behind or unreadable. The snapshot reports the same head behind so the guarded refresh can clear the hold; a queue base is left as before. Closes #5955 Co-authored-by: ksextonmelodic --- .../source-control/.claude-plugin/plugin.json | 2 +- plugins/source-control/CHANGELOG.md | 7 + .../skills/babysit-prs/reference/freshness.md | 45 +++- .../skills/babysit-prs/reference/safety.md | 16 +- .../babysit-prs/scripts/babysit_delta.py | 48 ++--- .../skills/babysit-prs/scripts/babysit_gh.py | 86 ++++++-- .../babysit-prs/scripts/babysit_merge.py | 42 +++- .../scripts/babysit_resolve_thread.py | 2 +- .../babysit-prs/scripts/request_review.py | 6 +- .../scripts/tests/test_babysit_delta.py | 46 +++- .../scripts/tests/test_babysit_gh.py | 71 +++++++ .../scripts/tests/test_babysit_merge.py | 16 ++ .../scripts/tests/test_babysit_merge_async.py | 6 + .../test_babysit_merge_base_freshness.py | 201 ++++++++++++++++++ .../tests/test_babysit_merge_branch_rules.py | 4 + .../tests/test_babysit_merge_review_settle.py | 6 + .../scripts/tests/test_review_trigger_race.py | 2 +- 17 files changed, 540 insertions(+), 66 deletions(-) create mode 100644 plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_base_freshness.py diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index a2867e5976..bd437926b2 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "source-control", - "version": "0.79.7", + "version": "0.79.8", "description": "Git and GitHub delivery: /commit (convention-checked subject, Co-authored-by trailer, surgical staging), /pull-request (prep, create, CI monitoring, review triage, merge, CI logs), /babysit-prs (safe-by-default PR fleet loop, opt-in worker and autopilot tiers), /babysit-loop (merge lane; merge is human until the repo adopts it), /worktree, /resolve-conflicts (intent-first), /check, and /setup (layered source-control.md convention config; Conventional Commits by default).", "author": { "name": "Melodic Software", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 4f2d5a6ee1..4e8a094b60 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,13 @@ 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.79.8] - 2026-10-04 + +### Fixed + +- **The babysit merge gate holds a `CLEAN` head that is behind its base ([#5955](https://github.com/melodic-software/claude-code-plugins/issues/5955)).** + Under loose required status checks GitHub reports a behind head `CLEAN`, and the gate compared the head against its base only when `mergeStateStatus` was `BLOCKED`, so it could squash-merge a behind head and drop base commits. Once a PR is otherwise ready (or held only by running checks), the gate now compares its head against the live base and holds it while behind, or while the compare cannot be read, and reports the result as `baseFreshness`. A base with strict required checks or a merge queue makes no compare: GitHub reports `BEHIND` on the first, and the queue tests the merged result on the second. The queue snapshot reports the same `CLEAN` or `HAS_HOOKS` head as `branch_freshness.state == "behind"` so the guarded refresh can clear the hold, reading the base's rules only for a behind head to leave a queue base as before. + ## [0.79.7] - 2026-10-04 ### Fixed diff --git a/plugins/source-control/skills/babysit-prs/reference/freshness.md b/plugins/source-control/skills/babysit-prs/reference/freshness.md index 0b7ebcdf0e..3420441799 100644 --- a/plugins/source-control/skills/babysit-prs/reference/freshness.md +++ b/plugins/source-control/skills/babysit-prs/reference/freshness.md @@ -25,13 +25,22 @@ missing from the branch), yet `mergeStateStatus` reported `BLOCKED`, never `BEHI only ever matched the literal string `BEHIND` could never open for that PR, a chicken-and-egg an automated queue cannot break out of on its own. -The snapshot engine closes that gap with one narrow, evidence-based fallback: when -`mergeStateStatus` is `BLOCKED`, it compares the base ref against the head SHA via GitHub's own -`GET /repos/{owner}/{repo}/compare/{basehead}`. If the compare proves outstanding base commits -(`status` in `behind`/`diverged` and `behind_by > 0`), the PR is classified -`branch_freshness.state == "behind"` (`source: "compare_api"`) exactly as if `mergeStateStatus` -had reported `BEHIND` directly. Any other cause of `BLOCKED`, a real merge conflict, a pending -human review, anything else, is untouched: the fallback only ever flips `BLOCKED` to `behind`, +`BEHIND` is also reported only where the base requires branches to be up to date. Under loose +required status checks GitHub merges a behind head, so a behind head with every other gate met +reads `CLEAN` (or `HAS_HOOKS`). + +The snapshot engine closes both gaps with one narrow, evidence-based fallback: when +`mergeStateStatus` is `BLOCKED`, `CLEAN`, or `HAS_HOOKS`, it compares the base ref against the +head SHA via GitHub's own `GET /repos/{owner}/{repo}/compare/{basehead}`. If the compare proves +outstanding base commits (`status` in `behind`/`diverged` and `behind_by > 0`), the PR is +classified `branch_freshness.state == "behind"` (`source: "compare_api"`) exactly as if +`mergeStateStatus` had reported `BEHIND` directly. A behind `CLEAN`/`HAS_HOOKS` head is the one +exception: its base's rules are read too, and on a base that requires a merge queue it stays +`not_reported_behind`, because the queue tests the PR against the latest base itself and needs no +branch update. An unreadable rules answer counts as no queue, since refreshing a behind branch is +always safe. Any +other cause of `BLOCKED`, a real merge conflict, a pending +human review, anything else, is untouched: the fallback only ever flips these states to `behind`, never invents eligibility the compare API did not prove, and every other invariant below (conflict check, human-review stop, worker lease, unique head ref, the per-source-SHA refresh ledger) is still enforced completely independently, on both the stored snapshot and a live @@ -66,6 +75,22 @@ observations, reproducible with the single-PR diagnostic below: read `mergeState refresh guarantee for `baseRefOid`, a GitHub changelog entry naming either, or a diagnostic run where `BLOCKED` no longer co-occurs with a positive `behind_by`. +### Verification record for the loose-base and merge-queue claims + +**Claims.** A base with loose required status checks lets a behind head merge, and a base that +requires a merge queue gives the same up-to-date guarantee without the branch being updated. + +**Basis.** The strict and loose rows of +[require status checks before merging](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging), +and [about merge queues](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#about-merge-queues). +The rulesets field read for the strict setting is `strict_required_status_checks_policy` on a +`required_status_checks` rule, as `GET /repos/{owner}/{repo}/rules/branches/{branch}` returns it. + +**Verified.** 2026-10-04, against both pages and a live rules read as of that day. + +**Recheck trigger.** Either page changing what the loose setting or a merge queue guarantees, or +the rules endpoint renaming the strict field. + ## Orchestrator-Only Refresh Procedure Only the orchestrator may refresh a branch: @@ -122,7 +147,11 @@ Squash-merging while the head is behind its base can silently drop commits that base after the PR branched, including the tests that covered them, with CI green throughout. Treat `branch_freshness.state == "behind"` as a hard stop on the merge path even when GitHub reports `mergeStateStatus` `CLEAN`/`HAS_HOOKS`: under a non-strict ruleset, GitHub does not itself -refuse a behind-base merge, so CLEAN does **not** imply an up-to-date base. +refuse a behind-base merge, so CLEAN does **not** imply an up-to-date base. The merge gate enforces +this on its own: on a base with neither strict required checks nor a merge queue, it compares an +otherwise-ready head against the live base and holds it while behind or while the compare cannot +be read (`baseFreshness` in its output). That is one extra API call per otherwise-ready PR, and +none on a strict or queue base. Before any merge: diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index d810c30318..059eaa3c2d 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -466,7 +466,8 @@ auto-mode safety classifier and blocks the call before the wrapper runs. --self-logins @me,` (thread list). - **What the merge gate actually evaluates.** It gates on GitHub's own `mergeStateStatus == CLEAN` plus explicit cross-checks of its own: branch rules, review decision, unresolved threads, the - check rollup keyed by check type and name, and head match. It reports the exact `blockers` list. + check rollup keyed by check type and name, head match, and base freshness (next bullet). It + reports the exact `blockers` list. React to those blockers; never bypass the gate. One reading caveat: a `ready: false` immediately following a `ready: true` on the same expected head is often GitHub's own mergeability recompute lag, so re-run the read-only check once before treating it as a real block. @@ -475,12 +476,17 @@ auto-mode safety classifier and blocks the call before the wrapper runs. states that this leaves mergeability checks, conflict reporting, and rule enforcement unchanged. So the gate keeps trusting `CLEAN` for mergeability; what can be up to 12 hours behind the base is the merge commit `pull_request` CI ran against. Only a strict up-to-date rule (`BEHIND`) proves - the head is current at merge; under a non-strict base the stale-base rule in - [freshness.md](freshness.md) is the guard, and the gate adds no hold of its own. **Claim, basis, - as of, recheck:** that regeneration rule, + the head is current at merge. Under a base with neither that rule nor a merge queue, a behind + head still reads `CLEAN`, so once a PR is otherwise ready the gate compares its head against the + live base and holds it while it is behind, or while the compare cannot be read (`baseFreshness` + in the output). The snapshot reports the same head `branch_freshness.state == "behind"`, and + [freshness.md](freshness.md)'s refresh clears the hold. A merge-queue base makes no compare: the + queue tests the PR against the latest base itself. **Claim, basis, as of, recheck:** that + regeneration rule, [changes to test merge commit generation](https://github.blog/changelog/2026-02-19-changes-to-test-merge-commit-generation-for-pull-requests), 2026-10-02, and a GitHub changelog entry that changes test-merge regeneration or says it now - affects mergeability. + affects mergeability. The loose-base and merge-queue claims carry their own record in + [freshness.md](freshness.md#verification-record-for-the-loose-base-and-merge-queue-claims). - **`--self-logins @me,` rides on every merge form too**, read-only and mutating alike. `@me` resolves to your own `gh` login and the `babysit_self_logins` extras follow it; drop the trailing `,` when that value is empty. On the merge gate this flag is what diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py index 5637552d8c..5befafc4d7 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py @@ -28,7 +28,7 @@ normalize_self_logins, ) from babysit_feedback import collect_feedback, human_stop_from_feedback -from babysit_gh import find_open_prs_for_head_ref +from babysit_gh import compare_shows_behind, find_open_prs_for_head_ref from babysit_review_trigger import ( DEFAULT_REVIEW_TRIGGER_CONFIG, ReviewTriggerConfig, @@ -233,20 +233,23 @@ def validated_stuck_check_age_seconds(value: float) -> float: def compute_branch_freshness(pr: dict[str, Any]) -> dict[str, Any]: """Classify branch staleness from `mergeStateStatus`, with one fallback. - Pure function: the only I/O this depends on (the BLOCKED-branch compare) - happens once, in `view_pr`, and is read here off `pr["_blocked_base_compare"]`. - This keeps classification network-free and keeps `view_pr` the single choke - point both the snapshot orchestrator and the branch-refresh CLI's - revalidation already call, so a live re-check gets the same enrichment for - free. - - Falls back to the compare-confirmed signal only when `mergeStateStatus` is - BLOCKED and the compare proves outstanding base commits (`behind_by > 0`, - `status` in {behind, diverged}). Every other cause of BLOCKED (a real merge - conflict, a pending human review, ...) is untouched by this function -- it - only ever flips BLOCKED to "behind"; conflict, human-stop, lease, unique - head-ref, and the per-source-SHA refresh ledger are all still enforced - independently by the caller. + Pure function: the only I/O this depends on (the base compare, and for a + behind CLEAN/HAS_HOOKS head the merge-queue rules read) happens once, in + `view_pr`, and is read here off `pr["_base_compare"]` and + `pr["_base_merge_queue"]`. This keeps classification network-free and keeps + `view_pr` the single choke point both the snapshot orchestrator and the + branch-refresh CLI's revalidation already call, so a live re-check gets the + same enrichment for free. + + Falls back to the compare-confirmed signal only when the compare proves + outstanding base commits (`behind_by > 0`, `status` in {behind, diverged}) + and `mergeStateStatus` is BLOCKED, or CLEAN/HAS_HOOKS on a base that does not + require a merge queue (a queue tests the PR against the latest base itself). + Every other cause of BLOCKED (a real merge conflict, a pending human review, + ...) is untouched by this function -- it only ever flips those states to + "behind"; conflict, human-stop, lease, unique head-ref, and the + per-source-SHA refresh ledger are all still enforced independently by the + caller. """ merge_state = str(pr.get("mergeStateStatus") or "").upper() mergeable = str(pr.get("mergeable") or "").upper() @@ -256,15 +259,12 @@ def compute_branch_freshness(pr: dict[str, Any]) -> dict[str, Any]: return {"state": "behind", "source": "mergeStateStatus"} if merge_state in {"", "UNKNOWN"}: return {"state": "unknown", "source": "mergeStateStatus"} - if merge_state == "BLOCKED": - compare = pr.get("_blocked_base_compare") - if ( - is_json_object(compare) - and compare.get("status") in {"behind", "diverged"} - and isinstance(compare.get("behind_by"), int) - and compare["behind_by"] > 0 - ): - return {"state": "behind", "source": "compare_api", "compare": compare} + compare = pr.get("_base_compare") + if compare_shows_behind(compare) and ( + merge_state == "BLOCKED" + or (merge_state in {"CLEAN", "HAS_HOOKS"} and not pr.get("_base_merge_queue")) + ): + return {"state": "behind", "source": "compare_api", "compare": compare} return {"state": "not_reported_behind", "source": "mergeStateStatus"} diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py index 547f1a2e9d..64b0a5d555 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py @@ -47,6 +47,10 @@ "url", ) VIEW_FIELDS = ",".join(VIEW_FIELD_NAMES) +# `mergeStateStatus` values under which `view_pr` compares the head against the +# live base: BLOCKED can mask BEHIND, and CLEAN/HAS_HOOKS do not imply an +# up-to-date head on a base that does not require one. +BASE_COMPARE_MERGE_STATES = frozenset({"BLOCKED", "CLEAN", "HAS_HOOKS"}) SEARCH_FIELDS = "number,repository,url,title,updatedAt,isDraft" RECONCILE_FIELDS = "number,url,title,updatedAt,isDraft" GITHUB_OWNER_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9-]*$") @@ -612,12 +616,15 @@ def view_pr(repo: str, number: int) -> dict[str, Any]: data["repo"] = repo data["_graphql_available"] = graphql_available data["baseRepositoryArchived"] = repository_is_archived(repo) - if str(data.get("mergeStateStatus") or "").upper() == "BLOCKED": - data["_blocked_base_compare"] = fetch_blocked_base_compare( - repo, - str(data.get("baseRefName") or ""), - str(data.get("headRefOid") or ""), - ) + merge_state = str(data.get("mergeStateStatus") or "").upper() + if merge_state in BASE_COMPARE_MERGE_STATES: + base_ref = str(data.get("baseRefName") or "") + compare = fetch_base_compare(repo, base_ref, str(data.get("headRefOid") or "")) + data["_base_compare"] = compare + # Only a mergeable head behind its base pays the rules read: BLOCKED + # keeps its queue-blind fallback, and an up-to-date head needs no answer. + if merge_state != "BLOCKED" and compare_shows_behind(compare): + data["_base_merge_queue"] = base_requires_merge_queue(repo, base_ref) return data @@ -788,17 +795,25 @@ def rest_view_pr(repo: str, number: int) -> dict[str, Any]: } -def fetch_blocked_base_compare( - repo: str, base_ref: str, head_sha: str +def fetch_base_compare( + repo: str, + base_ref: str, + head_sha: str, + *, + run_json: Callable[[list[str]], Any] | None = None, ) -> dict[str, Any] | None: - """Best-effort divergence check for a BLOCKED PR against the LIVE base tip. + """Best-effort divergence check for a PR head against the LIVE base tip. + + `mergeStateStatus` hides a behind head in two shapes. It is single-valued: + when a PR is both genuinely behind its base AND blocked by another gate + (failing required checks, missing review, ...), GitHub reports BLOCKED and + the BEHIND signal is lost -- observed live, not documented. And BEHIND is + reported only where the base requires branches to be up to date: loose + required checks let a behind head merge, so it reports CLEAN + (`reference/freshness.md` carries the source record). This recovers the + signal from GitHub's own compare endpoint for both. - GitHub's `mergeStateStatus` is a single-valued field: when a PR is both - genuinely behind its base AND blocked by another gate (failing required - checks, missing review, ...), GitHub reports BLOCKED and the BEHIND signal - is lost -- observed live, not documented. This recovers that signal from - GitHub's own compare endpoint so a stale-but-BLOCKED branch is not - permanently invisible to the refresh gate. + `run_json` lets the merge gate keep its own gh seam, as `view_pr_fields`. Compares against the base ref NAME, never the PR's cached `baseRefOid`: that field lags once the base branch advances past the PR's last sync @@ -807,18 +822,19 @@ def fetch_blocked_base_compare( `behind_by=11`, `status=diverged`, for the identical head commit). Only a ref NAME resolves to the live tip at query time. - Best-effort and fail-closed: any error returns None, and the caller falls - back to `not_reported_behind` -- identical to the no-fallback behavior. A - hiccup here must never fail the whole snapshot for that PR, and never - grants eligibility on uncertain data. + Best-effort: any error returns None. The snapshot then falls back to + `not_reported_behind` -- identical to the no-fallback behavior, so a hiccup + never fails the whole snapshot for that PR and never grants refresh + eligibility on uncertain data -- and the merge gate holds the PR instead. """ if not base_ref or not re.fullmatch(r"[0-9a-fA-F]{7,40}", head_sha): return None + runner = gh_json if run_json is None else run_json try: - data = gh_json( + data = runner( ["api", f"repos/{repo}/compare/{quote(base_ref, safe='')}...{head_sha}"] ) - except RuntimeError: + except (RuntimeError, json.JSONDecodeError): return None if not is_json_object(data): return None @@ -834,6 +850,34 @@ def fetch_blocked_base_compare( return {"status": status, "ahead_by": ahead_by, "behind_by": behind_by} +def compare_shows_behind(compare: Any) -> bool: + """Whether a `fetch_base_compare` result proves outstanding base commits.""" + return ( + is_json_object(compare) + and compare.get("status") in {"behind", "diverged"} + and isinstance(compare.get("behind_by"), int) + and compare["behind_by"] > 0 + ) + + +def base_requires_merge_queue(repo: str, base_ref: str) -> bool: + """Whether a ruleset requires a merge queue on the base branch. + + A queue tests the PR against the latest base itself, so a behind head on a + queue base needs no refresh (`reference/freshness.md` carries the source + record). An unreadable answer is False: refreshing a genuinely behind + branch is always safe. + """ + try: + rules = gh_json(["api", f"repos/{repo}/rules/branches/{quote(base_ref, safe='')}"]) + except (RuntimeError, json.JSONDecodeError): + return False + return any( + is_json_object(rule) and rule.get("type") == "merge_queue" + for rule in json_array(rules) + ) + + def flatten_paginated_items( value: Any, label: str, *, items_key: str | None = None ) -> list[dict[str, Any]]: 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 7e1f0fe582..2ac40937a2 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -42,6 +42,9 @@ A self-authored PR onto an unprotected NON-default base (a stack layer, or any feature-onto-feature merge) is held: the default branch's required checks never governed it. `--stacked-prs` replaces that hold for a native stack layer only. +- A head behind its base is held on a base that requires neither up-to-date + branches nor a merge queue, where GitHub reports a behind head `CLEAN`. One + base compare per otherwise-ready PR proves it; an unreadable compare holds. - A merge is held while a configured review bot still owes the LIVE head a review (`--review-bot-logins` with `--review-settle-minutes`, both or neither). A reviewer that re-reviews on push posts minutes after the head @@ -67,7 +70,8 @@ refuses the run at exit 2. Readiness is gated on GitHub's own `mergeStateStatus == CLEAN` (which integrates -required checks, up-to-date, approvals, and conversation resolution) plus +required checks, approvals, conversation resolution, and up-to-date where the +base requires it) plus explicit cross-checks so the *reason* for a block is always reported: the effective branch rules (`rules/branches`), the review decision, unresolved review threads, and the status-check rollup. @@ -112,6 +116,8 @@ from babysit_feedback import latest_reviews_by_author from babysit_gh import ( GraphQLUnavailableError, + compare_shows_behind, + fetch_base_compare, fetch_issue_comments, fetch_pull_request_commits, fetch_pull_request_review_comments, @@ -342,6 +348,8 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: `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. + * `requireUpToDate` (strict required status checks) is the OR: one active + strict ruleset is enough for GitHub to report a behind head BEHIND. * `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 @@ -354,6 +362,7 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: "requireSignatures": False, "requireLinearHistory": False, "mergeQueueRequired": False, + "requireUpToDate": False, } try: # `{branch}` is one path parameter. Percent-encode it, including `/`, @@ -384,6 +393,8 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: for c in params.get("required_status_checks", []) if is_json_object(c) and c.get("context") ) + if params.get("strict_required_status_checks_policy") is True: + summary["requireUpToDate"] = True 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 @@ -1439,6 +1450,34 @@ def evaluate( "governed this merge -- held (pass --allow-unprotected to override)" ) + # CLEAN proves an up-to-date head only where the base requires one: under + # loose required checks GitHub merges a behind head, and a squash of it can + # drop base commits. A merge queue tests the PR against the latest base + # itself, so a queue base needs no compare. Evaluated after every other + # hold but a running check, for the per-cycle cost reason above, and before + # `--auto` arms over those running checks. + freshness: dict[str, Any] = {"checked": False, "compare": None, "behind": None} + if ( + not merge_queue_required + and not rules.get("requireUpToDate") + and all(b in waiting for b in blockers) + ): + compare = fetch_base_compare(repo, base_ref, str(head or ""), run_json=gh_json) + behind = compare_shows_behind(compare) + freshness = {"checked": True, "compare": compare, "behind": behind} + if compare is None: + blockers.append( + f"head could not be compared against base {base_ref!r} -- the base " + "does not require up-to-date branches, so CLEAN does not prove this " + "head current; freshness is UNPROVEN, held" + ) + elif behind: + blockers.append( + f"head is {compare['behind_by']} commit(s) behind base {base_ref!r}, " + "which does not require up-to-date branches -- refresh the branch " + "before merging" + ) + # The layers below a stack layer land with it, so each runs the full gate -- # only once this PR is otherwise ready, for the same per-cycle cost reason. stack_result: dict[str, Any] = { @@ -1517,6 +1556,7 @@ def evaluate_layer(layer_number: int, layer_head: str) -> dict[str, Any]: "expectedHead": expected_head, "headMatches": head_matches, "effectiveRules": rules, + "baseFreshness": freshness, "requiredSignatures": signature_result, "requiredChecks": required_check_status, "graphqlAvailable": graphql_available, diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py index e180aa1101..50f29044d1 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py @@ -702,7 +702,7 @@ def verify_fix_commit(repo: str, number: int, sha: str) -> tuple[bool, str]: ): return False, "refused-evidence-unverifiable" # Every segment interpolated into the compare path is FORMAT-VALIDATED first, - # matching `babysit_gh.fetch_blocked_base_compare`'s rule for the identical + # matching `babysit_gh.fetch_base_compare`'s rule for the identical # call shape. Two of the three arrive in an API response body, so "the API # said so" is their only provenance: a crafted or compromised response # carrying path syntax would otherwise redirect this request to an diff --git a/plugins/source-control/skills/babysit-prs/scripts/request_review.py b/plugins/source-control/skills/babysit-prs/scripts/request_review.py index 850c4984cf..33f23bfaf0 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/request_review.py +++ b/plugins/source-control/skills/babysit-prs/scripts/request_review.py @@ -101,9 +101,9 @@ def validate_current_candidate( raise RuntimeError("PR head changed after the snapshot") merge_state = str(current.get("mergeStateStatus") or "").upper() mergeable = str(current.get("mergeable") or "").upper() - # BLOCKED can mask BEHIND: a head behind its base still reports BLOCKED, not - # BEHIND. Reuse the compare-confirmed freshness signal (`view_pr` already - # enriched `_blocked_base_compare`) so a stale-behind head is rejected here + # BLOCKED can mask BEHIND, and a loose base reports a behind head CLEAN. + # Reuse the compare-confirmed freshness signal (`view_pr` already + # enriched `_base_compare`) so a stale-behind head is rejected here # and the branch-refresh flow runs first, instead of spending the one-shot # review request on a SHA that is about to be rebuilt. behind = compute_branch_freshness(current)["state"] == "behind" diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_delta.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_delta.py index 9d4f439ffe..717b958b19 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_delta.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_delta.py @@ -127,7 +127,7 @@ def test_unknown_merge_state_is_unknown(self) -> None: def test_blocked_with_compare_confirmed_divergence_is_behind(self) -> None: pr = make_pr( mergeStateStatus="BLOCKED", - _blocked_base_compare={"status": "behind", "behind_by": 3}, + _base_compare={"status": "behind", "behind_by": 3}, ) result = delta.compute_branch_freshness(pr) self.assertEqual(result["state"], "behind") @@ -137,6 +137,50 @@ def test_blocked_without_compare_signal_is_not_reported_behind(self) -> None: result = delta.compute_branch_freshness(make_pr(mergeStateStatus="BLOCKED")) self.assertEqual(result["state"], "not_reported_behind") + def test_clean_head_behind_a_queue_less_base_is_behind(self) -> None: + for state in ("CLEAN", "HAS_HOOKS"): + with self.subTest(state=state): + pr = make_pr( + mergeStateStatus=state, + _base_compare={"status": "diverged", "behind_by": 4}, + _base_merge_queue=False, + ) + result = delta.compute_branch_freshness(pr) + self.assertEqual(result["state"], "behind") + self.assertEqual(result["source"], "compare_api") + + def test_clean_head_behind_a_queue_base_is_not_reported_behind(self) -> None: + pr = make_pr( + _base_compare={"status": "behind", "behind_by": 4}, + _base_merge_queue=True, + ) + result = delta.compute_branch_freshness(pr) + self.assertEqual(result["state"], "not_reported_behind") + + def test_clean_head_up_to_date_is_not_reported_behind(self) -> None: + pr = make_pr(_base_compare={"status": "ahead", "behind_by": 0}) + result = delta.compute_branch_freshness(pr) + self.assertEqual(result["state"], "not_reported_behind") + + def test_unstable_head_ignores_a_compare(self) -> None: + pr = make_pr( + mergeStateStatus="UNSTABLE", + _base_compare={"status": "behind", "behind_by": 4}, + ) + result = delta.compute_branch_freshness(pr) + self.assertEqual(result["state"], "not_reported_behind") + + def test_clean_head_behind_routes_to_the_refresh_not_the_gate(self) -> None: + pr = make_pr( + _base_compare={"status": "behind", "behind_by": 4}, + _base_merge_queue=False, + ) + result = classify(pr, make_prev()) + self.assertIn( + "branch behind main; reported CLEAN (confirmed via base compare)", + result["blockers"], + ) + class MutationPolicyTests(unittest.TestCase): def test_same_repo_pr_allows_branch_writes(self) -> None: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py index 4c050287a5..6752eebc4e 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py @@ -697,6 +697,77 @@ def run_json(args: list[str]) -> Any: self.assertIn("404", str(caught.exception)) +class ViewPrBaseCompareTests(unittest.TestCase): + """`view_pr` compares a BLOCKED or mergeable head against the live base, and + reads the base's merge-queue rule only for a mergeable head that is behind.""" + + HEAD = "a" * 40 + BEHIND = {"status": "behind", "ahead_by": 0, "behind_by": 2} + CURRENT = {"status": "ahead", "ahead_by": 1, "behind_by": 0} + + def _view( + self, + merge_state: str, + compare: dict[str, Any], + rules: list[dict[str, Any]] | None = None, + ) -> tuple[dict[str, Any], list[str]]: + paths: list[str] = [] + + def gh_json(args: list[str]) -> Any: + if args[:2] == ["pr", "view"]: + return { + "mergeStateStatus": merge_state, + "baseRefName": "main", + "headRefOid": self.HEAD, + } + paths.append(args[1]) + if "/compare/" in args[1]: + return compare + if "/rules/branches/" in args[1]: + return rules or [] + raise AssertionError(f"unexpected gh_json call: {args}") + + with ( + mock.patch.object(gh, "gh_json", side_effect=gh_json), + mock.patch.object(gh, "repository_is_archived", return_value=False), + ): + return gh.view_pr("owner/repo", 7), paths + + def test_a_clean_head_behind_a_queue_less_base_carries_both_reads(self) -> None: + data, paths = self._view("CLEAN", self.BEHIND) + self.assertEqual(data["_base_compare"], self.BEHIND) + self.assertIs(data["_base_merge_queue"], False) + self.assertEqual( + paths, + [ + f"repos/owner/repo/compare/main...{self.HEAD}", + "repos/owner/repo/rules/branches/main", + ], + ) + + def test_a_queue_rule_is_recorded(self) -> None: + data, _ = self._view("HAS_HOOKS", self.BEHIND, [{"type": "merge_queue"}]) + self.assertIs(data["_base_merge_queue"], True) + + def test_an_up_to_date_clean_head_pays_no_rules_read(self) -> None: + data, paths = self._view("CLEAN", self.CURRENT) + self.assertNotIn("_base_merge_queue", data) + self.assertEqual(paths, [f"repos/owner/repo/compare/main...{self.HEAD}"]) + + def test_a_blocked_head_keeps_its_queue_blind_fallback(self) -> None: + data, paths = self._view("BLOCKED", self.BEHIND) + self.assertEqual(data["_base_compare"], self.BEHIND) + self.assertNotIn("_base_merge_queue", data) + self.assertEqual(len(paths), 1) + + def test_other_merge_states_make_no_compare(self) -> None: + for state in ("BEHIND", "DIRTY", "UNSTABLE", "UNKNOWN", ""): + with self.subTest(state=state): + data, paths = self._view(state, self.BEHIND) + self.assertNotIn("_base_compare", data) + self.assertEqual(paths, []) + + class PaginatedShapeTests(unittest.TestCase): """`gh api --paginate --slurp` shapes each page like its own endpoint: an array per page for an array endpoint, an object per page for an object 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 cc29842828..9b5ed383f4 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 @@ -31,6 +31,8 @@ import refresh_pr_branch as refresh HEAD = "a" * 40 +# The base compare the freshness hold reads for an otherwise-ready PR. +UP_TO_DATE = {"status": "ahead", "ahead_by": 1, "behind_by": 0} STALE = "b" * 40 LANE = "lane-bot" APPROVER = "approver-bot" @@ -148,6 +150,8 @@ def _evaluate( tier: merge.AutopilotMergeTierConfig | None = TIER, ) -> dict[str, Any]: def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return pr if args[0] == "api": # branch rules @@ -760,6 +764,8 @@ def _evaluate(self, extra: frozenset[str]) -> dict[str, Any]: pr = _pr(author={"login": self.DEP_BOT}, reviewDecision="APPROVED") def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return pr if args[0] == "api": @@ -837,6 +843,8 @@ def _evaluate( repo_calls: list[list[str]] = [] def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return pr if args[0] == "api" and args[1] == "repos/owner/repo": @@ -951,6 +959,8 @@ def _evaluate( pr = _pr(**pr_overrides) def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return pr if args[0] == "api": # branch rules @@ -1128,6 +1138,8 @@ def _evaluate( ) def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: requested.extend(args[args.index("--json") + 1].split(",")) if graphql_refused: @@ -1250,6 +1262,8 @@ def _evaluate(self, rollup: list[dict[str, Any]], **pr: Any) -> dict[str, Any]: view = _pr(statusCheckRollup=rollup, reviewDecision="", **pr) def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return view if args[0] == "api": @@ -1614,6 +1628,8 @@ def _run_real_gate( self, ref: str, pr: dict[str, Any], *flags: str ) -> tuple[int, dict[str, Any]]: def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return pr if args[0] == "api": diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_async.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_async.py index 1eefe2ac7f..00e3308070 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_async.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_async.py @@ -30,6 +30,8 @@ import babysit_merge as merge HEAD = "c" * 40 +# The base compare the freshness hold reads for an otherwise-ready PR. +UP_TO_DATE = {"status": "ahead", "ahead_by": 1, "behind_by": 0} LAYER1 = "1" * 40 LAYER2 = "2" * 40 UUID = "3f2c9a1e-0000-4000-8000-000000000001" @@ -142,6 +144,8 @@ def capture(cmd: list[str]) -> Any: return _proc() def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if "merge-async/" in args[1]: self.polls.append(args) answer = poll_answers.pop(0) @@ -1214,6 +1218,8 @@ def _member( def _evaluate(self, *, stacked: bool = True, number: int = 3) -> dict[str, Any]: def gh_json(args: list[str]) -> Any: self.calls.append(args) + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return self.views[int(args[2])] path = args[1] diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_base_freshness.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_base_freshness.py new file mode 100644 index 0000000000..c67a46a6e8 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_base_freshness.py @@ -0,0 +1,201 @@ +"""The merge gate's hold on a head behind its base (#5955). + +Under loose required status checks GitHub reports a behind head `CLEAN`, so +`mergeStateStatus` alone lets the gate merge it, and a squash of a behind head +can drop base commits. The gate compares the head against the live base once +the PR is otherwise ready, unless the base requires up-to-date branches +(GitHub then reports BEHIND itself) or a merge queue (which tests the merged +result itself). + +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 = 5955 +UP_TO_DATE = {"status": "ahead", "ahead_by": 1, "behind_by": 0} +BEHIND = {"status": "diverged", "ahead_by": 1, "behind_by": 3} + + +def _status_checks(*, strict: bool) -> dict[str, Any]: + return { + "type": "required_status_checks", + "parameters": { + "required_status_checks": [{"context": "ci-status"}], + "strict_required_status_checks_policy": strict, + }, + } + + +LOOSE = [_status_checks(strict=False)] +STRICT = [_status_checks(strict=True)] +QUEUE = [_status_checks(strict=False), {"type": "merge_queue", "parameters": {}}] + + +def _check(name: str, conclusion: str | None) -> dict[str, Any]: + return { + "__typename": "CheckRun", + "name": name, + "status": "COMPLETED" if conclusion else "IN_PROGRESS", + "conclusion": conclusion or "", + } + + +def _pr(**overrides: Any) -> dict[str, Any]: + pr: dict[str, Any] = { + "state": "OPEN", + "isDraft": False, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviewDecision": "", + "headRefOid": HEAD, + "baseRefName": "main", + "author": {"login": "someone-else"}, + "url": "https://example/pr", + "title": "t", + "labels": [], + "statusCheckRollup": [_check("ci-status", "SUCCESS")], + "closingIssuesReferences": [], + } + pr.update(overrides) + return pr + + +class BaseFreshnessHarness(unittest.TestCase): + def _evaluate( + self, + rules: list[dict[str, Any]], + compare: Any = UP_TO_DATE, + **pr_overrides: Any, + ) -> dict[str, Any]: + self.compare_calls: list[list[str]] = [] + + def gh_json(args: list[str]) -> Any: + if args[:2] == ["pr", "view"]: + return _pr(**pr_overrides) + if args[0] == "api" and "/compare/" in args[1]: + self.compare_calls.append(args) + if isinstance(compare, Exception): + raise compare + return compare + if args[0] == "api" and "/rules/branches/" in args[1]: + return rules + if args[0] == "api" and args[1] == "repos/owner/repo": + 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, HEAD, {"owner"}, frozenset(), False, False, + ) + + def _freshness_blockers(self, result: dict[str, Any]) -> list[str]: + return [b for b in result["blockers"] if "base 'main'" in b] + + +class LooseBaseHoldsABehindCleanHead(BaseFreshnessHarness): + def test_clean_head_behind_a_loose_base_is_held(self) -> None: + result = self._evaluate(LOOSE, BEHIND) + self.assertFalse(result["ready"]) + self.assertEqual( + self._freshness_blockers(result), + [ + "head is 3 commit(s) behind base 'main', which does not require " + "up-to-date branches -- refresh the branch before merging" + ], + ) + self.assertTrue(result["baseFreshness"]["behind"]) + self.assertEqual( + self.compare_calls, [["api", f"repos/owner/repo/compare/main...{HEAD}"]] + ) + + def test_has_hooks_head_behind_a_loose_base_is_held(self) -> None: + result = self._evaluate(LOOSE, BEHIND, mergeStateStatus="HAS_HOOKS") + self.assertFalse(result["ready"]) + self.assertTrue(self._freshness_blockers(result)) + + def test_clean_head_up_to_date_with_a_loose_base_is_ready(self) -> None: + result = self._evaluate(LOOSE, UP_TO_DATE) + self.assertTrue(result["ready"], result["blockers"]) + self.assertEqual( + result["baseFreshness"], + {"checked": True, "compare": UP_TO_DATE, "behind": False}, + ) + self.assertEqual(len(self.compare_calls), 1) + + def test_an_unreadable_compare_holds(self) -> None: + result = self._evaluate(LOOSE, RuntimeError("gh: Server Error (HTTP 502)")) + self.assertFalse(result["ready"]) + self.assertTrue( + any("freshness is UNPROVEN" in b for b in result["blockers"]), + result["blockers"], + ) + + def test_auto_merge_is_not_armed_over_a_behind_head(self) -> None: + # Only a running required check holds the merge, which `--auto` may arm + # over; on a loose base GitHub would then merge the behind head itself. + result = self._evaluate( + LOOSE, + BEHIND, + mergeStateStatus="BLOCKED", + statusCheckRollup=[ + _check("ci-status", None), + _check("claude-review-status", "SUCCESS"), + _check("claude-security-review-status", "SUCCESS"), + ], + ) + self.assertFalse(result["autoMerge"]["ready"]) + self.assertTrue( + any("behind base" in b for b in result["autoMerge"]["blockers"]), + result["autoMerge"], + ) + + +class BasesThatProveFreshnessThemselvesMakeNoCompare(BaseFreshnessHarness): + def test_a_merge_queue_base_is_unchanged(self) -> None: + result = self._evaluate(QUEUE, BEHIND) + self.assertTrue(result["ready"], result["blockers"]) + self.assertEqual(result["mergeAction"], "merge_queue") + self.assertEqual(self.compare_calls, []) + self.assertFalse(result["baseFreshness"]["checked"]) + + def test_a_strict_base_makes_no_compare(self) -> None: + result = self._evaluate(STRICT, BEHIND) + self.assertTrue(result["ready"], result["blockers"]) + self.assertEqual(self.compare_calls, []) + + def test_a_pr_already_held_pays_no_compare(self) -> None: + result = self._evaluate(LOOSE, BEHIND, isDraft=True) + self.assertFalse(result["ready"]) + self.assertEqual(self.compare_calls, []) + + +class StrictRequiredChecksFold(unittest.TestCase): + def test_one_strict_ruleset_marks_the_base_up_to_date_required(self) -> None: + with mock.patch.object(merge, "gh_json", return_value=[*LOOSE, *STRICT]): + summary = merge.branch_rules("owner/repo", "main") + self.assertTrue(summary["requireUpToDate"]) + + def test_loose_rulesets_do_not(self) -> None: + with mock.patch.object(merge, "gh_json", return_value=LOOSE): + summary = merge.branch_rules("owner/repo", "main") + self.assertFalse(summary["requireUpToDate"]) + + +if __name__ == "__main__": + unittest.main() 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 83f5dea23b..c1a19f0137 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 @@ -26,6 +26,8 @@ import babysit_merge as merge HEAD = "a" * 40 +# The base compare the freshness hold reads for an otherwise-ready PR. +UP_TO_DATE = {"status": "ahead", "ahead_by": 1, "behind_by": 0} PR_NUMBER = 2157 CI_GATE = ["pr-title / pr-title", "do-not-merge / do-not-merge", "ci-status"] SECURITY_GATE = ["security-review / security-review"] @@ -172,6 +174,8 @@ class AnEmptyLaterRuleCannotUnprotectTheBase(unittest.TestCase): def _evaluate(self, *rules: dict[str, Any]) -> dict[str, Any]: def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return _pr() if args[0] == "api" and "/rules/branches/" in args[1]: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_review_settle.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_review_settle.py index 1a4ee5a822..8fa88123bf 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_review_settle.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_review_settle.py @@ -30,6 +30,8 @@ from repo_config_fake import RepoConfigFake HEAD = "a" * 40 +# The base compare the freshness hold reads for an otherwise-ready PR. +UP_TO_DATE = {"status": "ahead", "ahead_by": 1, "behind_by": 0} STALE = "b" * 40 REVIEWER = "chatgpt-codex-connector" PR_NUMBER = 1629 @@ -103,6 +105,8 @@ def _evaluate( def gh_json(args: list[str]) -> Any: calls.append(args) + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: base = pr if pr is not None else _pr() if check_starts is not None: @@ -445,6 +449,8 @@ def test_settle_and_tier_share_one_reviews_fetch(self) -> None: ) def gh_json(args: list[str]) -> Any: + if args[0] == "api" and "/compare/" in args[1]: + return UP_TO_DATE if args[:2] == ["pr", "view"]: return pr if args[0] == "api" and "/rules/branches/" in args[1]: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_review_trigger_race.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_review_trigger_race.py index 59316727c7..766dadd18f 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_review_trigger_race.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_review_trigger_race.py @@ -438,7 +438,7 @@ class ValidateCurrentCandidateFreshnessTests(unittest.TestCase): def test_blocked_head_masking_behind_is_rejected(self) -> None: pr = {"state": "OPEN", "headRefOid": HEAD, "isDraft": False, "mergeStateStatus": "BLOCKED", "mergeable": "MERGEABLE", - "_blocked_base_compare": {"status": "behind", "behind_by": 2}} + "_base_compare": {"status": "behind", "behind_by": 2}} with _entered(_candidate_patches(pr)[:2]): with self.assertRaises(RuntimeError) as ctx: request_review.validate_current_candidate( From 977a7ba11d3f756a4ca940bd30cc36ce17c0d374 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 4 Oct 2026 03:52:42 +0000 Subject: [PATCH 2/3] fix(source-control): tighten the babysit behind-base hold's rules reads and docs Count a strict required_status_checks rule only when it lists a check, since GitHub's strict setting takes no effect without one. Leave the merge-queue flag unknown on a failed rules read so the snapshot reports a CLEAN head not behind for that cycle instead of refreshing a possible queue base. Skip a behind head as a review-request candidate. State that the gate checks freshness at gate and arm time only, reads rulesets but not classic branch protection, and holds on a persistently failing compare until a human acts; correct the cost wording and a broken reflow. Co-authored-by: ksextonmelodic --- plugins/source-control/CHANGELOG.md | 2 +- .../skills/babysit-prs/reference/freshness.md | 31 +++++++--- .../skills/babysit-prs/reference/safety.md | 16 +++-- .../babysit-prs/scripts/babysit_delta.py | 11 +++- .../skills/babysit-prs/scripts/babysit_gh.py | 12 ++-- .../babysit-prs/scripts/babysit_merge.py | 17 ++++-- .../scripts/babysit_review_trigger.py | 6 +- .../scripts/tests/test_babysit_delta.py | 31 ++++++++++ .../scripts/tests/test_babysit_gh.py | 12 +++- .../test_babysit_merge_base_freshness.py | 58 ++++++++++++++++++- .../scripts/tests/test_review_trigger_race.py | 12 ++++ 11 files changed, 176 insertions(+), 32 deletions(-) diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 4e8a094b60..d4f8f4da31 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -8,7 +8,7 @@ All notable changes to the `source-control` plugin are documented here. Format f ### Fixed - **The babysit merge gate holds a `CLEAN` head that is behind its base ([#5955](https://github.com/melodic-software/claude-code-plugins/issues/5955)).** - Under loose required status checks GitHub reports a behind head `CLEAN`, and the gate compared the head against its base only when `mergeStateStatus` was `BLOCKED`, so it could squash-merge a behind head and drop base commits. Once a PR is otherwise ready (or held only by running checks), the gate now compares its head against the live base and holds it while behind, or while the compare cannot be read, and reports the result as `baseFreshness`. A base with strict required checks or a merge queue makes no compare: GitHub reports `BEHIND` on the first, and the queue tests the merged result on the second. The queue snapshot reports the same `CLEAN` or `HAS_HOOKS` head as `branch_freshness.state == "behind"` so the guarded refresh can clear the hold, reading the base's rules only for a behind head to leave a queue base as before. + Under loose required status checks GitHub reports a behind head `CLEAN`. The gate made no base compare of its own and the snapshot compared only a `BLOCKED` head, so the gate could squash-merge a behind head and drop base commits. When the gate runs on a PR that is otherwise ready (or held only by running checks), it now compares the head against the live base and holds it if it is behind or the compare cannot be read, and reports the result as `baseFreshness`. The check runs at gate time, including when the gate arms `--auto`; an auto-merge already armed is not re-checked if the base moves afterwards, a race that predates this change. The gate makes no compare on a base whose rulesets require up-to-date branches (a strict `required_status_checks` rule that lists at least one check, since GitHub's strict setting takes no effect without one) or a merge queue; classic branch protection is not read. The queue snapshot now compares every `BLOCKED`, `CLEAN`, or `HAS_HOOKS` PR on every cycle and reports a behind `CLEAN` or `HAS_HOOKS` head as `branch_freshness.state == "behind"` so the guarded refresh can clear the hold. For that head it also reads the base's rules, and it reports the head behind only when the read succeeds and shows no merge queue: on a failed read the head stays not behind for that cycle, so a transient failure cannot start a refresh (which disarms auto-merge and reruns CI and the AI reviews) on a queue base. A compare that keeps failing on a loose base holds the PR until a human acts. The review-request candidate skips a head the snapshot reports `behind`, matching the live re-check in `request_review.py`. ## [0.79.7] - 2026-10-04 diff --git a/plugins/source-control/skills/babysit-prs/reference/freshness.md b/plugins/source-control/skills/babysit-prs/reference/freshness.md index 3420441799..cdf7da549d 100644 --- a/plugins/source-control/skills/babysit-prs/reference/freshness.md +++ b/plugins/source-control/skills/babysit-prs/reference/freshness.md @@ -35,11 +35,12 @@ head SHA via GitHub's own `GET /repos/{owner}/{repo}/compare/{basehead}`. If the outstanding base commits (`status` in `behind`/`diverged` and `behind_by > 0`), the PR is classified `branch_freshness.state == "behind"` (`source: "compare_api"`) exactly as if `mergeStateStatus` had reported `BEHIND` directly. A behind `CLEAN`/`HAS_HOOKS` head is the one -exception: its base's rules are read too, and on a base that requires a merge queue it stays -`not_reported_behind`, because the queue tests the PR against the latest base itself and needs no -branch update. An unreadable rules answer counts as no queue, since refreshing a behind branch is -always safe. Any -other cause of `BLOCKED`, a real merge conflict, a pending +exception: its base's rules are read too, and it is `behind` only when that read succeeds and +shows no merge queue. On a queue base it stays `not_reported_behind`, because the queue tests the +PR against the latest base itself and needs no branch update. On an unreadable rules answer it +also stays `not_reported_behind` for that cycle: the refresh disarms auto-merge and its push +reruns CI and the AI reviews, which a transient failure must not start on a queue base. The next +snapshot reads the rules again. Any other cause of `BLOCKED`, a real merge conflict, a pending human review, anything else, is untouched: the fallback only ever flips these states to `behind`, never invents eligibility the compare API did not prove, and every other invariant below (conflict check, human-review stop, worker lease, unique head ref, the per-source-SHA refresh @@ -148,10 +149,22 @@ base after the PR branched, including the tests that covered them, with CI green Treat `branch_freshness.state == "behind"` as a hard stop on the merge path even when GitHub reports `mergeStateStatus` `CLEAN`/`HAS_HOOKS`: under a non-strict ruleset, GitHub does not itself refuse a behind-base merge, so CLEAN does **not** imply an up-to-date base. The merge gate enforces -this on its own: on a base with neither strict required checks nor a merge queue, it compares an -otherwise-ready head against the live base and holds it while behind or while the compare cannot -be read (`baseFreshness` in its output). That is one extra API call per otherwise-ready PR, and -none on a strict or queue base. +this on its own: on a base whose rulesets require neither strict required checks (a strict rule +that lists at least one required check) nor a merge queue, it compares an otherwise-ready head +against the live base when it runs, and holds the head if it is behind or the compare cannot be +read (`baseFreshness` in its output). Only rulesets are read: classic branch protection is not, +so a base that is strict or queue-gated only through classic protection still gets the compare. + +The check runs only when the gate runs. Once the gate has armed auto-merge (`--auto`), GitHub +merges the PR when its checks pass without the gate running again, so a base that moves after +arming is not re-checked; that race predates this check. A compare that keeps failing on a loose +base holds the PR until a human acts, because the snapshot does not report the head `behind` +without a compare that proves it, so no refresh clears the hold. + +Cost: the gate makes one compare per otherwise-ready PR on a base with no ruleset strict or queue +rule, and none on one with either. The snapshot compares every `BLOCKED`, `CLEAN`, or `HAS_HOOKS` +PR on every cycle, whatever the base, and reads the base's rules once more for each `CLEAN` or +`HAS_HOOKS` head the compare shows behind. Before any merge: diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index 059eaa3c2d..80c79456d3 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -476,12 +476,16 @@ auto-mode safety classifier and blocks the call before the wrapper runs. states that this leaves mergeability checks, conflict reporting, and rule enforcement unchanged. So the gate keeps trusting `CLEAN` for mergeability; what can be up to 12 hours behind the base is the merge commit `pull_request` CI ran against. Only a strict up-to-date rule (`BEHIND`) proves - the head is current at merge. Under a base with neither that rule nor a merge queue, a behind - head still reads `CLEAN`, so once a PR is otherwise ready the gate compares its head against the - live base and holds it while it is behind, or while the compare cannot be read (`baseFreshness` - in the output). The snapshot reports the same head `branch_freshness.state == "behind"`, and - [freshness.md](freshness.md)'s refresh clears the hold. A merge-queue base makes no compare: the - queue tests the PR against the latest base itself. **Claim, basis, as of, recheck:** that + the head is current at merge. Under a base whose rulesets carry neither that rule nor a merge + queue, a behind head still reads `CLEAN`, so when the gate runs on an otherwise-ready PR it + compares the head against the live base and holds it if it is behind or the compare cannot be + read (`baseFreshness` in the output). The compare runs at gate time, including when the gate + arms `--auto`; an auto-merge already armed is not re-checked if the base moves afterwards, a + race that predates this check. The snapshot reports the same head + `branch_freshness.state == "behind"`, and [freshness.md](freshness.md)'s refresh clears the + hold; a compare that keeps failing holds the PR until a human acts. A merge-queue base makes no + compare: the queue tests the PR against the latest base itself. Only rulesets are read, not + classic branch protection. **Claim, basis, as of, recheck:** that regeneration rule, [changes to test merge commit generation](https://github.blog/changelog/2026-02-19-changes-to-test-merge-commit-generation-for-pull-requests), 2026-10-02, and a GitHub changelog entry that changes test-merge regeneration or says it now diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py index 5befafc4d7..c83ff17c6b 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_delta.py @@ -243,8 +243,9 @@ def compute_branch_freshness(pr: dict[str, Any]) -> dict[str, Any]: Falls back to the compare-confirmed signal only when the compare proves outstanding base commits (`behind_by > 0`, `status` in {behind, diverged}) - and `mergeStateStatus` is BLOCKED, or CLEAN/HAS_HOOKS on a base that does not - require a merge queue (a queue tests the PR against the latest base itself). + and `mergeStateStatus` is BLOCKED, or CLEAN/HAS_HOOKS on a base whose rules + were read and require no merge queue (a queue tests the PR against the + latest base itself; an unread answer leaves the head not behind this cycle). Every other cause of BLOCKED (a real merge conflict, a pending human review, ...) is untouched by this function -- it only ever flips those states to "behind"; conflict, human-stop, lease, unique head-ref, and the @@ -262,7 +263,10 @@ def compute_branch_freshness(pr: dict[str, Any]) -> dict[str, Any]: compare = pr.get("_base_compare") if compare_shows_behind(compare) and ( merge_state == "BLOCKED" - or (merge_state in {"CLEAN", "HAS_HOOKS"} and not pr.get("_base_merge_queue")) + or ( + merge_state in {"CLEAN", "HAS_HOOKS"} + and pr.get("_base_merge_queue") is False + ) ): return {"state": "behind", "source": "compare_api", "compare": compare} return {"state": "not_reported_behind", "source": "mergeStateStatus"} @@ -531,6 +535,7 @@ def classify_pr( reaction_signals, bool(human_stop["required"]), review_trigger_allowed=bool(mutation_policy["review_trigger_allowed"]), + branch_behind=branch_freshness["state"] == "behind", config=config.review_trigger, ) foreign_activity = detect_foreign_activity(pr, prev, config) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py index 64b0a5d555..ae6a8194a6 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py @@ -860,18 +860,22 @@ def compare_shows_behind(compare: Any) -> bool: ) -def base_requires_merge_queue(repo: str, base_ref: str) -> bool: +def base_requires_merge_queue(repo: str, base_ref: str) -> bool | None: """Whether a ruleset requires a merge queue on the base branch. A queue tests the PR against the latest base itself, so a behind head on a queue base needs no refresh (`reference/freshness.md` carries the source - record). An unreadable answer is False: refreshing a genuinely behind - branch is always safe. + record). An unreadable answer is None, not False: the refresh disarms + auto-merge and its push reruns CI and the AI reviews, so a transient read + failure must not start one on a queue base. The head reads not behind for + that cycle and the next snapshot reads the rules again. """ try: rules = gh_json(["api", f"repos/{repo}/rules/branches/{quote(base_ref, safe='')}"]) except (RuntimeError, json.JSONDecodeError): - return False + return None + if not is_json_array(rules): + return None return any( is_json_object(rule) and rule.get("type") == "merge_queue" for rule in json_array(rules) 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 2ac40937a2..c0b4e8c3d4 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -349,7 +349,8 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: legitimately require the same context, hence the dedupe; the sort makes the reported set stable regardless of the order rulesets are returned in. * `requireUpToDate` (strict required status checks) is the OR: one active - strict ruleset is enough for GitHub to report a behind head BEHIND. + strict rule that lists a required check is enough for GitHub to report + a behind head BEHIND. * `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 @@ -388,12 +389,18 @@ def branch_rules(repo: str, branch: str) -> dict[str, object]: # 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( + rule_contexts = [ str(c["context"]) - for c in params.get("required_status_checks", []) + for c in json_array(params.get("required_status_checks")) if is_json_object(c) and c.get("context") - ) - if params.get("strict_required_status_checks_policy") is True: + ] + required_contexts.update(rule_contexts) + # The strict setting "will not take effect unless at least one status + # check is enabled" (https://docs.github.com/en/rest/repos/rules). + if ( + params.get("strict_required_status_checks_policy") is True + and rule_contexts + ): summary["requireUpToDate"] = True elif rtype == "pull_request": # Absence and unreadability are different facts. No key means the diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py index 7202084f25..1df1ef9d80 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py @@ -406,13 +406,16 @@ def classify_review_request( human_stop_required: bool = False, *, review_trigger_allowed: bool = False, + branch_behind: bool = False, config: ReviewTriggerConfig = DEFAULT_REVIEW_TRIGGER_CONFIG, ) -> dict[str, Any]: """Advance the review-request state machine for one PR. `review_trigger_allowed` is the caller's already-computed mutation-policy verdict (base repo allowed and not archived); passing it in keeps this - module free of any trust-boundary policy import. + module free of any trust-boundary policy import. `branch_behind` is the + caller's `branch_freshness` verdict: a behind head is refreshed before it + is reviewed, and `request_review.py` rejects one on its live re-check. """ head_sha = str(pr.get("headRefOid") or "") merge_state = str(pr.get("mergeStateStatus") or "").upper() @@ -455,6 +458,7 @@ def classify_review_request( and head_sha not in refresh_history and not pr.get("isDraft") and merge_state not in {"", "BEHIND", "DIRTY", "DRAFT", "UNKNOWN"} + and not branch_behind and mergeable != "CONFLICTING" and not current_head_review # Only reactions tied to THIS head gate candidacy. Reactions carry no diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_delta.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_delta.py index 717b958b19..70810ccd2f 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_delta.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_delta.py @@ -12,6 +12,7 @@ import pathlib import sys import unittest +from unittest import mock sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) @@ -157,6 +158,21 @@ def test_clean_head_behind_a_queue_base_is_not_reported_behind(self) -> None: result = delta.compute_branch_freshness(pr) self.assertEqual(result["state"], "not_reported_behind") + def test_clean_head_behind_an_unread_queue_answer_is_not_reported_behind( + self, + ) -> None: + # The refresh disarms auto-merge and reruns CI and the AI reviews, so a + # failed rules read must not start one on what may be a queue base. + for queue in (None, "absent"): + with self.subTest(queue=queue): + over: dict[str, object] = { + "_base_compare": {"status": "behind", "behind_by": 4} + } + if queue is None: + over["_base_merge_queue"] = None + result = delta.compute_branch_freshness(make_pr(**over)) + self.assertEqual(result["state"], "not_reported_behind") + def test_clean_head_up_to_date_is_not_reported_behind(self) -> None: pr = make_pr(_base_compare={"status": "ahead", "behind_by": 0}) result = delta.compute_branch_freshness(pr) @@ -181,6 +197,21 @@ def test_clean_head_behind_routes_to_the_refresh_not_the_gate(self) -> None: result["blockers"], ) + def test_the_review_request_candidate_sees_the_behind_verdict(self) -> None: + for queue, behind in ((False, True), (True, False)): + with self.subTest(queue=queue): + pr = make_pr( + _base_compare={"status": "behind", "behind_by": 4}, + _base_merge_queue=queue, + ) + with mock.patch.object( + delta, + "classify_review_request", + wraps=delta.classify_review_request, + ) as spy: + classify(pr, make_prev()) + self.assertIs(spy.call_args.kwargs["branch_behind"], behind) + class MutationPolicyTests(unittest.TestCase): def test_same_repo_pr_allows_branch_writes(self) -> None: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py index 6752eebc4e..f7d201b68e 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py @@ -709,7 +709,7 @@ def _view( self, merge_state: str, compare: dict[str, Any], - rules: list[dict[str, Any]] | None = None, + rules: Any = None, ) -> tuple[dict[str, Any], list[str]]: paths: list[str] = [] @@ -724,6 +724,8 @@ def gh_json(args: list[str]) -> Any: if "/compare/" in args[1]: return compare if "/rules/branches/" in args[1]: + if isinstance(rules, Exception): + raise rules return rules or [] raise AssertionError(f"unexpected gh_json call: {args}") @@ -749,6 +751,14 @@ def test_a_queue_rule_is_recorded(self) -> None: data, _ = self._view("HAS_HOOKS", self.BEHIND, [{"type": "merge_queue"}]) self.assertIs(data["_base_merge_queue"], True) + def test_an_unreadable_rules_answer_leaves_the_queue_unknown(self) -> None: + # Unknown, not "no queue": the snapshot then reports the head not + # behind, so a transient failure cannot refresh a queue-base head. + for rules in (RuntimeError("gh: Server Error (HTTP 502)"), {"message": "x"}): + with self.subTest(rules=rules): + data, _ = self._view("CLEAN", self.BEHIND, rules) + self.assertIsNone(data["_base_merge_queue"]) + def test_an_up_to_date_clean_head_pays_no_rules_read(self) -> None: data, paths = self._view("CLEAN", self.CURRENT) self.assertNotIn("_base_merge_queue", data) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_base_freshness.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_base_freshness.py index c67a46a6e8..7a848915bf 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_base_freshness.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_base_freshness.py @@ -76,8 +76,9 @@ def _pr(**overrides: Any) -> dict[str, Any]: class BaseFreshnessHarness(unittest.TestCase): def _evaluate( self, - rules: list[dict[str, Any]], + rules: list[dict[str, Any]] | Exception, compare: Any = UP_TO_DATE, + self_logins: frozenset[str] = frozenset(), **pr_overrides: Any, ) -> dict[str, Any]: self.compare_calls: list[list[str]] = [] @@ -91,6 +92,8 @@ def gh_json(args: list[str]) -> Any: raise compare return compare if args[0] == "api" and "/rules/branches/" in args[1]: + if isinstance(rules, Exception): + raise rules return rules if args[0] == "api" and args[1] == "repos/owner/repo": return {"name": "main"} @@ -101,7 +104,7 @@ def gh_json(args: list[str]) -> Any: mock.patch.object(merge, "fetch_review_threads", return_value=[]), ): return merge.evaluate( - "owner/repo", PR_NUMBER, HEAD, {"owner"}, frozenset(), False, False, + "owner/repo", PR_NUMBER, HEAD, {"owner"}, self_logins, False, False, ) def _freshness_blockers(self, result: dict[str, Any]) -> list[str]: @@ -146,6 +149,39 @@ def test_an_unreadable_compare_holds(self) -> None: result["blockers"], ) + def test_an_unreadable_rules_answer_still_compares_and_holds(self) -> None: + # The snapshot reports this head not behind (no refresh this cycle), so + # the gate must not read the failure as a strict or queue base. A + # self-authored PR passes the unprotected-base hold that would + # otherwise hold it first. + result = self._evaluate( + RuntimeError("gh: Server Error (HTTP 502)"), + BEHIND, + self_logins=frozenset({"me"}), + author={"login": "me"}, + ) + self.assertFalse(result["ready"]) + self.assertEqual(len(self._freshness_blockers(result)), 1, result["blockers"]) + self.assertTrue(result["baseFreshness"]["behind"]) + + def test_a_strict_rule_that_lists_no_check_compares(self) -> None: + strict_without_checks = { + "type": "required_status_checks", + "parameters": { + "required_status_checks": [], + "strict_required_status_checks_policy": True, + }, + } + result = self._evaluate( + [strict_without_checks], + BEHIND, + self_logins=frozenset({"me"}), + author={"login": "me"}, + ) + self.assertFalse(result["ready"]) + self.assertEqual(len(self._freshness_blockers(result)), 1, result["blockers"]) + self.assertEqual(len(self.compare_calls), 1) + def test_auto_merge_is_not_armed_over_a_behind_head(self) -> None: # Only a running required check holds the merge, which `--auto` may arm # over; on a loose base GitHub would then merge the behind head itself. @@ -196,6 +232,24 @@ def test_loose_rulesets_do_not(self) -> None: summary = merge.branch_rules("owner/repo", "main") self.assertFalse(summary["requireUpToDate"]) + def test_a_strict_rule_needs_a_required_check_of_its_own(self) -> None: + # GitHub: the strict setting "will not take effect unless at least one + # status check is enabled" (https://docs.github.com/en/rest/repos/rules). + # Another rule's check does not count for it. + strict_without_checks = { + "type": "required_status_checks", + "parameters": { + "required_status_checks": [{"integration_id": 1}], + "strict_required_status_checks_policy": True, + }, + } + with mock.patch.object( + merge, "gh_json", return_value=[*LOOSE, strict_without_checks] + ): + summary = merge.branch_rules("owner/repo", "main") + self.assertFalse(summary["requireUpToDate"]) + self.assertEqual(summary["requiredContexts"], ["ci-status"]) + if __name__ == "__main__": unittest.main() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_review_trigger_race.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_review_trigger_race.py index 766dadd18f..b9404ba68e 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_review_trigger_race.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_review_trigger_race.py @@ -395,6 +395,18 @@ def test_two_spaced_observations_become_eligible(self) -> None: self.assertEqual(result["state"], "eligible") self.assertTrue(result["request_eligible"]) + def test_a_head_the_snapshot_reports_behind_is_no_candidate(self) -> None: + # A loose base reports a behind head CLEAN; `request_review.py` rejects + # it live, so candidacy must not spend the window on it first (#5955). + prior = {"review_trigger": {"missing_head_sha": HEAD, + "missing_first_seen_at": "2026-07-10T00:00:00Z", + "missing_observations": 1}} + result = review_trigger.classify_review_request( + self._pr(), self._gate(), prior, "2026-07-10T00:05:00Z", + review_trigger_allowed=True, branch_behind=True, config=configured()) + self.assertFalse(result["request_eligible"]) + self.assertEqual(result["missing_head_sha"], "") + def test_dormant_gate_never_produces_a_candidate(self) -> None: gate = review_trigger.review_gate_state( checks.classify_checks([]), From b51ed9b73e5b544b1fe715c8dd9869d5e52f0f9b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 4 Oct 2026 04:07:15 +0000 Subject: [PATCH 3/3] fix(source-control): state only the freshness policy and link the upstream sections Co-authored-by: ksextonmelodic --- plugins/source-control/CHANGELOG.md | 2 +- .../skills/babysit-prs/reference/freshness.md | 54 ++++++++++--------- .../skills/babysit-prs/reference/safety.md | 9 ++-- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index d4f8f4da31..376b11f4f8 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -8,7 +8,7 @@ All notable changes to the `source-control` plugin are documented here. Format f ### Fixed - **The babysit merge gate holds a `CLEAN` head that is behind its base ([#5955](https://github.com/melodic-software/claude-code-plugins/issues/5955)).** - Under loose required status checks GitHub reports a behind head `CLEAN`. The gate made no base compare of its own and the snapshot compared only a `BLOCKED` head, so the gate could squash-merge a behind head and drop base commits. When the gate runs on a PR that is otherwise ready (or held only by running checks), it now compares the head against the live base and holds it if it is behind or the compare cannot be read, and reports the result as `baseFreshness`. The check runs at gate time, including when the gate arms `--auto`; an auto-merge already armed is not re-checked if the base moves afterwards, a race that predates this change. The gate makes no compare on a base whose rulesets require up-to-date branches (a strict `required_status_checks` rule that lists at least one check, since GitHub's strict setting takes no effect without one) or a merge queue; classic branch protection is not read. The queue snapshot now compares every `BLOCKED`, `CLEAN`, or `HAS_HOOKS` PR on every cycle and reports a behind `CLEAN` or `HAS_HOOKS` head as `branch_freshness.state == "behind"` so the guarded refresh can clear the hold. For that head it also reads the base's rules, and it reports the head behind only when the read succeeds and shows no merge queue: on a failed read the head stays not behind for that cycle, so a transient failure cannot start a refresh (which disarms auto-merge and reruns CI and the AI reviews) on a queue base. A compare that keeps failing on a loose base holds the PR until a human acts. The review-request candidate skips a head the snapshot reports `behind`, matching the live re-check in `request_review.py`. + Under loose required status checks GitHub reports a behind head `CLEAN`. The gate made no base compare of its own and the snapshot compared only a `BLOCKED` head, so the gate could squash-merge a behind head and drop base commits. When the gate runs on a PR that is otherwise ready (or held only by running checks), it now compares the head against the live base and holds it if it is behind or the compare cannot be read, and reports the result as `baseFreshness`. The check runs at gate time, including when the gate arms `--auto`; an auto-merge already armed is not re-checked if the base moves afterwards, a race that predates this change. The gate makes no compare on a base whose rulesets require up-to-date branches (a strict `required_status_checks` rule that lists at least one check) or a merge queue; classic branch protection is not read. The queue snapshot now compares every `BLOCKED`, `CLEAN`, or `HAS_HOOKS` PR on every cycle and reports a behind `CLEAN` or `HAS_HOOKS` head as `branch_freshness.state == "behind"` so the guarded refresh can clear the hold. For that head it also reads the base's rules, and it reports the head behind only when the read succeeds and shows no merge queue: on a failed read the head stays not behind for that cycle, so a transient failure cannot start a refresh (which disarms auto-merge and reruns CI and the AI reviews) on a queue base. A compare that keeps failing on a loose base holds the PR until a human acts. The review-request candidate skips a head the snapshot reports `behind`, matching the live re-check in `request_review.py`. ## [0.79.7] - 2026-10-04 diff --git a/plugins/source-control/skills/babysit-prs/reference/freshness.md b/plugins/source-control/skills/babysit-prs/reference/freshness.md index cdf7da549d..dab468ccf7 100644 --- a/plugins/source-control/skills/babysit-prs/reference/freshness.md +++ b/plugins/source-control/skills/babysit-prs/reference/freshness.md @@ -25,9 +25,10 @@ missing from the branch), yet `mergeStateStatus` reported `BLOCKED`, never `BEHI only ever matched the literal string `BEHIND` could never open for that PR, a chicken-and-egg an automated queue cannot break out of on its own. -`BEHIND` is also reported only where the base requires branches to be up to date. Under loose -required status checks GitHub merges a behind head, so a behind head with every other gate met -reads `CLEAN` (or `HAS_HOOKS`). +A behind head can also read `CLEAN` or `HAS_HOOKS`, so the snapshot does not trust those states +as proof of freshness on a base that does not enforce it; which base settings produce that is in +the +[loose-base and merge-queue record](#upstream-drift-record-for-the-loose-base-and-merge-queue-decisions). The snapshot engine closes both gaps with one narrow, evidence-based fallback: when `mergeStateStatus` is `BLOCKED`, `CLEAN`, or `HAS_HOOKS`, it compares the base ref against the @@ -36,11 +37,10 @@ outstanding base commits (`status` in `behind`/`diverged` and `behind_by > 0`), classified `branch_freshness.state == "behind"` (`source: "compare_api"`) exactly as if `mergeStateStatus` had reported `BEHIND` directly. A behind `CLEAN`/`HAS_HOOKS` head is the one exception: its base's rules are read too, and it is `behind` only when that read succeeds and -shows no merge queue. On a queue base it stays `not_reported_behind`, because the queue tests the -PR against the latest base itself and needs no branch update. On an unreadable rules answer it -also stays `not_reported_behind` for that cycle: the refresh disarms auto-merge and its push -reruns CI and the AI reviews, which a transient failure must not start on a queue base. The next -snapshot reads the rules again. Any other cause of `BLOCKED`, a real merge conflict, a pending +shows no merge queue. A queue base is exempt and stays `not_reported_behind`: the refresh is not +ours to run there. An unreadable rules answer also stays `not_reported_behind` for that cycle: the +refresh disarms auto-merge and its push reruns CI and the AI reviews, which a transient failure +must not start on a queue base. The next snapshot reads the rules again. Any other cause of `BLOCKED`, a real merge conflict, a pending human review, anything else, is untouched: the fallback only ever flips these states to `behind`, never invents eligibility the compare API did not prove, and every other invariant below (conflict check, human-review stop, worker lease, unique head ref, the per-source-SHA refresh @@ -76,21 +76,25 @@ observations, reproducible with the single-PR diagnostic below: read `mergeState refresh guarantee for `baseRefOid`, a GitHub changelog entry naming either, or a diagnostic run where `BLOCKED` no longer co-occurs with a positive `behind_by`. -### Verification record for the loose-base and merge-queue claims - -**Claims.** A base with loose required status checks lets a behind head merge, and a base that -requires a merge queue gives the same up-to-date guarantee without the branch being updated. - -**Basis.** The strict and loose rows of -[require status checks before merging](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging), -and [about merge queues](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#about-merge-queues). -The rulesets field read for the strict setting is `strict_required_status_checks_policy` on a -`required_status_checks` rule, as `GET /repos/{owner}/{repo}/rules/branches/{branch}` returns it. - -**Verified.** 2026-10-04, against both pages and a live rules read as of that day. - -**Recheck trigger.** Either page changing what the loose setting or a merge queue guarantees, or -the rules endpoint renaming the strict field. +### Upstream-drift record for the loose-base and merge-queue decisions + +The gate and the snapshot treat a base as enforcing freshness only when its rulesets carry a +strict required-checks rule that lists at least one check, or a merge queue; on any other base +they compare the head against the live base. Only rulesets are read, not classic branch +protection. + +- **Pointer**: when deciding which base settings make GitHub report `BEHIND`, merge a behind head, + or guarantee an up-to-date merge, fetch the + [require status checks before merging](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging) + section and + [about merge queues](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#about-merge-queues) + live. When reading a base's strict setting from rulesets, fetch the + [rules for a branch](https://docs.github.com/en/rest/repos/rules#get-rules-for-a-branch) + endpoint live. +- **As of**: 2026-10-04 +- **Recheck trigger**: either docs section changing what the loose or strict setting or a merge + queue guarantees, or the rules-for-a-branch endpoint renaming or reshaping the field the code + reads for the strict setting. ## Orchestrator-Only Refresh Procedure @@ -149,8 +153,8 @@ base after the PR branched, including the tests that covered them, with CI green Treat `branch_freshness.state == "behind"` as a hard stop on the merge path even when GitHub reports `mergeStateStatus` `CLEAN`/`HAS_HOOKS`: under a non-strict ruleset, GitHub does not itself refuse a behind-base merge, so CLEAN does **not** imply an up-to-date base. The merge gate enforces -this on its own: on a base whose rulesets require neither strict required checks (a strict rule -that lists at least one required check) nor a merge queue, it compares an otherwise-ready head +this on its own: on a base whose rulesets carry neither a strict required-checks rule that lists +at least one check nor a merge queue, it compares an otherwise-ready head against the live base when it runs, and holds the head if it is behind or the compare cannot be read (`baseFreshness` in its output). Only rulesets are read: classic branch protection is not, so a base that is strict or queue-gated only through classic protection still gets the compare. diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index 80c79456d3..d50ba8d314 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -477,20 +477,19 @@ auto-mode safety classifier and blocks the call before the wrapper runs. So the gate keeps trusting `CLEAN` for mergeability; what can be up to 12 hours behind the base is the merge commit `pull_request` CI ran against. Only a strict up-to-date rule (`BEHIND`) proves the head is current at merge. Under a base whose rulesets carry neither that rule nor a merge - queue, a behind head still reads `CLEAN`, so when the gate runs on an otherwise-ready PR it + queue, `CLEAN` is not proof of freshness, so when the gate runs on an otherwise-ready PR it compares the head against the live base and holds it if it is behind or the compare cannot be read (`baseFreshness` in the output). The compare runs at gate time, including when the gate arms `--auto`; an auto-merge already armed is not re-checked if the base moves afterwards, a race that predates this check. The snapshot reports the same head `branch_freshness.state == "behind"`, and [freshness.md](freshness.md)'s refresh clears the hold; a compare that keeps failing holds the PR until a human acts. A merge-queue base makes no - compare: the queue tests the PR against the latest base itself. Only rulesets are read, not - classic branch protection. **Claim, basis, as of, recheck:** that + compare. Only rulesets are read, not classic branch protection. **Claim, basis, as of, recheck:** that regeneration rule, [changes to test merge commit generation](https://github.blog/changelog/2026-02-19-changes-to-test-merge-commit-generation-for-pull-requests), 2026-10-02, and a GitHub changelog entry that changes test-merge regeneration or says it now - affects mergeability. The loose-base and merge-queue claims carry their own record in - [freshness.md](freshness.md#verification-record-for-the-loose-base-and-merge-queue-claims). + affects mergeability. The loose-base and merge-queue decisions carry their own record in + [freshness.md](freshness.md#upstream-drift-record-for-the-loose-base-and-merge-queue-decisions). - **`--self-logins @me,` rides on every merge form too**, read-only and mutating alike. `@me` resolves to your own `gh` login and the `babysit_self_logins` extras follow it; drop the trailing `,` when that value is empty. On the merge gate this flag is what