Merge pull request 'Multi-milestone work and documents on the new loop (M2.4)' (#103) from loop-milestones into main

docs/Architecture.md

@@ -670,8 +670,9 @@ Installing on any other host is dangerous.
   item ready (D5); a PR closed without merging makes it ready with that fact, and the
   agent asks what next. Redoing the milestone gets a new branch and PR, and the
   merge rule still applies to it.
-- **Parking pushes the item's branch,** so another item on the same project starts
+- **Parking pushes the item's work:** its branch, and anything uncommitted to a side
-  fresh and this one resumes from its branch.
+  branch of its own, never onto a PR's branch, which may have merged meanwhile.
+  Another item on the same project starts fresh, and this one resumes where it was.
 - **Publish only the work:** what the agent changed, counted from where the
   milestone started, never build artifacts; anything left out is reported.
 - **Pushes run on the build VM** with a push credential (a deploy key, or a

docs/Development-Plan.md

@@ -167,9 +167,22 @@ labelled and not.
   operator has spoken; `design` is steered by the driver's instructions.
 - Labels are read at intake; a label changed later is not picked up yet.
 
-**M2.4 Multi-milestone work and documents.** *Pre-approved.* Build plans in the
+**M2.4 Multi-milestone work and documents.** *Pre-approved.* **Done** (this PR).
-repository, `ARCHITECTURE.md` kept current, waiting for merges between milestones
+Build plans in the repository, `ARCHITECTURE.md` kept current, waiting for merges
-(D2, D4). End-to-end: a two-milestone feature.
+between milestones (D2, D4). End-to-end: a two-milestone feature.
+- Every milestone branch is cut from the latest default branch, so the next one
+  builds on the merged previous and a redo drops a closed PR's work. While the
+  item's PR is open, `start_milestone` is refused, the PR's state read from Gitea
+  at that moment. `finish_milestone(final)`: earlier PRs say "Part of #N", the
+  last "Closes #N".
+- `close_item` with the item's PR still open waits for the merge.
+- Documents are the driver's instructions only; the harness never parses a plan.
+- Parking stashes uncommitted work and pushes it to `<item>-parked`, put back on
+  resume, instead of committing it onto the item's branch: the walk found a
+  half-done review fix parked onto a PR that had merged meanwhile, where it was
+  lost.
+- Deferred: a milestone with no changes that wasn't expected to change code is
+  still raised with the operator (D4).
 
 **M2.5 Conversations and new projects.** *Needs discussion.* Work started in
 conversation, record issues carrying their item's ID, the new-project flow and its

nisse-dev/src/nisse_dev/git.py

@@ -210,8 +210,10 @@ class GitRepo:
         self._git("config", "user.name", name)
         self._git("config", "user.email", email)
 
-    def checkout_new_branch(self, name: str) -> None:
+    def checkout_new_branch(self, name: str, start: str | None = None) -> None:
-        self._git("checkout", "-q", "-b", name)
+        """A new branch, at ``start`` if given. Uncommitted changes come
+        along; git refuses if one would conflict with ``start``."""
+        self._git("checkout", "-q", "-b", name, *([start] if start else []))
 
     def checkout_existing_branch(self, name: str) -> None:
         self._git("checkout", "-q", name)
@@ -300,6 +302,46 @@ class GitRepo:
             "push", "-q", remote_url, f"HEAD:refs/heads/{branch}", credential=credential
         )
 
+    def push_commit(
+        self,
+        remote_url: str,
+        sha: str,
+        branch: str,
+        *,
+        credential: PushCredential | None = None,
+    ) -> None:
+        """Point the remote's ``branch`` at ``sha``, whatever it was."""
+        self._git(
+            "push",
+            "-q",
+            "-f",
+            remote_url,
+            f"{sha}:refs/heads/{branch}",
+            credential=credential,
+        )
+
+    def stash_all(self, message: str) -> str | None:
+        """Take every uncommitted change, untracked files included and build
+        artifacts not, off the work tree as a stash commit; returns its sha,
+        or None when there was nothing. The commit is kept by pushing it."""
+        if not self.has_changes():
+            return None
+        self._git("stash", "push", "-q", "-u", "-m", message)
+        sha = self._git("rev-parse", "stash@{0}")
+        self._git("stash", "drop", "-q")
+        return sha
+
+    def apply_stash(self, sha: str) -> None:
+        """Put a stash commit's changes back on the work tree, uncommitted."""
+        self._git("stash", "apply", "-q", sha)
+
+    def has_commit(self, sha: str) -> bool:
+        try:
+            self._git("cat-file", "-e", f"{sha}^{{commit}}")
+        except GitError:
+            return False
+        return True
+
     def discard_branch(self, branch: str, *, back_to: str) -> None:
         """Throw away a branch and everything on it.
 

nisse-dev/src/nisse_dev/loop/agent.py

@@ -25,6 +25,7 @@ from dataclasses import replace
 from nisse_dev.cancel import Cancel
 from nisse_dev.canonical import Fact, PlanSubmitted, UserMessage
 from nisse_dev.gates import command_form
+from nisse_dev.git import GitError
 from nisse_dev.gitea import GiteaClient, GiteaError, Issue
 from nisse_dev.loop.driver import Driver, RunOutcome
 from nisse_dev.loop.items import (
@@ -81,16 +82,28 @@ the project, and your first step. Wait for their go-ahead. If the request is
 unclear, ask_human instead of guessing: a restatement asks no questions, so
 the operator can agree to it as it stands.
 
-After the go-ahead, work in a milestone: call start_milestone before
+The project's documents are in its repository: read ARCHITECTURE.md and any
-changing anything (never create or switch branches yourself), make the
+build plans under docs/plans/ before you restate.
-change, run the project's tests, then call finish_milestone with a summary.
+
-The harness commits, pushes and opens a PR, and you wait for review. Review
+After the go-ahead, work in milestones, one PR each. Call start_milestone
-comments arrive as messages: make the changes within the same milestone (don't
+before changing anything (never create or switch branches yourself; each
-start another), run the tests, and call finish_milestone again to push the fix. When the PR has merged, call
+milestone starts from the latest default branch), make the change, run the
-close_item with what was done, what was verified, and what is left open. If
+project's tests, then call finish_milestone with a summary, saying whether it
-the PR is closed without merging, ask the operator what next. When a review
+is the final milestone. The harness commits, pushes and opens a PR, and you
-needs no change from you, or you are only waiting for the operator to merge
+wait for review. Review comments arrive as messages: make the changes within
-(you can't merge it yourself), call wait_for_review.
+the same milestone (don't start another), run the tests, and call
+finish_milestone again to push the fix. When a review needs no change from
+you, or you are only waiting for the operator to merge (you can't merge it
+yourself), call wait_for_review. The next milestone starts only after the
+previous PR has merged. When the last PR has merged, call close_item with
+what was done, what was verified, and what is left open. If a PR is closed
+without merging, ask the operator what next.
+
+Work longer than about one PR gets a build plan: a Markdown file under
+docs/plans/ giving, per milestone, the goal, the interfaces and behaviour, the
+edge cases, and the tests that are its acceptance criteria. Commit it with the
+first milestone, and mark each milestone done in it in that milestone's PR.
+Keep ARCHITECTURE.md current: a design change updates it in the same PR.
 
 {labels}Never label the issue yourself; you may suggest a label to the operator.
 
@@ -808,6 +821,20 @@ class LoopAgent:
                 loop,
                 str(args.get("summary", "")).strip(),
                 str(args.get("title", "")).strip(),
+                # Unsaid, it's final: the PR closes the issue, as before.
+                str(args.get("final", "yes")).strip().lower() != "no",
+            )
+        elif outcome.kind == "wait" and outcome.tool == "close_item" and loop.pr:
+            # Closing leaves nothing open (L2): an open PR is waited out first.
+            self._fact(
+                item,
+                f"close_item waits: PR #{loop.pr} is still open; the item closes "
+                "after it merges",
+            )
+            self._wait(
+                item,
+                replace(loop, wait=MERGE, note=f"PR #{loop.pr}"),
+                f"I'll close this once PR #{loop.pr} merges: {loop.pr_url}",
             )
         elif outcome.kind == "wait" and outcome.tool == "close_item":
             summary = str(args.get("summary", "")).strip()
@@ -828,6 +855,23 @@ class LoopAgent:
         item = self._store.work_item(item_id)
         assert item is not None
         loop = loop_of(item)
+        if loop.pr:
+            # D4: the next milestone waits for the merge, read from Gitea now,
+            # not from the last watch round.
+            try:
+                pr = self._gitea.pull_request(self._repo, loop.pr)
+            except GiteaError as exc:
+                return f"couldn't check PR #{loop.pr}: {exc}; try again", False
+            if pr.state == "open":
+                wait = (
+                    f"milestone {loop.milestone}'s PR #{loop.pr} hasn't merged yet; "
+                    "the next milestone starts after it merges. If you're only "
+                    "waiting for that, call wait_for_review."
+                )
+                return wait, False
+            how = "merged" if pr.merged else "was closed without merging"
+            self._fact(item, f"PR #{loop.pr} {how}")
+            loop = replace(loop, pr=0, pr_url="", branch="")
         if loop.branch:
             busy = f"milestone {loop.milestone} is still open; finish it first"
             return busy, False
@@ -835,7 +879,10 @@ class LoopAgent:
             return "milestones aren't available here", False
         number = loop.milestone + 1
         branch = f"{item.id}-milestone-{number}-{uuid.uuid4().hex[:4]}"
+        try:
             self._milestones.start(branch)
+        except GitError as exc:
+            return f"couldn't start the milestone: {exc}", False
         name = str(args.get("name", "")).strip()
         self._fact(item, f"milestone {number} started ({name}) on branch {branch}")
         save(
@@ -869,7 +916,12 @@ class LoopAgent:
         )
 
     def _finish(
-        self, item: WorkItem, loop: Loop, summary: str, title: str = ""
+        self,
+        item: WorkItem,
+        loop: Loop,
+        summary: str,
+        title: str = "",
+        final: bool = True,
     ) -> None:
         """finish_milestone: publish the milestone, or push a review fix."""
         if self._milestones is None:
@@ -918,7 +970,12 @@ class LoopAgent:
             # D4: finishing without a start cuts the branch now.
             number = loop.milestone + 1
             branch = f"{item.id}-milestone-{number}-{uuid.uuid4().hex[:4]}"
+            try:
                 self._milestones.start(branch)
+            except GitError as exc:
+                note = f"I couldn't start milestone {number}: {exc}"
+                self._wait(item, replace(loop, wait=ANSWER, note=note), note)
+                return
             loop = replace(loop, milestone=number, branch=branch)
         published = self._milestones.publish(
             item_id=item.id,
@@ -926,6 +983,7 @@ class LoopAgent:
             title=title or f"{item.title} (milestone {loop.milestone})",
             summary=summary,
             issue=loop.issue,
+            final=final,
         )
         left = (
             f" (left out {len(published.left_out)} build artifact(s))"
@@ -984,6 +1042,11 @@ class LoopAgent:
                 )
         save(self._store, item.id, CLOSED, loop, cause=outcome)
         self._store.close_work_item(item.id, outcome=outcome, cause=note)
+        if self._milestones is not None:
+            try:
+                self._milestones.drop_parked(f"{item.id}-")
+            except GitError:
+                pass  # a leftover side branch; nothing depends on it
         if loop.issue:
             try:
                 self._gitea.post_comment(self._repo, loop.issue, f"{outcome}: {note}")

nisse-dev/src/nisse_dev/loop/milestones.py

@@ -91,38 +91,73 @@ class Milestones:
     # -- parking and resuming (L1, D4) -----------------------------------------
 
     def park(self, *, keep: tuple[str, ...]) -> str | None:
-        """Before another item uses the workspace: commit whatever the item
+        """Before another item uses the workspace: nothing of the item on it
-        on it left, and push its branch, so nothing of a parked item lives
+        may live only on the build VM. Its uncommitted work is stashed and
-        only on the build VM. Only a harness branch of an open item (one
+        pushed to ``<item>-parked``, never committed onto its branch, which
-        starting with a ``keep`` prefix) is parked; returns its name."""
+        may be a PR's that has merged meanwhile; resuming puts it back
+        uncommitted, for wherever the work goes next. Commits the harness
+        made but couldn't push are pushed. Only an open item's harness branch
+        (one starting with a ``keep`` prefix, ``<item>-``) is parked; returns
+        its name."""
         git = self._git
         branch = git.current_branch()
-        if not keep or not branch.startswith(keep):
+        prefix = next((k for k in keep if branch.startswith(k)), None)
+        if prefix is None:
             return None
-        git.commit_all("WIP: parked while another work item runs")
+        sha = git.stash_all(f"parked from {branch}")
-        if git.remote_sha(self._remote, branch, credential=self._credential) != (
+        if sha is not None:
-            git.head_sha()
+            git.push_commit(
+                self._remote, sha, f"{prefix}parked", credential=self._credential
+            )
+        remote = git.remote_sha(self._remote, branch, credential=self._credential)
+        if remote != git.head_sha() and (
+            remote is not None or git.commits_since(f"origin/{self._base}") > 0
         ):
             git.push(self._remote, branch, credential=self._credential)
         return branch
 
-    def resume(self, branch: str, *, keep: tuple[str, ...], mine: str = "") -> None:
+    def resume(self, branch: str, *, keep: tuple[str, ...], mine: str) -> None:
-        """Back to a parked item: park whatever is on the workspace now, then
+        """Back to a parked item: park whatever is on the workspace now,
-        check out the item's branch. The local branch wins when it exists (it
+        check out the item's branch, and put its parked work back. The local
-        can only be ahead: parking pushed it); otherwise it comes from origin.
+        branch wins when it exists (it can only be ahead: parking pushed it);
-        A workspace already on one of the item's own branches (``mine``, a
+        otherwise it comes from origin. A workspace already on one of the
-        prefix) is where it left off, and stays."""
+        item's own branches (``mine``, its ``<item>-`` prefix) stays."""
         git = self._git
         current = git.current_branch()
-        if current == branch or (mine and current.startswith(mine)):
+        if current != branch and not current.startswith(mine):
-            return
             self.park(keep=keep)
             if branch in git.local_branches():
                 git.checkout_existing_branch(branch)
                 git.reset_to(branch, branch)
             else:
                 git.fetch(credential=self._credential)
-            git.reset_to(branch, f"origin/{branch}")
+                pushed = git.remote_sha(
+                    self._remote, branch, credential=self._credential
+                )
+                # A branch never pushed held nothing beyond the default branch.
+                start = f"origin/{branch}" if pushed else f"origin/{self._base}"
+                git.reset_to(branch, start)
+        # Even when it stayed: a crash may have come between parking and the
+        # next item's start.
+        self.unpark(mine)
+
+    def unpark(self, mine: str) -> None:
+        parked = f"{mine}parked"
+        git = self._git
+        sha = git.remote_sha(self._remote, parked, credential=self._credential)
+        if sha is None:
+            return
+        if not git.has_commit(sha):
+            git.fetch(credential=self._credential)
+        git.apply_stash(sha)
+        git.delete_remote_branch(self._remote, parked, credential=self._credential)
+
+    def drop_parked(self, mine: str) -> None:
+        """A closed item's parked work, if any, goes: closing set it aside."""
+        parked = f"{mine}parked"
+        git = self._git
+        if git.remote_sha(self._remote, parked, credential=self._credential):
+            git.delete_remote_branch(self._remote, parked, credential=self._credential)
 
     def _ahead_of_origin(self) -> bool:
         current = self._git.current_branch()
@@ -136,8 +171,12 @@ class Milestones:
     # -- milestones (D4) -------------------------------------------------------
 
     def start(self, branch: str) -> str:
-        """Cut the milestone's branch where the item is; returns its start."""
+        """Cut the milestone's branch from the latest default branch (D4):
-        self._git.checkout_new_branch(branch)
+        a milestone after a merge builds on it, and one redone after a closed
+        PR doesn't carry the rejected work. Uncommitted work comes along.
+        Returns where it starts."""
+        self._git.fetch(credential=self._credential)
+        self._git.checkout_new_branch(branch, f"origin/{self._base}")
         return self._git.head_sha()
 
     def publish(
@@ -148,6 +187,7 @@ class Milestones:
         title: str,
         summary: str,
         issue: int,
+        final: bool = True,
     ) -> Published:
         git = self._git
         try:
@@ -184,7 +224,7 @@ class Milestones:
                 head=branch,
                 base=self._base,
                 title=title,
-                body=_body(summary, issue, left_out),
+                body=_body(summary, issue, left_out, final),
             )
             self._store.commit_intent(intent, cause=f"opened PR #{pr.number}")
             return Published("opened", f"opened PR #{pr.number}", pr, left_out)
