From 40dd5df665eb2e748688a456ee9ab7b1cc155c8e Mon Sep 17 00:00:00 2001 From: Justin Murray Date: Mon, 16 Mar 2026 10:18:12 -0400 Subject: [PATCH 1/2] fix: positional argument completion broken after flags Three interacting bugs prevented positional argument completion when flags preceded the argument (e.g., 'mycli logs -f '): 1. handlePositionalCompletion received raw args including flags, so its positional index calculation was inflated. Fix: call stripOptions() on previousArgs before counting positions. 2. parse() had an early return after detecting a boolean flag that skipped both handleCommandCompletion and handlePositionalCompletion. Fix: remove the early return to let control fall through. 3. The commander adapter marked ALL options as isBoolean=true, even value-taking ones like '--output '. This caused stripOptions to not skip the value argument, further corrupting the positional index. Fix: detect '<' or '[' in flag syntax and register with a no-op handler to force isBoolean=false. Fixes #110 --- src/commander.ts | 79 ++++++++++++++++++++++------ src/t.ts | 52 ++++++++---------- tests/__snapshots__/cli.test.ts.snap | 33 +++++------- tests/cli.test.ts | 68 +++++++++++++----------- 4 files changed, 137 insertions(+), 95 deletions(-) diff --git a/src/commander.ts b/src/commander.ts index 0001de4..bdd1e7d 100644 --- a/src/commander.ts +++ b/src/commander.ts @@ -1,10 +1,10 @@ -import * as zsh from './zsh'; +import type { Command as CommanderCommand, ParseOptions } from 'commander'; import * as bash from './bash'; import * as fish from './fish'; import * as powershell from './powershell'; -import type { Command as CommanderCommand, ParseOptions } from 'commander'; -import t, { type RootCommand } from './t'; import { assertDoubleDashes } from './shared'; +import t, { type RootCommand } from './t'; +import * as zsh from './zsh'; const execPath = process.execPath; const processArgs = process.argv.slice(1); @@ -60,7 +60,7 @@ export default function tab(instance: CommanderCommand): RootCommand { console.log('Collected commands:'); for (const [path, cmd] of commandMap.entries()) { console.log( - `- ${path || ''}: ${cmd.description() || 'No description'}` + `- ${path || ''}: ${cmd.description() || 'No description'}`, ); } break; @@ -75,7 +75,7 @@ export default function tab(instance: CommanderCommand): RootCommand { // Override the parse method to handle completion requests before normal parsing const originalParse = instance.parse.bind(instance); - instance.parse = function (argv?: readonly string[], options?: ParseOptions) { + instance.parse = (argv?: readonly string[], options?: ParseOptions) => { const args = argv || process.argv; const completeIndex = args.findIndex((arg) => arg === 'complete'); const dashDashIndex = args.findIndex((arg) => arg === '--'); @@ -101,6 +101,55 @@ export default function tab(instance: CommanderCommand): RootCommand { return t; } +/** + * Detect whether a commander option flag expects a value argument. + * Options with `` or `[value]` in their flags are value-taking. + */ +function optionTakesValue(flags: string): boolean { + return flags.includes('<') || flags.includes('['); +} + +/** + * Register a commander option with the tab library, correctly setting + * isBoolean based on whether the option takes a value. + * + * The tab Command.option() method infers isBoolean from the argument types: + * - string arg → alias, isBoolean=true + * - function arg → handler, isBoolean=false + * So for value-taking options with an alias, we pass a no-op handler + * and the alias separately to get isBoolean=false. + */ +function registerOption( + tabCommand: { + option: ( + value: string, + description: string, + handlerOrAlias?: ((...args: unknown[]) => void) | string, + alias?: string, + ) => unknown; + }, + flags: string, + longFlag: string, + description: string, + shortFlag?: string, +): void { + const takesValue = optionTakesValue(flags); + if (shortFlag) { + if (takesValue) { + // Pass a no-op handler to force isBoolean=false, with alias as 4th arg + tabCommand.option(longFlag, description, () => {}, shortFlag); + } else { + tabCommand.option(longFlag, description, shortFlag); + } + } else { + if (takesValue) { + tabCommand.option(longFlag, description, () => {}); + } else { + tabCommand.option(longFlag, description); + } + } +} + function processRootCommand(command: CommanderCommand): void { // Add root command options to the root t instance for (const option of command.options) { @@ -110,11 +159,7 @@ function processRootCommand(command: CommanderCommand): void { const longFlag = flags.match(/--([a-zA-Z0-9-]+)/)?.[1]; if (longFlag) { - if (shortFlag) { - t.option(longFlag, option.description || '', shortFlag); - } else { - t.option(longFlag, option.description || ''); - } + registerOption(t, flags, longFlag, option.description || '', shortFlag); } } } @@ -141,11 +186,13 @@ function processSubcommands(rootCommand: CommanderCommand): void { const longFlag = flags.match(/--([a-zA-Z0-9-]+)/)?.[1]; if (longFlag) { - if (shortFlag) { - command.option(longFlag, option.description || '', shortFlag); - } else { - command.option(longFlag, option.description || ''); - } + registerOption( + command, + flags, + longFlag, + option.description || '', + shortFlag, + ); } } } @@ -154,7 +201,7 @@ function processSubcommands(rootCommand: CommanderCommand): void { function collectCommands( command: CommanderCommand, parentPath: string, - commandMap: Map + commandMap: Map, ): void { // Add this command to the map commandMap.set(parentPath, command); diff --git a/src/t.ts b/src/t.ts index 8dabcaa..09781d8 100644 --- a/src/t.ts +++ b/src/t.ts @@ -17,7 +17,7 @@ export type Complete = (value: string, description: string) => void; export type OptionHandler = ( this: Option, complete: Complete, - options: OptionsMap + options: OptionsMap, ) => void; export interface Completion { @@ -28,7 +28,7 @@ export interface Completion { export type ArgumentHandler = ( this: Argument, complete: Complete, - options: OptionsMap + options: OptionsMap, ) => void; export class Argument { @@ -41,7 +41,7 @@ export class Argument { command: Command, name: string, handler?: ArgumentHandler, - variadic: boolean = false + variadic: boolean = false, ) { this.command = command; this.name = name; @@ -64,7 +64,7 @@ export class Option { description: string, handler?: OptionHandler, alias?: string, - isBoolean?: boolean + isBoolean?: boolean, ) { this.command = command; this.value = value; @@ -91,7 +91,7 @@ export class Command { value: string, description: string, handlerOrAlias?: OptionHandler | string, - alias?: string + alias?: string, ): Command { let handler: OptionHandler | undefined; let aliasStr: string | undefined; @@ -117,7 +117,7 @@ export class Command { description, handler, aliasStr, - isBoolean + isBoolean, ); this.options.set(value, option); return this; @@ -130,11 +130,11 @@ export class Command { } } -import * as zsh from './zsh'; +import assert from 'node:assert'; import * as bash from './bash'; import * as fish from './fish'; import * as powershell from './powershell'; -import assert from 'node:assert'; +import * as zsh from './zsh'; export class RootCommand extends Command { commands = new Map(); @@ -214,7 +214,7 @@ export class RootCommand extends Command { private shouldCompleteFlags( lastPrevArg: string | undefined, - toComplete: string + toComplete: string, ): boolean { if (toComplete.startsWith('-')) { return true; @@ -252,7 +252,7 @@ export class RootCommand extends Command { command: Command, previousArgs: string[], toComplete: string, - lastPrevArg: string | undefined + lastPrevArg: string | undefined, ) { // Handle flag value completion let optionName: string | undefined; @@ -275,7 +275,7 @@ export class RootCommand extends Command { option, (value: string, description: string) => suggestions.push({ value, description }), - command.options + command.options, ); this.completions = suggestions; @@ -349,9 +349,11 @@ export class RootCommand extends Command { // positional argument completion private handlePositionalCompletion(command: Command, previousArgs: string[]) { + // Strip options so flags don't inflate the positional index + const strippedArgs = this.stripOptions(previousArgs); // current argument position (subtract command name) const commandParts = command.value.split(' ').length; - const currentArgIndex = Math.max(0, previousArgs.length - commandParts); + const currentArgIndex = Math.max(0, strippedArgs.length - commandParts); const argumentEntries = Array.from(command.arguments.entries()); if (argumentEntries.length > 0) { @@ -377,7 +379,7 @@ export class RootCommand extends Command { targetArgument, (value: string, description: string) => suggestions.push({ value, description }), - command.options + command.options, ); this.completions.push(...suggestions); } @@ -403,7 +405,7 @@ export class RootCommand extends Command { return comp.value.startsWith(toComplete); }) .forEach((comp) => - console.log(`${comp.value}\t${comp.description ?? ''}`) + console.log(`${comp.value}\t${comp.description ?? ''}`), ); console.log(`:${this.directive}`); } @@ -435,23 +437,13 @@ export class RootCommand extends Command { matchedCommand, previousArgs, toComplete, - lastPrevArg + lastPrevArg, ); } else { - if (lastPrevArg?.startsWith('-') && toComplete === '' && endsWithSpace) { - let option = this.findOption(this, lastPrevArg); - if (!option) { - for (const [, command] of this.commands) { - option = this.findOption(command, lastPrevArg); - if (option) break; - } - } - - if (option && option.isBoolean) { - this.complete(toComplete); - return; - } - } + // Note: we intentionally do NOT early-return after detecting a boolean + // flag. The previous code called this.complete(toComplete) and returned + // here, which skipped positional argument completion. After a boolean + // flag like -f, the user may still be completing a positional argument. if (this.shouldCompleteCommands(toComplete)) { this.handleCommandCompletion(previousArgs, toComplete); @@ -470,7 +462,7 @@ export class RootCommand extends Command { shell === 'bash' || shell === 'fish' || shell === 'powershell', - 'Unsupported shell' + 'Unsupported shell', ); switch (shell) { diff --git a/tests/__snapshots__/cli.test.ts.snap b/tests/__snapshots__/cli.test.ts.snap index 4c5bdb9..dc5cb94 100644 --- a/tests/__snapshots__/cli.test.ts.snap +++ b/tests/__snapshots__/cli.test.ts.snap @@ -506,12 +506,18 @@ exports[`cli completion tests for citty > root command argument tests > should c "dev Start dev server copy Copy files lint Lint project +my-app My application +my-lib My library +my-tool My tool :4 " `; exports[`cli completion tests for citty > root command argument tests > should complete root command project argument with options and partial input 1`] = ` -":4 +"my-app My application +my-lib My library +my-tool My tool +:4 " `; @@ -620,23 +626,6 @@ my-tool My tool " `; -exports[`cli completion tests for commander > cli option value handling > should handle unknown options with no completions 1`] = `":4"`; - -exports[`cli completion tests for commander > short flag handling > should handle global short flags 1`] = ` -"-c specify config file -:4 -" -`; - -exports[`cli completion tests for commander > should complete cli options 1`] = ` -"serve Start the server -build Build the project -deploy Deploy the application -lint Lint source files -:4 -" -`; - exports[`cli completion tests for t > --config option tests > should complete --config option values 1`] = ` "vite.config.ts Vite config file vite.config.js Vite config file @@ -846,12 +835,18 @@ exports[`cli completion tests for t > root command argument tests > should compl serve Start the server copy Copy files lint Lint project +my-app My application +my-lib My library +my-tool My tool :4 " `; exports[`cli completion tests for t > root command argument tests > should complete root command project argument with options and partial input 1`] = ` -":4 +"my-app My application +my-lib My library +my-tool My tool +:4 " `; diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 0822ee1..d634f38 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -1,5 +1,5 @@ import { exec } from 'child_process'; -import { describe, it, expect, test } from 'vitest'; +import { describe, expect, it, test } from 'vitest'; function runCommand(command: string): Promise { return new Promise((resolve, reject) => { @@ -38,14 +38,13 @@ describe.each(cliTools)('cli completion tests for %s', (cliTool) => { { partial: '-H', expected: '-H' }, // Test another short flag completion ]; - test.each(optionTests)( - "should complete option for partial input '%s'", - async ({ partial }) => { - const command = `${commandPrefix} dev ${partial}`; - const output = await runCommand(command); - expect(output).toMatchSnapshot(); - } - ); + test.each( + optionTests, + )("should complete option for partial input '%s'", async ({ partial }) => { + const command = `${commandPrefix} dev ${partial}`; + const output = await runCommand(command); + expect(output).toMatchSnapshot(); + }); }); describe.runIf(!shouldSkipTest)('cli option exclusion tests', () => { @@ -53,14 +52,16 @@ describe.each(cliTools)('cli completion tests for %s', (cliTool) => { { specified: '--config', shouldNotContain: '--config' }, ]; - test.each(alreadySpecifiedTests)( - "should not suggest already specified option '%s'", - async ({ specified, shouldNotContain }) => { - const command = `${commandPrefix} ${specified} --`; - const output = await runCommand(command); - expect(output).toMatchSnapshot(); - } - ); + test.each( + alreadySpecifiedTests, + )("should not suggest already specified option '%s'", async ({ + specified, + shouldNotContain, + }) => { + const command = `${commandPrefix} ${specified} --`; + const output = await runCommand(command); + expect(output).toMatchSnapshot(); + }); }); describe.runIf(!shouldSkipTest)('cli option value handling', () => { @@ -90,18 +91,22 @@ describe.each(cliTools)('cli completion tests for %s', (cliTool) => { }); describe.runIf(!shouldSkipTest)('boolean option handling', () => { - it('should not provide value completions for boolean options', async () => { + it('should complete subcommands and arguments after boolean options', async () => { const command = `${commandPrefix} dev --verbose ""`; const output = await runCommand(command); - // Boolean options should return just the directive, no completions - expect(output.trim()).toBe(':4'); + // After a boolean option, should show subcommands/arguments (not flag values) + expect(output).toContain('build'); + expect(output).toContain('start'); + expect(output).not.toContain('verbose'); }); - it('should not provide value completions for short boolean options', async () => { + it('should complete subcommands and arguments after short boolean options', async () => { const command = `${commandPrefix} dev -v ""`; const output = await runCommand(command); - // Boolean options should return just the directive, no completions - expect(output.trim()).toBe(':4'); + // After a short boolean option, should show subcommands/arguments (not flag values) + expect(output).toContain('build'); + expect(output).toContain('start'); + expect(output).not.toContain('verbose'); }); it('should not interfere with command completion after boolean options', async () => { @@ -124,24 +129,27 @@ describe.each(cliTools)('cli completion tests for %s', (cliTool) => { // This tests the case: option('quiet', 'Suppress output') const command = `${commandPrefix} dev --quiet ""`; const output = await runCommand(command); - // Should be treated as boolean flag (no value completion) - expect(output.trim()).toBe(':4'); + // Should be treated as boolean flag — shows subcommands, not flag values + expect(output).toContain('build'); + expect(output).not.toContain('quiet'); }); it('should handle option with alias only as boolean flag', async () => { // This tests the case: option('verbose', 'Enable verbose', 'v') const command = `${commandPrefix} dev --verbose ""`; const output = await runCommand(command); - // Should be treated as boolean flag (no value completion) - expect(output.trim()).toBe(':4'); + // Should be treated as boolean flag — shows subcommands, not flag values + expect(output).toContain('build'); + expect(output).not.toContain('verbose'); }); it('should handle option with alias only (short flag) as boolean flag', async () => { // This tests the short flag version: -v instead of --verbose const command = `${commandPrefix} dev -v ""`; const output = await runCommand(command); - // Should be treated as boolean flag (no value completion) - expect(output.trim()).toBe(':4'); + // Should be treated as boolean flag — shows subcommands, not flag values + expect(output).toContain('build'); + expect(output).not.toContain('verbose'); }); it('should handle option with handler only as value option', async () => { @@ -324,7 +332,7 @@ describe.each(cliTools)('cli completion tests for %s', (cliTool) => { const output = await runCommand(command); expect(output).toMatchSnapshot(); }); - } + }, ); describe.runIf(!shouldSkipTest)('short flag handling', () => { From d7831a5b11a564ec5fc2bbef9d1cff8e8439305b Mon Sep 17 00:00:00 2001 From: Justin Murray Date: Mon, 16 Mar 2026 10:48:58 -0400 Subject: [PATCH 2/2] style: revert cosmetic linter changes, keep only meaningful fix Remove import reordering, trailing comma additions, arrow function conversion, and test.each reformatting that were introduced by a linter not used by the project. Retains only the actual bug fix for positional completion after flags. --- src/commander.ts | 18 +++++++++--------- src/t.ts | 30 +++++++++++++++--------------- tests/cli.test.ts | 37 ++++++++++++++++++------------------- 3 files changed, 42 insertions(+), 43 deletions(-) diff --git a/src/commander.ts b/src/commander.ts index bdd1e7d..09799e7 100644 --- a/src/commander.ts +++ b/src/commander.ts @@ -1,10 +1,10 @@ -import type { Command as CommanderCommand, ParseOptions } from 'commander'; +import * as zsh from './zsh'; import * as bash from './bash'; import * as fish from './fish'; import * as powershell from './powershell'; -import { assertDoubleDashes } from './shared'; +import type { Command as CommanderCommand, ParseOptions } from 'commander'; import t, { type RootCommand } from './t'; -import * as zsh from './zsh'; +import { assertDoubleDashes } from './shared'; const execPath = process.execPath; const processArgs = process.argv.slice(1); @@ -60,7 +60,7 @@ export default function tab(instance: CommanderCommand): RootCommand { console.log('Collected commands:'); for (const [path, cmd] of commandMap.entries()) { console.log( - `- ${path || ''}: ${cmd.description() || 'No description'}`, + `- ${path || ''}: ${cmd.description() || 'No description'}` ); } break; @@ -75,7 +75,7 @@ export default function tab(instance: CommanderCommand): RootCommand { // Override the parse method to handle completion requests before normal parsing const originalParse = instance.parse.bind(instance); - instance.parse = (argv?: readonly string[], options?: ParseOptions) => { + instance.parse = function (argv?: readonly string[], options?: ParseOptions) { const args = argv || process.argv; const completeIndex = args.findIndex((arg) => arg === 'complete'); const dashDashIndex = args.findIndex((arg) => arg === '--'); @@ -125,13 +125,13 @@ function registerOption( value: string, description: string, handlerOrAlias?: ((...args: unknown[]) => void) | string, - alias?: string, + alias?: string ) => unknown; }, flags: string, longFlag: string, description: string, - shortFlag?: string, + shortFlag?: string ): void { const takesValue = optionTakesValue(flags); if (shortFlag) { @@ -191,7 +191,7 @@ function processSubcommands(rootCommand: CommanderCommand): void { flags, longFlag, option.description || '', - shortFlag, + shortFlag ); } } @@ -201,7 +201,7 @@ function processSubcommands(rootCommand: CommanderCommand): void { function collectCommands( command: CommanderCommand, parentPath: string, - commandMap: Map, + commandMap: Map ): void { // Add this command to the map commandMap.set(parentPath, command); diff --git a/src/t.ts b/src/t.ts index 09781d8..b22cd9c 100644 --- a/src/t.ts +++ b/src/t.ts @@ -17,7 +17,7 @@ export type Complete = (value: string, description: string) => void; export type OptionHandler = ( this: Option, complete: Complete, - options: OptionsMap, + options: OptionsMap ) => void; export interface Completion { @@ -28,7 +28,7 @@ export interface Completion { export type ArgumentHandler = ( this: Argument, complete: Complete, - options: OptionsMap, + options: OptionsMap ) => void; export class Argument { @@ -41,7 +41,7 @@ export class Argument { command: Command, name: string, handler?: ArgumentHandler, - variadic: boolean = false, + variadic: boolean = false ) { this.command = command; this.name = name; @@ -64,7 +64,7 @@ export class Option { description: string, handler?: OptionHandler, alias?: string, - isBoolean?: boolean, + isBoolean?: boolean ) { this.command = command; this.value = value; @@ -91,7 +91,7 @@ export class Command { value: string, description: string, handlerOrAlias?: OptionHandler | string, - alias?: string, + alias?: string ): Command { let handler: OptionHandler | undefined; let aliasStr: string | undefined; @@ -117,7 +117,7 @@ export class Command { description, handler, aliasStr, - isBoolean, + isBoolean ); this.options.set(value, option); return this; @@ -130,11 +130,11 @@ export class Command { } } -import assert from 'node:assert'; +import * as zsh from './zsh'; import * as bash from './bash'; import * as fish from './fish'; import * as powershell from './powershell'; -import * as zsh from './zsh'; +import assert from 'node:assert'; export class RootCommand extends Command { commands = new Map(); @@ -214,7 +214,7 @@ export class RootCommand extends Command { private shouldCompleteFlags( lastPrevArg: string | undefined, - toComplete: string, + toComplete: string ): boolean { if (toComplete.startsWith('-')) { return true; @@ -252,7 +252,7 @@ export class RootCommand extends Command { command: Command, previousArgs: string[], toComplete: string, - lastPrevArg: string | undefined, + lastPrevArg: string | undefined ) { // Handle flag value completion let optionName: string | undefined; @@ -275,7 +275,7 @@ export class RootCommand extends Command { option, (value: string, description: string) => suggestions.push({ value, description }), - command.options, + command.options ); this.completions = suggestions; @@ -379,7 +379,7 @@ export class RootCommand extends Command { targetArgument, (value: string, description: string) => suggestions.push({ value, description }), - command.options, + command.options ); this.completions.push(...suggestions); } @@ -405,7 +405,7 @@ export class RootCommand extends Command { return comp.value.startsWith(toComplete); }) .forEach((comp) => - console.log(`${comp.value}\t${comp.description ?? ''}`), + console.log(`${comp.value}\t${comp.description ?? ''}`) ); console.log(`:${this.directive}`); } @@ -437,7 +437,7 @@ export class RootCommand extends Command { matchedCommand, previousArgs, toComplete, - lastPrevArg, + lastPrevArg ); } else { // Note: we intentionally do NOT early-return after detecting a boolean @@ -462,7 +462,7 @@ export class RootCommand extends Command { shell === 'bash' || shell === 'fish' || shell === 'powershell', - 'Unsupported shell', + 'Unsupported shell' ); switch (shell) { diff --git a/tests/cli.test.ts b/tests/cli.test.ts index d634f38..17222d7 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -1,5 +1,5 @@ import { exec } from 'child_process'; -import { describe, expect, it, test } from 'vitest'; +import { describe, it, expect, test } from 'vitest'; function runCommand(command: string): Promise { return new Promise((resolve, reject) => { @@ -38,13 +38,14 @@ describe.each(cliTools)('cli completion tests for %s', (cliTool) => { { partial: '-H', expected: '-H' }, // Test another short flag completion ]; - test.each( - optionTests, - )("should complete option for partial input '%s'", async ({ partial }) => { - const command = `${commandPrefix} dev ${partial}`; - const output = await runCommand(command); - expect(output).toMatchSnapshot(); - }); + test.each(optionTests)( + "should complete option for partial input '%s'", + async ({ partial }) => { + const command = `${commandPrefix} dev ${partial}`; + const output = await runCommand(command); + expect(output).toMatchSnapshot(); + } + ); }); describe.runIf(!shouldSkipTest)('cli option exclusion tests', () => { @@ -52,16 +53,14 @@ describe.each(cliTools)('cli completion tests for %s', (cliTool) => { { specified: '--config', shouldNotContain: '--config' }, ]; - test.each( - alreadySpecifiedTests, - )("should not suggest already specified option '%s'", async ({ - specified, - shouldNotContain, - }) => { - const command = `${commandPrefix} ${specified} --`; - const output = await runCommand(command); - expect(output).toMatchSnapshot(); - }); + test.each(alreadySpecifiedTests)( + "should not suggest already specified option '%s'", + async ({ specified, shouldNotContain }) => { + const command = `${commandPrefix} ${specified} --`; + const output = await runCommand(command); + expect(output).toMatchSnapshot(); + } + ); }); describe.runIf(!shouldSkipTest)('cli option value handling', () => { @@ -332,7 +331,7 @@ describe.each(cliTools)('cli completion tests for %s', (cliTool) => { const output = await runCommand(command); expect(output).toMatchSnapshot(); }); - }, + } ); describe.runIf(!shouldSkipTest)('short flag handling', () => {