Merge pull request #733 from getagentseal/fix/windows-cli-and-error-telemetry

fix: resolve bundled CLI on Windows (100% not-found) + enrich cli_error telemetry

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)', () => {

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<unknown> {
   return new Promise<unknown>((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<unknown> {
   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 }
 

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<string, unknown>
+    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:\\')
   })
 })

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<string, unknown> {
+  const props: Record<string, unknown> = {}
+  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<unknown>
   spawnCliAction: (args: string[], opts?: { timeoutMs?: number }) => Promise<ActionResult>
@@ -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 }
     }
   }