Spec: nisse-review (Part R) before nisse-ops

A review agent comes second: it reads every ready PR in the repositories
it can see and discusses its points on the PR, never changing code.

- Architecture Part R: duty and items (R1), rounds capped and reset by
  anyone else commenting (R2), read-only reach bound to the item's
  repository, no approvals, no build VM for now (R3), peers (R4), local
  model with ask_expert, no Matrix room for now, own identity (R5).
- D5 and L11: a peer's review is constructive criticism, answered point by
  point; never an instruction, approval or go-ahead; nisse-dev doesn't
  wait for it before asking to merge.
- Part Q: letting nisse-review approve a limited subset of PRs.
- Development Plan: Phase 7 is nisse-core and nisse-review (M7.2-M7.6);
  nisse-ops moves to Phase 8 (M8.1-M8.5), with the MCP decisions so far.

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

docs/Architecture.md

@@ -19,6 +19,7 @@ overnight and expects to be treated well in return.
 |---|---|---|
 | Shared library | `nisse-core` | What every agent is built on (Part L) |
 | Dev Agent | `nisse-dev` | Designs and builds software projects (Part D) |
+| Review Agent | `nisse-review` | Reviews pull requests (Part R) |
 | Sysadmin Agent | `nisse-ops` | Operates infrastructure (Part O) |
 
 More agents are planned (3D design work, video-file management), each another
@@ -519,6 +520,8 @@ can do rather than trying to prevent steering.
 - **Instruction channels are operator-scoped:** Matrix from allowlisted identities,
   comments and assignments from allowlisted accounts. When someone else wrote an
   issue, the operator's assignment is the instruction and the text stays data.
+  Another agent's comments are a peer's (R4): weighed on their merits, labelled
+  as such, never instructions.
 - **Intake is parsed into structured fields in code** where possible, and what
   reaches a model is labelled by source (L6).
 - **Approval messages are rendered by code;** the classifier never sees untrusted
@@ -776,6 +779,12 @@ recovers a VM for now; `nisse-ops` later (O5).
 - **Operator comments on a PR** (conversation and inline) are instructions; the
   agent answers with commits to the same PR, replying where the comment was made.
   Intake is as in L8.
+- **A peer's review** (`nisse-review`, Part R) is constructive criticism, not
+  instruction: the agent answers every point, changing the code within the item's
+  scope or pushing back with its reasons, and asks the operator about anything that
+  would change the scope. A peer's comment never approves, never counts as a
+  go-ahead and never runs a command. The agent doesn't wait for a review before
+  asking the operator to merge: the operator may weigh in on the discussion.
 - **The PR may have moved on meanwhile.** After pushing a fix, the agent checks the
   PR: if it merged before the fix landed, the fix goes out as a new PR; if it was
   closed, the agent says so and asks.
@@ -846,6 +855,91 @@ secret's visibility itself.
 
 ---
 
