Skip to content

fix(feishu): prevent duplicate text send when sendMedia has no media URL#52192

Open
kevinWangSheng wants to merge 1 commit into
openclaw:mainfrom
kevinWangSheng:fix/feishu-sendmedia-duplicate-text
Open

fix(feishu): prevent duplicate text send when sendMedia has no media URL#52192
kevinWangSheng wants to merge 1 commit into
openclaw:mainfrom
kevinWangSheng:fix/feishu-sendmedia-duplicate-text

Conversation

@kevinWangSheng
Copy link
Copy Markdown
Contributor

Summary

  • In the Feishu sendMedia handler, text is sent unconditionally at line 160 before checking for mediaUrl
  • When sendMedia is called with text but no media URL, the text-only fallback at line 196 sends the same text again
  • This results in duplicate messages in Feishu chats

Root cause

The sendMedia function has three code paths:

  1. Text + media: sends text (line 161), then sends media (line 173) — correct
  2. Media only: skips text (line 160), sends media (line 173) — correct
  3. Text only (no media): sends text (line 161), skips media (line 171), sends text AGAIN (line 196) — bug

The pre-media text send at line 160 was not guarded by a mediaUrl check, so it fires even when there is no media to follow.

Fix

Add && mediaUrl to the condition at line 160, so the companion text message is only sent when media follows it. The text-only fallback at line 196 handles the no-media case correctly on its own.

Test plan

  • All 13 Feishu outbound tests pass (pnpm test -- extensions/feishu/src/outbound)
  • Code inspection confirms the three code paths now produce exactly one text send each

🤖 Generated with Claude Code

In the sendMedia handler, text was sent unconditionally at line 160
before checking for mediaUrl. When sendMedia is called with text but
without a media URL, the text-only fallback at line 196 sends the
same text again, resulting in duplicate messages in Feishu chats.

Guard the pre-media text send with a mediaUrl check so the companion
text message is only sent when media follows it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@openclaw-barnacle openclaw-barnacle Bot added channel: feishu Channel integration: feishu size: XS labels Mar 22, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 22, 2026

Greptile Summary

This PR fixes a duplicate-message bug in the Feishu sendMedia handler. Previously, the companion text send at line 160 fired unconditionally whenever text was non-empty, regardless of whether a mediaUrl was present. When called with text but no media URL, the code would send the text once at line 160 and then again via the no-media fallback at line 196 — resulting in two identical messages in Feishu chats.

The fix adds && mediaUrl to the guard condition so the companion text is only sent when media actually follows it. The no-media fallback at line 196 correctly handles the text-only case on its own.

  • Change: Guard at line 160 changed from if (text?.trim()) to if (text?.trim() && mediaUrl) — minimal, correct, and well-targeted.
  • All three code paths now produce exactly one send each: text+media → companion text then media; media-only → media only; text-only → text-only fallback.
  • Pre-existing edge case (not introduced by this PR): if the media upload fails in the catch block (line 181-192), the fallback sends only the raw URL link and silently drops any companion text that was already sent. Worth addressing in a follow-up.

Confidence Score: 5/5

  • Safe to merge — minimal, targeted fix with passing tests and no new edge cases introduced.
  • The change is a single boolean guard addition that cleanly closes the duplicate-send path. All 13 Feishu outbound tests pass, the PR description accurately describes the root cause, and the fix aligns with the existing no-media fallback logic at line 196. No regressions are introduced.
  • No files require special attention.

Reviews (1): Last reviewed commit: "fix(feishu): prevent duplicate text send..." | Re-trigger Greptile

@openclaw-barnacle
Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Apr 29, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 29, 2026

Codex review: needs real behavior proof before merge.

Summary
The PR changes Feishu sendMedia to send companion text before media only when mediaUrl is present, and updates the adjacent comment.

Reproducibility: yes. by source inspection: call non-comment Feishu sendMedia with non-empty text and no mediaUrl; current main sends the companion text and then reaches the no-media fallback. I did not run tests or a live Feishu repro because this review kept the checkout read-only.

Real behavior proof
Needs real behavior proof before merge: The PR body and comments only provide tests/code inspection; the contributor should add redacted live Feishu chat, terminal/log output, a recording, screenshot, or linked artifact proof, then update the PR body for automatic re-review or ask for @clawsweeper re-review.

Next step before merge
This valid contributor PR needs real behavior proof and a stale-branch refresh, not a ClawSweeper replacement PR.

Security
Cleared: The diff only changes Feishu outbound control flow and a nearby comment, with no dependency, workflow, secret, install, build, package, or supply-chain surface touched.

Review findings

  • [P2] Preserve voice-media suppression when refreshing — extensions/feishu/src/outbound.ts:160
Review details

Best possible solution:

Land a refreshed minimal Feishu guard equivalent to text?.trim() && mediaUrl && !suppressTextForVoiceMedia, with focused regression coverage for text plus missing mediaUrl.

Do we have a high-confidence way to reproduce the issue?

Yes, by source inspection: call non-comment Feishu sendMedia with non-empty text and no mediaUrl; current main sends the companion text and then reaches the no-media fallback. I did not run tests or a live Feishu repro because this review kept the checkout read-only.

Is this the best way to solve the issue?

Yes with changes: adding a mediaUrl requirement is the narrow fix, but the stale branch should be refreshed to preserve !suppressTextForVoiceMedia and add a focused regression test.

Full review comments:

  • [P2] Preserve voice-media suppression when refreshing — extensions/feishu/src/outbound.ts:160
    Current main already suppresses companion text for Feishu native voice media. Applying this stale patch as text?.trim() && mediaUrl would drop the current !suppressTextForVoiceMedia guard, so refresh the condition to keep both checks, for example text?.trim() && mediaUrl && !suppressTextForVoiceMedia.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

