diff --git a/src/cli-error-report.test.ts b/src/cli-error-report.test.ts index cf61ee23..b448bd79 100644 --- a/src/cli-error-report.test.ts +++ b/src/cli-error-report.test.ts @@ -1,6 +1,8 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import yaml from 'js-yaml'; -import { reportCliError } from './cli.js'; +import { CommanderError } from 'commander'; +import { createProgram, handleProgramParseError, reportCliError } from './cli.js'; +import { applyUnknownOptionContract, CommanderStructuralError } from './command-surface.js'; import { CliError, EXIT_CODES } from './errors.js'; describe('reportCliError', () => { @@ -61,3 +63,44 @@ describe('reportCliError', () => { } }); }); + +describe('handleProgramParseError', () => { + const previousExitCode = process.exitCode; + afterEach(() => { process.exitCode = previousExitCode; }); + + it('keeps Commander help and version as display exits, not envelopes', () => { + let text = ''; + const stderr = { write: (chunk: string) => { text += chunk; return true; } } as unknown as NodeJS.WritableStream; + handleProgramParseError(new CommanderError(0, 'commander.helpDisplayed', '(outputHelp)'), stderr); + expect(text).toBe(''); + expect(process.exitCode).toBe(0); + }); + + it('does not wrap web --help as an unknown error after the unknown-option contract is applied', async () => { + const program = createProgram('', ''); + applyUnknownOptionContract(program); + const stdout: string[] = []; + const log = vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { stdout.push(args.map(String).join(' ')); }); + let stderr = ''; + try { + await program.parseAsync(['node', 'webcmd', 'web', '--help']); + expect.unreachable(); + } catch (err) { + handleProgramParseError(err, { write: (chunk: string) => { stderr += chunk; return true; } } as unknown as NodeJS.WritableStream); + expect(err).toBeInstanceOf(CommanderError); + expect((err as CommanderError).code).toBe('commander.helpDisplayed'); + expect(process.exitCode).toBe(0); + expect(stderr).toBe(''); + } finally { + log.mockRestore(); + } + }); + + it('writes unknown-option structural errors to stderr', () => { + let text = ''; + const stderr = { write: (chunk: string) => { text += chunk; return true; } } as unknown as NodeJS.WritableStream; + handleProgramParseError(new CommanderStructuralError("error: unknown option '--foo'\nhelp: valid flags for `webcmd list`: -f\n", 2), stderr); + expect(text).toContain("error: unknown option '--foo'"); + expect(process.exitCode).toBe(2); + }); +}); diff --git a/src/cli.ts b/src/cli.ts index 2f6f76ec..ac826330 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -10,7 +10,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import * as readline from 'node:readline/promises'; import { fileURLToPath, pathToFileURL } from 'node:url'; -import { Command, Option } from 'commander'; +import { Command, CommanderError, Option } from 'commander'; import { findPackageRoot, getBuiltEntryCandidates } from './package-paths.js'; import { type CliCommand, getRegistry } from './registry.js'; // Side-effect import: registers client-owned `web fetch` in the core registry @@ -18,7 +18,7 @@ import { type CliCommand, getRegistry } from './registry.js'; import './fetch/command.js'; import { commandListPresentation, filterCommandsByTag, toPresentableCommand } from './command-presentation.js'; import { configureCompletionCommandSurface, configureListCommandSurface, configurePluginInstallSurface, configurePluginListSurface, configurePluginSearchSurface } from './builtin-command-surface.js'; -import { OUTPUT_FORMAT_HELP, resolveOutputFormat } from './command-surface.js'; +import { applyUnknownOptionContract, CommanderStructuralError, OUTPUT_FORMAT_HELP, resolveOutputFormat } from './command-surface.js'; import { render as renderOutput, formatErrorEnvelope, errorEnvelopeFormat, requestedFormatFromArgv } from './output.js'; import { PKG_VERSION } from './version.js'; import { printCompletionScript } from './completion.js'; @@ -2188,13 +2188,34 @@ export async function loadAntigravityServe(pluginsDir: string = PLUGINS_DIR): Pr * lost. `parseAsync` lets the rejection reach this catch. */ export async function runCli(BUILTIN_CLIS: string, USER_CLIS: string): Promise { + const program = createProgram(BUILTIN_CLIS, USER_CLIS); + applyUnknownOptionContract(program); try { - await createProgram(BUILTIN_CLIS, USER_CLIS).parseAsync(); + await program.parseAsync(); } catch (err) { - reportCliError(err); + handleProgramParseError(err); } } +const COMMANDER_DISPLAY_CODES = new Set([ + 'commander.help', + 'commander.helpDisplayed', + 'commander.version', +]); + +export function handleProgramParseError(err: unknown, stderr: NodeJS.WritableStream = process.stderr): void { + if (err instanceof CommanderStructuralError) { + stderr.write(err.output); + process.exitCode = err.exitCode; + return; + } + if (err instanceof CommanderError && COMMANDER_DISPLAY_CODES.has(err.code)) { + process.exitCode = err.exitCode; + return; + } + reportCliError(err, stderr); +} + /** Render a thrown error as the shared envelope and set the exit code it carries. */ export function reportCliError(err: unknown, stderr: NodeJS.WritableStream = process.stderr): void { const envelope = toEnvelope(err); diff --git a/src/command-surface.test.ts b/src/command-surface.test.ts index 6e3131a8..3a0142ef 100644 --- a/src/command-surface.test.ts +++ b/src/command-surface.test.ts @@ -1,11 +1,12 @@ import { describe, expect, it } from 'vitest'; -import { Command } from 'commander'; +import { Command, CommanderError } from 'commander'; import { CommanderStructuralError, coerceCommandArguments, configureCommandSurface, parseCommandSurface, parseOutputFormat, + structuralErrorFromCommander, type CommandSurfaceMetadata, type OutputFormat, type TraceMode, @@ -202,6 +203,23 @@ describe('configureCommandSurface', () => { }); }); +describe('unknown option contract', () => { + it('enumerates the command flags and exits 2', () => { + try { + parseCommandSurface(metadata, ['needle', '--unknown']); + expect.unreachable(); + } catch (err) { + expect(err).toBeInstanceOf(CommanderStructuralError); + const error = err as CommanderStructuralError; + expect(error.exitCode).toBe(2); + expect(error.output).toContain("error: unknown option '--unknown'"); + expect(error.output).toContain('help: valid flags for `webcmd demo search`:'); + expect(error.output).toContain('--format'); + expect(error.output).toContain('--trace'); + } + }); +}); + type ParseOutcome = | { kind: 'success'; args: Record } | { kind: 'unknown' | 'missing' | 'invalid' }; @@ -411,8 +429,12 @@ function captureReferenceSurface(argv: string[]): ExactSurfaceOutcome { root.parse([precedenceSurface.site, precedenceSurface.name, ...argv], { from: 'user' }); return outcome ?? { kind: 'success', args: {}, format: 'table', trace: 'off' }; } catch (error) { + if (error instanceof CommanderError && error.code === 'commander.helpDisplayed') return { kind: 'help' }; + if (error instanceof CommanderError) { + const structural = structuralErrorFromCommander(error, command, stderr); + return { kind: 'structural', stderr: structural.output, exitCode: structural.exitCode }; + } const commander = error as { code?: string; exitCode?: number }; - if (commander.code === 'commander.helpDisplayed') return { kind: 'help' }; return { kind: 'structural', stderr, exitCode: commander.exitCode ?? 1 }; } } diff --git a/src/command-surface.ts b/src/command-surface.ts index 75e93734..94e4998d 100644 --- a/src/command-surface.ts +++ b/src/command-surface.ts @@ -1,4 +1,4 @@ -import { Command } from 'commander'; +import { Command, CommanderError } from 'commander'; import { ArgumentError, CliError, EXIT_CODES } from './errors.js'; import type { Arg, CliCommand, CommandArgs } from './registry.js'; @@ -57,6 +57,70 @@ export class CommanderStructuralError extends Error { } } +export function visibleCommandFlags(command: Command): string[] { + const flags: string[] = []; + const seen = new Set(); + for (const option of command.options) { + for (const flag of [option.short, option.long]) { + if (!flag || seen.has(flag)) continue; + seen.add(flag); + flags.push(flag); + } + } + return flags; +} + +export function commandInvocationPath(command: Command): string { + const names: string[] = []; + for (let current: Command | null = command; current; current = current.parent) { + const name = current.name(); + if (name) names.unshift(name); + } + return names.join(' '); +} + +export function formatUnknownOptionError(err: CommanderError, command: Command): string { + const flags = visibleCommandFlags(command); + const path = commandInvocationPath(command); + const help = flags.length > 0 ? `help: valid flags for \`${path}\`: ${flags.join(', ')}\n` : ''; + const message = err.message.replace(/^error:\s*/i, ''); + return `error: ${message}\n${help}`; +} + +export function structuralErrorFromCommander( + error: CommanderError, + command: Command, + capturedStderr = '', +): CommanderStructuralError { + if (error.code === 'commander.unknownOption') { + return new CommanderStructuralError(formatUnknownOptionError(error, command), EXIT_CODES.USAGE_ERROR); + } + return new CommanderStructuralError(capturedStderr || `${error.message}\n`, error.exitCode); +} + +/** Walk argv to the leaf command Commander would have been parsing. */ +export function applyUnknownOptionContract(command: Command): void { + command.exitOverride((err) => { + if (err.code === 'commander.unknownOption') { + throw structuralErrorFromCommander(err, command); + } + throw err; + }); + for (const child of command.commands) applyUnknownOptionContract(child); +} + +export function resolveCommandFromArgv(root: Command, argv: readonly string[]): Command { + let current = root; + for (const token of argv) { + if (token === '--') break; + if (token.startsWith('-')) continue; + const child = current.commands.find(candidate => candidate.name() === token || candidate.aliases().includes(token)); + if (!child) break; + current = child; + } + return current; +} + /** Register the adapter argument grammar and its shared execution options. */ export function configureCommandSurface(command: Command, metadata: CommandSurfaceMetadata): void { for (const arg of metadata.args) { @@ -147,6 +211,9 @@ export function parseCommandSurface( help: true, }; } + if (error instanceof CommanderError) { + throw structuralErrorFromCommander(error, command, stderr); + } const output = stderr || `${typeof commander.message === 'string' ? commander.message : String(error)}\n`; throw new CommanderStructuralError( output, diff --git a/src/hosted/root-command-surface.test.ts b/src/hosted/root-command-surface.test.ts index a3fe4686..5c0e8f27 100644 --- a/src/hosted/root-command-surface.test.ts +++ b/src/hosted/root-command-surface.test.ts @@ -2,6 +2,7 @@ import { Writable } from 'node:stream'; import { Command, CommanderError } from 'commander'; import { describe, expect, it, vi } from 'vitest'; import { createProgram } from '../cli.js'; +import { applyUnknownOptionContract, CommanderStructuralError } from '../command-surface.js'; import { formatRootHelp } from '../command-presentation.js'; import { HOSTED_ROOT_HELP } from '../completion-shared.js'; import { CliError } from '../errors.js'; @@ -107,10 +108,14 @@ async function runActualLocalRoot(argv: string[]): Promise { for (const child of command.commands) configureTree(child); }; configureTree(program); + applyUnknownOptionContract(program); try { await program.parseAsync(argv, { from: 'user' }); return { exitCode: Number(process.exitCode ?? 0), stdout, stderr }; } catch (error) { + if (error instanceof CommanderStructuralError) { + return { exitCode: error.exitCode, stdout, stderr: error.output, errorCode: 'commander.unknownOption' }; + } if (error instanceof CommanderError) { return { exitCode: error.exitCode, stdout, stderr, errorCode: error.code }; } @@ -251,10 +256,29 @@ describe('hosted root command surface', () => { { name: 'trailing missing profile beats malformed prefix', argv: ['--unknown', 'github', '--profile'], stderr: "error: option '--profile ' argument missing\n", code: 'commander.optionMissingArgument' }, ])('matches actual local Commander root error bytes: $name', async ({ argv, stderr, code }) => { const local = await runActualLocalRoot(argv); - expect(local).toMatchObject({ exitCode: 1, stderr, errorCode: code }); - expect(() => parseHostedRootCommandSurface(argv)).toThrowError( - expect.objectContaining({ output: stderr, exitCode: 1 }), - ); + const unknown = code === 'commander.unknownOption'; + expect(local.exitCode).toBe(unknown ? 2 : 1); + expect(local.errorCode).toBe(code); + if (unknown) { + expect(local.stderr.startsWith(stderr)).toBe(true); + expect(local.stderr).toContain('help: valid flags for'); + } else { + expect(local.stderr).toBe(stderr); + } + try { + parseHostedRootCommandSurface(argv); + expect.unreachable(); + } catch (err) { + expect(err).toBeInstanceOf(CommanderStructuralError); + const structural = err as CommanderStructuralError; + expect(structural.exitCode).toBe(local.exitCode); + if (unknown) { + expect(structural.output.startsWith(stderr)).toBe(true); + expect(structural.output).toContain('help: valid flags for'); + } else { + expect(structural.output).toBe(stderr); + } + } }); it.each([ @@ -361,8 +385,8 @@ describe('hosted root preflight call order', () => { { name: 'missing profile', argv: ['--profile'], exitCode: 1, stdout: '', stderr: "error: option '--profile ' argument missing\n" }, { name: 'trailing missing profile beats help', argv: ['--help', 'github', '--profile'], exitCode: 1, stdout: '', stderr: "error: option '--profile ' argument missing\n" }, { name: 'trailing missing profile beats unknown', argv: ['--unknown', 'github', '--profile'], exitCode: 1, stdout: '', stderr: "error: option '--profile ' argument missing\n" }, - { name: 'unknown option', argv: ['-xV'], exitCode: 1, stdout: '', stderr: "error: unknown option '-xV'\n" }, - ])('$name terminates before Cloud discovery', async ({ argv, exitCode, stdout: expectedStdout, stderr: expectedStderr }) => { + { name: 'unknown option', argv: ['-xV'], exitCode: 2, stdout: '', stderr: "error: unknown option '-xV'\n" }, + ])('$name terminates before Cloud discovery', async ({ name, argv, exitCode, stdout: expectedStdout, stderr: expectedStderr }) => { const stdout = sink(); const stderr = sink(); const fetchImpl = vi.fn(); @@ -376,7 +400,12 @@ describe('hosted root preflight call order', () => { expect(result).toEqual({ handled: true, exitCode }); expect(stdout.text()).toBe(expectedStdout); - expect(stderr.text()).toBe(expectedStderr); + if (name === 'unknown option') { + expect(stderr.text().startsWith(expectedStderr)).toBe(true); + expect(stderr.text()).toContain('help: valid flags for'); + } else { + expect(stderr.text()).toBe(expectedStderr); + } expect(fetchImpl).not.toHaveBeenCalled(); }); diff --git a/src/hosted/runner.test.ts b/src/hosted/runner.test.ts index 59469e47..156c3368 100644 --- a/src/hosted/runner.test.ts +++ b/src/hosted/runner.test.ts @@ -1320,7 +1320,7 @@ describe('runHostedCli', () => { { name: 'ordinary unknown option', tail: ['account', '--token', 'secret', '--unknown'], - exitCode: 1, + exitCode: 2, stderr: "error: unknown option '--unknown'\n", help: false, }, @@ -1338,7 +1338,7 @@ describe('runHostedCli', () => { stderr: "error: too many arguments for 'whoami'. Expected 1 argument but got 2.\n", help: false, }, - ])('matches public Commander structural bytes and discovery order: $name', async ({ tail, exitCode, stderr: expectedStderr, help }) => { + ])('matches public Commander structural bytes and discovery order: $name', async ({ name, tail, exitCode, stderr: expectedStderr, help }) => { const structuralManifest = manifestWithStructuralArguments(); const stdout = sink(); const stderr = sink(); @@ -1355,7 +1355,12 @@ describe('runHostedCli', () => { }); expect(result).toEqual({ handled: true, exitCode }); - expect(stderr.text()).toBe(expectedStderr); + if (name === 'ordinary unknown option') { + expect(stderr.text().startsWith(expectedStderr)).toBe(true); + expect(stderr.text()).toContain('help: valid flags for'); + } else { + expect(stderr.text()).toBe(expectedStderr); + } if (help) expect(stdout.text()).toContain('Usage: webcmd github whoami [options]'); else expect(stdout.text()).toBe(''); expect(fetchImpl).toHaveBeenCalledTimes(1); @@ -1444,8 +1449,9 @@ describe('runHostedCli', () => { fetchImpl, }); - expect(result).toEqual({ handled: true, exitCode: 1 }); - expect(stderr.text()).toBe("error: unknown option '--profile'\n"); + expect(result).toEqual({ handled: true, exitCode: 2 }); + expect(stderr.text().startsWith("error: unknown option '--profile'\n")).toBe(true); + expect(stderr.text()).toContain('help: valid flags for'); expect(stdout.text()).toBe(''); expect(fetchImpl).toHaveBeenCalledTimes(1); }); @@ -1462,8 +1468,9 @@ describe('runHostedCli', () => { fetchImpl, }); - expect(result).toEqual({ handled: true, exitCode: 1 }); - expect(stderr.text()).toBe("error: unknown option '-dash'\n"); + expect(result).toEqual({ handled: true, exitCode: 2 }); + expect(stderr.text().startsWith("error: unknown option '-dash'\n")).toBe(true); + expect(stderr.text()).toContain('help: valid flags for'); expect(stdout.text()).toBe(''); expect(fetchImpl).not.toHaveBeenCalled(); }); diff --git a/src/hosted/runner.ts b/src/hosted/runner.ts index d45b0629..c8adca7f 100644 --- a/src/hosted/runner.ts +++ b/src/hosted/runner.ts @@ -13,7 +13,7 @@ import { configurePluginUpdateSurface, } from '../builtin-command-surface.js'; import { BrowserSessionArgvError, rejectMisplacedSessionSelectorArgv, rejectPositionalBrowserSessionArgv } from '../cli-argv-preprocess.js'; -import { CommanderStructuralError, MissingRequiredPositionalError, OUTPUT_FORMAT_HELP, parseOutputFormat } from '../command-surface.js'; +import { CommanderStructuralError, MissingRequiredPositionalError, OUTPUT_FORMAT_HELP, parseOutputFormat, resolveCommandFromArgv, structuralErrorFromCommander } from '../command-surface.js'; import { filterCommandsByTag, formatRootHelp, getCommandCompletionCandidates } from '../command-presentation.js'; import { HOSTED_BUILTIN_COMMANDS, @@ -508,7 +508,7 @@ async function runHostedSiteSurface(argv: readonly string[], literal: boolean, c await writeToStream(stdout, help); return; } - if (error instanceof CommanderError) throw new CommanderStructuralError(stderr || `${error.message}\n`, error.exitCode); + if (error instanceof CommanderError) throw structuralErrorFromCommander(error, resolveCommandFromArgv(root, ['site', ...argv]), stderr); throw error; } } @@ -561,7 +561,7 @@ async function runHostedAdapterSourceSurface(argv: readonly string[], literal: b await writeToStream(stdout, help); return; } - if (error instanceof CommanderError) throw new CommanderStructuralError(stderr || `${error.message}\n`, error.exitCode); + if (error instanceof CommanderError) throw structuralErrorFromCommander(error, resolveCommandFromArgv(root, ['adapter', ...argv]), stderr); throw error; } if (!parsed) throw new CommanderStructuralError("error: command 'adapter' did not run\n", EXIT_CODES.USAGE_ERROR); @@ -660,7 +660,7 @@ function parseHostedSessionSurface(argv: readonly string[], literal: boolean): P } catch (error) { if (!(error instanceof CommanderError)) throw error; if (error.code === 'commander.helpDisplayed') return { kind: 'help', output: stdout }; - throw new CommanderStructuralError(stderr || `${error.message}\n`, error.exitCode); + throw structuralErrorFromCommander(error, resolveCommandFromArgv(root, ['session', ...argv]), stderr); } if (!parsed) throw new CommanderStructuralError("error: command 'session' did not run\n", 1); return parsed; @@ -1137,7 +1137,7 @@ function parseHostedListSurface(argv: readonly string[], literal: boolean): Pars } catch (error) { if (!(error instanceof CommanderError)) throw error; if (error.code === 'commander.helpDisplayed') return { kind: 'help', output: stdout }; - throw new CommanderStructuralError(stderr || `${error.message}\n`, error.exitCode); + throw structuralErrorFromCommander(error, resolveCommandFromArgv(root, ['list', ...argv]), stderr); } if (!actionRan) throw new CommanderStructuralError("error: command 'list' did not run\n", 1); return { kind: 'run', format: parsedFormat, formatExplicit, ...(parsedTag !== undefined ? { tag: parsedTag } : {}) }; @@ -1198,7 +1198,7 @@ function parseHostedProfileSurface( } catch (error) { if (!(error instanceof CommanderError)) throw error; if (error.code === 'commander.helpDisplayed') return { kind: 'help', output: stdout }; - throw new CommanderStructuralError(stderr || `${error.message}\n`, error.exitCode); + throw structuralErrorFromCommander(error, resolveCommandFromArgv(root, ['profile', ...argv]), stderr); } if (!parsed) { throw new CommanderStructuralError("error: command 'profile' did not run\n", 1); @@ -1295,7 +1295,7 @@ function parseHostedPluginSurface( } catch (error) { if (!(error instanceof CommanderError)) throw error; if (error.code === 'commander.helpDisplayed') return { kind: 'help', output: stdout }; - throw new CommanderStructuralError(stderr || `${error.message}\n`, error.exitCode); + throw structuralErrorFromCommander(error, resolveCommandFromArgv(root, ['plugin', ...argv]), stderr); } if (!parsed) throw new CommanderStructuralError("error: command 'plugin' did not run\n", 1); return parsed; @@ -1328,7 +1328,7 @@ function parseHostedCompletionSurface( } catch (error) { if (!(error instanceof CommanderError)) throw error; if (error.code === 'commander.helpDisplayed') return { kind: 'help', output: stdout }; - throw new CommanderStructuralError(stderr || `${error.message}\n`, error.exitCode); + throw structuralErrorFromCommander(error, resolveCommandFromArgv(root, ['completion', ...argv]), stderr); } if (shell === undefined) { throw new CommanderStructuralError("error: missing required argument 'shell'\n", 1); diff --git a/src/root-command-surface.ts b/src/root-command-surface.ts index cfa72429..b312644f 100644 --- a/src/root-command-surface.ts +++ b/src/root-command-surface.ts @@ -1,5 +1,5 @@ import { Command, CommanderError } from 'commander'; -import { CommanderStructuralError } from './command-surface.js'; +import { CommanderStructuralError, structuralErrorFromCommander } from './command-surface.js'; import { PKG_VERSION } from './version.js'; export const ROOT_PROFILE_FLAGS = '--profile '; @@ -86,7 +86,7 @@ export function parseHostedRootCommandSurface(argv: readonly string[]): HostedRo if (error.code === 'commander.version') { return { kind: 'version', output: stdout || `${PKG_VERSION}\n` }; } - throw new CommanderStructuralError(stderr || `${error.message}\n`, error.exitCode); + throw structuralErrorFromCommander(error, root, stderr); } const { profile, session } = root.opts<{ profile?: string; session?: string }>();