From 3954c05381dbcd30851ae7a57ffa2e24fc1f240d Mon Sep 17 00:00:00 2001 From: Braden Ream <51544548+Bradenream@users.noreply.github.com> Date: Thu, 1 Oct 2026 18:27:09 -0400 Subject: [PATCH 1/3] fix: let string flags whose field is an object take JSON The generated metadata declares some expanded union-variant flags as plain strings although their fields are objects or nullable strings. A survey of all 38 expanded variant fields found 11: - test turn create --body-param.agent.payload (an object) - integration connect --body-param..credentials, for twilio, ujet, genesys, kustomer, dixa, sunshine and custom-handoff (objects) - evaluation create --body-param..description, for boolean, number and string (nullable strings) setFieldByPath cannot store text in those fields, so every one failed with "cannot convert string to ..." whatever value it was given. Only --body, or the variant's whole-JSON flag, could set them. buildStringField now hands such a flag to buildJSONField, the builder JSON flags use. An object takes JSON, and a nullable string takes plain text or null. Flags whose field holds text are unchanged. All 11 were checked on the built binary: each failed on master and now sends its value. --- internal/flagutil/metadata.go | 4 + internal/flagutil/stringflag.go | 48 +++++++++++ internal/flagutil/stringflag_test.go | 91 ++++++++++++++++++++ test/union-variant-flags.test.ts | 120 +++++++++++++++++++++++++++ 4 files changed, 263 insertions(+) create mode 100644 internal/flagutil/stringflag.go create mode 100644 internal/flagutil/stringflag_test.go create mode 100644 test/union-variant-flags.test.ts diff --git a/internal/flagutil/metadata.go b/internal/flagutil/metadata.go index 06a08a7b..5718bb88 100644 --- a/internal/flagutil/metadata.go +++ b/internal/flagutil/metadata.go @@ -540,6 +540,10 @@ func buildStringField(cmd *cobra.Command, v reflect.Value, m FlagMeta) error { if shouldSkipDefault(m, changed) { return nil } + // A string flag whose field cannot hold text takes JSON. See stringflag.go. + if !fieldHoldsText(v.Type(), m.FieldPath) { + return buildJSONField(cmd, v, m) + } return setFieldByPath(v, m.FieldPath, reflect.ValueOf(val)) } diff --git a/internal/flagutil/stringflag.go b/internal/flagutil/stringflag.go new file mode 100644 index 00000000..fa1d0e5f --- /dev/null +++ b/internal/flagutil/stringflag.go @@ -0,0 +1,48 @@ +// This file is not generated by Speakeasy. It lets a string flag carry JSON +// when the field it fills cannot hold text. +// +// The generated metadata declares some flags as plain strings although their +// fields are objects or nullable strings. Every one found is an expanded union +// variant field: +// +// vf test turn create --body-param.agent.payload an object +// vf integration connect --body-param..credentials objects, 7 flags +// vf evaluation create --body-param..description nullable strings, 3 flags +// +// setFieldByPath cannot store text in those fields, so each of them failed +// with "cannot convert string to ..." whatever value it was given. Such a flag +// now takes its value the way a JSON flag does, through buildJSONField: JSON +// for an object, and plain text or null for a nullable string. A flag whose +// field holds text is unchanged. + +package flagutil + +import ( + "reflect" + "strings" +) + +// fieldHoldsText reports whether the field at path in t can be set from a +// string flag's text, by the conversion setFieldByPath makes. A path that does +// not resolve reports true, leaving setFieldByPath to describe the problem. +func fieldHoldsText(t reflect.Type, path string) bool { + for _, name := range strings.Split(path, ".") { + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return true + } + field, ok := t.FieldByName(name) + if !ok { + return true + } + t = field.Type + } + // setFieldByPath allocates the pointer of an optional field and converts + // into what it points to. + if t.Kind() == reflect.Ptr { + t = t.Elem() + } + return reflect.TypeOf("").ConvertibleTo(t) +} diff --git a/internal/flagutil/stringflag_test.go b/internal/flagutil/stringflag_test.go new file mode 100644 index 00000000..f0ddb86a --- /dev/null +++ b/internal/flagutil/stringflag_test.go @@ -0,0 +1,91 @@ +package flagutil + +import ( + "reflect" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/voiceflow/cli/internal/sdk/optionalnullable" +) + +// turnPayload is shaped like test turn create's agent payload. +type turnPayload struct { + Sequential bool `json:"sequential"` +} + +type turnName string + +// stringFlagTarget has a field of each shape a generated string flag fills. +type stringFlagTarget struct { + Name string `json:"name"` + Kind turnName `json:"kind"` + Optional *string `json:"optional,omitempty"` + Payload turnPayload `json:"payload"` + Note optionalnullable.OptionalNullable[string] `json:"note,omitzero"` + Nested *struct{ Name string } `json:"nested,omitempty"` +} + +func TestFieldHoldsText(t *testing.T) { + target := reflect.TypeFor[stringFlagTarget]() + for path, want := range map[string]bool{ + "Name": true, + "Kind": true, + "Optional": true, + "Nested.Name": true, + "Payload": false, + "Note": false, + "NotAField": true, // setFieldByPath reports it + } { + if got := fieldHoldsText(target, path); got != want { + t.Errorf("fieldHoldsText(%s) = %v, want %v", path, got, want) + } + } +} + +// buildString runs buildStringField for one string flag set to value. +func buildString(t *testing.T, flag, path, value string) (stringFlagTarget, error) { + t.Helper() + cmd := &cobra.Command{Use: "vf"} + cmd.Flags().String(flag, "", "") + if err := cmd.Flags().Set(flag, value); err != nil { + t.Fatal(err) + } + var target stringFlagTarget + err := buildStringField(cmd, reflect.ValueOf(&target).Elem(), FlagMeta{FlagName: flag, FieldPath: path, Kind: FlagKindString, Required: true}) + return target, err +} + +func TestStringFlagOnAnObjectTakesJSON(t *testing.T) { + got, err := buildString(t, "payload", "Payload", `{"sequential":true}`) + if err != nil || !got.Payload.Sequential { + t.Fatalf("Payload = %+v, err = %v; want sequential true", got.Payload, err) + } +} + +func TestStringFlagOnANullableStringTakesTextOrNull(t *testing.T) { + got, err := buildString(t, "note", "Note", "Checks tone") + if value, ok := got.Note.GetOrZero(); err != nil || !ok || value != "Checks tone" { + t.Fatalf("Note = %v, err = %v; want the text", got.Note, err) + } + + got, err = buildString(t, "note", "Note", "null") + if err != nil || !got.Note.IsNull() { + t.Fatalf("Note = %v, err = %v; want an explicit null", got.Note, err) + } +} + +func TestStringFlagOnTextIsUnchanged(t *testing.T) { + // Text that happens to look like JSON is still stored as written. + got, err := buildString(t, "name", "Name", `{"sequential":true}`) + if err != nil || got.Name != `{"sequential":true}` { + t.Fatalf("Name = %q, err = %v; want the text as written", got.Name, err) + } +} + +func TestStringFlagOnAnObjectRejectsText(t *testing.T) { + _, err := buildString(t, "payload", "Payload", "sequential") + if err == nil || !strings.Contains(err.Error(), "invalid value for --payload") { + t.Fatalf("err = %v, want an invalid value error for --payload", err) + } +} diff --git a/test/union-variant-flags.test.ts b/test/union-variant-flags.test.ts new file mode 100644 index 00000000..002967ef --- /dev/null +++ b/test/union-variant-flags.test.ts @@ -0,0 +1,120 @@ +// Tests for string flags whose field is not text (internal/flagutil/stringflag.go). +// +// The generated metadata declares some expanded union-variant flags as plain +// strings although their fields are objects or nullable strings. Each failed +// with "cannot convert string to ..." whatever value it was given, so the flag +// could not be used at all. They now take JSON, the way a JSON flag does. +// +// Most cases read the request body out of the --dry-run preview on stderr; the +// credentials case sends its request to a local server instead, since the +// preview hides credentials. Nothing reaches the network. The exit code is not +// asserted: until the dry-run fix for operations that succeed with 201 lands, +// those dry runs exit 1 after printing the preview. +// +// Requires: go build -o vf ./cmd/vf + +import { execa } from 'execa'; +import * as fs from 'node:fs'; +import * as http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +const VF = path.resolve(__dirname, '..', 'vf'); + +// Every variable that puts the CLI into agent mode. Mirrors the list in +// internal/output/agentmode.go; a stray one on the host would change the output. +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', +]; + +let home: string; +let server: http.Server; +let serverURL: string; +let received: unknown[] = []; + +beforeAll(async () => { + // vf keeps credentials under HOME; an empty one keeps the developer's out. + home = fs.mkdtempSync(path.join(os.tmpdir(), 'vf-variant-flags-home-')); + + // Records each request body it receives. + server = http.createServer((req, res) => { + let body = ''; + req.on('data', (chunk) => (body += chunk)); + req.on('end', () => { + received.push(body ? JSON.parse(body) : null); + res.writeHead(200, { 'content-type': 'application/json' }); + res.end('{}'); + }); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + serverURL = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + fs.rmSync(home, { recursive: true, force: true }); +}); + +function vf(args: string[]) { + const env: Record = Object.fromEntries(AGENT_ENV_VARS.map((name) => [name, undefined])); + env.HOME = home; + return execa({ reject: false, timeout: 20_000, stdin: 'ignore', env, extendEnv: true })(VF, [...args, '--token', 'vfp_x']); +} + +const dryRun = (args: string[]) => vf([...args, '--dry-run']); + +/** The request body from a --dry-run preview, parsed. */ +function sentBody(stderr: string): Record { + const match = stderr.match(/\[DRY-RUN\] Body:\n([\s\S]*?)\n\[DRY-RUN\] Network call skipped\./); + expect(match, `no request body in:\n${stderr}`).not.toBeNull(); + return JSON.parse(match![1]!); +} + +const TURN = ['test', 'turn', 'create', '--project-id', 'p', '--environment-alias', 'main', '--body-param.agent.test-id', 't']; +const EVALUATION = [ + 'evaluation', 'create', '--project-id', 'p', '--body-param.boolean.enabled', '--body-param.boolean.name', 'n', + '--body-param.boolean.prompt', 'p', '--body-param.boolean.true-prompt', 't', '--body-param.boolean.false-prompt', 'f', +]; + +describe('a string flag whose field is not text', () => { + it('takes a JSON object: test turn create --body-param.agent.payload', async () => { + const result = await dryRun([...TURN, '--body-param.agent.payload', '{"sequential":true}']); + + expect(result.stderr).not.toContain('cannot convert'); + expect(sentBody(result.stderr)).toMatchObject({ type: 'agent', testID: 't', payload: { sequential: true } }); + }); + + // Sent to a local server rather than read from a --dry-run preview: the + // preview hides credentials, so only the request itself shows they arrived. + it('takes a JSON object: integration connect --body-param.twilio.credentials', async () => { + received = []; + const result = await vf([ + 'integration', 'connect', '--project-id', 'p', '--integration', 'twilio', '--server-url', serverURL, + '--body-param.twilio.credentials', '{"apiKeySid":"sid","apiKeySecret":"not-a-secret","accountSid":"acct"}', + ]); + + expect(result.stderr).not.toContain('cannot convert'); + expect(received).toEqual([ + { integration: 'twilio', credentials: { apiKeySid: 'sid', apiKeySecret: 'not-a-secret', accountSid: 'acct' } }, + ]); + }); + + it('takes plain text or null for a nullable string: evaluation create --body-param.boolean.description', async () => { + const text = await dryRun([...EVALUATION, '--body-param.boolean.description', 'Checks tone']); + expect(sentBody(text.stderr)).toMatchObject({ type: 'boolean', description: 'Checks tone' }); + + const cleared = await dryRun([...EVALUATION, '--body-param.boolean.description', 'null']); + expect(sentBody(cleared.stderr)).toMatchObject({ description: null }); + }); + + it('says it wants JSON when given text for an object', async () => { + const result = await dryRun([...TURN, '--body-param.agent.payload', 'sequential']); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain('invalid value for --body-param.agent.payload: expected a JSON value'); + }); +}); From fcafecda029f15979fcf59ac9d22effffd9e8692 Mon Sep 17 00:00:00 2001 From: Braden Ream <51544548+Bradenream@users.noreply.github.com> Date: Thu, 1 Oct 2026 18:49:27 -0400 Subject: [PATCH 2/3] fix: send a nullable string given '' as empty text, not leave it out A string flag whose field cannot hold text is built by buildJSONField, which reads an empty value as not given. So --body-param.boolean.description '' left description out of the request instead of sending "", though a string flag set to '' has always meant empty text. Copilot raised this in review. A nullable string given '' on purpose is now stored as "". A flag that is not given stays out, and an object given '' is still left out, as it would be from a JSON flag. Unit tests cover flags that are optional and flags that are neither optional nor required, each given '' and not given at all. --- internal/flagutil/metadata.go | 2 +- internal/flagutil/stringflag.go | 61 ++++++++++++++++++++++++---- internal/flagutil/stringflag_test.go | 57 +++++++++++++++++++++++--- test/union-variant-flags.test.ts | 6 ++- 4 files changed, 109 insertions(+), 17 deletions(-) diff --git a/internal/flagutil/metadata.go b/internal/flagutil/metadata.go index 5718bb88..4dab2887 100644 --- a/internal/flagutil/metadata.go +++ b/internal/flagutil/metadata.go @@ -542,7 +542,7 @@ func buildStringField(cmd *cobra.Command, v reflect.Value, m FlagMeta) error { } // A string flag whose field cannot hold text takes JSON. See stringflag.go. if !fieldHoldsText(v.Type(), m.FieldPath) { - return buildJSONField(cmd, v, m) + return buildNonTextField(cmd, v, m, val) } return setFieldByPath(v, m.FieldPath, reflect.ValueOf(val)) } diff --git a/internal/flagutil/stringflag.go b/internal/flagutil/stringflag.go index fa1d0e5f..7def68a1 100644 --- a/internal/flagutil/stringflag.go +++ b/internal/flagutil/stringflag.go @@ -14,35 +14,78 @@ // now takes its value the way a JSON flag does, through buildJSONField: JSON // for an object, and plain text or null for a nullable string. A flag whose // field holds text is unchanged. +// +// One difference from a JSON flag is kept: buildJSONField reads an empty value +// as not given, while a string flag set to '' on purpose means empty text. So +// a nullable string given '' is sent as "", not left out. package flagutil import ( + "fmt" "reflect" "strings" + + "github.com/spf13/cobra" + + "github.com/voiceflow/cli/internal/sdk/sdkinternal/utils" ) -// fieldHoldsText reports whether the field at path in t can be set from a -// string flag's text, by the conversion setFieldByPath makes. A path that does -// not resolve reports true, leaving setFieldByPath to describe the problem. -func fieldHoldsText(t reflect.Type, path string) bool { +// fieldTypeAt returns the type of the field at path in t, without allocating +// anything. ok is false when the path does not resolve. +func fieldTypeAt(t reflect.Type, path string) (reflect.Type, bool) { for _, name := range strings.Split(path, ".") { for t.Kind() == reflect.Ptr { t = t.Elem() } if t.Kind() != reflect.Struct { - return true + return nil, false } field, ok := t.FieldByName(name) if !ok { - return true + return nil, false } t = field.Type } + return t, true +} + +// fieldHoldsText reports whether the field at path in t can be set from a +// string flag's text, by the conversion setFieldByPath makes. A path that does +// not resolve reports true, leaving setFieldByPath to describe the problem. +func fieldHoldsText(t reflect.Type, path string) bool { + field, ok := fieldTypeAt(t, path) + if !ok { + return true + } // setFieldByPath allocates the pointer of an optional field and converts // into what it points to. - if t.Kind() == reflect.Ptr { - t = t.Elem() + if field.Kind() == reflect.Ptr { + field = field.Elem() + } + return reflect.TypeOf("").ConvertibleTo(field) +} + +// buildNonTextField sets, from a string flag, a field that cannot hold text. +func buildNonTextField(cmd *cobra.Command, v reflect.Value, m FlagMeta, val string) error { + if val == "" { + if field, ok := fieldTypeAt(v.Type(), m.FieldPath); ok && targetsStringValue(field) { + return setEmptyText(v, m) + } + } + return buildJSONField(cmd, v, m) +} + +// setEmptyText stores an explicit empty string in a nullable string field. +func setEmptyText(v reflect.Value, m FlagMeta) error { + field, err := navigateToField(v, m.FieldPath) + if err != nil { + return fmt.Errorf("failed to navigate to field for --%s: %w", m.FlagName, err) + } + target := reflect.New(field.Type()) + if err := utils.UnmarshalJsonFromString(`""`, target.Interface(), m.Annotations); err != nil { + return fmt.Errorf("invalid value for --%s: %w", m.FlagName, err) } - return reflect.TypeOf("").ConvertibleTo(t) + field.Set(target.Elem()) + return nil } diff --git a/internal/flagutil/stringflag_test.go b/internal/flagutil/stringflag_test.go index f0ddb86a..db7ae731 100644 --- a/internal/flagutil/stringflag_test.go +++ b/internal/flagutil/stringflag_test.go @@ -43,19 +43,64 @@ func TestFieldHoldsText(t *testing.T) { } } -// buildString runs buildStringField for one string flag set to value. -func buildString(t *testing.T, flag, path, value string) (stringFlagTarget, error) { +// buildFlag runs buildStringField for the string flag m, set to *value, or not +// given at all when value is nil. +func buildFlag(t *testing.T, m FlagMeta, value *string) (stringFlagTarget, error) { t.Helper() cmd := &cobra.Command{Use: "vf"} - cmd.Flags().String(flag, "", "") - if err := cmd.Flags().Set(flag, value); err != nil { - t.Fatal(err) + cmd.Flags().String(m.FlagName, "", "") + if value != nil { + if err := cmd.Flags().Set(m.FlagName, *value); err != nil { + t.Fatal(err) + } } var target stringFlagTarget - err := buildStringField(cmd, reflect.ValueOf(&target).Elem(), FlagMeta{FlagName: flag, FieldPath: path, Kind: FlagKindString, Required: true}) + err := buildStringField(cmd, reflect.ValueOf(&target).Elem(), m) return target, err } +// buildString runs buildStringField for a required string flag set to value. +func buildString(t *testing.T, flag, path, value string) (stringFlagTarget, error) { + t.Helper() + return buildFlag(t, FlagMeta{FlagName: flag, FieldPath: path, Kind: FlagKindString, Required: true}, &value) +} + +// A flag that is not required, given ” on purpose or not given at all. Empty +// text for a nullable string is sent as "", as a string flag's ” always was; +// an object has no empty text, so ” leaves it out, as it would a JSON flag. +func TestStringFlagThatIsNotRequiredGivenEmptyOrNothing(t *testing.T) { + empty := "" + for _, kind := range []struct { + name string + optional, required bool + }{ + {"optional", true, false}, + {"neither optional nor required", false, false}, + } { + t.Run(kind.name, func(t *testing.T) { + note := FlagMeta{FlagName: "note", FieldPath: "Note", Kind: FlagKindString, Optional: kind.optional, Required: kind.required} + payload := FlagMeta{FlagName: "payload", FieldPath: "Payload", Kind: FlagKindString, Optional: kind.optional, Required: kind.required} + + got, err := buildFlag(t, note, &empty) + if value, ok := got.Note.GetOrZero(); err != nil || !ok || value != "" { + t.Errorf("note given '': Note = %v, err = %v; want empty text", got.Note, err) + } + + got, err = buildFlag(t, note, nil) + if err != nil || got.Note.IsSet() { + t.Errorf("note not given: Note = %v, err = %v; want it left out", got.Note, err) + } + + for _, value := range []*string{&empty, nil} { + got, err = buildFlag(t, payload, value) + if err != nil || got.Payload.Sequential { + t.Errorf("payload given %v: Payload = %+v, err = %v; want it left out", value, got.Payload, err) + } + } + }) + } +} + func TestStringFlagOnAnObjectTakesJSON(t *testing.T) { got, err := buildString(t, "payload", "Payload", `{"sequential":true}`) if err != nil || !got.Payload.Sequential { diff --git a/test/union-variant-flags.test.ts b/test/union-variant-flags.test.ts index 002967ef..2ff15679 100644 --- a/test/union-variant-flags.test.ts +++ b/test/union-variant-flags.test.ts @@ -103,10 +103,14 @@ describe('a string flag whose field is not text', () => { ]); }); - it('takes plain text or null for a nullable string: evaluation create --body-param.boolean.description', async () => { + it('takes plain text, empty text or null for a nullable string: evaluation create --body-param.boolean.description', async () => { const text = await dryRun([...EVALUATION, '--body-param.boolean.description', 'Checks tone']); expect(sentBody(text.stderr)).toMatchObject({ type: 'boolean', description: 'Checks tone' }); + // Given '' on purpose, the field is sent empty rather than left out. + const empty = await dryRun([...EVALUATION, '--body-param.boolean.description', '']); + expect(sentBody(empty.stderr)).toMatchObject({ description: '' }); + const cleared = await dryRun([...EVALUATION, '--body-param.boolean.description', 'null']); expect(sentBody(cleared.stderr)).toMatchObject({ description: null }); }); From 2f5376636193d6cbe00a034038002b27b756b837 Mon Sep 17 00:00:00 2001 From: Braden Ream <51544548+Bradenream@users.noreply.github.com> Date: Fri, 2 Oct 2026 12:13:40 -0400 Subject: [PATCH 3/3] ci: re-run checks on master now that #37 has fixed its tests