Skip to content

fix(preview): keep human control across viewer disconnects - #628

Open
lukemaj wants to merge 1 commit into
mainfrom
t3/preserve-preview-control
Open

lukemaj wants to merge 1 commit into
mainfrom
t3/preserve-preview-control

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

What: Human control of an agent-opened preview tab survives viewer disconnects and passive agent reads.
Why: Viewer teardown called SessionControl.release, which made the opening agent the controller again without a human release.
So what: Review this fix for #627; merge and installation require Lukasz's approval and are not authorized here.

Closes #627.

Keep a disconnected human reservation in SessionControl, invalidating queued input and draining held keys/buttons without returning authority to agents. A reconnected human can choose Take control, then Release control to resume the agent. Failed replacement authorization retains the human reservation. Allow the opener's snapshots through the existing observation path while a human drives. Existing watch ownership fences remain unchanged.

This supports the current Objective by keeping shared browsing safe for Promachos and Drafters. The same server owner governs web, desktop streamed tabs, mobile, and remote viewers; no provider-specific or wire-contract changes are needed. The Browser guide explains reconnect recovery and the map includes #627.

Reproduction and root cause

On base 3ec7a5dd84753d18902a39ca3390f95879b25528, the two new broker/viewer regressions failed before the implementation change:

node_modules/.bin/vp test run apps/server/src/preview/ServerBrowser.test.ts -t 'an agent.*tab.*human'
  • Agent opens tab, human takes control, viewer scope closes: expected human, received agent.
  • Agent snapshots its own tab while a human drives: PreviewAutomationControlInterruptedError, reason humanControl.

The real broker, viewer handler and scope finalizer run against the existing fake Chromium/CDP boundary. Viewer disconnect is the reproduced handback mechanism. The precise renderer dependency or transport event that disconnected the original Messenger viewer is unknown; no live login or browser automation was performed. Status, navigation/focus, watch bindings and reconnect source paths were inspected; retained watch reads and watch stop/start attempts now have control-invariance coverage. Full investigation and upstream search: #627 (comment).

Proof

Candidate: 7c6abcdf82a80c0468c4329707cb893dfbcb4b3a.

node_modules/.bin/vp test run apps/server/src/preview/SessionControl.test.ts apps/server/src/preview/ServerBrowser.test.ts apps/server/src/preview/ServerBrowserStream.test.ts
node_modules/.bin/vp run --filter t3 typecheck
node_modules/.bin/vp lint apps/server/src/preview/SessionControl.ts apps/server/src/preview/SessionControl.test.ts apps/server/src/preview/ServerBrowser.ts apps/server/src/preview/ServerBrowser.test.ts
node_modules/.bin/vp fmt --check apps/server/src/preview/SessionControl.ts apps/server/src/preview/SessionControl.test.ts apps/server/src/preview/ServerBrowser.ts apps/server/src/preview/ServerBrowser.test.ts docs/map.md docs/user/browser.md
git diff --check
bash scripts/fork-check.sh --upstream upstream

81 tests passed; server typecheck passed; scoped lint passed with one existing no-control-regex warning; formatting, diff check and fork-stack checks passed. No repo-wide checks. Environment: Linux, available Node 26.8.2 (repository requests Node 24), pnpm 11.10.0, Vite+ 1.0.0. No version bump per repository policy. No website runtime/SEO impact; this changes the shared server browser controller. Independent exact-head review passed with no blocking findings: #628 (comment). Verified review/independent=success, posted by lukemaj, on the exact candidate SHA.

Elon record

Requirements and who asked: Lukasz requested sticky human control, passive read safety, failing-before proof and independent review for #627.
Deleted: Automatic release on viewer disconnect; no new mode, timer, preference, retry loop or browser setup.
Bottleneck: The control owner conflated a disconnected human with agent permission to resume.
Checked myself: Reproduced both baseline failures, traced passive reads and watch paths, then ran 81 focused tests, server typecheck, scoped lint/formatting and fork-stack proof.

Model: GPT-6.1 Sol. Harness: Codex through T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Oct 26, 2026
@lukemaj

