Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Guarder evidence —
|
|
[P1] Wait for the finishing worker before acknowledging flush ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:320
[P1] Complete the required guarder evidence before merge ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment promises a screenshot in a subsequent comment, but none is attached in the current comments. The PCM download is accessible and non-empty; the log alone does not satisfy the new-extension screenshot gate. Also, [P2] Propagate cancellation while awaiting the previous response drain ai_agents/agents/ten_packages/extension/kotoba_tts_python/kotoba_tts.py:364 Flush request A, start B while A's cancellation drain is pending, then flush B. Cancelling B's worker propagates cancellation into the awaited drain, but this handler swallows [P2] Flush request usage counters before starting the next request ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:327 The normal/error terminal path calls [P2] Capture the audio interval origin at audio-start emission ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:287
[P2] Add the mandatory transport shutdown and ownership tests ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_reconnect.py:16 Beyond the regressions above, the local suite does not exercise flush/shutdown while handshake or send is blocked, failure in the disconnect-reporting callback, or cancellation of old-session work after a replacement connection exists. These are explicit merge-gating cases in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f86171c01d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Kotoba TTS (Japanese) over the fal-hosted WebSocket API, built on AsyncTTS2BaseExtension and the official kotoba-sdk client. Sentence buffering for streamed LLM text, one reused connection with reconnect, cancel on flush, PCM16 at 8/16/24 kHz, fatal/non-fatal error mapping (including fal's 1008 Unauthorized close), TTFB and usage metrics, per-request dumps. Extension-local tests run against an in-process fake of the service (tests/fake_kotoba_server.py); tests/configs_fake lets the TTS guarder run without a service key.
f86171c to
4c8ef0a
Compare
|
[P1] Mark the response active before awaiting the text send ai_agents/agents/ten_packages/extension/kotoba_tts_python/kotoba_tts.py:277 If flush cancels the worker while [P1] Await the terminalizing worker before acknowledging flush ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:320
[P1] Supply current guarder evidence and the required screenshot ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment cites [P2] Propagate caller cancellation while awaiting a response drain ai_agents/agents/ten_packages/extension/kotoba_tts_python/kotoba_tts.py:364 Flush A, start B while A's cancellation drain is pending, then flush B. Cancelling B propagates cancellation into the awaited drain, but this handler swallows it and lets [P2] Redact retained credentials and signed URLs before logging configuration ai_agents/agents/ten_packages/extension/kotoba_tts_python/config.py:110
[P2] Emit and reset interrupted usage before accepting the next request ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:327 Cancellation sends audio end without calling [P2] Capture the event-interval origin at audio-start emission ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:287
[P2] Start TTFB timing at the vendor text request ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:245 This timestamp precedes both the awaited request-metrics emission and the lazy generator's connection/authentication handshake. First requests and reconnects therefore report local metrics backpressure and connection setup as synthesis TTFB. Capture the origin at the client's actual text-send boundary and carry it back to the extension; independently delay handshake, metrics delivery, and first audio in the test. [P2] Add the mandatory blocked-transport and connection-ownership cases ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_reconnect.py:16 The local suite does not exercise shutdown/flush during a blocked handshake or send, failure while reporting a disconnect, or cancellation of old-session work after a replacement connection exists. These are explicit merge-gating cases in |
…ounting, redaction) - Mark the response active before the text send and drop the socket when the send is interrupted, so a flush cannot leave a response on a socket the next request reuses. - Flush joins a worker that is already emitting its terminal events (bounded), and audio_end_sent is set only after the event is out, so no event of the old request lands after tts_flush_end and there is exactly one audio_end per request. - Propagate caller cancellation out of the cancel drain and drop the socket when a drain is interrupted (double flush no longer waits on the response timeout). - Emit usage metrics on interrupt so the next request's counters are clean; start the event interval at tts_audio_start emission; measure TTFB from the text send and report handshake time as connect_delay; count bytes only after the frame is sent; a dump write failure no longer fails the request. - Redact every retained parameter value and URL credentials in to_str(). - Status callbacks that raise are logged, never propagated. - Tests: cancellation under blocked send / slow handshake / double flush / stray audio / terminal race / hung worker / usage isolation / stop while blocked / failing callback; accounting under a controlled clock; redaction; dump failure. 65 passed. Also list kotoba_tts_python in the TTS guarder WebSocket allowlist so test_connection_status runs for it.
|
[P1] Supply guarder evidence for the current head and finish the screenshot upload ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The evidence comment identifies [P2] Sanitize the endpoint before returning vendor metadata ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:132 For a custom endpoint such as the credential-bearing URL already used in [P2] Handle dump-directory failures without stranding the request queue ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:207 With dumping enabled, an unwritable [P2] Report remote idle closure without waiting for another text request ai_agents/agents/ten_packages/extension/kotoba_tts_python/kotoba_tts.py:248 After reporting connected, the client does not observe the SDK receiver's termination while idle. When the service closes an idle socket, [P2] Exercise actual blocked writes and replacement-connection ownership ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_cancellation.py:62
[P2] Complete exact accounting coverage for error and interrupted termination ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_accounting.py:90 The controlled-clock test covers successful completion only. The partial-error and interrupted cases here assert duration but never assert the matching start-to-end interval; the other flush test only checks that it is positive. All audio fixtures also contain complete, millisecond-aligned samples, leaving alignment and rounding accounting untested. The mandatory local-test matrix requires exact assertions for these paths. Add controlled elapsed-time checks for error/interruption and split-sample or sub-millisecond chunks, checking duration against the PCM actually emitted and ensuring no residual sample crosses requests. |
…etup, idle close, alignment) - vendor_metadata() returns the endpoint with credentials and signed query values masked, so no event metadata carries URL secrets. - A dump_path that cannot be created disables recording for the request instead of raising out of request_tts() and stranding the base queue. - A watcher per connection observes the SDK receiver ending while idle and reports disconnected promptly, retiring only that session. - Only whole samples are emitted and counted; a trailing half sample is held until the next chunk and never crosses requests. - Tests: the actual write is gated and flushed / stopped while suspended; late old-session cleanup preserves the replacement connection; idle close is reported before the next text; controlled-clock interval on error and interrupt paths; split-sample chunks; unusable dump_path; endpoint secrets absent from event metadata. 74 passed.
|
[P1] Refresh guarder evidence for the current head ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment identifies [P2] Sanitize SDK error messages before forwarding them ai_agents/agents/ten_packages/extension/kotoba_tts_python/kotoba_tts.py:100 For a signed or credential-bearing custom endpoint that fails DNS resolution or refuses the connection, SDK 0.2.3 raises [P2] Retire a closed session after its active response finishes ai_agents/agents/ten_packages/extension/kotoba_tts_python/kotoba_tts.py:271 If the server sends [P2] Exercise old-session cleanup after the replacement is connected ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_cancellation.py:495 This test delays the server's cancelled response, but B's [P2] Cover dump finalization on large streams and partial errors ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_dump.py:22 The dump-enabled tests cover small successful responses and early flushes; none forces output past the writer's 64 KiB automatic-flush threshold or enables dumping during a vendor failure after partial audio. Thus the mandatory long-stream and error-terminal dump checks are absent, leaving buffered-tail preservation and cleanup during an outstanding write unverified. Add deterministic large-stream/slow-writer cases and a partial vendor error, asserting that finalized files exactly match emitted PCM and that a subsequent request remains isolated. |
…-driven tests - Drop the two redundant `except CancelledError: raise` clauses that pylint W0706 flagged; cancellation already passes through `except Exception`. Keep the intent as comments. - Remove the unused `connected` property and `SentenceBuffer.clear()`. - Tests from the coverage and matrix audit: unterminated tail sent at text_input_end, completed-id history eviction, unexpected negotiated format is fatal, unexpected worker exception ends the request and recovers, failing on_connecting / on_connected callbacks are logged only, blank / zero config fields and boolean-for-float are rejected, dump finalized on the error path, very fast response. 85 passed; pylint 10.00/10.
|
[P1] Refresh guarder evidence for the current head and attach the screenshot ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment identifies [P2] Sanitize SDK diagnostics before forwarding them ai_agents/agents/ten_packages/extension/kotoba_tts_python/kotoba_tts.py:100 For a credential-bearing custom endpoint that fails DNS resolution or refuses the connection, SDK 0.2.3 raises [P2] Retire a closed transport after its active response finishes ai_agents/agents/ten_packages/extension/kotoba_tts_python/kotoba_tts.py:267 If the server sends [P2] Finalize the recorder when flush takes over terminal cleanup ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:395 With dumping enabled, let the worker enter [P2] Exercise old-session cleanup after the replacement connects ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_cancellation.py:495 This test delays the server's cancelled response, but B's [P2] Cover dump finalization while an automatic write is pending ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_dump.py:22 The dump-enabled local tests use short responses or interrupt after a few chunks; none reaches the writer's 64 KiB automatic-flush boundary or holds an actual write pending during finalization. The new partial-error test also uses only 3,200 bytes. This leaves the required large-stream and cancellation-under-load behavior unprotected, particularly whether buffered tails survive completion or flush while a write is outstanding. Add deterministic output beyond that threshold with a controlled slow writer, assert finalized files exactly match emitted PCM, and verify a subsequent request stays isolated. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57ede877a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,72 @@ | |||
| { | |||
| "type": "extension", | |||
| "name": "kotoba_tts_python", | |||
There was a problem hiding this comment.
Attach current live guarder evidence for Kotoba
I inspected the supplied PR comments for this new TTS extension, but none provides current-HEAD evidence for the full live task tts-guarder-test EXTENSION=kotoba_tts_python run: the final counts, result screenshot, downloadable non-empty PCM, and its sample format are all absent. Add those artifacts before merging so real-service compatibility is established for the reviewed commit.
AGENTS.md reference: AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
…s, dump lifecycle, metrics) - Scrub endpoint credentials from every diagnostic string the client logs or emits (SDK errors quote the URL they tried); init errors are built from pydantic's error types only, never rejected input values; config type errors name the type, not the value; unexpected exceptions emit only their type. - A request that fails because initialization failed now gets a zero-duration ERROR terminal event before finish_request. - The handshake delay is attributed to the request that opened the connection even when that request ends without audio. - A server close right after response.done retires the session and reports disconnected once the response is consumed. - The recorder is finalized on the flush takeover path and when the app stops during terminal cleanup; _close_recorder flushes until the PCMWriter buffer is empty so no tail is lost behind a pending write. - Tests: connection refused on a credential-bearing endpoint, malformed property with a marker value, init-failed terminal event, connect_delay attribution, close after done, old/new connection ordering proof, takeover and stop-during-flush dumps, 96 KB dump with a slowed writer and flush under load. 92 passed; pylint 10.00/10.
|
[P1] Refresh guarder evidence for the current head and attach its screenshot ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment identifies [P2] Preserve recorder ownership when terminal cleanup is cancelled ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:537
[P2] Complete usage cleanup when flush takes over a terminal worker ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:421 Flush can arrive after [P2] Cover the required vendor rejection and recovery cases locally ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_error_msg.py:122 The local error tests cover authentication, unsupported language, transport failures, and |
…imits and text rejections The worker, flush and shutdown each ran a different subset of the terminal sequence, and a flush or stop that cancelled the worker partway could leave usage counters unsent, lose the tail of a dump, or orphan a request whose dump was still being written after the next request had started. - extension.py: a single _finalize() runs error, audio_end, usage, finish_request and the dump flush under a per-request lock; each step sets its flag right after its await, so a caller that takes over repeats only what was cut off. Requests stay in _finishing until their dump is closed and on_stop joins all of them. Flush waits on a `settled` event (events out) instead of the whole worker, so it never waits on the dump. The recorder is detached only after its flush, and each flush is shielded so a cancelled caller does not cancel the writer's task. TTFB reads the clock before any await. The connect-delay value is parked on the request before its send. - kotoba_tts.py: a 429 handshake maps to code rate_limited (retryable); request-level codes (invalid_text, text_too_long) are not retried. - tests: handshake_status_count knob; tests for transient and persistent 429, service-side text rejection, usage after a flush takeover, stop during a gated dump flush, a next request overlapping the previous dump flush then stop, and flush waiting on terminal events that land in time. - remove the stray .coverage data file.
|
[P1] Refresh guarder evidence for the current PR head ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment identifies [P2] Prevent queued requests from starting during shutdown cleanup ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:150 With dumping enabled, keep P in a slow terminal dump flush, have A currently synthesizing and B buffered, then stop. This snapshot includes P and A, but the base input loop keeps running until [P2] Give each dump a collision-resistant filename ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:274 Distinct valid request IDs such as [P2] Cover long text through the extension adapter ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_state_machine.py:24 The adapter tests send only a few short sentences; the hard-limit test exercises |
…hed request; refuse requests once stopping - on_stop: a request that never reached its end (no text_input_end) gets its worker stopped and its dump closed, without tts_audio_end; the base class emits none on shutdown either and the guarder's append test relies on it. Requests already in their terminal path are still completed. - on_stop sets _stopping first and request_tts refuses new input while it is set, so a request the base class releases during the shutdown waits (A flushing its dump, B completing, C queued) cannot start behind the snapshot; the snapshot is re-read until no request is left. - test: test_request_released_during_shutdown_never_starts.
|
[P1] Refresh guarder evidence for the current PR head ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment identifies [P2] Give each request instance a collision-resistant dump filename ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:298 With dumping enabled, distinct valid request IDs such as [P2] Cover long text through the extension adapter ai_agents/agents/ten_packages/extension/kotoba_tts_python/tests/test_state_machine.py:24 The adapter tests use only a few short sentences; the hard-limit test exercises |
Round 4: terminal path restructuredCodex's re-review of
Flush and shutdown can run concurrently (the runtime dispatches Two more items from the same pass:
Verification on |
…t per init-failed request; unique dump files; long-input tests - config: _redact_url masks every query value and the userinfo instead of matching a list of sensitive names (access_token, X-Amz-Signature and any other name are covered); _SENSITIVE_KEYS removed, params values were already all masked. - extension: the init-failed branch of request_tts runs after the finished-id check and records the id after its single error and audio_end(ERROR), so further deltas of the same request only release the base queue instead of repeating the terminal events. - extension: dump files are named <sanitized id>_<sha256[:8] of the raw id> and truncated on open, so ids that sanitize alike never share a file and a stale file is never appended to. - tests: URL redaction unit table, endpoint secrets under arbitrary query names, init failure across deltas, colliding dump ids, stale dump file, and test_long_input.py (2,000+ character input, 60 streamed deltas under backlog, flush under a 30-sentence backlog with recovery).
|
[P1] Refresh the guarder evidence and attach the required screenshot ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment identifies [P2] Let active-request flush finish before waiting for dump I/O ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:486 With dumping enabled, flush an actively streaming request while an automatic PCM write is still pending or the final file write is slow. |
Round 5: four findings on
|
| # | Finding | Change |
|---|---|---|
| 1 | Custom URL credentials can leak through the query-name list | _redact_url() no longer uses a name list: every query value is masked (access_token=***, X-Amz-Signature=***), userinfo becomes ***@, key names stay visible. The list _SENSITIVE_KEYS is gone; params values were already all masked. Tests: test_redacted_base_url_masks_every_query_value (unit, parametrized) and test_event_metadata_never_carries_endpoint_secrets now carries access_token and X-Amz-Signature values and asserts none reach any event payload. |
| 2 | Initialization failure emits duplicate terminal events per delta | The init-failed branch now runs after the finished-id check and records the id with _remember_completed() after its one error and one audio_end(ERROR); later deltas of the same id take the late-input path, which only releases the base queue. Test: test_init_failure_reports_each_request_once_across_deltas (three deltas, one error, one end, next request still reported). |
| 3 | PCM dump filenames collide; PCMWriter appends |
dump_file_name() = sanitized id + 8 hex of a SHA-256 of the raw id, and the file is truncated when the recorder opens. Tests: test_colliding_request_ids_get_separate_dump_files (a/b vs a_b, each file exact) and test_existing_dump_file_is_replaced_not_appended. |
| 4 | Long-input coverage | tests/test_long_input.py: one 2,000+ character input segmented into 40 sentences and spoken in order on one connection; 60 streamed deltas forming 30 sentences, order preserved under backlog; 30 sentences queued then flushed while the first streams: one INTERRUPTED end before tts_flush_end, at most two texts reached the service, no late frames, next request recovers on the same socket. |
Verification: tests/bin/start 110 passed, 1 xfailed, three consecutive runs identical; pylint 10.00/10; black -l 80 clean. Live guarder on 6bd6e2b0: 16 passed, 1 skipped (387.57 s); evidence comment above updated.
With dumping enabled, flushing an actively streaming request awaited the dump close inside _finalize, so a slow debug disk delayed tts_flush_end and held the base class's input gate although synthesis was already cancelled. - The dump close runs as a tracked task on the request (_close_dump), started once every terminal event is out; flush returns at `settled` on both of its paths. The request stays in _finishing until the task completes. - on_stop joins the worker, runs _finalize, joins the dump task (bounded by _TERMINAL_JOIN_SECONDS), then closes the dump inline for anything left: it is the one caller that waits for the file. _join_terminal_worker becomes _join_bounded and serves both tasks. - test: test_flush_during_streaming_does_not_wait_for_dump_io.
|
[P1] Refresh guarder evidence for the current head and attach the screenshot ai_agents/agents/ten_packages/extension/kotoba_tts_python/manifest.json:3 The owner's evidence comment identifies [P2] Handle cancellation of the joined worker during shutdown ai_agents/agents/ten_packages/extension/kotoba_tts_python/extension.py:613 If a worker stalls in its terminal events, flush starts |
Round 6: flush no longer waits on dump I/OFinding: with dumping enabled, flush of an actively streaming request awaited the dump close inside
Verification: |
Summary
Adds
kotoba_tts_python, a TTS extension for Kotoba Japanese text-to-speech served on fal (Kotoba-Technologies/kotoba-tts).AsyncTTS2BaseExtension, built on the officialkotoba-sdkclient.request_idand each complete sentence becomes one service request; the remainder is sent ontext_input_end.response.cancel), drops buffered text and reportstts_audio_endwithINTERRUPTED.params.sample_rate),tts_audio_endduration computed once from emitted bytes, event interval fromtts_audio_start.Unauthorized), unsupported language / sample rate. Non-fatal: worker and transport failures, a 429 on the handshake (rate_limited, retried once), a text the service rejects (invalid_text, not retried); the next request starts on a fresh connection._finalize) shared by completion, flush and shutdown: error,tts_audio_end, usage, state transition, dump flush. Each step records its completion, so a flush or stop that interrupts the worker completes only what is left; a request stays registered until its dump is closed, soon_stopcan finish a dump that is still being written after the next request has started.paramsvalue and every URL query value is masked, userinfo is stripped.Configuration
See
ai_agents/agents/ten_packages/extension/kotoba_tts_python/README.md.Language. The public
kotoba-ttsapp serves Japanese (language: "ja"). The guarder's built-in texts are English; the run below passes because the Japanese voice reads them, but the intended input is Japanese. The extension-local tests use Japanese text.Tests
Extension-local tests (
tests/) run against an in-process fake of the Kotoba service (tests/fake_kotoba_server.py) so the real SDK client is exercised: lifecycle, append / interleaved / late input, exact audio-end accounting under a controlled clock, flush before and after audio, flush while the send or handshake is blocked, double flush with a pending cancel drain, stray audio after cancel, flush racing the terminal path (events landing in time, and a hung worker taken over with usage still reported), flush during streaming with the dump write held, stop during a dump flush that outlasts the join bound, a next request overlapping the previous request's dump flush then stop, flush and stop while the write itself is suspended, late old-session cleanup with a replacement connection, idle close reported before the next text, reconnect without replay, failing status callbacks, split-sample alignment, unusable dump path, colliding and stale dump files, a 2,000-character input and a 30-sentence backlog with flush and recovery, endpoint secrets under arbitrary query names, init failure across several deltas, transient and persistent 429 on the handshake, service-side text rejection, error classification and secret redaction in logs and event metadata, metrics, dump.task lint-extension EXTENSION=kotoba_tts_python: 10.00/10.black -l 80clean.Outside the extension directory, the PR adds
kotoba_tts_pythonto the TTS guarder's WebSocket allowlist (tests/bin/start,test_connection_status.py) sotest_connection_statusruns for it.Guarder evidence
Extension:
kotoba_tts_python, HEADfd719c3a7fb74a97dc72ffc2894083e41723e89eScreenshot and
.pcmoutput (16 kHz, 16-bit, mono) attached below.