diff --git a/packages/cli/src/setup.ts b/packages/cli/src/setup.ts index 2ff58dbf..2208d2b4 100644 --- a/packages/cli/src/setup.ts +++ b/packages/cli/src/setup.ts @@ -12,6 +12,7 @@ import { runtimeInstallPolicyFromFlags } from './managed-python-command.js'; import { readManagedPythonRuntimeStatus } from './managed-python-runtime.js'; import { resolveProjectRuntimeRequirements } from './runtime-requirements.js'; import { isKtxSetupExitError } from './setup-interrupt.js'; +import type { CommandOutcome } from './telemetry/index.js'; import { type KtxAgentScope, type KtxAgentTarget, @@ -211,6 +212,80 @@ function setupTelemetryOutcome( return 'abandoned'; } +interface SetupCommandAnnotation { + outcome: CommandOutcome; + errorClass?: string; + errorDetail?: string; +} + +/** + * Classify a terminal non-ready setup status into the `command` telemetry + * outcome. The setup flow is the decision-maker and knows the difference: + * - `failed` is a genuine error; attach a step-scoped reason so the dashboard + * shows an actionable signature instead of a blank. + * - `missing-input` from a *non-interactive* run is an automation error + * (required flags absent and no prompt was possible); attach a reason too. + * - `missing-input` from an interactive prompt, or a project `cancelled`, is the + * user backing out of the wizard — an abort, not a failure. Keep it out of + * error telemetry so it stops inflating the error count. + * + * `interactive` must reflect whether a prompt could actually be shown — input + * is enabled AND a TTY is attached. `inputMode: 'auto'` alone is not enough: a + * piped/CI run without `--no-input` is still non-interactive, and steps such as + * the project step return `missing-input` ("pass --yes …") there without ever + * prompting. Treating that as an abort would make a broken automation run exit + * 0, so it must classify as an error. + * + * Reasons are synthetic, step-scoped strings (no user input), so they satisfy + * the telemetry privacy rules. The step's own `errorDetail`, when present, has + * already been vetted for the `setup_step` event and is safe to reuse. + */ +function setupCommandOutcomeAnnotation(input: { + status: 'failed' | 'missing-input' | 'cancelled'; + step: TelemetrySetupStep; + interactive: boolean; + errorDetail?: string; +}): SetupCommandAnnotation { + if (input.status === 'failed') { + return { + outcome: 'error', + errorClass: 'KtxSetupStepFailed', + errorDetail: input.errorDetail ?? `${input.step} setup step failed`, + }; + } + if (input.status === 'missing-input' && !input.interactive) { + return { + outcome: 'error', + errorClass: 'KtxSetupMissingInput', + errorDetail: `${input.step} setup step requires input not provided in a non-interactive run`, + }; + } + return { outcome: 'aborted' }; +} + +/** + * Single source of truth for how a non-ready setup step ends: the process exit + * code and the telemetry annotation are both derived from one classification, + * so they can never disagree. A genuine failure (`error`) exits non-zero; an + * abort — the user leaving an interactive wizard — exits 0, matching the entry + * menu's "Exit", a project cancellation, and a confirmed Ctrl+C. + */ +/** @internal */ +export function setupTerminalOutcome(input: { + status: 'failed' | 'missing-input' | 'cancelled'; + step: TelemetrySetupStep; + interactive: boolean; + errorDetail?: string; +}): { exitCode: number; annotation: SetupCommandAnnotation } { + const annotation = setupCommandOutcomeAnnotation(input); + return { exitCode: annotation.outcome === 'error' ? 1 : 0, annotation }; +} + +async function annotateSetupCommandOutcome(annotation: SetupCommandAnnotation): Promise { + const { annotateCommandOutcome } = await import('./telemetry/index.js'); + annotateCommandOutcome(annotation); +} + async function recordSetupStep(input: { projectDir: string; step: TelemetrySetupStep; @@ -573,6 +648,10 @@ async function runKtxSetupInner(args: KtxSetupArgs, io: KtxCliIo, deps: KtxSetup args.inputMode !== 'disabled' && !args.agents && (io.stdout.isTTY === true || deps.entryMenuDeps?.prompts !== undefined); + // A prompt is only possible when input is enabled AND a TTY is attached. A + // piped/CI `ktx setup` without `--no-input` is still `inputMode: 'auto'` but + // cannot prompt, so its `missing-input` is an automation error, not an abort. + const interactive = args.inputMode !== 'disabled' && io.stdout.isTTY === true; setupLoop: while (true) { entryAction = undefined; @@ -619,7 +698,13 @@ async function runKtxSetupInner(args: KtxSetupArgs, io: KtxCliIo, deps: KtxSetup } if (projectResult.status !== 'ready') { - return projectResult.status === 'cancelled' ? 0 : 1; + const terminal = setupTerminalOutcome({ + status: projectResult.status, + step: 'project', + interactive, + }); + await annotateSetupCommandOutcome(terminal.annotation); + return terminal.exitCode; } const agentsRequested = args.agents || entryAction === 'agents'; @@ -856,11 +941,15 @@ async function runKtxSetupInner(args: KtxSetupArgs, io: KtxCliIo, deps: KtxSetup ...(stepResult.errorDetail ? { errorDetail: stepResult.errorDetail } : {}), }); - if (stepResult.status === 'failed') { - return 1; - } - if (stepResult.status === 'missing-input') { - return 1; + if (stepResult.status === 'failed' || stepResult.status === 'missing-input') { + const terminal = setupTerminalOutcome({ + status: stepResult.status, + step, + interactive, + ...(stepResult.errorDetail ? { errorDetail: stepResult.errorDetail } : {}), + }); + await annotateSetupCommandOutcome(terminal.annotation); + return terminal.exitCode; } if (stepResult.status === 'back') { const previousIndex = previousNavigableStepIndex(stepIndex); diff --git a/packages/cli/src/telemetry/command-hook.ts b/packages/cli/src/telemetry/command-hook.ts index 99f8723e..5113c7fa 100644 --- a/packages/cli/src/telemetry/command-hook.ts +++ b/packages/cli/src/telemetry/command-hook.ts @@ -9,6 +9,9 @@ interface CommandSpan { hasProject: boolean; attachProjectGroup: boolean; startedAt: number; + annotatedOutcome?: CommandOutcome; + annotatedErrorClass?: string; + annotatedErrorDetail?: string; } export interface CompletedCommandSpan { @@ -29,6 +32,41 @@ export function beginCommandSpan(input: CommandSpan): void { activeCommandSpan = input; } +/** + * Let a command action record the true outcome and reason on the active span. + * + * The Commander wrapper can only derive an outcome from a thrown error or the + * process exit code, so a command that exits non-zero *without throwing* (e.g. + * `ktx setup` when the user abandons the wizard) lands as `outcome: 'error'` + * with no `errorClass`/`errorDetail` — an unactionable blank in the dashboard. + * The action is the decision-maker: it can mark the run `aborted`, or attach a + * scrubbed reason so the next occurrence is self-diagnosing. A later thrown + * error still wins (see {@link completeCommandSpan}), since that is the most + * authoritative signal and also feeds the `$exception` stream. No-ops when no + * span is active so call sites stay safe in tests and bare-help paths. + * + * Values are emitted verbatim and must already satisfy the telemetry privacy + * rules — pass synthetic or already-scrubbed strings, never raw user input. + */ +export function annotateCommandOutcome(input: { + outcome?: CommandOutcome; + errorClass?: string; + errorDetail?: string; +}): void { + if (!activeCommandSpan) { + return; + } + if (input.outcome !== undefined) { + activeCommandSpan.annotatedOutcome = input.outcome; + } + if (input.errorClass !== undefined) { + activeCommandSpan.annotatedErrorClass = input.errorClass; + } + if (input.errorDetail !== undefined) { + activeCommandSpan.annotatedErrorDetail = input.errorDetail; + } +} + export function completeCommandSpan(input: { completedAt: number; outcome: CommandOutcome; @@ -40,13 +78,17 @@ export function completeCommandSpan(input: { return undefined; } - const errorClass = input.error ? scrubErrorClass(input.error) : undefined; - const errorDetail = input.error ? formatErrorDetail(input.error) : undefined; + // Precedence: a thrown error is authoritative; otherwise an action's own + // annotation; otherwise the wrapper's exit-code-derived outcome. + const thrown = Boolean(input.error); + const outcome = thrown ? input.outcome : (span.annotatedOutcome ?? input.outcome); + const errorClass = thrown ? scrubErrorClass(input.error) : span.annotatedErrorClass; + const errorDetail = thrown ? formatErrorDetail(input.error) : span.annotatedErrorDetail; return { commandPath: span.commandPath, durationMs: Math.max(0, input.completedAt - span.startedAt), - outcome: input.outcome, + outcome, ...(errorClass ? { errorClass } : {}), ...(errorDetail ? { errorDetail } : {}), flagsPresent: span.flagsPresent, diff --git a/packages/cli/src/telemetry/index.ts b/packages/cli/src/telemetry/index.ts index e3716060..24a62d11 100644 --- a/packages/cli/src/telemetry/index.ts +++ b/packages/cli/src/telemetry/index.ts @@ -1,6 +1,7 @@ import { getKtxCliPackageInfo, type KtxCliIo, type KtxCliPackageInfo } from '../cli-runtime.js'; import { loadKtxProject } from '../context/project/project.js'; import { + annotateCommandOutcome, beginCommandSpan, completeCommandSpan, type CommandOutcome, @@ -18,7 +19,7 @@ import { import { computeTelemetryProjectId, loadTelemetryIdentity } from './identity.js'; import { buildProjectStackSnapshotFields } from './project-snapshot.js'; -export { beginCommandSpan, completeCommandSpan, reportException, shutdownTelemetryEmitter }; +export { annotateCommandOutcome, beginCommandSpan, completeCommandSpan, reportException, shutdownTelemetryEmitter }; export type { CommandOutcome, CompletedCommandSpan, ExceptionContext }; export async function showTelemetryNoticeIfNeeded(io: KtxCliIo, packageInfo: KtxCliPackageInfo): Promise { diff --git a/packages/cli/test/cli-program-telemetry.test.ts b/packages/cli/test/cli-program-telemetry.test.ts index fb624c66..4b8a07e5 100644 --- a/packages/cli/test/cli-program-telemetry.test.ts +++ b/packages/cli/test/cli-program-telemetry.test.ts @@ -135,6 +135,57 @@ describe('runCommanderKtxCli telemetry', () => { expect(io.stderr()).not.toContain(missingProjectDir); }); + it('emits aborted (not error) when setup exits non-zero after the user abandons the wizard', async () => { + const io = makeIo(true); + const deps: KtxCliDeps = { + setup: async () => { + // What runKtxSetup does when an interactive step is abandoned: it + // annotates the span and returns a non-zero exit code without throwing. + const { annotateCommandOutcome } = await import('../src/telemetry/index.js'); + annotateCommandOutcome({ outcome: 'aborted' }); + return 1; + }, + }; + + await expect( + runCommanderKtxCli(['--project-dir', tempDir, 'setup'], io.io, deps, info, { runInit: async () => 0 }), + ).resolves.toBe(1); + + expect(io.stderr()).toContain('"event":"command"'); + expect(io.stderr()).toContain('"commandPath":["ktx","setup"]'); + // The non-zero exit alone would have produced a blank "error"; the + // annotation reclassifies it as a user abort that leaves the error view. + expect(io.stderr()).toContain('"outcome":"aborted"'); + expect(io.stderr()).not.toContain('"outcome":"error"'); + expect(reportExceptionMock).not.toHaveBeenCalled(); + }); + + it('emits a self-diagnosing error reason when a setup step genuinely fails without throwing', async () => { + const io = makeIo(true); + const deps: KtxCliDeps = { + setup: async () => { + const { annotateCommandOutcome } = await import('../src/telemetry/index.js'); + annotateCommandOutcome({ + outcome: 'error', + errorClass: 'KtxSetupStepFailed', + errorDetail: 'runtime setup step failed', + }); + return 1; + }, + }; + + await expect( + runCommanderKtxCli(['--project-dir', tempDir, 'setup'], io.io, deps, info, { runInit: async () => 0 }), + ).resolves.toBe(1); + + expect(io.stderr()).toContain('"outcome":"error"'); + expect(io.stderr()).toContain('"errorClass":"KtxSetupStepFailed"'); + expect(io.stderr()).toContain('"errorDetail":"runtime setup step failed"'); + // Non-throwing failures have no exception twin; the command event carries + // the reason on its own. + expect(reportExceptionMock).not.toHaveBeenCalled(); + }); + it('does not import or emit telemetry for help, version, bare non-TTY, or unknown top-level command', async () => { const helpIo = makeIo(true); await expect(runCommanderKtxCli(['--help'], helpIo.io, {}, info, { runInit: async () => 0 })).resolves.toBe(0); diff --git a/packages/cli/test/setup.test.ts b/packages/cli/test/setup.test.ts index 4d46d08c..04ca32d1 100644 --- a/packages/cli/test/setup.test.ts +++ b/packages/cli/test/setup.test.ts @@ -7,9 +7,16 @@ import { writeKtxSetupState } from '../src/context/project/setup-config.js'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { localFakeBundleReport, persistLocalBundleReport } from './ingest.test-utils.js'; +import { beginCommandSpan, completeCommandSpan, resetCommandSpan } from '../src/telemetry/command-hook.js'; import { contextBuildCommands, writeKtxSetupContextState } from '../src/setup-context.js'; import { runDemoTour } from '../src/setup-demo-tour.js'; -import { formatKtxSetupCompletionSummary, formatKtxSetupStatus, readKtxSetupStatus, runKtxSetup } from '../src/setup.js'; +import { + formatKtxSetupCompletionSummary, + formatKtxSetupStatus, + readKtxSetupStatus, + runKtxSetup, + setupTerminalOutcome, +} from '../src/setup.js'; vi.mock('../src/setup-demo-tour.js', () => ({ runDemoTour: vi.fn(async () => 0), @@ -17,13 +24,13 @@ vi.mock('../src/setup-demo-tour.js', () => ({ const execFileAsync = promisify(execFile); -function makeIo() { +function makeIo(stdoutIsTTY = false) { let stdout = ''; let stderr = ''; return { io: { stdout: { - isTTY: false, + isTTY: stdoutIsTTY, write: (chunk: string) => { stdout += chunk; }, @@ -2667,3 +2674,216 @@ describe('setup status', () => { expect(embeddings).not.toHaveBeenCalled(); }); }); + +describe('setupTerminalOutcome', () => { + it('exits non-zero and reports a self-diagnosing error for a genuine step failure', () => { + expect(setupTerminalOutcome({ status: 'failed', step: 'runtime', interactive: true })).toEqual({ + exitCode: 1, + annotation: { + outcome: 'error', + errorClass: 'KtxSetupStepFailed', + errorDetail: 'runtime setup step failed', + }, + }); + }); + + it('reuses a step-provided errorDetail for failures', () => { + expect( + setupTerminalOutcome({ + status: 'failed', + step: 'sources', + interactive: true, + errorDetail: 'metabase source ingest failed', + }), + ).toEqual({ + exitCode: 1, + annotation: { + outcome: 'error', + errorClass: 'KtxSetupStepFailed', + errorDetail: 'metabase source ingest failed', + }, + }); + }); + + it('exits non-zero for missing input in a non-interactive run (an automation error)', () => { + // Both `--no-input` and a piped/CI run without a TTY are non-interactive. + expect(setupTerminalOutcome({ status: 'missing-input', step: 'models', interactive: false })).toEqual({ + exitCode: 1, + annotation: { + outcome: 'error', + errorClass: 'KtxSetupMissingInput', + errorDetail: 'models setup step requires input not provided in a non-interactive run', + }, + }); + }); + + it('exits zero and aborts (not errors) when the user leaves an interactive prompt', () => { + expect(setupTerminalOutcome({ status: 'missing-input', step: 'models', interactive: true })).toEqual({ + exitCode: 0, + annotation: { outcome: 'aborted' }, + }); + }); + + it('exits zero and aborts on a project cancellation', () => { + expect(setupTerminalOutcome({ status: 'cancelled', step: 'project', interactive: true })).toEqual({ + exitCode: 0, + annotation: { outcome: 'aborted' }, + }); + }); +}); + +describe('runKtxSetup command-span annotation', () => { + let tempDir: string; + + beforeEach(async () => { + tempDir = await mkdtemp(join(tmpdir(), 'ktx-setup-span-')); + }); + + afterEach(async () => { + resetCommandSpan(); + await rm(tempDir, { recursive: true, force: true }); + }); + + it('annotates the active command span with the step-failure reason on a non-throwing exit', async () => { + resetCommandSpan(); + beginCommandSpan({ + commandPath: ['ktx', 'setup'], + flagsPresent: {}, + hasProject: false, + attachProjectGroup: true, + startedAt: 0, + }); + const testIo = makeIo(); + + await expect( + runKtxSetup( + { + command: 'run', + projectDir: tempDir, + mode: 'auto', + agents: false, + skipAgents: true, + inputMode: 'disabled', + yes: true, + cliVersion: '0.2.0', + skipLlm: true, + skipEmbeddings: true, + databaseSchemas: [], + skipDatabases: true, + skipSources: true, + }, + testIo.io, + { + model: async () => ({ status: 'skipped', projectDir: tempDir }), + embeddings: async () => ({ status: 'skipped', projectDir: tempDir }), + databases: async () => ({ status: 'skipped', projectDir: tempDir }), + sources: async () => ({ status: 'skipped', projectDir: tempDir }), + runtime: async () => ({ + status: 'failed', + projectDir: tempDir, + requirements: { features: ['core'], requirements: [] }, + }), + }, + ), + ).resolves.toBe(1); + + // The wrapper would derive a blank 'error' from the non-zero exit; the + // annotation supplies the actionable reason instead. + const completed = completeCommandSpan({ completedAt: 1, outcome: 'error' }); + expect(completed?.outcome).toBe('error'); + expect(completed?.errorClass).toBe('KtxSetupStepFailed'); + expect(completed?.errorDetail).toBe('runtime setup step failed'); + }); + + it('exits zero and marks the span aborted when the user abandons an interactive step', async () => { + resetCommandSpan(); + beginCommandSpan({ + commandPath: ['ktx', 'setup'], + flagsPresent: {}, + hasProject: true, + attachProjectGroup: true, + startedAt: 0, + }); + await writeFile(join(tempDir, 'ktx.yaml'), 'connections: {}\n', 'utf-8'); + // A real interactive session: input enabled AND a TTY attached. + const testIo = makeIo(true); + + await expect( + runKtxSetup( + { + command: 'run', + projectDir: tempDir, + mode: 'auto', + agents: false, + agentScope: 'project', + skipAgents: true, + inputMode: 'auto', + yes: false, + cliVersion: '0.2.0', + skipLlm: true, + skipEmbeddings: false, + databaseSchemas: [], + skipDatabases: true, + skipSources: true, + }, + testIo.io, + { + // The user backs out of the embeddings prompt (interactive + // missing-input). That is an abort, not a failure. + embeddings: async () => ({ status: 'missing-input', projectDir: tempDir }), + }, + ), + ).resolves.toBe(0); + + const completed = completeCommandSpan({ completedAt: 1, outcome: 'error' }); + expect(completed?.outcome).toBe('aborted'); + expect(completed?.errorClass).toBeUndefined(); + expect(completed?.errorDetail).toBeUndefined(); + }); + + it('exits non-zero with a reason when input is missing in a non-TTY run (no --no-input)', async () => { + resetCommandSpan(); + beginCommandSpan({ + commandPath: ['ktx', 'setup'], + flagsPresent: {}, + hasProject: true, + attachProjectGroup: true, + startedAt: 0, + }); + await writeFile(join(tempDir, 'ktx.yaml'), 'connections: {}\n', 'utf-8'); + // `inputMode: 'auto'` but NO TTY — a piped/CI run that never prompted. The + // step's missing-input is an automation error, not a user abort, so it must + // exit non-zero rather than report an aborted "success". + const testIo = makeIo(false); + + await expect( + runKtxSetup( + { + command: 'run', + projectDir: tempDir, + mode: 'auto', + agents: false, + agentScope: 'project', + skipAgents: true, + inputMode: 'auto', + yes: false, + cliVersion: '0.2.0', + skipLlm: true, + skipEmbeddings: false, + databaseSchemas: [], + skipDatabases: true, + skipSources: true, + }, + testIo.io, + { + embeddings: async () => ({ status: 'missing-input', projectDir: tempDir }), + }, + ), + ).resolves.toBe(1); + + const completed = completeCommandSpan({ completedAt: 1, outcome: 'error' }); + expect(completed?.outcome).toBe('error'); + expect(completed?.errorClass).toBe('KtxSetupMissingInput'); + expect(completed?.errorDetail).toBe('embeddings setup step requires input not provided in a non-interactive run'); + }); +}); diff --git a/packages/cli/test/telemetry/command-hook.test.ts b/packages/cli/test/telemetry/command-hook.test.ts index 63909ac6..9a4c5fdd 100644 --- a/packages/cli/test/telemetry/command-hook.test.ts +++ b/packages/cli/test/telemetry/command-hook.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it } from 'vitest'; -import { beginCommandSpan, completeCommandSpan, resetCommandSpan } from '../../src/telemetry/command-hook.js'; +import { + annotateCommandOutcome, + beginCommandSpan, + completeCommandSpan, + resetCommandSpan, +} from '../../src/telemetry/command-hook.js'; describe('telemetry command hook', () => { it('builds a completed command event from a span', () => { @@ -53,4 +58,75 @@ describe('telemetry command hook', () => { expect(completed?.errorClass).toBe('KtxConnectionError'); expect(completed?.errorDetail).toBe('connect ECONNREFUSED 127.0.0.1:5432'); }); + + it('applies an annotated outcome when no error is thrown', () => { + resetCommandSpan(); + beginCommandSpan({ + commandPath: ['ktx', 'setup'], + flagsPresent: {}, + hasProject: false, + attachProjectGroup: true, + startedAt: 0, + }); + + annotateCommandOutcome({ outcome: 'aborted' }); + + // The wrapper derives 'error' from a non-zero exit code, but the action + // knows the user aborted — the annotation must win on the non-throwing path. + const completed = completeCommandSpan({ completedAt: 5, outcome: 'error' }); + expect(completed?.outcome).toBe('aborted'); + expect(completed?.errorClass).toBeUndefined(); + expect(completed?.errorDetail).toBeUndefined(); + }); + + it('applies an annotated reason so a non-throwing failure is self-diagnosing', () => { + resetCommandSpan(); + beginCommandSpan({ + commandPath: ['ktx', 'setup'], + flagsPresent: {}, + hasProject: false, + attachProjectGroup: true, + startedAt: 0, + }); + + annotateCommandOutcome({ + outcome: 'error', + errorClass: 'KtxSetupStepFailed', + errorDetail: 'runtime setup step failed', + }); + + const completed = completeCommandSpan({ completedAt: 5, outcome: 'error' }); + expect(completed?.outcome).toBe('error'); + expect(completed?.errorClass).toBe('KtxSetupStepFailed'); + expect(completed?.errorDetail).toBe('runtime setup step failed'); + }); + + it('lets a thrown error take precedence over an annotation', () => { + resetCommandSpan(); + beginCommandSpan({ + commandPath: ['ktx', 'setup'], + flagsPresent: {}, + hasProject: false, + attachProjectGroup: true, + startedAt: 0, + }); + + annotateCommandOutcome({ outcome: 'aborted', errorDetail: 'user aborted' }); + + class KtxSetupBoomError extends Error {} + const completed = completeCommandSpan({ + completedAt: 5, + outcome: 'error', + error: new KtxSetupBoomError('boom'), + }); + expect(completed?.outcome).toBe('error'); + expect(completed?.errorClass).toBe('KtxSetupBoomError'); + expect(completed?.errorDetail).toBe('boom'); + }); + + it('ignores annotation when no span is active', () => { + resetCommandSpan(); + expect(() => annotateCommandOutcome({ outcome: 'aborted' })).not.toThrow(); + expect(completeCommandSpan({ completedAt: 1, outcome: 'ok' })).toBeUndefined(); + }); });