Skip to content

fix: remove allow_authors and add ignore_authors to branch section #292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shenxianpeng merged 4 commits into from
Oct 6, 2025
Merged

fix: remove allow_authors and add ignore_authors to branch section #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b8025e
fix: remove allow_authors and add ignore_authors to branch section
shenxianpeng Oct 6, 2025
23f21d0
fix: update test base on review
shenxianpeng Oct 6, 2025
9edc741
fix: update test base on review
shenxianpeng Oct 6, 2025
ba56950
chore: update suggest and readme
shenxianpeng Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,8 @@ Check Branch Naming Failed

Type branch check failed ==> test-branch
It doesn't match regex: ^(feature|bugfix|hotfix|release|chore|feat|fix)\/.+|(master)|(main)|(HEAD)|(PR-.+)
The branch should follow Conventional Branch. See https://conventional-branches.github.io/
Suggest: git checkout -b <type>/<branch_name>
The branch should follow Conventional Branch. See https://conventional-branch.github.io/
Suggest: Use <type>/<description> with allowed types or ignore_authors in config branch section to bypass


Check Commit Signature Failed
Expand Down
2 changes: 1 addition & 1 deletion cchk.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ allow_fixup_commits = true
allow_wip_commits = false
require_body = false
require_signed_off_by = false
allow_authors = []
ignore_authors = ["dependabot[bot]", "copilot[bot]"]

[branch]
# https://conventional-branch.github.io/
conventional_branch = true
allow_branch_types = ["feature", "bugfix", "hotfix", "release", "chore", "feat", "fix"]
require_rebase_target = "main"
ignore_authors = ["dependabot[bot]", "copilot[bot]"]
60 changes: 51 additions & 9 deletions commit_check/engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from abc import ABC, abstractmethod
from dataclasses import dataclass
from enum import IntEnum
from dataclasses import field

