Skip to content

fix: stream vision downloads before size checks - #10440

Closed
WuKongAI-CMU wants to merge 1 commit into
NousResearch:mainfrom
WuKongAI-CMU:fix/vision-download-stream-size-cap
Closed

fix: stream vision downloads before size checks#10440
WuKongAI-CMU wants to merge 1 commit into
NousResearch:mainfrom
WuKongAI-CMU:fix/vision-download-stream-size-cap

Conversation

@WuKongAI-CMU

@WuKongAI-CMU WuKongAI-CMU commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stream remote vision downloads into a temporary file instead of buffering response.content
  • enforce _VISION_MAX_DOWNLOAD_BYTES chunk-by-chunk and delete partial files on failure
  • keep Content-Length as an early rejection, but ignore malformed values and rely on the streaming cap

Tests

  • source venv/bin/activate && pytest tests/tools/test_vision_tools.py -q
  • source venv/bin/activate && python -m compileall tools/vision_tools.py tests/tools/test_vision_tools.py
  • git diff --check

@alt-glitch alt-glitch added type/perf Performance improvement or optimization P2 Medium — degraded but workaround exists tool/vision Vision analysis and image generation labels Apr 26, 2026
Intern Dev @WuKongAI-CMU
fix: stream vision downloads before size checks
667b4e3 
The vision analyzer accepts arbitrary remote image URLs, so the size cap has to apply before response bodies are buffered in memory. The downloader now streams into a temporary file, counts bytes as chunks arrive, removes partial files on failure, and still uses Content-Length as an early rejection when it is valid.

Constraint: Remote image servers may omit or lie about Content-Length.

Rejected: Trust Content-Length plus len(response.content) | response.content buffers the full body before the actual cap can run.

Confidence: high

Scope-risk: narrow

Tested: source venv/bin/activate && pytest tests/tools/test_vision_tools.py -q

Tested: source venv/bin/activate && python -m compileall tools/vision_tools.py tests/tools/test_vision_tools.py

Tested: git diff --check

Not-tested: Live vision download against external CDNs.
@WuKongAI-CMU
WuKongAI-CMU force-pushed the fix/vision-download-stream-size-cap branch from 4c1f30f to 667b4e3 Compare April 27, 2026 03:33
@WuKongAI-CMU

WuKongAI-CMU commented Apr 27, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current upstream main (cb51baec) and force-pushed 667b4e3d.

Validation:

  • pytest tests/tools/test_vision_tools.py -q -> 66 passed, 6 skipped

@alt-glitch alt-glitch mentioned this pull request May 20, 2026
Closed
@alt-glitch alt-glitch mentioned this pull request Jul 7, 2026
Closed
teknium1
teknium1 reviewed Jul 12, 2026
View reviewed changes

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused image-path fix. The reported image issue is present on current main: tools/vision_tools.py:435 performs client.get(), and the effective cap is only checked after response.content at :457-460.

Problems

  • The same unbounded-buffer pattern remains in _download_video() at tools/vision_tools.py:1583-1608. A complete remote-vision media fix should cover that sibling path as well; #55028 is tracking an equivalent video change.
  • Current-main retry tests still mock and assert client.get() in tests/tools/test_vision_tools.py:1093-1158. Salvage needs to migrate those tests to a client.stream() async-context-manager mock.
  • The added tests are mock-only. AGENTS.md:84-87 calls for real-path coverage of file/network I/O; add a local streaming-server regression test for a chunked over-cap response and partial-file cleanup.

The streaming loop and temporary-file replacement in PR head 667b4e3d:tools/vision_tools.py:179-236 are directionally sound for the image path. This is an automated hermes-sweeper review.

