diff --git a/src/agent_comms/todos.py b/src/agent_comms/todos.py index 4902a37123..f1a788e236 100644 --- a/src/agent_comms/todos.py +++ b/src/agent_comms/todos.py @@ -2577,7 +2577,12 @@ def claimed(self, claim: OrderClaim) -> ClaimedProblemOrder: raise TodoError("Only a pending order can be claimed by its holder.") def review_verdicts(self) -> tuple[type[HeadVerdict], ...]: - return (ReviewedHeadVerdict, RejectedHeadVerdict) + """Any order that holds a head takes a decision on it, or a reviewer's mark first.""" + return (ReviewedHeadVerdict, RejectedHeadVerdict, UnderReviewHeadVerdict) + + def under_review(self, record: OrderHeadReviewRecord) -> CheckedProblemOrder: + """Only a claim owes a delivery verdict; elsewhere the head record is the whole mark.""" + return self def sent_back(self, reason: StrikeReason) -> CheckedProblemOrder: return self @@ -3189,8 +3194,8 @@ def closed_by_merge( def owed_claims(self) -> tuple[OrderClaim, ...]: return (self.claim,) - def review_verdicts(self) -> tuple[type[HeadVerdict], ...]: - return (*super().review_verdicts(), UnderReviewHeadVerdict) + def under_review(self, record: OrderHeadReviewRecord) -> ReviewingProblemOrder: + return self.copy_as(ReviewingProblemOrder, review=record) @dataclass(frozen=True, kw_only=True) @@ -4398,8 +4403,7 @@ def description(self, reviewer: str, subject: str) -> str: return f"Under review by {reviewer} on {subject}" def judge_order(self, current: Todo, record: OrderHeadReviewRecord) -> Todo: - return replace(current, order=current.order.checked().copy_as( - ReviewingProblemOrder, review=record)) + return replace(current, order=current.order.checked().under_review(record)) class HeadOrderLink(HeadReviewRelation, declared_name=None): diff --git a/tests/test_board_verbs.py b/tests/test_board_verbs.py index 082c9373ef..383b11144a 100644 --- a/tests/test_board_verbs.py +++ b/tests/test_board_verbs.py @@ -106,7 +106,7 @@ def test_held_board_commands_take_the_same_verdicts_as_the_order(board_review, c rules = LinkedUnreviewedHead.scan_reviews(comms.goals, "lead", ProgressRead(())) rule, = rules assert rule.todo == todo - assert (UnderReviewHeadVerdict in assert_review_preconditions(rule, comms, request)) == claim_it + assert UnderReviewHeadVerdict in assert_review_preconditions(rule, comms, request) frame = capture_frame(comms, "lead", ProgressRead(())) # The timer's own condition remains real: age the observed review timer in this read. timed = replace(todo, review_at=RecordedEventTime(time.time() - 120)) @@ -166,7 +166,7 @@ def test_shared_head_keeps_each_order_on_its_own_board_and_timer_line(board_revi assert {rule.todo.id for rule in rules} == {first.id, second.id} for rule in rules: verdicts = assert_review_preconditions(rule, comms, request) - assert (UnderReviewHeadVerdict in verdicts) == (rule.todo.id == second.id) + assert UnderReviewHeadVerdict in verdicts frame = capture_frame(comms, "lead", ProgressRead(())) timed = replace(first, review_at=RecordedEventTime(time.time() - 120)) frame = replace(frame, live=(LiveHold(timed, comms.registry.require("worker")),)) @@ -174,7 +174,7 @@ def test_shared_head_keeps_each_order_on_its_own_board_and_timer_line(board_revi timer, = timers assert timer.todo == timed assert timer.problem == first.id - assert UnderReviewHeadVerdict not in assert_review_preconditions(timer, comms, request) + assert UnderReviewHeadVerdict in assert_review_preconditions(timer, comms, request) def test_orderless_board_commands_do_not_invent_a_problem(board_review): @@ -203,7 +203,7 @@ def test_claim_link_survives_sendbacks_and_explicit_release_without_editing_goal assert released.order.holds_head(request) assert comms.goals.todos.resolve_order(released.creator_identity, previous.owner, released.id) == released rule, = LinkedUnreviewedHead.scan_reviews(comms.goals, "lead", ProgressRead(())) - assert UnderReviewHeadVerdict not in assert_review_preconditions(rule, comms, request) + assert UnderReviewHeadVerdict in assert_review_preconditions(rule, comms, request) # A former holder can already be doing different work. A head review must not edit that goal. other = comms.goals.todos.get("P1") ReleaseTodo(other.revision, other.creator_identity, other.held(), StrikeReason( diff --git a/tests/test_head_review.py b/tests/test_head_review.py index 71eb4f0b27..25375ca8ea 100644 --- a/tests/test_head_review.py +++ b/tests/test_head_review.py @@ -416,6 +416,25 @@ def test_linked_head_rejection_strikes_revises_and_notifies_its_holder(head_bus) assert StoredHeadReview.head_reviews(comms.goals.todos, request)[0].order.problem == todo.id +def test_a_reviewing_mark_on_an_unclaimed_linked_order_is_recorded(head_bus): + """Every order that takes ok or back on its linked head also takes the reviewing mark.""" + comms, _, request = head_bus + todo = comms.goals.todos.get("P2") + held = todo.held() + RecordPullRequest(todo.revision, held.owner_identity, + PullRequestLink(request.repo, request.pr), held.generation).record(comms.goals, todo.id) + todo = comms.goals.todos.get(todo.id) + scope = OrderHeadReview.for_request( + (todo,), request, lambda linked: UnquotedHeadReview.of(linked, ())) + invoke(comms, request, UnderReviewHeadVerdict(), scope) + changed = comms.goals.todos.get(todo.id) + assert changed.order == todo.order + assert changed.revision == todo.revision + 1 + review, = StoredHeadReview.head_reviews(comms.goals.todos, request) + assert type(review.verdict) is UnderReviewHeadVerdict + assert review.order.problem == todo.id + + def test_acceptance_over_the_growth_limit_needs_a_reason_naming_each_growing_file( head_bus, reviewed_source): comms, github, _ = head_bus