Skip to content

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

Closed
sanketsudake wants to merge 2 commits into
mainfrom
fix/serialise-daemon-spawn
Closed

fix(daemon): serialise the first attach so Chrome gets one consent prompt#17
sanketsudake wants to merge 2 commits into
mainfrom
fix/serialise-daemon-spawn

Conversation

@sanketsudake

Copy link
Copy Markdown
Owner

Closes the defect that froze a real Chrome session during the RFC work.

What happened

Ensure checked for a running daemon, found none, and spawned one — with nothing making that path single-file:

if c := TryConnect(sockPath); c != nil { return c, nil }
_ = os.Remove(sockPath)
cmd := exec.Command(exePath, "__daemon", sockPath)   // every caller gets here

Several chrome-cdp processes starting at once each found nothing and each spawned a daemon. Every spawned daemon attaches to Chrome, and every attach raises its own "Allow remote debugging?" prompt.

That prompt is browser-modal, so stacked prompts are not a slower version of one prompt: the visible dialog need not be the one holding input, and the browser looks frozen with no button that responds. That is exactly what was reported — "Chrome was frozen, I was not able to click on any of the option".

It happened because several review agents each ran the built binary to verify exploits, and their attaches raced. The daemon exists so the prompt happens once per session; nothing was making the first attach single-file.

The unlink is the other half. Outside a lock, a late caller's os.Remove(sockPath) can delete a socket a sibling daemon has just bound — orphaning a live daemon that no client can ever reach.

The fix

An exclusive flock covers spawn-and-wait, with a re-check inside it so callers that queued behind the holder find the daemon it started rather than duplicating it. That re-check is what makes N callers converge on one daemon and one prompt.

The wait is deliberately unbounded. The holder may be waiting out a prompt the user hasn't clicked yet, and blocking behind that is the correct outcome — spawning our own would add another prompt to the pile, which is the failure this exists to prevent.

The lock file is never unlinked: removing it would let a later caller lock a different inode and defeat the exclusion entirely.

The regression test

Eight concurrent Ensure calls against a substitutable spawn it can count. Proven red by disabling only the lock (keeping the test seam):

spawn_test.go:76: 8 daemons were spawned for 8 concurrent callers, want exactly 1
                  — each spawn attaches to Chrome and raises its own consent prompt

With the lock: exactly 1.

The fake spawn binds its socket 150 ms after being asked, so the window between "spawned" and "connectable" is real rather than instantaneous and every caller must still converge on the one listener.

Diagnostics

did you click Allow in Chrome? isn't much help when the reason you haven't is that you can't see it. The connect timeout now says the prompt may be hiding behind the window and that Chrome accepts no other input until it is answered — so a user seeing a frozen browser has the actual next step. skills/drive-chrome-cdp/SKILL.md says the same, since it previously warned about the wedge without saying the prompt is browser-modal.

Testing

gofmt, go vet, go test -race ./... all clean including the live-Chrome suite. No live Chrome was driven to develop this — the tests substitute the spawn, so the concurrency is exercised without attaching to a real browser.

Note for reviewers

A long t.TempDir() path breaks a Unix socket bind on darwin (sun_path caps near 104 bytes, and the dir embeds the test name — it failed with a bare bind: invalid argument). The test uses a short temp dir and says why, in case another socket test hits it.

sanketsudake and others added 2 commits July 27, 2026 17:35
…ompt

Ensure checked for a running daemon, found none, and spawned one — with
nothing making that path single-file. 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 "Allow
remote debugging?" prompt.

That prompt is browser-MODAL, so stacked prompts are not a slower version
of one prompt: the visible dialog need not be the one holding input, and
the browser looks frozen with no button that responds. It happened to a
real user's session during this work — several review agents each ran the
binary, and their attaches raced.

The unlink was the other half. Outside a lock, a late caller's
os.Remove(sockPath) can delete a socket a sibling daemon has just bound,
orphaning a live daemon that no client can reach.

An exclusive flock now covers spawn-and-wait, with a re-check inside it so
callers that queued behind the holder find the daemon it started instead
of duplicating it. The wait is deliberately unbounded: the holder may be
waiting out a prompt the user has not clicked, and blocking behind that is
correct — spawning our own would add to the pile.

The regression test runs eight concurrent Ensure calls against a fake
spawn it can count. Without the lock: 8 daemons, so 8 prompts. With it: 1.

