Skip to content

fix: complete referral and integration authorization recovery - #21827

Merged
aryanorastar merged 24 commits into
mainfrom
fix/connection-completion-recovery
Oct 25, 2026
Merged

aryanorastar merged 24 commits into
mainfrom
fix/connection-completion-recovery

Conversation

@aryanorastar

@aryanorastar aryanorastar commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

What changed and why

Follow-up to merged #21381; refs #21378. Referral activation and integration connections could remain stuck after browser return, adopt late results for a retired account/page, or reuse spent MCP authorization.

  • Share bounded, cancellable, serialized connection reads/waits across native integrations, tasks, marketplace setup and MCP. Publish only server-confirmed state. Save task defaults in choice order after a confirmed grant.
  • Fence registration, browser launch, status, detail and referral continuations by account, request and route ownership. Cache MCP registrations by account/URL and generation; non-OAuth registrations reach their explicit Enable action.
  • Obtain fresh single-use state/PKCE authorization for MCP retries, on the existing registration and OAuth client. Keep Connect busy for its bounded attempt. Rotate only the verifier, without erasing a concurrent callback's credentials.
  • Commit callback credentials/tools/approval transactionally only while owner and verifier still match. A superseded/deleted/changed-owner callback returns 409 without writing or enabling.
  • Preserve typed pending/failed/not-required marketplace verdicts, pCloud's fresh launch grants and signed query bytes. Never send Omi's Firebase bearer to vendor setup probes.
  • Bound referral token lookup, HTTP and decoding with one deadline; ambiguous token failures retain manual retry. Preserve entitlement authority and destination. External settings/home/referral signup use the system browser.

Product invariants affected

INV-UI-1, INV-UI-3, INV-PERF-1: retain shared feedback/navigation and localization primitives; serial polling owns its lifecycle and never treats browser launch as authorization. No recording, conversation-storage or entitlement-policy changes.

Merge scope versus auth rollout

The recovery code preserves Firebase's existing auth-domain selection, helper proxy and Google/Apple callback configuration. The diff does not change firebase.ts, firebaseAuthDomain.ts, firebaseAuthProxy.ts or the generated-server wiring. #21827 can therefore merge independently of #21355 once its own code/CI checks pass. This corrects the broader coupled merge hold in older readiness snapshots; it does not waive a changed provider boundary or claim the original incident is resolved.

The changed MCP boundary is verified with the production routes, actual local HTTP PKCE exchange/tool discovery, isolated real Redis and Firestore emulator transactions. That provider is synthetic; Firebase login is injected. This is relevant local integration evidence for the changed retry/commit protocol, not proof of Google/Apple console settings.

Release sequencing still matters: deploy the new owner-only POST /v1/apps/{app_id}/mcp/authorize on the target backend before releasing clients that use fresh retries. This is a release requirement, not a claim that merging deploys it. No manual deployment or provider-setting change is part of this task.

#21355 separately changes Firebase's helper origin. Its web client and generated Bun proxy must ship together after callback verification. The complete #21378 outcome also requires that fix and released-build/customer follow-up. Keep #21378 open; do not claim its unspecified remaining Try Again screen is fixed.

Exact remaining auth checks for #21355 / complete incident resolution

  1. Confirm the project's Google OAuth web client permits https://app.omi.me/__/auth/handler, and Apple's Service ID permits domain app.omi.me and that exact return URL. Preserve existing Firebase Hosting callbacks. Prior review records authorized-domain coverage through the parent omi.me; do not assume an extra subdomain entry is necessary.
  2. Run the fix(web): repair Google redirect sign-in and auth navigation timing #21355 client and generated Bun proxy together in an appropriate configured integration environment. Verify the same-origin Firebase helper is served (not an external redirect), with SAMEORIGIN framing for the helper while app pages retain their normal protection. The live /login currently returns 200 but /__/auth/iframe returns 404; this means the proposed helper fix is not live, not that its source is proven defective.
  3. Complete Google and Apple redirect sign-in in Safari/browser or simulator, desktop Google popup sign-in, cancellation/retry and referral return. Confirm session publication precedes navigation, repeated task query parameters and the intended destination survive, and referral application is not duplicated. Use authorized QA accounts/data. A physical phone is not an extra merge gate.

Existing signed-in provider consoles do not expose the needed settings to this account; no credential, role, permission or provider configuration was changed. Owner confirmation or provider-connected QA evidence is needed for those remaining checks, not another blanket CTO code approval.

How it was verified

Final verified head: 918e2b6 (test repairs a6c4d7b and 918e2b6; production auth source unchanged from ed6cd0f). All applicable current-commit CI has passed, including Formatting, all six backend shards and their aggregate, backend guardrails, web, Flutter, desktop Swift contracts and all eight CodeQL analyses. The scan results are empty, and the fixture finding is fixed/automatically resolved rather than dismissed. No applicable failing or pending check is being waived.

  • Reproduced the shard-2 collection failure through backend/test.sh: the old visibility test replaced Google/Firestore/packages in sys.modules and omitted transactional. Migrated it to the actual apps module with a fixture-scoped database seam. It checks in-place identity preservation and propagation of a missing-document error without deletion/recreation. Three-file isolated run passes 22 tests; the same 22 also pass together in one process.
  • Full local CI-equivalent shard 6/2 passed: 383 files, 8,519 tests, with the unchanged runner selection, per-file isolation and timing guard. No assertions, timing budgets or baselines were weakened.
  • The earlier Formatting job failed before formatter execution: setup-uv timed out downloading its manifest, leaving no pinned Black. Canonical local checks pass all 6 changed backend Python files with Black 26.5.1; Flutter 3.44.5/Dart 3.12.2 formatting passes all 33 surviving non-generated changed Dart files with zero edits. Final CI passes the normal setup/check path; no formatting gate is skipped.
  • make preflight passed all 63 selected checks, including web typecheck, all 887 Vitest tests and 6 Bun smoke tests, with four workers and unchanged assertions/timeouts. New follow-up after that run is documentation only; normal pre-push reruns the selected gates.
  • Prior unchanged-source auth evidence remains valid: 13 reauthorization/commit tests plus 6 state/security tests; 808 checks including all seven Firestore query/index guard files; real HTTP/Redis/Firestore emulator recovery integration; focused Python 3.11 typecheck (0 errors, existing warnings), async-blocker gate, generated-client/OpenAPI checks, targeted Flutter security/setup/recovery tests and 23 seeded mobile journeys. Simulator and synthetic-provider evidence is not physical-device or live-provider proof.
  • Final review found CodeQL alert 504 in the synthetic loopback OAuth fixture. Its callback was already checked for exact equality, but the response header still copied the request value. The fixture now emits only its fixed callback URL and explicitly rejects other URLs. All 4 integration tests pass through backend/test.sh with the local HTTP provider, Redis and Firestore emulator, including unregistered/CRLF redirects, encoded state and the unchanged production-route recovery scenario. No production HTTP handler or provider configuration changed.
  • A managed CodeQL JavaScript scan on c846bbb failed connecting to api.github.com (10-second connect timeout); GitHub rejected both job and workflow rerun. A normal empty refresh commit 223d4c6 reran standard checks with the same source tree, and all security scans then completed successfully. The fixture repair's own fresh scan now also passes on 918e2b6, with zero results across all eight languages/categories. No alert was dismissed or scan setting weakened.
  • The migrated test is registered in the single-process subset; two entries naming already-removed files were retired. A broader optional mixed-process isolation audit remains inconclusive/red on another file's duplicate Prometheus registration and a load-sensitive VAD timing guard. It is not represented as passed. The focused 22-test shared-process run and full isolated shard pass; the ordinary CI guardrail lane is unchanged.
  • Five earlier automated review findings have verified fixes and were resolved. No formal self-approval, force push or hook bypass.

