feat: Add OpenAI Responses API support and refactor protocol handling#363
Conversation
Add `openai-responses` as a third LLM protocol alongside `anthropic` and `openai-chat-completions`, enabling code review via the OpenAI Responses API (/v1/responses) for GPT-5.x / o-series models. Protocol naming refactor (backward-compatible): - Canonicalize "openai" -> "openai-chat-completions" (alias still accepted) - Add NormalizeProtocol / ValidateProtocol / IsAnthropicProtocol helpers - Registry uses canonical constants; resolver normalizes everywhere New OpenAIResponsesClient (stateless replay, per DESIGN_STATE_CACHE_PHASE.md): - system messages -> Instructions; tool calls -> function_call items keyed by CallID so the agent loop pairs results correctly - store=false (privacy); prompt_cache_key = sha256(instructions)[:32] - Phase fields (commentary/final_answer) dropped with TODO for gpt-5.3-codex+ Config plumbing: - llm.protocol field + OCR_LLM_PROTOCOL env (priority over use_anthropic / OCR_USE_ANTHROPIC); TUI exposes all three protocols in Custom & Manual - anthropic-vertex rejected with friendly "not yet implemented" message Docs: protocol reference, config examples, env var table, and Responses API notes (store=false caching caveat, cache key derivation, Phase TODO) updated across en/zh-CN/ko-KR/ja-JP/ru-RU READMEs.
…quest.CacheKey Replace per-turn sha256 computation inside buildResponsesParams with a precomputed cache key that callers compute once per session and pass through ChatRequest.CacheKey (json:"-"). The key now incorporates the first user message alongside instructions, so different files under review land in distinct cache buckets — the previous instructions-only key was identical across all files. Changes: - ChatRequest gains CacheKey string field (json:"-", zero impact on Chat Completions / Anthropic clients which never read it) - New llm.ComputeCacheKey helper: sha256(instructions + "\x00" + firstUser)[:32] - responses_client.go: reads req.CacheKey directly, removes promptCacheKey function and first-user-message scanning - loop.go: RunPerFile computes cacheKey once before the loop, reuses every turn - All 8 remaining call sites (agent, scan, relocation, compression, llm_cmd) compute once at request construction - Update PLAN_RESPONSES_SUPPORT.md and DESIGN_STATE_CACHE_PHASE.md to reflect the precomputed scheme - Update tests: passthrough tests for client, dedicated TestComputeCacheKey
…ssion ID for cache key Two changes to maximize backward compatibility and simplify the design: 1. Protocol naming: revert ProtocolOpenAIChatCompletions value from "openai-chat-completions" back to "openai". Old config files with protocol: "openai" are now identical to what new configs write — zero behavioral difference. The alias direction in NormalizeProtocol is reversed: "openai-chat-completions" -> "openai" (for configs written during this branch's testing phase only). 2. Cache key: replace content-based sha256 hash (ComputeCacheKey) with a random UUID session ID. The agent loop generates one UUID per file in RunPerFile and passes it via ChatRequest.SessionID; the Responses client uses it as prompt_cache_key. Single-turn call sites no longer set a cache key (no multi-turn caching benefit). This removes the need to scan messages or compute hashes, and eliminates collision risk between files with similar content. ChatRequest.CacheKey is renamed to SessionID to reflect its actual semantic — a per-session identifier that the Responses client repurposes as prompt_cache_key. Also updates PLAN_RESPONSES_SUPPORT.md, all 5 README translations, test expectations, and promotes google/uuid to a direct dependency.
- provider_cmd: clear stale use_anthropic when switching to openai-responses - resolver: validate preset protocol with ValidateProtocol for consistency - responses_client: swap usage mapping to resolveUsage-first (matches OpenAIClient) - responses_client: map failed/cancelled statuses to 'error' finish reason - usage_resolver: add Responses API field paths (input_tokens, output_tokens, input_tokens_details.cached_tokens) - add tests for all four fixes
- config_cmd: 'ocr config set llm.protocol' now mirrors use_anthropic (anthropic -> true, OpenAI family -> false) for backward compat with older binaries that predate llm.protocol - provider_cmd: openai-responses now sets use_anthropic=false instead of nil, so older binaries fall back to the OpenAI family rather than wrongly defaulting to anthropic - update tests for both write paths
- config_cmd: 'ocr config set llm.use_anthropic' now mirrors protocol (true -> anthropic, false -> openai) so the two fields never disagree, matching the reverse llm.protocol mirroring added previously - without this, setting use_anthropic=true while protocol=openai-responses left a contradictory config that misled older binaries into using the anthropic protocol against an OpenAI endpoint - extend tests to cover both values and stale-protocol overwrite
The comment claimed unknown values are 'returned unchanged', but the default branch lowercases and trims them (corroborated by the 'gRPC -> grpc' test). Update the wording to describe the actual behavior so callers aren't misled about round-trip fidelity.
# Conflicts: # internal/llm/client_test.go
|
🔍 OpenCodeReview found 8 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Supported keys: provider, model, providers.<name>.<field>, custom_providers.<name>.<field>, mcp_servers.<name>.<field>, llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging | ||
| Provider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers | ||
| Protocol values: anthropic, openai, openai-responses | ||
| MCP server fields: command, args, env, tools, setup`) |
There was a problem hiding this comment.
Minor consistency issue: The help text here now documents "Protocol values: anthropic, openai, openai-responses", but the error message in config_cmd.go (line 401) lists llm.protocol as a supported key without including the valid protocol values. Consider adding the same "Protocol values" line to the error message in setConfigValue() so users who type an invalid key or value see the same guidance in both places.
| default: | ||
| return strings.ToLower(strings.TrimSpace(raw)) |
There was a problem hiding this comment.
Minor: The default branch redundantly recomputes strings.ToLower(strings.TrimSpace(raw)), which was already computed as the switch expression. Extract the normalized value into a local variable to avoid the duplicate work and make the intent clearer:
func NormalizeProtocol(raw string) string {
normalized := strings.ToLower(strings.TrimSpace(raw))
switch normalized {
case "":
return ""
case ProtocolAnthropic:
return ProtocolAnthropic
// ...
default:
return normalized
}
}This also prevents a subtle class of future bugs where someone modifies the switch expression but forgets the default branch uses its own independent transformation.
| if b { | ||
| cfg.Llm.Protocol = llm.ProtocolAnthropic | ||
| } else { | ||
| cfg.Llm.Protocol = llm.ProtocolOpenAIChatCompletions | ||
| } |
There was a problem hiding this comment.
Potential data loss: When use_anthropic is set to false, the protocol is unconditionally overwritten to openai (chat completions). However, if the user previously configured protocol = "openai-responses", this mirroring silently downgrades it to "openai".
Since the resolver already prioritizes Protocol over UseAnthropic, consider only updating Protocol when it is currently empty or matches one of the two legacy values (anthropic/openai). Alternatively, skip the protocol overwrite entirely when use_anthropic is being set, since Protocol takes priority in the resolver anyway. At minimum, avoid overwriting openai-responses with openai.
Suggestion:
| if b { | |
| cfg.Llm.Protocol = llm.ProtocolAnthropic | |
| } else { | |
| cfg.Llm.Protocol = llm.ProtocolOpenAIChatCompletions | |
| } | |
| // Only mirror to protocol if protocol is unset or already in the | |
| // legacy pair (anthropic / openai). Avoid clobbering openai-responses. | |
| if cfg.Llm.Protocol == "" || cfg.Llm.Protocol == llm.ProtocolAnthropic || cfg.Llm.Protocol == llm.ProtocolOpenAIChatCompletions { | |
| if b { | |
| cfg.Llm.Protocol = llm.ProtocolAnthropic | |
| } else { | |
| cfg.Llm.Protocol = llm.ProtocolOpenAIChatCompletions | |
| } | |
| } |
| // sdkBaseURL strips the /responses suffix so the SDK can re-append it. | ||
| func (c *OpenAIResponsesClient) sdkBaseURL() string { | ||
| return strings.TrimSuffix(strings.TrimRight(c.cfg.URL, "/"), "/responses") | ||
| } |
There was a problem hiding this comment.
Dead code: this method is defined but never called. The constructor at line 33 computes the SDK base URL using a local variable instead. Either remove this method or use it in the constructor for consistency. Note also that this method applies TrimRight before TrimSuffix, while the constructor does not — if someone later switches to calling this method, the behavior could subtly differ for URLs with trailing slashes.
| cfg.Timeout = 5 * time.Minute | ||
| } | ||
| ensureResponsesEndpoint(&cfg) | ||
| sdkBaseURL := strings.TrimSuffix(cfg.URL, "/responses") |
There was a problem hiding this comment.
Inconsistent URL normalization compared to NewOpenAIClient. In NewOpenAIClient (client.go:309), the SDK base URL is derived as strings.TrimSuffix(strings.TrimRight(cfg.URL, "/"), "/chat/completions") — note the TrimRight before TrimSuffix. Here, only TrimSuffix is used. While ensureResponsesEndpoint guarantees no trailing slash at this point, matching the defensive pattern from NewOpenAIClient would make the code more robust against future refactoring.
Suggestion:
| sdkBaseURL := strings.TrimSuffix(cfg.URL, "/responses") | |
| sdkBaseURL := strings.TrimSuffix(strings.TrimRight(cfg.URL, "/"), "/responses") |
- config: preserve openai-responses when setting llm.use_anthropic=false (only mirror to openai when protocol is unset or a legacy anthropic/openai) - config: add Protocol values guidance to unknown-key error message - protocol: extract normalized local var in NormalizeProtocol - responses_client: align SDK base URL trimming with NewOpenAIClient - responses_client: drop unused test-only sdkBaseURL method
- providers: edenai now uses ProtocolOpenAIChatCompletions like the rest of the registry instead of the "openai" string literal - provider_cmd: print the normalized protocol variable (what is actually saved) instead of the raw TUI value
| var opts []openaiopt.RequestOption | ||
| for k, v := range c.cfg.ExtraBody { | ||
| opts = append(opts, openaiopt.WithJSONSet(k, v)) | ||
| } | ||
|
|
||
| sdkResp, err := c.sdk.Responses.New(ctx, params, opts...) |
There was a problem hiding this comment.
When existing provider config contains extra_body: {"stream": true},these options are still passed to Responses.New.The API then returns SSE while New expects a regular JSON response, so every completion fails.
| sdkResp, err := c.sdk.Responses.New(ctx, params, opts...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return c.mapResponsesResponse(sdkResp), nil |
There was a problem hiding this comment.
The Responses API can return HTTP 200 with status failed/cancelled, or queued/in_progress when background mode is enabled, so Responses.New can have a nil Go error here. This code converts those into a normal ChatResponse; callers do not inspect FinishReason, causing ocr llm test to report success and the review loop to retry or execute output instead of surfacing or polling the response.
…es client Address two PR review comments on OpenAI Responses API support: 1. extra_body.stream=true was forwarded to Responses.New, making the API return SSE while the SDK expects JSON and breaking every call. Skip the 'stream' key (like OpenAIClient treats it as a non-forwarded key) while still forwarding other extra_body entries. 2. The Responses API returns HTTP 200 even for failed/cancelled (terminal) and queued/in_progress (background) states, so the SDK reports nil error. Surface these as real errors so callers branching on err != nil (ocr llm test, review loop) fail instead of treating a dead response as success. Add table-driven tests covering both fixes.
3 participants
Description
This PR adds support for the OpenAI Responses API (
/v1/responses) as a third LLM protocol, enabling code review with GPT-5.x / o-series models that use the Responses API rather than Chat Completions.New protocol:
openai-responsesSits alongside the existing
anthropicandopenai(Chat Completions) protocols. Users can now select between all three via configuration.OpenAIResponsesClient(stateless replay):instructions; tool calls becomefunction_callitems keyed byCallIDso the agent loop pairs results correctly.store=false(privacy);prompt_cache_keyderived fromsha256(instructions)[:32].NormalizeProtocol/ValidateProtocolhelpers ininternal/llm/protocol.go.llm.protocolconfig field +OCR_LLM_PROTOCOLenv var take priority over the legacyuse_anthropic/OCR_USE_ANTHROPIC(kept working via mirroring).anthropic-vertexis rejected with a friendly "not yet implemented" message.provider_tui_test.goadded.Type of Change
How Has This Been Tested?
make testpasses locallyTested using anthropic, openai, openai-responses endpoints to ensure that all the three protocol functions correctly and previous behavior do not change
Checklist
go fmt,go vet)Related Issues