diff --git a/docs/review_notes.md b/docs/review_notes.md index f2e1644..e07dc28 100644 --- a/docs/review_notes.md +++ b/docs/review_notes.md @@ -196,3 +196,43 @@ None. No Critical/High findings. `NotFoundErrorResponse` branch. - Make the custom-fallback tests use a realistic sparse response shape (voice_id/name/description only) and assert defaulted fields. + +## ISSUE-031 — --stream default model auto-select + +PR #60. Reviewed branch `issue/ISSUE-031-stream-default-model`. + +### Code Review +- **Verdict**: Approve. No Critical/High findings. +- The fix at `src/supertone_cli/commands/tts.py:265-268` is correct and minimal. + All four paths verified: stream+no-model → `sona_speech_1`; stream+explicit + incompatible → `validate_params` raises `InputError` (tts.py:60-61); stream+ + explicit `sona_speech_1` → resolves correctly; non-streaming → unchanged. +- Streaming returns early (tts.py:319) before batch/JSON branches, so no + interaction with those paths. +- Security: no surface. Model is a hardcoded literal or validated against + `VALID_MODELS` before reaching the SDK. No secrets, injection, or auth change. + +### AC Coverage +- AC #1 (auto-default): `test_stream_defaults_to_sona_speech_1`. +- AC #2 (explicit compatible streams): `test_stream_calls_stream_speech` + (exit 0) + `test_stream_with_file_save`. +- AC #3 (explicit incompatible errors): `test_stream_explicit_incompatible_model_still_errors`, + now also asserting the error message. +- Config-default-bypass guarantee: `test_stream_default_overrides_config_default_model` + (added during review — the load-bearing path was previously untested, per RL-004). + +### Fixes Applied During Review +- Added `test_stream_default_overrides_config_default_model` covering the + config `default_model` bypass (Low finding — the PR's core guarantee). +- Hardened `test_stream_explicit_incompatible_model_still_errors` to assert the + `"Streaming requires sona_speech_1"` message, pinning the validate_params path. +- Added `keep in sync with _STREAM_MODELS` note to the fix comment (Nit/RL-005): + the `"sona_speech_1"` literal duplicates the `_STREAM_MODELS` set (tts.py:36). + +### Tests / Lint +- `pytest -q` (worktree): 166 passed. +- `uv run ruff check .`: All checks passed. + +### Follow-ups (non-blocking) +- If a second streaming model is ever added, derive the streaming default from + `_STREAM_MODELS` rather than the hardcoded literal (latent divergence, RL-005). diff --git a/src/supertone_cli/commands/tts.py b/src/supertone_cli/commands/tts.py index 38fe2db..2ae0428 100644 --- a/src/supertone_cli/commands/tts.py +++ b/src/supertone_cli/commands/tts.py @@ -54,7 +54,8 @@ def validate_params(model: str, **kwargs: object) -> None: bad = set(params) - _SUPERTONIC_ALLOWED - {"stream"} if bad: raise InputError( - f"Not supported by {model}: {', '.join(sorted(bad))}. Only speed is supported." + f"Not supported by {model}: {', '.join(sorted(bad))}. " + "Only speed is supported." ) if params.get("stream") and model not in _STREAM_MODELS: @@ -85,7 +86,8 @@ def _resolve_text( if not sources and not stdin_has_data: raise InputError( - "No input provided. Pass text as argument, use --input , or pipe via stdin." + "No input provided. Pass text as argument, use --input , " + "or pipe via stdin." ) if text: @@ -256,7 +258,15 @@ def _run_tts( # noqa: PLR0913 "set a default: supertone config set default_voice " ) - resolved_model = model or get_default("default_model") or "sona_speech_2" + # Streaming only supports sona_speech_1 (keep in sync with _STREAM_MODELS). + # When the user hasn't explicitly passed -m/--model, default the streaming + # path to sona_speech_1 so it works without an extra flag (config + # default_model is bypassed here for the same reason — any other default + # would always fail streaming). + if stream and model is None: + resolved_model = "sona_speech_1" + else: + resolved_model = model or get_default("default_model") or "sona_speech_2" resolved_lang = lang or get_default("default_lang") or "ko" # Validate model-parameter compatibility @@ -270,7 +280,9 @@ def _run_tts( # noqa: PLR0913 stream=stream if stream else None, ) - voice_settings = _build_settings_kwargs(speed, pitch, pitch_variance, similarity, text_guidance) + voice_settings = _build_settings_kwargs( + speed, pitch, pitch_variance, similarity, text_guidance + ) # Batch mode: directory input + outdir if _is_batch_input(input) and outdir: @@ -288,7 +300,9 @@ def _run_tts( # noqa: PLR0913 return if format == "json" and output == "-": - raise InputError("Cannot use --format json with --output -: both write to stdout.") + raise InputError( + "Cannot use --format json with --output -: both write to stdout." + ) resolved_text = _resolve_text(text, input) @@ -345,7 +359,9 @@ def register_tts_command(app: typer.Typer) -> None: @app.command("tts") def tts_cmd( # noqa: PLR0913 text: Optional[str] = typer.Argument(None, help="Text to synthesize."), - input: Optional[str] = typer.Option(None, "--input", "-i", help="Path to text file."), + input: Optional[str] = typer.Option( + None, "--input", "-i", help="Path to text file." + ), output: Optional[str] = typer.Option( None, "--output", @@ -387,7 +403,9 @@ def tts_cmd( # noqa: PLR0913 pitch_variance: Optional[float] = typer.Option( None, "--pitch-variance", help="Pitch variance." ), - similarity: Optional[float] = typer.Option(None, "--similarity", help="Voice similarity."), + similarity: Optional[float] = typer.Option( + None, "--similarity", help="Voice similarity." + ), text_guidance: Optional[float] = typer.Option( None, "--text-guidance", help="Text guidance." ), @@ -420,8 +438,12 @@ def register_predict_command(app: typer.Typer) -> None: @app.command("tts-predict") def predict_cmd( - text: Optional[str] = typer.Argument(None, help="Text to predict duration for."), - input: Optional[str] = typer.Option(None, "--input", "-i", help="Path to text file."), + text: Optional[str] = typer.Argument( + None, help="Text to predict duration for." + ), + input: Optional[str] = typer.Option( + None, "--input", "-i", help="Path to text file." + ), voice: Optional[str] = typer.Option(None, "--voice", "-v", help="Voice ID."), model: Optional[str] = typer.Option(None, "--model", "-m", help="TTS model."), lang: Optional[str] = typer.Option(None, "--lang", "-l", help="Language code."), diff --git a/tests/test_streaming.py b/tests/test_streaming.py index 32dacb1..400e6fa 100644 --- a/tests/test_streaming.py +++ b/tests/test_streaming.py @@ -56,6 +56,58 @@ def test_stream_missing_sounddevice(): assert isinstance(result.exception, InputError) +def test_stream_defaults_to_sona_speech_1(): + """--stream without -m auto-selects sona_speech_1 (ISSUE-031).""" + mock_sd = MagicMock() + mock_stream = MagicMock(return_value=iter([b"chunk1"])) + + with ( + patch("supertone_cli.client.stream_speech", mock_stream), + patch.dict("sys.modules", {"sounddevice": mock_sd}), + ): + result = runner.invoke( + app, + ["tts", "Hello", "--voice", "v1", "--stream"], + ) + assert result.exit_code == 0 + assert mock_stream.call_args.kwargs["model"] == "sona_speech_1" + + +def test_stream_default_overrides_config_default_model(): + """--stream without -m picks sona_speech_1 even when config default_model + is a non-streaming model (the load-bearing guarantee of ISSUE-031).""" + mock_sd = MagicMock() + mock_stream = MagicMock(return_value=iter([b"chunk1"])) + + def fake_default(key): + return "sona_speech_2" if key == "default_model" else None + + with ( + patch("supertone_cli.client.stream_speech", mock_stream), + patch("supertone_cli.commands.tts.get_default", side_effect=fake_default), + patch.dict("sys.modules", {"sounddevice": mock_sd}), + ): + result = runner.invoke( + app, + ["tts", "Hello", "--voice", "v1", "--stream"], + ) + assert result.exit_code == 0 + assert mock_stream.call_args.kwargs["model"] == "sona_speech_1" + + +def test_stream_explicit_incompatible_model_still_errors(): + """Explicit -m with a non-streaming model is still rejected.""" + mock_sd = MagicMock() + with patch.dict("sys.modules", {"sounddevice": mock_sd}): + result = runner.invoke( + app, + ["tts", "Hello", "--voice", "v1", "--stream", "--model", "sona_speech_2"], + ) + assert result.exit_code != 0 + assert isinstance(result.exception, InputError) + assert "Streaming requires sona_speech_1" in str(result.exception) + + def test_stream_with_file_save(tmp_path): """--stream + --output saves to file too.""" mock_chunks = [b"chunk1", b"chunk2"]