fix(browser): keep generated sessions reachable after scratch cleanup - #6083

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 09dd326dfc5c4f643070f84dd1fb11e152c4d432 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound partial fix, but it trades unbounded orphan daemons for unbounded per-spawn lifecycle directories in the persistent data home, with pruning deferred to the same open issue.

Watch

• Every agent-process spawn mints a fresh kc-<8hex> session and creates <data-home>/pw/<8hex>/{s,d} before any browser command runs; nothing ever deletes them ("safe connect-test-then-prune remains part of Agent browser daemons are never reclaimed: socket dies with the per-process scratch dir (~10.7 GB stranded, oldest 4 days) #5986"). Unlike scratch, the data home has no reclamation, so a busy host accumulates dir pairs forever — the fix for a leak ships its own leak, just cheaper. At minimum, create the dirs lazily-per-spawn only if this is bounded, or land a trivial empty-namespace sweep with it.
• The enable gate greps exact byte snippets (process.env.PWTEST_SOCKETS_DIR ||) in two named playwright-core files. A routine Playwright upgrade that refactors or re-bundles those files silently reverts sockets to scratch — the 10.7 GB failure mode returns with only a log warning, and no test goes red because the fixture fakes the tree. The fail-back is correct; the detection needs a canary against the real installed package or a surfaced (not just logged) signal.

Suggestions

• A false-positive grep costs only two unused dirs (behavior identical to fail-back), while a false-negative reinstates the leak — the asymmetry argues for a looser presence check (or unconditional injection) over exact-snippet matching in pinned files.

[DESIGN-REVIEWED] 09dd326

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 09dd326dfc5c4f643070f84dd1fb11e152c4d432 via the fork AI-review pipeline; updated in place on each push.

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 09dd326dfc5c4f643070f84dd1fb11e152c4d432 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All the reused mechanisms check out in the base tree (_cli_package_dirs, _manifest_for_cli_package, make_owner_only_dir/restrict_dir_to_owner, _to_thread_guarding_sandbox in both spawn paths), and PWTEST appears nowhere in base, so nothing is duplicated. The module's two existing operator-override siblings (cli_env_overrides, browser_session_env) both practice a hands-off doctrine the new code diverges from. Here is the review.

First-Principles-Verdict: CONCERNS

The reachability fix earns its place; the operator-root rewriting and the upstream source-scan are extra surface whose zero options cost nearly nothing.

What this change ships

Intent: keep an agent's browser daemon closeable after its scratch directory is reclaimed — a FIX.

1. Generated browser sessions survive scratch cleanup and stay closeable via the CLI — justified (the fix)
2. One agent's playwright-cli list no longer shows peers' browsers — declared, justified
3. Operator-set PWTEST roots are silently rewritten to a per-session subfolder — contradicts the module's own override doctrine
4. Relative operator roots refuse the whole feature — exists only because of item 3
5. Every spawn scans installed playwright-core sources to verify the env hooks — oversized
6. Two owner-only dirs created under data-home per agent spawn — justified (untrusted-agent boundary)
7. New public socket_dir/daemon_dir helpers — one consumer each
8. Spec section in browser.md — mandated same-commit

Watch

• Item 3 inverts a doctrine with 2 counted siblings in the same module: cli_env_overrides (launch.py:147) and browser_session_env (launch.py:188) both return {} when the operator set the variable. Rewriting an operator's PWTEST_SOCKETS_DIR to <root>/<8hex>/s changes what their setting means, and item 4's validation/rejection surface exists solely to support that rewrite.
• Item 5's zero option is byte-identical behavior today: current playwright honors the vars, and after a hypothetical upstream rename the fail-back outcome (sockets back under scratch) is the same with or without the check — the ~60 lines plus lru_cache buy one warning line.

Subtractions

