[ISSUE-030] Fix voices get 404 on cloned/custom voices — custom-voice fallback

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.

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.

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(

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():

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 ──────────────────────────────────────────────────────