The connect timeout message now says the prompt may be hiding behind the
window and that Chrome accepts no other input until it is answered, since
"did you click Allow" is not much help when the reason you have not is
that you cannot see it. The skill says the same.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Written from a controlled reproduction of the wedge that froze a real
browser during this work, which contradicted the first diagnosis.

Stacked prompts are real and #17 fixes them, but a SINGLE unanswered
prompt wedges Chrome just as thoroughly, so serialising the spawn is
necessary and not sufficient.

The mechanism, measured: while consent is pending, TCP connects
immediately and the WebSocket upgrade HANGS — it never completes and
never refuses. The client therefore has no error to classify, only
silence, which is why the failure surfaces as an undifferentiated
timeout. Three defects follow: the daemon gives up in ten seconds and
abandons the prompt it raised, so a later click grants consent to a
connection that no longer exists; ten seconds is not a human timescale
for a browser-modal dialog that can sit behind the window; and `doctor`
reports "attach ready" from the port file without ever probing, so the
one command whose job is to answer "can I connect?" said yes while every
connection was hanging.

Also recorded because it cost time: /json/version returning 404 is NOT a
consent signal — it returns 404 in this connection mode either way, and
an early reading of it as pending-consent was wrong.

The proposal is to wait for the consent rather than abandon it, classify
the pending state distinctly, make doctor probe, and lead the docs with
the launch flag that never prompts.

The pending state is just a listener that accepts and stalls, so nearly
all of it is testable with net.Listen and no browser — which matters,
because reproducing this by hand wedged a real browser twice and must
not become the regression test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sanketsudake

Copy link
Copy Markdown
Owner Author

The reproduction changed the diagnosis — this PR is necessary but not sufficient

I ran a controlled reproduction against a real Chrome. It contradicted the theory this PR was built on, so I've added RFC-0013 to the branch recording what actually happens.

Stacked prompts are real and this PR fixes them — 8 concurrent callers produced 8 daemons before, 1 after. But a single unanswered prompt wedges Chrome just as thoroughly. Serialising the spawn removes one cause, not the failure.

The mechanism, measured

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 whole thing. Chrome doesn't reject the connection while it waits for the user; it holds the upgrade open and says nothing. The client has no error to classify — only silence — which is why this surfaces as an undifferentiated timeout.

Three defects follow, each independent of concurrency:

  1. The daemon abandons the prompt it raised. It dials, the upgrade hangs, the dial times out in ~10s, 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. Observed directly: the daemon process was gone 25 seconds after the prompt appeared.
  2. 10s is not a human timescale for a browser-modal dialog that can sit behind the window. A user who hasn't clicked it usually hasn't seen it.
  3. doctor reports readiness it never verified. It read the port file and said "debug endpoint reachable — Path B attach ready", handing back a ws:// URL, while every actual connection was hanging.

A correction

I initially read /json/version returning 404 as a consent-pending signal. That was wrong — it returns 404 in this connection mode whether or not consent has been granted, because the chrome://inspect toggle path exposes the WebSocket without the HTTP JSON API. Any detection built on that would have been wrong too. Recorded in the RFC so it isn't rediscovered.

Also worth stating plainly

My own probing made the user's wedge worse. Each WebSocket upgrade attempt is another connection request and raised another prompt. The RFC's test plan is deliberately built on net.Listen stubs — a listener that accepts and stalls reproduces the pending state exactly — because reproducing this by hand wedged a real browser twice and must not become the regression test.

What still ships here

The serialisation, its 8-to-1 regression test, and the improved timeout message. RFC-0013 proposes the rest: wait for the consent instead of abandoning it, classify the pending state distinctly, make doctor probe, and lead the docs with --remote-debugging-port — the path that never prompts at all.

@sanketsudake

Copy link
Copy Markdown
Owner Author

Superseded by #18, which now targets main and contains these commits plus the RFC-0013 implementation built on them.

Keeping them together because the reproduction showed they are one fix, not two: serialising the spawn stops N prompts, but a single unanswered prompt wedges Chrome just as thoroughly. Reviewing the spawn lock without the consent lifecycle would review half a fix.

sanketsudake added a commit that referenced this pull request Jul 27, 2026
…ng on

#17 made the FIRST attach single-file, and VS-7 has held since: never two
prompts at once. US-5 — "at most one consent request" — did not. Behind the
spawn lock each queued caller in turn cleared the previous verdict, spawned its
own daemon and raised its own prompt at a browser that was already holding one.
Eight concurrent commands against an unanswered dialog came to about seventeen
minutes and eight sequential prompts: one at a time, which is not the same as
one.

