[chores:fix] Fine tuned commitizen rules #110 #593
Conversation
- Avoided overriding the main entrypoint, rely on cz_openwisp plugin - Added openwisp-commit shortcut command + usage docs, this command allows starting the guided commit procedure, facilitates amending, and provides a shortcut to check commit messages. - Modified existing openwisp-qa-check script to automatically use the new shortcut - The changes above allow using the new commitizen rules without the need of adding a new config file in every openwisp repository - Fixed guided commit rules: - Don't complain if issue is not referenced at all (is allowed) - If issue is referenced, ensure it matches title and body Related to #110
WalkthroughAdds a new Bash wrapper script Sequence Diagram(s)sequenceDiagram
participant User as User
participant Wrapper as openwisp-commit
participant CZ as cz (Commitizen CLI)
participant Validator as OpenWispCommitizen
participant Git as Git
User->>Wrapper: run openwisp-commit [--check | --amend | --rev-range]
alt --check
Wrapper->>CZ: cz -n cz_openwisp check --rev-range <range>
CZ->>Validator: validate(commit(s))
Validator-->>CZ: success / error
CZ-->>Wrapper: exit code
else --amend
Wrapper->>Git: git reset --soft HEAD~1
Wrapper->>CZ: cz -n cz_openwisp commit
CZ->>Validator: prompt & format message
Validator-->>CZ: formatted message (may append "Related to `#N`")
CZ->>Git: create commit
Wrapper->>CZ: cz -n cz_openwisp check --rev-range <range>
CZ->>Validator: validate(new commit)
Validator-->>CZ: result
else commit
Wrapper->>CZ: cz -n cz_openwisp commit
CZ->>Validator: prompt & format message
Validator-->>CZ: formatted message
CZ->>Git: create commit
Wrapper->>CZ: cz -n cz_openwisp check --rev-range <range>
CZ->>Validator: validate(commit)
end
Wrapper-->>User: exit status & messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2026-02-10T20:38:21.134ZApplied to files:
📚 Learning: 2026-02-06T20:46:32.980ZApplied to files:
🧬 Code graph analysis (1)openwisp_utils/releaser/tests/test_commitizen_rules.py (1)
🪛 Ruff (0.15.0)openwisp_utils/releaser/tests/test_commitizen_rules.py[error] 6-6: Starting a process with a partial executable path (S607) [warning] 96-96: Unpacked variable Prefix it with an underscore or any other dummy variable pattern (RUF059) [warning] 96-96: Unpacked variable Prefix it with an underscore or any other dummy variable pattern (RUF059) [warning] 103-103: Unpacked variable Prefix it with an underscore or any other dummy variable pattern (RUF059) [warning] 103-103: Unpacked variable Prefix it with an underscore or any other dummy variable pattern (RUF059) [warning] 110-110: Unpacked variable Prefix it with an underscore or any other dummy variable pattern (RUF059) [warning] 110-110: Unpacked variable Prefix it with an underscore or any other dummy variable pattern (RUF059) [error] 150-150: Starting a process with a partial executable path (S607) ⏰ 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 (8)
✏️ Tip: You can disable this entire section by setting 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
enhancement
releaser
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp-qa-check (1)
149-157:⚠️ Potential issue | 🟠 Major
--messageflag is now silently ignored, breaking existing workflows.The
runcheckcommitfunction delegates toopenwisp-commit --check, which always validatesHEAD^!and does not accept arbitrary commit message input. However,openwisp-qa-checkstill accepts and documents a--messageflag (line 29 in help text, line 229-231 in argument parsing), and still sets theCOMMIT_MESSAGEvariable (line 188 and 231). This variable is never used, and the flag has no effect.Users relying on
--messageto validate arbitrary commit messages (e.g., in CI pipelines that test specific commit messages) will see no validation occur. The documentation atdocs/developer/qa-checks.rstalso shows example usage with--message.Either forward the message capability to
openwisp-commit(if supported), or remove the dead--messagehandling, update the help text (line 29), and update the documentation.
🤖 Fix all issues with AI agents
In `@openwisp_utils/releaser/tests/test_commitizen_rules.py`:
- Around line 28-32: The test test_valid_commit_without_issue_single_line_body
duplicates test_valid_commit_without_issue: update
test_valid_commit_without_issue_single_line_body to use a single-line body
message (e.g., set message to "[chores] Good commit message\n\nSome
explanation." -> change to "[chores] Good commit message\n\nSome explanation."
actually should be a one-line body like "[chores] Good commit
message\n\nExplanation." or simply "[chores] Good commit message\n\nSome
explanation" to reflect a single-line body) or delete the duplicate test; ensure
you modify the message variable inside
test_valid_commit_without_issue_single_line_body (the input to run_cz_check) to
a true single-line body or remove the test to avoid redundancy, keeping
test_valid_commit_without_issue and run_cz_check intact.
In `@openwisp-commit`:
- Line 37: The printf call currently uses the literal format string "%%s" which
prints "%s" instead of substituting the argument; update the printf invocation
to use "%s" (single percent) so the "$1" parameter is consumed (i.e., change
printf "%%s\n" "$1" to printf "%s\n" "$1"). Ensure the surrounding code still
passes the correct argument variable to printf.
- Around line 20-44: The --rev-range case currently shifts and assigns REV_RANGE
without validating the next token; update the --rev-range branch (the case
handling that sets REV_RANGE) to first check that a following argument exists
and is not another option (i.e., $1 is non-empty and does not begin with '-')
before assigning; if the value is missing, print an error and call show_help and
exit with non-zero status so REV_RANGE is never silently set to empty.
- Around line 46-56: CZ_CHECK is hardcoded to use HEAD^! instead of the parsed
REV_RANGE and the command variables are defined before argument parsing, so
--rev-range has no effect; move the definitions of CZ_CMD, CZ_COMMIT, CZ_CHECK
and the run_commit_and_check function to after the argument-parsing loop (after
REV_RANGE and CHECK_MODE are set) and change CZ_CHECK to use "$REV_RANGE" (e.g.,
CZ_CHECK="$CZ_CMD check --rev-range \"$REV_RANGE\"") so both explicit checks and
run_commit_and_check pick up the overridden value.
🧹 Nitpick comments (4)
openwisp_utils/releaser/commitizen.py (2)
215-239: Theschema_patternregex cannot enforce issue symmetry — relies onvalidate_commit_message.The
with_issue_patternuses named groupstitle_issuesandbody_issues, but regex named groups don't enforce equality between captures. This means the pattern will match even when the title and body reference different issue numbers. The actual symmetry enforcement happens invalidate_commit_message(lines 158-191), so this works correctly in practice, but the named groups in the schema pattern could be misleading to future readers.Consider adding a brief comment noting that the pattern is intentionally permissive and that
validate_commit_messagehandles the stricter symmetry check.
5-5: Remove unused regex_TITLE_ISSUE_RE.
_TITLE_ISSUE_REon line 5 is defined but never referenced in the codebase. The actual regexes used are_TITLE_ISSUE_EXTRACT_RE(line 6) and_BODY_ISSUE_RE(lines 7-10).♻️ Remove unused regex
-_TITLE_ISSUE_RE = re.compile(r" #[^#\s]+$") _TITLE_ISSUE_EXTRACT_RE = re.compile(r" #(\d+)")openwisp_utils/releaser/tests/test_commitizen_rules.py (2)
83-100: Optional: prefix unusedout/errwith underscores to satisfy linter.Ruff RUF059 flags unused
outanderrvariables on lines 85, 92, and 99. These tests only check the return code without inspecting output.♻️ Example fix for one test
def test_empty_commit_message(): """Invalid: empty commit message.""" - code, out, err = run_cz_check("") + code, _out, _err = run_cz_check("") assert code != 0Apply the same pattern to
test_invalid_prefix_formatandtest_title_not_capitalized.
129-136: Ambiguous test intent: comments contradict the test name.The test is named
test_issue_not_at_end_of_title(suggesting invalid behavior) but the comments on lines 131 and 135 say "This is allowed now" and assertcode == 0. The test name gives the impression this should fail. Consider renaming to clarify the expected behavior, e.g.,test_issue_in_middle_of_title_is_valid.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/developer/qa-checks.rstopenwisp-commitopenwisp-qa-checkopenwisp_utils/releaser/commitizen.pyopenwisp_utils/releaser/tests/test_commitizen_rules.pysetup.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 560
File: openwisp_utils/commitizen/openwisp.py:9-25
Timestamp: 2026-02-06T20:46:32.980Z
Learning: In openwisp-utils, the commit message prefix "chores" (plural) is intentionally used instead of "chore" (singular) in the Commitizen ALLOWED_PREFIXES configuration.
📚 Learning: 2026-02-06T20:46:32.980Z
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 560
File: openwisp_utils/commitizen/openwisp.py:9-25
Timestamp: 2026-02-06T20:46:32.980Z
Learning: In openwisp-utils, the commit message prefix "chores" (plural) is intentionally used instead of "chore" (singular) in the Commitizen ALLOWED_PREFIXES configuration.
Applied to files:
openwisp-qa-checkopenwisp-commitdocs/developer/qa-checks.rstsetup.py
🧬 Code graph analysis (1)
openwisp_utils/releaser/tests/test_commitizen_rules.py (1)
openwisp_utils/releaser/commitizen.py (1)
message(84-98)
🪛 Ruff (0.15.0)
openwisp_utils/releaser/tests/test_commitizen_rules.py
[error] 6-6: Starting a process with a partial executable path
(S607)
[warning] 85-85: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 85-85: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 92-92: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 92-92: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 99-99: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 99-99: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[error] 142-142: Starting a process with a partial executable path
(S607)
⏰ 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)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.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~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
🔇 Additional comments (5)
setup.py (1)
23-37: LGTM!The plugin rename from
cz_conventional_commitstocz_openwispavoids overriding the default commitizen plugin, and the rationale is well-documented in the comment. Addingopenwisp-committo the scripts list properly exposes the new wrapper.docs/developer/qa-checks.rst (1)
130-175: LGTM!Documentation is clear, well-structured, and accurately reflects the new
openwisp-commitworkflow — including the--amend,--checkflags, auto-append behavior for issue references, and advanced directczusage.openwisp_utils/releaser/commitizen.py (2)
84-98: Verify body formatting when auto-appending "Related to".In
message(), whenmissing_issuesis non-empty, a\nis appended first, then\nRelated to #<issue>for each issue. If the user's body already ends without a trailing newline, the result is:<body>\n\nRelated to `#123`That looks correct (blank line separator). But if the body already ends with a newline, you'd get:
<body>\n\n\nRelated to `#123`This is mitigated by
.strip()on line 98 which trims trailing whitespace from the final output. However,body.strip()would also strip leading whitespace from the body if it had any. This is minor sinceanswers["how"].strip()on line 88 already strips the raw input.
157-191: Issue symmetry validation logic is sound.The three-way branch (title-only, body-only, mismatch) with clear error messages is well structured. One edge case to note:
_TITLE_ISSUE_EXTRACT_REmatches#<digits>anywhere in the title (not just at the end), so a commit like[fix] Handle#123edge casewould extract123from the title. This appears intentional per the test on line 129 of the test file.openwisp_utils/releaser/tests/test_commitizen_rules.py (1)
14-18: Good test coverage for symmetric issue references.The suite covers the key scenarios: issues in both title and body (with various keywords), no issues at all, multiple issues, and auto-appended "Related to". The asymmetric and mismatch error cases are also well-covered.
Also applies to: 42-50
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
openwisp-commit
Outdated
| exit 0 | ||
| ;; | ||
| *) | ||
| printf "Unknown argument: %%s\n" "$1" |
There was a problem hiding this comment.
Bug: %%s in printf produces literal %s instead of substituting the argument.
In bash printf, %% is the escape sequence for a literal % character. So %%s outputs the string %s and $1 is never consumed. This should be %s.
🐛 Proposed fix
- printf "Unknown argument: %%s\n" "$1"
+ printf "Unknown argument: %s\n" "$1"📝 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.
| printf "Unknown argument: %%s\n" "$1" | |
| printf "Unknown argument: %s\n" "$1" |
🤖 Prompt for AI Agents
In `@openwisp-commit` at line 37, The printf call currently uses the literal
format string "%%s" which prints "%s" instead of substituting the argument;
update the printf invocation to use "%s" (single percent) so the "$1" parameter
is consumed (i.e., change printf "%%s\n" "$1" to printf "%s\n" "$1"). Ensure the
surrounding code still passes the correct argument variable to printf.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openwisp_utils/releaser/tests/test_commitizen_rules.py (1)
129-136: Contradictory comments in test.Lines 131–132 say "This is allowed now since we extract all issues from title" and then "But let's test that the behavior is correct"—the hedging comments read like notes left during development. Consider simplifying:
Suggested cleanup
def test_issue_not_at_end_of_title(): - """Invalid: issue reference must be at end of title.""" - # This is allowed now since we extract all issues from title - # But let's test that the behavior is correct + """Valid: issue in the middle of title is still detected symmetrically.""" message = "[qa] Good `#1` commit message\n\n" "Body\n\n" "Fixes `#1`" code, out, err = run_cz_check(message) - # This should work since `#1` is in both title and body assert code == 0, f"Issue in middle of title should still work: {out + err}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp-commitopenwisp_utils/releaser/commitizen.pyopenwisp_utils/releaser/tests/test_commitizen_rules.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp-commit
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 560
File: openwisp_utils/commitizen/openwisp.py:9-25
Timestamp: 2026-02-06T20:46:32.980Z
Learning: In openwisp-utils, the commit message prefix "chores" (plural) is intentionally used instead of "chore" (singular) in the Commitizen ALLOWED_PREFIXES configuration.
🧬 Code graph analysis (1)
openwisp_utils/releaser/tests/test_commitizen_rules.py (1)
openwisp_utils/releaser/commitizen.py (1)
message(83-97)
🪛 Ruff (0.15.0)
openwisp_utils/releaser/tests/test_commitizen_rules.py
[error] 6-6: Starting a process with a partial executable path
(S607)
[warning] 85-85: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 85-85: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 92-92: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 92-92: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 99-99: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 99-99: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[error] 142-142: Starting a process with a partial executable path
(S607)
🔇 Additional comments (8)
openwisp_utils/releaser/commitizen.py (4)
83-97: Auto-append logic inmessage()looks correct.The set-difference approach for
missing_issuesand thesorted()call ensure deterministic ordering of auto-appendedRelated tolines. The final.strip()on the body properly cleans up whitespace.
99-115: Helper methods_extract_title_issues/_extract_body_issuesare clean and correct.They properly split on
\n, isolate the title vs. body, and delegate to the compiled regexes. Minor note:lines[0]is safe after theif not linesguard, and thelen(lines) < 2check for body is appropriate.
117-201: Validation logic is well-structured and produces clear error messages.The three-way branch at lines 161–190 (title-only issues, body-only issues, mismatched issues) gives actionable feedback to the user. The merge-commit early-return and prefix check are solid.
231-244:no_issue_patterndoesn't consume the body, so the schema regex may never match multi-line commits without issues.The
no_issue_patternuses[^\n]*$(end of first line) with a negative lookahead, but never consumes the body text. Combined with the outer\Zanchor, the full regex won't match a commit like"[chores] Title\n\nBody text."because the cursor stops at the end of the first line. This is currently masked becausevalidate_commit_messageis the actual validation path.If
schema_patternis purely informational / a fallback behindvalidate_commit_message, this is fine—but adding a comment would clarify that the regex is intentionally incomplete.Potential fix to also consume the body
no_issue_pattern = ( - r"\[[a-z0-9!/:-]+\] [A-Z][^\n]*" - r"$(?!\n\n.*(?:Close|Closes|Closed|Fix|Fixes|Fixed" - r"|Resolve|Resolves|Resolved|Related to) #\d+)" + r"\[[a-z0-9!/:-]+\] [A-Z][^\n]*" + r"(?:\n\n(?:(?!(?:Close|Closes|Closed|Fix|Fixes|Fixed" + r"|Resolve|Resolves|Resolved|Related to) #\d+).)*)?$" )[raise_minor_issue, request_verification]
commitizen validate_commit_message vs schema_pattern priorityopenwisp_utils/releaser/tests/test_commitizen_rules.py (4)
4-11: Test helperrun_cz_checkis clean and functional.Using
["cz", ...](partial path) triggers Ruff S607, but this is a test utility whereczis expected to be installed in the test environment's PATH. Low risk and acceptable for test code.
14-80: Good coverage of valid commit scenarios.The tests cover key valid patterns: issue symmetry with Fixes/Closes/Related to, multiple issues, double prefixes, merge commits, and no-issue commits. The assertion messages include captured output, which aids debugging.
83-126: Invalid commit tests are thorough with good error-message assertions.Checking that error output contains contextual terms like
"title","body", and"mismatch"ensures the user-facing messages stay helpful across refactors. Nice pattern.
189-210: Auto-append tests validate themessage()→validate_commit_message()round-trip.These tests confirm that the commit messages produced by
message()(with auto-appendedRelated tolines) pass validation, including the multi-issue variant. This is an important integration-level check.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| _TITLE_ISSUE_EXTRACT_RE = re.compile(r" #(\d+)") | ||
| _BODY_ISSUE_RE = re.compile( | ||
| r"(?:Close|Closes|Closed|Fix|Fixes|Fixed|Resolve|Resolves|Resolved|Related to) #(\d+)", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
_TITLE_ISSUE_EXTRACT_RE matches #<number> anywhere in the title, risking false positives.
The pattern #(\d+) will match any space-preceded #<number> token in the title, including text that isn't an issue reference (e.g., [docs] Updated Python #3.12 instructions would extract 3 as an issue number). Consider anchoring to the end of the title or requiring specific placement if this isn't desired.
Additionally, _BODY_ISSUE_RE only captures one issue per keyword occurrence. A line like Fixes #123 #124 would only capture `123`. Each issue number needs its own keyword (e.g., `Fixes `#123`\nFixes `#124). If that is the intended convention, a brief comment documenting it would help.
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
_TITLE_ISSUE_EXTRACT_REmatches#<number>anywhere in the title, risking false positives.The pattern
#(\d+)will match any space-preceded#<number>token in the title, including text that isn't an issue reference (e.g.,[docs] Updated Python#3.12instructionswould extract3as an issue number). Consider anchoring to the end of the title or requiring specific placement if this isn't desired.
This is not worth it, if something starts with #<number> in a commit title nowadays it's 99.9% a reference to an issue. The rest 0.1%, if ever happens, will be dealt with manually.
Additionally,
_BODY_ISSUE_REonly captures one issue per keyword occurrence. A line likeFixes#123#124would only capture `123`. Each issue number needs its own keyword (e.g., `Fixes `#123`\nFixes `#124). If that is the intended convention, a brief comment documenting it would help.
This is an issue and is going to be addressed in the next commit.
There was a problem hiding this comment.
@nemesifier, understood on both points!
For the title pattern, I've noted your preference to keep the simpler regex. It makes sense to optimize for the common case and handle rare edge cases manually when they arise.
Looking forward to seeing the fix for the body issue pattern in the next commit—being able to capture multiple issues from a single keyword line will make the validation more flexible.
✏️ Learnings added
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 593
File: openwisp_utils/releaser/commitizen.py:5-9
Timestamp: 2026-02-10T20:38:21.134Z
Learning: In openwisp-utils commitizen implementation, the `_TITLE_ISSUE_EXTRACT_RE` pattern intentionally matches any space-preceded `#<number>` in commit titles without anchoring to the end. This is acceptable because 99.9% of such patterns are actual issue references, and rare edge cases (like version numbers) will be handled manually if they occur.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 560
File: openwisp_utils/commitizen/openwisp.py:9-25
Timestamp: 2026-02-06T20:46:32.980Z
Learning: In openwisp-utils, the commit message prefix "chores" (plural) is intentionally used instead of "chore" (singular) in the Commitizen ALLOWED_PREFIXES configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/releaser/commitizen.py`:
- Around line 88-97: The set comparison fails because body_issues uses raw
matches from _BODY_ISSUE_RE (e.g. " `#123`") while title_issues uses digits from
_TITLE_ISSUE_EXTRACT_RE; update the body extraction so each match returned by
_BODY_ISSUE_RE is post-processed with _TITLE_ISSUE_EXTRACT_RE to extract the
digit strings (mirroring _extract_body_issues helper) — i.e., build body_issues
by iterating over _BODY_ISSUE_RE.findall(body) and collecting the digit captures
from _TITLE_ISSUE_EXTRACT_RE.findall(match) into a set so missing_issues
compares correctly in message().
🧹 Nitpick comments (3)
openwisp_utils/releaser/tests/test_commitizen_rules.py (2)
140-144: Test nametest_issue_not_at_end_of_titleis slightly misleading.The name implies this should be invalid, but the test asserts success (
code == 0). A name liketest_valid_commit_with_issue_in_middle_of_titlewould better convey the intent.
94-111: Nit: unusedout/errvariables flagged by Ruff (RUF059).In
test_empty_commit_message,test_invalid_prefix_format, andtest_title_not_capitalized, theoutanderrreturn values fromrun_cz_checkare unpacked but unused. You could use_to suppress the warnings, though this is very minor in test code.♻️ Suggested fix (optional)
def test_empty_commit_message(): """Invalid: empty commit message.""" - code, out, err = run_cz_check("") + code, _, _ = run_cz_check("") assert code != 0 def test_invalid_prefix_format(): """Invalid: missing square brackets around prefix.""" message = "qa Good commit message `#1`\n\n" "Body\n\n" "Fixes `#1`" - code, out, err = run_cz_check(message) + code, _, _ = run_cz_check(message) assert code != 0 def test_title_not_capitalized(): """Invalid: title doesn't start with capital letter.""" message = "[qa] bad commit message `#1`\n\n" "Body\n\n" "Fixes `#1`" - code, out, err = run_cz_check(message) + code, _, _ = run_cz_check(message) assert code != 0openwisp_utils/releaser/commitizen.py (1)
237-250:schema_patternregex doesn't match multi-line commits without issue references, but this doesn't affect validation.The regex analysis in the original comment is correct:
no_issue_patternuses[^\n]*$which matches only to the end of the first line, and the lookahead doesn't consume the body. The final\Zthen fails because unconsumed text remains. Tests confirm multi-line no-issue commits like"[chores] Good commit\n\nBody"fail theschema_patternregex.However, this is not a functional issue because
validate_commit_messageis the actual validator used bycz check, and the docstring explicitly acknowledges thatschema_patternprovides only "basic checks" while "actual symmetry enforcement...happens in the validate_commit_message method." Tests pass for multi-line no-issue commits because the validation flow usesvalidate_commit_message, notschema_pattern.That said, fixing the regex for consistency would be prudent: append
[\s\S]*after the lookahead inno_issue_patternto properly consume the entire message, ensuring the regex matches what the code actually validates.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_utils/releaser/commitizen.pyopenwisp_utils/releaser/tests/test_commitizen_rules.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 560
File: openwisp_utils/commitizen/openwisp.py:9-25
Timestamp: 2026-02-06T20:46:32.980Z
Learning: In openwisp-utils, the commit message prefix "chores" (plural) is intentionally used instead of "chore" (singular) in the Commitizen ALLOWED_PREFIXES configuration.
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 593
File: openwisp_utils/releaser/commitizen.py:5-9
Timestamp: 2026-02-10T20:38:21.134Z
Learning: In openwisp-utils commitizen implementation, the `_TITLE_ISSUE_EXTRACT_RE` pattern intentionally matches any space-preceded `#<number>` in commit titles without anchoring to the end. This is acceptable because 99.9% of such patterns are actual issue references, and rare edge cases (like version numbers) will be handled manually if they occur.
📚 Learning: 2026-02-10T20:38:21.134Z
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 593
File: openwisp_utils/releaser/commitizen.py:5-9
Timestamp: 2026-02-10T20:38:21.134Z
Learning: In openwisp-utils commitizen implementation, the `_TITLE_ISSUE_EXTRACT_RE` pattern intentionally matches any space-preceded `#<number>` in commit titles without anchoring to the end. This is acceptable because 99.9% of such patterns are actual issue references, and rare edge cases (like version numbers) will be handled manually if they occur.
Applied to files:
openwisp_utils/releaser/commitizen.pyopenwisp_utils/releaser/tests/test_commitizen_rules.py
📚 Learning: 2026-02-06T20:46:32.980Z
Learnt from: nemesifier
Repo: openwisp/openwisp-utils PR: 560
File: openwisp_utils/commitizen/openwisp.py:9-25
Timestamp: 2026-02-06T20:46:32.980Z
Learning: In openwisp-utils, the commit message prefix "chores" (plural) is intentionally used instead of "chore" (singular) in the Commitizen ALLOWED_PREFIXES configuration.
Applied to files:
openwisp_utils/releaser/commitizen.py
🧬 Code graph analysis (1)
openwisp_utils/releaser/tests/test_commitizen_rules.py (1)
openwisp_utils/releaser/commitizen.py (1)
message(83-97)
🪛 Ruff (0.15.0)
openwisp_utils/releaser/tests/test_commitizen_rules.py
[error] 6-6: Starting a process with a partial executable path
(S607)
[warning] 96-96: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 96-96: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 103-103: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 103-103: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 110-110: Unpacked variable out is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 110-110: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[error] 150-150: Starting a process with a partial executable path
(S607)
⏰ 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)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (3)
openwisp_utils/releaser/tests/test_commitizen_rules.py (3)
14-18: LGTM!Good test coverage for the core valid-commit-with-symmetric-issue-reference scenario.
28-32: Test is now differentiated fromtest_valid_commit_without_issue.The message now uses
"Some explanation"(no period) vs"Some explanation.", addressing the previous duplicate concern.
64-84: Good coverage for multiple-issue scenarios.Both separate-line (
Fixes#123\nFixes#124) and same-line (`Fixes `#123` `#124) variants are tested, exercising the_BODY_ISSUE_REgroup-capture and post-processing logic well.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
2 participants
Checklist
Reference to Existing Issue
Related to #110.