Repository navigation
feat(api): update API spec from langfuse/langfuse d9b656b - #2024
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.
| @@ -100,11 +89,3 @@ def dashboards(self): | |||
| client_wrapper=self._client_wrapper | |||
| ) | |||
| return self._dashboards | |||
There was a problem hiding this comment.
馃敶 Code that calls client.api.unstable.skills breaks with AttributeError after upgrading; on the base branch that call works. The skills properties were removed from UnstableClient and AsyncUnstableClient, and the skills re-exports were dropped from unstable/__init__.py, with no deprecated alias. Fix: keep a deprecated unstable.skills property (sync and async) and re-export that delegates to the new top-level SkillsClient, or ship this as an announced breaking change. Every request path also moved from api/public/unstable/skills to api/public/skills.
Why this was flagged
A user who adopted the Skills API calls langfuse.api.unstable.skills.get(...) or imports from langfuse.api.unstable import SkillVersionFileInput. After this diff, UnstableClient (langfuse/api/unstable/client.py) has no skills attribute, so the call raises AttributeError. The import raises too, because unstable/__init__.py __getattr__ finds no SkillVersionFileInput key in _dynamic_imports. The base branch serves both through the lazy skills property and the .skills lazy-import entries. Nothing in the diff adds a fallback or deprecation shim. A related condition: langfuse/api/skills/raw_client.py now sends every request to api/public/skills... instead of api/public/unstable/skills.... So a self-hosted Langfuse server older than spec d9b656b, which may only serve the unstable route, would answer the new top-level client with 404 NotFoundError.
Verification: Trigger: an existing user upgrades from v5.0.x, which ships unstable.skills (pyproject.toml version = "5.0.2"), and their code still uses the old path. The diff to langfuse/api/unstable/client.py deletes the skills property from both UnstableClient and AsyncUnstableClient. So langfuse.api.unstable.skills.get(...) now raises AttributeError. No deprecated alias or re-export is left behind.
| HttpResponse[SkillMetaListResponse] | ||
| """ | ||
| _response = self._client_wrapper.httpx_client.request( | ||
| "api/public/unstable/skills", | ||
| "api/public/skills", |
There was a problem hiding this comment.
馃煛 (optional) Users on a self-hosted Langfuse server that only serves the old unstable route will get 404s from every Skills call after upgrading this SDK. All sync and async raw_client methods now call api/public/skills... instead of api/public/unstable/skills..., with no fallback. Fix: confirm the server still serves api/public/unstable/skills, or document the minimum server version. Alternatively, on a 404 from the new path, retry once on the unstable path. Applies to all 7 sync and 7 async endpoints.
Why this was flagged
The trigger is a self-hosted Langfuse deployment older than langfuse/langfuse d9b656b, where Skills lives only under /api/public/unstable/skills. This checkout cannot enumerate those deployments. RawSkillsClient.list (langfuse/api/skills/raw_client.py:75-78) and the create, get, update, get_file_contents, set_labels and delete_version methods now request api/public/skills, api/public/skills/{skill_name}, api/public/skills/files/content and api/public/skills/{skill_name}/versions/{skill_version}. AsyncRawSkillsClient makes the same calls (around lines 889-1572). On the base branch these requests go to api/public/unstable/skills... and succeed against such servers. After the merge, the 404 is raised as a NotFoundError/ApiError. The SDK has no version negotiation or fallback that would catch this, so the Skills feature stops working until the server is upgraded.
Verification: At base 685dc88, every Skills call in langfuse/api/unstable/skills/raw_client.py went to api/public/unstable/skills.... In HEAD's langfuse/api/skills/raw_client.py, all 14 calls now go to api/public/skills..., including RawSkillsClient.list at about line 78. No code in the diff falls back to the unstable path, so a server that has only the old route returns 404 for every Skills operation.
| @property | ||
| def skills(self): | ||
| if self._skills is None: | ||
| from .skills.client import SkillsClient # noqa: E402 |
There was a problem hiding this comment.
Imports break the repository rule
The new skills properties import SkillsClient and AsyncSkillsClient inside their methods. The repository requires imports at the top of the module. Move both imports to module scope before merging.
Rule Used: Move imports to the top of the module instead of placing them within functions or methods. (source)
Learned From
langfuse/langfuse-python#1387
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/api/client.py
Line: 360
Comment:
**Imports break the repository rule**
The new `skills` properties import `SkillsClient` and `AsyncSkillsClient` inside their methods. The repository requires imports at the top of the module. Move both imports to module scope before merging.
**Rule Used:** Move imports to the top of the module instead of placing them within functions or methods. ([source](https://app.greptile.com/personal-org-4986/-/custom-context?memory=c960fc07-9928-409f-a18b-a780cbdded12))
**Learned From**
[langfuse/langfuse-python#1387](https://github.com/langfuse/langfuse-python/pull/1387)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Generated from langfuse/langfuse
mainat d9b656b.The import-placement requirement must be satisfied before merging; no behavioral failure was established.
Summary
Updates the generated API client from the upstream API definition.
The new
skillsproperties need to follow the repository's import-placement rule.Reviews (1) 路 Last reviewed commit: "feat(api): update API spec from langfuse..." 路 Reviewed by Greptile