Skip to content

feat(viking): auto-return image bytes from viking_read for vision models - #356

Open
Leoyzen wants to merge 2 commits into
mainfrom
feat/viking-image-vision-support
Open

feat(viking): auto-return image bytes from viking_read for vision models#356
Leoyzen wants to merge 2 commits into
mainfrom
feat/viking-image-vision-support

Conversation

@Leoyzen

@Leoyzen Leoyzen commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

概述

为 Viking 能力增加图片资源返回支持:当模型具备视觉能力(或 support_vision 显式开启)时,viking_read 对图片 URI 返回 BinaryImage 字节;否则返回文本 URI 说明(安全降级)。

背景问题

viking_read 通过 client.read() 读取图片时会把二进制字节按文本解码成乱码(utf-8→gbk→latin-1→replace)。模型无法理解图片内容。

改动

  1. 配置层 (agentpool_config/capabilities.py):VikingCapabilityConfig 新增三态开关 support_vision: bool | None = None
    • True — 强制返回图片字节
    • False — 强制返回文本 URI
    • None(默认)— 由模型能力自动判断
  2. Capability 层 (capabilities/viking/):
    • 新增 constants.py:与 openviking 服务器一致的 IMAGE_EXTENSIONS 集合 + MIME 映射(需手动同步服务器 parse/parsers/media/constants.py)
    • 新增 _should_return_image_bytes():三态决策,NNone 语义文档化(与 ModalityFilterCapability 的 pass-through 语义区分——Viking 未注入时按 text-only 安全降级)
  3. Agent factory 注入 (agents/native_agent/agent.py):capability 注入循环中为 VikingCapability 填充 model_capabilities(与 ModalityFilter 同模式,非自动注入)
  4. 工具层 (tools.py viking_read):按扩展名判定图片 → download_bytesBinaryImage;.svg 矢量格式永不返回字节(降级文本);混合 URI 列表按序处理;下载失败返回 viking_read error: 文本
  5. SDK:openviking-sdk 0.1.5 → 0.1.6(提供 AsyncHTTPClient.download_bytes)

测试

  • _should_return_image_bytes 三态矩阵(6 用例)
  • viking_read 图片分支 9 mock 场景(True/False/None 自动、非图片忽略、SVG 降级、混合 URI、下载错误、MIME 映射)
  • factory 注入 4 用例(vision→bytes、text-only→URI、无模型名→text-only、for_run 保留 + bridge 联动)
  • ModelCapabilities per-run copy 保留断言

备注

  • tasks 4.3.8(与 ModalityFilter 双重机制交互)与 6.5(真实服务器 e2e)留待集成环境验证
  • IMAGE_EXTENSIONS 与 openviking 服务器手动同步(见 design.md 风险矩阵)

OpenSpec change: openspec/changes/viking-image-vision-support/(本地规划文件,不进 git)

Add tri-state support_vision config (True=force bytes, False=force text,
None=auto-detect from model capabilities) to VikingCapability. The agent
factory now populates VikingCapability.model_capabilities with resolved
model capabilities, and viking_read detects image URIs by extension to
return BinaryImage parts (or text URI hints) accordingly.

- agentpool_config: support_vision field on VikingCapabilityConfig
- capabilities/viking: constants.py (IMAGE_EXTENSIONS/MIME), _should_return_image_bytes()
- tools.py: image branch in viking_read with SVG text-only degradation
- agent.py: populate viking cap model_capabilities in factory injection loop
- openviking-sdk upgraded to 0.1.6 for download_bytes support
@opencode-agent

opencode-agent Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review complete. Both specialist subagents (code + docs) verified my analysis; findings confirmed against the on-disk files.


Review: PR #356 — viking_read auto-return image bytes for vision models

The approach is sound and consistent with the ModalityFilterCapability precedent. Factory injection timing is correct (replace at agent.py:1313 happens before PydanticAgent construction at :1348, so tool closures bind to the populated cap, and for_run() preserves model_capabilities via dataclasses.replace). The tri-state _should_return_image_bytes() logic is clean, and the deliberate divergence from ModalityFilter's pass-through (producer must not emit BinaryImage it can't guarantee) is well-reasoned and documented. Verified: download_bytes genuinely new in SDK 0.1.6 (absent in 0.1.5), and read() returns decoded text — corroborating the garbled-bytes motivation. Existing .md viking_read tests are unaffected.

