diff --git a/docs/Architecture.md b/docs/Architecture.md index 9e9c36f..7d3c106 100644 --- a/docs/Architecture.md +++ b/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 diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index ef347c9..85c3deb 100644 --- a/docs/Development-Plan.md +++ b/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 diff --git a/nisse-core/README.md b/nisse-core/README.md index 8674ace..f9f8d95 100644 --- a/nisse-core/README.md +++ b/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).