-
Notifications
You must be signed in to change notification settings - Fork 1.4k
image attachment support for CLI #1541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
From the logs I can see the images are attached, but when the image size is large, it gets dropped due to |
|
When image size is proper, the next error I got is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds image attachment support to the GitHub Copilot CLI chat participant. The implementation introduces a new ImageStorage class for managing uploaded images and updates the prompt resolver to process ChatReferenceBinaryData references.
Key Changes
- New image storage system that saves images to global storage with automatic cleanup of old files
- Enhanced prompt resolver to handle binary data references alongside existing file and diagnostic references
- Updated participant capabilities to advertise image attachment support
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/extension/chatSessions/vscode-node/copilotCLIImageSupport.ts |
New file implementing image storage with persistent file management and cleanup logic |
src/extension/chatSessions/vscode-node/copilotCLIPromptResolver.ts |
Added image handling logic and refactored reference processing to use parallel execution |
src/extension/chatSessions/vscode-node/copilotCLIChatSessionsContribution.ts |
Updated import path for CopilotCLIPromptResolver from agents directory to local chatSessions directory |
src/extension/chatSessions/vscode-node/chatSessions.ts |
Updated import path for CopilotCLIPromptResolver to reflect new location |
package.json |
Added supportsImageAttachments: true capability to copilotcli chat session configuration |
Comments suppressed due to low confidence (3)
src/extension/chatSessions/vscode-node/copilotCLIPromptResolver.ts:123
- The
diagnosticTextsandallRefsTextsarrays are being concurrently modified by multiple promises withinPromise.all(). While JavaScript arrays support concurrent push operations, this creates non-deterministic ordering of diagnostic and reference texts. Since these are displayed to users in the prompt, the inconsistent ordering could make the output confusing or harder to follow across different invocations with the same inputs.
src/extension/chatSessions/vscode-node/copilotCLIPromptResolver.ts:26 - The image storage initialization happens synchronously in the constructor via
new ImageStorage(extensionContext), butImageStorage.initialize()is async and not awaited. This means whenstoreImage()is called on line 47, the storage directory may not exist yet, potentially causing the image storage operation to fail.
src/extension/chatSessions/vscode-node/copilotCLIPromptResolver.ts:88 - When a file has multiple diagnostics, the same file attachment is added to the
attachmentsarray multiple times (once per diagnostic). This creates duplicate attachments in the output. Consider collecting unique file URIs and adding each file only once, or implementing deduplication logic before returning the attachments.
| export class ImageStorage { | ||
| private readonly storageDir: URI; | ||
|
|
||
| constructor(private readonly context: IVSCodeExtensionContext) { | ||
| this.storageDir = URI.joinPath(this.context.globalStorageUri, 'copilot-cli-images'); | ||
| this.initialize(); | ||
| } | ||
|
|
||
| private async initialize(): Promise<void> { | ||
| try { | ||
| await vscode.workspace.fs.createDirectory(this.storageDir); | ||
| await this.cleanupOldImages(); | ||
| } catch (error) { | ||
| console.error('ImageStorage: Failed to initialize', error); | ||
| } | ||
| } |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ImageStorage class is directly using vscode.workspace.fs and console.error instead of the proper service abstractions. According to the coding guidelines:
- Use
IFileServiceinstead ofvscode.workspace.fsfor file operations - Use
ILogServiceinstead ofconsole.errorfor logging
The class should inject these services in the constructor to maintain consistency with the project's service-oriented architecture.
| constructor(private readonly context: IVSCodeExtensionContext) { | ||
| this.storageDir = URI.joinPath(this.context.globalStorageUri, 'copilot-cli-images'); | ||
| this.initialize(); | ||
| } | ||
|
|
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initialize() method is called in the constructor but is async and not awaited. This means the storage directory creation and cleanup may not complete before storeImage() is called, potentially causing file operation failures. Consider making initialization synchronous or ensuring it completes before allowing image storage operations.
| constructor(private readonly context: IVSCodeExtensionContext) { | |
| this.storageDir = URI.joinPath(this.context.globalStorageUri, 'copilot-cli-images'); | |
| this.initialize(); | |
| } | |
| private constructor(private readonly context: IVSCodeExtensionContext) { | |
| this.storageDir = URI.joinPath(this.context.globalStorageUri, 'copilot-cli-images'); | |
| } | |
| static async create(context: IVSCodeExtensionContext): Promise<ImageStorage> { | |
| const instance = new ImageStorage(context); | |
| await instance.initialize(); | |
| return instance; | |
| } |
| console.error('ImageStorage: Failed to initialize', error); | ||
| } | ||
| } | ||
|
|
||
| async storeImage(imageData: Uint8Array, mimeType: string): Promise<URI> { | ||
| const timestamp = Date.now(); | ||
| const randomId = Math.random().toString(36).substring(2, 10); | ||
| const extension = this.getExtension(mimeType); | ||
| const filename = `${timestamp}-${randomId}${extension}`; | ||
| const imageUri = URI.joinPath(this.storageDir, filename); | ||
|
|
||
| await vscode.workspace.fs.writeFile(imageUri, imageData); | ||
| return imageUri; | ||
| } | ||
|
|
||
| async getImage(uri: URI): Promise<Uint8Array | undefined> { | ||
| try { | ||
| const data = await vscode.workspace.fs.readFile(uri); | ||
| return data; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } | ||
|
|
||
| async deleteImage(uri: URI): Promise<void> { | ||
| try { | ||
| await vscode.workspace.fs.delete(uri); | ||
| } catch { | ||
| // Already deleted | ||
| } | ||
| } | ||
|
|
||
| async cleanupOldImages(maxAgeMs: number = 7 * 24 * 60 * 60 * 1000): Promise<void> { | ||
| try { | ||
| const entries = await vscode.workspace.fs.readDirectory(this.storageDir); | ||
| const now = Date.now(); | ||
| const cutoff = now - maxAgeMs; | ||
|
|
||
| for (const [filename, fileType] of entries) { | ||
| if (fileType === vscode.FileType.File) { | ||
| const fileUri = URI.joinPath(this.storageDir, filename); | ||
| try { | ||
| const stat = await vscode.workspace.fs.stat(fileUri); | ||
| if (stat.mtime < cutoff) { | ||
| await vscode.workspace.fs.delete(fileUri); | ||
| } | ||
| } catch { | ||
| // Skip files we can't access | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error('ImageStorage: Failed to cleanup old images', error); |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using console.error for logging violates the coding guideline to use ILogService instead. All error logging should be done through the injected log service to maintain consistency and enable proper log management.
| try { | ||
| const stat = await vscode.workspace.fs.stat(fileUri); | ||
| if (stat.mtime < cutoff) { | ||
| await vscode.workspace.fs.delete(fileUri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be more efficient to do a Promise.all(..)
But thats fine, can change after this is merged.
No description provided.