+## Part R — `nisse-review`
+
+Reviews every pull request that is ready, in the repositories it can see, and
+discusses its points on the PR. It reads and comments; it never changes code. Its
+purpose is a second, always-available look that pushes `nisse-dev` (and anyone
+else) towards better changes.
+
+### R1. How it works
+
+- **The duty:** watching the repositories its Gitea account can read for PRs
+  ready for review: open, not a draft, and not marked work in progress. Any
+  author's: the operator's, `nisse-dev`'s, or another tool's.
+- **A work item per PR,** scoped to its repository (L1). It closes when the PR
+  merges or closes.
+- **What starts a run:** the PR becoming ready, a new push, or a new comment or
+  review on it.
+- **The first review** is given the PR (title, description, its commits and diff,
+  its comments, and the issue it closes); the reviewer reads the rest as it needs
+  it (R3).
+- **A later review** covers what changed since its last one **and** its earlier
+  points: whether each was addressed, answered, or left alone, and it says so when
+  one was left unaddressed.
+- **What it checks:** code health; consistency with the PR's description, its
+  issue, and the repository's documented architecture and conventions; tests for
+  what changed, judged by reading them; and the CI result as Gitea reports it.
+- **It discusses on the PR,** in inline threads and top-level comments, and
+  answers replies to its points: it concedes a good argument and holds a point it
+  still believes, with reasons.
+
+### R2. Rounds
+
+Two agents can argue forever, so a PR's exchanges are capped.
+
+- **A round** is one review or set of replies the reviewer posts. After a capped
+  number of rounds (a tuning value) it posts a summary of what is still open, for
+  the operator, and stops answering on that PR.
+- **Anyone else commenting resets the count:** someone other than the reviewer
+  and the PR's agent author (in practice, the operator) has joined the
+  discussion. A push doesn't.
+
+### R3. Authority and reach
+
+- **Read-only on the repository:** code at any ref (not only what the PR touches),
+  documents, issues, other PRs, CI status, through its Gitea account. Reads are
+  bound by the harness to the item's repository, so what it might repeat in a
+  comment is already visible to that repository's readers.
+- **It writes only comments and reviews on the item's PR.** Its reviews never
+  approve and never request changes: the operator's approval alone counts for
+  merging (D5), and Gitea's branch protection enforces it.
+- **No execution for now:** no build VM and no commands (L13); coverage is judged
+  by reading the tests until a CI coverage report exists. If a reason appears
+  (running tests itself, measuring coverage), it can have a build VM of its own,
+  like `nisse-dev`'s (D3, D7).
+- **Its own memory** (L6.4), with notes per repository (L6.3), so it learns each
+  project's conventions and what the operator accepted or overruled.
+
+| Class | Examples |
+|---|---|
+| Harmless | Reading the repository, its issues and PRs, CI status, its notes |
+| Contained (in scope) | Reviews, comments and replies on its item's PR; its own notes |
+| Dangerous | Nothing is offered: no approval, no merge, no other repository |
+
+### R4. Its peers
+
+- **Its comments are a peer's** (L1, L11): `nisse-dev` weighs them and answers
+  (D5). Neither side's comments are instructions to the other.
+- **The operator's comments on a PR** are the operator's: the reviewer may take
+  them as settling a point, and stops pressing it.
+- **Prompt injection** (L11): PR text and code are untrusted; a steered reviewer can
+  only post a misleading comment on that PR, which the operator reads with the rest.
+
+### R5. Models and deployment
+
+- **The same local model drives it** as `nisse-dev` for now (P7), with
+  `ask_expert` for a frontier model's view (L9).
+- **No Matrix room** for now: the PR is its conversation. If every agent gets a
+  room, it gets one too. Until then the operator stops it on a PR by commenting
+  `stop` there (a review is asked for again with `review`), and stops it entirely
+  by stopping its container.
+- **Its own identity** (L12): a Gitea account (`nisse-review`) with read access to
+  the repositories it reviews, its own token and LiteLLM key, and its own
+  container on the agent host (X1), reaching only Gitea and LiteLLM.
+
+---
+
 ## Part O — `nisse-ops`
 
 Operates infrastructure across both sites, driven by issues and duties, with
@@ -1030,6 +1124,8 @@ shared between agents.
   sometimes from release branches: what the agent may do (a release is visible
   beyond its own area, so dangerous unless the trust dial says otherwise) and how a
   build plan names one.
+- **Letting `nisse-review` approve a limited subset of PRs:** which subset, and
+  how Gitea would enforce it (R3).
 - **Recovering the agent host.** It is a single point of failure for every agent,
   `nisse-ops` included, so its recovery can't depend on ops running. A recovery
   plan (rebuilding the host, restoring state from snapshots) is ops work still to

docs/Development-Plan.md

@@ -435,7 +435,7 @@ past the usual fixes (D3).
 - `loop.qnt` gained the block (`blocked_vm_runs_nothing`); it found that work in
   progress was lost on a reimage, hence parking first. The walk requests
   reimages and recovers them, reimaged or not.
-- Later, nisse-ops reimages a VM itself (M6.2, M7.6); a changed address means
+- Later, nisse-ops reimages a VM itself (M6.2, M8.5); a changed address means
   editing `build_host` and restarting.
 
 **M4.4** *(#137, with the operator).* **Done** (this PR). A VM that can't be
@@ -525,7 +525,7 @@ notes are written and read (L6.4).
   corrected; don't remember what it already holds.
 - Decided with the operator: `ARCHITECTURE.md` isn't put into every item's
   context (its size); the driver still reads it as told. The infrastructure
-  world model waits for `nisse-ops` (Phase 7).
+  world model waits for `nisse-ops` (Phase 8).
 - The walk writes project notes and checks they come only from that project's
   items.
 
@@ -553,7 +553,11 @@ LLDAP principal (L12).
 
 **M6.5 The browser tool.** *Pre-approved.* On the internal test network (D7).
 
-### Phase 7 — `nisse-core` and `nisse-ops`
+### Phase 7 — `nisse-core` and `nisse-review`
+
+`nisse-review` (Part R) comes before `nisse-ops`: it writes only comments, so it
+needs none of ops' machinery or infrastructure, while being a real second user of
+the loop. Decided with the operator, 2026-10-13.
 
 **M7.1 Extract `nisse-core`.** *Done.* What isn't the dev agent's moved to
 `nisse-core`, unchanged in behaviour, with its tests: the transcript and its
@@ -569,16 +573,39 @@ says so).
   `nisse_dev.state.StateStore` adds the build VM blocks (D3); stores already at
   version 5 open unchanged.
 - The vault names whose writes it records (`agent`), rather than assuming one.
