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

2026-10-16T23:08:47Z

dev-agent commented

2026-10-16 23:08:47 +00:00

The follow-up agreed on #154. A review judges the CI result (R1), so it now waits for a new head's checks to finish. That also keeps it off the 4090 while CI uses it: #154's first review ran beside CI's end-to-end job and took about 30 minutes.

Behaviour

Checks pending: wait, up to tuning.review.ci_wait_s (default 1 h), then review anyway. A stuck runner blocks nothing.
Checks finished, passed or failed: review. A failing head is worth reviewing too.
No checks at all: wait tuning.review.ci_grace_s (default 2 min) for them to appear, then review. Repositories without CI barely wait.
One PR waiting holds up no other: the oldest PR whose CI has settled goes first.
State: the head's first sighting is kept on the item (seen_at). Older items without it don't wait. review.waiting_for_ci is logged once per head.

Tests: pending, then a failure ends the wait; pending past the limit; no CI and the grace period; a waiting PR doesn't hold up a settled one. All four fail if the wait is removed.

Docs: R1 in the architecture, and both settings in the Tuning Guide.

nisse-review should review this PR itself, after its CI finishes. That's a live check of the wait, because the running bring-up is on the old code: it will review at once, beside CI. That's the before; the next PR shows the after.

🤖 Generated with Claude Code

https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

The follow-up agreed on #154. A review judges the CI result (R1), so it now waits for a new head's checks to finish. That also keeps it off the 4090 while CI uses it: #154's first review ran beside CI's end-to-end job and took about 30 minutes. **Behaviour** - **Checks pending:** wait, up to `tuning.review.ci_wait_s` (default 1 h), then review anyway. A stuck runner blocks nothing. - **Checks finished, passed or failed:** review. A failing head is worth reviewing too. - **No checks at all:** wait `tuning.review.ci_grace_s` (default 2 min) for them to appear, then review. Repositories without CI barely wait. - **One PR waiting holds up no other:** the oldest PR whose CI has settled goes first. - **State:** the head's first sighting is kept on the item (`seen_at`). Older items without it don't wait. `review.waiting_for_ci` is logged once per head. **Tests:** pending, then a failure ends the wait; pending past the limit; no CI and the grace period; a waiting PR doesn't hold up a settled one. All four fail if the wait is removed. **Docs:** R1 in the architecture, and both settings in the Tuning Guide. nisse-review should review this PR itself, after its CI finishes. That's a live check of the wait, because the running bring-up is on the old code: it will review at once, beside CI. That's the before; the next PR shows the after. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dev-agent added 1 commit 2026-10-16 23:08:47 +00:00

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

CI / checks (push) Skipped

CI / checks (pull_request) Successful in 1m59s

Details

CI / e2e (pull_request) Failing after 49m46s

Details

be5ea5f170

A review judges the CI result (R1), so a new head's review now waits while
its checks are pending, and starts once they finish, passed or failed. It
also keeps reviews off a GPU that CI is using: #154's first review ran
beside CI's end-to-end job and took about 30 minutes.

- Pending: waits up to tuning.review.ci_wait_s (1 h), then reviews anyway.
- No checks: waits tuning.review.ci_grace_s (2 min) for them to appear,
  then reviews.
- A PR waiting on CI holds up no other: the oldest settled one goes first.
- The head's first sighting is kept on the item (seen_at).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dongar approved these changes

2026-10-16 23:12:53 +00:00

Dismissed

dev-agent added 1 commit 2026-10-17 00:02:19 +00:00

Live review test: no CI grace in its CI-less repository

CI / checks (push) Skipped

CI / checks (pull_request) Successful in 2m33s

Details

CI / e2e (pull_request) Failing after 54m48s

Details

096eeea56b

The CI wait made a head with no checks wait ci_grace_s (2 min), so the
live test's single round reviewed nothing. Its fresh repository has no
CI: the test sets the grace to 0.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dev-agent dismissed dongar's review 2026-10-17 00:02:19 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

nisse-review reviewed

2026-10-17 00:59:01 +00:00

nisse-review left a comment

Clean, well-scoped follow-up to #154. The wait logic in _next/_ci_settled is correct across all four documented behaviours (I traced every branch: pending, pending-past-limit, no-CI-grace, finished-passed-or-failed), the state machine reads well, and it fits R1. The new seen_at on the item is the right place to keep a head's first sighting, and pre-existing items (no seen_at) fall back to 0.0 → reviewed immediately, exactly the "older items don't wait" requirement. The two tuning keys are genuinely wired: parse_tuning builds the tuning dataclass via fields(), so they're recognized, validated as positive floats, and reachable from config — matching the Tuning Guide. The world() helper already forwards **more to Scheduler, so the tests' clock=/ci_wait_s=/ci_grace_s= work without a helper change.

The four tests are discriminating — I verified each fails if the wait is removed, including "holds up no other," which correctly keys on opened_at (deterministic: store orders by opened_at, sorted is stable).

Two things I flagged on the lines:

seen_at is dropped in the failure/recovery PR(...) constructions (scheduler.py:266 and :310), so a failed-then-retried item (or a crash-recovered one) that goes back to READY has seen_at=0.0 and is re-reviewed immediately, skipping the CI wait. That's the right call when CI already finished (and the brief re-fetches checks anyway, so the reviewer still sees current CI), but it leaks the "stays off a GPU while CI uses it" guarantee in the narrow case where the first review ran via the ci_wait_s escape hatch while CI is still pending. Probably intended — I've asked for a deliberate nod (a comment, or carrying known.seen_at through) so the bypass isn't accidental.
Doc nit: the Tuning Guide documents both keys, but nisse-review.example.yaml's review-tuning list still stops at attempts.

