From b60a597a2e677513bf7c98c90fc78a6c245f9570 Mon Sep 17 00:00:00 2001 From: MarketDataDev02 Date: Mon, 28 Sep 2026 17:34:25 -0300 Subject: [PATCH 1/6] feat: wait for the checks another app reports after CI, before reading it A review is triggered when the Tests or Lint workflow finishes, and it skips while any check on the head commit is still running. Codecov, with its default wait_for_ci, reports only after every other check has finished, so its checks land 35 to 50 seconds after Tests, and nothing triggers a review when they do. Measured on sdk-py: the review read CI from 24 seconds before Codecov finished to 8 seconds after, so it usually reviewed without Codecov, and on 21efa02 it read while Codecov was in progress, skipped, and never came back. `wait_for_checks` lists such checks by name, and `wait_for_checks_seconds` caps the wait (default 300, at most 300). The new `reviewbot wait-for-checks` step, which runs before the Codex lease is borrowed, returns at once while a GitHub Actions job is still running, since its workflow triggers the review again; otherwise it polls until every listed check exists and has finished, or the cap passes. It never fails the job. The default lists nothing, so a repository that does not set it behaves as before. --- .github/workflows/review.yml | 15 +++ docs/setup.md | 21 +++- reviewbot/cli.py | 85 ++++++++++++++++ reviewbot/config.py | 10 ++ reviewbot/defaults/policy.yml | 6 ++ reviewbot/github.py | 16 +++ tests/test_config.py | 29 ++++++ tests/test_github.py | 19 ++++ tests/test_wait_for_checks.py | 183 ++++++++++++++++++++++++++++++++++ tests/test_workflow.py | 12 +++ 10 files changed, 395 insertions(+), 1 deletion(-) create mode 100644 tests/test_wait_for_checks.py diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 359a02a..98ebc6b 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -274,6 +274,21 @@ jobs: persist-credentials: false submodules: false + # A check another app reports after the CI workflows, such as Codecov, + # triggers nothing when it finishes, so a review that read CI before it + # did would skip with nothing to bring it back. The wait comes BEFORE the + # lease is borrowed, so no other repository waits on it. + - name: Wait for the checks the policy lists + if: steps.gate.outputs.trusted == 'true' + env: + GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + REVIEWBOT_FORCE: ${{ inputs.force }} + run: | + uv run --project bot reviewbot wait-for-checks \ + --pr "${{ steps.pr.outputs.number }}" \ + --repo "$GITHUB_REPOSITORY" + # BORROWED, NOT STORED. The real credential never leaves skynet; what # lands here is a copy whose refresh token is a placeholder, so this job # cannot refresh anything and cannot cause `refresh_token_reused` diff --git a/docs/setup.md b/docs/setup.md index b3657f5..d2c5d27 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -200,7 +200,7 @@ the label in the Actions list. ### 4. Add a policy, if the defaults are not what you want `.github/code-review/policy.yml` on the **base** branch. Absent keys keep the -defaults in `reviewbot/defaults/policy.yml`. Three choices actually matter: +defaults in `reviewbot/defaults/policy.yml`. Four choices actually matter: **Which model reviews.** `backends` is a LIST, so naming it replaces the default wholesale rather than adding to it. @@ -234,6 +234,25 @@ with `proof.paths` — and know that it is an ANY-match over the whole pull request, so one matching file gates every file. On sdk-py, `paths: ["src/marketdata/**"]` fires on 7 of 7 open pull requests. +**List the checks another app reports after CI.** Codecov, with its default +`wait_for_ci`, reports only once every other check has finished, so its checks +land after the `Tests` workflow that triggers the review. A review that reads CI +in that gap either misses Codecov or finds it in progress and skips, and nothing +triggers it again. Measured on sdk-py: Codecov reported 35 to 50 seconds after +`Tests`, and the review read CI from 24 seconds before it to 8 seconds after. +Name those checks, and the review waits for them before it reads CI: + +```yaml +wait_for_checks: ["codecov/patch", "codecov/project"] +wait_for_checks_seconds: 300 # the longest it waits, 0 to 300 +``` + +The wait starts only once no GitHub Actions job is left running, so an earlier +trigger still skips at once, and it runs before the Codex lease is borrowed. A +listed check that never appears costs the whole wait, and then the review reads +CI without it. Add the key only once the bot you call knows it: a policy with +an unknown key fails the review. + ### 5. Leave the check advisory until you trust it The `Code review` check is not a required status check anywhere today, so a diff --git a/reviewbot/cli.py b/reviewbot/cli.py index 37b49f8..1ddc002 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -251,6 +251,71 @@ def _fail(api, sha: str, check_name: str, message: str) -> int: return 1 +WAIT_POLL_SECONDS = 10.0 +ACTIONS_APP = "github-actions" + + +def wait_for_checks( + *, + repo: str, + token: str, + pr_number: int, + force: bool = False, + api=None, + sleep=_time.sleep, + clock=_time.monotonic, +) -> int: + """Wait for the checks the policy lists in `wait_for_checks`, before the + review reads CI's state. + + Returns at once while a GitHub Actions job is still running, because its + workflow triggers the review again when it finishes, and when the review is + forced or does not require green CI. Otherwise polls until every listed + check exists and has finished, or `wait_for_checks_seconds` have passed. + Whatever it cannot read, it leaves to `reviewbot run`. Always returns 0. + """ + if force: + return 0 + api = api or GitHub(repo, token) + try: + pull = api.pull_request(pr_number) or {} + base_ref = (pull.get("base") or {}).get("ref", "") + policy = config.load(api.file_at_ref(POLICY_PATH, base_ref or "HEAD")) + except (config.PolicyError, GitHubError) as exc: + print(f"reviewbot wait: {scrub(str(exc))}; the review decides without waiting") + return 0 + listed = policy["wait_for_checks"] + if not listed or not policy["require_ci_green"]: + return 0 + sha = (pull.get("head") or {}).get("sha", "") + seconds = policy["wait_for_checks_seconds"] + deadline = clock() + seconds + while True: + try: + runs = api.check_runs(sha, policy["check_name"]) + except GitHubError as exc: + print(f"reviewbot wait: {scrub(str(exc))}; the review decides without waiting") + return 0 + if any(run["app"] == ACTIONS_APP and run["status"] != "completed" for run in runs): + print( + "reviewbot wait: a GitHub Actions job is still running, and its workflow " + "triggers the review again when it finishes" + ) + return 0 + finished = {run["name"] for run in runs if run["status"] == "completed"} + missing = [name for name in listed if name not in finished] + if not missing: + print(f"reviewbot wait: {', '.join(listed)} finished") + return 0 + if clock() >= deadline: + print( + f"reviewbot wait: {', '.join(missing)} did not finish within {seconds}s; " + "the review reads CI without them" + ) + return 0 + sleep(WAIT_POLL_SECONDS) + + def run( *, event: dict, @@ -947,6 +1012,23 @@ def main(argv: list[str] | None = None) -> int: help="owner/name of the repository", ) + waiter = sub.add_parser( + "wait-for-checks", + help="wait for the checks the policy lists before the review reads CI", + ) + waiter.add_argument("--pr", type=int, required=True, help="pull request number") + waiter.add_argument( + "--repo", + default=os.environ.get("GITHUB_REPOSITORY"), + help="owner/name of the repository", + ) + waiter.add_argument( + "--force", + action="store_true", + default=os.environ.get("REVIEWBOT_FORCE", "") == "true", + help="do not wait: a forced review does not gate on CI", + ) + deriver = sub.add_parser( "derive-credential", help="write the derived copy the review jobs borrow" ) @@ -1046,6 +1128,9 @@ def main(argv: list[str] | None = None) -> int: if not args.repo: parser.error("GITHUB_REPOSITORY is not set and --repo was not given") + if args.command == "wait-for-checks": + return wait_for_checks(repo=args.repo, token=token, pr_number=args.pr, force=args.force) + event = {} if args.event and os.path.exists(args.event): with open(args.event) as handle: diff --git a/reviewbot/config.py b/reviewbot/config.py index 048d983..fca686b 100644 --- a/reviewbot/config.py +++ b/reviewbot/config.py @@ -34,6 +34,8 @@ "codex_reasoning_effort": str, "gate": bool, "require_ci_green": bool, + "wait_for_checks": list, + "wait_for_checks_seconds": int, "proof": dict, "ratings": bool, "decision_packets": bool, @@ -70,6 +72,8 @@ JOB_OVERHEAD_MINUTES = 10.0 # 2 * MAX + overhead must fit the budget, leaving room for a lease wait. MAX_TIMEOUT_MINUTES = 30.0 +# The wait for listed checks comes out of the same budget, before the lease. +MAX_WAIT_FOR_CHECKS_SECONDS = 300 class PolicyError(ValueError): @@ -175,12 +179,18 @@ def _validate(policy: dict) -> None: f"model time against a {JOB_BUDGET_MINUTES:g} minute job budget, and the job would be " f"killed mid-review" ) + if not 0 <= policy["wait_for_checks_seconds"] <= MAX_WAIT_FOR_CHECKS_SECONDS: + raise PolicyError( + f"wait_for_checks_seconds must be from 0 to {MAX_WAIT_FOR_CHECKS_SECONDS}: the wait " + f"comes out of the {JOB_BUDGET_MINUTES:g} minute job budget" + ) for key in ( "ignore_paths", "ignore_authors", "auto_approve_paths", "trusted_associations", "trusted_authors", + "wait_for_checks", ): for item in policy[key]: if not isinstance(item, str): diff --git a/reviewbot/defaults/policy.yml b/reviewbot/defaults/policy.yml index 4060731..0a6edf8 100644 --- a/reviewbot/defaults/policy.yml +++ b/reviewbot/defaults/policy.yml @@ -18,6 +18,12 @@ models: codex_reasoning_effort: high gate: true # check = failure on blocked, else success require_ci_green: true # blocked while the head commit's CI is red +# Checks another app reports after the CI workflows, by check-run name, such as +# ["codecov/patch", "codecov/project"]. Nothing triggers a review when one of +# them finishes, so once no GitHub Actions job is left running the review waits +# for each of them to exist and finish, up to wait_for_checks_seconds. +wait_for_checks: [] +wait_for_checks_seconds: 300 # 0 to 300 proof: # WARNS, it does not block. The model still judges the evidence and still # asks for it under "Before merge"; what it no longer does is overwrite its diff --git a/reviewbot/github.py b/reviewbot/github.py index b73139b..567b22e 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -415,6 +415,22 @@ def check_results(self, sha: str, exclude_check_name: str) -> list[dict]: ) return out + def check_runs(self, sha: str, exclude_check_name: str) -> list[dict]: + """The name, status and app of every check run on a commit but our own.""" + runs = ( + self._request("GET", f"/repos/{self.repo}/commits/{sha}/check-runs?per_page=100").data + or {} + ).get("check_runs", []) + return [ + { + "name": run.get("name") or "", + "status": run.get("status") or "", + "app": (run.get("app") or {}).get("slug") or "", + } + for run in runs + if run.get("name") != exclude_check_name + ] + def ci_state(self, sha: str, exclude_check_name: str) -> str: """`success`, `failure`, `pending` or `none` for everything but our own check.""" runs = ( diff --git a/tests/test_config.py b/tests/test_config.py index 9c5fd92..04dd421 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -138,3 +138,32 @@ def test_an_unknown_association_is_rejected(): def test_the_association_list_may_not_be_empty(): with pytest.raises(config.PolicyError): config.load("trusted_associations: []\n") + + +def test_no_check_is_waited_for_by_default(): + """A repository that lists nothing waits for nothing.""" + policy = config.defaults() + assert policy["wait_for_checks"] == [] + assert policy["wait_for_checks_seconds"] == 300 + + +def test_a_repository_lists_the_checks_to_wait_for(): + """The names and the longest wait come from the repository's policy.""" + policy = config.load( + 'wait_for_checks: ["codecov/patch", "codecov/project"]\nwait_for_checks_seconds: 120\n' + ) + assert policy["wait_for_checks"] == ["codecov/patch", "codecov/project"] + assert policy["wait_for_checks_seconds"] == 120 + + +@pytest.mark.parametrize("seconds", [-1, config.MAX_WAIT_FOR_CHECKS_SECONDS + 1]) +def test_the_wait_must_fit_the_job_budget(seconds): + """A negative wait, or one past the cap, is refused with the key's name.""" + with pytest.raises(config.PolicyError, match="wait_for_checks_seconds"): + config.load(f"wait_for_checks_seconds: {seconds}\n") + + +def test_the_checks_to_wait_for_are_names(): + """Each listed check is a check-run name, so a string.""" + with pytest.raises(config.PolicyError, match="wait_for_checks"): + config.load("wait_for_checks: [1]\n") diff --git a/tests/test_github.py b/tests/test_github.py index adefb52..08aea18 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -176,6 +176,25 @@ def test_ci_state_is_success_when_everything_passed(api, transport): assert api.ci_state("abc", exclude_check_name="Code review") == "success" +def test_check_runs_name_the_app_of_each_and_leave_out_our_own(api, transport): + """Each check run comes with the slug of the app that reports it.""" + transport.add( + "GET", + "/repos/MarketData-App/api/commits/abc/check-runs?per_page=100", + data={ + "check_runs": [ + {"name": "Tests", "status": "completed", "app": {"slug": "github-actions"}}, + {"name": "codecov/project", "status": "in_progress", "app": {"slug": "codecov"}}, + {"name": "Code review", "status": "completed", "app": {"slug": "marketdata"}}, + ] + }, + ) + assert api.check_runs("abc", "Code review") == [ + {"name": "Tests", "status": "completed", "app": "github-actions"}, + {"name": "codecov/project", "status": "in_progress", "app": "codecov"}, + ] + + def test_ci_state_is_none_without_any_check(api, transport): transport.add( "GET", diff --git a/tests/test_wait_for_checks.py b/tests/test_wait_for_checks.py new file mode 100644 index 0000000..7088332 --- /dev/null +++ b/tests/test_wait_for_checks.py @@ -0,0 +1,183 @@ +"""Tests for `reviewbot wait-for-checks`. + +A fake GitHub answers the policy and a scripted list of check runs per read, +and a fake clock that only moves when the code sleeps stands in for time, so +no test waits. + +Run: pytest tests/test_wait_for_checks.py +""" + +from reviewbot import cli +from reviewbot.github import GitHubError + +LISTED = 'wait_for_checks: ["codecov/patch", "codecov/project"]\nwait_for_checks_seconds: 60\n' + + +def actions(status): + """A GitHub Actions check run in the given status.""" + return {"name": "Tests", "status": status, "app": "github-actions"} + + +def codecov(name, status="completed"): + """A Codecov check run with the given name and status.""" + return {"name": name, "status": status, "app": "codecov"} + + +class FakeGitHub: + """Answers the pull request, the policy, and one scripted read of the check + runs per call, repeating the last one.""" + + def __init__(self, policy, reads): + """Keep the policy text and the reads, and count the reads made.""" + self.policy = policy + self.reads = list(reads) + self.seen = 0 + + def pull_request(self, number): + """A pull request on `main` whose head is `abc`.""" + return {"base": {"ref": "main"}, "head": {"sha": "abc"}} + + def file_at_ref(self, path, ref): + """The policy text, whatever the path.""" + return self.policy + + def check_runs(self, sha, exclude_check_name): + """The next scripted read, raised when it is an exception.""" + self.seen += 1 + read = self.reads.pop(0) if len(self.reads) > 1 else self.reads[0] + if isinstance(read, Exception): + raise read + return read + + +class FakeClock: + """A monotonic clock that moves forward only when the code sleeps.""" + + def __init__(self): + """Start at zero, with no sleep recorded.""" + self.now = 0.0 + self.slept = [] + + def __call__(self): + """The current time.""" + return self.now + + def sleep(self, seconds): + """Record the sleep and move the clock forward by it.""" + self.slept.append(seconds) + self.now += seconds + + +def wait(api, clock, force=False): + """Run the wait against the fakes and return its exit code.""" + return cli.wait_for_checks( + repo="o/r", + token="t", + pr_number=1, + force=force, + api=api, + sleep=clock.sleep, + clock=clock, + ) + + +def test_it_waits_until_every_listed_check_exists_and_finishes(capsys): + """The wait polls until both Codecov checks have appeared and finished.""" + api = FakeGitHub( + LISTED, + [ + [actions("completed")], + [actions("completed"), codecov("codecov/project", "in_progress")], + [actions("completed"), codecov("codecov/project"), codecov("codecov/patch")], + ], + ) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert api.seen == 3 + assert clock.slept == [cli.WAIT_POLL_SECONDS] * 2 + assert "codecov/patch, codecov/project finished" in capsys.readouterr().out + + +def test_a_running_actions_job_ends_the_wait_at_once(capsys): + """Its workflow triggers the review again when it finishes, so the earlier + trigger still skips at once instead of waiting beside the later one.""" + api = FakeGitHub(LISTED, [[actions("in_progress"), codecov("codecov/project", "queued")]]) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert clock.slept == [] + assert "a GitHub Actions job is still running" in capsys.readouterr().out + + +def test_it_gives_up_after_the_policy_seconds(capsys): + """A listed check that never appears costs the whole wait, then the review + goes on without it.""" + api = FakeGitHub(LISTED, [[actions("completed"), codecov("codecov/project")]]) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert clock.now >= 60 + assert "codecov/patch did not finish within 60s" in capsys.readouterr().out + + +def test_nothing_is_read_when_no_check_is_listed(): + """The default policy lists no check, so the step reads nothing.""" + api = FakeGitHub("", [[actions("completed")]]) + + assert wait(api, FakeClock()) == 0 + + assert api.seen == 0 + + +def test_a_forced_review_does_not_wait(): + """A forced review does not gate on CI, so there is nothing to wait for.""" + api = FakeGitHub(LISTED, [[actions("completed")]]) + + assert wait(api, FakeClock(), force=True) == 0 + + assert api.seen == 0 + + +def test_a_review_that_does_not_gate_on_ci_does_not_wait(): + """Without require_ci_green the listed checks change nothing.""" + api = FakeGitHub(LISTED + "require_ci_green: false\n", [[actions("completed")]]) + + assert wait(api, FakeClock()) == 0 + + assert api.seen == 0 + + +def test_a_malformed_policy_leaves_the_decision_to_the_review(capsys): + """The review reports a malformed policy; the wait does not fail the job.""" + api = FakeGitHub("wait_for_checks: [1]\n", [[actions("completed")]]) + + assert wait(api, FakeClock()) == 0 + + assert api.seen == 0 + assert "the review decides without waiting" in capsys.readouterr().out + + +def test_an_api_error_leaves_the_decision_to_the_review(capsys): + """A failed read ends the wait instead of failing the job.""" + api = FakeGitHub(LISTED, [GitHubError("GET check-runs: 502")]) + + assert wait(api, FakeClock()) == 0 + + assert "the review decides without waiting" in capsys.readouterr().out + + +def test_the_command_reaches_the_wait(monkeypatch): + """`reviewbot wait-for-checks` passes the pull request, the repository and + the token to the wait.""" + seen = {} + monkeypatch.setenv("GITHUB_TOKEN", "t") + monkeypatch.delenv("REVIEWBOT_FORCE", raising=False) + monkeypatch.setattr(cli, "wait_for_checks", lambda **kwargs: seen.update(kwargs) or 0) + + assert cli.main(["wait-for-checks", "--pr", "7", "--repo", "o/r"]) == 0 + + assert seen == {"repo": "o/r", "token": "t", "pr_number": 7, "force": False} diff --git a/tests/test_workflow.py b/tests/test_workflow.py index 9184118..5724f7c 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -178,6 +178,18 @@ def test_every_step_after_the_gate_is_conditional_on_it(): assert "steps.gate.outputs.trusted == 'true'" in condition, item.get("name") +def test_the_listed_checks_are_waited_for_before_the_lease_is_borrowed(): + """The wait runs after the gate and before the shared lease is taken, so no + other repository's review waits on it.""" + names = [(s.get("name") or "") for s in steps()] + wait_at = names.index("Wait for the checks the policy lists") + assert names.index("Check out the pull request head (read only)") < wait_at + assert wait_at < names.index("Borrow the Codex credential") + waiter = step("wait for the checks the policy lists") + assert "reviewbot wait-for-checks" in waiter["run"] + assert waiter["env"]["REVIEWBOT_FORCE"] == "${{ inputs.force }}" + + def test_the_gate_step_runs_the_tested_command(): gate = step("refuse a pull request") assert "reviewbot gate" in gate["run"] From 581766b9d8d5e6c46bb44eab7b5d042d469add2b Mon Sep 17 00:00:00 2001 From: MarketDataDev02 Date: Mon, 28 Sep 2026 19:00:05 -0300 Subject: [PATCH 2/6] test: date the lease report's hold from the clock, not the calendar `lease_report` reads its window, the last seven days, from the clock. The test fixed its only hold at 2026-09-21T10:00Z, so from 2026-09-28T10:00Z that hold fell outside the window and the test failed on every branch, main included. The hold now starts a day before the test runs. --- tests/test_lease_report.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test_lease_report.py b/tests/test_lease_report.py index 75c4890..12fea1a 100644 --- a/tests/test_lease_report.py +++ b/tests/test_lease_report.py @@ -234,11 +234,15 @@ def test_commits_asks_for_the_lease_path_on_the_lease_branch(): def test_the_command_prints_a_report_for_the_store(monkeypatch, capsys): + """The command reports a ten-minute hold from a day ago, a date taken from + the clock because the command reads its window from the clock too.""" from reviewbot import cli + start = dt.datetime.now(dt.UTC).replace(microsecond=0) - dt.timedelta(days=1) + stamp = "%Y-%m-%dT%H:%M:%SZ" history = [ - freed("MarketData-App/api#1#r1-1", "2026-09-21T10:10:00Z"), - taken("MarketData-App/api#1#r1-1", "2026-09-21T10:00:00Z"), + freed("MarketData-App/api#1#r1-1", (start + dt.timedelta(minutes=10)).strftime(stamp)), + taken("MarketData-App/api#1#r1-1", start.strftime(stamp)), ] class FakeApi: From cda96fff3c75def6bc7b5ff50a32b17b59f08303 Mon Sep 17 00:00:00 2001 From: MarketDataDev02 Date: Tue, 29 Sep 2026 20:26:57 -0300 Subject: [PATCH 3/6] fix: the wait ends on what the review gates on, and cannot fail the job The shortcut that ended the wait while any GitHub Actions check run was unfinished assumed that job's workflow would trigger the review again. Only the caller's `workflow_run.workflows` do, and only for a pull_request run: a CodeQL or Copilot job, a push-event run of Tests or a reporter workflow ended the wait, `run` skipped on the pending check, and nothing brought the review back (review finding 55a3117e). The wait now ends once every listed check exists and no check run is still running, from any app. With two trigger workflows both runs wait; the repositories that list Codecov borrow the Codex lease, so the second run then finds the head reviewed. Also: - a red conclusion ends the wait at once, since `run` skips it anyway; - at the deadline the log says whether the review reads CI without a check that never appeared, or skips because one is still running; - the last sleep ends at the deadline instead of overrunning it (7941a128); - `check_runs` reads every page and returns the conclusion (48de260e); - a transport error that is not a GitHubError no longer escapes the wait, and the step ends in `|| true`, so the wait can never skip the review; - the step runs before the pull request checkout, so a push during the wait is fetched instead of the new head being reviewed against the old files; - `_lease_minutes` leaves room for the wait when checks are listed and require_ci_green is on: from timeout_minutes 17 the worst case was 93 to 95 minutes against the job's 90; - a policy may not list the review's own check, which the wait never sees; - `lease_report` takes `now`, so its test uses fixed dates again (5e548d48); - setup.md says the wait needs require_ci_green, that a failing listed check skips the review, and that the names are check-run names. --- .github/workflows/review.yml | 29 ++-- docs/setup.md | 26 ++-- reviewbot/cli.py | 152 +++++++++++---------- reviewbot/config.py | 10 ++ reviewbot/defaults/policy.yml | 8 +- reviewbot/github.py | 23 +++- tests/test_checkout_command.py | 25 +++- tests/test_config.py | 13 ++ tests/test_github.py | 32 ++++- tests/test_lease_report.py | 13 +- tests/test_wait_for_checks.py | 235 ++++++++++++++++++++++++++++----- tests/test_workflow.py | 16 ++- 12 files changed, 419 insertions(+), 163 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 98ebc6b..a1c6eb1 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -261,6 +261,20 @@ jobs: --pr "${{ steps.pr.outputs.number }}" \ --repo "$GITHUB_REPOSITORY" + # Before the head checkout, so a push during the wait is still fetched. It + # holds this runner but never the Codex lease, and `|| true` keeps it from + # failing the job. + - name: Wait for the checks the policy lists + if: steps.gate.outputs.trusted == 'true' + env: + GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + REVIEWBOT_FORCE: ${{ inputs.force }} + run: | + uv run --project bot reviewbot wait-for-checks \ + --pr "${{ steps.pr.outputs.number }}" \ + --repo "$GITHUB_REPOSITORY" || true + - name: Check out the pull request head (read only) if: steps.gate.outputs.trusted == 'true' uses: actions/checkout@v7 @@ -274,21 +288,6 @@ jobs: persist-credentials: false submodules: false - # A check another app reports after the CI workflows, such as Codecov, - # triggers nothing when it finishes, so a review that read CI before it - # did would skip with nothing to bring it back. The wait comes BEFORE the - # lease is borrowed, so no other repository waits on it. - - name: Wait for the checks the policy lists - if: steps.gate.outputs.trusted == 'true' - env: - GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} - GITHUB_REPOSITORY: ${{ github.repository }} - REVIEWBOT_FORCE: ${{ inputs.force }} - run: | - uv run --project bot reviewbot wait-for-checks \ - --pr "${{ steps.pr.outputs.number }}" \ - --repo "$GITHUB_REPOSITORY" - # BORROWED, NOT STORED. The real credential never leaves skynet; what # lands here is a copy whose refresh token is a placeholder, so this job # cannot refresh anything and cannot cause `refresh_token_reused` diff --git a/docs/setup.md b/docs/setup.md index d2c5d27..1a3dbb4 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -154,8 +154,9 @@ Its comments carry four things that each cost a failed run: and the run reports success while doing nothing. - **Name every CI workflow** in `workflow_run.workflows`, by its `name:` and not its filename. The gate looks at every check on the commit, so whichever - finishes last is the one whose review proceeds. Earlier triggers skip cheaply; - a repeat on the same head skips too. Extra triggers are self-deduplicating. + finishes last is the one whose review proceeds. Earlier triggers skip cheaply, + or wait under `wait_for_checks` (section 4); a repeat on the same head skips + too. Extra triggers are self-deduplicating. - **Only review pull requests.** `workflow_run` fires for default-branch pushes as well, because CI runs there too. Without `github.event.workflow_run.event == 'pull_request'`, every merge starts a job @@ -240,18 +241,27 @@ land after the `Tests` workflow that triggers the review. A review that reads CI in that gap either misses Codecov or finds it in progress and skips, and nothing triggers it again. Measured on sdk-py: Codecov reported 35 to 50 seconds after `Tests`, and the review read CI from 24 seconds before it to 8 seconds after. -Name those checks, and the review waits for them before it reads CI: +Name those checks by their check-run names, and the review waits for them +before it reads CI: ```yaml wait_for_checks: ["codecov/patch", "codecov/project"] wait_for_checks_seconds: 300 # the longest it waits, 0 to 300 ``` -The wait starts only once no GitHub Actions job is left running, so an earlier -trigger still skips at once, and it runs before the Codex lease is borrowed. A -listed check that never appears costs the whole wait, and then the review reads -CI without it. Add the key only once the bot you call knows it: a policy with -an unknown key fails the review. +The wait runs only while `require_ci_green` is on, and not for a forced review. +It ends once every listed check exists and no check run on the head is still +running, from any app, so the earlier of two trigger workflows waits too: a +repository that borrows the Codex lease still reviews once, because the later +run finds the head reviewed, and any other may review twice. It ends early when +CI is already red. It holds the job's runner, which on the self-hosted runner +other repositories share, but never the Codex lease. + +A listed check that fails makes the review skip, like any red check: list +`codecov/patch` only if patch coverage should hold back the review, and +`codecov/project` alone otherwise. A listed check that never appears costs the +whole wait, and then the review reads CI without it. Add the key only once the +bot you call knows it: a policy with an unknown key fails the review. ### 5. Leave the check advisory until you trust it diff --git a/reviewbot/cli.py b/reviewbot/cli.py index 1ddc002..2602e74 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -252,7 +252,7 @@ def _fail(api, sha: str, check_name: str, message: str) -> int: WAIT_POLL_SECONDS = 10.0 -ACTIONS_APP = "github-actions" +RED_CONCLUSIONS = ("failure", "timed_out", "action_required") def wait_for_checks( @@ -265,55 +265,72 @@ def wait_for_checks( sleep=_time.sleep, clock=_time.monotonic, ) -> int: - """Wait for the checks the policy lists in `wait_for_checks`, before the - review reads CI's state. - - Returns at once while a GitHub Actions job is still running, because its - workflow triggers the review again when it finishes, and when the review is - forced or does not require green CI. Otherwise polls until every listed - check exists and has finished, or `wait_for_checks_seconds` have passed. - Whatever it cannot read, it leaves to `reviewbot run`. Always returns 0. + """Wait until every check the base-branch policy lists in `wait_for_checks` + exists and no check run is still running, for at most + `wait_for_checks_seconds`. + + `repo`, `token` and `pr_number` name the pull request, `force` skips the + wait, and `api`, `sleep` and `clock` replace the GitHub client, the sleep + and the monotonic clock. Returns 0 and raises nothing: what it cannot read, + and whether to review at all, it leaves to `reviewbot run`. """ if force: return 0 - api = api or GitHub(repo, token) try: - pull = api.pull_request(pr_number) or {} - base_ref = (pull.get("base") or {}).get("ref", "") - policy = config.load(api.file_at_ref(POLICY_PATH, base_ref or "HEAD")) + _wait_for_listed_checks(api or GitHub(repo, token), pr_number, sleep, clock) except (config.PolicyError, GitHubError) as exc: print(f"reviewbot wait: {scrub(str(exc))}; the review decides without waiting") - return 0 + except Exception as exc: # noqa: BLE001 - the wait must never fail the review + print( + f"reviewbot wait: {type(exc).__name__}: {scrub(str(exc))}; " + "the review decides without waiting" + ) + return 0 + + +def _wait_for_listed_checks(api, pr_number: int, sleep, clock) -> None: + """Poll the head commit's check runs until the listed checks and everything + else have finished, CI is red, or the policy's seconds have passed. + + `api` is the GitHub client, `pr_number` the pull request, and `sleep` and + `clock` the sleep and the monotonic clock. Returns None; raises + `config.PolicyError` for a malformed policy, and `GitHubError` or the + transport's own error for a failed read. + """ + pull = api.pull_request(pr_number) or {} + base_ref = (pull.get("base") or {}).get("ref", "") + policy = config.load(api.file_at_ref(POLICY_PATH, base_ref or "HEAD")) listed = policy["wait_for_checks"] if not listed or not policy["require_ci_green"]: - return 0 + return sha = (pull.get("head") or {}).get("sha", "") seconds = policy["wait_for_checks_seconds"] deadline = clock() + seconds while True: - try: - runs = api.check_runs(sha, policy["check_name"]) - except GitHubError as exc: - print(f"reviewbot wait: {scrub(str(exc))}; the review decides without waiting") - return 0 - if any(run["app"] == ACTIONS_APP and run["status"] != "completed" for run in runs): - print( - "reviewbot wait: a GitHub Actions job is still running, and its workflow " - "triggers the review again when it finishes" - ) - return 0 - finished = {run["name"] for run in runs if run["status"] == "completed"} - missing = [name for name in listed if name not in finished] - if not missing: + runs = api.check_runs(sha, policy["check_name"]) + if any(run["conclusion"] in RED_CONCLUSIONS for run in runs): + print("reviewbot wait: CI is already red on the head commit; nothing to wait for") + return + present = {run["name"] for run in runs} + missing = [name for name in listed if name not in present] + running = sorted({run["name"] for run in runs if run["status"] != "completed"}) + if not missing and not running: print(f"reviewbot wait: {', '.join(listed)} finished") - return 0 - if clock() >= deadline: - print( - f"reviewbot wait: {', '.join(missing)} did not finish within {seconds}s; " - "the review reads CI without them" - ) - return 0 - sleep(WAIT_POLL_SECONDS) + return + left = deadline - clock() + if left <= 0: + if running: + print( + f"reviewbot wait: {', '.join(running)} still running after {seconds}s; " + "the review skips while CI is unfinished" + ) + else: + print( + f"reviewbot wait: {', '.join(missing)} did not appear within {seconds}s; " + "the review reads CI without them" + ) + return + sleep(min(WAIT_POLL_SECONDS, left)) def run( @@ -637,36 +654,23 @@ def _will_run_codex(policy: dict | None) -> bool: def _lease_minutes(policy: dict | None) -> tuple[float, float]: - """(ttl, wait), both sized from the policy rather than guessed. + """The Codex lease's TTL and how long to wait for it, both in minutes. - A backend review is `for attempt in (1, 2)` around a call bounded by - `timeout_minutes`, so ONE backend can legitimately run for twice that. A - TTL shorter than the work it protects is worse than no TTL: it expires - under a job that is still working and hands the credential to a second one. - - So ttl covers the worst case with a margin, and the wait exceeds the ttl -- - that ordering is what lets a waiter outlast a holder that died without - checking in, instead of giving up just before the lease frees itself. + `policy` is the target repository's policy, or None when it could not be + read. The TTL covers a review's two attempts of `timeout_minutes` each, and + the wait outlasts the TTL unless the job budget, which must also hold the + overhead and the wait for listed checks, has no room for that. Returns + `(ttl, wait)`; raises nothing. """ - cap = float((policy or {}).get("timeout_minutes") or 15) + policy = policy or {} + cap = float(policy.get("timeout_minutes") or 15) ttl = 2 * cap + 5 wait = ttl + 5 - # The wait must also fit INSIDE the job's own budget alongside the review - # it is waiting to run. `JOB_BUDGET_MINUTES` mirrors review.yml's - # timeout-minutes; a policy with a large timeout_minutes would otherwise - # size a wait that the runner kills mid-queue, which looks like a failed - # review rather than a busy one. - # THE TWO GOALS CONFLICT ABOVE A CERTAIN timeout_minutes, and the arithmetic - # is worth writing down rather than rediscovering. We want - # ttl > 2*cap (outlast the work it protects) - # wait > ttl (outlast a dead holder's lease) - # wait + 2*cap + overhead <= budget (fit inside the job) - # Substituting gives 2*cap < 80 - 2*cap, i.e. cap < 20 for a 90 minute - # budget. Beyond that, fitting the job wins: a wait the runner kills - # mid-queue looks like a failed review, while a wait shorter than the TTL - # only means a job can give up while a DEAD holder's lease is still - # ticking -- rare, and it degrades to a Claude review rather than a red one. - room = JOB_BUDGET_MINUTES - (2 * cap) - JOB_OVERHEAD_MINUTES + checks = 0.0 + if policy.get("wait_for_checks") and policy.get("require_ci_green", True): + checks = policy.get("wait_for_checks_seconds", 0) / 60 + # Fitting the job budget wins over outlasting a dead holder's TTL. + room = JOB_BUDGET_MINUTES - (2 * cap) - JOB_OVERHEAD_MINUTES - checks return ttl, max(0.0, min(wait, room)) @@ -911,27 +915,21 @@ def _positive_days(text: str) -> int: return value -def lease_report(store: str, days: int, token: str) -> int: - """Print who held the shared Codex credential, and for how long. - - UNLIKE the credential commands, this one is allowed to fail. It reviews - nothing: a person runs it to answer "is the shared plan the bottleneck?", - and a report that silently prints an empty week would answer it wrongly. +def lease_report(store: str, days: int, token: str, now=None) -> int: + """Print who held the shared Codex credential in the last `days` days, and + for how long, from the store's commit history. - It reads the store's commit history, which records every HOLD. It cannot - count a run that WAITED -- a blocked job writes no commit -- and the report - it prints says so rather than leaving a reader to assume otherwise. + `store` is the credential store repository, `token` reads it, and `now` is + the end of the window, the current UTC time when None. Returns 0, or 1 when + the store answers with an error; a transport error propagates. """ import datetime as _dt from reviewbot import credentials - now = _dt.datetime.now(_dt.UTC) + now = now or _dt.datetime.now(_dt.UTC) try: - # MARGIN, not the window itself: a hold that began before the window - # and ended inside it arrives as a lone `lease freed` that pairs with - # nothing, so both the hold and its in-window time disappear. - # `credentials.lease_report` clips what this returns back to the window. + # The margin lets a hold that began before the window pair with its release. history = _store_api(store, token).commits( credentials.LEASE_PATH, credentials.LEASE_BRANCH, diff --git a/reviewbot/config.py b/reviewbot/config.py index fca686b..0fbda06 100644 --- a/reviewbot/config.py +++ b/reviewbot/config.py @@ -136,6 +136,11 @@ def _check_type(key: str, value, expected: type) -> None: def _validate(policy: dict) -> None: + """Check a merged policy's types and values. + + `policy` is the defaults with the repository's overrides applied. Returns + None; raises PolicyError naming the first key that is wrong. + """ for key, expected in _TOP_TYPES.items(): _check_type(key, policy[key], expected) for parent, table in _NESTED_TYPES.items(): @@ -198,3 +203,8 @@ def _validate(policy: dict) -> None: for item in policy["proof"]["paths"] + policy["auto_merge"]["authors"]: if not isinstance(item, str): raise PolicyError("proof.paths and auto_merge.authors must hold strings only") + if policy["check_name"] in policy["wait_for_checks"]: + raise PolicyError( + f"wait_for_checks may not list {policy['check_name']!r}, the review's own check, " + f"which the wait never sees" + ) diff --git a/reviewbot/defaults/policy.yml b/reviewbot/defaults/policy.yml index 0a6edf8..09cce49 100644 --- a/reviewbot/defaults/policy.yml +++ b/reviewbot/defaults/policy.yml @@ -18,10 +18,10 @@ models: codex_reasoning_effort: high gate: true # check = failure on blocked, else success require_ci_green: true # blocked while the head commit's CI is red -# Checks another app reports after the CI workflows, by check-run name, such as -# ["codecov/patch", "codecov/project"]. Nothing triggers a review when one of -# them finishes, so once no GitHub Actions job is left running the review waits -# for each of them to exist and finish, up to wait_for_checks_seconds. +# Checks another app reports after CI, by check-run name, such as +# ["codecov/patch", "codecov/project"]. With require_ci_green on, the review +# waits up to wait_for_checks_seconds for each to exist and for every check run +# to finish. A listed check that fails skips the review, like any red check. wait_for_checks: [] wait_for_checks_seconds: 300 # 0 to 300 proof: diff --git a/reviewbot/github.py b/reviewbot/github.py index 567b22e..449742b 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -416,16 +416,27 @@ def check_results(self, sha: str, exclude_check_name: str) -> list[dict]: return out def check_runs(self, sha: str, exclude_check_name: str) -> list[dict]: - """The name, status and app of every check run on a commit but our own.""" - runs = ( - self._request("GET", f"/repos/{self.repo}/commits/{sha}/check-runs?per_page=100").data - or {} - ).get("check_runs", []) + """The name, status and conclusion of every check run on a commit. + + `sha` is the commit and `exclude_check_name` the bot's own check, which + is left out. Every page of the listing is read. Raises GitHubError, or + the transport's own error, when a page cannot be read. + """ + path = f"/repos/{self.repo}/commits/{sha}/check-runs?per_page=100" + runs, page = [], 1 + while True: + reply = self._request("GET", path if page == 1 else f"{path}&page={page}").data + found = reply.get("check_runs") if isinstance(reply, dict) else None + found = found if isinstance(found, list) else [] + runs.extend(run for run in found if isinstance(run, dict)) + if len(found) < 100: + break + page += 1 return [ { "name": run.get("name") or "", "status": run.get("status") or "", - "app": (run.get("app") or {}).get("slug") or "", + "conclusion": run.get("conclusion") or "", } for run in runs if run.get("name") != exclude_check_name diff --git a/tests/test_checkout_command.py b/tests/test_checkout_command.py index 361ce63..bab27b5 100644 --- a/tests/test_checkout_command.py +++ b/tests/test_checkout_command.py @@ -13,7 +13,7 @@ import pytest -from reviewbot import cli, credentials, github +from reviewbot import cli, config, credentials, github from tests.conftest import FakeTransport STORE = "MarketData-App/code-review-credentials" @@ -760,6 +760,29 @@ def test_the_wait_never_outruns_the_job_budget(): assert wait + 2 * cap <= cli.JOB_BUDGET_MINUTES, f"cap={cap} ttl={ttl} wait={wait}" +@pytest.mark.parametrize("cap", range(1, int(config.MAX_TIMEOUT_MINUTES) + 1)) +def test_the_lease_wait_leaves_room_for_the_wait_for_listed_checks(cap): + """Overhead, the wait for listed checks, the lease wait and two review + attempts all fit inside the job's timeout.""" + policy = config.load( + f'timeout_minutes: {cap}\nwait_for_checks: ["codecov/patch"]\n' + f"wait_for_checks_seconds: {config.MAX_WAIT_FOR_CHECKS_SECONDS}\n" + ) + _, wait = cli._lease_minutes(policy) + total = cli.JOB_OVERHEAD_MINUTES + config.MAX_WAIT_FOR_CHECKS_SECONDS / 60 + wait + 2 * cap + assert total <= cli.JOB_BUDGET_MINUTES, f"cap={cap} wait={wait}" + + +@pytest.mark.parametrize( + "text", + ["", 'wait_for_checks: ["codecov/patch"]\nrequire_ci_green: false\n'], +) +def test_a_policy_that_does_not_wait_keeps_the_whole_lease_wait(text): + """No listed check, or no CI gate, means no wait to leave room for.""" + policy = config.load("timeout_minutes: 20\n" + text) + assert cli._lease_minutes(policy) == cli._lease_minutes({"timeout_minutes": 20}) + + def test_an_expired_issued_credential_is_not_borrowed(tmp_path, transport, capsys): """An expired copy authenticates nothing. diff --git a/tests/test_config.py b/tests/test_config.py index 04dd421..a779c43 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -167,3 +167,16 @@ def test_the_checks_to_wait_for_are_names(): """Each listed check is a check-run name, so a string.""" with pytest.raises(config.PolicyError, match="wait_for_checks"): config.load("wait_for_checks: [1]\n") + + +@pytest.mark.parametrize( + "text", + [ + 'wait_for_checks: ["Code review"]\n', + 'check_name: "Review"\nwait_for_checks: ["codecov/patch", "Review"]\n', + ], +) +def test_the_review_cannot_wait_for_its_own_check(text): + """The wait never sees the review's own check, so listing it is refused.""" + with pytest.raises(config.PolicyError, match="own check"): + config.load(text) diff --git a/tests/test_github.py b/tests/test_github.py index 08aea18..e136f8c 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -176,25 +176,43 @@ def test_ci_state_is_success_when_everything_passed(api, transport): assert api.ci_state("abc", exclude_check_name="Code review") == "success" -def test_check_runs_name_the_app_of_each_and_leave_out_our_own(api, transport): - """Each check run comes with the slug of the app that reports it.""" +def test_check_runs_give_the_status_and_conclusion_of_all_but_our_own(api, transport): + """Each check run comes with its status and conclusion; ours is left out.""" transport.add( "GET", "/repos/MarketData-App/api/commits/abc/check-runs?per_page=100", data={ "check_runs": [ - {"name": "Tests", "status": "completed", "app": {"slug": "github-actions"}}, - {"name": "codecov/project", "status": "in_progress", "app": {"slug": "codecov"}}, - {"name": "Code review", "status": "completed", "app": {"slug": "marketdata"}}, + {"name": "Tests", "status": "completed", "conclusion": "failure"}, + {"name": "codecov/project", "status": "in_progress", "conclusion": None}, + {"name": "Code review", "status": "completed", "conclusion": "success"}, ] }, ) assert api.check_runs("abc", "Code review") == [ - {"name": "Tests", "status": "completed", "app": "github-actions"}, - {"name": "codecov/project", "status": "in_progress", "app": "codecov"}, + {"name": "Tests", "status": "completed", "conclusion": "failure"}, + {"name": "codecov/project", "status": "in_progress", "conclusion": ""}, ] +def test_check_runs_read_every_page(api, transport): + """A commit with more than a page of check runs is read to the last one.""" + matrix = [{"name": f"test ({n})", "status": "completed"} for n in range(100)] + transport.add( + "GET", + "/repos/MarketData-App/api/commits/abc/check-runs?per_page=100", + data={"total_count": 101, "check_runs": matrix}, + ) + transport.add( + "GET", + "/repos/MarketData-App/api/commits/abc/check-runs?per_page=100&page=2", + data={"total_count": 101, "check_runs": [{"name": "codecov/patch", "status": "queued"}]}, + ) + runs = api.check_runs("abc", "Code review") + assert len(runs) == 101 + assert runs[-1] == {"name": "codecov/patch", "status": "queued", "conclusion": ""} + + def test_ci_state_is_none_without_any_check(api, transport): transport.add( "GET", diff --git a/tests/test_lease_report.py b/tests/test_lease_report.py index 12fea1a..48ce4f5 100644 --- a/tests/test_lease_report.py +++ b/tests/test_lease_report.py @@ -234,25 +234,24 @@ def test_commits_asks_for_the_lease_path_on_the_lease_branch(): def test_the_command_prints_a_report_for_the_store(monkeypatch, capsys): - """The command reports a ten-minute hold from a day ago, a date taken from - the clock because the command reads its window from the clock too.""" + """The command reports the one ten-minute hold in the week before `now`.""" from reviewbot import cli - start = dt.datetime.now(dt.UTC).replace(microsecond=0) - dt.timedelta(days=1) - stamp = "%Y-%m-%dT%H:%M:%SZ" history = [ - freed("MarketData-App/api#1#r1-1", (start + dt.timedelta(minutes=10)).strftime(stamp)), - taken("MarketData-App/api#1#r1-1", start.strftime(stamp)), + freed("MarketData-App/api#1#r1-1", "2026-09-21T10:10:00Z"), + taken("MarketData-App/api#1#r1-1", "2026-09-21T10:00:00Z"), ] class FakeApi: def commits(self, path, branch, since): + """The scripted history, once the lease file and branch are checked.""" assert path == credentials.LEASE_PATH assert branch == credentials.LEASE_BRANCH return history monkeypatch.setattr(cli, "_store_api", lambda repo, token: FakeApi()) - assert cli.lease_report(API, days=7, token="t") == 0 + now = dt.datetime(2026, 9, 22, tzinfo=dt.UTC) + assert cli.lease_report(API, days=7, token="t", now=now) == 0 out = capsys.readouterr().out assert "1 hold" in out and "1 holds" not in out assert "0h 10m" in out diff --git a/tests/test_wait_for_checks.py b/tests/test_wait_for_checks.py index 7088332..26bb0e5 100644 --- a/tests/test_wait_for_checks.py +++ b/tests/test_wait_for_checks.py @@ -2,48 +2,72 @@ A fake GitHub answers the policy and a scripted list of check runs per read, and a fake clock that only moves when the code sleeps stands in for time, so -no test waits. +no test waits. The last tests use the real client over a fake transport, to pin +the requests themselves. Run: pytest tests/test_wait_for_checks.py """ -from reviewbot import cli +import base64 + +import requests + +from reviewbot import cli, github from reviewbot.github import GitHubError +from tests.conftest import FakeTransport LISTED = 'wait_for_checks: ["codecov/patch", "codecov/project"]\nwait_for_checks_seconds: 60\n' -def actions(status): +def actions(status="completed", conclusion="success", name="Tests"): """A GitHub Actions check run in the given status.""" - return {"name": "Tests", "status": status, "app": "github-actions"} + return { + "name": name, + "status": status, + "conclusion": conclusion if status == "completed" else "", + } def codecov(name, status="completed"): """A Codecov check run with the given name and status.""" - return {"name": name, "status": status, "app": "codecov"} + return { + "name": name, + "status": status, + "conclusion": "success" if status == "completed" else "", + } + + +BOTH = [codecov("codecov/patch"), codecov("codecov/project")] class FakeGitHub: """Answers the pull request, the policy, and one scripted read of the check - runs per call, repeating the last one.""" + runs per call, repeating the last one; records what each read asked for.""" def __init__(self, policy, reads): - """Keep the policy text and the reads, and count the reads made.""" + """Keep the policy text and the reads, with no request recorded yet.""" self.policy = policy self.reads = list(reads) - self.seen = 0 + self.policy_reads = [] + self.check_reads = [] + + @property + def seen(self): + """How many times the check runs were read.""" + return len(self.check_reads) def pull_request(self, number): - """A pull request on `main` whose head is `abc`.""" - return {"base": {"ref": "main"}, "head": {"sha": "abc"}} + """A pull request from `abc` into `release/2.0`.""" + return {"base": {"ref": "release/2.0"}, "head": {"sha": "abc"}} def file_at_ref(self, path, ref): - """The policy text, whatever the path.""" + """The policy text, recording the path and ref asked for.""" + self.policy_reads.append((path, ref)) return self.policy def check_runs(self, sha, exclude_check_name): """The next scripted read, raised when it is an exception.""" - self.seen += 1 + self.check_reads.append((sha, exclude_check_name)) read = self.reads.pop(0) if len(self.reads) > 1 else self.reads[0] if isinstance(read, Exception): raise read @@ -53,15 +77,22 @@ def check_runs(self, sha, exclude_check_name): class FakeClock: """A monotonic clock that moves forward only when the code sleeps.""" + START = 1000.0 + def __init__(self): - """Start at zero, with no sleep recorded.""" - self.now = 0.0 + """Start away from zero, with no sleep recorded.""" + self.now = self.START self.slept = [] def __call__(self): """The current time.""" return self.now + @property + def waited(self): + """The seconds slept since the start.""" + return self.now - self.START + def sleep(self, seconds): """Record the sleep and move the clock forward by it.""" self.slept.append(seconds) @@ -82,13 +113,14 @@ def wait(api, clock, force=False): def test_it_waits_until_every_listed_check_exists_and_finishes(capsys): - """The wait polls until both Codecov checks have appeared and finished.""" + """The wait polls until both Codecov checks have appeared and finished, + reading the base branch's policy and the head commit's check runs.""" api = FakeGitHub( LISTED, [ - [actions("completed")], - [actions("completed"), codecov("codecov/project", "in_progress")], - [actions("completed"), codecov("codecov/project"), codecov("codecov/patch")], + [actions()], + [actions(), codecov("codecov/project", "in_progress")], + [actions(), *BOTH], ], ) clock = FakeClock() @@ -97,36 +129,104 @@ def test_it_waits_until_every_listed_check_exists_and_finishes(capsys): assert api.seen == 3 assert clock.slept == [cli.WAIT_POLL_SECONDS] * 2 + assert api.policy_reads == [(cli.POLICY_PATH, "release/2.0")] + assert api.check_reads == [("abc", "Code review")] * 3 + assert "codecov/patch, codecov/project finished" in capsys.readouterr().out + + +def test_a_listed_check_still_running_keeps_the_wait_going(capsys): + """Both checks exist, but one has not finished, so the wait goes on.""" + api = FakeGitHub( + LISTED, + [ + [actions(), codecov("codecov/patch"), codecov("codecov/project", "in_progress")], + [actions(), *BOTH], + ], + ) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert api.seen == 2 + assert clock.slept == [cli.WAIT_POLL_SECONDS] + assert "finished" in capsys.readouterr().out + + +def test_an_unrelated_actions_workflow_still_running_keeps_the_wait_going(capsys): + """A running Actions job whose workflow does not trigger the review cannot + bring it back, so the wait goes on until it finishes too.""" + api = FakeGitHub( + LISTED, + [ + [actions(), actions("in_progress", name="Analyze (python)"), *BOTH], + [actions(), actions(name="Analyze (python)"), *BOTH], + ], + ) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert api.seen == 2 + assert clock.slept == [cli.WAIT_POLL_SECONDS] assert "codecov/patch, codecov/project finished" in capsys.readouterr().out -def test_a_running_actions_job_ends_the_wait_at_once(capsys): - """Its workflow triggers the review again when it finishes, so the earlier - trigger still skips at once instead of waiting beside the later one.""" - api = FakeGitHub(LISTED, [[actions("in_progress"), codecov("codecov/project", "queued")]]) +def test_red_ci_ends_the_wait_at_once(capsys): + """A failed check makes the review skip whatever else reports, so there is + nothing to wait for.""" + api = FakeGitHub(LISTED, [[actions(conclusion="failure", name="Lint"), actions("in_progress")]]) clock = FakeClock() assert wait(api, clock) == 0 + assert api.seen == 1 assert clock.slept == [] - assert "a GitHub Actions job is still running" in capsys.readouterr().out + assert "CI is already red" in capsys.readouterr().out def test_it_gives_up_after_the_policy_seconds(capsys): - """A listed check that never appears costs the whole wait, then the review - goes on without it.""" - api = FakeGitHub(LISTED, [[actions("completed"), codecov("codecov/project")]]) + """A listed check that never appears costs the whole wait and no more, then + the review goes on without it.""" + api = FakeGitHub(LISTED, [[actions(), codecov("codecov/project")]]) clock = FakeClock() assert wait(api, clock) == 0 - assert clock.now >= 60 - assert "codecov/patch did not finish within 60s" in capsys.readouterr().out + assert clock.waited == 60 + assert api.seen == 7 + assert "codecov/patch did not appear within 60s" in capsys.readouterr().out + + +def test_the_last_sleep_ends_at_the_deadline(): + """A cap that is not a multiple of the poll interval is not overrun.""" + policy = 'wait_for_checks: ["codecov/patch"]\nwait_for_checks_seconds: 15\n' + api = FakeGitHub(policy, [[actions()]]) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert clock.slept == [cli.WAIT_POLL_SECONDS, 5.0] + assert api.seen == 3 + + +def test_a_check_still_running_at_the_deadline_is_reported_as_unfinished(capsys): + """The review skips while a check runs, and the log says so.""" + api = FakeGitHub( + LISTED, [[actions(), codecov("codecov/patch"), codecov("codecov/project", "in_progress")]] + ) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert clock.waited == 60 + out = capsys.readouterr().out + assert "codecov/project still running after 60s" in out + assert "the review skips while CI is unfinished" in out def test_nothing_is_read_when_no_check_is_listed(): - """The default policy lists no check, so the step reads nothing.""" - api = FakeGitHub("", [[actions("completed")]]) + """The default policy lists no check, so no check run is read.""" + api = FakeGitHub("", [[actions()]]) assert wait(api, FakeClock()) == 0 @@ -135,16 +235,17 @@ def test_nothing_is_read_when_no_check_is_listed(): def test_a_forced_review_does_not_wait(): """A forced review does not gate on CI, so there is nothing to wait for.""" - api = FakeGitHub(LISTED, [[actions("completed")]]) + api = FakeGitHub(LISTED, [[actions()]]) assert wait(api, FakeClock(), force=True) == 0 assert api.seen == 0 + assert api.policy_reads == [] def test_a_review_that_does_not_gate_on_ci_does_not_wait(): - """Without require_ci_green the listed checks change nothing.""" - api = FakeGitHub(LISTED + "require_ci_green: false\n", [[actions("completed")]]) + """Without require_ci_green the wait is skipped.""" + api = FakeGitHub(LISTED + "require_ci_green: false\n", [[actions()]]) assert wait(api, FakeClock()) == 0 @@ -153,7 +254,7 @@ def test_a_review_that_does_not_gate_on_ci_does_not_wait(): def test_a_malformed_policy_leaves_the_decision_to_the_review(capsys): """The review reports a malformed policy; the wait does not fail the job.""" - api = FakeGitHub("wait_for_checks: [1]\n", [[actions("completed")]]) + api = FakeGitHub("wait_for_checks: [1]\n", [[actions()]]) assert wait(api, FakeClock()) == 0 @@ -170,6 +271,59 @@ def test_an_api_error_leaves_the_decision_to_the_review(capsys): assert "the review decides without waiting" in capsys.readouterr().out +def raising_transport(method, url, headers, body): + """A transport whose connection always drops.""" + raise requests.ConnectionError("connection reset by peer") + + +def test_a_dropped_connection_leaves_the_decision_to_the_review(capsys): + """A transport error is not a GitHubError, and it must not fail the job + either.""" + api = github.GitHub("o/r", "t", transport=raising_transport, sleep=lambda s: None) + + assert wait(api, FakeClock()) == 0 + + out = capsys.readouterr().out + assert "ConnectionError" in out + assert "the review decides without waiting" in out + + +def policy_content(text): + """A contents-API answer carrying `text`.""" + return {"encoding": "base64", "content": base64.b64encode(text.encode()).decode()} + + +def test_the_real_client_reads_the_base_policy_and_the_head_check_runs(capsys): + """Over the real client, the policy comes from the base branch and the + check runs from the head commit.""" + transport = FakeTransport() + transport.add( + "GET", + "/repos/o/r/pulls/1", + data={"base": {"ref": "release/2.0"}, "head": {"sha": "abc"}}, + ) + transport.add( + "GET", + "/repos/o/r/contents/.github/code-review/policy.yml?ref=release/2.0", + data=policy_content(LISTED), + ) + transport.add( + "GET", + "/repos/o/r/commits/abc/check-runs?per_page=100", + data={"check_runs": [actions(), *BOTH]}, + ) + api = github.GitHub("o/r", "t", transport=transport, sleep=lambda s: None) + + assert wait(api, FakeClock()) == 0 + + assert "codecov/patch, codecov/project finished" in capsys.readouterr().out + assert [call["path"] for call in transport.calls] == [ + "/repos/o/r/pulls/1", + "/repos/o/r/contents/.github/code-review/policy.yml?ref=release/2.0", + "/repos/o/r/commits/abc/check-runs?per_page=100", + ] + + def test_the_command_reaches_the_wait(monkeypatch): """`reviewbot wait-for-checks` passes the pull request, the repository and the token to the wait.""" @@ -181,3 +335,16 @@ def test_the_command_reaches_the_wait(monkeypatch): assert cli.main(["wait-for-checks", "--pr", "7", "--repo", "o/r"]) == 0 assert seen == {"repo": "o/r", "token": "t", "pr_number": 7, "force": False} + + +def test_a_forced_run_reaches_the_wait_as_forced(monkeypatch): + """REVIEWBOT_FORCE, which the workflow sets from its `force` input, reaches + the wait.""" + seen = {} + monkeypatch.setenv("GITHUB_TOKEN", "t") + monkeypatch.setenv("REVIEWBOT_FORCE", "true") + monkeypatch.setattr(cli, "wait_for_checks", lambda **kwargs: seen.update(kwargs) or 0) + + assert cli.main(["wait-for-checks", "--pr", "7", "--repo", "o/r"]) == 0 + + assert seen["force"] is True diff --git a/tests/test_workflow.py b/tests/test_workflow.py index 5724f7c..50a6e81 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -178,15 +178,23 @@ def test_every_step_after_the_gate_is_conditional_on_it(): assert "steps.gate.outputs.trusted == 'true'" in condition, item.get("name") -def test_the_listed_checks_are_waited_for_before_the_lease_is_borrowed(): - """The wait runs after the gate and before the shared lease is taken, so no - other repository's review waits on it.""" +def test_the_listed_checks_are_waited_for_before_the_head_is_fetched(): + """The wait runs after the gate, before the head is checked out, so a push + during the wait is fetched, and before the Codex lease is taken.""" names = [(s.get("name") or "") for s in steps()] wait_at = names.index("Wait for the checks the policy lists") - assert names.index("Check out the pull request head (read only)") < wait_at + assert names.index("Refuse a pull request from outside the organisation") < wait_at + assert wait_at < names.index("Check out the pull request head (read only)") assert wait_at < names.index("Borrow the Codex credential") + + +def test_the_wait_step_runs_the_tested_command_and_cannot_fail_the_job(): + """The step reads with the repository's own App token, forwards `force`, + and a failure of the command never skips the review.""" waiter = step("wait for the checks the policy lists") assert "reviewbot wait-for-checks" in waiter["run"] + assert waiter["run"].rstrip().endswith("|| true") + assert waiter["env"]["GITHUB_TOKEN"] == "${{ steps.app-token.outputs.token }}" assert waiter["env"]["REVIEWBOT_FORCE"] == "${{ inputs.force }}" From 04125661dc604c7bccf4c71015f2f6dc3854b389 Mon Sep 17 00:00:00 2001 From: MarketDataDev02 Date: Tue, 29 Sep 2026 20:35:53 -0300 Subject: [PATCH 4/6] docs: main says what it takes, returns and raises --- reviewbot/cli.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/reviewbot/cli.py b/reviewbot/cli.py index 2602e74..47af77a 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -970,7 +970,11 @@ def credential_checkin(store: str, holder: str, codex_home: str, token: str) -> def main(argv: list[str] | None = None) -> int: - """`reviewbot run --event [--pr N]`.""" + """Parse the command line and run one `reviewbot` subcommand. + + `argv` holds the arguments, `sys.argv[1:]` when None. Returns the process + exit code; raises SystemExit on a usage error. + """ parser = argparse.ArgumentParser(prog="reviewbot") sub = parser.add_subparsers(dest="command", required=True) runner = sub.add_parser("run", help="review one pull request") From 3bdea486051d94b6c20ebc8c120e1cbdfde0bfe3 Mon Sep 17 00:00:00 2001 From: MarketDataDev02 Date: Wed, 30 Sep 2026 12:15:36 -0300 Subject: [PATCH 5/6] fix: review only the head the wait polled A push during the wait could move the pull request to a head whose listed checks were never polled: the checkout fetched it, and `run` reviewed it as soon as its own CI was green, before Codecov reported. That head's own trigger then found it already reviewed and skipped (review finding 04a30a84). The wait now writes the commit it polled as the step output `head`, and `run` skips when the pull request has moved past it; the new head's own CI triggers the review that waits for it. Nothing is written when the policy asks for no wait, so those reviews run as before. The red conclusions the wait stops on are now the constant `ci_state` uses, so the two cannot drift (0c04032b). `cancelled` and `stale` stay out of it: `ci_state` does not count them as red, so the review still runs on them, and the wait keeps waiting for the checks that review will read. --- .github/workflows/review.yml | 8 ++++-- docs/setup.md | 6 ++-- reviewbot/cli.py | 51 ++++++++++++++++++++++++---------- reviewbot/github.py | 14 +++++++--- tests/test_cli.py | 52 +++++++++++++++++++++++++++++++++++ tests/test_wait_for_checks.py | 43 ++++++++++++++++++++++++++--- tests/test_workflow.py | 12 ++++++-- 7 files changed, 157 insertions(+), 29 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index a1c6eb1..b326470 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -261,10 +261,11 @@ jobs: --pr "${{ steps.pr.outputs.number }}" \ --repo "$GITHUB_REPOSITORY" - # Before the head checkout, so a push during the wait is still fetched. It - # holds this runner but never the Codex lease, and `|| true` keeps it from - # failing the job. + # It holds this runner but never the Codex lease, and `|| true` keeps it + # from failing the job. `head` is the commit it waited for: the review + # skips when the pull request has moved past it. - name: Wait for the checks the policy lists + id: wait if: steps.gate.outputs.trusted == 'true' env: GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} @@ -380,6 +381,7 @@ jobs: # allowed the same author. One job, two opposite answers. REVIEWBOT_ORG_TOKEN: ${{ steps.org-token.outputs.token }} REVIEWBOT_FORCE: ${{ inputs.force }} + REVIEWBOT_WAITED_HEAD: ${{ steps.wait.outputs.head }} run: | uv run --project bot reviewbot run \ --event "$GITHUB_EVENT_PATH" \ diff --git a/docs/setup.md b/docs/setup.md index 1a3dbb4..e1f1a9d 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -254,8 +254,10 @@ It ends once every listed check exists and no check run on the head is still running, from any app, so the earlier of two trigger workflows waits too: a repository that borrows the Codex lease still reviews once, because the later run finds the head reviewed, and any other may review twice. It ends early when -CI is already red. It holds the job's runner, which on the self-hosted runner -other repositories share, but never the Codex lease. +CI is already red, by the rule the review itself applies. If the pull request +moves to a new head during the wait, this run skips: the new head's own CI +triggers a review that waits for it. The wait holds the job's runner, which on +the self-hosted runner other repositories share, but never the Codex lease. A listed check that fails makes the review skip, like any red check: list `codecov/patch` only if patch coverage should hold back the review, and diff --git a/reviewbot/cli.py b/reviewbot/cli.py index 47af77a..9522871 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -20,7 +20,7 @@ from reviewbot import result as result_mod from reviewbot.backends import base as backends from reviewbot.config import JOB_BUDGET_MINUTES, JOB_OVERHEAD_MINUTES -from reviewbot.github import GitHub, GitHubError +from reviewbot.github import FAILED_CONCLUSIONS, GitHub, GitHubError from reviewbot.redact import scrub CONFIG_DIR = ".github/code-review" @@ -252,7 +252,6 @@ def _fail(api, sha: str, check_name: str, message: str) -> int: WAIT_POLL_SECONDS = 10.0 -RED_CONCLUSIONS = ("failure", "timed_out", "action_required") def wait_for_checks( @@ -267,7 +266,8 @@ def wait_for_checks( ) -> int: """Wait until every check the base-branch policy lists in `wait_for_checks` exists and no check run is still running, for at most - `wait_for_checks_seconds`. + `wait_for_checks_seconds`, and write the commit it polled as the step + output `head`. `repo`, `token` and `pr_number` name the pull request, `force` skips the wait, and `api`, `sleep` and `clock` replace the GitHub client, the sleep @@ -277,46 +277,51 @@ def wait_for_checks( if force: return 0 try: - _wait_for_listed_checks(api or GitHub(repo, token), pr_number, sleep, clock) + head = _wait_for_listed_checks(api or GitHub(repo, token), pr_number, sleep, clock) except (config.PolicyError, GitHubError) as exc: print(f"reviewbot wait: {scrub(str(exc))}; the review decides without waiting") + return 0 except Exception as exc: # noqa: BLE001 - the wait must never fail the review print( f"reviewbot wait: {type(exc).__name__}: {scrub(str(exc))}; " "the review decides without waiting" ) + return 0 + if head: + _say_output("head", head) return 0 -def _wait_for_listed_checks(api, pr_number: int, sleep, clock) -> None: +def _wait_for_listed_checks(api, pr_number: int, sleep, clock) -> str: """Poll the head commit's check runs until the listed checks and everything else have finished, CI is red, or the policy's seconds have passed. `api` is the GitHub client, `pr_number` the pull request, and `sleep` and - `clock` the sleep and the monotonic clock. Returns None; raises - `config.PolicyError` for a malformed policy, and `GitHubError` or the - transport's own error for a failed read. + `clock` the sleep and the monotonic clock. Returns the commit it polled, or + "" when the policy asks for no wait; raises `config.PolicyError` for a + malformed policy, and `GitHubError` or the transport's own error for a + failed read. """ pull = api.pull_request(pr_number) or {} base_ref = (pull.get("base") or {}).get("ref", "") policy = config.load(api.file_at_ref(POLICY_PATH, base_ref or "HEAD")) listed = policy["wait_for_checks"] if not listed or not policy["require_ci_green"]: - return + return "" sha = (pull.get("head") or {}).get("sha", "") seconds = policy["wait_for_checks_seconds"] deadline = clock() + seconds while True: runs = api.check_runs(sha, policy["check_name"]) - if any(run["conclusion"] in RED_CONCLUSIONS for run in runs): + if any(run["conclusion"] in FAILED_CONCLUSIONS for run in runs): print("reviewbot wait: CI is already red on the head commit; nothing to wait for") - return + return sha present = {run["name"] for run in runs} missing = [name for name in listed if name not in present] running = sorted({run["name"] for run in runs if run["status"] != "completed"}) if not missing and not running: print(f"reviewbot wait: {', '.join(listed)} finished") - return + return sha left = deadline - clock() if left <= 0: if running: @@ -329,7 +334,7 @@ def _wait_for_listed_checks(api, pr_number: int, sleep, clock) -> None: f"reviewbot wait: {', '.join(missing)} did not appear within {seconds}s; " "the review reads CI without them" ) - return + return sha sleep(min(WAIT_POLL_SECONDS, left)) @@ -343,8 +348,18 @@ def run( api=None, org_api=None, force: bool = False, + waited_head: str = "", ) -> int: - """One whole review. Returns the process exit code.""" + """Review one pull request and publish the result. + + `event` is the triggering payload, `repo` and `token` name the repository + and its App token, `checkout` is the read-only checkout of the head, and + `pr_number` names the pull request when the event does not. `api` and + `org_api` replace the repository and organisation clients, `force` reviews + an already reviewed or unfinished head, and `waited_head` is the commit + `wait-for-checks` polled, which the head must still be. Returns the process + exit code, 1 when the bot itself failed; an unexpected error propagates. + """ api = api or GitHub(repo, token) number = _resolve_pr(event, api, pr_number) if number is None: @@ -414,6 +429,13 @@ def run( f"step passes REVIEWBOT_ORG_TOKEN.", ) + if waited_head and pr.head_sha != waited_head: + print( + f"reviewbot: skipped, the head moved from {waited_head[:7]} to {pr.head_sha[:7]} " + "after the wait; its own CI triggers the review that waits for it" + ) + return 0 + skip = policy_mod.should_skip(pr, policy, force=force) if skip: print(f"reviewbot: skipped, {skip}") @@ -1169,6 +1191,7 @@ def main(argv: list[str] | None = None) -> int: pr_number=args.pr, org_api=GitHub(args.repo, org_token) if org_token else None, force=args.force, + waited_head=os.environ.get("REVIEWBOT_WAITED_HEAD", ""), ) except Exception: # the job must fail loudly, never silently traceback.print_exc() diff --git a/reviewbot/github.py b/reviewbot/github.py index 449742b..1384987 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -21,6 +21,9 @@ RETRY_STATUSES = (403, 429, 500, 502, 503, 504) MAX_ATTEMPTS = 5 +# The check-run conclusions that make CI red. +FAILED_CONCLUSIONS = ("failure", "timed_out", "action_required") + # GitHub's own words for "this account can act on the repository". MAINTAINER_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) @@ -443,7 +446,12 @@ def check_runs(self, sha: str, exclude_check_name: str) -> list[dict]: ] def ci_state(self, sha: str, exclude_check_name: str) -> str: - """`success`, `failure`, `pending` or `none` for everything but our own check.""" + """The CI state of a commit: `success`, `failure`, `pending` or `none`. + + `sha` is the commit and `exclude_check_name` the bot's own check, which + is left out. Returns the state; raises GitHubError, or the transport's + own error, when the check runs cannot be read. + """ runs = ( self._request("GET", f"/repos/{self.repo}/commits/{sha}/check-runs?per_page=100").data or {} @@ -470,9 +478,7 @@ def ci_state(self, sha: str, exclude_check_name: str) -> str: except GitHubError: legacy = None - failed = any( - r.get("conclusion") in ("failure", "timed_out", "action_required") for r in runs - ) + failed = any(r.get("conclusion") in FAILED_CONCLUSIONS for r in runs) running = any(r.get("status") != "completed" for r in runs) if failed or legacy == "failure": return "failure" diff --git a/tests/test_cli.py b/tests/test_cli.py index d772449..4a6c523 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -585,3 +585,55 @@ def test_the_ignore_paths_reach_the_diff_cap(live_claude): ) review(api) assert api.gather_ignore_paths == ["tests/fixtures/**"] + + +# --- the head the wait polled ----------------------------------------------- + + +def waited_review(api, waited_head): + """Run the review as the workflow does after `wait-for-checks` polled `waited_head`.""" + return cli.run( + event=EVENT, + repo="MarketData-App/api", + token="ghs_x", + checkout="/w/pr", + api=api, + waited_head=waited_head, + ) + + +def test_a_head_that_moved_after_the_wait_is_not_reviewed(capsys): + """The new head's listed checks were never waited for; its own CI triggers + the review that waits for them.""" + api = FakeGitHub(make_pr(head_sha="b" * 40)) + + assert waited_review(api, "a" * 40) == 0 + + assert api.comments == [] + assert api.checks == [] + assert "the head moved from aaaaaaa to bbbbbbb" in capsys.readouterr().out + + +def test_the_head_the_wait_polled_goes_on_to_the_usual_checks(capsys): + """An unmoved head is not skipped for the pin, only for what `should_skip` says.""" + api = FakeGitHub(make_pr(ci_state="pending")) + + assert waited_review(api, "a" * 40) == 0 + + out = capsys.readouterr().out + assert "the head moved" not in out + assert "CI has not finished on the head commit" in out + + +def test_the_command_forwards_the_head_the_wait_polled(monkeypatch): + """REVIEWBOT_WAITED_HEAD, which the workflow sets from the wait step, reaches the run.""" + seen = {} + monkeypatch.setenv("GITHUB_TOKEN", "t") + monkeypatch.setenv("REVIEWBOT_WAITED_HEAD", "abc") + for name in ("GITHUB_EVENT_PATH", "REVIEWBOT_ORG_TOKEN", "REVIEWBOT_FORCE"): + monkeypatch.delenv(name, raising=False) + monkeypatch.setattr(cli, "run", lambda **kwargs: seen.update(kwargs) or 0) + + assert cli.main(["run", "--checkout", "/w/pr", "--repo", "o/r", "--pr", "7"]) == 0 + + assert seen["waited_head"] == "abc" diff --git a/tests/test_wait_for_checks.py b/tests/test_wait_for_checks.py index 26bb0e5..fd6e582 100644 --- a/tests/test_wait_for_checks.py +++ b/tests/test_wait_for_checks.py @@ -10,6 +10,7 @@ import base64 +import pytest import requests from reviewbot import cli, github @@ -171,10 +172,13 @@ def test_an_unrelated_actions_workflow_still_running_keeps_the_wait_going(capsys assert "codecov/patch, codecov/project finished" in capsys.readouterr().out -def test_red_ci_ends_the_wait_at_once(capsys): - """A failed check makes the review skip whatever else reports, so there is - nothing to wait for.""" - api = FakeGitHub(LISTED, [[actions(conclusion="failure", name="Lint"), actions("in_progress")]]) +@pytest.mark.parametrize("conclusion", github.FAILED_CONCLUSIONS) +def test_red_ci_ends_the_wait_at_once(capsys, conclusion): + """A check the review counts as red makes it skip whatever else reports, so + there is nothing to wait for.""" + api = FakeGitHub( + LISTED, [[actions(conclusion=conclusion, name="Lint"), actions("in_progress")]] + ) clock = FakeClock() assert wait(api, clock) == 0 @@ -184,6 +188,37 @@ def test_red_ci_ends_the_wait_at_once(capsys): assert "CI is already red" in capsys.readouterr().out +def test_the_wait_names_the_head_it_polled(tmp_path, monkeypatch): + """The polled commit becomes the step output `head`, which pins the review.""" + output = tmp_path / "output" + monkeypatch.setenv("GITHUB_OUTPUT", str(output)) + api = FakeGitHub(LISTED, [[actions(), *BOTH]]) + + assert wait(api, FakeClock()) == 0 + + assert output.read_text() == "head=abc\n" + + +@pytest.mark.parametrize( + ("policy", "force", "reads"), + [ + ("", False, [[actions()]]), + (LISTED, True, [[actions()]]), + (LISTED + "require_ci_green: false\n", False, [[actions()]]), + ("wait_for_checks: [1]\n", False, [[actions()]]), + (LISTED, False, [GitHubError("GET check-runs: 502")]), + ], +) +def test_no_head_is_named_when_nothing_was_waited_for(tmp_path, monkeypatch, policy, force, reads): + """Without a wait there is nothing to pin, so the review runs as before.""" + output = tmp_path / "output" + monkeypatch.setenv("GITHUB_OUTPUT", str(output)) + + assert wait(FakeGitHub(policy, reads), FakeClock(), force=force) == 0 + + assert not output.exists() + + def test_it_gives_up_after_the_policy_seconds(capsys): """A listed check that never appears costs the whole wait and no more, then the review goes on without it.""" diff --git a/tests/test_workflow.py b/tests/test_workflow.py index 50a6e81..569cada 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -179,8 +179,8 @@ def test_every_step_after_the_gate_is_conditional_on_it(): def test_the_listed_checks_are_waited_for_before_the_head_is_fetched(): - """The wait runs after the gate, before the head is checked out, so a push - during the wait is fetched, and before the Codex lease is taken.""" + """The wait runs after the gate, and before the head is checked out and the + Codex lease is taken.""" names = [(s.get("name") or "") for s in steps()] wait_at = names.index("Wait for the checks the policy lists") assert names.index("Refuse a pull request from outside the organisation") < wait_at @@ -192,12 +192,20 @@ def test_the_wait_step_runs_the_tested_command_and_cannot_fail_the_job(): """The step reads with the repository's own App token, forwards `force`, and a failure of the command never skips the review.""" waiter = step("wait for the checks the policy lists") + assert waiter["id"] == "wait" assert "reviewbot wait-for-checks" in waiter["run"] assert waiter["run"].rstrip().endswith("|| true") assert waiter["env"]["GITHUB_TOKEN"] == "${{ steps.app-token.outputs.token }}" assert waiter["env"]["REVIEWBOT_FORCE"] == "${{ inputs.force }}" +def test_the_review_is_pinned_to_the_head_the_wait_polled(): + """The review gets the commit the wait polled, so a push during the wait + cannot be reviewed before its own checks were waited for.""" + env = step("run the review")["env"] + assert env["REVIEWBOT_WAITED_HEAD"] == "${{ steps.wait.outputs.head }}" + + def test_the_gate_step_runs_the_tested_command(): gate = step("refuse a pull request") assert "reviewbot gate" in gate["run"] From e6244c95fc585f3288467e544e2835b1507775ee Mon Sep 17 00:00:00 2001 From: MarketDataDev02 Date: Wed, 30 Sep 2026 12:29:43 -0300 Subject: [PATCH 6/6] fix: ci_state reads every page of check runs, as the wait does The wait paginated the check runs but `ci_state` read only the first 100, so on a commit with more runs a failure on a later page stopped the wait as red while `run` saw green and reviewed anyway (review finding 5da929df). Both now read the listing through one helper that follows every page. --- reviewbot/github.py | 32 ++++++++++++++++++-------------- tests/test_github.py | 25 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/reviewbot/github.py b/reviewbot/github.py index 1384987..b9eeea5 100644 --- a/reviewbot/github.py +++ b/reviewbot/github.py @@ -418,12 +418,12 @@ def check_results(self, sha: str, exclude_check_name: str) -> list[dict]: ) return out - def check_runs(self, sha: str, exclude_check_name: str) -> list[dict]: - """The name, status and conclusion of every check run on a commit. + def _every_check_run(self, sha: str, exclude_check_name: str) -> list[dict]: + """Every check run on a commit as GitHub returns it, across every page. `sha` is the commit and `exclude_check_name` the bot's own check, which - is left out. Every page of the listing is read. Raises GitHubError, or - the transport's own error, when a page cannot be read. + is left out. Returns the check-run objects; raises GitHubError, or the + transport's own error, when a page cannot be read. """ path = f"/repos/{self.repo}/commits/{sha}/check-runs?per_page=100" runs, page = [], 1 @@ -433,30 +433,34 @@ def check_runs(self, sha: str, exclude_check_name: str) -> list[dict]: found = found if isinstance(found, list) else [] runs.extend(run for run in found if isinstance(run, dict)) if len(found) < 100: - break + return [run for run in runs if run.get("name") != exclude_check_name] page += 1 + + def check_runs(self, sha: str, exclude_check_name: str) -> list[dict]: + """The name, status and conclusion of every check run on a commit. + + `sha` is the commit and `exclude_check_name` the bot's own check, which + is left out. Every page of the listing is read. Raises GitHubError, or + the transport's own error, when a page cannot be read. + """ return [ { "name": run.get("name") or "", "status": run.get("status") or "", "conclusion": run.get("conclusion") or "", } - for run in runs - if run.get("name") != exclude_check_name + for run in self._every_check_run(sha, exclude_check_name) ] def ci_state(self, sha: str, exclude_check_name: str) -> str: """The CI state of a commit: `success`, `failure`, `pending` or `none`. `sha` is the commit and `exclude_check_name` the bot's own check, which - is left out. Returns the state; raises GitHubError, or the transport's - own error, when the check runs cannot be read. + is left out. Every page of the check runs is read. Returns the state; + raises GitHubError, or the transport's own error, when the check runs + cannot be read. """ - runs = ( - self._request("GET", f"/repos/{self.repo}/commits/{sha}/check-runs?per_page=100").data - or {} - ).get("check_runs", []) - runs = [r for r in runs if r.get("name") != exclude_check_name] + runs = self._every_check_run(sha, exclude_check_name) # The combined-status endpoint is the older API, and reading it needs # the App's `statuses` permission, which we deliberately do not have. # It is an optional signal: Actions and most modern CI report as check diff --git a/tests/test_github.py b/tests/test_github.py index e136f8c..91bd8f7 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -176,6 +176,31 @@ def test_ci_state_is_success_when_everything_passed(api, transport): assert api.ci_state("abc", exclude_check_name="Code review") == "success" +def test_ci_state_sees_a_failure_on_a_later_page(api, transport): + """The review reads every page, as the wait does, so a failure past the + first hundred check runs still makes CI red.""" + passed = [ + {"name": f"test ({n})", "status": "completed", "conclusion": "success"} for n in range(100) + ] + transport.add( + "GET", + "/repos/MarketData-App/api/commits/abc/check-runs?per_page=100", + data={"total_count": 101, "check_runs": passed}, + ) + transport.add( + "GET", + "/repos/MarketData-App/api/commits/abc/check-runs?per_page=100&page=2", + data={ + "total_count": 101, + "check_runs": [ + {"name": "codecov/patch", "status": "completed", "conclusion": "failure"} + ], + }, + ) + transport.add("GET", "/repos/MarketData-App/api/commits/abc/status", data={"state": "success"}) + assert api.ci_state("abc", exclude_check_name="Code review") == "failure" + + def test_check_runs_give_the_status_and_conclusion_of_all_but_our_own(api, transport): """Each check run comes with its status and conclusion; ours is left out.""" transport.add(