diff --git a/docs/superpowers/plans/2026-04-14-helper-page-lifecycle-hidden-domain-plan.md b/docs/superpowers/plans/2026-04-14-helper-page-lifecycle-hidden-domain-plan.md new file mode 100644 index 0000000..776c12c --- /dev/null +++ b/docs/superpowers/plans/2026-04-14-helper-page-lifecycle-hidden-domain-plan.md @@ -0,0 +1,475 @@ +# Helper Page Lifecycle Fix & Hidden Domain Support — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix duplicate browser-helper.html pages caused by WebSocket reconnections, add cleanup on Drop, and introduce a config switch for hidden-domain page opening. + +**Architecture:** Three changes: (1) lift `cached_host` from `serve_client()` to the outer `run()` loop so reconnections share one host, (2) enhance `Drop for LiveBrowserCallbackHost` to send a close-page command via browser WS, (3) add `use_hidden_domain: bool` parameter that selects between `sgBrowerserOpenPage`/`sgHideBrowerserOpenPage` and their corresponding close APIs. + +**Tech Stack:** Rust, tungstenite WebSocket crate, SuperRPA browser WS protocol + +--- + +### Task 1: Add `use_hidden_domain` field and update `bootstrap_helper_page` + +**Files:** +- Modify: `src/browser/callback_host.rs:28` (constant), `:44-51` (struct), `:215-252` (constructor), `:340-359` (bootstrap fn) + +- [ ] **Step 1: Change `HELPER_BOOTSTRAP_ACTION` from constant to a function of `use_hidden_domain`** + +Replace the constant and update `bootstrap_helper_page` to accept and use the flag: + +```rust +// DELETE this line: +// const HELPER_BOOTSTRAP_ACTION: &str = "sgBrowerserOpenPage"; + +// REPLACE bootstrap_helper_page signature and body: +fn bootstrap_helper_page( + browser_ws_url: &str, + request_url: &str, + helper_url: &str, + use_hidden_domain: bool, +) -> Result<(), PipeError> { + let (mut websocket, _) = connect(browser_ws_url) + .map_err(|err| PipeError::Protocol(format!("browser websocket connect failed: {err}")))?; + configure_bootstrap_socket(&mut websocket)?; + websocket + .send(Message::Text( + r#"{"type":"register","role":"web"}"#.to_string().into(), + )) + .map_err(|err| PipeError::Protocol(format!("browser websocket register failed: {err}")))?; + let _ = recv_bootstrap_prelude(&mut websocket); + let open_action = if use_hidden_domain { + "sgHideBrowerserOpenPage" + } else { + "sgBrowerserOpenPage" + }; + let payload = json!([ + request_url, + open_action, + helper_url, + ]) + .to_string(); + websocket + .send(Message::Text(payload.into())) + .map_err(|err| PipeError::Protocol(format!("helper bootstrap send failed: {err}")))?; + Ok(()) +} +``` + +- [ ] **Step 2: Add new fields to `LiveBrowserCallbackHost`** + +```rust +#[derive(Debug)] +pub(crate) struct LiveBrowserCallbackHost { + host: Arc, + shutdown: Arc, + server_thread: Mutex>>, + command_lock: Mutex<()>, + result_timeout: Duration, + browser_ws_url: String, + use_hidden_domain: bool, +} +``` + +- [ ] **Step 3: Update `start_with_browser_ws_url` to accept and store the new parameter** + +```rust +impl LiveBrowserCallbackHost { + pub(crate) fn start_with_browser_ws_url( + browser_ws_url: &str, + bootstrap_request_url: &str, + ready_timeout: Duration, + result_timeout: Duration, + use_hidden_domain: bool, + ) -> Result { + let listener = TcpListener::bind("127.0.0.1:0").map_err(|err| { + PipeError::Protocol(format!("failed to bind callback host listener: {err}")) + })?; + listener.set_nonblocking(true).map_err(|err| { + PipeError::Protocol(format!("failed to configure callback host listener: {err}")) + })?; + let origin = format!( + "http://{}", + listener.local_addr().map_err(|err| { + PipeError::Protocol(format!( + "failed to resolve callback host listener address: {err}" + )) + })? + ); + let host = Arc::new(BrowserCallbackHost::with_urls(&origin, browser_ws_url)); + let shutdown = Arc::new(AtomicBool::new(false)); + let thread_host = host.clone(); + let thread_shutdown = shutdown.clone(); + let server_thread = thread::spawn(move || serve_loop(listener, thread_host, thread_shutdown)); + + bootstrap_helper_page(browser_ws_url, bootstrap_request_url, host.helper_url(), use_hidden_domain)?; + wait_for_helper_ready(host.as_ref(), ready_timeout)?; + + let live_host = Self { + host, + shutdown, + server_thread: Mutex::new(Some(server_thread)), + command_lock: Mutex::new(()), + result_timeout, + browser_ws_url: browser_ws_url.to_string(), + use_hidden_domain, + }; + Ok(live_host) + } +``` + +- [ ] **Step 4: Fix the inline test struct literal that constructs `LiveBrowserCallbackHost` directly** + +In the `live_callback_host_treats_simulated_mouse_command_as_fire_and_forget` test (around line 1110), add the new fields: + +```rust + let host = LiveBrowserCallbackHost { + host: Arc::new(BrowserCallbackHost::new()), + shutdown: Arc::new(AtomicBool::new(false)), + server_thread: Mutex::new(None), + command_lock: Mutex::new(()), + result_timeout: Duration::from_millis(10), + browser_ws_url: "ws://127.0.0.1:12345".to_string(), + use_hidden_domain: false, + }; +``` + +- [ ] **Step 5: Run build to verify compilation** + +Run: `cargo build 2>&1` +Expected: 0 errors. The `HELPER_BOOTSTRAP_ACTION` constant removal and signature changes should all be internally consistent. + +- [ ] **Step 6: Run tests to verify existing behavior is preserved** + +Run: `cargo test -- callback_host 2>&1` +Expected: All existing callback_host tests pass (including `live_callback_host_sends_bootstrap_open_page_command` which still checks for `sgBrowerserOpenPage` since no caller passes `true` yet). + +- [ ] **Step 7: Commit** + +```bash +git add src/browser/callback_host.rs +git commit -m "feat(callback_host): add use_hidden_domain param to bootstrap_helper_page" +``` + +--- + +### Task 2: Enhance `Drop` to close the helper page + +**Files:** +- Modify: `src/browser/callback_host.rs:321-328` (Drop impl) + +- [ ] **Step 1: Add `close_helper_page` helper function** + +Add this function near `bootstrap_helper_page` (after line ~360): + +```rust +/// Best-effort attempt to close the helper page tab via browser WebSocket. +/// Silently ignores all errors — this runs during Drop and must not panic. +fn close_helper_page(browser_ws_url: &str, helper_url: &str, use_hidden_domain: bool) { + let close_action = if use_hidden_domain { + "sgHideBrowerserClosePage" + } else { + "sgBrowserClosePage" + }; + + let result: Result<(), Box> = (|| { + // Use a raw TcpStream with timeouts instead of tungstenite::connect + // which does not expose a connection timeout. + let addr = browser_ws_url + .trim_start_matches("ws://") + .trim_start_matches("wss://"); + let stream = TcpStream::connect_timeout( + &addr.parse().map_err(|e| format!("addr parse: {e}"))?, + Duration::from_millis(100), + )?; + stream.set_read_timeout(Some(Duration::from_millis(200)))?; + stream.set_write_timeout(Some(Duration::from_millis(200)))?; + let (mut websocket, _) = tungstenite::client( + browser_ws_url, + stream, + )?; + websocket.send(Message::Text( + r#"{"type":"register","role":"web"}"#.to_string().into(), + ))?; + // Drain the welcome prelude (best-effort, ignore timeout). + let _ = websocket.read(); + let payload = json!([helper_url, close_action, helper_url]).to_string(); + websocket.send(Message::Text(payload.into()))?; + Ok(()) + })(); + + if let Err(err) = result { + eprintln!("close_helper_page best-effort failed (harmless): {err}"); + } +} +``` + +- [ ] **Step 2: Update `Drop for LiveBrowserCallbackHost` to call `close_helper_page`** + +```rust +impl Drop for LiveBrowserCallbackHost { + fn drop(&mut self) { + // Best-effort: tell the browser to close the helper page tab. + close_helper_page( + &self.browser_ws_url, + self.host.helper_url(), + self.use_hidden_domain, + ); + + self.shutdown.store(true, Ordering::Relaxed); + if let Some(handle) = self.server_thread.lock().unwrap().take() { + let _ = handle.join(); + } + } +} +``` + +- [ ] **Step 3: Run build to verify compilation** + +Run: `cargo build 2>&1` +Expected: 0 errors. `close_helper_page` uses types already imported (`TcpStream`, `Duration`, `json!`, `Message`). + +- [ ] **Step 4: Run tests** + +Run: `cargo test -- callback_host 2>&1` +Expected: All pass. The Drop enhancement is best-effort and the test helper constructs hosts with `server_thread: Mutex::new(None)`, so Drop completes cleanly. + +- [ ] **Step 5: Commit** + +```bash +git add src/browser/callback_host.rs +git commit -m "feat(callback_host): close helper page on Drop via browser WS" +``` + +--- + +### Task 3: Lift `cached_host` to outer loop and update `serve_client` signature + +**Files:** +- Modify: `src/service/mod.rs:72-96` (run loop) +- Modify: `src/service/server.rs:232-241` (serve_client signature and cached_host init) + +- [ ] **Step 1: Change `serve_client` to accept `cached_host` as a parameter** + +In `src/service/server.rs`, change the function signature and remove the local `cached_host` variable: + +```rust +pub fn serve_client( + context: &AgentRuntimeContext, + session: &ServiceSession, + sink: Arc, + browser_ws_url: &str, + mac_policy: &MacPolicy, + cached_host: &mut Option>, +) -> Result<(), PipeError> { + // DELETE the line: let mut cached_host: Option> = None; + + loop { + // ... rest of function body unchanged, `cached_host` is now the parameter +``` + +The body references to `cached_host` remain identical — they just operate on the borrowed mutable reference instead of a local variable. + +- [ ] **Step 2: Update `start_with_browser_ws_url` call to pass `false` for `use_hidden_domain`** + +In `src/service/server.rs`, at the `LiveBrowserCallbackHost::start_with_browser_ws_url` call (around line 288), add the `false` argument: + +```rust + match LiveBrowserCallbackHost::start_with_browser_ws_url( + browser_ws_url, + &bootstrap_url, + Duration::from_secs(15), + BROWSER_RESPONSE_TIMEOUT, + false, // use_hidden_domain: visible tab for now + ) { +``` + +- [ ] **Step 3: Lift `cached_host` into `run()` in `mod.rs`** + +In `src/service/mod.rs`, declare `cached_host` before the loop and pass it to `serve_client`: + +```rust + // Add this import at the top of the function or file: + use crate::browser::callback_host::LiveBrowserCallbackHost; + + // Before the loop (after line 64, after `let session = ...`): + let mut cached_host: Option> = None; + + loop { + let (stream, _) = listener.accept()?; + let websocket = accept(stream) + .map_err(|err| PipeError::Protocol(format!("service websocket accept failed: {err}")))?; + let sink = Arc::new(ServiceEventSink::from_websocket(websocket)); + match session.try_attach_client() { + Ok(()) => { + let result = serve_client( + &runtime_context, + &session, + sink.clone(), + browser_ws_url, + &mac_policy, + &mut cached_host, + ); + session.detach_client(); + match result { + Ok(()) | Err(PipeError::PipeClosed) => {} + Err(err) => return Err(err), + } + } + Err(message) => { + sink.send_service_message(message)?; + } + } + } +``` + +- [ ] **Step 4: Update the `pub use` export if needed** + +Check `src/service/mod.rs:17`: +```rust +pub use server::{serve_client, ServiceEventSink, ServiceSession}; +``` +The signature change is compatible — `serve_client` is still public with an added parameter. Any external callers will get a compile error guiding them to add the parameter, which is the desired behavior. + +- [ ] **Step 5: Run build to verify compilation** + +Run: `cargo build 2>&1` +Expected: 0 errors. If there are external test files calling `serve_client`, they will fail here and need the new parameter added. + +- [ ] **Step 6: Run full test suite** + +Run: `cargo test 2>&1` +Expected: All tests pass. External test files that call `serve_client` indirectly through the service protocol tests should still work because they use the WS protocol layer, not `serve_client` directly. (Verified: grep found 0 test files referencing `serve_client` or `LiveBrowserCallbackHost`.) + +- [ ] **Step 7: Commit** + +```bash +git add src/service/mod.rs src/service/server.rs +git commit -m "fix(service): lift cached_host to outer loop to prevent duplicate helper pages" +``` + +--- + +### Task 4: Add tests for hidden domain bootstrap + +**Files:** +- Modify: `src/browser/callback_host.rs` (inline tests module, around line 1071) + +- [ ] **Step 1: Update existing `live_callback_host_sends_bootstrap_open_page_command` test** + +The test currently calls `start_with_browser_ws_url` with 4 args. Add the 5th arg `false`: + +```rust + #[test] + fn live_callback_host_sends_bootstrap_open_page_command() { + let (ws_url, frames, handle) = start_fake_browser_status_server(); + + let result = LiveBrowserCallbackHost::start_with_browser_ws_url( + &ws_url, + "https://www.zhihu.com", + Duration::from_millis(100), + Duration::from_millis(50), + false, + ); + assert!(result.is_err(), "expected timeout because no real helper page loads"); + drop(result); + handle.join().unwrap(); + + let sent = frames.lock().unwrap().clone(); + assert!( + sent.iter().any(|frame| frame.contains("sgBrowerserOpenPage")), + "bootstrap should send sgBrowerserOpenPage to the browser WS; sent frames: {sent:?}" + ); + assert!( + sent.iter().any(|frame| frame.contains("/sgclaw/browser-helper.html")), + "bootstrap should include the helper page URL; sent frames: {sent:?}" + ); + assert!( + sent.iter().any(|frame| frame.contains("https://www.zhihu.com")), + "bootstrap requestUrl should be the provided page URL; sent frames: {sent:?}" + ); + } +``` + +- [ ] **Step 2: Add new test for hidden domain bootstrap** + +Add this test after the existing one: + +```rust + #[test] + fn live_callback_host_hidden_domain_sends_hide_open_page_command() { + let (ws_url, frames, handle) = start_fake_browser_status_server(); + + let result = LiveBrowserCallbackHost::start_with_browser_ws_url( + &ws_url, + "https://www.zhihu.com", + Duration::from_millis(100), + Duration::from_millis(50), + true, + ); + assert!(result.is_err(), "expected timeout because no real helper page loads"); + drop(result); + handle.join().unwrap(); + + let sent = frames.lock().unwrap().clone(); + assert!( + sent.iter().any(|frame| frame.contains("sgHideBrowerserOpenPage")), + "hidden domain bootstrap should send sgHideBrowerserOpenPage; sent frames: {sent:?}" + ); + assert!( + !sent.iter().any(|frame| { + frame.contains("\"sgBrowerserOpenPage\"") + }), + "hidden domain bootstrap should NOT send visible sgBrowerserOpenPage; sent frames: {sent:?}" + ); + assert!( + sent.iter().any(|frame| frame.contains("/sgclaw/browser-helper.html")), + "bootstrap should include the helper page URL; sent frames: {sent:?}" + ); + } +``` + +- [ ] **Step 3: Run all callback_host tests** + +Run: `cargo test -- callback_host 2>&1` +Expected: All 3 tests pass: +- `live_callback_host_sends_bootstrap_open_page_command` — regression, visible domain +- `live_callback_host_hidden_domain_sends_hide_open_page_command` — new, hidden domain +- `live_callback_host_treats_simulated_mouse_command_as_fire_and_forget` — unchanged + +- [ ] **Step 4: Run full test suite** + +Run: `cargo test 2>&1` +Expected: All tests pass. + +- [ ] **Step 5: Commit** + +```bash +git add src/browser/callback_host.rs +git commit -m "test(callback_host): add hidden domain bootstrap test" +``` + +--- + +### Task 5: Full build verification + +**Files:** None (verification only) + +- [ ] **Step 1: Clean build** + +Run: `cargo build 2>&1` +Expected: 0 errors. Warnings about dead code in unrelated modules are acceptable. + +- [ ] **Step 2: Full test suite** + +Run: `cargo test 2>&1` +Expected: All tests pass. The pre-existing `lineloss_period_resolver_prompts_for_missing_period` failure (from previous work) is known and unrelated. + +- [ ] **Step 3: Verify the key behavioral changes in code** + +Manually confirm: +1. `src/service/mod.rs` — `cached_host` is declared BEFORE the `loop`, not inside `serve_client` +2. `src/browser/callback_host.rs` — `Drop::drop` calls `close_helper_page` before shutdown +3. `src/browser/callback_host.rs` — `bootstrap_helper_page` uses `"sgHideBrowerserOpenPage"` when `use_hidden_domain == true` and `"sgBrowerserOpenPage"` when `false` +4. `src/service/server.rs` — `start_with_browser_ws_url` call passes `false` as `use_hidden_domain`