Conversation
zoolsher
left a comment
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR fixes mention-only messages being lost by channel adapters. When a bot mention is stripped and nothing else remains, both Feishu and Discord now preserve the message as the visible @<bot name> via a shared preserveMentionOnlyText helper, replacing Feishu's prior "hello" substitution and Discord's empty-string result (which the shared inbox dropped). The change is cleanly factored: one helper owns the fallback so the two providers can't drift, and it comes with focused unit tests for the helper and both adapters. I verified the full changed files (mention-only.ts, feishu.ts, discord.ts), traced the call sites, and confirmed no other code depended on the old "hello" behavior.\n\nThe design is sound and the invariants (empty non-mention events and Feishu media-only events stay empty) hold for Feishu because of the explicit rawContentType === "text" && mentionedBot guard plus the if (!text) return gate. My one substantive concern is on the Discord side: the mentionedBot signal is derived from message.mentions.users.has(bot.id), which in discord.js also returns true for reply-pinged users, so a plain reply to the bot (no typed @mention) counts as a mention. The remaining notes are cosmetic/SSOT nits.
Verdict: COMMENT — The core fix is correct and well-tested; the only real concern is a Discord reply-ping edge case that is non-blocking, plus minor consistency nits.
3 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | correctness | packages/core/src/channels/discord.ts |
Discord mentionedBot includes reply-pings, injecting "@rome" into empty/media-only replies |
| P3 | consistency | packages/core/src/channels/mention-only.ts |
Fallback renders "@name" but resolveMentions renders in-prose mentions without "@" |
| P3 | maintainability | packages/core/src/channels/mention-only.ts |
Hardcoded "Rome" default duplicates the bot-name fallback in webchat.ts |
| const text = normalizeDiscordMessageText( | ||
| message.content, | ||
| bot, | ||
| bot ? message.mentions.users.has(bot.id) : false, |
There was a problem hiding this comment.
[P2] correctness — Discord mentionedBot includes reply-pings, injecting "@rome" into empty/media-only replies
message.mentions.users.has(bot.id) is true not only when the user types <@botId> but also whenever the message is a reply that pings the bot (discord.js includes the replied-to user in mentions.users). For a normal prose reply this is harmless (text is preserved as-is), but for an empty or media-only reply to the bot the text becomes "@Rome" instead of staying empty — injecting synthetic user input and diverging from the PR's stated invariant that media-only events stay empty (which Feishu honors). Consider using discord.js's own helper that excludes the reply ping: message.mentions.has(bot.id, { ignoreRepliedUser: true }), so mentionedBot reflects an actual typed mention.
| botName?: string, | ||
| ): string { | ||
| if (text || !mentionedBot) return text; | ||
| return `@${botName?.trim() || "Rome"}`; |
There was a problem hiding this comment.
[P3] consistency — Fallback renders "@name" but resolveMentions renders in-prose mentions without "@"
The fallback produces @${botName}, but Feishu's resolveMentions replaces mention keys with the bare display name (no leading @, see feishu.ts:544 and its test "@_user_1 hi" -> "Bob hi"). So a mention-only message surfaces to the agent as "@Rome" while the same bot mention embedded in prose surfaces as "Rome". Not a bug, but the two representations of the same fact are inconsistent; worth aligning the format intentionally.
| botName?: string, | ||
| ): string { | ||
| if (text || !mentionedBot) return text; | ||
| return `@${botName?.trim() || "Rome"}`; |
There was a problem hiding this comment.
[P3] maintainability — Hardcoded "Rome" default duplicates the bot-name fallback in webchat.ts
The literal "Rome" default here duplicates the same fallback in webchat.ts prettyAgentName (line 14). If the default bot display name ever changes, these can drift. Consider sourcing the fallback from a single shared constant/helper rather than repeating the string per adapter.
yunfanye
left a comment
yunfanye
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR cleanly centralizes the mention-only fallback and correctly fixes the ordinary Discord and Feishu text paths without changing downstream routing contracts. However, the Feishu gate is narrower than the stated invariant: supported rich-text post messages can undergo the same mention stripping and remain silently dropped.
Verdict: REQUEST_CHANGES — The Feishu implementation still loses mention-only messages when they arrive as supported rich-text posts.
1 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | correctness | packages/core/src/channels/feishu.ts |
Preserve mention-only rich-text posts too |
| const text = resolvedText || (m.rawContentType === "text" && m.mentionedBot ? "hello" : ""); | ||
| const text = preserveMentionOnlyText( | ||
| resolvedText, | ||
| m.rawContentType === "text" && m.mentionedBot, |
There was a problem hiding this comment.
[P1] correctness — Preserve mention-only rich-text posts too
Feishu post events are also text-bearing, and the channel normalizer strips the bot mention while retaining mentionedBot (SDK docs). A rich-text post containing only the bot mention therefore reaches this code with empty resolvedText, but this predicate evaluates false and the following if (!text) return still drops it. Treat at least post as mention-preservable while continuing to exclude media-only types, and add a post mention-only regression test.
2 participants
What this PR does
Mention-only messages lose their only content when a channel adapter strips the bot mention. Feishu replaced that event with
hello, which changed the stored user input. Discord left it empty, so the shared inbox dropped it.Both adapters now preserve the visible mention as
@<bot name>when no prose remains. Messages with prose still reach the agent without the bot mention.Follow-up to #37.
Design & Invariants
Channel adapters absorb provider-specific mention syntax before routing. A stripped bot mention remains meaningful when it is the whole message. Empty non-mention events and Feishu media-only events stay empty.
One shared helper owns the fallback so the providers cannot drift.
Test plan
pnpm exec biome checkon all changed filespnpm --filter @rome/core typecheckpnpm test:unit(5,720 tests)pnpm typecheck— the local checkout fails in untouched mobile and UI files after the dependency update.pnpm dev:all— the existingrome-rome-1container already owns host port 80.