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