from commit_check.rule_builder import ValidationRule
from commit_check.util import (
Expand All @@ -28,6 +29,7 @@ class ValidationContext:

stdin_text: Optional[str] = None
commit_file: Optional[str] = None
config: Dict = field(default_factory=dict)


class BaseValidator(ABC):
Expand All @@ -42,7 +44,45 @@ def validate(self, context: ValidationContext) -> ValidationResult:
pass

def _should_skip_validation(self, context: ValidationContext) -> bool:
"""Determine if validation should be skipped."""
"""
Determine if validation should be skipped.

Skip only when there is no stdin_text, no commit_file, and no commits.
"""
return (
context.stdin_text is None
and context.commit_file is None
and not has_commits()
)

def _should_skip_commit_validation(self, context: ValidationContext) -> bool:
"""
Determine if commit validation should be skipped.

Skip if the current author is in the ignore_authors list for commits,
or if no stdin_text, no commit_file, and no commits exist.
"""
ignore_authors = context.config.get("commit", {}).get("ignore_authors", [])
current_author = get_commit_info("an")
if current_author and current_author in ignore_authors:
return True
return (
context.stdin_text is None
and context.commit_file is None
and not has_commits()
)

def _should_skip_branch_validation(self, context: ValidationContext) -> bool:
"""
Determine if branch validation should be skipped.

Skip if the current author is in the ignore_authors list for branches,
or if no stdin_text and no commits exist.
"""
ignore_authors = context.config.get("branch", {}).get("ignore_authors", [])
current_author = get_commit_info("an")
if current_author and current_author in ignore_authors:
return True
return context.stdin_text is None and not has_commits()
Comment on lines +75 to 86

@coderabbitai coderabbitai Bot Oct 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Branch skip logic omits commit_file check.

The method only checks stdin_text and has_commits() (line 86), but omits commit_file unlike the other skip methods. This inconsistency means branch validation may incorrectly skip when a commit message file is present.

Apply this diff to align with the other skip methods:

     def _should_skip_branch_validation(self, context: ValidationContext) -> bool:
         """
         Determine if branch validation should be skipped.
 
         Skip if the current author is in the ignore_authors list for branches,
-        or if no stdin_text and no commits exist.
+        or if no stdin_text, no commit_file, and no commits exist.
         """
         ignore_authors = context.config.get("branch", {}).get("ignore_authors", [])
         current_author = get_commit_info("an")
         if current_author and current_author in ignore_authors:
             return True
-        return context.stdin_text is None and not has_commits()
+        return (
+            context.stdin_text is None
+            and context.commit_file is None
+            and not has_commits()
+        )
📝 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.

Suggested change
def _should_skip_branch_validation(self, context: ValidationContext) -> bool:
"""
Determine if branch validation should be skipped.
Skip if the current author is in the ignore_authors list for branches,
or if no stdin_text and no commits exist.
"""
ignore_authors = context.config.get("branch", {}).get("ignore_authors", [])
current_author = get_commit_info("an")
if current_author and current_author in ignore_authors:
return True
return context.stdin_text is None and not has_commits()
def _should_skip_branch_validation(self, context: ValidationContext) -> bool:
"""
Determine if branch validation should be skipped.
Skip if the current author is in the ignore_authors list for branches,
or if no stdin_text, no commit_file, and no commits exist.
"""
ignore_authors = context.config.get("branch", {}).get("ignore_authors", [])
current_author = get_commit_info("an")
if current_author and current_author in ignore_authors:
return True
return (
context.stdin_text is None
and context.commit_file is None
and not has_commits()
)
🤖 Prompt for AI Agents 
In commit_check/engine.py around lines 75 to 86, the branch-skip logic currently
only checks stdin_text and has_commits() but omits checking for a commit_file,
causing inconsistent behavior; modify the final return so it also requires
context.commit_file to be None (i.e., return context.stdin_text is None and
context.commit_file is None and not has_commits()), aligning this method with
the other skip methods.


def _print_failure(self, actual_value: str, regex_or_constraint: str = "") -> None:
Expand All @@ -58,7 +98,7 @@ class CommitMessageValidator(BaseValidator):
"""Validates commit messages against conventional commit standards."""

def validate(self, context: ValidationContext) -> ValidationResult:
if self._should_skip_validation(context):
if self._should_skip_commit_validation(context):
return ValidationResult.PASS

message = self._get_commit_message(context)
Expand Down Expand Up @@ -95,7 +135,7 @@ class SubjectValidator(BaseValidator):
"""Validates commit subject lines."""

def validate(self, context: ValidationContext) -> ValidationResult:
if self._should_skip_validation(context):
if self._should_skip_commit_validation(context):
return ValidationResult.PASS

subject = self._get_subject(context)
Expand Down Expand Up @@ -198,7 +238,8 @@ class AuthorValidator(BaseValidator):
"""Validates author information."""

def validate(self, context: ValidationContext) -> ValidationResult:
if self._should_skip_validation(context):
# Use commit skip logic for ignore_authors
if self._should_skip_commit_validation(context):
return ValidationResult.PASS

author_value = self._get_author_value(context)
Expand Down Expand Up @@ -243,6 +284,8 @@ class BranchValidator(BaseValidator):
"""Validates branch names."""

def validate(self, context: ValidationContext) -> ValidationResult:
if self._should_skip_branch_validation(context):
return ValidationResult.PASS
branch_name = (
context.stdin_text.strip() if context.stdin_text else get_branch_name()
)
Expand All @@ -263,7 +306,7 @@ class MergeBaseValidator(BaseValidator):
"""Validates merge base ancestry."""

def validate(self, context: ValidationContext) -> ValidationResult:
if not has_commits():
if self._should_skip_branch_validation(context):
return ValidationResult.PASS

current_branch = get_branch_name()
Expand Down Expand Up @@ -347,7 +390,7 @@ class BodyValidator(BaseValidator):
"""Validates that commit messages contain a body when required."""

def validate(self, context: ValidationContext) -> ValidationResult:
if self._should_skip_validation(context):
if self._should_skip_commit_validation(context):
return ValidationResult.PASS

message = self._get_commit_message(context)
Expand Down Expand Up @@ -395,7 +438,7 @@ class CommitTypeValidator(BaseValidator):
"""Base validator for special commit types (merge, revert, fixup, WIP, empty)."""

def validate(self, context: ValidationContext) -> ValidationResult:
if self._should_skip_validation(context):
if self._should_skip_commit_validation(context):
return ValidationResult.PASS

message = self._get_commit_message(context)
Expand Down Expand Up @@ -483,8 +526,6 @@ class ValidationEngine:
"subject_min_length": SubjectLengthValidator,
"author_name": AuthorValidator,
"author_email": AuthorValidator,
"allow_authors": AuthorValidator,
"ignore_authors": AuthorValidator,
"branch": BranchValidator,
"merge_base": MergeBaseValidator,
"require_signed_off_by": SignoffValidator,
Expand All @@ -494,6 +535,7 @@ class ValidationEngine:
"allow_empty_commits": CommitTypeValidator,
"allow_fixup_commits": CommitTypeValidator,
"allow_wip_commits": CommitTypeValidator,
"ignore_authors": CommitTypeValidator,

@coderabbitai coderabbitai Bot Oct 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify the "ignore_authors" validator mapping.

This mapping suggests ignore_authors is a validation check, but the code shows it's used in skip logic via context.config. Verify whether this mapping is intentional or if it creates unexpected behavior when rules contain an ignore_authors check.

Run this script to check if ignore_authors is used as a validation rule:

🏁 Script executed:

#!/bin/bash
# Search for ignore_authors used as a validation check in rules

# Check rule_builder.py for ignore_authors handling
echo "=== Checking rule_builder.py for ignore_authors ==="
ast-grep --pattern $'def build_rules($$$) {
  $$$
  ignore_authors
  $$$
}'

