From 74392e1470b4235d6b6c8551e036d0f6c1db6792 Mon Sep 17 00:00:00 2001 From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com> Date: Sun, 12 Jul 2026 00:51:22 -0700 Subject: [PATCH] Fix require_mention gating on whitespace-only bot_login/mention_login (#4055) github.bot_login and trigger.mention_login were read raw in the require_mention precedence chain, so a whitespace-only value (e.g. " ") never fell through to the working fallback the chain documents - Python truthiness lets " " win an `or` chain over agent.name or the operator default. The third link (channels.github.default_mention_login) was already correctly normalized and pinned by test_operator_default_blank_string_treated_as_none; this closes the gap for the other two by normalizing GitHubTriggerConfig.mention_login and GitHubAgentConfig.bot_login once, at the config layer, via a field_validator mirroring the pattern already used elsewhere in the config package. --- backend/app/gateway/github/dispatcher.py | 8 ++ backend/app/gateway/github/triggers.py | 5 ++ .../harness/deerflow/config/agents_config.py | 38 +++++++- backend/tests/test_github_dispatcher.py | 89 +++++++++++++++++++ 4 files changed, 138 insertions(+), 2 deletions(-) diff --git a/backend/app/gateway/github/dispatcher.py b/backend/app/gateway/github/dispatcher.py index b214eabe1..98f09e919 100644 --- a/backend/app/gateway/github/dispatcher.py +++ b/backend/app/gateway/github/dispatcher.py @@ -201,6 +201,14 @@ async def fanout_event( # ``coder`` with ``require_mention: true`` and no per-trigger or # per-agent override silently required ``@coder`` mentions instead # of ``@deerflow-bot``. + # + # ``github.bot_login`` is normalized (whitespace-only -> None) by + # ``GitHubAgentConfig``'s field validator, so this ``or`` chain + # correctly falls through a misconfigured ``bot_login: " "`` + # instead of comparing mentions against a literal whitespace + # string. ``operator_default_mention_login`` is a plain function + # argument (not a validated model field), so it is normalized here + # explicitly. 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) diff --git a/backend/app/gateway/github/triggers.py b/backend/app/gateway/github/triggers.py index 0412154ae..c89799ed5 100644 --- a/backend/app/gateway/github/triggers.py +++ b/backend/app/gateway/github/triggers.py @@ -186,6 +186,11 @@ def event_should_fire( return True, f"allow_authors:{author}" if trigger.require_mention: + # ``trigger.mention_login`` is normalized (whitespace-only -> None) + # by ``GitHubTriggerConfig``'s field validator, so this ``or`` falls + # through a misconfigured ``mention_login: " "`` to + # ``default_mention_login`` instead of gating on a literal + # whitespace string that no real ``@mention`` could ever match. login = trigger.mention_login or default_mention_login body = _comment_body(event, payload) # Boundary-aware @-mention match: ``@deerflow`` must NOT match diff --git a/backend/packages/harness/deerflow/config/agents_config.py b/backend/packages/harness/deerflow/config/agents_config.py index a11019c65..0715811a8 100644 --- a/backend/packages/harness/deerflow/config/agents_config.py +++ b/backend/packages/harness/deerflow/config/agents_config.py @@ -13,7 +13,7 @@ from pathlib import Path from typing import Any import yaml -from pydantic import BaseModel, Field, model_validator +from pydantic import BaseModel, Field, field_validator, model_validator from deerflow.config.paths import get_paths from deerflow.runtime.user_context import get_effective_user_id @@ -24,6 +24,25 @@ SOUL_FILENAME = "SOUL.md" AGENT_NAME_PATTERN = re.compile(r"^[A-Za-z0-9-]+$") +def _blank_to_none(value: str | None) -> str | None: + """Normalize a whitespace-only string to ``None``; leave real values untouched. + + A whitespace-only string (e.g. ``" "``) is truthy in Python, so an + unstripped ``value or fallback`` expression never falls through to the + fallback. The ``require_mention`` precedence chain (``trigger.mention_login`` + -> ``github.bot_login`` -> ``channels.github.default_mention_login`` -> + ``agent.name``, see AGENTS.md) relies on exactly that fallthrough, so both + of the config-sourced links are normalized here, once, at the model layer + — every reader downstream (today's and any future one) sees an honest + "unset" instead of a literal whitespace string that can never match a + real ``@mention``. + """ + if value is None: + return None + stripped = value.strip() + return stripped or None + + class GitHubTriggerConfig(BaseModel): """Per-event trigger filter inside a :class:`GitHubBinding`.""" @@ -38,9 +57,17 @@ class GitHubTriggerConfig(BaseModel): # talk to the bot without typing the handle every time. allow_authors: list[str] = Field(default_factory=list) # Override the global default bot mention login for this trigger only. - # Useful when one agent answers as @bot-a and another as @bot-b. + # Useful when one agent answers as @bot-a and another as @bot-b. A + # whitespace-only value is normalized to None (see ``_blank_to_none``) so + # it is treated as unset and falls through to ``github.bot_login`` instead + # of being compared against literally. mention_login: str | None = None + @field_validator("mention_login") + @classmethod + def _normalize_mention_login(cls, value: str | None) -> str | None: + return _blank_to_none(value) + class GitHubBinding(BaseModel): """One (agent, repo) binding with per-event trigger overrides.""" @@ -71,6 +98,8 @@ class GitHubAgentConfig(BaseModel): # ``mention_login`` the agent uses for trigger matching. None means # "fall back to mention_login / agent name", which is fine when those # match the bot identity, but should be set explicitly when they differ. + # A whitespace-only value is normalized to None (see ``_blank_to_none``) + # so it is treated as unset and falls through the rest of the chain. bot_login: str | None = None # Override the default github-channel ``recursion_limit`` (250). GitHub # runs are autonomous and long-running by nature — clone, explore, edit, @@ -87,6 +116,11 @@ class GitHubAgentConfig(BaseModel): # never fires from a webhook, even if it has a ``github:`` block. bindings: list[GitHubBinding] = Field(default_factory=list) + @field_validator("bot_login") + @classmethod + def _normalize_bot_login(cls, value: str | None) -> str | None: + return _blank_to_none(value) + @model_validator(mode="after") def _unique_binding_repos(self) -> "GitHubAgentConfig": """Reject duplicate ``repo`` values across ``bindings``. diff --git a/backend/tests/test_github_dispatcher.py b/backend/tests/test_github_dispatcher.py index 187e1a819..c2defa7ad 100644 --- a/backend/tests/test_github_dispatcher.py +++ b/backend/tests/test_github_dispatcher.py @@ -798,6 +798,95 @@ async def test_operator_default_blank_string_treated_as_none(base_dir: Path) -> 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 # ---------------------------------------------------------------------------