Well-scoped addition of Anthropic Messages API support with clean provider abstraction and comprehensive tests. Defaults remain backwards-compatible and the action/CLI wiring is consistent.
Recommendation
APPROVE — The implementation cleanly introduces a provider switch with minimal disruption, adds appropriate headers and request/response handling for Anthropic, and covers the new paths with tests. The shared request helper improves code reuse and error reporting. CI has passed. Consider a future minor refinement to align the comment in completeAnthropic with its behavior (it currently uses the last system message encountered), but this is non-blocking. Proceed to merge.
## Summary
Well-scoped addition of Anthropic Messages API support with clean provider abstraction and comprehensive tests. Defaults remain backwards-compatible and the action/CLI wiring is consistent.
## Recommendation
**APPROVE** — The implementation cleanly introduces a provider switch with minimal disruption, adds appropriate headers and request/response handling for Anthropic, and covers the new paths with tests. The shared request helper improves code reuse and error reporting. CI has passed. Consider a future minor refinement to align the comment in completeAnthropic with its behavior (it currently uses the last system message encountered), but this is non-blocking. Proceed to merge.
gpt-review-bot
approved these changes 2026-05-02 01:51:25 +00:00
The PR cleanly adds Anthropic Messages API support with a provider abstraction, shared HTTP handling, and tests. Changes are well-structured, tests cover new paths, and CI passed.
Findings
#
Severity
File
Line
Finding
1
[MINOR]
llm/client.go
156
completeAnthropic hardcodes MaxTokens = 8192 in the request body. Consider making this configurable or document why this fixed limit is used (and whether it should be smaller depending on model).
2
[MINOR]
llm/client.go
14
anthropic message mapping uses the same Message.Role/Message.Content fields as OpenAI. Verify this matches the exact Anthropic Messages API payload expectations (author vs role, content block shape). If the upstream API differs, the mapping may need adjustment.
3
[MINOR]
cmd/review-bot/main.go
34
The new --llm-provider flag is validated and applied, but the flag parsing and usage order is fine. Consider documenting provider behavior in CLI help or README so users know provider-specific headers/requirements (e.g., anthropic requires anthropic-version and x-api-key).
4
[NIT]
.gitea/actions/review/action.yml
34
The added default value line for llm-provider contains a trailing space after 'openai'. It's harmless but could be cleaned up for consistency.
5
[MINOR]
llm/client_test.go
220
Some tests assert exact float equality when checking the presence/value of the temperature field decoded from JSON (e.g. comparing temp != 0.7). While likely fine, exact float comparisons can be brittle; consider checking presence and approximate equality or type before comparing.
6
[NIT]
llm/client.go
220
doRequest now accepts a parse function and handles status checks after reading the body — this is good. Consider including the response status and a short snippet of the body in error messages only when safe (avoid leaking secrets) or add structured errors to aid higher-level handling.
Recommendation
APPROVE — Approve: the implementation is sound and the new provider abstraction is well integrated. Address the minor items to improve configurability and robustness: make Anthropic max_tokens configurable (or document the rationale), confirm the message payload shape exactly matches Anthropic's spec, optionally tighten tests that compare floats by value, and remove the trailing space in the action.yml default. Otherwise this is ready to merge.
## Summary
The PR cleanly adds Anthropic Messages API support with a provider abstraction, shared HTTP handling, and tests. Changes are well-structured, tests cover new paths, and CI passed.
## Findings
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [MINOR] | `llm/client.go` | 156 | completeAnthropic hardcodes MaxTokens = 8192 in the request body. Consider making this configurable or document why this fixed limit is used (and whether it should be smaller depending on model). |
| 2 | [MINOR] | `llm/client.go` | 14 | anthropic message mapping uses the same Message.Role/Message.Content fields as OpenAI. Verify this matches the exact Anthropic Messages API payload expectations (author vs role, content block shape). If the upstream API differs, the mapping may need adjustment. |
| 3 | [MINOR] | `cmd/review-bot/main.go` | 34 | The new --llm-provider flag is validated and applied, but the flag parsing and usage order is fine. Consider documenting provider behavior in CLI help or README so users know provider-specific headers/requirements (e.g., anthropic requires anthropic-version and x-api-key). |
| 4 | [NIT] | `.gitea/actions/review/action.yml` | 34 | The added default value line for llm-provider contains a trailing space after 'openai'. It's harmless but could be cleaned up for consistency. |
| 5 | [MINOR] | `llm/client_test.go` | 220 | Some tests assert exact float equality when checking the presence/value of the temperature field decoded from JSON (e.g. comparing temp != 0.7). While likely fine, exact float comparisons can be brittle; consider checking presence and approximate equality or type before comparing. |
| 6 | [NIT] | `llm/client.go` | 220 | doRequest now accepts a parse function and handles status checks after reading the body — this is good. Consider including the response status and a short snippet of the body in error messages only when safe (avoid leaking secrets) or add structured errors to aid higher-level handling. |
## Recommendation
**APPROVE** — Approve: the implementation is sound and the new provider abstraction is well integrated. Address the minor items to improve configurability and robustness: make Anthropic max_tokens configurable (or document the rationale), confirm the message payload shape exactly matches Anthropic's spec, optionally tighten tests that compare floats by value, and remove the trailing space in the action.yml default. Otherwise this is ready to merge.
rodin
merged commit 2adb23b3d9 into main2026-05-02 01:57:49 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
sonnet-review-bot
gpt-review-bot
Adds
--llm-providerflag (openai|anthropic) to switch between API formats.Anthropic Implementation
POST /messagesendpointx-api-key+anthropic-versionheadersmax_tokens: 8192for response generation[{type: "text", text: "..."}]Changes
llm/client.go:Providertype,completeAnthropic(),doRequest()shared helpercmd/review-bot/main.go:--llm-provider/LLM_PROVIDERflag.gitea/actions/review/action.yml:llm-providerinput + envllm/client_test.go: 4 new tests for Anthropic pathBackwards compatible — default provider is still
openai.Closes #18
Adds --llm-provider flag (openai|anthropic) to switch between API formats. Anthropic implementation: - POST /messages endpoint - x-api-key + anthropic-version headers - System prompt as top-level field (not a message) - max_tokens: 8192 for response generation - Parses content blocks [{type: "text", text: "..."}] Changes: - llm/client.go: Provider type, completeAnthropic(), doRequest() shared helper - cmd/review-bot/main.go: --llm-provider / LLM_PROVIDER flag - .gitea/actions/review/action.yml: llm-provider input + env - llm/client_test.go: 4 new tests for Anthropic path Backwards compatible — default provider is still openai. Closes #18Summary
Well-scoped addition of Anthropic Messages API support with clean provider abstraction and comprehensive tests. Defaults remain backwards-compatible and the action/CLI wiring is consistent.
Recommendation
APPROVE — The implementation cleanly introduces a provider switch with minimal disruption, adds appropriate headers and request/response handling for Anthropic, and covers the new paths with tests. The shared request helper improves code reuse and error reporting. CI has passed. Consider a future minor refinement to align the comment in completeAnthropic with its behavior (it currently uses the last system message encountered), but this is non-blocking. Proceed to merge.
Summary
The PR cleanly adds Anthropic Messages API support with a provider abstraction, shared HTTP handling, and tests. Changes are well-structured, tests cover new paths, and CI passed.
Findings
llm/client.gollm/client.gocmd/review-bot/main.go.gitea/actions/review/action.ymlllm/client_test.gollm/client.goRecommendation
APPROVE — Approve: the implementation is sound and the new provider abstraction is well integrated. Address the minor items to improve configurability and robustness: make Anthropic max_tokens configurable (or document the rationale), confirm the message payload shape exactly matches Anthropic's spec, optionally tighten tests that compare floats by value, and remove the trailing space in the action.yml default. Otherwise this is ready to merge.