Repository navigation
Add durable OAuth token refresh for EWS connections - #281
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary
Affected areas
Privacy and security
Configuration and migrations
Validation
WalkthroughThis change adds EWS OAuth setup, browser-based authorization, device-local token storage, refresh coordination, retry classification, tests, and documentation. ChangesEWS OAuth support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EWSSetupService
participant EWSOAuthService
participant AuthenticationServices
participant EWSAuthorizationPersisting
participant EWSMailboxConnectionAdapter
participant EWSOAuthRefreshCoordinator
participant EWSProvider
EWSSetupService->>EWSOAuthService: Start PKCE authorization
EWSOAuthService->>AuthenticationServices: Present system browser
AuthenticationServices-->>EWSOAuthService: Return callback and state
EWSOAuthService-->>EWSSetupService: Return OAuth tokens
EWSSetupService->>EWSAuthorizationPersisting: Save device-local authorization
EWSMailboxConnectionAdapter->>EWSOAuthRefreshCoordinator: Request provider authorization
EWSOAuthRefreshCoordinator->>EWSOAuthService: Refresh expiring token
EWSOAuthService-->>EWSOAuthRefreshCoordinator: Return refreshed tokens
EWSOAuthRefreshCoordinator->>EWSAuthorizationPersisting: Persist rotated tokens
EWSMailboxConnectionAdapter->>EWSProvider: Make authorized request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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 @.changeset/refresh-ews-oauth.md:
- Around line 1-4: Update the changeset frontmatter in refresh-ews-oauth.md by
regenerating it with pnpm changeset, selecting the affected package and
appropriate release type; preserve the existing OAuth summary in the body.
In `@apps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swift`:
- Around line 47-55: Update EWSSetupViewModel initialization so injected
authorizationStore and definitionSyncService are also used by connect() through
service. Either make service required or construct the default EWSSetupService
from those injected dependencies, ensuring reads and writes share one dependency
graph.
In `@apps/unwired-mail/unwired-mail/Services/DotEnvFile.swift`:
- Around line 27-31: Update DotEnvFile and the
EWSMailboxConnectionAdapter.value(...) resolution path so
EWS_OAUTH_CALLBACK_SCHEME is obtained only from the bundle/build setting, or
reject runtime values that differ from the registered URL scheme before
EWSOAuthRequest constructs redirect_uri. Keep other environment-backed settings
unchanged, and update README.md at the referenced configuration sections to
document that EWS_OAUTH_CALLBACK_SCHEME is build-time only.
In
`@apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift`:
- Around line 966-967: Synchronize InMemoryEWSAuthorizationStore’s values access
before relying on `@unchecked` Sendable: add a lock and wrap every read and
mutation in clearAll, connectionIds, load, and save with lock.withLock. Preserve
the existing persistence behavior and return values while ensuring all
dictionary access is serialized.
- Around line 1076-1091: Update refreshedTokens so only the caller that creates
the refreshTask schedules removal of refreshTasks[refreshKey], with the removal
performed by the task owner and guarded by the existing actor ownership check.
Remove the caller-wide defer so joiners and cancelled awaiters cannot clear an
unstructured task that is still running. Extend
testEWSConcurrentRefreshCallersShareOneTokenExchange with a third caller
arriving after a joiner resumes, verifying all callers share one token exchange.
- Around line 247-292: Update authenticate and its cancellation flow to prevent
concurrent authorizations from sharing or overwriting authenticationContinuation
and webAuthenticationSession. Reject or serialize a new authenticate call while
one is active, and ensure cancellation and completion only clear, cancel, and
resume state belonging to the active authorization rather than a newer session.
- Around line 213-245: The exchange method must prevent token request POST data
from being forwarded to another origin. Configure the URLSession used by
exchange with redirect handling that rejects redirects or permits only
same-origin targets, while preserving valid token exchanges; add a regression
test covering a cross-origin 307/308 redirect.
In `@apps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swift`:
- Around line 545-582: Add a sibling test alongside the existing refresh case
covering a token whose expiresAtMilliseconds is well beyond now plus the
five-minute leeway. Reuse the EWSMailboxConnectionAdapter setup, assert
oauthService.refreshCount is 0, and verify
client.loadedFolderAuthorizationCredentials contains the stored access token
rather than refreshed credentials.
- Around line 116-146: Add sibling tests near
testEWSOAuthRefreshMapsRejectedGrantToReauthorization covering an HTTP 503
transient refresh failure and asserting the thrown EWSOAuthError is not
.authorizationRejected. Add a coordinator-level failure-path test for
EWSOAuthRefreshCoordinator.refreshIfNeeded that simulates the same transient
failure and verifies the authorization record remains loadable, preserving the
refresh credential.
- Around line 6150-6167: Consolidate the duplicated request-body stream-reading
logic by making ewsRequestBodyText(_:) the shared extractor, then have both
ewsRequestBody(_:) and requestBody(_:) reuse it while preserving their
respective return types and fallback error behavior. Remove the standalone
stream loop from ewsRequestBody(_:) and keep the existing
httpBody/httpBodyStream handling semantics.
- Around line 51-63: Add tests around authorizationCode(from:) covering
rejection of callbacks with a mismatched redirect scheme or host, asserting the
appropriate callback validation error, and callbacks with an empty or missing
code, asserting EWSOAuthError.invalidAuthorizationCallback. Keep the existing
valid-code and invalid-state assertions unchanged.
- Around line 109-113: Extend the refresh-token request assertions in
EWSMailboxConnectionAdapterTests to verify that the captured body does not
contain any client-secret field or value. Keep the existing positive assertions
unchanged and use the existing body variable for the negative assertion.
- Around line 685-695: Update the shared-refresh test around
coordinator.refreshIfNeeded so it waits for an explicit signal that the second
caller has joined the in-flight refresh, rather than relying on Task.yield().
Only release refreshGate after that signal, while preserving the existing result
and refreshCount assertions.
- Around line 518-527: Extend
testEWSOAuthSetupUsesBrowserTokensAndPersistsRefreshCredential by capturing the
RecordingEWSDefinitionSyncService, encoding definitions.savedDefinition, and
asserting the encoded definition excludes both the access token and refresh
token. Keep the existing authorization and device-local persistence assertions
unchanged.
In `@docs/adr/0031-refresh-ews-oauth-on-device.md`:
- Around line 1-11: Update the ADR title/body terminology to use “On-Premises
Exchange Connection” instead of “supported on-premises EWS deployments.” Add a
concise consequences paragraph covering operator responsibility for supplying
and rotating provider endpoints and the client identifier, the user-visible
reauthorization prompt after invalid_grant, and loss of mailbox access when the
device’s Keychain authorization record is removed.
🪄 Autofix
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: 1efa8f0e-f7e0-48bd-9a9d-61ee39b38c5d
📒 Files selected for processing (10)
.changeset/refresh-ews-oauth.mdREADME.mdapps/unwired-mail/unwired-mail.xcodeproj/project.pbxprojapps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swiftapps/unwired-mail/unwired-mail/Info.plistapps/unwired-mail/unwired-mail/Services/DotEnvFile.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/DotEnvFileTests.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftdocs/adr/0031-refresh-ews-oauth-on-device.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Apple · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (7)
.changeset/**
📄 CodeRabbit inference engine (AGENTS.md)
Add a changeset with
pnpm changesetfor runtime behavior changes or exported type/API changes
Files:
.changeset/refresh-ews-oauth.md
apps/unwired-mail/**
📄 CodeRabbit inference engine (AGENTS.md)
Run
xcodebuild test -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17'for Apple app test coverage
Files:
apps/unwired-mail/unwired-mail.xcodeproj/project.pbxprojapps/unwired-mail/unwired-mail/Services/DotEnvFile.swiftapps/unwired-mail/unwired-mail/Info.plistapps/unwired-mail/unwired-mailTests/DotEnvFileTests.swiftapps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
apps/unwired-mail/**/*.swift: Use Swift formatting withswift-formatviaxcrunand validate withzsh scripts/check-apple-lint.zsh
Use SwiftLint in strict mode for linting Apple app code
Files:
apps/unwired-mail/unwired-mail/Services/DotEnvFile.swiftapps/unwired-mail/unwired-mailTests/DotEnvFileTests.swiftapps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mail/Services/DotEnvFile.swiftapps/unwired-mail/unwired-mailTests/DotEnvFileTests.swiftapps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/DotEnvFileTests.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swift
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Update README.md with developer setup, common commands, and high-level project navigation
Files:
README.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Treat docs as product truth, not marketing. Prefer precise domain language
from CONTEXT.md; flag undefined terms, synonym drift, and conflicts with
existing ADRs under docs/adr/. For ADR changes, require a clear decision,
consequences, and privacy or security boundary impact; flag silent
reversals of prior ADRs without an explicit supersession note. For
docs/agents/ and bootstrap docs, verify agent workflow, validation, and
triage guidance stay aligned with AGENTS.md. Flag broken cross-links,
speculative future claims presented as shipped behavior, and docs that
omit the Apple vs TypeScript ownership of a boundary they describe.
Files:
README.mddocs/adr/0031-refresh-ews-oauth-on-device.md
docs/adr/**
📄 CodeRabbit inference engine (AGENTS.md)
Document durable architecture decisions and privacy-boundary decisions in
docs/adr/
Files:
docs/adr/0031-refresh-ews-oauth-on-device.md
🪛 markdownlint-cli2 (0.23.2)
.changeset/refresh-ews-oauth.md
[warning] 4-4: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (11)
apps/unwired-mail/unwired-mailTests/DotEnvFileTests.swift (1)
61-65: LGTM!Also applies to: 79-83
apps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swift (2)
592-639: LGTM!
5724-5727: LGTM!Also applies to: 5761-5766, 5921-5948
apps/unwired-mail/unwired-mail.xcodeproj/project.pbxproj (1)
719-723: 🗄️ Data Integrity & IntegrationVerify that empty target settings do not mask local overrides.
The README says physical-device and Release builds can provide these values through
LocalSigning.xcconfig, but both application target configurations assign the five keys directly. If target-level settings overrideShared.xcconfig, the built Info.plist will contain empty OAuth endpoints, client ID, and scope, so OAuth will be unavailable outside dotenv-based Debug builds.Inspect the resolved
EWS_OAUTH_*values withxcodebuild -showBuildSettingsfor both Debug and Release after loading the intended local configuration. Move the defaults to the xcconfig or use inheritance if the local values are not preserved.Also applies to: 758-762
apps/unwired-mail/unwired-mail/Info.plist (1)
21-27: LGTM!Also applies to: 28-37
apps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swift (1)
363-376: LGTM!Also applies to: 389-391
apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift (5)
38-142: LGTM!
321-436: LGTM!
1194-1201: LGTM!Also applies to: 1244-1245, 1265-1266, 2149-2149, 2190-2194, 2283-2283
580-593: 🔒 Security & PrivacyReachability path
● Entry apps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swift:105 connect │ ▼ ● Sink apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftNo change is required for Keychain accessibility.
KeychainEWSAuthorizationStorewrites authorizations withkSecAttrAccessibleAfterFirstUnlockThisDeviceOnly. It does not setkSecAttrSynchronizable.
3742-3752: 🔒 Security & PrivacySensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External
Reachability path
● Entry apps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swift:105 connect │ ▼ ● Sink apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftConfirm that the EWS endpoint carrying the Bearer token is HTTPS-only.
This change routes every provider access through the coordinator, so an OAuth access token now travels as
Authorization: Bearertodefinition.endpointinSystemEWSClient.request(SystemEWSClient.swift Lines 783-855). The endpoint originates from user-entered setup input and passes throughEWSConnectionDefinition.validatedEndpoint, which is not in the provided context.EWSOAuthConfigurationenforces HTTPS for the provider endpoints, but that guard does not cover the EWS endpoint. Confirm thatvalidatedEndpointrejectshttp, so the access token never crosses a cleartext hop.#!/bin/bash # Description: Verify scheme enforcement for the EWS endpoint that receives the Bearer token. set -euo pipefail ast-grep run --pattern 'static func validatedEndpoint($$$) { $$$ }' --lang swift apps/unwired-mail rg -n -C12 'func validatedEndpoint' apps/unwired-mail
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
40-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExclude
EWS_OAUTH_CALLBACK_SCHEMEfrom the.env.locallist.Line 40 states that debug builds read "the
EWS_OAUTH_*settings" from.env.local.EWSOAuthConfiguration.bundledValue()readsEWS_OAUTH_AUTHORIZATION_ENDPOINT,EWS_OAUTH_CLIENT_ID,EWS_OAUTH_SCOPE, andEWS_OAUTH_TOKEN_ENDPOINTfrom the environment or.env.local, but it reads the callback scheme only fromBundle.mainInfo dictionary keyEWSOAuthCallbackScheme. A developer who follows line 40 and puts the callback scheme in.env.localgets a nil configuration with no error. Line 50 already states the build-time-only rule; make line 40 agree.📝 Proposed wording fix
-Debug builds running on the development Mac or in Simulator read `CONVEX_URL`, `CONVEX_SITE_URL`, the `EWS_OAUTH_*` settings, `GMAIL_OAUTH_CLIENT_ID`, `GMAIL_PUBSUB_TOPIC`, and `MICROSOFT_GRAPH_CLIENT_ID` from the repository-root `.env.local`; backend-only values are ignored. +Debug builds running on the development Mac or in Simulator read `CONVEX_URL`, `CONVEX_SITE_URL`, the `EWS_OAUTH_*` settings except `EWS_OAUTH_CALLBACK_SCHEME`, `GMAIL_OAUTH_CLIENT_ID`, `GMAIL_PUBSUB_TOPIC`, and `MICROSOFT_GRAPH_CLIENT_ID` from the repository-root `.env.local`; backend-only values are ignored.🤖 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 `@README.md` at line 40, Update the README sentence describing debug-build settings to list only the EWS OAuth values read from `.env.local`, excluding `EWS_OAUTH_CALLBACK_SCHEME`; preserve the existing guidance that this callback scheme is configured through the app’s bundled build-time setting as documented elsewhere.apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift (1)
1149-1176: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist rotated tokens from the owning task, not only from the awaiting caller.
Slot ownership is now correct: only the creating task removes
refreshTasks[refreshKey], and joiners no longer clear an in-flight entry. One rotation gap remains.
refreshedTokensreturns throughtry await refreshTask.value. If the awaiting caller is cancelled at that suspension point, the unstructuredrefreshTaskkeeps running and completes the token exchange. Persistence happens only inrefreshIfNeededafter the await, so the caller never reachesauthorizationStore.save. The provider has already consumed and rotated the refresh token, while the Keychain still holds the previous one.The next provider access then presents the consumed refresh token. A rotating provider answers
invalid_grant,refreshIfNeededclears the authorization, and the connection becomes authorization-required even though no credential was actually revoked. Provider access is routinely cancelled in this codebase, so this path is reachable:EWSSetupPanelcallstask?.cancel()on disappearance,invalidate()fences in-flight work, and the sync gate throwsCancellationError.Persist the refreshed tokens from the task that owns the exchange, so the rotation survives caller cancellation.
As per path instructions "Check actor isolation, cancellation, lifecycle transitions, persistence, retry and offline behavior, and Keychain accessibility."
🐛 Proposed fix to tie persistence to the exchange
Move the store write into the actor-isolated completion so it runs once per exchange, independent of which caller is still awaiting.
private func refreshedTokens( _ tokens: EWSOAuthTokens, - refreshKey: String + refreshKey: String, + productAccountId: String, + template: DeviceLocalEWSAuthorization ) async throws -> EWSOAuthTokens { let refreshTask: Task<EWSOAuthTokens, Error> if let inFlight = refreshTasks[refreshKey] { await onRefreshTaskJoined?() refreshTask = inFlight } else { let owner = UUID() refreshTask = Task { `@MainActor` [oauthService] in try await oauthService.refresh(tokens) } refreshTasks[refreshKey] = refreshTask refreshTaskOwners[refreshKey] = owner Task { [weak self] in - _ = await refreshTask.result - await self?.removeRefreshTask(refreshKey: refreshKey, owner: owner) + let result = await refreshTask.result + await self?.completeRefreshTask( + refreshKey: refreshKey, + owner: owner, + result: result, + productAccountId: productAccountId, + template: template + ) } } return try await refreshTask.value } - private func removeRefreshTask(refreshKey: String, owner: UUID) { + private func completeRefreshTask( + refreshKey: String, + owner: UUID, + result: Result<EWSOAuthTokens, Error>, + productAccountId: String, + template: DeviceLocalEWSAuthorization + ) { guard refreshTaskOwners[refreshKey] == owner else { return } refreshTasks[refreshKey] = nil refreshTaskOwners[refreshKey] = nil + guard case .success(let refreshedTokens) = result else { return } + try? authorizationStore.save( + DeviceLocalEWSAuthorization( + authorizationGeneration: template.authorizationGeneration, + credential: refreshedTokens.accessToken, + definition: template.definition, + hasOnlineArchive: template.hasOnlineArchive, + oauthTokens: refreshedTokens + ), + productAccountId: productAccountId + ) }
refreshIfNeededthen passescurrentas the template, and its ownauthorizationStore.savebecomes an idempotent rewrite of the same record. Add a test that cancels the sole caller during the exchange and asserts the storedoauthTokensequal the rotated tokens.🤖 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 `@apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift` around lines 1149 - 1176, Update refreshedTokens so the owning refreshTask persists the rotated EWSOAuthTokens after a successful oauthService.refresh, independent of caller cancellation; use the existing authorizationStore and preserve actor isolation and single-write-per-exchange behavior. Keep refreshIfNeeded’s save as the existing idempotent rewrite using current as its template, and add coverage that cancels the sole caller during refresh and verifies the stored oauthTokens are the rotated values.Source: Path instructions
🤖 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 `@apps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swift`:
- Around line 865-878: Synchronize the third caller in the coordinator refresh
test before releasing refreshGate. Update onRefreshTaskJoined to count joined
callers using the existing TestCounter pattern, wait until both the second and
third callers have joined the in-flight task, then release refreshGate; preserve
the assertions verifying all callers share freshTokens and
oauthService.refreshCount remains 1.
- Around line 533-556: Extend
testEWSOAuthTokenExchangeRejectsCrossOrigin307And308Redirects with positive
assertions that same-origin 307 and 308 redirects return the redirected request,
and add a non-307/308 status case verifying the policy’s expected behavior. Keep
the existing cross-origin rejection coverage and ensure the assertions exercise
EWSOAuthTokenRedirectPolicy.redirectedRequest directly.
---
Outside diff comments:
In
`@apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift`:
- Around line 1149-1176: Update refreshedTokens so the owning refreshTask
persists the rotated EWSOAuthTokens after a successful oauthService.refresh,
independent of caller cancellation; use the existing authorizationStore and
preserve actor isolation and single-write-per-exchange behavior. Keep
refreshIfNeeded’s save as the existing idempotent rewrite using current as its
template, and add coverage that cancels the sole caller during refresh and
verifies the stored oauthTokens are the rotated values.
In `@README.md`:
- Line 40: Update the README sentence describing debug-build settings to list
only the EWS OAuth values read from `.env.local`, excluding
`EWS_OAUTH_CALLBACK_SCHEME`; preserve the existing guidance that this callback
scheme is configured through the app’s bundled build-time setting as documented
elsewhere.
🪄 Autofix
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: d6da0e74-b6f5-4264-8f83-5e4a5807c5d5
📒 Files selected for processing (7)
README.mdapps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swiftapps/unwired-mail/unwired-mail/Services/DotEnvFile.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swiftapps/unwired-mail/unwired-mailTests/DotEnvFileTests.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftdocs/adr/0031-refresh-ews-oauth-on-device.md
💤 Files with no reviewable changes (2)
- apps/unwired-mail/unwired-mail/Services/DotEnvFile.swift
- apps/unwired-mail/unwired-mailTests/DotEnvFileTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Apple · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (6)
docs/adr/**
📄 CodeRabbit inference engine (AGENTS.md)
Document durable architecture decisions and privacy-boundary decisions in
docs/adr/
Files:
docs/adr/0031-refresh-ews-oauth-on-device.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Treat docs as product truth, not marketing. Prefer precise domain language
from CONTEXT.md; flag undefined terms, synonym drift, and conflicts with
existing ADRs under docs/adr/. For ADR changes, require a clear decision,
consequences, and privacy or security boundary impact; flag silent
reversals of prior ADRs without an explicit supersession note. For
docs/agents/ and bootstrap docs, verify agent workflow, validation, and
triage guidance stay aligned with AGENTS.md. Flag broken cross-links,
speculative future claims presented as shipped behavior, and docs that
omit the Apple vs TypeScript ownership of a boundary they describe.
Files:
docs/adr/0031-refresh-ews-oauth-on-device.mdREADME.md
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Update README.md with developer setup, common commands, and high-level project navigation
Files:
README.md
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
apps/unwired-mail/**/*.swift: Use Swift formatting withswift-formatviaxcrunand validate withzsh scripts/check-apple-lint.zsh
Use SwiftLint in strict mode for linting Apple app code
Files:
apps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift
apps/unwired-mail/**
📄 CodeRabbit inference engine (AGENTS.md)
Run
xcodebuild test -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17'for Apple app test coverage
Files:
apps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swiftapps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swift
🪛 LanguageTool
docs/adr/0031-refresh-ews-oauth-on-device.md
[style] ~15-~15: Consider using a different verb to strengthen your wording.
Context: ... device's Keychain authorization record removes mailbox access on that device while lea...
(REMOVE_REVOKE)
🔇 Additional comments (13)
README.md (1)
50-50: LGTM!apps/unwired-mail/unwired-mail/Features/Account/EWSSetupPanel.swift (1)
55-60: LGTM!Also applies to: 368-381, 394-396
apps/unwired-mail/unwired-mail/Services/Mailbox/EWSMailboxConnectionAdapter.swift (9)
117-119: LGTM!
213-248: LGTM!Also applies to: 337-373
250-334: LGTM!
421-435: LGTM!
1030-1077: LGTM!
1102-1147: LGTM!
1329-1330: LGTM!Also applies to: 1350-1351, 1371-1372, 1384-1385, 1457-1458, 2275-2279, 2368-2368
644-644: LGTM!Also applies to: 2247-2248
826-826: 🩺 Stability & AvailabilityNo additional synchronization is required for the remaining conformers.
KeychainEWSAuthorizationStoreandFailingConnectionIdEWSAuthorizationStorecontain only immutable state.InMemoryEWSAuthorizationStoreprotects its mutable dictionary withNSLock.> Likely an incorrect or invalid review comment.apps/unwired-mail/unwired-mailTests/EWSMailboxConnectionAdapterTests.swift (1)
64-74: LGTM!Also applies to: 125-126, 161-192, 600-603, 668-714, 765-813, 1583-1592, 5710-5710, 5724-5734, 6325-6353
docs/adr/0031-refresh-ews-oauth-on-device.md (1)
5-15: LGTM!
|
@coderabbitai review |
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a69b0837e
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@apps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swift`:
- Around line 62-83: Update testTransientEWSOAuthFailureRemainsPendingForRetry
to exercise the EWS path by using ewsConnection and assigning its id to the
message connectionId. Replace the count-only assertion with a load from
InMemoryPendingProviderActionStore that verifies the persisted action state is
.retrying. Keep the existing transient EWSOAuthError.tokenExchangeFailed failure
injection in PendingProviderActionService.perform so the test specifically
proves retry state retention rather than any generic stored action.
🪄 Autofix
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: 198e6a89-dc64-421c-9b52-a54f890ce88b
📒 Files selected for processing (4)
apps/unwired-mail/unwired-mail/Services/Mailbox/OutboxDeliveryService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/PendingProviderActionService.swiftapps/unwired-mail/unwired-mailTests/OutboxDeliveryServiceTests.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Apple · Unwired Mail
🧰 Additional context used
📓 Path-based instructions (3)
apps/unwired-mail/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
apps/unwired-mail/**/*.swift: Use Swift formatting withswift-formatviaxcrunand validate withzsh scripts/check-apple-lint.zsh
Use SwiftLint in strict mode for linting Apple app code
Files:
apps/unwired-mail/unwired-mail/Services/Mailbox/OutboxDeliveryService.swiftapps/unwired-mail/unwired-mailTests/OutboxDeliveryServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/PendingProviderActionService.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swift
⚙️ CodeRabbit configuration file
apps/unwired-mail/**/*.swift: Prioritize functional correctness, data protection, and Apple platform
behavior over style already enforced by swift-format and SwiftLint.
Check actor isolation, cancellation, lifecycle transitions, persistence,
retry and offline behavior, and Keychain accessibility. Flag credentials,
tokens, mail content, or decrypted Product Sync data that can be logged,
persisted outside approved device-local storage, or cross a documented
privacy boundary. Require focused tests for changed behavior and failure
paths.
Files:
apps/unwired-mail/unwired-mail/Services/Mailbox/OutboxDeliveryService.swiftapps/unwired-mail/unwired-mailTests/OutboxDeliveryServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/PendingProviderActionService.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swift
apps/unwired-mail/**
📄 CodeRabbit inference engine (AGENTS.md)
Run
xcodebuild test -project apps/unwired-mail/unwired-mail.xcodeproj -scheme unwired-mail -destination 'platform=iOS Simulator,name=iPhone 17'for Apple app test coverage
Files:
apps/unwired-mail/unwired-mail/Services/Mailbox/OutboxDeliveryService.swiftapps/unwired-mail/unwired-mailTests/OutboxDeliveryServiceTests.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/PendingProviderActionService.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swift
**/*Tests/**/*.swift
⚙️ CodeRabbit configuration file
**/*Tests/**/*.swift: Verify externally observable behavior, privacy invariants, error paths,
cancellation or restart behavior where relevant, and deterministic
isolation. Flag fragile sleeps, ordering dependencies, and tests that
duplicate the implementation instead of exercising it.
Files:
apps/unwired-mail/unwired-mailTests/OutboxDeliveryServiceTests.swiftapps/unwired-mail/unwired-mailTests/PendingProviderActionServiceTests.swift
🧠 Learnings (1)
📚 Learning: 2026-07-27T07:25:56.352Z
Learnt from: rajzik
Repo: unwired-dev/product PR: 201
File: apps/unwired-mail/unwired-mail/Services/Mailbox/OutboxDeliveryService.swift:0-0
Timestamp: 2026-07-27T07:25:56.352Z
Learning: In the EWS mail provider, keep the failure-disposition/classification logic intentionally different between `OutboxDeliveryService` and `PendingProviderActionService`.
- `OutboxDeliveryService` should treat uncertain sends as “ambiguous” (classify HTTP 5xx, HTTP 408/409/425, `ErrorTimeoutExpired`, and invalid/failed EWS responses as ambiguous) and reconcile before allowing another send to prevent duplicates.
- `PendingProviderActionService` should treat its retry-safe EWS response cases as transient so they trigger provider actions as retries.
During review, do not refactor these into a shared helper solely because the response-code checks overlap—only extract if you can prove the semantics (ambiguous-before-retry vs retry-safe transient) remain unchanged for each service.
Applied to files:
apps/unwired-mail/unwired-mail/Services/Mailbox/OutboxDeliveryService.swiftapps/unwired-mail/unwired-mail/Services/Mailbox/PendingProviderActionService.swift
🔇 Additional comments (2)
apps/unwired-mail/unwired-mail/Services/Mailbox/OutboxDeliveryService.swift (1)
275-280: 🎯 Functional CorrectnessVerify the shared
invalid_grantcontract without merging these classifiers.If
invalid_grantreaches either classifier asEWSOAuthError.tokenExchangeFailed(status: 400)or status 401, both services classify it as permanent instead of requiring reauthorization. Confirm thatEWSMailboxConnectionAdapterperforms the error-code mapping before these services receive the error.
apps/unwired-mail/unwired-mail/Services/Mailbox/OutboxDeliveryService.swift#L275-L280: preserve outbox ambiguity and retry semantics while mapping rejected grants to user action.apps/unwired-mail/unwired-mail/Services/Mailbox/PendingProviderActionService.swift#L199-L204: preserve pending-action retry semantics while mapping rejected grants to user action.apps/unwired-mail/unwired-mailTests/OutboxDeliveryServiceTests.swift (1)
47-55: 🎯 Functional Correctness | ⚡ Quick winCover the complete EWS OAuth retry boundary.
This test covers only status 429. The changed branch also handles 408, 409, 425, and 5xx. Add table-driven cases for those statuses and a non-retryable status such as 400.
[ suggest_recommended_refactor]
Proposed test shape
- func testTransientEWSOAuthFailureIsRetryable() { - guard - case .transient = outboxFailureDisposition( - for: EWSOAuthError.tokenExchangeFailed(status: 429) - ) - else { - return XCTFail("Expected transient EWS OAuth failure") + func testTransientEWSOAuthFailureIsRetryable() { + for status in [408, 409, 425, 429, 500, 503] { + guard case .transient = outboxFailureDisposition( + for: EWSOAuthError.tokenExchangeFailed(status: status) + ) else { + return XCTFail("Expected transient status \(status)") + } } }Source: Path instructions
|
@coderabbitai review |
|
1 participant
Summary
invalid_grantWhy
EWS OAuth connections previously stored only an access token. Once that token expired, the account could no longer authenticate without manually replacing the credential. This change gives supported on-premises EWS connections a durable, device-bound refresh lifecycle while failing closed for legacy token-only records.
Validation
mise exec -- zsh scripts/check-apple-lint.zsh— passed with 0 violationsD6D43A14-3B1A-41B6-B25A-0DFCA1BFB089) — 8 tests passedProductAccountSessionTests.testProductAccountDeletionBlocksMailActionsWhileBackendDeletionRuns; an isolated rerun reproduced the same async XCTest hangPublished at the user's explicit direction despite the full-suite hang above.
Closes #203
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.