refactor(go/plugins/anthropic): migrate to the typed-config constructors - #5874
Open
apascal07 wants to merge 4 commits into
Open
refactor(go/plugins/anthropic): migrate to the typed-config constructors#5874apascal07 wants to merge 4 commits into
apascal07 wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the Anthropic plugin to leverage Genkit's typed model support, delegating configuration validation and deserialization to the framework and simplifying the codebase. Feedback highlights a potential nil pointer dereference in DefineModel when dereferencing the opts parameter without a prior nil check.
…odel Migrates the Anthropic plugin and the Vertex AI Model Garden Claude models to the typed-config constructors: the model function receives an anthropic.MessageNewParams the framework has already validated and deserialized, so the hand-rolled configFromRequest type switch is gone. Both plugins built their own model action with the same options and the same generate closure, so they now share one constructor. It replaces ant.DefineModel, which registered nothing despite its name, returns the concrete *ai.ModelAction so the callers stop asserting their way back to api.Action, and honors the caller's label instead of overwriting it with a provider-prefixed model ID. The config schema is reflected once at package level rather than per model, which ListActions builds once per discovered model.
The config schema a model advertises is now enforced on every request, so what it accepts has to match what anthropic.MessageNewParams can hold: every config form the action boundary accepts is checked to deserialize into the SDK type, including the wrapper types the SDK uses for optional primitives and the thinking union. The forms it rejects (unknown fields, camelCase spellings of snake_case wire names, mistyped values) are pinned too, since the model function never sees them. Labels move with the constructor: curated ones are honored as given and the rest are derived from the provider and the model name.
… opts DefineModel dereferenced its ModelOptions pointer unguarded, so a nil panicked. A nil now takes the capabilities the plugin already resolves by name, curated for a known model and the Claude defaults for the rest, rather than an empty ModelOptions that would advertise a model supporting neither tools nor multiturn nor a system role.
…er's array The request's config is a shallow copy, so its Tools slice header still points at the caller's backing array. Appending the request's tools in place writes into that array's spare capacity, so a config hoisted into a package-level var or a ModelRef, which every request made with it shares, has concurrent requests writing the same slot and reading each other's tools. Clipping before the append forces an allocation. The test that claimed to pin this only compared lengths, which an in-place append leaves untouched while overwriting the slots past them. It now checks that the result does not share the caller's array and that the caller's spare capacity is still empty; it fails on the old append.
apascal07
force-pushed
the
ap/go-anthropic-typed-config
branch
from
July 31, 2026 22:28
267605e to
0b313a6
Compare
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.
Migrates the Anthropic plugin and the Vertex AI Model Garden Claude models to the typed-config constructors from #5849, the fourth PR of the train. Both surfaces are backed by the same
plugins/internal/anthropicpackage, so they move together and the hand-rolledconfigFromRequesttype switch is gone.Before: the plugin re-implements what the framework now does.
After: the type parameter is inferred from the function and the framework fills it.
toAnthropicRequestamends the request's own copy ofMessageNewParamsrather than the caller's struct.newModelwas reflecting the whole SDK params struct on every call, whichListActionsmakes once per discovered model.unexpected config type: %T.One constructor for both plugins
ant.DefineModeland the Anthropic plugin's ownnewModelbuilt the same action from the same options with the same generate closure, so the two collapse intoant.NewModel. The old name registered nothing despite theDefineprefix, which is what #5849 removed elsewhere in the framework.It returns the concrete
*ai.ModelAction, soListActions,ResolveAction, and the Model GardenInitno longer assert their way back toapi.Action. Two options fields that the old shared constructor dropped on the floor,StageandMetadata, now reach the descriptor because the options struct is passed through whole rather than copied field by field.Labels are the caller's to set:
ant.NewModelfills one in only when the caller leaves it empty, deriving"Anthropic - <name>"or"Vertex AI - <name>"from the provider. The old constructor overwrote whatever it was given with"<Provider>-<model id>", so the curated labels inAnthropicModelsnever reached the dev UI. Model Garden Claude models now showClaude Opus 4.7rather thanVertex AI-claude-opus-4-7, matching how the Llama models in the same plugin present their labels.Two bugs found while moving the config by value
A shared config had its tool list scribbled over by concurrent requests.
toAnthropicRequestappends the Genkit tools onto the config'sToolsrather than assigning, so that server-side tools (web search, code execution) set through the config survive. The config it now receives is a shallow copy, so that slice header still points at the caller's backing array, and a config hoisted into a package-level var or aModelRefis shared by every request made with it. The append wrote into that array's spare capacity, so two concurrent requests raced over the same slots and one request's tools surfaced in another's. Clipping before the append forces an allocation. The test that claimed to pin this only compared lengths, which an in-place append leaves untouched while overwriting the slots past them; it now asserts that the result does not share the caller's array and that the caller's spare capacity is still empty, and it fails on the old append.A nil
ModelOptionspanickedDefineModel. The pointer was dereferenced unguarded. A nil now takes the capabilities the plugin already resolves by name, curated for a known model and the Claude defaults for the rest, rather than an emptyModelOptionsthat would advertise a model supporting neither tools nor multiturn nor a system role.Behavior notes
The config schema the plugin supplies is unchanged, so what requests are accepted is unchanged with it. The schema is explicit rather than inferred because the SDK wraps optional primitives in
param.Opt[T], which reflects as an object but marshals as a bare value; the custom reflector maps those back to their underlying types, and the tests pin that everything the schema accepts deserializes intoMessageNewParams.Verification
Go 1.26.3,
go test ./plugins/anthropic/... ./plugins/internal/anthropic/... ./plugins/vertexai/modelgarden/...: 3 packages pass, 28 top-level tests and 88 including subtests, no failures.New coverage: the config schema reaches the request input schema so the framework rejects a config the SDK type cannot hold, everything the schema advertises deserializes into
MessageNewParams, the curated labels survive to the descriptor for both providers, the tool append leaves the caller's array untouched, andDefineModelwith a nil opts resolves the curated capabilities instead of panicking.