fix: let union-variant flags whose field is an object take JSON - #40
Closed
Bradenream wants to merge 3 commits into
Closed
Bradenream wants to merge 3 commits into
Bradenream wants to merge 3 commits into
Conversation
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.<provider>.credentials, for twilio, ujet, genesys, kustomer, dixa, sunshine and custom-handoff (objects) - evaluation create --body-param.<type>.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.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Explicitly supplied empty nullable strings are omitted instead of being sent as empty values.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes expanded union-variant flags so object and nullable-string fields use the CLI’s existing JSON handling.
Changes:
- Routes non-text targets through the JSON builder, preserving ordinary string behavior.
- Adds unit and CLI regression tests for objects, nullable strings, and invalid input.
| File | Description |
|---|---|
test/union-variant-flags.test.ts |
Tests request bodies and invalid-value errors. |
internal/flagutil/stringflag.go |
Detects fields that can hold plain text. |
internal/flagutil/stringflag_test.go |
Tests detection and string-builder behavior. |
internal/flagutil/metadata.go |
Delegates non-text string flags to JSON handling. |
Files not reviewed (1)
- internal/flagutil/metadata.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
effervescentia
approved these changes
Oct 2, 2026
Contributor
Merge activity
|
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.

Summary
Some commands expand a union variant into one flag per field (
--body-param.<variant>.<field>). The generated metadata declares some of those flags as plain strings although their fields are objects or nullable strings.setFieldByPathcannot store text in those fields, so each failed withcannot convert string to …whatever value it was given; only--body, or the variant's whole-JSON flag, could set them. A survey of all 38 expanded variant fields found 11:test turn create --body-param.agent.payload(an object)integration connect --body-param.<provider>.credentials, for twilio, ujet, genesys, kustomer, dixa, sunshine and custom-handoff (objects)evaluation create --body-param.<type>.description, for boolean, number and string (nullable strings, so plain text failed too)buildStringFieldnow hands such a flag tobuildJSONField, the builder that JSON flags use (internal/flagutil/stringflag.go). An object takes JSON, and a nullable string takes plain text ornull. Flags whose field holds text are unchanged.Before and after
cannot convert string to …--body-param.agent.payload sequential(text for an object)cannot convert …invalid value … expected a JSON value, with an exampleTest plan
gofmt,go vet ./...andgo test ./...pass;go.modis unchanged.internal/flagutil/stringflag_test.gocovers the type rule and the builder:Without the change, 3 of them fail.
test/union-variant-flags.test.tscovers the turn payload, Twilio credentials, an evaluation description (text and null) and a bad value. All 4 fail on master.--dry-runbody. The credentials case sends its request to a local server instead, because fix: hide camelCase secrets in --dry-run and --debug output #42 hides credentials in the preview.Full suite with fix: restore vf docs search, and make the test suite hermetic and green #37 applied: 82/82. With fix: restore vf docs search, and make the test suite hermetic and green #37, fix: stop waiting on the silent stdin an agent's shell hands vf #38 and the other three fixes from this batch: 101/101.
CI:
CLI behaviourshows master's 4 existing failures until fix: restore vf docs search, and make the test suite hermetic and green #37 merges.