diff --git a/docs/Architecture.md b/docs/Architecture.md index 3047c80..cf89b26 100644 --- a/docs/Architecture.md +++ b/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 - fresh and this one resumes from its branch. +- **Parking pushes the item's work:** its branch, and anything uncommitted to a side + 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 diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index add101d..a64fad0 100644 --- a/docs/Development-Plan.md +++ b/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 -repository, `ARCHITECTURE.md` kept current, waiting for merges between milestones -(D2, D4). End-to-end: a two-milestone feature. +**M2.4 Multi-milestone work and documents.** *Pre-approved.* **Done** (this PR). +Build plans in the repository, `ARCHITECTURE.md` kept current, waiting for merges +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 `-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 diff --git a/nisse-dev/src/nisse_dev/git.py b/nisse-dev/src/nisse_dev/git.py index ed8cb04..8808435 100644 --- a/nisse-dev/src/nisse_dev/git.py +++ b/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: - self._git("checkout", "-q", "-b", name) + def checkout_new_branch(self, name: str, start: str | None = None) -> None: + """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. diff --git a/nisse-dev/src/nisse_dev/loop/agent.py b/nisse-dev/src/nisse_dev/loop/agent.py index 56d9d4f..0669a9a 100644 --- a/nisse-dev/src/nisse_dev/loop/agent.py +++ b/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 -changing anything (never create or switch branches yourself), make the -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 -comments arrive as messages: make the changes within the same milestone (don't -start another), run the tests, and call finish_milestone again to push the fix. When the PR has merged, call -close_item with what was done, what was verified, and what is left open. If -the PR is closed without merging, ask the operator what next. 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 project's documents are in its repository: read ARCHITECTURE.md and any +build plans under docs/plans/ before you restate. + +After the go-ahead, work in milestones, one PR each. Call start_milestone +before changing anything (never create or switch branches yourself; each +milestone starts from the latest default branch), make the change, run the +project's tests, then call finish_milestone with a summary, saying whether it +is the final milestone. The harness commits, pushes and opens a PR, and you +wait for review. Review comments arrive as messages: make the changes within +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]}" - self._milestones.start(branch) + 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]}" - self._milestones.start(branch) + 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}") diff --git a/nisse-dev/src/nisse_dev/loop/milestones.py b/nisse-dev/src/nisse_dev/loop/milestones.py index 67d1a44..496cf0a 100644 --- a/nisse-dev/src/nisse_dev/loop/milestones.py +++ b/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 - on it left, and push its branch, so nothing of a parked item lives - only on the build VM. Only a harness branch of an open item (one - starting with a ``keep`` prefix) is parked; returns its name.""" + """Before another item uses the workspace: nothing of the item on it + may live only on the build VM. Its uncommitted work is stashed and + pushed to ``-parked``, never committed onto its branch, which + 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, ``-``) 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") - if git.remote_sha(self._remote, branch, credential=self._credential) != ( - git.head_sha() + sha = git.stash_all(f"parked from {branch}") + if sha is not None: + 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: - """Back to a parked item: park whatever is on the workspace now, then - check out the item's branch. The local branch wins when it exists (it - can only be ahead: parking pushed it); otherwise it comes from origin. - A workspace already on one of the item's own branches (``mine``, a - prefix) is where it left off, and stays.""" + def resume(self, branch: str, *, keep: tuple[str, ...], mine: str) -> None: + """Back to a parked item: park whatever is on the workspace now, + check out the item's branch, and put its parked work back. The local + branch wins when it exists (it can only be ahead: parking pushed it); + otherwise it comes from origin. A workspace already on one of the + item's own branches (``mine``, its ``-`` 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): + 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) + 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 - self.park(keep=keep) - if branch in git.local_branches(): - git.checkout_existing_branch(branch) - git.reset_to(branch, branch) - else: + if not git.has_commit(sha): git.fetch(credential=self._credential) - git.reset_to(branch, f"origin/{branch}") + 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.""" - self._git.checkout_new_branch(branch) + """Cut the milestone's branch from the latest default branch (D4): + 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] diff --git a/nisse-dev/src/nisse_dev/loop/tools.py b/nisse-dev/src/nisse_dev/loop/tools.py index 79f26b8..86815af 100644 --- a/nisse-dev/src/nisse_dev/loop/tools.py +++ b/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", diff --git a/nisse-dev/tests/test_loop_multi.py b/nisse-dev/tests/test_loop_multi.py new file mode 100644 index 0000000..b412d4e --- /dev/null +++ b/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"] diff --git a/nisse-dev/tests/test_loop_properties.py b/nisse-dev/tests/test_loop_properties.py index 0d8c5a3..d47930f 100644 --- a/nisse-dev/tests/test_loop_properties.py +++ b/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}" diff --git a/nisse-dev/tests/test_loop_queue.py b/nisse-dev/tests/test_loop_queue.py index a398b5a..69a723a 100644 --- a/nisse-dev/tests/test_loop_queue.py +++ b/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( - "--git-dir", str(world["bare"]), "show", f"refs/heads/{b_branch}:lines.py" - ) - assert pushed == "L = 1" + bare = ["--git-dir", str(world["bare"])] + parked = f"refs/heads/{item(world, 15).id}-parked" + assert git(*bare, "show", f"{parked}^3:lines.py") == "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"] diff --git a/tests/integration/test_end_to_end_loop.py b/tests/integration/test_end_to_end_loop.py index 0d941fe..91c6c0f 100644 --- a/tests/integration/test_end_to_end_loop.py +++ b/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)