-- Deferred until `nisse-ops` has a loop to share: splitting the loop (`loop/*`)
-  and configuration into a common part and the dev agent's. The dev agent's
-  `git`, delegation, `ask_expert`, executor tools and evaluation capture stay
-  in `nisse-dev`.
+- Deferred until a second agent has a loop to share: splitting the loop
+  (`loop/*`) and configuration into a common part and the dev agent's. The dev
+  agent's `git`, delegation, `ask_expert`, executor tools and evaluation capture
+  stay in `nisse-dev`.
 
-**M7.2 MCP servers** (#37). **M7.3 Approved step
-sequences** (O2). **M7.4 Ops intake and duties** from the infrastructure
-repository (O5). **M7.5 The ops tool VM and deployment** (O6). **M7.6 Requests
-between agents:** `nisse-dev` asking for build VMs (L1, O5). All *needs discussion*
-except M7.1.
+**M7.2 Share the loop.** *Needs discussion.* What of `nisse_dev.loop` and
+`config` is any agent's moves to `nisse-core`, shaped by what `nisse-review`
+needs; `nisse-dev` runs on it unchanged.
+
+**M7.3 `nisse-review`'s first slice.** *Needs discussion.* The duty and a work item
+per ready PR (R1), read-only tools bound to the item's repository (R3), a first
+review posted on the PR; bring-up on this box, and an end-to-end test against a
+PR in a test repository.
+
+**M7.4 `nisse-dev` answers a peer.** *Pre-approved.* Peer comments labelled as a
+peer's, answered point by point with a change or a reason (D5, R4).
+
+**M7.5 Discussion and rounds.** *Pre-approved.* Later reviews of what changed and
+of earlier points, replies on threads, the round cap and its reset, `stop` and
+`review` on a PR (R1, R2, R5).
+
+**M7.6 Deployment.** *Waits for the agent host (Phase 6).* Its image, container,
+Gitea account and secrets (R5).
+
+### Phase 8 — `nisse-ops`
+
+**M8.1 MCP servers** (#37). Settled so far: redaction scrubs each instance's own
+credentials (L12), with masking what an API returns as #144; search-then-dispatch
+meta-tools are avoided, not ruled out (X3). **M8.2 Approved step sequences** (O2).
+**M8.3 Ops intake and duties** from the infrastructure repository (O5), taking over
+the Terraform the operator is writing now. **M8.4 The ops tool VM and deployment**
+(O6). **M8.5 Requests between agents:** `nisse-dev` asking for build VMs (L1, O5).
+All *needs discussion*.
 
 ### Not yet scheduled
 
@@ -598,7 +625,7 @@ vision; one local model for every agent; several forges (#26); a room per projec
 | #27 per-project state, clone on use | M1.3 (clone on the build VM), M1.4 (projects) |
 | #28 room per project | Not yet scheduled (threads, M2.3, may make it unnecessary) |
 | #29 create a project from scratch | M2.5 (ownership settled by D4, D8) |
-| #37 MCP servers | M7.2 |
+| #37 MCP servers | M8.1 |
 | #42 per-turn commentary | M2.7 |
 | #62 branch at execution start | M2.1 |
 | #65 agent merges approved PRs | M3.3 |
@@ -606,6 +633,7 @@ vision; one local model for every agent; several forges (#26); a room per projec
 | #70 watch stall | M2.1 |
 | #77 conversational agent | M2.3 (questions), M2.7 |
 | #137 block on an unreachable build VM | M4.4 |
+| #144 redact secrets an MCP server returns | After M8.1 |
 
 ## 6. Implementation notes
 

nisse-core/README.md

@@ -19,4 +19,4 @@ never imports an agent.
   (`nisse-dev`'s are in `nisse_dev.loop.actions`).
 
 The loop is still `nisse-dev`'s, until `nisse-ops` has one to share it with
-(Development Plan M7.1).
+(Development Plan M7.2).