Skip to content

fix: reduce keychain prompts on startup #10669

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.

Sign up for GitHub

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

Merged
dvargasfuertes merged 1 commit into from
Feb 28, 2026
Merged

fix: reduce keychain prompts on startup #10669

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions clients/macos/vellum-assistant/App/APIKeyManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,12 @@ enum APIKeyManager {
// MARK: - Generic provider access

static func getKey(for provider: String) -> String? {
if provider == "anthropic" { migrateIfNeeded() }
if provider == "anthropic" {
// migrateIfNeeded returns the key if it was already read during
// the migration check, avoiding a redundant security CLI spawn
// (each spawn triggers a macOS keychain authorization prompt).
if let cached = migrateIfNeeded() { return cached }
}
return cliGetKey(service: service, account: provider)
}

Expand Down Expand Up @@ -102,9 +107,12 @@ enum APIKeyManager {
// MARK: - Migration

/// One-time migration from the legacy keychain entry to the daemon-shared entry.
private static func migrateIfNeeded() {
// Skip if new entry already exists
if cliGetKey(service: service, account: "anthropic") != nil { return }
/// Returns the current anthropic key if it was read during the check, so the
/// caller can reuse it without a second CLI invocation.
@discardableResult
private static func migrateIfNeeded() -> String? {
// Skip if new entry already exists — return the value we just read
if let existing = cliGetKey(service: service, account: "anthropic") { return existing }

// Read from legacy entry (uses Security.framework since the old entry was created with SecItemAdd)
let legacyQuery: [String: Any] = [
Expand All @@ -117,7 +125,7 @@ enum APIKeyManager {
var result: AnyObject?
guard SecItemCopyMatching(legacyQuery as CFDictionary, &result) == errSecSuccess,
let data = result as? Data,
let key = String(data: data, encoding: .utf8) else { return }
let key = String(data: data, encoding: .utf8) else { return nil }

// Write to new entry via CLI (no ACL restrictions)
cliSetKey(service: service, account: "anthropic", value: key)
Expand All @@ -129,6 +137,8 @@ enum APIKeyManager {
kSecAttrAccount as String: legacyAccount
]
SecItemDelete(deleteQuery as CFDictionary)

return key

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 28, 2026

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 👍 / 👎.

}

private static func notifyKeyDidChange() {
Expand Down