@@ -227,10 +267,13 @@ class Milestones:
             return Fixed("failed", f"pushing the fix failed: {exc}")
 
 
-def _body(summary: str, issue: int, left_out: tuple[str, ...]) -> str:
+def _body(
+    summary: str, issue: int, left_out: tuple[str, ...], final: bool = True
+) -> str:
     parts = [summary]
     if issue:
-        parts += ["", f"Closes #{issue}"]
+        # D1: the last PR closes the issue on merge; earlier ones are part of it.
+        parts += ["", f"Closes #{issue}" if final else f"Part of #{issue}"]
     if left_out:
         parts += ["", "## Left out", "", "Build artifacts, never part of the work:"]
         parts += [f"- `{path}`" for path in left_out]

nisse-dev/src/nisse_dev/loop/tools.py

@@ -56,6 +56,7 @@ FINISH_MILESTONE = _tool(
     "reviewed or merged.",
     summary="What the milestone did and how it was verified, for the PR.",
     title="A short title for the PR (ignored when pushing a review fix).",
+    final="yes if this milestone completes the issue, no if more milestones follow it.",
 )
 WAIT_FOR_REVIEW = _tool(
     "wait_for_review",

nisse-dev/tests/test_loop_multi.py

@@ -0,0 +1,176 @@
+"""Multi-milestone work on the new loop (M2.4): each milestone starts from
+the latest default branch, the next waits for the previous PR's merge (read
+from Gitea when asked), earlier PRs are part of the issue and the last closes
+it, and the item closes only with nothing left open (D1, D2, D4). Real git on
+the throwaway build host; a fake Gitea."""
+
+from typing import Any
+
+from test_loop_milestones import (
+    RESTATE,
+    Forge,
+    ScriptedModel,
+    call,
+    git,
+    item,
+    loop_agent,
+    say,
+)
+from test_loop_milestones import world as world  # noqa: PLC0414 -- the fixture
+
+from nisse_dev.canonical import ToolCall, ToolResult
+from nisse_dev.loop.items import MERGE, WAITING, loop_of
+
+
+def milestone(path: str, final: str) -> list[Any]:
+    return [
+        call("start_milestone", name=path),
+        call("write_file", path=path, content=f"# {path}\n"),
+        call("finish_milestone", summary=f"adds {path}", title=path, final=final),
+    ]
+
+
+def tree(world: dict[str, Any], branch: str) -> list[str]:
+    bare = str(world["bare"])
+    return git("--git-dir", bare, "ls-tree", "--name-only", branch).split()
+
+
+def result_of(world: dict[str, Any], tool: str) -> str:
+    events = world["store"].transcript(item(world).id, "driver").read()
+    ids = {
+        e.event.call_id
+        for e in events
+        if isinstance(e.event, ToolCall) and e.event.tool == tool
+    }
+    results = [
+        e.event.result
+        for e in events
+        if isinstance(e.event, ToolResult) and e.event.call_id in ids
+    ]
+    return results[-1]
+
+
+def test_the_second_milestone_builds_on_the_merged_first(world: dict[str, Any]) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        milestone("a.py", final="no"),
+        milestone("b.py", final="yes"),
+        [call("close_item", summary="both done")],
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    [first] = forge.prs
+    assert forge.prs[first]["body"].endswith("Part of #14")
+    forge.merge(first)
+    agent.watch()
+    agent._schedule()
+
+    second = max(forge.prs)
+    assert second != first
+    assert "Closes #14" in forge.prs[second]["body"]
+    assert forge.files(second) == ["b.py"]  # only its own work
+    head = forge.prs[second]["head"]
+    assert "a.py" in tree(world, head)  # cut from main, with the first merged
+    assert loop_of(item(world)).milestone == 2
+
+    forge.merge(second)
+    agent.watch()
+    agent._schedule()
+    assert item(world).status == "closed"
+
+
+def test_the_next_milestone_waits_for_the_merge(world: dict[str, Any]) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        milestone("a.py", final="no"),
+        [  # a message arrives while the PR is open: try to move on
+            call("start_milestone", name="b"),
+            call("wait_for_review", note="waiting for the merge"),
+        ],
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    say(transport, world, "how's it going?")
+    assert "hasn't merged yet" in result_of(world, "start_milestone")
+    assert (item(world).position, loop_of(item(world)).wait) == (WAITING, MERGE)
+    assert len(forge.prs) == 1
+
+
+def test_a_merge_not_yet_watched_is_read_from_gitea(world: dict[str, Any]) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        milestone("a.py", final="no"),
+        milestone("b.py", final="yes"),
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    [first] = forge.prs
+    forge.merge(first)  # merged in Gitea; no watch round has seen it
+    say(transport, world, "merged, carry on")
+    assert len(forge.prs) == 2
+    assert "a.py" in tree(world, forge.prs[max(forge.prs)]["head"])
+
+
+def test_a_redo_after_a_closed_pr_drops_the_rejected_work(
+    world: dict[str, Any],
+) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        milestone("a.py", final="yes"),
+        milestone("c.py", final="yes"),
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    [first] = forge.prs
+    forge.prs[first]["state"] = "closed"  # rejected
+    agent.watch()
+    agent._schedule()
+    redo = max(forge.prs)
+    assert forge.files(redo) == ["c.py"]
+    assert "a.py" not in tree(world, forge.prs[redo]["head"])
+
+
+def test_closing_with_a_pr_open_waits_for_the_merge(world: dict[str, Any]) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        milestone("a.py", final="yes"),
+        [call("close_item", summary="done, I think")],  # too soon
+        [call("close_item", summary="done")],
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    say(transport, world, "thanks")
+    assert item(world).status == "open"
+    assert (item(world).position, loop_of(item(world)).wait) == (WAITING, MERGE)
+    [pr] = forge.prs
+    forge.merge(pr)
+    agent.watch()
+    agent._schedule()
+    assert item(world).status == "closed"
+
+
+def test_an_unsaid_final_closes_the_issue(world: dict[str, Any]) -> None:
+    forge: Forge = world["forge"]
+    model = ScriptedModel(
+        RESTATE,
+        [
+            call("start_milestone", name="a"),
+            call("write_file", path="a.py", content="x = 1\n"),
+            call("finish_milestone", summary="adds a"),
+        ],
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, "go")
+    [pr] = forge.prs
+    assert "Closes #14" in forge.prs[pr]["body"]

nisse-dev/tests/test_loop_properties.py

@@ -79,7 +79,11 @@ class PolicyModel:
         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}"),
