feat: enhance Soniox ASR with sentence termination handling

[diyuyi-agora]

No description provided.

[github-actions]

Code Review — Soniox ASR sentence termination

Thanks for this. The sentence-boundary gating is a nice addition and the test suite is genuinely thorough (parametrized detector unit tests + end-to-end batch tests). A few things worth addressing before merge.

Bugs / correctness

1. Deferred vendor-final tokens are dropped on finalize (potential data loss).
In sentence_terminator mode, vendor is_final tokens without a detected boundary are emitted as non-final and buffered in _deferred_vendor_final_tokens. But _finalize_end() (and _handle_open, reconnect) now call _clear_deferred_vendor_final_tokens() without flushing. So if a session ends on a trailing incomplete sentence (no terminator punctuation — common in spontaneous speech), that text is only ever seen downstream as non-final and never gets a final emission. Consumers that wait for final=True to commit a segment will silently lose the last utterance. Consider flushing the deferred buffer as a final result on _finalize_end rather than clearing it.

2. README documents a config key that doesn't exist.
The README adds a require_sentence_end_punctuation (boolean, default true) bullet, but the implementation exposes this as holding_mode: \"sentence_terminator\" (new HoldingMode enum value) — there is no require_sentence_end_punctuation parameter anywhere in config.py or extension.py. The docs and tests disagree (tests use \"holding_mode\": \"sentence_terminator\"). Please align the README with the actual holding_mode value, and document that it falls under the existing holding-mode list.

Dead code

Three new methods are defined but never called anywhere:

• _send_published_asr_results (thin wrapper around _publish_asr_results)
• _create_asr_results_by_token_finality
• _split_tokens_by_finality

These add ~35 lines and are untested. Recommend removing them unless a follow-up wires them in.

Minor