A consent_pending verdict written by the previous holder is now inherited
rather than re-derived, for five seconds. Queued callers are released within
milliseconds of it being written, so that is long enough to drain a queue and
short enough that a user who has just found the dialog and clicked Allow is not
told to go looking for it again.

Ensure also takes a context. It had none, and chrome.Connect discards its own
by design, so --timeout stopped applying the moment a command needed a
connection: a caller queued behind a holder sitting on an unanswered prompt
inherited that wait with no way to say otherwise. The flock wait now runs on
its own goroutine and hands the lock back if the caller has given up, rather
than being abandoned holding it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sanketsudake added a commit that referenced this pull request Jul 27, 2026
* fix(daemon): serialise the first attach so Chrome gets one consent prompt

Ensure checked for a running daemon, found none, and spawned one — with
nothing making that path single-file. 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 "Allow
remote debugging?" prompt.

That prompt is browser-MODAL, so stacked prompts are not a slower version
of one prompt: the visible dialog need not be the one holding input, and
the browser looks frozen with no button that responds. It happened to a
real user's session during this work — several review agents each ran the
binary, and their attaches raced.

The unlink was the other half. Outside a lock, a late caller's
os.Remove(sockPath) can delete a socket a sibling daemon has just bound,
orphaning a live daemon that no client can reach.

An exclusive flock now covers spawn-and-wait, with a re-check inside it so
callers that queued behind the holder find the daemon it started instead
of duplicating it. The wait is deliberately unbounded: the holder may be
waiting out a prompt the user has not clicked, and blocking behind that is
correct — spawning our own would add to the pile.

The regression test runs eight concurrent Ensure calls against a fake
spawn it can count. Without the lock: 8 daemons, so 8 prompts. With it: 1.

The connect timeout message now says the prompt may be hiding behind the
window and that Chrome accepts no other input until it is answered, since
"did you click Allow" is not much help when the reason you have not is
that you cannot see it. The skill says the same.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: RFC-0013, surviving Chrome's consent prompt

Written from a controlled reproduction of the wedge that froze a real
browser during this work, which contradicted the first diagnosis.

Stacked prompts are real and #17 fixes them, but a SINGLE unanswered
prompt wedges Chrome just as thoroughly, so serialising the spawn is
necessary and not sufficient.

The mechanism, measured: while consent is pending, TCP connects
immediately and the WebSocket upgrade HANGS — it never completes and
never refuses. The client therefore has no error to classify, only
silence, which is why the failure surfaces as an undifferentiated
timeout. Three defects follow: the daemon gives up in ten seconds and
abandons the prompt it raised, so a later click grants consent to a
connection that no longer exists; ten seconds is not a human timescale
for a browser-modal dialog that can sit behind the window; and `doctor`
reports "attach ready" from the port file without ever probing, so the
one command whose job is to answer "can I connect?" said yes while every
connection was hanging.

Also recorded because it cost time: /json/version returning 404 is NOT a
consent signal — it returns 404 in this connection mode either way, and
an early reading of it as pending-consent was wrong.

The proposal is to wait for the consent rather than abandon it, classify
the pending state distinctly, make doctor probe, and lead the docs with
the launch flag that never prompts.

The pending state is just a listener that accepts and stalls, so nearly
all of it is testable with net.Listen and no browser — which matters,
because reproducing this by hand wedged a real browser twice and must
not become the regression test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(browser): a three-way probe of the debug endpoint

While Chrome holds its "Allow remote debugging?" consent prompt it does not
refuse the connection: it accepts the TCP connect and then holds the WebSocket
upgrade open, saying nothing. There is no error to classify, only silence — so
a boolean "reachable" puts that silence in the same bucket as a refused port,
and the tool cannot tell "nothing is listening" (fast, real failure) from
"waiting for a human" (not a failure at all).

This adds the observation the distinction needs: one upgrade, classified as
refused / pending / ready, with a dial timeout, a threshold past which silence
counts as the consent signature, and a total budget the same socket is held
open across. Holding it matters — an answer that arrives after we hung up is an
orphaned prompt, which is how clicking Allow came to grant consent to a
connection that no longer existed.

Nothing uses it yet. Note the deliberate absence of any /json/version check:
that endpoint returns 404 on the chrome://inspect path whether or not consent
has been granted, so detection built on it would be wrong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* feat(connect): wait out Chrome's consent prompt instead of abandoning it

