Skip to content

fix(go): resolve custom output formats and add DefineFormats - #5861

Open
apascal07 wants to merge 3 commits into
mainfrom
ap/go-format-registry-key
Open

fix(go): resolve custom output formats and add DefineFormats#5861
apascal07 wants to merge 3 commits into
mainfrom
ap/go-format-registry-key

Conversation

@apascal07

@apascal07 apascal07 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Fixes custom output format registration, which has been broken since the formatter API landed: DefineFormat stored formatters under the raw name while resolveFormat looked them up under the /format/ key, so a format registered as csv was never resolvable and Generate failed with INVALID_ARGUMENT. Only the built-in formats worked, because ConfigureFormats prefixed the key manually.

Ports DefineFormats(Formatter...) from the V2 branch (#5814) as the registration API. Each formatter registers under the name returned by its Name() method, so there is no separate name to get out of sync with the lookup key.

type csvFormatter struct{}
func (f csvFormatter) Name() string { return "csv" }
func (f csvFormatter) Handler(schema map[string]any) (ai.FormatHandler, error) { ... }

genkit.DefineFormats(g, csvFormatter{})

resp, err := genkit.Generate(ctx, g,
    ai.WithPrompt("List 3 countries and their capitals"),
    ai.WithOutputFormat("csv"),
)

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.DefineFormat removed, ai.DefineFormats added. The registry-taking helper never worked for custom formats, so nothing functional can depend on it.

Also consolidates formatter.go and the per-format files (text, json, jsonl, array, enum) into a single format.go, matching the V2 layout. The move is pure relocation: no handler behavior changes with it.

Verification

Go 1.26.3, full go build ./... and go test ./... in go/: 40 packages with tests pass, no failures.

Regression coverage at both layers: ai.TestDefineFormatsCustom resolves a custom format through resolveFormat and then drives the full Generate path with it, and genkit.TestDefineFormats covers the same through the genkit wrapper plus both name forms the deprecated DefineFormat has 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.

…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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread go/ai/format.go
Comment on lines +503 to +505
if result != nil {
results = append(results, result)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
if result != nil {
results = append(results, result)
}
if result != nil {
results = append(results, result)
}
if isLastLine {
currentPos += lineLen
}

Comment thread go/ai/format.go
Comment on lines +689 to +690
re := regexp.MustCompile(`['"]`)
clean := re.ReplaceAllString(accumulatedText.String(), "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
re := regexp.MustCompile(`['"]`)
clean := re.ReplaceAllString(accumulatedText.String(), "")
clean := strings.NewReplacer("'", "", "\"", "").Replace(accumulatedText.String())

Comment thread go/ai/format.go
Comment on lines +758 to +759
re := regexp.MustCompile(`['"]`)
clean := re.ReplaceAllString(text, "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
re := regexp.MustCompile(`['"]`)
clean := re.ReplaceAllString(text, "")
clean := strings.NewReplacer("'", "", "\"", "").Replace(text)

Comment thread go/ai/format.go
Comment on lines +157 to +159
if targetIndex != -1 {
messages[targetIndex].Content = append(messages[targetIndex].Content, part)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
	}

Comment thread go/ai/format.go
Comment on lines +328 to +329
text := base.ExtractJSONFromMarkdown(accumulatedText.String())
if text != "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 != "" {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant