# 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<BrowserCallbackHost>,
    shutdown: Arc<AtomicBool>,
    server_thread: Mutex<Option<JoinHandle<()>>>,
    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<Self, PipeError> {
        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<dyn std::error::Error>> = (|| {
        // 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<ServiceEventSink>,
    browser_ws_url: &str,
    mac_policy: &MacPolicy,
    cached_host: &mut Option<Arc<LiveBrowserCallbackHost>>,
) -> Result<(), PipeError> {
    // DELETE the line: let mut cached_host: Option<Arc<LiveBrowserCallbackHost>> = 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<Arc<LiveBrowserCallbackHost>> = 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`