diff --git a/nisse-dev/src/nisse_dev/loop/agent.py b/nisse-dev/src/nisse_dev/loop/agent.py index 7ed653a..e671034 100644 --- a/nisse-dev/src/nisse_dev/loop/agent.py +++ b/nisse-dev/src/nisse_dev/loop/agent.py @@ -561,8 +561,13 @@ class LoopAgent: f"PR #{loop.pr}: {fixed.note}", ) elif fixed.kind == "new-pr" and fixed.pr is not None: + # Later fixes go to the new PR's branch, never the merged one. loop = replace( - loop, pr=fixed.pr.number, pr_url=fixed.pr.url, fixes=loop.fixes + 1 + loop, + pr=fixed.pr.number, + pr_url=fixed.pr.url, + branch=fixed.branch, + fixes=loop.fixes + 1, ) self._wait( item, @@ -636,6 +641,15 @@ class LoopAgent: def _close(self, item: WorkItem, loop: Loop, outcome: str, note: str) -> None: self._fact(item, f"work item {outcome}: {note}") + if loop.pr and outcome == "abandoned": + # L2: abandoning closes the item's open PR with a note. + try: + self._gitea.post_comment(self._repo, loop.pr, f"abandoned: {note}") + self._gitea.close_pull_request(self._repo, loop.pr) + except GiteaError as exc: + self._transport.post( + f"could not close PR #{loop.pr}: {exc}", thread=loop.thread + ) save(self._store, item.id, CLOSED, loop, cause=outcome) self._store.close_work_item(item.id, outcome=outcome, cause=note) if loop.issue: diff --git a/nisse-dev/src/nisse_dev/loop/milestones.py b/nisse-dev/src/nisse_dev/loop/milestones.py index fa2cb11..efb6cea 100644 --- a/nisse-dev/src/nisse_dev/loop/milestones.py +++ b/nisse-dev/src/nisse_dev/loop/milestones.py @@ -32,6 +32,7 @@ class Fixed: kind: Literal["pushed", "nothing", "new-pr", "closed", "failed"] note: str pr: PullRequest | None = None + branch: str = "" # a new PR's branch, which later fixes go to class Milestones: @@ -177,7 +178,7 @@ class Milestones: summary=f"PR #{pr} merged before this review fix landed.", issue=0, ) - return Fixed("new-pr", new.note, new.pr) + return Fixed("new-pr", new.note, new.pr, follow) if state.state != "open": return Fixed("closed", f"PR #{pr} was closed", state) return Fixed("pushed", f"pushed {sha[:8]}", state) diff --git a/nisse-dev/tests/test_loop_milestones.py b/nisse-dev/tests/test_loop_milestones.py index 4fff477..8c4d643 100644 --- a/nisse-dev/tests/test_loop_milestones.py +++ b/nisse-dev/tests/test_loop_milestones.py @@ -96,6 +96,9 @@ class Forge: def close_issue(self, repo: str, index: int) -> None: self.closed_issues.append(index) + def close_pull_request(self, repo: str, index: int) -> None: + self.prs[index]["state"] = "closed" + # what the operator does in Gitea def merge(self, n: int) -> None: head = self.prs[n]["head"] @@ -429,3 +432,45 @@ def test_boot_asks_when_a_publish_never_reached_gitea(world: dict[str, Any]) -> assert store.pending_intents() == [] assert loop_of(item(world)).wait == ANSWER assert "publishing was cut off by a restart" in transport.outbound[-1].text + + +def test_a_fix_after_a_follow_up_pr_goes_to_the_follow_up( + world: dict[str, Any], +) -> None: + # Found by the property walk (seed 7): after a fix became a follow-up PR, + # the next fix was pushed to the original, merged branch and lost. + forge: Forge = world["forge"] + model = ScriptedModel( + RESTATE, + WORK + FINISH, + [call("write_file", path="fix1.py", content="a = 1\n"), *FINISH], + [call("write_file", path="fix2.py", content="b = 2\n"), *FINISH], + ) + agent, transport = loop_agent(world, model) + agent.intake() + say(transport, world, "go") + [first] = forge.prs + forge.comments[first] = [Comment(1, "don", "add fix1", "t")] + agent.watch() + forge.merge(first) # merged before the fix lands: a follow-up PR + agent._schedule() + follow = loop_of(item(world)).pr + assert follow != first + forge.comments[follow] = [Comment(2, "don", "add fix2", "t")] + agent.watch() + agent._schedule() + assert len(forge.prs) == 2 # pushed to the follow-up, not a third PR + assert "fix2.py" in forge.files(follow) + + +def test_abandoning_an_item_closes_its_open_pr(world: dict[str, Any]) -> None: + # Found by the property walk (seed 4); the model's fix 1 (L2). + forge: Forge = world["forge"] + agent, transport = loop_agent(world, ScriptedModel(RESTATE, WORK + FINISH)) + agent.intake() + say(transport, world, "go") + [pr] = forge.prs + say(transport, world, "abandon") + assert item(world).outcome == "abandoned" + assert forge.prs[pr]["state"] == "closed" + assert (pr, "abandoned: abandoned by @don:test") in forge.posted diff --git a/nisse-dev/tests/test_loop_properties.py b/nisse-dev/tests/test_loop_properties.py new file mode 100644 index 0000000..79e055a --- /dev/null +++ b/nisse-dev/tests/test_loop_properties.py @@ -0,0 +1,247 @@ +"""specs/loop.qnt's properties, checked against the real loop (M2.1, PR D). + +The Quint model checks the design; this checks the code. A seeded random +walk interleaves what the operator and Gitea can do (go-aheads, answers, +comments, merges, closes, abandon, a crash and restart) with an adaptive +scripted model, and checks every property after every step. A failure prints +the seed and the steps, so it replays exactly. +""" + +import random +import subprocess +from pathlib import Path +from typing import Any + +import pytest +from test_loop_milestones import DON, Forge, call +from test_loop_milestones import world as world # noqa: PLC0414 -- the fixture + +from nisse_dev.cancel import Cancel +from nisse_dev.canonical import Fact, ToolCall +from nisse_dev.gitea import Comment +from nisse_dev.loop.agent import LoopAgent +from nisse_dev.loop.driver import Driver +from nisse_dev.loop.items import ANSWER, GO_AHEAD, MERGE, WAITING, loop_of +from nisse_dev.oplog import OpLog +from nisse_dev.tools import Workspace +from nisse_dev.transport import InboundMessage, InMemoryTransport +from nisse_dev.worker import InlineWorker + +SEEDS = range(32) +STEPS = 20 + + +class PolicyModel: + """Plays a plausible driver: restate first, then a milestone, fixes + after comments, close after a merge; sometimes asks a question or + replies in prose. Seeded, so a walk replays exactly.""" + + def __init__(self, rng: random.Random) -> None: + self.rng = rng + self.n = 0 + + def complete(self, events, tools, model, system=None) -> list[Any]: + self.n += 1 + offered = {t["function"]["name"] for t in tools} + facts = [getattr(e.event, "text", "") for e in events] + roll = self.rng.random() + if roll < 0.08: + return [call("ask_human", question=f"question {self.n}?")] + if roll < 0.12: + return [ToolCall(call_id=f"p{self.n}", tool="nonsense", arguments={})] + if "finish_milestone" not in offered: + return [call("restate", text=f"restatement {self.n}")] + merged = any("merged" in t for t in facts[-6:]) + if merged and self.rng.random() < 0.8: + return [call("close_item", summary=f"done {self.n}")] + return [ + call("start_milestone", name=f"m{self.n}"), + call("write_file", path=f"f{self.n}.py", content=f"x = {self.n}\n"), + call("finish_milestone", summary=f"work {self.n}"), + ] + + +class Walk: + def __init__(self, world: dict[str, Any], seed: int) -> None: + self.world = world + self.rng = random.Random(seed) + self.seed = seed + self.steps: list[str] = [] + self.shown_at_go: list[int] = [] # the revision on screen at each go + self.agent, self.transport = self._agent() + + def _agent(self) -> tuple[LoopAgent, InMemoryTransport]: + transport = InMemoryTransport() + cancel = Cancel() + w = self.world + agent = LoopAgent( + w["store"], + transport, + w["forge"], + repo="don/sandbox", + project="sandbox", + account="nisse-dev", + operators={DON}, + gitea_operators={"don"}, + driver=Driver(PolicyModel(self.rng), "m", cancel=cancel), # type: ignore[arg-type] + workspace=Workspace(w["host"], w["repo"].root, timeout_s=30), + worker=InlineWorker(), + cancel=cancel, + oplog=OpLog("nisse-dev", _Null()), + milestones=w["milestones"], + ) + transport.on_message(agent.on_message) + return agent, transport + + def items(self) -> list[Any]: + return self.world["store"].work_items() + + def step(self) -> None: + forge: Forge = self.world["forge"] + open_items = [i for i in self.items() if i.status == "open"] + # Weighted towards what an item can actually do, so review paths + # are walked often, not drowned out by intake and crashes. + weights = {"intake": 2 if not open_items else 1, "crash": 1} + for item in open_items: + loop = loop_of(item) + weights.update({"go": 3, "answer": 2, "abandon": 1, "go-stale": 1}) + if loop.pr: + weights.update( + {"comment": 4, "merge": 2, "close-pr": 1, "comment-merge": 2} + ) + action = self.rng.choices(list(weights), list(weights.values()))[0] + self.steps.append(action) + item = open_items[0] if open_items else None + loop = loop_of(item) if item else None + if action == "intake": + self.agent.intake() + elif action == "crash": + # Volatile state is lost: a new agent over the same store. + self.agent, self.transport = self._agent() + self.agent.boot() + self.agent._schedule() + elif action in ("go", "answer", "abandon") and loop: + text = {"go": "go", "answer": "an answer", "abandon": "abandon"}[action] + if action == "go" and item.position == WAITING and loop.wait == GO_AHEAD: + self.shown_at_go.append(loop.rev) + self.transport.receive(DON, text, thread=loop.thread) + elif action == "go-stale" and loop and loop.rev > 1: + # A go sent while an older revision was on screen. + self.agent.handle( + InboundMessage(DON, "go", thread=loop.thread), about=loop.rev - 1 + ) + elif action == "comment" and loop: + forge.comments.setdefault(loop.pr, []).append( + Comment(len(self.steps) * 10, "don", f"comment {len(self.steps)}", "t") + ) + self.agent.watch() + self.agent._schedule() + elif ( + action == "comment-merge" and loop and forge.prs[loop.pr]["state"] == "open" + ): + # The race: a comment is seen, then the PR merges before the + # driver's fix lands. + forge.comments.setdefault(loop.pr, []).append( + Comment(len(self.steps) * 10, "don", f"comment {len(self.steps)}", "t") + ) + self.agent.watch() + forge.merge(loop.pr) + self.agent._schedule() + elif action == "merge" and loop and forge.prs[loop.pr]["state"] == "open": + forge.merge(loop.pr) + self.agent.watch() + self.agent._schedule() + elif action == "close-pr" and loop and forge.prs[loop.pr]["state"] == "open": + forge.prs[loop.pr]["state"] = "closed" + self.agent.watch() + self.agent._schedule() + + # -- the properties (specs/loop.qnt) ------------------------------------- + + def check(self) -> None: + where = f"seed {self.seed}, steps {self.steps}" + store, forge = self.world["store"], self.world["forge"] + items = self.items() + issues = [loop_of(i).issue for i in items if loop_of(i).issue] + assert len(issues) == len(set(issues)), f"one_item_per_issue: {where}" + for item in items: + loop = loop_of(item) + if item.status == "open" and item.position == WAITING: + explained = ( + (loop.wait == GO_AHEAD and loop.rev > 0) + or (loop.wait == ANSWER and bool(loop.note)) + or (loop.wait == MERGE and loop.pr > 0) + ) + assert explained, ( + f"waits_are_explainable: {item.position} {loop}: {where}" + ) + gos = [ + int(f.event.text.split("rev-")[1].split()[0]) + for f in store.transcript(item.id, "orchestrator").read() + if isinstance(f.event, Fact) + and f.event.text.startswith("go-ahead on rev-") + ] + assert all(rev in self.shown_at_go for rev in gos), ( + f"go_binds: {gos}: {where}" + ) + mine = [p for p in forge.prs.values() if p["head"].startswith(item.id)] + assert sum(p["state"] == "open" for p in mine) <= 1, ( + f"milestone_after_merge: {where}" + ) + if item.status == "closed": + assert not any(p["state"] == "open" for p in mine), ( + f"closed_items_leave_nothing (PR): {where}" + ) + assert not [ + i for i in store.pending_intents() if i.work_item == item.id + ], f"closed_items_leave_nothing (intent): {where}" + heads = [p["head"] for p in forge.prs.values()] + assert len(heads) == len(set(heads)), f"one_pr_per_milestone: {where}" + bare = self.world["bare"] + for n, pr in forge.prs.items(): + if not pr["merged"]: + continue + head = _rev(bare, f"refs/heads/{pr['head']}") + # A fix pushed after the merge must reach the operator as a PR + # (loop.qnt's lostFix): in main, or in a later PR, whatever they + # then decide about it. + landed = _is_ancestor(bare, head, "refs/heads/main") or any( + _is_ancestor(bare, head, f"refs/heads/{other['head']}") + for m, other in forge.prs.items() + if m > n + ) + assert landed, f"review_fixes_land (PR {n}): {where}" + + +class _Null: + def write(self, _: str) -> None: ... + + def flush(self) -> None: ... + + +def _rev(bare: Path, ref: str) -> str: + return subprocess.run( + ["git", "--git-dir", str(bare), "rev-parse", ref], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + +def _is_ancestor(bare: Path, commit: str, ref: str) -> bool: + return ( + subprocess.run( + ["git", "--git-dir", str(bare), "merge-base", "--is-ancestor", commit, ref], + capture_output=True, + check=False, + ).returncode + == 0 + ) + + +@pytest.mark.parametrize("seed", SEEDS) +def test_the_loop_keeps_its_properties(world: dict[str, Any], seed: int) -> None: + walk = Walk(world, seed) + for _ in range(STEPS): + walk.step() + walk.check() diff --git a/specs/README.md b/specs/README.md index 28bd402..5b071e5 100644 --- a/specs/README.md +++ b/specs/README.md @@ -65,6 +65,13 @@ Traces need about 80 steps to reach second milestones. The `w_*` values at the end are reachability witnesses: each should be *violated*, which shows the state it names is reachable. +**Against the real code.** `nisse-dev/tests/test_loop_properties.py` checks +the same properties against the new loop itself: a seeded random walk over +what the operator and Gitea can do (go-aheads, answers, comments, merges, +closes, abandon, a crash and restart), checking every property after every +step. It found two bugs the design model couldn't, because they were in the +code: abandon left an open PR, and a review fix after a follow-up PR went to +the merged branch. Each also has its own regression test. + **Keeping it true.** When the architecture changes a rule this models, change -the model in the same PR. When the new loop is built (Development Plan Phase 2), -each property gets a test against the real code, as `orchestrator.qnt`'s did. +the model and the walk in the same PR.