fix(api): initialize compatibility exports safely - #1789
Conversation
|
@claude review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| path.write_text( | ||
| f"{contents.rstrip()}\n\n{exports_marker}\n{imports}\n" | ||
| f"__all__ = [*__all__, {names}]\n" | ||
| defines_all = any( |
There was a problem hiding this comment.
When this version runs on output patched by the previous postprocessor, the existing compatibility marker returns before this new detection executes, leaving __all__ = [*__all__, ...] unchanged in modules without an earlier binding and causing import-time NameError. The postprocessor supports reruns, so it needs to repair the old marked output rather than treating every marker as proof that no patch is needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/patch_generated_score_compat.py
Line: 145
Comment:
**Previously patched exports remain broken**
When this version runs on output patched by the previous postprocessor, the existing compatibility marker returns before this new detection executes, leaving `__all__ = [*__all__, ...]` unchanged in modules without an earlier binding and causing import-time `NameError`. The postprocessor supports reruns, so it needs to repair the old marked output rather than treating every marker as proof that no patch is needed.
---
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: e8dfba165f
ℹ️ 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".
| or ( | ||
| isinstance(node, ast.AnnAssign) | ||
| and isinstance(node.target, ast.Name) | ||
| and node.target.id == "__all__" | ||
| ) |
There was a problem hiding this comment.
When a generated initializer contains an annotation without a value, such as __all__: list[str], this AnnAssign branch sets defines_all even though Python has not bound __all__. The postprocessor then emits __all__ = [*__all__, ...], reproducing the import-time NameError this change is intended to prevent. Require node.value is not None before treating an annotated assignment as initialized.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrow, correct fix to the codegen postprocessor's AST-based __all__ handling.
What was reviewed: the ast-based detection of an existing __all__ assignment (both Assign and AnnAssign forms), the fallback path when no __all__ exists yet, and idempotency on a second run. Manually reproduced the fixture and confirmed the generated __init__.py is valid Python and the postprocessor is a no-op on rerun.
Extended reasoning...
Overview
This PR touches only scripts/patch_generated_score_compat.py (a build-time codegen postprocessor, not runtime SDK code) and its unit test. It fixes a bug where the postprocessor unconditionally emitted __all__ = [*__all__, ...] even when the generated module had no prior __all__ definition, which produced invalid/broken generated code (mypy failure, potential NameError). The fix uses ast.parse to detect whether the module already defines __all__ (via Assign or AnnAssign) and branches accordingly.
Security risks
None. This script only runs during local/CI code generation against Fern-generated fixture files; it does not process user input or run in production request paths.
Level of scrutiny
Low-to-moderate is appropriate: this is internal tooling with narrow blast radius (only affects the generated langfuse/api compatibility shims), and it ships with direct test coverage for the exact regression being fixed. I manually re-ran the fixture-based postprocessor logic (network access for uv/pytest was unavailable in this sandbox) and confirmed: (1) the AST detection correctly falls back to __all__ = [...] when no prior assignment exists, (2) the output is syntactically valid, and (3) a second run is a correct no-op via the existing marker-based idempotency check.
Other factors
The diff is small, mechanical, and matches its stated purpose exactly — no scope creep. Test additions directly assert the new behavior (__all__ = [ present, [*__all__ absent) and the fixture was updated to reflect Fern's actual no-__all__ output shape. No outstanding review comments in the timeline beyond an automated @claude review trigger.
1 participant
What does this PR do?
Fixes the score-create compatibility postprocessor when Fern generates a package
__init__.pywithout an existing__all__.The postprocessor previously always emitted:
That is invalid when the generated module has no prior
__all__, causing the generated API PR's mypy failure and a potential import-timeNameError. The postprocessor now inspects the module AST and either extends an existing assignment or creates a fresh export list.CI failure: https://github.com/langfuse/langfuse-python/actions/runs/30450770977/job/90572011905?pr=1788
Generated API PR: #1788
Linear: https://linear.app/clickhouse/issue/LFE-10397
After this PR merges, the core SDK-generation workflow should be rerun from fresh Fern output to replace/update #1788.
Impacted packages
langfusegenerated API compatibility toolingVerification
1 failed, 2 passeduv run --frozen pytest tests/unit/test_patch_generated_score_compat.py—3 passed in 0.12suv run --frozen ruff check .—All checks passed!uv run --frozen ruff format --check .—617 files already formatteduv run --frozen mypy langfuse --no-error-summary— exit code 0Full E2E and live-provider suites were not run because this change only affects the deterministic generation postprocessor and its fixture; it does not change runtime SDK behavior until API generation runs.
Greptile Summary
Safely initializes compatibility exports when generated modules do not already define
__all__.__all__.Confidence Score: 3/5
The previously generated broken compatibility output must be repaired before this change is safe to merge.
The new AST-based initialization works on fresh Fern output, but the existing marker guard skips the logic when rerunning over output produced by the previous postprocessor, preserving the import-time
NameError.Files Needing Attention: scripts/patch_generated_score_compat.py
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(api): initialize compatibility expor..." | Re-trigger Greptile