From 14739bf1b7c7a6a0f46417c51f5c9d4285371fb5 Mon Sep 17 00:00:00 2001 From: BR <51544548+Bradenream@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:18:31 -0400 Subject: [PATCH 1/2] feat: accept raw text for JSON flags whose destination is a string (COR-13656) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split out of #21 as the contested half, stacked on the errors-only PR so it can be judged on its own. Fields marked `nullable: true` generate as FlagKindJSON, because JSON is the only encoding that expresses a real null distinctly from the string "null". Correct, and the reason --instructions null clears the field. The cost is that prose has to arrive JSON-quoted, and the workaround is quotes inside quotes — exactly what shell users and coding agents get wrong: vf agent update --instructions '"You are a support agent for Acme."' When a JSON flag fails to parse and its destination ultimately holds a Go string, the value is re-encoded as a JSON string and retried. 22 of 82 JSON flags qualify. Nullability is untouched: the null branch runs earlier. On the review objection — that this accepts a string for Markup fields, and so belongs at the API level rather than downstream in the generated CLI. Both halves are testable, and both are now pinned by tests rather than argued: No field's type changes. The retry only fires where the spec already declares `type: string`; the request body is byte-identical to what the JSON-quoted form produced, and to master. This is how the CLI reads a flag, not what the API accepts. Verified across plain text, quoted JSON, null, objects, arrays, empty and numeric input. Markup is unreachable. Markup is a union struct, and every field holding one holds it as a struct or a slice of them; stringLikeJSONTarget admits neither. --url on mcp-server create is []components.Markup and still rejects raw text. There is also no property in .speakeasy/out.openapi.yaml that references Markup at all. There is correspondingly nothing to fix upstream: the spec is already right. What is awkward is the mapping from "nullable string" to a JSON-encoded flag, which is codegen, not API design. The discriminator has to unwrap OptionalNullable[T], which is map[bool]*T and so indistinguishable from map[string]any by Kind alone. Requiring a bool key admits OptionalNullable[string] and rejects every ordinary map. Unit-tested against all 13 shapes that occur, including map[string]string and OptionalNullable[[]string]. The trade, stated plainly: for those 22 flags, malformed JSON that used to error is now stored as literal text. Right for markdown authors, wrong for someone who meant JSON and typo'd. Structs, maps and slices are excluded, so a mistyped object still fails loudly. One test from the errors-only PR is updated rather than kept: it asserted that --instructions rejects prose and explains how to quote it. That rejection is what this change removes, so the test now pins the behaviour that replaced it. Verified: 7 new tests plus the updated one; full suite shows the same pre-existing failures as master. --- internal/flagutil/metadata.go | 4 +- internal/flagutil/rawtext.go | 126 ++++++++++++++++++++++++++++++++++ test/flag-errors.test.ts | 23 +++---- test/flag-raw-text.test.ts | 91 ++++++++++++++++++++++++ 4 files changed, 228 insertions(+), 16 deletions(-) create mode 100644 internal/flagutil/rawtext.go create mode 100644 test/flag-raw-text.test.ts diff --git a/internal/flagutil/metadata.go b/internal/flagutil/metadata.go index a216714..c9daea9 100644 --- a/internal/flagutil/metadata.go +++ b/internal/flagutil/metadata.go @@ -818,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 jsonValueError(field.Type(), val, m, err) // see jsonerror.go + return setJSONFieldAsRawText(field, fieldType, isPtr, val, m, err) // see rawtext.go } field.Set(holder.Elem()) } else { target := reflect.New(fieldType) if err := utils.UnmarshalJsonFromString(val, target.Interface(), m.Annotations); err != nil { - return jsonValueError(field.Type(), val, m, err) // see jsonerror.go + return setJSONFieldAsRawText(field, fieldType, isPtr, val, m, err) // see rawtext.go } field.Set(target.Elem()) } diff --git a/internal/flagutil/rawtext.go b/internal/flagutil/rawtext.go new file mode 100644 index 0000000..74fc032 --- /dev/null +++ b/internal/flagutil/rawtext.go @@ -0,0 +1,126 @@ +// This file is not generated by Speakeasy — it lets flags that hold prose +// accept prose. +// +// A field marked `nullable: true` in the OpenAPI spec is generated as +// FlagKindJSON, because JSON is the only encoding that expresses a real null +// distinctly from the four-letter string "null". That is correct, and it is why +// --instructions null clears the field instead of setting it to the word. But +// it also means the obvious invocation fails, and the workaround is quotes +// inside quotes — exactly what shell users and coding agents get wrong: +// +// vf agent update --instructions '"You are a support agent for Acme."' +// +// So: when a JSON flag fails to parse AND its destination ultimately holds a Go +// string, re-encode the raw text as a JSON string and use that. Nullability is +// preserved because the null branch runs earlier, in buildJSONField. +// +// WHAT THIS DOES NOT DO. It does not change any field's type, and it cannot +// reach a Markup field: Markup is a union struct, every field holding one holds +// it as a struct or a slice, and stringLikeJSONTarget admits neither. The bytes +// on the wire are identical to what the JSON-quoted form already produced — +// this is an input-encoding change in the CLI, not an API change. Both claims +// are pinned by tests. +// +// The precedent is in buildJSONField already: reflect.Interface destinations +// have done exactly this ("Not valid JSON — treat as raw string") since before +// this file existed. +// +// The trade, stated plainly: for these flags, malformed JSON that used to fail +// now succeeds as literal text. That is right for someone writing markdown — +// the overwhelmingly common case for these fields — and wrong for someone who +// meant to pass JSON and typo'd. Structs, maps and slices are excluded, so a +// mistyped object still fails loudly everywhere it would be meaningless as text. + +package flagutil + +import ( + "encoding/json" + "fmt" + "reflect" + + "github.com/voiceflow/cli/internal/sdk/sdkinternal/utils" +) + +// stringLikeJSONTarget reports whether a FlagKindJSON destination ultimately +// holds a Go string, unwrapping pointers and OptionalNullable[T]. +// +// The OptionalNullable clause is the load-bearing part. OptionalNullable[T] is +// defined as map[bool]*T, so its reflect.Kind is Map — indistinguishable by kind +// alone from map[string]any. Requiring a bool key admits OptionalNullable[string] +// and rejects every ordinary map. Checking Kind() == String alone would match +// nothing at all, since no FlagKindJSON flag targets a bare string. +func stringLikeJSONTarget(t reflect.Type) bool { + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + // OptionalNullable[T] == map[bool]*T + 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() + } + } + return t.Kind() == reflect.String +} + +// setJSONFieldAsRawText is the recovery path for a JSON flag whose value did not +// parse. For string-valued destinations it retries with the input quoted as a +// JSON string; for everything else it converts the parse failure into a +// FlagValueError that explains what the flag actually wanted. +// +// The retry always builds a fresh destination rather than reusing the one the +// failed attempt wrote into: a partial unmarshal may already have mutated it, +// and silently shipping a half-populated value would be worse than the error +// this replaces. +func setJSONFieldAsRawText(field reflect.Value, fieldType reflect.Type, isPtr bool, val string, m FlagMeta, cause error) error { + if !stringLikeJSONTarget(field.Type()) { + return jsonValueError(field.Type(), val, m, cause) + } + + // json.Marshal of a string cannot fail, but handle it rather than ignore it. + quoted, err := json.Marshal(val) + if err != nil { + return &FlagValueError{ + Flag: m.FlagName, + Value: val, + Expected: "expected text or a JSON string", + Cause: cause, + } + } + + if isPtr { + holder := reflect.New(reflect.PtrTo(fieldType)) + holder.Elem().Set(reflect.New(fieldType)) + if err := utils.UnmarshalJsonFromString(string(quoted), holder.Interface(), m.Annotations); err != nil { + return rawTextRetryFailed(m, val, err, cause) + } + field.Set(holder.Elem()) + return nil + } + + target := reflect.New(fieldType) + if err := utils.UnmarshalJsonFromString(string(quoted), target.Interface(), m.Annotations); err != nil { + return rawTextRetryFailed(m, val, err, cause) + } + field.Set(target.Elem()) + return nil +} + +// rawTextRetryFailed reports a destination that looked string-shaped but refused +// a JSON string anyway. Reaching this means stringLikeJSONTarget and the SDK's +// unmarshaler disagree, so it names that explicitly instead of blaming the input. +// +// retryErr is the error from the quoted retry — the one that describes the +// disagreement. The first version reported the original parse error here, which +// only ever says "this text is not JSON": true, expected, and useless for +// diagnosing why the retry failed. The original is kept as the wrapped Cause so +// the full sequence is still recoverable. +func rawTextRetryFailed(m FlagMeta, val string, retryErr, cause error) error { + return &FlagValueError{ + Flag: m.FlagName, + Value: val, + Expected: fmt.Sprintf("could not be set from text (%v)", retryErr), + Hints: []string{"this is a bug in the CLI — please report it with the command you ran"}, + Cause: cause, + } +} diff --git a/test/flag-errors.test.ts b/test/flag-errors.test.ts index 7c59e52..3e0b3c5 100644 --- a/test/flag-errors.test.ts +++ b/test/flag-errors.test.ts @@ -77,20 +77,15 @@ describe('nullability is unaffected', () => { }); }); -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('a string-valued flag accepts prose directly', () => { + // In the errors-only PR this flag rejected prose and the test asserted that + // the error said how to quote it. The raw-text fallback removes the rejection, + // so what is pinned here is the behaviour that replaced it. The quoting + // example still exists for the flags the fallback does not cover. + it('takes prose without JSON quoting', async () => { + const r = await run([...BASE, '--instructions', 'You are a support agent.']); + expect(r.exitCode).toBe(0); + expect(sent(r.stderr + r.stdout, 'instructions')).toBe('"You are a support agent."'); }); }); diff --git a/test/flag-raw-text.test.ts b/test/flag-raw-text.test.ts new file mode 100644 index 0000000..e5c4e5c --- /dev/null +++ b/test/flag-raw-text.test.ts @@ -0,0 +1,91 @@ +// Tests for the raw-text fallback: JSON-typed flags whose destination is a Go +// string accept prose without JSON quoting. +// +// The two assertions this change lives or dies on are at the bottom: the wire +// payload is unchanged, and Markup fields are untouched. Both were raised in +// review and both are pinned here rather than argued. +// +// 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']; + +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', +]; + +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); +} + +function sent(output: string, field: string): string | null { + const m = output.match(new RegExp(`"${field}":\\s*(null|"(?:[^"\\\\]|\\\\.)*")`)); + return m ? m[1] : null; +} + +describe('string-valued JSON flags accept raw text', () => { + it('takes markdown prose without JSON quoting', async () => { + const r = await run([...BASE, '--instructions', 'You are a support agent for Acme.']); + expect(sent(r.stderr + r.stdout, 'instructions')).toBe('"You are a support agent for Acme."'); + }); + + it('preserves newlines and quotes in prose', async () => { + const r = await run([...BASE, '--instructions', 'Line one\nSay "hello" politely.']); + expect(sent(r.stderr + r.stdout, 'instructions')).toBe('"Line one\\nSay \\"hello\\" politely."'); + }); + + // The reason these flags are JSON-typed at all. If this regresses, the fallback + // has swallowed the one case the JSON encoding exists to express. + it('still sends a real JSON null for --instructions null', async () => { + const r = await run([...BASE, '--instructions', 'null']); + expect(sent(r.stderr + r.stdout, 'instructions')).toBe('null'); + }); + + it('leaves an explicitly quoted JSON string unchanged', async () => { + const r = await run([...BASE, '--instructions', '"already json"']); + expect(sent(r.stderr + r.stdout, 'instructions')).toBe('"already json"'); + }); + + // Documented edge case: valid JSON of the wrong shape is indistinguishable + // from prose, so it is stored as text. Asserted so the trade stays deliberate. + it('stores a JSON object passed to a text field as literal text', async () => { + const r = await run([...BASE, '--instructions', '{"note":"hi"}']); + expect(sent(r.stderr + r.stdout, 'instructions')).toBe('"{\\"note\\":\\"hi\\"}"'); + }); +}); + +describe('the fallback changes encoding, not types', () => { + // Raised in review: that this makes Markup fields accept a string, and so + // belongs at the API level. Neither half holds, and both are checked here + // rather than asserted in a comment. + + it('produces the same bytes as the JSON-quoted form', async () => { + const raw = await run([...BASE, '--instructions', 'You are a support agent.']); + const quoted = await run([...BASE, '--instructions', '"You are a support agent."']); + const a = sent(raw.stderr + raw.stdout, 'instructions'); + expect(a, 'raw text did not reach the body').toBe('"You are a support agent."'); + expect(a, 'the two input forms disagree on the wire').toBe(sent(quoted.stderr + quoted.stdout, 'instructions')); + }); + + // --url on mcp-server create is []components.Markup. Markup is a union struct, + // so stringLikeJSONTarget rejects it and the fallback never runs. If this ever + // starts passing, the change has grown past what it was reviewed as. + it('leaves Markup-valued flags strict', async () => { + const r = await run([ + 'mcp-server', 'create', '--project-id', 'p', '--environment-alias', 'main', + '--dry-run', '--token', 'vfp_x', '--name', 'n', '--url', 'not json', + ]); + expect(r.stderr, 'a Markup field accepted raw text').toContain('invalid value for --url'); + }); +}); From 9537cb834f8d6e0f7cb11e5836d6db9907880219 Mon Sep 17 00:00:00 2001 From: Ben Teichman Date: Fri, 4 Sep 2026 11:49:54 -0400 Subject: [PATCH 2/2] ci: convert schema before marshalling response (#27) --- internal/flagutil/metadata.go | 44 +++++++++++- internal/flagutil/rawtext.go | 126 ---------------------------------- 2 files changed, 42 insertions(+), 128 deletions(-) delete mode 100644 internal/flagutil/rawtext.go diff --git a/internal/flagutil/metadata.go b/internal/flagutil/metadata.go index c9daea9..06a08a7 100644 --- a/internal/flagutil/metadata.go +++ b/internal/flagutil/metadata.go @@ -800,6 +800,23 @@ func buildJSONField(cmd *cobra.Command, v reflect.Value, m FlagMeta) error { return nil } + // Nullable string fields land in this builder because OptionalNullable[T] + // needs three states (unset / null / value), which would otherwise force + // users to write --flag '"foo"'. Accept plain text too: keep the value as-is + // when it already parses as a JSON string, otherwise encode the raw text. + // Bare `null` is handled above and still means null, so a string whose + // literal value is `null` (or is itself quoted) must go through --body. + if targetsStringValue(fieldType) { + var s string + if err := json.Unmarshal([]byte(val), &s); err != nil { + encoded, encErr := json.Marshal(val) + if encErr != nil { + return fmt.Errorf("invalid value for --%s: %w", m.FlagName, encErr) + } + val = string(encoded) + } + } + // If the annotation specifies bigint:"string" or decimal:"string", the SDK's // unmarshalValue expects the value as a JSON string (e.g., "123"), not a bare // number. Wrap bare numbers in JSON quotes for user convenience. @@ -818,13 +835,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 setJSONFieldAsRawText(field, fieldType, isPtr, val, m, err) // see rawtext.go + 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 setJSONFieldAsRawText(field, fieldType, isPtr, val, m, err) // see rawtext.go + return jsonValueError(field.Type(), val, m, err) // see jsonerror.go } field.Set(target.Elem()) } @@ -832,6 +849,29 @@ func buildJSONField(cmd *cobra.Command, v reflect.Value, m FlagMeta) error { return nil } +// targetsStringValue reports whether a field ultimately holds a string, seeing +// through pointers and the map[bool]*T representation of +// optionalnullable.OptionalNullable. String-based enum types report true, since +// they share their underlying kind. +func targetsStringValue(t reflect.Type) bool { + for { + switch t.Kind() { + case reflect.Ptr: + t = t.Elem() + case reflect.Map: + // OptionalNullable[T] is map[bool]*T; a genuine map is not a string. + if t.Key().Kind() != reflect.Bool { + return false + } + t = t.Elem() + case reflect.String: + return true + default: + return false + } + } +} + func buildFileField(cmd *cobra.Command, v reflect.Value, m FlagMeta) error { path, changed := GetStringFlag(cmd, m.FlagName) diff --git a/internal/flagutil/rawtext.go b/internal/flagutil/rawtext.go deleted file mode 100644 index 74fc032..0000000 --- a/internal/flagutil/rawtext.go +++ /dev/null @@ -1,126 +0,0 @@ -// This file is not generated by Speakeasy — it lets flags that hold prose -// accept prose. -// -// A field marked `nullable: true` in the OpenAPI spec is generated as -// FlagKindJSON, because JSON is the only encoding that expresses a real null -// distinctly from the four-letter string "null". That is correct, and it is why -// --instructions null clears the field instead of setting it to the word. But -// it also means the obvious invocation fails, and the workaround is quotes -// inside quotes — exactly what shell users and coding agents get wrong: -// -// vf agent update --instructions '"You are a support agent for Acme."' -// -// So: when a JSON flag fails to parse AND its destination ultimately holds a Go -// string, re-encode the raw text as a JSON string and use that. Nullability is -// preserved because the null branch runs earlier, in buildJSONField. -// -// WHAT THIS DOES NOT DO. It does not change any field's type, and it cannot -// reach a Markup field: Markup is a union struct, every field holding one holds -// it as a struct or a slice, and stringLikeJSONTarget admits neither. The bytes -// on the wire are identical to what the JSON-quoted form already produced — -// this is an input-encoding change in the CLI, not an API change. Both claims -// are pinned by tests. -// -// The precedent is in buildJSONField already: reflect.Interface destinations -// have done exactly this ("Not valid JSON — treat as raw string") since before -// this file existed. -// -// The trade, stated plainly: for these flags, malformed JSON that used to fail -// now succeeds as literal text. That is right for someone writing markdown — -// the overwhelmingly common case for these fields — and wrong for someone who -// meant to pass JSON and typo'd. Structs, maps and slices are excluded, so a -// mistyped object still fails loudly everywhere it would be meaningless as text. - -package flagutil - -import ( - "encoding/json" - "fmt" - "reflect" - - "github.com/voiceflow/cli/internal/sdk/sdkinternal/utils" -) - -// stringLikeJSONTarget reports whether a FlagKindJSON destination ultimately -// holds a Go string, unwrapping pointers and OptionalNullable[T]. -// -// The OptionalNullable clause is the load-bearing part. OptionalNullable[T] is -// defined as map[bool]*T, so its reflect.Kind is Map — indistinguishable by kind -// alone from map[string]any. Requiring a bool key admits OptionalNullable[string] -// and rejects every ordinary map. Checking Kind() == String alone would match -// nothing at all, since no FlagKindJSON flag targets a bare string. -func stringLikeJSONTarget(t reflect.Type) bool { - for t.Kind() == reflect.Ptr { - t = t.Elem() - } - // OptionalNullable[T] == map[bool]*T - 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() - } - } - return t.Kind() == reflect.String -} - -// setJSONFieldAsRawText is the recovery path for a JSON flag whose value did not -// parse. For string-valued destinations it retries with the input quoted as a -// JSON string; for everything else it converts the parse failure into a -// FlagValueError that explains what the flag actually wanted. -// -// The retry always builds a fresh destination rather than reusing the one the -// failed attempt wrote into: a partial unmarshal may already have mutated it, -// and silently shipping a half-populated value would be worse than the error -// this replaces. -func setJSONFieldAsRawText(field reflect.Value, fieldType reflect.Type, isPtr bool, val string, m FlagMeta, cause error) error { - if !stringLikeJSONTarget(field.Type()) { - return jsonValueError(field.Type(), val, m, cause) - } - - // json.Marshal of a string cannot fail, but handle it rather than ignore it. - quoted, err := json.Marshal(val) - if err != nil { - return &FlagValueError{ - Flag: m.FlagName, - Value: val, - Expected: "expected text or a JSON string", - Cause: cause, - } - } - - if isPtr { - holder := reflect.New(reflect.PtrTo(fieldType)) - holder.Elem().Set(reflect.New(fieldType)) - if err := utils.UnmarshalJsonFromString(string(quoted), holder.Interface(), m.Annotations); err != nil { - return rawTextRetryFailed(m, val, err, cause) - } - field.Set(holder.Elem()) - return nil - } - - target := reflect.New(fieldType) - if err := utils.UnmarshalJsonFromString(string(quoted), target.Interface(), m.Annotations); err != nil { - return rawTextRetryFailed(m, val, err, cause) - } - field.Set(target.Elem()) - return nil -} - -// rawTextRetryFailed reports a destination that looked string-shaped but refused -// a JSON string anyway. Reaching this means stringLikeJSONTarget and the SDK's -// unmarshaler disagree, so it names that explicitly instead of blaming the input. -// -// retryErr is the error from the quoted retry — the one that describes the -// disagreement. The first version reported the original parse error here, which -// only ever says "this text is not JSON": true, expected, and useless for -// diagnosing why the retry failed. The original is kept as the wrapped Cause so -// the full sequence is still recoverable. -func rawTextRetryFailed(m FlagMeta, val string, retryErr, cause error) error { - return &FlagValueError{ - Flag: m.FlagName, - Value: val, - Expected: fmt.Sprintf("could not be set from text (%v)", retryErr), - Hints: []string{"this is a bug in the CLI — please report it with the command you ran"}, - Cause: cause, - } -}