Skip to content

Commit 49c4bd1

Browse files
authored
improvement(cli): recovery guidance on every parse error and body-shape refusal (#9646)
* improvement(cli): recovery guidance on every parse error and body-shape refusal * fix(cli): valid JSON recovery examples, short-flag siblings, root guidance, baseline
1 parent e5621b6 commit 49c4bd1

5 files changed

Lines changed: 228 additions & 41 deletions

File tree

‎packages/sim-cli/src/embed-isolation.test.ts‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,4 +117,69 @@ describe('embedded CLI output ownership', () => {
117117
expect(noOptions.stderr).toContain('sim workspaces get takes no options.')
118118
expect(noOptions.stderr).not.toContain('Options for')
119119
})
120+
121+
it('answers a value typed where a flag belongs with the command line that takes it', async () => {
122+
const transport = async () => {
123+
throw new Error('a parse error must not reach the API')
124+
}
125+
const [renamed, kbAsPositional, queryAsPositional, siblingFlag, handWritten] =
126+
await Promise.all([
127+
runEmbeddedCli(['files', 'rename', 'wf_1', 'report.pdf'], { ...identity, transport }),
128+
runEmbeddedCli(['knowledge', 'search', 'kb_1', '--query', 'leave'], {
129+
...identity,
130+
transport,
131+
}),
132+
runEmbeddedCli(['knowledge', 'search', 'leave policy', '--kb', 'kb_1'], {
133+
...identity,
134+
transport,
135+
}),
136+
runEmbeddedCli(['tables', 'rows', 'list', 'tbl_1', '--filter', '{}'], {
137+
...identity,
138+
transport,
139+
}),
140+
runEmbeddedCli(['files', 'upload', 'a.png', '--path', 'files/a.png'], {
141+
...identity,
142+
transport,
143+
}),
144+
])
145+
expect(renamed.exitCode).toBe(1)
146+
expect(renamed.stderr).toContain('"report.pdf" was passed as a positional; pass it as --name')
147+
expect(renamed.stderr).toContain('Example: sim files rename <fileId> --name <value>')
148+
expect(kbAsPositional.stderr).toContain('"kb_1" was passed as a positional; pass it as --kb')
149+
expect(queryAsPositional.exitCode).toBe(1)
150+
expect(queryAsPositional.stderr).toContain('Example: sim knowledge search --kb <value...>')
151+
expect(queryAsPositional.stderr).toContain('Options for sim knowledge search:')
152+
expect(siblingFlag.stderr).toContain('--filter is an option of:\n')
153+
expect(siblingFlag.stderr).toContain(
154+
' sim tables rows query — Query rows with a filter and sort'
155+
)
156+
expect(handWritten.exitCode).toBe(1)
157+
expect(handWritten.stderr).toContain('Options for sim files upload:')
158+
})
159+
160+
it('names the shape a body variant received and the flag that takes it', async () => {
161+
const transport = async () => {
162+
throw new Error('a shape error must not reach the API')
163+
}
164+
const [wrapped, oneRow, rowList] = await Promise.all([
165+
runEmbeddedCli(['tables', 'rows', 'create', 'tbl_1', '--rows', '{"rows":[{"a":1}]}'], {
166+
...identity,
167+
transport,
168+
}),
169+
runEmbeddedCli(['tables', 'rows', 'create', 'tbl_1', '--rows', '{"a":1}'], {
170+
...identity,
171+
transport,
172+
}),
173+
runEmbeddedCli(['tables', 'rows', 'create', 'tbl_1', '--data', '[{"a":1}]'], {
174+
...identity,
175+
transport,
176+
}),
177+
])
178+
expect(wrapped.exitCode).toBe(1)
179+
expect(wrapped.stderr).toContain(`e.g. --rows '[{"column":"value"}]'; got a JSON object`)
180+
expect(wrapped.stderr).toContain('Pass the array under its "rows" key itself')
181+
expect(oneRow.stderr).toContain('For that shape use --data.')
182+
expect(rowList.stderr).toContain('--data must be a JSON object')
183+
expect(rowList.stderr).toContain('For that shape use --rows.')
184+
})
120185
})

