feat: agentic cli qr login flow#30911
Conversation
There was a problem hiding this comment.
I don't think we've actually ever used the dev relay. I'm not certain if it makes sense to do so now or not. I would actually make test-dapp testing a little more cumbersome if we did use the dev relay here.
There was a problem hiding this comment.
+1, agreed — and I think this is actually a break, not just inconvenience. the dApp side mmconnect code hardcodes the prod relay with no env switch (connect-monorepo config), and the relay is a shared Centrifuge pub/sub broker — both ends have to be on the same server + channel or the handshake just times out. since *_dev builds set METAMASK_ENVIRONMENT='dev', a dev build would point at dev-api while every real dApp (and the test-dapp) stays on prod, so they'd never connect. also breaks resume() for connections that were established on prod.
this relayURL feeds handleConnectDeeplink too, so it hits the whole standard dApp path, not just agentic CLI. i'd keep the registry on prod (or have getMwpRelayUrl() return prod unconditionally) and scope any dev-relay override to the agentic flow if it actually needs one.
I would need to consult with @chakra-guy but IIRC last time I asked him he confirmed there is no active dev relay
| "expires_in": "Expires in {{time}}", | ||
| "expired": "Code expired", | ||
| "security_notice": "Securing the connection first. You'll authorize CLI access in the next step" | ||
| }, |
There was a problem hiding this comment.
should this have so much reference to CLI? I know the CLI is the only one triggering this flow by specifying an "untrusted" connection though
There was a problem hiding this comment.
Yeah agreed, maybe worth making this more generic but fine if we want to go with this since this is currently the only case where untrusted mode is used 🤔
| } | ||
| } | ||
|
|
||
| private async cleanupConnection(conn: Connection): Promise<void> { |
There was a problem hiding this comment.
this.disconnect already calls conn.disconnect. Is there a reason why the cleanupConnection helper is needed now?
There was a problem hiding this comment.
on a failed connect, conn isn't in this.connections yet (we only add it after connect() succeeds), so old this.disconnect(conn.id) → this.connections.get(id)?.disconnect() → no-op. that meant bridge.dispose() never ran, so we leaked the RPCBridgeAdapter (its global KeyringController:unlock sub + BackgroundBridge). cleanupConnection just calls conn.disconnect() directly so the bridge actually gets cleaned up. (transport's not the leak — WalletClient already tears that down itself on failure.) So that's all to say this is a good change for both the agentic and standard paths!
| export const showAgenticCliConnectionLoading = ( | ||
| conninfo: ConnectionInfo, | ||
| ): void => { | ||
| store.dispatch( | ||
| showSimpleNotification({ | ||
| id: conninfo.id, | ||
| autodismiss: AGENTIC_CLI_LOADING_AUTODISMISS_MS, | ||
| title: strings('sdk_connect_v2.show_loading.title'), | ||
| description: strings('sdk_connect_v2.show_loading.description', { | ||
| dappName: conninfo.metadata.dapp.name, | ||
| }), | ||
| status: 'pending', | ||
| }), | ||
| ); | ||
| }; | ||
|
|
||
| export const hideAgenticCliConnectionLoading = ( | ||
| conninfo: ConnectionInfo, | ||
| ): void => { | ||
| store.dispatch(hideNotificationById(conninfo.id)); | ||
| }; |
There was a problem hiding this comment.
thoughts on adding an overridable autodismiss timeout to app/core/SDKConnectV2/adapters/host-application-adapter.ts showConnectionLoading work instead of having the dispatch logic in two different places?
There was a problem hiding this comment.
Agreed, looks like the loading toast here is the same as the adapter's showConnectionLoading (identical strings/shape, just a 15s vs 10s autodismiss), so it might be worth consolidating rather than keeping a near-duplicate. Could be as simple as letting the adapter method take an optional timeout, but I'll leave the approach to you. The OTP UI feels genuinely agentic-specific, so that's fine to keep separate. Non-blocking either way.
| agenticCliStage, | ||
| url: redactUrl(url), | ||
| }); | ||
| deps.hostapp.showConnectionError(); |
There was a problem hiding this comment.
Error toast on dashboard close
Medium Severity
Closing or rejecting dashboard approval rejects open() with a user-facing closed message, but the agentic deeplink catch still calls showConnectionError(), so users see the generic “failed to establish connection” toast after an intentional dismiss.
Reviewed by Cursor Bugbot for commit db0be4b. Configure here.
There was a problem hiding this comment.
It is fine to show error toast when user dismiss or reject webview screen
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 53522dd. Configure here.
| await this.handleAgenticCliDeeplink(url, agenticCliReq); | ||
| } else { | ||
| await this.handleConnectDeeplink(url); | ||
| } |
There was a problem hiding this comment.
Invalid agentic-cli uses standard MWP
High Severity
When a connect deeplink declares connectionType.name of agentic-cli but fails agentic validation (e.g. non-HTTP dashboardUrl), routing treats it as a normal MWP session. That runs the trusted connect path and can persist the connection instead of the untrusted Agent CLI QR flow.
Reviewed by Cursor Bugbot for commit 53522dd. Configure here.
There was a problem hiding this comment.
it is expected to re-route to normal mwp session on failing of parsing agentic link
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: Performance Test Selection: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #30911 +/- ##
==========================================
+ Coverage 82.73% 82.84% +0.11%
==========================================
Files 5576 5596 +20
Lines 143554 144561 +1007
Branches 33190 33593 +403
==========================================
+ Hits 118774 119768 +994
+ Misses 16874 16723 -151
- Partials 7906 8070 +164 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
add follow up pr to address non blocking comment |
…taMask#31123) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until this PR meets the canonical Definition of Ready For Review in `docs/readme/ready-for-review.md`. In short: the template must be materially complete (not just section titles present), all status checks must be currently passing, and the only expected follow-up commits must be reviewer-driven. address comment : MetaMask#30911 (comment) --> ## **Description** Agentic CLI connection loading duplicated the same notification logic already implemented in the SDK Connect V2 host application adapter (`agenticCliLoading.ts` dispatched `showSimpleNotification` / `hideNotificationById` directly). This PR removes that duplicate module and routes Agentic CLI loading through `deps.hostapp.showConnectionLoading` / `hideConnectionLoading`, keeping a single code path for connection-loading UI. `showConnectionLoading` now accepts optional `ShowConnectionLoadingOptions` with `autodismissMs`. The default remains **10s** for standard SDK Connect flows; Agentic CLI passes **15s** via `AGENTIC_CLI_CONNECTION_LOADING_AUTODISMISS_MS` to accommodate the longer MWP → OTP → dashboard connect sequence. ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: MetaMask#30911 (comment) ## **Manual testing steps** ```gherkin Feature: Agentic CLI connection loading Scenario: user connects via Agentic CLI deeplink Given the app is unlocked and Agentic CLI connect is available When user opens an Agentic CLI connect deeplink Then a connection-loading notification appears with the dApp name And the notification auto-dismisses after ~15s if the flow stalls And the notification is hidden when the connect flow completes or fails Scenario: standard SDK Connect V2 connection loading is unchanged Given a non–Agentic CLI SDK Connect V2 session is establishing When connection loading is shown via the host adapter Then the loading notification still auto-dismisses after ~10s by default ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> N/A ## **Pre-merge author checklist** <!-- Every checklist item must be consciously assessed before marking this PR as "Ready for review". A checked box means you deliberately considered that responsibility, not that you literally performed every action listed. Unchecked boxes are ambiguous: they are not an implicit "N/A" and they are not a silent "skip". See `docs/readme/ready-for-review.md` for the full checklist semantics. --> - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. #### Performance checks (if applicable) - [x] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [x] I've tested with a power user scenario - Use these [power-user SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [x] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **Pre-merge reviewer checklist** <!-- Reviewer checklist items follow the same semantics as the author checklist: an unchecked box is ambiguous, a checked box means the reviewer consciously assessed that responsibility. See `docs/readme/ready-for-review.md`. --> - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > UI notification plumbing refactor with no auth or data-path changes; default 10s autodismiss behavior is preserved for non–Agentic CLI callers. > > **Overview** > Removes the Agentic CLI–specific loading helpers in `agenticCliLoading.ts` and routes connection loading through the shared SDK Connect V2 **host application adapter** instead. > > `showConnectionLoading` / `hideConnectionLoading` on `IHostApplicationAdapter` now accept optional `ShowConnectionLoadingOptions` with `autodismissMs` (default **10s**; Agentic CLI uses exported **15s** for longer MWP → OTP → dashboard flows). `AgenticCliMwpConnectionService` calls `deps.hostapp.showConnectionLoading` / `hideConnectionLoading` with that timeout rather than dispatching notifications directly. Tests were updated accordingly and adapter coverage was added for custom autodismiss. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit aa778c8. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Description
Adds end-to-end support for linking MetaMask Agent CLI via QR scan on mobile, and refactors that flow out of generic SDK Connect V2 handling into a dedicated
AgenticClimodule.Why: Agentic CLI connections differ from standard dApp MWP sessions—they are short-lived, use an OTP pairing step, require dashboard project selection, and must return an auth token to the CLI rather than persisting a wallet session in the connection registry.
This pr is based on the pr #30442 done by @tylerc-consensys
What changed:
app/core/AgenticCli/— New module that owns agentic CLI deeplink parsing (connectionType: { name: 'agentic-cli' }), MWP connection lifecycle (untrusted mode, no persistent save), OTP UI hooks, loading toasts, and the QR login pipeline:/api/v2/mm-qr-login/tokenauth-tokenmessage back to the CLI over MWPDashboard WebView (
AgenticCliDashboardWebview) — Modal screen + service with build-type origin allowlists, auth token in URL hash +Authorizationheader, strictmm-agentic-clipostMessage parsing, pending-request resolve/reject with 5-minute timeout, and token redaction in logs. QR-supplieddashboardUrl/dashboardAuthUrloverrides are ignored in favor of configured endpoints.OTP modal (
SDKConnectV2OtpModal) — Bottom sheet shown during MWP pairing with formatted OTP, countdown (1-minute display cap), and auto-dismiss on connect.SDK Connect V2 refactor —
ConnectionRegistryroutes agentic deeplinks to the new handler; MWP payload parsing and analytics move to shared utils (parseMwpConnectDeeplink,trackMwpEvent); relay URL respects dev vs prod; failed connects usecleanupConnectioninstead of always callingdisconnect.Changelog
CHANGELOG entry: Added QR login flow for MetaMask Agent CLI, including OTP pairing and dashboard project selection in a WebView.
Related issues
Fixes:
Manual testing steps
Build-type dashboard URLs
*_devhttps://test-dashboard.web3auth.io/agentic/login*_uathttps://dev-dashboard.web3auth.io/agentic/login*_prodhttps://dashboard.w3a.io/agentic/loginScreenshots/Recordings
Before
After
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist
Test coverage added/updated
AgenticCliMwpConnectionService.test.ts— deeplink handling, event wiring, error/cleanup pathsAgenticCliQrLoginService.test.ts— Hydra token → dashboard token → WebView → send auth tokenAgenticCliDashboardWebviewService.test.ts— origin allowlists, URL building, postMessage parsing, pending requestsAgenticCliDashboardWebview/index.test.tsx— WebView rendering, message handling, external link policySDKConnectV2OtpModal.test.tsx— OTP display and countdownconnection-registry.test.ts— agentic deeplink routing delegationSecurity notes for reviewers
source: "mm-agentic-cli"and canonical result types (approved,rejected,close,error).Note
High Risk
Touches authentication (Hydra bearer, dashboard tokens), WebView origin allowlists and postMessage handling, and MWP deeplink routing—security-sensitive paths with a large new surface area.
Overview
Adds MetaMask Agent CLI QR linking as a dedicated flow separate from normal SDK Connect V2 sessions: agentic MWP deeplinks (
connectionType: agentic-cli) are handled in a newapp/core/AgenticClimodule instead of persisting through the generic connection registry.The flow shows an OTP bottom sheet during pairing, runs a short untrusted MWP connect, then Hydra → dashboard token → allowlisted dashboard WebView for project approval, and returns an
auth-tokento the CLI before tearing down the temporary connection. QR-supplied dashboard URLs are ignored in favor of build-type config.New UI includes
SDKConnectV2OtpModal,AgenticCliDashboardWebview(origin allowlists,mm-agentic-clipostMessage contract, pending promise + timeout), and navigation routes. SDK Connect V2 now delegates agentic deeplinks, extractsparseMwpConnectPayload/trackMwpEvent, and usescleanupConnectionon failed connects.Reviewed by Cursor Bugbot for commit 53522dd. Bugbot is set up for automated code reviews on this repo. Configure here.