fix: remove the races behind the e2e flakes and the product bugs they exposed

[addiberra]

Purpose

Make the e2e suite trustworthy again. In a sample of the last 60 CI runs, 25 had at least one test that only passed on retry (44 retried results in total). A few tests also failed reliably on macOS. This PR:

• removes the timing assumptions behind those flakes;
• fixes the product races that several of them exposed; thirteen of these bugs are in v0.7.0 (listed below);
• restructures CI so e2e runs in parallel jobs, well within the 30-minute limit, and runs the unit suite in parallel as the required gate.

Addresses #253 and #233; partly addresses #438 (see "Known limitations").

What changed

Product fixes (all present in v0.7.0, so visible release notes)

• Personal state (src/shell/selection.ts): an idle open browser re-saved its own document on every live frame. A later browser then resumed whichever client saved last, not the document the user last picked. The document is now saved only when the selection moves or the user activates a document.
• Boot freshness (src/shell/events.ts, src/shell/recovery.ts): boot applied /api/state without recording its freshness, so the first live frame was accepted whatever its age. Through the hub, a page joining an already-open upstream (a second tab, or the upstream a reload leaves lingering for 3 s) receives that upstream's retained last frame, produced before boot's fetch, and re-applied older roots and repositories. Boot now records the snapshot's freshness and older frames are refused. A unit test drives the real module: boot at 2000, a frame at 1000 is refused, a frame at 3000 applies.
• Chat inventory (src/hub/live-broker.ts): a page that joined an existing inventory subscription (a second tab, or a reload within the linger window) never got the opening reconcile tick. So a conversation created between the page's list fetch and its subscription stayed missing from the chooser.
• Hub switcher (src/shell/hub-nav.ts): opening the menu starts a background refresh that rebuilt every entry. A click or Enter landing mid-rebuild was lost, and keyboard focus fell off the row. Entries are now kept in place when unchanged, and focus follows a replaced entry.
• Layout chooser (src/preview/layout-toolbar.ts): every render rebuilt the toolbar, so a click whose mousedown and mouseup straddled a render was dropped. The toolbar is now updated in place.
• Document loads (src/server/render-dispatch.ts, src/preview/load-retry.ts):
  • /api/document answered every render error with 404, and the client showed "File unavailable" permanently.
  • Real not-found is still 404 and binary is 415; other failures are now 500.
  • The preview retries 5xx or network failures after 250 ms, 1 s, 3 s and 10 s.
• Outline (outline-headings.ts): the scroll-spy trigger line sat above the scroll-padding that navigation lands headings at. So after clicking an outline entry, the highlight moved to the previous heading.
• Project search (search-model.ts): the results list was rebuilt on every stream chunk, including identical ones, which dropped keyboard focus from the selected hit. Unchanged markup is now kept, and a changed render refocuses the same hit.
• Touch tab bar (src/shell/tab-bar.ts, src/terminal/panel.ts): the arrow-key handler focused the target tab and then clicked it, and a click on the Terminal tab asks the terminal panel to focus its pane once one attaches. So keyboard tab navigation dropped focus into xterm whenever a pane attached in that window, and the next ArrowRight typed into the shell instead of moving to Files. Arrow/Home/End now activate the tab with holdFocus, which the panel honours on its show, attach and reveal paths; taps, clicks and Enter/Space still focus the terminal. A unit test drives the real tab bar and panel against a linkedom DOM and fails on the old code. initTabBar also returns a teardown now, so that test does not leave the module measuring a stale element for later suites in a serial run.
• SSH-agent guardian recovery (src/hub/credential-ssh-agent.ts): the guardian obeyed any well-formed stop on its control socket before the client had checked the reply's signature, and recovery sent stop straight from the record on disk. A recovery with a wrong nonce therefore failed closed as intended but still shut the real guardian down and removed both sockets. Recovery now sends a harmless status first; its signed reply proves the record belongs to this guardian, and only then does it send stop. The test no longer needs to win a race.
• serve shutdown (src/cli.ts): the session child printed its ready URL, the hub's ready signal, before installing its SIGINT/SIGTERM/SIGHUP handlers. A SIGTERM in that gap took Bun's default action: exit 143 and an orphaned OpenCode child. The handlers are installed before the URL is printed. Reproduced 10/10 with a SIGTERM sent on the URL line; 20/20 clean exits after.
• Hub state lease (src/hub/state-dir.ts): first-time lease acquisition opened SQLite with busy_timeout=0, and any SQLITE_BUSY was reported as "Hub state root is already in use". When several hubs started on a fresh state directory, the contenders' brief schema-setup locks (PRAGMA journal_mode, CREATE TABLE) made every one of them fail, so none started. Each attempt now uses a short busy timeout and retries a busy within a 500 ms window; only a lock still held after that counts as a live owner. A live owner is still refused (now after up to about 0.5 s instead of instantly). Reproduced 1 in 50 in the test loop and 4 in 600 with six contenders; 0 in 100 and 0 in 300 after. A deterministic test holds the lease file mid-setup and fails on the old code.
• Failed-shutdown lease (src/hub/main.ts): after a failed shutdown the hub reports that it keeps the state-root lease until the next signal, but its server is already stopped, so nothing held the process open: it exited 0 and the OS released the lease a successor could then take. createHubSignalShutdown now holds the process open while it retains the lease. Reproduced by delaying the test's second signal; two new unit tests.
• Hub log (src/hub/live-broker.ts): when every topic on a child fails at once, the hub logged one "live upstream … subscription failed" line per topic. Repeats per topic and status within 60 s are now folded into one summary line; the first line and the failure metric are unchanged.

Each fix has colocated unit tests, and most have a deterministic e2e reproduction that fails without the fix.

E2E harness

• Git scope: the e2e workspaces live inside the checkout, so every watcher refresh ran git over the whole uatu repo. GIT_CEILING_DIRECTORIES stops that. Git time per refresh fell from 310 ms to 11 ms median at idle, and from 4.3 s to 65 ms under load. It also fixes CI's "stdout maxBuffer length exceeded" and stops tests depending on the developer's working tree.
• Production bundle: the e2e server used Bun dev/HMR mode. Bun's <bun-hmr> overlay blocked clicks after a child stopped, and editing src/ reloaded open test pages. Pages are now served as a production bundle, left unminified because three tests patch served source text. Warm page load went from about 0.9 s to about 0.77 s.
• One browser per engine per worker: the six files that launched their own Chromium or WebKit per test (27 sites) now take a launchBrowser(engine) fixture from page-diagnostics.ts. Chromium is Playwright's worker browser; WebKit is one worker-scoped browser launched on first use. Each test gets fresh contexts, closed even after a timeout, and the failure diagnostics still attach. A self-launched WebKit's first page took 11–25 s under 4-worker load. This removed the launch cost but not the flows' own length (see the slow flows under CI below).
• Deterministic PTY shell: test terminals run zsh or bash with a private HOME/ZDOTDIR and a fixed prompt, instead of the developer's $SHELL and rc files. TERM_PROGRAM and TERM_SESSION_ID are stripped.
• Port allocation: ports now come from the worker's parallelIndex slot, not workerIndex, which grew with every worker restart until hub ports reached 4700 (a real uatu hub's default port). Hubs now use 21000+ and harness servers 20000+. Test servers exit when their worker's stdin closes, so a replacement worker can't collide with an orphan. Linked-worktree children are limited to their hub's port block.
• Per-test hub reset: hub-served tests no longer inherit state from the previous test on the worker.
• Shared helpers:
  • openTreeFile waits for the tree to be idle, then clicks and asserts the preview. Folder toggles in manual-selection hit-test the click point after the tree settles, because a Playwright-driven scroll re-renders the virtualized rows and the resolved button can become a different row.
  • terminal-helpers.ts provides readiness via data-terminal-ready plus a sentinel round trip, and file gates.
  • openHubMenu waits for the menu's refresh.
  • /__e2e/terminal-close-delay and /__e2e/terminal-sessions-delay are holds the test releases, not timers.
