fix(google): send thinkingLevel for any model with an effort ladder - #978
fix(google): send thinkingLevel for any model with an effort ladder#978DevMello wants to merge 2 commits into
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Google adapter now uses configured reasoning-effort ladders for eligible Gemini and Vertex models. Tests cover effort mapping, clamping, Vertex configuration, Flash fallback, and image-model exclusions. ChangesGoogle reasoning request handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ModelConfig
participant GoogleAdapter
participant GoogleRequest
ModelConfig->>GoogleAdapter: configured reasoning ladder
GoogleAdapter->>GoogleAdapter: map selected reasoning effort
GoogleAdapter->>GoogleRequest: set thinkingConfig.thinkingLevel
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I came into this review expecting to block it, and the diff changed my mind. Recording that plainly, because the initial reading was mine and it was wrong. The concern I started with. The title reads "for any model with an effort ladder", which sounds like the gate is widened to arbitrary configured Google-adapter providers — the proxy sending a wire field nobody verified the endpoint accepts. That would be the same mistake as promoting unknown capability to supported. What the diff actually does. I ran the same probe against both trees on a custom Google-adapter provider:
So the behavior does widen, but only where the user has asserted a ladder for that model. That is an opt-in capability assertion, not a guess — and it is the same shape we treat as the legitimate remedy elsewhere in the catalog, so blocking it here would be inconsistent with our own position. The underlying bug is real too: One thing I would like before merge, and it is documentation rather than code. Setting Also: the failing Nice, careful piece of work — particularly keeping Vertex frozen and excluding the image models rather than letting |
753ffdc to
6cc8695
Compare
|
✅ Deterministic PR hygiene checks passed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Line 82: Update the modelReasoningEfforts documentation to state that both
provider-wide reasoningEfforts and per-model ladders assert native reasoning
capability for eligible non-image Google models. Describe Cloud Code Assist as
emitting the selected effort through its envelope-specific reasoning path, not
as an exception that omits reasoning fields, while preserving the image-capable
model limitation and warning that unsupported custom endpoints may reject the
request.
In `@tests/google-hardening.test.ts`:
- Around line 380-392: Add a focused non-image test near the existing
image-model test that creates the Google adapter with provider-wide
reasoningEfforts ["low", "high"], builds a request with reasoning "high", and
asserts generationConfig.thinkingConfig equals { thinkingLevel: "high" }. Use
the existing createGoogleAdapter, provider, parsed, and request-building
patterns to verify the provider-wide ladder path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9562f786-4921-4c96-ae4a-0a005a5ee09a
📒 Files selected for processing (3)
docs-site/src/content/docs/reference/configuration/providers.mdsrc/adapters/google.tstests/google-hardening.test.ts
Second loop of the bug campaign, scoped to author corrected replacements for
four contributor PRs and close theirs as absorbed. Three adversarial audit
rounds refuted the premise, and the plan now records that instead of the
outcome it was written to produce.
The decisive finding: I judged "has the author responded to our review" by
`updatedAt`, which moves when WE comment and therefore can never show author
activity. Comparing last-commit time against review time gives the real
picture:
#1092 commits 09:38:19Z review 09:09:51Z -> acted, in under 30 minutes
#1068 commits 08:52:23Z review 09:13:15Z -> predates the review
#1036 commits 08-05 review 09:12:51Z -> no response yet
#997 commits 02:51Z review 09:16:02Z -> no response yet
Under the wrong reading I was about to close #1092 as absorbed — taking
credit for work its author did in direct response to my own request — and to
credit #1068 with agreeing to a review it had not yet read. Neither is a
process nit; both would have been visible to the contributor.
So the plan changes shape. #1092 and #1068 become re-reviews. #1036 and #997
get a stated 72-hour response window with a mandatory head re-check before
anything is authored or closed; "has not replied within an hour" is not
abandonment, and #997's author was active at 02:51Z. This loop therefore
authors no absorbed layer at all, and says so.
#1068's re-review carries a finding neither side has: its new test asserts
all three DeepSeek ids are in `noVisionModels`, but routing merges the
registry list, which holds only the `-free` one. Reproduced with the test's
own routeModel config — Pro=false, Flash=false, Flash-free=true — so two of
three cases fail. Latent because no check currently runs that suite.
Also corrected: the layers were called dependency-ordered when they share no
files, so s3/s4 are independent heads off `origin/dev` and the
`--update-refs` cascade is reserved for genuine chains; and #978's exclusion
no longer claims it is "already correct" when it needs an author-side docs
change.
Terminal outcome is deferred, not DONE. Shrinking the criteria to match what
finished would have hidden exactly the thing worth recording.
6cc8695 to
7bd15d5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
|
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The core fix is valuable and the current eligibility guard is appropriately narrow, but the latest head still needs the two follow-ups identified on the rebased diff.
- The documentation currently describes only
modelReasoningEffortsas the capability assertion.configuredReasoningEfforts()also returns provider-widereasoningEfforts, and that path now emitsgenerationConfig.thinkingConfig.thinkingLevelfor eligible non-image Google models. Please document both ladder sources. - “except Cloud Code Assist” is misleading: CCA does not emit this exact
generationConfigfield, but it does consume the selected effort throughresolveAntigravityEffortWireModel()and its envelope-specific wire-model path. Please describe that distinction instead of implying the effort is omitted. - Add the missing positive regression for a non-image Google model with provider-wide
reasoningEfforts. The current provider-wide test covers only the image-model exclusion, so it would still pass if the newly widened non-image provider-wide path stopped working.
No additional runtime blocker found in the adapter logic. Keeping changes requested until the contract text and provider-wide positive case match the actual implementation, then exact-head CI can make the final merge decision.
|
Thank you — this is landing as #1137 in the stacked campaign, with your Your runtime gate was already correct; what I added on top is the contract around it. The docs now state that both Verified: 129 pass / 0 fail across the affected suites; full Leaving this PR open for you. |
3 participants
Summary
gemini-3.1-pro-previewnow actually sendsgenerationConfig.thinkingConfig.thinkingLevelon the wire. The catalog has advertised low/medium/high for this model since the 3.6 rollout, but the adapter hardcoded thinking togemini-3.5-flashandgemini-3.6-flash, so the picker choice was silently ignored.google-vertexentry ships none, so the Vertex freeze documented in the 260722 rollout plan is preserved. Image models are excluded becausethinkingConfigwould suppressresponseModalities.Verification
bun test tests/google-hardening.test.ts tests/google-adapter.test.ts tests/google-antigravity-wire.test.ts tests/google-vertex-stream.test.ts(77 pass)bun run typecheck, the lint gate, andbun run privacy:scanbun test --isolate ./tests/full suite green (7999 pass, 0 fail)minimaltolow, Vertex emits only with an explicit ladder, unladdered direct flash keeps its slice, image models keepresponseModalitiesunder a provider-wide ladder.Checklist
Summary by CodeRabbit
New Features
Documentation
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.