From 07ecb4b8ed07a9f4efeaea1bb04c0b65fe3fcb7e Mon Sep 17 00:00:00 2001 From: Bradenream <51544548+Bradenream@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:21:54 +0000 Subject: [PATCH] fix: make flag failures legible, and help honest about flag types (COR-13656) (#23) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ``` 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. 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: ``` invalid value for --llm: the value is valid JSON but not the shape this flag expects you passed: [1,2] expected shape: --llm '{"key":"value"}' decoder reported: cannot unmarshal array into Go value of type map[string]json.RawMessage pass null to clear the field: --llm null ``` 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: ``` 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 ``` 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 | | before | after | |---|---|---| | `agent update` | `--instructions Name` | `--instructions string` | | `transcript search` | `--version-param environmentAlias` | `--version-param string` | pflag 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 are `draft` and `published`. Fixed where descriptions enter cobra, so a 13th is covered automatically. `--usage` is byte-identical — it was already correct. ## 4. Agent-mode errors print exactly once Every printer in `internal/output` writes to stderr *and* 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 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 run` to merge. - **35 tests: 20 fail against master, 15 pass on both.** The 15 are the invariants — including that `--instructions null` still sends a real JSON null. - Parsing verified identical to master on 7 input shapes. - Full suite shows the same pre-existing failures as master. ## 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. --- cmd/vf/main.go | 6 +- internal/cli/flagerrors.go | 49 +++++++ internal/cli/root.go | 2 +- internal/flagutil/description.go | 37 ++++++ internal/flagutil/flagerror.go | 75 +++++++++++ internal/flagutil/jsonerror.go | 115 ++++++++++++++++ internal/flagutil/metadata.go | 5 +- internal/output/agentmode.go | 4 +- internal/output/output.go | 2 +- internal/output/reported.go | 55 ++++++++ test/errors-teach.test.ts | 45 ++++++- test/flag-errors.test.ts | 216 +++++++++++++++++++++++++++++++ 12 files changed, 603 insertions(+), 8 deletions(-) create mode 100644 internal/cli/flagerrors.go create mode 100644 internal/flagutil/description.go create mode 100644 internal/flagutil/flagerror.go create mode 100644 internal/flagutil/jsonerror.go create mode 100644 internal/output/reported.go create mode 100644 test/flag-errors.test.ts diff --git a/cmd/vf/main.go b/cmd/vf/main.go index 86a343d..af77fb0 100644 --- a/cmd/vf/main.go +++ b/cmd/vf/main.go @@ -25,7 +25,11 @@ func main() { } if err := cli.Execute(); err != nil { - fmt.Fprintln(os.Stderr, err) + // An empty message means the producer already wrote it to stderr; + // printing again would duplicate it. See internal/output/reported.go. + if msg := err.Error(); msg != "" { + fmt.Fprintln(os.Stderr, msg) + } os.Exit(1) } } diff --git a/internal/cli/flagerrors.go b/internal/cli/flagerrors.go new file mode 100644 index 0000000..0fef3a2 --- /dev/null +++ b/internal/cli/flagerrors.go @@ -0,0 +1,49 @@ +// This file is not generated by Speakeasy — it renders flag-parsing failures +// through the same path as every other CLI-level error. +// +// flagutil returns a typed *flagutil.FlagValueError but cannot format it: the +// output package already imports flagutil, so flagutil importing output would be +// an import cycle. cli.Execute is the first point that has both the resolved +// *cobra.Command and permission to import output, so the rendering happens here. +// +// In agent mode this reuses output.AgentModeError, which is already the accepted +// envelope for non-API CLI errors (see configure.go and auth.go) and is already +// asserted by test/errors-teach.test.ts. Human mode keeps the plain multi-line +// form, which reads better in a terminal than JSON. + +package cli + +import ( + "errors" + + "github.com/spf13/cobra" + "github.com/voiceflow/cli/internal/flagutil" + "github.com/voiceflow/cli/internal/output" +) + +// renderFlagValueError converts a flag-value failure into the structured agent +// envelope. Errors of any other kind are returned untouched, so this is safe to +// wrap around the whole command execution. +// +// The message handed to AgentModeError is deliberately one line: the detail +// belongs in hints[], where a consumer can read it as structured data rather +// than parsing it out of a sentence. +// +// The echoed value goes through DisplayValue, never the raw fve.Value. This +// render path is the one that matters most for that: it feeds an agent's +// context, and an unbounded echo of a malformed blob both floods that context +// and can reproduce a secret that happened to be inside it. +func renderFlagValueError(cmd *cobra.Command, err error) error { + var fve *flagutil.FlagValueError + if !errors.As(err, &fve) || !output.IsAgentMode() { + return err + } + + hints := fve.Hints + if v := fve.DisplayValue(); v != "" { + hints = append([]string{"you passed: " + v}, hints...) + } + + return output.AgentModeError(cmd, "invalid_flag_value", + "invalid value for --"+fve.Flag+": "+fve.Expected, hints) +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 9b2f9c6..72d7277 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -226,7 +226,7 @@ func Execute() error { return runExplorer(rootCmd) } - return rootCmd.Execute() + return renderFlagValueError(rootCmd, rootCmd.Execute()) // see flagerrors.go } // shouldAutoExplore returns true when the CLI is invoked with no subcommand diff --git a/internal/flagutil/description.go b/internal/flagutil/description.go new file mode 100644 index 0000000..730d876 --- /dev/null +++ b/internal/flagutil/description.go @@ -0,0 +1,37 @@ +// This file is not generated by Speakeasy — it stops a flag's description from +// renaming the flag in help output. +// +// The problem it solves: pflag's UnquoteUsage treats the first back-quoted word +// in a usage string as the flag's value placeholder, and strips the quotes from +// the prose. That is a deliberate pflag feature, but our descriptions come from +// the OpenAPI spec, where backticks are ordinary prose emphasis. The result: +// +// --instructions Name Markdown text. Backticked Name resolves to a tool... +// +// The flag does not want a name. It wants markdown. Twelve flags are affected +// today. The worst is transcript search's --version-param, whose only legal +// values are draft and published, rendered as `environmentAlias` — an unrelated +// parameter name that appears nowhere in its own list of options. +// +// Backticks are replaced rather than deleted so the emphasis the spec author +// intended survives: "Backticked 'Name' resolves to a tool" still reads as a +// reference to a token, while pflag now falls back to the flag's real type. +// +// Doing this at the single point where descriptions enter cobra fixes all twelve +// and every future one automatically. The alternative — editing twelve +// descriptions across eight generated files — would be twelve standing merge +// conflicts that silently stop covering a thirteenth flag the day the spec adds +// one. + +package flagutil + +import "strings" + +// flagDescription prepares an OpenAPI description for use as a pflag usage +// string, neutralizing backticks so they cannot be read as a value placeholder. +func flagDescription(description string) string { + if !strings.Contains(description, "`") { + return description + } + return strings.ReplaceAll(description, "`", "'") +} diff --git a/internal/flagutil/flagerror.go b/internal/flagutil/flagerror.go new file mode 100644 index 0000000..6f8bf3f --- /dev/null +++ b/internal/flagutil/flagerror.go @@ -0,0 +1,75 @@ +// This file is not generated by Speakeasy — it gives flag-parsing failures a +// typed shape so they can be rendered with the same care as API errors. +// +// The problem it solves: errors produced while interpreting a flag value never +// reach output.Error. They are returned raw from the generated RunE, and because +// the root command sets SilenceErrors, cmd/vf/main.go prints them with a bare +// Fprintln. So an agent that mistypes a flag gets one unstructured line, while +// an agent that gets a 401 back from the API gets a structured envelope with +// hints and a docs URL. That is backwards: mistyped flags are the more common +// failure, and they are the one the caller can actually act on. +// +// flagutil cannot import output — output already imports flagutil, so that +// direction is an import cycle. This type is therefore pure data. It is carried +// up to cli.Execute, which has both the *cobra.Command and permission to import +// output, and is rendered there. + +package flagutil + +import ( + "fmt" + "strings" +) + +// FlagValueError reports a flag whose value could not be interpreted. +// +// It follows the house style already used for date parsing in this package — +// state the expectation, show a worked example, echo what was actually passed — +// so the reader can see the difference between what they wrote and what was +// wanted without consulting documentation. +type FlagValueError struct { + Flag string // flag name, without leading dashes + Value string // what the user actually passed + Expected string // one line: what the flag wanted instead + Hints []string // concrete next actions, most likely fix first + Cause error // underlying parse error, preserved for errors.Is/As +} + +func (e *FlagValueError) Error() string { + var b strings.Builder + fmt.Fprintf(&b, "invalid value for --%s: %s", e.Flag, e.Expected) + if e.Value != "" { + fmt.Fprintf(&b, "\n you passed: %s", truncateForError(e.Value)) + } + for _, h := range e.Hints { + fmt.Fprintf(&b, "\n %s", h) + } + return b.String() +} + +func (e *FlagValueError) Unwrap() error { return e.Cause } + +// DisplayValue is the value as it should be shown to a reader: shortened, and +// with newlines flattened. Exported because internal/cli renders these errors +// too and must not echo the raw value — an unbounded echo would flood an agent's +// context and can reproduce secrets that happened to sit inside a malformed +// blob. Every render path must go through this, never through Value directly. +func (e *FlagValueError) DisplayValue() string { return truncateForError(e.Value) } + +// truncateForError keeps a long value from burying the explanation. Instructions +// and prompts are routinely thousands of characters; echoing one in full pushes +// the hints off the reader's screen, which defeats the point of the hints. It +// also bounds what a malformed value can spill into a log or an agent's context. +// +// Counting and cutting are done in runes, not bytes. Slicing a byte index splits +// multi-byte characters in half, which writes invalid UTF-8 to stderr and makes +// the reported length wrong for any non-ASCII value. +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)" +} diff --git a/internal/flagutil/jsonerror.go b/internal/flagutil/jsonerror.go new file mode 100644 index 0000000..9e077e6 --- /dev/null +++ b/internal/flagutil/jsonerror.go @@ -0,0 +1,115 @@ +// This file is not generated by Speakeasy — it explains why a structured JSON +// flag rejected the value it was given. +// +// The failure it replaces named the wrong thing twice over: +// +// 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. And when the input IS +// valid JSON of the wrong shape, the same message insisted it was not valid +// JSON, sending the reader to re-check syntax that was never wrong. Meanwhile +// the decoder's own explanation, the only thing that knows which Go type was +// expected, was discarded. + +package flagutil + +import ( + "encoding/json" + "fmt" + "reflect" + "strings" +) + +// notJSONError explains why a structured JSON flag rejected its value. +// +// Two distinct failures used to collapse into one message, and the merged +// wording was actively false for the second: +// +// --llm 'gpt-4' not JSON at all +// --llm '[1,2]' valid JSON, but this flag wants an object +// +// Telling someone whose input is well-formed JSON that it "must be valid JSON" +// sends them to re-check syntax that was never wrong. So the shape mismatch is +// named separately, and the unmarshal error — the only thing that knows which +// Go type was expected — is surfaced instead of being swallowed. +func jsonValueError(t reflect.Type, val string, m FlagMeta, cause error) error { + hints := []string{} + expected := "expected a JSON value" + + if json.Valid([]byte(val)) { + // Syntax is fine; the shape is not. Say that, rather than blaming syntax. + expected = "the value is valid JSON but not the shape this flag expects" + } else { + hints = append(hints, "the value is not valid JSON") + } + + 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} +} + +// jsonShapeExample returns a value of the right shape for t, or "" when the +// shape cannot be stated confidently. +// +// The example must be something the CLI would actually accept. The first +// version of this hint hardcoded an object for every destination, so on the +// array-valued flags it told the caller to pass a value the same binary +// rejects — and a caller following the instruction exactly got back the +// identical error, with nothing new to try. A wrong example is worse than no +// example, so an unrecognized shape returns "" and the hint is omitted. +func jsonShapeExample(t reflect.Type) string { + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + // OptionalNullable[T] == map[bool]*T — unwrap to the value it carries. + if t.Kind() == reflect.Map && t.Key().Kind() == reflect.Bool && t.Elem().Kind() == reflect.Ptr { + t = t.Elem().Elem() + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + } + + switch t.Kind() { + case reflect.Slice, reflect.Array: + return `[{"key":"value"}]` + case reflect.Map, reflect.Struct: + return `{"key":"value"}` + case reflect.String: + // A nullable string is JSON-typed so that null can be expressed at all, + // which is why prose has to arrive JSON-quoted. Nothing said so before; + // the error named the first character of the sentence and stopped. + return `"your text here"` + default: + return "" + } +} + +// unmarshalDetail surfaces what the JSON decoder objected to, minus a prefix +// that is wrong in this context. +// +// The SDK's unmarshaler labels every failure "error unmarshalling json response +// body", but a flag value is a request the CLI has not sent yet — there is no +// response. Left intact it sends the reader looking for a server problem that +// does not exist. The remainder is worth keeping: it names the Go type that was +// expected, which is the only place that information appears at all. +func unmarshalDetail(cause error) string { + if cause == nil { + return "" + } + detail := cause.Error() + for _, prefix := range []string{"error unmarshalling json response body: ", "json: "} { + detail = strings.TrimPrefix(detail, prefix) + } + if detail == "" { + return "" + } + return "decoder reported: " + detail +} diff --git a/internal/flagutil/metadata.go b/internal/flagutil/metadata.go index 3862141..a216714 100644 --- a/internal/flagutil/metadata.go +++ b/internal/flagutil/metadata.go @@ -105,6 +105,7 @@ type FlagMeta struct { // credentials) are skipped to prevent local flags from shadowing them. func RegisterFlags(cmd *cobra.Command, meta []FlagMeta) { for _, m := range meta { + m.Description = flagDescription(m.Description) // see description.go // Skip if a flag with this name already exists — either inherited from // a parent command (persistent globals/security) or already registered // locally (e.g., operation security flags like username/password). @@ -817,13 +818,13 @@ func buildJSONField(cmd *cobra.Command, v reflect.Value, m FlagMeta) error { holder := reflect.New(reflect.PtrTo(fieldType)) holder.Elem().Set(reflect.New(fieldType)) if err := utils.UnmarshalJsonFromString(val, holder.Interface(), m.Annotations); err != nil { - return fmt.Errorf("invalid value for --%s: %w", m.FlagName, err) + return jsonValueError(field.Type(), val, m, err) // see jsonerror.go } field.Set(holder.Elem()) } else { target := reflect.New(fieldType) if err := utils.UnmarshalJsonFromString(val, target.Interface(), m.Annotations); err != nil { - return fmt.Errorf("invalid value for --%s: %w", m.FlagName, err) + return jsonValueError(field.Type(), val, m, err) // see jsonerror.go } field.Set(target.Elem()) } diff --git a/internal/output/agentmode.go b/internal/output/agentmode.go index 8d53fd9..653e9f5 100644 --- a/internal/output/agentmode.go +++ b/internal/output/agentmode.go @@ -176,11 +176,11 @@ func AgentModeError(cmd *cobra.Command, errorType, message string, hints []strin jsonData, err := json.MarshalIndent(envelope, "", " ") if err != nil { fmt.Fprintln(out, message) - return fmt.Errorf("%s", message) + return AlreadyReported(fmt.Errorf("%s", message)) // see reported.go } // No colorization in agent mode. fmt.Fprintln(out, string(jsonData)) - return fmt.Errorf("%s", message) + return AlreadyReported(fmt.Errorf("%s", message)) // see reported.go } diff --git a/internal/output/output.go b/internal/output/output.go index 68b4fbf..8763109 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -404,7 +404,7 @@ func Error(cmd *cobra.Command, err error) error { printJSON(out, jsonData, colorize) - return err + return AlreadyReported(err) // see reported.go } // tryReadRawBody attempts to read the raw HTTP response body from the response diff --git a/internal/output/reported.go b/internal/output/reported.go new file mode 100644 index 0000000..816d1da --- /dev/null +++ b/internal/output/reported.go @@ -0,0 +1,55 @@ +// This file is not generated by Speakeasy — it stops errors that have already +// been written to stderr from being written to stderr a second time. +// +// The problem it solves: every error printer in this package prints AND returns +// the error, and cmd/vf/main.go then prints whatever Execute returns. So a +// single failure reaches stderr twice: +// +// CLAUDECODE=1 vf configure +// { "error": "…", "error_type": "configure_blocked", "hints": [ … ] } +// the 'configure' command is interactive and cannot be used in agent mode +// +// AgentModeError's own documentation states the contract this violates — +// "outputs structured JSON exactly once to stderr… Callers must NOT print the +// error again" — but main.go is generated and prints unconditionally, so the +// contract was never actually kept. +// +// This matters most in agent mode, where the envelope is the machine-readable +// product. A trailing non-JSON line after a JSON object is exactly what breaks +// a naive parser, and this CLI's whole purpose is being driven by agents. +// +// Human (pretty) output is deliberately left alone. There the trailing line is +// the readable one-line summary that follows a raw API body, so suppressing it +// would remove the most useful part of the message rather than a duplicate. + +package output + +// alreadyReported marks an error whose message has been written to stderr by +// the code that produced it. +// +// Error returns the empty string, which is what actually suppresses the second +// print: main.go skips errors with an empty message. The alternative — exporting +// a predicate for main.go to call — does not work, because main.go imports only +// internal/cli and cannot see this package. +// +// The wrapped error is preserved through Unwrap, so errors.Is and errors.As +// continue to work on anything a caller wants to inspect. +// +// The cost of this design, stated plainly: any future code that formats one of +// these errors with %v or err.Error() renders nothing. That is contained today +// because exactly one place formats them — main.go — and it is guarded. Anything +// that needs the text must call Unwrap first. +type alreadyReported struct{ err error } + +func (e alreadyReported) Error() string { return "" } +func (e alreadyReported) Unwrap() error { return e.err } + +// AlreadyReported marks err as having been printed, so the process exits +// non-zero without repeating the message. Returns nil for nil so it is safe to +// wrap a return value directly. +func AlreadyReported(err error) error { + if err == nil { + return nil + } + return alreadyReported{err: err} +} diff --git a/test/errors-teach.test.ts b/test/errors-teach.test.ts index f218484..f14324a 100644 --- a/test/errors-teach.test.ts +++ b/test/errors-teach.test.ts @@ -16,7 +16,14 @@ const AGENT_ENV = { CLAUDE_CODE: '1', VF_TOKEN: '' } as const; const $vf = (args: string[], env: Record = {}) => execa({ reject: false, env: { ...AGENT_ENV, ...env }, stdin: 'ignore' })(VF, args); -/** stderr of agent-mode errors is a JSON envelope followed by plain lines. */ +/** + * In agent mode stderr is the JSON envelope and nothing else. It used to be the + * envelope followed by a duplicate plain-text line, because every printer in + * internal/output prints AND returns the error while cmd/vf/main.go printed + * whatever Execute returned. This parser reads the first balanced object, so it + * tolerated the duplicate; the "is nothing but JSON" assertions below are what + * actually pin the single-print contract. + */ function parseEnvelope(stderr: string): Record { const start = stderr.indexOf('{'); expect(start, `no JSON envelope in stderr:\n${stderr}`).toBeGreaterThanOrEqual(0); @@ -150,3 +157,39 @@ describe('error hint injection', () => { expect(JSON.stringify(envelope.hints)).toContain('export VF_TOKEN=vfp_'); }); }); + +describe('agent-mode errors are printed exactly once', () => { + // Every printer in internal/output writes the error to stderr and also returns + // it, and main.go printed whatever Execute returned — so a single failure + // reached stderr twice. AgentModeError's own doc comment promised the + // opposite ("outputs structured JSON exactly once… Callers must NOT print the + // error again"), but main.go is generated and printed unconditionally. + // + // This matters because the envelope is the machine-readable product: a + // trailing non-JSON line after a JSON object is exactly what breaks a parser + // that does the obvious thing. + 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 () => { + const result = await $vf(args); + expect(result.exitCode, 'must still fail').not.toBe(0); + // The strict test: the whole stream parses, with no trailing prose. + expect(() => JSON.parse(result.stderr.trim()), `stderr was not pure JSON:\n${result.stderr}`).not.toThrow(); + }); + + it(`${name}: the message does not appear twice`, async () => { + const result = await $vf(args); + const envelope = parseEnvelope(result.stderr); + const message = String(envelope.message ?? envelope.error ?? ''); + expect(message.length, 'envelope carried no message to check').toBeGreaterThan(0); + const after = result.stderr.slice(result.stderr.lastIndexOf('}') + 1); + expect(after.trim(), `text printed after the envelope:\n${after}`).toBe(''); + }); + } +}); diff --git a/test/flag-errors.test.ts b/test/flag-errors.test.ts new file mode 100644 index 0000000..7c59e52 --- /dev/null +++ b/test/flag-errors.test.ts @@ -0,0 +1,216 @@ +// Tests for how flag failures are REPORTED, and how help describes a flag. +// +// Three things ship together here because they are one experience: what you see +// when a flag value is wrong. +// +// 1. The message names the shape, shows an example, and keeps the decoder's +// own explanation instead of discarding it. +// 2. What it echoes back is bounded. +// 3. Help shows the flag's real type, not a word borrowed from its prose. +// +// Parsing behaviour is unchanged: every value accepted before is accepted now, +// and every value rejected before is still rejected. Only the message differs. +// +// Every case runs the real binary with --dry-run, so nothing here touches the +// network. Requires: go build -o vf ./cmd/vf + +import { execa } from 'execa'; +import * as path from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const VF = path.resolve(__dirname, '..', 'vf'); + +const BASE = ['agent', 'update', '--project-id', 'p', '--environment-alias', 'main', '--dry-run', '--token', 'vfp_x']; + +// Every variable that puts the CLI into agent mode. Mirrors the list in +// internal/output/agentmode.go — if that grows and this does not, a human-mode +// test running on a machine that sets the new one would silently assert against +// agent output. assertMode below is the guard against exactly that drift. +const AGENT_ENV_VARS = [ + 'CLAUDECODE', 'CLAUDE_CODE', 'CURSOR_AGENT', 'CODEX', 'AIDER', 'CLINE', + 'WINDSURF_AGENT', 'GITHUB_COPILOT', 'AMAZON_Q', 'GEMINI_CODE_ASSIST', + 'SRC_CODY', 'FORCE_AGENT_MODE', +]; + +/** + * Runs vf in a known output mode. + * + * Human mode has to clear ALL of AGENT_ENV_VARS, not just the obvious few: CI + * runs on GitHub, where GITHUB_COPILOT may well be set, and a stray one silently + * flips the CLI into agent mode so the assertions check the wrong renderer. + */ +function run(args: string[], opts: { agentMode?: boolean } = {}) { + const env: Record = Object.fromEntries( + AGENT_ENV_VARS.map((name) => [name, undefined]), + ); + if (opts.agentMode) env.CLAUDECODE = '1'; + return execa({ reject: false, timeout: 20_000, stdin: 'ignore', env, extendEnv: true })(VF, args); +} + +/** Fails loudly if the CLI rendered in the mode we did not ask for. */ +function assertMode(stderr: string, mode: 'human' | 'agent') { + const looksLikeAgent = stderr.trimStart().startsWith('{'); + expect( + looksLikeAgent, + `expected ${mode} output but got the other renderer — a new agent-detection env var is probably set and missing from AGENT_ENV_VARS:\n${stderr.slice(0, 200)}`, + ).toBe(mode === 'agent'); +} + +/** Pull a field back out of the --dry-run request preview. */ +function sent(output: string, field: string): string | null { + const m = output.match(new RegExp(`"${field}":\\s*(null|"(?:[^"\\\\]|\\\\.)*")`)); + return m ? m[1] : null; +} + +describe('nullability is unaffected', () => { + // These fields are JSON-typed precisely so that null can be expressed. Both + // hold on master and must keep holding: they are the reason the encoding is + // what it is, and the constraint any future relaxation has to respect. + it('--instructions null still sends a real JSON null', async () => { + const r = await run([...BASE, '--instructions', 'null']); + expect(sent(r.stderr + r.stdout, 'instructions')).toBe('null'); + }); + + it('an explicitly quoted JSON string is passed through unchanged', async () => { + const r = await run([...BASE, '--instructions', '"already json"']); + expect(sent(r.stderr + r.stdout, 'instructions')).toBe('"already json"'); + }); +}); + +describe('a string-valued flag says how to quote it', () => { + // The motivating failure. Prose still has to be JSON-quoted here, but the + // error now names the shape instead of naming the first character of the + // sentence and stopping. + it('offers a quoting example, and following it works', async () => { + const rejected = await run([...BASE, '--instructions', 'You are a support agent.']); + expect(rejected.exitCode).not.toBe(0); + const suggestion = (rejected.stderr + rejected.stdout).match( + /expected shape: --instructions '(.+)'/, + )?.[1]; + expect(suggestion, 'no quoting example offered').toBe('"your text here"'); + + const retry = await run([...BASE, '--instructions', '"You are a support agent."']); + expect(sent(retry.stderr + retry.stdout, 'instructions')).toBe('"You are a support agent."'); + }); +}); + +describe('structured flags still reject raw text', () => { + // The fallback must not become "anything goes". A mistyped object is still an + // error, because storing prose in a field that models an object is nonsense. + for (const flag of ['llm', 'knowledge-base-tool']) { + it(`--${flag} fails rather than silently storing text`, async () => { + const r = await run([...BASE, `--${flag}`, 'not json at all']); + expect(r.exitCode).not.toBe(0); + expect(r.stderr).toContain(`invalid value for --${flag}`); + }); + } + + it('explains what was wanted, echoes what was passed, and shows a worked example', async () => { + const r = await run([...BASE, '--llm', 'gpt-4']); + assertMode(r.stderr, 'human'); + expect(r.stderr).toContain('expected a JSON value'); + expect(r.stderr).toContain('you passed: gpt-4'); + expect(r.stderr).toContain(`--llm '{"key":"value"}'`); + expect(r.stderr).toContain('--llm null'); + }); + + it('emits a structured envelope in agent mode', async () => { + const r = await run([...BASE, '--llm', 'gpt-4'], { agentMode: true }); + assertMode(r.stderr, 'agent'); + const json = JSON.parse(r.stderr.slice(r.stderr.indexOf('{'), r.stderr.lastIndexOf('}') + 1)); + expect(json.error_type).toBe('invalid_flag_value'); + expect(json.error).toContain('invalid value for --llm'); + expect(json.hints.join(' ')).toContain('you passed: gpt-4'); + }); +}); + +describe('help shows the flag type, not a word from its description', () => { + // pflag reads the first back-quoted word in a usage string as the value + // placeholder. Descriptions come from the OpenAPI spec, where backticks are + // prose emphasis, so --instructions used to render as `--instructions Name`. + const cases: Array<[cmd: string[], flag: string, wrong: string]> = [ + [['agent', 'update'], 'instructions', 'Name'], + [['agent', 'update'], 'prompt', 'Name'], + [['transcript', 'search'], 'version-param', 'environmentAlias'], + ]; + + for (const [cmd, flag, wrong] of cases) { + it(`${cmd.join(' ')} --${flag} is labelled string, not ${wrong}`, async () => { + const r = await run([...cmd, '--help']); + const line = (r.stdout + r.stderr).split('\n').find((l) => l.includes(`--${flag} `)); + expect(line, `no help line for --${flag}`).toBeDefined(); + expect(line).toContain(`--${flag} string`); + expect(line).not.toContain(`--${flag} ${wrong}`); + }); + } + + it('keeps the description prose readable after backticks are neutralized', async () => { + const r = await run(['agent', 'update', '--help']); + expect(r.stdout + r.stderr).toContain("Backticked 'Name' resolves to"); + }); +}); + +describe('rejection messages are actionable, not circular', () => { + // The strongest form of this test: take the CLI's own suggestion and feed it + // back in. An earlier version hardcoded an object example for every JSON + // destination, so on array-valued flags it told the caller to pass a value the + // same binary rejects — following the instruction exactly reproduced the + // identical error, with nothing new to try. For an agent that is a loop. + const shaped: Array<[flag: string, extraArgs: string[]]> = [ + ['playbooks', []], // slice-valued -> must suggest an array + ['llm', []], // map-valued -> must suggest an object + ]; + + for (const [flag, extra] of shaped) { + it(`--${flag}: the suggested example is one the CLI accepts`, async () => { + const rejected = await run([...BASE, ...extra, `--${flag}`, 'not json at all']); + const suggestion = (rejected.stderr + rejected.stdout).match( + new RegExp(`expected shape: --${flag} '(.+)'`), + )?.[1]; + expect(suggestion, `no shape hint offered for --${flag}`).toBeDefined(); + + // Do exactly what the CLI said to do. It must not fail the same way. + const retry = await run([...BASE, ...extra, `--${flag}`, suggestion!]); + expect(retry.stderr, `the CLI's own suggestion ${suggestion} was rejected`).not.toContain( + `invalid value for --${flag}`, + ); + }); + } + + it('does not claim the input is invalid JSON when it is valid JSON', async () => { + // '[1,2]' parses fine; it is the wrong shape for a map-valued flag. Saying + // "not valid JSON" sends the reader to re-check syntax that was never wrong. + const r = await run([...BASE, '--llm', '[1,2]']); + expect(r.stderr).toContain('valid JSON but not the shape'); + expect(r.stderr).not.toContain('the value is not valid JSON'); + }); + + it('surfaces what the decoder objected to, without the misleading response-body prefix', async () => { + const r = await run([...BASE, '--llm', '[1,2]']); + expect(r.stderr).toContain('cannot unmarshal array'); + // A flag value is a request that was never sent; there is no response body. + expect(r.stderr).not.toContain('response body'); + }); +}); + +describe('echoed values are bounded', () => { + const big = 'x'.repeat(5_000); + + // The echo exists to reveal shell-quoting mistakes, so it stays — but an + // unbounded one floods an agent's context and reproduces whatever sat inside a + // malformed blob. Agent mode originally bypassed the cap entirely. + for (const mode of ['human', 'agent'] as const) { + it(`${mode} mode: a 5000-char value does not produce a 5000-char error`, async () => { + const r = await run([...BASE, '--llm', big], { agentMode: mode === 'agent' }); + expect(r.stderr.length, `error grew with the input (${r.stderr.length} bytes)`).toBeLessThan(1_500); + expect(r.stderr).toContain('chars)'); // says how long it really was + }); + } + + it('counts and cuts in characters, not bytes', async () => { + // Byte-slicing splits a multi-byte rune and misreports the length. + const r = await run([...BASE, '--llm', 'あ'.repeat(200)]); + expect(r.stderr).toContain('(200 chars)'); + expect(r.stderr).not.toContain('(600 chars)'); + }); +});