fix(api): preserve legacy score create alias - #1786
Conversation
|
@claude review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return False | ||
| if canonical_has_create: |
There was a problem hiding this comment.
When a run stops after writing the legacy client but before creating every type alias and package export, the marker-based early return treats that partial output as complete on retry, causing generation to report "No patch needed" while required legacy imports remain missing.
Knowledge Base Used: API Client (langfuse/api/)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/patch_generated_score_compat.py
Line: 183-184
Comment:
**Partial patches bypass recovery**
When a run stops after writing the legacy client but before creating every type alias and package export, the marker-based early return treats that partial output as complete on retry, causing generation to report "No patch needed" while required legacy imports remain missing.
**Knowledge Base Used:** [API Client (langfuse/api/)](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/api-client.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f02e1f05f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise RuntimeError( | ||
| f"Expected exactly one compatibility marker in {path}: {marker!r}; " | ||
| f"found {count}" |
There was a problem hiding this comment.
When a generated marker is missing or duplicated, this fail-closed path inlines an f-string in the raise statement. Build the formatted message in a variable first to comply with the repository's explicit exception-message convention.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
| return CanonicalScoresClient( | ||
| client_wrapper=self._raw_client._client_wrapper | ||
| ).create( |
There was a problem hiding this comment.
After Fern moves the operation, callers using the existing client.legacy.score_v1.with_raw_response.create(...) or client.legacy.with_raw_response.score_v1.create(...) surfaces will receive a RawScoreV1Client that no longer defines create. This postprocessor only restores the decoded-response methods by constructing the canonical high-level client, so both synchronous and asynchronous raw-response users still break; add corresponding raw-client compatibility aliases as well.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline findings, I also checked whether CLIENT_IMPORTS re-adds imports that Fern's regenerated legacy/score_v1/client.py would still have (causing ruff F811 duplicate-import errors) — it doesn't, since Fern removes those same imports along with create. I also checked whether the canonical scores.types modules referenced by the re-export shim (CreateScoreRequest/CreateScoreResponse/CreateScoreSource) actually exist there; they do, unaffected by the endpoint move. Neither is a real issue.
Extended reasoning...
Ruled out two candidate concerns raised by finder agents during this run, beyond the inline findings. Kept to a short informational note per the narrow ruled-out exception; not a full review body.
| def _append_type_exports(path: Path, imports_by_type: dict[str, str]) -> None: | ||
| exports_marker = "# Score-create compatibility aliases (LFE-10397)." | ||
| contents = path.read_text() | ||
| if exports_marker in contents: | ||
| return | ||
|
|
||
| imports = "\n".join( | ||
| f"from {module_name} import {type_name}" | ||
| for type_name, module_name in imports_by_type.items() | ||
| ) | ||
| names = ", ".join(repr(type_name) for type_name in TYPE_NAMES) | ||
| path.write_text( | ||
| f"{contents.rstrip()}\n\n{exports_marker}\n{imports}\n" | ||
| f"__all__ = [*__all__, {names}]\n" |
There was a problem hiding this comment.
🔴 _append_type_exports() writes __all__ = [*__all__, ...] to legacy/score_v1/init.py and legacy/init.py with no fallback if __all__ doesn't already exist, unlike legacy_types_path which has one. Fern emits header-only init.py files (no all) for packages with zero exported types — confirmed live in this repo at langfuse/api/prompt_version/init.py — and since legacy/score_v1/init.py currently exports only the three create-related types, it will very likely regenerate into that same header-only shape once create moves off legacy, causing a NameError at import time.
Extended reasoning...
The bug: _append_type_exports() (scripts/patch_generated_score_compat.py:133-146) unconditionally appends a trailing line __all__ = [*__all__, {names}] to whatever file it's given. This line assumes __all__ is already bound as a module global in that file. It is called on three targets: legacy_types_path, legacy_package_path (legacy/score_v1/__init__.py), and legacy_parent_package_path (legacy/__init__.py). Only the first of these has a fallback — lines 222-226 write a default __all__: list[str] = [] to legacy_types_path, but only in the 'file doesn't exist yet' branch. legacy_package_path and legacy_parent_package_path get no such treatment, regardless of whether they define __all__.
Why this isn't hypothetical: Fern emits a specific 'empty package' shape for subpackages that have a client but export zero types — just the auto-generated header comment and # isort: skip_file, with no __all__, no _dynamic_imports, no __getattr__. I verified this is exactly the current shape of langfuse/api/prompt_version/__init__.py in this repo, even though prompt_version/client.py and raw_client.py still exist:
# This file was auto-generated by Fern from our API Definition.
# isort: skip_file
I also verified the current legacy/score_v1/__init__.py exports only CreateScoreRequest, CreateScoreResponse, CreateScoreSource — i.e. exactly the types tied to create, which the coordinated core Fern PR (referenced in this PR's description as merge step 2) moves off legacy.score_v1 onto scores. Once that happens, score_v1 retains only delete(score_id), which has no request/response types to export. Fern will very likely regenerate legacy/score_v1/__init__.py into the same header-only, no-__all__ shape as prompt_version.
Step-by-step proof of the crash:
- Core Fern PR merges, moving
create's types offlegacy.score_v1. - Fern regenerates
legacy/score_v1/__init__.pyas header-only (no__all__), mirroringprompt_version/__init__.py's current shape. - This postprocessor runs, calls
_append_type_exports(legacy_package_path, {...}), which reads the header-only file, sees no marker, and appends:__all__ = [*__all__, 'CreateScoreRequest', 'CreateScoreResponse', 'CreateScoreSource']. - At Python import time, evaluating
[*__all__, ...]looks up the name__all__in the module's own namespace before the assignment completes — since it was never previously bound, this raisesNameError: name '__all__' is not defined. I reproduced this exact failure in isolation:exec(\"__all__ = [*__all__, 'X']\")raisesNameError: name '__all__' is not defined. - Any access to
client.legacy.score_v1(the parent package's__getattr__dynamic-import machinery lazily imports the submodule) now raises at import time instead of working — breaking the exact backward-compatible entry point this PR exists to preserve.
Why the tests don't catch it: test_patch_generated_score_compat.py's _write_generated_fixture always pre-seeds __all__ = [] in legacy/score_v1/__init__.py, legacy/__init__.py, and the types __init__.py (see the fixture around line 35). This never exercises Fern's real 'header-only, no-__all__' output shape, so the suite passes while the production regeneration path is unguarded.
Fix: apply the same fallback used for legacy_types_path to legacy_package_path and legacy_parent_package_path — write a default __all__: list[str] = [] (or otherwise ensure __all__ is bound) before calling _append_type_exports, or have _append_type_exports itself detect the absence of __all__ in the file's source and initialize it instead of assuming it exists.
| canonical_has_create = ( | ||
| "\n def create(" in canonical_client | ||
| and "\n async def create(" in canonical_client | ||
| ) | ||
| legacy_has_create = ( | ||
| "\n def create(" in legacy_client | ||
| and "\n async def create(" in legacy_client | ||
| ) | ||
|
|
||
| if legacy_has_create: | ||
| if LEGACY_ALIAS_MARKER in legacy_client and canonical_has_create: | ||
| return False | ||
| if canonical_has_create: | ||
| raise RuntimeError( | ||
| "Both canonical and legacy generated score clients define create; " | ||
| "refusing to introduce a third compatibility surface" | ||
| ) | ||
| return False | ||
|
|
||
| if not canonical_has_create: | ||
| raise RuntimeError( | ||
| "Neither generated score client defines both sync and async create methods" | ||
| ) |
There was a problem hiding this comment.
🟡 The postprocessor only patches legacy/score_v1/client.py, adding a delegating create/async create, but never touches legacy/score_v1/raw_client.py. Once the coordinated Fern PR removes create from the raw clients, client.legacy.score_v1.with_raw_response.create(...) will raise AttributeError even though the plain client.legacy.score_v1.create(...) keeps working. This is a real but narrow gap in legacy API preservation, with no test coverage for with_raw_response.
Extended reasoning...
patch_generated_api() in scripts/patch_generated_score_compat.py (lines 172-194) only reads and rewrites legacy/score_v1/client.py — it inserts SYNC_CREATE_METHOD/ASYNC_CREATE_METHOD before the delete methods and appends type re-exports to the package __init__.py files. It never opens or modifies legacy/score_v1/raw_client.py.
Today, raw_client.py defines both RawScoreV1Client.create (line 31) and AsyncRawScoreV1Client.create (line 297), and client.py's with_raw_response property (lines 22 and 176) returns self._raw_client, i.e. the Raw*Client instance. So client.legacy.score_v1.with_raw_response.create(...) currently works and returns the raw HTTP response wrapper — confirmed by grepping the live tree.
Once the coordinated core Fern PR (langfuse/langfuse#15528) moves the create operation off score_v1 in the OpenAPI spec, Fern will remove create from both the public client (ScoreV1Client/AsyncScoreV1Client) and the raw client (RawScoreV1Client/AsyncRawScoreV1Client), since Fern generates both from the same operation. This postprocessor only re-adds the method to the public client via a delegating shim that calls CanonicalScoresClient(client_wrapper=self._raw_client._client_wrapper).create(...) — it bypasses self._raw_client.create entirely rather than patching it. Nothing in the script reconstructs create on the raw client.
Proof walkthrough:
- Core Fern PR merges, regenerating
langfuse/apiwithcreateremoved fromScoreV1Client,AsyncScoreV1Client,RawScoreV1Client, andAsyncRawScoreV1Client. - This postprocessor runs, sees
legacy_has_create == Falseandcanonical_has_create == True, and patcheslegacy/score_v1/client.pyto add a delegatingcreate/async create— but does not touchraw_client.py. - A user calls
client.legacy.score_v1.create(...)→ works, via the new shim delegating toCanonicalScoresClient. - A user calls
client.legacy.score_v1.with_raw_response.create(...)→with_raw_responsereturnsRawScoreV1Client, which no longer hascreate(Fern removed it and the postprocessor never restored it) →AttributeError: 'RawScoreV1Client' object has no attribute 'create'.
This silently breaks part of the legacy API surface that previously worked, and the existing test suite (tests/unit/test_patch_generated_score_compat.py) only exercises the delegating method on the plain client fixture — it never instantiates or checks raw_client.py, so this regression would not be caught by CI even after the core Fern PR lands.
Fix: extend patch_generated_api() to also insert delegating create/async create methods into RawScoreV1Client/AsyncRawScoreV1Client in raw_client.py (likely returning the wrapped HttpResponse[CreateScoreResponse] type to match the raw client's convention), and add a fixture/test asserting with_raw_response.create still works after patching. Given the PR's stated scope is preserving the plain client.legacy.score_v1.create(...) call — which remains intact — and with_raw_response is a niche, low-level accessor with a clear migration path (client.scores.with_raw_response.create), this does not block the primary documented use case, so it is best flagged as a nit for the author to address, either now or in a quick follow-up before the core Fern PR lands.
1 participant
What does this PR do?
Adds the generator postprocessor that preserves the existing
client.legacy.score_v1.create(...)Python API after the canonical Fernoperation moves to
client.scores.create(...).The compatibility method keeps the generated signature and delegates to the
canonical generated scores client with the same client wrapper. It also
re-exports the legacy request/response/source type modules. The postprocessor
is idempotent for its own output and fails closed if Fern produces an
unexpected ownership shape.
Fixes LFE-10397
Core dependency: langfuse/langfuse#15528
(Fern endpoint move and generation workflow invocation).
Merge order:
Type of change
Verification
Targeted tests:
3 passed. The current generated API still owns create underlegacy, so the live-tree postprocessor correctly reports
No patch needed.;the core Fern PR's generated output will exercise the patching branch.
Full unit/e2e suites were not run because this PR only adds a deterministic
generation postprocessor and focused unit coverage; it does not change runtime
SDK source until the coordinated generated API update.
Checklist
code_review.md.coordinated upstream regeneration path.
Greptile Summary
Adds a Fern postprocessor and focused tests to preserve the legacy score-create API after ownership moves to the canonical scores resource.
Confidence Score: 4/5
The partial-patch recovery defect should be fixed before merging because a retried generation can silently retain incomplete compatibility output.
The script writes its marker-bearing client before the remaining files, then uses that marker alone to skip all work on retries, allowing missing type aliases and exports to persist.
Files Needing Attention: scripts/patch_generated_score_compat.py
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Fern[Fern-generated API] --> Patch[Compatibility postprocessor] Patch --> LegacyClient[legacy.score_v1 client alias] Patch --> LegacyTypes[legacy type aliases and exports] LegacyClient --> Scores[canonical scores client]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(api): preserve legacy score create a..." | Re-trigger Greptile
Context used: