mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-27 16:37:55 +00:00
838037188e
* feat(channels): share inbound webhook dedupe across pods via Postgres (#4120) * ci: run cross-pod inbound dedupe integration tests in CI Expose the job Postgres service via DEDUPE_TEST_POSTGRES_URL so the integration tests (issue #4120) actually execute instead of silently skipping. Normalize the URL for asyncpg (postgresql:// -> +asyncpg, drop libpq-only sslmode) and await the now-async _is_duplicate_inbound in test_github_dispatcher.
1741 lines
66 KiB
Python
1741 lines
66 KiB
Python
"""Tests for the GitHub webhook fan-out helper.
|
|
|
|
We do not exercise the full agent run here — that path now lives in the
|
|
ChannelManager and is covered by integration tests. These tests verify
|
|
the fan-out logic: bot-loop prevention, target extraction, registry
|
|
lookup, trigger filtering, and that one InboundMessage with the right
|
|
shape lands on the bus per matching binding.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
import yaml
|
|
|
|
from app.channels.message_bus import InboundMessage, MessageBus
|
|
from app.gateway.github.dispatcher import fanout_event
|
|
|
|
|
|
def _write_agent(base: Path, user_id: str, name: str, body: dict) -> Path:
|
|
agent_dir = base / "users" / user_id / "agents" / name
|
|
agent_dir.mkdir(parents=True, exist_ok=True)
|
|
(agent_dir / "config.yaml").write_text(yaml.safe_dump(body), encoding="utf-8")
|
|
return agent_dir
|
|
|
|
|
|
@pytest.fixture()
|
|
def base_dir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path:
|
|
monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path))
|
|
from deerflow.config import paths as paths_module
|
|
|
|
monkeypatch.setattr(paths_module, "_paths", None)
|
|
# Each test uses a fresh tmp_path, so the registry's mtime cache from
|
|
# a prior test would short-circuit the scan and return [] — drop it.
|
|
from app.gateway.github.registry import _invalidate_cache
|
|
|
|
_invalidate_cache()
|
|
return tmp_path
|
|
|
|
|
|
async def _drain(bus: MessageBus) -> list[InboundMessage]:
|
|
out: list[InboundMessage] = []
|
|
while not bus.inbound_queue.empty():
|
|
out.append(await bus.get_inbound())
|
|
return out
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Bot-loop prevention — per-agent self-event gate
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_self_event_skips_the_owning_agent_only(base_dir: Path) -> None:
|
|
"""Events sent by an agent's own bot account skip THAT agent only.
|
|
|
|
The reviewer (whose ``mention_login`` is ``llm-gateway-ai``) must not
|
|
re-trigger on its own comment. The coder (different identity) should
|
|
still fire on the same event if its triggers match.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"reviewer-llm-gateway",
|
|
{
|
|
"name": "reviewer-llm-gateway",
|
|
"github": {
|
|
"bindings": [
|
|
{
|
|
"repo": "zhfeng/llm-gateway",
|
|
"triggers": {
|
|
"issue_comment": {
|
|
"require_mention": True,
|
|
"mention_login": "llm-gateway-ai",
|
|
}
|
|
},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"coding-llm-gateway",
|
|
{
|
|
"name": "coding-llm-gateway",
|
|
"github": {
|
|
"bindings": [
|
|
{
|
|
"repo": "zhfeng/llm-gateway",
|
|
"triggers": {
|
|
"issue_comment": {
|
|
"require_mention": True,
|
|
"mention_login": "coding-llm-gateway-ai",
|
|
}
|
|
},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 5, "pull_request": {"url": "..."}},
|
|
"comment": {
|
|
"body": "Following up @coding-llm-gateway-ai please address this.",
|
|
"user": {"login": "llm-gateway-ai[bot]"},
|
|
},
|
|
"repository": {"full_name": "zhfeng/llm-gateway"},
|
|
"sender": {"login": "llm-gateway-ai[bot]"},
|
|
}
|
|
result = await fanout_event(bus, "issue_comment", "del-self", payload)
|
|
# Reviewer matched but skipped — sender is its own identity.
|
|
assert "reviewer-llm-gateway" in result["matched_agents"]
|
|
assert "reviewer-llm-gateway" not in result["fired_agents"]
|
|
assert any(s["agent"] == "reviewer-llm-gateway" and s["reason"] == "self_event" for s in result["skipped"])
|
|
# Coder fires — same event, different self-identity.
|
|
assert "coding-llm-gateway" in result["fired_agents"]
|
|
messages = await _drain(bus)
|
|
assert len(messages) == 1
|
|
assert messages[0].metadata["agent_name"] == "coding-llm-gateway"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_third_party_bot_events_are_not_skipped(base_dir: Path) -> None:
|
|
"""Events from other bots (Copilot, CodeRabbit, …) must reach the agents.
|
|
|
|
The old all-bots short-circuit blocked these as a side effect; the new
|
|
per-agent self-event gate only fires when ``sender.login`` matches the
|
|
agent's own identity.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"reviewer-llm-gateway",
|
|
{
|
|
"name": "reviewer-llm-gateway",
|
|
"github": {
|
|
"bindings": [
|
|
{
|
|
"repo": "zhfeng/llm-gateway",
|
|
"triggers": {
|
|
"issue_comment": {
|
|
"require_mention": True,
|
|
"mention_login": "llm-gateway-ai",
|
|
}
|
|
},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 9, "pull_request": {"url": "..."}},
|
|
"comment": {
|
|
"body": "Hey @llm-gateway-ai, here is my review.",
|
|
"user": {"login": "Copilot"},
|
|
},
|
|
"repository": {"full_name": "zhfeng/llm-gateway"},
|
|
"sender": {"login": "Copilot", "type": "Bot"},
|
|
}
|
|
result = await fanout_event(bus, "issue_comment", "del-copilot", payload)
|
|
assert "reviewer-llm-gateway" in result["fired_agents"]
|
|
assert result["skipped"] == []
|
|
messages = await _drain(bus)
|
|
assert len(messages) == 1
|
|
assert messages[0].metadata["agent_name"] == "reviewer-llm-gateway"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_agent_name_is_only_a_fallback_when_no_explicit_identity_is_set(base_dir: Path) -> None:
|
|
"""``agent.name`` must NOT be in the self-identity set when ``bot_login``
|
|
or any ``mention_login`` is configured.
|
|
|
|
Otherwise a real GitHub user whose login happens to equal an agent's
|
|
directory name (``reviewer``, ``coder``, ``bot`` — all valid GitHub
|
|
logins) would be silently dropped. The fallback only kicks in when the
|
|
operator gave us nothing more specific.
|
|
"""
|
|
bus = MessageBus()
|
|
# Agent named ``reviewer`` with an explicit bot_login that differs.
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"reviewer",
|
|
{
|
|
"name": "reviewer",
|
|
"github": {
|
|
"bot_login": "reviewer-app-bot",
|
|
"bindings": [
|
|
{
|
|
"repo": "a/b",
|
|
"triggers": {"pull_request": {"actions": ["opened"]}},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
# Real human user with login ``reviewer`` opens a PR.
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "user": {"login": "reviewer"}, "title": "Fix typo", "body": ""},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "reviewer"},
|
|
}
|
|
result = await fanout_event(bus, "pull_request", "del-collision", payload)
|
|
# The agent must fire — the human's login collides with the agent
|
|
# directory name, but ``bot_login`` is the only true self-identity.
|
|
assert result["fired_agents"] == ["reviewer"]
|
|
assert result["skipped"] == []
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_agent_name_fallback_still_works_with_no_explicit_identity(base_dir: Path) -> None:
|
|
"""When no bot_login / mention_login is set, agent.name IS the self-identity.
|
|
|
|
Preserves the safety net for the simplest config — an agent that just
|
|
posts as ``@<agent-name>`` without configuring anything else still
|
|
avoids re-triggering itself.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"solo-bot",
|
|
{
|
|
"name": "solo-bot",
|
|
"github": {
|
|
"bindings": [{"repo": "a/b", "triggers": {"pull_request": {"actions": ["opened"]}}}],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 2, "user": {"login": "solo-bot"}, "title": "x", "body": ""},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "solo-bot[bot]"},
|
|
}
|
|
result = await fanout_event(bus, "pull_request", "del-solo", payload)
|
|
assert result["fired_agents"] == []
|
|
assert any(s["reason"] == "self_event" for s in result["skipped"])
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Events without targets
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_ping_returns_no_target(base_dir: Path) -> None:
|
|
bus = MessageBus()
|
|
result = await fanout_event(bus, "ping", "del-1", {"zen": "x"})
|
|
assert result["matched_agents"] == []
|
|
assert any(r["reason"] == "no_target" for r in result["skipped"])
|
|
assert await _drain(bus) == []
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# No matching agents
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_no_matching_agents_returns_empty(base_dir: Path) -> None:
|
|
bus = MessageBus()
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "user": {"login": "zhfeng"}},
|
|
"repository": {"full_name": "a/b"},
|
|
}
|
|
result = await fanout_event(bus, "pull_request", "del-1", payload)
|
|
assert result == {"matched_agents": [], "fired_agents": [], "skipped": []}
|
|
assert await _drain(bus) == []
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_matching_agent_for_different_repo_skips(base_dir: Path) -> None:
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"bot",
|
|
{
|
|
"name": "bot",
|
|
"github": {"bindings": [{"repo": "other/repo"}]},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "user": {"login": "zhfeng"}},
|
|
"repository": {"full_name": "a/b"},
|
|
}
|
|
result = await fanout_event(bus, "pull_request", "del-1", payload)
|
|
assert result == {"matched_agents": [], "fired_agents": [], "skipped": []}
|
|
assert await _drain(bus) == []
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Happy paths
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_pull_request_opened_fires_and_publishes(base_dir: Path) -> None:
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"reviewer",
|
|
{
|
|
"name": "reviewer",
|
|
"github": {
|
|
"installation_id": 1234,
|
|
"bindings": [
|
|
{
|
|
"repo": "zhfeng/llm-gateway",
|
|
"triggers": {"pull_request": {"actions": ["opened"]}},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {
|
|
"number": 7,
|
|
"title": "Add feature",
|
|
"user": {"login": "zhfeng"},
|
|
"body": "This is my change.",
|
|
},
|
|
"repository": {"full_name": "zhfeng/llm-gateway"},
|
|
"sender": {"login": "zhfeng"},
|
|
}
|
|
result = await fanout_event(bus, "pull_request", "del-abc", payload)
|
|
assert result["matched_agents"] == ["reviewer"]
|
|
assert result["fired_agents"] == ["reviewer"]
|
|
assert result["skipped"] == []
|
|
|
|
messages = await _drain(bus)
|
|
assert len(messages) == 1
|
|
msg = messages[0]
|
|
assert msg.channel_name == "github"
|
|
assert msg.chat_id == "zhfeng/llm-gateway"
|
|
# topic_id pairs the PR number with the agent name so the
|
|
# ChannelStore key separates per-agent threads on the same PR.
|
|
assert msg.topic_id == "7:reviewer"
|
|
assert msg.user_id == "zhfeng"
|
|
assert msg.owner_user_id == "default"
|
|
assert "Add feature" in msg.text
|
|
assert msg.metadata["agent_name"] == "reviewer"
|
|
gh = msg.metadata["github"]
|
|
assert gh["repo"] == "zhfeng/llm-gateway"
|
|
assert gh["number"] == 7
|
|
assert gh["installation_id"] == 1234
|
|
assert gh["event"] == "pull_request"
|
|
assert gh["delivery_id"] == "del-abc"
|
|
# recursion_limit defaults to None when the agent doesn't set one;
|
|
# ChannelManager._resolve_run_params falls back to the channel default
|
|
# (250) in that case.
|
|
assert gh["recursion_limit"] is None
|
|
# Deterministic thread id is surfaced as preferred_thread_id so the
|
|
# manager's first-create path pins the LangGraph thread to it.
|
|
assert msg.metadata["preferred_thread_id"] == gh["thread_id"]
|
|
assert msg.metadata["preferred_thread_id"] # non-empty
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_per_agent_recursion_limit_flows_through_metadata(base_dir: Path) -> None:
|
|
"""An agent's ``github.recursion_limit`` is ferried to ChannelManager.
|
|
|
|
This is the bridge between the YAML config field and the actual
|
|
``run_config["recursion_limit"]`` the manager hands LangGraph: the
|
|
dispatcher reads it from the binding at fanout time and stashes it
|
|
in ``msg.metadata["github"]["recursion_limit"]`` so the manager
|
|
doesn't need to re-load the AgentConfig per delivery.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"refactorer",
|
|
{
|
|
"name": "refactorer",
|
|
"github": {
|
|
"installation_id": 1234,
|
|
"recursion_limit": 500,
|
|
"bindings": [
|
|
{
|
|
"repo": "owner/big-repo",
|
|
"triggers": {"pull_request": {"actions": ["opened"]}},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "user": {"login": "zhfeng"}, "title": "x", "body": ""},
|
|
"repository": {"full_name": "owner/big-repo"},
|
|
"sender": {"login": "zhfeng"},
|
|
}
|
|
await fanout_event(bus, "pull_request", "del-rl", payload)
|
|
messages = await _drain(bus)
|
|
assert len(messages) == 1
|
|
assert messages[0].metadata["github"]["recursion_limit"] == 500
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_issue_comment_with_mention_fires(base_dir: Path) -> None:
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"assistant",
|
|
{
|
|
"name": "assistant",
|
|
"github": {
|
|
"bindings": [
|
|
{
|
|
"repo": "zhfeng/llm-gateway",
|
|
"triggers": {
|
|
"issue_comment": {
|
|
"require_mention": True,
|
|
"mention_login": "assistant",
|
|
}
|
|
},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 11, "pull_request": {"url": "..."}},
|
|
"comment": {
|
|
"body": "Hey @assistant can you review this?",
|
|
"user": {"login": "zhfeng"},
|
|
},
|
|
"repository": {"full_name": "zhfeng/llm-gateway"},
|
|
"sender": {"login": "zhfeng"},
|
|
}
|
|
result = await fanout_event(bus, "issue_comment", "del-def", payload)
|
|
assert "assistant" in result["fired_agents"]
|
|
messages = await _drain(bus)
|
|
assert len(messages) == 1
|
|
assert messages[0].metadata["agent_name"] == "assistant"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_issue_comment_without_mention_skipped(base_dir: Path) -> None:
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"bot",
|
|
{
|
|
"name": "bot",
|
|
"github": {
|
|
"bindings": [
|
|
{
|
|
"repo": "a/b",
|
|
"triggers": {"issue_comment": {"require_mention": True}},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 2, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "general chat without mention", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
result = await fanout_event(bus, "issue_comment", "del-xyz", payload)
|
|
assert result["fired_agents"] == []
|
|
assert len(result["skipped"]) == 1
|
|
assert "mention" in result["skipped"][0]["reason"]
|
|
assert await _drain(bus) == []
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_require_mention_uses_bot_login_when_trigger_omits_mention_login(base_dir: Path) -> None:
|
|
"""Regression pin for willem-bd's finding #2 on PR #3754.
|
|
|
|
Mention gating must default to ``github.bot_login`` (the App's
|
|
@-handle, the same identity used by the self-event gate), not to the
|
|
agent's directory name. Previously this fell back to ``agent.name``,
|
|
so an operator who set ``github.bot_login: deerflow-bot`` on an agent
|
|
whose directory was named ``coder`` would see every legitimate
|
|
``@deerflow-bot`` mention rejected with ``mention required for @coder``
|
|
— the two gates disagreed on identity.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"coder", # agent directory name differs from the App's bot handle
|
|
{
|
|
"name": "coder",
|
|
"github": {
|
|
"bot_login": "deerflow-bot",
|
|
"bindings": [
|
|
{
|
|
"repo": "a/b",
|
|
# No per-trigger mention_login override — the
|
|
# default fallback path is what is under test.
|
|
"triggers": {"issue_comment": {"require_mention": True}},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
|
|
# Mentioning the configured bot_login should fire the agent.
|
|
mention_payload = {
|
|
"action": "created",
|
|
"issue": {"number": 7, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "hey @deerflow-bot please look at this", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
result = await fanout_event(bus, "issue_comment", "del-bot-mention", mention_payload)
|
|
assert result["fired_agents"] == ["coder"], result
|
|
drained = await _drain(bus)
|
|
assert len(drained) == 1
|
|
|
|
# Mentioning the agent's directory name (the previous fallback) must
|
|
# NOT fire — that was the exact misbehaviour reported.
|
|
bus_2 = MessageBus()
|
|
dirname_payload = {
|
|
**mention_payload,
|
|
"comment": {"body": "hey @coder look at this", "user": {"login": "alice"}},
|
|
}
|
|
result_2 = await fanout_event(bus_2, "issue_comment", "del-dir-mention", dirname_payload)
|
|
assert result_2["fired_agents"] == [], result_2
|
|
assert len(result_2["skipped"]) == 1
|
|
assert "mention required for @deerflow-bot" in result_2["skipped"][0]["reason"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_require_mention_falls_back_to_agent_name_when_no_bot_login(base_dir: Path) -> None:
|
|
"""When ``github.bot_login`` is unset, the agent's directory name is the
|
|
fallback — matching the precedence the self-event gate uses. This
|
|
preserves the existing behaviour for agents that never configured a
|
|
distinct App identity.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"assistant",
|
|
{
|
|
"name": "assistant",
|
|
"github": {
|
|
# No bot_login here.
|
|
"bindings": [
|
|
{
|
|
"repo": "a/b",
|
|
"triggers": {"issue_comment": {"require_mention": True}},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 9, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "hey @assistant please look", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
result = await fanout_event(bus, "issue_comment", "del-fallback", payload)
|
|
assert result["fired_agents"] == ["assistant"], result
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Operator-set default mention login (R8 — channels.github.default_mention_login)
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_operator_default_mention_login_used_when_agent_omits_bot_login(base_dir: Path) -> None:
|
|
"""Regression pin for willem-bd's R8 on PR #3754.
|
|
|
|
CLAUDE.md documents ``channels.github.default_mention_login`` as the
|
|
global default for ``require_mention`` triggers. When neither the
|
|
trigger nor the agent's ``github.bot_login`` sets a handle, this
|
|
operator-set default must be used as the fallback — *before* the
|
|
agent's directory name. Previously the chain skipped this step
|
|
entirely, so an operator setting ``default_mention_login:
|
|
deerflow-bot`` saw mentions still gated on ``@coder`` (the agent's
|
|
directory name).
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"coder",
|
|
{
|
|
"name": "coder",
|
|
"github": {
|
|
# No bot_login — exercises the operator-default branch.
|
|
"bindings": [
|
|
{"repo": "a/b", "triggers": {"issue_comment": {"require_mention": True}}},
|
|
],
|
|
},
|
|
},
|
|
)
|
|
|
|
# @deerflow-bot (the configured operator default) must fire.
|
|
fire_payload = {
|
|
"action": "created",
|
|
"issue": {"number": 7, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "hey @deerflow-bot please look", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
result = await fanout_event(
|
|
bus,
|
|
"issue_comment",
|
|
"del-opdef-fire",
|
|
fire_payload,
|
|
operator_default_mention_login="deerflow-bot",
|
|
)
|
|
assert result["fired_agents"] == ["coder"], result
|
|
|
|
# @coder (the agent-name last-resort fallback) must NOT fire when an
|
|
# operator default is configured — the operator's intent wins.
|
|
bus_2 = MessageBus()
|
|
skip_payload = {
|
|
**fire_payload,
|
|
"comment": {"body": "hey @coder look", "user": {"login": "alice"}},
|
|
}
|
|
result_2 = await fanout_event(
|
|
bus_2,
|
|
"issue_comment",
|
|
"del-opdef-skip",
|
|
skip_payload,
|
|
operator_default_mention_login="deerflow-bot",
|
|
)
|
|
assert result_2["fired_agents"] == [], result_2
|
|
assert "mention required for @deerflow-bot" in result_2["skipped"][0]["reason"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_agent_bot_login_outranks_operator_default(base_dir: Path) -> None:
|
|
"""Per-agent ``github.bot_login`` outranks the global operator default.
|
|
|
|
An agent that opts into its own App identity is the authority for its
|
|
own mention handle. The operator default is the *fallback* for agents
|
|
that haven't configured one — it must not override the per-agent
|
|
setting (otherwise distinct App-per-agent deployments break).
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"reviewer",
|
|
{
|
|
"name": "reviewer",
|
|
"github": {
|
|
"bot_login": "reviewer-bot", # per-agent identity
|
|
"bindings": [
|
|
{"repo": "a/b", "triggers": {"issue_comment": {"require_mention": True}}},
|
|
],
|
|
},
|
|
},
|
|
)
|
|
|
|
# The operator default is set, but per-agent bot_login wins.
|
|
fire_payload = {
|
|
"action": "created",
|
|
"issue": {"number": 7, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "hey @reviewer-bot please review", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
result = await fanout_event(
|
|
bus,
|
|
"issue_comment",
|
|
"del-perbot",
|
|
fire_payload,
|
|
operator_default_mention_login="deerflow-bot",
|
|
)
|
|
assert result["fired_agents"] == ["reviewer"], result
|
|
|
|
# Mentioning the operator default while the agent has its own bot_login
|
|
# must NOT fire — the operator default is irrelevant here.
|
|
bus_2 = MessageBus()
|
|
miss_payload = {
|
|
**fire_payload,
|
|
"comment": {"body": "hey @deerflow-bot review please", "user": {"login": "alice"}},
|
|
}
|
|
result_2 = await fanout_event(
|
|
bus_2,
|
|
"issue_comment",
|
|
"del-perbot-skip",
|
|
miss_payload,
|
|
operator_default_mention_login="deerflow-bot",
|
|
)
|
|
assert result_2["fired_agents"] == [], result_2
|
|
assert "mention required for @reviewer-bot" in result_2["skipped"][0]["reason"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_trigger_mention_login_outranks_operator_default(base_dir: Path) -> None:
|
|
"""Per-trigger ``mention_login`` is the most specific override — it
|
|
must outrank both ``github.bot_login`` and the operator default.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"agent-x",
|
|
{
|
|
"name": "agent-x",
|
|
"github": {
|
|
"bot_login": "agent-x-bot",
|
|
"bindings": [
|
|
{
|
|
"repo": "a/b",
|
|
# Per-trigger override — most specific wins.
|
|
"triggers": {
|
|
"issue_comment": {
|
|
"require_mention": True,
|
|
"mention_login": "trigger-handle",
|
|
}
|
|
},
|
|
},
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 7, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "hey @trigger-handle please", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
result = await fanout_event(
|
|
bus,
|
|
"issue_comment",
|
|
"del-trigger-override",
|
|
payload,
|
|
operator_default_mention_login="deerflow-bot",
|
|
)
|
|
assert result["fired_agents"] == ["agent-x"], result
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_operator_default_blank_string_treated_as_none(base_dir: Path) -> None:
|
|
"""An empty / whitespace-only operator default must not silently
|
|
substitute itself as the mention handle.
|
|
|
|
A misconfigured ``channels.github.default_mention_login: ""`` would
|
|
otherwise yield ``require_mention`` gating on the empty string,
|
|
which silently lets every mention through (or rejects everything,
|
|
depending on how downstream logic treats it). Whitespace is stripped
|
|
and falsy values fall through to the existing ``agent.name``
|
|
fallback — preserving the pre-R8 contract for misconfigured installs.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"assistant",
|
|
{
|
|
"name": "assistant",
|
|
"github": {
|
|
"bindings": [
|
|
{"repo": "a/b", "triggers": {"issue_comment": {"require_mention": True}}},
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 7, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "hey @assistant please", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
# Whitespace-only operator default → fall through to agent.name.
|
|
result = await fanout_event(
|
|
bus,
|
|
"issue_comment",
|
|
"del-blank-opdef",
|
|
payload,
|
|
operator_default_mention_login=" ",
|
|
)
|
|
assert result["fired_agents"] == ["assistant"], result
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_bot_login_whitespace_only_treated_as_none(base_dir: Path) -> None:
|
|
"""A whitespace-only ``github.bot_login`` must not silently become the
|
|
mention-gating handle.
|
|
|
|
AGENTS.md documents the whole ``require_mention`` precedence chain
|
|
(``trigger.mention_login`` -> ``github.bot_login`` ->
|
|
``channels.github.default_mention_login`` -> ``agent.name``) as treating
|
|
whitespace-only defaults as unset. A misconfigured ``bot_login: " "``
|
|
(e.g. a YAML templating slip) is truthy in Python, so an unstripped
|
|
``github.bot_login or operator_default or agent.name`` never falls
|
|
through to the working ``agent.name`` fallback — every legitimate
|
|
``@assistant`` mention is silently rejected and the trigger can never
|
|
fire again until an operator notices and fixes the typo.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"assistant",
|
|
{
|
|
"name": "assistant",
|
|
"github": {
|
|
"bot_login": " ", # whitespace-only — must be treated as unset
|
|
"bindings": [
|
|
{"repo": "a/b", "triggers": {"issue_comment": {"require_mention": True}}},
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 7, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "hey @assistant please", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
# Whitespace-only bot_login → falls through to the agent.name fallback.
|
|
result = await fanout_event(bus, "issue_comment", "del-blank-bot-login", payload)
|
|
assert result["fired_agents"] == ["assistant"], result
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_trigger_mention_login_whitespace_only_treated_as_none(base_dir: Path) -> None:
|
|
"""A whitespace-only per-trigger ``mention_login`` must not silently
|
|
become the mention-gating handle either.
|
|
|
|
Same contract as ``test_bot_login_whitespace_only_treated_as_none``, one
|
|
link higher in the precedence chain: ``event_should_fire`` reads
|
|
``trigger.mention_login`` first. A misconfigured
|
|
``mention_login: " "`` is truthy, so an unstripped
|
|
``trigger.mention_login or default_mention_login`` never falls through
|
|
to the agent's real ``github.bot_login`` handle.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"coder",
|
|
{
|
|
"name": "coder",
|
|
"github": {
|
|
"bot_login": "deerflow-bot", # the real, working fallback handle
|
|
"bindings": [
|
|
{
|
|
"repo": "a/b",
|
|
"triggers": {
|
|
"issue_comment": {
|
|
"require_mention": True,
|
|
"mention_login": " ", # whitespace-only — must be treated as unset
|
|
}
|
|
},
|
|
},
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "created",
|
|
"issue": {"number": 7, "pull_request": {"url": "..."}},
|
|
"comment": {"body": "hey @deerflow-bot please look", "user": {"login": "alice"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
# Whitespace-only trigger.mention_login → falls through to github.bot_login.
|
|
result = await fanout_event(bus, "issue_comment", "del-blank-trigger-mention", payload)
|
|
assert result["fired_agents"] == ["coder"], result
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Multiple agents
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_multiple_agents_on_same_repo_event(base_dir: Path) -> None:
|
|
bus = MessageBus()
|
|
for n in ("alpha", "beta"):
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
n,
|
|
{
|
|
"name": n,
|
|
"github": {
|
|
"bindings": [
|
|
{"repo": "a/b", "triggers": {"pull_request": {}}},
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "user": {"login": "x"}},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "x"},
|
|
}
|
|
result = await fanout_event(bus, "pull_request", "del-multi", payload)
|
|
assert sorted(result["fired_agents"]) == ["alpha", "beta"]
|
|
messages = await _drain(bus)
|
|
assert sorted(m.metadata["agent_name"] for m in messages) == ["alpha", "beta"]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Registry scan stays off the event loop
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_fanout_offloads_registry_scan_to_thread(base_dir: Path, monkeypatch: pytest.MonkeyPatch) -> None:
|
|
"""The registry rebuild must not block the asyncio event loop.
|
|
|
|
A slow filesystem (NFS, container volume, large agent set) would
|
|
otherwise push the webhook past GitHub's 10s delivery timeout. We
|
|
offload via ``asyncio.to_thread``; verify by intercepting
|
|
``build_github_agent_registry`` and asserting it runs on a non-main
|
|
thread.
|
|
"""
|
|
import threading
|
|
|
|
main_thread = threading.get_ident()
|
|
seen_threads: list[int] = []
|
|
|
|
from app.gateway.github import dispatcher as dispatcher_module
|
|
|
|
real = dispatcher_module.build_github_agent_registry
|
|
|
|
def _spy() -> dict:
|
|
seen_threads.append(threading.get_ident())
|
|
return real()
|
|
|
|
monkeypatch.setattr(dispatcher_module, "build_github_agent_registry", _spy)
|
|
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"agent-x",
|
|
{"name": "agent-x", "github": {"bindings": [{"repo": "a/b", "triggers": {"pull_request": {"actions": ["opened"]}}}]}},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "user": {"login": "u"}, "title": "x", "body": ""},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "u"},
|
|
}
|
|
await fanout_event(MessageBus(), "pull_request", "del-thread", payload)
|
|
|
|
assert seen_threads, "registry scan was not invoked"
|
|
assert main_thread not in seen_threads, "registry scan must run off the event loop"
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Per-agent thread separation
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_coder_and_reviewer_on_same_pr_get_distinct_threads(base_dir: Path) -> None:
|
|
"""Two agents bound to the same PR must NOT share a LangGraph thread.
|
|
|
|
Headline guarantee of the per-agent thread design. If both agents
|
|
landed on one thread:
|
|
* ``multitask_strategy="reject"`` would silently drop one run
|
|
on every dual-mention (``@coder @reviewer please ...``).
|
|
* Their message histories and sandbox state would interleave.
|
|
* Cancelling one would interrupt the other.
|
|
|
|
Verify here that ``preferred_thread_id`` and ``topic_id`` both
|
|
diverge between bindings on the same ``(repo, number)``. ``topic_id``
|
|
is the ChannelStore key component, so the manager will look up a
|
|
different cached thread per agent and pin each to its own deterministic
|
|
UUID5 on first arrival.
|
|
"""
|
|
bus = MessageBus()
|
|
for n, login in (("coder", "coder-bot"), ("reviewer", "reviewer-bot")):
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
n,
|
|
{
|
|
"name": n,
|
|
"github": {
|
|
"bot_login": login,
|
|
"bindings": [
|
|
{
|
|
"repo": "a/b",
|
|
"triggers": {"pull_request": {"actions": ["opened"]}},
|
|
}
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 7, "user": {"login": "alice"}, "title": "x", "body": ""},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "alice"},
|
|
}
|
|
result = await fanout_event(bus, "pull_request", "del-split", payload)
|
|
assert sorted(result["fired_agents"]) == ["coder", "reviewer"]
|
|
|
|
messages = await _drain(bus)
|
|
assert len(messages) == 2
|
|
by_agent = {m.metadata["agent_name"]: m for m in messages}
|
|
|
|
coder, reviewer = by_agent["coder"], by_agent["reviewer"]
|
|
|
|
# Distinct LangGraph threads.
|
|
assert coder.metadata["preferred_thread_id"] != reviewer.metadata["preferred_thread_id"]
|
|
# Distinct store rows (manager keys on (channel_name, chat_id, topic_id)).
|
|
assert coder.topic_id != reviewer.topic_id
|
|
assert coder.topic_id == "7:coder"
|
|
assert reviewer.topic_id == "7:reviewer"
|
|
# 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"]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Inbound dedupe identity — redelivery / replay protection
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_delivery_id_populates_inbound_dedupe_identity(base_dir: Path) -> None:
|
|
"""Each fanned-out message carries the identity ChannelManager dedupes on.
|
|
|
|
The inbound dedupe added for the IM channels in PR #3584 keys on a
|
|
top-level ``metadata["message_id"]`` plus a workspace id. The GitHub
|
|
channel added later (PR #3754) never populated either, so a redelivered
|
|
webhook (native "Redeliver" button, REST API, or an operator's own
|
|
recovery script — GitHub does not auto-retry a failed delivery) re-ran
|
|
the agent. Fan-out now stamps the ``X-GitHub-Delivery`` GUID (scoped per owning
|
|
user + agent) as the message id and the repo as the workspace id,
|
|
exactly where ``ChannelManager._inbound_dedupe_key`` looks.
|
|
"""
|
|
bus = MessageBus()
|
|
_write_agent(
|
|
base_dir,
|
|
"default",
|
|
"reviewer",
|
|
{"name": "reviewer", "github": {"installation_id": 1234, "bindings": [{"repo": "zhfeng/llm-gateway", "triggers": {"pull_request": {"actions": ["opened"]}}}]}},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 7, "title": "x", "user": {"login": "zhfeng"}, "body": ""},
|
|
"repository": {"full_name": "zhfeng/llm-gateway"},
|
|
"sender": {"login": "zhfeng"},
|
|
}
|
|
await fanout_event(bus, "pull_request", "del-abc", payload)
|
|
(msg,) = await _drain(bus)
|
|
|
|
# Workspace id: the manager fails closed without one; repo is stable + unique.
|
|
assert msg.workspace_id == "zhfeng/llm-gateway"
|
|
# Stable per-(delivery, user, agent) message id the manager keys dedupe
|
|
# on, read from the top level of metadata (not the nested ``github``
|
|
# block). ``user_id`` here is "default" — the owning user this agent
|
|
# config lives under.
|
|
assert msg.metadata["message_id"] == "del-abc:default:reviewer"
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_dedupe_identity_stable_across_redelivery_and_distinct_per_agent(base_dir: Path) -> None:
|
|
"""Redelivery reproduces the same ids (deduped); agents/deliveries differ.
|
|
|
|
A single delivery fans out to N agents, so the dedupe id is scoped to
|
|
(delivery, agent): replaying the same ``X-GitHub-Delivery`` yields identical
|
|
ids per agent (the manager drops the replay) while two agents on one
|
|
delivery — or a genuinely new delivery — keep distinct ids and still fire.
|
|
"""
|
|
bus = MessageBus()
|
|
for n in ("coder", "reviewer"):
|
|
_write_agent(base_dir, "default", n, {"name": n, "github": {"bindings": [{"repo": "a/b", "triggers": {"pull_request": {"actions": ["opened"]}}}]}})
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "title": "x", "user": {"login": "u"}, "body": ""},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "u"},
|
|
}
|
|
|
|
async def _ids(delivery: str) -> dict[str, str]:
|
|
await fanout_event(bus, "pull_request", delivery, payload)
|
|
return {m.metadata["agent_name"]: m.metadata["message_id"] for m in await _drain(bus)}
|
|
|
|
first = await _ids("del-1")
|
|
redelivery = await _ids("del-1")
|
|
second = await _ids("del-2")
|
|
|
|
# Per-agent within one delivery: distinct, so neither agent is dropped.
|
|
assert first["coder"] != first["reviewer"]
|
|
# Redelivery of the same GUID: identical per agent (manager will dedupe).
|
|
assert redelivery == first
|
|
# New delivery: every id changes, so both agents fire again.
|
|
assert second["coder"] != first["coder"]
|
|
assert second["reviewer"] != first["reviewer"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_dedupe_identity_distinguishes_same_agent_name_across_users(base_dir: Path) -> None:
|
|
"""Two different users' same-named agents must not collide (willem-bd, PR #4104).
|
|
|
|
``ChannelManager._inbound_dedupe_key`` indexes on
|
|
``(channel, workspace_id, chat_id, message_id)``. For GitHub both
|
|
``workspace_id`` and ``chat_id`` are the repo, so ``owner_user_id`` was
|
|
never represented anywhere in the key. Before folding ``match.user_id``
|
|
into the per-message id, two users each binding an agent named
|
|
``reviewer`` to the same repo+event produced the *identical* id
|
|
``f"{delivery_id}:reviewer"`` for both fan-out messages, so
|
|
``ChannelManager._is_duplicate_inbound`` silently dropped the second
|
|
user's run as a false-positive duplicate of the first — even though
|
|
GitHub only delivered the webhook once and both users' agents matched.
|
|
"""
|
|
bus = MessageBus()
|
|
for user_id in ("alice", "bob"):
|
|
_write_agent(
|
|
base_dir,
|
|
user_id,
|
|
"reviewer",
|
|
{
|
|
"name": "reviewer",
|
|
"github": {
|
|
"bindings": [
|
|
{"repo": "a/b", "triggers": {"pull_request": {"actions": ["opened"]}}},
|
|
],
|
|
},
|
|
},
|
|
)
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "title": "x", "user": {"login": "u"}, "body": ""},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "u"},
|
|
}
|
|
result = await fanout_event(bus, "pull_request", "del-cross-user", payload)
|
|
assert result["fired_agents"] == ["reviewer", "reviewer"]
|
|
|
|
messages = await _drain(bus)
|
|
assert len(messages) == 2
|
|
by_owner = {m.owner_user_id: m for m in messages}
|
|
assert set(by_owner) == {"alice", "bob"}
|
|
|
|
# The dedupe id must differ even though the agent name is identical —
|
|
# otherwise the two users' fan-out messages are indistinguishable to
|
|
# the manager's dedupe.
|
|
assert by_owner["alice"].metadata["message_id"] != by_owner["bob"].metadata["message_id"]
|
|
|
|
# Prove the actual observable consequence, not just that the raw ids
|
|
# differ: neither user's message is treated as a duplicate of the
|
|
# other inside the same ChannelManager dedupe window.
|
|
from app.channels.manager import ChannelManager
|
|
from app.channels.store import ChannelStore
|
|
|
|
manager = ChannelManager(bus=MessageBus(), store=ChannelStore(path=base_dir / "dedupe-store.json"))
|
|
assert await manager._is_duplicate_inbound(by_owner["alice"]) is False
|
|
assert await manager._is_duplicate_inbound(by_owner["bob"]) is False
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_missing_delivery_header_leaves_dedupe_open(base_dir: Path) -> None:
|
|
"""Absent ``X-GitHub-Delivery`` fails open (no dedupe id), never collapses.
|
|
|
|
``delivery_id`` originates from an optional header and can be empty. An
|
|
empty value must not become a constant key that would silently drop
|
|
distinct deliveries — it yields no dedupe id, i.e. the pre-fix behavior.
|
|
|
|
This asserts the actual manager-level consequence, not just the raw
|
|
dispatcher-layer id: today ``_inbound_dedupe_key`` returns ``None`` for a
|
|
falsy ``message_id``, so ``_is_duplicate_inbound`` returns ``False`` and
|
|
never records a key. A future change that let a missing id fall through
|
|
as a real (constant) key would silently collapse every header-less
|
|
delivery into "the same" message; asserting on two separate header-less
|
|
deliveries pins that neither is ever treated as a duplicate of the other
|
|
(willem-bd, PR #4104 review).
|
|
"""
|
|
from app.channels.manager import ChannelManager
|
|
from app.channels.store import ChannelStore
|
|
|
|
bus = MessageBus()
|
|
_write_agent(base_dir, "default", "reviewer", {"name": "reviewer", "github": {"bindings": [{"repo": "a/b", "triggers": {"pull_request": {"actions": ["opened"]}}}]}})
|
|
payload = {
|
|
"action": "opened",
|
|
"pull_request": {"number": 1, "title": "x", "user": {"login": "u"}, "body": ""},
|
|
"repository": {"full_name": "a/b"},
|
|
"sender": {"login": "u"},
|
|
}
|
|
manager = ChannelManager(bus=MessageBus(), store=ChannelStore(path=base_dir / "dedupe-store.json"))
|
|
|
|
await fanout_event(bus, "pull_request", "", payload)
|
|
(first,) = await _drain(bus)
|
|
assert first.metadata["message_id"] is None
|
|
assert await manager._is_duplicate_inbound(first) is False
|
|
|
|
await fanout_event(bus, "pull_request", "", payload)
|
|
(second,) = await _drain(bus)
|
|
assert second.metadata["message_id"] is None
|
|
assert await manager._is_duplicate_inbound(second) is False
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# 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
|