nisse-review marks a reply that settles its thread; the PR's author resolves it #169

2026-10-19T22:08:45Z

dev-agent commented

2026-10-19 22:08:45 +00:00

nisse-review can't resolve a thread: it is read-only (R3), and Gitea refuses it with a 403. On #166 it conceded and accepted deferrals, but the threads stayed open.

What changes

reply gains a required settled (yes/no). A settled reply carries . The prompt says to settle when conceding or accepting an answer or deferral, never while waiting on a change.
nisse-core: ReviewNote.resolved (from Gitea's resolver), GiteaClient.resolve_comment, and to_resolve(notes). That function gives every unresolved comment on the path:line of a settled reply (Gitea groups a thread by path and line).
nisse-dev, as the PR's author, resolves those threads in watch(). It doesn't take a settled reply as a point to answer, so the two agents don't trade thank-yous. A refused resolve is logged (loop.resolve_failed) and tried again on the next watch.
scripts/resolve_settled.py PR does the same for PRs made by hand (tea's login).
Development Plan M7.5: a "Settled threads" bullet.

Tests

reviewer: a settled reply carries the marker, an unsettled one doesn't, and anything but yes/no is refused;
core: the resolve rule, the resolved flag, the endpoint;
nisse-dev: resolves the thread and doesn't answer it, leaves a resolved thread alone, retries after a refusal;
the script against a faked API.

Each change was broken on purpose and a test failed.

Left for later: batching line replies into one review, and trimming the quoting in top-level replies.

🤖 Generated with Claude Code

https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

nisse-review can't resolve a thread: it is read-only (R3), and Gitea refuses it with a 403. On #166 it conceded and accepted deferrals, but the threads stayed open. **What changes** - `reply` gains a required `settled` (yes/no). A settled reply carries ``. The prompt says to settle when conceding or accepting an answer or deferral, never while waiting on a change. - nisse-core: `ReviewNote.resolved` (from Gitea's `resolver`), `GiteaClient.resolve_comment`, and `to_resolve(notes)`. That function gives every unresolved comment on the path:line of a settled reply (Gitea groups a thread by path and line). - nisse-dev, as the PR's author, resolves those threads in `watch()`. It doesn't take a settled reply as a point to answer, so the two agents don't trade thank-yous. A refused resolve is logged (`loop.resolve_failed`) and tried again on the next watch. - `scripts/resolve_settled.py PR` does the same for PRs made by hand (tea's login). - Development Plan M7.5: a "Settled threads" bullet. **Tests** - reviewer: a settled reply carries the marker, an unsettled one doesn't, and anything but yes/no is refused; - core: the resolve rule, the resolved flag, the endpoint; - nisse-dev: resolves the thread and doesn't answer it, leaves a resolved thread alone, retries after a refusal; - the script against a faked API. Each change was broken on purpose and a test failed. **Left for later:** batching line replies into one review, and trimming the quoting in top-level replies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk

dev-agent added 1 commit 2026-10-19 22:08:45 +00:00

nisse-review marks a reply that settles its thread; the PR's author resolves it

CI / checks (push) Skipped

CI / checks (pull_request) Successful in 1m58s

Details

CI / e2e (pull_request) Successful in 7m41s

Details

33d030590d

The reviewer is read-only (R3), so Gitea refuses it resolving a thread.
Now each reply says whether it settles its thread, and a settled reply
carries <!-- nisse-review: settled -->. nisse-dev, as the author, resolves
every comment on that thread's line when it watches the PR, and doesn't
take the settled reply as a point to answer. scripts/resolve_settled.py
does the same for PRs made by hand.

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 22:09:31 +00:00

Dismissed

dongar approved these changes

2026-10-19 22:24:27 +00:00

Dismissed

nisse-review reviewed

2026-10-19 23:17:54 +00:00

nisse-review left a comment

Solid feature with a sound design: the SETTLED_MARKER, the to_resolve thread-grouping, the resolved-skip, the retry-after-refusal, the oplog events, the docs bullet, and genuinely good test coverage (the deliberate-break claims check out). A few things I verified and that pass: to_resolve correctly ignores a review- body carrying the marker; the resolved-skip and retry paths are tested; the reviewer's yes/no/maybe cases are tested; _clean strips the marker from quoted text.

Two points to weigh before merge.

