Commit f58b0f2
authored
feat: Add OpenAI Responses API support and refactor protocol handling (#363)
* feat(llm): add OpenAI Responses API support
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.
* refactor(llm): switch PromptCacheKey to precomputed scheme via ChatRequest.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
* refactor(llm): use canonical protocol name "openai" and UUID-based session 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.
* refactor(llm): remove IsAnthropicProtocol helper and anthropic-vertex special case
* refactor(llm): remove openai-chat-completions branch-internal alias
* docs: remove DESIGN_STATE_CACHE_PHASE and PLAN_RESPONSES_SUPPORT design notes
* fix(llm): address code review findings on Responses API support
- 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
* fix(llm): mirror use_anthropic when setting llm.protocol
- 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
* fix(llm): mirror protocol when setting llm.use_anthropic
- 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
* docs(llm): fix NormalizeProtocol comment to match lowercasing behavior
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.
* docs(llm): drop OpenAI Responses API implementation notes from READMEs
* fix(llm): address code review findings on Responses API support
- 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
* fix(llm): use protocol constants consistently
- 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
* fix(llm): drop stream key and surface non-completed status in Responses 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.1 parent a32f852 commit f58b0f2
25 files changed
Lines changed: 2055 additions & 108 deletions
File tree
- cmd/opencodereview
- internal
- llmloop
- llm
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
219 | 219 | | |
220 | 220 | | |
221 | 221 | | |
222 | | - | |
| 222 | + | |
223 | 223 | | |
224 | 224 | | |
225 | 225 | | |
| |||
253 | 253 | | |
254 | 254 | | |
255 | 255 | | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
256 | 267 | | |
257 | 268 | | |
258 | 269 | | |
| |||
752 | 763 | | |
753 | 764 | | |
754 | 765 | | |
755 | | - | |
| 766 | + | |
756 | 767 | | |
757 | 768 | | |
758 | 769 | | |
| |||
767 | 778 | | |
768 | 779 | | |
769 | 780 | | |
770 | | - | |
| 781 | + | |
| 782 | + | |
771 | 783 | | |
772 | 784 | | |
773 | 785 | | |
| |||
827 | 839 | | |
828 | 840 | | |
829 | 841 | | |
| 842 | + | |
830 | 843 | | |
831 | | - | |
832 | | - | |
| 844 | + | |
833 | 845 | | |
834 | 846 | | |
835 | 847 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
219 | 219 | | |
220 | 220 | | |
221 | 221 | | |
222 | | - | |
| 222 | + | |
223 | 223 | | |
224 | 224 | | |
225 | 225 | | |
| |||
253 | 253 | | |
254 | 254 | | |
255 | 255 | | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
256 | 267 | | |
257 | 268 | | |
258 | 269 | | |
| |||
710 | 721 | | |
711 | 722 | | |
712 | 723 | | |
713 | | - | |
| 724 | + | |
714 | 725 | | |
715 | 726 | | |
716 | 727 | | |
| |||
725 | 736 | | |
726 | 737 | | |
727 | 738 | | |
728 | | - | |
| 739 | + | |
| 740 | + | |
729 | 741 | | |
730 | 742 | | |
731 | 743 | | |
| |||
785 | 797 | | |
786 | 798 | | |
787 | 799 | | |
| 800 | + | |
788 | 801 | | |
789 | | - | |
| 802 | + | |
790 | 803 | | |
791 | 804 | | |
792 | 805 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
219 | 219 | | |
220 | 220 | | |
221 | 221 | | |
222 | | - | |
| 222 | + | |
223 | 223 | | |
224 | 224 | | |
225 | 225 | | |
| |||
253 | 253 | | |
254 | 254 | | |
255 | 255 | | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
256 | 267 | | |
257 | 268 | | |
258 | 269 | | |
| |||
759 | 770 | | |
760 | 771 | | |
761 | 772 | | |
762 | | - | |
| 773 | + | |
763 | 774 | | |
764 | 775 | | |
765 | 776 | | |
| |||
774 | 785 | | |
775 | 786 | | |
776 | 787 | | |
777 | | - | |
| 788 | + | |
| 789 | + | |
778 | 790 | | |
779 | 791 | | |
780 | 792 | | |
| |||
834 | 846 | | |
835 | 847 | | |
836 | 848 | | |
| 849 | + | |
837 | 850 | | |
838 | | - | |
839 | | - | |
| 851 | + | |
840 | 852 | | |
841 | 853 | | |
842 | 854 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
219 | 219 | | |
220 | 220 | | |
221 | 221 | | |
222 | | - | |
| 222 | + | |
223 | 223 | | |
224 | 224 | | |
225 | 225 | | |
| |||
253 | 253 | | |
254 | 254 | | |
255 | 255 | | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
256 | 267 | | |
257 | 268 | | |
258 | 269 | | |
| |||
756 | 767 | | |
757 | 768 | | |
758 | 769 | | |
759 | | - | |
| 770 | + | |
760 | 771 | | |
761 | 772 | | |
762 | 773 | | |
| |||
771 | 782 | | |
772 | 783 | | |
773 | 784 | | |
774 | | - | |
| 785 | + | |
| 786 | + | |
775 | 787 | | |
776 | 788 | | |
777 | 789 | | |
| |||
831 | 843 | | |
832 | 844 | | |
833 | 845 | | |
| 846 | + | |
834 | 847 | | |
835 | | - | |
836 | | - | |
| 848 | + | |
837 | 849 | | |
838 | 850 | | |
839 | 851 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
219 | 219 | | |
220 | 220 | | |
221 | 221 | | |
222 | | - | |
| 222 | + | |
223 | 223 | | |
224 | 224 | | |
225 | 225 | | |
| |||
253 | 253 | | |
254 | 254 | | |
255 | 255 | | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
256 | 267 | | |
257 | 268 | | |
258 | 269 | | |
| |||
741 | 752 | | |
742 | 753 | | |
743 | 754 | | |
744 | | - | |
| 755 | + | |
745 | 756 | | |
746 | 757 | | |
747 | 758 | | |
| |||
756 | 767 | | |
757 | 768 | | |
758 | 769 | | |
759 | | - | |
| 770 | + | |
| 771 | + | |
760 | 772 | | |
761 | 773 | | |
762 | 774 | | |
| |||
816 | 828 | | |
817 | 829 | | |
818 | 830 | | |
| 831 | + | |
819 | 832 | | |
820 | | - | |
821 | | - | |
| 833 | + | |
822 | 834 | | |
823 | 835 | | |
824 | 836 | | |
| |||
0 commit comments