fix(desktop): connect Calendar with Google sign-in, and stop swallowing connector errors - #12262
fix(desktop): connect Calendar with Google sign-in, and stop swallowing connector errors#12262aryanorastar wants to merge 9 commits into
Conversation
The connector sheet stores an operation's user-facing message as a plain
String and re-sanitizes it at display time. The sanitizer's last rule was
`text.hasSuffix(".") && text.count < 120` — a guess at "is this curated"
layered on top of the raw-error fingerprints that already ran.
CalendarReaderError.notSignedIn's description is exactly 120 characters,
so it failed that ceiling by one and rendered as the generic fallback:
"Couldn't connect to Calendar. Try again." That is the single most common
Google connect failure and the one that most needs its next step shown.
Four more messages were lost the same way, for lacking a trailing period:
both pythonNotFound cases and Gmail's network and decrypt errors.
Sentence shape does not separate copy this app wrote from raw system
text. The fingerprints do, so they are now the whole decision — and the
`Error Domain=` / `nsurlerror` pair is replaced by `errordomain`, which
also catches bare NSPOSIXErrorDomain spellings the old checks missed.
Verified:
- desktop/macos/scripts/dev-feedback.py --once swift
'UserFacingErrorPresentationTests' → Executed 7 tests, 0 failures.
- Against the unmodified predicate the same suite reports 5 failures, the
first of them literally
`XCTAssertEqual failed: ("Couldn't connect to Calendar. Try again.")` —
the reported string, reproduced from production code.
Failure-Class: none
Desktop Calendar was the only client still authenticating by decrypting Google auth cookies out of a Chromium profile. That mechanism has three failure modes with no recoverable action, all in issue BasedHardware#10459: no Chromium browser installed (Safari or Firefox users), cookies re-encrypted under a scheme the reader will not open, and a declined browser Keychain "Safe Storage" prompt. Each one dead-ends at "Try again", where nothing about the machine has changed and trying again cannot help. The backend already owns a real Google Calendar OAuth grant — the same one the mobile app connects. This routes the desktop through it: - readEvents prefers the account's server-held grant and keeps cookies as the fallback, so the fix reaches all six call sites (connector, Settings, onboarding, chat provider, automation bridge) through the one function they share rather than six call-site patches. - Connect Calendar now opens Google's consent screen and polls for the grant when both sources are exhausted, mirroring the existing connectX flow, instead of dead-ending. - verifyConnection reports connected off a real grant-backed fetch, so status stops claiming "needs sign in" to accounts that hold a grant. - The event payload gains location, description and all_day, which the cookie reader read straight from Google; without them every memory built from an event silently loses detail. max_results ceiling goes 100 → 500 for the connector's one-pass year of history (Google's own cap is 2500). The cookie reader stays. Retiring it means deleting ~800 lines of Swift, its embedded Python, the Settings account picker and their tests — the unreviewable migration AGENTS.md says not to fold into a bug fix. It stays tracked on BasedHardware#10459. Gmail is deliberately not migrated: gmail.readonly is a Google *restricted* scope and integrations_registry.py withholds it from the consent request until verification and CASA are granted, so routing Gmail through the grant would report connected: false forever. It keeps the cookie reader and, from the previous commit, honest errors. Verified: - backend: python3 -m pytest tests/unit/test_google_calendar_event_response.py → 4 passed. - desktop: dev-feedback.py --once swift 'UserFacingErrorPresentationTests|GoogleCalendarGrantEventTests' → Executed 9 tests, 0 failures. - Built and launched as the named bundle omi-calendar-oauth against https://api.omi.me/ (confirmed via omi-ctl health). - All three endpoints the desktop now calls are live in prod today: v1/integrations/google_calendar, .../oauth-url and v1/calendar/google/events each return 401 unauthenticated, while a control path under the same prefix returns 404. NOT verified, stated plainly: the authenticated grant read was not exercised end-to-end. The named bundle came up signed out ("AUTH_LISTENER: No saved session") and signing in is not an action I take on someone's account, so the live grant → events → memories path still needs one manual run by a signed-in maintainer. Failure-Class: none
… flow The new APIClient+GoogleCalendarGrant.swift is the transport that google-connector-read already exercises through CalendarReaderService, so it belongs in the same flow's covers list. Verified: python3 desktop/macos/scripts/check-e2e-flow-coverage.py --strict --base origin/main -> Covered: 4, Uncovered: 0.
Picks up the three fields the desktop connector needs from GoogleCalendarEvent (location, description, all_day) and the max_results ceiling moving 100 -> 500. Verified: backend/.venv/bin/python scripts/export_openapi.py --surface app-client --write ../docs/api-reference/app-client-openapi.json -> 19 insertions, 1 deletion, all in GoogleCalendarEvent and its max_results bound.
Generated artifacts only, required by the openapi-contract gate. Purely additive: location, description and all_day arrive as optional fields on GoogleCalendarEvent in each client. Verified: backend/.venv/bin/python scripts/generate_ts_openapi_types.py -> 4 files, 12 insertions, 0 deletions.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Git-on-my-level
left a comment
There was a problem hiding this comment.
Requesting changes for one concrete, fixable issue — the underlying work is strong, and the diagnosis quality (down to the 120-character sanitizer ceiling and the honest "not verified" section) is exactly what this repo wants in a connector fix.
What I verified as correct
backend/routers/google_calendar.py— the newlocation/description/all_dayfields truncate for transport ([:200]/[:300]), and the all-day derivation ('date' in start_raw and 'dateTime' not in start_raw) matches Google's wire shape. All four tests inbackend/tests/unit/test_google_calendar_event_response.pypass in CI (confirmed in the suite log).desktop/macos/Desktop/Sources/APIClient+GoogleCalendarGrant.swift— both routes you call exist on main:GET /v1/integrations/{app_key}returnsIntegrationResponse {connected, app_key}andGET /v1/integrations/{app_key}/oauth-urlreturnsOAuthUrlResponse {auth_url}, soIntegrationConnectionResponse/IntegrationOAuthURLResponsedecode the real wire shapes. Themin(max(maxResults, 1), 500)clamp matches the new backend ceiling.GoogleCalendarGrantEventTests.swiftdecoding into the sameCalendarEventvalue the cookie reader produced is the right invariant to pin.desktop/macos/Desktop/Sources/CalendarReaderService.swift— grant-first with cookie fallback viatry?is the right availability ordering for the #10459 failure modes, andhasGoogleGrant()failing closed tofalse(never throwing into the read path) is correct.desktop/macos/Desktop/Sources/MainWindow/Components/UserFacingErrorPresentation.swift— replacing the sentence-shape heuristic with raw-error fingerprints (errordomaincatching bothError Domain=and bareNS*ErrorDomain) is strictly stronger, andUserFacingErrorPresentationTests.swiftdriving off the production enums in both directions (curated copy survives, raw system text stays hidden) is exactly how this should be locked down.ConnectorImportOperations.swiftextractingsaveCalendarEventsand mirroring theconnectXconsent-then-poll shape reads cleanly;docs/api-reference/app-client-openapi.jsonmatches the backend model changes, the four regeneratedomiApi.generated.tsclients are consistent, and the e2egoogle-connector-read.yamlcoverage line plus the changelog entry are appreciated.
The blocking issue: Backend unit suite fails on the desktop REST inventory contract
tests/unit/test_desktop_rest_inventory.py::test_every_in_scope_desktop_rest_route_exists_in_app_client_openapi fails with:
missing: ['/v1/integrations/google_calendar', '/v1/integrations/google_calendar/oauth-url']
That test extracts concrete route literals from every APIClient*.swift source. The new file hardcodes v1/integrations/google_calendar and v1/integrations/google_calendar/oauth-url, but the app-client OpenAPI spec carries these as the templated /v1/integrations/{app_key} and /v1/integrations/{app_key}/oauth-url paths, so the normalized forms can't match. To be clear about severity: this is an SSoT bookkeeping gap, not a runtime one — the live routes are real (your 401 probes and my check of the routers agree). The test's message documents the accepted fixes: interpolate the app key in the Swift route literal so it extracts as the templated path, add the routes to the spec, or list them in KNOWN_MISSING_ROUTES per the convention in that file. Please pick one so the suite goes green.
Non-blocking, while you're in here
- In
connectCalendarViaOAuth, the grant read isreadEventsViaGrant(...) ?? []— if the grant check passes in the poll loop but the innerhasGoogleGrant()re-check misses (transient blip), the import reports success with "Read 0 calendar events". Anilthere arguably deserves the "Connected to Google, but reading your calendar failed" branch instead of an empty success. - Raising the route-wide
max_resultsceiling to 500 applies to every client of/v1/calendar/google/events, not just the connector import. It's authenticated and well under Google's 2500 cap, but it's a route-wide API surface change worth a maintainer's explicit nod.
Keeping the cookie reader (retirement tracked on #10459) and not migrating Gmail while gmail.readonly is withheld as a restricted scope were the right scoping calls — I confirmed the withholding comment in integrations_registry.py.
Beyond the CI fix, this reorders desktop Calendar auth to prefer the server-held grant and auto-launches Google's consent screen when both sources fail — a product/auth-direction change that should get maintainer sign-off before merge.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
…key path
test_desktop_rest_inventory extracts route literals from every APIClient*.swift
source and matches them against the app-client OpenAPI spec. Both routes were
written with the app key baked into the literal:
get("v1/integrations/google_calendar")
get("v1/integrations/google_calendar/oauth-url")
The spec carries these as /v1/integrations/{app_key} and
/v1/integrations/{app_key}/oauth-url, and _normalize_for_match only rewrites
{...} placeholders -- so a literal key can never match a templated path and the
routes read as missing from the spec:
E assert not ['/v1/integrations/google_calendar',
'/v1/integrations/google_calendar/oauth-url']
Nothing was actually missing: both templated routes are in the spec and both
backend routes exist (integrations.py:288 and :421). Interpolating the key makes
the Swift say what the contract is -- these are the {app_key} routes with
google_calendar as the key, not routes of their own. The extractor rewrites
Swift interpolation to {param}, which is the same form _normalize_for_match
reduces {app_key} to, so they match.
Chose this over KNOWN_MISSING_ROUTES: nothing is missing, so listing them would
record a gap that does not exist, and the entry would outlive the confusion.
Verified the mechanism rather than just the outcome:
extracted: /v1/integrations/{param}
/v1/integrations/{param}/oauth-url
tests/unit/test_desktop_rest_inventory.py 9 passed (was 1 failed, 8 passed)
Failure-Class: none
|
@Git-on-my-level fixed on I took the first of your three options: interpolate the app key so the literal extracts as the templated path. private let googleCalendarAppKey = "google_calendar"
get("v1/integrations/\(googleCalendarAppKey)")
get("v1/integrations/\(googleCalendarAppKey)/oauth-url")
I picked it over Verified the mechanism, not just the green:
On your two non-blocking notes — I've left both, deliberately. The The route-wide The auth-direction question you raised at the end (preferring the server-held grant and auto-launching consent when both sources fail) is also still open and still a human's. |
|
Follow-up on the two red checks now showing here —
mkdir -p /tmp/mainweb
git archive origin/main web/app | tar -x -C /tmp/mainweb
cd /tmp/mainweb/web/app && bun install --frozen-lockfileThis PR only surfaces it: Fix is up as #12288 — six lines, the two declared deps plus their resolution entries, no transitive drift. I also merged current The route-inventory fix from |
kodjima33
left a comment
There was a problem hiding this comment.
Real bug fix (Calendar OAuth grant path + truncated error message, issue #10459) from a trusted backend contributor, but mixed backend+desktop+web diff is 514 lines (>300 ceiling) and Backend unit suite is currently failing (pre-existing BYOK test breakage on main, unrelated to this diff) — confidence gate not met at 4/5, approve only per policy, not merging.
|
Still in progress, but the implementation itself is complete. The requested route-inventory change remains fixed in That remaining red is inherited from Once #12302 lands and this PR's backend job is rerun/rebased, the only remaining gates are maintainer product-direction sign-off and the stale requested-changes review state. @Git-on-my-level need human response: your concrete blocking request was addressed in |
Resolved on the current head (64088db): the hardcoded v1/integrations/google_calendar literals are now interpolated through googleCalendarAppKey, so the desktop REST inventory extraction normalizes to the templated /v1/integrations/{app_key} paths — test_every_in_scope_desktop_rest_route_exists_in_app_client_openapi passes in this head's Backend unit suite log. This review was the only remaining requested-changes state, so it is being dismissed as stale.
|
Follow-up on the requested changes from my earlier review — re-verified on this head (64088db) before clearing it. Route-inventory blocker: resolved and verified. Interpolating The remaining With the maintainer approval on this head covering the auth-direction change (grant-first with cookie fallback, the consent-screen launch when both sources are exhausted, and the route-wide by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Refreshed on current
|
|
Reviewed across the backend router, the macOS connector path, and the generated clients. This fixes the #10459 failure modes the right way: the desktop stops depending on decrypting Google cookies out of a Chromium profile and reads through the server-held Calendar grant instead, with the cookie reader kept as a fallback so nobody working today regresses. Grant-first ordering in CalendarReaderService.readEvents, the fallback on both absent-grant and failed grant read, and the backend-mediated consent flow in ConnectorImportOperations.connectCalendarViaOAuth all look sound, and the wire shape is tested on both sides. Notes, none blocking:
Thanks — a well-scoped fix with real tests on both sides of the wire. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Summary
Two users reported Gmail and Google Calendar failing to connect on desktop, with this and nothing else:
There are two defects behind that screenshot. One is why the connect fails; the other is why the user can't tell.
1. The connect fails because the desktop authenticates by reading browser cookies
Desktop Calendar was the only client still decrypting Google auth cookies out of a Chromium profile. Issue #10459 documents three failure modes with no recoverable action:
no_browserand an immediate failure.In every one of those, "Try again" is not advice — nothing about the machine has changed between attempts.
The backend already owns a real Google Calendar OAuth grant, the same one the mobile app connects. This routes the desktop through it:
readEventsprefers the account's server-held grant, cookies second. That is one edit in the function all six call sites share — connector, Settings, both onboarding paths, chat provider, automation bridge — rather than six call-site patches.connectXflow, instead of dead-ending.verifyConnectionreports connected off a real grant-backed fetch, so status stops telling grant-holders they need to sign in.location,descriptionandall_day. The cookie reader read these straight from Google; without them every memory built from an event silently loses detail.max_resultsgoes 100 → 500 for the connector's one-pass year of history (Google's own list cap is 2500).What this PR deliberately does not do
The cookie reader stays. Retiring it means deleting ~800 lines of Swift, its embedded Python, the Settings account picker and their tests — the unreviewable migration
AGENTS.mdsays not to fold into a bug fix. It stays tracked on #10459. Ordering is grant-first with cookies as fallback, so nobody the scraper serves today stops working.Gmail is not migrated.
gmail.readonlyis a Google restricted scope, andintegrations_registry.pywithholds it from the consent request until verification and CASA are granted:Routing Gmail through the grant today would report
connected: falseforever. Gmail keeps the cookie reader, and gets honest errors from part 2. Unblocking it is Google's decision, not a code change.2. The user can't tell, because the one message that would have helped is exactly one character too long
The connector sheet stores an operation's user-facing message as a plain
Stringand re-sanitizes it at display time (AppsPage.swift:1817). The sanitizer's last rule was:CalendarReaderError.notSignedIn's description is exactly 120 characters. It fails that ceiling by one and renders as the generic fallback — which for.integration("Calendar")is literally"Couldn't connect to Calendar. Try again.", the reported string. It is also the failure the cookie reader produces most often, and the one that most needs its next step shown.Measuring every connector error string against the old predicate:
The other four are lost for lacking a trailing period — the interpolated ones end in a detail fragment.
Sentence shape does not separate copy this app wrote from raw system text. The raw-error fingerprints that already ran above it do, so they are now the whole decision. The
Error Domain=/nsurlerrorpair is replaced byerrordomain, which also catches the bareNSPOSIXErrorDomainspellings the old checks missed — so this is strictly stronger at hiding raw text while no longer eating curated copy.Verification
The regression test reproduces the reported string from unmodified production code. Reverting only
shouldPreserveCuratedCopyand re-running:Five failures, one per lost message. With the fix, 7/7 pass. The test drives off the production
CalendarReaderError/GmailReaderErrorenums rather than copied literals, so editing a message into a shape the sanitizer drops fails here instead of shipping.dev-feedback.py --once swift 'UserFacingErrorPresentationTests|GoogleCalendarGrantEventTests'python3 -m pytest tests/unit/test_google_calendar_event_response.py./scripts/swift-test-suites.shmake preflightEndpoints are live in prod today. Built and launched as named bundle
omi-calendar-oauthagainsthttps://api.omi.me/(confirmed viaomi-ctl health), then probed the three routes the desktop now calls:401 means the route exists and wants auth; the control under the same prefix 404s. So the desktop's new calls resolve against production as-is. Only the added event fields and the raised
max_resultsneed a backend deploy.Not verified, stated plainly
The authenticated grant read was not exercised end to end. The named bundle came up signed out (
AUTH_LISTENER: No saved session) and signing into an account is not an action I take on someone's behalf, so the live grant → events → memories path still needs one manual run by a signed-in maintainer. Everything up to the auth boundary is covered above.The reporters' own failure was not reproduced locally. This Mac's Arc profile holds live
SAPISID/__Secure-1PSIDcookies in a version the reader decrypts, and Calendar connect works here — the failure needs a machine with no Chromium, or cookies under a scheme the reader skips. The cookie-path diagnosis is taken from #10459's evidence, not from my own measurement. The display defect is deterministic given the string, and is proven above.Product invariants
INV-INT-1(integrations harness over heuristics). This change moves toward the invariant on three counts: it deletes a heuristic rather than adding one; the OAuth setup flow ends in a functional probe rather than a latch; andverifyConnection's new branch performs a real one-event fetch, so "Connected" continues to mean verified now.Failure-Class: none