diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 359a02a..b326470 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -261,6 +261,21 @@ jobs: --pr "${{ steps.pr.outputs.number }}" \ --repo "$GITHUB_REPOSITORY" + # 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 }} + 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 @@ -366,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 b3657f5..e1f1a9d 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 @@ -200,7 +201,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 +235,36 @@ 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 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 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, 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 +`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 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..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" @@ -251,6 +251,93 @@ def _fail(api, sha: str, check_name: str, message: str) -> int: return 1 +WAIT_POLL_SECONDS = 10.0 + + +def wait_for_checks( + *, + repo: str, + token: str, + pr_number: int, + force: bool = False, + api=None, + sleep=_time.sleep, + clock=_time.monotonic, +) -> 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`, 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 + 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 + try: + 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) -> 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 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 "" + 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 FAILED_CONCLUSIONS for run in runs): + print("reviewbot wait: CI is already red on the head commit; nothing to wait for") + 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 sha + 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 sha + sleep(min(WAIT_POLL_SECONDS, left)) + + def run( *, event: dict, @@ -261,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: @@ -332,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}") @@ -572,36 +676,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. - - 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. + """The Codex lease's TTL and how long to wait for it, both in minutes. - 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)) @@ -846,27 +937,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, @@ -907,7 +992,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") @@ -947,6 +1036,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 +1152,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: @@ -1082,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/config.py b/reviewbot/config.py index 048d983..0fbda06 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): @@ -132,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(): @@ -175,12 +184,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): @@ -188,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 4060731..09cce49 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 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: # 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..b9eeea5 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"}) @@ -415,13 +418,49 @@ def check_results(self, sha: str, exclude_check_name: str) -> list[dict]: ) return out + 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. 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 + 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: + 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 self._every_check_run(sha, 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 = ( - 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] + """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. 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._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 @@ -443,9 +482,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_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_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_config.py b/tests/test_config.py index 9c5fd92..a779c43 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -138,3 +138,45 @@ 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") + + +@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 adefb52..91bd8f7 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -176,6 +176,68 @@ 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( + "GET", + "/repos/MarketData-App/api/commits/abc/check-runs?per_page=100", + data={ + "check_runs": [ + {"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", "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 75c4890..48ce4f5 100644 --- a/tests/test_lease_report.py +++ b/tests/test_lease_report.py @@ -234,6 +234,7 @@ 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 the one ten-minute hold in the week before `now`.""" from reviewbot import cli history = [ @@ -243,12 +244,14 @@ def test_the_command_prints_a_report_for_the_store(monkeypatch, capsys): 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 new file mode 100644 index 0000000..fd6e582 --- /dev/null +++ b/tests/test_wait_for_checks.py @@ -0,0 +1,385 @@ +"""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. The last tests use the real client over a fake transport, to pin +the requests themselves. + +Run: pytest tests/test_wait_for_checks.py +""" + +import base64 + +import pytest +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="completed", conclusion="success", name="Tests"): + """A GitHub Actions check run in the given status.""" + 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, + "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; records what each read asked for.""" + + def __init__(self, policy, reads): + """Keep the policy text and the reads, with no request recorded yet.""" + self.policy = policy + self.reads = list(reads) + 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 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, 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.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 + return read + + +class FakeClock: + """A monotonic clock that moves forward only when the code sleeps.""" + + START = 1000.0 + + def __init__(self): + """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) + 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, + reading the base branch's policy and the head commit's check runs.""" + api = FakeGitHub( + LISTED, + [ + [actions()], + [actions(), codecov("codecov/project", "in_progress")], + [actions(), *BOTH], + ], + ) + clock = FakeClock() + + assert wait(api, clock) == 0 + + 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 + + +@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 + + assert api.seen == 1 + assert clock.slept == [] + 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.""" + api = FakeGitHub(LISTED, [[actions(), codecov("codecov/project")]]) + clock = FakeClock() + + assert wait(api, clock) == 0 + + 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 no check run is read.""" + api = FakeGitHub("", [[actions()]]) + + 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()]]) + + 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 wait is skipped.""" + api = FakeGitHub(LISTED + "require_ci_green: false\n", [[actions()]]) + + 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()]]) + + 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 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.""" + 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} + + +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 9184118..569cada 100644 --- a/tests/test_workflow.py +++ b/tests/test_workflow.py @@ -178,6 +178,34 @@ 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_head_is_fetched(): + """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 + 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 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"]