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)'); + }); +});