Feishu: include pending history media (images) in inbound context#54967
Feishu: include pending history media (images) in inbound context#54967xuruiray wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds best-effort inclusion of media (e.g. screenshots posted before a bot mention) from pending group-history entries into the inbound agent context for Feishu group chats.\n\n- Confidence Score: 5/5Safe to merge; the change is additive, best-effort, and falls back gracefully on any error. All paths are bounded (3 candidates, 10 items max), every resolution failure is caught and logged, the deduplication is correct, and no existing behavior is altered when the feature isn't triggered. The only issue found is a trivially unnecessary optional-chain guard. No files require special attention.
|
| Filename | Overview |
|---|---|
| extensions/feishu/src/bot.ts | Adds best-effort resolution of media from pending group-history entries before bot dispatch. Logic is correctly gated, properly bounded (3 candidates / 10 items), and each resolution is wrapped in try/catch. One minor style nit: optional-chain guard on a non-nullable type. |
| extensions/feishu/src/send.ts | Adds rawContent to the object returned by parseFeishuMessageItem. Simple, correct — the variable was already captured; this just exposes it on the returned struct. |
| extensions/feishu/src/types.ts | Adds rawContent?: string to FeishuMessageInfo with clear JSDoc distinguishing it from the parsed content field. Clean and well-documented change. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/feishu/src/bot.ts
Line: 806-817
Comment:
**Unnecessary optional chaining on non-nullable type**
`m` is of type `FeishuMediaInfo` (non-nullable, drawn from `FeishuMediaInfo[]`), and `path` is a required `string` field on that type. The `m?.path` optional chain and the associated `if (!m?.path)` guard are therefore dead code — `m` can never be nullish here.
```suggestion
mediaList = merged.filter((m) => {
if (!m.path) {
return false;
}
if (seen.has(m.path)) {
return false;
}
seen.add(m.path);
return true;
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feishu: include pending history media (i..." | Re-trigger Greptile
| } catch (err) { | ||
| log( | ||
| `feishu[${account.accountId}]: failed to resolve pending history media for ${messageId}: ${String(err)}`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (historyMediaList.length > 0) { | ||
| // Prefer history media first so placeholders in history align with MediaPaths order. | ||
| const merged = [...historyMediaList, ...mediaList]; | ||
| const seen = new Set<string>(); | ||
| mediaList = merged.filter((m) => { |
There was a problem hiding this comment.
m is of type FeishuMediaInfo (non-nullable, drawn from FeishuMediaInfo[]), and path is a required string field on that type. The m?.path optional chain and the associated if (!m?.path) guard are therefore dead code — m can never be nullish here.
| } catch (err) { | |
| log( | |
| `feishu[${account.accountId}]: failed to resolve pending history media for ${messageId}: ${String(err)}`, | |
| ); | |
| } | |
| } | |
| if (historyMediaList.length > 0) { | |
| // Prefer history media first so placeholders in history align with MediaPaths order. | |
| const merged = [...historyMediaList, ...mediaList]; | |
| const seen = new Set<string>(); | |
| mediaList = merged.filter((m) => { | |
| mediaList = merged.filter((m) => { | |
| if (!m.path) { | |
| return false; | |
| } | |
| if (seen.has(m.path)) { | |
| return false; | |
| } | |
| seen.add(m.path); | |
| return true; | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/bot.ts
Line: 806-817
Comment:
**Unnecessary optional chaining on non-nullable type**
`m` is of type `FeishuMediaInfo` (non-nullable, drawn from `FeishuMediaInfo[]`), and `path` is a required `string` field on that type. The `m?.path` optional chain and the associated `if (!m?.path)` guard are therefore dead code — `m` can never be nullish here.
```suggestion
mediaList = merged.filter((m) => {
if (!m.path) {
return false;
}
if (seen.has(m.path)) {
return false;
}
seen.add(m.path);
return true;
});
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f8ccc8d80
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .filter((entry) => | ||
| Boolean(entry.messageId) && /<media:|!\[image\]/.test(String(entry.body ?? "")), | ||
| ) |
There was a problem hiding this comment.
Match pending image entries without placeholder text
When requireMention is enabled, pending history entries are recorded from ctx.content, and for Feishu image messages that content is the raw JSON body (for example with image_key), not <media:...> or ![image]. This filter therefore misses the common “image sent before mention” path, so the new history-media enrichment silently skips those screenshots and the inbound media context is still incomplete in the primary scenario this commit is trying to fix.
Useful? React with 👍 / 👎.
| historyMediaList.push(...resolved); | ||
| if (historyMediaList.length >= 10) { | ||
| break; |
There was a problem hiding this comment.
Apply media-item cap before bulk-resolving a message
The 10-item guard runs only after historyMediaList.push(...resolved), but resolveFeishuMediaList already downloads all media in that message (notably all embedded media in post content) before returning. In a group with a pending rich post containing many images, this can still download far more than 10 files and cause the latency spike this change explicitly tries to bound.
Useful? React with 👍 / 👎.
b773278 to
b2e9d51
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2e9d512f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (entry) => | ||
| Boolean(entry.messageId) && /<media:|!\[image\]/.test(String(entry.body ?? "")), | ||
| ) |
There was a problem hiding this comment.
Match pending media candidates by Feishu payload keys
This filter only selects pending entries whose rendered history text contains <media: or ![image], but pending group history is recorded from ctx.content, and for Feishu image/file/audio events that value is typically raw JSON (for example containing image_key/file_key). In mention-required groups, the common “media first, mention later” flow is therefore skipped, so the new history-media enrichment misses the exact pending screenshots/files it is intended to attach.
Useful? React with 👍 / 👎.
| historyMediaList.push(...resolved); | ||
| if (historyMediaList.length >= 10) { | ||
| break; |
There was a problem hiding this comment.
Enforce history media cap before resolving each message
The 10-item guard runs only after historyMediaList.push(...resolved), but resolveFeishuMediaList has already downloaded all media for that message (including every embedded asset in a rich post) before returning. In practice, one pending post with many attachments can still trigger far more than 10 downloads, so the intended latency/cost bound is not actually enforced.
Useful? React with 👍 / 👎.
|
Codex review: needs changes before merge. Summary Reproducibility: yes. Source inspection gives a high-confidence path: a mention-required Feishu group buffers a screenshot into pending history, then a bare bot mention arrives with no triggering-message media, and the proposed history-media block is reached too late. Next step before merge Security Review findings
Review detailsBest possible solution: Land a Feishu-owned fix that preserves rawContent, resolves bounded pending-history media before skip and audio decisions, enforces caps before downloads, adds focused regressions and a changelog entry, and coordinates with #66432 only where a shared helper is useful. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection gives a high-confidence path: a mention-required Feishu group buffers a screenshot into pending history, then a bare bot mention arrives with no triggering-message media, and the proposed history-media block is reached too late. Is this the best way to solve the issue? No, not yet. The Feishu plugin is the right boundary and rawContent plumbing is useful, but the patch needs earlier media resolution, pre-download cap enforcement, and post-merge audio index handling. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4556707cb7d8. |
Problem
In Feishu group chats, users often post screenshots before mentioning the bot. Those screenshot messages are buffered as pending history (text placeholders like
![image]/<media:image>), but the bot does not include the actual media in the inbound agent context.Changes
FeishuMessageInfoto preserverawContentreturned by Feishu message APIs.Notes