fix(cli): check .pre-commit-config.yaml for conventional commit#310
fix(cli): check .pre-commit-config.yaml for conventional commit#310dbasunag wants to merge 2 commits intoambient-code:mainfrom
Conversation
AgentReady Code ReviewSummaryThis PR extends Attribute Impact Analysis
The Security ReviewNo security issues found. The file read is wrapped in a broad Code QualityIssues to address: 1. Overly broad exception type ( except Exception:
passSince the only operation inside the # Suggested fix
except OSError:
pass2. Misleading test name ( The test writes Suggested rename: 3. Remediation steps not updated for the new detection scope The remediation=Remediation(
summary="Configure conventional commits with commitlint",
steps=["Install commitlint", "Configure husky for commit-msg hook"],
tools=["commitlint", "husky"],
commands=["npm install --save-dev @commitlint/cli @commitlint/config-conventional husky"],
...
)Now that the assessor detects Python-native steps=[
"Option A (Python/pre-commit): Add conventional-pre-commit to .pre-commit-config.yaml",
"Option B (JS/commitlint): Install commitlint and configure husky for commit-msg hook",
],
tools=["pre-commit", "conventional-pre-commit", "commitlint", "husky"],
commands=[
"# Python (pre-commit):",
"pip install pre-commit && pre-commit install --hook-type commit-msg",
"# JS (commitlint + husky):",
"npm install --save-dev @commitlint/cli @commitlint/config-conventional husky",
],4. PR description is empty All checkboxes are unchecked and the description fields contain only the template placeholders. This makes it harder to review intent and blocks automated PR comment generation in CI. Please fill in the description, type of change, and testing notes. AgentReady Pattern Compliance
Coordination NoteRelated PR #308 ( Recommendations
VerdictREQUEST CHANGES — The core detection logic is correct and the test coverage is thorough. Items 1–4 above should be addressed before merge, with item 2 (remediation update) being the most impactful for end users. Generated by AgentReady Review Agent |
dbasunag
force-pushed
the
feature/conventional_commit
branch
from
7884f7a to
4bd482e
Compare
AgentReady Code Review
SummaryThis PR extends Attribute Impact Analysis
Prior Review Status
Remaining Issues1. Empty PR description (minor, process) All description fields are still template placeholders. At a minimum, fill in the "Changes Made" section and the "Related Issues" field. This blocks traceability and makes the automated CI comment generation less useful. 2. Test docstring/name mismatch
"""Test that invalid YAML in pre-commit config is handled gracefully."""But since the implementation uses string matching (not YAML parsing), the test is actually verifying that content without the target patterns returns """Test that pre-commit config without conventional commit patterns fails."""3.
# Note: This test is a no-op when run as root (root ignores file permissions)4. Pattern-matching scope The three patterns cover the main pre-commit hook libraries:
Not covered: 5. Rebase conflict risk with PR #308 PR #308 ( Code Quality
Security ReviewNo issues. File content is read and matched against static string patterns — no execution, no user-controlled input, no subprocess calls. The VerdictAPPROVE with minor comments — The core logic is correct, exception handling is appropriately scoped, and test coverage is thorough (10 cases). The two items to address before merge are:
Neither blocks correctness. Coordinate merge order with PR #308 to avoid a rebase collision. Generated by AgentReady Review Agent |
dbasunag
force-pushed
the
feature/conventional_commit
branch
from
4bd482e to
0dbfa79
Compare
AgentReady Code Review — PR #310Reviewer: Claude Code (AgentReady Development Agent) SummaryThis PR extends Overall verdict: Approve with minor issues noted below. AgentReady Attribute Impact
Issues🔴 Conflict with PR #308 — Expanded commitlint formats not includedPR #310 still only checks for Recommendation: One PR should absorb the other. Specifically, PR #310 should incorporate the expanded commitlint format list from PR #308 before merging. If both land separately, whichever is second will need a rebase. # PR #310 currently has (line ~240):
has_commitlint = (repository.path / ".commitlintrc.json").exists()
# Should be (incorporating PR #308's list):
commitlint_configs = [
".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",
]
has_commitlint = any((repository.path / cfg).exists() for cfg in commitlint_configs)🟡
|
Outdated review (click to expand)Outdated review (click to expand)Outdated review (click to expand)Outdated review (click to expand)Outdated review (click to expand)🤖 AgentReady Code ReviewPR Status: 6 issues found (3 🔴 Critical, 2 🟡 Major, 1 🔵 Minor)
🔴 Critical Issues (Confidence ≥90) — Auto-Fix Recommended1.
|
| # | Issue | Attribute | Tier | Confidence | Status |
|---|---|---|---|---|---|
| 1 | import os inside function |
code_quality |
3 | 95% | 🔴 Unresolved from prior review |
| 2 | Docstring mismatch | inline_documentation |
3 | 92% | 🔴 Unresolved from prior review |
| 3 | Merge conflict with PR #308 | conventional_commits |
2 | 93% | 🔴 Unresolved from prior review |
| 4 | Missing encoding="utf-8" |
code_quality |
3 | 85% | 🟡 New finding |
| 5 | Pattern false positives | conventional_commits |
2 | 88% | 🟡 New finding |
| 6 | Empty PR description | documentation |
3 | 99% | 🔵 Unresolved from prior review |
Prior review items confirmed resolved ✅: except OSError (not bare except), remediation updated with Python-native pre-commit path, test renamed to test_precommit_no_matching_patterns.
- Auto-Fix Candidates: 3 critical issues flagged for resolution
- Manual Review: 2 major issues require human judgment
- Net Score Improvement Potential: +3.0 points from
conventional_commitsfix → 83.0/100 (Gold) - AgentReady Assessment: Run
agentready assess .after merge to verify score
🤖 Generated with Claude Code
If this review was useful, react with 👍. Otherwise, react with 👎.
![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 #310 extends ConventionalCommitsAssessor to detect conventional commit tooling configured via .pre-commit-config.yaml, broadening coverage to Python-first repos. The implementation is largely correct and well-tested, but there are a few issues worth addressing before merge — including a latent conflict with PR #308 and a false-positive risk in the pattern-matching approach.
AgentReady Attribute Impact
| Attribute | Impact | Notes |
|---|---|---|
test_coverage |
✅ Positive | 10 new unit tests added covering all meaningful branches |
pre_commit_hooks |
✅ Positive | Assessor now correctly recognises Python pre-commit ecosystem |
type_annotations |
➡️ Neutral | No new unannotated functions introduced |
code_complexity |
Text-based pattern matching is fragile; see below |
Estimated Score Impact: +1–2 points (improved detection reduces false-fail penalty for Python repos)
Issues Found
🔴 Critical
- Latent merge conflict with PR #308 (
stub_assessors.py:236): PR #310 retainshas_commitlint = (repository.path / ".commitlintrc.json").exists(), checking only one config format. PR #308 (open) expands this to all 14 official commitlint formats by modifying the exact same lines. These two PRs will conflict at merge time and the fix from #308 will be lost if #310 merges first. Coordinate merge order or incorporate #308's commitlint list directly.
🟡 Warnings
-
False-positive risk from substring matching (
stub_assessors.py:248–255): The check usesany(pattern in content for pattern in [...]). A comment like# We chose NOT to use conventional-precommit-linterwould trigger a false positive pass. Prefer checking theid:field values after YAML-parsing the config, or at minimum narrow the pattern toid: conventional-precommit-linter(with theid:prefix) to reduce false matches. -
Missing
package.jsoncommitlint field: Both this PR and the original code ignorepackage.jsonwith a"commitlint"key, which is an officially supported config location per the commitlint docs. Low urgency but worth noting for completeness. -
Non-standard import placement (
tests/unit/test_assessors_stub.py:1049):import osis declared inside the test functiontest_precommit_file_permission_error. Move to the top of the file per PEP 8 / project style.
🟢 Suggestions
-
Misleading test name (
test_precommit_no_matching_patterns): The docstring says "invalid YAML" but the test name says "no matching patterns." Since the implementation uses text search (not YAML parsing), invalid YAML isn't actually tested as an error condition — it just fails to match patterns. Rename totest_precommit_no_conventional_patterns_invalid_yamlor split into two distinct tests to clarify intent. -
Cosmetic-only diff noise: Many of the changes in
test_assessors_stub.pyare pure formatting rewrites of multi-line string calls (e.g.,write_text("""...""")→write_text(\n"""..."""\n)). These inflate the diff significantly and obscure the functional changes. Separate formatting-only commits from functional ones, or omit reformatting unrelated code. -
Pattern list coverage: The three patterns checked (
conventional-precommit-linter,conventional-pre-commit,commitlint-pre-commit-hook) are reasonable, butjorisroovers/gitlintwithconventional-commitstype is also widely used in Python repos and worth adding.
Security Assessment
No security issues identified. The OSError catch on read_text() is appropriate; the code does not execute the config file contents.
Positive Changes
- Strong test coverage with both happy-path and edge-case scenarios (permission error, empty config, invalid YAML, multiple tools configured simultaneously)
- Remediation guidance updated to present both Python (pre-commit) and JS (commitlint + husky) options — improves actionability for Python-first users
- Evidence message in the fail path correctly updated to reflect expanded detection scope
Recommendation
REQUEST_CHANGES — Coordinate with PR #308 to avoid losing the multi-format commitlint fix, and tighten the pattern-matching to reduce false positives. The test suite is solid; the implementation just needs the two targeted fixes above before merge.
dbasunag
force-pushed
the
feature/conventional_commit
branch
from
0dbfa79 to
18742ad
Compare
📈 Test Coverage Report
Coverage calculated from unit tests only |
AgentReady Code Review — PR #310Reviewer: Claude Code (AgentReady Review Agent) SummaryThis PR extends Strengths
IssuesMedium1. Conflict with PR #308 ( PR #308 replaces the single 2. String-based pattern matching can produce false positives (line 246–252) has_precommit_conventional = any(
pattern in content
for pattern in [
"conventional-precommit-linter",
"conventional-pre-commit",
"commitlint-pre-commit-hook",
]
)A commented-out hook or a documentation string containing import yaml
try:
data = yaml.safe_load(content)
repos = data.get("repos", []) if isinstance(data, dict) else []
for repo in repos:
for hook in repo.get("hooks", []):
if hook.get("id", "") in {"conventional-precommit-linter", ...}:
has_precommit_conventional = True
except yaml.YAMLError:
pass # fall back to string matching or return FalseMinor3. Incomplete tool coverage Common conventional-commit tools not detected:
These could be follow-up issues rather than blocking this PR. 4. Misleading test name ( def test_precommit_no_matching_patterns(self, tmp_path):
"""Test that invalid YAML in pre-commit config is handled gracefully."""The test name says "no matching patterns" but the docstring says "invalid YAML". The file 5. Permission test may fail as root (line ~895) os.chmod(precommit_config, 0o000)If tests run as root (e.g., some CI containers), AgentReady Attribute Compliance
RecommendationRequest changes on the conflict with PR #308 before merging. The string-matching false-positive risk is medium severity and worth addressing. The other items are minor and acceptable as follow-up issues. |
dbasunag
force-pushed
the
feature/conventional_commit
branch
2 times, most recently
from
702ba37 to
18b8b4f
Compare
AgentReady Code Review — PR #310Reviewed by: Claude Code (AgentReady Review Agent) SummaryThis PR extends AgentReady Attribute Impact
Score Impact: This fix directly benefits Python-first repositories (the primary AgentReady target audience) that use Code QualityStrengths:
Issues: 1. False positive risk with string matching (medium) The assessor reads the YAML file as raw text and does substring matching: has_precommit_conventional = any(
pattern in content
for pattern in [
"conventional-precommit-linter",
"conventional-pre-commit",
"commitlint-pre-commit-hook",
]
)This will match on:
A YAML-parse approach would be more accurate, though it adds a dependency. Alternatively, restricting the pattern match to known YAML structure (e.g., checking for the pattern only in 2. content = precommit_config.read_text()Should be explicit: 3. Conflict with PR #308 (blocking for one of them) This PR's diff is based on the original code (
4. Limited pre-commit hook coverage (minor) The three patterns detected are reasonable but incomplete. Notable missing hooks:
This is acceptable as a first pass — the attribute description says "≥80% of recent commits follow convention", so this is still an improvement over the status quo. 5. Test: The test is named "no matching patterns" but the docstring says "invalid YAML" and uses malformed YAML as input. These are two different scenarios. Consider splitting or renaming. SecurityNo security concerns. File access is limited to Best Practices
Specific Change Requests# 1. Add encoding parameter
content = precommit_config.read_text(encoding="utf-8")
# 2. Consider adding a comment explaining the string-match trade-off
# Note: Using string matching rather than YAML parsing to avoid adding
# a pyyaml dependency. This may match commented-out entries.VerdictApprove with minor changes — This is a valuable contribution that addresses a real gap for Python repositories. The test suite is thorough and the logic is sound. Address the encoding parameter and coordinate with PR #308 before final merge. The false-positive risk from string matching is acceptable as a documented trade-off. |
AgentReady Code ReviewPR: fix(cli): check .pre-commit-config.yaml for conventional commit SummaryThis PR extends Overall: Strong implementation with good test coverage, but has a few false-positive risks and one CI-reliability concern. AgentReady Attribute Compliance
Issues🔴 CriticalNone. 🟡 Important1.
conventional_patterns = {
"conventional-precommit-linter",
"conventional-pre-commit",
"commitlint-pre-commit-hook",
"gitlint", # ← concern
"committed", # ← concern
}
Recommendation: Remove 2. String fallback can match comments The YAML error fallback uses content-level string matching: except yaml.YAMLError:
has_precommit_conventional = any(
pattern in content for pattern in conventional_patterns
)A 3. Permission error test is a no-op when running as root def test_precommit_file_permission_error(self, tmp_path):
os.chmod(precommit_config, 0o000)
# "Note: This test is a no-op when run as root"CI containers often run as root, making this test silently pass regardless of the import pytest
@pytest.mark.skipif(os.getuid() == 0, reason="chmod has no effect as root")
def test_precommit_file_permission_error(self, tmp_path):
...🟢 Suggestions4. Conflict with PR #308 Both this PR and PR #308 modify the same 5. Missing A common pattern in Node.js monorepos: commitlint config lives in 6. Empty PR description The "Changes Made" section has three empty bullets ( Strengths
VerdictRequest changes — small issues to address before merge:
The core logic and test coverage are solid. |
dbasunag
force-pushed
the
feature/conventional_commit
branch
from
18b8b4f to
e6d0d86
Compare
There was a problem hiding this comment.
AgentReady Code Review — PR #310
fix(cli): check .pre-commit-config.yaml for conventional commit
Summary
This PR meaningfully expands ConventionalCommitsAssessor to detect conventional commit tooling in Python-first workflows — specifically package.json commitlint config and pre-commit hooks. The YAML-based parsing approach (vs. naive string matching) is the right call and avoids false positives from comments. Test coverage is excellent at 14 new cases. One required change: this PR should also incorporate the full commitlint config format list from PR #308 to avoid a regression.
AgentReady Attribute Compliance
Attribute affected: conventional_commits (Tier 2, weight ~0.03)
| Aspect | Assessment |
|---|---|
| Fix correctness | ✅ Correct detection logic for both new paths |
| YAML parsing approach | ✅ Correctly uses yaml.safe_load not string matching |
| Error handling | ✅ JSONDecodeError, OSError, YAMLError all caught |
| pyyaml dependency | ✅ Already in pyproject.toml (pyyaml>=6.0) |
| Test coverage | ✅ 14 new test cases covering all key paths |
| Remediation quality | ✅ Improved with language-specific options (Python vs JS) |
| Score impact | conventional_commits 0 → 100 for pre-commit users (+~0.9 overall) |
Security
No security issues found.
- File reads are wrapped in
OSErrorexception handlers yaml.safe_load()used (notyaml.load()— avoids arbitrary code execution)- No subprocess calls, no user-controlled execution paths
Code Quality
Strengths:
- YAML-parsed URL matching is more robust than string/regex matching against raw file content
yaml.safe_load()is correctly chosen overyaml.load()- The
conventional_patternsset accurately avoids generic linters (gitlint,committed) that don't enforce conventional format by default — the inline comment explains this decision well test_precommit_file_permission_errorwith@pytest.mark.skipif(os.getuid() == 0, ...)is a thoughtful edge case
Issues:
1. [Required] Missing commitlint config formats from PR #308
This PR only checks .commitlintrc.json, but PR #308 identified 14 valid formats (.commitlintrc, .commitlintrc.yaml, .commitlintrc.ts, commitlint.config.js, etc.). If this PR merges without those additions, false negatives for those formats remain. Either rebase on top of PR #308 or incorporate the full list.
2. [Minor] Redundant assignment in except yaml.YAMLError block
has_precommit_conventional is already initialized to False before the try block. The explicit reset inside except yaml.YAMLError is dead code. Remove it or replace with pass.
3. [Minor] Misleading test docstring
test_precommit_invalid_yaml_fallback says 'falls back to string matching' but the implementation does not fall back to string matching — it fails gracefully with False. Update the docstring.
4. [Non-blocking] conventional_patterns as a class-level constant
The patterns set is redefined inside assess() on every call. Since it is a fixed constant, a class-level attribute would be cleaner.
5. [Non-blocking] Pass evidence doesn't identify which tool was detected
All pass paths return 'Commit linting configured' regardless of detection method. Including the specific detection path in evidence would help with diagnostics.
Best Practices
- ✅ Library-first: no global state introduced
- ✅ Strategy pattern preserved
- ✅ Graceful degradation: missing/unreadable files do not crash
- ✅ Conventional commit style in PR title (
fix(cli):) - ✅ Tests confirm behavior, not just execution
Required Changes
- Incorporate the full commitlint config format list (14 formats from PR #308) to avoid false negatives for
.commitlintrc.yaml,.commitlintrc.ts,commitlint.config.js, etc.
Suggestions (non-blocking)
- Remove redundant
has_precommit_conventional = Falseinexcept yaml.YAMLError - Fix misleading docstring in
test_precommit_invalid_yaml_fallback - Extract
conventional_patternsto a class-level constant - Add evidence specificity to identify which config method was detected
Verdict
REQUEST CHANGES — The core logic is sound and the test coverage is a significant improvement. One required change before merge: incorporate the full commitlint config format list from PR #308. The remaining items are minor/non-blocking.
AgentReady Code Review — PR #310
SummaryGood implementation that expands conventional commit detection to Python-ecosystem tooling. Proper YAML parsing (avoiding false positives from string matching), solid error handling, and thorough test coverage. A few things to address. IssuesMediumMisleading test docstring for The docstring says "falls back to string matching" but the implementation returns Tests in Tests using Minor
Notable missing tools:
This correctly detects No detection of Commitizen is configured in What's Good
Attribute Impact
Verdict: Approve with minor fixes. Fix the misleading docstring before merge. Consider adding |
2 participants
Description
Add support for conventional commit check via pre-commit config.
Type of Change
Related Issues
Fixes #
Relates to #
Changes Made
Conventional_commit check now identifies
Testing
pytest)Checklist
Screenshots (if applicable)
Additional Notes