diff --git a/.github/reviewer-bot-tests/test_reviewer_bot.py b/.github/reviewer-bot-tests/test_reviewer_bot.py index ee50d2483..a9f93928e 100644 --- a/.github/reviewer-bot-tests/test_reviewer_bot.py +++ b/.github/reviewer-bot-tests/test_reviewer_bot.py @@ -191,6 +191,20 @@ def test_parse_command_malformed_unknown(): assert args == ["greetings"] +@pytest.mark.parametrize( + ("guidance_builder", "builder_args"), + [ + (reviewer_bot.get_issue_guidance, ("alice", "bob")), + (reviewer_bot.get_fls_audit_guidance, ("alice", "bob")), + (reviewer_bot.get_pr_guidance, ("alice", "bob")), + ], +) +def test_guidance_release_commands_are_explicit(guidance_builder, builder_args): + guidance = guidance_builder(*builder_args) + assert "@guidelines-bot /release [reason]" in guidance + assert "@guidelines-bot /release @username [reason]" in guidance + + def test_github_api_error_handling(monkeypatch): class FakeResponse: def __init__(self, status_code, content): @@ -426,7 +440,10 @@ def test_handle_comment_event_commands_command(stub_api, captured_comments): handled = reviewer_bot.handle_comment_event(state) assert handled is False assert len(captured_comments) == 1 - assert "Available Commands" in captured_comments[0]["body"] + response = captured_comments[0]["body"] + assert "Available Commands" in response + assert "@guidelines-bot /release [reason]" in response + assert "@guidelines-bot /release @username [reason]" in response def test_handle_comment_event_away_command(stub_api, captured_comments, monkeypatch): @@ -462,6 +479,49 @@ def test_handle_comment_event_release_command_self(stub_api, captured_comments, assert "has released" in captured_comments[0]["body"] +def test_handle_comment_event_release_command_self_not_current_suggests_target( + stub_api, captured_comments, monkeypatch +): + state = make_state() + state["active_reviews"]["42"] = { + "skipped": [], + "current_reviewer": "alice", + "assignment_method": "round-robin", + } + monkeypatch.setattr(reviewer_bot, "get_issue_assignees", lambda *args, **kwargs: ["alice"]) + os.environ["COMMENT_BODY"] = "@guidelines-bot /release" + os.environ["COMMENT_AUTHOR"] = "bob" + os.environ["ISSUE_NUMBER"] = "42" + + handled = reviewer_bot.handle_comment_event(state) + + assert handled is False + assert len(captured_comments) == 1 + response = captured_comments[0]["body"] + assert "@bob is not the current reviewer" in response + assert "Current reviewer: @alice" in response + assert "@guidelines-bot /release @alice" in response + assert "triage+ required" in response + + +def test_handle_release_command_self_not_assigned_uses_single_assignee_hint(stub_api, monkeypatch): + state = make_state() + monkeypatch.setattr(reviewer_bot, "get_issue_assignees", lambda *args, **kwargs: ["alice"]) + + response, success = reviewer_bot.handle_release_command( + state=state, + issue_number=42, + comment_author="bob", + args=[], + ) + + assert success is False + assert "@bob is not assigned to this issue/PR" in response + assert "Current assignee(s): @alice" in response + assert "@guidelines-bot /release @alice" in response + assert "triage+ required" in response + + def test_handle_comment_event_release_command_other_requires_permission( stub_api, captured_comments, monkeypatch ): diff --git a/scripts/reviewer_bot.py b/scripts/reviewer_bot.py index 954168613..b6ccf1323 100644 --- a/scripts/reviewer_bot.py +++ b/scripts/reviewer_bot.py @@ -20,8 +20,12 @@ - Assign yourself as the reviewer for this issue/PR - Removes any existing reviewer assignment - @guidelines-bot /release [@username] [reason] - - Release your assignment from this issue/PR (or someone else's with triage+ permission) + @guidelines-bot /release [reason] + - Release your own assignment from this issue/PR + - Leaves this issue/PR unassigned + + @guidelines-bot /release @username [reason] + - Release someone else's assignment from this issue/PR (triage+ required) - Does NOT auto-assign the next reviewer (use /pass for that) @guidelines-bot /rectify @@ -96,7 +100,7 @@ COMMANDS = { "pass": "Pass this review to next in queue", "away": "Step away from queue until date (YYYY-MM-DD)", - "release": "Release assignment (yours, or @username with triage+ permission)", + "release": "Release your assignment (/release) or another's (/release @username, triage+)", "rectify": "Reconcile this issue/PR's review state from GitHub", "claim": "Claim this review for yourself", "r?": "Assign a reviewer (@username or 'producers')", @@ -673,7 +677,8 @@ def get_issue_guidance(reviewer: str, issue_author: str) -> str: If you need to pass this review: - `{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} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission) +- `{BOT_MENTION} /release [reason]` - Release your own assignment and leave this issue unassigned +- `{BOT_MENTION} /release @username [reason]` - Release another reviewer's assignment (triage+ required) To assign someone else: - `{BOT_MENTION} /r? @username` - Assign a specific reviewer @@ -709,7 +714,8 @@ def get_fls_audit_guidance(reviewer: str, issue_author: str) -> str: If you need to pass this review: - `{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} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission) +- `{BOT_MENTION} /release [reason]` - Release your own assignment and leave this issue unassigned +- `{BOT_MENTION} /release @username [reason]` - Release another reviewer's assignment (triage+ required) To assign someone else: - `{BOT_MENTION} /r? @username` - Assign a specific reviewer @@ -753,7 +759,8 @@ def get_pr_guidance(reviewer: str, pr_author: str) -> str: If you need to pass this review: - `{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} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission) +- `{BOT_MENTION} /release [reason]` - Release your own assignment and leave this PR unassigned +- `{BOT_MENTION} /release @username [reason]` - Release another reviewer's assignment (triage+ required) To assign someone else: - `{BOT_MENTION} /r? @username` - Assign a specific reviewer @@ -1437,7 +1444,8 @@ def handle_commands_command() -> tuple[str, bool]: f"**Pass or step away:**\n" f"- `{BOT_MENTION} /pass [reason]` - Pass this review to next in queue (current reviewer only)\n" f"- `{BOT_MENTION} /away YYYY-MM-DD [reason]` - Step away from queue until a date\n" - f"- `{BOT_MENTION} /release [@username] [reason]` - Release assignment (yours or someone else's with triage+ permission)\n\n" + f"- `{BOT_MENTION} /release [reason]` - Release your own assignment and leave this unassigned\n" + f"- `{BOT_MENTION} /release @username [reason]` - Release another reviewer's assignment (triage+ required)\n\n" f"**Assign reviewers:**\n" f"- `{BOT_MENTION} /r? @username` - Assign a specific reviewer\n" f"- `{BOT_MENTION} /r? producers` - Request the next reviewer from the queue\n" @@ -1572,10 +1580,19 @@ def handle_release_command(state: dict, issue_number: int, # Trying to release self when not assigned if tracked_reviewer: return (f"❌ @{comment_author} is not the current reviewer. " - f"Current reviewer: @{tracked_reviewer}"), False + f"Current reviewer: @{tracked_reviewer}\n\n" + f"If you meant to release @{tracked_reviewer}, use " + f"`{BOT_MENTION} /release @{tracked_reviewer}` " + f"(triage+ required)."), False elif current_assignees: - return (f"❌ @{comment_author} is not assigned to this issue/PR. " - f"Current assignee(s): @{', @'.join(current_assignees)}"), False + response = (f"❌ @{comment_author} is not assigned to this issue/PR. " + f"Current assignee(s): @{', @'.join(current_assignees)}") + if len(current_assignees) == 1: + current_assignee = current_assignees[0] + response += (f"\n\nIf you meant to release @{current_assignee}, use " + f"`{BOT_MENTION} /release @{current_assignee}` " + f"(triage+ required).") + return response, False else: return "❌ No reviewer is currently assigned to release.", False