The daemon used to dial, hit a ~10s deadline while Chrome held its consent
dialog, write a generic error and exit — leaving the modal on screen with
nothing behind it. Clicking Allow then granted consent to a connection that no
longer existed, and because the prompt is browser-modal the whole browser stayed
frozen. Ten seconds is not a human timescale for a dialog that can sit behind
the window.

So the ladder now decides on the three-way probe state, and an open port with a
hanging upgrade is its own rung: consent_pending, distinct from "enable the
toggle". The daemon holds that upgrade open for consent_timeout (default 120s,
flag > env > file), publishes a marker beside its socket the moment it starts
waiting — it is detached, so a file is the only channel it has — and Ensure
moves its own deadline when it sees one. A refused endpoint is untouched by all
of this and still fails in milliseconds, which is the property that makes a
two-minute wait safe to have.

consent_pending is a new error.code on the EXISTING exit 3: callers branch on
the code, and the number is contract.

doctor now probes. It used to read DevToolsActivePort and report "Path B attach
ready" without ever connecting, and during the reproduction it said ready while
every connection was hanging — a diagnostic that reports readiness it never
verified sends the user looking anywhere but the dialog. It prefers a running
daemon (already holding a verified connection, and asking it raises no prompt),
falls back to one upgrade after saying it is about to, and takes --no-probe for
a user who wants a diagnosis and not a connection.

Every failure message now leads with --remote-debugging-port, which never
prompts, and offers chrome://inspect second with a note that it prompts on every
fresh attach. Recommending the toggle first walked each new user straight into
this.

Tests are stub listeners that accept and stall — the manual reproduction wedged
a real browser twice and must not be the regression test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: lead with the launch flag, document the consent prompt

Every setup path in the docs sent the user to chrome://inspect first, which is
the one route that raises a browser-modal consent prompt on every fresh attach —
so the documentation itself walked each new reader into the failure. The flag
goes first now, the toggle second with what it costs stated plainly.

Also documents what the prompt actually does (modal to the whole browser, can
hide behind the window, blocks all other input), the consent_pending code on
exit 3, --consent-timeout and the consent_timeout config key, and doctor's three
states plus the fact that it answers through a running daemon rather than
opening a connection of its own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(connect): resolve --port to a ws URL before probing it

An explicit --port names an http:// endpoint, and the upgrade probe would have
handshaked against "/" on it — never a 101, so the healthiest possible setup
(the launch flag this RFC now recommends to everyone) would classify as refused
and fall through to "enable the toggle".

Resolve it through /json/version first, which is the same lookup chromedp's
remote allocator does, so the probe and the attach agree on the endpoint. That
API locates a browser and can never classify one: it answers identically whether
or not consent is pending, which is the trap this RFC records.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(doctor): verify readiness, and stop echoing every open tab

RFC-0013 item 3 is "doctor reports readiness it never verified". The fix
removed the unverified claim at the port-file level and reintroduced it one
level up: the daemon path emitted state:ready on `running: true` alone, and
`running` came from dialling the Unix socket and closing it. Three claims
stacked — TryConnect's, a swallowed StatusInfo error, and a __status that
hardcoded "connected": true — so the trigger is ordinary. Start a daemon, quit
Chrome: the chromedp connection is dead, Serve holds the listener for another
30 minutes, and doctor answers ok:true / ready / exit 0 while every subsequent
command fails. VS-5 asks for the ready case to be established by a completed
round trip, not by a file or a socket.

__status now reports `connected` as the answer to the List it already made,
Status propagates StatusInfo's error instead of discarding it, and doctor
requires that positive evidence before claiming ready.

The same payload was also a privacy leak. doctor blanket-copied the daemon's
status map, which carried []target.Info — every open tab's title and full URL.
SKILL.md makes `doctor --json` step 1 of every agent session, so each session
pulled OAuth callbacks, reset tokens and internal hostnames into the transcript
before a tab had even been selected. The status payload now carries a
target_count, and doctor copies an allowlist rather than the map.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(probe): bound the handshake read in size and in time

The probe expects one line of HTTP back. It read with an unbounded
bufio.Reader.ReadString('\n') and set no read deadline, so the only ceiling on
either was the caller's total wait — 120s in the daemon.

Anything on the machine that can bind the loopback debug port can therefore
answer with a stream that never contains a newline. Measured against a local
listener doing exactly that: 18 GB of heap in six seconds, ~3 GB/s, and the
daemon's full budget would reach hundreds of gigabytes.

