Repository navigation
auth: capture Sign in with Apple / OAuth failures instead of swallowing them - #5600
Closed
lawrencecchen wants to merge 2 commits into
Closed
lawrencecchen wants to merge 2 commits into
lawrencecchen wants to merge 2 commits into
Conversation
Sign in with Apple and other OAuth failures were collapsed into the generic "Something went wrong. Please try again." message in AuthCoordinator's OAuth catch and captured nowhere, so a production auth break was invisible. - Add an injected AuthErrorReporting seam (with NoopAuthErrorReporting default) to AuthCoordinator. The OAuth catch reports the real, un-wrapped error with the flow + provider before mapping it to a display-safe AuthError. User cancellation is not reported. The thrown error type is unchanged, so the existing AuthError(displaySafe:) mapping and the iOS SignInView error rendering keep working (no regression). - Wire a Sentry-backed reporter at the macOS composition root (SentryAuthErrorReporter) that extracts the Stack error code (e.g. INVALID_APPLE_CREDENTIALS) and respects the telemetry opt-out. CmuxAuthRuntime stays SDK-free behind the AuthClient seam; iOS keeps the no-op. - iOS: give the existing ios_sign_in_failed PostHog event distinct failure_reason values for INVALID_APPLE_CREDENTIALS and related OAuth codes so a structural break (e.g. an unregistered Apple bundle id) is visible in analytics instead of collapsing into a generic oauth_error. Values are enum-like codes, never the error text or the user's email. - Tests: a failed OAuth sign-in reports the underlying error with the provider; a cancelled sign-in does not report. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…it files Address autoreview: - The macOS app signs in through HostBrowserSignInFlow (the web app's hosted sign-in), which completes via completeExternalSignIn(), not AuthCoordinator.signInWithApple(). Capture session-validation failures there too (flow "browser_oauth") so the primary macOS OAuth/Apple path is no longer invisible. Cancellation is still skipped. Adds a regression test. - Split AuthErrorReporting.swift into one-type-per-file (AuthErrorReporting, AuthErrorContext, NoopAuthErrorReporting) per the package organization rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces an error reporting seam for authentication sign-in failures in the Cmux auth framework. A new ChangesError Reporting Seam for Authentication Sign-In Failures
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR adds an
Confidence Score: 4/5Safe to merge — the change adds observability only and does not alter any auth flow control paths or user-visible behaviour. All production auth paths are unchanged: the new errorReporter.report call is fire-and-forget before the existing throw, and the noop default means every existing call site is unaffected. The only concrete fixes left are two minor style issues (double AuthError(displaySafe:) evaluation) and a missing symmetry test for browser-flow cancellation suppression. The two AuthCoordinator catch blocks have a minor double-evaluation pattern worth tidying, and AuthCoordinatorTests.swift is missing a completeExternalSignIn cancellation-suppression test. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI
participant AuthCoordinator
participant AuthClient
participant AuthErrorReporting
participant AuthError
UI->>AuthCoordinator: signInWithApple() / signInWithGoogle()
AuthCoordinator->>AuthClient: signInWithOAuth(provider:anchor:)
AuthClient-->>AuthCoordinator: throws OAuthError (e.g. INVALID_APPLE_CREDENTIALS)
AuthCoordinator->>AuthError: displaySafe(error)
alt "mapped != .cancelled"
AuthCoordinator->>AuthErrorReporting: "report(error:, context: flow=oauth, provider=…)"
Note over AuthErrorReporting: macOS: SentrySDK.capture()<br/>iOS/tests: NoopAuthErrorReporting
end
AuthCoordinator-->>UI: throw displayError ?? rawError
UI->>AuthCoordinator: completeExternalSignIn() [macOS browser flow]
AuthCoordinator->>AuthClient: currentUser(throwOnMissing:)
AuthClient-->>AuthCoordinator: throws (session validation failure)
AuthCoordinator->>AuthError: displaySafe(error)
alt "mapped != .cancelled"
AuthCoordinator->>AuthErrorReporting: "report(error:, context: flow=browser_oauth)"
end
AuthCoordinator-->>UI: throw displayError ?? rawError
Reviews (1): Last reviewed commit: "auth: also capture the macOS hosted-brow..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
411
to
424
| } catch { | ||
| // Capture the real, un-wrapped error before it collapses into the | ||
| // generic display message, so OAuth/Apple sign-in breaks (e.g. a | ||
| // Stack INVALID_APPLE_CREDENTIALS from the native Apple token | ||
| // exchange) stop being invisible. User cancellation is not a | ||
| // failure, so it is not reported. | ||
| if AuthError(displaySafe: error) != .cancelled { | ||
| errorReporter.report( | ||
| error: error, | ||
| context: AuthErrorContext(flow: "oauth", provider: provider) | ||
| ) | ||
| } | ||
| throw AuthError(displaySafe: error) ?? error | ||
| } |
Contributor
There was a problem hiding this comment.
AuthError(displaySafe: error) is evaluated twice: once for the cancellation guard and again in the throw. If the mapping ever becomes non-trivial (or a future change makes it fallible in a different way) the two calls could diverge. A single binding makes the intent unambiguous and keeps the two uses in sync.
Suggested change
| } catch { | |
| // Capture the real, un-wrapped error before it collapses into the | |
| // generic display message, so OAuth/Apple sign-in breaks (e.g. a | |
| // Stack INVALID_APPLE_CREDENTIALS from the native Apple token | |
| // exchange) stop being invisible. User cancellation is not a | |
| // failure, so it is not reported. | |
| if AuthError(displaySafe: error) != .cancelled { | |
| errorReporter.report( | |
| error: error, | |
| context: AuthErrorContext(flow: "oauth", provider: provider) | |
| ) | |
| } | |
| throw AuthError(displaySafe: error) ?? error | |
| } | |
| } catch { | |
| // Capture the real, un-wrapped error before it collapses into the | |
| // generic display message, so OAuth/Apple sign-in breaks (e.g. a | |
| // Stack INVALID_APPLE_CREDENTIALS from the native Apple token | |
| // exchange) stop being invisible. User cancellation is not a | |
| // failure, so it is not reported. | |
| let displayError = AuthError(displaySafe: error) | |
| if displayError != .cancelled { | |
| errorReporter.report( | |
| error: error, | |
| context: AuthErrorContext(flow: "oauth", provider: provider) | |
| ) | |
| } | |
| throw displayError ?? error | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
447
to
460
| } catch { | ||
| // The macOS hosted-browser flow (Sign in with Apple/Google via the | ||
| // web app) completes here, so capture its session-validation | ||
| // failures too; otherwise that primary macOS path stays invisible. | ||
| // The provider is not known at this seam (the browser ran the OAuth | ||
| // choice), so only the flow is reported. | ||
| if AuthError(displaySafe: error) != .cancelled { | ||
| errorReporter.report( | ||
| error: error, | ||
| context: AuthErrorContext(flow: "browser_oauth") | ||
| ) | ||
| } | ||
| throw AuthError(displaySafe: error) ?? error | ||
| } |
Contributor
There was a problem hiding this comment.
signInWithOAuth: AuthError(displaySafe: error) is called once for the cancellation check and again for the throw. Extracting to a let keeps both uses in sync.
Suggested change
| } catch { | |
| // The macOS hosted-browser flow (Sign in with Apple/Google via the | |
| // web app) completes here, so capture its session-validation | |
| // failures too; otherwise that primary macOS path stays invisible. | |
| // The provider is not known at this seam (the browser ran the OAuth | |
| // choice), so only the flow is reported. | |
| if AuthError(displaySafe: error) != .cancelled { | |
| errorReporter.report( | |
| error: error, | |
| context: AuthErrorContext(flow: "browser_oauth") | |
| ) | |
| } | |
| throw AuthError(displaySafe: error) ?? error | |
| } | |
| } catch { | |
| // The macOS hosted-browser flow (Sign in with Apple/Google via the | |
| // web app) completes here, so capture its session-validation | |
| // failures too; otherwise that primary macOS path stays invisible. | |
| // The provider is not known at this seam (the browser ran the OAuth | |
| // choice), so only the flow is reported. | |
| let displayError = AuthError(displaySafe: error) | |
| if displayError != .cancelled { | |
| errorReporter.report( | |
| error: error, | |
| context: AuthErrorContext(flow: "browser_oauth") | |
| ) | |
| } | |
| throw displayError ?? error | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
353
to
+372
| #expect(coordinator.isAuthenticated == false) | ||
| #expect(coordinator.isLoading == false) | ||
| } | ||
|
|
||
| @Test func browserFlowCompletionFailureIsReported() async { | ||
| // The macOS hosted-browser sign-in (Apple/Google via the web app) | ||
| // completes through completeExternalSignIn(); a validation failure | ||
| // there must reach the reporter so the primary macOS path is visible. | ||
| let client = FakeAuthClient() | ||
| await client.setThrowOnCurrentUser(AuthError.serverError(500, "auth_failed")) | ||
| let reporter = FakeAuthErrorReporter() | ||
| let (coordinator, _) = makeCoordinator(client: client, errorReporter: reporter) | ||
|
|
||
| await #expect(throws: (any Error).self) { | ||
| try await coordinator.completeExternalSignIn() | ||
| } | ||
|
|
||
| #expect(reporter.reports.count == 1) | ||
| #expect(reporter.reports.first?.context.flow == "browser_oauth") | ||
| } |
Contributor
There was a problem hiding this comment.
oauthCancellationIsNotReported verifies that AuthError.cancelled from native OAuth doesn't reach the reporter, but there's no symmetric test for completeExternalSignIn. The production guard is identical in both paths, so a future refactor that accidentally drops it from completeExternalSignIn would go undetected. A browserFlowCancellationIsNotReported test covering the same scenario via setThrowOnCurrentUser(AuthError.cancelled) would close the gap.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTests.swift`:
- Around line 98-101: Update the tests that inspect reporter.reports to also
assert the underlying error contract: after the existing checks on
reporter.reports.count and reports.first?.context.flow/provider, add an
assertion that reports.first?.error is the backend error with the expected
Stack/Auth error code (check the error's code/enum value used in your auth
stack), and in the browser-flow test explicitly assert
reports.first?.context.provider == nil (since provider is intentionally unknown
there); locate these changes around the tests referencing reporter.reports and
reports.first? to add the error-code and nil-provider assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28a6c2d6-0fe1-4633-bae2-f1c35b9233b7
📒 Files selected for processing (10)
Packages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Coordinator/AuthCoordinator.swiftPackages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Diagnostics/AuthErrorContext.swiftPackages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Diagnostics/AuthErrorReporting.swiftPackages/CmuxAuthRuntime/Sources/CmuxAuthRuntime/Diagnostics/NoopAuthErrorReporting.swiftPackages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTests.swiftPackages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/Fakes.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SignInView.swiftSources/Auth/MacAuthComposition.swiftSources/Auth/SentryAuthErrorReporter.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+98
to
+101
| #expect(reporter.reports.count == 1) | ||
| #expect(reporter.reports.first?.context.flow == "oauth") | ||
| #expect(reporter.reports.first?.context.provider == "apple") | ||
| } |
There was a problem hiding this comment.
Strengthen assertions to lock the “underlying error” contract.
These tests currently validate only flow/provider metadata; they would still pass if the reporter started receiving a wrapped display-safe error instead of the original backend error. Please also assert the captured reports.first?.error carries the expected Stack/Auth code, and in the browser-flow test assert context.provider == nil (provider is intentionally unknown at that seam).
Suggested assertion additions
@@
`#expect`(reporter.reports.count == 1)
`#expect`(reporter.reports.first?.context.flow == "oauth")
`#expect`(reporter.reports.first?.context.provider == "apple")
+ let oauthError = reporter.reports.first?.error as? AuthError
+ `#expect`(oauthError == AuthError.serverError(400, "INVALID_APPLE_CREDENTIALS"))
@@
`#expect`(reporter.reports.count == 1)
`#expect`(reporter.reports.first?.context.flow == "browser_oauth")
+ `#expect`(reporter.reports.first?.context.provider == nil)
+ let browserError = reporter.reports.first?.error as? AuthError
+ `#expect`(browserError == AuthError.serverError(500, "auth_failed"))Also applies to: 370-372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/CmuxAuthRuntime/Tests/CmuxAuthRuntimeTests/AuthCoordinatorTests.swift`
around lines 98 - 101, Update the tests that inspect reporter.reports to also
assert the underlying error contract: after the existing checks on
reporter.reports.count and reports.first?.context.flow/provider, add an
assertion that reports.first?.error is the backend error with the expected
Stack/Auth error code (check the error's code/enum value used in your auth
stack), and in the browser-flow test explicitly assert
reports.first?.context.provider == nil (since provider is intentionally unknown
there); locate these changes around the tests referencing reporter.reports and
reports.first? to add the error-code and nil-provider assertions.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Sign in with Apple (and other OAuth) failures were collapsed into the generic "Something went wrong. Please try again." in
AuthCoordinator's OAuth catch and captured nowhere. A real auth break was invisible: no Sentry event on macOS, and on iOS the sign-in catch had no error capture at all.This PR makes that failure path observable. It does not change the thrown error type, so the existing
AuthError(displaySafe:)mapping and the iOSSignInViewerror rendering keep working (no regression).What
AuthErrorReportingseam (injected,NoopAuthErrorReportingdefault) onAuthCoordinator. Both the native-OAuth catch and the macOS hosted-browser completion path (completeExternalSignIn) report the real, un-wrapped error with the flow + provider before it is mapped to a display-safeAuthError. User cancellation is not reported.SentryAuthErrorReporter) wired at the composition root; it extracts the Stack error code (e.g.INVALID_APPLE_CREDENTIALS) and honors the telemetry opt-out.CmuxAuthRuntimestays SDK-free behind theAuthClientseam; iOS keeps the no-op.ios_sign_in_failedPostHog event now has distinctfailure_reasonvalues forINVALID_APPLE_CREDENTIALSand related OAuth codes, so a structural break is visible in analytics instead of collapsing into a genericoauth_error. Values are enum-like codes, never error text or the user's email.Context: the SIWA break (config/architecture, reported separately, not fixed by this PR)
iOS SIWA uses native
ASAuthorizationController; Stack validates the Apple identity token'saud(the app bundle id) against the project's registeredapple_bundle_ids. Tagged dev builds use per-tag bundle ids (dev.cmux.ios.<tag>) that can never be pre-registered, so native SIWA on dev tagged builds is structurally broken and returnsINVALID_APPLE_CREDENTIALS. The Apple client secret is valid (not expired) and TestFlight's bundle (dev.cmux.app.beta) is registered, so those axes are not the cause.Note: the user-reported symptom was the generic message, but
INVALID_APPLE_CREDENTIALSalready maps to a specific message on iOS. The generic message only appears for an unrecognized error code, so the user's actual failure is likely a different, still-unidentified code. This PR's capture + PostHog enrichment is what will surface that real code on the next attempt.Verification
swift test(CmuxAuthRuntime): 26/26 pass including the new reporter tests.🤖 Generated with Claude Code