From 370d76f85a115fbc6dd43a298e9a4f2a79a7efc8 Mon Sep 17 00:00:00 2001 From: Agnik47 <140933190+Agnik47@users.noreply.github.com> Date: Tue, 25 Aug 2026 21:55:27 +0530 Subject: [PATCH] fix(cli): an adapter arg named json must not crash every command (#441) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `configureCommandSurface` registers an adapter's own arguments first, then added the shared options unconditionally. Commander throws on a duplicate flag, and this runs while the CLI is being built, so a single adapter argument named `json` aborted startup for *every* command — `list`, `doctor`, and the `plugin uninstall` needed to remove the offending plugin, leaving no recovery path through the CLI. The official LinkedIn plugin ships such an adapter (`thread-snapshot`), so installing it bricked webcmd. Guard every shared option the adapter path registers — `--format`, `--json`, `--trace`, `-v/--verbose`, and the browser trio — the way `ensureOutputFormatOptions` in the same file already guarded its own, and reuse one helper for both so the two paths cannot drift again. An adapter that names a flag keeps it; webcmd drops its own rather than refusing to run. Format resolution has to agree about who owns a shadowed `--json`, or the flag would silently do two things: set the adapter's argument *and* switch the output format. Argv preprocessing already resolves this collision in the adapter's favour, so `outputFormatIsExplicit`/`requestedOutputFormat` now honour `--json` as the format alias only on commands where webcmd registered it. `-f json` is unaffected and remains the way to ask for JSON output there. Help follows the same rule: a shared option the adapter shadows is no longer listed under "Common options", in both the text and structured renderings, since advertising it would name a flag that is not registered and show the same flag twice with two different meanings. --- src/command-presentation.test.ts | 39 +++++++++++++ src/command-presentation.ts | 62 ++++++++++++++------ src/command-surface.test.ts | 62 ++++++++++++++++++++ src/command-surface.ts | 98 +++++++++++++++++++++++++------- src/commanderAdapter.test.ts | 43 ++++++++++++++ 5 files changed, 265 insertions(+), 39 deletions(-) diff --git a/src/command-presentation.test.ts b/src/command-presentation.test.ts index d5f3e83a..46fb8a5b 100644 --- a/src/command-presentation.test.ts +++ b/src/command-presentation.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { Strategy, type CliCommand } from './registry.js'; import { + commandHelpData, commandListPresentation, commandListRows, filterCommandsByTag, @@ -167,3 +168,41 @@ describe('shared command presentation', () => { .toEqual(['issue-list', 'issues']); }); }); + +describe('help for a command that shadows a shared flag', () => { + const shadowing = toPresentableCommand({ + site: 'demo', + name: 'snapshot', + access: 'read', + description: 'Snapshot a thread', + browser: false, + args: [{ name: 'json', type: 'bool', default: false, help: 'Return only the snapshot string' }], + }); + + it('lists the shadowed flag once, with the adapter’s meaning', () => { + const help = formatCommandHelp(shadowing); + expect(help).toContain('Return only the snapshot string'); + // Advertising the alias would name a flag that is no longer registered. + expect(help).not.toContain('Alias of --format json'); + }); + + it('still lists the other shared options', () => { + const help = formatCommandHelp(shadowing); + expect(help).toContain('Common options:'); + expect(help).toContain('-f, --format '); + }); + + it('omits the shadowed flag from structured help too', () => { + const data = commandHelpData(shadowing) as { common_options: Array<{ name: string }> }; + expect(data.common_options.map((option) => option.name)).not.toContain('json'); + expect(data.common_options.map((option) => option.name)).toContain('format'); + }); + + it('leaves a command that shadows nothing unchanged', () => { + const plain = toPresentableCommand({ + site: 'demo', name: 'search', access: 'read', description: 'Search', + browser: false, args: [{ name: 'limit', type: 'int', default: 10 }], + }); + expect(formatCommandHelp(plain)).toContain('Alias of --format json'); + }); +}); diff --git a/src/command-presentation.ts b/src/command-presentation.ts index b0c0b17c..8630db20 100644 --- a/src/command-presentation.ts +++ b/src/command-presentation.ts @@ -406,31 +406,55 @@ export function siteHelpData(site: string, commands: readonly PresentableCommand } export function commandHelpData(command: PresentableCommand): Record { + const shadowed = shadowedCommonOptions(command); + const unshadowed = (options: readonly T[]): T[] => + options.filter((option) => !shadowed.has(option.name)); return { site: command.site, ...compactCommand(command), - common_options: COMMON_OPTIONS.map(compactCommonOption), - ...(command.browser ? { browser_common_options: BROWSER_COMMON_OPTIONS.map(compactCommonOption) } : {}), + common_options: unshadowed(COMMON_OPTIONS).map(compactCommonOption), + ...(command.browser ? { browser_common_options: unshadowed(BROWSER_COMMON_OPTIONS).map(compactCommonOption) } : {}), output_formats: ['table', 'plain', 'yaml', 'json', 'md', 'csv'], }; } -export function formatCommonOptionsHelp(): string { - const rows = COMMON_OPTIONS.map((option) => { - const details: string[] = [option.help]; - if ('default' in option) details.push(`default: ${option.default}`); - if ('choices' in option) details.push(`choices: ${option.choices.join(', ')}`); - return [option.flags, details.join(' ')] as [string, string]; - }); +/** + * Names of shared options this command shadows with an argument of its own. + * + * An adapter argument named after a shared flag keeps that flag — webcmd skips + * registering its own (see `addSharedOption` in command-surface.ts). Help has + * to skip it too, or it advertises a flag that is not registered and lists the + * same flag twice with two different meanings. + */ +function shadowedCommonOptions(command?: PresentableCommand): Set { + if (!command) return new Set(); + return new Set(commandOptions(command).map((arg) => arg.name)); +} + +function formatCommonOptionRows( + options: typeof COMMON_OPTIONS | typeof BROWSER_COMMON_OPTIONS, + command?: PresentableCommand, +): Array<[string, string]> { + const shadowed = shadowedCommonOptions(command); + return options + .filter((option) => !shadowed.has(option.name)) + .map((option) => { + const details: string[] = [option.help]; + if ('default' in option) details.push(`default: ${option.default}`); + if ('choices' in option) details.push(`choices: ${option.choices.join(', ')}`); + return [option.flags, details.join(' ')] as [string, string]; + }); +} + +export function formatCommonOptionsHelp(command?: PresentableCommand): string { + const rows = formatCommonOptionRows(COMMON_OPTIONS, command); + if (rows.length === 0) return ''; return ['Common options:', ...formatRows(rows)].join('\n'); } -export function formatBrowserCommonOptionsHelp(): string { - const rows = BROWSER_COMMON_OPTIONS.map((option) => { - const details: string[] = [option.help]; - if ('choices' in option) details.push(`choices: ${option.choices.join(', ')}`); - return [option.flags, details.join(' ')] as [string, string]; - }); +export function formatBrowserCommonOptionsHelp(command?: PresentableCommand): string { + const rows = formatCommonOptionRows(BROWSER_COMMON_OPTIONS, command); + if (rows.length === 0) return ''; return ['Browser common options:', ...formatRows(rows)].join('\n'); } @@ -473,8 +497,12 @@ export function formatCommandHelp(command: PresentableCommand): string { ] as [string, string]); if (optionRows.length) lines.push('Command options:', ...formatRows(optionRows), ''); - lines.push(formatCommonOptionsHelp(), ''); - if (command.browser) lines.push(formatBrowserCommonOptionsHelp(), ''); + const commonOptionsHelp = formatCommonOptionsHelp(command); + if (commonOptionsHelp) lines.push(commonOptionsHelp, ''); + if (command.browser) { + const browserOptionsHelp = formatBrowserCommonOptionsHelp(command); + if (browserOptionsHelp) lines.push(browserOptionsHelp, ''); + } const meta = [ `Access: ${command.access}`, diff --git a/src/command-surface.test.ts b/src/command-surface.test.ts index 51f33735..f647a999 100644 --- a/src/command-surface.test.ts +++ b/src/command-surface.test.ts @@ -494,3 +494,65 @@ describe('complete Commander structural grammar and precedence parity', () => { expect(captureSharedSurface(argv)).toEqual(captureReferenceSurface(argv)); }); }); + +describe('shared options an adapter shadows', () => { + // linkedin/thread-snapshot ships exactly this: an argument named `json`. + // Registering webcmd's own `--json` alias on top of it made Commander throw + // while the CLI was still being built, so every command crashed at startup + // — including the `plugin uninstall` needed to remove the plugin. + const shadowing = { + command: 'demo/snapshot', + browser: true, + defaultFormat: 'table', + args: [ + { name: 'thread-url', required: true, type: 'string' }, + { name: 'json', type: 'boolean', default: false }, + ], + } satisfies CommandSurfaceMetadata; + + it.each(['json', 'format', 'trace', 'verbose', 'window', 'site-session', 'keep-tab'])( + 'registers a command whose argument is named %s without throwing', + (name) => { + const metadataFor = { + command: `demo/${name}-arg`, + browser: true, + args: [{ name, type: 'boolean', default: false }], + } satisfies CommandSurfaceMetadata; + expect(() => configureCommandSurface(new Command('demo'), metadataFor)).not.toThrow(); + }, + ); + + it('registers every shared option when nothing collides', () => { + const command = new Command('demo'); + configureCommandSurface(command, { command: 'demo/plain', browser: true, args: [] }); + const flags = command.options.map((option) => option.long); + expect(flags).toEqual(expect.arrayContaining([ + '--format', '--json', '--trace', '--verbose', '--window', '--site-session', '--keep-tab', + ])); + }); + + it('keeps the adapter argument rather than webcmd’s alias when they collide', () => { + const command = new Command('demo'); + configureCommandSurface(command, shadowing); + const jsonOptions = command.options.filter((option) => option.long === '--json'); + expect(jsonOptions).toHaveLength(1); + expect(jsonOptions[0]!.description).not.toBe('Alias of --format json'); + }); + + it('does not treat a shadowed --json as a request for JSON output', () => { + // Argv preprocessing already resolves this collision in the adapter's + // favour, so format resolution has to agree or --json means two things. + expect(parseCommandSurface(shadowing, ['--thread-url', 'https://example.com/t/1', '--json'])) + .toMatchObject({ format: 'table', formatExplicit: false }); + }); + + it('passes a shadowed --json through to the adapter', () => { + const parsed = parseCommandSurface(shadowing, ['--thread-url', 'https://example.com/t/1', '--json']); + expect(parsed.args.json).toBe(true); + }); + + it('still honours -f json on a command that shadows --json', () => { + expect(parseCommandSurface(shadowing, ['--thread-url', 'https://example.com/t/1', '-f', 'json'])) + .toMatchObject({ format: 'json', formatExplicit: true }); + }); +}); diff --git a/src/command-surface.ts b/src/command-surface.ts index fb9d949d..12f66620 100644 --- a/src/command-surface.ts +++ b/src/command-surface.ts @@ -1,4 +1,4 @@ -import { Command, CommanderError } from 'commander'; +import { Command, CommanderError, Option } from 'commander'; import { ArgumentError, CliError, EXIT_CODES, type ErrorEnvelope } from './errors.js'; import type { Arg, CliCommand, CommandArgs } from './registry.js'; @@ -225,15 +225,16 @@ export function configureCommandSurface(command: Command, metadata: CommandSurfa else command.option(flag, arg.help ?? ''); } - addOutputFormatOption(command) - .option('--trace ', `Trace capture: ${TRACE_MODES.join(', ')}`, 'off') - .option('-v, --verbose', 'Debug output', false); + // Every shared option below is guarded: the adapter's own arguments are + // registered first, and any of these names may collide with one of them. + addOutputFormatOption(command); + addSharedOption(command, '--trace ', `Trace capture: ${TRACE_MODES.join(', ')}`, 'off'); + addSharedOption(command, '-v, --verbose', 'Debug output', false); if (metadata.browser) { - command - .option('--window ', `Browser window mode: ${BROWSER_WINDOW_MODES.join(' or ')} (default: background)`) - .option('--site-session ', `Adapter site session lifecycle: ${SITE_SESSION_MODES.join(' or ')}`) - .option('--keep-tab ', 'Keep the browser tab lease after the command finishes'); + addSharedOption(command, '--window ', `Browser window mode: ${BROWSER_WINDOW_MODES.join(' or ')} (default: background)`); + addSharedOption(command, '--site-session ', `Adapter site session lifecycle: ${SITE_SESSION_MODES.join(' or ')}`); + addSharedOption(command, '--keep-tab ', 'Keep the browser tab lease after the command finishes'); } } @@ -441,11 +442,64 @@ export function resolveOutputFormat(raw: string | undefined): OutputFormat | nul } } +/** Long and short flags already registered on `command`. */ +function registeredFlags(command: Command): Set { + const flags = new Set(); + for (const option of command.options) { + if (option.short) flags.add(option.short); + if (option.long) flags.add(option.long); + } + return flags; +} + +/** + * Commands where webcmd — not the adapter — owns `--json`. + * + * An adapter may declare an argument named `json`, in which case the flag + * means whatever that adapter says it means and must not be read as + * `--format json`. Argv preprocessing already resolves the collision this way + * (`knownCommandOptions` lets adapter args overwrite the shared entries), so + * format resolution has to agree, or `--json` would silently do two things. + */ +const WEBCMD_OWNS_JSON_ALIAS = new WeakSet(); + +/** + * Add a shared option unless the command already declares one of its flags. + * + * Commander throws on a duplicate flag, and these options are registered + * while the CLI is being built, so one adapter argument named after a shared + * flag used to abort startup for *every* command — including the + * `plugin uninstall` needed to remove the offending plugin. An adapter that + * names a flag keeps it; webcmd drops its own rather than refusing to run. + */ +function addSharedOption( + command: Command, + flags: string, + description: string, + defaultValue?: unknown, +): boolean { + const option = new Option(flags, description); + const taken = registeredFlags(command); + if ((option.short && taken.has(option.short)) || (option.long && taken.has(option.long))) return false; + if (defaultValue !== undefined) option.default(defaultValue); + command.addOption(option); + return true; +} + /** Register `-f/--format` plus the `--json` alias on one command. */ export function addOutputFormatOption(command: Command, defaultFormat = 'table'): Command { - return command - .option('-f, --format ', OUTPUT_FORMAT_HELP, defaultFormat) - .option('--json', JSON_FORMAT_ALIAS_HELP, false); + const taken = registeredFlags(command); + if (!taken.has('--format')) { + command.option( + taken.has('-f') ? '--format ' : '-f, --format ', + OUTPUT_FORMAT_HELP, + defaultFormat, + ); + } + if (addSharedOption(command, '--json', JSON_FORMAT_ALIAS_HELP, false)) { + WEBCMD_OWNS_JSON_ALIAS.add(command); + } + return command; } /** @@ -465,27 +519,27 @@ export function addOutputFormatOption(command: Command, defaultFormat = 'table') export function ensureOutputFormatOptions(command: Command): void { for (const child of command.commands) { if (child.commands.length === 0 && (child as Command & { _allowUnknownOption?: boolean })._allowUnknownOption !== true) { - const flags = new Set(); - for (const option of child.options) { - if (option.short) flags.add(option.short); - if (option.long) flags.add(option.long); - } - if (!flags.has('--format')) { - child.option(flags.has('-f') ? '--format ' : '-f, --format ', OUTPUT_FORMAT_HELP, 'table'); - } - if (!flags.has('--json')) child.option('--json', JSON_FORMAT_ALIAS_HELP, false); + addOutputFormatOption(child); } ensureOutputFormatOptions(child); } } +/** + * True when `--json` on this command is webcmd's format alias rather than an + * adapter argument that happens to be named `json`. + */ +function jsonAliasPassed(command: Command): boolean { + return WEBCMD_OWNS_JSON_ALIAS.has(command) && command.getOptionValueSource('json') === 'cli'; +} + export function outputFormatIsExplicit(command: Command): boolean { - return command.getOptionValueSource('format') === 'cli' || command.getOptionValueSource('json') === 'cli'; + return command.getOptionValueSource('format') === 'cli' || jsonAliasPassed(command); } /** Resolve `--json` onto `--format json` unless `--format` was also passed. */ export function requestedOutputFormat(command: Command, format: unknown): unknown { - return command.getOptionValueSource('json') === 'cli' && command.getOptionValueSource('format') !== 'cli' + return jsonAliasPassed(command) && command.getOptionValueSource('format') !== 'cli' ? 'json' : format; } diff --git a/src/commanderAdapter.test.ts b/src/commanderAdapter.test.ts index bc131782..c110f49e 100644 --- a/src/commanderAdapter.test.ts +++ b/src/commanderAdapter.test.ts @@ -508,3 +508,46 @@ describe('commanderAdapter error envelope output', () => { stderrSpy.mockRestore(); }); }); + +describe('registering an adapter that shadows a shared flag', () => { + // The reported crash: registration happens while the CLI is being built, so + // one plugin command with an argument named `json` aborted startup for every + // command — `list`, `doctor`, and even `plugin uninstall`. + const shadowing: CliCommand = { + site: 'linkedin', + name: 'thread-snapshot', + access: 'read', + description: 'Snapshot a thread', + browser: true, + args: [ + { name: 'thread-url', required: true, help: 'Thread URL' }, + { name: 'json', type: 'bool', default: false, help: 'Return only the snapshot string' }, + ], + func: vi.fn(), + }; + + it('registers without throwing', () => { + const program = new Command(); + const siteCmd = program.command('linkedin'); + expect(() => registerCommandToProgram(siteCmd, shadowing)).not.toThrow(); + }); + + it('leaves the adapter owning the flag', () => { + const program = new Command(); + const siteCmd = program.command('linkedin'); + registerCommandToProgram(siteCmd, shadowing); + const registered = siteCmd.commands.find((child) => child.name() === 'thread-snapshot')!; + const jsonOptions = registered.options.filter((option) => option.long === '--json'); + expect(jsonOptions).toHaveLength(1); + expect(jsonOptions[0]!.description).toBe('Return only the snapshot string'); + }); + + it('does not disturb sibling commands that shadow nothing', () => { + const program = new Command(); + const siteCmd = program.command('linkedin'); + registerCommandToProgram(siteCmd, shadowing); + registerCommandToProgram(siteCmd, { ...shadowing, name: 'timeline', args: [] }); + const timeline = siteCmd.commands.find((child) => child.name() === 'timeline')!; + expect(timeline.options.map((option) => option.long)).toContain('--json'); + }); +});