fix: finalize reviewer feedback handoff ordering

Author: PLeVasseur

scripts/reviewer_bot_core/review_state_machine.py

@@ -97,6 +97,18 @@ def _compare_records(left: dict | None, right: dict | None) -> int:
     return 0
 
 
+def _clear_handoff_after_newer_contributor_event(review_data: dict, timestamp: str) -> bool:
+    handoff = review_data.get("current_cycle_reviewer_handoff")
+    if not isinstance(handoff, dict):
+        return False
+    contributor_time = parse_github_timestamp(timestamp)
+    handoff_time = parse_github_timestamp(handoff.get("timestamp"))
+    if contributor_time is None or handoff_time is None or contributor_time <= handoff_time:
+        return False
+    review_data["current_cycle_reviewer_handoff"] = None
+    return True
+
+
 def semantic_key_seen(review_data: dict, channel_name: str, semantic_key: str) -> bool:
     channel = _ensure_channel_map(review_data, channel_name)
     return semantic_key in channel["seen_keys"]
@@ -135,6 +147,8 @@ def accept_channel_event(
     current = channel.get("accepted")
     if _compare_records(candidate, current) >= 0:
         channel["accepted"] = candidate
+    if channel_name in {"contributor_comment", "contributor_revision"}:
+        _clear_handoff_after_newer_contributor_event(review_data, timestamp)
     return True
 
 

scripts/reviewer_bot_lib/commands.py

@@ -480,11 +480,6 @@ def handle_release_command(
         target_username = args[0].lstrip("@")
         reason = " ".join(args[1:]) if len(args) > 1 else None
         releasing_other = target_username.lower() != comment_author.lower()
-        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 releasing_other and permission_status != "granted":
-            return (f"❌ @{comment_author} does not have permission to release other reviewers. Triage access or higher is required."), False
     else:
         target_username = comment_author
         reason = " ".join(args) if args else None
@@ -500,6 +495,12 @@ def handle_release_command(
     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."

scripts/reviewer_bot_lib/comment_application.py

@@ -280,7 +280,13 @@ def _feedback_handoff_should_replace(existing: object, candidate: dict) -> bool:
         return existing_key is None
     if existing_key is None:
         return True
-    return candidate_key >= existing_key
+    candidate_timestamp, candidate_source = candidate_key
+    existing_timestamp, existing_source = existing_key
+    if candidate_timestamp > existing_timestamp:
+        return True
+    if candidate_timestamp < existing_timestamp:
+        return False
+    return candidate_source == existing_source
 
 
 def handle_feedback_command(bot, state: dict, request: CommentEventRequest, decision) -> CommandExecutionResult:
@@ -325,7 +331,7 @@ def handle_feedback_command(bot, state: dict, request: CommentEventRequest, deci
     before = review_data.get("current_cycle_reviewer_handoff")
     if not _feedback_handoff_should_replace(before, handoff):
         return CommandExecutionResult(
-            response="ℹ️ Ignored stale `/feedback` handoff because a newer reviewer handoff is already recorded.",
+            response="ℹ️ Ignored stale `/feedback` handoff because a newer or same-time reviewer handoff is already recorded.",
             success=True,
             state_changed=False,
         )

tests/contract/reviewer_bot/test_review_state_contract.py

@@ -252,6 +252,80 @@ def test_current_cycle_reviewer_handoff_is_cleared_by_reviewer_replacement_and_r
     assert review["current_cycle_reviewer_handoff"] is None
 
 
+def test_current_cycle_reviewer_handoff_is_cleared_by_newer_contributor_followup():
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_cycle_reviewer_handoff"] = {
+        "source_event_key": "issue_comment:100",
+        "timestamp": "2026-03-17T10:00:00Z",
+        "actor": "alice",
+        "command_name": "feedback",
+        "reviewed_head_sha": None,
+    }
+
+    changed = review_state.accept_channel_event(
+        review,
+        "contributor_comment",
+        semantic_key="issue_comment:101",
+        timestamp="2026-03-17T11:00:00Z",
+        actor="dana",
+    )
+
+    assert changed is True
+    assert review["current_cycle_reviewer_handoff"] is None
+
+
+def test_current_cycle_reviewer_handoff_survives_older_contributor_followup():
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    handoff = {
+        "source_event_key": "issue_comment:100",
+        "timestamp": "2026-03-17T10:00:00Z",
+        "actor": "alice",
+        "command_name": "feedback",
+        "reviewed_head_sha": None,
+    }
+    review["current_cycle_reviewer_handoff"] = dict(handoff)
+
+    changed = review_state.accept_channel_event(
+        review,
+        "contributor_comment",
+        semantic_key="issue_comment:99",
+        timestamp="2026-03-17T09:00:00Z",
+        actor="dana",
+    )
+
+    assert changed is True
+    assert review["current_cycle_reviewer_handoff"] == handoff
+
+
+def test_current_cycle_reviewer_handoff_is_cleared_by_newer_contributor_revision():
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_cycle_reviewer_handoff"] = {
+        "source_event_key": "issue_comment:100",
+        "timestamp": "2026-03-17T10:00:00Z",
+        "actor": "alice",
+        "command_name": "feedback",
+        "reviewed_head_sha": "head-1",
+    }
+
+    changed = review_state.accept_channel_event(
+        review,
+        "contributor_revision",
+        semantic_key="pull_request_sync:42:head-2",
+        timestamp="2026-03-17T11:00:00Z",
+        reviewed_head_sha="head-2",
+        source_precedence=1,
+    )
+
+    assert changed is True
+    assert review["current_cycle_reviewer_handoff"] is None
+
+
 def test_list_open_tracked_review_items_returns_only_assigned_entries():
     state = make_state()
     review_state.ensure_review_entry(state, 42, create=True)

tests/unit/reviewer_bot/test_commands.py

@@ -258,6 +258,30 @@ def test_release_command_accepts_confirmed_pr_reviewer_when_live_reviewers_are_e
     assert review["current_reviewer"] is None
 
 
+def test_release_command_resolves_reviewer_authority_before_triage_fallback(monkeypatch):
+    harness = CommandHarness(monkeypatch)
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "bob"
+    harness.stub_assignees(["bob"])
+    calls = []
+    original_resolver = commands.assignment_flow.resolve_reviewer_command_authority
+
+    def resolve_with_order(*args, **kwargs):
+        calls.append("resolver")
+        return original_resolver(*args, **kwargs)
+
+    monkeypatch.setattr(commands.assignment_flow, "resolve_reviewer_command_authority", resolve_with_order)
+    harness.runtime.github.get_user_permission_status = lambda username, required_permission="triage": calls.append("permission") or "granted"
+
+    response, success = harness.handle_release(state, 42, "alice", ["@bob"])
+
+    assert success is True
+    assert "@alice has released @bob" in response
+    assert calls[:2] == ["resolver", "permission"]
+
+
 def test_rectify_command_accepts_confirmed_pr_reviewer_when_live_reviewers_are_empty(monkeypatch):
     harness = CommandHarness(monkeypatch)
     state = make_state()
@@ -279,6 +303,31 @@ def test_rectify_command_accepts_confirmed_pr_reviewer_when_live_reviewers_are_e
     assert calls == [(harness.runtime, state, 42)]
 
 
+def test_rectify_command_resolves_reviewer_authority_before_triage_fallback(monkeypatch):
+    harness = CommandHarness(monkeypatch)
+    state = make_state()
+    harness.stub_assignees([])
+    calls = []
+    original_resolver = reconcile.assignment_flow.resolve_reviewer_command_authority
+
+    def resolve_with_order(*args, **kwargs):
+        calls.append("resolver")
+        return original_resolver(*args, **kwargs)
+
+    monkeypatch.setattr(reconcile.assignment_flow, "resolve_reviewer_command_authority", resolve_with_order)
+    harness.runtime.github.get_user_permission_status = lambda username, required_permission="triage": calls.append("permission") or "granted"
+    monkeypatch.setattr(
+        reconcile,
+        "reconcile_active_review_entry",
+        lambda bot, current_state, issue_number: calls.append("reconcile") or ("rectified", True, False),
+    )
+
+    message, success, changed = harness.handle_rectify(state, 42, "maintainer")
+
+    assert (message, success, changed) == ("rectified", True, False)
+    assert calls == ["resolver", "permission", "reconcile"]
+
+
 def test_assign_from_queue_posts_guidance_only_once(monkeypatch):
     harness = CommandHarness(monkeypatch)
     state = make_state()
@@ -493,6 +542,10 @@ def test_claim_command_fails_closed_when_assignees_unavailable(monkeypatch):
 def test_release_command_fails_closed_when_permission_unavailable(monkeypatch):
     harness = CommandHarness(monkeypatch)
     state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "bob"
+    harness.stub_assignees(["bob"])
     harness.runtime.github.get_user_permission_status = lambda username, required_permission="triage": "unavailable"
 
     response, success = harness.handle_release(state, 42, "alice", ["@bob"])
@@ -894,11 +947,138 @@ def test_feedback_command_does_not_overwrite_newer_handoff(monkeypatch):
         "reviewed_head_sha": None,
     }
     assert side_effects.comments == [
-        (42, "ℹ️ Ignored stale `/feedback` handoff because a newer reviewer handoff is already recorded.")
+        (42, "ℹ️ Ignored stale `/feedback` handoff because a newer or same-time reviewer handoff is already recorded.")
     ]
     assert side_effects.reactions == [(100, "eyes"), (100, "+1")]
 
 
+def test_feedback_command_does_not_overwrite_same_timestamp_different_event(monkeypatch):
+    harness = CommandHarness(monkeypatch)
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    review["current_cycle_reviewer_handoff"] = {
+        "source_event_key": "issue_comment:100",
+        "timestamp": "2026-03-17T10:00:00Z",
+        "actor": "alice",
+        "command_name": "feedback",
+        "reviewed_head_sha": None,
+    }
+    harness.stub_assignees(["alice"])
+    side_effects = harness.capture_comment_side_effects()
+    request = harness.typed_comment_request(
+        issue_number=42,
+        actor="alice",
+        body="@guidelines-bot /feedback",
+        issue_author="dana",
+        is_pull_request=False,
+        comment_id=101,
+        created_at="2026-03-17T10:00:00Z",
+    )
+
+    changed = comment_application.apply_comment_command(
+        harness.runtime,
+        state,
+        request,
+        {"command": "feedback", "args": [], "command_count": 1},
+        classify_issue_comment_actor=lambda current_request: "repo_user_principal",
+    )
+
+    assert changed is False
+    assert review["current_cycle_reviewer_handoff"]["source_event_key"] == "issue_comment:100"
+    assert side_effects.comments == [
+        (42, "ℹ️ Ignored stale `/feedback` handoff because a newer or same-time reviewer handoff is already recorded.")
+    ]
+
+
+def test_feedback_command_replays_same_event_idempotently(monkeypatch):
+    harness = CommandHarness(monkeypatch)
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    review["last_reviewer_activity"] = "2026-03-17T10:00:00Z"
+    review["current_cycle_reviewer_handoff"] = {
+        "source_event_key": "issue_comment:100",
+        "timestamp": "2026-03-17T10:00:00Z",
+        "actor": "alice",
+        "command_name": "feedback",
+        "reviewed_head_sha": None,
+    }
+    harness.stub_assignees(["alice"])
+    harness.capture_comment_side_effects()
+    request = harness.typed_comment_request(
+        issue_number=42,
+        actor="alice",
+        body="@guidelines-bot /feedback",
+        issue_author="dana",
+        is_pull_request=False,
+        comment_id=100,
+        created_at="2026-03-17T10:00:00Z",
+    )
+
+    changed = comment_application.apply_comment_command(
+        harness.runtime,
+        state,
+        request,
+        {"command": "feedback", "args": [], "command_count": 1},
+        classify_issue_comment_actor=lambda current_request: "repo_user_principal",
+    )
+
+    assert changed is False
+    assert review["current_cycle_reviewer_handoff"] == {
+        "source_event_key": "issue_comment:100",
+        "timestamp": "2026-03-17T10:00:00Z",
+        "actor": "alice",
+        "command_name": "feedback",
+        "reviewed_head_sha": None,
+    }
+
+
+def test_feedback_command_overwrites_older_handoff(monkeypatch):
+    harness = CommandHarness(monkeypatch)
+    state = make_state()
+    review = review_state.ensure_review_entry(state, 42, create=True)
+    assert review is not None
+    review["current_reviewer"] = "alice"
+    review["current_cycle_reviewer_handoff"] = {
+        "source_event_key": "issue_comment:100",
+        "timestamp": "2026-03-17T09:00:00Z",
+        "actor": "alice",
+        "command_name": "feedback",
+        "reviewed_head_sha": None,
+    }
+    harness.stub_assignees(["alice"])
+    harness.capture_comment_side_effects()
+    request = harness.typed_comment_request(
+        issue_number=42,
+        actor="alice",
+        body="@guidelines-bot /feedback",
+        issue_author="dana",
+        is_pull_request=False,
+        comment_id=101,
+        created_at="2026-03-17T10:00:00Z",
+    )
+
+    changed = comment_application.apply_comment_command(
+        harness.runtime,
+        state,
+        request,
+        {"command": "feedback", "args": [], "command_count": 1},
+        classify_issue_comment_actor=lambda current_request: "repo_user_principal",
+    )
+
+    assert changed is True
+    assert review["current_cycle_reviewer_handoff"] == {
+        "source_event_key": "issue_comment:101",
+        "timestamp": "2026-03-17T10:00:00Z",
+        "actor": "alice",
+        "command_name": "feedback",
+        "reviewed_head_sha": None,
+    }
+
+
 def test_feedback_command_plus_text_does_not_record_reviewer_comment_channel(monkeypatch):
     harness = CommentRoutingHarness(monkeypatch)
     state = make_state()

tests/unit/reviewer_bot/test_reviewer_response_equivalence.py

@@ -490,7 +490,7 @@ def test_issue_feedback_handoff_before_cycle_boundary_is_not_consumed():
     assert result["current_cycle_reviewer_handoff"] is None
 
 
-def test_issue_feedback_handoff_is_consumed_as_stale_after_newer_contributor_followup():
+def test_issue_feedback_handoff_is_cleared_after_newer_contributor_followup():
     state = make_state()
     review = make_tracked_review_state(
         state,
@@ -516,8 +516,9 @@ def test_issue_feedback_handoff_is_consumed_as_stale_after_newer_contributor_fol
     result = reviewer_response_policy.derive_reviewer_response_state(review, issue_is_pull_request=False)
 
     assert result["state"] == "awaiting_reviewer_response"
-    assert result["reason"] == "contributor_comment_newer"
-    assert result["anchor_timestamp"] == "2026-03-17T11:00:00Z"
+    assert result["reason"] == "no_reviewer_activity"
+    assert result["current_cycle_reviewer_handoff"] is None
+    assert review["current_cycle_reviewer_handoff"] is None
 
 
 def test_pr_feedback_handoff_requires_current_tracked_head():