-
Notifications
You must be signed in to change notification settings - Fork 18
Secure API storage #102
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
Secure API storage #102
Conversation
WalkthroughRefactors store access from package-level functions to an injected StoreMethods instance backed by OS keyring. Threads the Store through CLI commands (LLM setup/update and commit message creation). Adds keyring initialization in main and introduces utility functions for config path handling. Updates go.mod to include keyring dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Main as cmd/commit-msg/main.go
participant Store as store.KeyringInit()
participant CLI as cmd.StoreInit(...)
participant Cmd as cobra commands
participant LLM as SetupLLM/UpdateLLM/CreateCommitMsg
participant KR as OS Keyring
User->>Main: Run commit-msg
Main->>Store: KeyringInit()
Store-->>Main: *StoreMethods
Main->>CLI: StoreInit(*StoreMethods)
Main->>Cmd: Execute()
rect rgba(200,230,255,0.2)
note over Cmd,LLM: Store injected into command flows
Cmd->>LLM: SetupLLM(Store) / UpdateLLM(Store)
LLM->>KR: Save/Update/Delete API Key
KR-->>LLM: Result
Cmd->>LLM: CreateCommitMsg(Store, dryRun, autoCommit)
LLM->>KR: DefaultLLMKey()
KR-->>LLM: API Key
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/cli/llmSetup.go (1)
16-69: Guard against nil Store to avoid panics
SetupLLMnow dereferencesStoreimmediately. IfStoreInitis skipped (tests, alternate entrypoints, future reuse), this will panic before Cobra can surface a friendly error. Add a nil check up front (same forUpdateLLM) and return a descriptive error instead. For example:func SetupLLM(Store *store.StoreMethods) error { + if Store == nil { + return errors.New("credential store not initialized") + }Apply the same guard in
UpdateLLMto keep both code paths safe.cmd/cli/createMsg.go (1)
26-36: Handle nil Store before calling DefaultLLMKey
CreateCommitMsgdereferencesStoreright away. If the caller forgot to runStoreInit(easy in tests or other entrypoints), this panics instead of producing a user-friendly message. Add a guard before the first use:func CreateCommitMsg(Store *store.StoreMethods, dryRun bool, autoCommit bool) { + if Store == nil { + pterm.Error.Println("Credential store not initialized. Run: commit llm setup") + os.Exit(1) + }This keeps the CLI resilient and aligned with the other error paths.
📜 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)
cmd/cli/createMsg.go(1 hunks)cmd/cli/llmSetup.go(6 hunks)cmd/cli/root.go(4 hunks)cmd/cli/store/store.go(15 hunks)cmd/commit-msg/main.go(1 hunks)go.mod(2 hunks)utils/storeUtils.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/cli/createMsg.go (2)
cmd/cli/root.go (1)
Store(14-14)cmd/cli/store/store.go (1)
StoreMethods(19-21)
cmd/commit-msg/main.go (2)
cmd/cli/store/store.go (1)
KeyringInit(24-32)cmd/cli/root.go (2)
StoreInit(17-19)Execute(33-38)
cmd/cli/llmSetup.go (2)
cmd/cli/root.go (1)
Store(14-14)cmd/cli/store/store.go (1)
StoreMethods(19-21)
cmd/cli/root.go (3)
cmd/cli/store/store.go (1)
StoreMethods(19-21)cmd/cli/llmSetup.go (2)
SetupLLM(16-72)UpdateLLM(76-173)cmd/cli/createMsg.go (1)
CreateCommitMsg(26-250)
cmd/cli/store/store.go (2)
pkg/types/types.go (2)
Config(60-63)LLMProvider(5-5)utils/storeUtils.go (3)
GetConfigPath(30-68)CheckConfig(9-17)CreateConfigFile(19-28)
⏰ 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)
DFanso
left a comment
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.
LGTM 🎊
Description
Changed the storage of API keys from local config.json to OS credential manager.
Shifted the storage utilities like CheckConfig, CreateConfigFile, GetConfigPath to storeUtils
Type of Change
Related Issue
Fixes #(issue number)
#94
Changes Made
Testing
Checklist
Screenshots (if applicable)
Now config.json stores only Saved LLM and default LLM
Additional Notes
Tested with windows, need to test in other OS like MacOS and Linux
For Hacktoberfest Participants
Thank you for your contribution! 🎉
Summary by CodeRabbit
New Features
Improvements
Chores
Note: You may be prompted to re-enter your API key the first time after updating.