Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jobs:
uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0

- name: Install pre-commit and test dependencies
run: uv pip install --system pre-commit jsonschema
run: uv pip install --system pre-commit jsonschema pytest gitlint-core

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[high] protected-path

This PR modifies .github/workflows/lint.yml, which is under the .github/ protected path. The PR has no linked issue providing authorization for modifying governance/infrastructure files. Human approval is always required for protected-path changes.

Suggested fix: File an issue documenting the need to add pytest and gitlint-core to the CI lint workflow, and link it to this PR.


- name: Install lychee
run: |
Expand Down
1 change: 1 addition & 0 deletions .gitlint
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[general]
# Enable conventional commits title check
contrib=CT1
extra-path=gitlint_rules/

[title-max-length]
line-length=100
Expand Down
11 changes: 11 additions & 0 deletions COMMITS.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,17 @@ Apply the same discipline to `fix` — bumping a dependency version is `chore`,

The parenthesized scope is optional but encouraged. Use it to identify the subsystem: `feat(appsetup)`, `fix(mint)`, `docs(adr)`, `chore(ci)`. When fixing a specific issue, prefer the issue number as scope: `fix(#123): ...`.

### Forbidden type + scope combinations

Some type/scope pairs are **enforced as errors** by gitlint (rule `UC1`). These combinations mislead users by putting infrastructure changes in user-facing release-note sections.

| Forbidden | Why | Use instead |
|---|---|---|
| `fix(ci)` | CI changes are not user-visible bug fixes | `ci(<subsystem>)` |
| `feat(ci)` | CI changes are not user-visible features | `ci(<subsystem>)` |
| `fix(e2e)` | E2E test changes are not user-visible bug fixes | `ci(e2e)` |
| `feat(e2e)` | E2E test changes are not user-visible features | `ci(e2e)` |

## Breaking changes

Breaking changes **must** be marked in both commit messages and PR titles. GoReleaser builds release notes from merged PR titles (`use: github` in `.goreleaser.yml`), so an unmarked PR title means the breaking change is invisible to users reading the release notes. This has caused real incidents — users upgraded with no warning that their agents would stop working.
Expand Down
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ script-test:
$(call run-timed,bash internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh)
$(call run-timed,python3 internal/scaffold/fullsend-repo/scripts/process-fix-result-test.py)
$(call run-timed,python3 skills/topissues/scripts/topissues_test.py)
$(call run-timed,python3 -m pytest gitlint_rules_test.py -v)
Comment thread
qodo-code-review[bot] marked this conversation as resolved.

test: lint-all go-test script-test lint-eval-cases

Expand Down
51 changes: 51 additions & 0 deletions gitlint_rules/forbidden_type_scope.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
"""Gitlint rule that forbids misleading type+scope combinations.

Types like ``feat`` and ``fix`` appear in user-facing release notes.
Scopes like ``ci`` and ``e2e`` describe infrastructure, not user-visible
changes. Using them together pollutes the release notes with entries
that mean nothing to end users.
"""

import re

from gitlint.rules import CommitRule, RuleViolation

# Pattern: type(scope): description
_CONVENTIONAL = re.compile(r"^(?P<type>\w+)\((?P<scope>[^)]+)\)")

Comment on lines +13 to +15

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Informational

2. Regex misses colon delimiter 🐞 Bug ≡ Correctness

ForbiddenTypeScope’s regex matches type(scope) without requiring the documented : delimiter,
so UC1 can emit its “pollutes release notes” message on malformed/non-conventional titles that
happen to start with fix(ci)/feat(ci) etc.
Agent Prompt
### Issue description
`_CONVENTIONAL` is documented as matching `type(scope): description`, but the actual regex only matches `type(scope)` and does not require `:` (or whitespace after it). This can cause UC1 to run (and emit a misleading message) even when the title does not follow the documented Conventional Commits format.

### Issue Context
The repo’s commit format documentation explicitly requires `:<space>` after the scope.

### Fix Focus Areas
- gitlint_rules/forbidden_type_scope.py[13-15]
- COMMITS.md[5-13]
- gitlint_rules_test.py[26-40]

### Suggested fix
Update the regex to require the delimiter, and optionally support breaking markers:
- e.g. `re.compile(r"^(?P<type>\w+)\((?P<scope>[^)]+)\)!?:\s")`

Add a test case demonstrating that malformed titles (e.g. `fix(ci) update workflow` without `:`) do not trigger UC1.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

# Map of (type, scope) -> suggested replacement.
# Scope matches are case-insensitive.
_FORBIDDEN = {
("feat", "ci"): "ci(<subsystem>)",
("fix", "ci"): "ci(<subsystem>)",
("feat", "e2e"): "ci(e2e)",
("fix", "e2e"): "ci(e2e)",
}


class ForbiddenTypeScope(CommitRule):
name = "forbidden-type-scope"
id = "UC1"

def validate(self, commit):
title = commit.message.title
m = _CONVENTIONAL.match(title)
if not m:
return []

ctype = m.group("type").lower()
scope = m.group("scope").lower()
key = (ctype, scope)

if key not in _FORBIDDEN:
return []

suggestion = _FORBIDDEN[key]
return [
RuleViolation(
self.id,
f'"{ctype}({scope})" pollutes release notes with non-user-facing changes. '
f'Use "{suggestion}: ..." instead.',
line_nr=1,
)
]
87 changes: 87 additions & 0 deletions gitlint_rules_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
"""Tests for the ForbiddenTypeScope gitlint rule."""

import pytest
from gitlint.rules import RuleViolation

from gitlint_rules.forbidden_type_scope import ForbiddenTypeScope


class FakeCommit:
"""Minimal stand-in for a gitlint commit object."""

def __init__(self, title):
self.message = type("msg", (), {"title": title})()


def run_rule(title):
"""Run ForbiddenTypeScope against a commit title and return violations."""
rule = ForbiddenTypeScope()
commit = FakeCommit(title)
return rule.validate(commit)


# --- should be rejected ---


@pytest.mark.parametrize(
"title",
[
"fix(ci): update workflow",
"feat(ci): add new job",
"fix(e2e): repair test",
"feat(e2e): add new test",
],
)
def test_rejects_forbidden_combinations(title):
violations = run_rule(title)
assert violations, f"Expected violation for {title!r}"
assert len(violations) == 1
assert isinstance(violations[0], RuleViolation)


def test_fix_ci_suggests_ci_subsystem():
violations = run_rule("fix(ci): update workflow")
assert "ci(<subsystem>)" in violations[0].message.lower()


def test_feat_e2e_suggests_ci_e2e():
violations = run_rule("feat(e2e): add new test")
assert "ci(e2e)" in violations[0].message.lower()


# --- should be allowed ---


@pytest.mark.parametrize(
"title",
[
"ci(lint): update linter config",
"ci(e2e): fix flaky test",
"fix(mint): correct token refresh",
"feat(review-agent): add outcome labels",
"chore(ci): bump action version",
"test(e2e): add new scenario",
"refactor(ci): simplify matrix",
"docs: update readme",
"fix(#123): handle nil pointer",
],
)
def test_allows_valid_combinations(title):
violations = run_rule(title)
assert not violations, f"Unexpected violation for {title!r}: {violations}"


# --- should not crash on non-conventional titles ---


@pytest.mark.parametrize(
"title",
[
"just a plain message",
"WIP",
"",
],
)
def test_ignores_non_conventional(title):
violations = run_rule(title)
assert not violations
Loading