feat(llm): Introduce type safety checks - #1089
Open
Prajna1999 wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR introduces typed Kaapi completion variants, centralizes parameter normalization, moves STS language validation into request models, updates execution paths and tests, and centralizes persisted LLM audio output resolution. ChangesLLM configuration and execution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
changed the title
OpenAPI changes 🔴 8 breaking changesCaution Downstream consumers may need an update before merging. Breaking changes ·
|
| Method | Path | Change | |
|---|---|---|---|
| 🔴 | POST |
/api/v1/configs |
added the new required request property config_blob/completion/anyOf[subschema #3: ProxyCompletionConfig -> subschema #5: ProxyCompletionConfig]/params/client_llm_url |
| 🔴 | POST |
/api/v1/configs |
removed #/components/schemas/KaapiCompletionConfig from the config_blob/completion request property anyOf list |
| 🔴 | POST |
/api/v1/llm/call |
added the new required request property config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #3: ProxyCompletionConfig -> subschema #5: ProxyCompletionConfig]/params/client_llm_url |
| 🔴 | POST |
/api/v1/llm/call |
removed #/components/schemas/KaapiCompletionConfig from the config/blob/anyOf[subschema #1: ConfigBlob]/completion request property anyOf list |
| 🔴 | POST |
/api/v1/llm/chain |
added the new required request property blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #3: ProxyCompletionConfig -> subschema #5: ProxyCompletionConfig]/params/client_llm_url |
| 🔴 | POST |
/api/v1/llm/chain |
removed #/components/schemas/KaapiCompletionConfig from the blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion request property anyOf list |
| 🔴 | POST |
/api/v1/llm/chain/sts |
request property input_language was restricted to a list of enum values |
| 🔴 | POST |
/api/v1/llm/chain/sts |
request property output_language/anyOf[subschema #1]/ was restricted to a list of enum values |
Full changelog · 63
| Method | Path | Change | |
|---|---|---|---|
| 🔴 | POST |
/api/v1/configs |
added the new required request property config_blob/completion/anyOf[subschema #3: ProxyCompletionConfig -> subschema #5: ProxyCompletionConfig]/params/client_llm_url |
| 🔴 | POST |
/api/v1/configs |
removed #/components/schemas/KaapiCompletionConfig from the config_blob/completion request property anyOf list |
| 🔴 | POST |
/api/v1/llm/call |
added the new required request property config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #3: ProxyCompletionConfig -> subschema #5: ProxyCompletionConfig]/params/client_llm_url |
| 🔴 | POST |
/api/v1/llm/call |
removed #/components/schemas/KaapiCompletionConfig from the config/blob/anyOf[subschema #1: ConfigBlob]/completion request property anyOf list |
| 🔴 | POST |
/api/v1/llm/chain |
added the new required request property blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion/anyOf[subschema #3: ProxyCompletionConfig -> subschema #5: ProxyCompletionConfig]/params/client_llm_url |
| 🔴 | POST |
/api/v1/llm/chain |
removed #/components/schemas/KaapiCompletionConfig from the blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion request property anyOf list |
| 🔴 | POST |
/api/v1/llm/chain/sts |
request property input_language was restricted to a list of enum values |
| 🔴 | POST |
/api/v1/llm/chain/sts |
request property output_language/anyOf[subschema #1]/ was restricted to a list of enum values |
| 🟢 | — |
— |
removed the schema KaapiCompletionConfig |
| 🟢 | — |
— |
a breaking change was detected but the version is still 0.5.0 |
| 🟢 | POST |
/api/v1/configs |
added #/components/schemas/KaapiTextCompletionConfig, #/components/schemas/KaapiSTTCompletionConfig, #/components/schemas/KaapiTTSCompletionConfig to the config_blob/completion request property anyOf list |
| 🟢 | POST |
/api/v1/llm/call |
added #/components/schemas/KaapiTextCompletionConfig, #/components/schemas/KaapiSTTCompletionConfig, #/components/schemas/KaapiTTSCompletionConfig to the config/blob/anyOf[subschema #1: ConfigBlob]/completion request property anyOf list |
| 🟢 | POST |
/api/v1/llm/chain |
added #/components/schemas/KaapiTextCompletionConfig, #/components/schemas/KaapiSTTCompletionConfig, #/components/schemas/KaapiTTSCompletionConfig to the blocks/items/config/blob/anyOf[subschema #1: ConfigBlob]/completion request property anyOf list |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new as-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new as-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new auto enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new auto enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new bn-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new bn-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new brx-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new brx-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new doi-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new doi-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new en-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new en-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new gu-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new gu-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new hi-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new hi-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new kn-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new kn-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new kok-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new kok-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ks-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ks-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new mai-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new mai-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ml-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ml-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new mni-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new mni-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new mr-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new mr-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ne-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ne-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new od-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new od-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new pa-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new pa-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new sa-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new sa-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new sat-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new sat-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new sd-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new sd-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ta-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ta-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new te-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new te-IN enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new unknown enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new unknown enum value to the request property output_language/anyOf[subschema #1]/ |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ur-IN enum value to the request property input_language |
| 🟢 | POST |
/api/v1/llm/chain/sts |
added the new ur-IN enum value to the request property output_language/anyOf[subschema #1]/ |
main ↔ 6ef6f5d1 · 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/tests/models/llm/test_request.py (1)
10-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
build_kaapi_completion_configcall sites aligned with the typed contract.The builder signature accepts
Providervalues,CompletionType, and typed params models, but these tests pass raw strings and plain dictionaries. Update the affected tests to useProvider.OPENAI/Provider.GOOGLE,CompletionType.TEXT, andTextLLMParams, or narrow the builder contract if raw dicts are intentional.🤖 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/models/llm/test_request.py` around lines 10 - 17, Update build_kaapi_completion_config call sites to satisfy its typed contract: in backend/app/tests/models/llm/test_request.py lines 10-17 and backend/app/tests/services/llm/test_mappers.py lines 895-899, use Provider.OPENAI or Provider.GOOGLE, CompletionType.TEXT, and TextLLMParams instead of raw strings and dictionaries; preserve the existing test scenarios without narrowing the builder contract.Source: Coding guidelines
🧹 Nitpick comments (5)
backend/app/api/routes/llm.py (1)
35-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove output resolution to an LLM service.
This route now owns storage access, URI conversion, and model validation. Extract
_resolve_llm_outputinto the LLM service layer; keep the route limited to request/response orchestration. As per coding guidelines, API routes expose REST endpoints while services contain business logic.🤖 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/api/routes/llm.py` around lines 35 - 65, Move the _resolve_llm_output function and its storage, URI conversion, presigned-URL error handling, and LLMOutput validation logic into the LLM service layer. Update the route to call the service-level resolver, leaving the endpoint responsible only for request/response orchestration and removing its direct business-logic dependencies.Source: Coding guidelines
backend/app/models/llm/request.py (3)
345-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
X | Yinstead ofUnion[...]insideAnnotated[...].Static analysis flags both discriminated-union type aliases for the deprecated
Union[...]syntax; the codebase already targets Python 3.11+, so the PEP 604|syntax works fine here (including insideAnnotated).♻️ Suggested fix
KaapiCompletionConfig = Annotated[ - Union[ - KaapiTextCompletionConfig, KaapiSTTCompletionConfig, KaapiTTSCompletionConfig - ], + KaapiTextCompletionConfig | KaapiSTTCompletionConfig | KaapiTTSCompletionConfig, Field(discriminator="type"), ] ... CompletionConfig = Annotated[ - Union[ - NativeCompletionConfig, - KaapiTextCompletionConfig, - KaapiSTTCompletionConfig, - KaapiTTSCompletionConfig, - ProxyCompletionConfig, - ], + NativeCompletionConfig + | KaapiTextCompletionConfig + | KaapiSTTCompletionConfig + | KaapiTTSCompletionConfig + | ProxyCompletionConfig, Field(discriminator="provider"), ]Also applies to: 397-403
🤖 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/models/llm/request.py` around lines 345 - 347, Replace the Union[...] syntax in both discriminated-union type aliases around the Kaapi completion configurations with PEP 604 | syntax, preserving the existing Annotated metadata and member types.Source: Linters/SAST tools
308-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
_default_providervalidator between STT and TTS variants.The identical
model_validator(mode="after")defaultingprovidertoProvider.GOOGLEis copy-pasted betweenKaapiSTTCompletionConfigandKaapiTTSCompletionConfig. SinceKaapiTextCompletionConfigintentionally omits this default, it can't simply move to_KaapiCompletionConfigBase, but it could be extracted into a small mixin/shared helper that only STT/TTS opt into, to avoid future drift between the two copies.♻️ Example extraction
+class _DefaultsToGoogleMixin: + `@model_validator`(mode="after") + def _default_provider(self) -> Self: + if self.provider is None: + self.provider = Provider.GOOGLE + return self + + -class KaapiSTTCompletionConfig(_KaapiCompletionConfigBase): +class KaapiSTTCompletionConfig(_KaapiCompletionConfigBase, _DefaultsToGoogleMixin): type: Literal[CompletionType.STT] = Field( ..., description="Completion config type. Params schema varies by type" ) params: STTLLMParams = Field( ..., description="Kaapi-standardized parameters mapped to provider-specific API" ) - - `@model_validator`(mode="after") - def _default_provider(self) -> Self: - if self.provider is None: - self.provider = Provider.GOOGLE - return selfAlso applies to: 323-335
🤖 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/models/llm/request.py` around lines 308 - 320, Extract the duplicated _default_provider model validator from KaapiSTTCompletionConfig and KaapiTTSCompletionConfig into a shared mixin or helper, then have only those STT and TTS configuration classes opt into it. Preserve KaapiTextCompletionConfig’s behavior of leaving provider unset and keep the existing Provider.GOOGLE default behavior unchanged.
1074-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
("auto", "unknown")sentinel tuple into a named constant.The same magic tuple appears in both
normalize_language_casingandvalidate_output_language. As per coding guidelines, "Do not use magic values; extract repeated literals into constants, enums, or settings." A shared module-level constant (e.g.STT_ONLY_LANGUAGE_SENTINELS = ("auto", "unknown")) would remove the duplication and keep the two validators in sync if a sentinel is ever added/removed.🤖 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/models/llm/request.py` around lines 1074 - 1096, Define a shared module-level constant for the STT-only language sentinels ("auto", "unknown"), then update normalize_language_casing and validate_output_language to reference it instead of repeating the tuple. Preserve the existing sentinel handling and validation behavior.Source: Coding guidelines
backend/app/api/routes/llm_sts.py (1)
109-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow
providertype hint to match the callee's contract.
_inline_call_config'sprovider: str | Noneis wider thanbuild_kaapi_completion_config'sprovider: KaapiProvider | None. Since this is an internal typed-safety refactor, tightening the hint here would keep the type-checking benefit end-to-end.🤖 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/api/routes/llm_sts.py` around lines 109 - 122, The _inline_call_config provider parameter is broader than build_kaapi_completion_config accepts. Narrow its type annotation from str | None to KaapiProvider | None, using the existing KaapiProvider symbol and preserving the provider forwarding behavior.
🤖 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/api/routes/llm.py`:
- Around line 59-61: Update the logger.warning call in _resolve_llm_output so
its prefix uses _resolve_llm_output instead of get_llm_call_status, while
preserving the existing error details and job_id context.
- Line 57: Extract the 3600-second expiration value used in the
storage.get_signed_url call within the surrounding route logic into a clearly
named, unit-suffixed constant, then pass that constant as expires_in. Keep the
existing one-hour TTL behavior unchanged.
- Around line 35-40: Update _resolve_llm_output to use the narrowest concrete
type for raw_content based on the validated LLM response schema, replacing
unconstrained dict, and change its return annotation from LLMOutput | None to
LLMOutput. Preserve the existing validation behavior where valid input returns
LLMOutput and invalid input raises.
In `@backend/app/crud/assessment/batch.py`:
- Around line 417-423: Update the typed-parameter normalization in the batch
completion flow to call model_dump with exclude_unset=True alongside
exclude_none=True, preserving only explicitly provided Kaapi values such as
temperature. Ensure the nearby Temperature suppression checks in the same
normalization logic are based on provided values, not default-populated fields,
while leaving native dict handling unchanged.
In `@backend/app/models/llm/constants.py`:
- Around line 66-68: Update the comments above STSLanguageCode to identify this
Literal alias as the single source of truth for accepted speech-to-speech
language codes, and state that SUPPORTED_LANGUAGE_CODES is derived from it via
get_args. Do not describe SUPPORTED_LANGUAGE_CODES as authoritative.
In `@backend/app/services/llm/jobs.py`:
- Around line 837-847: Update the guardrail-direct-response path before the
branch around config_blob.completion.params.get("model") to support typed Kaapi
text, STT, and TTS configurations. Normalize those configs with
transform_kaapi_config_to_native or convert params through kaapi_params_as_dict
before accessing model, while preserving the existing guarded-response behavior.
---
Outside diff comments:
In `@backend/app/tests/models/llm/test_request.py`:
- Around line 10-17: Update build_kaapi_completion_config call sites to satisfy
its typed contract: in backend/app/tests/models/llm/test_request.py lines 10-17
and backend/app/tests/services/llm/test_mappers.py lines 895-899, use
Provider.OPENAI or Provider.GOOGLE, CompletionType.TEXT, and TextLLMParams
instead of raw strings and dictionaries; preserve the existing test scenarios
without narrowing the builder contract.
---
Nitpick comments:
In `@backend/app/api/routes/llm_sts.py`:
- Around line 109-122: The _inline_call_config provider parameter is broader
than build_kaapi_completion_config accepts. Narrow its type annotation from str
| None to KaapiProvider | None, using the existing KaapiProvider symbol and
preserving the provider forwarding behavior.
In `@backend/app/api/routes/llm.py`:
- Around line 35-65: Move the _resolve_llm_output function and its storage, URI
conversion, presigned-URL error handling, and LLMOutput validation logic into
the LLM service layer. Update the route to call the service-level resolver,
leaving the endpoint responsible only for request/response orchestration and
removing its direct business-logic dependencies.
In `@backend/app/models/llm/request.py`:
- Around line 345-347: Replace the Union[...] syntax in both discriminated-union
type aliases around the Kaapi completion configurations with PEP 604 | syntax,
preserving the existing Annotated metadata and member types.
- Around line 308-320: Extract the duplicated _default_provider model validator
from KaapiSTTCompletionConfig and KaapiTTSCompletionConfig into a shared mixin
or helper, then have only those STT and TTS configuration classes opt into it.
Preserve KaapiTextCompletionConfig’s behavior of leaving provider unset and keep
the existing Provider.GOOGLE default behavior unchanged.
- Around line 1074-1096: Define a shared module-level constant for the STT-only
language sentinels ("auto", "unknown"), then update normalize_language_casing
and validate_output_language to reference it instead of repeating the tuple.
Preserve the existing sentinel handling and validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7a0b7f7-4cae-4c8a-bb6c-1c2830b7950f
📒 Files selected for processing (26)
backend/app/api/routes/llm.pybackend/app/api/routes/llm_sts.pybackend/app/core/langfuse/langfuse.pybackend/app/crud/assessment/batch.pybackend/app/crud/evaluations/core.pybackend/app/crud/model_config.pybackend/app/models/llm/__init__.pybackend/app/models/llm/constants.pybackend/app/models/llm/request.pybackend/app/services/evaluations/batch_job.pybackend/app/services/llm/chain/utils.pybackend/app/services/llm/jobs.pybackend/app/services/llm/mappers.pybackend/app/tests/api/routes/configs/test_version.pybackend/app/tests/api/routes/test_evaluation_fast.pybackend/app/tests/api/routes/test_evaluation_v2.pybackend/app/tests/api/routes/test_improve_prompt.pybackend/app/tests/api/routes/test_llm.pybackend/app/tests/crud/evaluations/test_fast_judge.pybackend/app/tests/crud/test_llm.pybackend/app/tests/models/llm/test_request.pybackend/app/tests/services/llm/test_jobs.pybackend/app/tests/services/llm/test_mappers.pybackend/app/tests/services/llm/test_sts.pybackend/app/tests/utils/llm.pybackend/app/tests/utils/test_data.py
Comment on lines
+35
to
+40
| def _resolve_llm_output( | ||
| raw_content: dict, | ||
| project_id: int, | ||
| session: Session, | ||
| job_id: UUID, | ||
| ) -> LLMOutput | None: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the helper contract.
dict is unconstrained, and this function never returns None: validation either returns LLMOutput or raises. As per coding guidelines, every parameter and return value needs a narrow type.
Proposed fix
def _resolve_llm_output(
- raw_content: dict,
+ raw_content: dict[str, object],
project_id: int,
session: Session,
job_id: UUID,
-) -> LLMOutput | None:
+) -> LLMOutput:📝 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_llm_output( | |
| raw_content: dict, | |
| project_id: int, | |
| session: Session, | |
| job_id: UUID, | |
| ) -> LLMOutput | None: | |
| def _resolve_llm_output( | |
| raw_content: dict[str, object], | |
| project_id: int, | |
| session: Session, | |
| job_id: UUID, | |
| ) -> LLMOutput: |
🤖 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/api/routes/llm.py` around lines 35 - 40, Update
_resolve_llm_output to use the narrowest concrete type for raw_content based on
the validated LLM response schema, replacing unconstrained dict, and change its
return annotation from LLMOutput | None to LLMOutput. Preserve the existing
validation behavior where valid input returns LLMOutput and invalid input
raises.
Source: Coding guidelines
Comment on lines
+59
to
+61
| logger.warning( | ||
| f"[get_llm_call_status] Failed to generate presigned URL for audio: {e} | job_id={job_id}" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the helper’s name in this log prefix.
This warning originates in _resolve_llm_output, not get_llm_call_status; the current prefix misattributes presigning failures. As per coding guidelines, every log line must be prefixed with its function name.
🤖 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/api/routes/llm.py` around lines 59 - 61, Update the
logger.warning call in _resolve_llm_output so its prefix uses
_resolve_llm_output instead of get_llm_call_status, while preserving the
existing error details and job_id context.
Source: Coding guidelines
Comment on lines
+66
to
+68
| # BCP-47 language codes accepted by the speech-to-speech endpoint (STT input / | ||
| # TTS output). Single source of truth: `SUPPORTED_LANGUAGE_CODES` in | ||
| # `app/services/llm/chain/utils.py` derives from this via `get_args`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the alias the documented source of truth.
This comment calls SUPPORTED_LANGUAGE_CODES authoritative while also stating that it is derived from STSLanguageCode. Since utils.py derives the set via get_args, describe this Literal as the source of truth to avoid future edits to the wrong declaration.
Suggested wording
-# TTS output). Single source of truth: `SUPPORTED_LANGUAGE_CODES` in
-# `app/services/llm/chain/utils.py` derives from this via `get_args`.
+# TTS output). This Literal is the single source of truth; `SUPPORTED_LANGUAGE_CODES`
+# in `app/services/llm/chain/utils.py` is derived from it via `get_args`.📝 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
| # BCP-47 language codes accepted by the speech-to-speech endpoint (STT input / | |
| # TTS output). Single source of truth: `SUPPORTED_LANGUAGE_CODES` in | |
| # `app/services/llm/chain/utils.py` derives from this via `get_args`. | |
| # BCP-47 language codes accepted by the speech-to-speech endpoint (STT input / | |
| # TTS output). This Literal is the single source of truth; `SUPPORTED_LANGUAGE_CODES` | |
| # in `app/services/llm/chain/utils.py` is derived from it via `get_args`. |
🤖 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/models/llm/constants.py` around lines 66 - 68, Update the
comments above STSLanguageCode to identify this Literal alias as the single
source of truth for accepted speech-to-speech language codes, and state that
SUPPORTED_LANGUAGE_CODES is derived from it via get_args. Do not describe
SUPPORTED_LANGUAGE_CODES as authoritative.
Prajna1999
added
the
breaking-change-approved
Prajna1999
marked this pull request as ready for review
July 28, 2026 10:20
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Prajna1999
added
ready-for-review
and removed
breaking-change-approved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/tests/crud/evaluations/test_fast_judge.py (2)
171-174: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd missing type annotations to the new test helpers.
_both_metrics_responseneeds annotations forusageand the returned response type;_summary_responseneeds a return annotation;_run_pipelineneedssummary_side_effectannotated; and the added helper/test methods inTestRunOverallSummaryandTestVerdictBandOnTraceScoresneed parameter and-> Noneannotations for every method. Use concrete response/fixture types instead ofAny.🤖 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/crud/evaluations/test_fast_judge.py` around lines 171 - 174, Update the new test helpers and methods in _both_metrics_response, _summary_response, _run_pipeline, TestRunOverallSummary, and TestVerdictBandOnTraceScores with complete type annotations: use concrete response and fixture types, annotate usage and summary_side_effect, add each helper’s return type, and mark every test/helper method with parameter annotations and -> None where applicable; do not use Any.Source: Coding guidelines
5-5: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse integer types for judge scores.
_both_metrics_responsedocuments integer 0–5 judge scores, but the parameters usetuple[float, str], which allows invalid fixtures such as(2.5, "..."). Change bothground_truthandpromptannotations totuple[int, str].Proposed fix
- ground_truth: tuple[float, str] = (4, "conveys the same facts"), - prompt: tuple[float, str] = (3, "answered in the wrong language"), + ground_truth: tuple[int, str] = (4, "conveys the same facts"), + prompt: tuple[int, str] = (3, "answered in the wrong language"),🤖 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/crud/evaluations/test_fast_judge.py` at line 5, Update the _both_metrics_response fixture annotations so both ground_truth and prompt use tuple[int, str] instead of tuple[float, str], enforcing integer judge scores while preserving the existing fixture behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@backend/app/tests/crud/evaluations/test_fast_judge.py`:
- Around line 171-174: Update the new test helpers and methods in
_both_metrics_response, _summary_response, _run_pipeline, TestRunOverallSummary,
and TestVerdictBandOnTraceScores with complete type annotations: use concrete
response and fixture types, annotate usage and summary_side_effect, add each
helper’s return type, and mark every test/helper method with parameter
annotations and -> None where applicable; do not use Any.
- Line 5: Update the _both_metrics_response fixture annotations so both
ground_truth and prompt use tuple[int, str] instead of tuple[float, str],
enforcing integer judge scores while preserving the existing fixture behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a672a14-8247-4277-809e-19a90555b997
📒 Files selected for processing (5)
backend/app/core/langfuse/langfuse.pybackend/app/crud/evaluations/core.pybackend/app/models/llm/constants.pybackend/app/models/llm/request.pybackend/app/tests/crud/evaluations/test_fast_judge.py
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/app/models/llm/constants.py
- backend/app/core/langfuse/langfuse.py
- backend/app/crud/evaluations/core.py
- backend/app/models/llm/request.py
- Add _CompactParamsSerializerMixin to Text/STT/TTS LLM params so every model_dump (Celery request_data, persisted config blobs) reproduces the pre-typed wire format: None fields dropped, unset temperature dropped. Without it a JSON round-trip baked temperature=0.1 into model_fields_set and providers received a temperature the user never set. - Fix guardrail direct-response branch in jobs.py: it runs before the Kaapi->native transform, so params may be a typed model without .get(). - Fix stale KaapiCompletionConfig class-style caller in test_improve_prompt_v2.py (TypeError: cannot instantiate Union). - Widen build_kaapi_completion_config annotations to the str/dict forms callers actually pass; simplify kaapi_params_as_dict. - Add round-trip regression test for unset temperature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unset temperature is dropped from params dumps now, so the mapper never receives it: no default 0.1 in the openai result, and no spurious suppression warning for reasoning models. Add explicit-temperature passthrough coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/models/llm/request.py (1)
1099-1106: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the broad
Anyannotations.
normalize_language_casingdeclaresdata: Anyand returnsAny. Use a concrete raw-input type such asobject, or define the exact accepted mapping union.As per coding guidelines, Python functions must use narrow type hints for every parameter and return value, and must not use
-> Anyas a substitute.🤖 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/models/llm/request.py` around lines 1099 - 1106, Update the normalize_language_casing validator’s broad Any annotations to use a narrow raw-input type, such as object, for both its data parameter and return value. Preserve the existing behavior of returning non-dictionary inputs unchanged while allowing dictionary inputs to continue through language-casing normalization.Source: Coding guidelines
backend/app/services/llm/mappers.py (1)
531-544: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNormalize raw dictionaries before provider mapping.
kaapi_params_as_dict()preserves anydict[str, Any], including{"temperature": null}or{"temperature": 0.1}from legacy configs. DropNonevalues and unset/default temperature for dictionaries too, or load dicts through the compact model serializer, to keep provider mappers off the legacy defaults path.🤖 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/mappers.py` around lines 531 - 544, The kaapi_params_as_dict function currently returns raw dictionaries without applying compact parameter normalization. Update its dict branch to remove None-valued fields and omit unset/default temperature, either by applying equivalent filtering or by loading the dictionary through the compact model serializer, while preserving normalized model behavior and returning a plain dict for provider mappers.
🤖 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.
Outside diff comments:
In `@backend/app/models/llm/request.py`:
- Around line 1099-1106: Update the normalize_language_casing validator’s broad
Any annotations to use a narrow raw-input type, such as object, for both its
data parameter and return value. Preserve the existing behavior of returning
non-dictionary inputs unchanged while allowing dictionary inputs to continue
through language-casing normalization.
In `@backend/app/services/llm/mappers.py`:
- Around line 531-544: The kaapi_params_as_dict function currently returns raw
dictionaries without applying compact parameter normalization. Update its dict
branch to remove None-valued fields and omit unset/default temperature, either
by applying equivalent filtering or by loading the dictionary through the
compact model serializer, while preserving normalized model behavior and
returning a plain dict for provider mappers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c15cfff3-ca4d-44ef-b210-4f9917826098
📒 Files selected for processing (6)
backend/app/models/llm/request.pybackend/app/services/llm/jobs.pybackend/app/services/llm/mappers.pybackend/app/tests/api/routes/test_improve_prompt_v2.pybackend/app/tests/models/llm/test_request.pybackend/app/tests/services/llm/test_mappers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/services/llm/test_mappers.py
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #1079
Summary
New Features
Bug Fixes
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit