fix: close reviewer command authority gaps

Author: PLeVasseur

REVIEWING.md

@@ -74,19 +74,6 @@ Use this to release your assignment from an issue/PR without automatically assig
 @guidelines-bot /release Need to focus on other priorities
 ```
 
-### Record Feedback for Contributor Follow-Up
-
-```
-@guidelines-bot /feedback
-```
-
-Use this after you have provided reviewer feedback and are waiting for the contributor to respond. This keeps reviewer-bot's review state aligned without marking the review complete or changing the assigned reviewer.
-
-**Example:**
-```
-@guidelines-bot /feedback
-```
-
 ### Rectify Review State
 
 ```
@@ -99,7 +86,6 @@ permission to persist reviewer-bot state.
 
 Who can run it:
 - The currently assigned reviewer
-- A maintainer with triage+ permission
 
 What it does (for the current PR only):
 - If the latest review by the assigned reviewer is `APPROVED`, it marks the review complete.
@@ -213,7 +199,6 @@ Life happens! Any of these actions will reset the 14-day clock:
 - **Post a review comment** - Any substantive feedback counts
 - **Use `/pass [reason]`** - Pass the review to the next person if you can't review it
 - **Use `/away YYYY-MM-DD [reason]`** - Step away temporarily (e.g., "On vacation until 2025-02-15")
-- **Use `/feedback`** - Record that reviewer feedback is ready for contributor follow-up
 - **Use `/rectify`** - Reconcile PR review state when review activity happened but bot state is stale
 
 #### Before You Pass: Consider the Learning Opportunity

scripts/reviewer_bot_core/review_state_types.py

@@ -45,6 +45,17 @@ class ReviewChannelState:
     seen_keys: list[str] = field(default_factory=list)
 
 
