diff --git a/docs/setup.md b/docs/setup.md index e1f1a9d..8c4f37b 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -235,21 +235,26 @@ 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 +**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: +So by default the review waits for `codecov/patch` and `codecov/project` +before it reads CI. Codecov reports on sdk-py, sdk-csharp, sdk-php, sdk-go and +sdk-java; a repository whose CI does not report to it, like sdk-js, turns the +wait off, or names its own late checks by their check-run names, since the +list replaces the default: ```yaml -wait_for_checks: ["codecov/patch", "codecov/project"] -wait_for_checks_seconds: 300 # the longest it waits, 0 to 300 +wait_for_checks: [] # this repository does not report to Codecov +wait_for_checks_seconds: 120 # the longest it waits, 0 to 300 ``` -The wait runs only while `require_ci_green` is on, and not for a forced review. +The wait runs only while `require_ci_green` is on, and not for a forced review +or a pull request whose changed files all match `ignore_paths`, which the +review skips anyway. 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 @@ -259,11 +264,14 @@ 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. +A check that fails makes the review skip, like any red check, listed or not, +because the review reads every check: with the wait, a failing `codecov/patch` +reliably skips it. To keep coverage from holding a review back, make that +status informational in the repository's Codecov configuration +(`informational: true`). A listed check that never appears costs the whole +wait, and then the review reads CI without it: that is a pull request whose +coverage never reached Codecov, or every review in a repository that does not +report to it and has not turned the wait off. ### 5. Leave the check advisory until you trust it diff --git a/reviewbot/cli.py b/reviewbot/cli.py index 9522871..83c006a 100644 --- a/reviewbot/cli.py +++ b/reviewbot/cli.py @@ -266,8 +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`, and write the commit it polled as the step - output `head`. + `wait_for_checks_seconds` and not at all when every changed file matches + `ignore_paths`, 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 @@ -298,9 +298,9 @@ def _wait_for_listed_checks(api, pr_number: int, sleep, clock) -> str: `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. + "" when the policy asks for no wait or every changed file matches + `ignore_paths`; 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", "") @@ -308,6 +308,10 @@ def _wait_for_listed_checks(api, pr_number: int, sleep, clock) -> str: listed = policy["wait_for_checks"] if not listed or not policy["require_ci_green"]: return "" + paths = [item["path"] for item in api.changed_files(pr_number)] + if facts.all_match(paths, policy["ignore_paths"]): + print("reviewbot wait: every changed file matches ignore_paths; nothing to wait for") + return "" sha = (pull.get("head") or {}).get("sha", "") seconds = policy["wait_for_checks_seconds"] deadline = clock() + seconds diff --git a/reviewbot/defaults/policy.yml b/reviewbot/defaults/policy.yml index 09cce49..adf5211 100644 --- a/reviewbot/defaults/policy.yml +++ b/reviewbot/defaults/policy.yml @@ -18,12 +18,13 @@ 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 +# Checks another app reports after CI, by check-run name. 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, unless every changed file matches ignore_paths. A +# listed check that fails skips the review, like any red check. A repository +# whose CI does not report to Codecov sets `wait_for_checks: []`. +wait_for_checks: ["codecov/patch", "codecov/project"] +wait_for_checks_seconds: 120 # 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/tests/test_checkout_command.py b/tests/test_checkout_command.py index bab27b5..b0e605a 100644 --- a/tests/test_checkout_command.py +++ b/tests/test_checkout_command.py @@ -775,7 +775,7 @@ def test_the_lease_wait_leaves_room_for_the_wait_for_listed_checks(cap): @pytest.mark.parametrize( "text", - ["", 'wait_for_checks: ["codecov/patch"]\nrequire_ci_green: false\n'], + ["wait_for_checks: []\n", '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.""" diff --git a/tests/test_config.py b/tests/test_config.py index a779c43..b9128e2 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -140,11 +140,16 @@ def test_the_association_list_may_not_be_empty(): config.load("trusted_associations: []\n") -def test_no_check_is_waited_for_by_default(): - """A repository that lists nothing waits for nothing.""" +def test_codecov_is_waited_for_by_default(): + """A repository that sets nothing waits up to two minutes for Codecov.""" policy = config.defaults() - assert policy["wait_for_checks"] == [] - assert policy["wait_for_checks_seconds"] == 300 + assert policy["wait_for_checks"] == ["codecov/patch", "codecov/project"] + assert policy["wait_for_checks_seconds"] == 120 + + +def test_a_repository_can_turn_the_wait_off(): + """An empty list replaces the default, for a repository without Codecov.""" + assert config.load("wait_for_checks: []\n")["wait_for_checks"] == [] def test_a_repository_lists_the_checks_to_wait_for(): diff --git a/tests/test_wait_for_checks.py b/tests/test_wait_for_checks.py index fd6e582..9c5e9e1 100644 --- a/tests/test_wait_for_checks.py +++ b/tests/test_wait_for_checks.py @@ -45,10 +45,12 @@ 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.""" + def __init__(self, policy, reads, files=("src/client.py",)): + """Keep the policy text, the reads and the changed files, with no + request recorded yet.""" self.policy = policy self.reads = list(reads) + self.files = list(files) self.policy_reads = [] self.check_reads = [] @@ -66,6 +68,10 @@ def file_at_ref(self, path, ref): self.policy_reads.append((path, ref)) return self.policy + def changed_files(self, number): + """The pull request's changed files.""" + return [{"path": path, "status": "modified"} for path in self.files] + 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)) @@ -202,7 +208,7 @@ def test_the_wait_names_the_head_it_polled(tmp_path, monkeypatch): @pytest.mark.parametrize( ("policy", "force", "reads"), [ - ("", False, [[actions()]]), + ("wait_for_checks: []\n", False, [[actions()]]), (LISTED, True, [[actions()]]), (LISTED + "require_ci_green: false\n", False, [[actions()]]), ("wait_for_checks: [1]\n", False, [[actions()]]), @@ -260,12 +266,58 @@ def test_a_check_still_running_at_the_deadline_is_reported_as_unfinished(capsys) def test_nothing_is_read_when_no_check_is_listed(): - """The default policy lists no check, so no check run is read.""" + """A repository that turns the wait off has no check run read.""" + api = FakeGitHub("wait_for_checks: []\n", [[actions()]]) + + assert wait(api, FakeClock()) == 0 + + assert api.seen == 0 + + +def test_codecov_is_waited_for_by_default(capsys): + """A repository that sets nothing waits for both Codecov checks.""" + api = FakeGitHub("", [[actions()], [actions(), *BOTH]]) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert clock.slept == [cli.WAIT_POLL_SECONDS] + assert "codecov/patch, codecov/project finished" in capsys.readouterr().out + + +def test_the_default_wait_gives_up_after_two_minutes(capsys): + """Where Codecov never reports, the default costs two minutes at most.""" api = FakeGitHub("", [[actions()]]) + clock = FakeClock() + + assert wait(api, clock) == 0 + + assert clock.waited == 120 + assert "did not appear within 120s" in capsys.readouterr().out + + +def test_a_pull_request_whose_files_are_all_ignored_does_not_wait(tmp_path, monkeypatch, capsys): + """The review skips such a pull request anyway, so nothing is waited for or pinned.""" + output = tmp_path / "output" + monkeypatch.setenv("GITHUB_OUTPUT", str(output)) + policy = LISTED + 'ignore_paths: ["docs/**", "*.md"]\n' + api = FakeGitHub(policy, [[actions()]], files=["docs/setup.md", "README.md"]) assert wait(api, FakeClock()) == 0 assert api.seen == 0 + assert not output.exists() + assert "every changed file matches ignore_paths" in capsys.readouterr().out + + +def test_one_file_outside_ignore_paths_keeps_the_wait(): + """A pull request the review will read still waits for the listed checks.""" + policy = LISTED + 'ignore_paths: ["docs/**", "*.md"]\n' + api = FakeGitHub(policy, [[actions(), *BOTH]], files=["docs/setup.md", "src/client.py"]) + + assert wait(api, FakeClock()) == 0 + + assert api.seen == 1 def test_a_forced_review_does_not_wait(): @@ -342,6 +394,11 @@ def test_the_real_client_reads_the_base_policy_and_the_head_check_runs(capsys): "/repos/o/r/contents/.github/code-review/policy.yml?ref=release/2.0", data=policy_content(LISTED), ) + transport.add( + "GET", + "/repos/o/r/pulls/1/files?per_page=100&page=1", + data=[{"filename": "src/client.py", "status": "modified"}], + ) transport.add( "GET", "/repos/o/r/commits/abc/check-runs?per_page=100", @@ -355,6 +412,7 @@ def test_the_real_client_reads_the_base_policy_and_the_head_check_runs(capsys): 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/pulls/1/files?per_page=100&page=1", "/repos/o/r/commits/abc/check-runs?per_page=100", ]