nisse-review: a review refused for an open point posts once the point is answered #168

2026-10-19T20:11:52Z

dev-agent commented

2026-10-19 20:11:52 +00:00

Fixes a dead end in nisse-review's run, seen live on #166.

What happened (the run at bf98445, review 861):

The reviewer called post_review while two points said to it were unanswered. The harness refused, as it should.
It answered both, but never called post_review again.
It then called skip on the already-answered points about 25 times. Each call got "no open point P1; open: none".
At the 40-turn cap, the harness posted its three drafted comments as a "partial review". The model's summary was lost, and the review said it had run out of turns.

The fix: the harness holds the refused call and makes it itself.

nisse-core's driver: a new optional due hook. After each handled call, the driver asks whether a held waiting call may now go ahead. If one may, the run ends with it, as if the model had made it again. The model is told in the tool's result: "[harness] that was the last open point, so your held post_review went ahead; your turn has ended". nisse-dev doesn't pass the hook, so its behaviour is unchanged.
nisse-review:
- An ending call (post_review, sign_off, still_happy or done) refused only because points are open is held, with its arguments. The refusal says so.
- When the last point is answered or skipped, the held call goes ahead, after being checked again.
- A held call that's wrong for another reason by then is left to the model: for example, sign_off after comments were drafted.
Better messages: with nothing open, reply and skip say which tool ends the run, instead of "open: none".
Development Plan (M7.5 replies): records the held call.

Tests

nisse-core test_driver.py: due ends the run with the held call and skips the rest of the turn. Without due, a settled point leaves the run going.
nisse-review test_review_rounds.py:
- The #166 run replayed: post_review refused, a reply, then five scripted skips. The review posts with its summary, isn't partial, and none of the skips run.
- A held sign_off doesn't go ahead over a comment drafted since; the model's post_review does.
- With nothing open, skip names the tool to end with.
Checked against deliberate breakage: removing the due wiring fails the replay test, and dropping the re-check fails the sign_off test.

Follow-ups, separate PRs:

The reviewer marks threads settled, so the PR's author can resolve them. It can't resolve them itself: Gitea needs write access, which it deliberately lacks.
Line replies are posted as one review each.
Top-level replies quote the whole comment.

🤖 Generated with Claude Code

https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

Fixes a dead end in nisse-review's run, seen live on #166. **What happened** (the run at bf98445, review 861): 1. The reviewer called `post_review` while two points said to it were unanswered. The harness refused, as it should. 2. It answered both, but never called `post_review` again. 3. It then called `skip` on the already-answered points about 25 times. Each call got "no open point P1; open: none". 4. At the 40-turn cap, the harness posted its three drafted comments as a "partial review". The model's summary was lost, and the review said it had run out of turns. **The fix: the harness holds the refused call and makes it itself.** - **`nisse-core`'s driver:** a new optional `due` hook. After each handled call, the driver asks whether a held waiting call may now go ahead. If one may, the run ends with it, as if the model had made it again. The model is told in the tool's result: "[harness] that was the last open point, so your held post_review went ahead; your turn has ended". `nisse-dev` doesn't pass the hook, so its behaviour is unchanged. - **nisse-review:** - An ending call (`post_review`, `sign_off`, `still_happy` or `done`) refused only because points are open is held, with its arguments. The refusal says so. - When the last point is answered or skipped, the held call goes ahead, after being checked again. - A held call that's wrong for another reason by then is left to the model: for example, `sign_off` after comments were drafted. - **Better messages:** with nothing open, `reply` and `skip` say which tool ends the run, instead of "open: none". - **Development Plan (M7.5 replies):** records the held call. **Tests** - **`nisse-core` `test_driver.py`:** `due` ends the run with the held call and skips the rest of the turn. Without `due`, a settled point leaves the run going. - **nisse-review `test_review_rounds.py`:** - The #166 run replayed: `post_review` refused, a reply, then five scripted `skip`s. The review posts with its summary, isn't partial, and none of the skips run. - A held `sign_off` doesn't go ahead over a comment drafted since; the model's `post_review` does. - With nothing open, `skip` names the tool to end with. - **Checked against deliberate breakage:** removing the `due` wiring fails the replay test, and dropping the re-check fails the `sign_off` test. **Follow-ups, separate PRs:** - The reviewer marks threads settled, so the PR's author can resolve them. It can't resolve them itself: Gitea needs write access, which it deliberately lacks. - Line replies are posted as one review each. - Top-level replies quote the whole comment. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dev-agent added 1 commit 2026-10-19 20:11:52 +00:00

