From a8dd1188e779a154f706d4bf46994951536f76a3 Mon Sep 17 00:00:00 2001 From: Chi-Hsuan Huang Date: Mon, 29 Jun 2026 15:53:15 +0800 Subject: [PATCH 1/2] feat(pr-review): let rework agent skip non-actionable bot comments via skip file --- SPEC.md | 4 + lib/symphony_elixir/pr_review_poller.ex | 65 ++++++++++++++++ lib/symphony_elixir/prompt_builder.ex | 7 +- lib/symphony_elixir/workspace.ex | 47 ++++++++++++ test/symphony_elixir/core_test.exs | 2 +- .../symphony_elixir/pr_review_poller_test.exs | 75 +++++++++++++++++++ test/symphony_elixir/workspace_test.exs | 24 ++++++ 7 files changed, 219 insertions(+), 5 deletions(-) diff --git a/SPEC.md b/SPEC.md index 7015f737dd..cad41cdcbf 100644 --- a/SPEC.md +++ b/SPEC.md @@ -681,6 +681,10 @@ Fields: own identity here. - `review_comments.reply_after_addressing` (boolean) - Polling-mode default: `false`. + - When enabled, the rework agent may mark clearly non-actionable bot comments (author + ending in `[bot]`) as skip by writing their ids to `.symphony-skip-comments.json` at + the workspace root; the poller suppresses the auto-reply for those ids (bot authors + only) and removes the file. The file is git-excluded so it is never committed. - `review_comments.request_review_after_push` (boolean) - Polling-mode default: `false`. - `checks.enabled` (boolean) diff --git a/lib/symphony_elixir/pr_review_poller.ex b/lib/symphony_elixir/pr_review_poller.ex index 5753ba1c55..9ab9321cbe 100644 --- a/lib/symphony_elixir/pr_review_poller.ex +++ b/lib/symphony_elixir/pr_review_poller.ex @@ -1931,12 +1931,16 @@ defmodule SymphonyElixir.PrReviewPoller do end end + @skip_comments_filename ".symphony-skip-comments.json" + defp reply_to_comments(record, comments, github, opts, now) do record = put_addressed_commit_sha(record) + skip_ids = consume_skip_comment_ids(record) {inline_comments, pr_level_comments} = comments |> reject_replied_comments(record) + |> reject_skipped_comments(skip_ids, record) |> Enum.split_with(&inline_comment?/1) case reply_to_inline_comments(record, inline_comments, github, opts, now) do @@ -1950,6 +1954,67 @@ defmodule SymphonyElixir.PrReviewPoller do Enum.reject(comments, &(Map.get(&1, :id) in replied_ids)) end + # The rework agent can mark clearly non-actionable bot comments in a workspace + # skip file so they don't receive the auto-reply note. We only honor the skip for + # bot-authored comments (the agent can never silence a human reviewer this way), + # and the file is consumed per run so a reused worktree carries no stale entries. + defp reject_skipped_comments(comments, skip_ids, record) do + {skipped, kept} = + Enum.split_with(comments, fn comment -> + MapSet.member?(skip_ids, Map.get(comment, :id)) and bot_author?(comment) + end) + + log_skipped_comments(skipped, record) + kept + end + + defp bot_author?(comment) do + author = Map.get(comment, :author) + is_binary(author) and String.ends_with?(author, "[bot]") + end + + # Reads the agent-written skip list (local workspaces only) and deletes the file. + # Any missing/unreadable/malformed file yields an empty set so behavior degrades + # to "reply as usual" rather than silently dropping replies. + defp consume_skip_comment_ids(record) do + case skip_comments_path(record) do + nil -> + MapSet.new() + + path -> + ids = read_skip_comment_ids(path) + _ = File.rm(path) + MapSet.new(ids) + end + end + + defp skip_comments_path(record) do + case Map.get(record, :workspace_path) do + path when is_binary(path) and path != "" -> Path.join(path, @skip_comments_filename) + _ -> nil + end + end + + defp read_skip_comment_ids(path) do + with {:ok, body} <- File.read(path), + {:ok, %{"skip_comment_ids" => ids}} when is_list(ids) <- Jason.decode(body) do + Enum.filter(ids, &is_binary/1) + else + _ -> [] + end + end + + defp log_skipped_comments([], _record), do: :ok + + defp log_skipped_comments(skipped, record) do + ids = Enum.map_join(skipped, ",", &to_string(Map.get(&1, :id))) + + Logger.info( + "PR review auto-reply skipped non-actionable bot comments " <> + "issue_id=#{Map.get(record, :issue_id)} count=#{length(skipped)} ids=#{ids}" + ) + end + defp inline_comment?(%{kind: "inline_comment"}), do: true defp inline_comment?(_comment), do: false diff --git a/lib/symphony_elixir/prompt_builder.ex b/lib/symphony_elixir/prompt_builder.ex index b16584cd69..a6c99dc700 100644 --- a/lib/symphony_elixir/prompt_builder.ex +++ b/lib/symphony_elixir/prompt_builder.ex @@ -672,14 +672,13 @@ defmodule SymphonyElixir.PromptBuilder do a) Commit a fix that resolves the comment, reference the comment id in the commit message body, then reply with the commit hash and concrete change. b) Reply with explicit pushback explaining why no code change is being made. c) Reply deferring the change with concrete reasoning (e.g., out of scope for this PR plus a follow-up reference). - d) Reply that the comment is non-actionable only when it is a genuine duplicate of another comment, or a purely informational note (including an automated summary that slipped past `ignored_reviewers`) that requests no change. + d) If — and only if — the comment is from an automated bot (its author ends with `[bot]`) AND is clearly non-actionable (a generated summary, duplicate, or informational note that requests no change), do NOT reply. Instead add its id to the `skip_comment_ids` list in the JSON file `.symphony-skip-comments.json` at the repository root (create it if absent), shaped as `{"skip_comment_ids": ["", "..."]}`. Do not stage or commit this file — Symphony reads and removes it. Never use this path for a human reviewer; choose a, b, or c instead. Make each reply read as an automated Symphony AI response. Prefer specific wording like: - "Symphony AI handled this in ``: removed the duplicate fallback while keeping the lookup order unchanged." - "Symphony AI is leaving this unchanged because ``." - "Symphony AI is deferring this because ``; follow-up: ``." - - "Symphony AI is treating this as informational and made no change because ``." - Never paste internal comment ids (for example `PRR_...` node ids or raw numeric ids) into reply text posted to GitHub; readers cannot interpret them. Refer to the comment naturally (for example "this review" or "this comment"). Comment ids belong only in commit message bodies. - Do not silently skip a comment, and do not rely on Symphony's generic auto-reply fallback as the primary response. The review agent verifies each comment id has either an associated commit or an outbound reply; that check is internal and does not require quoting the id in the reply text. + Never paste internal comment ids (for example `PRR_...` node ids or raw numeric ids) into reply text posted to GitHub; readers cannot interpret them. Refer to the comment naturally (for example "this review" or "this comment"). Comment ids belong only in commit message bodies and the skip file. + Every comment must end in exactly one of: an associated commit, an outbound reply, or an entry in the skip file. Do not silently skip a comment by doing none of these, and do not rely on Symphony's generic auto-reply fallback as the primary response. """ end diff --git a/lib/symphony_elixir/workspace.ex b/lib/symphony_elixir/workspace.ex index 4e745ec15b..6b40300dc5 100644 --- a/lib/symphony_elixir/workspace.ex +++ b/lib/symphony_elixir/workspace.ex @@ -163,6 +163,10 @@ defmodule SymphonyElixir.Workspace do end end + # File the rework agent writes to mark clearly non-actionable bot review comments + # so they are skipped by the auto-reply. Excluded from git so it is never tracked. + @skip_comments_filename ".symphony-skip-comments.json" + defp ensure_worktree_workspace(workspace, issue_context, nil, settings) do with {:ok, repo} <- local_worktree_repo(settings), :ok <- maybe_fetch_worktree_repo(repo, settings), @@ -171,6 +175,7 @@ defmodule SymphonyElixir.Workspace do create_base_ref = worktree_create_base_ref(repo, issue_context, base_ref), {:ok, created?} <- add_or_reuse_local_worktree(repo, workspace, branch, base_ref, create_base_ref) do + ensure_skip_comments_excluded(workspace) {:ok, workspace, created?} else {:error, reason, output} -> @@ -310,6 +315,48 @@ defmodule SymphonyElixir.Workspace do run_git(workspace, ["reset", "--hard", base_ref]) end + # Adds the skip-comments file to the worktree's git exclude so the agent can write + # it without it ever being tracked or accidentally committed. Best-effort: any + # failure here must not block workspace creation. + defp ensure_skip_comments_excluded(workspace) do + with {:ok, common_dir} <- git_output(workspace, ["rev-parse", "--git-common-dir"]) do + exclude_path = + common_dir + |> IO.iodata_to_binary() + |> String.trim() + |> resolve_workspace_relative(workspace) + |> Path.join("info/exclude") + + ensure_exclude_entry(exclude_path, "/#{@skip_comments_filename}") + end + + :ok + end + + defp resolve_workspace_relative(path, workspace) do + case Path.type(path) do + :absolute -> path + _ -> Path.join(workspace, path) + end + end + + defp ensure_exclude_entry(exclude_path, entry) do + existing = + case File.read(exclude_path) do + {:ok, contents} -> contents + _ -> "" + end + + if entry in String.split(existing, "\n") do + :ok + else + _ = File.mkdir_p(Path.dirname(exclude_path)) + prefix = if String.trim_trailing(existing, "\n") == "", do: "", else: String.trim_trailing(existing, "\n") <> "\n" + _ = File.write(exclude_path, prefix <> entry <> "\n") + :ok + end + end + defp add_local_worktree(repo, workspace, branch, base_ref) do File.mkdir_p!(Path.dirname(workspace)) diff --git a/test/symphony_elixir/core_test.exs b/test/symphony_elixir/core_test.exs index 1721f0792b..fde7ef4840 100644 --- a/test/symphony_elixir/core_test.exs +++ b/test/symphony_elixir/core_test.exs @@ -2215,7 +2215,7 @@ defmodule SymphonyElixir.CoreTest do assert prompt =~ "For each comment below, do EXACTLY ONE of the following" assert prompt =~ "Make each reply read as an automated Symphony AI response." assert prompt =~ "Symphony AI handled this in ``" - assert prompt =~ "Symphony AI is treating this as informational and made no change" + assert prompt =~ "add its id to the `skip_comment_ids` list in the JSON file `.symphony-skip-comments.json`" assert prompt =~ "Never paste internal comment ids" assert prompt =~ "[id=comment-1] Reviewer: [inline_comment] lib/example.ex:42" assert prompt =~ "\nPlease split this function.\n" diff --git a/test/symphony_elixir/pr_review_poller_test.exs b/test/symphony_elixir/pr_review_poller_test.exs index 60ccee8d31..fce51b377f 100644 --- a/test/symphony_elixir/pr_review_poller_test.exs +++ b/test/symphony_elixir/pr_review_poller_test.exs @@ -1589,6 +1589,81 @@ defmodule SymphonyElixir.PrReviewPollerTest do assert [%{last_addressed_comment_id: "comment-2", pending_reviewer_comments: []}] = RunStore.list_pr_reviews() end + test "auto reply skips bot comment ids the agent recorded in the workspace skip file" do + now = ~U[2026-05-01 09:00:00Z] + {workspace_path, reviewed_sha} = git_workspace_with_commit!() + _commit_sha = add_git_commit!(workspace_path, "fix: address review") + + skip_file = Path.join(workspace_path, ".symphony-skip-comments.json") + File.write!(skip_file, Jason.encode!(%{"skip_comment_ids" => ["bot-noise"]})) + + 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_auto_reply: true + ) + + :ok = + put_review(now, %{ + status: "rework_requested", + workspace_path: workspace_path, + reviewed_commit_sha: reviewed_sha, + pending_last_addressed_comment_id: "bot-noise", + pending_reviewer_comments: [ + %{id: "comment-1", kind: "inline_comment", author: "human-reviewer", body: "Please split this.", path: "lib/example.ex", line: 42}, + %{id: "bot-noise", kind: "comment", author: "coderabbitai[bot]", body: "Summary of changes."} + ] + }) + + assert :ok = PrReviewPoller.complete_pending_reviewer_comments("issue-1780", github: ActionGitHub, now: now) + + # The human comment is still replied to. + assert_receive {:github_reply, "https://github.com/example/repo/pull/1780", %{id: "comment-1"}, _body} + # The skipped bot comment gets no reply (neither inline nor summary). + refute_receive {:github_reply, _, %{id: "bot-noise"}, _} + refute_receive {:github_reply, _, %{id: "pr-review-summary"}, _} + + # The skip file is consumed (deleted) after the run. + refute File.exists?(skip_file) + end + + test "auto reply ignores skip entries for human (non-bot) comments" do + now = ~U[2026-05-01 09:00:00Z] + {workspace_path, reviewed_sha} = git_workspace_with_commit!() + _commit_sha = add_git_commit!(workspace_path, "fix: address review") + + File.write!( + Path.join(workspace_path, ".symphony-skip-comments.json"), + Jason.encode!(%{"skip_comment_ids" => ["comment-1"]}) + ) + + 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_auto_reply: true + ) + + :ok = + put_review(now, %{ + status: "rework_requested", + workspace_path: workspace_path, + reviewed_commit_sha: reviewed_sha, + 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) + + # A human comment is replied to even if the agent (wrongly) listed it to skip. + assert_receive {:github_reply, "https://github.com/example/repo/pull/1780", %{id: "comment-1"}, _body} + end + test "auto reply uses a recorded follow-up sha that differs from the reviewed sha" do now = ~U[2026-05-01 09:00:00Z] diff --git a/test/symphony_elixir/workspace_test.exs b/test/symphony_elixir/workspace_test.exs index acf1b0341b..4be2c61e8e 100644 --- a/test/symphony_elixir/workspace_test.exs +++ b/test/symphony_elixir/workspace_test.exs @@ -222,6 +222,30 @@ defmodule SymphonyElixir.WorkspaceTest do end end + test "worktree excludes the agent skip-comments file from version control" do + test_root = unique_tmp("workspace-skip-exclude") + primary_repo = Path.join(test_root, "primary") + workspace_root = Path.join(test_root, "workspaces") + + try do + create_primary_repo!(primary_repo) + + write_workflow_file!(Workflow.workflow_file_path(), + workspace_root: workspace_root, + workspace_strategy: "worktree", + workspace_repo: primary_repo, + workspace_fetch_before_dispatch: false + ) + + assert {:ok, workspace} = Workspace.create_for_issue("RSM-SKIP") + + assert {_output, 0} = + Workspace.safe_git(["-C", workspace, "check-ignore", ".symphony-skip-comments.json"]) + after + File.rm_rf(test_root) + end + end + defp unique_tmp(name) do Path.join(System.tmp_dir!(), "symphony-elixir-#{name}-#{System.unique_integer([:positive])}") end From 2eda96e4ce2d435a7d45989970c3b012b56de37a Mon Sep 17 00:00:00 2001 From: Chi-Hsuan Huang Date: Mon, 29 Jun 2026 17:01:17 +0800 Subject: [PATCH 2/2] fix(pr-review): use in/2 for skip-id membership to satisfy dialyzer opacity --- lib/symphony_elixir/pr_review_poller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/symphony_elixir/pr_review_poller.ex b/lib/symphony_elixir/pr_review_poller.ex index 9ab9321cbe..a6c7f7ba80 100644 --- a/lib/symphony_elixir/pr_review_poller.ex +++ b/lib/symphony_elixir/pr_review_poller.ex @@ -1961,7 +1961,7 @@ defmodule SymphonyElixir.PrReviewPoller do defp reject_skipped_comments(comments, skip_ids, record) do {skipped, kept} = Enum.split_with(comments, fn comment -> - MapSet.member?(skip_ids, Map.get(comment, :id)) and bot_author?(comment) + Map.get(comment, :id) in skip_ids and bot_author?(comment) end) log_skipped_comments(skipped, record)