nisse-review: a retried or recovered review keeps its head's CI wait
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
This commit is contained in:
1 parent
59fba3bb76
commit
637761ba5a
3 files changed
+68
-3
No files matched your search
@@ -35,6 +35,7 @@ gitea:
|
|||||||
# notes: ro
|
# notes: ro
|
||||||
|
|
||||||
# tuning: # docs/Tuning-Guide.md
|
# 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,
|
||||||
|
# ci_wait_s: 3600, ci_grace_s: 120}
|
||||||
# expert: {consult_budget: 3}
|
# expert: {consult_budget: 3}
|
||||||
# context: {keep_recent_turns: 6, safety_margin: 0.1}
|
# context: {keep_recent_turns: 6, safety_margin: 0.1}
|
||||||
@@ -212,6 +212,12 @@ class Scheduler:
|
|||||||
return False
|
return False
|
||||||
return not (state == "none" and waited < self._ci_grace_s)
|
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 ---------------------------------------------------------------
|
# -- a review ---------------------------------------------------------------
|
||||||
|
|
||||||
def review(self, item: WorkItem) -> None:
|
def review(self, item: WorkItem) -> None:
|
||||||
@@ -254,7 +260,16 @@ class Scheduler:
|
|||||||
self._store.update_work_item(
|
self._store.update_work_item(
|
||||||
item.id,
|
item.id,
|
||||||
position=REVIEWED,
|
position=REVIEWED,
|
||||||
data=data(PR(known.repo, known.number, pr.head_sha, pr.head_sha, 0)),
|
data=data(
|
||||||
|
PR(
|
||||||
|
known.repo,
|
||||||
|
known.number,
|
||||||
|
pr.head_sha,
|
||||||
|
pr.head_sha,
|
||||||
|
0,
|
||||||
|
self._seen(known, pr.head_sha),
|
||||||
|
)
|
||||||
|
),
|
||||||
cause="review posted",
|
cause="review posted",
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -263,7 +278,14 @@ class Scheduler:
|
|||||||
) -> None:
|
) -> None:
|
||||||
attempts = known.attempts + 1
|
attempts = known.attempts + 1
|
||||||
head = pr.head_sha if pr else known.head
|
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)
|
self._oplog.emit("review.failed", cause=why[:300], work_item=item.id)
|
||||||
if attempts < self._attempts:
|
if attempts < self._attempts:
|
||||||
self._store.update_work_item(
|
self._store.update_work_item(
|
||||||
@@ -313,6 +335,7 @@ class Scheduler:
|
|||||||
head,
|
head,
|
||||||
head if posted else known.reviewed,
|
head if posted else known.reviewed,
|
||||||
known.attempts,
|
known.attempts,
|
||||||
|
self._seen(known, head),
|
||||||
)
|
)
|
||||||
self._store.update_work_item(
|
self._store.update_work_item(
|
||||||
item.id,
|
item.id,
|
||||||
|
|||||||
@@ -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)
|
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:
|
def test_a_configured_repository_it_cannot_read_is_left_out() -> None:
|
||||||
gitea = FakeGitea(repos={REPO: []})
|
gitea = FakeGitea(repos={REPO: []})
|
||||||
out = io.StringIO()
|
out = io.StringIO()
|
||||||
|
|||||||
Reference in new issue
Block a user