Skip to content

fix: reduce keychain prompts on startup#10669

Merged
dvargasfuertes merged 1 commit into
mainfrom
dvargas/fix-keychain-prompts
Feb 28, 2026
Merged

fix: reduce keychain prompts on startup#10669
dvargasfuertes merged 1 commit into
mainfrom
dvargas/fix-keychain-prompts

Conversation

@dvargasfuertes
Copy link
Copy Markdown
Contributor

@dvargasfuertes dvargasfuertes commented Feb 28, 2026

Summary

  • APIKeyManager.migrateIfNeeded() already reads the anthropic key via /usr/bin/security CLI to check if migration is needed — but getKey(for:) was discarding the result and spawning a second /usr/bin/security process to read the same value
  • Each /usr/bin/security process spawn triggers a separate macOS keychain authorization prompt
  • Now migrateIfNeeded() returns the key it read, and getKey(for:) reuses it — eliminating one of the three prompts on startup (3 → 2)

The three prompts were:

  1. migrateIfNeeded()cliGetKey("anthropic") → process feat: initialize Next.js app in /web directory #1
  2. getKey(for:)cliGetKey("anthropic") → process feat: add platform terraform for GKE deployment #2 (eliminated)
  3. Daemon loadConfig()getSecureKey("anthropic") → process ci: add terraform apply workflow on platform changes #3

Test plan

  • Build and launch the macOS app
  • Verify only 2 keychain prompts appear on startup instead of 3
  • Verify API key read still works correctly after the change

🤖 Generated with Claude Code


Open with Devin

migrateIfNeeded() already reads the anthropic key via `security
find-generic-password` to check if migration is needed. Previously,
getKey(for:) would discard that result and spawn a second
`/usr/bin/security` process to read the same value — each process
spawn triggers a separate macOS keychain authorization prompt.

Now migrateIfNeeded() returns the key it read, and getKey(for:) reuses
it directly. This eliminates one of the three keychain prompts users
see on every app launch.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d29878323

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

]
SecItemDelete(deleteQuery as CFDictionary)

return key
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Confirm migrated key persisted before returning it

In the migration path, cliSetKey(...) is fire-and-forget, but this function now unconditionally returns key and getKey(for:) short-circuits on that value. If writing the new keychain item fails (for example, the keychain authorization is denied), callers will still see a non-nil API key while the daemon cannot read it from the shared entry, causing an inconsistent "key configured" state that was previously surfaced as nil on read. Re-read the shared entry (or otherwise verify write success) before returning the migrated key.

Useful? React with 👍 / 👎.

@dvargasfuertes dvargasfuertes merged commit 8cc0ee0 into main Feb 28, 2026
1 check passed
@dvargasfuertes dvargasfuertes deleted the dvargas/fix-keychain-prompts branch February 28, 2026 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant