Repository navigation
fix(tui): embed pasted images and carry path in description - #55412
K0nigsberg wants to merge 2 commits into
Conversation
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
With this change the model can see the path of a pasted local image or PDF. I checked this by calling resolvePastedAttachments and toLLMMessages from both checkouts with a small Bun script; I couldn't drive a real paste through the TUI. The changed tests and the typecheck script in packages/core and packages/tui pass.
Public API: the server API, SDK, plugins and config are unchanged. Two things clients rely on do change: every inline image or PDF that has a name now gets an extra Attached file: <name> text part, and TUI path pastes now send the absolute path as filename.
Two things are worth fixing before merge (details inline):
- Any inline name is shown to the model as a file. The TUI names every clipboard image
"clipboard", so the model is now toldAttached file: clipboard. Display names from other clients get the same treatment, and the model can't tell them apart from real paths. - The absolute path is stored in
name, which is used for display. That field feeds the transcript label, the[SVG: …]label and thefilenamesent to providers, so the user's full local path now appears in all three. On a remote server the path also doesn't exist where the agent runs.
It would be cleaner to keep name as the short file name and pass the path separately. For example, the TUI could send local pastes as file:// URIs, which the existing attachmentLocation handling already shows to the model. Or the core could add the path text only for real local sources, so it doesn't rely on whatever ends up in name.
This also changes the same lines in local-attachment.ts as #54845, so the two will conflict.
| // Pasted/clipboard uploads admit as inline sources and keep the client name | ||
| // (often a filesystem path). Surface that name as model-visible text because | ||
| // media.filename alone is not reliably available to the agent. | ||
| const inlineAttachmentName = (file: FileAttachment) => (file.source.type === "inline" ? file.name : undefined) |
There was a problem hiding this comment.
This treats any inline name as something the model can open. Clipboard images from the TUI are always named "clipboard" (prompt/index.tsx and mini/footer.prompt.tsx), so the model now sees Attached file: clipboard. Other clients send display names like image.png. In both cases the model may try to read a file that isn't there. Could the path text come only from a real local source, such as a file: URI (already handled by attachmentLocation) or a separate path field, instead of the display name?
There was a problem hiding this comment.
Agreed — dropped the inline-name → Attached file path. Core lowering again only emits location text for real file: sources via attachmentLocation.
Clipboard bitmaps stay data: + name: "clipboard" with no fake path note.
| const filename = path.basename(item.filepath) | ||
| // Keep the absolute path in `filename` so V2 admission stores it on | ||
| // FileAttachment.name and toLLMMessages can expose it to the model. | ||
| const filename = item.filepath |
There was a problem hiding this comment.
filename becomes FileAttachment.name, which is a display name. It's used for the transcript chip (routes/session/index.tsx:2342), the [SVG: …] paste label, and the filename sent to OpenAI, Anthropic and Bedrock. With this change all of these show the user's full absolute path. With a remote server, the path is also only meaningful on the client machine. It would be better to keep the short file name here and send the path separately, e.g. as a file:// URI when the server is local.
There was a problem hiding this comment.
Updated: filename is the basename again, and the path is passed as a file:// URI so admission keeps source.uri for attachmentLocation. That also avoids putting absolute paths into transcript labels / provider filename metadata.
Still overlaps local-attachment.ts with #54845 — should be a straightforward conflict once one merges.
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
Keep the short basename in FileAttachment.name for display and provider filename metadata. Send the filesystem path as a file:// URI so admission retains source.uri and existing toLLMMessages location text can surface it. Clipboard bitmaps stay data: URIs named "clipboard" and do not get a fake Attached file path note. Fixes anomalyco#41454
6b7f2be to
9296e12
Compare
K0nigsberg
changed the title
|
Addressed the review feedback:
Thanks for the concrete direction. |
There was a problem hiding this comment.
Both points from my last review are fixed: clipboard images no longer give the model Attached file: clipboard, and the attachment name is the short file name again. Sending file:// URIs reuses the location handling the server already has.
I checked it by calling resolvePastedAttachments from both checkouts: the base embeds the file as data: and the PR sends file:///… with the short name. The existing core test shows file: sources reach the model as Attached file: <path>. I couldn't drive a real terminal paste through the TUI. The two changed TUI test files pass (19 tests), and the TUI typecheck script passes.
Public API: unchanged. The prompt request format is the same; the TUI just sends file:// URIs it already uses for @ mentions.
One thing needs fixing before merge (inline): the server now has to open the pasted path itself. That fails with a remote server (--server) and with files that are gone by the time the prompt is submitted. Both work today because the TUI embeds the file's contents at paste time.
| return { | ||
| type: "file" as const, | ||
| uri: `data:${item.attachment.mime};base64,${Buffer.from(item.attachment.content).toString("base64")}`, | ||
| uri: pathToFileURL(item.filepath).href, |
There was a problem hiding this comment.
This changes when the file is read: before, the TUI embedded the file's contents at paste time; now the server opens the path when the prompt is submitted. Two cases that work today will break:
- Remote server (
--server <url>): the path is on the user's machine, so the server'sreadFileAttachmentcan't open it and the prompt fails. - Files that disappear before submit, such as a macOS screenshot dragged from its thumbnail or other temporary files. [FEATURE]: TUI should capture dropped/pasted files at drop time when the source path is ephemeral #46664 asks for the opposite: capture the contents at paste time.
Also, the TUI still reads up to 20 MB of the file above (readLocalAttachment) and then throws the contents away.
Could you keep the embedded data: contents and send the path alongside them? Or send file:// only when the server is local, and fall back to data: otherwise?
There was a problem hiding this comment.
Good catch — switched back to embedding data: at paste time.
The absolute path now rides in FileAttachment.description (basename stays in name). toLLMMessages only emits Attached file: <path> when that description is an absolute filesystem path, so clipboard / free-form descriptions stay out of the path note.
That keeps remote --server and ephemeral paths working while still giving the model a real location when one exists.
Keep data: embedding at paste time so remote servers and ephemeral paths still work. Stash the absolute filesystem path in FileAttachment.description with the short basename in name, and have toLLMMessages emit Attached file text only for absolute path descriptions. Fixes anomalyco#41454
K0nigsberg
changed the title
|
Follow-up addressed: paste embeds |
There was a problem hiding this comment.
My last concern is fixed: the TUI embeds the file's contents at paste time again, so remote --server sessions and temporary files keep working. The earlier Attached file: clipboard and full-path-name problems are also still gone.
I checked resolvePastedAttachments from both checkouts (same embedded image; the PR adds description: <absolute path>). The server keeps description when it saves the attachment, and the new core test shows it becomes Attached file: <path> only for absolute paths. None of the provider adapters read the description metadata, so the path isn't sent twice. I couldn't drive a real paste through the TUI. Core (30) and TUI (19) tests pass, and the typecheck script passes in both packages.
Public API: unchanged. One new convention: an inline image or PDF whose description is an absolute path now also gets an Attached file: <path> note for the model. Other clients can opt in, and existing ones are unaffected unless their description happens to look like an absolute path.
One thing to fix: the file: handling left over from the previous attempt (inline). It's no longer needed for this fix and changes how @-mentioned images behave. A smaller design point, also inline: the description-as-path convention is implicit, so a short note on both sides would help.
| function attachmentKind(uri: string) { | ||
| if (uri.startsWith("data:image/")) return "Image" | ||
| if (uri.startsWith("data:application/pdf;")) return "PDF" | ||
| if (uri.startsWith("file:")) { |
There was a problem hiding this comment.
This is left over from the file:// approach. Pastes no longer create file: links, but @-mentions of image files do (autocomplete.tsx uses pathToFileURL). With isPromptImageUri, @-mentioned images now show up in the prompt's image previews and preview dialog. The TUI also tries to load them from a path that may only exist on the server when using --server. Since this fix doesn't need it, please drop the file: branch and isPromptImageUri (and their uses in prompt/index.tsx and mini/footer.prompt.tsx), along with the file:// label test in attachment.test.ts.
| // Local TUI path pastes embed bytes as data: and stash the absolute path in | ||
| // description. Only treat absolute filesystem paths as locations — never | ||
| // display names like "clipboard" or free-form descriptions. | ||
| const absoluteFilesystemPath = (value: string) => |
There was a problem hiding this comment.
Not blocking: the TUI and the core now share an unwritten rule that a description that looks like an absolute path is the file's location. That's fine as a narrow fix, but any client that puts a path-like value in description will get the note too. A short comment on both sides (here and in local-attachment.ts) pointing to each other would make the rule easier to find, so it doesn't drift.
|
pregunto este fix, hace que cuando uno pegue una ruta de una imagen, la deje como ruta y no la resuelva como imagen? |
Issue for this PR
Fixes #41454
Type of change
What does this PR do?
Pasted local image/PDF paths were submitted as
data:URIs with only a short filename, so the model never saw the filesystem path. This keeps embedding the file bytes at paste time (so remote--serverand ephemeral paths still work), stores the absolute path inFileAttachment.description, and hastoLLMMessagesemitAttached file: <path>only when that description is an absolute filesystem path.Clipboard bitmaps stay
data:+name: "clipboard"with no path note. Free-form descriptions are not treated as paths.How did you verify your code works?
cd packages/core && bun test test/session-runner-message.test.ts— 30 passcd packages/tui && bun test test/prompt/local-attachment.test.ts test/prompt/attachment.test.ts— 19 passbun typecheckinpackages/coreandpackages/tuiScreenshots / recordings
N/A — attachment admission / model-context change. Prompt chips stay
[Image N].Checklist
Added by the opencode bot (AI-generated).
Related: #48190, #46664