diff --git a/CHANGELOG.md b/CHANGELOG.md index 98d62769e1..29a1a88eec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -162,6 +162,11 @@ ## Proposed +- Re-fetch authoritative open/Draft state and exact head immediately before + both direct-merge and auto-merge mutations. A caller's stale Ready snapshot, + a closed or unavailable PR, or a moved head now fails closed before any + guarded GitHub merge command. + - Keep target-repository old-head Actions inventory and destructive-boundary revalidation when review execution is centralized, while excluding only bare or rendered OpenCode workflow names whose lifecycle belongs to the diff --git a/docs/doctoring/draft-merge-mutation-boundary.md b/docs/doctoring/draft-merge-mutation-boundary.md new file mode 100644 index 0000000000..ac5cbe3b69 --- /dev/null +++ b/docs/doctoring/draft-merge-mutation-boundary.md @@ -0,0 +1,50 @@ +# Draft merge mutation boundary + +Decision date: **2026-09-07** + +## Problem + +The scheduler normally excludes Draft pull requests during inspection. A PR can +change lifecycle state after that decision, or another caller can invoke the +mutation helper directly. Without a second guard, direct merge or auto-merge +could proceed from stale Ready-state authority. + +## Decision + +`enable_auto_merge` and `merge_pr` retain their cheap caller-snapshot Draft +guard. After dry-run handling and mutation-actor validation, both now call one +shared boundary that reuses the existing direct REST authority read. That read +must prove the same repository and PR number remain open, expose an explicit +Draft value of `false`, and retain the expected exact head. Missing, malformed, +closed, Draft, or moved-head evidence fails closed before `gh pr merge`. + +This boundary is intentionally inside both mutation entrypoints. The earlier +`inspect_pr` approval revalidation remains useful, but cannot protect a direct +caller or a lifecycle transition occurring after that decision-level check. +`--match-head-commit` remains the final GitHub head guard; it does not replace +the live Draft-state check. + +## Failure scenes + +- A Ready snapshot becomes Draft on the same head: mutation is refused. +- The PR closes, becomes unavailable, or returns malformed authority: mutation + is refused. +- The head changes after inspection: mutation is refused before GitHub CLI. +- A direct helper call supplies a Draft PR: no GitHub command is executed. +- A freshly open, non-Draft PR on the expected head follows the existing + guarded merge flow unchanged. + +## Evidence and follow-up + +The original RED `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2` +covered only an already-Draft caller snapshot. Corrective RED +`897c7e6505a4c5dc203471109e425996f91fb9c9` covers +both mutation entrypoints across same-head Ready→Draft, moved-head, missing-PR, +and exact-ready cases. The focused scheduler suite passes locally under +`GITHUB_ACTIONS=true` and `-W error`; fresh exact-head hosted checks and +independent review remain required. + +## Reference + +GitHub. (2026). *Pull requests and draft pull requests*. +https://docs.github.com/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 04750d0cea..0d7db93ec6 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -3421,3 +3421,20 @@ same name in another file can carry the opposite safety property.** `234d98dec14ae7a91819857f561b78d0d424ec98` and `32a0d66cd1210f6fae1cb675265ce4ce49f63167`; focused local contract evidence is not hosted authority, and fresh exact-head hosted checks remain required. + + +### Draft merge mutation boundary + +- **Status:** Proposed +- **Owner:** `ContextualWisdomLab/.github` +- **Problem:** Scheduler decision code filtered Draft PRs, but the direct-merge + and auto-merge mutation functions did not revalidate lifecycle state. +- **Action:** Re-fetch repository/PR/current head and live Draft state at both + irreversible mutation boundaries; reject lifecycle or head changes before + any merge or auto-merge command. +- **Evidence:** The original RED + `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2` covered only an already-Draft + caller. Corrective RED `897c7e6505a4c5dc203471109e425996f91fb9c9` + exercises both mutation entrypoints for a + same-head Ready→Draft race, moved head, missing live PR, and exact-ready + control. Fresh exact-head hosted checks remain required before integration. diff --git a/scripts/ci/pr_review_merge_scheduler_core.py b/scripts/ci/pr_review_merge_scheduler_core.py index 08da1eecd9..105428bdcb 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -2638,23 +2638,50 @@ def run_head_guarded_merge( run(merge_args) +def require_fresh_merge_target(repo: str, pr: dict[str, Any]) -> str: + """Return the exact live Ready head or fail closed before a merge mutation.""" + target_repo = validate_github_repository(repo) + number = int(pr["number"]) + expected_head = validate_git_sha(pr["headRefOid"]).lower() + try: + fresh_pr = _fresh_open_pr_for_cancellation(target_repo, number) + except (KeyError, RuntimeError, TypeError, ValueError) as exc: + raise RuntimeError( + f"merge mutation refused because PR #{number} is no longer open " + f"with authoritative lifecycle and head evidence ({exc})" + ) from exc + if fresh_pr["draft"] is not False: + raise RuntimeError(f"merge mutation refused because PR #{number} became draft") + fresh_head = validate_git_sha(str((fresh_pr["head"] or {}).get("sha") or "")).lower() + if fresh_head != expected_head: + raise RuntimeError( + f"merge mutation refused because PR #{number} head changed from " + f"{short_sha(expected_head)} to {short_sha(fresh_head)}" + ) + return fresh_head + + def enable_auto_merge(repo: str, pr: dict[str, Any], *, dry_run: bool) -> None: """Enable auto-merge for a PR at its current head using an allowed method.""" + if pr.get("isDraft"): + raise RuntimeError("enable-auto-merge refused for draft PR") number = str(pr["number"]) if dry_run: return require_github_actions_mutation_actor("enable-auto-merge") - head = validate_git_sha(pr["headRefOid"]) + head = require_fresh_merge_target(repo, pr) run_head_guarded_merge(repo, number, head, auto=True) def merge_pr(repo: str, pr: dict[str, Any], *, dry_run: bool) -> None: """Merge a current-head-approved PR immediately with a head guard.""" + if pr.get("isDraft"): + raise RuntimeError("direct-merge refused for draft PR") number = str(pr["number"]) if dry_run: return require_github_actions_mutation_actor("direct-merge") - head = validate_git_sha(pr["headRefOid"]) + head = require_fresh_merge_target(repo, pr) run_head_guarded_merge(repo, number, head, auto=False) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 84666a687e..4568a020f1 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -4541,6 +4541,15 @@ def fake_run(args, stdin=None): monkeypatch.setenv("GITHUB_ACTIONS", "true") monkeypatch.setenv("GH_TOKEN", "workflow-token") + monkeypatch.setattr( + sched, + "_fresh_open_pr_for_cancellation", + lambda _repo, _number: { + "state": "open", + "draft": False, + "head": {"sha": head_sha}, + }, + ) sched.enable_auto_merge("owner/repo", pr, dry_run=False) sched.merge_pr("owner/repo", pr, dry_run=False) sched.disable_auto_merge("owner/repo", pr, dry_run=False) @@ -10874,3 +10883,117 @@ def test_withheld_mutation_guidance_uses_recorded_reason_after_environment_chang assert "workflow GITHUB_TOKEN" in "\n".join( sched.head_mutation_credential_upgrade_summary([decision]) ) + + +def test_draft_pr_cannot_reach_merge_mutations(monkeypatch): + """Defense in depth rejects drafts at both guarded merge boundaries.""" + calls = [] + monkeypatch.setattr(sched, "run", lambda args: calls.append(args) or "") + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.setenv("GH_TOKEN", "workflow-token") + draft_pr = make_pr(isDraft=True, headRefOid="a" * 40) + + for mutation in (sched.enable_auto_merge, sched.merge_pr): + with pytest.raises(RuntimeError, match="draft PR"): + mutation("owner/repo", draft_pr, dry_run=False) + + assert calls == [] + + +@pytest.mark.parametrize("mutation", (sched.enable_auto_merge, sched.merge_pr)) +def test_merge_mutation_rechecks_live_draft_state(monkeypatch, mutation): + """A Ready snapshot cannot mutate after the live PR becomes Draft.""" + head_sha = "a" * 40 + snapshot = make_pr(number=7, isDraft=False, headRefOid=head_sha) + fresh_draft = {"state": "open", "draft": True, "head": {"sha": head_sha}} + mutation_calls = [] + monkeypatch.setattr(sched, "require_github_actions_mutation_actor", lambda _action: None) + monkeypatch.setattr( + sched, "_fresh_open_pr_for_cancellation", lambda _repo, _number: fresh_draft + ) + monkeypatch.setattr( + sched, + "run_head_guarded_merge", + lambda *args, **kwargs: mutation_calls.append((args, kwargs)), + ) + + with pytest.raises(RuntimeError, match="became draft"): + mutation("owner/repo", snapshot, dry_run=False) + + assert mutation_calls == [] + + +@pytest.mark.parametrize("mutation", (sched.enable_auto_merge, sched.merge_pr)) +def test_merge_mutation_rechecks_live_head(monkeypatch, mutation): + """A head change after inspection cannot reach a merge mutation.""" + snapshot = make_pr(number=7, isDraft=False, headRefOid="a" * 40) + moved = {"state": "open", "draft": False, "head": {"sha": "b" * 40}} + mutation_calls = [] + monkeypatch.setattr(sched, "require_github_actions_mutation_actor", lambda _action: None) + monkeypatch.setattr( + sched, "_fresh_open_pr_for_cancellation", lambda _repo, _number: moved + ) + monkeypatch.setattr( + sched, + "run_head_guarded_merge", + lambda *args, **kwargs: mutation_calls.append((args, kwargs)), + ) + + with pytest.raises(RuntimeError, match="head changed"): + mutation("owner/repo", snapshot, dry_run=False) + + assert mutation_calls == [] + + +@pytest.mark.parametrize("mutation", (sched.enable_auto_merge, sched.merge_pr)) +def test_merge_mutation_fails_closed_when_live_pr_disappears(monkeypatch, mutation): + """A missing live PR cannot authorize a merge mutation.""" + snapshot = make_pr(number=7, isDraft=False, headRefOid="a" * 40) + mutation_calls = [] + monkeypatch.setattr(sched, "require_github_actions_mutation_actor", lambda _action: None) + + def missing_live_pr(_repo, _number): + raise ValueError("PR #7 in owner/repo is not a resolvable open pull request") + + monkeypatch.setattr(sched, "_fresh_open_pr_for_cancellation", missing_live_pr) + monkeypatch.setattr( + sched, + "run_head_guarded_merge", + lambda *args, **kwargs: mutation_calls.append((args, kwargs)), + ) + + with pytest.raises(RuntimeError, match="no longer open"): + mutation("owner/repo", snapshot, dry_run=False) + + assert mutation_calls == [] + + +@pytest.mark.parametrize( + ("mutation", "expected_auto"), + ((sched.enable_auto_merge, True), (sched.merge_pr, False)), +) +def test_merge_mutation_uses_fresh_exact_ready_pr( + monkeypatch, mutation, expected_auto +): + """An open, Ready, exact-head re-fetch preserves the guarded merge path.""" + head_sha = "a" * 40 + snapshot = make_pr(number=7, isDraft=False, headRefOid=head_sha) + fresh_ready = {"state": "open", "draft": False, "head": {"sha": head_sha}} + fetch_calls = [] + mutation_calls = [] + monkeypatch.setattr(sched, "require_github_actions_mutation_actor", lambda _action: None) + monkeypatch.setattr( + sched, + "_fresh_open_pr_for_cancellation", + lambda repo, number: fetch_calls.append((repo, number)) or fresh_ready, + ) + monkeypatch.setattr( + sched, + "run_head_guarded_merge", + lambda *args, **kwargs: mutation_calls.append((args, kwargs)), + ) + + mutation("owner/repo", snapshot, dry_run=False) + + assert fetch_calls == [("owner/repo", 7)] + assert mutation_calls == [(('owner/repo', '7', head_sha), {'auto': expected_auto})]