mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-09 03:18:44 +00:00
fix(lens): block teamless feedback writes and fix retention test (#45266)
* fix(lens): reject feedback writes from callers with no team or key * test(lens): teamless callers cannot overwrite feedback * test(lens): expect lens_feedback retention during schema setup * refactor(lens): flatten feedback summaries without a stacked comprehension * chore(lens-ui): drop routine comment on the feedback panel * chore(lens-ui): drop routine comment on the feedback view * chore(lens-ui): drop routine comment on the low feedback check * chore(lens-ui): drop routine comment on the post stub
This commit is contained in:
parent
d8c0e2c715
commit
f824d11a22
8 changed files with 36 additions and 25 deletions
|
|
@ -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 "")
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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, ...]:
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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 ?? "";
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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<unknown>,
|
||||
feedback: (traces: TraceKey[]) => Promise<TraceFeedbackSummary[]> = async (traces) => unrated(traces),
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue