Skip to content

Add Foundation Models 27 prompt attachments - #310

Closed
qoli wants to merge 4 commits into
huggingface:mainfrom
qoli:codex/upstream-prompt-attachments
Closed

qoli wants to merge 4 commits into
huggingface:mainfrom
qoli:codex/upstream-prompt-attachments

Conversation

@qoli

@qoli qoli commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add the Foundation Models 27-style typed image Attachment prompt surface
  • preserve ordered text and image segments through Prompt builders and LanguageModelSession transcripts
  • bridge CGImage and local image URLs to the native FoundationModels API on OS 27, with explicit errors on unsupported system runtimes

Scope

This PR is intentionally prompt-only. Tool-output attachments, instruction attachments, labels, CIImage, pixel buffers, and data attachments are left for separate reviewable changes.

Validation

  • swift format lint --strict --recursive .
  • swift test --filter PromptAttachmentTests (4 tests)
  • CI=1 swift test (684 tests, 72 suites)
  • xcodebuild build -scheme AnyLanguageModel -destination generic/platform=iOS\ Simulator CODE_SIGNING_ALLOWED=NO
  • xcodebuild build-for-testing -scheme AnyLanguageModel -destination generic/platform=watchOS\ Simulator CODE_SIGNING_ALLOWED=NO

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

Foundation-backed requests duplicate attachment prompts, and replayed image attachments lose their orientation.

2 open findings
What changed in this PR

Adds typed image attachments to prompts while preserving text/image ordering across builders, transcripts, and Foundation Models bridges.

Changes:

  • Introduces URL and CGImage prompt attachments.
  • Preserves ordered prompt segments in session transcripts.
  • Bridges attachments to Foundation Models on OS 27 with tests and documentation.
File Description
Attachment.swift Defines typed image attachments and transcript encoding.
Prompt.swift Adds component-based prompt construction.
LanguageModelSession.swift Records ordered prompt segments.
SystemLanguageModel.swift Bridges attachments to Foundation Models.
FoundationLanguageModel.swift Adds throwing attachment conversion.
PromptAttachmentTests.swift Tests ordering, encoding, and native bridging.
README.md Documents prompt attachments and limitations.

🧠 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/Attachment.swift
Comment thread Sources/AnyLanguageModel/Models/SystemLanguageModel.swift

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

Reusing a prompt produces duplicate transcript segment identifiers.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

Comment thread Sources/AnyLanguageModel/Prompt.swift
@mattt
mattt requested a balanced review from Copilot October 12, 2026 12:30

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

Optional branches can emit phantom text segments, and duplicate suppression can resend mutable image prompts.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Duplicate suppression redundantly re-encodes attachment payloads

Sources/​AnyLanguageModel/​Models/​SystemLanguageModel.swift:345

Duplicate suppression lowers the prompt a second time. For an oriented URL attachment this rereads and re-encodes the file after the session has already recorded its segments; if the file changes or becomes unreadable between those operations, the payload comparison fails and the same current prompt remains in history while also being passed as fmPrompt. CGImage prompts also pay for PNG encoding twice. Compare against the already-lowered current prompt occurrence instead of regenerating attachment payloads here.

🧠 Review effort: Balanced

@qoli

qoli commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the previously missed attachment re-lowering issue in 94deb2e. Recorded prompt occurrences now use deterministic segment IDs, duplicate suppression matches those IDs without reopening/re-encoding attachment sources, and empty Prompt builders retain one empty text segment. Added regression coverage for deleting an attachment file after transcript recording and for empty builders. Fresh verification: CI=1 swift test (687 tests passed).

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

Unsupported attachment failures are opaque, and documented platform support overstates actual availability.

0 open findings

Previously missed (2)

In code that hasn't changed since last review

Medium severity Add actionable descriptions to thrown bridge errors

Sources/​AnyLanguageModel/​Models/​SystemLanguageModel.swift:563

These thrown errors are internal and have no localized description, so callers only receive an opaque message such as PromptBridgeError error 0 when an attachment is rejected. Add actionable messages that distinguish an unsupported runtime/toolchain from a non-file URL.

Low severity Document Swift and platform requirements accurately

README.md:404

This overstates platform support: the bridge is compiled only with Swift 6.4+ and explicitly excludes tvOS and watchOS, where line 467 throws attachmentsUnavailable even on OS 27. Document the actual compiler and platform requirements so users do not expect this example to work there.

🧠 Review effort: Balanced

@mattt

mattt commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks, @qoli. To get this into 1.0, I opened #321 with your commits and a few fixes on top: the watchOS build, descriptions for the attachment errors, and the platform requirements in the README. I'll close this in favor of that one. #312 and #316 will need a rebase on main after it merges. Thanks again!

@mattt mattt closed this Oct 13, 2026
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