unifed api: stt sarvam #579
Conversation
…-stt-new' into feature/unified-api-stt-new merge upstream
…ssistant_id handling
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persistent LLM call tracking, multimodal input resolution, new native and third‑party LLM providers, provider registry/client factories, partial config-version creation with immutable-type checks, migrations/models/CRUD/services, job integration, and extensive test updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API
participant ConfigCRUD as Config CRUD
participant JobSvc as Job Service
participant LLMCrud as LLM CRUD
participant InputRes as Input Resolver
participant Registry as Provider Registry
participant Provider
participant DB
Client->>API: POST /configs/{id}/versions (partial)
API->>ConfigCRUD: create_from_partial_or_raise(...)
ConfigCRUD->>DB: select latest version / insert merged version
DB-->>ConfigCRUD: latest/version created
API-->>Client: 201 Created
Client->>API: POST /jobs/execute
API->>JobSvc: start_job()
JobSvc->>DB: insert Job
JobSvc->>ConfigCRUD: resolve_config_blob(job)
ConfigCRUD-->>JobSvc: ConfigBlob
JobSvc->>LLMCrud: create_llm_call(...)
LLMCrud->>DB: insert llm_call
DB-->>LLMCrud: llm_call_id
LLMCrud-->>JobSvc: llm_call_id
JobSvc->>InputRes: resolve_input(QueryInput)
InputRes-->>JobSvc: resolved_input (text | temp_file)
JobSvc->>Registry: get_provider_class(provider_type)
Registry-->>JobSvc: ProviderClass
JobSvc->>Provider: instantiate via create_client(...)
JobSvc->>Provider: execute(config, query, resolved_input)
Provider-->>JobSvc: LLMCallResponse
JobSvc->>LLMCrud: update_llm_call_response(llm_call_id, response...)
LLMCrud->>DB: update llm_call
DB-->>LLMCrud: updated record
LLMCrud-->>JobSvc: updated record
JobSvc->>InputRes: cleanup_temp_file(if any)
JobSvc->>DB: update Job status
JobSvc-->>API: job complete
API-->>Client: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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/services/doctransform/zerox_transformer.py (1)
20-47:⚠️ Potential issue | 🟡 MinorPrefix all log messages with the function name.
Current log entries in
transform()don’t include the required[ZeroxTransformer.transform]prefix. Please align them with the logging guideline.
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)}")".Suggested fix
- logger.info(f"ZeroxTransformer Started: (model={self.model})") + logger.info(f"[ZeroxTransformer.transform] ZeroxTransformer Started: (model={self.model})") ... - logger.error( - f"ZeroxTransformer timed out for {input_path} (model={self.model})" - ) + logger.error( + f"[ZeroxTransformer.transform] ZeroxTransformer timed out for {input_path} (model={self.model})" + ) ... - logger.error( - f"ZeroxTransformer failed for {input_path}: {e}\n" - "This may be due to a missing Poppler installation or a corrupt PDF file.", - exc_info=True, - ) + logger.error( + f"[ZeroxTransformer.transform] ZeroxTransformer failed for {input_path}: {e}\n" + "This may be due to a missing Poppler installation or a corrupt PDF file.", + exc_info=True, + )backend/app/tests/services/doctransformer/test_job/utils.py (2)
73-99:⚠️ Potential issue | 🟡 MinorAdd type hints to factory helpers and nested callables.
The factory functions
create_failing_convert_documentandcreate_persistent_failing_convert_document, along with their nested callables, are missing type annotations. This violates the coding guideline requiring type hints on all function parameters and return values.Suggested fix
Add imports:
+from collections.abc import Callable +from typing import NoReturnApply type hints:
-def create_failing_convert_document(fail_count: int = 1): +def create_failing_convert_document(fail_count: int = 1) -> Callable[..., Path]: """Create a side effect function that fails specified times then succeeds.""" call_count = 0 - def failing_convert_document(*args, **kwargs): + def failing_convert_document(*args: object, **kwargs: object) -> Path: nonlocal call_count call_count += 1 if call_count <= fail_count: raise Exception("Transient error") output_path = args[1] if len(args) > 1 else kwargs.get("output_path") if output_path: output_path.write_text("Success after retries", encoding="utf-8") return output_path raise ValueError("output_path is required") return failing_convert_document -def create_persistent_failing_convert_document( - error_message: str = "Persistent error", -): +def create_persistent_failing_convert_document( + error_message: str = "Persistent error", +) -> Callable[..., NoReturn]: """Create a side effect function that always fails.""" - def persistent_failing_convert_document(*args, **kwargs): + def persistent_failing_convert_document(*args: object, **kwargs: object) -> NoReturn: raise Exception(error_message) return persistent_failing_convert_document
53-58:⚠️ Potential issue | 🟡 MinorAdd optional type hint to
expected_contentparameter.The parameter defaults to
None, so the type should allowNoneto satisfy type checking. Update the type hint tostr | None.Proposed fix
- expected_content: str = None, + expected_content: str | None = None,This aligns with the coding guideline to always add type hints to all function parameters and return values in Python code.
backend/app/tests/crud/config/test_config.py (1)
19-31:⚠️ Potential issue | 🟡 MinorAdd a return type hint (and consider a factory-style fixture).
The fixture is missing a return type annotation, and the test guideline prefers factory-pattern fixtures.As per coding guidelines “Always add type hints to all function parameters and return values in Python code” and “backend/app/tests/**/*.py: Use factory pattern for test fixtures in backend/app/tests/”.🔧 Proposed fix for the return type hint
-@pytest.fixture -def example_config_blob(): +@pytest.fixture +def example_config_blob() -> ConfigBlob: return ConfigBlob( completion=NativeCompletionConfig( provider="openai-native", type="text", params={ "model": "gpt-4", "temperature": 0.8, "max_tokens": 1500, }, ) )
🤖 Fix all issues with AI agents
In `@backend/app/alembic/versions/042_add_llm_call_table.py`:
- Around line 1-16: Update the module header docstring so the human-readable
Revision ID and Revises lines match the actual alembic variables: change the
header "Revision ID: 041" to "Revision ID: 042" and "Revises: 040" to "Revises:
041" to align with the `revision = "042"` and `down_revision = "041"` values in
this migration file (look for the top-of-file docstring and the
`revision`/`down_revision` symbols).
- Around line 20-21: Add explicit return type annotations to the Alembic
migration functions: annotate both upgrade() and downgrade() with -> None (e.g.,
def upgrade() -> None: and def downgrade() -> None:) so they satisfy the
project's typing requirements; update the function signatures where upgrade and
downgrade are defined in this migration file.
In `@backend/app/alembic/versions/043_remove_enum_checks_llm_call_provider.py`:
- Around line 20-30: The column comment on the "provider" column in the
upgrade() migration contains a likely typo ("AI provider as sent by user (e.g
openai, -native, google)"); update the comment to the correct provider token
(e.g. "AI provider as sent by user (e.g. openai, openai-native, google)" or
whichever canonical name is used) inside the op.alter_column call in upgrade(),
ensuring the existing_comment/existing_type/nullable parameters remain unchanged
and only the comment string is corrected.
In `@backend/app/api/routes/config/version.py`:
- Around line 31-41: Update the ConfigVersionCreatePartial model docstring in
backend/app/models/config/version.py to correctly state immutability: change the
incorrect sentence that says "provider and type are inherited... and cannot be
changed" to reflect that only "type is inherited from the existing config and
cannot be changed," while "provider, model, and params can be changed"; ensure
the docstring on the ConfigVersionCreatePartial class matches the route
docstring and the behavior enforced by _validate_immutable_fields.
In `@backend/app/crud/llm.py`:
- Around line 143-146: The log statement in create_llm_call uses an undefined
variable provider which will raise a NameError; update the logger.info call that
logs db_llm_call.id, job_id, provider, model to use the function parameter
original_provider instead of provider (i.e., reference original_provider in the
message) so the runtime uses the correct variable and the log prints the
provider value.
In `@backend/app/models/llm/request.py`:
- Around line 187-198: The model validator method validate_params in class
KaapiCompletionConfig (annotated with `@model_validator`(mode="after")) lacks a
return type; update its signature to include -> "KaapiCompletionConfig" so it
reads def validate_params(self) -> "KaapiCompletionConfig": (keep the body
unchanged and continue returning self) to satisfy the project's type-hinting
guideline.
- Around line 4-11: Remove the duplicate imports of Field and SQLModel (there
are two sqlmodel import lines in the top imports) so each symbol is imported
only once, and add explicit return type annotations "-> Self" to the three
validator methods validate_conversation_logic, validate_params, and
validate_config_logic (and import Self from typing or typing_extensions if not
already available) because they return self; ensure the validator signatures are
updated accordingly.
In `@backend/app/services/llm/jobs.py`:
- Around line 205-206: Remove the unused variable assignment
user_sent_config_provider = "" to fix the Ruff F841 lint error: locate the
assignment of user_sent_config_provider and delete that line (or replace it with
intended usage if it was meant to be used later in the function / job handler).
- Around line 287-312: The cleanup block currently unconditionally calls
cleanup_temp_file when resolved_input differs from request.query.input, which
can treat plain text as a file path; change it to only call cleanup_temp_file
when resolved_input is an audio-derived object created by the resolver (e.g.,
check isinstance(resolved_input, (AudioBase64Input, AudioUrlInput)) or
equivalent sentinel the resolver returns) and add imports for AudioBase64Input
and AudioUrlInput from app.models.llm.request so the isinstance check resolves;
keep the existing compare to request.query.input as an extra guard if desired.
In `@backend/app/services/llm/mappers.py`:
- Around line 67-73: The code sets
openai_params["tools"][...]["max_num_results"] using "max_num_results or 20",
which overwrites an explicit 0; change the assignment to preserve falsy but
valid values by using an explicit None check (e.g. use max_num_results if
max_num_results is not None else 20) when populating openai_params["tools"] so
that a passed 0 remains 0; update the block that builds the "tools" dict (the
code that references openai_params["tools"], "type": "file_search", and the
max_num_results key) accordingly.
In `@backend/app/services/llm/providers/gai.py`:
- Around line 22-44: Add missing type annotations and an explicit error path:
annotate GoogleAIProvider.__init__ with a return type of -> None, and update
_parse_input signature to include types (e.g., query_input: Any,
completion_type: str, provider: str) and keep -> str; change the function so
that if completion_type != "stt" it raises a ValueError (instead of implicitly
returning None), and keep the existing branch that returns the string when
query_input is str and raises when it's not (preserve the ValueError message
f"{provider} STT require file path").
- Around line 136-152: The execute method in gai.GAIProvider currently only
returns for completion_type == "stt" and falls through for "text" or "tts",
causing implicit None returns; update execute (method name execute, variable
completion_type) to explicitly return a (None, error_message) tuple for
unsupported/non-handled completion types (e.g., "text" and "tts") after the
existing if-block, using a clear error string (include completion_type) so
callers that do result, error = instance.execute(...) always get a tuple; keep
existing _execute_stt call unchanged.
In `@backend/app/services/llm/providers/registry.py`:
- Around line 97-140: Remove the ad-hoc test harness in the module (the entire
if __name__ == "__main__": block that constructs mock_credentials, calls
LLMProvider.get_provider_class, ProviderClass.create_client, instantiates
ProviderClass, builds NativeCompletionConfig and QueryParams, and calls
instance.execute); either delete it from registry.py or move it into a separate
test/debug script under tests/ or scripts/ and replace print calls with proper
logging in that script; ensure production code (registry.py and functions like
LLMProvider.get_provider_class, ProviderClass.create_client, and
instance.execute) contains no hardcoded paths, getenv usage, or console prints
left behind.
- Around line 67-72: The variable credential_provider is assigned from
provider_type.replace("-native", "") twice (duplicate lines); remove the
redundant assignment so credential_provider is set only once (keep the single
assignment and its explanatory comment) and ensure no other logic depends on the
duplicate; update the block where credential_provider and provider_type are
handled to contain a single assignment to credential_provider.
In `@backend/app/services/llm/providers/sai.py`:
- Around line 95-115: Guard against sarvam_response.transcript being None before
calling .split(): when constructing output_tokens_estimate and
LLMOutput(text=...), use a safe transcript variable (e.g., transcript =
sarvam_response.transcript or "") or conditional logic so you never call
.split() on None; then compute output_tokens_estimate = len(transcript.split())
and pass transcript into LLMOutput and provider_response_id fallback logic in
LLMResponse and Usage creation (LLMCallResponse, LLMResponse, Usage).
- Around line 22-25: Remove the hardcoded SARVAM_API_KEY and the print call:
stop setting SARVAM_API_KEY to a fallback secret and remove the stdout print;
instead, rely on os.getenv to provide the key and, if you want a warning in
SarvamAIProvider.__init__, call logger.warning(f"[SarvamAIProvider.__init__]
SARVAM_API_KEY not set") (use mask_string when including any partial secret)
rather than printing or embedding a real key in source; ensure no default secret
value remains anywhere in the module.
- Around line 30-38: The __init__ method in class SarvamAIProvider lacks an
explicit return type; update the SarvamAIProvider.__init__ signature to include
the return annotation "-> None" (i.e., def __init__(self, client: SarvamAI) ->
None:) to comply with the project's function return type guideline, keeping the
rest of the body (super().__init__(client); self.client = client) unchanged.
In `@backend/app/tests/services/llm/test_mappers.py`:
- Around line 1-317: The test module has been entirely commented out which
removes coverage; restore the tests by uncommenting the file and its imports and
test classes (references: KaapiLLMParams, KaapiCompletionConfig,
NativeCompletionConfig, map_kaapi_to_openai_params,
transform_kaapi_config_to_native) so they run normally, or if intentionally
disabled mark individual tests with pytest.mark.skip and add a TODO/link to a
tracked issue; ensure pytest is imported and any mocked tests (e.g., using
MagicMock) remain valid after re-enabling so the CI regains these checks.
In `@backend/app/tests/utils/test_data.py`:
- Around line 342-343: The code reads completion_config.get("type") into
config_type without a default which can lead to Pydantic validation errors for
NativeCompletionConfig/KaapiCompletionConfig; set a safe default (e.g., "text")
when reading the field so config_type = completion_config.get("type", "text")
(or equivalent) before passing into NativeCompletionConfig/KaapiCompletionConfig
construction to ensure the required "type" is always present and one of
"text"/"stt"/"tts".
🧹 Nitpick comments (15)
backend/app/tests/utils/test_data.py (2)
360-373: Provider value may not match Kaapi Literal types.The else branch assumes any provider not in
["openai-native", "google-native"]is a valid Kaapi provider, butKaapiCompletionConfig.providerisLiteral["openai", "google"]. Ifproviderhas an unexpected value (e.g.,"sarvam"or a typo), this will fail Pydantic validation.Consider adding explicit validation or a mapping for edge cases.
💡 Optional: Add explicit provider handling
else: # For Kaapi providers (openai, google) + kaapi_provider = provider if provider in ["openai", "google"] else "openai" config_blob = ConfigBlob( completion=KaapiCompletionConfig( - provider=provider, + provider=kaapi_provider, type=config_type, params={
321-337: Imports inside function body.Moving
select,and_, andConfigVersionimports inside the function works but deviates from typical module-level imports. This is acceptable in test utilities to avoid circular imports, but ensure this pattern is intentional.backend/app/celery/worker.py (2)
14-18: Consider usingOptional[int]for the type hint.The parameter
concurrency: int = Noneis technically incorrect sinceNoneis not anint. For clarity and type checker compliance, consider usingOptional[int].Suggested fix
+from typing import Optional + def start_worker( queues: str = "default,high_priority,low_priority,cron", - concurrency: int = None, + concurrency: Optional[int] = None, loglevel: str = "info", ):
30-31: Log messages should follow the[function_name]prefix pattern.As per coding guidelines, log messages should be prefixed with the function name in square brackets.
Suggested fix
- logger.info(f"Starting Celery worker with {concurrency} processes") - logger.info(f"Consuming queues: {queues}") + logger.info(f"[start_worker] Starting Celery worker with {concurrency} processes") + logger.info(f"[start_worker] Consuming queues: {queues}")backend/app/celery/beat.py (1)
6-6: Unused import:Celeryis imported but never used.The
Celeryclass is imported but onlycelery_app(from line 7) is used in this module.Suggested fix
import logging -from celery import Celery from app.celery.celery_app import celery_appbackend/app/alembic/versions/043_remove_enum_checks_llm_call_provider.py (1)
20-21: Add return type hints toupgrade()anddowngrade()functions.Per coding guidelines, all functions should have type hints. Alembic migration functions should return
None.Suggested fix
-def upgrade(): +def upgrade() -> None: # ### commands auto generated by Alembic - please adjust! ###-def downgrade(): +def downgrade() -> None: # ### commands auto generated by Alembic - please adjust! ###Also applies to: 33-34
backend/app/tests/seed_data/seed_data.py (1)
21-22: Unused imports:ConfigandConfigVersionare imported but never used.These models are imported but not referenced anywhere in the file. If they were added for future use or to ensure model registration, consider adding a comment explaining the intent. Otherwise, they should be removed to keep imports clean.
🧹 Proposed fix to remove unused imports
from app.models import ( APIKey, Organization, Project, User, Credential, Assistant, Document, - Config, - ConfigVersion, )backend/app/models/config/version.py (1)
99-116: Consider adding validation for emptyconfig_blob.Unlike
ConfigVersionBasewhich hasvalidate_blob_not_empty, this model allows an emptyconfig_blob. While this might be intentional (to allow creating a new version with only acommit_messagechange), it could lead to confusion. If empty blobs should be rejected, add similar validation:from pydantic import field_validator `@field_validator`("config_blob") def validate_blob_not_empty(cls, value): if not value: raise ValueError("config_blob cannot be empty") return valueIf allowing empty blobs is intentional (inheriting everything from the previous version), consider documenting this behavior explicitly in the docstring.
backend/app/tests/crud/test_llm.py (1)
42-46: Add return type hint to fixture.The
test_jobfixture is missing a return type annotation. Per coding guidelines, all function parameters and return values should have type hints.Proposed fix
`@pytest.fixture` -def test_job(db: Session): +def test_job(db: Session) -> "Job": """Create a test job for LLM call tests.""" crud = JobCrud(db) return crud.create(job_type=JobType.LLM_API, trace_id="test-llm-trace")You'll need to import
Jobfromapp.modelsor use a string annotation if there are circular import concerns.backend/app/tests/api/routes/configs/test_version.py (1)
471-519: Remove unused import.
TextLLMParamsis imported on line 477 but never used in this test function.Proposed fix
def test_create_version_cannot_change_type_from_text_to_stt( db: Session, client: TestClient, user_api_key: TestAuthContext, ) -> None: """Test that config type cannot be changed from 'text' to 'stt' in a new version.""" - from app.models.llm.request import KaapiCompletionConfig, TextLLMParams + from app.models.llm.request import KaapiCompletionConfigbackend/app/services/llm/providers/registry.py (2)
93-94: Log message should include function name prefix.Per coding guidelines, log messages should be prefixed with the function name in square brackets.
Proposed fix
- logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True) + logger.error(f"[get_llm_provider] Failed to initialize {provider_type} client: {e}", exc_info=True)
13-26: Review temporary imports before merging.The comment on line 15 indicates these are "temporary imports". Several imports appear unused in the production code path (
GenerateContentConfig,LLMCallResponse,LLMOutput,LLMResponse,Usage) and are only used by the ad hoc testing block.Once the testing code is removed, these imports should also be cleaned up along with
load_dotenv()andos.Expected cleanup after removing test code
-import os -from dotenv import load_dotenv import logging from sqlmodel import Session -from openai import OpenAI from app.crud import get_provider_credential from app.services.llm.providers.base import BaseProvider from app.services.llm.providers.oai import OpenAIProvider from app.services.llm.providers.gai import GoogleAIProvider -from google.genai.types import GenerateContentConfig - -# temporary import - -from app.models.llm import ( - NativeCompletionConfig, - LLMCallResponse, - QueryParams, - LLMOutput, - LLMResponse, - Usage, -) - -load_dotenv() logger = logging.getLogger(__name__)backend/app/services/llm/input_resolver.py (1)
86-111: Consider streaming for large audio files and safer error handling.Two potential improvements:
Memory usage:
response.contentloads the entire file into memory. For large audio files, this could cause memory issues. Consider usingstream=Truewith chunked writing.HTTPError safety: On line 94,
e.responsecould theoretically beNonein edge cases, though this is rare withraise_for_status().Proposed fix with streaming support
def resolve_audio_url(url: str) -> tuple[str, str | None]: """Fetch audio from URL and write to temp file. Returns (file_path, error).""" try: - response = requests.get(url, timeout=60) + response = requests.get(url, timeout=60, stream=True) response.raise_for_status() except requests.Timeout: return "", f"Timeout fetching audio from URL: {url}" except requests.HTTPError as e: - return "", f"HTTP error fetching audio: {e.response.status_code}" + status = e.response.status_code if e.response else "unknown" + return "", f"HTTP error fetching audio: {status}" except Exception as e: return "", f"Failed to fetch audio from URL: {str(e)}" content_type = response.headers.get("content-type", "audio/wav") ext = get_file_extension(content_type.split(";")[0].strip()) try: with tempfile.NamedTemporaryFile( suffix=ext, delete=False, prefix="audio_" ) as tmp: - tmp.write(response.content) + for chunk in response.iter_content(chunk_size=8192): + if chunk: + tmp.write(chunk) temp_path = tmp.name logger.info(f"[resolve_audio_url] Wrote audio to temp file: {temp_path}") return temp_path, None except Exception as e: return "", f"Failed to write fetched audio to temp file: {str(e)}"backend/app/services/llm/jobs.py (1)
161-178: Consider gating the debug job dump.Fetching and logging recent jobs on a miss can be noisy in production; consider a debug flag or removal once the issue is resolved.
backend/app/tests/services/llm/providers/test_gai.py (1)
52-81: Add fixture type hints and consider factory-style fixtures.Type hints keep tests consistent with typing guidance, and factory fixtures improve reuse when variations are needed.
As per coding guidelines: **/*.py: Always add type hints to all function parameters and return values in Python code; backend/app/tests/**/*.py: Use factory pattern for test fixtures in backend/app/tests/.✍️ Example adjustment
- def mock_client(self): + def mock_client(self) -> MagicMock: @@ - def provider(self, mock_client): + def provider(self, mock_client: MagicMock) -> GoogleAIProvider: @@ - def stt_config(self): + def stt_config(self) -> NativeCompletionConfig: @@ - def query_params(self): + def query_params(self) -> QueryParams:
| """add_llm_call_table | ||
|
|
||
| Revision ID: 041 | ||
| Revises: 040 | ||
| Create Date: 2026-01-26 15:20:23.873332 | ||
|
|
||
| """ | ||
| from alembic import op | ||
| import sqlalchemy as sa | ||
| import sqlmodel.sql.sqltypes | ||
| from sqlalchemy.dialects import postgresql | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision = "042" | ||
| down_revision = "041" | ||
| branch_labels = None |
There was a problem hiding this comment.
Fix the revision header to match the revision variables.
The header says “Revision ID: 041” and “Revises: 040” while the actual revision = "042" and down_revision = "041". This mismatch is confusing and suggests the migration wasn’t generated with the expected rev‑id.
🔧 Suggested header fix
-Revision ID: 041
-Revises: 040
+Revision ID: 042
+Revises: 041Based on learnings: Generate database migrations using alembic revision --autogenerate -m "Description" --rev-id <number> where rev-id is the latest existing revision ID + 1.
📝 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.
| """add_llm_call_table | |
| Revision ID: 041 | |
| Revises: 040 | |
| Create Date: 2026-01-26 15:20:23.873332 | |
| """ | |
| from alembic import op | |
| import sqlalchemy as sa | |
| import sqlmodel.sql.sqltypes | |
| from sqlalchemy.dialects import postgresql | |
| # revision identifiers, used by Alembic. | |
| revision = "042" | |
| down_revision = "041" | |
| branch_labels = None | |
| """add_llm_call_table | |
| Revision ID: 042 | |
| Revises: 041 | |
| Create Date: 2026-01-26 15:20:23.873332 | |
| """ | |
| from alembic import op | |
| import sqlalchemy as sa | |
| import sqlmodel.sql.sqltypes | |
| from sqlalchemy.dialects import postgresql | |
| # revision identifiers, used by Alembic. | |
| revision = "042" | |
| down_revision = "041" | |
| branch_labels = None |
🤖 Prompt for AI Agents
In `@backend/app/alembic/versions/042_add_llm_call_table.py` around lines 1 - 16,
Update the module header docstring so the human-readable Revision ID and Revises
lines match the actual alembic variables: change the header "Revision ID: 041"
to "Revision ID: 042" and "Revises: 040" to "Revises: 041" to align with the
`revision = "042"` and `down_revision = "041"` values in this migration file
(look for the top-of-file docstring and the `revision`/`down_revision` symbols).
| def upgrade(): | ||
| # ### commands auto generated by Alembic - please adjust! ### |
There was a problem hiding this comment.
Add return type hints for upgrade and downgrade.
These functions should be explicitly annotated to satisfy the project’s typing requirement.
✅ 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: 164-165
🤖 Prompt for AI Agents
In `@backend/app/alembic/versions/042_add_llm_call_table.py` around lines 20 - 21,
Add explicit return type annotations to the Alembic migration functions:
annotate both upgrade() and downgrade() with -> None (e.g., def upgrade() ->
None: and def downgrade() -> None:) so they satisfy the project's typing
requirements; update the function signatures where upgrade and downgrade are
defined in this migration file.
| def upgrade(): | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.alter_column( | ||
| "llm_call", | ||
| "provider", | ||
| existing_type=sa.VARCHAR(), | ||
| comment="AI provider as sent by user (e.g openai, -native, google)", | ||
| existing_comment="AI provider: openai, google, anthropic", | ||
| existing_nullable=False, | ||
| ) | ||
| # ### end Alembic commands ### |
There was a problem hiding this comment.
Potential typo in the column comment: -native may be incomplete.
The comment "AI provider as sent by user (e.g openai, -native, google)" contains -native which appears incomplete. Should this be openai-native or google-native based on the provider naming convention?
Suggested fix (if `openai-native` was intended)
op.alter_column(
"llm_call",
"provider",
existing_type=sa.VARCHAR(),
- comment="AI provider as sent by user (e.g openai, -native, google)",
+ comment="AI provider as sent by user (e.g. openai, openai-native, google)",
existing_comment="AI provider: openai, google, anthropic",
existing_nullable=False,
)🤖 Prompt for AI Agents
In `@backend/app/alembic/versions/043_remove_enum_checks_llm_call_provider.py`
around lines 20 - 30, The column comment on the "provider" column in the
upgrade() migration contains a likely typo ("AI provider as sent by user (e.g
openai, -native, google)"); update the comment to the correct provider token
(e.g. "AI provider as sent by user (e.g. openai, openai-native, google)" or
whichever canonical name is used) inside the op.alter_column call in upgrade(),
ensuring the existing_comment/existing_type/nullable parameters remain unchanged
and only the comment string is corrected.
| """ | ||
| Create a new version for an existing configuration. | ||
| The version number is automatically incremented. | ||
|
|
||
| Only include the fields you want to update in config_blob. | ||
| Provider, model, and params can be changed. | ||
| Type is inherited from existing config and cannot be changed. | ||
| """ | ||
| version_crud = ConfigVersionCrud( | ||
| session=session, project_id=current_user.project_.id, config_id=config_id | ||
| ) | ||
| version = version_crud.create_or_raise(version_create=version_create) | ||
| version = version_crud.create_from_partial_or_raise(version_create=version_create) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find _validate_immutable_fields implementation
rg -n -A 25 "_validate_immutable_fields" --type=py backend/Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 4079
🏁 Script executed:
# Find ConfigVersionCreatePartial model definition
rg -n -B 5 -A 15 "class ConfigVersionCreatePartial" --type=py backend/Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1662
🏁 Script executed:
# Check the version.py file to understand the context better
cat -n backend/app/api/routes/config/version.py | head -50Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1966
Correct the ConfigVersionCreatePartial model docstring to accurately reflect immutable fields.
The route docstring correctly states "Provider, model, and params can be changed. Type is inherited from existing config and cannot be changed," which matches the actual behavior in _validate_immutable_fields (which only validates type immutability). However, the ConfigVersionCreatePartial model docstring in backend/app/models/config/version.py incorrectly claims that both "provider and type are inherited... and cannot be changed" when in fact only type is immutable. Update the model docstring to reflect that provider and type have different mutability constraints.
🤖 Prompt for AI Agents
In `@backend/app/api/routes/config/version.py` around lines 31 - 41, Update the
ConfigVersionCreatePartial model docstring in
backend/app/models/config/version.py to correctly state immutability: change the
incorrect sentence that says "provider and type are inherited... and cannot be
changed" to reflect that only "type is inherited from the existing config and
cannot be changed," while "provider, model, and params can be changed"; ensure
the docstring on the ConfigVersionCreatePartial class matches the route
docstring and the behavior enforced by _validate_immutable_fields.
| logger.info( | ||
| f"[create_llm_call] Created LLM call id={db_llm_call.id}, " | ||
| f"job_id={job_id}, provider={provider}, model={model}" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/app/crud/llm.py | head -160 | tail -30Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1020
🏁 Script executed:
cat -n backend/app/crud/llm.py | head -130 | tail -50Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1983
🏁 Script executed:
cat -n backend/app/crud/llm.py | head -80 | tail -50Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1859
Fix undefined provider in log message.
The log message references an undefined provider variable. The function parameter is original_provider (line 65), which should be used instead. This will cause a NameError at runtime when the log is executed, even though the database write succeeds.
Suggested fix
- f"[create_llm_call] Created LLM call id={db_llm_call.id}, "
- f"job_id={job_id}, provider={provider}, model={model}"
+ f"[create_llm_call] Created LLM call id={db_llm_call.id}, "
+ f"job_id={job_id}, provider={original_provider}, model={model}"🧰 Tools
🪛 Ruff (0.14.14)
[error] 145-145: Undefined name provider
(F821)
🤖 Prompt for AI Agents
In `@backend/app/crud/llm.py` around lines 143 - 146, The log statement in
create_llm_call uses an undefined variable provider which will raise a
NameError; update the logger.info call that logs db_llm_call.id, job_id,
provider, model to use the function parameter original_provider instead of
provider (i.e., reference original_provider in the message) so the runtime uses
the correct variable and the log prints the provider value.
| SARVAM_API_KEY = os.getenv("SARVAM_API_KEY") | ||
| if not SARVAM_API_KEY: | ||
| SARVAM_API_KEY = "sk_lmsvfc31_On1bxqwDAqYZoijqBfblr3yf" # for testing only | ||
| print("Requested Action: Please set SARVAM_API_KEY , Going ahead with a trail key for testing purposes.") |
There was a problem hiding this comment.
Remove hardcoded Sarvam API key and stdout print.
Hardcoded secrets can leak and violate security expectations; rely on credential injection instead. If you want a warning, log via logger.warning with a [SarvamAIProvider.__init__] prefix.
🔐 Suggested fix
-SARVAM_API_KEY = os.getenv("SARVAM_API_KEY")
-if not SARVAM_API_KEY:
- SARVAM_API_KEY = "sk_lmsvfc31_On1bxqwDAqYZoijqBfblr3yf" # for testing only
- print("Requested Action: Please set SARVAM_API_KEY , Going ahead with a trail key for testing purposes.")🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/sai.py` around lines 22 - 25, Remove the
hardcoded SARVAM_API_KEY and the print call: stop setting SARVAM_API_KEY to a
fallback secret and remove the stdout print; instead, rely on os.getenv to
provide the key and, if you want a warning in SarvamAIProvider.__init__, call
logger.warning(f"[SarvamAIProvider.__init__] SARVAM_API_KEY not set") (use
mask_string when including any partial secret) rather than printing or embedding
a real key in source; ensure no default secret value remains anywhere in the
module.
| class SarvamAIProvider(BaseProvider): | ||
| def __init__(self, client: SarvamAI): | ||
| """Initialize SarvamAI provider with client. | ||
|
|
||
| Args: | ||
| client: SarvamAI client instance | ||
| """ | ||
| super().__init__(client) | ||
| self.client = client |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "sai.py" -path "*/llm/providers/*"Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 118
🏁 Script executed:
cat -n backend/app/services/llm/providers/sai.py | head -50Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1824
🏁 Script executed:
rg "def __init__" backend/app/services/llm/providers/ -A 3 -B 1Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1468
🏁 Script executed:
cat -n backend/app/services/llm/providers/sai.py | sed -n '40,100p'Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 3220
🏁 Script executed:
rg "def " backend/app/services/llm/providers/sai.py -A 1 | head -40Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 497
🏁 Script executed:
cat -n backend/app/services/llm/providers/base.py | head -40Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1593
🏁 Script executed:
python3 << 'EOF'
# Check if __init__ methods in provider files have return type hints
import re
files_to_check = [
"backend/app/services/llm/providers/sai.py",
"backend/app/services/llm/providers/oai.py",
"backend/app/services/llm/providers/gai.py",
"backend/app/services/llm/providers/base.py"
]
for filepath in files_to_check:
try:
with open(filepath, 'r') as f:
content = f.read()
# Find __init__ method
init_match = re.search(r'def __init__\([^)]*\)(\s*->.*)?:', content)
if init_match:
has_return_type = ' -> ' in (init_match.group(1) or '')
status = "✓ has return type" if has_return_type else "✗ missing return type"
print(f"{filepath}: {status}")
print(f" {init_match.group(0)}")
except Exception as e:
print(f"Error reading {filepath}: {e}")
EOFRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 492
Add explicit return type on __init__.
The __init__ method is missing the -> None return type annotation, which is required by the coding guideline for all function parameters and return values.
✍️ Suggested fix
- def __init__(self, client: SarvamAI):
+ def __init__(self, client: SarvamAI) -> None:🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/sai.py` around lines 30 - 38, The __init__
method in class SarvamAIProvider lacks an explicit return type; update the
SarvamAIProvider.__init__ signature to include the return annotation "-> None"
(i.e., def __init__(self, client: SarvamAI) -> None:) to comply with the
project's function return type guideline, keeping the rest of the body
(super().__init__(client); self.client = client) unchanged.
| # SarvamAI does not provide token usage directly for STT, so we'll use placeholders | ||
| # You might estimate based on transcript length or set to 0 | ||
| input_tokens_estimate = 0 # Not directly provided by SarvamAI STT | ||
| output_tokens_estimate = len(sarvam_response.transcript.split()) # Estimate by word count | ||
| total_tokens_estimate = input_tokens_estimate + output_tokens_estimate | ||
|
|
||
| llm_response = LLMCallResponse( | ||
| response=LLMResponse( | ||
| provider_response_id=sarvam_response.request_id or "unknown", | ||
| conversation_id=None, # SarvamAI STT doesn't have conversation_id | ||
| provider=provider_name, | ||
| model=model, | ||
| output=LLMOutput(text=sarvam_response.transcript or ""), | ||
| ), | ||
| usage=Usage( | ||
| input_tokens=input_tokens_estimate, | ||
| output_tokens=output_tokens_estimate, | ||
| total_tokens=total_tokens_estimate, | ||
| reasoning_tokens=None, # Not provided by SarvamAI | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Guard against missing transcript before .split().
If sarvam_response.transcript is None, this raises AttributeError and fails the request.
🐛 Suggested fix
- input_tokens_estimate = 0 # Not directly provided by SarvamAI STT
- output_tokens_estimate = len(sarvam_response.transcript.split()) # Estimate by word count
+ transcript = sarvam_response.transcript or ""
+ input_tokens_estimate = 0 # Not directly provided by SarvamAI STT
+ output_tokens_estimate = len(transcript.split()) # Estimate by word count
total_tokens_estimate = input_tokens_estimate + output_tokens_estimate
@@
- output=LLMOutput(text=sarvam_response.transcript or ""),
+ output=LLMOutput(text=transcript),📝 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.
| # SarvamAI does not provide token usage directly for STT, so we'll use placeholders | |
| # You might estimate based on transcript length or set to 0 | |
| input_tokens_estimate = 0 # Not directly provided by SarvamAI STT | |
| output_tokens_estimate = len(sarvam_response.transcript.split()) # Estimate by word count | |
| total_tokens_estimate = input_tokens_estimate + output_tokens_estimate | |
| llm_response = LLMCallResponse( | |
| response=LLMResponse( | |
| provider_response_id=sarvam_response.request_id or "unknown", | |
| conversation_id=None, # SarvamAI STT doesn't have conversation_id | |
| provider=provider_name, | |
| model=model, | |
| output=LLMOutput(text=sarvam_response.transcript or ""), | |
| ), | |
| usage=Usage( | |
| input_tokens=input_tokens_estimate, | |
| output_tokens=output_tokens_estimate, | |
| total_tokens=total_tokens_estimate, | |
| reasoning_tokens=None, # Not provided by SarvamAI | |
| ), | |
| ) | |
| # SarvamAI does not provide token usage directly for STT, so we'll use placeholders | |
| # You might estimate based on transcript length or set to 0 | |
| transcript = sarvam_response.transcript or "" | |
| input_tokens_estimate = 0 # Not directly provided by SarvamAI STT | |
| output_tokens_estimate = len(transcript.split()) # Estimate by word count | |
| total_tokens_estimate = input_tokens_estimate + output_tokens_estimate | |
| llm_response = LLMCallResponse( | |
| response=LLMResponse( | |
| provider_response_id=sarvam_response.request_id or "unknown", | |
| conversation_id=None, # SarvamAI STT doesn't have conversation_id | |
| provider=provider_name, | |
| model=model, | |
| output=LLMOutput(text=transcript), | |
| ), | |
| usage=Usage( | |
| input_tokens=input_tokens_estimate, | |
| output_tokens=output_tokens_estimate, | |
| total_tokens=total_tokens_estimate, | |
| reasoning_tokens=None, # Not provided by SarvamAI | |
| ), | |
| ) |
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/sai.py` around lines 95 - 115, Guard
against sarvam_response.transcript being None before calling .split(): when
constructing output_tokens_estimate and LLMOutput(text=...), use a safe
transcript variable (e.g., transcript = sarvam_response.transcript or "") or
conditional logic so you never call .split() on None; then compute
output_tokens_estimate = len(transcript.split()) and pass transcript into
LLMOutput and provider_response_id fallback logic in LLMResponse and Usage
creation (LLMCallResponse, LLMResponse, Usage).
| # """ | ||
| # Unit tests for LLM parameter mapping functions. | ||
|
|
||
| from app.models.llm import KaapiLLMParams, KaapiCompletionConfig, NativeCompletionConfig | ||
| from app.services.llm.mappers import ( | ||
| map_kaapi_to_openai_params, | ||
| transform_kaapi_config_to_native, | ||
| ) | ||
|
|
||
|
|
||
| class TestMapKaapiToOpenAIParams: | ||
| """Test cases for map_kaapi_to_openai_params function.""" | ||
|
|
||
| def test_basic_model_mapping(self): | ||
| """Test basic model parameter mapping.""" | ||
| kaapi_params = KaapiLLMParams(model="gpt-4o") | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result == {"model": "gpt-4o"} | ||
| assert warnings == [] | ||
|
|
||
| def test_instructions_mapping(self): | ||
| """Test instructions parameter mapping.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="gpt-4", | ||
| instructions="You are a helpful assistant.", | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "gpt-4" | ||
| assert result["instructions"] == "You are a helpful assistant." | ||
| assert warnings == [] | ||
|
|
||
| def test_temperature_mapping(self): | ||
| """Test temperature parameter mapping for non-reasoning models.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="gpt-4", | ||
| temperature=0.7, | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "gpt-4" | ||
| assert result["temperature"] == 0.7 | ||
| assert warnings == [] | ||
|
|
||
| def test_temperature_zero_mapping(self): | ||
| """Test that temperature=0 is correctly mapped (edge case).""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="gpt-4", | ||
| temperature=0.0, | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["temperature"] == 0.0 | ||
| assert warnings == [] | ||
|
|
||
| def test_reasoning_mapping_for_reasoning_models(self): | ||
| """Test reasoning parameter mapping to OpenAI format for reasoning-capable models.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="o1", | ||
| reasoning="high", | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "o1" | ||
| assert result["reasoning"] == {"effort": "high"} | ||
| assert warnings == [] | ||
|
|
||
| def test_knowledge_base_ids_mapping(self): | ||
| """Test knowledge_base_ids mapping to OpenAI tools format.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="gpt-4", | ||
| knowledge_base_ids=["vs_abc123", "vs_def456"], | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "gpt-4" | ||
| assert "tools" in result | ||
| assert len(result["tools"]) == 1 | ||
| assert result["tools"][0]["type"] == "file_search" | ||
| assert result["tools"][0]["vector_store_ids"] == ["vs_abc123", "vs_def456"] | ||
| assert result["tools"][0]["max_num_results"] == 20 # default | ||
| assert warnings == [] | ||
|
|
||
| def test_knowledge_base_with_max_num_results(self): | ||
| """Test knowledge_base_ids with custom max_num_results.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="gpt-4", | ||
| knowledge_base_ids=["vs_abc123"], | ||
| max_num_results=50, | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["tools"][0]["max_num_results"] == 50 | ||
| assert warnings == [] | ||
|
|
||
| def test_complete_parameter_mapping(self): | ||
| """Test mapping all compatible parameters together.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="gpt-4o", | ||
| instructions="You are an expert assistant.", | ||
| temperature=0.8, | ||
| knowledge_base_ids=["vs_123"], | ||
| max_num_results=30, | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "gpt-4o" | ||
| assert result["instructions"] == "You are an expert assistant." | ||
| assert result["temperature"] == 0.8 | ||
| assert result["tools"][0]["type"] == "file_search" | ||
| assert result["tools"][0]["vector_store_ids"] == ["vs_123"] | ||
| assert result["tools"][0]["max_num_results"] == 30 | ||
| assert warnings == [] | ||
|
|
||
| def test_reasoning_suppressed_for_non_reasoning_models(self): | ||
| """Test that reasoning is suppressed with warning for non-reasoning models.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="gpt-4", | ||
| reasoning="high", | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "gpt-4" | ||
| assert "reasoning" not in result | ||
| assert len(warnings) == 1 | ||
| assert "reasoning" in warnings[0].lower() | ||
| assert "does not support reasoning" in warnings[0] | ||
|
|
||
| def test_temperature_suppressed_for_reasoning_models(self): | ||
| """Test that temperature is suppressed with warning for reasoning models when reasoning is set.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="o1", | ||
| temperature=0.7, | ||
| reasoning="high", | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "o1" | ||
| assert result["reasoning"] == {"effort": "high"} | ||
| assert "temperature" not in result | ||
| assert len(warnings) == 1 | ||
| assert "temperature" in warnings[0].lower() | ||
| assert "suppressed" in warnings[0] | ||
|
|
||
| def test_temperature_without_reasoning_for_reasoning_models(self): | ||
| """Test that temperature is suppressed for reasoning models even without explicit reasoning parameter.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="o1", | ||
| temperature=0.7, | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "o1" | ||
| assert "temperature" not in result | ||
| assert "reasoning" not in result | ||
| assert len(warnings) == 1 | ||
| assert "temperature" in warnings[0].lower() | ||
| assert "suppressed" in warnings[0] | ||
|
|
||
| def test_minimal_params(self): | ||
| """Test mapping with minimal parameters (only model).""" | ||
| kaapi_params = KaapiLLMParams(model="gpt-4") | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result == {"model": "gpt-4"} | ||
| assert warnings == [] | ||
|
|
||
| def test_only_knowledge_base_ids(self): | ||
| """Test mapping with only knowledge_base_ids and model.""" | ||
| kaapi_params = KaapiLLMParams( | ||
| model="gpt-4", | ||
| knowledge_base_ids=["vs_xyz"], | ||
| ) | ||
|
|
||
| result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| assert result["model"] == "gpt-4" | ||
| assert "tools" in result | ||
| assert result["tools"][0]["vector_store_ids"] == ["vs_xyz"] | ||
| assert warnings == [] | ||
|
|
||
|
|
||
| class TestTransformKaapiConfigToNative: | ||
| """Test cases for transform_kaapi_config_to_native function.""" | ||
|
|
||
| def test_transform_openai_config(self): | ||
| """Test transformation of Kaapi OpenAI config to native format.""" | ||
| kaapi_config = KaapiCompletionConfig( | ||
| provider="openai", | ||
| params=KaapiLLMParams( | ||
| model="gpt-4", | ||
| temperature=0.7, | ||
| ), | ||
| ) | ||
|
|
||
| result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| assert isinstance(result, NativeCompletionConfig) | ||
| assert result.provider == "openai-native" | ||
| assert result.params["model"] == "gpt-4" | ||
| assert result.params["temperature"] == 0.7 | ||
| assert warnings == [] | ||
|
|
||
| def test_transform_with_all_params(self): | ||
| """Test transformation with all Kaapi parameters.""" | ||
| kaapi_config = KaapiCompletionConfig( | ||
| provider="openai", | ||
| params=KaapiLLMParams( | ||
| model="gpt-4o", | ||
| instructions="System prompt here", | ||
| temperature=0.5, | ||
| knowledge_base_ids=["vs_abc"], | ||
| max_num_results=25, | ||
| ), | ||
| ) | ||
|
|
||
| result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| assert result.provider == "openai-native" | ||
| assert result.params["model"] == "gpt-4o" | ||
| assert result.params["instructions"] == "System prompt here" | ||
| assert result.params["temperature"] == 0.5 | ||
| assert result.params["tools"][0]["type"] == "file_search" | ||
| assert result.params["tools"][0]["max_num_results"] == 25 | ||
| assert warnings == [] | ||
|
|
||
| def test_transform_with_reasoning(self): | ||
| """Test transformation with reasoning parameter for reasoning-capable models.""" | ||
| kaapi_config = KaapiCompletionConfig( | ||
| provider="openai", | ||
| params=KaapiLLMParams( | ||
| model="o1", | ||
| reasoning="medium", | ||
| ), | ||
| ) | ||
|
|
||
| result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| assert result.provider == "openai-native" | ||
| assert result.params["model"] == "o1" | ||
| assert result.params["reasoning"] == {"effort": "medium"} | ||
| assert warnings == [] | ||
|
|
||
| def test_transform_with_both_temperature_and_reasoning(self): | ||
| """Test that transformation handles temperature + reasoning intelligently for reasoning models.""" | ||
| kaapi_config = KaapiCompletionConfig( | ||
| provider="openai", | ||
| params=KaapiLLMParams( | ||
| model="o1", | ||
| temperature=0.7, | ||
| reasoning="high", | ||
| ), | ||
| ) | ||
|
|
||
| result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| assert result.provider == "openai-native" | ||
| assert result.params["model"] == "o1" | ||
| assert result.params["reasoning"] == {"effort": "high"} | ||
| assert "temperature" not in result.params | ||
| assert len(warnings) == 1 | ||
| assert "temperature" in warnings[0].lower() | ||
| assert "suppressed" in warnings[0] | ||
|
|
||
| def test_unsupported_provider_raises_error(self): | ||
| """Test that unsupported providers raise ValueError.""" | ||
| # Note: This would require modifying KaapiCompletionConfig to accept other providers | ||
| # For now, this tests the error handling in the mapper | ||
| # We'll create a mock config that bypasses validation | ||
| from unittest.mock import MagicMock | ||
|
|
||
| mock_config = MagicMock() | ||
| mock_config.provider = "unsupported-provider" | ||
| mock_config.params = KaapiLLMParams(model="some-model") | ||
|
|
||
| with pytest.raises(ValueError) as exc_info: | ||
| transform_kaapi_config_to_native(mock_config) | ||
|
|
||
| assert "Unsupported provider" in str(exc_info.value) | ||
|
|
||
| def test_transform_preserves_param_structure(self): | ||
| """Test that transformation correctly structures nested parameters.""" | ||
| kaapi_config = KaapiCompletionConfig( | ||
| provider="openai", | ||
| params=KaapiLLMParams( | ||
| model="gpt-4", | ||
| knowledge_base_ids=["vs_1", "vs_2", "vs_3"], | ||
| max_num_results=15, | ||
| ), | ||
| ) | ||
|
|
||
| result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| # Verify the nested structure is correct | ||
| assert isinstance(result.params["tools"], list) | ||
| assert isinstance(result.params["tools"][0], dict) | ||
| assert isinstance(result.params["tools"][0]["vector_store_ids"], list) | ||
| assert len(result.params["tools"][0]["vector_store_ids"]) == 3 | ||
| assert warnings == [] | ||
| # Tests the transformation of Kaapi-abstracted parameters to provider-native formats. | ||
| # """ | ||
|
|
||
| # import pytest | ||
|
|
||
| # from app.models.llm import KaapiLLMParams, KaapiCompletionConfig, NativeCompletionConfig | ||
| # from app.services.llm.mappers import ( | ||
| # map_kaapi_to_openai_params, | ||
| # transform_kaapi_config_to_native, | ||
| # ) | ||
|
|
||
|
|
||
| # class TestMapKaapiToOpenAIParams: | ||
| # """Test cases for map_kaapi_to_openai_params function.""" | ||
|
|
||
| # def test_basic_model_mapping(self): | ||
| # """Test basic model parameter mapping.""" | ||
| # kaapi_params = KaapiLLMParams(model="gpt-4o") | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result == {"model": "gpt-4o"} | ||
| # assert warnings == [] | ||
|
|
||
| # def test_instructions_mapping(self): | ||
| # """Test instructions parameter mapping.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # instructions="You are a helpful assistant.", | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "gpt-4" | ||
| # assert result["instructions"] == "You are a helpful assistant." | ||
| # assert warnings == [] | ||
|
|
||
| # def test_temperature_mapping(self): | ||
| # """Test temperature parameter mapping for non-reasoning models.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # temperature=0.7, | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "gpt-4" | ||
| # assert result["temperature"] == 0.7 | ||
| # assert warnings == [] | ||
|
|
||
| # def test_temperature_zero_mapping(self): | ||
| # """Test that temperature=0 is correctly mapped (edge case).""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # temperature=0.0, | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["temperature"] == 0.0 | ||
| # assert warnings == [] | ||
|
|
||
| # def test_reasoning_mapping_for_reasoning_models(self): | ||
| # """Test reasoning parameter mapping to OpenAI format for reasoning-capable models.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="o1", | ||
| # reasoning="high", | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "o1" | ||
| # assert result["reasoning"] == {"effort": "high"} | ||
| # assert warnings == [] | ||
|
|
||
| # def test_knowledge_base_ids_mapping(self): | ||
| # """Test knowledge_base_ids mapping to OpenAI tools format.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # knowledge_base_ids=["vs_abc123", "vs_def456"], | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "gpt-4" | ||
| # assert "tools" in result | ||
| # assert len(result["tools"]) == 1 | ||
| # assert result["tools"][0]["type"] == "file_search" | ||
| # assert result["tools"][0]["vector_store_ids"] == ["vs_abc123", "vs_def456"] | ||
| # assert result["tools"][0]["max_num_results"] == 20 # default | ||
| # assert warnings == [] | ||
|
|
||
| # def test_knowledge_base_with_max_num_results(self): | ||
| # """Test knowledge_base_ids with custom max_num_results.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # knowledge_base_ids=["vs_abc123"], | ||
| # max_num_results=50, | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["tools"][0]["max_num_results"] == 50 | ||
| # assert warnings == [] | ||
|
|
||
| # def test_complete_parameter_mapping(self): | ||
| # """Test mapping all compatible parameters together.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="gpt-4o", | ||
| # instructions="You are an expert assistant.", | ||
| # temperature=0.8, | ||
| # knowledge_base_ids=["vs_123"], | ||
| # max_num_results=30, | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "gpt-4o" | ||
| # assert result["instructions"] == "You are an expert assistant." | ||
| # assert result["temperature"] == 0.8 | ||
| # assert result["tools"][0]["type"] == "file_search" | ||
| # assert result["tools"][0]["vector_store_ids"] == ["vs_123"] | ||
| # assert result["tools"][0]["max_num_results"] == 30 | ||
| # assert warnings == [] | ||
|
|
||
| # def test_reasoning_suppressed_for_non_reasoning_models(self): | ||
| # """Test that reasoning is suppressed with warning for non-reasoning models.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # reasoning="high", | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "gpt-4" | ||
| # assert "reasoning" not in result | ||
| # assert len(warnings) == 1 | ||
| # assert "reasoning" in warnings[0].lower() | ||
| # assert "does not support reasoning" in warnings[0] | ||
|
|
||
| # def test_temperature_suppressed_for_reasoning_models(self): | ||
| # """Test that temperature is suppressed with warning for reasoning models when reasoning is set.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="o1", | ||
| # temperature=0.7, | ||
| # reasoning="high", | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "o1" | ||
| # assert result["reasoning"] == {"effort": "high"} | ||
| # assert "temperature" not in result | ||
| # assert len(warnings) == 1 | ||
| # assert "temperature" in warnings[0].lower() | ||
| # assert "suppressed" in warnings[0] | ||
|
|
||
| # def test_temperature_without_reasoning_for_reasoning_models(self): | ||
| # """Test that temperature is suppressed for reasoning models even without explicit reasoning parameter.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="o1", | ||
| # temperature=0.7, | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "o1" | ||
| # assert "temperature" not in result | ||
| # assert "reasoning" not in result | ||
| # assert len(warnings) == 1 | ||
| # assert "temperature" in warnings[0].lower() | ||
| # assert "suppressed" in warnings[0] | ||
|
|
||
| # def test_minimal_params(self): | ||
| # """Test mapping with minimal parameters (only model).""" | ||
| # kaapi_params = KaapiLLMParams(model="gpt-4") | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result == {"model": "gpt-4"} | ||
| # assert warnings == [] | ||
|
|
||
| # def test_only_knowledge_base_ids(self): | ||
| # """Test mapping with only knowledge_base_ids and model.""" | ||
| # kaapi_params = KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # knowledge_base_ids=["vs_xyz"], | ||
| # ) | ||
|
|
||
| # result, warnings = map_kaapi_to_openai_params(kaapi_params) | ||
|
|
||
| # assert result["model"] == "gpt-4" | ||
| # assert "tools" in result | ||
| # assert result["tools"][0]["vector_store_ids"] == ["vs_xyz"] | ||
| # assert warnings == [] | ||
|
|
||
|
|
||
| # class TestTransformKaapiConfigToNative: | ||
| # """Test cases for transform_kaapi_config_to_native function.""" | ||
|
|
||
| # def test_transform_openai_config(self): | ||
| # """Test transformation of Kaapi OpenAI config to native format.""" | ||
| # kaapi_config = KaapiCompletionConfig( | ||
| # provider="openai", | ||
| # params=KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # temperature=0.7, | ||
| # ), | ||
| # ) | ||
|
|
||
| # result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| # assert isinstance(result, NativeCompletionConfig) | ||
| # assert result.provider == "openai-native" | ||
| # assert result.params["model"] == "gpt-4" | ||
| # assert result.params["temperature"] == 0.7 | ||
| # assert warnings == [] | ||
|
|
||
| # def test_transform_with_all_params(self): | ||
| # """Test transformation with all Kaapi parameters.""" | ||
| # kaapi_config = KaapiCompletionConfig( | ||
| # provider="openai", | ||
| # params=KaapiLLMParams( | ||
| # model="gpt-4o", | ||
| # instructions="System prompt here", | ||
| # temperature=0.5, | ||
| # knowledge_base_ids=["vs_abc"], | ||
| # max_num_results=25, | ||
| # ), | ||
| # ) | ||
|
|
||
| # result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| # assert result.provider == "openai-native" | ||
| # assert result.params["model"] == "gpt-4o" | ||
| # assert result.params["instructions"] == "System prompt here" | ||
| # assert result.params["temperature"] == 0.5 | ||
| # assert result.params["tools"][0]["type"] == "file_search" | ||
| # assert result.params["tools"][0]["max_num_results"] == 25 | ||
| # assert warnings == [] | ||
|
|
||
| # def test_transform_with_reasoning(self): | ||
| # """Test transformation with reasoning parameter for reasoning-capable models.""" | ||
| # kaapi_config = KaapiCompletionConfig( | ||
| # provider="openai", | ||
| # params=KaapiLLMParams( | ||
| # model="o1", | ||
| # reasoning="medium", | ||
| # ), | ||
| # ) | ||
|
|
||
| # result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| # assert result.provider == "openai-native" | ||
| # assert result.params["model"] == "o1" | ||
| # assert result.params["reasoning"] == {"effort": "medium"} | ||
| # assert warnings == [] | ||
|
|
||
| # def test_transform_with_both_temperature_and_reasoning(self): | ||
| # """Test that transformation handles temperature + reasoning intelligently for reasoning models.""" | ||
| # kaapi_config = KaapiCompletionConfig( | ||
| # provider="openai", | ||
| # params=KaapiLLMParams( | ||
| # model="o1", | ||
| # temperature=0.7, | ||
| # reasoning="high", | ||
| # ), | ||
| # ) | ||
|
|
||
| # result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| # assert result.provider == "openai-native" | ||
| # assert result.params["model"] == "o1" | ||
| # assert result.params["reasoning"] == {"effort": "high"} | ||
| # assert "temperature" not in result.params | ||
| # assert len(warnings) == 1 | ||
| # assert "temperature" in warnings[0].lower() | ||
| # assert "suppressed" in warnings[0] | ||
|
|
||
| # def test_unsupported_provider_raises_error(self): | ||
| # """Test that unsupported providers raise ValueError.""" | ||
| # # Note: This would require modifying KaapiCompletionConfig to accept other providers | ||
| # # For now, this tests the error handling in the mapper | ||
| # # We'll create a mock config that bypasses validation | ||
| # from unittest.mock import MagicMock | ||
|
|
||
| # mock_config = MagicMock() | ||
| # mock_config.provider = "unsupported-provider" | ||
| # mock_config.params = KaapiLLMParams(model="some-model") | ||
|
|
||
| # with pytest.raises(ValueError) as exc_info: | ||
| # transform_kaapi_config_to_native(mock_config) | ||
|
|
||
| # assert "Unsupported provider" in str(exc_info.value) | ||
|
|
||
| # def test_transform_preserves_param_structure(self): | ||
| # """Test that transformation correctly structures nested parameters.""" | ||
| # kaapi_config = KaapiCompletionConfig( | ||
| # provider="openai", | ||
| # params=KaapiLLMParams( | ||
| # model="gpt-4", | ||
| # knowledge_base_ids=["vs_1", "vs_2", "vs_3"], | ||
| # max_num_results=15, | ||
| # ), | ||
| # ) | ||
|
|
||
| # result, warnings = transform_kaapi_config_to_native(kaapi_config) | ||
|
|
||
| # # Verify the nested structure is correct | ||
| # assert isinstance(result.params["tools"], list) | ||
| # assert isinstance(result.params["tools"][0], dict) | ||
| # assert isinstance(result.params["tools"][0]["vector_store_ids"], list) | ||
| # assert len(result.params["tools"][0]["vector_store_ids"]) == 3 | ||
| # assert warnings == [] |
There was a problem hiding this comment.
Tests are fully commented out, disabling coverage.
Commenting out the entire module removes these checks from the suite and hides regressions. Please restore the tests or replace them with explicit skips (e.g., pytest.mark.skip) plus a tracked follow‑up.
If you want, I can help re-enable or update these tests for the new mapper behavior.
🤖 Prompt for AI Agents
In `@backend/app/tests/services/llm/test_mappers.py` around lines 1 - 317, The
test module has been entirely commented out which removes coverage; restore the
tests by uncommenting the file and its imports and test classes (references:
KaapiLLMParams, KaapiCompletionConfig, NativeCompletionConfig,
map_kaapi_to_openai_params, transform_kaapi_config_to_native) so they run
normally, or if intentionally disabled mark individual tests with
pytest.mark.skip and add a TODO/link to a tracked issue; ensure pytest is
imported and any mocked tests (e.g., using MagicMock) remain valid after
re-enabling so the CI regains these checks.
| config_type = completion_config.get("type") | ||
| provider = completion_config.get("provider", "openai-native") |
There was a problem hiding this comment.
Missing default for config_type could cause validation errors.
The type field is mandatory in both NativeCompletionConfig and KaapiCompletionConfig with valid values "text", "stt", or "tts". If the previous version's config_blob lacks a type key, config_type will be None, causing Pydantic validation to fail on lines 350 and 365.
🛡️ Proposed fix to add a default value
- config_type = completion_config.get("type")
+ config_type = completion_config.get("type", "text")Based on learnings: the type field in config_blob.completion is compulsory/mandatory during config creation with valid values "text", "stt", or "tts".
📝 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.
| config_type = completion_config.get("type") | |
| provider = completion_config.get("provider", "openai-native") | |
| config_type = completion_config.get("type", "text") | |
| provider = completion_config.get("provider", "openai-native") |
🤖 Prompt for AI Agents
In `@backend/app/tests/utils/test_data.py` around lines 342 - 343, The code reads
completion_config.get("type") into config_type without a default which can lead
to Pydantic validation errors for NativeCompletionConfig/KaapiCompletionConfig;
set a safe default (e.g., "text") when reading the field so config_type =
completion_config.get("type", "text") (or equivalent) before passing into
NativeCompletionConfig/KaapiCompletionConfig construction to ensure the required
"type" is always present and one of "text"/"stt"/"tts".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/providers/registry.py`:
- Around line 1-2: Remove the module-level dotenv side-effect: delete the "from
dotenv import load_dotenv" import and the module-level call to load_dotenv() in
registry.py (the top-level import block and the stray load_dotenv() call),
leaving environment loading to application bootstrap or tests; also verify no
other code in the file relies on load_dotenv() so you don't introduce
missing-config issues.
- Line 96: The logger.error call in get_llm_provider is missing the required
function-name prefix; update the log message in the exception handler where
logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True)
is called so it starts with "[get_llm_provider]" (e.g., "[get_llm_provider]
Failed to initialize {provider_type} client: {e}") ensuring exc_info=True
remains set and no other behavior changes.
- Around line 14-25: Remove the ad hoc testing code and its unused imports: drop
GenerateContentConfig, NativeCompletionConfig, LLMCallResponse, QueryParams,
LLMOutput, LLMResponse, and Usage from the top of registry.py and delete the
entire if __name__ == "__main__": testing block that references them; ensure
only production imports remain and run tests/lint to confirm no remaining
references to those symbols (look for any usage inside functions or the removed
block before committing).
🧹 Nitpick comments (1)
backend/app/services/llm/providers/sai.py (1)
40-44: Consider a more specific return type forcreate_client.The return type
Anyis imprecise. Since this method returns aSarvamAIclient instance, the return type should reflect that.Proposed fix
`@staticmethod` - def create_client(credentials: dict[str, Any]) -> Any: + def create_client(credentials: dict[str, Any]) -> SarvamAI: if "api_key" not in credentials: raise ValueError("API Key for SarvamAI Not Set") return SarvamAI(api_subscription_key=credentials["api_key"])
| import os | ||
| from dotenv import load_dotenv |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove dotenv import and load_dotenv() call from production module.
The load_dotenv() call at module level (line 27) causes a side effect on every import, which is inappropriate for production code. This appears to be leftover from the ad hoc testing block.
Proposed fix
-import os
-from dotenv import load_dotenv
import loggingAnd remove line 27:
-load_dotenv()Also applies to: 27-27
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/registry.py` around lines 1 - 2, Remove
the module-level dotenv side-effect: delete the "from dotenv import load_dotenv"
import and the module-level call to load_dotenv() in registry.py (the top-level
import block and the stray load_dotenv() call), leaving environment loading to
application bootstrap or tests; also verify no other code in the file relies on
load_dotenv() so you don't introduce missing-config issues.
| from google.genai.types import GenerateContentConfig | ||
|
|
||
| # temporary import | ||
|
|
||
| from app.models.llm import ( | ||
| NativeCompletionConfig, | ||
| LLMCallResponse, | ||
| QueryParams, | ||
| LLMOutput, | ||
| LLMResponse, | ||
| Usage, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused imports tied to ad hoc testing code.
These imports (GenerateContentConfig, NativeCompletionConfig, LLMCallResponse, QueryParams, LLMOutput, LLMResponse, Usage) are only used in the if __name__ == "__main__": block. They should be removed along with the testing code.
Proposed fix
-from google.genai.types import GenerateContentConfig
-
-# temporary import
-
-from app.models.llm import (
- NativeCompletionConfig,
- LLMCallResponse,
- QueryParams,
- LLMOutput,
- LLMResponse,
- Usage,
-)📝 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.
| from google.genai.types import GenerateContentConfig | |
| # temporary import | |
| from app.models.llm import ( | |
| NativeCompletionConfig, | |
| LLMCallResponse, | |
| QueryParams, | |
| LLMOutput, | |
| LLMResponse, | |
| Usage, | |
| ) |
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/registry.py` around lines 14 - 25, Remove
the ad hoc testing code and its unused imports: drop GenerateContentConfig,
NativeCompletionConfig, LLMCallResponse, QueryParams, LLMOutput, LLMResponse,
and Usage from the top of registry.py and delete the entire if __name__ ==
"__main__": testing block that references them; ensure only production imports
remain and run tests/lint to confirm no remaining references to those symbols
(look for any usage inside functions or the removed block before committing).
| # Re-raise ValueError for credential/configuration errors | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True) |
There was a problem hiding this comment.
Add function name prefix to log message.
The log message is missing the required [get_llm_provider] prefix per coding guidelines.
Proposed fix
- logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True)
+ logger.error(f"[get_llm_provider] Failed to initialize {provider_type} client: {e}", exc_info=True)As per coding guidelines: Prefix all log messages with the function name in square brackets.
📝 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.
| logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True) | |
| logger.error(f"[get_llm_provider] Failed to initialize {provider_type} client: {e}", exc_info=True) |
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/registry.py` at line 96, The logger.error
call in get_llm_provider is missing the required function-name prefix; update
the log message in the exception handler where logger.error(f"Failed to
initialize {provider_type} client: {e}", exc_info=True) is called so it starts
with "[get_llm_provider]" (e.g., "[get_llm_provider] Failed to initialize
{provider_type} client: {e}") ensuring exc_info=True remains set and no other
behavior changes.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/providers/sai.py`:
- Around line 120-127: The logs in the execute_stt function are being prefixed
with provider_name instead of the required function name; update the logger
calls in execute_stt (e.g., the info log using sarvam_response.request_id and
the error log using error_message) to use the function name in square brackets
(e.g., "[execute_stt]") as the prefix, and ensure any sensitive values are
wrapped with mask_string(...) per guidelines; change both logger.info and
logger.error invocations in the execute_stt function accordingly.
- Line 5: The module imports the external package name sarvamai (see the import
"from sarvamai import SarvamAI" in sai.py) but it is not declared in project
dependencies; add "sarvamai" to your dependency manifest (e.g., pyproject.toml
[tool.poetry.dependencies] or requirements.txt) with an appropriate version
constraint, update the lockfile (poetry lock / pip-compile or pip freeze) and
run the installer (poetry install or pip install -r requirements.txt) to ensure
the package is available at runtime.
In
`@backend/app/tests/services/llm/providers/STTproviders/test_STT_GeminiProvider.py`:
- Around line 67-99: The function get_llm_provider has a duplicated assignment
of credential_provider (remove the extra credential_provider =
provider_type.replace("-native", "")) and the error log should be prefixed with
the function name in square brackets; update the except Exception block to call
logger.error(f"[get_llm_provider] Failed to initialize {provider_type} client:
{mask_string(str(e))}", exc_info=True) (use mask_string for sensitive info)
while keeping the existing re-raise of ValueError and the RuntimeError raise
behavior.
- Around line 104-112: The test currently prints the raw GEMINI_API_KEY in the
__main__ block (variable GEMINI_API_KEY) which leaks secrets; remove the print
that outputs the full key and instead either omit printing it entirely or print
a masked version (e.g., only show a fixed prefix/suffix or replace with
asterisks) when checking presence, and keep the existing presence check/exit(1)
logic intact. Locate the prints in the main guard (if __name__ == "__main__")
and update the output statements to avoid exposing the secret.
- Around line 9-12: The import for GoogleAIProvider is pointing to the wrong
module; update the import statement that references GoogleAIProvider so it
imports from app.services.llm.providers.gai instead of
app.services.llm.providers.gai2 (replace the import of GoogleAIProvider
accordingly to reference the correct module name `gai`).
In
`@backend/app/tests/services/llm/providers/STTproviders/test_STT_SarvamProvider.py`:
- Around line 1-22: The file has duplicate imports of os and tempfile (they
appear twice at top and again near the bottom), causing Ruff F811; remove the
redundant imports so only a single import line for os and for tempfile remains
(keep the earliest/top-level imports and delete the later ad-hoc duplicates),
then run the linter to confirm F811 is resolved—look for the duplicate import
statements in test_STT_SarvamProvider.py to locate and remove the extra lines.
- Around line 73-104: Remove the duplicate assignment to credential_provider in
get_llm_provider (keep a single credential_provider =
provider_type.replace("-native", "")), and update the error log call
(logger.error) to include the function name prefix in square brackets and mask
sensitive values; e.g., change the message to include "[get_llm_provider]" at
the start and apply mask_string(...) to any sensitive variable you include (for
example mask_string(credentials) or mask_string(provider_type)) while keeping
exc_info=True.
- Around line 108-119: Remove the hardcoded Sarvam API key in the __main__ test
block: replace the literal assignment to SARVAM_API_KEY and construction of
mock_credentials with loading from a safe source (e.g.,
os.environ.get("SARVAM_API_KEY")) or a test fixture/mocked secret provider;
ensure the code validates presence and exits if missing (preserve the existing
if not SARVAM_API_KEY check) and update mock_credentials to use the sourced
value (mock_credentials = {"api_key": SARVAM_API_KEY}) so no secrets are
committed in test_STT_SarvamProvider.py.
🧹 Nitpick comments (1)
backend/app/models/llm/request.py (1)
376-459: Usesa_column_kwargs["comment"]for column comments.Several fields embed comments inside
sa.Column; the guideline preferssa_column_kwargsfor status/type/JSON metadata fields. As per coding guidelines: backend/app/models/**/*.py: Usesa_column_kwargs["comment"]to describe database columns, especially for non-obvious purposes, status/type fields, JSON/metadata columns, and foreign keys.🛠️ Example fix (apply similarly to other fields)
- input_type: Literal["text", "audio", "image"] = Field( - ..., - sa_column=sa.Column( - sa.String, - nullable=False, - comment="Input type: text, audio, image", - ), - ) + input_type: Literal["text", "audio", "image"] = Field( + ..., + sa_column=sa.Column(sa.String, nullable=False), + sa_column_kwargs={"comment": "Input type: text, audio, image"}, + )
| import os | ||
| from typing import Any | ||
|
|
||
| from sarvamai import SarvamAI |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check dependency manifests for sarvamai
rg -n "sarvamai" -g 'pyproject.toml' -g 'requirements*.txt' -g 'Pipfile*' -g 'setup.cfg' -g 'setup.py'Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 57
Add sarvamai to the project's dependencies.
The sarvamai package is imported on line 5 but is not declared in any dependency manifest (pyproject.toml, requirements.txt, setup.py, etc.). This import will fail at runtime, preventing the module from being loaded. Add the package to your project's dependency declarations.
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/sai.py` at line 5, The module imports the
external package name sarvamai (see the import "from sarvamai import SarvamAI"
in sai.py) but it is not declared in project dependencies; add "sarvamai" to
your dependency manifest (e.g., pyproject.toml [tool.poetry.dependencies] or
requirements.txt) with an appropriate version constraint, update the lockfile
(poetry lock / pip-compile or pip freeze) and run the installer (poetry install
or pip install -r requirements.txt) to ensure the package is available at
runtime.
| logger.info( | ||
| f"[{provider_name}.execute_stt] Successfully transcribed audio: {sarvam_response.request_id}" | ||
| ) | ||
| return llm_response, None | ||
|
|
||
| except Exception as e: | ||
| error_message = f"SarvamAI STT transcription failed: {str(e)}" | ||
| logger.error(f"[{provider_name}.execute_stt] {error_message}", exc_info=True) |
There was a problem hiding this comment.
Prefix STT logs with the function name.
The guideline calls for the function name in square brackets; the current prefix uses the provider name instead. 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)}").
🛠️ Suggested fix
- logger.info(
- f"[{provider_name}.execute_stt] Successfully transcribed audio: {sarvam_response.request_id}"
- )
+ logger.info(
+ f"[SarvamAIProvider._execute_stt] Successfully transcribed audio: {sarvam_response.request_id}"
+ )
@@
- logger.error(f"[{provider_name}.execute_stt] {error_message}", exc_info=True)
+ logger.error(
+ f"[SarvamAIProvider._execute_stt] {error_message}", exc_info=True
+ )🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/sai.py` around lines 120 - 127, The logs
in the execute_stt function are being prefixed with provider_name instead of the
required function name; update the logger calls in execute_stt (e.g., the info
log using sarvam_response.request_id and the error log using error_message) to
use the function name in square brackets (e.g., "[execute_stt]") as the prefix,
and ensure any sensitive values are wrapped with mask_string(...) per
guidelines; change both logger.info and logger.error invocations in the
execute_stt function accordingly.
| from app.services.llm.providers.base import BaseProvider | ||
| from app.services.llm.providers.oai import OpenAIProvider | ||
| from app.services.llm.providers.gai2 import GoogleAIProvider | ||
| from app.tests.services.llm.providers.STTproviders.test_data_speechsamples import mydata |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate GoogleAIProvider implementation and confirm module path
fd -g 'gai2.py' -g 'gai.py' backend/app/services/llm/providers
rg -n "class GoogleAIProvider" backend/app/services/llm/providers -g '*.py'Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 227
Correct the GoogleAIProvider import path from gai2 to gai.
The GoogleAIProvider class is defined in backend/app/services/llm/providers/gai.py, not gai2.py. Change the import on line 11 from from app.services.llm.providers.gai2 import GoogleAIProvider to from app.services.llm.providers.gai import GoogleAIProvider.
🤖 Prompt for AI Agents
In
`@backend/app/tests/services/llm/providers/STTproviders/test_STT_GeminiProvider.py`
around lines 9 - 12, The import for GoogleAIProvider is pointing to the wrong
module; update the import statement that references GoogleAIProvider so it
imports from app.services.llm.providers.gai instead of
app.services.llm.providers.gai2 (replace the import of GoogleAIProvider
accordingly to reference the correct module name `gai`).
| def get_llm_provider( | ||
| session: Session, provider_type: str, project_id: int, organization_id: int | ||
| ) -> BaseProvider: | ||
| provider_class = LLMProvider.get_provider_class(provider_type) | ||
|
|
||
| # e.g "openai-native" -> "openai", "claude-native" -> "claude" | ||
| credential_provider = provider_type.replace("-native", "") | ||
|
|
||
| # e.g., "openai-native" → "openai", "claude-native" → "claude" | ||
| credential_provider = provider_type.replace("-native", "") | ||
|
|
||
| credentials = get_provider_credential( | ||
| session=session, | ||
| provider=credential_provider, | ||
| project_id=project_id, | ||
| org_id=organization_id, | ||
| ) | ||
|
|
||
| if not credentials: | ||
| raise ValueError( | ||
| f"Credentials for provider '{credential_provider}' not configured for this project." | ||
| ) | ||
|
|
||
| try: | ||
| client = provider_class.create_client(credentials=credentials) | ||
| return provider_class(client=client) | ||
| except ValueError: | ||
| # Re-raise ValueError for credential/configuration errors | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True) | ||
| raise RuntimeError(f"Could not connect to {provider_type} services.") | ||
|
|
There was a problem hiding this comment.
Deduplicate credential_provider assignment and fix log prefix.
There’s a repeated assignment, and the error log should include the function name prefix. 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)}").
🛠️ Suggested fix
- # e.g "openai-native" -> "openai", "claude-native" -> "claude"
- credential_provider = provider_type.replace("-native", "")
-
- # e.g., "openai-native" → "openai", "claude-native" → "claude"
- credential_provider = provider_type.replace("-native", "")
+ # e.g., "openai-native" → "openai", "claude-native" → "claude"
+ credential_provider = provider_type.replace("-native", "")
@@
- logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True)
+ logger.error(
+ f"[get_llm_provider] Failed to initialize {provider_type} client: {e}",
+ exc_info=True,
+ )🤖 Prompt for AI Agents
In
`@backend/app/tests/services/llm/providers/STTproviders/test_STT_GeminiProvider.py`
around lines 67 - 99, The function get_llm_provider has a duplicated assignment
of credential_provider (remove the extra credential_provider =
provider_type.replace("-native", "")) and the error log should be prefixed with
the function name in square brackets; update the except Exception block to call
logger.error(f"[get_llm_provider] Failed to initialize {provider_type} client:
{mask_string(str(e))}", exc_info=True) (use mask_string for sensitive info)
while keeping the existing re-raise of ValueError and the RuntimeError raise
behavior.
| if __name__ == "__main__": | ||
| print("Hello N, Starting GoogleAIProvider STT test...") | ||
| # 1. Simulate environment/credentials | ||
| # GEMINI_KEY is already defined as GEMINI_API_KEY in the notebook | ||
| GEMINI_API_KEY = os.getenv("GEMINI_API_KEY") | ||
| print(f"GEMINI_API_KEY: {GEMINI_API_KEY}") | ||
| if not GEMINI_API_KEY: | ||
| print("GEMINI_API_KEY is not set.") | ||
| exit(1) |
There was a problem hiding this comment.
Avoid printing the API key to stdout.
This leaks secrets in logs; remove the print or mask the value.
🔐 Suggested fix
- print(f"GEMINI_API_KEY: {GEMINI_API_KEY}")🤖 Prompt for AI Agents
In
`@backend/app/tests/services/llm/providers/STTproviders/test_STT_GeminiProvider.py`
around lines 104 - 112, The test currently prints the raw GEMINI_API_KEY in the
__main__ block (variable GEMINI_API_KEY) which leaks secrets; remove the print
that outputs the full key and instead either omit printing it entirely or print
a masked version (e.g., only show a fixed prefix/suffix or replace with
asterisks) when checking presence, and keep the existing presence check/exit(1)
logic intact. Locate the prints in the main guard (if __name__ == "__main__")
and update the output statements to avoid exposing the secret.
| import os | ||
| from dotenv import load_dotenv | ||
| import logging | ||
|
|
||
| from sqlmodel import Session | ||
| from openai import OpenAI | ||
|
|
||
| from app.crud import get_provider_credential | ||
| from app.services.llm.providers.base import BaseProvider | ||
| from app.services.llm.providers.oai import OpenAIProvider | ||
| from app.services.llm.providers.gai2 import GoogleAIProvider | ||
| from app.services.llm.providers.sai import SarvamAIProvider | ||
|
|
||
| from app.tests.services.llm.providers.STTproviders.test_data_speechsamples import mydata | ||
|
|
||
| import tempfile | ||
|
|
||
|
|
||
| # ad hoc testing code for SarvamAIProvider | ||
| import os | ||
| import tempfile | ||
|
|
There was a problem hiding this comment.
Remove duplicate os/tempfile imports.
They’re redefined later in the file and trip Ruff F811.
🧹 Suggested fix
-# ad hoc testing code for SarvamAIProvider
-import os
-import tempfile
+# ad hoc testing code for SarvamAIProvider📝 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.
| import os | |
| from dotenv import load_dotenv | |
| import logging | |
| from sqlmodel import Session | |
| from openai import OpenAI | |
| from app.crud import get_provider_credential | |
| from app.services.llm.providers.base import BaseProvider | |
| from app.services.llm.providers.oai import OpenAIProvider | |
| from app.services.llm.providers.gai2 import GoogleAIProvider | |
| from app.services.llm.providers.sai import SarvamAIProvider | |
| from app.tests.services.llm.providers.STTproviders.test_data_speechsamples import mydata | |
| import tempfile | |
| # ad hoc testing code for SarvamAIProvider | |
| import os | |
| import tempfile | |
| import os | |
| from dotenv import load_dotenv | |
| import logging | |
| from sqlmodel import Session | |
| from openai import OpenAI | |
| from app.crud import get_provider_credential | |
| from app.services.llm.providers.base import BaseProvider | |
| from app.services.llm.providers.oai import OpenAIProvider | |
| from app.services.llm.providers.gai2 import GoogleAIProvider | |
| from app.services.llm.providers.sai import SarvamAIProvider | |
| from app.tests.services.llm.providers.STTproviders.test_data_speechsamples import mydata | |
| import tempfile | |
| # ad hoc testing code for SarvamAIProvider |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 20-20: Redefinition of unused os from line 1: os redefined here
Remove definition: os
(F811)
[error] 21-21: Redefinition of unused tempfile from line 16: tempfile redefined here
Remove definition: tempfile
(F811)
🤖 Prompt for AI Agents
In
`@backend/app/tests/services/llm/providers/STTproviders/test_STT_SarvamProvider.py`
around lines 1 - 22, The file has duplicate imports of os and tempfile (they
appear twice at top and again near the bottom), causing Ruff F811; remove the
redundant imports so only a single import line for os and for tempfile remains
(keep the earliest/top-level imports and delete the later ad-hoc duplicates),
then run the linter to confirm F811 is resolved—look for the duplicate import
statements in test_STT_SarvamProvider.py to locate and remove the extra lines.
| def get_llm_provider( | ||
| session: Session, provider_type: str, project_id: int, organization_id: int | ||
| ) -> BaseProvider: | ||
| provider_class = LLMProvider.get_provider_class(provider_type) | ||
|
|
||
| # e.g "openai-native" -> "openai", "claude-native" -> "claude" | ||
| credential_provider = provider_type.replace("-native", "") | ||
|
|
||
| # e.g., "openai-native" → "openai", "claude-native" → "claude" | ||
| credential_provider = provider_type.replace("-native", "") | ||
|
|
||
| credentials = get_provider_credential( | ||
| session=session, | ||
| provider=credential_provider, | ||
| project_id=project_id, | ||
| org_id=organization_id, | ||
| ) | ||
|
|
||
| if not credentials: | ||
| raise ValueError( | ||
| f"Credentials for provider '{credential_provider}' not configured for this project." | ||
| ) | ||
|
|
||
| try: | ||
| client = provider_class.create_client(credentials=credentials) | ||
| return provider_class(client=client) | ||
| except ValueError: | ||
| # Re-raise ValueError for credential/configuration errors | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True) | ||
| raise RuntimeError(f"Could not connect to {provider_type} services.") |
There was a problem hiding this comment.
Deduplicate credential_provider assignment and fix log prefix.
There’s a repeated assignment, and the log message should include the function name prefix. 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)}").
🛠️ Suggested fix
- # e.g "openai-native" -> "openai", "claude-native" -> "claude"
- credential_provider = provider_type.replace("-native", "")
-
- # e.g., "openai-native" → "openai", "claude-native" → "claude"
- credential_provider = provider_type.replace("-native", "")
+ # e.g., "openai-native" → "openai", "claude-native" → "claude"
+ credential_provider = provider_type.replace("-native", "")
@@
- logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True)
+ logger.error(
+ f"[get_llm_provider] Failed to initialize {provider_type} client: {e}",
+ exc_info=True,
+ )🤖 Prompt for AI Agents
In
`@backend/app/tests/services/llm/providers/STTproviders/test_STT_SarvamProvider.py`
around lines 73 - 104, Remove the duplicate assignment to credential_provider in
get_llm_provider (keep a single credential_provider =
provider_type.replace("-native", "")), and update the error log call
(logger.error) to include the function name prefix in square brackets and mask
sensitive values; e.g., change the message to include "[get_llm_provider]" at
the start and apply mask_string(...) to any sensitive variable you include (for
example mask_string(credentials) or mask_string(provider_type)) while keeping
exc_info=True.
| if __name__ == "__main__": | ||
| # 1. Simulate environment/credentials | ||
| # SARVAM_API_KEY is already defined in the notebook | ||
| SARVAM_API_KEY = "sk_lmsvfc31_On1bxqwDAqYZoijqBfblr3yf" # for testing only | ||
|
|
||
| if not SARVAM_API_KEY: | ||
| print("SARVAM_API_KEY is not set.") | ||
| exit(1) | ||
|
|
||
| # This dictionary mimics what get_provider_credential would return from the DB | ||
| mock_credentials = {"api_key": SARVAM_API_KEY} | ||
|
|
There was a problem hiding this comment.
Remove the hardcoded Sarvam API key from the test script.
Hardcoded secrets should never be committed, even in ad-hoc test blocks.
🔐 Suggested fix
- SARVAM_API_KEY = "sk_lmsvfc31_On1bxqwDAqYZoijqBfblr3yf" # for testing only
+ SARVAM_API_KEY = os.getenv("SARVAM_API_KEY")🤖 Prompt for AI Agents
In
`@backend/app/tests/services/llm/providers/STTproviders/test_STT_SarvamProvider.py`
around lines 108 - 119, Remove the hardcoded Sarvam API key in the __main__ test
block: replace the literal assignment to SARVAM_API_KEY and construction of
mock_credentials with loading from a safe source (e.g.,
os.environ.get("SARVAM_API_KEY")) or a test fixture/mocked secret provider;
ensure the code validates presence and exits if missing (preserve the existing
if not SARVAM_API_KEY check) and update mock_credentials to use the sourced
value (mock_credentials = {"api_key": SARVAM_API_KEY}) so no secrets are
committed in test_STT_SarvamProvider.py.
…oogle STT v2 doesnot use api key
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/providers/gasr.py`:
- Around line 32-57: The create_client function currently validates project_id
but only returns a speech_v2.SpeechClient, losing project_id for callers (e.g.,
__init__), so change create_client to return both the client and the project_id
— either as a small dataclass/namedtuple (e.g., ClientWithProject) or by
attaching project_id as an attribute on the returned SpeechClient object; update
callers that consume create_client to accept the new return shape and use the
provided project_id instead of re-extracting it from credentials.
- Around line 1-10: Remove duplicate and unused imports: delete the repeated
"from typing import Any, Dict, Optional, Tuple" and the unused "Literal",
"Union", and "base64" imports; keep only the necessary google imports
(speech_v2, types, google.auth) and a single import for typing.Any if needed.
Replace all legacy typing generics and Optionals in this module (e.g., any
occurrences of typing.Dict, typing.Tuple, typing.Optional, typing.Any) with
built-in generics and PEP 604 unions — e.g., dict[str, Any], tuple[X | None, Y],
X | None — and update any function/method signatures and internal annotations in
this file (search for usage in functions/classes within providers/gasr.py) to
use those modern forms. Ensure import lines now only include what is actually
referenced (e.g., "from typing import Any" only if Any remains) and run linting
to confirm the Ruff F811 duplicate-import and unused-import issues are resolved.
- Around line 26-30: Change GoogleSpeechtoTextAIProvider.__init__ so it no
longer requires project_id at instantiation and remove the redundant self.client
assignment: accept project_id: Optional[str] = None, call
super().__init__(client) only, and if project_id is None, derive it from the
provided client (e.g., read credentials/project info attached to the client) or
read a fallback attached earlier by create_client; alternatively ensure
create_client attaches project_id to the client before returning so the provider
can read client.project_id; update any code that expects provider.project_id
accordingly.
- Around line 88-170: In _execute_stt, fix the provider_response_id extraction
to read the nested request id from the STT response metadata (use
response.metadata.request_id when present) instead of hasattr(response,
"request_id") and provide a sensible fallback; also rename or clarify the
token-estimate variables (input_tokens_estimate, output_tokens_estimate,
total_tokens_estimate) to reflect that they are character-based estimates (e.g.
input_chars_estimate, output_chars_estimate, total_chars_estimate) or add a
comment indicating they are character-count approximations so the naming isn’t
misleading; update where these names are used (LLMCallResponse/Usage and any
logging) accordingly.
In `@backend/app/tests/services/llm/providers/STTproviders/test_STT_GoogleASR.py`:
- Around line 118-200: The __main__ block in test_STT_GoogleASR.py contains
hardcoded secret file paths (file_path), a hardcoded project_id, and debug
prints; remove these hardcoded values and debug prints from the test file,
replace them with environment-variable reads (e.g., read KEY_FILENAME and
GCP_PROJECT_ID via os.getenv) and fail fast with a clear error if missing, or
move the manual test into a non-committed scripts/ utility that documents usage;
ensure GoogleSpeechtoTextAIProvider client creation uses the env-provided key
filename and project_id, add a .gitignore entry suggestion for service account
JSON files, and delete the Nan.. debug prints so no secrets or local debug code
are committed.
- Around line 1-200: This file is an ad-hoc debug script (not a test) that must
be removed or replaced with proper pytest tests: delete or move this file out of
tests and remove hardcoded credentials
(probable-splice-440907-h2-56b4982d92cd.json and project_id values) and any
sensitive identifiers; remove duplicated/unused imports (duplicate os/tempfile,
OpenAI, Session, SarvamAIProvider, LLMOutput, LLMResponse, Usage) and remove
duplicated logic (LLMProvider class and get_llm_provider helper) so tests import
the real implementations; if you need coverage, replace with pytest tests that
use fixtures/mocks for GoogleSpeechtoTextAIProvider and its
create_client/execute methods (mock audio generation instead of
create_audio_file_from_mydata) and assert behavior, or if this is only
debugging, delete the file from the tests tree.
- Around line 84-115: get_llm_provider currently instantiates providers with
only client which breaks providers like GoogleSpeechtoTextAIProvider that
require project_id; update the instantiation logic in get_llm_provider to try
creating the provider with both client and project_id (e.g.,
provider_class(client=client, project_id=project_id)) and fall back to
provider_class(client=client) if that raises a TypeError, so providers that need
extra args (like GoogleSpeechtoTextAIProvider) are constructed correctly while
keeping compatibility with existing providers; reference
provider_class.create_client and GoogleSpeechtoTextAIProvider in the change.
🧹 Nitpick comments (5)
backend/app/models/llm/request.py (3)
155-165:NativeCompletionConfig.providerdescription is stale after adding new provider literals.The description still says
"Native provider type (e.g., openai-native)"but now supports"google-native","sarvamai-native", and"google-speech-to-text-native". Consider updating the description to reflect the full set.
175-177:KaapiCompletionConfig.providerdescription is stale.Description says
"LLM provider (openai)"but now also supports"google".
462-473: Consider addingonupdate=nowtosa_column_kwargsfor consistency with automatic database-level timestamp updates.While
default_factory=nowsets the timestamp at row creation, subsequent updates rely on application-layer logic to refresh the timestamp. The codebase already handles this reliably through explicitupdated_at = now()assignments in CRUD operations (e.g.,collection_job.py). However, onlythreads.pyincludesonupdate=now, suggesting that adding it here would provide an additional safeguard against direct database modifications that bypass the application layer while keeping the field pattern consistent.Proposed fix
updated_at: datetime = Field( default_factory=now, nullable=False, - sa_column_kwargs={"comment": "Timestamp when the LLM call was last updated"}, + sa_column_kwargs={ + "comment": "Timestamp when the LLM call was last updated", + "onupdate": now, + }, )backend/app/services/llm/providers/gasr.py (2)
59-86:_parse_inputperforms a file-existence check that_execute_sttrepeats at line 109.The file existence is validated in
_parse_input(line 80) and again in_execute_stt(line 109). While the double-check is a TOCTOU-safe defense, the_parse_inputmethod is called fromexecuteonly to re-validate whatresolved_inputalready provides. Consider whether_parse_inputadds value here, or if the validation in_execute_sttalone suffices.
162-164: Log message uses[{provider_name}.execute_stt]but the function is_execute_stt.Per coding guidelines, log messages should be prefixed with the function name in square brackets. The log format should be
[_execute_stt]rather than[{provider_name}.execute_stt]. Same applies to the error log on line 169.As per coding guidelines: "Prefix all log messages with the function name in square brackets."
| from google.cloud import speech_v2 | ||
| from google.cloud.speech_v2 import types | ||
| import google.auth | ||
|
|
||
| import base64 | ||
| from typing import Any, Dict, Optional, Tuple | ||
|
|
||
| import logging | ||
| import os | ||
| from typing import Any, Dict, Optional, Tuple, Literal, Union |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clean up duplicate and unused imports.
- Lines 6 and 10 duplicate
Any,Dict,Optional,Tuple(Ruff F811). LiteralandUnionon line 10 are never used.base64on line 5 is imported but never used.- Use modern built-in generics (
dict,tuple,X | None) instead of deprecatedtyping.Dict,typing.Tuple,typing.Optional(Python 3.11+ per coding guidelines).
Proposed fix
from google.cloud import speech_v2
from google.cloud.speech_v2 import types
import google.auth
-import base64
-from typing import Any, Dict, Optional, Tuple
-
import logging
import os
-from typing import Any, Dict, Optional, Tuple, Literal, Union
+from typing import Any
from app.models.llm import (
NativeCompletionConfig,Then update type annotations throughout the file to use dict[str, Any], tuple[X | None, str | None], etc.
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 6-6: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 6-6: typing.Tuple is deprecated, use tuple instead
(UP035)
[warning] 10-10: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 10-10: typing.Tuple is deprecated, use tuple instead
(UP035)
[error] 10-10: Redefinition of unused Any from line 6: Any redefined here
Remove definition: Any
(F811)
[error] 10-10: Redefinition of unused Dict from line 6: Dict redefined here
Remove definition: Dict
(F811)
[error] 10-10: Redefinition of unused Optional from line 6: Optional redefined here
Remove definition: Optional
(F811)
[error] 10-10: Redefinition of unused Tuple from line 6: Tuple redefined here
Remove definition: Tuple
(F811)
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/gasr.py` around lines 1 - 10, Remove
duplicate and unused imports: delete the repeated "from typing import Any, Dict,
Optional, Tuple" and the unused "Literal", "Union", and "base64" imports; keep
only the necessary google imports (speech_v2, types, google.auth) and a single
import for typing.Any if needed. Replace all legacy typing generics and
Optionals in this module (e.g., any occurrences of typing.Dict, typing.Tuple,
typing.Optional, typing.Any) with built-in generics and PEP 604 unions — e.g.,
dict[str, Any], tuple[X | None, Y], X | None — and update any function/method
signatures and internal annotations in this file (search for usage in
functions/classes within providers/gasr.py) to use those modern forms. Ensure
import lines now only include what is actually referenced (e.g., "from typing
import Any" only if Any remains) and run linting to confirm the Ruff F811
duplicate-import and unused-import issues are resolved.
| class GoogleSpeechtoTextAIProvider(BaseProvider): | ||
| def __init__(self, client: speech_v2.SpeechClient, project_id: str): | ||
| super().__init__(client) | ||
| self.client = client | ||
| self.project_id = project_id # Store project_id for use in requests |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the gasr.py file and check its contents
find . -name "gasr.py" -type fRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Find the BaseProvider class definition
find . -path "*/providers/base.py" -o -path "*/base_provider.py" | head -5Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Search for BaseProvider class definition
rg "class BaseProvider" -t py -B2 -A10 --max-count=5Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1054
🏁 Script executed:
#!/bin/bash
# Check how providers are instantiated in registry or service code
rg "provider_class\(" -t py -C3 -g '!*test*' | head -100Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 2895
🏁 Script executed:
#!/bin/bash
# Get full BaseProvider implementation
cat -n backend/app/services/llm/providers/base.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 3089
🏁 Script executed:
#!/bin/bash
# Get full gasr.py implementation
cat -n backend/app/services/llm/services/llm/providers/gasr.py 2>/dev/null || cat -n ./backend/app/services/llm/providers/gasr.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 10043
🏁 Script executed:
#!/bin/bash
# Check other provider implementations to see how they handle similar issues
ls -la backend/app/services/llm/providers/ | grep -E "\.py$"Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Get a sample of other provider implementations
for file in backend/app/services/llm/providers/openai.py backend/app/services/llm/providers/anthropic.py; do
if [ -f "$file" ]; then
echo "=== $file ==="
head -50 "$file"
fi
doneRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Get oai.py (OpenAI provider)
cat -n backend/app/services/llm/providers/oai.py | head -60Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 2161
🏁 Script executed:
#!/bin/bash
# Get gai.py (Google AI provider)
cat -n backend/app/services/llm/providers/gai.py | head -60Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 2257
🏁 Script executed:
#!/bin/bash
# Get sai.py (Anthropic provider)
cat -n backend/app/services/llm/providers/sai.py | head -60Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 2343
🏁 Script executed:
#!/bin/bash
# Check how get_llm_provider is actually used - specifically for GoogleSpeechtoTextAIProvider
rg "GoogleSpeechtoTextAIProvider\|gasr\|google.*stt" -t py -C5 -g '!*test*'Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check the registry file to see if GoogleSpeechtoTextAIProvider is registered
cat -n backend/app/services/llm/providers/registry.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 5708
🏁 Script executed:
#!/bin/bash
# Check if GoogleSpeechtoTextAIProvider is imported or referenced anywhere
rg "GoogleSpeechtoTextAI\|from.*gasr\|import.*gasr" -t pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check the __init__.py file in providers directory
cat -n backend/app/services/llm/providers/__init__.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 389
Fix constructor to work with factory pattern before registration, and remove redundant self.client assignment.
The constructor requires both client and project_id, but the registry pattern at line 91 calls provider_class(client=client). When GoogleSpeechtoTextAIProvider is added to the registry, this will raise a TypeError for the missing project_id parameter.
Additionally, line 29 (self.client = client) is redundant since BaseProvider.__init__ already sets self.client.
Suggested fixes:
- Attach
project_idto the client object before returning fromcreate_client, or - Override instantiation in the registry to pass additional parameters, or
- Accept
project_idas an optional parameter with a fallback to retrieve it from the client credentials.
Remove the redundant self.client = client assignment.
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/gasr.py` around lines 26 - 30, Change
GoogleSpeechtoTextAIProvider.__init__ so it no longer requires project_id at
instantiation and remove the redundant self.client assignment: accept
project_id: Optional[str] = None, call super().__init__(client) only, and if
project_id is None, derive it from the provided client (e.g., read
credentials/project info attached to the client) or read a fallback attached
earlier by create_client; alternatively ensure create_client attaches project_id
to the client before returning so the provider can read client.project_id;
update any code that expects provider.project_id accordingly.
| @staticmethod | ||
| def create_client(credentials: Dict[str, Any]) -> speech_v2.SpeechClient: | ||
| """Create and return a Google Cloud Speech-to-Text client instance. | ||
|
|
||
| Args: | ||
| credentials: A dictionary containing 'key_filename', 'project_id', and optionally 'api_endpoint'. | ||
|
|
||
| Returns: | ||
| A configured speech_v2.SpeechClient instance. | ||
| """ | ||
| key_filename = credentials.get("key_filename") | ||
| project_id = credentials.get("project_id") | ||
| api_endpoint = credentials.get("api_endpoint", "global") # Default to global if not specified | ||
|
|
||
| if not key_filename: | ||
| raise ValueError("Service account key filename is not provided in credentials.") | ||
| if not project_id: | ||
| raise ValueError("Project ID is not provided in credentials.") | ||
|
|
||
| creds, _ = google.auth.load_credentials_from_file(key_filename) | ||
|
|
||
| client_options = {} | ||
| if api_endpoint != "global": | ||
| client_options["api_endpoint"] = f"{api_endpoint}-speech.googleapis.com" | ||
|
|
||
| return speech_v2.SpeechClient(credentials=creds, client_options=client_options) |
There was a problem hiding this comment.
create_client validates project_id but doesn't return it — caller has no way to retrieve it from the client.
create_client validates that project_id exists in credentials (line 48-49) but only returns the SpeechClient. The project_id is needed by __init__ but is lost after create_client returns. This forces callers to separately extract project_id from credentials, breaking the encapsulation that create_client is supposed to provide.
Consider returning a named tuple or dataclass containing both the client and project_id, or storing project_id on the client object.
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/gasr.py` around lines 32 - 57, The
create_client function currently validates project_id but only returns a
speech_v2.SpeechClient, losing project_id for callers (e.g., __init__), so
change create_client to return both the client and the project_id — either as a
small dataclass/namedtuple (e.g., ClientWithProject) or by attaching project_id
as an attribute on the returned SpeechClient object; update callers that consume
create_client to accept the new return shape and use the provided project_id
instead of re-extracting it from credentials.
| def _execute_stt( | ||
| self, | ||
| completion_config: NativeCompletionConfig, | ||
| resolved_input: str, # Now expecting a file path (str), consistent with GoogleAIProvider | ||
| include_provider_raw_response: bool = False, | ||
| ) -> Tuple[Optional[LLMCallResponse], Optional[str]]: | ||
| provider_name = self.get_provider_name() | ||
| generation_params = completion_config.params | ||
|
|
||
| model = generation_params.get("model", "chirp_3") | ||
| language_codes = generation_params.get("language_codes", ["en-US"]) | ||
| recognizer_id = generation_params.get("recognizer_id", "_") | ||
| location = generation_params.get("location", "global") | ||
|
|
||
| current_project_id = self.project_id | ||
|
|
||
| if not current_project_id: | ||
| return None, "Missing project_id in GoogleSpeechtoTextAIProvider initialization." | ||
|
|
||
| # Read the audio file into bytes within _execute_stt | ||
| try: | ||
| if not os.path.exists(resolved_input): | ||
| return None, f"Audio file not found at path: {resolved_input}" | ||
| with open(resolved_input, "rb") as f: | ||
| resolved_input_bytes = f.read() | ||
| except Exception as e: | ||
| return None, f"Failed to read audio file: {str(e)}" | ||
|
|
||
| config = types.RecognitionConfig( | ||
| auto_decoding_config=types.AutoDetectDecodingConfig(), | ||
| language_codes=language_codes, | ||
| model=model, | ||
| ) | ||
|
|
||
| try: | ||
| request = types.RecognizeRequest( | ||
| recognizer=f"projects/{current_project_id}/locations/{location}/recognizers/{recognizer_id}", | ||
| config=config, | ||
| content=resolved_input_bytes, # Direct content is passed here | ||
| ) | ||
|
|
||
| response: types.RecognizeResponse = self.client.recognize(request=request) | ||
|
|
||
| transcript = "" | ||
| total_output_chars = 0 | ||
| if response.results: | ||
| for result in response.results: | ||
| if result.alternatives: | ||
| transcript += result.alternatives[0].transcript | ||
| total_output_chars += len(result.alternatives[0].transcript) | ||
|
|
||
| input_tokens_estimate = 0 | ||
| output_tokens_estimate = total_output_chars | ||
| total_tokens_estimate = input_tokens_estimate + output_tokens_estimate | ||
|
|
||
| llm_response = LLMCallResponse( | ||
| response=LLMResponse( | ||
| provider_response_id=response.request_id if hasattr(response, "request_id") else "unknown", | ||
| conversation_id=None, | ||
| provider=provider_name, | ||
| model=model, | ||
| output=LLMOutput(text=transcript), | ||
| ), | ||
| usage=Usage( | ||
| input_tokens=input_tokens_estimate, | ||
| output_tokens=output_tokens_estimate, | ||
| total_tokens=total_tokens_estimate, | ||
| reasoning_tokens=None, | ||
| ), | ||
| ) | ||
|
|
||
| if include_provider_raw_response: | ||
| llm_response.provider_raw_response = types.RecognizeResponse.to_dict(response) | ||
|
|
||
| logger.info( | ||
| f"[{provider_name}.execute_stt] Successfully transcribed audio: {llm_response.response.provider_response_id}" | ||
| ) | ||
| return llm_response, None | ||
|
|
||
| except Exception as e: | ||
| error_message = f"Google Cloud Speech-to-Text transcription failed: {str(e)}" | ||
| logger.error(f"[{provider_name}.execute_stt] {error_message}", exc_info=True) | ||
| return None, error_message |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Google Cloud Speech-to-Text v2 API RecognizeResponse attributes request_id
💡 Result:
In Speech-to-Text v2, RecognizeResponse does not have a top-level request_id. Instead, it’s in the response metadata:
RecognizeResponse.metadatais aRecognitionResponseMetadataobject. [1]RecognitionResponseMetadatacontainsrequest_id(“Global request identifier auto-generated by the API”). [2]
So the field path is:
- Proto/JSON field:
metadata.request_id[2] - .NET:
response.Metadata.RequestId[1][3] - Java:
response.getMetadata().getRequestId()[4]
Sources:
[1] Google Cloud .NET docs: RecognizeResponse properties (Metadata)
[2] Google Cloud RPC reference: RecognitionResponseMetadata.request_id
[3] Google Cloud .NET docs: RecognitionResponseMetadata.RequestId
[4] Google Cloud Java docs: RecognitionResponseMetadata.getRequestId()
🌐 Web query:
Google Cloud Speech-to-Text v2 API valid model names chirp_3
💡 Result:
In Google Cloud Speech-to-Text v2, the RecognitionConfig.model field accepts these model name strings:
chirp_3chirp_2telephony(for 8 kHz phone-call audio) [1]
So for Chirp 3 specifically, the valid model name is exactly: chirp_3 (as used in the v2 examples/docs). [2]
Sources:
[1] https://docs.cloud.google.com/speech-to-text/docs/transcription-model
[2] https://cloud.google.com/speech-to-text/v2/docs/chirp-model
🏁 Script executed:
# First, let's find and examine the gasr.py file to understand the context better
fd "gasr.py" --type fRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 117
🏁 Script executed:
# Check what version of google-cloud-speech is used in the project
fd "requirements" -o fd "setup.py" -o fd "pyproject.toml" | xargs grep -l speech 2>/dev/null || echo "Searching for dependencies..."Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 245
🏁 Script executed:
# Look for the GoogleSpeechtoTextAIProvider class definition to understand more context
rg "class GoogleSpeechtoTextAIProvider" -A 20Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1863
🏁 Script executed:
# Check the full function signature and logging messages in gasr.py
sed -n '88,170p' backend/app/services/llm/providers/gasr.py | head -90Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 3666
🏁 Script executed:
# Also check if there are type hints on the function
rg "_execute_stt" -A 10 backend/app/services/llm/providers/gasr.py | head -20Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1007
🏁 Script executed:
# Verify the logging pattern in the file
rg "logger\.(info|error|warning|debug)" backend/app/services/llm/providers/gasr.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 241
Fix request_id attribute access and clarify token estimation terminology.
-
Line 145:
RecognizeResponsein Speech-to-Text v2 does not have a top-levelrequest_idattribute. The request ID is nested within metadata: useresponse.metadata.request_idinstead ofhasattr(response, "request_id"), which always falls back to"unknown". -
Lines 139-141:
output_tokens_estimateis set to character count, which is misleading. Either rename the variables to reflect character counts or document that these are estimates based on character length, not actual token counts from the Speech-to-Text API.
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/gasr.py` around lines 88 - 170, In
_execute_stt, fix the provider_response_id extraction to read the nested request
id from the STT response metadata (use response.metadata.request_id when
present) instead of hasattr(response, "request_id") and provide a sensible
fallback; also rename or clarify the token-estimate variables
(input_tokens_estimate, output_tokens_estimate, total_tokens_estimate) to
reflect that they are character-based estimates (e.g. input_chars_estimate,
output_chars_estimate, total_chars_estimate) or add a comment indicating they
are character-count approximations so the naming isn’t misleading; update where
these names are used (LLMCallResponse/Usage and any logging) accordingly.
| import os | ||
| from dotenv import load_dotenv | ||
| import logging | ||
|
|
||
| from sqlmodel import Session | ||
| from openai import OpenAI | ||
|
|
||
| from app.crud import get_provider_credential | ||
| from app.services.llm.providers.base import BaseProvider | ||
| from app.services.llm.providers.oai import OpenAIProvider | ||
| from app.services.llm.providers.gai2 import GoogleAIProvider | ||
| from app.services.llm.providers.sai import SarvamAIProvider | ||
| from app.services.llm.providers.gasr import GoogleSpeechtoTextAIProvider | ||
|
|
||
| from app.services.llm.providers.tests_data import mydata | ||
|
|
||
| import tempfile | ||
|
|
||
|
|
||
| # ad hoc testing code for SarvamAIProvider | ||
| import os | ||
| import tempfile | ||
|
|
||
| # temporary import | ||
|
|
||
| from app.models.llm import ( | ||
| NativeCompletionConfig, | ||
| LLMCallResponse, | ||
| QueryParams, | ||
| LLMOutput, | ||
| LLMResponse, | ||
| Usage, | ||
| ) | ||
|
|
||
| load_dotenv() | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def create_audio_file_from_mydata(audio_data: bytes) -> str: | ||
| """ | ||
| Creates a temporary WAV file from provided audio data (bytes) | ||
| and returns the path to the file. | ||
| """ | ||
| temp_audio_file = tempfile.NamedTemporaryFile(delete=False, suffix='.wav') | ||
| temp_audio_file.write(audio_data) | ||
| temp_audio_file.close() | ||
| print(f"Created temporary audio file at: {temp_audio_file.name}") | ||
| return temp_audio_file.name | ||
|
|
||
|
|
||
| class LLMProvider: | ||
| OPENAI_NATIVE = "openai-native" | ||
| OPENAI = "openai" | ||
| # Future constants for native providers: | ||
| # CLAUDE_NATIVE = "claude-native" | ||
| GOOGLE_NATIVE = "google-native" | ||
|
|
||
| _registry: dict[str, type[BaseProvider]] = { | ||
| OPENAI_NATIVE: OpenAIProvider, | ||
| OPENAI: OpenAIProvider, | ||
| # Future native providers: | ||
| # CLAUDE_NATIVE: ClaudeProvider, | ||
| GOOGLE_NATIVE: GoogleAIProvider, | ||
| } | ||
|
|
||
| @classmethod | ||
| def get_provider_class(cls, provider_type: str) -> type[BaseProvider]: | ||
| """Return the provider class for a given name.""" | ||
| provider = cls._registry.get(provider_type) | ||
| if not provider: | ||
| raise ValueError( | ||
| f"Provider '{provider_type}' is not supported. " | ||
| f"Supported providers: {', '.join(cls._registry.keys())}" | ||
| ) | ||
| return provider | ||
|
|
||
| @classmethod | ||
| def supported_providers(cls) -> list[str]: | ||
| """Return a list of supported provider names.""" | ||
| return list(cls._registry.keys()) | ||
|
|
||
|
|
||
| def get_llm_provider( | ||
| session: Session, provider_type: str, project_id: int, organization_id: int | ||
| ) -> BaseProvider: | ||
| provider_class = LLMProvider.get_provider_class(provider_type) | ||
|
|
||
| # e.g "openai-native" -> "openai", "claude-native" -> "claude" | ||
| credential_provider = provider_type.replace("-native", "") | ||
|
|
||
| # e.g., "openai-native" → "openai", "claude-native" → "claude" | ||
| credential_provider = provider_type.replace("-native", "") | ||
|
|
||
| credentials = get_provider_credential( | ||
| session=session, | ||
| provider=credential_provider, | ||
| project_id=project_id, | ||
| org_id=organization_id, | ||
| ) | ||
|
|
||
| if not credentials: | ||
| raise ValueError( | ||
| f"Credentials for provider '{credential_provider}' not configured for this project." | ||
| ) | ||
|
|
||
| try: | ||
| client = provider_class.create_client(credentials=credentials) | ||
| return provider_class(client=client) | ||
| except ValueError: | ||
| # Re-raise ValueError for credential/configuration errors | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True) | ||
| raise RuntimeError(f"Could not connect to {provider_type} services.") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
|
|
||
| file_path = 'probable-splice-440907-h2-56b4982d92cd.json' | ||
| file_path = 'app/services/llm/providers/probable-splice-440907-h2-56b4982d92cd.json' | ||
| file_path = 'app/tests/services/llm/providers/STTproviders/probable-splice-440907-h2-56b4982d92cd.json' | ||
|
|
||
| print(f"Current working directory: {os.getcwd()}") | ||
| if os.path.exists(file_path): | ||
| print(f"Nan..The file '{file_path}' exists.") | ||
| else: | ||
| print(f"Nan..The file '{file_path}' does not exist.") | ||
|
|
||
| # Update the LLMProvider registry to include GoogleSpeechtoTextAIProvider | ||
| if "google-speech-to-text-native" not in LLMProvider._registry: | ||
| LLMProvider._registry["google-speech-to-text-native"] = GoogleSpeechtoTextAIProvider | ||
| print("GoogleSpeechtoTextAIProvider registered successfully in LLMProvider.") | ||
| else: | ||
| print("GoogleSpeechtoTextAIProvider was already registered.") | ||
|
|
||
| print(f"Supported providers after update: {LLMProvider.supported_providers()}") | ||
|
|
||
| # key_filename = "probable-splice-440907-h2-56b4982d92cd.json" | ||
| key_filename = file_path | ||
|
|
||
| audio_file_path = create_audio_file_from_mydata(mydata) | ||
|
|
||
| project_id = 'probable-splice-440907-h2' | ||
|
|
||
|
|
||
| # 1. Prepare credentials for client creation | ||
| credentials = { | ||
| "key_filename": key_filename, | ||
| "project_id": project_id, | ||
| "api_endpoint": "asia-northeast1" # Specify the endpoint based on model availability | ||
| } | ||
|
|
||
| # 2. Create the Speech Client using the static method | ||
| try: | ||
| # create_client now only returns the client object | ||
| speech_client_obj = GoogleSpeechtoTextAIProvider.create_client(credentials) | ||
| except ValueError as e: | ||
| print(f"Error creating client: {e}") | ||
| speech_client_obj = None | ||
|
|
||
| # Check if the project_id is available in credentials, which is now expected | ||
| if speech_client_obj and credentials.get('project_id'): | ||
| # 3. Instantiate the provider, passing the client object and the project_id explicitly | ||
| provider = GoogleSpeechtoTextAIProvider(client=speech_client_obj, project_id=credentials['project_id']) | ||
|
|
||
| # 4. Define completion configuration (similar to NativeCompletionConfig) | ||
| completion_config = NativeCompletionConfig( | ||
| provider="google-speech-to-text-native", # Use the registered provider name | ||
| type="stt", | ||
| params={ | ||
| "model": "chirp_3", | ||
| "language_codes": ["auto"], # Using 'ta-IN' based on the audio content, 'auto' is also an option | ||
| "recognizer_id": "_", # Use underscore for default recognizer if not specified | ||
| "location": "asia-northeast1", # Must match the api_endpoint for the client | ||
| "project_id": credentials['project_id'] # Pass the project ID for consistency if needed in params | ||
| } | ||
| ) | ||
|
|
||
| test_query = QueryParams( | ||
| input={"type": "text", "content": "Transcription request"} | ||
| ) | ||
|
|
||
| # 5. Execute STT | ||
| # Use the 'audio_file_path' variable created from 'mydata' | ||
| print(audio_file_path) | ||
| response, error = provider.execute( | ||
| completion_config=completion_config, | ||
| query=test_query, # dummy QueryParams for STT | ||
| resolved_input=audio_file_path, # Use the path to the temporary audio file | ||
| include_provider_raw_response=False | ||
| ) | ||
|
|
||
| if error: | ||
| print(f"STT Error: {error}") | ||
| elif response: | ||
| print(f"Transcript from GoogleSpeechtoTextAIProvider: {response.response.output.text}") | ||
| print(f"Usage: Input Tokens={response.usage.input_tokens}, Output Tokens={response.usage.output_tokens}, Total Tokens={response.usage.total_tokens}") | ||
| else: | ||
| print("Could not initialize GoogleSpeechtoTextAIProvider: Client or project ID missing.") |
There was a problem hiding this comment.
This file is not a test — it's an ad-hoc debug script that should not be merged.
Several critical concerns:
-
No test functions: There are no
test_-prefixed functions, nopytestorunittestusage. This file won't be discovered or executed by any test runner. The PR checklist item "Add/verify tests" is unchecked, and this file doesn't fulfill it. -
Hardcoded credential references (lines 120-122): The file references a GCP service account key file (
probable-splice-440907-h2-56b4982d92cd.json) by name, and a GCP project ID (probable-splice-440907-h2) on line 144. Even if the actual key file isn't committed, these identifiers shouldn't be in source control. -
Duplicate code: The
LLMProviderregistry class andget_llm_providerhelper (lines 52-115) appear to duplicate production code. Tests should import from the actual modules. -
Dead code & duplicate imports:
osimported twice (lines 1, 21),tempfileimported twice (lines 17, 22),credential_providerassigned twice identically (lines 90, 93), unused imports (OpenAI,Session,SarvamAIProvider,LLMOutput,LLMResponse,Usage). -
Misleading comment: Line 20 says "ad hoc testing code for SarvamAIProvider" but the file tests Google ASR.
This should be replaced with proper pytest-based tests using mocks/fixtures, or removed entirely.
🧰 Tools
🪛 Ruff (0.14.14)
[error] 21-21: Redefinition of unused os from line 1: os redefined here
Remove definition: os
(F811)
[error] 22-22: Redefinition of unused tempfile from line 17: tempfile redefined here
Remove definition: tempfile
(F811)
🤖 Prompt for AI Agents
In `@backend/app/tests/services/llm/providers/STTproviders/test_STT_GoogleASR.py`
around lines 1 - 200, This file is an ad-hoc debug script (not a test) that must
be removed or replaced with proper pytest tests: delete or move this file out of
tests and remove hardcoded credentials
(probable-splice-440907-h2-56b4982d92cd.json and project_id values) and any
sensitive identifiers; remove duplicated/unused imports (duplicate os/tempfile,
OpenAI, Session, SarvamAIProvider, LLMOutput, LLMResponse, Usage) and remove
duplicated logic (LLMProvider class and get_llm_provider helper) so tests import
the real implementations; if you need coverage, replace with pytest tests that
use fixtures/mocks for GoogleSpeechtoTextAIProvider and its
create_client/execute methods (mock audio generation instead of
create_audio_file_from_mydata) and assert behavior, or if this is only
debugging, delete the file from the tests tree.
| def get_llm_provider( | ||
| session: Session, provider_type: str, project_id: int, organization_id: int | ||
| ) -> BaseProvider: | ||
| provider_class = LLMProvider.get_provider_class(provider_type) | ||
|
|
||
| # e.g "openai-native" -> "openai", "claude-native" -> "claude" | ||
| credential_provider = provider_type.replace("-native", "") | ||
|
|
||
| # e.g., "openai-native" → "openai", "claude-native" → "claude" | ||
| credential_provider = provider_type.replace("-native", "") | ||
|
|
||
| credentials = get_provider_credential( | ||
| session=session, | ||
| provider=credential_provider, | ||
| project_id=project_id, | ||
| org_id=organization_id, | ||
| ) | ||
|
|
||
| if not credentials: | ||
| raise ValueError( | ||
| f"Credentials for provider '{credential_provider}' not configured for this project." | ||
| ) | ||
|
|
||
| try: | ||
| client = provider_class.create_client(credentials=credentials) | ||
| return provider_class(client=client) | ||
| except ValueError: | ||
| # Re-raise ValueError for credential/configuration errors | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Failed to initialize {provider_type} client: {e}", exc_info=True) | ||
| raise RuntimeError(f"Could not connect to {provider_type} services.") |
There was a problem hiding this comment.
get_llm_provider on line 109 calls provider_class(client=client) — incompatible with GoogleSpeechtoTextAIProvider.
GoogleSpeechtoTextAIProvider.__init__ requires both client and project_id, but this helper only passes client. This will raise a TypeError at runtime if used with the Google ASR provider. This is the same interface mismatch flagged in gasr.py.
🤖 Prompt for AI Agents
In `@backend/app/tests/services/llm/providers/STTproviders/test_STT_GoogleASR.py`
around lines 84 - 115, get_llm_provider currently instantiates providers with
only client which breaks providers like GoogleSpeechtoTextAIProvider that
require project_id; update the instantiation logic in get_llm_provider to try
creating the provider with both client and project_id (e.g.,
provider_class(client=client, project_id=project_id)) and fall back to
provider_class(client=client) if that raises a TypeError, so providers that need
extra args (like GoogleSpeechtoTextAIProvider) are constructed correctly while
keeping compatibility with existing providers; reference
provider_class.create_client and GoogleSpeechtoTextAIProvider in the change.
| if __name__ == "__main__": | ||
|
|
||
| file_path = 'probable-splice-440907-h2-56b4982d92cd.json' | ||
| file_path = 'app/services/llm/providers/probable-splice-440907-h2-56b4982d92cd.json' | ||
| file_path = 'app/tests/services/llm/providers/STTproviders/probable-splice-440907-h2-56b4982d92cd.json' | ||
|
|
||
| print(f"Current working directory: {os.getcwd()}") | ||
| if os.path.exists(file_path): | ||
| print(f"Nan..The file '{file_path}' exists.") | ||
| else: | ||
| print(f"Nan..The file '{file_path}' does not exist.") | ||
|
|
||
| # Update the LLMProvider registry to include GoogleSpeechtoTextAIProvider | ||
| if "google-speech-to-text-native" not in LLMProvider._registry: | ||
| LLMProvider._registry["google-speech-to-text-native"] = GoogleSpeechtoTextAIProvider | ||
| print("GoogleSpeechtoTextAIProvider registered successfully in LLMProvider.") | ||
| else: | ||
| print("GoogleSpeechtoTextAIProvider was already registered.") | ||
|
|
||
| print(f"Supported providers after update: {LLMProvider.supported_providers()}") | ||
|
|
||
| # key_filename = "probable-splice-440907-h2-56b4982d92cd.json" | ||
| key_filename = file_path | ||
|
|
||
| audio_file_path = create_audio_file_from_mydata(mydata) | ||
|
|
||
| project_id = 'probable-splice-440907-h2' | ||
|
|
||
|
|
||
| # 1. Prepare credentials for client creation | ||
| credentials = { | ||
| "key_filename": key_filename, | ||
| "project_id": project_id, | ||
| "api_endpoint": "asia-northeast1" # Specify the endpoint based on model availability | ||
| } | ||
|
|
||
| # 2. Create the Speech Client using the static method | ||
| try: | ||
| # create_client now only returns the client object | ||
| speech_client_obj = GoogleSpeechtoTextAIProvider.create_client(credentials) | ||
| except ValueError as e: | ||
| print(f"Error creating client: {e}") | ||
| speech_client_obj = None | ||
|
|
||
| # Check if the project_id is available in credentials, which is now expected | ||
| if speech_client_obj and credentials.get('project_id'): | ||
| # 3. Instantiate the provider, passing the client object and the project_id explicitly | ||
| provider = GoogleSpeechtoTextAIProvider(client=speech_client_obj, project_id=credentials['project_id']) | ||
|
|
||
| # 4. Define completion configuration (similar to NativeCompletionConfig) | ||
| completion_config = NativeCompletionConfig( | ||
| provider="google-speech-to-text-native", # Use the registered provider name | ||
| type="stt", | ||
| params={ | ||
| "model": "chirp_3", | ||
| "language_codes": ["auto"], # Using 'ta-IN' based on the audio content, 'auto' is also an option | ||
| "recognizer_id": "_", # Use underscore for default recognizer if not specified | ||
| "location": "asia-northeast1", # Must match the api_endpoint for the client | ||
| "project_id": credentials['project_id'] # Pass the project ID for consistency if needed in params | ||
| } | ||
| ) | ||
|
|
||
| test_query = QueryParams( | ||
| input={"type": "text", "content": "Transcription request"} | ||
| ) | ||
|
|
||
| # 5. Execute STT | ||
| # Use the 'audio_file_path' variable created from 'mydata' | ||
| print(audio_file_path) | ||
| response, error = provider.execute( | ||
| completion_config=completion_config, | ||
| query=test_query, # dummy QueryParams for STT | ||
| resolved_input=audio_file_path, # Use the path to the temporary audio file | ||
| include_provider_raw_response=False | ||
| ) | ||
|
|
||
| if error: | ||
| print(f"STT Error: {error}") | ||
| elif response: | ||
| print(f"Transcript from GoogleSpeechtoTextAIProvider: {response.response.output.text}") | ||
| print(f"Usage: Input Tokens={response.usage.input_tokens}, Output Tokens={response.usage.output_tokens}, Total Tokens={response.usage.total_tokens}") | ||
| else: | ||
| print("Could not initialize GoogleSpeechtoTextAIProvider: Client or project ID missing.") |
There was a problem hiding this comment.
__main__ block contains hardcoded secrets and debug prints — must not be committed.
Lines 120-122 hardcode paths to a GCP service account JSON key file, and line 144 hardcodes a GCP project ID. Even without the actual key file in the repo, these are sensitive identifiers. Lines 126-128 contain debug prints ("Nan..The file...") that indicate this is local development/debugging code.
If manual integration testing is needed, consider:
- A
scripts/directory with clear documentation - Environment variables for all credential paths and project IDs
- A
.gitignoreentry for service account JSON files
🤖 Prompt for AI Agents
In `@backend/app/tests/services/llm/providers/STTproviders/test_STT_GoogleASR.py`
around lines 118 - 200, The __main__ block in test_STT_GoogleASR.py contains
hardcoded secret file paths (file_path), a hardcoded project_id, and debug
prints; remove these hardcoded values and debug prints from the test file,
replace them with environment-variable reads (e.g., read KEY_FILENAME and
GCP_PROJECT_ID via os.getenv) and fail fast with a clear error if missing, or
move the manual test into a non-committed scripts/ utility that documents usage;
ensure GoogleSpeechtoTextAIProvider client creation uses the env-provided key
filename and project_id, add a .gitignore entry suggestion for service account
JSON files, and delete the Nan.. debug prints so no secrets or local debug code
are committed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/providers/gasr.py`:
- Around line 131-137: The code concatenates Google STT result segments without
separators which can fuse words; in the gasr.py handler (the block using
response.results and result.alternatives[0].transcript, and variables transcript
and total_output_chars) update the concatenation logic to ensure a separator is
inserted when needed: before appending next_segment =
result.alternatives[0].transcript, check if transcript is non-empty and neither
transcript.endswith(whitespace) nor next_segment.startswith(whitespace) and if
so append a single space before next_segment, then update total_output_chars by
len(next_segment) (plus the added space if inserted); keep all other behaviors
the same.
- Line 26: The provider class GoogleSpeechtoTextAIProvider is never registered
so it can't be instantiated via the factory; to fix it add an import for
GoogleSpeechtoTextAIProvider into the package __init__.py and add an entry to
the _registry dict in registry.py mapping a provider key (e.g.,
"google-stt-native") to GoogleSpeechtoTextAIProvider so get_llm_provider() and
supported providers can construct it.
🧹 Nitpick comments (3)
backend/app/services/llm/providers/gasr.py (3)
59-86: File-existence is checked twice — here and again in_execute_stt(line 109).
_parse_inputvalidates the file exists (line 80), and_execute_sttchecksos.path.existsagain (line 109) before reading. Consider removing the redundant check in_execute_sttsince_parse_inputalready guarantees the path is valid by the time execution proceeds (TOCTOU aside, the second check doesn't add meaningful safety since the file could vanish between either check and theopencall).
162-163: Log message prefix doesn't match function name.Per coding guidelines, log messages should be prefixed with the function name in brackets. The current format
[{provider_name}.execute_stt]should be[_execute_stt].- logger.info( - f"[{provider_name}.execute_stt] Successfully transcribed audio: {llm_response.response.provider_response_id}" - ) + logger.info( + f"[_execute_stt] Successfully transcribed audio: {llm_response.response.provider_response_id}" + )Same applies to the error log on line 169.
As per coding guidelines, "Prefix all log messages with the function name in square brackets."
201-212: Log messages inexecutealso don't follow the function-name prefix convention, and file is missing trailing newline.Same log prefix issue as in
_execute_stt— use[execute]instead of[{self.get_provider_name()}.execute]. Also, Ruff W292 flags no trailing newline at end of file (line 212).Proposed fix
except ValueError as e: error_message = f"Input validation error: {str(e)}" logger.error( - f"[{self.get_provider_name()}.execute] {error_message}", exc_info=True + f"[execute] {error_message}", exc_info=True ) return None, error_message except Exception as e: error_message = f"Unexpected error occurred during {self.get_provider_name()} execution: {str(e)}" logger.error( - f"[{self.get_provider_name()}.execute] {error_message}", exc_info=True + f"[execute] {error_message}", exc_info=True ) return None, error_message +As per coding guidelines, "Prefix all log messages with the function name in square brackets."
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Google Speech-to-Text provider implementation using Google Cloud's Speech-to-Text API (speech_v2) | ||
| class GoogleSpeechtoTextAIProvider(BaseProvider): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if GoogleSpeechtoTextAIProvider is referenced anywhere outside its own file
rg "GoogleSpeechtoTextAIProvider" --type=py -l
echo "---"
# Check registry for any gasr references
rg "gasr" --type=py -C3Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1042
🏁 Script executed:
#!/bin/bash
# Find registry.py and __init__.py files in the providers directory
find backend/app/services/llm/providers -name "registry.py" -o -name "__init__.py" | head -20
echo "---"
# Check the __init__.py in providers directory
cat -n backend/app/services/llm/providers/__init__.py 2>/dev/null || echo "File not found"
echo "---"
# Search for registry file
fd "registry" --type f backend/app/services/llm/Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Read the registry.py file
cat -n backend/app/services/llm/providers/registry.py
echo "---"
# Also check the entire __init__.py to see full content
wc -l backend/app/services/llm/providers/__init__.py
cat -n backend/app/services/llm/providers/__init__.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 6076
Provider class is not registered — it cannot be used through the factory.
GoogleSpeechtoTextAIProvider is not imported in __init__.py and not added to the _registry dict in registry.py. This means it cannot be instantiated via get_llm_provider() or appear in supported providers. Add the import to __init__.py and register it in the _registry dict with an appropriate provider type key (e.g., "google-stt-native").
🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/gasr.py` at line 26, The provider class
GoogleSpeechtoTextAIProvider is never registered so it can't be instantiated via
the factory; to fix it add an import for GoogleSpeechtoTextAIProvider into the
package __init__.py and add an entry to the _registry dict in registry.py
mapping a provider key (e.g., "google-stt-native") to
GoogleSpeechtoTextAIProvider so get_llm_provider() and supported providers can
construct it.
| transcript = "" | ||
| total_output_chars = 0 | ||
| if response.results: | ||
| for result in response.results: | ||
| if result.alternatives: | ||
| transcript += result.alternatives[0].transcript | ||
| total_output_chars += len(result.alternatives[0].transcript) |
There was a problem hiding this comment.
Transcript concatenation may merge words across result segments.
When multiple results are returned, concatenating result.alternatives[0].transcript without any separator (line 136) can produce fused words (e.g., "hello""world" → "helloworld"). Google STT v2 batch responses don't always include trailing whitespace in each segment.
Proposed fix
transcript = ""
total_output_chars = 0
if response.results:
for result in response.results:
if result.alternatives:
- transcript += result.alternatives[0].transcript
+ segment = result.alternatives[0].transcript
+ if transcript and segment and not transcript.endswith(" "):
+ transcript += " "
+ transcript += segment
- total_output_chars += len(result.alternatives[0].transcript)
+ total_output_chars += len(segment)🤖 Prompt for AI Agents
In `@backend/app/services/llm/providers/gasr.py` around lines 131 - 137, The code
concatenates Google STT result segments without separators which can fuse words;
in the gasr.py handler (the block using response.results and
result.alternatives[0].transcript, and variables transcript and
total_output_chars) update the concatenation logic to ensure a separator is
inserted when needed: before appending next_segment =
result.alternatives[0].transcript, check if transcript is non-empty and neither
transcript.endswith(whitespace) nor next_segment.startswith(whitespace) and if
so append a single space before next_segment, then update total_output_chars by
len(next_segment) (plus the added space if inserted); keep all other behaviors
the same.
3 participants
Summary
Target issue is #564
Explain the motivation for making this change. What existing problem does the pull request solve?
Sarvam STT
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
New Features
Tests