• chat-touch drill-down intermittent (retried 5 times in 6 CI runs): a test race, not a product bug. The test tapped the answer option while the drill-down timeline was still scrolling to its end; Playwright's retry of an unstable click scrolls the target into view, which the app correctly read as the reader scrolling away, so it stopped following. The test now waits until the timeline is at its end and still. 6 failures in 57 before, 150/150 after at 8 workers.
• notification-presence.e2e.ts (added on main during this work): its grace-period sleeps now wait on the hub's notification journal for the actual decision (held, discarded or sent). This also fixed a latent bug where pushes from one test matched another test's conversation ("conversation-1" matched "conversation-10"); each test now enrols its own device. 40/40 after; the file runs in about 10 s.
• Fixed sleeps replaced with the conditions they stood in for: watcher latency (fs.utimes + 800 ms), sleep 1 in typed shell input, route stalls, and "nothing happened" waits, which now await the positive event before asserting absence. standardBeforeEach now ends on real conditions, and the preview-settle check fails loudly instead of falling through.

CI, the perf project, and parallel unit tests

• validate is split into:
  • changes
  • unit, which now runs bun run test:ci (bun test --parallel=4 with committed per-file timings so the slowest files start first; about 70 s locally against about 155 s serially). It ran as a non-required shadow job for runs 5–8 while the real-process tests that failed only under parallel contention were fixed (below), and became the gate after 10 clean and 5 CPU-loaded local runs all passed
  • an e2e matrix of 3 legs: --project=e2e leg 1 and leg 2 with 4 workers each, and --project=perf with 2 workers
  • e2e-report, which merges the blob reports into the HTML report on failure
  • validate, which always runs and passes only if every job succeeded, or all were skipped for docs-only changes. The required-check name is unchanged.
• Legs by file list, not --shard: --shard splits by test count, and the first CI run showed shard 1 at 18.0 min against 13.6. UATU_E2E_LEG=1 runs an explicit 34-file list; UATU_E2E_LEG=2 runs everything else, so a new file lands in leg 2. Unset, the whole suite runs. --list: leg 1 = 397, leg 2 = 452, e2e = 849, perf = 21, total 870; the two legs' union equals --project=e2e and no test is in both. A move of document-tree between the legs was tried and reverted: with it in leg 1 the legs ran 18.0 / 15.3 min, without it 11.0 / 15.9, and the stage waits for the slower leg, so the lower maximum won; the WebKit flows' time depends on what runs beside them, so worker-time sums under-predict this.
• Playwright browsers are cached.
• perf project: 21 @perf-tagged timing-budget tests (chat-follow-stability, the long-output shell tests, and hub-live-stream).
• The long-output test now passes on deterministic work counters: owners, resets, parsed characters, line writes and paints. Wall-clock time is reported against the old budget, and only fails the test at twice that budget (3 s → 6 s, and 1 s → 2 s for hub-live-stream).
• Slow flows: the 16 "full running scrollback flow" tests, the 2 "long loaded history" tests and the WebKit "item A and B geometry" test carry test.slow(). Blob-report timings from the second run show they are long, not racy: the Chromium scrollback flows pass at a 22.7 s median (max 23.9 s) against the 30 s default, the WebKit ones cross 30 s under four-worker contention and pass at 12.8 s on the quieter retry, the geometry test passes at 29.7 s on WebKit and the history test at 23 s. worktree-ui (90 s ×7), worktree-integration and chat-follow-stability already extend their budgets the same way. Nothing is skipped or loosened; a hang still fails.
• Traces: the e2e project uses retain-on-first-failure; perf keeps on-first-retry.
• Browser-bundle guard (src/shared/browser-bundle-discipline.test.ts): walks the page's static imports from src/app.ts and fails if any reachable module imports a Node built-in, since only the compiled-binary smoke test would otherwise notice.
• Parallel-safe unit files: src/hub/main.integration.test.ts takes OS-assigned ports instead of 4795–4799. Tests with real-time bounds or races that failed when files share the cores now wait for the event they stood in for: a killed descendant being reaped, a fake mkfifo released by the test instead of sleep 1, the terminal server's close event, live chat events being applied, the recovered SSH-agent guardian's processes exiting; plus fake timers, wider admission windows, and explicit budgets for files that spawn real processes or real Git. A cron test tied with the every-minute wakeup at 20:02 daily. No test is skipped or excluded; the count is unchanged.
• Source-run build identity (src/shared/version.ts): every module importing it ran git twice through Bun.spawnSync at load. Under parallel tests that hit a known Bun bug where spawnSync loses a child's exit and spins forever (Bug: spawnSync never returns: child exit lost (child stays zombie), wait loop busy-spins at 100% CPU re-registering a finished pipe reader (macOS ARM64) oven-sh/bun#34069; fix spawnSync: release polls and keep-alives on the loop they counted on oven-sh/bun#40078 is unreleased), hanging a test worker. It now reads the branch and commit from Git's own files and asks git only for cases they don't cover. Compiled binaries inject their build identity, so released builds were never affected; this is not a release note.
• New scripts: test:ci, test:e2e:perf and test:e2e:no-perf. CLAUDE.md's stale test facts are corrected.

Verification

