From 2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 13:54:21 +0900 Subject: [PATCH 1/6] test(scheduler): reproduce draft merge mutation reachability --- tests/test_pr_review_merge_scheduler.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 7e10cf555c..cb3421a1dd 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -10887,3 +10887,18 @@ def test_central_dispatch_skips_non_authoritative_target_actions_inventory( decision = inspect(make_pr(baseRefName="feature-base"), trigger_reviews=False) assert decision.action == "skip" + + +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 == [] From 5abc0a0284bcf2ee11715e642f882af73f1e8adb Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 13:54:53 +0900 Subject: [PATCH 2/6] fix(scheduler): reject draft merge mutations --- CHANGELOG.md | 4 +++ .../draft-merge-mutation-boundary.md | 33 +++++++++++++++++++ docs/product-technical-gap-baseline.md | 13 ++++++++ scripts/ci/pr_review_merge_scheduler_core.py | 4 +++ 4 files changed, 54 insertions(+) create mode 100644 docs/doctoring/draft-merge-mutation-boundary.md diff --git a/CHANGELOG.md b/CHANGELOG.md index c47c4bda6b..515e3ed893 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -162,6 +162,10 @@ ## Proposed +- Reject Draft pull requests again at both direct-merge and auto-merge + mutation functions. This defense-in-depth boundary prevents a stale caller + decision from reaching guarded GitHub mutations after PR lifecycle changes. + - Skip target-repository old-head Actions inventory when review execution is centralized. Same-repository stale-run cleanup remains enabled; central review lifecycle is handled in the configured dispatch repository, avoiding diff --git a/docs/doctoring/draft-merge-mutation-boundary.md b/docs/doctoring/draft-merge-mutation-boundary.md new file mode 100644 index 0000000000..f0a9a327fa --- /dev/null +++ b/docs/doctoring/draft-merge-mutation-boundary.md @@ -0,0 +1,33 @@ +# 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` each reject `isDraft` before actor +validation, head-SHA processing, or any GitHub command. The upstream decision +filter remains in place; this is a minimal defense-in-depth invariant at the +irreversible boundary. + +## Failure scenes + +- A Ready PR becomes Draft after inspection: mutation is refused. +- A direct helper call supplies a Draft PR: no GitHub command is executed. +- A non-Draft PR follows the existing guarded expected-head flow unchanged. + +## Evidence and follow-up + +RED commit: `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2`. +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 ded5f53046..7dbae89a11 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -3416,3 +3416,16 @@ same name in another file can carry the opposite safety property.** - **Evidence:** RED commit `08a16caa4fdb0d0d86c44bb8cd7aed611beaab7b`; fresh exact-head hosted checks remain required before integration. + + +### 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:** Reject Draft PRs at both mutation entrypoints before actor, SHA, + or GitHub mutation processing. +- **Evidence:** RED commit + `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2`; 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 5034ebd29e..cd8550f211 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -2640,6 +2640,8 @@ def run_head_guarded_merge( 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 @@ -2650,6 +2652,8 @@ def enable_auto_merge(repo: str, pr: dict[str, Any], *, dry_run: bool) -> None: 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 From 8e7d76f756a8e1f3816db620edfe9e1a63e75328 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 8 Sep 2026 11:09:43 +0900 Subject: [PATCH 3/6] test(scheduler): inherit proven credential fixtures --- tests/test_pr_review_merge_scheduler.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index cb3421a1dd..8ebf314dbf 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -34,6 +34,8 @@ def workflow_starting_mutation_credential(monkeypatch): workflow-starting credential exactly like the scheduler workflow does. """ monkeypatch.setenv("SCHEDULER_MUTATION_TOKEN_SOURCE", "PR_REVIEW_MERGE_TOKEN") + monkeypatch.setenv("GH_TOKEN", "selected-mutation-token") + monkeypatch.setenv("SCHEDULER_WORKFLOW_TOKEN", "workflow-runner-token") @pytest.fixture(autouse=True) @@ -1785,7 +1787,11 @@ def map(self, func, items): ), ) cancelled = [] - monkeypatch.setattr(sched, "run_github_actions", cancelled.append) + monkeypatch.setattr( + sched, + "run_github_actions", + lambda args, stdin=None: cancelled.append(args), + ) monkeypatch.setattr(sched, "require_github_actions_control_actor", lambda x: None) run_ids = sched.cancel_stale_opencode_runs("owner/repo", "workflow", make_pr(), dry_run=False) @@ -1796,7 +1802,7 @@ def map(self, func, items): def test_force_cancel_failure_logs_reason_and_does_not_raise(monkeypatch, capsys): - def fail_cancel(args): + def fail_cancel(args, stdin=None): raise RuntimeError( "Command failed (1): gh api -X POST " "repos/owner/repo/actions/runs/29263154177/force-cancel; " @@ -1821,7 +1827,7 @@ def fail_cancel(args): def test_force_cancel_multiple_runs_reports_only_failures(monkeypatch): - def maybe_fail(args): + def maybe_fail(args, stdin=None): if "runs/2/force-cancel" in " ".join(args): raise RuntimeError("GitHub returned HTTP 500") return "" From 897c7e6505a4c5dc203471109e425996f91fb9c9 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 8 Sep 2026 15:22:25 +0900 Subject: [PATCH 4/6] test(scheduler): reproduce live draft merge race --- tests/test_pr_review_merge_scheduler.py | 110 ++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 988dcb7602..582c0a5213 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) @@ -10872,6 +10881,8 @@ 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 = [] @@ -10885,3 +10896,102 @@ def test_draft_pr_cannot_reach_merge_mutations(monkeypatch): 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})] From c60a8e03ce302bd000ddd069114c8c91b1b45084 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 8 Sep 2026 15:22:44 +0900 Subject: [PATCH 5/6] fix(scheduler): revalidate live merge lifecycle --- CHANGELOG.md | 7 ++-- .../draft-merge-mutation-boundary.md | 32 ++++++++++++++----- docs/product-technical-gap-baseline.md | 8 +++-- scripts/ci/pr_review_merge_scheduler_core.py | 27 ++++++++++++++-- 4 files changed, 58 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff80811321..438c8c4ea1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -162,9 +162,10 @@ ## Proposed -- Reject Draft pull requests again at both direct-merge and auto-merge - mutation functions. This defense-in-depth boundary prevents a stale caller - decision from reaching guarded GitHub mutations after PR lifecycle changes. +- 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 diff --git a/docs/doctoring/draft-merge-mutation-boundary.md b/docs/doctoring/draft-merge-mutation-boundary.md index f0a9a327fa..4020742fad 100644 --- a/docs/doctoring/draft-merge-mutation-boundary.md +++ b/docs/doctoring/draft-merge-mutation-boundary.md @@ -11,21 +11,37 @@ could proceed from stale Ready-state authority. ## Decision -`enable_auto_merge` and `merge_pr` each reject `isDraft` before actor -validation, head-SHA processing, or any GitHub command. The upstream decision -filter remains in place; this is a minimal defense-in-depth invariant at the -irreversible boundary. +`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 PR becomes Draft after inspection: mutation is refused. +- 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 non-Draft PR follows the existing guarded expected-head flow unchanged. +- A freshly open, non-Draft PR on the expected head follows the existing + guarded merge flow unchanged. ## Evidence and follow-up -RED commit: `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2`. -Fresh exact-head hosted checks and independent review remain required. +The original RED `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2` +covered only an already-Draft caller snapshot. Corrective RED `1afea4e` 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 diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 04ad86c5c7..789cc5c866 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -3429,6 +3429,8 @@ same name in another file can carry the opposite safety property.** - **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:** RED commit - `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2`; fresh lifecycle-race RED and - exact-head hosted checks remain required before integration. +- **Evidence:** The original RED + `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2` covered only an already-Draft + caller. Corrective RED `1afea4e` 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 c3f50acb8c..105428bdcb 100644 --- a/scripts/ci/pr_review_merge_scheduler_core.py +++ b/scripts/ci/pr_review_merge_scheduler_core.py @@ -2638,6 +2638,29 @@ 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"): @@ -2646,7 +2669,7 @@ def enable_auto_merge(repo: str, pr: dict[str, Any], *, dry_run: bool) -> None: 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) @@ -2658,7 +2681,7 @@ def merge_pr(repo: str, pr: dict[str, Any], *, dry_run: bool) -> None: 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) From d739e0d8d6285261da0a2f530181a929f19a202d Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 8 Sep 2026 15:27:45 +0900 Subject: [PATCH 6/6] docs(scheduler): bind lifecycle evidence to remote RED --- docs/doctoring/draft-merge-mutation-boundary.md | 3 ++- docs/product-technical-gap-baseline.md | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/doctoring/draft-merge-mutation-boundary.md b/docs/doctoring/draft-merge-mutation-boundary.md index 4020742fad..ac5cbe3b69 100644 --- a/docs/doctoring/draft-merge-mutation-boundary.md +++ b/docs/doctoring/draft-merge-mutation-boundary.md @@ -37,7 +37,8 @@ the live Draft-state check. ## Evidence and follow-up The original RED `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2` -covered only an already-Draft caller snapshot. Corrective RED `1afea4e` covers +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 diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 789cc5c866..66741170bd 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -3431,6 +3431,7 @@ same name in another file can carry the opposite safety property.** any merge or auto-merge command. - **Evidence:** The original RED `2d140a84203a0df0cb86cd6b6ab31fc37bbdbda2` covered only an already-Draft - caller. Corrective RED `1afea4e` exercises both mutation entrypoints for a + 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.