fix(daemon): serialise the first attach so Chrome gets one consent prompt

docs/rfc/0013-consent-prompt-lifecycle.md

@@ -0,0 +1,158 @@
+# RFC-0013: Surviving Chrome's consent prompt
+
+- **Status:** Draft
+- **Priority:** P0
+- **Area:** connection
+- **Depends on:** the spawn serialisation in #17 (necessary, not sufficient)
+
+## Summary
+
+Stop `chrome-cdp` from wedging the user's browser on the one-time "Allow remote debugging?" prompt.
+Three changes: wait for the consent it asks for instead of abandoning it, detect the pending state instead of reporting a generic failure, and prefer the connection path that never prompts.
+
+## What happened
+
+A user's Chrome froze with the consent dialog on screen and no button responding.
+It was reproduced deliberately afterwards, and the reproduction contradicted the first diagnosis.
+
+The initial theory was **stacked prompts**: several `chrome-cdp` processes had started at once, each found no daemon, each spawned one, and each spawned daemon attached to Chrome and raised its own prompt.
+That much is real, and #17 fixes it — eight concurrent callers produced eight daemons before the fix and one after.
+
+But the controlled reproduction showed a **single** prompt wedges Chrome just as thoroughly.
+Serialising the spawn is necessary and not sufficient.
+
+### What the reproduction established
+
+Measured against a real Chrome made debug-enabled through the `chrome://inspect/#remote-debugging` toggle:
+
+| Probe | While consent is pending |
+|-------|--------------------------|
+| TCP connect to `127.0.0.1:9222` | succeeds immediately |
+| `GET /json/version` | 404 in under a millisecond |
+| WebSocket upgrade to the browser endpoint | **hangs — never completes, never refuses** |
+
+The hang is the mechanism.
+Chrome does not reject the connection while it waits for the user; it holds the upgrade open and says nothing.
+So the client has no error to classify — only silence — which is why the failure surfaces as an undifferentiated timeout.
+
+Three consequences followed, each independently a defect:
+
+1. **The daemon abandons the prompt it raised.**
+   `chrome.Connect` dials, the upgrade hangs, the dial times out in about ten seconds, and the daemon writes its error and exits.
+   The modal is left on screen with nothing behind it.
+   Clicking Allow then grants consent to a connection that no longer exists.
+2. **Ten seconds is not a human timescale for a dialog that may be invisible.**
+   The prompt is browser-modal and can sit behind the window.
+   A user who has not clicked it has usually not *seen* it, and by the time they do, the process that asked is gone.
+3. **`doctor` reports readiness it never verified.**
+   It reads the `DevToolsActivePort` file and reports "debug endpoint reachable — Path B attach ready", handing back a `ws://` URL.
+   It does not probe.
+   The one command whose job is to answer "can I connect?" answered yes while every connection was hanging.
+
+A fourth observation is recorded because it cost time: **`/json/version` returning 404 is not a consent signal.**
+It returns 404 in this connection mode whether or not consent has been granted — the toggle path exposes the WebSocket without the HTTP JSON API.
+An early reading of that 404 as "consent pending" was wrong, and any detection built on it would be too.
+
+## User stories
+
+**US-1 — Do not freeze my browser.**
+As a user, I want a tool that asks for consent to still be there when I answer, so that a prompt I did not see immediately does not leave my browser unusable.
+*Acceptance:* a prompt answered minutes after it appears results in a working connection, not an orphaned dialog.
+
+**US-2 — Tell me what is happening.**
+As a user staring at an unresponsive browser, I want to be told a consent prompt is pending and where to find it, so that I know this is a dialog and not a crash.
+*Acceptance:* while the upgrade is hanging, the CLI reports a distinct pending state naming the prompt, not a generic connection failure.
+
+**US-3 — Do not ask at all when you do not have to.**
+As a user, I want to be steered to the launch flag that skips consent entirely, so that routine use never involves a modal.
+*Acceptance:* when no usable endpoint exists, the CLI recommends `--remote-debugging-port` before it recommends the toggle that prompts.
+
+**US-4 — Recover without losing my tabs.**
+As a user whose browser is already wedged, I want to be told the actual remedy, so that I am not left force-quitting on a guess.
+*Acceptance:* the failure message names the recovery and does not imply the browser has crashed.
+
+**US-5 — One prompt, not many.**
+As a user running several commands at once, I want at most one consent request.
+*Acceptance:* covered by #17; this RFC keeps it true.
+
+## Proposed changes
+
+### 1. Wait for the consent
+
+The daemon's initial connect must outlive the dialog.
+Replace the single short dial with a bounded wait — proposal: `consent_timeout`, default **120s** — during which the daemon stays alive and keeps the pending upgrade open.
+
+Distinguishing the pending state from a dead endpoint is what makes a long wait safe.
+A refused TCP connection is a real failure and must stay fast; a hanging *upgrade* against an *open* port is the consent signature and is the only case that earns the long wait.
+
+### 2. Report the pending state
+
+Add `CodeConsentPending = "consent_pending"` mapped to the existing connection exit code, so a caller can branch on it without a new number.
+
+While waiting, the CLI says a consent prompt is pending, that it is browser-modal and may be behind the window, and that Chrome will accept no other input until it is answered.
+That last clause is the part a user cannot deduce, and is why a frozen browser reads as a crash.
+
+### 3. Make `doctor` probe
+
+`doctor` must attempt the upgrade rather than trusting the port file, and report one of: no endpoint, consent pending, or ready.
+A diagnostic that reports readiness without testing it is worse than no diagnostic, because it sends the user looking somewhere else.
+
+Probing is itself a connection request, so `doctor` must reuse a live daemon when one exists rather than raising a prompt of its own.
+
+### 4. Prefer the path that never prompts
+
+When no usable endpoint exists, recommend relaunching Chrome with `--remote-debugging-port=9222` **first**, and the `chrome://inspect` toggle second with a note that it prompts on every fresh attach.
+The toggle is currently presented as the primary route, which routes every new user through the failure this RFC exists to remove.
+
+## Verification scenarios
+
+**VS-1 — A hanging upgrade is classified as consent pending, not as a timeout.**
+Given a listener that accepts TCP and never completes the WebSocket upgrade, when the daemon connects, then it reports `consent_pending` and stays alive rather than exiting.
+
+**VS-2 — A refused endpoint still fails fast.**
+Given a closed port, when the daemon connects, then it fails within a second or two with `connection_failed`, not after the consent timeout.
+
+**VS-3 — Consent answered late still works.**
+Given a listener that completes the upgrade after 30 seconds, when the daemon connects, then the connection succeeds and no prompt is orphaned.
+
+**VS-4 — The consent wait is bounded.**
+Given a listener that never completes the upgrade, when `consent_timeout` elapses, then the daemon exits with a message naming the prompt and the recovery.
+
+**VS-5 — `doctor` distinguishes all three states.**
+Table over: no endpoint, open-but-hanging, and ready — each reported distinctly, and the ready case verified by a completed upgrade rather than by the port file alone.
+
+**VS-6 — `doctor` does not raise its own prompt.**
+Given a running daemon, when `doctor` runs, then it answers through the daemon and initiates no new connection.
+
+**VS-7 — Concurrency stays at one prompt.**
+The guard from #17, restated here so this RFC's changes cannot regress it.
+
+## Test plan
+
+The pending state is a **local listener that accepts and stalls**, so almost all of this is testable with `net.Listen` and no browser at all — which matters, because the manual reproduction wedged a real browser twice and must not be the regression test.
+
+- **Pure/stub (`-short`):** VS-1 through VS-5 against hand-built listeners — refusing, stalling, and completing-after-a-delay.
+  This is where the classification logic belongs.
+- **Daemon:** VS-6 and VS-7 against the existing socket harness.
+- **Live Chrome:** none.
+  Consent cannot be granted programmatically, and a test that needs a human click is not a test.
+
+Note for anyone extending this: a long `t.TempDir()` path breaks a Unix socket bind on darwin — `sun_path` caps near 104 bytes and the directory embeds the test name, which fails with a bare `bind: invalid argument`.
+
+## Out of scope
+
+- Granting or suppressing consent programmatically.
+  It is a deliberate user decision and the tool should not try to route around it.
+- Anything about Chrome's own modal behaviour, which is not ours to change.
+- Windows: the project ships linux and darwin only.
+
+## Open questions
+
+1. Should the CLI **refuse to raise a prompt at all** unless the user opts in — erroring with "relaunch with `--remote-debugging-port`" instead?
+   Safest, but it removes the zero-config path that makes the tool pleasant on first use.
+   **Recommendation:** keep the prompt, fix the waiting, and lead with the flag in the docs.
+2. Is 120s the right consent timeout?
+   Long enough for a hidden dialog, short enough that a genuinely dead endpoint is not mistaken for a slow human.
+   **Recommendation:** 120s, as a config key so it can be argued with.
+3. Should `doctor` be able to probe *without* a daemon, accepting that it may raise a prompt?
+   **Recommendation:** yes, but say so before doing it, since the user ran a diagnostic and did not ask to connect.