‎packages/sim-cli/src/program.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { attachSecretCommands } from './commands/secrets'
99
import { telemetryCommand } from './commands/telemetry'
1010
import { OUTPUT_FORMATS } from './config/index'
1111
import {
12+
addParseErrorGuidanceToTree,
1213
assertNoReservedProgramFlags,
1314
buildGeneratedCommands,
1415
refuseHelpAfterUnknownCommand,
@@ -211,6 +212,7 @@ export function buildProgram(
211212
)
212213

213214
refuseHelpAfterUnknownCommand(program)
215+
addParseErrorGuidanceToTree(program)
214216
assertNoReservedProgramFlags(program)
215217

216218
return program

‎packages/sim-cli/src/runtime/build.ts‎

Lines changed: 125 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Command } from 'commander'
1+
import { Command, type Option } from 'commander'
22
import { CLI_CONTRACT } from '../contract/commands'
33
import type { CommandSpec, CommandVariantSpec } from '../contract/types'
44
import { V2_OPERATIONS, type V2OperationName } from '../generated/v2-api'
@@ -93,46 +93,133 @@ function looksLikeAnId(token: string): boolean {
9393
)
9494
}
9595

96+
const MISSING_OPTION_ERROR = /^error: required option '(.+?)' not specified/
97+
const EXCESS_ARGUMENTS_ERROR = /^error: too many arguments/
98+
99+
interface CommandInternals {
100+
_actionHandler?: unknown
101+
_hidden?: boolean
102+
}
103+
104+
function longFlagSyntax(option: Option): string {
105+
return option.flags.replace(/^-[A-Za-z], /, '')
106+
}
107+
108+
/** The command line with its positionals and every required flag, as `--help` would read it. */
109+
function usageExample(command: Command): string {
110+
const parts = [commandPath(command), argumentSyntax(command)]
111+
for (const option of command.options) if (option.mandatory) parts.push(longFlagSyntax(option))
112+
return parts.filter(Boolean).join(' ')
113+
}
114+
115+
function optionList(command: Command): string {
116+
const flags = command.options.filter((option) => !option.hidden).map((option) => option.flags)
117+
return flags.length > 0
118+
? `Options for ${commandPath(command)}: ${flags.join(', ')}`
119+
: `${commandPath(command)} takes no options.`
120+
}
121+
122+
/** Operands past the declared positionals: the values a caller typed where a flag was wanted. */
123+
function extraOperands(command: Command): string[] {
124+
const declared = command.registeredArguments.length
125+
if (command.registeredArguments.some((argument) => argument.variadic)) return []
126+
return command.args.filter((arg) => !arg.startsWith('-')).slice(declared)
127+
}
128+
96129
/**
97-
* Appends a worked example to the parse errors a positional argument causes,
98-
* and the command's own flags to an unknown option.
130+
* The visible sibling commands that do declare a flag this one does not, each
131+
* with its description, so a read never picks a sibling that deletes.
132+
*/
133+
function siblingsDeclaring(command: Command, flag: string): string[] {
134+
const siblings = command.parent?.commands ?? []
135+
return siblings
136+
.filter(
137+
(sibling) =>
138+
sibling !== command &&
139+
!(sibling as Command & CommandInternals)._hidden &&
140+
sibling.options.some(
141+
(option) => !option.hidden && (option.long === flag || option.short === flag)
142+
)
143+
)
144+
.map((sibling) => ` ${commandPath(sibling)} — ${sibling.description().split('\n', 1)[0]}`)
145+
}
146+
147+
/**
148+
* Appends recovery guidance to commander's parse errors, so the retry needs no
149+
* `--help` round.
99150
*
100-
* Covers the argument being absent and the argument being swallowed as an
101-
* option because its id opens with a dash; the second needs the `--` escape,
102-
* which commander never mentions. A guessed flag gets the real list: commander
103-
* suggests only a near spelling, so `--row-ids` for `--row` got a bare error
104-
* and a caller had to look the command up before retrying.
151+
* - A missing positional or required flag gets the full command line, and a
152+
* value typed as a positional where the flag was wanted (`files rename
153+
* <id> new.pdf`, `knowledge search <kbId>`) is named against that flag.
154+
* - Too many positionals get the same command line and the flag list.
155+
* - An unknown option gets the command's own flags, and the sibling commands
156+
* that do take it: `tables rows list --filter` belongs to `tables rows
157+
* query`, which commander's near-spelling suggestion never reaches.
158+
* - An id swallowed as an option because it opens with a dash gets the `--`
159+
* escape, which commander never mentions.
105160
*/
106-
function addParseErrorGuidance(command: Command): Command {
107-
const outputError = command.configureOutput().outputError
108-
if (!outputError) throw new Error('Commander output formatter is not configured')
109-
110-
command.configureOutput({
111-
outputError: (message, write) => {
112-
outputError(message, write)
113-
114-
if (message.startsWith('error: missing required argument ')) {
115-
const syntax = argumentSyntax(command)
116-
const example = syntax ? `${commandPath(command)} ${syntax}` : commandPath(command)
117-
write(`Example: ${example}\n`)
118-
return
119-
}
161+
function parseErrorGuidance(command: Command, message: string): string[] {
162+
if (message.startsWith('error: missing required argument ')) {
163+
return [`Example: ${usageExample(command)}`]
164+
}
120165

121-
const token = UNKNOWN_OPTION_TOKEN.exec(message)?.[1]
122-
if (!token) return
123-
if (looksLikeAnId(token) && command.registeredArguments.length > 0) {
124-
write(`Example: ${commandPath(command)} -- ${token}\n`)
125-
return
126-
}
127-
const flags = command.options.filter((option) => !option.hidden).map((option) => option.flags)
128-
write(
129-
flags.length > 0
130-
? `Options for ${commandPath(command)}: ${flags.join(', ')}\n`
131-
: `${commandPath(command)} takes no options.\n`
132-
)
133-
},
134-
})
135-
return command
166+
const missingOption = MISSING_OPTION_ERROR.exec(message)?.[1]
167+
if (missingOption) {
168+
const extra = extraOperands(command)
169+
const flag = missingOption.split(' ', 1)[0]
170+
const moved =
171+
extra.length === 1
172+
? [`"${extra[0]}" was passed as a positional; pass it as ${flag} "${extra[0]}".`]
173+
: []
174+
return [...moved, `Example: ${usageExample(command)}`]
175+
}
176+
177+
if (EXCESS_ARGUMENTS_ERROR.test(message)) {
178+
return [`Example: ${usageExample(command)}`, optionList(command)]
179+
}
180+
181+
const token = UNKNOWN_OPTION_TOKEN.exec(message)?.[1]
182+
if (!token) return []
183+
if (looksLikeAnId(token) && command.registeredArguments.length > 0) {
184+
return [`Example: ${commandPath(command)} -- ${token}`]
185+
}
186+
const flag = token.split('=', 1)[0]
187+
const owners = siblingsDeclaring(command, flag)
188+
return [
189+
optionList(command),
190+
...(owners.length > 0 ? [`${flag} is an option of:`, ...owners] : []),
191+
]
192+
}
193+
194+
/**
195+
* Wraps the command's own `error`, which every commander parse failure on it
196+
* goes through. The output configuration cannot carry this: commands made with
197+
* `.command()` share their parent's configuration object, so a formatter set on
198+
* one would answer for all of them.
199+
*/
200+
function addParseErrorGuidance(command: Command): void {
201+
const error = command.error.bind(command)
202+
command.error = (message, options) => {
203+
const guidance = parseErrorGuidance(command, message)
204+
return error(guidance.length > 0 ? `${message}\n${guidance.join('\n')}` : message, options)
205+
}
206+
}
207+
208+
/**
209+
* Attaches {@link addParseErrorGuidance} to every command that parses its own
210+
* arguments, generated or attached by hand (`files upload`, `secrets set`), so
211+
* a hand-written command does not answer a guessed flag with a bare error.
212+
* Pure dispatchers below the root are left to commander's unknown-command handling.
213+
*/
214+
export function addParseErrorGuidanceToTree(program: Command): void {
215+
const walk = (command: Command): void => {
216+
const parsesOwnArguments =
217+
command.commands.length === 0 ||
218+
Boolean((command as Command & CommandInternals)._actionHandler)
219+
if (parsesOwnArguments || !command.parent) addParseErrorGuidance(command)
220+
for (const child of command.commands) walk(child)
221+
}
222+
walk(program)
136223
}
137224

138225
/**
@@ -374,7 +461,7 @@ function configureOperation(
374461
}
375462

376463
function buildLeaf(operation: V2OperationName, spec: CommandSpec, leafName: string): Command {
377-
return addParseErrorGuidance(configureOperation(new Command(leafName), operation, spec))
464+
return configureOperation(new Command(leafName), operation, spec)
378465
}
379466

380467
/**

‎packages/sim-cli/src/runtime/request.ts‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,47 @@
11
import { closeSync, existsSync, fstatSync, openSync, readSync } from 'node:fs'
22
import { CLI_CONTRACT } from '../contract/commands'
3-
import type { CommandSpec, FlagSpec } from '../contract/types'
3+
import type { BodyVariantSpec, CommandSpec, FlagSpec } from '../contract/types'
44
import { embedStore } from '../embed-context'
55
import { V2_OPERATIONS, type V2OperationName } from '../generated/v2-api'
66
import { type QueryValue, SimApiError } from '../http/client'
77
import { embeddedFileContent } from '../transfer/local-file'
88
import { camel, kebab } from './derive'
99
import type { OperationSpec } from './types'
1010

11+
function jsonKind(value: unknown): string {
12+
if (Array.isArray(value)) return 'a JSON array'
13+
if (value === null) return 'null'
14+
return typeof value === 'object' ? 'a JSON object' : `a JSON ${typeof value}`
15+
}
16+
17+
/**
18+
* The refusal for a body variant given the wrong JSON shape, stating what came
19+
* in and the spelling that takes it. `--rows '{"rows":[...]}'` and `--rows`
20+
* with a single row were the recurring misses; neither value is reshaped,
21+
* because guessing which part the caller meant is the server's write to make.
22+
*/
23+
function bodyVariantShapeError(
24+
variant: BodyVariantSpec,
25+
parsed: unknown,
26+
variants: readonly BodyVariantSpec[]
27+
): string {
28+
const flag = `--${variant.name}`
29+
const example = variant.kind === 'array' ? `'[{"column":"value"}]'` : `'{"column":"value"}'`
30+
const base = `${flag} must be a JSON ${variant.kind}, e.g. ${flag} ${example}; got ${jsonKind(parsed)}.`
31+
if (variant.kind === 'array' && parsed && typeof parsed === 'object' && !Array.isArray(parsed)) {
32+
if (Array.isArray((parsed as Record<string, unknown>)[variant.property])) {
33+
return `${base} Pass the array under its "${variant.property}" key itself, not the object wrapping it.`
34+
}
35+
}
36+
const other = variants.find(
37+
(candidate) =>
38+
candidate !== variant && candidate.kind === (Array.isArray(parsed) ? 'array' : 'object')
39+
)
40+
return other && parsed && typeof parsed === 'object'
41+
? `${base} For that shape use --${other.name}.`
42+
: base
43+
}
44+
1145
/** One request field, as the generator describes it. */
1246
export interface FieldSpec {
1347
kind: 'string' | 'number' | 'integer' | 'boolean' | 'enum' | 'array' | 'object' | 'unknown'
@@ -803,7 +837,7 @@ export async function buildRequest(
803837
(!parsed || typeof parsed !== 'object' || Array.isArray(parsed))) ||
804838
(variant.kind === 'array' && !Array.isArray(parsed))
805839
) {
806-
throw new SimApiError(`--${variant.name} must be a JSON ${variant.kind}`, 0)
840+
throw new SimApiError(bodyVariantShapeError(variant, parsed, commandSpec.bodyVariants), 0)
807841
}
808842
return boundedRequest({
809843
params,

‎scripts/check-unused-exports.baseline.json‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5908,7 +5908,6 @@
59085908
"packages/sim-cli/src/config/index.ts#normalizeEndpoint",
59095909
"packages/sim-cli/src/config/index.ts#telemetryStatePath",
59105910
"packages/sim-cli/src/config/paths.ts#configDir",
5911-
"packages/sim-cli/src/contract/types.ts#BodyVariantSpec",
59125911
"packages/sim-cli/src/contract/types.ts#PathFlagSpec",
59135912
"packages/sim-cli/src/http/client.ts#OAuthRefresher",
59145913
"packages/sim-cli/src/output/terminal-text.ts#artPad",

0 commit comments

Comments
 (0)