Fallback to current Feishu message for reactions#496
Conversation
evandance
left a comment
There was a problem hiding this comment.
Thanks — the friction this addresses is real (agents do try react({ emoji: '👍' }) first). Two changes here with very different risk profiles; please split.
readReactionMessageId — ready to merge (after split). Falling back to toolContext.currentMessageId is clean. SDK type checks out, precedence is right (messageId > message_id > fallback), tests cover the three branches.
Alias map — silent regression that CI didn't catch. Two entries in FEISHU_REACTION_ALIASES collide with canonical Feishu types that are already valid and in use:
['clap', 'APPLAUSE'] // `CLAP` ≠ `APPLAUSE` in Feishu (both valid, different emojis)
['heart', 'LOVE'] // `HEART` ≠ `LOVE` in Feishu (both valid, different emojis)The case-insensitive lookup (MAP.get(normalized) ?? MAP.get(normalized.toLowerCase()) ?? normalized) silently rewrites canonical uppercase type names at runtime:
normalizeReactionEmoji('HEART') -> 'LOVE' ⚠️
normalizeReactionEmoji('CLAP') -> 'APPLAUSE' ⚠️
There's no existing test exercising 'HEART' or 'CLAP' through normalizeReactionEmoji. The new 'MUSCLE' passthrough test only works because there's no muscle collision. So CI stays green even though any agent passing FeishuEmoji.HEART or FeishuEmoji.CLAP (canonical names already exported from src/messaging/outbound/reactions.ts) would post a different emoji after this merges.
Recommended fix: move aliasing to the tool schema, not code. The reason agents pass '👍' is the tool's emoji field description doesn't tell them what shape to use. The right place to teach the model is the schema, not a runtime alias map. It teaches once (the model sees the schema on every tool call and learns the canonical shape upfront, no validation-error round-trip), stays in sync automatically (VALID_FEISHU_EMOJI_TYPES is the authoritative list sourced from official Feishu docs and the schema description can reference it directly), keeps the typed contract honest (type stored = type passed, no hidden rewrites for future readers to chase), and rules out this regression class entirely (schema documentation can't silently rewrite caller input). Concretely, extend the emoji field description with something like "Feishu reaction type name (e.g. THUMBSUP, LAUGH, HEART, LOVE). See the full list in VALID_FEISHU_EMOJI_TYPES. Emoji unicode like 👍/+1 is not accepted — use the canonical type name."
Fallback (only if schema-layer isn't enough). If you have a trace showing the model keeps passing '👍' even after the description is updated, code-layer aliasing becomes justified. In that case, scope it to emoji unicode → canonical type only (drop word aliases like thumbsup/heart/clap — those collide) and gate on "not already in VALID_FEISHU_EMOJI_TYPES" so that if a value is already canonical it returns as-is and skips the map. This eliminates the regression class entirely and makes the contract explicit ("we only translate emoji unicode, never canonical names").
Action items. First, split this PR — readReactionMessageId + its 3 tests can ship as a standalone PR, and I'll approve and merge it as soon as the split lands. Second, for the alias half, update the tool schema description first; reopen with code-layer aliasing only if there's evidence schema docs aren't sufficient.
|
Thanks, this is a good catch. I agree the two parts should be split, and the I will keep the |
|
The branch is now narrowed to the mergeable The runtime emoji alias map from the earlier review commit is no longer in this PR, so canonical Validation already run locally:
Ready for re-review. |
b000586 to
c3d1513
Compare
|
Updated in c3d1513.\n\nChanges:\n- Rebased onto current |
Summary
react/reactionsfall back totoolContext.currentMessageIdwhen callers omitmessageId/message_id.Validation
Follow-up
Emoji-shape guidance should be handled separately in tool schema/description wording, without hidden runtime rewrites such as
HEART -> LOVEorCLAP -> APPLAUSE.