docs/rfc/README.md

@@ -40,6 +40,7 @@ The ordering below follows that: make the interaction surface complete, then mak
 | [0010](0010-page-reading-ergonomics.md) | Page-reading ergonomics: `text --article`, `eval --await` | P2 | reading | Draft |
 | [0011](0011-session-recording.md) | Session recording: `record` and GIF export | P2 | capture | Draft |
 | [0012](0012-domain-allowlist.md) | Domain allow-list: bounding what the CLI may drive | P2 | safety | Draft |
+| [0013](0013-consent-prompt-lifecycle.md) | Surviving Chrome's consent prompt | P0 | connection | Draft |
 
 ## Dependency graph
 

internal/daemon/lifecycle.go

@@ -96,16 +96,38 @@ func Ensure(sockPath, exePath string, env []string) (*Client, error) {
 	if c := TryConnect(sockPath); c != nil {
 		return c, nil
 	}
+
+	// From here on, exactly one process at a time. Concurrent invocations that
+	// all find no daemon would otherwise each spawn one, and each spawned daemon
+	// attaches to Chrome — which raises a SEPARATE browser-modal "Allow remote
+	// debugging?" prompt. Several stacked prompts is not a slower version of
+	// one: the visible dialog need not be the one holding input, so the whole
+	// browser looks frozen with no button that responds. The daemon exists so
+	// that prompt happens once per session; nothing was making the FIRST attach
+	// single-file.
+	//
+	// The unlinks below are the other half. Outside the lock they can delete a
+	// socket a sibling daemon has just bound, orphaning a live daemon that no
+	// client can ever reach.
+	unlock, err := lockSpawn(sockPath)
+	if err != nil {
+		return nil, err
+	}
+	defer unlock()
+
+	// Re-check under the lock: while we waited, the holder may have started the
+	// daemon we were about to duplicate. This is what makes N callers converge
+	// on one daemon and one prompt.
+	if c := TryConnect(sockPath); c != nil {
+		return c, nil
+	}
+
 	_ = os.Remove(sockPath)          // clear a stale socket file
 	_ = os.Remove(sockPath + ".err") // and a stale error, so we only read THIS spawn's
 
-	cmd := exec.Command(exePath, "__daemon", sockPath)
-	cmd.Env = env
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} // detach into its own session
-	if err := cmd.Start(); err != nil {
-		return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot start daemon: " + err.Error()}
+	if err := spawnDaemon(exePath, sockPath, env); err != nil {
+		return nil, err
 	}
-	_ = cmd.Process.Release()
 
 	for range 100 { // up to ~10s for the first Allow-dialog click
 		time.Sleep(100 * time.Millisecond)
@@ -118,7 +140,43 @@ func Ensure(sockPath, exePath string, env []string) (*Client, error) {
 			return nil, decodeConnectErr(data)
 		}
 	}
-	return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "daemon did not start within 10s — did you click Allow in Chrome?"}
+	return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "daemon did not start within 10s — Chrome may be waiting on its \"Allow remote debugging?\" prompt; it can hide behind the window, and until it is answered Chrome accepts no other input"}
+}
+
+// spawnDaemon starts the detached daemon process. It is a variable so a test can
+// substitute a spawn it can count, without a real Chrome or a real binary.
+var spawnDaemon = func(exePath, sockPath string, env []string) error {
+	cmd := exec.Command(exePath, "__daemon", sockPath)
+	cmd.Env = env
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} // detach into its own session
+	if err := cmd.Start(); err != nil {
+		return &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot start daemon: " + err.Error()}
+	}
+	_ = cmd.Process.Release()
+	return nil
+}
+
+// lockSpawn takes an exclusive advisory lock covering the spawn-and-wait for one
+// socket path, and returns the release. The lock file is never removed: unlinking
+// it would let a later caller lock a different inode and defeat the exclusion.
+//
+// The wait is deliberately unbounded. The holder may be waiting out a consent
+// prompt the user has not clicked yet, and blocking behind it is the correct
+// outcome — spawning our own would add another prompt to the pile, which is the
+// failure this exists to prevent.
+func lockSpawn(sockPath string) (func(), error) {
+	f, err := os.OpenFile(sockPath+".lock", os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot open the daemon spawn lock: " + err.Error()}
+	}
+	if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil {
+		_ = f.Close()
+		return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot take the daemon spawn lock: " + err.Error()}
+	}
+	return func() {
+		_ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN)
+		_ = f.Close()
+	}, nil
 }
 
 // RunDaemon connects Chrome and serves sockPath until idle or stopped. Used by

