[fix] Avoid Commitizen circular import in changelog bot#670
[fix] Avoid Commitizen circular import in changelog bot#670pushpitkamboj wants to merge 2 commits into
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThis PR changes get_openwisp_commitizen() to dynamically locate and execute openwisp_utils.releaser.commitizen using runpy.run_path while temporarily injecting stub commitizen modules into sys.modules and restoring sys.modules afterwards. It adds importlib.util and runpy imports and extends tests to verify plugin loading, ImportError when the module is missing, and sys.modules restoration on failure. Sequence Diagram(s)sequenceDiagram
participant Caller
participant Loader as get_openwisp_commitizen
participant Sys as sys.modules
participant Runpy as runpy.run_path
Caller->>Loader: call()
Loader->>Sys: inject stubs (commitizen, commitizen.cz, commitizen.cz.base)
Loader->>Runpy: run_path(plugin_file)
Runpy->>Loader: plugin globals (OpenWispCommitizen)
Loader->>Sys: restore previous sys.modules
Loader->>Caller: return plugin instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
coderabbitai
Bot
added
bug
github_actions
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The PR correctly fixes the circular import issue (#669) by using The solution includes comprehensive test coverage:
Files Reviewed (2 files)
Reviewed by kimi-k2.5-0127 · 300,399 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/actions/bot-changelog-generator/test_generate_changelog.py:
- Around line 535-546: The test uses MagicMock() for the pattern which bypasses
real regex validation; replace the MagicMock with the actual compiled schema by
calling plugin.schema_pattern() (or plugin.schema_pattern().compile() if
schema_pattern returns a string) and pass that to plugin.validate_commit_message
in the test so the real validation path is exercised; update the test assertion
if needed to reflect true success/failure using the produced pattern from
plugin.schema_pattern().
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 78788f2c-5862-46f1-b606-c9f95c1b0100
📒 Files selected for processing (2)
.github/actions/bot-changelog-generator/generate_changelog.py.github/actions/bot-changelog-generator/test_generate_changelog.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-05T09:59:15.097Z
Learnt from: pushpitkamboj
Repo: openwisp/openwisp-utils PR: 584
File: .github/actions/bot-changelog-generator/generate_changelog.py:356-364
Timestamp: 2026-03-05T09:59:15.097Z
Learning: In generate_changelog.py, reviews should confirm that validate_changelog_output acts as an output safety filter: it should require the produced changelog text to begin with a valid tag ([feature], [fix], or [change]) and to include a correctly formed PR reference pattern. It should NOT require that the referenced PR number or URL exactly match the current PR. For review, check: (1) the first non-space token matches one of the allowed tags, (2) the PR reference pattern is present and well-formed (e.g., contains a recognizable PR identifier or URL), and (3) there are no additional hard-coded cross-checks that would make it overly strict. This pattern should apply to this file specifically and guide future changes to similar output-filter functions without assuming cross-repo constraints.
Applied to files:
.github/actions/bot-changelog-generator/generate_changelog.py
| result = plugin.validate_commit_message( | ||
| commit_msg=( | ||
| "[feature] Added new functionality #123\n\n" | ||
| "Adds the new behavior with a user-focused summary.\n\n" | ||
| "Closes #123" | ||
| ), | ||
| pattern=MagicMock(), | ||
| allow_abort=False, | ||
| allowed_prefixes=[], | ||
| max_msg_length=COMMIT_SUBJECT_LIMIT, | ||
| commit_hash="TEST", | ||
| ) |
There was a problem hiding this comment.
Use the real schema pattern in this regression test.
Line 541 passes MagicMock() as pattern, which can make regex checks trivially succeed and leaves the production path untested. Compile plugin.schema_pattern() here so this actually verifies the validation flow the bot uses.
Suggested change
+import re
...
result = plugin.validate_commit_message(
commit_msg=(
"[feature] Added new functionality `#123`\n\n"
"Adds the new behavior with a user-focused summary.\n\n"
"Closes `#123`"
),
- pattern=MagicMock(),
+ pattern=re.compile(plugin.schema_pattern()),
allow_abort=False,
allowed_prefixes=[],
max_msg_length=COMMIT_SUBJECT_LIMIT,
commit_hash="TEST",
)📝 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.
| result = plugin.validate_commit_message( | |
| commit_msg=( | |
| "[feature] Added new functionality #123\n\n" | |
| "Adds the new behavior with a user-focused summary.\n\n" | |
| "Closes #123" | |
| ), | |
| pattern=MagicMock(), | |
| allow_abort=False, | |
| allowed_prefixes=[], | |
| max_msg_length=COMMIT_SUBJECT_LIMIT, | |
| commit_hash="TEST", | |
| ) | |
| result = plugin.validate_commit_message( | |
| commit_msg=( | |
| "[feature] Added new functionality `#123`\n\n" | |
| "Adds the new behavior with a user-focused summary.\n\n" | |
| "Closes `#123`" | |
| ), | |
| pattern=re.compile(plugin.schema_pattern()), | |
| allow_abort=False, | |
| allowed_prefixes=[], | |
| max_msg_length=COMMIT_SUBJECT_LIMIT, | |
| commit_hash="TEST", | |
| ) |
🤖 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 @.github/actions/bot-changelog-generator/test_generate_changelog.py around
lines 535 - 546, The test uses MagicMock() for the pattern which bypasses real
regex validation; replace the MagicMock with the actual compiled schema by
calling plugin.schema_pattern() (or plugin.schema_pattern().compile() if
schema_pattern returns a string) and pass that to plugin.validate_commit_message
in the test so the real validation path is exercised; update the test assertion
if needed to reflect true success/failure using the produced pattern from
plugin.schema_pattern().
|
coverage: 96.46% (-1.0%) from 97.458% — pushpitkamboj:fix/changelog_imports into openwisp:master |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
There was a problem hiding this comment.
Approach is overengineered for the underlying problem
This is the most important issue with the PR. The fix replaces from openwisp_utils.releaser.commitizen import OpenWispCommitizen with a 40-line dance that:
- Builds fake
commitizen,commitizen.cz,commitizen.cz.baseModuleTypestubs. - Defines stub
_BaseCommitizenand_ValidationResultclasses inside the function body. - Temporarily monkey-patches
sys.modulesto point at the stubs. - Calls
runpy.run_path(spec.origin)to execute the plugin file as a fresh module. - Restores
sys.modulesin afinallyblock.
A one-line ordering fix produces the same behavior with no fakes, no runpy, no sys.modules mutation:
import commitizen.cz # force plugin discovery to complete first
from openwisp_utils.releaser.commitizen import OpenWispCommitizenVerified locally: pre-importing commitizen.cz resolves discovery before openwisp_utils.releaser.commitizen is on the import stack, so ep.load() can fully import and resolve the attribute. After this, the returned class is the real OpenWispCommitizen subclass of the real commitizen.cz.base.BaseCommitizen:
Class: OpenWispCommitizen
Module: openwisp_utils.releaser.commitizen
isinstance BaseCommitizen: True
valid: True errors: []
The PR's approach is correct in outcome but introduces fragility (see below) that the simple ordering fix does not. There is no justification in the description, commit message, or code comments for why the heavier path was chosen.
Plugin no longer inherits from the real BaseCommitizen
With runpy.run_path plus the stubbed commitizen.cz.base, the returned class's MRO is:
[OpenWispCommitizen, _BaseCommitizen, object]
instead of [OpenWispCommitizen, BaseCommitizen, object] from the real package. Consequences:
isinstance(plugin, commitizen.cz.base.BaseCommitizen)returnsFalse.- Class-level attributes the real
BaseCommitizenprovides (bump_pattern,default_style_config,commit_parser,template_loader,style, etc.) are missing. Any future call into the plugin that relies on inherited behavior will silently fail or raiseAttributeError. - The class's
__module__is<run_path>rather thanopenwisp_utils.releaser.commitizen, which breaks tracebacks, pickling, and tooling that introspects modules. - The plugin's
validate_commit_messagereturns instances of the stub_ValidationResult(a regular class), not the realValidationResult(aNamedTuple). Tuple unpacking,_asdict(), and structural typing checks againstNamedTuplewould all fail.
These are not theoretical: any future enhancement of the plugin or its callers that assumes the real base classes will quietly diverge from the live runtime behavior.
_CommitizenConfig is now silently invalid
_CommitizenConfig.settings = {} does not satisfy what real BaseCommitizen.__init__ does (self.config.settings.update({"style": ...})). With the runpy/stub approach the stub _BaseCommitizen.__init__ doesn't touch settings, so the mismatch is masked. If anyone later switches back to the simpler import or accidentally uses the real plugin, the config will need to be updated. There's no comment indicating this dependency.
runpy.run_path re-executes the file on every call
get_openwisp_commitizen() is called once per validation in the action's main flow, but tests call it directly. Each call re-reads and re-execs the plugin source file from disk, recreates fake modules, and mutates sys.modules. The previous implementation was a cached from … import after the first call. Caching is straightforward to add (module-level _PLUGIN_CLASS = None), and the omission is not justified.
Test placement is wrong
The three new tests (test_loads_commitizen_plugin_without_circular_import, test_raises_if_commitizen_plugin_module_cannot_be_located, test_restores_sys_modules_if_plugin_loading_fails) exercise get_openwisp_commitizen, not validate_changelog_output. They sit inside class TestValidateChangelogOutput. They should live in a new class TestGetOpenwispCommitizen(unittest.TestCase) for accurate test discovery and reporting.
test_restores_sys_modules_if_plugin_loading_fails is fragile to ordering
The assertion:
self.assertNotIn("commitizen.cz.base", generate_changelog.sys.modules)passes only if no earlier test in the same process imported commitizen.cz.base directly. It currently passes because the suite avoids real commitizen imports until test_loads_commitizen_plugin_without_circular_import (which uses runpy). If a future test or import order change loads the real commitizen.cz.base before this test runs, the assertion will spuriously fail. Either skip this assertion or scope patch.dict over sys.modules more strictly so the test is independent of process-wide state.
Test does not verify cleanup actually had something to clean
In test_restores_sys_modules_if_plugin_loading_fails, runpy.run_path is mocked to raise before the function adds the fake commitizen.cz.base to sys.modules for real (the test mocks runpy after the assignment, so the assignments do happen — but the test never asserts the assignments happened mid-flight). It would be stronger to capture sys.modules['commitizen'] while inside runpy.run_path (e.g., set the side_effect to inspect/mutate then raise) to prove the swap happened and then was reversed.
ImportError message is misleading for namespace packages
if spec is None or not spec.origin:
raise ImportError("Could not locate openwisp_utils.releaser.commitizen")A spec with origin is None happens for namespace packages and similar; the failure mode is "file has no source path", not "module not found". The message conflates two situations. Minor, but worth distinguishing.
Use of runpy is unusual for plugin loading
runpy.run_path is intended for executing scripts, not for loading a class out of a packaged module. The conventional API is importlib.util.module_from_spec plus spec.loader.exec_module, which preserves __name__, __loader__, and lets the module be cached in sys.modules if desired. This is a code-smell that future readers will find disorienting.
Dead/unused parameter pattern (pre-existing, but newly exercised by tests)
The plugin's validate_commit_message signature accepts pattern: re.Pattern[str], but the method body never uses it. The PR's new test passes pattern=MagicMock() which masks the unused-parameter smell. Not introduced by this PR, but the new test surfaces it. Worth either using pattern inside the method or removing it from the signature.
Documentation drift
The docstring """Load the plugin without triggering Commitizen plugin auto-discovery.""" is accurate but doesn't tell the reader why this is needed (the circular import). A one-line # See issue #669 — circular import via commitizen.plugin entry point would prevent a future maintainer from "simplifying" the function back into the broken form. The PR description references the issue, but PR descriptions don't survive into the source tree.
Recommendation
Replace the entire get_openwisp_commitizen() body with the ordering fix:
def get_openwisp_commitizen():
"""Load the OpenWISP Commitizen plugin lazily.
Imports ``commitizen.cz`` first so that the plugin auto-discovery in
``commitizen.cz.__init__`` finishes before ``openwisp_utils.releaser.commitizen``
is added to ``sys.modules``; otherwise the discovery re-imports our module
while it is still mid-initialization (see issue #669).
"""
import commitizen.cz # noqa: F401 -- force plugin discovery first
from openwisp_utils.releaser.commitizen import OpenWispCommitizen
return OpenWispCommitizen(_CommitizenConfig())This:
- Fixes the bug.
- Returns the real
OpenWispCommitizen(correct MRO, correct__module__, realValidationResult). - Eliminates
runpy, the fake modules, thesys.modulesmutation, the newimportlib.utilandrunpyimports, and the stub classes. - Removes the need for two of the three new tests; the remaining test (
test_loads_commitizen_plugin_without_circular_import) becomes a clean regression test against the real circular-import scenario.
If the project has reasons to avoid populating the commitizen plugin registry at runtime (e.g., to keep startup time down), the simpler alternative is still preferable but a comment justifying the heavier approach should be added — none currently exists.
|
Closed in favour of #671. |
3 participants
Checklist
Reference to Existing Issue
Closes #669
Description of Changes
get_openwisp_commitizen()in.github/actions/bot-changelog-generator/generate_changelog.pyto avoid normal importing ofopenwisp_utils.releaser.commitizen.commitizen.pluginentry point.