fix(go): resolve custom output formats and add DefineFormats - #5861
fix(go): resolve custom output formats and add DefineFormats#5861apascal07 wants to merge 3 commits into
Conversation
…solve DefineFormat stored formatters under the raw name while resolveFormat looked them up under /format/<name>, so custom formats registered via genkit.DefineFormat or ai.DefineFormat were never resolvable. Derive the key inside DefineFormat, tolerating already-prefixed names for callers that worked around the mismatch, and deprecate the registry-taking ai.DefineFormat in favor of genkit.DefineFormat.
Port DefineFormats(Formatter...) from the V2 branch: formats register under the name returned by Name(), removing the name/key mismatch entirely. genkit.DefineFormat stays as a deprecated shim that honors its explicit name (tolerating already-prefixed names); the registry-taking ai.DefineFormat is removed in favor of ai.DefineFormats.
Merge formatter.go and the per-format files (text, json, jsonl, array, enum) into a single format.go, and rename formatter_test.go to format_test.go, matching the V2 branch layout. No behavior change.
There was a problem hiding this comment.
Code Review
This pull request consolidates several formatter implementations (array, enum, JSON, JSONL, and text) into a single go/ai/format.go file and introduces a new DefineFormats API to register multiple formatters at once, deprecating the single DefineFormat method. Feedback on these changes highlights a streaming bug in parseJSONL where a failure to advance the cursor on the last line causes duplicate items, performance inefficiencies from compiling regular expressions inside ParseMessage and parseEnum on every call, a potential side-effect from mutating the caller's message slice in place, and a case where ParseMessage can silently succeed with empty content if JSON extraction fails.
| if result != nil { | ||
| results = append(results, result) | ||
| } |
There was a problem hiding this comment.
In parseJSONL, when allowPartial is true during streaming, if the last line is successfully parsed as a complete JSON object, the cursor (currentPos) is not advanced past it because isLastLine is true. This causes the same line to be re-parsed and returned again in subsequent chunks, leading to duplicate items in the stream. Advancing currentPos by lineLen when the last line is successfully parsed resolves this issue.
| if result != nil { | |
| results = append(results, result) | |
| } | |
| if result != nil { | |
| results = append(results, result) | |
| } | |
| if isLastLine { | |
| currentPos += lineLen | |
| } |
| re := regexp.MustCompile(`['"]`) | ||
| clean := re.ReplaceAllString(accumulatedText.String(), "") |
There was a problem hiding this comment.
Compiling a regular expression with regexp.MustCompile inside ParseMessage on every call is inefficient and can impact performance. Since you are only removing single and double quotes, using strings.NewReplacer is significantly faster and avoids regex compilation overhead.
| re := regexp.MustCompile(`['"]`) | |
| clean := re.ReplaceAllString(accumulatedText.String(), "") | |
| clean := strings.NewReplacer("'", "", "\"", "").Replace(accumulatedText.String()) |
| re := regexp.MustCompile(`['"]`) | ||
| clean := re.ReplaceAllString(text, "") |
There was a problem hiding this comment.
Compiling a regular expression with regexp.MustCompile inside parseEnum on every streaming chunk is inefficient. Replacing this with strings.NewReplacer avoids regex compilation overhead and improves streaming performance.
| re := regexp.MustCompile(`['"]`) | |
| clean := re.ReplaceAllString(text, "") | |
| clean := strings.NewReplacer("'", "", "\"", "").Replace(text) |
| if targetIndex != -1 { | ||
| messages[targetIndex].Content = append(messages[targetIndex].Content, part) | ||
| } |
There was a problem hiding this comment.
Appending to messages[targetIndex].Content in place mutates the caller's original message slice and message struct, which can lead to unexpected side effects if the caller reuses or logs the original request. It is safer to copy the message and the slice before modifying them.
if targetIndex != -1 {
msgCopy := *messages[targetIndex]
msgCopy.Content = append(slices.Clone(messages[targetIndex].Content), part)
messagesCopy := slices.Clone(messages)
messagesCopy[targetIndex] = &msgCopy
return messagesCopy
}| text := base.ExtractJSONFromMarkdown(accumulatedText.String()) | ||
| if text != "" { |
There was a problem hiding this comment.
If base.ExtractJSONFromMarkdown fails to extract any JSON (e.g., if the model returned conversational text instead of JSON), text will be empty. If there are also no nonTextParts, ParseMessage will silently succeed and return a message with empty content. We should explicitly return an error in this case to fail fast.
text := base.ExtractJSONFromMarkdown(accumulatedText.String())
if text == "" && len(nonTextParts) == 0 {
return nil, errors.New("message does not contain valid JSON")
}
if text != "" {
Fixes custom output format registration, which has been broken since the formatter API landed:
DefineFormatstored formatters under the raw name whileresolveFormatlooked them up under the/format/key, so a format registered ascsvwas never resolvable andGeneratefailed withINVALID_ARGUMENT. Only the built-in formats worked, becauseConfigureFormatsprefixed the key manually.Ports
DefineFormats(Formatter...)from the V2 branch (#5814) as the registration API. Each formatter registers under the name returned by itsName()method, so there is no separate name to get out of sync with the lookup key.API changes:
genkit.DefineFormats(g, formatters...)added as the registration touchpoint. It is variadic because apps typically register their formats once at startup.genkit.DefineFormat(g, name, formatter)deprecated. It now registers under the corrected key, honoring its explicit name and tolerating names that already carry the/format/prefix, which was the only way to make it resolve before this fix.ai.DefineFormatremoved,ai.DefineFormatsadded. The registry-taking helper never worked for custom formats, so nothing functional can depend on it.Also consolidates
formatter.goand the per-format files (text, json, jsonl, array, enum) into a singleformat.go, matching the V2 layout. The move is pure relocation: no handler behavior changes with it.Verification
Go 1.26.3, full
go build ./...andgo test ./...ingo/: 40 packages with tests pass, no failures.Regression coverage at both layers:
ai.TestDefineFormatsCustomresolves a custom format throughresolveFormatand then drives the fullGeneratepath with it, andgenkit.TestDefineFormatscovers the same through the genkit wrapper plus both name forms the deprecatedDefineFormathas to accept (bare and already/format/-prefixed). Both exercise the resolution path that had no custom-format coverage at all, which is why the mismatched key survived this long.