From 3d2eb75ae5e3059378403c637113465f083a135b Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 29 Jul 2026 16:31:24 -0700 Subject: [PATCH 1/3] Classify top-level reviewer feedback with the author-action binary --- .../pull-request-dashboard/classification.py | 99 ++++++++++++++++++- .../eval/score_reviewer_feedback.py | 7 +- .../test_top_level_actions.py | 71 ++++++------- 3 files changed, 126 insertions(+), 51 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/classification.py b/.github/scripts/pull-request-dashboard/classification.py index 1e5b7d9e676..28aec6b316b 100644 --- a/.github/scripts/pull-request-dashboard/classification.py +++ b/.github/scripts/pull-request-dashboard/classification.py @@ -167,6 +167,58 @@ never affects any other item.""" +REVIEWER_FEEDBACK_PROMPT_TEMPLATE = ( + """You are triaging top-level feedback items from pull request reviewers. + +""" + + BATCH_CONTRACT + + """ + +Each item contains the reviewer's login in `requester`, the PR author's login in +`pr_author`, and the comment text in `body`. First-person statements in `body` +are the reviewer speaking, never the PR author. + +Question: does this item leave something unresolved that `pr_author` must handle +before this pull request can merge? + + - author_action: anything the author would answer or act on, including + questions, requests, objections, remarks that reject the pull request's + premise or necessity without asking for anything, an answer to a question + the author asked, and a statement that this pull request is blocked on + another pull request, release, or decision + - no_author_action: the item needs nothing from the PR author, such as pure + approval, thanks, a status summary, or a repository automation command (for + example "/workflow-approve", "/rerun", or "/easycla") + +Read the whole item before deciding. Approval is no_author_action however it is +phrased ("LGTM", "I'm fine with the API changes", "looks good to me, feel free +to merge"), and stays no_author_action when it carries a suggestion the reviewer +explicitly leaves for later ("we can clean this up post submission", "an +opportunity to refactor after a point fix release", "left one small +maintainability comment"). An item that ends by telling the author they may +merge is always no_author_action. + +Compare every login and team mentioned in `body` against `pr_author`. An item +asking a different person or team to review, decide, or weigh in is +no_author_action even when it describes a concern with this pull request. + +Do not decide whether the author already responded. That is determined later +from comment timestamps. + +When you cannot tell, answer author_action: ambiguity keeps the item with the +author. + +Respond with a single JSON object and nothing else. Include exactly one result +for every input discussion_id and copy each discussion_id exactly: +{{"items": [{{"discussion_id": "input id", "verdict": "author_action" | "no_author_action", "reason": "short explanation grounded in this item"}}]}} + +---BEGIN TOP-LEVEL FEEDBACK--- +{discussions} +---END TOP-LEVEL FEEDBACK--- +""" +) + + PRAISE_PROMPT_TEMPLATE = ( """You are triaging single comments left by reviewers on pull requests. @@ -241,6 +293,7 @@ TOP_LEVEL_DISCUSSION_ACTIONS = ("author", "none", "unclear") # Each binary lists its fail-safe verdict first: an unreadable answer keeps the # item with the author rather than handing the pull request to reviewers. +REVIEWER_FEEDBACK_VERDICTS = ("author_action", "no_author_action") AUTHOR_REPLY_VERDICTS = ("deferral", "complete") PRAISE_VERDICTS = ("not_praise", "praise") @@ -915,6 +968,7 @@ def review_thread_author_reply_input(discussion: dict[str, Any]) -> dict[str, An REVIEW_THREAD_REPLY_ACTIONS = {"deferral": "author", "complete": "reviewer"} +REVIEWER_FEEDBACK_ACTIONS = {"author_action": "author", "no_author_action": "none"} PRAISE_ACTIONS = {"praise": "none", "not_praise": "author"} # Pure praise is short. The longest in a 441 pull request corpus was 13 characters, # so this is wide headroom, and anything longer stays the author's without a call. @@ -929,7 +983,7 @@ def _could_be_praise(discussion: dict[str, Any]) -> bool: return len(" ".join((comments[-1].get("body") or "").split())) <= PRAISE_MAX_CHARS -def _thread_record(record: dict[str, Any], actions: dict[str, str]) -> dict[str, Any]: +def _verdict_record(record: dict[str, Any], actions: dict[str, str]) -> dict[str, Any]: """Restate a binary's verdict as the discussion action the dashboard routes on. A failed call keeps the thread with its author whatever verdict it still parsed. @@ -1003,7 +1057,7 @@ def classify_review_threads( # a praise call that failed keeps its failure rather than becoming a clean # deterministic answer, and routes to the author like any other unusable verdict failed_praise: dict[str, dict[str, Any]] = { - discussion_id: _thread_record(record, PRAISE_ACTIONS) + discussion_id: _verdict_record(record, PRAISE_ACTIONS) for discussion_id, record in praise.items() if record.get("failed") } @@ -1047,7 +1101,7 @@ def classify_review_threads( prompt_input=review_thread_author_reply_input, ) for discussion_id, record in replies.items(): - classifications_by_id[discussion_id] = _thread_record(record, REVIEW_THREAD_REPLY_ACTIONS) + classifications_by_id[discussion_id] = _verdict_record(record, REVIEW_THREAD_REPLY_ACTIONS) for discussion_id, since in since_by_id.items(): if since and discussion_id in classifications_by_id: classifications_by_id[discussion_id]["since"] = since @@ -1308,6 +1362,43 @@ def run_llm_for_verdict_batch( return records +def classify_reviewer_feedback( + number: int, + discussions: list[dict[str, Any]], + model: str, + cache_in: dict[str, dict[str, Any]], + cache_out: dict[str, dict[str, Any]], +) -> dict[str, dict[str, Any]]: + records = classify_top_level_items( + number, + discussions, + model, + cache_in, + cache_out, + prompt_template=REVIEWER_FEEDBACK_PROMPT_TEMPLATE, + prompt_input=top_level_reviewer_feedback_prompt_input, + run_batch=lambda batch, m: [ + record + for items, prompt in verdict_prompt_batches( + batch, + REVIEWER_FEEDBACK_PROMPT_TEMPLATE, + top_level_reviewer_feedback_prompt_input, + ) + for record in run_llm_for_verdict_batch( + items, m, prompt, REVIEWER_FEEDBACK_VERDICTS + ) + ], + fallback_decision=lambda reason: unclear_verdict_decision( + reason, REVIEWER_FEEDBACK_VERDICTS + ), + warning_label="reviewer_feedback", + ) + return { + discussion_id: _verdict_record(record, REVIEWER_FEEDBACK_ACTIONS) + for discussion_id, record in records.items() + } + + def classify_author_replies( number: int, discussions: list[dict[str, Any]], @@ -1378,7 +1469,7 @@ def classify_discussion_domains( review_thread_classifications = classify_review_threads( number, review_threads, model, cache_in, cache_out ) - top_level_classifications = classify_top_level_reviewer_feedback_items( + top_level_classifications = classify_reviewer_feedback( number, top_level_items, model, cache_in, cache_out ) top_level_author_comment_classifications = classify_top_level_author_comments( diff --git a/.github/scripts/pull-request-dashboard/eval/score_reviewer_feedback.py b/.github/scripts/pull-request-dashboard/eval/score_reviewer_feedback.py index 4227758f488..97fd9d4fc99 100644 --- a/.github/scripts/pull-request-dashboard/eval/score_reviewer_feedback.py +++ b/.github/scripts/pull-request-dashboard/eval/score_reviewer_feedback.py @@ -39,12 +39,7 @@ "reviewer_feedback": ( "REVIEWER_FEEDBACK_PROMPT_TEMPLATE", ("verdict",), - {"substantive": "substantive", "noise": "noise"}, - ), - "reviewer_feedback_confirm": ( - "REVIEWER_FEEDBACK_CONFIRM_PROMPT_TEMPLATE", - ("verdict",), - {"other": "substantive", "confirmed": "noise"}, + {"author_action": "substantive", "no_author_action": "noise"}, ), } diff --git a/.github/scripts/pull-request-dashboard/test_top_level_actions.py b/.github/scripts/pull-request-dashboard/test_top_level_actions.py index e4dd5494a68..5dc8ef5d9f4 100644 --- a/.github/scripts/pull-request-dashboard/test_top_level_actions.py +++ b/.github/scripts/pull-request-dashboard/test_top_level_actions.py @@ -22,6 +22,7 @@ ) from classification import ( PRAISE_VERDICTS, + REVIEWER_FEEDBACK_VERDICTS, classify_discussion_domains, classify_review_threads, parse_discussion_decision, @@ -67,6 +68,15 @@ def review_thread_discussion(discussion_id: str) -> dict: } +def verdict_record(discussion_id: str, verdict: str = "author_action") -> dict: + return { + "discussion_id": discussion_id, + "discussion_kind": "top-level-feedback", + "failed": False, + "decision": {"verdict": verdict, "reason": "action requested"}, + } + + def classification(discussion_id: str) -> dict: return { "discussion_id": discussion_id, @@ -999,27 +1009,20 @@ def test_author_comment_batch_rejects_cross_discussion_feedback_key( @patch("classification.save_classification_cache") @patch("classification.load_classification_cache", return_value={}) - @patch("classification.run_llm_for_top_level_reviewer_feedback_batch") @patch("classification.run_llm_for_verdict_batch") def test_a_thread_the_author_has_not_answered_needs_no_model( self, - run_inline, run_batch, _load_cache, save_cache, ) -> None: - run_batch.side_effect = lambda discussions, _model: [ - { - "discussion_id": discussion["discussion_id"], - "discussion_kind": discussion["discussion_kind"], - "failed": False, - "decision": { - "discussion_action": "author", - "reason": "Reviewer asked a question", - }, - } - for discussion in discussions - ] + asked = [] + + def batch(items, _model, _prompt, verdicts): + asked.append(verdicts) + return [verdict_record(item["discussion_id"]) for item in items] + + run_batch.side_effect = batch thread = review_thread_discussion("inline") thread["discussion_facts"] = {"latest_comment_role": "reviewer"} # a real reviewer comment, too long to be a praise candidate, so no binary runs @@ -1037,7 +1040,7 @@ def test_a_thread_the_author_has_not_answered_needs_no_model( classify_feedback_domains(123, [thread], [top_level_item("top-level")], "model") ) - run_inline.assert_not_called() + self.assertEqual([REVIEWER_FEEDBACK_VERDICTS], asked) self.assertEqual( review_thread_classifications[0]["decision"]["discussion_action"], "author" ) @@ -1045,7 +1048,6 @@ def test_a_thread_the_author_has_not_answered_needs_no_model( [record["discussion_id"] for record in top_level_classifications], ["top-level"], ) - @patch("classification.save_classification_cache") @patch("classification.load_classification_cache", return_value={}) @patch("classification.run_llm_for_top_level_reviewer_feedback_batch", return_value=[]) @@ -1247,7 +1249,7 @@ def test_author_reply_expanded_prompt_calls_are_bounded( @patch("classification.save_classification_cache") @patch("classification.load_classification_cache", return_value={}) - @patch("classification.run_llm_for_top_level_reviewer_feedback_batch") + @patch("classification.run_llm_for_verdict_batch") def test_later_run_classifies_only_failed_top_level_item( self, run_batch, @@ -1257,15 +1259,12 @@ def test_later_run_classifies_only_failed_top_level_item( valid = top_level_item("valid") missing = top_level_item("missing") run_batch.return_value = [ - classification("valid"), + verdict_record("valid"), { "discussion_id": "missing", "discussion_kind": "top-level-feedback", "failed": True, - "decision": { - "discussion_action": "unclear", - "reason": "Missing result", - }, + "decision": {"verdict": "author_action", "reason": "Missing result"}, }, ] @@ -1275,7 +1274,7 @@ def test_later_run_classifies_only_failed_top_level_item( self.assertEqual(len(cached), 1) load_cache.return_value = cached run_batch.reset_mock() - run_batch.return_value = [classification("missing")] + run_batch.return_value = [verdict_record("missing")] classify_feedback_domains(123, [], [valid, missing], "model") @@ -1286,16 +1285,15 @@ def test_later_run_classifies_only_failed_top_level_item( @patch("classification.save_classification_cache") @patch("classification.load_classification_cache", return_value={}) - @patch("classification.run_llm_for_top_level_reviewer_feedback_batch") + @patch("classification.run_llm_for_verdict_batch") def test_top_level_cache_ignores_mutable_facts_but_includes_body( self, run_batch, load_cache, save_cache, ) -> None: - run_batch.side_effect = lambda discussions, _model: [ - classification(discussion["discussion_id"]) - for discussion in discussions + run_batch.side_effect = lambda items, _m, _p, _v: [ + verdict_record(item["discussion_id"]) for item in items ] discussion = top_level_item("top-level") discussion["comments"] = [{"body": "Could you clarify this?"}] @@ -1319,24 +1317,15 @@ def test_top_level_cache_ignores_mutable_facts_but_includes_body( @patch("classification.TOP_LEVEL_CLASSIFICATION_BATCH_SIZE", 10) @patch("classification.save_classification_cache") @patch("classification.load_classification_cache", return_value={}) - @patch("classification.run_llm_for_top_level_reviewer_feedback_batch") + @patch("classification.run_llm_for_verdict_batch") def test_uncached_top_level_classification_is_batched_and_bounded( self, run_batch, _load_cache, save_cache, ) -> None: - run_batch.side_effect = lambda discussions, _model: [ - { - "discussion_id": discussion["discussion_id"], - "discussion_kind": discussion["discussion_kind"], - "failed": False, - "decision": { - "discussion_action": "none", - "reason": "No action", - }, - } - for discussion in discussions + run_batch.side_effect = lambda items, _m, _p, _v: [ + verdict_record(item["discussion_id"], "no_author_action") for item in items ] discussions = [top_level_item(f"item-{index}") for index in range(23)] @@ -1349,7 +1338,7 @@ def test_uncached_top_level_classification_is_batched_and_bounded( self.assertEqual(len(classifications), 23) self.assertEqual( [record["decision"]["discussion_action"] for record in classifications], - ["none"] * 20 + ["unclear"] * 3, + ["none"] * 20 + ["author"] * 3, ) self.assertEqual( [bool(record.get("failed")) for record in classifications], @@ -1395,7 +1384,7 @@ def test_over_limit_changes_requested_fails_instead_of_guessing( self.assertEqual( classifications[0]["decision"], { - "discussion_action": "unclear", + "discussion_action": "author", "reason": "Exceeded per-PR classification limit", }, ) From 867c0c82e1d596d2c52197bfe2af83c626df4d52 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 07:00:02 -0700 Subject: [PATCH 2/3] Say that merge permission neither creates nor settles an author action --- .github/scripts/pull-request-dashboard/classification.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/classification.py b/.github/scripts/pull-request-dashboard/classification.py index 28aec6b316b..4f751b13c4a 100644 --- a/.github/scripts/pull-request-dashboard/classification.py +++ b/.github/scripts/pull-request-dashboard/classification.py @@ -195,8 +195,9 @@ to merge"), and stays no_author_action when it carries a suggestion the reviewer explicitly leaves for later ("we can clean this up post submission", "an opportunity to refactor after a point fix release", "left one small -maintainability comment"). An item that ends by telling the author they may -merge is always no_author_action. +maintainability comment"). Permission to merge is not itself a request and never +makes an item author_action, but it does not settle one either: an item that also +asks, questions, or objects is author_action despite ending that way. Compare every login and team mentioned in `body` against `pr_author`. An item asking a different person or team to review, decide, or weigh in is From 26bb5abab9d0ff73053488f6d67d13d110874221 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 07:07:59 -0700 Subject: [PATCH 3/3] Drop the merge-permission rule the approval line already covers --- .github/scripts/pull-request-dashboard/classification.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/classification.py b/.github/scripts/pull-request-dashboard/classification.py index 4f751b13c4a..c8c4f455273 100644 --- a/.github/scripts/pull-request-dashboard/classification.py +++ b/.github/scripts/pull-request-dashboard/classification.py @@ -195,9 +195,7 @@ to merge"), and stays no_author_action when it carries a suggestion the reviewer explicitly leaves for later ("we can clean this up post submission", "an opportunity to refactor after a point fix release", "left one small -maintainability comment"). Permission to merge is not itself a request and never -makes an item author_action, but it does not settle one either: an item that also -asks, questions, or objects is author_action despite ending that way. +maintainability comment"). Compare every login and team mentioned in `body` against `pr_author`. An item asking a different person or team to review, decide, or weigh in is