+@dataclass
+class CurrentCycleReviewerHandoff:
+    """Maps to the exact persisted `/feedback` reviewer handoff shape."""
+
+    source_event_key: str
+    timestamp: str
+    actor: str
+    command_name: str
+    reviewed_head_sha: str | None = None
+
+
 @dataclass
 class ReviewEntryState:
     """Maps to the persisted review entry fields used by the future C1 cutover.
@@ -79,4 +90,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
+    current_cycle_reviewer_handoff: CurrentCycleReviewerHandoff | None = None

scripts/reviewer_bot_core/reviewer_response_policy.py

@@ -333,6 +333,12 @@ def derive_reviewer_response_state(
             None,
             issue_is_pull_request=False,
         )
+        if _compare_cross_channel_conversation(
+            contributor_comment,
+            reviewer_handoff,
+            parse_timestamp=live_review_support.parse_github_timestamp,
+        ) > 0:
+            reviewer_handoff = None
         if reviewer_review_helpers.compare_records(
             reviewer_handoff,
             latest_reviewer_response,
@@ -446,6 +452,25 @@ def derive_reviewer_response_state(
         issue_is_pull_request=True,
     )
 
+    contributor_handoff = contributor_comment
+    contributor_revision = _contributor_revision_handoff_record(
+        review_data,
+        current_head,
+        reviewer_review if isinstance(reviewer_review, dict) else None,
+    )
+    if reviewer_review_helpers.compare_records(
+        contributor_revision,
+        contributor_handoff,
+        parse_timestamp=live_review_support.parse_github_timestamp,
+    ) > 0:
+        contributor_handoff = contributor_revision
+    if _compare_cross_channel_conversation(
+        contributor_handoff,
+        reviewer_handoff,
+        parse_timestamp=live_review_support.parse_github_timestamp,
+    ) > 0:
+        reviewer_handoff = None
+
     if not reviewer_comment and not reviewer_review and not reviewer_handoff:
         if not had_reviewer_review:
             return _decorate_response(
@@ -474,19 +499,6 @@ def derive_reviewer_response_state(
     ) > 0:
         latest_reviewer_response = reviewer_handoff
 
-    contributor_handoff = contributor_comment
-    contributor_revision = _contributor_revision_handoff_record(
-        review_data,
-        current_head,
-        reviewer_review if isinstance(reviewer_review, dict) else None,
-    )
-    if reviewer_review_helpers.compare_records(
-        contributor_revision,
-        contributor_handoff,
-        parse_timestamp=live_review_support.parse_github_timestamp,
-    ) > 0:
-        contributor_handoff = contributor_revision
-
     if _compare_cross_channel_conversation(
         contributor_handoff,
         latest_reviewer_response,

scripts/reviewer_bot_core/state_adapters.py

@@ -12,6 +12,7 @@
 
 from .review_state_types import (
     AcceptedChannelRecord,
+    CurrentCycleReviewerHandoff,
     DismissalAcceptedRecord,
     ReviewChannelState,
     ReviewEntryState,
@@ -38,6 +39,14 @@
     "source_artifact_name",
 }
 
+_REVIEWER_HANDOFF_KEYS = {
+    "source_event_key",
+    "timestamp",
+    "actor",
+    "command_name",
+    "reviewed_head_sha",
+}
+
 
 def _migrate_repair_marker(marker: Any) -> dict[str, Any] | None:
     if not isinstance(marker, dict):
@@ -213,6 +222,45 @@ def _channel_to_persisted(channel: ReviewChannelState) -> dict[str, Any]:
     }
 
 
+def _reviewer_handoff_from_persisted(value: Any) -> CurrentCycleReviewerHandoff | None:
+    if not isinstance(value, dict) or set(value) != _REVIEWER_HANDOFF_KEYS:
+        return None
+    source_event_key = value.get("source_event_key")
+    timestamp = value.get("timestamp")
+    actor = value.get("actor")
+    command_name = value.get("command_name")
+    reviewed_head_sha = value.get("reviewed_head_sha")
+    if not isinstance(source_event_key, str) or not source_event_key.strip():
+        return None
+    if not isinstance(timestamp, str) or not timestamp.strip():
+        return None
+    if not isinstance(actor, str) or not actor.strip():
+        return None
+    if command_name != "feedback":
+        return None
+    if reviewed_head_sha is not None and (not isinstance(reviewed_head_sha, str) or not reviewed_head_sha.strip()):
+        return None
+    return CurrentCycleReviewerHandoff(
+        source_event_key=source_event_key,
+        timestamp=timestamp,
+        actor=actor,
+        command_name=command_name,
+        reviewed_head_sha=reviewed_head_sha,
+    )
+
+
+def _reviewer_handoff_to_persisted(handoff: CurrentCycleReviewerHandoff | None) -> dict[str, Any] | None:
+    if handoff is None:
+        return None
+    return {
+        "source_event_key": handoff.source_event_key,
+        "timestamp": handoff.timestamp,
+        "actor": handoff.actor,
+        "command_name": handoff.command_name,
+        "reviewed_head_sha": handoff.reviewed_head_sha,
+    }
+
+
 def review_entry_from_persisted(review_entry: dict[str, Any] | list[Any] | None) -> ReviewEntryState | None:
     if review_entry is None:
         return None
@@ -297,9 +345,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,
+        current_cycle_reviewer_handoff=_reviewer_handoff_from_persisted(
+            review_entry.get("current_cycle_reviewer_handoff")
+        ),
     )
 
 
@@ -331,7 +379,9 @@ 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),
+        "current_cycle_reviewer_handoff": _reviewer_handoff_to_persisted(
+            review_entry.current_cycle_reviewer_handoff
+        ),
     }
 
 

scripts/reviewer_bot_lib/assignment_flow.py

@@ -220,12 +220,20 @@ def resolve_reviewer_command_authority(
     }
     if not resolution["authorized"] or actor is None:
         return resolution
+    return require_reviewer_command_actor(resolution, actor)
+
+
+def require_reviewer_command_actor(resolution: dict[str, object], actor: str) -> dict[str, object]:
+    if not resolution.get("authorized"):
+        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
+    if isinstance(tracked_reviewer, str) and tracked_reviewer.lower() == actor.lower():
+        return resolution
+    denied = dict(resolution)
+    denied["authorized"] = False
+    denied["authorization_status"] = "actor_not_current_reviewer"
+    denied["reason"] = "actor_not_current_reviewer"
+    return denied
 
 
 def reviewer_command_authority_failure_message(command_name: str, resolution: dict[str, object]) -> str:

scripts/reviewer_bot_lib/commands.py

@@ -176,6 +176,7 @@ def handle_pass_command(
         assignment_request,
         actor=comment_author,
     )
+    authority = assignment_flow.require_reviewer_command_actor(authority, comment_author)
     if not authority.get("authorized"):
         return _reviewer_command_authority_error("pass", authority), False
     issue_data = authority.get("review_data")
@@ -426,7 +427,7 @@ def handle_queue_command(
 
 
 def handle_commands_command(bot) -> tuple[str, bool]:
-    return (f"ℹ️ **Available Commands**\n\n**Pass or step away:**\n- `{bot.BOT_MENTION} /pass [reason]` - Pass this review to next in queue (current reviewer only)\n- `{bot.BOT_MENTION} /away YYYY-MM-DD [reason]` - Step away from queue until a date\n- `{bot.BOT_MENTION} /feedback` - Mark reviewer feedback ready for contributor follow-up\n- `{bot.BOT_MENTION} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission)\n\n**Assign reviewers:**\n- `{bot.BOT_MENTION} /r? @username` - Assign a specific reviewer\n- `{bot.BOT_MENTION} /r? producers` - Request the next reviewer from the queue\n- `{bot.BOT_MENTION} /claim` - Claim this review for yourself\n\n**Other:**\n- `{bot.BOT_MENTION} /done` - Mark a tracked non-PR issue review complete\n- `{bot.BOT_MENTION} /label +label-name` - Add a label\n- `{bot.BOT_MENTION} /label -label-name` - Remove a label\n- `{bot.BOT_MENTION} /rectify` - Reconcile this issue/PR review state from GitHub\n- `{bot.BOT_MENTION} /accept-no-fls-changes` - Update spec.lock and open a PR when no guidelines are impacted\n- `{bot.BOT_MENTION} /queue` - Show current queue status\n- `{bot.BOT_MENTION} /sync-members` - Sync queue with members.md"), True
+    return (f"ℹ️ **Available Commands**\n\n**Pass or step away:**\n- `{bot.BOT_MENTION} /pass [reason]` - Pass this review to next in queue (current reviewer only)\n- `{bot.BOT_MENTION} /away YYYY-MM-DD [reason]` - Step away from queue until a date\n- `{bot.BOT_MENTION} /feedback` - Mark reviewer feedback ready for contributor follow-up\n- `{bot.BOT_MENTION} /release [reason]` - Release your current reviewer assignment\n\n**Assign reviewers:**\n- `{bot.BOT_MENTION} /r? @username` - Assign a specific reviewer\n- `{bot.BOT_MENTION} /r? producers` - Request the next reviewer from the queue\n- `{bot.BOT_MENTION} /claim` - Claim this review for yourself\n\n**Other:**\n- `{bot.BOT_MENTION} /done` - Mark a tracked non-PR issue review complete\n- `{bot.BOT_MENTION} /label +label-name` - Add a label\n- `{bot.BOT_MENTION} /label -label-name` - Remove a label\n- `{bot.BOT_MENTION} /rectify` - Reconcile this issue/PR review state from GitHub (current reviewer only)\n- `{bot.BOT_MENTION} /accept-no-fls-changes` - Update spec.lock and open a PR when no guidelines are impacted\n- `{bot.BOT_MENTION} /queue` - Show current queue status\n- `{bot.BOT_MENTION} /sync-members` - Sync queue with members.md"), True
 
 
 def handle_claim_command(
@@ -475,11 +476,9 @@ def handle_release_command(
     request = request or build_assignment_request(bot, issue_number=issue_number)
     target_username = None
     reason = None
-    releasing_other = False
     if args and args[0].startswith("@"):
         target_username = args[0].lstrip("@")
         reason = " ".join(args[1:]) if len(args) > 1 else None
-        releasing_other = target_username.lower() != comment_author.lower()
     else:
         target_username = comment_author
         reason = " ".join(args) if args else None
@@ -489,22 +488,18 @@ def handle_release_command(
         issue_data = state["active_reviews"][issue_key]
         if isinstance(issue_data, dict):
             assignment_method = issue_data.get("assignment_method")
-    authority = reviewer_authority or assignment_flow.resolve_reviewer_command_authority(bot, state, request)
+    authority = reviewer_authority or assignment_flow.resolve_reviewer_command_authority(
+        bot,
+        state,
+        request,
+        actor=comment_author,
+    )
+    authority = assignment_flow.require_reviewer_command_actor(authority, comment_author)
     if not authority.get("authorized"):
         return _reviewer_command_authority_error("release", authority), False
     tracked_reviewer = str(authority["tracked_reviewer"])
     if target_username.lower() != tracked_reviewer.lower():
         return (f"❌ @{target_username} is not the current reviewer. Current reviewer: @{tracked_reviewer}"), False
-    if releasing_other:
-        permission_status = bot.github.get_user_permission_status(comment_author, "triage")
-        if permission_status == "unavailable":
-            return "❌ Unable to verify triage permissions right now; refusing to continue.", False
-        if permission_status != "granted":
-            return (f"❌ @{comment_author} does not have permission to release other reviewers. Triage access or higher is required."), False
-    if not releasing_other and comment_author.lower() != tracked_reviewer.lower():
-        return (
-            f"❌ Only the current reviewer (@{tracked_reviewer}) can use `/release` without triage+ permission."
-        ), False
     result = assignment_flow.confirm_reviewer_release(
         bot,
         state,
@@ -517,8 +512,6 @@ def handle_release_command(
             result.get("diagnostic_changed") or result.get("cleared_current_reviewer")
         )
     reason_text = f" Reason: {reason}" if reason else ""
-    if releasing_other:
-        return (f"✅ @{comment_author} has released @{target_username} from this review.{reason_text}\n\n_This issue/PR is now unassigned. Use `{bot.BOT_MENTION} /r? producers` to assign the next reviewer from the queue, or `{bot.BOT_MENTION} /claim` to claim it._"), True
     return (f"✅ @{target_username} has released this review.{reason_text}\n\n_This issue/PR is now unassigned. Use `{bot.BOT_MENTION} /r? producers` to assign the next reviewer from the queue, or `{bot.BOT_MENTION} /claim` to claim it._"), True
 
 

scripts/reviewer_bot_lib/comment_application.py

@@ -226,7 +226,7 @@ def _execute_release(bot, state: dict, decision, assignment_request: AssignmentR
         bot,
         state,
         assignment_request,
-        actor=None,
+        actor=decision.actor,
     )
     return _build_execution_result(
         decision.command_id,

scripts/reviewer_bot_lib/config.py

@@ -186,7 +186,7 @@
     "pass": "Pass this review to next in queue",
     "away": "Step away from queue until date (YYYY-MM-DD)",
     "feedback": "Record that reviewer feedback is ready for contributor follow-up",
-    "release": "Release assignment (yours, or @username with triage+ permission)",
+    "release": "Release your current reviewer assignment",
     "rectify": "Reconcile this issue/PR's review state from GitHub",
     "claim": "Claim this review for yourself",
     "r?": "Assign a reviewer (@username or 'producers')",

scripts/reviewer_bot_lib/guidance.py

@@ -48,7 +48,7 @@ def get_issue_guidance(reviewer: str, issue_author: str) -> str:
 - `{BOT_MENTION} /pass [reason]` - Pass just this issue to the next reviewer
 - `{BOT_MENTION} /away YYYY-MM-DD [reason]` - Step away from the queue until a date
 - `{BOT_MENTION} /feedback` - Mark reviewer feedback ready for contributor follow-up
-- `{BOT_MENTION} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission)
+- `{BOT_MENTION} /release [reason]` - Release your current reviewer assignment
 
 To assign someone else:
 - `{BOT_MENTION} /r? @username` - Assign a specific reviewer
@@ -81,7 +81,7 @@ def get_generic_issue_guidance(reviewer: str, issue_author: str) -> str:
 - `{BOT_MENTION} /pass [reason]` - Pass just this issue to the next reviewer
 - `{BOT_MENTION} /away YYYY-MM-DD [reason]` - Step away from the queue until a date
 - `{BOT_MENTION} /feedback` - Mark reviewer feedback ready for contributor follow-up
-- `{BOT_MENTION} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission)
+- `{BOT_MENTION} /release [reason]` - Release your current reviewer assignment
 
 To assign someone else:
 - `{BOT_MENTION} /r? @username` - Assign a specific reviewer
@@ -119,7 +119,7 @@ def get_fls_audit_guidance(reviewer: str, issue_author: str) -> str:
 - `{BOT_MENTION} /pass [reason]` - Pass just this issue to the next reviewer
 - `{BOT_MENTION} /away YYYY-MM-DD [reason]` - Step away from the queue until a date
 - `{BOT_MENTION} /feedback` - Mark reviewer feedback ready for contributor follow-up
-- `{BOT_MENTION} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission)
+- `{BOT_MENTION} /release [reason]` - Release your current reviewer assignment
 
 To assign someone else:
 - `{BOT_MENTION} /r? @username` - Assign a specific reviewer
@@ -165,7 +165,7 @@ def get_pr_guidance(reviewer: str, pr_author: str) -> str:
 - `{BOT_MENTION} /pass [reason]` - Pass just this PR to the next reviewer
 - `{BOT_MENTION} /away YYYY-MM-DD [reason]` - Step away from the queue until a date
 - `{BOT_MENTION} /feedback` - Mark reviewer feedback ready for contributor follow-up
-- `{BOT_MENTION} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission)
+- `{BOT_MENTION} /release [reason]` - Release your current reviewer assignment
 
 To assign someone else:
 - `{BOT_MENTION} /r? @username` - Assign a specific reviewer