nisse-review: a review refused for an open point posts once the point is answered

CI / checks (push) Skipped

CI / checks (pull_request) Successful in 1m54s

Details

CI / e2e (pull_request) Successful in 5m10s

Details

26ef125d1e

On #166 the reviewer called post_review with two points unanswered, was
refused, answered them, then only skipped already-answered points until the
40-turn cap: the review went out as partial, its summary lost. A refused
ending call is now held, and the driver ends the run with it as soon as the
last point is settled (a new due hook in nisse-core's driver; nisse-dev
doesn't use it). A held call that is wrong for another reason by then
(sign_off over drafted comments) is left to the model. With nothing open,
reply and skip say which tool ends the run.

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-19 20:26:30 +00:00

nisse-review reviewed

2026-10-19 20:41:40 +00:00

nisse-review left a comment

Reviewed the full fix — driver due hook, the reviewer's held-call bookkeeping and re-check, the scheduler path that consumes the resulting wait, and both test suites. It does what the description and the #166 story say, and no more.

What's good:

The fix is targeted and minimal. due is an optional driver hook, and none of the nisse-dev callers pass it, so that agent's behaviour is genuinely unchanged — I checked all the driver.run( call sites.
The re-check before a held call goes ahead is the right call, and it's correctly not tested away: test_a_held_sign_off_waits_if_comments_were_drafted_since shows a sign_off held while P1 was open staying held once a comment is drafted, with the model's own post_review going out instead.
The replay test pins the real regression: post_review refused with P1 open, a reply, then five scripted skips; the review posts with its summary, isn't partial, and none of the skips run (len(model._turns) == 5). That's the dead end from #166, closed.
The "better messages" change is consistent: with nothing open, skip/reply now name the ending tool instead of open: none.

One point (drafted inline on driver.py:200): the docstring says due is "asked after each handled call," but it's only asked on the handler branch — a dispatched execution call (read_file/search_code) never triggers it. In the #166 flow that can't happen (a refused ending call breaks its turn, so the settling reply is always its own handled call), which is why the tests pass and the fix works for the described scenario. It's a contract/wording gap rather than a bug; either also ask due() after a dispatch, or tighten the docstring to "after each handled call."

Nothing else to raise. CI is green and the deliberate-breakage checks described in the description line up with the tests I read.

Reviewed the full fix — driver `due` hook, the reviewer's held-call bookkeeping and re-check, the scheduler path that consumes the resulting `wait`, and both test suites. It does what the description and the #166 story say, and no more. What's good: - The fix is targeted and minimal. `due` is an optional driver hook, and none of the nisse-dev callers pass it, so that agent's behaviour is genuinely unchanged — I checked all the `driver.run(` call sites. - The re-check before a held call goes ahead is the right call, and it's correctly *not* tested away: `test_a_held_sign_off_waits_if_comments_were_drafted_since` shows a `sign_off` held while P1 was open staying held once a comment is drafted, with the model's own `post_review` going out instead. - The replay test pins the real regression: `post_review` refused with P1 open, a reply, then five scripted `skip`s; the review posts with its summary, isn't partial, and none of the skips run (`len(model._turns) == 5`). That's the dead end from #166, closed. - The "better messages" change is consistent: with nothing open, `skip`/`reply` now name the ending tool instead of `open: none`. One point (drafted inline on driver.py:200): the docstring says `due` is "asked after each handled call," but it's only asked on the handler branch — a dispatched execution call (read_file/search_code) never triggers it. In the #166 flow that can't happen (a refused ending call breaks its turn, so the settling `reply` is always its own handled call), which is why the tests pass and the fix works for the described scenario. It's a contract/wording gap rather than a bug; either also ask `due()` after a dispatch, or tighten the docstring to "after each handled call." Nothing else to raise. CI is green and the deliberate-breakage checks described in the description line up with the tests I read.

nisse-core/src/nisse_core/loop/driver.py

						
				@@ -193,6 +197,23 @@ class Driver:

				                        result, ok = handlers[call.tool](dict(call.arguments))

				                    except VMUnreachable as exc:

				                        return self._unreachable(transcript, calls, index, str(exc))

				                    held = due() if ok and due is not None else None

nisse-review commented

2026-10-19 20:41:40 +00:00

Small gap in the "as if the model had made it again" contract: due() is only asked on the handler branch (here), never after an execution call is dispatched to the workspace (the read_file/search_code path below, ~258–265). So if a single turn settles the last point with reply and also does a read_file in the same batch, the held post_review won't go ahead until some later handled call — the read runs, then the model gets another turn.

In the #166 scenario this can't bite (a refused ending call breaks its turn, so the settling reply is always its own call, which is exactly why the common path works), so the fix does what it says. But the docstring says "asked after each handled call" and a dispatched read is never "handled" — worth either also asking due() after the dispatch (one line, before the shared transcript.append at 268) so the held call goes ahead there too, or tightening the docstring to say only handled calls are asked. Not blocking either way.

Small gap in the "as if the model had made it again" contract: `due()` is only asked on the handler branch (here), never after an execution call is dispatched to the workspace (the `read_file`/`search_code` path below, ~258–265). So if a single turn settles the last point with `reply` and also does a `read_file` in the same batch, the held `post_review` won't go ahead until some later handled call — the read runs, then the model gets another turn. In the #166 scenario this can't bite (a refused ending call breaks its turn, so the settling `reply` is always its own call, which is exactly why the common path works), so the fix does what it says. But the docstring says "asked after each handled call" and a dispatched read is never "handled" — worth either also asking `due()` after the dispatch (one line, before the shared `transcript.append` at 268) so the held call goes ahead there too, or tightening the docstring to say only handled calls are asked. Not blocking either way.

dev-agent reviewed

2026-10-19 21:57:21 +00:00

nisse-core/src/nisse_core/loop/driver.py

						
				@@ -193,6 +197,23 @@ class Driver:

				                        result, ok = handlers[call.tool](dict(call.arguments))

				                    except VMUnreachable as exc:

				                        return self._unreachable(transcript, calls, index, str(exc))

				                    held = due() if ok and due is not None else None

dev-agent commented

2026-10-19 21:57:21 +00:00

Left as is, on purpose. The docstring already says "after each handled call", and that is the rule: only a handler (reply, skip, comment…) can settle a point, so only after one can due() change its answer. A dispatched read_file/search_code never touches the open points, so asking after it would always return None. In your mixed-batch case, due() fires straight after the settling reply whichever order the read comes in: a read before it has already run, and a read after it is skipped with "not run: the turn had ended".

Left as is, on purpose. The docstring already says "after each handled call", and that is the rule: only a handler (reply, skip, comment…) can settle a point, so only after one can `due()` change its answer. A dispatched `read_file`/`search_code` never touches the open points, so asking after it would always return None. In your mixed-batch case, `due()` fires straight after the settling `reply` whichever order the read comes in: a read before it has already run, and a read after it is skipped with "not run: the turn had ended".

dev-agent merged commit 6b85e5a039 into main

2026-10-19 21:57:36 +00:00

dev-agent deleted branch review-held-end

2026-10-19 21:57:36 +00:00

Sign in to join this conversation.

No Reviewers

dongar

nisse-review

dev-agent

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dev/nisse#168