Skip to content

Preserve attachments in tool outputs - #312

Open
qoli wants to merge 31 commits into
huggingface:mainfrom
qoli:codex/upstream-tool-output-attachments
Open

qoli wants to merge 31 commits into
huggingface:mainfrom
qoli:codex/upstream-tool-output-attachments

Conversation

@qoli

@qoli qoli commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Scope

Preserve ordered text/image Tool outputs while retaining OS 26 deployment support and the OS 27 native bridge. #310's foundation landed through #321; #316's attachment README work is included here and #316 remains closed. No dependency on #313–#315.

  • Ordinary local Tool image output snapshots bytes/MIME once at output creation. The inferred UTType must conform to .image; PDF/text files remain errors.
  • Deleting the original file does not break continuation, Codable persistence or native replay. Native prompt/delegate/ToolCallRecord paths share the same snapshots; upstream Honour the tool execution delegate in SystemLanguageModel #297's delegate is preserved, not reimplemented.
  • At Tool-output wire lowering only, Anthropic accepts JPEG/PNG/GIF/WebP and Gemini function responses accept PNG/JPEG/WebP. Other MIME becomes ordered <image> text; canonical/Codable images and original bytes/MIME remain unchanged. Supported read failures and Tool failures still throw. User-image input is not degraded; no downloads, transcoding or retries were added.
  • Gemini remote Tool image URLs remain ordered URL text without downloading. Function-response parts use inlineData, not fileData. The existing README note requires Gemini 3+ for image function responses.
  • Adapter lowering remains Compatibility SPI.

Upstream synchronization

Normal history-preserving synchronization with upstream main 7013d51, retaining #351 typed numeric bounds, #336 final-request usage and #340 HTTPTransport, along with #297/#325/#331/#343/#352/#354/#356. No force-push, new checkout or replacement PR. The transport/usage changes are inherited without removing the attachment lowering or expanding fallback scope.

Fresh local verification (2026-10-22)

Head aed9f2a. CI=1 GIT_CONFIG_GLOBAL=/dev/null swift test: 861 tests / 90 suites passed. Strict formatting/diff checks passed. These are compile/offline tests, not live API, model-weight or Apple Intelligence inference acceptance.

Regressions cover supported/unsupported MIME, TIFF/SVG/GIF snapshots deleted before continuation, placeholder order, streaming/non-streaming Codable replay, legacy URL-backed history, explicit read failures, unchanged user images, and native delegate/no-delegate snapshots.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Gemini tool output loses preceding text, and public-facing documentation and bridge errors need correction.

3 open findings
What changed in this PR

Adds ordered image attachment preservation across prompts, transcripts, tool outputs, and Foundation Models bridges.

Changes:

  • Introduces typed image attachments and ordered prompt segments.
  • Preserves attachment-bearing tool outputs.
  • Adds Foundation Models bridging, tests, and documentation.
File Description
README.md Documents prompt attachments.
Attachment.swift Defines typed image attachments.
Prompt.swift Preserves ordered prompt components.
Tool.swift Lowers tool output into transcript segments.
LanguageModelSession.swift Records segmented prompts.
SystemLanguageModel.swift Bridges attachments to Foundation Models.
FoundationLanguageModel.swift Handles throwing prompt conversion.
PromptAttachmentTests.swift Tests attachment preservation and bridging.
DynamicInstructionsTests.swift Enables Compatibility SPI access.
ToolExecutionDelegateTests.swift Enables Compatibility SPI access.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Sources/AnyLanguageModel/Tool.swift Outdated
Comment thread Sources/AnyLanguageModel/Models/SystemLanguageModel.swift Outdated
Comment thread README.md Outdated
@qoli

qoli commented Oct 11, 2026

Copy link
Copy Markdown
Contributor Author

CI note: the sole red job failed during actions/checkout, before any package code ran. All three fetch attempts ended with Could not resolve host: github.com; the other six matrix jobs succeeded. GitHub does not allow the fork author to rerun this upstream workflow without repository admin rights.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Optional prompt branches can emit spurious empty segments, and platform support messaging overstates availability.

1 open finding
Previously missed (2)

In code that hasn't changed since last review

Medium severity Omit nil optional branches from prompt components

Sources/​AnyLanguageModel/​Prompt.swift:144

The component-wise join now exposes buildOptional(nil) as a real empty text component. For example, Prompt { Attachment(...); if false { "note" }; Attachment(...) } lowers to image/empty-text/image rather than just the two images, so an absent branch is forwarded to providers. Represent a nil optional with no components instead.

Low severity Clarify Foundation Models platform support documentation

