test(line): regression coverage for typed media-cache routing - #35785
test(line): regression coverage for typed media-cache routing#35785jethac wants to merge 2 commits into
Conversation
|
@alt-glitch Thanks for flagging this as a duplicate! I’ve reviewed the previous attempts (#34233, #27142). My implementation builds on those by explicitly ensuring backward compatibility—preserving the legacy image-cache aliases so we don't break the rest of the gateway or existing tests. It also enforces filesystem strictness by segregating generic media from images, which I found was required to pass the current e2e test suite. Would love your feedback on whether this path is consistent with the current architectural direction. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
🔍 What this does
Two fixes in the LINE platform adapter:
- Message type mapping: Replaces the binary
MessageType.IMAGE if msg_type == "text" else MessageType.IMAGEbug (which crashed on voice/video/image) with a proper dict-based mapping (text→TEXT,image→PHOTO,audio→VOICE,video→VIDEO). - Media caching: Introduces
cache_media_from_bytes()/cache_media_from_url()as generalized media-caching utilities, refactoringcache_image_from_bytes()/cache_image_from_url()to delegate to them. LINE adapter now usescache_media_from_bytes()for non-image media.
✅ Looks Good
- Correctness of type mapping: The dict-based approach is clean and handles all LINE message types. Falls back to
MessageType.TEXTfor unknown types — safe default. - Refactoring quality: The generalization from
cache_image_from_bytes→cache_media_from_bytesis well-structured. Backward compat aliases preserve all existing callers. Theis_imageflag differentiatescache/imagesvscache/mediasubdirectories. - Edge cases: The
cache_image_from_bytesstill validates image data via_looks_like_image()before delegating, preserving existing safety checks. - LINE adapter: Proper use of
cache_media_from_bytes(data, ext=ext, is_image=(msg_type == "image"))correctly routes image vs audio/video to the right cache dir.
💡 Suggestion (non-blocking)
Consider moving the inner import uuid and from hermes_constants imports in cache_media_from_bytes to module-level imports for readability. The current inline imports avoid circular deps but add cognitive overhead.
Reviewed by Hermes Agent (cron)
724eb8e to
f082ec7
Compare
f082ec7 to
0c99e9d
Compare
0c99e9d to
9374f6f
Compare
|
Rebased this PR onto current Additional cleanup in this update:
Local verification: uv run --with pytest --with aiohttp python -m pytest \
tests/gateway/test_media_download_retry.py \
tests/gateway/test_line_plugin.py \
-q -o 'addopts='
# 115 passed |
9fd7ab5 to
44e6dea
Compare
|
Quick follow-up on the attribution change: CI flagged Since I’m leaving Google and want future attribution tied to my personal account, I rewrote this PR stack to use No runtime code changed from that attribution-only rewrite; targeted tests were re-run after the rebase. The three PRs remain stacked locally: #35785 base, #40931 on top, #40933 on top. |
95ac1d4 to
573b156
Compare
573b156 to
63a1998
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for preserving image validation while addressing a real LINE media path issue. Current main already contains the MessageType mapping fix from 7309f3bef (plugins/platforms/line/adapter.py:141-149); the remaining defect is that _download_media() still sends audio, video, and files through cache_image_from_bytes() at plugins/platforms/line/adapter.py:1070.
Problems
- The new
cache_media_from_bytes/cache_media_from_urlAPI duplicates existing cache infrastructure: typed helpers atgateway/platforms/base.py:826,:933, and:1553, plus the existing classifier/routercache_media_bytes()at:1651. - The added tests cover the new URL helper, but the LINE production path calls the bytes helper.
tests/gateway/test_line_plugin.py:650-676currently tests only type mapping and does not cover_download_media().
Suggested changes
- Re-scope the salvage to route LINE payloads through the existing typed helpers (or
cache_media_bytes()), retaining the established cache directories and validation behavior. - Add mocked LINE download regressions for audio, video, and file messages.
Automated hermes-sweeper review.
| @@ -1067,7 +1068,7 @@ async def _download_media(self, message_id: str, msg_type: str) -> Optional[str] | |||
| "file": ".bin", | |||
| }.get(msg_type, ".bin") | |||
| try: | |||
There was a problem hiding this comment.
gateway/platforms/base.py already provides cache_audio_from_bytes (line 826), cache_video_from_bytes (line 933), cache_document_from_bytes (line 1553), and cache_media_bytes (line 1651). Please dispatch this method through those established helpers instead of introducing a parallel generic cache API; that preserves the existing cache contracts and avoids an otherwise-unused generic URL helper.
There was a problem hiding this comment.
Done in 05a13e2 — the parallel cache_media_from_bytes/cache_media_from_url API is removed entirely (gateway/platforms/base.py is now byte-identical to main) and _download_media() dispatches directly on the LINE message type: image → cache_image_from_bytes, audio → cache_audio_from_bytes, video → cache_video_from_bytes, file → cache_document_from_bytes (threading LINE's fileName through, falling back to line_file.bin). I went with direct typed dispatch rather than cache_media_bytes() since LINE's content API already fixes the format per type — no classification needed — and it preserves the raw cache-path contract this adapter surfaces. Nine new mocked regressions in tests/gateway/test_line_plugin.py now cover the production path per type plus fetch/cache failure modes.
63a1998 to
6e02e4d
Compare
|
Thanks for the review — re-scoped exactly as suggested; specifics are in the inline reply. Rebased onto current main, so the MessageType-mapping half (already absorbed via 7309f3b) has dropped out of the diff; what remains is the real defect fix — audio/video/file payloads no longer round-trip through |
…35785 itself Bisected commit-by-commit: this line survives cleanly through the author-map/flake-fix/multi-agent merges and disappears exactly at merging fix/line-media-adapter. Confirmed via diff against that PR branch's own merge-base that the deletion is baked into the PR's own commit history (unrelated to its actual LINE-media-caching purpose -- almost certainly incidental collateral from however that PR's author-map commit was originally assembled), not an artifact of this merge. No PR has any legitimate reason to remove this mapping, so restoring it here. (The duplicate "jethachan@gmail.com" entry, by contrast, is harmless: two of jethac's own PRs -- NousResearch#63560 and NousResearch#35785 -- each independently add the identical line/comment, consistent with ~75 other pre-existing duplicate keys already in this file on upstream/main. Left as-is.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
6e02e4d to
4931c88
Compare
Rebase note: upstream 73e193c ("fix(line): normalize inbound media types and cache routing") independently shipped a superset of this branch's adapter fix — typed cache helper dispatch, fileName threading, and a (path, media_type) return from _download_media. The adapter changes are therefore dropped in favor of upstream's version. What remains from the original commit: the mocked regression tests covering _download_media routing for all four LINE content types, cache/fetch failure fallbacks, and fileName threading from the message event — adapted to upstream's keyword-only ``filename=`` parameter and tuple return, and to media_types now carrying MIME types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Map the contributor email used on the LINE media PR stack to the jethac GitHub handle, replacing the stale legacy-map entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4931c88 to
a18ce7e
Compare
Summary
Scope change (2026-07-29): upstream
73e193c03independently shipped a superset of this PR's original adapter fix (typed media-cache dispatch,fileNamethreading,(path, media_type)tuple returns), so the code changes here were dropped in the rebase. What remains — and what this PR now contributes — is the regression test coverage upstream doesn't have:TestDownloadMediaRouting: locks in typed media-cache routing for the LINE adapter — text/image/audio/video messages map to their correctMessageTypeenums and cached media resolves through the keyword-onlyfilename=/(path, media_type)tuple signatures, with MIME-typedmedia_types.AttributeError: IMAGEon non-text messages), so it can't regress silently.Original motivation (fixed upstream, now covered by these tests): the adapter crashed on any non-text message and mis-cached media by treating all downloads as untyped blobs.
Test evidence
tests/gateway/test_line_plugin.py+ contributor-map suite: 113 passed on current main (f75b577b9).