lukemaj commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent review found no blocking findings at 7c6abcdf82a80c0468c4329707cb893dfbcb4b3a for Issue #627.
Why: Disconnect preserves a human reservation, and passive reads and viewer reattachment do not grant agent control.
So what: Record review/independent=success on this exact head; merge and installation still require human approval.

Reviewed head: 7c6abcdf82a80c0468c4329707cb893dfbcb4b3a.
Reviewed base: 3ec7a5dd84753d18902a39ca3390f95879b25528.
Verdict: PASS, no blocking findings in the inspected scope.

Read Issue #627 and both investigation/handoff comments in full, repository AGENTS.md, the complete six-file diff, and surrounding consumers under the operations verification and code-review procedures. Independently traced the following:

  • Disconnected reservation and drains: SessionControl.ts changes ownership to the disconnected-human symbol immediately, advancing the generation and invalidating queued work. The existing serial tail drains running/tracked work and releases held keys/buttons before a replacement human's queued work can act. Agent admission continues to reject this non-null owner.
  • Authorization and competing humans: A failed replacement takeover restores the previous reservation only if that takeover still owns the same generation. A later takeover, disconnect or close cannot be overwritten by rollback. Another viewer cannot take, release or disconnect a connected controller. A disconnected reservation permits an authorized human to explicitly take control, then release it; stale control/action messages retain their generation fences.
  • Passive reads and reconnect: ServerBrowser.ts routes the opener's snapshot/wait reads through observation while human-owned, preserving authorization and login/dialog restrictions. Status does not change ownership. Screencast pause/resume does not release control. Reattaching a viewer neither clears the reservation nor automatically takes an agent-opened tab. The stream's unclaimed value describes the absent connected controller; automation status still reports human ownership. Existing web and mobile controls allow explicit Take control in that state.
  • All controller consumers and cleanup: Inspected SessionControl admission, generation/ref invalidation, input budgets, viewer finalization, status, downloads, file choosers, agent close/end-login/dialog guards, settlement and tab teardown. The symbol remains non-null for authority checks and settlement protection, and is not serialized. Closing a controller rejects subsequent actions rather than reopening authority. Existing idle eviction and browser/tab destruction remain lifetime boundaries, not persistence guarantees.
  • Watch fences: Retained reads use PageWatchStore, not the live control queue; stop does not acquire control. Start still requires agent control. Generation changes terminate active watches, and retained reads/emits recheck their existing ownership/navigation/login/authorization fences. No watch path inspected restores agent authority.

Scope is appropriate: two existing controller implementation files, focused regressions, the browser guide and the existing feature-map reference. No new component, mode, timeout, wire contract, provider-specific path, version change or unrelated implementation was added. The change supports shared browsing for the current Objective.

Proof limits: This was a read-only source review; I ran no tests, launched no browser, made no repository changes and did not touch the live install. I inspected the regression assertions and the author's exact-head proof record: baseline owner/snapshot failures, 81/81 focused tests, server typecheck, scoped lint with one pre-existing regex warning, formatting/diff and fork checks. Those results are author-reported, not independent reruns, and are not a complete repository gate. The broker/viewer regressions use the existing fake Chromium/CDP boundary. The original Messenger renderer/transport disconnect trigger was not observed live, and actual browser/client behavior, browser-host recreation and restart persistence are not live-verified by this review.

Elon record:

  • Requirements and who asked: The requester requires independent exact-head review of Preview tab control returns to agent while a human is using it #627's sticky human control and passive-read/reconnect safety.
  • Deleted: No implementation, extra agents, test reruns, browser launch or expanded design was needed for this source review.
  • Bottleneck: Verify that disconnected ownership cannot become agent authority through surrounding lifecycle consumers.
  • Checked myself: Read the exact diff and ownership/input/authorization/cleanup/read/watch consumers; verified local and live PR head/base identity and the clean shared workspace.

Reviewer: GPT-6.1 Sol, Codex through T3 Code; independent of the authoring turn.

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: 3ec7a5d · PR result: 7c6abcd · Source CI: cancelled

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preview tab control returns to agent while a human is using it

1 participant