README.md:402

The bridge excludes tvOS and watchOS, while FoundationLanguageModel is available on watchOS 27. Therefore “on OS 27” overstates platform support. Narrow this documentation to the three platforms actually handled by Prompt.toFoundationModels().

🧠 Review effort: Balanced

Comment thread Sources/AnyLanguageModel/Models/SystemLanguageModel.swift Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

URL-backed Gemini tool attachments currently serialize into an unsupported and incomplete function-response file payload.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Comment thread Sources/AnyLanguageModel/Models/GeminiLanguageModel.swift Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Anthropic forwards local Tool image URLs unchanged, causing requests containing those attachments to fail.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Inline local file attachments instead of sending inaccessible file:// URLs

Sources/​AnyLanguageModel/​Tool.swift:130

A Tool returning Attachment(imageURL: localFileURL) without an explicit orientation now produces a URL-backed image segment. Anthropic forwards it as source.type = "url" with the literal file:// URL, both in streaming tool results and transcript replay (AnthropicLanguageModel.swift:719,1094,1258–1259). The remote service cannot read the caller's filesystem, so these requests fail instead of delivering the attachment. Inline local files as base64 with a supported MIME type in the Anthropic adapter, and add a local-file Tool-output transport regression test.

🧠 Review effort: Balanced

@qoli

qoli commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the local-file transport issue from the latest review in 4d9cd91. Anthropic now reads local PNG/JPEG/GIF/WebP attachments at request encoding time and sends base64 with the matching media type, rather than sending file:// URLs. Remote image URLs are unchanged; unsupported local types fail explicitly. Added regressions for streamed Tool continuation and subsequent transcript replay (including nonstreaming Tool results). Fresh swift test --filter 'ProviderUsageTests|AnthropicRequestTests': 40 tests passed; strict formatting and diff checks passed. Propagated into the existing docs stack #316.

@mattt

mattt commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

Thanks, @qoli. Inlining local images looks right to me. One thing: Anthropic and Gemini read the file each time a request is encoded, which includes every replay of the transcript. If the file is deleted or moved after the tool call, every request after that fails. Could you read the data once, when the tool output is created, and keep it in the transcript?

Since #316 only adds the README on top of this, could you fold those changes in here, so it lands as one PR?

@qoli

qoli commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, @mattt — addressed in 5a17ad3. Local Tool image attachments are now read once in Tool.makeOutputSegments(from:), and the resulting bytes/MIME type are retained in the Codable transcript. The regression deletes the source file in didExecuteToolCall (before continuation encoding), then persists/restores and replays the history; both Anthropic and Gemini pass in response and streaming modes. Remote image URLs remain unchanged.

I also folded the README changes from #316 into this branch and closed the redundant documentation PR. This branch includes current upstream main, including #343. Fresh validation: 63 focused tests and the complete 758-test/76-suite run passed, together with strict formatting/diff checks.

@qoli

qoli commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Merged main 371b142 normally and reconciled the attachment work with the native delegate path from #297 in cf756b1. The delegate and ToolCallRecord flow are retained; ordinary Tool execution now records and forwards the snapshot-preserving segments instead of flattening images into text. A new no-inference regression covers both delegate/no-delegate cases, removes the local PNG before transcript encoding, then round-trips and replays its bytes/MIME. Both cases reproduced image loss before the fix. Fresh CI-mode SwiftPM run: 802 tests in 82 suites passed; changed-file format lint and diff checks passed. This does not claim live model inference.

@mattt

mattt commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Thanks, @qoli. Reading local images once looks good. One more thing on Gemini: a remote image URL in a tool output now throws while the request is being built, after the tool has already run, so retrying fails the same way, and a transcript from another provider can fail on every Gemini request. The Gemini API has nowhere in a function response to put a URL, so could you send it as text at that position instead — the way the Ollama path already does — and update the test to match?

Two smaller things:

  • Gemini only accepts images in function responses from Gemini 3 on; older models reject the request. Could the README mention that?
  • makeOutputSegments treats any file with a MIME type as an image, so a PDF would get through and then be rejected by the provider. Could it check that the type conforms to .image?

@qoli

qoli commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for checking the snapshot path. The file-type issue is fixed in c96bd09: local Tool image output now requires an inferred UTType that conforms to .image, rather than merely a known MIME type. New .pdf/.txt regressions reproduced acceptance before the guard and pass after it.

After merging current main 303f8a1, fresh CI-mode SwiftPM validation passes 828 tests / 85 suites, including the deleted-file/Codable/native replay cases and the upstream delegate path. Formatting and diff checks pass.

The remote Gemini URL-as-text change and the Gemini 3+ README note are still follow-up items; neither is included in this push, so I am not marking those parts addressed.

@qoli

qoli commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

The remaining two Gemini items from #312 (comment) are now implemented in 07ad954.

  • A remote image URL in a Tool output is appended as URL text at its original position, without downloading it. Local-file/inline image handling is unchanged.
  • The canonical and Codable transcript still retain the original image segment, so the provider-specific text conversion does not rewrite stored history.
  • The README now states that image function responses require Gemini 3 or later.
  • The replacement regression first reproduced unsupportedURL in both streaming and non-streaming. It now checks the immediate continuation and a Codable-restored replay request for before / URL / after ordering, no image wire parts for the remote URL, and unchanged canonical image data.

Fresh CI-mode validation on #312: the focused attachment/thought-signature suites pass 24 tests / 2 suites, and the full run passes 828 / 85. Updated #346 and #347 pass 896 / 91 and 895 / 91; draft #317 passes 930 / 92, plus 798 / 83 with the isolated AsyncHTTPClient scratch build. Strict formatting and diff checks pass. No live inference is claimed.

Together with c96bd09's UTType .image guard, all three items in that comment are addressed. The existing PRs and dependency notes have been updated; #317 remains draft.

@mattt

mattt commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Thanks, @qoli. All three look good. One more case of the same kind: any local file that conforms to .image goes to Anthropic or Gemini as image data, including types they don't list, like TIFF or SVG. Anthropic takes only JPEG, PNG, GIF, and WebP, and Gemini's list is different again (no GIF). A request with one of those could fail after the tool has run, and again on every replay. Could Anthropic and Gemini check the type against what they accept, and send the rest as the <image> placeholder that tool outputs used before?

Also, geminiFunctionResponseData(from:) is only called for file URLs now, so its isFileURL guard and unsupportedURL can go.

@qoli

qoli commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, @mattt. The redundant isFileURL guard and private unsupportedURL error in geminiFunctionResponseData are removed in 14d1b74; its caller already selects the local-file path.

The remote-URL-as-text conversion, original segment ordering, canonical/Codable image, local snapshots and native delegate/recording behavior are unchanged. Fresh CI-mode SwiftPM verification on this head passed 828 tests / 85 suites, plus strict formatting and diff checks.

The provider MIME allowlists / unsupported-format <image> placeholder are a separate behavior change still pending confirmation, so that part is not yet addressed in this head. I have not silently added transcoding, downloads or another fallback.

@qoli

qoli commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, @mattt. The provider MIME allowlist / <image> behavior is now implemented in #312 77e57d0, and normally merged into #346 cb8603b, #347 2592e6f and the draft aggregate #317 d151eb8.

  • Anthropic Tool-output image data accepts JPEG/PNG/GIF/WebP; Gemini function-response image data accepts PNG/JPEG/WebP. Other image MIME types become <image> text at the original text position. Legacy unsupported local-file URL segments take the same placeholder path.
  • This is provider-wire lowering only: canonical/Codable image segments and the original snapshot bytes/MIME remain intact, including after deleting the local file. Supported image bytes are not transcoded. User-image input behavior, the existing Gemini remote-URL-as-text conversion, and native delegate/recording paths are unchanged.
  • No downloads, retries or catch-and-continue behavior were added. Ordinary Tool reads and supported legacy URL image reads still fail explicitly when the file is missing.
  • New regressions first failed on the old conversion, then passed for supported/unsupported inline MIME, TIFF/SVG/GIF snapshots, placeholder order, file deletion and Codable replay. They inspect streaming continuations, Gemini non-streaming continuations, and replay in both modes. Integrate dynamic profiles with Anthropic Tool loops #346 also has a dedicated profile regression that caught its separate non-streaming in-flight encoder; that path now uses the same Tool-only conversion. Ordinary Anthropic non-streaming remains a single request/return.

Fresh local CI=1 GIT_CONFIG_GLOBAL=/dev/null swift test: #312 834 tests / 85 suites, #346 905 / 91, #347 903 / 91. Integrated #317: 940 / 92 default, 801 / 83 with AsyncHTTPClient, 1106 / 102 with MLX/Llama/CoreML, using separate trait scratch paths in the same checkout. Strict formatting/diff checks passed. These are compile/offline checks, not live API/model-weight inference acceptance.

The redundant Gemini local-file guard/error cleanup from the previous reply remains in place. No constructor visibility or Ollama PR-split decision is bundled into this change.

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.

3 participants