From ab2f5a077e14a0133700959d7d9ed0df02b5ee9e Mon Sep 17 00:00:00 2001
From: teknium1 <127238744+teknium1@users.noreply.github.com>
Date: Sat, 4 Jul 2026 13:58:10 -0700
Subject: [PATCH] =?UTF-8?q?fix(vision):=20address=20review=20=E2=80=94=20r?=
=?UTF-8?q?estore=20bare=20relative=20paths,=20remove=20dead=20code,=20asy?=
=?UTF-8?q?nc=20I/O?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
- resolve_image_source: bare cwd-relative filenames ('pic.png') resolve
again (main accepted them; the path-shape gate regressed them — review
by egilewski). Unknown explicit schemes (ftp://, s3://) still rejected.
- Local backend: nonexistent path now raises a clean 'image file not
found' instead of a misleading sandbox-fallback message.
- W4: remove the now-dead path-based _detect_image_mime_type (suffix-trust
SVG acceptance) so future callers can't reintroduce it.
- W3: SVG sources rejected with a dedicated actionable message + test.
- Polish: host read_bytes / temp-file write_bytes offloaded via
asyncio.to_thread (matches the container exec-read); unused
ResolveContext.cfg/extra_roots fields dropped; duplicate policy check
documented as intentional pre-flight short-circuit.
---
tests/tools/test_image_source.py | 32 ++++++++
tests/tools/test_vision_native_fast_path.py | 4 +-
tools/image_source.py | 86 +++++++++++++--------
tools/vision_tools.py | 19 +----
4 files changed, 92 insertions(+), 49 deletions(-)
diff --git a/tests/tools/test_image_source.py b/tests/tools/test_image_source.py
index 56d07d9fe42..ca93abb64d7 100644
--- a/tests/tools/test_image_source.py
+++ b/tests/tools/test_image_source.py
@@ -69,6 +69,38 @@ class TestLocalBackend:
res = await isrc.resolve_image_source(f"file://{img}", isrc.ResolveContext())
assert res.mime == "image/jpeg"
+ @pytest.mark.asyncio
+ async def test_bare_relative_path_resolves(self, tmp_path, monkeypatch):
+ """A cwd-relative bare filename ('pic.png') is a valid local source —
+ main accepted it; the resolver must not regress it (PR review)."""
+ isrc = _reload(monkeypatch, tmp_path / "hermes")
+ monkeypatch.setenv("TERMINAL_ENV", "local")
+ img = tmp_path / "pic.png"
+ img.write_bytes(PNG)
+ monkeypatch.chdir(tmp_path)
+ res = await isrc.resolve_image_source("pic.png", isrc.ResolveContext())
+ assert res.data == PNG
+ assert res.origin == "file"
+
+ @pytest.mark.asyncio
+ async def test_unknown_url_scheme_rejected(self, tmp_path, monkeypatch):
+ isrc = _reload(monkeypatch, tmp_path / "hermes")
+ monkeypatch.setenv("TERMINAL_ENV", "local")
+ with pytest.raises(isrc.UnsupportedScheme):
+ await isrc.resolve_image_source(
+ "ftp://example.com/pic.png", isrc.ResolveContext())
+
+ @pytest.mark.asyncio
+ async def test_svg_rejected_with_conversion_hint(self, tmp_path, monkeypatch):
+ """SVG has no raster magic bytes; the resolver rejects it with a
+ dedicated, actionable message (review note W3)."""
+ isrc = _reload(monkeypatch, tmp_path / "hermes")
+ monkeypatch.setenv("TERMINAL_ENV", "local")
+ svg = tmp_path / "art.svg"
+ svg.write_bytes(b'')
+ with pytest.raises(isrc.NotAnImage, match="SVG is not supported"):
+ await isrc.resolve_image_source(str(svg), isrc.ResolveContext())
+
class TestNonLocalBackendConfinement:
"""The security model: under a sandbox backend, host reads are confined to
diff --git a/tests/tools/test_vision_native_fast_path.py b/tests/tools/test_vision_native_fast_path.py
index 4423e9c9cb3..d66b52aec36 100644
--- a/tests/tools/test_vision_native_fast_path.py
+++ b/tests/tools/test_vision_native_fast_path.py
@@ -119,9 +119,9 @@ class TestVisionAnalyzeNative:
assert isinstance(result, str)
parsed = json.loads(result)
assert parsed.get("success") is False
- # Unified resolver: unrecognized/non-existent local source.
+ # Unified resolver: local backend reports a clean not-found.
err = parsed.get("error", "").lower()
- assert "not reachable" in err or "unrecognized image source" in err or "no active sandbox" in err
+ assert "image file not found" in err or "no active sandbox" in err
def test_empty_image_url_returns_error(self):
result = asyncio.get_event_loop().run_until_complete(
diff --git a/tools/image_source.py b/tools/image_source.py
index 770ffc4419e..9046577359c 100644
--- a/tools/image_source.py
+++ b/tools/image_source.py
@@ -26,11 +26,13 @@ mechanism that fixes "vision can't see container files" also closes the escape.
"""
from __future__ import annotations
+import asyncio
import base64
import os
-from dataclasses import dataclass, field
+import re
+from dataclasses import dataclass
from pathlib import Path
-from typing import Any, Optional
+from typing import Optional
# Raw-bytes INGEST budget — what the resolver will load before handing off.
# This is deliberately the 50MB download cap (tools/vision_tools._VISION_MAX_DOWNLOAD_BYTES),
@@ -70,8 +72,6 @@ class NotAnImage(ImageResolutionError):
@dataclass
class ResolveContext:
task_id: Optional[str] = None
- cfg: Optional[dict[str, Any]] = None
- extra_roots: tuple = field(default_factory=tuple)
@dataclass
@@ -81,6 +81,11 @@ class ResolvedImage:
origin: str # one of: data | http | file | local | container
+# Explicit URL scheme, e.g. "ftp://", "s3://". Bare Windows drive paths
+# ("C:\x.png") don't match because they lack the "//".
+_SCHEME_RE = re.compile(r"^[A-Za-z][A-Za-z0-9+.\-]*://")
+
+
async def resolve_image_source(src: str, ctx: ResolveContext) -> ResolvedImage:
if not isinstance(src, str) or not src.strip():
raise SourceNotFound("image_url is required", src=str(src))
@@ -94,27 +99,34 @@ async def resolve_image_source(src: str, ctx: ResolveContext) -> ResolvedImage:
raise SourceUnsafe(reason, src=s)
return _finalize(await _download_to_bytes(s), "", "http", s)
- candidate = s[len("file://"):] if s.startswith("file://") else s
- if s.startswith("file://") or _looks_like_path(candidate):
- p = Path(os.path.expanduser(candidate))
- # Confinement decision (see module docstring). Under a non-local backend
- # a path is host-readable ONLY if it lands in a media cache (after
- # translating a container-visible cache path back to its host mount);
- # every other path is read inside the sandbox via exec-read, so a host
- # path outside the caches never yields the host's bytes.
- host_target = _permitted_host_read_target(p, ctx)
- if host_target is not None and host_target.is_file():
- return _finalize(host_target.read_bytes(), "", "file", s)
- # Not a permitted host read (or the host file is absent) -> read the
- # bytes inside the sandbox. On the local backend this reaches the same
- # host file; under a sandbox it reads the container's filesystem.
- return await _resolve_container_fallback(p, ctx, s)
+ if _SCHEME_RE.match(s) and not s.lower().startswith("file://"):
+ raise UnsupportedScheme(
+ "Unrecognized image source scheme. Use an http(s) URL, a local "
+ "file path, a file:// URI, or a data: URL.",
+ src=s,
+ )
- raise UnsupportedScheme(
- "Unrecognized image source. Use an http(s) URL, a local file path, "
- "a file:// URI, or a data: URL.",
- src=s,
- )
+ # Everything else is a filesystem path — including bare relative names
+ # like "pic.png" (accepted on main; a path-shape gate here regressed them).
+ candidate = s[len("file://"):] if s.lower().startswith("file://") else s
+ p = Path(os.path.expanduser(candidate))
+ # Confinement decision (see module docstring). Under a non-local backend
+ # a path is host-readable ONLY if it lands in a media cache (after
+ # translating a container-visible cache path back to its host mount);
+ # every other path is read inside the sandbox via exec-read, so a host
+ # path outside the caches never yields the host's bytes.
+ host_target = _permitted_host_read_target(p, ctx)
+ if host_target is not None and host_target.is_file():
+ data = await asyncio.to_thread(host_target.read_bytes)
+ return _finalize(data, "", "file", s)
+ if _is_local_terminal_backend():
+ # Local backend: any path was host-readable, so a miss simply means
+ # the file doesn't exist — no sandbox to fall back to.
+ raise SourceNotFound(f"image file not found: '{p}'", src=s, origin="file")
+ # Not a permitted host read (or the host file is absent) -> read the
+ # bytes inside the sandbox. Under a sandbox this reads the container's
+ # filesystem, never the host's.
+ return await _resolve_container_fallback(p, ctx, s)
def _resolve_data_url(s: str) -> tuple[bytes, str]:
@@ -135,8 +147,12 @@ def _resolve_data_url(s: str) -> tuple[bytes, str]:
def _http_block_reason(url: str) -> Optional[str]:
"""Return a human-readable block reason, or None when the URL is allowed.
- Preserves the specific website-policy message (rather than collapsing it to
- a generic string) so the agent sees *why* a fetch was refused.
+ Pre-flight short-circuit: policy-blocked URLs are refused BEFORE any
+ network I/O. ``_download_image`` re-checks policy internally (per attempt
+ and against the final redirect target) — that second evaluation is
+ intentional, not redundant: this one guarantees no bytes move for a
+ blocked URL; the inner one covers redirects and non-resolver callers.
+ Preserves the specific website-policy message so the agent sees *why*.
"""
from tools.url_safety import is_safe_url
from tools.website_policy import check_website_access
@@ -157,16 +173,15 @@ async def _download_to_bytes(url: str) -> bytes:
with tempfile.NamedTemporaryFile(suffix=".img", delete=False) as tf:
tmp = Path(tf.name)
try:
- await _download_image(url, tmp) # enforces the 50MB stream cap + redirect SSRF guard
- return tmp.read_bytes()
+ # Enforces the 50MB stream cap, redirect SSRF guard, and website policy.
+ await _download_image(url, tmp)
+ return await asyncio.to_thread(tmp.read_bytes)
+ except PermissionError as exc: # website policy block
+ raise SourceUnsafe(str(exc), src=url, origin="http")
finally:
tmp.unlink(missing_ok=True)
-def _looks_like_path(s: str) -> bool:
- return s.startswith(("/", "~", "./", "../")) or (len(s) > 1 and s[1] == ":")
-
-
def _is_local_terminal_backend() -> bool:
"""True when the terminal backend runs directly on the host.
@@ -288,5 +303,12 @@ def _finalize(data: bytes, declared_mime: str, origin: str, src: str) -> Resolve
raise SourceTooLarge("image exceeds size limit", src=src, origin=origin)
sniffed = _detect_image_mime_type_from_bytes(data)
if sniffed is None:
+ if b" Optional[str]:
"""Magic-byte MIME sniff on raw bytes (authoritative; no extension trust).
Returns ``None`` for anything without a recognized image header — including
- SVG, which has no magic bytes and is not ingestible as vision by any
- provider. Callers that accept SVG-by-text (path-based) layer that on top.
+ SVG, which has no magic bytes and cannot be ingested as raster vision input
+ by any provider. The resolver surfaces a dedicated error for SVG sources.
"""
header = data[:64]
if header.startswith(b"\x89PNG\r\n\x1a\n"):
@@ -242,17 +242,6 @@ def _detect_image_mime_type_from_bytes(data: bytes) -> Optional[str]:
return None
-def _detect_image_mime_type(image_path: Path) -> Optional[str]:
- """Return a MIME type when the file looks like a supported image."""
- with image_path.open("rb") as f:
- mime = _detect_image_mime_type_from_bytes(f.read(64))
- if mime is None and image_path.suffix.lower() == ".svg":
- head = image_path.read_text(encoding="utf-8", errors="ignore")[:4096].lower()
- if " bool:
"""Return True only for transient image-download failures worth retrying.
@@ -871,7 +860,7 @@ async def _vision_analyze_native(
temp_dir = get_hermes_dir("cache/vision", "temp_vision_images")
temp_dir.mkdir(parents=True, exist_ok=True)
temp_image_path = temp_dir / f"temp_image_{uuid.uuid4()}.img"
- temp_image_path.write_bytes(resolved.data)
+ await asyncio.to_thread(temp_image_path.write_bytes, resolved.data)
should_cleanup = True
image_data_url = await _run_encode_on_cpu_executor(
@@ -1018,7 +1007,7 @@ async def vision_analyze_tool(
temp_dir = get_hermes_dir("cache/vision", "temp_vision_images")
temp_dir.mkdir(parents=True, exist_ok=True)
temp_image_path = temp_dir / f"temp_image_{uuid.uuid4()}.img"
- temp_image_path.write_bytes(resolved.data)
+ await asyncio.to_thread(temp_image_path.write_bytes, resolved.data)
should_cleanup = True
# Get image file size for logging