feat: tts request

[YiminW]

No description provided.

[github-actions]

Review — feat: tts request (rime_tts 0.4.10 → 0.4.11)

Small, focused diff: emit a TTS request metric from request_tts() before handing text to the vendor client, plus matching version bumps. The version bump is correctly mirrored in both manifest.json and pyproject.toml, which matches the repo convention. A few things worth addressing before merge.

1. A metrics failure can abort real audio synthesis (main concern)

The new await sits inside the large try block in request_tts, above await self.client.send_text(t):

self.sent_tts = True
self.metrics_add_output_characters(len(t.text))
if t.text:
    await self.send_tts_request_metrics(...)
await self.client.send_text(t)

If send_tts_request_metrics raises for any reason, control jumps to the generic except Exception handler, which builds a ModuleError(NON_FATAL_ERROR) and — because self.current_request_finished or t.text_input_end is commonly true on the final chunk — calls _handle_tts_audio_end(reason=TTSAudioEndReason.ERROR). Net effect: a telemetry problem surfaces to the end user as a failed TTS turn, and send_text never runs, so no audio is produced at all.

Telemetry should be best-effort. Suggest isolating it:

if t.text:
    try:
        await self.send_tts_request_metrics(
            request_id=t.request_id,
            request_time_ms=int(time.time() * 1000),
            request_text=t.text,
        )
    except Exception as e:
        self.ten_env.log_warn(f"Failed to send tts request metrics: {e}")

Also note self.sent_tts = True is already set at that point, so an exception here leaves the state machine believing text was sent when it was not.

2. Emits once per text chunk, not once per request — is that intended?

request_tts() is invoked per incoming text chunk; the method itself distinguishes a new request via if t.request_id != self.current_request_id and tracks completion with t.text_input_end. Since the new call is unconditional (beyond the if t.text: guard), a streamed request with N chunks emits N metrics events sharing the same request_id.

The parameter name request_time_ms reads like "when this request started", which would argue for emitting inside the new-request branch instead. But request_text=t.text is per-chunk, which argues the opposite. Whichever is intended, could you confirm — and if it is meant to be per-request, move the call into the t.request_id != self.current_request_id block so downstream consumers are not left to de-duplicate.

3. Guard is inconsistent with how "empty" is defined elsewhere in this method

The new code gates on if t.text:, but a few lines below, emptiness is tested as t.text.strip() == "". Whitespace-only text (a single space) therefore passes the new guard and emits a metric, while the surrounding logic treats it as empty. Using if t.text.strip(): would make the two consistent.

Separately, metrics_add_output_characters(len(t.text)) remains unconditional while the new metric is guarded — fine if deliberate, just noting the asymmetry.

4. Base-class dependency pin may need bumping

I could not verify that send_tts_request_metrics exists on AsyncTTS2BaseExtension: ten_ai_base is a system dependency, not vendored in this repo, so there is no local definition to check. A repo-wide grep finds the symbol only at this new call site — no other extension calls it, and none of the 11 TTS extensions using metrics_add_output_characters use this API.

manifest.json still pins ten_ai_base at "0.7" (unchanged by this PR), which is a loose range. If this method landed in a specific 0.7.x patch, resolving an older patch yields AttributeError at runtime — inside the try, so it manifests as the failure mode in item 1 rather than an obvious import error. Please confirm the minimum ten_ai_base version providing this method and bump the pin if needed.

5. No test coverage for the new behavior

tests/test_metrics.py already exists and asserts on TTFB via the metrics data channel, so there is a natural place to extend. Nothing currently asserts that the new request metric is emitted, that it carries the expected request_id and request_text, or that empty text suppresses it. Given that items 2 and 3 are both about when the event fires, a test pinning that down would be valuable — the existing ExtensionTesterMetrics.on_data handler is a ready template.

Minor

• time is newly imported while the file already imports datetime and uses datetime.now() for request_start_ts. int(datetime.now().timestamp() * 1000) would avoid introducing a second time source. Wall-clock is the right choice for a timestamp, so this is cosmetic only.
• The PR body is empty and the title feat: tts request is quite vague. A sentence on what consumes this metric, and why it is rime-only for now, would help reviewers and future archaeology. The conventional-commit prefix itself is correct per AGENTS.md.
• Only rime_tts emits this today. If a broader rollout across TTS vendors is planned, worth noting so the pattern stays consistent.

Nothing here is architecturally wrong — item 1 is the one I would treat as blocking, since it converts a telemetry hiccup into user-visible audio loss. Items 2 and 3 are quick clarifications.

[github-actions]

Review of the added test_request_tts_reports_request_metrics_once test.