# Check rules_catalog.py for ignore_authors rules
echo -e "\n=== Checking rules_catalog.py for ignore_authors ==="
rg -nP --type=py 'ignore_authors.*:.*ValidationRule|ValidationRule.*ignore_authors' commit_check/rules_catalog.py

# Check TOML config for ignore_authors usage
echo -e "\n=== Checking TOML for ignore_authors ==="
rg -nP 'ignore_authors' --glob='*.toml'

# Check tests for ignore_authors as a validation rule (not config)
echo -e "\n=== Checking tests for ignore_authors as a rule check ==="
rg -nP 'ValidationRule.*check.*ignore_authors|ignore_authors.*ValidationRule' tests/

Length of output: 338

Remove ignore_authors from VALIDATOR_MAP: this key is only for skip logic (configured in cchk.toml), not a validation rule, so mapping it to CommitTypeValidator is incorrect.

🤖 Prompt for AI Agents 
In commit_check/engine.py around line 538, remove the "ignore_authors":
CommitTypeValidator entry from VALIDATOR_MAP because "ignore_authors" is only
used for skip logic (configured in cchk.toml) and is not a validation rule;
delete that key-to-class mapping and adjust surrounding commas/formatting so the
dict remains valid after removal.

}

def __init__(self, rules: List[ValidationRule]):
Expand Down
1 change: 1 addition & 0 deletions commit_check/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ def main() -> int:
context = ValidationContext(
stdin_text=stdin_content,
commit_file=commit_file_path,
config=config_data,
)

