feat(protocol): preserve media attachment names - #3548
Conversation
🦋 Changeset detectedLatest commit: 0dedb84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe96535219
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| content.push({ | ||
| type: 'image', | ||
| source: { kind: 'url', url: buildDaemonFileUrl(finalFile.meta.id) }, | ||
| name: part.name ?? file.meta.name, |
There was a problem hiding this comment.
For file-backed images and videos, this now emits a non-empty name, but the existing integration tests in packages/kap-server/test/prompts.test.ts still compare the complete response against objects without that property. In particular, carries an uncompressed uploaded image... and carries an uploaded video... will fail whenever the kap-server suite runs, so update those expectations to include the preserved filenames rather than reverting the behavior.
AGENTS.md reference: AGENTS.md:L61-L61
Useful? React with 👍 / 👎.
| else if (part.type === 'video' && part.source.kind === 'session_media') parts.push({ type: 'video_url', videoUrl: { url: buildDaemonFileUrl(part.source.file_id), id: part.source.file_id } }); | ||
| else if (part.type === 'image' && part.source.kind === 'url') parts.push({ type: 'image_url', imageUrl: { url: part.source.url, id: part.source.id, name: part.name } }); | ||
| else if (part.type === 'image' && part.source.kind === 'base64') parts.push({ type: 'image_url', imageUrl: { url: `data:${part.source.media_type};base64,${part.source.data}`, name: part.name } }); | ||
| else if (part.type === 'image' && part.source.kind === 'session_media') parts.push({ type: 'image_url', imageUrl: { url: buildDaemonFileUrl(part.source.file_id), id: part.source.file_id, name: part.name } }); |
There was a problem hiding this comment.
When a client reuses an existing session_media attachment without the new optional name field—as existing clients and historical prompt payloads do—this conversion copies undefined even though assertPromptSessionMediaRefs opens a SessionMediaFile whose persisted metadata includes the original name. Consequently the core part, turn.started attachment, and reconstructed transcript remain nameless; enrich session-media parts from the store metadata when the request omits the name, including the analogous video path.
Useful? React with 👍 / 👎.
| content.push({ | ||
| type: 'image', | ||
| source: { kind: 'base64', media_type: compressed.mimeType, data: compressed.base64 }, | ||
| name: part.name, |
There was a problem hiding this comment.
When a named base64 image resolves to a MIME type unsupported by the model, the earlier branch returns before reaching this preserved-name path and constructs the resulting file attachment with the generic image.<ext> name. Thus a valid request such as a named TIFF loses its filename in both the response and cold transcript; use the supplied part.name as the attachment name, with the generated name only as a fallback.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe96535219
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| content.push({ | ||
| type: 'image', | ||
| source: { kind: 'url', url: buildDaemonFileUrl(finalFile.meta.id) }, | ||
| name: part.name ?? file.meta.name, |
There was a problem hiding this comment.
When a named source.kind === 'file' image has an unsupported model MIME type, such as AVIF, the earlier downgrade branch constructs both the notice and file attachment from file.meta.name and returns before reaching this new part.name fallback. Consequently supported uploaded images preserve the request's filename while unsupported uploaded images silently revert to the upload-store name; apply the same part.name ?? file.meta.name selection in the downgrade branch.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54d002d2ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const imageContentSchema = z.object({ | ||
| type: z.literal('image'), | ||
| source: imageSourceSchema, | ||
| name: z.string().min(1).optional(), |
There was a problem hiding this comment.
When a consumer parses prompt responses, message history, or prompt.submitted/prompt.steered events through @moonshot-ai/protocol, packages/protocol/src/message.ts still omits name from both media schemas, so Zod strips the newly emitted field and the exported ImageContent/VideoContent types cannot expose it. Update the shared image and video schemas alongside this kap-server schema so protocol consumers actually retain the filename.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Related Issue
No linked issue. This is the server-side companion for MoonshotAI/kimi-code-app#532.
Problem
The file store retained original filenames, but image and video message parts dropped them while passing through kap-server, agent events, and transcript reconstruction. Clients therefore lost media filenames after an authoritative transcript refresh, a cold rebuild, or steering.
What changed
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.