Note on scope: ten_ai_base is a build-time system dependency (declared in manifest.json as {"type": "system", "name": "ten_ai_base", "version": "0.7"}, fetched into .ten/app/ten_packages/system/ten_ai_base/interface per tests/bin/start). It is not in this checkout and I could not read it, so the points below about AsyncTTS2HttpExtension.request_tts and send_tts_request_metrics are inferences from the in-repo call sites, not confirmed readings. Please sanity-check them against the installed package.

1. The test asserts on a base class that lives outside this package

extension.py defines only create_config, create_client, vendor, vendor_metadata, and synthesize_audio_sample_rate. It does not define request_tts — that is inherited from ten_ai_base.tts2_http.AsyncTTS2HttpExtension. The docstring says as much ("OpenAI inherits request reporting from the HTTP TTS base class"), which is the tell: nothing here exercises openai_tts2_python code. A change to the base class calling convention will fail this test with no defect in this extension, and a real defect in this extension cannot fail it. If the goal is to pin the base contract, the test belongs alongside ten_ai_base. If the goal is to verify OpenAI emits request metrics end to end, see point 4.

2. assert metrics["request_time_ms"] > 0 is either vacuous or flaky

The only send_tts_request_metrics call site in the repo is rime_tts/extension.py:396:

await self.send_tts_request_metrics(
    request_id=t.request_id,
    request_time_ms=int(time.time() * 1000),
    request_text=t.text,
)

That is an absolute Unix epoch timestamp in milliseconds, not an elapsed duration. If tts2_http follows the same convention, the value is always around 1.7e12 and > 0 can never fail — the assertion tests nothing. If instead it is a genuine duration, the stub client returns immediately with no yields and no I/O, so elapsed time is sub-millisecond and int() truncates it to 0 — an intermittent failure that will be painful to diagnose in CI. Worth resolving which semantic is intended and asserting it precisely, for example by patching the clock and asserting an exact value.

3. await_args.kwargs hard-codes keyword dispatch

If the base invokes send_tts_request_metrics(request_id, ts, text) positionally, kwargs is empty and the three assertions fail with a KeyError rather than a readable diff. assert_awaited_once_with(...) binds against the signature and is robust to either style.

4. Direct instantiation may make the test pass for the wrong reason

Every other test in this package drives the extension through ExtensionTester plus set_test_mode_single, patching at the httpx boundary (@patch("openai_tts2_python.openai_tts.AsyncClient")). Constructing OpenAITTSExtension("tts") directly and attaching MagicMock skips the lifecycle, which has two consequences:

• extension.config is a bare MagicMock with only dump set to False. Every other attribute returns a truthy MagicMock, so any if self.config.X: inside the base takes the true branch regardless of intent. The test can pass through a code path that never runs in production.
• The base may touch state normally initialised in on_init/on_start. The comparable rime_tts path relies on recorder_map, request_start_ts, current_request_finished, last_completed_request_id, and stop_event; if tts2_http does the same, an unset attribute either raises or silently short-circuits.

The existing test_metrics.py in this same directory already asserts on TTFB by observing the emitted metrics data event through the harness. Extending that pattern to assert the request metrics event would test the real wiring and would not depend on the base class internal calling convention at all.

5. Placement and coverage

test_params.py is organised under a # ================ test params passthrough ================ banner; a request-metrics test fits test_metrics.py, which already exists here. On coverage, the test name says "once" but only the single empty-stream path is exercised — there is no case for empty or whitespace text (the rime path guards with if t.text:, so a no-emit case looks meaningful), no multi-chunk case confirming metrics are still reported exactly once, and no error path.

6. Minor

• if False: yield None, None is an obscure way to build an empty async generator and reads as dead code to both reviewers and linters. return followed by an unreachable yield, with a one-line comment explaining the intent, is the more conventional idiom.
• Per AGENTS.md, test: is the type for test additions. This PR changes exactly one test file, so test: ... fits better than feat: tts request. The PR description is also empty — a sentence on what regression this locks down would help future readers, especially given the cross-package coupling in point 1.

Nothing here is a blocker on correctness of shipped code, since the change is test-only. The substantive concerns are points 2 and 4: as written the test can pass without verifying anything, or fail intermittently.

[github-actions]

Review

Small, focused addition — one test in openai_tts2_python/tests/test_params.py. The intent (locking in that request-level TTS metrics get reported) is worth having, but as written the test sits in the wrong place, asserts something weaker than its name claims, and targets code that does not live in this repo. Details below.

1. The test targets an external dependency, not this extension