# Run validation
Expand Down
12 changes: 2 additions & 10 deletions commit_check/rule_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ def _build_single_rule(
return self._build_length_rule(catalog_entry, "subject_max_length")
elif check == "subject_min_length":
return self._build_length_rule(catalog_entry, "subject_min_length")
elif check == "allow_authors":
return self._build_author_list_rule(catalog_entry, "allow_authors")
elif check == "ignore_authors":
return self._build_author_list_rule(catalog_entry, "ignore_authors")
elif check == "merge_base":
Expand Down Expand Up @@ -174,15 +172,9 @@ def _build_author_list_rule(
if not isinstance(author_list, list) or not author_list:
return None

if config_key == "allow_authors":
return ValidationRule(
check=catalog_entry.check,
error=catalog_entry.error,
suggest=catalog_entry.suggest,
allowed=author_list,
)
else: # ignore_authors
if config_key == "ignore_authors":
return ValidationRule(check=catalog_entry.check, ignored=author_list)
return None

def _build_merge_base_rule(
self, catalog_entry: RuleCatalogEntry
Expand Down
16 changes: 8 additions & 8 deletions commit_check/rules_catalog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,6 @@ class RuleCatalogEntry:
error="The committer's email seems invalid",
suggest="git config user.email yourname@example.com",
),
RuleCatalogEntry(
check="allow_authors",
regex=None,
error="Author is not allowed",
suggest="Use a configured author or adjust configuration",
),
RuleCatalogEntry(
check="ignore_authors",
regex=None,
Expand All @@ -117,13 +111,19 @@ class RuleCatalogEntry:
RuleCatalogEntry(
check="branch",
regex=None, # Built dynamically from config
error="The branch should follow Conventional Branch. See https://conventional-branches.github.io/",
suggest="git checkout -b <type>/<branch_name>",
error="The branch should follow Conventional Branch. See https://conventional-branch.github.io/",
suggest="Use <type>/<description> with allowed types or ignore_authors in config branch section to bypass",
),
RuleCatalogEntry(
check="merge_base",
regex=None, # Provided by config
error="Current branch is not rebased onto target branch",
suggest="Rebase or merge with the target branch",
),
RuleCatalogEntry(
check="ignore_authors",
regex=None,
error=None,
suggest=None,
),
]
12 changes: 6 additions & 6 deletions docs/configuration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ Example Configuration
allow_fixup_commits = true
allow_wip_commits = false
require_body = false
# allow_authors = [] # Optional - all authors allowed by default
# ignore_authors = [] # Optional - no authors ignored by default
require_signed_off_by = false
# required_signoff_name = "Your Name" # Optional
Expand All @@ -47,6 +46,7 @@ Example Configuration
conventional_branch = true
allow_branch_types = ["feature", "bugfix", "hotfix", "release", "chore", "feat", "fix"]
# require_rebase_target = "main" # Optional - no rebase requirement by default
# ignore_authors = [] # Optional - no authors ignored by default


Options Table Description
Expand Down Expand Up @@ -120,11 +120,6 @@ Options Table Description
- bool
- false
- Require a body in the commit message.
* - commit
- allow_authors
- list[str]
- [] (all allowed)
- List of allowed authors. If empty, all authors are allowed except those in ignore_authors.
* - commit
- ignore_authors
- list[str]
Expand All @@ -150,3 +145,8 @@ Options Table Description
- str
- None (no requirement)
- Target branch for rebase requirement. If not set, no rebase validation is performed.
* - branch
- ignore_authors
- list[str]
- [] (none ignored)
- List of authors to ignore (i.e., always allow).
1 change: 0 additions & 1 deletion docs/migration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ YAML (v1.x) vs TOML (v2.0+)
allow_wip_commits = false
require_body = false
require_signed_off_by = false
allow_authors = []
ignore_authors = ["dependabot[bot]", "copilot[bot]"]

[branch]
Expand Down
4 changes: 1 addition & 3 deletions docs/what-is-new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,10 @@ Flexible author validation with allow/ignore lists.

[commit]
# Built-in validation with sensible defaults for author name/email
# Optional: restrict to specific authors
allow_authors = ["John Doe <john@example.com>", "Jane Smith <jane@example.com>"]
# Optional: ignore specific authors (e.g., bots)
ignore_authors = ["dependabot[bot]", "renovate[bot]"]

**Benefits**: Built-in validation patterns, flexible allow/ignore lists, automatic bot detection.
**Benefits**: Built-in validation patterns, flexible ignore lists, automatic bot detection.

Signed-off-by Requirements
^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
3 changes: 1 addition & 2 deletions tests/engine_comprehensive_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,6 @@ def test_validation_engine_validator_map(self):
"subject_min_length": SubjectLengthValidator,
"author_name": AuthorValidator,
"author_email": AuthorValidator,
"allow_authors": AuthorValidator,
"ignore_authors": AuthorValidator,
"branch": BranchValidator,
"merge_base": MergeBaseValidator,
"require_signed_off_by": SignoffValidator,
Expand All @@ -248,6 +246,7 @@ def test_validation_engine_validator_map(self):
"allow_empty_commits": CommitTypeValidator,
"allow_fixup_commits": CommitTypeValidator,
"allow_wip_commits": CommitTypeValidator,
"ignore_authors": CommitTypeValidator,
}

for check, validator_class in expected_mappings.items():
Expand Down
Loading
Loading