internal/daemon/spawn_test.go

@@ -0,0 +1,132 @@
+package daemon
+
+import (
+	"net"
+	"os"
+	"path/filepath"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+)
+
+// TestEnsureSpawnsOneDaemonUnderConcurrency is the guard for a failure that took
+// down a user's whole browser.
+//
+// Ensure used to check for a running daemon, find none, and spawn one — with no
+// exclusion. Several chrome-cdp processes starting at once therefore each found
+// nothing and each spawned a daemon, and every spawned daemon attaches to Chrome,
+// raising its own browser-modal "Allow remote debugging?" prompt. Stacked prompts
+// are not a slower version of one prompt: the visible dialog need not be the one
+// holding input, so Chrome looks frozen with no button that responds.
+//
+// The unlink was the other half of it. Outside a lock, a late caller's
+// os.Remove(sockPath) can delete a socket a sibling daemon has just bound,
+// orphaning a live daemon nothing can reach.
+func TestEnsureSpawnsOneDaemonUnderConcurrency(t *testing.T) {
+	sock := filepath.Join(shortTempDir(t), "d.sock")
+
+	var spawns atomic.Int32
+	restore := swapSpawn(func(_, sockPath string, _ []string) error {
+		spawns.Add(1)
+		// Behave like the real daemon: bind the socket, a moment later, so the
+		// window between spawning and being connectable is real rather than
+		// instantaneous. Every caller must still converge on this one listener.
+		go func() {
+			time.Sleep(150 * time.Millisecond)
+			ln, err := net.Listen("unix", sockPath)
+			if err != nil {
+				t.Errorf("fake daemon could not bind %s: %v", sockPath, err)
+				return
+			}
+			t.Cleanup(func() { _ = ln.Close() })
+			for {
+				c, err := ln.Accept()
+				if err != nil {
+					return
+				}
+				_ = c.Close()
+			}
+		}()
+		return nil
+	})
+	defer restore()
+
+	const callers = 8
+	var wg sync.WaitGroup
+	errs := make([]error, callers)
+	clients := make([]*Client, callers)
+	for i := range callers {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			clients[i], errs[i] = Ensure(sock, "unused", nil)
+		}()
+	}
+	wg.Wait()
+
+	for i, err := range errs {
+		if err != nil {
+			t.Errorf("caller %d: Ensure failed: %v", i, err)
+		} else if clients[i] == nil {
+			t.Errorf("caller %d: got a nil client and no error", i)
+		}
+	}
+	if got := spawns.Load(); got != 1 {
+		t.Fatalf("%d daemons were spawned for %d concurrent callers, want exactly 1 — "+
+			"each spawn attaches to Chrome and raises its own consent prompt", got, callers)
+	}
+}
+
+// TestEnsureReusesARunningDaemon pins the fast path: an already-listening socket
+// is connected to without taking the lock or spawning anything.
+func TestEnsureReusesARunningDaemon(t *testing.T) {
+	sock := filepath.Join(shortTempDir(t), "d.sock")
+	ln, err := net.Listen("unix", sock)
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	defer ln.Close()
+	go func() {
+		for {
+			c, err := ln.Accept()
+			if err != nil {
+				return
+			}
+			_ = c.Close()
+		}
+	}()
+
+	var spawns atomic.Int32
+	restore := swapSpawn(func(string, string, []string) error {
+		spawns.Add(1)
+		return nil
+	})
+	defer restore()
+
+	if _, err := Ensure(sock, "unused", nil); err != nil {
+		t.Fatalf("Ensure against a live daemon: %v", err)
+	}
+	if got := spawns.Load(); got != 0 {
+		t.Errorf("spawned %d daemons while one was already running, want 0", got)
+	}
+}
+
+// shortTempDir returns a temp dir with a SHORT path. A Unix socket address is
+// capped near 104 bytes on darwin, and t.TempDir() embeds the test's name — long
+// enough here to fail the bind with a bare "invalid argument".
+func shortTempDir(t *testing.T) string {
+	t.Helper()
+	dir, err := os.MkdirTemp("", "cdpd")
+	if err != nil {
+		t.Fatalf("MkdirTemp: %v", err)
+	}
+	t.Cleanup(func() { _ = os.RemoveAll(dir) })
+	return dir
+}
+
+func swapSpawn(fn func(exePath, sockPath string, env []string) error) func() {
+	prev := spawnDaemon
+	spawnDaemon = fn
+	return func() { spawnDaemon = prev }
+}

skills/drive-chrome-cdp/SKILL.md

@@ -23,6 +23,8 @@ Because it drives the real profile, live logins are reused: **type no credential
    `chrome-cdp daemon status --json` shows it; `--no-daemon` bypasses it.
 3. **Avoid re-triggering the consent prompt.**
    A fresh attach (the first command after `daemon stop`, or after a Chrome restart) re-shows Chrome's "Allow remote debugging?" prompt; if it isn't clicked it can wedge Chrome.
+   The prompt is **browser-modal**, so an unanswered one freezes the whole browser, not just the tab — and it can hide behind the window.
+   Answer it before assuming Chrome has crashed.
    Keep the daemon alive — don't `daemon stop` mid-session.
    If a command returns `connection_failed`, its message now says whether to click the Allow prompt (it can hide behind the window) or restart Chrome.
    To skip the prompt entirely, have the user launch Chrome with `--remote-debugging-port=9222` (e.g. `open -a "Google Chrome" --args --remote-debugging-port=9222`).