Add Claude support via Google Vertex AI#153
Conversation
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| AuthHeader string `json:"auth_header,omitempty"` | ||
| Model string `json:"model,omitempty"` | ||
| UseAnthropic *bool `json:"use_anthropic,omitempty"` // nil = default true; false = OpenAI protocol | ||
| UseVertex bool `json:"use_anthropic_vertex,omitempty"` |
There was a problem hiding this comment.
Minor naming inconsistency: The Go field name UseVertex doesn't align with the JSON tag use_anthropic_vertex. All other fields follow a direct mapping between Go name and JSON tag (e.g., UseAnthropic ↔ use_anthropic, VertexProjectID ↔ vertex_project_id). Consider renaming the field to UseAnthropicVertex to match the JSON tag, or changing the JSON tag to use_vertex to match the field name. This would improve maintainability and reduce confusion.
| if model == "" { | ||
| return ResolvedEndpoint{}, false, fmt.Errorf("%s is required when %s is enabled", envOCRLLMModel, envOCRUseVertex) | ||
| } | ||
| if projectID == "" { | ||
| return ResolvedEndpoint{}, false, fmt.Errorf("%s or %s is required when %s is enabled", envOCRVertexProject, envCCVertexProject, envOCRUseVertex) | ||
| } | ||
| if region == "" { | ||
| return ResolvedEndpoint{}, false, fmt.Errorf("%s or %s is required when %s is enabled", envOCRVertexRegion, envCCVertexRegion, envOCRUseVertex) | ||
| } |
There was a problem hiding this comment.
Bug: Hard error breaks strategy fallback chain. When OCR_USE_ANTHROPIC_VERTEX is set to true but a required field (model, project ID, or region) is missing, this function returns an error. In ResolveEndpoint, any non-nil error from a strategy immediately terminates resolution (line 69-70), preventing fallback to subsequent strategies like OCR config file, OCR env, or Claude Code env.
This is inconsistent with how other strategies handle missing fields — e.g., tryOCREnv (line 122-123) and tryCCEnv (line 362-363) return (ResolvedEndpoint{}, false, nil) when required fields are absent, allowing the next strategy to be tried.
Consider whether this fail-fast behavior is intentional. If a user sets OCR_USE_ANTHROPIC_VERTEX=true but has a valid non-Vertex config in their config file, they may be surprised that resolution fails entirely rather than falling back. If fail-fast is desired for explicit opt-in, document this behavior. Otherwise, return false, nil to allow fallback.
| func firstNonEmpty(values ...string) string { | ||
| for _, value := range values { | ||
| if strings.TrimSpace(value) != "" { | ||
| return value | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Bug: Returns untrimmed value despite trimming for the emptiness check. firstNonEmpty uses strings.TrimSpace(value) to test if a value is non-empty, but returns the original value with potential leading/trailing whitespace. If a user accidentally sets OCR_VERTEX_PROJECT_ID=' my-project ', the returned value will include spaces, which could cause subtle issues in URL construction or API calls.
Return the trimmed value instead for consistency.
Suggestion:
| func firstNonEmpty(values ...string) string { | |
| for _, value := range values { | |
| if strings.TrimSpace(value) != "" { | |
| return value | |
| } | |
| } | |
| return "" | |
| } | |
| func firstNonEmpty(values ...string) string { | |
| for _, value := range values { | |
| trimmed := strings.TrimSpace(value) | |
| if trimmed != "" { | |
| return trimmed | |
| } | |
| } | |
| return "" | |
| } |
lizhengfeng101
left a comment
lizhengfeng101
left a comment
There was a problem hiding this comment.
Code Review
Overall well-structured PR with clean design and good test coverage. A few items to consider:
High Priority
1. Strategy priority & error behavior in resolver.go
tryOCRVertexEnv is placed first in the strategy list, ahead of the config file:
{"OCR Vertex environment", tryOCRVertexEnv},
{"OCR config file", ...},
{"OCR environment", tryOCREnv},
...The original priority order is config file > env. Now Vertex env jumps to the top. More critically, when OCR_USE_ANTHROPIC_VERTEX=true but model/project/region are missing, tryOCRVertexEnv returns an error instead of falling back, which blocks all subsequent strategies. A user who sets OCR_USE_ANTHROPIC_VERTEX=true but forgets OCR_LLM_MODEL will get an error even if they have a fully valid config file.
Suggestion: either move Vertex env after the config file (consistent with other env strategies), or return (empty, false, nil) instead of an error when required fields are missing, so resolution can fall through to the next strategy.
Medium Priority
2. Missing locale README updates
README.md and README.zh-CN.md are updated, but README.ru-RU.md, README.ja-JP.md, and README.ko-KR.md are not. These should be synced for consistency.
3. defer/recover for panic handling in NewAnthropicVertexClient
defer func() {
if recovered := recover(); recovered != nil {
client = nil
err = fmt.Errorf("initialize Google Vertex AI authentication: %v", recovered)
}
}()Using recover for regular error paths is unusual in Go. If the Anthropic SDK's WithGoogleAuth can panic in specific scenarios (e.g., missing ADC), it would be helpful to add a comment explaining when/why this happens. Otherwise, if it doesn't actually panic, this can be removed.
Low Priority
4. No integration with the modern provider system
Vertex AI is only supported via the legacy llm config block and env vars, not through the modern providers system (e.g., ocr config set provider vertex-ai). Fine for now, but worth noting as a follow-up.
5. UseVertex type inconsistency
UseAnthropic is *bool (to distinguish unset from false), while UseVertex is bool. The type choice is reasonable since the default behavior differs, but the JSON tag naming is inconsistent: use_anthropic vs use_anthropic_vertex. Consider use_vertex for brevity.
6. Transitive dependency weight
The PR pulls in cloud.google.com/go/auth, google.golang.org/api, go.opencensus.io (deprecated, Google is migrating to OpenTelemetry), etc. This is expected for Google Cloud integration but worth mentioning the binary size trade-off.
Positives
- Testability: The
var vertexGoogleAuthOptionpackage-level variable for mock injection is a clean testing pattern. - Error messages: Clear and actionable when configuration is incomplete.
vertexBaseURLfunction: Correctly handlesglobal,us,euspecial regions.- Documentation: English and Chinese READMEs are well-written with complete examples.
- Test coverage: Both resolver and client paths are covered, including the panic recovery edge case.
| envOCRLLMModel = "OCR_LLM_MODEL" | ||
| envOCRLLMAuthHeader = "OCR_LLM_AUTH_HEADER" | ||
| envOCRUseAnthropic = "OCR_USE_ANTHROPIC" | ||
| envOCRUseVertex = "OCR_USE_ANTHROPIC_VERTEX" |
There was a problem hiding this comment.
Maybe we should just use some name like OCR_USE_VERTEX, not just focus on ANTHROPIC models on it.
4 participants
What
This PR adds support for using Anthropic Claude through Google Vertex AI.
Users can enable it with Application Default Credentials (ADC), without configuring an Anthropic API key or custom LLM endpoint.
Changes
OCR_USE_ANTHROPIC_VERTEX,OCR_VERTEX_PROJECT_ID, andOCR_VERTEX_REGIONANTHROPIC_VERTEX_PROJECT_IDCLOUD_ML_REGIONGOOGLE_CLOUD_PROJECTas a project ID fallbackllmconfig fields for Vertex AITesting
go test ./...I also tested against a real GCP project. The request reached Vertex AI with
claude-sonnet-4-6, but the project currently has zero quota forglobal_online_prediction_requests_per_base_model, so the live request was blocked by429 RESOURCE_EXHAUSTED.This confirms the local ADC and Vertex routing path, while a full live completion still requires Anthropic model quota on the GCP project.
Fixes #56