Onboarding: Revert Google enum fix#1062
Conversation
📝 WalkthroughWalkthroughGoogle provider configuration now masks only ChangesGoogle provider behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
changed the title
OpenAPI changes ⚪ No API surface changesNote This PR does not modify the API contract.
|
Prajna1999
changed the title
![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
🧹 Nitpick comments (1)
backend/app/tests/services/llm/test_mappers.py (1)
901-906: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that valid routing emits no warnings.
The test verifies the provider and model but ignores
warnings; a mapper regression could therefore pass while silently dropping parameters.Proposed assertion
assert native_config.provider == "google-native" assert native_config.params["model"] == "gemini-2.5-pro" + assert warnings == []🤖 Prompt for 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. In `@backend/app/tests/services/llm/test_mappers.py` around lines 901 - 906, Update the test around transform_kaapi_config_to_native to assert that warnings is empty for this valid routing configuration, while preserving the existing provider and model assertions.
🤖 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 `@backend/app/core/providers.py`:
- Line 56: Update the provider configuration’s sensitive_fields list to retain
sa_key alongside api_key, ensuring mask_credential_fields() continues masking
legacy Google service-account credentials, including private_key, in API-facing
responses.
In `@backend/app/tests/services/llm/test_mappers.py`:
- Line 892: Update the test_text_completion_maps_via_google_mapper method
signature to include the missing None return annotation, while retaining the
existing db: Session parameter annotation.
---
Nitpick comments:
In `@backend/app/tests/services/llm/test_mappers.py`:
- Around line 901-906: Update the test around transform_kaapi_config_to_native
to assert that warnings is empty for this valid routing configuration, while
preserving the existing provider and model assertions.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 3da82390-f6c8-4923-9f49-a948e4c10713
📒 Files selected for processing (3)
backend/app/core/providers.pybackend/app/services/llm/mappers.pybackend/app/tests/services/llm/test_mappers.py
💤 Files with no reviewable changes (1)
- backend/app/services/llm/mappers.py
| def test_text_completion_is_rejected(self, db: Session): | ||
| """``google`` is audio-only (Vertex STT/TTS) — text completions | ||
| must be routed through ``google-aistudio``.""" | ||
| def test_text_completion_maps_via_google_mapper(self, db: Session): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the missing -> None return annotation.
As per coding guidelines: **/*.py: Use type hints on every function parameter and return value.
🤖 Prompt for 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.
In `@backend/app/tests/services/llm/test_mappers.py` at line 892, Update the
test_text_completion_maps_via_google_mapper method signature to include the
missing None return annotation, while retaining the existing db: Session
parameter annotation.
Source: Coding guidelines
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🎉 This PR is included in version 1.4.0-main.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
3 participants
Issue #1060
Summary
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
This was a hotfix already deployed to prod. To be merged with main for staging and next steps.
Original PR description
Issue #1060
Summary
Why it's safe
Reviewer confirm
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
This was a hotfix already deployed to prod. To be merged with main for staging and next steps.