Tests

Relevant coverage: connection_read_test.dart, connection_wait_test.dart, integration_oauth_test.dart, android_hanging_flows_test.dart, app_setup_status_test.dart, app_detail_session_test.dart, app_install_flow_test.dart, mcp_connection_recovery_test.dart, test_mcp_oauth_reauthorize.py, test_mcp_oauth_callback_state.py, test_mcp_oauth_recovery_emulator.py, test_app_visibility_missing_doc_guard.py, referrals.test.ts and LoginClient referral/token tests.

UI evidence

Existing real LoginClient rendering with synthetic auth and local failed/successful claims; not production Google sign-in or customer verification. Unchanged client behavior/layout reuses this evidence and the recorded simulator/local journeys.

Referral activation failure and retry
Successful retry continuation

Failure class (fixes)

Failure-Class: FC-continuation-resumed-after-unmount

Account/request/mounted fences surround awaited client dependencies; shared ConnectionRead/ConnectionWait own deadlines and cancellation. Controlled late-response regressions exercise the production owners. The additional durable MCP callback guard compares owner/verifier transactionally before committing.

Line-Count-Exception: backend/routers/apps.py | 2696 -> 2750 | The MCP re-authorize route shares the registration, state-store and PKCE helpers of the existing add, callback and refresh MCP routes in this router; splitting only it out would separate one OAuth flow across files.

Failure-Class: FC-continuation-resumed-after-unmount
Failure-Class: none

This fences local task-selection intent; the registered transport-session callback and transactional membership classes have different authorities and lifecycles.
@Git-on-my-level Git-on-my-level added needs-maintainer-review Needs a human maintainer to sign off before merge security-review Touches auth, provider routing, secrets, or security-sensitive surfaces flutter flutter work labels Oct 16, 2026
@Git-on-my-level

Copy link
Copy Markdown
Member

Thanks @aryanorastar — this is a thorough follow-up to #21381. I reviewed the diff across the mobile and web client paths; the direction looks right and several changes are clear security improvements. Specifics:

What looks good

  • app/lib/backend/http/api/apps.dart: getAppSetupStatus now requires a typed successful boolean verdict (is_setup_completed must be a real bool; {}, [], "true", non-JSON, and 503 all yield ApiFailure), and the vendor probe uses a plain transport that does not forward Omi's Firebase bearer token and cannot refresh/expire the Omi session on a vendor 401. Backed by app/test/unit/backend/app_setup_status_test.dart.
  • app/lib/services/integrations/connection_read.dart and connection_wait.dart: probes are serialized (never overlap), deadline-bounded, cancellable, and late verdicts after cancellation are ignored — covered in connection_wait_test.dart and connection_read_test.dart.
  • app/lib/services/integrations/integration_oauth.dart: isCurrent fences before URL fetch, before browser launch, and after the launch receipt, so a late URL or late browser receipt cannot act for a departed page. Retiring the spoofed user agents (app/lib/utils/browser.dart deleted) and embedded Google-popup handling in favor of the system browser (omi_external_browser_page.dart, referral_page.dart, app_home_web_page.dart) is the right call.
  • web/app/src/lib/referrals.ts: one 15s deadline spans token lookup, POST and decode, with AbortController cancelling the actual request; LoginClient.tsx scopes the claim to uid/code/environment and blocks late dispatch after an account or route change.

Notes for maintainers

  • This is stacked on the still-open fix(web): repair Google redirect sign-in and auth navigation timing #21355 for the web Firebase auth helper, and the PR documents that both must ship together with a coordinated deploy. That release coupling plus the auth/session surface is a maintainer sign-off decision, not something I'll formally approve.
  • LoginClient.tsx assigns claimOwner.current during render. It works, but moving that assignment into the existing effect would be more strictly correct under React's render-purity expectations — non-blocking.
  • The changelog entry and both ARCHITECTURE.md notes match the implemented behavior as far as static review can verify; the Samsung S26 Ultra real-sign-in and referral-result checks in web/app/docs/referral-recovery.md remain release acceptance items.

Leaving for human maintainer review: the OAuth/session-recovery flow and the #21355 release dependency need maintainer sign-off before merge.

This feedback is AI-generated on behalf of the maintainers.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

Selecting Asana while an earlier Todoist authorization was still
launching saved Asana, then the late Todoist launch receipt started
polling and saved Todoist too (defaults ['asana', 'todoist']). Each
selection now claims an attempt before its first await and checks it
after every await (dialog, launch, polling, save), so a newer selection,
a departed page or an account change retires older work. A launch that
throws reports failure instead of leaving authorization stuck starting.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aryanorastar

Copy link
Copy Markdown
Collaborator Author

Pushed 55f85de: the latest task selection now wins even while an older authorization is still launching.

  • Bug: with Todoist disconnected and Asana connected, starting Todoist and then selecting Asana saved Asana. The held Todoist launch then resolved, started polling, and saved Todoist too: defaults ['asana', 'todoist'] instead of ['asana']. The earlier retirement only covered attempts that had already reached polling.
  • Fix (task_integrations_page.dart): each selection claims an attempt token before its first await and checks it after every await (confirmation dialog, URL lookup/browser launch, polling, default save). A newer selection, a departed page or an account change retires the older attempt. A launch that throws reports failure instead of leaving authorization stuck as starting, and the post-save loading flag clears in a finally. Server-confirmed status and the bearer-free vendor probe are unchanged.
  • Tests (android_hanging_flows_test.dart):
    • Supersession while the launch is still pending. This is the reported repro, and the old code fails it with ['asana', 'todoist'].
    • A late receipt after the page closes.
    • A late receipt after an account change (old code: ['todoist'] for the new account).
    • A thrown launch that then retries cleanly.
  • Checks: 16 Flutter suites, 76 passed; analyze clean. Web via Bun: typecheck clean, 17 passed. make preflight: 28 checks passed.
  • Release docs: fix(web): repair Google redirect sign-in and auth navigation timing #21355 is still open (mergeable, CI green, unapproved). The integrations ARCHITECTURE note and mobile-browser-auth.md now require one coordinated web-helper + client deploy and real Google/Apple sign-ins, including on Samsung S26 Ultra. Mocked tests do not verify production sign-in.

@Git-on-my-level

Copy link
Copy Markdown
Member

