From 7ef37b6d18173d67aa4d9824400f84a78c0ec27a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 3 Nov 2026 15:35:39 -0800 Subject: [PATCH 1/2] improvement(cli): recovery guidance on every parse error and body-shape refusal --- packages/sim-cli/src/embed-isolation.test.ts | 65 ++++++++ packages/sim-cli/src/program.ts | 2 + packages/sim-cli/src/runtime/build.ts | 161 ++++++++++++++----- packages/sim-cli/src/runtime/request.ts | 38 ++++- 4 files changed, 226 insertions(+), 40 deletions(-) diff --git a/packages/sim-cli/src/embed-isolation.test.ts b/packages/sim-cli/src/embed-isolation.test.ts index 9c694539d0c..7437f8a57b2 100644 --- a/packages/sim-cli/src/embed-isolation.test.ts +++ b/packages/sim-cli/src/embed-isolation.test.ts @@ -117,4 +117,69 @@ describe('embedded CLI output ownership', () => { expect(noOptions.stderr).toContain('sim workspaces get takes no options.') expect(noOptions.stderr).not.toContain('Options for') }) + + it('answers a value typed where a flag belongs with the command line that takes it', async () => { + const transport = async () => { + throw new Error('a parse error must not reach the API') + } + const [renamed, kbAsPositional, queryAsPositional, siblingFlag, handWritten] = + await Promise.all([ + runEmbeddedCli(['files', 'rename', 'wf_1', 'report.pdf'], { ...identity, transport }), + runEmbeddedCli(['knowledge', 'search', 'kb_1', '--query', 'leave'], { + ...identity, + transport, + }), + runEmbeddedCli(['knowledge', 'search', 'leave policy', '--kb', 'kb_1'], { + ...identity, + transport, + }), + runEmbeddedCli(['tables', 'rows', 'list', 'tbl_1', '--filter', '{}'], { + ...identity, + transport, + }), + runEmbeddedCli(['files', 'upload', 'a.png', '--path', 'files/a.png'], { + ...identity, + transport, + }), + ]) + expect(renamed.exitCode).toBe(1) + expect(renamed.stderr).toContain('"report.pdf" was passed as a positional; pass it as --name') + expect(renamed.stderr).toContain('Example: sim files rename --name ') + expect(kbAsPositional.stderr).toContain('"kb_1" was passed as a positional; pass it as --kb') + expect(queryAsPositional.exitCode).toBe(1) + expect(queryAsPositional.stderr).toContain('Example: sim knowledge search --kb ') + expect(queryAsPositional.stderr).toContain('Options for sim knowledge search:') + expect(siblingFlag.stderr).toContain('--filter is an option of:\n') + expect(siblingFlag.stderr).toContain( + ' sim tables rows query — Query rows with a filter and sort' + ) + expect(handWritten.exitCode).toBe(1) + expect(handWritten.stderr).toContain('Options for sim files upload:') + }) + + it('names the shape a body variant received and the flag that takes it', async () => { + const transport = async () => { + throw new Error('a shape error must not reach the API') + } + const [wrapped, oneRow, rowList] = await Promise.all([ + runEmbeddedCli(['tables', 'rows', 'create', 'tbl_1', '--rows', '{"rows":[{"a":1}]}'], { + ...identity, + transport, + }), + runEmbeddedCli(['tables', 'rows', 'create', 'tbl_1', '--rows', '{"a":1}'], { + ...identity, + transport, + }), + runEmbeddedCli(['tables', 'rows', 'create', 'tbl_1', '--data', '[{"a":1}]'], { + ...identity, + transport, + }), + ]) + expect(wrapped.exitCode).toBe(1) + expect(wrapped.stderr).toContain('got a JSON object') + expect(wrapped.stderr).toContain('Pass the array under its "rows" key itself') + expect(oneRow.stderr).toContain('For that shape use --data.') + expect(rowList.stderr).toContain('--data must be a JSON object') + expect(rowList.stderr).toContain('For that shape use --rows.') + }) }) diff --git a/packages/sim-cli/src/program.ts b/packages/sim-cli/src/program.ts index 7cca9a83a54..c5da098746a 100644 --- a/packages/sim-cli/src/program.ts +++ b/packages/sim-cli/src/program.ts @@ -9,6 +9,7 @@ import { attachSecretCommands } from './commands/secrets' import { telemetryCommand } from './commands/telemetry' import { OUTPUT_FORMATS } from './config/index' import { + addParseErrorGuidanceToTree, assertNoReservedProgramFlags, buildGeneratedCommands, refuseHelpAfterUnknownCommand, @@ -211,6 +212,7 @@ export function buildProgram( ) refuseHelpAfterUnknownCommand(program) + addParseErrorGuidanceToTree(program) assertNoReservedProgramFlags(program) return program diff --git a/packages/sim-cli/src/runtime/build.ts b/packages/sim-cli/src/runtime/build.ts index ac2f132eda9..803a509bff6 100644 --- a/packages/sim-cli/src/runtime/build.ts +++ b/packages/sim-cli/src/runtime/build.ts @@ -1,4 +1,4 @@ -import { Command } from 'commander' +import { Command, type Option } from 'commander' import { CLI_CONTRACT } from '../contract/commands' import type { CommandSpec, CommandVariantSpec } from '../contract/types' import { V2_OPERATIONS, type V2OperationName } from '../generated/v2-api' @@ -93,46 +93,131 @@ function looksLikeAnId(token: string): boolean { ) } +const MISSING_OPTION_ERROR = /^error: required option '(.+?)' not specified/ +const EXCESS_ARGUMENTS_ERROR = /^error: too many arguments/ + +interface CommandInternals { + _actionHandler?: unknown + _hidden?: boolean +} + +function longFlagSyntax(option: Option): string { + return option.flags.replace(/^-[A-Za-z], /, '') +} + +/** The command line with its positionals and every required flag, as `--help` would read it. */ +function usageExample(command: Command): string { + const parts = [commandPath(command), argumentSyntax(command)] + for (const option of command.options) if (option.mandatory) parts.push(longFlagSyntax(option)) + return parts.filter(Boolean).join(' ') +} + +function optionList(command: Command): string { + const flags = command.options.filter((option) => !option.hidden).map((option) => option.flags) + return flags.length > 0 + ? `Options for ${commandPath(command)}: ${flags.join(', ')}` + : `${commandPath(command)} takes no options.` +} + +/** Operands past the declared positionals: the values a caller typed where a flag was wanted. */ +function extraOperands(command: Command): string[] { + const declared = command.registeredArguments.length + if (command.registeredArguments.some((argument) => argument.variadic)) return [] + return command.args.filter((arg) => !arg.startsWith('-')).slice(declared) +} + +/** + * The visible sibling commands that do declare a flag this one does not, each + * with its description, so a read never picks a sibling that deletes. + */ +function siblingsDeclaring(command: Command, flag: string): string[] { + const siblings = command.parent?.commands ?? [] + return siblings + .filter( + (sibling) => + sibling !== command && + !(sibling as Command & CommandInternals)._hidden && + sibling.options.some((option) => !option.hidden && option.long === flag) + ) + .map((sibling) => ` ${commandPath(sibling)} — ${sibling.description().split('\n', 1)[0]}`) +} + /** - * Appends a worked example to the parse errors a positional argument causes, - * and the command's own flags to an unknown option. + * Appends recovery guidance to commander's parse errors, so the retry needs no + * `--help` round. * - * Covers the argument being absent and the argument being swallowed as an - * option because its id opens with a dash; the second needs the `--` escape, - * which commander never mentions. A guessed flag gets the real list: commander - * suggests only a near spelling, so `--row-ids` for `--row` got a bare error - * and a caller had to look the command up before retrying. + * - A missing positional or required flag gets the full command line, and a + * value typed as a positional where the flag was wanted (`files rename + * new.pdf`, `knowledge search `) is named against that flag. + * - Too many positionals get the same command line and the flag list. + * - An unknown option gets the command's own flags, and the sibling commands + * that do take it: `tables rows list --filter` belongs to `tables rows + * query`, which commander's near-spelling suggestion never reaches. + * - An id swallowed as an option because it opens with a dash gets the `--` + * escape, which commander never mentions. */ -function addParseErrorGuidance(command: Command): Command { - const outputError = command.configureOutput().outputError - if (!outputError) throw new Error('Commander output formatter is not configured') - - command.configureOutput({ - outputError: (message, write) => { - outputError(message, write) - - if (message.startsWith('error: missing required argument ')) { - const syntax = argumentSyntax(command) - const example = syntax ? `${commandPath(command)} ${syntax}` : commandPath(command) - write(`Example: ${example}\n`) - return - } +function parseErrorGuidance(command: Command, message: string): string[] { + if (message.startsWith('error: missing required argument ')) { + return [`Example: ${usageExample(command)}`] + } - const token = UNKNOWN_OPTION_TOKEN.exec(message)?.[1] - if (!token) return - if (looksLikeAnId(token) && command.registeredArguments.length > 0) { - write(`Example: ${commandPath(command)} -- ${token}\n`) - return - } - const flags = command.options.filter((option) => !option.hidden).map((option) => option.flags) - write( - flags.length > 0 - ? `Options for ${commandPath(command)}: ${flags.join(', ')}\n` - : `${commandPath(command)} takes no options.\n` - ) - }, - }) - return command + const missingOption = MISSING_OPTION_ERROR.exec(message)?.[1] + if (missingOption) { + const extra = extraOperands(command) + const flag = missingOption.split(' ', 1)[0] + const moved = + extra.length === 1 + ? [`"${extra[0]}" was passed as a positional; pass it as ${flag} "${extra[0]}".`] + : [] + return [...moved, `Example: ${usageExample(command)}`] + } + + if (EXCESS_ARGUMENTS_ERROR.test(message)) { + return [`Example: ${usageExample(command)}`, optionList(command)] + } + + const token = UNKNOWN_OPTION_TOKEN.exec(message)?.[1] + if (!token) return [] + if (looksLikeAnId(token) && command.registeredArguments.length > 0) { + return [`Example: ${commandPath(command)} -- ${token}`] + } + const flag = token.split('=', 1)[0] + const owners = siblingsDeclaring(command, flag) + return [ + optionList(command), + ...(owners.length > 0 ? [`${flag} is an option of:`, ...owners] : []), + ] +} + +/** + * Wraps the command's own `error`, which every commander parse failure on it + * goes through. The output configuration cannot carry this: commands made with + * `.command()` share their parent's configuration object, so a formatter set on + * one would answer for all of them. + */ +function addParseErrorGuidance(command: Command): void { + const error = command.error.bind(command) + command.error = (message, options) => { + const guidance = parseErrorGuidance(command, message) + return error(guidance.length > 0 ? `${message}\n${guidance.join('\n')}` : message, options) + } +} + +/** + * Attaches {@link addParseErrorGuidance} to every command that parses its own + * arguments, generated or attached by hand (`files upload`, `secrets set`), so + * a hand-written command does not answer a guessed flag with a bare error. + * Pure dispatchers are left to commander's unknown-command handling. + */ +export function addParseErrorGuidanceToTree(program: Command): void { + const walk = (command: Command): void => { + const parsesOwnArguments = + command.commands.length === 0 || + Boolean((command as Command & CommandInternals)._actionHandler) + if (command.parent && parsesOwnArguments) addParseErrorGuidance(command) + for (const child of command.commands) walk(child) + } + walk(program) } /** @@ -374,7 +459,7 @@ function configureOperation( } function buildLeaf(operation: V2OperationName, spec: CommandSpec, leafName: string): Command { - return addParseErrorGuidance(configureOperation(new Command(leafName), operation, spec)) + return configureOperation(new Command(leafName), operation, spec) } /** diff --git a/packages/sim-cli/src/runtime/request.ts b/packages/sim-cli/src/runtime/request.ts index f2f6f6efed3..b986cb51af3 100644 --- a/packages/sim-cli/src/runtime/request.ts +++ b/packages/sim-cli/src/runtime/request.ts @@ -1,6 +1,6 @@ import { closeSync, existsSync, fstatSync, openSync, readSync } from 'node:fs' import { CLI_CONTRACT } from '../contract/commands' -import type { CommandSpec, FlagSpec } from '../contract/types' +import type { BodyVariantSpec, CommandSpec, FlagSpec } from '../contract/types' import { embedStore } from '../embed-context' import { V2_OPERATIONS, type V2OperationName } from '../generated/v2-api' import { type QueryValue, SimApiError } from '../http/client' @@ -8,6 +8,40 @@ import { embeddedFileContent } from '../transfer/local-file' import { camel, kebab } from './derive' import type { OperationSpec } from './types' +function jsonKind(value: unknown): string { + if (Array.isArray(value)) return 'a JSON array' + if (value === null) return 'null' + return typeof value === 'object' ? 'a JSON object' : `a JSON ${typeof value}` +} + +/** + * The refusal for a body variant given the wrong JSON shape, stating what came + * in and the spelling that takes it. `--rows '{"rows":[...]}'` and `--rows` + * with a single row were the recurring misses; neither value is reshaped, + * because guessing which part the caller meant is the server's write to make. + */ +function bodyVariantShapeError( + variant: BodyVariantSpec, + parsed: unknown, + variants: readonly BodyVariantSpec[] +): string { + const flag = `--${variant.name}` + const example = variant.kind === 'array' ? `'[{...}, {...}]'` : `'{...}'` + const base = `${flag} must be a JSON ${variant.kind}, e.g. ${flag} ${example}; got ${jsonKind(parsed)}.` + if (variant.kind === 'array' && parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + if (Array.isArray((parsed as Record)[variant.property])) { + return `${base} Pass the array under its "${variant.property}" key itself, not the object wrapping it.` + } + } + const other = variants.find( + (candidate) => + candidate !== variant && candidate.kind === (Array.isArray(parsed) ? 'array' : 'object') + ) + return other && parsed && typeof parsed === 'object' + ? `${base} For that shape use --${other.name}.` + : base +} + /** One request field, as the generator describes it. */ export interface FieldSpec { kind: 'string' | 'number' | 'integer' | 'boolean' | 'enum' | 'array' | 'object' | 'unknown' @@ -803,7 +837,7 @@ export async function buildRequest( (!parsed || typeof parsed !== 'object' || Array.isArray(parsed))) || (variant.kind === 'array' && !Array.isArray(parsed)) ) { - throw new SimApiError(`--${variant.name} must be a JSON ${variant.kind}`, 0) + throw new SimApiError(bodyVariantShapeError(variant, parsed, commandSpec.bodyVariants), 0) } return boundedRequest({ params, From 02089c9a39800d294c4f598f46ac210b98115e94 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 3 Nov 2026 15:44:26 -0800 Subject: [PATCH 2/2] fix(cli): valid JSON recovery examples, short-flag siblings, root guidance, baseline --- packages/sim-cli/src/embed-isolation.test.ts | 2 +- packages/sim-cli/src/runtime/build.ts | 8 +++++--- packages/sim-cli/src/runtime/request.ts | 2 +- scripts/check-unused-exports.baseline.json | 1 - 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/sim-cli/src/embed-isolation.test.ts b/packages/sim-cli/src/embed-isolation.test.ts index 7437f8a57b2..9606b0b4342 100644 --- a/packages/sim-cli/src/embed-isolation.test.ts +++ b/packages/sim-cli/src/embed-isolation.test.ts @@ -176,7 +176,7 @@ describe('embedded CLI output ownership', () => { }), ]) expect(wrapped.exitCode).toBe(1) - expect(wrapped.stderr).toContain('got a JSON object') + expect(wrapped.stderr).toContain(`e.g. --rows '[{"column":"value"}]'; got a JSON object`) expect(wrapped.stderr).toContain('Pass the array under its "rows" key itself') expect(oneRow.stderr).toContain('For that shape use --data.') expect(rowList.stderr).toContain('--data must be a JSON object') diff --git a/packages/sim-cli/src/runtime/build.ts b/packages/sim-cli/src/runtime/build.ts index 803a509bff6..435dc20f036 100644 --- a/packages/sim-cli/src/runtime/build.ts +++ b/packages/sim-cli/src/runtime/build.ts @@ -137,7 +137,9 @@ function siblingsDeclaring(command: Command, flag: string): string[] { (sibling) => sibling !== command && !(sibling as Command & CommandInternals)._hidden && - sibling.options.some((option) => !option.hidden && option.long === flag) + sibling.options.some( + (option) => !option.hidden && (option.long === flag || option.short === flag) + ) ) .map((sibling) => ` ${commandPath(sibling)} — ${sibling.description().split('\n', 1)[0]}`) } @@ -207,14 +209,14 @@ function addParseErrorGuidance(command: Command): void { * Attaches {@link addParseErrorGuidance} to every command that parses its own * arguments, generated or attached by hand (`files upload`, `secrets set`), so * a hand-written command does not answer a guessed flag with a bare error. - * Pure dispatchers are left to commander's unknown-command handling. + * Pure dispatchers below the root are left to commander's unknown-command handling. */ export function addParseErrorGuidanceToTree(program: Command): void { const walk = (command: Command): void => { const parsesOwnArguments = command.commands.length === 0 || Boolean((command as Command & CommandInternals)._actionHandler) - if (command.parent && parsesOwnArguments) addParseErrorGuidance(command) + if (parsesOwnArguments || !command.parent) addParseErrorGuidance(command) for (const child of command.commands) walk(child) } walk(program) diff --git a/packages/sim-cli/src/runtime/request.ts b/packages/sim-cli/src/runtime/request.ts index b986cb51af3..995a7cf23b7 100644 --- a/packages/sim-cli/src/runtime/request.ts +++ b/packages/sim-cli/src/runtime/request.ts @@ -26,7 +26,7 @@ function bodyVariantShapeError( variants: readonly BodyVariantSpec[] ): string { const flag = `--${variant.name}` - const example = variant.kind === 'array' ? `'[{...}, {...}]'` : `'{...}'` + const example = variant.kind === 'array' ? `'[{"column":"value"}]'` : `'{"column":"value"}'` const base = `${flag} must be a JSON ${variant.kind}, e.g. ${flag} ${example}; got ${jsonKind(parsed)}.` if (variant.kind === 'array' && parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { if (Array.isArray((parsed as Record)[variant.property])) { diff --git a/scripts/check-unused-exports.baseline.json b/scripts/check-unused-exports.baseline.json index 78f7592cff0..0c8c49366a4 100644 --- a/scripts/check-unused-exports.baseline.json +++ b/scripts/check-unused-exports.baseline.json @@ -5908,7 +5908,6 @@ "packages/sim-cli/src/config/index.ts#normalizeEndpoint", "packages/sim-cli/src/config/index.ts#telemetryStatePath", "packages/sim-cli/src/config/paths.ts#configDir", - "packages/sim-cli/src/contract/types.ts#BodyVariantSpec", "packages/sim-cli/src/contract/types.ts#PathFlagSpec", "packages/sim-cli/src/http/client.ts#OAuthRefresher", "packages/sim-cli/src/output/terminal-text.ts#artPad",