fix(vision): cap browser_vision native embeds (full-page screenshots OOM local models, wedge Anthropic sessions) - #63850
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Good fix. Vision caps for native embeds prevent oversized full-page screenshots from silently blowing context budget. Test coverage at both the browser-tool and native-fast-path levels confirms the cap is enforced at every entry point that contributes to the embed.
Strengths
- Cap is symmetric across
browser_visionand the native fast path — single boundary, no double-bypass. - Tests assert pixel/dimension limits, not arbitrary counts, so the invariant survives future intent changes.
Light follow-up (non-blocking)
- Consider documenting the cap value in a config constant so it's discoverable, not just enforced.
Reviewed by Hermes Agent in batch mode
|
Thanks for the review! On the non-blocking follow-up: the cap values are module constants with full rationale docs — |
…l-page screenshots OOM local models and wedge Anthropic sessions] browser_vision embedded full-page screenshots into conversation history at full resolution on the native vision fast path, despite its comment claiming consistency with vision_analyze — which has applied the proactive embed caps (4 MB / 7900 px) since the wedged-session incident, precisely because history-embedded images are re-sent on every turn. A full-page capture is arbitrarily tall, so uncapped embeds accumulate until they wedge Anthropic sessions with non-retryable 400s or OOM local vision models during prefill (observed in the wild: four consecutive Metal kIOGPUCommandBufferCallbackErrorOutOfMemory crashes on an MLX VLM as retries re-sent the same oversized context). Extract the cap logic into a shared _apply_embed_caps helper so both _build_native_vision_tool_result call sites enforce identical limits: vision_analyze rewires to the helper (semantics-preserving, guarded by its existing oversized-embed regression test) and browser_vision now caps before embedding. The aux path is untouched — it sends the full- resolution image once, outside history, with its own reactive resize. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
26d19a5 to
98d3939
Compare
|
Thanks for the focused fix. Current main still takes
Automated hermes-sweeper review. |
SummarySeventeen PRs touch this browser-vision complex: #11752 addresses remote screenshot persistence; #19647, #19765, #20118, and #21833 make full-page capture optional; #39226, #44922, #47468, #47551, and #63850 add native-embed caps; the others concern media handling, vision routing, native fast paths, or numeric configuration rather than the reported capture or embed causes. Related pull requests
DuplicatesViewport-control family: #19647, #19765, #20118, and #21833, with closed #20118 the most complete reference and #21833 differing mainly on default policy. Embed-cap family: #39226, #44922, #47468, #47551, and #63850; #44922 and #47468 are documented duplicates of #39226, while #47551 is a narrower byte-only duplicate. Routing lineage: #29987 was superseded by merged #34562; #24872 and #24875 compete on #24842; #51978 and #51981 are backend-specific halves of the same finite-config bug class. Suggested consolidationKeep #47468 open with a salvage path: rebase it onto current main, preserve its byte-and-dimension trigger, incorporate #63850's shared helper and real-image tests, and require verified post-resize compliance with both _EMBED_TARGET_BYTES and _EMBED_MAX_DIMENSION before native attachment. After that validation lands in #47468, #39226, #44922, #47551, and #63850 can be closed as duplicates; this explicitly departs from the keep_open reviews on #39226, #44922, and #63850 only because their visible diffs implement the same cap path while none closes the post-resize validation gap, so their unique tests and helper work must first be preserved in #47468. Complex graphflowchart TD
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I19620(["issue #19620 (open)"])
I47467(["issue #47467 (open)"])
I54371(["issue #54371 (open)"])
subgraph Dup39226 ["PRs duplicating each other"]
P39226["PR #39226 (open)"]
P44922["PR #44922 (open)"]
P47468["PR #47468 (open)"]
P47551["PR #47551 (closed)"]
P63850["PR #63850 (open)"]
end
P63850 -.->|partial| I19620
P63850 -.->|partial| I47467
P63850 -.->|partial| I54371
class I19620 open
class I47467 open
class I54371 open
class P39226 open
class P44922 open
class P47468 open
class P47551 closed
class P63850 open
class P47468 best
class P63850 target
click I19620 "https://github.com/NousResearch/hermes-agent/issues/19620"
click I47467 "https://github.com/NousResearch/hermes-agent/issues/47467"
click I54371 "https://github.com/NousResearch/hermes-agent/issues/54371"
click P39226 "https://github.com/NousResearch/hermes-agent/pull/39226"
click P44922 "https://github.com/NousResearch/hermes-agent/pull/44922"
click P47468 "https://github.com/NousResearch/hermes-agent/pull/47468"
click P47551 "https://github.com/NousResearch/hermes-agent/pull/47551"
click P63850 "https://github.com/NousResearch/hermes-agent/pull/63850"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 17 pull requests and 5 issues in this complex. Each diff was read against this issue; Assessment working set: 150 kB of PR diffs, 43 kB of issue/PR text, 17 kB of discussion (32 comments), 38 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Heads up for a maintainer: this PR's CI run has been stuck in |
What does this PR do?
Applies the proactive embed caps (4 MB / 7900 px) to
browser_vision's native vision fast path. When the main model supports vision,browser_visionembeds its full-page screenshot into conversation history at full resolution — despite the code comment claiming "Consistent with vision_analyze", which has capped its embeds since the wedged-session incident (tools/vision_tools.py:548-566explains why: history-embedded images are re-sent every turn, so an oversized embed permanently wedges Anthropic sessions with non-retryable 400s). A full-page capture is arbitrarily tall, making this the worst-case producer of oversized embeds.Real-world evidence: on a 36 GB Apple Silicon machine running an MLX VLM (qwen3.6-27b via LM Studio), accumulated
browser_visionscreenshots at ~50k-token context crashed the model four consecutive times as retries re-sent the same context:Related Issue
Related: #63849 (the remaining accumulation half of the problem — capped screenshots still pile up on the OpenAI-compatible path; filed as a design question because eviction interacts with the prompt-caching invariant).
Type of Change
Changes Made
tools/vision_tools.py— extract the cap logic into a shared_apply_embed_caps(image_path, image_data_url, mime_type)helper (identity passthrough under both caps; resize via the existing_resize_image_for_vision; returns an error string instead of embedding when even resizing can't get under the 20 MB hard ceiling)._vision_analyze_nativerewires to the helper — semantics-preserving, net-negative diff, guarded by the existingtest_oversized_image_resized_under_embed_capregression test.tools/browser_tool.py— the native fast path now caps the screenshot before embedding, scoped strictly inside the_should_use_native_vision_fast_path()branch. The auxiliary path is byte-identical: it sends the full-resolution image once, outside history, and keeps its existing reactive resize-and-retry.tests/tools/test_vision_native_fast_path.py— newTestApplyEmbedCaps: oversized-by-bytes → resized under 4 MB; oversized-by-dimension (200×9000 tall page, tiny bytes) → longest side ≤ 7900 px; under-cap image → identity passthrough (no recompression).tests/tools/test_browser_vision_embed_caps.py(new) — integration through the realbrowser_visionwith only the browser daemon faked: oversized screenshot embeds under the cap; tiny screenshot embeds byte-for-byte at full resolution (guards over-eager capping).How to Test
browser_visionon a long page (full-page screenshot > 4 MB or > 7900 px tall).scripts/run_tests.sh tests/tools/test_vision_native_fast_path.py tests/tools/test_browser_vision_embed_caps.py tests/tools/test_vision_tools.py→ 115 passed.scripts/run_tests.sh tests/tools/test_browser*→ 488 passed.Checklist
Code
fix(vision): …)scripts/run_tests.sh, the CI-parity wrapper)Documentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/A (no config keys changed)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/AScreenshots / Logs
🤖 Generated with Claude Code