Reviewed this follow-up to #21381 against the current head. The recovery model is coherent and well tested; observations on the significant files:

  • app/lib/services/integrations/connection_wait.dart + connection_read.dart: the probe loop serializes checks (_checking plus _refreshQueued) so a foreground refresh never overlaps a slow read, and ConnectionRead gives callers a real deadline even when the underlying SDK ignores cancellation. Covered by connection_wait_test.dart and connection_read_test.dart including hung-probe and late-result cases.
  • app/lib/backend/http/api/apps.dart: getAppSetupStatus routes vendor-origin probes through a plain HttpPoolManager transport (15s timeout, no retries, no auth headers) instead of the authenticated API path, so a vendor 401 cannot refresh/expire the Omi session or receive the Firebase bearer token; uid is pinned at call time and re-checked via canSend. app_setup_status_test.dart covers verdict parsing and malformed bodies.
  • app/lib/pages/apps/app_detail/app_setup_owner.dart: setup reads are fenced by generation, uid, app id and setup URL; auto-install fires only on a verified connected verdict after an explicit user action. One nuance worth a manual UX pass: an ApiSuccess notRequired resolves the wait as failed (per ARCHITECTURE.md an absent endpoint never claims a connection), so legacy apps without a completion endpoint will surface the failure state rather than silence.
  • app/lib/pages/apps/add_mcp_server_page.dart: a retry with the same URL reuses the existing server registration instead of re-creating it, and connected now requires status approved AND enabled, with account-change resets of the pending registration.
  • app/lib/pages/settings/task_integrations_page.dart: _ownSelection gives each selection ownership of dialog, launch, polling and default save, so a late receipt from a retired attempt cannot overwrite a newer choice; covered in android_hanging_flows_test.dart.
  • web/app/src/lib/referrals.ts + LoginClient.tsx: one 15-second deadline spans token lookup, POST and decode via AbortController; dispatch is gated by the uid/code/environment owner plus an attempt counter, and retry is manual with no automatic request loop. Retryable classification is limited to 429/5xx/invalid response shape.
  • Retiring the user-agent spoof list (app/lib/utils/browser.dart removed, OmiExternalBrowserPage using the system browser) aligns with Google's guidance against OAuth in embedded webviews.

No concrete defect found. Because this changes OAuth completion handling, vendor-probe credential separation and the login referral claim dispatch, the auth-flow behavior itself needs maintainer sign-off before merge — that is what the security-review and needs-maintainer-review labels track here, not a specific bug.

— automated maintainer feedback on behalf of the Omi maintainers


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@Git-on-my-level Git-on-my-level added the positive-signal Automation verified a genuine fix/quality contribution label Oct 17, 2026
…saves

An MCP registration's response stored the URL in the field at response
time, so editing the URL from A to B while A registered filed A's
registration under B and a retry reopened A's authorization. The page
now owns the submitted URL before awaiting, never launches a response
for a URL the user has since edited, and stops waiting on a server when
its URL changes.

Overlapping default saves (Todoist, then Asana while Todoist's save is
pending) could finish out of order and leave Todoist selected, because
the provider fenced only the account. Saves now run one at a time in
choice order, a queued choice replaced before it is sent is skipped,
and only the latest choice publishes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aryanorastar

Copy link
Copy Markdown
Collaborator Author

Pushed d9fa6f5: two remaining races fixed, an MCP registration reused for the wrong server and overlapping task-default saves.

  • MCP retry reused the wrong server (add_mcp_server_page.dart): the response stored whatever URL was in the field when it arrived. Submitting A and editing to B while A registered filed A's registration under B, so a retry reopened A's authorization.
    • The page now captures the submitted URL before awaiting and keys the registration and auth URL to it. A response for a URL the user has since edited never launches.
    • Editing the URL while waiting on a server stops that wait, so Connect registers the new server.
  • Overlapping default saves (task_integration_provider.dart): with Todoist and Asana both connected, choosing Todoist then Asana while Todoist's save was pending could finish out of order and leave Todoist selected. The provider fenced only the account, and the page's check came after the save.
    • Default writes now run one at a time in choice order, a queued choice replaced before it is sent is skipped, and only the latest choice publishes.
    • A timed-out write that the server applies late is not covered; the server would need to reject stale writes for that.
  • Tests:
    • MCP: edit during pending registration; edit while waiting on authorization.
    • Provider: choice order and latest-only publication; a replaced queued choice is not sent.
    • Page: the Todoist → Asana scenario.
    • All four reproduce on the previous head, including selectedApp == todoist on the page, and pass now.
  • Checks: 15 integration/settings suites, 78 passed; flutter analyze clean; make preflight: 28 checks passed.
  • Docs: the integrations ARCHITECTURE note and the mobile-browser-auth.md checklist are updated, including keeping Android: referral Google signup blank and integration connection status hangs #21378 open until release and customer confirmation.

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: the recovery logic here is correct and well-scoped — no blocking issues found, and several changes are clear security improvements. Merge sign-off left to a human maintainer given the auth-sensitive surface.

What I verified on this head:

  • app/lib/backend/http/api/apps.dart — getAppSetupStatus routes vendor-origin probes through HttpPoolManager with empty headers (15s, no retries), so a vendor endpoint can no longer receive the Firebase bearer token or trigger Omi session refresh on a 401; same-origin requests keep the authenticated transport. app/test/unit/backend/app_setup_status_test.dart pins exactly this (headers empty, uid query only).
  • app/lib/services/integrations/integration_oauth.dart — isCurrent fencing before URL fetch, after fetch, and after browser launch, so a late OAuth URL or launch receipt for a departed page/account cannot start completion; URL validation (scheme/host/no userInfo) retained and OAuth URLs stay out of logs/errors.
  • app/lib/services/integrations/connection_read.dart + connection_wait.dart — clean cancellable primitives: probes never overlap (refresh queues behind a slow read), hung probes end the wait, late results cannot publish twice. Ownership is consistent across all four call sites (app detail, MCP, integrations, task integrations), with dispose/clearUserData cancellation in both providers.
  • app/lib/pages/apps/add_mcp_server_page.dart — registration is keyed to its submitted URL (_submittedUrl + _serverChanged): editing the URL stops the old wait and never reuses the earlier server's auth URL; a connected verdict requires status approved AND enabled, so approval alone is no longer reported as connected.
  • app/lib/providers/task_integration_provider.dart — default writes serialize in choice order, a choice superseded before it is sent is skipped, and only the latest publishes; covered by the new ordering tests.
  • web/app/src/lib/referrals.ts + LoginClient.tsx — one AbortController deadline bounds token lookup, POST and decode; response shape validated (claimed boolean, trial_days non-negative integer); claim ownership fenced by uid/code/environment/mount; retry offered only for retryable failures, terminal rejections keep continuation.
  • Docs (app/docs/mobile-browser-auth.md, services/integrations/ARCHITECTURE.md, web/app/docs/referral-recovery.md) match the code as far as I could cross-check, including honestly tracking the still-open verification items.

Security: net positive — bearer-token exposure to vendor endpoints removed, user-agent spoofing retired (app/lib/utils/browser.dart deleted).
UX: integration settings/app-home and referral signup now open the system browser instead of an embedded view, and failed checks/referral claims get a real Try again.
Performance: polling stays bounded (3s interval, 20s probe timeout, 5m overall) and serialized — no added query pressure versus the previous timer loops.
Blast radius: all integration/task/MCP/referral connection flows; the server-confirmed-state-only model bounds the failure mode to "not connected" rather than mis-connected.

