Cli interface #46
Cli interface #46
Conversation
WalkthroughAdds a Cobra-based CLI for LLM provider management and commit-message generation, a JSON-backed local store for LLM configs, refactors main() to delegate to the CLI entrypoint, updates go.mod with prompt/CLI dependencies, and fixes a trailing newline in LICENSE. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Main as main()
participant CLI as cmd.Execute()
participant Cobra as Cobra rootCmd
participant LLMCmd as llm subcommands
participant Gen as CreateCommitMsg
participant Store as store (config.json)
participant Git as Git repo
participant Provider as LLM API
User->>Main: run binary
Main->>CLI: Execute()
CLI->>Cobra: rootCmd.Execute()
alt Manage LLM providers
User->>Cobra: llm setup / llm update
Cobra->>LLMCmd: RunE -> SetupLLM / UpdateLLM
LLMCmd->>Store: Save / ChangeDefault / UpdateAPIKey / DeleteModel
Store-->>LLMCmd: result
LLMCmd-->>User: confirmation / error
else Generate commit msg
User->>Cobra: commit (.)
Cobra->>Gen: CreateCommitMsg()
Gen->>Store: DefaultLLMKey()
Store-->>Gen: provider + api key
Gen->>Git: validate repo & get changes
Git-->>Gen: diffs
Gen->>Provider: request generation
Provider-->>Gen: generated message
Gen-->>User: display + clipboard attempt
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
LICENSE(0 hunks)cmd/cli/createMsg.go(1 hunks)cmd/cli/llmSetup.go(1 hunks)cmd/cli/root.go(1 hunks)cmd/cli/store/store.go(1 hunks)cmd/commit-msg/main.go(1 hunks)go.mod(3 hunks)
💤 Files with no reviewable changes (1)
- LICENSE
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/cli/createMsg.go (5)
cmd/cli/store/store.go (2)
DefaultLLMKey(147-189)Config(18-21)internal/git/operations.go (2)
IsRepository(16-23)GetChanges(26-130)pkg/types/types.go (1)
RepoConfig(10-13)internal/stats/statistics.go (1)
GetFileStatistics(14-74)internal/display/display.go (3)
ShowFileStatistics(20-96)ShowCommitMessage(99-115)ShowChangesPreview(118-133)
cmd/cli/llmSetup.go (1)
cmd/cli/store/store.go (6)
LLMProvider(13-16)Save(23-82)ListSavedModels(192-226)ChangeDefault(229-263)UpdateAPIKey(322-362)DeleteModel(266-319)
cmd/commit-msg/main.go (1)
cmd/cli/root.go (1)
Execute(24-29)
cmd/cli/root.go (2)
cmd/cli/llmSetup.go (2)
SetupLLM(11-48)UpdateLLM(50-113)cmd/cli/createMsg.go (1)
CreateCommitMsg(21-140)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cmd/cli/store/store.go (1)
291-294: Return an error when refusing to delete the default model.Line 294: When the target model is the default and other providers exist, the function prints a warning but returns
nil, causing the caller to treat the operation as successful. This was flagged in a previous review and remains unfixed.Apply this diff to return an error instead:
if Model == cfg.Default { if len(cfg.LLMProviders) > 1 { - fmt.Println("Please set other model as default, Cant delete default model") - return nil + return fmt.Errorf("cannot delete %s while it is the default; set another default first", Model)
🧹 Nitpick comments (5)
cmd/cli/store/store.go (5)
24-27: Remove unnecessary struct initialization.The
cfgvariable is initialized here but immediately overwritten when unmarshaling existing data from the config file (lines 53-58). This is inefficient and potentially confusing.Apply this diff to declare
cfgwithout initialization:- cfg := Config{ - LLMConfig.LLM, - []LLMProvider{LLMConfig}, - } + var cfg Config
86-94: Simplify the redundant condition.Line 89: The condition
err != nil || os.IsNotExist(err)is redundant becauseos.IsNotExist(err)is only meaningful whenerr != nil. The function can be simplified to just return whether the stat call succeeded.Apply this diff to simplify the logic:
func checkConfig(configPath string) bool { - - _,err := os.Stat(configPath) - if err != nil ||os.IsNotExist(err) { - return false - } - - return true + _, err := os.Stat(configPath) + return err == nil }
97-106: Rename function to match its behavior.The function is named
createConfigFilebut only creates the parent directory, not the file itself. This is misleading and could confuse future maintainers.Apply this diff to rename the function to better reflect its purpose:
-func createConfigFile(configPath string) error { +func ensureConfigDir(configPath string) error { err := os.MkdirAll(filepath.Dir(configPath), 0700) if err != nil {Then update all call sites (lines 37, etc.) to use the new name:
- err := createConfigFile(configPath) + err := ensureConfigDir(configPath)
181-186: Simplify by returning directly.Lines 183-184: The intermediate assignment to
useModelis unnecessary. You can return the address of the slice element directly.Apply this diff to simplify:
for i, p := range cfg.LLMProviders { if p.LLM == defaultLLM { - useModel= cfg.LLMProviders[i] - return &useModel, nil + return &cfg.LLMProviders[i], nil } }
216-217: Consider moving user-facing message to the caller.Line 216: This function mixes data access logic with user-facing output. The caller (CLI command) should handle presenting messages to the user based on the returned error, keeping this function focused on data operations.
Apply this diff to remove the user-facing message:
} else { - fmt.Println("Config file is empty, Please add atlead one LLM Key") return nil, errors.New("config file is empty") }The caller can then check for this specific error and display an appropriate message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cli/llmSetup.go(1 hunks)cmd/cli/store/store.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cli/llmSetup.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
cmd/cli/store/store.go (4)
47-49: Fix the awkward line break.The
else ifstatement is split across two lines unnecessarily, reducing readability.Apply this diff to fix the formatting:
- } else if - err != nil { + } else if err != nil { return err }
225-259: Validate that the model exists before setting it as default.Line 251 sets
cfg.Default = Modelwithout verifying thatModelexists incfg.LLMProviders. This can result in an invalid default, causingDefaultLLMKey()to fail with "not found default model in config".Apply this diff to add validation:
if len(data) > 0 { err = json.Unmarshal(data, &cfg) if err != nil { return err } } + // Validate that the model exists + found := false + for _, p := range cfg.LLMProviders { + if p.LLM == Model { + found = true + break + } + } + if !found { + return fmt.Errorf("model %s not found in configuration", Model) + } + cfg.Default = Model
290-296: Return an error when refusing to delete the default model.Line 293 returns
nilwhen the target is still the default and other providers exist. This causes the caller (which prints "model deleted") to report success even though nothing happened.Apply this diff:
if Model == cfg.Default { if len(cfg.LLMProviders) > 1 { - fmt.Println("Please set other model as default, Cant delete default model") - return nil + return fmt.Errorf("cannot delete %s while it is the default; set another default first", Model) } else { return os.WriteFile(configPath, []byte("{}"), 0600) }
345-356: Return an error if the model is not found.Lines 345-349 loop through
cfg.LLMProvidersbut never track whether a matching model was found. If the specifiedModeldoesn't exist, the function silently succeeds, writing the unchanged config back to disk and returningnil, misleading the caller.Apply this diff to track whether the update occurred:
+ found := false for i, p := range cfg.LLMProviders { if p.LLM == Model { cfg.LLMProviders[i].APIKey = APIKey + found = true + break } } + if !found { + return fmt.Errorf("model %s not found in configuration", Model) + } data, err = json.MarshalIndent(cfg, "", " ")
🧹 Nitpick comments (5)
cmd/cli/llmSetup.go (3)
20-23: Wrap the underlying error for better debugging.Line 22 returns a generic error message without including the underlying error from
prompt.Run(), making it harder to diagnose issues.Apply this diff to wrap the error:
_, model, err := prompt.Run() if err != nil { - return fmt.Errorf("prompt failed") + return fmt.Errorf("prompt failed: %w", err) }
47-47: Add newline to success message.Line 47 prints a success message without a trailing newline, which may cause formatting issues when subsequent output is displayed.
Apply this diff:
- fmt.Println("LLM model added") + fmt.Println("LLM model added\n")Or use
Printfwith\n:- fmt.Println("LLM model added") + fmt.Printf("LLM model added\n")
90-118: Move prompt declaration inside the switch case.Lines 90-92 declare
apiKeypromptbut it's only used in case 1 (line 103). Declaring it at the top of the function reduces readability and could lead to confusion.Additionally, lines 101, 111, and 117 are missing trailing newlines in their printf statements.
Apply this diff:
- apiKeyprompt := promptui.Prompt { - Label: "Enter API Key", - } - - switch opNo { case 0: err := store.ChangeDefault(model) if err != nil { return err } - fmt.Printf("%s set as default", model) + fmt.Printf("%s set as default\n", model) case 1: + apiKeyprompt := promptui.Prompt{ + Label: "Enter API Key", + Mask: '*', + } apiKey, err := apiKeyprompt.Run() if err != nil { return err } err = store.UpdateAPIKey(model, apiKey) if err != nil { return err } - fmt.Printf("%s API Key Updated", model) + fmt.Printf("%s API Key Updated\n", model) case 2: err := store.DeleteModel(model) if err != nil { return err } - fmt.Printf("%s model deleted", model) + fmt.Printf("%s model deleted\n", model) }Note: The diff also adds masking to the API key prompt (as recommended in the earlier comment).
cmd/cli/store/store.go (2)
24-27: Use named fields for struct initialization.Lines 24-27 use positional fields to initialize the
Configstruct. If the struct definition changes, this code will break silently or produce incorrect results.Apply this diff:
cfg := Config{ - LLMConfig.LLM, - []LLMProvider{LLMConfig}, + Default: LLMConfig.LLM, + LLMProviders: []LLMProvider{LLMConfig}, }
86-94: Simplify the condition.Line 89 has a redundant condition: if
err != nil, checkingos.IsNotExist(err)is unnecessary since any error should return false.Apply this diff:
func checkConfig(configPath string) bool { _,err := os.Stat(configPath) - if err != nil ||os.IsNotExist(err) { + if err != nil { return false } return true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cli/llmSetup.go(1 hunks)cmd/cli/store/store.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/cli/llmSetup.go (2)
cmd/cli/store/store.go (6)
LLMProvider(12-15)Save(22-83)ListSavedModels(191-222)ChangeDefault(225-259)UpdateAPIKey(318-358)DeleteModel(262-315)cmd/commit-msg/main.go (1)
main(22-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: GitGuardian Security Checks
|
Hey @GauravJangra9988 can u fix merge conflicts |
DFanso
added
enhancement
|
Made the required fixes |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
cmd/cli/store/store.go (5)
12-15: Consider renaming theLLMfield toModel.The field
LLMstores the model name (e.g., "OpenAI", "Claude"), but the struct is calledLLMProviderand the JSON tag is"model". Renaming the field toModelwould improve clarity and align with the JSON representation.Apply this diff:
type LLMProvider struct { - LLM string `json:"model"` + Model string `json:"model"` APIKey string `json:"api_key"` }Note: This change would require updating all references to
LLMProvider.LLMthroughout the codebase (inllmSetup.goand within this file).
24-27: Use named fields in struct initialization.The
Configstruct is initialized with positional arguments, which is fragile if fields are reordered or added in the future.Apply this diff:
cfg := Config{ - LLMConfig.LLM, - []LLMProvider{LLMConfig}, + Default: LLMConfig.LLM, + LLMProviders: []LLMProvider{LLMConfig}, }
169-169: Replace magic number with a descriptive constant or comment.The check
len(data) > 2assumes minimal JSON is{}(2 bytes). This could be clearer with either a named constant or a comment explaining the rationale.Apply this diff:
+ // Check if config has content beyond empty JSON object "{}" if len(data) > 2 {
226-260: Consider validation or making the function internal.While the current callers ensure the model exists before calling
ChangeDefault, as a public function it lacks validation. You have two options:
- Add validation (defense-in-depth for future callers)
- Make it internal by renaming to
changeDefaultif it's only meant for package useBased on learnings from past review: The author noted that the UI validates the model exists, so validation here may be redundant. However, public APIs benefit from defensive checks.
For option 1, validation:
if len(data) > 0 { err = json.Unmarshal(data, &cfg) if err != nil { return err } } + // Validate model exists + found := false + for _, p := range cfg.LLMProviders { + if p.LLM == Model { + found = true + break + } + } + if !found { + return fmt.Errorf("model %s not found in configuration", Model) + } + cfg.Default = Model
318-358: Consider validating that the model exists.Similar to
ChangeDefault, this public function silently succeeds even if the specified model doesn't exist. While the current UI ensures valid input, adding validation would make the API more robust.Based on learnings from past review: The author noted that the UI only allows selection of existing models. However, as this is an exported function, defensive validation would prevent silent failures if called from other contexts.
Apply this diff:
+ found := false for i, p := range cfg.LLMProviders { if p.LLM == Model { cfg.LLMProviders[i].APIKey = APIKey + found = true + break } } + if !found { + return fmt.Errorf("model %s not found in configuration", Model) + } data, err = json.MarshalIndent(cfg, "", " ")cmd/cli/llmSetup.go (2)
14-14: Consider defining provider types or constants.The hardcoded list of providers could be better organized. Per the comment from DFanso, consider:
- Defining provider constants or an enum
- Adding documentation about what API keys are expected for each
- Making the list extensible
Based on learnings from past review: DFanso requested adding types to the LLMs, and the author agreed.
Apply this approach:
+// Supported LLM providers +const ( + ProviderOpenAI = "OpenAI" + ProviderClaude = "Claude" + ProviderGemini = "Gemini" + ProviderGrok = "Grok" +) + +var supportedProviders = []string{ + ProviderOpenAI, + ProviderClaude, + ProviderGemini, + ProviderGrok, +} + func SetupLLM() error { - providers := []string{"OpenAI", "Claude", "Gemini", "Grok"} prompt := promptui.Select{ Label: "Select LLM", - Items: providers, + Items: supportedProviders, }
20-23: Preserve the underlying error for better debugging.Line 22: The error message "prompt failed" is generic and loses the context of what actually went wrong (e.g., user cancelled, I/O error, etc.).
Apply this diff:
_, model, err := prompt.Run() if err != nil { - return fmt.Errorf("prompt failed") + return fmt.Errorf("prompt failed: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cli/llmSetup.go(1 hunks)cmd/cli/store/store.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/cli/llmSetup.go (1)
cmd/cli/store/store.go (6)
LLMProvider(12-15)Save(22-82)ListSavedModels(192-223)ChangeDefault(226-260)UpdateAPIKey(318-358)DeleteModel(263-315)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Go Binary (windows-latest)
🔇 Additional comments (3)
cmd/cli/store/store.go (3)
107-145: LGTM!The platform-specific config path resolution is correctly implemented, following OS conventions (Windows LOCALAPPDATA, macOS Application Support, Linux XDG_CONFIG_HOME).
192-223: LGTM!The function correctly loads and returns the saved models configuration with appropriate error handling.
263-315: LGTM!The delete logic correctly handles the constraint that the default model cannot be deleted without first changing the default, and properly clears the config when deleting the only model.
|
hey @GauravJangra9988 can u update the documentation about new cli feature |
|
ok |
2 participants
Description
Used cobra cli to add, delete or update models or api keys instead of setting through environment variables.
Models and API keys data stores in local file system.
Along with Models a default model parameter is also stored to that.
Default model can be changed easily.
Clean command line interface to manage the commit-msg.
Type of Change
Related Issue
#37
Fixes #(issue number)
#37
Changes Made
Testing
Checklist
Screenshots (if applicable)
Additional Notes
Added cobra cli to set Models and API keys
Stores Models and API keys data locally on system
Multiple models can added and switch between them
User can add, delete or update the models or API keys
For Hacktoberfest Participants
Thank you for your contribution! 🎉
Summary by CodeRabbit
New Features
Chores
Documentation