Conversation
📝 WalkthroughWalkthroughAdds a Telnyx telephony provider: REST call control, webhook handlers, outbound/inbound flows, a WebSocket-based audio streamer, vault credential handling, unit tests, and factory wiring to instantiate Telnyx components. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Gin API
participant Vault
participant TelnyxAPI as Telnyx REST
participant WS as Telnyx WebSocket
Client->>API: OutboundCall(to, from, vaultCred, ...)
API->>Vault: fetch(api_key, connection_id)
Vault-->>API: credentials
API->>TelnyxAPI: POST /v2/calls (to, from, connection_id, stream_url)
TelnyxAPI-->>API: {data: {call_control_id,...}}
API-->>Client: CallInfo (provider: telnyx, ChannelUUID, Extra)
Note over WS,TelnyxAPI: Media stream established via stream_url (WebSocket)
sequenceDiagram
participant TelnyxWS as Telnyx WebSocket
participant Streamer
participant AudioProc as Audio Processor
participant Assistant
TelnyxWS->>Streamer: "start" (stream_id, call_control_id)
Streamer->>Streamer: set ChannelUUID, emit stream_started
TelnyxWS->>Streamer: "media" (base64 payload)
Streamer->>AudioProc: decode base64 -> raw audio
AudioProc-->>Streamer: buffered audio
Streamer->>Assistant: CreateVoiceRequest when threshold met
Assistant-->>Streamer: response audio (Linear16 16kHz)
Streamer->>AudioProc: resample -> PCMU 8kHz, chunk
Streamer->>TelnyxWS: send "media" frames (base64 PCMU)
TelnyxWS->>Streamer: "stop" or disconnect
Streamer->>Streamer: emit user disconnected, cancel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go (2)
367-367: Non-idiomatic Go method nameget_credentials.The test calls
telephony.get_credentials(vaultCred)which uses snake_case. Go conventions prefer camelCase for unexported methods (e.g.,getCredentials). This should be addressed in the source file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go` at line 367, The test is calling a non-idiomatic snake_case method name get_credentials; rename the unexported method to camelCase (getCredentials) in the telnyx telephony implementation and update all call sites (including the test call telephony.get_credentials(vaultCred)) to telephony.getCredentials(vaultCred); ensure the function declaration (get_credentials) in the telnyx type and any references (method receiver names, imports) are updated consistently and run gofmt/go vet to verify styling.
322-390: Test coverage is good but consider adding OutboundCall/HangupCall tests.The test suite covers the core webhook handlers, credential extraction, and WebSocket event parsing well. However,
OutboundCallandHangupCallare not tested. Consider adding unit tests with HTTP mocking (usinghttptest.Server) to verify:
- Successful API call construction and response parsing
- Error handling for 4xx/5xx responses
- Missing/invalid credentials handling
As per coding guidelines: "Include at least a success path, fallback/error path, and factory/selection behavior assertion in backend tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go` around lines 322 - 390, Add unit tests for Telnyx outbound and hangup flows: create tests that instantiate NewTelnyxTelephony (use get_credentials with valid and invalid protos to simulate credential presence/absence), then exercise OutboundCall and HangupCall against an httptest.Server that mocks Telnyx HTTP responses; include at least a success path (200/2xx returns parsed correctly), an error path (4xx/5xx returns and cause proper error handling), and a credentials-missing path (ensure calls fail when get_credentials returns error). Use unique names OutboundCall and HangupCall to locate methods and validate both request construction (headers/body) and response handling.api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go (2)
137-141: Ignored error fromhandleMediaEventmay hide decoding failures.The error returned by
handleMediaEventis discarded. While the current implementation only returnsnilerrors, this pattern could silently swallow future errors.♻️ Suggested fix
case "media": - msg, _ := tws.handleMediaEvent(event) + msg, err := tws.handleMediaEvent(event) + if err != nil { + tws.Logger.Warnf("Failed to handle media event: %v", err) + } if msg != nil { tws.PushInput(msg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go` around lines 137 - 141, The code currently discards the error from handleMediaEvent in the media case; update the case to capture the returned error (from tws.handleMediaEvent) and handle it instead of ignoring it—e.g., check if err != nil and log or return it, while only calling tws.PushInput(msg) when msg != nil and err == nil; reference the media case and the tws.handleMediaEvent and tws.PushInput calls to locate where to add the error check and appropriate logging/handling.
199-202: Silent nil return when connection is nil may hide issues.
Sendreturnsnilwhenconnectionisnil, which silently discards all outgoing messages. This could make debugging difficult if the connection is unexpectedly closed. Consider logging or returning an error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go` around lines 199 - 202, The Send method on telnyxWebsocketStreamer currently returns nil when tws.connection is nil, silently discarding messages; update Send (telnyxWebsocketStreamer.Send) to surface this situation by returning a descriptive error (or at minimum logging the condition with the connection state) instead of nil, so callers can detect/handle a closed/missing connection; ensure the error message references the telnyxWebsocketStreamer/connection so it’s clear in logs and test behavior.api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go (1)
98-111: Silent error handling when reading request body in fallback path.On line 100, the error from
c.GetRawData()is silently discarded. If reading the body fails, the code continues with an empty/partial payload, which could lead to confusing "missing caller number" errors.♻️ Suggested fix
if clientNumber == "" { // Try from request body - body, _ := c.GetRawData() + body, err := c.GetRawData() + if err != nil { + tpc.logger.Debugf("Failed to read request body for caller extraction: %v", err) + } var payload map[string]interface{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go` around lines 98 - 111, The fallback branch that tries to extract clientNumber from the request body currently discards the error from c.GetRawData(); change it to capture the error (err := c.GetRawData()), and if err != nil stop the fallback (log or return a 400/appropriate error response) instead of continuing to json.Unmarshal an empty body. Update the block around clientNumber, c.GetRawData(), and json.Unmarshal so any read error is handled (e.g., return early with a clear error log/response) before attempting to parse payload["data"].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go`:
- Around line 314-328: The method get_credentials on telnyxTelephony should be
renamed to idiomatic camelCase (getCredentials) and you must guard against a nil
vaultCredential to avoid a panic: add an immediate nil check for the
vaultCredential parameter and return a descriptive error if nil, then call
vaultCredential.GetValue().AsMap(); keep the existing checks for "api_key" and
"connection_id" and return them as strings, and update all callers to use
getCredentials (or adjust any interface implementations) so references remain
consistent.
- Around line 346-350: The HangupCall function creates an http.Client without a
timeout which can hang; change the client creation in HangupCall (where client
:= &http.Client{} and client.Do(req) is called) to use a client with a sensible
Timeout (e.g., &http.Client{Timeout: <duration>}) or reuse a shared client
configured with timeouts, and ensure the new client is used for the Do(req) call
so the request cannot block indefinitely (mirror the fix applied for
OutboundCall).
- Around line 54-76: The error returns for reading and unmarshaling the request
body in the Telnyx handler do not preserve the original error chain; change the
two returns that currently do `fmt.Errorf("failed to read request body")` and
`fmt.Errorf("failed to parse request body")` to wrap the original `err` using
the `%w` verb (e.g., `fmt.Errorf("failed to read request body: %w", err)`) and
likewise for the JSON unmarshal case, while keeping the existing
`tpc.logger.Errorf(...)` calls that log the error details; leave the
type-assertion/field-missing branches as-is since there is no underlying error
to wrap.
- Around line 189-196: The HTTP client is created without a timeout (client :=
&http.Client{}), which can hang indefinitely; update the code that performs the
request (the block creating client and calling client.Do(req)) to use a client
with a sensible timeout (e.g. &http.Client{Timeout: 10 * time.Second}) and add
the necessary import for time, or alternatively ensure the request uses a
context with deadline (req = req.WithContext(ctx)) so the call cannot block
forever; reference the client variable and the client.Do(req) call when making
the change.
- Around line 270-305: The InboundCall function currently extracts callControlID
but never uses it; either remove the unused extraction block (the code that
reads callControlID from c.Query and the JSON body) or wire it into the
call/session flow: for example add callControlID to the response params returned
to Telnyx (include "call_control_id": callControlID in the gin.H params), and/or
log/store it via your telephony/session manager (use tpc's logger or save to the
session associated with internal_type.GetContextAnswerPath("telnyx", ctxID)).
Ensure the chosen change touches the InboundCall method and the callControlID
symbol so there is no dead code left.
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go`:
- Around line 143-147: In the "stop" case handling (case "stop") the code calls
tws.Cancel() before tws.PushDisconnection(...), which can cancel the streamer
context and close the connection before the disconnection event is sent; change
the order so
tws.PushDisconnection(protos.ConversationDisconnection_DISCONNECTION_TYPE_USER)
is invoked before tws.Cancel(), keeping tws.Logger.Info("Telnyx stream stopped")
and the return intact so the disconnection is reliably pushed while the streamer
context is still active.
- Around line 336-343: telnyxWebsocketStreamer has a race on the connection
field: runWebSocketReader, Send, sendMedia, sendClear, sendDTMF and Cancel
access/modify tws.connection without synchronization; add a mutex (e.g.,
sync.RWMutex or sync.Mutex) to the telnyxWebsocketStreamer struct, protect all
reads of tws.connection with RLock/RUnlock (or Lock/Unlock for simple mutex) and
all writes/close/nil assignments (in Cancel and send* methods) with Lock/Unlock,
and ensure runWebSocketReader uses the same lock when accessing connection so no
goroutine can close or nil it concurrently.
- Around line 350-395: Remove the unused exported function
ProcessIncomingCallWebhook from websocket.go because it duplicates the
telnyxTelephony.InboundCall implementation; update any intended external
exposure by either wiring that behavior through the existing
telnyxTelephony.InboundCall (used by HandleReceiveCall) or document a new
wrapper if truly needed—delete ProcessIncomingCallWebhook and ensure all call
handling relies on telnyxTelephony.InboundCall (and adjust imports/exports if
necessary).
---
Nitpick comments:
In
`@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go`:
- Line 367: The test is calling a non-idiomatic snake_case method name
get_credentials; rename the unexported method to camelCase (getCredentials) in
the telnyx telephony implementation and update all call sites (including the
test call telephony.get_credentials(vaultCred)) to
telephony.getCredentials(vaultCred); ensure the function declaration
(get_credentials) in the telnyx type and any references (method receiver names,
imports) are updated consistently and run gofmt/go vet to verify styling.
- Around line 322-390: Add unit tests for Telnyx outbound and hangup flows:
create tests that instantiate NewTelnyxTelephony (use get_credentials with valid
and invalid protos to simulate credential presence/absence), then exercise
OutboundCall and HangupCall against an httptest.Server that mocks Telnyx HTTP
responses; include at least a success path (200/2xx returns parsed correctly),
an error path (4xx/5xx returns and cause proper error handling), and a
credentials-missing path (ensure calls fail when get_credentials returns error).
Use unique names OutboundCall and HangupCall to locate methods and validate both
request construction (headers/body) and response handling.
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go`:
- Around line 98-111: The fallback branch that tries to extract clientNumber
from the request body currently discards the error from c.GetRawData(); change
it to capture the error (err := c.GetRawData()), and if err != nil stop the
fallback (log or return a 400/appropriate error response) instead of continuing
to json.Unmarshal an empty body. Update the block around clientNumber,
c.GetRawData(), and json.Unmarshal so any read error is handled (e.g., return
early with a clear error log/response) before attempting to parse
payload["data"].
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go`:
- Around line 137-141: The code currently discards the error from
handleMediaEvent in the media case; update the case to capture the returned
error (from tws.handleMediaEvent) and handle it instead of ignoring it—e.g.,
check if err != nil and log or return it, while only calling tws.PushInput(msg)
when msg != nil and err == nil; reference the media case and the
tws.handleMediaEvent and tws.PushInput calls to locate where to add the error
check and appropriate logging/handling.
- Around line 199-202: The Send method on telnyxWebsocketStreamer currently
returns nil when tws.connection is nil, silently discarding messages; update
Send (telnyxWebsocketStreamer.Send) to surface this situation by returning a
descriptive error (or at minimum logging the condition with the connection
state) instead of nil, so callers can detect/handle a closed/missing connection;
ensure the error message references the telnyxWebsocketStreamer/connection so
it’s clear in logs and test behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1c5a3c5-d1e0-49e8-95a1-d59f9c401555
📒 Files selected for processing (4)
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.goapi/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.goapi/assistant-api/internal/channel/telephony/internal/telnyx/websocket.goapi/assistant-api/internal/channel/telephony/telephony.go
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go
Show resolved
Hide resolved
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go
Outdated
Show resolved
Hide resolved
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go
Show resolved
Hide resolved
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go
Outdated
Show resolved
Hide resolved
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go
Outdated
Show resolved
Hide resolved
api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go
Show resolved
Hide resolved
api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go
Show resolved
Hide resolved
api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds Telnyx as a new telephony provider in assistant-api, including Call Control API integration and WebSocket-based bidirectional audio streaming, wired into the existing telephony factory/streamer patterns (Twilio/Vonage/etc.).
Changes:
- Add Telnyx provider to telephony provider enum +
GetTelephony()/NewStreamer()factories. - Implement Telnyx telephony provider (inbound/outbound, status callbacks, hangup).
- Implement Telnyx WebSocket streamer + add initial unit tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| api/assistant-api/internal/channel/telephony/telephony.go | Registers Telnyx in provider + streamer factories. |
| api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go | Implements Telnyx telephony provider (webhooks, outbound call creation, inbound response, hangup). |
| api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go | Implements Telnyx WS protocol handling and audio send/receive pipeline. |
| api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go | Adds unit tests for Telnyx telephony + WS event parsing. |
| // ProcessIncomingCallWebhook handles an incoming call webhook from Telnyx. | ||
| // This is called by the inbound call handler to start streaming. | ||
| func ProcessIncomingCallWebhook(c *gin.Context, appCfg *config.AssistantConfig, logger commons.Logger) error { | ||
| // Parse the webhook payload |
There was a problem hiding this comment.
ProcessIncomingCallWebhook references config.AssistantConfig, but this file does not import the github.com/rapidaai/api/assistant-api/config package, so it won’t compile. Add the missing import (or move/remove this helper if it’s not intended to be used).
| if clientNumber == "" { | ||
| // Try from request body | ||
| body, _ := c.GetRawData() | ||
| var payload map[string]interface{} | ||
| if err := json.Unmarshal(body, &payload); err == nil { | ||
| if data, ok := payload["data"].(map[string]interface{}); ok { | ||
| if payloadData, ok := data["payload"].(map[string]interface{}); ok { | ||
| if from, ok := payloadData["from"].(string); ok { | ||
| clientNumber = from | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ReceiveCall may call c.GetRawData() to parse JSON, but the inbound dispatcher calls ReceiveCall and then InboundCall with the same *gin.Context. GetRawData() consumes Request.Body, so any later attempt to read the body (e.g., in InboundCall) will see an empty payload. Consider reading the body once and restoring it, or avoid reading the body in InboundCall altogether.
| // Try from request body | ||
| body, _ := c.GetRawData() | ||
| var payload map[string]interface{} | ||
| if err := json.Unmarshal(body, &payload); err == nil { | ||
| if data, ok := payload["data"].(map[string]interface{}); ok { | ||
| if payloadData, ok := data["payload"].(map[string]interface{}); ok { | ||
| if from, ok := payloadData["from"].(string); ok { | ||
| clientNumber = from | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if clientNumber == "" { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Missing caller number"}) | ||
| return nil, fmt.Errorf("missing or empty 'from' query parameter") | ||
| } | ||
|
|
||
| info := &internal_type.CallInfo{ | ||
| CallerNumber: clientNumber, | ||
| Provider: telnyxProvider, | ||
| Status: "SUCCESS", | ||
| StatusInfo: internal_type.StatusInfo{Event: "webhook", Payload: queryParams}, | ||
| Extra: make(map[string]string), | ||
| } | ||
|
|
||
| // Extract call_control_id if present | ||
| if v, ok := queryParams["call_control_id"]; ok && v != "" { | ||
| info.ChannelUUID = v | ||
| info.Extra["call_control_id"] = v | ||
| } |
There was a problem hiding this comment.
When the caller number is parsed from the webhook JSON body, call_control_id from that same payload is not extracted into CallInfo.ChannelUUID / Extra. This means the dispatcher will persist an empty ChannelUUID for most Telnyx inbound calls (unless call_control_id is also provided as a query param). Extract call_control_id from the JSON payload and populate CallInfo consistently.
| // Get call_control_id from the request (Telnyx sends it in the webhook) | ||
| callControlID := c.Query("call_control_id") | ||
| if callControlID == "" { | ||
| // Try from request body | ||
| body, _ := c.GetRawData() | ||
| var payload map[string]interface{} | ||
| if err := json.Unmarshal(body, &payload); err == nil { | ||
| if data, ok := payload["data"].(map[string]interface{}); ok { | ||
| if payloadData, ok := data["payload"].(map[string]interface{}); ok { | ||
| if ccid, ok := payloadData["call_control_id"].(string); ok { | ||
| callControlID = ccid | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
InboundCall parses call_control_id from query/body but the value is never used afterwards. Besides being dead code, the extra GetRawData() call can interact badly with earlier body reads in ReceiveCall. Either use callControlID (e.g., to populate metadata) or remove this parsing to keep the handler side-effect free.
| // Send the request | ||
| client := &http.Client{} | ||
| resp, err := client.Do(req) | ||
| if err != nil { |
There was a problem hiding this comment.
OutboundCall uses client := &http.Client{} without a timeout. If Telnyx is slow/unreachable, this can hang the request indefinitely and tie up goroutines. Use a client with a reasonable timeout (or use a request context with deadline) similar to the Exotel implementation.
| req.Header.Set("Authorization", "Bearer "+apiKey) | ||
|
|
||
| client := &http.Client{} | ||
| resp, err := client.Do(req) |
There was a problem hiding this comment.
HangupCall also creates an http.Client with no timeout, which can hang shutdown/end-conversation flows if Telnyx doesn’t respond. Set a timeout or use a context-aware request so call teardown is bounded.
| // get_credentials extracts API key and connection ID from vault credential. | ||
| func (tpc *telnyxTelephony) get_credentials(vaultCredential *protos.VaultCredential) (string, string, error) { | ||
| credMap := vaultCredential.GetValue().AsMap() | ||
|
|
||
| apiKey, ok := credMap["api_key"] | ||
| if !ok { | ||
| return "", "", fmt.Errorf("api_key not found in vault credential") | ||
| } | ||
|
|
||
| connectionID, ok := credMap["connection_id"] | ||
| if !ok { | ||
| return "", "", fmt.Errorf("connection_id not found in vault credential") | ||
| } | ||
|
|
||
| return fmt.Sprintf("%v", apiKey), fmt.Sprintf("%v", connectionID), nil | ||
| } |
There was a problem hiding this comment.
Method name get_credentials uses underscores, which is inconsistent with Go naming (camelCase) and with existing telephony implementations (e.g., clientParam, Auth). Rename to getCredentials (and update call sites/tests) to match the codebase’s Go style.
| func TestGetCredentials(t *testing.T) { | ||
| cfg := &config.AssistantConfig{} | ||
| logger := commons.NewLogger("test") | ||
| telephony, _ := NewTelnyxTelephony(cfg, logger) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| credMap map[string]interface{} | ||
| expectErr bool | ||
| expectAPI string | ||
| expectConn string | ||
| }{ | ||
| { | ||
| name: "valid credentials", | ||
| credMap: map[string]interface{}{ | ||
| "api_key": "test-api-key", | ||
| "connection_id": "test-connection-id", | ||
| }, | ||
| expectErr: false, | ||
| expectAPI: "test-api-key", | ||
| expectConn: "test-connection-id", | ||
| }, | ||
| { | ||
| name: "missing api_key", | ||
| credMap: map[string]interface{}{ | ||
| "connection_id": "test-connection-id", | ||
| }, | ||
| expectErr: true, | ||
| }, | ||
| { | ||
| name: "missing connection_id", | ||
| credMap: map[string]interface{}{ | ||
| "api_key": "test-api-key", | ||
| }, | ||
| expectErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| structValue, _ := structpb.NewStruct(tt.credMap) | ||
| vaultCred := &protos.VaultCredential{ | ||
| Value: structValue, | ||
| } | ||
|
|
||
| apiKey, connID, err := telephony.get_credentials(vaultCred) | ||
|
|
There was a problem hiding this comment.
NewTelnyxTelephony returns an internal_type.Telephony interface, but this test later calls telephony.get_credentials(...), which won’t compile because get_credentials isn’t part of the interface. Either type-assert to *telnyxTelephony (and handle failure) or instantiate &telnyxTelephony{...} directly for testing the helper.
| func TestNewTelnyxTelephony(t *testing.T) { | ||
| cfg := &config.AssistantConfig{ | ||
| PublicAssistantHost: "test.example.com", | ||
| } | ||
| logger := commons.NewLogger("test") | ||
|
|
||
| telephony, err := NewTelnyxTelephony(cfg, logger) | ||
|
|
There was a problem hiding this comment.
Tests use commons.NewLogger("test"), but pkg/commons doesn’t define NewLogger (it exposes NewApplicationLogger instead). This will fail to compile; switch these test initializations to commons.NewApplicationLogger(...) (handling the returned error) or use the existing test helper pattern used in other telephony tests.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go (1)
302-307:⚠️ Potential issue | 🟠 MajorPotential nil pointer dereference when
GetValue()returns nil.The code checks if
vaultCredentialis nil but doesn't check ifvaultCredential.GetValue()returns nil. Per the proto definition (seeprotos/vault-api.pb.go:87-92),GetValue()returns nil if theValuefield is unset. Calling.AsMap()on nil will panic.🔧 Suggested fix
func (tpc *telnyxTelephony) getCredentials(vaultCredential *protos.VaultCredential) (string, string, error) { - if vaultCredential == nil { - return "", "", fmt.Errorf("vault credential is nil") + if vaultCredential == nil || vaultCredential.GetValue() == nil { + return "", "", fmt.Errorf("vault credential is nil or empty") } credMap := vaultCredential.GetValue().AsMap()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go` around lines 302 - 307, In telnyxTelephony.getCredentials, guard against a nil Value by checking vaultCredential.GetValue() before calling AsMap(); if GetValue() is nil return a descriptive error (e.g., "vault credential value is nil") rather than calling vaultCredential.GetValue().AsMap(); update the error handling path so callers receive the error and no nil-pointer panic occurs.
🧹 Nitpick comments (1)
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go (1)
448-493: Test creates HTTP server but doesn't verify actual HTTP behavior.The test sets up
httptest.Serverwith response handling but only verifiesgetCredentials. The server URL is never used to make actual outbound calls. Consider either:
- Inject the base URL to enable full HTTP flow testing
- Remove the unused server setup to clarify test scope
The same pattern applies to
TestHangupCall(lines 536-570).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go` around lines 448 - 493, The test sets up an httptest.Server but never uses its URL, so it only verifies getCredentials and not HTTP interactions; either remove the unused server setup or modify NewTelnyxTelephony (or provide a constructor/test hook) to accept a baseURL/HTTP client so the test can point telephony's outgoing requests at server.URL and assert request/response behavior — update tests TestHangupCall similarly; locate NewTelnyxTelephony and getCredentials in the telnyx telephony code and add a configurable baseURL or injectable http.Client, then change the test to pass server.URL (or the injected client) and assert the handler is hit and responses are handled as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go`:
- Around line 502-511: The test declares an unused opts variable which causes
lint failures and means TestOutboundCall_MissingCredentials isn't using options;
either remove the unused declaration or construct and pass opts into
telephony.OutboundCall to exercise the options code path. Locate the opts
declaration in TestOutboundCall_MissingCredentials and either delete it or
create a meaningful internal_type.TestOption value and replace the trailing nil
(one of the last two arguments) in the telephony.OutboundCall(nil,
"+15551234567", "+15559876543", 1, 1, nil, nil) call with opts so the test uses
the options parameter and still asserts the expected error/status from
telephony.OutboundCall.
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go`:
- Around line 326-328: sendClear and sendDTMF use an RLock-then-Lock pattern
that can race and cause nil deref; instead, under the read lock cache the
connection into a local variable (e.g., conn := tws.connection), release the
read lock, then acquire the write lock, check that conn != nil before writing,
and use the cached conn to call WriteMessage (and return an error if nil).
Update both sendClear and sendDTMF to follow this pattern and avoid directly
accessing tws.connection after upgrading locks.
- Around line 279-304: In sendMedia, avoid dereferencing tws.connection after
releasing the RLock: cache the connection in conn (already done) and use
conn.WriteMessage instead of tws.connection.WriteMessage; ensure you check conn
!= nil (and tws.streamID != "") before marshalling and then call
conn.WriteMessage with the cached variable so Cancel() cannot set tws.connection
= nil between locks and cause a nil deref (symbols: sendMedia, tws.connection,
conn, tws.mu, WriteMessage).
- Around line 167-178: Protect updates to streamID, callControlID, and
ChannelUUID in handleStartEvent by acquiring the streamer's mutex before writing
and releasing it after (e.g., tws.mu.Lock()/Unlock()); likewise ensure read
access in sendMedia, sendClear, and sendDTMF uses the same mutex (e.g.,
tws.mu.RLock()/RUnlock()) so there is no race between handleStartEvent and those
methods; apply the lock around the assignments in
telnyxWebsocketStreamer.handleStartEvent and add corresponding read locks in
sendMedia, sendClear, and sendDTMF.
---
Duplicate comments:
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go`:
- Around line 302-307: In telnyxTelephony.getCredentials, guard against a nil
Value by checking vaultCredential.GetValue() before calling AsMap(); if
GetValue() is nil return a descriptive error (e.g., "vault credential value is
nil") rather than calling vaultCredential.GetValue().AsMap(); update the error
handling path so callers receive the error and no nil-pointer panic occurs.
---
Nitpick comments:
In
`@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go`:
- Around line 448-493: The test sets up an httptest.Server but never uses its
URL, so it only verifies getCredentials and not HTTP interactions; either remove
the unused server setup or modify NewTelnyxTelephony (or provide a
constructor/test hook) to accept a baseURL/HTTP client so the test can point
telephony's outgoing requests at server.URL and assert request/response behavior
— update tests TestHangupCall similarly; locate NewTelnyxTelephony and
getCredentials in the telnyx telephony code and add a configurable baseURL or
injectable http.Client, then change the test to pass server.URL (or the injected
client) and assert the handler is hit and responses are handled as expected.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4faec2f-6a73-48bd-a082-2c06ae3ffd84
📒 Files selected for processing (3)
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.goapi/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.goapi/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go
| opts := &internal_type.TestOption{} | ||
|
|
||
| info, err := telephony.OutboundCall(nil, "+15551234567", "+15559876543", 1, 1, nil, nil) | ||
| if err == nil { | ||
| t.Error("expected error for nil vault credential") | ||
| } | ||
| if info.Status != "FAILED" { | ||
| t.Errorf("expected FAILED status, got %s", info.Status) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused variable opts will cause lint/vet failures.
The variable opts is declared but never used. This also indicates that TestOutboundCall_MissingCredentials isn't exercising the full code path since options aren't being passed.
🔧 Suggested fix
func TestOutboundCall_MissingCredentials(t *testing.T) {
cfg := &config.AssistantConfig{
PublicAssistantHost: "test.example.com",
}
logger := commons.NewLogger("test")
telephony, _ := NewTelnyxTelephony(cfg, logger)
- opts := &internal_type.TestOption{}
-
info, err := telephony.OutboundCall(nil, "+15551234567", "+15559876543", 1, 1, nil, nil)📝 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.
| opts := &internal_type.TestOption{} | |
| info, err := telephony.OutboundCall(nil, "+15551234567", "+15559876543", 1, 1, nil, nil) | |
| if err == nil { | |
| t.Error("expected error for nil vault credential") | |
| } | |
| if info.Status != "FAILED" { | |
| t.Errorf("expected FAILED status, got %s", info.Status) | |
| } | |
| } | |
| info, err := telephony.OutboundCall(nil, "+15551234567", "+15559876543", 1, 1, nil, nil) | |
| if err == nil { | |
| t.Error("expected error for nil vault credential") | |
| } | |
| if info.Status != "FAILED" { | |
| t.Errorf("expected FAILED status, got %s", info.Status) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go`
around lines 502 - 511, The test declares an unused opts variable which causes
lint failures and means TestOutboundCall_MissingCredentials isn't using options;
either remove the unused declaration or construct and pass opts into
telephony.OutboundCall to exercise the options code path. Locate the opts
declaration in TestOutboundCall_MissingCredentials and either delete it or
create a meaningful internal_type.TestOption value and replace the trailing nil
(one of the last two arguments) in the telephony.OutboundCall(nil,
"+15551234567", "+15559876543", 1, 1, nil, nil) call with opts so the test uses
the options parameter and still asserts the expected error/status from
telephony.OutboundCall.
| func (tws *telnyxWebsocketStreamer) handleStartEvent(event TelnyxWebSocketEvent) { | ||
| if event.Start != nil { | ||
| tws.streamID = event.StreamID | ||
| tws.callControlID = event.Start.CallControlID | ||
| tws.ChannelUUID = event.Start.CallControlID | ||
|
|
||
| tws.Logger.Debugf("Telnyx stream started | stream_id: %s, call_control_id: %s, format: %s %dHz", | ||
| tws.streamID, tws.callControlID, | ||
| event.Start.MediaFormat.Encoding, | ||
| event.Start.MediaFormat.SampleRate) | ||
| } | ||
| } |
There was a problem hiding this comment.
Data race on streamID, callControlID, and ChannelUUID fields.
These fields are modified without holding the mutex while sendMedia, sendClear, and sendDTMF read them concurrently. Although the write happens once at start, this is still a data race detectable by the Go race detector.
🔧 Suggested fix
func (tws *telnyxWebsocketStreamer) handleStartEvent(event TelnyxWebSocketEvent) {
if event.Start != nil {
+ tws.mu.Lock()
tws.streamID = event.StreamID
tws.callControlID = event.Start.CallControlID
tws.ChannelUUID = event.Start.CallControlID
+ tws.mu.Unlock()
tws.Logger.Debugf("Telnyx stream started | stream_id: %s, call_control_id: %s, format: %s %dHz",
tws.streamID, tws.callControlID,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go`
around lines 167 - 178, Protect updates to streamID, callControlID, and
ChannelUUID in handleStartEvent by acquiring the streamer's mutex before writing
and releasing it after (e.g., tws.mu.Lock()/Unlock()); likewise ensure read
access in sendMedia, sendClear, and sendDTMF uses the same mutex (e.g.,
tws.mu.RLock()/RUnlock()) so there is no race between handleStartEvent and those
methods; apply the lock around the assignments in
telnyxWebsocketStreamer.handleStartEvent and add corresponding read locks in
sendMedia, sendClear, and sendDTMF.
| func (tws *telnyxWebsocketStreamer) sendMedia(audioData []byte) error { | ||
| tws.mu.RLock() | ||
| conn := tws.connection | ||
| tws.mu.RUnlock() | ||
|
|
||
| if conn == nil || tws.streamID == "" { | ||
| return nil | ||
| } | ||
|
|
||
| message := map[string]interface{}{ | ||
| "event": "media", | ||
| "stream_id": tws.streamID, | ||
| "media": map[string]interface{}{ | ||
| "payload": tws.encoder.EncodeToString(audioData), | ||
| }, | ||
| } | ||
|
|
||
| messageJSON, err := json.Marshal(message) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal media message: %w", err) | ||
| } | ||
|
|
||
| tws.mu.Lock() | ||
| defer tws.mu.Unlock() | ||
| return tws.connection.WriteMessage(websocket.TextMessage, messageJSON) | ||
| } |
There was a problem hiding this comment.
Race condition: tws.connection can be nil when WriteMessage is called.
The method reads conn under RLock (line 281), releases the lock (line 282), then acquires Lock (line 301) and calls tws.connection.WriteMessage (line 303). If Cancel() runs between releasing RLock and acquiring Lock, it sets tws.connection = nil, causing a nil pointer dereference.
Use the cached conn variable for writing instead of accessing tws.connection directly.
🔧 Suggested fix
func (tws *telnyxWebsocketStreamer) sendMedia(audioData []byte) error {
tws.mu.RLock()
conn := tws.connection
tws.mu.RUnlock()
if conn == nil || tws.streamID == "" {
return nil
}
message := map[string]interface{}{
"event": "media",
"stream_id": tws.streamID,
"media": map[string]interface{}{
"payload": tws.encoder.EncodeToString(audioData),
},
}
messageJSON, err := json.Marshal(message)
if err != nil {
return fmt.Errorf("failed to marshal media message: %w", err)
}
tws.mu.Lock()
defer tws.mu.Unlock()
- return tws.connection.WriteMessage(websocket.TextMessage, messageJSON)
+ if tws.connection == nil {
+ return nil
+ }
+ return conn.WriteMessage(websocket.TextMessage, messageJSON)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go`
around lines 279 - 304, In sendMedia, avoid dereferencing tws.connection after
releasing the RLock: cache the connection in conn (already done) and use
conn.WriteMessage instead of tws.connection.WriteMessage; ensure you check conn
!= nil (and tws.streamID != "") before marshalling and then call
conn.WriteMessage with the cached variable so Cancel() cannot set tws.connection
= nil between locks and cause a nil deref (symbols: sendMedia, tws.connection,
conn, tws.mu, WriteMessage).
| tws.mu.Lock() | ||
| defer tws.mu.Unlock() | ||
| return tws.connection.WriteMessage(websocket.TextMessage, messageJSON) |
There was a problem hiding this comment.
Same race condition in sendClear and sendDTMF.
Both methods have the same RLock-then-Lock pattern that can cause nil pointer dereference. Apply the same fix: use the cached conn variable and add a nil check after acquiring the write lock.
🔧 Suggested fix for sendClear
tws.mu.Lock()
defer tws.mu.Unlock()
- return tws.connection.WriteMessage(websocket.TextMessage, messageJSON)
+ if tws.connection == nil {
+ return nil
+ }
+ return conn.WriteMessage(websocket.TextMessage, messageJSON)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go`
around lines 326 - 328, sendClear and sendDTMF use an RLock-then-Lock pattern
that can race and cause nil deref; instead, under the read lock cache the
connection into a local variable (e.g., conn := tws.connection), release the
read lock, then acquire the write lock, check that conn != nil before writing,
and use the cached conn to call WriteMessage (and return an error if nil).
Update both sendClear and sendDTMF to follow this pattern and avoid directly
accessing tws.connection after upgrading locks.
|
Thanks @a692570 for raising PR. If you can please update your branch from main and resolve conflicts, Do let us know if you require any help. |
Implements Telnyx Call Control API + WebSocket streaming for the Rapida
voice-ai platform.
- `telephony.go`: Implements the Telephony interface for Telnyx
- OutboundCall: POST to Telnyx Call Control API with stream_url
- ReceiveCall: Parse inbound call webhooks
- InboundCall: Return JSON to initiate WebSocket streaming
- StatusCallback: Handle Telnyx webhook events (call.answered, etc.)
- Auth: Extract api_key and connection_id from vault credentials
- HangupCall: Gracefully terminate calls via Call Control API
- `websocket.go`: Implements bidirectional audio streaming
- Handles Telnyx JSON WebSocket protocol (start/media/stop events)
- Audio resampling: PCMU 8kHz <-> Linear16 16kHz
- Media events: base64-encoded PCMU audio
- DTMF support and audio interruption (clear command)
- Factory updates in `telephony.go`:
- Added `Telnyx` constant to Telephony enum
- Added cases in GetTelephony() and NewStreamer()
- start: `{"event":"start","stream_id":"...","start":{...}}`
- media: `{"event":"media","media":{"payload":"<base64>"}}`
- stop: `{"event":"stop","stop":{"call_control_id":"..."}}`
- clear: `{"event":"clear","stream_id":"..."}`
- dtmf: `{"event":"dtmf","dtmf":{"digit":"1"}}`
- Unit tests for telephony.go (StatusCallback, ReceiveCall, Auth)
- WebSocket event parsing tests
- Interface compliance check
- No hardcoded credentials
- All secrets from vault credentials (api_key, connection_id)
1e3fb7b to
e390fc8
Compare
|
Thanks @iamprashant! I've rebased the branch onto main and resolved the conflict in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/assistant-api/internal/channel/telephony/telephony.go`:
- Line 141: The return statement currently appends an extra ", nil" to
internal_asterisk_websocket.NewAsteriskWebsocketStreamer(logger,
opt.WebSocketConn, cc, vaultCred) which already returns (internal_type.Streamer,
error); remove the trailing ", nil" so the function simply returns the two
values produced by NewAsteriskWebsocketStreamer. Update the return in
telephony.go (the call to NewAsteriskWebsocketStreamer) to return only its
direct result.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65eeb1ed-88cb-4866-936d-9a35426e338e
📒 Files selected for processing (4)
api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.goapi/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.goapi/assistant-api/internal/channel/telephony/internal/telnyx/websocket.goapi/assistant-api/internal/channel/telephony/telephony.go
✅ Files skipped from review due to trivial changes (1)
- api/assistant-api/internal/channel/telephony/internal/telnyx/telephony_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/assistant-api/internal/channel/telephony/internal/telnyx/telephony.go
- api/assistant-api/internal/channel/telephony/internal/telnyx/websocket.go
| return internal_asterisk_audiosocket.NewStreamer(logger, opt.AudioSocketConn, opt.AudioSocketReader, opt.AudioSocketWriter, cc, vaultCred) | ||
| } | ||
| return internal_asterisk_websocket.NewAsteriskWebsocketStreamer(logger, opt.WebSocketConn, cc, vaultCred) | ||
| return internal_asterisk_websocket.NewAsteriskWebsocketStreamer(logger, opt.WebSocketConn, cc, vaultCred), nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify constructor signature and call site arity in this package.
set -euo pipefail
echo "== Asterisk websocket constructor signature =="
rg -n -C2 'func\s+NewAsteriskWebsocketStreamer\s*\(' api/assistant-api/internal/channel/telephony/internal/asterisk/websocket/websocket.go
echo
echo "== Current call site in telephony factory =="
rg -n -C2 'NewAsteriskWebsocketStreamer\s*\(' api/assistant-api/internal/channel/telephony/telephony.goRepository: rapidaai/voice-ai
Length of output: 734
🏁 Script executed:
#!/bin/bash
# Get the full function signature including return type
sed -n '50,60p' api/assistant-api/internal/channel/telephony/internal/asterisk/websocket/websocket.goRepository: rapidaai/voice-ai
Length of output: 584
Remove the , nil appended to the constructor call on line 141.
internal_asterisk_websocket.NewAsteriskWebsocketStreamer(...) already returns (internal_type.Streamer, error). Appending , nil creates an invalid return with 3 values and will fail compilation.
Fix
- return internal_asterisk_websocket.NewAsteriskWebsocketStreamer(logger, opt.WebSocketConn, cc, vaultCred), nil
+ return internal_asterisk_websocket.NewAsteriskWebsocketStreamer(logger, opt.WebSocketConn, cc, vaultCred)📝 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.
| return internal_asterisk_websocket.NewAsteriskWebsocketStreamer(logger, opt.WebSocketConn, cc, vaultCred), nil | |
| return internal_asterisk_websocket.NewAsteriskWebsocketStreamer(logger, opt.WebSocketConn, cc, vaultCred) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/assistant-api/internal/channel/telephony/telephony.go` at line 141, The
return statement currently appends an extra ", nil" to
internal_asterisk_websocket.NewAsteriskWebsocketStreamer(logger,
opt.WebSocketConn, cc, vaultCred) which already returns (internal_type.Streamer,
error); remove the trailing ", nil" so the function simply returns the two
values produced by NewAsteriskWebsocketStreamer. Update the return in
telephony.go (the call to NewAsteriskWebsocketStreamer) to return only its
direct result.
Summary
Implements Telnyx Call Control API + WebSocket streaming for the Rapida voice-ai platform.
This PR adds a new telephony provider for Telnyx, following the existing patterns established by Twilio, Vonage, and other providers.
Implementation
telephony.goImplements the
Telephonyinterface for Telnyx:/v2/calls) withstream_urlfor bidirectional audiocall.answered,call.hangup, etc.)api_keyandconnection_idfrom vault credentialswebsocket.goImplements bidirectional audio streaming via WebSocket:
start/media/stopevents)clearcommand)Factory updates (
telephony.go)Telnyxconstant toTelephonyenumGetTelephony()andNewStreamer()factoriesTelnyx WebSocket Protocol
{"event":"start","stream_id":"...","start":{"call_control_id":"..."}}{"event":"media","media":{"payload":"<base64 PCMU>"}}{"event":"stop","stop":{"call_control_id":"..."}}{"event":"clear","stream_id":"..."}{"event":"dtmf","dtmf":{"digit":"1"}}Tests
telephony.go(StatusCallback, ReceiveCall, Auth)Security
api_key,connection_id)Testing Checklist
Summary by CodeRabbit
New Features
Tests