Conversation
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR makes four targeted browser fixes: adds --restore-last-session to preserve session cookies across a clean Chrome restart; makes the CDP stealth guard tolerate idle WebSocketTimeoutException in its main loop instead of dying after ~5s; replaces the manual per-target attach loop with Target.setAutoAttach/auto-attach events (eliminating duplicate-attach locale failures and unifying existing/new/iframe handling through one code path); and fixes a real readiness bug where mktemp pre-created the ready file so the supervisor reported readiness before the guard started — now mktemp -d + a guard-written ready marker gates startup correctly.\n\nThe changes are coherent and the test coverage is strong: new fixtures exercise idle, command-timeout, existing, and empty scenarios, plus a supervisor test for guard-exit-before-ready. I verified the auto-attach path preserves stealth-before-execution because waitForDebuggerOnStart pauses restored/new tabs until configure_target resumes them, so moving set_ready() ahead of per-target configuration does not leak. I have two non-blocking observations around failure handling, below. Overall this is a solid, well-scoped fix.
Verdict: COMMENT — The fixes are correct, focused, and well-tested; only two non-blocking robustness/stealth-integrity concerns are worth considering.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | error-handling | scripts/docker/rome-apply-cdp-stealth.test.ts |
Tab is resumed without stealth after a command-response timeout |
| P3 | error-handling | scripts/docker/rome-apply-cdp-stealth.sh |
Idle-timeout handling cannot detect a half-open (silently dead) connection |
| expect(result.stdout).not.toContain("stealth configured for page new"); | ||
| expect(result.commands).toContainEqual( | ||
| expect.objectContaining({ | ||
| method: "Runtime.runIfWaitingForDebugger", |
There was a problem hiding this comment.
[P2] error-handling — Tab is resumed without stealth after a command-response timeout
This test codifies that on a command-response timeout (Emulation.setLocaleOverride etc.), configure_target catches the exception, skips the remaining stealth steps (UA override, Page.addScriptToEvaluateOnNewDocument, etc.), and then the finally block still sends Runtime.runIfWaitingForDebugger — un-pausing a tab that never received the stealth JS or UA override. For a stealth component this is the worst outcome: the page proceeds to load and issue requests with the un-overridden automation fingerprint, which defeats the guard's purpose. Consider retrying the failed target once, or keeping it paused / closing it on injection failure rather than resuming it un-stealthed. At minimum, log this at error level (err, stderr) rather than log so the supervisor/operator can see a tab was released without protection.
| raw = ws.recv() | ||
| try: | ||
| raw = ws.recv() | ||
| except websocket.WebSocketTimeoutException: |
There was a problem hiding this comment.
[P3] error-handling — Idle-timeout handling cannot detect a half-open (silently dead) connection
Swallowing every WebSocketTimeoutException and continue-ing is correct for a quiet-but-alive browser. However, if the TCP connection goes half-open (browser gone but no FIN/close received), ws.recv() will keep timing out every 5s forever and the guard will never hit the browser CDP connection closed path, so the supervisor won't restart Chrome. Consider bounding this — e.g. after N consecutive idle timeouts, issue a cheap liveness command (Target.getTargets); if that command-response times out, treat it as a disconnect and fail(...).
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR adds Chrome session restoration, replaces duplicate manual CDP attachment with browser-level automatic attachment, tolerates idle WebSocket timeouts, and waits for an explicit guard readiness marker. I reviewed all changed files and their launcher/container call sites; the guard changes are directionally sound, but the restore flag conflicts with the unconditional startup URL, and the new lifecycle tests are not wired into CI.
Verdict: REQUEST_CHANGES — The session-restore launch path introduces unbounded blank-tab accumulation on routine browser restarts.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | reliability | scripts/docker/rome-start-chrome-cdp.sh |
Restored sessions accumulate a blank tab on every restart |
| P2 | testing | scripts/docker/rome-apply-cdp-stealth.test.ts |
New guard lifecycle tests are not executed by CI |
| --no-default-browser-check | ||
| --password-store=basic | ||
| # A persistent profile alone does not restore session cookies after a clean exit. | ||
| --restore-last-session |
There was a problem hiding this comment.
[P1] reliability — Restored sessions accumulate a blank tab on every restart
--restore-last-session is combined later with an unconditional positional $CHROME_URL (defaulting to about:blank). Each supervised restart restores the prior tabs and opens another blank tab, so the persistent profile accumulates an unbounded number of targets. Omit the startup URL on restored launches, or only pass it when the profile has no restorable session.
| }); | ||
| }); | ||
|
|
||
| describe("rome-apply-cdp-stealth.sh guard lifecycle", () => { |
There was a problem hiding this comment.
[P2] testing — New guard lifecycle tests are not executed by CI
No root test command runs Rstest over scripts/**/*.test.ts; test:unit:rest only enumerates workspace tests and selected .mjs Node tests. Consequently, pnpm test:unit and CI can pass without executing this suite or the new supervisor readiness test. Add a root Rstest command to test:unit:rest, or move these tests into an existing tested workspace.
yunfanye
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR correctly replaces duplicate manual CDP attachments with automatic attachment, distinguishes idle receive timeouts from command failures, and fixes the pre-created readiness marker. I read all five changed files and their launcher/container call sites, verified the test suites are collected by the core Rstest configuration, and exercised the four guard fixture scenarios plus shell syntax locally.
The session-restoration change nevertheless introduces two startup regressions. Restored pages can execute before the guard attaches, and the unconditional startup URL creates another persistent blank tab on every restart.
Verdict: REQUEST_CHANGES — The new session-restoration path can expose restored pages before stealth is installed and causes persistent blank-tab accumulation across supervised restarts.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | security | scripts/docker/rome-start-chrome-cdp.sh |
Restored pages can load before stealth is installed |
| P1 | reliability | scripts/docker/rome-start-chrome-cdp.sh |
Restarts accumulate persistent blank tabs |
| --no-default-browser-check | ||
| --password-store=basic | ||
| # A persistent profile alone does not restore session cookies after a clean exit. | ||
| --restore-last-session |
There was a problem hiding this comment.
[P1] security — Restored pages can load before stealth is installed
--restore-last-session lets Chrome create and navigate saved tabs during process startup, but start_stealth_guard runs only after CDP and its external proxy are ready. Existing targets are not paused (waitingForDebugger is false), and Page.addScriptToEvaluateOnNewDocument cannot retrofit the already-running document, so initial requests and site JavaScript can observe the unmodified browser identity. Avoid auto-navigating restored tabs unless the guard can attach before navigation, or preserve session cookies without restoring pages; add an integration test that checks overrides from restored-page startup code.
| --no-first-run | ||
| --no-default-browser-check | ||
| --password-store=basic | ||
| # A persistent profile alone does not restore session cookies after a clean exit. |
There was a problem hiding this comment.
[P1] reliability — Restarts accumulate persistent blank tabs
The restore flag is combined with the unconditional positional $CHROME_URL later in the launcher, whose default is about:blank. Chrome restores the previous tabs and opens that URL as an additional tab, so clean supervised restarts grow the persisted target set without bound; this reproduces as 1 → 2 → 3 pages over three clean Chrome restarts. Omit the startup URL when restoring, or make startup-URL and restore modes mutually exclusive so repeated restarts converge.
What this PR does
Closing Chrome discards session-only site logins even when its profile survives. The CDP stealth guard also exits after five idle seconds. Duplicate attachments can fail locale setup, and the supervisor can report readiness before the guard starts.
Restore the last browser session, tolerate idle event timeouts, configure browser-level targets through automatic attachment, and wait for the guard to write its readiness signal.
This draft contains only a cherry-pick of
de8f8755e(fix(browser): preserve sessions and keep the stealth guard running) onto currentmain. It provides an isolated comparison with #347.Design & Invariants
Test plan
de8f8755ebyte for byte.pnpm typecheckpnpm test:unitRome startedand a healthy local container.pnpm dev:all: the existing host port 80 conflict between Rome and Traefik blocks this check.Host checks used Node 24 because Nix is unavailable on the test host. Live results cover the existing signed-in profile and do not establish compatibility for every site or profile.
Not in this PR
Recursive iframe attachment from
592561210, excluded for the requested first-commit comparison. The known OOPIF coverage gap remains.Native browser identity preservation from
7a74829d2, excluded for the same comparison.Startup tab convergence: repeated restarts added blank tabs during the comparison. The launcher behavior is unchanged in this isolated commit.