diff --git a/litellm/proxy/lens/feedback_endpoints.py b/litellm/proxy/lens/feedback_endpoints.py index f9479873a84..59dac3f35ec 100644 --- a/litellm/proxy/lens/feedback_endpoints.py +++ b/litellm/proxy/lens/feedback_endpoints.py @@ -56,6 +56,8 @@ def write_scope(auth: UserAPIKeyAuth) -> Scope: return Scope(all_teams=True) if auth.user_role == LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY: raise HTTPException(403, "Admin viewers cannot write feedback") + if not auth.team_id and not auth.token: + raise HTTPException(403, "Feedback requires a team or API key") return Scope(team_id=auth.team_id or "", api_key_hash="" if auth.team_id else auth.token or "") diff --git a/litellm/proxy/lens/feedback_repository.py b/litellm/proxy/lens/feedback_repository.py index 9ee1af164e2..3228cd2792a 100644 --- a/litellm/proxy/lens/feedback_repository.py +++ b/litellm/proxy/lens/feedback_repository.py @@ -1,6 +1,7 @@ import hashlib from collections.abc import Mapping from datetime import datetime, timezone +from itertools import chain from typing import Final, Protocol from litellm.proxy.lens.feedback_models import Feedback, FeedbackInput, TraceFeedback, TraceFeedbackSummary @@ -165,7 +166,7 @@ class ClickHouseFeedbackStore: **access_parameters(scope).model_dump(), trace_ids=sorted({t.trace_id for t in traces}) ), ) - return tuple(summary for trace in traces for summary in _summaries(trace, rows)) + return tuple(chain.from_iterable(_summaries(trace, rows) for trace in traces)) def _summaries(trace: TraceIdentity, rows: tuple[FeedbackSummaryRow, ...]) -> tuple[TraceFeedbackSummary, ...]: diff --git a/tests/test_litellm_rust/test_traces.py b/tests/test_litellm_rust/test_traces.py index f07bc776bce..3596d1d343c 100644 --- a/tests/test_litellm_rust/test_traces.py +++ b/tests/test_litellm_rust/test_traces.py @@ -174,10 +174,11 @@ async def test_schema_setup_uses_configured_retention(recording_server: Recordin request.raw_body for request in recording_server.requests if b"MODIFY TTL" in request.raw_body ) assert all(b"INTERVAL 7 DAY" in statement for statement in ttl_statements) - assert tuple(request.raw_body.strip() for request in recording_server.requests[-3:]) == ( + assert tuple(request.raw_body.strip() for request in recording_server.requests[-4:]) == ( b"ALTER TABLE `trace_test`.otel_traces MODIFY TTL toDateTime(Timestamp) + INTERVAL 7 DAY", b"ALTER TABLE `trace_test`.agent_traces_by_key MODIFY TTL toDateTime(StartTs) + INTERVAL 7 DAY", b"ALTER TABLE `trace_test`.spend_logs MODIFY TTL toDateTime(start_time) + INTERVAL 7 DAY", + b"ALTER TABLE `trace_test`.lens_feedback MODIFY TTL toDateTime(CreatedAt) + INTERVAL 7 DAY", ) diff --git a/tests/unit/proxy/lens/test_feedback_endpoints.py b/tests/unit/proxy/lens/test_feedback_endpoints.py index 3ee7c79f169..71d60e22eea 100644 --- a/tests/unit/proxy/lens/test_feedback_endpoints.py +++ b/tests/unit/proxy/lens/test_feedback_endpoints.py @@ -94,34 +94,31 @@ class FakeClickHouse(ClickHouseStorage): and self._visible(str(r["TeamId"]), str(r["ApiKeyHash"]), parameters) ) case LensFeedbackSummaryParams(): - live = tuple( + live: Final = tuple( r for r in self._latest() if r["TraceId"] in parameters.trace_ids and self._visible(str(r["TeamId"]), str(r["ApiKeyHash"]), parameters) ) - keys = sorted({(str(r["TeamId"]), str(r["ApiKeyHash"]), str(r["TraceId"])) for r in live}) - return tuple( - FeedbackSummaryRow( - trace_id=trace, - trace_ref=ref(team, key, trace), - count=len(scores), - average=sum(scores) / len(scores), - lowest=min(scores), - ) - for team, key, trace in keys - for scores in [ - [ - int(str(r["Score"])) - for r in live - if (r["TeamId"], r["ApiKeyHash"], r["TraceId"]) == (team, key, trace) - ] - ] - ) + keys: Final = sorted({(str(r["TeamId"]), str(r["ApiKeyHash"]), str(r["TraceId"])) for r in live}) + return tuple(_summary_row(live, team, key, trace) for team, key, trace in keys) case _: raise AssertionError(f"unexpected query {query.name}") +def _summary_row(live: tuple[Mapping[str, object], ...], team: str, key: str, trace: str) -> FeedbackSummaryRow: + scores: Final = tuple( + int(str(r["Score"])) for r in live if (r["TeamId"], r["ApiKeyHash"], r["TraceId"]) == (team, key, trace) + ) + return FeedbackSummaryRow( + trace_id=trace, + trace_ref=ref(team, key, trace), + count=len(scores), + average=sum(scores) / len(scores), + lowest=min(scores), + ) + + def store(**traces: tuple[tuple[str, str], ...]) -> ClickHouseFeedbackStore: return ClickHouseFeedbackStore(FakeClickHouse(traces or {"t1": (("team-a", "key-a"),)})) @@ -207,6 +204,20 @@ async def test_viewers_can_read_but_not_write_and_non_admins_cannot_read_in_lens assert (write.value.status_code, read.value.status_code) == (403, 403) +@pytest.mark.asyncio +async def test_a_caller_without_a_team_or_key_cannot_write_on_a_teamless_trace() -> None: + feedback: Final = store(t1=(("", "key-a"),)) + await submit_feedback(submission(9, "mine", user="customer-1"), ADMIN, feedback, T0) + + with pytest.raises(HTTPException) as write: + await submit_feedback(submission(1, "overwrite", user="customer-1"), INTERNAL, feedback, T0) + with pytest.raises(HTTPException) as delete: + await delete_feedback(FeedbackDeletion(trace_id="t1", user="customer-1"), INTERNAL, feedback, T0) + + assert (write.value.status_code, delete.value.status_code) == (403, 403) + assert [f.score for f in (await read_feedback(FeedbackTarget(trace_id="t1"), ADMIN, feedback)).feedback] == [9] + + @pytest.mark.asyncio async def test_tenant_comes_from_the_trace_and_author_defaults_to_the_caller() -> None: feedback: Final = store() diff --git a/ui/litellm-dashboard/src/components/lens/traces/detail/feedback/FeedbackPanel.tsx b/ui/litellm-dashboard/src/components/lens/traces/detail/feedback/FeedbackPanel.tsx index f06591acce8..74ea709a731 100644 --- a/ui/litellm-dashboard/src/components/lens/traces/detail/feedback/FeedbackPanel.tsx +++ b/ui/litellm-dashboard/src/components/lens/traces/detail/feedback/FeedbackPanel.tsx @@ -46,7 +46,6 @@ function Entry({ entry }: { entry: Feedback }) { ); } -/** End-user feedback on this run, shown first so a developer reads what the user said before the steps. */ export function FeedbackPanel({ summary, accessToken }: FeedbackPanelProps) { const api = useTracesApi(accessToken); const traceRef = summary.trace_ref ?? ""; diff --git a/ui/litellm-dashboard/src/components/lens/traces/detail/feedback/feedback.ts b/ui/litellm-dashboard/src/components/lens/traces/detail/feedback/feedback.ts index 6a572997657..01a26e788a7 100644 --- a/ui/litellm-dashboard/src/components/lens/traces/detail/feedback/feedback.ts +++ b/ui/litellm-dashboard/src/components/lens/traces/detail/feedback/feedback.ts @@ -6,7 +6,6 @@ export interface FeedbackView { readonly lowest: number; } -/** End-user feedback on one run, newest first, or null when nobody has rated it. */ export function feedbackView(feedback: readonly Feedback[]): FeedbackView | null { if (feedback.length === 0) return null; const scores = feedback.map((entry) => entry.score); diff --git a/ui/litellm-dashboard/src/components/lens/traces/list/AgentTracesSection.integration.test.tsx b/ui/litellm-dashboard/src/components/lens/traces/list/AgentTracesSection.integration.test.tsx index f0ba76367b3..5a32aa4e610 100644 --- a/ui/litellm-dashboard/src/components/lens/traces/list/AgentTracesSection.integration.test.tsx +++ b/ui/litellm-dashboard/src/components/lens/traces/list/AgentTracesSection.integration.test.tsx @@ -81,7 +81,6 @@ const unrated = (traces: TraceKey[]): TraceFeedbackSummary[] => lowest: null, })); -/** Routes the shared POST mock: findings get `findings`, the feedback summary gets `feedback`. */ const stubPost = ( findings: (traces: TraceKey[]) => Promise, feedback: (traces: TraceKey[]) => Promise = async (traces) => unrated(traces), diff --git a/ui/litellm-dashboard/src/components/lens/traces/list/useTraceFeedback.ts b/ui/litellm-dashboard/src/components/lens/traces/list/useTraceFeedback.ts index b35542a0943..fa091c784f0 100644 --- a/ui/litellm-dashboard/src/components/lens/traces/list/useTraceFeedback.ts +++ b/ui/litellm-dashboard/src/components/lens/traces/list/useTraceFeedback.ts @@ -13,7 +13,6 @@ export type TraceFeedbackState = export const traceFeedbackKey = (accessToken: string) => ["traceFeedback", accessToken] as const; -/** A run some end user scored at or below the low-score threshold. */ export const isLowFeedback = (state: TraceFeedbackState | undefined): boolean => state?.status === "ready" && state.summary.count > 0 && (state.summary.lowest ?? Infinity) <= LOW_SCORE;