fix: let union-variant flags whose field is an object take JSON

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 buildNonTextField(cmd, v, m, val)
+	}
 	return setFieldByPath(v, m.FieldPath, reflect.ValueOf(val))
 }
 

internal/flagutil/stringflag.go

@@ -0,0 +1,91 @@
+// 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.<provider>.credentials      objects, 7 flags
+//	vf evaluation create        --body-param.<type>.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.
+//
+// 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"
+)
+
+// 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 nil, false
+		}
+		field, ok := t.FieldByName(name)
+		if !ok {
+			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 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)
+	}
+	field.Set(target.Elem())
+	return nil
+}

internal/flagutil/stringflag_test.go

@@ -0,0 +1,136 @@
+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)
+		}
+	}
+}
+
+// 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(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(), 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 {
+		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)
+	}
+}

test/union-variant-flags.test.ts

@@ -0,0 +1,124 @@
+// 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<void>((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<void>((resolve) => server.close(() => resolve()));
+  fs.rmSync(home, { recursive: true, force: true });
+});
+
+function vf(args: string[]) {
+  const env: Record<string, string | undefined> = 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<string, unknown> {
+  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, 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 });
+  });
+
+  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');
+  });
+});