• _effective_holding_mode redundancy: the if configured == HoldingMode.SENTENCE_TERMINATOR: return HoldingMode.SENTENCE_TERMINATOR branch is a no-op given the trailing return configured. Can be dropped.
• stable semantics change: _create_single_asr_result now sets \"stable\": is_final (group-level) instead of token.is_final (per-token). This changes per-word stability in mixed batches. If intentional, worth a comment; otherwise it may surprise downstream word-level consumers.
• Inconsistent property key: _send_asr_results_data uses set_property_from_json(None, ...) while _send_translation_results uses set_property_from_json(\"\", ...). Use one consistently.
• requirements.txt: pysbd>=0.3.4 is an open lower bound. Pinning (==) or capping the major is safer for reproducible builds, per repo conventions. (Also missing trailing newline — pre-existing.)

Performance

split_at_last_complete_sentence rebuilds cumulative with += and re-evaluates the boundary at every token (O(n²) string work for long buffers), and _pysbd_probe_tail_boundary runs pySBD segmentation on every ambiguous single-dot tail. For long English vendor-final buffers this could add up. Likely fine at typical utterance lengths, but worth confirming on long-form input. Caching the segmenter per language (already done) helps; consider also short-circuiting when the tail clearly isn't ..

Tests

Good coverage of the detector and the happy paths. Two gaps to consider:

• No test for the finalize-flush case in item (1) — i.e., session ends with deferred text and no terminator.
• The dead-code methods are untested (another reason to remove them).

[github-actions]

PR Review: feat: enhance Soniox ASR with sentence termination handling

Nice work. This adds a sentence_terminator holding mode that gates vendor is_final segments until a real sentence boundary is detected (pySBD for ambiguous English ., plus local CJK/cluster terminators), and introduces a batched asr_results emission path for mixed final/non-final segments. The text_utils.py helper is well-factored and the boundary-detector tests are thorough. A few things worth a closer look.

Potential bugs

1. Deferred final tokens appear to be dropped on finalize (possible data loss).
In _finalize_end the change calls self._clear_deferred_vendor_final_tokens() but does not flush them first. In sentence_terminator mode, a trailing utterance with no terminator (e.g. 台北选手戴资颖 with no 。) is only ever emitted as non-final (see test_vendor_final_without_sentence_end_is_sent_as_non_final). If the session finalizes before a boundary arrives, those held tokens are discarded and no final=True result is ever produced for that segment. Downstream consumers that key off the final result would lose the last sentence. The same clear-without-flush happens on reconnect (on_audio_frame) and _handle_open. Is dropping intended, or should these be flushed as a final segment first? This case isn't covered by a test.

2. Translations are emitted once per ASR result in a mixed batch.
_publish_asr_results loops over every result and, for each, calls _send_translation_results(translation_tokens, result.final, ...). In the mixed final+non-final path that means the same translation_tokens get sent twice — once with final=True and once with final=False — for what is conceptually one translation. The pre-existing NOTE explains the multi-language split, but this new batch path looks like it can double-emit a translation with conflicting finality. Worth confirming the downstream contract handles that.

Code quality

3. split_at_last_complete_sentence returns two always-equal values.
emit_len and defer_start are identical in every branch (last_boundary + 1 in the partial case, len(token_texts) in the full case). The second element is redundant — either collapse to a single return value or document why the API keeps both.

4. ends_with_boundary computes stripped then ignores it.
It passes the original text to _boundary_at_tail (which re-strips), so the local stripped only serves the empty-check. Minor, but the variable reads as if it feeds the boundary call.

5. Backward compatibility of the new asr_results batch message.
The mixed path now short-circuits to a single batched asr_results Data message instead of separate asr_result sends. Any downstream consumer that only listens for asr_result needs to also handle asr_results. Worth a note in the README/changelog if that protocol surface is consumed externally.

Minor

• Version bump goes 0.4.5 → 0.5.1 → 0.5.2, skipping 0.5.0. Intentional?
• requirements.txt still has no trailing newline. Good call pinning pysbd==0.3.4 exactly.
• config.py: switching to try/except ValueError for HoldingMode parsing is a clean improvement over the hardcoded membership check — it now picks up sentence_terminator automatically.
• Dead helpers _create_asr_results_by_token_finality / _split_tokens_by_finality and _send_published_asr_results were added then removed across later commits — good cleanup, nothing left dangling.

Tests

Strong coverage on SentenceBoundaryDetector (decimals, abbreviations, clusters, CJK ellipsis, comma/semicolon negatives) and the mixed-batch / prefix-suffix / non-zh-en passthrough integration cases. The main gap is the finalize-while-deferred scenario in (1) — worth adding a test for what happens to held tokens when the session ends without a terminator.

Overall the logic is solid and well-tested; the finalize-drop behavior in (1) is the one item I'd want resolved or confirmed before merge.

[wangyoucao577]

pysbd can't handle this?

[diyuyi-agora]

Because soniox is outputted in a streaming manner through tokens.
For both "12.Hello" and "12.5", it is possible to first receive the token "12.", and then receive the next token. When receiving "12.", only the currently existing token and the next token can be used for judgment.

Currently, pysbd only focuses on making judgments based on Mr., which is difficult to cover with rules

[wangyoucao577]

add some comments here since it's for backward compatibility.

[diyuyi-agora]

DONE

[github-actions]

Code Review: Soniox ASR sentence termination handling

Thanks for this — the sentence_terminator mode is a nice addition and the test coverage is genuinely strong. A few things worth addressing before merge.

Potential bugs

1. Swapped comment vs. logic in _publish_asr_results translation gating (extension.py)
The branch comments appear inverted relative to the code:

if self.config is None or self.config.holding_mode != HoldingMode.SENTENCE_TERMINATOR:
    # In SENTENCE_TERMINATOR mode, asr_results are sent in segments...  <-- but this branch is the NON-sentence_terminator path
    if result.final:
        await self._send_translation_results(...)
else:
    # other modes, send translation for each segment  <-- but this is the SENTENCE_TERMINATOR branch
    await self._send_translation_results(...)

The behavior may well be intentional, but the comments describe the opposite branch they sit in. Please fix the comments (or the logic) so the next reader isn't misled. The docstring above says "translation is sent only for final segments," which also conflicts with the else branch sending translation for every segment regardless of result.final.

2. _flush_deferred_vendor_final_tokens uses bare timestamps/no id continuity
On <fin>, deferred tokens are force-emitted as final via _create_asr_results(tokens, True) then _publish_asr_results(..., []). Translation is intentionally dropped here (documented), which is fine for current deployments, but worth a TODO marker since enabling translation + sentence_terminator later would silently lose the trailing segment's translation.

3. result.id assignment for index 0
In _publish_asr_results, only index > 0 and not result.id gets a fresh uuid. The first segment relies on send_asr_result setting the id. This is the prior contract, but in a multi-segment final batch the per-segment ids now matter more (downstream correlation). Confirm send_asr_result always populates result.id for the first segment; the fallback result.id or self.uuid suggests it can be None.

Behavior change to flag

4. stable field semantics changed (_create_single_asr_result)

"stable": is_final,   # was: token.is_final

This changes per-word stable from the vendor's token-level finality to the segment-level finality across all holding modes, not just the new one. In sentence_terminator mode a vendor-final token can be emitted as non-final (deferred), so reporting stable=False for it is arguably more correct — but this also affects false/finalize/endpointing_only modes. Is the change to existing modes intentional? If not, gate it on the holding mode.

Robustness

5. pySBD probe correctness (_pysbd_probe_tail_boundary)
The f"{stripped} Z." probe is clever but fragile: it assumes appending " Z." reliably forces a split decision. For inputs already ending in non-period terminators this path isn't reached (good), but for edge inputs like a lone "." or whitespace-only after rstrip, segmenter.segment(probe) behavior is worth a unit test. Consider asserting parts is non-empty (already done) and adding a test for stripped == ".".

6. No graceful fallback if pysbd import fails
text_utils.py does a top-level import pysbd. Since this is a newly pinned dependency (pysbd==0.3.4), an import failure now breaks the whole extension at load even for users on holding_mode=false. Consider deferring the import into SentenceBoundaryDetector.__init__ or _get_segmenter so non-sentence_terminator users aren't impacted by a missing/broken pysbd install.

Minor

7. data.set_property_from_json(None, ...) vs ""
You changed the first arg from "" to None in two places (_send_asr_results_data, _send_translation_results). Good consistency if None is the correct "root path" sentinel, but please confirm against other extensions in the repo — a silent semantic difference here would be hard to debug.

8. requirements.txt now ends with a newline (good), and pins pysbd==0.3.4 exactly (good, matches repo dependency-pinning guidance).

9. Version bump 0.4.5 → 0.5.2 skips 0.5.0/0.5.1 — fine if intentional, just noting.

Tests

Coverage is excellent — mixed-language batches, deferred flush on finalize, abbreviation handling, decimals, CJK ellipsis, and non-zh/en passthrough are all exercised. Two gaps worth adding:

• A sentence_terminator + translation test to lock in the gating in finding Update README.md #1.
• A reconnect test asserting _clear_deferred_vendor_final_tokens() runs and uuid is regenerated on _needs_reconnect, since that path now resets sentence state.

Overall solid work. The main blockers are the misleading comments (#1) and confirming the stable semantics change (#4) is intended for all modes.

[github-actions]

PR Review: Enhance Soniox ASR with sentence termination handling

Thanks for this — the sentence_terminator feature is well-structured and the unit coverage on text_utils.py is genuinely thorough. A few things worth a closer look before merge.

Behavioral changes that reach modes beyond sentence_terminator

These changes affect false/finalize/endpointing_only too, so they deserve explicit confirmation that they're intended:

1. New asr_results (plural) batch is now emitted for every mode. _publish_asr_results always calls _send_asr_results_data, and it's reached from both _send_transcript_and_translation and _send_mixed_transcript_and_translation. On main only the per-segment asr_result was sent. The change is additive (existing asr_result consumers are unaffected), but every publish now sends an extra data message. Please confirm downstream graphs are prepared for asr_results and that no consumer double-processes both messages. manifest.json has "api": {}, so neither name is declared there — worth verifying the base interface covers asr_results.

2. stable word field semantics changed for all modes. _create_single_asr_result changed "stable": token.is_final → "stable": is_final (group finality). In the non-final path that merges vendor-final tokens (holding modes), words that were previously stable: true per-token will now report stable: false. If any consumer relies on per-word stability this is a silent behavior change. If intentional, a note in the README/changelog would help.

Possible issues

3. _get_uuid() is newly referenced but not defined in this extension. It's assumed to exist on AsyncASRBaseExtension (the base isn't vendored in the tree, so I couldn't verify it directly). The rest of the file only ever uses self.uuid, so please confirm _get_uuid() actually exists on the base version pinned by the manifest — an AttributeError here would only surface at runtime on the reconnect and multi-segment-id paths.

4. Boundary detection uses only the first token's language for the whole merged buffer. In _adjust_tokens_for_sentence_end_finality, language = map_language_code(merged_tokens[0].language ...) drives split_at_last_complete_sentence across all tokens, including mixed zh/en buffers. A buffer that starts zh but ends with an English sentence (or vice versa) could be classified by the wrong family. _create_asr_results regroups by language afterward, but the boundary decision already happened on the whole buffer. Probably an acceptable edge case, but worth a comment.

5. Deferred tokens are flushed without translation on session finalize. _flush_deferred_vendor_final_tokens publishes with [] translations by design (documented in the docstring). That's fine for current deployments, but it's a real limitation if sentence_terminator is ever combined with translation — the final flushed segment would lose its translation. A README caveat would make this explicit.

Minor

• set_property_from_json("", ...) → set_property_from_json(None, ...) aligns with the dominant convention in the repo (the "" form is rare). Good cleanup.
• pysbd==0.3.4 is correctly pinned to an exact version.
• Version bump 0.4.5 → 0.5.2 skips 0.5.0/0.5.1; just confirm that's intentional and not a leftover.
• The broad try/except in _handle_transcript swallows any error from the new boundary logic and logs it, but deferred-token state could be left mid-update if pySBD raises. Low risk given the pinned version, but consider clearing/recovering deferred state on exception.

Test coverage

Strong on the unit side (text_utils parametrized cases for zh/en/abbrev/decimal/cluster, plus integration tests for defer/flush/prefix-suffix). Two gaps worth adding: a test exercising the new asr_results batch in pure false/default mode (to lock in the additive-message contract from point 1), and a sentence_terminator + translation case to pin down the behavior in point 5.

Overall solid work — the core deferral/boundary logic reads cleanly and the index contract in split_at_last_complete_sentence is well documented.