From ad4e75d8587b84cb76d78d6e153a7874bd7ed0c1 Mon Sep 17 00:00:00 2001 From: pillip Date: Mon, 15 Jun 2026 16:02:02 +0900 Subject: [PATCH 1/2] fix: voices get falls back to custom-voice endpoint on 404 (ISSUE-030) get_voice queried only the preset endpoint (/v1/voices/{id}), so cloned voices (/v1/custom-voices/{id}) returned 404 even though edit/delete work on the same id. Add a narrow 404 detector (_is_not_found_error) and fall back to client.custom_voices.get_custom_voice, building Voice(type=custom). Auth/network errors still propagate without a spurious second call; if both endpoints miss, raise APIError with a clear not-found message. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/supertone_cli/client.py | 39 ++++++++++++++- tests/test_client.py | 94 ++++++++++++++++++++++++++++++++++++- tests/test_voices.py | 27 +++++++++++ 3 files changed, 156 insertions(+), 4 deletions(-) diff --git a/src/supertone_cli/client.py b/src/supertone_cli/client.py index 09153cc..e3187b9 100644 --- a/src/supertone_cli/client.py +++ b/src/supertone_cli/client.py @@ -389,8 +389,32 @@ def clone_voice(name: str, sample_path: str) -> CloneResult: raise APIError(str(exc)) from exc +def _is_not_found_error(exc: Exception) -> bool: + """Detect a NOT-FOUND (404) error from an SDK exception. + + Checked narrowly so that auth/network errors do NOT trigger the + custom-voice fallback. Mirrors the lazy-import pattern in + ``_is_auth_error`` -- the SDK is imported only when needed. + """ + try: + from supertone.errors import NotFoundErrorResponse + + if isinstance(exc, NotFoundErrorResponse): + return True + except ImportError: + pass + + return getattr(exc, "status_code", None) == 404 + + def get_voice(voice_id: str) -> Voice: - """Get details of a specific voice.""" + """Get details of a specific voice. + + Tries the preset endpoint first (``/v1/voices/{id}``). If that returns + a 404, falls back to the custom-voice endpoint (``/v1/custom-voices/{id}``) + so cloned voices resolve too. Auth/network errors propagate without a + spurious fallback call. + """ client = get_client() try: v = client.voices.get_voice(voice_id=voice_id) @@ -408,7 +432,18 @@ def get_voice(voice_id: str) -> Voice: except Exception as exc: if _is_auth_error(exc): raise AuthError(str(exc)) from exc - raise APIError(str(exc)) from exc + if not _is_not_found_error(exc): + raise APIError(str(exc)) from exc + # Preset 404 -> fall back to the custom-voice endpoint. + try: + v = client.custom_voices.get_custom_voice(voice_id=voice_id) + return _build_voice(v, voice_type="custom") + except (AuthError, APIError): + raise + except Exception as inner: + if _is_auth_error(inner): + raise AuthError(str(inner)) from inner + raise APIError(f"Voice not found: {voice_id}") from inner def edit_custom_voice( diff --git a/tests/test_client.py b/tests/test_client.py index ed582df..24c1000 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -264,7 +264,9 @@ def test_search_voices_returns_filtered_list(): assert len(voices) == 1 assert isinstance(voices[0], Voice) assert voices[0].id == "v2" - mock_client.voices.search_voices.assert_called_once_with(language="ko", gender="female") + mock_client.voices.search_voices.assert_called_once_with( + language="ko", gender="female" + ) def test_get_voice_returns_voice(): @@ -288,6 +290,92 @@ def test_get_voice_returns_voice(): assert voice.name == "Test" +class _StatusError(Exception): + """Simple SDK-like error carrying a status_code (mirrors how the + client detects 404/401 via getattr(exc, 'status_code', None)).""" + + def __init__(self, message: str, status_code: int) -> None: + super().__init__(message) + self.status_code = status_code + + +def test_get_voice_preset_does_not_call_custom(): + """When the preset endpoint succeeds, the custom endpoint is NOT called.""" + from supertone_cli.client import get_voice + from supertone_cli.models import Voice + + mock_client = MagicMock() + mock_v = MagicMock() + mock_v.voice_id = "v1" + mock_v.name = "Preset" + mock_v.language = "ko" + mock_v.gender = "male" + mock_v.age = "adult" + mock_v.use_cases = ["audiobook"] + mock_client.voices.get_voice.return_value = mock_v + + with patch("supertone_cli.client.get_client", return_value=mock_client): + voice = get_voice("v1") + assert isinstance(voice, Voice) + assert voice.type == "preset" + mock_client.custom_voices.get_custom_voice.assert_not_called() + + +def test_get_voice_falls_back_to_custom_on_404(): + """When the preset endpoint 404s, fall back to the custom endpoint.""" + from supertone_cli.client import get_voice + from supertone_cli.models import Voice + + mock_client = MagicMock() + mock_client.voices.get_voice.side_effect = _StatusError("not found", 404) + + mock_cv = MagicMock() + mock_cv.voice_id = "c1" + mock_cv.name = "Cloned" + mock_cv.language = "ko" + mock_cv.gender = "female" + mock_cv.age = "young" + mock_cv.use_cases = [] + mock_client.custom_voices.get_custom_voice.return_value = mock_cv + + with patch("supertone_cli.client.get_client", return_value=mock_client): + voice = get_voice("c1") + assert isinstance(voice, Voice) + assert voice.type == "custom" + assert voice.id == "c1" + mock_client.custom_voices.get_custom_voice.assert_called_once_with( + voice_id="c1" + ) + + +def test_get_voice_both_miss_raises_api_error(): + """When both preset and custom endpoints 404, raise APIError.""" + from supertone_cli.client import get_voice + + mock_client = MagicMock() + mock_client.voices.get_voice.side_effect = _StatusError("not found", 404) + mock_client.custom_voices.get_custom_voice.side_effect = _StatusError( + "not found", 404 + ) + + with patch("supertone_cli.client.get_client", return_value=mock_client): + with pytest.raises(APIError): + get_voice("missing") + + +def test_get_voice_auth_error_does_not_fall_back(): + """An auth error on the preset lookup surfaces as AuthError; no fallback.""" + from supertone_cli.client import get_voice + + mock_client = MagicMock() + mock_client.voices.get_voice.side_effect = _StatusError("unauthorized", 401) + + with patch("supertone_cli.client.get_client", return_value=mock_client): + with pytest.raises(AuthError): + get_voice("v1") + mock_client.custom_voices.get_custom_voice.assert_not_called() + + def test_edit_custom_voice_returns_result(): from supertone_cli.client import edit_custom_voice from supertone_cli.models import CloneResult @@ -310,7 +398,9 @@ def test_delete_custom_voice_calls_sdk(): mock_client = MagicMock() with patch("supertone_cli.client.get_client", return_value=mock_client): delete_custom_voice("v1") - mock_client.custom_voices.delete_custom_voice.assert_called_once_with(voice_id="v1") + mock_client.custom_voices.delete_custom_voice.assert_called_once_with( + voice_id="v1" + ) def test_stream_speech_yields_chunks(): diff --git a/tests/test_voices.py b/tests/test_voices.py index dc610fa..bfa478c 100644 --- a/tests/test_voices.py +++ b/tests/test_voices.py @@ -142,6 +142,33 @@ def test_voices_get_format_json(): assert data["name"] == "Test" +def test_voices_get_custom_voice_human_readable(): + """voices get on a custom voice exits 0 and shows the custom type.""" + voice = Voice( + id="c1", + name="My Clone", + type="custom", + languages=["ko"], + gender="female", + age="young", + use_cases=[], + ) + with patch("supertone_cli.client.get_voice", return_value=voice): + result = runner.invoke(app, ["voices", "get", "c1"]) + assert result.exit_code == 0 + assert "custom" in result.output + + +def test_voices_get_custom_voice_format_json(): + """voices get --format json on a custom voice reports type custom.""" + voice = Voice(id="c1", name="My Clone", type="custom", languages=["ko"]) + with patch("supertone_cli.client.get_voice", return_value=voice): + result = runner.invoke(app, ["voices", "get", "c1", "--format", "json"]) + assert result.exit_code == 0 + data = json.loads(result.output) + assert data["type"] == "custom" + + # ── voices edit ────────────────────────────────────────────────────── From 0f41117966d3ef5e64433123ad5e7264e05efd44 Mon Sep 17 00:00:00 2001 From: pillip Date: Mon, 15 Jun 2026 16:09:55 +0900 Subject: [PATCH 2/2] review(ISSUE-030): add review notes and lessons (RL-003, RL-004) Senior review: APPROVE. No Critical/High findings. Verified narrow 404 detection (auth/network errors do not trigger fallback, no spurious second call), preset path unchanged. Two non-blocking Low follow-ups recorded. uv run pytest: 163 passed, 1 skipped. ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/review_lessons.md | 23 +++++++++++ docs/review_notes.md | 90 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) diff --git a/docs/review_lessons.md b/docs/review_lessons.md index d2d923e..4580874 100644 --- a/docs/review_lessons.md +++ b/docs/review_lessons.md @@ -26,3 +26,26 @@ counter, and an Observed-In list. - Prevention: When branching on a format marker that a spec allows in multiple forms, normalize case (or match both) before testing. Low impact when inputs are constrained, but cheap to guard at write time. + +## [RL-003] Test mocks expose attributes the real SDK response does not + +- Category: Testing +- Frequency: 1 +- Observed-In: ISSUE-030 (custom-voice fallback tests build `MagicMock`s with + `gender`/`age`/`language`/`use_cases`, but the real `GetCustomVoiceResponse` + only carries `voice_id`/`name`/`description`; the test would not catch a + regression that assumes a non-existent required attribute) +- Prevention: When mocking an SDK response, mirror the real model's actual field + set (check `model_fields`). Prefer constructing the real response object or a + mock limited to the documented fields, so the test fails if production code + starts depending on attributes the endpoint never returns. + +## [RL-004] New branch in a helper is only exercised via one detection path + +- Category: Testing +- Frequency: 1 +- Observed-In: ISSUE-030 (`_is_not_found_error` has a typed-isinstance branch + and a `status_code == 404` branch; tests only cover the latter) +- Prevention: When a helper has multiple detection branches (typed exception vs + duck-typed attribute), add a direct unit test per branch rather than relying + on one integration path to cover all of them. diff --git a/docs/review_notes.md b/docs/review_notes.md index 536e1e6..f2e1644 100644 --- a/docs/review_notes.md +++ b/docs/review_notes.md @@ -106,3 +106,93 @@ None. No Critical/High findings; the Low items are intentionally not gold-plated - Optional: accept lowercase `t` separator and/or validate input format in the CLI layer for clearer error messages than a server 400. + +## ISSUE-030 — voices get custom fallback + +### Verdict: APPROVE + +The fix is correct, minimal, and conforms to project patterns. All ACs are met, +tests pass (163 passed, 1 skipped), ruff is clean. No fixes required. + +### Code Review findings + +- (Correct) `_is_not_found_error` follows the established lazy-import pattern of + `_is_auth_error` (no top-level `import supertone`), preserving the startup + latency contract from architecture.md (client.py is the only SDK-importing + module). Verified no module-level SDK import was added. +- (Correct) Ordering in `get_voice` is sound: `_is_auth_error` (401/403) is + checked before `_is_not_found_error` (404), so auth errors raise `AuthError` + and never trigger the custom-voice fallback. Confirmed the SDK base + `SupertoneError` always sets `status_code` from the HTTP response, so the + typed-isinstance check and the `status_code == 404` check are both valid; the + typed check is harmless belt-and-suspenders. +- (Correct) Network/transport errors have no `status_code` and are not + `NotFoundErrorResponse`, so `_is_not_found_error` returns False and they + propagate as `APIError` with no spurious second call. Matches AC. +- (Correct) Preset success path is byte-for-byte unchanged. +- (Correct) The inner fallback re-checks `_is_auth_error(inner)` so a custom + endpoint that itself returns 401/403 surfaces as `AuthError`, not a misleading + "Voice not found". Good defensive handling. +- (Low — test realism) The real `GetCustomVoiceResponse` SDK model exposes only + `voice_id`, `name`, `description` — it has NO `gender`/`age`/`language`/ + `use_cases`. The fallback handles this gracefully because `_build_voice` reads + via `_attr` with safe defaults (gender/age -> None, languages/use_cases -> + []), so no crash. However, the new tests build `MagicMock`s with all of those + attributes populated, which does not reflect the real sparse response. The + tests still pass and exercise the branch, but they would not catch a future + regression where `_build_voice` assumes a required attribute. Non-blocking; + consider asserting the realistic sparse shape (e.g. `gender is None`). +- (Low — coverage gap) Tests cover the generic `status_code == 404` detection + path but never the typed `isinstance(exc, NotFoundErrorResponse)` branch of + `_is_not_found_error`. The branch is trivial and the SDK guarantees + `status_code`, so impact is minimal; a direct unit test of + `_is_not_found_error` with a constructed `NotFoundErrorResponse` would close + the gap. + +### Security Findings + +- No new injection, secret-exposure, deserialization, or access-control issues. + No shell execution; `voice_id` flows only as a keyword arg to the SDK. +- (Informational) Both `APIError(str(exc))` and the literal + `APIError(f"Voice not found: {voice_id}")` rely on the existing top-level + handler's `sanitize_message` for API-key stripping, consistent with all other + branches in client.py. No regression vs existing code. The `voice_id` is + user-supplied but contains no secret. Severity: none. + +### AC verification + +- cloned id -> Type: custom, exit 0: covered (test_get_voice_falls_back_to_custom_on_404, + test_voices_get_custom_voice_human_readable). +- preset id unchanged (Type: preset), no custom call: covered + (test_get_voice_preset_does_not_call_custom). +- neither found -> APIError (exit 1): covered (test_get_voice_both_miss_raises_api_error). +- --format json on custom -> type "custom": covered + (test_voices_get_custom_voice_format_json). +- auth error -> no fallback, AuthError: covered + (test_get_voice_auth_error_does_not_fall_back). + +### Self-review + +- Severity re-assessment: no finding rises above Low; no exploit path exists. +- False-positive check: the preset path uses inline `Voice(...)` while the + fallback uses `_build_voice` — this duplication is pre-existing and unchanged + by this PR, so not flagged as a new issue. +- Blind-spot scan: checked injection, secrets, network-error misclassification, + ordering — all clean. +- Confidence: High. + +### Fixes applied during review + +None. No Critical/High findings. + +### Tests / Lint + +- `uv run pytest -q`: 163 passed, 1 skipped. +- `uv run ruff check .`: All checks passed. + +### Follow-ups (non-blocking) + +- Add a unit test for `_is_not_found_error` covering the typed + `NotFoundErrorResponse` branch. +- Make the custom-fallback tests use a realistic sparse response shape + (voice_id/name/description only) and assert defaulted fields.