ci: convert schema before marshalling response

[effervescentia]

No description provided.

[linear-code]

COR-13656

[Copilot]

🟡 Changes recommended

The new targetsStringValue helper can misclassify non-OptionalNullable map types (bool-keyed maps without pointer values), which can lead to incorrect auto-quoting behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adjusts JSON-typed flag handling so nullable string destinations (notably OptionalNullable[string]) can accept plain text input by encoding it as a JSON string before unmarshalling, and removes the prior retry-based raw-text fallback helper.

Changes:

• Encode non–JSON-string inputs as JSON strings up front when the destination ultimately holds a string.
• Simplify JSON unmarshal failure handling to use jsonValueError rather than a separate raw-text retry path.
• Remove internal/flagutil/rawtext.go as the behavior is now implemented in buildJSONField.

File summaries

File	Description
internal/flagutil/rawtext.go	Removes the old “retry as JSON-quoted string” recovery helper logic.
internal/flagutil/metadata.go	Adds string-target detection and pre-encodes raw text for nullable/string-like JSON destinations; updates error path to jsonValueError.

Review details

Files not reviewed (1)

• internal/flagutil/metadata.go: Generated file

Suppressed comments (1)

internal/flagutil/metadata.go:866

• targetsStringValue treats any map with a bool key as OptionalNullable, but OptionalNullable[T] is specifically map[bool]*T. Without checking Elem() is a pointer, a type like map[bool]string would be misclassified as "string-valued" and have its input auto-quoted incorrectly. jsonShapeExample already guards this by checking t.Elem().Kind() == reflect.Ptr (internal/flagutil/jsonerror.go:72-74).

• Files reviewed: 1/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Bradenream]

Approving — this is better than what it replaces, and I checked rather than assumed.

Encoding before the unmarshal instead of retrying after it is the right shape. It drops the double attempt, removes any question about the first failed attempt having partially written the target, and structurally eliminates the shadowed-error bug Copilot caught in my version. 42 lines against 128.

What I verified:

• The existing suite passes unchanged against this implementation — including the two assertions the review objection turned on (wire payload identical, Markup left strict).
• Behavioural differential: 0 real differences across 56 invocations over 8 flags (--llm, --playbooks, --knowledge-base-tool, --instructions, --end-tool, --url, --description ×2) and inputs raw text, {"a":1}, [1,2], null, "x", 123, empty. Nine apparent diffs were header map-iteration order, not behaviour.
• targetsStringValue over 13 type shapes — matches the version it replaces on 12, including every Markup form.
• go build, go vet, gofmt clean.

Three notes, none blocking.

1. This moves ~40 lines into generated metadata.go. The version it replaces kept that file at 2 changed lines and put the logic in a non-generated file. Every line in metadata.go rides through each regeneration, so this is the one thing I'd actually suggest changing — and it keeps your design intact:

// in metadata.go, at the same point:
val = encodeRawTextForStringTarget(fieldType, val)

with encodeRawTextForStringTarget and targetsStringValue living in a non-generated file next to jsonerror.go. json.Marshal of a string cannot fail — invalid UTF-8 is replaced with U+FFFD rather than erroring — so the helper needs no error return and the call site is one line.

2. targetsStringValue is looser than the version it replaces. It descends into any bool-keyed map without requiring a pointer element, so map[bool]string reports true. Every map[bool] in the SDK today is map[bool]*T, so this is theoretical — but a line in the comment pinning the intent would stop it drifting.

3. The discriminator has no Go unit test. The Markup guarantee now rests entirely on one TypeScript integration test. This is cheap and pins the review objection permanently:

func TestTargetsStringValue(t *testing.T) {
	cases := []struct {
		name string
		typ  reflect.Type
		want bool
	}{
		{"OptionalNullable[string]", reflect.TypeOf(optionalnullable.OptionalNullable[string]{}), true},
		{"*string", reflect.TypeOf((*string)(nil)), true},
		{"map[string]any", reflect.TypeOf(map[string]any{}), false},
		{"map[string]string", reflect.TypeOf(map[string]string{}), false},
		{"[]string", reflect.TypeOf([]string{}), false},
		{"OptionalNullable[[]string]", reflect.TypeOf(optionalnullable.OptionalNullable[[]string]{}), false},
		{"OptionalNullable[float64]", reflect.TypeOf(optionalnullable.OptionalNullable[float64]{}), false},
		{"components.Markup", reflect.TypeOf(components.Markup{}), false},
		{"[]components.Markup", reflect.TypeOf([]components.Markup{}), false},
		{"OptionalNullable[[]components.Markup]", reflect.TypeOf(optionalnullable.OptionalNullable[[]components.Markup]{}), false},
	}
	for _, c := range cases {
		if got := targetsStringValue(c.typ); got != c.want {
			t.Errorf("%s: got %v want %v", c.name, got, c.want)
		}
	}
}

Two smaller things:

• The title is ci: but nothing here touches CI — refactor: or fix: would read better in the changelog.
• jsonShapeExample's reflect.String case is now unreachable: string targets can no longer reach the error path, so the --instructions '"your text here"' hint never fires. Harmless, but it is an orphan now — worth deleting in the same pass.