OpenAITTSExtension does not define request_tts — it only implements create_config, create_client, vendor, vendor_metadata, and synthesize_audio_sample_rate. request_tts and send_tts_request_metrics come from AsyncTTS2HttpExtension in ten_ai_base, which manifest.json declares as a system dependency pinned at version 0.7 and which is not vendored into this repo (PYTHONPATH in tests/bin/start points at .ten/app/ten_packages/system/ten_ai_base/interface, populated at install time).

Two consequences:

• The docstring — "OpenAI inherits request reporting from the HTTP TTS base class" — is exactly right about what is being tested, which is the problem: this is a contract test for ten_ai_base, placed in a vendor extension. send_tts_request_metrics has only one call site anywhere in this repo (rime_tts/extension.py:396), so nothing here proves the shared HTTP base emits it.
• 13 extensions in this repo inherit AsyncTTS2HttpExtension. If this behavior is a base-class guarantee, testing it once upstream is better than 13 near-identical copies. If the goal is instead to pin the base-class contract from the consumer side, please say so in the PR description so the duplication reads as deliberate.

I could not run this test to confirm it passes — ten_ai_base is not installed in my environment, so I can't verify the base class actually reports these metrics under these keyword names. Worth confirming it passes against the pinned 0.7.

2. Wrong file

test_params.py is organized around a single concern, marked by the # ================ test params passthrough ================ section header. The new function is inserted above that header, so it now reads as though a metrics test introduces the params section.

More to the point, tests/test_metrics.py already exists in this same directory. That's where this belongs.

3. request_time_ms > 0 does not assert anything

assert metrics["request_time_ms"] > 0

Based on the only in-repo producer, this value is int(time.time() * 1000) — a wall-clock epoch timestamp, not an elapsed duration. > 0 is satisfied by any epoch value, so this assertion cannot fail short of the field being zero or absent. It also reads as if it were checking a duration, which invites future confusion about the field's meaning.

Compare xai_asr_python/tests/test_metrics.py, which pins a real lower bound (assert delay_ms >= 50 after seeding a known 75 ms offset). Here, asserting the value is within a few seconds of int(time.time() * 1000) would actually catch a unit mix-up (seconds vs. ms) or a swapped field.

4. The name promises "once" but the test never exercises repetition

test_request_tts_reports_request_metrics_once issues a single request_tts call, so assert_awaited_once only rules out a double-report within one call. The interesting regression — one metrics event per request, not per text chunk — is untested. Sending two TTSTextInputs with the same request_id (first text_input_end=False, then True) and still asserting a single await would earn the name.

Also untested: empty/whitespace text. rime_tts guards with if t.text:, and OpenAITTSClient.get early-returns (None, END) on empty input, so "empty text reports no request metrics" looks like intended behavior and is a cheap second case.

5. Stub client nits

class StubClient:
    async def get(self, text, request_id):
        if False:
            yield None, None

The if False: yield idiom to force an async-generator function is obscure. Clearest is to yield a realistic terminal event instead of nothing:

async def get(self, text, request_id):
    yield None, TTS2HttpResponseEventType.END

That matches the real signature (AsyncIterator[Tuple[bytes | None, TTS2HttpResponseEventType]]), exercises the base class's completion path rather than an empty stream, and drops the dead branch. test_metrics.py already imports TTS2HttpResponseEventType, so the type is available.

Minor: extension.config = MagicMock() means every unset attribute returns a truthy Mock, so if the base class later reads e.g. config.params, this test silently takes a nonsense path instead of failing. A real OpenAITTSConfig(...) — as the neighboring test_vendor_metadata_* tests use — is sturdier. Unused text / request_id params on the stub are fine.

6. Conventions

• Commit/PR type. AGENTS.md reserves test: for test additions; this PR adds only a test file but is titled feat: tts request. Suggest test(openai_tts2): .... The PR number gets appended on merge per the same section.
• PR description is empty. Given that the test asserts against an external pinned dependency, a line on why it lives here would help reviewers.
• Import placement. import asyncio landed between from pathlib import Path and import json. The sibling test_metrics.py groups asyncio after the unittest.mock import — worth matching. (The pre-existing duplicate from pathlib import Path at lines 2 and 16 is not yours.)

Security / performance

Nothing of concern. No secrets, network calls, or credential handling touched; vendor_metadata masking is untouched. Test-only change, no runtime path affected, and it adds negligible suite time.

Summary

The behavior is worth pinning, but I'd suggest: move it to test_metrics.py, assert request_time_ms against a real bound, add the multi-chunk case so "once" means something, replace the if False: yield stub with one that yields END, and retitle to test:. The bigger question is whether this belongs in ten_ai_base instead — happy to be told the consumer-side pin is intentional.