feat: Add multi scope lint support#19
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds multi-scope parsing and validation to commitlint-scope via a new configurable ChangesMulti-Scope Parsing and Validation
Sequence DiagramsequenceDiagram
participant CommitMessage
participant ScopeParser
participant FindOutsiders
participant OutsiderFinder
CommitMessage->>ScopeParser: Parse(message)
ScopeParser->>ScopeParser: Extract regex group
ScopeParser->>ScopeParser: Split by separator<br/>(comma, pipe, etc.)
ScopeParser->>ScopeParser: Trim and filter<br/>empty entries
ScopeParser-->>FindOutsiders: Return []string scopes
FindOutsiders->>OutsiderFinder: Find(scopes[0])
OutsiderFinder-->>FindOutsiders: Return candidates
FindOutsiders->>FindOutsiders: Intersect with<br/>Find(scopes[1], ...N)
FindOutsiders-->>CommitMessage: Return intersected<br/>outsiders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 Hi, check manually please. Be worry about release correctness |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details
gh pr view 19 --repo thumbrise/commitlint-scope --json title,body,baseRefName,headRefName,files,labels,state,mergeStateStatus,mergeable
echo "---FILES CHANGED---"
gh pr diff 19 --repo thumbrise/commitlint-scope --name-only
echo "---PR CHECKS---"
gh pr checks 19 --repo thumbrise/commitlint-scope 2>/dev/null || echo "No checks info"Length of output: 2479 🏁 Script executed: #!/bin/bash
# Inspect key release-related files
echo "=== package.json ==="
cat package.json 2>/dev/null || echo "Not found"
echo "=== CHANGELOG ==="
cat CHANGELOG.md 2>/dev/null || echo "Not found"
echo "=== .releaserc / release.config.js / semantic-release config ==="
cat .releaserc 2>/dev/null || cat .releaserc.json 2>/dev/null || cat .releaserc.js 2>/dev/null || cat release.config.js 2>/dev/null || echo "Not found"
echo "=== .github/workflows ==="
ls .github/workflows/ 2>/dev/null || echo "No workflows dir"Length of output: 3825 🏁 Script executed: #!/bin/bash
# Check workflow files and any version/release related config
for f in $(ls .github/workflows/ 2>/dev/null); do
echo "=== .github/workflows/$f ==="
cat ".github/workflows/$f"
echo ""
done
echo "=== package.json scripts and version ==="
cat package.json | python -c "import sys,json; d=json.load(sys.stdin); print('version:', d.get('version')); print('scripts:', json.dumps(d.get('scripts',{}), indent=2)); print('release config:', json.dumps(d.get('release',{}), indent=2))"Length of output: 4593 🏁 Script executed: #!/bin/bash
# Check recent git tags and commits on the PR branch
echo "=== Recent tags ==="
git tag --sort=-version:refname | head -20
echo "=== Recent commits on branch ==="
git log --oneline -15
echo "=== PR branch vs main diff (key files) ==="
git diff origin/main...HEAD -- package.json CHANGELOG.md 2>/dev/null | head -100Length of output: 1312 🏁 Script executed: #!/bin/bash
echo "=== .goreleaser.yml / .goreleaser.yaml ==="
cat .goreleaser.yml 2>/dev/null || cat .goreleaser.yaml 2>/dev/null || echo "Not found"
echo "=== go.mod ==="
cat go.mod
echo "=== Full PR diff ==="
gh pr diff 19 --repo thumbrise/commitlint-scopeLength of output: 24731 🏁 Script executed: #!/bin/bash
echo "=== release-template.hbs ==="
cat release-template.hbs 2>/dev/null || echo "Not found"
echo "=== .commitlint-scope.yaml ==="
cat .commitlint-scope.yaml 2>/dev/null || echo "Not found"
echo "=== commitlint.config.js ==="
cat commitlint.config.js 2>/dev/null || cat commitlint.config.cjs 2>/dev/null || echo "Not found"
echo "=== All semver-related: package-lock.json or npm-shrinkwrap.json check ==="
ls -la package-lock.json npm-shrinkwrap.json 2>/dev/null || echo "No lockfile found"Length of output: 2186
Release Correctness Review — PR
|
| Item | Status |
|---|---|
| All CI checks (Lint, Test, commitlint, commitlint-scope) | ✅ Pass |
package-lock.json present for npm ci |
✅ Present |
semantic-release config (release.config.js) |
✅ Correct |
goreleaser config (version: 2) |
✅ Present |
| Docker login secrets configured in workflow | DOCKERHUB_USERNAME/DOCKERHUB_TOKEN being set |
🔴 Critical: Double GitHub Release
File: release.config.js + .goreleaser.yaml
release.config.js includes @semantic-release/github which creates a GitHub Release, and then immediately after, goreleaser runs via @semantic-release/exec and also tries to create a GitHub Release (the release: key in .goreleaser.yaml is empty/default, meaning goreleaser will attempt to publish its own release).
# .goreleaser.yaml
release:
# empty = goreleaser creates/publishes a GitHub release// release.config.js
'`@semantic-release/github`', // ← creates GitHub release `#1`
['`@semantic-release/exec`', {
publishCmd: '... goreleaser release ...' // ← goreleaser creates GitHub release `#2`
}]Risk: goreleaser may fail with a conflict on the already-created release, or produce two releases with different assets.
Fix options (pick one):
- Remove
@semantic-release/githuband let goreleaser be the sole publisher:-'`@semantic-release/github`', - Or disable the GitHub release in goreleaser and keep
@semantic-release/github:# .goreleaser.yaml release: disable: true
🟠 Logic Bug: findOutsiders Uses Union Instead of Intersection
File: pkg/validator/validator.go, findOutsiders function
For a multi-scope commit like feat(api, db): ... touching api/handler.go, db/schema.sql, core/other.go:
Find("api", files)returns[db/schema.sql, core/other.go]— files not matchingapi/**Find("db", files)returns[api/handler.go, core/other.go]— files not matchingdb/**- Current (union):
[db/schema.sql, core/other.go, api/handler.go]← incorrectly flags valid files - Correct (intersection):
[core/other.go]← only files matching NO scope
The current implementation will false-positive on files that are valid for one scope but not another.
// Current — WRONG for multi-scope
func findOutsiders(finder OutsiderFinder, scopes []string, files []string) []Outsider {
seen := make(map[string]bool)
var allOutsiders []Outsider
for _, scope := range scopes {
for _, o := range finder.Find(scope, files) { // union of outsiders
if !seen[o.File] {
seen[o.File] = true
allOutsiders = append(allOutsiders, o)
}
}
}
return allOutsiders
}The correct approach is to keep only files that are outsiders for all scopes:
func findOutsiders(finder OutsiderFinder, scopes []string, files []string) []Outsider {
if len(scopes) == 0 {
return nil
}
// Start with the full outsider set for the first scope
outsidersByFile := make(map[string]Outsider)
for _, o := range finder.Find(scopes[0], files) {
outsidersByFile[o.File] = o
}
// Intersect with outsiders from remaining scopes
for _, scope := range scopes[1:] {
scopeOutsiders := make(map[string]bool)
for _, o := range finder.Find(scope, files) {
scopeOutsiders[o.File] = true
}
for file := range outsidersByFile {
if !scopeOutsiders[file] {
delete(outsidersByFile, file) // valid for this scope → not an outsider
}
}
}
result := make([]Outsider, 0, len(outsidersByFile))
for _, o := range outsidersByFile {
result = append(result, o)
}
return result
}🟡 Test Masks the Bug
File: pkg/validator/validator_test.go, SetupExpectations
The mock returns the same pre-computed outsiders for every scope call:
for _, scope := range c.scopes {
outsider.EXPECT().Find(scope, c.files).Return(outsiders) // same list for all scopes
}This means the test never exercises the union-vs-intersection difference. It would pass with either the current (wrong) or the corrected implementation. An integration-level test with the real OutsiderFinder is needed to catch this.
ℹ️ First Release Will Be v1.0.0
There are no existing git tags. semantic-release will scan the full commit history, find feat!: Scopes-Patterns matrix support... (a breaking change), and produce version 1.0.0. This is likely intentional given package.json has "version": "1.0.0", but worth confirming.
Summary
| Issue | Severity | Blocks Release? |
|---|---|---|
Double GitHub Release (@semantic-release/github + goreleaser) |
🔴 Critical | Yes — likely |
findOutsiders union vs intersection logic bug |
🟠 High | No, but ships broken feature |
| Test doesn't catch the logic bug | 🟡 Medium | No |
First release = v1.0.0 (no tags) |
ℹ️ Info | Intentional? |
I'd recommend blocking merge until at least the double-release issue is resolved, and ideally the findOutsiders logic is fixed too.
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
Good to hear the 🐇 ˶ᵔ ᵕ ᵔ˶ ✏️ Learnings added
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/validator/validator.go (2)
193-196: ⚡ Quick winSort outsider results before returning for deterministic output.
findOutsidersbuildsresultfrom a map, so output order is nondeterministic. Stable ordering avoids flaky assertions and inconsistent CLI/JSON output.Proposed patch
import ( "context" "errors" "fmt" "log/slog" + "sort" ) @@ result := make([]Outsider, 0, len(outsidersByFile)) for _, o := range outsidersByFile { result = append(result, o) } + sort.Slice(result, func(i, j int) bool { + return result[i].File < result[j].File + }) return result }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/validator/validator.go` around lines 193 - 196, findOutsiders currently builds result from the map outsidersByFile which yields nondeterministic order; fix by sorting the result slice before returning. After the loop that appends to result in function findOutsiders, call sort.Slice (importing sort) and provide a comparator that orders Outsider entries deterministically (e.g., first by File/path field and then by Name/identifier field) so output is stable for CLI/JSON and tests.
79-79: Make outsider ordering deterministic (and separator defaulting is already consistent)
NewDefaultScopeParsernormalizes an emptycfg.ScopeSeparatorto",", soNewValidator(Config{}, ...)won’t diverge fromLoadConfigfor separator defaulting. In contrast,findOutsidersreturnsOutsidersby ranging overoutsidersByFile(map iteration), so theOutsidersslice order can vary—sort byOutsider.Filebefore returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/validator/validator.go` at line 79, The findOutsiders function builds an Outsiders slice by ranging over outsidersByFile (a map), producing nondeterministic order; after assembling the []Outsider, sort it deterministically by the Outsider.File field (e.g., using sort.Slice or sort.SliceStable) before returning so callers of NewValidator/LoadConfig get stable ordering; update findOutsiders to perform this sort on the resulting Outsiders slice.docs/schema/config.v3.json (1)
13-16: ⚡ Quick winAdd an explicit schema default for
scopeSeparator.The description says the default is
",", but the schema doesn’t declare it. Add"default": ","so schema consumers/editors can apply it consistently.Suggested patch
"scopeSeparator": { "description": "Character used to separate multiple scopes within the parentheses. Default: ','. Example: 'feat(api, db): message' with separator ',' yields scopes ['api', 'db'].", - "type": "string" + "type": "string", + "default": "," },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/schema/config.v3.json` around lines 13 - 16, The schema property "scopeSeparator" currently documents a default of "," but doesn't declare it; update the JSON schema entry for scopeSeparator (the object keyed "scopeSeparator") by adding "default": "," alongside its "description" and "type" so editors and tooling can apply the documented default value consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/validator/config_test.go`:
- Around line 121-123: The test only asserts cfg.ScopeSeparator when
tt.wantSeparator is non-empty, letting regressions to the default separator
slip; update the test in config_test.go to set a default expected separator
(",") for success cases and assert cfg.ScopeSeparator unconditionally for
non-error cases by using tt.wantSeparator = "," when tt.wantSeparator == "" (or
otherwise treating empty as the default) and then calling the existing
assert.Equal(t, tt.wantSeparator, cfg.ScopeSeparator) for each non-failing test;
reference tt.wantSeparator and cfg.ScopeSeparator to locate and change the test
logic.
In `@README.md`:
- Around line 125-126: Clarify that multiple scopes in a single commit are
treated as alternatives (OR), not conjunctions: update the sentence that
currently reads "Multiple scopes in a single commit are resolved independently —
`feat(api, db): ...` will check files against patterns for both `api` and `db`."
to explicitly state that a file is accepted if it matches any one of the listed
scopes and only flagged if it is outside all parsed scopes; also keep the note
about `scopeSeparator` for alternate separators. Mention the example `feat(api,
db): ...` and the `scopeSeparator` config key to help readers find the relevant
behavior.
---
Nitpick comments:
In `@docs/schema/config.v3.json`:
- Around line 13-16: The schema property "scopeSeparator" currently documents a
default of "," but doesn't declare it; update the JSON schema entry for
scopeSeparator (the object keyed "scopeSeparator") by adding "default": ","
alongside its "description" and "type" so editors and tooling can apply the
documented default value consistently.
In `@pkg/validator/validator.go`:
- Around line 193-196: findOutsiders currently builds result from the map
outsidersByFile which yields nondeterministic order; fix by sorting the result
slice before returning. After the loop that appends to result in function
findOutsiders, call sort.Slice (importing sort) and provide a comparator that
orders Outsider entries deterministically (e.g., first by File/path field and
then by Name/identifier field) so output is stable for CLI/JSON and tests.
- Line 79: The findOutsiders function builds an Outsiders slice by ranging over
outsidersByFile (a map), producing nondeterministic order; after assembling the
[]Outsider, sort it deterministically by the Outsider.File field (e.g., using
sort.Slice or sort.SliceStable) before returning so callers of
NewValidator/LoadConfig get stable ordering; update findOutsiders to perform
this sort on the resulting Outsiders slice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa22d30e-5796-47e7-9f33-0294ab052e23
📒 Files selected for processing (12)
.commitlint-scope.yaml.mockery.ymlREADME.mdcmd/commands/init.godocs/schema/config.v3.jsonpkg/validator/config.gopkg/validator/config_test.gopkg/validator/mocks_test.gopkg/validator/scope_parser.gopkg/validator/scope_parser_test.gopkg/validator/validator.gopkg/validator/validator_test.go
- Sort findOutsiders result by File (non-deterministic map iteration) - Assert ScopeSeparator unconditionally in config tests - Rewrite README multi-scope section with explicit intersection semantics
thumbrise
force-pushed
the
multiscope-lint
branch
from
ba83fa0 to
b7f9e68
Compare
|
🎉 This PR is included in version 3.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
1 participant
Summary by CodeRabbit
Release Notes
New Features
scopeSeparatoroption to specify the delimiter between multiple scopes in commit messages. Defaults to comma.Documentation
scopeSeparatorsetting and examples for commits with multiple scopes.