Fix Gemini3 reasoning-only response handling #12
+2,591
−491
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replaces method call with property access for model_info_service readiness check in main.py. Also ensures text content is stripped of whitespace before appending in AntigravityProvider, improving data consistency.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to fc75422 in 2 minutes and 15 seconds. Click for details.
- Reviewed
53lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/proxy_app/main.py:951
- Draft comment:
Using the property 'is_ready' (without calling) is correct if it's defined as a property. Confirm that the interface of model_info_service has been updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold1%The comment is asking the PR author to confirm an update to the interface, which is not allowed according to the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
2. src/rotator_library/providers/antigravity_provider.py:1844
- Draft comment:
In _gemini_to_openai_non_streaming, if a response contains only reasoning content (no text), the 'content' field is omitted. Consider changing the condition from 'elif not text_content and not reasoning_content:' to 'elif not text_content:' so that an empty content field is always set, ensuring compatibility with OpenAI's message format. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment is about the logic for setting the content field in OpenAI format responses. The current code only setsmessage["content"] = ""when BOTH text_content and reasoning_content are absent. The comment suggests that if there's reasoning_content but no text_content, we should still set an empty content field. However, I need to consider: Is this actually required by OpenAI's format? Looking at the code structure, when tool_calls exist, content is explicitly removed (line 1845). This suggests that content is optional in some cases. The question is whether having reasoning_content without content is valid in OpenAI's format. The diff shows this is in the non-streaming response handler. The change moved the empty content assignment to be more specific (only when there's no text AND no reasoning). This seems intentional - if there's reasoning_content, perhaps content is not required. Without seeing OpenAI's actual spec or evidence that this causes issues, I cannot confirm this is a bug. I don't have access to OpenAI's API specification to verify whether the content field is required when reasoning_content is present. The comment makes an assumption about "compatibility with OpenAI's message format" but provides no evidence that the current implementation causes issues. The code change in the diff appears intentional, with a comment explaining the logic. The comment is speculative ("Consider changing") and doesn't provide evidence of an actual compatibility issue. The diff shows this logic was deliberately changed with an explanatory comment. Without concrete evidence that this breaks OpenAI compatibility or causes errors, this appears to be a suggestion based on assumptions rather than a demonstrated bug. This comment should be deleted. It's speculative and suggests a change without providing evidence that the current implementation causes any issues. The diff shows the logic was intentionally changed with a clear comment explaining the reasoning. The comment violates the rule against speculative comments ("Consider changing...").
Workflow ID: wflow_cqBL5GcEke6AL9WW
You can customize 
Owner
|
I weren't notified i got PR's open, so i missed this completely. |
Owner
|
I am gonna review this in ~14 hours. Is this the final form, or..? Seems like so far it's qol of extracting stuff - would be nice to have more descriptive commit descriptions(llm generated, cause no human is writing those) As for changes so far: I made each provider to be self-contained, with refactoring to be done when the feature branch is basically complete. Not strictly a problem for this right now, but there was indeed a lot of "bloat", and probably still is. |
Mirrowel
reviewed
Nov 30, 2025
Comment on lines
+114
to
+115
| if not api_key: | ||
| raise ValueError("api_key parameter is required and cannot be empty") |
Owner
There was a problem hiding this comment.
Actually - it is not required. Proxy uses the Api key. Library itself serves up what proxy requests, if it requests.
So it's not it's job to validate the key or even ask for it - Proxy does that already. Library doesn't even receive proxy's API key, and provider uses any api key it has to check(usually. Antigravity doesn't have dynamic discovery in the endpoint)
Mirrowel
reviewed
Nov 30, 2025
Comment on lines
209
to
224
| def validate_reasoning_effort(reasoning_effort: Optional[str]) -> None: | ||
| """ | ||
| Validate reasoning_effort parameter for Claude models. | ||
| Args: | ||
| reasoning_effort: Optional reasoning effort level | ||
| Raises: | ||
| ValueError: If reasoning_effort is invalid | ||
| """ | ||
| if reasoning_effort is not None: | ||
| valid_levels = {'low', 'medium', 'high'} | ||
| if reasoning_effort not in valid_levels: | ||
| raise ValueError( | ||
| f"reasoning_effort must be one of {', '.join(valid_levels)}, got '{reasoning_effort}'" | ||
| ) No newline at end of file |
Owner
There was a problem hiding this comment.
Another problem of validation - reasoning effort of "disabled" (iirc) also exists, and disables reasoning.
Why? models that reason by default need a way to disable reasoning, which is why disabled exists.
Strictly here - this is correct. But imo it should be consistent for all providers, supporting same levels.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 01d39f0 in 2 minutes and 22 seconds. Click for details.
- Reviewed
3549lines of code in14files - Skipped
1files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/rotator_library/providers/antigravity_provider.py:1014
- Draft comment:
Gemini 3 reasoning conversion branch: currently, the code hits apassunder the condition for Gemini 3 when_enable_gemini3_thinking_conversionis true. This means that responses with reasoning_content but no regular text might be dropped. Until full conversion logic is implemented, consider a temporary fallback (e.g. appending the reasoning content as plain text) or at least add a clear log/error so that reasoning-only responses are not silently ignored. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% Looking at the code flow in_transform_assistant_message, after the Gemini 3 conversion branch (lines 1035-1040), the code continues to add regular content (line 1047-1048) and tool calls (lines 1050-1101). At the end (lines 1103-1111), there's a safety check that ensures at least one part is returned by adding an empty text part if needed. So even if reasoning_content exists but isn't converted, the message won't be completely dropped - it will have at least an empty text part. The comment's concern about "silently ignored" responses seems overstated. However, the comment does have a point that the reasoning_content itself is being ignored in the Gemini 3 case when conversion is enabled. But this appears to be intentional - it's a "Future enhancement" with a flag to control it. The flag defaults to True (line 385), so this is opt-in behavior. The code has a safety mechanism (lines 1103-1111) that prevents messages from being completely dropped. The comment may be overstating the severity of the issue. Also, this is clearly marked as "Future enhancement" and controlled by a feature flag, suggesting this is intentional incomplete functionality rather than a bug. While there is a safety mechanism to prevent empty messages, the reasoning_content itself is indeed being silently ignored when this branch is hit. The comment's suggestion to add logging or a temporary fallback is reasonable for incomplete functionality. However, there IS already a debug log on line 1037 that indicates conversion is happening, which provides some visibility. The comment raises a valid concern about incomplete functionality, but the code already has: 1) a debug log indicating conversion is happening, 2) a safety mechanism preventing completely empty messages, and 3) a feature flag to control this behavior. The comment is somewhat speculative ("might be dropped") and the suggestion is already partially addressed by existing logging. This is clearly marked as future work.
2. .github/prompts/pr-review.md:30
- Draft comment:
Typo: There's no space between the heading markers and the text in the heading on this line. Consider changing "###STRICT RULES FOR COMMENT SIGNAL:" to "### STRICT RULES FOR COMMENT SIGNAL:" for better readability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This is a straightforward formatting fix for markdown. The comment correctly identifies a typo where the space is missing between ### and the heading text. While this is a minor style issue, it's actually a valid markdown syntax concern - some markdown parsers may not properly render headings without the space. The comment provides a clear suggestion block that makes it a one-click fix. However, I need to consider the rules: "Do NOT comment on anything that would be obviously caught by the build" and "Do NOT make comments that are obvious or unimportant." This is a trivial style preference that doesn't affect functionality. The rules also say to "Trust linters for formatting" and avoid "minor stylistic points." While this is a legitimate markdown formatting issue, it falls under "trivial style preferences" that the rules explicitly say not to comment on. A markdown linter would catch this, and it doesn't affect the actual functionality of the prompt file. The rules are very clear about avoiding minor stylistic points and trusting linters for formatting issues. The critique is valid. This is exactly the type of minor formatting issue that should be caught by linters rather than manual review. It doesn't impact functionality, readability is barely affected, and it's the definition of a trivial nit. The rules explicitly state to avoid such comments. This comment should be deleted. It's a trivial markdown formatting issue that should be caught by a linter, not flagged in a PR review. It violates the rule against commenting on minor stylistic points and obvious/unimportant issues.
3. .github/prompts/pr-review.md:196
- Draft comment:
Potential formatting issue: The code block starting with "bash" is not closed. If this is unintentional, please add a closing "" to ensure proper markdown formatting. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_EV0pr6BLKsV2nLez
You can customize
Author
|
@Mirrowel Gotta be honest, still getting used to the whole GitHub thing. Anyway, thank you so much for the code review. Learned a lot from your feedback and from working on the project in general. |
Owner
I only made a glance. My bot agent is supposed to review, but workflows failed for this PR for some reason. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Important
Enhance Gemini3 reasoning-only response handling in Antigravity provider with improved error handling, utility functions, and compliance workflows.
antigravity_provider.py.acompletion()andget_models()._get_context_window()for model-specific context window overrides.antigravity_utilspackage for better organization.json_parsers.py,request_helpers.py, andschema_transformers.pyfor JSON parsing, request ID generation, and schema normalization.antigravity_types.pyfor TypedDict definitions to improve type safety.antigravity_validators.pyfor input validation functions.compliance-check.ymlfor AI-powered compliance verification.status-check-init.ymlto initialize compliance status checks.This description was created by
for 01d39f0. You can customize this summary. It will automatically update as commits are pushed.