diff --git a/.changeset/start-tunnel-node-pty.md b/.changeset/start-tunnel-node-pty.md new file mode 100644 index 000000000..b0d27302c --- /dev/null +++ b/.changeset/start-tunnel-node-pty.md @@ -0,0 +1,5 @@ +--- +'@bradygaster/squad-cli': patch +--- + +Validate node-pty availability before side effects in squad start --tunnel 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/package.json b/packages/squad-cli/package.json index f4bc9a92b..e84fa7aa5 100644 --- a/packages/squad-cli/package.json +++ b/packages/squad-cli/package.json @@ -208,5 +208,9 @@ "type": "git", "url": "git+https://github.com/bradygaster/squad.git", "directory": "packages/squad-cli" + }, + "optionalDependencies": { + "node-pty": "^1.1.0", + "qrcode-terminal": "^0.12.0" } } diff --git a/packages/squad-cli/src/cli/commands/start.ts b/packages/squad-cli/src/cli/commands/start.ts index 4c85cf835..b31de6f66 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,7 +40,35 @@ export interface StartOptions { command?: string; } +async function checkNodePty(): Promise { + try { + 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 { + nodePty = await checkNodePty(); + } catch (err) { + const detail = err instanceof Error && typeof err.cause === 'string' ? err.cause : undefined; + console.error(`${YELLOW}\u2717${RESET} ${(err as Error).message}`); + console.error(` ${DIM}npm install -g node-pty${RESET}`); + if (detail) { + console.error(`\n${DIM}Error: ${detail}${RESET}`); + } + process.exit(1); + } + const { repo, branch } = getGitInfo(cwd); const machine = getMachineId(); const squadDir = storage.existsSync(path.join(cwd, '.squad')) @@ -47,7 +77,7 @@ export async function runStart(cwd: string, options: StartOptions): Promise { console.log(code); }); } catch {} @@ -120,9 +150,6 @@ export async function runStart(cwd: string, options: StartOptions): Promise { - 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: 'inherit', + 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,29 +189,28 @@ 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. */ - 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'); - - // Allow MODULE_NOT_FOUND if it's for an optional dependency (node-pty), - // not for the command module itself - 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); - } + 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)); } // ============================================================================ @@ -194,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`) @@ -204,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'); }); @@ -262,6 +289,37 @@ describe('CLI packaging smoke test', { timeout: 120_000 }, () => { }); } + 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 c395e4d53..1a5db82ce 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 () => { @@ -25,3 +30,139 @@ describe('CLI: start command', () => { 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('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'); + + // 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 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; + // checkNodePty MUST run before RemoteBridge construction + expect(helperCallPos).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(helperCallPos).toBeLessThan(bridgeStartPos); + } + + // Find createTunnel call + const createTunnelPattern = /createTunnel\(/; + const createTunnelMatch = source.slice(functionStart).match(createTunnelPattern); + if (createTunnelMatch) { + const createTunnelPos = functionStart + createTunnelMatch.index; + expect(helperCallPos).toBeLessThan(createTunnelPos); + } + }); +});