feat: prompt for missing API key in CLI and GUI#42
feat: prompt for missing API key in CLI and GUI#42gregoire22enpc wants to merge 1 commit intosilverstein:mainfrom
Conversation
When a summarization engine requires an API key (claude, openai, mistral) and none is found in the environment or config file: - CLI: interactive prompt after recording finishes, persists to config - GUI: modal dialog when selecting the engine or after recording completes API keys resolve with env var taking precedence over config file, so existing workflows that set ANTHROPIC_API_KEY etc. are unaffected. Changes: - Add ApiKeysConfig to Config with resolve/set helpers - Replace raw std::env::var() calls in summarize.rs with config.resolve_api_key() - CLI: ensure_api_key() runs after recording and before cmd_process/cmd_watch - GUI: cmd_set_api_key + cmd_check_api_key Tauri commands - GUI: API key modal in index.html, triggered on engine change or missing-api-key notice Made-with: Cursor
silverstein
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The resolve_api_key() fallback chain in summarize.rs is a nice improvement — env var → config file is a reasonable pattern.
However, this PR has a few issues that would need to be addressed before merging:
1. Interactive prompt breaks headless/agent usage (blocking)
ensure_api_key() calls stdin.read_line() without checking if stdin is a TTY. This will hang indefinitely when Minutes is invoked non-interactively — which is the primary usage mode for many users:
claude -p "record and summarize"(Claude Code headless)codex/geminiheadless agentsminutes watch(daemon — no terminal at all)- CI/CD or cron invocations
- Any programmatic use of
minutes process
We specifically designed the CLI to work without interactive prompts so that AI agents can drive it. This would be a regression.
If an interactive prompt is desired, it needs at minimum:
if atty::is(atty::Stream::Stdin) {
// prompt for key
} else {
// return error with instructions, don't block
}2. Plaintext API keys in config.toml (security concern)
Storing secrets in ~/.config/minutes/config.toml is a security downgrade from env-var-only. Config files get:
- Backed up to iCloud/Dropbox/Time Machine
- Accidentally committed to git
- Shared in bug reports ("paste your config")
- Read by any process with user permissions (no filesystem ACL)
Environment variables aren't perfect either, but they don't persist to disk. If config-file storage is desired, it should use the OS keychain (macOS Keychain, etc.) or at minimum a separate file with 0600 permissions — not inline in the general config.
3. std::env::set_var is unsound in multi-threaded code
Both the CLI (ensure_api_key) and Tauri (cmd_set_api_key) call std::env::set_var. This is unsound in multi-threaded programs and Rust is moving toward making it unsafe. The Tauri app is definitely multi-threaded. The key should be threaded through the Config struct instead.
4. No tests
None of the new public APIs (resolve_api_key, required_api_key_env, set_api_key, ApiKeysConfig) have unit tests.
The summarize.rs refactor (env var → config.resolve_api_key()) is genuinely good and I'd happily merge that part. The interactive prompting and config-file key storage need a different approach though.
|
Hi @silverstein, thanks for the comments. Yes indeed, good catch, they are remaining issues. That's why I left the PR in draft. I will make sure to address those! |
Summary
Changes
Test plan
Made with Cursor