Feat/vobiz telephony#141
Open
Piyush-sahoo wants to merge 8 commits into
Open
Conversation
Add Vobiz as a telephony provider with two transports: WebSocket media streaming (vobiz_websocket) and auto-provisioned SIP trunking (vobiz_sip) reusing the existing SIP engine. Includes the Vobiz REST client (trunk + standalone credential provisioning with credential_uuid attach), vault-backed credential storage, inbound/outbound call handling, answer-XML Stream generation, status-callback parsing, and mulaw/L16 audio processing. Wires both providers into the telephony and streamer factories, widens SIP selection and the answer_url route to cover vobiz_sip, and adds UI provider catalog entries.
Embed the Vobiz brand mark (inline SVG data URI) as the icon for the vobiz_sip and vobiz_websocket provider cards, replacing the generic SIP icon.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Vobiz telephony support (WebSocket + auto-provisioned SIP): REST client, provider implementations, audio resampling/processor, websocket streamer, SIP provisioning and vault integration, router and UI updates, and direct vault helper. ChangesVobiz Telephony Provider Implementation
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant VobizProvider
participant VobizClient
participant VobizAPI as Vobiz API
Client->>VobizProvider: OutboundCall(toPhone, fromPhone, assistant, context_id)
VobizProvider->>VobizProvider: validate creds, build answerURL/callbacks
VobizProvider->>VobizClient: MakeCall(authID, authToken, MakeCallRequest)
VobizClient->>VobizAPI: POST /calls (auth headers)
VobizAPI-->>VobizClient: 2xx {RequestUUID}
VobizClient-->>VobizProvider: CallResponse
VobizProvider->>Client: CallInfo (with RequestUUID)
sequenceDiagram
participant WebSocket as Vobiz WebSocket
participant Streamer
participant AudioProcessor
participant MediaSession as Internal Media
WebSocket->>Streamer: VobizMediaEvent {EventType: start, streamID}
Streamer->>MediaSession: Start()
WebSocket->>Streamer: VobizMediaEvent {EventType: media, payload}
Streamer->>AudioProcessor: ProcessProviderAudioFrame(mulaw 8kHz)
AudioProcessor->>MediaSession: InputAudioFrame (linear16 16kHz)
MediaSession->>AudioProcessor: ProcessAssistantAudio(linear16 16kHz)
AudioProcessor->>Streamer: NextOutputFrame (mulaw 8kHz)
Streamer->>WebSocket: sendPlayAudio(VobizPlayAudioMessage)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Piyush-sahoo
force-pushed
the
feat/vobiz-telephony
branch
from
e7c5124 to
3aa77ba
Compare
![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 (10)
api/assistant-api/sip/sip.go (2)
580-580: ⚡ Quick winConsider using Telephony constants instead of string literals.
The query uses
[]string{"sip", "vobiz_sip"}as string literals, butTelephonyconstants are defined inapi/assistant-api/internal/channel/telephony/telephony.go. Using[]string{channel_telephony.SIP.String(), channel_telephony.VobizSIP.String()}would improve maintainability and prevent silent breakage if constant values change.♻️ Refactor to use constants
- Where("apd.telephony_provider IN ? AND apd.status = ?", []string{"sip", "vobiz_sip"}, type_enums.RECORD_ACTIVE). + Where("apd.telephony_provider IN ? AND apd.status = ?", []string{channel_telephony.SIP.String(), channel_telephony.VobizSIP.String()}, type_enums.RECORD_ACTIVE).You'll need to add the import:
channel_telephony "github.com/rapidaai/api/assistant-api/internal/channel/telephony"🤖 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 `@api/assistant-api/sip/sip.go` at line 580, Replace the string literals in the DB filter with the Telephony constants: change the slice []string{"sip", "vobiz_sip"} to use channel_telephony.SIP.String() and channel_telephony.VobizSIP.String() so the query in the Where(...) call uses the defined constants; also add the import alias channel_telephony "github.com/rapidaai/api/assistant-api/internal/channel/telephony" at the top of the file so the symbols resolve.
81-83: ⚡ Quick winConsider constructing VaultClient once and reusing the instance.
NewVaultClientGRPCis called twice with identical arguments — once for the deployment service (line 81) and once for the engine's own field (line 83). Constructing the client once and passing the same instance to both would avoid duplicate initialization and potential duplicate gRPC connections.♻️ Refactor to construct once
+ vaultClient := web_client.NewVaultClientGRPC(&config.AppConfig, logger, redis) return &SIPEngine{ cfg: config, logger: logger, postgres: postgres, redis: redis, opensearch: opensearch, assistantConversationService: internal_assistant_service.NewAssistantConversationService(logger, postgres, storage_files.NewStorage(config.AssetStoreConfig, logger)), assistantService: internal_assistant_service.NewAssistantService(config, logger, postgres, opensearch), - deploymentService: internal_assistant_service.NewAssistantDeploymentService(config, logger, postgres, web_client.NewVaultClientGRPC(&config.AppConfig, logger, redis)), + deploymentService: internal_assistant_service.NewAssistantDeploymentService(config, logger, postgres, vaultClient), storage: storage_files.NewStorage(config.AssetStoreConfig, logger), - vaultClient: web_client.NewVaultClientGRPC(&config.AppConfig, logger, redis), + vaultClient: vaultClient, callContextStore: callcontext.NewStore(postgres, logger), }🤖 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 `@api/assistant-api/sip/sip.go` around lines 81 - 83, The code calls web_client.NewVaultClientGRPC(&config.AppConfig, logger, redis) twice; create the Vault client once and reuse it by instantiating a single variable (e.g., vaultClient := web_client.NewVaultClientGRPC(&config.AppConfig, logger, redis)) and pass that same instance into internal_assistant_service.NewAssistantDeploymentService(...) for the deploymentService parameter and assign it to the engine's vaultClient field, updating uses of deploymentService and vaultClient accordingly to avoid duplicate initialization and gRPC connections.api/assistant-api/sip/registration/record.go (1)
35-35: ⚡ Quick winConsider using Telephony constants instead of string literals.
The query uses
[]string{"sip", "vobiz_sip"}as string literals, butTelephonyconstants are defined inapi/assistant-api/internal/channel/telephony/telephony.go. Using[]string{channel_telephony.SIP.String(), channel_telephony.VobizSIP.String()}would improve maintainability and prevent silent breakage if constant values change.♻️ Refactor to use constants
- Where("telephony_provider IN ? AND status = ?", []string{"sip", "vobiz_sip"}, type_enums.RECORD_ACTIVE). + Where("telephony_provider IN ? AND status = ?", []string{channel_telephony.SIP.String(), channel_telephony.VobizSIP.String()}, type_enums.RECORD_ACTIVE).You'll need to add the import:
channel_telephony "github.com/rapidaai/api/assistant-api/internal/channel/telephony"🤖 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 `@api/assistant-api/sip/registration/record.go` at line 35, Replace the string literals []string{"sip", "vobiz_sip"} used in the GORM Where clause with the Telephony constants (e.g. channel_telephony.SIP.String() and channel_telephony.VobizSIP.String()) so the query becomes Where("telephony_provider IN ? AND status = ?", []string{channel_telephony.SIP.String(), channel_telephony.VobizSIP.String()}, type_enums.RECORD_ACTIVE); add the import alias channel_telephony from the internal/channel/telephony package and ensure the code compiles and tests pass after the change.api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go (7)
446-449: 💤 Low valueUse
fmt.Errorfinstead oferrors.Newfor consistency.As per coding guidelines, Go errors should use
fmt.Errorffor consistent formatting and to enable context wrapping when needed.🔧 Proposed fix
if credIDStr == "" { - return nil, errors.New("a Vobiz SIP credential is required to enable inbound calls") + return nil, fmt.Errorf("a Vobiz SIP credential is required to enable inbound calls") }🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 446 - 449, Replace the errors.New call with fmt.Errorf for consistency and potential wrapping: when reading optMap["rapida.credential_id"] into credIDStr and returning the error "a Vobiz SIP credential is required to enable inbound calls", use fmt.Errorf to construct the error (and add fmt to the imports if missing); keep the same message but use fmt.Errorf to follow project error formatting conventions.Source: Coding guidelines
465-467: 💤 Low valueUse
fmt.Errorfinstead oferrors.Newfor consistency.As per coding guidelines, Go errors should use
fmt.Errorffor consistent formatting and to enable context wrapping when needed.🔧 Proposed fix
if eService.cfg.SIPConfig == nil || eService.cfg.SIPConfig.ExternalIP == "" { - return nil, errors.New("the assistant SIP server has no external IP configured; cannot provision inbound") + return nil, fmt.Errorf("the assistant SIP server has no external IP configured; cannot provision inbound") }🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 465 - 467, Replace the errors.New call with fmt.Errorf to follow project conventions: in the block that checks eService.cfg.SIPConfig and eService.cfg.SIPConfig.ExternalIP (the code returning "return nil, errors.New(...)"), change it to return nil, fmt.Errorf("the assistant SIP server has no external IP configured; cannot provision inbound") and ensure the "fmt" package is imported; keep the message intact so it can be wrapped later if needed.Source: Coding guidelines
454-456: 💤 Low valueUse
fmt.Errorfinstead oferrors.Newfor consistency.As per coding guidelines, Go errors should use
fmt.Errorffor consistent formatting and to enable context wrapping when needed.🔧 Proposed fix
if eService.vaultClient == nil { - return nil, errors.New("inbound provisioning is unavailable in this context") + return nil, fmt.Errorf("inbound provisioning is unavailable in this context") }🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 454 - 456, The code returns errors.New(...) when eService.vaultClient is nil; replace that with fmt.Errorf("inbound provisioning is unavailable in this context") to follow project convention and enable future error wrapping, and add an import for "fmt" in the file if not already present; update the return in the method where eService.vaultClient is checked (the block starting with "if eService.vaultClient == nil") to use fmt.Errorf.Source: Coding guidelines
476-478: 💤 Low valueUse
fmt.Errorfinstead oferrors.Newfor consistency.As per coding guidelines, Go errors should use
fmt.Errorffor consistent formatting and to enable context wrapping when needed.🔧 Proposed fix
if authID == "" || authToken == "" { - return nil, errors.New("the Vobiz SIP credential is missing auth_id/auth_token") + return nil, fmt.Errorf("the Vobiz SIP credential is missing auth_id/auth_token") }🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 476 - 478, Replace the errors.New call with fmt.Errorf to follow project error formatting conventions: change `return nil, errors.New("the Vobiz SIP credential is missing auth_id/auth_token")` to `return nil, fmt.Errorf("the Vobiz SIP credential is missing auth_id/auth_token")`, and add the fmt import if not present; keep the same message so callers' behavior is unchanged but now supports wrapping/formatting for variables like authID/authToken.Source: Coding guidelines
469-472: 💤 Low valueUse
fmt.Errorfinstead oferrors.Newfor consistency.As per coding guidelines, Go errors should use
fmt.Errorffor consistent formatting and to enable context wrapping when needed.🔧 Proposed fix
if err != nil { - return nil, errors.New("could not read the Vobiz SIP credential") + return nil, fmt.Errorf("could not read the Vobiz SIP credential: %w", err) }🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 469 - 472, Replace the plain errors.New usage in the GetCredential error branch with fmt.Errorf so the original error is wrapped and preserved; specifically, in the block after calling eService.vaultClient.GetCredential(ctx, auth, credentialID) (variables cred, err, credentialID), return fmt.Errorf("could not read the Vobiz SIP credential: %w", err) instead of errors.New(...), and add an import for "fmt" if missing.Source: Coding guidelines
450-453: 💤 Low valueUse
fmt.Errorfinstead oferrors.Newfor consistency.As per coding guidelines, Go errors should use
fmt.Errorffor consistent formatting and to enable context wrapping when needed.🔧 Proposed fix
if err != nil { - return nil, errors.New("invalid Vobiz SIP credential") + return nil, fmt.Errorf("invalid Vobiz SIP credential") }🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 450 - 453, Replace the errors.New call when parsing the SIP credential ID with fmt.Errorf and wrap the original parse error for context: after strconv.ParseUint(credIDStr, 10, 64) returns err, return fmt.Errorf("invalid Vobiz SIP credential: %w", err) (referencing credIDStr, credentialID and the ParseUint call) so the original error is preserved and formatting is consistent.Source: Coding guidelines
443-445: 💤 Low valueUse
fmt.Errorfinstead oferrors.Newfor consistency.As per coding guidelines, Go errors should use
fmt.Errorffor consistent formatting and to enable context wrapping when needed.🔧 Proposed fix
if did == "" { - return nil, errors.New("a caller-ID phone number is required to enable inbound calls") + return nil, fmt.Errorf("a caller-ID phone number is required to enable inbound calls") }🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 443 - 445, Replace the errors.New call with fmt.Errorf to follow the project's error formatting convention: change return nil, errors.New("a caller-ID phone number is required to enable inbound calls") to return nil, fmt.Errorf("a caller-ID phone number is required to enable inbound calls") and ensure the fmt package is imported (or add it to the import list if missing) in assistant.deployment.impl.service.go; the affected check uses the did variable that validates caller ID.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/assistant-api/api/talk/answer.go`:
- Around line 44-46: The log message hard-codes "vobiz" even though this handler
is provider-agnostic; update the error log to reference the path param/provider
variable instead of "vobiz". Locate the handler where cApi.logger.Errorf is
called (the answer handler that reads the telephony provider from the request,
e.g., via c.Param("provider") or a local variable named provider) and change the
message to include that provider (for example: use provider variable in the
format string like "%s answer XML build failed: %v") so the log is generic and
reflects the actual provider.
In `@api/web-api/internal/service/vault/service.go`:
- Around line 38-40: The branch that directly returns vs.provisionVobiz(ctx,
auth, name, credential) should capture the error and wrap it with fmt.Errorf
using the %w verb to preserve the chain; replace the direct return with e.g. err
:= vs.provisionVobiz(ctx, auth, name, credential); if err != nil { return
fmt.Errorf("provisionVobiz failed: %w", err) } and ensure fmt is imported. Keep
the wrapped message concise and reference provisionVobiz/ vs.provisionVobiz so
the source of the failure is clear.
In `@api/web-api/internal/service/vault/vobiz.go`:
- Around line 176-178: The call to rand.Read(b) in the function in vobiz.go
returns the raw error; update the error return to wrap it with context using
fmt.Errorf and %w (for example: return "", fmt.Errorf("reading random bytes:
%w", err)) so the cryptographic error from rand.Read is preserved with
contextual information; import fmt if not already present and keep the existing
return types intact.
- Around line 139-141: The DB query error returned from the call that ends with
").Find(&rows).Error" should be wrapped with context using fmt.Errorf and %w
before returning; change the error return in that conditional to something like:
return nil, fmt.Errorf("querying vault vobiz rows: %w", err) (use an
appropriate, concise message referencing the operation) so the original error
chain is preserved.
- Around line 55-57: The validation check for sipUsername currently returns a
plain fmt.Errorf string; create a sentinel error (e.g. ErrMissingSIPUsername)
and return a wrapped error using fmt.Errorf with %w so callers can unwrap it.
Update the check in vobiz.go (the sipUsername validation) to return
fmt.Errorf("vobiz sip_username is required: %w", ErrMissingSIPUsername) instead
of the bare message, and declare ErrMissingSIPUsername = errors.New("missing
sip_username") near the top of the file.
- Around line 52-54: The validation branch that checks authID/authToken should
return a wrapped, contextual error instead of a plain fmt.Errorf string; create
a sentinel error (e.g., validationErr := errors.New("auth_id and auth_token are
required")) and return fmt.Errorf("vobiz validation: %w", validationErr) from
the authID/authToken check in the function that contains the authID/authToken
validation so the error is consistently wrapped with context (refer to the
authID/authToken validation block in vobiz.go).
- Around line 64-66: The validation return for sipPassword should wrap a
sentinel error for consistency: replace the bare fmt.Errorf(...) return in the
sipPassword length check with a wrapped error using fmt.Errorf("vobiz
sip_password must be at least 8 characters: %w", ErrInvalidParameter) (or create
a package-level sentinel like ErrInvalidParameter/ErrInvalidConfig if none
exists) so callers can inspect the underlying error while preserving the
descriptive message; update the sipPassword length check in vobiz.go
accordingly.
In `@pkg/clients/vobiz/client.go`:
- Around line 67-84: The validation error returned when trunk.TrunkID or
trunk.TrunkDomain is missing in CreateTrunk should be wrapped with a %w-wrapped
sentinel error to preserve error chains; define a package-level error (e.g.,
ErrInvalidTrunkResponse) and change the failing return in CreateTrunk (the
fmt.Errorf that includes resp.ToString()) to wrap that sentinel using "%w"
(e.g., fmt.Errorf("vobiz trunk response missing trunk_id/trunk_domain: %s: %w",
resp.ToString(), ErrInvalidTrunkResponse)). This keeps the existing
message/context while enabling callers to inspect the underlying validation
error.
In `@ui/src/app/components/base/modal/create-provider-credential-modal/index.tsx`:
- Around line 120-122: The success message check in
create-provider-credential-modal (index.tsx) uses provider.code === 'vobiz' but
the actual provider code is 'vobiz_sip', so the specialized Vobiz message never
appears; update the condition that reads provider.code === 'vobiz' to match the
real identifier (e.g., provider.code === 'vobiz_sip') or use a more flexible
check like provider.code?.startsWith('vobiz') so the Vobiz-specific success
message is shown for the real provider code.
- Around line 73-74: The conditional that makes the sip_password optional checks
provider?.code === 'vobiz' but the actual provider code is 'vobiz_sip'; update
the check in create-provider-credential-modal (the if handling field.name ===
'sip_password') to match the real provider code (e.g., change 'vobiz' to
'vobiz_sip' or compare against a canonical provider constant), so the branch
returns false for the correct provider code.
In `@ui/src/providers/vobiz_websocket/telephony.json`:
- Line 10: Update the placeholder value in the telephony.json entry that
currently uses "placeholder": "+91..." to a neutral, consistent example (e.g.,
an E.164 example like "e.g., +15551234567" or a generic "+...") so it matches
other Vobiz provider files (see the "placeholder" key in
vobiz_websocket/telephony.json and compare with vobiz_sip/telephony.json);
replace the regional "+91..." string with the chosen neutral placeholder in the
"placeholder" field.
---
Nitpick comments:
In
`@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go`:
- Around line 446-449: Replace the errors.New call with fmt.Errorf for
consistency and potential wrapping: when reading optMap["rapida.credential_id"]
into credIDStr and returning the error "a Vobiz SIP credential is required to
enable inbound calls", use fmt.Errorf to construct the error (and add fmt to the
imports if missing); keep the same message but use fmt.Errorf to follow project
error formatting conventions.
- Around line 465-467: Replace the errors.New call with fmt.Errorf to follow
project conventions: in the block that checks eService.cfg.SIPConfig and
eService.cfg.SIPConfig.ExternalIP (the code returning "return nil,
errors.New(...)"), change it to return nil, fmt.Errorf("the assistant SIP server
has no external IP configured; cannot provision inbound") and ensure the "fmt"
package is imported; keep the message intact so it can be wrapped later if
needed.
- Around line 454-456: The code returns errors.New(...) when
eService.vaultClient is nil; replace that with fmt.Errorf("inbound provisioning
is unavailable in this context") to follow project convention and enable future
error wrapping, and add an import for "fmt" in the file if not already present;
update the return in the method where eService.vaultClient is checked (the block
starting with "if eService.vaultClient == nil") to use fmt.Errorf.
- Around line 476-478: Replace the errors.New call with fmt.Errorf to follow
project error formatting conventions: change `return nil, errors.New("the Vobiz
SIP credential is missing auth_id/auth_token")` to `return nil, fmt.Errorf("the
Vobiz SIP credential is missing auth_id/auth_token")`, and add the fmt import if
not present; keep the same message so callers' behavior is unchanged but now
supports wrapping/formatting for variables like authID/authToken.
- Around line 469-472: Replace the plain errors.New usage in the GetCredential
error branch with fmt.Errorf so the original error is wrapped and preserved;
specifically, in the block after calling eService.vaultClient.GetCredential(ctx,
auth, credentialID) (variables cred, err, credentialID), return
fmt.Errorf("could not read the Vobiz SIP credential: %w", err) instead of
errors.New(...), and add an import for "fmt" if missing.
- Around line 450-453: Replace the errors.New call when parsing the SIP
credential ID with fmt.Errorf and wrap the original parse error for context:
after strconv.ParseUint(credIDStr, 10, 64) returns err, return
fmt.Errorf("invalid Vobiz SIP credential: %w", err) (referencing credIDStr,
credentialID and the ParseUint call) so the original error is preserved and
formatting is consistent.
- Around line 443-445: Replace the errors.New call with fmt.Errorf to follow the
project's error formatting convention: change return nil, errors.New("a
caller-ID phone number is required to enable inbound calls") to return nil,
fmt.Errorf("a caller-ID phone number is required to enable inbound calls") and
ensure the fmt package is imported (or add it to the import list if missing) in
assistant.deployment.impl.service.go; the affected check uses the did variable
that validates caller ID.
In `@api/assistant-api/sip/registration/record.go`:
- Line 35: Replace the string literals []string{"sip", "vobiz_sip"} used in the
GORM Where clause with the Telephony constants (e.g.
channel_telephony.SIP.String() and channel_telephony.VobizSIP.String()) so the
query becomes Where("telephony_provider IN ? AND status = ?",
[]string{channel_telephony.SIP.String(), channel_telephony.VobizSIP.String()},
type_enums.RECORD_ACTIVE); add the import alias channel_telephony from the
internal/channel/telephony package and ensure the code compiles and tests pass
after the change.
In `@api/assistant-api/sip/sip.go`:
- Line 580: Replace the string literals in the DB filter with the Telephony
constants: change the slice []string{"sip", "vobiz_sip"} to use
channel_telephony.SIP.String() and channel_telephony.VobizSIP.String() so the
query in the Where(...) call uses the defined constants; also add the import
alias channel_telephony
"github.com/rapidaai/api/assistant-api/internal/channel/telephony" at the top of
the file so the symbols resolve.
- Around line 81-83: The code calls
web_client.NewVaultClientGRPC(&config.AppConfig, logger, redis) twice; create
the Vault client once and reuse it by instantiating a single variable (e.g.,
vaultClient := web_client.NewVaultClientGRPC(&config.AppConfig, logger, redis))
and pass that same instance into
internal_assistant_service.NewAssistantDeploymentService(...) for the
deploymentService parameter and assign it to the engine's vaultClient field,
updating uses of deploymentService and vaultClient accordingly to avoid
duplicate initialization and gRPC connections.
🪄 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: 3342ae23-68f0-4c57-971f-db3909204bc2
📒 Files selected for processing (26)
api/assistant-api/api/assistant-deployment/assistant_deployment.goapi/assistant-api/api/assistant-deployment/create_assistant_phone_deployment.goapi/assistant-api/api/talk/answer.goapi/assistant-api/internal/channel/telephony/internal/vobiz/internal/audio_processor.goapi/assistant-api/internal/channel/telephony/internal/vobiz/internal/status_callback.goapi/assistant-api/internal/channel/telephony/internal/vobiz/internal/types.goapi/assistant-api/internal/channel/telephony/internal/vobiz/telephony.goapi/assistant-api/internal/channel/telephony/internal/vobiz/websocket.goapi/assistant-api/internal/channel/telephony/outbound.goapi/assistant-api/internal/channel/telephony/streamer.goapi/assistant-api/internal/channel/telephony/telephony.goapi/assistant-api/internal/services/assistant/assistant.deployment.impl.service.goapi/assistant-api/router/assistant.goapi/assistant-api/sip/registration/record.goapi/assistant-api/sip/sip.goapi/web-api/internal/service/vault/service.goapi/web-api/internal/service/vault/vobiz.gocmd/assistant/assistant.gopkg/clients/vobiz/client.gopkg/clients/vobiz/client_test.gopkg/clients/vobiz/types.goui/src/app/components/base/modal/create-provider-credential-modal/index.tsxui/src/providers/provider.development.jsonui/src/providers/provider.production.jsonui/src/providers/vobiz_sip/telephony.jsonui/src/providers/vobiz_websocket/telephony.json
Comment on lines
+55
to
+57
| if sipUsername == "" { | ||
| return nil, fmt.Errorf("vobiz sip_username is required") | ||
| } |
There was a problem hiding this comment.
Wrap validation error with context.
The error message is constructed with fmt.Errorf but doesn't wrap an underlying error. For consistency with the coding guidelines, use fmt.Errorf for all error returns.
🔧 Proposed fix
if sipUsername == "" {
- return nil, fmt.Errorf("vobiz sip_username is required")
+ return nil, fmt.Errorf("vobiz: sip_username is required")
}🤖 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 `@api/web-api/internal/service/vault/vobiz.go` around lines 55 - 57, The
validation check for sipUsername currently returns a plain fmt.Errorf string;
create a sentinel error (e.g. ErrMissingSIPUsername) and return a wrapped error
using fmt.Errorf with %w so callers can unwrap it. Update the check in vobiz.go
(the sipUsername validation) to return fmt.Errorf("vobiz sip_username is
required: %w", ErrMissingSIPUsername) instead of the bare message, and declare
ErrMissingSIPUsername = errors.New("missing sip_username") near the top of the
file.
Source: Coding guidelines
- Fix provider-code checks in the credential modal (vobiz -> vobiz_sip) so sip_password is correctly optional and the provisioning success message shows. - Make the answer-XML error log provider-agnostic (it serves any telephony provider). - Wrap the crypto-rand error with %w in the SIP password generator. - Use a generic E.164 placeholder for the vobiz_websocket caller-ID field.
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)
api/web-api/internal/service/vault/vobiz.go (1)
68-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSilent error swallowing may cause duplicate trunk creation.
If
findExistingVobizSIPfails due to a transient DB error, the error is silently ignored and the code proceeds to provision a new trunk. This could create duplicate trunks in Vobiz for the sametrunkNamewhen the DB recovers.Consider returning the error when the lookup fails, rather than only when
err == nil:🔧 Proposed fix
// Idempotency: reuse an existing trunk provisioned for the same name. - if existing, err := vs.findExistingVobizSIP(ctx, auth, trunkName); err == nil && existing != nil { + existing, err := vs.findExistingVobizSIP(ctx, auth, trunkName) + if err != nil { + return nil, err // already wrapped with context + } + if existing != nil { vs.logger.Debugf("vobiz trunk %q already provisioned, reusing existing sip credential", trunkName) return existing, nil }🤖 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 `@api/web-api/internal/service/vault/vobiz.go` around lines 68 - 72, The current idempotency check silently swallows errors from vs.findExistingVobizSIP leading to possible duplicate trunk creation; update the logic in the provision flow so that if vs.findExistingVobizSIP(ctx, auth, trunkName) returns a non-nil err you surface that error (e.g., return it up the call chain or wrap and return) instead of proceeding to provision a new trunk, while keeping the existing early-return when an existing record is found; locate the conditional that calls vs.findExistingVobizSIP and add explicit error handling for err != nil (logging + return err) so transient DB failures won't allow duplicate provisioning.
🤖 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 `@api/web-api/internal/service/vault/vobiz.go`:
- Around line 68-72: The current idempotency check silently swallows errors from
vs.findExistingVobizSIP leading to possible duplicate trunk creation; update the
logic in the provision flow so that if vs.findExistingVobizSIP(ctx, auth,
trunkName) returns a non-nil err you surface that error (e.g., return it up the
call chain or wrap and return) instead of proceeding to provision a new trunk,
while keeping the existing early-return when an existing record is found; locate
the conditional that calls vs.findExistingVobizSIP and add explicit error
handling for err != nil (logging + return err) so transient DB failures won't
allow duplicate provisioning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e54b9f45-f453-44d8-a128-77706fecaa91
📒 Files selected for processing (1)
api/web-api/internal/service/vault/vobiz.go
When a vobiz_sip phone deployment enables inbound calls (rapida.sip_inbound=true), the deployment service auto-creates the Vobiz-side config via the REST API: an origination URI pointing at this assistant-api's configured SIP address (sip.external_ip:port), an inbound trunk referencing it, and assigns the deployment's DID — then persists rapida.inbound_trunk_id/uri_id so re-saves reuse the trunk. Vobiz failures (number already attached / not in account) surface to the UI via a typed InboundProvisioningError; all other errors keep the generic message, so non-vobiz flows are unchanged. The Vobiz credential is read with a redis-free one-shot gRPC fetch (web.GetCredentialDirect), so no vault-client/redis wiring is threaded through the bootstrap/router/constructors. Adds CreateOriginationURI + AssignNumber + inbound trunk fields to the Vobiz client.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
pkg/clients/web/vault_client.go (1)
146-157: ⚡ Quick winConsider accepting a context parameter with timeout support.
The function uses
context.Background()without any deadline. gRPC calls have no default timeout, so if the vault service becomes unresponsive, this call will hang indefinitely. The existing methods in this file (GetCredential,GetOauth2Credential) accept a context parameter, allowing callers to control timeouts and cancellation.♻️ Suggested refactor to accept context with timeout
-func GetCredentialDirect(cfg *config.AppConfig, logger commons.Logger, auth types.SimplePrinciple, vaultId uint64) (*vault_api.VaultCredential, error) { +func GetCredentialDirect(ctx context.Context, cfg *config.AppConfig, logger commons.Logger, auth types.SimplePrinciple, vaultId uint64) (*vault_api.VaultCredential, error) { conn, err := grpc.NewClient(cfg.Web.Host, grpc.WithTransportCredentials(insecure.NewCredentials())) if err != nil { return nil, fmt.Errorf("vault: unable to connect to web-api: %w", err) } defer conn.Close() token, err := types.CreateServiceScopeToken(auth, cfg.Secret) if err != nil { return nil, fmt.Errorf("vault: unable to create service token: %w", err) } - ctx := metadata.NewOutgoingContext(context.Background(), metadata.New(map[string]string{types.SERVICE_SCOPE_KEY: token})) + ctx = metadata.NewOutgoingContext(ctx, metadata.New(map[string]string{types.SERVICE_SCOPE_KEY: token}))Callers can then pass a context with timeout:
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() cred, err := web_client.GetCredentialDirect(ctx, cfg, logger, auth, vaultId)🤖 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 `@pkg/clients/web/vault_client.go` around lines 146 - 157, Update GetCredentialDirect to accept a context.Context parameter (e.g., change signature to GetCredentialDirect(ctx context.Context, cfg *config.AppConfig, logger commons.Logger, auth types.SimplePrinciple, vaultId uint64)) and use that context when creating the outgoing metadata context instead of context.Background() (replace metadata.NewOutgoingContext(context.Background(), ...) with metadata.NewOutgoingContext(ctx, ...)). Ensure the function uses the passed context for the gRPC call so callers can set timeouts/cancellation (caller sites should create a context with timeout and pass it in) and keep token creation via types.CreateServiceScopeToken and conn lifecycle unchanged.api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go (1)
29-35: 💤 Low valueConsider unexporting the Message field.
The
InboundProvisioningError.Messagefield is exported but only accessed via theError()method. Unexported fields are preferred when direct external access isn't required.♻️ Proposed refactor
-type InboundProvisioningError struct{ Message string } +type InboundProvisioningError struct{ message string } -func (e *InboundProvisioningError) Error() string { return e.Message } +func (e *InboundProvisioningError) Error() string { return e.message }Then update all instantiations to use lowercase
message:field name.🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 29 - 35, The InboundProvisioningError struct exposes its Message field unnecessarily; change the struct to use an unexported field (rename Message to message) and keep the Error() method as-is to surface the message; then update every instantiation of InboundProvisioningError to set the unexported field using the lowercase name (e.g. InboundProvisioningError{message: "..."}), leaving calls to Error() unchanged.
🤖 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
`@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go`:
- Around line 427-570: Add a new *_test.go alongside
assistant.deployment.impl.service.go that provides unit tests for the new
provisioning logic: write table-driven tests for maybeProvisionVobizInbound
covering provider check, rapida.sip_inbound flag, missing phone/credential
parsing errors, and successful path (mock web_client.GetCredentialDirect to
return creds); unit tests for provisionVobizInbound that mock vobiz.Client
(CreateOriginationURI, CreateTrunk, AssignNumber, DeleteTrunk) to assert call
ordering, successful return, and rollback when AssignNumber fails; tests for
existingInboundTrunk that seed a fake DB/ORM with AssistantPhoneDeployment
entries to verify idempotency lookup; tests for vobizAssignError that construct
vobiz.VobizAPIError with different StatusCode values to assert mapped messages;
and a small test asserting InboundProvisioningError values; use dependency
injection/mocks for eService.provisionVobizInbound and vobiz client, and
table-driven subtests for clarity.
- Around line 477-482: The code uses cred.GetValue().AsMap() and then performs
unchecked type assertions for authID and authToken, allowing one missing or
wrong-typed field to slip through; update the extraction to verify types
explicitly (e.g., check that value["auth_id"] and value["auth_token"] exist and
are strings) and return an InboundProvisioningError with the same message if
either assertion fails or a field is missing/empty, so both type and presence
are enforced before proceeding.
- Around line 456-459: The parse error from strconv.ParseUint on credIDStr is
being swallowed; modify the error return so it preserves context by wrapping the
original err using fmt.Errorf with the %w verb and include it in the returned
InboundProvisioningError (e.g., construct the InboundProvisioningError to carry
the wrapped error or set its Message to a descriptive string and an Err field
containing fmt.Errorf("parsing credential ID %q: %w", credIDStr, err)); update
the block around credentialID, err := strconv.ParseUint(credIDStr, 10, 64) to
return the wrapped error instead of the generic message so callers can inspect
the underlying parsing error.
- Around line 558-570: The fallback return in vobizAssignError currently builds
an InboundProvisioningError with fmt.Sprintf which discards the original error
chain; change this so the original err is wrapped (use fmt.Errorf with "%w") or
modify InboundProvisioningError to carry a Cause error field and set it to err,
then return the InboundProvisioningError containing the wrapped/original error
so error unwrapping works (update vobizAssignError to wrap or set Cause and
ensure consumers still detect InboundProvisioningError).
- Around line 534-538: The DB query error is being swallowed; update the error
branch after
eService.postgres.DB(...).Preload("TelephonyOption").Where(...).Find(&deployments).Error
to log the error (include the error value and context such as assistantId and
telephony_provider "vobiz_sip") before returning; use the service logger (e.g.,
eService.logger.Errorf or similar) or fallback to log.Printf to emit a message
like "failed querying deployments for assistant %s provider vobiz_sip: %v" and
then return the same values so the failure is visible in logs.
- Around line 499-504: The error returned from client.CreateOriginationURI is
being converted to a string and wrapped in an InboundProvisioningError, which
loses error unwrapping; update the error handling in the CreateOriginationURI
call to wrap the original error using fmt.Errorf with the %w verb (e.g.,
fmt.Errorf("failed to create the Vobiz origination URI: %w", err)) so callers
can unwrap it, and if you must return an InboundProvisioningError preserve that
type while also returning or logging the wrapped error (or refactor
InboundProvisioningError to hold an inner error) so error chain information from
CreateOriginationURI is not lost.
- Around line 473-475: When GetCredentialDirect(&eService.cfg.AppConfig,
eService.logger, auth, credentialID) returns an error, preserve the original
error by wrapping it with fmt.Errorf rather than discarding it; replace the
current return that creates an InboundProvisioningError with something like:
create a wrappedErr := fmt.Errorf("could not read the Vobiz SIP credential
(credentialID=%s): %w", credentialID, err) and return nil, wrappedErr (or set
InboundProvisioningError to include the wrappedErr string) and add the fmt
import if missing so the original error chain from GetCredentialDirect is
preserved.
- Around line 506-518: The CreateTrunk error is being converted to a string via
fmt.Sprintf which loses the original error for unwrapping; update the error
return from the CreateTrunk call to preserve the error chain by wrapping the
original err with fmt.Errorf and the %w verb (e.g. return ...,
fmt.Errorf("failed to create the Vobiz inbound trunk: %w", err)) or
alternatively extend the InboundProvisioningError type to carry the underlying
error and set that field instead; locate the CreateTrunk invocation and the
return that constructs InboundProvisioningError and change it to wrap/attach the
original err so callers can unwrap it.
In `@pkg/clients/web/vault_client.go`:
- Around line 140-170: Add a new test file in the same package that exercises
GetCredentialDirect and covers (1) success returning a VaultCredential, (2)
service-level error returned via res.Error.HumanMessage, and (3) RPC-level
failure (e.g. client connection error); implement each test by starting a local
gRPC server that registers a fake implementation of vault_api.VaultServiceServer
which inspects the incoming metadata and responds to GetCredentialRequest
accordingly, then call GetCredentialDirect (which uses grpc.NewClient and
vault_api.NewVaultServiceClient) and assert the returned value/error matches the
scenario; ensure the tests build/run without external services by using an
in-process grpc.Server with insecure creds and distinct handlers that return a
populated GetCredentialResponse, a response with Error set, and by closing the
server early to simulate RPC errors.
---
Nitpick comments:
In
`@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go`:
- Around line 29-35: The InboundProvisioningError struct exposes its Message
field unnecessarily; change the struct to use an unexported field (rename
Message to message) and keep the Error() method as-is to surface the message;
then update every instantiation of InboundProvisioningError to set the
unexported field using the lowercase name (e.g.
InboundProvisioningError{message: "..."}), leaving calls to Error() unchanged.
In `@pkg/clients/web/vault_client.go`:
- Around line 146-157: Update GetCredentialDirect to accept a context.Context
parameter (e.g., change signature to GetCredentialDirect(ctx context.Context,
cfg *config.AppConfig, logger commons.Logger, auth types.SimplePrinciple,
vaultId uint64)) and use that context when creating the outgoing metadata
context instead of context.Background() (replace
metadata.NewOutgoingContext(context.Background(), ...) with
metadata.NewOutgoingContext(ctx, ...)). Ensure the function uses the passed
context for the gRPC call so callers can set timeouts/cancellation (caller sites
should create a context with timeout and pass it in) and keep token creation via
types.CreateServiceScopeToken and conn lifecycle unchanged.
🪄 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: 07f7946f-c0e7-4af5-be1b-073b5a6d0831
📒 Files selected for processing (5)
api/assistant-api/api/assistant-deployment/create_assistant_phone_deployment.goapi/assistant-api/internal/services/assistant/assistant.deployment.impl.service.gopkg/clients/vobiz/client.gopkg/clients/vobiz/types.gopkg/clients/web/vault_client.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/clients/vobiz/types.go
- pkg/clients/vobiz/client.go
Comment on lines
+427
to
+570
| // maybeProvisionVobizInbound provisions Vobiz inbound routing when a vobiz_sip | ||
| // deployment enables inbound calls: it creates an origination URI pointing at | ||
| // this assistant-api's SIP server (cfg.SIPConfig.ExternalIP:Port), an inbound | ||
| // trunk referencing it, and assigns the DID. Returns extra telephony options | ||
| // (inbound_trunk_id / inbound_uri_id) to persist. Non-vobiz / non-inbound paths | ||
| // early-return (nil, nil), so they are byte-for-byte unchanged. | ||
| func (eService assistantDeploymentService) maybeProvisionVobizInbound( | ||
| ctx context.Context, auth types.SimplePrinciple, assistantId uint64, | ||
| phoneProvider string, opts []*protos.Metadata, | ||
| ) ([]*protos.Metadata, error) { | ||
| if phoneProvider != "vobiz_sip" { | ||
| return nil, nil | ||
| } | ||
| optMap := make(map[string]string, len(opts)) | ||
| for _, o := range opts { | ||
| optMap[o.GetKey()] = o.GetValue() | ||
| } | ||
| if optMap["rapida.sip_inbound"] != "true" { | ||
| return nil, nil | ||
| } | ||
|
|
||
| did := optMap["phone"] | ||
| if did == "" { | ||
| return nil, &InboundProvisioningError{Message: "a caller-ID phone number is required to enable inbound calls"} | ||
| } | ||
| credIDStr := optMap["rapida.credential_id"] | ||
| if credIDStr == "" { | ||
| return nil, &InboundProvisioningError{Message: "a Vobiz SIP credential is required to enable inbound calls"} | ||
| } | ||
| credentialID, err := strconv.ParseUint(credIDStr, 10, 64) | ||
| if err != nil { | ||
| return nil, &InboundProvisioningError{Message: "invalid Vobiz SIP credential"} | ||
| } | ||
|
|
||
| // Idempotency: reuse an existing inbound trunk for the same DID if a prior | ||
| // deployment already provisioned one (avoids duplicate Vobiz trunks on re-save). | ||
| if trunkID, uriID := eService.existingInboundTrunk(ctx, assistantId, did); trunkID != "" { | ||
| eService.logger.Infof("vobiz inbound: reusing existing trunk %s for DID %s", trunkID, did) | ||
| return inboundMetadata(trunkID, uriID), nil | ||
| } | ||
|
|
||
| if eService.cfg.SIPConfig == nil || eService.cfg.SIPConfig.ExternalIP == "" { | ||
| return nil, &InboundProvisioningError{Message: "the assistant SIP server has no external IP configured; cannot provision inbound"} | ||
| } | ||
|
|
||
| // Read the Vobiz account credentials via a redis-free, one-shot gRPC fetch. | ||
| cred, err := web_client.GetCredentialDirect(&eService.cfg.AppConfig, eService.logger, auth, credentialID) | ||
| if err != nil { | ||
| return nil, &InboundProvisioningError{Message: "could not read the Vobiz SIP credential"} | ||
| } | ||
| value := cred.GetValue().AsMap() | ||
| authID, _ := value["auth_id"].(string) | ||
| authToken, _ := value["auth_token"].(string) | ||
| if authID == "" || authToken == "" { | ||
| return nil, &InboundProvisioningError{Message: "the Vobiz SIP credential is missing auth_id/auth_token"} | ||
| } | ||
|
|
||
| sipAddr := fmt.Sprintf("%s:%d", eService.cfg.SIPConfig.ExternalIP, eService.cfg.SIPConfig.Port) | ||
| trunkID, uriID, err := eService.provisionVobizInbound(ctx, authID, authToken, sipAddr, did) | ||
| if err != nil { | ||
| return nil, err // already an *InboundProvisioningError | ||
| } | ||
| return inboundMetadata(trunkID, uriID), nil | ||
| } | ||
|
|
||
| // provisionVobizInbound performs the three Vobiz API calls and rolls back the | ||
| // trunk if number assignment fails. | ||
| func (eService assistantDeploymentService) provisionVobizInbound( | ||
| ctx context.Context, authID, authToken, sipAddr, did string, | ||
| ) (string, string, error) { | ||
| client := vobiz.NewClient() | ||
|
|
||
| uri, err := client.CreateOriginationURI(ctx, authID, authToken, vobiz.CreateOriginationURIRequest{ | ||
| URI: sipAddr, Transport: "udp", Priority: 1, Weight: 10, Enabled: true, | ||
| }) | ||
| if err != nil { | ||
| return "", "", &InboundProvisioningError{Message: fmt.Sprintf("failed to create the Vobiz origination URI: %v", err)} | ||
| } | ||
|
|
||
| trunk, err := client.CreateTrunk(ctx, authID, authToken, vobiz.CreateTrunkRequest{ | ||
| Name: fmt.Sprintf("rapida-inbound-%s", did), | ||
| TrunkStatus: "enabled", | ||
| TrunkDirection: "inbound", | ||
| Transport: "udp", | ||
| ConcurrentCallsLimit: 10, | ||
| CpsLimit: 2, | ||
| PrimaryURIUUID: uri.ID, | ||
| InboundDestination: uri.ID, | ||
| }) | ||
| if err != nil { | ||
| return "", "", &InboundProvisioningError{Message: fmt.Sprintf("failed to create the Vobiz inbound trunk: %v", err)} | ||
| } | ||
|
|
||
| if err := client.AssignNumber(ctx, authID, authToken, did, trunk.TrunkID); err != nil { | ||
| // Roll back the just-created trunk; surface a user-friendly message. | ||
| if delErr := client.DeleteTrunk(ctx, authID, authToken, trunk.TrunkID); delErr != nil { | ||
| eService.logger.Errorf("vobiz inbound: failed to roll back trunk %s: %v", trunk.TrunkID, delErr) | ||
| } | ||
| return "", "", vobizAssignError(did, err) | ||
| } | ||
| return trunk.TrunkID, uri.ID, nil | ||
| } | ||
|
|
||
| // existingInboundTrunk returns the inbound trunk/URI ids already provisioned for | ||
| // the given DID on an active vobiz_sip deployment of this assistant, if any. | ||
| func (eService assistantDeploymentService) existingInboundTrunk(ctx context.Context, assistantId uint64, did string) (string, string) { | ||
| var deployments []internal_assistant_entity.AssistantPhoneDeployment | ||
| if err := eService.postgres.DB(ctx).Preload("TelephonyOption"). | ||
| Where("assistant_id = ? AND telephony_provider = ? AND status = ?", assistantId, "vobiz_sip", type_enums.RECORD_ACTIVE). | ||
| Find(&deployments).Error; err != nil { | ||
| return "", "" | ||
| } | ||
| for _, dep := range deployments { | ||
| o := dep.GetOptions() | ||
| phone, _ := o.GetString("phone") | ||
| trunkID, _ := o.GetString("rapida.inbound_trunk_id") | ||
| uriID, _ := o.GetString("rapida.inbound_uri_id") | ||
| if phone == did && trunkID != "" { | ||
| return trunkID, uriID | ||
| } | ||
| } | ||
| return "", "" | ||
| } | ||
|
|
||
| func inboundMetadata(trunkID, uriID string) []*protos.Metadata { | ||
| return []*protos.Metadata{ | ||
| {Key: "rapida.inbound_trunk_id", Value: trunkID}, | ||
| {Key: "rapida.inbound_uri_id", Value: uriID}, | ||
| } | ||
| } | ||
|
|
||
| // vobizAssignError maps a Vobiz number-assignment failure to a clear UI message. | ||
| func vobizAssignError(did string, err error) error { | ||
| var apiErr *vobiz.VobizAPIError | ||
| if errors.As(err, &apiErr) { | ||
| switch apiErr.StatusCode { | ||
| case 404: | ||
| return &InboundProvisioningError{Message: fmt.Sprintf("phone number %s is not in your Vobiz account", did)} | ||
| case 400, 409: | ||
| return &InboundProvisioningError{Message: fmt.Sprintf("phone number %s is already attached to another Vobiz trunk — unlink it first", did)} | ||
| } | ||
| } | ||
| return &InboundProvisioningError{Message: fmt.Sprintf("failed to attach %s to the Vobiz inbound trunk: %v", did, err)} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add test coverage for the new provisioning logic.
This file adds substantial new functionality (inbound provisioning workflow, error handling, idempotency, rollback logic) but no corresponding test file is included.
As per coding guidelines, backend changes in api/**, pkg/**, or cmd/** must include corresponding *_test.go updates in the same package.
Consider adding unit tests for:
maybeProvisionVobizInbound: validation logic, early returns, error pathsprovisionVobizInbound: API call sequencing, rollback on failureexistingInboundTrunk: idempotency query logicvobizAssignError: error mapping for different status codesInboundProvisioningError: error type behavior
🤖 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
`@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go`
around lines 427 - 570, Add a new *_test.go alongside
assistant.deployment.impl.service.go that provides unit tests for the new
provisioning logic: write table-driven tests for maybeProvisionVobizInbound
covering provider check, rapida.sip_inbound flag, missing phone/credential
parsing errors, and successful path (mock web_client.GetCredentialDirect to
return creds); unit tests for provisionVobizInbound that mock vobiz.Client
(CreateOriginationURI, CreateTrunk, AssignNumber, DeleteTrunk) to assert call
ordering, successful return, and rollback when AssignNumber fails; tests for
existingInboundTrunk that seed a fake DB/ORM with AssistantPhoneDeployment
entries to verify idempotency lookup; tests for vobizAssignError that construct
vobiz.VobizAPIError with different StatusCode values to assert mapped messages;
and a small test asserting InboundProvisioningError values; use dependency
injection/mocks for eService.provisionVobizInbound and vobiz client, and
table-driven subtests for clarity.
Source: Coding guidelines
Comment on lines
+140
to
+170
| // GetCredentialDirect fetches a vault credential via a one-shot gRPC call to the | ||
| // vault service WITHOUT the redis-backed caching client. The internal | ||
| // service-scope auth token is a stateless JWT (no redis), so callers that don't | ||
| // hold a RedisConnector (e.g. the deployment service) can still read a | ||
| // credential. Intended for infrequent reads; it opens and closes its own | ||
| // connection per call. | ||
| func GetCredentialDirect(cfg *config.AppConfig, logger commons.Logger, auth types.SimplePrinciple, vaultId uint64) (*vault_api.VaultCredential, error) { | ||
| conn, err := grpc.NewClient(cfg.Web.Host, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("vault: unable to connect to web-api: %w", err) | ||
| } | ||
| defer conn.Close() | ||
|
|
||
| token, err := types.CreateServiceScopeToken(auth, cfg.Secret) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("vault: unable to create service token: %w", err) | ||
| } | ||
| ctx := metadata.NewOutgoingContext(context.Background(), metadata.New(map[string]string{types.SERVICE_SCOPE_KEY: token})) | ||
|
|
||
| res, err := vault_api.NewVaultServiceClient(conn).GetCredential(ctx, &vault_api.GetCredentialRequest{VaultId: vaultId}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("vault: GetCredential failed: %w", err) | ||
| } | ||
| if res.GetSuccess() && res.GetData() != nil { | ||
| return res.GetData(), nil | ||
| } | ||
| if res.GetError() != nil { | ||
| return nil, fmt.Errorf("vault: %s", res.GetError().HumanMessage) | ||
| } | ||
| return nil, errors.New("vault: credential not found") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for existing tests covering GetCredentialDirect in this package
# Search for test files in the same package
fd -e go -p 'test' pkg/clients/web/
# Search for any tests referencing GetCredentialDirect
rg -n 'GetCredentialDirect' --type go -g '*_test.go'Repository: rapidaai/voice-ai
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== *_test.go under pkg/clients/web =="
find pkg/clients/web -maxdepth 5 -type f -name '*_test.go' | sort
echo
echo "== References to GetCredentialDirect in tests =="
rg -n --type go -g '*_test.go' 'GetCredentialDirect' pkg/clients/web || true
echo
echo "== References to GetCredentialDirect in non-tests (sanity) =="
rg -n --type go 'GetCredentialDirect' pkg/clients/web || true
echo
echo "== Confirm file under review exists =="
ls -l pkg/clients/web/vault_client.goRepository: rapidaai/voice-ai
Length of output: 640
Add unit tests for GetCredentialDirect in pkg/clients/web
No *_test.go files (and no GetCredentialDirect references) exist under pkg/clients/web, so success/error/not-found behavior for the exported helper isn’t covered. Add a *_test.go in the same package with unit tests for these scenarios.
🤖 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 `@pkg/clients/web/vault_client.go` around lines 140 - 170, Add a new test file
in the same package that exercises GetCredentialDirect and covers (1) success
returning a VaultCredential, (2) service-level error returned via
res.Error.HumanMessage, and (3) RPC-level failure (e.g. client connection
error); implement each test by starting a local gRPC server that registers a
fake implementation of vault_api.VaultServiceServer which inspects the incoming
metadata and responds to GetCredentialRequest accordingly, then call
GetCredentialDirect (which uses grpc.NewClient and
vault_api.NewVaultServiceClient) and assert the returned value/error matches the
scenario; ensure the tests build/run without external services by using an
in-process grpc.Server with insecure creds and distinct handlers that return a
populated GetCredentialResponse, a response with Error set, and by closing the
server early to simulate RPC errors.
Source: Coding guidelines
- InboundProvisioningError now carries its underlying cause (Unwrap) so error chains are preserved; constructors pass the cause and the handler logs it. - Log (instead of silently swallowing) a failed idempotency lookup so a DB error can't masquerade as 'no existing trunk' and cause duplicate provisioning. - Harden the credential type assertions (check the assertion ok flags). - Add Vobiz client tests for CreateOriginationURI and AssignNumber (incl. the already-attached error path).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go (1)
352-356:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake Vobiz provisioning and deployment persistence atomic.
maybeProvisionVobizInboundcan fully create/attach the inbound trunk beforearchiveDeploymentRecords,db.Create(deployment), and the telephony-option insert run. If any of those DB steps fail, the request returns an error but the DID stays provisioned in Vobiz with no persisted deployment/metadata to rediscover on retry. That leaves orphaned external state and makes the next attempt non-idempotent.This needs one compensating strategy across the whole flow: either persist inside a DB transaction and clean up Vobiz resources on rollback, or switch to a pending-state/outbox flow so external side effects happen only after durable local state exists.
Also applies to: 379-425
🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 352 - 356, The Vobiz provisioning call in maybeProvisionVobizInbound currently runs before persisting deployment state, risking orphaned external resources; change the flow so external provisioning is only performed after durable DB state or is wrapped in a transaction with compensating cleanup: either (A) create the deployment record and insert telephony options (archiveDeploymentRecords, db.Create(deployment), and the telephony-option insert) inside a DB transaction first, then call maybeProvisionVobizInbound and on failure rollback/cleanup the transaction and call a Vobiz cleanup helper to deprovision; or (B) adopt an outbox/pending-state pattern where you persist a pending deployment row and outbox event, commit, and only then run maybeProvisionVobizInbound asynchronously; update caller logic where maybeProvisionVobizInbound is invoked (also for the similar block around lines handling 379-425) to use one of these approaches and add a Vobiz deprovision helper to guarantee cleanup on rollback/failure.
♻️ Duplicate comments (1)
api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go (1)
539-545:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t continue provisioning when the idempotency lookup fails.
Logging helps, but returning
("", "")still turns a DB failure into “no existing trunk”, so the caller provisions a new Vobiz trunk/assignment on the next line. That preserves the duplicate-provisioning bug this lookup is meant to prevent. Bubble the query error up and abort the create instead.🤖 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 `@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go` around lines 539 - 545, The idempotency DB lookup failure is being swallowed by returning ("", ""), which forces provisioning to proceed; modify the handler that calls eService.postgres.DB(...).Preload("TelephonyOption").Where(...).Find(&deployments).Error so that on err you return/propagate the error instead of returning empty strings—i.e. stop returning ("", "") after logger.Errorf and instead return an error (wrap err with context mentioning assistantId and did), update the method signature (and its callers) to return an error (e.g. (string,string,error) or propagate via existing error path), and ensure the caller aborts create when this query fails; keep the logger.Errorf call but do not convert DB errors into “no existing trunk”.
🤖 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 `@pkg/clients/vobiz/client_test.go`:
- Around line 138-140: The test is asserting the decoded server path via
r.URL.Path so it doesn't verify that the DID was percent-encoded on the wire;
change the assertion in the test (the check around r.URL.Path in the test
function) to check r.RequestURI or r.URL.RawPath and assert that it contains
"%2B" (e.g., verify "/api/v1/Account/AUTH_ID/numbers/%2B919262171438/assign")
instead of using r.URL.Path.
---
Outside diff comments:
In
`@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go`:
- Around line 352-356: The Vobiz provisioning call in maybeProvisionVobizInbound
currently runs before persisting deployment state, risking orphaned external
resources; change the flow so external provisioning is only performed after
durable DB state or is wrapped in a transaction with compensating cleanup:
either (A) create the deployment record and insert telephony options
(archiveDeploymentRecords, db.Create(deployment), and the telephony-option
insert) inside a DB transaction first, then call maybeProvisionVobizInbound and
on failure rollback/cleanup the transaction and call a Vobiz cleanup helper to
deprovision; or (B) adopt an outbox/pending-state pattern where you persist a
pending deployment row and outbox event, commit, and only then run
maybeProvisionVobizInbound asynchronously; update caller logic where
maybeProvisionVobizInbound is invoked (also for the similar block around lines
handling 379-425) to use one of these approaches and add a Vobiz deprovision
helper to guarantee cleanup on rollback/failure.
---
Duplicate comments:
In
`@api/assistant-api/internal/services/assistant/assistant.deployment.impl.service.go`:
- Around line 539-545: The idempotency DB lookup failure is being swallowed by
returning ("", ""), which forces provisioning to proceed; modify the handler
that calls
eService.postgres.DB(...).Preload("TelephonyOption").Where(...).Find(&deployments).Error
so that on err you return/propagate the error instead of returning empty
strings—i.e. stop returning ("", "") after logger.Errorf and instead return an
error (wrap err with context mentioning assistantId and did), update the method
signature (and its callers) to return an error (e.g. (string,string,error) or
propagate via existing error path), and ensure the caller aborts create when
this query fails; keep the logger.Errorf call but do not convert DB errors into
“no existing trunk”.
🪄 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: 15132a85-b4e8-459e-b517-bb8219040b25
📒 Files selected for processing (3)
api/assistant-api/api/assistant-deployment/create_assistant_phone_deployment.goapi/assistant-api/internal/services/assistant/assistant.deployment.impl.service.gopkg/clients/vobiz/client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/assistant-api/api/assistant-deployment/create_assistant_phone_deployment.go
iamprashant
requested changes
Jun 15, 2026
iamprashant
left a comment
iamprashant
left a comment
Member
There was a problem hiding this comment.
@Piyush-sahoo Thanks for submitting PR.
If adding a branded telephony, we only allow websocket other channels are very generic so any provider can be used.
- Changes should only be in telephony/internal/{provider}**
a. Implement how call will be handled
b. Transfer / end call / Status Callback - Update in Factory to have new provider
- In UI update json only a provider taged as Telephony and credential needed to connect.
You can use codex/claude skill for integrating web-socket based telephony.
Address maintainer review on rapidaai#141: branded telephony providers are WebSocket-only. Remove the vobiz_sip auto-provisioning path (trunk / origination-URI / number-assignment via the Vobiz API, vault and SIP-engine wiring) and keep only the vobiz_websocket provider, its factory registration, and the UI provider config. - drop VobizSIP factory entry; revert SIP-engine + outbound changes - remove web-api vault provisioning + deployment auto-provision service - trim pkg/clients/vobiz to the MakeCall surface the WS provider needs - remove vobiz_sip from the UI provider registry
Move the Vobiz REST client (MakeCall) from pkg/clients/vobiz into
telephony/internal/vobiz/internal so the provider is self-contained like
twilio/telnyx — no shared pkg dependency, uses net/http directly. Keeps
all provider code within telephony/internal/{provider} per review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Related Issues
Fixes #
Checklist
General
Testing
Documentation
Security
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests