feat: accept raw text for JSON flags whose destination is a string (COR-13656) - #24
Closed
Bradenream wants to merge 2 commits into
Closed
feat: accept raw text for JSON flags whose destination is a string (COR-13656)#24Bradenream wants to merge 2 commits into
Bradenream wants to merge 2 commits into
Conversation
This was referenced Sep 1, 2026
There was a problem hiding this comment.
🟡 Changes recommended
There is a concrete error-wrapping bug in the new fallback path where a marshal failure reports the wrong underlying cause.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a “raw text fallback” for JSON-typed flags that ultimately populate Go string destinations, so users can pass prose without JSON-quoting while preserving null semantics for nullable fields.
Changes:
- Introduces
setJSONFieldAsRawTextandstringLikeJSONTargetto retry invalid JSON inputs as JSON strings when the destination unwraps tostring. - Updates JSON-flag parsing (
buildJSONField) to route JSON unmarshal failures through the raw-text fallback. - Adds/updates Vitest coverage to pin wire-encoding invariants and ensure Markup-typed flags remain strict.
File summaries
| File | Description |
|---|---|
| test/flag-raw-text.test.ts | New end-to-end tests for raw-text fallback behavior and invariants (wire equality, Markup stays strict). |
| test/flag-errors.test.ts | Updates expectations to reflect that prose is now accepted for --instructions rather than rejected with a quoting hint. |
| internal/flagutil/rawtext.go | Implements raw-text fallback logic for string-like JSON flag destinations and a dedicated retry-failure error path. |
| internal/flagutil/metadata.go | Switches JSON unmarshal failure handling to use the new raw-text fallback. |
Review details
Files not reviewed (1)
- internal/flagutil/metadata.go: Generated file
- Files reviewed: 3/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+80
to
+89
| // 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, | ||
| } | ||
| } |
effervescentia
changed the base branch from
braden/flag-errors/COR-13656
to
master
September 4, 2026 15:50
Contributor
Author
Merge activity
|
…OR-13656) 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.
Bradenream
force-pushed
the
braden/flag-raw-text/COR-13656
branch
from
September 4, 2026 15:51
554bb9b to
9537cb8
Compare
effervescentia
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: truegenerate asFlagKindJSON, because JSON is the only encoding that expresses a realnulldistinctly from the string"null". That is correct, and it is why--instructions nullclears 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
nullbranch runs earlier.On the review objection
@effervescentia — you asked for changes on the grounds that this accepts a
stringforMarkupfields, 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: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.
Markupis a union struct, and every field holding one holds it as a struct or a slice of them.stringLikeJSONTargetadmits neither — unit-tested againstMarkup,[]Markup,*Markup,OptionalNullable[Markup]andOptionalNullable[[]Markup].--urlonmcp-server createis[]components.Markupand still rejects raw text; there is a regression test for that specifically. Separately, no property in.speakeasy/out.openapi.yamlreferencesMarkupat 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]ismap[bool]*T, so itsreflect.KindisMap— indistinguishable frommap[string]anyby kind alone. Requiring a bool key admitsOptionalNullable[string]and rejects every ordinary map. Unit-tested against all 13 shapes that occur, including the near-missesmap[string]stringandOptionalNullable[[]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
--instructionsrejects prose and explains how to quote it. That rejection is what this removes, so the test now pins the behaviour that replaced it.