diff --git a/.agents/skills/ten-tts-review/SKILL.md b/.agents/skills/ten-tts-review/SKILL.md new file mode 100644 index 0000000000..60e403aaa0 --- /dev/null +++ b/.agents/skills/ten-tts-review/SKILL.md @@ -0,0 +1,120 @@ +--- +name: ten-tts-review +description: Review implementation changes inside TEN Framework TTS extension directories for actionable correctness defects. Use when a pull request or diff changes Python/source code, extension-local configuration or manifests, dependencies, or tests under `ai_agents/agents/ten_packages/extension/` for a TTS extension. Focus on request lifecycle, exact `tts_audio_end` duration and event-interval accounting, streaming and flush races, PCM format, vendor errors, metrics, secrets, and regression coverage. Do not use for `ten_ai_base`, `tts_guarder`, graphs, examples, audio routing outside the extension, other extension types, or documentation-only changes. +--- + +# TEN TTS review + +Find defects that can affect users or make the TTS contract unreliable. Prefer a +small number of high-confidence findings over a broad checklist report. + +## Review workflow + +1. Read the repository instructions before reviewing. At minimum, load + `AGENTS.md`, `docs/ai/L0_repo_card.md`, and all files in `docs/ai/L1/`. +2. Establish the exact diff and its base. For a pull request, inspect the PR + metadata and full diff. For a local review, use the merge base with the target + branch. Do not infer a defect from a truncated patch. +3. Build the in-scope file list first. Review only changed files under + `ai_agents/agents/ten_packages/extension//`, where the extension + name or its imports/manifest identify it as TTS. Source code, `addon.py`, + `config.py`, `manifest.json`, `property.json`, dependency files, and + extension-local tests are in scope. README-only changes do not trigger this + skill. Ignore every changed path outside that extension directory, including + `ten_ai_base`, `integration_tests/tts_guarder`, examples, and graphs. +4. Determine whether each in-scope TTS extension is new by checking whether its + extension directory exists at the pull request merge base. Do not infer this + from the number of added files: a replacement, rename, or large update to an + existing directory is not a new extension. +5. Classify each in-scope extension from its actual inheritance and imports, not + its directory name: + - `AsyncTTS2HttpExtension` -> HTTP implementation + - direct `AsyncTTS2BaseExtension` -> WebSocket/custom implementation +6. Load [references/common.md](references/common.md) and + [references/tests.md](references/tests.md) for every review. Then load exactly + the applicable transport guide: + - HTTP: [references/http.md](references/http.md) + - WebSocket/custom: [references/websocket.md](references/websocket.md) + If a pull request changes multiple TTS extensions, classify and review each + independently; load both transport guides only when both shapes are present. + For every new TTS extension, also load and apply + [references/guarder.md](references/guarder.md). +7. Read the changed functions in full. You may inspect the installed base-class + methods they call to understand the contract, but do not review `ten_ai_base` + itself or ask the pull request to change it. Compare with one recent extension + using the same transport shape; copied legacy patterns are not proof. +8. Follow each stateful path through success, empty input, vendor error, + cancellation, and shutdown. For HTTP extensions, distinguish inherited base + behavior from extension-owned behavior before raising a finding. +9. Audit every changed call to `send_tts_audio_end()`. Independently verify + `request_total_audio_duration_ms` and `request_event_interval_ms`; these fields + are mandatory correctness checks, including partial, interrupted, error, and + empty-audio paths. When an HTTP extension inherits this logic unchanged, do + not demand a duplicate implementation in the extension. +10. Treat extension-local tests as a merge requirement. Every in-scope TTS + extension must have its own tests and satisfy the mandatory matrix in + `references/tests.md`; shared guarder or integration tests do not substitute + for them. Report a missing suite or mandatory category even when the + production defect has not yet been demonstrated. +11. For each new TTS extension, inspect pull request comments and enforce the + guarder evidence gate. The full guarder suite must pass against the current + pull request HEAD, and a comment must include both a result screenshot and a + downloadable non-empty PCM output file. Guarder evidence does not replace + extension-local tests. +12. Report implementation issues only when introduced or made materially worse + by in-scope diff hunks. The testing baselines are explicit exceptions: report + extension-local test gaps for every touched TTS extension and guarder-evidence + gaps for every new TTS extension. + Verify each finding against surrounding code; do not flag style, preferences, + or purely hypothetical risks. + +## Evidence standard + +A finding should form a complete causal chain: + +`changed code -> reachable runtime condition -> violated invariant -> user-visible impact` + +Before reporting, answer all of these: + +- What input, event ordering, or vendor response triggers it? +- Why do guards, the base class, or cleanup code not prevent it? +- Which request, audio, error, metric, or secret is affected? +- Is the faulty behavior owned by the changed extension rather than an unchanged + installed base class? +- Can the location be narrowed to changed lines? + +If any answer is missing, investigate further or omit the finding. + +## Severity + +- **P0**: widespread outage, credential exposure, or destructive behavior. +- **P1**: common requests break, cross-request audio leaks, flush is violated, or + the extension cannot recover without restart. A new or changed TTS extension + with no extension-local test suite is also P1. A new TTS extension without + current, passing guarder evidence and both required comment attachments is P1. +- **P2**: a realistic edge case produces wrong audio/events/metrics or resource + leakage with meaningful impact, or the mandatory local-test matrix is + materially incomplete. +- **P3**: limited but concrete correctness or maintainability defect worth fixing. + +Do not inflate severity because code is concurrent or vendor-facing. + +## Output + +List findings first, ordered by severity. Use this form: + +```text +[P1] Imperative, specific title +path/to/file.py:123 +Explain the triggering sequence, the incorrect behavior, and its impact in one +compact paragraph. Mention the relevant contract or test when useful, and give a +bounded fix direction without writing a patch. +``` + +Keep line ranges tight and inside the diff whenever possible. Do not combine +independent bugs in one finding. Do not add praise, a walkthrough of the patch, or +generic testing advice. If there are no actionable findings, say: + +```text +No actionable TTS-specific findings. +``` diff --git a/.agents/skills/ten-tts-review/evals/evals.json b/.agents/skills/ten-tts-review/evals/evals.json new file mode 100644 index 0000000000..adc9913e5c --- /dev/null +++ b/.agents/skills/ten-tts-review/evals/evals.json @@ -0,0 +1,67 @@ +{ + "skill_name": "ten-tts-review", + "evals": [ + { + "id": 1, + "prompt": "Review commit 3f9b4c6c4 as a pull-request diff against its first parent. Apply the TTS review only to changed files inside the fish_audio_tts_python extension directory.", + "expected_output": "A concise, line-specific review that traces fish_audio TTS flush and terminal-event behavior and avoids generic checklist comments.", + "files": [], + "expectations": [ + "Every finding identifies a reachable event ordering and user-visible consequence", + "The review classifies fish_audio_tts_python as WebSocket/custom and applies the direct-base transport checks", + "The review checks exactly-once audio_end and post-flush emission behavior", + "The review does not ask cancel_tts to call finish_request when the base class owns cleanup", + "The review produces no findings for ten_ai_base, tts_guarder, examples, or graph files", + "The output uses P0-P3 priorities and tight changed-line locations" + ] + }, + { + "id": 2, + "prompt": "Review commit dc488ff14 as if it were a new TTS extension PR. Review only files under ai_agents/agents/ten_packages/extension/mistral_tts_python and ignore every other changed path.", + "expected_output": "A high-signal review of the initial Mistral/Voxtral TTS implementation, especially emitted audio representation, sample-rate truth, streaming, secrets, and tests.", + "files": [], + "expectations": [ + "The review verifies that vendor audio bytes match TEN pcm_frame metadata", + "The review classifies mistral_tts_python as HTTP and applies the HTTP client stream contract", + "The review does not require lifecycle methods already inherited unchanged from AsyncTTS2HttpExtension", + "The review considers sample conversion across streamed chunk boundaries", + "The review checks that request-only fields and credentials are not forwarded or logged", + "The review treats extension-local tests as required and checks the full common matrix plus HTTP-specific cases", + "The review recognizes the extension directory as new and requires current passing guarder evidence with a screenshot and downloadable PCM output in a pull request comment", + "The tts_guarder runner and documentation changes in the commit receive no findings", + "Generic praise and non-actionable style feedback are absent" + ] + }, + { + "id": 3, + "prompt": "Review commit 26cca0807 against its first parent. Limit this TTS review to ai_agents/agents/ten_packages/extension/gradium_tts_python, even though the commit also changes examples and generated manifests.", + "expected_output": "A focused review of latency, TTFB/event ordering, sample-rate handling, configuration, failure completion, and regression coverage in the initial Gradium TTS change.", + "files": [], + "expectations": [ + "The review follows success, vendor error, iterator end, and cancellation paths", + "The review classifies gradium_tts_python as WebSocket/custom rather than applying the HTTP-base checklist", + "The review detects whole-response buffering or duplicate TTFB when present", + "The review separately validates audio duration from emitted PCM bytes and event interval from audio_start to audio_end", + "The review treats extension-local tests as required and checks the full common matrix plus WebSocket-specific lifecycle cases", + "The review recognizes the extension directory as new and requires current passing guarder evidence with a screenshot and downloadable PCM output in a pull request comment", + "Each reported issue explains why base-class guards do not prevent it", + "Example graph and manifest-lock changes receive no findings", + "The output says no actionable findings instead of inventing issues when evidence is insufficient" + ] + }, + { + "id": 4, + "prompt": "Review commit 85c777953 as a TTS extension change, limited to ai_agents/agents/ten_packages/extension/cosy_tts_python. Pay particular attention to every tts_audio_end payload.", + "expected_output": "A line-specific review that detects audio-end interval measurement from the vendor request timestamp instead of the audio-start timestamp and verifies the PCM duration formula independently.", + "files": [], + "expectations": [ + "The review defines request_event_interval_ms as elapsed time from tts_audio_start to tts_audio_end", + "The review applies the WebSocket/custom lifecycle and concurrency guide to the direct AsyncTTS2BaseExtension subclass", + "The review explains that measuring from request_start_ts incorrectly includes TTFB", + "The review checks request_total_audio_duration_ms using emitted PCM bytes, sample rate, sample width, and channels", + "The review checks success, partial error, interruption, and no-audio terminal paths", + "Findings remain limited to the cosy_tts_python extension directory" + ] + } + ] +} diff --git a/.agents/skills/ten-tts-review/references/common.md b/.agents/skills/ten-tts-review/references/common.md new file mode 100644 index 0000000000..7016c37c51 --- /dev/null +++ b/.agents/skills/ten-tts-review/references/common.md @@ -0,0 +1,152 @@ +# Common TEN TTS extension review guide + +Load this guide for every in-scope TTS extension review. Apply it only to changed +files inside the extension directory. + +## Scope and source of truth + +An extension is in scope when its manifest imports the TTS interface or its +implementation inherits a TTS base class. Review implementation code, vendor +clients, addon/configuration code, extension manifest/property files, dependency +metadata, and extension-local tests. + +Do not produce findings for `ten_ai_base`, `integration_tests/tts_guarder`, +examples, graphs, or any other directory. The installed base implementation may +be read only to understand which behavior the extension inherits. + +Use these sources in descending order of authority: + +1. the changed extension and its local tests; +2. the public contract of the installed base class it consumes; +3. a recent extension with the same transport shape; +4. repository documentation and older implementations. + +Copied code is comparison evidence, not proof that a pattern is correct. + +## Request lifecycle and ordering + +Trace every request ID separately through success, empty input, vendor error, +cancellation, and shutdown. + +Verify: + +- Non-empty output sends `tts_audio_start` before the first PCM frame and exactly + one `tts_audio_end` after the last frame. +- Appended text with the same request ID stays one request. A non-final segment + does not complete it, and a final segment does not leave it stuck. +- Empty or whitespace final input does not hang or emit bogus audio. +- Explicit vendor end, iterator exhaustion, error, timeout, and exception paths + converge on one terminal path without duplicate end events or cleanup. +- Request metadata and frames remain associated with the correct request when + request IDs are interleaved. +- Completion releases subsequent work and cleans up request state, recorders, + counters, timestamps, and pending messages owned by the extension. + +Do not require an HTTP extension to reproduce lifecycle work that it inherits +unchanged from `AsyncTTS2HttpExtension`. + +## `tts_audio_end` accounting — mandatory + +Audit every changed extension-owned path that sends `tts_audio_end`. The two +numeric fields have different meanings and must be calculated independently. + +### `request_total_audio_duration_ms` + +This is the media duration represented by PCM frames actually emitted for the +request: + +```text +duration_ms = emitted_pcm_bytes * 1000 + / (sample_rate_hz * bytes_per_sample * channels) +``` + +Verify: + +- Count bytes after decoding, decompression, resampling, format conversion, and + frame alignment. Do not count headers, compressed bytes, base64 text, + conversion input, or a partial sample that was never emitted. +- Use the rate, width, and channels attached to that request's emitted frames. +- Accumulate the whole request ID, including appended text, and reset only at the + request boundary. +- Sum bytes first and convert once; per-chunk rounding accumulates error. +- Success reports all emitted PCM. Error/interruption reports only PCM already + emitted. No emitted audio reports `0`. +- Duplicate terminal handlers cannot reset or reuse counters before the winning + path constructs its payload. + +### `request_event_interval_ms` + +This is elapsed real time from emitting `tts_audio_start` to emitting the matching +`tts_audio_end`. It is neither audio duration nor total vendor request latency. + +Verify: + +- Capture the start exactly once when the first `tts_audio_start` is sent. Starting + at connect, vendor request, or text-send time incorrectly includes TTFB. +- Capture the matching end when `tts_audio_end` is sent. Appended segments do not + replace the original start. +- Prefer a monotonic clock; wall-clock adjustment can corrupt elapsed time. +- Success, error-after-audio, and interrupted-after-audio use the same start/end + boundary. If no audio-start event was sent, report `0`. +- Keep timestamps per request or prove that callbacks cannot pair different + requests. +- Do not substitute PCM duration: synthesis may run faster or slower than real + time. + +A wrong formula, origin, association, or reset boundary is a correctness defect, +not a metrics preference. + +## Audio correctness and latency + +Verify: + +- `pcm_frame` bytes are raw signed PCM matching the declared sample rate, width, + channels, and endianness—not WAV/MP3 headers or vendor-specific floats. +- Resampling and conversion preserve samples split across chunk boundaries. +- Float conversion maps non-finite values safely, clamps before scaling, and does + not overflow or wrap. +- Frame lengths align to `sample_width * channels`; final leftover handling does + not invent, drop silently, or count an incomplete sample. +- Useful chunks are forwarded promptly instead of buffering the whole response. +- A configurable sample rate is proven by extension-local tests; a fixed rate is + acceptable when it matches the vendor. + +## Configuration, security, errors, and metrics + +Verify: + +- Manifest name, addon decorator, dependencies, and imported interface agree + within the extension. +- Required configuration is validated before use. Internal fields such as API + keys, dump controls, base URL, and framework-only audio settings are not + forwarded as vendor payload parameters unless the vendor requires them. +- Keys, authorization headers, signed URLs, tokens, sensitive config, and user + text are not exposed in logs, exceptions, metadata, or fixtures. +- Provider errors include useful vendor information and use fatal/non-fatal + severity consistent with recoverability. +- Error paths terminate the current request without poisoning later requests. +- TTFB is measured from the actual vendor request to the first non-empty audio and + emitted once per request. It must not be reused for audio-end event interval. +- Usage, byte, chunk, character, and duration counters are request-scoped. +- Dump writers write emitted PCM, are isolated per request, and flush on every + terminal path without path traversal through request IDs. + +## Extension-local tests + +Apply the complete merge-gating matrix in [tests.md](tests.md). The suite must +exercise the extension-owned config, client/stream adapter, conversion, event, +and cleanup paths with deterministic fakes; tests of only a base class, shared +guarder, or external integration graph do not satisfy the requirement. + +## Common false positives + +Do not report: + +- inherited base behavior as a defect in an extension that does not override it; +- lack of subtitle alignment when the vendor exposes no timestamps; +- a fixed rate that matches a fixed-rate vendor; +- absence of `tts_audio_start` for a request that emits no frames; +- old defects outside the diff unless the change makes them reachable or worse; +- speculative races without a concrete ordering and observable impact; +- formatting or naming preferences already enforced by CI; +- findings against out-of-scope directories. diff --git a/.agents/skills/ten-tts-review/references/guarder.md b/.agents/skills/ten-tts-review/references/guarder.md new file mode 100644 index 0000000000..ffee1500f4 --- /dev/null +++ b/.agents/skills/ten-tts-review/references/guarder.md @@ -0,0 +1,59 @@ +# New-extension guarder evidence + +Apply this gate only when the TTS extension directory does not exist at the pull +request merge base. Apply it independently to every new TTS extension in the +pull request. + +## Required run + +Require the pull request owner to run the full default guarder suite from +`ai_agents/` with the extension's real service credentials: + +```bash +task tts-guarder-test EXTENSION= +``` + +Use `CONFIG_DIR=` only when the extension does not use +the default `tests/configs`. Passing one selected test is not a full guarder run. +Capability-based skips performed by the guarder are acceptable only when the +comment identifies them and the extension genuinely lacks that capability. + +Never ask the review workflow to expose or print credentials. A failed run, +credential error, or unavailable service is not passing evidence. + +## Required pull request comment + +Inspect all current pull request comments. Require a comment from the pull +request owner or an authorized test workflow that contains, for each new +extension: + +- the extension directory name and the tested pull request HEAD commit SHA; +- the exact guarder command and its final pass/fail/skip counts; +- an inline screenshot showing the command context and final successful test + summary, with credentials and sensitive text redacted; +- a downloadable, non-empty `.pcm` file produced by that guarder run, or a + downloadable archive whose named contents include that `.pcm` file; +- the PCM sample rate, sample width/bit depth, and channel count needed to play + and independently check the raw audio. + +The screenshot and PCM link must be accessible from the pull request comment; +text claiming that they exist locally is insufficient. A CI log link alone is +not a screenshot, and a waveform image or encoded audio file is not the required +PCM output. + +Evidence is stale when its stated SHA differs from the current pull request HEAD. +If the pull request adds multiple TTS extensions, one comment may aggregate the +evidence, but it must contain a distinct result and PCM file for each extension. + +## Review outcome + +Report one P1 merge-blocking finding per new extension when the run is missing or +failed, the evidence is stale, the screenshot is missing, or the PCM attachment +is missing, empty, inaccessible, or not identified as guarder output. Group +multiple missing fields for the same extension into one finding. Anchor the +finding to that extension's newly added `manifest.json` or main extension entry +point and state exactly which evidence is absent. + +Do not review or request implementation changes inside `tts_guarder`; it remains +out of scope. Report infrastructure problems separately as the reason the +required evidence has not yet been satisfied. diff --git a/.agents/skills/ten-tts-review/references/http.md b/.agents/skills/ten-tts-review/references/http.md new file mode 100644 index 0000000000..913add7b20 --- /dev/null +++ b/.agents/skills/ten-tts-review/references/http.md @@ -0,0 +1,71 @@ +# HTTP TTS extension review guide + +Load this guide only when the extension inherits `AsyncTTS2HttpExtension`. + +## Ownership boundary + +The HTTP base normally owns input queuing, request state, audio start/end events, +metrics, dump lifecycle, and terminal cleanup. The extension normally supplies +configuration, client construction, vendor identity/audio format, and an HTTP +client that yields response events. + +First map every changed method to extension-owned or inherited behavior. Treat an +override of `request_tts`, `cancel_tts`, lifecycle hooks, or audio-end helpers as a +larger risk surface: verify why the override is needed and whether it duplicates +or bypasses base guarantees. Do not ask for duplicated lifecycle code when the +base implementation is inherited unchanged. + +## HTTP client stream contract + +Verify: + +- The client checks HTTP status and content type before yielding audio. Error JSON, + HTML, and authentication responses never become PCM frames. +- Response bytes are streamed incrementally. Calls that buffer the full body, + await complete synthesis, or parse a container before yielding do not silently + defeat time-to-first-audio requirements. +- Every request produces an unambiguous sequence of response chunks followed by + one end or error condition. Iterator exhaustion without an explicit end is + handled by the inherited contract rather than leaving the request open. +- Empty network chunks do not trigger audio start or TTFB. +- Conversion to the PCM format declared by the extension occurs before chunks are + yielded to the base. Stateful conversion preserves partial samples across HTTP + chunk boundaries. +- Authentication/config failures are distinguishable from transient request, + timeout, and transport failures so the base receives the correct error class. +- Connect, response-header, first-byte, inter-chunk, and total-request timeouts are + intentional. Cancellation interrupts the active stream promptly. +- Response/session objects close on success, error, cancellation, and shutdown. + +## Configuration and client construction + +Verify: + +- `create_config()` parses and validates the extension schema without mutating a + shared parameters dictionary across requests. +- `create_client()` receives a validated config and does not log secrets or place + them in URLs unnecessarily. +- Internal parameters are removed from the vendor payload without accidentally + deleting supported free-form vendor options. +- The configured output format, sample rate, channels, and sample width match what + the HTTP client actually yields. +- Configuration updates cannot leave a stale client using old credentials or + audio settings. +- Client metadata returned to the base is request-safe and redacted. + +## Cancellation and tests + +The client cancellation primitive must stop the active response body, not merely +set a flag after all bytes have already been buffered. Cleanup must be idempotent +because cancellation can race with normal iterator completion. + +Extension-local tests should drive a fake HTTP client through: + +- chunked success and stream exhaustion; +- non-2xx response before audio; +- error after partial PCM; +- empty chunks and an empty body; +- cancellation during first-byte wait and during streaming; +- PCM conversion split across HTTP chunks; +- inherited lifecycle behavior without reimplementing the base. + diff --git a/.agents/skills/ten-tts-review/references/tests.md b/.agents/skills/ten-tts-review/references/tests.md new file mode 100644 index 0000000000..5f7e6c0374 --- /dev/null +++ b/.agents/skills/ten-tts-review/references/tests.md @@ -0,0 +1,134 @@ +# Required extension-local test matrix + +Every TTS extension must own a deterministic local test suite under its +extension directory. Treat this matrix as a merge gate for every TTS extension +touched by the pull request. Shared base-class, guarder, graph, example, and live +service tests may supplement the suite but cannot replace it. + +Prefer fake clients, fake streams, controlled clocks, and fixed PCM fixtures. +Tests must traverse the extension-owned adapter and assert emitted TEN events; +tests that only instantiate config objects or mock the method under test do not +cover the behavior. + +## Core success and request lifecycle + +Require tests that prove: + +- a normal request emits playable PCM with the declared sample rate, sample + width, channel count, and deterministic event order; +- each request emits `tts_audio_start` and exactly one matching `tts_audio_end` + when audio is produced, with the correct request ID; +- `text_input_end` finalizes the correct request without losing buffered text or + terminating another request; +- sequential requests transition through queued, processing, finalizing, and + completed states without sharing audio, counters, or terminal events; +- appended and rapidly interleaved input remains associated with its request; +- a successful request still works after a recoverable error, cancellation, + flush, retry, or reconnect. + +## Exact audio-end accounting + +These tests are mandatory, not optional metric coverage: + +- Feed known emitted PCM bytes and assert + `request_total_audio_duration_ms == emitted_bytes * 1000 / + (sample_rate * sample_width_bytes * channels)` using the final output format. +- When conversion or alignment changes the bytes, assert against bytes actually + emitted, including chunks split across input boundaries. Sum bytes first and + calculate once so per-chunk rounding cannot accumulate. +- Use a controlled monotonic clock and assert `request_event_interval_ms` is the + elapsed time from the matching `tts_audio_start` to `tts_audio_end`, rather + than request latency, TTFB, vendor processing time, or audio duration. +- Cover success, partial audio followed by error, flush/cancellation after audio + starts, and no-audio termination. Each path must use the correct zero/non-zero + values and emit at most one end event. + +## Flush and cancellation + +Require tests for flush after audio has started, cancellation while connecting +or awaiting the first byte, cancellation during streaming, and cancellation near +normal completion. Assert that: + +- already emitted audio is accounted for exactly; +- no audio or stale terminal event is emitted after the flush boundary; +- resources and dump writers close on every terminal path; +- a late callback cannot revive or complete the cancelled request; +- a subsequent request starts and completes normally. + +## Configuration and parameter mapping + +Require fixtures or equivalent parameterized tests for default settings, custom +settings, at least two supported audio-output configurations, dump enabled, +missing required settings, and invalid settings. Assert that: + +- required credentials and endpoints fail before a request is sent; +- supported voice, speed, language, sample-rate, and audio controls map to the + client request correctly; unsupported controls are rejected or ignored by an + explicit contract test; +- free-form parameters pass through, while framework-only and internal fields do + not leak into the service payload; +- URL/base-URL and authorization-header precedence is deterministic; +- empty values, booleans used as numbers, non-finite numbers, and out-of-range + values are rejected or normalized to documented safe defaults; +- metadata uses canonical field names and omits empty values; +- string representations, logs, exceptions, and fixtures redact credentials, + authorization headers, signed URLs, tokens, and user text. + +## Error behavior and recovery + +Use deterministic failures to cover invalid credentials or required parameters, +authentication rejection, quota or rate limiting, timeout, network failure, +invalid input, and a generic service failure. Where the service exposes language +constraints, include unsupported-language behavior. Assert that: + +- unrecoverable configuration/authentication errors are fatal and transient + transport/service errors are non-fatal when retry or later requests can work; +- emitted errors carry useful service metadata and diagnostic context without + exposing secrets or user text; +- an error before audio and an error after partial audio terminate exactly once; +- request state, buffers, metrics, and connections are clean for the next + successful request. + +## Metrics and dump output + +Require tests for normal, long or multi-chunk, and very fast responses, plus a +flush or cancellation path. Assert that TTFB is measured from service-request +start to the first non-empty audio chunk and emitted once per request. Assert +request-scoped byte, chunk, character, duration, and timing values do not leak +between requests. + +With dumping enabled, assert the dump file exists, contains exactly the PCM bytes +emitted to TEN in the same order, and is finalized on success, error, flush, and +cancellation. + +## Robustness + +Require deterministic tests for concurrent requests, rapid request sequences, +long text, large or many audio chunks, cancellation under load, and retry after a +transient network failure. Assert bounded buffering, correct request ownership, +no duplicated audio, no deadlock, and recovery without restarting the extension. + +## Transport additions + +In addition to the common matrix: + +- HTTP implementations must cover non-success status before audio, invalid or + unexpected content type, chunked stream exhaustion, cancellation closing the + response, and conversion whose input sample boundaries span HTTP chunks. +- WebSocket/custom streaming implementations must cover reconnect after a + dropped connection, replacement-connection ownership, disabling callbacks on + the old connection, disconnect/cancel ordering, failure while reporting a + disconnect, and concurrent cancellation that must not close a replacement + connection. + +If a transport or capability is genuinely absent, the inapplicable conditional +case may be omitted. The reviewer must verify that absence from implementation +and manifest/config rather than accepting an unexplained test gap. + +## Reporting gaps + +- No extension-local suite: report one P1 merge-blocking finding. +- Missing mandatory sections or transport cases: group closely related omissions + into the smallest useful P2 findings; do not list every test name separately. +- Name the unprotected runtime behavior and expected assertion. Do not give vague + advice such as "add more tests." diff --git a/.agents/skills/ten-tts-review/references/websocket.md b/.agents/skills/ten-tts-review/references/websocket.md new file mode 100644 index 0000000000..1a11203a34 --- /dev/null +++ b/.agents/skills/ten-tts-review/references/websocket.md @@ -0,0 +1,85 @@ +# WebSocket and custom TTS extension review guide + +Load this guide when the extension inherits `AsyncTTS2BaseExtension` directly or +owns a comparable long-lived/custom transport. These extensions own substantially +more lifecycle and concurrency behavior than HTTP-base extensions. + +## Connection and protocol lifecycle + +Trace connect, authenticate/handshake, send text, finalize, receive audio, vendor +end, error, reconnect, cancel, and close. + +Verify: + +- The vendor's endpoint, event names, encoding, request/finalize messages, and + close semantics match the actual protocol. +- Connection state changes reflect usable transport state. A socket is not marked + connected before authentication/handshake succeeds, or disconnected while an + active receive task can still publish data. +- Send and receive loops have clear ownership and termination. Background tasks, + timers, callbacks, pings, and queues cannot survive shutdown unnoticed. +- Malformed events, server errors, close frames, ping/keepalive failure, and + inter-chunk stalls terminate or recover the correct request instead of hanging. +- SDK callbacks crossing threads or loops schedule work safely and cannot mutate + completed request state. + +## Request identity and concurrency + +Verify: + +- Each incoming vendor event is correlated to the correct TEN request. A mutable + global `current_request_id` is safe only when the protocol and task ownership + prove that old callbacks cannot overlap a new request. +- Appended text shares the intended vendor context; different request IDs do not + mix contexts, audio, timestamps, counters, metadata, or terminal events. +- Locks are not held while awaiting operations that need the receive/cancel path + to acquire the same lock. +- Queued or buffered text is released exactly once after completion and is not + replayed during reconnect. +- Per-request start/end timestamps and emitted-byte counters follow the common + audio-end accounting rules on every terminal path. + +## Flush and cancellation + +Treat flush as a transport boundary, not only a local queue clear. + +Verify: + +- The active vendor operation is cancelled, closed, or assigned a generation that + prevents all later callbacks from publishing old audio. +- The interrupted request emits at most one appropriate `tts_audio_end`, while + base-owned cleanup is not duplicated. +- No old frame or terminal event can appear after flush completion. A new request + after flush still works. +- Cancellation interrupts connect, send, receive, first-audio wait, reconnect + backoff, and finalization without deadlock. +- `asyncio.CancelledError` is not converted to an ordinary vendor error or + swallowed while the producer keeps running. +- Cleanup is idempotent when vendor end, receive-loop exit, flush, and shutdown + race with each other. + +## Reconnect and recovery + +Verify: + +- Reconnect is bounded and only claimed when wired into the active request path. +- Retry state resets after success and terminal failure is surfaced when the + ceiling is reached. +- Reconnect does not resend already accepted text, duplicate audio, reuse a closed + context, or attach pre-disconnect audio to a new request. +- Whether an interrupted in-flight request is resumed, replayed, or failed is + explicit and proven by tests. +- A request after a recoverable failure succeeds without restarting the extension. + +## WebSocket/custom transport tests + +Use a deterministic fake socket or SDK callback source to cover: + +- handshake success/failure and server-side error events; +- fragmented/binary audio and control messages; +- close before audio, after partial audio, and without an explicit vendor end; +- flush and shutdown while connect/send/receive is blocked; +- late callback from an old request after a new request starts; +- reconnect without replay, duplication, or request-ID leakage; +- exactly-once audio end and accurate partial duration/interval accounting. + diff --git a/.github/prompts/code-review.md b/.github/prompts/code-review.md index 21c44b3ab4..20f441c455 100644 --- a/.github/prompts/code-review.md +++ b/.github/prompts/code-review.md @@ -30,6 +30,23 @@ merge baseline (see docs/ai/L1/05_workflows.md): If the PR is not ASR-related, skip the ASR section. +## TTS extension changes (required when applicable) + +If the diff changes implementation, configuration, dependencies, or tests +inside `ai_agents/agents/ten_packages/extension//`, read +`.agents/skills/ten-tts-review/SKILL.md` and follow it for those extension +files. Load its referenced common, testing, transport, and new-extension +guarder guides as instructed. For a newly added TTS extension, inspect the +existing PR comments and verify current, passing guarder evidence, including +the required result screenshot and downloadable PCM output. + +Do not apply that skill to `ten_ai_base`, `tts_guarder`, examples, graphs, or +files outside the TTS extension directory. Its evidence standard and finding +format take precedence over the generic checklist above. + +Report only actionable issues introduced by this PR. Keep locations tight and +avoid summaries or praise when there are no findings. + Use `gh pr diff` (and `gh pr view` as needed) with the PR NUMBER above to inspect this PR's changes. The checked-out worktree is the base branch, not the PR head; do not run scripts or install dependencies from the PR branch. diff --git a/AGENTS.md b/AGENTS.md index ea3a7af04c..a28987798c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,3 +59,12 @@ sections 6 (generate) and 7 (bootstrap). - `ai_agents/` — primary area for agents, examples, server, integrations - `core/`, `packages/`, `build/` — framework internals + +## Domain Skills + +- For pull-request or diff reviews of implementation, configuration, dependency, + or test changes inside `ai_agents/agents/ten_packages/extension//`, + load and follow + [ten-tts-review](.agents/skills/ten-tts-review/SKILL.md). +- Do not apply that skill to `ten_ai_base`, `tts_guarder`, examples, graphs, or + paths outside the TTS extension directory.