An 8 KiB LimitReader ends that stream at the limit, where the missing newline
becomes an ordinary refusal. The read deadline is the same bound expressed in
time, so the goroutine cannot outlive the wait even if nobody closes the socket
— but reaching it is reported as SILENCE rather than as a failed handshake,
because classifying a deadline as "refused" would turn a Chrome still holding
its consent prompt into a dead endpoint.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(connect): make consent detectable on the path that prompts

ResolveWSURL declared "no endpoint" whenever /json/version did not answer 200.
But RFC-0013's fourth observation is that on the chrome://inspect toggle path
/json/version 404s regardless of consent state — the toggle exposes the
WebSocket without the HTTP JSON API. So the lookup failing carried no
information, and treating it as failure blinded the tool on the only path that
raises the dialog.

The result: `chrome-cdp --port 9222 <verb>` against a toggle-path Chrome
holding an unanswered prompt classified the endpoint as refused, fell to
InstructToggle, and told the user "Chrome is running but not debug-enabled" —
about a Chrome that is debug-enabled and is showing them the consent dialog at
that moment. They were sent to re-enable a setting that was already on.

A failed lookup now falls back to the ws:// root of the same host:port. In the
granted case that costs nothing (an endpoint that will not upgrade there is
classified refused, which is where it already was); in the pending case the
hang becomes visible, which is the one unambiguous consent signature there is.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(consent): one meaning for consent_timeout, in every layer

The value crossed four layers and three of them read it differently.
chrome.Connect mapped <= 0 to DefaultConsentTimeout; daemon.Ensure took the
zero literally; main forwarded CHROME_CDP_CONSENT_TIMEOUT to the daemon only
when > 0. So with consent_timeout = "0s" (or a negative value) the daemon
waited the full 120s while its client gave up at 10s and printed "still waiting
... after 0s" — which is the orphaned-prompt failure this parameter exists to
prevent, restored through its zero value.

