diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..55b8932 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,13 @@ +# Changelog + +## Unreleased + +### Fixed + +- Correct legacy feedback serialization: existing `request_id`/`score` arguments + now send `trace_id`/integer `value`. Only integer scores from −10 through 10 + are valid; integral floats are accepted without rescaling. Fractional, + out-of-range, boolean, non-numeric and nonfinite scores fail before dispatch. +- Reject unsupported nonempty comments, including whitespace-only text, instead + of sending them. Omitted, `None` and empty comments are omitted. Both client + classes retain their synchronous feedback helper and `None` return shape. diff --git a/README.md b/README.md index 76ea141..e2b02c4 100644 --- a/README.md +++ b/README.md @@ -213,6 +213,33 @@ parses the gateway's `x-routeplane-*` response headers: `provider`, `trace_id`, `request_id`, `cache`, `guardrails`, `hedged`, `shed`, `budget_remaining`, `budget_warning`, `compliance_warning`, `pii_masked`, `idempotent_replayed`. +## Legacy feedback + +Use the gateway-generated request ID from response metadata, not the provider's +completion body ID: + +```python +completion, meta = client.create_with_meta( + model="gpt-4o-mini", + messages=[{"role": "user", "content": "Hello!"}], +) +if meta.request_id is not None: + client.feedback.create(request_id=meta.request_id, score=1) +``` + +The helper keeps its `request_id` and `score` arguments and sends +`{"trace_id": "req_...", "value": 1}` to `POST /v1/feedback`. Scores must be +integers from −10 through 10. Integral floats such as `1.0` become JSON integers +without rescaling. Fractional, out-of-range and nonfinite values raise +`ValueError`; booleans and non-numeric types raise `TypeError`, before dispatch. + +Comments are unsupported: omitted, `None` or empty-string comments are omitted +from the wire. Every nonempty comment, including whitespace-only text, raises +`ValueError`; other comment types raise `TypeError`. Successful calls return +`None`, acknowledging acceptance rather than durable storage or target existence. +The feedback helper is synchronous on both `Routeplane` and `AsyncRouteplane`; +do not `await` it. + ## Prompt management Managed prompt templates are fetched, rendered, and run through the ordinary diff --git a/src/routeplane/resources/feedback.py b/src/routeplane/resources/feedback.py index bb76389..7407219 100644 --- a/src/routeplane/resources/feedback.py +++ b/src/routeplane/resources/feedback.py @@ -4,7 +4,7 @@ from typing import Optional -from ._base import BaseResource, prune_none +from ._base import BaseResource __all__ = ["FeedbackResource"] @@ -19,6 +19,23 @@ def create( score: float, comment: Optional[str] = None, ) -> None: - """``POST /v1/feedback`` — score a request (``request_id``) with an optional note.""" - body = prune_none({"request_id": request_id, "score": score, "comment": comment}) + """Score a gateway request using the legacy ``trace_id``/``value`` wire. + + ``score`` must be an integer from -10 through 10; integral floats are + accepted without rescaling. Invalid types raise ``TypeError`` and + invalid values raise ``ValueError`` before any request is sent. + Comments are unsupported: omit them or pass ``None``/``""``. + Returns ``None`` on acknowledgement, not a durability guarantee. + """ + if isinstance(score, bool) or not isinstance(score, (int, float)): + raise TypeError("score must be a number (int or float), not a boolean") + # Check the bounds before converting: this also rejects NaN/infinity + # and avoids coercing huge integers or truncating fractional scores. + if not -10 <= score <= 10 or int(score) != score: + raise ValueError("score must be an integer from -10 through 10") + if comment is not None and not isinstance(comment, str): + raise TypeError("comment must be a string or None") + if comment is not None and comment != "": + raise ValueError("comment is not supported by the legacy feedback endpoint") + body = {"trace_id": request_id, "value": int(score)} self._post("feedback", json=body) diff --git a/tests/test_feedback.py b/tests/test_feedback.py new file mode 100644 index 0000000..37190cf --- /dev/null +++ b/tests/test_feedback.py @@ -0,0 +1,85 @@ +"""Legacy feedback wire and pre-dispatch validation on every exposed path.""" + +import json + +import httpx +import pytest +import pytest_asyncio +import respx + +from routeplane import AsyncRouteplane, Routeplane +from routeplane.resources import FeedbackResource + +BASE = "https://gateway.example.test/v1" + + +@pytest_asyncio.fixture(params=["resource", "sync_client", "async_client"]) +async def feedback(request): + if request.param == "resource": + with httpx.Client() as transport: + yield FeedbackResource(api_key="rp_test", base_url=BASE, http_client=transport) + elif request.param == "sync_client": + with Routeplane(api_key="rp_test", base_url=BASE) as client: + yield client.feedback + else: + async with AsyncRouteplane(api_key="rp_test", base_url=BASE) as client: + # REST helpers on AsyncRouteplane are intentionally synchronous. + yield client.feedback + + +@pytest.mark.parametrize("score", [-10, 0, 10, -10.0, -1.0, -0.0, 1.0, 10.0]) +@pytest.mark.parametrize("comment", [{}, {"comment": None}, {"comment": ""}]) +@pytest.mark.parametrize("status", [200, 204]) +@respx.mock +async def test_feedback_legacy_wire(feedback, score, comment, status): + route = respx.post(f"{BASE}/feedback").mock(return_value=httpx.Response(status)) + assert feedback.create(request_id="req_from_gateway", score=score, **comment) is None + assert route.call_count == 1 + sent = route.calls.last.request + body = json.loads(sent.content) + assert body == {"trace_id": "req_from_gateway", "value": int(score)} + assert type(body["value"]) is int + assert sent.headers["x-routeplane-api-key"] == "rp_test" + + +@pytest.mark.parametrize( + "score", [-11, 11, -10.1, 10.1, -0.5, 0.5, float("nan"), float("inf"), float("-inf"), 10**1000] +) +@respx.mock +async def test_feedback_invalid_score_value_never_dispatches(feedback, score): + with pytest.raises(ValueError, match="score.*integer.*-10.*10"): + feedback.create(request_id="req_1", score=score) + assert len(respx.calls) == 0 + + +@pytest.mark.parametrize("score", [True, False, None, "1", [], {}, 1 + 0j]) +@respx.mock +async def test_feedback_invalid_score_type_never_dispatches(feedback, score): + with pytest.raises(TypeError, match="score.*number"): + feedback.create(request_id="req_1", score=score) + assert len(respx.calls) == 0 + + +@pytest.mark.parametrize("comment", ["note", " ", "\t", "\n", "\u200b"]) +@respx.mock +async def test_feedback_unsupported_comment_never_dispatches(feedback, comment): + with pytest.raises(ValueError, match="comment.*not supported"): + feedback.create(request_id="req_1", score=0, comment=comment) + assert len(respx.calls) == 0 + + +@pytest.mark.parametrize("comment", [False, 0, [], {}]) +@respx.mock +async def test_feedback_invalid_comment_type_never_dispatches(feedback, comment): + with pytest.raises(TypeError, match="comment"): + feedback.create(request_id="req_1", score=0, comment=comment) + assert len(respx.calls) == 0 + + +@respx.mock +async def test_feedback_preserves_http_error_contract(feedback): + route = respx.post(f"{BASE}/feedback").mock(return_value=httpx.Response(401)) + with pytest.raises(httpx.HTTPStatusError) as error: + feedback.create(request_id="req_1", score=0) + assert error.value.response.status_code == 401 + assert route.call_count == 1 diff --git a/tests/test_meta_convenience.py b/tests/test_meta_convenience.py index b220972..c37efb7 100644 --- a/tests/test_meta_convenience.py +++ b/tests/test_meta_convenience.py @@ -1,4 +1,9 @@ -"""Tests for ``create_with_meta`` / ``stream_with_meta`` on both clients.""" +"""Tests for ``create_with_meta`` / ``stream_with_meta`` on both clients. + +Inject httpx clients so respx intercepts every request. Newer OpenAI versions +default to httpx2, which these httpx fixtures do not mock. This deliberately +tests the supported explicit-client path without changing production defaults. +""" import json @@ -54,7 +59,7 @@ def _sse(): @respx.mock def test_create_with_meta_sync(): respx.post(CHAT).mock(return_value=httpx.Response(200, json=_COMPLETION, headers=_META_HEADERS)) - client = Routeplane(api_key="rp_test", base_url=BASE) + client = Routeplane(api_key="rp_test", base_url=BASE, http_client=httpx.Client()) try: completion, meta = client.create_with_meta( model="gpt-4o", messages=[{"role": "user", "content": "hi"}] @@ -69,7 +74,7 @@ def test_create_with_meta_sync(): @respx.mock def test_stream_with_meta_sync(): respx.post(CHAT).mock(return_value=_sse()) - client = Routeplane(api_key="rp_test", base_url=BASE) + client = Routeplane(api_key="rp_test", base_url=BASE, http_client=httpx.Client()) try: stream = client.stream_with_meta( model="gpt-4o", messages=[{"role": "user", "content": "hi"}] @@ -85,7 +90,7 @@ def test_stream_with_meta_sync(): @respx.mock async def test_create_with_meta_async(): respx.post(CHAT).mock(return_value=httpx.Response(200, json=_COMPLETION, headers=_META_HEADERS)) - client = AsyncRouteplane(api_key="rp_test", base_url=BASE) + client = AsyncRouteplane(api_key="rp_test", base_url=BASE, http_client=httpx.AsyncClient()) try: completion, meta = await client.create_with_meta( model="gpt-4o", messages=[{"role": "user", "content": "hi"}] @@ -99,7 +104,7 @@ async def test_create_with_meta_async(): @respx.mock async def test_stream_with_meta_async(): respx.post(CHAT).mock(return_value=_sse()) - client = AsyncRouteplane(api_key="rp_test", base_url=BASE) + client = AsyncRouteplane(api_key="rp_test", base_url=BASE, http_client=httpx.AsyncClient()) try: stream = await client.stream_with_meta( model="gpt-4o", messages=[{"role": "user", "content": "hi"}] diff --git a/tests/test_resources.py b/tests/test_resources.py index 3dbf08c..73d4bd5 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -199,15 +199,16 @@ def test_feedback_create_prunes_comment(): route = respx.post(f"{BASE}/feedback").mock(return_value=httpx.Response(204)) assert FeedbackResource(**_kwargs()).create(request_id="req_1", score=1.0) is None body = _body(route) - assert body == {"request_id": "req_1", "score": 1.0} + assert body == {"trace_id": "req_1", "value": 1} + assert type(body["value"]) is int assert "comment" not in body @respx.mock def test_feedback_create_with_comment(): - route = respx.post(f"{BASE}/feedback").mock(return_value=httpx.Response(204)) - FeedbackResource(**_kwargs()).create(request_id="req_1", score=0.0, comment="meh") - assert _body(route) == {"request_id": "req_1", "score": 0.0, "comment": "meh"} + with pytest.raises(ValueError, match="comment.*not supported"): + FeedbackResource(**_kwargs()).create(request_id="req_1", score=0.0, comment="meh") + assert len(respx.calls) == 0 # --- residency -------------------------------------------------------------