[ISSUE-029] Fix usage analytics date serialization — ISO-8601 datetime

docs/review_lessons.md

@@ -0,0 +1,28 @@
+# Review Lessons
+
+Preventable patterns extracted during code review. Each entry has a stable ID,
+a category (Code Quality / Security / Testing / Architecture), a Frequency
+counter, and an Observed-In list.
+
+---
+
+## [RL-001] Date/datetime passed to API without normalizing to the format the endpoint requires
+
+- Category: Code Quality
+- Frequency: 1
+- Observed-In: ISSUE-029 (usage analytics forwarded `YYYY-MM-DD` to an endpoint
+  requiring full ISO-8601 datetime, causing a server 400)
+- Prevention: When a CLI flag accepts a human-friendly date but the SDK/endpoint
+  needs a stricter format, normalize at the client boundary and decide
+  start-of-day vs end-of-day semantics explicitly. Catch at implementation time
+  by reading the SDK/endpoint contract for each datetime argument.
+
+## [RL-002] Format/separator detection uses an exact-case substring check
+
+- Category: Code Quality
+- Frequency: 1
+- Observed-In: ISSUE-029 (`"T" in value` misses ISO-8601's permitted lowercase
+  `t` separator)
+- 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.

docs/review_notes.md

@@ -36,3 +36,73 @@
 ## Follow-ups
 - After 1–2 releases, drop one of the duplicate "no httpx in `list_custom_voices`" source-inspection tests.
 - Consider a project policy: cosmetic reformat-only hunks land in their own commit, not mixed with logic changes.
+
+## ISSUE-029 — usage analytics ISO datetime
+
+Bug-fix PR adding `_to_iso_datetime(value, *, end)` in `src/supertone_cli/client.py`
+and applying it to `start_time`/`end_time` in `get_usage_analytics` only.
+
+### Verdict: APPROVE
+
+Scope is minimal and matches the issue exactly. `get_voice_usage` is untouched
+(0 references in the client diff). The lazy `supertone` import inside
+`get_usage_analytics` is preserved; `_to_iso_datetime` is a pure string helper
+that imports nothing, so startup latency (NFR-002) is unaffected.
+
+### Code Review findings
+
+- AC met: date-only start -> `...T00:00:00Z`; date-only end -> `...T23:59:59Z`;
+  a value already containing `T` is passed through unchanged. Confirmed by
+  `client.py:472-482, 497-498`.
+- Tests assert SDK kwargs via a mocked `client.usage.get_usage` (start-of-day,
+  end-of-day, and pass-through cases) and a CLI test exits 0. Test code reviewed
+  with the same rigor; the mock shape (`data` -> bucket -> `results`) matches what
+  `get_usage_analytics` parses, so the green is meaningful, not a false positive.
+- [Low] Lowercase `t` detection gap: the membership test `"T" in value` only
+  matches uppercase `T`. ISO-8601 permits a lowercase `t` separator. A value like
+  `2026-06-01t08:30:00Z` would be mis-treated as date-only and mangled into
+  `...t08:30:00ZT00:00:00Z`. Not exploitable and not produced by the documented
+  `YYYY-MM-DD` CLI input; left as-is to avoid gold-plating. Fix if ever needed:
+  `if "T" in value or "t" in value`.
+- [Low] No validation of empty/malformed input: `""` becomes `"T00:00:00Z"` and
+  would yield a server 400. This is no worse than pre-fix behavior and no upstream
+  validation existed; out of scope for this PR.
+- [Info] The `Z` (UTC) assumption is hardcoded. Acceptable: the CLI documents
+  plain `YYYY-MM-DD` and the endpoint accepts UTC; documenting timezone behavior
+  in `--help` would be a nice future touch, not a blocker.
+
+### Security Findings
+
+- No new secrets, credentials, or API keys introduced.
+- No injection surface: helper does pure string concatenation of values that are
+  later sent as SDK kwargs over HTTPS (no shell, no SQL, no template).
+- Error handling unchanged: existing `AuthError`/`APIError` mapping in
+  `get_usage_analytics` is preserved; the helper raises nothing new.
+- No XSS / deserialization / CORS surface (CLI, no web output).
+- Overall: no security findings at any severity.
+
+### Self-review
+
+- Severity re-assessment: only two Low findings; neither has an exploit path or
+  data-loss path, so Low is justified (not politics).
+- False-positive check: verified the pass-through branch and mock shape against
+  the real parsing code; confirmed `get_voice_usage` diff is empty.
+- Blind-spot scan: re-read for injection/secrets/error-handling/auth — none apply
+  to a pure string helper on a CLI.
+- AC verification: all three transformation rules + `get_voice_usage` untouched +
+  test expectations are satisfied.
+- Confidence: High.
+
+### Tests / Lint
+
+- `uv run pytest -q`: 157 passed, 1 skipped.
+- `uv run ruff check .`: All checks passed.
+
+### Fixes applied during review
+
+None. No Critical/High findings; the Low items are intentionally not gold-plated.
+
+### Follow-ups (non-blocking)
+
+- Optional: accept lowercase `t` separator and/or validate input format in the
+  CLI layer for clearer error messages than a server 400.

