diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 7a5eff241e6..00739f36350 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -17,6 +17,10 @@ on: description: JSON array of changed specs; empty runs the full suite required: false type: string + ssh_source_changed: + description: '"true" when the PR touches SSH execution source; gates the Docker-SSH lane' + required: false + type: string workflow_dispatch: inputs: ref: @@ -221,14 +225,21 @@ jobs: ssh-docker-watcher-isolation: name: ssh docker watcher isolation needs: build + # Why ssh_source_changed first: this lane used to trigger on SSH source only as a side + # effect of one route listing a startup-readiness spec — pruning that spec would have + # silently retired the whole lane. The signal is now derived from the SSH routes directly. + # The two spec clauses stay for their honest purpose: changed-e2e hands these specs to this + # lane, so editing one must still run it here. if: >- inputs.test_files == '' || + inputs.ssh_source_changed == 'true' || contains(inputs.test_files, 'tests/e2e/ssh-startup-exec-readiness.spec.ts') || contains(inputs.test_files, 'tests/e2e/paired-startup-exec-readiness.spec.ts') runs-on: ubuntu-latest - # Why 35: the parking, retention, startup-exec, and paired parity specs run - # serially on isolated Electron/SSH fixtures after watcher isolation. - timeout-minutes: 35 + # Why 60: this lane now also runs the remaining Docker-SSH specs serially. They average + # ~18s but several budget 4-10 minutes per test, so a slow run lands far above the old 35 + # — and the sharded lanes already show that a lane which times out is a lane nobody trusts. + timeout-minutes: 60 steps: - name: Checkout @@ -260,6 +271,14 @@ jobs: if: always() run: xvfb-run --auto-servernum env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm run test:e2e:ssh-docker-terminal-parking + # Why here rather than the sharded lanes: the shards set no ORCA_E2E_SSH_DOCKER, so every + # spec below skipped itself while the shard still reported green. Running them on this one + # VM pays the fixture image build once instead of ten times, and keeps an SSH regression + # legible as an SSH-named failure. + - name: Run remaining Docker SSH E2E + if: always() + run: xvfb-run --auto-servernum env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm run test:e2e:ssh-docker + - name: Upload watcher isolation traces if: failure() uses: actions/upload-artifact@v7 diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 9fbe35a173f..9f3b14f5903 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -804,6 +804,7 @@ jobs: outputs: should_run: ${{ steps.filter.outputs.should_run }} test_files: ${{ steps.filter.outputs.test_files }} + ssh_source_changed: ${{ steps.filter.outputs.ssh_source_changed }} steps: - name: Checkout uses: actions/checkout@v6 @@ -826,6 +827,11 @@ jobs: # authorities, exclusions, and sentinels without evaluating workflow shell. TEST_FILES_JSON="$(printf '%s\n' "$CHANGED" | node config/scripts/pr-e2e-source-routing.mjs)" echo "test_files=$TEST_FILES_JSON" >> "$GITHUB_OUTPUT" + # Why a separate signal: the Docker-SSH lane must trigger on SSH source, not on a + # spec name surviving in a route's list. Same routes, so the two cannot drift. + SSH_SOURCE_CHANGED="$(printf '%s\n' "$CHANGED" | node config/scripts/pr-e2e-source-routing.mjs --ssh-source)" + echo "ssh_source_changed=$SSH_SOURCE_CHANGED" >> "$GITHUB_OUTPUT" + echo "SSH source changed: $SSH_SOURCE_CHANGED" if [ "$TEST_FILES_JSON" != '[]' ]; then echo "should_run=true" >> "$GITHUB_OUTPUT" echo "Changed E2E specs: $TEST_FILES_JSON" @@ -844,6 +850,7 @@ jobs: uses: ./.github/workflows/e2e.yml with: test_files: ${{ needs.e2e-paths.outputs.test_files }} + ssh_source_changed: ${{ needs.e2e-paths.outputs.ssh_source_changed }} verify: if: always() diff --git a/config/scripts/pr-e2e-gate-contract.test.mjs b/config/scripts/pr-e2e-gate-contract.test.mjs index 665edd01ec0..8cd622b300c 100644 --- a/config/scripts/pr-e2e-gate-contract.test.mjs +++ b/config/scripts/pr-e2e-gate-contract.test.mjs @@ -1,9 +1,14 @@ -import { existsSync, readFileSync } from 'node:fs' +import { existsSync, readdirSync, readFileSync } from 'node:fs' import { join, resolve } from 'node:path' import { parse as parseJsonc } from 'jsonc-parser' import { describe, expect, it } from 'vitest' import { parse as parseYaml } from 'yaml' -import { PR_E2E_SOURCE_ROUTES, selectPrE2eSpecs } from './pr-e2e-source-routing.mjs' +import { + hasSshSourceChange, + PR_E2E_SOURCE_ROUTES, + selectPrE2eSpecs, + SSH_SOURCE_ROUTE_IDS +} from './pr-e2e-source-routing.mjs' const projectDir = resolve(import.meta.dirname, '../..') const prWorkflow = parseYaml(readFileSync(join(projectDir, '.github/workflows/pr.yml'), 'utf8')) @@ -209,6 +214,8 @@ describe('PR E2E gate contract', () => { 'src/main/providers/ssh-', 'src/main/ipc/pty', 'src/relay/', + 'src/shared/ssh-', + 'src/renderer/src/store/slices/direct-ssh-', 'src/renderer/src/components/terminal-pane/remote-runtime-' ] for (const authority of sshSourceAuthorities) { @@ -217,10 +224,24 @@ describe('PR E2E gate contract', () => { ) } + // Why named files rather than prefixes: these seams are single modules, and a prefix + // here would route their unrelated neighbours. + for (const file of [ + 'src/main/runtime/public-ssh-state.ts', + 'src/renderer/src/startup/ssh-startup-reconnect.ts', + 'src/renderer/src/store/slices/ssh.ts' + ]) { + expect(selectPrE2eSpecs([file]), file).toContain( + 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts' + ) + } + const mappedSpecs = [ 'tests/e2e/pty-input-write-queue-ssh.spec.ts', 'tests/e2e/ssh-cold-activation-restore.spec.ts', 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts', + 'tests/e2e/ssh-port-forward-lifecycle.spec.ts', + 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts', 'tests/e2e/ssh-startup-exec-readiness.spec.ts', 'tests/e2e/ssh-terminal-window-wake-stale-grid-repro.spec.ts' ] @@ -249,6 +270,140 @@ describe('PR E2E gate contract', () => { expect(changedInstall.run).toContain('openssh-client') }) + it('routes direct-SSH workspace and tab restore from its unnamed source seams', () => { + // Why by name: none of these carry "ssh", so the SSH authorities above never reach them + // — a closed-tab tombstone and a dropped default-tabs marker both shipped through it. + for (const file of [ + 'src/renderer/src/hooks/remote-workspace-session-merge.ts', + 'src/main/ipc/remote-workspace-snapshot-normalization.ts', + 'src/renderer/src/lib/worktree-initial-terminal-seeding.ts', + 'src/renderer/src/lib/worktree-default-terminal-tabs.ts', + 'src/shared/remote-workspace-session-projection.ts', + 'src/renderer/src/components/terminal/initial-terminal.ts' + ]) { + const specs = selectPrE2eSpecs([file]) + expect(specs, file).toContain('tests/e2e/ssh-cold-activation-restore.spec.ts') + expect(specs, file).toContain('tests/e2e/ssh-reconnect-tab-destruction.spec.ts') + } + + expect( + selectPrE2eSpecs(['src/renderer/src/hooks/remote-workspace-session-merge.test.ts']) + ).toEqual([]) + }) + + it('triggers the Docker-SSH lane from SSH source, not from a spec name', () => { + // The behavioural half of the invariant, and the part that actually matters: an SSH source + // edit is recognised as one, through the same routes that select the specs. + for (const file of [ + 'src/main/ssh/connection.ts', + 'src/relay/pty-handler.ts', + 'src/renderer/src/store/slices/direct-ssh-pane-retry-ledger.ts', + 'src/renderer/src/hooks/remote-workspace-session-merge.ts', + 'src/main/ipc/remote-workspace-snapshot-normalization.ts' + ]) { + expect(hasSshSourceChange([file]), file).toBe(true) + } + for (const file of [ + 'src/main/git/git-status.ts', + 'src/renderer/src/components/tab-bar/BrowserTab.tsx', + 'src/main/ssh/connection.test.ts' + ]) { + expect(hasSshSourceChange([file]), file).toBe(false) + } + + // Why: the signal must stay derived from the routes. A route id that no longer exists would + // silently narrow it to nothing. + for (const id of SSH_SOURCE_ROUTE_IDS) { + expect( + PR_E2E_SOURCE_ROUTES.map((route) => route.id), + id + ).toContain(id) + } + + // Why text and not structure: a job `if:` is only ever available as a string. The strongest + // available assertion is that the source signal is its own disjunct, so the lane no longer + // depends on a spec name surviving in a route's spec list. + const sshLaneCondition = e2eWorkflow.jobs['ssh-docker-watcher-isolation'].if + expect(sshLaneCondition).toContain("inputs.ssh_source_changed == 'true' ||") + + expect(e2eWorkflow.on.workflow_call.inputs.ssh_source_changed.type).toBe('string') + expect(prWorkflow.jobs['e2e-paths'].outputs.ssh_source_changed).toBe( + '${{ steps.filter.outputs.ssh_source_changed }}' + ) + expect(prWorkflow.jobs.e2e.with.ssh_source_changed).toBe( + '${{ needs.e2e-paths.outputs.ssh_source_changed }}' + ) + expect(filterStep.run).toContain('pr-e2e-source-routing.mjs --ssh-source') + expect(filterStep.run).toContain('ssh_source_changed=$SSH_SOURCE_CHANGED') + }) + + it('gives every Docker-gated SSH spec a lane that runs it', () => { + // Why this shape: the sharded lanes set no ORCA_E2E_SSH_DOCKER, so a Docker-gated spec + // that no runner names runs nowhere and still reports green — the silent skip this file + // exists to prevent. Asserting reachability rather than a literal keeps that true when + // the lanes move. + // Why these two are exempt: each needs something CI cannot give it, recorded in + // run-ssh-docker-e2e.mjs so the gap stays legible rather than looking like coverage. + const unreachableSpecs = new Set([ + 'tests/e2e/ssh-docker-relay-perf.spec.ts', + 'tests/e2e/ssh-codex-display-artifacts-repro.spec.ts', + 'tests/e2e/ssh-docker-bulk-open-freeze-repro.spec.ts' + ]) + // Why comments are stripped: this file's own runner lists the two exempt specs by name in a + // prose comment. A substring scan over raw text would count any spec merely *discussed* in a + // runner as claimed by it -- the silent skip this assertion exists to catch, re-entering + // through the documentation. + const stripComments = (text) => + text.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, '') + const laneRunners = [ + 'run-ssh-docker-e2e.mjs', + 'run-ssh-docker-watcher-isolation-e2e.mjs', + 'run-ssh-docker-terminal-parking-e2e.mjs' + ].map((file) => stripComments(readFileSync(join(projectDir, 'config/scripts', file), 'utf8'))) + + // Why a comparison and not the bare name: preview and demo specs cite the flag in a + // "how to run me" comment without gating on it. Why a regex rather than one literal: an + // equally-valid spelling (double quotes, or a `!==` guard) would escape a fixed-string scan + // and the spec would silently leave the contract. + const dockerGateExpression = /ORCA_E2E_SSH_DOCKER\s*[!=]==\s*['"]1['"]/ + const dockerGatedSpecs = readdirSync(join(projectDir, 'tests/e2e')) + .filter((file) => file.endsWith('.spec.ts')) + .map((file) => `tests/e2e/${file}`) + .filter((spec) => dockerGateExpression.test(readFileSync(join(projectDir, spec), 'utf8'))) + expect(dockerGatedSpecs.length).toBeGreaterThan(0) + + const unclaimed = dockerGatedSpecs.filter( + (spec) => !unreachableSpecs.has(spec) && !laneRunners.some((runner) => runner.includes(spec)) + ) + expect( + unclaimed, + `Docker-gated specs claimed by no lane runner: ${unclaimed.join(', ')}` + ).toEqual([]) + + // Why: an exemption that outlives its spec would quietly excuse a real gap. + for (const spec of unreachableSpecs) { + expect(dockerGatedSpecs, spec).toContain(spec) + // Why also assert absence from every runner: `unreachableSpecs` short-circuits the + // unclaimed check above, so a spec could be documented as exempt while a runner still + // invokes it -- an exemption that reads as coverage removal but changes nothing, and a + // lane that stays red for a reason the file says it excluded. + for (const runner of laneRunners) { + expect(runner.includes(spec), `${spec} is exempt but still invoked by a lane runner`).toBe( + false + ) + } + } + + const laneStep = e2eWorkflow.jobs['ssh-docker-watcher-isolation'].steps.find( + (step) => step.name === 'Run remaining Docker SSH E2E' + ) + expect(laneStep.run).toContain('test:e2e:ssh-docker') + // Why: the added serial tests, several budgeting 4-10 minutes each, do not fit the old 35. + expect( + e2eWorkflow.jobs['ssh-docker-watcher-isolation']['timeout-minutes'] + ).toBeGreaterThanOrEqual(60) + }) + it('scopes the VM rollback oracle to the PR range and recipe schema authorities', () => { expect(rollbackStep.run).toContain('--merge-base "$BASE_SHA" "$HEAD_SHA"') expect(rollbackStep.run).toContain('src/shared/ephemeral-vm-recipes.ts') diff --git a/config/scripts/pr-e2e-source-routing.mjs b/config/scripts/pr-e2e-source-routing.mjs index 04f570f0094..108a4d5ab10 100644 --- a/config/scripts/pr-e2e-source-routing.mjs +++ b/config/scripts/pr-e2e-source-routing.mjs @@ -18,12 +18,32 @@ export const PR_E2E_SOURCE_ROUTES = [ 'tests/e2e/pty-input-write-queue-ssh.spec.ts', 'tests/e2e/ssh-cold-activation-restore.spec.ts', 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts', + 'tests/e2e/ssh-port-forward-lifecycle.spec.ts', + 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts', 'tests/e2e/ssh-startup-exec-readiness.spec.ts', 'tests/e2e/ssh-terminal-window-wake-stale-grid-repro.spec.ts' ], + // Why the store/startup/shared additions: the SSH-named authorities stop at the main + // process and the pane component, but the reconnect ledgers and retained-payload + // admission that decide whether a pane rebinds live in the renderer store. matches: (file) => isProductSource(file) && - /^(?:src\/main\/ssh\/|src\/main\/providers\/ssh-|src\/main\/ipc\/(?:ssh-|pty)|src\/relay\/|src\/renderer\/src\/components\/terminal-pane\/(?:pty-|ssh-|remote-runtime-|terminal-parked-pty))/.test( + /^(?:src\/main\/ssh\/|src\/main\/providers\/ssh-|src\/main\/ipc\/(?:ssh-|pty)|src\/main\/runtime\/(?:public-ssh-state|ssh-file-explorer-chunk-read)\.ts|src\/relay\/|src\/shared\/(?:ssh-|skill-ssh-relay-contract)|src\/renderer\/src\/startup\/(?:ssh-startup-reconnect|startup-ssh-connection-restore)\.ts|src\/renderer\/src\/store\/slices\/(?:ssh|direct-ssh-)|src\/renderer\/src\/components\/terminal-pane\/(?:pty-|ssh-|remote-runtime-|terminal-parked-pty))/.test( + file + ) + }, + { + // Why a sibling route rather than more paths on ssh-terminal-source: these modules carry + // no "ssh" in their names, and only the two restore specs gate them. Folding them in + // would run the whole SSH terminal list for a tab-tombstone edit. + id: 'ssh-workspace-session-restore', + specs: [ + 'tests/e2e/ssh-cold-activation-restore.spec.ts', + 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts' + ], + matches: (file) => + isProductSource(file) && + /^(?:src\/main\/ipc\/remote-workspace|src\/shared\/remote-workspace-|src\/renderer\/src\/hooks\/remote-workspace-|src\/renderer\/src\/lib\/worktree-(?:initial-terminal-seeding|default-terminal-tabs)\.ts|src\/renderer\/src\/components\/terminal\/initial-terminal)/.test( file ) }, @@ -131,6 +151,18 @@ export function selectPrE2eSpecs(changedPaths, reportRoute = () => undefined) { return [...specs].sort((left, right) => left.localeCompare(right)) } +/** Routes whose authorities are SSH execution source, and so require the Docker-SSH lane. */ +export const SSH_SOURCE_ROUTE_IDS = ['ssh-terminal-source', 'ssh-workspace-session-restore'] + +// Why derive this from the routes instead of a second path list: the Docker-SSH lane used to +// trigger only because one route happened to list a startup-readiness spec, so pruning that +// spec would have silently retired the lane. Two lists that must agree is how that drifted. +export function hasSshSourceChange(changedPaths) { + return PR_E2E_SOURCE_ROUTES.filter((route) => SSH_SOURCE_ROUTE_IDS.includes(route.id)).some( + (route) => changedPaths.some(route.matches) + ) +} + if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { let input = '' process.stdin.setEncoding('utf8') @@ -138,6 +170,10 @@ if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) input += chunk } const changedPaths = input.split(/\r?\n/).filter(Boolean) - const specs = selectPrE2eSpecs(changedPaths, (message) => console.error(message)) - process.stdout.write(`${JSON.stringify(specs)}\n`) + if (process.argv.includes('--ssh-source')) { + process.stdout.write(`${hasSshSourceChange(changedPaths)}\n`) + } else { + const specs = selectPrE2eSpecs(changedPaths, (message) => console.error(message)) + process.stdout.write(`${JSON.stringify(specs)}\n`) + } } diff --git a/config/scripts/run-ssh-docker-e2e.mjs b/config/scripts/run-ssh-docker-e2e.mjs new file mode 100644 index 00000000000..8000c8a0c26 --- /dev/null +++ b/config/scripts/run-ssh-docker-e2e.mjs @@ -0,0 +1,93 @@ +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', + ORCA_E2E_WEB_CLIENT: '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) +} + +// Why one explicit list: these specs self-skip without ORCA_E2E_SSH_DOCKER and no sharded lane +// sets it, so a spec in no runner runs nowhere. The gate contract proves every flag-reading +// spec is claimed here, by the watcher-isolation or parking runner, or by a listed exclusion. +// +// Deliberately absent, and therefore still covered by no CI trigger: +// ssh-docker-relay-perf.spec.ts — wall-clock latency thresholds; flaky budgets here would +// cost the lane its credibility. NOTE: a runner script test:e2e:ssh-docker-perf exists in +// package.json but NO workflow invokes it, so this spec currently runs in no CI lane at +// all. Recorded as a real gap, not as coverage living somewhere else. +// ssh-codex-display-artifacts-repro.spec.ts — installs a real remote codex binary that CI +// runners do not have (observed as `spawn codex ENOENT`). Runs in no CI lane at all. +// ssh-docker-bulk-open-freeze-repro.spec.ts — two reasons, both disqualifying: +// (a) it is a perf oracle, not a correctness one: SOFT_FREEZE_LAG_MS=2500 / +// HARD_FREEZE_LAG_MS=5000 measured by a renderer lag probe under a deliberate +// 5-pane output flood on a 420s budget. Same rule as ssh-docker-relay-perf above. +// (b) it is ROTTED: four call sites are out of date against terminal.ts's current +// helpers — execInTerminal gained a ptyId parameter and splitActiveTerminalPane +// gained a direction, so it cannot compile, let alone pass. Repairing it needs two +// semantic decisions (which ptyId to capture, which split direction) that change +// what the repro measures. Tracked in stablyai/orca#16764. +// +// Why both projects: ssh-port-forward-lifecycle is @headful, which the headless project +// grep-inverts away. +// +// Known gaps in SSH e2e coverage, recorded here because nothing else names them: +// - The job that runs this is still called `ssh-docker-watcher-isolation`, though watcher +// isolation is now one spec of many. Renaming it changes the GitHub check name and can +// break required-check config, so the name understates the job on purpose. +// - E2E does not gate merges: `verify.needs` in pr.yml omits `e2e` while the suite is red on +// main. Nothing in this lane blocks a PR yet. pr.yml's Require-successful-checks comment +// has the exact wiring to flip it, and the gate contract asserts the current state. +// - Five specs and one unit test are gated on env vars no workflow sets, so they run nowhere +// and are not Docker-gated, which puts them outside this file's contract: +// local-ssh-browser-routing (ORCA_E2E_LOCAL_SSH_BROWSER) +// ssh-client-hosted-browser-drop-reconnect (ORCA_E2E_SSH_CLIENT_HOSTED_BROWSER) +// nested-runtime-ssh-lifecycle, nested-runtime-ssh-routing (ORCA_E2E_NESTED_RUNTIME_SSH) +// ssh-localhost (ORCA_E2E_SSH_LOCALHOST) +// ssh-browser-network-execution-route.docker.unit.test.ts (ORCA_RUN_DOCKER_SSH_BROWSER_E2E) +// Runner scripts for the first four sit unused in package.json; no workflow calls them. +const result = spawnSync( + pnpm, + [ + 'exec', + 'playwright', + 'test', + 'tests/e2e/pty-input-write-queue-ssh.spec.ts', + 'tests/e2e/ssh-ai-vault-session-history.spec.ts', + 'tests/e2e/ssh-cold-activation-restore.spec.ts', + 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts', + 'tests/e2e/ssh-external-image-preview.spec.ts', + 'tests/e2e/ssh-pi-compatible-agent-title.spec.ts', + 'tests/e2e/ssh-port-forward-lifecycle.spec.ts', + 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts', + 'tests/e2e/ssh-restart-tab-accumulation.spec.ts', + 'tests/e2e/ssh-skill-installation.spec.ts', + 'tests/e2e/ssh-terminal-window-wake-stale-grid-repro.spec.ts', + '--config', + 'tests/playwright.config.ts', + '--project', + 'electron-headless', + '--project', + 'electron-headful', + '--workers=1', + ...extraArgs + ], + spawnOptions +) + +process.exit(result.status ?? 1) diff --git a/package.json b/package.json index 7b46b369406..c8e59299c70 100644 --- a/package.json +++ b/package.json @@ -111,6 +111,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": "node config/scripts/run-ssh-docker-e2e.mjs", "test:e2e:ssh-client-hosted-browser": "node config/scripts/run-ssh-client-hosted-browser-drop-reconnect-e2e.mjs", "test:e2e:local-ssh-browser": "node config/scripts/run-local-ssh-browser-routing-e2e.mjs", "test:e2e:ssh-docker-terminal-parking": "node config/scripts/run-ssh-docker-terminal-parking-e2e.mjs", diff --git a/tests/e2e/helpers/docker-ssh-relay-connection.ts b/tests/e2e/helpers/docker-ssh-relay-connection.ts index 53fca0dbb0c..a17ad916e66 100644 --- a/tests/e2e/helpers/docker-ssh-relay-connection.ts +++ b/tests/e2e/helpers/docker-ssh-relay-connection.ts @@ -16,6 +16,13 @@ type DockerSshRelayConnectionOptions = { relayGracePeriodSeconds?: number remotePath?: string viaProxyJump?: boolean + /** + * Seed a terminal tab when the worktree has none. Default true. + * + * Why it is optional: a spec asking whether the PRODUCT adds a tab cannot tell this helper's + * tab from the one under test, so it must be able to leave the worktree empty. + */ + seedInitialTab?: boolean } export async function connectDockerSshRelayTarget( @@ -24,7 +31,7 @@ export async function connectDockerSshRelayTarget( options: DockerSshRelayConnectionOptions = {} ): Promise { return page.evaluate( - async ({ target, remotePath, relayGracePeriodSeconds, viaProxyJump }) => { + async ({ target, remotePath, relayGracePeriodSeconds, viaProxyJump, seedInitialTab }) => { const store = window.__store if (!store) { throw new Error('Store unavailable') @@ -147,7 +154,7 @@ export async function connectDockerSshRelayTarget( throw new Error(`No remote worktree found for ${result.repo.path}`) } store.getState().setActiveWorktree(worktree.id) - if ((store.getState().tabsByWorktree[worktree.id] ?? []).length === 0) { + if (seedInitialTab && (store.getState().tabsByWorktree[worktree.id] ?? []).length === 0) { store.getState().createTab(worktree.id) } store.getState().setActiveTabType('terminal') @@ -168,6 +175,7 @@ export async function connectDockerSshRelayTarget( ? DOCKER_SSH_PROXY_JUMP_REMOTE_REPO_PATH : DOCKER_SSH_RELAY_REMOTE_REPO_PATH), viaProxyJump: options.viaProxyJump ?? false, + seedInitialTab: options.seedInitialTab ?? true, relayGracePeriodSeconds: options.relayGracePeriodSeconds ?? 1 } ) diff --git a/tests/e2e/helpers/docker-ssh-relay-terminal-tabs.ts b/tests/e2e/helpers/docker-ssh-relay-terminal-tabs.ts new file mode 100644 index 00000000000..4ed1731e1d2 --- /dev/null +++ b/tests/e2e/helpers/docker-ssh-relay-terminal-tabs.ts @@ -0,0 +1,38 @@ +import { expect, type Page } from '@stablyai/playwright-test' + +import { waitForActivePanePtyId, waitForActiveTerminalManager } from './terminal' + +/** Add one more terminal tab to a connected remote worktree and wait for it to own a PTY. */ +export async function createRemoteTerminalTab(page: Page, worktreeId: string): Promise { + const tabId = await page.evaluate((id) => { + const state = window.__store?.getState() + if (!state) { + throw new Error('Store unavailable') + } + const tab = state.createTab(id, undefined, undefined, { activate: true }) + state.setActiveTab(tab.id) + state.setActiveTabType('terminal') + return tab.id + }, worktreeId) + await expect + .poll(() => page.evaluate(() => window.__store?.getState().activeTabId ?? null), { + timeout: 10_000 + }) + .toBe(tabId) + await waitForActiveTerminalManager(page, 60_000) + await waitForActivePanePtyId(page, 60_000) +} + +export async function readRemoteTerminalTabs( + page: Page, + worktreeId: string +): Promise<{ id: string; ptyId: string | null }[]> { + return page.evaluate( + (id) => + (window.__store?.getState().tabsByWorktree[id] ?? []).map((tab) => ({ + id: tab.id, + ptyId: tab.ptyId + })), + worktreeId + ) +} diff --git a/tests/e2e/ssh-cold-activation-restore.spec.ts b/tests/e2e/ssh-cold-activation-restore.spec.ts index f49ad210c0b..844074c092f 100644 --- a/tests/e2e/ssh-cold-activation-restore.spec.ts +++ b/tests/e2e/ssh-cold-activation-restore.spec.ts @@ -1,4 +1,4 @@ -import type { ElectronApplication, Page } from '@stablyai/playwright-test' +import type { ElectronApplication } from '@stablyai/playwright-test' import { test, expect } from './helpers/orca-app' import { waitForActiveWorktree, waitForSessionReady } from './helpers/store' import { @@ -6,6 +6,10 @@ import { waitForActivePanePtyId, waitForActiveTerminalManager } from './helpers/terminal' +import { + createRemoteTerminalTab, + readRemoteTerminalTabs +} from './helpers/docker-ssh-relay-terminal-tabs' import { cleanupDockerSshRelayTarget, DOCKER_SSH_RELAY_REMOTE_REPO_PATH, @@ -21,40 +25,6 @@ const TAB_COUNT = 6 test.use({ seedTestRepo: false }) -async function createRemoteTerminalTab(page: Page, worktreeId: string): Promise { - const tabId = await page.evaluate((id) => { - const state = window.__store?.getState() - if (!state) { - throw new Error('Store unavailable') - } - const tab = state.createTab(id, undefined, undefined, { activate: true }) - state.setActiveTab(tab.id) - state.setActiveTabType('terminal') - return tab.id - }, worktreeId) - await expect - .poll(() => page.evaluate(() => window.__store?.getState().activeTabId ?? null), { - timeout: 10_000 - }) - .toBe(tabId) - await waitForActiveTerminalManager(page, 60_000) - await waitForActivePanePtyId(page, 60_000) -} - -async function readRemoteTerminalTabs( - page: Page, - worktreeId: string -): Promise<{ id: string; ptyId: string | null }[]> { - return page.evaluate( - (id) => - (window.__store?.getState().tabsByWorktree[id] ?? []).map((tab) => ({ - id: tab.id, - ptyId: tab.ptyId - })), - worktreeId - ) -} - function readRemoteProof(target: DockerSshRelayTarget, path: string): string | null { try { return execDockerSshRelayTargetCommand(target, `cat ${path}`) diff --git a/tests/e2e/ssh-reconnect-tab-destruction.spec.ts b/tests/e2e/ssh-reconnect-tab-destruction.spec.ts index 95d46a36187..c6e3a393347 100644 --- a/tests/e2e/ssh-reconnect-tab-destruction.spec.ts +++ b/tests/e2e/ssh-reconnect-tab-destruction.spec.ts @@ -85,12 +85,12 @@ test.describe('SSH reconnect tab destruction', () => { await openTerminalTabInActiveGroup(orcaPage) // Only that the tab exists in the store — no waiting for its manager or PTY. Every wait here // is time the debounced upload can use to land, which is what made this spec miss the bug. - const tabsBefore = await orcaPage.evaluate(() => { + const tabIdsBefore = await orcaPage.evaluate(() => { const state = window.__store?.getState() const worktreeId = state?.activeWorktreeId - return worktreeId ? (state?.tabsByWorktree?.[worktreeId]?.length ?? 0) : 0 + return worktreeId ? (state?.tabsByWorktree?.[worktreeId] ?? []).map((tab) => tab.id) : [] }) - expect(tabsBefore).toBeGreaterThanOrEqual(2) + expect(tabIdsBefore.length).toBeGreaterThanOrEqual(2) // Deliberately NOTHING between creating the tab and reconnecting. The destruction only fires // while the tab's creation is still unuploaded, so idling here — as waiting for a TUI to draw @@ -101,19 +101,26 @@ test.describe('SSH reconnect tab destruction', () => { // Checked BEFORE any paint assertion: survival and repaint are different failures, and this // order names which one broke instead of collapsing both into "no output". - const tabCounts = await orcaPage.evaluate(() => { + const tabState = await orcaPage.evaluate(() => { const state = window.__store?.getState() const worktreeId = state?.activeWorktreeId return { - inSlice: worktreeId ? (state?.tabsByWorktree?.[worktreeId]?.length ?? 0) : 0, + tabIds: worktreeId + ? (state?.tabsByWorktree?.[worktreeId] ?? []).map((tab) => tab.id) + : [], // __paneManagers is a Map. Object.keys on a Map silently returns [], which reads as // "nothing is mounted" regardless of the truth — that cost a full debugging cycle. paneManagers: window.__paneManagers?.size ?? 0 } }) - expect(tabCounts.inSlice, 'the reconnect destroyed the tab').toBeGreaterThanOrEqual(2) + // The exact set, not a lower bound: `>= 2` passes just as happily on a reconnect that ADDS a + // tab as on one that keeps it, so it could never fail on the accumulation half of this bug. expect( - tabCounts.paneManagers, + tabState.tabIds.slice().sort(), + 'the reconnect changed the tab set: it destroyed a tab or spuriously added one' + ).toEqual(tabIdsBefore.slice().sort()) + expect( + tabState.paneManagers, 'the tab survived but its pane manager did not' ).toBeGreaterThanOrEqual(1) diff --git a/tests/e2e/ssh-restart-tab-accumulation.spec.ts b/tests/e2e/ssh-restart-tab-accumulation.spec.ts new file mode 100644 index 00000000000..cd0a314bfbd --- /dev/null +++ b/tests/e2e/ssh-restart-tab-accumulation.spec.ts @@ -0,0 +1,257 @@ +import type { ElectronApplication, Page, TestInfo } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { waitForActiveWorktree, waitForSessionReady } from './helpers/store' +import { waitForActivePanePtyId, waitForActiveTerminalManager } from './helpers/terminal' +import { createRemoteTerminalTab } from './helpers/docker-ssh-relay-terminal-tabs' +import { + cleanupDockerSshRelayTarget, + startDockerSshRelayTarget, + type DockerSshRelayTarget +} from './helpers/docker-ssh-relay-target' +import { connectDockerSshRelayTarget } from './helpers/docker-ssh-relay-connection' +import { createRestartSession } from './helpers/orca-restart' + +const RUN_DOCKER_SSH = process.env.ORCA_E2E_SSH_DOCKER === '1' +const RESTART_CYCLES = 3 +/** Consecutive agreeing samples that count as "the strip stopped changing". */ +const SETTLED_SAMPLES = 3 + +test.use({ seedTestRepo: false }) + +type WorkspaceTabSnapshot = { + /** Terminal tabs the store owns for the remote worktree. */ + worktreeTabIds: string[] + /** Tabs the strip actually renders — what the user counts. */ + stripTabIds: string[] + /** Why: a restart that re-adds the worktree under a new id would grow the window without growing the row above. */ + totalTabCount: number + remoteWorktreeCount: number +} + +async function readWorkspaceTabSnapshot( + page: Page, + worktreeId: string, + repoId: string +): Promise { + const store = await page.evaluate( + ({ worktreeId, repoId }) => { + const state = window.__store?.getState() + if (!state) { + throw new Error('Store unavailable') + } + return { + worktreeTabIds: (state.tabsByWorktree[worktreeId] ?? []).map((tab) => tab.id), + totalTabCount: Object.values(state.tabsByWorktree).reduce( + (total, tabs) => total + tabs.length, + 0 + ), + remoteWorktreeCount: (state.worktreesByRepo[repoId] ?? []).length + } + }, + { worktreeId, repoId } + ) + const stripTabIds = await page + .locator('.terminal-tab-strip [data-tab-id]') + .evaluateAll((elements) => + elements.map((element) => (element as HTMLElement).dataset.tabId ?? '') + ) + return { ...store, stripTabIds } +} + +/** + * Sample the workspace until its tabs stop changing. + * + * Why: restore is not one event — the session rehydrates, then the relay reconnects, then worktree + * activation runs. A tab spawned by the last of those is invisible to a snapshot taken after the + * first, so sampling too early would let an accumulation bug pass. + */ +async function waitForSettledWorkspaceTabs( + page: Page, + worktreeId: string, + repoId: string +): Promise { + let latest: WorkspaceTabSnapshot = { + worktreeTabIds: [], + stripTabIds: [], + totalTabCount: 0, + remoteWorktreeCount: 0 + } + let previousKey = '' + let agreements = 0 + await expect + .poll( + async () => { + latest = await readWorkspaceTabSnapshot(page, worktreeId, repoId) + const key = JSON.stringify(latest) + agreements = key === previousKey ? agreements + 1 : 0 + previousKey = key + return agreements + }, + { + timeout: 60_000, + intervals: [1_000], + message: 'the remote workspace tab set never stopped changing' + } + ) + .toBeGreaterThanOrEqual(SETTLED_SAMPLES) + return latest +} + +async function waitForRestoredRemoteWorktree( + page: Page, + targetId: string, + worktreeId: string +): Promise { + await waitForSessionReady(page, 60_000) + await expect.poll(() => waitForActiveWorktree(page), { timeout: 60_000 }).toBe(worktreeId) + await expect + .poll( + () => + page.evaluate( + (id) => window.__store?.getState().sshConnectionStates.get(id)?.status, + targetId + ), + { timeout: 90_000, message: 'renderer SSH state did not restore' } + ) + .toBe('connected') + await waitForActiveTerminalManager(page, 60_000) + await waitForActivePanePtyId(page, 60_000) +} + +/** Quit through the same beforeunload flush a real window close performs, then prove it landed. */ +async function flushSessionBeforeQuit( + page: Page, + targetId: string, + worktreeId: string, + tabIds: string[] +): Promise { + await page.evaluate(() => window.dispatchEvent(new Event('beforeunload'))) + await expect + .poll( + () => + page.evaluate( + async ({ targetId, worktreeId, tabIds }) => { + const persisted = await window.api.session.get() + const persistedIds = new Set( + (persisted.tabsByWorktree[worktreeId] ?? []).map((tab) => tab.id) + ) + return ( + persisted.activeConnectionIdsAtShutdown?.includes(targetId) === true && + tabIds.every((tabId) => persistedIds.has(tabId)) + ) + }, + { targetId, worktreeId, tabIds } + ), + { timeout: 15_000, message: 'SSH tabs and active target were not persisted before quit' } + ) + .toBe(true) +} + +function describeGrowth(baseline: WorkspaceTabSnapshot, cycles: WorkspaceTabSnapshot[]): string { + const baselineTabIds = new Set(baseline.worktreeTabIds) + const perCycle = cycles.map((cycle, index) => { + const added = cycle.worktreeTabIds.filter((tabId) => !baselineTabIds.has(tabId)) + const suffix = added.length > 0 ? ` (+${added.length}: ${added.join(', ')})` : '' + return `restart${index + 1}=${cycle.worktreeTabIds.length}${suffix}/strip${cycle.stripTabIds.length}/all${cycle.totalTabCount}/worktrees${cycle.remoteWorktreeCount}` + }) + return `baseline=${baseline.worktreeTabIds.length}/strip${baseline.stripTabIds.length}/all${baseline.totalTabCount}/worktrees${baseline.remoteWorktreeCount} ${perCycle.join(' ')}` +} + +async function runRestartCycles(testInfo: TestInfo, initialTabCount: number): Promise { + const restart = createRestartSession(testInfo) + let target: DockerSshRelayTarget | null = null + let app: ElectronApplication | null = null + try { + target = startDockerSshRelayTarget(testInfo) + const firstLaunch = await restart.launch() + app = firstLaunch.app + let page = firstLaunch.page + await waitForSessionReady(page) + const remote = await connectDockerSshRelayTarget(page, target) + await expect + .poll(() => waitForActiveWorktree(page), { timeout: 30_000 }) + .toBe(remote.worktreeId) + await waitForActiveTerminalManager(page, 60_000) + await waitForActivePanePtyId(page, 60_000) + + while ( + (await readWorkspaceTabSnapshot(page, remote.worktreeId, remote.repoId)).worktreeTabIds + .length < initialTabCount + ) { + await createRemoteTerminalTab(page, remote.worktreeId) + } + const baseline = await waitForSettledWorkspaceTabs(page, remote.worktreeId, remote.repoId) + expect(baseline.worktreeTabIds).toHaveLength(initialTabCount) + expect(baseline.stripTabIds.slice().sort()).toEqual(baseline.worktreeTabIds.slice().sort()) + + // Why: every cycle runs before anything is asserted, so a failure reports whether the strip + // grows by one per restart or duplicates wholesale — those have different causes. + const cycles: WorkspaceTabSnapshot[] = [] + for (let cycle = 0; cycle < RESTART_CYCLES; cycle += 1) { + await flushSessionBeforeQuit( + page, + remote.targetId, + remote.worktreeId, + baseline.worktreeTabIds + ) + await restart.close(app) + app = null + const relaunch = await restart.launch() + app = relaunch.app + page = relaunch.page + await waitForRestoredRemoteWorktree(page, remote.targetId, remote.worktreeId) + cycles.push(await waitForSettledWorkspaceTabs(page, remote.worktreeId, remote.repoId)) + } + + const growth = describeGrowth(baseline, cycles) + expect( + cycles.map((cycle) => cycle.stripTabIds.length), + `tab strip grew across restarts: ${growth}` + ).toEqual(Array.from({ length: RESTART_CYCLES }, () => initialTabCount)) + expect( + cycles.map((cycle) => cycle.totalTabCount), + `total tab count grew across restarts: ${growth}` + ).toEqual(cycles.map(() => baseline.totalTabCount)) + expect( + cycles.map((cycle) => cycle.remoteWorktreeCount), + `the remote repo gained worktree rows across restarts: ${growth}` + ).toEqual(cycles.map(() => baseline.remoteWorktreeCount)) + const expectedTabIds = baseline.worktreeTabIds.slice().sort() + for (const [index, cycle] of cycles.entries()) { + expect(cycle.worktreeTabIds.slice().sort(), `restart ${index + 1}: ${growth}`).toEqual( + expectedTabIds + ) + expect( + cycle.stripTabIds.slice().sort(), + `restart ${index + 1} rendered different tabs: ${growth}` + ).toEqual(expectedTabIds) + } + } finally { + if (app) { + await restart.close(app) + } + await restart.dispose() + cleanupDockerSshRelayTarget(target) + } +} + +test.describe('SSH restart tab accumulation', () => { + test.skip(!RUN_DOCKER_SSH, 'Set ORCA_E2E_SSH_DOCKER=1 to run Docker-backed SSH tests.') + test.skip(process.platform === 'win32', 'Docker SSH restore uses POSIX SSH tooling.') + + // Why three restarts and not one: a single restart only proves one restore was clean. Users reach + // this by returning to the same SSH workspace day after day, so the invariant worth pinning is + // that N restarts add nothing — one spurious tab per launch is invisible to a one-shot test. + test('keeps a single restored SSH tab across repeated quit and relaunch cycles', async (// oxlint-disable-next-line no-empty-pattern -- This restart test owns every Electron launch. + {}, testInfo) => { + test.setTimeout(600_000) + await runRestartCycles(testInfo, 1) + }) + + // Why a second size: separates "one spurious default tab per restart" from "the set duplicates". + test('keeps every restored SSH tab across repeated quit and relaunch cycles', async (// oxlint-disable-next-line no-empty-pattern -- This restart test owns every Electron launch. + {}, testInfo) => { + test.setTimeout(600_000) + await runRestartCycles(testInfo, 3) + }) +})