+            call(
+                "finish_milestone",
+                summary=f"work {self.n}",
+                final=self.rng.choice(["yes", "no"]),
+            ),
         ]
 
 
@@ -308,6 +312,7 @@ class Walk:
             f"one_active (nothing left running): {where}"
         )
         self._parked_work_safe(where)
+        self._milestones_start_fresh(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"]
@@ -325,6 +330,29 @@ class Walk:
             )
             assert landed, f"review_fixes_land (PR {n}): {where}"
 
+    def _milestones_start_fresh(self, where: str) -> None:
+        """D4: a milestone starts from the latest default branch, so one
+        redone after a closed PR never carries the rejected work."""
+        forge: Forge = self.world["forge"]
+        bare = self.world["bare"]
+        for item in self.items():
+            mine = sorted(
+                (n, p)
+                for n, p in forge.prs.items()
+                if p["head"].startswith(f"{item.id}-milestone-")
+                and "-fix-" not in p["head"]
+            )
+            for i, (n, earlier) in enumerate(mine):
+                if earlier["merged"]:
+                    continue
+                rejected = _rev(bare, f"refs/heads/{earlier['head']}")
+                if _is_ancestor(bare, rejected, "refs/heads/main"):
+                    continue
+                for m, later in mine[i + 1 :]:
+                    assert not _is_ancestor(
+                        bare, rejected, f"refs/heads/{later['head']}"
+                    ), f"milestones_start_fresh: PR {m} carries PR {n}: {where}"
+
     def _parked_work_safe(self, where: str) -> None:
         """L1, D4: nothing of a parked item lives only on the build VM: what
         every branch of an open item that isn't on the workspace points at is
@@ -367,7 +395,17 @@ class Walk:
             for path in written:
                 if here and (root / path).exists():
                     continue
-                history = _git(bare, "log", "--all", "--format=%H", "--", path)
+                history = _git(
+                    # Full history: a parked stash keeps new files in its
+                    # third parent, which plain log simplifies away.
+                    bare,
+                    "log",
+                    "--all",
+                    "--full-history",
+                    "--format=%H",
+                    "--",
+                    path,
+                )
                 assert history, f"parked_work_safe: {path} of #{item.id} lost: {where}"
 
 

nisse-dev/tests/test_loop_queue.py

@@ -88,12 +88,14 @@ def test_switching_parks_the_work_and_resuming_finds_it(
     agent.watch()
     agent._schedule()
 
-    # Switching to 14 parked 15: its half-done work is committed and pushed.
+    # Switching to 14 parked 15: its half-done work is pushed as a stash to
+    # a side branch (new files in its third parent), never onto 15's branch.
     b_branch = loop_of(item(world, 15)).branch
-    pushed = git(
+    bare = ["--git-dir", str(world["bare"])]
-        "--git-dir", str(world["bare"]), "show", f"refs/heads/{b_branch}:lines.py"
+    parked = f"refs/heads/{item(world, 15).id}-parked"
-    )
+    assert git(*bare, "show", f"{parked}^3:lines.py") == "L = 1"
-    assert pushed == "L = 1"
+    # The branch has no commits of its own, so it wasn't pushed at all.
+    assert b_branch not in git(*bare, "branch", "--list")
     # 14's fix went to its own branch, without 15's work.
     assert loop_of(item(world, 14)).wait == MERGE
     files = git("--git-dir", str(world["bare"]), "ls-tree", "--name-only", a_branch)
@@ -105,6 +107,8 @@ def test_switching_parks_the_work_and_resuming_finds_it(
     say(transport, world, 15, "one per line")
     assert world["repo"].current_branch() == b_branch
     assert (world["repo"].root / "lines.py").read_text() == "L = 1\n"
+    # Put back, so the side branch is gone.
+    assert item(world, 15).id + "-parked" not in git(*bare, "branch", "--list")
     assert loop_of(item(world, 15)).wait == MERGE
     assert len(forge.prs) == 2
 
@@ -190,3 +194,52 @@ def test_status_shows_the_line(world: dict[str, Any]) -> None:
     assert "#15 Add count_lines: ready, 2 of 2 in line" in agent.status()
     agent.handle(InboundMessage(DON, "do #15 next"), None)
     assert "#15 Add count_lines: ready, 1 of 2 in line" in agent.status()
+
+
+def test_a_fix_half_done_when_its_pr_merges_survives_a_switch(
+    world: dict[str, Any],
+) -> None:
+    """Found by the walk: work on a review fix, parked after the PR merged,
+    was pushed onto the merged branch, where nobody would see it. Now it
+    comes back with the item and goes out in its next PR."""
+    forge: Forge = world["forge"]
+    two_issues(forge)
+    model = ScriptedModel(
+        [call("restate", text="count_words")],  # 14
+        [call("restate", text="count_lines")],  # 15
+        [  # 14: a PR
+            call("start_milestone", name="words"),
+            call("write_file", path="words.py", content="W = 1\n"),
+            call("finish_milestone", summary="words", final="yes"),
+        ],
+        [  # 14, a review comment: half a fix, then a question
+            call("write_file", path="fix.py", content="F = 1\n"),
+            call("ask_human", question="fix it in words.py too?"),
+        ],
+        [  # 15 runs meanwhile
+            call("start_milestone", name="lines"),
+            call("write_file", path="lines.py", content="L = 1\n"),
+            call("finish_milestone", summary="lines"),
+        ],
+        [call("finish_milestone", summary="the fix", final="yes")],  # 14 again
+    )
+    agent, transport = loop_agent(world, model)
+    agent.intake()
+    say(transport, world, 14, "go")
+    [pr] = forge.prs
+    forge.comments[pr] = [Comment(7, "don", "add fix.py", "t")]
+    agent.watch()
+    agent._schedule()
+    assert loop_of(item(world, 14)).wait == ANSWER
+    say(transport, world, 15, "go")  # parks 14's half-done fix
+    forge.merge(pr)
+    agent.watch()
+    agent._schedule()  # 14 resumes; its fix goes out as a new PR
+
+    bare = ["--git-dir", str(world["bare"])]
+    merged_head = forge.prs[pr]["head"]
+    assert "fix.py" not in git(*bare, "ls-tree", "--name-only", merged_head).split()
+    newest = max(
+        n for n, p in forge.prs.items() if p["head"].startswith(item(world, 14).id)
+    )
+    assert newest != pr and forge.files(newest) == ["fix.py"]

tests/integration/test_end_to_end_loop.py

@@ -425,3 +425,59 @@ def test_a_plain_yes_is_a_go_ahead(
             prs.append(pr)
     finally:
         operator.tidy(issues, prs)
+
+
+def test_a_two_milestone_feature(
+    running: tuple[RunningLoop, InMemoryTransport, str],
+) -> None:
+    """M2.4: work in two milestones. The first PR carries the build plan and
+    is part of the issue; the second starts from the merged first and closes
+    it; the item closes after both merge."""
+    agent, transport, run = running
+    operator = Operator(agent, transport)
+    gitea, repo = agent.gitea, agent.config.gitea_repo
+    as_operator = operator_gitea(agent.config.gitea_url)
+    issues: list[int] = []
+    prs: list[int] = []
+    try:
+        body = (
+            f"{task(run)}\n\nThen, separately, {dependent_task(run)[0].lower()}"
+            f"{dependent_task(run)[1:]}\n\nDo this in two milestones, one PR each: "
+            f"vowels_{run} first, then consonants_{run} after the first has merged. "
+            "Write a short build plan for it."
+        )
+        issues.append(as_operator.create_issue(repo, f"letters_{run}", body))
+        as_operator._request(
+            "PATCH",
+            f"/repos/{repo}/issues/{issues[0]}",
+            json={"assignees": [agent.account]},
+        )
+        operator.tick()
+
+        # --- milestone 1: the plan and vowels; part of the issue ---
+        item = operator.drive_until(MERGE, issues[0])
+        first = loop_of(item).pr
+        prs.append(first)
+        pr = gitea._request("GET", f"/repos/{repo}/pulls/{first}")
+        assert f"Part of #{issues[0]}" in pr["body"], pr["body"]
+        files = [
+            f["filename"]
+            for f in gitea._request("GET", f"/repos/{repo}/pulls/{first}/files")
+        ]
+        assert any(f.startswith("docs/plans/") for f in files), files
+        merge(as_operator, repo, first)
+        operator.tick()
+
+        # --- milestone 2: from the merged first; closes the issue ---
+        item = operator.drive_until(MERGE, issues[0])
+        second = loop_of(item).pr
+        assert second != first
+        prs.append(second)
+        pr = gitea._request("GET", f"/repos/{repo}/pulls/{second}")
+        assert f"Closes #{issues[0]}" in pr["body"], pr["body"]
+        diff = gitea._http.get(f"/repos/{repo}/pulls/{second}.diff").text
+        assert f"consonants_{run}" in diff
+        assert f"\n+def vowels_{run}" not in diff  # built on the merged first
+        operator.merge_and_close(issues[0])
+    finally:
+        operator.tidy(issues, prs)