Skip to content

feat: speko.ai router extensions - #2307

Open
idafoh wants to merge 4 commits into
TEN-framework:mainfrom
idafoh:feat/speko-router-extensions
Open

idafoh wants to merge 4 commits into
TEN-framework:mainfrom
idafoh:feat/speko-router-extensions

Conversation

@idafoh

@idafoh idafoh commented Sep 1, 2026

Copy link
Copy Markdown

No description provided.

@idafoh idafoh changed the title Feat/speko router extensions feat: speko.ai router extensions Sep 1, 2026
return {}
metadata: dict[str, Any] = {
"base_url": self.config.base_url,
"routing": self.config.routing,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        "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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 wangyimin-agora left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change code as comment and run tts guarder test for this new extension

@diyuyi-agora diyuyi-agora left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. commit() timeout leaves a pending _finalize_waiter — after a timeout, the future is never failed/cleared, so a subsequent commit() can block forever on await self._finalize_waiter. This can also happen when the vendor auto-finalizes before asr_finalize (covered by test_same_session_finalize_reconnect).

  2. vendor_metadata() is missing key — please align with other ASR extensions (azure_asr_python, tencent_asr_python) and the rime_tts convention already referenced in this PR.

Should fix

  1. No on_deinit() cleanup for audio_dumper (only stopped in stop_connection()).
  2. session.closed usage is stored but never forwarded to the extension/metrics path.
  3. send_audio() errors only return False — consider disconnect/reconnect instead of leaving a half-dead session.
  4. Per-turn timestamp state is not reset_total_audio_bytes / _final_cursor_ms accumulate across multiple finalize cycles in the same session, which can break timestamp accuracy when segments is absent.
  5. Auto-final before asr_finalize may emit a spurious timeout error even though asr_finalize_end is still sent.
  6. 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_python

Pay 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.

Comment thread ai_agents/agents/ten_packages/extension/speko_asr_python/client.py
Comment thread ai_agents/agents/ten_packages/extension/speko_asr_python/client.py
Comment thread ai_agents/agents/ten_packages/extension/speko_asr_python/extension.py Outdated
Comment thread ai_agents/agents/ten_packages/extension/speko_asr_python/README.md
@idafoh

idafoh commented Sep 11, 2026

Copy link
Copy Markdown
Author

@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?

@github-actions

Copy link
Copy Markdown

Review: feat: speko.ai router extensions (1/3)

Three new extensions (speko_asr_python, speko_llm2_python, speko_tts2_python) plus guarder registration. The code is more careful than a typical first vendor integration: clean client/extension split, lock_buf()/unlock_buf() paired in finally, is_connected() gated on the real session.ready handshake rather than a non-null client, expected closes suppressed via _closing, and the buffered-frame flush delegating back to the base so per-frame session metadata is preserved.

Reviewed against docs/ai/L1/L2/asr_plugin_design_review.md. There are MUST violations I'd treat as blockers, mostly reconnect policy, stale-callback isolation, and result shape.

Disclosure: ten_ai_base is not vendored in the checked-out worktree, so I could not read asr.py directly. Claims about base behavior come from the design guide and siblings (xai_asr_python, soniox_asr_python). Correct me where the base contract differs.

ASR design review

  • lifecycle: FAIL — no latch against connect attempts after invalid config; dumper torn down in stop_connection()
  • connection state: FAIL — no generation/epoch guard; duplicate disconnected+error per root cause
  • buffering: PASS — bounded ModeKeep, explicit flush on session.ready
  • finalize: PARTIAL — one asr_finalize_end on normal paths, but silent turns stall ~10s and tear down the session
  • reconnect: FAIL — no backoff, no ceiling, no escalation to fatal
  • result shape: FAIL — vendor speaker at metadata root; raw vendor timestamps not mapped
  • metrics: PASS
  • tests: FAIL — several required §13.1 tests absent; no redaction assertion

@github-actions

Copy link
Copy Markdown

Merge blockers (ASR) — 1/2

1. start_connection() can leave the state machine stuck in connecting.

async def start_connection(self) -> None:
    if self._permanent_error is not None:
        return

The base wraps start_connection() and emits connecting before the override runs (guide 4.2). Returning here never closes that transition, so connection_status stays connecting and connecting -> disconnected is never reported. The self.config is None branch just below does call on_disconnected(...); the _permanent_error branch must too.

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

is_connected() is self.client is not None and self.client.is_ready. close() only clears _ready after it awaits send(session.close) and the 2s drain, so during that window send_audio() returns True and hands frames to a dying socket. Guide 6.3.3 / 9.3 require is_connected() to be False across a replacement so the base buffers instead. Swap the reference out first (client, self.client = self.client, None), as _reset_connection() and stop_connection() already do.

3. No callback-generation guard, so stale callbacks mutate live state.

on_event / on_disconnect are bound to the extension and shared by every client generation, with no epoch or identity check (guide 5.2, and on the merge-blocking list). Two concrete paths:

  • _reset_connection() calls _on_router_disconnect(error) explicitly, and the old listener's finally also fires on_disconnect(error) because error is not None overrides the _closing suppression. Two send_asr_error and two on_disconnected for one failure (guide 10: same root cause should not emit duplicates).
  • In start_connection(), closing an old client that latched an error fires _on_router_disconnect after the new client exists, marking the fresh connection disconnected.

@github-actions

Copy link
Copy Markdown

Merge blockers (ASR) — 2/2

4. Empty final transcripts are emitted as real turns. _send_transcript() sends unconditionally, so a transcript.final with text: "" produces an empty final asr_result. Guide 7.4 and the merge-blocking list: an empty final is not a valid user turn. Drop empty interims, and consume an empty final as the finalize acknowledgement rather than forwarding it downstream.

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 FATAL_ERROR at the ceiling, and a counter reset after a successful handshake — none of the four is present. soniox_asr_python/reconnect_manager.py is the reference. The guarder's test_connection_status_reconnection also asserts connecting >= 2 with all-non-fatal codes; worth confirming that passes.

6. Buffered-frame flush drops per-frame session attribution. The session.ready handler drains buffered_frames and re-queues them but never reads each frame's metadata. Compare xai_asr_python._flush_buffered_audio_frames(), which parses metadata per frame and updates self.metadata / self.session_id before sending. Guide 6.3.5 makes this a MUST so frames from an older session are not attributed to the current one. As written, every replayed frame inherits whatever self.metadata happens to be, so session_id on the resulting results can be wrong.

7. Vendor timestamps are used raw, ignoring the timeline. _send_transcript() takes segment["start_ms"] / end_ms straight from the vendor for both start_ms and ASRWord. Guide 6.4 says vendor stream-relative values should be mapped through audio_timeline.get_audio_duration_before_time(); using them raw makes timestamps wrong after a reconnect or any dropped audio. Also, int(segment["start_ms"]) uses direct subscripting, so a segment missing the key raises KeyError inside the event loop instead of degrading — the sibling .get("text", "") calls are defensive but these are not.

8. finalize() unconditionally advances the turn cursor in finally. Even when commit() raised because the session was never connected or the vendor never produced a final, the cursor advances and _total_audio_bytes resets, so subsequent start_ms values are derived from audio the vendor never confirmed. Worth separating "finalize completed" from "finalize attempted" here.

@github-actions

Copy link
Copy Markdown

Security

Credentials are exposed in vendor_metadata(). All three extensions do:

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 connection_status_changed. There is no such test in this PR (test_metadata_contains_key_and_language asserts the plaintext key is present, which is the opposite assertion). Please either drop key/api_key from the payload entirely — nothing downstream needs the credential — or add a test that inspects the emitted event JSON and confirms masking. deepgram_asr_python emits a single key field; the duplicate alias here doubles the exposure surface for no benefit.

Positives: Authorization headers are never logged, to_str() routes the key through utils.encrypt, error logs carry only code/message, and property.json uses ${env:SPEKO_API_KEY|} rather than a committed literal.

Test coverage

The 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:

Required test Status
test_asr_result Partial — no assertion on the emitted Data JSON (guide 7.5 requires testing post-send JSON, not the ASRResult object)
test_finalize Partial — timeout and disconnected covered; no finalize_id / session_id echo assertion
test_connection_status Missing — no connecting -> connected -> disconnected payload/redaction test
test_reconnect Missing — no backoff, ceiling, or counter-reset test
test_reconnect_lifecycle Partial — stop-joins-reconnect covered; no stale-callback test
test_invalid_params Missing — nothing asserts one fatal error and no later connect attempt
test_vendor_error Missing — no severity-vs-vendor_info separation test
test_dump Missing
test_metrics Missing — no connect-delay / TTFW / TTLW session-metadata test
test_vendor_metadata Present but asserts the opposite of redaction (see Security)
test_audio_timeline Missing — no timestamp-continuity-across-reconnect test

Two structural notes on the tests themselves:

  • tests/test_client.py imports from client import ... (top-level) while test_extension.py imports from speko_asr_python.client import .... Both work only because conftest.py injects two different sys.path entries and stubs speko_asr_python.addon into sys.modules. That stub is a real smell — it silences an import error rather than fixing it, and it means addon.py is never actually imported under test. Please pick one import style.
  • Several tests reach into privates (extension._connection_machine.try_connecting(), extension._handle_audio_frame, client._finalize_waiter). _connection_machine is used by no other extension in the repo; if the base renames it, these break silently. Acceptable for race tests, but worth a comment noting the coupling.

Also: please confirm task asr-guarder-test EXTENSION=speko_asr_python CONFIG_DIR=tests/configs was run and state the result in the PR body. Guide 13.2 makes this a gate, and blockers 1, 4, and 5 all look likely to surface there.

@github-actions

Copy link
Copy Markdown

Non-blocking observations

Unrelated reformatting in shared files. integration_tests/asr_guarder/tests/test_connection_status.py carries ~60 lines of pure churn, rewriting assert x, (msg) into assert (x), msg across assertions this PR does not touch. This looks like a Black version older than the repo's — the existing formatting is Black >= 24 style (hug-the-parens), the diff reverts it to pre-24 style. It will conflict with any concurrent PR in this file and obscures the one line that matters (adding speko_asr_python to the frozenset). Please revert the churn and keep only the registration additions; also check which Black version you're running, since task format on the extension dirs may be introducing the same drift there.

ModuleErrorCode mapping is narrower than the guide. _module_error_code() treats only authentication_failed and insufficient_credit as fatal, but _permanent_error latches on four codes including route_not_found and capability_unsupported. So those two latch permanently (never retried, extension is dead) while being reported downstream as NON_FATAL_ERROR, telling the caller to expect a retry that will never happen. Make the two sets agree.

buffer_duration_ms default of 5000 is generous. At 16 kHz mono s16 that is 160 KB, which is fine, but buffer_strategy() computes bytes_per_ms with integer division: 16000 * 1 * 2 // 1000 == 32 is exact, but at 44100 Hz it truncates 88.2 to 88, understating the limit by ~0.2%. Harmless, though computing in bytes-per-second and dividing once at the end would be cleaner.

_audio_duration_ms() truncates per call. int(byte_count * 1000 / bytes_per_second) is called separately per finalize, and the truncation error accumulates into _turn_start_ms across turns. Over a long session this drifts. Consider accumulating bytes and deriving ms from the running total, or tracking a fractional remainder.

LLM: on_stop calls super().on_stop() before cleanup. Every other extension in this PR does cleanup first, then super(). Ordering here means the base tears down before the client is closed.

LLM: _send_metrics hand-rolls a metrics Data message rather than using a base helper, unlike the ASR/TTS paths which use send_connect_delay_metrics / metrics_connect_delay. Worth confirming this is the intended pattern for llm2.

TTS: _recorders dict is keyed by request_id and only flushed on finalize/cancel/stop. If _begin_request is entered for a new request_id while a prior recorder is still pending (the guard is text_input.request_id != self.current_request_id), the old entry is left in the dict until on_stop sweeps it. Bounded by session length, but it is a slow leak of open file handles.

Docs. docs/ai/L1/03_code_map.md and the extension inventory in the L1 docs are not updated for three new extensions. Per AGENTS.md ("update docs — code changed since last last_reviewed date") this is expected when adding packages.

Version pin. manifest.json declares ten_ai_base "0.7", consistent with 74 of 75 existing extensions. Good.

Summary

The engineering instinct here is strong — the finalize handshake with generation tracking in commit(), the cancel-safe handshake teardown, and the deliberate "reconnect on next frame, never replay an ambiguous send" choice are all better than typical. The gap is that the connection state machine has not been walked against the guide's transition rules, and the six blockers above are concentrated there: a connecting state that can never close, a replacement window where is_connected() lies, and no generation guard so old-client callbacks corrupt new-client state. Those three interact, so I would fix them together rather than individually.

Recommendation: changes requested. Blockers 1-6 plus the vendor_metadata credential exposure should land before merge; the reconnect policy (5) and the missing guarder run are the two I would prioritize, since they gate the acceptance suite.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think _permanent_error means fatal error, which cannot recover, but _module_error_code is reported as non fatal error

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right, that was inconsistent. Fixed in both ASR and TTS: permanent errors now report fatal severity and prevent further connection attempts.

@wangyimin-agora wangyimin-agora left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. please provide Screenshot of tts guarder test result
  2. fix error type of _module_error_code()
  3. fix failed CI jobs. For format/lint issue, you can run task lint in ten framewwork image container

@diyuyi-agora

Copy link
Copy Markdown
Contributor

Rebase the main branch and push it again. Have AI review it again.

@github-actions

Copy link
Copy Markdown

Review: changes requested

Merge blockers

  1. [P1] Preserve each finalize context and serialize finalization. finalize() discards its session_id at speko_asr_python/extension.py:257, while send_asr_finalize_end() relies on the base class metadata captured from the most recent audio frame. A finalize whose metadata differs from that frame will therefore acknowledge the wrong session. Two concurrent finalizes can also share the base mutable finalize_id and both reach send_asr_finalize_end() at line 289, producing duplicate acknowledgements for the later request. Capture finalize metadata before the base handler and protect the commit/finalize handshake with a per-request context or a serialization lock. This violates the ASR finalize MUST requirements.

  2. [P1] Translate Router segment times onto the TEN audio timeline. speko_asr_python/extension.py:362-373 forwards segments[*].start_ms directly, but only the no-segments fallback adds _turn_start_ms at lines 375-379. After a finalize, the Router can restart segment timing at zero, so the second turn emits timestamps that move backwards despite _turn_start_ms advancing at lines 283-287. Map vendor-relative segment and word timestamps through audio_timeline, including reconnect/turn offsets, and add a two-cycle and reconnect regression with real segments. The current behavior hits the guide's timestamp merge-blocker condition.

  3. [P1] Give connection replacement a single serialized owner. The client callbacks at extension.py:116-123 do not carry an epoch/client identity, and _on_router_disconnect() at lines 334-354 unconditionally changes the shared connection state. A late disconnect from an old socket can therefore report the newly ready client as disconnected. start_connection, send_audio, finalize, and stop_connection also operate on self.client without a common send/finalize/swap lock (99, 161, 228, 257), allowing close or replacement to race an in-flight frame or commit. Add a connection generation guard, a consistent lock order, and a bounded reconnect manager that is cancelled during stop. The current per-frame retry scheduling at lines 205-225 has neither retry backoff nor a retry ceiling.

  4. [P1] Latch invalid local configuration and validate the URL before connecting. The config only checks that base_url is non-empty (config.py:65-72). When parsing fails, on_init() clears self.config (extension.py:83-96), but on_audio_frame() continues scheduling _reconnect() for every later ingress frame (209-218); start_connection() then reports another fatal disconnected transition (103-107). Add an initialization-failure latch that prevents every subsequent attempt, validate the URL scheme/authority and all supported parameter values during configuration parsing, and test exactly one fatal error with no later connection attempt. This is an ASR configuration MUST violation.

  5. [P1] Do not expose credentials embedded in a configurable Router URL. Both ASR and TTS log their effective base_url through to_str(), and ASR includes it verbatim in connection-status vendor metadata (speko_asr_python/extension.py:54-67; config.py:96-100). A URL with userinfo or signed query parameters will be emitted in plaintext. Redact/remove URL credentials and sensitive query keys before logging or status reporting, then add an emitted connection_status_changed JSON test. This violates the ASR logging and vendor-metadata MUST rules.

  6. [P1] Complete the required ASR coverage before enabling the guarder entry. The new tests exercise mocked client and a few lifecycle paths, but do not cover the required emitted-result JSON and metadata merge behavior, finalize metadata/session echo, concurrent finalize, invalid local config latch, stale callbacks, bounded reconnect policy, timeline continuity with segments, dump byte equality, connection-status redaction, or end-to-end metrics. Adding the extension name to the shared guarder set only opts it in; it is not evidence that these paths pass. Add the required standalone tests and attach sequential task test-extension and task asr-guarder-test results. The ASR guide classifies happy-path-only coverage as merge blocking.

  7. [P2] Remove the inherited auto-routing objective for explicit LLM routes. speko_llm2_python/config.py:40-48 accepts routing.mode = explicit but, unlike the ASR/TTS configs, does not remove the default objective merged from property.json. _routing_for_request() subsequently forwards the mixed explicit/objective object (extension.py:234-250), which the Router can reject. Normalize explicit routing in the LLM config and add the same regression already present for ASR/TTS.

The current commitlint check also fails because commit 9f45a0c1 has the non-conventional subject Address the pr review comments (#1). Reword or squash it before merge.

ASR design review

Area Status Notes
Lifecycle Pass Uses AsyncASRBaseExtension and calls the base initialization/deinitialization chain.
Connection state Fail Late socket callbacks can mutate the replacement connection state.
Buffering Pass The 5-second policy is bounded and ready handling requeues buffered frames.
Finalize Fail Finalize session metadata is not preserved and concurrent requests are not serialized.
Reconnect Fail No epoch guard, bounded backoff/retry ceiling, or common replacement lock.
Result shape Fail Segment/word times are not mapped to the TEN timeline; vendor speaker metadata is also emitted outside metadata.asr_info at line 384.
Metrics Fail Connect delay and usage calls exist, but required result/timeline metadata coverage is absent.
Tests Fail Required standalone and guarder coverage/evidence is missing.

I did not run PR code or install its dependencies because this review was performed from the base worktree.

@idafoh
idafoh force-pushed the feat/speko-router-extensions branch from 7ae3ac0 to 4d27e9d Compare September 18, 2026 08:45
@github-actions

Copy link
Copy Markdown

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.

@github-actions

Copy link
Copy Markdown

Review findings

  1. [P1] Merge blocker: bound the reconnect buffer by a product-level maximum. speko_asr_python/config.py:32,71-72 only rejects negative buffer_duration_ms; extension.py:271-279 converts any accepted value into the retained-audio byte limit. A graph override can therefore retain arbitrary PCM while disconnected. At the advertised maximum input format, the manifest int32 range permits a multi-petabyte queue before the nominal limit is reached. This violates the ASR guide MUST that disconnect buffering have a bounded byte limit. Add a defensible maximum duration or byte count (reject or clamp it), document the overflow policy, and cover an oversized value in the standalone tests.

  2. [P1] Merge blocker: do not reconnect after a non-retryable setup failure. A Router upgrade HTTP 400 is normalized as invalid_request with retryable=False in speko_asr_python/client.py:133-145, but extension.py:698-707 classifies that code as NON_FATAL_ERROR, and _reset_connection() always schedules recovery at :488-506. Invalid request/configuration errors therefore retry through the full backoff budget and only then report retry_exhausted. Classify non-retryable connection setup/protocol failures as fatal before calling _schedule_reconnect() (using the vendor retryability flag rather than a partial code list), and add a 400/no-reconnect regression. This violates the ASR connection/reconnect MUST rules.

  3. [P2] Normalize malformed non-stream LLM responses into the extension error path. speko_llm2_python/client.py:170 calls response.json() without wrapping a decode failure, and from_envelope() at :30-37 assumes error is an object. Either malformed 2xx JSON or {"error":"..."} can raise JSONDecodeError/AttributeError; extension.py:195-199 catches only SpekoRouterError, so no standard TEN error is emitted. Convert those invalid payloads to SpekoRouterError and add tests for malformed successful and error envelopes.

ASR design review

Area Status Notes
Lifecycle Pass Base lifecycle chaining and idempotent cleanup are present.
Connection state Fail Non-retryable setup failures are reported as recoverable.
Buffering Fail Configurable retained-audio limit has no upper bound.
Finalize Pass Finalize context serialization and bounded disconnected/timeout completion are covered.
Reconnect Fail Invalid setup errors consume reconnect retries instead of latching fatal.
Result shape Pass Emitted JSON tests cover metadata placement, empty-final suppression, and timeline mapping.
Metrics Pass Connect delay, TTFW/TTLW, vendor metrics, and session metadata have focused coverage.
Tests Fail Add the two boundary regressions above and attach sequential standalone/ASR-guarder results; no test or guarder check is currently reported on this PR.

I reviewed the latest PR diff and did not execute PR code or install its dependencies because this worktree is the base branch.

@idafoh

idafoh commented Sep 18, 2026

Copy link
Copy Markdown
Author

please provide Screenshot of tts guarder test result

Screenshot 2026-09-18 at 2 49 33 PM Screenshot 2026-09-18 at 3 00 29 PM

Here are the test results

@YiminW

YiminW commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

great, no more comments for tts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants