Enhance keychain initialization with custom password prompt for interactive terminal - #30
Conversation
WalkthroughAdds interactive password handling for the keyring in NewCliSession: sets a sanitized KeychainName and configures KeychainPasswordFunc to prompt for a password via terminal when interactive; errors when not attached to a terminal. Introduces necessary imports for terminal password reading. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as NewCliSession
participant TTY as Terminal
participant KR as Keyring
U->>CLI: Initialize session
CLI->>CLI: Sanitize serviceName -> KeychainName
CLI->>KR: Initialize with KeychainPasswordFunc
KR-->>CLI: Request password if locked
alt stdin is a terminal
CLI->>TTY: Prompt "Enter keychain password:"
TTY-->>CLI: term.ReadPassword() (no echo)
CLI->>KR: Provide password
KR-->>CLI: Continue
else non-interactive
CLI-->>U: Error: interactive terminal required
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frameworks/cli/cliSession.go (3)
110-116: Fix: invalid range over int (compile error).
rangeover an int is invalid. Use a classic for-loop.- for i := range chunks { + for i := 0; i < chunks; i++ { chunkKey := fmt.Sprintf("%s_chunk_%d", key, i) if err := c.keyring.Remove(chunkKey); err != nil { break } }
132-137: Fix: invalid range over int (compile error).Same issue here.
- for i := range chunks { + for i := 0; i < chunks; i++ { chunkKey := fmt.Sprintf("%s_chunk_%d", key, i) if err := c.keyring.Remove(chunkKey); err != nil { break } }
141-151: Fix: invalid range over int + undefined min() (compile errors).Use a classic loop and compute end safely without an undefined helper.
- for i := range chunks { - start := i * chunkSize - end := min(start+chunkSize, len(t)) + for i := 0; i < chunks; i++ { + start := i * chunkSize + end := start + chunkSize + if end > len(t) { + end = len(t) + } chunkKey := fmt.Sprintf("%s_chunk_%d", key, i) if err := c.keyring.Set(keyring.Item{ Key: chunkKey, Data: t[start:end], }); err != nil { return fmt.Errorf("failed to save token chunk %d: %w", i, err) } }
🧹 Nitpick comments (2)
frameworks/cli/cliSession.go (2)
191-202: Prompt UX/security: write prompt/errors to stderr, add newline, avoid syscall.
- Write prompt and errors to stderr (stdout may be piped).
- Add a newline after ReadPassword so the next output starts on a new line.
- Use term.ReadPassword(int(os.Stdin.Fd())) and drop syscall import.
- KeychainPasswordFunc: func(prompt string) (string, error) { + KeychainPasswordFunc: func(prompt string) (string, error) { if !term.IsTerminal(int(os.Stdin.Fd())) { return "", fmt.Errorf("cannot initialize keychain, please run in interactive terminal first to provide password") } - fmt.Printf("%s", prompt) - password, err := term.ReadPassword(int(syscall.Stdin)) - if err != nil { - fmt.Println("\nError reading password:", err) - return "", err - } - return string(password), nil + fmt.Fprint(os.Stderr, prompt) + password, err := term.ReadPassword(int(os.Stdin.Fd())) + fmt.Fprintln(os.Stderr) + if err != nil { + fmt.Fprintln(os.Stderr, "Error reading password:", err) + return "", err + } + return string(password), nil }})
6-8: Remove syscall import after switching to os.Stdin.Fd()."fmt" - "os" - "strings" - "syscall" + "os" + "strings"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frameworks/cli/cliSession.go(2 hunks)
🔇 Additional comments (2)
frameworks/cli/cliSession.go (2)
190-190: KeychainName change may break existing tokens; confirm migration/compat.Setting a custom KeychainName (vs default login keychain) can cause a new store, making previously saved tokens inaccessible without migration or fallback. Please confirm intended break or add detection/migration.
155-155: Ensure Go 1.19+ for fmt.Appendf or fall back to strconv
The function fmt.Appendf was introduced in Go 1.19 (pkg.go.dev, blog.carlana.net).
Verify that your module’s go.mod (at the project root) declares
go 1.19or later. I wasn’t able to locate a go.mod in the repo root—please confirm the module file path and its Go version.If you need to support Go <1.19, replace the call with strconv.Itoa to avoid the fmt.Appendf requirement:
— countData := fmt.Appendf(nil, "%d", chunks) + import "strconv" + countData := []byte(strconv.Itoa(chunks))
1 participant
Introduce a custom password prompt for keychain initialization, ensuring that the process requires an interactive terminal for password input. This change improves user experience by providing a secure way to enter passwords.