Repository navigation
feat(api): update API spec from langfuse/langfuse 2cb8041 - #2095
langfuse-bot wants to merge 1 commit into
Conversation
|
@claude review |
There was a problem hiding this comment.
Claude Code Review
No review was started: this request came from a bot account. Manual reviews can only be requested by someone with write access to this repository. Ask a maintainer to comment @claude review, or have your automation post the comment from a user account with write access.
Tip: disable this comment in your organization's Code Review settings.
| rule_ids: typing_extensions.Annotated[ | ||
| typing.List[str], FieldMetadata(alias="ruleIds") | ||
| ] = pydantic.Field() | ||
| """ | ||
| The rules that run the facet. | ||
| """ |
There was a problem hiding this comment.
🔴 Callers on a Langfuse server that does not yet send ruleIds now get a pydantic ValidationError from list_facets and get_facet. On the base branch those facets parsed fine. TopicFacet.rule_ids is declared required (pydantic.Field() with no default). parse_obj_as validates strictly, and the raw client only catches JSONDecodeError. Fix: make rule_ids optional (typing.Optional[typing.List[str]] = None) in the API definition, the same way the sibling Topic.rule_id and TopicFacetCounts.rule_id were added, so responses without ruleIds still parse.
Why this was flagged
The trigger is a server deployment, such as a self-hosted Langfuse older than spec 2cb8041, whose topic-facets responses have no ruleIds key. I could not enumerate which server versions callers run. client.unstable.topics.list_facets() / get_facet() in raw_client.py call parse_obj_as(type_=TopicFacetList/TopicFacet, ...). On pydantic v2 that is TypeAdapter.validate_python (core/pydantic_utilities.py:58-59), which raises on the missing required field rule_ids (topic_facet.py:22-24). The raw client's try block only handles JSONDecodeError, so the ValidationError reaches the caller. On the base branch TopicFacet had no rule_ids and the same response parsed. The model's extra="allow" config tolerates unknown keys but does not help with a missing required key. The other fields added in this change (Topic.rule_id, TopicFacetCounts.rule_id, ObservationsV2Meta.comment) all default to None.
Verification: The diff adds rule_ids with pydantic.Field() and no default at langfuse/api/unstable/topics/types/topic_facet.py:22-24, which makes it required. parse_obj_as in core/pydantic_utilities.py:53-60 raises a ValidationError when a required field is missing. The try blocks catch only JSONDecodeError, so payloads without ruleIds that parsed on the base now fail.
| comment: typing.Optional[str] = pydantic.Field(default=None) | ||
| """ | ||
| Present when an empty page has a cursor; continue requesting pages until the cursor is absent. | ||
| """ |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Batch evaluations over observations can stop partway and still report completed=True with no resume token. The cause is the server's new empty pages that carry a cursor. This spec says a page can be empty while meta.cursor is set, and paging must continue until the cursor is absent. The SDK's own loop at langfuse/batch_evaluation.py:1057 stops on cursor is None or not items, so it treats an empty page as the end. Fix: end the loop only when next_cursor is None, while keeping the max_items stop and the per-page dedupe working. [also at: langfuse/api/observations/client.py:1 - pre-existing: Batch evaluations over observations can stop early and report completion, skipping the remaining observations.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
The trigger is a server that sends the contract added at observations_v2meta.py:14-17 and client.py docstrings: data is empty, meta.cursor is set, and comment is present. The spec calls this normal for requests whose bounded scan finds no matches in a slice. The SDK caller run_batched_evaluation on observations fetches pages through _fetch_batch_with_retry (batch_evaluation.py:1115-1123) and at batch_evaluation.py:1057 runs if cursor is None or not items: has_more = False; break. After the break it returns completed=True and has_more_items=False (batch_evaluation.py:1081-1085), and the resume token holds a live cursor that nothing asks the user to follow. Observations after the empty slice are never evaluated and no error is shown. This PR updates the generated client to document the contract but leaves the only in-repo consumer unchanged, so nothing guards against the early stop.
Verification: pre-existing. The trigger is a Langfuse server that returns a page with empty data while meta.cursor is still set. In langfuse/batch_evaluation.py the loop runs if cursor is None or not items: has_more = False; break (lines 1056-1059), so an empty page with a non-null cursor ends the run. The caller gets a "complete" result with no resume token. The base has the same loop.
Generated from langfuse/langfuse
mainat 2cb8041.

Confidence Score: 5/5
The PR appears safe to merge; no actionable issues were found in the changed code.Summary
Updates the generated API client from the upstream API definition.
Reviews (1) · Last reviewed commit: "feat(api): update API spec from langfuse..." · Reviewed by Greptile