fix: Check for all official commitlint config file formats#308
fix: Check for all official commitlint config file formats#308kami619 merged 1 commit intoambient-code:mainfrom
Conversation
AgentReady Code Review — PR #308fix: Check for all official commitlint config file formats SummaryThis is a straightforward and correct bug fix. The AgentReady Attribute ComplianceAttribute affected:
Score impact example: A repo using Code QualityStrengths:
Minor concerns:
SecurityNo security concerns. This change only reads from the filesystem using TestingGap: No tests were added for this fix, and the PR checklist reflects this (unit tests unchecked). The existing Suggested test cases to add: @pytest.mark.parametrize("config_file", [
".commitlintrc",
".commitlintrc.yaml",
".commitlintrc.yml",
".commitlintrc.ts",
"commitlint.config.js",
"commitlint.config.ts",
])
def test_conventional_commits_detects_config_formats(tmp_path, config_file):
(tmp_path / config_file).touch()
repo = Repository(path=tmp_path)
assessor = ConventionalCommitsAssessor()
finding = assessor.assess(repo)
assert finding.status == "pass"
assert finding.score == 100.0
def test_conventional_commits_fails_with_no_config(tmp_path):
repo = Repository(path=tmp_path)
assessor = ConventionalCommitsAssessor()
finding = assessor.assess(repo)
assert finding.status == "fail"
assert finding.score == 0.0Tests would also catch regressions if the config list is edited in the future. VerdictApprove with non-blocking suggestions. The core fix is correct, minimal, and clearly scoped. The two follow-up items (package.json support, test coverage) are pre-existing gaps that should be tracked separately rather than blocking this fix. Recommend adding test coverage before or shortly after merge given this project targets >80% test coverage per CLAUDE.md. |
📈 Test Coverage Report
Coverage calculated from unit tests only |
AgentReady Code ReviewPR #308 · fix: Check for all official commitlint config file formats SummaryThis PR fixes a false-negative bug in File changed: AgentReady Attribute Compliance
Security Review
No security concerns. Code QualityThe implementation is clean and correct. Before (only one format checked): has_commitlint = (repository.path / ".commitlintrc.json").exists()After (all official formats, idiomatic has_commitlint = any(
(repository.path / cfg).exists() for cfg in commitlint_configs
)Strengths:
Minor issues:
Testing Gap (Requires Attention)No unit tests were added. Per CLAUDE.md:
There are currently no direct unit tests for Suggested tests to add in @pytest.mark.parametrize("config_file", [
".commitlintrc",
".commitlintrc.yaml",
".commitlintrc.yml",
".commitlintrc.ts",
"commitlint.config.js",
"commitlint.config.ts",
])
def test_conventional_commits_detects_all_config_formats(tmp_path, config_file):
(tmp_path / config_file).touch()
repo = Repository(path=tmp_path)
assessor = ConventionalCommitsAssessor()
finding = assessor.assess(repo)
assert finding.status == "pass"
assert finding.score == 100.0
def test_conventional_commits_fails_when_no_config(tmp_path):
repo = Repository(path=tmp_path)
finding = ConventionalCommitsAssessor().assess(repo)
assert finding.status == "fail"
assert finding.score == 0.0Estimated Score Impact
VerdictThe fix is correct and well-implemented. The only blocking concern before merge is the missing unit tests. The stale comment and Recommend: Add unit tests covering the new config formats, update the inline comment, then merge. Review generated by AgentReady code review skill |
|
@ntkathole, thank you for this contribution, I think you make a valid case for broadening the assessor scope, did you want to address the comment around |
AgentReady Code Review — PR #308Reviewer: Claude Code (AgentReady Development Agent) SummaryThis PR fixes a real bug: Overall verdict: Request changes — no tests, and conflicts with PR #310. AgentReady Attribute Impact
Issues🔴 No tests addedPer CLAUDE.md: "All new assessors must have unit tests". This PR modifies detection logic in a production assessor but the PR checklist confirms no tests were written ( Each new config format should have at least a positive detection test. At minimum, tests for the formats most likely to be encountered in the wild ( 🔴 Conflict with PR #310Both PRs modify the exact same lines in Recommendation: Coordinate with PR #310 author to combine both changes into one PR, or handle as a sequential rebase. The combined assessor should include both: the expanded commitlint format list from this PR and the pre-commit hook detection from PR #310. 🟡
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AgentReady Code Review
Summary
PR #308 is a small, focused fix that expands ConventionalCommitsAssessor to recognise all 14 official commitlint config file formats, rather than just .commitlintrc.json. The change is correct and complete for the commitlint side, but ships without tests and has a coordination issue with the open PR #310.
AgentReady Attribute Impact
| Attribute | Impact | Notes |
|---|---|---|
test_coverage |
Fix is untested; no new unit tests added | |
pre_commit_hooks |
✅ Positive | Fewer false-fail results for repos using non-JSON commitlint configs |
Estimated Score Impact: +0–1 points (reduces false-fail penalty for repos using .commitlintrc.yaml, .commitlintrc.ts, etc.)
Issues Found
🔴 Critical
- Latent merge conflict with PR #310 (
stub_assessors.py:236–254): PR #310 (open) modifies the sameassess()block, including thehas_commitlintvariable. If PR #308 merges first, PR #310 must be rebased and updated to incorporate the new multi-format list. If PR #310 merges first, this PR's fix is lost entirely. Coordinate with the PR #310 author.
🟡 Warnings
-
No tests (
tests/): The PR checklist explicitly marks test coverage as not done. At minimum, add a parameterised test verifying each of the new config formats is detected. Example:@pytest.mark.parametrize("config_file", [ ".commitlintrc.yaml", ".commitlintrc.yml", ".commitlintrc.ts", "commitlint.config.js", "commitlint.config.ts", ]) def test_commitlint_config_formats(self, tmp_path, config_file): (tmp_path / config_file).write_text("extends: ['@commitlint/config-conventional']") ... assert finding.status == "pass"
-
Missing
package.jsoncommitlint key: The official commitlint docs listpackage.jsonwith a"commitlint"field as a supported config source. This is out of scope for a focused fix, but worth a follow-up issue.
🟢 Suggestions
- Missing
.commitlintrc.mts: The list includes.commitlintrc.ctsbut not.commitlintrc.mts(ESM TypeScript). Neither is in the current official docs, so this is minor — just flag it for awareness.
Security Assessment
No security issues identified. The change is a pure static list extension with no file I/O or execution.
Positive Changes
- The
any(... for cfg in commitlint_configs)pattern is clean and easily extensible - Covers all formats listed in the official commitlint documentation
Recommendation
REQUEST_CHANGES — Add at least a parameterised unit test for the new config formats, and coordinate merge order with PR #310 to avoid losing this fix. The underlying change itself is correct.
AgentReady Code Review — PR #308Reviewer: Claude Code (AgentReady Review Agent) SummaryThis PR fixes a real gap: the original assessor checked only Strengths
IssuesHigh1. No tests included All test checklist items are unchecked in the PR description. This is a direct miss against the project's test requirements (CLAUDE.md: "All new assessors must have unit tests"). The fix should include at minimum:
Without tests, a future refactor could silently drop formats from the list with no breakage signal. Medium2. Conflict with PR #310 PR #310 modifies the same Minor3. A significant number of projects configure conventional commits via 4. No Commitizen also supports standalone config files ( AgentReady Attribute Compliance
RecommendationRequest changes: Tests must be added before merging. The change itself is correct — this is a low-risk, high-value fix that just needs test coverage to meet project standards. Once tests are added and the conflict with #310 is resolved (or acknowledged with a merge order), this is ready. |
AgentReady Code Review — PR #308Reviewed by: Claude Code (AgentReady Review Agent) SummaryThis PR expands AgentReady Attribute Impact
Score Impact: This change reduces false negatives for the Code QualityStrengths:
Issues: 1. Missing tests (blocking) At minimum, tests should verify:
2. Conflict with PR #310
3. Missing SecurityNo security concerns. This change only calls Best Practices
VerdictRequest Changes — The fix itself is correct, but tests are required before merge. Additionally, this PR should be coordinated with PR #310 since both touch the same assessor. |
AgentReady Code ReviewPR: fix: Check for all official commitlint config file formats SummaryThis PR extends Overall: Correct fix, but needs tests before merge. AgentReady Attribute Compliance
Issues🔴 CriticalNone. 🟡 Important1. No tests added The PR checklist explicitly marks "I have added tests" as unchecked. Per CLAUDE.md: "All new assessors must have unit tests". The existing
2. Conflict with PR #310 Both this PR and PR #310 modify the same 🟢 Suggestions3. Missing Many Node.js projects configure commitlint directly in 4. Worth noting in documentation: Strengths
VerdictRequest changes — the logic is correct, but tests are required per project standards before this can merge. Adding 3-4 test cases for the new file formats would make this PR complete. |
There was a problem hiding this comment.
AgentReady Code Review — PR #308
fix: Check for all official commitlint config file formats
Summary
This is a correct and well-scoped bug fix. ConventionalCommitsAssessor was limited to .commitlintrc.json, causing false negatives for the majority of valid commitlint setups. The fix is minimal and addresses the root cause directly.
AgentReady Attribute Compliance
Attribute affected: conventional_commits (Tier 2, weight ~0.03)
| Aspect | Assessment |
|---|---|
| Fix correctness | ✅ Addresses real false negatives |
| Config list completeness | ✅ Matches official commitlint docs |
| Backwards compatibility | ✅ No breaking changes, purely additive |
| Assessor pattern | ✅ Consistent with existing BaseAssessor implementation |
Score impact: Repos using any config format other than .commitlintrc.json (e.g. .commitlintrc.yaml, commitlint.config.ts) would have incorrectly scored 0 on this Tier 2 attribute. After this fix they correctly score 100.
Security
No security concerns. Path.exists() only — no file I/O, no subprocess, no user input.
Code Quality
Strengths:
any()with a generator expression is idiomatic and efficient- The config list is exhaustive and matches the official commitlint configuration reference
- Single responsibility: change is minimal and laser-focused
Issues:
1. No tests added — The PR checklist shows unit tests unchecked, and there is currently zero test coverage for ConventionalCommitsAssessor. This is the only required change for approval.
Suggested parametrized test to add in tests/unit/test_assessors_stub.py:
@pytest.mark.parametrize("config_file", [
".commitlintrc",
".commitlintrc.yaml",
".commitlintrc.yml",
".commitlintrc.ts",
"commitlint.config.js",
"commitlint.config.ts",
"commitlint.config.cts",
])
def test_conventional_commits_detects_all_config_formats(tmp_path, config_file):
subprocess.run(["git", "init"], cwd=tmp_path, capture_output=True, check=True)
(tmp_path / config_file).touch()
repo = Repository(
path=tmp_path, name="test", url=None, branch="main",
commit_hash="abc", languages={}, total_files=1, total_lines=1,
)
finding = ConventionalCommitsAssessor().assess(repo)
assert finding.status == "pass"
assert finding.score == 100.02. Pre-existing gap not addressed here — .husky/ existing only proves husky is installed, not that a commit-msg hook actually invokes commitlint. Out of scope for this PR, but a follow-up issue would be useful.
3. Coordination with PR #310 — PR #310 also modifies ConventionalCommitsAssessor and adds package.json + pre-commit detection. One of these PRs should build on the other to avoid merge conflicts and missing features. If #308 merges first, #310 will need a rebase that incorporates the full config list.
Best Practices
The commitlint_configs list is defined inside assess(). Since it's a fixed constant, it would be more appropriate as a class-level attribute or module-level constant, making it easier to extend and test:
class ConventionalCommitsAssessor(BaseAssessor):
_COMMITLINT_CONFIGS = [
".commitlintrc",
".commitlintrc.json",
# ...
]This is a non-blocking suggestion.
Required Changes
- Add unit tests for at least the new config formats — this is blocking per project test requirements
Suggestions (non-blocking)
- Extract
commitlint_configsto a class-level constant - File a follow-up issue for
package.jsoncommitlint detection (covered in PR #310) - Coordinate merge order with PR #310 to avoid rework
Verdict
REQUEST CHANGES — The fix logic is correct and complete, but unit tests are required before merge per project policy. Adding a parametrized test for the new config formats (see example above) would satisfy this requirement.
ntkathole
force-pushed
the
fix_commitlint
branch
2 times, most recently
from
92f4a4a to
29df278
Compare
Signed-off-by: ntkathole <nikhilkathole2683@gmail.com>
ntkathole
force-pushed
the
fix_commitlint
branch
from
29df278 to
b79600f
Compare
|
@ntkathole for the contribution. Appreciate you adding the tests as well. lgtm. |
## [2.29.1](v2.29.0...v2.29.1) (2026-02-20) ### Bug Fixes * Check for all official commitlint config file formats ([#308](#308)) ([50588cf](50588cf))
|
🎉 This PR is included in version 2.29.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Description
The
ConventionalCommitsAssessorin agentready was hardcoded to only check for.commitlintrc.jsonand.husky/, but the projects using other formats like .commitlintrc.yaml- a valid commitlint config format wasn't being recognized.Type of Change
Changes Made
Updated the assessor in
src/agentready/assessors/stub_assessors.pyto check for all official commitlint config file formats (.commitlintrc, .commitlintrc.json, .commitlintrc.yaml, .commitlintrc.yml, .commitlintrc.js, .commitlintrc.cjs, .commitlintrc.mjs, .commitlintrc.ts, .commitlintrc.cts, commitlint.config.js, commitlint.config.cjs, commitlint.config.mjs, commitlint.config.ts, commitlint.config.cts).Testing
pytest)Checklist