From 02ff16c6552aed208ea0a9a72df820cd79fad821 Mon Sep 17 00:00:00 2001 From: Bradenream <51544548+Bradenream@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:15:10 +0000 Subject: [PATCH] feat: accept raw text for JSON flags whose destination is a string (COR-13656) (#24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #23. This is the contested half of #21, split out so it can be judged on its own. ## What it does Fields marked `nullable: true` generate as `FlagKindJSON`, because JSON is the only encoding that expresses a real `null` distinctly from the string `"null"`. That is correct, and it is why `--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: ```bash 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 @effervescentia — you asked for changes on the grounds that this accepts a `string` for `Markup` fields, and so belongs at the API level rather than downstream in the generated CLI. **The principle is right. I could not reproduce the premise**, and rather than argue it I have pinned both halves with tests: **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 already produced, and to master: ``` branch, raw text "instructions": "You are a support agent." branch, JSON-quoted "instructions": "You are a support agent." master, JSON-quoted "instructions": "You are a support agent." ``` 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 — unit-tested against `Markup`, `[]Markup`, `*Markup`, `OptionalNullable[Markup]` and `OptionalNullable[[]Markup]`. `--url` on `mcp-server create` is `[]components.Markup` and still rejects raw text; there is a regression test for that specifically. Separately, no property in `.speakeasy/out.openapi.yaml` references `Markup` at all. **So there is 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 rather than API design. If you would still rather not carry this in the CLI, I am happy to drop it; #23 has three-quarters of the value and no type-shaped surface at all. ## The discriminator `OptionalNullable[T]` is `map[bool]*T`, so its `reflect.Kind` is `Map` — 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 the near-misses `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 — the overwhelmingly common case for these fields — and wrong for someone who meant JSON and typo'd. Structs, maps and slices are excluded, so a mistyped object still fails loudly everywhere it would be meaningless as text. ## Verification - 7 new tests, including the two that this change lives or dies on: wire payload unchanged, and Markup left strict. - One test from #23 is **updated rather than kept** — it asserted that `--instructions` rejects prose and explains how to quote it. That rejection is what this removes, so the test now pins the behaviour that replaced it. - Full suite shows the same pre-existing failures as master. --- internal/flagutil/metadata.go | 40 +++++++++++++++ test/flag-errors.test.ts | 23 ++++----- test/flag-raw-text.test.ts | 91 +++++++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 14 deletions(-) create mode 100644 test/flag-raw-text.test.ts diff --git a/internal/flagutil/metadata.go b/internal/flagutil/metadata.go index a216714..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. @@ -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/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'); + }); +});