fix(feishu): fallback to original message_id for forwarded resource download - #63110
luxuguang-leo wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real missing fallback: current main returns empty values after resource-download failures in plugins/platforms/feishu/adapter.py:3893-3954.
Problems
- The new retry is only in
_download_feishu_message_resource()(PRplugins/platforms/feishu/adapter.py:3818). Images use_download_feishu_image()from thenormalized.image_keysloop (plugins/platforms/feishu/adapter.py:3784-3788on current main), so forwarded images still cannot use the fallback described in the PR. - The new
message.getcall usesasyncio.to_thread(PRplugins/platforms/feishu/adapter.py:3843). Use_run_blockinginstead: commitb296915c82c9da02bd6edacf52490e68f85e1f16moved Feishu SDK calls to the adapter-owned executor to prevent shared-executor shutdown failures. TestForwardedResourceFallbackonly tests helper return values (PRtests/gateway/test_feishu.py:4953-5045), not a failed initial download followed by a successful alternate-ID retry, nor the image path.
Suggested changes
- Share a bounded alternate-ID retry between file/media and image resource downloads, retain
_run_blocking, and add behavior tests for both paths.
Automated hermes-sweeper review.
| # When the current message_id's resource download fails, the message may | ||
| # be a forward of an original message. Try fetching the message to see | ||
| # if root_id/parent_id points to a source message, and retry with that. | ||
| alt_id = await self._resolve_forwarded_source_message_id(message_id) |
There was a problem hiding this comment.
This fallback is reachable only for normalized.media_refs; normalized.image_keys use _download_feishu_image() and still return on failure. Please apply the same bounded alternate-message retry to the image path or extract a shared helper, otherwise the PR's forwarded-image case remains broken.
| return None | ||
| try: | ||
| request = self._build_get_message_request(message_id) | ||
| response = await asyncio.to_thread(self._client.im.v1.message.get, request) |
There was a problem hiding this comment.
Please use await self._run_blocking(self._client.im.v1.message.get, request) here. Current main routes Feishu SDK calls through the adapter-owned executor after b296915 to avoid default-executor shutdown failures.
| asyncio.run(_run()) | ||
|
|
||
|
|
||
| class TestForwardedResourceFallback(unittest.TestCase): |
There was a problem hiding this comment.
Add an end-to-end downloader behavior test: make the initial resource request fail, resolve an alternate ID, and make the retry succeed. Cover both the message-resource and image download paths.
8892553 to
9385801
Compare
When a user forwards a file or image in a Feishu group chat, the resource (file_key/image_key) belongs to the original message, not the forwarded copy. Download fails because message_resource.get associates resources with the message they were first uploaded in. This adds a fallback that resolves the source message_id via message.get API and retries the download with it. - _resolve_forwarded_source_message_id: finds root_id/parent_id/ upper_message_id from the forwarded message - _retry_with_forwarded_source: shared helper called from both download paths on failure - Both _download_feishu_image and _download_feishu_message_resource now retry with the resolved source message_id Addresses all three review points from NousResearch#63110: 1. Image path now has the same fallback 2. Uses self._run_blocking() (standard pattern after b296915) 3. Full test coverage: 6 resolve + 4 retry integration tests
c068f66 to
2ac3b50
Compare
|
@teknium1 This PR has been updated addressing all three review points. E2E verified on live Feishu — forwarded images and PDFs now download correctly. Would appreciate a re-review when you get a chance. |
What does this PR do?
When a user forwards a file or image in a Feishu group chat, the resource (
file_key/image_key) belongs to the original message, not the forwarded copy. The download always fails because Feishu'smessage_resource.getAPI associates resources with the message they were first uploaded in.This adds a fallback: when the initial download fails, resolve the source
message_idviamessage.getAPI (checkingroot_id→parent_id→upper_message_id) and retry the download with it.Both download paths are covered:
_download_feishu_image) — retries with resolved source message_id_download_feishu_message_resource) — same fallbackA shared helper
_retry_with_forwarded_sourceprevents code duplication.Changes Made
adapter.py(+77 lines):_resolve_forwarded_source_message_id()— callsmessage.getvia_run_blocking(), extractsroot_id→parent_id→upper_message_idas source_retry_with_forwarded_source()— shared helper called from both download paths on failure_download_feishu_image— retries with resolved source when initial download fails_download_feishu_message_resource— retries with resolved source when all request types failtests/gateway/test_feishu.py(+116 lines):_resolve_forwarded_source_message_id(root_id, parent priority, no source, self-ref, API failure)_retry_with_forwarded_source(image fallback, file fallback, no source, self-ref)Review Comments Addressed
This PR fixes all three review points from the previous iteration:
_download_feishu_image) had no fallback — only file path did_retry_with_forwarded_sourceasyncio.to_thread()instead of adapter's_run_blocking()await self._run_blocking(self._client.im.v1.message.get, request)E2E Verification
Tested on a live Feishu group chat with
require_mention: false:_download_feishu_imagefails → fallback resolves source ID → retry succeeds → image cached locally ✅_download_feishu_message_resourcefails → fallback resolves source ID → retry succeeds → PDF extracted and readable ✅All 10 unit/integration tests pass:
How to Test
python3 -m pytest tests/gateway/test_feishu.py -q -k "forwarded or resolve_forwarded or retry_with"Checklist