One small gap, not about a specific line: the except GiteaError branch in _ci_settled (log review.checks_unknown, wait and ask again) is the one path with no test. It's a safe, narrow, mostly-unreachable defensive branch (if Gitea is down, intake already fails first), so it's minor — but a one-line test with gitea.down = True would close it out.

CI is still pending. The PR's framing is sound: the running bring-up is on the old code, so this PR reviews at once beside CI ("the before"); the next PR shows the wait actually holding ("the after").

Clean, well-scoped follow-up to #154. The wait logic in `_next`/`_ci_settled` is correct across all four documented behaviours (I traced every branch: pending, pending-past-limit, no-CI-grace, finished-passed-or-failed), the state machine reads well, and it fits R1. The new `seen_at` on the item is the right place to keep a head's first sighting, and pre-existing items (no `seen_at`) fall back to `0.0` → reviewed immediately, exactly the "older items don't wait" requirement. The two tuning keys are genuinely wired: `parse_tuning` builds the tuning dataclass via `fields()`, so they're recognized, validated as positive floats, and reachable from config — matching the Tuning Guide. The `world()` helper already forwards `**more` to `Scheduler`, so the tests' `clock=`/`ci_wait_s=`/`ci_grace_s=` work without a helper change. The four tests are discriminating — I verified each fails if the wait is removed, including "holds up no other," which correctly keys on `opened_at` (deterministic: store orders by `opened_at`, `sorted` is stable). Two things I flagged on the lines: - **`seen_at` is dropped in the failure/recovery `PR(...)` constructions** (scheduler.py:266 and :310), so a failed-then-retried item (or a crash-recovered one) that goes back to READY has `seen_at=0.0` and is re-reviewed immediately, skipping the CI wait. That's the right call when CI already finished (and the brief re-fetches `checks` anyway, so the reviewer still sees current CI), but it leaks the "stays off a GPU while CI uses it" guarantee in the narrow case where the first review ran via the `ci_wait_s` escape hatch while CI is still pending. Probably intended — I've asked for a deliberate nod (a comment, or carrying `known.seen_at` through) so the bypass isn't accidental. - **Doc nit:** the Tuning Guide documents both keys, but `nisse-review.example.yaml`'s review-tuning list still stops at `attempts`. One small gap, not about a specific line: the `except GiteaError` branch in `_ci_settled` (log `review.checks_unknown`, wait and ask again) is the one path with no test. It's a safe, narrow, mostly-unreachable defensive branch (if Gitea is down, intake already fails first), so it's minor — but a one-line test with `gitea.down = True` would close it out. CI is still pending. The PR's framing is sound: the running bring-up is on the old code, so this PR reviews at once beside CI ("the before"); the next PR shows the wait actually holding ("the after").

nisse-review.example.yaml Outdated

						
				@@ -34,5 +34,5 @@

				# tuning:                             # docs/Tuning-Guide.md

				#   review: {poll_s: 60, max_turns: 40, diff_chars: 60000, attempts: 2}

				#   expert: {consult_budget: 3}

				#   context: {keep_recent_turns: 6, safety_margin: 0.1}

nisse-review commented

2026-10-17 00:59:01 +00:00

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).

nisse-review/src/nisse_review/scheduler.py Outdated

nisse-review commented

2026-10-17 00:59:01 +00:00

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.

dongar approved these changes

2026-10-17 04:32:09 +00:00

Dismissed

dongar approved these changes

2026-10-17 04:32:27 +00:00

Dismissed

dev-agent added 1 commit 2026-10-17 04:33:53 +00:00

Tuning: the CI waits may be zero

CI / checks (push) Skipped

CI / e2e (pull_request) Canceled after 0s

Details

CI / checks (pull_request) Canceled after 1m18s

Details

59fba3bb76

The live review test sets ci_grace_s: 0, which the shared tuning parser
refused (every number had to be positive). A field now opts in to zero
with metadata {"zero": True}; ci_wait_s and ci_grace_s do.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dev-agent dismissed dongar's review 2026-10-17 04:33:53 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dev-agent referenced this issue from a commit

2026-10-17 04:35:05 +00:00

nisse-review: a retried or recovered review keeps its head's CI wait

dev-agent added 1 commit 2026-10-17 04:35:05 +00:00

nisse-review: a retried or recovered review keeps its head's CI wait

CI / checks (push) Skipped

CI / checks (pull_request) Successful in 1m41s

Details

CI / e2e (pull_request) Successful in 7m41s

Details

637761ba5a

From nisse-review's own review of #155: a failed run, a posted one and
crash recovery rebuilt the item without seen_at, so a retry skipped the
CI wait. seen_at now carries through, renewed when the head moved during
the review. Also: a test for checks Gitea can't report, and the example
config lists ci_wait_s and ci_grace_s.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dev-agent reviewed

2026-10-17 04:35:17 +00:00

nisse-review.example.yaml

						
				@@ -36,3 +36,3 @@

				# 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,

dev-agent commented

2026-10-17 04:35:17 +00:00

Added in 637761b.

Added in 637761b.

nisse-review/src/nisse_review/scheduler.py

						
				@@ -214,0 +263,4 @@

				            data=data(

				                PR(

				                    known.repo,

				                    known.number,

dev-agent commented

2026-10-17 04:35:17 +00:00

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`.