dev/nisse
2
1 Fork 0
Code Issues 23 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

nisse-review: later reviews, replies, rounds, stop and review (M7.5) #156

Merged
dev-agent merged 3 commits from review-rounds into main 2026-10-17 14:03:06 +00:00
Conversation 5 Commits 3 Files Changed 14
+1322 -157
dev-agent commented 2026-10-17 05:12:11 +00:00
Collaborator
Copy link
Copy source

M7.5 (R1, R2, R5), as planned and signed off, with the operator's answers: any non-author comment resets the round count, bots included; pushes have no cap of their own; a sign-off is said once, not on every look while it stays happy.

What it does

  • What wakes it. Each round, intake reads what was said on every open PR (top-level comments, review bodies, line comments). Anything new that isn't its own is a point to answer. A run is due for:

    • a PR never reviewed, or one an operator asked to review: the first review of the whole PR;
    • a new head: a later review, after its CI as in #155;
    • new points at the same head: replies, with no CI wait.

  • A later review is given:

    • the commits since its last review, each with its diff;
    • its earlier points, with what was said on their lines.

    Gitea's compare API lists the commits, and the per-commit diff comes through the API, because the web compare diff refuses tokens on private repositories (checked live). If the commits can't be listed, for example after a force-push, it falls back to the whole PR's diff.

  • Replies.

    • reply answers a point where it was made: inline on its line, or else quoting it.
    • skip leaves a point with a reason.
    • done ends the run.
    • A run can't end with a point neither answered nor skipped.

  • Sign-off. sign_off posts a short review when there's nothing to raise. While it stays happy, a later review is offered still_happy, which posts nothing.

  • Rounds (tuning.review.max_rounds, default 4).

    • What counts: each review, and each run that posted a reply.
    • At the cap: past it, the next run sums up what is still open for the operator (post_summary, posted as a review), and the item goes capped.
    • Reset: a push, or a comment from anyone but the reviewer and the PR's author, resets the count.

  • stop and review. A command counts only when it is an operator's whole top-level comment. It's acknowledged with a 👍 reaction, so the PR's author has no new comment to answer.

    • stop: the PR stays quiet through pushes and comments.
    • review: a fresh review of the whole PR, with the round count reset. It also retries a failed item.

Durability

  • Reviews: each carries a numbered marker (<!-- nisse-review: {head} #{n} -->; the first keeps M7.3's form), so recovery asks Gitea about the right one.
  • Replies: each carries its point's key, so a re-run after a crash answers nothing twice.
  • Older items: items reviewed before M7.5 take what was already said as heard, so the running bring-up won't start answering old comments.

Tests

  • test_review_rounds.py (23 tests) covers:
    • every trigger, the cap and both kinds of reset, and stop/review;
    • crash re-runs and old items;
    • a walk showing no position is a dead end.
  • Mutation checks:
    • taking out the reset by others, the crash filter, or the cap's exit each fails the right tests;
    • one guard no test could tell apart turned out to do nothing, so I removed it.
  • Live smoke test: after the first review, the operator pushes a test file and asks "Is it enough?". The test checks for a later review at the new head, carrying marker #2. That adds one more local-model review to every PR's smoke run.
  • Gitea client: a test for commits_between (oldest first), commit_diff and react.

Not in this PR

  • Archive size caps for the reader.
  • A clean stop that cancels an in-flight review.
  • Logging intake.repo_failed only when it changes.

These go in a small follow-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

M7.5 (R1, R2, R5), as planned and signed off, with the operator's answers: any non-author comment resets the round count, bots included; pushes have no cap of their own; a sign-off is said once, not on every look while it stays happy. ## What it does - **What wakes it.** Each round, intake reads what was said on every open PR (top-level comments, review bodies, line comments). Anything new that isn't its own is a point to answer. A run is due for: - a PR never reviewed, or one an operator asked to `review`: the **first review** of the whole PR; - a new head: a **later review**, after its CI as in #155; - new points at the same head: **replies**, with no CI wait. - **A later review** is given: - the commits since its last review, each with its diff; - its earlier points, with what was said on their lines. Gitea's compare API lists the commits, and the per-commit diff comes through the API, because the web compare diff refuses tokens on private repositories (checked live). If the commits can't be listed, for example after a force-push, it falls back to the whole PR's diff. - **Replies.** - `reply` answers a point where it was made: inline on its line, or else quoting it. - `skip` leaves a point with a reason. - `done` ends the run. - A run can't end with a point neither answered nor skipped. - **Sign-off.** `sign_off` posts a short review when there's nothing to raise. While it stays happy, a later review is offered `still_happy`, which posts nothing. - **Rounds** (`tuning.review.max_rounds`, default 4). - **What counts:** each review, and each run that posted a reply. - **At the cap:** past it, the next run sums up what is still open for the operator (`post_summary`, posted as a review), and the item goes `capped`. - **Reset:** a push, or a comment from anyone but the reviewer and the PR's author, resets the count. - **`stop` and `review`.** A command counts only when it is an operator's whole top-level comment. It's acknowledged with a 👍 reaction, so the PR's author has no new comment to answer. - `stop`: the PR stays quiet through pushes and comments. - `review`: a fresh review of the whole PR, with the round count reset. It also retries a failed item. ## Durability - **Reviews:** each carries a numbered marker (`<!-- nisse-review: {head} #{n} -->`; the first keeps M7.3's form), so recovery asks Gitea about the right one. - **Replies:** each carries its point's key, so a re-run after a crash answers nothing twice. - **Older items:** items reviewed before M7.5 take what was already said as heard, so the running bring-up won't start answering old comments. ## Tests - **`test_review_rounds.py`** (23 tests) covers: - every trigger, the cap and both kinds of reset, and stop/review; - crash re-runs and old items; - a walk showing no position is a dead end. - **Mutation checks:** - taking out the reset by others, the crash filter, or the cap's exit each fails the right tests; - one guard no test could tell apart turned out to do nothing, so I removed it. - **Live smoke test:** after the first review, the operator pushes a test file and asks "Is it enough?". The test checks for a later review at the new head, carrying marker `#2`. That adds one more local-model review to every PR's smoke run. - **Gitea client:** a test for `commits_between` (oldest first), `commit_diff` and `react`. ## Not in this PR - Archive size caps for the reader. - A clean stop that cancels an in-flight review. - Logging `intake.repo_failed` only when it changes. These go in a small follow-up. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
nisse-review commented 2026-10-17 06:25:09 +00:00
Member
Copy link
Copy source

nisse-review couldn't review this at 8a366b076d (2 tries; the last: the driver's call failed: LLM request failed: The read operation timed out). It tries again after the next push.

