[suchintan]

https://github.com/Skyvern-AI/rustwright-cloud/pull/238

[claude]

Summary

Large sync PR (49 files, ~8.7k additions) bundling several real behavior changes: removal of disable_playwright_compat()/RUSTWRIGHT_UNSAFE_DOM_FASTPATH, per-page ephemeral browser contexts, a unified MCP response profile, and a custom→regex-crate swap for network filters. Most of it is clean (GIL handling, no new unsafe, no injection/shell issues, DOM-fastpath removal is thorough, drag-and-drop fix is well tested), but there's one confirmed correctness bug in the new Rust-side console/page-error event replay path, plus some test-coverage and doc-consistency gaps worth closing before/after merge.

🔴 Critical Issues (1)

• src/lib.rs:41832-41853 — console/page-error event cursor never advances, causing duplicate event replay. PyConsoleEventWaiter::wait_internal captures event_cursor (a Copy u64) by value into the py.detach closure and calls wait_for_console_event(...), which returns (RwResult<String>, u64) — the second element is the advanced cursor. But the closure only reads result.0 at the end (line 41853) and rebuilds its return tuple from the original pre-call event_cursor binding (line 41848), not from result.1. Line 41851 then writes that stale, never-advanced cursor back to self.event_cursor. Contrast with the sibling PyWorkerEventWaiter/PyWorkerCloseEventWaiter (~line 42045+), which correctly destructure let (result, event_cursor) = browser.block_on_raw(...) — confirming this is a one-off slip, not intentional.
  • Failure scenario: any repeated page.console_event_waiter().wait()/wait_matching() call, especially once the broadcast channel lags (RecvError::Lagged) and replay falls back to event_log.entries_since(event_cursor.max(oldest_seq)) — since event_cursor is stuck at its initial subscribe-time value, previously-returned console/page-error events get replayed and handed back to Python again on subsequent waits. This silently produces wrong/duplicate results rather than crashing, so it's easy to miss in casual testing.

🟡 Suggestions (4)

• mcp/src/config.rs — the legacy per-client env toggles (RUSTWRIGHT_MCP_BUDGET, _DISTILL, _HEADER, _CONSOLE_DEDUP, _NET_NOTE, _LEAN_DESCRIPTIONS) are now silently ignored with the unified profile, while invalid MAX_RESPONSE_BYTES/MAX_RESPONSE_LINES values still warn. Anyone with these pinned in an env/config for compatibility gets silently switched to new defaults with no signal. Consider a deprecation warning on read, at least for one release.
• mcp/tests/ — no e2e test actually triggers truncation at the real 9 KiB/200-line defaults; existing tests override to 4096/16, so a boundary bug at the literal shipped default (e.g. a char-boundary panic when slicing near the 9 KiB cutoff) wouldn't be caught. Also, tools-list-legacy.json, tools-list-lean.json, and codex-mcp-initialize-0.146.0.json under mcp/tests/fixtures/ are now orphaned (no remaining references) — worth deleting alongside the profile consolidation.
• tests/test_playwright_compat_optin.py — coverage only proves old disable_playwright_compat() call sites were deleted; nothing asserts the new "one-way" property directly (e.g. hasattr(rustwright, "disable_playwright_compat") is False, or that re-enabling compat after use has no off switch). Worth a direct assertion given this is called out as a Breaking change.
• CHANGELOG.md — "Network request filters now accept the standard Rust regex syntax" is listed under Changed, but the underlying implementation swapped a hand-rolled ~200-line regex-subset parser for the regex crate. That can silently change which existing user-supplied filter patterns compile or match differently than before — seems more like a Breaking entry than Changed.

📝 Minor / Style (2)

• docs/PARITY.md vs README.md — PARITY.md states "clean on about 2 of 4 targets" while README states "3 of 4 public fingerprint targets clean." Not introduced by this PR, but given the review mandate to keep the fingerprint-honesty framing precise and consistent, worth reconciling in a follow-up.
• python/rustwright/async_api.py — this PR bundles a fair amount of new concurrency machinery (invoke_on_loop/invoke_in_current_loop, several new locks: _dialog_state_lock, _worker_state_lock, _worker_console_forward_lock, _worker_interest_lock, _page_cdp_event_condition) that isn't called out in the CHANGELOG bullets and wasn't in scope for a deep race/deadlock trace here — flagging for a dedicated concurrency review pass. Separately, _emit_event(..., isolate_exceptions=True) swallows all but the first handler's exception when isolating; worth double-checking this matches Playwright's actual "one bad listener doesn't break others" semantics rather than masking real errors from listeners 2..N.