fix(action): honor an explicit llm_protocol instead of forcing openai or anthropic - #1192
fix(action): honor an explicit llm_protocol instead of forcing openai or anthropic#1192po-et wants to merge 1 commit into
Conversation
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). |
Qiyuanqiii
left a comment
There was a problem hiding this comment.
Request changes — the runtime fix looks correct, but the inherited protocol is currently missing from the checkpoint fingerprint.
Configure OCR now correctly resolves the effective protocol from llm_protocol first and inherited OCR_LLM_PROTOCOL second. However, Resolve review range fingerprints only ${{ inputs.llm_protocol }} via OCR_FP_LLM_PROTOCOL.
With checkpoint_range: true, this means two runs can have the same fingerprint even when the effective protocol changes solely through the job/step environment, for example:
OCR_LLM_PROTOCOL=openai
→
OCR_LLM_PROTOCOL=openai-responses
If llm_protocol remains empty and the other inputs are unchanged, the previous checkpoint may therefore be treated as configuration-compatible and the next run can review only the delta, despite the effective LLM configuration having changed. This conflicts with the checkpoint contract that configuration changes must fail closed to a full review.
Could we fingerprint the same normalized effective protocol that Configure OCR resolves (input first, inherited env second), while retaining the current backward compatibility when neither source is set?
It would also be useful to add a contract test covering the env-only path: changing inherited OCR_LLM_PROTOCOL with identical action inputs should invalidate the previous checkpoint.
Other than this checkpoint interaction, the protocol precedence, use_anthropic mirroring, validation, documentation, and Configure OCR contract coverage look good.
… or anthropic The Configure OCR step derived llm.protocol from llm_use_anthropic alone, so it always wrote openai or anthropic. Anyone on the Responses API was sent to /v1/chat/completions no matter what OCR_LLM_PROTOCOL they set. Add an optional llm_protocol input (anthropic, openai, openai-responses). When it is set, or when OCR_LLM_PROTOCOL is inherited from the job environment, that value is written and llm.use_anthropic is mirrored from it, the same way `ocr config set llm.protocol` mirrors the boolean. An unknown value fails the step with the accepted list. The explicit protocol joins the checkpoint fingerprint, appended only when set so existing checkpoints stay valid. Fixes alibaba#1134
16a51f0 to
672c1b0
Compare
Fixes #1134.
The Configure OCR step derived
llm.protocolfromllm_use_anthropicalone, so it always wroteopenaioranthropic, and users on the Responses API were sent to/v1/chat/completionsno matter what they put inOCR_LLM_PROTOCOL.This adds an optional
llm_protocolinput (anthropic,openai,openai-responses). When it is set, or whenOCR_LLM_PROTOCOLis inherited from the job environment, that value is written andllm.use_anthropicis mirrored from it, the same wayocr config set llm.protocolmirrors the boolean. An unknown value fails the step with the accepted list. With neither set, nothing changes. The explicit protocol is added to the checkpoint fingerprint, appended only when set so existing checkpoints stay valid for everyone who never uses the input.Contract tests cover the input, the inherited variable, precedence between the two, case folding, and the rejection of an unknown value. The GitHub Actions README documents the input.