diff --git a/app/electron/cli.test.ts b/app/electron/cli.test.ts index f71789d..faf8544 100644 --- a/app/electron/cli.test.ts +++ b/app/electron/cli.test.ts @@ -2,9 +2,9 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync, chmodSync } from 'node:fs' import { tmpdir } from 'node:os' -import { dirname, join } from 'node:path' +import { dirname, join, isAbsolute, relative, win32, posix } from 'node:path' -import { spawnCli, spawnCliAction, spawnEnvFor, spawnSpecFor, killAll, CliError, nodeManagerDirs, resolveCodeburnPath, resolveTarget } from './cli' +import { spawnCli, spawnCliAction, spawnEnvFor, spawnSpecFor, killAll, CliError, nodeManagerDirs, notFoundStage, resolveCodeburnPath, resolveTarget } from './cli' let dir: string const originalBin = process.env.CODEBURN_BIN @@ -136,6 +136,103 @@ describe('resolveTarget (bundled CLI in the packaged app)', () => { expect(resolveTarget()).toBeNull() expect(resolveCodeburnPath()).toBeNull() }) + + // The Windows P0: the packaged app set CODEBURN_BUNDLED_CLI to a C:\ path, but + // the guard was `startsWith('/')` (POSIX-only), so the bundled CLI was skipped + // and resolution fell through to a PATH search that finds nothing → not-found + // on 100% of Windows installs. The guard is now `path.isAbsolute`, which is + // the platform variant (win32 on Windows). These tests pin both the intent + // (relative paths are still rejected as a safety guard) and the Windows fix. + it('resolves an absolute CODEBURN_BUNDLED_CLI (as the packaged app sets it)', () => { + delete process.env.CODEBURN_BIN + delete process.env.VITE_DEV_SERVER_URL + process.env.CODEBURN_PATH_DIRS = '' + process.env.CODEBURN_CLI_PATH_FILE = join(dir, 'no-persisted-path') + const entry = bundledEntry('launch.js') + expect(isAbsolute(entry)).toBe(true) + process.env.CODEBURN_BUNDLED_CLI = entry + expect(resolveTarget()).toEqual({ kind: 'bundled', entry }) + }) + + it('rejects a relative CODEBURN_BUNDLED_CLI even when the file exists (guards relative injection)', () => { + delete process.env.CODEBURN_BIN + delete process.env.VITE_DEV_SERVER_URL + process.env.CODEBURN_PATH_DIRS = '' + process.env.CODEBURN_CLI_PATH_FILE = join(dir, 'no-persisted-path') + const entry = bundledEntry('rel-launch.js') + const rel = relative(process.cwd(), entry) // resolvable via cwd, but NOT absolute + expect(isAbsolute(rel)).toBe(false) + process.env.CODEBURN_BUNDLED_CLI = rel + // File exists (isFile true), so only the isAbsolute guard can reject it. + expect(resolveTarget()).toBeNull() + }) + + it('rejects a relative CODEBURN_BIN override even when the file exists', () => { + delete process.env.VITE_DEV_SERVER_URL + process.env.CODEBURN_PATH_DIRS = '' + process.env.CODEBURN_CLI_PATH_FILE = join(dir, 'no-persisted-path') + delete process.env.CODEBURN_BUNDLED_CLI + const bin = join(dir, 'rel-codeburn.js') + writeFileSync(bin, '#!/usr/bin/env node\n', { mode: 0o755 }) + chmodSync(bin, 0o755) + const rel = relative(process.cwd(), bin) + expect(isAbsolute(rel)).toBe(false) + process.env.CODEBURN_BIN = rel + expect(resolveTarget()).toBeNull() + }) +}) + +describe('absolute-path guard is cross-platform (path.isAbsolute, not startsWith("/"))', () => { + // On the POSIX CI host `isAbsolute` is path.posix.isAbsolute, so these assert + // the per-platform variants directly to encode the Windows intent regardless + // of where the suite runs. + const winPath = 'C:\\Users\\x\\resources\\cli\\dist\\launch.js' + + it('accepts a Windows absolute bundled path where the old startsWith("/") guard rejected it', () => { + expect(winPath.startsWith('/')).toBe(false) // old guard: dropped it → the P0 + expect(win32.isAbsolute(winPath)).toBe(true) // new guard on Windows: accepted + expect(win32.isAbsolute('cli\\dist\\launch.js')).toBe(false) // relative still rejected + }) + + it('accepts a POSIX absolute path and rejects a relative one (macOS/Linux unchanged)', () => { + expect(posix.isAbsolute('/res/cli/dist/launch.js')).toBe(true) + expect(posix.isAbsolute('cli/dist/launch.js')).toBe(false) + }) +}) + +describe('notFoundStage (non-sensitive telemetry enum for a not-found)', () => { + it('reports bundled-not-absolute for a relative CODEBURN_BUNDLED_CLI', () => { + delete process.env.CODEBURN_BIN + process.env.CODEBURN_BUNDLED_CLI = 'cli/dist/launch.js' + expect(notFoundStage()).toBe('bundled-not-absolute') + }) + + it('reports bundled-missing for an absolute CODEBURN_BUNDLED_CLI whose file is absent', () => { + delete process.env.CODEBURN_BIN + process.env.CODEBURN_BUNDLED_CLI = join(dir, 'nope', 'cli.js') + expect(notFoundStage()).toBe('bundled-missing') + }) + + it('reports bin-not-absolute for a relative CODEBURN_BIN override', () => { + process.env.CODEBURN_BIN = 'relative/codeburn' + delete process.env.CODEBURN_BUNDLED_CLI + expect(notFoundStage()).toBe('bin-not-absolute') + }) + + it('reports no-path-match when nothing is configured', () => { + delete process.env.CODEBURN_BIN + delete process.env.CODEBURN_BUNDLED_CLI + expect(notFoundStage()).toBe('no-path-match') + }) + + it('spawnCli rejects not-found carrying the resolution stage as detail', async () => { + delete process.env.CODEBURN_BIN + delete process.env.VITE_DEV_SERVER_URL + process.env.CODEBURN_PATH_DIRS = '' + process.env.CODEBURN_CLI_PATH_FILE = join(dir, 'no-persisted-path') + process.env.CODEBURN_BUNDLED_CLI = 'cli/dist/launch.js' // relative → bundled-not-absolute + await expect(spawnCli(['status'])).rejects.toMatchObject({ kind: 'not-found', detail: 'bundled-not-absolute' }) + }) }) describe('spawnSpecFor (bundled CLI runs via Electron-as-node)', () => { diff --git a/app/electron/cli.ts b/app/electron/cli.ts index ccbc461..c6a6fcf 100644 --- a/app/electron/cli.ts +++ b/app/electron/cli.ts @@ -1,7 +1,7 @@ import { spawn, type ChildProcess } from 'node:child_process' import { accessSync, constants, existsSync, readdirSync, readFileSync, statSync } from 'node:fs' import { homedir, platform } from 'node:os' -import { delimiter, dirname, join } from 'node:path' +import { delimiter, dirname, isAbsolute, join } from 'node:path' // Runs entirely in the Electron main process. This module must NOT import // `electron` so it stays unit-testable in a plain node environment. @@ -19,13 +19,29 @@ export type ActionResult = { ok: boolean; stdout: string; stderr: string; code: export type CliTarget = { kind: 'external'; bin: string } | { kind: 'bundled'; entry: string } type SpawnSpec = { bin: string; args: string[]; env: NodeJS.ProcessEnv } +/** + * Which resolution/spawn stage produced a `not-found`, as a non-sensitive enum + * for telemetry. Never contains a path — just names what was missing so a + * not-found is self-diagnosing without a repro. See {@link notFoundStage}. + */ +export type NotFoundStage = + | 'bin-not-absolute' + | 'bin-not-executable' + | 'bundled-not-absolute' + | 'bundled-missing' + | 'spawn-error' + | 'no-path-match' + /** Structured failure so the renderer can pick the right empty/permission state. */ export class CliError extends Error { readonly kind: CliErrorKind - constructor(kind: CliErrorKind, message: string) { + /** For `not-found` only: the non-sensitive stage enum. Undefined otherwise. */ + readonly detail?: NotFoundStage + constructor(kind: CliErrorKind, message: string, detail?: NotFoundStage) { super(message) this.name = 'CliError' this.kind = kind + this.detail = detail } } @@ -157,7 +173,7 @@ function readPersistedPath(): string | null { const file = persistedPathFile() if (!existsSync(file)) return null const value = readFileSync(file, 'utf-8').trim() - if (value && value.startsWith('/') && isExecutableFile(value)) return value + if (value && isAbsolute(value) && isExecutableFile(value)) return value } catch { // unreadable — fall through to PATH search } @@ -181,7 +197,7 @@ function readPersistedPath(): string | null { */ export function resolveTarget(): CliTarget | null { const override = process.env.CODEBURN_BIN - if (override && override.startsWith('/') && isExecutableFile(override)) return { kind: 'external', bin: override } + if (override && isAbsolute(override) && isExecutableFile(override)) return { kind: 'external', bin: override } // Dev convenience: when launched by the Vite dev server, prefer the repo's own // freshly-built CLI over a stale globally-installed/persisted one, so @@ -199,7 +215,7 @@ export function resolveTarget(): CliTarget | null { // It is passed as an argument to Electron-as-node, so it only needs to be a // readable file — no exec bit or working shebang required. const bundled = process.env.CODEBURN_BUNDLED_CLI - if (bundled && bundled.startsWith('/') && isFile(bundled)) return { kind: 'bundled', entry: bundled } + if (bundled && isAbsolute(bundled) && isFile(bundled)) return { kind: 'bundled', entry: bundled } const persisted = readPersistedPath() if (persisted) return { kind: 'external', bin: persisted } @@ -217,6 +233,27 @@ export function resolveCodeburnPath(): string | null { return target.kind === 'bundled' ? target.entry : target.bin } +/** + * Why {@link resolveTarget} found nothing, recomputed from env as a + * non-sensitive enum for telemetry. Mirrors resolveTarget's order and reports + * the first stage that disqualified a candidate (e.g. a bundled path that isn't + * absolute — the Windows P0 — vs. one that's absolute but missing, vs. no PATH + * match at all). Only enum strings escape here: never a path, arg, or message. + */ +export function notFoundStage(): NotFoundStage { + const override = process.env.CODEBURN_BIN + if (override) { + if (!isAbsolute(override)) return 'bin-not-absolute' + if (!isExecutableFile(override)) return 'bin-not-executable' + } + const bundled = process.env.CODEBURN_BUNDLED_CLI + if (bundled) { + if (!isAbsolute(bundled)) return 'bundled-not-absolute' + if (!isFile(bundled)) return 'bundled-missing' + } + return 'no-path-match' +} + function runCli(spec: SpawnSpec, cmdLabel: string, timeoutMs: number, onStderr?: (chunk: string) => void): Promise { return new Promise((resolve, reject) => { const child = spawn(spec.bin, spec.args, { shell: false, stdio: ['ignore', 'pipe', 'pipe'], env: spec.env }) @@ -261,7 +298,7 @@ function runCli(spec: SpawnSpec, cmdLabel: string, timeoutMs: number, onStderr?: }) child.on('error', err => { - finish(() => reject(new CliError('not-found', err.message))) + finish(() => reject(new CliError('not-found', err.message, 'spawn-error'))) }) child.on('close', code => { @@ -297,7 +334,7 @@ export function spawnCli( opts: { timeoutMs?: number; onStderr?: (chunk: string) => void; extraEnv?: NodeJS.ProcessEnv } = {}, ): Promise { const target = resolveTarget() - if (!target) return Promise.reject(new CliError('not-found', 'codeburn CLI not found')) + if (!target) return Promise.reject(new CliError('not-found', 'codeburn CLI not found', notFoundStage())) const spec = spawnSpecFor(target, args) if (opts.extraEnv) spec.env = { ...spec.env, ...opts.extraEnv } diff --git a/app/electron/main.test.ts b/app/electron/main.test.ts index 256d831..4788314 100644 --- a/app/electron/main.test.ts +++ b/app/electron/main.test.ts @@ -428,7 +428,7 @@ describe('createBridgeHandlers (telemetry wiring)', () => { expect(coldStarts[0]![1]).toMatchObject({ timedOut: true }) }) - it('tracks cli_error kinds from failing handlers', async () => { + it('tracks cli_error with the failing kind and the CLI subcommand (cmd = argv[0])', async () => { const telemetry = fakeTelemetry() const failing = { ...deps(telemetry), @@ -436,6 +436,35 @@ describe('createBridgeHandlers (telemetry wiring)', () => { } const handlers = createBridgeHandlers(failing) await handlers['codeburn:getSessions']!('week', 'all') - expect(telemetry.track).toHaveBeenCalledWith('cli_error', { kind: 'timeout' }) + expect(telemetry.track).toHaveBeenCalledWith('cli_error', { cmd: 'sessions', kind: 'timeout' }) + }) + + it('includes the resolution-stage detail for a not-found (self-diagnosing without a repro)', async () => { + const telemetry = fakeTelemetry() + const failing = { + ...deps(telemetry), + // Mirrors the Windows P0: bundled path present but rejected by the resolver. + spawnCli: vi.fn(async () => { throw new CliError('not-found', 'codeburn CLI not found', 'bundled-not-absolute') }), + } + const handlers = createBridgeHandlers(failing) + await handlers['codeburn:getPlans']!('week') + expect(telemetry.track).toHaveBeenCalledWith('cli_error', { cmd: 'status', kind: 'not-found', detail: 'bundled-not-absolute' }) + }) + + it('never leaks a path or message into cli_error telemetry, even when the error carries one', async () => { + const telemetry = fakeTelemetry() + const failing = { + ...deps(telemetry), + // A spawn-time ENOENT whose message embeds a filesystem path. + spawnCli: vi.fn(async () => { + throw new CliError('not-found', 'spawn C:\\Users\\alice\\secret\\codeburn.exe ENOENT', 'spawn-error') + }), + } + const handlers = createBridgeHandlers(failing) + await handlers['codeburn:getSessions']!('week', 'all') + const props = telemetry.track.mock.calls.find(([name]) => name === 'cli_error')![1] as Record + expect(props).toEqual({ cmd: 'sessions', kind: 'not-found', detail: 'spawn-error' }) + expect(JSON.stringify(props)).not.toContain('secret') + expect(JSON.stringify(props)).not.toContain('C:\\') }) }) diff --git a/app/electron/main.ts b/app/electron/main.ts index 4aefa4b..705ab5c 100644 --- a/app/electron/main.ts +++ b/app/electron/main.ts @@ -140,6 +140,26 @@ function toEnvelopeError(err: unknown): { kind: string; message: string } { return { kind: 'nonzero', message: sanitizeError(err instanceof Error ? err.message : String(err)) } } +/** + * Props for a `cli_error` telemetry event. Deliberately carries only + * non-sensitive enums so the event is diagnosable without a repro yet leaks + * nothing: `cmd` is the CLI subcommand (argv[0], a fixed literal like 'status'/ + * 'sessions' — never the full args, which can hold paths), and `detail` is the + * not-found resolution/spawn stage. The error's `message` (which may contain a + * path or stderr) is never read here — only `kind` and the stage enum are. + */ +function cliErrorProps(err: unknown, cmd: string | undefined): Record { + const props: Record = {} + if (cmd) props.cmd = cmd + if (err instanceof CliError) { + props.kind = err.kind + if (err.kind === 'not-found' && err.detail) props.detail = err.detail + } else { + props.kind = 'nonzero' + } + return props +} + type Deps = { spawnCli: (args: string[], opts?: { timeoutMs?: number; onStderr?: (chunk: string) => void; extraEnv?: NodeJS.ProcessEnv }) => Promise spawnCliAction: (args: string[], opts?: { timeoutMs?: number }) => Promise @@ -181,11 +201,14 @@ export function createBridgeHandlers(deps: Deps = { spawnCli, spawnCliAction, re } const run = (build: (...args: any[]) => string[]): Handler => async (...args: any[]) => { + let cmd: string | undefined try { - return { ok: true, value: await deps.spawnCli(build(...args)) } + const argv = build(...args) + cmd = argv[0] + return { ok: true, value: await deps.spawnCli(argv) } } catch (err) { const error = toEnvelopeError(err) - telemetry?.track('cli_error', { kind: error.kind }) + telemetry?.track('cli_error', cliErrorProps(err, cmd)) return { ok: false, error } } } @@ -215,7 +238,7 @@ export function createBridgeHandlers(deps: Deps = { spawnCli, spawnCliAction, re } catch (err) { const error = toEnvelopeError(err) if (!overviewWarmed) emitColdStart(error.kind === 'timeout') - telemetry?.track('cli_error', { kind: error.kind }) + telemetry?.track('cli_error', cliErrorProps(err, 'status')) return { ok: false, error } } }