Skip to content

feat(host-core): let Read return an image the model can be shown - #765

Open
zhangqingkun976 wants to merge 1 commit into
vastsa:mainfrom
zhangqingkun976:feat/read-images-from-disk
Open

zhangqingkun976 wants to merge 1 commit into
vastsa:mainfrom
zhangqingkun976:feat/read-images-from-disk

Conversation

@zhangqingkun976

@zhangqingkun976 zhangqingkun976 commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #711.

The report is exact: with an image-capable model, asking the agent to read an image from a path fails while attaching the same file to the prompt works. The reason is in the tool, not in the transport — and the transport half of this already existed, untested.

What was actually missing

Read refuses any file whose extension is in BINARY_EXTENSIONS (png, webp, ico …) and then refuses anything the byte sniff calls binary, both as TOOL_BINARY_CONTENT ("has no text to read"). So the model was told the file had no content, which is true of text and false of an image.

The other half was already wired: a host tool result shaped {text, images: [{data, mimeType}]} becomes [{type: "text"}, {type: "image"}…] in the runtime, gated on the model declaring image input (visionFromModelConfig). Nothing produced that shape, and the path had no test. pi's own AgentToolResult.content is (TextContent | ImageContent)[], so this needed no new plumbing.

What the change does

input result
shot.png (a real PNG, model declares image input) one text block plus an image block with image/png and standard base64
the same, model declares text only the text block alone, saying an image is there and to say so rather than guess
notes.png (text renamed) TOOL_BINARY_CONTENT — a request carrying a broken image fails whole, so the magic number has to agree with the extension
huge.png (4 MB) TOOL_IMAGE_TOO_LARGE, naming the size and the alternative
.pdf, .zip, … unchanged TOOL_BINARY_CONTENT

The 3 MB raw bound exists because base64 inflates by four thirds and the strictest per-image ceiling this build talks to is 5 MB encoded. The base64 payload never reaches a persisted UI message or transcript record — the stored detail keeps imageCount, which the existing attachment rule already required.

Verification

  • host-core: read_returns_an_image_for_the_model_to_view, read_refuses_an_image_extension_whose_bytes_are_not_an_image, read_refuses_an_image_too_large_to_inline (the fixture is a real 1×1 PNG, decoded from base64 in the test); cargo test -p host-core --locked → 583 passed, 4 failed, the four being the pre-existing Windows path-separator assertions in mcp_servers, user_skills, scheduled_rpc and scheduled_tools.
  • agent-runtime: two new tests cover the model-side bridge for the first time (attached with vision; dropped without, text kept). On the rebased head 5cf79539, with @pi-desktop/shared rebuilt first, the suite is 766 passed / 1 failed, the failure being the pre-existing native-pi-session case, and src/runtime.test.ts — the new bridge among its tests — passes in full.
  • cargo fmt --all --check 0; cargo clippy -p host-core --all-targets --all-features exit 0 (one pre-existing warning in user_skills.rs, untouched); shared typecheck 0; pnpm docs:check (79 EN/zh pairs, 499 pages) exit 0.

E2E: partial. pnpm test:e2e (scripts/e2e-smoke.mjs, the headless protocol suite) → 23/23 passed, 2 skipped on this branch, and it does exercise the changed code: E2E-013-read-tool runs the real Read tool through the host binary built from this branch, across the RPC boundary. What is not run is the scenario written below — E2E-TOOL-read-returns-an-image-the-model-can-see needs a live provider request with a fixture image, and this repository's E2E set does not drive a real image read. Alternative validation: the host tests exercise the real Read tool through execute_tool, and the runtime tests exercise the real tool callback and assert on the exact content blocks pi-ai would send. Remaining risk: how a specific provider renders the image (and whether a relay strips image blocks) is not observed here.

Docs

03-runtime/16-tool-result-limits.md §4/§6 and its acceptance list, 03-runtime/08-error-codes.md (the new code plus the widened meaning of the old one), 08-meta/decisions-log.md D609, and the E2E scenario above — all mirrored to docs/zh-CN/….

D609 was free when this was opened and still is: D606 (calibration), D607
(plugin crash) and D608 (tool-call ids) have since landed in main, and
#748/#756/#760 are closed. D604/D605 still belong to the open #721, so nothing
needs renumbering unless that one lands first.

Base and head

base 206085c07 · head 5cf79539 · 11 files, +378 / −7. Rebased onto current main (82 commits
further on); it applied cleanly except for one same-point insertion in
ModelSelectionPanes.tsx on the branches that touch it, where both sides were
kept. The published tree is verified to be the same object the gates ran on:

local tree : 283a47d16b4976278399741e8270d1e5350c0f5d
remote tree: 283a47d16b4976278399741e8270d1e5350c0f5d

@zhangqingkun976
zhangqingkun976 force-pushed the feat/read-images-from-disk branch 2 times, most recently from 214e7c0 to 7b5ffe4 Compare September 21, 2026 05:40
@zhangqingkun976

Copy link
Copy Markdown
Contributor Author

Rebased onto main (ab39a9b4) so the new Head contains latest base gate passes. Head is now 7b5ffe4e.

The only conflicts were append-vs-append — the E2E scenario catalog and the decisions log, where main and this branch each added an entry — and both sides are kept. The identifier bands are still free: upstream's newest ADR is 0299 and its newest decisions-log entry is D607 (the plugin crash report, which is this repository's own fix for issue #747 built on my #756).

@zhangqingkun976
zhangqingkun976 force-pushed the feat/read-images-from-disk branch from 7b5ffe4 to d71d97e Compare September 21, 2026 09:28
@zhangqingkun976

