Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
stampercasey
left a comment
There was a problem hiding this comment.
Review from Claude Code — see inline comments for individual findings. Blockers are all in ReferCompleteCallback; the BXML verb itself (Refer.cs) is in good shape.
stampercasey
left a comment
There was a problem hiding this comment.
Review from Claude Code — see inline comments for individual findings. Blockers are all in ReferCompleteCallback; the BXML verb itself (Refer.cs) is in good shape.
| /// <param name="cause">Reason the call failed - hangup, busy, timeout, cancel, rejected, callback-error, invalid-bxml, application-error, account-limit, node-capacity-exceeded, error, or unknown..</param> | ||
| /// <param name="errorMessage">Text explaining the reason that caused the call to fail in case of errors..</param> | ||
| /// <param name="errorId">Bandwidth's internal id that references the error event..</param> | ||
| public ReferCompleteCallback(string eventType = default(string), DateTime eventTime = default(DateTime), string accountId = default(string), string applicationId = default(string), string from = default(string), string to = default(string), CallDirectionEnum? direction = default(CallDirectionEnum?), string callId = default(string), string callUrl = default(string), DateTime? enqueuedTime = default(DateTime?), DateTime startTime = default(DateTime), DateTime? answerTime = default(DateTime?), string tag = default(string), int? sipResponseCode = default(int?), string cause = default(string), string errorMessage = default(string), string errorId = default(string)) |
There was a problem hiding this comment.
[BLOCKER] Wrong/missing REFER-specific callback fields
This constructor signature is missing three fields that api-specs#2142 and the published docs define for this event, and includes fields that do not belong here.
Missing fields:
referCallStatus(string:"success"or"failure") — the primary outcome indicator; consumers cannot determine call outcome without itreferSipResponseCode(int, optional) — SIP response code for the REFER requestnotifySipResponseCode(int, optional) — final SIP code reported via NOTIFY
Present but not in spec (appear copied from another callback):
sipResponseCode→ should bereferSipResponseCodecause→ not a field in this callbackerrorMessage→ not a field in this callbackerrorId→ not a field in this callback
Ref: https://github.com/Bandwidth/api-specs/pull/2142 — see the referComplete.mdx payload table.
| /// <value>The SIP response code from the REFER request, indicating the outcome of the operation (e.g., 200 for success, 404 for not found).</value> | ||
| /// <example>200</example> | ||
| [DataMember(Name = "sipResponseCode", EmitDefaultValue = true)] | ||
| public int? SipResponseCode { get; set; } |
There was a problem hiding this comment.
[BLOCKER] Field name mismatch — spec uses referSipResponseCode, not sipResponseCode
The [DataMember(Name = "sipResponseCode")] attribute will deserialize the wrong JSON key, so this field will always be null on a real server callback.
// Should be:
[DataMember(Name = "referSipResponseCode", EmitDefaultValue = true)]
public int? ReferSipResponseCode { get; set; }| /// <value>Reason the call failed - hangup, busy, timeout, cancel, rejected, callback-error, invalid-bxml, application-error, account-limit, node-capacity-exceeded, error, or unknown.</value> | ||
| /// <example>busy</example> | ||
| [DataMember(Name = "cause", EmitDefaultValue = false)] | ||
| public string Cause { get; set; } |
There was a problem hiding this comment.
[BLOCKER] cause is not a field in the referComplete callback
Copied from another callback model (disconnect / transcriptionAvailable). Not present in the referComplete spec. The outcome field here is referCallStatus ("success" or "failure"), which is missing from this class entirely.
| /// <value>Text explaining the reason that caused the call to fail in case of errors.</value> | ||
| /// <example>Call c-2a913f94-6a486f3a-3cae-4034-bcc3-f0c9fa77ca2f is already bridged with another call</example> | ||
| [DataMember(Name = "errorMessage", EmitDefaultValue = true)] | ||
| public string ErrorMessage { get; set; } |
There was a problem hiding this comment.
[BLOCKER] errorMessage is not a field in the referComplete callback
Not present in the spec for this event. Remove alongside cause and errorId.
| /// <value>Bandwidth's internal id that references the error event.</value> | ||
| /// <example>4642074b-7b58-478b-96e4-3a60955c6765</example> | ||
| [DataMember(Name = "errorId", EmitDefaultValue = true)] | ||
| public string ErrorId { get; set; } |
There was a problem hiding this comment.
[BLOCKER] errorId is not a field in the referComplete callback
Not present in the spec for this event. Remove alongside cause and errorMessage.
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="ReferCompleteCallback" /> class. | ||
| /// </summary> | ||
| /// <param name="eventType">The event type, value can be one of the following: answer, bridgeComplete, bridgeTargetComplete, conferenceCreated, conferenceRedirect, conferenceMemberJoin, conferenceMemberExit, conferenceCompleted, conferenceRecordingAvailable, disconnect, dtmf, gather, initiate, machineDetectionComplete, recordingComplete, recordingAvailable, redirect, transcriptionAvailable, transferAnswer, transferComplete, transferDisconnect..</param> |
There was a problem hiding this comment.
[MINOR] eventType description does not list referComplete
The doc string enumerates event type values (bridgeComplete, conferenceCreated, etc.) but omits referComplete. Since this class is specific to one event, the description should say "Value is referComplete" or at minimum add referComplete to the list.
| /// <summary> | ||
| /// BXML tag to represent a SIP URI for the refer verb. | ||
| /// </summary> | ||
| public class SipUri : IVerb |
There was a problem hiding this comment.
[MINOR] SipUri nested class implements IVerb
SipUri is a child element, not a verb. Transfer.cs also has a SipUri type (public SipUri[] SipUris). Two questions before merge:
- Should
Referreuse that existingSipUritype instead of defining its own nested class? - If the nested class is intentional, should it implement
IVerbor a more appropriate interface?
| /// </summary> | ||
| public Refer() | ||
| { | ||
| ReferCompleteMethod = "POST"; |
There was a problem hiding this comment.
[MINOR] Default "POST" is always emitted in output XML
Setting ReferCompleteMethod = "POST" in the constructor means the attribute is always serialized, even when the user never sets it. Transfer leaves TransferCompleteMethod null and lets the server apply its default. Not wrong, but a deliberate divergence from the Transfer pattern — worth a comment explaining the intent if this is by design.
| ------------ | ------------- | ------------- | ------------- | ||
| **ReferCompleteUrl** | **string** | URL to receive the `referComplete` callback when the REFER is finished. | [optional] | ||
| **ReferCompleteMethod** | **string** | HTTP method to use for the `referComplete` callback. Must be `GET` or `POST`. | [optional] [default to `POST`] | ||
| **Tag** | **string** | Optional custom string to include in callbacks. Max 4096 characters. | [optional] |
There was a problem hiding this comment.
[MINOR] tag max-length mismatch: this doc says 4096, spec says 256
The published BXML reference states the tag attribute max is 256 characters. Please align before merge.
| var bxml = new Response(refer).ToBXML(); | ||
| Assert.Contains("sip:bob@biloxi.example.com", bxml); | ||
| Assert.Contains("referCompleteMethod=\"POST\"", bxml); | ||
| } |
There was a problem hiding this comment.
[MINOR] No test for missing (required) <SipUri> enforcement
<SipUri> is required per spec. What does new Response(new Refer()).ToBXML() produce today — invalid XML, silent omission, or an exception? Consider adding a test (and enforcement in ToBXML() or a guard in WithSipUri) to surface the constraint clearly.
Also missing: a test for the WithSipUri(SipUri) object-overload confirming sip: validation fires via that path.
Hold as Draft until VAPI-2916 / api-specs#2142 lands.