support Custom Endpoints #26
Conversation
Summary of ChangesHello @m7medVision, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of the AI commit message generator by allowing users to specify custom API endpoints for their chosen AI providers. This change facilitates integration with local AI models like Ollama, enterprise proxy servers, or other OpenAI-compatible services, moving away from hardcoded endpoints. Concurrently, the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces custom API endpoint support for providers, removes OpenRouter, and updates configurations. The code changes include modifications to the README, command-line tools, internal configurations, and provider implementations. I have focused on identifying potential issues related to correctness and error handling, and have provided suggestions for improvement.
| func validateEndpointURL(endpoint string) error { | ||
| if endpoint == "" { | ||
| return nil // Empty endpoint is valid (will use default) | ||
| } | ||
|
|
||
| parsedURL, err := url.Parse(endpoint) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid URL format: %w", err) | ||
| } | ||
|
|
||
| if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { | ||
| return fmt.Errorf("endpoint must use http or https protocol") | ||
| } | ||
|
|
||
| if parsedURL.Host == "" { | ||
| return fmt.Errorf("endpoint must have a valid host") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The validateEndpointURL function is duplicated in cmd/config.go. This duplication increases the risk of inconsistencies and makes maintenance more difficult. Consider creating a single, reusable validation function in a common utility package and calling it from both places.
| endpoint, err := config.GetEndpoint() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error getting endpoint: %v\n", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
This block retrieves the endpoint, but the error is only handled by printing to stderr and exiting. It would be better to propagate this error so that the calling function can handle it gracefully, potentially providing more context to the user or attempting a fallback mechanism.
Consider returning the error to allow the caller to handle it.
| func validateEndpointURL(val interface{}) error { | ||
| endpoint, ok := val.(string) | ||
| if !ok { | ||
| return fmt.Errorf("endpoint must be a string") | ||
| } | ||
|
|
||
| // Empty string is valid (uses default) | ||
| if endpoint == "" { | ||
| return nil | ||
| } | ||
|
|
||
| parsedURL, err := url.Parse(endpoint) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid URL format: %w", err) | ||
| } | ||
|
|
||
| if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { | ||
| return fmt.Errorf("endpoint must use http or https protocol") | ||
| } | ||
|
|
||
| if parsedURL.Host == "" { | ||
| return fmt.Errorf("endpoint must have a valid host") | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The validateEndpointURL function could benefit from more robust validation. Currently, it checks for http or https schemes and a valid host. However, it doesn't check for other potential issues, such as invalid characters, malformed URLs, or excessively long URLs. Consider adding additional checks to ensure that the endpoint URL is well-formed and safe to use.
For example, you could use a regular expression to validate the URL format or check the URL length to prevent buffer overflow issues.
| func validateEndpointURL(endpoint string) error { | ||
| if endpoint == "" { | ||
| return nil // Empty endpoint is valid (will use default) | ||
| } | ||
|
|
||
| parsedURL, err := url.Parse(endpoint) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid URL format: %w", err) | ||
| } | ||
|
|
||
| if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { | ||
| return fmt.Errorf("endpoint must use http or https protocol") | ||
| } | ||
|
|
||
| if parsedURL.Host == "" { | ||
| return fmt.Errorf("endpoint must have a valid host") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The validateEndpointURL function could benefit from more robust validation. Currently, it checks for http or https schemes and a valid host. However, it doesn't check for other potential issues, such as invalid characters, malformed URLs, or excessively long URLs. Consider adding additional checks to ensure that the endpoint URL is well-formed and safe to use.
For example, you could use a regular expression to validate the URL format or check the URL length to prevent buffer overflow issues.
| func TestSetEndpoint_Validation(t *testing.T) { | ||
| // Reset configuration for clean test | ||
| cfg = nil | ||
| viper.Reset() | ||
|
|
||
| // Initialize config | ||
| InitConfig() | ||
|
|
||
| testCases := []struct { | ||
| endpoint string | ||
| valid bool | ||
| }{ | ||
| {"", true}, // Empty should be valid (default) | ||
| {"https://api.openai.com/v1", true}, // Valid HTTPS URL | ||
| {"http://localhost:11434", true}, // Valid HTTP URL | ||
| {"ftp://invalid.com", false}, // Invalid protocol | ||
| {"not-a-url", false}, // Invalid format | ||
| {"https://", false}, // Missing host | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| err := SetEndpoint("test", tc.endpoint) | ||
| if tc.valid && err != nil { | ||
| t.Errorf("Expected valid endpoint %s to pass, but got error: %v", tc.endpoint, err) | ||
| } else if !tc.valid && err == nil { | ||
| t.Errorf("Expected invalid endpoint %s to fail, but it passed", tc.endpoint) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The test case "https://" is marked as invalid due to the missing host. However, url.Parse in Go handles this case without error, resulting in an empty Host field. While the current validation logic correctly identifies this as invalid, the test description might be misleading. Consider updating the test description to accurately reflect the behavior of url.Parse and the validation logic.
This pull request introduces support for custom API endpoints per provider, allowing users to configure and use alternative or local AI inference servers for commit message generation. It removes OpenRouter as a built-in provider and updates the configuration, CLI, and documentation to reflect these changes. The main themes are provider configuration flexibility, codebase simplification, and improved validation.
Provider configuration and flexibility:
openai,copilot) via the newendpoint_urlfield in configuration, including CLI and interactive config support, and validation for endpoint URLs. [1] [2] [3] [4] [5]Documentation and model selection:
README.mdto document custom endpoint configuration, removed OpenRouter as a built-in provider, and added examples for local and alternative endpoints. [1] [2] [3]Testing and validation:
Minor fixes:
openaifor provider-related tests. [1] [2] [3]These changes collectively make the tool more flexible for enterprise, local, or alternative AI model usage, while simplifying the provider configuration experience.