diff --git a/.github/scripts/pull-request-dashboard/CONTEXT.md b/.github/scripts/pull-request-dashboard/CONTEXT.md index 80521d0c215..19c097e3786 100644 --- a/.github/scripts/pull-request-dashboard/CONTEXT.md +++ b/.github/scripts/pull-request-dashboard/CONTEXT.md @@ -30,9 +30,12 @@ diagnostics keep typed classification results and freeze only the source discussion records. `state.py` owns the JSON boundary. Its dashboard facts, stored-result, and state -codecs translate the immutable contracts to the version 16 -`dashboard-state.json` shape. Malformed pull request entries are discarded -individually, so one bad entry does not prevent valid entries from loading. +codecs translate the immutable contracts to the version 17 +`dashboard-state.json` shape. Versions 11 through 13 and version 16 migrate +forward. Versions 14 and 15 belong to incompatible state shapes and regenerate +instead; an integration that combines those shapes must allocate a newer +version. Malformed pull request entries are discarded individually, so one bad +entry does not prevent valid entries from loading. `dashboard_state_update.py` owns the acceptance transaction for one pull request slot. It prepares the cached starting value, reconciles an evaluation with the @@ -154,10 +157,12 @@ reviewer routes keeps the existing wait. ### Reviewer handoff -An acknowledged dashboard override binds a reviewer handoff to one head SHA. -While that head remains current, the handoff routes directly to approvers and -bypasses discussions, approvals, conflicts, required checks, and the Copilot -gate. A push or newer actionable human reviewer feedback ends the handoff. +An acknowledged dashboard override records the observed head as its durable +binding identity. The handoff routes directly to approvers across later pushes +and bypasses discussions, approvals, conflicts, required checks, and the Copilot +gate. Newer actionable human reviewer feedback ends it, including a reply or +edit on a review thread that the handoff had suppressed. Legacy acknowledgements +without the persistence marker retain their original head-bound behavior. ## Routing snapshot diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index e2f374aa607..fb095995420 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -74,6 +74,11 @@ the implementation understandable and operationally cheap. reminders and re-review requests already sent; the delivery version check makes it skip delivery instead. Rolling forward is the way out, and a paused dashboard is the cheaper failure. +- Dashboard state version 17 reads production versions 11 through 13 and the + pre-persistent-handoff version 16. Versions 14 and 15 describe incompatible + state shapes from parallel work, so this version regenerates them rather than + guessing at compatibility. An integration that combines those shapes must + allocate a newer state version. ## Queue and Workflow Concurrency @@ -413,10 +418,11 @@ the implementation understandable and operationally cheap. would hold every ready PR with its author waiting for a review that never runs, so only branches with automatic review are listed and PRs targeting other branches route normally. -- Copilot findings normally return a PR to the author through ordinary - discussion routing: an inline finding is an unresolved review thread, and an - actionable one routes the PR to "waiting on author." In that common path the - gate never fires and no re-review is requested. +- Every unresolved, non-outdated thread Copilot started is an author action, + even after an author reply or a later clean review. This ownership bypasses + generic praise and author-reply classification and ends only when the thread + is resolved or outdated. The gate therefore never needs its expiration path + to retain these findings. - Findings are counted from unresolved, non-outdated review threads Copilot started, not from the comment count on its review. A review's comment count never shrinks, so it keeps counting feedback the author has since addressed @@ -425,8 +431,10 @@ the implementation understandable and operationally cheap. pushed, which is the one change a re-review can respond to. Findings on the current head sit on unchanged code, so asking Copilot to look at it again would reach the same verdict and be requested again on the next pass; those - threads clear when the author resolves them or pushes a fix, which is a - re-request in its own right. + threads stop counting only when the author resolves them or GitHub marks them + outdated after the referenced code moves. A later clean review does not close + an existing thread. A push makes the review stale and triggers a re-request, + but the push alone does not prove that a finding was fixed. - The other state is a first review that never arrived. The gate otherwise relies entirely on automatic Copilot code review to produce it, so when GitHub silently never starts one, the pull request waits on its author forever for a @@ -489,39 +497,47 @@ the implementation understandable and operationally cheap. stale review is the ordinary state between a push and the next re-review, and an icon that is always present says nothing about which PRs are actually waiting. -- An effective reviewer-routing override is a break-glass handoff for the - current head. It forces the reviewer route and bypasses required checks, - Copilot review, merge conflicts, discussion actions, and approval routing. The - author may be stuck or may need a person to explain a basic problem, so no - automated blocker can prevent the handoff. A later push restores normal - routing and gates. Actionable human reviewer feedback posted after the command - also ends the handoff, because a reviewer has answered the request for help and - assigned the next action to the author. Praise, informational comments, bot - feedback, and feedback posted before the command do not end it. -- While a handoff is active, the dashboard classifies only newer human reviewer - feedback. Old discussions and classification failures therefore cannot block - the break-glass route. Once newer feedback produces an author action, normal - discussion classification and routing resume. The dashboard records that - transition in its live status comment so an author reply or a lost state cache - cannot reactivate the same command. A newer command can establish a new - handoff on the same head. +- An effective reviewer-routing override is a break-glass handoff. It forces the + reviewer route across later pushes and bypasses required checks, Copilot + review, merge conflicts, discussion actions, and approval routing. The author + may be stuck or may need a person to explain a basic problem, so no automated + blocker can prevent the handoff. Top-level feedback last changed at or before + the command stays retired. Review threads are not retired by the command, but + their existing work does not override the handoff. Actionable human reviewer + feedback on an unresolved, non-outdated thread with an effective content + timestamp after the command ends the handoff, because a reviewer has answered + the request for help and assigned the next action to the author. That includes + a new reply or edit on an older active review thread. Praise, informational + comments, bot feedback, inactive threads, and feedback last changed at or + before the command do not end it. +- While a handoff is active, the dashboard classifies only human reviewer + feedback with content activity after the command. Older discussions and + classification failures therefore cannot block the break-glass route. Once + newer feedback produces an author action, normal discussion classification + and routing resume, subject to the permanent top-level feedback cutoff. + The dashboard records that transition in its live status comment so an author + reply or a lost state cache cannot reactivate the same command. A newer + command can establish a new handoff on the same head and advance the cutoff. - The dashboard binds a command to the head it sees when it first reads that command, and records that head in an acknowledgement marker on either the - command reply or the live status comment. The handoff is then a comparison of - two strings: the recorded head and the current one. The earlier design - instead ordered the command against the push by comparing the comment - timestamp with the head push time from `GET /repos/{repo}/activity`. - Do not reintroduce that. Both timestamps have one-second resolution and come - from different APIs, so the ordering is sometimes unknowable, which forces a - third "cannot tell" state that every later pass has to carry forward and every - failure path has to preserve by hand. It also fails unsafely in the case the - handoff exists for: when the activity lookup returns nothing, the command - hangs unacknowledged. Binding to the observed head removes the extra API call - and keeps the answer in GitHub rather than in `dashboard-state.json`, so a - failed pass or a dropped cache cannot corrupt it. The cost is that a push - between the command and the pass that reads it belongs to the handoff instead - of ending it, which is the safer direction: the author asked for help, and the - worst case is one extra handoff the author can end with another push. + command reply or the live status comment. A persistence marker makes the + cross-push behavior explicit. Legacy acknowledgements without that marker + remain head-bound, as do pending bindings restored from compatible legacy + state, so deployment cannot reactivate old handoffs that had already expired. + A separate companion marker records the frozen timestamp used for the + permanent top-level feedback cutoff. Marker lookup is scoped to the bound + command. A legacy acknowledgement without that cutoff does not retire + feedback, because the command's current edit timestamp cannot reconstruct the + original cutoff. The first observation freezes the cutoff for that command, + so editing the command later cannot retire intervening feedback; only a newer + command advances it. +- Recording the observed head also avoids ordering the command against a push by + comparing the comment timestamp with the head push time from + `GET /repos/{repo}/activity`. Both timestamps have one-second resolution and + come from different APIs, so the ordering is sometimes unknowable. Binding to + the observed head removes that extra API call. A push between the command and + the pass that first reads it belongs to the handoff, as does any later push + after the persistence marker is recorded. - The gate does not wait for the required checks before requesting the review, so the two run at once. A route computed while checks are still running is provisional, but the only outcome that matters here is a failure, and a @@ -565,12 +581,20 @@ the implementation understandable and operationally cheap. `actions/stale` reads, so no PR in a dashboard repository could go stale. The dashboard app is never a PR's author, so `role_for` always classifies its comments as `bot` and they never count. -- An inline review thread's wait age and list position come from its last - comment's `createdAt`, never its edit time. Wait age is what makes a neglected - thread visible, so a reviewer fixing a typo in their own comment must not make - a weeks-old thread look freshly raised. Top-level feedback items date from - their creation time for the same reason, so editing a comment cannot reorder - the list or reset how long an item has been waiting. +- An inline review thread's list position comes from its last comment's + `createdAt`, never its edit time. Effective content activity selects the + latest participant and requester and supplies the pending action timestamp, + so editing an older request can reactivate it without reordering the + classifier transcript or list. The same selected comment supplies praise and + author-reply classification, and removing praise leaves the remaining + conversation order intact. Top-level feedback items still date from their + creation time, so editing a comment cannot reorder that list. +- The ignored praise index is a current-evaluation projection used for reviewer + attribution. Classification caches retain the praise verdict but not its + position, so each refresh recomputes the index from current thread activity. + Pending actions are not persisted in dashboard state, which keeps state + version 17 valid. Legacy projections with only `ignored_last_comment` retain + creation-last behavior. ## Top-Level Feedback @@ -587,14 +611,16 @@ the implementation understandable and operationally cheap. deleted source comments authoritative without additional reconciliation. Cached classifications avoid repeated LLM calls, while dashboard state retains the author reply already observed for each item. -- An explicit author reply is the only thing that closes a top-level item. - Commits, PR title edits, and PR description edits are not tied to the item - they would close, so any push after the feedback arrived would close every - open item at once and hide feedback nobody had answered. The status comment - lists the exact open discussions and explains how to give each one an outcome, - which makes an explicit reply both cheap and unambiguous. An author's explicit - commitment to future work in the current PR is a self-deferral, not a - completed reply, so the item continues waiting on the author. +- In normal routing, an explicit author reply is the only thing that closes a + top-level item. Commits, PR title edits, and PR description edits are not tied + to the item they would close, so a push alone never closes feedback. The + break-glass reviewer command is the deliberate bulk exception: it retires + top-level items at or before its cutoff while leaving review threads open. + The status comment lists the exact open discussions and explains how to give + each one an outcome, which makes an explicit reply both cheap and unambiguous. + An author's explicit commitment to future work in the current PR is a + self-deferral, not a completed reply, so the item continues waiting on the + author. - Each model call classifies up to ten uncached top-level feedback items independently, while retaining a separate cache entry for every item. A refresh processes at most 200 such items per PR. Exceeding that cap means the @@ -638,8 +664,10 @@ the implementation understandable and operationally cheap. the classifier is told which kind of item it is reading, so the summary rule below applies only to them. Review state stays independent of that: a `CHANGES_REQUESTED` state affects only the reviewer's badge; it does not - affect dashboard actions or routing. Empty review summaries are ignored; - their inline comments, if any, define independent actions. + affect dashboard actions or routing. Formal review states are ordered by + submission time, while summary edits use their content timestamp for + lifecycle decisions. Empty review summaries are ignored; their inline + comments, if any, define independent actions. - Who a comment opens by addressing is extracted in code and passed to the classifier as `addressed_to`, rather than left for the model to find in the body. A reviewer routinely names other people, pull requests, and prior work @@ -664,10 +692,9 @@ the implementation understandable and operationally cheap. contains requests, so a reviewer whose only actionable point is in the summary should post it as its own comment. - The author reply that closed an item is retained in the cached PR result. It - is reused only when it is newer than the item's creation time, which an edit - never moves. Accepted tradeoff: a substantively rewritten request keeps the - reply that answered its earlier text, so a reviewer who needs the new text - answered should post it as a new comment. Ordinary requester-confirmation + is reused only when it is newer than the item's effective content timestamp. + Editing an older request with new work therefore reopens the item when the + edit is newer than the completed author reply. Ordinary requester-confirmation timestamps are not persisted. - Reviewers should prefer inline comments when feedback needs explicit closure. Blocking PR-wide feedback should use GitHub's **Request changes** review state; diff --git a/.github/scripts/pull-request-dashboard/classification_execution.py b/.github/scripts/pull-request-dashboard/classification_execution.py index ec69743838c..3060c17f684 100644 --- a/.github/scripts/pull-request-dashboard/classification_execution.py +++ b/.github/scripts/pull-request-dashboard/classification_execution.py @@ -719,11 +719,15 @@ def _classify_review_threads( by_id[discussion_id] = with_result_metadata( result, since=( - discussion.comments[-1].timestamp - if discussion.comments - else "" + discussion.selected_activity_timestamp + or ( + discussion.comments[-1].timestamp + if discussion.comments + else "" + ) ), ignored_last_comment=(discussion_id in ignored), + ignored_comment_index=discussion.ignored_comment_index, ) return by_id diff --git a/.github/scripts/pull-request-dashboard/classification_policy.py b/.github/scripts/pull-request-dashboard/classification_policy.py index 29ee9862291..096faa070d7 100644 --- a/.github/scripts/pull-request-dashboard/classification_policy.py +++ b/.github/scripts/pull-request-dashboard/classification_policy.py @@ -7,11 +7,12 @@ import re from collections.abc import Mapping, Sequence from dataclasses import dataclass, field, replace +from datetime import datetime, timezone from enum import Enum from types import MappingProxyType from typing import Any -from utils import truncate +from utils import parse_ts, truncate DISCUSSION_COMMENT_BODY_MAX_CHARS = 500 @@ -19,6 +20,7 @@ TOP_LEVEL_CLASSIFICATION_BATCH_SIZE = 10 AUTHOR_COMMENT_DIAGNOSTIC_ITEM_LIMIT = 10 PRAISE_MAX_CHARS = 80 +_MIN_TIMESTAMP = datetime.min.replace(tzinfo=timezone.utc) class _PromptTooLongError(ValueError): @@ -364,6 +366,17 @@ class DiscussionComment: timestamp: str = "" actor_role: str = "" body: str = "" + activity_timestamp: str = "" + + @property + def effective_activity_timestamp(self) -> str: + timestamp = parse_ts(self.timestamp) + activity_timestamp = parse_ts(self.activity_timestamp) + if activity_timestamp is not None and ( + timestamp is None or activity_timestamp >= timestamp + ): + return self.activity_timestamp + return self.timestamp if timestamp is not None else "" @dataclass(frozen=True) @@ -380,6 +393,10 @@ class ClassificationDiscussion: pr_author: str = "" source_kind: str = "" candidate_feedback: tuple[CandidateFeedback, ...] = () + strict_author_action: bool = False + selected_comment_index: int | None = None + selected_activity_timestamp: str = "" + ignored_comment_index: int | None = None def __post_init__(self) -> None: object.__setattr__(self, "comments", tuple(self.comments)) @@ -409,6 +426,9 @@ def from_record(cls, record: Mapping[str, Any]) -> ClassificationDiscussion: timestamp=str(comment.get("timestamp") or ""), actor_role=str(comment.get("actor_role") or ""), body=str(comment.get("body") or ""), + activity_timestamp=str( + comment.get("activity_timestamp") or "" + ), ) for comment in _mapping_items(record.get("comments")) ) @@ -426,13 +446,24 @@ def from_record(cls, record: Mapping[str, Any]) -> ClassificationDiscussion: pr_author=str(record.get("pr_author") or ""), source_kind=str(record.get("source_kind") or ""), candidate_feedback=candidate_feedback, + strict_author_action=bool(record.get("strict_author_action")), ) def with_comments( self, comments: Sequence[DiscussionComment], + *, + selected_comment_index: int | None = None, + selected_activity_timestamp: str = "", + ignored_comment_index: int | None = None, ) -> ClassificationDiscussion: - return replace(self, comments=tuple(comments)) + return replace( + self, + comments=tuple(comments), + selected_comment_index=selected_comment_index, + selected_activity_timestamp=selected_activity_timestamp, + ignored_comment_index=ignored_comment_index, + ) @dataclass(frozen=True) @@ -484,6 +515,7 @@ class ClassificationSuccess: cli_call: bool = False since: str = "" ignored_last_comment: bool = False + ignored_comment_index: int | None = None @property def failed(self) -> bool: @@ -502,6 +534,7 @@ class ClassificationFailure: cli_call: bool = False since: str = "" ignored_last_comment: bool = False + ignored_comment_index: int | None = None @property def failed(self) -> bool: @@ -518,6 +551,7 @@ class ClassificationDeferred: decision: ClassificationDecision since: str = "" ignored_last_comment: bool = False + ignored_comment_index: int | None = None @property def failed(self) -> bool: @@ -802,10 +836,17 @@ def review_thread_author_reply_input( discussion: ClassificationDiscussion, ) -> dict[str, Any]: body = "" - for comment in reversed(discussion.comments): - if comment.actor_role == "author": - body = comment.body - break + selected_index = discussion.selected_comment_index + if ( + selected_index is not None + and 0 <= selected_index < len(discussion.comments) + ): + body = discussion.comments[selected_index].body + else: + for comment in reversed(discussion.comments): + if comment.actor_role == "author": + body = comment.body + break return { "discussion_id": discussion.identity.discussion_id, "body": body, @@ -815,9 +856,10 @@ def review_thread_author_reply_input( def praise_prompt_input( discussion: ClassificationDiscussion, ) -> dict[str, Any]: + selected = _latest_review_thread_comment(discussion.comments) return { "discussion_id": discussion.identity.discussion_id, - "body": discussion.comments[-1].body if discussion.comments else "", + "body": selected.comment.body if selected is not None else "", } @@ -1566,20 +1608,53 @@ def map_verdict_result( def prepare_praise_candidates( discussions: Sequence[ClassificationDiscussion], ) -> tuple[ClassificationDiscussion, ...]: - return tuple(discussion for discussion in discussions if _could_be_praise(discussion)) + return tuple( + discussion + for discussion in discussions + if not discussion.strict_author_action and _could_be_praise(discussion) + ) def _could_be_praise(discussion: ClassificationDiscussion) -> bool: - comments = discussion.comments - role = comments[-1].actor_role if comments else "" - if not comments or role in ("author", "bot"): + selected = _latest_review_thread_comment(discussion.comments) + if selected is None or selected.comment.actor_role in ("author", "bot"): return False return ( - len(" ".join(comments[-1].body.split())) + len(" ".join(selected.comment.body.split())) <= PRAISE_MAX_CHARS ) +@dataclass(frozen=True) +class _ReviewThreadCommentSelection: + index: int + comment: DiscussionComment + activity_timestamp: str + + +def _latest_review_thread_comment( + comments: Sequence[DiscussionComment], +) -> _ReviewThreadCommentSelection | None: + if not comments: + return None + index, comment = max( + enumerate(comments), + key=lambda item: ( + ( + parse_ts(item[1].effective_activity_timestamp) + or _MIN_TIMESTAMP + ), + parse_ts(item[1].timestamp) or _MIN_TIMESTAMP, + item[0], + ), + ) + return _ReviewThreadCommentSelection( + index, + comment, + comment.effective_activity_timestamp, + ) + + def resolve_review_thread_policy( discussions: Sequence[ClassificationDiscussion], praise_results: Mapping[str, ClassificationResult], @@ -1599,17 +1674,33 @@ def resolve_review_thread_policy( resolved: dict[str, ClassificationResult] = dict(failed_praise) author_replies: list[ClassificationDiscussion] = [] since_by_id: dict[str, str] = {} + ignored_comment_index_by_id: dict[str, int] = {} for discussion in discussions: discussion_id = discussion.identity.discussion_id if discussion_id in failed_praise: continue comments = list(discussion.comments) dropped = discussion_id in ignored - if dropped: - comments.pop() - if comments: - since_by_id[discussion_id] = comments[-1].timestamp - if dropped and not comments: + selected = _latest_review_thread_comment(comments) + if dropped and selected is not None: + ignored_comment_index_by_id[discussion_id] = selected.index + comments.pop(selected.index) + selected = _latest_review_thread_comment(comments) + if selected is not None: + since_by_id[discussion_id] = ( + selected.activity_timestamp + or selected.comment.timestamp + ) + if discussion.strict_author_action: + resolved[discussion_id] = ClassificationSuccess( + discussion.identity, + ActionDecision( + DiscussionAction.AUTHOR, + "This unresolved Copilot finding remains author work until " + "the thread is resolved or outdated.", + ), + ) + elif dropped and not comments: resolved[discussion_id] = ClassificationSuccess( discussion.identity, ActionDecision( @@ -1617,14 +1708,26 @@ def resolve_review_thread_policy( "This thread is only praise.", ), ) - elif comments and comments[-1].actor_role == "author": - author_replies.append(discussion.with_comments(comments)) + elif ( + selected is not None + and selected.comment.actor_role == "author" + ): + author_replies.append( + discussion.with_comments( + comments, + selected_comment_index=selected.index, + selected_activity_timestamp=selected.activity_timestamp, + ignored_comment_index=( + ignored_comment_index_by_id.get(discussion_id) + ), + ) + ) else: resolved[discussion_id] = ClassificationSuccess( discussion.identity, ActionDecision( DiscussionAction.AUTHOR, - "The last comment on this unresolved thread is not the author's.", + "The latest activity on this unresolved thread is not the author's.", ), ) resolved = { @@ -1632,6 +1735,9 @@ def resolve_review_thread_policy( result, since=since_by_id.get(discussion_id, ""), ignored_last_comment=(discussion_id in ignored), + ignored_comment_index=ignored_comment_index_by_id.get( + discussion_id + ), ) for discussion_id, result in resolved.items() } @@ -1643,6 +1749,7 @@ def with_result_metadata( *, since: str = "", ignored_last_comment: bool = False, + ignored_comment_index: int | None = None, ) -> ClassificationResult: return replace( result, @@ -1650,6 +1757,11 @@ def with_result_metadata( ignored_last_comment=( ignored_last_comment or result.ignored_last_comment ), + ignored_comment_index=( + ignored_comment_index + if ignored_comment_index is not None + else result.ignored_comment_index + ), ) @@ -1742,6 +1854,8 @@ def classification_result_to_record( record["since"] = result.since if result.ignored_last_comment: record["ignored_last_comment"] = True + if result.ignored_comment_index is not None: + record["ignored_comment_index"] = result.ignored_comment_index return record @@ -1758,6 +1872,7 @@ def cached_classification_record( "response_text", "stderr", "usage", + "ignored_comment_index", ) } diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 93b3a0fe349..93493607300 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -130,11 +130,16 @@ int ID of the command bound to the reviewer handoff. dashboard_override_head_sha str Head an override is bound to; - the handoff is active while it - equals head_sha and has not - been cleared by newer feedback. + identifies the durable command + binding. dashboard_override_since str (iso) Effective content timestamp of the command bound to the handoff. + dashboard_top_level_feedback_cutoff + str (iso) Top-level feedback at or before + this command timestamp is retired. + dashboard_override_persistent bool Handoff remains active across + pushes until newer feedback + clears it. dashboard_override_cleared_by_feedback bool Actionable human reviewer feedback ended the handoff. diff --git a/.github/scripts/pull-request-dashboard/dashboard_contracts.py b/.github/scripts/pull-request-dashboard/dashboard_contracts.py index 8a58111fdb2..af1ac4c4e58 100644 --- a/.github/scripts/pull-request-dashboard/dashboard_contracts.py +++ b/.github/scripts/pull-request-dashboard/dashboard_contracts.py @@ -37,6 +37,8 @@ class DashboardCommandReply: route: DashboardRoute | None = None held_gates: str = "" since: str = "" + top_level_feedback_cutoff: str = "" + persistent_handoff: bool = False def __post_init__(self) -> None: if self.comment_id <= 0: @@ -60,6 +62,12 @@ def __post_init__(self) -> None: ) if self.kind != "routed" and self.route is not None: raise ValueError("only routed dashboard command replies may include a route") + if self.kind != "routed" and self.persistent_handoff: + raise ValueError("only routed replies may persist a reviewer handoff") + if self.persistent_handoff and self.route is DashboardRoute.AUTHOR: + raise ValueError( + "persistent reviewer handoff replies require a reviewer route" + ) @dataclass(frozen=True) @@ -85,6 +93,8 @@ class DashboardFacts: dashboard_override_bound_command_id: int = 0 dashboard_override_head_sha: str = "" dashboard_override_since: str = "" + dashboard_top_level_feedback_cutoff: str = "" + dashboard_override_persistent: bool = False dashboard_override_cleared_by_feedback: bool = False dashboard_command_replies: tuple[DashboardCommandReply, ...] = () copilot_review_requested: bool = False diff --git a/.github/scripts/pull-request-dashboard/dashboard_override.py b/.github/scripts/pull-request-dashboard/dashboard_override.py index e39c0dc143b..b06f0891198 100644 --- a/.github/scripts/pull-request-dashboard/dashboard_override.py +++ b/.github/scripts/pull-request-dashboard/dashboard_override.py @@ -5,6 +5,7 @@ import re from collections.abc import Sequence from dataclasses import dataclass +from datetime import datetime from dashboard_contracts import ( DashboardCommandReply, @@ -28,15 +29,29 @@ r"" ) OVERRIDE_ACK_MARKER_PREFIX = "" ) +TOP_LEVEL_FEEDBACK_CUTOFF_MARKER_PREFIX = ( + "" +) +PERSISTENT_HANDOFF_MARKER_PREFIX = ( + "" +) PRE_REVIEW_ROUTES = ("author",) @@ -47,6 +62,8 @@ class DashboardOverrideFacts: bound_command_id: int head_sha: str since: str + top_level_feedback_cutoff: str + persistent_handoff: bool cleared_by_feedback: bool command_replies: tuple[DashboardCommandReply, ...] @@ -63,9 +80,9 @@ def author_override_guidance(staleness_note: str = "") -> str: guidance = ( "If you need reviewer or maintainer help, comment " "`/dashboard route:reviewers` to request routing from waiting on the " - "author to waiting on reviewers. The dashboard binds the request to " - "the head it sees when it reads the command, and a later push restores " - "normal routing." + "author to waiting on reviewers. Top-level feedback through that command " + "is retired; unresolved review threads remain open. The handoff remains " + "active across pushes until newer actionable human feedback arrives." ) if staleness_note: guidance = f"{guidance} {staleness_note}" @@ -137,13 +154,18 @@ def latest_authorized_command( def _effective_command_timestamp(comment: IssueComment) -> str: - created_at = parse_ts(comment.created_at) - content_updated_at = parse_ts(comment.content_updated_at) - if content_updated_at is not None and ( - created_at is None or content_updated_at >= created_at - ): - return comment.content_updated_at - return comment.created_at + return comment.effective_content_timestamp + + +def _latest_valid_timestamp(*values: str) -> str: + candidates = [ + (parsed, value) + for value in values + if (parsed := parse_ts(value)) is not None + ] + if not candidates: + return "" + return max(candidates, key=lambda item: item[0])[1] def dashboard_override_facts( @@ -178,6 +200,16 @@ def dashboard_override_facts( if previous_facts is not None else "" ) + previous_top_level_feedback_cutoff = ( + previous_facts.dashboard_top_level_feedback_cutoff + if previous_facts is not None + else "" + ) + previous_persistent_handoff = ( + previous_facts.dashboard_override_persistent + if previous_facts is not None + else False + ) previous_cleared = ( previous_facts.dashboard_override_cleared_by_feedback if previous_facts is not None @@ -198,8 +230,11 @@ def dashboard_override_facts( acknowledged_since, acknowledgement_created_at, ) = acknowledged_override(source.issue_comments) + previous_binding_command_id = ( + previous_bound_command_id or previous_command_id + ) previous_binding_matches = bool( - bound_command_id == previous_bound_command_id + bound_command_id == previous_binding_command_id and bound_head and bound_head == previous_head_sha ) @@ -210,6 +245,35 @@ def dashboard_override_facts( or _override_command_effective_at(source.issue_comments, bound_command_id) or acknowledgement_created_at ) + existing_command_binding = bool( + command_id + and command_id == previous_binding_command_id + ) + new_command_binding = bool(command_id and not existing_command_binding) + top_level_feedback_cutoff = _latest_valid_timestamp( + previous_top_level_feedback_cutoff, + command_created_at if command_id and not existing_command_binding else "", + ( + acknowledged_top_level_feedback_cutoff( + source.issue_comments, + bound_command_id, + ) + if bound_command_id + else "" + ), + ) + persistent_handoff = bool( + bound_head + and ( + new_command_binding + or (previous_binding_matches and previous_persistent_handoff) + or acknowledges_persistent_handoff( + source.issue_comments, + bound_command_id, + bound_head, + ) + ) + ) cleared_command_id, cleared_head = status_reviewer_handoff_clearance( source.issue_comments ) @@ -232,6 +296,8 @@ def dashboard_override_facts( # binding from that acknowledgement. head_sha=bound_head, since=override_since, + top_level_feedback_cutoff=top_level_feedback_cutoff, + persistent_handoff=persistent_handoff, cleared_by_feedback=cleared_by_feedback, command_replies=pending_command_replies(source, author, reviewers), ) @@ -285,6 +351,45 @@ def acknowledges_override(text: str, command_id: int, head_sha: str) -> bool: ) +def acknowledged_top_level_feedback_cutoff( + comments: Sequence[IssueComment], + command_id: int = 0, +) -> str: + candidates: list[tuple[datetime, int, str]] = [] + for comment in comments or []: + if not _is_dashboard_app_comment(comment): + continue + for match in _TOP_LEVEL_FEEDBACK_CUTOFF_MARKER_RE.finditer(comment.body): + marker_command_id = int(match.group(1)) + if command_id and marker_command_id != command_id: + continue + value = match.group(2) + parsed = parse_ts(value) + if parsed is not None: + candidates.append((parsed, marker_command_id, value)) + if not candidates: + return "" + return max(candidates, key=lambda item: (item[0], item[1]))[2] + + +def acknowledges_persistent_handoff( + comments: Sequence[IssueComment], + command_id: int, + head_sha: str, +) -> bool: + if not command_id or not head_sha: + return False + return any( + _is_dashboard_app_comment(comment) + and any( + int(match.group(1)) == command_id + and match.group(2) == head_sha + for match in _PERSISTENT_HANDOFF_MARKER_RE.finditer(comment.body) + ) + for comment in comments or [] + ) + + def acknowledged_override( comments: Sequence[IssueComment], ) -> tuple[int, str, str, str]: @@ -392,6 +497,20 @@ def override_ack_marker( return f"{OVERRIDE_ACK_MARKER_PREFIX}{comment_id}{head}{since} -->" +def top_level_feedback_cutoff_marker( + comment_id: int, + cutoff: str, +) -> str: + return f"{TOP_LEVEL_FEEDBACK_CUTOFF_MARKER_PREFIX}{comment_id}:{cutoff} -->" + + +def persistent_handoff_marker( + comment_id: int, + head_sha: str, +) -> str: + return f"{PERSISTENT_HANDOFF_MARKER_PREFIX}{comment_id}:{head_sha} -->" + + def render_command_reply(reply: DashboardCommandReply) -> str: user = reply.user mention = f"@{user}, " if user else "" @@ -406,9 +525,8 @@ def render_command_reply(reply: DashboardCommandReply) -> str: route = reply.route.value held_gates = reply.held_gates if route in PRE_REVIEW_ROUTES: - # An active handoff always routes to approvers, so a pre-review - # route means the command is bound to a head that has been pushed - # over. + # New handoffs persist across pushes. A pre-review route can remain + # only for a legacy acknowledgement without the persistence marker. message = ( "your reviewer-routing request is not active for the current " "pull request head; comment `/dashboard route:reviewers` again " @@ -426,6 +544,33 @@ def render_command_reply(reply: DashboardCommandReply) -> str: ) else: message = "this pull request was routed to reviewers." + if reply.persistent_handoff: + message = ( + f"{message} The handoff remains active across pushes until newer " + "actionable human feedback arrives." + ) + has_top_level_feedback_cutoff = ( + parse_ts(reply.top_level_feedback_cutoff) is not None + ) + has_handoff_timestamp = parse_ts(reply.since) is not None + if has_top_level_feedback_cutoff and has_handoff_timestamp: + message = ( + f"{message} Top-level feedback through this request will not return; " + "unresolved review threads remain open." + ) + elif has_top_level_feedback_cutoff: + message = ( + f"{message} The existing top-level feedback cutoff remains in effect, " + "but no additional top-level feedback was retired because the " + "dashboard could not determine a safe command time; unresolved review " + "threads remain open." + ) + else: + message = ( + f"{message} No top-level feedback was retired because the dashboard " + "could not determine a safe command time; unresolved review threads " + "remain open." + ) elif kind == "unknown_command": subcommand = reply.subcommand attempted = DASHBOARD_COMMAND_PREFIX + (f" {subcommand}" if subcommand else "") @@ -449,6 +594,17 @@ def render_command_reply(reply: DashboardCommandReply) -> str: reply.since, ) ) + if has_top_level_feedback_cutoff: + markers.append( + top_level_feedback_cutoff_marker( + comment_id, + reply.top_level_feedback_cutoff, + ) + ) + if reply.persistent_handoff and reply.head_sha: + markers.append( + persistent_handoff_marker(comment_id, reply.head_sha) + ) return "\n".join([ *markers, f"{mention}{message}", @@ -475,10 +631,12 @@ def append_command_ack_reply( ) -> DashboardFacts: """Queue the reply that acknowledges an override command. - The reply carries the acknowledgement marker, which records the bound head - and feedback cutoff and stops the command from being processed again. A - command superseded by reviewer feedback is acknowledged in the status comment - instead of producing another top-level comment. + The reply carries markers that record the command binding, permanent + feedback cutoff, and persistent handoff. A command superseded by reviewer + feedback is acknowledged in the status comment instead of producing another + top-level comment. A persistent handoff that currently routes to the author + stays unacknowledged until a reviewer route returns, so the reply always + records the handoff it announces. """ cleared_by_feedback = facts.dashboard_override_cleared_by_feedback command_id = ( @@ -497,6 +655,8 @@ def append_command_ack_reply( ) if cleared_by_feedback: return facts.with_changes(dashboard_override_since=override_since) + if facts.dashboard_override_persistent and route is DashboardRoute.AUTHOR: + return facts.with_changes(dashboard_override_since=override_since) kind = "routed" replies = facts.dashboard_command_replies reply = DashboardCommandReply( @@ -511,6 +671,10 @@ def append_command_ack_reply( else "" ), since=override_since, + top_level_feedback_cutoff=( + facts.dashboard_top_level_feedback_cutoff + ), + persistent_handoff=facts.dashboard_override_persistent, ) if command_reply_exists(source.issue_comments, reply): return facts diff --git a/.github/scripts/pull-request-dashboard/dashboard_test_support.py b/.github/scripts/pull-request-dashboard/dashboard_test_support.py index 8bd0f95fdbd..390cfa9fac9 100644 --- a/.github/scripts/pull-request-dashboard/dashboard_test_support.py +++ b/.github/scripts/pull-request-dashboard/dashboard_test_support.py @@ -109,6 +109,7 @@ def review_source(**changes: Any) -> Review: "state": "COMMENTED", "submitted_at": "2026-08-16T07:00:00Z", "updated_at": "2026-08-16T07:00:00Z", + "content_updated_at": "2026-08-16T07:00:00Z", } values.update(changes) return Review(**values) @@ -133,6 +134,7 @@ def review_thread_comment(**changes: Any) -> ReviewThreadComment: "url": "https://example.test/review-comment/1", "body": "Please update this.", "created_at": "2026-08-16T07:00:00Z", + "updated_at": "2026-08-16T07:00:00Z", "actor": actor("reviewer"), } values.update(changes) @@ -244,6 +246,7 @@ def pull_request_source( "body": value.body, "submitted_at": value.submitted_at, "updated_at": value.updated_at, + "content_updated_at": value.content_updated_at, } for value in reviews ], @@ -272,6 +275,7 @@ def pull_request_source( "url": comment.url, "body": comment.body, "createdAt": comment.created_at, + "lastEditedAt": comment.updated_at, "author": _actor_json(comment.actor), "reactionGroups": [ { diff --git a/.github/scripts/pull-request-dashboard/discussion_lifecycle.py b/.github/scripts/pull-request-dashboard/discussion_lifecycle.py index 2aea9f0bba6..23d669f9182 100644 --- a/.github/scripts/pull-request-dashboard/discussion_lifecycle.py +++ b/.github/scripts/pull-request-dashboard/discussion_lifecycle.py @@ -2,6 +2,7 @@ from collections.abc import Callable, Mapping from dataclasses import dataclass +from datetime import datetime, timezone from enum import Enum from typing import Any, TypedDict @@ -27,6 +28,7 @@ POSITIVE_ACK_REACTIONS = {"THUMBS_UP", "HOORAY", "HEART", "ROCKET"} _HUMAN_REVIEWER_ROLES = frozenset({"approver", "outsider"}) +_MIN_TIMESTAMP = datetime.min.replace(tzinfo=timezone.utc) class LifecycleMode(Enum): @@ -110,28 +112,55 @@ def _discussion_comment( reviewers: set[str], body: str, positive_reactors: set[str] | None = None, + activity_timestamp: str = "", ) -> dict[str, Any]: - return { + comment = { "timestamp": timestamp, "actor": actor, "actor_role": role_for(actor, author, reviewers), "body": truncate(body), "positive_reactors": sorted(positive_reactors or set()), } + if activity_timestamp and activity_timestamp != timestamp: + comment["activity_timestamp"] = activity_timestamp + return comment def _add_discussion_facts( discussion: dict[str, Any], comments: list[dict[str, Any]], conflicts: str, + *, + latest_comment: dict[str, Any] | None = None, ) -> dict[str, Any]: + latest = latest_comment if latest_comment is not None else comments[-1] discussion["discussion_facts"] = { - "latest_comment_role": comments[-1].get("actor_role"), + "latest_comment_role": latest.get("actor_role"), "current_conflicts": conflicts, } return discussion +def _latest_activity_comment( + comments: list[dict[str, Any]], +) -> dict[str, Any]: + return max( + enumerate(comments), + key=lambda item: ( + ( + parse_ts( + item[1].get("activity_timestamp") + or item[1].get("timestamp") + or "" + ) + or _MIN_TIMESTAMP + ), + parse_ts(item[1].get("timestamp") or "") or _MIN_TIMESTAMP, + item[0], + ), + )[1] + + def _positive_reaction_logins( comment: ReviewThreadComment, ) -> set[str]: @@ -146,21 +175,15 @@ def _positive_reaction_logins( return logins -def _group_review_threads( - source: DiscussionInput, - *, - include_inactive: bool = False, -) -> list[dict[str, Any]]: +def _group_review_threads(source: DiscussionInput) -> list[dict[str, Any]]: discussions: list[dict[str, Any]] = [] reviewers = set(source.reviewers) for discussion in source.review_threads: - if ( - discussion.is_resolved or discussion.is_outdated - ) and not include_inactive: + if discussion.is_resolved or discussion.is_outdated: continue raw_comments = discussion.comments - thread_url = raw_comments[0].url if raw_comments else "" ordered = sorted(raw_comments, key=lambda comment: comment.created_at) + root_comment = ordered[0] if ordered else None comments = [ _discussion_comment( comment.created_at, @@ -169,6 +192,7 @@ def _group_review_threads( reviewers, comment.body, _positive_reaction_logins(comment), + comment.effective_content_timestamp, ) for comment in ordered ] @@ -177,6 +201,7 @@ def _group_review_threads( comment["actor_role"] == "author" for comment in comments ): continue + latest_activity_comment = _latest_activity_comment(comments) discussions.append( _add_discussion_facts( { @@ -185,14 +210,23 @@ def _group_review_threads( or f"review-discussion-{len(discussions) + 1}" ), "discussion_kind": "review-comment-thread", + "strict_author_action": bool( + root_comment + and root_comment.actor.is_copilot_reviewer + ), "path": discussion.path or None, "line": discussion.line, "resolved": False, - "discussion_url": thread_url, + "discussion_url": ( + root_comment.url if root_comment is not None else "" + ), + "requester": latest_activity_comment.get("actor") or "", + "pr_author": source.author, "comments": comments, }, comments, source.conflicts, + latest_comment=latest_activity_comment, ) ) discussions.sort(key=lambda thread: thread["comments"][-1]["timestamp"]) @@ -219,6 +253,13 @@ def _derive_top_level_items(source: DiscussionInput) -> list[dict[str, Any]]: "body": body, "positive_reactors": [], } + activity_timestamp = ( + event.get("content_timestamp") + or event.get("timestamp") + or root_timestamp + ) + if activity_timestamp != root_timestamp: + comment["activity_timestamp"] = activity_timestamp if ( event.get("source_id") is not None and comment["actor"] @@ -272,7 +313,7 @@ def _derive_top_level_author_comment_items( ) items: list[dict[str, Any]] = [] for event in source.events: - timestamp = event.get("created_timestamp") or event.get("timestamp") or "" + timestamp = event.get("timestamp") or event.get("created_timestamp") or "" if ( event.get("kind") != "issue-comment" or event.get("actor_role") != "author" @@ -288,17 +329,30 @@ def _derive_top_level_author_comment_items( "body": truncate(event.get("body") or ""), "positive_reactors": [], } - candidate_feedback = [ - { + author_timestamp = parse_ts(timestamp) + candidate_feedback = [] + for item in top_level_items: + item_comments = item.get("comments") or [] + feedback_timestamp = parse_ts( + (item_comments[-1] if item_comments else {}).get( + "activity_timestamp" + ) + or item.get("root_timestamp") + or "" + ) + if ( + author_timestamp is None + or feedback_timestamp is None + or feedback_timestamp >= author_timestamp + ): + continue + candidate_feedback.append({ "discussion_id": item["discussion_id"], "body": "\n\n".join( item_comment.get("body") or "" - for item_comment in (item.get("comments") or []) + for item_comment in item_comments ), - } - for item in top_level_items - if (item.get("root_timestamp") or "") < timestamp - ] + }) items.append( _add_discussion_facts( { @@ -315,9 +369,43 @@ def _derive_top_level_author_comment_items( return items -def prepare_discussions(source: DiscussionInput) -> PreparedDiscussions: +def _top_level_feedback_timestamp(discussion: dict[str, Any]) -> str: + comments = discussion.get("comments") or () + root_comment = comments[0] if comments else {} + return ( + root_comment.get("activity_timestamp") + or discussion.get("root_timestamp") + or "" + ) + + +def _top_level_items_after_cutoff( + items: list[dict[str, Any]], + cutoff_value: str, +) -> list[dict[str, Any]]: + cutoff = parse_ts(cutoff_value) + if cutoff is None: + return items + return [ + item + for item in items + if ( + (activity := parse_ts(_top_level_feedback_timestamp(item))) is None + or activity > cutoff + ) + ] + + +def prepare_discussions( + source: DiscussionInput, + *, + top_level_feedback_cutoff: str = "", +) -> PreparedDiscussions: review_threads = _group_review_threads(source) - top_level_items = _derive_top_level_items(source) + top_level_items = _top_level_items_after_cutoff( + _derive_top_level_items(source), + top_level_feedback_cutoff, + ) top_level_author_comment_items = _derive_top_level_author_comment_items( source, top_level_items, @@ -340,19 +428,25 @@ def _filter_handoff_feedback( if ( comment.get("actor_role") in _HUMAN_REVIEWER_ROLES and comment.get("actor") != pr_author - and after_cutoff(comment.get("timestamp") or "") + and after_cutoff( + comment.get("activity_timestamp") + or comment.get("timestamp") + or "" + ) ) ] if not comments: return None + latest_activity_comment = _latest_activity_comment(comments) filtered = {**discussion, "comments": comments} - filtered["requester"] = comments[-1].get("actor") or "" + filtered["requester"] = latest_activity_comment.get("actor") or "" filtered["pr_author"] = pr_author return _add_discussion_facts( filtered, comments, (discussion.get("discussion_facts") or {}).get("current_conflicts") or "unknown", + latest_comment=latest_activity_comment, ) @@ -361,7 +455,7 @@ def reviewer_handoff_feedback( override_since: str, pr_author: str, ) -> PreparedDiscussions: - """Select human reviewer feedback created after a reviewer handoff command.""" + """Select human reviewer feedback changed after a reviewer handoff command.""" cutoff = parse_ts(override_since) if cutoff is None: return PreparedDiscussions((), (), ()) @@ -397,7 +491,7 @@ def prepare_reviewer_handoff_feedback( pr_author: str, ) -> PreparedDiscussions: prepared = PreparedDiscussions( - tuple(_group_review_threads(source, include_inactive=True)), + tuple(_group_review_threads(source)), tuple(_derive_top_level_items(source)), (), ) @@ -580,13 +674,13 @@ def _collect_author_evidence( author_comment_outcomes: list[AuthorCommentOutcome], author_comment_source_state: AuthorCommentSourceState | None, ) -> tuple[dict[str, str], int | None]: - root_timestamp = discussion.get("root_timestamp") or "" + feedback_timestamp = _top_level_feedback_timestamp(discussion) evidence: dict[str, str] = {} reply_source_id: int | None = None previous_reply = (previous_entry.get("evidence") or {}).get("reply") or "" previous_reply_source_id = previous_entry.get("reply_source_id") if ( - previous_reply > root_timestamp + previous_reply > feedback_timestamp and _should_restore_author_reply( author_comment_outcomes, author_comment_source_state, @@ -602,7 +696,7 @@ def _collect_author_evidence( completed_reply = _completed_author_reply_after( discussion["discussion_id"], - root_timestamp, + feedback_timestamp, author_comment_outcomes, ) if completed_reply: @@ -636,9 +730,13 @@ def _review_thread_pending_actions( f"({classification.identity.kind.value}) requires " f"ActionDecision, got {type(decision).__name__}" ) - action = decision.action discussion_id = classification.identity.discussion_id discussion = by_id.get(discussion_id) + action = ( + DiscussionAction.AUTHOR + if (discussion or {}).get("strict_author_action") + else decision.action + ) comments = (discussion or {}).get("comments") or [] if action is not DiscussionAction.NONE and comments: entry = { @@ -651,6 +749,10 @@ def _review_thread_pending_actions( } if classification.ignored_last_comment: entry["ignored_last_comment"] = True + if classification.ignored_comment_index is not None: + entry["ignored_comment_index"] = ( + classification.ignored_comment_index + ) pending_actions[discussion_id] = entry return pending_actions @@ -679,6 +781,7 @@ def _advance_top_level_actions( ) action = decision.action root_timestamp = discussion.get("root_timestamp") or "" + feedback_timestamp = _top_level_feedback_timestamp(discussion) if action not in ( DiscussionAction.AUTHOR, DiscussionAction.UNCLEAR, @@ -701,7 +804,7 @@ def _advance_top_level_actions( continue handoff = _latest_author_comment_handoff( discussion["discussion_id"], - root_timestamp, + feedback_timestamp, author_comment_outcomes, ) if handoff is not None: diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index f69b3cdfda2..fe2b3b1adc6 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -179,6 +179,7 @@ def gh_pr_view(repo: str, number: int) -> dict[str, Any]: state submittedAt updatedAt + lastEditedAt author { login } @@ -296,6 +297,11 @@ def fetch_pr_reviews(owner: str, repo_name: str, number: int) -> list[dict[str, "body": review.get("body") or "", "submitted_at": review.get("submittedAt") or "", "updated_at": review.get("updatedAt") or "", + "content_updated_at": ( + review.get("lastEditedAt") + or review.get("submittedAt") + or "" + ), }) page_info = connection.get("pageInfo") or {} if not page_info.get("hasNextPage"): @@ -827,6 +833,7 @@ def load_reviewer_set(org: str, approver_team_slugs: list[str]) -> set[str]: url body createdAt + lastEditedAt author { login } @@ -863,6 +870,7 @@ def load_reviewer_set(org: str, approver_team_slugs: list[str]) -> set[str]: url body createdAt + lastEditedAt author { login } diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 8b77ed8ef82..9d0ac0129ef 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -14,9 +14,11 @@ ) from dashboard_override import ( PRE_REVIEW_ROUTES, + acknowledged_top_level_feedback_cutoff, acknowledged_override, acknowledges_override, override_ack_marker, + top_level_feedback_cutoff_marker, ) from dashboard_contracts import ( DashboardFacts, @@ -379,6 +381,13 @@ def render_status_comment( facts.dashboard_override_since, ), ) + if facts.dashboard_top_level_feedback_cutoff: + optional_markers.append( + top_level_feedback_cutoff_marker( + bound_command_id, + facts.dashboard_top_level_feedback_cutoff, + ) + ) optional_markers.append( reviewer_handoff_cleared_marker(bound_command_id, bound_head), ) @@ -468,10 +477,17 @@ def upsert_status_comment( if comments: comment = comments[0] if preserve_clearance: + normalized_comments = normalize_issue_comments(comments) command_id, head_sha = status_reviewer_handoff_clearance(comments) clearance_marker = reviewer_handoff_cleared_marker(command_id, head_sha) acknowledged_id, acknowledged_head, since, _ = acknowledged_override( - normalize_issue_comments(comments) + normalized_comments + ) + top_level_feedback_cutoff = ( + acknowledged_top_level_feedback_cutoff( + normalized_comments, + command_id, + ) ) acknowledgement_marker = override_ack_marker( command_id, @@ -489,6 +505,20 @@ def upsert_status_comment( preserved_markers.append(acknowledgement_marker) if clearance_marker not in body: preserved_markers.append(clearance_marker) + if ( + top_level_feedback_cutoff + and top_level_feedback_cutoff_marker( + command_id, + top_level_feedback_cutoff, + ) + not in body + ): + preserved_markers.append( + top_level_feedback_cutoff_marker( + command_id, + top_level_feedback_cutoff, + ) + ) if preserved_markers: lines = body.splitlines() lines[2:2] = preserved_markers diff --git a/.github/scripts/pull-request-dashboard/pull_request_activity.py b/.github/scripts/pull-request-dashboard/pull_request_activity.py index b3fbf89d374..df866b935c9 100644 --- a/.github/scripts/pull-request-dashboard/pull_request_activity.py +++ b/.github/scripts/pull-request-dashboard/pull_request_activity.py @@ -119,11 +119,7 @@ def _issue_comment_event( else comment.body ) login = reviewer_actor_login(comment.actor) - timestamp = ( - comment.content_updated_at - or comment.created_at - or comment.updated_at - ) + timestamp = comment.effective_content_timestamp return { "source_id": comment.database_id or None, "discussion_url": comment.url, @@ -174,6 +170,7 @@ def _review_event( "discussion_url": review.url, "kind": "review-state", "timestamp": review.submitted_at, + "content_timestamp": review.effective_content_timestamp, "created_timestamp": review.submitted_at, "actor": login, "actor_role": role_for(login, author, approver_logins), @@ -224,7 +221,7 @@ def _latest_substantive_activity( actor_roles: set[str], ) -> datetime | None: timestamps = [ - parse_ts(event["timestamp"]) + parse_ts(event.get("content_timestamp") or event["timestamp"]) for event in events if event.get("actor_role") in actor_roles and is_substantive_activity(event) diff --git a/.github/scripts/pull-request-dashboard/pull_request_evaluation.py b/.github/scripts/pull-request-dashboard/pull_request_evaluation.py index 604fe04beb6..b02be5056f0 100644 --- a/.github/scripts/pull-request-dashboard/pull_request_evaluation.py +++ b/.github/scripts/pull-request-dashboard/pull_request_evaluation.py @@ -257,6 +257,8 @@ def _compute_facts( dashboard_override_bound_command_id=override.bound_command_id, dashboard_override_head_sha=override.head_sha, dashboard_override_since=override.since, + dashboard_top_level_feedback_cutoff=override.top_level_feedback_cutoff, + dashboard_override_persistent=override.persistent_handoff, dashboard_override_cleared_by_feedback=override.cleared_by_feedback, dashboard_command_replies=override.command_replies, copilot_review_requested=any( @@ -492,7 +494,10 @@ def evaluate_pull_request( config.approver_logins, facts.conflicts, ) - prepared_discussions = prepare_discussions(discussion_input) + prepared_discussions = prepare_discussions( + discussion_input, + top_level_feedback_cutoff=facts.dashboard_top_level_feedback_cutoff, + ) if manual_reviewer_handoff: # Old discussions cannot block a break-glass handoff. Only newer # human feedback is classified to decide whether the reviewer has diff --git a/.github/scripts/pull-request-dashboard/pull_request_source.py b/.github/scripts/pull-request-dashboard/pull_request_source.py index 4701f26586a..9b1adddf691 100644 --- a/.github/scripts/pull-request-dashboard/pull_request_source.py +++ b/.github/scripts/pull-request-dashboard/pull_request_source.py @@ -18,7 +18,7 @@ required_code_scanning_checks, unreported_required_contexts, ) -from utils import is_copilot_reviewer_login +from utils import is_copilot_reviewer_login, parse_ts def gh_pr_view(repo: str, number: int) -> dict[str, Any]: @@ -213,6 +213,18 @@ class IssueComment: actor: Actor = field(default_factory=Actor) performed_via_app_slug: str = "" + @property + def effective_content_timestamp(self) -> str: + created_at = parse_ts(self.created_at) + content_updated_at = parse_ts(self.content_updated_at) + if content_updated_at is not None and ( + created_at is None or content_updated_at >= created_at + ): + return self.content_updated_at + if created_at is not None: + return self.created_at + return self.updated_at if parse_ts(self.updated_at) is not None else "" + def is_from_app(self, app_slug: str) -> bool: return ( self.performed_via_app_slug == app_slug @@ -242,6 +254,17 @@ class Review: body: str = "" submitted_at: str = "" updated_at: str = "" + content_updated_at: str = "" + + @property + def effective_content_timestamp(self) -> str: + submitted_at = parse_ts(self.submitted_at) + content_updated_at = parse_ts(self.content_updated_at) + if content_updated_at is not None and ( + submitted_at is None or content_updated_at >= submitted_at + ): + return self.content_updated_at + return self.submitted_at @dataclass(frozen=True) @@ -273,9 +296,20 @@ class ReviewThreadComment: url: str = "" body: str = "" created_at: str = "" + updated_at: str = "" actor: Actor = field(default_factory=Actor) reaction_groups: tuple[ReactionGroup, ...] = () + @property + def effective_content_timestamp(self) -> str: + created_at = parse_ts(self.created_at) + updated_at = parse_ts(self.updated_at) + if updated_at is not None and ( + created_at is None or updated_at >= created_at + ): + return self.updated_at + return self.created_at + def __post_init__(self) -> None: object.__setattr__( self, @@ -583,6 +617,12 @@ def normalize_reviews(values: Any) -> tuple[Review, ...]: item.get("submitted_at") or item.get("submittedAt") ), updated_at=_text(item.get("updated_at") or item.get("updatedAt")), + content_updated_at=_text( + item.get("content_updated_at") + or item.get("lastEditedAt") + or item.get("submitted_at") + or item.get("submittedAt") + ), )) return tuple(reviews) @@ -629,6 +669,13 @@ def normalize_review_threads(values: Any) -> tuple[ReviewThread, ...]: created_at=_text( comment.get("createdAt") or comment.get("created_at") ), + updated_at=_text( + comment.get("lastEditedAt") + or comment.get("updatedAt") + or comment.get("updated_at") + or comment.get("createdAt") + or comment.get("created_at") + ), actor=normalize_actor( comment.get("author") or comment.get("user") ), diff --git a/.github/scripts/pull-request-dashboard/reviewer_state.py b/.github/scripts/pull-request-dashboard/reviewer_state.py index adbac1a6599..4db14ced3f4 100644 --- a/.github/scripts/pull-request-dashboard/reviewer_state.py +++ b/.github/scripts/pull-request-dashboard/reviewer_state.py @@ -143,7 +143,19 @@ def _reviewers_with_open_threads( continue comments = discussion.get("comments") or [] if entry.get("ignored_last_comment"): - comments = comments[:-1] + ignored_index = entry.get("ignored_comment_index") + if ( + isinstance(ignored_index, int) + and not isinstance(ignored_index, bool) + and 0 <= ignored_index < len(comments) + ): + comments = [ + comment + for index, comment in enumerate(comments) + if index != ignored_index + ] + else: + comments = comments[:-1] for comment in comments: if ( comment.get("actor_role") in ("approver", "outsider", "bot") diff --git a/.github/scripts/pull-request-dashboard/routing_decision.py b/.github/scripts/pull-request-dashboard/routing_decision.py index a9d4ce21db0..d60c6cffbe7 100644 --- a/.github/scripts/pull-request-dashboard/routing_decision.py +++ b/.github/scripts/pull-request-dashboard/routing_decision.py @@ -54,10 +54,14 @@ class RoutingOutcome: def reviewer_handoff_active(facts: DashboardFacts) -> bool: - """Return whether the uncleared reviewer handoff matches the current head.""" + """Return whether the reviewer handoff remains active.""" return ( bool(facts.dashboard_override_head_sha) - and facts.dashboard_override_head_sha == facts.head_sha + and bool(facts.head_sha) + and ( + facts.dashboard_override_persistent + or facts.dashboard_override_head_sha == facts.head_sha + ) and not facts.dashboard_override_cleared_by_feedback ) diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 4d9499c3dee..7f35e7145a4 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -36,7 +36,11 @@ # current vector, ordinary state loaders may regenerate mismatched disposable # caches. Every constant ending in _STATE_VERSION or _REVISION is included. # dashboard-state.json: accepted PR routing results and backfill readiness. -DASHBOARD_STATE_VERSION = 16 +# Versions 14 and 15 describe incompatible state shapes from parallel changes. +# Version 16 is the pre-persistent-handoff lifecycle shape. An integration that +# combines the incompatible shapes must assign another version. +DASHBOARD_STATE_VERSION = 17 +DASHBOARD_STATE_COMPATIBLE_VERSIONS = (11, 12, 13, 16) # backfill-state.json: round-robin cursor used by full dashboard refreshes. BACKFILL_STATE_VERSION = 3 # notification-state.json: pending and delivered Slack notification records. @@ -49,7 +53,7 @@ STATUS_COMMENT_ROLLOUT_STATE_VERSION = 2 # Rendered status-comment behavior. Increment when existing comments need to # adopt a change; hourly runs durably roll it out to all open PRs. -STATUS_COMMENT_REVISION = 19 +STATUS_COMMENT_REVISION = 20 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None @@ -415,6 +419,14 @@ def _decode_command_reply(value: Any) -> DashboardCommandReply: value.get("since", _MISSING), "facts.dashboard_command_replies.since", ), + top_level_feedback_cutoff=_string( + value.get("top_level_feedback_cutoff", _MISSING), + "facts.dashboard_command_replies.top_level_feedback_cutoff", + ), + persistent_handoff=_boolean( + value.get("persistent_handoff", _MISSING), + "facts.dashboard_command_replies.persistent_handoff", + ), ) @@ -428,6 +440,12 @@ def _encode_command_reply(reply: DashboardCommandReply) -> dict[str, Any]: } if reply.since: stored["since"] = reply.since + if reply.top_level_feedback_cutoff: + stored["top_level_feedback_cutoff"] = ( + reply.top_level_feedback_cutoff + ) + if reply.persistent_handoff: + stored["persistent_handoff"] = True if reply.kind == "routed": # DashboardCommandReply refuses a routed reply without a route. stored["route"] = reply.route.value @@ -490,6 +508,14 @@ def decode_dashboard_facts(value: Any) -> DashboardFacts: value.get("dashboard_override_since", _MISSING), "facts.dashboard_override_since", ), + dashboard_top_level_feedback_cutoff=_string( + value.get("dashboard_top_level_feedback_cutoff", _MISSING), + "facts.dashboard_top_level_feedback_cutoff", + ), + dashboard_override_persistent=_boolean( + value.get("dashboard_override_persistent", _MISSING), + "facts.dashboard_override_persistent", + ), dashboard_override_cleared_by_feedback=_boolean( value.get("dashboard_override_cleared_by_feedback", _MISSING), "facts.dashboard_override_cleared_by_feedback", @@ -680,6 +706,12 @@ def encode_dashboard_facts(facts: DashboardFacts) -> dict[str, Any]: ) if facts.dashboard_override_since: stored["dashboard_override_since"] = facts.dashboard_override_since + if facts.dashboard_top_level_feedback_cutoff: + stored["dashboard_top_level_feedback_cutoff"] = ( + facts.dashboard_top_level_feedback_cutoff + ) + if facts.dashboard_override_persistent: + stored["dashboard_override_persistent"] = True if facts.dashboard_override_cleared_by_feedback: stored["dashboard_override_cleared_by_feedback"] = True if facts.ci_failing_count is not None: @@ -827,6 +859,7 @@ def load_dashboard_state_cache() -> DashboardState | None: state = load_state_file( dashboard_state_path(), DASHBOARD_STATE_VERSION, + compatible_versions=DASHBOARD_STATE_COMPATIBLE_VERSIONS, ) if state is None: return None diff --git a/.github/scripts/pull-request-dashboard/test_classification_execution.py b/.github/scripts/pull-request-dashboard/test_classification_execution.py index 64e7a143635..e82ae413890 100644 --- a/.github/scripts/pull-request-dashboard/test_classification_execution.py +++ b/.github/scripts/pull-request-dashboard/test_classification_execution.py @@ -1063,6 +1063,20 @@ def test_long_reviewer_request_needs_no_model(self) -> None: assert isinstance(result.decision, ActionDecision) self.assertIs(result.decision.action, DiscussionAction.AUTHOR) + def test_unresolved_copilot_finding_does_not_use_reply_classifier(self) -> None: + thread = self.thread( + ("bot", "Please fix this.", "2026-03-12T00:00:00Z"), + ("author", "Fixed it.", "2026-05-20T00:00:00Z"), + ) + thread["strict_author_action"] = True + + result, runner = self.classify(thread) + + self.assertEqual(runner.requests, []) + assert isinstance(result.decision, ActionDecision) + self.assertIs(result.decision.action, DiscussionAction.AUTHOR) + self.assertEqual(result.since, "2026-05-20T00:00:00Z") + def test_praise_keeps_the_previous_request_and_wait_age(self) -> None: result, runner = self.classify(self.thread( ("approver", "Please fix this.", "2026-03-12T00:00:00Z"), @@ -1096,6 +1110,102 @@ def test_praise_after_completed_author_reply_hands_back_to_reviewer( self.assertEqual(result.since, "2026-03-12T00:00:00Z") self.assertTrue(result.ignored_last_comment) + def test_edited_older_praise_is_classified_and_removed_by_identity( + self, + ) -> None: + thread = self.thread( + ("approver", "Please fix this.", "2026-03-12T00:00:00Z"), + ("approver", "LGTM", "2026-04-12T00:00:00Z"), + ("author", "Fixed it.", "2026-05-20T00:00:00Z"), + ) + thread["comments"][1]["activity_timestamp"] = "2026-06-20T00:00:00Z" + + result, runner = self.classify( + thread, + responder=lambda request: successful_response( + request, + praise="praise", + author_reply="complete", + ), + ) + + self.assertEqual(len(runner.requests), 2) + self.assertEqual(prompt_items(runner.requests[0])[0]["body"], "LGTM") + self.assertEqual( + prompt_items(runner.requests[1])[0]["body"], + "Fixed it.", + ) + assert isinstance(result.decision, ActionDecision) + self.assertIs(result.decision.action, DiscussionAction.REVIEWER) + self.assertEqual(result.since, "2026-05-20T00:00:00Z") + self.assertTrue(result.ignored_last_comment) + + def test_cached_praise_recomputes_ignored_comment_index(self) -> None: + thread = self.thread( + ("approver", "Please fix this.", "2026-03-12T00:00:00Z"), + ("approver", "LGTM", "2026-04-12T00:00:00Z"), + ("author", "Fixed it.", "2026-05-20T00:00:00Z"), + ) + thread["comments"][1]["activity_timestamp"] = "2026-06-20T00:00:00Z" + request = execution_request(review_threads=(thread,)) + cache = MemoryClassificationCacheStore() + first_runner = FakeModelRunner( + responder=lambda model_request: successful_response( + model_request, + praise="praise", + author_reply="complete", + ) + ) + ClassificationService(first_runner, cache).classify(request) + cached_runner = FakeModelRunner() + + result = ClassificationService(cached_runner, cache).classify( + request + ).review_threads[0] + + self.assertEqual(cached_runner.requests, []) + self.assertTrue(result.ignored_last_comment) + self.assertEqual(result.ignored_comment_index, 1) + + def test_edited_older_author_reply_supplies_body_and_result_time( + self, + ) -> None: + thread = self.thread( + ( + "approver", + "Please update the implementation and tests.", + "2026-03-12T00:00:00Z", + ), + ("author", "The complete fix is ready.", "2026-04-12T00:00:00Z"), + ("author", "One part remains.", "2026-05-20T00:00:00Z"), + ) + thread["comments"][1]["activity_timestamp"] = "2026-06-20T00:00:00Z" + + result, runner = self.classify( + thread, + responder=lambda request: successful_response( + request, + author_reply="deferral", + ), + ) + + self.assertEqual(len(runner.requests), 1) + self.assertEqual( + prompt_items(runner.requests[0])[0]["body"], + "The complete fix is ready.", + ) + assert isinstance(result.decision, ActionDecision) + self.assertIs(result.decision.action, DiscussionAction.AUTHOR) + self.assertEqual(result.since, "2026-06-20T00:00:00Z") + self.assertEqual( + [comment["body"] for comment in thread["comments"]], + [ + "Please update the implementation and tests.", + "The complete fix is ready.", + "One part remains.", + ], + ) + def test_failed_praise_and_author_reply_calls_fail_safe_to_author( self, ) -> None: diff --git a/.github/scripts/pull-request-dashboard/test_classification_policy.py b/.github/scripts/pull-request-dashboard/test_classification_policy.py index cd226947f1f..0b90f7c25a3 100644 --- a/.github/scripts/pull-request-dashboard/test_classification_policy.py +++ b/.github/scripts/pull-request-dashboard/test_classification_policy.py @@ -57,6 +57,7 @@ def discussion( requester: str = "", pr_author: str = "", candidate_feedback: tuple[tuple[str, str], ...] = (), + strict_author_action: bool = False, ) -> ClassificationDiscussion: return ClassificationDiscussion( DiscussionIdentity(discussion_id, kind), @@ -73,6 +74,7 @@ def discussion( CandidateFeedback(feedback_id, feedback_body) for feedback_id, feedback_body in candidate_feedback ), + strict_author_action=strict_author_action, ) @@ -483,6 +485,7 @@ def test_success_projection_matches_the_dashboard_record(self) -> None: cli_call=True, since="2026-01-02T03:04:05Z", ignored_last_comment=True, + ignored_comment_index=1, ) self.assertEqual( @@ -498,8 +501,13 @@ def test_success_projection_matches_the_dashboard_record(self) -> None: "_copilot_cli_call": True, "since": "2026-01-02T03:04:05Z", "ignored_last_comment": True, + "ignored_comment_index": 1, }, ) + self.assertNotIn( + "ignored_comment_index", + cached_classification_record(result), + ) def test_failure_and_cache_projections_preserve_the_old_shape(self) -> None: result = ClassificationFailure( @@ -705,6 +713,34 @@ def test_long_reviewer_comment_needs_no_model_request(self) -> None: assert isinstance(result.decision, ActionDecision) self.assertEqual(result.decision.action, DiscussionAction.AUTHOR) + def test_unresolved_copilot_finding_bypasses_author_reply_policy(self) -> None: + thread = ClassificationDiscussion( + DiscussionIdentity("thread-1", DiscussionKind.REVIEW_THREAD), + ( + DiscussionComment( + "2026-01-02T03:04:05Z", + "bot", + "Please fix this.", + ), + DiscussionComment( + "2026-01-02T04:05:06Z", + "author", + "Done.", + ), + ), + strict_author_action=True, + ) + + self.assertEqual(prepare_praise_candidates([thread]), ()) + plan = resolve_review_thread_policy([thread], {}) + + self.assertEqual(plan.author_replies, ()) + result = plan.resolved[0] + self.assertIsInstance(result, ClassificationSuccess) + assert isinstance(result.decision, ActionDecision) + self.assertEqual(result.decision.action, DiscussionAction.AUTHOR) + self.assertEqual(result.since, "2026-01-02T04:05:06Z") + def test_praise_and_author_reply_shortcuts_preserve_handoffs(self) -> None: thread = discussion( "thread-1", diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index 50551df5719..417da2978fa 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -9,7 +9,9 @@ from copilot_review import ( REQUEST_CONFIRMATION_ATTEMPTS, + copilot_review_status, copilot_first_review_overdue, + open_copilot_finding_urls, set_copilot_first_review_missing_since, set_copilot_review_request_needed, stale_request_reason, @@ -19,7 +21,11 @@ record_copilot_review_observation, ) from dashboard_test_support import ( + actor, dashboard_facts, + review_source, + review_thread, + review_thread_comment, stored_dashboard_result, ) from routing_snapshot import build_routing_snapshot @@ -55,6 +61,55 @@ def review_result(route: str = "approver", **fact_changes): ) +class CopilotFindingLifecycleTest(unittest.TestCase): + def test_clean_current_review_keeps_open_findings(self) -> None: + reviews = (review_source( + actor=actor("copilot"), + commit_id="current-head", + finding_count=0, + submitted_at="2026-07-20T02:00:00Z", + ),) + threads = ( + review_thread( + node_id="open", + comments=(review_thread_comment( + url="https://example.test/open", + actor=actor("copilot"), + created_at="2026-07-20T01:00:00Z", + updated_at="2026-07-20T01:00:00Z", + ),), + ), + review_thread( + node_id="resolved", + is_resolved=True, + comments=(review_thread_comment( + url="https://example.test/resolved", + actor=actor("copilot"), + created_at="2026-07-20T01:00:00Z", + updated_at="2026-07-20T01:00:00Z", + ),), + ), + review_thread( + node_id="outdated", + is_outdated=True, + comments=(review_thread_comment( + url="https://example.test/outdated", + actor=actor("copilot"), + created_at="2026-07-20T01:00:00Z", + updated_at="2026-07-20T01:00:00Z", + ),), + ), + ) + + self.assertEqual( + (True, False, True), + copilot_review_status(reviews, "current-head", threads), + ) + self.assertEqual( + ("https://example.test/open",), + open_copilot_finding_urls(threads), + ) + class CopilotFirstReviewMissingSinceTest(unittest.TestCase): def test_starts_clock_when_review_is_missing(self) -> None: facts = set_copilot_first_review_missing_since( diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index dc959f4e793..daefbc29806 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -53,6 +53,10 @@ review_thread_comment, stored_dashboard_result, ) +from classification_execution import ( + ClassificationExecutionRequest, + ClassificationService, +) from classification_policy import ( ActionDecision, AuthorCommentDecision, @@ -64,8 +68,14 @@ DiscussionIdentity, DiscussionKind, FeedbackOutcome, + resolve_review_thread_policy, +) +from classification_test_support import ( + FakeClassificationOperation, + FakeModelRunner, + MemoryClassificationCacheStore, + successful_response, ) -from classification_test_support import FakeClassificationOperation from pull_request_source import ( PullRequestSource, fetch_pull_request_source, @@ -167,6 +177,16 @@ def evaluate_pr( ) +class ReviewThreadPolicyOperation(FakeClassificationOperation): + def classify( + self, + request: ClassificationExecutionRequest, + ) -> DiscussionClassifications: + self.requests.append(request) + plan = resolve_review_thread_policy(request.review_threads, {}) + return DiscussionClassifications(plan.resolved, (), ()) + + class AuthorNudgeEpisodeTest(unittest.TestCase): def test_preserves_episode_while_route_remains_author(self) -> None: facts = _assign_author_nudge_episode( @@ -400,6 +420,43 @@ def test_compute_facts_uses_prepared_approval_count(self) -> None: self.assertEqual(1, facts.approval_count) + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_edited_old_changes_request_does_not_displace_newer_approval( + self, + fetch_raw: Mock, + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(), + reviews=( + review_source( + database_id=1, + state="CHANGES_REQUESTED", + body="", + submitted_at="2026-08-16T07:00:00Z", + content_updated_at="2026-08-16T09:00:00Z", + ), + review_source( + database_id=2, + state="APPROVED", + body="", + submitted_at="2026-08-16T08:00:00Z", + ), + ), + ) + + result = evaluate_pr({"number": 7}) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(1, result.facts.approval_count) + reviewer = next( + reviewer + for reviewer in result.facts.reviewers + if reviewer.login == "reviewer" + ) + self.assertTrue(reviewer.approved) + self.assertFalse(reviewer.changes_requested) + @patch("pull_request_evaluation.resolve_routing", wraps=resolve_routing) @patch("pull_request_evaluation.fetch_pull_request_source") def test_evaluation_routes_pending_reviewers_and_projects_reviewer_rows( @@ -435,16 +492,27 @@ def test_evaluation_routes_pending_reviewers_and_projects_reviewer_rows( ) @patch("pull_request_evaluation.fetch_pull_request_source") - def test_unresolved_copilot_thread_remains_an_author_action( + def test_unresolved_copilot_thread_with_completed_reply_remains_author_action( self, fetch_raw: Mock, ) -> None: thread_url = "https://example.test/review-comment/copilot" fetch_raw.return_value = pull_request_source( pull_request=pull_request_metadata(), - reviews=(review_source( - actor=actor("copilot"), - ),), + reviews=( + review_source( + actor=actor("copilot"), + commit_id="old-head", + finding_count=1, + submitted_at="2026-08-16T07:00:00Z", + ), + review_source( + actor=actor("copilot"), + commit_id="abcdef123456", + finding_count=0, + submitted_at="2026-08-16T08:00:00Z", + ), + ), review_threads=(review_thread( comments=( review_thread_comment( @@ -497,10 +565,78 @@ def test_unresolved_copilot_thread_remains_an_author_action( self.assertIsInstance(ungated_result, EvaluationSuccess) assert isinstance(ungated_result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, ungated_result.route) self.assertEqual( - (), + (thread_url,), ungated_result.facts.author_action_review_thread_urls, ) + self.assertEqual( + { + "PRRT_1": { + "action": "author", + "since": "2026-08-16T07:30:00Z", + }, + }, + ungated_result.pending_actions, + ) + self.assertIn( + ReviewerSummary( + login="copilot-pull-request-reviewer[bot]", + open_thread=True, + ), + ungated_result.facts.reviewers, + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_resolved_or_outdated_copilot_thread_releases_author_hold( + self, + fetch_raw: Mock, + ) -> None: + for state in ({"is_resolved": True}, {"is_outdated": True}): + with self.subTest(state=state): + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(), + reviews=( + review_source( + actor=actor("copilot"), + commit_id="old-head", + finding_count=1, + submitted_at="2026-08-16T07:00:00Z", + ), + review_source( + actor=actor("copilot"), + commit_id="abcdef123456", + finding_count=0, + submitted_at="2026-08-16T08:00:00Z", + ), + ), + review_threads=(review_thread( + comments=( + review_thread_comment(actor=actor("copilot")), + review_thread_comment( + node_id="PRRC_2", + body="Handled in a follow-up.", + created_at="2026-08-16T07:30:00Z", + actor=actor("author"), + ), + ), + **state, + ),), + ) + + result = evaluate_pr( + {"number": 7}, + require_clean_copilot_review_branches=["main"], + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.APPROVER, result.route) + self.assertEqual({}, result.pending_actions) + self.assertEqual( + (), + result.facts.author_action_review_thread_urls, + ) @patch("pull_request_evaluation.fetch_pull_request_source") def test_override_binds_to_the_observed_head_before_classification( @@ -580,6 +716,8 @@ def test_override_binds_to_the_observed_head_before_classification( head_sha="abcdef123456", route=DashboardRoute.APPROVER, since="2026-08-16T08:00:00Z", + top_level_feedback_cutoff="2026-08-16T08:00:00Z", + persistent_handoff=True, ), ), result.facts.dashboard_command_replies, @@ -587,162 +725,767 @@ def test_override_binds_to_the_observed_head_before_classification( self.assertEqual(classifier.requests, []) @patch("pull_request_evaluation.fetch_pull_request_source") - def test_actionable_review_after_override_ends_handoff( - self, fetch_raw: Mock + def test_override_without_head_uses_non_persistent_acknowledgement( + self, + fetch_raw: Mock, ) -> None: fetch_raw.return_value = pull_request_source( - pull_request=pull_request_metadata(title="Routing integration"), - issue_comments=(issue_comment( - database_id=102, - body="/dashboard route:reviewers", - created_at="2026-08-16T08:00:00Z", - ),), - reviews=(review_source( - database_id=501, - url="https://example.test/pull/7#pullrequestreview-501", - state="COMMENTED", - submitted_at="2026-08-16T09:00:00Z", - body="Please update this.", - ),), + pull_request=pull_request_metadata(head_sha=""), + issue_comments=( + issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ), + ), + review_threads=( + review_thread( + node_id="thread-1", + comments=( + review_thread_comment( + body="Please update this.", + created_at="2026-08-16T09:00:00Z", + ), + ), + ), + ), ) - classification = action_classification( - "pr-review-501", - DiscussionKind.TOP_LEVEL_FEEDBACK, + "thread-1", + DiscussionKind.REVIEW_THREAD, DiscussionAction.AUTHOR, "The reviewer requested a change.", ) classifier = FakeClassificationOperation( - DiscussionClassifications((), (classification,), ()), - reviewer_feedback_result=(classification,), + DiscussionClassifications((classification,), (), ()) ) + result = evaluate_pr( {"number": 7}, classification_service=classifier, ) - self.assertIsNotNone(result) - assert result is not None self.assertIsInstance(result, EvaluationSuccess) assert isinstance(result, EvaluationSuccess) self.assertEqual(DashboardRoute.AUTHOR, result.route) - self.assertTrue(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual("", result.facts.dashboard_override_head_sha) + self.assertFalse(result.facts.dashboard_override_persistent) self.assertEqual( - { - "pr-review-501": { - "action": "author", - "since": "2026-08-16T09:00:00Z", - } - }, - result.pending_actions, + ( + DashboardCommandReply( + 102, + "routed", + "author", + route=DashboardRoute.AUTHOR, + since="2026-08-16T08:00:00Z", + top_level_feedback_cutoff="2026-08-16T08:00:00Z", + ), + ), + result.facts.dashboard_command_replies, ) - self.assertEqual((), result.facts.dashboard_command_replies) - self.assertEqual(len(classifier.requests), 1) - self.assertEqual(len(classifier.reviewer_feedback_requests), 1) @patch("pull_request_evaluation.fetch_pull_request_source") - def test_non_actionable_review_keeps_handoff( - self, fetch_raw: Mock + def test_persistent_handoff_without_head_defers_acknowledgement( + self, + fetch_raw: Mock, ) -> None: fetch_raw.return_value = pull_request_source( - pull_request=pull_request_metadata(title="Routing integration"), - issue_comments=(issue_comment( - database_id=102, - body="/dashboard route:reviewers", - created_at="2026-08-16T08:00:00Z", - ),), - reviews=(review_source( - database_id=501, - state="COMMENTED", - submitted_at="2026-08-16T09:00:00Z", - body="Nice work.", - ),), + pull_request=pull_request_metadata(head_sha=""), + issue_comments=( + issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ), + ), + review_threads=( + review_thread( + node_id="thread-1", + comments=( + review_thread_comment( + body="Please update this.", + created_at="2026-08-16T09:00:00Z", + ), + ), + ), + ), ) - classification = action_classification( - "pr-review-501", - DiscussionKind.TOP_LEVEL_FEEDBACK, - DiscussionAction.NONE, - "The reviewer left praise.", + "thread-1", + DiscussionKind.REVIEW_THREAD, + DiscussionAction.AUTHOR, + "The reviewer requested a change.", ) classifier = FakeClassificationOperation( - reviewer_feedback_result=(classification,) + DiscussionClassifications((classification,), (), ()) ) + result = evaluate_pr( {"number": 7}, + previous_result=stored_dashboard_result( + 7, + route=DashboardRoute.APPROVER, + facts=dashboard_facts( + dashboard_override_command_id=102, + dashboard_override_bound_command_id=102, + dashboard_override_command_user="author", + dashboard_override_head_sha="abcdef123456", + dashboard_override_since="2026-08-16T08:00:00Z", + dashboard_override_persistent=True, + dashboard_top_level_feedback_cutoff="2026-08-16T08:00:00Z", + ), + ), classification_service=classifier, ) - self.assertIsNotNone(result) - assert result is not None self.assertIsInstance(result, EvaluationSuccess) assert isinstance(result, EvaluationSuccess) - self.assertEqual(DashboardRoute.APPROVER, result.route) - self.assertFalse(result.facts.dashboard_override_cleared_by_feedback) - self.assertEqual({}, result.pending_actions) - self.assertEqual(classifier.requests, []) - self.assertEqual(len(classifier.reviewer_feedback_requests), 1) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertTrue(result.facts.dashboard_override_persistent) + self.assertEqual((), result.facts.dashboard_command_replies) - @patch("pull_request_evaluation.fetch_pull_request_source") - def test_feedback_classification_failure_does_not_block_handoff( - self, fetch_raw: Mock - ) -> None: fetch_raw.return_value = pull_request_source( - pull_request=pull_request_metadata(title="Routing integration"), - issue_comments=(issue_comment( - database_id=102, - body="/dashboard route:reviewers", - created_at="2026-08-16T08:00:00Z", - ),), - reviews=(review_source( - database_id=501, - state="COMMENTED", - submitted_at="2026-08-16T09:00:00Z", - body="Please update this.", - ),), - ) - - failed = ClassificationFailure( - DiscussionIdentity( - "pr-review-501", - DiscussionKind.TOP_LEVEL_FEEDBACK, - ), - ActionDecision( - DiscussionAction.AUTHOR, - "The reviewer requested a change.", - ), - ClassificationDiagnostics(error="model failed"), - ) - classifier = FakeClassificationOperation( - reviewer_feedback_result=(failed,) + pull_request=pull_request_metadata(head_sha="abcdef123456"), + issue_comments=fetch_raw.return_value.issue_comments, + review_threads=fetch_raw.return_value.review_threads, ) - result = evaluate_pr( + resumed = evaluate_pr( {"number": 7}, + previous_result=stored_dashboard_result( + 7, + route=result.route, + facts=result.facts, + ), classification_service=classifier, ) - self.assertIsNotNone(result) - assert result is not None - self.assertIsInstance(result, EvaluationSuccess) - assert isinstance(result, EvaluationSuccess) - self.assertEqual(DashboardRoute.APPROVER, result.route) - self.assertFalse(result.facts.dashboard_override_cleared_by_feedback) - self.assertEqual(classifier.requests, []) - self.assertEqual(len(classifier.reviewer_feedback_requests), 1) + self.assertIsInstance(resumed, EvaluationSuccess) + assert isinstance(resumed, EvaluationSuccess) + self.assertEqual(DashboardRoute.APPROVER, resumed.route) + self.assertEqual( + ( + DashboardCommandReply( + 102, + "routed", + "author", + head_sha="abcdef123456", + route=DashboardRoute.APPROVER, + since="2026-08-16T08:00:00Z", + top_level_feedback_cutoff="2026-08-16T08:00:00Z", + persistent_handoff=True, + ), + ), + resumed.facts.dashboard_command_replies, + ) @patch("pull_request_evaluation.fetch_pull_request_source") - def test_informational_inline_feedback_keeps_handoff( - self, fetch_raw: Mock + def test_actionable_feedback_after_push_ends_persistent_handoff( + self, + fetch_raw: Mock, ) -> None: fetch_raw.return_value = pull_request_source( - pull_request=pull_request_metadata(title="Routing integration"), - issue_comments=(issue_comment( - database_id=102, - body="/dashboard route:reviewers", - created_at="2026-08-16T08:00:00Z", - ),), + pull_request=pull_request_metadata(head_sha="new-head"), + issue_comments=( + issue_comment( + database_id=101, + actor=actor("reviewer"), + body="Please update the top-level documentation.", + created_at="2026-08-16T07:00:00Z", + ), + issue_comment( + database_id=102, + actor=actor("author"), + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + content_updated_at="2026-08-16T10:00:00Z", + ), + issue_comment( + database_id=103, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body=( + "\n" + "\n" + "" + ), + created_at="2026-08-16T08:01:00Z", + ), + issue_comment( + database_id=104, + url="https://example.test/comment/new", + actor=actor("reviewer"), + body="Please add a current example.", + created_at="2026-08-16T09:00:00Z", + ), + ), review_threads=(review_thread( - node_id="thread-1", + node_id="old-thread", + comments=(review_thread_comment( + url="https://example.test/review-comment/old", + created_at="2026-08-16T07:00:00Z", + ),), + ),), + ) + classifier = FakeClassificationOperation( + DiscussionClassifications( + ( + action_classification( + "old-thread", + DiscussionKind.REVIEW_THREAD, + DiscussionAction.AUTHOR, + "The unresolved thread still needs author action.", + ), + ), + ( + action_classification( + "pr-issue-comment-104", + DiscussionKind.TOP_LEVEL_FEEDBACK, + DiscussionAction.AUTHOR, + "The new top-level request needs author action.", + ), + ), + (), + ), + reviewer_feedback_result=( + action_classification( + "pr-issue-comment-104", + DiscussionKind.TOP_LEVEL_FEEDBACK, + DiscussionAction.AUTHOR, + "The new top-level request needs author action.", + ), + ), + ) + + result = evaluate_pr( + {"number": 7}, + previous_result=stored_dashboard_result( + 7, + route=DashboardRoute.AUTHOR, + top_level_history={ + "pr-issue-comment-101": { + "evidence": {"reply": "2026-08-16T07:30:00Z"}, + } + }, + ), + classification_service=classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertTrue(result.facts.dashboard_override_persistent) + self.assertTrue(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual( + { + "old-thread": { + "action": "author", + "since": "2026-08-16T07:00:00Z", + }, + "pr-issue-comment-104": { + "action": "author", + "since": "2026-08-16T09:00:00Z", + }, + }, + result.pending_actions, + ) + self.assertEqual( + ("https://example.test/comment/new",), + result.facts.author_action_top_level_feedback_urls, + ) + self.assertEqual( + ("https://example.test/review-comment/old",), + result.facts.author_action_review_thread_urls, + ) + self.assertEqual({}, result.top_level_history) + self.assertEqual( + ("pr-issue-comment-104",), + tuple( + discussion.identity.discussion_id + for discussion in classifier.requests[0].top_level_items + ), + ) + self.assertEqual( + ("old-thread",), + tuple( + discussion.identity.discussion_id + for discussion in classifier.requests[0].review_threads + ), + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_push_does_not_end_persistent_handoff(self, fetch_raw: Mock) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(head_sha="new-head"), + issue_comments=( + issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ), + issue_comment( + database_id=103, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body=( + "\n" + "\n" + "" + ), + ), + ), + review_threads=(review_thread( + node_id="old-thread", + comments=(review_thread_comment( + body="Please update this.", + created_at="2026-08-16T07:00:00Z", + ),), + ),), + ) + classifier = FakeClassificationOperation( + error=AssertionError("old discussions must remain suppressed") + ) + + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.APPROVER, result.route) + self.assertTrue(result.facts.dashboard_override_persistent) + self.assertFalse(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual({}, result.pending_actions) + self.assertEqual([], classifier.requests) + self.assertEqual([], classifier.reviewer_feedback_requests) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_new_reply_on_old_thread_ends_persistent_handoff( + self, + fetch_raw: Mock, + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(head_sha="new-head"), + issue_comments=( + issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ), + issue_comment( + database_id=103, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body=( + "\n" + "\n" + "" + ), + ), + ), + review_threads=(review_thread( + node_id="thread-1", + comments=( + review_thread_comment( + body="Please update this.", + created_at="2026-08-16T07:00:00Z", + ), + review_thread_comment( + body="This still needs the requested update.", + created_at="2026-08-16T09:00:00Z", + ), + ), + ),), + ) + classification = action_classification( + "thread-1", + DiscussionKind.REVIEW_THREAD, + DiscussionAction.AUTHOR, + "The reviewer renewed the request.", + ) + classifier = FakeClassificationOperation( + DiscussionClassifications((classification,), (), ()), + reviewer_feedback_result=(classification,), + ) + + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertTrue(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual(("thread-1",), tuple(result.pending_actions)) + self.assertEqual( + "author", + result.pending_actions["thread-1"]["action"], + ) + self.assertEqual( + "2026-08-16T09:00:00Z", + result.pending_actions["thread-1"]["since"], + ) + self.assertEqual( + ["This still needs the requested update."], + [ + comment.body + for comment in classifier.reviewer_feedback_requests[0] + .discussions[0] + .comments + ], + ) + self.assertEqual( + ["Please update this.", "This still needs the requested update."], + [ + comment.body + for comment in classifier.requests[0].review_threads[0].comments + ], + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_actionable_review_after_override_ends_handoff( + self, fetch_raw: Mock + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + issue_comments=(issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ),), + reviews=(review_source( + database_id=501, + url="https://example.test/pull/7#pullrequestreview-501", + state="COMMENTED", + submitted_at="2026-08-16T09:00:00Z", + body="Please update this.", + ),), + ) + + classification = action_classification( + "pr-review-501", + DiscussionKind.TOP_LEVEL_FEEDBACK, + DiscussionAction.AUTHOR, + "The reviewer requested a change.", + ) + classifier = FakeClassificationOperation( + DiscussionClassifications((), (classification,), ()), + reviewer_feedback_result=(classification,), + ) + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsNotNone(result) + assert result is not None + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertTrue(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual( + { + "pr-review-501": { + "action": "author", + "since": "2026-08-16T09:00:00Z", + } + }, + result.pending_actions, + ) + self.assertEqual((), result.facts.dashboard_command_replies) + self.assertEqual(len(classifier.requests), 1) + self.assertEqual(len(classifier.reviewer_feedback_requests), 1) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_actionable_review_edited_after_override_ends_handoff( + self, fetch_raw: Mock + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + issue_comments=(issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ),), + reviews=(review_source( + database_id=501, + url="https://example.test/pull/7#pullrequestreview-501", + state="COMMENTED", + submitted_at="2026-08-16T07:00:00Z", + updated_at="2026-08-16T09:00:00Z", + content_updated_at="2026-08-16T09:00:00Z", + body="Please update this.", + ),), + ) + classification = action_classification( + "pr-review-501", + DiscussionKind.TOP_LEVEL_FEEDBACK, + DiscussionAction.AUTHOR, + "The edited review requested a change.", + ) + classifier = FakeClassificationOperation( + DiscussionClassifications((), (classification,), ()), + reviewer_feedback_result=(classification,), + ) + + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertTrue(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual( + "2026-08-16T07:00:00Z", + result.pending_actions["pr-review-501"]["since"], + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_actionable_issue_feedback_after_override_ends_handoff( + self, fetch_raw: Mock + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + issue_comments=( + issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ), + issue_comment( + database_id=501, + actor=actor("reviewer"), + body="The test is unfortunately still failing here.", + created_at="2026-08-16T09:00:00Z", + ), + ), + ) + feedback = action_classification( + "pr-issue-comment-501", + DiscussionKind.TOP_LEVEL_FEEDBACK, + DiscussionAction.AUTHOR, + "The reviewer reported that the defect remains.", + ) + classifier = FakeClassificationOperation( + DiscussionClassifications((), (feedback,), ()), + reviewer_feedback_result=(feedback,), + ) + + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertTrue(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual((), result.facts.dashboard_command_replies) + self.assertEqual( + {"pr-issue-comment-501": { + "action": "author", + "since": "2026-08-16T09:00:00Z", + }}, + result.pending_actions, + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_completed_author_response_retires_only_matching_review_work( + self, fetch_raw: Mock + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + issue_comments=( + issue_comment( + database_id=301, + body="The requested audit is complete.", + created_at="2026-08-16T09:00:00Z", + ), + ), + reviews=( + review_source( + database_id=201, + actor=actor("reviewer"), + state="CHANGES_REQUESTED", + body="Please complete the audit.", + submitted_at="2026-08-16T08:00:00Z", + ), + review_source( + database_id=202, + actor=actor("other-reviewer"), + state="COMMENTED", + body="Please update the release note.", + submitted_at="2026-08-16T08:30:00Z", + ), + ), + ) + audit = action_classification( + "pr-review-201", + DiscussionKind.TOP_LEVEL_FEEDBACK, + DiscussionAction.AUTHOR, + "The review requested an audit.", + ) + release_note = action_classification( + "pr-review-202", + DiscussionKind.TOP_LEVEL_FEEDBACK, + DiscussionAction.AUTHOR, + "The review requested a release note.", + ) + reply = ClassificationSuccess( + DiscussionIdentity( + "pr-author-reply-301", + DiscussionKind.TOP_LEVEL_AUTHOR_REPLY, + ), + AuthorCommentDecision(( + FeedbackOutcome( + "pr-review-201", + DiscussionAction.NONE, + "The author completed the audit.", + ), + FeedbackOutcome( + "pr-review-202", + DiscussionAction.AUTHOR, + "The author did not mention the release note.", + ), + )), + ) + classifier = FakeClassificationOperation( + DiscussionClassifications((), (audit, release_note), (reply,)) + ) + + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertNotIn("pr-review-201", result.pending_actions) + self.assertEqual( + { + "action": "author", + "since": "2026-08-16T09:00:00Z", + }, + result.pending_actions["pr-review-202"], + ) + self.assertEqual( + {"reply": "2026-08-16T09:00:00Z"}, + result.top_level_history["pr-review-201"]["evidence"], + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_non_actionable_review_keeps_handoff( + self, fetch_raw: Mock + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + issue_comments=(issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ),), + reviews=(review_source( + database_id=501, + state="COMMENTED", + submitted_at="2026-08-16T09:00:00Z", + body="Nice work.", + ),), + ) + + classification = action_classification( + "pr-review-501", + DiscussionKind.TOP_LEVEL_FEEDBACK, + DiscussionAction.NONE, + "The reviewer left praise.", + ) + classifier = FakeClassificationOperation( + reviewer_feedback_result=(classification,) + ) + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsNotNone(result) + assert result is not None + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.APPROVER, result.route) + self.assertFalse(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual({}, result.pending_actions) + self.assertEqual(classifier.requests, []) + self.assertEqual(len(classifier.reviewer_feedback_requests), 1) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_feedback_classification_failure_does_not_block_handoff( + self, fetch_raw: Mock + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + issue_comments=(issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ),), + reviews=(review_source( + database_id=501, + state="COMMENTED", + submitted_at="2026-08-16T09:00:00Z", + body="Please update this.", + ),), + ) + + failed = ClassificationFailure( + DiscussionIdentity( + "pr-review-501", + DiscussionKind.TOP_LEVEL_FEEDBACK, + ), + ActionDecision( + DiscussionAction.AUTHOR, + "The reviewer requested a change.", + ), + ClassificationDiagnostics(error="model failed"), + ) + classifier = FakeClassificationOperation( + reviewer_feedback_result=(failed,) + ) + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsNotNone(result) + assert result is not None + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.APPROVER, result.route) + self.assertFalse(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual(classifier.requests, []) + self.assertEqual(len(classifier.reviewer_feedback_requests), 1) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_informational_inline_feedback_keeps_handoff( + self, fetch_raw: Mock + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + issue_comments=(issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ),), + review_threads=(review_thread( + node_id="thread-1", path="src/example.py", line=7, comments=( @@ -791,17 +1534,11 @@ def test_informational_inline_feedback_keeps_handoff( ) @patch("pull_request_evaluation.fetch_pull_request_source") - def test_inactive_inline_feedback_ends_handoff( + def test_inactive_edited_inline_feedback_keeps_handoff( self, fetch_raw: Mock ) -> None: - classification = action_classification( - "thread-1", - DiscussionKind.REVIEW_THREAD, - DiscussionAction.AUTHOR, - "The reviewer requested a change.", - ) classifier = FakeClassificationOperation( - reviewer_feedback_result=(classification,) + error=AssertionError("inactive threads must not be classified") ) for state in ({"is_resolved": True}, {"is_outdated": True}): with self.subTest(state=state): @@ -817,7 +1554,8 @@ def test_inactive_inline_feedback_ends_handoff( **state, comments=(review_thread_comment( body="Please update this.", - created_at="2026-08-16T09:00:00Z", + created_at="2026-08-16T07:30:00Z", + updated_at="2026-08-16T09:00:00Z", ),), ),), ) @@ -829,18 +1567,159 @@ def test_inactive_inline_feedback_ends_handoff( self.assertIsNotNone(result) assert result is not None - self.assertTrue( + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.APPROVER, result.route) + self.assertFalse( result.facts.dashboard_override_cleared_by_feedback ) - request = classifier.reviewer_feedback_requests[-1] - self.assertEqual( - ["Please update this."], - [comment.body for comment in request.discussions[0].comments], + self.assertEqual({}, result.pending_actions) + + self.assertEqual([], classifier.reviewer_feedback_requests) + self.assertEqual([], classifier.requests) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_edited_reviewer_request_after_author_reply_clears_to_author( + self, + fetch_raw: Mock, + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + issue_comments=(issue_comment( + database_id=102, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + ),), + review_threads=(review_thread( + node_id="thread-1", + comments=( + review_thread_comment( + actor=actor("root-reviewer"), + body="Please update the implementation and tests.", + created_at="2026-08-16T07:00:00Z", + updated_at="2026-08-16T09:00:00Z", + ), + review_thread_comment( + actor=actor("author"), + body="Handled in the latest commit.", + created_at="2026-08-16T07:30:00Z", + ), + ), + ),), + ) + classification = action_classification( + "thread-1", + DiscussionKind.REVIEW_THREAD, + DiscussionAction.AUTHOR, + "The edited request needs author action.", + ) + classifier = ReviewThreadPolicyOperation( + reviewer_feedback_result=(classification,) + ) + + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertTrue(result.facts.dashboard_override_cleared_by_feedback) + self.assertEqual( + { + "thread-1": { + "action": "author", + "since": "2026-08-16T09:00:00Z", + }, + }, + result.pending_actions, + ) + self.assertEqual(1, len(classifier.requests)) + thread = classifier.requests[0].review_threads[0] + self.assertEqual("root-reviewer", thread.requester) + self.assertEqual( + [ + "Please update the implementation and tests.", + "Handled in the latest commit.", + ], + [comment.body for comment in thread.comments], + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_edited_older_praise_attributes_remaining_thread_participants( + self, + fetch_raw: Mock, + ) -> None: + fetch_raw.return_value = pull_request_source( + pull_request=pull_request_metadata(title="Routing integration"), + review_threads=(review_thread( + node_id="thread-1", + comments=( + review_thread_comment( + actor=actor("root-reviewer"), + body="Please update the implementation and tests.", + created_at="2026-08-16T07:00:00Z", + ), + review_thread_comment( + actor=actor("praise-reviewer"), + body="LGTM", + created_at="2026-08-16T08:00:00Z", + updated_at="2026-08-16T10:00:00Z", + ), + review_thread_comment( + actor=actor("author"), + body="Fixed it.", + created_at="2026-08-16T09:00:00Z", + ), + ), + ),), + ) + classifier = ClassificationService( + FakeModelRunner( + responder=lambda request: successful_response( + request, + praise="praise", + author_reply="complete", ) - self.assertEqual((), classifier.requests[-1].review_threads) + ), + MemoryClassificationCacheStore(), + ) - self.assertEqual(2, len(classifier.reviewer_feedback_requests)) - self.assertEqual(2, len(classifier.requests)) + result = evaluate_pr( + {"number": 7}, + classification_service=classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.APPROVER, result.route) + self.assertEqual( + { + "thread-1": { + "action": "reviewer", + "since": "2026-08-16T09:00:00Z", + "ignored_last_comment": True, + "ignored_comment_index": 1, + }, + }, + result.pending_actions, + ) + self.assertEqual( + (ReviewerSummary(login="root-reviewer", open_thread=True),), + result.facts.reviewers, + ) + self.assertEqual( + [ + "Please update the implementation and tests.", + "LGTM", + "Fixed it.", + ], + [ + comment["body"] + for comment in result.diagnostics.review_threads[0]["comments"] + ], + ) @patch("pull_request_evaluation.fetch_pull_request_source") def test_author_reply_does_not_reactivate_cleared_handoff( @@ -1157,6 +2036,60 @@ def test_current_head_matches_latest_clean_copilot_review(self) -> None: self.assertTrue(facts.copilot_review_exists) self.assertFalse(facts.copilot_review_needed) + def test_current_clean_review_keeps_older_open_copilot_findings( + self, + ) -> None: + facts = evaluation_facts( + { + "pr": { + "updatedAt": "2026-07-20T03:00:00Z", + "createdAt": "2026-07-20T01:00:00Z", + "author": {"login": "author"}, + "assignees": [], + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + "headRefOid": "current-head", + }, + "reviews": [ + { + "id": 10, + "commit_id": "old-head", + "finding_count": 1, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T01:30:00Z", + }, + { + "id": 20, + "commit_id": "current-head", + "finding_count": 0, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T02:30:00Z", + }, + ], + "review_threads": [{ + "id": "thread-1", + "isResolved": False, + "isOutdated": False, + "comments": { + "nodes": [{ + "id": "comment-1", + "url": "https://example.com/1", + "body": "Please fix this.", + "createdAt": "2026-07-20T01:30:00Z", + "author": {"login": "copilot"}, + }], + }, + }], + "checks": [], + }, + "author", + [], + ) + + self.assertTrue(facts.copilot_review_exists) + self.assertFalse(facts.copilot_review_stale) + self.assertTrue(facts.copilot_review_needed) + def test_late_stale_review_does_not_replace_clean_current_head_review(self) -> None: facts = evaluation_facts( { diff --git a/.github/scripts/pull-request-dashboard/test_dashboard_contracts.py b/.github/scripts/pull-request-dashboard/test_dashboard_contracts.py index 7ec9c672098..1f0bd46a156 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard_contracts.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard_contracts.py @@ -32,6 +32,29 @@ def test_routed_reply_rejects_failure_routes(self) -> None: ): DashboardCommandReply(1, "routed", route=route) + def test_non_routed_reply_rejects_persistent_handoff(self) -> None: + with self.assertRaisesRegex( + ValueError, + "only routed replies may persist a reviewer handoff", + ): + DashboardCommandReply( + 1, + "unauthorized", + persistent_handoff=True, + ) + + def test_persistent_handoff_reply_rejects_author_route(self) -> None: + with self.assertRaisesRegex( + ValueError, + "persistent reviewer handoff replies require a reviewer route", + ): + DashboardCommandReply( + 1, + "routed", + route=DashboardRoute.AUTHOR, + persistent_handoff=True, + ) + class EvaluationResultContractTest(unittest.TestCase): def test_success_rejects_failure_routes(self) -> None: diff --git a/.github/scripts/pull-request-dashboard/test_dashboard_override.py b/.github/scripts/pull-request-dashboard/test_dashboard_override.py index 74e6e61cbad..2eff650a7e9 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard_override.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard_override.py @@ -1,10 +1,14 @@ from __future__ import annotations +import json +from pathlib import Path +import tempfile import unittest from unittest.mock import call, patch import dashboard_override import dashboard_override_delivery +import state from dashboard_contracts import DashboardCommandReply, DashboardRoute from dashboard_test_support import ( actor, @@ -14,6 +18,7 @@ stored_dashboard_result, ) from pull_request_source import IssueComment +from routing_decision import reviewer_handoff_active def override_input( @@ -32,6 +37,10 @@ def result_facts( "dashboard_override_bound_command_id": override.bound_command_id, "dashboard_override_head_sha": override.head_sha, "dashboard_override_since": override.since, + "dashboard_top_level_feedback_cutoff": ( + override.top_level_feedback_cutoff + ), + "dashboard_override_persistent": override.persistent_handoff, "dashboard_override_cleared_by_feedback": override.cleared_by_feedback, "dashboard_command_replies": override.command_replies, } @@ -44,7 +53,7 @@ def test_override_guidance_matches_pre_review_route(self) -> None: guidance = dashboard_override.author_override_guidance() self.assertIn("waiting on the author to waiting on reviewers", guidance) - self.assertIn("the head it sees when it reads the command", guidance) + self.assertIn("remains active across pushes", guidance) self.assertNotIn("immediately", guidance) def test_dashboard_command_body_remainder(self) -> None: @@ -292,6 +301,16 @@ def test_renders_command_replies(self) -> None: "routed", "author", route=DashboardRoute.APPROVER, + since="2026-08-16T08:00:00Z", + top_level_feedback_cutoff="2026-08-16T08:00:00Z", + ) + ) + conservative = dashboard_override.render_command_reply( + DashboardCommandReply( + 8, + "routed", + "author", + route=DashboardRoute.APPROVER, ) ) gate_held = dashboard_override.render_command_reply( @@ -326,6 +345,16 @@ def test_renders_command_replies(self) -> None: self.assertIn(dashboard_override.command_reply_marker(4), routed) self.assertIn(dashboard_override.override_ack_marker(4), routed) self.assertIn("@author, this pull request was routed to reviewers.", routed) + self.assertIn( + "Top-level feedback through this request will not return; unresolved " + "review threads remain open.", + routed, + ) + self.assertIn( + "No top-level feedback was retired because the dashboard could not " + "determine a safe command time; unresolved review threads remain open.", + conservative, + ) self.assertIn(dashboard_override.command_reply_marker(5), gate_held) self.assertIn( "@author, your reviewer-routing request was recorded; the reviewer " @@ -369,6 +398,7 @@ def test_ack_marker_records_the_bound_head(self) -> None: head_sha="abcdef123456", route=DashboardRoute.APPROVER, since="2026-08-16T08:00:00Z", + persistent_handoff=True, ) ) @@ -377,6 +407,16 @@ def test_ack_marker_records_the_bound_head(self) -> None: "7:abcdef123456:2026-08-16T08:00:00Z -->", body, ) + self.assertIn( + "", + body, + ) + self.assertIn( + "The handoff remains active across pushes until newer actionable " + "human feedback arrives.", + body, + ) def test_pending_command_binds_to_the_head_this_pass_observed(self) -> None: source = override_input( @@ -392,6 +432,44 @@ def test_pending_command_binds_to_the_head_this_pass_observed(self) -> None: self.assertEqual(5, facts.command_id) self.assertEqual("current-head", facts.head_sha) + self.assertTrue(facts.persistent_handoff) + + def test_persistent_marker_restores_handoff_after_push(self) -> None: + source = override_input(issue_comment( + database_id=9, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body="\n".join([ + dashboard_override.override_ack_marker(5, "bound-head"), + dashboard_override.persistent_handoff_marker(5, "bound-head"), + ]), + )) + + facts = dashboard_override.dashboard_override_facts( + source, + "author", + None, + "later-head", + ) + + self.assertEqual(5, facts.bound_command_id) + self.assertEqual("bound-head", facts.head_sha) + self.assertTrue(facts.persistent_handoff) + + def test_legacy_acknowledgement_remains_head_bound(self) -> None: + source = override_input(issue_comment( + database_id=9, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body=dashboard_override.override_ack_marker(5, "bound-head"), + )) + + facts = dashboard_override.dashboard_override_facts( + source, + "author", + None, + "later-head", + ) + + self.assertFalse(facts.persistent_handoff) def test_pending_command_keeps_its_first_observed_head(self) -> None: source = override_input( @@ -441,6 +519,10 @@ def test_pending_command_keeps_its_first_observed_cutoff(self) -> None: ) self.assertEqual("2026-08-16T08:00:00Z", retry.since) + self.assertEqual( + "2026-08-16T08:00:00Z", + retry.top_level_feedback_cutoff, + ) def test_new_pending_command_binds_to_the_newly_observed_head(self) -> None: previous_source = override_input( @@ -490,17 +572,21 @@ def test_acknowledged_command_keeps_its_bound_head(self) -> None: self.assertEqual("2026-08-16T08:00:00Z", facts.since) self.assertEqual("bound-head", facts.head_sha) - def test_acknowledged_command_keeps_its_recorded_cutoff(self) -> None: - for previous_facts, acknowledged_since in ( + def test_acknowledged_command_keeps_only_a_durable_cutoff(self) -> None: + for previous_facts, acknowledged_since, expected_cutoff in ( ( dashboard_facts( dashboard_override_bound_command_id=5, dashboard_override_head_sha="bound-head", dashboard_override_since="2026-08-16T09:00:00Z", + dashboard_top_level_feedback_cutoff=( + "2026-08-16T09:00:00Z" + ), ), "", + "2026-08-16T09:00:00Z", ), - (None, "2026-08-16T09:00:00Z"), + (None, "2026-08-16T09:00:00Z", ""), ): with self.subTest(previous_facts=previous_facts): source = override_input( @@ -530,6 +616,263 @@ def test_acknowledged_command_keeps_its_recorded_cutoff(self) -> None: ) self.assertEqual("2026-08-16T09:00:00Z", facts.since) + self.assertEqual( + expected_cutoff, + facts.top_level_feedback_cutoff, + ) + + def test_cutoff_marker_is_bound_to_the_acknowledged_command(self) -> None: + source = override_input( + issue_comment( + database_id=9, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body="\n".join(( + dashboard_override.override_ack_marker( + 5, + "bound-head", + "2026-08-16T08:00:00Z", + ), + dashboard_override.top_level_feedback_cutoff_marker( + 5, + "2026-08-16T08:00:00Z", + ), + )), + ), + issue_comment( + database_id=10, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body=dashboard_override.top_level_feedback_cutoff_marker( + 6, + "2026-08-16T10:00:00Z", + ), + ), + ) + + facts = dashboard_override.dashboard_override_facts( + source, + "author", + None, + "current-head", + ) + + self.assertEqual(5, facts.bound_command_id) + self.assertEqual( + "2026-08-16T08:00:00Z", + facts.top_level_feedback_cutoff, + ) + + def test_newer_command_advances_permanent_top_level_cutoff(self) -> None: + source = override_input( + issue_comment( + database_id=6, + body="/dashboard route:reviewers", + created_at="2026-08-16T10:00:00Z", + ), + ) + previous_facts = dashboard_facts( + dashboard_override_bound_command_id=5, + dashboard_override_head_sha="old-head", + dashboard_top_level_feedback_cutoff="2026-08-16T08:00:00Z", + ) + + facts = dashboard_override.dashboard_override_facts( + source, + "author", + None, + "new-head", + previous_facts, + ) + + self.assertEqual( + "2026-08-16T10:00:00Z", + facts.top_level_feedback_cutoff, + ) + + def test_legacy_pending_binding_does_not_gain_persistence(self) -> None: + source = override_input(issue_comment( + database_id=5, + body="/dashboard route:reviewers", + created_at="2026-08-16T08:00:00Z", + content_updated_at="2026-08-16T10:00:00Z", + )) + for version in (13, 16): + with ( + self.subTest(version=version), + tempfile.TemporaryDirectory() as temp_dir, + ): + legacy_facts = dashboard_facts( + dashboard_override_command_id=5, + dashboard_override_command_user="author", + dashboard_override_bound_command_id=( + 5 if version == 16 else 0 + ), + dashboard_override_head_sha="bound-head", + dashboard_override_since="2026-08-16T08:00:00Z", + dashboard_top_level_feedback_cutoff=( + "2026-08-16T08:00:00Z" + if version == 16 + else "" + ), + ) + stored = state.encode_dashboard_state(dashboard_state( + stored_dashboard_result(7, facts=legacy_facts) + )) + stored["version"] = version + with patch("state._state_dir", Path(temp_dir)): + state.dashboard_state_path().write_text( + json.dumps(stored), + encoding="utf-8", + ) + loaded = state.load_dashboard_state_cache() + self.assertIsNotNone(loaded) + assert loaded is not None + previous_facts = loaded.results[0].facts + + migrated = dashboard_override.dashboard_override_facts( + source, + "author", + None, + "bound-head", + previous_facts, + ) + + self.assertFalse(migrated.persistent_handoff) + self.assertEqual( + "2026-08-16T08:00:00Z", + migrated.since, + ) + self.assertEqual( + ( + "2026-08-16T08:00:00Z" + if version == 16 + else "" + ), + migrated.top_level_feedback_cutoff, + ) + acknowledgement = dashboard_override.append_command_ack_reply( + source, + result_facts(migrated, author="author"), + DashboardRoute.APPROVER, + ).dashboard_command_replies[0] + self.assertFalse(acknowledgement.persistent_handoff) + acknowledgement_body = dashboard_override.render_command_reply( + acknowledgement + ) + self.assertNotIn( + dashboard_override.PERSISTENT_HANDOFF_MARKER_PREFIX, + acknowledgement_body, + ) + delivered_source = override_input( + *source.issue_comments, + issue_comment( + database_id=9, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body=acknowledgement_body, + ), + ) + + after_push = dashboard_override.dashboard_override_facts( + delivered_source, + "author", + None, + "new-head", + result_facts(migrated), + ) + self.assertFalse(after_push.persistent_handoff) + self.assertFalse( + reviewer_handoff_active( + result_facts(after_push, head_sha="new-head") + ) + ) + + def test_missing_new_command_time_preserves_permanent_cutoff(self) -> None: + source = override_input( + issue_comment( + database_id=6, + body="/dashboard route:reviewers", + created_at="", + updated_at="", + content_updated_at="", + ), + ) + previous_facts = dashboard_facts( + dashboard_override_bound_command_id=5, + dashboard_override_head_sha="old-head", + dashboard_top_level_feedback_cutoff="2026-08-16T08:00:00Z", + ) + + facts = dashboard_override.dashboard_override_facts( + source, + "author", + None, + "new-head", + previous_facts, + ) + + self.assertEqual( + "2026-08-16T08:00:00Z", + facts.top_level_feedback_cutoff, + ) + + def test_acknowledgement_preserves_cutoff_across_cache_loss(self) -> None: + source = override_input( + issue_comment( + database_id=6, + body="/dashboard route:reviewers", + created_at="", + updated_at="", + content_updated_at="", + ), + ) + previous_facts = dashboard_facts( + dashboard_override_bound_command_id=5, + dashboard_override_head_sha="old-head", + dashboard_top_level_feedback_cutoff="2026-08-16T08:00:00Z", + ) + override = dashboard_override.dashboard_override_facts( + source, + "author", + None, + "new-head", + previous_facts, + ) + facts = dashboard_override.append_command_ack_reply( + source, + result_facts(override, author="author"), + DashboardRoute.APPROVER, + ) + + reply = facts.dashboard_command_replies[0] + body = dashboard_override.render_command_reply(reply) + self.assertIn( + "", + body, + ) + self.assertIn( + "The existing top-level feedback cutoff remains in effect, but no " + "additional top-level feedback was retired because the dashboard could " + "not determine a safe command time; unresolved review threads remain " + "open.", + body, + ) + + restored = dashboard_override.dashboard_override_facts( + override_input(issue_comment( + database_id=9, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body=body, + created_at="2026-08-16T10:00:00Z", + )), + "author", + None, + "new-head", + ) + + self.assertEqual( + "2026-08-16T08:00:00Z", + restored.top_level_feedback_cutoff, + ) def test_acknowledged_command_accepts_a_graphql_command_timestamp(self) -> None: source = override_input( @@ -578,7 +921,7 @@ def test_deleted_command_keeps_previous_handoff_cutoff(self) -> None: self.assertEqual("2026-08-16T08:00:00Z", facts.since) - def test_acknowledgement_recovers_cutoff_after_command_deletion(self) -> None: + def test_legacy_acknowledgement_does_not_recover_permanent_cutoff(self) -> None: source = override_input( issue_comment( database_id=9, @@ -601,8 +944,9 @@ def test_acknowledgement_recovers_cutoff_after_command_deletion(self) -> None: self.assertEqual(5, facts.bound_command_id) self.assertEqual("bound-head", facts.head_sha) self.assertEqual("2026-08-16T08:00:00Z", facts.since) + self.assertEqual("", facts.top_level_feedback_cutoff) - def test_deleted_command_uses_acknowledgement_timestamp_as_cutoff(self) -> None: + def test_acknowledgement_timestamp_restores_only_handoff_since(self) -> None: source = override_input( issue_comment( database_id=9, @@ -622,6 +966,7 @@ def test_deleted_command_uses_acknowledgement_timestamp_as_cutoff(self) -> None: self.assertEqual(5, facts.bound_command_id) self.assertEqual("bound-head", facts.head_sha) self.assertEqual("2026-08-16T08:00:00Z", facts.since) + self.assertEqual("", facts.top_level_feedback_cutoff) def test_status_marker_clears_only_its_bound_handoff(self) -> None: source = override_input( @@ -711,6 +1056,32 @@ def test_forged_acknowledgement_does_not_bind_a_head(self) -> None: self.assertEqual(5, facts.command_id) self.assertEqual("current-head", facts.head_sha) + def test_forged_marker_does_not_persist_handoff(self) -> None: + source = override_input( + issue_comment( + database_id=9, + actor=actor("opentelemetry-pr-dashboard[bot]"), + body=dashboard_override.override_ack_marker(5, "bound-head"), + ), + issue_comment( + database_id=10, + actor=actor("outsider"), + body=dashboard_override.persistent_handoff_marker( + 5, + "bound-head", + ), + ), + ) + + facts = dashboard_override.dashboard_override_facts( + source, + "author", + None, + "later-head", + ) + + self.assertFalse(facts.persistent_handoff) + def test_appends_routed_reply_for_break_glass_command_that_cleared_nothing(self) -> None: facts = dashboard_facts( author="author", @@ -1121,6 +1492,8 @@ def test_rebuilds_unacknowledged_reply_across_refreshes(self) -> None: head_sha="current-head", route=DashboardRoute.APPROVER, since="2026-08-16T07:00:00Z", + top_level_feedback_cutoff="2026-08-16T07:00:00Z", + persistent_handoff=True, ), ), facts.dashboard_command_replies, @@ -1180,7 +1553,7 @@ def test_command_that_cleared_nothing_is_acknowledged_where_it_is_routed(self) - facts = dashboard_override.append_command_ack_reply( source, result_facts(override, author="author"), - DashboardRoute.AUTHOR, + DashboardRoute.APPROVER, ) self.assertEqual( @@ -1190,8 +1563,10 @@ def test_command_that_cleared_nothing_is_acknowledged_where_it_is_routed(self) - "routed", "author", head_sha="current-head", - route=DashboardRoute.AUTHOR, + route=DashboardRoute.APPROVER, since="2026-08-16T07:00:00Z", + top_level_feedback_cutoff="2026-08-16T07:00:00Z", + persistent_handoff=True, ), ), facts.dashboard_command_replies, @@ -1223,6 +1598,8 @@ def test_conflict_does_not_defer_override_acknowledgement(self) -> None: head_sha="current-head", route=DashboardRoute.APPROVER, since="2026-08-16T07:00:00Z", + top_level_feedback_cutoff="2026-08-16T07:00:00Z", + persistent_handoff=True, ), ), facts.dashboard_command_replies, @@ -1289,7 +1666,7 @@ def test_delivers_command_acknowledgement(self, _load_state, _gh_api, run_gh) -> call([ "gh", "api", "--method", "POST", "repos/open-telemetry/example/issues/7/comments", - "-f", "body=\n\n@author, your reviewer-routing request was recorded; the reviewer handoff is waiting on the Copilot review.\n", + "-f", "body=\n\n@author, your reviewer-routing request was recorded; the reviewer handoff is waiting on the Copilot review. No top-level feedback was retired because the dashboard could not determine a safe command time; unresolved review threads remain open.\n", ]), ], run_gh.call_args_list, diff --git a/.github/scripts/pull-request-dashboard/test_discussion_lifecycle.py b/.github/scripts/pull-request-dashboard/test_discussion_lifecycle.py index 72ef192c148..5e8d7de46b5 100644 --- a/.github/scripts/pull-request-dashboard/test_discussion_lifecycle.py +++ b/.github/scripts/pull-request-dashboard/test_discussion_lifecycle.py @@ -7,6 +7,7 @@ AuthorCommentDecision, ClassificationDeferred, ClassificationDiagnostics, + ClassificationDiscussion, ClassificationFailure, ClassificationSuccess, DiscussionAction, @@ -14,6 +15,7 @@ DiscussionIdentity, DiscussionKind, FeedbackOutcome, + reviewer_feedback_prompt_input, ) from discussion_lifecycle import ( DiscussionInput, @@ -153,7 +155,7 @@ def test_prepares_threads_feedback_and_author_replies(self) -> None: "url": "https://example.test/thread/root", "body": "Please fix this.", "createdAt": ROOT_TIMESTAMP, - "author": {"login": "reviewer"}, + "author": {"login": "copilot"}, }, ] }, @@ -206,8 +208,9 @@ def test_prepares_threads_feedback_and_author_replies(self) -> None: ) self.assertEqual( prepared.review_threads[0]["discussion_url"], - "https://example.test/thread/first", + "https://example.test/thread/root", ) + self.assertTrue(prepared.review_threads[0]["strict_author_action"]) self.assertEqual( [item["discussion_id"] for item in prepared.top_level_items], ["pr-issue-comment-201", "pr-issue-comment-202"], @@ -298,6 +301,180 @@ def test_orders_discussions_by_creation_time_not_edit_time(self) -> None: ["2026-07-14T02:00:00Z", "2026-07-14T03:00:00Z"], ) + def test_edited_author_response_uses_its_content_timestamp(self) -> None: + prepared = prepare_discussions( + DiscussionInput( + (), + ( + { + "kind": "issue-comment", + "source_id": 201, + "created_timestamp": "2026-07-14T02:00:00Z", + "timestamp": "2026-07-14T02:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "body": "Please update the description.", + }, + { + "kind": "issue-comment", + "source_id": 301, + "created_timestamp": "2026-07-14T03:00:00Z", + "timestamp": "2026-07-14T06:00:00Z", + "actor": "author", + "actor_role": "author", + "body": "Both requests are complete.", + }, + { + "kind": "issue-comment", + "source_id": 202, + "created_timestamp": "2026-07-14T05:00:00Z", + "timestamp": "2026-07-14T05:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "body": "Please add another test.", + }, + ), + "author", + frozenset({"reviewer"}), + "no", + ) + ) + + reply = prepared.top_level_author_comment_items[0] + self.assertEqual("2026-07-14T06:00:00Z", reply["comments"][0]["timestamp"]) + self.assertEqual( + ["pr-issue-comment-201", "pr-issue-comment-202"], + [ + feedback["discussion_id"] + for feedback in reply["candidate_feedback"] + ], + ) + + def test_author_response_excludes_feedback_edited_after_it(self) -> None: + prepared = prepare_discussions( + DiscussionInput( + (), + ( + { + "kind": "issue-comment", + "source_id": 201, + "created_timestamp": "2026-07-14T02:00:00Z", + "timestamp": "2026-07-14T07:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "body": "Please also update the tests.", + }, + { + "kind": "issue-comment", + "source_id": 301, + "created_timestamp": "2026-07-14T03:00:00Z", + "timestamp": "2026-07-14T06:00:00Z", + "actor": "author", + "actor_role": "author", + "body": "The original request is complete.", + }, + ), + "author", + frozenset({"reviewer"}), + "no", + ) + ) + + self.assertEqual( + [], + prepared.top_level_author_comment_items[0]["candidate_feedback"], + ) + + def test_top_level_cutoff_retires_only_unchanged_old_feedback(self) -> None: + prepared = prepare_discussions( + DiscussionInput( + normalize_review_threads(({ + "id": "old-thread", + "isResolved": False, + "isOutdated": False, + "comments": { + "nodes": [{ + "url": "https://example.test/thread/old", + "body": "Please update the implementation.", + "createdAt": "2026-07-14T01:00:00Z", + "author": {"login": "reviewer"}, + }], + }, + },)), + ( + { + "kind": "issue-comment", + "source_id": 201, + "created_timestamp": "2026-07-14T01:00:00Z", + "timestamp": "2026-07-14T01:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "body": "Old top-level request.", + }, + { + "kind": "issue-comment", + "source_id": 202, + "created_timestamp": "2026-07-14T02:00:00Z", + "timestamp": "2026-07-14T05:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "body": "Edited top-level request.", + }, + { + "kind": "review-state", + "source_id": 203, + "created_timestamp": "2026-07-14T03:00:00Z", + "timestamp": "2026-07-14T03:00:00Z", + "content_timestamp": "2026-07-14T05:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "state": "COMMENTED", + "body": "Edited review summary.", + }, + { + "kind": "issue-comment", + "source_id": 204, + "created_timestamp": "2026-07-14T04:00:00Z", + "timestamp": "2026-07-14T04:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "body": "Same-second top-level request.", + }, + { + "kind": "issue-comment", + "source_id": 301, + "created_timestamp": "2026-07-14T06:00:00Z", + "timestamp": "2026-07-14T06:00:00Z", + "actor": "author", + "actor_role": "author", + "body": "I addressed the current requests.", + }, + ), + "author", + frozenset({"reviewer"}), + "no", + ), + top_level_feedback_cutoff="2026-07-14T04:00:00Z", + ) + + self.assertEqual( + ["old-thread"], + [thread["discussion_id"] for thread in prepared.review_threads], + ) + self.assertEqual( + ["pr-issue-comment-202", "pr-review-203"], + [item["discussion_id"] for item in prepared.top_level_items], + ) + self.assertEqual( + ["pr-issue-comment-202", "pr-review-203"], + [ + item["discussion_id"] + for item in prepared.top_level_author_comment_items[0][ + "candidate_feedback" + ] + ], + ) + def test_ignores_author_only_review_threads(self) -> None: prepared = prepare_discussions( DiscussionInput( @@ -829,6 +1006,51 @@ def test_restores_legacy_history_without_source_id(self) -> None: self.assertEqual(outcome.pending_actions, {}) self.assertEqual(outcome.top_level_history, legacy_history) + def test_edited_feedback_invalidates_legacy_history_without_source_id( + self, + ) -> None: + prepared = prepare_discussions( + DiscussionInput( + (), + ({ + "kind": "issue-comment", + "source_id": 201, + "created_timestamp": "2026-07-14T07:00:00Z", + "timestamp": "2026-07-14T09:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "body": "Please update this.", + },), + "author", + frozenset({"reviewer"}), + "no", + ) + ) + outcome = resolve_discussions( + prepared, + DiscussionClassifications( + (), + (classification("pr-issue-comment-201", "author"),), + (), + ), + { + "pr-issue-comment-201": { + "evidence": {"reply": "2026-07-14T07:30:00Z"}, + } + }, + ) + + self.assertEqual( + outcome.pending_actions, + { + "pr-issue-comment-201": { + "action": "author", + "since": "2026-07-14T07:00:00Z", + } + }, + ) + self.assertEqual(outcome.top_level_history, {}) + def test_recovers_source_id_for_legacy_history(self) -> None: outcome = resolve_discussions( PreparedDiscussions( @@ -1085,6 +1307,101 @@ def test_handoff_feedback_keeps_same_second_feedback_out_of_scope(self) -> None: self.assertEqual(PreparedDiscussions((), (), ()), filtered) + def test_handoff_feedback_uses_effective_content_timestamps(self) -> None: + source = DiscussionInput( + normalize_review_threads(({ + "id": "edited-thread", + "isResolved": False, + "isOutdated": False, + "comments": { + "nodes": [{ + "url": "https://example.test/thread/edited", + "body": "Please also update the tests.", + "createdAt": "2026-07-14T02:00:00Z", + "lastEditedAt": "2026-07-14T06:00:00Z", + "author": {"login": "reviewer"}, + }], + }, + },)), + ({ + "kind": "issue-comment", + "source_id": 201, + "created_timestamp": "2026-07-14T03:00:00Z", + "timestamp": "2026-07-14T05:00:00Z", + "actor": "reviewer", + "actor_role": "approver", + "body": "The test is still failing.", + },), + "author", + frozenset({"reviewer"}), + "no", + ) + + filtered = reviewer_handoff_feedback( + prepare_discussions(source), + "2026-07-14T04:00:00Z", + "author", + ) + + self.assertEqual( + ["edited-thread"], + [item["discussion_id"] for item in filtered.review_threads], + ) + self.assertEqual( + ["pr-issue-comment-201"], + [item["discussion_id"] for item in filtered.top_level_items], + ) + + def test_handoff_feedback_preserves_conversation_order_after_edit(self) -> None: + source = DiscussionInput( + normalize_review_threads(({ + "id": "edited-thread", + "isResolved": False, + "isOutdated": False, + "comments": { + "nodes": [ + { + "url": "https://example.test/thread/edited", + "body": "Root request, edited later.", + "createdAt": "2026-07-14T02:00:00Z", + "lastEditedAt": "2026-07-14T06:00:00Z", + "author": {"login": "root-reviewer"}, + }, + { + "url": "https://example.test/thread/follow-up", + "body": "Newer follow-up.", + "createdAt": "2026-07-14T05:00:00Z", + "author": {"login": "follow-up-reviewer"}, + }, + ], + }, + },)), + (), + "author", + frozenset({"root-reviewer"}), + "no", + ) + + filtered = reviewer_handoff_feedback( + prepare_discussions(source), + "2026-07-14T04:00:00Z", + "author", + ) + + discussion = filtered.review_threads[0] + prompt_input = reviewer_feedback_prompt_input( + ClassificationDiscussion.from_record(discussion) + ) + self.assertEqual( + "Root request, edited later.\n\nNewer follow-up.", + prompt_input["body"], + ) + self.assertEqual("root-reviewer", prompt_input["requester"]) + self.assertEqual( + "approver", + discussion["discussion_facts"]["latest_comment_role"], + ) + if __name__ == "__main__": unittest.main() diff --git a/.github/scripts/pull-request-dashboard/test_eval_baseline.py b/.github/scripts/pull-request-dashboard/test_eval_baseline.py index 4452236fe5a..98678e95a39 100644 --- a/.github/scripts/pull-request-dashboard/test_eval_baseline.py +++ b/.github/scripts/pull-request-dashboard/test_eval_baseline.py @@ -1,4 +1,5 @@ import contextlib +from datetime import datetime import io import json import sys @@ -157,12 +158,26 @@ def test_counts_and_configuration_describe_the_new_measurement(self) -> None: rebuilt["baseline_generated_at"], rebuilt["measurements_updated_at"], ) - self.assertEqual( - {rebuilt["baseline_generated_at"]}, - {case["measurement_date"] for case in rebuilt["cases"]}, - ) self.assertNotIn("generated_at", rebuilt) + def test_rebuild_uses_one_measurement_date(self) -> None: + with patch.object(regenerate_baseline, "datetime") as clock: + clock.now.side_effect = [ + datetime.fromisoformat("2026-09-02T23:59:59+00:00"), + datetime.fromisoformat("2026-09-03T00:00:00+00:00"), + ] + + rebuilt = rebuild( + payload(case("a")), + [{"a": "author_action"}], + "model", + ) + + self.assertEqual("2026-09-02", rebuilt["baseline_generated_at"]) + self.assertEqual("2026-09-02", rebuilt["measurements_updated_at"]) + self.assertEqual("2026-09-02", rebuilt["cases"][0]["measurement_date"]) + clock.now.assert_called_once_with(regenerate_baseline.UTC) + class RunBatchCachingTest(unittest.TestCase): def setUp(self) -> None: diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index a23e5a27a14..ab081e3c904 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -10,6 +10,7 @@ fetch_pr_issue_comments, fetch_pr_reviews, fetch_review_requests, + fetch_review_threads, fetch_latest_draft_transitions, gh_branch_rules, gh_pr_check_rollup, @@ -325,6 +326,38 @@ def test_fetch_pr_issue_comments_paginates(self, graphql) -> None: self.assertIn("isMinimized", graphql.call_args_list[0].args[0]) self.assertEqual(graphql.call_args_list[1].args[1]["after"], "cursor-1") self.assertEqual(graphql.call_count, 2) + @patch("github_cli.gh_graphql") + def test_requests_and_preserves_comment_edit_timestamps(self, graphql) -> None: + graphql.return_value = { + "data": { + "repository": { + "pullRequest": { + "reviewThreads": { + "nodes": [{ + "id": "PRRT_1", + "comments": { + "nodes": [{ + "id": "PRRC_1", + "createdAt": "2026-08-20T08:00:00Z", + "lastEditedAt": "2026-08-20T08:30:00Z", + }], + "pageInfo": {"hasNextPage": False}, + }, + }], + "pageInfo": {"hasNextPage": False}, + } + } + } + } + } + + threads = fetch_review_threads("open-telemetry", "example", 7) + + self.assertEqual( + "2026-08-20T08:30:00Z", + threads[0]["comments"]["nodes"][0]["lastEditedAt"], + ) + self.assertIn("lastEditedAt", graphql.call_args.args[0]) @patch("github_cli.gh_graphql") def test_fetch_pr_issue_comments_rejects_missing_page_cursor( @@ -1536,6 +1569,7 @@ def test_fetch_pr_reviews_normalizes_paginated_reviews(self, graphql) -> None: "body": "Please clarify this.", "submittedAt": "2026-07-15T03:55:00Z", "updatedAt": "2026-07-15T03:57:33Z", + "lastEditedAt": "2026-07-15T03:56:00Z", } ], "pageInfo": { @@ -1563,6 +1597,7 @@ def test_fetch_pr_reviews_normalizes_paginated_reviews(self, graphql) -> None: "body": "Looks good.", "submittedAt": "2026-07-15T04:00:00Z", "updatedAt": "2026-07-15T04:00:00Z", + "lastEditedAt": None, } ], "pageInfo": {"hasNextPage": False}, @@ -1586,6 +1621,7 @@ def test_fetch_pr_reviews_normalizes_paginated_reviews(self, graphql) -> None: "body": "Please clarify this.", "submitted_at": "2026-07-15T03:55:00Z", "updated_at": "2026-07-15T03:57:33Z", + "content_updated_at": "2026-07-15T03:56:00Z", }, { "id": 5000000000, @@ -1597,12 +1633,14 @@ def test_fetch_pr_reviews_normalizes_paginated_reviews(self, graphql) -> None: "body": "Looks good.", "submitted_at": "2026-07-15T04:00:00Z", "updated_at": "2026-07-15T04:00:00Z", + "content_updated_at": "2026-07-15T04:00:00Z", }, ], ) review_query = graphql.call_args_list[0].args[0] self.assertIn("comments {", review_query) self.assertIn("totalCount", review_query) + self.assertIn("lastEditedAt", review_query) self.assertEqual(graphql.call_args_list[1].args[1]["after"], "cursor-1") self.assertEqual(graphql.call_count, 2) diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py index a965c15f816..749d15a34ae 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -118,6 +118,7 @@ def test_persists_feedback_cleared_handoff_in_status_comment(self) -> None: DashboardRoute.AUTHOR, dashboard_override_bound_command_id=12, dashboard_override_head_sha="abcdef123456", + dashboard_top_level_feedback_cutoff="2026-08-16T08:00:00Z", dashboard_override_cleared_by_feedback=True, ), ) @@ -136,6 +137,11 @@ def test_persists_feedback_cleared_handoff_in_status_comment(self) -> None: "12:abcdef123456 -->", body, ) + self.assertIn( + "", + body, + ) self.assertEqual( (98, "no-status-marker"), status_reviewer_handoff_clearance([ @@ -876,6 +882,8 @@ def test_does_not_update_unchanged_comment(self, _comments: object) -> None: "\n" "\n" + "\n" "" ), @@ -901,6 +909,11 @@ def test_updates_comment_and_deletes_duplicates(self, _comments: object) -> None "12:bound-head -->", self.commands[0][-1], ) + self.assertIn( + "", + self.commands[0][-1], + ) @patch.object(pr_status_comment, "managed_status_comments", return_value=[]) def test_does_not_create_comment_when_creation_is_disabled( @@ -1371,15 +1384,15 @@ def test_targeted_update_completes_drained_rollout( def test_new_revision_queues_every_open_pr(self) -> None: state = pr_status_comment.prepare_rollout_state( { - "target_revision": 0, - "completed_revision": 0, + "target_revision": 16, + "completed_revision": 16, "pending_pr_numbers": [], }, {12, 34}, ) self.assertEqual(pr_status_comment.STATUS_COMMENT_REVISION, state["target_revision"]) - self.assertEqual(0, state["completed_revision"]) + self.assertEqual(16, state["completed_revision"]) self.assertEqual([12, 34], state["pending_pr_numbers"]) def test_current_revision_drops_closed_prs_from_queue(self) -> None: diff --git a/.github/scripts/pull-request-dashboard/test_pull_request_activity.py b/.github/scripts/pull-request-dashboard/test_pull_request_activity.py index f5f4a41cc66..f479fe1c1ef 100644 --- a/.github/scripts/pull-request-dashboard/test_pull_request_activity.py +++ b/.github/scripts/pull-request-dashboard/test_pull_request_activity.py @@ -236,6 +236,24 @@ def test_edited_comments_use_activity_time_but_creation_order(self) -> None: ], ) + def test_edited_review_keeps_submission_time_for_state_ordering(self) -> None: + event = activity( + reviews=[ + { + "database_id": 1, + "submitted_at": "2026-07-14T01:00:00Z", + "updated_at": "2026-07-14T03:00:00Z", + "content_updated_at": "2026-07-14T03:00:00Z", + "user": {"login": "reviewer"}, + "state": "CHANGES_REQUESTED", + "body": "Please update this.", + } + ], + ).events[0] + + self.assertEqual("2026-07-14T01:00:00Z", event["timestamp"]) + self.assertEqual("2026-07-14T03:00:00Z", event["content_timestamp"]) + def test_copilot_identity_shapes_are_normalized(self) -> None: timeline = activity( issue_comments=[ @@ -329,6 +347,25 @@ def test_review_states_and_body_text_determine_substantive_activity(self) -> Non timeline.latest_approver_activity_at, ) + def test_review_edit_updates_content_activity_clock(self) -> None: + timeline = activity( + reviews=[ + { + "id": 1, + "submitted_at": "2026-07-14T01:00:00Z", + "content_updated_at": "2026-07-14T03:00:00Z", + "user": {"login": "reviewer"}, + "state": "CHANGES_REQUESTED", + "body": "Please also update the tests.", + } + ], + ) + + self.assertEqual( + datetime(2026, 7, 14, 3, tzinfo=timezone.utc), + timeline.latest_approver_activity_at, + ) + def test_all_activity_clocks_are_independent(self) -> None: timeline = activity( issue_comments=[ diff --git a/.github/scripts/pull-request-dashboard/test_pull_request_source.py b/.github/scripts/pull-request-dashboard/test_pull_request_source.py index c874fb6ccc7..e5cc257f54b 100644 --- a/.github/scripts/pull-request-dashboard/test_pull_request_source.py +++ b/.github/scripts/pull-request-dashboard/test_pull_request_source.py @@ -7,6 +7,7 @@ from github_cli import TransientGhError from pull_request_source import ( Actor, + IssueComment, PullRequestMetadata, fetch_pull_request_source, normalize_actor, @@ -16,6 +17,16 @@ class PullRequestSourceNormalizationTest(unittest.TestCase): + def test_issue_comment_uses_updated_at_only_as_a_missing_source_fallback( + self, + ) -> None: + self.assertEqual( + "2026-08-20T04:00:00Z", + IssueComment( + updated_at="2026-08-20T04:00:00Z" + ).effective_content_timestamp, + ) + def test_non_numeric_issue_comment_database_id_normalizes_to_zero(self) -> None: comments = normalize_issue_comments([ {"databaseId": "not-a-number", "body": "Comment"} @@ -92,6 +103,7 @@ def test_normalizes_mixed_gh_rest_and_graphql_shapes(self) -> None: "author": {"login": "reviewer"}, "state": "approved", "submittedAt": "2026-08-20T07:00:00Z", + "lastEditedAt": "2026-08-20T07:30:00Z", } ], "review_requests": [ @@ -116,6 +128,7 @@ def test_normalizes_mixed_gh_rest_and_graphql_shapes(self) -> None: "url": "https://example.test/thread/1", "body": "Please update this.", "createdAt": "2026-08-20T08:00:00Z", + "lastEditedAt": "2026-08-20T08:30:00Z", "author": {"login": "reviewer"}, "reactionGroups": [ { @@ -175,8 +188,16 @@ def test_normalizes_mixed_gh_rest_and_graphql_shapes(self) -> None: self.assertEqual("status[bot]", source.issue_comments[0].actor.login) self.assertEqual("reviewer", source.review_comments[0].actor.login) self.assertEqual("head-sha", source.reviews[0].commit_id) + self.assertEqual( + "2026-08-20T07:30:00Z", + source.reviews[0].content_updated_at, + ) self.assertEqual(13, source.reviews[0].database_id) self.assertEqual("maintainers", source.review_requests[0].login) + self.assertEqual( + "2026-08-20T08:30:00Z", + source.review_threads[0].comments[0].updated_at, + ) self.assertEqual( ("author",), source.review_threads[0] diff --git a/.github/scripts/pull-request-dashboard/test_routing_decision.py b/.github/scripts/pull-request-dashboard/test_routing_decision.py index 08e60408f3e..c74cee28048 100644 --- a/.github/scripts/pull-request-dashboard/test_routing_decision.py +++ b/.github/scripts/pull-request-dashboard/test_routing_decision.py @@ -335,7 +335,7 @@ def test_pending_checks_still_hold_maintainer_owned_action_route( self.assertEqual(DashboardRoute.AUTHOR, outcome.route) self.assertTrue(outcome.facts.route_held_for_gates) - def test_reviewer_handoff_is_bound_to_the_current_head(self) -> None: + def test_reviewer_handoff_supports_persistent_and_legacy_bindings(self) -> None: self.assertTrue( reviewer_handoff_active( dashboard_facts( @@ -353,6 +353,15 @@ def test_reviewer_handoff_is_bound_to_the_current_head(self) -> None: ) ) ) + self.assertTrue( + reviewer_handoff_active( + dashboard_facts( + dashboard_override_head_sha="old-head", + dashboard_override_persistent=True, + head_sha="new-head", + ) + ) + ) for facts in ( {"dashboard_override_head_sha": "old-head", "head_sha": "new-head"}, {"dashboard_override_head_sha": "", "head_sha": "current-head"}, @@ -363,6 +372,17 @@ def test_reviewer_handoff_is_bound_to_the_current_head(self) -> None: "head_sha": "current-head", "dashboard_override_cleared_by_feedback": True, }, + { + "dashboard_override_head_sha": "old-head", + "dashboard_override_persistent": True, + "head_sha": "new-head", + "dashboard_override_cleared_by_feedback": True, + }, + { + "dashboard_override_head_sha": "old-head", + "dashboard_override_persistent": True, + "head_sha": "", + }, ): with self.subTest(facts=facts): self.assertFalse( diff --git a/.github/scripts/pull-request-dashboard/test_routing_snapshot.py b/.github/scripts/pull-request-dashboard/test_routing_snapshot.py index f76e93a2186..312e87dac53 100644 --- a/.github/scripts/pull-request-dashboard/test_routing_snapshot.py +++ b/.github/scripts/pull-request-dashboard/test_routing_snapshot.py @@ -4,6 +4,13 @@ import unittest from unittest.mock import patch +from dashboard_test_support import ( + actor, + pull_request_source, + review_source, + review_thread, + review_thread_comment, +) from pull_request_source import normalize_pull_request_source from routing_snapshot import build_routing_snapshot, fetch_routing_snapshot @@ -38,6 +45,65 @@ def representative_raw() -> dict: class RoutingSnapshotTest(unittest.TestCase): + def test_fixture_preserves_review_activity_fingerprint_fields(self) -> None: + source = pull_request_source( + reviews=(review_source( + database_id=17, + actor=actor("copilot"), + content_updated_at="2026-08-16T08:00:00Z", + ),), + review_threads=(review_thread(comments=(review_thread_comment( + actor=actor("copilot"), + updated_at="2026-08-16T09:00:00Z", + ),)),), + ) + + inputs = source.fingerprint.routing_inputs() + + self.assertEqual( + "2026-08-16T08:00:00Z", + inputs["reviews"][0]["content_updated_at"], + ) + thread_comment = inputs["review_threads"][0]["comments"]["nodes"][0] + self.assertEqual( + "2026-08-16T09:00:00Z", + thread_comment["lastEditedAt"], + ) + + def test_fixture_fingerprint_tracks_review_activity_changes(self) -> None: + def snapshot( + *, + review_edited_at: str = "2026-08-16T08:00:00Z", + thread_edited_at: str = "2026-08-16T09:00:00Z", + ): + return build_routing_snapshot(pull_request_source( + reviews=(review_source( + database_id=17, + actor=actor("copilot"), + content_updated_at=review_edited_at, + ),), + review_threads=(review_thread( + comments=(review_thread_comment( + actor=actor("copilot"), + updated_at=thread_edited_at, + ),), + ),), + )) + + baseline = snapshot().routing_input_fingerprint + changes = ( + snapshot( + review_edited_at="2026-08-16T10:00:00Z" + ).routing_input_fingerprint, + snapshot( + thread_edited_at="2026-08-16T10:00:00Z" + ).routing_input_fingerprint, + ) + + for changed in changes: + with self.subTest(changed=changed): + self.assertNotEqual(baseline, changed) + def test_preserves_characterized_fingerprints_and_component_digests(self) -> None: snapshot = build_routing_snapshot( normalize_pull_request_source(representative_raw()) diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index a6aff7d3a66..4725ca936e2 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -23,9 +23,11 @@ AUTHOR_NUDGE_STATE_VERSION, BACKFILL_STATE_VERSION, COPILOT_REVIEW_REQUEST_STATE_VERSION, + DASHBOARD_STATE_COMPATIBLE_VERSIONS, DASHBOARD_STATE_VERSION, NOTIFICATION_STATE_VERSION, STATUS_COMMENT_ROLLOUT_STATE_VERSION, + STATUS_COMMENT_REVISION, author_nudge_state_path, backfill_state_path, copilot_review_request_state_path, @@ -179,6 +181,129 @@ def test_dashboard_state_save_writes_explicit_shape(self) -> None: }, ) + def test_dashboard_state_migrates_safe_production_versions(self) -> None: + for version in DASHBOARD_STATE_COMPATIBLE_VERSIONS: + with self.subTest(version=version), tempfile.TemporaryDirectory() as temp_dir: + with patch("state._state_dir", Path(temp_dir)): + dashboard_state_path().write_text( + json.dumps({ + "version": version, + "initial_backfill_complete": True, + "prs": {}, + }), + encoding="utf-8", + ) + + self.assertEqual( + DashboardState(initial_backfill_complete=True), + load_dashboard_state_cache(), + ) + + def test_dashboard_state_rejects_incompatible_versions(self) -> None: + for version in (14, 15): + with self.subTest(version=version), tempfile.TemporaryDirectory() as temp_dir: + with ( + patch("state._state_dir", Path(temp_dir)), + redirect_stderr(StringIO()), + ): + dashboard_state_path().write_text( + json.dumps({ + "version": version, + "initial_backfill_complete": True, + "prs": {}, + }), + encoding="utf-8", + ) + + self.assertIsNone(load_dashboard_state_cache()) + + def test_version_thirteen_state_does_not_invent_durable_cutoff(self) -> None: + legacy_facts = dashboard_facts( + dashboard_override_since="2026-08-16T08:00:00Z", + dashboard_command_replies=( + DashboardCommandReply( + 91, + "routed", + "alice", + head_sha="current-head", + route=DashboardRoute.APPROVER, + since="2026-08-16T08:00:00Z", + ), + ), + ) + stored = encode_dashboard_state(dashboard_state( + stored_dashboard_result(facts=legacy_facts) + )) + stored["version"] = 13 + + with tempfile.TemporaryDirectory() as temp_dir, patch( + "state._state_dir", + Path(temp_dir), + ): + dashboard_state_path().write_text( + json.dumps(stored), + encoding="utf-8", + ) + + loaded = load_dashboard_state_cache() + + self.assertIsNotNone(loaded) + assert loaded is not None + facts = loaded.results[0].facts + self.assertEqual("2026-08-16T08:00:00Z", facts.dashboard_override_since) + self.assertEqual("", facts.dashboard_top_level_feedback_cutoff) + self.assertFalse(facts.dashboard_override_persistent) + self.assertEqual( + "", + facts.dashboard_command_replies[0].top_level_feedback_cutoff, + ) + self.assertFalse( + facts.dashboard_command_replies[0].persistent_handoff + ) + + def test_version_sixteen_state_does_not_invent_persistent_handoff(self) -> None: + legacy_facts = dashboard_facts( + dashboard_override_head_sha="bound-head", + dashboard_top_level_feedback_cutoff="2026-08-16T08:00:00Z", + dashboard_command_replies=( + DashboardCommandReply( + 91, + "routed", + "alice", + head_sha="bound-head", + route=DashboardRoute.APPROVER, + top_level_feedback_cutoff="2026-08-16T08:00:00Z", + ), + ), + ) + stored = encode_dashboard_state(dashboard_state( + stored_dashboard_result(facts=legacy_facts) + )) + stored["version"] = 16 + + with tempfile.TemporaryDirectory() as temp_dir, patch( + "state._state_dir", + Path(temp_dir), + ): + dashboard_state_path().write_text( + json.dumps(stored), + encoding="utf-8", + ) + + loaded = load_dashboard_state_cache() + + self.assertIsNotNone(loaded) + assert loaded is not None + facts = loaded.results[0].facts + self.assertEqual( + "2026-08-16T08:00:00Z", + facts.dashboard_top_level_feedback_cutoff, + ) + self.assertFalse(facts.dashboard_override_persistent) + self.assertFalse( + facts.dashboard_command_replies[0].persistent_handoff + ) + def test_dashboard_facts_codec_round_trip(self) -> None: facts = dashboard_facts( author="alice", @@ -191,6 +316,8 @@ def test_dashboard_facts_codec_round_trip(self) -> None: dashboard_override_bound_command_id=91, dashboard_override_head_sha="current-head", dashboard_override_since="2026-08-16T08:00:00Z", + dashboard_top_level_feedback_cutoff="2026-08-16T08:00:00Z", + dashboard_override_persistent=True, dashboard_override_cleared_by_feedback=True, dashboard_command_replies=( DashboardCommandReply( @@ -201,6 +328,8 @@ def test_dashboard_facts_codec_round_trip(self) -> None: route=DashboardRoute.APPROVER, held_gates="the required checks", since="2026-08-16T08:00:00Z", + top_level_feedback_cutoff="2026-08-16T08:00:00Z", + persistent_handoff=True, ), DashboardCommandReply( 91, @@ -423,6 +552,10 @@ def test_dashboard_facts_rejects_null_non_optional_fields(self) -> None: ({"author": None}, "facts.author must be a string"), ({"assignees": None}, "facts.assignees must be an array of strings"), ({"is_draft": None}, "facts.is_draft must be a boolean"), + ( + {"dashboard_override_persistent": None}, + "facts.dashboard_override_persistent must be a boolean", + ), ({"approval_count": None}, "facts.approval_count must be an integer"), ( {"dashboard_command_replies": None}, @@ -442,6 +575,18 @@ def test_dashboard_facts_rejects_null_non_optional_fields(self) -> None: }, "facts.dashboard_command_replies.comment_id must be an integer", ), + ( + { + "dashboard_command_replies": [{ + "comment_id": 1, + "kind": "routed", + "route": "approver", + "persistent_handoff": None, + }] + }, + "facts.dashboard_command_replies.persistent_handoff " + "must be a boolean", + ), ) for facts, message in cases: @@ -641,8 +786,10 @@ def test_legacy_dashboard_state_payload_reencodes_to_current_shape(self) -> None def test_notification_state_version_is_independent(self) -> None: self.assertEqual(BACKFILL_STATE_VERSION, 3) self.assertEqual(NOTIFICATION_STATE_VERSION, 3) - self.assertEqual(DASHBOARD_STATE_VERSION, 16) + self.assertEqual(DASHBOARD_STATE_VERSION, 17) + self.assertEqual(DASHBOARD_STATE_COMPATIBLE_VERSIONS, (11, 12, 13, 16)) self.assertEqual(STATUS_COMMENT_ROLLOUT_STATE_VERSION, 2) + self.assertEqual(STATUS_COMMENT_REVISION, 20) self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 3) self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 6) @@ -993,6 +1140,27 @@ def test_new_delivery_version_rejects_older_workers(self) -> None: self.assertFalse(claim_delivery_versions()) self.assertEqual(newer, load_delivery_versions()) + def test_lifecycle_state_version_blocks_incompatible_delivery_workers(self) -> None: + current = current_delivery_versions() + for old_version in (13, 16): + old_worker = {**current, "DASHBOARD_STATE_VERSION": old_version} + with self.subTest(old_version=old_version), tempfile.TemporaryDirectory() as temp_dir: + with patch("state._state_dir", Path(temp_dir)): + with patch( + "state.current_delivery_versions", + return_value=old_worker, + ): + self.assertTrue(claim_delivery_versions()) + + self.assertTrue(claim_delivery_versions()) + + with patch( + "state.current_delivery_versions", + return_value=old_worker, + ): + self.assertFalse(claim_delivery_versions()) + self.assertEqual(current, load_delivery_versions()) + def test_delivery_versions_fail_closed(self) -> None: malformed_versions = [ "not json",