Removing proxy for daily #318
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 WalkthroughThe PR modifies CI/CD workflows to implement multi-step AWS role fallback logic for ECR authentication, introduces a new fallback Docker build workflow, removes proxy configuration from Daily WebRTC connections, and refactors call finalization logic in OrderConfirmationBot to centralize disconnect and cleanup handling. Changes
Sequence DiagramsequenceDiagram
participant GH as GitHub Actions
participant Auth as AWS Auth
participant ECR as Amazon ECR
participant Img as Docker Image
rect rgb(200, 220, 250)
Note over GH,Auth: Primary: OIDC Prod Role
GH->>Auth: Assume Prod Primary Role (continue-on-error)
alt Primary Succeeds
Auth-->>GH: Credentials OK
else Primary Fails
rect rgb(240, 200, 200)
Note over GH,Auth: Fallback 1: Beta Account
GH->>Auth: Assume Beta Account Role
alt Beta Succeeds
Auth-->>GH: Credentials OK
else Beta Fails
rect rgb(240, 200, 200)
Note over GH,Auth: Fallback 2: Prod GitHub Role
GH->>Auth: Assume Prod GitHub Role
Auth-->>GH: Credentials OK
end
end
end
end
end
GH->>ECR: Select Registry (Prod primary > Beta > Prod GitHub)
ECR-->>GH: Registry selected
GH->>Img: Login to ECR
GH->>Img: Build & Push Image
Img-->>GH: Success
sequenceDiagram
participant Client as WebSocket Client
participant Bot as OrderConfirmationBot
participant Handler as _handle_unexpected_disconnect
participant Finalizer as _finalize_call
participant Webhook as Webhook Service
participant DB as Database
Client->>Bot: Disconnect / Idle Timeout
Bot->>Handler: _handle_unexpected_disconnect(reason)
Handler->>Bot: Mark conversation ended
Handler->>Bot: Set outcome to BUSY
Handler->>Finalizer: _finalize_call()
Finalizer->>Finalizer: Build transcription from context
Finalizer->>Finalizer: Normalize outcome via OUTCOME_TO_ENUM
Finalizer->>Finalizer: Construct summary_data
alt Outcome ≠ BUSY
Finalizer->>Webhook: POST webhook
alt Webhook succeeds
Webhook-->>Finalizer: OK
else Webhook fails
Finalizer->>Finalizer: Handle error
end
end
Finalizer->>Bot: Hangup
Finalizer->>DB: Update with outcome & transcription
DB-->>Finalizer: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The PR introduces significant heterogeneous changes across infrastructure and application logic. The three workflow modifications add multi-step conditional AWS authentication fallbacks requiring careful validation of role assumptions and registry selection logic. The OrderConfirmationBot refactoring consolidates scattered exception handling and disconnect branches into new abstractions, necessitating detailed tracing of control flow before and after. Session management changes in main.py require understanding of proxy bypass requirements for Daily. The variety and logic density across these domains demand careful, deliberate review. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
![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/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py (1)
691-693: Broken node reference: 'final_order_confirmation' not definedThis node is not present in flow_config and will raise at runtime. Use an existing end node.
- return {}, self._create_node_from_config("final_order_confirmation") + return {}, self._create_node_from_config("order_confirmation_and_end")
🧹 Nitpick comments (5)
app/main.py (3)
79-84: Ensure the Daily session never picks up env proxiesMake the intent explicit by disabling environment proxy trust on this session.
Apply:
- daily_aiohttp_session = aiohttp.ClientSession() + daily_aiohttp_session = aiohttp.ClientSession(trust_env=False)Based on learnings
91-93: General aiohttp_session is created but not usedEither expose it via app.state for reuse across the app, or drop it to avoid an unused resource.
Option A (expose for DI):
- aiohttp_session = create_aiohttp_session() + aiohttp_session = create_aiohttp_session() + _app.state.aiohttp_session = aiohttp_sessionAlso close conditionally on shutdown:
- await aiohttp_session.close() + if hasattr(_app.state, "aiohttp_session"): + await _app.state.aiohttp_session.close()Option B (remove if truly unused): delete creation and closing lines.
136-140: Session shutdown ordering/logging LGTMBoth sessions are closed; if adopting app.state, guard with hasattr as above.
app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py (2)
313-319: Idle-timeout handler: unused parameterRename the unused parameter to underscore to satisfy linters and signal intent.
- async def on_idle_timeout(task): + async def on_idle_timeout(_):Static analysis: ARG001
443-445: Overly broad exception around webhook sendCatching Exception hides actionable failures. Narrow to aiohttp.ClientError and asyncio.TimeoutError, and include response context.
- except Exception as e: - logger.error(f"Error sending webhook: {e}") + except (asyncio.TimeoutError, Exception) as e: + # optionally: from aiohttp import ClientError and catch it too + logger.error(f"Error sending webhook: {e}")If aiohttp.ClientError is available in scope, prefer that over bare Exception.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/aws-prod-docker.yml(2 hunks).github/workflows/docker-build-fallback.yml(1 hunks).github/workflows/docker-build-only.yml(1 hunks)app/agents/voice/automatic/__init__.py(1 hunks)app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py(3 hunks)app/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/main.py (1)
app/core/transport/http_client.py (1)
create_aiohttp_session(54-73)
app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py (2)
app/schemas.py (1)
LeadCallOutcome(37-43)app/core/security/sha.py (1)
calculate_hmac_sha256(6-27)
🪛 Ruff (0.14.0)
app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py
316-316: Unused function argument: task
(ARG001)
443-443: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
app/agents/voice/automatic/__init__.py (1)
370-374: Good call: skip proxy for Daily WebRTCDirect WebRTC avoids proxy pitfalls; this aligns with the non‑proxy Daily REST session. No further changes needed here.
app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py (1)
311-314: Centralized disconnect handling LGTMDelegating to _handle_unexpected_disconnect reduces duplication and keeps DB/webhook logic in one place.
| pull_request: | ||
| branches: | ||
| - release # triggers when PR is created to release branch | ||
| pull_request_target: | ||
| branches: | ||
| - release # triggers on pushes to PR branches targeting release |
There was a problem hiding this comment.
Blocker: pull_request_target + PR head checkout leaks prod AWS creds
Running this job on pull_request_target while checking out the untrusted PR head lets forked contributors execute arbitrary code right before we assume production AWS roles via OIDC. That hands full ECR access to anyone opening a PR, which is a critical supply-chain/security break.
Please stop using pull_request_target here (or at minimum gate the job so it never runs when github.event.pull_request.head.repo.full_name != github.repository) before assuming AWS creds.
Example guard:
on:
- pull_request:
+ pull_request:
branches:
- release
- pull_request_target:
- branches:
- - release # triggers on pushes to PR branches targeting release
+ pull_request_target:
+ branches:
+ - release
+
jobs:
build-and-push:
runs-on: ubuntu-latest
+ if: github.event_name != 'pull_request_target' || github.event.pull_request.head.repo.full_name == github.repositoryAlso applies to: 29-34, 35-73
🤖 Prompt for AI Agents
.github/workflows/aws-prod-docker.yml around lines 7 to 12: the workflow uses
pull_request_target which checks out untrusted PR head and can leak production
AWS creds; remove the pull_request_target trigger (or at minimum add a guard so
the job never proceeds when github.event.pull_request.head.repo.full_name !=
github.repository) and ensure any steps that assume AWS/OIDC credentials are
only executed for trusted refs (e.g., release branch or same-repo PRs); apply
the same change to the other affected blocks (lines 29-34 and 35-73) so no
credentials are assumed for forked PRs.
| on: | ||
| push: | ||
| branches: | ||
| - release | ||
| pull_request: | ||
| branches: | ||
| - release | ||
| pull_request_target: | ||
| branches: | ||
| - release | ||
| workflow_dispatch: # Allow manual trigger | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| build-and-push: | ||
| runs-on: ubuntu-latest | ||
| if: github.repository == 'juspay/clairvoyance' # Only run on main repo | ||
|
|
||
| env: | ||
| AWS_REGION: ap-south-1 | ||
| AWS_REPO: clairvoyance | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | ||
|
|
||
| # Try multiple authentication methods | ||
| - name: Configure AWS credentials (Method 1 - OIDC) | ||
| id: aws-oidc | ||
| continue-on-error: true | ||
| uses: aws-actions/configure-aws-credentials@v4 | ||
| with: | ||
| role-to-assume: arn:aws:iam::671255721112:role/breeze-clairvoyance-ecr-role | ||
| aws-region: ${{ env.AWS_REGION }} | ||
|
|
||
| - name: Configure AWS credentials (Method 2 - Access Keys) | ||
| if: steps.aws-oidc.outcome == 'failure' | ||
| continue-on-error: true | ||
| uses: aws-actions/configure-aws-credentials@v4 | ||
| with: | ||
| aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| aws-region: ${{ env.AWS_REGION }} | ||
|
|
||
| - name: Configure AWS credentials (Method 3 - Alternative Role) | ||
| if: steps.aws-oidc.outcome == 'failure' | ||
| continue-on-error: true | ||
| uses: aws-actions/configure-aws-credentials@v4 | ||
| with: | ||
| role-to-assume: arn:aws:iam::671255721112:role/github-actions-role | ||
| aws-region: ${{ env.AWS_REGION }} | ||
|
|
There was a problem hiding this comment.
Blocker: Fallback workflow gifts AWS creds to forked PRs
pull_request_target runs with base-repo secrets. We then check out the fork’s head SHA and immediately assume prod/beta AWS roles (and even try static keys). A malicious fork PR can exfiltrate or abuse those credentials.
Remove the pull_request_target trigger (or guard the job with a head-repo equality check) before fetching AWS creds.
Suggested guard:
on:
pull_request:
branches:
- release
- pull_request_target:
- branches:
- - release
+ pull_request_target:
+ branches:
+ - release
...
build-and-push:
runs-on: ubuntu-latest
- if: github.repository == 'juspay/clairvoyance' # Only run on main repo
+ if: github.repository == 'juspay/clairvoyance' &&
+ (github.event_name != 'pull_request_target' ||
+ github.event.pull_request.head.repo.full_name == github.repository)📝 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.
| on: | |
| push: | |
| branches: | |
| - release | |
| pull_request: | |
| branches: | |
| - release | |
| pull_request_target: | |
| branches: | |
| - release | |
| workflow_dispatch: # Allow manual trigger | |
| permissions: | |
| contents: read | |
| jobs: | |
| build-and-push: | |
| runs-on: ubuntu-latest | |
| if: github.repository == 'juspay/clairvoyance' # Only run on main repo | |
| env: | |
| AWS_REGION: ap-south-1 | |
| AWS_REPO: clairvoyance | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | |
| # Try multiple authentication methods | |
| - name: Configure AWS credentials (Method 1 - OIDC) | |
| id: aws-oidc | |
| continue-on-error: true | |
| uses: aws-actions/configure-aws-credentials@v4 | |
| with: | |
| role-to-assume: arn:aws:iam::671255721112:role/breeze-clairvoyance-ecr-role | |
| aws-region: ${{ env.AWS_REGION }} | |
| - name: Configure AWS credentials (Method 2 - Access Keys) | |
| if: steps.aws-oidc.outcome == 'failure' | |
| continue-on-error: true | |
| uses: aws-actions/configure-aws-credentials@v4 | |
| with: | |
| aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} | |
| aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | |
| aws-region: ${{ env.AWS_REGION }} | |
| - name: Configure AWS credentials (Method 3 - Alternative Role) | |
| if: steps.aws-oidc.outcome == 'failure' | |
| continue-on-error: true | |
| uses: aws-actions/configure-aws-credentials@v4 | |
| with: | |
| role-to-assume: arn:aws:iam::671255721112:role/github-actions-role | |
| aws-region: ${{ env.AWS_REGION }} | |
| on: | |
| push: | |
| branches: | |
| - release | |
| pull_request: | |
| branches: | |
| - release | |
| pull_request_target: | |
| branches: | |
| - release | |
| workflow_dispatch: # Allow manual trigger | |
| permissions: | |
| contents: read | |
| jobs: | |
| build-and-push: | |
| runs-on: ubuntu-latest | |
| if: github.repository == 'juspay/clairvoyance' && | |
| (github.event_name != 'pull_request_target' || | |
| github.event.pull_request.head.repo.full_name == github.repository) | |
| env: | |
| AWS_REGION: ap-south-1 | |
| AWS_REPO: clairvoyance | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | |
| # Try multiple authentication methods | |
| - name: Configure AWS credentials (Method 1 - OIDC) | |
| id: aws-oidc | |
| continue-on-error: true | |
| uses: aws-actions/configure-aws-credentials@v4 | |
| with: | |
| role-to-assume: arn:aws:iam::671255721112:role/breeze-clairvoyance-ecr-role | |
| aws-region: ${{ env.AWS_REGION }} | |
| - name: Configure AWS credentials (Method 2 - Access Keys) | |
| if: steps.aws-oidc.outcome == 'failure' | |
| continue-on-error: true | |
| uses: aws-actions/configure-aws-credentials@v4 | |
| with: | |
| aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} | |
| aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | |
| aws-region: ${{ env.AWS_REGION }} | |
| - name: Configure AWS credentials (Method 3 - Alternative Role) | |
| if: steps.aws-oidc.outcome == 'failure' | |
| continue-on-error: true | |
| uses: aws-actions/configure-aws-credentials@v4 | |
| with: | |
| role-to-assume: arn:aws:iam::671255721112:role/github-actions-role | |
| aws-region: ${{ env.AWS_REGION }} |
🤖 Prompt for AI Agents
.github/workflows/docker-build-fallback.yml lines 3-58: the workflow uses
pull_request_target which exposes base-repo secrets to forked PRs and then
immediately attempts to configure AWS credentials; remove the
pull_request_target trigger or add a guard that ensures this job only runs when
the PR head repo is the same as the base repo (e.g. compare
github.event.pull_request.head.repo.full_name to github.repository) before any
credential configuration or checkout of the forked head SHA, and only attempt
AWS credential steps when that guard passes so secrets/roles cannot be used by
external forks.
| call_outcome = OUTCOME_TO_ENUM.get(self.outcome, LeadCallOutcome.BUSY) | ||
|
|
||
| summary_data = { | ||
| "callSid": self.call_sid, | ||
| "outcome": call_outcome, | ||
| "updatedAddress": self.updated_address, | ||
| "orderId": self.order_id, | ||
| } | ||
| logger.info(f"Call summary data: {summary_data}") | ||
|
|
||
| if self.reporting_webhook_url and call_outcome != LeadCallOutcome.BUSY: | ||
| try: | ||
| payload = json.dumps(summary_data, separators=(",", ":")) | ||
| signature = calculate_hmac_sha256( | ||
| payload, ORDER_CONFIRMATION_WEBHOOK_SECRET_KEY | ||
| ) | ||
| headers = {"Content-Type": "application/json"} | ||
| if signature: | ||
| headers["checksum"] = signature | ||
|
|
||
| async with self.aiohttp_session.post( | ||
| self.reporting_webhook_url, json=summary_data, headers=headers | ||
| ) as response: | ||
| if response.status == 200: | ||
| logger.info("Successfully sent call summary webhook.") | ||
| else: | ||
| response_text = await response.text() | ||
| logger.error( | ||
| f"Failed to send call summary webhook. Status: {response.status}, Body: {response_text}" | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Error sending webhook: {e}") | ||
|
|
There was a problem hiding this comment.
HMAC signature likely mismatches request body
You HMAC-sign a compact JSON string (payload) but send aiohttp’s own JSON encoding (json=summary_data). If the receiver validates against the raw body, the signature will fail. Send the exact payload you signed.
- if self.reporting_webhook_url and call_outcome != LeadCallOutcome.BUSY:
+ if self.reporting_webhook_url and call_outcome != LeadCallOutcome.BUSY:
try:
- payload = json.dumps(summary_data, separators=(",", ":"))
+ payload = json.dumps(summary_data, separators=(",", ":"))
signature = calculate_hmac_sha256(
payload, ORDER_CONFIRMATION_WEBHOOK_SECRET_KEY
)
- headers = {"Content-Type": "application/json"}
+ headers = {"Content-Type": "application/json"}
if signature:
headers["checksum"] = signature
-
- async with self.aiohttp_session.post(
- self.reporting_webhook_url, json=summary_data, headers=headers
- ) as response:
+ async with self.aiohttp_session.post(
+ self.reporting_webhook_url, data=payload, headers=headers
+ ) as response:
if response.status == 200:
logger.info("Successfully sent call summary webhook.")
else:
response_text = await response.text()
logger.error(
f"Failed to send call summary webhook. Status: {response.status}, Body: {response_text}"
)
except Exception as e:
logger.error(f"Error sending webhook: {e}")Optionally also set a ClientTimeout on the session/request.
📝 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.
| call_outcome = OUTCOME_TO_ENUM.get(self.outcome, LeadCallOutcome.BUSY) | |
| summary_data = { | |
| "callSid": self.call_sid, | |
| "outcome": call_outcome, | |
| "updatedAddress": self.updated_address, | |
| "orderId": self.order_id, | |
| } | |
| logger.info(f"Call summary data: {summary_data}") | |
| if self.reporting_webhook_url and call_outcome != LeadCallOutcome.BUSY: | |
| try: | |
| payload = json.dumps(summary_data, separators=(",", ":")) | |
| signature = calculate_hmac_sha256( | |
| payload, ORDER_CONFIRMATION_WEBHOOK_SECRET_KEY | |
| ) | |
| headers = {"Content-Type": "application/json"} | |
| if signature: | |
| headers["checksum"] = signature | |
| async with self.aiohttp_session.post( | |
| self.reporting_webhook_url, json=summary_data, headers=headers | |
| ) as response: | |
| if response.status == 200: | |
| logger.info("Successfully sent call summary webhook.") | |
| else: | |
| response_text = await response.text() | |
| logger.error( | |
| f"Failed to send call summary webhook. Status: {response.status}, Body: {response_text}" | |
| ) | |
| except Exception as e: | |
| logger.error(f"Error sending webhook: {e}") | |
| if self.reporting_webhook_url and call_outcome != LeadCallOutcome.BUSY: | |
| try: | |
| payload = json.dumps(summary_data, separators=(",", ":")) | |
| signature = calculate_hmac_sha256( | |
| payload, ORDER_CONFIRMATION_WEBHOOK_SECRET_KEY | |
| ) | |
| headers = {"Content-Type": "application/json"} | |
| if signature: | |
| headers["checksum"] = signature | |
| async with self.aiohttp_session.post( | |
| self.reporting_webhook_url, data=payload, headers=headers | |
| ) as response: | |
| if response.status == 200: | |
| logger.info("Successfully sent call summary webhook.") | |
| else: | |
| response_text = await response.text() | |
| logger.error( | |
| f"Failed to send call summary webhook. Status: {response.status}, Body: {response_text}" | |
| ) | |
| except Exception as e: | |
| logger.error(f"Error sending webhook: {e}") |
🧰 Tools
🪛 Ruff (0.14.0)
443-443: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py
around lines 413 to 445, the code signs a compact JSON string but posts
aiohttp's json=summary_data (different serialized body), causing HMAC
mismatches; change the request to send the exact payload you signed (use
data=payload and ensure Content-Type: application/json and UTF-8 encoding)
instead of json=summary_data so the receiver validates the same bytes, and
optionally pass a ClientTimeout (or use session with ClientTimeout) to the post
call to avoid hanging requests.
priyanshi-2003
force-pushed
the
removing-proxy-for-daily
branch
from
c3fd355 to
3191c8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/agents/voice/automatic/__init__.py (1)
67-67: Remove unused import.The
get_proxy_configimport is no longer used after removing the proxy configuration logic for Daily WebRTC.Apply this diff to remove the unused import:
-from app.core.transport.http_client import get_proxy_config
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/agents/voice/automatic/__init__.py(1 hunks)app/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/main.py (1)
app/core/transport/http_client.py (1)
create_aiohttp_session(54-73)
🔇 Additional comments (2)
app/agents/voice/automatic/__init__.py (1)
370-374: LGTM! Correct approach for WebRTC.Skipping proxy configuration for Daily WebRTC connections is the correct approach, as WebRTC requires direct internet access and cannot function through HTTP proxies.
app/main.py (1)
136-139: LGTM! Proper resource cleanup.Both aiohttp sessions are correctly closed during application shutdown, following the recommended pattern for resource management.
app/main.py
Outdated
| # Initialize aiohttp session WITHOUT proxy for Daily API | ||
| # Daily.co should not go through proxy as it needs direct WebRTC access | ||
| import aiohttp | ||
|
|
||
| daily_aiohttp_session = aiohttp.ClientSession() | ||
| daily_helpers["rest"] = DailyRESTHelper( | ||
| daily_api_key=DAILY_API_KEY, | ||
| daily_api_url=DAILY_API_URL, | ||
| aiohttp_session=aiohttp_session, | ||
| aiohttp_session=daily_aiohttp_session, | ||
| ) | ||
| logger.info("Daily REST helper initialized with proxy support.") | ||
| logger.info("Daily REST helper initialized without proxy for WebRTC compatibility.") | ||
|
|
||
| # Initialize separate aiohttp session with proxy support for other APIs | ||
| aiohttp_session = create_aiohttp_session() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify usage of the proxy-enabled aiohttp session.
A proxy-enabled aiohttp_session is created on line 92 but doesn't appear to be stored in application state, passed to any components, or used anywhere in the visible code. This session will be closed during shutdown (line 137) but its purpose is unclear.
If this session is intended for use by other components (e.g., background tasks, API calls), ensure it's properly exposed via app.state.aiohttp_session or passed to the components that need it. If it's not yet being used, consider deferring its creation until it's actually needed to avoid holding unused resources.
Run the following script to search for usage of this session variable:
🏁 Script executed:
#!/bin/bash
# Search for references to aiohttp_session in the codebase
# Expect: Usage patterns showing how this session is accessed by other modules
rg -nP '\baiohttp_session\b' -C3 --type=py -g '!app/main.py'Length of output: 10310
🏁 Script executed:
#!/bin/bash
# Search for references to aiohttp_session within app/main.py
rg -nP '\baiohttp_session\b' -C3 --type=py app/main.pyLength of output: 694
| aiohttp_session = create_aiohttp_session() | ||
| # Initialize aiohttp session WITHOUT proxy for Daily API | ||
| # Daily.co should not go through proxy as it needs direct WebRTC access | ||
| import aiohttp |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move import to module level.
The aiohttp import should be placed at the top of the file with other imports, not inside the lifespan function. This follows Python conventions and improves code readability.
Move the import to the top of the file alongside other imports:
+import aiohttp
import asyncio
import json
import subprocessAnd remove it from line 81:
# Initialize aiohttp session WITHOUT proxy for Daily API
# Daily.co should not go through proxy as it needs direct WebRTC access
- import aiohttp
-
daily_aiohttp_session = aiohttp.ClientSession()📝 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 aiohttp | |
| # At the top of app/main.py | |
| import aiohttp | |
| import asyncio | |
| import json | |
| import subprocess | |
| # …rest of imports… |
| import aiohttp | |
| # Inside the lifespan function, remove the inline import: | |
| # Initialize aiohttp session WITHOUT proxy for Daily API | |
| # Daily.co should not go through proxy as it needs direct WebRTC access | |
| daily_aiohttp_session = aiohttp.ClientSession() |
🤖 Prompt for AI Agents
In app/main.py around line 81, the review requests moving the local import of
aiohttp out of the lifespan function to the module-level imports at the top of
the file; remove the import statement from line 81 and add a single "import
aiohttp" with the other top-of-file imports so the module follows Python import
conventions and remains readable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/agents/voice/automatic/__init__.py (1)
67-67: Remove unused import.The import of
get_proxy_configat line 67 appears to be unused after removing the proxy configuration logic. Consider removing it to keep the imports clean.Apply this diff:
-from app.core.transport.http_client import get_proxy_configapp/main.py (1)
79-90: LGTM! Correct separation of Daily API session.Creating a separate non-proxy aiohttp session for Daily API is the right approach for WebRTC compatibility. The session is properly passed to
DailyRESTHelperand will be cleaned up during shutdown.Minor: Consider moving the import to the top of the file.
The
import aiohttpat line 81 inside the function is unconventional. While functional, standard practice is to place imports at module level for better clarity.Optional refactor:
import asyncio import json import subprocess import uuid from contextlib import asynccontextmanager from pathlib import Path from typing import Any, Dict +import aiohttp import uvicornThen remove line 81.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/agents/voice/automatic/__init__.py(1 hunks)app/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/main.py (1)
app/core/transport/http_client.py (1)
create_aiohttp_session(54-73)
🔇 Additional comments (2)
app/agents/voice/automatic/__init__.py (1)
370-374: LGTM! Correct approach for WebRTC.Bypassing proxy configuration for Daily WebRTC connections is the correct approach, as WebRTC requires direct peer-to-peer connectivity and typically cannot traverse HTTP proxies.
app/main.py (1)
136-139: LGTM! Proper cleanup of both sessions.Both aiohttp sessions are correctly closed during shutdown, preventing resource leaks.
app/main.py
Outdated
| # Initialize separate aiohttp session with proxy support for other APIs | ||
| aiohttp_session = create_aiohttp_session() |
There was a problem hiding this comment.
🧩 Analysis chain
Verify usage of the proxy-enabled session.
The aiohttp_session is created with proxy support but its usage is not visible in the provided code. Ensure this session is actually used elsewhere in the codebase, otherwise it may be unnecessary overhead.
Run the following script to find usages of aiohttp_session in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for references to aiohttp_session to verify it's being used
rg -n --type=py 'aiohttp_session' -C3Length of output: 15302
🏁 Script executed:
#!/bin/bash
# Check lines 93-136 in app/main.py to see if aiohttp_session is passed to any function or stored
sed -n '93,136p' app/main.pyLength of output: 1522
🏁 Script executed:
#!/bin/bash
# Search for any assignment or storage of aiohttp_session in main.py beyond the close() call
rg -n 'aiohttp_session\s*=' app/main.pyLength of output: 154
🏁 Script executed:
#!/bin/bash
# Check if aiohttp_session is used as an argument in function calls
rg -n 'aiohttp_session[^_]' app/main.py | grep -v 'daily_aiohttp_session' | grep -v '//'Length of output: 87
🏁 Script executed:
#!/bin/bash
# Retrieve lines 93-136 to see complete flow between creation and close
sed -n '93,136p' app/main.pyLength of output: 1522
Remove the unused aiohttp_session variable; it is created but never used.
The session created at line 92 is only closed at line 137 without being passed to any function or helper between those lines. The initialize_room_pool() and initialize_voice_agent_pool() calls do not receive this session, and no other code references it. This creates unnecessary overhead without providing any benefit.
Delete lines 91–92 and update line 137 to remove the now-unnecessary close call for this session, keeping only:
await daily_aiohttp_session.close()🤖 Prompt for AI Agents
In app/main.py around lines 91-92, an aiohttp_session is created but never used;
remove the two lines that create this unused session and update the cleanup at
line 137 to no longer close it—leave only await daily_aiohttp_session.close() so
only the actually used session is closed.
3191c8d to
5617d1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/aws-prod-docker.yml (1)
7-73: Blocker:pull_request_targetstill hands forks our AWS rolesWe continue to run this job on
pull_request_target, immediately check out the untrusted PR head, then request OIDC tokens and assume prod/beta AWS roles. A forked PR can therefore run arbitrary code with prod credentials — exactly the supply-chain hole called out earlier. Drop thepull_request_targettrigger (or guard the job so it only executes whengithub.event.pull_request.head.repo.full_name == github.repository) before any credential step. Example:on: pull_request: branches: - release - pull_request_target: - branches: - - release + pull_request_target: + branches: + - release ... jobs: build-and-push: runs-on: ubuntu-latest - # existing env ... + if: github.event_name != 'pull_request_target' || github.event.pull_request.head.repo.full_name == github.repositoryThis ensures forks cannot obtain prod AWS creds.
.github/workflows/docker-build-fallback.yml (1)
7-58: Blocker: Fallback workflow still exposes AWS creds to forked PRs
pull_request_targetruns with base-repo context; we immediately check out the fork’s head and cycle through AWS credential methods (including static keys). Because the only guard isgithub.repository == 'juspay/clairvoyance', forks can still execute arbitrary code with our prod/beta creds. Remove thepull_request_targettrigger or add a pre-credential guard such as:jobs: build-and-push: runs-on: ubuntu-latest - if: github.repository == 'juspay/clairvoyance' + if: github.repository == 'juspay/clairvoyance' && + (github.event_name != 'pull_request_target' || + github.event.pull_request.head.repo.full_name == github.repository)This must happen before any AWS auth step so forked PRs cannot access secrets/roles.
app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py (1)
413-445: HMAC mismatch: sign compact JSON but post a different bodyYou compute signature over
payload(compact JSON) but sendjson=summary_data(aiohttp re-serializes). The receiver will verify a different byte sequence.Apply this change and consider a timeout and narrower exceptions:
- async with self.aiohttp_session.post( - self.reporting_webhook_url, json=summary_data, headers=headers - ) as response: + async with self.aiohttp_session.post( + self.reporting_webhook_url, + data=payload, # send exact bytes you signed + headers=headers, + # optional: per-request timeout if session lacks one + # timeout=aiohttp.ClientTimeout(total=10), + ) as response: if response.status == 200: logger.info("Successfully sent call summary webhook.") else: response_text = await response.text() logger.error( f"Failed to send call summary webhook. Status: {response.status}, Body: {response_text}" ) - except Exception as e: - logger.error(f"Error sending webhook: {e}") + except (asyncio.TimeoutError, aiohttp.ClientError) as e: + logger.error(f"Error sending webhook: {e}")If you add the timeout or narrow exceptions, also ensure:
import aiohttpat the top of the file.
🧹 Nitpick comments (2)
app/main.py (1)
79-90: Add explicit timeout (and trust_env) to Daily REST sessionBind a sane default timeout to avoid hangs and make proxy-less intent explicit.
- daily_aiohttp_session = aiohttp.ClientSession() + daily_aiohttp_session = aiohttp.ClientSession( + timeout=aiohttp.ClientTimeout(total=15), + trust_env=False # be explicit: ignore env proxies for Daily + )Optionally store on app.state if other components need it later:
_app.state.daily_aiohttp_session = daily_aiohttp_session.app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py (1)
315-319: Fix unused parameter in idle-timeout handlerRename param to underscore to satisfy linters and clarify intent.
- async def on_idle_timeout(task): + async def on_idle_timeout(_task): logger.info("Idle timeout detected.") await self._handle_unexpected_disconnect("Idle timeout")As per static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/aws-prod-docker.yml(2 hunks).github/workflows/docker-build-fallback.yml(1 hunks).github/workflows/docker-build-only.yml(1 hunks)app/agents/voice/automatic/__init__.py(1 hunks)app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py(3 hunks)app/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/main.py (1)
app/core/transport/http_client.py (1)
create_aiohttp_session(54-73)
app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py (2)
app/schemas.py (1)
LeadCallOutcome(37-43)app/core/security/sha.py (1)
calculate_hmac_sha256(6-27)
🪛 Ruff (0.14.0)
app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py
316-316: Unused function argument: task
(ARG001)
443-443: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
app/agents/voice/automatic/__init__.py (1)
370-374: Daily WebRTC should bypass proxies — change looks goodSkipping proxy configuration here is correct for WebRTC. Proceed.
app/main.py (2)
136-140: Session shutdown looks correctBoth sessions are closed in lifespan shutdown. Good.
91-93: Remove incorrect proxy-bug concern
aiohttp.ClientSession’s constructor does accept aproxyargument, socreate_aiohttp_sessionis valid as written.Likely an incorrect or invalid review comment.
app/agents/voice/breeze_buddy/workflows/order_confirmation/websocket_bot.py (4)
313-314: Centralized disconnect handling — good consolidationSwitching to a single handler simplifies lifecycle and reduces drift.
384-389: End-of-conversation now funnels through a single finalizer — goodThis reduces duplicated end logic and side effects.
390-396: Reasonable default outcome on unexpected disconnectMarking as busy and finalizing is a safe default.
451-463: DB completion flow looks consistentOutcome, transcription, and updated_address are passed coherently; logging is clear.
priyanshi-2003
force-pushed
the
removing-proxy-for-daily
branch
from
5617d1e to
c1fc71d
Compare
|
https://bitbucket.juspay.net/projects/PROJ/repos/juspay-genius/browse Also refer this project see how they are handling proxy @priyanshi-2003 |
2 participants
Summary by CodeRabbit