diff --git a/AGENTS.md b/AGENTS.md index 823bd71..30bb0fb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -97,11 +97,12 @@ by ear. There is no scripted end-to-end call check. Translation is a fixed rulebook (`src/matrix/twilio-voice.json`), not a guess. - **Supported:** most core voice verbs — Say, Play, Gather, Pause, Hangup, - Redirect, Record, Number, Sip (translate cleanly to BXML). A few of these - still drop attributes safely rather than silently: `Say`/`Play`'s - `loop="0"` (infinite) can't be expressed inline in BXML, so it emits once and - warns; `Record`'s `transcribe` and `playBeep` attributes have no direct BXML - equivalent. + Redirect, Record, Number, Sip (each maps to a matching BXML verb). A few of + these still drop or adjust attributes safely rather than silently: + `Say`/`Play`'s `loop="0"` (infinite) can't be expressed inline in BXML, so it + emits once and warns; `Record`'s `transcribe`, `playBeep`, and `trim` + attributes have no direct BXML equivalent, and its `finishOnKey` can only + hold one character where Twilio allows a set (see below). - **Lossy / partial (feature loss — needs a human decision):** - `Reject` — maps to `Hangup`, but Bandwidth answers before hanging up, so the caller may be billed for a short call. @@ -122,10 +123,54 @@ Translation is a fixed rulebook (`src/matrix/twilio-voice.json`), not a guess. TwiML document and re-translates the verbs after that Gather. Set `actionOnEmptyResult="true"` to request the action with `Digits=""` instead. On a translator instance that has not seen the document (restart, second - replica) the action is requested as a fallback. + replica) a Gather **with** an `action` falls back to requesting it anyway. + A Gather with **no** `action` attribute re-requests the current document, + as on Twilio — but the same no-document fallback can't do that for a + document it never cached, so it ends the call instead, rather than + re-fetching and replaying that same document forever. **In a multi-replica + deployment with no session affinity routing a call's `/bw/continue` + requests back to the replica that served its last document, this means a + silent caller on a bare Gather can have their call ended instead of + continuing to the next verb — a real behavior difference from Twilio, + worth fixing with a shared (not per-process) document cache if it matters + for your deployment.** `finishOnKey`'s + Twilio default (`"#"`) is emitted explicitly, since Bandwidth's own default + for this attribute is `""` (disabled) — a real difference. `timeout` + (default 5) sets both `firstDigitTimeout` and `interDigitTimeout`; the + *omitted* case is a no-op either way, since Bandwidth already defaults both + to 5s on its own, but an *explicit* `timeout` previously only reached + `firstDigitTimeout`, silently leaving `interDigitTimeout` at Bandwidth's + default regardless of what was set. `numDigits`, `timeout`, and `maxLength` + (on `Record`, below) are kept within the ranges Bandwidth documents, with a + warning naming the original value if it had to change (checked, and + warned about, once per Twilio attribute even where it maps to more than + one BXML attribute); a value that isn't a plain number at all falls back + to Twilio's own default instead of being passed through for Bandwidth to + reject, and is never emitted in scientific notation. `numDigits` + specifically is also rounded to a whole number, since there's no such + thing as collecting half a digit — `timeout`/`maxLength` keep a + fractional value as written, since a fractional duration is a real, + normal concept Bandwidth's own docs allow. - `Record` — the `action` receives `RecordingUrl`, `RecordingSid`, and `RecordingDuration` from Bandwidth's `recordComplete`; Bandwidth's event has - no equivalent of Twilio's `Digits` (the key that stopped the recording). + no equivalent of Twilio's `Digits` (the key that stopped the recording). A + Record with no `action` attribute re-requests the current document, as on + Twilio. `maxLength` (default 3600s) and `timeout` (Twilio: silence ends the + recording, default 5s, mapped to Bandwidth's `silenceTimeout`, which + previously had no BXML equivalent at all) are emitted explicitly rather + than left to Bandwidth's own (lower or disabled) defaults, and kept within + Bandwidth's documented ranges like Gather's `numDigits`/`timeout` above. + `finishOnKey` is narrowed to Bandwidth's single terminating digit + (preferring `#`, then `*`, then the first valid character present), since + Bandwidth cannot represent Twilio's default set of any key — a value made + of nothing but one valid character repeated (e.g. `"##"`) isn't actually + narrowed to anything different, so that doesn't warn. A value with no + digit, `*`, or `#` at all turns the stop key off (`terminatingDigits=""`) + rather than falling back to Bandwidth's own default (`"#"`), since that + would give the customer a stop key they never wrote into their TwiML. + `playBeep` and `trim` warn unless set to the one value (`"false"` / + `"do-not-trim"`) where Bandwidth's fixed behavior (no beep, never trims) + happens to match Twilio's. - `Start` — the `Transcription` noun maps to `StartTranscription`; the `Siprec` and `VirtualAgent` nouns are unsupported. - `Refer` — Bandwidth only honors `Refer` on inbound SIP URI calls, so a PSTN diff --git a/src/compatibility-check/analyze.ts b/src/compatibility-check/analyze.ts index 2210e82..ed4a5b7 100644 --- a/src/compatibility-check/analyze.ts +++ b/src/compatibility-check/analyze.ts @@ -1,5 +1,5 @@ import { loadMatrix } from "../matrix/load.js"; -import { translateTwiml, type Finding } from "../translator/translate.js"; +import { translateTwiml, dedupeFindings, type Finding } from "../translator/translate.js"; const matrix = loadMatrix(); @@ -68,6 +68,5 @@ export function analyzeSource(file: string, source: string): FileAnalysis { } } - const dedup = new Map(findings.map((f) => [`${f.severity}:${f.verb}:${f.message}`, f])); - return { file, verbs: [...verbs], findings: [...dedup.values()], sdkDetected }; + return { file, verbs: [...verbs], findings: dedupeFindings(findings), sdkDetected }; } diff --git a/src/matrix/twilio-voice.json b/src/matrix/twilio-voice.json index 4b7ee9a..531c0a5 100644 --- a/src/matrix/twilio-voice.json +++ b/src/matrix/twilio-voice.json @@ -32,11 +32,11 @@ "action": { "bxml": "gatherUrl", "status": "supported", - "notes": "No digits or speech does not request the action; the verbs after the Gather run instead (Twilio semantics). actionOnEmptyResult=\"true\" requests the action with Digits=\"\"." + "notes": "No digits or speech does not request the action; the verbs after the Gather run instead (Twilio semantics). actionOnEmptyResult=\"true\" requests the action with Digits=\"\". A Gather with no action attribute re-requests the current document, matching Twilio, when the server knows the document's URL (standalone BXML generation keeps the original warning instead)." }, - "numDigits": { "bxml": "maxDigits", "status": "supported" }, - "timeout": { "bxml": "firstDigitTimeout", "status": "supported" }, - "finishOnKey": { "bxml": "terminatingDigits", "status": "supported" }, + "numDigits": { "bxml": "maxDigits", "status": "supported", "notes": "Kept within Bandwidth's 1-50 range, with a warning naming the original value if it had to change; left unset (Bandwidth's own default of 50) when omitted." }, + "timeout": { "bxml": "firstDigitTimeout / interDigitTimeout", "status": "supported", "notes": "Twilio's single timeout (default 5s) sets both Bandwidth attributes, since Bandwidth splits the pause-before-first-digit and gap-between-digits behavior Twilio combines into one value. Each is kept within its own range (firstDigitTimeout 0-60, interDigitTimeout 1-60), with a warning naming the original value if it had to change." }, + "finishOnKey": { "bxml": "terminatingDigits", "status": "supported", "notes": "Twilio's default (\"#\") is emitted explicitly when omitted or whitespace-only, since Bandwidth's own default for this attribute is \"\" (disabled). finishOnKey=\"\" passes through unchanged (both systems treat it as disabled). A character Bandwidth doesn't recognize as a key (only 0-9, *, and # work) is dropped from the set, with a warning." }, "input": { "bxml": "input", "status": "supported", "notes": "Twilio dtmf / speech / 'dtmf speech' map to BW input dtmf / speech / dtmf_speech. Speech results arrive as the BW gather event's text field, surfaced to the customer app as Twilio's SpeechResult param." } } }, @@ -79,13 +79,15 @@ "action": { "bxml": "recordCompleteUrl", "status": "supported", - "notes": "The callback carries RecordingUrl, RecordingSid, RecordingDuration. Bandwidth's event has no equivalent of Twilio's Digits." + "notes": "The callback carries RecordingUrl, RecordingSid, RecordingDuration. Bandwidth's event has no equivalent of Twilio's Digits. A Record with no action attribute re-requests the current document, matching Twilio, when the server knows the document's URL (standalone BXML generation keeps the original warning instead)." }, "recordingStatusCallback": { "bxml": "recordingAvailableUrl", "status": "supported", "notes": "Async 'recording ready' webhook. Translator receives the BW recordingAvailableUrl event, reshapes it into Twilio recordingStatusCallback params, and forwards it; RecordingUrl points back at the translator's recording facade. recordingStatusCallbackMethod=GET is not honored (forwarded via POST)." }, - "maxLength": { "bxml": "maxDuration", "status": "supported" }, - "finishOnKey": { "bxml": "terminatingDigits", "status": "supported" }, + "maxLength": { "bxml": "maxDuration", "status": "supported", "notes": "Twilio's default (3600s) is emitted explicitly when omitted, since Bandwidth's own default is 60s. Limited to Bandwidth's 10800s (3h) maximum, with a warning naming the original value if it was higher." }, + "timeout": { "bxml": "silenceTimeout", "status": "supported", "notes": "Not previously mapped at all, so a recording never stopped on silence; now defaults to Twilio's 5 seconds when omitted and is limited to Bandwidth's 10800s maximum. timeout=\"0\" (Twilio: disable) and silenceTimeout=\"0\" (Bandwidth: no timeout) agree, so it passes through unchanged." }, + "finishOnKey": { "bxml": "terminatingDigits", "status": "partial", "notes": "Bandwidth's Record takes one terminating digit (0-9, *, or #); Twilio's finishOnKey is a set (default \"1234567890*#\", any key). A single valid character passes through unchanged; a value made of several distinct valid characters, or any invalid ones mixed in (including the omitted or whitespace-only default), is narrowed to one digit, preferring '#' then '*' then the first valid character present, with a warning. A value with no valid character at all (e.g. a letter) turns the stop key off (terminatingDigits=\"\") rather than falling back to Bandwidth's own default (\"#\"), since that would give the customer a stop key they never wrote into their TwiML." }, "transcribe": { "bxml": "transcribe", "status": "partial", "notes": "Engine and callback shape differ." }, - "playBeep": { "bxml": null, "status": "partial", "notes": "No direct equivalent; translator can prepend a beep PlayAudio later." } + "playBeep": { "bxml": null, "status": "partial", "notes": "No direct equivalent; translator can prepend a beep PlayAudio later. Twilio's default is true (plays a beep), so a warning is emitted unless playBeep is explicitly \"false\" (the one value Bandwidth's no-beep behavior matches)." }, + "trim": { "bxml": null, "status": "partial", "notes": "No equivalent; Bandwidth never trims silence. Twilio's default is trim-silence, so a warning is emitted unless trim is explicitly \"do-not-trim\" (the one value Bandwidth's never-trims behavior matches)." } } }, "Dial": { diff --git a/src/server/app.ts b/src/server/app.ts index e2ee826..f1d2ba2 100644 --- a/src/server/app.ts +++ b/src/server/app.ts @@ -186,7 +186,13 @@ export function buildApp(config: ServerConfig, deps: ServerDeps): FastifyInstanc // plus the flag that says the customer wants the action even then. if (kind === "action" && ctx?.gatherIndex !== undefined) { const onEmpty = ctx.actionOnEmptyResult ? "&onEmpty=1" : ""; - return `${config.publicBaseUrl}/bw/continue?next=${encodeURIComponent(absolute)}&gather=${ctx.gatherIndex}${onEmpty}`; + // Marks a Gather with no action (this URL points back at the document + // that served it, not a real customer action) so /bw/continue can tell + // the two apart if it ever needs to resume this document and can't: + // falling back to "request the action anyway" is reasonable for a real + // action, but would just replay this same prompt forever for a self-reference. + const self = ctx.selfReference ? "&self=1" : ""; + return `${config.publicBaseUrl}/bw/continue?next=${encodeURIComponent(absolute)}&gather=${ctx.gatherIndex}${onEmpty}${self}`; } // A Dial's action names the Dial (matching its legs' transferDisconnectUrl) // and how many legs it rings, so /bw/continue joins only that Dial's legs. @@ -331,6 +337,7 @@ export function buildApp(config: ServerConfig, deps: ServerDeps): FastifyInstanc rewriteUrl: rewriter(documentUrl, doc), callbackAuth: { username: config.webhookUser, password: config.webhookPassword }, transferLegUrl: (dialIndex) => `${config.publicBaseUrl}/bw/transfer-leg?dial=${doc}.${dialIndex}`, + documentUrl, ...extra, }); if (result.hasErrors) { @@ -451,7 +458,14 @@ export function buildApp(config: ServerConfig, deps: ServerDeps): FastifyInstanc app.post("/bw/continue", async (req, reply) => { const event = req.body as BwEvent; if (!event || !isSafeBwId(event.callId)) return reply.code(400).send(serverErrors.invalidParam("callId")); - const query = req.query as { next?: string; gather?: string; onEmpty?: string; dial?: string; legs?: string }; + const query = req.query as { + next?: string; + gather?: string; + onEmpty?: string; + dial?: string; + legs?: string; + self?: string; + }; if (!query.next) return reply.code(400).send(serverErrors.missingParam("next")); const record = store.get(event.callId) ?? @@ -477,8 +491,21 @@ export function buildApp(config: ServerConfig, deps: ServerDeps): FastifyInstanc resumeAfterGather: gatherIndex, }); } - // No document to resume (fresh instance, or an old-style URL): the only - // way to keep the call alive is to ask the action anyway. + // No document to resume (fresh instance, second replica, or an + // old-style URL). For a real customer action, asking it anyway is a + // reasonable fallback. For a Gather with no action -- this URL + // points back at the document that served it, not a real action -- + // asking "the action" would re-fetch and replay that exact same + // document, and since nothing about the situation changes, it would + // keep replaying forever. End the call instead, the way Twilio ends + // a call that runs out of TwiML to execute. + if (query.self === "1") { + app.log.warn( + { callId: event.callId }, + "empty gather on a Gather with no action, with no document to resume; ending the call rather than replaying it", + ); + return reply.type("application/xml").send(bxmlDocument([])); + } app.log.warn({ callId: event.callId }, "empty gather with no document to resume; requesting action"); } params = gatherParams(record, config.accountSid, { diff --git a/src/translator/translate.ts b/src/translator/translate.ts index 6698dee..53f3ce5 100644 --- a/src/translator/translate.ts +++ b/src/translator/translate.ts @@ -22,6 +22,15 @@ export interface RewriteContext { dialIndex?: number; /** How many targets (Number/Sip) this rings, i.e. how many legs to expect. */ dialTargets?: number; + /** This URL was generated because the verb had no action attribute and the + * server fell back to the document's own URL (see TranslateOptions.documentUrl), + * not because the customer wrote this URL themselves. Lets the server tell + * "re-request the customer's real action" apart from "re-request myself", + * which matters when there is no cached document to resume after a Gather + * with no input: re-requesting a real action is a reasonable fallback, but + * re-requesting the document that produced this same Gather would just + * replay the same prompt forever. */ + selfReference?: boolean; } export type RewriteUrl = (url: string, kind: UrlKind, ctx?: RewriteContext) => string; @@ -42,6 +51,13 @@ export interface TranslateOptions { * to resume a document when a Gather ends with no input: Twilio continues * with the following verbs, Bandwidth expects fresh BXML from the gatherUrl. */ resumeAfterGather?: number; + /** Absolute URL of the TwiML document being translated. A / + * with no action re-requests this same URL on Twilio; the translator + * replicates that by rewriting it as the action, which only the server can + * usefully resolve (it knows the URL it just fetched and can fetch it + * again). Standalone BXML generation leaves this unset, so a missing + * action keeps producing a warning instead of a URL nothing will serve. */ + documentUrl?: string; } export interface TranslateResult { @@ -237,6 +253,7 @@ export function translateTwiml(twiml: string, opts: TranslateOptions = {}): Tran gatherSeq = 0; dialSeq = 0; currentTransferLegUrl = opts.transferLegUrl; + currentDocumentUrl = opts.documentUrl; const root = parseTwiml(twiml); const findings: Finding[] = []; const rewrite: RewriteUrl = opts.rewriteUrl ?? ((u: string) => u); @@ -256,11 +273,33 @@ export function translateTwiml(twiml: string, opts: TranslateOptions = {}): Tran if (opts.callbackAuth) stampCallbackAuth(els, opts.callbackAuth); return { bxml: bxmlDocument(els), - findings, + findings: dedupeFindings(findings), hasErrors: findings.some((f) => f.severity === "error"), }; } +/** + * Collapses findings that repeat the exact same severity+verb+message to a + * single occurrence. A generic finding (e.g. "Record's playBeep defaults to + * true...") says nothing new the second time it's reported for the same + * verb, and a document with many instances of one verb (e.g. 50 bare + * s) would otherwise bury any findings that differ — like the + * out-of-range warnings, which embed the offending value and so remain + * distinct per occurrence. Order is preserved; only later duplicates drop. + * Exported so compatibility-check's analyzeSource can apply the same rule + * across the several blocks one file may contain, instead of + * keeping its own separate dedup logic that could drift from this one. + */ +export function dedupeFindings(findings: Finding[]): Finding[] { + const seen = new Set(); + return findings.filter((f) => { + const key = `${f.severity}\u0000${f.verb}\u0000${f.message}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + }); +} + function unsupported(node: TwimlNode, findings: Finding[], detail?: string): null { const m = matrix.verbs[node.name]; findings.push({ @@ -276,6 +315,125 @@ function warn(verb: string, message: string, findings: Finding[]): void { findings.push({ severity: "warning", verb, message, docsUrl: matrix.verbs[verb]?.docsUrl }); } +/** True for an attribute that's absent or holds only whitespace — e.g. a + * template variable that rendered empty. Treated the same as "not set" for + * numeric attributes: JavaScript's Number() reads whitespace-only text as 0 + * (Number(" ") === 0), which would otherwise look like a real, in-range zero + * rather than a value nobody actually set. */ +function isBlank(value: string | undefined): boolean { + return value === undefined || value.trim() === ""; +} + +/** `value`, or `fallback` when `value` is blank (see isBlank). */ +function orDefault(value: string | undefined, fallback: string): string { + return isBlank(value) ? fallback : (value as string); +} + +/** + * Formats a finite number as plain decimal text. `String(n)` switches to + * scientific notation for very small magnitudes (`String(0.0000001)` is + * `"1e-7"`), which Bandwidth's BXML parser almost certainly doesn't expect + * for a duration or count; this always returns a plain decimal string. + */ +function toPlainDecimal(n: number): string { + const s = String(n); + if (!/e/i.test(s)) return s; + return n.toFixed(20).replace(/0+$/, "").replace(/\.$/, ""); +} + +/** + * Resolves a TwiML attribute to a plain, finite decimal number, warning once + * and falling back to `fallback` when it isn't a usable number at all (empty, + * "abc", "Infinity") rather than being passed through — Bandwidth's BXML + * parser expects a plain decimal number here, and silently forwarding "abc" + * would just move today's rejected-call problem from "out of range" to "not + * a number in the first place". A value that IS a number (including forms + * Bandwidth almost certainly doesn't accept, like "0x3C" or "1e1") is + * normalized to its plain decimal form. Blank input (see isBlank) should be + * resolved to a default by the caller before this runs, so this only ever + * warns about a value that's actually present but unusable. + */ +function resolveNumeric(verb: string, twilioAttr: string, value: string, fallback: string, findings: Finding[]): string { + const n = Number(value); + if (!Number.isFinite(n)) { + warn(verb, `${twilioAttr}="${value}" isn't a plain number Bandwidth can use; using ${fallback} instead.`, findings); + return fallback; + } + return toPlainDecimal(n); +} + +/** + * Rounds an already-numeric value (see resolveNumeric) to the nearest whole + * number, warning if it wasn't one. For a *count* — Gather's numDigits is the + * only one of these attributes — a fraction has no real meaning (there's no + * such thing as collecting 2.5 digits), unlike the duration attributes + * (timeout, maxLength), where Bandwidth's own docs allow decimal values and a + * fraction is a normal, real concept (a 2.5-second pause), so those are left + * as whatever plain decimal the customer wrote. + */ +function roundToInteger(verb: string, twilioAttr: string, value: string, findings: Finding[]): string { + const n = Number(value); + const rounded = Math.round(n); + if (rounded !== n) warn(verb, `${twilioAttr}="${value}" isn't a whole number; using ${rounded} instead.`, findings); + return String(rounded); +} + +/** + * Keeps an already-numeric value (see resolveNumeric) within the range + * Bandwidth documents for the BXML attribute it maps to, warning when it had + * to change. Warnings name the Twilio attribute the customer actually wrote + * (not Bandwidth's name for it) while still saying which BXML attribute is + * affected, so the warning can be found by searching the customer's own + * TwiML and still makes sense next to the generated BXML. + */ +function clampToRange( + verb: string, + twilioAttr: string, + bxmlAttr: string, + value: string, + min: number, + max: number, + findings: Finding[], +): string { + const n = Number(value); + if (n < min) { + warn(verb, `${twilioAttr}="${value}" is below Bandwidth's minimum for ${bxmlAttr} (${min}); using ${min} instead.`, findings); + return String(min); + } + if (n > max) { + warn(verb, `${twilioAttr}="${value}" is above Bandwidth's maximum for ${bxmlAttr} (${max}); using ${max} instead.`, findings); + return String(max); + } + return value; +} + +/** + * Resolves a Gather/Record's `action` to the URL it should actually request: + * the explicit `action` if given, or — when the server knows the URL of the + * document being translated — that same URL, matching Twilio's "no action + * re-requests the current document" behavior. Returns undefined, and warns, + * when there is neither (standalone BXML generation has no server to send a + * request back to). Shared by translateGather and translateRecord, which + * otherwise had identical branching here. + */ +function resolveActionUrl( + verb: string, + action: string | undefined, + kind: UrlKind, + rewrite: RewriteUrl, + findings: Finding[], + ctx?: RewriteContext, +): string | undefined { + if (action) return rewrite(action, kind, ctx); + if (currentDocumentUrl !== undefined) return rewrite(currentDocumentUrl, kind, { ...ctx, selfReference: true }); + warn( + verb, + `${verb} without an action attribute re-requests the current document URL on Twilio; set an explicit action for identical behavior through the translator.`, + findings, + ); + return undefined; +} + // Twilio's documented ceiling for Say/Play loop counts; also our hard bound on // a single verb's expansion so one attribute cannot force unbounded allocation. const MAX_LOOP = 1000; @@ -413,10 +571,32 @@ function translateGather( findings: Finding[], rewrite: RewriteUrl, ): XmlEl[] | null { + // Twilio's timeout (default 5) governs both the pause before the first digit + // and the gap between digits; Bandwidth splits these into two attributes. + // Bandwidth happens to default both to 5s on its own, so an omitted timeout + // is a no-op either way — the real fix is that an EXPLICIT timeout used to + // only reach firstDigitTimeout, leaving interDigitTimeout at Bandwidth's + // default regardless of what the customer set. A blank timeout (empty or + // all whitespace) is treated the same as omitted: Twilio doesn't accept a + // blank duration either. Validated as a number once, not once per BXML + // attribute it maps to, so a non-numeric value doesn't produce two + // near-identical warnings. + const timeout = resolveNumeric("Gather", "timeout", orDefault(node.attrs.timeout, "5"), "5", findings); const attrs: Record = { - maxDigits: node.attrs.numDigits, - firstDigitTimeout: node.attrs.timeout, - terminatingDigits: node.attrs.finishOnKey, + maxDigits: isBlank(node.attrs.numDigits) + ? undefined + : clampToRange( + "Gather", + "numDigits", + "maxDigits", + roundToInteger("Gather", "numDigits", resolveNumeric("Gather", "numDigits", node.attrs.numDigits as string, "50", findings), findings), + 1, + 50, + findings, + ), + firstDigitTimeout: clampToRange("Gather", "timeout", "firstDigitTimeout", timeout, 0, 60, findings), + interDigitTimeout: clampToRange("Gather", "timeout", "interDigitTimeout", timeout, 1, 60, findings), + terminatingDigits: gatherTerminatingDigits(node.attrs.finishOnKey, findings), }; // Twilio input: "dtmf" | "speech" | "dtmf speech". BW: dtmf | speech | dtmf_speech. if (node.attrs.input) { @@ -437,17 +617,8 @@ function translateGather( // server can resume after this Gather when Bandwidth reports no input // (Twilio continues with the next verb unless actionOnEmptyResult="true"). const gatherIndex = ++gatherSeq; - if (node.attrs.action) - attrs.gatherUrl = rewrite(node.attrs.action, "action", { - gatherIndex, - actionOnEmptyResult: node.attrs.actionOnEmptyResult === "true", - }); - else - warn( - "Gather", - "Gather without an action attribute re-requests the current document URL on Twilio; set an explicit action for identical behavior through the translator.", - findings, - ); + const gatherCtx = { gatherIndex, actionOnEmptyResult: node.attrs.actionOnEmptyResult === "true" }; + attrs.gatherUrl = resolveActionUrl("Gather", node.attrs.action, "action", rewrite, findings, gatherCtx); const children: XmlEl[] = []; for (const child of node.children) { const el = translateVerb(child, findings, rewrite); @@ -463,17 +634,127 @@ let gatherSeq = 0; let dialSeq = 0; // The server's transferDisconnect endpoint for the current translation, if any. let currentTransferLegUrl: ((dialIndex: number) => string) | undefined; +// The absolute URL of the document being translated, if the caller knows it +// (the server does; standalone BXML generation does not). See documentUrl. +// +// This makes six pieces of module state reset per translateTwiml call and +// subject to the same reentrancy caveat as bxmlByteBudget above. Multiple +// reviews have suggested threading one mutable context object through every +// verb-translating function instead, which would remove the caveat entirely. +// That's a real improvement, deliberately deferred rather than done inline +// here: it touches the signature of every translateX function and its +// recursive call sites, which is a larger, separate refactor than this fix +// warrants risking right before merge. Worth doing as its own follow-up. +let currentDocumentUrl: string | undefined; + +// The only characters Bandwidth recognizes as a key in either verb's +// terminatingDigits: Gather's set, or the one character Record takes. +const VALID_TERMINATING_KEYS = new Set([..."0123456789*#"]); + +/** True for a non-empty string that holds only whitespace — almost certainly + * a template artifact, not a deliberate value. Used to treat a + * whitespace-only finishOnKey the same as omitted (Twilio's own default), + * not as the deliberate "turn the stop-key feature off" value that an exact + * empty string is, the same way a blank numeric attribute falls back to its + * default rather than being read as a real zero (see isBlank). */ +function isWhitespaceOnly(value: string): boolean { + return value !== "" && value.trim() === ""; +} + +/** + * Keeps only the characters Bandwidth recognizes as keys in a Gather's + * finishOnKey — a set, like Twilio's: any of these ends the Gather. A + * character outside 0-9/*\/# would never match a real key press, silently + * disabling the feature the same way an entirely invalid value would, so + * each invalid character is dropped — with a warning — rather than passed + * through for Bandwidth to silently ignore. Twilio's default ("#") is exactly + * matched by Bandwidth's explicit "#" (unlike Record's default, which only + * partially matches — see recordTerminatingDigits), so the not-set case + * doesn't need a warning. + */ +function gatherTerminatingDigits(finishOnKey: string | undefined, findings: Finding[]): string { + if (finishOnKey === "") return ""; // both systems treat this as "disabled" + if (finishOnKey === undefined || isWhitespaceOnly(finishOnKey)) return "#"; + const valid = [...finishOnKey].filter((c) => VALID_TERMINATING_KEYS.has(c)).join(""); + if (valid !== finishOnKey) + warn( + "Gather", + `finishOnKey="${finishOnKey}" has a character Bandwidth doesn't recognize as a key (only 0-9, "*", and "#" work); using "${valid}" instead.`, + findings, + ); + return valid; +} + +/** + * Bandwidth's Record terminatingDigits takes a single character from + * `VALID_TERMINATING_KEYS`, unlike Gather's set of acceptable characters and + * Twilio's own default of "any key" (1234567890*#). Twilio's default and + * Bandwidth's own default ("#") happen to agree on "#" stopping the + * recording, so the not-set case is a partial match: only "#" stops it here, + * not the other nine keys. An explicit multi-character finishOnKey is + * narrowed the same way, preferring "#" then "*" then the first valid + * character present, since Bandwidth cannot represent a set — but a value + * made up of nothing but repeats of one single valid character (e.g. "##") + * isn't actually narrowed to anything different, so that doesn't warn either. + * A value with no valid character at all (e.g. "a") can't be narrowed to + * anything; on Twilio that value would never match a real key either, so + * nothing should stop the recording early — emitting Bandwidth's own default + * ("#") instead would introduce a stop key the customer never asked for, so + * this disables the feature ("") rather than falling back to it. + */ +function recordTerminatingDigits(finishOnKey: string | undefined, findings: Finding[]): string { + if (finishOnKey === "") return ""; // both systems treat this as "disabled" + if (finishOnKey === undefined || isWhitespaceOnly(finishOnKey)) { + warn( + "Record", + `finishOnKey defaults to "1234567890*#" (any key stops the recording) on Twilio; Bandwidth's Record only supports one terminating digit, so only "#" will stop it here.`, + findings, + ); + return "#"; + } + const valid = [...finishOnKey].filter((c) => VALID_TERMINATING_KEYS.has(c)); + if (valid.length === 0) { + warn( + "Record", + `finishOnKey="${finishOnKey}" has no digit, "*", or "#" for Bandwidth's Record to use as a terminating key; no key will stop the recording early (the way none of "${finishOnKey}" would stop it on Twilio either).`, + findings, + ); + return ""; + } + // A value made entirely of repeats of the one valid character (e.g. "##") + // narrows to exactly that character with nothing lost, so it's an exact + // match in every way that matters, not something worth warning about. + if (new Set(valid).size === 1 && valid.length === finishOnKey.length) return valid[0]!; + const chosen = valid.includes("#") ? "#" : valid.includes("*") ? "*" : valid[0]!; + warn( + "Record", + `finishOnKey="${finishOnKey}" has more than one character; Bandwidth's Record only supports one terminating digit, so only "${chosen}" will stop it here.`, + findings, + ); + return chosen; +} function translateRecord( node: TwimlNode, findings: Finding[], rewrite: RewriteUrl, ): XmlEl[] | null { + // Twilio's maxLength defaults to 3600s; Bandwidth's own maxDuration default + // is 60s, so the omitted case must be explicit to avoid recordings cut + // short at a minute. Twilio's timeout (silence ends the recording) has no + // BXML attribute at all today; silenceTimeout fills that gap. Twilio's + // timeout="0" (disable) and Bandwidth's silenceTimeout="0" (no timeout) + // agree, so no special-casing is needed for that value. A blank value + // (empty or all whitespace) is treated the same as omitted: Twilio doesn't + // accept a blank duration either. + const maxLength = resolveNumeric("Record", "maxLength", orDefault(node.attrs.maxLength, "3600"), "3600", findings); + const timeout = resolveNumeric("Record", "timeout", orDefault(node.attrs.timeout, "5"), "5", findings); const attrs: Record = { - maxDuration: node.attrs.maxLength, - terminatingDigits: node.attrs.finishOnKey, + maxDuration: clampToRange("Record", "maxLength", "maxDuration", maxLength, 1, 10800, findings), + silenceTimeout: clampToRange("Record", "timeout", "silenceTimeout", timeout, 0, 10800, findings), + terminatingDigits: recordTerminatingDigits(node.attrs.finishOnKey, findings), + recordCompleteUrl: resolveActionUrl("Record", node.attrs.action, "record", rewrite, findings), }; - if (node.attrs.action) attrs.recordCompleteUrl = rewrite(node.attrs.action, "record"); // Twilio's async "recording is ready" webhook (recordingStatusCallback) maps to // Bandwidth's recordingAvailableUrl. The translator receives the BW event, reshapes // it into Twilio recording-callback params, and forwards it to the customer. @@ -494,8 +775,22 @@ function translateRecord( "Transcription engines and callback payloads differ between Twilio and Bandwidth.", findings, ); - if (node.attrs.playBeep && node.attrs.playBeep !== "false") - warn("Record", "playBeep has no BXML equivalent; no beep will play before recording.", findings); + // Twilio's playBeep defaults to true; warn unless it's explicitly disabled, + // since that's the only value where Bandwidth's "never beeps" matches. + if (node.attrs.playBeep !== "false") + warn( + "Record", + "playBeep defaults to true (plays a beep) on Twilio; Bandwidth has no equivalent, so no beep will play before recording.", + findings, + ); + // Twilio's trim defaults to "trim-silence"; warn unless it's explicitly + // "do-not-trim", the only value where Bandwidth's "never trims" matches. + if (node.attrs.trim !== "do-not-trim") + warn( + "Record", + "Twilio trims leading/trailing silence by default (trim-silence); Bandwidth never trims recordings, so no equivalent exists.", + findings, + ); return [{ name: "Record", attrs }]; } diff --git a/test/compatibility-check.test.ts b/test/compatibility-check.test.ts index 31f70fe..9d44857 100644 --- a/test/compatibility-check.test.ts +++ b/test/compatibility-check.test.ts @@ -26,6 +26,16 @@ vr.enqueue("support");`; const a = analyzeSource("other.js", `console.log("nothing here")`); expect(a.verbs).toEqual([]); }); + + it("collapses the same finding repeated across several blocks in one file (VAPI-4042)", () => { + // Each bare independently warns about playBeep; a file with many + // of them (even across separate embedded documents) should report it + // once, not once per occurrence, via the shared dedupeFindings helper. + const src = Array.from({ length: 5 }, (_, i) => `\`\``).join(";\n"); + const a = analyzeSource("many-records.js", src); + const playBeepFindings = a.findings.filter((f) => /playBeep/.test(f.message)); + expect(playBeepFindings).toHaveLength(1); + }); }); describe("report", () => { diff --git a/test/fixtures/twiml/gather-record-limits.xml b/test/fixtures/twiml/gather-record-limits.xml new file mode 100644 index 0000000..516d654 --- /dev/null +++ b/test/fixtures/twiml/gather-record-limits.xml @@ -0,0 +1,4 @@ + + + + diff --git a/test/server-continue.test.ts b/test/server-continue.test.ts index cb7abe1..c964087 100644 --- a/test/server-continue.test.ts +++ b/test/server-continue.test.ts @@ -529,6 +529,97 @@ describe("/bw/continue recordComplete -> Record action", () => { }); }); +// ─── Gather/Record with no action (VAPI-4042) ────────────────────────────── + +describe("Gather/Record with no action re-request the current document, like Twilio", () => { + it("Gather posts Digits back to the same voice URL that served it", async () => { + const VOICE = "https://customer.test/voice"; + const { app, customerPost } = makeApp({ + [VOICE]: `fallback`, + }); + const first = await initiate(app, "c-noaction-1"); + const gatherUrl = /gatherUrl="([^"]+)"/.exec(first.body)![1].replace(/&/g, "&"); + // No action attribute, yet the rewriter still points gatherUrl at /bw/continue + // carrying the current document's own URL as `next`. + expect(gatherUrl).toContain(encodeURIComponent(VOICE)); + const res = await app.inject({ + method: "POST", + url: new URL(gatherUrl).pathname + new URL(gatherUrl).search, + headers: { authorization: webhookAuth }, + payload: bwEvent("c-noaction-1", { eventType: "gather", digits: "5" }), + }); + expect(res.statusCode).toBe(200); + const { url, params } = customerPost(1)!; + expect(url).toBe(VOICE); + expect(params.Digits).toBe("5"); + }); + + it("Record posts RecordingUrl back to the same voice URL that served it", async () => { + const VOICE = "https://customer.test/voice"; + const { app, customerPost } = makeApp({ + [VOICE]: ``, + }); + const first = await initiate(app, "c-noaction-2"); + const recordUrl = /recordCompleteUrl="([^"]+)"/.exec(first.body)![1].replace(/&/g, "&"); + expect(recordUrl).toContain(encodeURIComponent(VOICE)); + const res = await app.inject({ + method: "POST", + url: new URL(recordUrl).pathname + new URL(recordUrl).search, + headers: { authorization: webhookAuth }, + payload: bwEvent("c-noaction-2", { eventType: "recordComplete", recordingId: "r-noaction", duration: "PT4S" }), + }); + expect(res.statusCode).toBe(200); + const { url, params } = customerPost(1)!; + expect(url).toBe(VOICE); + expect(params.RecordingSid).toBe(toRecordingSid("r-noaction")); + }); + + it("resumes past the Gather on empty input when the same instance has the document, even with no action", async () => { + const VOICE = "https://customer.test/voice"; + const { app, posts } = makeApp({ + [VOICE]: `fallback`, + }); + const first = await initiate(app, "c-noaction-3"); + const gatherUrl = /gatherUrl="([^"]+)"/.exec(first.body)![1].replace(/&/g, "&"); + const res = await app.inject({ + method: "POST", + url: new URL(gatherUrl).pathname + new URL(gatherUrl).search, + headers: { authorization: webhookAuth }, + payload: bwEvent("c-noaction-3", { eventType: "gather", digits: "" }), + }); + expect(res.body).toContain("fallback"); + expect(posts()).toBe(1); // resumed locally; no request to the customer at all + }); + + it("ends the call, rather than replaying the same prompt forever, when a different instance has no document to resume", async () => { + // A Gather with no action self-references the document that served it. If + // the empty-result callback lands on an instance with no cached copy of + // that document (restart, another replica), falling back to "request the + // action anyway" would re-fetch and replay this exact same prompt -- and + // since nothing about the situation changes, it would keep replaying + // forever. Two separate makeApp() calls simulate two separate instances + // sharing no in-memory state. + const VOICE = "https://customer.test/voice"; + const docs = { [VOICE]: `fallback` }; + const { app: instanceA } = makeApp(docs); + const first = await initiate(instanceA, "c-noaction-4"); + const gatherUrl = /gatherUrl="([^"]+)"/.exec(first.body)![1].replace(/&/g, "&"); + expect(gatherUrl).toContain("&self=1"); + + const { app: instanceB, posts: postsB } = makeApp(docs); + const res = await instanceB.inject({ + method: "POST", + url: new URL(gatherUrl).pathname + new URL(gatherUrl).search, + headers: { authorization: webhookAuth }, + payload: bwEvent("c-noaction-4", { eventType: "gather", digits: "" }), + }); + expect(res.statusCode).toBe(200); + expect(res.body).not.toContain(" { diff --git a/test/translate-gather-record.test.ts b/test/translate-gather-record.test.ts index 90a946b..74d97f4 100644 --- a/test/translate-gather-record.test.ts +++ b/test/translate-gather-record.test.ts @@ -1,8 +1,20 @@ import { describe, it, expect } from "vitest"; -import { translateTwiml } from "../src/translator/translate.js"; +import { translateTwiml, type RewriteContext, type UrlKind } from "../src/translator/translate.js"; const rw = (url: string) => `https://translator.test/bw/continue?next=${encodeURIComponent(url)}`; +/** A rewriter that behaves like the server's: resolves a relative/empty url + * against `base` and records every call, so a Gather/Record's "no action" + * rewrite can be inspected. */ +function serverLikeRewriter(base = "https://customer.test/voice") { + const calls: { url: string; kind: UrlKind; ctx?: RewriteContext }[] = []; + const rewriteUrl = (url: string, kind: UrlKind, ctx?: RewriteContext) => { + calls.push({ url, kind, ctx }); + return `https://translator.test/bw/continue?next=${encodeURIComponent(new URL(url, base).toString())}`; + }; + return { calls, rewriteUrl, base }; +} + describe("Gather", () => { it("maps attributes and nested prompts", () => { const r = translateTwiml( @@ -11,6 +23,7 @@ describe("Gather", () => { ); expect(r.bxml).toContain(`maxDigits="1"`); expect(r.bxml).toContain(`firstDigitTimeout="7"`); + expect(r.bxml).toContain(`interDigitTimeout="7"`); expect(r.bxml).toContain(`terminatingDigits="#"`); expect(r.bxml).toContain(`gatherUrl="https://translator.test/bw/continue?next=%2Fmenu"`); expect(r.bxml).toContain(`Press 1`); @@ -34,6 +47,224 @@ describe("Gather", () => { const r = translateTwiml(``); expect(r.findings.some((f) => f.severity === "warning" && /action/.test(f.message))).toBe(true); }); + + describe("Twilio defaults (VAPI-4042)", () => { + it("emits finishOnKey's Twilio default (#) when omitted, rather than Bandwidth's own default (disabled)", () => { + const r = translateTwiml(``, { rewriteUrl: rw }); + expect(r.bxml).toContain(`terminatingDigits="#"`); + expect(r.findings).toEqual([]); + }); + + it("passes finishOnKey=\"\" through unchanged (Twilio and Bandwidth both treat it as disabled)", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits=""`); + expect(r.findings).toEqual([]); + }); + + it("drops a character Bandwidth doesn't recognize as a key, and warns", () => { + // "ab" would never match a real key press, so it would silently disable + // the stop-key feature if passed through as written. + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).not.toContain(`terminatingDigits="ab"`); + expect( + r.findings.some((f) => f.severity === "warning" && /finishOnKey="ab"/.test(f.message)), + ).toBe(true); + }); + + it("keeps the valid characters in a mixed set and drops only the invalid one", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits="1#"`); + }); + + it("treats a whitespace-only finishOnKey the same as omitted, not as an invalid value to filter", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits="#"`); + expect(r.findings).toEqual([]); + }); + + it("emits timeout's Twilio default (5) as both firstDigitTimeout and interDigitTimeout when omitted", () => { + const r = translateTwiml(``, { rewriteUrl: rw }); + expect(r.bxml).toContain(`firstDigitTimeout="5"`); + expect(r.bxml).toContain(`interDigitTimeout="5"`); + }); + + it("does not emit maxDigits when numDigits is omitted (Bandwidth's own default of 50 is close enough to Twilio's unbounded default)", () => { + const r = translateTwiml(``, { rewriteUrl: rw }); + expect(r.bxml).not.toContain("maxDigits"); + }); + }); + + describe("keeping values within Bandwidth's limits (VAPI-4042)", () => { + it("brings an out-of-range timeout back within limits for both firstDigitTimeout and interDigitTimeout, with a warning for each", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`firstDigitTimeout="60"`); + expect(r.bxml).toContain(`interDigitTimeout="60"`); + const warnings = r.findings.filter((f) => f.severity === "warning"); + // Warnings name "timeout" (what the customer wrote), not Bandwidth's + // attribute names, but still say which BXML attribute is affected. + expect(warnings.some((f) => /timeout="100".*firstDigitTimeout/.test(f.message))).toBe(true); + expect(warnings.some((f) => /timeout="100".*interDigitTimeout/.test(f.message))).toBe(true); + }); + + it("enforces interDigitTimeout's minimum (1) separately from firstDigitTimeout's minimum (0)", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + // firstDigitTimeout="0" is in range (0-60): left alone. + expect(r.bxml).toContain(`firstDigitTimeout="0"`); + // interDigitTimeout's minimum is 1: timeout="0" is raised to 1. + expect(r.bxml).toContain(`interDigitTimeout="1"`); + expect( + r.findings.some((f) => f.severity === "warning" && /timeout="0".*interDigitTimeout/.test(f.message)), + ).toBe(true); + expect(r.findings.some((f) => /firstDigitTimeout/.test(f.message))).toBe(false); + }); + + it("limits numDigits to Bandwidth's 50-digit maximum", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDigits="50"`); + expect( + r.findings.some((f) => f.severity === "warning" && /numDigits="75"/.test(f.message)), + ).toBe(true); + }); + + it("leaves an in-range value untouched with no warning", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`firstDigitTimeout="10"`); + expect(r.bxml).toContain(`interDigitTimeout="10"`); + expect(r.bxml).toContain(`maxDigits="4"`); + expect(r.findings).toEqual([]); + }); + + it("treats timeout=\"\" the same as omitted, not as 0", () => { + // Number("") is 0, which is in-range for firstDigitTimeout (min 0) but + // below interDigitTimeout's minimum (1) -- either way, an empty value + // should fall back to Twilio's default (5), not be read as a number. + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`firstDigitTimeout="5"`); + expect(r.bxml).toContain(`interDigitTimeout="5"`); + expect(r.findings).toEqual([]); + }); + + it("treats a whitespace-only timeout the same as empty, not as 0", () => { + // Number(" ") is also 0 -- a template variable that rendered as a + // space, not nothing, must not be read as a real zero either. + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`firstDigitTimeout="5"`); + expect(r.bxml).toContain(`interDigitTimeout="5"`); + expect(r.findings).toEqual([]); + }); + + it("treats a whitespace-only numDigits the same as omitted", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).not.toContain("maxDigits"); + expect(r.findings).toEqual([]); + }); + + it("normalizes a non-decimal but in-range value instead of passing it through as written", () => { + // Number("0x3C") is 60 in JavaScript, but Bandwidth's BXML parser almost + // certainly expects a plain decimal number, not a hex literal. + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`firstDigitTimeout="60"`); + expect(r.bxml).toContain(`interDigitTimeout="60"`); + expect(r.bxml).not.toContain("0x3C"); + }); + + it("normalizes scientific notation to a plain decimal", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDigits="10"`); + expect(r.bxml).not.toContain("1e1"); + }); + + it("falls back to Bandwidth's own default and warns when the value isn't a number at all", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDigits="50"`); + expect(r.bxml).not.toContain("abc"); + expect( + r.findings.some((f) => f.severity === "warning" && /numDigits="abc"/.test(f.message)), + ).toBe(true); + }); + + it("never emits scientific notation, even for a very small value", () => { + // String(0.0000001) is "1e-7" in JavaScript; Bandwidth's BXML parser + // almost certainly expects a plain decimal number. + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).not.toContain("e-7"); + expect(r.bxml).not.toMatch(/[0-9]e[-+]?[0-9]/i); + }); + + it("rounds numDigits to a whole number and warns, since there's no such thing as collecting half a digit", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDigits="3"`); + expect( + r.findings.some((f) => f.severity === "warning" && /numDigits="2.5" isn't a whole number/.test(f.message)), + ).toBe(true); + }); + + it("warns only once for a non-numeric timeout, not once per BXML attribute it maps to", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + const warnings = r.findings.filter((f) => /timeout="abc"/.test(f.message)); + expect(warnings).toHaveLength(1); + }); + }); + + describe("no action, re-requesting the current document (VAPI-4042)", () => { + it("points gatherUrl at the current document when the server provides one, with no warning", () => { + const { calls, rewriteUrl, base } = serverLikeRewriter(); + const r = translateTwiml(``, { + rewriteUrl, + documentUrl: base, + }); + expect(r.bxml).toContain(`gatherUrl="https://translator.test/bw/continue?next=${encodeURIComponent(base)}"`); + expect(r.findings.some((f) => /action attribute/.test(f.message))).toBe(false); + // The rewriter still gets the Gather's document position and empty-result + // flag, exactly as it would for an explicit action, plus selfReference so + // the server can tell this apart from a real customer action. + const call = calls.find((c) => c.kind === "action"); + expect(call?.ctx).toEqual({ gatherIndex: 1, actionOnEmptyResult: false, selfReference: true }); + }); + + it("still warns when no documentUrl is given (standalone BXML generation has no server to re-request)", () => { + const { rewriteUrl } = serverLikeRewriter(); + const r = translateTwiml(``, { rewriteUrl }); + expect(r.bxml).not.toContain("gatherUrl"); + expect(r.findings.some((f) => f.severity === "warning" && /action attribute/.test(f.message))).toBe( + true, + ); + }); + }); }); describe("Record", () => { @@ -46,4 +277,232 @@ describe("Record", () => { expect(r.bxml).toContain(`terminatingDigits="#"`); expect(r.bxml).toContain(`recordCompleteUrl="https://translator.test/bw/continue?next=%2Fdone"`); }); + + describe("Twilio defaults (VAPI-4042)", () => { + it("emits maxLength's Twilio default (3600) when omitted, rather than Bandwidth's own default (60)", () => { + const r = translateTwiml(``, { rewriteUrl: rw }); + expect(r.bxml).toContain(`maxDuration="3600"`); + }); + + it("emits timeout as silenceTimeout, defaulting to Twilio's 5 seconds (Bandwidth has no silence timeout today)", () => { + const r = translateTwiml(``, { rewriteUrl: rw }); + expect(r.bxml).toContain(`silenceTimeout="5"`); + }); + + it("passes timeout=\"0\" through unchanged (Twilio and Bandwidth both treat it as disabled)", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`silenceTimeout="0"`); + }); + }); + + describe("keeping values within Bandwidth's limits (VAPI-4042)", () => { + it("limits maxLength to Bandwidth's 10800s (3h) maximum", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDuration="10800"`); + expect( + r.findings.some((f) => f.severity === "warning" && /maxLength="14400".*maxDuration/.test(f.message)), + ).toBe(true); + }); + + it("limits timeout (silenceTimeout) to Bandwidth's 10800s maximum", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`silenceTimeout="10800"`); + }); + + it("keeps a fractional maxLength as-is, unlike numDigits: a 90.5-second recording limit is a real concept", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDuration="90.5"`); + expect(r.findings.some((f) => /maxLength="90.5"/.test(f.message))).toBe(false); + }); + + it("treats maxLength=\"\" the same as omitted, not as 0", () => { + // Number("") is 0, which is below maxDuration's minimum of 1 -- without + // treating "" as missing, a maxLength="" cuts a recording off at 1s. + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDuration="3600"`); + }); + + it("treats timeout=\"\" the same as omitted, not as 0", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`silenceTimeout="5"`); + }); + + it("treats a whitespace-only maxLength the same as empty, not as 0", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDuration="3600"`); + }); + + it("treats a whitespace-only timeout the same as empty, not as 0", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`silenceTimeout="5"`); + }); + }); + + describe("finishOnKey -> a single terminatingDigit (VAPI-4042)", () => { + it("warns when omitted: Bandwidth's own default (#) only covers one of Twilio's default any-key set", () => { + const r = translateTwiml(``, { rewriteUrl: rw }); + expect(r.bxml).toContain(`terminatingDigits="#"`); + expect( + r.findings.some((f) => f.severity === "warning" && /finishOnKey defaults to/.test(f.message)), + ).toBe(true); + }); + + it("passes a single explicit digit through with no warning (exact match)", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits="5"`); + expect(r.findings.some((f) => /finishOnKey/.test(f.message))).toBe(false); + }); + + it("prefers # when a multi-character set includes it, and warns", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits="#"`); + expect( + r.findings.some((f) => f.severity === "warning" && /finishOnKey="12#"/.test(f.message)), + ).toBe(true); + }); + + it("falls back to the first character when # is not in a multi-character set, and warns", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits="1"`); + expect(r.findings.some((f) => /finishOnKey="123"/.test(f.message))).toBe(true); + }); + + it("picks the first valid character in a mixed set rather than blindly the first character", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits="1"`); + expect(r.bxml).not.toContain(`terminatingDigits="a"`); + }); + + it("disables the stop key and warns when there is no valid key at all, rather than falling back to Bandwidth's own default", () => { + // On Twilio, "a" never matches a real key either, so no key stops the + // recording early. Falling back to Bandwidth's own default ("#") would + // introduce a stop key the customer never asked for. + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits=""`); + expect( + r.findings.some((f) => f.severity === "warning" && /finishOnKey="a"/.test(f.message)), + ).toBe(true); + }); + + it("does not warn when a repeated single valid character narrows to nothing different", () => { + // "##" narrows to "#", exactly what a bare "#" would already produce -- + // there's no real narrowing or information loss to warn about. + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits="#"`); + expect(r.findings.some((f) => /finishOnKey/.test(f.message))).toBe(false); + }); + + it("treats a whitespace-only finishOnKey the same as omitted, not as an invalid value to disable", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`terminatingDigits="#"`); + expect( + r.findings.some((f) => f.severity === "warning" && /finishOnKey defaults to/.test(f.message)), + ).toBe(true); + }); + }); + + describe("playBeep and trim (VAPI-4042)", () => { + it("warns when playBeep is omitted (Twilio's default is true) and when explicitly true", () => { + for (const twiml of [ + ``, + ``, + ]) { + const r = translateTwiml(twiml, { rewriteUrl: rw }); + expect(r.findings.some((f) => f.severity === "warning" && /playBeep/.test(f.message))).toBe(true); + } + }); + + it("does not warn about playBeep when explicitly false (the one value Bandwidth matches)", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.findings.some((f) => /playBeep/.test(f.message))).toBe(false); + }); + + it("warns when trim is omitted (Twilio's default is trim-silence) and when explicitly trim-silence", () => { + for (const twiml of [ + ``, + ``, + ]) { + const r = translateTwiml(twiml, { rewriteUrl: rw }); + expect(r.findings.some((f) => f.severity === "warning" && /trim/.test(f.message))).toBe(true); + } + }); + + it("does not warn about trim when explicitly do-not-trim (the one value Bandwidth matches)", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.findings.some((f) => /trim/.test(f.message))).toBe(false); + }); + }); + + describe("no action, re-requesting the current document (VAPI-4042)", () => { + it("points recordCompleteUrl at the current document when the server provides one, with no 'missing action' warning", () => { + const { calls, rewriteUrl, base } = serverLikeRewriter(); + const r = translateTwiml(``, { rewriteUrl, documentUrl: base }); + expect(r.bxml).toContain( + `recordCompleteUrl="https://translator.test/bw/continue?next=${encodeURIComponent(base)}"`, + ); + expect(r.findings.some((f) => /action attribute/.test(f.message))).toBe(false); + expect(calls.some((c) => c.kind === "record" && c.url === base)).toBe(true); + }); + + it("warns when no documentUrl is given (standalone BXML generation has no server to re-request)", () => { + const { rewriteUrl } = serverLikeRewriter(); + const r = translateTwiml(``, { rewriteUrl }); + expect(r.bxml).not.toContain("recordCompleteUrl"); + expect(r.findings.some((f) => f.severity === "warning" && /action attribute/.test(f.message))).toBe( + true, + ); + }); + }); +}); + +describe("repeated identical findings collapse to one (VAPI-4042)", () => { + it("a document with many bare Records reports each generic warning once, not once per Record", () => { + const manyRecords = Array.from({ length: 10 }, (_, i) => ``).join(""); + const r = translateTwiml(`${manyRecords}`, { rewriteUrl: rw }); + const playBeepWarnings = r.findings.filter((f) => /playBeep/.test(f.message)); + expect(playBeepWarnings).toHaveLength(1); + }); + + it("does not collapse findings that differ, e.g. out-of-range warnings naming different values", () => { + const r = translateTwiml( + ``, + { rewriteUrl: rw }, + ); + const maxLengthWarnings = r.findings.filter((f) => /maxLength=/.test(f.message)); + expect(maxLengthWarnings).toHaveLength(2); + }); });