src/supertone_cli/client.py

@@ -469,6 +469,19 @@ def get_usage() -> Usage:
         raise APIError(str(exc)) from exc
 
 
+def _to_iso_datetime(value: str, *, end: bool) -> str:
+    """Normalise a date or datetime string to a full ISO-8601 datetime.
+
+    The ``/v1/usage`` endpoint requires a full ISO-8601 datetime; a plain
+    ``YYYY-MM-DD`` triggers a server 400. A date-only value is expanded to
+    the start of the day (``end=False``) or the end of the day (``end=True``).
+    A value that already contains a ``T`` (full datetime) is returned as-is.
+    """
+    if "T" in value:
+        return value
+    return f"{value}T23:59:59Z" if end else f"{value}T00:00:00Z"
+
+
 def get_usage_analytics(
     start_time: str,
     end_time: str,
@@ -481,8 +494,8 @@ def get_usage_analytics(
 
         bw = BucketWidth(bucket_width)
         response = client.usage.get_usage(
-            start_time=start_time,
-            end_time=end_time,
+            start_time=_to_iso_datetime(start_time, end=False),
+            end_time=_to_iso_datetime(end_time, end=True),
             bucket_width=bw,
         )
         results = []

tests/test_usage.py

@@ -1,7 +1,7 @@
 """Tests for usage commands."""
 
 import json
-from unittest.mock import patch
+from unittest.mock import MagicMock, patch
 
 from typer.testing import CliRunner
 
@@ -118,6 +118,75 @@ def test_usage_analytics_empty():
     assert "No usage data" in result.output
 
 
+# ── usage analytics date serialization (ISSUE-029) ───────────────────
+
+
+def _build_analytics_mock_client():
+    """Build a MagicMock client whose get_usage returns a parseable shape."""
+    mock_client = MagicMock()
+    mock_bucket = MagicMock()
+    mock_bucket.starting_at = "2026-06-01"
+    mock_bucket.ending_at = "2026-06-02"
+    mock_result = MagicMock()
+    mock_result.minutes_used = 5.5
+    mock_result.voice_id = "v1"
+    mock_result.voice_name = "Voice1"
+    mock_result.model = "sona_speech_2"
+    mock_bucket.results = [mock_result]
+    mock_response = MagicMock()
+    mock_response.data = [mock_bucket]
+    mock_client.usage.get_usage.return_value = mock_response
+    return mock_client
+
+
+def test_analytics_date_only_start_converts_to_iso():
+    """A date-only start is forwarded to the SDK as start-of-day ISO datetime."""
+    from supertone_cli.client import get_usage_analytics
+
+    mock_client = _build_analytics_mock_client()
+    with patch("supertone_cli.client.get_client", return_value=mock_client):
+        get_usage_analytics("2026-06-01", "2026-06-15", "day")
+
+    kwargs = mock_client.usage.get_usage.call_args.kwargs
+    assert kwargs["start_time"] == "2026-06-01T00:00:00Z"
+
+
+def test_analytics_date_only_end_converts_to_iso():
+    """A date-only end is forwarded to the SDK as end-of-day ISO datetime."""
+    from supertone_cli.client import get_usage_analytics
+
+    mock_client = _build_analytics_mock_client()
+    with patch("supertone_cli.client.get_client", return_value=mock_client):
+        get_usage_analytics("2026-06-01", "2026-06-15", "day")
+
+    kwargs = mock_client.usage.get_usage.call_args.kwargs
+    assert kwargs["end_time"] == "2026-06-15T23:59:59Z"
+
+
+def test_analytics_full_datetime_passed_through_unchanged():
+    """A value already containing a 'T' is forwarded unchanged."""
+    from supertone_cli.client import get_usage_analytics
+
+    mock_client = _build_analytics_mock_client()
+    with patch("supertone_cli.client.get_client", return_value=mock_client):
+        get_usage_analytics("2026-06-01T08:30:00Z", "2026-06-15T19:45:00Z", "day")
+
+    kwargs = mock_client.usage.get_usage.call_args.kwargs
+    assert kwargs["start_time"] == "2026-06-01T08:30:00Z"
+    assert kwargs["end_time"] == "2026-06-15T19:45:00Z"
+
+
+def test_usage_analytics_date_range_cli_exits_zero():
+    """CLI analytics with plain dates exits 0 with the SDK mocked."""
+    mock_client = _build_analytics_mock_client()
+    with patch("supertone_cli.client.get_client", return_value=mock_client):
+        result = runner.invoke(
+            app,
+            ["usage", "analytics", "--start", "2026-06-01", "--end", "2026-06-15"],
+        )
+    assert result.exit_code == 0
+
+
 # ── usage voices ─────────────────────────────────────────────────────