Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThe changes simplify keychain storage and migration by removing explicit SecAccess configuration and eliminating active keychain item rewriting. The migration process now only marks completion via UserDefaults instead of performing keychain I/O operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1f8769a24
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // without prompting for the system password on every app launch. | ||
| kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, | ||
| kSecAttrAccess as String: secAccess | ||
| kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly |
There was a problem hiding this comment.
Preserve explicit keychain ACL when adding API keys
The new SecItemAdd query no longer includes kSecAttrAccess, so macOS falls back to a default ACL bound to the app’s current code signature; in this repo’s unsigned distribution context (dhavnii/UpdateManager.swift:5), that can cause existing saved keys to trigger password prompts again after rebuilds/updates because the signer identity changes. The removed SecAccessCreate(..., [] ...) path was the compatibility mechanism for that case, so this change reintroduces a launch-time keychain access regression for users who persist API keys.
Useful? React with 👍 / 👎.
| if allSucceeded { | ||
| UserDefaults.standard.set(true, forKey: migrationKey) | ||
| } | ||
| UserDefaults.standard.set(true, forKey: migrationKey) |
There was a problem hiding this comment.
Do not mark keychain migration complete before migrating
migrateKeychainAccessibility() now sets the migration flag immediately without rewriting any stored items, so users who still have legacy keychain entries will never be upgraded because later launches exit early on this flag. The previous implementation only marked completion after per-provider rewrite attempts; with this change, affected upgrades can remain permanently on legacy keychain settings unless users manually delete/re-enter keys.
Useful? React with 👍 / 👎.
@codex
Summary by CodeRabbit