Copy link
Copy Markdown
Contributor Author

Rebased onto main (b71fcf05, 39 commits further on); the code and both docs applied cleanly. New head d71d97ed.

Two things changed in the description, both about evidence:

Local gates on the rebased head: cargo fmt --all -- --check 0; cargo clippy -p host-core --all-targets --all-features exit 0; cargo test -p host-core --locked 552 passed / 4 failed (the four pre-existing Windows path-separator asserts); agent-runtime typecheck 0 with the suite at 751 passed / 1 failed (the same pre-existing native-pi-session case, and src/runtime.test.ts — 215 tests, the new bridge among them — passes in full); shared typecheck 0; pnpm docs:check exit 0; node scripts/check-pr-base-main.mjs passed.

@zhangqingkun976
zhangqingkun976 force-pushed the feat/read-images-from-disk branch from d71d97e to c65b117 Compare September 21, 2026 12:34
@zhangqingkun976

Copy link
Copy Markdown
Contributor Author

Rebased onto main (43a37373, 24 commits further on). New head c65b1177, one commit.

It applied cleanly, including the convertToLlm seam in runtime.ts that main has since rewritten: your tool-call-dedupe.ts refactor of my #780 now sits there, and git's three-way merge composed it with this change instead of dropping either side. That composition is verified rather than assumed — on the two branches that add a pass at that seam (#705 and #721, a superset of it) the seam now reads this.narrowToolResultsUnderPressure(this.dropDuplicateToolCalls(messages)), so the dedupe runs first and the tiering second. Both passes are identity functions when they have nothing to do, which keeps this PR's original promise that an ordinary request is byte-identical.

Local gates on the rebased head: node scripts/check-pr-base-main.mjs passed; cargo fmt --all -- --check 0 where the branch touches Rust; pnpm docs:check 79 EN/zh pairs, 499 pages on the branches that touch docs. mergeable=true, behind=0.

One thing to know when reading the JS job: main has landed image generation and its new suite is red on main itself. Pristine 43a37373 fails 8 tests across 4 files (native-pi-session, parent-host-proxy, image-generation, openai-images-contract); this branch fails exactly the same 8 and adds its own on top. I have not touched those files.

A session asked to look at an image on disk could only be told the file was
binary content and had no text to read (issue vastsa#711), while the same app happily
sent an image the user attached. The runtime already turned a host tool result
shaped `{text, images: [{data, mimeType}]}` into a model-visible image block;
no tool produced that shape.

- `Read` now returns it for `.png`, `.jpg`, `.jpeg`, `.gif` and `.webp`. The
  block is attached only when the active model accepts images, and the text is
  kept either way, so a model without vision reports that it cannot see the
  image instead of inventing its contents.
- The magic number decides the type and has to agree with the extension: a text
  file renamed `.png` is still `TOOL_BINARY_CONTENT`, because a request carrying
  a broken image fails as a whole.
- An image above 3 MB raw (base64 inflates by four thirds, and the strictest
  per-image ceiling in use is 5 MB encoded) is refused with the new
  `TOOL_IMAGE_TOO_LARGE`, naming the size and the alternative, rather than being
  cut into an unreadable fragment.
- The base64 payload never enters a persisted UI message or transcript record;
  the stored detail keeps `imageCount`, as the attachment rule already required.

C:\Users\10470\.pi-desktop\scratch\cb9e2d41-8a55-4a18-899d-92ca2791ab51\commit-711-r2.txt
@zhangqingkun976
zhangqingkun976 force-pushed the feat/read-images-from-disk branch from c65b117 to 5cf7953 Compare September 22, 2026 01:34
@zhangqingkun976

Copy link
Copy Markdown
Contributor Author

Correction to my previous comment on this PR. I wrote that pristine main failed 8 tests across 4 files (native-pi-session, parent-host-proxy, image-generation, openai-images-contract) and that the image-generation failures were main's own. That was wrong, and the fault was mine, not main's.

What actually happened: my local packages/shared/dist was stale. main had landed image generation, which added exports to @pi-desktop/shared, and my built dist predated them — so agent-runtime imported names that did not exist in the artifact it was type-checking and running against. Six of the eight failures, and the three typecheck errors I saw, were produced by my environment.

After rebuilding @pi-desktop/shared and re-measuring on pristine main (206085c07):

  • pnpm --filter @pi-desktop/agent-runtime typecheck → clean, 0 errors
  • pnpm --filter @pi-desktop/agent-runtime test → 763 passed, 2 failed

The two real baseline failures are:

  1. src/native-pi-session.test.ts > native fork children > never deletes a foreign publication and classifies the failure path-free — a long-standing Windows path assertion, unrelated to this change.
  2. src/hosted-search-contract.test.ts > forwards hosted_search_update as message_update — a 5 s timeout that is intermittent: it passes standalone and on reruns.

So main is not red, and image generation is not broken. I should have rebuilt the workspace dependency before drawing a conclusion from a failure I did not recognise; the fact that the failures appeared only after main gained a new feature should have pointed at my stale artifact first.

This head 5cf79539 was measured the same way, with the dependency rebuilt: the suite fails exactly the single native-pi-session case above, and nothing else. No file in the image-generation or hosted-search area is touched by this PR.

Apologies for the noise — and for stating a baseline I had not verified. The CI panel on this head is the authoritative record, and it is green.

This branch has not been deployed

No deployments
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.

[Bug] Claude在会话中不支持哦读取路径中图片

1 participant