From 61bd3d59fa49d4d2adcff7a4a26c60d4212f803a Mon Sep 17 00:00:00 2001 From: Chi-Hsuan Huang Date: Fri, 22 May 2026 14:01:42 +0800 Subject: [PATCH] fix(pr-review): post auto-reply outside the rework-only path Summary: - Require posting `github_reply_to_review_comment` in WORKFLOW.md when addressing review comments in code, not just for pushback. - Lift the `status == "rework_requested"` gate in `complete_reviewer_comment_record/2`: any pr_review record with `pending_reviewer_comments` now completes (auto-replies if enabled and advances the addressed cursor). - Update the existing "ignores pending comments unless waiting for rework" test to assert the new contract and add a focused test that auto-reply fires for non-rework statuses with pending comments. Rationale: - Copilot review comments that arrive during the initial agent run (before the issue reaches `In Review`) were addressed in code but never replied to, because the poller had no rework cycle to drive the auto-reply and WORKFLOW.md only required a reply for pushback. - The agent-side change catches the same-run-as-PR-open case; the poller change is defense-in-depth for cooling-down/deferred/non- rework records that still carry pending comments. Tests: - mix test test/symphony_elixir/pr_review_poller_test.exs - mix test test/symphony_elixir/core_test.exs - mix specs.check - mix format --check-formatted --- WORKFLOW.md | 5 ++- lib/symphony_elixir/pr_review_poller.ex | 20 ++++++++-- .../symphony_elixir/pr_review_poller_test.exs | 40 ++++++++++++++++--- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/WORKFLOW.md b/WORKFLOW.md index 055bbc0354..25b9d59878 100644 --- a/WORKFLOW.md +++ b/WORKFLOW.md @@ -270,8 +270,9 @@ When a ticket has an attached PR, run this protocol before moving to `In Review` - Inline review comments via `github_list_pr_review_comments()`. - Review summaries and states via `github_list_pr_reviews()`. 3. Treat every actionable reviewer comment (human or bot), including inline review comments, as blocking until one of these is true: - - code/test/docs updated to address it, or - - explicit, justified pushback reply is posted on that thread. + - code/test/docs updated to address it AND a short reply is posted on that thread via `github_reply_to_review_comment(comment_id, body)` confirming what changed (link the addressing commit when useful), or + - explicit, justified pushback reply is posted on that thread via `github_reply_to_review_comment(comment_id, body)`. + Replies are mandatory even when the comment is addressed in code, including during the first PR feedback sweep before moving to `In Review`. Top-level PR comments use `github_add_pr_comment` instead. 4. Update the workpad plan/checklist to include each feedback item and its resolution status. 5. Re-run validation after feedback-driven changes and push updates. 6. Repeat this sweep until there are no outstanding actionable comments. diff --git a/lib/symphony_elixir/pr_review_poller.ex b/lib/symphony_elixir/pr_review_poller.ex index 99953b9e8d..c9786c12dd 100644 --- a/lib/symphony_elixir/pr_review_poller.ex +++ b/lib/symphony_elixir/pr_review_poller.ex @@ -174,10 +174,22 @@ defmodule SymphonyElixir.PrReviewPoller do end defp complete_reviewer_comment_record(record, opts) do - if Map.get(record, :status) == "rework_requested" do - do_complete_reviewer_comment_record(record, opts) - else - :ok + cond do + Map.get(record, :status) == "rework_requested" -> + do_complete_reviewer_comment_record(record, opts) + + has_pending_reviewer_comments?(record) -> + do_complete_reviewer_comment_record(record, opts) + + true -> + :ok + end + end + + defp has_pending_reviewer_comments?(record) do + case Map.get(record, :pending_reviewer_comments) do + [_ | _] -> true + _ -> false end end diff --git a/test/symphony_elixir/pr_review_poller_test.exs b/test/symphony_elixir/pr_review_poller_test.exs index da7ec912ee..e56e2e7525 100644 --- a/test/symphony_elixir/pr_review_poller_test.exs +++ b/test/symphony_elixir/pr_review_poller_test.exs @@ -1442,6 +1442,35 @@ defmodule SymphonyElixir.PrReviewPollerTest do assert [%{last_addressed_comment_id: "comment-2", pending_reviewer_comments: []}] = RunStore.list_pr_reviews() end + test "auto reply fires for non-rework statuses when pending comments are present" do + now = ~U[2026-05-01 09:00:00Z] + + write_workflow_file!(Workflow.workflow_file_path(), + tracker_kind: "memory", + pr_review_mode: "polling", + pr_review_cooldown_minutes: 30, + pr_review_stale_days: 7, + pr_review_ignored_users: ["agent-user"], + pr_review_auto_reply: true + ) + + :ok = + put_review(now, %{ + status: "cooling_down", + pending_last_addressed_comment_id: "comment-1", + pending_reviewer_comments: [ + %{id: "comment-1", kind: "inline_comment", author: "human-reviewer", body: "Please split this.", path: "lib/example.ex", line: 42} + ] + }) + + assert :ok = PrReviewPoller.complete_pending_reviewer_comments("issue-1780", github: ActionGitHub, now: now) + + assert_receive {:github_reply, "https://github.com/example/repo/pull/1780", %{id: "comment-1"}, reply_body} + assert reply_body =~ "addressed" + + assert [%{last_addressed_comment_id: "comment-1", pending_reviewer_comments: []}] = RunStore.list_pr_reviews() + end + test "auto reply does not duplicate successful replies when request review fails after cursor advancement" do now = ~U[2026-05-01 09:00:00Z] @@ -1616,7 +1645,7 @@ defmodule SymphonyElixir.PrReviewPollerTest do assert stored_error =~ "comment-1" end - test "completion ignores pending comments unless the review record is waiting for rework" do + test "completion advances pending comments for any status when auto reply is disabled" do now = ~U[2026-05-01 09:00:00Z] :ok = @@ -1636,12 +1665,11 @@ defmodule SymphonyElixir.PrReviewPollerTest do assert [ %{ status: "watching", - pending_last_addressed_comment_id: "comment-1", - pending_reviewer_comments: [%{id: "comment-1"}] - } = record + last_addressed_comment_id: "comment-1", + pending_last_addressed_comment_id: nil, + pending_reviewer_comments: [] + } ] = RunStore.list_pr_reviews() - - refute Map.has_key?(record, :last_addressed_comment_id) end test "polling clears stale pending comments after the addressed cursor catches up" do