diff --git a/.changeset/fish-multi-segment-delegation.md b/.changeset/fish-multi-segment-delegation.md new file mode 100644 index 0000000..d110f8b --- /dev/null +++ b/.changeset/fish-multi-segment-delegation.md @@ -0,0 +1,7 @@ +--- +'@bomb.sh/tab': patch +--- + +fix(fish): pass multi-segment CLI paths as separate arguments + +In fish, completing a package-manager-delegated CLI with more than one path segment (e.g. `pnpm --`) returned nothing and fell back to the package manager's own flags. The generated fish script built the request with `string join ' '` + `eval`, and because fish does not expand bare `(...)` command substitution inside double quotes, the whole path collapsed into a single token on re-parse. The template now invokes the backend directly with fish list expansion, so every segment reaches the completion backend as its own argument. zsh and bash already quoted each argument individually; PowerShell was audited and quotes each token before `Invoke-Expression`, so it is unaffected. diff --git a/src/fish.ts b/src/fish.ts index 28e016b..289a729 100644 --- a/src/fish.ts +++ b/src/fish.ts @@ -31,17 +31,15 @@ function __${nameForVar}_perform_completion # Extract all args except the last one set -l args (commandline -opc) - # Extract the last arg and escape it in case it is a space or wildcard - set -l lastArg (string escape -- (commandline -ct)) + # Extract the token currently being completed (may be empty on a trailing space) + set -l lastArg (commandline -ct) __${nameForVar}_debug "args: $args" __${nameForVar}_debug "last arg: $lastArg" - # Build the completion request command - set -l requestComp "${exec} complete -- (string join ' ' -- (string escape -- $args[2..-1])) $lastArg" - - __${nameForVar}_debug "Calling $requestComp" - set -l results (eval $requestComp 2> /dev/null) + # Pass each arg as its own token (no string join/eval, which would collapse multi-segment paths); quote "$lastArg" so an empty trailing token is still sent. + __${nameForVar}_debug "Calling ${exec} complete -- $args[2..-1] $lastArg" + set -l results (${exec} complete -- $args[2..-1] "$lastArg" 2> /dev/null) # Some programs may output extra empty lines after the directive. # Let's ignore them or else it will break completion. diff --git a/tests/__snapshots__/shell.test.ts.snap b/tests/__snapshots__/shell.test.ts.snap index 6a54925..27d8d34 100644 --- a/tests/__snapshots__/shell.test.ts.snap +++ b/tests/__snapshots__/shell.test.ts.snap @@ -15,17 +15,15 @@ function __testcli_perform_completion # Extract all args except the last one set -l args (commandline -opc) - # Extract the last arg and escape it in case it is a space or wildcard - set -l lastArg (string escape -- (commandline -ct)) + # Extract the token currently being completed (may be empty on a trailing space) + set -l lastArg (commandline -ct) __testcli_debug "args: $args" __testcli_debug "last arg: $lastArg" - # Build the completion request command - set -l requestComp "/usr/bin/node /path/to/testcli complete -- (string join ' ' -- (string escape -- $args[2..-1])) $lastArg" - - __testcli_debug "Calling $requestComp" - set -l results (eval $requestComp 2> /dev/null) + # Pass each arg as its own token (no string join/eval, which would collapse multi-segment paths); quote "$lastArg" so an empty trailing token is still sent. + __testcli_debug "Calling /usr/bin/node /path/to/testcli complete -- $args[2..-1] $lastArg" + set -l results (/usr/bin/node /path/to/testcli complete -- $args[2..-1] "$lastArg" 2> /dev/null) # Some programs may output extra empty lines after the directive. # Let's ignore them or else it will break completion. @@ -253,17 +251,15 @@ function __test_cli_app_perform_completion # Extract all args except the last one set -l args (commandline -opc) - # Extract the last arg and escape it in case it is a space or wildcard - set -l lastArg (string escape -- (commandline -ct)) + # Extract the token currently being completed (may be empty on a trailing space) + set -l lastArg (commandline -ct) __test_cli_app_debug "args: $args" __test_cli_app_debug "last arg: $lastArg" - # Build the completion request command - set -l requestComp "/usr/bin/node /path/to/testcli complete -- (string join ' ' -- (string escape -- $args[2..-1])) $lastArg" - - __test_cli_app_debug "Calling $requestComp" - set -l results (eval $requestComp 2> /dev/null) + # Pass each arg as its own token (no string join/eval, which would collapse multi-segment paths); quote "$lastArg" so an empty trailing token is still sent. + __test_cli_app_debug "Calling /usr/bin/node /path/to/testcli complete -- $args[2..-1] $lastArg" + set -l results (/usr/bin/node /path/to/testcli complete -- $args[2..-1] "$lastArg" 2> /dev/null) # Some programs may output extra empty lines after the directive. # Let's ignore them or else it will break completion. diff --git a/tests/completion-exec.test.ts b/tests/completion-exec.test.ts index a5d8b76..3d3e4ec 100644 --- a/tests/completion-exec.test.ts +++ b/tests/completion-exec.test.ts @@ -28,7 +28,9 @@ function extractExecCommand(shell: string, script: string): string | null { match = script.match(/requestComp="(.+?) complete --/); break; case 'fish': - match = script.match(/set -l requestComp "(.+?) complete --/); + // The fish template invokes the backend directly (no requestComp/eval), + // so match the direct call: `set -l results ( complete -- ...)`. + match = script.match(/set -l results \((.+?) complete --/); break; case 'powershell': match = script.match(/\$RequestComp = "& (.+?) complete '--'/); diff --git a/tests/fish-integration.test.ts b/tests/fish-integration.test.ts index b05c303..f843032 100644 --- a/tests/fish-integration.test.ts +++ b/tests/fish-integration.test.ts @@ -1,8 +1,11 @@ -import { describe, it, expect, beforeAll } from 'vitest'; -import { execSync, spawnSync } from 'child_process'; -import * as path from 'path'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execSync, spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { dirname, join, resolve, delimiter } from 'node:path'; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import * as fish from '../src/fish'; -// Check if fish is available function isFishAvailable(): boolean { try { execSync('fish --version', { stdio: 'pipe' }); @@ -12,118 +15,100 @@ function isFishAvailable(): boolean { } } -// Run fish command and return output -function runFish(script: string): string { - const result = spawnSync('fish', ['-c', script], { - encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], - }); - if (result.error) { - throw result.error; - } - return result.stdout + result.stderr; -} +const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '..'); +const fixtureDir = join(repoRoot, 'tests', 'fixtures', 'mycli'); +const tabCli = join(repoRoot, 'dist', 'bin', 'cli.mjs'); -// Simulate TAB completion in fish -function simulateTab(completionScript: string, commandLine: string): string[] { - const script = ` - source (echo '${completionScript.replace(/'/g, "\\'")}' | psub) - complete --do-complete "${commandLine}" - `; - const output = runFish(script); - return output - .split('\n') - .filter((line) => line.trim() !== '') - .map((line) => line.trim()); +function shQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; } -describe.skipIf(!isFishAvailable())( - 'fish shell completion integration tests', - () => { - const demoCliPath = path.join( - __dirname, - '../examples/demo-cli-cac/demo-cli-cac.js' - ); - let completionScript: string; - - beforeAll(() => { - // Generate the completion script from demo-cli-cac - const result = spawnSync('node', [demoCliPath, 'complete', 'fish'], { - encoding: 'utf-8', - cwd: path.dirname(demoCliPath), - }); - completionScript = result.stdout; - }); - - it('should complete subcommands when pressing TAB after command', () => { - const completions = simulateTab(completionScript, 'demo-cli-cac '); +describe.skipIf(!isFishAvailable())('fish delegation completion', () => { + let cacheDir: string; + let scriptPath: string; - // Should contain subcommands - expect(completions.some((c) => c.startsWith('start'))).toBe(true); - expect(completions.some((c) => c.startsWith('build'))).toBe(true); - }); + beforeAll(() => { + if (!existsSync(tabCli)) { + execSync('pnpm build', { cwd: repoRoot, stdio: 'ignore' }); + } - it('should complete flags when pressing TAB after --', () => { - const completions = simulateTab(completionScript, 'demo-cli-cac --'); + cacheDir = mkdtempSync(join(tmpdir(), 'tab-fish-integration-')); - // Should contain global flags - expect(completions.some((c) => c.includes('--config'))).toBe(true); - expect(completions.some((c) => c.includes('--debug'))).toBe(true); - expect(completions.some((c) => c.includes('--help'))).toBe(true); - expect(completions.some((c) => c.includes('--version'))).toBe(true); - }); + // Generate the pnpm completer wired to this repo's backend. + const exec = `${process.execPath} ${tabCli} pnpm`; + scriptPath = join(cacheDir, 'pnpm.fish'); + writeFileSync(scriptPath, fish.generate('pnpm', exec)); + }); - it('should complete subcommand-specific flags', () => { - const completions = simulateTab( - completionScript, - 'demo-cli-cac start --' - ); + afterAll(() => { + if (cacheDir) rmSync(cacheDir, { recursive: true, force: true }); + }); - // Should contain start-specific flag - expect(completions.some((c) => c.includes('--port'))).toBe(true); - // Should also contain global flags - expect(completions.some((c) => c.includes('--config'))).toBe(true); + // Run the generated completer for a command line and return the completion + // values (the part before the tab-separated description). + function complete(commandLine: string): string[] { + const script = `source ${shQuote(scriptPath)}\ncomplete --do-complete ${shQuote( + commandLine + )}`; + + const result = spawnSync('fish', ['-c', script], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${fixtureDir}${delimiter}${process.env.PATH ?? ''}`, + TAB_COMPLETION_CACHE_DIR: cacheDir, + }, }); - it('should complete build command flags', () => { - const completions = simulateTab( - completionScript, - 'demo-cli-cac build --' - ); - - // Should contain build-specific flag - expect(completions.some((c) => c.includes('--mode'))).toBe(true); - // Should also contain global flags - expect(completions.some((c) => c.includes('--config'))).toBe(true); - }); + return (result.stdout + result.stderr) + .split('\n') + .map((line) => line.trim()) + .filter((line) => line !== ''); + } - it('should show descriptions with completions', () => { - const completions = simulateTab(completionScript, 'demo-cli-cac '); + it('completes a delegated CLI\u2019s subcommands (single segment)', () => { + const completions = complete('pnpm mycli '); + expect(completions.some((c) => c.startsWith('start'))).toBe(true); + expect(completions.some((c) => c.startsWith('build'))).toBe(true); + expect(completions.some((c) => c.startsWith('db'))).toBe(true); + }); - // Check that descriptions are included (tab-separated) - const startCompletion = completions.find((c) => c.startsWith('start')); - expect(startCompletion).toContain('Start the application'); + it('includes descriptions from the delegated CLI', () => { + const completions = complete('pnpm mycli '); + const start = completions.find((c) => c.startsWith('start')); + expect(start).toContain('Start the application'); + }); - const buildCompletion = completions.find((c) => c.startsWith('build')); - expect(buildCompletion).toContain('Build the application'); - }); + it('filters delegated subcommands by partial input', () => { + const completions = complete('pnpm mycli st'); + expect(completions.some((c) => c.startsWith('start'))).toBe(true); + expect(completions.some((c) => c.startsWith('build'))).toBe(false); + }); - it('should filter completions based on partial input', () => { - const completions = simulateTab(completionScript, 'demo-cli-cac st'); + it('completes the delegated CLI\u2019s flags', () => { + const completions = complete('pnpm mycli --'); + expect(completions.some((c) => c.includes('--config'))).toBe(true); + expect(completions.some((c) => c.includes('--debug'))).toBe(true); + }); - // Should only show completions starting with 'st' - expect(completions.some((c) => c.startsWith('start'))).toBe(true); - // 'build' should not appear - expect(completions.some((c) => c.startsWith('build'))).toBe(false); - }); + // The following cases have a subcommand *after* the CLI name, so they are the + // multi-segment paths that regressed. Each must reach the delegated CLI with + // every segment intact. + it('completes a nested subcommand (two-segment path)', () => { + const completions = complete('pnpm mycli db '); + expect(completions.some((c) => c.startsWith('migrate'))).toBe(true); + expect(completions.some((c) => c.startsWith('seed'))).toBe(true); + }); - it('should filter flag completions based on partial input', () => { - const completions = simulateTab(completionScript, 'demo-cli-cac --c'); + it('completes flags after a subcommand (two-segment path)', () => { + const completions = complete('pnpm mycli start --'); + expect(completions.some((c) => c.includes('--port'))).toBe(true); + expect(completions.some((c) => c.includes('--config'))).toBe(true); + }); - // Should show --config - expect(completions.some((c) => c.includes('--config'))).toBe(true); - // Should not show --debug (doesn't start with --c) - expect(completions.some((c) => c.includes('--debug'))).toBe(false); - }); - } -); + it('completes flags after a nested subcommand (three-segment path)', () => { + const completions = complete('pnpm mycli db migrate --'); + expect(completions.some((c) => c.includes('--force'))).toBe(true); + expect(completions.some((c) => c.includes('--name'))).toBe(true); + }); +}); diff --git a/tests/fixtures/mycli/mycli b/tests/fixtures/mycli/mycli new file mode 100755 index 0000000..7e27ef8 --- /dev/null +++ b/tests/fixtures/mycli/mycli @@ -0,0 +1,60 @@ +#!/usr/bin/env node +// Minimal tab-protocol CLI fixture with a nested command tree, used to drive the +// fish delegation integration tests (`pnpm mycli `). It emits +// `\t` lines plus a trailing `:`. + +const argv = process.argv.slice(2); +if (argv[0] !== 'complete') { + console.log('mycli demo'); + process.exit(0); +} + +const dashIndex = argv.indexOf('--'); +const rest = dashIndex === -1 ? [] : argv.slice(dashIndex + 1); +const toComplete = rest.length ? rest[rest.length - 1] : ''; +const prevWords = rest.slice(0, -1).filter((w) => !w.startsWith('-')); +const commandPath = prevWords.join(' '); + +const globalFlags = [ + ['--config', 'Specify config file'], + ['--debug', 'Enable debugging'], + ['--help', 'Display help'], + ['--version', 'Display version'], +]; + +const tree = { + '': { + subs: [ + ['start', 'Start the application'], + ['build', 'Build the application'], + ['db', 'Database commands'], + ], + flags: [], + }, + start: { subs: [], flags: [['--port', 'Port to use']] }, + build: { subs: [], flags: [['--mode', 'Build mode']] }, + db: { + subs: [ + ['migrate', 'Run migrations'], + ['seed', 'Seed the database'], + ], + flags: [], + }, + 'db migrate': { + subs: [], + flags: [ + ['--force', 'Force the migration'], + ['--name', 'Migration name'], + ], + }, +}; + +const node = tree[commandPath] || { subs: [], flags: [] }; +const candidates = toComplete.startsWith('-') + ? [...node.flags, ...globalFlags] + : node.subs; + +for (const [value, description] of candidates) { + if (value.startsWith(toComplete)) console.log(`${value}\t${description}`); +} +console.log(':4'); diff --git a/tests/shell-empty-argv.test.ts b/tests/shell-empty-argv.test.ts index 5a8bcad..43999dd 100644 --- a/tests/shell-empty-argv.test.ts +++ b/tests/shell-empty-argv.test.ts @@ -55,6 +55,24 @@ const cases: CompletionCase[] = [ line: 'demo dev ', expected: ['dev', ''], }, + { + // Regression guard for delegated CLIs reached through a package manager, + // e.g. `pnpm `. Every segment of the path must reach the + // backend as its own argument. fish previously collapsed these into a + // single space-joined token via `string join` + `eval`. + label: 'multi-segment path with trailing space', + words: ['demo', 'sub', 'nested', ''], + current: 4, + line: 'demo sub nested ', + expected: ['sub', 'nested', ''], + }, + { + label: 'multi-segment path while completing a flag', + words: ['demo', 'sub', 'nested', '--'], + current: 4, + line: 'demo sub nested --', + expected: ['sub', 'nested', '--'], + }, ]; function execFileAsync( @@ -151,9 +169,12 @@ const completionArgs = fs.appendFileSync(capturePath, JSON.stringify(completionArgs) + '\\n'); -// Emit one matching completion so shells that use native filtering still exit successfully. -// The argv capture is what this test actually asserts. -process.stdout.write('dev\\tStart dev server\\n:4\\n'); +// Emit one completion that matches whatever token is currently being typed so +// shells doing native prefix filtering (e.g. bash's compgen) still find a match +// and exit successfully. The argv capture is what this test actually asserts. +const lastArg = completionArgs.length ? completionArgs[completionArgs.length - 1] : ''; +const value = lastArg.length ? lastArg : 'dev'; +process.stdout.write(value + '\\tcompletion\\n:4\\n'); `.trimStart() );