From 56b58ec443b46fe8f4c639a48bb2b2a9237b034b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andi=20Kr=C3=BCger?= <15608729+andikrueger@users.noreply.github.com> Date: Tue, 31 Mar 2026 11:03:09 +0200 Subject: [PATCH 1/4] Fix issue #711: squad start --tunnel validates node-pty before side effects Problem: 'squad start --tunnel' printed tunnel URL and created resources before checking if node-pty was available, leading to confusing failures. Solution: 1. Added node-pty (^1.1.0) and qrcode-terminal (^0.12.0) as optionalDependencies in packages/squad-cli/package.json 2. Moved node-pty import check to the START of runStart() function, before getGitInfo(), RemoteBridge instantiation, bridge.start(), or createTunnel() 3. Added regression test that verifies import order by parsing source This ensures missing node-pty is caught immediately without printing success messages or allocating resources. Test coverage: - test/cli/start.test.ts: Verifies node-pty import precedes side effects - test/cli-packaging-smoke.test.ts: Existing 'start' routability test Closes #711 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- packages/squad-cli/package.json | 4 ++ packages/squad-cli/src/cli/commands/start.ts | 18 +++++-- test/cli/start.test.ts | 52 ++++++++++++++++++++ 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/packages/squad-cli/package.json b/packages/squad-cli/package.json index 239ce8395..97d46586e 100644 --- a/packages/squad-cli/package.json +++ b/packages/squad-cli/package.json @@ -182,6 +182,10 @@ "react": "^19.2.4", "vscode-jsonrpc": "^8.2.1" }, + "optionalDependencies": { + "node-pty": "^1.1.0", + "qrcode-terminal": "^0.12.0" + }, "devDependencies": { "@types/node": "^22.0.0", "@types/react": "^19.2.14", diff --git a/packages/squad-cli/src/cli/commands/start.ts b/packages/squad-cli/src/cli/commands/start.ts index 4c85cf835..59f679d03 100644 --- a/packages/squad-cli/src/cli/commands/start.ts +++ b/packages/squad-cli/src/cli/commands/start.ts @@ -39,6 +39,18 @@ export interface StartOptions { } export async function runStart(cwd: string, options: StartOptions): Promise { + // ─── Verify node-pty availability FIRST (before any side effects) ─── + let nodePty: any; + try { + // @ts-ignore — node-pty is an optional native dependency + nodePty = await import('node-pty'); + } catch (err) { + console.error(`${YELLOW}✗${RESET} node-pty not available. Install it for PTY support:`); + console.error(` ${DIM}npm install -g node-pty${RESET}`); + console.error(`\n${DIM}Error: ${(err as Error).message}${RESET}`); + process.exit(1); + } + const { repo, branch } = getGitInfo(cwd); const machine = getMachineId(); const squadDir = storage.existsSync(path.join(cwd, '.squad')) @@ -47,7 +59,7 @@ export async function runStart(cwd: string, options: StartOptions): Promise { expect(mod.default).toBeUndefined(); }); }); + +/** + * Issue #711: Verify node-pty is checked BEFORE bridge/tunnel creation + * + * Regression test: if node-pty import fails, the command must exit + * immediately without creating RemoteBridge or tunnel side effects. + */ +describe('CLI: start command - node-pty requirement (issue #711)', () => { + it('verifies node-pty import appears before RemoteBridge construction in source', async () => { + // Read the source file directly to verify implementation order + const fs = await import('node:fs'); + const path = await import('node:path'); + + const startTsPath = path.resolve(process.cwd(), 'packages/squad-cli/src/cli/commands/start.ts'); + const source = fs.readFileSync(startTsPath, 'utf-8'); + + // Find the start of runStart function + const functionStart = source.indexOf('export async function runStart'); + expect(functionStart).toBeGreaterThan(-1); + + // Find node-pty import position (relative to function start) + const nodePtyImportPattern = /await import\(['"]node-pty['"]\)/; + const nodePtyMatch = source.slice(functionStart).match(nodePtyImportPattern); + expect(nodePtyMatch).toBeTruthy(); + const nodePtyPos = functionStart + (nodePtyMatch?.index || 0); + + // Find RemoteBridge construction + const bridgePattern = /new RemoteBridge\(/; + const bridgeMatch = source.slice(functionStart).match(bridgePattern); + if (bridgeMatch) { + const bridgePos = functionStart + bridgeMatch.index; + // node-pty import MUST come before RemoteBridge construction + expect(nodePtyPos).toBeLessThan(bridgePos); + } + + // Find bridge.start() call + const bridgeStartPattern = /bridge\.start\(\)/; + const bridgeStartMatch = source.slice(functionStart).match(bridgeStartPattern); + if (bridgeStartMatch) { + const bridgeStartPos = functionStart + bridgeStartMatch.index; + expect(nodePtyPos).toBeLessThan(bridgeStartPos); + } + + // Find createTunnel call + const createTunnelPattern = /createTunnel\(/; + const createTunnelMatch = source.slice(functionStart).match(createTunnelPattern); + if (createTunnelMatch) { + const createTunnelPos = functionStart + createTunnelMatch.index; + expect(nodePtyPos).toBeLessThan(createTunnelPos); + } + }); +}); From 896599dcd51518f0e21b5dd0a755900c5b45450b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andi=20Kr=C3=BCger?= <15608729+andikrueger@users.noreply.github.com> Date: Tue, 31 Mar 2026 11:06:27 +0200 Subject: [PATCH 2/4] test(cli): require graceful node-pty error in smoke test (issue #711) Update cli-packaging-smoke.test.ts to verify that the 'start' command shows a graceful error message when node-pty is missing, rather than excusing any MODULE_NOT_FOUND error. This ensures the fix in start.ts (checking node-pty before creating RemoteBridge or tunnel) works correctly in packaged installs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/cli-packaging-smoke.test.ts | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/test/cli-packaging-smoke.test.ts b/test/cli-packaging-smoke.test.ts index 127aba870..0efb43bd0 100644 --- a/test/cli-packaging-smoke.test.ts +++ b/test/cli-packaging-smoke.test.ts @@ -163,9 +163,10 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { /** * Helper to verify a command was routed (not unknown, not module error). - * Some commands may have optional dependencies (e.g., node-pty for 'start') - * that aren't packaged. We still consider them routed if the error is about - * a missing optional dependency, not the command itself being unknown. + * + * Issue #711: The `start` command requires node-pty (optionalDependency). + * If node-pty is missing, the command should show a graceful error message + * ("node-pty not available"), NOT crash with raw MODULE_NOT_FOUND. */ function expectCommandRouted(result: { stdout: string; stderr: string; timedOut?: boolean }, command?: string) { // If the command timed out, it was routed — it started executing @@ -175,15 +176,16 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { const output = result.stdout + result.stderr; expect(output.toLowerCase()).not.toContain('unknown command'); - // Allow MODULE_NOT_FOUND if it's for an optional dependency (node-pty), - // not for the command module itself + // Special case for 'start' command: if node-pty is missing, we should see + // the graceful error message, NOT a raw MODULE_NOT_FOUND crash + if (command === 'start' && output.includes('node-pty')) { + // Should have graceful error, not raw module error + expect(output).toMatch(/node-pty not available/i); + return; + } + + // For all other commands, MODULE_NOT_FOUND is a test failure if (output.match(/MODULE_NOT_FOUND|Cannot find module/i)) { - // If it's node-pty, that's OK — it's an optional dep for the start command - if (output.includes('node-pty')) { - // This is expected — node-pty is an optional dependency - return; - } - // Otherwise fail — this is a real module error expect(output).not.toMatch(/MODULE_NOT_FOUND|Cannot find module/i); } } @@ -258,7 +260,7 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { for (const cmd of COMMANDS) { it(`command "${cmd}" is routable`, () => { const result = runCommand([cmd]); - expectCommandRouted(result); + expectCommandRouted(result, cmd); }); } From e59bcb21c6e51537b7defe18491a88786c97f781 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andi=20Kr=C3=BCger?= <15608729+andikrueger@users.noreply.github.com> Date: Tue, 31 Mar 2026 13:17:59 +0200 Subject: [PATCH 3/4] docs(changelog): add entry for issue #711 node-pty validation fix Closes #711 (changelog gate requirement) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4651c121..ace9f5a56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,11 @@ All notable changes to this project will be documented in this file. - **ESLint with TypeScript support** (#493) — static analysis for common runtime anti-patterns - **Integration** — available in CLI pre-flight checks +### Fixed — CLI node-pty Validation +- **squad start --tunnel** (#711) — validates node-pty availability before creating RemoteBridge or tunnel, preventing terminal corruption when package is missing +- Graceful error message guides users to install node-pty when required +- Includes smoke test coverage for node-pty failure scenarios + ### Fixed — CLI Terminal Rendering - Eliminated scroll-to-top flicker caused by Ink's fullscreen `clearTerminal` path firing on every render cycle - Reduced re-render churn via memoized elapsed-time display (one-second granularity gate) and consolidated animation intervals From 8b9a5ea1d6833348390fa301ca9b4e4469f580c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andi=20Kr=C3=BCger?= <15608729+andikrueger@users.noreply.github.com> Date: Tue, 31 Mar 2026 15:24:02 +0200 Subject: [PATCH 4/4] fix: address PR 715 review feedback - extract checkNodePty helper for start - sync workspace optional dependencies in lockfile - make packaging smoke tests detect ESM missing modules - add deterministic forced-missing node-pty coverage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- package-lock.json | 31 +++ packages/squad-cli/src/cli/commands/start.ts | 27 ++- test/cli-packaging-smoke.test.ts | 196 ++++++++++++------- test/cli/start.test.ts | 115 +++++++++-- 4 files changed, 282 insertions(+), 87 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3f358a711..cd4ea54a5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7240,6 +7240,24 @@ "dev": true, "license": "MIT" }, + "node_modules/node-addon-api": { + "version": "7.1.1", + "resolved": "https://registry.npmjs.org/node-addon-api/-/node-addon-api-7.1.1.tgz", + "integrity": "sha512-5m3bsyrjFWE1xf7nz7YXdN4udnVtXK6/Yfgn5qnahL6bCkf2yKt4k3nuTKAtT4r3IG8JNR2ncsIMdZuAzJjHQQ==", + "license": "MIT", + "optional": true + }, + "node_modules/node-pty": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/node-pty/-/node-pty-1.1.0.tgz", + "integrity": "sha512-20JqtutY6JPXTUnL0ij1uad7Qe1baT46lyolh2sSENDd4sTzKZ4nmAFkeAARDKwmlLjPx6XKRlwRUxwjOy+lUg==", + "hasInstallScript": true, + "license": "MIT", + "optional": true, + "dependencies": { + "node-addon-api": "^7.1.0" + } + }, "node_modules/onetime": { "version": "5.1.2", "resolved": "https://registry.npmjs.org/onetime/-/onetime-5.1.2.tgz", @@ -7551,6 +7569,15 @@ "node": ">=6" } }, + "node_modules/qrcode-terminal": { + "version": "0.12.0", + "resolved": "https://registry.npmjs.org/qrcode-terminal/-/qrcode-terminal-0.12.0.tgz", + "integrity": "sha512-EXtzRZmC+YGmGlDFbXKxQiMZNwCLEO6BANKXG4iCtSIM0yqc/pappSx3RIKr4r0uh5JsBckOXeKrB3Iz7mdQpQ==", + "optional": true, + "bin": { + "qrcode-terminal": "bin/qrcode-terminal.js" + } + }, "node_modules/queue-microtask": { "version": "1.2.3", "resolved": "https://registry.npmjs.org/queue-microtask/-/queue-microtask-1.2.3.tgz", @@ -8923,6 +8950,10 @@ }, "engines": { "node": ">=22.5.0" + }, + "optionalDependencies": { + "node-pty": "^1.1.0", + "qrcode-terminal": "^0.12.0" } }, "packages/squad-cli/node_modules/@esbuild/aix-ppc64": { diff --git a/packages/squad-cli/src/cli/commands/start.ts b/packages/squad-cli/src/cli/commands/start.ts index 59f679d03..3eca47d29 100644 --- a/packages/squad-cli/src/cli/commands/start.ts +++ b/packages/squad-cli/src/cli/commands/start.ts @@ -30,6 +30,8 @@ const RESET = '\x1b[0m'; const DIM = '\x1b[2m'; const GREEN = '\x1b[32m'; const YELLOW = '\x1b[33m'; +const MISSING_MODULE_RE = + /\b(?:MODULE_NOT_FOUND|ERR_MODULE_NOT_FOUND)\b|Cannot find module|Cannot find package/i; export interface StartOptions { tunnel: boolean; @@ -38,16 +40,33 @@ export interface StartOptions { command?: string; } +async function checkNodePty(): Promise { + try { + // @ts-ignore — node-pty is an optional native dependency + return await import('node-pty'); + } catch (err) { + const detail = err instanceof Error ? err.message : String(err); + if (MISSING_MODULE_RE.test(detail)) { + throw new Error('node-pty not available. Install it for PTY support:'); + } + throw new Error('node-pty not available. Install it for PTY support:', { + cause: detail, + }); + } +} + export async function runStart(cwd: string, options: StartOptions): Promise { // ─── Verify node-pty availability FIRST (before any side effects) ─── let nodePty: any; try { - // @ts-ignore — node-pty is an optional native dependency - nodePty = await import('node-pty'); + nodePty = await checkNodePty(); } catch (err) { - console.error(`${YELLOW}✗${RESET} node-pty not available. Install it for PTY support:`); + const detail = err instanceof Error && typeof err.cause === 'string' ? err.cause : undefined; + console.error(`${YELLOW}✗${RESET} ${(err as Error).message}`); console.error(` ${DIM}npm install -g node-pty${RESET}`); - console.error(`\n${DIM}Error: ${(err as Error).message}${RESET}`); + if (detail) { + console.error(`\n${DIM}Error: ${detail}${RESET}`); + } process.exit(1); } diff --git a/test/cli-packaging-smoke.test.ts b/test/cli-packaging-smoke.test.ts index 0efb43bd0..6dedbdc3e 100644 --- a/test/cli-packaging-smoke.test.ts +++ b/test/cli-packaging-smoke.test.ts @@ -11,16 +11,36 @@ */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import { mkdtempSync, rmSync, existsSync } from 'node:fs'; +import { mkdtempSync, rmSync, existsSync, readFileSync, renameSync } from 'node:fs'; import { join } from 'node:path'; -import { execSync } from 'node:child_process'; +import { execSync, execFileSync } from 'node:child_process'; import { tmpdir } from 'node:os'; +const NO_COLOR_ENV = { + ...process.env, + NO_COLOR: '1', + FORCE_COLOR: '0', + npm_config_loglevel: process.env['npm_config_loglevel'] ?? 'warn', +}; +const MISSING_MODULE_RE = /MODULE_NOT_FOUND|ERR_MODULE_NOT_FOUND|Cannot find module|Cannot find package/i; + +interface CommandResult { + stdout: string; + stderr: string; + exitCode: number; + timedOut?: boolean; +} + +interface InstalledCli { + tempDir: string; + cliEntryPath: string; +} + describe('CLI packaging smoke test', { timeout: 120_000 }, () => { - let tempDir: string; + let packageArtifactsDir: string | undefined; + let installedCli: InstalledCli | undefined; let sdkTarball: string; let cliTarball: string; - let cliEntryPath: string; beforeAll(() => { const cwd = process.cwd(); @@ -45,53 +65,56 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { execSync('npm run build', { cwd: cliDir, stdio: 'inherit', env: buildEnv }); } - // Pack both packages - console.log('Packing squad-sdk...'); - const sdkPackOutput = execSync('npm pack --quiet', { + packageArtifactsDir = mkdtempSync(join(tmpdir(), 'squad-cli-pack-')); + + // Pack both packages into an isolated temp directory so reruns do not + // reuse or mutate repo-local tarballs between installs. + const sdkPackOutput = execSync(`npm pack --quiet --pack-destination "${packageArtifactsDir}"`, { cwd: sdkDir, encoding: 'utf8', - env: { ...process.env, NO_COLOR: '1', FORCE_COLOR: '0' }, + env: NO_COLOR_ENV, }).trim(); - sdkTarball = join(sdkDir, sdkPackOutput.split('\n').pop()!.trim()); + sdkTarball = join(packageArtifactsDir, sdkPackOutput.split('\n').pop()!.trim()); - console.log('Packing squad-cli...'); - const cliPackOutput = execSync('npm pack --quiet', { + const cliPackOutput = execSync(`npm pack --quiet --pack-destination "${packageArtifactsDir}"`, { cwd: cliDir, encoding: 'utf8', - env: { ...process.env, NO_COLOR: '1', FORCE_COLOR: '0' }, + env: NO_COLOR_ENV, }).trim(); - cliTarball = join(cliDir, cliPackOutput.split('\n').pop()!.trim()); + cliTarball = join(packageArtifactsDir, cliPackOutput.split('\n').pop()!.trim()); - // Create temp directory and install - tempDir = mkdtempSync(join(tmpdir(), 'squad-cli-test-')); - console.log(`Installing packages in ${tempDir}...`); + const installPackedCli = (prefix: string): InstalledCli => { + const tempDir = mkdtempSync(join(tmpdir(), prefix)); - // Initialize package.json - execSync('npm init -y', { - cwd: tempDir, - stdio: 'ignore', - env: { ...process.env, NO_COLOR: '1', FORCE_COLOR: '0' }, - }); + execSync('npm init -y', { + cwd: tempDir, + stdio: 'ignore', + env: NO_COLOR_ENV, + }); - // Install both tarballs - execSync(`npm install "${sdkTarball}" "${cliTarball}"`, { - cwd: tempDir, - stdio: 'inherit', - env: { ...process.env, NO_COLOR: '1', FORCE_COLOR: '0' }, - }); + execSync(`npm install "${sdkTarball}" "${cliTarball}"`, { + cwd: tempDir, + stdio: 'pipe', + env: NO_COLOR_ENV, + }); - cliEntryPath = join( - tempDir, - 'node_modules', - '@bradygaster', - 'squad-cli', - 'dist', - 'cli-entry.js', - ); - - if (!existsSync(cliEntryPath)) { - throw new Error(`CLI entry point not found at ${cliEntryPath}`); - } + const cliEntryPath = join( + tempDir, + 'node_modules', + '@bradygaster', + 'squad-cli', + 'dist', + 'cli-entry.js', + ); + + if (!existsSync(cliEntryPath)) { + throw new Error(`CLI entry point not found at ${cliEntryPath}`); + } + + return { tempDir, cliEntryPath }; + }; + + installedCli = installPackedCli('squad-cli-test-'); }, 90000); afterAll(() => { @@ -118,9 +141,8 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { } }; - cleanupWithRetry(tempDir); - cleanupWithRetry(sdkTarball); - cleanupWithRetry(cliTarball); + cleanupWithRetry(installedCli?.tempDir ?? ''); + cleanupWithRetry(packageArtifactsDir ?? ''); }); /** @@ -133,13 +155,17 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { * and `start` hang waiting for infrastructure; a timeout means they were * routed successfully. */ - function runCommand(args: string[]): { stdout: string; stderr: string; exitCode: number; timedOut?: boolean } { + function runCommand(args: string[], cli = installedCli): CommandResult { + if (!cli) { + throw new Error('CLI package is not installed for this test'); + } + try { - const stdout = execSync(`node "${cliEntryPath}" ${args.join(' ')}`, { - cwd: tempDir, + const stdout = execFileSync(process.execPath, [cli.cliEntryPath, ...args], { + cwd: cli.tempDir, encoding: 'utf8', timeout: 2000, - env: { ...process.env, NO_COLOR: '1', FORCE_COLOR: '0' }, + env: NO_COLOR_ENV, }); return { stdout, stderr: '', exitCode: 0 }; } catch (err: any) { @@ -163,31 +189,28 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { /** * Helper to verify a command was routed (not unknown, not module error). - * - * Issue #711: The `start` command requires node-pty (optionalDependency). - * If node-pty is missing, the command should show a graceful error message - * ("node-pty not available"), NOT crash with raw MODULE_NOT_FOUND. */ - function expectCommandRouted(result: { stdout: string; stderr: string; timedOut?: boolean }, command?: string) { + function expectCommandRouted(result: { stdout: string; stderr: string; timedOut?: boolean }) { // If the command timed out, it was routed — it started executing // but hung waiting for infrastructure (e.g., rc, start, aspire) if (result.timedOut) return; const output = result.stdout + result.stderr; expect(output.toLowerCase()).not.toContain('unknown command'); - - // Special case for 'start' command: if node-pty is missing, we should see - // the graceful error message, NOT a raw MODULE_NOT_FOUND crash - if (command === 'start' && output.includes('node-pty')) { - // Should have graceful error, not raw module error - expect(output).toMatch(/node-pty not available/i); - return; - } - - // For all other commands, MODULE_NOT_FOUND is a test failure - if (output.match(/MODULE_NOT_FOUND|Cannot find module/i)) { - expect(output).not.toMatch(/MODULE_NOT_FOUND|Cannot find module/i); - } + expect(output).not.toMatch(MISSING_MODULE_RE); + } + + function isDependencyInstalled(cli: InstalledCli | undefined, dependency: string): boolean { + return Boolean(findDependencyPath(cli, dependency)); + } + + function findDependencyPath(cli: InstalledCli | undefined, dependency: string): string | undefined { + if (!cli) return undefined; + + return [ + join(cli.tempDir, 'node_modules', dependency), + join(cli.tempDir, 'node_modules', '@bradygaster', 'squad-cli', 'node_modules', dependency), + ].find(path => existsSync(path)); } // ============================================================================ @@ -196,8 +219,9 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { // ============================================================================ it('squad-cli has no file: dependencies (breaks global installs)', () => { - const pkgPath = join(tempDir, 'node_modules', '@bradygaster', 'squad-cli', 'package.json'); - const pkg = JSON.parse(require('fs').readFileSync(pkgPath, 'utf8')); + expect(installedCli).toBeDefined(); + const pkgPath = join(installedCli!.tempDir, 'node_modules', '@bradygaster', 'squad-cli', 'package.json'); + const pkg = JSON.parse(readFileSync(pkgPath, 'utf8')); const deps = pkg.dependencies || {}; for (const [name, version] of Object.entries(deps)) { expect(String(version), `${name} has file: dependency — will break global installs`) @@ -206,9 +230,10 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { }); it('squad-sdk resolves as a real package (not a workspace link)', () => { - const sdkPkg = join(tempDir, 'node_modules', '@bradygaster', 'squad-sdk', 'package.json'); + expect(installedCli).toBeDefined(); + const sdkPkg = join(installedCli!.tempDir, 'node_modules', '@bradygaster', 'squad-sdk', 'package.json'); expect(existsSync(sdkPkg), 'squad-sdk not installed as dependency of squad-cli').toBe(true); - const pkg = JSON.parse(require('fs').readFileSync(sdkPkg, 'utf8')); + const pkg = JSON.parse(readFileSync(sdkPkg, 'utf8')); expect(pkg.name).toBe('@bradygaster/squad-sdk'); }); @@ -260,10 +285,41 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { for (const cmd of COMMANDS) { it(`command "${cmd}" is routable`, () => { const result = runCommand([cmd]); - expectCommandRouted(result, cmd); + expectCommandRouted(result); }); } + it('start command gracefully handles forced-missing node-pty', () => { + expect(installedCli).toBeDefined(); + + const nodePtyPath = findDependencyPath(installedCli, 'node-pty'); + if (!nodePtyPath) { + const result = runCommand(['start']); + expect(result.timedOut).not.toBe(true); + + const output = result.stdout + result.stderr; + expect(output).toMatch(/node-pty not available/i); + expect(output).not.toMatch(MISSING_MODULE_RE); + return; + } + + const hiddenNodePtyPath = `${nodePtyPath}-forced-missing`; + renameSync(nodePtyPath, hiddenNodePtyPath); + + try { + expect(isDependencyInstalled(installedCli, 'node-pty')).toBe(false); + + const result = runCommand(['start']); + expect(result.timedOut).not.toBe(true); + + const output = result.stdout + result.stderr; + expect(output).toMatch(/node-pty not available/i); + expect(output).not.toMatch(MISSING_MODULE_RE); + } finally { + renameSync(hiddenNodePtyPath, nodePtyPath); + } + }); + // Aliases it('alias "watch" routes same as "triage"', () => { const watchResult = runCommand(['watch']); diff --git a/test/cli/start.test.ts b/test/cli/start.test.ts index c58ea9c14..635ab0c64 100644 --- a/test/cli/start.test.ts +++ b/test/cli/start.test.ts @@ -5,7 +5,12 @@ * Does NOT spawn PTY or create tunnels (requires native deps + network). */ -import { describe, it, expect } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +afterEach(() => { + vi.restoreAllMocks(); + vi.resetModules(); +}); describe('CLI: start command', () => { it('module exports runStart function', async () => { @@ -33,31 +38,115 @@ describe('CLI: start command', () => { * immediately without creating RemoteBridge or tunnel side effects. */ describe('CLI: start command - node-pty requirement (issue #711)', () => { - it('verifies node-pty import appears before RemoteBridge construction in source', async () => { + it('exits before bridge or tunnel setup when node-pty is missing', async () => { + const exitSignal = new Error('process.exit'); + const remoteBridgeCtor = vi.fn(); + const remoteBridgeStart = vi.fn(async () => 0); + const createTunnel = vi.fn(); + const destroyTunnel = vi.fn(); + const getGitInfo = vi.fn(() => ({ repo: 'owner/repo', branch: 'test-branch' })); + const getMachineId = vi.fn(() => 'machine-id'); + const isDevtunnelAvailable = vi.fn(() => true); + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + const exitMock = vi.spyOn(process, 'exit').mockImplementation((( + code?: string | number | null, + ) => { + (exitSignal as Error & { exitCode?: string | number | null }).exitCode = code; + throw exitSignal; + }) as never); + + vi.doMock('@bradygaster/squad-sdk', () => { + class FSStorageProvider { + existsSync(): boolean { + return false; + } + + statSync(): undefined { + return undefined; + } + + appendSync(): void {} + } + + class RemoteBridge { + constructor(_config: unknown) { + remoteBridgeCtor(_config); + } + + setStaticHandler = vi.fn(); + start = remoteBridgeStart; + getSessionToken = vi.fn(() => 'session-token'); + getAuditLogPath = vi.fn(() => 'audit.log'); + getSessionExpiry = vi.fn(() => Date.now() + 60_000); + setPassthrough = vi.fn(); + passthroughFromAgent = vi.fn(); + stop = vi.fn(); + } + + return { FSStorageProvider, RemoteBridge }; + }); + + vi.doMock('../../packages/squad-cli/src/cli/commands/rc-tunnel.js', () => ({ + isDevtunnelAvailable, + createTunnel, + destroyTunnel, + getMachineId, + getGitInfo, + })); + + vi.doMock('node-pty', () => { + throw new Error('Cannot find package "node-pty" imported from start.ts'); + }); + + const { runStart } = await import('../../packages/squad-cli/src/cli/commands/start.ts'); + + await expect(runStart(process.cwd(), { tunnel: true, port: 0 })).rejects.toBe(exitSignal); + + expect(exitMock).toHaveBeenCalledWith(1); + expect((exitSignal as Error & { exitCode?: string | number | null }).exitCode).toBe(1); + + const errorOutput = consoleError.mock.calls.flat().join('\n'); + expect(errorOutput).toMatch(/node-pty not available/i); + expect(errorOutput).toMatch(/npm install -g node-pty/i); + + expect(getGitInfo).not.toHaveBeenCalled(); + expect(getMachineId).not.toHaveBeenCalled(); + expect(isDevtunnelAvailable).not.toHaveBeenCalled(); + expect(remoteBridgeCtor).not.toHaveBeenCalled(); + expect(remoteBridgeStart).not.toHaveBeenCalled(); + expect(createTunnel).not.toHaveBeenCalled(); + expect(destroyTunnel).not.toHaveBeenCalled(); + }); + + it('uses checkNodePty helper before RemoteBridge construction in source', async () => { // Read the source file directly to verify implementation order const fs = await import('node:fs'); const path = await import('node:path'); const startTsPath = path.resolve(process.cwd(), 'packages/squad-cli/src/cli/commands/start.ts'); const source = fs.readFileSync(startTsPath, 'utf-8'); - - // Find the start of runStart function + + // Verify the dedicated helper exists and owns the optional import + const helperStart = source.indexOf('async function checkNodePty'); + expect(helperStart).toBeGreaterThan(-1); + const functionStart = source.indexOf('export async function runStart'); expect(functionStart).toBeGreaterThan(-1); + + const helperSource = source.slice(helperStart, functionStart); + expect(helperSource).toMatch(/await import\(['"]node-pty['"]\)/); - // Find node-pty import position (relative to function start) - const nodePtyImportPattern = /await import\(['"]node-pty['"]\)/; - const nodePtyMatch = source.slice(functionStart).match(nodePtyImportPattern); - expect(nodePtyMatch).toBeTruthy(); - const nodePtyPos = functionStart + (nodePtyMatch?.index || 0); + // Find helper call position (relative to function start) + const helperCallPos = source.indexOf('await checkNodePty()', functionStart); + expect(helperCallPos).toBeGreaterThan(-1); // Find RemoteBridge construction const bridgePattern = /new RemoteBridge\(/; const bridgeMatch = source.slice(functionStart).match(bridgePattern); if (bridgeMatch) { const bridgePos = functionStart + bridgeMatch.index; - // node-pty import MUST come before RemoteBridge construction - expect(nodePtyPos).toBeLessThan(bridgePos); + // checkNodePty MUST run before RemoteBridge construction + expect(helperCallPos).toBeLessThan(bridgePos); } // Find bridge.start() call @@ -65,7 +154,7 @@ describe('CLI: start command - node-pty requirement (issue #711)', () => { const bridgeStartMatch = source.slice(functionStart).match(bridgeStartPattern); if (bridgeStartMatch) { const bridgeStartPos = functionStart + bridgeStartMatch.index; - expect(nodePtyPos).toBeLessThan(bridgeStartPos); + expect(helperCallPos).toBeLessThan(bridgeStartPos); } // Find createTunnel call @@ -73,7 +162,7 @@ describe('CLI: start command - node-pty requirement (issue #711)', () => { const createTunnelMatch = source.slice(functionStart).match(createTunnelPattern); if (createTunnelMatch) { const createTunnelPos = functionStart + createTunnelMatch.index; - expect(nodePtyPos).toBeLessThan(createTunnelPos); + expect(helperCallPos).toBeLessThan(createTunnelPos); } }); });