Repository navigation
refactor(cdp): migrate all scanners to in-process transport; drop TCP DevTools port - #3636
Conversation
Drop the local duplicated CdpConn / browser_ws_url / CDP_HOST / CDP_PORT
plumbing and route through crate::cdp::connect_and_attach_matching_in_process,
which uses the per-account in-process DevTools channel installed by
webview_accounts::open. Net effect: no TCP loopback attach needed for
either full or fast scan tick.
- Drop tokio_tungstenite + futures_util::{SinkExt, StreamExt} + connect_async
imports (no remaining users in this module).
- Switch dom_snapshot.rs and idb.rs to crate::cdp::CdpConn.
- scan_dom_once now takes &AppHandle<R> so the in-process transport can be
looked up; spawn_scanner loop passes it through.
- Predicate compares page url prefix/suffix; canonical helper already
filters kind == "page".
Drop the streaming-MITM CdpConn duplicate (and its tungstenite pending table that V1 never used) in favour of the canonical in-process CdpConn. crate::cdp::connect_and_attach_matching_in_process supplies the attached session, then CdpConn::pump_events streams every Network.* / WebSocket.* event into the existing dispatch_event closure with mutable DiscordIngestState captured in the closure. - Drop CDP_HOST / CDP_PORT import + tokio_tungstenite + futures_util + tokio::sync::oneshot. - Drop local CdpTarget / parse_targets / browser_ws_url helpers. - Drop the local CdpConn struct + impl + its custom pump_events; the canonical pump_events is sufficient for V1's events-only stream. - dom_scan_once now takes &AppHandle<R> + account_id so it can use the in-process attach helper; spawn_dom_poll passes them through. - pump_events callback signature is FnMut(&str, &Value) which fits dispatch_event's sync per-event invocation; pending-table machinery re-introduced when Network.getResponseBody is wired up.
The slack scanner had two TCP-WS attach paths (IDB scan + DOM scan), each with its own local CdpConn duplicate. Both move to the canonical crate::cdp::CdpConn via the per-account in-process transport. CDP transport - WebviewCdpTransport::call now delegates to call_with_timeout(timeout); the default 35s budget unchanged for existing callers. - CdpConn (the per-attach handle) gains call_with_timeout that forwards to the in-process transport — required because slack's IDB batch serialise (Runtime.callFunctionOn over ~32 RemoteObjects) needs a 60s budget which the legacy local CdpConn used to provide. Slack scanner - Drop CDP_HOST / CDP_PORT import + tokio_tungstenite + futures_util. - Drop local CdpTarget / parse_targets / browser_ws_url / CdpConn / CdpConn::call_with_timeout — the canonical CdpConn now provides the same surface. - scan_once and dom_scan_once now take &AppHandle<R> and route through crate::cdp::target::conn_for_account, reusing the in-process channel for both target enumeration and attach. The pin / strict / relaxed resolution hierarchy is preserved verbatim. - idb.rs switches its CdpConn import to crate::cdp::CdpConn so the 60s call_with_timeout call site keeps working unchanged.
Mirror of the slack migration — drop the local CdpConn duplicate (with its own 30s default + 60s call_with_timeout) and route through crate::cdp::target::connect_and_attach_matching_in_process. The canonical CdpConn::call_with_timeout (added in the slack commit) covers the 60s budget the IDB batch serialise needs. - Drop CDP_HOST / CDP_PORT import + tokio_tungstenite + futures_util. - Drop local CdpTarget / parse_targets / browser_ws_url / CdpConn. - scan_once + dom_scan_once now take &AppHandle<R> + account_id and reuse the per-account in-process channel for both target enumeration and attach. - idb.rs switches its CdpConn import to crate::cdp::CdpConn.
Single TCP-WS callsite swaps from crate::cdp::connect_and_attach_matching (legacy WebSocket) to crate::cdp::target::connect_and_attach_matching_in_process. scan_once now takes &AppHandle<R> + account_id so it can look up the per-account in-process transport.
The Meet stack opens its own WebviewWindow per call (label "meet-call-<request_id>") and four TCP-WS attach paths used to drive audio bridge, video bridge, caption listener, camera diagnostics, and join scanner. All four move to the per-webview in-process CDP transport. CDP transport - Add install_for_label / conn_for_label / connect_and_attach_matching_in_process_by_label as label-keyed counterparts of the account-keyed helpers. Required because Meet webviews use "meet-call-<request_id>" not "acct_<id>". - install_for_account now wraps install_for_label. meet_call - After the WebviewWindow is built, call cdp::install_for_label so the Meet webview's in-process transport is wired up before any of the downstream bridges attach. Idempotent; transient failures fall back to the conn_for_label retry path. meet_scanner / meet_audio / meet_video - Thread request_id through wait_for_meet_target, install_audio_bridge, and spawn_diagnostics_poller so each attach resolves the Meet window's label and goes through the in-process channel instead of TCP loopback. - Caption listener's second attach also routes through connect_and_attach_matching_in_process_by_label.
With every scanner (whatsapp / discord / slack / telegram / wechat / meet_video / meet_audio / meet_scanner) now on the in-process CDP transport, the legacy TCP loopback DevTools surface no longer has any callers and can be removed. - cdp/target.rs: delete browser_ws_url() and connect_and_attach_matching(); drop the Duration import that was only needed for the reqwest timeout. - cdp/conn.rs: rewrite CdpConn to be a thin wrapper over Arc<WebviewCdpTransport> — drop CdpConn::open_ws, the LegacyWs enum variant, the futures_util / tokio_tungstenite / oneshot uses, and the LEGACY_CALL_TIMEOUT constant. call / call_with_timeout / pump_events now forward unconditionally to the in-process transport. - cdp/mod.rs: drop CDP_HOST / CDP_PORT constants, drop browser_ws_url and connect_and_attach_matching re-exports, add connect_and_attach_matching_in_process / connect_and_attach_matching_in_process_by_label / conn_for_label to the re-export list. Module-level doc rewritten to describe the single remaining transport. - gmessages_scanner: refresh TODO comment that referenced CDP_HOST / CDP_PORT — the scaffold should reuse the in-process helpers when it graduates to a running scanner.
With every scanner on the in-process CDP transport the loopback DevTools listener is no longer needed. Dropping the "--remote-debugging-port=19222" command-line arg means CEF starts without binding any TCP DevTools surface, so same-UID processes can no longer attach a CDP client at 127.0.0.1:19222. Comment refresh in webview_accounts to drop the stale reference to the listener that no longer exists.
📝 WalkthroughWalkthroughAll CDP communication is migrated from a dual-backend (in-process + legacy loopback WebSocket) model to exclusively in-process transport. ChangesIn-process CDP migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
sanil-23
left a comment
There was a problem hiding this comment.
Approve ✅ (with comments — one fast-follow filed)
Security-positive, carefully executed refactor that finishes the in-process CDP cutover and closes the unauthenticated same-UID loopback DevTools surface. CI is fully green. The mechanical migration of all six scanners preserves every predicate, attach shape, pin/fallback hierarchy, and log-and-continue error path; the get_or_create install path correctly serializes observer registration under the registry mutex (CEF has no un-register hook), and pending-map cleanup is leak-free on all three exit paths.
One Major (medium-confidence) concern is Discord-specific and tracked as a fast-follow rather than gating this security win: #3693. Discord is the only scanner using the long-lived pump_events loop, and (1) it can't self-heal a dead/stale session — pump_events only returns on transport teardown, not on page-target loss (Ctrl+R / renderer crash / SPA hard-nav), so it never re-attaches until the webview is destroyed; and (2) the 256-slot broadcast drops frames on Lagged with no idle watchdog, despite the cdp/conn.rs comment promising one. See inline comments.
Non-blocking items to fix in this PR (zero-risk):
- The
run_mitm_sessiondoc comment ("Returns when the underlying WebSocket closes, the page target disappears…") and thecdp/conn.rsLagged comment ("the caller's idle watchdog will eventually time out the session") no longer match the in-process implementation — please correct both. cdp/in_process.rs:519cites integration testcdp_in_process_e2e.rs, which doesn't exist anywhere in the repo (pre-existing from #3498). Either land it or drop the reference;cdp/conn.rs+cdp/target.rscurrently ship with zero unit tests, and the newpump_eventssession-id filter is pure, testable logic.
Minor / low-confidence: call_with_timeout changed from a per-read timeout (reset on every inbound frame) to a total wall-clock cap. For Slack's 60s IDB walk (slack_scanner/idb.rs:314) this is almost certainly equivalent (dedicated session, no interleaved frames), but it's a real semantic change worth a line in the parity contract rather than "preserved verbatim."
Nice work overall — approving on the basis that #3693 owns the Discord watchdog and the doc/test-reference cleanups land here.
| // closure. V1 doesn't issue any in-stream calls (responses table from | ||
| // the previous TCP impl is gone — re-introduce a request/response API | ||
| // here when V1.5 backfills `Network.getResponseBody`). | ||
| log::info!("[discord][{}] event pump started", account_id); |
There was a problem hiding this comment.
Major (medium confidence) — tracked in #3693. This long-lived pump can't self-heal a dead session: pump_events only returns on RecvError::Closed (transport/webview teardown), not when the attached page target dies (Discord Ctrl+R, renderer crash, SPA hard-nav). Target.detachedFromTarget isn't acted on (discovery never enabled; dispatch_event ignores unknown methods), so the outer reconnect loop is never re-entered and the MITM silently stops capturing until the webview is destroyed.
Suggest an idle watchdog (> Discord's ~41s gateway heartbeat, e.g. 90s) so a stalled/destroyed session forces a return → re-attach. Also: the run_mitm_session doc comment above ("Returns when the underlying WebSocket closes, the page target disappears, or any dispatch hits an unrecoverable error") is now false for all three clauses — please correct it in this PR.
| } | ||
| on_event(&method, ¶ms); | ||
| } | ||
| Err(RecvError::Lagged(skipped)) => { |
There was a problem hiding this comment.
Major (medium confidence) — tracked in #3693. On Lagged the pump logs and continues, silently dropping CDP frames once the single consumer falls >256 behind (EVENT_CHANNEL_CAP). The comment above states recovery relies on "the caller's idle watchdog" — but the Discord consumer (the only long-lived pump) has no watchdog, so dropped Network.* frames during a burst = missed messages with no re-sync. Either validate 256 against Discord's burst volume or drain this consumer into an unbounded queue, and reword the comment so it doesn't promise a watchdog that doesn't exist for this caller.
# Conflicts: # app/src-tauri/src/cdp/mod.rs # app/src-tauri/src/cdp/target.rs # app/src-tauri/src/lib.rs
The doc comment promised "the caller's idle watchdog will eventually time out the session" but the long-lived Discord consumer has no watchdog, so Lagged-dropped frames are not re-synced. Reword to describe the actual behaviour and reference the fast-follow (tinyhumansai#3693) that adds the watchdog. Addresses @sanil-23 on cdp/conn.rs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src-tauri/src/whatsapp_scanner/mod.rs (1)
282-351: 💤 Low valueRemove dead code:
let _ = app;Line 348 has
let _ = app;which appears to be leftover from the refactoring. Theappparameter is already used on lines 297-300 forconnect_and_attach_matching_in_process, so this binding serves no purpose.🧹 Proposed cleanup
let _ = cdp .call( "Target.detachFromTarget", json!({ "sessionId": page_session }), None, ) .await; - let _ = app; snap.capture_report = capture_report; Ok(snap) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src-tauri/src/whatsapp_scanner/mod.rs` around lines 282 - 351, Remove the dead code `let _ = app;` from the scan_once function. The app parameter is already used earlier in the function when calling connect_and_attach_matching_in_process on lines 297-300, so this binding is unnecessary leftover from refactoring and should be deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src-tauri/src/whatsapp_scanner/mod.rs`:
- Around line 282-351: Remove the dead code `let _ = app;` from the scan_once
function. The app parameter is already used earlier in the function when calling
connect_and_attach_matching_in_process on lines 297-300, so this binding is
unnecessary leftover from refactoring and should be deleted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fe24e45-044b-461c-976f-f4f897c94bc2
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
app/src-tauri/src/cdp/conn.rsapp/src-tauri/src/cdp/in_process.rsapp/src-tauri/src/cdp/mod.rsapp/src-tauri/src/cdp/target.rsapp/src-tauri/src/discord_scanner/mod.rsapp/src-tauri/src/gmessages_scanner/mod.rsapp/src-tauri/src/lib.rsapp/src-tauri/src/meet_audio/inject.rsapp/src-tauri/src/meet_audio/mod.rsapp/src-tauri/src/meet_call/mod.rsapp/src-tauri/src/meet_scanner/mod.rsapp/src-tauri/src/meet_video/inject.rsapp/src-tauri/src/slack_scanner/idb.rsapp/src-tauri/src/slack_scanner/mod.rsapp/src-tauri/src/telegram_scanner/idb.rsapp/src-tauri/src/telegram_scanner/mod.rsapp/src-tauri/src/webview_accounts/mod.rsapp/src-tauri/src/wechat_scanner/mod.rsapp/src-tauri/src/whatsapp_scanner/dom_snapshot.rsapp/src-tauri/src/whatsapp_scanner/idb.rsapp/src-tauri/src/whatsapp_scanner/mod.rs
… DevTools port (tinyhumansai#3636)\n\n
Summary
--remote-debugging-port=19222to CEF at startup. The Tauri shell no longer binds a loopback DevTools listener.CdpConn::open_ws,browser_ws_url,connect_and_attach_matching,CDP_HOST,CDP_PORT, and the four scanner-localCdpConnduplicates (whatsapp, discord, slack, telegram).install_for_label/conn_for_label/connect_and_attach_matching_in_process_by_label) so Meet'smeet-call-<request_id>webview can attach without a TCP listener.call_with_timeoutso Slack's IDB batch serialise (Runtime.callFunctionOnover ~32 RemoteObjects) keeps the 60s budget it had on the legacy transport.Problem
#3498 landed the in-process CDP transport (
Webview::send_dev_tools_message+on_dev_tools_protocol) but only migrated the per-account session opener. Six scanner modules still attached overhttp://127.0.0.1:19222/json/version→ loopback WebSocket. While any one of them kept that code path alive, the Tauri shell had to keep passing--remote-debugging-port=19222, which binds an unauthenticated DevTools listener that every same-UID process on the machine can attach to.Concrete consequences:
puppeteer.connect({ browserURL: 'http://127.0.0.1:19222/' })and read or modify the embedded webviews (WhatsApp, Slack, Telegram, Discord, etc.).struct CdpConn(whatsapp / discord / slack / telegram / meet scanners) plus their per-modulebrowser_ws_url()andparse_targets()duplicates kept piling drift on top of the same machinery.Solution
CDP transport
cdp/in_process.rs: extract the registry-lookup-or-install body intoinstall_for_label(label)and rewriteinstall_for_accountasinstall_for_label(format!("acct_{id}")). Required because Meet's CEF window uses labelmeet-call-<request_id>, notacct_<id>.WebviewCdpTransport::callis split intocall/call_with_timeout; defaultcallkeeps the 35s budget,call_with_timeoutlets Slack's IDB walker pass 60s.cdp/target.rs: addconn_for_label<R>(app, label)andconnect_and_attach_matching_in_process_by_label<R, F>(app, label, pred)as label-keyed counterparts of the existing account-keyed helpers. Both fall back toinstall_for_labelon a cache miss so a transient install race at window creation doesn't permanently lock the surface out of CDP.cdp/conn.rs: rewriteCdpConnas a thin wrapper overArc<WebviewCdpTransport>. DropLegacyWs,connect_async,tokio_tungstenite::Message, theoneshotpending table, andLEGACY_CALL_TIMEOUT.call/call_with_timeout/pump_eventsforward unconditionally to the in-process transport.cdp/mod.rs: dropCDP_HOST/CDP_PORTand thebrowser_ws_url/connect_and_attach_matchingre-exports; addconn_for_label,connect_and_attach_matching_in_process[_by_label],install_for_labelto the surface.Scanners
For every scanner, the per-tick attach now reads:
For Meet (which is window-keyed, not account-keyed):
Per-scanner deltas:
CDP_HOST/CDP_PORT, localCdpConn,browser_ws_url,parse_targets,CdpTarget;scan_dom_oncenow takes&AppHandle<R>so the in-process transport can be looked up; siblingdom_snapshot.rsandidb.rsswitch theirCdpConnimport tocrate::cdp::CdpConn.crate::cdp::CdpConn::pump_eventswith a closure capturing&mut DiscordIngestState. V1 doesn't issue in-stream calls, so dropping the unusedpending: HashMapis a simplification, not a regression.dom_scan_oncegains&AppHandle<R>+account_id.CdpConnduplicates (the one withcall_with_timeout); bothscan_onceanddom_scan_oncenow take&AppHandle<R>and reuse the per-account in-process channel for both target enumeration and attach. The pin / strict-fragment / relaxed/clientfallback hierarchy is preserved verbatim.meet_call::window_label_for(request_id).install_audio_bridge,spawn_diagnostics_poller, andwait_for_meet_targetthread&AppHandle<R>+request_idthrough.Wiring
meet_call/mod.rs: afterWebviewWindowBuilder::build(), callcrate::cdp::install_for_label(&label)so the Meet webview's in-process transport is wired up before any downstream bridge attaches. Idempotent; on transient install failure the downstreamconn_for_labelretries.lib.rs: deleteargs.push(("--remote-debugging-port", Some("19222")))so the embedded CEF browser never binds a TCP DevTools listener at startup. Surrounding comments rewritten to match.webview_accounts/mod.rs: comment refresh — the listener no longer exists.gmessages_scanner/mod.rs: TODO comment refreshed to point atconn_for_account/connect_and_attach_matching_in_processinstead of the long-goneCDP_HOST/CDP_PORT.Submission Checklist
Target.getTargetsresolution. Existing test coverage for each scanner exercises the swapped attach path unchanged. (Tests added or updated per Testing Strategy.)Closes #NNNin the## Relatedsection.Impact
http://127.0.0.1:19222/json/versionwill fail withECONNREFUSED; nothing in-tree depends on that surface.Webview::send_dev_tools_messagechannel, which stays within the process boundary.connect_asynchandshake + tungstenite read loop. Attach setup goes from a TCP/json/versionGET + WebSocket handshake to a single registry lookup.tauri::Webview::open_devtools()or attach from inside the running process. No production code path relied on it.Related
tokio-tungstenitestays inCargo.tomlbecausemeet_video/frame_bus.rsandwebview_apis/server.rsstill use it for their WebSocket servers. Once those land their own in-process replacements the crate can be dropped.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/7979-scanner-migratione46b1ad97Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo fmt --check+cargo check --manifest-path app/src-tauri/Cargo.tomlValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Webview::send_dev_tools_messagechannel introduced in feat(cef): in-process CDP DevTools transport (foundation) #3498.127.0.0.1:19222/json/versionwill fail withECONNREFUSED.Parity Contract
url.starts_with(url_prefix) && url.ends_with(url_fragment)), Slack's pin-on-strict-match resolution hierarchy, Slack's 60s IDB serialise budget (now viacall_with_timeout), Discord's per-event dispatch shape (now via canonicalpump_events), Meet's per-call label routing (meet-call-<request_id>).conn_for_label/conn_for_accountretryinstall_for_label/install_for_accountonce on a registry miss so a transient install race at window creation doesn't permanently lock the surface out of CDP.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Release Notes