• Replace the base-namespacing branch in browser_socket_env with the sibling doctrine: operator-set SOCKETS_ENV/DAEMON_DIR_ENV → return {}. Deletes the absolute-path validation, the relative-root rejection, and 2 tests; reachability is still fixed (the operator's root is outside scratch by their own choice).
• Defer cli_lifecycle_env_supported + _source_contains (install.py, ~60 lines, 3 tests) to when an upstream rename is observed; inject unconditionally.
• Underscore-prefix socket_dir/daemon_dir — 1 consumer each (browser_socket_env), no external caller.

[FIRST-PRINCIPLES-REVIEWED] 09dd326

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 09dd326dfc5c4f643070f84dd1fb11e152c4d432 via the fork AI-review pipeline; updated in place on each push.

Both findings on 3fa1d31 addressed in 029d2c7.

• platform_compat.py — PID identity released before the group signal — FIXED (Linux), residual documented (macOS).

  Daemon group exits and its ID is reused after token verification -> orphan sweep signals the unrelated replacement group.

  The window was real, and on Linux it is now closed rather than narrowed: kill_self_led_group_pinned opens a pidfd on the target before validation and holds it across killpg. A pidfd holds a reference to the process object, so the kernel cannot recycle that PID — and therefore cannot create a new group under that id — until the fd is released after the signal returns. A process that vanished before the pin is refused at pidfd_open without signalling anything. Three new tests pin the pinned path, the vanished-process refusal, and the no-pidfd fallback.

  Fix: Revert the group-kill reaper until signaling remains tied to validated identity.

  Pushing back on the revert specifically, for the macOS / pre-5.3-kernel fallback that remains token-guarded: POSIX offers no stronger primitive there — there is no pidfd equivalent and no handle to hold, which is precisely the residual kill_process_tree_pinned's own contract documents ("POSIX is deliberately untouched … the residual probe-to-signal window there is the pre-existing one"). Every existing kill path in this module and in session_pid's orphan sweeps carries the same in-process check-to-signal residual; the fallback here re-reads the start token immediately before the signal, which is the tightest guard the platform admits. Reverting would trade a microsecond-scale, platform-irreducible window (already narrower than the shipped sweeps') for reintroducing a measured ~10.7 GB standing leak.

• session_pid.py:1500/:1623, browser_cli/reap.py:192 — function-local imports — FIXED. All three hoisted to module scope. Verified first that no cycle exists: browser_cli/__init__.py is docstring-only, and sel's only package imports are platform_compat / atomic_write / config.paths, none of which reach session_pid or acp.

All findings on 029d2c7 addressed in 8c7de92.

GPT — BLOCKING reap.py:171 — token captured after the argv/age snapshot — FIXED.

Old daemon exits after the caller reads argv/age and its PID is reused by a detached marked process with a dead scratch owner -> stale daemon data validates against the replacement token.

Correct: the mixed-generation snapshot was the gap. Fixed along the suggested shape — the kill phase in kill_orphan_mcps now captures the start token BEFORE its cmdline read, and kill_browser_daemon(pid, cmdline, age, start_token) takes that pre-read token as a required parameter instead of deriving its own. Every identity read now postdates the token, so either the data describes the pinned process, or a pre-token recycle means every read consistently describes the replacement — which must then satisfy the full predicate (including its own fresh age) on its own merits; a post-token recycle flips the pinned re-check in kill_self_led_group_pinned and refuses. The wiring test asserts the exact pre-read token reaches the kill, and a missing-token call refuses before any read.

First Principles — Watch: description claims a lazy import the diff does not have — FIXED. The import was hoisted to module scope in the previous round at GPT's own advisory; the PR body's "What changed" is updated in place to describe the module-level import.

First Principles — Subtraction 1 (drop the sig parameter) — FIXED. kill_self_led_group_pinned(pid, expected_start_time) now hardcodes SIGKILL; the one call site and tests updated.

First Principles — Subtraction 2 (drop the double predicate evaluation) — FIXED. The kill phase routes on the read-nothing cmdline_is_browser_daemon shape check; the full environment-reading predicate runs once, inside kill_browser_daemon. This halves the per-kill macOS ps spawns as suggested.

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

All concerns on 79563d94 are dispositioned in f03ae2d0.

GPT BLOCKING — shared roots expose browsers across chat families — REBUTTED as pre-existing, with a narrowing change retained.

Two generated sessions in one workspace -> approved playwright-cli list reveals the peer name -> -s=<peer> reads or controls its authenticated browser.

That authority exists on origin/main before this diff. Playwright's default registry is user-global (~/Library/Caches/ms-playwright/daemon on macOS / ~/.cache/ms-playwright/daemon on Linux), and current Kiro Crew explicitly documents it: config/prompt.md:144 says playwright-cli list shows other sessions' browsers; builtin_skills/web-browse/SKILL.md:157-158 says it shows every browser on the machine. A same-UID agent can already learn a peer kc-* name and pass -s=<peer>, or supply arbitrary PWTEST roots. This diff adds no close/signal/socket-read authority.

The implementation nonetheless narrows ambient visibility: generated sessions now receive separate <base>/<8hex>/s and <base>/<8hex>/d roots, so an ordinary list in one family does not enumerate peers. Focused independent review confirmed that scanning the common parent or peer environment uses the same pre-existing same-UID authority and is not a diff-introduced regression blocker.

Design Watch — upstream test hooks may disappear silently — FIXED. The active @playwright/cli package (first/most-specific package resolved through the existing launcher anchor) is now checked before injection. Its serving playwright-core must contain the exact execution snippets process.env.PWTEST_DAEMON_SESSION_DIR and process.env.PWTEST_SOCKETS_DIR ||. Reads are size-bounded and cached by path/mtime/size. Missing/renamed hooks warn and fail back. A stale standalone package cannot satisfy a different active installation; positive, missing-hook, and stale-fallback tests pin this.

Design Watch — permanent small-file growth — ACCEPTED-AND-DEFERRED to #5986. Per-session namespace directories can remain after a crash. Safe connect-test-then-prune requires the same cross-process ownership decision this PR deliberately avoids; adding unattended deletion here would reintroduce the withdrawn authority problem. The limitation and follow-up are now explicit in browser.md and #5986 remains open.

Additional correctness finding from focused review — relative configured PWTEST roots — FIXED. Relative roots are rejected before directory creation because gateway and agent CWDs can differ. Absolute configured roots are treated as bases and namespaced per generated session.

The blocking caller-controlled-root finding is fixed in 09dd326d.

App cron PWTEST_*=/home/user/.ssh -> extra_env -> ACP spawn -> gateway creates/chmods directories under .ssh.

Correct. The prior code passed the fully merged child environment — including caller/app/cron extra_env — into browser_socket_env, giving caller values authority over gateway filesystem preparation.

Both ACP spawn paths now construct lifecycle input as exactly {**os.environ, **browser_env} and call the helper only when browser_session_env generated a kc-* name. Caller extra_env is excluded from directory selection. The trusted lifecycle paths returned by the helper are then applied after caller env merging, overriding caller PWTEST values in the generated-session child environment. Non-generated operator sessions skip gateway lifecycle preparation entirely and retain their explicit environment.

A behavioral test constructs AcpClient with caller-controlled PWTEST roots and asserts neither reaches the lifecycle helper, while the generated session name does. The shared source-wiring test pins the trusted merge in both AcpClient and AcpRuntime. Both paths remain under _to_thread_guarding_sandbox for cancellation cleanup.

Focused independent re-review: blocker CLOSED, no new blocker found.