Acceptance criteria:

  • pnpm test extensions/feishu/src/outbound.test.ts

What I checked:

  • current-main-sha: origin/main resolves to the provided current-main SHA used for source inspection. (0513b285ef95)
  • current-main-duplicate-path: Current main computes suppressTextForVoiceMedia, then sends companion text when text?.trim() && !suppressTextForVoiceMedia; with no mediaUrl, suppression is false and the handler later reaches the no-media text fallback, so text can be sent twice. (extensions/feishu/src/outbound.ts:710, 0513b285ef95)
  • optional-media-contract: ChannelOutboundContext.mediaUrl is optional, so sendMedia implementations must safely handle text with no media URL. (src/channels/plugins/outbound.types.ts:20, 0513b285ef95)
  • voice-helper-contract: The Feishu voice-media helper explicitly returns false when no mediaUrl is present, so the duplicate-send fix needs to add a separate mediaUrl guard while keeping voice suppression. (extensions/feishu/src/media.ts:894, 0513b285ef95)
  • current-main-voice-regression-guard: Current Feishu outbound tests cover suppressing duplicate text for voice media and preserving ordinary audio captions, so a refreshed PR must not drop that behavior. (extensions/feishu/src/outbound.test.ts:983, 0513b285ef95)
  • pr-diff-scope: The submitted patch only changes the older guard from text?.trim() to text?.trim() && mediaUrl, which is correct for the duplicate path but stale relative to current main's !suppressTextForVoiceMedia condition. (extensions/feishu/src/outbound.ts:160, 1d5d28ee5df9)

Likely related people:

  • steipete: Recent GitHub commit history shows repeated current-main Feishu outbound, media, and test maintenance, including voice-degradation behavior and test cleanup adjacent to this path. (role: recent area contributor; confidence: high; commits: 8dd6a2d323e1, 186de9daa0ed, 53d007bc878c; files: extensions/feishu/src/outbound.ts, extensions/feishu/src/outbound.test.ts, extensions/feishu/src/media.ts)
  • vincentkoc: Introduced the current Feishu voice-media text suppression helper, outbound guard, and related tests that a refresh of this PR must preserve. (role: recent feature contributor; confidence: high; commits: 14312ff5704f, 2161b46032e0, 4c72e605cdc7; files: extensions/feishu/src/outbound.ts, extensions/feishu/src/outbound.test.ts, extensions/feishu/src/media.ts)
  • doodlewind: Introduced the Feishu plugin outbound adapter shape that sent text before media and then fell back to text when no media URL existed. (role: introduced behavior; confidence: medium; commits: 2267d58afcc7; files: extensions/feishu/src/outbound.ts)

Remaining risk / open question:

  • The contributor has not provided after-fix real behavior proof from Feishu or equivalent redacted runtime output.
  • The branch is dirty/stale against current main and must compose the mediaUrl guard with the existing voice-media suppression guard.
  • There is no focused regression test yet for non-comment Feishu sendMedia with text and no mediaUrl.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 0513b285ef95.

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 29, 2026

Codex review: needs maintainer review before merge.

What this changes:

The PR changes the Feishu outbound sendMedia handler to send companion text before media only when mediaUrl is present, and updates the adjacent comment.

Maintainer follow-up before merge:

This is an active contributor implementation PR for a valid, narrow Feishu bug; the next action is maintainer review of the proposed guard and whether to require a regression test, not a replacement ClawSweeper fix PR.

Review details

Best possible solution:

Land the minimal mediaUrl guard in the Feishu outbound sendMedia handler and add or request a focused regression test proving text plus no mediaUrl sends exactly one Feishu text message, leaving broader upload-failure behavior to a separate follow-up if maintainers want it.

Acceptance criteria:

  • pnpm test extensions/feishu/src/outbound.test.ts

What I checked:

Likely related people:

  • steipete: Current local history shows the Feishu extension files, including this outbound handler and tests, present through recent main/release commits by Peter Steinberger; prior review context also links recent Feishu outbound/test cleanup to this maintainer. (role: recent maintainer; confidence: high; commits: 34d11d57579d, be8c24633aaa; files: extensions/feishu/src/outbound.ts, extensions/feishu/src/outbound.test.ts, docs/channels/feishu.md)
  • vincentkoc: Prior ClawSweeper history for this exact path links recent Feishu outbound/media sequencing work to this contributor. (role: recent adjacent maintainer; confidence: medium; commits: ad2516b1c876, 2161b46032e0, e4cc8cd97513; files: extensions/feishu/src/outbound.ts, extensions/feishu/src/outbound.test.ts)
  • doodlewind: Prior path-history review ties the original Feishu outbound adapter and text-first/no-media-fallback sendMedia shape to this contributor. (role: introduced behavior; confidence: medium; commits: 2267d58afcc7, 5f6e1c19bd18; files: extensions/feishu/src/outbound.ts)
  • guoqunabc: Prior path history links reply targeting and replyToId forwarding changes to the same Feishu sendMedia area and adjacent tests. (role: adjacent maintainer; confidence: medium; commits: 63ce7c74bdc0; files: extensions/feishu/src/outbound.ts, extensions/feishu/src/outbound.test.ts)

Remaining risk / open question:

  • The PR fixes the observable duplicate-send path, but it does not add a focused regression test for text plus missing mediaUrl.
  • Validation here was limited to read-only source, test, docs, PR context, and history inspection; no tests were run because this review was required to keep the checkout byte-for-byte clean.

Codex review notes: model gpt-5.5, reasoning high; reviewed against e46dccb35374.

@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: feishu Channel integration: feishu size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant