Skip to content

Preserve attachments in tool outputs - #312

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

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

Conversation

@qoli

@qoli qoli commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

The prompt-attachment foundation from #310 landed on main through #321. This branch now integrates that upstream result and keeps the remaining change focused on Tool output attachments.

Summary

  • lower ordinary Tool prompt outputs into ordered transcript segments instead of flattening them to text
  • preserve image outputs through the Foundation Models Tool wrapper on OS 27
  • expose the existing provider-adapter lowering helper through the Compatibility SPI

Scope

This is limited to Tool output attachments. It does not add instruction attachments, labels, CIImage, pixel buffers, or data attachments.

Validation

  • swift test --filter PromptAttachmentTests — 11 tests passed on current upstream main
  • swift format lint --strict --recursive .
  • full Swift, iOS Simulator, and watchOS validation from the earlier branch revision

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
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 on lines +802 to +803
case .url(let url):
parts.append(.fileData(GeminiFileData(fileURI: url.absoluteString)))
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.

2 participants