diff --git a/docs/review_lessons.md b/docs/review_lessons.md new file mode 100644 index 0000000..d2d923e --- /dev/null +++ b/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. diff --git a/docs/review_notes.md b/docs/review_notes.md index bd279bc..536e1e6 100644 --- a/docs/review_notes.md +++ b/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. diff --git a/src/supertone_cli/client.py b/src/supertone_cli/client.py index c1ea02e..09153cc 100644 --- a/src/supertone_cli/client.py +++ b/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 = [] diff --git a/tests/test_usage.py b/tests/test_usage.py index f8cec54..e093230 100644 --- a/tests/test_usage.py +++ b/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 ─────────────────────────────────────────────────────