From 5c92250cd450ad17f505d8286efedada580a70e5 Mon Sep 17 00:00:00 2001 From: Ben Teichman Date: Thu, 3 Sep 2026 14:05:54 -0400 Subject: [PATCH] ci: convert schema before marshalling response --- 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, - } -}