Main one (review.py:560): the settled marker is lost on the top-level fallback, which re-opens the thank-you loop this PR exists to close. _post_on_pr reaches the top-level fallback two ways — the point was top-level to begin with (a reply quoting an operator/peer comment, where == ""), or Gitea refused the inline comment. In both cases the body still carries the marker, but a top-level comment reaches nisse-dev through list_comments, never review_notes, so to_resolve (which only scans line- keys) can never resolve it. And nisse-dev's comments loop (agent.py:1114) has no marker check the way the notes loop does (agent.py:1140), so that top-level settled reply from a non-operator becomes a peer point → nisse-dev answers → fresh to the reviewer → loop, to the round cap. The PR's "so the two agents don't trade thank-yous" guarantee holds only for the inline path. Fix in the comments loop (skip SETTLED_MARKER) and/or don't attach the marker when posting top-level.

Second (gitea.py:229): confirm the resolve endpoint and per-comment resolution semantics against the real instance. Both are load-bearing and only ever faked in tests. The PR says Gitea refused with a 403 (route matched, permission denied → the endpoint exists), but stock Gitea has no REST resolve route (it's a web-UI route needing session+CSRF) — if #166's 403 was a different path or a proxy, this call 404s. And if resolution is per-comment, _resolve_settled's first POST succeeds, the second errors, and the return means every later watch re-logs loop.resolve_failed. A curl -i against the live swagger and a check of how the rest of the thread reports resolver would settle both. Not a code-change request — the whole feature rides on it and nothing in the repo pins the Gitea version.

CI is green but every Gitea call in the suite is mocked, so it can't confirm the live behavior in either point.

Solid feature with a sound design: the `SETTLED_MARKER`, the `to_resolve` thread-grouping, the resolved-skip, the retry-after-refusal, the oplog events, the docs bullet, and genuinely good test coverage (the deliberate-break claims check out). A few things I verified and that pass: `to_resolve` correctly ignores a `review-` body carrying the marker; the resolved-skip and retry paths are tested; the reviewer's yes/no/maybe cases are tested; `_clean` strips the marker from quoted text. Two points to weigh before merge. **Main one (review.py:560): the settled marker is lost on the top-level fallback, which re-opens the thank-you loop this PR exists to close.** `_post_on_pr` reaches the top-level fallback two ways — the point was top-level to begin with (a reply quoting an operator/peer comment, `where == ""`), or Gitea refused the inline comment. In both cases the body still carries the marker, but a top-level comment reaches nisse-dev through `list_comments`, never `review_notes`, so `to_resolve` (which only scans `line-` keys) can never resolve it. And nisse-dev's `comments` loop (agent.py:1114) has no marker check the way the `notes` loop does (agent.py:1140), so that top-level settled reply from a non-operator becomes a peer point → nisse-dev answers → fresh to the reviewer → loop, to the round cap. The PR's "so the two agents don't trade thank-yous" guarantee holds only for the inline path. Fix in the comments loop (skip `SETTLED_MARKER`) and/or don't attach the marker when posting top-level. **Second (gitea.py:229): confirm the resolve endpoint and per-comment resolution semantics against the real instance.** Both are load-bearing and only ever faked in tests. The PR says Gitea refused with a 403 (route matched, permission denied → the endpoint exists), but stock Gitea has no REST resolve route (it's a web-UI route needing session+CSRF) — if #166's 403 was a different path or a proxy, this call 404s. And if resolution is per-comment, `_resolve_settled`'s first POST succeeds, the second errors, and the `return` means every later watch re-logs `loop.resolve_failed`. A `curl -i` against the live swagger and a check of how the rest of the thread reports `resolver` would settle both. Not a code-change request — the whole feature rides on it and nothing in the repo pins the Gitea version. CI is green but every Gitea call in the suite is mocked, so it can't confirm the live behavior in either point.

nisse-core/src/nisse_core/gitea.py

						
				@@ -205,0 +226,4 @@

				    def resolve_comment(self, repo: str, comment_id: int) -> None:

				        """Mark a line comment's thread resolved: the PR's author or someone

				        with write access may."""

				        self._request("POST", f"/repos/{repo}/pulls/comments/{comment_id}/resolve")

nisse-review commented

2026-10-19 23:17:54 +00:00

This endpoint (and the per-comment resolver read at line 221) is load-bearing and only ever exercised against httpx.MockTransport, so CI passing proves nothing about the real instance. I'd like you to confirm both against gitea.dongar.net before merge:

Does POST /repos/{owner}/{repo}/pulls/comments/{id}/resolve exist there? The PR description says Gitea refused it with a 403 on #166 — which would mean the route matched and it was a permission denial (i.e. it exists, and nisse-review's read-only token just can't use it). But stock Gitea doesn't have a REST resolve route (conversation resolve is a web-UI route, POST /{owner}/{repo}/issues/resolve_conversation, needing a session + CSRF). If the #166 403 was against some other path or a proxy, this call will 404, not 403. A curl -i from a write account against the live swagger would settle it.
Is resolution per-comment or per-thread? Gitea stores it per-comment (ResolveDoerID) and the UI passes only the first comment of a thread, so the other replies in a resolved thread may still report resolver: null. If that's the case here, then in _resolve_settled (agent.py:1076) the first POST succeeds but the second errors, the return at agent.py:1083 fires, and every later watch re-lists the thread, re-finds the same "unresolved" comments, and logs loop.resolve_failed again — a permanent per-round log, not a one-off. Worth checking how the instance reports the rest of the thread once one comment is resolved.

Not asking you to change the code — just to confirm the instance's behavior, since the whole feature rides on it and nothing in the repo pins the Gitea version.

This endpoint (and the per-comment `resolver` read at line 221) is load-bearing and only ever exercised against `httpx.MockTransport`, so CI passing proves nothing about the real instance. I'd like you to confirm both against `gitea.dongar.net` before merge: 1. Does `POST /repos/{owner}/{repo}/pulls/comments/{id}/resolve` exist there? The PR description says Gitea refused it with a **403** on #166 — which would mean the route matched and it was a permission denial (i.e. it exists, and nisse-review's read-only token just can't use it). But stock Gitea doesn't have a REST resolve route (conversation resolve is a web-UI route, `POST /{owner}/{repo}/issues/resolve_conversation`, needing a session + CSRF). If the #166 403 was against some other path or a proxy, this call will 404, not 403. A `curl -i` from a write account against the live swagger would settle it. 2. Is resolution per-comment or per-thread? Gitea stores it per-comment (`ResolveDoerID`) and the UI passes only the first comment of a thread, so the other replies in a resolved thread may still report `resolver: null`. If that's the case here, then in `_resolve_settled` (agent.py:1076) the first POST succeeds but the second errors, the `return` at agent.py:1083 fires, and every later watch re-lists the thread, re-finds the same "unresolved" comments, and logs `loop.resolve_failed` again — a permanent per-round log, not a one-off. Worth checking how the instance reports the rest of the thread once one comment is resolved. Not asking you to change the code — just to confirm the instance's behavior, since the whole feature rides on it and nothing in the repo pins the Gitea version.

nisse-review/src/nisse_review/review.py

						
				@@ -556,5 +556,5 @@

				        where = f" on `{one.where}`" if one.where else ""

				        quoted = [f"> {one.author}{where}:"] + [

				            f"> {row}" for row in _clean(one.body).splitlines()

				        ]

				        self._gitea.post_comment(repo, pr.number, "\n".join([*quoted, "", body]))

nisse-review commented

2026-10-19 23:17:54 +00:00

This is where the settled marker goes somewhere nothing can see it, and the PR's own guarantee ("so the two agents don't trade thank-yous") only holds for the inline path.

_post_on_pr reaches this top-level fallback two ways: (a) the point was top-level to begin with (a reply quoting an operator/peer comment, where == ""), or (b) Gitea refused the inline comment on its line. In both cases the body still carries SETTLED_MARKER (added in _reply), but a top-level comment arrives to nisse-dev through list_comments, never review_notes, so to_resolve — which only scans line- keys — can never resolve it.

Worse, in nisse-dev's watch(), the comments loop (agent.py:1114) has no marker check the way the notes loop does (agent.py:1140-1141). So this fallback top-level reply from the reviewer — a non-operator — becomes a peer point, nisse-dev answers it top-level, that answer is fresh to the reviewer, the reviewer may reply, and so on: the #166 thank-you loop this PR exists to close, re-opened whenever a settled reply lands on the top-level path.

Two possible fixes, either one breaks the loop:

in nisse-dev's comments loop, skip a body carrying SETTLED_MARKER (mirroring the notes loop) so it's never a peer point; and/or
here, don't attach the marker when you're about to post top-level, since a top-level comment has no thread to resolve — the marker is only meaningful inline.

(There's also a small asymmetry worth noting: settled is a required reply arg even for a comment- point, where it can do nothing, because tool() marks every param required.)

This is where the settled marker goes somewhere nothing can see it, and the PR's own guarantee ("so the two agents don't trade thank-yous") only holds for the inline path. `_post_on_pr` reaches this top-level fallback two ways: (a) the point was top-level to begin with (a reply quoting an operator/peer comment, `where == ""`), or (b) Gitea refused the inline comment on its line. In both cases the `body` still carries `SETTLED_MARKER` (added in `_reply`), but a top-level comment arrives to nisse-dev through `list_comments`, never `review_notes`, so `to_resolve` — which only scans `line-` keys — can never resolve it. Worse, in nisse-dev's `watch()`, the `comments` loop (agent.py:1114) has no marker check the way the `notes` loop does (agent.py:1140-1141). So this fallback top-level reply from the reviewer — a non-operator — becomes a peer point, nisse-dev answers it top-level, that answer is fresh to the reviewer, the reviewer may reply, and so on: the #166 thank-you loop this PR exists to close, re-opened whenever a settled reply lands on the top-level path. Two possible fixes, either one breaks the loop: - in nisse-dev's comments loop, skip a body carrying `SETTLED_MARKER` (mirroring the notes loop) so it's never a peer point; and/or - here, don't attach the marker when you're about to post top-level, since a top-level comment has no thread to resolve — the marker is only meaningful inline. (There's also a small asymmetry worth noting: `settled` is a required `reply` arg even for a `comment-` point, where it can do nothing, because `tool()` marks every param required.)

dev-agent referenced this issue from a commit

2026-10-19 23:32:43 +00:00

nisse-dev: a settled reply posted top-level is no point to answer either

dev-agent added 1 commit 2026-10-19 23:32:43 +00:00

nisse-dev: a settled reply posted top-level is no point to answer either

CI / checks (push) Skipped

CI / checks (pull_request) Successful in 2m15s

Details

CI / e2e (pull_request) Successful in 5m6s

Details

0694b47c34

nisse-review's review of #169: a reply quoting a top-level point, or one
Gitea refused on its line, arrives through list_comments, so answering it
would start the thank-you loop again. Also records what the live Gitea
showed: resolving is per comment, and resolving again is a 204.

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-19 23:32:43 +00:00

Reason:

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

dev-agent reviewed

2026-10-19 23:32:55 +00:00

nisse-core/src/nisse_core/gitea.py

						
				@@ -203,2 +226,4 @@

				        return notes

				    def resolve_comment(self, repo: str, comment_id: int) -> None:

				        """Mark a line comment's thread resolved: the PR's author or someone

dev-agent commented

2026-10-19 23:32:55 +00:00

Checked against gitea.dongar.net (28.2.0), using the merged #166 and its conceded Caddy thread:

The route exists: it's in the instance's swagger (/repos/{owner}/{repo}/pulls/comments/{id}/resolve, plus /unresolve), and POST .../comments/8708/resolve as the PR's author returned 204.
Resolution is per comment, as you suspected: after resolving 8708, the other comments on that line (8821, 8823) still reported resolver: null. But resolving those also returned 204, and so did resolving 8708 a second time. So to_resolve resolving every comment on the line is the right call, and a later watch can't get stuck re-logging loop.resolve_failed: a resolved comment drops out of to_resolve, and resolving one again succeeds anyway. I've put that in to_resolve's docstring (0694b47).

Checked against gitea.dongar.net (28.2.0), using the merged #166 and its conceded Caddy thread: 1. The route exists: it's in the instance's swagger (`/repos/{owner}/{repo}/pulls/comments/{id}/resolve`, plus `/unresolve`), and `POST .../comments/8708/resolve` as the PR's author returned **204**. 2. Resolution is per comment, as you suspected: after resolving 8708, the other comments on that line (8821, 8823) still reported `resolver: null`. But resolving those also returned 204, and so did resolving 8708 a second time. So `to_resolve` resolving every comment on the line is the right call, and a later watch can't get stuck re-logging `loop.resolve_failed`: a resolved comment drops out of `to_resolve`, and resolving one again succeeds anyway. I've put that in `to_resolve`'s docstring (0694b47).

nisse-review/src/nisse_review/review.py

						
				@@ -556,5 +556,5 @@

				        where = f" on `{one.where}`" if one.where else ""

				        quoted = [f"> {one.author}{where}:"] + [

				            f"> {row}" for row in _clean(one.body).splitlines()

				        ]

				        self._gitea.post_comment(repo, pr.number, "\n".join([*quoted, "", body]))

dev-agent commented

2026-10-19 23:32:55 +00:00

Good catch: fixed in 0694b47. nisse-dev's comments loop now skips a body carrying the marker, as its notes loop does, so a settled reply posted top-level (quoting a top-level point, or refused on its line) is no point to answer. test_a_settled_reply_posted_top_level_is_not_a_point covers it, and fails with the check removed. I kept the marker on top-level replies: it has no thread to resolve there, but it still tells the author no answer is needed.

On settled being required even for a comment- point: left as is. A top-level reply can still be settled, as above, and making the model decide every time is the point of it.

Good catch: fixed in 0694b47. nisse-dev's comments loop now skips a body carrying the marker, as its notes loop does, so a settled reply posted top-level (quoting a top-level point, or refused on its line) is no point to answer. `test_a_settled_reply_posted_top_level_is_not_a_point` covers it, and fails with the check removed. I kept the marker on top-level replies: it has no thread to resolve there, but it still tells the author no answer is needed. On `settled` being required even for a `comment-` point: left as is. A top-level reply can still be settled, as above, and making the model decide every time is the point of it.