Normalisation now happens where flag, environment and config file actually
resolve: config.ResolveFrom/FromEnv for the file and the environment (including
the daemon subprocess's own resolution), and App.connOpts for the flag, which
is the one path config resolution never sees. Both call the same
chrome.ClampConsentTimeout, so no two layers can drift.

The range is clamped as well as the zero. Below one second the wait is not a
wait; above ten minutes it stops being a timeout, because the daemon spawn lock
is held for its duration and an inherited CHROME_CDP_CONSENT_TIMEOUT=8760h
would have blocked every other invocation for a year.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(doctor): diagnose the Chrome --port names

doctor called browser.FindPortFile("") and never consulted a.port, so
`doctor --port 9333` read the DevToolsActivePort file, probed whichever Chrome
that file happened to name, and reported THAT browser healthy. Every other verb
resolves --port ahead of the port file, which makes doctor the one command that
can answer a question about a different browser than the one asked about — and
the one command whose entire job is to be believed.

Endpoint resolution moves into browser.FindEndpoint, shared by chrome.Connect
and doctor so the command that diagnoses the connection and the command that
makes it cannot disagree about where Chrome is. doctor also resolves the
WebSocket URL before probing, which an http:// endpoint from --port requires
and which it previously never did.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(daemon): three ways a spawn could fail without saying so

Each of these ends with the user being told about a consent prompt that has
nothing to do with what happened.

A daemon that published its .pending marker and then died left Ensure with no
exit but the deadline: ~130s of waiting for a process that no longer existed.
spawnDaemon called Process.Release() and discarded the handle, so no liveness
check was possible. It now keeps a handle and reaps the child in the
background, which is both the reaping and the signal — kill(pid, 0) would not
have worked, because a dead child is a zombie until it is waited for, and a
zombie answers a liveness signal perfectly well. Child gone with no .err is an
immediate daemon_error.

lockSpawn blocked in complete silence. A second command run during a pending
prompt hung for over two minutes with no output — the same "my tool has frozen
and I do not know why" that US-2 exists to end, reached through the fix for
US-2. It now tries the lock non-blocking first purely so contention can be
named, then blocks as before.

RunDaemon reported nothing for anything that failed AFTER the connect. The
daemon is detached, so its stderr is never read: a failed net.Listen exited
silently, and Ensure — seeing the pending marker and no .err — reported the
consent prompt for a bind failure. The RFC's own darwin sun_path note makes
that reachable. Bind failures now write the .err sidecar with a daemon_error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(daemon): stop queued callers re-raising the prompt they are waiting on

#17 made the FIRST attach single-file, and VS-7 has held since: never two
prompts at once. US-5 — "at most one consent request" — did not. Behind the
spawn lock each queued caller in turn cleared the previous verdict, spawned its
own daemon and raised its own prompt at a browser that was already holding one.
Eight concurrent commands against an unanswered dialog came to about seventeen
minutes and eight sequential prompts: one at a time, which is not the same as
one.

A consent_pending verdict written by the previous holder is now inherited
rather than re-derived, for five seconds. Queued callers are released within
milliseconds of it being written, so that is long enough to drain a queue and
short enough that a user who has just found the dialog and clicked Allow is not
told to go looking for it again.

Ensure also takes a context. It had none, and chrome.Connect discards its own
by design, so --timeout stopped applying the moment a command needed a
connection: a caller queued behind a holder sitting on an unanswered prompt
inherited that wait with no way to say otherwise. The flock wait now runs on
its own goroutine and hands the lock back if the caller has given up, rather
than being abandoned holding it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* refactor(consent): one authored explanation of the prompt

The "browser-modal / BEHIND the window / accepts no other input / not a crash"
paragraph was hand-written in five places: chrome.consentPendingMsg,
chrome.connectFailMsg, the daemon's wait notice, Ensure's give-up message, and
doctor's consent_pending state. It had already drifted — one copy said "behind"
where the others shouted it, one said "blocks all other input" — and two test
files asserted on a hardcoded substring list that one of those five copies
would have failed.

browser.ConsentPromptAdvice is now the one authored version, with each call
site composing its own prefix, exactly as browser.EnableAdvice already does.
The two assertion lists collapse to a single strings.Contains against the
const, which is the part that matters: a list of substrings cannot tell five
paragraphs apart, and that is how they drifted in the first place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(connect): tell the user about the prompt on the --no-daemon path too

Options.OnConsentPending was assigned in exactly one place — the daemon — and
the --no-daemon connector passed a ConsentTimeout with no hook at all. So
`--no-daemon` waited out the prompt in complete silence: up to two minutes with
the browser frozen and nothing written to stderr.

RFC-0013's US-2 asks to be told a consent prompt is pending WHILE it is
pending, and this is the one path where nothing else can do it — there is no
daemon to publish a .pending marker and no Ensure to read one. No test caught
it, because every test that exercises the hook supplies its own.

The options now come from directConnectOptions, which exists as a named
function so the hook can be asserted on at all: cmd/chrome-cdp had no tests,
which is part of why an unwired seam went unnoticed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(doctor): say what the probe's `ready` verdict cost

browser.Upgrade's doc comment states the governing model: a probe that
connects, learns the answer and hangs up has spent the user's click on a
connection nobody kept. ProbeWS does exactly that on every outcome including
WSReady, so doctor's `ready` was falsified by the act of producing it — on the
chrome://inspect path the next command is a fresh attach and prompts again.

Of the two ways to make the comment and the code agree, doctor takes the
honest-output one. Handing the live socket to the connection it just proved
possible is the other, and doctor is the wrong place for it: it is a
diagnostic, it was not asked to connect, and it has nothing to hand the socket
to. So the ready verdict now says the probe's connection was closed, that the
next command may prompt again, and that the daemon is how to be asked once per
session — and ProbeWS's own comment says why it hangs up.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* refactor(probe): one loop, two named timings, and no discarded answer

AwaitUpgrade was a clamp, two timers, two selects, a remainder computation and
a `rest <= 0` branch, with the WSPending exit written three times. One loop with
two independent timers is the same behaviour in a third of the lines — except
for the case the branch had and the loop does not.

When PendingAfter >= Total the remainder came out <= 0 and the function
returned WSPending having never looked at the answer channel again. That is
every doctor probe, because ProbeWS passes the same value for both; and
onPending runs in between, which is not instantaneous — the daemon's writes a
file. Anything the endpoint delivered during it was discarded unread and its
socket closed, so a completed handshake was reported as a pending consent
prompt. The loop consults the channel once more before giving up, and the read
deadline now sits just past the budget rather than exactly on it, so an answer
landing as the wait ends is still readable rather than being cut off by the
backstop meant to protect against a runaway goroutine.

The three positional durations become UpgradeTimings{PendingAfter, Total}:
they were easy to swap and the consequences are not symmetric. dialTimeout
leaves the signature entirely — two packages had independently declared the
same two seconds for the same loopback dial, and no caller has information that
would make a different value right.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* refactor: move the endpoint probe next to the connection it feeds

internal/browser's own package doc says it is "deliberately free of chromedp so
it unit-tests without a live browser". That claim was true of a
DevToolsActivePort parser and a decision table. It did not survive the probe:
a TCP dialer, an HTTP client, a hand-rolled RFC 6455 handshake, a reader
goroutine, and a live net.Conn handed across the package boundary for
internal/chrome to close.

The socket work moves to internal/chrome/probe.go, next to the connection it
exists to serve and to the code that owns that net.Conn's lifetime. What stays
behind is what the doc actually describes: WSState (the vocabulary a probe
answers in), Probe, Action, DecideConnection, the endpoint resolution, and the
two advice constants. Both package docs now say what their package is.

The tests move unchanged — they are net.Listen-based and need no browser in
either package. `settle` had to be renamed on the way in, which is its own
small argument for the move: internal/chrome already had a settle(), and every
"settle" in this package means "wait until it stops moving" while the probe's
meant "close it or keep it". It is now `upgraded`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* refactor(doctor): one vocabulary for the endpoint's three states

There were four ways to name the same three answers: browser.WSState's
constants, browser.Action, doctor's own stateNoEndpoint/stateConsentPending/
stateReady, and a bare "unverified" string literal. Two of them already
disagreed — WSState.String() returned "refused" where doctor's envelope said
"no_endpoint" — and the only thing keeping that from shipping as two names for
one state was that WSState.String() had no callers at all. That is not a
design, it is a coincidence.

WSState.String() is now the wire value, doctor derives its `state` field from
the probe's answer in one assignment instead of three, and the switch carries
only the prose and the exit code that genuinely differ per outcome.
"unverified" becomes a named constant and is documented as the one value with
no WSState behind it, because it is the absence of a probe rather than the
result of one. doctor's --help promised three states while four shipped; it now
names all four.

Action.String() goes: no caller anywhere, and the branch had grown a case on it
that nothing would ever have printed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test: one stall-listener harness instead of three

The consent-pending state is a TCP listener that accepts and says nothing, so
every scenario in RFC-0013 is reproducible with net.Listen and no browser —
which matters more than usual here, because reproducing it by hand wedged a
real user's Chrome twice and a test that needs a human to click a browser-modal
dialog is not a test.

Three packages needed the same three listeners and each had grown its own.
stallListener existed twice with different signatures; answerListener and
lateAnswerListener were the same function; stubEndpoint was a third variant
with a port file bolted on — around 170 lines, with separately maintained
comments explaining the same thing three times. Only two of the three counted
accepted connections, which is the assertion that proves the property the whole
RFC turns on: every connection to the debug endpoint is a consent request, so
"how many did we open" is the question being asked.

internal/probetest gives all three Stall, Answer, Closed, the connection count,
the answered-on-a-live-socket flag, and the port file. It cannot live in
chrometest, which imports internal/chrome — one of the packages that needs it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test: remove two real flakiness risks

The daemon's pending-marker test sat a 3s consent budget and a 3s poll around a
2s threshold living in another package, which the test had no way to reach.
Measured: 3.05s against a marker at ~2.0s — a 1.5x margin on exactly the kind
of timing this repo has been bitten by four times on macOS CI.

The threshold is now chrome.Options.ConsentPendingAfter, alongside the
ConsentTimeout and OnConsentPending it belongs with. The test sets 100ms for a
30x margin, and both the mutable package var and the shrinkPendingThreshold
helper delete themselves.

The probe's own announce test asserted `pendingAt > 400ms` against a 100ms
timer: an absolute wall-clock ceiling on a timer plus a goroutine wakeup, on CI
boxes that are not idle. The property is an ordering — the announcement lands
during the wait, not on the way out — and its sibling in the connect test
already expressed it that way with `pendingAt > elapsed/2`. Now both do.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: the smaller findings, including two low-severity security ones

Handshake verification. The Sec-WebSocket-Key was generated with crypto/rand
and then never checked against Sec-WebSocket-Accept, so "ready" meant nothing
more than "the status line said 101" — a listener replying "HTTP/9.9 101
whatever" passed. Chrome's debug port is a loopback port any local process can
bind, and being told ready by something that is not a WebSocket server is how
the probe hands chromedp a socket that will never speak CDP. The accept key is
now computed and compared, and Upgrade: websocket is required. probetest's
stubs complete the handshake properly, so they are testing the thing.

Endpoint validation. ResolveWSURL returned any webSocketDebuggerUrl verbatim
and http.Client follows up to ten redirects, so a request about 127.0.0.1 was
verified to come back with ws://10.1.2.3:4444/pwned — the URL the probe dials
and chromedp attaches to. Redirects are no longer followed, and the answer is
only accepted if it names the endpoint that was asked (scheme ws/wss, no
userinfo, same port, same host modulo two spellings of loopback).

Also: attach() now gets the RESOLVED ws URL, so chromedp does not repeat the
lookup and the comment claiming the probe and the attach agree on what they are
talking to is true; Probe.PortFileWS becomes Endpoint, which is what it holds
half the time, and DecideConnection drops the guard that restated its own
precondition; Upgrade.Close loses the nil-receiver half that cannot fire; the
.pending payload says who it is for; spawn_test uses wg.Go; and three comments
that narrated the change rather than the result are cut to what stays true.

One correction to the review: Action.String() was reported as having no callers
and is kept, because %v in the ladder's table test is one — without it a
failure reads "= 4, want 2".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: the behaviour that changed under review

doctor's four states and what each is worth; that a merely-running daemon is
not evidence of a connection; that it honours --port; that a probe-derived
`ready` costs the consent click it just spent; and that it reports a tab COUNT
rather than the tab list, which is the part the Agent Skill most needed to
know since it runs `doctor --json` before anything else.

Also: consent_timeout's clamp and why "0s" cannot mean "do not wait"; the
notices on the --no-daemon and queued-caller paths; and the two verification
scenarios the review added — a bounded handshake read, and a 101 that is not a
completion of our own handshake.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(record): assert --scale against the box asked for, not a second capture

macOS CI failed the --scale assertion again: a 0.5 recording of a pinned
800x600 viewport came back a square 300x300. Reproduced locally at about
one run in eight.

Chasing it found the viewport is not stable enough to be the reference.
It was observed reverting BETWEEN two adjacent recordings — the scale-1
capture saw the pinned 800x600 and the scale-0.5 one saw 600x600 — with
both CDP's visual viewport and the page's own innerWidth/innerHeight
agreeing on the wrong value, so a cross-check between them does not catch
it either. A test that compares one capture against another is therefore
measuring window stability, not --scale.

So the claim is decomposed instead. Live: Chrome honours the cap it was
given, asserted against the max box RecordStart already reports rather
than against a second recording. Pure: the cap is round(viewport x scale),
now scaleBox with a table test and no browser at all. Together those are
VS-8, and neither half is hostage to a viewport that moves.

The recorder also now cross-checks CDP's visual viewport against the
page's own before sizing. That does not fix this failure, but the two
measures were separately observed disagreeing after a resize, and sizing
a whole recording from a box that never existed gives every frame the
wrong aspect.

Verified: 10 consecutive TestRecordLive runs green, where the original
failed 1 in 8.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(record): the screencast cap is an upper bound, so assert it as one

CI: "--scale 0.5 produced a 207x207 frame against the 378x207 box it
asked Chrome for". The frame was correct and the assertion was wrong.

record.go says so directly, on the field the test was reading: the caps
are "not a promise about the frames — a screencast frame is never
UPSCALED to fill them, so the real dimensions are whatever the compositor
produces WITHIN this box". I asserted equality against a documented upper
bound, and it held only while the window happened not to move between the
box being computed and the frame being produced.

207x207 inside a 378x207 box is Chrome behaving as specified on a window
that resized underneath it. That is Chrome's business, not this feature's.

What is ours to guarantee is that the cap is honoured AS a cap and that a
scaled recording still produces pixels, so that is what the live test now
checks. The arithmetic making the cap half the viewport is
TestScaleSizesTheBox, with no browser involved.

Better failure messages are what found this: naming the box alongside the
frame turned three rounds of guessing at viewport timing into one reading
of the contract.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
sanketsudake added a commit that referenced this pull request Jul 27, 2026
All thirteen are implemented and merged, so the index and each header now
say so and link the PR that did it. RFC-0002, 0003, 0004 and 0011 also
name the follow-up PRs that materially changed them, since a reader
chasing why the shipped behaviour differs from the proposal should not
have to find that by bisecting.

The folder is now a design record rather than a to-do list, and the
README says which it is. The gap analysis is left in the present tense of
when it was written — the reasoning is the point, and rewriting it into
the past would lose why that ordering was chosen.

RFC-0013's references to #17 are replaced: that PR was superseded by #18,
which carries its commits, so the links pointed at a closed PR.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sanketsudake
sanketsudake deleted the fix/serialise-daemon-spawn branch July 27, 2026 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant