Base64 to be deprecated while image upload - #44
Conversation
Perf timing logs added for debugging story PDF generation cluttered prod logs; stripped them, keeping the underlying base64/import fixes.
Adding haiku and sonnet model id
Adding haiku and sonnet model id
…tra-service into fix/base64_dep
📝 WalkthroughWalkthroughChangesLLM model routing
Story media cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CompanyBot
participant LLMHandlers
participant ModelProvider
CompanyBot->>LLMHandlers: provide custom_model
LLMHandlers->>LLMHandlers: resolve model_to_use or model_id
LLMHandlers->>ModelProvider: send request with resolved model
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
VishnuKrishnathu
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
Action performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@chatbot/llm_models/llm_script.py`:
- Around line 29-38: Update get_custom_model to safely handle malformed or
non-object other_params: catch JSON decoding failures, validate that parsed
values are mappings before calling .get(), and return None (or the established
controlled configuration error) for invalid optional settings so callers remain
on their normal error path.
- Around line 261-264: Update get_pricing_from_company_bot and its pricing data
so supported Bedrock Claude model IDs, including the newly listed Haiku and
Sonnet IDs, have exact pricing entries; when pricing_data.get(model_id) misses,
return None instead of falling back to the llama3-3-70b entry. Preserve existing
exact-match pricing behavior for all other models.
- Around line 116-119: Validate the custom model selected in the
model-resolution flow before assigning it to provider-specific handlers. In the
logic around get_custom_model, handle_openai_model, handle_openai_response_api,
and handle_bedrock_model, check other_params['custom_model'] and
company_bot.llm_model so only a model compatible with the selected provider is
passed as model or modelId; otherwise continue using the provider-appropriate
configured model.
In `@chatbot/utils/shikshalokam_story_utils.py`:
- Around line 435-437: Remove the three debug print calls for profile, story
title, and story.formatted_content from the PDF update flow. Do not emit user or
story data to stdout; retain existing behavior and use only redacted structured
logging if diagnostics are required elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 536aaf28-ecf8-4c91-84e2-d325a7cddec4
📒 Files selected for processing (6)
chatbot/llm_models/llm_script.pychatbot/models/enums.pychatbot/models/story_models.pychatbot/utils/shikshalokam_story_utils.pychatbot/views/story_views.pyshikshalokam_mohini/settings.py
| def get_custom_model(company_bot): | ||
| """Return company_bot.other_params['custom_model'] if set, else None.""" | ||
| if not company_bot: | ||
| return None | ||
| other_params = company_bot.get('other_params') if isinstance(company_bot, dict) else getattr( | ||
| company_bot, 'other_params', None | ||
| ) | ||
| if isinstance(other_params, str): | ||
| other_params = json.loads(other_params) | ||
| return other_params.get('custom_model') if other_params else None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle malformed other_params before parsing it.
json.loads at Line 37 can raise json.JSONDecodeError. A valid JSON list, string, or number can then reach Line 38 without a .get() method. The Bedrock and Responses API handlers call this helper before their try blocks. A malformed optional setting can therefore abort the request instead of using the normal error path.
Catch parse errors, require an object, and return None or a controlled configuration error.
Proposed fix
def get_custom_model(company_bot):
"""Return company_bot.other_params['custom_model'] if set, else None."""
if not company_bot:
return None
other_params = company_bot.get('other_params') if isinstance(company_bot, dict) else getattr(
company_bot, 'other_params', None
)
if isinstance(other_params, str):
- other_params = json.loads(other_params)
- return other_params.get('custom_model') if other_params else None
+ try:
+ other_params = json.loads(other_params)
+ except json.JSONDecodeError:
+ return None
+ if not isinstance(other_params, dict):
+ return None
+ custom_model = other_params.get('custom_model')
+ if not isinstance(custom_model, str):
+ return None
+ return custom_model.strip() or None📝 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.
| def get_custom_model(company_bot): | |
| """Return company_bot.other_params['custom_model'] if set, else None.""" | |
| if not company_bot: | |
| return None | |
| other_params = company_bot.get('other_params') if isinstance(company_bot, dict) else getattr( | |
| company_bot, 'other_params', None | |
| ) | |
| if isinstance(other_params, str): | |
| other_params = json.loads(other_params) | |
| return other_params.get('custom_model') if other_params else None | |
| def get_custom_model(company_bot): | |
| """Return company_bot.other_params['custom_model'] if set, else None.""" | |
| if not company_bot: | |
| return None | |
| other_params = company_bot.get('other_params') if isinstance(company_bot, dict) else getattr( | |
| company_bot, 'other_params', None | |
| ) | |
| if isinstance(other_params, str): | |
| try: | |
| other_params = json.loads(other_params) | |
| except json.JSONDecodeError: | |
| return None | |
| if not isinstance(other_params, dict): | |
| return None | |
| custom_model = other_params.get('custom_model') | |
| if not isinstance(custom_model, str): | |
| return None | |
| return custom_model.strip() or None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@chatbot/llm_models/llm_script.py` around lines 29 - 38, Update
get_custom_model to safely handle malformed or non-object other_params: catch
JSON decoding failures, validate that parsed values are mappings before calling
.get(), and return None (or the established controlled configuration error) for
invalid optional settings so callers remain on their normal error path.
There was a problem hiding this comment.
It's okay. Don't want to touch old code
There was a problem hiding this comment.
@VishnuKrishnathu, acknowledged. I will leave this change out of PR #44.
The malformed other_params case remains a follow-up risk in the existing code. If you later want to address it, I can help create a separate issue.
🐇︵💫
You are interacting with an AI system.
| custom_model = get_custom_model(company_bot) | ||
| if custom_model: | ||
| model_to_use = custom_model | ||
| elif model_name: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'custom_model|LLMProvider|company_bot\.llm_model|handle_openai_model|handle_bedrock_model|handle_openai_response_api' \
--glob '*.py' .Repository: ELEVATE-Project/mitra-service
Length of output: 50388
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== llm_script outline =="
ast-grep outline chatbot/llm_models/llm_script.py --view expanded | sed -n '1,220p'
echo
echo "== llm_script relevant lines =="
sed -n '1,180p' chatbot/llm_models/llm_script.py | cat -n
sed -n '240,280p' chatbot/llm_models/llm_script.py | cat -n
sed -n '620,660p' chatbot/llm_models/llm_script.py | cat -n
echo
echo "== focused provider/model usages in llm_script.py =="
rg -n -C 3 'def get_custom_model|get_custom_model|LLMProvider|model_to_use|model_name|provider|model_id|modelId|invoke_model|chat completions|OpenAI' chatbot/llm_models/llm_script.pyRepository: ELEVATE-Project/mitra-service
Length of output: 24350
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== LLMModel enum and references =="
rg -n "class LLMModel|LLMModel\." chatbot/models chatbot --glob '*.py' | sed -n '1,240p'
echo
echo "== get_custom_model read-only semantic probe =="
python3 - <<'PY'
class CompanyBotLike:
def __init__(self, other_params=None):
self.other_params = other_params
def get_custom_model(company_bot):
other_params = company_bot.get('other_params') if isinstance(company_bot, dict) else getattr(
company_bot, 'other_params', None
)
if isinstance(other_params, str):
import json
other_params = json.loads(other_params)
return other_params.get('custom_model') if other_params else None
samples = [
{'other_params': {'custom_model': 'anthropic.claude-sonnet-4-20250514-v1:0'}},
{'other_params': '{"custom_model":"anthropic.claude-sonnet-4-20250514-v1:0"}'},
type('Obj', (), {'other_params': {'custom_model': 'gpt-4.1-mini'}}),
]
for company_bot in samples:
model_name = 'gsm8k-template'
company_bot_llm_model = 'llama3-1-8b'
custom_model = get_custom_model(company_bot)
open_ai_model_to_use = custom_model or model_name or company_bot_llm_model
bedrock_model_id = custom_model or model_name or None
print({
'input_type': type(company_bot).__name__,
'provider_agnostic_model_value': custom_model,
'fallback_to_caller_model_name': model_name,
'open_ai_openai_model': open_ai_model_to_use,
'bedrock_model_id': bedrock_model_id,
})
PYRepository: ELEVATE-Project/mitra-service
Length of output: 2080
Validate the resolved model against the selected provider.
get_custom_model can return a Bedrock model ID, but handle_openai_model and handle_openai_response_api pass it directly as model to OpenAI. handle_bedrock_model has the same provider-neutral override behavior. Check other_params['custom_model'] and company_bot.llm_model before these handlers assign model/modelId.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@chatbot/llm_models/llm_script.py` around lines 116 - 119, Validate the custom
model selected in the model-resolution flow before assigning it to
provider-specific handlers. In the logic around get_custom_model,
handle_openai_model, handle_openai_response_api, and handle_bedrock_model, check
other_params['custom_model'] and company_bot.llm_model so only a model
compatible with the selected provider is passed as model or modelId; otherwise
continue using the provider-appropriate configured model.
There was a problem hiding this comment.
It's okay. Don't want to touch old code
There was a problem hiding this comment.
@VishnuKrishnathu, acknowledged. I will leave this provider-validation change out of PR #44.
🐇︵︵
You are interacting with an AI system.
| custom_model = get_custom_model(company_bot) | ||
| if custom_model: | ||
| model_id = custom_model | ||
| elif model_name: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'model_pricing|llama3-3-70b|claude-haiku-4-5-20251001-v1:0|claude-sonnet-4-5-20250929-v1:0' \
--glob '*.py' \
--glob '*.json' \
--glob '*.yaml' \
--glob '*.yml' .Repository: ELEVATE-Project/mitra-service
Length of output: 34328
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Relevant llm_script sections:"
sed -n '250,350p' chatbot/llm_models/llm_script.py
echo "---"
rg -n "get_pricing_from_company_bot|handle_bedrock_model|model_pricing|LLAMA_3_3_70B_INSTRUCT|CLAUDE" chatbot/observability chatbot --glob '*.py' | head -200Repository: ELEVATE-Project/mitra-service
Length of output: 26416
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Pricing exact-miss fallback paths:"
sed -n '179,222p' chatbot/llm_models/llm_script.py
echo "--- guest script duplicate"
sed -n '332,370p' chatbot/scripts/guest_discussion/onetime_script.py
echo "--- get_custom_model references"
rg -n "def get_custom_model|get_custom_model\\(" chatbot --glob '*.py'
echo "--- model_pricing keys in tracked data files"
rg -n -C 3 "model_pricing|llama3-3-70b|us\.anthropic\.claude-haiku-4-5|us\.anthropic\.claude-sonnet-4-5|claude-haiku-4-5|claude-sonnet-4-5" \
--glob '*.py' --glob '*.json' --glob '*.yaml' --glob '*.yml' --glob '*.md' --glob '*.txt' . | head -250Repository: ELEVATE-Project/mitra-service
Length of output: 32109
Do not fall back to Llama pricing for unmatched Bedrock models.
get_pricing_from_company_bot uses pricing_data.get(model_id) and then looks up pricing_data.get('llama3-3-70b'). New Claude IDs such as us.anthropic.claude-haiku-4-5-20251001-v1:0 and us.anthropic.claude-sonnet-4-5-20250929-v1:0 can therefore log incorrect costs if exact pricing entries are missing.
Add exact pricing keys for each supported model, or return None on exact miss.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@chatbot/llm_models/llm_script.py` around lines 261 - 264, Update
get_pricing_from_company_bot and its pricing data so supported Bedrock Claude
model IDs, including the newly listed Haiku and Sonnet IDs, have exact pricing
entries; when pricing_data.get(model_id) misses, return None instead of falling
back to the llama3-3-70b entry. Preserve existing exact-match pricing behavior
for all other models.
There was a problem hiding this comment.
It's okay. Don't want to touch old code
There was a problem hiding this comment.
@VishnuKrishnathu, acknowledged. I will keep this existing pricing fallback behavior unchanged in this PR.
🐇 …
You are interacting with an AI system.
| print("profile: ", profile) | ||
| print("story: ", story.title) | ||
| print("story format: ", story.formatted_content) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove user data from PDF-flow debug output.
These print calls write profile data and user-generated story content to stdout during every PDF update. This conflicts with the production log cleanup objective and can expose sensitive data. Remove the calls. Use redacted structured logging only when diagnostics are required.
Proposed fix
- print("profile: ", profile)
- print("story: ", story.title)
- print("story format: ", story.formatted_content)📝 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.
| print("profile: ", profile) | |
| print("story: ", story.title) | |
| print("story format: ", story.formatted_content) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@chatbot/utils/shikshalokam_story_utils.py` around lines 435 - 437, Remove the
three debug print calls for profile, story title, and story.formatted_content
from the PDF update flow. Do not emit user or story data to stdout; retain
existing behavior and use only redacted structured logging if diagnostics are
required elsewhere.
There was a problem hiding this comment.
It's okay. Don't want to touch old code
There was a problem hiding this comment.
@VishnuKrishnathu, acknowledged. I will leave the existing PDF-flow debug output unchanged in this PR.
🐇︵︵
You are interacting with an AI system.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores