From 75958aef288427d7d0d4290d88d36f6f95baddb8 Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Mon, 19 Oct 2026 13:16:07 +0000 Subject: [PATCH 1/2] Development Plan: every agent is its own container image Each agent ships as its own image, since that's how it runs on the agent host. nisse-review's image becomes M7.6, ahead of the agent host; its deployment moves to M7.7. nisse-ops' image comes with its first slice. Also fixes two stale references to M7.6. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk --- docs/Development-Plan.md | 32 ++++++++++++++++++++------- nisse-dev/src/nisse_dev/loop/agent.py | 2 +- scripts/review-bringup.sh | 4 ++-- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index 95b8adc..4664ae3 100644 --- a/docs/Development-Plan.md +++ b/docs/Development-Plan.md @@ -39,8 +39,8 @@ Only `nisse-dev` has real code; `nisse-core` and `nisse-ops` are placeholders. | **D1–D2** Workflow | partial | On the new loop: issue intake, restatement and go-ahead (M2.1), labels (M2.3); no documents in the repo | | **D3–D4** Workspace, git | partial | Idempotent publishing; no fresh workspace (#62), `git add -A` (#80) | | **D5** Review | partial | PR conversation comments only (#69), watch stall (#70) | -| **D7–D8** Deployment, projects | partial | Runs from a venv as a systemd unit on this box; project entries in config (M1.4); egress not enforced | -| **O**, **X** | none | No container image yet | +| **D7–D8** Deployment, projects | partial | Bring-up runs `nisse-dev`'s image under systemd on this box, `nisse-review` from a venv; project entries in config (M1.4); egress not enforced | +| **O**, **X** | partial | `nisse-dev` has its image (M1.5); `nisse-review` doesn't yet (M7.6); nothing runs on the agent host | | `specs/` | done for now | `loop.qnt` (the architecture, #91); `orchestrator.qnt` went with `control.py` (M2.8) | --- @@ -86,6 +86,11 @@ Only `nisse-dev` has real code; `nisse-core` and `nisse-ops` are placeholders. (`-k`) or diagnosed (`--lf`); `scripts/e2e.sh` reports what a run spent. Manual live runs are for the new loop's first runs and phase ends. Findings are filed as issues and either fixed before moving on or deliberately deferred. +- **Every agent is its own container image** (X1), since that's how each one + runs on the agent host: built from the repository's `Dockerfile`, published by + `release.yml` to `gitea.dongar.net/dev/`, and tested over the image the + way `nisse-dev`'s is (M1.5). A new agent's phase includes its image, before the + agent host is ready for it, and its bring-up runs from that image. - **The Quint model moves with the loop:** `loop.qnt` models the architecture, and its properties are tests (the property walk), changed in the same PR as a rule they cover. @@ -541,7 +546,8 @@ Terraform isn't running; the operator is planning both, Terraform with hand (Ubuntu 24 now, 26 soon), with `vm ready` for a rebuilt one's host key. **M6.1 The agent host.** *Needs discussion.* Containers, volumes, snapshots, -per-agent networks (X1, X2, X4). +per-agent networks (X1, X2, X4). Each agent runs there from its own published +image (§3). **M6.2 Build VMs by Terraform.** *Needs discussion.* The template, VLAN and egress (D7, X2), collection when idle. @@ -718,9 +724,18 @@ of earlier points, replies on threads, the round cap and its reset, `stop` and L6 above. Each cut fitting still makes is logged (`context.trimmed`). Tests: `test_compact.py` in nisse-core. -**M7.6 Deployment.** *Waits for the agent host (Phase 6).* Its image, container, -Gitea account and secrets (R5). The image needs `git`, and `scratch/` a -persistent volume, so clones outlive a restart. +**M7.6 `nisse-review`'s image.** *Pre-approved.* Doesn't wait for the agent +host. Its own image, as `nisse-dev`'s (M1.5, §3): a `nisse-review` target in the +`Dockerfile`, running as its own user, with state in a volume, configuration +mounted read-only and secrets as environment variables. The image needs `git`, +and `scratch/` a persistent volume, so clones outlive a restart. `release.yml` +publishes `gitea.dongar.net/dev/nisse-review` beside `nisse-dev`, and +`review-bringup.sh` runs the image instead of a virtualenv. +- Acceptance: a release publishes both images, and the review end-to-end tests + run over the published `nisse-review` image. + +**M7.7 Deployment.** *Waits for the agent host (Phase 6).* Its container on the +agent host, Gitea account and secrets (R5). ### Phase 8 — `nisse-ops` @@ -729,8 +744,9 @@ credentials (L12), with masking what an API returns as #144; search-then-dispatc 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*. +(O6); its own image comes with its first slice, as every agent's does (§3). +**M8.5 Requests between agents:** `nisse-dev` asking for build VMs (L1, O5). All +*needs discussion*. ### Not yet scheduled diff --git a/nisse-dev/src/nisse_dev/loop/agent.py b/nisse-dev/src/nisse_dev/loop/agent.py index 10ab567..194a2be 100644 --- a/nisse-dev/src/nisse_dev/loop/agent.py +++ b/nisse-dev/src/nisse_dev/loop/agent.py @@ -3056,7 +3056,7 @@ class LoopAgent: """Block a build VM (D3): every project on it waits until it is recovered. Said once at top level, and in each waiting item's thread. The recovery is the operator's for now, or the VM's coming back; - nisse-ops' later (M7.6).""" + nisse-ops' later (M8.5).""" if host in self._store.vm_blocks(): return # already said self._store.block_vm( diff --git a/scripts/review-bringup.sh b/scripts/review-bringup.sh index 1c5c8d2..764d97f 100755 --- a/scripts/review-bringup.sh +++ b/scripts/review-bringup.sh @@ -2,8 +2,8 @@ # Bring-up runs of nisse-review on this development VM (Development Plan M7.3). # # It runs from the exported code under test, in a virtualenv of its own, as a -# transient systemd unit, so it outlives the shell that started it. Its -# container comes with M7.6. +# transient systemd unit, so it outlives the shell that started it. It runs +# from its image after M7.6, and on the agent host after M7.7. # # scripts/review-bringup.sh start [REF] export REF (default HEAD), install # it, and start the reviewer From 26ef125d1ed66fa62e5c8b13215537b1fa37b9fb Mon Sep 17 00:00:00 2001 From: Don Garrett Date: Mon, 19 Oct 2026 20:11:40 +0000 Subject: [PATCH 2/2] nisse-review: a review refused for an open point posts once the point is answered 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) Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk --- docs/Development-Plan.md | 7 ++- nisse-core/src/nisse_core/loop/driver.py | 23 ++++++- nisse-core/tests/test_driver.py | 69 +++++++++++++++++++++ nisse-review/src/nisse_review/review.py | 78 ++++++++++++++++++------ nisse-review/tests/test_review_rounds.py | 66 ++++++++++++++++++++ 5 files changed, 221 insertions(+), 22 deletions(-) diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index 4664ae3..5b00a9f 100644 --- a/docs/Development-Plan.md +++ b/docs/Development-Plan.md @@ -683,8 +683,11 @@ of earlier points, replies on threads, the round cap and its reset, `stop` and can't be listed, and its earlier points with what was said on their lines. - **Replies:** `reply` answers a point where it was made (inline on its line, else quoting it), `skip` leaves it with a reason, `done` ends; every point is - answered or skipped before a run may end. Each reply carries its point's key, - so a run re-done after a crash answers nothing twice. + answered or skipped before a run may end. An ending call refused for that is + held, and goes ahead as soon as the last point is settled, rather than left + to the model to make again (on #166 it didn't, and looped to the turn cap). + Each reply carries its point's key, so a run re-done after a crash answers + nothing twice. - **Sign-off:** `sign_off` posts a short review when there is nothing to raise; while it stays happy, a later review offers `still_happy`, which posts nothing (operator: no repeated LGTMs). diff --git a/nisse-core/src/nisse_core/loop/driver.py b/nisse-core/src/nisse_core/loop/driver.py index da54c83..374b9c2 100644 --- a/nisse-core/src/nisse_core/loop/driver.py +++ b/nisse-core/src/nisse_core/loop/driver.py @@ -108,6 +108,7 @@ class Driver: on_turn: Callable[[str], None] | None = None, screen: Callable[[ToolCall], Screening] | None = None, max_turns: int | None = None, + due: Callable[[], tuple[str, dict[str, Any], str] | None] | None = None, ) -> RunOutcome: """``waiting``: the control tools that end the run, for the scheduler to act on. ``handlers`` answer those that don't (e.g. @@ -115,7 +116,10 @@ class Driver: ``checks`` vet a waiting tool's arguments: a problem is the call's failed result and the run goes on, so the model puts it right. ``screen`` judges each execution call before it reaches the tool VM - (P4).""" + (P4). ``due``, asked after each handled call, names a waiting call + the harness held back that may now go ahead (tool, arguments, what + to tell the model): the run ends with it, as if the model had made + it again.""" handlers = handlers or {} checks = checks or {} model = model or self._model # the work item's pinned driver (L9) @@ -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 + if held is not None: + tool, arguments, said = held + transcript.append( + ToolResult( + call_id=call.call_id, + result=f"{result}\n\n[harness] {said}; your turn " + "has ended", + success=True, + ) + ) + skip_calls( + transcript, + calls[index + 1 :], + "not run: the turn had ended", + ) + return RunOutcome("wait", tool, arguments) elif call.tool not in offered or workspace is None: result, ok = ( f"no such tool: {call.tool}; the tools are " diff --git a/nisse-core/tests/test_driver.py b/nisse-core/tests/test_driver.py index 37b1a8e..e381699 100644 --- a/nisse-core/tests/test_driver.py +++ b/nisse-core/tests/test_driver.py @@ -81,3 +81,72 @@ def test_a_tool_not_named_waiting_doesnt_end_the_run(tmp_path: Path) -> None: ) assert ran.tool == "post_review" assert results(log)[0].startswith("no such tool: ask_human") + + +NOTE = tool("note", "Note something.", text="The note.") + + +def test_a_held_call_goes_ahead_once_due_says_so(tmp_path: Path) -> None: + # post_review is refused while a point is open and held by the agent; + # the handled call that settles the point ends the run with it. + open_points = ["P1"] + held: dict[str, Any] = {} + + def refuse(args: dict[str, Any]) -> str | None: + if open_points: + held["call"] = ("post_review", args) + return "answer P1 first" + return None + + def settle(args: dict[str, Any]) -> tuple[str, bool]: + open_points.clear() + return "answered P1", True + + def due() -> tuple[str, dict[str, Any], str] | None: + if "call" in held and not open_points: + tool_, args = held.pop("call") + return tool_, args, "your held post_review went ahead" + return None + + model = Scripted( + [call("post_review", body="the summary")], + [call("note", text="P1"), call("read_file", path="a.py")], + # never reached: the run ended on the note + [call("note", text="again")], + ) + log = transcript(tmp_path) + reader = Reader() + ran = Driver(model, "m").run( + log, + system="s", + tools=[READ, POST, NOTE], + waiting={"post_review"}, + workspace=reader, + handlers={"note": settle}, + checks={"post_review": refuse}, + due=due, + ) + assert (ran.kind, ran.tool, ran.arguments) == ( + "wait", + "post_review", + {"body": "the summary"}, + ) + assert results(log) == [ + "answer P1 first", + "answered P1\n\n[harness] your held post_review went ahead; your turn has ended", + "not run: the turn had ended", + ] + assert reader.calls == [] # the read after it never ran + + +def test_without_due_a_settled_point_leaves_the_run_going(tmp_path: Path) -> None: + model = Scripted([call("note", text="x")], [call("post_review", body="b")]) + ran = Driver(model, "m").run( + transcript(tmp_path), + system="s", + tools=[POST, NOTE], + waiting={"post_review"}, + workspace=None, + handlers={"note": lambda args: ("noted", True)}, + ) + assert (ran.tool, ran.arguments) == ("post_review", {"body": "b"}) diff --git a/nisse-review/src/nisse_review/review.py b/nisse-review/src/nisse_review/review.py index 4afcb1b..5762acd 100644 --- a/nisse-review/src/nisse_review/review.py +++ b/nisse-review/src/nisse_review/review.py @@ -18,7 +18,7 @@ from nisse_core.knowledge.handlers import Knowledge from nisse_core.log.canonical import Fact from nisse_core.loop.driver import Driver, RunOutcome from nisse_core.loop.expert import BudgetExhausted, ExpertDesk -from nisse_core.loop.tools import ASK_EXPERT, tool +from nisse_core.loop.tools import ASK_EXPERT, names, tool from nisse_core.models.llm import LLMError from nisse_core.oplog import OpLog from nisse_core.state.store import Transcript @@ -168,6 +168,10 @@ class Draft: points: dict[str, Said] = field(default_factory=dict) # P1 -> the point replied: list[str] = field(default_factory=list) # keys answered skipped: list[str] = field(default_factory=list) # keys left, with reason + # An ending call refused only because points were open: it goes ahead + # once the last is answered or skipped, not left to the model to repeat. + held: tuple[str, dict[str, Any]] | None = None + ends: tuple[str, ...] = () # the run's ending tools, for the messages def open_points(self) -> list[str]: settled = {*self.replied, *self.skipped} @@ -283,19 +287,53 @@ class Reviewer: elif kind == LATER: tools.append(STILL_HAPPY) - def answered_first(args: dict[str, Any]) -> str | None: - left = draft.open_points() - if left: - return "first answer or skip each point said to you: " + ", ".join(left) - return None + draft.ends = tuple(t for t in names(tools) if t in WAITING) - def no_comments(args: dict[str, Any]) -> str | None: - if draft.comments: + def answered_first(tool: str) -> Callable[[dict[str, Any]], str | None]: + def check(args: dict[str, Any]) -> str | None: + left = draft.open_points() + if not left: + return None + draft.held = (tool, args) return ( - f"you drafted {len(draft.comments)} comment(s): post them " - "with post_review" + "first answer or skip each point said to you: " + + ", ".join(left) + + f". Your {tool} is held: it goes ahead once the last is " + "answered or skipped" ) - return answered_first(args) + + return check + + def no_comments(tool: str) -> Callable[[dict[str, Any]], str | None]: + def check(args: dict[str, Any]) -> str | None: + if draft.comments: + return ( + f"you drafted {len(draft.comments)} comment(s): post them " + "with post_review" + ) + return answered_first(tool)(args) + + return check + + checks = { + "post_review": answered_first("post_review"), + "sign_off": no_comments("sign_off"), + "still_happy": no_comments("still_happy"), + "done": answered_first("done"), + } + + def due() -> tuple[str, dict[str, Any], str] | None: + if draft.held is None or draft.open_points(): + return None + tool, args = draft.held + draft.held = None + if checks[tool](args) is not None: + return None # wrong for another reason now: the model's to fix + return ( + tool, + args, + f"that was the last open point, so your held {tool} went ahead", + ) outcome = self._driver.run( transcript, @@ -304,13 +342,9 @@ class Reviewer: waiting=WAITING, workspace=repo, handlers=handlers, - checks={ - "post_review": answered_first, - "sign_off": no_comments, - "still_happy": no_comments, - "done": answered_first, - }, + checks=checks, max_turns=self._max_turns, + due=due, ) return outcome, draft @@ -535,8 +569,14 @@ class Reviewer: wanted = f"P{wanted}" if wanted in draft.open_points(): return wanted, draft.points[wanted] - left = ", ".join(draft.open_points()) or "none" - return f"no open point {wanted or '(none given)'}; open: {left}" + left = draft.open_points() + if not left: + return ( + f"no open point {wanted or '(none given)'}: every point said to " + "you is answered or skipped. End your run with " + + " or ".join(draft.ends or ("your ending tool",)) + ) + return f"no open point {wanted or '(none given)'}; open: {', '.join(left)}" def _comment( self, repo: Repository, draft: Draft, args: dict[str, Any] diff --git a/nisse-review/tests/test_review_rounds.py b/nisse-review/tests/test_review_rounds.py index 513a9fc..4548c6f 100644 --- a/nisse-review/tests/test_review_rounds.py +++ b/nisse-review/tests/test_review_rounds.py @@ -409,3 +409,69 @@ def test_review_with_a_comment_beside_it_is_one_fresh_review(tmp_path: Path) -> known = pr_of(item(w)) assert (known.full, known.fresh, item(w).position) == (False, (), REVIEWED) assert w["scheduler"].round() is False # nothing more due + + +# --- a held ending call (the run on #166 at bf98445) -------------------------------- + + +def later_with_a_point(tmp_path: Path, *turns: list[Any]) -> dict[str, Any]: + """A push and an answer on its line since its first review: a later run + with P1 to answer.""" + w = reviewed(tmp_path, *turns) + push(w) + w["gitea"].note("nisse-dev", "split() is fine: no tabs here.", "count.py:2") + return w + + +def test_a_review_refused_for_an_open_point_posts_once_it_is_answered( + tmp_path: Path, +) -> None: + # The model posted with P1 open, answered it, then only skipped answered + # points until the turn cap: the review went out partial, its summary lost. + loop = [[call("skip", point="P1", reason="already answered")]] * 5 + w = later_with_a_point( + tmp_path, + [call("post_review", summary="Both commits do what they say.")], + [call("reply", point="P1", text="Fair: conceded.")], + *loop, + ) + assert w["scheduler"].round() is True + review = w["gitea"].posted[-1] + assert "Both commits do what they say." in review["body"] + assert "partial" not in review["body"].lower() + assert item(w).position == REVIEWED + model = w["scheduler"]._reviewer._driver._client + assert len(model._turns) == 5 # none of the skips ran + told = str(model.seen[-1]) + assert "Your post_review is held" in told + + +def test_a_held_sign_off_waits_if_comments_were_drafted_since(tmp_path: Path) -> None: + # Held while P1 was open, but a drafted comment needs post_review now: + # the harness doesn't sign off over it; the model posts instead. + w = later_with_a_point( + tmp_path, + [call("sign_off", summary="All good.")], + [call("comment", path="count.py", line="2", text="One more thing.")], + [call("reply", point="P1", text="Agreed.")], + [call("post_review", summary="One more point.")], + ) + assert w["scheduler"].round() is True + review = w["gitea"].posted[-1] + assert "One more point." in review["body"] + assert ["One more thing." in c.body for c in review["comments"]] == [True] + + +def test_with_nothing_open_a_skip_says_how_to_end(tmp_path: Path) -> None: + w = later_with_a_point( + tmp_path, + [call("reply", point="P1", text="Agreed.")], + [call("skip", point="P1", reason="again")], + [call("post_review", summary="Done.")], + ) + w["scheduler"].round() + model = w["scheduler"]._reviewer._driver._client + assert "every point said to you is answered or skipped. End your run with" in str( + model.seen[-1] + ) + assert "post_review" in str(model.seen[-1]).split("End your run with")[1]