Repository navigation
feat(chat-runtime,host-service): every adapter hands the agent its attachments - #8849
superset-app[bot] wants to merge 3 commits into
Conversation
…tachments Part of the attachments plan (PR 4, seam 4). The Claude SDK adapter rendered an attachment part as `[name]`, and the Codex adapter dropped it with a notice. Only ACP resolved it. This is why desktop chat still flattens attachments into path tags in the text. Now all three take one resolver from the host (chat-v3/resolveAttachment.ts). It branches on the identity the part carries. `fileId` is a cloud workspace's storage object: served from the box's cached copy, or imported now when the store is empty, as on a recreated box. `attachmentId` is the host store entry itself. Each hand-out stamps lastUsedAt for the sweep. Bytes are read only for images, since every adapter gives other files by path. - Claude: an image goes as a base64 image block in the SDK's content-array form. Any other file goes as a "# Attached files" line with its name and path, because store files are named by id. - Codex: an image goes as app-server's `localImage` by path. Any other file goes as the same text block. - An unreadable file is named to the agent, not dropped silently. - Resolution is chained per adapter, so prompt order holds. A text-only prompt with nothing queued is still pushed synchronously. The protocol's attachment part now needs `attachmentId` or `fileId` (it needed `attachmentId` before), so a client can send a cloud part by fileId alone. Clients send parts only to hosts at or above ATTACHMENT_PARTS_MIN_HOST_VERSION (1.41.0, the first release with this change). agents.run's attached files block uses the same attachedFilesBlock, so terminal launches also list files by name. A failed resolve reports to Sentry. The host has no PostHog client, so the plan's PostHog event is not sent from here.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
 [Medium impact] Attachment delivery changes are contained to agent prompt workflows. The PR appears safe to merge, with a non-blocking improvement to avoid unused image reads for Codex. Findings
