Add phone number blacklist feature with dual enforcement#608
Add phone number blacklist feature with dual enforcement#608swaroopvarma1 wants to merge 1 commit intoreleasefrom
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 Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces a blacklist phone numbers feature for Breeze Buddy with four main components: a new database table and accessor layer with Redis caching for blacklist lookups, API endpoints for managing blacklists with admin-only access control, schema models for request and response objects, and integration points in lead push and call backlog processing to block blacklisted numbers. Changes
Sequence DiagramsequenceDiagram
actor Client
participant API as API Layer
participant Handler as Handler
participant Cache as Redis Cache
participant DB as Database
participant CallMgr as Call Manager
Client->>API: POST /blacklist (add number)
API->>Handler: add_blacklist_handler()
Handler->>DB: insert_blacklisted_number_query()
DB-->>Handler: BlacklistedNumber created
Handler->>Cache: Invalidate cache
Handler-->>API: Return BlacklistedNumber
API-->>Client: 200 OK
Client->>API: POST /leads (push lead with phone)
API->>Handler: push_lead_handler(customer_mobile_number)
Handler->>Cache: is_number_blacklisted(phone, merchant)
alt Cache Hit
Cache-->>Handler: is_blacklisted=true
else Cache Miss
Handler->>DB: is_number_blacklisted_query()
DB-->>Handler: Check result
Handler->>Cache: Store result (5min TTL)
end
alt Number is Blacklisted
Handler-->>API: HTTP 403 Forbidden
API-->>Client: Error: Phone number is blacklisted
else Number is Clean
Handler->>Handler: Continue validation
API-->>Client: 200 OK
end
CallMgr->>CallMgr: process_backlog_leads()
CallMgr->>Cache: is_number_blacklisted(phone)
Cache-->>CallMgr: is_blacklisted flag
alt Blacklisted
CallMgr->>DB: Mark lead FINISHED/BLACKLISTED
CallMgr->>CallMgr: Release lock, continue
else Not Blacklisted
CallMgr->>CallMgr: Proceed with call
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fcb6848e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await redis.delete(_cache_key(phone_number, merchant_id)) | ||
| await redis.delete(_cache_key(phone_number)) |
There was a problem hiding this comment.
The new blacklist checks always call is_number_blacklisted with a merchant ID (in both lead push and backlog processing), so cached decisions are stored under merchant-scoped keys. When a global blacklist entry is added/removed (merchant_id=None), _invalidate_cache only deletes the non-merchant key, which means existing blacklist:<phone>:<merchant> entries stay stale and can keep returning the old allow/block decision until TTL expiry.
Useful? React with 👍 / 👎.
| created_by VARCHAR(255) DEFAULT NULL, | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() NOT NULL, | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() NOT NULL, | ||
| UNIQUE(phone_number, merchant_id) |
There was a problem hiding this comment.
Using UNIQUE(phone_number, merchant_id) does not prevent duplicate global entries in PostgreSQL because NULL values in merchant_id are considered distinct for uniqueness checks. As a result, repeated global blacklist inserts for the same phone number can succeed instead of hitting the duplicate path, which undermines the API's "already blacklisted" behavior and creates inconsistent records.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Implements a Breeze Buddy phone-number blacklist with “dual enforcement” to block calls both when leads are pushed and when backlog leads are executed, plus admin-only APIs to manage the blacklist. This fits into Breeze Buddy’s existing layered architecture (migration → queries/decoder/accessor → handlers/routers) and adds a Redis-backed fast path for high-volume cron processing.
Changes:
- Added
blacklisted_numberspersistence (migration) plus query/decoder/accessor layers (with Redis caching). - Enforced blacklist checks at lead push time and backlog processing time.
- Added admin-only REST endpoints to add/list/check/remove blacklisted numbers.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| plan.md | Implementation plan and rationale for dual enforcement + caching. |
| app/schemas/breeze_buddy/core.py | Adds BlacklistedNumber + CreateBlacklistNumberRequest schemas. |
| app/schemas/init.py | Re-exports new blacklist schemas. |
| app/schemas.py | Re-exports new blacklist schemas (alternate schema export module). |
| app/database/migrations/017_create_blacklisted_numbers_table.sql | Creates blacklisted_numbers table + indexes. |
| app/database/queries/breeze_buddy/blacklisted_numbers.py | Adds normalization + insert/delete/existence/list queries. |
| app/database/decoder/breeze_buddy/blacklisted_numbers.py | Decodes DB rows into BlacklistedNumber models. |
| app/database/accessor/breeze_buddy/blacklisted_numbers.py | Accessor layer with Redis caching + invalidation. |
| app/database/accessor/init.py | Exposes blacklist accessor functions for use in handlers/managers. |
| app/api/routers/breeze_buddy/leads/handlers.py | Push-time blacklist enforcement (reject blacklisted leads). |
| app/api/routers/breeze_buddy/blacklist/init.py | Adds blacklist management endpoints. |
| app/api/routers/breeze_buddy/blacklist/handlers.py | Business logic for add/list/check/remove operations. |
| app/api/routers/breeze_buddy/blacklist/rbac.py | Admin-only access gate for blacklist management. |
| app/api/routers/breeze_buddy/init.py | Registers the blacklist router in the Breeze Buddy route group. |
| app/ai/voice/agents/breeze_buddy/managers/calls.py | Pick-time blacklist enforcement (skip + mark outcome BLACKLISTED). |
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() NOT NULL, | ||
| UNIQUE(phone_number, merchant_id) | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_blacklisted_numbers_phone_number | ||
| ON blacklisted_numbers (phone_number); | ||
| CREATE INDEX IF NOT EXISTS idx_blacklisted_numbers_merchant_id | ||
| ON blacklisted_numbers (merchant_id); |
There was a problem hiding this comment.
The UNIQUE(phone_number, merchant_id) constraint won’t prevent duplicate global blacklist entries because Postgres treats NULLs as distinct. This allows inserting the same phone_number multiple times with merchant_id NULL, which can cause duplicate results and delete operations returning multiple rows. Consider enforcing uniqueness with partial unique indexes (one for (phone_number) WHERE merchant_id IS NULL, and one for (phone_number, merchant_id) WHERE merchant_id IS NOT NULL) or store a non-NULL sentinel value for global scope.
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() NOT NULL, | |
| UNIQUE(phone_number, merchant_id) | |
| ); | |
| CREATE INDEX IF NOT EXISTS idx_blacklisted_numbers_phone_number | |
| ON blacklisted_numbers (phone_number); | |
| CREATE INDEX IF NOT EXISTS idx_blacklisted_numbers_merchant_id | |
| ON blacklisted_numbers (merchant_id); | |
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() NOT NULL | |
| ); | |
| CREATE INDEX IF NOT EXISTS idx_blacklisted_numbers_phone_number | |
| ON blacklisted_numbers (phone_number); | |
| CREATE INDEX IF NOT EXISTS idx_blacklisted_numbers_merchant_id | |
| ON blacklisted_numbers (merchant_id); | |
| CREATE UNIQUE INDEX IF NOT EXISTS ux_blacklisted_numbers_global | |
| ON blacklisted_numbers (phone_number) | |
| WHERE merchant_id IS NULL; | |
| CREATE UNIQUE INDEX IF NOT EXISTS ux_blacklisted_numbers_scoped | |
| ON blacklisted_numbers (phone_number, merchant_id) | |
| WHERE merchant_id IS NOT NULL; |
| def _cache_key(phone_number: str, merchant_id: Optional[str] = None) -> str: | ||
| """Build Redis cache key for blacklist lookup.""" | ||
| normalized = normalize_phone_number(phone_number) | ||
| if merchant_id: | ||
| return f"blacklist:{normalized}:{merchant_id}" | ||
| return f"blacklist:{normalized}" | ||
|
|
There was a problem hiding this comment.
The cache-key strategy is inconsistent with the lookup semantics that include both per-merchant and global blacklist entries. If a global blacklist entry is added/removed, existing per-merchant cache keys (e.g., blacklist:{phone}:{merchant}) can remain cached as "not blacklisted" and will incorrectly allow calls until TTL expiry. Consider caching per phone number only (e.g., store a global flag + merchant-id set) or otherwise ensure all per-merchant keys for that phone are invalidated on global changes.
| try: | ||
| query_text, values = is_number_blacklisted_query(phone_number, merchant_id) | ||
| result = await run_parameterized_query(query_text, values) | ||
| is_blocked = bool(result and result[0]["is_blacklisted"]) | ||
|
|
||
| # Cache the result | ||
| try: | ||
| redis = await get_redis_service() | ||
| await redis.setex( | ||
| key=cache_key, | ||
| value="1" if is_blocked else "0", | ||
| ttl_seconds=BLACKLIST_CACHE_TTL, | ||
| ) | ||
| except Exception as e: | ||
| logger.warning(f"Redis cache set error for blacklist: {e}") |
There was a problem hiding this comment.
run_parameterized_query returns None on DB errors (it swallows exceptions). In that case, this code treats the number as not blacklisted and caches "0", potentially disabling enforcement for BLACKLIST_CACHE_TTL even after the DB recovers. Consider detecting result is None (vs an empty result) and skipping caching, and decide on an explicit fallback (often fail-closed for compliance) or propagate an error so callers can handle it appropriately.
| logger.error(f"Error adding to blacklist: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Error adding to blacklist: {str(e)}", | ||
| ) |
There was a problem hiding this comment.
This handler returns the raw exception text to the client via the HTTPException detail. Even though this is admin-only, it can leak internal implementation details. Prefer logging the exception (as you already do) and returning a generic error message in the response.
| logger.error(f"Error listing blacklisted numbers: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Error listing blacklisted numbers: {str(e)}", | ||
| ) |
There was a problem hiding this comment.
This handler returns the raw exception text to the client via the HTTPException detail. Prefer returning a generic message (and keep the full exception only in logs) to avoid leaking internal details.
| logger.error(f"Error checking blacklist: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Error checking blacklist: {str(e)}", | ||
| ) |
There was a problem hiding this comment.
This handler returns the raw exception text to the client via the HTTPException detail. Prefer returning a generic message (and keep the full exception only in logs) to avoid leaking internal details.
| logger.error(f"Error removing from blacklist: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Error removing from blacklist: {str(e)}", | ||
| ) |
There was a problem hiding this comment.
This handler returns the raw exception text to the client via the HTTPException detail. Prefer returning a generic message (and keep the full exception only in logs) to avoid leaking internal details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
app/database/accessor/breeze_buddy/blacklisted_numbers.py (1)
24-24: Move cache TTL to centralized config.
BLACKLIST_CACHE_TTL = 300is a runtime config knob and should be sourced from the central config module rather than hardcoded in accessor logic.As per coding guidelines: "Load ALL configuration from
app/core/config/static.pyusingget_required_env()for mandatory variables; never import directly fromos.environelsewhere".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/database/accessor/breeze_buddy/blacklisted_numbers.py` at line 24, Replace the hardcoded BLACKLIST_CACHE_TTL constant with a value loaded from the centralized config: add a required config entry (e.g. BLACKLIST_CACHE_TTL) in app/core/config/static.py that reads the env var via get_required_env() and converts it to int, then in app/database/accessor/breeze_buddy/blacklisted_numbers.py remove the hardcoded 300 and import the TTL from the static config module (use the same symbol name BLACKLIST_CACHE_TTL) so all runtime config is sourced centrally.app/api/routers/breeze_buddy/blacklist/handlers.py (1)
77-95: Consider masking phone number in logs.The
check_blacklist_handlerlogs the full phone number at line 81. For PII protection, consider masking all but the last 4 digits in production logs, similar to the recommendation for the lead handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/routers/breeze_buddy/blacklist/handlers.py` around lines 77 - 95, The handler check_blacklist_handler currently logs the full phone_number; replace that with a masked version (e.g., keep only the last 4 digits and replace preceding characters with asterisks, handling short numbers gracefully) and use that masked value in the logger.info call; implement the masking inline or call a small helper (e.g., mask_phone_number) and update the logger.info(f"Admin {current_user.username} checking blacklist for {masked_phone}") so PII is not exposed while keeping the username context.app/database/queries/breeze_buddy/blacklisted_numbers.py (1)
12-18: Consider edge case handling for invalid phone numbers.The normalization returns an empty string for inputs with no digits, which could lead to unexpected behavior (e.g., blacklisting an empty string). Consider adding validation or returning
Nonefor invalid inputs.def normalize_phone_number(phone_number: str) -> str: """ Normalize a phone number by stripping non-digit characters and keeping only the last 10 digits (for Indian numbers). """ digits = re.sub(r"\D", "", phone_number) + if not digits: + raise ValueError("Phone number contains no digits") return digits[-10:] if len(digits) >= 10 else digitsAlternatively, handle this at the caller level before invoking the query builders.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/database/queries/breeze_buddy/blacklisted_numbers.py` around lines 12 - 18, The normalize_phone_number function currently returns an empty string when no digits are found; change it to detect when re.sub(r"\D", "", phone_number) yields an empty string and return None (or raise a ValueError) instead of an empty string to avoid blacklisting empty values—update normalize_phone_number to return Optional[str] and ensure callers of normalize_phone_number (the query builders that use this function) check for None and skip or handle invalid inputs before building blacklisting queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/routers/breeze_buddy/blacklist/rbac.py`:
- Around line 27-35: Replace the string comparison against "admin" with the
UserRole enum: update the check in rbac.py that currently reads if
current_user.role != "admin": to if current_user.role != UserRole.ADMIN:,
importing UserRole if not already present; keep the existing logging
(logger.warning with current_user.username and current_user.role) and the
HTTPException behavior unchanged so only the role comparison uses the enum type.
In `@app/database/accessor/breeze_buddy/blacklisted_numbers.py`:
- Around line 84-85: The logger.info calls currently emit raw phone_number
(e.g., logger.info(f"Adding {phone_number} to blacklist (merchant:
{merchant_id})")), which leaks PII; replace direct phone_number logging with a
masked or hashed representation by introducing a small helper (e.g.,
mask_phone_number or hash_phone_number) and call that before logging so logs
contain only a masked value (such as showing only the last 4 digits or an HMAC)
together with merchant_id; update every logger usage that references
phone_number (the logger.info lines that interpolate phone_number) to use the
helper instead.
- Around line 27-33: The current cache key conflates phone- and merchant-level
entries causing stale merchant keys; introduce a distinct phone-level key and a
separate merchant-level key so global updates only need to invalidate one
phone-level entry: implement a new _phone_cache_key(phone_number) that returns
f"blacklist:{normalize_phone_number(phone_number)}:phone" and change
_cache_key(phone_number, merchant_id) to return
f"blacklist:{normalized}:merchant:{merchant_id}" when merchant_id is provided;
update lookup logic to check the merchant key first then fall back to the phone
key, and update add/remove/invalidate code (the blacklist add/remove handlers)
to delete only the phone-level key on global changes and the merchant-level key
on per-merchant changes.
- Around line 69-71: The except block in blacklisted_numbers.py currently
swallows all exceptions and returns False (using phone_number and logger), which
fails open; instead change the handler to fail-closed or surface the error:
replace the generic logger.error + return False with logger.exception(...) to
include stack trace and then re-raise the exception (or explicitly return True
if you want a hard fail-closed policy), and update callers of the function to
handle/propagate the error appropriately; ensure you only catch more specific
exceptions where appropriate and write/adjust tests to cover DB/query failures.
In `@app/schemas/breeze_buddy/core.py`:
- Around line 268-273: Create validation for CreateBlacklistNumberRequest:
constrain phone_number to sensible bounds (e.g., enforce E.164-like pattern or
min/max digits such as min_length=7, max_length=15 or regex like
r"^\+?[1-9]\d{6,14}$") and constrain reason to max_length=500 to match DB
VARCHAR(500) (and optionally a min_length=1 if required). Use Pydantic
constrained types (e.g., constr) or field validators on the
CreateBlacklistNumberRequest class to enforce these rules and update imports
accordingly.
In `@plan.md`:
- Around line 184-207: The fenced ASCII architecture block (the Push Lead API
diagram) is missing a language hint which triggers MD040; update the opening
fence of that block to include a language identifier (e.g., add "text" after the
``` for the architecture diagram) so the markdown linter recognizes the block
type and leave the closing fence unchanged; locate the fenced block by the
diagram header "Push Lead API (leads/handlers.py)" or the ascii box content and
add the language hint to the opening fence.
---
Nitpick comments:
In `@app/api/routers/breeze_buddy/blacklist/handlers.py`:
- Around line 77-95: The handler check_blacklist_handler currently logs the full
phone_number; replace that with a masked version (e.g., keep only the last 4
digits and replace preceding characters with asterisks, handling short numbers
gracefully) and use that masked value in the logger.info call; implement the
masking inline or call a small helper (e.g., mask_phone_number) and update the
logger.info(f"Admin {current_user.username} checking blacklist for
{masked_phone}") so PII is not exposed while keeping the username context.
In `@app/database/accessor/breeze_buddy/blacklisted_numbers.py`:
- Line 24: Replace the hardcoded BLACKLIST_CACHE_TTL constant with a value
loaded from the centralized config: add a required config entry (e.g.
BLACKLIST_CACHE_TTL) in app/core/config/static.py that reads the env var via
get_required_env() and converts it to int, then in
app/database/accessor/breeze_buddy/blacklisted_numbers.py remove the hardcoded
300 and import the TTL from the static config module (use the same symbol name
BLACKLIST_CACHE_TTL) so all runtime config is sourced centrally.
In `@app/database/queries/breeze_buddy/blacklisted_numbers.py`:
- Around line 12-18: The normalize_phone_number function currently returns an
empty string when no digits are found; change it to detect when re.sub(r"\D",
"", phone_number) yields an empty string and return None (or raise a ValueError)
instead of an empty string to avoid blacklisting empty values—update
normalize_phone_number to return Optional[str] and ensure callers of
normalize_phone_number (the query builders that use this function) check for
None and skip or handle invalid inputs before building blacklisting queries.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/ai/voice/agents/breeze_buddy/managers/calls.pyapp/api/routers/breeze_buddy/__init__.pyapp/api/routers/breeze_buddy/blacklist/__init__.pyapp/api/routers/breeze_buddy/blacklist/handlers.pyapp/api/routers/breeze_buddy/blacklist/rbac.pyapp/api/routers/breeze_buddy/leads/handlers.pyapp/database/accessor/__init__.pyapp/database/accessor/breeze_buddy/blacklisted_numbers.pyapp/database/decoder/breeze_buddy/blacklisted_numbers.pyapp/database/migrations/017_create_blacklisted_numbers_table.sqlapp/database/queries/breeze_buddy/blacklisted_numbers.pyapp/schemas.pyapp/schemas/__init__.pyapp/schemas/breeze_buddy/core.pyplan.md
| if current_user.role != "admin": | ||
| logger.warning( | ||
| f"Non-admin user {current_user.username} (role: {current_user.role}) " | ||
| f"attempted to {operation}" | ||
| ) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail=f"Admin access required to {operation}", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find UserRole enum definition
ast-grep --pattern 'class UserRole {
$$$
}'
# Also search for enum definitions more broadly
rg -n "UserRole" --type=py | head -20Repository: juspay/clairvoyance
Length of output: 1415
🏁 Script executed:
# Check auth schema files for role definitions
fd -e py -p "auth" "app/schemas"Repository: juspay/clairvoyance
Length of output: 95
🏁 Script executed:
# Look at the actual rbac.py file to understand context
cat -n app/api/routers/breeze_buddy/blacklist/rbac.pyRepository: juspay/clairvoyance
Length of output: 1306
🏁 Script executed:
# Check the UserRole enum values
cat -n app/schemas/breeze_buddy/auth.py | head -50Repository: juspay/clairvoyance
Length of output: 1647
🏁 Script executed:
# Also check UserInfo definition to see role type
rg -n "class UserInfo" --type=py -A 10Repository: juspay/clairvoyance
Length of output: 808
🏁 Script executed:
# Check how role comparisons are done elsewhere in the codebase
rg -n "\.role\s*[!=<>]" --type=py -C 2 | head -50Repository: juspay/clairvoyance
Length of output: 2995
🏁 Script executed:
# Check if UserRole enum is used directly in comparisons elsewhere
rg -n "UserRole\." --type=py | grep -i "admin\|reseller\|merchant"Repository: juspay/clairvoyance
Length of output: 659
Use UserRole.ADMIN enum instead of string "admin" for role comparison.
The current_user.role field is typed as UserRole (an enum), and the codebase standard throughout app/core/security/authorization.py is to compare against UserRole.ADMIN rather than the string "admin". Change line 27 from if current_user.role != "admin": to if current_user.role != UserRole.ADMIN: for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/routers/breeze_buddy/blacklist/rbac.py` around lines 27 - 35, Replace
the string comparison against "admin" with the UserRole enum: update the check
in rbac.py that currently reads if current_user.role != "admin": to if
current_user.role != UserRole.ADMIN:, importing UserRole if not already present;
keep the existing logging (logger.warning with current_user.username and
current_user.role) and the HTTPException behavior unchanged so only the role
comparison uses the enum type.
| def _cache_key(phone_number: str, merchant_id: Optional[str] = None) -> str: | ||
| """Build Redis cache key for blacklist lookup.""" | ||
| normalized = normalize_phone_number(phone_number) | ||
| if merchant_id: | ||
| return f"blacklist:{normalized}:{merchant_id}" | ||
| return f"blacklist:{normalized}" | ||
|
|
There was a problem hiding this comment.
Global blacklist changes can leave merchant cache entries stale.
With per-merchant boolean keys, adding/removing a global entry only invalidates the base key and one merchant key. Other merchant-specific keys remain stale until TTL expiry, so blacklisted numbers may still be allowed temporarily.
🛠️ Suggested direction
-# cache key stores boolean per (phone, merchant)
+# cache key should be per phone only, value should encode scope membership
+# e.g. {"global": true, "merchant_ids": ["m1", "m2"]}Then invalidate exactly one phone-level key on add/remove.
Also applies to: 173-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/database/accessor/breeze_buddy/blacklisted_numbers.py` around lines 27 -
33, The current cache key conflates phone- and merchant-level entries causing
stale merchant keys; introduce a distinct phone-level key and a separate
merchant-level key so global updates only need to invalidate one phone-level
entry: implement a new _phone_cache_key(phone_number) that returns
f"blacklist:{normalize_phone_number(phone_number)}:phone" and change
_cache_key(phone_number, merchant_id) to return
f"blacklist:{normalized}:merchant:{merchant_id}" when merchant_id is provided;
update lookup logic to check the merchant key first then fall back to the phone
key, and update add/remove/invalidate code (the blacklist add/remove handlers)
to delete only the phone-level key on global changes and the merchant-level key
on per-merchant changes.
app/schemas/breeze_buddy/core.py
Outdated
| class CreateBlacklistNumberRequest(BaseModel): | ||
| """Request to add a phone number to the blacklist""" | ||
|
|
||
| phone_number: str | ||
| merchant_id: Optional[str] = None | ||
| reason: Optional[str] = None |
There was a problem hiding this comment.
Add input bounds to blacklist request schema.
CreateBlacklistNumberRequest currently accepts unconstrained values. At minimum, validate reason length (DB is VARCHAR(500)) and add basic phone_number bounds to prevent avoidable DB failures.
🧩 Suggested schema hardening
class CreateBlacklistNumberRequest(BaseModel):
"""Request to add a phone number to the blacklist"""
- phone_number: str
+ phone_number: str = Field(min_length=7, max_length=32)
merchant_id: Optional[str] = None
- reason: Optional[str] = None
+ reason: Optional[str] = Field(default=None, max_length=500)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/schemas/breeze_buddy/core.py` around lines 268 - 273, Create validation
for CreateBlacklistNumberRequest: constrain phone_number to sensible bounds
(e.g., enforce E.164-like pattern or min/max digits such as min_length=7,
max_length=15 or regex like r"^\+?[1-9]\d{6,14}$") and constrain reason to
max_length=500 to match DB VARCHAR(500) (and optionally a min_length=1 if
required). Use Pydantic constrained types (e.g., constr) or field validators on
the CreateBlacklistNumberRequest class to enforce these rules and update imports
accordingly.
plan.md
Outdated
| ``` | ||
| ┌─────────────────────┐ | ||
| │ Push Lead API │ | ||
| │ (leads/handlers.py) │ | ||
| └─────────┬───────────┘ | ||
| │ | ||
| ▼ | ||
| ┌─────────────────────┐ | ||
| ┌────►│ is_number_blacklisted│◄────┐ | ||
| │ └─────────┬───────────┘ │ | ||
| │ │ │ | ||
| │ ┌──────┴──────┐ │ | ||
| │ ▼ ▼ │ | ||
| │ ┌───────┐ ┌──────────┐ │ | ||
| │ │ Redis │ │ Postgres │ │ | ||
| │ │ Cache │ │ Table │ │ | ||
| │ └───────┘ └──────────┘ │ | ||
| │ │ | ||
| ┌─────────┴───────────┐ ┌───────────┴──────────┐ | ||
| │ Cron: Process │ │ Blacklist Mgmt API │ | ||
| │ Backlog Leads │ │ (add/remove/list) │ | ||
| │ (managers/calls.py) │ │ Invalidates cache │ | ||
| └─────────────────────┘ └──────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language hint to the architecture fenced block.
At Line 184, the fenced block has no language identifier (MD040).
✏️ Suggested markdown fix
-```
+```text
┌─────────────────────┐
│ Push Lead API │
@@
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plan.md` around lines 184 - 207, The fenced ASCII architecture block (the
Push Lead API diagram) is missing a language hint which triggers MD040; update
the opening fence of that block to include a language identifier (e.g., add
"text" after the ``` for the architecture diagram) so the markdown linter
recognizes the block type and leave the closing fence unchanged; locate the
fenced block by the diagram header "Push Lead API (leads/handlers.py)" or the
ascii box content and add the language hint to the opening fence.
swaroopvarma1
force-pushed
the
claude/backlist-calls-feature-uuDO7
branch
from
0fcb684 to
87a506a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
swaroopvarma1
force-pushed
the
claude/backlist-calls-feature-uuDO7
branch
from
87a506a to
f99944d
Compare
- Add blacklisted_numbers DB table (migration 017) with phone_number, merchant_id (NULL = global), reason, and created_by columns - Use partial unique indexes to correctly handle NULL merchant_id - Add full data layer: schema models, queries, decoder, accessor with Redis cache (configurable TTL via BLACKLIST_CACHE_TTL env var) - Add CRUD API endpoints (POST/GET/DELETE /blacklist) with admin RBAC - Enforce blacklist at lead push time (HTTP 403) for immediate feedback - Enforce blacklist at cron call-pick time as safety net, marking leads as FINISHED with outcome BLACKLISTED - Phone numbers are normalized to last 10 digits for consistent matching - Fail-closed on DB errors to prevent calling blocked numbers - PII masking in all log lines (phone numbers show only last 4 digits) - Generic error messages returned to clients (no raw exception text) - Input validation on phone_number length and reason field https://claude.ai/code/session_014juKikX794D1QC5zMW1xWX
swaroopvarma1
force-pushed
the
claude/backlist-calls-feature-uuDO7
branch
from
f99944d to
a084d95
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
app/database/accessor/breeze_buddy/blacklisted_numbers.py:212
- Cache invalidation is incomplete for global blacklist updates. When
merchant_idisNone(global entry),_invalidate_cacheonly deletes the non-merchant key (twice) and does not remove any existingblacklist:{phone}:{merchant}keys, so per-merchant cache entries can remain stale (e.g., cached "0") for up toBLACKLIST_CACHE_TTL, allowing calls to a globally blacklisted number. Consider either (a) using a single cache key per normalized phone that encodes global/merchant scopes, or (b) on global add/remove, scanning/deletingblacklist:{normalized}:*keys as well.
await redis.delete(_cache_key(phone_number, merchant_id))
await redis.delete(_cache_key(phone_number))
except Exception as e:
logger.warning(f"Redis cache invalidation error: {e}")
| logger.info( | ||
| f"Skipping lead {locked_lead.id} - phone number {blacklist_phone} is blacklisted" | ||
| ) |
There was a problem hiding this comment.
This log line prints the full customer phone number ({blacklist_phone}), which is sensitive PII. Consider masking the number (last 4 digits) or avoiding logging it, similar to the masking used in the blacklist accessor/handlers.
| from app.schemas import BlacklistedNumber | ||
| from app.services.redis.client import get_redis_service | ||
|
|
||
| BLACKLIST_CACHE_TTL = int(os.getenv("BLACKLIST_CACHE_TTL", "300")) # Default 5 minutes |
There was a problem hiding this comment.
This accessor reads BLACKLIST_CACHE_TTL via os.getenv directly. Elsewhere Redis-related config is centralized in app.core.config.static and imported (e.g., app/services/redis/client.py imports REDIS_TTL). To keep configuration consistent and avoid env reads scattered across modules, add BLACKLIST_CACHE_TTL to app.core.config.static (or reuse an existing TTL constant) and import it here instead of calling os.getenv.
| if customer_mobile and await is_number_blacklisted( | ||
| customer_mobile, req.merchant | ||
| ): | ||
| logger.warning( | ||
| f"Blacklisted number {customer_mobile} rejected for merchant {req.merchant}" | ||
| ) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="Phone number is blacklisted", | ||
| ) |
There was a problem hiding this comment.
The blacklist check runs before payload validation and does not verify type. If customer_mobile_number is present but not a string (e.g., int/list), is_number_blacklisted() will raise (regex expects a str), resulting in a 500 instead of a clean 400 validation error. Consider moving the blacklist check after validate_payload(...) (or at least gate it with isinstance(customer_mobile, str) / safe normalization).
| if customer_mobile and await is_number_blacklisted( | |
| customer_mobile, req.merchant | |
| ): | |
| logger.warning( | |
| f"Blacklisted number {customer_mobile} rejected for merchant {req.merchant}" | |
| ) | |
| raise HTTPException( | |
| status_code=status.HTTP_403_FORBIDDEN, | |
| detail="Phone number is blacklisted", | |
| ) | |
| if customer_mobile is not None: | |
| normalized_mobile = str(customer_mobile) | |
| if await is_number_blacklisted(normalized_mobile, req.merchant): | |
| logger.warning( | |
| f"Blacklisted number {normalized_mobile} rejected for merchant {req.merchant}" | |
| ) | |
| raise HTTPException( | |
| status_code=status.HTTP_403_FORBIDDEN, | |
| detail="Phone number is blacklisted", | |
| ) |
| logger.warning( | ||
| f"Blacklisted number {customer_mobile} rejected for merchant {req.merchant}" |
There was a problem hiding this comment.
This warning log includes the raw customer phone number ({customer_mobile}), which is sensitive PII and is otherwise masked in the blacklist code. Consider logging a masked version (e.g., last 4 digits) or omitting the number entirely while keeping lead_id/merchant context.
| logger.warning( | |
| f"Blacklisted number {customer_mobile} rejected for merchant {req.merchant}" | |
| # Avoid logging full phone number PII; log only last 4 digits for context. | |
| customer_mobile_str = str(customer_mobile) | |
| masked_suffix = customer_mobile_str[-4:] if len(customer_mobile_str) >= 4 else customer_mobile_str | |
| logger.warning( | |
| f"Blacklisted number ending with {masked_suffix} rejected for merchant {req.merchant}" |
| if blacklist_phone and await is_number_blacklisted( | ||
| blacklist_phone, locked_lead.merchant_id |
There was a problem hiding this comment.
blacklist_phone is passed to is_number_blacklisted() without a type check. If an older lead has customer_mobile_number stored as a non-string, this can raise and hit the outer except which does not release the lead lock, potentially leaving the lead stuck locked. Consider guarding with isinstance(blacklist_phone, str) (and/or safe normalization) before calling is_number_blacklisted().
| if blacklist_phone and await is_number_blacklisted( | |
| blacklist_phone, locked_lead.merchant_id | |
| # Normalize phone to string when possible and guard against invalid types | |
| if isinstance(blacklist_phone, (int, float)): | |
| blacklist_phone = str(blacklist_phone) | |
| if ( | |
| isinstance(blacklist_phone, str) | |
| and blacklist_phone.strip() | |
| and await is_number_blacklisted( | |
| blacklist_phone, locked_lead.merchant_id | |
| ) |
Summary
Implements a comprehensive phone number blacklisting system for Breeze Buddy that prevents calls to blacklisted numbers at both lead push time and call execution time. The feature includes a dedicated database table, Redis caching for performance, and admin-only management APIs.
Key Changes
Database Layer
blacklisted_numberstable with per-merchant and global blacklist supportData Access Layer
API Enforcement
push_lead_handler: Rejects leads with blacklisted numbers upfront with 403 Forbiddenprocess_backlog_leads: Skips leads with blacklisted numbers during cron execution, marks them as BLACKLISTED outcomeManagement APIs
POST /blacklist- Add a number to the blacklistGET /blacklist- List all blacklisted numbers (with optional merchant filter)GET /blacklist/check/{phone_number}- Check if a number is blacklistedDELETE /blacklist/{phone_number}- Remove a number from the blacklistSchemas
BlacklistedNumbermodel for database representationCreateBlacklistNumberRequestfor API requestsImplementation Details
https://claude.ai/code/session_014juKikX794D1QC5zMW1xWX
Summary by CodeRabbit
Release Notes
New Features
Documentation