diff --git a/.smoke-evidence/E2E_FIX_VERIFICATION.md b/.smoke-evidence/E2E_FIX_VERIFICATION.md new file mode 100644 index 00000000000..01ea29fe76e --- /dev/null +++ b/.smoke-evidence/E2E_FIX_VERIFICATION.md @@ -0,0 +1,134 @@ +# Adversarial verification — 6 e2e fixes (branch fix-e2e-tests-2, uncommitted) + +Independent review. All 66 unit tests across the 3 changed unit-test files PASS +(`vitest run` on worktree-teardown / focus-existing-window / terminal-parked-tab-watchers). + +Overall: **SHIP.** 4 CORRECT, 2 RISKY-but-safe. No fix masks a real product bug or +weakens a real invariant. One PR-note on #6. + +--- + +## #1 worktree-teardown selector_not_found tolerance — VERDICT: CORRECT (ship) + +Root cause independently confirmed: `stopPtysForDestructiveWorktreeRemoval` +(orca-runtime.ts:2725, `requirePhysicalStop:true`) → `killAllProcessesForWorktree` +→ runtime sweep → `stopTerminalsForWorktree(worktreeId)` which INTERNALLY re-resolves +via `resolveWorktreeSelector('id:'+id)` (line 20355). That resolve legitimately throws +`selector_not_found` for a worktree already unlisted by git during removal — the very +case `resolveWorktreeRemovalTarget` already anticipates and treats as benign (line +18128-18139, falls back to persisted metadata). Old predicate only tolerated +`runtime_unavailable`, so the sweep fail-closed and the whole removal returned +ok:false. This IS the failure. + +Not masking a real teardown: the runtime sweep is the renderer-graph path and is +NON-authoritative. Provider sweep + registry sweep run independently in the same +Promise.all, keyed on the RAW worktreeId (prefix `${id}@@` / session.worktreeId / cwd), +not on selector resolution. Under `requirePhysicalStop`, sweepProviderByPrefix calls +`listProcesses()` un-caught (fails closed on throw) and the final `stopAttempts` check +(line 140-145) still throws if any discovered PTY couldn't be physically stopped. So +real live PTYs are still caught and can still block removal. When selector_not_found +is thrown the runtime sweep did nothing anyway (throws before iterating leaves/ptysById), +so swallowing it only drops a spurious block, not a real one. +New unit test validates the tolerance (rejects selector_not_found, empty provider/registry, +requirePhysicalStop:true → runtimeStopped:0, no throw). + +Narrow note (not blocking): SSH path passes `includeLocalRegistry:false`, so provider +sweep is the sole net there. It still runs + fails closed, but a hypothetical SSH pty in +ptysById whose provider row lacks prefix/worktreeId/cwd would be uncovered — an extreme, +pre-existing edge, not introduced here. + +## #2 worktree-scroll-to-current 3-rows-vs-2 — VERDICT: CORRECT / pollution (no code change; transitive) + +Mechanism confirmed but the task's "shared Electron instance" wording is imprecise. Each +test gets a FRESH Electron + fresh userDataDir (orca-app.ts:133-167), so it is NOT store +pollution. The real vector is the **worker-scoped on-disk seed git repo** (testRepoPath, +scope:'worker'). `createIsolatedWorktree` materializes a real git worktree on that shared +repo (worktree-lifecycle.spec.ts:82-86 says so explicitly). When #1's removeWorktree +returns ok:false, both the test body AND the afterEach best-effort cleanup fail, leaking +a git worktree on the shared repo. The scroll test (runs later — "lifecycle" < "scroll" +alphabetically, same shard) disables all sidebar filters and attaches the same seed repo, +so git lists the leaked worktree → 3 rows vs 2. Matches CI (memory: shard10 root-caused to +selector_not_found teardown). Fixing #1 makes both removals succeed → no leak. RECOMMEND a +combined shard run to confirm empirically (only proof not run here). + +## #3 parked-tab-watchers branch reorder — VERDICT: CORRECT (ship) + +Reorder puts `disposersByPtyId.size > 1` (surviving sibling leaf) BEFORE the `hadPrimary` +short-circuit, so a parked split's dead leaf is collapsed out of the persisted store even +when hadPrimary=true. Sound: `hadPrimary` (pty-exit-delivery.ts:38) just means a primary +handler was registered; detach() retains it (pty-transport.ts:979-993) but the PaneManager +is destroyed while parked, so its manager.closePane writes nothing to the store — the old +hadPrimary-first path disposed without collapsing → dead PTY binding survived to reveal → +ghost pane. Exactly failure #3 (stuck 2 panes, deadPtyStillBound). + +No regression to the sole-pane / whole-tab-close case: that has size===1, so the new +size>1 branch is skipped and the hadPrimary branch runs unchanged. No double-collapse: +the retained primary is a no-op vs the store (manager destroyed), and collapseParkedExitedLeaf +is idempotent — detachTerminalLayoutLeaf returns null for an already-absent leaf +(terminal-layout-leaf-detach.ts:18-19) and the `if (detached)` guard skips. Sidecar only +exists while parked (started/disposed with park/reveal), and reveal disposes it in the same +effect flush before remount, closing the both-alive window. New unit test asserts exactly +the (size>1, hadPrimary:true) case: store collapsed to the survivor leaf, only the dead +watcher disposed. + +## #4 terminal-tab-close-restart-persistence assertion change — VERDICT: CORRECT (ship) [HIGH-scrutiny cleared] + +Test-design bug, NOT a persistence regression papered over. The auto-create is real, +intended, established behavior: Terminal.tsx:1225-1251 `useEffect` keyed on +[workspaceSessionReady, activeWorktreeId, createTab, reconcileWorktreeTabModel] (NOT tab +count) calls `shouldAutoCreateInitialTerminal(renderableTabCount===0)` → createTab, and its +own comment says re-running on tab-count changes "would recreate a terminal immediately +after the user intentionally closed the last visible one." That's why the SAME test's +mid-session post-close assertion `toEqual([])` (line 81) still holds (worktree not +re-activated), while post-restart it doesn't (worktree re-activates → fresh "Terminal 1", +new id). Original `toEqual([])`-after-restart coupled to an unintended invariant. + +The durable-close guarantee (PR #8958) is closed-tab IDENTITY, not empty list. New +assertions check exactly that on both surfaces: renderer tabs lack `closedTabId`, and +runtime `terminal.list` has no terminal with `tabId===closedTabId`. A genuine resurrection +would carry the persisted `closedTabId` and be caught; a fresh Terminal 1 has a new id and +is correctly ignored. Also more robust to auto-create timing than the old count assertion. + +## #5 combined-diff-scroll-restore conditional scrollTop cap — VERDICT: RISKY-but-safe (ship) + +Not hiding an anchoring regression. The real visual-anchoring invariant — +`|afterLineClick.top - afterSwitch.top| < 80` — stays UNCONDITIONAL. Only the raw +`scrollTop < 40` cap is now gated on `scrollHeight` being unchanged. Rationale is correct: +when a section above swaps estimated→measured height, Chromium native scroll anchoring +(no overflow-anchor:none here) shifts raw scrollTop specifically to keep the visible row +pinned — so scrollTop moving while `top` holds is correct, not a regression. Mirrors the +pre-existing `getLargestBackwardScrollJump` "at a stable content height" guard in the same +test. A real regression at stable height still hits the strict cap; a real visual jump +(with or without height change) still fails the unconditional `top` check. + +Minor nit (non-blocking): the gate is exact `scrollHeight === scrollHeight`, so even a 1px +height delta disables the 40px cap entirely. Safe because `top` remains unconditional, but +slightly coarse; a tolerance band would be tighter. Acceptable. + +## #6 focus-existing-window bounded reopen retry — VERDICT: RISKY (ship as defensive + diagnostic; causal link speculative) + +Retry logic is CORRECT: `REOPEN_MAX_ATTEMPTS=3` (1 initial + 2 retries), guard +`attempt >= MAX` before scheduling → no infinite loop; 300ms injected timer; single-pane +callers see 'pending' just as before. Test-harness `makeTimer` fix (splice due entries +before invoking) is necessary and right — without it a re-scheduled same-ms entry would be +double-fired by a later run(). Unit tests cover transient-recover and give-up-after-3. + +Honest caveats: +- Causation is SPECULATIVE — agent reproduced 0/16 locally. A swallowed openWindow() throw + is a *plausible* cause of the 60s firstWindow timeout, but the real CI flake may be + elsewhere (second-instance handler not firing, CDP attach, etc.). The forwarded logs + (test-side + exported forwardElectronProcessLogs) are the genuine value: next CI failure + will leave a trail. Change is low-risk and defensive regardless. +- PR-NOTE: confirm `openWindow()` is safe to call again after a throw — if a throwing + attempt can leave a partially-created/registered window, the retry could double-open + (openWindowWithRetry re-calls openWindow() directly, not getWindow()). If openWindow is + idempotent/singleton this is fine. +- Minor: a retry-recovered window skips the win32 moveTop/pulseAlwaysOnTop/retryFocus that + the synchronous path runs (function already returned 'pending'). Cosmetic on the rare + retry path. + +--- + +### Ship recommendation per fix +1 CORRECT · 2 CORRECT(transitive; recommend combined shard run) · 3 CORRECT · 4 CORRECT · +5 CORRECT (minor nit) · 6 SHIP as defensive+diagnostic (verify openWindow re-entrancy at PR). diff --git a/src/main/runtime/worktree-teardown.test.ts b/src/main/runtime/worktree-teardown.test.ts index 827ec95d92d..1025c6be020 100644 --- a/src/main/runtime/worktree-teardown.test.ts +++ b/src/main/runtime/worktree-teardown.test.ts @@ -567,6 +567,25 @@ describe('killAllProcessesForWorktree', () => { expect(result.runtimeStopped).toBe(0) }) + it('tolerates an unresolved runtime selector during destructive removal', async () => { + // Why: a just-created/removed worktree can be absent from the runtime + // graph; that means zero runtime-owned PTYs, not a failed teardown. + const stopTerminalsForWorktree = vi.fn().mockRejectedValue(new Error('selector_not_found')) + const runtime = { + stopTerminalsForWorktree + } as unknown as Parameters[1]['runtime'] + const localProvider = createProviderStub(async () => []) + listRegisteredPtysMock.mockReturnValue([]) + + const result = await killAllProcessesForWorktree('w1', { + runtime, + localProvider, + requirePhysicalStop: true + }) + + expect(result.runtimeStopped).toBe(0) + }) + it('fails destructive teardown closed when the runtime sweep rejects', async () => { const stopTerminalsForWorktree = vi.fn().mockRejectedValue(new Error('runtime sweep failed')) const runtime = { diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 77ebaa8737a..8dfe63a9f7e 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -93,15 +93,21 @@ export async function killAllProcessesForWorktree( return current } - // Why: headless CLI has no ready renderer graph; only that known sentinel - // may fall through to the provider and registry physical-owner sweeps. + // Why: headless CLI has no ready renderer graph, and a just-created/removed + // worktree may not resolve in the graph yet; either case means zero + // runtime-owned PTYs, so both sentinels fall through instead of failing + // destructive removal closed. const runtimeSweep = deps.runtime ? settleBeforeDeadline( () => deps.runtime!.stopTerminalsForWorktree(worktreeId, { deadline, stopPty }), { stopped: 0 }, deadline, deps.requirePhysicalStop ? deadlineError : undefined, - (error) => !(error instanceof Error && error.message === 'runtime_unavailable') + (error) => + !( + error instanceof Error && + (error.message === 'runtime_unavailable' || error.message === 'selector_not_found') + ) ) : Promise.resolve({ stopped: 0 }) const providerSweep = settleBeforeDeadline( diff --git a/src/main/window/focus-existing-window.test.ts b/src/main/window/focus-existing-window.test.ts index e452489a6eb..3d0b54d7824 100644 --- a/src/main/window/focus-existing-window.test.ts +++ b/src/main/window/focus-existing-window.test.ts @@ -63,7 +63,14 @@ function makeTimer(): { return callbacks.length }, run: (ms) => { - for (const entry of callbacks.filter((entry) => entry.ms === ms)) { + // Why: splice out matching entries before invoking — real timers fire + // once, and a retry callback here can schedule a fresh same-ms entry + // that a later run() call must not confuse with one already fired. + const due = callbacks.filter((entry) => entry.ms === ms) + for (const entry of due) { + callbacks.splice(callbacks.indexOf(entry), 1) + } + for (const entry of due) { entry.callback() } }, @@ -154,4 +161,97 @@ describe('focusExistingMainWindow', () => { expect(openedWindow.calls.show).toHaveBeenCalledTimes(1) expect(openedWindow.calls.focus).toHaveBeenCalledTimes(1) }) + + it('retries a transient reopen failure instead of stranding the app windowless', () => { + const app = makeFakeApp() + const timer = makeTimer() + const openedWindow = makeFakeWindow() + const warn = vi.fn() + let attempts = 0 + const openWindow = vi.fn(() => { + attempts += 1 + if (attempts < 2) { + throw new Error('transient failure') + } + return openedWindow + }) + + const result = focusExistingMainWindow({ + app, + getWindow: () => null, + openWindow, + warn, + setTimeout: timer.setTimeout + }) + + // Why: the first attempt's own return value is synchronous, so a retry + // scheduled after the first throw still reports 'pending' immediately. + expect(result).toBe('pending') + expect(warn).toHaveBeenCalledTimes(1) + expect(openWindow).toHaveBeenCalledTimes(1) + + timer.run(300) + expect(openWindow).toHaveBeenCalledTimes(2) + expect(openedWindow.calls.show).toHaveBeenCalledTimes(1) + expect(openedWindow.calls.focus).toHaveBeenCalledTimes(1) + }) + + it('adopts a window that appeared during the retry gap instead of opening a duplicate', () => { + // Why: openMainWindow is not idempotent — a duplicate open would orphan the + // window already on screen. If getWindow() resolves before the retry fires, + // the retry must reveal that window and never call openWindow again. + const app = makeFakeApp() + const timer = makeTimer() + const raceWindow = makeFakeWindow() + const warn = vi.fn() + let liveWindow: BrowserWindow | null = null + const openWindow = vi.fn(() => { + // First attempt throws, but a concurrent path lands a window before retry. + liveWindow = raceWindow + throw new Error('transient failure') + }) + + const result = focusExistingMainWindow({ + app, + getWindow: () => liveWindow, + openWindow, + warn, + setTimeout: timer.setTimeout + }) + + expect(result).toBe('pending') + expect(openWindow).toHaveBeenCalledTimes(1) + + timer.run(300) + // Retry adopts the raced-in window; it must not construct a second one. + expect(openWindow).toHaveBeenCalledTimes(1) + expect(raceWindow.calls.show).toHaveBeenCalledTimes(1) + expect(raceWindow.calls.focus).toHaveBeenCalledTimes(1) + }) + + it('gives up after exhausting reopen retries', () => { + const timer = makeTimer() + const warn = vi.fn() + const openWindow = vi.fn(() => { + throw new Error('persistent failure') + }) + + const result = focusExistingMainWindow({ + app: makeFakeApp(), + getWindow: () => null, + openWindow, + warn, + setTimeout: timer.setTimeout + }) + + expect(result).toBe('pending') + expect(openWindow).toHaveBeenCalledTimes(1) + + timer.run(300) + timer.run(300) + // Why: REOPEN_MAX_ATTEMPTS caps total tries at 3 (1 initial + 2 retries); + // a further run() call must not schedule or fire a 4th attempt. + expect(openWindow).toHaveBeenCalledTimes(3) + expect(warn).toHaveBeenCalledTimes(3) + }) }) diff --git a/src/main/window/focus-existing-window.ts b/src/main/window/focus-existing-window.ts index 1ff9d86478f..874c9adbfa5 100644 --- a/src/main/window/focus-existing-window.ts +++ b/src/main/window/focus-existing-window.ts @@ -64,6 +64,45 @@ function retryFocus(window: BrowserWindow, app: Pick, setTimer: Fo }, 100) } +// Why: a second-instance/activate reopen can race transient startup pressure +// (e.g. GPU/process churn right after another launch attempt exits); one +// swallowed throw would otherwise strand the app with no window until some +// later external trigger happens to retry. +const REOPEN_MAX_ATTEMPTS = 3 +const REOPEN_RETRY_DELAY_MS = 300 + +function openWindowWithRetry( + opts: Pick, + setTimer: FocusTimer, + attempt: number +): BrowserWindow | null { + try { + return opts.openWindow() + } catch (error) { + opts.warn?.('[window] Failed to reopen main window for second-instance launch', error) + if (attempt >= REOPEN_MAX_ATTEMPTS) { + return null + } + setTimer(() => { + // Why: openWindow() (openMainWindow) is not idempotent — it constructs and + // registers a fresh BrowserWindow on every call. Between attempts another + // path (or a first attempt that threw after creating its window) may have + // produced a live window, so adopt it instead of opening a duplicate that + // would orphan the one already on screen. + const existing = opts.getWindow() + const window = + existing && !existing.isDestroyed() + ? existing + : openWindowWithRetry(opts, setTimer, attempt + 1) + if (window) { + safelyFocusApp(opts.app) + safelyRevealWindow(window) + } + }, REOPEN_RETRY_DELAY_MS) + return null + } +} + export function focusExistingMainWindow( opts: FocusExistingMainWindowOptions ): FocusExistingMainWindowResult { @@ -76,13 +115,11 @@ export function focusExistingMainWindow( if (!opts.app.isReady()) { return 'pending' } - try { - window = opts.openWindow() - openedWindow = true - } catch (error) { - opts.warn?.('[window] Failed to reopen main window for second-instance launch', error) + window = openWindowWithRetry(opts, setTimer, 1) + if (!window) { return 'pending' } + openedWindow = true } safelyFocusApp(opts.app) diff --git a/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts b/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts index 00c73ae9be1..d2a56f66bf7 100644 --- a/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts @@ -259,6 +259,42 @@ describe('terminal-parked-tab-watchers', () => { expect(getParkedTerminalWatcherTabIds()).toEqual([TAB_ID]) }) + it('collapses a dead split leaf even when a stale primary handler also observed the exit', () => { + // Why (regression, #ghost-blank-pane): a genuinely parked tab's PaneManager + // is already destroyed, so the retained primary exit handler's own + // split-collapse path is a no-op against the persisted layout — hadPrimary + // must not skip this sidecar's collapse for a surviving sibling leaf. + capturePanes([ + { ptyId: PTY_ID, paneId: 1, leafId: LEAF_ID, drivesTabTitle: true }, + { ptyId: SECOND_PTY_ID, paneId: 2, leafId: SECOND_LEAF_ID, drivesTabTitle: false } + ]) + syncParked() + mockStoreState.terminalLayoutsByTabId[TAB_ID] = { + root: { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: LEAF_ID }, + second: { type: 'leaf', leafId: SECOND_LEAF_ID } + }, + activeLeafId: SECOND_LEAF_ID, + expandedLeafId: null, + ptyIdsByLeafId: { [LEAF_ID]: PTY_ID, [SECOND_LEAF_ID]: SECOND_PTY_ID } + } + + const exited = exitSubscriptions.find((entry) => entry.ptyId === SECOND_PTY_ID) + exited?.callback(0, { hadPrimary: true }) + + expect(mockStoreState.setTabLayout).toHaveBeenCalledWith(TAB_ID, { + root: { type: 'leaf', leafId: LEAF_ID }, + activeLeafId: LEAF_ID, + expandedLeafId: null, + ptyIdsByLeafId: { [LEAF_ID]: PTY_ID } + }) + expect(startedWatchers[1].dispose).toHaveBeenCalledTimes(1) + expect(startedWatchers[0].dispose).not.toHaveBeenCalled() + expect(getParkedTerminalWatcherTabIds()).toEqual([TAB_ID]) + }) + it('seeds each watcher with the pane slot last known runtime title', () => { mockStoreState.runtimePaneTitlesByTabId = { [TAB_ID]: { 1: '⠋ Build feature' } } capturePanes([ diff --git a/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.ts b/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.ts index 8261e1a66c2..581378cf40c 100644 --- a/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.ts +++ b/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.ts @@ -146,16 +146,16 @@ function startParkedTabWatchers( const unsubscribeExit = subscribeToPtyExit(ptyId, (_code, { hadPrimary }) => { // Why: while parked this sidecar is the only exit observer, so teardown must run here or dead leaves resurrect on reveal. useAppStore.getState().clearRuntimePaneTitle(tab.id, pane.paneId) - if (hadPrimary) { - // Why: primary exit owner already closes the tab/leaf; just retire parked observation so one exit can't double-confirm. + if (disposersByPtyId.size > 1) { + // Why: a parked PaneManager is gone, so its retained primary cannot remove a dead split leaf from persisted layout. + discardPreHandlerPtyState(ptyId) + collapseParkedExitedLeaf(tab.id, ptyId) disposersByPtyId.get(ptyId)?.() disposersByPtyId.delete(ptyId) return } - if (disposersByPtyId.size > 1) { - // Why: dead split leaf never remounts; consume its buffered frame and tombstone duplicate exits. - discardPreHandlerPtyState(ptyId) - collapseParkedExitedLeaf(tab.id, ptyId) + if (hadPrimary) { + // Why: the sole pane's primary owner closes its tab; retire the sidecar to avoid duplicate confirmation. disposersByPtyId.get(ptyId)?.() disposersByPtyId.delete(ptyId) return diff --git a/tests/e2e/combined-diff-scroll-restore.spec.ts b/tests/e2e/combined-diff-scroll-restore.spec.ts index 784e67c3eb0..496bead3db6 100644 --- a/tests/e2e/combined-diff-scroll-restore.spec.ts +++ b/tests/e2e/combined-diff-scroll-restore.spec.ts @@ -453,8 +453,16 @@ test.describe('Combined diff scroll restore', () => { // Assert the viewport barely moved rather than an exact anchor key: sections // are ~viewport-sized, so a sub-pixel focus scroll from the click can flip the // topmost-visible key by one without meaningfully moving the scroll position. - expect(Math.abs(afterLineClick.scrollTop - afterSwitch.scrollTop)).toBeLessThan(40) expect(Math.abs(afterLineClick.top - afterSwitch.top)).toBeLessThan(80) + // Why: when a section above the viewport swaps its estimated height for + // Monaco's measured one, scrollHeight changes and Chromium's default + // scroll anchoring (no `overflow-anchor: none` here) legitimately moves + // raw scrollTop to keep the row above asserted `top` pinned — that is + // not an anchoring regression, so only cap scrollTop drift when content + // height held steady (same rule as getLargestBackwardScrollJump above). + if (afterLineClick.scrollHeight === afterSwitch.scrollHeight) { + expect(Math.abs(afterLineClick.scrollTop - afterSwitch.scrollTop)).toBeLessThan(40) + } } finally { rmSync(fixture.repoPath, { recursive: true, force: true }) } diff --git a/tests/e2e/headless-serve-desktop-activation.spec.ts b/tests/e2e/headless-serve-desktop-activation.spec.ts index 0baa26d0761..77b85929d70 100644 --- a/tests/e2e/headless-serve-desktop-activation.spec.ts +++ b/tests/e2e/headless-serve-desktop-activation.spec.ts @@ -3,7 +3,7 @@ import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'no import os from 'node:os' import path from 'node:path' import { _electron as electron, type ElectronApplication } from '@stablyai/playwright-test' -import { test, expect } from './helpers/orca-app' +import { test, expect, forwardElectronProcessLogs } from './helpers/orca-app' import { TEST_REPO_PATH_FILE } from './global-setup' import { getE2ECompletedOnboardingProfile } from './helpers/e2e-completed-onboarding-profile' import { getOrcaElectronLaunchArgs } from './helpers/electron-launch-args' @@ -87,7 +87,7 @@ async function waitForProcessExit(child: ChildProcess, timeoutMs: number): Promi test.describe.configure({ mode: 'serial' }) test('promotes the headless owner without replacing its daemon terminal', async (// oxlint-disable-next-line no-empty-pattern -- This lifecycle test owns both launches and intentionally opts out of the default app fixture. -{}) => { +{}, testInfo) => { const repoPath = readFileSync(TEST_REPO_PATH_FILE, 'utf8').trim() if (!repoPath || !existsSync(repoPath)) { test.skip(true, 'Global setup did not produce a seeded test repo') @@ -113,6 +113,8 @@ test('promotes the headless owner without replacing its daemon terminal', async }) const resolvedHome = await serveApp.evaluate(({ app }) => app.getPath('home')) assertElectronResolvedIsolatedHome(resolvedHome, homeIsolation) + // Why: this spec bypasses the app fixture, so opt into its gated Electron log capture for CI failures. + forwardElectronProcessLogs(serveApp, testInfo) const ownerPid = serveApp.process().pid const client = new RuntimeClient(userDataDir, 5_000) @@ -154,13 +156,26 @@ test('promotes the headless owner without replacing its daemon terminal', async ) .toContain(beforeMarker) + const forwardAppLogs = process.env.ORCA_E2E_FORWARD_APP_LOGS === '1' activatingProcess = spawn(electronPath, getOrcaElectronLaunchArgs(mainPath, false), { env, - stdio: 'ignore' + stdio: forwardAppLogs ? 'pipe' : 'ignore' }) activatingProcess.on('error', (error) => { console.error('[e2e] activating process failed to spawn:', error) }) + if (forwardAppLogs) { + const prefix = '[e2e] activating process' + activatingProcess.stdout?.on('data', (chunk: Buffer) => { + console.log(`${prefix} stdout: ${chunk.toString().trimEnd()}`) + }) + activatingProcess.stderr?.on('data', (chunk: Buffer) => { + console.error(`${prefix} stderr: ${chunk.toString().trimEnd()}`) + }) + activatingProcess.on('exit', (code, signal) => { + console.log(`${prefix} exit: code=${code ?? 'null'} signal=${signal ?? 'null'}`) + }) + } const page = await serveApp.firstWindow({ timeout: 60_000 }) await page.waitForLoadState('domcontentloaded') diff --git a/tests/e2e/helpers/orca-app.ts b/tests/e2e/helpers/orca-app.ts index 0e8f4aaf386..7fc62563bf9 100644 --- a/tests/e2e/helpers/orca-app.ts +++ b/tests/e2e/helpers/orca-app.ts @@ -111,7 +111,10 @@ function shouldLaunchHeadful(testInfo: TestInfo): boolean { return testInfo.project.metadata.orcaHeadful === true } -function forwardElectronProcessLogs(app: ElectronApplication, testInfo: TestInfo): void { +// Why: exported so specs that launch their own ElectronApplication outside +// this fixture (e.g. multi-instance lifecycle tests) can still opt into the +// same ORCA_E2E_FORWARD_APP_LOGS-gated stdout/stderr capture. +export function forwardElectronProcessLogs(app: ElectronApplication, testInfo: TestInfo): void { if (process.env.ORCA_E2E_FORWARD_APP_LOGS !== '1') { return } diff --git a/tests/e2e/terminal-tab-close-restart-persistence.spec.ts b/tests/e2e/terminal-tab-close-restart-persistence.spec.ts index 10efd6491c7..12cf89319d2 100644 --- a/tests/e2e/terminal-tab-close-restart-persistence.spec.ts +++ b/tests/e2e/terminal-tab-close-restart-persistence.spec.ts @@ -99,7 +99,23 @@ test('durable whole-tab close removes a split tab across restart', async (// oxl // Why: wait past initial worktree effects so this checks resurrection, not // only the first hydrated frame before default-tab logic has run. await secondLaunch.page.waitForTimeout(1_000) - expect(await getWorktreeTabs(secondLaunch.page, worktreeId)).toEqual([]) + // Why: reattaching to an emptied worktree intentionally spawns a fresh + // "Terminal 1" tab (Terminal.tsx's shouldAutoCreateInitialTerminal + // fallback fires whenever the active worktree has zero renderable tabs — + // true for a durably-closed worktree just like a brand-new one). That + // fallback is unrelated to this test and reproduces even mid-session with + // no restart at all, so asserting an eternally-empty tab list here is + // wrong. What "durable" actually promises is that the specific closed + // split tab never comes back — assert on its identity, not on tab count. + const restoredTabs = await getWorktreeTabs(secondLaunch.page, worktreeId) + expect(restoredTabs.some((tab) => tab.id === closedTabId)).toBe(false) + + const afterRestart = await client.call('terminal.list', { + worktree: `id:${worktreeId}` + }) + expect( + afterRestart.result.terminals.filter((terminal) => terminal.tabId === closedTabId) + ).toEqual([]) } finally { if (firstApp) { await session.close(firstApp)