[MarketDataDev02]

Closes #24

The problem

A review is triggered when the Tests or Lint workflow finishes, and should_skip skips it while any check run on the head commit is not completed ("CI has not finished on the head commit"). Codecov, with its default wait_for_ci, posts codecov/patch and codecov/project only after every other check has finished: it creates and completes them within one or two seconds, 35 to 50 seconds after Tests. Nothing triggers a review when they finish.

Measured on MarketDataApp/sdk-py #159, comparing when the Tests-triggered run started reviewbot run with when Codecov's checks completed:

head	run	margin	outcome
7759248	36462367405	-24 s	reviewed, Codecov not yet reported
9f65a39	36445268644	-15 s	reviewed, Codecov not yet reported
b8035b2	36436007907	-3 s	reviewed, Codecov not yet reported
bbeea3c	36452710753	+2 s	reviewed
7f929dc	36460028984	+8 s	reviewed
21efa02	36465597062	-5 s	reviewbot: skipped, CI has not finished on the head commit

So a review usually reads CI before Codecov exists, and require_ci_green never sees coverage; and when it reads during the second Codecov is in progress (21efa02: checks created 18:30:28Z, completed 18:30:30Z, read about 18:30:26Z), it skips and nothing brings it back. That head was reviewed only after a manual @marketdata-code-review re-review. Codecov reports on sdk-py, sdk-csharp and sdk-php.

The change

• wait_for_checks (list of check-run names, default []) and wait_for_checks_seconds (default 300, from 0 to 300) in the policy, validated in config.py. A policy may not list its own check_name, which the wait never sees.
• GitHub.check_runs(sha, exclude_check_name): the name, status and conclusion of every check run but the bot's own, across every page of the listing. ci_state reads the same pages through the same helper, so the wait and the review judge the same runs.
• reviewbot wait-for-checks --pr N: reads the policy from the base branch; does nothing when forced, when require_ci_green is off or when nothing is listed; otherwise polls every 10 seconds, the last sleep ending at the deadline, until every listed check exists and no check run on the head is still running, from any app. It stops at once when CI is already red, by the same FAILED_CONCLUSIONS that ci_state uses (failure, timed_out, action_required), since run skips that anyway. cancelled and stale are not red for ci_state, so the review still runs on them and the wait keeps waiting for the checks it will read. At the deadline it logs whether the review reads CI without a check that never appeared, or skips because one is still running. Any error, a raw transport error included, is logged and the command exits 0.
• The wait writes the commit it polled as the step output head, and reviewbot run gets it as REVIEWBOT_WAITED_HEAD: when the pull request has moved past that commit, the run skips, and the new head's own CI triggers the review that waits for its checks. Nothing is written when the policy asks for no wait, so those reviews run as before.
• review.yml: a "Wait for the checks the policy lists" step (id: wait) after the gate and before the pull request checkout and "Borrow the Codex credential", so the wait holds the runner but never the shared lease. It ends in || true, so it cannot skip the review.
• docs/setup.md: the fourth choice under "Add a policy", including that the wait needs require_ci_green, that a failing listed check skips the review, and that the names are check-run names.

A repository that does not set the key behaves as before: the step reads the pull request and the policy, and returns.

With two trigger workflows (sdk-py's Tests and Lint) both runs wait. The repositories that list Codecov borrow the Codex lease, so the second run then finds the head reviewed and skips; a repository without the lease may review twice.

Budget

The wait is at most 300 seconds and happens before the lease, so it adds to the job's wall time, not to the lease. _lease_minutes subtracts it from the lease wait's room when checks are listed and require_ci_green is on, so overhead, the wait, the lease wait and both review attempts fit the 90-minute job for every timeout_minutes from 1 to 30.

Rollout order

Merge this first. Then each repository adds, on its default branch:

wait_for_checks: ["codecov/patch", "codecov/project"]

The policy loader rejects unknown keys, so a repository that sets it before this is merged fails every review with "policy.yml is malformed". Once a repository sets it, a red Codecov check makes the review skip with "CI is red on the head commit", as require_ci_green intends; list only codecov/project to gate on the project status alone.

Tests

• tests/test_wait_for_checks.py (24): waits until both Codecov checks exist and finish, reading the base branch's policy and the head's check runs; a listed check still running, and an unrelated Actions workflow still running, keep the wait going; each of the three red conclusions ends it at once; it gives up after exactly the cap, the last sleep ending at the deadline, and says whether a check never appeared or is still running; it names the polled head, and names none when forced, unlisted, not gating on CI, malformed or failing; no listed check, a forced review, or require_ci_green: false read nothing; a malformed policy, an API error and a dropped connection leave the decision to the review; the real client over a fake transport requests the base policy and the head's check runs; the CLI passes the pull request, repository, token and force.
• tests/test_cli.py (3): a head that moved after the wait is not reviewed; an unmoved head goes on to should_skip; the command forwards REVIEWBOT_WAITED_HEAD.
• tests/test_config.py (7), tests/test_github.py (3), tests/test_workflow.py (3), tests/test_checkout_command.py (32): the defaults, a repository's list, the cap, the names and the own-check refusal; the status and conclusion per check run, every page read, and a failure on a later page making ci_state red; the step order, id, token, force, || true and the pinned head reaching the review; the lease wait leaving room for the wait at every timeout_minutes, and keeping its whole room when nothing waits.

uv run pytest: 755 passed on Windows. The other 54 fail on an unmodified checkout of main on Windows too, because their fake claude and codex executables are POSIX scripts; ubuntu CI runs them. ruff check . and ruff format --check . are clean.

Also: a test that expired

tests/test_lease_report.py::test_the_command_prints_a_report_for_the_store failed this pull request's first CI run and fails on main as well: it dated its only hold 2026-09-21T10:00Z, while lease_report read its seven-day window from the clock, so from 2026-09-28T10:00Z the hold fell outside the window. lease_report now takes now, and the test passes a fixed one with its fixed dates.

[marketdata-code-review]

🟡 Needs changes — Code review

This change adds policy-driven polling for delayed check runs, pins the subsequent review to the polled head, and accounts for that wait in the credential-lease budget. The polling deadline does not actually bound elapsed runtime because another API read starts after the final sleep reaches the deadline.

Rating patch 4/6 (works with reservations) · proof 5/6 (reproducible) · overall 4/6

Findings

Should fix

• Wait can exceed its configured deadline (correctness, 6f0f217d)
  The loop checks the deadline only after fetching every page of check runs, so the final sleep reaches the deadline and then starts another potentially slow or retried API request. This breaks the documented maximum and can invalidate the job-budget calculation. Check the clock before polling again after a sleep, or otherwise include request time in the enforced timeout.

  reviewbot/cli.py: runs = api.check_runs(sha, policy["check_name"]) occurs before left = deadline - clock() on every loop iteration.

Since last review

• Resolved: 5da929df
• 0 still open, 1 new

---

Reviewed e6244c9 · revision 5 · codex (gpt-5.6-sol) · claude unavailable · rules: default