Adopt an already-published PR instead of publishing again
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 1m12s
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 1m12s
Fixes counterexample 5 from specs/orchestrator.qnt. A crash after the PR was opened but before PR_OPEN was recorded left the agent in EXECUTING; the restart question's 'continue' and 'start' then published again, Gitea refused a second PR, and there was no way back to PR_OPEN. On the live run the executor model happened to notice the existing work; recovery must not depend on that. - Boot (A3 step 4): in EXECUTING, if Gitea shows the milestone's PR open, adopt it, watch it from the start, and resume in PR_OPEN with a notice. Gitea unreachable or the PR closed: the restart question, as before. - MilestonePublisher.publish is idempotent: after pushing, an open PR for the branch is adopted (and so updated) rather than a second one opened. This also covers a PR opened whose response was lost. - The model adopts too, gains the lost-response outcome, and now counts PRs opened per milestone (one_pr_per_milestone was a flag the fixed model could never set). A mutant without adoption fails the new scenario; the properties held over 200,000 traces each. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPQ14L3LikyefCMFBqEbKE
This commit is contained in:
1 parent
6fdec6922b
commit
8bd1ac0fa2
6 files changed
+165
-24
No files matched your search
@@ -130,6 +130,8 @@ class MilestonePublisher(Protocol):
|
||||
|
||||
def close_pull_request(self, index: int, note: str) -> None: ...
|
||||
|
||||
def published_pull_request(self, task_id: str, milestone: int) -> Any: ...
|
||||
|
||||
|
||||
class PlanBoard(Protocol):
|
||||
"""The Gitea side of plan gating; the concrete one is in
|
||||
@@ -323,6 +325,27 @@ class Orchestrator:
|
||||
"restarted while waiting on you: "
|
||||
+ pending.get("prompt", "my last question still stands")
|
||||
)
|
||||
elif state.state == EXECUTING and (published := self._published_pr()):
|
||||
# A3 step 4: Gitea shows this milestone's PR open, so it was
|
||||
# published and only PR_OPEN went unrecorded. Adopt the PR rather
|
||||
# than ask, and never publish it again. Nothing watched it yet,
|
||||
# so every comment on it is new.
|
||||
milestone = int(self._data.get("milestone", 1))
|
||||
self._data["pr"] = {
|
||||
"number": published.number,
|
||||
"url": published.url,
|
||||
"milestone": milestone,
|
||||
"last_comment": self._watch(
|
||||
"review", published.number, self.handle_review_comment, after=0
|
||||
),
|
||||
}
|
||||
self._unwatch("plan")
|
||||
self._transition(PR_OPEN, cause=f"boot: milestone {milestone} published")
|
||||
self._transport.send_message(
|
||||
f"restarted after milestone {milestone} was published: PR "
|
||||
f"#{published.number} is open ({published.url}); watching it for "
|
||||
"review comments"
|
||||
)
|
||||
elif state.state == PR_OPEN:
|
||||
# Waiting on review is a committed state like a gate (A3 step
|
||||
# 8): resume, and watch the PR again from its cursor.
|
||||
@@ -980,6 +1003,20 @@ class Orchestrator:
|
||||
f"task on {project} abandoned{closed}. Say 'use <project>' to start another"
|
||||
)
|
||||
|
||||
def _published_pr(self) -> Any:
|
||||
"""The open PR for the current milestone, if Gitea shows one; None
|
||||
when there is none or Gitea cannot say (then the restart question
|
||||
is asked as before)."""
|
||||
task_id = self._data.get("task_id")
|
||||
if self._publisher is None or not task_id:
|
||||
return None
|
||||
try:
|
||||
return self._publisher.published_pull_request(
|
||||
str(task_id), int(self._data.get("milestone", 1))
|
||||
)
|
||||
except GiteaError:
|
||||
return None
|
||||
|
||||
def _return_to_pr_open(self, cause: str) -> None:
|
||||
self._transition(PR_OPEN, cause=cause)
|
||||
pr = self._data.get("pr") or {}
|
||||
|
||||
@@ -84,6 +84,19 @@ class MilestonePublisher:
|
||||
cause=f"publishing milestone {milestone}",
|
||||
)
|
||||
self._git.push(self._remote, branch, token=self._token)
|
||||
# Idempotent: if a PR for this branch is already open (a crash after
|
||||
# opening it, or a response lost on the way back), the push above
|
||||
# has updated it; adopt it rather than fail on a second.
|
||||
existing = self.published_pull_request(task_id, milestone)
|
||||
if existing is not None:
|
||||
self._store.commit_intent(
|
||||
intent_id, cause=f"adopted open PR #{existing.number}"
|
||||
)
|
||||
return PublishResult(
|
||||
existing,
|
||||
f"PR #{existing.number} was already open for this milestone; "
|
||||
f"pushed to it: {existing.url}",
|
||||
)
|
||||
pr = self._client.open_pull_request(
|
||||
self._repo,
|
||||
head=branch,
|
||||
@@ -94,6 +107,17 @@ class MilestonePublisher:
|
||||
self._store.commit_intent(intent_id, cause=f"opened PR #{pr.number}")
|
||||
return PublishResult(pr, f"opened PR #{pr.number}: {pr.url}")
|
||||
|
||||
def published_pull_request(
|
||||
self, task_id: str, milestone: int
|
||||
) -> PullRequest | None:
|
||||
"""The open PR for a milestone's branch, if there is one. A closed one
|
||||
does not count: the operator closed it, so a new one may be opened.
|
||||
Raises GiteaError when Gitea cannot say."""
|
||||
pr = self._client.find_pull_request(
|
||||
self._repo, self.branch_for(task_id, milestone)
|
||||
)
|
||||
return pr if pr is not None and pr.state == "open" else None
|
||||
|
||||
def comment(self, index: int, body: str) -> None:
|
||||
"""Reply on the PR. Courtesy only — the pushed commit is the real
|
||||
answer — so a failure here must not derail the review loop."""
|
||||
|
||||
@@ -16,6 +16,7 @@ class FakeGitea:
|
||||
def __init__(self, *, fail: bool = False) -> None:
|
||||
self.opened: list[dict[str, Any]] = []
|
||||
self.existing: list[str] = []
|
||||
self.closed: set[str] = set()
|
||||
self._fail = fail
|
||||
|
||||
def open_pull_request(
|
||||
@@ -33,7 +34,8 @@ class FakeGitea:
|
||||
|
||||
def find_pull_request(self, repo: str, head: str) -> PullRequest | None:
|
||||
if head in self.existing:
|
||||
return PullRequest(number=1, state="open", head=head, url="u")
|
||||
state = "closed" if head in self.closed else "open"
|
||||
return PullRequest(number=1, state=state, head=head, url="u")
|
||||
return None
|
||||
|
||||
|
||||
@@ -212,3 +214,32 @@ def test_a_followup_the_executor_committed_itself_is_pushed(
|
||||
# Nothing new since that push: no second push, and it says so.
|
||||
assert publisher.push_followup(task_id="t", milestone=1, message="m") is None
|
||||
assert len(git.pushed) == 2
|
||||
|
||||
|
||||
def test_publishing_again_adopts_the_open_pr_instead_of_opening_another(
|
||||
store: StateStore, tmp_path: Path
|
||||
) -> None:
|
||||
# E.g. Gitea opened the PR but the response was lost, and the milestone
|
||||
# was published again: the PR is adopted and updated, never duplicated.
|
||||
git = FakeGit(tmp_path / "work")
|
||||
client = FakeGitea()
|
||||
publisher = build(store, git, client)
|
||||
publisher.publish(task_id="t", milestone=1, project="demo", plan="p")
|
||||
(tmp_path / "work" / "more.txt").write_text("more")
|
||||
result = publisher.publish(task_id="t", milestone=1, project="demo", plan="p")
|
||||
assert len(client.opened) == 1
|
||||
assert result.pull_request is not None and result.pull_request.number == 1
|
||||
assert "already open" in result.note
|
||||
assert git.pushed == ["t/milestone-1", "t/milestone-1"] # updated it
|
||||
assert store.pending_intents() == []
|
||||
|
||||
|
||||
def test_a_closed_pr_is_not_adopted(store: StateStore, tmp_path: Path) -> None:
|
||||
git = FakeGit(tmp_path / "work")
|
||||
client = FakeGitea()
|
||||
client.existing.append("t/milestone-1")
|
||||
client.closed.add("t/milestone-1")
|
||||
publisher = build(store, git, client)
|
||||
assert publisher.published_pull_request("t", 1) is None
|
||||
publisher.publish(task_id="t", milestone=1, project="demo", plan="p")
|
||||
assert len(client.opened) == 1 # the operator closed it; a new one opens
|
||||
@@ -134,6 +134,12 @@ class Publisher:
|
||||
PullRequest(number=20, state="open", head=branch, url="u"), "opened PR #20"
|
||||
)
|
||||
|
||||
def published_pull_request(self, task_id: str, milestone: int) -> Any:
|
||||
branch = f"{task_id}/milestone-{milestone}"
|
||||
if branch in self._gitea.prs:
|
||||
return PullRequest(number=20, state="open", head=branch, url="u")
|
||||
return None
|
||||
|
||||
def push_followup(self, **kwargs: Any) -> str | None:
|
||||
return "abc12345"
|
||||
|
||||
@@ -249,7 +255,6 @@ def test_an_afterthought_rejects_the_next_plan(tmp_path: Path) -> None:
|
||||
assert agent.gate().get("rev") == 2 # arch was rev-1; not re-planned
|
||||
|
||||
|
||||
@XFAIL
|
||||
def test_crash_after_opening_the_pr_publishes_again(tmp_path: Path) -> None:
|
||||
"""crashAfterOpeningThePrPublishesAgainTest"""
|
||||
gitea = FakeGitea()
|
||||
@@ -261,9 +266,11 @@ def test_crash_after_opening_the_pr_publishes_again(tmp_path: Path) -> None:
|
||||
assert agent.state == EXECUTING # the PR exists; PR_OPEN was never recorded
|
||||
|
||||
agent = Agent(tmp_path, gitea) # restart: the intent is verified
|
||||
agent.say("continue", "start milestone 1")
|
||||
# There is a PR for this milestone: the agent must end up watching it,
|
||||
# not trying to open another.
|
||||
# There is a PR for this milestone: the agent adopts it on boot and ends
|
||||
# up watching it, never trying to open another.
|
||||
assert agent.state == PR_OPEN
|
||||
assert "PR #20 is open" in agent.transport.outbound[-1].text
|
||||
agent.say("continue", "start milestone 1") # no longer means anything
|
||||
assert agent.publisher.attempts == 0
|
||||
assert agent.state == PR_OPEN
|
||||
|
||||
@@ -321,3 +328,22 @@ def test_the_harness_reaches_pr_open_without_faults(tmp_path: Path) -> None:
|
||||
assert agent.state == PR_OPEN
|
||||
assert agent.publisher.attempts == 1
|
||||
_ = (AWAIT_INPUT, MILESTONE_PLANNING)
|
||||
|
||||
|
||||
def test_boot_asks_as_before_when_gitea_cannot_say(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
gitea = FakeGitea()
|
||||
agent = Agent(tmp_path, gitea)
|
||||
agent.say("use demo", "approve", "approve")
|
||||
agent.publisher.crash_after_opening = True
|
||||
with pytest.raises(SystemExit):
|
||||
agent.say("start milestone 1")
|
||||
|
||||
def unreachable(self: Publisher, task_id: str, milestone: int) -> Any:
|
||||
raise GiteaError("gitea is down")
|
||||
|
||||
monkeypatch.setattr(Publisher, "published_pull_request", unreachable)
|
||||
agent = Agent(tmp_path, gitea)
|
||||
assert agent.state == AWAIT_INPUT
|
||||
assert "restarted mid-EXECUTING" in agent.transport.outbound[-1].text
|
||||
Reference in new issue
Block a user