From 584086cb3704163e0fd80d7cb55246c06005213f Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 6 Aug 2026 21:54:32 -0400 Subject: [PATCH 1/4] fix(repo): backfill fork upstream for SSH repos on connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fork on an SSH host kept `upstream === undefined` until someone opened its settings page, so its GitHub Project rows stayed hidden and its fork badge never rendered. Run the one-shot backfill for a connection's repos when that connection reports `connected` — the first moment the probe can succeed — instead of skipping SSH repos outright. All passes now share one chain, so a burst of connects queues behind the startup pass rather than fanning out `gh repo view` past its semaphore. Fixes #12967 --- ...runtime-fork-upstream-ssh-backfill.test.ts | 210 ++++++++++++++++++ src/main/runtime/orca-runtime.ts | 68 +++++- 2 files changed, 270 insertions(+), 8 deletions(-) create mode 100644 src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts diff --git a/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts b/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts new file mode 100644 index 000000000000..2cc6e92b2461 --- /dev/null +++ b/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts @@ -0,0 +1,210 @@ +/** + * A fork on an SSH host used to keep `upstream === undefined` until someone opened + * its settings page — so its GitHub Project rows stayed hidden and its fork badge + * never rendered (#12967). Connect is the first moment the probe can succeed, so + * the backfill runs there: best-effort, sequential, and never awaited by callers. + */ +import { describe, expect, it, vi, beforeEach } from 'vitest' +import type { Repo } from '../../shared/types' +import type { SshConnectionState } from '../../shared/ssh-types' +import { OrcaRuntimeService } from './orca-runtime' +import { getRepoUpstream } from '../github/client' + +const getRepoUpstreamMock = vi.hoisted(() => vi.fn()) + +vi.mock('../github/client', async (importOriginal) => ({ + ...((await importOriginal()) as Record), + getRepoUpstream: getRepoUpstreamMock +})) + +const UPSTREAM = { owner: 'stablyai', repo: 'orca' } + +function makeRepo(overrides: Partial): Repo { + return { + id: 'repo', + path: '/srv/orca', + displayName: 'orca', + badgeColor: '#000', + addedAt: 1, + kind: 'git', + ...overrides + } as Repo +} + +function createRuntime(repos: Repo[]) { + const updateRepo = vi.fn((id: string, updates: Partial) => { + const repo = repos.find((entry) => entry.id === id) + if (!repo) { + return null + } + Object.assign(repo, updates) + return repo + }) + const runtime = new OrcaRuntimeService({ + getRepos: () => [...repos], + getRepo: (id: string) => repos.find((repo) => repo.id === id) ?? null, + updateRepo, + getAllWorktreeMeta: () => ({}), + getWorktreeMeta: () => null, + setWorktreeMeta: vi.fn(), + removeWorktreeMeta: vi.fn(), + getGitHubCache: () => null, + getSettings: () => ({}) + } as never) + return { runtime, repos, updateRepo } +} + +function sshState(targetId: string, status: SshConnectionState['status']): SshConnectionState { + return { targetId, status, error: null, reconnectAttempt: 0 } +} + +// Why: the backfill is deliberately fire-and-forget, so tests drain the queue +// rather than awaiting a handle the production caller never has. +async function drainBackfill(): Promise { + for (let tick = 0; tick < 20; tick += 1) { + await Promise.resolve() + } +} + +beforeEach(() => { + getRepoUpstreamMock.mockReset() + getRepoUpstreamMock.mockResolvedValue(null) +}) + +describe('fork upstream backfill for SSH repos', () => { + it('resolves the upstream when the connection comes up', async () => { + getRepoUpstreamMock.mockResolvedValue(UPSTREAM) + const { runtime, repos } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) + + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + await drainBackfill() + + expect(getRepoUpstream).toHaveBeenCalledWith('/srv/orca', 'ssh-1') + expect(repos[0].upstream).toEqual(UPSTREAM) + }) + + it('migrates the auto-detected origin avatar to the upstream, like the local pass', async () => { + getRepoUpstreamMock.mockResolvedValue(UPSTREAM) + const { runtime, repos } = createRuntime([ + makeRepo({ + id: 'ssh-fork', + connectionId: 'ssh-1', + repoIcon: { type: 'image', src: 'https://avatars/fork.png', source: 'github' } + }), + makeRepo({ + id: 'ssh-chosen', + path: '/srv/other', + connectionId: 'ssh-1', + repoIcon: { type: 'emoji', emoji: '🦈' } + }) + ]) + + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + await drainBackfill() + + expect(repos[0].repoIcon).toMatchObject({ type: 'image', source: 'github' }) + expect(repos[0].repoIcon).not.toMatchObject({ src: 'https://avatars/fork.png' }) + expect(repos[1].repoIcon).toEqual({ type: 'emoji', emoji: '🦈' }) + }) + + it('skips folder repos, other connections, and already-resolved repos', async () => { + const { runtime } = createRuntime([ + makeRepo({ id: 'folder', connectionId: 'ssh-1', kind: 'folder' }), + makeRepo({ id: 'other-host', path: '/srv/b', connectionId: 'ssh-2' }), + makeRepo({ id: 'local', path: '/home/a' }), + makeRepo({ id: 'resolved', path: '/srv/c', connectionId: 'ssh-1', upstream: null }) + ]) + + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + await drainBackfill() + + expect(getRepoUpstream).not.toHaveBeenCalled() + }) + + it('probes each repo once, not once per reconnect', async () => { + const { runtime } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) + + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + await drainBackfill() + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'disconnected')) + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + await drainBackfill() + + expect(getRepoUpstream).toHaveBeenCalledTimes(1) + }) + + it('retries on the next connect when the probe threw', async () => { + getRepoUpstreamMock.mockRejectedValueOnce(new Error('ssh channel closed')) + const { runtime, repos } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) + + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + await drainBackfill() + expect(repos[0].upstream).toBeUndefined() + + getRepoUpstreamMock.mockResolvedValue(UPSTREAM) + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + await drainBackfill() + + expect(repos[0].upstream).toEqual(UPSTREAM) + }) + + it('never overlaps probes when several connections come up at once', async () => { + let inFlight = 0 + let maxInFlight = 0 + getRepoUpstreamMock.mockImplementation(async () => { + inFlight += 1 + maxInFlight = Math.max(maxInFlight, inFlight) + await Promise.resolve() + inFlight -= 1 + return null + }) + const { runtime } = createRuntime([ + makeRepo({ id: 'a', path: '/srv/a', connectionId: 'ssh-1' }), + makeRepo({ id: 'b', path: '/srv/b', connectionId: 'ssh-2' }), + makeRepo({ id: 'c', path: '/srv/c', connectionId: 'ssh-3' }) + ]) + + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + runtime.notifySshStateChanged('ssh-2', sshState('ssh-2', 'connected')) + runtime.notifySshStateChanged('ssh-3', sshState('ssh-3', 'connected')) + await drainBackfill() + + expect(getRepoUpstream).toHaveBeenCalledTimes(3) + expect(maxInFlight).toBe(1) + }) + + it('returns from the connect notification without waiting on the probe', async () => { + let resolveProbe: ((value: unknown) => void) | undefined + getRepoUpstreamMock.mockImplementation( + () => + new Promise((resolve) => { + resolveProbe = resolve + }) + ) + const { runtime, repos } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) + + runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + + expect(getRepoUpstream).not.toHaveBeenCalled() + await drainBackfill() + expect(getRepoUpstream).toHaveBeenCalledOnce() + expect(repos[0].upstream).toBeUndefined() + + resolveProbe?.(UPSTREAM) + await drainBackfill() + expect(repos[0].upstream).toEqual(UPSTREAM) + }) + + it('leaves the startup pass local-only', async () => { + const { runtime } = createRuntime([ + makeRepo({ id: 'local', path: '/home/a' }), + makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' }) + ]) + + runtime.setNotifier({ reposChanged: vi.fn() } as never) + await drainBackfill() + + expect(getRepoUpstream).toHaveBeenCalledExactlyOnceWith('/home/a', null) + }) +}) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 526acea823bb..2befd211dc3c 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2920,6 +2920,10 @@ export class OrcaRuntimeService { >() private worktreeLifecycleListeners = new Set<(event: RuntimeWorktreeLifecycleEvent) => void>() private forkBackfillStarted = false + // Why: one chain keeps every backfill pass sequential, so a burst of SSH + // connects can never fan out `gh repo view` calls past the shared semaphore. + private forkUpstreamBackfillQueue: Promise = Promise.resolve() + private forkUpstreamBackfilledConnectionIds = new Set() private agentBrowserBridge: AgentBrowserBridge | null = null private offscreenBrowserBackend: BrowserBackend | null = null private emulatorBridge: EmulatorBridge | null = null @@ -5296,6 +5300,8 @@ export class OrcaRuntimeService { this.invalidateSshWorktreeScanCache(targetId) if (state.status !== 'connected') { this.cancelLegacyWorkerTerminalRecoveryRetry(`ssh:${targetId}`) + } else { + this.backfillForkUpstreamsForConnection(targetId) } this.emitClientEvent({ type: 'sshStateChanged', targetId, state: getPublicSshState(state)! }) } @@ -19390,22 +19396,66 @@ export class OrcaRuntimeService { } // Why: repos added before fork detection existed have no stored `upstream`, so - // their avatar/badge would never self-correct. Resolve it once at startup for - // local git repos; SSH repos resolve lazily when their settings open (their - // connection may not be up yet). Sequential to respect the gh rate limit; - // failures leave `upstream` unset so the next launch retries. - private async backfillForkUpstreams(): Promise { + // their avatar/badge/project rows would never self-correct. Resolve it once at + // startup for repos whose host is already reachable; SSH repos wait for their + // connection instead (their host may not be up yet). + private backfillForkUpstreams(): Promise { + return this.queueForkUpstreamBackfill((repo) => !repo.connectionId) + } + + // Why: an SSH fork used to stay `upstream === undefined` until someone opened + // its settings page, hiding it from project rows and from the fork badge with + // no visible cause (#12967). Connect is the first moment the probe can succeed. + // Fire-and-forget so nothing on the connection path waits on the network. + private backfillForkUpstreamsForConnection(connectionId: string): void { + if (this.forkUpstreamBackfilledConnectionIds.has(connectionId)) { + return + } + this.forkUpstreamBackfilledConnectionIds.add(connectionId) + void this.queueForkUpstreamBackfill((repo) => repo.connectionId === connectionId) + .then((complete) => { + // Why: a pass interrupted by a probe failure left `upstream` unset, so let + // the next connect retry rather than stranding it until the app restarts. + if (!complete) { + this.forkUpstreamBackfilledConnectionIds.delete(connectionId) + } + }) + .catch(() => { + this.forkUpstreamBackfilledConnectionIds.delete(connectionId) + }) + } + + // Why: chained, never concurrent — `getRepoUpstream` shells out to `gh` behind a + // process-global semaphore, so passes queue instead of fanning out. + private queueForkUpstreamBackfill(selects: (repo: Repo) => boolean): Promise { + const pass = this.forkUpstreamBackfillQueue.then(() => this.runForkUpstreamBackfill(selects)) + this.forkUpstreamBackfillQueue = pass.then( + () => undefined, + () => undefined + ) + return pass + } + + /** Resolves false when any probe failed, so the caller can retry that scope later. */ + private async runForkUpstreamBackfill(selects: (repo: Repo) => boolean): Promise { + let complete = true try { const store = this.requireStore() let changed = false for (const repo of store.getRepos()) { - if (repo.upstream !== undefined || repo.kind === 'folder' || repo.connectionId) { + if (repo.upstream !== undefined || repo.kind === 'folder' || !selects(repo)) { continue } let upstream: GitHubOwnerRepo | null try { - upstream = await getRepoUpstream(repo.path, null) + // Why: same resolution the settings page uses, so backfilled and + // lazily-resolved upstreams can never disagree for the same repo. + const options = this.getHostedReviewExecutionOptions(repo) + upstream = await (options + ? getRepoUpstream(repo.path, repo.connectionId ?? null, options) + : getRepoUpstream(repo.path, repo.connectionId ?? null)) } catch { + complete = false continue } const updates: Partial = { upstream: upstream ?? null } @@ -19419,8 +19469,10 @@ export class OrcaRuntimeService { if (changed) { this.notifyReposChanged() } + return complete } catch { - // Best-effort startup backfill; never disrupt launch. + // Best-effort; never disrupt launch or a connection. + return false } } From 87f922300e3edafc355841ca401c14f6cc806f27 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 6 Aug 2026 23:50:00 -0400 Subject: [PATCH 2/4] test(e2e): prove SSH fork upstream backfill against a real Docker relay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drives the actual #12967 sequence end to end: a legacy repo whose persisted `upstream` was stripped, an app relaunch, then an SSH connect that must backfill it and render the fork badge with no settings visit. Deterministic and offline — getRepoUpstream short-circuits on the `upstream` git remote before it reaches `gh`, so the container's two remotes decide the answer with no GitHub auth or network. Verified by revert: with the fix backed out, upstream stays unresolved for the full 90s after connect. --- ...-ssh-docker-fork-upstream-backfill-e2e.mjs | 41 +++ package.json | 1 + .../ssh-docker-fork-upstream-backfill.spec.ts | 242 ++++++++++++++++++ 3 files changed, 284 insertions(+) create mode 100644 config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs create mode 100644 tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts diff --git a/config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs b/config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs new file mode 100644 index 000000000000..4bcf6502164c --- /dev/null +++ b/config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs @@ -0,0 +1,41 @@ +import { spawnSync } from 'node:child_process' + +const rawExtraArgs = process.argv.slice(2) +const extraArgs = rawExtraArgs[0] === '--' ? rawExtraArgs.slice(1) : rawExtraArgs +const pnpm = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm' +const env = { + ...process.env, + ORCA_E2E_SSH_DOCKER: '1' +} + +// Why: Node's CVE-2024-27980 hardening rejects .cmd spawns without shell on Windows. +const spawnOptions = { + stdio: 'inherit', + env, + shell: process.platform === 'win32' +} + +const runtime = spawnSync(pnpm, ['run', 'ensure:electron-runtime'], spawnOptions) + +if (runtime.status !== 0) { + process.exit(runtime.status ?? 1) +} + +const result = spawnSync( + pnpm, + [ + 'exec', + 'playwright', + 'test', + 'tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts', + '--config', + 'tests/playwright.config.ts', + '--project', + 'electron-headless', + '--workers=1', + ...extraArgs + ], + spawnOptions +) + +process.exit(result.status ?? 1) diff --git a/package.json b/package.json index 06407e811446..d1ed1261d7a2 100644 --- a/package.json +++ b/package.json @@ -96,6 +96,7 @@ "test:e2e:terminal-perf:html-report": "node config/scripts/generate-terminal-perf-html-report.mjs", "test:e2e:ssh-docker-perf": "node config/scripts/run-ssh-docker-perf-e2e.mjs", "test:e2e:ssh-docker-watcher-isolation": "node config/scripts/run-ssh-docker-watcher-isolation-e2e.mjs", + "test:e2e:ssh-docker-fork-upstream-backfill": "node config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs", "test:e2e:ssh-docker-terminal-parking": "node config/scripts/run-ssh-docker-terminal-parking-e2e.mjs", "test:e2e:nested-runtime-ssh": "node config/scripts/run-nested-runtime-ssh-e2e.mjs", "test:e2e:source-control-scale": "pnpm run ensure:electron-runtime && npx playwright test tests/e2e/source-control-large-file-count.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1", diff --git a/tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts b/tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts new file mode 100644 index 000000000000..1ec1d62e5b4b --- /dev/null +++ b/tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts @@ -0,0 +1,242 @@ +/** + * Live proof for #12967 against a real Docker SSH host with a real relay. + * + * A fork added before fork detection existed persists no `upstream`, and the + * startup backfill used to skip every repo with a `connectionId` — so the fork + * badge never rendered and GitHub Project rows stayed hidden until someone + * opened that repo's settings page. This drives the actual user sequence: + * legacy store state on disk, app launch, SSH connect, badge appears. + * + * Deterministic and offline: `getRepoUpstream` reads the `upstream` git remote + * before it ever shells out to `gh`, so the container's remotes decide the + * answer — no GitHub auth, no network, no real fork required. + */ +import { readFileSync, readdirSync, writeFileSync } from 'node:fs' +import path from 'node:path' + +import type { Page } from '@stablyai/playwright-test' + +import { connectDockerSshRelayTarget } from './helpers/docker-ssh-relay-connection' +import { + cleanupDockerSshRelayTarget, + execDockerSshRelayTargetCommand, + shellQuote, + startDockerSshRelayTarget, + type DockerSshRelayTarget +} from './helpers/docker-ssh-relay-target' +import { test, expect } from './helpers/orca-app' +import { createRestartSession, readRestartRendererState } from './helpers/orca-restart' + +const RUN_DOCKER_SSH = process.env.ORCA_E2E_SSH_DOCKER === '1' + +const FORK_REPO_PATH = '/tmp/orca-docker-fork-upstream-repo' +const FORK_ORIGIN_URL = 'https://github.com/orca-e2e-contributor/orca.git' +const UPSTREAM_URL = 'https://github.com/stablyai/orca.git' +const EXPECTED_UPSTREAM = { owner: 'stablyai', repo: 'orca' } +const FORK_BADGE_LABEL = 'Fork of stablyai/orca' + +/** A fork clone: `origin` is the personal copy, `upstream` is the parent. */ +function seedForkRepoOnTarget(target: DockerSshRelayTarget): void { + execDockerSshRelayTargetCommand( + target, + [ + `rm -rf ${shellQuote(FORK_REPO_PATH)}`, + `mkdir -p ${shellQuote(FORK_REPO_PATH)}`, + `cd ${shellQuote(FORK_REPO_PATH)}`, + 'git init', + 'git config user.email e2e@test.local', + 'git config user.name "Orca Fork Upstream E2E"', + `git remote add origin ${shellQuote(FORK_ORIGIN_URL)}`, + `git remote add upstream ${shellQuote(UPSTREAM_URL)}`, + "printf '%s\\n' 'fork upstream backfill fixture' > README.md", + 'git add README.md', + 'git commit -m initial' + ].join(' && ') + ) +} + +type PersistedUpstream = { owner: string; repo: string; host?: string } | null | 'absent' + +/** + * Why `readRestartRendererState`: a relaunched window replaces its document + * during initial hydration, which destroys the execution context mid-evaluate. + * The wrapper turns that transition into `null` so the enclosing poll retries. + */ +async function readRepoUpstream( + page: Page, + repoId: string +): Promise { + return readRestartRendererState(() => + page.evaluate(async (repoId) => { + const store = window.__store + if (!store) { + throw new Error('Store unavailable') + } + await store.getState().fetchRepos() + const repo = store.getState().repos.find((candidate) => candidate.id === repoId) + if (!repo) { + return 'gone' as const + } + // Why: `undefined` (never probed) and `null` (probed, not a fork) are + // different states and both survive this boundary badly — name them. + return repo.upstream === undefined ? ('absent' as const) : repo.upstream + }, repoId) + ) +} + +/** + * Every file the live store could be writing. + * + * Why not just `/orca-data.json`: the Store is constructed with the + * *active profile's* data file (`index.ts` → `new Store({ dataFile })`), which + * lives under `profiles//`. The launch dir only holds the seeded profile. + */ +function candidateStoreFiles(userDataPath: string): string[] { + const profilesDir = path.join(userDataPath, 'profiles') + let profileFiles: string[] = [] + try { + profileFiles = readdirSync(profilesDir, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => path.join(profilesDir, entry.name, 'orca-data.json')) + } catch { + // No profiles directory yet. + } + return [...profileFiles, path.join(userDataPath, 'orca-data.json')] +} + +function findStoreFileWithRepo(userDataPath: string, repoId: string): string | null { + for (const storePath of candidateStoreFiles(userDataPath)) { + try { + const data = JSON.parse(readFileSync(storePath, 'utf8')) as { repos?: { id: string }[] } + if ((data.repos ?? []).some((repo) => repo.id === repoId)) { + return storePath + } + } catch { + // Why: the store rewrites via a temp file, so a read can land mid-swap. + } + } + return null +} + +/** Why: saves are debounced ~1s, so closing right after `addRemote` can beat the write. */ +async function waitForPersistedRepo(userDataPath: string, repoId: string): Promise { + await expect + .poll(() => findStoreFileWithRepo(userDataPath, repoId) !== null, { + timeout: 30_000, + message: + `repo ${repoId} never reached any store file under ${userDataPath}. ` + + `Checked: ${candidateStoreFiles(userDataPath).join(' | ')}` + }) + .toBe(true) + const storePath = findStoreFileWithRepo(userDataPath, repoId) + if (!storePath) { + throw new Error(`Store file for ${repoId} disappeared after polling`) + } + return storePath +} + +/** + * Rewrite the persisted repo to the pre-fork-detection shape. + * + * Why not `repos.update`: `sanitizeRepoUpdatesForPersistence` treats an + * `undefined` upstream as "no change" and drops it, so the field cannot be + * cleared through the app. Legacy state only exists on disk. + */ +function stripPersistedUpstream(storePath: string, repoId: string): void { + const data = JSON.parse(readFileSync(storePath, 'utf8')) as { + repos?: { id: string; upstream?: unknown }[] + } + const repo = data.repos?.find((candidate) => candidate.id === repoId) + if (!repo) { + throw new Error(`Repo ${repoId} not found in ${storePath}`) + } + if (!('upstream' in repo)) { + throw new Error(`Repo ${repoId} had no persisted upstream to strip — fixture assumption broke`) + } + delete repo.upstream + writeFileSync(storePath, `${JSON.stringify(data, null, 2)}\n`) +} + +test.describe('Docker SSH fork upstream backfill', () => { + test.skip(!RUN_DOCKER_SSH, 'Set ORCA_E2E_SSH_DOCKER=1 to run Docker-backed SSH tests.') + test.skip(process.platform === 'win32', 'Docker SSH fixtures use POSIX tooling.') + + test('backfills a legacy SSH fork and renders its badge once the connection comes up', async (// oxlint-disable-next-line no-empty-pattern -- Playwright's second fixture arg is testInfo; the first must be an object destructure to opt out of the default fixture set. + {}, testInfo) => { + test.slow() + let target: DockerSshRelayTarget | null = null + const session = createRestartSession(testInfo) + + try { + target = startDockerSshRelayTarget(testInfo) + seedForkRepoOnTarget(target) + + // ── Launch 1: add the remote fork the normal way ────────────────── + const first = await session.launch() + const remote = await connectDockerSshRelayTarget(first.page, target, { + remotePath: FORK_REPO_PATH + }) + // Add-time detection already handles SSH, so this proves the probe itself + // works over the relay — isolating the bug to the startup backfill. + await expect + .poll(() => readRepoUpstream(first.page, remote.repoId), { + timeout: 60_000, + message: 'add-time detection never resolved the fork upstream over SSH' + }) + .toMatchObject(EXPECTED_UPSTREAM) + // Why: ask the app where it actually persists rather than assuming the + // launch dir — profile redirection would silently strip nothing. + const userDataPath = await first.app.evaluate(({ app }) => app.getPath('userData')) + const storePath = await waitForPersistedRepo(userDataPath, remote.repoId) + await session.close(first.app) + + stripPersistedUpstream(storePath, remote.repoId) + + // ── Launch 2: the repo now looks like it predates fork detection ── + const second = await session.launch() + // Why poll: this doubles as the hydration barrier — the relaunched window + // swaps its document once before the store is readable. + await expect + .poll(() => readRepoUpstream(second.page, remote.repoId), { + timeout: 30_000, + message: 'relaunched app never surfaced the stripped repo as unresolved' + }) + .toBe('absent') + await expect(second.page.getByLabel(FORK_BADGE_LABEL)).toHaveCount(0) + + const connected = await second.page.evaluate(async (targetId) => { + const store = window.__store + if (!store) { + throw new Error('Store unavailable') + } + const state = await window.api.ssh.connect({ targetId }) + if (!state || state.status !== 'connected') { + throw new Error(`SSH target did not connect: ${JSON.stringify(state)}`) + } + store.getState().setSshConnectionState(targetId, state) + return state.status + }, remote.targetId) + expect(connected).toBe('connected') + + // ── The fix: connect alone backfills, with no settings visit ────── + await expect + .poll(() => readRepoUpstream(second.page, remote.repoId), { + timeout: 90_000, + message: 'connecting the SSH target did not backfill the fork upstream' + }) + .toMatchObject(EXPECTED_UPSTREAM) + await expect(second.page.getByLabel(FORK_BADGE_LABEL).first()).toBeVisible({ + timeout: 30_000 + }) + + testInfo.annotations.push({ + type: 'ssh-fork-upstream-backfill', + description: `target=${remote.targetId} repo=${remote.repoId} upstream=stablyai/orca` + }) + await session.close(second.app) + } finally { + await session.dispose() + cleanupDockerSshRelayTarget(target) + } + }) +}) From 6bfb86f7bd6829104326a3bcc7ef9ceae089d9fc Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Fri, 7 Aug 2026 02:46:03 -0400 Subject: [PATCH 3/4] fix(repo): harden SSH fork upstream backfill --- ...runtime-fork-upstream-ssh-backfill.test.ts | 200 +++++++++++++++--- src/main/runtime/orca-runtime.ts | 144 ++++++++++--- 2 files changed, 289 insertions(+), 55 deletions(-) diff --git a/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts b/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts index 2cc6e92b2461..cd8af11ecd27 100644 --- a/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts +++ b/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts @@ -1,14 +1,20 @@ /** * A fork on an SSH host used to keep `upstream === undefined` until someone opened * its settings page — so its GitHub Project rows stayed hidden and its fork badge - * never rendered (#12967). Connect is the first moment the probe can succeed, so - * the backfill runs there: best-effort, sequential, and never awaited by callers. + * never rendered (#12967). Connected is the first published state with a ready + * provider, so the backfill runs there: best-effort, bounded, generation-safe, + * and never awaited by callers. */ -import { describe, expect, it, vi, beforeEach } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { Repo } from '../../shared/types' import type { SshConnectionState } from '../../shared/ssh-types' import { OrcaRuntimeService } from './orca-runtime' import { getRepoUpstream } from '../github/client' +import { + getSshGitProvider, + registerSshGitProvider, + unregisterSshGitProvider +} from '../providers/ssh-git-dispatch' const getRepoUpstreamMock = vi.hoisted(() => vi.fn()) @@ -18,6 +24,7 @@ vi.mock('../github/client', async (importOriginal) => ({ })) const UPSTREAM = { owner: 'stablyai', repo: 'orca' } +const registeredConnectionIds = new Set() function makeRepo(overrides: Partial): Repo { return { @@ -58,6 +65,19 @@ function sshState(targetId: string, status: SshConnectionState['status']): SshCo return { targetId, status, error: null, reconnectAttempt: 0 } } +function notifyConnected(runtime: OrcaRuntimeService, targetId: string): void { + registeredConnectionIds.add(targetId) + if (!getSshGitProvider(targetId)) { + registerSshGitProvider(targetId, {} as never) + } + runtime.notifySshStateChanged(targetId, sshState(targetId, 'connected')) +} + +function notifyDisconnected(runtime: OrcaRuntimeService, targetId: string): void { + unregisterSshGitProvider(targetId) + runtime.notifySshStateChanged(targetId, sshState(targetId, 'disconnected')) +} + // Why: the backfill is deliberately fire-and-forget, so tests drain the queue // rather than awaiting a handle the production caller never has. async function drainBackfill(): Promise { @@ -71,12 +91,19 @@ beforeEach(() => { getRepoUpstreamMock.mockResolvedValue(null) }) +afterEach(() => { + for (const connectionId of registeredConnectionIds) { + unregisterSshGitProvider(connectionId) + } + registeredConnectionIds.clear() +}) + describe('fork upstream backfill for SSH repos', () => { it('resolves the upstream when the connection comes up', async () => { getRepoUpstreamMock.mockResolvedValue(UPSTREAM) const { runtime, repos } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + notifyConnected(runtime, 'ssh-1') await drainBackfill() expect(getRepoUpstream).toHaveBeenCalledWith('/srv/orca', 'ssh-1') @@ -99,7 +126,7 @@ describe('fork upstream backfill for SSH repos', () => { }) ]) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + notifyConnected(runtime, 'ssh-1') await drainBackfill() expect(repos[0].repoIcon).toMatchObject({ type: 'image', source: 'github' }) @@ -115,63 +142,186 @@ describe('fork upstream backfill for SSH repos', () => { makeRepo({ id: 'resolved', path: '/srv/c', connectionId: 'ssh-1', upstream: null }) ]) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + notifyConnected(runtime, 'ssh-1') await drainBackfill() expect(getRepoUpstream).not.toHaveBeenCalled() }) - it('probes each repo once, not once per reconnect', async () => { - const { runtime } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) + it('leaves best-effort null unresolved without retrying every reconnect', async () => { + const { runtime, repos, updateRepo } = createRuntime([ + makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' }) + ]) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + notifyConnected(runtime, 'ssh-1') runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) await drainBackfill() - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'disconnected')) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + notifyDisconnected(runtime, 'ssh-1') + notifyConnected(runtime, 'ssh-1') await drainBackfill() expect(getRepoUpstream).toHaveBeenCalledTimes(1) + expect(repos[0].upstream).toBeUndefined() + expect(updateRepo).not.toHaveBeenCalled() }) it('retries on the next connect when the probe threw', async () => { getRepoUpstreamMock.mockRejectedValueOnce(new Error('ssh channel closed')) const { runtime, repos } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + notifyConnected(runtime, 'ssh-1') await drainBackfill() expect(repos[0].upstream).toBeUndefined() getRepoUpstreamMock.mockResolvedValue(UPSTREAM) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + notifyDisconnected(runtime, 'ssh-1') + notifyConnected(runtime, 'ssh-1') + await drainBackfill() + + expect(repos[0].upstream).toEqual(UPSTREAM) + }) + + it('retries a newer provider generation when reconnect wins the failure race', async () => { + let rejectProbe: ((error: Error) => void) | undefined + getRepoUpstreamMock + .mockImplementationOnce( + () => + new Promise((_, reject) => { + rejectProbe = reject + }) + ) + .mockResolvedValueOnce(UPSTREAM) + const { runtime, repos } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) + + notifyConnected(runtime, 'ssh-1') + await drainBackfill() + notifyDisconnected(runtime, 'ssh-1') + notifyConnected(runtime, 'ssh-1') + rejectProbe?.(new Error('old provider disconnected')) + await drainBackfill() + + expect(getRepoUpstream).toHaveBeenCalledTimes(2) + expect(repos[0].upstream).toEqual(UPSTREAM) + }) + + it('discards a stale null returned after the provider changed', async () => { + let resolveProbe: ((value: null) => void) | undefined + getRepoUpstreamMock + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveProbe = resolve + }) + ) + .mockResolvedValueOnce(UPSTREAM) + const { runtime, repos } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) + + notifyConnected(runtime, 'ssh-1') + await drainBackfill() + notifyDisconnected(runtime, 'ssh-1') + notifyConnected(runtime, 'ssh-1') + resolveProbe?.(null) await drainBackfill() + expect(getRepoUpstream).toHaveBeenCalledTimes(2) expect(repos[0].upstream).toEqual(UPSTREAM) }) - it('never overlaps probes when several connections come up at once', async () => { + it('lets a healthy host pass a stalled host while bounding concurrent probes', async () => { let inFlight = 0 let maxInFlight = 0 - getRepoUpstreamMock.mockImplementation(async () => { + let releaseA: (() => void) | undefined + let releaseB: (() => void) | undefined + getRepoUpstreamMock.mockImplementation(async (path) => { inFlight += 1 maxInFlight = Math.max(maxInFlight, inFlight) - await Promise.resolve() - inFlight -= 1 - return null + try { + if (path === '/srv/a') { + await new Promise((resolve) => { + releaseA = resolve + }) + } + if (path === '/srv/b') { + await new Promise((resolve) => { + releaseB = resolve + }) + } + return UPSTREAM + } finally { + inFlight -= 1 + } }) - const { runtime } = createRuntime([ + const { runtime, repos } = createRuntime([ makeRepo({ id: 'a', path: '/srv/a', connectionId: 'ssh-1' }), makeRepo({ id: 'b', path: '/srv/b', connectionId: 'ssh-2' }), makeRepo({ id: 'c', path: '/srv/c', connectionId: 'ssh-3' }) ]) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) - runtime.notifySshStateChanged('ssh-2', sshState('ssh-2', 'connected')) - runtime.notifySshStateChanged('ssh-3', sshState('ssh-3', 'connected')) + notifyConnected(runtime, 'ssh-1') + notifyConnected(runtime, 'ssh-2') + notifyConnected(runtime, 'ssh-3') await drainBackfill() - expect(getRepoUpstream).toHaveBeenCalledTimes(3) - expect(maxInFlight).toBe(1) + expect(getRepoUpstream).toHaveBeenCalledTimes(2) + expect(getRepoUpstream).not.toHaveBeenCalledWith('/srv/c', 'ssh-3') + expect(maxInFlight).toBe(2) + + releaseB?.() + await drainBackfill() + + expect(repos[1].upstream).toEqual(UPSTREAM) + expect(repos[2].upstream).toEqual(UPSTREAM) + expect(repos[0].upstream).toBeUndefined() + expect(maxInFlight).toBe(2) + + releaseA?.() + await drainBackfill() + + expect(repos[0].upstream).toEqual(UPSTREAM) + }) + + it('does not persist a stale null when a queued provider disconnects', async () => { + let releaseA: (() => void) | undefined + let releaseB: (() => void) | undefined + getRepoUpstreamMock.mockImplementation((path) => { + if (path === '/srv/a') { + return new Promise((resolve) => { + releaseA = () => resolve(null) + }) + } + if (path === '/srv/b') { + return new Promise((resolve) => { + releaseB = () => resolve(null) + }) + } + return Promise.resolve(UPSTREAM) + }) + const { runtime, repos } = createRuntime([ + makeRepo({ id: 'a', path: '/srv/a', connectionId: 'ssh-1' }), + makeRepo({ id: 'b', path: '/srv/b', connectionId: 'ssh-2' }), + makeRepo({ id: 'c', path: '/srv/c', connectionId: 'ssh-3' }) + ]) + + notifyConnected(runtime, 'ssh-1') + notifyConnected(runtime, 'ssh-2') + await drainBackfill() + expect(getRepoUpstream).toHaveBeenCalledTimes(2) + + notifyConnected(runtime, 'ssh-3') + notifyDisconnected(runtime, 'ssh-3') + releaseA?.() + await drainBackfill() + + expect(repos[2].upstream).toBeUndefined() + expect(getRepoUpstream).not.toHaveBeenCalledWith('/srv/c', 'ssh-3') + + notifyConnected(runtime, 'ssh-3') + await drainBackfill() + + expect(repos[2].upstream).toEqual(UPSTREAM) + + releaseB?.() + await drainBackfill() }) it('returns from the connect notification without waiting on the probe', async () => { @@ -184,7 +334,7 @@ describe('fork upstream backfill for SSH repos', () => { ) const { runtime, repos } = createRuntime([makeRepo({ id: 'ssh-fork', connectionId: 'ssh-1' })]) - runtime.notifySshStateChanged('ssh-1', sshState('ssh-1', 'connected')) + notifyConnected(runtime, 'ssh-1') expect(getRepoUpstream).not.toHaveBeenCalled() await drainBackfill() diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 2befd211dc3c..c1849dcab4e9 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2787,6 +2787,13 @@ function getSetupRunnerCommandPlatformForLaunch( return getSetupRunnerCommandPlatformForPath(setup?.runnerScriptPath ?? '', fallbackPlatform) } +type ForkUpstreamSshScope = { + connectionId: string + providerGeneration: number +} + +const FORK_UPSTREAM_BACKFILL_CONCURRENCY = 2 + export class OrcaRuntimeService { private readonly runtimeId = randomUUID() private readonly startedAt = Date.now() @@ -2920,10 +2927,11 @@ export class OrcaRuntimeService { >() private worktreeLifecycleListeners = new Set<(event: RuntimeWorktreeLifecycleEvent) => void>() private forkBackfillStarted = false - // Why: one chain keeps every backfill pass sequential, so a burst of SSH - // connects can never fan out `gh repo view` calls past the shared semaphore. - private forkUpstreamBackfillQueue: Promise = Promise.resolve() + private forkUpstreamBackfillActivePasses = 0 + private forkUpstreamBackfillWaiters: (() => void)[] = [] private forkUpstreamBackfilledConnectionIds = new Set() + private forkUpstreamBackfillReadyGenerationByConnectionId = new Map() + private forkUpstreamBackfillRunningConnectionIds = new Set() private agentBrowserBridge: AgentBrowserBridge | null = null private offscreenBrowserBackend: BrowserBackend | null = null private emulatorBridge: EmulatorBridge | null = null @@ -5300,6 +5308,7 @@ export class OrcaRuntimeService { this.invalidateSshWorktreeScanCache(targetId) if (state.status !== 'connected') { this.cancelLegacyWorkerTerminalRecoveryRetry(`ssh:${targetId}`) + this.forkUpstreamBackfillReadyGenerationByConnectionId.delete(targetId) } else { this.backfillForkUpstreamsForConnection(targetId) } @@ -19400,7 +19409,9 @@ export class OrcaRuntimeService { // startup for repos whose host is already reachable; SSH repos wait for their // connection instead (their host may not be up yet). private backfillForkUpstreams(): Promise { - return this.queueForkUpstreamBackfill((repo) => !repo.connectionId) + return this.queueForkUpstreamBackfill(() => + this.runForkUpstreamBackfill((repo) => !repo.connectionId) + ) } // Why: an SSH fork used to stay `upstream === undefined` until someone opened @@ -19408,36 +19419,99 @@ export class OrcaRuntimeService { // no visible cause (#12967). Connect is the first moment the probe can succeed. // Fire-and-forget so nothing on the connection path waits on the network. private backfillForkUpstreamsForConnection(connectionId: string): void { - if (this.forkUpstreamBackfilledConnectionIds.has(connectionId)) { + if ( + this.forkUpstreamBackfilledConnectionIds.has(connectionId) || + !getSshGitProvider(connectionId) + ) { return } - this.forkUpstreamBackfilledConnectionIds.add(connectionId) - void this.queueForkUpstreamBackfill((repo) => repo.connectionId === connectionId) - .then((complete) => { - // Why: a pass interrupted by a probe failure left `upstream` unset, so let - // the next connect retry rather than stranding it until the app restarts. - if (!complete) { - this.forkUpstreamBackfilledConnectionIds.delete(connectionId) + this.forkUpstreamBackfillReadyGenerationByConnectionId.set( + connectionId, + getSshGitProviderGeneration(connectionId) + ) + if (this.forkUpstreamBackfillRunningConnectionIds.has(connectionId)) { + return + } + this.forkUpstreamBackfillRunningConnectionIds.add(connectionId) + void this.drainForkUpstreamBackfillForConnection(connectionId) + .catch(() => undefined) + .finally(() => { + this.forkUpstreamBackfillRunningConnectionIds.delete(connectionId) + if ( + !this.forkUpstreamBackfilledConnectionIds.has(connectionId) && + this.forkUpstreamBackfillReadyGenerationByConnectionId.has(connectionId) + ) { + this.backfillForkUpstreamsForConnection(connectionId) } }) - .catch(() => { - this.forkUpstreamBackfilledConnectionIds.delete(connectionId) - }) } - // Why: chained, never concurrent — `getRepoUpstream` shells out to `gh` behind a - // process-global semaphore, so passes queue instead of fanning out. - private queueForkUpstreamBackfill(selects: (repo: Repo) => boolean): Promise { - const pass = this.forkUpstreamBackfillQueue.then(() => this.runForkUpstreamBackfill(selects)) - this.forkUpstreamBackfillQueue = pass.then( - () => undefined, - () => undefined + // Why: bound migration subprocesses without letting one stalled host block every other host. + private async queueForkUpstreamBackfill(run: () => Promise): Promise { + await this.acquireForkUpstreamBackfillPermit() + try { + return await run() + } finally { + this.releaseForkUpstreamBackfillPermit() + } + } + + private acquireForkUpstreamBackfillPermit(): Promise { + if (this.forkUpstreamBackfillActivePasses < FORK_UPSTREAM_BACKFILL_CONCURRENCY) { + this.forkUpstreamBackfillActivePasses += 1 + return Promise.resolve() + } + return new Promise((resolve) => this.forkUpstreamBackfillWaiters.push(resolve)) + } + + private releaseForkUpstreamBackfillPermit(): void { + const next = this.forkUpstreamBackfillWaiters.shift() + if (next) { + next() + return + } + this.forkUpstreamBackfillActivePasses -= 1 + } + + private async drainForkUpstreamBackfillForConnection(connectionId: string): Promise { + while (!this.forkUpstreamBackfilledConnectionIds.has(connectionId)) { + const providerGeneration = + this.forkUpstreamBackfillReadyGenerationByConnectionId.get(connectionId) + if (providerGeneration === undefined) { + return + } + const scope = { connectionId, providerGeneration } + const complete = await this.queueForkUpstreamBackfill(() => + this.runForkUpstreamBackfill((repo) => repo.connectionId === connectionId, scope) + ) + if (complete) { + this.forkUpstreamBackfilledConnectionIds.add(connectionId) + this.forkUpstreamBackfillReadyGenerationByConnectionId.delete(connectionId) + return + } + if ( + this.forkUpstreamBackfillReadyGenerationByConnectionId.get(connectionId) === + providerGeneration + ) { + this.forkUpstreamBackfillReadyGenerationByConnectionId.delete(connectionId) + } + } + } + + private isForkUpstreamBackfillScopeCurrent(scope: ForkUpstreamSshScope): boolean { + return ( + this.forkUpstreamBackfillReadyGenerationByConnectionId.get(scope.connectionId) === + scope.providerGeneration && + getSshGitProviderGeneration(scope.connectionId) === scope.providerGeneration && + getSshGitProvider(scope.connectionId) !== undefined ) - return pass } /** Resolves false when any probe failed, so the caller can retry that scope later. */ - private async runForkUpstreamBackfill(selects: (repo: Repo) => boolean): Promise { + private async runForkUpstreamBackfill( + selects: (repo: Repo) => boolean, + sshScope?: ForkUpstreamSshScope + ): Promise { let complete = true try { const store = this.requireStore() @@ -19446,16 +19520,26 @@ export class OrcaRuntimeService { if (repo.upstream !== undefined || repo.kind === 'folder' || !selects(repo)) { continue } + if (sshScope && !this.isForkUpstreamBackfillScopeCurrent(sshScope)) { + complete = false + break + } let upstream: GitHubOwnerRepo | null try { - // Why: same resolution the settings page uses, so backfilled and - // lazily-resolved upstreams can never disagree for the same repo. - const options = this.getHostedReviewExecutionOptions(repo) - upstream = await (options - ? getRepoUpstream(repo.path, repo.connectionId ?? null, options) - : getRepoUpstream(repo.path, repo.connectionId ?? null)) + upstream = await getRepoUpstream(repo.path, repo.connectionId ?? null) } catch { complete = false + if (sshScope && !this.isForkUpstreamBackfillScopeCurrent(sshScope)) { + break + } + continue + } + if (sshScope && !this.isForkUpstreamBackfillScopeCurrent(sshScope)) { + complete = false + break + } + // Why: SSH null may hide a transient Git/gh failure, so only persist a positive result. + if (sshScope && !upstream) { continue } const updates: Partial = { upstream: upstream ?? null } From 665ca639b14b47cd2ad73e290f927b4cc6a307d6 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Mon, 10 Aug 2026 00:27:59 -0400 Subject: [PATCH 4/4] test(e2e): harden SSH backfill review coverage --- config/scripts/pnpm-cli-spawn.mjs | 16 ++++++++ config/scripts/pnpm-cli-spawn.test.mjs | 34 +++++++++++++++ ...-ssh-docker-fork-upstream-backfill-e2e.mjs | 41 +++++++++---------- ...runtime-fork-upstream-ssh-backfill.test.ts | 19 ++++----- .../ssh-docker-fork-upstream-backfill.spec.ts | 14 +------ 5 files changed, 79 insertions(+), 45 deletions(-) create mode 100644 config/scripts/pnpm-cli-spawn.mjs create mode 100644 config/scripts/pnpm-cli-spawn.test.mjs diff --git a/config/scripts/pnpm-cli-spawn.mjs b/config/scripts/pnpm-cli-spawn.mjs new file mode 100644 index 000000000000..f1e847a36982 --- /dev/null +++ b/config/scripts/pnpm-cli-spawn.mjs @@ -0,0 +1,16 @@ +export function resolvePnpmCliSpawn( + args, + { + platform = process.platform, + nodePath = process.execPath, + npmExecPath = process.env.npm_execpath + } = {} +) { + if (platform !== 'win32') { + return { command: 'pnpm', args } + } + if (!npmExecPath) { + throw new Error('Run this command through pnpm so its Windows CLI path is available.') + } + return { command: nodePath, args: [npmExecPath, ...args] } +} diff --git a/config/scripts/pnpm-cli-spawn.test.mjs b/config/scripts/pnpm-cli-spawn.test.mjs new file mode 100644 index 000000000000..18d1eace838c --- /dev/null +++ b/config/scripts/pnpm-cli-spawn.test.mjs @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest' +import { resolvePnpmCliSpawn } from './pnpm-cli-spawn.mjs' + +describe('resolvePnpmCliSpawn', () => { + it('uses the executable directly on POSIX', () => { + expect(resolvePnpmCliSpawn(['run', 'typecheck'], { platform: 'linux' })).toEqual({ + command: 'pnpm', + args: ['run', 'typecheck'] + }) + }) + + it('preserves whitespace arguments without a Windows command shell', () => { + expect( + resolvePnpmCliSpawn(['exec', 'playwright', '--grep', 'legacy SSH fork'], { + platform: 'win32', + nodePath: 'C:\\Program Files\\nodejs\\node.exe', + npmExecPath: 'C:\\pnpm\\pnpm.cjs' + }) + ).toEqual({ + command: 'C:\\Program Files\\nodejs\\node.exe', + args: ['C:\\pnpm\\pnpm.cjs', 'exec', 'playwright', '--grep', 'legacy SSH fork'] + }) + }) + + it('requires pnpm package-script context on Windows', () => { + expect(() => + resolvePnpmCliSpawn([], { + platform: 'win32', + nodePath: 'node.exe', + npmExecPath: undefined + }) + ).toThrow('Run this command through pnpm') + }) +}) diff --git a/config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs b/config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs index 4bcf6502164c..a174cf1b1b0f 100644 --- a/config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs +++ b/config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjs @@ -1,41 +1,40 @@ import { spawnSync } from 'node:child_process' +import { resolvePnpmCliSpawn } from './pnpm-cli-spawn.mjs' const rawExtraArgs = process.argv.slice(2) const extraArgs = rawExtraArgs[0] === '--' ? rawExtraArgs.slice(1) : rawExtraArgs -const pnpm = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm' const env = { ...process.env, ORCA_E2E_SSH_DOCKER: '1' } -// Why: Node's CVE-2024-27980 hardening rejects .cmd spawns without shell on Windows. const spawnOptions = { stdio: 'inherit', - env, - shell: process.platform === 'win32' + env } -const runtime = spawnSync(pnpm, ['run', 'ensure:electron-runtime'], spawnOptions) +function runPnpm(args) { + const invocation = resolvePnpmCliSpawn(args) + return spawnSync(invocation.command, invocation.args, spawnOptions) +} + +const runtime = runPnpm(['run', 'ensure:electron-runtime']) if (runtime.status !== 0) { process.exit(runtime.status ?? 1) } -const result = spawnSync( - pnpm, - [ - 'exec', - 'playwright', - 'test', - 'tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts', - '--config', - 'tests/playwright.config.ts', - '--project', - 'electron-headless', - '--workers=1', - ...extraArgs - ], - spawnOptions -) +const result = runPnpm([ + 'exec', + 'playwright', + 'test', + 'tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts', + '--config', + 'tests/playwright.config.ts', + '--project', + 'electron-headless', + '--workers=1', + ...extraArgs +]) process.exit(result.status ?? 1) diff --git a/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts b/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts index cd8af11ecd27..a93fa929d690 100644 --- a/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts +++ b/src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts @@ -1,10 +1,4 @@ -/** - * A fork on an SSH host used to keep `upstream === undefined` until someone opened - * its settings page — so its GitHub Project rows stayed hidden and its fork badge - * never rendered (#12967). Connected is the first published state with a ready - * provider, so the backfill runs there: best-effort, bounded, generation-safe, - * and never awaited by callers. - */ +// Legacy SSH forks backfill only after a ready provider publishes connected (#12967). import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { Repo } from '../../shared/types' import type { SshConnectionState } from '../../shared/ssh-types' @@ -78,8 +72,7 @@ function notifyDisconnected(runtime: OrcaRuntimeService, targetId: string): void runtime.notifySshStateChanged(targetId, sshState(targetId, 'disconnected')) } -// Why: the backfill is deliberately fire-and-forget, so tests drain the queue -// rather than awaiting a handle the production caller never has. +// Why: fire-and-forget production work exposes no handle, so tests flush its microtask queue. async function drainBackfill(): Promise { for (let tick = 0; tick < 20; tick += 1) { await Promise.resolve() @@ -129,8 +122,12 @@ describe('fork upstream backfill for SSH repos', () => { notifyConnected(runtime, 'ssh-1') await drainBackfill() - expect(repos[0].repoIcon).toMatchObject({ type: 'image', source: 'github' }) - expect(repos[0].repoIcon).not.toMatchObject({ src: 'https://avatars/fork.png' }) + expect(repos[0].repoIcon).toEqual({ + type: 'image', + src: 'https://github.com/stablyai.png?size=64', + source: 'github', + label: 'stablyai/orca' + }) expect(repos[1].repoIcon).toEqual({ type: 'emoji', emoji: '🦈' }) }) diff --git a/tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts b/tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts index 1ec1d62e5b4b..996833e86de1 100644 --- a/tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts +++ b/tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts @@ -1,16 +1,4 @@ -/** - * Live proof for #12967 against a real Docker SSH host with a real relay. - * - * A fork added before fork detection existed persists no `upstream`, and the - * startup backfill used to skip every repo with a `connectionId` — so the fork - * badge never rendered and GitHub Project rows stayed hidden until someone - * opened that repo's settings page. This drives the actual user sequence: - * legacy store state on disk, app launch, SSH connect, badge appears. - * - * Deterministic and offline: `getRepoUpstream` reads the `upstream` git remote - * before it ever shells out to `gh`, so the container's remotes decide the - * answer — no GitHub auth, no network, no real fork required. - */ +// Offline proof: a distinct upstream remote resolves before gh, requiring no network or auth. import { readFileSync, readdirSync, writeFileSync } from 'node:fs' import path from 'node:path'