From a128c1d128dd445a6d7369a3aba867983d67a803 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 9 Aug 2026 22:31:02 +0900 Subject: [PATCH 1/2] fix(scheduler): fail after summarized action errors --- .../workflows/pr-review-merge-scheduler.yml | 8 +++-- CHANGELOG.md | 3 ++ docs/doctoring/pr-review-merge-scheduler.md | 24 ++++++++++++++ scripts/ci/pr_review_merge_scheduler.py | 13 +++++++- tests/test_pr_review_merge_scheduler.py | 4 +-- .../test_required_workflow_queue_contract.py | 31 +++++++++++++++++++ 6 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 docs/doctoring/pr-review-merge-scheduler.md diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 8e1157060..f06f06bd8 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -854,8 +854,12 @@ jobs: # PR_REVIEW_MERGE_TOKEN does not cover it. The automation can never # merge those PRs regardless, so this is a skipped, non-fatal # "unavailable" repository, not a failure the sweep can act on. Any - # other non-zero exit is a genuine per-repository failure. - if printf '%s' "$sweep_output" | grep -qF "Resource not accessible by integration"; then + # other non-zero exit is a genuine per-repository failure. A + # versioned scheduler payload proves the repository was readable + # and the failure was an attempted per-PR action, even when that + # action's API error contains the same 403 wording. + if printf '%s' "$sweep_output" | grep -qF "Resource not accessible by integration" && + ! printf '%s' "$sweep_output" | grep -qF '"schema_version": "pr-review-merge-scheduler/v2"'; then echo "::warning::Skipping ${repo_full_name}: the sweep credential lacks access (HTTP 403 Resource not accessible by integration). Install the OpenCode app on this repository or grant PR_REVIEW_MERGE_TOKEN access to include it in the sweep." unavailable=$((unavailable + 1)) unavailable_repos+=("$repo_full_name") diff --git a/CHANGELOG.md b/CHANGELOG.md index bf30091dd..7e817c320 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Made pull-request scheduler mutation and dispatch failures fail the targeted + workflow and organization sweep after the complete structured decision + summary is emitted, while ordinary policy waits remain successful. - Bounded the Strix quality self-test's deterministic timeout fixtures to 3-second process and 5-second fake-sleep budgets so exact-head policy evidence completes inside the existing job limit without changing production Strix scanner timeouts, providers, credentials, or review semantics. - Allowed commas and ASCII parentheses in the bounded Strix changed-file path policy so legal tracked Packrat fixtures can receive exact-head security analysis, while rejecting raw `..` components before normalization and keeping controls, backslashes, whitespace ambiguity, and shell punctuation fail-closed. - Bound each review-agent invocation key to the wrapper's complete canonical payload, including the base branch and requesting actor; altered fields with a valid-format key now fail before durable-leader election or forwarding, and wrapper write permission is job-scoped. diff --git a/docs/doctoring/pr-review-merge-scheduler.md b/docs/doctoring/pr-review-merge-scheduler.md new file mode 100644 index 000000000..abfe3431e --- /dev/null +++ b/docs/doctoring/pr-review-merge-scheduler.md @@ -0,0 +1,24 @@ +# PR review and merge scheduler + +## Terminal result policy + +The scheduler isolates a failed mutation or dispatch to its pull request and +continues the bounded scan. It emits the human-readable lines, job summary, and +versioned JSON decision payload for every inspected pull request before choosing +the process result. + +An `action_error` is a material execution failure, so one or more such decisions +produce a non-zero terminal result after the summary is written. Policy outcomes +such as `wait`, `block`, `skip`, and deferred capacity do not make an otherwise +healthy scheduler invocation fail. + +Targeted single-pull-request runs and the organization sweep use the same +terminal policy. The organization sweep preserves each repository's captured +summary, records that repository as failed, finishes its bounded repository +walk, and then fails the job. A repository is classified as unavailable only +when the scheduler fails before emitting its versioned structured payload and +the error proves that the sweep credential cannot read the repository. + +This separation keeps ordinary governance waits visible without reporting them +as incidents, while preventing a failed merge, update, auto-merge, or review +dispatch from producing a passing workflow result. diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 75e18c860..30140656c 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -2774,6 +2774,17 @@ def print_summary( ) +def scheduler_exit_code(decisions: list[Decision]) -> int: + """Return failure after a complete scan when a requested action failed. + + Ordinary policy outcomes remain successful scheduler executions. A caught + ``action_error`` is different: the scheduler attempted a mutation or + dispatch and could not complete it. The caller must receive that failure + only after :func:`print_summary` has preserved every per-PR decision. + """ + return 1 if any(decision.action == "action_error" for decision in decisions) else 0 + + def markdown_cell(value: object) -> str: """Escape a value for a compact GitHub Actions summary table cell.""" return str(value).replace("|", "\\|").replace("\n", "
") @@ -3792,7 +3803,7 @@ def main(argv: list[str]) -> int: base_branch=args.base_branch, project_flow=args.project_flow, ) - return 0 + return scheduler_exit_code(decisions) if __name__ == "__main__": # pragma: no cover diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 3e421e903..e6aad8d14 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -4472,7 +4472,7 @@ def fake_inspect(repo, pr, **kwargs): 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 sched.main(["--repo", "owner/repo", "--base-branch", "main", "--project-flow", "github"]) == 1 assert seen == [1, 2] output = capsys.readouterr().out assert "PR #1: action_error: Command failed (1): gh pr merge 1; GraphQL: Resource not accessible by integration" in output @@ -4561,7 +4561,7 @@ def fake_inspect(repo, pr, **kwargs): 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 sched.main(["--repo", "owner/repo", "--base-branch", "main", "--project-flow", "github"]) == 1 assert seen == [1, 2, 3] output = capsys.readouterr().out assert "PR #1: action_error:" in output diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 233c08584..fbd1a321e 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -803,6 +803,7 @@ def test_org_queue_sweep_treats_inaccessible_repositories_as_non_fatal() -> None # The 403 signal is classified as a skipped, non-fatal "unavailable" repo. assert "ORG_SWEEP_MAX_UNAVAILABLE" in workflow assert 'grep -qF "Resource not accessible by integration"' in workflow + assert "grep -qF '\"schema_version\": \"pr-review-merge-scheduler/v2\"'" in workflow assert "unavailable=$((unavailable + 1))" in workflow assert 'unavailable_repos+=("$repo_full_name")' in workflow assert "the sweep credential lacks access (HTTP 403" in workflow @@ -819,6 +820,36 @@ def test_org_queue_sweep_treats_inaccessible_repositories_as_non_fatal() -> None assert "ORG_SWEEP_MAX_UNAVAILABLE must be a non-negative integer" in workflow +def test_scheduler_action_errors_propagate_after_structured_summary() -> None: + """Targeted and organization scans must fail after retaining their summary.""" + workflow = workflow_text("pr-review-merge-scheduler.yml") + + targeted = workflow.split(" - name: Run scheduler", 1)[1].split( + "\n org-queue-sweep:", 1 + )[0] + assert 'python3 scripts/ci/pr_review_merge_scheduler.py "${args[@]}"' in targeted + assert "continue-on-error: true" not in targeted + assert "|| true" not in targeted + + org_sweep = workflow.split(" org-queue-sweep:", 1)[1] + assert 'sweep_output="$(python3 scripts/ci/pr_review_merge_scheduler.py "${args[@]}" 2>&1)"' in org_sweep + assert "sweep_rc=$?" in org_sweep + assert 'if [ "$sweep_rc" -ne 0 ]; then' in org_sweep + assert "grep -qF '\"schema_version\": \"pr-review-merge-scheduler/v2\"'" in org_sweep + assert "failures=$((failures + 1))" in org_sweep + + +def test_scheduler_exit_policy_is_documented() -> None: + policy = (REPO_ROOT / "docs/doctoring/pr-review-merge-scheduler.md").read_text( + encoding="utf-8" + ) + + assert "action_error" in policy + assert "non-zero" in policy + assert "targeted" in policy + assert "organization sweep" in policy + + def test_fix_scheduler_cancels_superseded_cron_runs() -> None: workflow = workflow_text("pr-review-fix-scheduler.yml") From 323835b0e3f37468252353e2a0adaaa23ea1653b Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 9 Aug 2026 22:41:59 +0900 Subject: [PATCH 2/2] test(scheduler): bind terminal policy to live step name --- docs/doctoring/pr-review-merge-scheduler.md | 2 +- tests/test_required_workflow_queue_contract.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/doctoring/pr-review-merge-scheduler.md b/docs/doctoring/pr-review-merge-scheduler.md index abfe3431e..34e22c92e 100644 --- a/docs/doctoring/pr-review-merge-scheduler.md +++ b/docs/doctoring/pr-review-merge-scheduler.md @@ -12,7 +12,7 @@ produce a non-zero terminal result after the summary is written. Policy outcomes such as `wait`, `block`, `skip`, and deferred capacity do not make an otherwise healthy scheduler invocation fail. -Targeted single-pull-request runs and the organization sweep use the same +A targeted single-pull-request run and the organization sweep use the same terminal policy. The organization sweep preserves each repository's captured summary, records that repository as failed, finishes its bounded repository walk, and then fails the job. A repository is classified as unavailable only diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index fbd1a321e..91dc1c5e5 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -824,7 +824,7 @@ def test_scheduler_action_errors_propagate_after_structured_summary() -> None: """Targeted and organization scans must fail after retaining their summary.""" workflow = workflow_text("pr-review-merge-scheduler.yml") - targeted = workflow.split(" - name: Run scheduler", 1)[1].split( + targeted = workflow.split(" - name: Inspect PR review and merge queue", 1)[1].split( "\n org-queue-sweep:", 1 )[0] assert 'python3 scripts/ci/pr_review_merge_scheduler.py "${args[@]}"' in targeted