Conversation
d769ad1 to
cc972d7
Compare
dcf89bb to
54c60ac
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54c60ac13c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b94ba49ee7
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c611c54f5
ℹ️ 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".
|
jvsena42
left a comment
There was a problem hiding this comment.
Three LOW notes inline and no HIGH/MEDIUM. Pubky Ring adoption sits behind the Paykit UI flag, so all of this reaches only opted-in users on released builds.
Checked:
- Borrowed secret is never persisted.
persistSessionAccess(shouldStoreLocalSecret:false)handles that. It throws if a local secret already exists, the backup snapshot is nil while a reference exists, and the receiver marker is suppressed for borrowed identities. validate. It pins version, sourceApp, pubky and a 64-hex secret, and re-derives the public key. A Ring record can't be attributed to another pubky, and ato.bitkit:record can't be adopted as Ring's.- No secrets in logs, URLs, the pasteboard or toasts.
- Approval sheet. It resolves the key for the displayed
publicKey, rejects local and shared keys coexisting, and approves the pinnedrequest.rawUrl. - Adoption rollback order. Reference first, then
signIn, then the pubky check. Failure runsclearExternalSessionAccess→deleteReference.createIdentityLockedanduseSharedRingIdentityLockedrefuse to run over existing state. - Lifecycle lock. No nested acquisition.
- AppReset. Mirrors are deleted and re-verified before private state is touched.
wipeEntireKeychainis now scoped toEnv.keychainGroup. The fail-closed wipe is deliberate and tested, and thepubky.sharedentitlement is present for Debug and Release. - Upgrade from v2.5.0. Existing keys are unchanged. With no reference,
resolveSessionInitializationrestores as before, and legacy relay sessions pass the new provenance guards. - Parity with synonymdev/bitkit-android#1109. Android disconnects on unavailable too, but it reads through a ContentProvider, so the locked-Keychain case below is iOS-only.
- Figma, Handoff v62. Contacts Import, the identity cards, the key icon, the
pubkyGreentoken and the title/illustration layout all match.
14de1dd to
ab2d476
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2d476f02
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cdcb0c45e
ℹ️ 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".
|
@codex review |
jvsena42
left a comment
There was a problem hiding this comment.
Delta since my last pass (ab2d476f, 930ed301, f738e631, 9b7cde64, merge 5cdcb0c4): no HIGH, no MEDIUM. One LOW follow-up on my existing thread: disconnecting on definitive source loss still leaves the borrowed key's Paykit endpoints published.
Checked against the Android twin synonymdev/bitkit-android#1109:
createIdentityover a restored legacy Ring session: absent here.createIdentityLockedrefuses a stored.paykitSessionwith no.pubkySecretKey. It shares the lifecycle lock withinitializeLocked, and.profileswaps toProfileViewonceisAuthenticatedflips.- Private endpoints on Ring loss: present, and broader. iOS removes neither public nor private endpoints on that path (thread reply).
Clean in the delta:
ab2d476f.errSecItemNotFoundstill maps tosourceIdentityMissing. Locked or unavailable Keychain statuses, and non-vault errors, are deferred with the reference kept, so writes still fail source validation.930ed301. Every Paykit request/proof mutation revalidates the borrowed source and checks cancellation before writing. No direct SDK mutation remains outsidePubkyService, and the revalidation takes no lock.f738e631..walletbackup omits the SDK snapshot whenever a reference exists, and throws if the reference is unreadable, so the previous upload stays intact. Restoring a nil snapshot clears SDK state after the reference, session and secret are gone.- Borrowed secret. Still never persisted, backed up or logged. Keychain groups are unchanged.
9b7cde64. Create shows only after a successful empty discovery. Loading and error states show only their own card, and the error card has Retry. This matches what codex asked for.- Merge. No conflict resolutions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b7cde645b
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7e860ec8a
ℹ️ 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".
| @@ -636,12 +641,14 @@ struct PaykitPaymentRequestService { | |||
| acceptedPaymentEndpointIdentifiers: acceptedPaymentEndpointIdentifiers, | |||
There was a problem hiding this comment.
Gate all remaining SDK writes on source validation
When Ring revokes a borrowed identity while Bitkit remains foreground, this wrapper protects the proposal itself but not every subsequent mutation: synchronize() still invokes processPendingMessages() without revalidation, and ContactPaymentsService.setEnabled() reaches the public endpoint and receiver-marker SDK writers without this hook. Polling can therefore deliver a previously queued message, or the user can toggle Contact Payments, using the cached session after its owner has revoked access. Fresh evidence after the prior fix is these remaining write paths; enforce source validation at the common SDK mutation boundary rather than only around selected request operations.
Useful? React with 👍 / 👎.
jvsena42
left a comment
There was a problem hiding this comment.
Delta since 9b7cde64 (950899df, d7e860ec): no HIGH, no MEDIUM. One LOW inline. The endpoint-removal point on my Keychain thread is still open; d7e860ec addressed codex's local-cache point only. I added notes there on the ordering to use when fixing it.
Checked and clean:
950899dfgating.isPaykitMaintenanceAlloweddefaults to false, is reset on every.active, and is set from a validation that runs underidentityLifecycleLock. Adoption and sign-in serialize on the same lock, so maintenance never runs for an unvalidated identity. A Bitkit-owned identity validates immediately (no reference), so pending sign-out and profile-deletion retries still run on foreground.- Endpoints.
refreshPublicPaykitEndpointsreturning empty only reachesapplyPublishedEndpoints([])through callers that are already gated. User toggles thrownoSupportedEndpointinstead of unpublishing. d7e860ecordering. It runs session →closeAndClear→ reference. The reference is kept if session deletion throws, and a missing session doesn't loop.- Key material. Neither commit touches the borrowed secret. The backup snapshot is still omitted while a reference exists.
Part of #571. Builds on merged #636.
Description
pubky.sharedKeychain group.Mirrors are verified before pruning or private deletion. Missing entitlements defer mirroring without deleting private data.
Release gate
Merge/release after Ring #342. Apple must provision
pubky.sharedfor both App IDs, regenerate profiles, and pass signed two-app physical-device interoperability.Design
Bitkit handoff: Profile choice and contact import overview.
Validation
Current head
f0266f12contains masterf4d4dbe25; every PR commit is signed.git diff --checkpassed.Manual Tests
Tests used a disposable zero-balance simulator wallet and staging identities. Simulator results do not replace the signed physical-device release gate.
/pub/pubky.app/:rw; an independent requester verifies the exact key and scope.Automated coverage
PubkyAuthURLSchemeTests,PubkyProfileManagerTests,SharedPubkyIdentityTests,PaykitSdkClientConfigTests,PubkyAuthApprovalSheetTests,PubkyAuthRequestTests, andWalletViewModelReceiveTests.