From ac0ee0c38ed4b7c46e6e91054dac5dfca36d14f7 Mon Sep 17 00:00:00 2001 From: Yug Patel <5773665+Detoy@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:32:05 +0530 Subject: [PATCH] fix(cli): explain switch values like --vulns=false instead of 'unknown option' Commander rejects a value on an on/off switch (--vulns=false) and an undefined --no- form (--no-vulns) as an unknown option. That is safe, but the message is misleading, and for --no-vulns it suggests --vulns, the opposite of what was asked. Check the dispatched args before parsing and, for those inputs only, fail with a usage error (exit 5) that names the switch and shows the form that works. Pass-through commands and args after -- are left alone. A table-driven test pins how vg scan parses --vulns and --offline, and DOCS.md notes that switches take no value. Closes #395 Signed-off-by: Yug Patel <5773665+Detoy@users.noreply.github.com> --- DOCS.md | 2 + src/cli-switch-values.test.ts | 137 ++++++++++++++++++++++++++++++++++ src/cli-switch-values.ts | 83 ++++++++++++++++++++ src/cli.ts | 6 +- 4 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 src/cli-switch-values.test.ts create mode 100644 src/cli-switch-values.ts diff --git a/DOCS.md b/DOCS.md index a9f7991d..1561d750 100644 --- a/DOCS.md +++ b/DOCS.md @@ -2549,6 +2549,8 @@ vg scan [path] [--vulns] [--full] [--iac] [--format text|json|sarif|md] [--out < | `--quiet` | — | Suppress promotional output; scan results are unaffected | | `--allow-unsafe-root` | — | Scan a filesystem root, an OS-image layout, or a tree over the walk budget anyway. See [Unsafe roots](#unsafe-roots) | +Switches (flags that take no value, such as `--vulns`, `--offline` or `--no-graph`) are off unless you pass them. They do not accept a value or an invented `--no-` form: `--vulns=false`, `--vulns=true` and `--no-vulns` stop with exit code `5` and name the form that works. In a CI template, add or leave out the flag itself rather than passing `true` or `false`. + By default, the scan writes `.vibgrate/scan_result.json`. Use `--no-local-artifacts` or `--max-privacy` to suppress local JSON artifact files. ### Unsafe roots diff --git a/src/cli-switch-values.test.ts b/src/cli-switch-values.test.ts new file mode 100644 index 00000000..25ead9b3 --- /dev/null +++ b/src/cli-switch-values.test.ts @@ -0,0 +1,137 @@ +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { Command, CommanderError } from 'commander'; +import { describe, expect, it } from 'vitest'; +import { buildProgram } from './cli.js'; +import { switchValueError } from './cli-switch-values.js'; + +/** + * CI templates often spell switches out explicitly (`--vulns=false`, + * `--no-vulns`), usually from a templated variable. `vg` must never read such + * an input as "on", and when it rejects one the error must name the switch and + * show a form that works. + */ + +const SWITCHES = ['--vulns', '--offline'] as const; +const attr = (flag: string): string => flag.slice(2); + +class Parsed extends Error { + constructor(readonly opts: Record) { + super('parsed'); + } +} + +/** + * Parse `vg scan ...args` through the real program and return the + * options commander resolved, or the error it rejected the input with. The + * scan itself never runs. + */ +async function parseScan(args: string[]): Promise | CommanderError> { + const scan = buildProgram().commands.find((c) => c.name() === 'scan') as Command; + scan.exitOverride(); + scan.configureOutput({ writeErr: () => {}, writeOut: () => {} }); + scan.hook('preAction', (_cmd, action) => { + throw new Parsed(action.opts()); + }); + try { + await scan.parseAsync([os.tmpdir(), ...args], { from: 'user' }); + } catch (e) { + if (e instanceof Parsed) return e.opts; + if (e instanceof CommanderError) return e; + throw e; + } + throw new Error('scan did not reach its action'); +} + +describe('vg scan switches parse as documented', () => { + for (const flag of SWITCHES) { + describe(flag, () => { + it('is off when absent', async () => { + const opts = await parseScan([]); + expect(opts).not.toBeInstanceOf(CommanderError); + expect((opts as Record)[attr(flag)]).toBeUndefined(); + }); + + it(`is on with ${flag}`, async () => { + const opts = await parseScan([flag]); + expect((opts as Record)[attr(flag)]).toBe(true); + }); + + for (const input of [`${flag}=true`, `${flag}=false`, `${flag}=maybe`, `${flag}=`, `--no-${attr(flag)}`]) { + it(`rejects ${input} and never turns the switch on`, async () => { + const res = await parseScan([input]); + expect(res).toBeInstanceOf(CommanderError); + expect((res as CommanderError).code).toBe('commander.unknownOption'); + }); + } + }); + } +}); + +describe('switchValueError', () => { + const program = buildProgram(); + + for (const flag of SWITCHES) { + for (const value of ['true', 'false', 'maybe', '']) { + it(`explains ${flag}=${value} with the switch and a working example`, () => { + const msg = switchValueError(program, ['scan', '.', `${flag}=${value}`]); + expect(msg).toContain(`${flag} is an on/off switch and takes no value`); + expect(msg).toContain(`got ${flag}=${value}`); + expect(msg).toContain(`vg scan ${flag}`); + }); + } + + it(`explains --no-${attr(flag)} without suggesting the opposite`, () => { + const msg = switchValueError(program, ['scan', `--no-${attr(flag)}`, '.']); + expect(msg).toContain(`--no-${attr(flag)} is not an option`); + expect(msg).toContain(`${flag} is off unless you pass it`); + expect(msg).toContain(`vg scan ${flag}`); + }); + } + + it('explains a value on a --no- switch the command does define', () => { + expect(switchValueError(program, ['scan', '--no-graph=1'])).toContain( + '--no-graph is an on/off switch and takes no value (got --no-graph=1)', + ); + }); + + it.each([ + [['scan', '.', '--vulns', '--offline']], + [['scan', '.', '--format=json']], + [['scan', '.', '--bogus=1']], + [['scan', '.', '--no-graph']], + [['scan', '.', '--', '--vulns=false']], + [['scan', '.', '--no-such-switch']], + ])('leaves valid or unrelated input to commander: %j', (args) => { + expect(switchValueError(program, args)).toBeUndefined(); + }); + + it('does not inspect arguments a pass-through command forwards', () => { + // `vg serve ...` hands everything after the agent name to the agent. + // `--http` is a serve switch, so without the pass-through rule this would match. + expect(switchValueError(program, ['serve', 'claude', '--http=1'])).toBeUndefined(); + }); +}); + +describe('vg scan with a switch value, end to end', () => { + const pkgRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'vg-switch-')); + fs.writeFileSync(path.join(dir, 'package.json'), '{"name":"fixture","version":"1.0.0"}\n'); + + const run = (flag: string) => + spawnSync( + process.execPath, + ['--import', 'tsx', path.join(pkgRoot, 'src/cli.ts'), 'scan', dir, '--offline', '--no-graph', '--no-daemon', '--quiet', '--no-local-artifacts', flag], + { cwd: pkgRoot, encoding: 'utf8', timeout: 45_000, env: { ...process.env, NO_COLOR: '1', VIBGRATE_NO_KERNEL: '1', VIBGRATE_DSN: '' } }, + ); + + it.each(['--vulns=false', '--no-vulns'])('%s exits with the usage code and a working example', (flag) => { + const res = run(flag); + expect(res.status).toBe(5); + expect(res.stderr).toContain('vg scan --vulns'); + expect(res.stderr).not.toContain('Did you mean'); + }, 60_000); +}); diff --git a/src/cli-switch-values.ts b/src/cli-switch-values.ts new file mode 100644 index 00000000..b3ab4319 --- /dev/null +++ b/src/cli-switch-values.ts @@ -0,0 +1,83 @@ +import type { Command, Option } from 'commander'; + +/** + * Explain an on/off switch written with a value (`--vulns=false`) or with a + * `--no-` prefix the command does not define (`--no-vulns`). + * + * Commander rejects both as an unknown option, which is safe but unhelpful: + * `--vulns=false` reads as "unknown option" for a switch that exists, and for + * `--no-vulns` it suggests `--vulns`, the opposite of what was asked. Returns + * a usage message for the first such argument, or `undefined` to leave the + * input to commander. Only inputs commander would reject anyway are matched. + */ +export function switchValueError(program: Command, args: string[]): string | undefined { + const { cmd, rest } = resolveCommand(program, args); + if (isPassThrough(cmd)) return undefined; + const usage = commandPath(cmd); + + for (const arg of rest) { + if (arg === '--') break; + if (!arg.startsWith('--')) continue; + + const eq = arg.indexOf('='); + if (eq > 0) { + const flag = arg.slice(0, eq); + const opt = switchNamed(cmd, flag); + if (!opt || (!opt.negate && opt.defaultValue !== undefined)) continue; + const hint = flag.startsWith('--no-') + ? `use \`${usage} ${flag}\`, or leave it out` + : `it is off unless you pass it: use \`${usage} ${flag}\` to turn it on, or leave it out`; + return `${flag} is an on/off switch and takes no value (got ${arg}); ${hint}.`; + } + + if (arg.startsWith('--no-') && !findOption(cmd, arg)) { + const positive = `--${arg.slice('--no-'.length)}`; + const opt = switchNamed(cmd, positive); + if (!opt || opt.negate || opt.defaultValue !== undefined) continue; + return `${arg} is not an option. ${positive} is off unless you pass it, so leave it out (to turn it on: \`${usage} ${positive}\`).`; + } + } + return undefined; +} + +/** Walk leading subcommand names (`models catalog ...`) down to the leaf. */ +function resolveCommand(program: Command, args: string[]): { cmd: Command; rest: string[] } { + let cmd = program; + let i = 0; + for (; i < args.length; i++) { + const sub = cmd.commands.find((c) => c.name() === args[i] || c.aliases().includes(args[i])); + if (!sub) break; + cmd = sub; + } + return { cmd, rest: args.slice(i) }; +} + +/** + * A pass-through command (`vg serve ...`) forwards options after its + * first operand to another program, so their spelling is not ours to judge. + * Commander exposes the setter but not a getter for this. + */ +function isPassThrough(cmd: Command): boolean { + return (cmd as unknown as { _passThroughOptions?: boolean })._passThroughOptions === true; +} + +/** + * Only the leaf's own options count: the program enables positional options, + * so a parent's flag is not accepted after a subcommand name (global flags are + * declared on every subcommand instead, see cli-options.applyGlobalOptions). + */ +function findOption(cmd: Command, long: string): Option | undefined { + return cmd.options.find((o) => o.long === long); +} + +/** The option named `long` if it takes no value: `--vulns`, and also `--no-graph`. */ +function switchNamed(cmd: Command, long: string): Option | undefined { + const opt = findOption(cmd, long); + return opt && !opt.required && !opt.optional ? opt : undefined; +} + +function commandPath(cmd: Command): string { + const names: string[] = []; + for (let c: Command | null = cmd; c; c = c.parent) names.unshift(c.name()); + return names.join(' '); +} diff --git a/src/cli.ts b/src/cli.ts index 5a13fe47..b232ae05 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -45,13 +45,14 @@ import { registerReview } from './commands/review.js'; import { ConfigFileError } from './core-open/config.js'; import { LockfileParseError } from './core-open/utils/lockfile-parse.js'; import { UnsafeRootError } from './engine/root-safety.js'; -import { CliError, ExitCode } from './util/exit.js'; +import { CliError, ExitCode, usageError } from './util/exit.js'; import { c, info, disableColor, exitAfterFlush } from './util/output.js'; // Drift-reporting commands (merged from the Vibgrate CLI). These run on the // open base engine (`@vibgrate/core-open`) — no proprietary kernel. import { initCommand } from './reporting/commands/init.js'; import { scanCommand } from './reporting/commands/scan.js'; +import { switchValueError } from './cli-switch-values.js'; import { fixCommand } from './reporting/commands/fix.js'; import { baselineCommand } from './reporting/commands/baseline.js'; import { reportCommand } from './reporting/commands/report.js'; @@ -398,6 +399,9 @@ export async function main(argv = process.argv): Promise { // dispatch() can throw (e.g. a moved-command hint) — keep it inside the // handler so those surface as a clean `error:` line, not a raw stack. const dispatched = dispatch(raw, cwd); + // Commander would reject these as an unknown option; say what works instead. + const switchError = switchValueError(program, dispatched); + if (switchError) throw usageError(switchError); await program.parseAsync(dispatched, { from: 'user' }); } catch (err) { handleError(err);