nisse-review: a new head's review waits for its CI #155

Merged
dev-agent merged 4 commits from review-waits-for-ci into main 2026-10-17 04:47:09 +00:00
3 changed files with 68 additions and 3 deletions
Showing only changes of commit 637761ba5a - Show all commits

No files matched your search

+2 -1
View File
@@ -35,6 +35,7 @@ gitea:
# notes: ro
# tuning: # docs/Tuning-Guide.md
# review: {poll_s: 60, max_turns: 40, diff_chars: 60000, attempts: 2}
# review: {poll_s: 60, max_turns: 40, diff_chars: 60000, attempts: 2,
Outdated
Review

The Tuning Guide now documents tuning.review.ci_wait_s and tuning.review.ci_grace_s, but this example — the canonical "here are the review tuning keys" list — still stops at attempts. Worth adding them so the example and the guide line up (both parse fine via fields(); this is just the doc completeness).

The Tuning Guide now documents `tuning.review.ci_wait_s` and `tuning.review.ci_grace_s`, but this example — the canonical "here are the review tuning keys" list — still stops at `attempts`. Worth adding them so the example and the guide line up (both parse fine via `fields()`; this is just the doc completeness).
Review

Added in 637761b.

Added in 637761b.
# ci_wait_s: 3600, ci_grace_s: 120}
# expert: {consult_budget: 3}
# context: {keep_recent_turns: 6, safety_margin: 0.1}
+25 -2
View File
@@ -212,6 +212,12 @@ class Scheduler:
return False
return not (state == "none" and waited < self._ci_grace_s)
def _seen(self, known: PR, head: str) -> float:
"""When ``head`` was first seen: kept for the head the item knew, now
for one pushed during the review, so a retry still waits for CI. A
retry at a head already past ``ci_wait_s`` doesn't wait again."""
return known.seen_at if head == known.head else self._clock()
# -- a review ---------------------------------------------------------------
def review(self, item: WorkItem) -> None:
@@ -254,7 +260,16 @@ class Scheduler:
self._store.update_work_item(
item.id,
position=REVIEWED,
data=data(PR(known.repo, known.number, pr.head_sha, pr.head_sha, 0)),
data=data(
PR(
known.repo,
known.number,
Outdated
Review

This PR(...) (like the success one at line 257 and recover()'s at 310) omits seen_at, so it lands at the default 0.0. When the review failed but still has attempts left, the item goes back to READY with seen_at=0.0, and next round _ci_settled computes waited = now - 0.0 — huge — so it's "settled" and re-reviewed immediately, without re-waiting for CI.

That's the right call when CI already finished (the retry is at the same head), and the reviewer's brief re-fetches checks anyway so it still sees the current CI result. But it means the "stays off a GPU that CI is using" guarantee from the PR description leaks in one narrow case: if the first review ran via the "review anyway after ci_wait_s" escape while CI is still pending, the retry also skips the wait and runs beside CI again — exactly the #154 contention. Same for a push that lands mid-review and then the review fails (the new head is reviewed without its wait).

Probably fine/intended (a retry shouldn't restart a full hour of waiting), but it's the one place the wait is silently bypassed, so I'd like a deliberate nod — either a comment saying "a failed retry at a seen head doesn't re-wait" or carry known.seen_at through here.

This `PR(...)` (like the success one at line 257 and `recover()`'s at 310) omits `seen_at`, so it lands at the default `0.0`. When the review failed but still has attempts left, the item goes back to READY with `seen_at=0.0`, and next round `_ci_settled` computes `waited = now - 0.0` — huge — so it's "settled" and re-reviewed immediately, **without re-waiting for CI**. That's the right call when CI already finished (the retry is at the same head), and the reviewer's brief re-fetches `checks` anyway so it still sees the current CI result. But it means the "stays off a GPU that CI is using" guarantee from the PR description leaks in one narrow case: if the first review ran via the "review anyway after `ci_wait_s`" escape while CI is *still* pending, the retry also skips the wait and runs beside CI again — exactly the #154 contention. Same for a push that lands mid-review and then the review fails (the new head is reviewed without its wait). Probably fine/intended (a retry shouldn't restart a full hour of waiting), but it's the one place the wait is silently bypassed, so I'd like a deliberate nod — either a comment saying "a failed retry at a seen head doesn't re-wait" or carry `known.seen_at` through here.
Review

Good catch — fixed in 637761b. Posted, failed and recovered items now carry seen_at (Scheduler._seen), renewed when the head moved during the review. A retry at a head already past ci_wait_s still doesn't re-wait; that's the documented escape, so a stuck runner can't hold a review forever. test_a_failed_review_keeps_its_heads_wait fails without the fix. (The mid-review push case was already caught by the next round's intake noticing the moved head; there's a test for it now too.) The unreported-checks branch has a test as well: test_checks_gitea_cannot_report_wait_a_round.

Good catch — fixed in 637761b. Posted, failed and recovered items now carry `seen_at` (`Scheduler._seen`), renewed when the head moved during the review. A retry at a head already past `ci_wait_s` still doesn't re-wait; that's the documented escape, so a stuck runner can't hold a review forever. `test_a_failed_review_keeps_its_heads_wait` fails without the fix. (The mid-review push case was already caught by the next round's intake noticing the moved head; there's a test for it now too.) The unreported-checks branch has a test as well: `test_checks_gitea_cannot_report_wait_a_round`.
pr.head_sha,
pr.head_sha,
0,
self._seen(known, pr.head_sha),
)
),
cause="review posted",
)
@@ -263,7 +278,14 @@ class Scheduler:
) -> None:
attempts = known.attempts + 1
head = pr.head_sha if pr else known.head
new = PR(known.repo, known.number, head, known.reviewed, attempts)
new = PR(
known.repo,
known.number,
head,
known.reviewed,
attempts,
self._seen(known, head),
)
self._oplog.emit("review.failed", cause=why[:300], work_item=item.id)
if attempts < self._attempts:
self._store.update_work_item(
@@ -313,6 +335,7 @@ class Scheduler:
head,
head if posted else known.reviewed,
known.attempts,
self._seen(known, head),
)
self._store.update_work_item(
item.id,
+41
View File
@@ -394,6 +394,47 @@ def test_a_pr_waiting_on_ci_holds_up_no_other(tmp_path: Path) -> None:
assert (item(w, 7).position, item(w, 8).position) == (READY, REVIEWED)
def test_a_failed_review_keeps_its_heads_wait(tmp_path: Path) -> None:
# nisse-review on #155: a retry lost seen_at and skipped the wait.
clock = Clock()
gitea = FakeGitea(repos={REPO: [pr()]})
prose = [AssistantText(text="I think it's fine.", model="m")]
w = world(tmp_path, Scripted(prose, prose), gitea, clock=clock, attempts=2)
w["scheduler"].round()
assert (item(w).position, pr_of(item(w)).seen_at) == (READY, 1000.0)
def test_a_head_pushed_during_a_failed_review_waits_for_its_ci(
tmp_path: Path,
) -> None:
clock = Clock()
class Pushed(FakeGitea):
def post_review(self, *args: Any, **kwargs: Any) -> int:
clock.now += 50
self.repos[REPO] = [pr(head_sha="fff000")]
self.ci["fff000"] = "pending"
raise GiteaError("422: the head moved")
gitea = Pushed(repos={REPO: [pr()]})
w = world(tmp_path, Scripted(*REVIEW), gitea, clock=clock)
w["scheduler"].round()
assert w["scheduler"].round() is False # the new head's CI is pending
def test_checks_gitea_cannot_report_wait_a_round(tmp_path: Path) -> None:
class NoChecks(FakeGitea):
def checks(self, repo: str, sha: str) -> str:
raise GiteaError("502: bad gateway")
out = io.StringIO()
w = world(tmp_path, Scripted(*REVIEW), NoChecks(repos={REPO: [pr()]}))
w["scheduler"]._oplog = OpLog("t", out)
assert w["scheduler"].round() is False
assert item(w).position == READY
assert '"review.checks_unknown"' in out.getvalue()
def test_a_configured_repository_it_cannot_read_is_left_out() -> None:
gitea = FakeGitea(repos={REPO: []})
out = io.StringIO()