diff --git a/.github/workflows/release-cut.yml b/.github/workflows/release-cut.yml index 96af9bd0cd1..a107536f302 100644 --- a/.github/workflows/release-cut.yml +++ b/.github/workflows/release-cut.yml @@ -878,6 +878,14 @@ jobs: } Get-Item -LiteralPath $file } + # The standalone node.exe that hosts the terminal daemon outside the + # install-dir kill zone the NSIS updater sweeps. See + # src/main/daemon/daemon-host-relocation.ts. + $daemonHostNode = 'dist/win-unpacked/resources/daemon-host/node.exe' + if (-not (Test-Path -LiteralPath $daemonHostNode -PathType Leaf)) { + throw "Missing Windows daemon-host runtime file: $daemonHostNode" + } + Get-Item -LiteralPath $daemonHostNode - name: Install SignPath PowerShell module if: matrix.platform == 'win' diff --git a/config/daemon-host-node-runtime.cjs b/config/daemon-host-node-runtime.cjs new file mode 100644 index 00000000000..944d9a94d5b --- /dev/null +++ b/config/daemon-host-node-runtime.cjs @@ -0,0 +1,41 @@ +const { copyFileSync, existsSync, mkdirSync } = require('node:fs') +const { join } = require('node:path') + +// Where the standalone daemon-host node.exe is staged under packaged resources. +// Mirrored at runtime by src/main/daemon/daemon-host-relocation.ts, which copies +// it into userData/daemon-host// and forks the terminal daemon from it. +const DAEMON_HOST_DIR = 'daemon-host' +const DAEMON_HOST_NODE_EXE = 'node.exe' + +/** + * Stages a standalone Windows node.exe into resources/daemon-host so the + * detached terminal daemon can be forked from a userData copy that lives + * outside the install directory the NSIS updater force-closes mid-update. + * + * Source is the build host's own node.exe (process.execPath). It is version- + * correct by construction: engines.node pins the build to Node 24.x, whose NAPI + * 10 runtime loads node-pty's Electron-42-built conpty.node. Being an official + * Node binary, it also carries the OpenJS Foundation Authenticode signature, so + * it ships validly signed regardless of whether SignPath re-signs nested PEs. + * + * Windows x64 only — the sole Windows build target. Other targets ship no + * node.exe and the runtime falls open to forking the install-dir Electron host. + */ +function ensurePackagedDaemonHostNode(resourcesDir, electronPlatformName) { + if (electronPlatformName !== 'win32') { + return + } + const nodeExePath = process.execPath + // The build must run under standalone Node (not Electron) so execPath is a + // real node.exe with a NAPI runtime; anything else would strand the daemon. + if (!/node\.exe$/i.test(nodeExePath) || !existsSync(nodeExePath)) { + throw new Error( + `[daemon-host] cannot stage node.exe: build host execPath is not a node.exe (${nodeExePath})` + ) + } + const destDir = join(resourcesDir, DAEMON_HOST_DIR) + mkdirSync(destDir, { recursive: true }) + copyFileSync(nodeExePath, join(destDir, DAEMON_HOST_NODE_EXE)) +} + +module.exports = { ensurePackagedDaemonHostNode, DAEMON_HOST_DIR, DAEMON_HOST_NODE_EXE } diff --git a/config/electron-builder.config.cjs b/config/electron-builder.config.cjs index 9e482825a73..9880f0d9de4 100644 --- a/config/electron-builder.config.cjs +++ b/config/electron-builder.config.cjs @@ -7,6 +7,7 @@ const { prunePackagedRuntimeNodeModules, verifyPackagedMainRuntimeDeps } = require('./packaged-runtime-node-modules.cjs') +const { ensurePackagedDaemonHostNode } = require('./daemon-host-node-runtime.cjs') const isMacRelease = process.env.ORCA_MAC_RELEASE === '1' const isLinuxArm64Release = process.env.ORCA_LINUX_ARM64_RELEASE === '1' @@ -128,6 +129,10 @@ module.exports = { return } prunePackagedRuntimeNodeModules(resourcesDir, context.electronPlatformName, context.arch) + // Why: stage the standalone node.exe that hosts the terminal daemon outside + // the install-dir kill zone the NSIS updater sweeps (win32 only; no-ops + // elsewhere). See src/main/daemon/daemon-host-relocation.ts. + ensurePackagedDaemonHostNode(resourcesDir, context.electronPlatformName) verifyPackagedMainRuntimeDeps(resourcesDir) chmodUnixCliLaunchers(resourcesDir, context.electronPlatformName) chmodMacServeSimHelpers(resourcesDir, context.electronPlatformName) diff --git a/config/scripts/electron-builder-config.test.mjs b/config/scripts/electron-builder-config.test.mjs index 3f318219eeb..ce03bd6b49a 100644 --- a/config/scripts/electron-builder-config.test.mjs +++ b/config/scripts/electron-builder-config.test.mjs @@ -17,6 +17,22 @@ const { prunePackagedZodSources, verifyPackagedMainRuntimeDeps } = require('../packaged-runtime-node-modules.cjs') +const { ensurePackagedDaemonHostNode } = require('../daemon-host-node-runtime.cjs') + +// process.execPath is read-only in the types; point it at a fixture node.exe. +function withStubbedExecPath(value, fn) { + const original = process.execPath + Object.defineProperty(process, 'execPath', { value, configurable: true, writable: true }) + try { + return fn() + } finally { + Object.defineProperty(process, 'execPath', { + value: original, + configurable: true, + writable: true + }) + } +} describe('electron-builder config', () => { it('excludes repo-only source trees from app.asar', () => { @@ -207,6 +223,52 @@ describe('electron-builder config', () => { } }) + it('stages the build host node.exe into resources/daemon-host for Windows', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-daemon-host-node-')) + try { + const fakeHostDir = join(root, 'host') + await mkdir(fakeHostDir, { recursive: true }) + const fakeNodeExe = join(fakeHostDir, 'node.exe') + await writeFile(fakeNodeExe, 'fake-node-binary', 'utf8') + const resourcesDir = join(root, 'resources') + await mkdir(resourcesDir, { recursive: true }) + + withStubbedExecPath(fakeNodeExe, () => ensurePackagedDaemonHostNode(resourcesDir, 'win32')) + + await expect(readFile(join(resourcesDir, 'daemon-host', 'node.exe'), 'utf8')).resolves.toBe( + 'fake-node-binary' + ) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + + it('does not stage a daemon-host node.exe on non-Windows platforms', async () => { + const resourcesDir = await mkdtemp(join(tmpdir(), 'orca-daemon-host-node-skip-')) + try { + ensurePackagedDaemonHostNode(resourcesDir, 'linux') + await expect(readdir(resourcesDir)).resolves.toEqual([]) + } finally { + await rm(resourcesDir, { recursive: true, force: true }) + } + }) + + it('fails the Windows build when the host binary is not a node.exe', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-daemon-host-node-bad-')) + try { + const notNode = join(root, 'electron.exe') + await writeFile(notNode, 'not-node', 'utf8') + const resourcesDir = join(root, 'resources') + await mkdir(resourcesDir, { recursive: true }) + + expect(() => + withStubbedExecPath(notNode, () => ensurePackagedDaemonHostNode(resourcesDir, 'win32')) + ).toThrow(/not a node\.exe/) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + it('includes @parcel/watcher in the packaged runtime closure', () => { // Why: the main process imports '@parcel/watcher' for filesystem change // events; if it is absent from the packaged closure the serve host silently diff --git a/config/scripts/package-electron-runtime-contract.test.mjs b/config/scripts/package-electron-runtime-contract.test.mjs index c106b166feb..43a898dbca2 100644 --- a/config/scripts/package-electron-runtime-contract.test.mjs +++ b/config/scripts/package-electron-runtime-contract.test.mjs @@ -178,6 +178,9 @@ describe('Electron runtime package contract', () => { 'dist/win-unpacked/resources/node_modules/node-pty/build/Release' ) expect(steps[verifyNodePtyIndex].run).toContain('conpty/conpty.dll') + // The daemon-host node.exe must be staged alongside the node-pty runtime so + // the terminal daemon can be forked from outside the install-dir kill zone. + expect(steps[verifyNodePtyIndex].run).toContain('resources/daemon-host/node.exe') const uploadThroughDownloadScript = steps .slice(uploadIndex, downloadIndex + 1) diff --git a/package.json b/package.json index 8744db177eb..b6a585da685 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "orca", - "version": "1.4.124-rc.2", + "version": "1.4.124-rc.5", "description": "Next-gen IDE for parallel agentic development", "homepage": "https://github.com/stablyai/orca", "author": "stablyai", diff --git a/src/main/daemon/daemon-host-relocation.test.ts b/src/main/daemon/daemon-host-relocation.test.ts new file mode 100644 index 00000000000..9656164091f --- /dev/null +++ b/src/main/daemon/daemon-host-relocation.test.ts @@ -0,0 +1,100 @@ +import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import os from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// Mutable Electron app stub, hoisted so the vi.mock factory can close over it. +const { electronApp } = vi.hoisted(() => ({ + electronApp: { + isPackaged: false, + userDataPath: '', + version: '0.0.0-test', + getPath: (): string => electronApp.userDataPath, + getVersion: (): string => electronApp.version + } +})) + +vi.mock('electron', () => ({ app: electronApp })) + +import { resolveDaemonHostSourceDir } from './daemon-host-relocation' + +let tempDir: string + +beforeEach(() => { + tempDir = mkdtempSync(join(os.tmpdir(), 'daemon-host-relocation-')) +}) + +afterEach(() => { + vi.unstubAllEnvs() + try { + rmSync(tempDir, { recursive: true, force: true }) + } catch {} +}) + +describe('resolveDaemonHostSourceDir', () => { + it('returns the daemon-host dir when it holds a node.exe', () => { + const resources = join(tempDir, 'resources') + mkdirSync(join(resources, 'daemon-host'), { recursive: true }) + writeFileSync(join(resources, 'daemon-host', 'node.exe'), 'host') + expect(resolveDaemonHostSourceDir(resources)).toBe(join(resources, 'daemon-host')) + }) + + it('returns null when the daemon-host dir has no node.exe', () => { + const resources = join(tempDir, 'resources') + mkdirSync(join(resources, 'daemon-host'), { recursive: true }) + expect(resolveDaemonHostSourceDir(resources)).toBeNull() + }) + + it('returns null when the daemon-host dir is absent', () => { + expect(resolveDaemonHostSourceDir(join(tempDir, 'resources'))).toBeNull() + }) +}) + +// process.resourcesPath is typed read-only; point it at a fixture for the test. +function stubResourcesPath(value: string | undefined): void { + Object.defineProperty(process, 'resourcesPath', { value, configurable: true, writable: true }) +} + +// Copies a real node.exe path shape; the win32 guard in the module means this +// only exercises the relocation path on Windows. +describe.runIf(process.platform === 'win32')('installRelocatedDaemonHost', () => { + const originalResourcesPath = process.resourcesPath + + afterEach(() => { + stubResourcesPath(originalResourcesPath) + vi.resetModules() + }) + + it('relocates a bundled node.exe and exposes its userData path', async () => { + const resources = join(tempDir, 'resources') + mkdirSync(join(resources, 'daemon-host'), { recursive: true }) + writeFileSync(join(resources, 'daemon-host', 'node.exe'), 'fake-node-host') + stubResourcesPath(resources) + electronApp.userDataPath = join(tempDir, 'userData') + electronApp.version = '9.9.9' + vi.stubEnv('ORCA_FORCE_DAEMON_HOST_RELOCATION', '1') + + // Fresh module so the one-shot install/singleton state is not carried over. + vi.resetModules() + const mod = await import('./daemon-host-relocation') + mod.installRelocatedDaemonHost() + + const execPath = mod.getRelocatedDaemonHostExecPath() + expect(execPath).toBe(join(electronApp.userDataPath, 'daemon-host', '9.9.9', 'node.exe')) + expect(readFileSync(execPath!, 'utf8')).toBe('fake-node-host') + }) + + it('fails open to null when no bundled node.exe is present', async () => { + const resources = join(tempDir, 'resources') + mkdirSync(resources, { recursive: true }) + stubResourcesPath(resources) + electronApp.userDataPath = join(tempDir, 'userData') + vi.stubEnv('ORCA_FORCE_DAEMON_HOST_RELOCATION', '1') + + vi.resetModules() + const mod = await import('./daemon-host-relocation') + mod.installRelocatedDaemonHost() + + expect(mod.getRelocatedDaemonHostExecPath()).toBeNull() + }) +}) diff --git a/src/main/daemon/daemon-host-relocation.ts b/src/main/daemon/daemon-host-relocation.ts new file mode 100644 index 00000000000..21cdcffe3c7 --- /dev/null +++ b/src/main/daemon/daemon-host-relocation.ts @@ -0,0 +1,83 @@ +import { existsSync } from 'node:fs' +import { join } from 'node:path' +import { app } from 'electron' +import { ensureRelocatedRuntime } from '../pty/install-dir-runtime-relocation' + +/** + * Relocates the Node host that runs the detached terminal daemon out of the + * app install directory into userData. + * + * Why: the daemon is `fork()`ed as plain Node via ELECTRON_RUN_AS_NODE, so its + * process image is the install-dir Orca.exe. The NSIS update installer + * force-closes every process whose image path is under the install directory + * before replacing files, which kills the daemon — and every live terminal it + * owns — mid-update. Running the daemon from a version-keyed node.exe staged in + * userData takes its image out of the installer's kill zone so it survives. + * + * Fail-open: if no bundled node.exe is present (dev, or a build that predates + * shipping it), relocation no-ops and the caller keeps forking the install-dir + * Orca.exe host — the pre-relocation behavior, with zero regression. + */ +const RELOCATED_DAEMON_HOST_EXE = 'node.exe' + +let relocatedDaemonHostExecPath: string | null = null +let installed = false + +/** + * The bundled daemon-host dir under app resources, or null when no node.exe is + * shipped there. Kept separate from install so the resources path is testable. + */ +export function resolveDaemonHostSourceDir(resourcesPath: string): string | null { + const dir = join(resourcesPath, 'daemon-host') + return existsSync(join(dir, RELOCATED_DAEMON_HOST_EXE)) ? dir : null +} + +/** + * Copies the bundled daemon-host node.exe into a version-keyed userData dir + * (once) and records its path for the daemon fork. Safe to call more than once; + * only the first call does work. + */ +export function installRelocatedDaemonHost(): void { + if (installed) { + return + } + installed = true + if (process.platform !== 'win32') { + return + } + // The install-dir kill zone only exists for packaged installs; a dev override + // lets the relocation path be exercised without a full NSIS build. + if (!app.isPackaged && process.env.ORCA_FORCE_DAEMON_HOST_RELOCATION !== '1') { + return + } + // Why: fail-open contract — this must never throw. resourcesPath is unset + // outside a real Electron process (tests, node-hosted tooling). + if (typeof process.resourcesPath !== 'string') { + return + } + const sourceDir = resolveDaemonHostSourceDir(process.resourcesPath) + if (!sourceDir) { + return + } + const userData = app.getPath('userData') + const destDir = ensureRelocatedRuntime({ + sourceDir, + destRoot: join(userData, 'daemon-host'), + version: app.getVersion(), + // Same runtimeDir daemon-init writes daemon-v.pid into, so a surviving + // daemon's host dir is pinned by its recorded appVersion and never reclaimed + // while it runs. + daemonRuntimeDir: join(userData, 'daemon') + }) + if (destDir) { + relocatedDaemonHostExecPath = join(destDir, RELOCATED_DAEMON_HOST_EXE) + } +} + +/** + * The relocated node.exe to fork the daemon from, or null to fall back to the + * install-dir Electron host (ELECTRON_RUN_AS_NODE). + */ +export function getRelocatedDaemonHostExecPath(): string | null { + return relocatedDaemonHostExecPath +} diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index ff18ecc3397..68c492a249e 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -36,6 +36,11 @@ import { isDaemonStaleForCurrentBundle, killStaleDaemon } from './daemon-health' +import { + getRelocatedDaemonHostExecPath, + installRelocatedDaemonHost +} from './daemon-host-relocation' +import { startSpan } from '../observability/tracer' import { DegradedDaemonPtyProvider } from './degraded-daemon-pty-provider' import { getLocalPtyProvider, @@ -153,6 +158,16 @@ function createPreservedDaemonHandle( protocolVersion = PROTOCOL_VERSION, mode?: 'degraded-new-pty-fallback' ): DaemonProcessHandle { + // Why: the trace file is the only artifact available in field + // investigations of update-survival incidents; adopt-vs-fresh-fork is the + // first question every one of them asks. + const adoptSpan = startSpan('daemon.adopt', { + attributes: { + 'daemon.protocol_version': protocolVersion, + ...(mode ? { 'daemon.mode': mode } : {}) + } + }) + adoptSpan.end() const handle: DaemonProcessHandle = { shutdown: async () => { await cleanupDaemonForProtocol(runtimeDir, protocolVersion) @@ -260,6 +275,25 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { await killStaleDaemon(runtimeDir, socketPath, tokenPath) const userDataPath = app.getPath('userData') + // Why: staged here instead of app startup so the ~70MB node.exe copy stays + // off the first-paint path, and is skipped entirely on launches that adopt + // a live daemon (no fork). Latched — repeat calls are free. + installRelocatedDaemonHost() + // Why: on Windows a relocated node.exe in userData hosts the daemon so its + // process image escapes the install-dir the NSIS updater force-closes; when + // absent (dev, non-win32, pre-relocation builds) fall back to the install-dir + // Electron host run as plain Node via ELECTRON_RUN_AS_NODE. + const relocatedHostExecPath = getRelocatedDaemonHostExecPath() + // Why: without this span a silent fallback to the install-dir host (back + // inside the updater kill zone) is indistinguishable from the fixed path + // in field trace files. + const forkSpan = startSpan('daemon.fork', { + attributes: { + 'daemon.host': relocatedHostExecPath ? 'relocated-node' : 'install-dir-electron', + ...(relocatedHostExecPath ? { 'daemon.host_exec_path': relocatedHostExecPath } : {}), + 'app.version': app.getVersion() + } + }) const child = fork(entryPath, ['--socket', socketPath, '--token', tokenPath], { // Why: detached daemons can outlive dev worktrees. Starting from // userData keeps process.cwd() valid after a repo/worktree is deleted. @@ -269,13 +303,17 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { // open, which would prevent Electron from exiting cleanly. detached: true, stdio: ['ignore', 'ignore', 'ignore', 'ipc'], - // Why: ELECTRON_RUN_AS_NODE makes the forked process run as a plain - // Node.js process instead of an Electron renderer/main process. Without - // it, Electron's GPU/display initialization can interfere with native - // module operations like node-pty's posix_spawn of the spawn-helper. + // Why: a standalone node.exe is already plain Node; reset execArgv so the + // Electron main process's node flags don't leak into it. + ...(relocatedHostExecPath ? { execPath: relocatedHostExecPath, execArgv: [] } : {}), env: { ...process.env, - ELECTRON_RUN_AS_NODE: '1', + // Why: ELECTRON_RUN_AS_NODE makes the forked Electron binary run as a + // plain Node.js process instead of an Electron main process. Without it, + // Electron's GPU/display initialization can interfere with native module + // operations like node-pty's posix_spawn of the spawn-helper. A relocated + // node.exe is already plain Node, so the var is omitted there. + ...(relocatedHostExecPath ? {} : { ELECTRON_RUN_AS_NODE: '1' }), // Why: the detached daemon is plain Node and cannot call Electron's // app.getPath(), but shell-ready rcfiles must live outside swept tmp. ORCA_USER_DATA_PATH: userDataPath @@ -307,6 +345,7 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { // Already dead } } + forkSpan.fail(error) reject(error) } function onReadyMessage(msg: unknown): void { @@ -318,6 +357,10 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { // Why: the daemon process is detached after readiness; leaving // startup listeners attached retains this launch promise closure. cleanupStartupListeners() + if (child.pid !== undefined) { + forkSpan.setAttribute('daemon.pid', child.pid) + } + forkSpan.end() if (child.pid) { // Why: JSON pid file carries pid + process start time so later // killStaleDaemon() can verify the pid still belongs to the daemon diff --git a/src/main/index.ts b/src/main/index.ts index a361df3e3ac..7eccdffc7a5 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -578,6 +578,9 @@ if (hasSingleInstanceLock) { initOpenCodeUsagePath() // Why: must run before anything loads node-pty (first PTY spawn) so main // and the daemon it forks both pick up ORCA_NODE_PTY_NATIVE_DIR. + // (The daemon-host node.exe relocation deliberately does NOT run here: its + // ~70MB copy would sit on the first-paint path. daemon-init stages it at + // fork time instead.) installRelocatedNodePtyNativeRuntime() crashReports = CrashReportStore.fromUserData() recordCrashBreadcrumb('app_started', { diff --git a/src/main/pty/install-dir-runtime-relocation.test.ts b/src/main/pty/install-dir-runtime-relocation.test.ts new file mode 100644 index 00000000000..0844b5a8322 --- /dev/null +++ b/src/main/pty/install-dir-runtime-relocation.test.ts @@ -0,0 +1,251 @@ +import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync, existsSync } from 'node:fs' +import os from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { getDaemonPidPath, serializeDaemonPidFile } from '../daemon/daemon-spawner' +import type { DaemonPidFile } from '../daemon/daemon-spawner' +import { + collectInUseRuntimeVersions, + ensureRelocatedRuntime +} from './install-dir-runtime-relocation' + +let tempDir: string + +beforeEach(() => { + tempDir = mkdtempSync(join(os.tmpdir(), 'install-dir-relocation-')) +}) + +afterEach(() => { + try { + rmSync(tempDir, { recursive: true, force: true }) + } catch {} +}) + +// A generic runtime tree with a nested dir and a .pdb symbol file, to exercise +// recursive copy and the symbol-exclusion rule independent of any one runtime. +function seedSourceDir(dir: string): void { + mkdirSync(join(dir, 'nested'), { recursive: true }) + writeFileSync(join(dir, 'runtime.exe'), 'host') + writeFileSync(join(dir, 'runtime.dll'), 'lib') + writeFileSync(join(dir, 'runtime.pdb'), 'symbols') + writeFileSync(join(dir, 'nested', 'inner.bin'), 'inner') +} + +// Writes a daemon pid file exactly as the daemon does (userData/daemon/daemon-v.pid). +function writeDaemonPidFile( + daemonRuntimeDir: string, + protocolVersion: number, + pidFile: DaemonPidFile +): void { + mkdirSync(daemonRuntimeDir, { recursive: true }) + writeFileSync( + getDaemonPidPath(daemonRuntimeDir, protocolVersion), + serializeDaemonPidFile(pidFile) + ) +} + +// Deterministic liveness that treats the given pids as running, so tests never +// depend on the host's real process table. +function aliveFor(...alivePids: number[]): (pid: number) => boolean { + const set = new Set(alivePids) + return (pid) => set.has(pid) +} + +describe('collectInUseRuntimeVersions', () => { + it('returns an empty set when the daemon dir does not exist', () => { + expect(collectInUseRuntimeVersions(join(tempDir, 'no-daemon-dir')).size).toBe(0) + }) + + it('collects the app version a live daemon pins', () => { + const daemonDir = join(tempDir, 'daemon') + writeDaemonPidFile(daemonDir, 18, { pid: 4321, startedAtMs: null, appVersion: '1.4.124-rc.1' }) + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(4321)) + + expect([...inUse]).toEqual(['1.4.124-rc.1']) + }) + + it('omits the version of a daemon whose pid is dead', () => { + const daemonDir = join(tempDir, 'daemon') + writeDaemonPidFile(daemonDir, 18, { pid: 4321, startedAtMs: null, appVersion: '1.4.124-rc.1' }) + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(/* nobody */)) + + expect(inUse.size).toBe(0) + }) + + it('collects one version per live daemon across protocol versions', () => { + const daemonDir = join(tempDir, 'daemon') + writeDaemonPidFile(daemonDir, 18, { pid: 100, startedAtMs: null, appVersion: '1.4.124-rc.1' }) + writeDaemonPidFile(daemonDir, 19, { pid: 200, startedAtMs: null, appVersion: '1.4.124-rc.2' }) + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(100, 200)) + + expect([...inUse].sort()).toEqual(['1.4.124-rc.1', '1.4.124-rc.2']) + }) + + it('ignores a daemon whose pid file records no app version', () => { + const daemonDir = join(tempDir, 'daemon') + // Pre-relocation daemon: JSON pid file without appVersion. + writeDaemonPidFile(daemonDir, 18, { pid: 4321, startedAtMs: null }) + // Legacy daemon: bare-integer pid file (appVersion parses to null). + mkdirSync(daemonDir, { recursive: true }) + writeFileSync(getDaemonPidPath(daemonDir, 17), '9999') + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(4321, 9999)) + + expect(inUse.size).toBe(0) + }) + + it('ignores non-pid files in the daemon dir', () => { + const daemonDir = join(tempDir, 'daemon') + mkdirSync(daemonDir, { recursive: true }) + writeFileSync(join(daemonDir, 'daemon-v18.token'), 'secret') + writeFileSync(join(daemonDir, 'daemon-v18.sock'), 'x') + writeFileSync(join(daemonDir, 'notes.txt'), 'x') + + expect(collectInUseRuntimeVersions(daemonDir, aliveFor(1, 2, 3)).size).toBe(0) + }) + + it('skips a malformed pid file without throwing', () => { + const daemonDir = join(tempDir, 'daemon') + mkdirSync(daemonDir, { recursive: true }) + writeFileSync(getDaemonPidPath(daemonDir, 18), 'not-a-pid-at-all') + writeDaemonPidFile(daemonDir, 19, { pid: 200, startedAtMs: null, appVersion: '2.0.0' }) + + const inUse = collectInUseRuntimeVersions(daemonDir, aliveFor(200)) + + expect([...inUse]).toEqual(['2.0.0']) + }) + + it('treats the current process as alive under the default liveness probe', () => { + const daemonDir = join(tempDir, 'daemon') + // startedAtMs null so startTimeMatches short-circuits true on every platform. + writeDaemonPidFile(daemonDir, 18, { + pid: process.pid, + startedAtMs: null, + appVersion: '3.0.0' + }) + + expect([...collectInUseRuntimeVersions(daemonDir)]).toEqual(['3.0.0']) + }) +}) + +describe('ensureRelocatedRuntime', () => { + it('copies the runtime tree (without symbols) and returns the version dir', () => { + const sourceDir = join(tempDir, 'source') + seedSourceDir(sourceDir) + const destRoot = join(tempDir, 'dest') + + const destDir = ensureRelocatedRuntime({ + sourceDir, + destRoot, + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) + + expect(destDir).toBe(join(destRoot, '1.2.3')) + expect(readFileSync(join(destDir!, 'runtime.exe'), 'utf8')).toBe('host') + expect(readFileSync(join(destDir!, 'runtime.dll'), 'utf8')).toBe('lib') + expect(readFileSync(join(destDir!, 'nested', 'inner.bin'), 'utf8')).toBe('inner') + expect(existsSync(join(destDir!, 'runtime.pdb'))).toBe(false) + }) + + it('skips recopying once the completion marker exists', () => { + const sourceDir = join(tempDir, 'source') + seedSourceDir(sourceDir) + const destRoot = join(tempDir, 'dest') + ensureRelocatedRuntime({ + sourceDir, + destRoot, + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) + + writeFileSync(join(sourceDir, 'runtime.exe'), 'changed-after-first-copy') + const destDir = ensureRelocatedRuntime({ + sourceDir, + destRoot, + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) + + expect(readFileSync(join(destDir!, 'runtime.exe'), 'utf8')).toBe('host') + }) + + it('redoes an interrupted copy that has no completion marker', () => { + const sourceDir = join(tempDir, 'source') + seedSourceDir(sourceDir) + const destRoot = join(tempDir, 'dest') + mkdirSync(join(destRoot, '1.2.3'), { recursive: true }) + writeFileSync(join(destRoot, '1.2.3', 'runtime.exe'), 'torn partial copy') + + const destDir = ensureRelocatedRuntime({ + sourceDir, + destRoot, + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) + + expect(readFileSync(join(destDir!, 'runtime.exe'), 'utf8')).toBe('host') + }) + + it('reclaims a stale version dir once no live daemon pins it', () => { + const sourceDir = join(tempDir, 'source') + seedSourceDir(sourceDir) + const destRoot = join(tempDir, 'dest') + const daemonRuntimeDir = join(tempDir, 'daemon') + ensureRelocatedRuntime({ sourceDir, destRoot, version: '1.0.0', daemonRuntimeDir }) + + // The 1.0.0 daemon exited, so its pid is dead — nothing pins 1.0.0. + writeDaemonPidFile(daemonRuntimeDir, 18, { pid: 4321, startedAtMs: null, appVersion: '1.0.0' }) + + const destDir = ensureRelocatedRuntime({ + sourceDir, + destRoot, + version: '2.0.0', + daemonRuntimeDir, + isDaemonPidAlive: aliveFor(/* 4321 is dead */) + }) + + expect(destDir).toBe(join(destRoot, '2.0.0')) + expect(existsSync(join(destRoot, '1.0.0'))).toBe(false) + expect(existsSync(join(destRoot, '2.0.0', 'runtime.exe'))).toBe(true) + }) + + it('preserves a stale version dir a surviving daemon still pins', () => { + const sourceDir = join(tempDir, 'source') + seedSourceDir(sourceDir) + const destRoot = join(tempDir, 'dest') + const daemonRuntimeDir = join(tempDir, 'daemon') + ensureRelocatedRuntime({ sourceDir, destRoot, version: '1.0.0', daemonRuntimeDir }) + + // The 1.0.0 daemon survived the update and still loads its 1.0.0 runtime dir + // on demand — deleting it would strand the running daemon. + writeDaemonPidFile(daemonRuntimeDir, 18, { pid: 4321, startedAtMs: null, appVersion: '1.0.0' }) + + const destDir = ensureRelocatedRuntime({ + sourceDir, + destRoot, + version: '2.0.0', + daemonRuntimeDir, + isDaemonPidAlive: aliveFor(4321) + }) + + expect(destDir).toBe(join(destRoot, '2.0.0')) + expect(existsSync(join(destRoot, '1.0.0', 'runtime.exe'))).toBe(true) + expect(existsSync(join(destRoot, '2.0.0', 'runtime.exe'))).toBe(true) + }) + + it('fails open when the source dir is missing', () => { + expect( + ensureRelocatedRuntime({ + sourceDir: join(tempDir, 'does-not-exist'), + destRoot: join(tempDir, 'dest'), + version: '1.2.3', + daemonRuntimeDir: join(tempDir, 'daemon') + }) + ).toBeNull() + }) +}) diff --git a/src/main/pty/install-dir-runtime-relocation.ts b/src/main/pty/install-dir-runtime-relocation.ts new file mode 100644 index 00000000000..21ce82fd03e --- /dev/null +++ b/src/main/pty/install-dir-runtime-relocation.ts @@ -0,0 +1,145 @@ +import { + copyFileSync, + existsSync, + mkdirSync, + readdirSync, + readFileSync, + rmSync, + writeFileSync +} from 'node:fs' +import { join } from 'node:path' +import { parseDaemonPidFile, startTimeMatches } from '../daemon/daemon-health' + +/** + * Relocates a per-version runtime directory out of the app install directory + * into userData, and reclaims old version dirs no live daemon still pins. + * + * Why: the Windows NSIS update installer force-closes every process (and, for + * the terminal runtime, every native binary loaded from a process) whose image + * lives under the install directory before replacing files. Copying the runtime + * to a version-keyed userData dir takes it out of that kill zone so the detached + * daemon and its PTYs survive updates. Both node-pty's native runtime and the + * daemon's own Node host share this machinery. + */ +export const RELOCATION_COMPLETE_MARKER = '.relocation-complete' + +function copyRuntimeTree(sourceDir: string, destDir: string): void { + mkdirSync(destDir, { recursive: true }) + for (const entry of readdirSync(sourceDir, { withFileTypes: true })) { + const sourcePath = join(sourceDir, entry.name) + const destPath = join(destDir, entry.name) + if (entry.isDirectory()) { + copyRuntimeTree(sourcePath, destPath) + } else if (entry.isFile() && !/\.pdb$/i.test(entry.name)) { + copyFileSync(sourcePath, destPath) + } + } +} + +export type IsDaemonPidAlive = (pid: number, startedAtMs: number | null) => boolean + +function isDaemonPidAliveDefault(pid: number, startedAtMs: number | null): boolean { + try { + process.kill(pid, 0) + } catch { + return false + } + return startTimeMatches(pid, startedAtMs) +} + +/** + * Runtime version dirs still claimed by a live daemon, read from the daemon + * pid files under `daemonRuntimeDir` (`daemon-v.pid`). + * + * Why: a daemon deliberately survives app updates, and it loads its version's + * relocated runtime (node-pty binaries, and — once relocated — its own Node + * host) on demand. Each pid file records that `appVersion`, so a live daemon's + * version dir must never be reclaimed while the process is still running. + */ +export function collectInUseRuntimeVersions( + daemonRuntimeDir: string, + isPidAlive: IsDaemonPidAlive = isDaemonPidAliveDefault +): Set { + const inUse = new Set() + let entries + try { + entries = readdirSync(daemonRuntimeDir, { withFileTypes: true }) + } catch { + return inUse + } + for (const entry of entries) { + if (!entry.isFile() || !/^daemon-v\d+\.pid$/.test(entry.name)) { + continue + } + let parsed + try { + parsed = parseDaemonPidFile(readFileSync(join(daemonRuntimeDir, entry.name), 'utf8')) + } catch { + continue + } + // appVersion null => a pre-relocation daemon that loads from the install + // dir and thus pins no version dir here. + if (parsed && parsed.appVersion !== null && isPidAlive(parsed.pid, parsed.startedAtMs)) { + inUse.add(parsed.appVersion) + } + } + return inUse +} + +function removeStaleRuntimeVersions( + destRoot: string, + keepVersion: string, + inUseVersions: ReadonlySet +): void { + let entries + try { + entries = readdirSync(destRoot, { withFileTypes: true }) + } catch { + return + } + for (const entry of entries) { + if (!entry.isDirectory() || entry.name === keepVersion || inUseVersions.has(entry.name)) { + continue + } + // Why: Windows maps the daemon's binaries with FILE_SHARE_DELETE, so a + // rename/delete succeeds even while a surviving daemon still needs them — + // a rename heuristic would delete in-use dirs and strand the daemon. Only + // dirs no live daemon claims via its pid file are safe to remove. + try { + rmSync(join(destRoot, entry.name), { recursive: true, force: true }) + } catch { + // Still locked or already gone — retry on a future launch. + } + } +} + +/** + * Ensures `sourceDir` is copied to `destRoot/version` (once, guarded by a + * completion marker) and reclaims sibling version dirs no live daemon pins. + * Returns the version dir, or null on any failure (callers fail open to + * loading from the install dir — the pre-relocation behavior). + */ +export function ensureRelocatedRuntime(options: { + sourceDir: string + destRoot: string + version: string + daemonRuntimeDir: string + isDaemonPidAlive?: IsDaemonPidAlive +}): string | null { + const { sourceDir, destRoot, version, daemonRuntimeDir, isDaemonPidAlive } = options + const destDir = join(destRoot, version) + try { + // Why: the marker is written only after a full copy, so a crash mid-copy + // leaves no marker and the next launch redoes the copy from scratch. + if (!existsSync(join(destDir, RELOCATION_COMPLETE_MARKER))) { + rmSync(destDir, { recursive: true, force: true }) + copyRuntimeTree(sourceDir, destDir) + writeFileSync(join(destDir, RELOCATION_COMPLETE_MARKER), '') + } + const inUseVersions = collectInUseRuntimeVersions(daemonRuntimeDir, isDaemonPidAlive) + removeStaleRuntimeVersions(destRoot, version, inUseVersions) + return destDir + } catch { + return null + } +} diff --git a/src/main/pty/node-pty-runtime-relocation.test.ts b/src/main/pty/node-pty-runtime-relocation.test.ts index 1dfbe033885..e5f9fbd61ac 100644 --- a/src/main/pty/node-pty-runtime-relocation.test.ts +++ b/src/main/pty/node-pty-runtime-relocation.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync, existsSync } from 'node:fs' +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs' import { createRequire } from 'node:module' import os from 'node:os' import { dirname, join } from 'node:path' @@ -12,10 +12,8 @@ vi.mock('electron', () => ({ } })) -import { - ensureRelocatedNodePtyNativeRuntime, - resolveNodePtyNativeSourceDir -} from './node-pty-runtime-relocation' +import { ensureRelocatedRuntime } from './install-dir-runtime-relocation' +import { resolveNodePtyNativeSourceDir } from './node-pty-runtime-relocation' let tempDir: string @@ -31,17 +29,6 @@ afterEach(() => { } catch {} }) -function seedSourceDir(dir: string): void { - mkdirSync(join(dir, 'conpty'), { recursive: true }) - writeFileSync(join(dir, 'conpty.node'), 'binding') - writeFileSync(join(dir, 'conpty_console_list.node'), 'listing') - writeFileSync(join(dir, 'pty.node'), 'winpty-binding') - writeFileSync(join(dir, 'winpty-agent.exe'), 'agent') - writeFileSync(join(dir, 'conpty.pdb'), 'symbols') - writeFileSync(join(dir, 'conpty', 'conpty.dll'), 'dll') - writeFileSync(join(dir, 'conpty', 'OpenConsole.exe'), 'console-host') -} - describe('resolveNodePtyNativeSourceDir', () => { it('prefers the rebuilt build/Release binding over prebuilds', () => { const pkg = join(tempDir, 'node-pty') @@ -66,69 +53,6 @@ describe('resolveNodePtyNativeSourceDir', () => { }) }) -describe('ensureRelocatedNodePtyNativeRuntime', () => { - it('copies the runtime tree (without symbols) and returns the version dir', () => { - const sourceDir = join(tempDir, 'source') - seedSourceDir(sourceDir) - const destRoot = join(tempDir, 'dest') - - const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.2.3' }) - - expect(destDir).toBe(join(destRoot, '1.2.3')) - expect(readFileSync(join(destDir!, 'conpty.node'), 'utf8')).toBe('binding') - expect(readFileSync(join(destDir!, 'conpty', 'OpenConsole.exe'), 'utf8')).toBe('console-host') - expect(readFileSync(join(destDir!, 'conpty', 'conpty.dll'), 'utf8')).toBe('dll') - expect(existsSync(join(destDir!, 'conpty.pdb'))).toBe(false) - }) - - it('skips recopying once the completion marker exists', () => { - const sourceDir = join(tempDir, 'source') - seedSourceDir(sourceDir) - const destRoot = join(tempDir, 'dest') - ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.2.3' }) - - writeFileSync(join(sourceDir, 'conpty.node'), 'changed-after-first-copy') - const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.2.3' }) - - expect(readFileSync(join(destDir!, 'conpty.node'), 'utf8')).toBe('binding') - }) - - it('redoes an interrupted copy that has no completion marker', () => { - const sourceDir = join(tempDir, 'source') - seedSourceDir(sourceDir) - const destRoot = join(tempDir, 'dest') - mkdirSync(join(destRoot, '1.2.3'), { recursive: true }) - writeFileSync(join(destRoot, '1.2.3', 'conpty.node'), 'torn partial copy') - - const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.2.3' }) - - expect(readFileSync(join(destDir!, 'conpty.node'), 'utf8')).toBe('binding') - }) - - it('removes stale version dirs but keeps the current one', () => { - const sourceDir = join(tempDir, 'source') - seedSourceDir(sourceDir) - const destRoot = join(tempDir, 'dest') - ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '1.0.0' }) - - const destDir = ensureRelocatedNodePtyNativeRuntime({ sourceDir, destRoot, version: '2.0.0' }) - - expect(destDir).toBe(join(destRoot, '2.0.0')) - expect(existsSync(join(destRoot, '1.0.0'))).toBe(false) - expect(existsSync(join(destRoot, '2.0.0', 'conpty.node'))).toBe(true) - }) - - it('fails open when the source dir is missing', () => { - expect( - ensureRelocatedNodePtyNativeRuntime({ - sourceDir: join(tempDir, 'does-not-exist'), - destRoot: join(tempDir, 'dest'), - version: '1.2.3' - }) - ).toBeNull() - }) -}) - // Loads the real win32 conpty binding, so it cannot run on other platforms. describe.runIf(process.platform === 'win32')('patched node-pty loader override', () => { it('loads the conpty binding from ORCA_NODE_PTY_NATIVE_DIR', () => { @@ -137,10 +61,11 @@ describe.runIf(process.platform === 'win32')('patched node-pty loader override', const sourceDir = resolveNodePtyNativeSourceDir(nodePtyPackageDir) expect(sourceDir).not.toBeNull() - const destDir = ensureRelocatedNodePtyNativeRuntime({ + const destDir = ensureRelocatedRuntime({ sourceDir: sourceDir!, destRoot: join(tempDir, 'relocated-runtime'), - version: 'loader-test' + version: 'loader-test', + daemonRuntimeDir: join(tempDir, 'daemon') }) expect(destDir).not.toBeNull() diff --git a/src/main/pty/node-pty-runtime-relocation.ts b/src/main/pty/node-pty-runtime-relocation.ts index a3821dde5d4..7c1631d7816 100644 --- a/src/main/pty/node-pty-runtime-relocation.ts +++ b/src/main/pty/node-pty-runtime-relocation.ts @@ -1,15 +1,8 @@ -import { - copyFileSync, - existsSync, - mkdirSync, - readdirSync, - renameSync, - rmSync, - writeFileSync -} from 'node:fs' +import { existsSync } from 'node:fs' import { createRequire } from 'node:module' import { dirname, join } from 'node:path' import { app } from 'electron' +import { ensureRelocatedRuntime } from './install-dir-runtime-relocation' /** * Relocates node-pty's Windows native runtime (conpty.node, conpty.dll, @@ -25,8 +18,6 @@ import { app } from 'electron' */ export const NODE_PTY_NATIVE_DIR_ENV_VAR = 'ORCA_NODE_PTY_NATIVE_DIR' -const RELOCATION_COMPLETE_MARKER = '.relocation-complete' - export function resolveNodePtyNativeSourceDir(nodePtyPackageDir: string): string | null { // Packaged builds carry the rebuilt binding in build/Release; dev installs // (and the forced-relocation test path) load from prebuilds. @@ -42,68 +33,6 @@ export function resolveNodePtyNativeSourceDir(nodePtyPackageDir: string): string return null } -function copyRuntimeTree(sourceDir: string, destDir: string): void { - mkdirSync(destDir, { recursive: true }) - for (const entry of readdirSync(sourceDir, { withFileTypes: true })) { - const sourcePath = join(sourceDir, entry.name) - const destPath = join(destDir, entry.name) - if (entry.isDirectory()) { - copyRuntimeTree(sourcePath, destPath) - } else if (entry.isFile() && !/\.pdb$/i.test(entry.name)) { - copyFileSync(sourcePath, destPath) - } - } -} - -function removeStaleRuntimeVersions(destRoot: string, keepVersion: string): void { - let entries - try { - entries = readdirSync(destRoot, { withFileTypes: true }) - } catch { - return - } - for (const entry of entries) { - if (!entry.isDirectory() || entry.name === keepVersion) { - continue - } - // Why: an adopted daemon from a previous version may still run (or later - // respawn) binaries out of its own version dir. Renaming a directory - // fails on Windows while anything inside is open, so a successful rename - // proves the dir is unused and safe to delete. - const doomedPath = join(destRoot, `${entry.name}.stale`) - try { - renameSync(join(destRoot, entry.name), doomedPath) - rmSync(doomedPath, { recursive: true, force: true }) - } catch { - // Still in use (or already being cleaned) — retry on a future launch. - } - } -} - -export function ensureRelocatedNodePtyNativeRuntime(options: { - sourceDir: string - destRoot: string - version: string -}): string | null { - const { sourceDir, destRoot, version } = options - const destDir = join(destRoot, version) - try { - // Why: the marker is written only after a full copy, so a crash mid-copy - // leaves no marker and the next launch redoes the copy from scratch. - if (!existsSync(join(destDir, RELOCATION_COMPLETE_MARKER))) { - rmSync(destDir, { recursive: true, force: true }) - copyRuntimeTree(sourceDir, destDir) - writeFileSync(join(destDir, RELOCATION_COMPLETE_MARKER), '') - } - removeStaleRuntimeVersions(destRoot, version) - return destDir - } catch { - // Fail open: node-pty keeps loading from the install dir, which is the - // pre-relocation behavior (sessions then don't survive updates). - return null - } -} - export function installRelocatedNodePtyNativeRuntime(): void { if (process.platform !== 'win32') { return @@ -126,10 +55,14 @@ export function installRelocatedNodePtyNativeRuntime(): void { if (!sourceDir) { return } - const destDir = ensureRelocatedNodePtyNativeRuntime({ + const userData = app.getPath('userData') + const destDir = ensureRelocatedRuntime({ sourceDir, - destRoot: join(app.getPath('userData'), 'node-pty-runtime'), - version: app.getVersion() + destRoot: join(userData, 'node-pty-runtime'), + version: app.getVersion(), + // Keyed to daemon-init's runtimeDir (userData/daemon), where surviving + // daemons write daemon-v.pid recording the runtime version they pin. + daemonRuntimeDir: join(userData, 'daemon') }) if (destDir) { process.env[NODE_PTY_NATIVE_DIR_ENV_VAR] = destDir diff --git a/tests/e2e/github-cli-stall-repro.spec.ts b/tests/e2e/github-cli-stall-repro.spec.ts index dd199a1817f..31a58f63b71 100644 --- a/tests/e2e/github-cli-stall-repro.spec.ts +++ b/tests/e2e/github-cli-stall-repro.spec.ts @@ -21,6 +21,10 @@ if (args[0] === 'api' && args.includes('rate_limit')) { console.log(JSON.stringify({ resources: { core: { limit: 5000, remaining: 5000, reset: 0 }, graphql: { limit: 5000, remaining: 5000, reset: 0 }, search: { limit: 30, remaining: 30, reset: 0 } } })) process.exit(0) } +if (args[0] === 'api' && joined.includes('search/issues')) { + console.log(JSON.stringify({ total_count: 0, incomplete_results: false, items: [] })) + process.exit(0) +} if (args[0] === 'issue' && args[1] === 'list') { console.log('[]') process.exit(0) @@ -113,23 +117,18 @@ test('GitHub Tasks drawer recovers when gh stalls on issue details', async ({ return { repoId: repo.id } }, testRepoPath) - // Why: GitHub item details render inline (a plain container, not a Radix - // dialog), so scope to the detail surface via its stable test id. - const drawer = orcaPage - .getByTestId('github-item-detail') - .filter({ hasText: 'Issue detail fetch that hangs in gh' }) - await expect(drawer).toBeVisible() + // The item detail opens as an inline task-detail page (no longer a modal + // dialog): the item title heading proves it mounted. + const detailHeading = orcaPage.getByRole('heading', { + name: /Issue detail fetch that hangs in gh/ + }) + await expect(detailHeading).toBeVisible({ timeout: 10_000 }) - // Why: this is the user-visible regression signal. Before ghExecFileAsync had - // a default timeout, the drawer's pending details promise never settled and - // the conversation pane stayed stuck in its loading shell. Since #6473 a - // failed detail fetch degrades to an explicit "unable to load" error rather - // than an empty shell, so the stable proof of recovery is that the drawer - // settles on that message and stops spinning instead of hanging. - await expect(drawer.getByText('Unable to load details for this GitHub item.')).toBeVisible({ + // Why: the bounded gh timeout rejects instead of hanging, so this terminal + // error text (replacing the body, not a spinner) proves the stall recovered. + await expect(orcaPage.getByText('Unable to load details for this GitHub item.')).toBeVisible({ timeout: 5_000 }) - await expect(drawer.locator('.animate-spin')).toHaveCount(0) expect(repoId).toBeTruthy() }) diff --git a/tests/e2e/helpers/worktree-registration.ts b/tests/e2e/helpers/worktree-registration.ts new file mode 100644 index 00000000000..96a4bce5e2f --- /dev/null +++ b/tests/e2e/helpers/worktree-registration.ts @@ -0,0 +1,151 @@ +import type { Page } from '@stablyai/playwright-test' +import { expect } from './orca-app' +import type { CommitMessageAiSettings } from '../../../src/shared/types' + +// Why: these specs create worktrees via raw `git worktree add`, which bypasses +// Orca's own add/remove path — the one thing that invalidates the main-process +// worktrees.list scan cache (DETECTED_WORKTREE_SCAN_CACHE_TTL_MS = 5_000). So a +// read inside the TTL window can serve a stale miss. No renderer-reachable +// cache-invalidation seam exists without adding product surface, so poll past +// the deterministic 5s boundary. Budget 10s (not 6s, which sits dangerously +// close to a 5s TTL if one scan is slow); the informative message keeps a +// genuinely never-loading worktree failing loudly. +const WORKTREE_CACHE_TTL_POLL_MS = 10_000 + +/** + * Loads the repo's worktrees into the renderer store and resolves the id of the + * worktree at `targetWorktreePath`, polling past the 5s scan-cache TTL so a + * raw `git worktree add` that Orca never observed still becomes visible. + */ +async function resolveE2eWorktreeId( + page: Page, + repoPath: string, + targetWorktreePath: string +): Promise { + let worktreeId: string | null = null + await expect + .poll( + async () => { + worktreeId = await page.evaluate( + async ({ repoPath, targetWorktreePath }) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + await store.getState().fetchRepos() + const repo = store.getState().repos.find((entry) => entry.path === repoPath) + if (!repo) { + throw new Error(`Seeded E2E repo was not registered: ${repoPath}`) + } + // Why: use the store's own fetch (like loadWorktreesUntilPathsPresent) + // so both TTL workarounds stay behaviorally identical. + await store.getState().fetchWorktrees(repo.id) + const listedWorktrees = store.getState().worktreesByRepo[repo.id] ?? [] + const normalize = (value: string): string => + value.startsWith('/private/var/') ? value.slice('/private'.length) : value + const worktree = listedWorktrees.find( + (entry) => normalize(entry.path) === normalize(targetWorktreePath) + ) + return worktree?.id ?? null + }, + { repoPath, targetWorktreePath } + ) + return worktreeId + }, + { + timeout: WORKTREE_CACHE_TTL_POLL_MS, + message: `E2E worktree was not loaded within the worktree-cache TTL window: ${targetWorktreePath}` + } + ) + .not.toBeNull() + + if (!worktreeId) { + throw new Error(`E2E worktree was not loaded: ${targetWorktreePath}`) + } + return worktreeId +} + +/** + * Shared setup for the Source Control specs: resolves the target worktree + * (surviving the scan-cache TTL race), activates it, optionally applies AI + * commit-message settings, loads its git status, and opens the Source Control + * sidebar tab. + */ +export async function openSourceControlForWorktree( + page: Page, + repoPath: string, + targetWorktreePath: string, + options: { commitMessageAi?: CommitMessageAiSettings } = {} +): Promise { + const worktreeId = await resolveE2eWorktreeId(page, repoPath, targetWorktreePath) + + await page.evaluate( + async ({ worktreeId, commitMessageAi }) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + const worktree = Object.values(store.getState().worktreesByRepo) + .flat() + .find((entry) => entry.id === worktreeId) + if (!worktree) { + throw new Error(`E2E worktree disappeared from the store: ${worktreeId}`) + } + store.getState().setActiveWorktree(worktree.id) + if (commitMessageAi) { + await store.getState().updateSettings({ commitMessageAi }) + } + const status = await window.api.git.status({ worktreePath: worktree.path }) + store.getState().setGitStatus(worktree.id, status) + store.getState().setRightSidebarTab('source-control') + store.getState().setRightSidebarOpen(true) + }, + { worktreeId, commitMessageAi: options.commitMessageAi ?? null } + ) + + await expect + .poll( + async () => + page.evaluate(() => { + const state = window.__store?.getState() + return Boolean(state?.rightSidebarOpen && state?.rightSidebarTab === 'source-control') + }), + { timeout: 5_000 } + ) + .toBe(true) +} + +/** + * Polls `fetchWorktrees` past the scan-cache TTL until every expected path is + * registered in the store. Used by the Workspace Space git-status spec, which + * adds 60 worktrees via raw git and would otherwise read a stale partial scan. + */ +export async function loadWorktreesUntilPathsPresent( + page: Page, + repoId: string, + expectedPaths: string[] +): Promise { + await expect + .poll( + async () => + page.evaluate( + async ({ repoId, expectedPaths }) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + await store.getState().fetchWorktrees(repoId) + const registered = new Set( + (store.getState().worktreesByRepo[repoId] ?? []).map((entry) => entry.path) + ) + return expectedPaths.every((entry) => registered.has(entry)) + }, + { repoId, expectedPaths } + ), + { + timeout: WORKTREE_CACHE_TTL_POLL_MS, + message: `Not all worktrees registered within the worktree-cache TTL window (${expectedPaths.length} expected)` + } + ) + .toBe(true) +} diff --git a/tests/e2e/onboarding.spec.ts b/tests/e2e/onboarding.spec.ts index 884178fbbd1..1fd2e1cc846 100644 --- a/tests/e2e/onboarding.spec.ts +++ b/tests/e2e/onboarding.spec.ts @@ -241,15 +241,17 @@ test.describe('Onboarding flow', () => { .toBe(oppositeTheme) await continueOnboarding(orcaPage) + // Why: the theme Continue persists step 2, then persists *through* any + // skipped optional steps (integrations is skipped when gh is installed, + // windows_terminal off macOS), so lastCompletedStep can land at 2, 3, or 4. + // Key off the settled "theme step committed" lower bound rather than a fixed + // window that assumed integrations always renders. await expect - .poll(async () => (await getOnboardingState(orcaPage)).lastCompletedStep >= 2, { + .poll(async () => (await getOnboardingState(orcaPage)).lastCompletedStep, { timeout: 5_000, - message: 'lastCompletedStep did not advance after second Continue' + message: 'lastCompletedStep did not advance past the theme step after second Continue' }) - // Why: when optional steps (Integrations/Windows terminal) are auto-skipped - // — e.g. gh is installed, so Integrations is skipped on Linux CI — the - // second Continue persists through them, landing lastCompletedStep past 3. - .toBe(true) + .toBeGreaterThanOrEqual(2) await expect .poll(async () => (await getSettings(orcaPage)).theme, { timeout: 5_000 }) .toBe(oppositeTheme) @@ -421,11 +423,35 @@ test.describe('Onboarding flow', () => { if (!store) { throw new Error('window.__store is not available') } - // Why: seed a live, protocol-compatible runtime status so env-e2e's - // execution host reads 'available' (selectable). Since #5362 a - // configured-but-statusless server correctly reads 'disconnected', which - // makes the Add Project dialog fall back to the Local host UI instead of - // the server UI this test asserts. + // Why: after #5071 the server-path add step gates on the registered + // runtime-environment list (store.runtimeEnvironments), not just the + // activeRuntimeEnvironmentId setting. Seed a redacted environment so the + // host option exists and the "on host" add UI renders. + const now = Date.now() + store.getState().setRuntimeEnvironments([ + { + id: 'env-e2e', + name: 'E2E Server', + createdAt: now, + updatedAt: now, + lastUsedAt: null, + runtimeId: null, + source: 'manual', + endpoints: [ + { + id: 'ws-env-e2e', + kind: 'websocket', + label: 'WebSocket', + endpoint: 'wss://e2e.invalid/ws' + } + ], + preferredEndpointId: 'ws-env-e2e' + } + ]) + // Why: a runtime host is only auto-selectable (health 'available') when it + // has a live, protocol-compatible status; without one it reads + // 'disconnected' and the Add Project dialog falls back to Local Mac. + // runtimeProtocolVersion 3 clears MIN_COMPATIBLE_RUNTIME_SERVER_VERSION. store.getState().setRuntimeEnvironmentStatus('env-e2e', { status: { runtimeId: 'env-e2e-runtime', @@ -434,9 +460,10 @@ test.describe('Onboarding flow', () => { authoritativeWindowId: null, liveTabCount: 0, liveLeafCount: 0, - runtimeProtocolVersion: 3 + runtimeProtocolVersion: 3, + minCompatibleRuntimeClientVersion: 1 }, - checkedAt: Date.now() + checkedAt: now }) await store.getState().updateSettings({ activeRuntimeEnvironmentId: 'env-e2e' }) }) @@ -449,13 +476,9 @@ test.describe('Onboarding flow', () => { await onboardingFooterButton(orcaPage, SKIP_TO_PROJECT_SETUP_BUTTON).click() await expectAddProjectDialog(orcaPage) - // The runtime server host (env-e2e) is selected and reachable — this is the - // proof that Skip preserved the server context instead of falling back to - // Local. The setup actions ("… on this host") target that server. - await expect(orcaPage.getByText('env-e2e')).toBeVisible() - await expect( - orcaPage.getByText(/Existing Git repository or folder on this host/i) - ).toBeVisible() + // The runtime env is selected as the Add Project host and the browse action + // is host-scoped, proving the server project-setup UI is preserved on skip. + await expect(orcaPage.getByText('Existing Git repository or folder on this host')).toBeVisible() await expect(orcaPage.getByRole('button', { name: /Browse folder/i })).toBeVisible() await expect(orcaPage.getByRole('button', { name: /Clone from URL/i })).toBeVisible() await expect(orcaPage.getByRole('button', { name: /Create new project/i })).toBeVisible() diff --git a/tests/e2e/settings-display-name-ime.spec.ts b/tests/e2e/settings-display-name-ime.spec.ts index ac06fab31a2..8001c6d3338 100644 --- a/tests/e2e/settings-display-name-ime.spec.ts +++ b/tests/e2e/settings-display-name-ime.spec.ts @@ -113,10 +113,12 @@ async function typeHangulGanadaSlowly( await page.waitForTimeout(200) } - // Why: commit the composition the way a real IME does when the user accepts - // the buffer. Since #6238 the input defers persistence until compositionend, - // so without this the store never receives the composed syllables. - await session.send('Input.insertText', { text: `${committed}${pending}` }) + // Why: a real IME commits the pending syllable (space/enter) at the end of a + // word, firing compositionend. Without this final commit the controlled + // input stays in composing state, so the component's defer-until-compositionend + // persist never runs. insertText replaces the composing region in place, so it + // finalizes to the same text rather than double-committing. + await session.send('Input.insertText', { text: committed + pending }) } test.describe('Repository Display Name IME composition', () => { diff --git a/tests/e2e/source-control-commit-draft-persistence.spec.ts b/tests/e2e/source-control-commit-draft-persistence.spec.ts index ff4ebea9295..4607e027baf 100644 --- a/tests/e2e/source-control-commit-draft-persistence.spec.ts +++ b/tests/e2e/source-control-commit-draft-persistence.spec.ts @@ -4,6 +4,7 @@ import os from 'node:os' import path from 'node:path' import { test, expect } from './helpers/orca-app' import { waitForSessionReady } from './helpers/store' +import { openSourceControlForWorktree } from './helpers/worktree-registration' type E2eWorktree = { branchName: string @@ -46,79 +47,6 @@ function cleanupWorktree(repoPath: string, worktreePath: string, branchName: str } } -async function openSourceControlForWorktree( - page: Parameters[0], - repoPath: string, - targetWorktreePath: string -): Promise { - await page.evaluate( - async ({ repoPath, targetWorktreePath }) => { - const store = window.__store - if (!store) { - throw new Error('window.__store is not available') - } - - const state = store.getState() - await state.fetchRepos() - const repo = store.getState().repos.find((entry) => entry.path === repoPath) - if (!repo) { - throw new Error(`Seeded E2E repo was not registered: ${repoPath}`) - } - - const normalizeMacTmpPath = (value: string): string => - value.startsWith('/private/var/') ? value.slice('/private'.length) : value - - // Why: worktrees:list serves a 5s detection-scan cache that a raw - // `git worktree add` (done outside Orca) does not invalidate, so the - // freshly-created worktree can be briefly absent from the boot scan. - // Poll until the cache refreshes and surfaces it. - const deadline = Date.now() + 10_000 - let listedWorktrees = await window.api.worktrees.list({ repoId: repo.id }) - let worktree = listedWorktrees.find( - (entry) => normalizeMacTmpPath(entry.path) === normalizeMacTmpPath(targetWorktreePath) - ) - while (!worktree && Date.now() < deadline) { - await new Promise((resolve) => setTimeout(resolve, 250)) - listedWorktrees = await window.api.worktrees.list({ repoId: repo.id }) - worktree = listedWorktrees.find( - (entry) => normalizeMacTmpPath(entry.path) === normalizeMacTmpPath(targetWorktreePath) - ) - } - store.setState((current) => ({ - worktreesByRepo: { - ...current.worktreesByRepo, - [repo.id]: listedWorktrees - } - })) - if (!worktree) { - throw new Error( - `E2E worktree was not loaded: ${targetWorktreePath}; listed=${listedWorktrees - .map((entry) => entry.path) - .join(', ')}` - ) - } - - store.getState().setActiveWorktree(worktree.id) - const status = await window.api.git.status({ worktreePath: worktree.path }) - store.getState().setGitStatus(worktree.id, status) - store.getState().setRightSidebarOpen(true) - store.getState().setRightSidebarTab('source-control') - }, - { repoPath, targetWorktreePath } - ) - - await expect - .poll( - async () => - page.evaluate(() => { - const state = window.__store?.getState() - return Boolean(state?.rightSidebarOpen && state?.rightSidebarTab === 'source-control') - }), - { timeout: 5_000 } - ) - .toBe(true) -} - test.describe('Source Control commit draft persistence', () => { test('preserves a typed draft when the sidebar tab remounts', async ({ orcaPage, diff --git a/tests/e2e/source-control-commit-message-ai.spec.ts b/tests/e2e/source-control-commit-message-ai.spec.ts index b51417794e5..60fd8c49dc5 100644 --- a/tests/e2e/source-control-commit-message-ai.spec.ts +++ b/tests/e2e/source-control-commit-message-ai.spec.ts @@ -4,6 +4,7 @@ import os from 'node:os' import path from 'node:path' import { test, expect } from './helpers/orca-app' import { waitForSessionReady } from './helpers/store' +import { openSourceControlForWorktree } from './helpers/worktree-registration' function createWorktreeWithStagedChange(repoPath: string): { branchName: string @@ -50,80 +51,16 @@ test.describe('Source Control AI commit messages', () => { try { await waitForSessionReady(orcaPage) - await orcaPage.evaluate( - async ({ repoPath, worktreePath: targetWorktreePath, agentCommand: command }) => { - const store = window.__store - if (!store) { - throw new Error('window.__store is not available') - } - const state = store.getState() - await state.fetchRepos() - const repo = store.getState().repos.find((entry) => entry.path === repoPath) - if (!repo) { - throw new Error(`Seeded E2E repo was not registered: ${repoPath}`) - } - const normalizeMacTmpPath = (value: string): string => - value.startsWith('/private/var/') ? value.slice('/private'.length) : value - - // Why: worktrees:list serves a 5s detection-scan cache that a raw - // `git worktree add` (done outside Orca) does not invalidate, so the - // freshly-created worktree can be briefly absent from the boot scan. - // Poll until the cache refreshes and surfaces it. - const deadline = Date.now() + 10_000 - let listedWorktrees = await window.api.worktrees.list({ repoId: repo.id }) - let worktree = listedWorktrees.find( - (entry) => normalizeMacTmpPath(entry.path) === normalizeMacTmpPath(targetWorktreePath) - ) - while (!worktree && Date.now() < deadline) { - await new Promise((resolve) => setTimeout(resolve, 250)) - listedWorktrees = await window.api.worktrees.list({ repoId: repo.id }) - worktree = listedWorktrees.find( - (entry) => normalizeMacTmpPath(entry.path) === normalizeMacTmpPath(targetWorktreePath) - ) - } - store.setState((current) => ({ - worktreesByRepo: { - ...current.worktreesByRepo, - [repo.id]: listedWorktrees - } - })) - if (!worktree) { - throw new Error( - `E2E worktree was not loaded: ${targetWorktreePath}; listed=${listedWorktrees - .map((entry) => entry.path) - .join(', ')}` - ) - } - - store.getState().setActiveWorktree(worktree.id) - await store.getState().updateSettings({ - commitMessageAi: { - enabled: true, - agentId: 'custom', - selectedModelByAgent: {}, - selectedThinkingByModel: {}, - customPrompt: '', - customAgentCommand: command - } - }) - const status = await window.api.git.status({ worktreePath: worktree.path }) - store.getState().setGitStatus(worktree.id, status) - store.getState().setRightSidebarTab('source-control') - store.getState().setRightSidebarOpen(true) - }, - { repoPath: testRepoPath, worktreePath, agentCommand } - ) - - await expect - .poll( - async () => - orcaPage.evaluate(() => { - const state = window.__store?.getState() - return Boolean(state?.rightSidebarOpen && state?.rightSidebarTab === 'source-control') - }), - { timeout: 5_000 } - ) - .toBe(true) + await openSourceControlForWorktree(orcaPage, testRepoPath, worktreePath, { + commitMessageAi: { + enabled: true, + agentId: 'custom', + selectedModelByAgent: {}, + selectedThinkingByModel: {}, + customPrompt: '', + customAgentCommand: agentCommand + } + }) const textarea = orcaPage.getByRole('textbox', { name: 'Commit message' }) await expect(textarea).toBeVisible({ timeout: 10_000 }) diff --git a/tests/e2e/terminal-shortcuts.spec.ts b/tests/e2e/terminal-shortcuts.spec.ts index 4895699d8ff..fbea6ebf1fb 100644 --- a/tests/e2e/terminal-shortcuts.spec.ts +++ b/tests/e2e/terminal-shortcuts.spec.ts @@ -413,43 +413,6 @@ async function pressShiftedRussianLayoutKey(page: Page): Promise<{ }) } -// Why: handleRequestClosePane pops a "Close Terminal?" dialog when the pane -// reports a running child process. Under E2E, a freshly split pane's -// proc.process is briefly unset so the check returns true spuriously. Click -// Close when the dialog appears so the test's chord-routing assertion stays -// deterministic; no-op when it doesn't. -async function closeActiveSplitPaneWithChord( - page: Page, - mod: string, - expectedCountAfter: number -): Promise { - // Guard: the split pane must still be present. Otherwise Cmd/Ctrl+W closes - // the last pane, which closes the tab and (on Linux) quits the whole app, - // surfacing later as a confusing "target page closed" instead of a clear - // "the split pane went away" signal. - await waitForPaneCount(page, expectedCountAfter + 1) - await focusActiveTerminal(page) - await page.keyboard.press(`${mod}+w`) - // The close is either immediate or gated behind a running-process - // confirmation dialog whose async hasChildProcesses check can surface it a - // beat late (a fixed short wait races it under CI load). Confirm the dialog - // whenever it appears and wait for the pane to actually drain. - await expect - .poll( - async () => { - // The running-process confirmation dialog's affirmative button is - // "Stop and Close" (not "Close"). - const confirmButton = page.getByRole('button', { name: 'Stop and Close', exact: true }) - if (await confirmButton.isVisible().catch(() => false)) { - await confirmButton.click().catch(() => {}) - } - return countVisibleTerminalPanes(page) - }, - { timeout: 10_000, message: 'split pane did not close after Cmd/Ctrl+W' } - ) - .toBe(expectedCountAfter) -} - async function pressAndExpectWrite( page: Page, app: ElectronApplication, @@ -482,6 +445,34 @@ const mod = isMac ? 'Meta' : 'Control' const splitVerticalChord = isMac ? `${mod}+d` : `${mod}+Shift+d` const splitHorizontalChord = isMac ? `${mod}+Shift+d` : 'Alt+Shift+d' +// Why: a freshly split pane can transiently still report a running child, so +// poll for the confirm dialog and pane-count settling instead of a fixed wait. +async function closeActivePaneAndSettle(page: Page, expectedCount: number): Promise { + await focusActiveTerminal(page) + await page.keyboard.press(`${mod}+w`) + // The "Stop running command?" confirm surfaces a "Stop and Close" action when + // the pane still reports a running child. + const confirmButton = page.getByRole('button', { name: /Stop and Close/i }) + await expect + .poll( + async () => { + if (await confirmButton.isVisible().catch(() => false)) { + // Why: surface a click failure so a real actionability/strict-mode + // error isn't hidden behind the generic pane-count timeout. + await confirmButton.click().catch((err) => { + console.warn('closeActivePaneAndSettle: confirm click failed', err) + }) + } + return countVisibleTerminalPanes(page) + }, + { + timeout: 10_000, + message: `Expected ${expectedCount} visible terminal panes after close` + } + ) + .toBe(expectedCount) +} + // Why: serial mode is load-bearing. Tests mutate shared Electron app state // (pane layout, terminal buffer, expand toggle) and the pty:write spy log is // a single main-process singleton. Parallel execution would interleave chord @@ -771,6 +762,9 @@ test.describe('Terminal Shortcuts', () => { await focusActiveTerminal(orcaPage) await orcaPage.keyboard.press(splitVerticalChord) await waitForPaneCount(orcaPage, panesBeforeSplit + 1) + // Why: ensure the new split pane's PTY is actually bound before we later + // close it, so the close cycle can't race an in-progress split. + await waitForActivePanePtyId(orcaPage) // Cmd/Ctrl+] and Cmd/Ctrl+[ cycle focus (no pane-count change). await focusActiveTerminal(orcaPage) @@ -803,19 +797,15 @@ test.describe('Terminal Shortcuts', () => { .toBe(false) // Cmd/Ctrl+W closes the active split pane (not the whole tab: >1 pane). - // Why: the close handler checks hasChildProcesses async; a freshly - // spawned pane can transiently report a running child (node-pty's - // proc.process lags the spawn), which surfaces a confirmation dialog - // instead of closing immediately. Confirm it if it appears — the test - // only needs to prove the chord routed to the close handler. - await closeActiveSplitPaneWithChord(orcaPage, mod, panesBeforeSplit) + await closeActivePaneAndSettle(orcaPage, panesBeforeSplit) // Split horizontally (chord varies by platform — see splitHorizontalChord). const panesBeforeHSplit = await countVisibleTerminalPanes(orcaPage) await focusActiveTerminal(orcaPage) await orcaPage.keyboard.press(splitHorizontalChord) await waitForPaneCount(orcaPage, panesBeforeHSplit + 1) - await closeActiveSplitPaneWithChord(orcaPage, mod, panesBeforeHSplit) + await waitForActivePanePtyId(orcaPage) + await closeActivePaneAndSettle(orcaPage, panesBeforeHSplit) // Cmd/Ctrl+F toggles the search overlay. await focusActiveTerminal(orcaPage) diff --git a/tests/e2e/workspace-space-git-status.spec.ts b/tests/e2e/workspace-space-git-status.spec.ts index 3794ff200c9..45b756a662f 100644 --- a/tests/e2e/workspace-space-git-status.spec.ts +++ b/tests/e2e/workspace-space-git-status.spec.ts @@ -1,15 +1,21 @@ import { execFileSync } from 'node:child_process' -import { mkdtempSync, rmSync } from 'node:fs' +import { mkdtempSync, realpathSync, rmSync } from 'node:fs' import os from 'node:os' import path from 'node:path' import { test, expect } from './helpers/orca-app' +import { loadWorktreesUntilPathsPresent } from './helpers/worktree-registration' test.describe('Workspace Space git status checks', () => { test('checks every scanned deletable row, including rows after the first 50', async ({ orcaPage, testRepoPath }) => { - const worktreeParent = mkdtempSync(path.join(os.tmpdir(), 'orca-space-git-status-')) + // Why: on symlinked tmpdirs (/var→/private/var on macOS, /tmp→… on CI) Orca + // registers worktrees under their realpath, so the parent must be canonical + // before `git worktree add` or the recorded paths won't match and rows drop. + const worktreeParent = realpathSync( + mkdtempSync(path.join(os.tmpdir(), 'orca-space-git-status-')) + ) const worktreePaths = Array.from({ length: 60 }, (_, index) => path.join(worktreeParent, `worktree-${index}`) ) @@ -21,54 +27,57 @@ test.describe('Workspace Space git status checks', () => { stdio: 'pipe' }) } + const registeredWorktreePaths = worktreePaths.map((worktreePath) => + realpathSync(worktreePath) + ) + + const repoId = await orcaPage.evaluate((testRepoPath) => { + const store = window.__store + if (!store) { + throw new Error('Expected e2e store to be exposed') + } + const repo = store.getState().repos.find((item) => item.path === testRepoPath) + if (!repo) { + throw new Error('Expected test repo to be loaded') + } + return repo.id + }, testRepoPath) + + // Why: the 60 worktrees were added via raw git, so poll past the 5s scan + // cache TTL until every path registers before deriving the space rows. + await loadWorktreesUntilPathsPresent(orcaPage, repoId, registeredWorktreePaths) + await orcaPage.evaluate( - async ({ testRepoPath, worktreeParent, expectedCount }) => { + async ({ testRepoPath, worktreePaths }) => { const store = window.__store if (!store) { throw new Error('Expected e2e store to be exposed') } - const repo = store.getState().repos.find((item) => item.path === testRepoPath) + const initialState = store.getState() + const repo = initialState.repos.find((item) => item.path === testRepoPath) if (!repo) { throw new Error('Expected test repo to be loaded') } + await window.api.git.status({ worktreePath: worktreePaths[0] }) - // Why: worktrees are created via raw `git worktree add`, which git - // records with the path as given (not symlink-canonicalized), so - // match against the git-reported store paths rather than realpathSync'd - // ones — otherwise on macOS the /var vs /private/var mismatch makes - // git:status reject the probe as an unregistered path. Also poll: - // fetchWorktrees serves a 5s scan cache that raw adds don't invalidate. - const normalizeMacTmpPath = (value: string): string => - value.startsWith('/private/var/') ? value.slice('/private'.length) : value - const parentKey = normalizeMacTmpPath(worktreeParent) - const deadline = Date.now() + 20_000 - const findCreatedWorktrees = () => - (store.getState().worktreesByRepo[repo.id] ?? []).filter((worktree) => - normalizeMacTmpPath(worktree.path).startsWith(`${parentKey}/`) - ) - await store.getState().fetchWorktrees(repo.id) - let worktrees = findCreatedWorktrees() - while (worktrees.length < expectedCount && Date.now() < deadline) { - await new Promise((resolve) => setTimeout(resolve, 250)) - await store.getState().fetchWorktrees(repo.id) - worktrees = findCreatedWorktrees() - } - if (worktrees.length !== expectedCount) { + const state = store.getState() + const expectedPaths = new Set(worktreePaths) + const worktrees = (state.worktreesByRepo[repo.id] ?? []).filter((worktree) => + expectedPaths.has(worktree.path) + ) + if (worktrees.length !== worktreePaths.length) { throw new Error( - `Expected ${expectedCount} registered worktrees, got ${ + `Expected ${worktreePaths.length} registered worktrees, got ${ worktrees.length - } from ${(store.getState().worktreesByRepo[repo.id] ?? []).length}: ${( - store.getState().worktreesByRepo[repo.id] ?? [] + } from ${(state.worktreesByRepo[repo.id] ?? []).length}: ${( + state.worktreesByRepo[repo.id] ?? [] ) .slice(0, 5) .map((worktree) => worktree.path) .join(', ')}` ) } - // Probe with a git-reported store path (a registered worktree root). - await window.api.git.status({ worktreePath: worktrees[0].path }) - const rows = worktrees.map((worktree, index) => ({ worktreeId: worktree.id, repoId: repo.id, @@ -121,7 +130,7 @@ test.describe('Workspace Space git status checks', () => { }) store.getState().openSpacePage() }, - { testRepoPath, worktreeParent, expectedCount: worktreePaths.length } + { testRepoPath, worktreePaths: registeredWorktreePaths } ) await expect diff --git a/tests/e2e/worktree.spec.ts b/tests/e2e/worktree.spec.ts index 0ebb5feb34b..3c980756e8e 100644 --- a/tests/e2e/worktree.spec.ts +++ b/tests/e2e/worktree.spec.ts @@ -19,7 +19,6 @@ * crash in whatever replaces it. */ -import { execFileSync } from 'node:child_process' import type { ConsoleMessage } from '@stablyai/playwright-test' import { test, expect } from './helpers/orca-app' import { @@ -217,24 +216,12 @@ test.describe('Create Workspace', () => { test('reuses a resolved pasted GitHub URL when quick create submits', async ({ electronApp, - orcaPage, - testRepoPath + orcaPage }) => { const title = `E2E smart URL resolution ${Date.now()}` const url = 'https://github.com/stablyai/orca/pull/2049' const linkedWorkspacePattern = new RegExp(title.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) - // Why: create builds from whatever base resolvePrBase resolves to. The - // disposable fixture repo has no `origin`, so a remote-tracking base like - // `origin/main` can't resolve. Resolve to the fixture's actual local HEAD - // branch instead so `git worktree add` succeeds offline — without adding an - // origin remote, which would perturb the pasted-URL work-item lookup below. - const baseBranch = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { - cwd: testRepoPath - }) - .toString() - .trim() - try { await orcaPage.getByRole('button', { name: 'New workspace', exact: true }).click() @@ -243,7 +230,7 @@ test.describe('Create Workspace', () => { await expect(dialog.locator('[data-workspace-name-input="true"]')).toBeVisible() await electronApp.evaluate( - ({ ipcMain }, { title, url, baseBranch }) => { + ({ ipcMain }, { title, url }) => { const counters = globalThis as unknown as { __smartGitHubLookupCount: number __smartResolvePrBaseCount: number @@ -276,40 +263,21 @@ test.describe('Create Workspace', () => { } ) ipcMain.removeHandler('worktrees:resolvePrBase') + // Why: the fixture repo has no remote and its default branch name + // depends on the host's git init.defaultBranch (main vs master), so + // resolve the PR base to HEAD, which always exists regardless. ipcMain.handle('worktrees:resolvePrBase', () => { counters.__smartResolvePrBaseCount += 1 - return { baseBranch } + return { baseBranch: 'HEAD' } }) }, - { title, url, baseBranch } + { title, url } ) const nameInput = dialog.getByPlaceholder(/Type a name/i) await expect(nameInput).toBeVisible() await nameInput.fill(url) - // Why: wait for the pasted URL to resolve via a single work-item lookup. - // The base resolution (resolvePrBase) fires separately and its timing - // relative to this point is not deterministic, so only the lookup is - // asserted here; the reuse invariant is checked in full after create. - await expect - .poll(() => - electronApp.evaluate(() => { - const counters = globalThis as unknown as { __smartGitHubLookupCount?: number } - return counters.__smartGitHubLookupCount ?? -1 - }) - ) - .toBe(1) - const createButton = dialog.getByRole('button', { name: /Create (Workspace|Worktree)/i }) - await expect(createButton).toBeEnabled() - await createButton.click() - - await expect(dialog).toBeHidden({ timeout: 15_000 }) - await expect(orcaPage.getByRole('option', { name: linkedWorkspacePattern })).toBeVisible({ - timeout: 10_000 - }) - await expect(orcaPage.getByRole('option', { name: url })).toHaveCount(0) - await expect(orcaPage.getByText('Linked PR #2049')).toBeVisible() await expect .poll(() => electronApp.evaluate(() => { @@ -323,8 +291,34 @@ test.describe('Create Workspace', () => { } }) ) - // Why: counts are unchanged from before create — the submit reused the - // already-resolved URL/base instead of looking them up again. + .toEqual({ githubLookupCount: 1, resolvePrBaseCount: 0 }) + const createButton = dialog.getByRole('button', { name: /Create (Workspace|Worktree)/i }) + await expect(createButton).toBeEnabled() + await createButton.click() + + await expect(dialog).toBeHidden({ timeout: 15_000 }) + await expect(orcaPage.getByRole('option', { name: linkedWorkspacePattern })).toBeVisible({ + timeout: 10_000 + }) + await expect(orcaPage.getByRole('option', { name: url })).toHaveCount(0) + await expect(orcaPage.getByText('Linked PR #2049')).toBeVisible() + // Why: quick create reuses the single GitHub lookup from typing (no + // redundant re-fetch), and since #5733 ("Create PR worktrees from the PR + // head") it resolves the PR start point exactly once at submit time — so + // the base resolves once here rather than being skipped. + await expect + .poll(() => + electronApp.evaluate(() => { + const counters = globalThis as unknown as { + __smartGitHubLookupCount?: number + __smartResolvePrBaseCount?: number + } + return { + githubLookupCount: counters.__smartGitHubLookupCount ?? -1, + resolvePrBaseCount: counters.__smartResolvePrBaseCount ?? -1 + } + }) + ) .toEqual({ githubLookupCount: 1, resolvePrBaseCount: 1 }) } finally { await orcaPage @@ -371,9 +365,10 @@ test.describe('Create Workspace', () => { }) ) ipcMain.removeHandler('worktrees:resolvePrBase') - // Why: the fixture repo's default branch is master and has no - // remote; resolve the PR base to a ref that actually exists. - ipcMain.handle('worktrees:resolvePrBase', () => ({ baseBranch: 'master' })) + // Why: the fixture repo has no remote and its default branch name + // depends on the host's git init.defaultBranch (main vs master), so + // resolve the PR base to HEAD, which always exists regardless. + ipcMain.handle('worktrees:resolvePrBase', () => ({ baseBranch: 'HEAD' })) }, { title, url } )