furyhawk/deer-flow
1
0
Fork 0
mirror of https://github.com/bytedance/deer-flow.git synced 2026-07-21 02:05:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(channels): scope review-comment suppression to bindings that also see the review

Browse Source 
The redundant pull_request_review_comment filter suppressed every
companion comment unconditionally, before the registry lookup even ran.
That premise only holds for a binding that also subscribes to
pull_request_review on the same repo -- events are opt-in per binding,
so a binding registered for pull_request_review_comment alone never
receives the parent review event and the companion comments were its
only delivery of the review's inline content. Suppressing those too was
a silent, total loss for that binding, not noise reduction.

Move the check into the per-agent loop so it only suppresses a matched
binding's companion comment when that same binding also has an active
pull_request_review trigger on this repo, reusing the existing registry
lookup rather than re-walking bindings by hand. Bindings subscribed to
pull_request_review_comment alone now always fire, matching the
opt-in-per-binding contract documented in triggers.py.
This commit is contained in:
Daoyuan Li
2026-07-12 22:43:37 -07:00
parent cb975443ac
commit b3791b33bb
2 changed files with 209 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/app/gateway/github/dispatcher.py
+69 -26
View File
@@ -7,9 +7,9 @@ first-class :class:`Channel` (see ``app/channels/github.py``):
POST /api/webhooks/github
→ verify HMAC (route)
→ :func:`fanout_event` (this module)
• drop redundant review-comment webhook noise
• 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
@@ -102,17 +102,15 @@ def _is_self_event(
def _is_redundant_review_comment(payload: dict[str, Any]) -> bool:
"""Return True if this ``pull_request_review_comment`` is fan-out noise
from a ``pull_request_review`` submission the agent already saw.
"""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 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.
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
@@ -125,6 +123,29 @@ def _is_redundant_review_comment(payload: dict[str, Any]) -> bool:
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 therefore an
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).
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.
"""
comment = payload.get("comment")
if not isinstance(comment, dict):
@@ -173,22 +194,7 @@ async def fanout_event(
repo, number = target
# 2. Redundant review-comment fan-out filter. Checked here — before
# the registry lookup / per-agent loop below — because redundancy
# is a property of the EVENT itself, not of any specific agent
# binding: filtering once up front avoids resolving the registry
# and iterating every matched binding for an event no binding
# should ever need to see. See :func:`_is_redundant_review_comment`.
if event == "pull_request_review_comment" and _is_redundant_review_comment(payload):
logger.info(
"github_fanout: skipped (reason=redundant_review_comment, repo=%s#%s, delivery=%s)",
repo,
number,
delivery_id,
)
return {"matched_agents": [], "fired_agents": [], "skipped": [{"reason": "redundant_review_comment"}]}
# 3. Bound-agent lookup. The registry is mtime-cached internally so
# 2. Bound-agent lookup. The registry is mtime-cached internally so
# the warm path is iterdir + stat only — but the cold path (and
# every first call after an operator edit) parses every
# config.yaml on disk. Run it off the event loop in both cases so
@@ -204,6 +210,24 @@ 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 has an 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.
# ``covered_by_review_trigger`` 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).
is_redundant_review_comment = event == "pull_request_review_comment" and _is_redundant_review_comment(payload)
covered_by_review_trigger: set[tuple[str, str]] = {(m.user_id, m.agent.name) for m in lookup_agents(registry, repo, "pull_request_review")} if is_redundant_review_comment else set()
for match in matches:
agent = match.agent
# ``cfg.github`` is non-None on every match by construction —
@@ -227,7 +251,26 @@ async def fanout_event(
skipped.append({"agent": agent.name, "reason": "self_event"})
continue
# 5. Trigger filter.
# 5. 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 — i.e. it has its own 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.
if is_redundant_review_comment and (match.user_id, agent.name) in covered_by_review_trigger:
logger.info(
"github_fanout: agent=%s skipped (reason=redundant_review_comment, repo=%s#%s, delivery=%s)",
agent.name,
repo,
number,
delivery_id,
)
skipped.append({"agent": agent.name, "reason": "redundant_review_comment"})
continue
# 6. 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``:
@@ -268,7 +311,7 @@ async def fanout_event(
skipped.append({"agent": agent.name, "reason": reason})
continue
# 6. Build prompt + publish inbound message onto the bus.
# 7. Build prompt + publish inbound message onto the bus.
prompt = build_prompt(event, payload)
thread_id = resolve_thread_id(repo, number, agent.name)
backend/tests/test_github_dispatcher.py
+140 -7
View File
@@ -1048,14 +1048,32 @@ async def test_coder_and_reviewer_on_same_pr_get_distinct_threads(base_dir: Path
# `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 must be suppressed before it reaches
any agent binding -- even one that would otherwise match.
`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
@@ -1072,7 +1090,13 @@ async def test_review_comment_companion_to_review_is_suppressed(base_dir: Path)
"bindings": [
{
"repo": "a/b",
"triggers": {"pull_request_review_comment": {"require_mention": False}},
"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},
},
}
],
},
@@ -1091,14 +1115,123 @@ async def test_review_comment_companion_to_review_is_suppressed(base_dir: Path)
"sender": {"login": "coderabbitai[bot]"},
}
result = await fanout_event(bus, "pull_request_review_comment", "del-storm-1", payload)
# Filtered before the registry lookup even runs -- matched_agents stays
# empty, exactly like the existing "no_target" early-return shape.
assert result["matched_agents"] == [], result
# 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["reason"] == "redundant_review_comment" for s in result["skipped"]), 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