diff --git a/backend/app/gateway/github/dispatcher.py b/backend/app/gateway/github/dispatcher.py index 98f09e919..15cf45b1a 100644 --- a/backend/app/gateway/github/dispatcher.py +++ b/backend/app/gateway/github/dispatcher.py @@ -7,8 +7,9 @@ first-class :class:`Channel` (see ``app/channels/github.py``): POST /api/webhooks/github → verify HMAC (route) → :func:`fanout_event` (this module) - • filter bots • look up bound agents + • filter bots + • drop redundant review-comment webhook noise, per binding • apply per-binding trigger filter • publish one :class:`InboundMessage` per surviving agent → ChannelManager picks it up off the bus @@ -32,7 +33,7 @@ from app.gateway.github.identity import extract_target, resolve_thread_id from app.gateway.github.prompts import build_prompt from app.gateway.github.registry import build_github_agent_registry, lookup_agents from app.gateway.github.triggers import event_should_fire -from deerflow.config.agents_config import GitHubAgentConfig +from deerflow.config.agents_config import GitHubAgentConfig, GitHubTriggerConfig logger = logging.getLogger(__name__) @@ -100,6 +101,90 @@ def _is_self_event( return sender_login.lower() in {s.lower() for s in self_logins} +def _is_redundant_review_comment(payload: dict[str, Any]) -> bool: + """Return True if this ``pull_request_review_comment`` has the *shape* + of fan-out noise from a ``pull_request_review`` submission: a companion + inline comment that the review event already covers. + + GitHub fires one ``pull_request_review_comment`` webhook per inline + comment attached to a review submission, ON TOP OF the single + ``pull_request_review`` event for the review as a whole. A bot + reviewer (CodeRabbit routinely posts 20-30 inline comments per review) + therefore floods the webhook with 20-30 near-duplicate deliveries. + + Each such companion comment carries ``pull_request_review_id`` (the id + of the review it belongs to) and — the discriminator — no + ``in_reply_to_id``. ``in_reply_to_id`` is only set when a human (or + bot) is replying *within* an existing review-comment thread, which is + a genuine new interaction, not fan-out, and must still fire. + + This mirrors the shape GitHub's REST API has always used for + review-thread comments (``GET /repos/{owner}/{repo}/pulls/comments``), + which the webhook ``comment`` object is drawn from: + ``pull_request_review_id`` is present on every review-thread comment; + ``in_reply_to_id`` is present only on replies. + + IMPORTANT: a ``True`` result is NOT by itself a "safe to drop" signal — + see the per-binding gate in :func:`fanout_event`, which only suppresses + a companion comment for a binding that *also* has its own + ``pull_request_review`` trigger on the same repo AND whose *resolved* + trigger does not itself require a mention (see the ``require_mention`` + gap note below) — i.e. an unconditional, independent path to the + review. A binding that subscribes to ``pull_request_review_comment`` + alone never receives the parent review event, so unconditionally + dropping its companion comments would be a silent, total loss of the + review's inline content for it — not noise reduction (PR #4131 review + feedback from willem-bd / zhfeng). + + ``require_mention`` gap (PR #4131 review, Medium finding, willem-bd — + second round, against the per-binding gate above): a dual-subscribed + binding's ``pull_request_review`` trigger is only a *guaranteed* + independent path when that trigger does not itself gate on + ``require_mention``. If it does, the paired review event can be + silently dropped by :func:`app.gateway.github.triggers.event_should_fire`'s + own mention check against ``review["body"]`` — the review's + *top-level* summary, which this ``pull_request_review_comment`` + payload never carries (there is no way to see, from a comment + delivery, what the sibling review's own summary said). A human + ``@mention`` that lives only inside one inline comment — not the + review summary — would otherwise be lost twice over: the review event + is filtered out (``no_mention``) *and* the one inline comment that + actually carries the mention is dropped here as "redundant", via a + narrower path than the original bug. The per-binding gate therefore + additionally requires ``require_mention`` to be false on the paired + trigger before treating it as coverage. This trades a small amount of + residual redundancy (an extra companion delivery on occasions when the + review's own summary happened to carry the mention too, or + ``allow_authors``/self-event would have let the review through anyway) + for zero silent loss — the same trade the original fix already made at + a coarser grain. It deliberately does not attempt to replay + ``allow_authors`` or an ``actions`` whitelist that might also be + configured on the paired trigger; those are accepted as out of scope + for this narrower fix, same as ``self_event``. + + Residual caveat (PR #4131 review, Concern 3, zhfeng): GitHub documents + ``pull_request_review_id`` on the review-comment schema as nullable + ("integer or null"), confirming *some* review comments can lack a + backing review, but public docs do not state whether the "Add single + comment" UI action (as opposed to a multi-comment review) can ever + produce a comment with ``pull_request_review_id`` set and + ``in_reply_to_id`` absent *without* a companion ``pull_request_review`` + event also firing. If that combination is possible, a binding with its + own ``pull_request_review`` trigger could still lose such a comment + under the per-binding gate. Confirmed via a real/documented webhook + payload capture before ruling this out; treat it as an open, + low-probability risk rather than a settled non-issue. (Distinct from + the ``require_mention`` gap above: this caveat questions whether the + paired event fires *at all* for a given delivery shape; the + ``require_mention`` gap is about a paired event that fires but is then + filtered by its own trigger config, which the gate now accounts for.) + """ + comment = payload.get("comment") + if not isinstance(comment, dict): + return False + return comment.get("pull_request_review_id") is not None and comment.get("in_reply_to_id") is None + + async def fanout_event( bus: MessageBus, event: str, @@ -157,6 +242,31 @@ async def fanout_event( sender_login = (payload.get("sender") or {}).get("login") + # 3. Redundant review-comment fan-out filter — see + # :func:`_is_redundant_review_comment`. Whether the PAYLOAD has the + # shape of review fan-out is a property of the event and computed + # once here, but whether that fan-out is safe to DROP is a property + # of the individual binding: only a binding that also has its own + # ``pull_request_review`` trigger on this repo, WITHOUT that trigger + # itself requiring a mention, has a *guaranteed* independent path to + # the review's content — so the actual suppression decision is made + # per-agent below (next to the self-event gate), not here. + # ``review_trigger_by_binding`` reuses :func:`lookup_agents` against + # the registry we just built — a binding only appears in the + # ``(repo, "pull_request_review")`` slot if it explicitly lists that + # event under its own ``triggers:`` (opt-in per binding, see + # ``triggers.py``) — so this does not duplicate any trigger-matching + # logic, and it stays correctly scoped to this repo (an agent with a + # second binding on a *different* repo does not count). It maps to + # the binding's *resolved* :class:`GitHubTriggerConfig` (not just + # membership) so the per-agent gate below can also check + # ``require_mention`` — see the ``require_mention`` gap note on + # :func:`_is_redundant_review_comment` for why a mention-gated review + # trigger cannot be trusted as coverage (PR #4131 review, Medium + # finding, willem-bd). + is_redundant_review_comment = event == "pull_request_review_comment" and _is_redundant_review_comment(payload) + review_trigger_by_binding: dict[tuple[str, str], GitHubTriggerConfig] = {(m.user_id, m.agent.name): m.trigger for m in lookup_agents(registry, repo, "pull_request_review")} if is_redundant_review_comment else {} + for match in matches: agent = match.agent # ``cfg.github`` is non-None on every match by construction — @@ -165,7 +275,7 @@ async def fanout_event( assert github is not None trigger = match.trigger - # 3. Self-event gate — skip events triggered by this agent's own + # 4. Self-event gate — skip events triggered by this agent's own # bot account. Other bots (Copilot, CodeRabbit, Dependabot, …) # are legitimate signals and pass through. The identity set is # derived from the agent's whole ``github`` config (bot_login @@ -180,10 +290,11 @@ async def fanout_event( skipped.append({"agent": agent.name, "reason": "self_event"}) continue - # 4. Trigger filter. - # ``default_mention_login`` mirrors the precedence used by - # ``_is_self_event`` above, then extended with the operator - # default from ``channels.github.default_mention_login``: + # 5. Trigger filter — computed once, up front, rather than right + # before its own gate further below. ``default_mention_login`` + # mirrors the precedence used by ``_is_self_event`` above, then + # extended with the operator default from + # ``channels.github.default_mention_login``: # # 1. ``trigger.mention_login`` — per-event override (handled # inside ``event_should_fire``; we only pass the fallback). @@ -209,9 +320,61 @@ async def fanout_event( # string. ``operator_default_mention_login`` is a plain function # argument (not a validated model field), so it is normalized here # explicitly. + # + # Computing this here (rather than at its old location right before + # its own gate) lets the redundant-review-comment gate just below + # also consult the verdict for a more precise skip reason, instead + # of always reporting ``redundant_review_comment`` even when this + # binding's own trigger would have skipped the event anyway for an + # unrelated reason (PR #4131 review, Minor finding, willem-bd). + # ``event_should_fire`` is a pure function of its arguments, so + # computing it once here and reusing it below is not a behavior + # change from calling it at the old step 6 location. operator_default = (operator_default_mention_login or "").strip() or None default_mention_login = github.bot_login or operator_default or agent.name fire, reason = event_should_fire(event, payload, trigger, default_mention_login) + + # 6. Redundant review-comment gate, per binding (PR #4131 review — + # willem-bd / zhfeng). Only suppress THIS binding's companion + # comment when it is also registered for ``pull_request_review`` + # on this repo AND that trigger's own ``require_mention`` is not + # set — i.e. it has an unconditional, independent path to the + # review content that makes the companion comment genuinely + # redundant *for it*. A binding registered for + # ``pull_request_review_comment`` alone never receives the + # parent review event at all, so it still fires here even though + # the payload has the fan-out shape. A binding whose review + # trigger DOES require a mention is also not treated as covered: + # this ``pull_request_review_comment`` payload never carries the + # review's own top-level body, so there is no way to verify from + # here whether that mention check would actually pass for the + # paired review event — see the ``require_mention`` gap note on + # :func:`_is_redundant_review_comment`. + # + # When this binding IS suppressed as redundant, the skip reason + # prefers this binding's own trigger verdict (``reason`` from + # step 5 above) over the generic ``redundant_review_comment`` + # label whenever that verdict is ALSO a skip — e.g. this + # companion's own ``pull_request_review_comment`` trigger + # separately requires a mention it doesn't have. The event is + # skipped either way; this only makes the logged reason more + # useful for operator debugging (PR #4131 review, Minor finding, + # willem-bd). + review_trigger = review_trigger_by_binding.get((match.user_id, agent.name)) + if is_redundant_review_comment and review_trigger is not None and not review_trigger.require_mention: + skip_reason = reason if not fire else "redundant_review_comment" + logger.info( + "github_fanout: agent=%s skipped (reason=%s, repo=%s#%s, delivery=%s)", + agent.name, + skip_reason, + repo, + number, + delivery_id, + ) + skipped.append({"agent": agent.name, "reason": skip_reason}) + continue + + # 7. Apply the trigger filter's verdict from step 5. if not fire: logger.info( "github_fanout: agent=%s skipped (reason=%s)", @@ -221,7 +384,7 @@ async def fanout_event( skipped.append({"agent": agent.name, "reason": reason}) continue - # 5. Build prompt + publish inbound message onto the bus. + # 8. Build prompt + publish inbound message onto the bus. prompt = build_prompt(event, payload) thread_id = resolve_thread_id(repo, number, agent.name) diff --git a/backend/app/gateway/github/prompts.py b/backend/app/gateway/github/prompts.py index f9c0d35f3..c029e646c 100644 --- a/backend/app/gateway/github/prompts.py +++ b/backend/app/gateway/github/prompts.py @@ -130,9 +130,26 @@ def _pr_review_prompt(payload: dict[str, Any]) -> str: number = pr.get("number") parent_block = _render_parent_context(pr, "pull request") review = payload.get("review") or {} + review_id = review.get("id") state = review.get("state") or "(unknown state)" author = (review.get("user") or {}).get("login") or "(unknown)" body = _truncate(review.get("body")) + # This payload carries only the review's own top-level summary. Any + # inline comments the reviewer left arrive as SEPARATE + # `pull_request_review_comment` webhook deliveries, and the dispatcher + # suppresses those as redundant fan-out for a binding that (like this + # one) also listens for `pull_request_review` on this repo — see + # `dispatcher.py::_is_redundant_review_comment`. That suppression is + # only genuinely redundant if the agent actually recovers the inline + # content from here, so tell it how (PR #4131 review, Concern 1, + # zhfeng): without this, the filter and the prompt were working against + # each other — the filter suppressed the only events that carried the + # inline content, and nothing ever told the agent to go get it. Omitted + # when `review.id` (or the PR number) is missing/malformed so the + # instruction never renders a broken `gh api` path. + fetch_hint = "" + if review_id is not None and number is not None: + fetch_hint = f"This review's inline comments are not included in this message. Before deciding what to do, fetch them with `gh api repos/{repo}/pulls/{number}/reviews/{review_id}/comments`.\n\n" return ( f"A pull request review was submitted on #{number} in {repo}.\n\n" f"{parent_block}\n" @@ -140,6 +157,7 @@ def _pr_review_prompt(payload: dict[str, Any]) -> str: f" Reviewer: {author}\n" f" State: {state}\n\n" f" Body:\n{body or '(no review body)'}\n\n" + f"{fetch_hint}" f"Decide what action (if any) to take in response to this review, in the context " f"of the parent pull request above. Your final assistant message is for the run " f"log only — it will NOT be posted to GitHub. If you want to reply (or push a fix), " diff --git a/backend/tests/test_github_dispatcher.py b/backend/tests/test_github_dispatcher.py index c2defa7ad..07d02fa0e 100644 --- a/backend/tests/test_github_dispatcher.py +++ b/backend/tests/test_github_dispatcher.py @@ -1034,3 +1034,527 @@ async def test_coder_and_reviewer_on_same_pr_get_distinct_threads(base_dir: Path # Each metadata mirrors its own thread id. assert coder.metadata["preferred_thread_id"] == coder.metadata["github"]["thread_id"] assert reviewer.metadata["preferred_thread_id"] == reviewer.metadata["github"]["thread_id"] + + +# --------------------------------------------------------------------------- +# Redundant review-comment fan-out suppression (issue #4121, narrower slice) +# +# GitHub fires one `pull_request_review_comment` webhook per inline comment +# attached to a review submission, IN ADDITION to the single +# `pull_request_review` event for the review as a whole. A bot reviewer like +# CodeRabbit routinely leaves 20-30 inline comments per review, so this +# floods the webhook with near-duplicate deliveries that carry nothing the +# agent doesn't already have -- it fetches every inline comment itself (via +# `gh api`) when it processes the parent `pull_request_review` event. Each +# such companion comment carries `pull_request_review_id` and (unless it is +# itself a reply within an existing thread) no `in_reply_to_id`. +# +# PR #4131 review (willem-bd, zhfeng -- "Request changes"): the first cut of +# this filter suppressed every such companion comment unconditionally, +# before the registry lookup / per-agent loop even ran. That is only safe +# for a binding that ALSO subscribes to `pull_request_review` on the same +# repo -- it has its own path to the review content, via +# `_pr_review_prompt`, so the companion comment is genuinely redundant for +# it. A binding that subscribes to `pull_request_review_comment` ALONE +# never receives the parent review event at all (events are opt-in per +# binding, see `triggers.py`), so the parent event was never a substitute +# for it in the first place -- suppressing the companion comment too would +# silently drop the entire review submission's inline content for that +# binding, with no recovery path. The filter is now applied per matched +# binding: it only suppresses when that SAME binding also has its own +# `pull_request_review` trigger configured for this repo. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_review_comment_companion_to_review_is_suppressed(base_dir: Path) -> None: + """A `pull_request_review_comment` that is pure fan-out from a parent + `pull_request_review` submission is suppressed for a binding that is + ALSO registered for `pull_request_review` on the same repo -- that + binding has its own path to the review content, so the companion + comment is genuinely redundant for it. This is the original bug fix's + value and must be preserved by the binding-scoped gate. + + ``pull_request_review_id`` is set (this comment belongs to a review) + and ``in_reply_to_id`` is absent (this is not a reply-to-a-reply), so + this is the classic "CodeRabbit storm" companion event. + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "reviewer", + { + "name": "reviewer", + "github": { + "bindings": [ + { + "repo": "a/b", + "triggers": { + # Dual-subscribed: this binding also listens for + # the parent review event, so it has its own + # path to the review's content. + "pull_request_review": {}, + "pull_request_review_comment": {"require_mention": False}, + }, + } + ], + }, + }, + ) + payload = { + "action": "created", + "pull_request": {"number": 7}, + "comment": { + "body": "nit: consider renaming this variable.", + "user": {"login": "coderabbitai[bot]"}, + "pull_request_review_id": 999001, + "in_reply_to_id": None, + }, + "repository": {"full_name": "a/b"}, + "sender": {"login": "coderabbitai[bot]"}, + } + result = await fanout_event(bus, "pull_request_review_comment", "del-storm-1", payload) + # The binding-scoped gate runs inside the per-agent loop (after the + # registry lookup), so the agent DOES show up as matched -- it just + # doesn't fire. This is more informative than the old global early + # return (which reported `matched_agents: []` even when an agent would + # have matched -- PR #4131 review, zhfeng's "Minor" observability note). + assert result["matched_agents"] == ["reviewer"], result + assert result["fired_agents"] == [], result + assert any(s["agent"] == "reviewer" and s["reason"] == "redundant_review_comment" for s in result["skipped"]), result + assert await _drain(bus) == [] + + +@pytest.mark.asyncio +async def test_review_comment_only_binding_not_suppressed(base_dir: Path) -> None: + """Regression fix (PR #4131 review, Concern 1 -- zhfeng, "Request + changes"): a binding registered for `pull_request_review_comment` + ALONE (no `pull_request_review` trigger) must still fire on a + companion comment, even though the payload has the exact same + fan-out shape as the previous test. + + Such a binding never receives the parent `pull_request_review` event + at all -- events are opt-in per binding (`triggers.py`) -- so the + companion comment is its ONLY delivery of this review's inline + content. Unconditionally suppressing it (the as-reviewed behavior) + would be a silent, total loss of the review's inline comments for + this binding, not noise reduction. This is the same operator config + zhfeng's review used as the concrete failure scenario: + `pull_request_review_comment: {require_mention: false}` with no + `pull_request_review` binding, receiving a CodeRabbit-style inline + comment. + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "reviewer", + { + "name": "reviewer", + "github": { + "bindings": [ + { + "repo": "a/b", + # Single-subscribed: NO `pull_request_review` trigger. + "triggers": {"pull_request_review_comment": {"require_mention": False}}, + } + ], + }, + }, + ) + payload = { + "action": "created", + "pull_request": {"number": 7}, + "comment": { + "body": "nit: consider renaming this variable.", + "user": {"login": "coderabbitai[bot]"}, + "pull_request_review_id": 999001, + "in_reply_to_id": None, + }, + "repository": {"full_name": "a/b"}, + "sender": {"login": "coderabbitai[bot]"}, + } + result = await fanout_event(bus, "pull_request_review_comment", "del-storm-2", payload) + assert result["matched_agents"] == ["reviewer"], result + assert result["fired_agents"] == ["reviewer"], result + assert result["skipped"] == [], result + messages = await _drain(bus) + assert len(messages) == 1 + assert messages[0].metadata["agent_name"] == "reviewer" + + +@pytest.mark.asyncio +async def test_review_comment_not_suppressed_when_review_trigger_is_on_different_repo(base_dir: Path) -> None: + """The binding-scoped gate must key off the trigger on THIS repo, not + merely "does this agent have a `pull_request_review` trigger anywhere". + + An agent can have multiple `github` bindings (one per repo). An agent + that listens for `pull_request_review` on repo X and + `pull_request_review_comment`-only on repo Y must still fire on Y's + companion comments -- the review coverage on X is irrelevant to Y. + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "multi-repo-bot", + { + "name": "multi-repo-bot", + "github": { + "bindings": [ + { + "repo": "owner/x", + "triggers": {"pull_request_review": {}}, + }, + { + "repo": "owner/y", + "triggers": {"pull_request_review_comment": {"require_mention": False}}, + }, + ], + }, + }, + ) + payload = { + "action": "created", + "pull_request": {"number": 3}, + "comment": { + "body": "nit on repo Y.", + "user": {"login": "coderabbitai[bot]"}, + "pull_request_review_id": 42, + "in_reply_to_id": None, + }, + "repository": {"full_name": "owner/y"}, + "sender": {"login": "coderabbitai[bot]"}, + } + result = await fanout_event(bus, "pull_request_review_comment", "del-storm-3", payload) + assert result["fired_agents"] == ["multi-repo-bot"], result + assert result["skipped"] == [], result + + +@pytest.mark.asyncio +async def test_review_comment_reply_within_thread_still_fires(base_dir: Path) -> None: + """A genuine reply within an existing review-comment thread is a + distinct interaction, not fan-out noise -- it must still fire even + though ``pull_request_review_id`` is set, because ``in_reply_to_id`` + marks it as a reply rather than a fresh review-companion comment. + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "reviewer", + { + "name": "reviewer", + "github": { + "bindings": [ + { + "repo": "a/b", + "triggers": {"pull_request_review_comment": {"require_mention": False}}, + } + ], + }, + }, + ) + payload = { + "action": "created", + "pull_request": {"number": 7}, + "comment": { + "body": "Good catch, fixed in the latest push.", + "user": {"login": "alice"}, + "pull_request_review_id": 999001, + "in_reply_to_id": 555002, + }, + "repository": {"full_name": "a/b"}, + "sender": {"login": "alice"}, + } + result = await fanout_event(bus, "pull_request_review_comment", "del-reply-1", payload) + assert result["fired_agents"] == ["reviewer"], result + assert result["skipped"] == [] + messages = await _drain(bus) + assert len(messages) == 1 + + +@pytest.mark.asyncio +async def test_review_comment_without_review_id_still_fires(base_dir: Path) -> None: + """A `pull_request_review_comment` with no `pull_request_review_id` at + all is not part of any review fan-out and must still fire. + + (Every real GitHub `pull_request_review_comment` carries this field, + but the filter must key off its actual presence rather than assume + it, so a malformed/legacy payload is never silently swallowed.) + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "reviewer", + { + "name": "reviewer", + "github": { + "bindings": [ + { + "repo": "a/b", + "triggers": {"pull_request_review_comment": {"require_mention": False}}, + } + ], + }, + }, + ) + payload = { + "action": "created", + "pull_request": {"number": 7}, + "comment": { + "body": "Standalone inline comment, no parent review.", + "user": {"login": "alice"}, + }, + "repository": {"full_name": "a/b"}, + "sender": {"login": "alice"}, + } + result = await fanout_event(bus, "pull_request_review_comment", "del-noreviewid-1", payload) + assert result["fired_agents"] == ["reviewer"], result + assert result["skipped"] == [] + + +@pytest.mark.asyncio +async def test_issue_comment_unaffected_by_review_comment_filter(base_dir: Path) -> None: + """A standalone top-level `issue_comment` must be completely unaffected + by the review-comment redundancy filter -- it is a different event + name entirely, so the filter must not touch it. + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "assistant", + { + "name": "assistant", + "github": { + "bindings": [ + { + "repo": "a/b", + "triggers": {"issue_comment": {"require_mention": False}}, + } + ], + }, + }, + ) + payload = { + "action": "created", + "issue": {"number": 3, "pull_request": {"url": "..."}}, + "comment": {"body": "just a regular comment", "user": {"login": "alice"}}, + "repository": {"full_name": "a/b"}, + "sender": {"login": "alice"}, + } + result = await fanout_event(bus, "issue_comment", "del-issue-1", payload) + assert result["fired_agents"] == ["assistant"], result + assert result["skipped"] == [] + + +# --------------------------------------------------------------------------- +# require_mention conditional-delivery gap (PR #4131 review, Medium + Minor +# findings, willem-bd -- second review round, against the per-binding gate +# above which already shipped in response to the FIRST round) +# +# The per-binding gate's premise is that a binding also registered for +# `pull_request_review` on this repo has an independent path to the review +# content, so its companion comments are genuinely redundant for it. That +# premise only holds if the review event is GUARANTEED to fire. If the +# binding's `pull_request_review` trigger itself has `require_mention: +# true`, the review event can be silently dropped by its own mention check +# against `review["body"]` (the review's top-level summary) -- a field +# this `pull_request_review_comment` payload never carries, so there is no +# way to verify from here whether that check would pass. A human +# `@mention` living only inside one inline comment (not the review +# summary) would then be lost twice over: the review is filtered +# (`no_mention`) *and* the one inline comment that actually carries the +# mention is dropped here as "redundant" -- the same silent-loss shape as +# the original bug, just via a narrower path. `dispatcher.py` now treats a +# mention-gated review trigger as NOT covering its companion comments. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_review_comment_not_suppressed_when_review_trigger_requires_mention(base_dir: Path) -> None: + """Reproduces willem-bd's exact Medium-finding scenario: a dual-subscribed + binding whose `pull_request_review` trigger has `require_mention: true`, + and a review whose summary would carry no mention typed only inside one + inline comment. + + Only the `pull_request_review_comment` half of the scenario can be + exercised at this layer -- the two events are separate webhook + deliveries -- but that is exactly where the bug lived: on the + as-reviewed code, this binding's mere registration for + `pull_request_review` was enough to mark it "covered" and suppress the + companion, regardless of that trigger's own `require_mention`. Since the + paired review event's delivery can't be verified from a comment + payload, that was a silent loss. The fix lets this companion fire on + its own terms instead. + + The companion's own `pull_request_review_comment` trigger ALSO requires + a mention here (the strictest version of the scenario), and the mention + is present only in the comment body -- proving the fix actually + delivers the content, not just an incidental extra fire. + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "reviewer", + { + "name": "reviewer", + "github": { + "bindings": [ + { + "repo": "a/b", + "triggers": { + # The gap: this trigger requiring a mention means + # the paired `pull_request_review` event is not a + # guaranteed delivery path for this binding. + "pull_request_review": {"require_mention": True}, + "pull_request_review_comment": {"require_mention": True}, + }, + } + ], + }, + }, + ) + payload = { + "action": "created", + "pull_request": {"number": 7}, + "comment": { + # The mention lives ONLY here -- the (separate, not modeled in + # this payload) review summary has none. + "body": "@reviewer this needs another look before merging.", + "user": {"login": "alice"}, + "pull_request_review_id": 999001, + "in_reply_to_id": None, + }, + "repository": {"full_name": "a/b"}, + "sender": {"login": "alice"}, + } + result = await fanout_event(bus, "pull_request_review_comment", "del-req-mention-1", payload) + assert result["fired_agents"] == ["reviewer"], result + assert result["skipped"] == [], result + messages = await _drain(bus) + assert len(messages) == 1 + assert messages[0].metadata["agent_name"] == "reviewer" + assert "@reviewer this needs another look" in messages[0].text + + +@pytest.mark.asyncio +async def test_review_comment_gate_is_independent_per_agent_in_same_call(base_dir: Path) -> None: + """Two bindings on the same repo/event, evaluated in the SAME + `fanout_event` call: one dual-subscribed (genuinely covered -> + suppressed) and one review-comment-only (not covered -> fires). + Willem-bd's review flagged this combined case as untested -- the + per-binding decision must be independent within a single call, not + accidentally global or order-dependent. + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "coder", + { + "name": "coder", + "github": { + "bindings": [ + { + "repo": "a/b", + "triggers": { + "pull_request_review": {}, + "pull_request_review_comment": {"require_mention": False}, + }, + } + ], + }, + }, + ) + _write_agent( + base_dir, + "default", + "notifier", + { + "name": "notifier", + "github": { + "bindings": [ + { + "repo": "a/b", + "triggers": {"pull_request_review_comment": {"require_mention": False}}, + } + ], + }, + }, + ) + payload = { + "action": "created", + "pull_request": {"number": 7}, + "comment": { + "body": "nit: consider renaming this variable.", + "user": {"login": "coderabbitai[bot]"}, + "pull_request_review_id": 999001, + "in_reply_to_id": None, + }, + "repository": {"full_name": "a/b"}, + "sender": {"login": "coderabbitai[bot]"}, + } + result = await fanout_event(bus, "pull_request_review_comment", "del-multi-agent-1", payload) + assert set(result["matched_agents"]) == {"coder", "notifier"}, result + assert result["fired_agents"] == ["notifier"], result + assert result["skipped"] == [{"agent": "coder", "reason": "redundant_review_comment"}], result + messages = await _drain(bus) + assert len(messages) == 1 + assert messages[0].metadata["agent_name"] == "notifier" + + +@pytest.mark.asyncio +async def test_review_comment_redundant_skip_prefers_own_trigger_reason_when_it_also_fails(base_dir: Path) -> None: + """Minor finding (willem-bd): when a companion is suppressed as + redundant AND would independently have failed its own trigger filter + (e.g. its own `require_mention` isn't satisfied), the more specific + reason is reported instead of the generic `redundant_review_comment` -- + useful for operator debugging even though the event is skipped either + way. + """ + bus = MessageBus() + _write_agent( + base_dir, + "default", + "reviewer", + { + "name": "reviewer", + "github": { + "bindings": [ + { + "repo": "a/b", + "triggers": { + # Not mention-gated -- genuinely covered. + "pull_request_review": {}, + # But THIS trigger requires a mention the + # comment body below doesn't have. + "pull_request_review_comment": {"require_mention": True}, + }, + } + ], + }, + }, + ) + payload = { + "action": "created", + "pull_request": {"number": 7}, + "comment": { + "body": "nit: consider renaming this variable.", + "user": {"login": "coderabbitai[bot]"}, + "pull_request_review_id": 999001, + "in_reply_to_id": None, + }, + "repository": {"full_name": "a/b"}, + "sender": {"login": "coderabbitai[bot]"}, + } + result = await fanout_event(bus, "pull_request_review_comment", "del-precedence-1", payload) + assert result["fired_agents"] == [], result + assert len(result["skipped"]) == 1 + reason = result["skipped"][0]["reason"] + assert reason != "redundant_review_comment", result + assert "mention" in reason, result diff --git a/backend/tests/test_github_prompts.py b/backend/tests/test_github_prompts.py index 3105cd6f4..ed6f3009a 100644 --- a/backend/tests/test_github_prompts.py +++ b/backend/tests/test_github_prompts.py @@ -185,6 +185,43 @@ def test_pull_request_review_prompt_includes_state() -> None: assert "alice" in prompt +def test_pull_request_review_prompt_instructs_fetching_inline_comments() -> None: + """PR #4131 review (Concern 1, zhfeng): the dispatcher's redundant + review-comment gate assumes the agent recovers inline comment content + from the parent `pull_request_review` event -- nothing enforced that + before this. The prompt must tell the agent how, with the exact + `gh api` path GitHub uses for a review's inline comments. + """ + payload = { + "action": "submitted", + "pull_request": {"number": 5}, + "review": { + "id": 999001, + "state": "changes_requested", + "user": {"login": "alice"}, + "body": "Looks risky", + }, + "repository": {"full_name": "a/b"}, + } + prompt = build_prompt("pull_request_review", payload) + assert "gh api repos/a/b/pulls/5/reviews/999001/comments" in prompt + + +def test_pull_request_review_prompt_omits_fetch_hint_without_review_id() -> None: + """Guard: never render a broken `.../reviews/None/comments` path when + the payload is missing `review.id` (or the PR number) -- omit the + instruction entirely rather than pointing the agent at a bad command. + """ + payload = { + "action": "submitted", + "pull_request": {"number": 5}, + "review": {"state": "approved", "user": {"login": "alice"}, "body": "LGTM"}, + "repository": {"full_name": "a/b"}, + } + prompt = build_prompt("pull_request_review", payload) + assert "gh api" not in prompt + + def test_issues_prompt() -> None: payload = { "action": "opened",