diff --git a/src/agent_comms/head_review.py b/src/agent_comms/head_review.py index fec928b60e..3bda1a1f21 100644 --- a/src/agent_comms/head_review.py +++ b/src/agent_comms/head_review.py @@ -85,9 +85,9 @@ def among(cls, repositories: tuple[RepositoryName, ...], pr: int, head: str) -> raise RecordProblems(problems) return cls(held[0].repo, pr, head) - @classmethod - def at(cls, repo: RepositoryName, pr: int) -> OpenPullRequestHead: - """Read GitHub's head, its base branch and draft state in one request.""" + @staticmethod + def forge_read(repo: RepositoryName, pr: int) -> str: + """GitHub's head, base branch, draft state and state in one request.""" read = repo.rest( f"pulls/{pr}", "--jq", @@ -95,7 +95,11 @@ def at(cls, repo: RepositoryName, pr: int) -> OpenPullRequestHead: ) if read.returncode: raise ToolRefusedError(read.stderr.strip() or f"#{pr} could not be read.") - return OpenPullRequestHead.from_forge(repo, pr, read.stdout) + return read.stdout + + @classmethod + def at(cls, repo: RepositoryName, pr: int) -> OpenPullRequestHead: + return OpenPullRequestHead.from_forge(repo, pr, cls.forge_read(repo, pr)) def change_identity(self, reader: WorkBranches, base: str) -> PullRequestChange: """Compare the PR's patch, never the unrelated changes in its base tree. @@ -118,6 +122,18 @@ def same_pull_request(self, other: PullRequestHead) -> bool: def current(self) -> OpenPullRequestHead: """GitHub's read of the PR, refused unless the reviewed head is still its head.""" current = self.at(self.repo, self.pr) + if not current.holds(self.head): + raise ToolRefusedError( + f"#{self.pr} moved to {current.head[:9]} since {self.head} was reviewed; " + "review again." + ) + return current + + def closed_or_open(self) -> ReadPullRequestHead: + """GitHub's read in whatever state the PR is in, refused unless this is still its head: + a closed PR's head can only be dropped, so nothing here may arm or integrate it.""" + head, base, *_ = self.forge_read(self.repo, self.pr).split() + current = ReadPullRequestHead(self.repo, self.pr, head, base=base) if not current.holds(self.head): raise ToolRefusedError( f"#{self.pr} moved to {current.head[:9]} since {self.head} was reviewed; review again." @@ -768,7 +784,7 @@ def record_review( class PreparedHeadReview: """The current forge head and its checked status text, read once before recording.""" - current: OpenPullRequestHead + current: ReadPullRequestHead subject: str diff --git a/src/agent_comms/order_tools.py b/src/agent_comms/order_tools.py index ca07785d86..d1416fc574 100644 --- a/src/agent_comms/order_tools.py +++ b/src/agent_comms/order_tools.py @@ -227,7 +227,7 @@ def apply(self, comms: Comms, caller: ThreadCaller) -> HeadReviewed: actors.append(caller.head_review_actor(comms, self.request, order)) heads = [] with RecordProblems.collecting(problems, '--head', 'supply the current PR head SHA with --head'): - heads.append(self.request.current()) + heads.append(self.operation.current_head(self.request)) preparations = [] for current in heads: with RecordProblems.collecting(problems, self.operation.preparation_field, diff --git a/src/agent_comms/todos.py b/src/agent_comms/todos.py index 796d8fb2ba..3219b44a07 100644 --- a/src/agent_comms/todos.py +++ b/src/agent_comms/todos.py @@ -4273,6 +4273,10 @@ class HeadVerdict(HeadReview, declared_name=None): preparation_field: ClassVar[str] = '--head' preparation_fix: ClassVar[str] = 'supply the current PR head SHA with --head' + def current_head(self, request: PullRequestHead) -> ReadPullRequestHead: + """The read this decision stands on: an open PR at the reviewed head.""" + return request.current() + @classmethod def require_ok(cls, request: PullRequestHead) -> None: """The commander's admin merge needs this exact head's recorded ok, nothing more.""" @@ -4305,7 +4309,7 @@ def reviewed(self, goals: Goals, request: PullRequestHead, prepared: PreparedHea self.after_record(goals, recorded) return HeadReviewed(recorded, self.landing(prepared.current)) - def landing(self, current: OpenPullRequestHead) -> MergeArm: + def landing(self, current: ReadPullRequestHead) -> MergeArm: """Source verdicts never request merge; native explicit promotion owns enqueueing.""" return UntouchedMergeArm() @@ -4488,6 +4492,10 @@ class SupersededHeadVerdict(DecidedHeadVerdict): def supersedes(self) -> bool: return True + def current_head(self, request: PullRequestHead) -> ReadPullRequestHead: + """Dropping a head is how a closed PR's lost commits get their recorded reason.""" + return request.closed_or_open() + def description(self, reviewer: str, subject: str) -> str: return f"{super().description(reviewer, subject)} (replaced by {self.replacement})" diff --git a/tests/test_head_review.py b/tests/test_head_review.py index 107c511d0c..77f0cefa97 100644 --- a/tests/test_head_review.py +++ b/tests/test_head_review.py @@ -1413,3 +1413,16 @@ def test_another_commanders_linked_request_stays_off_the_orderless_board(head_bu held.generation).record(comms.goals, todo.id) assert ready.pr not in {rule.request.pr for rule in OrderlessPullRequest.scan_orderless(comms.goals, ProgressRead(()))} + + +def test_only_superseded_may_decide_a_closed_head(head_bus): + comms, github, request = head_bus + stamp = datetime.fromtimestamp(time.time() - 601, UTC).strftime('%Y-%m-%dT%H:%M:%SZ') + github.put(request.repo, ClosedListedPullRequest( + request.pr, 'feature', request.head, closed_at=stamp, updated=stamp, + base='main', reviewable_times=(stamp,))) + with pytest.raises(Exception, match='no longer an open source'): + invoke(comms, request, ReviewedHeadVerdict(ACCEPTED)) + invoke(comms, request, SupersededHeadVerdict(ACCEPTED, 'P2')) + reviewed, = StoredHeadReview.head_reviews(comms.goals.todos, request) + assert reviewed.verdict.supersedes()