From 2180e27988bf025d80c50c0a6e951bfd8e6a4ee4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 25 Jun 2026 15:45:47 +0900 Subject: [PATCH] test: add scheduler decision contract fixtures --- PR_GOVERNANCE_AUDIT.md | 9 +- scripts/ci/pr_review_merge_scheduler.py | 145 ++++++++++++++++++++++-- tests/test_pr_review_merge_scheduler.py | 72 ++++++++++-- 3 files changed, 206 insertions(+), 20 deletions(-) diff --git a/PR_GOVERNANCE_AUDIT.md b/PR_GOVERNANCE_AUDIT.md index bd89e09b6..e2ada1a5b 100644 --- a/PR_GOVERNANCE_AUDIT.md +++ b/PR_GOVERNANCE_AUDIT.md @@ -130,6 +130,7 @@ The checked-in scheduler already does the minimal central path: - dispatches OpenCode only after same-head Strix evidence is complete, including failed Strix evidence that OpenCode must explain from logs. - records mutation failures as `action_error` for the affected PR and continues scanning later PRs, so a permission failure on one merge/update action does not hide the rest of the queue. - writes the same per-PR decisions to the GitHub Actions step summary, so conflict repair and update-branch decisions are visible without opening raw logs. +- prints a machine-readable `pr-review-merge-scheduler/v1` JSON contract with every inspected PR, the scheduler action, the bounded decision value (`UPDATE_BRANCH`, `WAIT`, `REQUEST_CHANGES`, or `NO_ACTION`), and structured `guidance` for states that need action: `merge_conflict_repair` includes the base/head branches, repair steps, and merge-or-rebase commands; `github_actions_update_branch` names `github-actions[bot]`, the workflow `GITHUB_TOKEN`, `pull-requests: write`, `expected_head_sha`, and the new-head evidence required before merge. - caps each GraphQL PR page at 25 nodes, so large queues can be scanned without hitting GitHub's query resource limit. Small proof run: @@ -142,18 +143,18 @@ $ python3 scripts/ci/pr_review_merge_scheduler.py --repo ContextualWisdomLab/sco PR #119: block: merge conflict: DIRTY; base=develop, head=bolt/perf-format-number-1301647661105713430; run `gh pr checkout 119`, `git fetch origin develop`, then `git merge --no-ff origin/develop` or `git rebase origin/develop`; use `git status --short` to find conflicted files, resolve conflict markers in the PR branch, rerun focused checks, and push the same bolt/perf-format-number-1301647661105713430 branch (use `git push --force-with-lease` only if rebased) ... PR #127: wait: current head is approved; auto-merge disabled by scheduler inputs -{"base_branch": "develop", "counts": {"block": 6, "wait": 1}, "dry_run": true, "inspected": 7, "project_flow": "git-flow"} +{"base_branch": "develop", "counts": {"block": 6, "wait": 1}, "decisions": [...], "dry_run": true, "inspected": 7, "project_flow": "git-flow", "schema_version": "pr-review-merge-scheduler/v1"} $ python3 scripts/ci/pr_review_merge_scheduler.py --repo ContextualWisdomLab/.github --base-branch main --project-flow github-flow --dry-run --max-prs 40 --no-trigger-reviews --no-enable-auto-merge PR #44: wait: current head is approved; auto-merge already enabled ... -{"base_branch": "main", "counts": {"block": 24, "wait": 1}, "dry_run": true, "inspected": 25, "project_flow": "github-flow"} +{"base_branch": "main", "counts": {"block": 24, "wait": 1}, "decisions": [...], "dry_run": true, "inspected": 25, "project_flow": "github-flow", "schema_version": "pr-review-merge-scheduler/v1"} $ python3 scripts/ci/pr_review_merge_scheduler.py --repo ContextualWisdomLab/bandscope --base-branch develop --project-flow git-flow --dry-run --max-prs 40 --no-trigger-reviews --no-enable-auto-merge PR #378: wait: OpenCode review is already in progress PR #381: wait: OpenCode review is already in progress ... -{"base_branch": "develop", "counts": {"block": 38, "wait": 2}, "dry_run": true, "inspected": 40, "project_flow": "git-flow"} +{"base_branch": "develop", "counts": {"block": 38, "wait": 2}, "decisions": [...], "dry_run": true, "inspected": 40, "project_flow": "git-flow", "schema_version": "pr-review-merge-scheduler/v1"} ``` ## Rollout List @@ -178,7 +179,7 @@ PR #381: wait: OpenCode review is already in progress - PR #721 in `naruon` remains the historical fixture for this proof: head `b683deaf8b4761399321799279f58d884db57141`, current-head OpenCode approval `4558310923`, unresolved review threads `0`, and `mergeStateStatus=BEHIND`. Central `.github` dry-run selected `update_branch`, but `naruon` workflow run `28073586594` used the then-stale repo-local scheduler and did not update it. PR #756 has since rolled the central scheduler into `naruon`, so the next proof must use a fresh current-head outdated PR instead of reusing stale evidence from #721. - `naruon` workflow run `28073490721` failed at `gh pr merge 694 --auto --merge --match-head-commit 76416321742af4c8dcd0f96927f64b7548d66fd8` with `GraphQL: Resource not accessible by integration (enablePullRequestAutoMerge)`. This is a DX/governance action failure, not a source-code finding, and the scheduler now records it per PR instead of aborting the scan. - `naruon` PR #756 completed the repo-local rollout for the scheduler contract. Its initial head failed backend governance and Scorecard because `actions: write`/`contents: write` were broader than the repo policy allows; the amended and merged head restores minimal `GITHUB_TOKEN` permissions, keeps `trigger_reviews` and `enable_auto_merge` defaulted off, keeps `update_branches` defaulted on, and still dry-runs PR #694/#721 as `update_branch`. -- `update-branch` `422/403` behavior still needs a safe fixture or a real blocked case before claiming standardized handling. +- `update-branch` `422/403` now has a safe fixture: unit tests simulate both permission-denied and stale `expected_head_sha` failures, assert they become `action_error`, and assert later PRs are still inspected. A real live `422/403` case is still useful as operational evidence, but it is no longer missing from the decision contract test surface. - Public repo drift is real, not hypothetical: only `.github` matched the central scheduler/workflow byte-for-byte in the 2026-06-25 scan. Some drift is policy-specific and should not be overwritten blindly, but `bandscope` had behaviorally unsafe drift and now has PR #450. - Required-check interpretation should stay delegated to GitHub native auto-merge until a repo needs immediate merge. - PR #28 proves the self-modifying trusted workflow bootstrap path after newer same-head evidence exists, but it does not prove update-branch behavior, stale approval dismissal after a head change, or cross-repository rollout. diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index df56b3dfb..c1e907181 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -83,6 +83,107 @@ class Decision: reason: str +def contract_decision(decision: Decision) -> str: + """Map scheduler actions into the bounded PR decision contract.""" + if decision.action == "update_branch": + return "UPDATE_BRANCH" + if decision.action in {"wait", "security_dispatch", "review_dispatch", "action_error"}: + return "WAIT" + if decision.action in {"skip", "auto_merge"}: + return "NO_ACTION" + if decision.action == "block" and "current-head OpenCode review requested changes" in decision.reason: + return "REQUEST_CHANGES" + return "WAIT" + + +def decision_payload( + decisions: list[Decision], + *, + counts: dict[str, int], + dry_run: bool, + base_branch: str, + project_flow: str, +) -> dict[str, Any]: + """Return the machine-readable scheduler decision contract.""" + return { + "schema_version": "pr-review-merge-scheduler/v1", + "base_branch": base_branch, + "dry_run": dry_run, + "inspected": len(decisions), + "counts": counts, + "project_flow": project_flow, + "decisions": [decision_contract_entry(decision) for decision in decisions], + } + + +def decision_contract_entry(decision: Decision) -> dict[str, Any]: + """Return one machine-readable decision contract entry.""" + entry: dict[str, Any] = { + "pr": decision.pr, + "action": decision.action, + "contract_decision": contract_decision(decision), + "reason": decision.reason, + } + guidance = decision_guidance(decision) + if guidance: + entry["guidance"] = guidance + return entry + + +def decision_guidance(decision: Decision) -> dict[str, Any] | None: + """Return actionable repair or automation guidance for known scheduler states.""" + parsed_conflict = parse_conflict_reason(decision.reason) + if parsed_conflict: + state, base_ref, head_ref = parsed_conflict + base_remote = f"origin/{base_ref}" + quoted_base_ref = shlex.quote(base_ref) + quoted_base_remote = shlex.quote(base_remote) + return { + "type": "merge_conflict_repair", + "merge_state": state, + "base_ref": base_ref, + "head_ref": head_ref, + "summary": "Repair the PR branch against the latest base branch, then push the same branch so review and required checks rerun on the new head.", + "steps": [ + "Check out the PR branch.", + "Fetch the latest base branch.", + "Choose merge or rebase; do not treat the conflict as an OpenCode finding.", + "Resolve conflict markers in the PR branch and stage the resolved files.", + "Run the focused checks for the changed area.", + "Push the PR branch; use --force-with-lease only if the branch was rebased.", + ], + "commands": [ + f"gh pr checkout {decision.pr}", + f"git fetch origin {quoted_base_ref}", + f"git merge --no-ff {quoted_base_remote}", + f"# or: git rebase {quoted_base_remote}", + "git status --short", + "git add ", + "# merge path: git commit", + "# rebase path: git rebase --continue", + "git push", + "# rebase path only: git push --force-with-lease", + ], + } + if decision.action == "update_branch": + return { + "type": "github_actions_update_branch", + "actor": "github-actions[bot]", + "token": "workflow GITHUB_TOKEN", + "required_permission": "pull-requests: write", + "head_guard": "expected_head_sha", + "summary": "GitHub Actions requests the PR branch update mechanically; the updated head must be reviewed again before merge.", + "next_required_evidence": [ + "new head SHA after the update_branch mutation", + "OpenCode approval on that exact new head", + "same-head Strix evidence", + "required GitHub Checks success", + "zero active unresolved review threads", + ], + } + return None + + def run(args: Sequence[str], *, stdin: str | None = None) -> str: """Run a command and return stdout, raising with stderr on failure.""" if isinstance(args, str) or not all(isinstance(arg, str) for arg in args): @@ -461,13 +562,13 @@ def print_summary( ) print( json.dumps( - { - "base_branch": base_branch, - "dry_run": dry_run, - "inspected": len(decisions), - "counts": counts, - "project_flow": project_flow, - }, + decision_payload( + decisions, + counts=counts, + dry_run=dry_run, + base_branch=base_branch, + project_flow=project_flow, + ), sort_keys=True, ) ) @@ -817,6 +918,36 @@ def self_test() -> None: assert "git rebase origin/main" in decision.reason assert "git status --short" in decision.reason assert "resolve conflict markers" in decision.reason + conflict_guidance = decision_guidance(decision) + assert conflict_guidance + assert conflict_guidance["type"] == "merge_conflict_repair" + assert conflict_guidance["merge_state"] == "DIRTY" + assert "git status --short" in conflict_guidance["commands"] + assert contract_decision(Decision(1, "update_branch", "ok")) == "UPDATE_BRANCH" + assert contract_decision(Decision(1, "wait", "ok")) == "WAIT" + assert contract_decision(Decision(1, "action_error", "ok")) == "WAIT" + assert contract_decision(Decision(1, "auto_merge", "ok")) == "NO_ACTION" + assert contract_decision(Decision(1, "skip", "ok")) == "NO_ACTION" + assert ( + contract_decision(Decision(1, "block", "current-head OpenCode review requested changes")) + == "REQUEST_CHANGES" + ) + assert contract_decision(Decision(1, "block", "merge conflict: DIRTY")) == "WAIT" + update_guidance = decision_guidance(Decision(1, "update_branch", "ok")) + assert update_guidance + assert update_guidance["actor"] == "github-actions[bot]" + assert update_guidance["head_guard"] == "expected_head_sha" + assert decision_guidance(Decision(1, "wait", "ok")) is None + payload = decision_payload( + [Decision(1, "update_branch", "ok")], + counts={"update_branch": 1}, + dry_run=True, + base_branch="main", + project_flow="github-flow", + ) + assert payload["schema_version"] == "pr-review-merge-scheduler/v1" + assert payload["decisions"][0]["contract_decision"] == "UPDATE_BRANCH" + assert payload["decisions"][0]["guidance"]["actor"] == "github-actions[bot]" print("self-test passed") diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index aa64d4bf7..203012842 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -291,13 +291,26 @@ def test_print_summary_writes_github_step_summary(monkeypatch, tmp_path, capsys) output = capsys.readouterr().out assert "PR #7: block: merge conflict: DIRTY" in output - assert json.loads(output.splitlines()[-1]) == { - "base_branch": "main", - "counts": {"block": 1, "update_branch": 1}, - "dry_run": True, - "inspected": 2, - "project_flow": "github-flow", - } + payload = json.loads(output.splitlines()[-1]) + assert payload["schema_version"] == "pr-review-merge-scheduler/v1" + assert payload["base_branch"] == "main" + assert payload["counts"] == {"block": 1, "update_branch": 1} + assert payload["dry_run"] is True + assert payload["inspected"] == 2 + assert payload["project_flow"] == "github-flow" + assert payload["decisions"][0]["contract_decision"] == "WAIT" + assert payload["decisions"][1]["contract_decision"] == "UPDATE_BRANCH" + assert payload["decisions"][0]["guidance"]["type"] == "merge_conflict_repair" + assert payload["decisions"][0]["guidance"]["merge_state"] == "DIRTY" + assert payload["decisions"][0]["guidance"]["base_ref"] == "main" + assert payload["decisions"][0]["guidance"]["head_ref"] == "feature|x" + assert "gh pr checkout 7" in payload["decisions"][0]["guidance"]["commands"] + assert "git merge --no-ff origin/main" in payload["decisions"][0]["guidance"]["commands"] + assert payload["decisions"][1]["guidance"]["type"] == "github_actions_update_branch" + assert payload["decisions"][1]["guidance"]["actor"] == "github-actions[bot]" + assert payload["decisions"][1]["guidance"]["token"] == "workflow GITHUB_TOKEN" + assert payload["decisions"][1]["guidance"]["required_permission"] == "pull-requests: write" + assert payload["decisions"][1]["guidance"]["head_guard"] == "expected_head_sha" summary = summary_path.read_text(encoding="utf-8") assert "## PR review merge scheduler" in summary assert "| #7 | block | merge conflict: DIRTY; base=main, head=feature\\|x |" in summary @@ -450,7 +463,10 @@ def test_print_summary_self_test_parse_args_and_main(monkeypatch, capsys): ) output = capsys.readouterr().out assert "PR #1: wait: ready" in output - assert json.loads(output.strip().splitlines()[-1])["counts"] == {"wait": 2} + payload = json.loads(output.strip().splitlines()[-1]) + assert payload["schema_version"] == "pr-review-merge-scheduler/v1" + assert payload["counts"] == {"wait": 2} + assert [decision["contract_decision"] for decision in payload["decisions"]] == ["WAIT", "WAIT"] sched.self_test() assert "self-test passed" in capsys.readouterr().out @@ -500,7 +516,45 @@ def fake_inspect(repo, pr, **kwargs): assert "PR #1: action_error: Command failed (1): gh pr merge 1; GraphQL: Resource not accessible by integration" in output assert "scheduler GitHub token could not perform merge or auto-merge" in output assert "PR #2: wait: next PR still inspected" in output - assert json.loads(output.strip().splitlines()[-1])["counts"] == {"action_error": 1, "wait": 1} + payload = json.loads(output.strip().splitlines()[-1]) + assert payload["counts"] == {"action_error": 1, "wait": 1} + assert payload["decisions"][0]["contract_decision"] == "WAIT" + assert payload["decisions"][1]["contract_decision"] == "WAIT" + + +def test_main_keeps_scanning_after_update_branch_403_and_422(monkeypatch, capsys): + prs = [make_pr(number=1), make_pr(number=2), make_pr(number=3)] + seen = [] + + def fake_inspect(repo, pr, **kwargs): + seen.append(pr["number"]) + if pr["number"] == 1: + raise RuntimeError( + "Command failed (1): gh api -X PUT repos/owner/repo/pulls/1/update-branch\n" + "HTTP 403: Resource not accessible by integration" + ) + if pr["number"] == 2: + raise RuntimeError( + "Command failed (1): gh api -X PUT repos/owner/repo/pulls/2/update-branch\n" + "HTTP 422: expected_head_sha does not match current head" + ) + return sched.Decision(pr["number"], "wait", "next PR still inspected") + + monkeypatch.setattr(sched, "fetch_open_prs", lambda repo, max_prs: prs) + monkeypatch.setattr(sched, "inspect_pr", fake_inspect) + + assert sched.main(["--repo", "owner/repo", "--base-branch", "main", "--project-flow", "github"]) == 0 + assert seen == [1, 2, 3] + output = capsys.readouterr().out + assert "PR #1: action_error:" in output + assert "pull-requests: write" in output + assert "do not widen `contents` just for update-branch" in output + assert "PR #2: action_error:" in output + assert "PR head likely changed after inspection" in output + assert "PR #3: wait: next PR still inspected" in output + payload = json.loads(output.strip().splitlines()[-1]) + assert payload["counts"] == {"action_error": 2, "wait": 1} + assert [decision["contract_decision"] for decision in payload["decisions"]] == ["WAIT", "WAIT", "WAIT"] def test_action_error_guidance_distinguishes_update_branch_from_merge():