-
-
Notifications
You must be signed in to change notification settings - Fork 407
feat: add support for Github Copilot as a provider #1564
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: master
Are you sure you want to change the base?
Conversation
bugbot run |
…rrors, token calculation (rn approximation)
bugbot run :( |
bugbot run |
should be fixed |
bugbot run |
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.
Bug: Refactoring Breaks Provider Validation
The createModelInstance
method was refactored, introducing a call to the undefined this.getProviderConstructor(model)
, which causes a runtime error for all non-GitHub Copilot providers. This refactoring also removed critical validation checks for model existence and API key presence, leading to less informative errors or failures when API keys are missing for these providers.
src/LLMProviders/chatModelManager.ts#L363-L378
obsidian-copilot/src/LLMProviders/chatModelManager.ts
Lines 363 to 378 in ba51103
new Notice(`Failed to set chat model: ${e.message}`); | |
ChatModelManager.chatModel = null; | |
} | |
} | |
async createModelInstance(model: CustomModel): Promise<BaseChatModel> { | |
if (model.provider === ChatModelProviders.GITHUB_COPILOT) { | |
const provider = new GitHubCopilotProvider(); | |
return new CopilotChatModel({ provider, modelName: model.name }); | |
} | |
const AIConstructor = this.getProviderConstructor(model); | |
const config = await this.getModelConfig(model); | |
return new AIConstructor(config); | |
} |
Bug: Constructor Ignores Config, Provider Uninitialized
The ChatGitHubCopilot
class's constructor ignores its config
parameter, creating an unconfigured GitHubCopilotProvider
. This suggests the class is either unused or incorrectly implemented.
src/LLMProviders/githubCopilotChatModel.ts#L84-L108
obsidian-copilot/src/LLMProviders/githubCopilotChatModel.ts
Lines 84 to 108 in ba51103
export class ChatGitHubCopilot { | |
private provider: GitHubCopilotProvider; | |
constructor(config: any) { | |
this.provider = new GitHubCopilotProvider(); | |
} | |
async send(messages: { role: string; content: string }[], model = "gpt-4") { | |
return this.provider.sendChatMessage(messages, model); | |
} | |
getAuthState() { | |
return this.provider.getAuthState(); | |
} | |
async startAuth() { | |
return this.provider.startDeviceCodeFlow(); | |
} | |
async pollForAccessToken() { | |
return this.provider.pollForAccessToken(); | |
} | |
async fetchCopilotToken() { | |
return this.provider.fetchCopilotToken(); | |
} | |
resetAuth() { | |
this.provider.resetAuth(); | |
} | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
omg mb |
fixed. i tested the other models too. do i have to provide api keys for the integration test that keeps failing? |
Can't wait for this to get merged |
sorry, i will review the code later |
ok ty, sorry if my code isn't perfect |
You can now authenticate with Github to use Github Copilot as a model provider.
#506
does not support streaming as it already bypasses CORS
does not support auto-getting available models