fix(voice): strip break and markup tags before adding say() text to chat_ctx (#4802) - #7251
fix(voice): strip break and markup tags before adding say() text to chat_ctx (#4802)#7251rrfunde wants to merge 9 commits into
Conversation
…hat_ctx (livekit#4802) Prevent <break> SSML and provider markup tags from leaking into ChatContext assistant messages when using say().
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d568d1e63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| speech_handle._item_added([msg]) | ||
| self._session._conversation_item_added(msg) | ||
| clean_text = strip_all_markup(forwarded_text).strip() |
There was a problem hiding this comment.
When valid SSML places a pause directly between words, such as session.say('Hello<break time="1s"/>world'), strip_all_markup() removes the self-closing tag without inserting a separator, so the stored assistant message becomes Helloworld even though the user heard “Hello … world.” This corrupts the conversation history for a common tag layout; preserve a word boundary when removing a break between non-whitespace characters.
Useful? React with 👍 / 👎.
| ) | ||
| speech_handle._item_added([msg]) | ||
| self._session._conversation_item_added(msg) | ||
| clean_text = strip_all_markup(forwarded_text).strip() |
There was a problem hiding this comment.
For providers supporting broader SSML, this sanitizer still leaves markup in history because strip_all_markup() only recognizes the expressive-provider tag union. For example, ElevenLabs explicitly accepts <phoneme> when SSML parsing is enabled (livekit-plugins/livekit-plugins-elevenlabs/livekit/plugins/elevenlabs/tts.py:513), but session.say('<phoneme alphabet="ipa" ph="...">tomato</phoneme>') stores that entire tag unchanged and therefore retains the same few-shot leakage this change is intended to prevent. The cleanup needs to cover supported SSML such as phoneme tags, preferably according to the active provider.
Useful? React with 👍 / 👎.
… tags for chat context
|
Addressed the review feedback:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f37313ad5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Unwrap SSML wrapping tags to keep inner text | ||
| while True: | ||
| unwrapped = _SSML_WRAPPING_RE.sub(r"\2", text) |
There was a problem hiding this comment.
When session.say() receives valid adjacent paragraph or sentence elements, such as <p>Hello</p><p>world</p>, the new wrapper substitution replaces each element with only its inner text and stores Helloworld in the chat context even though the TTS renders a structural boundary. Handle <p> and <s> separately by inserting a separator rather than treating them like inline wrappers.
Useful? React with 👍 / 👎.
|
|
||
| _SSML_BREAK_RE = re.compile(r"<\s*/?\s*break\b[^>]*\/?>", re.IGNORECASE) | ||
| _SSML_WRAPPING_RE = re.compile( | ||
| r"<\s*(?P<tag>phoneme|sub|say-as|prosody|emphasis|voice|lang|speak|p|s|w|audio|mstts:[a-zA-Z0-9_-]+)\b[^>]*>(.*?)</\s*(?P=tag)\s*>", |
There was a problem hiding this comment.
When SSML speech is interrupted before its closing tag is forwarded, the synchronized transcript can end with text such as <phoneme alphabet="ipa" ph="...">tomato. The new regex requires a matching closing tag, while the standalone pass excludes phoneme, prosody, say-as, and most other wrappers, so the opening markup is still stored and can few-shot later LLM turns. After balanced unwrapping, strip lone recognized SSML delimiters as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
|
|
||
| # Replace structural SSML tags (<p>, <s>) with inner text + space separator | ||
| while True: | ||
| replaced = _SSML_STRUCTURAL_RE.sub(r"\2 ", text) |
There was a problem hiding this comment.
🟡 Structural tags corrupt trailing punctuation
When punctuation follows a closed <p> or <s>, strip_chat_markup inserts a space before it. Chat history changes Hello! into Hello !, few-shotting malformed punctuation into later responses.
Learn more
Structural tags need a separator only when removing the boundary would concatenate surrounding words. The unconditional trailing space also separates punctuation and contractions from their preceding text. This sanitized value becomes the assistant message in the TTS task, so the modified punctuation persists in conversation history.
Example: strip_chat_markup("<p>Hello</p>!") returns "Hello !"; the expected plain text is "Hello!". Adjacent blocks such as <p>Hello</p><p>world</p> still require "Hello world".
Recommended fix: Preserve a boundary conditionally. Add whitespace only when the next structural block or surrounding plain text would otherwise join two word characters, while leaving punctuation attached.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cac0d13e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| re.IGNORECASE | re.DOTALL, | ||
| ) | ||
| _SSML_WRAPPING_RE = re.compile( | ||
| r"<\s*(?P<tag>phoneme|sub|say-as|prosody|emphasis|voice|lang|speak|w|audio|mstts:[a-zA-Z0-9_-]+)\b[^>]*>(.*?)</\s*(?P=tag)\s*>", |
There was a problem hiding this comment.
When the AWS TTS plugin is configured with text_type="ssml", _run() forwards the session text unchanged as Polly SSML (livekit-plugins/livekit-plugins-aws/livekit/plugins/aws/tts.py:163-170). This new fixed whitelist recognizes only common unprefixed tags and mstts:*, so a supported call using an Amazon tag, such as <amazon:effect name="drc">Breaking news</amazon:effect>, is still stored verbatim in chat_ctx, preserving the markup leakage this change is intended to eliminate. The newly introduced whitelist is fresh evidence that the previously reported broad-SSML issue remains for provider-specific dialects.
Useful? React with 👍 / 👎.
|
Fixed:
|
|
Addressed review feedback: generalized SSML tag pattern to include namespaced provider tags ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7798379c73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| re.IGNORECASE | re.DOTALL, | ||
| ) | ||
| _SSML_STANDALONE_RE = re.compile( | ||
| rf"<\s*/?\s*(?:speak|p|s|mark|{_SSML_TAG_PATTERN})\b[^>]*\/?>", |
There was a problem hiding this comment.
When say() is used with a plain-text TTS configuration or text-only output, this pattern still treats ordinary HTML/XML examples as SSML because strip_chat_markup() receives no provider or SSML-mode information. For example, session.say("Use <p> and </p> tags") is forwarded to the user with the tags intact but stored as Use and tags, causing subsequent LLM turns to see content different from what was delivered; only strip tags that the active output actually interprets as markup.
Useful? React with 👍 / 👎.
| r"[a-zA-Z][a-zA-Z0-9_-]*:[a-zA-Z0-9_-]+" | ||
| ) | ||
| _SSML_WRAPPING_RE = re.compile( | ||
| rf"<\s*(?P<tag>{_SSML_TAG_PATTERN})\b[^>]*>(.*?)</\s*(?P=tag)\s*>", |
There was a problem hiding this comment.
For valid SSML whose quoted attribute contains >, the opening-tag match ends at that character rather than at the actual delimiter. For example, <sub alias="2 > 1">comparison</sub> is sanitized to 1">comparison instead of comparison, leaving attribute fragments in chat history; parse quoted attributes without treating their angle brackets as the end of the tag.
Useful? React with 👍 / 👎.
…eral markup when SSML disabled
|
Addressed review feedback:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36fc2e509d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if provider in ("azure", "cartesia"): | ||
| return True |
There was a problem hiding this comment.
When audio output uses Azure TTS, this check never enables SSML handling because the plugin exposes provider == "Azure TTS" (livekit-plugins/livekit-plugins-azure/livekit/plugins/azure/tts.py:217) rather than "azure", and its options contain none of the preceding SSML flags. Azure always embeds the supplied text into an SSML <speak><voice> document (tts.py:262-293), so input such as <p>Hello</p><p>world</p> is interpreted structurally during synthesis but remains verbatim in chat history. Normalize the exposed provider value or use an explicit capability instead of comparing against these lowercase internal keys.
Useful? React with 👍 / 👎.
|
Addressed review feedback:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78d360700
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Unwrap inline SSML wrapping tags to keep inner text | ||
| while True: | ||
| unwrapped = _SSML_WRAPPING_RE.sub(r"\2", text) |
There was a problem hiding this comment.
<sub> elements
When SSML contains <sub alias="World Wide Web Consortium">W3C</sub>, the TTS speaks the alias, but this generic unwrapping stores W3C in the assistant history. This makes subsequent turns reason over text different from what the user heard; handle <sub> separately and store its decoded alias value rather than its inner text.
Useful? React with 👍 / 👎.
| if any(tag in text_lower for tag in ("<phoneme", "<prosody", "<say-as")): | ||
| return True |
There was a problem hiding this comment.
For text-only output or a plain-text TTS, prose such as Explain <phoneme alphabet="ipa">tomato</phoneme> is delivered with the literal tags, but this content-based check declares it SSML and removes them from chat history. The new auto-detection heuristic is fresh evidence that the previously reported plain-markup mismatch remains for these tags; SSML mode should come from the active output configuration rather than from the text being sanitized.
Useful? React with 👍 / 👎.
| opts = getattr(tts, "_opts", None) | ||
| if opts is not None: | ||
| if getattr(opts, "text_type", None) == "ssml": |
There was a problem hiding this comment.
When an SSML-enabled engine is supplied through tts.FallbackAdapter or an explicit tts.StreamAdapter, this examines the adapter's _opts rather than the wrapped engine's options. For example, FallbackAdapter([google.TTS(enable_ssml=True, use_streaming=False)]) interprets <p>Hello</p><p>world</p> as SSML, while this function sees neither enable_ssml nor a recognized provider and leaves the tags in history; delegate an explicit SSML capability through these standard adapters.
Useful? React with 👍 / 👎.
…ove SSML text heuristics
|
Addressed review feedback:
|
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 3 new potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| tts=self.tts | ||
| if (self._session.output.audio_enabled and self.tts is not None) | ||
| else None, |
There was a problem hiding this comment.
🟡 Supplied-audio transcript markup corruption
strip_chat_markup treats supplied-audio text as SSML whenever the session's unused TTS supports it. Literal <p> and <s> content disappears from assistant history although supplied audio bypasses synthesis.
Learn more
A say() call can provide its own audio plus text used for transcription and chat history. That path skips perform_tts_inference, but cleanup currently consults the session TTS anyway. An SSML-capable configured TTS therefore changes text it never synthesized.
Example: A session configured with Azure calls say("Use <p> and </p> tags", audio=recording). The recording plays directly, but the stored assistant message becomes "Use and tags" instead of preserving the supplied transcript.
Recommended fix: Pass the TTS to strip_chat_markup only when audio is None and synthesis actually uses the configured TTS.
| tts=self.tts | |
| if (self._session.output.audio_enabled and self.tts is not None) | |
| else None, | |
| tts=self.tts | |
| if ( | |
| audio is None | |
| and self._session.output.audio_enabled | |
| and self.tts is not None | |
| ) | |
| else None, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Provider-specific namespaced tags (e.g. Amazon Polly, Azure) are always speech markup | ||
| while True: | ||
| unwrapped = _SSML_NAMESPACED_WRAPPING_RE.sub(r"\2", text) | ||
| if unwrapped == text: | ||
| break | ||
| text = unwrapped | ||
| text = _SSML_NAMESPACED_STANDALONE_RE.sub(" ", text) | ||
| text = _SSML_NAMESPACED_INCOMPLETE_RE.sub("", text) |
There was a problem hiding this comment.
🟡 Literal namespaced XML disappears
strip_chat_markup removes every namespaced XML element even when SSML is disabled. Spoken XML examples lose tags and standalone elements in assistant history.
Learn more
Namespaced syntax is not sufficient to identify speech markup. The pattern accepts arbitrary names such as foo:bar, and these substitutions run before _is_ssml_enabled. Standard tags receive an SSML gate, but arbitrary namespaced tags do not.
Example: session.say("Use <foo:bar>value</foo:bar> in the document") stores "Use value in the document", even in a text-only session. The expected history preserves the XML example because no SSML mode was selected.
Recommended fix: Gate namespaced cleanup on confirmed SSML input, or restrict unconditional cleanup to a documented set of provider speech namespaces with explicit signaling for ambiguous text.
Was this helpful? React with 👍 or 👎 to provide feedback.
| instances = getattr(curr, "_tts_instances", None) | ||
| if instances is not None and isinstance(instances, (list, tuple)): | ||
| to_check.extend(instances) |
There was a problem hiding this comment.
🟡 Inactive fallback enables SSML stripping
_is_ssml_enabled returns true when any fallback TTS supports SSML, even if a plain-text provider serves the request. Literal standard tags then vanish from assistant history.
Learn more
A fallback adapter selects one provider per request and records the provider that actually served it as _active_instance. Scanning every configured instance treats their input formats as a union. That does not describe the provider that produced the audio.
Example: A fallback list contains OpenAI first and Azure second. OpenAI successfully speaks "Use <p> and </p> tags" as plain text, but Azure's presence enables SSML cleanup and stores "Use and tags".
Recommended fix: Determine cleanup from the provider that served the completed request. For fallback adapters, inspect _active_instance after synthesis rather than enabling every member's input dialect.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48804e256c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| instances = getattr(curr, "_tts_instances", None) | ||
| if instances is not None and isinstance(instances, (list, tuple)): | ||
| to_check.extend(instances) |
There was a problem hiding this comment.
When a FallbackAdapter mixes engines with different SSML modes—for example, a Google primary with enable_ssml=False and an SSML-enabled fallback—scanning every _tts_instances entry returns true even when the plain-text primary produced the audio. Standard tags are then removed from history despite being delivered as literal text; the reverse ordering also misrepresents output after failover. The newly added traversal of all fallback instances is fresh evidence beyond the earlier wrapped-adapter report; determine the mode from the engine that served this request rather than any possible fallback.
Useful? React with 👍 / 👎.
| # Collapse any introduced horizontal whitespace runs | ||
| text = re.sub(r"[^\S\r\n]+", " ", text) | ||
| return text.strip() |
There was a problem hiding this comment.
When valid SSML contains escaped text such as <speak>AT&T</speak> or an escaped <sub> alias, the TTS XML parser speaks the decoded characters, but this function returns AT&T unchanged after removing the wrappers. That leaves entity markup in assistant history and makes later turns see text different from what was spoken; decode XML character and numeric references in retained SSML text and aliases before returning it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9db9405: decoded XML character and numeric references (html.unescape) in retained SSML text and substituted aliases when SSML is active, while keeping XML references untouched when SSML is disabled. Added unit tests.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
|
|
||
| # Decode XML character and numeric references in retained SSML text and aliases | ||
| if is_ssml or had_namespaced: | ||
| text = html.unescape(text) |
There was a problem hiding this comment.
🟡 C1 character references change value
For C1 numeric references, html.unescape applies HTML’s Windows-1252 remapping instead of XML decoding. Chat history then records different characters from the SSML text sent to synthesis.
Learn more
SSML uses XML character-reference rules, while html.unescape implements HTML5 parsing. HTML5 remaps numeric references from U+0080 through U+009F using the Windows-1252 table. XML preserves those valid code points, so the sanitizer can no longer represent the text passed to the TTS provider.
Example: strip_chat_markup("<speak>€</speak>") returns €. An XML processor resolves € to U+0080 instead, so the stored assistant message differs from the synthesized input.
Recommended fix: Decode only XML’s five predefined named entities and numeric references. Validate numeric code points against XML’s allowed character ranges, and preserve or reject invalid references consistently.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9db9405b55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tts=self.tts | ||
| if (self._session.output.audio_enabled and self.tts is not None) | ||
| else None, |
There was a problem hiding this comment.
When session.say(..., audio=frames) is used while an SSML-capable TTS such as Azure is configured, _tts_task_impl takes the audio is not None branch and never sends the text to that TTS, but this still passes self.tts to the sanitizer. Literal transcript text such as Use <p> and </p> is consequently rewritten in chat history according to a model that did not synthesize the supplied audio; only infer SSML mode from self.tts when audio is None.
Useful? React with 👍 / 👎.
| text = html.unescape(text) | ||
|
|
||
| # Collapse any introduced horizontal whitespace runs | ||
| text = re.sub(r"[^\S\r\n]+", " ", text) |
There was a problem hiding this comment.
When SSML is disabled but the text contains any < character, the fast return is skipped and this unconditional normalization collapses user-authored tabs and repeated spaces even if no markup was removed. For example, text-only session.say("Use <p> and preserve\tspacing") is delivered in its original form but stored as Use <p> and preserve spacing; limit normalization to whitespace introduced by tag substitutions so plain-text history remains faithful.
Useful? React with 👍 / 👎.
| # Remove standalone / framing tags | ||
| text = _SSML_STANDALONE_RE.sub(" ", text) |
There was a problem hiding this comment.
Valid SSML may contain XML comments, which synthesis ignores, but the cleanup only removes recognized element delimiters. Thus <speak>Hello<!-- internal directive -->world</speak> is stored as Hello<!-- internal directive -->world, retaining unheard markup and allowing it to influence later LLM turns; strip XML comments (and other non-spoken XML framing) while SSML mode is active.
Useful? React with 👍 / 👎.
1 participant
Fixes #4802
Summary
When using
session.say()with text containing pause tags like<break time="1s"/>or provider markup tags, the tags are synthesized as pauses by the TTS engine, but previously leaked directly into theChatContextassistant message. Storing these raw tags in chat history caused subsequent LLM turns to be few-shotted into emitting unsupported markup tags.Changes
AgentActivity._tts_task_impl, sanitizeforwarded_textwithstrip_all_markupbefore adding it as an assistant message tochat_ctx.tests/test_say_break_tag.py.