Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a new API endpoint and supporting database layers for retrieving Langfuse score analytics filtered by merchant ID and optional date ranges. The feature spans API routing, database accessors, and query builders with critical duplicate declarations across multiple files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
priyanshi-2003
force-pushed
the
observability-as-a-whole
branch
from
ea048ef to
beab3e7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/routers/breeze_buddy/merchants/__init__.py (1)
16-39: Critical: Authentication removed from admin-only endpoint.The authentication dependency has been completely removed from this route, matching the bypass in the handler. The docstring is now inconsistent with the implementation:
- Claims "Admin-only endpoint" but has no auth
- Documents "403: If user is not admin" but this error can never be raised
This should be addressed together with the handler changes.
🔎 Recommended fix
-from fastapi import APIRouter +from fastapi import APIRouter, Depends + +from app.core.security.rbac import get_current_user_with_rbac +from app.schemas import UserInfo from app.schemas.breeze_buddy.merchants import MerchantsResponse from .handlers import list_merchants_handler router = APIRouter() @router.get("/merchants", response_model=MerchantsResponse) async def list_merchants( - # TODO: Re-enable auth after testing - # current_user: UserInfo = Depends(get_current_user_with_rbac), + current_user: UserInfo = Depends(get_current_user_with_rbac), ): ... - # TODO: Re-enable auth after testing - return await list_merchants_handler(None) + return await list_merchants_handler(current_user)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/api/routers/breeze_buddy/deprecated/dashboard.pyapp/api/routers/breeze_buddy/merchants/__init__.pyapp/api/routers/breeze_buddy/merchants/handlers.pyapp/database/__init__.pyapp/database/accessor/__init__.pyapp/database/accessor/breeze_buddy/lead_call_tracker.pyapp/database/queries/breeze_buddy/lead_call_tracker.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T12:11:37.686Z
Learnt from: swaroopvarma1
Repo: juspay/clairvoyance PR: 447
File: app/database/queries/breeze_buddy/call_execution_config.py:268-285
Timestamp: 2025-12-23T12:11:37.686Z
Learning: Guideline: In Breeze Buddy, treat shop_identifier as identifying individual merchant locations, while merchant_id acts as a parent/group identifier. Therefore, functions that retrieve shop_identifiers should be named as 'merchants' functions (e.g., get_merchants_, list_merchants_). Apply this naming consistently across all Breeze Buddy query modules and related tests. This pattern should apply to all Python files under app/database/queries/breeze_buddy/ (and similar modules following the same domain naming) to maintain consistency across the codebase.
Applied to files:
app/database/queries/breeze_buddy/lead_call_tracker.py
🧬 Code graph analysis (5)
app/database/accessor/__init__.py (1)
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
get_langfuse_scores_by_merchant(459-505)
app/api/routers/breeze_buddy/deprecated/dashboard.py (1)
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
get_langfuse_scores_by_merchant(459-505)
app/api/routers/breeze_buddy/merchants/__init__.py (1)
app/api/routers/breeze_buddy/merchants/handlers.py (1)
list_merchants_handler(13-53)
app/api/routers/breeze_buddy/merchants/handlers.py (1)
app/database/accessor/breeze_buddy/call_execution_config.py (1)
get_all_merchants(304-331)
app/database/accessor/breeze_buddy/lead_call_tracker.py (2)
app/database/queries/breeze_buddy/lead_call_tracker.py (1)
get_langfuse_scores_by_merchant_query(435-487)app/database/queries/__init__.py (1)
run_parameterized_query(14-32)
🪛 Ruff (0.14.10)
app/api/routers/breeze_buddy/deprecated/dashboard.py
354-354: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/database/accessor/breeze_buddy/lead_call_tracker.py
502-502: Consider moving this statement to an else block
(TRY300)
503-503: Do not catch blind exception: Exception
(BLE001)
app/database/queries/breeze_buddy/lead_call_tracker.py
468-486: Possible SQL injection vector through string-based query construction
(S608)
🔇 Additional comments (5)
app/database/queries/breeze_buddy/lead_call_tracker.py (1)
435-487: LGTM - Query function is correctly parameterized.The static analysis SQL injection warning (S608) is a false positive here. All user-provided values (
merchant_id,start_date,end_date) are properly bound via parameterized placeholders ($1,$2, etc.), and no user input is directly interpolated into the SQL string. The dynamic WHERE clause construction only uses hardcoded column names with parameter placeholders.The aggregation logic correctly handles edge cases with
NULLIF(COUNT(*), 0)to prevent division by zero.app/database/__init__.py (1)
46-48: Fallback logic is reasonable for development, but verify production behavior.The fallback to the original password when KMS decryption fails is sensible for local development. However, ensure that in production environments, KMS decryption failures are monitored/alerted on, as silently falling back could mask configuration issues or indicate a security degradation.
app/database/accessor/__init__.py (1)
19-19: LGTM!The new
get_langfuse_scores_by_merchantfunction is correctly imported and exported, following the established pattern in this module.Also applies to: 76-76
app/database/accessor/breeze_buddy/lead_call_tracker.py (2)
459-505: LGTM - New accessor function follows established patterns.The implementation is consistent with other accessor functions in this file:
- Proper logging at entry
- Parameterized query usage
- Exception handling with logging
- Safe handling of None values in
pass_rate_percentconversionThe static analysis hints (TRY300, BLE001) apply equally to other functions in this file. If you want to address them, it would be better done as a broader refactor across the module.
16-16: Import addition is correct.The new query function import aligns with the new accessor function added below.
| @router.get("/breeze/langfuse-scores") | ||
| async def get_langfuse_scores_analytics( | ||
| start_date: Optional[str] = None, | ||
| end_date: Optional[str] = None, | ||
| merchant_id: Optional[str] = None, | ||
| # TODO: Re-enable auth after testing | ||
| # session: dict = Depends(get_breeze_buddy_session), | ||
| ): |
There was a problem hiding this comment.
Critical: Authentication is disabled on a new analytics endpoint.
This new endpoint exposes Langfuse analytics data without any authentication. The commented-out session dependency and auth check should not be merged to the release branch in this state.
🔎 Recommended fix
@router.get("/breeze/langfuse-scores")
async def get_langfuse_scores_analytics(
start_date: Optional[str] = None,
end_date: Optional[str] = None,
merchant_id: Optional[str] = None,
- # TODO: Re-enable auth after testing
- # session: dict = Depends(get_breeze_buddy_session),
+ session: dict = Depends(get_breeze_buddy_session),
):
...
- # TODO: Re-enable auth after testing
- # if not session:
- # raise HTTPException(status_code=401, detail="Not authenticated")
+ if not session:
+ raise HTTPException(status_code=401, detail="Not authenticated")Also applies to: 333-335
🤖 Prompt for AI Agents
In app/api/routers/breeze_buddy/deprecated/dashboard.py around lines 289-296
(and also lines 333-335), the new Langfuse analytics endpoints have
authentication disabled; restore the commented-out session dependency and
enforce auth: add the parameter session: dict =
Depends(get_breeze_buddy_session) to the endpoint signature, validate the
session (or required permissions) at the start of the handler and raise an
HTTPException(401/403) on failure, and remove any temporary bypasses so the
endpoint only returns data for authenticated/authorized users; run/update
related tests to cover auth behavior.
| # Calculate summary | ||
| total_calls = max([s["total_evaluated"] for s in scores_data], default=0) |
There was a problem hiding this comment.
Incorrect logic for total_calls_evaluated calculation.
Using max() on total_evaluated across evaluators assumes that all evaluators are applied to the same set of calls. If different evaluators are applied to different subsets of calls, this metric would be misleading.
Consider clarifying the semantic meaning or using a more accurate calculation:
🔎 Alternative approaches
Option 1 - If you want the maximum evaluations by any single evaluator:
# Current (keep as-is but rename for clarity)
max_evaluations = max([s["total_evaluated"] for s in scores_data], default=0)Option 2 - If you want total unique calls evaluated (requires query change):
# Would need a separate query to count DISTINCT call_ids where langfuse_scores IS NOT NULLOption 3 - Sum of all evaluations:
total_evaluations = sum(s["total_evaluated"] for s in scores_data)🤖 Prompt for AI Agents
In app/api/routers/breeze_buddy/deprecated/dashboard.py around lines 373-374,
the calculation using max([s["total_evaluated"] for s in scores_data],
default=0) is misleading if evaluators cover different call subsets; either (A)
if you intended the maximum evaluations by a single evaluator, rename the
variable to max_evaluations and keep the max logic, (B) if you intended the
total number of evaluations across all evaluators, replace with a sum of
s["total_evaluated"], or (C) if you intended the number of unique calls
evaluated, change the data source/query to return COUNT(DISTINCT call_id) where
scores exist and use that value here; update the variable name and any
downstream usage to match the chosen semantics.
| # TODO: Re-enable auth after testing | ||
| # Validate admin access | ||
| require_admin_access(current_user, "list merchants") | ||
| # require_admin_access(current_user, "list merchants") | ||
|
|
||
| logger.info(f"Admin user {current_user.username} requesting merchants list") | ||
| # logger.info(f"Admin user {current_user.username} requesting merchants list") | ||
| logger.info("Requesting merchants list (auth bypassed for testing)") | ||
|
|
||
| try: | ||
| # Get all unique merchants from database | ||
| merchants = await get_all_merchants() | ||
|
|
||
| logger.info( | ||
| f"Returning {len(merchants)} merchants to admin {current_user.username}" | ||
| ) | ||
| # TODO: Re-enable auth after testing | ||
| # logger.info( | ||
| # f"Returning {len(merchants)} merchants to admin {current_user.username}" | ||
| # ) | ||
| logger.info(f"Returning {len(merchants)} merchants (auth bypassed)") |
There was a problem hiding this comment.
Critical: Authentication is disabled on an admin-only endpoint.
Disabling authentication on an endpoint that exposes merchant data is a significant security risk. Even for testing purposes, this should not be merged to the release branch with auth bypassed.
Issues:
- The
require_admin_accesscall is commented out, allowing unauthenticated access - The function signature claims
current_user: UserInfobut acceptsNone - The docstring still states "Admin-only endpoint" and "must be admin", which is now misleading
🔎 Recommended approach
Consider one of these alternatives:
- Use feature flags: Enable auth bypass only via environment variable in non-production environments
- Separate test routes: Create a distinct test router that's excluded from production builds
- Re-enable auth before merge: If this is truly for testing, create a separate testing branch
+import os
+
+TESTING_MODE = os.getenv("TESTING_MODE", "false").lower() == "true"
+
async def list_merchants_handler(current_user: UserInfo) -> MerchantsResponse:
- # TODO: Re-enable auth after testing
- # Validate admin access
- # require_admin_access(current_user, "list merchants")
+ # Only bypass auth in explicit testing mode
+ if not TESTING_MODE:
+ require_admin_access(current_user, "list merchants")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/api/routers/breeze_buddy/merchants/handlers.py around lines 29 to 44,
authentication has been bypassed for testing: the require_admin_access call is
commented out, logger messages note "auth bypassed", and the function accepts a
current_user annotated as UserInfo but may be None while the docstring still
claims "Admin-only endpoint". Restore secure behavior by re-enabling
require_admin_access for production code (uncomment the call), ensure
current_user is validated and typed as non-Optional UserInfo (or add a runtime
check that returns 401/403 if missing), update or remove "auth bypassed" log
lines, and change the docstring to accurately reflect that this is an admin-only
endpoint; if you must support testing bypass, gate it behind a clear
non-production feature flag or a separate test-only route so no auth bypass
reaches release branches.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/api/routers/breeze_buddy/deprecated/dashboard.pyapp/database/accessor/__init__.pyapp/database/accessor/breeze_buddy/lead_call_tracker.pyapp/database/queries/breeze_buddy/lead_call_tracker.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-23T12:11:37.686Z
Learnt from: swaroopvarma1
Repo: juspay/clairvoyance PR: 447
File: app/database/queries/breeze_buddy/call_execution_config.py:268-285
Timestamp: 2025-12-23T12:11:37.686Z
Learning: Guideline: In Breeze Buddy, treat shop_identifier as identifying individual merchant locations, while merchant_id acts as a parent/group identifier. Therefore, functions that retrieve shop_identifiers should be named as 'merchants' functions (e.g., get_merchants_, list_merchants_). Apply this naming consistently across all Breeze Buddy query modules and related tests. This pattern should apply to all Python files under app/database/queries/breeze_buddy/ (and similar modules following the same domain naming) to maintain consistency across the codebase.
Applied to files:
app/database/queries/breeze_buddy/lead_call_tracker.py
🧬 Code graph analysis (2)
app/database/accessor/__init__.py (1)
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
get_langfuse_scores_by_merchant(459-505)
app/api/routers/breeze_buddy/deprecated/dashboard.py (1)
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
get_langfuse_scores_by_merchant(459-505)
🪛 Ruff (0.14.10)
app/database/accessor/breeze_buddy/lead_call_tracker.py
502-502: Consider moving this statement to an else block
(TRY300)
503-503: Do not catch blind exception: Exception
(BLE001)
app/database/queries/breeze_buddy/lead_call_tracker.py
468-486: Possible SQL injection vector through string-based query construction
(S608)
app/api/routers/breeze_buddy/deprecated/dashboard.py
354-354: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (4)
app/database/accessor/__init__.py (1)
19-19: LGTM!The new
get_langfuse_scores_by_merchantfunction is properly imported and exported in the public interface.Also applies to: 76-76
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
459-505: LGTM!The new accessor function follows the established patterns in this module:
- Logging input parameters
- Delegating to the query builder
- Mapping results to a well-defined dict structure
- Consistent error handling with graceful fallback to empty list
app/database/queries/breeze_buddy/lead_call_tracker.py (1)
435-487: LGTM!The query function is well-structured:
- Uses parameterized queries (
$1,$2, etc.) to prevent SQL injection (the static analysis S608 warning is a false positive since table names are constants)- The CTE correctly flattens the JSONB array of scores
- Aggregation logic for pass/fail counts and percentages is correct
app/api/routers/breeze_buddy/deprecated/dashboard.py (1)
373-374: Verify thetotal_calls_evaluatedcalculation logic.Using
max()assumes all evaluators are applied to the same set of calls. If evaluators can be applied to different subsets of calls, this would be misleading. Consider whethertotal_calls_evaluatedshould represent the count of unique calls evaluated (distinct call_ids) rather than the maximum evaluator count.
| @router.get("/breeze/langfuse-scores") | ||
| async def get_langfuse_scores_analytics( | ||
| start_date: Optional[str] = None, | ||
| end_date: Optional[str] = None, | ||
| merchant_id: Optional[str] = None, | ||
| # TODO: Re-enable auth after testing | ||
| # session: dict = Depends(get_breeze_buddy_session), | ||
| ): |
There was a problem hiding this comment.
Critical: Authentication is disabled on this endpoint.
The endpoint lacks authentication (get_breeze_buddy_session dependency is commented out). This exposes potentially sensitive Langfuse score analytics data without any access control. Other endpoints in this file require authentication (lines 27, 173).
This should not be merged to the release branch without re-enabling authentication.
🔎 Proposed fix to re-enable authentication
@router.get("/breeze/langfuse-scores")
async def get_langfuse_scores_analytics(
start_date: Optional[str] = None,
end_date: Optional[str] = None,
merchant_id: Optional[str] = None,
- # TODO: Re-enable auth after testing
- # session: dict = Depends(get_breeze_buddy_session),
+ session: dict = Depends(get_breeze_buddy_session),
):And uncomment the session check at lines 333-335:
- # TODO: Re-enable auth after testing
- # if not session:
- # raise HTTPException(status_code=401, detail="Not authenticated")
+ if not session:
+ raise HTTPException(status_code=401, detail="Not authenticated")🤖 Prompt for AI Agents
In app/api/routers/breeze_buddy/deprecated/dashboard.py around lines 289-296,
the get_langfuse_scores_analytics endpoint has authentication disabled (the
get_breeze_buddy_session dependency is commented out), exposing sensitive
analytics; re-enable auth by uncommenting and restoring the session dependency
in the function signature (add session: dict =
Depends(get_breeze_buddy_session)), and also uncomment and enforce the session
check at lines 333-335 so the endpoint aborts with proper error/redirect when
the session is missing or unauthorized; ensure imports and typing remain correct
and run tests/lint.
| Query Parameters: | ||
| - start_date: Start date in ISO format (YYYY-MM-DD) - Required | ||
| - end_date: End date in ISO format (YYYY-MM-DD) - Required | ||
| - merchant_id: Optional merchant ID to filter by |
There was a problem hiding this comment.
Docstring states dates are "Required" but parameters are Optional.
The docstring on lines 301-302 claims start_date and end_date are "Required", but the function signature declares them as Optional[str] = None. Either enforce the requirement or fix the documentation.
🔎 Proposed fix (if dates should remain optional)
Query Parameters:
- - start_date: Start date in ISO format (YYYY-MM-DD) - Required
- - end_date: End date in ISO format (YYYY-MM-DD) - Required
+ - start_date: Start date in ISO format (YYYY-MM-DD) - Optional
+ - end_date: End date in ISO format (YYYY-MM-DD) - Optional
- merchant_id: Optional merchant ID to filter by📝 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.
| Query Parameters: | |
| - start_date: Start date in ISO format (YYYY-MM-DD) - Required | |
| - end_date: End date in ISO format (YYYY-MM-DD) - Required | |
| - merchant_id: Optional merchant ID to filter by | |
| Query Parameters: | |
| - start_date: Start date in ISO format (YYYY-MM-DD) - Optional | |
| - end_date: End date in ISO format (YYYY-MM-DD) - Optional | |
| - merchant_id: Optional merchant ID to filter by |
priyanshi-2003
force-pushed
the
observability-as-a-whole
branch
2 times, most recently
from
128bb97 to
cf65566
Compare
| ) | ||
|
|
||
|
|
||
| @router.get("/langfuse-scores") |
There was a problem hiding this comment.
analytics/evaluator-scores
priyanshi-2003
force-pushed
the
observability-as-a-whole
branch
3 times, most recently
from
d81aa07 to
9848ff2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
priyanshi-2003
force-pushed
the
observability-as-a-whole
branch
3 times, most recently
from
f605d8b to
1d6c251
Compare
| from datetime import datetime, timedelta, timezone | ||
|
|
||
| from fastapi import HTTPException |
priyanshi-2003
force-pushed
the
observability-as-a-whole
branch
from
1d6c251 to
0caf864
Compare
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.