refactor: harden reviewer handoff command surface

Author: PLeVasseur

scripts/reviewer_bot_core/comment_command_policy.py

@@ -11,6 +11,7 @@
 class OrdinaryCommandId(StrEnum):
     PASS = "pass"
     AWAY = "away"
+    FEEDBACK = "feedback"
     DONE = "done"
     LABEL = "label"
     SYNC_MEMBERS = "sync-members"
@@ -103,6 +104,20 @@ def decide_comment_command(bot, request, classified, *, actor_class: str, comman
             success=False,
             react=True,
         )
+    if command == OrdinaryCommandId.FEEDBACK.value:
+        if args:
+            return InlineResponseDecision(
+                response=f"❌ `/feedback` does not accept arguments. Usage: `{bot.BOT_MENTION} /feedback`",
+                success=False,
+                react=True,
+            )
+        return ExecuteOrdinaryCommandDecision(
+            command_id=OrdinaryCommandId.FEEDBACK,
+            issue_number=issue_number,
+            actor=comment_author,
+            raw_args=(),
+            needs_assignment_request=False,
+        )
     if command == OrdinaryCommandId.DONE.value:
         return ExecuteOrdinaryCommandDecision(
             command_id=OrdinaryCommandId.DONE,

scripts/reviewer_bot_core/review_state_machine.py

@@ -161,6 +161,7 @@ def _reset_cycle_state(review_data: dict) -> None:
         review_data[channel] = {"accepted": None, "seen_keys": []}
     review_data["current_cycle_completion"] = {}
     review_data["current_cycle_write_approval"] = {}
+    review_data["current_cycle_reviewer_handoff"] = None
     review_data["overdue_anchor"] = None
 
 
@@ -204,6 +205,9 @@ def clear_current_reviewer(state: dict, issue_number: int) -> bool:
     if review_data.get("assignment_method") is not None:
         review_data["assignment_method"] = None
         changed = True
+    if review_data.get("current_cycle_reviewer_handoff") is not None:
+        review_data["current_cycle_reviewer_handoff"] = None
+        changed = True
     clear_transition_timers(review_data)
     return changed
 

scripts/reviewer_bot_core/review_state_types.py

@@ -79,3 +79,4 @@ class ReviewEntryState:
     review_dismissal: ReviewChannelState = field(default_factory=ReviewChannelState)
     current_cycle_completion: dict[str, Any] = field(default_factory=dict)
     current_cycle_write_approval: dict[str, Any] = field(default_factory=dict)
+    current_cycle_reviewer_handoff: dict[str, Any] | None = None

scripts/reviewer_bot_core/reviewer_response_policy.py

@@ -195,6 +195,43 @@ def _record_for_current_reviewer(record: dict | None | object, current_reviewer:
     return record
 
 
+def _current_cycle_reviewer_handoff_record(
+    review_data: dict,
+    current_reviewer: str,
+    current_head: str | None,
+    *,
+    issue_is_pull_request: bool,
+) -> dict | None:
+    handoff = review_data.get("current_cycle_reviewer_handoff")
+    if not isinstance(handoff, dict):
+        return None
+    if handoff.get("command_name") != "feedback":
+        return None
+    actor = handoff.get("actor")
+    timestamp = handoff.get("timestamp")
+    source_event_key = handoff.get("source_event_key")
+    if not isinstance(actor, str) or actor.lower() != current_reviewer.lower():
+        return None
+    if not isinstance(timestamp, str) or not timestamp.strip():
+        return None
+    if not isinstance(source_event_key, str) or not source_event_key.strip():
+        return None
+    reviewed_head_sha = handoff.get("reviewed_head_sha")
+    if issue_is_pull_request:
+        if not isinstance(reviewed_head_sha, str) or reviewed_head_sha != current_head:
+            return None
+    elif reviewed_head_sha is not None:
+        return None
+    return {
+        "semantic_key": source_event_key,
+        "timestamp": timestamp,
+        "actor": actor,
+        "reviewed_head_sha": reviewed_head_sha,
+        "source_precedence": 1,
+        "payload": {"command_name": "feedback"},
+    }
+
+
 def _contributor_revision_handoff_record(review_data: dict, current_head: str | None, reviewer_review: dict | None) -> dict | None:
     contributor_revision = review_data.get("contributor_revision", {}).get("accepted")
     if not isinstance(contributor_revision, dict):
@@ -283,6 +320,18 @@ def derive_reviewer_response_state(
             parse_timestamp=live_review_support.parse_github_timestamp,
         ) > 0:
             latest_reviewer_response = reviewer_review
+        reviewer_handoff = _current_cycle_reviewer_handoff_record(
+            review_data,
+            current_reviewer,
+            None,
+            issue_is_pull_request=False,
+        )
+        if reviewer_review_helpers.compare_records(
+            reviewer_handoff,
+            latest_reviewer_response,
+            parse_timestamp=live_review_support.parse_github_timestamp,
+        ) > 0:
+            latest_reviewer_response = reviewer_handoff
         completion = review_data.get("current_cycle_completion")
         if isinstance(completion, dict) and completion.get("completed"):
             return _decorate_response(
@@ -292,6 +341,7 @@ def derive_reviewer_response_state(
                 anchor_timestamp=latest_reviewer_response.get("timestamp") if isinstance(latest_reviewer_response, dict) else None,
                 reviewer_comment=reviewer_comment,
                 reviewer_review=reviewer_review,
+                current_cycle_reviewer_handoff=reviewer_handoff,
                 contributor_comment=contributor_comment,
                 contributor_handoff=contributor_comment,
             )
@@ -303,6 +353,7 @@ def derive_reviewer_response_state(
                 anchor_timestamp=latest_reviewer_response.get("timestamp") if isinstance(latest_reviewer_response, dict) else None,
                 reviewer_comment=reviewer_comment,
                 reviewer_review=reviewer_review,
+                current_cycle_reviewer_handoff=reviewer_handoff,
                 contributor_comment=contributor_comment,
                 contributor_handoff=contributor_comment,
             )
@@ -314,6 +365,7 @@ def derive_reviewer_response_state(
                 anchor_timestamp=_initial_reviewer_anchor(review_data),
                 reviewer_comment=reviewer_comment,
                 reviewer_review=reviewer_review,
+                current_cycle_reviewer_handoff=reviewer_handoff,
                 contributor_comment=contributor_comment,
                 contributor_handoff=contributor_comment,
             )
@@ -329,6 +381,7 @@ def derive_reviewer_response_state(
                 anchor_timestamp=contributor_comment.get("timestamp") if isinstance(contributor_comment, dict) else None,
                 reviewer_comment=reviewer_comment,
                 reviewer_review=reviewer_review,
+                current_cycle_reviewer_handoff=reviewer_handoff,
                 contributor_comment=contributor_comment,
                 contributor_handoff=contributor_comment,
             )
@@ -339,6 +392,7 @@ def derive_reviewer_response_state(
             anchor_timestamp=latest_reviewer_response.get("timestamp") if isinstance(latest_reviewer_response, dict) else None,
             reviewer_comment=reviewer_comment,
             reviewer_review=reviewer_review,
+            current_cycle_reviewer_handoff=reviewer_handoff,
             contributor_comment=contributor_comment,
             contributor_handoff=contributor_comment,
         )
@@ -350,7 +404,14 @@ def derive_reviewer_response_state(
             scope_fields={"current_scope_key": None, "current_scope_basis": None},
         )
 
-    if not reviewer_comment and not reviewer_review:
+    reviewer_handoff = _current_cycle_reviewer_handoff_record(
+        review_data,
+        current_reviewer,
+        current_head,
+        issue_is_pull_request=True,
+    )
+
+    if not reviewer_comment and not reviewer_review and not reviewer_handoff:
         if not had_reviewer_review:
             return _decorate_response(
                 state="awaiting_reviewer_response",
@@ -359,6 +420,7 @@ def derive_reviewer_response_state(
                 anchor_timestamp=_initial_reviewer_anchor(review_data),
                 reviewer_comment=reviewer_comment,
                 reviewer_review=reviewer_review,
+                current_cycle_reviewer_handoff=reviewer_handoff,
                 contributor_comment=contributor_comment,
                 contributor_handoff=None,
             )
@@ -370,6 +432,12 @@ def derive_reviewer_response_state(
         parse_timestamp=live_review_support.parse_github_timestamp,
     ) > 0:
         latest_reviewer_response = reviewer_review
+    if reviewer_review_helpers.compare_records(
+        reviewer_handoff,
+        latest_reviewer_response,
+        parse_timestamp=live_review_support.parse_github_timestamp,
+    ) > 0:
+        latest_reviewer_response = reviewer_handoff
 
     contributor_handoff = contributor_comment
     contributor_revision = _contributor_revision_handoff_record(
@@ -402,6 +470,7 @@ def derive_reviewer_response_state(
             current_head_sha=current_head,
             reviewer_comment=reviewer_comment,
             reviewer_review=reviewer_review,
+            current_cycle_reviewer_handoff=reviewer_handoff,
             contributor_comment=contributor_comment,
             contributor_handoff=contributor_handoff,
         )
@@ -419,6 +488,7 @@ def derive_reviewer_response_state(
                 current_head_sha=current_head,
                 reviewer_comment=reviewer_comment,
                 reviewer_review=reviewer_review,
+                current_cycle_reviewer_handoff=reviewer_handoff,
                 contributor_comment=contributor_comment,
                 contributor_handoff=contributor_handoff,
             )
@@ -438,21 +508,24 @@ def derive_reviewer_response_state(
             current_head_sha=current_head,
             reviewer_comment=reviewer_comment,
             reviewer_review=reviewer_review,
+            current_cycle_reviewer_handoff=reviewer_handoff,
             contributor_comment=contributor_comment,
             contributor_handoff=contributor_handoff,
         )
 
     latest_review_head = reviewer_review.get("reviewed_head_sha") if isinstance(reviewer_review, dict) else None
     if not isinstance(latest_review_head, str) or latest_review_head != current_head:
-        if isinstance(reviewer_comment, dict):
+        if isinstance(reviewer_comment, dict) or isinstance(reviewer_handoff, dict):
+            response_anchor = reviewer_comment if isinstance(reviewer_comment, dict) else reviewer_handoff
             return _decorate_response(
                 state="awaiting_contributor_response",
                 reason="accepted_same_scope_reviewer_activity",
                 scope_fields=_current_scope_fields(review_data, current_reviewer, current_head, contributor_handoff),
-                anchor_timestamp=reviewer_comment.get("timestamp") if isinstance(reviewer_comment.get("timestamp"), str) else None,
+                anchor_timestamp=response_anchor.get("timestamp") if isinstance(response_anchor.get("timestamp"), str) else None,
                 current_head_sha=current_head,
                 reviewer_comment=reviewer_comment,
                 reviewer_review=reviewer_review,
+                current_cycle_reviewer_handoff=reviewer_handoff,
                 contributor_comment=contributor_comment,
                 contributor_handoff=contributor_handoff,
             )
@@ -464,6 +537,7 @@ def derive_reviewer_response_state(
             current_head_sha=current_head,
             reviewer_comment=reviewer_comment,
             reviewer_review=reviewer_review,
+            current_cycle_reviewer_handoff=reviewer_handoff,
             contributor_comment=contributor_comment,
             contributor_handoff=contributor_handoff,
         )
@@ -486,6 +560,7 @@ def derive_reviewer_response_state(
             current_head_sha=current_head,
             reviewer_comment=reviewer_comment,
             reviewer_review=reviewer_review,
+            current_cycle_reviewer_handoff=reviewer_handoff,
             contributor_comment=contributor_comment,
             contributor_handoff=contributor_handoff,
         )
@@ -498,6 +573,7 @@ def derive_reviewer_response_state(
             current_head_sha=current_head,
             reviewer_comment=reviewer_comment,
             reviewer_review=reviewer_review,
+            current_cycle_reviewer_handoff=reviewer_handoff,
             contributor_comment=contributor_comment,
             contributor_handoff=contributor_handoff,
         )
@@ -509,6 +585,7 @@ def derive_reviewer_response_state(
         current_head_sha=current_head,
         reviewer_comment=reviewer_comment,
         reviewer_review=reviewer_review,
+        current_cycle_reviewer_handoff=reviewer_handoff,
         contributor_comment=contributor_comment,
         contributor_handoff=contributor_handoff,
     )

scripts/reviewer_bot_core/state_adapters.py

@@ -297,6 +297,9 @@ def review_entry_from_persisted(review_entry: dict[str, Any] | list[Any] | None)
         current_cycle_write_approval=deepcopy(review_entry.get("current_cycle_write_approval") or {})
         if isinstance(review_entry.get("current_cycle_write_approval"), dict)
         else {},
+        current_cycle_reviewer_handoff=deepcopy(review_entry.get("current_cycle_reviewer_handoff"))
+        if isinstance(review_entry.get("current_cycle_reviewer_handoff"), dict)
+        else None,
     )
 
 
@@ -328,6 +331,7 @@ def review_entry_to_persisted(review_entry: ReviewEntryState) -> dict[str, Any]:
         "review_dismissal": _channel_to_persisted(review_entry.review_dismissal),
         "current_cycle_completion": deepcopy(review_entry.current_cycle_completion),
         "current_cycle_write_approval": deepcopy(review_entry.current_cycle_write_approval),
+        "current_cycle_reviewer_handoff": deepcopy(review_entry.current_cycle_reviewer_handoff),
     }
 
 

scripts/reviewer_bot_lib/assignment_flow.py

@@ -117,14 +117,6 @@ def resolve_reviewer_authority(
     is_pull_request: bool,
 ) -> dict[str, object]:
     tracked_reviewer = review_data.get("current_reviewer") if isinstance(review_data, dict) else None
-    if not isinstance(tracked_reviewer, str) or not tracked_reviewer.strip():
-        return {
-            "authority_status": "no_tracked_reviewer",
-            "tracked_reviewer": None,
-            "live_control_plane_reviewers": [],
-            "reason": "no_tracked_reviewer",
-        }
-
     result = bot.github.get_issue_assignees_result(issue_number, is_pull_request=is_pull_request)
     _hard_fail_if_permission_denied(result, action="reviewer authority read", issue_number=issue_number)
     if not result.ok or not isinstance(result.payload, list):
@@ -136,6 +128,13 @@ def resolve_reviewer_authority(
         }
 
     live_control_plane_reviewers = [value for value in result.payload if isinstance(value, str) and value.strip()]
+    if not isinstance(tracked_reviewer, str) or not tracked_reviewer.strip():
+        return {
+            "authority_status": "no_tracked_reviewer",
+            "tracked_reviewer": None,
+            "live_control_plane_reviewers": live_control_plane_reviewers,
+            "reason": "no_tracked_reviewer",
+        }
     normalized_reviewers = {value.lower() for value in live_control_plane_reviewers}
     tracked_key = tracked_reviewer.lower()
     if is_pull_request:
@@ -175,6 +174,80 @@ def resolve_reviewer_authority(
     }
 
 
+def resolve_reviewer_command_authority(
+    bot,
+    state: dict,
+    request,
+    *,
+    actor: str | None = None,
+) -> dict[str, object]:
+    issue_number = request.issue_number
+    review_data = ensure_review_entry(state, issue_number)
+    if review_data is None:
+        result = bot.github.get_issue_assignees_result(issue_number, is_pull_request=bool(request.is_pull_request))
+        _hard_fail_if_permission_denied(result, action="reviewer authority read", issue_number=issue_number)
+        if not result.ok or not isinstance(result.payload, list):
+            return {
+                "authorized": False,
+                "authorization_status": "live_read_unavailable",
+                "review_data": None,
+                "tracked_reviewer": None,
+                "live_control_plane_reviewers": [],
+                "reason": str(result.failure_kind or "live_control_plane_unavailable"),
+            }
+        return {
+            "authorized": False,
+            "authorization_status": "no_active_review",
+            "review_data": None,
+            "tracked_reviewer": None,
+            "live_control_plane_reviewers": [value for value in result.payload if isinstance(value, str) and value.strip()],
+            "reason": "no_active_review",
+        }
+    authority = resolve_reviewer_authority(
+        bot,
+        issue_number,
+        review_data,
+        is_pull_request=bool(request.is_pull_request),
+    )
+    status = str(authority.get("authority_status"))
+    resolution = {
+        "authorized": status == "tracked_reviewer_confirmed",
+        "authorization_status": status,
+        "review_data": review_data,
+        "tracked_reviewer": authority.get("tracked_reviewer"),
+        "live_control_plane_reviewers": list(authority.get("live_control_plane_reviewers") or []),
+        "reason": authority.get("reason"),
+    }
+    if not resolution["authorized"] or actor is None:
+        return resolution
+    tracked_reviewer = resolution.get("tracked_reviewer")
+    if not isinstance(tracked_reviewer, str) or tracked_reviewer.lower() != actor.lower():
+        resolution["authorized"] = False
+        resolution["authorization_status"] = "actor_not_current_reviewer"
+        resolution["reason"] = "actor_not_current_reviewer"
+    return resolution
+
+
+def reviewer_command_authority_failure_message(command_name: str, resolution: dict[str, object]) -> str:
+    status = str(resolution.get("authorization_status") or "")
+    tracked_reviewer = resolution.get("tracked_reviewer")
+    live_reviewers = [value for value in resolution.get("live_control_plane_reviewers") or [] if isinstance(value, str)]
+    if status == "live_read_unavailable":
+        return "❌ Unable to determine current assignees/reviewers from GitHub; refusing to continue."
+    if status in {"no_active_review", "no_tracked_reviewer"}:
+        return "❌ No active tracked review exists for this issue/PR."
+    if status == "actor_not_current_reviewer" and isinstance(tracked_reviewer, str) and tracked_reviewer:
+        return f"❌ Only the current reviewer (@{tracked_reviewer}) can use `/{command_name}`."
+    if status == "control_plane_mismatch":
+        if live_reviewers:
+            return (
+                f"❌ Unable to confirm @{tracked_reviewer} as the current reviewer from GitHub. "
+                f"Live reviewer(s): @{', @'.join(live_reviewers)}."
+            )
+        return f"❌ Unable to confirm @{tracked_reviewer} as the current reviewer from GitHub."
+    return f"❌ Unable to confirm current reviewer authority for `/{command_name}`."
+
+
 def _post_assignment_guidance(bot, request, reviewer: str) -> None:
     if request.is_pull_request:
         bot.github.post_comment(request.issue_number, get_pr_guidance(reviewer, request.issue_author))