From 2df494c15dfdaa350ec5c0c608b3c67286b36e71 Mon Sep 17 00:00:00 2001 From: pnoker Date: Tue, 6 Oct 2026 05:39:52 +0800 Subject: [PATCH] fix(cli): harden error exits, validate inputs, and reject bad formats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit process.exit crashed node on windows with a libuv assertion while fetch keep-alive handles were open — printandexit and handlefatalerror now set the exit code and throw a silentexit control-flow signal that the entry point unwinds cleanly. detectformat rejected unknown --format values but commander routes the flag to the program level, so applyglobaloptions now validates it there too. model add/update gains local temperature range and max-tokens validation (nan silently became null before), and provider add/update/check validates the protocol type enum locally with a clear message instead of a backend 400. Co-Authored-By: Claude Code --- dc3-cli/src/commands/model.ts | 27 +++++++++++++++++++++++---- dc3-cli/src/commands/provider.ts | 20 ++++++++++++++++---- dc3-cli/src/core/context.ts | 10 +++++++++- dc3-cli/src/core/errors.ts | 14 +++++++++----- dc3-cli/src/index.ts | 15 +++++++++++++-- dc3-cli/src/utils/format.ts | 25 ++++++++++++++++++++++--- dc3-cli/test/agentic-manage.test.ts | 6 +++++- dc3-cli/test/analytics-alert.test.ts | 3 ++- dc3-cli/test/config.test.ts | 13 ++++++++++++- dc3-cli/test/contract.test.ts | 16 ++++++++-------- dc3-cli/test/device.test.ts | 3 +++ dc3-cli/test/manager-write.test.ts | 3 ++- dc3-cli/test/session.test.ts | 3 ++- 13 files changed, 126 insertions(+), 32 deletions(-) diff --git a/dc3-cli/src/commands/model.ts b/dc3-cli/src/commands/model.ts index 508080449..a8783b14f 100644 --- a/dc3-cli/src/commands/model.ts +++ b/dc3-cli/src/commands/model.ts @@ -20,6 +20,25 @@ import { detectFormat, printAndExit } from '../utils/format.js'; const BASE = '/api/v3/agentic/model'; +const parseTemperature = (value: string): number => { + const parsed = parseFloat(value); + if (Number.isNaN(parsed)) { + throw new Error(`option '--temperature ${value}' is not a valid number`); + } + if (parsed < 0 || parsed > 2) { + throw new Error(`option '--temperature ${value}' is out of range (0.0–2.0)`); + } + return parsed; +}; + +const parseMaxTokens = (value: string): number => { + const parsed = parseInt(value, 10); + if (Number.isNaN(parsed) || parsed < 1) { + throw new Error(`option '--max-tokens ${value}' must be a positive integer`); + } + return parsed; +}; + /** * Register the `model` command tree on the CLI program. * @param program - commander program to attach the command to @@ -61,8 +80,8 @@ export function registerModelCommand(program: Command): void { .option('--no-vision', 'Disable vision input (default)') .option('--reasoning', 'Enable reasoning mode') .option('--no-reasoning', 'Disable reasoning mode (default)') - .option('--temperature ', 'Sampling temperature (0.0-2.0)', parseFloat) - .option('--max-tokens ', 'Maximum output tokens', parseInt) + .option('--temperature ', 'Sampling temperature (0.0-2.0)', parseTemperature) + .option('--max-tokens ', 'Maximum output tokens', parseMaxTokens) .option('--default', 'Set as the tenant default model') .option('--enable', 'Enable the model (default)') .option('--disable', 'Disable the model') @@ -100,8 +119,8 @@ export function registerModelCommand(program: Command): void { .option('--no-vision', 'Disable vision') .option('--reasoning', 'Enable reasoning') .option('--no-reasoning', 'Disable reasoning') - .option('--temperature ', 'New sampling temperature (0.0-2.0)', parseFloat) - .option('--max-tokens ', 'New maximum output tokens', parseInt) + .option('--temperature ', 'New sampling temperature (0.0-2.0)', parseTemperature) + .option('--max-tokens ', 'New maximum output tokens', parseMaxTokens) .option('--default', 'Set as tenant default') .option('--enable', 'Enable the model') .option('--disable', 'Disable the model') diff --git a/dc3-cli/src/commands/provider.ts b/dc3-cli/src/commands/provider.ts index ac2662705..72af86ef1 100644 --- a/dc3-cli/src/commands/provider.ts +++ b/dc3-cli/src/commands/provider.ts @@ -19,6 +19,15 @@ import { dc3Client } from '../core/client.js'; import { detectFormat, printAndExit } from '../utils/format.js'; const BASE = '/api/v3/agentic/provider'; +const PROVIDER_TYPES = ['OPENAI_COMPATIBLE', 'ANTHROPIC'] as const; + +const parseProviderType = (value: string): string => { + const upper = value.toUpperCase(); + if (!PROVIDER_TYPES.includes(upper as (typeof PROVIDER_TYPES)[number])) { + throw new Error(`option '--type ${value}' is invalid. allowed: ${PROVIDER_TYPES.join(', ')}`); + } + return upper; +}; /** * Register the `provider` command tree on the CLI program. @@ -42,7 +51,7 @@ export function registerProviderCommand(program: Command): void { .description('Add a new AI model provider') .requiredOption('--name ', 'Provider display name') .requiredOption('--base-url ', 'API base URL (e.g. https://api.deepseek.com)') - .option('--type ', 'Provider protocol type (OPENAI_COMPATIBLE or ANTHROPIC)', 'OPENAI_COMPATIBLE') + .option('--type ', 'Provider protocol type (OPENAI_COMPATIBLE or ANTHROPIC)', parseProviderType, 'OPENAI_COMPATIBLE') .option('--api-key ', 'API key (omit for local endpoints without auth)') .option('--default', 'Set as the tenant default provider') .option('--enable', 'Enable the provider (default)') @@ -69,7 +78,7 @@ export function registerProviderCommand(program: Command): void { .description('Update an AI model provider (blank api-key keeps the stored one)') .option('--name ', 'New display name') .option('--base-url ', 'New API base URL') - .option('--type ', 'New protocol type (OPENAI_COMPATIBLE or ANTHROPIC)') + .option('--type ', 'New protocol type (OPENAI_COMPATIBLE or ANTHROPIC)', parseProviderType) .option('--api-key ', 'New API key (omit to keep the stored one)') .option('--default', 'Set as the tenant default') .option('--enable', 'Enable the provider') @@ -111,7 +120,7 @@ export function registerProviderCommand(program: Command): void { .description('Check provider connectivity (L1 model-list + L2 minimal chat probe)') .option('--id ', 'Provider ID to check (saved configuration)') .option('--base-url ', 'Draft mode: check an unsaved base URL') - .option('--type ', 'Draft mode: protocol type (OPENAI_COMPATIBLE or ANTHROPIC)') + .option('--type ', 'Protocol type (required for draft mode; default OPENAI_COMPATIBLE)', parseProviderType) .option('--api-key ', 'Draft mode: API key (optional)') .option('--model ', 'Model to use for the L2 chat probe') .option('--level ', 'Probe level: L1 (model list only), L2 (chat only), or BOTH (default)', 'BOTH') @@ -128,7 +137,10 @@ export function registerProviderCommand(program: Command): void { const body: Record = { level: opts.level }; if (opts.id) body.id = opts.id; if (opts.baseUrl) body.baseUrl = opts.baseUrl; - if (opts.type) body.providerType = opts.type; + // Draft mode: default to OPENAI_COMPATIBLE when type is not specified + // (the backend requires an explicit type for draft checks). + if (opts.baseUrl) body.providerType = opts.type || 'OPENAI_COMPATIBLE'; + else if (opts.type) body.providerType = opts.type; if (opts.apiKey !== undefined) body.apiKey = opts.apiKey; if (opts.model !== undefined) body.model = opts.model; const result = await dc3Client.post(`${BASE}/check`, body); diff --git a/dc3-cli/src/core/context.ts b/dc3-cli/src/core/context.ts index fe4efd0b7..d914c6e28 100644 --- a/dc3-cli/src/core/context.ts +++ b/dc3-cli/src/core/context.ts @@ -15,7 +15,7 @@ * along with this program. If not, see . */ import { configManager } from './config-manager.js'; -import type { OutputFormat } from '../utils/format.js'; +import { SilentExit, type OutputFormat } from '../utils/format.js'; const OUTPUT_FORMATS: readonly OutputFormat[] = ['json', 'table', 'yaml']; @@ -47,6 +47,14 @@ export async function applyGlobalOptions(opts: { if (opts.profile && opts.profile.trim()) { await configManager.setProfileOverride(opts.profile.trim()); } + // Reject unknown --format values here rather than silently dropping them: + // commander routes --format to the program level even when the subcommand + // also declares it, so detectFormat never sees the raw invalid string. + if (opts.format !== undefined && opts.format !== '' && !OUTPUT_FORMATS.includes(opts.format as OutputFormat)) { + process.stderr.write(`error: unknown format '${opts.format}' (expected json, table, or yaml)\n`); + process.exitCode = 1; + throw new SilentExit(); + } const settings = await configManager.getSettings(); globalFormat = parseFormat(opts.format); settingsFormat = parseFormat(settings.output_format); diff --git a/dc3-cli/src/core/errors.ts b/dc3-cli/src/core/errors.ts index 0dfe2934f..6fe08d84d 100644 --- a/dc3-cli/src/core/errors.ts +++ b/dc3-cli/src/core/errors.ts @@ -63,11 +63,15 @@ export class ApiError extends Error { */ export function handleFatalError(err: Error): never { process.stderr.write(`Error: ${err.message}\n`); + // Set the exit code and throw instead of process.exit(): the abrupt exit + // crashes Node on Windows with a libuv assertion while fetch keep-alive + // handles are still open. The entry point's catch unwinds cleanly. if (err instanceof AuthError) { - process.exit(3); + process.exitCode = 3; + } else if (err instanceof NetworkError) { + process.exitCode = 2; + } else { + process.exitCode = 1; } - if (err instanceof NetworkError) { - process.exit(2); - } - process.exit(1); + throw err; } diff --git a/dc3-cli/src/index.ts b/dc3-cli/src/index.ts index 4171e61a9..b77792c10 100644 --- a/dc3-cli/src/index.ts +++ b/dc3-cli/src/index.ts @@ -16,6 +16,7 @@ */ import { Command } from 'commander'; import { handleFatalError } from './core/errors.js'; +import { SilentExit } from './utils/format.js'; import { applyGlobalOptions } from './core/context.js'; import { registerConfigCommand } from './commands/config.js'; import { registerAuthCommand } from './commands/auth.js'; @@ -77,5 +78,15 @@ registerProviderCommand(program); registerModelCommand(program); registerAttachmentCommand(program); -// Parse and map fatal errors to the contract exit codes (auth 3, network 2, business 1) -program.parseAsync(process.argv).catch(handleFatalError); +// Parse and map fatal errors to the contract exit codes (auth 3, network 2, business 1). +// SilentExit is the control-flow signal from printAndExit — the exit code is already +// set on process.exitCode, so just terminate without printing an error. +program.parseAsync(process.argv).catch((error: unknown) => { + if (error instanceof SilentExit) return; + try { + handleFatalError(error instanceof Error ? error : new Error(String(error))); + } catch { + // handleFatalError printed the message and set process.exitCode already; + // swallow the re-thrown error so the process unwinds cleanly. + } +}); diff --git a/dc3-cli/src/utils/format.ts b/dc3-cli/src/utils/format.ts index c4ac73137..dfa2aa550 100644 --- a/dc3-cli/src/utils/format.ts +++ b/dc3-cli/src/utils/format.ts @@ -115,8 +115,15 @@ export function formatOutput(data: unknown, format: OutputFormat = 'json'): stri * @returns the format to render with */ export function detectFormat(explicit?: string): OutputFormat { - if (explicit === 'json' || explicit === 'table' || explicit === 'yaml') { - return explicit; + if (explicit !== undefined && explicit !== '') { + if (explicit === 'json' || explicit === 'table' || explicit === 'yaml') { + return explicit; + } + // Reject unknown formats instead of silently falling back — the user + // explicitly asked for something the CLI does not support. + process.stderr.write(`error: unknown format '${explicit}' (expected json, table, or yaml)\n`); + process.exitCode = 1; + throw new SilentExit(); } const globalOverride = getGlobalFormatOverride(); if (globalOverride) { @@ -138,5 +145,17 @@ export function detectFormat(explicit?: string): OutputFormat { export function printAndExit(data: unknown, format: OutputFormat = 'json', exitCode = 0): never { const output = formatOutput(data, format); if (output) process.stdout.write(output + '\n'); - process.exit(exitCode); + // Use exitCode + throw instead of process.exit(): the abrupt exit crashes + // Node on Windows with a libuv assertion when fetch keep-alive handles are + // still open. Setting the code and unwinding lets the runtime close them. + process.exitCode = exitCode; + throw new SilentExit(); +} + +/** Control-flow signal: unwinds to the top-level catch which exits cleanly. */ +export class SilentExit extends Error { + constructor() { + super('silent exit'); + this.name = 'SilentExit'; + } } diff --git a/dc3-cli/test/agentic-manage.test.ts b/dc3-cli/test/agentic-manage.test.ts index d465e0f8d..7acdda9da 100644 --- a/dc3-cli/test/agentic-manage.test.ts +++ b/dc3-cli/test/agentic-manage.test.ts @@ -65,7 +65,7 @@ const run = async (args: string[]): Promise => { vi.spyOn(process, 'exit').mockImplementation((() => undefined) as never); vi.spyOn(process.stdout, 'write').mockImplementation(() => true); try { - await program.parseAsync(args, { from: 'user' }); + try { await program.parseAsync(args, { from: 'user' }); } catch (e) { if (e?.name !== 'SilentExit') throw e; } } finally { (process.exit as unknown as ReturnType).mockRestore?.(); (process.stdout.write as ReturnType).mockRestore(); @@ -133,6 +133,10 @@ describe('provider command', () => { vi.spyOn(process.stdout, 'write').mockImplementation(() => true); try { await program.parseAsync(['provider', 'check', '--format', 'json'], { from: 'user' }); + expect.unreachable('should have thrown SilentExit'); + } catch (error) { + expect((error as Error).name).toBe('SilentExit'); + expect(process.exitCode).toBe(1); } finally { (process.stdout.write as ReturnType).mockRestore(); } diff --git a/dc3-cli/test/analytics-alert.test.ts b/dc3-cli/test/analytics-alert.test.ts index 3ef28767e..090cad1b0 100644 --- a/dc3-cli/test/analytics-alert.test.ts +++ b/dc3-cli/test/analytics-alert.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { SilentExit } from '../src/utils/format.js'; const fetchCalls: Array<{ url: string; init: RequestInit }> = []; @@ -39,7 +40,7 @@ async function run(args: string[]): Promise { }); vi.spyOn(process, 'exit').mockImplementation(((code?: number) => undefined) as never); try { - await program.parseAsync(args, { from: 'user' }); + try { await program.parseAsync(args, { from: 'user' }); } catch (e) { if (!(e instanceof SilentExit)) throw e; } } finally { (process.stdout.write as ReturnType).mockRestore(); } diff --git a/dc3-cli/test/config.test.ts b/dc3-cli/test/config.test.ts index a5f7b9010..cff918bda 100644 --- a/dc3-cli/test/config.test.ts +++ b/dc3-cli/test/config.test.ts @@ -1,3 +1,4 @@ +import { SilentExit } from '../src/utils/format.js'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { existsSync } from 'node:fs'; import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; @@ -157,7 +158,17 @@ describe('config file corruption handling', () => { let code = 0; try { - await program.parseAsync(['config', 'set', key, value], { from: 'user' }); + try { + await program.parseAsync(['config', 'set', key, value], { from: 'user' }); + } catch (e) { + if (e instanceof ExitSignal) { + code = e.code; + } else if (e?.name === 'SilentExit') { + code = process.exitCode ?? 0; + } else { + throw e; + } + } } catch (error) { if (error instanceof ExitSignal) { code = error.code; diff --git a/dc3-cli/test/contract.test.ts b/dc3-cli/test/contract.test.ts index 90038751c..43f1e0d7b 100644 --- a/dc3-cli/test/contract.test.ts +++ b/dc3-cli/test/contract.test.ts @@ -34,14 +34,13 @@ function setTty(value: boolean | undefined): void { /** Run handleFatalError and return the exit code it selected. */ function exitCodeFor(err: Error): number { + process.exitCode = 0; try { handleFatalError(err); - } catch (thrown) { - if (thrown instanceof ExitSignal) { - return thrown.code; - } - throw thrown; + } catch { + // handleFatalError prints, sets process.exitCode, and re-throws; read the code } + return process.exitCode ?? 1; throw new Error('handleFatalError returned without exiting'); } @@ -92,10 +91,11 @@ describe('detectFormat resolution chain', () => { expect(detectFormat()).toBe('json'); }); - it('invalid format values are ignored at every level', async () => { + it('invalid format values are rejected with a stderr error', async () => { (configManager.getSettings as ReturnType).mockResolvedValue({}); - await applyGlobalOptions({ format: 'xml' }); - expect(detectFormat('xml')).toBe('json'); + vi.spyOn(process.stderr, 'write').mockImplementation(() => true); + await expect(applyGlobalOptions({ format: 'xml' })).rejects.toThrow(); + (process.stderr.write as ReturnType).mockRestore(); }); it('applyGlobalOptions routes --profile to the validated profile override', async () => { diff --git a/dc3-cli/test/device.test.ts b/dc3-cli/test/device.test.ts index 356aafd1c..80e8c50cf 100644 --- a/dc3-cli/test/device.test.ts +++ b/dc3-cli/test/device.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { SilentExit } from '../src/utils/format.js'; import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -42,6 +43,8 @@ async function run(args: string[]): Promise { vi.spyOn(process, 'exit').mockImplementation((() => undefined) as never); try { await program.parseAsync(args, {from: 'user'}); + } catch (error) { + if (!(error instanceof SilentExit)) throw error; } finally { (process.stdout.write as ReturnType).mockRestore(); } diff --git a/dc3-cli/test/manager-write.test.ts b/dc3-cli/test/manager-write.test.ts index 15f768ed0..79da497cf 100644 --- a/dc3-cli/test/manager-write.test.ts +++ b/dc3-cli/test/manager-write.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { SilentExit } from '../src/utils/format.js'; import { Command } from "commander"; import { registerCommandCommand } from "../src/commands/command.js"; import { registerDeviceCommand } from "../src/commands/device.js"; @@ -106,7 +107,7 @@ async function run(register: Register, args: string[]): Promise { return true; }); vi.spyOn(process, "exit").mockImplementation((() => undefined) as never); - await program.parseAsync(args, { from: "user" }); + try { await program.parseAsync(args, { from: "user" }); } catch (e) { if (!(e instanceof SilentExit)) throw e; } return output; } diff --git a/dc3-cli/test/session.test.ts b/dc3-cli/test/session.test.ts index 9a5692651..9fd1613c9 100644 --- a/dc3-cli/test/session.test.ts +++ b/dc3-cli/test/session.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { SilentExit } from '../src/utils/format.js'; const fetchCalls: Array<{ url: string; init: RequestInit }> = []; @@ -38,7 +39,7 @@ async function run(args: string[]): Promise { }); vi.spyOn(process, 'exit').mockImplementation(((code?: number) => undefined) as never); try { - await program.parseAsync(args, { from: 'user' }); + try { await program.parseAsync(args, { from: 'user' }); } catch (e) { if (!(e instanceof SilentExit)) throw e; } } finally { (process.stdout.write as ReturnType).mockRestore(); }