feat(sinks): add Telnyx PSTN sink for outbound call playback - #1043
feat(sinks): add Telnyx PSTN sink for outbound call playback#1043a692570 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an opt-in Telnyx outbound calling sink with persisted settings, call tracking, asynchronous generation playback, webhook handling, REST and MCP interfaces, tests, and setup documentation. ChangesTelnyx outbound calling sink
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟡 Moderate · up to Outbound calls may fail when call records are persisted, invalid phone numbers may produce opaque carrier errors, and webhook failures may leave calls stuck without clear recovery; merge should wait for these bounded correctness and reliability issues to be fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Client
participant Voicebox
participant Telnyx
participant Webhook
participant Generation
Client->>Voicebox: Submit text or generation_id
Voicebox->>Generation: Create or validate generation
Voicebox->>Telnyx: Create outbound call
Telnyx->>Webhook: Send answered event
Webhook->>Generation: Wait for completed audio
Webhook->>Telnyx: Start audio playback
Telnyx->>Webhook: Send playback-ended event
Webhook->>Telnyx: Hang up when enabled
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
backend/routes/sinks.py (2)
168-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain the original exception.
Add
from excso theValueErrortraceback survives in logs. Ruff reports this as B904.♻️ Proposed refactor
except ValueError as exc: - raise HTTPException(status_code=400, detail=str(exc)) + raise HTTPException(status_code=400, detail=str(exc)) from exc🤖 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 `@backend/routes/sinks.py` around lines 168 - 169, Update the ValueError handler in the surrounding route function to chain the original exception when raising HTTPException, preserving exc as the explicit cause while keeping the existing status code and detail unchanged.Source: Linters/SAST tools
42-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the response builder.
The
TelnyxSettingsResponseconstruction is identical inget_telnyx_settingsandupdate_telnyx_settings. Extract a small helper so a new settings field needs one edit.♻️ Proposed refactor
+def _settings_response(row) -> models.TelnyxSettingsResponse: + return models.TelnyxSettingsResponse( + enabled=bool(row.enabled), + api_key_masked=sinks_service.mask_key(row.api_key), + api_key_set=bool(row.api_key), + from_number=row.from_number, + public_base_url=row.public_base_url, + default_profile_id=row.default_profile_id, + auto_hangup=bool(row.auto_hangup), + )Then return
_settings_response(row)from both endpoints.Also applies to: 70-78
🤖 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 `@backend/routes/sinks.py` around lines 42 - 50, Extract the duplicated TelnyxSettingsResponse construction into a shared _settings_response(row) helper, preserving all existing field mappings and conversions. Update both get_telnyx_settings and update_telnyx_settings to return _settings_response(row) so future settings fields require one change.backend/mcp_server/tools.py (2)
322-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the no-op exception wrapper.
The
except ValueErrorblock re-raises a newValueErrorwith the same message. It changes nothing for the caller. Remove it and let the original exception propagate.♻️ Proposed refactor
- try: - result = await sinks_service.place_call( - to=to, - text=text, - generation_id=generation_id, - profile_id=vp.id, - profile_name=vp.name, - engine=engine, - language=language, - personality=use_persona, - auto_hangup=auto_hangup, - db=db, - ) - except ValueError as exc: - raise ValueError(str(exc)) from exc + result = await sinks_service.place_call( + to=to, + text=text, + generation_id=generation_id, + profile_id=vp.id, + profile_name=vp.name, + engine=engine, + language=language, + personality=use_persona, + auto_hangup=auto_hangup, + db=db, + )🤖 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 `@backend/mcp_server/tools.py` around lines 322 - 336, Remove the redundant ValueError try/except wrapper around the sinks_service.place_call invocation in the tool handler, allowing the original exception to propagate unchanged while preserving the existing call arguments and result handling.
289-319: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFetch the client binding once.
The code queries
MCPClientBindingfor the sameclient_idtwice: once for the profile and once for the personality default. Load it once and reuse the row. This also removes the nestedifthat Ruff reports as SIM102.♻️ Proposed refactor
+ binding = None + if client_id: + binding = ( + db.query(MCPClientBinding) + .filter(MCPClientBinding.client_id == client_id) + .first() + ) + if vp is None: # Per-client binding next. - if client_id: - binding = ( - db.query(MCPClientBinding) - .filter(MCPClientBinding.client_id == client_id) - .first() - ) - if binding and binding.profile_id: - vp = resolve_profile(binding.profile_id, client_id, db) + if binding and binding.profile_id: + vp = resolve_profile(binding.profile_id, client_id, db) if vp is None: # Sink default last. s = sinks_service.get_settings(db) if s.default_profile_id: vp = resolve_profile(s.default_profile_id, client_id, db) if vp is None: raise ValueError( "No voice profile resolved. Pass `profile=`, set a " "default in Voicebox → Settings → Sinks → Telnyx, or " "bind a profile to this client in Settings → MCP." ) resolved_personality = personality - if resolved_personality is None and client_id: - binding = ( - db.query(MCPClientBinding) - .filter(MCPClientBinding.client_id == client_id) - .first() - ) - if binding is not None: - resolved_personality = bool(binding.default_personality) + if resolved_personality is None and binding is not None: + resolved_personality = bool(binding.default_personality)🤖 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 `@backend/mcp_server/tools.py` around lines 289 - 319, The profile and personality resolution flow queries MCPClientBinding twice for the same client_id. Fetch the binding once before the profile fallback logic, reuse it for both profile and personality resolution, and simplify the personality condition to avoid the nested if flagged by Ruff SIM102.Source: Linters/SAST tools
backend/services/sinks.py (1)
224-224: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUse
datetime.UTCfor both timestamps. Ruff reports UP017 atbackend/services/sinks.py#L224and#L336. Remove the unusedtimezoneimport.🤖 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 `@backend/services/sinks.py` at line 224, Update both timestamp constructions in backend/services/sinks.py at lines 224 and 336 to use datetime.UTC instead of timezone.utc, and remove the now-unused timezone import.Source: Linters/SAST tools
🤖 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 `@backend/mcp_server/tools.py`:
- Around line 238-247: Update voicebox_call to validate to against the same
E.164 pattern enforced by CallRequest before invoking place_call. Reject invalid
values with a clear ValueError correction message, and leave the existing call
flow unchanged for valid numbers.
In `@backend/models.py`:
- Around line 917-921: Update the public_base_url field validation in the model
containing public_base_url to accept only valid HTTPS URLs, while allowing None
for unset configuration. Reject HTTP URLs and non-URL strings during settings
validation, preserving the existing 200-character limit and description.
In `@backend/routes/sinks.py`:
- Around line 140-144: Update the profile-resolution flow in the endpoint so
omitted profiles first resolve the per-client MCP binding using the request’s
client identifier, before falling back to the sink’s default profile. Reuse the
existing binding-aware resolution behavior from backend/mcp_server/tools.py and
ensure explicit profiles retain precedence.
In `@backend/services/sinks.py`:
- Around line 427-434: Align the call status contract across all affected sites
by choosing one value and applying it consistently:
backend/services/sinks.py#L427-L434 in place_call, backend/models.py#L870-L884
in the CallResponse docstring, backend/routes/sinks.py#L105-L113 in
call_endpoint, and docs/content/docs/overview/sinks.mdx#L111-L120 in the sample
response. Either return and document "dialing" or preserve the persisted
"initiating" value and update every documentation reference accordingly.
- Around line 206-215: Update the call creation flow around
TelnyxClient.create_call to use only data.get("call_control_id") and raise
ValueError when it is missing; do not fall back to call_session_id. Catch Telnyx
create_call HTTPStatusError failures and convert them into ValueError with the
relevant Telnyx response detail so backend/routes/sinks.py continues mapping
rejected requests to HTTP 400.
- Around line 264-268: Update the webhook token validation around
row.webhook_secret to fail closed when the stored secret is missing, returning
the existing bad_token response. For non-missing secrets, compare
row.webhook_secret and token with secrets.compare_digest, importing secrets as
needed; remove the legacy-row acceptance logic.
- Around line 283-296: Update the webhook handler around _wait_for_generation to
claim the row before waiting: add the answered/in-progress state to the
duplicate guard, set the intermediate status, and commit it before starting
asynchronous work. Move generation waiting and playback_start into a background
task, then return the webhook acknowledgment immediately so retries
short-circuit and cannot start playback twice.
- Around line 138-155: Add the Telnyx connection_id setting and
migration/default handling, include it in is_configured validation, and update
create_call to require and send connection_id in the POST /calls payload. Update
dial_and_play to read the configured connection_id and pass it to create_call,
preserving the existing outbound call flow.
In `@docs/content/docs/overview/sinks.mdx`:
- Around line 65-75: Update the Telnyx sink documentation around “Test
connection” and the “Call” button to clearly mark both UI features as planned or
not yet available, rather than describing them as currently usable. Preserve the
existing sink setup and call behavior descriptions while relocating these
controls to an appropriate roadmap note if needed.
- Around line 57-63: Update the field label in the configuration table from
default_profile to default_profile_id, matching the API field name used by
TelnyxSettingsResponse and TelnyxSettingsUpdate; leave the description
unchanged.
- Around line 158-160: Update the Telnyx webhook documentation link in the sink
overview text to the current official Telnyx webhook documentation URL, keeping
the surrounding signature-verification guidance unchanged.
---
Nitpick comments:
In `@backend/mcp_server/tools.py`:
- Around line 322-336: Remove the redundant ValueError try/except wrapper around
the sinks_service.place_call invocation in the tool handler, allowing the
original exception to propagate unchanged while preserving the existing call
arguments and result handling.
- Around line 289-319: The profile and personality resolution flow queries
MCPClientBinding twice for the same client_id. Fetch the binding once before the
profile fallback logic, reuse it for both profile and personality resolution,
and simplify the personality condition to avoid the nested if flagged by Ruff
SIM102.
In `@backend/routes/sinks.py`:
- Around line 168-169: Update the ValueError handler in the surrounding route
function to chain the original exception when raising HTTPException, preserving
exc as the explicit cause while keeping the existing status code and detail
unchanged.
- Around line 42-50: Extract the duplicated TelnyxSettingsResponse construction
into a shared _settings_response(row) helper, preserving all existing field
mappings and conversions. Update both get_telnyx_settings and
update_telnyx_settings to return _settings_response(row) so future settings
fields require one change.
In `@backend/services/sinks.py`:
- Line 224: Update both timestamp constructions in backend/services/sinks.py at
lines 224 and 336 to use datetime.UTC instead of timezone.utc, and remove the
now-unused timezone import.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bdf236e9-4554-4c8d-986e-c28f041d460d
📒 Files selected for processing (10)
backend/database/__init__.pybackend/database/migrations.pybackend/database/models.pybackend/mcp_server/tools.pybackend/models.pybackend/routes/__init__.pybackend/routes/sinks.pybackend/services/sinks.pydocs/content/docs/overview/meta.jsondocs/content/docs/overview/sinks.mdx
| async def voicebox_call( | ||
| to: str, | ||
| text: str | None = None, | ||
| generation_id: str | None = None, | ||
| profile: str | None = None, | ||
| engine: str | None = None, | ||
| personality: bool | None = None, | ||
| language: str | None = None, | ||
| auto_hangup: bool | None = None, | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate to as E.164 in the MCP tool.
CallRequest.to enforces ^\+[1-9]\d{6,14}$ on the REST surface. This tool accepts any string. An agent that passes "555-123-4567" reaches Telnyx, and create_call raises httpx.HTTPStatusError, which the tool does not convert to a ValueError. The agent then receives an opaque error instead of a correction hint.
Validate the format before calling place_call.
🛡️ Proposed fix
+import re
...
if bool(text) == bool(generation_id):
raise ValueError(
"Pass exactly one of `text` (generate inline) or "
"`generation_id` (use an existing generation)."
)
+ if not re.fullmatch(r"\+[1-9]\d{6,14}", to):
+ raise ValueError(
+ "`to` must be an E.164 phone number, e.g. '+15551234567'."
+ )
+🤖 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 `@backend/mcp_server/tools.py` around lines 238 - 247, Update voicebox_call to
validate to against the same E.164 pattern enforced by CallRequest before
invoking place_call. Reject invalid values with a clear ValueError correction
message, and leave the existing call flow unchanged for valid numbers.
| public_base_url: Optional[str] = Field( | ||
| None, | ||
| max_length=200, | ||
| description="Public HTTPS URL of this Voicebox server (ngrok, Tailscale Funnel, etc.).", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the public_base_url scheme.
The description states the value is a public HTTPS URL. The field accepts any string up to 200 characters. Telnyx does not POST to http:// URLs, which docs/content/docs/overview/sinks.mdx also states. If a user saves an http:// URL or a non-URL string, the save succeeds and the failure appears later as an opaque Telnyx error during the call.
Reject a non-HTTPS value at settings-save time.
🛡️ Proposed fix to validate the URL scheme
public_base_url: Optional[str] = Field(
None,
max_length=200,
+ pattern=r"^https://\S+$",
description="Public HTTPS URL of this Voicebox server (ngrok, Tailscale Funnel, etc.).",
)📝 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.
| public_base_url: Optional[str] = Field( | |
| None, | |
| max_length=200, | |
| description="Public HTTPS URL of this Voicebox server (ngrok, Tailscale Funnel, etc.).", | |
| ) | |
| public_base_url: Optional[str] = Field( | |
| None, | |
| max_length=200, | |
| pattern=r"^https://\S+$", | |
| description="Public HTTPS URL of this Voicebox server (ngrok, Tailscale Funnel, etc.).", | |
| ) |
🤖 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 `@backend/models.py` around lines 917 - 921, Update the public_base_url field
validation in the model containing public_base_url to accept only valid HTTPS
URLs, while allowing None for unset configuration. Reject HTTP URLs and non-URL
strings during settings validation, preserving the existing 200-character limit
and description.
| return { | ||
| "call_control_id": call_row.call_control_id, | ||
| "status": call_row.status, | ||
| "generation_id": used_generation_id, | ||
| "profile": profile_name, | ||
| "to": to, | ||
| "poll_url": f"/generate/{used_generation_id}/status", | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The returned call status is "initiating", not the documented "dialing". dial_and_play persists status="initiating" and place_call returns that value verbatim, but three documentation sites promise "dialing". Pick one value and align every site.
backend/services/sinks.py#L427-L434: return the documented"dialing"value, or change the persisted status at line 223 to match the contract.backend/models.py#L870-L884: update theCallResponsedocstring to the value actually returned.backend/routes/sinks.py#L105-L113: update thecall_endpointdocstring claimstatus="dialing".docs/content/docs/overview/sinks.mdx#L111-L120: update the sample response body"status": "dialing".
📍 Affects 4 files
backend/services/sinks.py#L427-L434(this comment)backend/models.py#L870-L884backend/routes/sinks.py#L105-L113docs/content/docs/overview/sinks.mdx#L111-L120
🤖 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 `@backend/services/sinks.py` around lines 427 - 434, Align the call status
contract across all affected sites by choosing one value and applying it
consistently: backend/services/sinks.py#L427-L434 in place_call,
backend/models.py#L870-L884 in the CallResponse docstring,
backend/routes/sinks.py#L105-L113 in call_endpoint, and
docs/content/docs/overview/sinks.mdx#L111-L120 in the sample response. Either
return and document "dialing" or preserve the persisted "initiating" value and
update every documentation reference accordingly.
| | Field | Value | | ||
| | --- | --- | | ||
| | `api_key` | Your Telnyx API key | | ||
| | `from_number` | The number you bought, in E.164 (`+1...`) | | ||
| | `public_base_url` | The tunnel URL from step 2 | | ||
| | `default_profile` | The voice profile to use when a caller doesn't specify one | | ||
| | `auto_hangup` | Whether to hang up the call after playback ends (default: on) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the real field name default_profile_id.
The other rows in this table use the exact API field names (api_key, from_number, public_base_url, auto_hangup). The profile row says default_profile. The field is default_profile_id in TelnyxSettingsResponse and TelnyxSettingsUpdate.
📝 Proposed fix
-| `default_profile` | The voice profile to use when a caller doesn't specify one |
+| `default_profile_id` | The voice profile to use when a caller doesn't specify one |📝 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.
| | Field | Value | | |
| | --- | --- | | |
| | `api_key` | Your Telnyx API key | | |
| | `from_number` | The number you bought, in E.164 (`+1...`) | | |
| | `public_base_url` | The tunnel URL from step 2 | | |
| | `default_profile` | The voice profile to use when a caller doesn't specify one | | |
| | `auto_hangup` | Whether to hang up the call after playback ends (default: on) | | |
| | Field | Value | | |
| | --- | --- | | |
| | `api_key` | Your Telnyx API key | | |
| | `from_number` | The number you bought, in E.164 (`+1...`) | | |
| | `public_base_url` | The tunnel URL from step 2 | | |
| | `default_profile_id` | The voice profile to use when a caller doesn't specify one | | |
| | `auto_hangup` | Whether to hang up the call after playback ends (default: on) | |
🤖 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 `@docs/content/docs/overview/sinks.mdx` around lines 57 - 63, Update the field
label in the configuration table from default_profile to default_profile_id,
matching the API field name used by TelnyxSettingsResponse and
TelnyxSettingsUpdate; leave the description unchanged.
| Hit **Test connection** — Voicebox will ping Telnyx and tell you if the key | ||
| is valid. | ||
|
|
||
| ## Using the sink | ||
|
|
||
| ### From the UI | ||
|
|
||
| When the Telnyx sink is configured, a **Call** button appears on each | ||
| generation card next to Play and Speak. Click it, enter a destination | ||
| number, and dial. The generation plays on the call once the callee picks | ||
| up. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the UI features as not yet available.
The PR objectives state that this change includes no frontend UI changes. This page documents a Test connection button and a Call button on each generation card as available today. A reader will look for controls that do not exist yet.
Label both as planned, or move them to a roadmap note.
🤖 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 `@docs/content/docs/overview/sinks.mdx` around lines 65 - 75, Update the Telnyx
sink documentation around “Test connection” and the “Call” button to clearly
mark both UI features as planned or not yet available, rather than describing
them as currently usable. Preserve the existing sink setup and call behavior
descriptions while relocating these controls to an appropriate roadmap note if
needed.
| For full webhook signature verification against Telnyx's HMAC header, see | ||
| the [Telnyx webhook docs](https://developers.telnyx.com/docs/api/v2/webhooks). | ||
| This is a follow-up hardening step tracked in the sink's TODO. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check that the Telnyx webhook docs URL resolves.
curl -sSL -o /dev/null -w '%{http_code} %{url_effective}\n' \
'https://developers.telnyx.com/docs/api/v2/webhooks'Repository: jamiepine/voicebox
Length of output: 259
Update the Telnyx webhook documentation link
The URL redirects to an unrelated OpenAI chat endpoint. Replace it with the current Telnyx webhook documentation URL.
🤖 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 `@docs/content/docs/overview/sinks.mdx` around lines 158 - 160, Update the
Telnyx webhook documentation link in the sink overview text to the current
official Telnyx webhook documentation URL, keeping the surrounding
signature-verification guidance unchanged.
…etry-safe The sink could not place a call. Telnyx's Dial API requires connection_id alongside to/from, and playback_start takes audio_url (there is no media_url field on that action), so every dial and every playback returned 422. Add a connection_id setting with a migration, correct the playback field, and pass Telnyx's error detail through as a 400 instead of letting raise_for_status surface an opaque 500. Webhooks are delivered at least once, and call.answered could block up to 30s waiting for a generation while holding the request open, so Telnyx would retry and a second playback_start would double the audio. Ack immediately, claim the row before doing slow work, and run the wait in a background task with its own session. Also: resolve profiles through the same single resolve_profile call that /speak uses so per-client bindings and the global default apply to /call too, honor the binding's default engine and personality, compare the webhook token in constant time and fail closed when it is absent, drop the call_session_id fallback that would silently address an uncontrollable call, and thread auto_hangup in at row creation rather than patching it after the dial. Docs gain the two undocumented required fields (connection_id, enabled), a warning that tunneling port 17493 publishes every unauthenticated Voicebox route, and a correction that the Call button is not shipped yet.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/mcp_server/tools.py (1)
253-256: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe docstring omits the global capture default from the precedence chain.
The docstring states "explicit arg → per-client MCP binding → sink default".
resolve_profileinbackend/mcp_server/resolve.pyalso consultsCaptureSettings.default_playback_voice_idbefore returningNone. The real order is explicit arg → per-client binding → global capture default → sink default, sos.default_profile_idis only used when the global default is unset or missing. Update the docstring, or resolve the sink default before the global default if the sink default is meant to win.Also applies to: 284-294
🤖 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 `@backend/mcp_server/tools.py` around lines 253 - 256, Update the profile resolution documentation around the tool’s profile argument to include the global capture default between the per-client MCP binding and sink default. Ensure the documented precedence matches resolve_profile: explicit argument, per-client binding, CaptureSettings.default_playback_voice_id, then s.default_profile_id.
♻️ Duplicate comments (1)
backend/services/sinks.py (1)
289-302: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMap Telnyx transport failures to
ValueErrortoo.
_raise_for_telnyx_statuscovers HTTP 4xx/5xx responses, so a rejected number or badconnection_idnow reaches the route as a client error. A transport failure does not. If Telnyx is unreachable or the 10 s timeout expires,httpx.ConnectErrororhttpx.ReadTimeoutpropagates out ofdial_and_play, andbackend/routes/sinks.pymaps onlyValueErrorto a 400, so the caller gets an opaque 500.🐛 Proposed fix
- async with TelnyxClient(s.api_key) as client: - data = await client.create_call( - to=to, - from_=s.from_number, - connection_id=s.connection_id, - webhook_url=webhook_url, - ) + try: + async with TelnyxClient(s.api_key) as client: + data = await client.create_call( + to=to, + from_=s.from_number, + connection_id=s.connection_id, + webhook_url=webhook_url, + ) + except httpx.HTTPError as exc: + raise ValueError(f"Could not reach Telnyx: {exc}") from exc🤖 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 `@backend/services/sinks.py` around lines 289 - 302, Update dial_and_play to catch Telnyx transport failures, including httpx.ConnectError and httpx.ReadTimeout raised by TelnyxClient.create_call, and re-raise them as ValueError with useful context so backend/routes/sinks.py preserves the client-error response path. Leave the existing call_control_id validation and HTTP-status handling unchanged.
🧹 Nitpick comments (5)
backend/tests/test_telnyx_sink.py (1)
42-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReaching into
client._clientcouples the test to a private attribute.
_client_withbuilds aTelnyxClient, discards itshttpx.AsyncClient, and replaces it. The discarded client is never closed, and any rename of_clientbreaks all six tests at once. Consider an optional injection point onTelnyxClient, for example atransportkeyword forwarded tohttpx.AsyncClient, so the tests use the public constructor.🤖 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 `@backend/tests/test_telnyx_sink.py` around lines 42 - 56, Add an optional transport injection parameter to the TelnyxClient constructor and forward it to httpx.AsyncClient. Update _client_with to construct TelnyxClient with the MockTransport directly, removing the private _client replacement and preventing the unused client from being leaked.backend/services/sinks.py (2)
374-384: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog exceptions raised inside detached tasks.
_run_call_answeredguardsplayback_start, but the settings read, the generation poll, and everydb.commit()run unguarded. If one of them raises, the task dies, the row stays inpreparing, and the only trace is an asyncio "Task exception was never retrieved" message emitted at collection time. Add a done callback that logs the exception so a stuck call is diagnosable.♻️ Proposed refactor
def _spawn(coro) -> None: task = asyncio.create_task(coro) _BACKGROUND_TASKS.add(task) - task.add_done_callback(_BACKGROUND_TASKS.discard) + task.add_done_callback(_on_task_done) + + +def _on_task_done(task: asyncio.Task) -> None: + _BACKGROUND_TASKS.discard(task) + if task.cancelled(): + return + exc = task.exception() + if exc is not None: + logger.error("Telnyx webhook background task failed", exc_info=exc)🤖 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 `@backend/services/sinks.py` around lines 374 - 384, Update _spawn to add a done callback that retrieves and logs any exception from the detached task, while preserving _BACKGROUND_TASKS cleanup. Ensure normal completion and cancellation do not produce error logs, and use the module’s existing logging mechanism.
479-479: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApply Ruff UP017 to both UTC call sites. Import
UTCexplicitly and replace bothtimezone.utcuses withUTC. The project requires Python 3.12 and targetspy312.🤖 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 `@backend/services/sinks.py` at line 479, Update both UTC call sites, including the assignment in the relevant completion flow, to import UTC explicitly and replace timezone.utc with UTC. Remove the no-longer-needed timezone import if applicable, preserving the existing timezone-aware datetime behavior.Source: Linters/SAST tools
backend/mcp_server/tools.py (1)
296-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared binding-defaults lookup.
Lines 296-311 repeat lines 85-99 of
voicebox_speakexactly: loadMCPClientBinding, then deriveresolved_personalityandresolved_engine. A future default (for exampledefault_language) must then be added in two places. Extract one helper, for example_resolve_binding_defaults(client_id, personality, engine, db), and call it from both tools.🤖 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 `@backend/mcp_server/tools.py` around lines 296 - 311, The duplicated binding lookup and default resolution in voicebox_speak and the shown tool should be centralized. Add a shared _resolve_binding_defaults helper that accepts client_id, personality, engine, and db, performs the MCPClientBinding lookup, and returns the resolved defaults; replace both tools’ duplicated blocks with calls to this helper while preserving their existing resolved_personality, use_persona, and resolved_engine behavior.requirements.txt (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck pinning consistency for
httpx.
httpxis now a declared runtime dependency, which is correct becausebackend/services/sinks.pyandbackend/services/cloud.pyimport it directly. The entry carries no version constraint. If the rest of this file pins versions, pinhttpxtoo so builds stay reproducible.#!/bin/bash # Description: Show the requirements manifest to compare pinning style. cat -n requirements.txt🤖 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 `@requirements.txt` at line 10, Update the httpx dependency entry in requirements.txt to follow the file’s established version-pinning convention, using an explicit compatible version constraint consistent with the other runtime dependencies.
🤖 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.
Outside diff comments:
In `@backend/mcp_server/tools.py`:
- Around line 253-256: Update the profile resolution documentation around the
tool’s profile argument to include the global capture default between the
per-client MCP binding and sink default. Ensure the documented precedence
matches resolve_profile: explicit argument, per-client binding,
CaptureSettings.default_playback_voice_id, then s.default_profile_id.
---
Duplicate comments:
In `@backend/services/sinks.py`:
- Around line 289-302: Update dial_and_play to catch Telnyx transport failures,
including httpx.ConnectError and httpx.ReadTimeout raised by
TelnyxClient.create_call, and re-raise them as ValueError with useful context so
backend/routes/sinks.py preserves the client-error response path. Leave the
existing call_control_id validation and HTTP-status handling unchanged.
---
Nitpick comments:
In `@backend/mcp_server/tools.py`:
- Around line 296-311: The duplicated binding lookup and default resolution in
voicebox_speak and the shown tool should be centralized. Add a shared
_resolve_binding_defaults helper that accepts client_id, personality, engine,
and db, performs the MCPClientBinding lookup, and returns the resolved defaults;
replace both tools’ duplicated blocks with calls to this helper while preserving
their existing resolved_personality, use_persona, and resolved_engine behavior.
In `@backend/services/sinks.py`:
- Around line 374-384: Update _spawn to add a done callback that retrieves and
logs any exception from the detached task, while preserving _BACKGROUND_TASKS
cleanup. Ensure normal completion and cancellation do not produce error logs,
and use the module’s existing logging mechanism.
- Line 479: Update both UTC call sites, including the assignment in the relevant
completion flow, to import UTC explicitly and replace timezone.utc with UTC.
Remove the no-longer-needed timezone import if applicable, preserving the
existing timezone-aware datetime behavior.
In `@backend/tests/test_telnyx_sink.py`:
- Around line 42-56: Add an optional transport injection parameter to the
TelnyxClient constructor and forward it to httpx.AsyncClient. Update
_client_with to construct TelnyxClient with the MockTransport directly, removing
the private _client replacement and preventing the unused client from being
leaked.
In `@requirements.txt`:
- Line 10: Update the httpx dependency entry in requirements.txt to follow the
file’s established version-pinning convention, using an explicit compatible
version constraint consistent with the other runtime dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 576306a3-d5ed-4091-a451-47b96e8ad27d
📒 Files selected for processing (9)
backend/database/migrations.pybackend/database/models.pybackend/mcp_server/tools.pybackend/models.pybackend/routes/sinks.pybackend/services/sinks.pybackend/tests/test_telnyx_sink.pydocs/content/docs/overview/sinks.mdxrequirements.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/models.py
- backend/routes/sinks.py
- backend/database/models.py
|
Pushed two commits ( Two wire-contract bugs
My earlier "tested end to end, playback clean" note in the description was wrong and I've corrected it. Both paths are now covered by tests that assert the exact JSON sent to Telnyx, and I verified those tests fail if either fix is reverted. Live PSTN re-verification is still pending on my side. Webhook retry safety
The handler now acks immediately, claims the call row before doing anything slow, and runs the wait in a background task with its own session. Retried deliveries see the claim and exit. Smaller fixes
Answers to my own open questions1. Webhook signature verification: I'd take it as a follow-up, but I'll do it here if you'd rather. The per-call token scopes each webhook to one call and fails closed, which is a real gate, but it rides in the query string and so lands in the access logs of whatever proxy fronts the tunnel. Telnyx's Ed25519 signature ( 2. Sink vs. pipeline routing: I'd merge additive now. The Telnyx-specific surface is the HTTP client and the two webhook events, and that logic survives a reframing into a sink node unchanged. The settings table is isolated and the routes are namespaced under 3. Frontend "Call" button: follow-up, and the docs now say so. The original docs described a button that didn't exist, which was my error; that section is now marked as not yet shipped. Backend surface is complete and usable via MCP and curl today. Happy to add the UI here if you'd prefer to review it as one unit. One thing worth your opinionTelnyx has to reach this server, but Voicebox binds to loopback and has no request auth, so a plain That works, but it puts the burden on the user and it's easy to skip. If sinks become a recurring pattern, Voicebox might want a first-class "expose only these paths" mode. That's beyond this PR, and I didn't want to invent that surface unilaterally, but the need shows up the moment any sink needs an inbound callback. |
1 participant
Add Telnyx PSTN sink: call any phone and play generations on the line
What this adds
Voicebox today runs the full voice I/O loop locally (clone, generate, dictate,
agent speech) but stops at the speaker. There's no way to take a generation and
play it on a real phone call. The roadmap lists "Pipeline routing … webhook +
MCP sinks" and "Platform sinks: Apple Notes, Obsidian, and other opt-in
integrations."
This PR ships the first reference sink, Telnyx, and the convention future sinks
follow. Generation stays local; only the PSTN leg touches the carrier.
What's in the box
voicebox.callMCP tool: dial a number and play a generation on the call.Mirrors
voicebox.speak's shape (profile resolution, personality rewrite,engine override).
POST /call: REST wrapper, same pattern asPOST /speak.POST /sinks/telnyx/webhook: receives Telnyxcall.answeredandplayback.ended. Per-call webhook secret stops third-party triggering.GET/PUT /sinks/telnyx: settings CRUD (api_key masked in responses,plus a
missingarray naming any unset required field).GET /sinks/telnyx/health: verify the API key against Telnyx.TelnyxSettingsandTelnyxCallsSQLite tables: created idempotently onfirst boot via the existing migration pattern.
sinks.mdxpage in the overview.backend/tests/test_telnyx_sink.py, 17 cases.Flow
The caller does not block on the callee picking up. The MCP/REST call returns as
soon as Telnyx accepts the dial; playback happens asynchronously via the webhook.
Telnyx wants a 2xx within 2000 ms and retries otherwise, and it queues
consecutive play commands, so the webhook acknowledges before doing any slow
work and claims the call row so a retried delivery can't start a second
playback on the same call.
HTTP-only, no SDK
Uses
httpxagainstapi.telnyx.com/v2. Three endpoints:POST /calls,POST /calls/{id}/actions/playback_start,POST /calls/{id}/actions/hangup.No
telnyxpackage, no PyInstaller bundling changes.httpxwas already imported at module scope byservices/cloud.pybut wasnever declared, so this PR adds it to
requirements.txt.Config
(
connection_id), from-number,public_base_url(ngrok/Funnel URL), defaultvoice profile, auto-hangup toggle, and an
enabledmaster switch.sinks will need.
Security note on
public_base_urlTelnyx has to reach this server, but Voicebox binds to 127.0.0.1 and has no
request authentication. A bare
ngrok http 17493therefore publishes everyroute, including the profile, generation, and settings APIs. The docs call this
out and tell users to front the tunnel with a proxy that allows only
POST /sinks/telnyx/webhookandGET /audio/.Worth a maintainer opinion: if sinks become a common pattern, Voicebox may want
a first-class "expose these paths only" mode rather than leaving each user to
build it.
What this PR does NOT do
playback_startwith an audio URL.No SIPREC, no bidirectional audio.
"Call" button on the generation card is a small follow-up PR, and the docs
say so rather than describing a button that isn't there.
Files
backend/database/models.py:TelnyxSettings+TelnyxCallORMbackend/database/__init__.py: re-exportsbackend/database/migrations.py:_migrate_telnyx_settings,_migrate_telnyx_calls(idempotent CREATE TABLE, plus aconnection_idbackfill for anyone who ran an earlier build of this branch)
backend/models.py:CallRequest,CallResponse,TelnyxSettingsResponse,TelnyxSettingsUpdatePydantic modelsbackend/services/sinks.py:TelnyxClient(thin httpx wrapper) +dial_and_play,handle_telnyx_webhook,place_callorchestrationbackend/routes/sinks.py:POST /call, settings CRUD, webhook receiverbackend/routes/__init__.py: register the new routerbackend/mcp_server/tools.py:voicebox.calltool, next tovoicebox.speakbackend/tests/test_telnyx_sink.py: wire contract, config gate, webhook gaterequirements.txt: declarehttpxdocs/content/docs/overview/sinks.mdx: setup + usage guidedocs/content/docs/overview/meta.json: add the new pageWhy Telnyx
Setup
Control Application, and assign the number to it. Copy the application ID.
behind a proxy restricted to the two paths above. Copy the HTTPS URL.
from-number, the public URL, pick a default voice profile, turn on
enabled.Verification
backend/tests/test_telnyx_sink.pypasses (17 cases). Covers the requestbodies sent to Telnyx, the config gate, constant-time token comparison, and
single-fire playback under retried webhook delivery.
POST /callreturns 400 naming the missing fields; theMCP tool raises with the same message.
An earlier revision of this branch sent no
connection_idand used amedia_urlfield that doesn't exist onplayback_start, so both the dialand the playback would have been rejected by the API. That's fixed here and
covered by tests, but I want to re-run it end to end on hardware before
anyone treats the call path as confirmed working.
Open questions for maintainers
webhook_secretquery param, compared in constant time, failing closed whenabsent. Telnyx also signs every event with
telnyx-signature-ed25519andtelnyx-timestampover{timestamp}|{raw body}, verifiable against theaccount public key. That authenticates the sender rather than just scoping
the URL, and it's the better gate for an internet-exposed endpoint, but it
needs an Ed25519 dependency and a public-key setting. Want it in this PR, or
as a follow-up?
settings table, additive, not blocking on the broader pipeline-routing
refactor. When that lands, the Telnyx sink can be reframed as a sink node
and the HTTP + webhook logic carries over. Merge the additive version now
and migrate later, or hold until the sink interface is settled?
left for a follow-up so this PR stays reviewable, and the docs mark it as
not yet shipped. Acceptable, or should the UI ship here?
License
MIT, no CLA friction. The contributor is a Telnyx employee but is contributing
as an individual user of the project. No Telnyx affiliation claim beyond the
choice of carrier.
Summary by CodeRabbit
New Features
Documentation