PR #16941 adversarial review, ROUND 2 (Opus, read-only)
https://github.com/pingdotgg/t3code/pull/16941
Head repo/branch: hogeheer499-commits/t3code fix/preview-timeout-isolation-16921
Original contributor head: 102deab30a01e955345c4f8f9bcdb2e97d2d5d31 (ancestor of HEAD; contributor commits preserved)
Reviewed head: e7bc457af8bb799b28ceec39f127fbf6f930120b (/home/ubuntu/wt/terminal-preview-21, unpushed)
Base: origin/main 5569a42bff98f06b297115542b25c942f8607c16 (merge-base; main merged in 30354ec5d6)
Scope: full diff origin/main...HEAD (4 files: ServerBrowser.ts, SessionControl.ts and their tests). No other PR reviewed.

VERDICT: merge (after the primary publishes e7bc457af8 and CI/bots are green on that head).
No confirmed findings this round. The r1 medium finding is fixed with no new regression.

------------------------------------------------------------------------------
R1 finding status
------------------------------------------------------------------------------
Opus r1 Finding 1 (medium): viewer input on user-opened tabs stalled behind a tracked initial navigation.
FIXED. ServerBrowser.ts:902-903 now calls track() only when tab.control.agentId !== null; the user-tab
branch is byte-identical to main (`void tab.initialNavigation.catch(constVoid)`). SessionControl.agentId
is readonly and comes from snapshot.automationOwner, so a user tab can never switch branches later.
The ensureTab callers that reach this code are: agent open (automationOwner set) and its mirrored
"opened" event (deduped through pendingTabs); user open via mirrorManagerEvent (owner null); findTab
lazy recreation; desktop reattach and adopted popups (PDF only; agent popups inherit the opener's
owner). Every agent-owned case gets the same handoff rule, and every user-owned case matches main.

Sol r1: no findings; nothing to recheck.

------------------------------------------------------------------------------
Fresh full pass. Checked and correct
------------------------------------------------------------------------------
- Receipt deadline: handleRequest computes performance.now() + request.timeoutMs once, and the
  timeout is monotonic. The broker always sets timeoutMs (in-process host), and the host deadline
  falls before the caller's timeoutMs + 1s grace.
- SessionControl.action ordering: assertOpen, then epoch/permission (BrowserControlInterrupted wins),
  then the deadline (>=), then run(). The deadline is checked only before run(), so started work keeps
  its full normal budget, and the tail reservation holds until it settles. Declining the historical
  CodeRabbit "cap navigation budget" suggestion is still correct under the started-work rule.
- Queue subtlety (pre-existing, but it matters for the fix): enqueue reads `pending` only when the
  previous action settles. A track() on an idle queue therefore gates the second action, not the
  first. For an expired agent open this is exactly right: track() runs in createTab, the open action
  is then enqueued and rejected at dispatch, and its settlement chains the tail onto pending. So the
  next take()/human input or agent action drains the initial navigation, which matches main, where
  the non-expired open awaited initialNavigation inside the queue. The new user-tab test depends on
  this: auto-take runs first on the idle tail, and in r1 the correction then stalled.
- Agent retry after an expired open with a small timeout (< 15s): the retry can wait behind the
  initial navigation (<= NAVIGATION_TIMEOUT_MS) and expire at dispatch. Main is no better: the
  stale open kept the queue for initial navigation plus waitForLoadState, then ran late. Not a
  regression.
- recordingStop has no deadline, but ownership and queue checks still apply. recordingStart passes
  the generic deadline, and tab.recordingStart is set inside run(), so an expired queued start never
  begins capture. The comment at ServerBrowser.ts:1859-1860 is accurate.
- Unchanged routing: status (unqueued), non-owner reads via observe(), close and dialog (outside the
  queue), human()/system() (no deadline), viewer navigate/history/reload via trackUntilHandoff
  (same as main). Desktop-runtime tabs use the same core.
- Error mapping: DOMException "TimeoutError" maps to PreviewAutomationTimeoutError. The new message
  "expired before it could start" is accurate for both queue wait and pre-dispatch time.
- Tests (PR adds 5; 65 total). Each guards a distinct behavior: the queue rule unit test, the
  broker-level click that never reaches the page, the late recording stop after a caller timeout,
  the expired open that drains before human input, and the new user-tab correction dispatching
  before the initial commit. The new test is a real regression guard. The red log
  (16941-r2-viewer-red.log) shows r1 code leaving the last goto at /stalled; main and head pass.
  It holds the gate through a finalizer and joins only after release, so cleanup cannot deadlock.
  I request no further coverage.
- makeContext goto hook (`initialNavigation`) defaults to null and is reset in beforeEach, so it
  has no effect on other tests.

------------------------------------------------------------------------------
Nits (non-blocking, optional)
------------------------------------------------------------------------------
- ServerBrowser.test.ts (user-opened tab test): one setImmediate tick is used as the "correction had
  a chance to dispatch" barrier. This is deterministic today: Effect 4.0.5's scheduler uses
  setImmediate (FIFO), and the viewer input path is microtask-only. A Deferred resolved from the
  goto mock on the /fixed call would make it independent of the scheduler. Passed 15/15 isolated
  reruns plus the full suite. Not worth churn unless it flakes.
- Carried from r1, unchanged and cosmetic: the top-level describe in SessionControl.test.ts.

------------------------------------------------------------------------------
Pre-existing / out of scope (not regressions)
------------------------------------------------------------------------------
- Pre-existing: a broker timeout disconnects the shared host (PreviewAutomationBroker.ts:657-658)
  when updateCurrentTab !== false. Another request's timeout can fail a caller early. The live
  proof avoids this with updateCurrentTab:false, and it is disclosed as a service/browser
  integration proof, not a full MCP/client proof.
- Native coverage gap: macOS/Windows/iOS/Android/Electron were not verified. This is server-only
  dispatch code, and no client, contract or native code changed. This is a verification limit,
  not a defect.

------------------------------------------------------------------------------
GitHub feedback (16941-feedback-r2.json)
------------------------------------------------------------------------------
The remote head is still the original 102deab30a. There are no new reviews or bot findings since
r1. The latest comment is the maintainer takeover note. Checks on the old head: CodeRabbit and
Macroscope Correctness passed, and Approvability is NEUTRAL (human approval gate, not a source
finding). Nothing exists yet on e7bc457af8, so CI and bots must be rechecked after the push.
Body draft (16941-body-draft.md) matches the head's behavior. It still has the REVIEW_PENDING and
PROOF_KIT_PENDING placeholders to fill.

------------------------------------------------------------------------------
Verification performed / limits
------------------------------------------------------------------------------
- ./node_modules/.bin/vp test run apps/server/src/preview/SessionControl.test.ts
  apps/server/src/preview/ServerBrowser.test.ts -> 2 files, 65/65 pass at e7bc457af8.
- 15 consecutive filtered reruns of the new and changed ServerBrowser cases: 15/15 pass.
- Read the primary's r2 lint/typecheck/test logs (lint clean; typecheck shows only unrelated
  suggestions) and the red log. I did not rerun lint/typecheck, apply my own mutants, or start a
  browser or server; that was out of scope, and live proof belongs to the primary.
- Worktree left clean. No edits, checkout, commits, pushes, comments or delegates.
