Refactor/options clients#55
Conversation
![managerbot-app[bot]](https://avatars.githubusercontent.com/in/853044?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed Changes
Managerbot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Files not reviewed (1)
- .pi/skills/options-development/SKILL.md
Comments suppressed due to low confidence (3)
-
lib/llm_gateway/adapters/anthropic_option_mapper.rb:27
- (are_you_sure) The `cache_retention` field was previously forwarded as-is to the Anthropic API, but it has been removed from both `VALID_OPTIONS` and `MANAGED_OPTIONS`. This means if a caller passes `cache_retention` to the Anthropic mapper, it will now be passed through unmapped and trigger a validation error, rather than being silently dropped or forwarded. Consider adding `cache_retention` to `MANAGED_OPTIONS` if it should be silently discarded for Anthropic, or document this intentional breaking change. test/unit/options/anthropic_option_mapper_test.rb:47
- (test_completeness) The test checks that passing `messages:`, `tools:`, and `system:` raises an `ArgumentError`, which validates the validation boundary. However, there is no test verifying that `cache_retention` (previously supported) is now rejected. If this is an intentional breaking change, a test documenting the new behaviour would make this explicit. test/unit/options/openai_chat_completions_option_mapper_test.rb:46
- (test_completeness) The integration-style test using a fake client is a good addition, but there is no longer a unit test verifying that invalid `cache_retention` values (e.g., `"week"`) raise an `ArgumentError`. The old test for this was removed. Consider adding it back since the validation logic still exists in `normalize_cache_retention`.
| stream | ||
| stream_options | ||
| temperature | ||
| tool_choice |
There was a problem hiding this comment.
tool_choice is listed in VALID_OPTIONS but the comment above states that transcript/tool structural fields (messages, tools) are intentionally excluded. If tool_choice is passed as an option it will be forwarded to the API, but tools itself is excluded. This asymmetry may cause confusion — consider clarifying the comment or removing tool_choice from VALID_OPTIONS for consistency.
|
|
||
| map_cache_key!(mapped_options) | ||
| map_prompt_cache_retention!(mapped_options) | ||
| if mapped_options[:prompt_cache_key] && !mapped_options[:prompt_cache_retention] |
There was a problem hiding this comment.
When cache_retention is "none", both prompt_cache_key and prompt_cache_retention are deleted from mapped_options. However, normalize_cache_retention("none") returns nil rather than raising an error (line 112). This means setting prompt_cache_retention to nil via the normalize call and then deleting it is redundant. Consider skipping the normalize_cache_retention call altogether when cache_retention == "none" to avoid the unnecessary intermediate assignment.
| mapped_options[:prompt_cache_retention] = normalize_cache_retention("short") | ||
| end | ||
|
|
||
| if cache_retention.to_s == "none" |
There was a problem hiding this comment.
Same redundancy as in the Chat Completions mapper: normalize_cache_retention("none") returns nil, so mapped_options[:prompt_cache_retention] is first set to nil and then the key is deleted. Consider guarding the normalize call with cache_retention.to_s != "none" to avoid the redundant assignment.
| response_format = options[:response_format] | ||
| mapped_options[:text] = text_with_response_format(mapped_options[:text], response_format) unless response_format.nil? | ||
|
|
||
| reasoning = mapped_options.delete(:reasoning) |
There was a problem hiding this comment.
The reasoning key is handled differently here compared to the Chat Completions mapper: it is added to VALID_OPTIONS for the Responses API (since it is a native API parameter), whereas in Chat Completions it maps to reasoning_effort. However, reasoning is also in MANAGED_OPTIONS, which means it is stripped before mapping and then re-added. Be careful: if someone passes reasoning as a native hash (e.g., { effort: "high" }), it will still go through normalize_reasoning, which only accepts string levels. This could silently reject valid native usage — consider checking the type before normalizing.
|
|
||
| def text_with_response_format(text, response_format) | ||
| text_options = text ? text.dup : {} | ||
| text_options[:format] = response_format.is_a?(String) ? { type: response_format } : response_format |
There was a problem hiding this comment.
In text_with_response_format, text_options is initialized as an empty hash {} when text is nil, but the method uses text.dup when text is truthy. If text is a frozen hash or a non-Hash object (e.g., a string), dup will produce a shallow copy which may be fine, but calling [:format] on a non-Hash text will raise a NoMethodError. Consider adding a type guard: text_options = text.is_a?(Hash) ? text.dup : {}.
| end | ||
|
|
||
| assert_raises(ArgumentError) do | ||
| LlmGateway::Adapters::OpenAI::Responses::OptionMapper.map(system: "You are helpful") |
There was a problem hiding this comment.
The system: keyword argument is captured in the fake client's stream_responses signature but is unused. If this is intentional (matching the real client's signature), add a leading _ to make it _system: to signal it is intentionally unused and avoid potential linter warnings.
1 participant
No description provided.