From 673aed354ebfb1a0927a3bd0d3339a7eb22027ad Mon Sep 17 00:00:00 2001 From: Tristan Simas Date: Thu, 29 Oct 2026 20:40:54 -0400 Subject: [PATCH] Take back a merge recorded through a prerequisite pull request A merged order whose only merge came through a linked prerequisite PR (P935: #3596, 33bd05c64) could not change owner: MergedProblemOrder refuses transfer, and the check and run corrections keep the wrong merged state. CorrectMergedLinkTodo joins the CorrectOrderTodo family: the commander names the exact link and recorded merge commit, and the order is pending again under its own id, check, run, edits and times, with only that link removed. The correction contract now returns an OpenProblemOrder, so a correction may change the order's state instead of casting it to its own type. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RnH6dR85kyFKcc2tQAJLhK --- src/agent_comms/todos.py | 53 +++++++++++++++++++++++---- tests/test_holder_standing_rules.py | 57 ++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 8 deletions(-) diff --git a/src/agent_comms/todos.py b/src/agent_comms/todos.py index a3e7130b1f..133bfab2f2 100644 --- a/src/agent_comms/todos.py +++ b/src/agent_comms/todos.py @@ -663,6 +663,13 @@ def existing(cls, repo: RepositoryName, number: int) -> Self: return link +@dataclass(frozen=True) +class MergedLink: + """A pull request an order links and the merge commit the order recorded for it.""" + link: PullRequestLink + sha: str + + @dataclass(frozen=True) class PullRequest(HeadReviewStatus, DeclaredFamily, affix="PullRequest"): number: int @@ -1983,7 +1990,7 @@ def names(self, path: str) -> bool: class OrderCorrectionReason(PlainText): - """Why the recorded check or run was wrong, in enough words to stand on the record.""" + """Why the recorded part of an order was wrong, in enough words to stand on the record.""" refusal = ( "Say what was wrong with the recorded order in {}+ words; jargon {} hides what happened." @@ -2800,9 +2807,16 @@ def withdrawn(self, previous: Assignment, why: str, superseded_by: Successor) -> class OpenProblemOrder(CheckedProblemOrder, declared_name=None): """An active order keeps corrections and accepts its checked delivery by identity.""" - def corrected(self, correction: CorrectOrderTodo) -> Self: + def corrected(self, correction: CorrectOrderTodo) -> OpenProblemOrder: return correction.corrected(self) + def merged_link(self, link: PullRequestLink) -> MergedLink: + """The merge this order recorded through ``link``; an unmerged order recorded none.""" + raise TodoError(f"Order {self.id} records no merge to take back.") + + def unmerged(self, link: PullRequestLink) -> OpenProblemOrder: + raise TodoError(f"Order {self.id} records no merge to take back.") + def require_delivery(self, order_id: OrderId) -> None: if self.id != order_id: raise TodoError("The order changed; refresh its state before delivering it.") @@ -2845,6 +2859,16 @@ def require_linkable(self) -> None: raise TodoError(f"Order {self.id} merged as {self.sha[:9]}; its pull requests are closed. " "Ask the commander for a send-back before opening or linking another PR.") + def merged_link(self, link: PullRequestLink) -> MergedLink: + if link not in self.pull_requests: + raise TodoError(f"Order {self.id} does not link {link.url}; read it again.") + return MergedLink(link, self.sha) + + def unmerged(self, link: PullRequestLink) -> PendingProblemOrder: + """Pending again under its own id, check, run, edits and times, without ``link``.""" + return self.copy_as(PendingProblemOrder, pull_requests=tuple( + kept for kept in self.pull_requests if kept != link)) + def waiting_in(self, todo: Todo) -> Todo: return (todo if todo.order == self else replace(todo, order=self, revision=todo.revision + 1)) @@ -4974,7 +4998,7 @@ def changed(self, current: Todo) -> Todo: _text(self.reason, "Correction reason", 2000) return replace(current, order=current.order.corrected(self), revision=current.revision + 1) - def corrected[Order: OpenProblemOrder](self, order: Order) -> Order: + def corrected(self, order: OpenProblemOrder) -> OpenProblemOrder: if self.previous != self.part(order): raise TodoError("The corrected part changed since it was read; read it again.") return replace(self.replaced(order), corrections=(*order.corrections, self)) @@ -4984,8 +5008,8 @@ def part(self, order: OpenProblemOrder) -> object: """The order's current value of the part this correction replaces.""" @abstractmethod - def replaced[Order: OpenProblemOrder](self, order: Order) -> Order: - """The order with this correction's part replaced.""" + def replaced(self, order: OpenProblemOrder) -> OpenProblemOrder: + """The order with this correction's part replaced; its state may change with it.""" @dataclass(frozen=True) @@ -5004,7 +5028,7 @@ def prepare(self, goals: Goals, current: Todo) -> Self: def part(self, order: OpenProblemOrder) -> OrderCheck: return order.check - def replaced[Order: OpenProblemOrder](self, order: Order) -> Order: + def replaced(self, order: OpenProblemOrder) -> OpenProblemOrder: self.check.require_writable() return replace(order, check=self.check) @@ -5018,10 +5042,25 @@ class CorrectRunTodo(CorrectOrderTodo): def part(self, order: OpenProblemOrder) -> OrderRun: return order.run - def replaced[Order: OpenProblemOrder](self, order: Order) -> Order: + def replaced(self, order: OpenProblemOrder) -> OpenProblemOrder: return replace(order, run=self.run) +@dataclass(frozen=True) +class CorrectMergedLinkTodo(CorrectOrderTodo): + """The commander takes back a merge recorded through a pull request that never held this + order's work, such as a merged prerequisite. The order is pending again under its own id, + check, run, edits and times; only that link leaves it. Any other linked pull request stays, + so a merge of real work on it is read again from the forge, never dropped.""" + previous: MergedLink + + def part(self, order: OpenProblemOrder) -> MergedLink: + return order.merged_link(self.previous.link) + + def replaced(self, order: OpenProblemOrder) -> OpenProblemOrder: + return order.unmerged(self.previous.link) + + @dataclass(frozen=True) class WordingCorrection(TodoOperation, StoryEvent): """Core's answer to a published unit message whose words hide what happened; no strike.""" diff --git a/tests/test_holder_standing_rules.py b/tests/test_holder_standing_rules.py index 257ae54ff8..666f5af325 100644 --- a/tests/test_holder_standing_rules.py +++ b/tests/test_holder_standing_rules.py @@ -34,11 +34,14 @@ from agent_comms.task_attachments.commander_pings import CommanderUpdate from agent_comms.comms import wire from agent_comms.coordinator import Coordination -from agent_comms.errors import CommittedMessageError, HumanSendRefusedError, RelationViolationError, TodoConflict +from agent_comms.errors import ( + CommittedMessageError, HumanSendRefusedError, RelationViolationError, TodoConflict, TodoError, +) from agent_comms.field_codec import FieldCodec, InputId, MessageId, OrderId, RecordProblems, TurnId from agent_comms.goal_actions import ( CreateProblemGoalAction, ReissuedOrderGoalAction, + SetGoalAction, ) from agent_comms.goal_presentation import RunnableExecution from agent_comms.goal_states import ActiveGoal @@ -69,6 +72,9 @@ from agent_comms.threads import Thread from agent_comms.todos import ( ClaimOrder, + CorrectMergedLinkTodo, + MergedLink, + PullRequestLink, ResumeClaimOrder, ExpectedProblem, OrderClaim, @@ -440,6 +446,55 @@ def test_goal_generation_cannot_transfer_to_a_different_owner(held_bus): assert held_bus.goals.todos.get(todo.id) == todo +def test_merge_through_a_prerequisite_link_is_taken_back_and_the_order_changes_owner(held_bus): + """P935: a prerequisite PR linked to the order merged it; only that link leaves, the + order is pending again under its own id, check, run and times, and then changes owner.""" + store = held_bus.goals.todos + lead = held_bus.registry.require("lead").incarnation + held = store.get("P1") + prerequisite = PullRequestLink(held.repo, 3596) + implementation = PullRequestLink(held.repo, 3700) + with store.writing() as connection: + replace(held, revision=held.revision + 1, order=replace( + held.order.checked(), pull_requests=(prerequisite, implementation), + ).merged("33bd05c64abc")).upsert(connection) + merged = store.get("P1") + reason = "the merged prerequisite pull request held none of this order's checker work" + + def correction(revision: int, previous: MergedLink) -> CorrectMergedLinkTodo: + return CorrectMergedLinkTodo(revision, lead, reason, previous=previous) + + for refused in (correction(held.revision, MergedLink(prerequisite, "33bd05c64abc")), + correction(merged.revision, MergedLink(PullRequestLink(held.repo, 1), + "33bd05c64abc")), + correction(merged.revision, MergedLink(prerequisite, "0123456789ab"))): + with pytest.raises(TodoError): + store.apply("P1", refused) + assert store.get("P1") == merged + with pytest.raises(TodoError, match="records no merge"): + correction(held.revision, MergedLink(prerequisite, "33bd05c64abc")).apply(held) + + store.apply("P1", correction(merged.revision, MergedLink(prerequisite, "33bd05c64abc"))) + corrected = store.get("P1") + assert isinstance(corrected.order, PendingProblemOrder) + assert corrected.order.pull_requests == (implementation,) + assert corrected.order.corrections[-1].previous == MergedLink(prerequisite, "33bd05c64abc") + assert (replace(corrected.order, pull_requests=(), corrections=()) + == replace(held.order, pull_requests=(), corrections=())) + assert (corrected.holder, corrected.review_at) == (merged.holder, merged.review_at) + + held_bus.goals.update_goal("outsider", SetGoalAction(text="Absorb the checker audit work.")) + goals = {name: held_bus.registry.require(name).goal for name in ("worker", "outsider")} + outsider = held_bus.registry.require("outsider") + store.assign((ExpectedProblem("P1", corrected.revision),), owner=outsider, assigner=lead, + generation=goals["outsider"].require().id, request=corrected.request.require(), + review_at=corrected.review_at, order=corrected.order) + assigned = store.get("P1") + assert assigned.held().owner_identity == outsider.incarnation + assert assigned.order == corrected.order + assert {name: held_bus.registry.require(name).goal for name in goals} == goals + + def test_unanswered_uses_age_direct_target_and_later_answer_references(held_bus): now = time.time() publish(held_bus, "worker", "lead", now - 7200) # before every held order