Fix with agent prompt### Issue 1
packages/host-service/src/chat-v3/resolveAttachment.ts:41-43
`createAttachmentResolver` reads every image and builds a base64 string, even for Codex, which only uses `file.path`. Large images therefore add an unnecessary file read and memory use before Codex can start. Let adapters request bytes only when they need them, so Codex can resolve images by path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds one host resolver for attachments identified by
Acknowledged by Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Attachment part] --> B{Has fileId?}
B -->|Yes| C[Look up cached cloud copy]
C -->|Missing| D[Import cloud file]
C -->|Found| F[Return stored file]
D --> F
B -->|No| E[Look up attachmentId]
E --> F
F --> G{Adapter}
G --> H[Claude: image bytes or named path]
G --> I[Codex: local image path or named path]
G --> J[ACP: image bytes or file link]
Reviews (1) · Last reviewed commit: "feat(chat-runtime,host-service): every a..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| data: mimeType.startsWith("image/") | ||
| ? (await readFile(found.path)).toString("base64") | ||
| : "", |
There was a problem hiding this comment.
createAttachmentResolver reads every image and builds a base64 string, even for Codex, which only uses file.path. Large images therefore add an unnecessary file read and memory use before Codex can start. Let adapters request bytes only when they need them, so Codex can resolve images by path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/chat-v3/resolveAttachment.ts
Line: 41-43
Comment:
**Codex reads unused image bytes**
`createAttachmentResolver` reads every image and builds a base64 string, even for Codex, which only uses `file.path`. Large images therefore add an unnecessary file read and memory use before Codex can start. Let adapters request bytes only when they need them, so Codex can resolve images by path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.… attachments An attachment-only message can carry an empty text part, and the Anthropic API rejects an empty text content block.
…floor Desktop and mobile both decide between attachment parts and path tags from a host's version. One function next to ATTACHMENT_PARTS_MIN_HOST_VERSION keeps the two clients on the same rule. It coerces the version, so a canary or a dev build of a release at or above the floor counts.
|
Folded into #9064, which now carries the whole attachments plan (commit list in its description). Closing in favor of that PR. |
Summary
PR 4 of the attachments plan (v6), seam 4: every harness gets the file, from whichever id the part carries. Claude SDK and Codex stop dropping attachments, and all three adapters share one host resolver.
Why
[name]. The Codex adapter dropped it, with the notice "Attachments are not supported by the codex harness". Only ACP sent the file. That is why desktop chat still puts attachments into the text as path tags, and so desktop and mobile can't render each other's attachments.fileId) has to resolve on a box whose store is empty, for example a recreated box resuming a session. No adapter could do that.How it works
host-service/src/chat-v3/resolveAttachment.ts, passed to ACP, Claude and Codex frommount.ts):fileIdset: use the box's cached copy (handOutAttachmentByFileId), or elseimportCloudAttachmentsit now and use that.attachmentId: the host store entry (handOutAttachment).lastUsedAt. Bytes are read (base64) only for images. Other files go by path, so a 200 MB PDF is not read into memory. A thrown error (for example a failed import) goes to Sentry (reportAttachmentResolveFailure), and the adapter getsnull.AttachmentPart,ResolvedAttachment,ResolveAttachmentinharness/types.ts. ACP'sresolveAttachmentnow gets the whole part instead of anattachmentId.AcpAttachmentstays as an alias.attachedFilesBlock(harness/attachedFiles/):# Attached files … - <name>: <path>. Store files are named<id>/<id>.<ext>, so a bare path would give the agent only a UUID.agents.run's terminal-launch block uses it too, so it now lists names as well.imageblock in the SDK's content-array form. Any other file goes as the attached-files text. A file that can't be resolved goes as[name: could not be read].{ type: "localImage", path }. Any other file goes as the attached-files text, and an unresolved one as the same "could not be read" text. As the plan says, we send the image even to a text-only model and let app-server reject it.attachmentIdorfileId(it requiredattachmentIdbefore), so a client can send a cloud part byfileIdalone. Mobile'sUserMessageBubblekeeps showing parts that have a host id and skips a fileId-only part for now. The mobile PR adds the cloud branch.ATTACHMENT_PARTS_MIN_HOST_VERSION = "1.41.0"in@superset/shared/attachment-limits.desktop-v1.40.0is the latest tag, and this is not in it. If this misses 1.41.0, raise the constant before the desktop PR uses it.hostReadsAttachmentParts(version)in@superset/shared/host-versionis the client-side check, so desktop (feat(desktop): attachments go where the workspace lives, as chat parts #8851) and mobile use the same rule. It coerces the version, so a canary or dev build of 1.41.0 or later counts (tested at, above and below the floor, and for an unknown version).Where this differs from the plan, or the plan is out of date against main
chat-v3/resolveAttachment.ts, not insideacpHarnesses.ts, because three adapters now use it. It is passed throughcreateChatV3Mount({ api })so the box import has an API client.tRPC / version compatibility
attachmentIdis now optional, and at least one of the two ids is required. An older host still requiresattachmentId, so a fileId-only part sent to it fails validation. That is why clients send parts only at or aboveATTACHMENT_PARTS_MIN_HOST_VERSION. No client on main sends a fileId-only part yet. Parts the host builds itself (feat(host-service): sweep the attachment store by last use, and keep fileId on cloud copies #8846) carry both ids.Testing
bun test: 384 pass, 0 fail (the Claude adapter suite is 38/38 after 45ec975). New tests:textfixture,turn/startparams now recorded byFixturePlayer): an image becomeslocalImage; a non-image becomes name + path; an unreadable one is named.attachedFilesBlockformat.resolveAttachment.test.ts: a host part reads from the store; a fileId part on a box with a cached copy does not import; a fileId part on an empty store imports;fileIdwins when both are set; a missing id gives null; a failed import is reported and gives null; a non-image's bytes are not read.bun test: 2651 pass, 1 fail. The failure is the samehealth.checksandbox-mode test that fails on feat(host-service): sweep the attachment store by last use, and keep fileId on cloud copies #8846: it reads this machine's real boot stamps, because this workspace is a real cloud box.bun run typecheckpasses in chat, chat-runtime, host-service, cli and apps/desktop. apps/mobile has the same 190 errors with and without this PR (I diffed the error lists; this PR first added 2 inUserMessageBubble, which are fixed). Biome is clean on the changed files.Known limitations