fix: make flag failures legible, and help honest about flag types (COR-13656) - #23
Closed
Bradenream wants to merge 1 commit into
Closed
fix: make flag failures legible, and help honest about flag types (COR-13656)#23Bradenream wants to merge 1 commit into
Bradenream wants to merge 1 commit into
Conversation
…R-13656) Split out of #21 so the contested part can be argued separately. Nothing here changes what the CLI accepts: every value parsed before parses now, every value rejected before is still rejected, and the request body is byte-identical to master across plain text, quoted JSON, null, objects, arrays, empty and numeric input. Only the messages change. 1. Rejection messages name the problem instead of the first character. vf agent update --llm 'gpt-4' invalid value for --llm: error unmarshalling json response body: invalid character 'g' looking for beginning of value There is no response body — the request was never sent. Worse, valid JSON of the wrong shape got the same treatment: --llm '[1,2]' parses perfectly, and the CLI insisted it was not valid JSON, sending the reader to re-check syntax that was never wrong. And the decoder's own explanation, the only thing that knows which Go type was expected, was discarded entirely. Now: shape mismatch is named separately from syntax error, an example of the right shape is derived from the destination, and the decoder's message is surfaced minus its misleading prefix. The example is derived rather than fixed because a hardcoded one told array-valued flags to pass an object — following the CLI's own advice reproduced the identical error. For the nullable string fields this is the whole fix from the caller's side: invalid value for --instructions: expected a JSON value you passed: You are a support agent. the value is not valid JSON expected shape: --instructions '"your text here"' decoder reported: invalid character 'Y' looking for beginning of value pass null to clear the field: --instructions null Following that advice works. Previously nothing said the value had to be JSON-quoted at all. 2. Flag errors reach the same renderer as everything else. They never did: they returned raw from the generated RunE and printed via a bare Fprintln, so a mistyped flag got one unstructured line while an API 401 got a structured envelope — backwards, since mistyped flags are far more common. flagutil cannot import output (output imports flagutil), so failures carry a typed FlagValueError up to Execute, which renders it. What gets echoed is bounded, in both modes, and counted in runes. 3. Help shows the flag's type, not a word from its prose. pflag reads the first back-quoted word in a usage string as the value placeholder. Descriptions come from the OpenAPI spec, where backticks are emphasis, so --instructions rendered as "--instructions Name" for a flag that wants markdown. 12 flags; the worst is transcript search's --version-param, whose only legal values are draft and published, labelled with an unrelated parameter name. Fixed where descriptions enter cobra, so a 13th is covered automatically. --usage output is byte-identical; it was already correct. 4. Agent-mode errors print exactly once. Every printer in internal/output writes to stderr and also returns the error, and main.go printed whatever Execute returned. AgentModeError's own doc comment promised a single print, but main.go is generated and printed unconditionally, so the contract was never kept. stderr is now a single parseable JSON document for all three agent-mode error shapes. Pretty output is untouched — there the trailing line is the readable summary after a raw API body, not a duplicate. Footprint on generated files is 12 insertions and 7 deletions across five files; everything else is new non-generated files. Verified: 35 tests, 20 failing against master and 15 passing on both — the 15 are the invariants, including that --instructions null still sends a real JSON null. Full suite shows the same pre-existing failures as master.
There was a problem hiding this comment.
🟡 Changes recommended
One of the new JSON-flag hints (“pass null to clear the field”) is emitted unconditionally and can be misleading for required/non-nullable JSON flags, and truncateForError can avoid allocating a full []rune for very large inputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves the CLI’s UX and agent-friendliness around flag parsing failures by introducing richer, typed flag-value errors, routing them through the standard output/error renderer (including agent-mode JSON envelopes), fixing help placeholders impacted by backticks in OpenAPI descriptions, and preventing duplicate stderr printing for already-rendered errors.
Changes:
- Introduces
flagutil.FlagValueError+ JSON-shape-aware diagnostics for JSON-typed flags, including bounded value echoing. - Routes flag-value errors through
cli.Execute()so agent mode renders them via the same structured envelope path as other CLI-level errors. - Prevents duplicate stderr output in agent mode by returning a wrapped “already reported” error and updating
cmd/vf/main.goto skip printing empty messages; also neutralizes backticks in flag descriptions to keep help type placeholders accurate.
File summaries
| File | Description |
|---|---|
| test/flag-errors.test.ts | Adds end-to-end tests for flag error messaging, bounded echoing, agent-mode envelopes, and help placeholder correctness. |
| test/errors-teach.test.ts | Tightens assertions that agent-mode stderr is only JSON (no trailing duplicate prose). |
| internal/output/reported.go | Adds an internal wrapper error type that suppresses second-printing by making Error() empty while preserving Unwrap(). |
| internal/output/output.go | Returns AlreadyReported(err) from output.Error to prevent main from printing the same error again. |
| internal/output/agentmode.go | Returns AlreadyReported(...) from AgentModeError to avoid duplicate printing via main.go. |
| internal/flagutil/metadata.go | Normalizes descriptions via flagDescription before registering flags to avoid pflag placeholder hijacking. |
| internal/flagutil/description.go | Replaces backticks with apostrophes so pflag doesn’t treat prose emphasis as a value placeholder. |
| internal/flagutil/flagerror.go | Introduces FlagValueError with bounded, rune-counted value display for error messages. |
| internal/flagutil/jsonerror.go | Adds JSON syntax-vs-shape-aware error reporting and derived “expected shape” examples. |
| internal/cli/root.go | Wraps command execution with flag-value error rendering for agent mode. |
| internal/cli/flagerrors.go | Renders *FlagValueError via output.AgentModeError in agent mode while leaving human mode unchanged. |
| cmd/vf/main.go | Skips printing returned errors with an empty message to prevent duplicate stderr output. |
Review details
Files not reviewed (5)
- cmd/vf/main.go: Generated file
- internal/cli/root.go: Generated file
- internal/flagutil/metadata.go: Generated file
- internal/output/agentmode.go: Generated file
- internal/output/output.go: Generated file
- Files reviewed: 7/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+67
to
+75
| func truncateForError(s string) string { | ||
| const max = 120 | ||
| s = strings.ReplaceAll(s, "\n", " ") | ||
| runes := []rune(s) | ||
| if len(runes) <= max { | ||
| return s | ||
| } | ||
| return string(runes[:max]) + "… (" + fmt.Sprint(len(runes)) + " chars)" | ||
| } |
Comment on lines
+48
to
+56
| if example := jsonShapeExample(t); example != "" { | ||
| hints = append(hints, fmt.Sprintf("expected shape: --%s '%s'", m.FlagName, example)) | ||
| } | ||
| if detail := unmarshalDetail(cause); detail != "" { | ||
| hints = append(hints, detail) | ||
| } | ||
| hints = append(hints, "pass null to clear the field: --"+m.FlagName+" null") | ||
|
|
||
| return &FlagValueError{Flag: m.FlagName, Value: val, Expected: expected, Hints: hints, Cause: cause} |
effervescentia
requested changes
Sep 2, 2026
effervescentia
left a comment
effervescentia
left a comment
Contributor
There was a problem hiding this comment.
I tried running yarn codegen locally and it failed with this error
Generating SDK for cli...
commercial license requires a validated license token: attach one with licensetoken.WithToken (or --license-token / SPEAKEASY_LICENSE_TOKEN); authenticated state asserting a commercial license is not proof by itself
To get help, send the following reproduction command to the Speakeasy team:
speakeasy repro voiceflow_engineering_01a0628d-8b94-7353-8a54-4311bcf5883b
Step Failed: Generating Cli SDK
Step Failed: Workflow
Step Failed: Target: voiceflow-sdk
failed to generate "cli"
Failed to run with Speakeasy version 1.796.3: failed to run with version 1.796.3: exit status 1
effervescentia
approved these changes
Sep 3, 2026
Comment on lines
+171
to
+179
| const cases: Array<[name: string, args: string[]]> = [ | ||
| ['CLI-level error (AgentModeError)', ['configure']], | ||
| ['preflight error (no token)', ['workspace', 'list']], | ||
| ['flag-value error', ['agent', 'update', '--project-id', 'p', '--environment-alias', 'main', | ||
| '--dry-run', '--token', 'vfp_x', '--llm', 'not json']], | ||
| ]; | ||
|
|
||
| for (const [name, args] of cases) { | ||
| it(`${name}: stderr is nothing but the JSON envelope`, async () => { |
Contributor
There was a problem hiding this comment.
🟢 use it.each() instead here as the idiomatic way to run the same test across multiple inputs
Contributor
Author
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #21 so the contested part can be argued separately. This half changes no parsing behaviour at all — every value accepted before is accepted now, every value rejected before is still rejected, and the request body is byte-identical to master across plain text, quoted JSON,
null, objects, arrays, empty and numeric input. Only the messages change.1. Rejection messages name the problem, not the first character
There is no response body — the request was never sent. Worse, valid JSON of the wrong shape got the same treatment:
--llm '[1,2]'parses perfectly, and the CLI insisted it was not valid JSON, sending the reader to re-check syntax that was never wrong. The decoder's own explanation — the only thing that knows which Go type was expected — was discarded.Now shape mismatch is named separately from syntax error, an example of the right shape is derived from the destination, and the decoder's message is surfaced minus its misleading prefix:
The example is derived rather than fixed because a hardcoded one told array-valued flags to pass an object — following the CLI's own advice reproduced the identical error. A test now takes the suggestion and feeds it back in.
For the nullable string fields this is the entire fix from the caller's side:
Following that advice works. Previously nothing said the value had to be JSON-quoted at all.
2. Flag errors reach the same renderer as everything else
They never did — they returned raw from the generated
RunEand printed via a bareFprintln, so a mistyped flag got one unstructured line while an API 401 got a structured envelope. Backwards, since mistyped flags are far more common.flagutilcannot importoutput(outputimportsflagutil), so failures carry a typedFlagValueErrorup toExecute, which renders it. What gets echoed is bounded in both modes, and counted in runes.3. Help shows the flag's type, not a word from its prose
agent update--instructions Name--instructions stringtranscript search--version-param environmentAlias--version-param stringpflag reads the first back-quoted word in a usage string as the value placeholder, and our descriptions come from the spec where backticks are emphasis. 12 flags. The worst is
--version-param, whose only legal values aredraftandpublished. Fixed where descriptions enter cobra, so a 13th is covered automatically.--usageis byte-identical — it was already correct.4. Agent-mode errors print exactly once
Every printer in
internal/outputwrites to stderr and returns the error, andmain.goprinted whateverExecutereturned.AgentModeError's own doc comment promised a single print, butmain.gois generated and printed unconditionally, so the contract was never kept. stderr is now a single parseable JSON document for all three agent-mode error shapes.Pretty output is deliberately untouched: there the trailing line is the readable summary that follows a raw API body, not a duplicate.
Footprint and verification
12 insertions, 7 deletions across five generated files. Everything else is new non-generated files, so there is very little for
speakeasy runto merge.--instructions nullstill sends a real JSON null.Relationship to #21
#21 is closed in favour of this plus #24. The raw-text fallback — the part @effervescentia requested changes on — is isolated there so this can land on its own.