Follow the window a site opens, so a person can finish a popup sign-in - #286
Merged
Merged
Conversation
zopeVaibhav
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 28, 2026 05:58
9 tasks
davidmckayv
added a commit
that referenced
this pull request
Aug 28, 2026
…288) * Hold a Bot's live screen per socket rather than per Bot A Bot has one live screen and a second connection replaces the first, but the replaced socket is not closed by that: it stays open and closes whenever its client gets round to it, which on an ordinary reconnect is after the replacement is already casting. So anything that stops a viewer has to establish that it owns the one it is stopping, and today only the close handler asks. The input handler, the two computer handlers and the follow loop's own attach all reach for whatever the session holds. This adds the slot that makes ownership something a caller cannot skip. A claim is taken for the socket before the browser is asked for a page, which is what lets a close landing during a cold launch be honoured at all: there is something to release before there is a cast, and the cast the launch produces afterwards is refused. A refused install stops the cast it was given and a refused follow loop is cancelled, because a caller that forgets leaks a screencast or a 1Hz timer against a browser nobody is watching. Three answers rather than two when asked what a socket may do, because a claim with no cast yet is a screen still opening and not a screen that ended, and telling somebody the wrong one of those is the whole reason the notification exists. Occupancy counts a teardown that is still running, so the session sweep cannot drop a Bot mid-handover. The predicate stays for now; index.ts moves onto the slot next, and doing that in one change rather than two keeps every commit building. No Playwright: Screencast arrives as a type, which is erased, so this and its tests run without Chrome. Each rule is covered both ways, and the suite was checked against seventeen deliberate breakages of the module to confirm none of it passes vacuously. * Ask which socket is acting before acting on its behalf The handlers reached for whatever the session held. Only the close handler asked whether the socket in front of it owned the screen, and the input handler, the sweep and the follow loop's own attach did not, which is three call sites of a question that was only ever answered at one. They go through the slot now, so the question is not one a caller can skip. The claim is taken before the browser is asked for a page: a close landing during a cold launch releases it, and the cast that launch produces afterwards is refused and stopped rather than installed for a socket that has gone. That was leaving a screencast and a 1Hz interval behind, and the interval called currentPage every second, which is a launch path, so a computer somebody had stopped came back up on its own. Input is answered rather than dropped. A superseded socket's typing used to dispatch through the session's cast and land in the page the current viewer was watching, and its sender heard nothing because the old check returned before reaching anything that could report. A socket that is still starting and a socket that is gone are told different things, because telling somebody whose screen is opening that it ended is a worse answer than none. The wheel is untouched and still refuses: it stands after the ownership question rather than instead of it, since the two refuse different things and owning the screen was never permission to drive it. The real-process tests are asked for by name, bun run test:live-screen, the way tests/smoke does. index.ts imports Playwright at module scope and CI installs the root workspaces and the two Bots but not this directory, so an ungated file would throw on import there and redden the build through the test-count floor. Reading the flag before the dynamic import keeps the default suite honest where Playwright was never installed, which was confirmed by running it with the package removed. Both failures were watched failing against the old handlers first: the stopped computer restarting, and the key from a replaced window arriving in the live one. The wheel case and the superseded close cannot fail that way, since both are existing behaviour, so each was confirmed to bite by breaking the check it guards and watching it go red. * Take the live screen down with the browser it was showing Stopping a computer released the wheel and left the viewer running, so the follow loop asked for a page a second later, which starts a browser, and the computer somebody had just stopped was up again. Reset had the same hole. Both are fixed by not putting the teardown in either handler. A browser closes from four places, and only two of them are requests: the cap closes one after somebody else's launch and the idle sweep closes one on a timer, neither with any request involved. A viewer surviving those kept the same loop asking for a page, so a Bot with somebody watching was immune to the idle timeout and came straight back after a cap eviction, which is the same failure arriving by a route no handler is on. So the close announces itself and the screen comes down with it. stop now goes through the same eviction path as the cap and the sweep, because a request is one more reason a browser closes rather than a different kind of closing. profiles still knows nothing about viewers: it says a browser went away and the module that owns the screen decides what that means. The announcement is awaited before the context closes, so a follow tick cannot fit a relaunch into the gap. The two request paths are covered against the real process. The cap and the sweep are reachable from no endpoint, and deliberately so, so they are driven through createProfiles rather than by adding a way in that only a test would use. Each observation was made both ways: the stopped computer restarting itself was watched failing first, and removing the announcement afterwards was confirmed to redden all four paths, while making stop close a browser its own way reddened the two request cases and left the other two green, which is what says the request path really does share the close rather than repeating it. * Drop the predicate the slot replaced It had one caller and the slot took that over, so it is dead now. Kept through the previous change only so index.ts moved across in one commit rather than leaving the tree unbuildable between two. Also says why one browser close stays silent. profiles announces a close so the screen watching it comes down, but the half-dead path closes a context whose browser has already gone and launches a replacement on the next line, and the follow loop re-attaches to it within the second. Telling the viewer there would end a screen that is about to be fine, which is the opposite of what the announcement is for. That is the only close of the four that does not fire it, and a reader checking coverage should not have to work out whether it was forgotten. * Drive the launch that fails, not just the ones that work open's catch was the one path in this change with no coverage against the real process. A file where the profile directory would go makes Chromium refuse to start, so currentPage throws with the claim already held, which is the arrangement the catch exists for and the one the cold-launch timing makes reachable. What it pins is that the socket is told and closed rather than left waiting on a browser that will never arrive, and that the Bot is still usable afterwards. The claim being released with it stays covered by viewer.test.ts, because a leaked claim changes nothing observable from outside this process until the session map has grown, and reaching that state through the port would prove less than the unit test already does. * Close a browser somebody asked to close, even one still starting Review found the request path falling through the mechanism the rest of this branch added. Everything hangs off closing a browser that is running, and a launch is not one of those until it finishes, so a stop or reset arriving inside a cold start answered "nothing was running" and left the browser up a moment later with the live screen still on it, its follow loop keeping it marked recently used so the idle sweep would not reclaim it either. Reset was worse: it deleted the profile directory that the finishing launch recreated, leaving the Bot signed into what it was told to forget. A request now waits for the launch it is racing. Deliberately not inside the eviction itself, because the cap evicts from inside a launch and would then be able to wait on the launch it is running under. The waits that were unbounded are bounded now. Telling a viewer its browser went away sits on another Bot's launch path, and stopping a cast decides whether a session can ever be swept, so a screencast that never answers used to pin a launch, keep a session occupied for the life of the process, and on the way out hold every profile's flush until the container was killed. Smaller things the same pass turned up: a refused cast was stopped outside the teardown accounting, so occupancy could report nothing casting while Chrome was still encoding; the close handler created a session for a Bot that had none, which the comment three lines above it argues against; the announcement callback was optional at the one call site the fix depends on; and a stale doc block described a body that had moved. Four tests were weaker than they read. The cold-close case never established that a browser had started, so it would have passed on a machine where the launch failed. "It did not come back" was one check after a fixed sleep, which cannot tell a relaunch that never happened from one still in flight, so it is held across a whole cold start now. The superseded message was asserted as any non-empty string. And the branch that tells somebody their screen is still opening, the whole reason there are three standings rather than two, had no test at all. The port was fixed at Tailscale's default, so on a host running it this file could never have started; it asks the operating system for a free one now. The announcement tests leaked their environment knobs and passed on the alphabetical order of the files, since Bun shares one module registry across a run and ignores the order they are named in. docs/development.md documents the lane beside test:smoke, including that it needs this directory's own install. * Say what changes for somebody running a Bot people watch The checklist in docs/development.md asks for this when a deployment behaves differently afterwards, and it does: a computer that was stopped stays stopped, a watched Bot stops being immune to the idle timeout and the cap, and a viewer whose screen ends is told why. It also says plainly that the app does not show that message yet, so nobody reads this and expects to see it on screen. * Follow the window a site opens, and come back when it closes (#286) --------- Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
agent-computerbound a Bot to the PlaywrightPagecaptured when Chromium launched, and to nothingthe site opened afterwards.
live.setwas the only write of that record andcontext.pages()wasread on that one line, so a window arriving later was invisible to
/stream,/read,/snapshot,/screenshotand every input path, all of which resolvecurrentPage.Popup OAuth is how a large share of "Sign in with Google" buttons work, so the flows a person takes
the wheel to finish were the ones they could not finish.
The browser now follows the window the site opens and returns to the opener when it closes, which is
what a sign-in popup does when it succeeds. The choice is one function in
live-page.ts, kept out ofprofiles.tsfor the reasonbrowser-eviction.tsandviewer.tsare:profiles.tsimportsPlaywright at module scope, so a decision living there needs a browser merely to be imported by a
test.
Two things fell out of the work:
button is drawn reached the opener's own button and navigated the page the Bot was working on.
page()treats a closed page as a deadbrowser and closes the whole persistent context. A sign-in popup closes itself the moment it
succeeds, so following one without moving back to the opener would have thrown away the profile
that had just received the sign-in.
Refs needed less than expected.
resolveRefalready refuses a ref from a superseded generation, socurrentPagebumpssession.snapshotIdwhen the page changes and a stale ref is refused with theexisting "take a new snapshot" rather than matching something on the wrong document.
Fixes #270.
Where it runs
retargetcloses over theliverecordprofiles.tsalready keeps, andsession.livePageis one reference beside thesnapshotIdthe same session object already holds. Both describe a browser running inside thisprocess, which is the one thing here that cannot be anywhere else.
agent-computercontainer and every call about it is routed to that container; a second replicahas its own browsers and its own records, exactly as before this change.
from Playwright's own event callbacks, and
page()still serialises launches throughstarting.there: the viewer re-reads
currentPageon its one-second follow interval and re-attaches thescreencast when the page it is casting is no longer the page the Bot is on. No new socket and no
cross-process delivery.
process already owns, and they go away with it.
Boundary and audit
is added, removed or reordered, and
actsOnTheComputerstill refuses a Bot acting while aperson holds the wheel.
change of page, which takes the existing
StaleSnapshotErrorpath and its 409.from what Chromium reports as open, never from a caller.
Worth saying rather than ticking: this widens what a Bot's own endpoints act on, from the page it
launched with to a page the site chose to open. The egress proxy is set on the context, so a followed
page browses through the same proxy under the same rules; the profile, the workspace and the control
gate are unchanged. What a Bot may do does not change. Where it may do it now includes a window the
site opened, which is the behaviour a person watching the screen already assumed it had.
Changelog
CHANGELOG.mdunderUnreleased.Proof
The new check fails on the bug.
agent-computer/tests/follows-popup.test.tslaunches a realChromium through
createProfiles, opens a popup and asserts the Bot's page follows it and comes backwhen it closes. Asked for by name, like the deployment journey at the repository root, because the
machine running
bun testis not required to have a browser:Against
main'sprofiles.tsandindex.ts, with only the test added:On this branch:
1 pass, 0 fail.End to end over HTTP, with a control. A fixture page with two buttons that differ only in how they
open the next document, both pressed through
/snapshotand/click. Each destination reports itsown load, so a popup that opened and is not being watched is distinguishable from one that never
opened.
Three runs of each arm on
mainand on the branch, plus two of each against the container image onmain. The same-document arm never changed, on either side.The live screen and the person holding the wheel. Watching
/streamwhile the popup opens, thentaking the wheel and clicking at the coordinates the popup's button is drawn at:
SAME DOCUMENTonmainis the click reaching the opener's other button and navigating the page theBot was working on.
What the change could have broken, checked on the branch.
409, "That list of elements is out of date: it wastaken for snapshot 2 and the page is now at 3. Take a new snapshot and use the refs from it."
/readanswers200, and the opener isstill open.
bun testinagent-computer: 179 pass, 1 skip, 0 fail. Rootbun run lint,format:checkandtypecheck: clean.Not fixed here. A Bot's browser still announces itself as headless, which some providers refuse
on its own. This makes the popup reachable and clickable; it does not make every provider accept the
browser it is reached in.