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
| "gcs_bucket", | ||
| ], | ||
| sensitive_fields=["api_key", "sa_key"], | ||
| sensitive_fields=["api_key"], |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep sa_key masked until legacy Vertex credentials are removed.
mask_credential_fields() derives its mask list from this configuration. With only api_key listed, a stored Google sa_key object—including private_key, as shown in backend/app/tests/crud/test_credentials.py:89-132—passes through unchanged and can be exposed by API-facing credential responses.
Retain sa_key as sensitive, or explicitly strip legacy service-account data before returning credentials.
Proposed fix
- sensitive_fields=["api_key"],
+ sensitive_fields=["api_key", "sa_key"],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sensitive_fields=["api_key"], | |
| sensitive_fields=["api_key", "sa_key"], |
🤖 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/core/providers.py` at 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.
| 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
1 participant
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.