diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index 7010b262..05d863b0 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -4210,6 +4210,8 @@ jobs: body="$(printf '%s\n' \ "## Pull request overview" \ "" \ + "OpenCode reviewed the current-head bounded evidence and found no blocking issues." \ + "" \ "$model_summary" \ "" \ "## Findings" \ diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 4ab0c1b8..09a045bf 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -4,7 +4,7 @@ on: push: branches: [main, develop, master] pull_request_target: - types: [opened, synchronize, reopened, ready_for_review, auto_merge_enabled, closed] + types: [opened, synchronize, reopened, ready_for_review, closed] pull_request_review: types: [submitted, dismissed] workflow_run: diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index c13adfe9..f1bdaeeb 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -120,6 +120,13 @@ DEFAULT_UPDATE_BRANCH_HEAD_POLL_SECONDS = 5.0 OPENCODE_WORKFLOW_NAMES = {"OpenCode Review", "Required OpenCode Review"} RUNNING_CHECK_STATES = {"PENDING", "EXPECTED", "QUEUED", "IN_PROGRESS", "WAITING", "REQUESTED"} +ACTIVE_WORKFLOW_RUN_STATUSES = ( + "queued", + "in_progress", + "pending", + "waiting", + "requested", +) FAILED_CHECK_CONCLUSIONS = {"FAILURE", "ERROR", "CANCELLED", "TIMED_OUT", "STARTUP_FAILURE"} ACTION_REQUIRED_CONCLUSIONS = {"ACTION_REQUIRED"} GIT_REF_RE = re.compile(r"^(?!-)[A-Za-z0-9._/-]+$") @@ -177,13 +184,40 @@ class Decision: SENSITIVE_DATA_SCRUB_PATTERNS = ( - (re.compile(r'(?i)(bearer\s+)[^\s"\'\\]+'), r'\1***'), - (re.compile(r'(?i)(token\s+)[^\s"\'\\]+'), r'\1***'), + ( + re.compile( + r"-----BEGIN ([A-Z0-9 ]*PRIVATE KEY)-----.*?-----END \1-----", + re.DOTALL, + ), + "***PRIVATE KEY***", + ), + ( + re.compile( + r"(?i)\b((?:https?|ssh|git|postgres(?:ql)?|mysql)://)" + r"[^/\s:@]+:[^@/\s]+@" + ), + r"\1***:***@", + ), + ( + re.compile( + r"(?i)([\"']?(?:api[_-]?key|access[_-]?token|refresh[_-]?token|" + r"id[_-]?token|client[_-]?secret|password|passwd|secret|" + r"aws[_-]?secret[_-]?access[_-]?key|aws[_-]?session[_-]?token)" + r"[\"']?\s*:\s*)[\"'][^\"'\r\n]+[\"']" + ), + r'\1"***"', + ), + (re.compile(r'(?i)(bearer\s+)[A-Za-z0-9._~+/=-]{8,}'), r'\1***'), + (re.compile(r'(?i)(token\s+)[A-Za-z0-9._~+/=-]{8,}'), r'\1***'), (re.compile(r'(?i)\b(?:github_pat_[A-Za-z0-9_]+|gh[pousr]_[A-Za-z0-9_]+)\b'), '***'), (re.compile(r'\b(sk-[A-Za-z0-9_-]+)'), '***'), (re.compile(r'\b(xox[baprs]-[A-Za-z0-9-]+)'), '***'), - (re.compile(r'\b(AKIA[0-9A-Z]{16})'), '***'), - (re.compile(r'(?i)((?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|password|passwd|secret)\s*[:=]\s*)["\']?[^"\'\s]+["\']?'), r'\1***'), + (re.compile(r'\b((?:AKIA|ASIA)[0-9A-Z]{16})'), '***'), + ( + re.compile(r'\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\b'), + '***', + ), + (re.compile(r'(?i)((?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|password|passwd|secret|(?:aws[_-]?)?secret[_-]?access[_-]?key|(?:aws[_-]?)?session[_-]?token)\s*[:=]\s*)["\']?[^"\'\s,}]+["\']?'), r'\1***'), (re.compile(r'(?i)((?:authorization|proxy-authorization)\s*:\s*(?:bearer|basic)\s+)[A-Za-z0-9._~+\/=-]+'), r'\1***'), ) @@ -197,6 +231,14 @@ def scrub_sensitive_data(text: str | None) -> str | None: return text +def review_thread_evidence_available(pr: dict[str, Any]) -> bool: + """Return whether the PR payload contains authoritative thread evidence.""" + if pr.get("reviewThreadEvidenceAvailable") is False: + return False + review_threads = pr.get("reviewThreads") + return isinstance(review_threads, dict) and isinstance(review_threads.get("nodes"), list) + + def mutation_token_source() -> str: """Return the configured scheduler mutation credential source.""" return (os.environ.get("SCHEDULER_MUTATION_TOKEN_SOURCE") or "github-token").strip() or "github-token" @@ -727,7 +769,10 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: "maintainerCanModify": bool(pr.get("maintainer_can_modify")), "headRepository": {"nameWithOwner": head_repo.get("full_name") or repo}, "autoMergeRequest": pr.get("auto_merge"), - "reviewThreads": {"nodes": []}, + # The REST API does not expose review threads. An empty list would be a + # dangerous false claim that there are zero unresolved conversations. + "reviewThreads": None, + "reviewThreadEvidenceAvailable": False, "files": {"nodes": [{"path": file.get("filename")} for file in files if file.get("filename")]}, "reviews": {"nodes": [rest_review_node(review) for review in reviews]}, "statusCheckRollup": { @@ -1343,23 +1388,15 @@ def dismiss_stale_opencode_change_requests(repo: str, pr: dict[str, Any], *, dry f"expected {expected_head}, observed {live_head or ''}" ) + dismissed = 0 for review_id in review_ids: message = ( "Superseded automated OpenCode change request from a previous head; " f"exact current head {expected_head} has a later OpenCode approval." ) - run( - [ - "gh", - "api", - "-X", - "PUT", - f"repos/{repo}/pulls/{number}/reviews/{review_id}/dismissals", - "-f", - f"message={message}", - ] - ) - return len(review_ids) + if dismiss_pull_request_review(repo, number, review_id, message=message): + dismissed += 1 + return dismissed def failed_status_checks(pr: dict[str, Any]) -> list[str]: @@ -1446,16 +1483,23 @@ def run_head_guarded_merge( auto: bool, ) -> None: """Run a head-guarded merge using an allowed repository merge method.""" + merge_flag = repository_auto_merge_flag(repo) if auto else "--squash" args = ["gh", "pr", "merge", number, "--repo", repo] if auto: args.append("--auto") - args.extend(["--squash", "--match-head-commit", head]) + print( + f"PR #{number}: enabling auto-merge with repository-enabled method " + f"{merge_flag.removeprefix('--')} at guarded head {head}." + ) + args.extend([merge_flag, "--match-head-commit", head]) try: run(args) return except RuntimeError as exc: detail = str(exc).lower() - if not any(marker in detail for marker in SQUASH_MERGE_DISABLED_MARKERS): + if merge_flag != "--squash" or not any( + marker in detail for marker in SQUASH_MERGE_DISABLED_MARKERS + ): raise reason = str(exc).splitlines()[-1][:400] @@ -1471,6 +1515,35 @@ def run_head_guarded_merge( run(merge_args) +def repository_auto_merge_flag(repo: str) -> str: + """Return the first merge method enabled in repository settings.""" + raw = run_github_read(["gh", "api", f"repos/{repo}"]) + try: + settings = json.loads(raw) + except json.JSONDecodeError as exc: + raise RuntimeError( + f"Could not select an auto-merge method for {repo}: " + "repository settings were not valid JSON." + ) from exc + if not isinstance(settings, dict): + raise RuntimeError( + f"Could not select an auto-merge method for {repo}: " + "repository settings were not an object." + ) + + for setting, flag in ( + ("allow_squash_merge", "--squash"), + ("allow_merge_commit", "--merge"), + ("allow_rebase_merge", "--rebase"), + ): + if settings.get(setting) is True: + return flag + raise RuntimeError( + f"Could not enable auto-merge for {repo}: repository settings expose " + "no enabled merge method." + ) + + 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.""" number = str(pr["number"]) @@ -1822,7 +1895,9 @@ def rerun_actions_job(repo: str, job_id: str, *, dry_run: bool, action: str) -> run_github_actions(["gh", "api", "-X", "POST", f"repos/{repo}/actions/jobs/{job_id}/rerun"]) -def active_workflow_runs(repo: str, statuses: Sequence[str] = ("queued", "in_progress")) -> list[dict[str, Any]]: +def active_workflow_runs( + repo: str, statuses: Sequence[str] = ACTIVE_WORKFLOW_RUN_STATUSES +) -> list[dict[str, Any]]: """Return active workflow runs for a repository.""" runs: list[dict[str, Any]] = [] for status in statuses: @@ -1855,7 +1930,7 @@ def stale_pr_run_ids( pr: dict[str, Any], *, workflow: str | None = None, - statuses: Sequence[str] = ("queued", "in_progress"), + statuses: Sequence[str] = ACTIVE_WORKFLOW_RUN_STATUSES, ) -> list[str]: """Return active run ids for older heads of the same pull request.""" head = str(pr.get("headRefOid") or "").lower() @@ -1887,9 +1962,16 @@ def active_review_run_refs( *, run_title: str, workflow_aliases: frozenset[str], - statuses: Sequence[str] = ("queued", "in_progress"), + dispatch_workflow_paths: frozenset[str], + statuses: Sequence[str] = ACTIVE_WORKFLOW_RUN_STATUSES, ) -> tuple[list[tuple[str, str]], list[tuple[str, str]]]: - """Return repository-qualified current and stale review workflow runs.""" + """Return repository-qualified current and stale review workflow runs. + + GitHub exposes a workflow's dynamic ``run-name`` as both ``name`` and + ``display_title`` for repository-dispatch runs. Match the guarded title + before fixed workflow aliases, while requiring the canonical trusted + workflow path so another dispatch workflow cannot spoof dedupe evidence. + """ target_repo = validate_github_repository(repo) dispatch_repo = repository_dispatch_target(target_repo) repositories = tuple(dict.fromkeys((target_repo, dispatch_repo))) @@ -1902,15 +1984,15 @@ def active_review_run_refs( for run_repo in repositories: for run_data in active_workflow_runs(run_repo, statuses): run_name = str(run_data.get("name") or "") - if run_name != workflow and run_name not in workflow_aliases: - continue run_id = run_data.get("id") if not run_id: continue run_ref = (run_repo, str(run_id)) display_title = str(run_data.get("display_title") or "") if ( - run_data.get("event") == "repository_dispatch" + run_repo == dispatch_repo + and run_data.get("event") == "repository_dispatch" + and str(run_data.get("path") or "") in dispatch_workflow_paths and display_title.startswith(dispatch_title_prefix) ): dispatched_head = display_title.removeprefix(dispatch_title_prefix).lower() @@ -1918,6 +2000,8 @@ def active_review_run_refs( continue (current if dispatched_head == head else stale).append(run_ref) continue + if run_name != workflow and run_name not in workflow_aliases: + continue if run_repo != target_repo: continue run_head = str(run_data.get("head_sha") or "").lower() @@ -1936,7 +2020,7 @@ def active_opencode_run_refs( repo: str, workflow: str, pr: dict[str, Any], - statuses: Sequence[str] = ("queued", "in_progress"), + statuses: Sequence[str] = ACTIVE_WORKFLOW_RUN_STATUSES, ) -> tuple[list[tuple[str, str]], list[tuple[str, str]]]: """Return repository-qualified current and stale OpenCode run references. @@ -1952,6 +2036,7 @@ def active_opencode_run_refs( pr, run_title="Required OpenCode Review", workflow_aliases=frozenset(OPENCODE_WORKFLOW_NAMES), + dispatch_workflow_paths=frozenset({".github/workflows/opencode-review.yml"}), statuses=statuses, ) @@ -1960,7 +2045,7 @@ def active_opencode_run_ids( repo: str, workflow: str, pr: dict[str, Any], - statuses: Sequence[str] = ("queued", "in_progress"), + statuses: Sequence[str] = ACTIVE_WORKFLOW_RUN_STATUSES, ) -> tuple[list[str], list[str]]: """Return current-head and stale OpenCode run ids for one pull request. @@ -2111,6 +2196,7 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry pr, run_title="Strix Security Scan", workflow_aliases=frozenset({"Strix Security Scan"}), + dispatch_workflow_paths=frozenset({".github/workflows/strix.yml"}), ) force_cancel_workflow_run_refs(stale_run_refs) if current_run_refs: @@ -2273,7 +2359,77 @@ def inspect_pr( f"stacked PR onto {base_ref}; OpenCode review {opencode_state}", ) + if not review_thread_evidence_available(pr): + return Decision( + number, + "wait", + "review-thread evidence unavailable; refuse merge and auto-merge until the " + "GraphQL reviewThreads query succeeds for the current head", + ) + outdated_cleanup_count = resolve_outdated_review_threads(pr, dry_run=dry_run) + if outdated_cleanup_count and not dry_run: + observed_head = str(pr.get("headRefOid") or "") + try: + refreshed_prs = fetch_pr(repo, int(number)) + except RuntimeError as exc: + return with_outdated_thread_cleanup_note( + Decision( + number, + "wait", + "review-thread refresh failed after outdated-thread cleanup; refuse merge " + f"until a fresh current-head snapshot succeeds: {bounded_error_summary(str(exc), limit=180)}", + ), + outdated_cleanup_count, + dry_run=False, + ) + if not refreshed_prs: + return with_outdated_thread_cleanup_note( + Decision( + number, + "wait", + "pull request disappeared during outdated-thread cleanup; refuse further mutation", + ), + outdated_cleanup_count, + dry_run=False, + ) + refreshed_pr = refreshed_prs[0] + refreshed_head = str(refreshed_pr.get("headRefOid") or "") + if refreshed_head != observed_head: + return with_outdated_thread_cleanup_note( + Decision( + number, + "wait", + "pull request head changed during outdated-thread cleanup; rerun the scheduler " + "against the new head before any merge mutation", + ), + outdated_cleanup_count, + dry_run=False, + ) + if not review_thread_evidence_available(refreshed_pr): + return with_outdated_thread_cleanup_note( + Decision( + number, + "wait", + "review-thread refresh fell back to incomplete REST evidence after cleanup; " + "refuse merge and auto-merge until GraphQL reviewThreads succeeds", + ), + outdated_cleanup_count, + dry_run=False, + ) + if refreshed_pr.get("baseRefName") != base_branch: + return with_outdated_thread_cleanup_note( + Decision( + number, + "wait", + "pull request base changed during outdated-thread cleanup; rerun the scheduler " + "before any merge mutation", + ), + outdated_cleanup_count, + dry_run=False, + ) + pr = refreshed_pr + base_ref = pr.get("baseRefName") stale_review_cleanup_count = 0 stale_approval_cleanup_count, retained_stale_approval_count = dismiss_stale_opencode_approvals( repo, @@ -2725,7 +2881,10 @@ def print_summary( counts: dict[str, int] = {} for decision in decisions: counts[decision.action] = counts.get(decision.action, 0) + 1 - print(f"PR #{decision.pr}: {decision.action}: {decision.reason}") + print( + f"PR #{decision.pr}: {log_safe_text(decision.action)}: " + f"{log_safe_text(decision.reason)}" + ) write_actions_summary( decisions, counts=counts, @@ -2747,15 +2906,40 @@ def print_summary( ) +def log_safe_text(value: object) -> str: + """Neutralize line breaks and workflow-command sentinels in untrusted output.""" + return ( + str(value) + .replace("\r", r"\r") + .replace("\n", r"\n") + .replace("::", ":\u200b:") + ) + + +MARKDOWN_TEXT_ESCAPES = str.maketrans( + { + "&": "&", + "<": "<", + ">": ">", + "|": "|", + "[": "[", + "]": "]", + "(": "(", + ")": ")", + "`": "`", + "!": "!", + } +) + + def markdown_cell(value: object) -> str: """Escape a value for a compact GitHub Actions summary table cell.""" - return str(value).replace("|", "\\|").replace("\n", "
") + return log_safe_text(value).translate(MARKDOWN_TEXT_ESCAPES) def markdown_code_span(value: object) -> str: """Escape a value for a compact Markdown inline code span.""" - escaped = str(value).replace("`", "\\`") - return f"`{escaped}`" + return f"{markdown_cell(value)}" def write_actions_summary( @@ -2863,7 +3047,8 @@ def conflict_repair_summary(decisions: list[Decision]) -> list[str]: lines.extend( [ "", - f"PR #{decision.pr} is `{state}` against `{base_ref}` from `{head_ref}`:", + f"PR #{decision.pr} is {markdown_code_span(state)} against " + f"{markdown_code_span(base_ref)} from {markdown_code_span(head_ref)}:", "", "```bash", f"gh pr checkout {decision.pr}", @@ -3086,12 +3271,14 @@ def workflow_action_required_summary(decisions: list[Decision]) -> list[str]: def bounded_error_summary(text: str, *, limit: int = 500) -> str: """Cap an action-error message without dropping the actionable prefix.""" - return text if len(text) <= limit else text[: limit - 1].rstrip() + "..." + scrubbed = scrub_sensitive_data(text) or "" + return scrubbed if len(scrubbed) <= limit else scrubbed[: limit - 1].rstrip() + "..." def summarize_action_error(exc: RuntimeError) -> str: """Return a compact, log-safe scheduler action error summary.""" - lines = [line.strip() for line in str(exc).splitlines() if line.strip()] + scrubbed_error = scrub_sensitive_data(str(exc)) or "" + lines = [line.strip() for line in scrubbed_error.splitlines() if line.strip()] if not lines: return "scheduler action failed without stderr" summary = "; ".join(lines[:2]) diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index c8eb1d34..b8e44187 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -8,6 +8,8 @@ import pytest +from scripts.ci.opencode_existing_approval_gate import PRIMARY_APPROVAL_MARKER + def test_code_reviewer_subagent_contract_is_configured(): """Guard the read-only code-reviewer subagent contract.""" @@ -1444,6 +1446,11 @@ def test_merge_scheduler_uses_escalating_mutation_credentials(): assert '--branch-update-limit "$ORG_SWEEP_BRANCH_UPDATE_LIMIT"' in workflow assert "pull_request_review:" in workflow assert "types: [submitted, dismissed]" in workflow + assert ( + "types: [opened, synchronize, reopened, ready_for_review, closed]" + in workflow + ) + assert "auto_merge_enabled" not in workflow assert ( "github.event_name == 'pull_request_review' && " "format('pr-{0}', github.event.pull_request.number)" in workflow @@ -1805,6 +1812,7 @@ def test_opencode_approve_review_publication_failure_fails_closed(): " - name: Publish central OpenCode fast approval", 1 )[1].split(" - name: Publish OpenCode review outcome", 1)[0] assert "continue-on-error: true" in fast_approval + assert PRIMARY_APPROVAL_MARKER in fast_approval assert "def latest_peer_checks:" in fast_approval assert 'group_by([.app.slug // "", .name // ""])' in fast_approval assert fast_approval.count("latest_peer_checks") == 3 diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 1fe10da7..bbbaf280 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -777,6 +777,139 @@ def map(self, func, items): assert len(resolved) == count +def test_rest_fallback_marks_review_threads_unavailable_and_blocks_merge(monkeypatch): + """REST fallback must not turn missing review-thread evidence into zero blockers.""" + payloads = { + "repos/owner/repo/pulls/7/reviews?per_page=100": [], + "repos/owner/repo/commits/head/check-runs?per_page=100": {"check_runs": []}, + "repos/owner/repo/pulls/7/files?per_page=20": [], + } + monkeypatch.setattr(sched, "gh_api_json", payloads.__getitem__) + rest_pr = sched.rest_pr_node( + "owner/repo", + { + "number": 7, + "title": "REST fallback", + "draft": False, + "mergeable": True, + "mergeable_state": "clean", + "head": {"ref": "feature", "sha": "head", "repo": {"full_name": "owner/repo"}}, + "base": {"ref": "main", "sha": "base"}, + }, + ) + + assert rest_pr["reviewThreads"] is None + assert rest_pr["reviewThreadEvidenceAvailable"] is False + decision = inspect( + rest_pr, + trigger_reviews=False, + merge_mode="direct", + ) + assert decision.action == "wait" + assert "review-thread evidence unavailable" in decision.reason + + +def test_outdated_thread_cleanup_refetches_before_merge(monkeypatch): + """A newly active thread discovered after cleanup must stop the merge.""" + original = make_pr( + reviewThreads={ + "nodes": [ + {"id": "old", "isResolved": False, "isOutdated": True}, + ] + }, + reviews={"nodes": [opencode_review("APPROVED", "head")]}, + ) + refreshed = make_pr( + reviewThreads={ + "nodes": [ + {"id": "new", "isResolved": False, "isOutdated": False}, + ] + }, + reviews={"nodes": [opencode_review("APPROVED", "head")]}, + ) + merges = [] + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda *args, **kwargs: None) + monkeypatch.setattr(sched, "resolve_outdated_review_threads", lambda *args, **kwargs: 1) + monkeypatch.setattr(sched, "fetch_pr", lambda repo, number: [refreshed]) + monkeypatch.setattr(sched, "merge_pr", lambda *args, **kwargs: merges.append(args)) + + decision = inspect( + original, + dry_run=False, + trigger_reviews=False, + merge_mode="direct", + ) + + assert decision.action == "block" + assert decision.reason == "1 unresolved review thread(s)" + assert merges == [] + assert decision.notes[0].startswith("Resolved 1 outdated review thread(s)") + + +@pytest.mark.parametrize( + ("refresh_outcome", "reason_fragment"), + [ + ("error", "review-thread refresh failed after outdated-thread cleanup"), + ("missing", "pull request disappeared during outdated-thread cleanup"), + ("head_changed", "pull request head changed during outdated-thread cleanup"), + ("rest_fallback", "review-thread refresh fell back to incomplete REST evidence"), + ("base_changed", "pull request base changed during outdated-thread cleanup"), + ], +) +def test_outdated_thread_cleanup_refresh_failures_stop_mutation( + monkeypatch, + refresh_outcome, + reason_fragment, +): + """Every incomplete or changed refresh snapshot must fail closed.""" + original = make_pr( + reviewThreads={ + "nodes": [ + {"id": "old", "isResolved": False, "isOutdated": True}, + ] + }, + reviews={"nodes": [opencode_review("APPROVED", "head")]}, + ) + + def refresh_pr(repo, number): + assert repo == "owner/repo" + assert number == 1 + if refresh_outcome == "error": + raise RuntimeError("refresh transport failed") + if refresh_outcome == "missing": + return [] + if refresh_outcome == "head_changed": + return [make_pr(headRefOid="new-head")] + if refresh_outcome == "rest_fallback": + return [ + make_pr( + reviewThreads=None, + reviewThreadEvidenceAvailable=False, + ) + ] + return [make_pr(baseRefName="develop")] + + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda *args, **kwargs: None) + monkeypatch.setattr(sched, "resolve_outdated_review_threads", lambda *args, **kwargs: 1) + monkeypatch.setattr(sched, "fetch_pr", refresh_pr) + monkeypatch.setattr( + sched, + "merge_pr", + lambda *args, **kwargs: pytest.fail("refresh failure must stop merge mutation"), + ) + + decision = inspect( + original, + dry_run=False, + trigger_reviews=False, + merge_mode="direct", + ) + + assert decision.action == "wait" + assert reason_fragment in decision.reason + assert decision.notes[0].startswith("Resolved 1 outdated review thread(s)") + + def test_cancel_stale_opencode_runs_uses_bounded_executor_for_multiple_runs(monkeypatch): seen_workers = [] @@ -1563,6 +1696,10 @@ def test_actions_call_gh_with_expected_arguments(monkeypatch): def fake_run(args, stdin=None): calls.append(args) + if args == ["gh", "api", "repos/owner/repo"]: + return json.dumps( + {"allow_squash_merge": False, "allow_merge_commit": True} + ) if args[:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"]: return '{"workflow_runs": []}' return "" @@ -1586,10 +1723,12 @@ def fake_run(args, stdin=None): sched.update_branch("owner/repo", pr, dry_run=False) sched.dispatch_strix_evidence("owner/repo", "Strix Security Scan", pr, dry_run=False) sched.dispatch_opencode_review("owner/repo", "OpenCode Review", pr, dry_run=False) - assert calls[0][:4] == ["gh", "pr", "merge", "1"] - assert "--squash" in calls[0] - assert calls[0][-2:] == ["--match-head-commit", head_sha] - assert calls[1] == [ + assert calls[0] == ["gh", "api", "repos/owner/repo"] + assert calls[1][:4] == ["gh", "pr", "merge", "1"] + assert "--auto" in calls[1] + assert "--merge" in calls[1] + assert calls[1][-2:] == ["--match-head-commit", head_sha] + assert calls[2] == [ "gh", "pr", "merge", @@ -1600,12 +1739,25 @@ def fake_run(args, stdin=None): "--match-head-commit", head_sha, ] - assert calls[2] == ["gh", "pr", "merge", "1", "--repo", "owner/repo", "--disable-auto"] - assert calls[3][:4] == ["gh", "api", "-X", "PUT"] - assert calls[3][-1] == f"expected_head_sha={head_sha}" - assert calls[4][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[5][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[6] == [ + assert calls[3] == ["gh", "pr", "merge", "1", "--repo", "owner/repo", "--disable-auto"] + assert calls[4][:4] == ["gh", "api", "-X", "PUT"] + assert calls[4][-1] == f"expected_head_sha={head_sha}" + active_run_calls = [ + [ + "gh", + "api", + "--method", + "GET", + "repos/owner/repo/actions/runs", + "-f", + f"status={status}", + "-F", + "per_page=100", + ] + for status in sched.ACTIVE_WORKFLOW_RUN_STATUSES + ] + assert calls[5:10] == active_run_calls + assert calls[10] == [ "gh", "api", "-X", @@ -1614,9 +1766,8 @@ def fake_run(args, stdin=None): "--input", "-", ] - assert calls[7][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[8][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[9] == [ + assert calls[11:16] == active_run_calls + assert calls[16] == [ "gh", "api", "-X", @@ -1641,11 +1792,8 @@ def fake_run(args, stdin=None): ) sched.dispatch_opencode_review("owner/repo", "OpenCode Review", required_workflow_pr, dry_run=False) sched.dispatch_strix_evidence("owner/repo", "Strix Security Scan", required_workflow_pr, dry_run=False) - assert calls[:2] == [ - ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs", "-f", "status=queued", "-F", "per_page=100"], - ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs", "-f", "status=in_progress", "-F", "per_page=100"], - ] - assert calls[2:] == [ + assert calls[:5] == active_run_calls + assert calls[5:] == [ ["gh", "api", "-X", "POST", "repos/owner/repo/dispatches", "--input", "-"], ["gh", "api", "-X", "POST", "repos/owner/repo/actions/jobs/202/rerun"], ] @@ -1728,6 +1876,11 @@ def fake_run(args, stdin=None): return "" monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setattr( + sched, + "run_github_read", + lambda _args: json.dumps({"allow_squash_merge": True}), + ) sched.run_head_guarded_merge( "owner/repo", @@ -1739,12 +1892,73 @@ def fake_run(args, stdin=None): assert len(calls) == 2 assert "--squash" in calls[0] assert "--merge" in calls[1] + assert ("--auto" in calls[0]) is auto assert ("--auto" in calls[1]) is auto assert calls[0][-2:] == ["--match-head-commit", head_sha] assert calls[1][-2:] == ["--match-head-commit", head_sha] assert "Squash merges are not allowed" in capsys.readouterr().out +@pytest.mark.parametrize( + ("settings", "expected_flag"), + [ + ({"allow_squash_merge": True, "allow_merge_commit": True}, "--squash"), + ({"allow_squash_merge": False, "allow_merge_commit": True}, "--merge"), + ( + { + "allow_squash_merge": False, + "allow_merge_commit": False, + "allow_rebase_merge": True, + }, + "--rebase", + ), + ], +) +def test_head_guarded_auto_merge_uses_repository_enabled_method( + monkeypatch, capsys, settings, expected_flag +): + calls = [] + head_sha = "a" * 40 + + monkeypatch.setattr(sched, "run_github_read", lambda _args: json.dumps(settings)) + monkeypatch.setattr(sched, "run", lambda args, stdin=None: calls.append(args) or "") + + sched.run_head_guarded_merge("owner/repo", "7", head_sha, auto=True) + + assert calls == [ + [ + "gh", + "pr", + "merge", + "7", + "--repo", + "owner/repo", + "--auto", + expected_flag, + "--match-head-commit", + head_sha, + ] + ] + assert expected_flag.removeprefix("--") in capsys.readouterr().out + + +@pytest.mark.parametrize( + ("payload", "message"), + [ + ("not-json", "not valid JSON"), + ("[]", "not an object"), + ("{}", "no enabled merge method"), + ], +) +def test_repository_auto_merge_flag_explains_invalid_settings( + monkeypatch, payload, message +): + monkeypatch.setattr(sched, "run_github_read", lambda _args: payload) + + with pytest.raises(RuntimeError, match=message): + sched.repository_auto_merge_flag("owner/repo") + + def test_head_guarded_merge_does_not_mask_unrelated_failure(monkeypatch): calls = [] @@ -1785,9 +1999,21 @@ def fake_run_with_env(args, *, stdin=None, env=None): assert [call[2] for call in calls] == ["workflow-actions-token"] * len(calls) assert calls[0][0] == ["gh", "api", "-X", "POST", "repos/owner/repo/actions/jobs/101/rerun"] - assert calls[1][0][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[2][0][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[3][0] == [ + assert [call[0] for call in calls[1:6]] == [ + [ + "gh", + "api", + "--method", + "GET", + "repos/owner/repo/actions/runs", + "-f", + f"status={status}", + "-F", + "per_page=100", + ] + for status in sched.ACTIVE_WORKFLOW_RUN_STATUSES + ] + assert calls[6][0] == [ "gh", "api", "-X", @@ -1796,9 +2022,21 @@ def fake_run_with_env(args, *, stdin=None, env=None): "--input", "-", ] - assert calls[4][0][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[5][0][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[6][0] == [ + assert [call[0] for call in calls[7:12]] == [ + [ + "gh", + "api", + "--method", + "GET", + "repos/owner/repo/actions/runs", + "-f", + f"status={status}", + "-F", + "per_page=100", + ] + for status in sched.ACTIVE_WORKFLOW_RUN_STATUSES + ] + assert calls[12][0] == [ "gh", "api", "-X", @@ -2078,10 +2316,11 @@ def test_dispatch_opencode_review_deduplicates_current_head_repository_dispatch( head_sha = "a" * 40 current_dispatch = { "id": 9100, - "name": "Required OpenCode Review", + "name": f"Required OpenCode Review owner/repo#1@{head_sha}", "event": "repository_dispatch", "head_sha": "default-branch-sha", "display_title": f"Required OpenCode Review owner/repo#1@{head_sha}", + "path": ".github/workflows/opencode-review.yml", "pull_requests": [], } @@ -2094,7 +2333,7 @@ def fake_run(args, stdin=None): "GET", "repos/ContextualWisdomLab/.github/actions/runs", ]: - if "status=queued" in args: + if "status=pending" in args: return json.dumps({"workflow_runs": [current_dispatch]}) return json.dumps({"workflow_runs": []}) if "/actions/runs" in " ".join(args): @@ -2132,18 +2371,20 @@ def test_dispatch_strix_cancels_stale_central_run_and_keeps_current(monkeypatch, central_runs = [ { "id": 9300, - "name": "Strix Security Scan", + "name": f"Strix Security Scan owner/repo#1@{stale_sha}", "event": "repository_dispatch", "head_sha": "default-branch-sha", "display_title": f"Strix Security Scan owner/repo#1@{stale_sha}", + "path": ".github/workflows/strix.yml", "pull_requests": [], }, { "id": 9301, - "name": "Strix Security Scan", + "name": f"Strix Security Scan owner/repo#1@{head_sha}", "event": "repository_dispatch", "head_sha": "default-branch-sha", "display_title": f"Strix Security Scan owner/repo#1@{head_sha}", + "path": ".github/workflows/strix.yml", "pull_requests": [], }, ] @@ -2200,9 +2441,10 @@ def test_central_run_filter_ignores_malformed_and_non_dispatch_titles(monkeypatc central_runs = [ { "id": 9400, - "name": "Required OpenCode Review", + "name": "Required OpenCode Review owner/repo#1@not-a-sha", "event": "repository_dispatch", "display_title": "Required OpenCode Review owner/repo#1@not-a-sha", + "path": ".github/workflows/opencode-review.yml", "pull_requests": [], }, { @@ -2212,6 +2454,14 @@ def test_central_run_filter_ignores_malformed_and_non_dispatch_titles(monkeypatc "head_sha": head_sha, "pull_requests": [{"number": 1}], }, + { + "id": 9402, + "name": f"Required OpenCode Review owner/repo#1@{head_sha}", + "event": "repository_dispatch", + "display_title": f"Required OpenCode Review owner/repo#1@{head_sha}", + "path": ".github/workflows/untrusted-review.yml", + "pull_requests": [], + }, ] def fake_active_runs(repo, statuses=("queued", "in_progress")): @@ -2260,7 +2510,7 @@ def test_active_run_filters_and_stale_opencode_dry_run(monkeypatch): ) == [] -def test_cancel_stale_pr_runs_force_cancels_queued_and_in_progress_old_heads(monkeypatch): +def test_cancel_stale_pr_runs_force_cancels_all_active_old_heads(monkeypatch): calls = [] head_sha = "a" * 40 stale_same_pr = { @@ -2293,6 +2543,12 @@ def test_cancel_stale_pr_runs_force_cancels_queued_and_in_progress_old_heads(mon "head_sha": "older-running-head", "pull_requests": [{"number": 1}], } + stale_pending = { + "id": 9006, + "name": "Required OpenCode Review", + "head_sha": "older-pending-head", + "pull_requests": [{"number": 1}], + } def fake_run(args, stdin=None): calls.append(args) @@ -2301,8 +2557,10 @@ def fake_run(args, stdin=None): runs = [stale_same_pr, current_same_pr, stale_other_pr, stale_strix] elif "status=in_progress" in args: runs = [stale_in_progress] - else: # pragma: no cover - the assertion below exposes new states - raise AssertionError(args) + elif "status=pending" in args: + runs = [stale_pending] + else: + runs = [] return json.dumps({"workflow_runs": runs}) return "" @@ -2316,10 +2574,11 @@ def fake_run(args, stdin=None): dry_run=False, ) - assert run_ids == ["9001", "9004", "9005"] + assert run_ids == ["9001", "9004", "9005", "9006"] assert ["gh", "api", "-X", "POST", "repos/owner/repo/actions/runs/9001/force-cancel"] in calls assert ["gh", "api", "-X", "POST", "repos/owner/repo/actions/runs/9004/force-cancel"] in calls assert ["gh", "api", "-X", "POST", "repos/owner/repo/actions/runs/9005/force-cancel"] in calls + assert ["gh", "api", "-X", "POST", "repos/owner/repo/actions/runs/9006/force-cancel"] in calls assert not any("9002/force-cancel" in " ".join(call) for call in calls) assert not any("9003/force-cancel" in " ".join(call) for call in calls) assert any("status=in_progress" in " ".join(call) for call in calls) @@ -2406,7 +2665,7 @@ def fake_graphql(query, **fields): assert all(query == sched.RESOLVE_REVIEW_THREAD_MUTATION for query, _ in calls) -def test_dismiss_stale_opencode_change_requests_is_current_head_guarded(monkeypatch): +def test_dismiss_stale_opencode_change_requests_is_current_head_guarded(monkeypatch, capsys): exact_head = "a" * 40 unapproved = make_pr( headRefOid=exact_head, @@ -2453,11 +2712,26 @@ def test_dismiss_stale_opencode_change_requests_is_current_head_guarded(monkeypa monkeypatch.setenv("GITHUB_ACTIONS", "true") monkeypatch.setenv("GH_TOKEN", "workflow-token") + states = iter([exact_head, "DISMISSED", "DISMISSED"]) + monkeypatch.setattr(sched, "run_github_read", lambda args, stdin=None: calls.append(args) or next(states)) assert sched.dismiss_stale_opencode_change_requests("owner/repo", pr, dry_run=False) == 2 assert calls[0] == ["gh", "api", "repos/owner/repo/pulls/1", "--jq", ".head.sha"] assert calls[1][:5] == ["gh", "api", "-X", "PUT", "repos/owner/repo/pulls/1/reviews/201/dismissals"] - assert calls[2][:5] == ["gh", "api", "-X", "PUT", "repos/owner/repo/pulls/1/reviews/202/dismissals"] - assert all(call[-2] == "-f" and call[-1].startswith("message=") for call in calls[1:]) + assert calls[2] == ["gh", "api", "repos/owner/repo/pulls/1/reviews/201", "--jq", ".state"] + assert calls[3][:5] == ["gh", "api", "-X", "PUT", "repos/owner/repo/pulls/1/reviews/202/dismissals"] + assert calls[4] == ["gh", "api", "repos/owner/repo/pulls/1/reviews/202", "--jq", ".state"] + assert all(call[-2] == "-f" and call[-1].startswith("message=") for call in (calls[1], calls[3])) + + calls.clear() + monkeypatch.setattr(sched, "run_github_read", lambda args, stdin=None: calls.append(args) or exact_head) + + def reject_dismissal(args, stdin=None): + calls.append(args) + raise RuntimeError("Branch protections do not permit dismissing this review (HTTP 403)") + + monkeypatch.setattr(sched, "run", reject_dismissal) + assert sched.dismiss_stale_opencode_change_requests("owner/repo", pr, dry_run=False) == 0 + assert "Branch protections do not permit dismissing this review (HTTP 403)" in capsys.readouterr().out calls.clear() monkeypatch.setattr(sched, "run_github_read", lambda args, stdin=None: calls.append(args) or ("d" * 40)) @@ -2657,28 +2931,31 @@ def test_print_summary_writes_github_step_summary(monkeypatch, tmp_path, capsys) assert payload["decisions"][5]["guidance"]["head_repository"] == "fork/repo" 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; changed files to inspect first:" in summary + assert "| #7 | block | merge conflict: DIRTY; base=main, head=feature|x; changed files to inspect first:" in summary assert "do not retry update-branch until the conflict is repaired" in summary assert "### Outdated review threads" in summary assert "Would resolve 1 outdated review thread(s)" in summary assert ( "| #8 | update_branch | current-head OpenCode review approved; " - "branch update requested with workflow GITHUB_TOKEN inside GitHub Actions as github-actions[bot] |" + "branch update requested with workflow GITHUB_TOKEN inside GitHub Actions as " + "github-actions[bot] |" ) in summary assert "| #12 | merge | current head is approved; direct merge requested with workflow GITHUB_TOKEN" in summary assert "fresh same-head OpenCode review" in summary assert "### Conflict repair" in summary assert "When GitHub shows `Conflicting`" in summary assert "`update-branch` is not a conflict resolver" in summary - assert "PR #7 is `DIRTY` against `main` from `feature\\|x`:" not in summary - assert "PR #7 is `DIRTY` against `main` from `feature|x`:" in summary + assert ( + "PR #7 is DIRTY against main from " + "feature|x:" + ) in summary assert "gh pr checkout 7" in summary assert "git fetch origin main" in summary assert "git merge --no-ff origin/main" in summary assert "Changed files to inspect first:" in summary - assert "- `scripts/ci/pr_review_merge_scheduler.py`" in summary - assert "- `tests/test_pr_review_merge_scheduler.py`" in summary - assert "- `docs/has\\`tick.md`" in summary + assert "- scripts/ci/pr_review_merge_scheduler.py" in summary + assert "- tests/test_pr_review_merge_scheduler.py" in summary + assert "- docs/has`tick.md" in summary assert "git push --force-with-lease" in summary assert "### Branch update requests" in summary assert "Requested `update-branch` for PR #8 with `workflow GITHUB_TOKEN`" in summary @@ -2695,6 +2972,24 @@ def test_print_summary_writes_github_step_summary(monkeypatch, tmp_path, capsys) assert "- PR #11: ask the author of `fork/repo` to update the branch" in summary +def test_untrusted_output_text_cannot_inject_actions_commands_or_markdown(): + malicious = ( + "src/normal.py\n::error name=compromised::yes\n" + " [CLICK](https://attacker.invalid)" + ) + + log_text = sched.log_safe_text(malicious) + assert "\n" not in log_text + assert "::error" not in log_text + + cell = sched.markdown_cell(malicious) + code = sched.markdown_code_span(malicious) + for rendered in (cell, code): + assert "