Comment thread tools/vision_tools.py
event_hooks={"response": [_ssrf_redirect_guard]},
) as client:
response = await client.get(
async with client.stream(

@teknium1 teknium1 Jul 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main has retry-classification coverage at tests/tools/test_vision_tools.py:1093-1158 that mocks client.get() and asserts get.await_count. When salvaging this switch to client.stream(), update that helper and its assertions to use an async stream context manager; otherwise the current retry tests no longer exercise the intended status-error path.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/streaming Streaming responses: gateway delivery, provider wire labels Jul 12, 2026
@kshitijk4poor

kshitijk4poor commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Closing as stale — the PR is 14923 commits behind main and cannot be cherry-picked. The three-dot diff reverts 32 commits of vision_tools.py evolution, including:

  • create_ssrf_safe_async_client() → raw httpx.AsyncClient (drops connect-time DNS SSRF guard)
  • _ssrf_redirect_guard → vulnerable next_request-only version (reintroduces redirect SSRF bypass)
  • _is_retryable_download_error() → blind retry of 4xx (404 now takes 14s instead of failing fast)
  • Lazy auxiliary client import → eager import (50ms cold-start regression)

The core streaming approach is correct and we'll implement it fresh on current main, applying the streaming + chunk-by-chunk size cap to both _download_image() and the sibling _download_video() (same buffering bug). Credit to @WuKongAI-CMU for the approach — the temp-file-with-atomic-replace and running bytes_written cap pattern is the right design.

Thanks for the PR!

@kshitijk4poor kshitijk4poor mentioned this pull request Aug 7, 2026
Merged
kshitijk4poor added a commit that referenced this pull request Aug 7, 2026
@kshitijk4poor
fix(vision): stream image and video downloads with chunk-by-chunk siz…
b7eb97a 
…e cap

_download_image() and _download_video() both used client.get() +
response.content, buffering the entire media body into memory before
checking the size cap. A server that omits Content-Length could send
an arbitrarily large payload, causing OOM.

Extract _stream_download_to_file() shared helper: streams via
client.stream() + aiter_bytes(), writes chunks to a temp file, enforces
the running byte count against the cap after each chunk, and atomically
replaces onto the destination on success. Cleans up the temp file on
failure. Uses utils.atomic_replace() for cross-device/symlink safety.

Malformed Content-Length values are now caught and ignored instead of
crashing with ValueError; the streaming cap is the authoritative guard.

Approach adapted from PR #10440 by @WuKongAI-CMU (closed as stale —
14923 commits behind, reverted 32 commits of vision_tools.py evolution
including SSRF-safe client, retry classification, and lazy imports).

Closes #10440
ma1138569845 pushed a commit to ma1138569845/dechnicAuditor-agent that referenced this pull request Aug 10, 2026
@kshitijk4poor @ma1138569845
fix(vision): stream image and video downloads with chunk-by-chunk siz…
dc05eca 
…e cap

_download_image() and _download_video() both used client.get() +
response.content, buffering the entire media body into memory before
checking the size cap. A server that omits Content-Length could send
an arbitrarily large payload, causing OOM.

Extract _stream_download_to_file() shared helper: streams via
client.stream() + aiter_bytes(), writes chunks to a temp file, enforces
the running byte count against the cap after each chunk, and atomically
replaces onto the destination on success. Cleans up the temp file on
failure. Uses utils.atomic_replace() for cross-device/symlink safety.

Malformed Content-Length values are now caught and ignored instead of
crashing with ValueError; the streaming cap is the authoritative guard.

Approach adapted from PR NousResearch#10440 by @WuKongAI-CMU (closed as stale —
14923 commits behind, reverted 32 commits of vision_tools.py evolution
including SSRF-safe client, retry classification, and lazy imports).

Closes NousResearch#10440
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
@kshitijk4poor
fix(vision): stream image and video downloads with chunk-by-chunk siz…
9f53399 
…e cap

_download_image() and _download_video() both used client.get() +
response.content, buffering the entire media body into memory before
checking the size cap. A server that omits Content-Length could send
an arbitrarily large payload, causing OOM.

Extract _stream_download_to_file() shared helper: streams via
client.stream() + aiter_bytes(), writes chunks to a temp file, enforces
the running byte count against the cap after each chunk, and atomically
replaces onto the destination on success. Cleans up the temp file on
failure. Uses utils.atomic_replace() for cross-device/symlink safety.

Malformed Content-Length values are now caught and ignored instead of
crashing with ValueError; the streaming cap is the authoritative guard.

Approach adapted from PR NousResearch#10440 by @WuKongAI-CMU (closed as stale —
14923 commits behind, reverted 32 commits of vision_tools.py evolution
including SSRF-safe client, retry classification, and lazy imports).

Closes NousResearch#10440
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
@melon-xf
fix(vision): stream image and video downloads with chunk-by-chunk siz…
235f3f0 
…e cap

_download_image() and _download_video() both used client.get() +
response.content, buffering the entire media body into memory before
checking the size cap. A server that omits Content-Length could send
an arbitrarily large payload, causing OOM.

Extract _stream_download_to_file() shared helper: streams via
client.stream() + aiter_bytes(), writes chunks to a temp file, enforces
the running byte count against the cap after each chunk, and atomically
replaces onto the destination on success. Cleans up the temp file on
failure. Uses utils.atomic_replace() for cross-device/symlink safety.

Malformed Content-Length values are now caught and ignored instead of
crashing with ValueError; the streaming cap is the authoritative guard.

Approach adapted from PR NousResearch#10440 by @WuKongAI-CMU (closed as stale —
14923 commits behind, reverted 32 commits of vision_tools.py evolution
including SSRF-safe client, retry classification, and lazy imports).

Closes NousResearch#10440
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@teknium1 teknium1 teknium1 left review comments

Assignees

No one assigned

Labels

area/streaming Streaming responses: gateway delivery, provider wire P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/vision Vision analysis and image generation type/perf Performance improvement or optimization

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@WuKongAI-CMU @kshitijk4poor @teknium1 @alt-glitch