Tell a paired phone when its saved key is unreadable - #1066
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
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 |
b96a3b1 to
85282fa
Compare
`hasPairedHost` answered two different questions with one boolean: "has this phone ever been paired" and "can this phone read the credential for the pairing it has". A Keychain entry that is missing, cleared out from under the app, or unreadable made a paired phone report as never-paired. What that looks like to a user: they pair, the sheet confirms the machine, and the accountless gate then re-presents the same connect sheet they just finished — with nothing said about why. They have already done the step it is asking for, so the app reads as ignoring them. It is the same user-facing class as the token_unreadable work in #1019; the accountless gate simply never got that treatment. `syncPairedHostCredentialState` now separates `notPaired` from `credentialUnreadable`, and `hasPairedHost` is defined as its `isUsable` case so every existing gate keeps its exact behaviour — an unusable credential still blocks entry, because without the secret the phone genuinely cannot use the pairing. Only the words change: the gate names the fault and offers "Pair again", which is the existing repair vocabulary on this platform (`laneOfflineAction`), rather than the unexplained loop. The classifier is pure and takes the credential lookup as a parameter, because the branch that matters is only reachable with a real Keychain fault. The test drives it directly with a persisted paired profile and a lookup that returns nil — standing in for the `errSecMissingEntitlement` (-34018) read failure — and asserts the state is `credentialUnreadable`, is not `notPaired`, and still gates entry. Found while diagnosing why a simulator build could pair over LAN and forget on relaunch. That turned out to be an unentitled build with no Keychain access at all, not a product defect — but it surfaced this gate, which is real and does affect a device whose Keychain entry goes bad. 1306 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
85282fa to
d56fc63
Compare
1 participant
A paired iPhone whose saved key cannot be read reported as never paired, so the accountless gate re-presented the same connect sheet the user had just completed — with nothing said about why.
The bug
hasPairedHostanswered two different questions with one boolean:MobileAccessGateViewthen doesif hasPairedHost { onContinue() } else { presentedSheet = .pairMachine }. A user whose Keychain entry is missing, cleared, or unreadable pairs successfully, watches the sheet confirm the machine, taps continue — and gets the pairing sheet again. Forever. They have already done the thing it is asking for, so it reads as the app ignoring them.This is the same user-facing class as the
token_unreadablework in #1019. The accountless gate simply never got that treatment.The fix
syncPairedHostCredentialStateseparates the two states:notPairedreadycredentialUnreadablehasPairedHostis now defined aspairedHostCredentialState.isUsable, so every existing gate keeps its exact behaviour — an unusable credential still blocks entry, because without the secret the phone genuinely cannot use that pairing. Only the words change. "Pair again" is the existing repair vocabulary on this platform (laneOfflineAction), not a new affordance.Test
The classifier is pure and takes the credential lookup as a parameter, because the branch that matters is only reachable with a real Keychain fault. The test drives it with a persisted paired profile and a lookup returning nil — standing in for the
errSecMissingEntitlement(-34018) read failure — and asserts the state iscredentialUnreadable, is notnotPaired, and still gates entry.How this was found
Diagnosing why a simulator build could pair over LAN and forget on relaunch. Evidence gathered:
ade.sync.hostProfileandhostProfilesare written on pair and survive relaunch (dumped from the app container before and after).-34018 "Client has neither application-identifier nor keychain-access-groups entitlements".codesign -d --entitlements :-returns an empty dict for every local build variant, because iOS Simulator builds here carry no entitlements.VQ372F39G6.com.ade.iosfrom an earlier properly-entitled build — same code path, works when entitled.So the thing that blocked local verification was an unentitled build, not a product defect, and real devices are unaffected by that symptom. But it surfaced this gate, which is real: a device whose Keychain entry goes bad gets the unexplained loop. That is what this PR fixes.
MobileTrustResetPolicyis not implicated — its flag stays unset precisely becauseclearAllConnectionTokens()returns false on the entitlement error, and it returns before its delete loop, so it deletes nothing.Verification
1306 tests, 13 failing cases — byte-identical to the clean-
mainbaseline.🤖 Generated with Claude Code
Greptile Summary
The PR separates an absent pairing from a saved pairing whose credential cannot be read, while preserving the existing access gate for unusable credentials.
hasPairedHostfrom its usability.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
.readyas the sole usable state.isUsable.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Load active or saved host profile] --> B{Paired profile exists?} B -- No --> C[notPaired] B -- Yes --> D{Credential lookup succeeds?} D -- Yes --> E[ready] D -- No --> F[credentialUnreadable] C --> G[Offer initial pairing] E --> H[Continue into app] F --> I[Explain saved-key fault] I --> J[Offer Pair again]Reviews (3): Last reviewed commit: "fix(ios): tell a paired phone its saved ..." | Re-trigger Greptile