88 lines
4.3 KiB
Markdown
88 lines
4.3 KiB
Markdown
# PR#119: The Codex Runner Reviews the Longest-Waiting PR First
|
|
|
|
Version: minor
|
|
|
|
> **WARNING:** This document describes only the change applied in this PR.
|
|
> It may already be outdated once the next PR is merged.
|
|
> Historic PR documentation is not maintained along with new PRs; treat it as a snapshot.
|
|
|
|
## Related Links
|
|
|
|
- [PR#88](2026-10-11-PR#88-codex-runner.md): the Codex runner and `max_reviews_per_run`.
|
|
|
|
## Problem
|
|
|
|
The Codex runner reviews the ready PRs in the order of the configuration and of `list_open_prs()`:
|
|
server, then repository, then PR.
|
|
`max_reviews_per_run` bounds the reviews of a run.
|
|
A PR with frequent pushes thus gets a review in each run, while a ready PR behind it waits.
|
|
|
|
On 2026-10-17, werkrevisor PR#118 got Codex reviews of new heads at 14:00, 14:06, 14:15 and 14:21.
|
|
werkrevisor PR#117 got its werkrevisor result for `c175f10` at 14:15:36; its Codex review started only at 14:21:11.
|
|
|
|
## Non-Goals
|
|
|
|
- No parallel reviews; the reviews run one after another, as before.
|
|
- No change of the systemd timer.
|
|
|
|
## Specification
|
|
|
|
### Feature: the Codex runner serves the ready PRs by their wait time
|
|
|
|
#### Scenario#119.01: the PR that waits longest gets the review
|
|
|
|
… so that a PR with frequent pushes cannot delay the other PRs.
|
|
|
|
- **Given** two repositories with one ready PR each, where the PR of the second repository has the older werkrevisor result,
|
|
and `max_reviews_per_run = 1`
|
|
- **When** the runner runs
|
|
- **Then** the PR of the second repository gets the Codex review, the other one keeps the status `pending`
|
|
|
|
##### Verified by
|
|
|
|
- [test_codexrunner.CodexRunnerTest.test_the_pr_that_waits_longest_goes_first](../../tests/test_codexrunner.py)
|
|
|
|
#### Scenario#119.02: a PR whose review failed keeps its place
|
|
|
|
… so that a failed attempt does not move a PR behind the PRs that became ready after it.
|
|
|
|
- **Given** a ready PR whose last Codex attempt failed, and a ready PR with a newer werkrevisor result,
|
|
and `max_reviews_per_run = 1`
|
|
- **When** the runner runs
|
|
- **Then** the PR with the failed attempt gets the next attempt
|
|
|
|
##### Verified by
|
|
|
|
- [test_codexrunner.CodexRunnerTest.test_a_failed_pr_keeps_its_place](../../tests/test_codexrunner.py)
|
|
|
|
## Solution
|
|
|
|
- `Runner.run` first collects the ready PRs of all servers and repositories, then reviews them oldest first.
|
|
A ready PR is not WIP, has a werkrevisor result for its head that is not RED, and has no Codex comment for its head.
|
|
- The wait time starts at `created_at` of the werkrevisor result comment for the head.
|
|
The runner compares the times as instants, so offsets like `+02:00` and `Z` sort correctly.
|
|
A time it cannot read puts the PR behind all others.
|
|
- `max_reviews_per_run` applies to the ordered list;
|
|
each PR beyond it keeps `pending` "waiting for the Codex review" in gate mode.
|
|
- A PR with a Codex comment for its head gets its gate status at once, while the runner collects; it uses no quota, as before.
|
|
- A failure in one review is logged, and the next PR in the order is still served.
|
|
- A failure with one PR while the runner collects is logged; the other PRs of its repository stay candidates.
|
|
Before this fix, one failing PR discarded the ready PRs that the runner had collected before it;
|
|
the Codex review of the first head of this PR, `83e913a`, found this in a line comment of the PR.
|
|
- The runner takes a factory for its Gitea clients, so the tests run it over several repositories.
|
|
- README "Codex runner" and the plan name the order.
|
|
- Version 0.48.0; PR#120 took 0.47.1 before.
|
|
|
|
## Open Questions
|
|
|
|
- [x] Does a PR with failed attempts keep its place by wait time?
|
|
Yes: after `MAX_ATTEMPTS` (3) failed attempts the runner posts the error comment, and the PR is no candidate any more.
|
|
So a failing PR takes at most 3 reviews from the others, and its retry is not delayed behind newer PRs.
|
|
- [x] Does the runner fetch a PR again before its review?
|
|
No, as before: it reviews the head it saw when it collected the PRs.
|
|
A push during the run gets its review in a later run, after werkrevisor reviewed the new head.
|
|
- [x] Is there a mutation-test gate?
|
|
No: the required mutation tests apply to Kotlin and Java; `codexrunner.py` is Python.
|
|
The new tests were checked against two wrong orders: without sorting, three tests fail;
|
|
with the times sorted as text, the test with the offset `+02:00` fails.
|