feat(telephony): rename outbound_number to telephony_numbers + per-merchant ownership#934
Conversation
WalkthroughThe PR renames the Breeze Buddy outbound-number domain to telephony numbers across schemas, storage, APIs, analytics, dispatch, call providers, transfers, inbound handling, RBAC, and tests. It also adds tenant ownership rules, shared-pool handling, number updates, and a compatibility database view. ChangesTelephony number migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/database/migrations/037_rename_outbound_number_to_telephony_numbers.sql (1)
43-43: 🧹 Nitpick | 🔵 TrivialOperational note: this view snapshots columns at creation time.
Any migration that adds a column to
telephony_numberswhile this compatibility view is still live will need aCREATE OR REPLACE VIEW(or the new column needs a default/nullable) — otherwise old-pod inserts throughoutbound_numberwill violate NOT NULL on the new column since the view doesn't expose it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/database/migrations/037_rename_outbound_number_to_telephony_numbers.sql` at line 43, Update the compatibility view created by the migration around outbound_number so future telephony_numbers columns do not cause old-pod inserts through the view to violate NOT NULL constraints. Use the project’s supported view-update strategy, or ensure subsequently added columns are nullable or have defaults while this view remains active.app/api/routers/breeze_buddy/numbers/rbac.py (1)
86-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Optional[List[str]]instead ofList[str] | None.The file already imports
Optionaland uses it elsewhere (template_reseller_id: Optional[str]); this parameter mixes in PEP 604 union syntax instead.As per coding guidelines,
**/*.py: "Use Optional[T], List[T], Dict[str, Any], Union for type annotations".♻️ Proposed fix
def filter_numbers_by_rbac( numbers: List[TelephonyNumber], current_user: UserInfo, - pinned_number_ids: List[str] | None = None, + pinned_number_ids: Optional[List[str]] = None, ) -> List[TelephonyNumber]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/routers/breeze_buddy/numbers/rbac.py` around lines 86 - 90, Update the pinned_number_ids annotation in filter_numbers_by_rbac to use Optional[List[str]] instead of List[str] | None, reusing the file’s existing Optional import and preserving the parameter’s optional behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/database/queries/breeze_buddy/telephony_number.py`:
- Around line 216-259: The telephony-number PATCH flow cannot clear stale
merchant ownership during a reseller-only change. Update
app/database/queries/breeze_buddy/telephony_number.py in
update_telephony_number_query,
app/database/accessor/breeze_buddy/telephony_number.py in its corresponding
update accessor, and app/api/routers/breeze_buddy/numbers/handlers.py in the
number update handler to add and propagate an explicit merchant_id-clearing
transition, or consistently reject reseller-only ownership changes; ensure
reseller_id updates never leave mismatched ownership metadata.
In `@tests/breeze_buddy/test_number_picker.py`:
- Line 101: Fix pyrefly type errors at every _get_available_number call site by
applying the test suite’s established structural-test-double suppression
convention, preferably cast(Any, ...) if that is the existing pattern. Wrap
make_config() and template arguments as needed at the referenced calls,
including both arguments at line 246, without changing runtime behavior.
---
Nitpick comments:
In `@app/api/routers/breeze_buddy/numbers/rbac.py`:
- Around line 86-90: Update the pinned_number_ids annotation in
filter_numbers_by_rbac to use Optional[List[str]] instead of List[str] | None,
reusing the file’s existing Optional import and preserving the parameter’s
optional behavior.
In `@app/database/migrations/037_rename_outbound_number_to_telephony_numbers.sql`:
- Line 43: Update the compatibility view created by the migration around
outbound_number so future telephony_numbers columns do not cause old-pod inserts
through the view to violate NOT NULL constraints. Use the project’s supported
view-update strategy, or ensure subsequently added columns are nullable or have
defaults while this view remains active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70666bc7-8650-4e19-98cb-4a3a6eeb197d
📒 Files selected for processing (43)
app/ai/voice/agents/breeze_buddy/agent/inbound.pyapp/ai/voice/agents/breeze_buddy/dispatch/alerts.pyapp/ai/voice/agents/breeze_buddy/dispatch/channel_semaphore.pyapp/ai/voice/agents/breeze_buddy/dispatch/reconcilers.pyapp/ai/voice/agents/breeze_buddy/dispatch/worker.pyapp/ai/voice/agents/breeze_buddy/handlers/internal/hold_and_consult.pyapp/ai/voice/agents/breeze_buddy/handlers/internal/warm_transfer.pyapp/ai/voice/agents/breeze_buddy/managers/calls.pyapp/ai/voice/agents/breeze_buddy/services/call_redirect.pyapp/ai/voice/agents/breeze_buddy/services/telephony/base_provider.pyapp/ai/voice/agents/breeze_buddy/services/telephony/exotel/conference.pyapp/ai/voice/agents/breeze_buddy/services/telephony/exotel/exotel.pyapp/ai/voice/agents/breeze_buddy/services/telephony/plivo/conference.pyapp/ai/voice/agents/breeze_buddy/services/telephony/plivo/plivo.pyapp/ai/voice/agents/breeze_buddy/services/telephony/twilio/conference.pyapp/ai/voice/agents/breeze_buddy/services/telephony/twilio/twilio.pyapp/api/routers/breeze_buddy/analytics/__init__.pyapp/api/routers/breeze_buddy/analytics/handlers.pyapp/api/routers/breeze_buddy/leads/handlers.pyapp/api/routers/breeze_buddy/numbers/__init__.pyapp/api/routers/breeze_buddy/numbers/handlers.pyapp/api/routers/breeze_buddy/numbers/rbac.pyapp/api/routers/breeze_buddy/telephony/answer/handlers.pyapp/api/routers/breeze_buddy/templates/handlers.pyapp/database/accessor/__init__.pyapp/database/accessor/breeze_buddy/analytics.pyapp/database/accessor/breeze_buddy/dispatch.pyapp/database/accessor/breeze_buddy/telephony_number.pyapp/database/decoder/breeze_buddy/telephony_number.pyapp/database/migrations/037_rename_outbound_number_to_telephony_numbers.sqlapp/database/queries/breeze_buddy/analytics.pyapp/database/queries/breeze_buddy/dispatch.pyapp/database/queries/breeze_buddy/lead_call_tracker.pyapp/database/queries/breeze_buddy/telephony_number.pyapp/schemas.pyapp/schemas/__init__.pyapp/schemas/breeze_buddy/__init__.pyapp/schemas/breeze_buddy/analytics.pyapp/schemas/breeze_buddy/core.pytests/breeze_buddy/dispatch/conftest.pytests/breeze_buddy/dispatch/test_end_to_end.pytests/breeze_buddy/dispatch/test_reconcilers.pytests/breeze_buddy/test_number_picker.py
| def update_telephony_number_query( | ||
| telephony_number_id: str, | ||
| status: Optional[TelephonyNumberStatus] = None, | ||
| maximum_channels: Optional[int] = None, | ||
| reseller_id: Optional[str] = None, | ||
| merchant_id: Optional[str] = None, | ||
| clear_ownership: bool = False, | ||
| ) -> Tuple[str, List[Any]]: | ||
| """ | ||
| Generate a partial-update query for a telephony number. | ||
|
|
||
| None means "leave unchanged"; clear_ownership=True nulls BOTH reseller_id | ||
| and merchant_id (returns the number to the shared platform pool) and wins | ||
| over any reseller_id/merchant_id values passed alongside it. | ||
| """ | ||
| sets: List[str] = [] | ||
| values: List[Any] = [telephony_number_id] | ||
|
|
||
| def add(col: str, val: Any) -> None: | ||
| values.append(val) | ||
| sets.append(f'"{col}" = ${len(values)}') | ||
|
|
||
| if status is not None: | ||
| add("status", status.value) | ||
| if maximum_channels is not None: | ||
| add("maximum_channels", maximum_channels) | ||
| if clear_ownership: | ||
| sets.append('"reseller_id" = NULL') | ||
| sets.append('"merchant_id" = NULL') | ||
| else: | ||
| if reseller_id is not None: | ||
| add("reseller_id", reseller_id) | ||
| if merchant_id is not None: | ||
| add("merchant_id", merchant_id) | ||
| sets.append('"updated_at" = NOW()') | ||
|
|
||
| text = f""" | ||
| UPDATE "{TELEPHONY_NUMBER_TABLE}" | ||
| SET {", ".join(sets)} | ||
| WHERE "id" = $1 | ||
| RETURNING *; | ||
| """ | ||
| return text, values | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== telephony_number query ==\n'
sed -n '1,320p' app/database/queries/breeze_buddy/telephony_number.py
printf '\n== telephony_number accessor ==\n'
sed -n '1,420p' app/database/accessor/breeze_buddy/telephony_number.py
printf '\n== update number handler ==\n'
sed -n '1,280p' app/api/routers/breeze_buddy/numbers/handlers.py
printf '\n== request/ownership models search ==\n'
rg -n "clear_ownership|merchant_id|reseller_id|UpdateTelephonyNumberRequest|_resolve_ownership|ownership" app/api app/database -g '*.py'Repository: juspay/clairvoyance
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== UpdateTelephonyNumberRequest and TelephonyNumber schema ==\n'
rg -n "class UpdateTelephonyNumberRequest|class TelephonyNumber|shared_pool|clear_ownership|maximum_channels" app/schemas app -g '*.py' -g '*.json' -g '*.yaml' -g '*.yml'
printf '\n== telephony number decoder ==\n'
sed -n '1,220p' app/database/decoder/breeze_buddy/telephony_number.py
printf '\n== telephony number table/schema references ==\n'
rg -n 'telephony_numbers|reseller_id|merchant_id' app -g '*.py' -g '*.sql' -g '*.yaml' -g '*.yml'
printf '\n== telephony number migrations/schema files ==\n'
fd -a 'telephony_number' . || true
fd -a 'telephony_numbers' . || true
fd -a '*.sql' . || trueRepository: juspay/clairvoyance
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== request schema definitions ==\n'
rg -n "class UpdateTelephonyNumberRequest|class CreateTelephonyNumberRequest|class TelephonyNumber" app/schemas -g '*.py' -A 80 -B 20
printf '\n== telephony number decoder (focused) ==\n'
sed -n '1,120p' app/database/decoder/breeze_buddy/telephony_number.py
printf '\n== migration 009 ==\n'
sed -n '1,120p' app/database/migrations/009_add_merchant_template_to_outbound_number.sql
printf '\n== any constraints/checks on telephony_numbers ==\n'
rg -n "CHECK .*telephony|UNIQUE .*telephony|FOREIGN KEY .*telephony|merchant_id.*reseller_id|reseller_id.*merchant_id" app/database/migrations app -g '*.sql' -g '*.py'Repository: juspay/clairvoyance
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== numbers router docs ==\n'
sed -n '1,220p' app/api/routers/breeze_buddy/numbers/__init__.py
printf '\n== call manager ownership handling ==\n'
sed -n '240,330p' app/ai/voice/agents/breeze_buddy/managers/calls.py
printf '\n== dispatch alert ownership wording ==\n'
sed -n '170,230p' app/ai/voice/agents/breeze_buddy/dispatch/alerts.py
printf '\n== telephony number migrations around ownership ==\n'
sed -n '1,220p' app/database/migrations/018_add_reseller_id_and_merchant_identifier_columns.sql
sed -n '1,120p' app/database/migrations/022_drop_legacy_merchant_id_and_shop_identifier_columns.sql
sed -n '1,120p' app/database/migrations/037_rename_outbound_number_to_telephony_numbers.sqlRepository: juspay/clairvoyance
Length of output: 27604
reseller_id-only updates can leave stale ownership metadata behind. The request/query shape has no way to express reseller_id = new and merchant_id = NULL in one PATCH, so re-parenting a merchant-owned number can produce a mismatched (reseller_id, merchant_id) row. Add an explicit transition for clearing merchant_id or reject reseller-only ownership changes.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 252-257: Possible SQL injection vector through string-based query construction
(S608)
📍 Affects 3 files
app/database/queries/breeze_buddy/telephony_number.py#L216-L259(this comment)app/database/accessor/breeze_buddy/telephony_number.py#L306-L349app/api/routers/breeze_buddy/numbers/handlers.py#L119-L181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/database/queries/breeze_buddy/telephony_number.py` around lines 216 -
259, The telephony-number PATCH flow cannot clear stale merchant ownership
during a reseller-only change. Update
app/database/queries/breeze_buddy/telephony_number.py in
update_telephony_number_query,
app/database/accessor/breeze_buddy/telephony_number.py in its corresponding
update accessor, and app/api/routers/breeze_buddy/numbers/handlers.py in the
number update handler to add and propagate an explicit merchant_id-clearing
transition, or consistently reject reseller-only ownership changes; ensure
reseller_id updates never leave mismatched ownership metadata.
There was a problem hiding this comment.
Addressed in 080bad9 at the layer you traced it to — see the reply on the handlers.py thread: ownership updates are now full reassignments (set_ownership=True rewrites both columns, None → NULL), so a reseller-only update can no longer leave a stale merchant_id behind. Unit-tested at the query-builder level.
There was a problem hiding this comment.
@swaroopvarma1, verified. set_ownership=True now atomically rewrites both ownership columns, including binding None as NULL, while clear_ownership still clears both fields. This resolves the stale ownership metadata path. Thanks for adding query-builder coverage.
🧠 Learnings used
Learnt from: murdore
Repo: juspay/clairvoyance PR: 930
File: app/api/routers/breeze_buddy/leads/rbac.py:106-108
Timestamp: 2026-07-21T05:53:58.824Z
Learning: In this repository, the CI lint gate runs Black, isort, autoflake, and pyrefly, and does not run Ruff. Therefore, when reviewing Python changes, treat Ruff-specific findings (e.g., rules like F541) as non-blocking code-quality cleanup items rather than CI-breaking issues. Only mark Ruff findings as blocking if the CI/lint-gate configuration is updated to include Ruff.
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed 43 changed files. The rename migration, picker tier logic, and RBAC scoping for /numbers are sound, but I found new blocking issues that must be fixed before merge.
New issues raised
| Severity | Count | Files |
|---|---|---|
| 🔒 CRITICAL | 3 | app/api/routers/breeze_buddy/numbers/rbac.py (×2), app/api/routers/breeze_buddy/analytics/handlers.py |
| 1 | app/api/routers/breeze_buddy/numbers/handlers.py |
Blocking reasons:
- PII in authenticated error responses —
require_number_in_tenant_scopereturns the raw phone number (number.number) in HTTP 400 details for both the merchant-mismatch and reseller-mismatch branches. Phone numbers are PII and should not be exposed in wire responses; usenumber.idinstead. - Cross-tenant PII leak in analytics —
get_telephony_numbers_analyticsreturns the rawnumberfield for every telephony number with no tenant scoping. Non-admin users can requestAnalyticsType.TELEPHONY_NUMBERSand receive phone numbers owned by other merchants/umbrellas. This needs the same ownership + pinned-id filtering used by/numbers, or admin-only gating. - PATCH can create hybrid/incorrect ownership — a reseller-only PATCH on a merchant-owned number leaves the old
merchant_idin place because the query builder treatsmerchant_id=Noneas "unchanged". The row ends up with a newreseller_idand a stalemerchant_id, violating the ownership model.
Existing comments acknowledged (not duplicated)
- CodeRabbit's comment on stale ownership metadata during reseller-only updates in
telephony_number.py— myhandlers.pycomment traces the same root cause to the handler layer and proposes a concrete fix. - CodeRabbit's pyrefly type-error comment in
tests/breeze_buddy/test_number_picker.py— CI-blocking, please apply the suggestedcast(Any, …)pattern. - Tara-ag's comment on logging the raw phone number in
managers/calls.py— please redact that log line.
Next steps
- Remove raw phone numbers from
require_number_in_tenant_scopeerror details. - Scope or redact the
telephony_numbersanalytics response for non-admin callers. - Ensure
PATCH /numbers/{id}clearsmerchant_idwhen moving a number to umbrella-only ownership (or reject the transition explicitly). - Address the acknowledged existing comments.
Once the CRITICAL items are resolved, this should be good to approve.
| f"outbound_number_id {template.outbound_number_id} which does not " | ||
| "exist. This is a misconfiguration that will not self-heal." | ||
| ) | ||
| else: |
There was a problem hiding this comment.
💡 MINOR: PII in warning log
This warning logs the raw phone number (telephony_number.number). Phone numbers are PII, and the project rules flag logging PII. While the audit-trail intent is clear, prefer logging the number ID and keep the raw number out of application logs.
logger.warning(
f"_get_available_number: grandfathered cross-merchant pin — "
f"template {config.template} (merchant {config.merchant_id}) "
f"dials from number_id {telephony_number.id} owned by "
f"merchant {telephony_number.merchant_id}."
)If operators need the actual number, surface it via an admin-facing audit API or masked UI rather than logs.
There was a problem hiding this comment.
Fixed in 080bad9 — the grandfathered-pin warning now logs number_id (your suggested wording). Also redacted the success-path "Using outbound number …" info log in the same function to number_id, since it had the same PII issue. The unit test now asserts the raw number never appears in the warning.
| ) | ||
| return | ||
| if number.reseller_id is not None and number.reseller_id != template_reseller_id: | ||
| raise HTTPException( |
There was a problem hiding this comment.
🔒 CRITICAL: PII exposure in API error response
This HTTPException detail interpolates number.number (the raw phone number) into the 400 response returned to the client. Phone numbers are PII, and the project rules flag returning PII in unauthenticated/error responses.
Suggested fix: return the number's UUID (number.id) in the error detail, and keep the raw phone number out of the wire response. Operators can resolve the number from the admin UI or an audit endpoint.
detail=(
f"Number {number.id} is provisioned for merchant "
f"'{number.merchant_id}' and cannot be pinned by a "
f"template of '{template_merchant_id}'. Use a shared "
"number, one of this merchant's own numbers, or "
"re-assign the number first."
),Same applies to the reseller-only branch a few lines below.
There was a problem hiding this comment.
Fixed in 080bad9 — the 400 detail now carries number.id instead of the raw phone number (both branches; comment added noting the id can be resolved via GET /numbers/{id}).
|
|
||
| Returns: | ||
| List of outbound numbers (currently returns all for read operations) | ||
| def filter_numbers_by_rbac( |
There was a problem hiding this comment.
🔒 CRITICAL: PII exposure in API error response (reseller branch)
Same issue as the merchant branch above: the raw phone number (number.number) is interpolated into the HTTP 400 response body. Return number.id instead so the error response never carries PII.
detail=(
f"Number {number.id} belongs to the "
f"'{number.reseller_id}' umbrella and cannot be pinned by a "
f"template under '{template_reseller_id}'."
),There was a problem hiding this comment.
Fixed in 080bad9 — reseller branch now returns number.id as well; no raw phone numbers in any wire response from this module.
| Raises: | ||
| HTTPException: 404 if not found, 400 on invalid ownership | ||
| """ | ||
| logger.info( |
There was a problem hiding this comment.
merchant_id when moving from merchant-owned to umbrella-owned
The handler's _resolve_ownership helper is called with update.merchant_id and update.reseller_id. If the existing number is merchant-owned and the admin sends only reseller_id (intending to move it to umbrella ownership), _resolve_ownership receives merchant_id=None and returns (None, reseller_id). The generated SQL then omits merchant_id from the SET clause, leaving the old merchant_id in place. The row ends up with both reseller_id (new umbrella) and merchant_id (old merchant), which violates the ownership model and could make the number appear merchant-owned to the picker/RBAC.
Suggested fix: in update_number_handler, when update.merchant_id is explicitly None but update.reseller_id is provided (and clear_ownership is false), explicitly clear merchant_id in the update. One way is to pass a sentinel that distinguishes "omit" from "clear to NULL" down to the query builder, e.g.:
merchant_id, reseller_id = await _resolve_ownership(
update.merchant_id, update.reseller_id
)
# If the caller explicitly set reseller_id without merchant_id, clear any
# existing merchant_id so the row becomes umbrella-owned, not hybrid-owned.
if update.reseller_id is not None and update.merchant_id is None:
merchant_id = None # signal "set NULL"
# update_telephony_number_query must then emit '"merchant_id" = NULL'Then adjust update_telephony_number_query so that merchant_id=None passed as an explicit kwarg emits "merchant_id" = NULL rather than being skipped. Alternatively, add a dedicated clear_merchant_id flag to the request schema and query builder.
This addresses the data-integrity concern raised in the existing CodeRabbit comment on telephony_number.py about reseller-only updates leaving stale ownership metadata.
There was a problem hiding this comment.
Fixed in 080bad9 — ownership on PATCH is now all-or-nothing: touching either ownership field makes the handler resolve the full pair and pass set_ownership=True, and the query builder then rewrites BOTH columns (None → NULL). A reseller-only PATCH on a merchant-owned number now nulls the stale merchant_id. clear_ownership still wins over everything; a status/capacity-only PATCH leaves ownership untouched. Covered by two new unit tests on the query builder (test_update_query_reassignment_rewrites_both_ownership_columns, test_update_query_without_ownership_flags_leaves_ownership_untouched).
|
|
||
|
|
||
| async def get_outbound_numbers_analytics( | ||
| async def get_telephony_numbers_analytics( |
There was a problem hiding this comment.
🔒 CRITICAL: Analytics outbound-numbers response leaks PII and lacks tenant scoping
get_telephony_numbers_analytics returns the raw phone number (record["number"]) for every outbound number, plus the reseller_id/merchant_id ownership fields, without any RBAC filtering. The analytics router applies apply_hierarchical_filters to filters, but that only constrains the underlying lead_call_tracker rows via reseller_id/merchant_id; the LEFT JOIN still returns every telephony_numbers row that has ever been used (and NULL-owner rows for unused numbers), exposing phone numbers across tenants.
Impact: a merchant user can request AnalyticsType.TELEPHONY_NUMBERS and receive phone numbers owned by other merchants/umbrellas.
Suggested fix: either (a) scope this analytics type to admin-only in the router, or (b) post-filter the results through the same ownership + pinned-id logic used by /numbers, and redact/mask the number field for non-admin callers. At minimum:
if current_user.role != "admin":
# Reuse the numbers RBAC helper
from app.api.routers.breeze_buddy.numbers.rbac import filter_numbers_by_rbac
outbound_data = [
{"id": r["id"], ...} for r in outbound_data
]
# ...then filter and redact numberAlso consider whether number should be returned at all in analytics for non-admin users; the /numbers endpoint already gates this.
There was a problem hiding this comment.
Fixed in 080bad9 — get_telephony_numbers_analytics now applies the exact same visibility rule as GET /numbers for non-admins: owned by the caller's merchants/umbrellas ∪ template-pinned ids. The predicate was extracted into numbers/rbac.py::number_in_rbac_scope and is shared by filter_numbers_by_rbac and the analytics handler, so the two can't drift. Admins keep the fleet view. Covered by test_telephony_analytics_scoped_like_numbers_endpoint.
5072479 to
080bad9
Compare
Review fixes — 080bad9 (amended)All blocking items from @Tara-ag's review + CodeRabbit are addressed:
Tests: 15/15 (12 existing + 3 new: ownership-reassignment query ×2, analytics tenant scoping). black + isort applied. |
080bad9 to
2274b16
Compare
Visibility model tightened — 2274b16 (amended)Ownership visibility is now strictly downward:
The important part: merchant JWTs carry their umbrella in Tests now 17/17 (new: merchant-with-umbrella-JWT sees nothing umbrella-wide; reseller sees merchant-owned numbers under their umbrella; analytics asserts the same split). pyrefly clean. |
2274b16 to
ff8ef1a
Compare
CodeRabbit nitpicks — triage (ff8ef1a)
|
ff8ef1a to
23a36b6
Compare
|
23a36b6: grandfathered cross-merchant pin log bumped warning → error (deliberate, with a code comment): error-level feeds the existing ops alerting, so every grandfathered pin that still dials stays visible until the post-deploy ownership relabel clears them. Call behavior unchanged — it still never blocks. Test updated to assert the error-level path. 17/17. |
23a36b6 to
c137f2e
Compare
Self-review finding (multi-agent) — fixed in c137f2eCritical, caught before merge: Impact would have been: merchants dialing from shared/pinned numbers see an empty phone page, 404 on single-GET, and those rows missing from analytics (under-exposure, not a leak). Fix: |
…rchant ownership
The table has stored inbound DIDs as well as outbound caller IDs ever since
inbound calling landed, so the old name is a misnomer. Migration 037 renames
the table, indexes and constraints, and leaves an updatable compatibility
VIEW named outbound_number so pods on the previous release keep working
through a rolling deploy. Wire-visible column names (template/
lead_call_tracker.outbound_number_id) are intentionally NOT renamed.
Per-merchant ownership (reseller_id/merchant_id existed since 018/022 but
were vestigial — the picker only ever matched the NULL/NULL shared pool):
- picker: template pin > merchant-owned number > legacy shared pool, so
numbers shared across many merchants/templates keep working unchanged
- POST /numbers: ownership is an explicit choice (merchant_id with umbrella
auto-fill from merchants.reseller_id, reseller-only, or shared_pool=true;
previously reseller_id was force-required yet unused by routing)
- new PATCH /numbers/{id}: assign/release ownership, status, max channels
(semaphore resync rides the channel-token reconciler)
- GET /numbers + /numbers/{id}: real RBAC scoping — admin sees the fleet,
others see owned numbers plus the ids their templates pin (the old filter
returned the entire fleet to any authenticated user)
- template pins are tenant-scoped: require_number_in_tenant_scope rejects
NEW or CHANGED template.outbound_number_id values pointing at another
merchant's/umbrella's number (shared pool always allowed). Existing
cross-merchant pins are grandfathered — unrelated GET->edit->PUT round
trips keep passing, and the picker dials them with a loud warning until
the ownership backfill re-labels the fleet (prod audit: 4 shared
workhorse numbers carry stale merchant labels, pinned by up to 540
foreign merchants)
- analytics outbound-numbers rows now carry reseller_id/merchant_id
Python identifiers swept outbound_number -> telephony_number (56 files,
mechanical); protected: outbound_number_id columns/fields and the plivo
callback param key "outbound_number" (wire compat). Unit tests cover the
three picker tiers, the RBAC filter, tenant-scope pin validation and the
grandfathered-pin path (12 tests).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F51zhmjVXXKYhM1RHf2huz
c137f2e to
fb61c48
Compare
Autonomous review summaryState: PR is already merged; an existing pending review by another reviewer prevented opening a new pending review, so this summary is posted as a general comment. Scope: Reviewed the diff against the project standards (SQL safety, auth/RBAC, input validation, secrets/PII, async resilience, migrations). I did not duplicate points already raised and acknowledged in existing comment threads. Findings not yet resolved in the merged code
Already-handled issues (not duplicated)
Migration note
DecisionBecause the PR is already merged and the outstanding CRITICAL/MAJOR items above were not addressed before merge, they should be tracked as follow-up work (security/auth hardening and observability/PII cleanup). The two CRITICAL items (template-handler tenant trust and analytics PII return) warrant a follow-up PR before this feature is considered fully hardened. |
Why
The
outbound_numbertable has stored inbound DIDs as well as outbound caller IDs ever since inbound calling landed (template.outbound_number_iddoubles as the inbound routing key), so the name is a misnomer. And itsreseller_id/merchant_idcolumns (added in 018/022) were vestigial: routing never read them,POST /numbersforce-required areseller_idit never used, andGET /numbersreturned the entire fleet to any authenticated user.What
Migration 038 — zero-downtime rename (renumbered from 037 after rebase: release now carries
037_add_inbound_outside_hours_message.sql)outbound_number→telephony_numbers, with index + constraint renames.outbound_numberso pods on the previous release keep reading AND writing through the old name during a rolling deploy. Dropped in a later migration once old pods are gone.template.outbound_number_id,lead_call_tracker.outbound_number_id) are intentionally NOT renamed — they're client-facing API fields; a later contract-cleanup phase handles them with a dual-field window.Per-merchant ownership (backward compatible)
merchant_id == config.merchant_id) > legacy shared NULL/NULL pool. Merchants without owned numbers behave exactly as before, so numbers shared across many merchants/templates keep working.merchant_id(umbrella auto-filled frommerchants.reseller_id),reseller_idonly, orshared_pool: true; anything else 400s.PATCH /numbers/{id}: assign/release ownership (clear_ownership), status,maximum_channels(the channel-token reconciler re-syncs the Redis semaphore on its next pass).require_number_in_tenant_scoperejects NEW or CHANGEDtemplate.outbound_number_idvalues pointing at another merchant's/umbrella's number (shared pool always allowed; admins included). Existing cross-merchant pins are grandfathered: unrelated GET→edit→PUT round-trips keep passing, and the picker dials them with a loudgrandfathered cross-merchant pinwarning instead of dropping live traffic.outbound-numbersrows now carryreseller_id/merchant_id(additive).Naming sweep
outbound_number→telephony_number/OutboundNumber→TelephonyNumber; query/accessor/decoder modules renamed. Protected for wire compat:outbound_number_ididentifiers and the plivo callback param literal"outbound_number"(in-flight call URLs straddle deploys).Prod data note (existing mess, measured on a prod snapshot)
649 templates across 568 merchants pin a number labeled as another merchant's — but it collapses to 4 stale-labeled shared workhorse numbers (one pinned by 540 merchants yet labeled
acme). Plan after deploy:PATCH … clear_ownershipto relabel the de-facto shared ones; until then grandfathering + call-time warnings hold the line. Worth doing promptly — the new merchant-owned picker tier makes stale labels active.Verification
tests/breeze_buddy/test_number_picker.py— 12 tests: the three picker tiers, RBAC filter (incl. wildcard-no-leak), tenant-scope pin validation, grandfathered-pin warn path.test_chat_analyticscases that fail on cleanreleasetoo./numbersthrough the view after the table was renamed underneath it.migrate.py statusclean. Prod migration pending deploy sequencing.🤖 Generated with Claude Code
https://claude.ai/code/session_01F51zhmjVXXKYhM1RHf2huz
Summary by CodeRabbit