-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: add GitHub Copilot Provider that support agent mode #7010 #7072
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
…7010 - Implemented the Copilot component with authentication and model selection features. - Added tests for the Copilot component covering authentication states, error handling, and model management. - Updated localization files to include Copilot-related strings in multiple languages. - Enhanced the model validation functions to support Copilot models. - Modified the useSelectedModel hook to handle Copilot model selection. - Updated the index file to export the Copilot component.
|
||
const handler = new CopilotHandler(mockOptions) | ||
|
||
await expect(handler.completePrompt("Test prompt")).rejects.toThrow(" Copilotetion error: API Error") |
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.
Typographical issue: The error message string contains "Copilotetion error: API Error" which appears to be a typo. Perhaps it should be something like "Copilot completion error: API Error"?
await expect(handler.completePrompt("Test prompt")).rejects.toThrow(" Copilotetion error: API Error") | |
await expect(handler.completePrompt("Test prompt")).rejects.toThrow(" Copilot completion error: API Error") |
timeout: getApiRequestTimeout(), | ||
}) | ||
} catch (error) { | ||
throw new Error(`Failed to authenticate with Copilot: ${error}`) |
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.
Typographical error: The error message 'Failed to authenticate with Copilot: ...' contains an extra space before 'Copilot'. Please remove the extra space.
throw new Error(`Failed to authenticate with Copilot: ${error}`) | |
throw new Error(`Failed to authenticate with Copilot: ${error}`) |
return response.choices[0]?.message.content || "" | ||
} catch (error) { | ||
if (error instanceof Error) { | ||
throw new Error(` Copilotetion error: ${error.message}`) |
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.
Typographical error: The error message ' Copilotetion error: ...' has a leading extra space and a misspelling. Consider revising it to something like 'Copilot completion error: ...' to improve clarity.
throw new Error(` Copilotetion error: ${error.message}`) | |
throw new Error(`Copilot completion error: ${error.message}`) |
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.
Thank you for your contribution! I've reviewed the GitHub Copilot Provider implementation and found it to be well-structured overall. The OAuth device flow authentication is properly implemented, and the TypeScript typing is comprehensive. However, there are some issues that need attention before merging.
timeout: getApiRequestTimeout(), | ||
}) | ||
} catch (error) { | ||
throw new Error(`Failed to authenticate with Copilot: ${error}`) |
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.
Typo: There's an extra space here - should be "Copilot" not " Copilot"
throw new Error(`Failed to authenticate with Copilot: ${error}`) | |
throw new Error(`Failed to authenticate with Copilot: ${error}`) |
return response.choices[0]?.message.content || "" | ||
} catch (error) { | ||
if (error instanceof Error) { | ||
throw new Error(` Copilotetion error: ${error.message}`) |
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.
Same typo here - should be "Copilot completion" not " Copilotetion"
throw new Error(` Copilotetion error: ${error.message}`) | |
throw new Error(`Copilot completion error: ${error.message}`) |
} | ||
return result | ||
} catch (error) { | ||
console.error("Failed to fetch Copilot:", error) |
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.
Incomplete error message - should be "Failed to fetch Copilot models:" or similar
console.error("Failed to fetch Copilot:", error) | |
console.error("Failed to fetch Copilot models:", error) |
/** | ||
* Determine the X-Initiator header based on message roles | ||
*/ | ||
private determineInitiator(messages: Anthropic.Messages.MessageParam[]): string { |
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.
Is this X-Initiator logic correct? The method seems complex and the logic for determining 'agent' vs 'user' might not cover all edge cases. Could you add more comments explaining the rationale, or consider simplifying this?
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.
Yeah. This method is mainly used to determine whether the last request was sent by the user. However, there is currently no such flag, which can only determine whether the text in the message has the specified tag.
|
||
private constructor() { | ||
// Store tokens in user's home directory | ||
this.tokenDir = join(homedir(), ".roo-code", "copilot") |
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.
Security concern: Tokens are stored in plaintext at ~/.roo-code/copilot/tokens.json. Have you considered using VSCode's secret storage API instead for better security?
Example:
await context.secrets.store('copilot_access_token', accessToken)
apiKey: "placeholder", | ||
defaultHeaders: { | ||
...DEFAULT_HEADERS, | ||
"editor-version": "vscode/1.85.1", |
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.
These version strings are hard-coded. Could they be made configurable or pulled from the actual VSCode/extension version?
"editor-version": "vscode/1.85.1", | |
"editor-version": vscode.version, | |
"editor-plugin-version": `copilot/${extensionVersion}`, |
await this.ensureAuthenticated() | ||
|
||
// Use the authenticated client to fetch models | ||
const response = await fetch(`${this.client.baseURL}models`, { |
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.
Missing rate limiting logic. GitHub APIs have rate limits - should we add retry logic with exponential backoff for resilience?
vi.mock("../transform/openai-format") | ||
vi.mock("../transform/model-params") | ||
|
||
describe("CopilotHandler", () => { |
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.
Test coverage could be improved - consider adding tests for:
- Error recovery scenarios
- Token refresh logic
- Rate limiting behavior
- Full authentication flow including timeout scenarios
Related GitHub Issue
Closes: #7010
Description
VS Code LM API cannot achieve multiple use of the tool consuming only one advanced request. Therefore, I implemented a Copilot Provider, mimicking the Copilot authentication process found at https://github.com/BerriAI/litellm and https://github.com/sst/opencode.
In my own test, I chose claude-4 and had him write a web version of Angry Birds. This involved reading and writing eight files, requiring approximately 20 tool calls and consuming only one premium requeust.
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
Adds GitHub Copilot Provider with agent mode, including authentication, model selection, and validation, with tests and localization updates.
CopilotHandler
incopilot.ts
for GitHub Copilot Provider with authentication and model selection.webviewMessageHandler.ts
to handle Copilot authentication and model requests.validate.ts
anduseSelectedModel.ts
.copilot.test.ts
andwebviewMessageHandler.copilot.test.ts
.provider-settings.copilot.test.ts
.Copilot
component inApiOptions.tsx
for UI integration.provider-settings.ts
to include Copilot schema.This description was created by
for 5b02f1b. You can customize this summary. It will automatically update as commits are pushed.