Conversation
| return {} | ||
| metadata: dict[str, Any] = { | ||
| "base_url": self.config.base_url, | ||
| "routing": self.config.routing, |
There was a problem hiding this comment.
"key":
"base_url":
"language":
"api_key":
"routing":
key and api_key have same vaule. you can refer to TEN-Agent/ai_agents/agents/ten_packages/extension/rime_tts/extension.py
| message=str(error), | ||
| vendor_info=ModuleErrorVendorInfo(vendor=self.vendor()), | ||
| ) | ||
| await self._finalize_request( |
There was a problem hiding this comment.
This exception may occur while processing a non-final text chunk (text_input_end=False). Calling _finalize_request() here prematurely completes the whole request, so the remaining chunks with the same request ID can no longer be processed. Please only finalize when the request has received its final input; for an intermediate-chunk error, report the error and reset/reconnect the client without calling finish_request(), similar to the Rime TTS implementation.
| reason=reason, | ||
| extra_metadata={"router_usage": self._router_usage}, | ||
| ) | ||
| await self._close_client() |
There was a problem hiding this comment.
client.close() can raise an error while draining the final session.close response, as demonstrated by test_close_surfaces_terminal_router_error. If that happens here, finish_request() below is skipped. Since _finalized has already been set to True, the outer error handler only emits an error and does not complete the request state, leaving _processing_request_id occupied and potentially blocking all subsequent requests. Please ensure finish_request() and recorder cleanup always run exactly once, even when closing the vendor session fails. Also avoid emitting a successful tts_audio_end before handling a close-time error.
|
|
||
| async def _stream_text(self, text: str, request_id: str) -> None: | ||
| client = await self._ensure_client() | ||
| self.metrics_add_input_characters(len(text)) |
There was a problem hiding this comment.
Please use metrics_add_output_characters(len(text)) here. TTS extensions, including Rime TTS and the HTTP TTS base implementation, count text submitted for synthesis as output characters. The current implementation records it as input characters, so output_characters remains zero for every Speko request and the reported usage metrics are inconsistent with other TTS extensions.
wangyimin-agora
left a comment
There was a problem hiding this comment.
please change code as comment and run tts guarder test for this new extension
diyuyi-agora
left a comment
There was a problem hiding this comment.
Review: speko_asr_python
Thanks for adding the Speko Router ASR extension — the overall structure looks good and follows existing TEN ASR patterns (AsyncASRBaseExtension, separated client/config, guarder-ready property configs, client unit tests).
Below are findings focused on the ASR extension. (TTS feedback is tracked separately.)
Blocking
-
commit()timeout leaves a pending_finalize_waiter— after a timeout, the future is never failed/cleared, so a subsequentcommit()can block forever onawait self._finalize_waiter. This can also happen when the vendor auto-finalizes beforeasr_finalize(covered bytest_same_session_finalize_reconnect). -
vendor_metadata()is missingkey— please align with other ASR extensions (azure_asr_python,tencent_asr_python) and therime_ttsconvention already referenced in this PR.
Should fix
- No
on_deinit()cleanup foraudio_dumper(only stopped instop_connection()). session.closedusage is stored but never forwarded to the extension/metrics path.send_audio()errors only returnFalse— consider disconnect/reconnect instead of leaving a half-dead session.- Per-turn timestamp state is not reset —
_total_audio_bytes/_final_cursor_msaccumulate across multiple finalize cycles in the same session, which can break timestamp accuracy whensegmentsis absent. - Auto-final before
asr_finalizemay emit a spurious timeout error even thoughasr_finalize_endis still sent. - Missing extension-level / guarder coverage — only client tests exist today; please add README instructions and run:
cd ai_agents/agents/ten_packages/extension/speko_asr_python/tests && ./bin/start
cd ai_agents && task asr-guarder-test EXTENSION=speko_asr_pythonPay special attention to test_asr_finalize, test_same_session_finalize_reconnect, test_metrics, and test_audio_timestamp.
Happy to re-review once these are addressed.
|
@wangyimin-agora @diyuyi-agora thanks for the reviews! I've pushed updates addressing the ASR/TTS feedback, added regression tests, and synced with the latest main. Could you please take another look when you have a chance? |
Review:
|
Merge blockers (ASR) — 1/21. async def start_connection(self) -> None:
if self._permanent_error is not None:
returnThe base wraps 2. Audio can be sent to a client that is being torn down. if self.client is not None:
await self.client.close() # self.client still set, is_ready still True
3. No callback-generation guard, so stale callbacks mutate live state.
|
Merge blockers (ASR) — 2/24. Empty final transcripts are emitted as real turns. 5. Unbounded reconnect with no backoff and no fatal escalation. async def _reconnect(self) -> None:
await self._ensure_connection()
if not self.is_connected():
await asyncio.sleep(0.5)A flat 0.5s delay, retried as long as frames keep arriving. Guide 9.2 requires a backoff policy, a bounded retry strategy, escalation to 6. Buffered-frame flush drops per-frame session attribution. The 7. Vendor timestamps are used raw, ignoring the timeline. 8. |
SecurityCredentials are exposed in metadata: dict[str, Any] = {
"key": self.config.api_key,
"api_key": self.config.api_key,
...
}Two aliases for the same secret are returned, relying entirely on the base redactor recognizing both key names. Guide 11.2 permits handing credentials to the redactor only when the names are recognized, and requires a test proving the values are masked in Positives: Test coverageThe standalone tests are genuinely thoughtful — the finalize-timeout-does-not-poison-next-commit case, the cancel-during-handshake case, and the auto-final-before-commit case are the kind of edge cases usually missing from a first vendor PR. That said, measured against the guide's 13.1 table:
Two structural notes on the tests themselves:
Also: please confirm |
Non-blocking observationsUnrelated reformatting in shared files.
LLM: LLM: TTS: Docs. Version pin. SummaryThe engineering instinct here is strong — the finalize handshake with generation tracking in Recommendation: changes requested. Blockers 1-6 plus the Happy to re-review once the connection-lifecycle changes are in — and to look at any of the above in more detail if a finding seems wrong. I reviewed the diff only and could not execute the test suite or the guarder in this environment, so the runtime claims above are read from code, not observed. |
| "route_not_found", | ||
| "capability_unsupported", | ||
| }: | ||
| self._permanent_error = error |
There was a problem hiding this comment.
I think _permanent_error means fatal error, which cannot recover, but _module_error_code is reported as non fatal error
There was a problem hiding this comment.
You’re right, that was inconsistent. Fixed in both ASR and TTS: permanent errors now report fatal severity and prevent further connection attempts.
|
Rebase the main branch and push it again. Have AI review it again. |
Review: changes requestedMerge blockers
The current ASR design review
I did not run PR code or install its dependencies because this review was performed from the base worktree. |
7ae3ac0 to
4d27e9d
Compare
Review findings\n\n1. [P1] Treat non-retryable connection setup failures as fatal. In speko_asr_python/client.py:130-143, a 400 upgrade response becomes invalid_request with retryable=False. However, speko_asr_python/extension.py:662-672 maps it to NON_FATAL_ERROR, and _reset_connection() schedules reconnection at :454-472. A rejected session configuration will therefore reconnect through all five attempts before ending as retry_exhausted. This violates the ASR guide MUST rule that invalid configuration and unrecoverable protocol failures are fatal and do not enter recovery. Classify non-retryable handshake/configuration failures, including invalid_request, as FATAL_ERROR before scheduling reconnects, and add coverage that asserts no reconnect task is created.\n\n2. [P1] Bound buffer_duration_ms to a defensible maximum. speko_asr_python/config.py:32,67-77 only rejects negative values, while extension.py:263-274 converts the configured duration directly into the retained-audio byte limit. The manifest permits an int32; at the advertised 192 kHz, 8-channel PCM input, its maximum allows a roughly 6.6 PB queue. A disconnected session can consequently retain audio until the worker exhausts memory. This is an ASR guide MUST violation: disconnect buffering must have a bounded byte limit. Set and validate a product-level maximum duration/byte limit, document the overflow behavior, and test rejection or clamping of an oversized value.\n\n### ASR design review\n\n- Lifecycle: PASS - base lifecycle methods are chained and stop/deinit cleanup is idempotent in the added unit coverage.\n- Connection state: FAIL - non-retryable setup failures are reported as recoverable and enter reconnect.\n- Buffering: FAIL - the configured retention limit has no safe upper bound.\n- Finalize: PASS - serialized finalize context and disconnected/timeout acknowledgement paths are covered.\n- Reconnect: FAIL - invalid session configuration can consume the reconnect loop instead of latching fatal immediately.\n- Result shape: PASS - emitted JSON coverage verifies metadata session placement, base-generated IDs, empty-result suppression, and timestamps.\n- Metrics: PASS - connect delay, TTFW/TTLW, vendor metrics, and session metadata have focused coverage.\n- Tests: FAIL - add boundary tests for oversized buffering and non-retryable handshake failures. I did not run PR scripts because this worktree is the base branch; the required live ASR guarder and CI evidence are not yet available (checks are pending).\n\nThe TTS and LLM additions also have focused mocked transport/lifecycle coverage and redact configured API keys/URLs. No additional concrete issue found there in this static review. |
Review findings
ASR design review
I reviewed the latest PR diff and did not execute PR code or install its dependencies because this worktree is the base branch. |
|
great, no more comments for tts |


No description provided.