Anthropic and Google Vertex: Add Claude and Gemini to llm_call endpoint#896
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an in-memory AudioRef abstraction, implements Anthropic (Claude) and Google Vertex providers (STT/TTS), adds GCS audio staging and masking helpers, updates mappers/configs/registry for new providers and default models, and aligns tests to the AudioRef-based flow. ChangesMulti-Provider LLM Expansion with Audio Abstraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Prajna1999
changed the title
…laude-integration
OpenAPI changes 🔴 6 breaking changesCaution Downstream consumers may need an update before merging. Breaking changes ·
|
| Method | Path | Change | |
|---|---|---|---|
| 🔴 | GET |
/api/v1/models |
added the new anthropic enum value to the data/anyOf[subschema #1: ModelConfigListPublic]/data/items/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models |
added the new google-vertex enum value to the data/anyOf[subschema #1: ModelConfigListPublic]/data/items/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models/grouped |
added the new anthropic enum value to the data/anyOf[subschema #1]/additionalProperties/items/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models/grouped |
added the new google-vertex enum value to the data/anyOf[subschema #1]/additionalProperties/items/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models/{provider}/{model_name} |
added the new anthropic enum value to the data/anyOf[subschema #1: ModelConfigPublic]/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models/{provider}/{model_name} |
added the new google-vertex enum value to the data/anyOf[subschema #1: ModelConfigPublic]/provider response property for the response status 200 |
Full changelog · 18
| Method | Path | Change | |
|---|---|---|---|
| 🔴 | GET |
/api/v1/models |
added the new anthropic enum value to the data/anyOf[subschema #1: ModelConfigListPublic]/data/items/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models |
added the new google-vertex enum value to the data/anyOf[subschema #1: ModelConfigListPublic]/data/items/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models/grouped |
added the new anthropic enum value to the data/anyOf[subschema #1]/additionalProperties/items/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models/grouped |
added the new google-vertex enum value to the data/anyOf[subschema #1]/additionalProperties/items/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models/{provider}/{model_name} |
added the new anthropic enum value to the data/anyOf[subschema #1: ModelConfigPublic]/provider response property for the response status 200 |
| 🔴 | GET |
/api/v1/models/{provider}/{model_name} |
added the new google-vertex enum value to the data/anyOf[subschema #1: ModelConfigPublic]/provider response property for the response status 200 |
| 🟢 | POST |
/api/v1/configs |
added the new anthropic enum value to the request property config_blob/completion/anyOf[subschema #2: KaapiCompletionConfig]/provider/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/configs |
added the new anthropic-native enum value to the request property config_blob/completion/anyOf[subschema #1: NativeCompletionConfig]/provider |
| 🟢 | POST |
/api/v1/configs |
added the new google-vertex enum value to the request property config_blob/completion/anyOf[subschema #2: KaapiCompletionConfig]/provider/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/configs |
added the new google-vertex-native enum value to the request property config_blob/completion/anyOf[subschema #1: NativeCompletionConfig]/provider |
| 🟢 | POST |
/api/v1/llm/call |
added the new anthropic enum value to the request property config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #2: KaapiCompletionConfig]/provider/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/call |
added the new anthropic-native enum value to the request property config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #1: NativeCompletionConfig]/provider |
| 🟢 | POST |
/api/v1/llm/call |
added the new google-vertex enum value to the request property config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #2: KaapiCompletionConfig]/provider/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/call |
added the new google-vertex-native enum value to the request property config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #1: NativeCompletionConfig]/provider |
| 🟢 | POST |
/api/v1/llm/chain |
added the new anthropic enum value to the request property blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #2: KaapiCompletionConfig]/provider/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain |
added the new anthropic-native enum value to the request property blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #1: NativeCompletionConfig]/provider |
| 🟢 | POST |
/api/v1/llm/chain |
added the new google-vertex enum value to the request property blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #2: KaapiCompletionConfig]/provider/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain |
added the new google-vertex-native enum value to the request property blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #1: NativeCompletionConfig]/provider |
main ↔ b62c3648 · generated by oasdiff
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/app/utils.py (1)
716-720:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the downloaded MIME type when the request omits one.
For URL audio, this branch falls back to
"audio/wav"whenever the caller does not sendmime_type, even thoughdownload_audio_bytes()already inspects the realContent-Type. After this refactor that wrong MIME now drivesAudioRef.mime_type, temp-file suffix selection, and any provider-side content metadata, so anaudio/mpegURL can be mislabeled as WAV.🤖 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/utils.py` around lines 716 - 720, The URL-audio branch currently forces mime_type = query_input.content.mime_type or "audio/wav", which overrides the real Content-Type detected by download_audio_bytes and causes AudioRef.mime_type and temp-file suffix to be wrong; change the logic so resolve_audio_url (and/or download_audio_bytes it uses) is allowed to detect and return the true MIME when query_input.content.mime_type is missing—i.e., pass None or leave mime unset when content.mime_type is falsy, update resolve_audio_url to return the detected mime (or an AudioRef with correct mime) and ensure AudioRef.mime_type is set from that detection rather than the default "audio/wav"; keep resolve_audio_base64 behavior unchanged for base64 inputs.backend/app/services/llm/providers/gai.py (2)
161-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid logging merged instructions verbatim.
Line 162 writes the fully merged prompt/instruction text to logs. That content is caller-controlled and can contain sensitive data, so this should be reduced to metadata only and prefixed with the function name.
Suggested change
- logger.info( - f"The merged instructions is {merged_instruction} and output language is {output_language} and input language is {input_language}" - ) + logger.info( + f"[GoogleAIProvider._execute_stt] Prepared transcription request | " + f"provider={provider}, input_language={input_language}, " + f"output_language={output_language}, has_custom_instructions={bool(instructions)}" + )As per coding guidelines,
**/*.py:Prefix all log messages with the function name in square brackets: logger.info(f"[function_name] Message {mask_string(sensitive_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/services/llm/providers/gai.py` around lines 161 - 163, The current logger.info call logs the full caller-controlled merged_instruction; replace it with a metadata-only log prefixed by the function name in square brackets (do not emit the raw prompt). Update the logger.info that references merged_instruction/output_language/input_language to log only: the function name in brackets, the languages, and a masked version of merged_instruction using mask_string(merged_instruction) (e.g., "[function_name] merged_instruction=%s output_language=%s input_language=%s" with mask_string applied to merged_instruction). Ensure no raw merged_instruction is ever passed to the logger and keep output_language/input_language as plain metadata.
468-480:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
GoogleAIProvider.execute()resolved_input typing with STTAudioRef
LLMProviderBase.execute()typesresolved_inputasstr | AudioRef | list[ContentPart] | MultiModalInput, andgai.py’s_execute_stt()requiresAudioRef(with a runtimeisinstancecheck).gai.py’sexecute()override currently omitsAudioRef, even though the"stt"branch forwardsresolved_inputinto_execute_stt().Suggested change
def execute( self, completion_config: NativeCompletionConfig, query: QueryParams, - resolved_input: str | list[ContentPart] | MultiModalInput, + resolved_input: AudioRef | str | list[ContentPart] | MultiModalInput, include_provider_raw_response: bool = False, ) -> tuple[LLMCallResponse | None, str | None]:Also check the same override type-narrowing pattern in
eai.pyandsai.py(execute()typed asstrwhile_execute_stt()expectsAudioRef).🤖 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/services/llm/providers/gai.py` around lines 468 - 480, The override GoogleAIProvider.execute currently types resolved_input as str | list[ContentPart] | MultiModalInput but forwards it to _execute_stt which requires an AudioRef; update the execute signature to include AudioRef (matching LLMProviderBase.execute) so the "stt" branch passes a correctly typed value, and add/adjust any local type checks/casts as needed; also inspect and apply the same fix to the execute overrides in eai.py and sai.py to ensure their resolved_input signatures include AudioRef and align with their _execute_stt expectations.backend/app/tests/services/llm/providers/test_gai.py (1)
91-247: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd type hints to all updated test method parameters.
As per coding guidelines, all Python function parameters and return values must have type hints. The test methods that now accept
audio_refshould annotate it withAudioReftype.♻️ Example for one test method
def test_stt_success_with_auto_language( - self, provider, mock_client, stt_config, query_params, audio_ref + self, provider: GoogleAIProvider, mock_client: MagicMock, + stt_config: NativeCompletionConfig, query_params: QueryParams, + audio_ref: AudioRef - ): + ) -> None:Apply this pattern to all test methods that were updated to include the
audio_refparameter.As per coding guidelines: "Always add type hints to all function parameters and return values in Python code."
🤖 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/providers/test_gai.py` around lines 91 - 247, Several test functions (test_stt_success_with_auto_language, test_stt_with_specific_input_language, test_stt_with_translation, test_stt_with_custom_instructions, test_stt_with_include_provider_raw_response, test_stt_with_type_error, test_stt_with_generic_exception, test_stt_with_invalid_input_type, test_stt_with_valid_audio_ref) now take audio_ref and must include type hints; update each function signature to annotate audio_ref as AudioRef (and other params if missing) and add a return annotation (-> None) per project guidelines, and ensure AudioRef is imported or available in the test module so the names resolve.
🧹 Nitpick comments (12)
backend/app/utils.py (1)
695-700: ⚡ Quick winType
query_inputexplicitly onresolve_input().The return side of this contract is now fully annotated, but the entry parameter is still untyped. Please annotate
query_inputwith the supported input union (or a shared alias) so the resolver/provider boundary stays typed in both directions.As per coding guidelines "Always add type hints to all function parameters and return values in Python code" and "Use Python 3.11+ with type hints throughout the codebase".
🤖 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/utils.py` around lines 695 - 700, The function resolve_input currently lacks a type for its query_input parameter; update its signature to explicitly type query_input with the same supported union used in the return annotation (e.g. query_input: "str | AudioRef | list[ImageContent] | list[PDFContent] | MultiModalInput | None") so the resolver/provider boundary is fully typed; ensure any referenced names (AudioRef, ImageContent, PDFContent, MultiModalInput) are imported or available (or use forward-references/annotations) and keep the existing return annotation on resolve_input unchanged.backend/app/services/llm/jobs.py (1)
303-314: ⚡ Quick winAnnotate the yielded type of
resolved_input_context().This context manager now carries the widened
AudioRef/multimodal contract, but it still has no return annotation. Please make the yield type explicit here asIterator[...](ideally reusing a sharedResolvedInputalias) so this boundary stays typed end-to-end.As per coding guidelines "Always add type hints to all function parameters and return values in Python code" and "Use Python 3.11+ with type hints throughout the codebase".
🤖 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/services/llm/jobs.py` around lines 303 - 314, The context manager resolved_input_context currently lacks a return type; update its signature to explicitly annotate the yielded type, e.g. def resolved_input_context(...) -> Iterator[ResolvedInput]:, and ensure you import or define the ResolvedInput alias and Iterator (from typing) in this module; if ResolvedInput is not already available, import it from the shared types or add a local alias that unions the allowed resolved types (e.g. AudioRef | TextInput | ImageInput | PDFInput | list) so the context manager is typed end-to-end.backend/app/services/llm/providers/eai.py (1)
276-291: ⚡ Quick winBroaden
execute()input typing for STT calls.Line 280 still types
resolved_inputasstr, but_execute_stt()now explicitly requiresAudioRef. The override should reflect both supported shapes so the type contract matches actual usage.Suggested change
def execute( self, completion_config: NativeCompletionConfig, query: QueryParams, # noqa: ARG002 - Required by base class interface, unused for STT/TTS - resolved_input: str, + resolved_input: AudioRef | str, include_provider_raw_response: bool = False, ) -> tuple[LLMCallResponse | None, str | None]:As per coding guidelines,
**/*.py:Always add type hints to all function parameters and return values in Python codeandUse Python 3.11+ with type hints throughout the codebase.🤖 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/services/llm/providers/eai.py` around lines 276 - 291, The execute() signature types resolved_input too narrowly as str but _execute_stt() expects AudioRef; update the execute method parameter typing to accept both shapes (e.g., resolved_input: str | AudioRef or Union[str, AudioRef]) and add/import the AudioRef symbol so the type contract matches actual STT usage; keep the existing return type and ensure any noqa/comment is adjusted if needed.backend/app/services/llm/providers/sai.py (1)
252-267: ⚡ Quick winUpdate
execute()to acceptAudioRefas well.Line 256 still narrows
resolved_inputtostr, even though_execute_stt()now requiresAudioRef. Expanding the annotation here keeps the override consistent with the migrated STT flow.Suggested change
def execute( self, completion_config: NativeCompletionConfig, query: QueryParams, # noqa: ARG002 - Required by base class interface, unused for STT/TTS - resolved_input: str, + resolved_input: AudioRef | str, include_provider_raw_response: bool = False, ) -> tuple[LLMCallResponse | None, str | None]:As per coding guidelines,
**/*.py:Always add type hints to all function parameters and return values in Python codeandUse Python 3.11+ with type hints throughout the codebase.🤖 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/services/llm/providers/sai.py` around lines 252 - 267, The execute() signature narrows resolved_input to str but _execute_stt() now requires an AudioRef; update the parameter type on execute to accept both (e.g., resolved_input: str | AudioRef) and add the corresponding AudioRef import where other types (NativeCompletionConfig, QueryParams) are imported so the override remains consistent with the migrated STT flow; ensure any type checks or branches that assume a string are adjusted to handle AudioRef before calling _execute_stt.backend/app/tests/services/llm/test_input_resolver.py (1)
45-57: ⚡ Quick winAdd return annotations to the newly added test methods.
The new tests are otherwise fine, but they were added without type hints. Please annotate them with
-> None(and any parameters where applicable) to match the repo's Python typing rule. As per coding guidelines,**/*.py: Always add type hints to all function parameters and return values in Python code.Also applies to: 88-112
🤖 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_input_resolver.py` around lines 45 - 57, The test functions (e.g., test_audio_base64_input_returns_audio_ref and the other test methods around lines 88-112) are missing return type annotations; update each test function signature to include a return annotation of -> None (and annotate any parameters if any exist) so they comply with the repo typing rule; locate the unittest/pytest functions in backend/app/tests/services/llm/test_input_resolver.py (function names like test_audio_base64_input_returns_audio_ref) and simply append the return type hint without changing behavior.backend/app/tests/services/llm/providers/test_sai.py (1)
84-88: ⚡ Quick winAdd type hints to the added fixture and test.
These new callables are still missing parameter/return annotations. Please type them so this file stays consistent with the repo's Python 3.11 typing standard. As per coding guidelines,
**/*.py: Always add type hints to all function parameters and return values in Python code.Also applies to: 178-188
🤖 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/providers/test_sai.py` around lines 84 - 88, The new test fixture temp_audio_file (and the other callables around the referenced region) lack Python 3.11 type annotations; update temp_audio_file to declare its parameter and return types (e.g., def temp_audio_file(self) -> AudioRef after importing AudioRef from app.core.audio_utils) and add appropriate parameter/return annotations to the other test functions/methods in the 178-188 area (use typing.Any, None, or more specific types as appropriate) so every function signature in this file has explicit parameter and return type hints per the repo standard.backend/app/tests/services/llm/providers/test_eai.py (2)
73-77: ⚡ Quick winPromote this
AudioRefsetup into a shared factory fixture.This same inline STT fixture now exists in multiple provider suites, so future changes to the audio bytes or MIME type will drift quickly. A shared factory under
backend/app/tests/would keep these inputs consistent across providers. As per coding guidelines,backend/app/tests/**/*.py: Use factory pattern for test fixtures inbackend/app/tests/.🤖 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/providers/test_eai.py` around lines 73 - 77, Extract the inline AudioRef creation into a shared test factory (e.g., audio_ref_factory or fixture) and replace temp_audio_file with a call to that factory; specifically, create a reusable factory function/pytest fixture that returns AudioRef(bytes_=b"fake audio data", mime_type="audio/wav") and update the temp_audio_file helper (and other provider tests) to use that factory instead of constructing AudioRef inline so changes to the audio bytes or MIME type are centralized and consistent across tests.
73-77: ⚡ Quick winAdd type hints to the new fixture and test.
These newly added callables are still untyped. Please annotate parameters and return values so the tests stay aligned with the repo's Python 3.11 typing requirement. As per coding guidelines,
**/*.py: Always add type hints to all function parameters and return values in Python code.Also applies to: 159-169
🤖 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/providers/test_eai.py` around lines 73 - 77, The fixture temp_audio_file should be fully typed: add a return annotation of -> AudioRef and, if you import AudioRef locally, keep that import; change "def temp_audio_file(self):" to a no-arg fixture signature (or include any pytest fixture param types) with "-> AudioRef". Also update the tests referenced around lines 159-169 to annotate all function parameters (e.g., temp_audio_file: AudioRef, other fixtures with their concrete types) and add explicit return types of -> None for test functions so every function in the file has parameter and return type hints.backend/app/tests/test_utils.py (1)
262-272: ⚡ Quick winType the modified test signature.
mock_downloadand the return value are unannotated on this updated test. Please add the usual mock parameter annotation and-> Noneto keep the file aligned with the repo's typing standard. As per coding guidelines,**/*.py: Always add type hints to all function parameters and return values in Python code.🤖 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/test_utils.py` around lines 262 - 272, Update the test signature for test_returns_audio_ref to include a type annotation for the mock parameter and an explicit return type: change the function definition to annotate mock_download (e.g. mock_download: unittest.mock.Mock or unittest.mock.MagicMock) and keep the -> None return hint; modify the def for test_returns_audio_ref accordingly so the parameter and return value follow the repo typing standard while leaving the test body (AudioRef, resolve_audio_url assertions) unchanged.backend/app/services/llm/providers/gai_vertex.py (1)
73-80: ⚡ Quick winAdd
-> Noneto the new constructors.Both
VertexClient.__init__andGoogleVertexAIProvider.__init__are missing explicit return annotations. As per coding guidelines, "Always add type hints to all function parameters and return values in Python code".Also applies to: 103-105
🤖 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/services/llm/providers/gai_vertex.py` around lines 73 - 80, The constructors VertexClient.__init__ and GoogleVertexAIProvider.__init__ lack explicit return type annotations; update both __init__ signatures to include the return type "-> None" (i.e., def __init__(... ) -> None:) and ensure any other new constructor definitions in the same file follow this pattern so all function parameters and return values have type hints per the project's typing guidelines.backend/app/services/llm/providers/claude.py (1)
33-33: ⚡ Quick winAdd the constructor return annotation.
ClaudeProvider.__init__is missing-> None, which breaks the repo-wide Python typing rule for new code. As per coding guidelines, "Always add type hints to all function parameters and return values in Python code".🤖 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/services/llm/providers/claude.py` at line 33, The __init__ method on ClaudeProvider is missing a return type annotation; update the signature of ClaudeProvider.__init__(self, client: Anthropic) to include -> None so it reads as a constructor with an explicit None return type, keeping the parameter typing intact and adhering to the repo typing rule for functions and methods.backend/app/alembic/versions/064_add_anthropic_google_vertex_to_provider_enum.py (1)
18-18: ⚡ Quick winAdd return types to the Alembic entrypoints.
The new
upgradeanddowngradefunctions are missing-> None.Suggested fix
-def upgrade(): +def upgrade() -> None: @@ -def downgrade(): +def downgrade() -> None:As per coding guidelines, "Always add type hints to all function parameters and return values in Python code".
Also applies to: 106-106
🤖 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/alembic/versions/064_add_anthropic_google_vertex_to_provider_enum.py` at line 18, The Alembic migration entrypoints are missing explicit return type annotations; update the upgrade and downgrade function definitions (functions named upgrade and downgrade in this migration module) to include the return type -> None on their signatures so both functions are annotated as returning None.
🤖 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/alembic/versions/064_add_anthropic_google_vertex_to_provider_enum.py`:
- Around line 30-31: The migration seeds placeholder numeric values into the
model_config.pricing column which will corrupt cost estimates used by
estimate_model_cost in backend/app/crud/model_config.py; modify the Alembic
migration (064_add_anthropic_google_vertex_to_provider_enum) to insert NULL (not
fabricated numbers) for the pricing field for Anthropic/Google/Vertex entries so
pricing is absent until verified, and leave a comment TODO to add a follow-up
migration that fills in real rates once confirmed.
- Around line 106-112: The downgrade currently deletes all rows with provider IN
('anthropic', 'google-vertex'), which removes entries added after this
migration; change the DELETE in downgrade (the op.execute block) to target only
the exact (provider, model_name) pairs that this migration inserted by using a
WHERE (provider, model_name) IN ( ... ) clause listing the same tuples seeded in
the upgrade INSERT (the same values used in the migration's INSERT INTO
global.model_config ... ON CONFLICT DO NOTHING). Update the SQL in the downgrade
op.execute to include those specific tuples (matching provider and model_name
strings) so rollback only removes the rows this migration added.
In `@backend/app/core/audio_utils.py`:
- Around line 16-27: The _MIME_TO_EXT mapping is missing WAV aliases so
AudioRef.to_path() can pick the proper .wav suffix; add "audio/wave" and
"audio/x-wav" entries mapping to ".wav" in the _MIME_TO_EXT dict (the same
canonical value used for "audio/wav") so it matches
backend/app/utils.py:get_file_extension() behavior and prevents temp files from
being created with a generic ".audio" extension.
In `@backend/app/core/cloud/storage.py`:
- Around line 387-397: The logs in upload_audio_to_gcs currently emit raw
bucket, key and uri values; update the logger.error and logger.info calls to
mask those identifiers using the existing _mask() helper (e.g., use
_mask(bucket_name), _mask(key)) and construct a masked uri like
f"gs://{_mask(bucket_name)}/{_mask(key)}" before logging, preserving
exc_info=True on errors and keeping the existing "[upload_audio_to_gcs]" prefix
in each message.
In `@backend/app/core/providers.py`:
- Around line 141-150: The current masking in mask_credential_fields collapses
non-string values (e.g., the google-vertex sa_key dict) into a scalar
"********", changing JSON shape; update mask_credential_fields so that when
masked[field_name] is a dict (or list) you recursively preserve the container
shape and mask its string leaves (reusing mask_string) instead of replacing the
whole object with "********" — keep the dict/list structure for fields like
sa_key while replacing only string values, so reads preserve shape and writes
can validate properly.
- Around line 51-60: Provider.GOOGLE_VERTEX currently lists "gcs_bucket" as a
required_field in the ProviderConfig which blocks valid TTS-only credential
rows; remove "gcs_bucket" from the global required_fields on
Provider.GOOGLE_VERTEX and instead perform a conditional check for the bucket
only in the Google Vertex STT/GCS staging code path in
backend/app/services/llm/providers/gai_vertex.py (the STT-related function(s)
that perform GCS staging/transcription), raising a clear validation error there
if GCS is needed but missing.
In `@backend/app/services/llm/mappers.py`:
- Around line 574-591: The google-vertex branch is accidentally forwarding
Gemini-defaulted model IDs (materialized by
KaapiCompletionConfig.validate_params) into Vertex; before calling
map_kaapi_to_google_params in the provider=="google-vertex" block, copy
kaapi_config.params and remove any 'model' key (or otherwise ensure 'model' is
unset) so Vertex-specific defaults can apply, then pass that sanitized params
dict to map_kaapi_to_google_params and return the
NativeCompletionConfig(provider="google-vertex-native", ...) as before;
alternatively add a flag to map_kaapi_to_google_params to skip forwarding model,
but ensure kaapi_config.params' model is not forwarded for google-vertex.
In `@backend/app/services/llm/providers/claude.py`:
- Around line 43-46: The create_client function currently only checks for the
presence of the "api_key" key and will allow blank values; update create_client
to validate credentials.get("api_key") for truthiness and raise ValueError
(e.g., "Anthropic API key not configured for this project.") if it's missing or
empty before constructing Anthropic(api_key=...), so blank strings are rejected
early and avoid later runtime failures.
In `@backend/app/services/llm/providers/gai_vertex.py`:
- Around line 193-214: The code uploads audio via upload_audio_to_gcs to gs_uri
but never deletes the staged object; modify _execute_stt to wrap the Vertex STT
call in a try/finally (or context manager) so that after the STT request
finishes or raises you always attempt to remove the GCS object referenced by
gs_uri (use whichever existing helper you have to delete objects or add a small
delete_gcs_object(gs_uri, bucket, sa_info, project_id) call); make sure deletion
errors are logged (using logger.error) but do not mask the original STT error or
change the STT return path.
In `@backend/app/tests/services/llm/providers/test_claude.py`:
- Around line 44-64: Add explicit type annotations to the pytest fixtures:
annotate the mock_client fixture to return MagicMock (or a more specific client
type), annotate provider to return ClaudeProvider, annotate text_config to
return NativeCompletionConfig, and annotate query_params to return QueryParams;
ensure any fixture parameters are typed (e.g., provider(mock_client: MagicMock))
and that return types use the exact classes from the diff (MagicMock,
ClaudeProvider, NativeCompletionConfig, QueryParams) to satisfy the project's
typing guidelines.
- Around line 66-242: Add explicit type hints to all test functions in this file
(e.g., test_create_client_requires_api_key, test_create_client_with_api_key,
test_execute_success_text_input, test_execute_does_not_override_user_max_tokens,
test_execute_instructions_renamed_to_system,
test_execute_strips_instructions_when_system_also_set,
test_execute_multimodal_text_image_pdf, test_execute_strips_conversation_param,
test_execute_joins_only_text_blocks,
test_execute_includes_raw_response_when_requested,
test_execute_returns_error_on_anthropic_api_error,
test_execute_returns_error_on_unexpected_kwarg): annotate each parameter with a
concrete type where available or typing.Any for fixture values (import Any from
typing) and set the return annotation to -> None for every test function; keep
names and bodies unchanged.
- Line 19: Update the test to stop importing the nonexistent DEFAULT_MAX_TOKENS
and instead use the actual exported constant DEFAULT_ANTHROPIC_MAX_TOKENS from
the Claude provider (or add an alias EXPORT DEFAULT_MAX_TOKENS in the provider
if you prefer that API); also add proper type hints to the test helper
mock_claude_message (give it a return type) and annotate all test fixtures and
function parameters/returns in this test file to comply with the project
type-hint guidelines, referencing the symbols ClaudeProvider,
DEFAULT_ANTHROPIC_MAX_TOKENS, and mock_claude_message when making the changes.
In `@backend/app/tests/services/llm/providers/test_eai.py`:
- Around line 159-169: The test should also assert that the SDK/mock client is
never invoked when STT receives an invalid non-AudioRef input: update
test_stt_rejects_non_audioref_input to check that mock_client (the injected mock
in the test) has zero calls after calling provider.execute(stt_config,
query_params, "/nonexistent/path/audio.wav"); this enforces the early-return
behavior implemented in execute in backend/app/services/llm/providers/eai.py
(the block handling STT inputs) so the provider returns the "AudioRef input"
error without calling the underlying SDK.
In `@backend/app/tests/services/llm/providers/test_gai_vertex.py`:
- Around line 70-76: Add type hints to the autouse fixture _mock_gcs: annotate
the monkeypatch parameter as pytest.MonkeyPatch and the function return type as
None (e.g., def _mock_gcs(monkeypatch: pytest.MonkeyPatch) -> None:). Ensure
pytest is imported for the MonkeyPatch type if not already.
- Around line 119-309: The test methods (e.g.,
test_create_client_requires_all_fields, test_stt_happy_path,
test_tts_happy_path_wav, test_text_completion_is_rejected, etc.) are missing
type hints; update each test method signature to annotate all parameters (such
as provider: GoogleVertexAIProvider, stt_config: NativeCompletionConfig, query:
str, audio_ref: AudioRef, monkeypatch: pytest.MonkeyPatch, etc. as appropriate)
and add an explicit return type of -> None; ensure any imported typing names or
fixtures used are referenced correctly or imported at top of the test module so
the annotated signatures are valid.
- Around line 80-116: Update the pytest fixture signatures to include type
hints: annotate client() as -> VertexClient, provider(client: VertexClient) as
-> GoogleVertexAIProvider, query() as -> QueryParams, audio_ref() as ->
AudioRef, stt_config() as -> NativeCompletionConfig, and tts_config() as ->
NativeCompletionConfig so all fixture return types and the provider parameter
are explicitly typed (referencing the fixtures named client, provider, query,
audio_ref, stt_config, and tts_config).
In `@backend/app/tests/services/llm/providers/test_gai.py`:
- Around line 80-84: The audio_ref pytest fixture is missing type hints: add a
return type of AudioRef and type-annotate the fixture parameter (e.g., self:
Any) so the signature becomes def audio_ref(self: Any) -> AudioRef:; ensure you
import Any from typing if not already and keep the AudioRef import as-is to
satisfy the return annotation.
In `@backend/app/tests/services/llm/providers/test_registry.py`:
- Line 108: The test function test_google_vertex_falls_back_to_platform_settings
is missing type hints for the tmp_path parameter and its return type; update the
signature to annotate tmp_path as pathlib.Path (or Path if Path is imported from
pathlib) and add an explicit return type of None, keeping the existing db:
Session annotation intact and adding any necessary import for Path from pathlib
if not already present.
In `@backend/app/utils.py`:
- Around line 663-668: resolve_audio_url currently calls download_audio_bytes
which only validates the original URL but then does a default requests.get that
allows redirects and uses trust_env, enabling SSRF via redirect or proxy; update
the download flow so no redirects or env proxies can change the origin: either
make download_audio_bytes perform the HTTP GET with allow_redirects=False and
requests.Session(trust_env=False) or, after the request, verify response.url (or
response.history) has the same origin as the validated input before returning
bytes; specifically change download_audio_bytes (and any code path used by
resolve_audio_url) to disable redirects/trust_env or to assert final_url.origin
== validated_origin and return an error if not.
---
Outside diff comments:
In `@backend/app/services/llm/providers/gai.py`:
- Around line 161-163: The current logger.info call logs the full
caller-controlled merged_instruction; replace it with a metadata-only log
prefixed by the function name in square brackets (do not emit the raw prompt).
Update the logger.info that references
merged_instruction/output_language/input_language to log only: the function name
in brackets, the languages, and a masked version of merged_instruction using
mask_string(merged_instruction) (e.g., "[function_name] merged_instruction=%s
output_language=%s input_language=%s" with mask_string applied to
merged_instruction). Ensure no raw merged_instruction is ever passed to the
logger and keep output_language/input_language as plain metadata.
- Around line 468-480: The override GoogleAIProvider.execute currently types
resolved_input as str | list[ContentPart] | MultiModalInput but forwards it to
_execute_stt which requires an AudioRef; update the execute signature to include
AudioRef (matching LLMProviderBase.execute) so the "stt" branch passes a
correctly typed value, and add/adjust any local type checks/casts as needed;
also inspect and apply the same fix to the execute overrides in eai.py and
sai.py to ensure their resolved_input signatures include AudioRef and align with
their _execute_stt expectations.
In `@backend/app/tests/services/llm/providers/test_gai.py`:
- Around line 91-247: Several test functions
(test_stt_success_with_auto_language, test_stt_with_specific_input_language,
test_stt_with_translation, test_stt_with_custom_instructions,
test_stt_with_include_provider_raw_response, test_stt_with_type_error,
test_stt_with_generic_exception, test_stt_with_invalid_input_type,
test_stt_with_valid_audio_ref) now take audio_ref and must include type hints;
update each function signature to annotate audio_ref as AudioRef (and other
params if missing) and add a return annotation (-> None) per project guidelines,
and ensure AudioRef is imported or available in the test module so the names
resolve.
In `@backend/app/utils.py`:
- Around line 716-720: The URL-audio branch currently forces mime_type =
query_input.content.mime_type or "audio/wav", which overrides the real
Content-Type detected by download_audio_bytes and causes AudioRef.mime_type and
temp-file suffix to be wrong; change the logic so resolve_audio_url (and/or
download_audio_bytes it uses) is allowed to detect and return the true MIME when
query_input.content.mime_type is missing—i.e., pass None or leave mime unset
when content.mime_type is falsy, update resolve_audio_url to return the detected
mime (or an AudioRef with correct mime) and ensure AudioRef.mime_type is set
from that detection rather than the default "audio/wav"; keep
resolve_audio_base64 behavior unchanged for base64 inputs.
---
Nitpick comments:
In
`@backend/app/alembic/versions/064_add_anthropic_google_vertex_to_provider_enum.py`:
- Line 18: The Alembic migration entrypoints are missing explicit return type
annotations; update the upgrade and downgrade function definitions (functions
named upgrade and downgrade in this migration module) to include the return type
-> None on their signatures so both functions are annotated as returning None.
In `@backend/app/services/llm/jobs.py`:
- Around line 303-314: The context manager resolved_input_context currently
lacks a return type; update its signature to explicitly annotate the yielded
type, e.g. def resolved_input_context(...) -> Iterator[ResolvedInput]:, and
ensure you import or define the ResolvedInput alias and Iterator (from typing)
in this module; if ResolvedInput is not already available, import it from the
shared types or add a local alias that unions the allowed resolved types (e.g.
AudioRef | TextInput | ImageInput | PDFInput | list) so the context manager is
typed end-to-end.
In `@backend/app/services/llm/providers/claude.py`:
- Line 33: The __init__ method on ClaudeProvider is missing a return type
annotation; update the signature of ClaudeProvider.__init__(self, client:
Anthropic) to include -> None so it reads as a constructor with an explicit None
return type, keeping the parameter typing intact and adhering to the repo typing
rule for functions and methods.
In `@backend/app/services/llm/providers/eai.py`:
- Around line 276-291: The execute() signature types resolved_input too narrowly
as str but _execute_stt() expects AudioRef; update the execute method parameter
typing to accept both shapes (e.g., resolved_input: str | AudioRef or Union[str,
AudioRef]) and add/import the AudioRef symbol so the type contract matches
actual STT usage; keep the existing return type and ensure any noqa/comment is
adjusted if needed.
In `@backend/app/services/llm/providers/gai_vertex.py`:
- Around line 73-80: The constructors VertexClient.__init__ and
GoogleVertexAIProvider.__init__ lack explicit return type annotations; update
both __init__ signatures to include the return type "-> None" (i.e., def
__init__(... ) -> None:) and ensure any other new constructor definitions in the
same file follow this pattern so all function parameters and return values have
type hints per the project's typing guidelines.
In `@backend/app/services/llm/providers/sai.py`:
- Around line 252-267: The execute() signature narrows resolved_input to str but
_execute_stt() now requires an AudioRef; update the parameter type on execute to
accept both (e.g., resolved_input: str | AudioRef) and add the corresponding
AudioRef import where other types (NativeCompletionConfig, QueryParams) are
imported so the override remains consistent with the migrated STT flow; ensure
any type checks or branches that assume a string are adjusted to handle AudioRef
before calling _execute_stt.
In `@backend/app/tests/services/llm/providers/test_eai.py`:
- Around line 73-77: Extract the inline AudioRef creation into a shared test
factory (e.g., audio_ref_factory or fixture) and replace temp_audio_file with a
call to that factory; specifically, create a reusable factory function/pytest
fixture that returns AudioRef(bytes_=b"fake audio data", mime_type="audio/wav")
and update the temp_audio_file helper (and other provider tests) to use that
factory instead of constructing AudioRef inline so changes to the audio bytes or
MIME type are centralized and consistent across tests.
- Around line 73-77: The fixture temp_audio_file should be fully typed: add a
return annotation of -> AudioRef and, if you import AudioRef locally, keep that
import; change "def temp_audio_file(self):" to a no-arg fixture signature (or
include any pytest fixture param types) with "-> AudioRef". Also update the
tests referenced around lines 159-169 to annotate all function parameters (e.g.,
temp_audio_file: AudioRef, other fixtures with their concrete types) and add
explicit return types of -> None for test functions so every function in the
file has parameter and return type hints.
In `@backend/app/tests/services/llm/providers/test_sai.py`:
- Around line 84-88: The new test fixture temp_audio_file (and the other
callables around the referenced region) lack Python 3.11 type annotations;
update temp_audio_file to declare its parameter and return types (e.g., def
temp_audio_file(self) -> AudioRef after importing AudioRef from
app.core.audio_utils) and add appropriate parameter/return annotations to the
other test functions/methods in the 178-188 area (use typing.Any, None, or more
specific types as appropriate) so every function signature in this file has
explicit parameter and return type hints per the repo standard.
In `@backend/app/tests/services/llm/test_input_resolver.py`:
- Around line 45-57: The test functions (e.g.,
test_audio_base64_input_returns_audio_ref and the other test methods around
lines 88-112) are missing return type annotations; update each test function
signature to include a return annotation of -> None (and annotate any parameters
if any exist) so they comply with the repo typing rule; locate the
unittest/pytest functions in
backend/app/tests/services/llm/test_input_resolver.py (function names like
test_audio_base64_input_returns_audio_ref) and simply append the return type
hint without changing behavior.
In `@backend/app/tests/test_utils.py`:
- Around line 262-272: Update the test signature for test_returns_audio_ref to
include a type annotation for the mock parameter and an explicit return type:
change the function definition to annotate mock_download (e.g. mock_download:
unittest.mock.Mock or unittest.mock.MagicMock) and keep the -> None return hint;
modify the def for test_returns_audio_ref accordingly so the parameter and
return value follow the repo typing standard while leaving the test body
(AudioRef, resolve_audio_url assertions) unchanged.
In `@backend/app/utils.py`:
- Around line 695-700: The function resolve_input currently lacks a type for its
query_input parameter; update its signature to explicitly type query_input with
the same supported union used in the return annotation (e.g. query_input: "str |
AudioRef | list[ImageContent] | list[PDFContent] | MultiModalInput | None") so
the resolver/provider boundary is fully typed; ensure any referenced names
(AudioRef, ImageContent, PDFContent, MultiModalInput) are imported or available
(or use forward-references/annotations) and keep the existing return annotation
on resolve_input unchanged.
🪄 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
Run ID: d6a5590c-be4f-4daf-91e0-1e3b14d28bfc
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
backend/app/alembic/versions/064_add_anthropic_google_vertex_to_provider_enum.pybackend/app/core/audio_utils.pybackend/app/core/cloud/__init__.pybackend/app/core/cloud/storage.pybackend/app/core/config.pybackend/app/core/providers.pybackend/app/crud/model_config.pybackend/app/models/llm/constants.pybackend/app/models/llm/request.pybackend/app/models/model_config.pybackend/app/services/llm/jobs.pybackend/app/services/llm/mappers.pybackend/app/services/llm/providers/__init__.pybackend/app/services/llm/providers/base.pybackend/app/services/llm/providers/claude.pybackend/app/services/llm/providers/eai.pybackend/app/services/llm/providers/gai.pybackend/app/services/llm/providers/gai_vertex.pybackend/app/services/llm/providers/oai.pybackend/app/services/llm/providers/registry.pybackend/app/services/llm/providers/sai.pybackend/app/tests/crud/test_credentials.pybackend/app/tests/services/llm/providers/test_claude.pybackend/app/tests/services/llm/providers/test_eai.pybackend/app/tests/services/llm/providers/test_gai.pybackend/app/tests/services/llm/providers/test_gai_vertex.pybackend/app/tests/services/llm/providers/test_registry.pybackend/app/tests/services/llm/providers/test_sai.pybackend/app/tests/services/llm/test_input_resolver.pybackend/app/tests/test_utils.pybackend/app/utils.pybackend/pyproject.toml
Comment on lines
+106
to
+112
| def downgrade(): | ||
| op.execute( | ||
| """ | ||
| DELETE FROM global.model_config | ||
| WHERE provider IN ('anthropic', 'google-vertex') | ||
| """ | ||
| ) |
There was a problem hiding this comment.
Scope the downgrade delete to the rows this migration inserted.
DELETE ... WHERE provider IN ('anthropic', 'google-vertex') will also remove model configs created after this migration for those providers. Because the upgrade is idempotent (ON CONFLICT ... DO NOTHING), rollback should target the exact (provider, model_name) pairs seeded here, not every Anthropic/Vertex row.
🤖 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/alembic/versions/064_add_anthropic_google_vertex_to_provider_enum.py`
around lines 106 - 112, The downgrade currently deletes all rows with provider
IN ('anthropic', 'google-vertex'), which removes entries added after this
migration; change the DELETE in downgrade (the op.execute block) to target only
the exact (provider, model_name) pairs that this migration inserted by using a
WHERE (provider, model_name) IN ( ... ) clause listing the same tuples seeded in
the upgrade INSERT (the same values used in the migration's INSERT INTO
global.model_config ... ON CONFLICT DO NOTHING). Update the SQL in the downgrade
op.execute to include those specific tuples (matching provider and model_name
strings) so rollback only removes the rows this migration added.
Comment on lines
+16
to
+27
| _MIME_TO_EXT = { | ||
| "audio/wav": ".wav", | ||
| "audio/mpeg": ".mp3", | ||
| "audio/mp3": ".mp3", | ||
| "audio/ogg": ".ogg", | ||
| "audio/flac": ".flac", | ||
| "audio/webm": ".webm", | ||
| "audio/mp4": ".mp4", | ||
| "audio/m4a": ".m4a", | ||
| "audio/aac": ".aac", | ||
| "audio/aiff": ".aiff", | ||
| } |
There was a problem hiding this comment.
Add the missing WAV MIME aliases here.
AudioRef.to_path() now relies on this table to pick the temp-file suffix, but audio/wave and audio/x-wav are missing even though backend/app/utils.py:get_file_extension() already treats them as WAV. Valid WAV uploads using those MIME types will therefore materialize as *.audio, which can break SDKs that infer the format from the filename extension.
Suggested fix
_MIME_TO_EXT = {
"audio/wav": ".wav",
+ "audio/wave": ".wav",
+ "audio/x-wav": ".wav",
"audio/mpeg": ".mp3",
"audio/mp3": ".mp3",
"audio/ogg": ".ogg",🤖 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/audio_utils.py` around lines 16 - 27, The _MIME_TO_EXT
mapping is missing WAV aliases so AudioRef.to_path() can pick the proper .wav
suffix; add "audio/wave" and "audio/x-wav" entries mapping to ".wav" in the
_MIME_TO_EXT dict (the same canonical value used for "audio/wav") so it matches
backend/app/utils.py:get_file_extension() behavior and prevents temp files from
being created with a generic ".audio" extension.
Comment on lines
+387
to
+397
| logger.error( | ||
| f"[upload_audio_to_gcs] Upload failed | " | ||
| f"bucket={bucket_name}, key={key}, error={e}", | ||
| exc_info=True, | ||
| ) | ||
| raise CloudStorageError(f"GCS upload failed: {e}") from e | ||
|
|
||
| uri = f"gs://{bucket_name}/{key}" | ||
| logger.info( | ||
| f"[upload_audio_to_gcs] Uploaded | " | ||
| f"uri={uri}, mime={mime}, size_kb={size / 1024:.1f}" |
There was a problem hiding this comment.
Mask the staged GCS identifiers in these logs.
Lines 387-397 log raw bucket, key, and uri values for uploaded audio, which leaks storage metadata for user media into both error and info logs. This file already introduced _mask() for the S3 paths, so the GCS helper should apply the same masking before logging. As per coding guidelines, "Prefix all log messages with the function name in square brackets: logger.info(f"[function_name] Message {mask_string(sensitive_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/core/cloud/storage.py` around lines 387 - 397, The logs in
upload_audio_to_gcs currently emit raw bucket, key and uri values; update the
logger.error and logger.info calls to mask those identifiers using the existing
_mask() helper (e.g., use _mask(bucket_name), _mask(key)) and construct a masked
uri like f"gs://{_mask(bucket_name)}/{_mask(key)}" before logging, preserving
exc_info=True on errors and keeping the existing "[upload_audio_to_gcs]" prefix
in each message.
Comment on lines
+51
to
+60
| Provider.GOOGLE_VERTEX: ProviderConfig( | ||
| required_fields=[ | ||
| "api_key", | ||
| "project_id", | ||
| "location", | ||
| "sa_key", | ||
| "gcs_bucket", | ||
| ], | ||
| sensitive_fields=["api_key", "sa_key"], | ||
| ), |
There was a problem hiding this comment.
Make gcs_bucket conditional for Google Vertex credentials.
backend/app/services/llm/providers/gai_vertex.py:105-138 only treats api_key, project_id, and location as required, and the provider’s TTS path does not need GCS staging. Requiring gcs_bucket here rejects valid TTS-only credential rows and makes CRUD validation stricter than runtime. Validate the bucket on the STT path instead, or make this requirement conditional.
🤖 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` around lines 51 - 60, Provider.GOOGLE_VERTEX
currently lists "gcs_bucket" as a required_field in the ProviderConfig which
blocks valid TTS-only credential rows; remove "gcs_bucket" from the global
required_fields on Provider.GOOGLE_VERTEX and instead perform a conditional
check for the bucket only in the Google Vertex STT/GCS staging code path in
backend/app/services/llm/providers/gai_vertex.py (the STT-related function(s)
that perform GCS staging/transcription), raising a clear validation error there
if GCS is needed but missing.
Comment on lines
+80
to
+116
| @pytest.fixture | ||
| def client(self) -> VertexClient: | ||
| return VertexClient( | ||
| api_key="k", | ||
| project_id="p", | ||
| location="us-central1", | ||
| sa_info={"type": "service_account", "project_id": "p"}, | ||
| gcs_bucket="test-bucket", | ||
| ) | ||
|
|
||
| @pytest.fixture | ||
| def provider(self, client) -> GoogleVertexAIProvider: | ||
| return GoogleVertexAIProvider(client=client) | ||
|
|
||
| @pytest.fixture | ||
| def query(self) -> QueryParams: | ||
| return QueryParams(input="ignored") | ||
|
|
||
| @pytest.fixture | ||
| def audio_ref(self) -> AudioRef: | ||
| return AudioRef(bytes_=b"RIFFfake", mime_type="audio/wav") | ||
|
|
||
| @pytest.fixture | ||
| def stt_config(self) -> NativeCompletionConfig: | ||
| return NativeCompletionConfig( | ||
| provider="google-vertex-native", | ||
| type="stt", | ||
| params={"model": "gemini-2.5-flash", "input_language": "auto"}, | ||
| ) | ||
|
|
||
| @pytest.fixture | ||
| def tts_config(self) -> NativeCompletionConfig: | ||
| return NativeCompletionConfig( | ||
| provider="google-vertex-native", | ||
| type="tts", | ||
| params={"model": "gemini-2.5-flash-preview-tts", "voice": "Kore"}, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hints to pytest fixtures.
As per coding guidelines, all Python function parameters and return values must have type hints. The fixtures should annotate their return types.
♻️ Example fixes
`@pytest.fixture`
- def client(self) -> VertexClient:
+ def client(self) -> VertexClient:
return VertexClient(...)
`@pytest.fixture`
- def provider(self, client) -> GoogleVertexAIProvider:
+ def provider(self, client: VertexClient) -> GoogleVertexAIProvider:
return GoogleVertexAIProvider(client=client)
`@pytest.fixture`
- def query(self) -> QueryParams:
+ def query(self) -> QueryParams:
return QueryParams(input="ignored")
`@pytest.fixture`
- def audio_ref(self) -> AudioRef:
+ def audio_ref(self) -> AudioRef:
return AudioRef(...)Apply similar patterns to stt_config and tts_config fixtures.
As per coding guidelines: "Always add type hints to all function parameters and return values in Python code."
🤖 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/providers/test_gai_vertex.py` around lines 80
- 116, Update the pytest fixture signatures to include type hints: annotate
client() as -> VertexClient, provider(client: VertexClient) as ->
GoogleVertexAIProvider, query() as -> QueryParams, audio_ref() as -> AudioRef,
stt_config() as -> NativeCompletionConfig, and tts_config() as ->
NativeCompletionConfig so all fixture return types and the provider parameter
are explicitly typed (referencing the fixtures named client, provider, query,
audio_ref, stt_config, and tts_config).
Comment on lines
+119
to
+309
| def test_create_client_requires_all_fields(self): | ||
| with pytest.raises(ValueError, match="project_id, location"): | ||
| GoogleVertexAIProvider.create_client({"api_key": "k"}) | ||
|
|
||
| def test_create_client_builds_endpoint(self): | ||
| c = GoogleVertexAIProvider.create_client( | ||
| {"api_key": "k", "project_id": "p", "location": "us-central1"} | ||
| ) | ||
| assert "us-central1-aiplatform.googleapis.com" in c.endpoint("m") | ||
| assert "projects/p/locations/us-central1" in c.endpoint("m") | ||
| assert "models/m:generateContent" in c.endpoint("m") | ||
|
|
||
| # ── STT ────────────────────────────────────────────────────────────────── | ||
| def test_stt_happy_path(self, provider, stt_config, query, audio_ref): | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| return_value=_mock_http_ok(_stt_response("hi there")), | ||
| ) as mock_post: | ||
| resp, err = provider.execute(stt_config, query, audio_ref) | ||
|
|
||
| assert err is None | ||
| assert resp.response.output.content.value == "hi there" | ||
| assert resp.response.model == "gemini-2.5-flash" | ||
| assert resp.usage.input_tokens == 10 | ||
| assert resp.usage.output_tokens == 5 | ||
|
|
||
| kwargs = mock_post.call_args.kwargs | ||
| assert kwargs["params"] == {"key": "k"} | ||
| parts = kwargs["json"]["contents"][0]["parts"] | ||
| assert parts[0]["fileData"]["mimeType"] == "audio/wav" | ||
| assert parts[0]["fileData"]["fileUri"].startswith("gs://test-bucket/") | ||
| assert "Detect the spoken language automatically" in parts[1]["text"] | ||
|
|
||
| def test_stt_rejects_non_audioref_input(self, provider, stt_config, query): | ||
| resp, err = provider.execute(stt_config, query, "/some/path.wav") | ||
| assert resp is None | ||
| assert "AudioRef input" in err | ||
|
|
||
| def test_stt_rejects_unsupported_mime(self, provider, stt_config, query): | ||
| bad = AudioRef(bytes_=b"x", mime_type="audio/xyz") | ||
| resp, err = provider.execute(stt_config, query, bad) | ||
| assert resp is None | ||
| assert "Unsupported audio mime" in err | ||
|
|
||
| def test_stt_gcs_upload_failure_returns_clean_error( | ||
| self, provider, stt_config, query, audio_ref, monkeypatch | ||
| ): | ||
| monkeypatch.setattr( | ||
| "app.services.llm.providers.gai_vertex.upload_audio_to_gcs", | ||
| MagicMock(side_effect=RuntimeError("bucket denied")), | ||
| ) | ||
| resp, err = provider.execute(stt_config, query, audio_ref) | ||
| assert resp is None | ||
| assert "Failed to stage audio for Vertex STT" in err | ||
| assert "bucket denied" in err | ||
|
|
||
| def test_stt_http_error_returns_clean_message( | ||
| self, provider, stt_config, query, audio_ref | ||
| ): | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| return_value=_mock_http_err(403, "permission denied"), | ||
| ): | ||
| resp, err = provider.execute(stt_config, query, audio_ref) | ||
| assert resp is None | ||
| assert "Vertex AI HTTP 403" in err | ||
| assert "permission denied" in err | ||
|
|
||
| def test_stt_network_error_returns_clean_message( | ||
| self, provider, stt_config, query, audio_ref | ||
| ): | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| side_effect=requests.ConnectionError("dns boom"), | ||
| ): | ||
| resp, err = provider.execute(stt_config, query, audio_ref) | ||
| assert resp is None | ||
| assert "Vertex AI request failed" in err | ||
|
|
||
| def test_stt_missing_transcript_returns_error( | ||
| self, provider, stt_config, query, audio_ref | ||
| ): | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| return_value=_mock_http_ok({"candidates": []}), | ||
| ): | ||
| resp, err = provider.execute(stt_config, query, audio_ref) | ||
| assert resp is None | ||
| assert "missing transcript text" in err | ||
|
|
||
| def test_stt_input_language_overrides_prompt(self, provider, query, audio_ref): | ||
| config = NativeCompletionConfig( | ||
| provider="google-vertex-native", | ||
| type="stt", | ||
| params={ | ||
| "model": "gemini-2.5-flash", | ||
| "input_language": "hi-IN", | ||
| "output_language": "en-IN", | ||
| "instructions": "be precise", | ||
| }, | ||
| ) | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| return_value=_mock_http_ok(_stt_response()), | ||
| ) as mock_post: | ||
| provider.execute(config, query, audio_ref) | ||
|
|
||
| prompt = mock_post.call_args.kwargs["json"]["contents"][0]["parts"][1]["text"] | ||
| assert prompt.startswith("be precise") | ||
| assert "hi-IN" in prompt | ||
| assert "translate to en-IN" in prompt | ||
|
|
||
| # ── TTS ────────────────────────────────────────────────────────────────── | ||
| def test_tts_happy_path_wav(self, provider, tts_config, query): | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| return_value=_mock_http_ok(_tts_response()), | ||
| ) as mock_post: | ||
| resp, err = provider.execute(tts_config, query, "hello") | ||
|
|
||
| assert err is None | ||
| assert resp.response.output.content.format == "base64" | ||
| assert resp.response.output.content.mime_type == "audio/wav" | ||
| decoded = base64.b64decode(resp.response.output.content.value) | ||
| assert decoded[:4] == b"RIFF" | ||
|
|
||
| sent = mock_post.call_args.kwargs["json"] | ||
| assert sent["generationConfig"]["responseModalities"] == ["AUDIO"] | ||
| assert ( | ||
| sent["generationConfig"]["speechConfig"]["voiceConfig"][ | ||
| "prebuiltVoiceConfig" | ||
| ]["voiceName"] | ||
| == "Kore" | ||
| ) | ||
|
|
||
| def test_tts_rejects_non_string_input(self, provider, tts_config, query): | ||
| resp, err = provider.execute(tts_config, query, ["not a string"]) | ||
| assert resp is None | ||
| assert "text string as input" in err | ||
|
|
||
| def test_tts_rejects_empty_input(self, provider, tts_config, query): | ||
| resp, err = provider.execute(tts_config, query, " ") | ||
| assert resp is None | ||
| assert "Text input cannot be empty" in err | ||
|
|
||
| def test_tts_missing_audio_returns_error(self, provider, tts_config, query): | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| return_value=_mock_http_ok({"candidates": [{"content": {"parts": []}}]}), | ||
| ): | ||
| resp, err = provider.execute(tts_config, query, "hello") | ||
| assert resp is None | ||
| assert "missing audio data" in err | ||
|
|
||
| def test_tts_language_is_forwarded(self, provider, query): | ||
| config = NativeCompletionConfig( | ||
| provider="google-vertex-native", | ||
| type="tts", | ||
| params={"model": "gemini-2.5-flash-preview-tts", "language": "en-US"}, | ||
| ) | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| return_value=_mock_http_ok(_tts_response()), | ||
| ) as mock_post: | ||
| provider.execute(config, query, "hi") | ||
| speech = mock_post.call_args.kwargs["json"]["generationConfig"]["speechConfig"] | ||
| assert speech["languageCode"] == "en-US" | ||
|
|
||
| # ── execute dispatcher ─────────────────────────────────────────────────── | ||
| def test_text_completion_is_rejected(self, provider, query): | ||
| config = NativeCompletionConfig( | ||
| provider="google-vertex-native", | ||
| type="text", | ||
| params={"model": "gemini-2.5-flash"}, | ||
| ) | ||
| resp, err = provider.execute(config, query, "hello") | ||
| assert resp is None | ||
| assert "does not support completion type 'text'" in err | ||
|
|
||
| def test_raw_response_included_when_requested( | ||
| self, provider, stt_config, query, audio_ref | ||
| ): | ||
| raw = _stt_response() | ||
| with patch( | ||
| "app.services.llm.providers.gai_vertex.requests.post", | ||
| return_value=_mock_http_ok(raw), | ||
| ): | ||
| resp, _ = provider.execute( | ||
| stt_config, query, audio_ref, include_provider_raw_response=True | ||
| ) | ||
| assert resp.provider_raw_response == raw |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hints to all test methods.
As per coding guidelines, all Python function parameters and return values must have type hints. Test methods should annotate their parameters and return -> None.
♻️ Example for test methods
- def test_create_client_requires_all_fields(self):
+ def test_create_client_requires_all_fields(self) -> None:
with pytest.raises(ValueError, match="project_id, location"):
- def test_stt_happy_path(self, provider, stt_config, query, audio_ref):
+ def test_stt_happy_path(
+ self,
+ provider: GoogleVertexAIProvider,
+ stt_config: NativeCompletionConfig,
+ query: QueryParams,
+ audio_ref: AudioRef
+ ) -> None:Apply this pattern to all test methods in the class.
As per coding guidelines: "Always add type hints to all function parameters and return values in Python code."
🤖 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/providers/test_gai_vertex.py` around lines 119
- 309, The test methods (e.g., test_create_client_requires_all_fields,
test_stt_happy_path, test_tts_happy_path_wav, test_text_completion_is_rejected,
etc.) are missing type hints; update each test method signature to annotate all
parameters (such as provider: GoogleVertexAIProvider, stt_config:
NativeCompletionConfig, query: str, audio_ref: AudioRef, monkeypatch:
pytest.MonkeyPatch, etc. as appropriate) and add an explicit return type of ->
None; ensure any imported typing names or fixtures used are referenced correctly
or imported at top of the test module so the annotated signatures are valid.
Comment on lines
+80
to
+84
| @pytest.fixture | ||
| def audio_ref(self): | ||
| from app.core.audio_utils import AudioRef | ||
|
|
||
| return AudioRef(bytes_=b"fake audio data", mime_type="audio/wav") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hint to the audio_ref fixture.
As per coding guidelines, all Python function parameters and return values must have type hints.
♻️ Proposed fix
`@pytest.fixture`
- def audio_ref(self):
+ def audio_ref(self) -> AudioRef:
from app.core.audio_utils import AudioRef
return AudioRef(bytes_=b"fake audio data", mime_type="audio/wav")As per coding guidelines: "Always add type hints to all function parameters and return values in Python code."
🤖 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/providers/test_gai.py` around lines 80 - 84,
The audio_ref pytest fixture is missing type hints: add a return type of
AudioRef and type-annotate the fixture parameter (e.g., self: Any) so the
signature becomes def audio_ref(self: Any) -> AudioRef:; ensure you import Any
from typing if not already and keep the AudioRef import as-is to satisfy the
return annotation.
Comment on lines
+663
to
+668
| def resolve_audio_url(url: str, mime_type: str) -> tuple["AudioRef | None", str | None]: | ||
| """Download audio from a public URL into an in-memory AudioRef.""" | ||
| audio_bytes, error = download_audio_bytes(url) | ||
| if error: | ||
| return "", error | ||
|
|
||
| ext = get_file_extension(mime_type) | ||
| try: | ||
| with tempfile.NamedTemporaryFile( | ||
| suffix=ext, delete=False, prefix="audio_" | ||
| ) as tmp: | ||
| tmp.write(audio_bytes) | ||
| temp_path = tmp.name | ||
| logger.info(f"[resolve_audio_url] Downloaded audio to temp file: {temp_path}") | ||
| return temp_path, None | ||
| except Exception as e: | ||
| return "", f"Failed to write audio to temp file: {str(e)}" | ||
| if error or not audio_bytes: | ||
| return None, error | ||
| return AudioRef(bytes_=audio_bytes, mime_type=mime_type), None |
There was a problem hiding this comment.
Keep URL audio downloads on the validated origin.
This path still goes through download_audio_bytes(), which validates only the original URL and then performs a default requests.get(...). Because redirects are enabled and trust_env is left on, a public URL can bounce the worker to a different host after validation, which weakens the SSRF guard on user-controlled audio URLs.
Suggested fix
def download_audio_bytes(url: str) -> tuple[bytes | None, str | None]:
"""Download audio from a public URL. Returns (bytes, error)."""
@@
- try:
- with requests.get(url, timeout=30, stream=True) as resp:
+ try:
+ with requests.Session() as session:
+ session.trust_env = False
+ with session.get(
+ url,
+ timeout=30,
+ stream=True,
+ allow_redirects=False,
+ ) as resp:
resp.raise_for_status()📝 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.
Suggested change
| def resolve_audio_url(url: str, mime_type: str) -> tuple["AudioRef | None", str | None]: | |
| """Download audio from a public URL into an in-memory AudioRef.""" | |
| audio_bytes, error = download_audio_bytes(url) | |
| if error: | |
| return "", error | |
| ext = get_file_extension(mime_type) | |
| try: | |
| with tempfile.NamedTemporaryFile( | |
| suffix=ext, delete=False, prefix="audio_" | |
| ) as tmp: | |
| tmp.write(audio_bytes) | |
| temp_path = tmp.name | |
| logger.info(f"[resolve_audio_url] Downloaded audio to temp file: {temp_path}") | |
| return temp_path, None | |
| except Exception as e: | |
| return "", f"Failed to write audio to temp file: {str(e)}" | |
| if error or not audio_bytes: | |
| return None, error | |
| return AudioRef(bytes_=audio_bytes, mime_type=mime_type), None | |
| def resolve_audio_url(url: str, mime_type: str) -> tuple[AudioRef | None, str | None]: | |
| """Download audio from a public URL into an in-memory AudioRef.""" | |
| audio_bytes, error = download_audio_bytes(url) | |
| if error or not audio_bytes: | |
| return None, error | |
| return AudioRef(bytes_=audio_bytes, mime_type=mime_type), None |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 663-663: Remove quotes from type annotation
Remove quotes
(UP037)
🤖 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/utils.py` around lines 663 - 668, resolve_audio_url currently
calls download_audio_bytes which only validates the original URL but then does a
default requests.get that allows redirects and uses trust_env, enabling SSRF via
redirect or proxy; update the download flow so no redirects or env proxies can
change the origin: either make download_audio_bytes perform the HTTP GET with
allow_redirects=False and requests.Session(trust_env=False) or, after the
request, verify response.url (or response.history) has the same origin as the
validated input before returning bytes; specifically change download_audio_bytes
(and any code path used by resolve_audio_url) to disable redirects/trust_env or
to assert final_url.origin == validated_origin and return an error if not.
Collaborator
Author
|
Checking breaking changes, coderabbit comments and CI failure |
This was
linked to
issues
Jun 4, 2026
Prajna1999
commented
Jun 4, 2026
|
|
||
| def _load_platform_sa_info() -> dict | None: | ||
| """Load the platform-default GCP SA JSON from disk, if configured.""" | ||
| sa_path = settings.GCP_SA_KEY_PATH |
Collaborator
Author
There was a problem hiding this comment.
use .env based loading mechanism instead of file based. Also check how ECS task def handles JSON files.
kartpop
requested changes
Jun 4, 2026
| GCP_PROJECT_ID: str = "" | ||
| # Filesystem path to the platform-default GCP service-account JSON. | ||
| # Used by the registry fallback when a project has no google-vertex row. | ||
| GCP_SA_KEY_PATH: str = "" |
Collaborator
There was a problem hiding this comment.
how will this work in ECS?
GCP_SA_KEY_PATH
|
|
||
| def _load_platform_sa_info() -> dict | None: | ||
| """Load the platform-default GCP SA JSON from disk, if configured.""" | ||
| sa_path = settings.GCP_SA_KEY_PATH |
kartpop
approved these changes
Jun 4, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/services/llm/providers/gai_vertex.py`:
- Around line 50-70: The helper _load_platform_sa_info currently only parses
inline JSON and returns None for non-JSON strings, breaking the filesystem-path
contract and leaving VertexClient.sa_info unset; update _load_platform_sa_info
to handle the path case: if settings.GCP_SA_KEY does not start with "{" treat it
as a filesystem path, attempt to read the file contents, parse JSON and return
the dict, and on any OSError/IOError or json.JSONDecodeError log a clear warning
(similar format to the existing logger.warning) and return None; ensure you
reference and preserve the existing function name _load_platform_sa_info and
that VertexClient.sa_info reads the returned dict as before.
- Around line 123-139: The create_client function is currently logging the raw
Vertex API key via logger.info(f"Vertex API Key {api_key}") which leaks
credentials; remove that raw log and instead log the presence/source safely
using the masking helper (e.g., mask_string) and follow the project convention
of prefixing with the function name in square brackets; update the logger.info
call(s) in create_client (the lines that reference api_key and the subsequent
logger.info block) to avoid printing api_key directly and to use
logger.info(f"[create_client] ... {mask_string(api_key_or_placeholder)}") or
simply omit the key while still logging source/project_id/location.
- Around line 284-287: The call in _execute_stt currently logs an error
unconditionally before checking the result of self._post, causing successful
calls (err is None) to be logged as errors; change the logic so you only call
logger.error when err is truthy (i.e., move the logger.error into the if err:
branch or gate it behind a check), and ensure the log message references the
model/payload context if available to aid debugging while leaving successful
responses unlogged.
🪄 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
Run ID: 045af3db-b2c2-4ba6-bae3-d4f34f5ff8a1
📒 Files selected for processing (3)
backend/app/core/config.pybackend/app/services/llm/providers/gai_vertex.pybackend/app/tests/services/llm/providers/test_registry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/core/config.py
Comment on lines
+50
to
+70
| def _load_platform_sa_info() -> dict | None: | ||
| """Load the platform-default GCP SA JSON. | ||
|
|
||
| Supports two configuration shapes for settings.GCP_SA_KEY: | ||
| 1. Raw JSON string (e.g. injected via env var / secret manager) | ||
| 2. Filesystem path to a JSON key file | ||
| """ | ||
| sa_value = settings.GCP_SA_KEY | ||
| if not sa_value: | ||
| return None | ||
|
|
||
| stripped = sa_value.strip() | ||
| if stripped.startswith("{"): | ||
| try: | ||
| return json.loads(stripped) | ||
| except json.JSONDecodeError as e: | ||
| logger.warning( | ||
| f"[_load_platform_sa_info] GCP_SA_KEY looks like JSON but " | ||
| f"failed to parse | error={e}" | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Resolve the GCP_SA_KEY contract mismatch.
The docstring says settings.GCP_SA_KEY supports a filesystem path, and the new registry test now passes a path, but this helper only parses inline JSON and returns None for any non-JSON string. That leaves VertexClient.sa_info unset and breaks the platform-fallback STT path. Either implement the path branch here or standardize on inline JSON and update the docstring/test to match.
Based on learnings: "use .env based loading mechanism instead of file based. Also check how ECS task def handles JSON files."
🤖 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/services/llm/providers/gai_vertex.py` around lines 50 - 70, The
helper _load_platform_sa_info currently only parses inline JSON and returns None
for non-JSON strings, breaking the filesystem-path contract and leaving
VertexClient.sa_info unset; update _load_platform_sa_info to handle the path
case: if settings.GCP_SA_KEY does not start with "{" treat it as a filesystem
path, attempt to read the file contents, parse JSON and return the dict, and on
any OSError/IOError or json.JSONDecodeError log a clear warning (similar format
to the existing logger.warning) and return None; ensure you reference and
preserve the existing function name _load_platform_sa_info and that
VertexClient.sa_info reads the returned dict as before.
Comment on lines
+123
to
+139
| def create_client(credentials: dict[str, Any]) -> Any: | ||
| # Fall back to platform-shared defaults from settings for any field | ||
| # the caller didn't provide. The SA JSON falls back to the file at | ||
| # settings.GCP_SA_KEY; BYOK rows pass `sa_key` inline. | ||
| credentials = credentials or {} | ||
| api_key = credentials.get("api_key") or settings.GCP_VERTEX_API_KEY | ||
| logger.info(f"Vertex API Key {api_key}") | ||
| project_id = credentials.get("project_id") or settings.GCP_PROJECT_ID | ||
| location = credentials.get("location") or settings.GCP_VERTEX_LOCATION | ||
| gcs_bucket = credentials.get("gcs_bucket") or settings.GCS_AUDIO_BUCKET | ||
| sa_info = credentials.get("sa_key") or _load_platform_sa_info() | ||
|
|
||
| source = "byok" if credentials.get("api_key") else "platform" | ||
| logger.info( | ||
| f"[create_client] vertex creds | source={source}, " | ||
| f"project_id={project_id}, location={location}" | ||
| ) |
There was a problem hiding this comment.
Stop logging the raw Vertex API key.
Line 129 writes the full API key to application logs. That exposes a credential and bypasses the repo’s masking convention. Remove this log or route it through the existing masking helper before merge.
🔒 Minimal fix
- logger.info(f"Vertex API Key {api_key}")
project_id = credentials.get("project_id") or settings.GCP_PROJECT_IDAs per coding guidelines: "Prefix all log messages with the function name in square brackets: logger.info(f\"[function_name] Message {mask_string(sensitive_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/services/llm/providers/gai_vertex.py` around lines 123 - 139, The
create_client function is currently logging the raw Vertex API key via
logger.info(f"Vertex API Key {api_key}") which leaks credentials; remove that
raw log and instead log the presence/source safely using the masking helper
(e.g., mask_string) and follow the project convention of prefixing with the
function name in square brackets; update the logger.info call(s) in
create_client (the lines that reference api_key and the subsequent logger.info
block) to avoid printing api_key directly and to use
logger.info(f"[create_client] ... {mask_string(api_key_or_placeholder)}") or
simply omit the key while still logging source/project_id/location.
Comment on lines
+284
to
+287
| data, err = self._post(model, payload) | ||
| logger.error(f"[_execute_stt] Error post making the call to Vertes is {err}") | ||
| if err: | ||
| return None, err |
There was a problem hiding this comment.
Don’t emit an error log on the success path.
logger.error(...) runs before the if err: guard, so every successful STT request produces an error entry with err=None. That will skew error dashboards and alerting; log only inside the failure branch.
🩹 Suggested change
data, err = self._post(model, payload)
- logger.error(f"[_execute_stt] Error post making the call to Vertes is {err}")
if err:
+ logger.error(
+ f"[GoogleVertexAIProvider._execute_stt] Vertex POST failed | err={err}"
+ )
return None, err🤖 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/services/llm/providers/gai_vertex.py` around lines 284 - 287, The
call in _execute_stt currently logs an error unconditionally before checking the
result of self._post, causing successful calls (err is None) to be logged as
errors; change the logic so you only call logger.error when err is truthy (i.e.,
move the logger.error into the if err: branch or gate it behind a check), and
ensure the log message references the model/payload context if available to aid
debugging while leaving successful responses unlogged.
vprashrex
approved these changes
Jun 4, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/app/tests/services/llm/providers/test_claude.py (1)
1-242:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: All ClaudeProvider tests are disabled—this blocks production readiness.
The entire test suite for the newly added ClaudeProvider is commented out, leaving zero test coverage for critical paths including client creation, multimodal content formatting, Files API uploads, error handling, and parameter mapping. Merging a new provider without active tests violates the PR checklist claim that "tests present/checked."
Past review comments have already identified the root causes preventing these tests from running:
- Import error (line 19):
DEFAULT_MAX_TOKENSdoesn't exist; should beDEFAULT_ANTHROPIC_MAX_TOKENS- Missing type hints: Fixtures (lines 44-64) and all test methods (lines 66-242) lack parameter and return annotations required by coding guidelines
✅ Required actions before merge
- Uncomment the entire test file
- Fix the import on line 19:
from app.services.llm.providers.claude import ClaudeProvider, DEFAULT_ANTHROPIC_MAX_TOKENS- Update line 94 to reference the correct constant:
assert call_kwargs["max_tokens"] == DEFAULT_ANTHROPIC_MAX_TOKENS- Add type hints to all fixtures and test methods per coding guidelines (see past review comments for detailed examples)
As per coding guidelines: "Always add type hints to all function parameters and return values in Python code."
🤖 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/providers/test_claude.py` around lines 1 - 242, Uncomment the entire test file and re-enable the ClaudeProvider tests; update the import to pull DEFAULT_ANTHROPIC_MAX_TOKENS from app.services.llm.providers.claude (replace DEFAULT_MAX_TOKENS), change the assertion in TestClaudeProvider.test_execute_success_text_input to compare call_kwargs["max_tokens"] to DEFAULT_ANTHROPIC_MAX_TOKENS, and add proper type hints to all pytest fixtures and test methods (e.g., mock_client, provider, text_config, query_params and each test_* method) to satisfy the project typing guidelines so the suite runs and type checks.
🤖 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.
Duplicate comments:
In `@backend/app/tests/services/llm/providers/test_claude.py`:
- Around line 1-242: Uncomment the entire test file and re-enable the
ClaudeProvider tests; update the import to pull DEFAULT_ANTHROPIC_MAX_TOKENS
from app.services.llm.providers.claude (replace DEFAULT_MAX_TOKENS), change the
assertion in TestClaudeProvider.test_execute_success_text_input to compare
call_kwargs["max_tokens"] to DEFAULT_ANTHROPIC_MAX_TOKENS, and add proper type
hints to all pytest fixtures and test methods (e.g., mock_client, provider,
text_config, query_params and each test_* method) to satisfy the project typing
guidelines so the suite runs and type checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e8902f1-e5c5-425d-963e-f92e72d2ba12
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
backend/app/core/cloud/storage.pybackend/app/tests/services/llm/providers/test_claude.pybackend/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/pyproject.toml
- backend/app/core/cloud/storage.py
Prajna1999
added
the
breaking-change-approved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/app/tests/services/llm/providers/test_claude.py (1)
98-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winType annotations are still incomplete across test methods.
Most tests still omit typed fixture parameters and explicit
-> Nonereturns (for example Line 98+ and Line 196+). Please annotate all test signatures consistently.♻️ Minimal pattern to apply
- def test_requires_api_key(self): + def test_requires_api_key(self) -> None: - def test_simple_string_input(self, provider, mock_client, config, query): + def test_simple_string_input( + self, + provider: ClaudeProvider, + mock_client: MagicMock, + config: NativeCompletionConfig, + query: QueryParams, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code" and "Use Python 3.11+ with type hints throughout the codebase".
Also applies to: 114-190, 196-443
🤖 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/providers/test_claude.py` around lines 98 - 108, Update the test function signatures in test_claude.py (e.g., test_requires_api_key and test_returns_anthropic_client) to include explicit type annotations for any pytest fixtures/parameters and add return type -> None; ensure all other test functions in the same file (and ranges called out around lines 114-190 and 196-443) follow the same pattern so every def has annotated parameters and a -> None return annotation, preserving existing names like ClaudeProvider.create_client and the patched "app.services.llm.providers.claude.Anthropic".backend/app/tests/services/llm/providers/test_gai_vertex.py (1)
72-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing parameter/return annotations across fixtures and tests.
Line 73, Line 93, and many test methods from Line 121 onward still omit required type hints (including explicit
-> Noneon tests). This violates repository typing rules and was previously flagged.♻️ Minimal pattern to apply
- def _mock_gcs(monkeypatch): + def _mock_gcs(monkeypatch: pytest.MonkeyPatch) -> None: - def provider(self, client) -> GoogleVertexAIProvider: + def provider(self, client: VertexClient) -> GoogleVertexAIProvider: - def test_stt_happy_path(self, provider, stt_config, query, audio_ref): + def test_stt_happy_path( + self, + provider: GoogleVertexAIProvider, + stt_config: NativeCompletionConfig, + query: QueryParams, + audio_ref: AudioRef, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code" and "Use Python 3.11+ with type hints throughout the codebase".
Also applies to: 93-93, 121-448
🤖 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/providers/test_gai_vertex.py` around lines 72 - 73, Add explicit type annotations to the fixtures and test functions that currently lack them: annotate the autouse fixture _mock_gcs as def _mock_gcs(monkeypatch: pytest.MonkeyPatch) -> None, and similarly add parameter and return type hints (-> None) to all test functions and other fixtures referenced later (every test from the block starting around line 121 onward). Ensure imports include pytest for the MonkeyPatch type if not already present, and follow the same pattern for any other fixtures (e.g., use appropriate types for fixture params) so every function signature has full parameter and return annotations.
🧹 Nitpick comments (4)
backend/app/tests/services/llm/test_multimodal.py (1)
511-513: ⚡ Quick winAssert against the shared Google default constant instead of a literal model string.
Using a literal here makes the test fail on unrelated default-model updates.
🤖 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_multimodal.py` around lines 511 - 513, Replace the literal "gemini-2.5-pro" in the assertion with the shared Google default constant (e.g. GOOGLE_DEFAULT_MODEL): import that constant from the module where Google defaults are defined and change the assertion to assert call_kwargs["model"] == GOOGLE_DEFAULT_MODEL so the test uses the centralized default instead of a hardcoded string; target the assertion around mock_client.models.generate_content.call_args[1].backend/app/tests/services/llm/test_mappers.py (1)
496-499: ⚡ Quick winAvoid hardcoded default-model literals in fallback assertions.
These assertions are brittle against future default-model updates. Prefer importing and asserting against the shared default constants used by the mapper layer.
Suggested diff
+from app.models.llm.constants import ( + DEFAULT_SARVAM_TTS_MODEL, + DEFAULT_ELEVENLABS_TTS_MODEL, +) ... - assert result["model"] == "bulbul:v3" + assert result["model"] == DEFAULT_SARVAM_TTS_MODEL ... - assert result["model_id"] == "eleven_v3" + assert result["model_id"] == DEFAULT_ELEVENLABS_TTS_MODELAlso applies to: 754-757
🤖 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 496 - 499, Replace the hardcoded "bulbul:v3" in the test assertions with the shared default constant used by the mapper layer: import the default constant from the mapper module (e.g., from app.services.llm.mappers import DEFAULT_MODEL or DEFAULT_VOICE_MODEL — whichever constant is used by the mapper) and assert result["model"] == DEFAULT_MODEL; do the same replacement for the second occurrence at lines ~754-757, leaving the other assertions (speaker, target_language_code, warnings) unchanged.backend/app/tests/services/llm/providers/test_claude.py (1)
64-92: ⚡ Quick winConvert pytest fixtures to factory-style fixtures for tests.
Fixtures in this section directly construct and return objects; test guidelines require factory pattern fixtures under
backend/app/tests/.As per coding guidelines: "backend/app/tests/**/*.py: Use factory pattern for test fixtures in
backend/app/tests/."🤖 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/providers/test_claude.py` around lines 64 - 92, The current pytest fixtures (query, config, mock_client, provider) return concrete objects directly; update them to factory-style fixtures that return callables which create and return fresh instances when invoked (e.g., query_factory -> callable that constructs QueryParams(input=...), config_factory -> callable that constructs NativeCompletionConfig(...), mock_client_factory -> callable building a new MagicMock with messages/files behavior, provider_factory -> callable that accepts a client and returns ClaudeProvider(client=...)). Modify tests to call these factories to get instances; reference the existing symbols QueryParams, NativeCompletionConfig, MagicMock, and ClaudeProvider when implementing the factories so each test gets isolated fresh objects.backend/app/tests/services/llm/providers/test_gai_vertex.py (1)
82-118: ⚡ Quick winSwitch test fixtures to the factory pattern in
backend/app/tests/.Current fixtures directly return concrete objects. The test guideline for
backend/app/tests/**/*.pyrequires factory-style fixtures; please convert these fixtures to return callables/builders.As per coding guidelines: "backend/app/tests/**/*.py: Use factory pattern for test fixtures in
backend/app/tests/."🤖 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/providers/test_gai_vertex.py` around lines 82 - 118, Convert the fixtures to factories that return callables which create instances on demand: change the client, provider, query, audio_ref, stt_config, and tts_config fixtures so they return a zero-argument function (or lambda) that constructs and returns a new VertexClient, GoogleVertexAIProvider (constructed with the client factory), QueryParams, AudioRef, and NativeCompletionConfig respectively; update tests to call these factories when they need an instance. Ensure you reference the same constructors/classes (VertexClient, GoogleVertexAIProvider, QueryParams, AudioRef, NativeCompletionConfig) and preserve the current init args (api_key, project_id, location, sa_info, gcs_bucket, provider/type/params, etc.) in the factory builders.
🤖 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/tests/services/llm/test_mappers.py`:
- Around line 488-499: The two new test functions (e.g.,
test_missing_model_falls_back_to_default and the other test added around lines
746-757) need explicit return type hints to conform to repository typing
standards; update their signatures to include "-> None" (e.g., def
test_missing_model_falls_back_to_default() -> None:) and do the same for the
other test function(s) in this file so all new tests declare a return type of
None.
In `@backend/app/tests/services/llm/test_multimodal.py`:
- Around line 498-500: Add a return type annotation to the test function
test_missing_model_falls_back_to_default by changing its signature to include ->
None; locate the method named test_missing_model_falls_back_to_default in the
test_multimodal.py file (it uses self._make_provider() inside) and annotate the
function definition so it reads with a return type of None to satisfy the
project type-hinting guideline.
---
Duplicate comments:
In `@backend/app/tests/services/llm/providers/test_claude.py`:
- Around line 98-108: Update the test function signatures in test_claude.py
(e.g., test_requires_api_key and test_returns_anthropic_client) to include
explicit type annotations for any pytest fixtures/parameters and add return type
-> None; ensure all other test functions in the same file (and ranges called out
around lines 114-190 and 196-443) follow the same pattern so every def has
annotated parameters and a -> None return annotation, preserving existing names
like ClaudeProvider.create_client and the patched
"app.services.llm.providers.claude.Anthropic".
In `@backend/app/tests/services/llm/providers/test_gai_vertex.py`:
- Around line 72-73: Add explicit type annotations to the fixtures and test
functions that currently lack them: annotate the autouse fixture _mock_gcs as
def _mock_gcs(monkeypatch: pytest.MonkeyPatch) -> None, and similarly add
parameter and return type hints (-> None) to all test functions and other
fixtures referenced later (every test from the block starting around line 121
onward). Ensure imports include pytest for the MonkeyPatch type if not already
present, and follow the same pattern for any other fixtures (e.g., use
appropriate types for fixture params) so every function signature has full
parameter and return annotations.
---
Nitpick comments:
In `@backend/app/tests/services/llm/providers/test_claude.py`:
- Around line 64-92: The current pytest fixtures (query, config, mock_client,
provider) return concrete objects directly; update them to factory-style
fixtures that return callables which create and return fresh instances when
invoked (e.g., query_factory -> callable that constructs QueryParams(input=...),
config_factory -> callable that constructs NativeCompletionConfig(...),
mock_client_factory -> callable building a new MagicMock with messages/files
behavior, provider_factory -> callable that accepts a client and returns
ClaudeProvider(client=...)). Modify tests to call these factories to get
instances; reference the existing symbols QueryParams, NativeCompletionConfig,
MagicMock, and ClaudeProvider when implementing the factories so each test gets
isolated fresh objects.
In `@backend/app/tests/services/llm/providers/test_gai_vertex.py`:
- Around line 82-118: Convert the fixtures to factories that return callables
which create instances on demand: change the client, provider, query, audio_ref,
stt_config, and tts_config fixtures so they return a zero-argument function (or
lambda) that constructs and returns a new VertexClient, GoogleVertexAIProvider
(constructed with the client factory), QueryParams, AudioRef, and
NativeCompletionConfig respectively; update tests to call these factories when
they need an instance. Ensure you reference the same constructors/classes
(VertexClient, GoogleVertexAIProvider, QueryParams, AudioRef,
NativeCompletionConfig) and preserve the current init args (api_key, project_id,
location, sa_info, gcs_bucket, provider/type/params, etc.) in the factory
builders.
In `@backend/app/tests/services/llm/test_mappers.py`:
- Around line 496-499: Replace the hardcoded "bulbul:v3" in the test assertions
with the shared default constant used by the mapper layer: import the default
constant from the mapper module (e.g., from app.services.llm.mappers import
DEFAULT_MODEL or DEFAULT_VOICE_MODEL — whichever constant is used by the mapper)
and assert result["model"] == DEFAULT_MODEL; do the same replacement for the
second occurrence at lines ~754-757, leaving the other assertions (speaker,
target_language_code, warnings) unchanged.
In `@backend/app/tests/services/llm/test_multimodal.py`:
- Around line 511-513: Replace the literal "gemini-2.5-pro" in the assertion
with the shared Google default constant (e.g. GOOGLE_DEFAULT_MODEL): import that
constant from the module where Google defaults are defined and change the
assertion to assert call_kwargs["model"] == GOOGLE_DEFAULT_MODEL so the test
uses the centralized default instead of a hardcoded string; target the assertion
around mock_client.models.generate_content.call_args[1].
🪄 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
Run ID: badf1c84-2983-402d-9c45-20d9ea09ba4d
📒 Files selected for processing (7)
backend/app/tests/services/llm/providers/test_claude.pybackend/app/tests/services/llm/providers/test_gai.pybackend/app/tests/services/llm/providers/test_gai_vertex.pybackend/app/tests/services/llm/providers/test_registry.pybackend/app/tests/services/llm/test_mappers.pybackend/app/tests/services/llm/test_multimodal.pybackend/app/tests/test_utils.py
💤 Files with no reviewable changes (1)
- backend/app/tests/services/llm/providers/test_gai.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/services/llm/providers/test_registry.py
Comment on lines
+488
to
+499
| def test_missing_model_falls_back_to_default(self): | ||
| """Missing model falls back to DEFAULT_SARVAM_TTS_MODEL without warnings.""" | ||
| kaapi_params = {"voice": "Shubh", "language": "hi-IN"} | ||
|
|
||
| result, warnings = map_kaapi_to_sarvam_params( | ||
| kaapi_params, completion_type="tts" | ||
| ) | ||
|
|
||
| assert result == {} | ||
| assert len(warnings) == 1 | ||
| assert "model" in warnings[0].lower() | ||
| assert result["model"] == "bulbul:v3" | ||
| assert result["speaker"] == "Shubh" | ||
| assert result["target_language_code"] == "hi-IN" | ||
| assert warnings == [] |
There was a problem hiding this comment.
Add return type hints to the new test methods.
Both newly added test functions should declare -> None to match the repository’s Python typing standard.
As per coding guidelines: **/*.py: Always add type hints to all function parameters and return values in Python code.
Also applies to: 746-757
🤖 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 488 - 499, The
two new test functions (e.g., test_missing_model_falls_back_to_default and the
other test added around lines 746-757) need explicit return type hints to
conform to repository typing standards; update their signatures to include "->
None" (e.g., def test_missing_model_falls_back_to_default() -> None:) and do the
same for the other test function(s) in this file so all new tests declare a
return type of None.
Comment on lines
+498
to
+500
| def test_missing_model_falls_back_to_default(self): | ||
| """No model in params → provider uses DEFAULT_TEXT_MODELS['google'].""" | ||
| provider, mock_client = self._make_provider() |
There was a problem hiding this comment.
Type annotate the new test method.
Please add -> None to test_missing_model_falls_back_to_default.
As per coding guidelines: **/*.py: Always add type hints to all function parameters and return values in Python code.
🤖 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_multimodal.py` around lines 498 - 500,
Add a return type annotation to the test function
test_missing_model_falls_back_to_default by changing its signature to include ->
None; locate the method named test_missing_model_falls_back_to_default in the
test_multimodal.py file (it uses self._make_provider() inside) and annotate the
function definition so it reads with a return type of None to satisfy the
project type-hinting guideline.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Target issue is #872 and #905
Add claude and gemini via vertex to the llm/call.
Summary
New Features
Refactor
Chores
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.Summary by CodeRabbit
New Features
Tests