fix(python): derive scenario.__version__ from package metadata#800
fix(python): derive scenario.__version__ from package metadata#800langwatch-agent wants to merge 1 commit into
Conversation
scenario.__version__ has read "0.1.0" since the first release while the package shipped 0.7.31. Nothing bumps it: release-please updates pyproject.toml, the changelog and the manifest, and .release-please-config.json declares no extra-files, so the literal never moved across 76 releases. Alias the constant the tracing layer already resolves through importlib.metadata, giving the version one source of truth in pyproject.toml. Setting the literal to the current number instead would read correctly today and drift again at the next release, which is how it reached 0.1.0 vs 0.7.31. The repo already settled this for the same value on the tracing side: issue #744's AC is that the version is read from package metadata, not hardcoded. The public constant and the scenario.sdk.version trace attribute now cannot disagree about which SDK produced a run, and both fall back to "unknown" when metadata is unavailable. Tests cover both value assertions plus a guard that rejects a literal reintroduced here, since the value assertions alone would pass right up until the next release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
langwatch-agent
added
the
tech-debt-fixer
|
Automated low-risk assessment This PR was evaluated against the repository's Low-Risk Pull Requests procedure and does not qualify as low risk.
This PR requires a manual review before merging. |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Walkthrough
ChangesPackage version synchronization
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/tests/test_package_version.py`:
- Line 38: Add an assertion that scenario.__file__ is not None before passing it
to pathlib.Path in the test setup, narrowing the type while validating the
module file path; keep the existing read_text flow unchanged.
- Around line 41-47: Update the AST traversal in the version-assignment check to
recognize both ast.Assign and ast.AnnAssign nodes, while preserving the existing
__version__ target detection and hardcoded-literal validation for either
assignment form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d5e0b27-bcf1-4083-83ec-41ccbafdeeb0
📒 Files selected for processing (2)
python/scenario/__init__.pypython/tests/test_package_version.py
| import ast | ||
| import pathlib | ||
|
|
||
| source = pathlib.Path(scenario.__file__).read_text() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that __file__ is not None for type safety.
The __file__ attribute of a module is typed as str | None. Passing it directly to pathlib.Path can cause a type-checking error. Add an assertion to narrow the type and ensure the file path is valid.
🛠️ Proposed fix
- source = pathlib.Path(scenario.__file__).read_text()
+ assert scenario.__file__ is not None, "scenario module must have a __file__ attribute"
+ source = pathlib.Path(scenario.__file__).read_text()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| source = pathlib.Path(scenario.__file__).read_text() | |
| assert scenario.__file__ is not None, "scenario module must have a __file__ attribute" | |
| source = pathlib.Path(scenario.__file__).read_text() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/tests/test_package_version.py` at line 38, Add an assertion that
scenario.__file__ is not None before passing it to pathlib.Path in the test
setup, narrowing the type while validating the module file path; keep the
existing read_text flow unchanged.
| for node in tree.body: | ||
| if not isinstance(node, ast.Assign): | ||
| continue | ||
| if not any( | ||
| isinstance(t, ast.Name) and t.id == "__version__" for t in node.targets | ||
| ): | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle annotated assignments to prevent test evasion.
The current check only looks for ast.Assign nodes. If a developer later introduces __version__ with a type annotation (e.g., __version__: str = "1.0.0"), it will be parsed as an ast.AnnAssign node. The test will silently skip it, allowing a hardcoded literal to be reintroduced.
Update the condition to handle both assignment types.
🛠️ Proposed fix
for node in tree.body:
- if not isinstance(node, ast.Assign):
- continue
- if not any(
- isinstance(t, ast.Name) and t.id == "__version__" for t in node.targets
- ):
- continue
+ is_version_assign = False
+ if isinstance(node, ast.Assign):
+ is_version_assign = any(
+ isinstance(t, ast.Name) and t.id == "__version__" for t in node.targets
+ )
+ elif isinstance(node, ast.AnnAssign):
+ is_version_assign = isinstance(node.target, ast.Name) and node.target.id == "__version__"
+
+ if not is_version_assign:
+ continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for node in tree.body: | |
| if not isinstance(node, ast.Assign): | |
| continue | |
| if not any( | |
| isinstance(t, ast.Name) and t.id == "__version__" for t in node.targets | |
| ): | |
| continue | |
| for node in tree.body: | |
| is_version_assign = False | |
| if isinstance(node, ast.Assign): | |
| is_version_assign = any( | |
| isinstance(t, ast.Name) and t.id == "__version__" for t in node.targets | |
| ) | |
| elif isinstance(node, ast.AnnAssign): | |
| is_version_assign = isinstance(node.target, ast.Name) and node.target.id == "__version__" | |
| if not is_version_assign: | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/tests/test_package_version.py` around lines 41 - 47, Update the AST
traversal in the version-assignment check to recognize both ast.Assign and
ast.AnnAssign nodes, while preserving the existing __version__ target detection
and hardcoded-literal validation for either assignment form.
1 participant
Context
Handling the CodeRabbit comment on the release PR #658, which flagged that
scenario.__version__is"0.1.0"while the package ships0.7.32.The drift is real (
__version__ = "0.1.0"atpython/scenario/__init__.py:263vs0.7.31on main), but the suggested fix was to set the literal to the current version, and that is what caused the bug. Nothing bumps this constant: release-please updatespyproject.toml, the changelog and the manifest, and.release-please-config.jsondeclares noextra-files. That is why a literal survived 76 releases untouched. Setting it to0.7.32reads correctly today and is wrong again the moment 0.7.33 ships.This PR is opened against
mainrather than pushed to #658, because #658 is a release-please branch that gets regenerated on every run (a manual commit there would be clobbered), and a release PR should carry release metadata only.Change
__version__now aliases the constant the tracing layer already resolves throughimportlib.metadata, so the version has a single source of truth: theversioninpyproject.toml.The repo already settled this exact question for the same value on the tracing side.
_tracing/sdk_metadata.pyreads package metadata "rather than a hardcoded literal", and issue #744's AC is explicit: "Version is NOT hardcoded — read from package metadata."__version__was simply never brought in line. A side effect worth having: the public constant and thescenario.sdk.versiontrace attribute can no longer disagree about which SDK produced a run.No new import cost or cycle:
__init__.pyalready imports_tracingat line 102.Evidence
Verified against the real installed distribution:
tests/test_package_version.py)tests/test_sdk_version_attributes.py)__version__ = 0.7.31Mutation-tested, and this is the part that makes the case. Rather than only asserting the tests pass, I ran them against both alternatives:
__init__.pystate"0.1.0"(main today)"0.7.31"(the suggested fix)_SCENARIO_SDK_VERSION(this PR)That middle row is the whole argument: a hardcoded value is indistinguishable from correct until the next release. So the third test asserts the AST node for
__version__is not a constant, which is what actually prevents the regression from returning.Not verified: the full
pytest tests/sweep timed out locally on tests that reach the network; I did not chase it, since this change touches only a module constant and the import smoke plus both version suites are green. CI is the check that matters here.Advisory note
Advisory PR from the tech-debt-fixer bot, for human review, not to be merged by the bot.