Still wanted before ship: the release-acceptance items the PR itself tracks (real Google return on Samsung S26 Ultra, iOS regression validation, identifying the customer's remaining Try Again screen), and a note that Frontend Lint was skipped on this head — the locally reported Vitest/typecheck runs are the only web coverage visible here, so a maintainer may want the web suite re-run or the path filter checked.

USER IMPACT: After signing into an integration or referral in your browser, the app keeps checking in the background, shows a retry when a check fails, and a delayed reply can no longer change a page or account you have already left.

Human maintainer sign-off is warranted for the OAuth/connection-recovery semantics across mobile and web 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.

aryanorastar and others added 3 commits October 19, 2026 17:51
Review of #21827 reproduced a remaining race: submit URL A, hold the browser
launch, edit the URL to B, complete the launch, and the page started polling
A while the form showed B. _serverChanged only cancelled an existing wait,
and during the launch none existed yet; the launch completion and polling
checked mounted/account but not ownership of the submitted URL.

Each Connect press now mints an immutable attempt (submitted URL, account,
generation). Editing the URL retires the whole attempt: pending registration,
browser launch, direct detail read and wait. Every await rechecks _owns before
polling, navigating, showing an error or changing the loading state, for both
the first registration and the cached-registration retry; returning to the
old URL does not revive a retired attempt. The registration stays keyed to its
URL and account, so an explicit new Connect on that URL still reuses it.

Four held-Future regressions cover the pending browser launch, the cached
retry, the direct detail read and A -> B -> A; all four fail on the reviewed
head (the launch case with reads ['server-a']).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…change

Follow-up to the attempt-ownership change: the wait completion returned early
when the attempt no longer owned the page, so a wait ended by an account
change left the page in its polling state. The completion now releases
polling whenever it is still the page's wait, and only reports a verdict
while its attempt owns the page. Explicit mounted checks beside _owns()
satisfy use_build_context_synchronously.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aryanorastar

Copy link
Copy Markdown
Collaborator Author

Pushed 0f5dc4c (from d9fa6f5): MCP connect attempts now own the page.

  • Each Connect press mints one attempt holding its submitted URL, account and generation. Editing the URL retires the whole attempt (registration await, browser launch, cached retry, detail read, wait), and every await rechecks ownership before polling, navigating, showing an error or touching loading state.
  • A→B→A does not bring the old attempt back. The registration stays keyed to its URL, so an explicit Connect on that URL reuses it without a second create.
  • Polling is released for the page's own wait even after an account change; a verdict shows only if the attempt still owns the page.
  • Merged current origin/main (no conflicts in these files).

Tests: 4 held-Future cases added to mcp_connection_recovery_test.dart (pending browser launch, cached retry, pending direct detail read, A→B→A). All 4 fail on the previous page. MCP file 11/11; combined integration suites 53/53; analyzer clean; git diff --check clean; make preflight passed.

Still human-confirmed, not covered by these mocks: #21355 web helper, Google/Apple callback config, real sign-in.

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 19, 2026

Copy link
Copy Markdown

Auto-fix isn't included in your plan ↗

Upgrade to Team

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since my last review: addressed (nothing was blocking then; the three new commits further harden MCP — URL-edit now retires the whole connect attempt including a pending browser launch, and polling is released for its own wait even after an account change) · still open (nothing from my prior review) · newly introduced (a red Dart Analyze & Tests / Mobile Release Eligibility pair — but not by this PR, see below).

Verdict: the production changes on this head are correct and well-scoped; the only failing checks are inherited from main and already fixed there. No PR change needed beyond refreshing main.

What I verified on the new commits (a22b2e3, 5a002b7 merge, 0f5dc4c):

  • app/lib/pages/apps/add_mcp_server_page.dart — each Connect press now mints an attempt owned by generation+uid+submitted-URL (_beginAttempt/_owns/_retireAttempt); editing the URL retires the attempt's registration, launch, read and wait; every await rechecks ownership before touching context or loading state, and the new mounted && guards before _showError(context, ...) close a real use_build_context_synchronously hazard (error-severity in this repo).
  • app/test/widgets/mcp_connection_recovery_test.dart + app/test/support/test_browser.dart — the pending-launch Completer in TestBrowser lets tests hold a browser open mid-launch and assert retired attempts never poll or navigate; coverage matches the new ownership semantics.
  • Security posture unchanged and good: app/lib/backend/http/api/apps.dart still isolates vendor-origin probes (HttpPoolManager, empty headers, 15s, no retries — no Firebase bearer token, no Omi session refresh on a vendor 401), and integration_oauth.dart's new isCurrent fences stop a late URL or launch receipt from acting for a departed owner.

Why CI is red (not this PR's fault): both failing checks trace to one unused_import in app/test/widgets/home_ask_anything_button_test.dart:7 — a file this PR does not touch. It arrived on main with the #19854 merge (3260067), sat inside this PR's merge ref, and was fixed on main by 8b65fde shortly after. I reproduced the exact CI analyzer (Flutter 3.44.5) against both trees: this PR's head alone reports zero unused_import; the merge ref reports exactly that one. Merging or rebasing onto current main should turn both checks green as-is.

Performance/cost: no new per-call load beyond the existing bounded probe loop (3s interval, 20s probe timeout, 5m wait ceiling); no external spend.

USER IMPACT: integrations and referral sign-in keep checking after you return from the browser, offer a real retry when a check fails, and a stale screen or account switch can no longer leave a spinner hanging or connect the wrong thing.

Still wanted before merge: a green analyzer run after the main refresh, and the release acceptance items already listed in app/docs/mobile-browser-auth.md (Samsung S26 Ultra referral round-trip; identifying the customer's repeated Try Again screen) before the store release.

Leaving merge sign-off to a human maintainer: this PR changes OAuth/connection handling and account fencing, which deserves maintainer judgement rather than automated approval.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@aryanorastar

aryanorastar commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator Author

Pushed 1bb10cc: merged current main (8edd56d). No code changes.

  • Why: Dart Analyze & Tests and Mobile Release Eligibility failed on the analyzer ratchet (unused_import: 1 found, baseline 0). With current main merged, app/scripts/analyze_ratchet.sh passes locally. I could not pin which file produced that one import: the previous head analyzes clean here, so the difference is probably in CI's generated code. Its source is unconfirmed, not shown to come from main.
  • Scope: the merge is clean, with no conflicts; the diff against main is exactly this PR's files.
  • Checks: this PR's tests, 53 passed.

@Git-on-my-level

Copy link
Copy Markdown
Member

Since my last review: addressed (the previously red Dart Analyze & Tests / Mobile Release Eligibility pair is green on this head) · still open (nothing from my prior review) · newly introduced (nothing — the only change is a merge of main, verified below).

Verdict: no new issues on this head. The only change since my last review is a merge of current main (1bb10cc); I blob-compared all 45 files this PR touches and exactly one changed.

What I verified on the merge:

  • app/lib/backend/http/api/apps.dart — the merge adopts main's fix(app): never log app API key secrets or call tokens #21088 secret-logging hardening: listApiKeysServer, createApiKeyServer and addMcpServer now log only the status code, so API key secrets and OAuth auth_url state tokens no longer reach logs. The PR's own getAppSetupStatus section is byte-identical to the code verified on the previous head (vendor probes still isolated via HttpPoolManager, empty headers, 15s, no retries — no Firebase bearer token or session refresh on vendor endpoints).
  • The other 44 PR files are unchanged since 0f5dc4c — checked by direct blob comparison, not assumed from the merge commit message.

Security: the merge direction is strictly positive for the paths this PR touches — better log hygiene on the very API surface (MCP registration, app keys) this PR reworks.

USER IMPACT: none beyond what this PR already changes — this update only syncs the branch with main; the merge itself introduces no user-visible behavior change.

Still open before merge (unchanged): the on-device acceptance items in app/docs/mobile-browser-auth.md (Samsung S26 Ultra referral Google signup, the reported integrations connecting, identifying the separate repeated Try Again screen) and coordinated web deployment with #21355.

Merge sign-off stays with a human maintainer: auth-sensitive OAuth/referral recovery across mobile and web needs maintainer approval.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

Automatic integration preserves the referral/OAuth/MCP recovery boundary; all 53 focused provider, browser and widget regressions pass. Keep draft: production auth helper is still 404 and live Google/Apple/Samsung acceptance remains unverified. No deployment.
The current-main task-delivery fixtures must accept the providers silent named argument added by this PR. Both overrides now preserve the production API shape. Task-delivery/default-selection focused verification: 12 tests passed. No production behavior or release state changes; keep draft pending final-head analyzer/CI and real provider acceptance.
@aryanorastar

Copy link
Copy Markdown
Collaborator Author

Non-device verification follow-up — 2026-10-23

All checks on current head 2cc2f0e30e2caf1861fc57bfdc8f10fe08066c6a are successful or intentionally skipped. No new source changes were made in this pass.

Fresh read-only live checks: https://app.omi.me/__/auth/iframe still returns HTTP 404, while /login returns 200. Dependency #21355 remains open and unmerged. This is not a verified Google/Apple callback round trip, and deploying unrelated services is not an authorized workaround.

Still draft: coordinate the helper integration, then verify real Google/Apple sign-in, referral return/cancel and integration completion in a browser/native client. The reported Samsung/iOS regression checks remain a later device gate.

Pairwise integration with #22826 also conflicts in the shared app-setup API, app-detail owner and install-flow tests. Preserve fresh grants, vendor-bearer isolation and retired-account fencing when the first PR lands; rerun both regression sets on the final merged tree. No branch was changed by the merge-tree diagnostic. No merge or deployment performed.

@aryanorastar
aryanorastar marked this pull request as draft October 23, 2026 16:32
…eferral retry

- An approved non-OAuth MCP registration opens its detail page, where Enable
  lives, and is announced as connected only when the server reports it enabled.
  OAuth completion still requires approved and enabled.
- Registration identity lives on the immutable attempt; a cache keyed by
  account and URL serves explicit retries, so a late registration for a retired
  attempt cannot replace or cancel the current one. The Connect button no
  longer stays locked on a retired attempt's request.
- A null ID token for a signed-in referral claim is retryable: the user keeps
  Try again, still fenced to the account and deadline.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aryanorastar

Copy link
Copy Markdown
Collaborator Author

Pushed 283c148cdd with three fixes:

  • Non-OAuth MCP setup can finish. The backend registers a server without OAuth as approved but not enabled. _navigateToAppDetail rejected that, so the user never reached Enable. The approved registration now opens its detail page, where Enable is, and the app says "connected" only once the server reports the app enabled. OAuth completion still waits for approved and enabled.
  • A retired registration can't overwrite the current one. The registration response used to write the shared _appId/_authUrl before checking ownership. A late registration A could therefore replace B or cancel B's wait. Registration identity now stays with its own attempt, and a separate cache keyed by account and URL serves explicit retries. The Connect button also no longer stays locked on a retired attempt's request; previously it blocked connecting B until A returned.
  • A temporary token failure keeps the referral retry. getIdToken returns null both when signed out and when Firebase can't read the token. A claim only starts for a signed-in account, so a null token is now a retryable failure. Try again stays, the retry is still limited to the same account and deadline, and nothing retries automatically.

Evidence:

  • Six new tests fail on the previous head: four MCP widget tests and two LoginClient tests that use the real claim function.
  • Flutter: MCP recovery 15, install flow 12 and integration service tests 13, all passing; analyzer ratchet passed.
  • Web: login and lib tests 437 passed; typecheck clean.
  • scripts/pr-preflight: 28 checks passed. An earlier run hit 15-second test timeouts in an unrelated web suite while this Mac was under heavy load; that suite passes on its own.

Combined check with #22826, done in a separate local checkout and not pushed: the two PRs conflict in apps.dart, app_detail.dart and app_install_flow_test.dart. The resolution keeps both PRs' protections:

On the combined tree, 57 Flutter tests, 30 backend tests and the 73-test pCloud plugin suite pass. The resolution is recorded so whichever PR merges second can reuse it.

@aryanorastar

Copy link
Copy Markdown
Collaborator Author

I ran the non-OAuth MCP flow on the iPhone 16 Pro simulator with 283c148cdd, using the local backend and a synthetic MCP server on loopback that offers two tools and no OAuth.

  • In Settings → Integrations → Add MCP Server, tapping Connect registered the server. The backend stored the app approved and not enabled.
  • The app opened the server's detail page with Enable and both tools listed. On the previous head, this case showed "Failed to connect".
  • Tapping Enable sent POST /v1/apps/enable (200). The page switched to Disable, and the server now reports enabled: true.

Not covered by this run: a real third-party MCP server, OAuth providers, and the registration race on a device. The race is covered by the new widget tests.

@aryanorastar
aryanorastar marked this pull request as ready for review October 24, 2026 03:29

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 283c148cdd

ℹ️ 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".

Comment thread app/lib/pages/apps/add_mcp_server_page.dart
Comment thread app/lib/services/integrations/connection_wait.dart
@aryanorastar
aryanorastar marked this pull request as draft October 24, 2026 10:22
aryanorastar and others added 3 commits October 24, 2026 16:00
…probe budgets

- A late reply for an older same-account/URL registration can no longer
  replace a newer cached registration (per-key request ordering).
- POST /v1/apps/{app_id}/mcp/authorize issues a new single-use state and PKCE
  pair for an existing registration; an explicit retry uses it instead of
  reopening the consumed or expired original URL, and launches nothing if
  fresh authorization is unavailable.
- Task and integration connection waits give each probe the provider's own
  45s status budget plus margin, so a slow but valid read no longer ends
  authorization early.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aryanorastar

Copy link
Copy Markdown
Collaborator Author

Pushed b98b032e49 with the three review fixes, after merging current main:

  • Same-URL registration race: cache writes are now ordered per account and URL, so a late reply for an older registration can never replace a newer cached one. The regression is your reproduction; it fails with the guard removed (it re-authorizes old-registration).
  • Expired or consumed OAuth on retry: new owner-only POST /v1/apps/{app_id}/mcp/authorize issues a fresh single-use state and PKCE pair for the same registration and OAuth client. It stores only the new code verifier, so an older URL fails its token exchange without changing the app. An explicit retry uses it instead of the original URL, and launches nothing if fresh authorization is unavailable. I added the route-policy entry and regenerated the OpenAPI spec and the TS, Dart and Swift clients.
  • Mismatched deadlines: the task and integration settings waits now give each probe its provider's own 45 s status budget plus 5 s. A 30 s status read now completes Todoist authorization; that test fails on the previous page.

Tests:

  • Flutter: 62 passed (MCP recovery, install flow, integration services, settings flows, provider failures). Analyzer ratchet passed.
  • Backend: MCP OAuth 10 (4 new) and route-policy inventory 26 passed.
  • Generated-client checks are clean, and the web typecheck passes.
  • Preflight: 60 checks passed.

Not exercised: a real OAuth MCP provider. Staying in draft until CI is green; merge is still held behind #21355.

@mintlify

mintlify Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
omi 🟢 Ready View Preview Oct 25, 2026, 6:54 AM

💡 Tip: Enable Automations to automatically generate PRs for you.

@Git-on-my-level

Copy link
Copy Markdown
Member

Since my last review: addressed (nothing was open) · still open (nothing from prior reviews) · newly introduced (nothing blocking — the two new commits extend the fix and check out, details below).

Verdict: the changes on this head are correct and well-scoped; the new MCP re-authorization path is a genuine security improvement. No blocking issues.

What I verified on the delta (283c148, 70b84d7, plus generated client regen):

  • backend/routers/apps.py — the new authorize_mcp_server endpoint has the right ladder: 404 unknown app, 403 when app_data['uid'] != uid (before any state is created or tokens touched), 422 for registrations without an OAuth client, 502 when provider metadata is unavailable. Each call mints a fresh single-use state + PKCE pair and mutates only code_verifier, so a stale authorization URL can no longer strand a retry, and delete_app_cache_by_id keeps the cache honest.
  • backend/tests/unit/test_mcp_oauth_reauthorize.py — the owner-only test asserts the failure path has zero side effects (no updates, empty Redis store), and the 502 test verifies the original verifier is left untouched. Solid hermetic coverage.
  • app/lib/backend/http/api/apps.dart — authorizeMcpServer logs only the status code, never the auth URL/state token, and returns null on any failure.
  • app/lib/pages/apps/add_mcp_server_page.dart — an explicit retry reuses its registration but requests a fresh authorization URL, with an _owns(attempt) recheck after the await; the new _registrationRequests/_cachedRequest fencing means a late reply from an older request can never replace a newer cached registration.
  • web/app/src/lib/referrals.ts — token-null is now a retryable failure (getIdToken returning null is ambiguous between signed-out and a transient Firebase failure). Safe: LoginClient still starts claims only for a signed-in uid, and no fetch happens without a token; the new LoginClientReferralToken tests cover the stalled-token and account-change cases.
  • backend/route_policy_manifest.yaml — the new route declares firebase_id_token auth in the credentials domain. Note it carries rate_limit: none, consistent with the sibling MCP routes, but each call writes a Redis state and performs an outbound metadata discovery, so a per-uid limit would be cheap hardening if a maintainer wants it.

Security: the delta tightens rather than loosens the OAuth surface (single-use state enforced per retry, owner checks, no token leakage into logs). Blast radius: an error in the new endpoint would affect only MCP-server retry flows, and its failure modes fail closed.

USER IMPACT: Retrying a stuck MCP server sign-in now gets a fresh working authorization link instead of a dead one, slow integration status checks no longer break a connection, and a failed referral activation shows a Try again that actually retries.

Still wanted before merge: the on-device release checks already tracked in app/docs/mobile-browser-auth.md (Samsung S26 Ultra referral round-trip, reported integration connections), and a maintainer's call on rate limiting the new authorize endpoint.

Leaving for human maintainer review: this head adds a new server-side OAuth endpoint in the credentials domain — it reads correct to me, but merge sign-off on a new auth surface belongs to a maintainer.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

…e verifier

- An explicit retry holds Connect while its fresh authorization is requested
  (bounded like registration), so a second press cannot issue a competing
  authorization that replaces the verifier of the URL being opened.
- Re-authorization writes only external_integration.mcp_oauth_tokens.
  code_verifier with a field-path update instead of writing back a token map
  read before provider discovery, so tokens saved by a concurrent callback
  are never erased.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aryanorastar

Copy link
Copy Markdown
Collaborator Author

Pushed fbc7212696 with fixes for the two remaining review findings:

  • Overlapping authorization retries: an explicit retry now holds Connect while it requests fresh authorization. The request has the same 45 s bound as registration, and the button is released on success, failure or timeout. A second press can no longer issue a competing authorization that replaces the verifier of the URL being opened. With a held authorization, repeated presses now make one request; the previous page made two.
  • Tokens saved by a concurrent callback: re-authorization now writes only external_integration.mcp_oauth_tokens.code_verifier, as a Firestore field-path update (new set_mcp_code_verifier). It no longer writes back a token map read before provider discovery, so a callback that commits tokens during that wait keeps its access and refresh tokens. That test fails if the old map write is restored.

Tests:

  • Flutter: 63 passed, including the reviewer's busy reproduction, plus a check that a failed authorization releases Connect. Analyzer ratchet passed.
  • Backend: MCP OAuth 11 passed, including the reviewer's callback-race reproduction. Route-policy inventory 26 passed, and pyright is clean on the changed files.
  • Preflight: 61 checks passed.

The field-path write was tested against a fake that follows Firestore field-path semantics, not against the Firestore emulator. Still in draft: CI on this head and #21355 remain.

aryanorastar added a commit that referenced this pull request Oct 25, 2026
…22826)

## Summary

Refs #20213. Builds on #20498 while preserving its contributor commits;
do not merge the unsafe foundation first.

This PR completes the account/consent/durable-delivery boundary rather
than adding another UID-only observer or uncorrelated audio buffer.
Maintainer authorization now covers merging verified source and the
repository's normal automatic development-deployment/app-release
pipelines. No manual deployment, pCloud credential provisioning,
provider activation, remote-data mutation or issue closure is requested.

## Changes

- Opt-in per-app backend transport: account-bound one-use setup
capabilities, exact origin checks, scoped completion reads, signed
finalized delivery, unchanged idempotency/SSRF/redaction for existing
apps.
- Secure cookie sessions, CSRF-protected POST settings/disconnect,
explicit transcript/audio opt-in, authoritative US/EU OAuth callback
routing and truthful pending revocation.
- Encrypted transactional jobs with per-file receipts, stable
account/conversation paths, bounded retries, restart leases, expiry
scrubbing, key rotation and account/consent/generation fences.
- Saved audio uses the canonical Omi playback path, not UID-only live
buffers. Current app/account/conversation access is rechecked before
provider writes. Missing/unready audio is visible, not reported as
uploaded.
- Real FastAPI/provider/store tests are registered in both deterministic
manifest lanes; no production framework/model shims or public/default
encryption key.

## Architecture and rollout constraints

The standalone service requires one replica on an explicitly configured
persistent volume. It does not silently fall back from Redis to JSON or
deploy onto ephemeral Cloud Run disk. No existing Redis/file data is
read, deleted or automatically migrated. Any existing installation must
be inventoried and migrated explicitly before rollout.

The shared plugin SDK remains models-only. Transport and job
architecture/checklist are documented in
`backend/docs/plugin-transport.md` and
`plugins/omi-pcloud-app/README.md`; capture/playback ownership is
recorded in the pipeline guide. User-facing setup/status remains the
existing mobile/desktop integration flow.

`OMI_PLUGIN_TRANSPORT_JSON` and the matching standalone configuration
are intentionally unprovisioned. Reviewed runtime secret bindings on
every finalization host, a pCloud developer app/test account, minimal
provider permissions/callbacks, persistent-volume backup/restore, real
connected US/EU export/retry/revocation and separately authorized
rollout remain release gates. Live provider configuration has not been
verified; local fixtures are not provider credentials. **Code readiness
and activation are separate.** The new registered pCloud transport stays
inactive until provisioned; activation remains blocked on the release
gates above. Existing-client fresh-grant and credential-isolation fixes
do affect later app builds, so the whole PR is not described as
inactive. Keep #20213 open until they are verified.

Line-Count-Exception: backend/utils/apps.py | 1716 -> 1731 | Re-enable
validation owns the probe list, so the registered-transport branch
(trusted-origin check plus non-mutating health probe instead of an
unsigned delivery) belongs inside validate_app_endpoints_for_reenable
rather than a parallel validator.

## UI evidence

UI-Evidence: none

Both app-home and setup-step actions fetch fresh launch grants. Flutter
`app_detail.dart`, macOS `AppsPage.swift`, Windows `AppDetailSheet.tsx`
and web `AppDetail.tsx` retain the existing layout. Web uses a
same-styled button, reserves a tab synchronously, then navigates it to
the fresh URL. A failed refresh closes the reserved tab and shows a
retryable inline error. Local browser evidence below uses synthetic
loopback grants, not a connected provider.

## Review follow-up

- **Launch grants on every client:** macOS, Windows and web reused the
grant loaded with the sheet, so a second Open (or one after the grant
expired) failed. Each now refetches app details right before opening, as
Flutter already did.
- **Folder changes for unfinished uploads:** a queued or retrying job
kept the folder captured at enqueue. It now adopts the current
destination before uploading and restarts its receipts there; a folder
change mid-upload fails the job with a retry prompt instead of splitting
it across folders.
- **Completed receipts after expiry:** expiry scrubbed completed and
canceled jobs too, wiping their per-file receipts. It now scrubs only
unfinished payloads; rows still go after the 30-day purge.
- **Failed backups beyond 30 entries:** the list was the newest 30 rows,
so older failed jobs lost their retry. It now returns the newest 30 plus
every failed or pending job (bounded by the existing 100 unfinished-job
cap).
- Each item has a test that failed before the fix: three plugin tests,
plus a Windows and a web test that open twice and expect a new grant
each time.

## Second review follow-up (2026-10-23)

- **Disconnect retires pending OAuth attempts.** Previously, with
attempts A and B pending, completing A, disconnecting, then completing B
restored an active connection. Disconnect now rotates a durable
per-account authorization epoch (also when no connection exists).
Connect stamps the epoch on the OAuth state; the callback rechecks it
under the account lock before the token exchange (a retired code is
never exchanged) and again after it (a token issued to a retired attempt
is revoked, not stored). If pCloud does not confirm that revocation, the
page says so and points to removing the app in pCloud; a newer
connection is never overwritten.
- **Windows/Linux first-time Install uses fresh grants.** `Apps.tsx`
`beginSetupFlow` built setup and completion URLs from the undecorated
catalog item. It now fetches this account's app details first, keeps
signed query bytes via `urlWithUidParam`, and launches, polls or enables
nothing after a failed read, an account change or an unmount. The
Install button stays busy during the read, so a repeated attempt gets a
new grant.
- Regressions that failed before the fix: four plugin route tests (two
pending attempts then Disconnect; Disconnect with no connection;
Disconnect during a paused token exchange, with revocation confirmed and
unconfirmed) and six Windows page-level Install tests. Guard tests also
cover a newer connection surviving a stale exchange, a new Connect after
Disconnect, another account staying connected, and canceled jobs staying
canceled.
- Evidence on `137509030a`: plugin suite 73 passed (real FastAPI routes
and SQLite, synthetic provider I/O, network denied); Windows
Apps/AppDetailSheet/appInstall 61 passed, typecheck clean; isolated
loopback service smoke (boot and restart) passed. No live pCloud OAuth:
the developer console is signed in, but app creation awaits pCloud
approval, so client credentials and a real token exchange remain
unverified.

## Earlier verification (before the 2026-10-23 update)

- `bash scripts/run-plugin-pcloud-tests.sh`: 67 real
installed-service/provider/storage/signing contract tests passed with
live provider requests blocked, including completed/partial receipt
preservation on disconnect.
- Selected backend runner: 132 tests passed in ten isolated files at the
unchanged local 0.30s CPU guard, covering app setup/install, real
finalized fanout composition, saved-audio access and existing
finalization/playback behavior.
- Route-policy inventory: 27 tests passed at the same unchanged CPU
guard; GET/HEAD declare scoped plugin authority without expanding the
legacy missing-route baseline.
- Pinned backend type check: 0 errors, 41,730 warnings; existing warning
debt remains.
- Actual local browser/HTTP path with synthetic provider IO:
account-bound link consumption/cookie handoff, capability URL scrubbing,
opt-in defaults, saved preferences, retry admission/completion and
failed revocation checked. Desktop and 390px embedded production-page
rendering inspected; no live pCloud account or native-client build was
used.
- Local PR preflight: 38 selected manifest checks passed. The
failure-class recurrence/history audit explicitly skips this shallow
checkout; full-history CI remains authoritative. Normal pre-push results
are recorded separately below.
- Normal pre-push gate passed without bypasses, including the same
60-test plugin suite, pinned backend type check, OpenAPI checks and
three changed backend test files. Its broad 2,064-file selector was
bounded by the repository's unchanged 40-file cap; this is not a claim
that the complete backend/platform suite ran locally.

Local source verification is on `bbace15c92`, based on main
`c84b9138f4c46b76728465656e4653308c64a289`. Main advanced ten
non-overlapping commits during this work; final GitHub merge-tree checks
remain a gate before any merge.

Expected consent behavior comes from #20213. Region routing, OAuth token
behavior and revocation are defined by [pCloud
authorization](https://docs.pcloud.com/methods/oauth_2.0/authorize.html),
[OAuth tokens](https://docs.pcloud.com/methods/oauth_2.0/) and
[logout](https://docs.pcloud.com/methods/auth/logout.html). Existing
mock metadata/response conventions are not treated as authority over
those contracts.

Failure-Class: FC-unsigned-disconnect-deletes-tokens

Line-Count-Exception:
desktop/macos/Desktop/Sources/MainWindow/Pages/AppsPage.swift | 3748 ->
3759 | Fresh home and setup-step launches and page/account retirement
stay in the existing detail-sheet owner; AppSetupURL owns fresh-read
fencing and signed-query preservation.

Line-Count-Exception: backend/routers/apps.py | 2673 -> 2696 | Existing
marketplace integration entry points need setup-capability decoration
and one GET/HEAD handoff; signing, authorization and audio logic are
extracted into focused utility modules, with route-composition tests.

Why not a new one-off guard: this PR provides the account-bound
transport primitive and durable state owner, then exercises production
HTTP/state transitions; the existing unsigned-disconnect class already
records this trust-boundary failure. The new manifest entry runs that
behavioral suite rather than scraping source for a preferred
implementation.

UI-Evidence: none — Python-rendered standalone setup is outside
released-client UI paths; local synthetic screenshots and browser
observations are recorded in this task, not advertised as
deployed/customer proof.


<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/BasedHardware/omi/pull/22826?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="View guided diff"
src="https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a>
<a
href="https://www.cubic.dev/action/auto-fix/pr/BasedHardware/omi/22826?returnTo=https%3A%2F%2Fgithub.com%2FBasedHardware%2Fomi%2Fpull%2F22826&source=description"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/turn-off-auto-fix-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/turn-off-auto-fix-light.svg"><img
alt="Turn off auto-fix"
src="https://www.cubic.dev/buttons/turn-off-auto-fix-light.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->





## 2026-10-23 maintainer review and merge gates

Ready for code review at Aryan's request; the gates below block merge,
not review. The receipt/folder/retry-history fixes from the prior review
are present. This pass resolves the current-main manifest conflicts,
refreshes both home and setup-step grants on every client, preserves
signed query parameters on Windows, rejects stale/failed launch reads,
and removes mobile vendor completion bearer forwarding, signed-URL
logging and app-detail body logging. The existing catalog owner also
rejects retired account/ABA detail reads. Existing legacy apps without a
completion endpoint keep optional setup. Local evidence: 67 installed
plugin/provider/store tests, 78 isolated backend tests, 25 Flutter
launch/authorization/privacy tests, 46 Windows renderer/install tests,
10 native Swift URL/launch-boundary tests, final Flutter analyzer, and
the complete web check (6 Bun smoke + 876 Vitest tests) pass. The full
macOS debug build also passed in 619.16 seconds with two jobs; this is
not macOS 14 runtime acceptance or the full desktop test suite. A real
browser with the production AppDetail and synthetic loopback HTTP grants
accepted distinct grants on repeated setup clicks, closed a reserved tab
after a forced 503, and recovered on retry. These are synthetic provider
boundaries, not live pCloud OAuth/US/EU exports or installed-client
acceptance. The source fix is c43b217; documentation-only follow-ups
7f34a3c and 9deac74 repair the backend instruction budget and pin
a removed historical page reference to its measured source snapshot. At
that historical checkpoint those items remained open. The current
code-readiness and separate activation decisions are recorded in the
2026-10-25 update below. The mobile vendor isolation intentionally
overlaps #21827; retain that boundary when integrating the two PRs. No
deployment or credentials provisioned.




## Maintainer readiness and scope update (2026-10-25)

- Final reviewed head: `922986c34d2f8e86595c48c48e7616d2c5fb7e46`; no
new blocking code defect identified in the current follow-up. Fresh
local `bash scripts/run-plugin-pcloud-tests.sh`: 73 passed with
synthetic provider IO and network denied. The changed transport
implementation is unchanged from the previously reviewed head; prior
valid browser, client-launch, storage, account/epoch and revocation
regression evidence is reused, not presented as a fresh live-provider
run.
- Current-head CI is green: 46 successful checks, 15 appropriately
skipped, none pending/failing before readiness. All six backend shards
and aggregate, required Mobile Release Eligibility, backend hermetic
gate, Windows renderer/build smoke, Swift contracts, Flutter checks,
Android emulator acceptance and iOS compile passed. The earlier head's
backend wall-budget failure is not a failure of this head.
- Local metadata-only preflight: five selected checks passed; final diff
check and merge-tree against current main are clean. No baseline, timing
budget, branch protection or required check was bypassed. The earlier
shallow local history skip is not represented as a full-history ratchet
pass.
- The maintainer explicitly accepted the repository's normal automatic
development deployments and app release train accompanying this merge.
This is not authorization to create/configure the pCloud developer app
or provision its transport. Those remain separate operator work.
- Keep `OMI_PLUGIN_TRANSPORT_JSON` and the standalone provider
configuration unprovisioned. Before pCloud activation, verify approved
company/provider ownership, minimal grants and exact callbacks, real
US/EU export/retry/revocation, and persistent-volume backup/restore.
Keep #20213 open; a source merge is not customer recovery or proof of
live-provider operation.
- This PR overlaps #21827 in mobile app setup and completion handling.
Integrate the second PR against the merged first PR, retain
vendor-bearer isolation/fresh grants and account/attempt ownership,
regenerate API clients after schema merges, and rerun the affected
tests.
Resolve app-setup overlap with merged pCloud transport, regenerate the combined TypeScript and Swift client counts, and cover fresh grants followed by delayed completion and exactly one enable. Preserve vendor bearer isolation and request/account ownership.
An in-flight callback must compare its owner and PKCE verifier atomically before persisting or enabling. Cover supersession at both provider awaits, current callback success, replay, retired ownership and local HTTP/Redis/Firestore integration.

Failure-Class: none
Comment thread backend/tests/integration/test_mcp_oauth_recovery_emulator.py Fixed
Load the actual apps module and patch only its database seam. Preserve in-place updates and missing-document errors instead of shadowing SDK imports. Register the migrated test in the clean subset and remove two references to already-removed files.
The recovery PR preserves Firebase auth-domain, helper proxy and callback configuration. Keep the actual MCP backend-before-client release dependency, and retain provider configuration/round-trip verification for #21355 and incident closure without turning unchanged external settings into an extra code-merge gate.
The managed CodeQL JavaScript job failed with an api.github.com connection timeout, and GitHub rejects job/run retries for this dynamic workflow. Retrigger normal checks without changing source, assertions, security settings or required gates.
@aryanorastar
aryanorastar marked this pull request as ready for review October 25, 2026 10:48
@aryanorastar
aryanorastar merged commit d2bcd55 into main Oct 25, 2026
66 checks passed
@aryanorastar
aryanorastar deleted the fix/connection-completion-recovery branch October 25, 2026 10:50

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 918e2b6345

ℹ️ 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".

Comment on lines +39 to +40
final opened = await launchIntegrationOAuth(
() async => widget.url.toString(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fetch a fresh launch grant when reopening an app home

For transport-authenticated app homes, returning from the initial browser launch and pressing Open again reuses the same single-use grant because every launch reads the fixed widget.url. This fails even if the browser still has a valid session: plugins/omi-pcloud-app/main.py checks and consumes the supplied token before consulting the session, and rejects a reused nonce with HTTP 401. AppHomeWebPage supplies the signed URL only once, so this new reopening action cannot recover without leaving the page. Allow app-home callers to fetch fresh details and obtain a new launch URL for each Open/retry action, while retaining fixed URLs for referral pages.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flutter flutter work needs-maintainer-review Needs a human maintainer to sign off before merge positive-signal Automation verified a genuine fix/quality contribution security-review Touches auth, provider routing, secrets, or security-sensitive surfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants