From 3d45209a374f077428eaa8077d0c1ae9afbfa6bd Mon Sep 17 00:00:00 2001 From: Madhu Ramasubramanian Date: Tue, 13 Oct 2026 11:00:01 -0400 Subject: [PATCH 1/5] VAPI-4042: emit Twilio's Gather/Record defaults and clamp to BW's ranges Twilio and Bandwidth default differently for several Gather/Record attributes; leaving them unset let Bandwidth's own default silently change IVR behavior instead of matching Twilio's. Gather: - finishOnKey omitted -> emit "#" (Twilio's default); Bandwidth's own default is "" (disabled). finishOnKey="" still passes through. - timeout omitted -> default "5", set as both firstDigitTimeout and interDigitTimeout (Bandwidth splits what Twilio combines). - numDigits/timeout clamped to Bandwidth's documented ranges with a warning naming the original value. - A Gather with no action re-requests the current document, matching Twilio, when the server provides the document's URL. Record: - maxLength omitted -> emit "3600" (Twilio's default); Bandwidth's own default is 60s. - timeout (silence ends the recording) is now mapped at all, to silenceTimeout; previously dropped entirely. timeout="0" (Twilio: disable) and silenceTimeout="0" (Bandwidth: no timeout) agree. - finishOnKey narrowed to Bandwidth's single terminating digit (preferring #, then *, then the first character), with a warning, since Bandwidth can't represent Twilio's multi-key set (default: any key). A single explicit digit passes through unchanged. - playBeep/trim now warn on the omitted (Twilio-default) case too, not only when explicitly set to a non-matching value. - A Record with no action re-requests the current document, matching Twilio, when the server provides the document's URL. The "re-request current document" behavior is driven by a new TranslateOptions.documentUrl, set only by the server (one line in app.ts); standalone BXML generation leaves it unset and keeps the original warning, since there's no live document for it to re-fetch. --- AGENTS.md | 24 +- src/matrix/twilio-voice.json | 18 +- src/server/app.ts | 1 + src/translator/translate.ts | 142 +++++++++-- test/fixtures/twiml/gather-record-limits.xml | 4 + test/server-continue.test.ts | 46 ++++ test/translate-gather-record.test.ts | 245 ++++++++++++++++++- 7 files changed, 453 insertions(+), 27 deletions(-) create mode 100644 test/fixtures/twiml/gather-record-limits.xml diff --git a/AGENTS.md b/AGENTS.md index 823bd71..9b30e73 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,8 +100,8 @@ Translation is a fixed rulebook (`src/matrix/twilio-voice.json`), not a guess. 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. + warns; `Record`'s `transcribe`, `playBeep`, and `trim` attributes have no + direct BXML equivalent. - **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 +122,26 @@ 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) the action is requested as a fallback. A Gather with no `action` + attribute re-requests the current document, as on Twilio. Twilio's + defaults are emitted explicitly rather than left to Bandwidth's own + (different) defaults: `finishOnKey="#"`, and `timeout` (default 5) sets + both `firstDigitTimeout` and `interDigitTimeout`. `numDigits`, `timeout`, + and `finishOnKey` (on `Record`, below) are clamped to Bandwidth's + documented ranges with a warning naming the original value. - `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 defaults. `finishOnKey` is narrowed to + Bandwidth's single terminating digit (preferring `#`, then `*`, then the + first character) since Bandwidth cannot represent Twilio's default set of + any key. `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/matrix/twilio-voice.json b/src/matrix/twilio-voice.json index 4b7ee9a..31d5bab 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": "Clamped to Bandwidth's 1-50 range with a warning naming the original value; 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 clamped independently to its own range (firstDigitTimeout 0-60, interDigitTimeout 1-60) with a warning naming the original value." }, + "finishOnKey": { "bxml": "terminatingDigits", "status": "supported", "notes": "Twilio's default (\"#\") is emitted explicitly when omitted, since Bandwidth's own default for this attribute is \"\" (disabled). finishOnKey=\"\" passes through unchanged (both systems treat it as disabled)." }, "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. Clamped to Bandwidth's 10800s (3h) maximum with a warning naming the original value." }, + "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 clamped 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; Twilio's finishOnKey is a set (default \"1234567890*#\", any key). A single character passes through unchanged; a multi-character set (including the omitted default) is narrowed to one digit, preferring '#' then '*' then the first character, with a warning." }, "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..c7be285 100644 --- a/src/server/app.ts +++ b/src/server/app.ts @@ -331,6 +331,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) { diff --git a/src/translator/translate.ts b/src/translator/translate.ts index 6698dee..dd702d8 100644 --- a/src/translator/translate.ts +++ b/src/translator/translate.ts @@ -42,6 +42,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 +244,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); @@ -276,6 +284,33 @@ function warn(verb: string, message: string, findings: Finding[]): void { findings.push({ severity: "warning", verb, message, docsUrl: matrix.verbs[verb]?.docsUrl }); } +/** + * Clamps a numeric TwiML attribute to the range Bandwidth documents for the + * BXML attribute it maps to, warning when the original value fell outside it. + * A non-numeric value is returned unchanged — validating shape isn't this + * function's job; Bandwidth will reject malformed BXML on its own. + */ +function clampAttr( + verb: string, + bxmlAttr: string, + value: string, + min: number, + max: number, + findings: Finding[], +): string { + const n = Number(value); + if (!Number.isFinite(n)) return value; + if (n < min) { + warn(verb, `${bxmlAttr}="${value}" is below Bandwidth's minimum of ${min}; clamped to ${min}.`, findings); + return String(min); + } + if (n > max) { + warn(verb, `${bxmlAttr}="${value}" exceeds Bandwidth's maximum of ${max}; clamped to ${max}.`, findings); + return String(max); + } + return value; +} + // 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 +448,22 @@ 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. + // Leaving them unset would let Bandwidth's own defaults apply instead + // (terminatingDigits "" i.e. disabled, and no interDigitTimeout at all). + const timeout = node.attrs.timeout ?? "5"; const attrs: Record = { - maxDigits: node.attrs.numDigits, - firstDigitTimeout: node.attrs.timeout, - terminatingDigits: node.attrs.finishOnKey, + maxDigits: node.attrs.numDigits + ? clampAttr("Gather", "numDigits", node.attrs.numDigits, 1, 50, findings) + : undefined, + firstDigitTimeout: clampAttr("Gather", "firstDigitTimeout", timeout, 0, 60, findings), + interDigitTimeout: clampAttr("Gather", "interDigitTimeout", timeout, 1, 60, findings), + // Twilio's default finishOnKey is "#"; Bandwidth's own default for + // terminatingDigits is "" (disabled), so the omitted case must be explicit. + // finishOnKey="" (Twilio: disable) passes through unchanged, matching + // Bandwidth's own disable value. + terminatingDigits: node.attrs.finishOnKey ?? "#", }; // Twilio input: "dtmf" | "speech" | "dtmf speech". BW: dtmf | speech | dtmf_speech. if (node.attrs.input) { @@ -437,17 +484,20 @@ 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 + const gatherCtx = { gatherIndex, actionOnEmptyResult: node.attrs.actionOnEmptyResult === "true" }; + if (node.attrs.action) { + attrs.gatherUrl = rewrite(node.attrs.action, "action", gatherCtx); + } else if (currentDocumentUrl !== undefined) { + // Twilio re-requests the current document's URL when no action is given. + attrs.gatherUrl = rewrite(currentDocumentUrl, "action", gatherCtx); + } else { + // Standalone BXML generation has no server to re-request a document from. 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 children: XmlEl[] = []; for (const child of node.children) { const el = translateVerb(child, findings, rewrite); @@ -463,6 +513,37 @@ 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. +let currentDocumentUrl: string | undefined; + +/** + * Bandwidth's Record terminatingDigits takes a single character, 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 omitted 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 + * character, since Bandwidth cannot represent a set. + */ +function recordTerminatingDigits(finishOnKey: string | undefined, findings: Finding[]): string { + if (finishOnKey === undefined) { + 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 "#"; + } + if (finishOnKey.length <= 1) return finishOnKey; // exact match: "", a digit, "*", or "#" + const chosen = finishOnKey.includes("#") ? "#" : finishOnKey.includes("*") ? "*" : finishOnKey[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, @@ -470,10 +551,29 @@ function translateRecord( rewrite: RewriteUrl, ): XmlEl[] | null { const attrs: Record = { - maxDuration: node.attrs.maxLength, - terminatingDigits: node.attrs.finishOnKey, + // 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. + maxDuration: clampAttr("Record", "maxDuration", node.attrs.maxLength ?? "3600", 1, 10800, findings), + silenceTimeout: clampAttr("Record", "silenceTimeout", node.attrs.timeout ?? "5", 0, 10800, findings), + terminatingDigits: recordTerminatingDigits(node.attrs.finishOnKey, findings), }; - if (node.attrs.action) attrs.recordCompleteUrl = rewrite(node.attrs.action, "record"); + if (node.attrs.action) { + attrs.recordCompleteUrl = rewrite(node.attrs.action, "record"); + } else if (currentDocumentUrl !== undefined) { + // Twilio re-requests the current document's URL when no action is given. + attrs.recordCompleteUrl = rewrite(currentDocumentUrl, "record"); + } else { + // Standalone BXML generation has no server to re-request a document from. + warn( + "Record", + "Record without an action attribute re-requests the current document URL on Twilio; set an explicit action for identical behavior through the translator.", + findings, + ); + } // 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 +594,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/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..3fd55ed 100644 --- a/test/server-continue.test.ts +++ b/test/server-continue.test.ts @@ -529,6 +529,52 @@ 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")); + }); +}); + // ─── Gather empty result ───────────────────────────────────────────────────── describe("/bw/continue gather with no input", () => { diff --git a/test/translate-gather-record.test.ts b/test/translate-gather-record.test.ts index 90a946b..510feef 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,105 @@ 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("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("clamping to Bandwidth's ranges (VAPI-4042)", () => { + it("clamps an out-of-range timeout 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"); + expect(warnings.some((f) => /firstDigitTimeout="100"/.test(f.message))).toBe(true); + expect(warnings.some((f) => /interDigitTimeout="100"/.test(f.message))).toBe(true); + }); + + it("clamps interDigitTimeout's floor (1) separately from firstDigitTimeout's floor (0)", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + // firstDigitTimeout="0" is in range (0-60): no clamp. + expect(r.bxml).toContain(`firstDigitTimeout="0"`); + // interDigitTimeout's minimum is 1: timeout="0" is clamped up. + expect(r.bxml).toContain(`interDigitTimeout="1"`); + expect( + r.findings.some((f) => f.severity === "warning" && /interDigitTimeout="0"/.test(f.message)), + ).toBe(true); + expect(r.findings.some((f) => /firstDigitTimeout="0"/.test(f.message))).toBe(false); + }); + + it("clamps numDigits above 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([]); + }); + }); + + 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. + const call = calls.find((c) => c.kind === "action"); + expect(call?.ctx).toEqual({ gatherIndex: 1, actionOnEmptyResult: false }); + }); + + 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 +158,135 @@ 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("clamping to Bandwidth's ranges (VAPI-4042)", () => { + it("clamps maxLength above Bandwidth's 10800s (3h) maximum", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`maxDuration="10800"`); + expect( + r.findings.some((f) => f.severity === "warning" && /maxDuration="14400"/.test(f.message)), + ).toBe(true); + }); + + it("clamps timeout (silenceTimeout) above Bandwidth's 10800s maximum", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).toContain(`silenceTimeout="10800"`); + }); + }); + + 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); + }); + }); + + 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, + ); + }); + }); }); From 2f8750fb126b4a8a9a795b9475bc9c28a3da4130 Mon Sep 17 00:00:00 2001 From: Madhu Ramasubramanian Date: Tue, 13 Oct 2026 11:28:31 -0400 Subject: [PATCH 2/5] VAPI-4042: use plain English in customer-facing text and test names The warning a customer sees when a value is outside Bandwidth's range said "clamped to X" instead of plainly saying what happened. It now says "using X instead". The matrix notes, AGENTS.md, and the new test names follow the same change ("kept within Bandwidth's limits" / "limits X to Bandwidth's maximum" rather than "clamped"). The clampAttr function name and its internal comments are unchanged; "clamp" was already an internal term in this file (the Say/Play loop count logic), so this only touches text a person actually reads. --- AGENTS.md | 5 +++-- src/matrix/twilio-voice.json | 8 ++++---- src/translator/translate.ts | 4 ++-- test/translate-gather-record.test.ts | 18 +++++++++--------- 4 files changed, 18 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9b30e73..0838514 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -127,8 +127,9 @@ Translation is a fixed rulebook (`src/matrix/twilio-voice.json`), not a guess. defaults are emitted explicitly rather than left to Bandwidth's own (different) defaults: `finishOnKey="#"`, and `timeout` (default 5) sets both `firstDigitTimeout` and `interDigitTimeout`. `numDigits`, `timeout`, - and `finishOnKey` (on `Record`, below) are clamped to Bandwidth's - documented ranges with a warning naming the original value. + and `finishOnKey` (on `Record`, below) are kept within the ranges + Bandwidth documents, with a warning naming the original value if it + had to change. - `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). A diff --git a/src/matrix/twilio-voice.json b/src/matrix/twilio-voice.json index 31d5bab..90a0c4c 100644 --- a/src/matrix/twilio-voice.json +++ b/src/matrix/twilio-voice.json @@ -34,8 +34,8 @@ "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=\"\". 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", "notes": "Clamped to Bandwidth's 1-50 range with a warning naming the original value; 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 clamped independently to its own range (firstDigitTimeout 0-60, interDigitTimeout 1-60) with a warning naming the original value." }, + "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, since Bandwidth's own default for this attribute is \"\" (disabled). finishOnKey=\"\" passes through unchanged (both systems treat it as disabled)." }, "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." } } @@ -82,8 +82,8 @@ "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", "notes": "Twilio's default (3600s) is emitted explicitly when omitted, since Bandwidth's own default is 60s. Clamped to Bandwidth's 10800s (3h) maximum with a warning naming the original value." }, - "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 clamped to Bandwidth's 10800s maximum. timeout=\"0\" (Twilio: disable) and silenceTimeout=\"0\" (Bandwidth: no timeout) agree, so it passes through unchanged." }, + "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; Twilio's finishOnKey is a set (default \"1234567890*#\", any key). A single character passes through unchanged; a multi-character set (including the omitted default) is narrowed to one digit, preferring '#' then '*' then the first character, with a warning." }, "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. 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)." }, diff --git a/src/translator/translate.ts b/src/translator/translate.ts index dd702d8..e06cb06 100644 --- a/src/translator/translate.ts +++ b/src/translator/translate.ts @@ -301,11 +301,11 @@ function clampAttr( const n = Number(value); if (!Number.isFinite(n)) return value; if (n < min) { - warn(verb, `${bxmlAttr}="${value}" is below Bandwidth's minimum of ${min}; clamped to ${min}.`, findings); + warn(verb, `${bxmlAttr}="${value}" is below Bandwidth's minimum of ${min}; using ${min} instead.`, findings); return String(min); } if (n > max) { - warn(verb, `${bxmlAttr}="${value}" exceeds Bandwidth's maximum of ${max}; clamped to ${max}.`, findings); + warn(verb, `${bxmlAttr}="${value}" is above Bandwidth's maximum of ${max}; using ${max} instead.`, findings); return String(max); } return value; diff --git a/test/translate-gather-record.test.ts b/test/translate-gather-record.test.ts index 510feef..69b361a 100644 --- a/test/translate-gather-record.test.ts +++ b/test/translate-gather-record.test.ts @@ -75,8 +75,8 @@ describe("Gather", () => { }); }); - describe("clamping to Bandwidth's ranges (VAPI-4042)", () => { - it("clamps an out-of-range timeout for both firstDigitTimeout and interDigitTimeout, with a warning for each", () => { + 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, }); @@ -87,13 +87,13 @@ describe("Gather", () => { expect(warnings.some((f) => /interDigitTimeout="100"/.test(f.message))).toBe(true); }); - it("clamps interDigitTimeout's floor (1) separately from firstDigitTimeout's floor (0)", () => { + it("enforces interDigitTimeout's minimum (1) separately from firstDigitTimeout's minimum (0)", () => { const r = translateTwiml(``, { rewriteUrl: rw, }); - // firstDigitTimeout="0" is in range (0-60): no clamp. + // firstDigitTimeout="0" is in range (0-60): left alone. expect(r.bxml).toContain(`firstDigitTimeout="0"`); - // interDigitTimeout's minimum is 1: timeout="0" is clamped up. + // 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" && /interDigitTimeout="0"/.test(f.message)), @@ -101,7 +101,7 @@ describe("Gather", () => { expect(r.findings.some((f) => /firstDigitTimeout="0"/.test(f.message))).toBe(false); }); - it("clamps numDigits above Bandwidth's 50-digit maximum", () => { + it("limits numDigits to Bandwidth's 50-digit maximum", () => { const r = translateTwiml(``, { rewriteUrl: rw, }); @@ -178,8 +178,8 @@ describe("Record", () => { }); }); - describe("clamping to Bandwidth's ranges (VAPI-4042)", () => { - it("clamps maxLength above Bandwidth's 10800s (3h) maximum", () => { + describe("keeping values within Bandwidth's limits (VAPI-4042)", () => { + it("limits maxLength to Bandwidth's 10800s (3h) maximum", () => { const r = translateTwiml(``, { rewriteUrl: rw, }); @@ -189,7 +189,7 @@ describe("Record", () => { ).toBe(true); }); - it("clamps timeout (silenceTimeout) above Bandwidth's 10800s maximum", () => { + it("limits timeout (silenceTimeout) to Bandwidth's 10800s maximum", () => { const r = translateTwiml(``, { rewriteUrl: rw, }); From 6ae633b3f88eaf63ff15bfbb70e1842ab780448e Mon Sep 17 00:00:00 2001 From: Madhu Ramasubramanian Date: Tue, 13 Oct 2026 16:21:44 -0400 Subject: [PATCH 3/5] VAPI-4042: fix empty/invalid value handling found in review - "" on timeout/maxLength was read as 0 (Number("") === 0), not as missing: cut voicemail to 1 second, and sent an empty firstDigitTimeout. Treated as missing now (clampAttr's callers use || instead of ?? for these). - A value that isn't a plain finite number ("abc", "Infinity") now warns and falls back to Twilio's default, instead of being copied into the BXML as-is for Bandwidth to reject. A value that IS a number but an unusual form ("0x3C", "1e1") is normalized to a plain decimal instead of being emitted as written. - Record's finishOnKey narrowing now only picks a real keypad character: "ab1" narrows to "1", not "a", and a value with no digit, "*", or "#" at all (e.g. "a") is dropped instead of passed through, so Bandwidth's own default applies. - Range warnings now name the Twilio attribute the customer actually wrote (timeout, maxLength) rather than only Bandwidth's name for it (firstDigitTimeout, maxDuration), while still saying which BXML attribute is affected. - Identical findings (verb + message) collapse to one per document, so a document with many ordinary bare Records doesn't bury a real out-of-range warning under repeats of the same generic one. - Corrected AGENTS.md: Bandwidth already defaults firstDigitTimeout and interDigitTimeout to 5s on its own, so an omitted Gather timeout isn't a behavior change; the real fix is that an explicit timeout previously only reached firstDigitTimeout. Also listed maxLength among the range-limited attributes and stopped calling finishOnKey's narrowing a range limit. - Pulled the duplicated "no action -> re-request the current document, else warn" branching out of translateGather/translateRecord into one resolveActionUrl helper. --- AGENTS.md | 45 ++++--- src/matrix/twilio-voice.json | 2 +- src/translator/translate.ts | 178 ++++++++++++++++++--------- test/translate-gather-record.test.ts | 106 +++++++++++++++- 4 files changed, 247 insertions(+), 84 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0838514..519e71f 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`, `playBeep`, and `trim` 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. @@ -123,13 +124,18 @@ Translation is a fixed rulebook (`src/matrix/twilio-voice.json`), not a guess. `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. A Gather with no `action` - attribute re-requests the current document, as on Twilio. Twilio's - defaults are emitted explicitly rather than left to Bandwidth's own - (different) defaults: `finishOnKey="#"`, and `timeout` (default 5) sets - both `firstDigitTimeout` and `interDigitTimeout`. `numDigits`, `timeout`, - and `finishOnKey` (on `Record`, below) are kept within the ranges - Bandwidth documents, with a warning naming the original value if it - had to change. + attribute re-requests the current document, as on Twilio. `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; 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. - `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). A @@ -137,12 +143,15 @@ Translation is a fixed rulebook (`src/matrix/twilio-voice.json`), not a guess. 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 defaults. `finishOnKey` is narrowed to - Bandwidth's single terminating digit (preferring `#`, then `*`, then the - first character) since Bandwidth cannot represent Twilio's default set of - any key. `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. + 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 with no + digit, `*`, or `#` at all is dropped rather than passed through, letting + Bandwidth's own default (`"#"`) apply. `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/matrix/twilio-voice.json b/src/matrix/twilio-voice.json index 90a0c4c..5961680 100644 --- a/src/matrix/twilio-voice.json +++ b/src/matrix/twilio-voice.json @@ -84,7 +84,7 @@ "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", "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; Twilio's finishOnKey is a set (default \"1234567890*#\", any key). A single character passes through unchanged; a multi-character set (including the omitted default) is narrowed to one digit, preferring '#' then '*' then the first character, with a warning." }, + "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 multi-character set (including the omitted 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) is dropped, falling back to Bandwidth's own default (\"#\"), rather than emitting a character Bandwidth doesn't recognize as a key." }, "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. 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)." } diff --git a/src/translator/translate.ts b/src/translator/translate.ts index e06cb06..b00b3f3 100644 --- a/src/translator/translate.ts +++ b/src/translator/translate.ts @@ -264,11 +264,30 @@ 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 verb+message within one + * document 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. + */ +function dedupeFindings(findings: Finding[]): Finding[] { + const seen = new Set(); + return findings.filter((f) => { + const key = `${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({ @@ -285,30 +304,71 @@ function warn(verb: string, message: string, findings: Finding[]): void { } /** - * Clamps a numeric TwiML attribute to the range Bandwidth documents for the - * BXML attribute it maps to, warning when the original value fell outside it. - * A non-numeric value is returned unchanged — validating shape isn't this - * function's job; Bandwidth will reject malformed BXML on its own. + * Keeps a numeric TwiML attribute within the range Bandwidth documents for + * the BXML attribute it maps to, warning when the original value had to + * change. Warnings name the Twilio attribute the customer actually wrote + * (not Bandwidth's name for it), so they can find it in their own TwiML. + * A value JavaScript's Number() can't turn into a finite number at all (empty, + * "abc", "Infinity") warns and falls back to `fallback` 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". Any value + * that IS a number (including forms Bandwidth almost certainly doesn't accept, + * like "0x3C" or "1e1") is normalized to its plain decimal form before being + * returned, in range or not, so the generated BXML never carries a hex or + * scientific-notation string. */ function clampAttr( verb: string, + twilioAttr: string, bxmlAttr: string, value: string, min: number, max: number, + fallback: string, findings: Finding[], ): string { const n = Number(value); - if (!Number.isFinite(n)) return value; + if (!Number.isFinite(n)) { + warn(verb, `${twilioAttr}="${value}" isn't a plain number Bandwidth can use for ${bxmlAttr}; using ${fallback} instead.`, findings); + return fallback; + } if (n < min) { - warn(verb, `${bxmlAttr}="${value}" is below Bandwidth's minimum of ${min}; using ${min} instead.`, findings); + warn(verb, `${twilioAttr}="${value}" is below Bandwidth's minimum for ${bxmlAttr} (${min}); using ${min} instead.`, findings); return String(min); } if (n > max) { - warn(verb, `${bxmlAttr}="${value}" is above Bandwidth's maximum of ${max}; using ${max} instead.`, findings); + warn(verb, `${twilioAttr}="${value}" is above Bandwidth's maximum for ${bxmlAttr} (${max}); using ${max} instead.`, findings); return String(max); } - return value; + return String(n); +} + +/** + * 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); + 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 @@ -450,19 +510,23 @@ function translateGather( ): 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. - // Leaving them unset would let Bandwidth's own defaults apply instead - // (terminatingDigits "" i.e. disabled, and no interDigitTimeout at all). - const timeout = node.attrs.timeout ?? "5"; + // 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. timeout="" is treated the + // same as omitted: Twilio doesn't accept a blank duration either. + const timeout = node.attrs.timeout || "5"; const attrs: Record = { maxDigits: node.attrs.numDigits - ? clampAttr("Gather", "numDigits", node.attrs.numDigits, 1, 50, findings) + ? clampAttr("Gather", "numDigits", "maxDigits", node.attrs.numDigits, 1, 50, "50", findings) : undefined, - firstDigitTimeout: clampAttr("Gather", "firstDigitTimeout", timeout, 0, 60, findings), - interDigitTimeout: clampAttr("Gather", "interDigitTimeout", timeout, 1, 60, findings), + firstDigitTimeout: clampAttr("Gather", "timeout", "firstDigitTimeout", timeout, 0, 60, "5", findings), + interDigitTimeout: clampAttr("Gather", "timeout", "interDigitTimeout", timeout, 1, 60, "5", findings), // Twilio's default finishOnKey is "#"; Bandwidth's own default for // terminatingDigits is "" (disabled), so the omitted case must be explicit. // finishOnKey="" (Twilio: disable) passes through unchanged, matching - // Bandwidth's own disable value. + // Bandwidth's own disable value — unlike timeout/maxLength above, an + // empty finishOnKey is a real, documented "off" value on both sides. terminatingDigits: node.attrs.finishOnKey ?? "#", }; // Twilio input: "dtmf" | "speech" | "dtmf speech". BW: dtmf | speech | dtmf_speech. @@ -485,19 +549,7 @@ function translateGather( // (Twilio continues with the next verb unless actionOnEmptyResult="true"). const gatherIndex = ++gatherSeq; const gatherCtx = { gatherIndex, actionOnEmptyResult: node.attrs.actionOnEmptyResult === "true" }; - if (node.attrs.action) { - attrs.gatherUrl = rewrite(node.attrs.action, "action", gatherCtx); - } else if (currentDocumentUrl !== undefined) { - // Twilio re-requests the current document's URL when no action is given. - attrs.gatherUrl = rewrite(currentDocumentUrl, "action", gatherCtx); - } else { - // Standalone BXML generation has no server to re-request a document from. - 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, - ); - } + 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); @@ -517,16 +569,23 @@ let currentTransferLegUrl: ((dialIndex: number) => string) | undefined; // (the server does; standalone BXML generation does not). See documentUrl. let currentDocumentUrl: string | undefined; +// The only characters Bandwidth's Record terminatingDigits can hold. +const RECORD_TERMINATING_KEYS = new Set([..."0123456789*#"]); + /** - * Bandwidth's Record terminatingDigits takes a single character, 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 omitted 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 - * character, since Bandwidth cannot represent a set. + * Bandwidth's Record terminatingDigits takes a single character from + * `RECORD_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 omitted 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. A value with no + * valid character at all (e.g. "a") can't be narrowed to anything, so it's + * dropped — letting Bandwidth's own default ("#") apply — rather than + * emitting a value Bandwidth doesn't recognize as a key. */ -function recordTerminatingDigits(finishOnKey: string | undefined, findings: Finding[]): string { +function recordTerminatingDigits(finishOnKey: string | undefined, findings: Finding[]): string | undefined { if (finishOnKey === undefined) { warn( "Record", @@ -535,8 +594,18 @@ function recordTerminatingDigits(finishOnKey: string | undefined, findings: Find ); return "#"; } - if (finishOnKey.length <= 1) return finishOnKey; // exact match: "", a digit, "*", or "#" - const chosen = finishOnKey.includes("#") ? "#" : finishOnKey.includes("*") ? "*" : finishOnKey[0]!; + if (finishOnKey === "") return ""; // both systems treat this as "disabled" + const valid = [...finishOnKey].filter((c) => RECORD_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; leaving it at Bandwidth's own default ("#") instead.`, + findings, + ); + return undefined; + } + if (finishOnKey.length === 1) return finishOnKey; // the one character is already valid: exact match + 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.`, @@ -550,30 +619,19 @@ function translateRecord( 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. Empty strings are + // treated the same as omitted: Twilio doesn't accept a blank duration either. const attrs: Record = { - // 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. - maxDuration: clampAttr("Record", "maxDuration", node.attrs.maxLength ?? "3600", 1, 10800, findings), - silenceTimeout: clampAttr("Record", "silenceTimeout", node.attrs.timeout ?? "5", 0, 10800, findings), + maxDuration: clampAttr("Record", "maxLength", "maxDuration", node.attrs.maxLength || "3600", 1, 10800, "3600", findings), + silenceTimeout: clampAttr("Record", "timeout", "silenceTimeout", node.attrs.timeout || "5", 0, 10800, "5", 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"); - } else if (currentDocumentUrl !== undefined) { - // Twilio re-requests the current document's URL when no action is given. - attrs.recordCompleteUrl = rewrite(currentDocumentUrl, "record"); - } else { - // Standalone BXML generation has no server to re-request a document from. - warn( - "Record", - "Record without an action attribute re-requests the current document URL on Twilio; set an explicit action for identical behavior through the translator.", - findings, - ); - } // 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. diff --git a/test/translate-gather-record.test.ts b/test/translate-gather-record.test.ts index 69b361a..a77cbab 100644 --- a/test/translate-gather-record.test.ts +++ b/test/translate-gather-record.test.ts @@ -83,8 +83,10 @@ describe("Gather", () => { expect(r.bxml).toContain(`firstDigitTimeout="60"`); expect(r.bxml).toContain(`interDigitTimeout="60"`); const warnings = r.findings.filter((f) => f.severity === "warning"); - expect(warnings.some((f) => /firstDigitTimeout="100"/.test(f.message))).toBe(true); - expect(warnings.some((f) => /interDigitTimeout="100"/.test(f.message))).toBe(true); + // 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)", () => { @@ -96,9 +98,9 @@ describe("Gather", () => { // 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" && /interDigitTimeout="0"/.test(f.message)), + r.findings.some((f) => f.severity === "warning" && /timeout="0".*interDigitTimeout/.test(f.message)), ).toBe(true); - expect(r.findings.some((f) => /firstDigitTimeout="0"/.test(f.message))).toBe(false); + expect(r.findings.some((f) => /firstDigitTimeout/.test(f.message))).toBe(false); }); it("limits numDigits to Bandwidth's 50-digit maximum", () => { @@ -120,6 +122,48 @@ describe("Gather", () => { 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("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); + }); }); describe("no action, re-requesting the current document (VAPI-4042)", () => { @@ -185,7 +229,7 @@ describe("Record", () => { }); expect(r.bxml).toContain(`maxDuration="10800"`); expect( - r.findings.some((f) => f.severity === "warning" && /maxDuration="14400"/.test(f.message)), + r.findings.some((f) => f.severity === "warning" && /maxLength="14400".*maxDuration/.test(f.message)), ).toBe(true); }); @@ -195,6 +239,22 @@ describe("Record", () => { }); expect(r.bxml).toContain(`silenceTimeout="10800"`); }); + + 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"`); + }); }); describe("finishOnKey -> a single terminatingDigit (VAPI-4042)", () => { @@ -231,6 +291,24 @@ describe("Record", () => { 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("drops the attribute and warns when there is no valid key at all, rather than emitting an invalid one", () => { + const r = translateTwiml(``, { + rewriteUrl: rw, + }); + expect(r.bxml).not.toContain("terminatingDigits"); + expect( + r.findings.some((f) => f.severity === "warning" && /finishOnKey="a"/.test(f.message)), + ).toBe(true); + }); }); describe("playBeep and trim (VAPI-4042)", () => { @@ -290,3 +368,21 @@ describe("Record", () => { }); }); }); + +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); + }); +}); From 44cb56c9b8288e34114df2a19d27a27187a1353d Mon Sep 17 00:00:00 2001 From: Madhu Ramasubramanian Date: Thu, 15 Oct 2026 13:24:27 -0400 Subject: [PATCH 4/5] VAPI-4042: fix a Gather-with-no-action replay loop, and other review findings - Most serious: a Gather with no action that reaches a server instance with no cached copy of its document (restart, a different replica) now ends the call instead of re-fetching and replaying the exact same prompt. Before this fix, the same conditions would recur every time, looping forever. A new RewriteContext.selfReference flag (carried as &self=1 on the URL) lets /bw/continue tell "this points at a real customer action" apart from "this points at myself". - A whitespace-only value (e.g. " ") hit the same Number("")===0 bug as an empty one; both are now treated as not set. - Gather's finishOnKey had no validation at all: "ab" (no real keypad key) went out unchanged. It's now filtered to the characters Bandwidth recognizes (0-9, *, #), same as Record already was. - An invalid Record finishOnKey (no real key at all) now disables the stop key ("") instead of falling back to Bandwidth's own default ("#"), which would have given a customer's TwiML a stop key they never asked for. - translate.ts's dedupeFindings and compatibility-check/analyze.ts's own separate dedup logic disagreed on what counts as a duplicate (one included severity, the other didn't). Now there's one shared, exported dedupeFindings both use. - Checked numDigits="2.5" against `band bxml raw` directly: Bandwidth accepts decimal values for these attributes (its own docs say so), so this is not a bug. - Re-confirmed against Bandwidth's live docs that firstDigitTimeout and interDigitTimeout both already default to 5s on Bandwidth's side, same as previously corrected. - Left the module-level translation state (now six variables reset per call, including the new one from this PR) as a follow-up: threading a context object through every verb-translation function instead is a much larger, separate change from this PR's scope. --- src/compatibility-check/analyze.ts | 5 +- src/server/app.ts | 34 +++++++- src/translator/translate.ts | 118 +++++++++++++++++++-------- test/compatibility-check.test.ts | 10 +++ test/server-continue.test.ts | 45 ++++++++++ test/translate-gather-record.test.ts | 64 ++++++++++++++- 6 files changed, 231 insertions(+), 45 deletions(-) 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/server/app.ts b/src/server/app.ts index c7be285..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. @@ -452,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) ?? @@ -478,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 b00b3f3..7b940a3 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; @@ -270,18 +279,21 @@ export function translateTwiml(twiml: string, opts: TranslateOptions = {}): Tran } /** - * Collapses findings that repeat the exact same verb+message within one - * document 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 + * 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. */ -function dedupeFindings(findings: Finding[]): Finding[] { +export function dedupeFindings(findings: Finding[]): Finding[] { const seen = new Set(); return findings.filter((f) => { - const key = `${f.verb}\u0000${f.message}`; + const key = `${f.severity}\u0000${f.verb}\u0000${f.message}`; if (seen.has(key)) return false; seen.add(key); return true; @@ -303,6 +315,20 @@ 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); +} + /** * Keeps a numeric TwiML attribute within the range Bandwidth documents for * the BXML attribute it maps to, warning when the original value had to @@ -362,7 +388,7 @@ function resolveActionUrl( ctx?: RewriteContext, ): string | undefined { if (action) return rewrite(action, kind, ctx); - if (currentDocumentUrl !== undefined) return rewrite(currentDocumentUrl, 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.`, @@ -513,21 +539,17 @@ function translateGather( // 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. timeout="" is treated the - // same as omitted: Twilio doesn't accept a blank duration either. - const timeout = node.attrs.timeout || "5"; + // 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. + const timeout = orDefault(node.attrs.timeout, "5"); const attrs: Record = { - maxDigits: node.attrs.numDigits - ? clampAttr("Gather", "numDigits", "maxDigits", node.attrs.numDigits, 1, 50, "50", findings) - : undefined, + maxDigits: isBlank(node.attrs.numDigits) + ? undefined + : clampAttr("Gather", "numDigits", "maxDigits", node.attrs.numDigits as string, 1, 50, "50", findings), firstDigitTimeout: clampAttr("Gather", "timeout", "firstDigitTimeout", timeout, 0, 60, "5", findings), interDigitTimeout: clampAttr("Gather", "timeout", "interDigitTimeout", timeout, 1, 60, "5", findings), - // Twilio's default finishOnKey is "#"; Bandwidth's own default for - // terminatingDigits is "" (disabled), so the omitted case must be explicit. - // finishOnKey="" (Twilio: disable) passes through unchanged, matching - // Bandwidth's own disable value — unlike timeout/maxLength above, an - // empty finishOnKey is a real, documented "off" value on both sides. - terminatingDigits: node.attrs.finishOnKey ?? "#", + terminatingDigits: gatherTerminatingDigits(node.attrs.finishOnKey, findings), }; // Twilio input: "dtmf" | "speech" | "dtmf speech". BW: dtmf | speech | dtmf_speech. if (node.attrs.input) { @@ -569,23 +591,50 @@ let currentTransferLegUrl: ((dialIndex: number) => string) | undefined; // (the server does; standalone BXML generation does not). See documentUrl. let currentDocumentUrl: string | undefined; -// The only characters Bandwidth's Record terminatingDigits can hold. -const RECORD_TERMINATING_KEYS = new Set([..."0123456789*#"]); +// 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*#"]); + +/** + * 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 omitted case + * doesn't need a warning. + */ +function gatherTerminatingDigits(finishOnKey: string | undefined, findings: Finding[]): string { + if (finishOnKey === undefined) return "#"; + if (finishOnKey === "") return ""; // both systems treat this as "disabled" + 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 - * `RECORD_TERMINATING_KEYS`, unlike Gather's set of acceptable characters and + * `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 omitted 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. A value with no - * valid character at all (e.g. "a") can't be narrowed to anything, so it's - * dropped — letting Bandwidth's own default ("#") apply — rather than - * emitting a value Bandwidth doesn't recognize as a key. + * 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 | undefined { +function recordTerminatingDigits(finishOnKey: string | undefined, findings: Finding[]): string { if (finishOnKey === undefined) { warn( "Record", @@ -595,14 +644,14 @@ function recordTerminatingDigits(finishOnKey: string | undefined, findings: Find return "#"; } if (finishOnKey === "") return ""; // both systems treat this as "disabled" - const valid = [...finishOnKey].filter((c) => RECORD_TERMINATING_KEYS.has(c)); + 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; leaving it at Bandwidth's own default ("#") instead.`, + `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 undefined; + return ""; } if (finishOnKey.length === 1) return finishOnKey; // the one character is already valid: exact match const chosen = valid.includes("#") ? "#" : valid.includes("*") ? "*" : valid[0]!; @@ -624,11 +673,12 @@ function translateRecord( // 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. Empty strings are - // treated the same as omitted: Twilio doesn't accept a blank duration either. + // 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 attrs: Record = { - maxDuration: clampAttr("Record", "maxLength", "maxDuration", node.attrs.maxLength || "3600", 1, 10800, "3600", findings), - silenceTimeout: clampAttr("Record", "timeout", "silenceTimeout", node.attrs.timeout || "5", 0, 10800, "5", findings), + maxDuration: clampAttr("Record", "maxLength", "maxDuration", orDefault(node.attrs.maxLength, "3600"), 1, 10800, "3600", findings), + silenceTimeout: clampAttr("Record", "timeout", "silenceTimeout", orDefault(node.attrs.timeout, "5"), 0, 10800, "5", findings), terminatingDigits: recordTerminatingDigits(node.attrs.finishOnKey, findings), recordCompleteUrl: resolveActionUrl("Record", node.attrs.action, "record", rewrite, findings), }; 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/server-continue.test.ts b/test/server-continue.test.ts index 3fd55ed..c964087 100644 --- a/test/server-continue.test.ts +++ b/test/server-continue.test.ts @@ -573,6 +573,51 @@ describe("Gather/Record with no action re-request the current document, like Twi 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(" { 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("emits timeout's Twilio default (5) as both firstDigitTimeout and interDigitTimeout when omitted", () => { const r = translateTwiml(``, { rewriteUrl: rw }); expect(r.bxml).toContain(`firstDigitTimeout="5"`); @@ -135,6 +154,25 @@ describe("Gather", () => { 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. @@ -176,9 +214,10 @@ describe("Gather", () => { 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. + // 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 }); + 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)", () => { @@ -255,6 +294,20 @@ describe("Record", () => { }); 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)", () => { @@ -300,11 +353,14 @@ describe("Record", () => { expect(r.bxml).not.toContain(`terminatingDigits="a"`); }); - it("drops the attribute and warns when there is no valid key at all, rather than emitting an invalid one", () => { + 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).not.toContain("terminatingDigits"); + expect(r.bxml).toContain(`terminatingDigits=""`); expect( r.findings.some((f) => f.severity === "warning" && /finishOnKey="a"/.test(f.message)), ).toBe(true); From a90483067932723097bfc481dece79aa7c060929 Mon Sep 17 00:00:00 2001 From: Madhu Ramasubramanian Date: Fri, 16 Oct 2026 12:02:42 -0400 Subject: [PATCH 5/5] VAPI-4042: fix scientific notation, duplicate warnings, and stale docs - A very small value (e.g. timeout="0.0000001") came out as firstDigitTimeout="1e-7" -- String(n) switches to scientific notation below 1e-6, contradicting this code's own claim that it never would. Fixed with a dedicated plain-decimal formatter. - numDigits is now rounded to a whole number with a warning (numDigits="2.5" -> "3"): there's no such thing as collecting half a digit. timeout/maxLength keep a fractional value as written -- a fractional duration is a real concept, and Bandwidth's own docs allow decimal values there (confirmed: band bxml raw accepts them). - A non-numeric Gather timeout (e.g. "abc") produced two near-identical warnings, one per BXML attribute it maps to. Numeric validation now happens once per Twilio attribute, range-checking happens separately, so it warns once. - Gather's finishOnKey="##" warned about "narrowing" even though "#" is kept exactly -- nothing was actually lost. Now only warns when there's a genuine choice between distinct keys or an invalid character is being dropped. - A whitespace-only finishOnKey (e.g. " ") turned the stop key off, unlike every other blank attribute, which falls back to Twilio's default. Now treated the same way as omitted on both verbs. - matrix.json and AGENTS.md still described Record's invalid-finishOnKey case as "dropped, falling back to Bandwidth's default (#)" -- stale from before the previous round changed that to disabling the stop key instead. Corrected, and extended both to cover the Gather character-filtering and single-warning behavior these docs were missing entirely. - Documented two things explicitly rather than silently fixing them a fourth time: a bare Gather's no-document-to-resume fallback ends the call rather than loop, which is a real behavior gap from Twilio on a multi-replica deployment with no session affinity -- flagged in AGENTS.md as worth a shared document cache if it matters for a given deployment. And the module-level translation state, now flagged three times across three reviews, is left as a deliberate follow-up in a code comment explaining why: the real fix (a context object threaded through every verb function) is a larger, separate change than felt safe to rush right before merge. --- AGENTS.md | 39 +++++-- src/matrix/twilio-voice.json | 4 +- src/translator/translate.ts | 151 ++++++++++++++++++++------- test/translate-gather-record.test.ts | 64 ++++++++++++ 4 files changed, 207 insertions(+), 51 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 519e71f..30bb0fb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -123,8 +123,17 @@ 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. A Gather with no `action` - attribute re-requests the current document, as on Twilio. `finishOnKey`'s + 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 @@ -133,9 +142,15 @@ Translation is a fixed rulebook (`src/matrix/twilio-voice.json`), not a guess. `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; 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. + 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). A @@ -147,11 +162,15 @@ Translation is a fixed rulebook (`src/matrix/twilio-voice.json`), not a guess. 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 with no - digit, `*`, or `#` at all is dropped rather than passed through, letting - Bandwidth's own default (`"#"`) apply. `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. + 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/matrix/twilio-voice.json b/src/matrix/twilio-voice.json index 5961680..531c0a5 100644 --- a/src/matrix/twilio-voice.json +++ b/src/matrix/twilio-voice.json @@ -36,7 +36,7 @@ }, "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, since Bandwidth's own default for this attribute is \"\" (disabled). finishOnKey=\"\" passes through unchanged (both systems treat it as disabled)." }, + "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." } } }, @@ -84,7 +84,7 @@ "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", "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 multi-character set (including the omitted 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) is dropped, falling back to Bandwidth's own default (\"#\"), rather than emitting a character Bandwidth doesn't recognize as a key." }, + "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. 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)." } diff --git a/src/translator/translate.ts b/src/translator/translate.ts index 7b940a3..53f3ce5 100644 --- a/src/translator/translate.ts +++ b/src/translator/translate.ts @@ -330,35 +330,72 @@ function orDefault(value: string | undefined, fallback: string): string { } /** - * Keeps a numeric TwiML attribute within the range Bandwidth documents for - * the BXML attribute it maps to, warning when the original value had to - * change. Warnings name the Twilio attribute the customer actually wrote - * (not Bandwidth's name for it), so they can find it in their own TwiML. - * A value JavaScript's Number() can't turn into a finite number at all (empty, - * "abc", "Infinity") warns and falls back to `fallback` 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". Any value - * that IS a number (including forms Bandwidth almost certainly doesn't accept, - * like "0x3C" or "1e1") is normalized to its plain decimal form before being - * returned, in range or not, so the generated BXML never carries a hex or - * scientific-notation 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 clampAttr( +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, - 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 for ${bxmlAttr}; using ${fallback} instead.`, findings); - return fallback; - } if (n < min) { warn(verb, `${twilioAttr}="${value}" is below Bandwidth's minimum for ${bxmlAttr} (${min}); using ${min} instead.`, findings); return String(min); @@ -367,7 +404,7 @@ function clampAttr( warn(verb, `${twilioAttr}="${value}" is above Bandwidth's maximum for ${bxmlAttr} (${max}); using ${max} instead.`, findings); return String(max); } - return String(n); + return value; } /** @@ -541,14 +578,24 @@ function translateGather( // 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. - const timeout = orDefault(node.attrs.timeout, "5"); + // 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: isBlank(node.attrs.numDigits) ? undefined - : clampAttr("Gather", "numDigits", "maxDigits", node.attrs.numDigits as string, 1, 50, "50", findings), - firstDigitTimeout: clampAttr("Gather", "timeout", "firstDigitTimeout", timeout, 0, 60, "5", findings), - interDigitTimeout: clampAttr("Gather", "timeout", "interDigitTimeout", timeout, 1, 60, "5", findings), + : 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. @@ -589,12 +636,31 @@ let dialSeq = 0; 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 @@ -603,12 +669,12 @@ const VALID_TERMINATING_KEYS = new Set([..."0123456789*#"]); * 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 omitted case + * partially matches — see recordTerminatingDigits), so the not-set case * doesn't need a warning. */ function gatherTerminatingDigits(finishOnKey: string | undefined, findings: Finding[]): string { - if (finishOnKey === undefined) return "#"; 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( @@ -624,18 +690,21 @@ function gatherTerminatingDigits(finishOnKey: string | undefined, findings: Find * `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 omitted case is a partial match: only "#" stops it here, + * 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. 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. + * 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 === undefined) { + 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.`, @@ -643,7 +712,6 @@ function recordTerminatingDigits(finishOnKey: string | undefined, findings: Find ); return "#"; } - if (finishOnKey === "") return ""; // both systems treat this as "disabled" const valid = [...finishOnKey].filter((c) => VALID_TERMINATING_KEYS.has(c)); if (valid.length === 0) { warn( @@ -653,7 +721,10 @@ function recordTerminatingDigits(finishOnKey: string | undefined, findings: Find ); return ""; } - if (finishOnKey.length === 1) return finishOnKey; // the one character is already valid: exact match + // 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", @@ -676,9 +747,11 @@ function translateRecord( // 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: clampAttr("Record", "maxLength", "maxDuration", orDefault(node.attrs.maxLength, "3600"), 1, 10800, "3600", findings), - silenceTimeout: clampAttr("Record", "timeout", "silenceTimeout", orDefault(node.attrs.timeout, "5"), 0, 10800, "5", findings), + 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), }; diff --git a/test/translate-gather-record.test.ts b/test/translate-gather-record.test.ts index 28ada31..74d97f4 100644 --- a/test/translate-gather-record.test.ts +++ b/test/translate-gather-record.test.ts @@ -82,6 +82,14 @@ describe("Gather", () => { 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"`); @@ -202,6 +210,34 @@ describe("Gather", () => { 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)", () => { @@ -279,6 +315,14 @@ describe("Record", () => { 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. @@ -365,6 +409,26 @@ describe("Record", () => { 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)", () => {