• CI runs on this PR (retries: 2, so "retried" means a test passed only on a retry):
  • Run 1 (bc620a8, first run of the new layout): green; 12 retried, all 30 s timeouts: 9 WebKit chat-shell-scrollback tests, chat-responsiveness long history, manual-selection touch multi-root, terminal-hub-navigation released-late. Shards 18.0 / 13.6 min; the browser cache missed only because it did not exist yet.
  • Run 2 (ac554fd, shared browser, leg split, parallel unit): green; 11 retried (the 8 WebKit scrollback flows, 2 long-history, the touch-tab focus test). Legs 14.0 / 16.5 min; parallel unit step 51 s (serial was 166 s); browser cache hit.
  • Run 3 (f88eb0a, slow budgets, tab-bar fix): e2e green with 1 retried (chat-touch.e2e.ts:813, a one-off); parallel unit failed one real-process test (a 5 s child-process budget, fixed).
  • Run 4 (7589535): e2e green with 0 retried across 440 + 409 + 21; parallel unit failed two different real-process tests, which led to the guardian and serve shutdown fixes above.
  • Run 5 (d30b1a4, serial gate + parallel shadow): e2e green with 0 retried (397 + 452 + 21), legs 11.0 / 15.9 min, unit-parallel green in 63 s; the serial unit job failed 9 chat/viewport tests with NaNpx, a module-state leak from the new panel test under CI's serial file order (fixed; see the tab-bar entry).
  • Runs 6 and 7 (d188a55 tab-bar teardown; 27d630b guardian, exit-wait and serve shutdown fixes): e2e green, 2 and 1 retried (all chat-touch.e2e.ts:813, below); unit-parallel failed on run 6 (the exit-wait race, fixed in run 7) and passed on run 7. The serial unit job failed on both with a second serial-only cause: an unhandled cancelAnimationFrame is not a function from preact between tests, which aborted search-model.test.ts (0 of 23 ran). events.test.ts (new here) stubs requestAnimationFrame on a linkedom window, which forwards to globalThis, then imports events.ts, which loads the tree and preact/hooks; preact decides once, at that load, that its effect flush may call cancelAnimationFrame. tree-view.test.ts stubbed rAF with a function that never fires, so a flush scheduled by its afterEach dispose ran on preact's 35 ms fallback after the file had deleted cancelAnimationFrame. Both suites now queue frame callbacks and flush them before restoring globals.
  • Run 8 (35202f4, animation-frame flush): serial unit green, 4,553 pass / 0 fail / 12 skip, no unhandled errors, full count restored; unit-parallel green; e2e legs green with 1 retried (chat-touch.e2e.ts:852, below), 396 + 452 + 21, legs 16.5 / 15.1 min; validate green.
  • Run 9 (4461e4a, the final round's fixes, first run with the parallel unit gate): e2e green, 1 retried (terminal-hub-navigation "explicit new-tab gesture", a one-off also seen once on run 1); the parallel unit gate failed one test: a timed-out SSH public-key read came back as a clean exit with no output and failed with the wrong message (fixed: readPublicKey now reports a timeout as a timeout, as its two sibling helpers already did).
  • Run 10 (37a8f68): e2e green with 0 retried (397 + 452 + 21) and all 4,561 unit tests passed, but the compiled-binary smoke test failed: the page never left "Connecting". The final round's version.ts change had added static node:fs/node:path imports to a module the page also loads; bun build --compile keeps them as real imports, the browser cannot fetch node:fs, and the page script failed to load. The e2e server's bundler stubs such imports, which is why all 870 e2e tests still passed. Reproduced 3/3 locally and bisected to that commit; never released. Fixed by looking the modules up at run time, plus a new test that walks the page's import graph from src/app.ts and fails on any Node built-in import.
  • Run 11 (eecf1b6): all green, including the parallel unit gate and the smoke test; e2e with 0 retried for the second run in a row (397 + 452 + 21, legs 16.3 / 16.0 min); unit job 94 s, its test step 52 s (the serial suite was about 155 s).
• Full e2e suite on the author's machine (macOS, 4 workers, retries 0): before rebasing onto current main, 850 passed, 0 failed, 0 flaky, in 12.9 min. After rebasing and fixing the port allocation: 863 passed, 1 failed, in 14.8 min; the failure was a 30 s timeout in one chat-shell-scrollback test that then passed 10/10 in isolation.
• Targeted before/after runs:
  • terminal-hub-navigation ×10: 21 of 110 failed before, 0 of 110 after (at normal load).
  • The document-tree follow-default pair: 8 of 24 failed before, 24/24 passed after.
  • The mobile terminal tests: 5 failures in 80 before, 96/96 after.
  • The files touched by the harness batch ×3: 30 failures in 1002 before, 1 after (then fixed); the final ×2 run passed 668/668.
  • manual-selection touch multi-root ×15: 14/15 and 19/20 before, 15/15 after. terminal-hub-navigation "released late" ×15: 15/15 after; the two files together 61/61.
• Unit tests: bun run test:ci passed 4,560 / 0 fail in two final runs (about 68 s), and in the gate-promotion stress series 10 clean runs plus 5 runs with 2–4 cores kept busy all passed. The 4 src/hub/credential-context.test.ts failures seen earlier on the author's machine came from the Hub-managed workspace's Git/SSH wrapper directory on PATH (CLAUDE.md warns about this); with it removed they pass. The CI-order replay that reproduced the viewport leak (per-file shims importing the real tests in CI's order) passes 376/376 with the teardown. The guardian and shutdown fixes: 20/20 alone and 5/5 alongside a parallel hub run.
• Checks: bun run typecheck is clean, and actionlint + shellcheck report no issues on the workflows.

Review rounds

• Round 1 (nine commits, a3439b3..e2fba83): /api/document 500 path logs the document and error; the load retry re-arms on a same-file selection via an activation counter; the outline scroll-spy caches scroll-padding-top; the switcher menu reconciles entries by identity instead of position; the e2e hub harness awaits a stopped child's exit and a free port before reusing it; a rejected hub shutdown exits instead of retaining the lease; a rejected WebKit launch is retried instead of cached; the chmod-based EACCES tests skip as root. CI run 12 green, 0 retried.
• Round 2 (1e99f3b..b13f83c): the SSH-agent recovery checks the recorded sockets before the status probe again (a missing or swapped socket fails at once with the precise error, not after a 2 s timeout); the switcher's fork control is keyed on data-fork-for=<id> so same-named checkouts can't swap focus; the source-run Git walk honours GIT_CEILING_DIRECTORIES; a replaced diagnostics sink retires its pending fold timers; a live frame's failed reload restarts the retry schedule. Two optional items (keep the last render while a same-document reload retries; answer EACCES/EISDIR with a final 4xx) are left for follow-up issues by agreement. CI run 13: all green (unit gate, smoke test, validate), e2e with 0 retried for the fourth run in a row (397 + 452 + 21; legs 16.3 / 11.4 min).

Known limitations and risks

• CI-only verification for part of the work. The author's VM had no display for part of the work, so the shared browser, the slow budgets, the leg split and the tab-bar fix's e2e path were first verified on the PR's CI runs; rendering was back for the final round, and the full local suite below includes them.
• Bun spawnSync hang: the known Bun bug above can still hit the remaining synchronous spawns in tests (execFileSync("git", …) in several hub integration tests, Bun.spawnSync in credential-context.test.ts and scripts/agent-coverage.test.ts). A proper fix needs a Bun release containing spawnSync: release polls and keep-alives on the loop they counted on oven-sh/bun#40078.
• Wall-clock budgets now have a 2× hard guard; time over the original budget is annotated rather than failing. This trades sensitivity for not failing on a slow runner.
• Unconfirmed root causes, retested on a working display with no product change; none appears in this PR's CI runs:
  • the ⌘G "1 of 4" flake (Two E2E tests fail on macOS but pass in CI #233 item 2): not reproduced in 382 runs, 60 of them directly after "finds matches in the rendered view";
  • WebKit stuck on "Connecting" (e2e flakes without a root cause: lost tree click in personal-state, WebKit stuck on "Connecting" #438): not reproduced in 168 WebKit runs;
  • a file click right after boot that never opens the file (e2e flakes without a root cause: lost tree click in personal-state, WebKit stuck on "Connecting" #438 mode B): not reproduced in 172 personal-state runs or in 4,440 cycles of a stress script built for it, but seen once in find.e2e.ts at load average about 90. No code path that drops the click was found; find.e2e.ts now attaches the page's request log on failure, which will show whether the click reached the app. On main, personal-state flaked in 4 of the last 10 runs, all mode A, which the selection.ts fix above addresses.
• Load sensitivity that remains: the hub fixture's sign-in can take 20 s under heavy load (a browser delay plus argon2), which eats most of a test's 30 s budget; leg 2 ran its 452 tests in 16.1 min on run 6 and 10.9 on run 7, so runner variance dominates the leg balance.

BEGIN_COMMIT_OVERRIDE
fix(shell): stop an idle browser from overwriting the document another browser picked last
fix(shell): refuse live updates older than the state the page loaded at boot
fix(chat): show conversations created just before a page subscribes to the chat list
fix(shell): keep workspace switcher entries and focus in place while the open menu refreshes
fix(preview): stop layout clicks from being lost when the preview re-renders
fix(preview): retry documents that failed to load instead of showing them as unavailable
fix(preview): keep a clicked outline entry highlighted after its heading scrolls into place
fix(sidebar): keep keyboard focus on a project search result while results update
fix(shell): keep keyboard focus on the tab when arrow keys move to the Terminal tab
fix(hub): stop a wrong-nonce SSH-agent recovery from shutting down the real guardian
fix(cli): handle a stop signal that arrives as soon as a session reports ready
fix(hub): start one hub, not none, when several start at once on a fresh state directory
fix(hub): keep the state lease held after a failed shutdown until the next signal
fix(hub): fold repeated live-upstream failure lines in the hub log into one summary
END_COMMIT_OVERRIDE

🤖 Generated with Claude Code

[tjakobsson]

@claude please review this PR. Focus on correctness of the product fixes (selection persistence, boot freshness, live-broker reconcile tick, hub-nav in-place refresh, layout toolbar, render-dispatch status codes + load retry, tab-bar holdFocus, SSH-agent guardian recovery, serve shutdown ordering, state-dir lease retry, failed-shutdown lease hold) and on whether the CI restructure keeps every test in exactly one leg. Flag anything that changes behavior beyond what the PR description claims.

[claude]

Claude finished @tjakobsson's task in 1m 55s —— View job

---

Review complete ✅

• Gather context (PR description, CLAUDE.md, changed files)
• Review personal-state / boot-freshness fixes (selection.ts, events.ts, recovery.ts, boot.ts)
• Review live-broker reconcile tick + hub-nav in-place refresh
• Review preview fixes: layout toolbar, load-retry, render-dispatch status codes, outline
• Review tab-bar holdFocus + terminal panel
• Review SSH-agent guardian recovery + serve shutdown ordering
• Review hub state-dir lease retry + failed-shutdown lease hold + live-broker log folding
• Verify CI leg restructure keeps every e2e test in exactly one leg
• Synthesize findings

I read each fix against git diff origin/main...HEAD for its files plus surrounding context, and ran the review in parallel across seven focused passes so each fix got a dedicated, skeptical look rather than a skim. Bottom line: I found no correctness bugs in any of the eleven product fixes, and the CI leg split is structurally sound. A few small nitpicks below, nothing blocking.

Product fixes — all check out

• Selection persistence (src/shell/selection.ts): the changed || origin === "navigation" guard is captured against the pre-mutation id, so watcher re-confirmations (default origin: "reconcile") no longer persist, while every other write site (follow.ts, anchors.ts, real selection moves) still does. No other write path to the persisted documentPath exists, and clearDocumentSelection correctly still persists unconditionally on close. selection-persistence.test.ts genuinely discriminates pre/post-fix behavior.
• Boot freshness (events.ts + recovery.ts): confirmed a single module-level watermark (appliedFreshness) backs boot, live frames, and the recovery/reconnect path — no divergence between paths. Comparison is strict-greater-than (ties correctly rejected), and recordApplied only ever raises the watermark. No async ordering gap: the live topic is only subscribed after boot's watermark is set.
• Live-broker reconcile tick (src/hub/live-broker.ts): joinerOwedOpeningTick correctly targets subscribers with cursor === undefined on an already-live upstream with seq > 0; attachLive fires exactly once per subscriber so there's no double-delivery, and refcounting is untouched by this hunk. One reviewer noted this also protects the very-first subscriber, not just late joiners — a superset of the described bug, not a regression.
• Hub-nav in-place refresh (src/shell/hub-nav.ts): the outerHTML+JSON signature is position-indexed but always embeds data-workspace-id/href, so two distinct entries can't silently collide; focus-key lookup happens post-reconcile against live DOM, guarded so it only refocuses when the previously-focused node was actually replaced. Refresh cadence is unchanged (verified by a test asserting exactly one extra /api/hub/state read).
• Layout toolbar (new layout-toolbar.ts): listeners are attached once at node creation; re-renders only call syncLayoutToolbar for attribute/class toggling, so there's no duplicate-listener or stale-closure risk. Test asserts DOM node identity across renders and a single click producing a single selection event.
• Render-dispatch status codes + load-retry: documentErrorStatus maps only the exact not-found/ENOENT cases to 404 and the binary case to 415; nothing else got silently reclassified (traced findDocument — no separate throw path for malformed ids). isTransientDocumentFailure retries only on null/>=500, never on 4xx, so a real 404 won't get a pointless retry delay and a genuine transient 500 will. The 250ms/1s/3s/10s backoff only engages on failure, so the common path's perceived load time is unaffected.
• Outline scroll-spy: spyTriggerOffset reads scrollPaddingTop live from the same scroll root the navigation code scrolls, rather than duplicating a magic number — more robust than a hardcoded constant, not just a fix.
• Tab-bar holdFocus: arrow/Home/End now pass holdFocus: true, click/Enter/Space don't; the flag is a plain call parameter never stored at module scope, so it can't leak across tab switches, and it's threaded through all of setVisible/onActiveTabChange/the pane-attach paths, including the async createSessionRemote gap the bug report describes. initTabBar's new teardown correctly removes all listeners and prevents the stale-element bug in serial test runs.
• SSH-agent guardian recovery: recovery now proves guardian identity via a signed status reply (using the same HMAC/nonce scheme stop relied on) before ever sending stop. A wrong-nonce recovery correctly fails closed and leaves the real guardian alive — the new test proves this by having the original guardian still start/shutdown cleanly afterward. No new TOCTOU beyond the pre-existing same-uid/0600-socket trust boundary.
• serve shutdown ordering: pure relocation of the signal-handler install to before the ready-URL print, with zero awaits in between and all closed-over handles (server, watchSession, etc.) already initialized by that point. New integration test sends SIGTERM at the exact moment start() resolves (i.e., right after the ready line), reproducing the real race.
• State-dir lease retry: the 500 ms retry window isn't a blind time-based guess — a genuine owner holds PRAGMA locking_mode=EXCLUSIVE, so it deterministically stays SQLITE_BUSY for its entire lifetime; the window only smooths over transient contention between fresh contenders racing through schema setup. No scenario found where a live owner could be misclassified as available under load.
• Failed-shutdown lease hold: holdProcess() fires only on stateLeaseHeld: true or a rejected shutdown(), never on the successful path (forceExit is unchanged there), and can only fire once given the state machine's transitions.
• Live-broker log folding: the failure metric increments unconditionally before the folding sink runs, so log folding never affects the metric; dedupe key is topic+status with independent windows, so a distinct failure type during a fold window still logs immediately.

Nitpicks (non-blocking)

1. render-dispatch.test.ts / routes.test.ts use chmod(filePath, 0o000) to force an EACCES for the new "unreadable file → 500" case. This is a no-op if the test process ever runs as root (root bypasses POSIX permission bits) — low risk on standard GitHub runners, but worth a process.getuid?.() !== 0 guard given this PR's flake-hunting theme. Fix this →
2. src/terminal/panel.test.ts covers the fresh-session (addPaneInteractive) path for holdFocus but not the attachSessionBatch path (existing orphaned sessions found via inventory GET). Code inspection shows the same reset-before-resume ordering protects it, so it's very likely correct, but untested.
3. src/shell/hub-nav.ts's forkButton click handler closes over the outer worktreeApi variable directly, which isn't part of the kept node's identity signature (outerHTML + JSON.stringify(entry.main)). If worktreeApi ever changed value while an entry's source stayed byte-identical, a kept/reused node would retain a stale worktreeApi. Currently unreachable in practice (worktreeApi is set once from the initial hub probe), so theoretical only.
4. In src/terminal/panel.ts, the pre-existing (not introduced by this PR) pane-restore loop inside setVisible calls addPane(record, { focus: false }) synchronously before the holdFocus-driven focusPaneWhenReady reset runs later in the same function. If a prior parked-focus intent were already pending, this loop could consume it regardless of the new holdFocus flag. Requires a fairly contrived prior state and predates this PR — flagging for awareness, not as something this PR needs to fix.

CI leg restructure

Structurally verified correct: leg 2's testIgnore is the literal complement of leg 1's hardcoded testMatch list (not a second independently-maintained list), so a new e2e file automatically lands in leg 2 as the PR claims. All 34 leg-1 filenames exist with no typos/dupes. The perf project's grep/e2e's grepInvert use the identical @perf regex, so no test can double-run across e2e and perf, and perf bypasses the leg split entirely (leg-agnostic by construction). validate's needs: [changes, unit, e2e] correctly fails if any of the 3 e2e matrix legs fails.

One caveat: the review sandbox couldn't execute bunx playwright test --list (no bash/network approval for that command), so the exact claimed counts (397/452/849/21) are unverified by this review — only the partition mechanism was confirmed sound, not the literal numbers. Worth a maintainer spot-check of bunx playwright test --list --project=e2e vs. UATU_E2E_LEG=1/=2 if that precision matters.

Not reviewed in depth

Given the PR's stated focus, this review concentrated on the eleven listed product fixes plus the CI leg split, per your request. The broader e2e harness changes (shared browser fixture, deterministic PTY, port allocation, per-test hub reset) and the ~30 individual e2e test file edits were not independently re-verified beyond what's needed to confirm they don't imply a product-behavior change.
· Branch: fix/e2e-flakes

[tjakobsson]

Thanks for this — the product fixes read well and hold up against their callers, and the CI restructure is a real improvement. I ran a separate review pass over the full diff (not only the eleven fixes) and found eight things worth fixing before merge. The first is a harness regression this PR introduces; 2–6 are gaps in the new mechanisms; 7–8 are minor.

Inline comments below. Please push fixes to this branch and I'll re-run the suite.

[tjakobsson]

Port pinning + non-awaited SIGKILL can restart a child on a port its predecessor still holds.

terminate() (below) resolves right after child.kill("SIGKILL") without awaiting the exit event. That was harmless before because every start took a fresh port; now this.ports.get(workspace.id) hands the replacement the same port. Sequence: resetForTest → stop → terminate(); the child's SIGTERM shutdown awaits singleAgentRouter.dispose() and watchSession.stop() (chokidar close is known to hang, see cli.ts) → 2 s later SIGKILL fires and terminate() resolves while the process is still dying → start spawns on the same port → EADDRINUSE → the child exits before announcing → reset reports error → every later test on that worker fails in hubReset.

Fix: make terminate() await exit after SIGKILL too (as hub-fixtures stopChild does), or waitForPortsFree before reuse.

[addiberra]

Fixed in 01082fc. terminate() now resolves only once the child has actually exited: it waits up to 2 s after SIGTERM, then sends SIGKILL and waits for the exit event (bounded at 3 s) instead of resolving right after the kill. As a second guard, start() calls waitForPortsFree([port]) before reusing a workspace's pinned port, so a dying predecessor can't cause EADDRINUSE.

[tjakobsson]

The new 500 branch swallows the underlying error.

A renderer throw or an fs read failure (EMFILE, EACCES) becomes { error: "document render failed" } with nothing on the server side. The client retries at 250 ms / 1 s / 3 s / 10 s and then shows "Select it again to retry", and the hub/session log has no trace of which file or why. The old 404 path hid the same errors, but this PR defines 500 as "the server failing", which is exactly the case that needs a console.error with the error and the document id.

[addiberra]

Fixed in a3439b3. Only the 500 path in /api/document logs: console.error("uatu: /api/document render failed for <documentId>:", error). 404 and 415 stay silent, and the response body is still the generic document render failed. routes.test.ts asserts the 500 case logs once with the document path and the EACCES error, and the 404 case logs nothing.

[tjakobsson]

"Select it again to retry" doesn't re-arm the schedule.

Attempts are keyed by selectionGeneration + documentId. Re-selecting the same tree row goes through setSelectedId(sameId, "navigation"), which leaves selectionGeneration unchanged, so failed(sameKey) finds attempts exhausted and returns false → the same dead-end message with no retry scheduled. The user has to pick a different file first.

Either call settle() on a navigation-origin activation, or fold a user-activation counter into the key.

[addiberra]

Fixed in d66b1bb, with a user-activation counter in the key (so selection doesn't import preview). setSelectedId(…, "navigation") now bumps an activation count even when the id is unchanged; a watcher reconcile doesn't. mount.ts keys the retry on selection generation + activation + document id. The new test in load-retry.test.ts uses the real selection module: it exhausts the schedule, checks a reconcile of the same id leaves it exhausted, then checks a navigation of the same id starts a fresh schedule at the first delay.

[tjakobsson]

Reconciling by position reintroduces the lost click on any list-shape change.

reconcileMenuEntries compares next[i] to container.children[i]. If the background /api/hub/state refresh inserts, removes or regroups one workspace above the row the user is pressing, every following entry's positional signature differs and existing.replaceWith(node) runs between mousedown and mouseup — the click is dropped. menuFocusKey recovers focus, but not the click.

menuFocusKey already derives a stable identity (data-workspace-id / aria-label / href); keying the reconcile on that (match by identity, then move/insert) keeps unchanged rows regardless of index.

[addiberra]

Fixed in 821d0de. reconcileMenuEntries now matches entries by stable identity (group headers by data-repository, then the same workspace id / aria-label / href menuFocusKey uses; dividers by signature; duplicate identities pair in order). An entry is reused when identity and signature both match, so an unchanged row keeps its node when rows are inserted, removed or regrouped above it; kept nodes already in order don't move (longest-increasing-subsequence pass), the rest are placed with insertBefore, and the children end in exactly the new order. New tests cover insert above, remove above, regroup, and a changed row with focus following it; the first three fail on the positional version. On the worktreeApi closure the other review raised: the fork handler reads worktreeApi at click time, and unsetting it changes the header's markup and so its signature; I added a comment rather than code.

[tjakobsson]

A rejected WebKit launch is cached for the worker's lifetime.

launched ??= playwright.webkit.launch() stores the promise even when it rejects (a transient spawn failure or browser-process timeout under load). Every later launchBrowser("webkit") on this worker awaits the same rejection, and Playwright's per-test retries can't help because the fixture never re-launches.

launched ??= playwright.webkit.launch().catch(error => { launched = undefined; throw error; });

[addiberra]

Fixed in e2fba83, as suggested: a rejected WebKit launch clears the cached promise (.catch(error => { launched = undefined; throw error; })), so the next test on that worker launches again. Teardown ignores a rejected or cleared launch and only closes a browser that actually started.

[tjakobsson]

retain on the rejection path holds the process with no proof the lease is held.

shutdownHub() never rejects (it catches internally and returns stateLeaseHeld), so a rejection here means the wrapper threw after the lease may already be released. retain() then sets up the 2^31-1 ms interval and reports a retained lease it doesn't hold, and the hub stays alive until an operator sends another signal (which force-exits 1).

Only the resolved stateLeaseHeld === true branch should hold the process; the rejection branch should log and forceExit(1).

[addiberra]

Fixed in 626f554. If shutdown() rejects, createHubSignalShutdown logs uatu hub: shutdown failed: … and calls forceExit(1); it no longer holds the process or reports a retained lease. Only a resolved stateLeaseHeld === true holds. The old test that asserted the hold on rejection is replaced by one asserting the log, exit code 1, and no hold or retained report.

[tjakobsson]

Minor: this assertSocket pair duplicates lines 289–290 with only the status round trip in between. Only this second pair is load-bearing (a missing socket makes request() fail anyway), so the first pair can go — or keep the first as pre-flight and drop this one. Two copies of the same guard drift when one is edited.

[addiberra]

Done in 71ccc09: removed the pair before the status probe and kept the one right before stop, with a comment on why it's there. One consequence worth knowing: that first pair was what produced the specific errors two recovery tests asserted (does not match its ownership record, ENOENT). Those cases now fail at the status probe with the generic SSH guardian request failed, so both tests expect that message and use a short stopTimeoutMs; they still check the ownership record and agent socket are left untouched.

[tjakobsson]

Minor: getComputedStyle(scrollRoot) now runs on every scroll-driven rAF tick to read scrollPaddingTop, which only changes on layout/ui-mode changes. This is the hottest path in the preview while reading. Reading it once in resolveRoots() (and on layout/mode change) and caching does the same job.

[addiberra]

Done in dc5b013: scroll-padding-top is cached per scroll-spy attachment and cleared on re-attach (every remount, including a split/single layout switch), on resize (the value depends on the ≤900px stacked media query and --device-safe-top), on UI-mode change, and via a MutationObserver on <html> class/style, since the macOS desktop host can change --titlebar-inset without a resize. The spyTriggerOffset tests are unchanged.

[tjakobsson]

@claude please review again. Nine commits landed since your last pass (a3439b3..e2fba83) addressing the review findings: /api/document 500 logging, load-retry re-arm on same-file selection with an activation counter in selection.ts, cached scroll-padding-top in the outline scroll-spy, hub-nav reconcile keyed on entry identity instead of position, hub-server terminate() awaiting exit + waitForPortsFree before port reuse, shutdown rejection now exits instead of retaining the lease, the duplicate assertSocket pair removed before the guardian status probe, WebKit launch rejection cleared from the cache, and the chmod EACCES tests skipped as root. Focus on the delta: does each fix hold, and does any introduce a new regression (especially the identity-keyed reconcile in hub-nav.ts and the activation counter in selection.ts).

[claude]

Claude finished @tjakobsson's task in 3m 27s —— View job

---

Re-review complete ✅ — delta a3439b3..e2fba83 (9 commits)

• Gather context: diff a3439b3..e2fba83, read CLAUDE.md
• Review /api/document 500 logging (routes.ts) — a3439b3
• Review load-retry activation counter (selection.ts, load-retry.ts, mount.ts) — d66b1bb
• Review cached scroll-padding-top (outline.ts) — dc5b013
• Review hub-nav identity-keyed reconcile (hub-nav.ts) — 821d0de
• Review hub-server terminate()/waitForPortsFree (hub-server.ts) — 01082fc
• Review hub shutdown-rejection exit path (main.ts) — 626f554
• Review duplicate assertSocket removal (credential-ssh-agent.ts) — 71ccc09
• Review WebKit launch rejection cache clear (page-diagnostics.ts) — e2fba83
• Review chmod EACCES root-skip (render-dispatch.test.ts / routes.test.ts) — cbe4029
• Synthesize findings

Bottom line: all nine fixes hold up, and I found no new regressions — including in the two I looked at hardest, the identity-keyed hub-nav reconcile and the selection activation counter.

The two flagged as highest-risk

reconcileMenuEntries identity keying (src/shell/hub-nav.ts:488-566): traced the full algorithm by hand against both new tests.

• Matching: menuEntryKey gives repository headers a repository:<id> key and defers to menuFocusKey (workspace: / label: / href:) for everything else; dividers have no identity key and fall back to signature matching. Old children are bucketed by key in document order; new entries dequeue from the same-key bucket in order, so duplicate identities (two dividers, or the x/x case in the unit test) pair off correctly by position rather than colliding.
• A dequeued old node is only reused if its cached signature still matches the freshly-built entry's signature; otherwise the new node wins and the old one is dropped — this is what makes a genuinely changed row (e.g. row("a") → row("a","renamed")) get replaced even though it shares an identity key.
• The kept nodes then get a proper LIS (patience-sort + predecessor backtracking) over their old indices, and only the non-stable ones are moved via backward insertBefore. I verified the LIS/backtracking is the standard correct construction, not an approximation.
• This closes the exact hole the earlier review flagged: an insert/remove/regroup above the pressed row no longer touches that row's node, because matching no longer depends on index at all.
• One theoretical soft spot, not a regression: if the same identity key legitimately appears twice and the two instances also reorder relative to each other between renders, the dequeue-by-document-order pairing can mismatch and replace both instead of recognizing the swap. This requires duplicate workspace ids/aria-labels/hrefs, which shouldn't occur for workspace rows in practice — dividers are the only realistic duplicate-key case, and dividers are content-free, so there's nothing to "mismatch."
• The worktreeApi closure concern from the last round is addressed by comment (worktreeApi is read at click time, and the header's signature already covers workspace) rather than by code — reasonable, since worktreeApi is set once from the initial hub probe and the signature would change if a header re-rendered with a different one anyway.

Selection activation counter (src/shell/selection.ts, src/preview/load-retry.ts, src/preview/mount.ts): selectionActivation increments on every navigation-origin setSelectedId call, independent of selectionGeneration (which only moves when the id actually changes) and independent of the personal-state persistence guard added in the original PR (changed || origin === "navigation") — the two mechanisms don't interact. documentLoadRetryKey folds selectionGeneration + activation + documentId, and the activation value is captured once at executeLoadDocument entry, alongside loadToken, so retries scheduled from the same failed load keep reusing that snapshot rather than re-reading a live counter (verified: the automatic retry callback calls loadDocument(documentId) directly, which does not call setSelectedId, so it can't bump its own key mid-schedule). A fresh user click on the already-selected row bumps activation without touching selectionGeneration, correctly re-arming the schedule; a watcher reconcile bumps neither. This matches the new load-retry.test.ts case exactly and I don't see a way for it to double-arm or leak between documents, since documentId is still part of the key.

The other seven, briefly

• /api/document 500 logging (routes.ts): logs only on the 500 branch (console.error with document id + error), 404/415 stay silent, response body unchanged. Exactly as described.
• Outline scroll-padding caching (outline.ts): cache is invalidated on scroll-spy re-attach, viewport/UI-mode change, and via a MutationObserver on <html> class/style (for the desktop host's titlebar inset). Every code path that can change scroll-padding-top has a corresponding invalidation; no stale-cache scenario found.
• hub-server terminate() (tests/e2e/hub-server.ts): now genuinely awaits the exit event after SIGKILL (bounded 3 s via settlesWithin, using the same exited promise registered once via child.once), and start() calls the pre-existing bounded waitForPortsFree before reusing a pinned port. Both the SIGTERM-grace and SIGKILL-wait phases share one exit listener, so there's no double-registration or missed-event window.
• Hub shutdown rejection (src/hub/main.ts): the rejection branch now only calls reportFailure + forceExit(1); retain()/reportRetained are reachable solely from the resolved stateLeaseHeld: true path. state is left at "shutting-down" on rejection (never "lease-retained"), so a second signal still correctly force-exits rather than hanging.
• Duplicate assertSocket removal (credential-ssh-agent.ts): confirmed request() independently authenticates every call (guardian-side HMAC over nonce + command + challenge, timingSafeEqual-checked) and fails closed on connectError/error/malformed reply — so the removed pre-flight identity check before status was genuinely redundant, not a weakened check. The identity check right before stop is kept and is arguably itself now redundant given stop goes through the same authenticated request(), but keeping it as defense-in-depth is harmless.
• WebKit launch rejection caching (page-diagnostics.ts): rejection clears launched and rethrows, so the next launchBrowser("webkit") call retries instead of awaiting a stale rejection; teardown uses .catch(() => undefined) so it tolerates "never launched" and "launch failed" without throwing during cleanup.
• chmod EACCES root-skip (render-dispatch.test.ts, routes.test.ts): test.skipIf(process.getuid?.() === 0), matching the suggested fix; process.getuid?.() degrades safely (undefined === 0 → false → test runs) on platforms without getuid.

Verification note

I wasn't able to execute bun test in this sandbox (the tool run required interactive approval that wasn't available), so this pass is static/read-through only — tracing each changed function against its callers and the new/updated tests' assertions, not a live test run. Everything above is consistent with the test expectations in the diff, but a maintainer should still let CI confirm.

No blocking issues found in this delta.
· Branch: fix/e2e-flakes

[tjakobsson]

Thanks for the fast turnaround — all eight earlier findings are addressed and the fixes read well; the identity-keyed reconcile and the activation counter are both cleaner than what I sketched. Second pass over the new head found two things I'd like fixed before merge and five smaller ones (marked minor/optional inline).

One of the two is on me: in the guardian recovery I offered "drop the first pair or drop the second", and dropping the first turns out to lose the fast, precise failure. Details inline, with what I'd do instead.

Blocking: credential-ssh-agent.ts (recovery error quality/time), hub-nav.ts (fork-control focus key collides across same-named checkouts).
Minor / optional: mount.ts, render-dispatch.ts, load-retry.ts, live-broker.ts, version.ts.

[tjakobsson]

Please put the pre-flight assertSocket pair back before this status probe — my earlier comment offered either option and this is the one that loses something. The test diff shows it: the two recovery cases that asserted does not match its ownership record / ENOENT now assert the generic SSH guardian request failed.

In production that means a control socket that was removed or replaced after a crash no longer fails at once with the identity mismatch; request(record, "status") waits the full stopTimeoutMs (2 s), then throws the advice to remove the runtime directory — steering the operator toward deleting state for what was an identity check. Healthy recovery also now pays up to 2× stopTimeoutMs in the worst case (status + stop).

Suggested shape: pre-flight pair → status → stop, and drop the post-status pair instead (the signed status reply already proves the nonce's owner is at that socket; a swap in the microseconds between is what the nonce protects against, not the metadata check). Restore the two precise test assertions with it.

[addiberra]

Done in 1e99f3b, with the shape you suggested: pre-flight assertSocket pair → status → stop, and the post-status pair dropped (the signed status reply is the identity proof; the pre-flight is the fast, precise failure). The two precise assertions are back (does not match its ownership record, ENOENT) and the stopTimeoutMs: 200 overrides are gone: the replaced-socket case now fails in ~180 ms with the default 2 s timeout, most of that being the guardian's own start in the test's setup. The forged-challenge test still passes, so a wrong-nonce recovery still leaves the real guardian alive. Sorry for picking the wrong option; your reasoning here was right.

[tjakobsson]

Fork-control focus keys collide across same-named checkouts. The fork button's aria-label is Add worktree to ${workspaceMenuLabel(workspace)}, so two registered main checkouts with the same display name (legal — the file's own comment says the menu disambiguates by path) both key to label:Add worktree to docs. If focus is on the second repo's button when a refresh replaces that header, .find(item => menuFocusKey(item) === focusedKey) returns the first repo's button, and Enter forks the wrong repository.

This matters more now that the same key drives entry matching in the reconcile. Give the fork control a data-workspace-id (or a dedicated data-fork-for) and key on that, as rows are.

[addiberra]

Fixed in 1ca81d6. The fork control carries data-fork-for=<main checkout id>, and menuFocusKey keys it as fork:<id> (checked right after data-workspace-id, before any aria-label fallback), so it can't collide with its row's workspace:<id> or with a same-named repo's control. No other menu control keyed on a label: rows use data-workspace-id, headers data-repository, and the dashboard/sign-out links have unique hrefs. New tests: two main checkouts both named "docs", focus on the second repo's fork button, a refresh replaces both headers → focus lands on the second repo's new button (fails on the old code); and a refresh that changes only the first header keeps the second's header and button nodes. One correction to the premise: the reconcile matches only the menu's direct children, and the fork button sits inside its header (keyed by data-repository), so the collision could misplace focus but never mismatched reconcile entries.

[tjakobsson]

Minor (UX, optional but I'd take it): when the failing load is a reload of the document already on screen — a watcher frame for the same selection while the child answers 500 under load, or the hub proxy 503s during a child restart — renderUnavailableDocument tears down the rendered content, clears the outline and hides the facts strip, so the reader loses their place for up to 14 s while the schedule runs. The retry machinery makes "keep the last good render, retry quietly (maybe a small notice)" the natural behavior here; blanking was what the old code did because it had nothing else to do. Only blank when there is no current render for this document.

[addiberra]

Agreed on the behaviour, and I'd rather not fold it into this PR: keeping the last good render while a same-document reload retries changes what "unavailable" means for the outline and the facts strip too, and deserves its own change with its own e2e. Could you file it as an issue? Happy to take it next.

[tjakobsson]

Minor: everything non-ENOENT is 500 = "transient", so a document that fails permanently — a renderer throwing on its content, EISDIR, EACCES — is retried four times by every open client (five renders and five console.error lines per view, ten with two tabs), the user reads "Retrying…" for 14 s for a file that will never render, and "Select it again" restarts the cycle. EACCES/EISDIR are stable conditions, not the server failing; a 403 (or any final 4xx) for those would keep the retry for the cases it was built for (EMFILE, a mid-restart child) without the churn. Renderer throws are a judgment call — a 500 is honest there — so this is really about the fs codes.

[addiberra]

Agreed that EACCES/EISDIR are stable conditions and shouldn't be retried; I'd like to handle it in its own PR rather than here, since it's a semantics change to the status mapping this PR just introduced and I'd want the client's final-4xx message and the retry schedule reviewed together. Could you file it as an issue? The renderer-throw case I'd keep at 500, as you say.

[tjakobsson]

Minor: once the four attempts for a key are used up, failed keeps returning false for that key. A later watcher frame for the same selection does perform one fresh fetch (good), but if that one also fails the schedule is not re-armed — the frame is new evidence the file changed, so it would be reasonable to treat it like an activation and start over. Cheap version: settle() when a load is triggered by a live frame rather than a retry.

[addiberra]

Done in b13f83c. Loads now carry a trigger: only the retry timer's own reload is a retry; any other failed load (a live frame's reload of the current selection, a navigation, a view/layout change) resets the attempt count and starts the schedule over at the first delay, so a failed retry can never re-arm itself. The rule lives in load-retry.ts (failed(key, retry, trigger)) and the new test exhausts the schedule, fails a live-frame reload of the same selection, and asserts a fresh schedule that then runs out again without re-arming. Side effect worth knowing: a Source/Rendered toggle on an exhausted document also restarts it, which seems right (it's a user action).

[tjakobsson]

Minor: each (topic,status) arms one unref'd timer that closes over this sink's options.log, and nothing cancels it. If the sink is replaced via setLiveUpstreamDiagnostics (the e2e harness's quiet sink; unit tests that don't inject setTimer), a summary line still fires through the retired default sink's console.error up to 60 s later — the very harness noise the quiet sink exists to stop. Either return a dispose() that clears open timers and call it on replacement, or have the callback consult the current sink at fire time.

[addiberra]

Done in ab07bfa, option (a): the folding sink now has dispose(), which clears its open summary timers, drops the pending counts (passing them to the new sink would summarise lines it never saw) and silences it for good; setLiveUpstreamDiagnostics retires the previous sink when it installs a different one, and null now builds a fresh default sink instead of reusing a possibly disposed module instance. A unit test folds a repeat, swaps the sink, advances past the window and asserts neither sink logs. Metrics are unchanged.

[tjakobsson]

Minor: the walk honours GIT_DIR/GIT_COMMON_DIR/GIT_WORK_TREE but not GIT_CEILING_DIRECTORIES (or GIT_DISCOVERY_ACROSS_FILESYSTEM), which the git rev-parse it replaces did. This PR itself exports GIT_CEILING_DIRECTORIES into the e2e servers' environment, so a source run below a ceiling now reports the ancestor repository's branch@sha where the old code fell back to main@unknown. Stop the walk at any ceiling directory (return null so the caller asks Git, which will refuse the same way).

[addiberra]

Done in 5c75d93. readGitHeadFromFiles now reads GIT_CEILING_DIRECTORIES from its env parameter with Git's rules (split on the platform delimiter; relative entries ignored; absolute entries symlink-resolved unless they follow an empty entry; the starting directory still searched) and returns null before ascending into a ceiling, so the caller asks Git, which refuses the same way. Tests check each case against real git rev-parse (ceiling at the repo's parent, at the repo root, a mixed list, a symlink alias, and that process.env is not consulted). GIT_DISCOVERY_ACROSS_FILESYSTEM is not modelled — it would need a device comparison at each level — and the doc comment says so.

[tjakobsson]

@addiberra — to be clear about where this stands: CI is green on e2fba83 and the bot review found nothing blocking. From my side, two items stand between this and approval; the other five in my last review are optional and can land here or as follow-ups, your choice.

Needed for approval

1. src/hub/credential-ssh-agent.ts — put the pre-flight assertSocket pair back before the status probe (and drop the post-status pair). A missing/swapped control socket should fail at once with does not match its ownership record / ENOENT, not wait stopTimeoutMs and advise removing the runtime directory. Restore the two precise test assertions with it. This one is my fault — I offered either option and the wrong one got picked.
2. src/shell/hub-nav.ts — key the fork control on a data-workspace-id (or similar) instead of its aria-label, so two same-named checkouts can't swap focus onto the other repo's "Add worktree" button across a refresh.

Optional (mount.ts keep-last-render, render-dispatch.ts EACCES/EISDIR as final, load-retry.ts re-arm on a live frame, live-broker.ts orphaned fold timers, version.ts GIT_CEILING_DIRECTORIES) — say which you're taking and which you'd rather I file as issues.

Once 1 and 2 are in, I'll re-run the suite and approve.

[tjakobsson]

Approving at b13f83c. Both items I asked for are in exactly the shape requested (pre-flight → status → stop with the precise assertions back; fork control keyed fork:<id>), the three minors you took are done, and CI ran the full suite green on this head (unit, both e2e legs, perf, contracts).

The two you asked me to file: #468 (keep the last good render while a same-document reload retries) and #469 (EACCES/EISDIR as final; also covers a non-JSON 200 body being retried, which I noticed on this pass).

Two non-blocking notes, take them or leave them — I won't hold the PR for either:

• With trigger === "request" now resetting the schedule, the activation component of the retry key no longer changes any outcome (a retry always shares its scheduling load's key; a new key only arises from a request). selectionActivation/getSelectionActivation in selection.ts and the key helper could go, leaving a two-state schedule. My earlier ask created the redundancy, so no complaint if you'd rather leave it.
• src/shell/selection-persistence.test.ts has no sibling module; CLAUDE.md's colocation rule would put these cases in selection.test.ts (or name a module they belong to).

Thanks for the thoroughness across four rounds — the PR body's per-run evidence made this much easier to review.