fix(feishu): download images in Feishu thread history and inline attachment hints [AI-assisted]#66432
Conversation
…chment hints Previously, image (and other media) messages in thread history were silently reduced to a placeholder string like "[image message]". The agent could see the placeholder text but never received the actual image data. Root cause: resolveFeishuMediaList was only called for the current inbound message, never for historical thread messages fetched via listFeishuThreadMessages. Changes: - Add rawContent (optional) field to FeishuThreadMessageInfo so the raw Feishu API body JSON is available after parsing. - Store rawContent in listFeishuThreadMessages results. - Replace the synchronous historyParts.map() with an async for-loop that calls resolveFeishuMediaList for each media message in thread history (image/file/audio/video/media/sticker/post types). - Embed downloaded media paths as [media attached: /path (mime) | /path] hints in the message body, matching the format the agent runner already uses for current-message attachments. - Errors are caught per-message and logged; history still renders without the attachment if download fails. - Add a test covering the image download and ThreadHistoryBody hint. AI-assisted: authored by ClawGuard (OpenClaw agent) via Feishu thread. Testing: pnpm test:extension feishu — 615 tests passing (614 existing + 1 new).
|
Took a look through this — nice fix, the reuse of A couple of things I'd want addressed before merging:
Also looked into whether this should be using |
|
Codex review: needs changes before merge. What this changes: The PR preserves raw Feishu content on fetched thread-history messages, resolves media-typed history messages through the existing Feishu media downloader, appends inline attachment hints to ThreadHistoryBody, and adds a happy-path Feishu bot test. Required change before merge: The remaining blockers are narrow and mechanical enough for an automated repair attempt on the PR branch or a replacement branch: starter-media handling, aggregate history-media caps, focused tests, and changelog. Security review: Security review needs attention: Security review found no CI, dependency, secret, or supply-chain change, but the diff introduces a concrete resource-exhaustion concern by resolving every media item in fetched thread history without an aggregate cap. Review findings:
Review detailsBest possible solution: Land a bounded Feishu-owned fix: preserve rawContent for both fetched root/starter messages and fetched thread-history messages, resolve media only after sender visibility filtering, cap historical media resolution by recent messages/items, keep the existing attachment hint contract, add happy-path, failure-path, starter-media, and cap tests, and include the required changelog entry. Do we have a high-confidence way to reproduce the issue? Yes. A high-confidence source-level reproduction is available by mocking a Feishu thread history item with contentType image and parsed content [image message]: current main formats that placeholder into ThreadHistoryBody. A root/starter image remains a second reproducible gap because root messages still come from getMessageFeishu without rawContent and are excluded from listFeishuThreadMessages. Is this the best way to solve the issue? Partly. Reusing resolveFeishuMediaList and the existing [media attached: ...] hint format is the right direction, but the PR needs starter-message coverage and bounded history downloads before it is the narrow maintainable fix. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4babd925c40d. |
Greptile SummaryThis PR resolves a gap where media messages in Feishu thread history were only ever surfaced as placeholder text ( Confidence Score: 4/5Safe to merge; no correctness or security issues found, only minor style and test-coverage observations. All findings are P2: a redundant array allocation inside a loop, sequential rather than parallel async downloads, and a missing error-path test case. No logic bugs, data-loss risks, or security concerns were identified. No files require special attention beyond the P2 notes above. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/feishu/src/bot.ts
Line: 965
Comment:
**`mediaTypes` array recreated on every loop iteration**
`mediaTypes` is declared with `const` inside the `for...of` loop body, so a new array is allocated for every history message. Since this list is a fixed constant, it should be hoisted above the loop (or de-duplicated against the identical array already defined inside `resolveFeishuMediaList`).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/feishu/src/bot.ts
Line: 960-994
Comment:
**Sequential media downloads across history messages**
Each `resolveFeishuMediaList` call is awaited one-at-a-time inside the loop. With up to 20 thread history messages (the `limit` used in `listFeishuThreadMessages`) that each contain media, this processes downloads serially. Collecting all the media-resolution promises and awaiting them with `Promise.all` (then reassembling the results in order) would significantly reduce latency in the common case.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/feishu/src/bot.test.ts
Line: 2672-2751
Comment:
**Error-path not exercised by the new test**
The test covers the happy path (download succeeds, hint appears in `ThreadHistoryBody`), but the per-message error path is untested: when `downloadMessageResourceFeishu` rejects, the expectation is that the message still appears in history (without the attachment hint) and a log entry is emitted. Worth adding a second case that mocks a rejection and asserts the envelope is still pushed with the original `[image message]` body.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(feishu): download images in Feishu t..." | Re-trigger Greptile |
| let msgBody = msg.content; | ||
|
|
||
| // Download and inline media for image/file/audio/video/sticker/post messages. | ||
| const mediaTypes = ["image", "file", "audio", "video", "media", "sticker", "post"]; |
There was a problem hiding this comment.
mediaTypes array recreated on every loop iteration
mediaTypes is declared with const inside the for...of loop body, so a new array is allocated for every history message. Since this list is a fixed constant, it should be hoisted above the loop (or de-duplicated against the identical array already defined inside resolveFeishuMediaList).
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/bot.ts
Line: 965
Comment:
**`mediaTypes` array recreated on every loop iteration**
`mediaTypes` is declared with `const` inside the `for...of` loop body, so a new array is allocated for every history message. Since this list is a fixed constant, it should be hoisted above the loop (or de-duplicated against the identical array already defined inside `resolveFeishuMediaList`).
How can I resolve this? If you propose a fix, please make it concise.| for (const msg of historyMessages) { | ||
| const role = msg.senderType === "app" ? "assistant" : "user"; | ||
| return core.channel.reply.formatAgentEnvelope({ | ||
| channel: "Feishu", | ||
| from: `${msg.senderId ?? "Unknown"} (${role})`, | ||
| timestamp: msg.createTime, | ||
| body: msg.content, | ||
| envelope: envelopeOptions, | ||
| }); | ||
| }); | ||
| let msgBody = msg.content; | ||
|
|
||
| // Download and inline media for image/file/audio/video/sticker/post messages. | ||
| const mediaTypes = ["image", "file", "audio", "video", "media", "sticker", "post"]; | ||
| if (mediaTypes.includes(msg.contentType)) { | ||
| try { | ||
| const mediaItems = await resolveFeishuMediaList({ | ||
| cfg, | ||
| messageId: msg.messageId, | ||
| messageType: msg.contentType, | ||
| content: msg.rawContent ?? "", | ||
| maxBytes: mediaMaxBytes, | ||
| log, | ||
| accountId: account.accountId, | ||
| }); | ||
| if (mediaItems.length > 0) { | ||
| const attachmentHints = mediaItems | ||
| .map( | ||
| (m) => | ||
| `[media attached: ${m.path} (${m.contentType ?? "application/octet-stream"}) | ${m.path}]`, | ||
| ) | ||
| .join("\n"); | ||
| msgBody = [msgBody, attachmentHints].filter(Boolean).join("\n"); | ||
| log( | ||
| `feishu[${account.accountId}]: downloaded ${mediaItems.length} media item(s) from thread history message ${msg.messageId}`, | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| log( | ||
| `feishu[${account.accountId}]: failed to download media for thread history message ${msg.messageId}: ${String(err)}`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Each resolveFeishuMediaList call is awaited one-at-a-time inside the loop. With up to 20 thread history messages (the limit used in listFeishuThreadMessages) that each contain media, this processes downloads serially. Collecting all the media-resolution promises and awaiting them with Promise.all (then reassembling the results in order) would significantly reduce latency in the common case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/bot.ts
Line: 960-994
Comment:
**Sequential media downloads across history messages**
Each `resolveFeishuMediaList` call is awaited one-at-a-time inside the loop. With up to 20 thread history messages (the `limit` used in `listFeishuThreadMessages`) that each contain media, this processes downloads serially. Collecting all the media-resolution promises and awaiting them with `Promise.all` (then reassembling the results in order) would significantly reduce latency in the common case.
How can I resolve this? If you propose a fix, please make it concise.| ); | ||
| }); | ||
|
|
||
| it("downloads images from thread history messages and inlines attachment hints", async () => { | ||
| mockShouldComputeCommandAuthorized.mockReturnValue(false); | ||
| mockGetMessageFeishu.mockResolvedValue({ | ||
| messageId: "om_topic_root", | ||
| chatId: "oc-group", | ||
| content: "thread root", | ||
| contentType: "text", | ||
| threadId: "omt_topic_img", | ||
| }); | ||
| const imgRawContent = JSON.stringify({ image_key: "img_history_key_1" }); | ||
| mockListFeishuThreadMessages.mockResolvedValue([ | ||
| { | ||
| messageId: "om_img_msg", | ||
| senderId: "ou-sender", | ||
| senderType: "user", | ||
| content: "[image message]", | ||
| contentType: "image", | ||
| rawContent: imgRawContent, | ||
| createTime: 1710000000000, | ||
| }, | ||
| ]); | ||
| mockDownloadMessageResourceFeishu.mockResolvedValueOnce({ | ||
| buffer: Buffer.from("fake-image-data"), | ||
| contentType: "image/png", | ||
| }); | ||
| mockSaveMediaBuffer.mockResolvedValueOnce({ | ||
| path: "/fake/.openclaw/media/inbound/thread-img.png", | ||
| contentType: "image/png", | ||
| }); | ||
|
|
||
| const cfg: ClawdbotConfig = { | ||
| channels: { | ||
| feishu: { | ||
| groups: { | ||
| "oc-group": { | ||
| requireMention: false, | ||
| groupSessionScope: "group_topic", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } as ClawdbotConfig; | ||
|
|
||
| const event: FeishuMessageEvent = { | ||
| sender: { sender_id: { open_id: "ou-sender" } }, | ||
| message: { | ||
| message_id: "om_text_followup", | ||
| root_id: "om_topic_root", | ||
| thread_id: "omt_topic_img", | ||
| chat_id: "oc-group", | ||
| chat_type: "group", | ||
| message_type: "text", | ||
| content: JSON.stringify({ text: "what was in that image?" }), | ||
| }, | ||
| }; | ||
|
|
||
| await dispatchMessage({ cfg, event }); | ||
|
|
||
| // The image should have been downloaded via the resource API. | ||
| expect(mockDownloadMessageResourceFeishu).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| messageId: "om_img_msg", | ||
| fileKey: "img_history_key_1", | ||
| type: "image", | ||
| }), | ||
| ); | ||
| // ThreadHistoryBody should include the attachment hint with the saved path. | ||
| expect(mockFinalizeInboundContext).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| ThreadHistoryBody: expect.stringContaining("/fake/.openclaw/media/inbound/thread-img.png"), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it("does not dispatch twice for the same image message_id (concurrent dedupe)", async () => { | ||
| mockShouldComputeCommandAuthorized.mockReturnValue(false); | ||
|
|
There was a problem hiding this comment.
The test covers the happy path (download succeeds, hint appears in ThreadHistoryBody), but the per-message error path is untested: when downloadMessageResourceFeishu rejects, the expectation is that the message still appears in history (without the attachment hint) and a log entry is emitted. Worth adding a second case that mocks a rejection and asserts the envelope is still pushed with the original [image message] body.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/bot.test.ts
Line: 2672-2751
Comment:
**Error-path not exercised by the new test**
The test covers the happy path (download succeeds, hint appears in `ThreadHistoryBody`), but the per-message error path is untested: when `downloadMessageResourceFeishu` rejects, the expectation is that the message still appears in history (without the attachment hint) and a log entry is emitted. Worth adding a second case that mocks a rejection and asserts the envelope is still pushed with the original `[image message]` body.
How can I resolve this? If you propose a fix, please make it concise.
Problem
When an image (or other media) message exists in a Feishu thread's history, the agent only receives the placeholder text
[image message]— it never sees the actual image data.Root cause:
resolveFeishuMediaListwas only called for the current inbound message. Historical thread messages fetched vialistFeishuThreadMessageswent throughparseFeishuMessageContent, which returns[image message]formsgType === "image"and similar placeholders for other media types.Solution
For each message in thread history that has a media content type (
image,file,audio,video,media,sticker,post), callresolveFeishuMediaListto download and save the media locally, then embed the saved path as a[media attached: /path (mime) | /path]hint in the message body.This matches the format the embedded agent runner already uses for current-message attachments, so no changes to the runner are needed.
Changes
extensions/feishu/src/send.ts— Added optionalrawContentfield toFeishuThreadMessageInfoand populate it fromitem.body?.contentso the raw Feishu API JSON is available for media key extraction.extensions/feishu/src/bot.ts— Changed the synchronoushistoryParts.map()to an asyncfor...ofloop that callsresolveFeishuMediaListfor media messages. Download errors are caught per-message and logged; history still renders without the attachment if download fails.extensions/feishu/src/bot.test.ts— Added a test that sends a thread history message withcontentType: "image"andrawContent, and asserts thatdownloadMessageResourceFeishuis called and the resulting path appears inThreadHistoryBody.Testing
pnpm buildpasses cleanly.pnpm check/pnpm tsgohas 3 pre-existing failures inextensions/qa-lab/src/scenario-catalog.test.tsthat are already red onmainCI (verified by checking outmainand running the same check — same errors, no change).AI Disclosure
claude-sonnet-4-6.pnpm test:extension feishupasses. Not tested against a live Feishu account (no test credentials available in this environment).