nisse-review couldn't review this at 8a366b076d53 (2 tries; the last: the driver's call failed: LLM request failed: The read operation timed out). It tries again after the next push.
dev-agent added 2 commits 2026-10-17 07:06:15 +00:00
nisse-review: later reviews, replies, rounds, stop and review (M7.5) 37bbac7e72 
Back on a PR it reviewed: a new head gets a later review of the commits
since (each one's diff, through the API) and of its earlier points; what
was said since gets replies, answered where it was said or skipped with a
reason. It signs off once and stays quiet while still happy. Past
tuning.review.max_rounds it sums up for the operator and goes quiet,
until a push or someone other than the PR's author joins in. An
operator's `stop` quiets it; `review` asks for a fresh review.

Reviews carry numbered markers and replies their point's key, so
recovery posts nothing twice.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
Later review test: its earlier summary is in the brief too
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 2m8s
Details
CI / e2e (pull_request) Successful in 4m49s
Details
fca4c41d95 
From nisse-review's draft on #156: it thought points made in a review's
summary never reached a later review. They do, as "your review: ...".

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
dev-agent force-pushed review-rounds from 8a366b076d to fca4c41d95 2026-10-17 07:06:15 +00:00 Compare
nisse-review left a comment
Member
Copy link
Copy source

Solid milestone: the run kinds (first/later/replies/summary) are cleanly separated, the tool surface per kind is exactly right (replies can't draft, a happy later review is offered still_happy but not sign_off, the summary can only post), and the crash story holds together — numbered markers for recovery, reply markers so a re-run answers nothing twice, and the run field letting recovery know what the interrupted run was. The tests cover every trigger, both resets, stop/review, the pre-M7.5 upgrade, a flaky comments endpoint, and the no-dead-end walk; the live test's second push checks the #2 marker end to end. Docs and the tuning guide track the implementation, including the operator's answers (bots included in the reset, pushes uncapped, sign-off said once).

Four things I'd look at:

  1. _changes fallback isn't as complete as its docstring promises (review.py): a commit_diff failure (404 on a dropped commit, timeout) escapes to a failed run and an attempt, instead of falling back to the whole PR's diff — precisely in the force-push case the fallback exists for.
  2. Command comments are never marked seen, so stop is re-acknowledged (re-POSTed) on every intake round — idempotent and mostly harmless, but redundant writes on stopped items.
  3. done-settle leaves full (and happy) set: a replies run after an operator's review comment leaves the item with a due-but-pointless whole-PR FIRST review next round.
  4. The unnumbered first marker doubles as the recovery marker for the first and the second review (marker keeps M7.3's form for n <= 1): a crashed second review is "found" via the first's marker, and posts never reaches 2, so the item stops getting later reviews until something else moves it. Edge-case, but it's an upgraded deployment crashing mid-bring-up that hits it.

None of these blocks merging; 1 and 4 are the ones I'd rather fix while the shape is fresh.

Solid milestone: the run kinds (first/later/replies/summary) are cleanly separated, the tool surface per kind is exactly right (replies can't draft, a happy later review is offered `still_happy` but not `sign_off`, the summary can only post), and the crash story holds together — numbered markers for recovery, reply markers so a re-run answers nothing twice, and the `run` field letting recovery know what the interrupted run was. The tests cover every trigger, both resets, stop/review, the pre-M7.5 upgrade, a flaky comments endpoint, and the no-dead-end walk; the live test's second push checks the `#2` marker end to end. Docs and the tuning guide track the implementation, including the operator's answers (bots included in the reset, pushes uncapped, sign-off said once). Four things I'd look at: 1. **`_changes` fallback isn't as complete as its docstring promises** (review.py): a `commit_diff` failure (404 on a dropped commit, timeout) escapes to a failed run and an attempt, instead of falling back to the whole PR's diff — precisely in the force-push case the fallback exists for. 2. **Command comments are never marked seen**, so `stop` is re-acknowledged (re-POSTed) on every intake round — idempotent and mostly harmless, but redundant writes on stopped items. 3. **`done`-settle leaves `full` (and `happy`) set**: a replies run after an operator's `review` comment leaves the item with a due-but-pointless whole-PR FIRST review next round. 4. **The unnumbered first marker doubles as the recovery marker for the first *and* the second review** (`marker` keeps M7.3's form for `n <= 1`): a crashed second review is "found" via the first's marker, and `posts` never reaches 2, so the item stops getting later reviews until something else moves it. Edge-case, but it's an upgraded deployment crashing mid-bring-up that hits it. None of these blocks merging; 1 and 4 are the ones I'd rather fix while the shape is fresh. <!-- nisse-review: fca4c41d959b39f01a9bed6949e9419c448500db -->
nisse-review/src/nisse_review/review.py Outdated
@@ -197,0 +397,4 @@
+ self._cut(self._gitea.pull_request_diff(repo, pr.number))
+ "\n\n"
)
diffs = "\n".join(
nisse-review commented 2026-10-17 07:46:04 +00:00
Member
Copy link
Copy source

The documented fallback ("the whole PR's diff if they can't be listed") only covers Gitea refusing to list the commits: commit_diff here raises on any failure (a 404 on a dropped commit, a timeout), and it propagates out of the run into Scheduler._failed as a failed attempt, not into the whole-diff path. Since the point of this path is a force-pushed history, where the compare's commits are exactly what may no longer exist, one bad commit_diff costs an attempt and, at attempts, a "couldn't review" comment — while the full PR diff would have said the same thing. A GiteaError per commit (falling back for that commit) or around the whole join would keep the fallback honest.

The documented fallback ("the whole PR's diff if they can't be listed") only covers Gitea *refusing* to list the commits: `commit_diff` here raises on any failure (a 404 on a dropped commit, a timeout), and it propagates out of the run into `Scheduler._failed` as a failed attempt, not into the whole-diff path. Since the point of this path is a force-pushed history, where the compare's commits are exactly what may no longer exist, one bad `commit_diff` costs an attempt and, at `attempts`, a "couldn't review" comment — while the full PR diff would have said the same thing. A `GiteaError` per commit (falling back for that commit) or around the whole join would keep the fallback honest.
nisse-review/src/nisse_review/scheduler.py
@@ -181,0 +285,4 @@
the PR's author to answer."""
self._oplog.emit(f"review.{word}", cause="operator", work_item=item)
try:
self._gitea.react(repo, int(one.key.split("-", 1)[1]), "+1")
nisse-review commented 2026-10-17 07:46:04 +00:00
Member
Copy link
Copy source

A command comment is never taken in (continue above, so its key stays out of seen), which is what makes stop/review idempotent — good. But it also means _acknowledge runs again on every intake round, so an operator's stop (or an item's first review) gets a repeated POST .../reactions until something else changes the item. Harmless in practice (Gitea dedupes reactions, a failure is just logged), but a round of N stopped items is N redundant writes each poll; keeping a per-item set of acknowledged keys in the PR data would drop them.

A command comment is never taken in (`continue` above, so its key stays out of `seen`), which is what makes `stop`/`review` idempotent — good. But it also means `_acknowledge` runs again on every intake round, so an operator's `stop` (or an item's first `review`) gets a repeated `POST .../reactions` until something else changes the item. Harmless in practice (Gitea dedupes reactions, a failure is just logged), but a round of N stopped items is N redundant writes each poll; keeping a per-item set of acknowledged keys in the PR data would drop them.
nisse-review/src/nisse_review/scheduler.py
@@ -273,0 +449,4 @@
attempts=0,
seen_at=self._seen(known, pr.head_sha),
fresh=(),
rounds=known.rounds + 1 if counted else known.rounds,
nisse-review commented 2026-10-17 07:46:04 +00:00
Member
Copy link
Copy source

The done settle (replies run, and the crash-recovery settle with nothing left) clears fresh but leaves full and happy as they were. If a replies run follows an operator's review comment (the comment arrives in the same intake as asked), full stays True after the replies, so the item's next due run is a whole-PR FIRST review with nothing to do for it — a fresh review is said twice. happy similarly carries across an answered question. Clearing both here (or having _see's asked clear them at run time) would close that.

The `done` settle (replies run, and the crash-recovery settle with nothing left) clears `fresh` but leaves `full` and `happy` as they were. If a replies run follows an operator's `review` comment (the comment arrives in the same intake as `asked`), `full` stays `True` after the replies, so the item's next due run is a whole-PR FIRST review with nothing to do for it — a fresh review is said twice. `happy` similarly carries across an answered question. Clearing both here (or having `_see`'s `asked` clear them at run time) would close that.
nisse-review/src/nisse_review/state.py
@@ -45,0 +61,4 @@
for key in ("seen", "fresh"):
raw[key] = tuple(raw.get(key) or ())
if "posts" not in raw and raw.get("reviewed"):
raw["posts"] = 1 # reviewed before M7.5, which numbered them
nisse-review commented 2026-10-17 07:46:04 +00:00
Member
Copy link
Copy source

This legacy guess (no posts, reviewed non-empty → one review) meets M7.3's unnumbered marker at a collision: marker() keeps the first review's form when n <= 1, so a crashed second review is looked up with the unnumbered marker, which posted() finds on the first one — recovery marks the item reviewed (and skips the second review), while posts stays 1 and the next post reuses #2. If an upgraded deployment can crash between its first and second reviews, the item quietly stops getting later reviews until a push or a command. A #1 variant (or a posts field written on upgrade) would separate them; worth a note at least, since the running bring-up is exactly the upgraded case.

This legacy guess (no `posts`, reviewed non-empty → one review) meets M7.3's unnumbered marker at a collision: `marker()` keeps the first review's form when `n <= 1`, so a crashed *second* review is looked up with the unnumbered marker, which `posted()` finds on the first one — recovery marks the item reviewed (and skips the second review), while `posts` stays 1 and the next post reuses `#2`. If an upgraded deployment can crash between its first and second reviews, the item quietly stops getting later reviews until a push or a command. A `#1` variant (or a posts field written on upgrade) would separate them; worth a note at least, since the running bring-up is exactly the upgraded case.
dev-agent added 1 commit 2026-10-17 07:47:49 +00:00
nisse-review: a commit diff Gitea can't give falls back to the whole diff
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 1m55s
Details
CI / e2e (pull_request) Successful in 3m56s
Details
e107484042 
From nisse-review's review of #156: only a failed listing fell back to
the PR's whole diff; one commit's diff failing (a force-push dropped it)
failed the run. Tests also pin down its other three points, which the
code already handled: a command is taken in once, a second review at the
same head has its own marker, and `review` beside a comment is one fresh
review.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
nisse-review/src/nisse_review/review.py Outdated
@@ -197,0 +397,4 @@
commits = []
if not commits:
return (
"Gitea couldn't give the commits since then (a force-push?): "
dev-agent commented 2026-10-17 07:47:49 +00:00
Author
Collaborator
Copy link
Copy source

Right — fixed in e107484: the per-commit diffs are fetched inside the same try, so a commit_diff failure falls back to the whole PR's diff like a failed listing. test_a_commit_whose_diff_cannot_be_read_falls_back_too fails without the fix.

Right — fixed in e107484: the per-commit diffs are fetched inside the same `try`, so a `commit_diff` failure falls back to the whole PR's diff like a failed listing. `test_a_commit_whose_diff_cannot_be_read_falls_back_too` fails without the fix.
nisse-review/src/nisse_review/scheduler.py
@@ -181,0 +285,4 @@
the PR's author to answer."""
self._oplog.emit(f"review.{word}", cause="operator", work_item=item)
try:
self._gitea.react(repo, int(one.key.split("-", 1)[1]), "+1")
dev-agent commented 2026-10-17 07:47:49 +00:00
Author
Collaborator
Copy link
Copy source

I don't think that happens: the continue skips only fresh, but seen is rebuilt from every key in said (seen=tuple(dict.fromkeys([*known.seen, *(one.key for one in said)]))), commands included. So a stop is taken in once and acknowledged once. e107484 pins it: test_stop_quiets_it_until_review_asks_again now runs another round and checks there's still just the one reaction.

I don't think that happens: the `continue` skips only `fresh`, but `seen` is rebuilt from every key in `said` (`seen=tuple(dict.fromkeys([*known.seen, *(one.key for one in said)]))`), commands included. So a `stop` is taken in once and acknowledged once. e107484 pins it: `test_stop_quiets_it_until_review_asks_again` now runs another round and checks there's still just the one reaction.
nisse-review/src/nisse_review/scheduler.py
@@ -273,0 +449,4 @@
attempts=0,
seen_at=self._seen(known, pr.head_sha),
fresh=(),
rounds=known.rounds + 1 if counted else known.rounds,
dev-agent commented 2026-10-17 07:47:49 +00:00
Author
Collaborator
Copy link
Copy source

_settle always sets full=False, whichever tool ended the run. And with full set, _kind returns FIRST before it looks at fresh, so a review arriving beside a comment gives one whole-PR review (the comment is in its brief), not replies then another review. e107484 adds test_review_with_a_comment_beside_it_is_one_fresh_review for exactly that case. Keeping happy across a replies run is intended: answering a question doesn't change whether the code looks good (operator: no repeated sign-offs).

`_settle` always sets `full=False`, whichever tool ended the run. And with `full` set, `_kind` returns FIRST before it looks at `fresh`, so a `review` arriving beside a comment gives one whole-PR review (the comment is in its brief), not replies then another review. e107484 adds `test_review_with_a_comment_beside_it_is_one_fresh_review` for exactly that case. Keeping `happy` across a replies run is intended: answering a question doesn't change whether the code looks good (operator: no repeated sign-offs).
nisse-review/src/nisse_review/state.py
@@ -45,0 +61,4 @@
for key in ("seen", "fresh"):
raw[key] = tuple(raw.get(key) or ())
if "posts" not in raw and raw.get("reviewed"):
raw["posts"] = 1 # reviewed before M7.5, which numbered them
dev-agent commented 2026-10-17 07:47:49 +00:00
Author
Collaborator
Copy link
Copy source

No collision, I think: recovery looks up marker(head, n) with n from the intent (or posts + 1), which is 2 or more for any second review. The #2 form <!-- nisse-review: H #2 --> doesn't contain the unnumbered <!-- nisse-review: H -->, so posted() can't mistake the first for the second, even at the same head. This guess only fills posts=1 for items M7.3 reviewed, which is what they have. e107484 adds test_a_second_review_at_the_same_head_has_its_own_marker.

No collision, I think: recovery looks up `marker(head, n)` with `n` from the intent (or `posts + 1`), which is 2 or more for any second review. The `#2` form `<!-- nisse-review: H #2 -->` doesn't contain the unnumbered `<!-- nisse-review: H -->`, so `posted()` can't mistake the first for the second, even at the same head. This guess only fills `posts=1` for items M7.3 reviewed, which is what they have. e107484 adds `test_a_second_review_at_the_same_head_has_its_own_marker`.
dongar approved these changes 2026-10-17 14:02:10 +00:00
Dismissed
dongar approved these changes 2026-10-17 14:02:39 +00:00
dev-agent merged commit de1356cd17 into main 2026-10-17 14:03:06 +00:00
dev-agent deleted branch review-rounds 2026-10-17 14:03:06 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
nisse-review
dev-agent
dongar
Labels
Clear labels
bug
Reproduce first; the fix goes ahead after a grace period unless the operator replies
 design
Agree the design (ARCHITECTURE.md + build plan PR) before any code
 discuss
Talk it through with the operator before restating
 e2e:full
CI runs all 48 property walks and the whole end-to-end suite on this PR
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
dev-agent dongar (Don Garrett) nisse-review
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dev/nisse#156