Blocking

  1. PR body overstates test coverage — the core code path is untested. The claimed "9 mock 场景" for the viking_read image branch do not exist in the diff. Grep confirms zero references to download_bytes, BinaryImage, _is_image_resource, or svg in tests/capabilities/viking/test_viking.py (the one BinaryImage hit is a docstring). Only the _should_return_image_bytes matrix and 4 factory-injection tests were added. tools.py:433-483 (image detection, download_bytes, BinaryImage construction, MIME mapping, SVG/text degradation, mixed lists, download-error path) is the feature's primary logic and has no direct tests. Per tests/AGENTS.md these need L1 mock-client unit tests (note: the mock_client fixture must explicitly set client.download_bytes = AsyncMock(return_value=b"...")).

Major

  1. Stale dependency floor. pyproject.toml:164 still declares viking = ["openviking-sdk>=0.1.0"], but tools.py:445 unconditionally calls client.download_bytes(), which only exists in 0.1.6. The uv.lock bump alone doesn't protect a fresh install or re-resolve landing on 0.1.5 → AttributeError → silent viking_read error: degradation. Bump to >=0.1.6.
  2. Model-facing tool docstring not updated. tools.py:409-426 is the viking_read description that flows into the tool schema and OpenCode listing, yet it still reads as text-only ("File content with line number prefixes..."). Models will receive BinaryImage parts or [Image resource: ...] hints with zero guidance. Add a Returns note on vision-dependent behavior.

Minor

  1. _is_image_resource (tools.py:51) uses PurePosixPath(uri).suffix, which breaks on ?query/#fragment (verified: .png?x=1 → suffix .png?x=1, silently falls back to text). Use urlparse(uri).path. Low risk for viking:// paths, but failure is silent.
  2. Registry inconsistency: the ModalityFilter block registers the populated instance at TURN scope (agent.py:1284-1299); the Viking block doesn't, leaving the AGENT-scope registry entry with model_capabilities=None. Benign today, latent trap.
  3. Unused re-export: viking/__init__.py:30-31 imports IMAGE_EXTENSIONS as ... and _IMAGE_MIME_TYPES as ... but never uses them (tools.py imports from constants directly). Dead code.
  4. New type suppression: tests/test_agent_factory_modality.py:324 adds # type: ignore[arg-type], against the AGENTS.md red line (repo already has ~925 in tests, but this is a fresh one).
  5. Mixed-list ordering: text sections are joined first, all BinaryImage parts appended at the end — not URI order, so image↔description association is positional for 2+ images. Consider interleaving or documenting.
  6. Docstring wording: "Result of viking_read for image URIs." (3 sites) is a verbless fragment; the field controls the result. Recommend "Whether viking_read returns image bytes for image URIs." Also "unset/model None" at __init__.py:109-112 is garbled.
  7. Inaccurate cross-ref: constants.py:16-17 points SVG exclusion at _should_return_image_bytes, but it's enforced by IMAGE_EXTENSIONS in _is_image_resource.

Docs / process notes

  • The manual-sync contract for IMAGE_EXTENSIONS/_IMAGE_MIME_TYPES is documented in-file but has no guard test or sync skill, despite the repo's sync-acp-spec/sync-skills-spec precedent — drift (e.g., server adds .avif) silently degrades to text. Consider a pinning test or a note in capabilities/AGENTS.md.
  • No OpenSpec change record is in git, per AGENTS.md's workflow requirement (PR body says it's local planning).
  • Docstring-level docs are sufficient for this incremental feature; no docs/explanation/ addition required. Pre-existing gaps noted (not blocking): viking absent from docs/explanation/capabilities.md and capabilities/AGENTS.md; support_vision won't appear in generated schema docs since config-schema.json has no viking entries; mkdocs.yml doesn't list tool-display-capability.md.

Behavioral note

This PR silently activates multimodal_bridge modality checks for vision models — model_capabilities was previously never assigned in production, so _supports_modality always returned False and the bridge always degraded to text. This is broader than "viking_read returns bytes," but it's intended and now tested.

Recommendation: address #1 (add the claimed image-branch tests), #2, and #3 before merge; the rest can be follow-ups.

New%20session%20-%202026-08-05T12%3A19%3A42.394Z
opencode session  |  github run

The viking_read image-branch tests (download_bytes -> BinaryImage, SVG
degradation, mixed URIs, error handling) were accidentally edited into the
main working tree instead of this worktree and never committed. Restore
them here and address review findings:

- tools.py: prefix image markers with 1-based index (#N) so return_value
  markers map 1:1 to BinaryImage order in ToolReturn.content (review M1)
- constants.py: rename _IMAGE_MIME_TYPES -> IMAGE_MIME_TYPES (public, used
  cross-module); keep .svg in IMAGE_EXTENSIONS, excluded from byte path
- tools.py: SVG always degrades to text hint even with support_vision=True
- tests: add multi-image index/order mapping test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant