vrr/open-code-review
2
0
Fork 0
mirror of https://github.com/alibaba/open-code-review.git synced 2026-07-09 17:28:58 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Add option to merge system and user review rules (#161)

Browse source 
* feat: add a command param to choose whether append user rule after system rule

* feat: add some unit tests

* fix: remove unused merge system rule agent arg

* feat: Adjust the rule concatenation logic

* fix: fix typo

* test: make merge system rule tests behavior-focused

* test: update the param usage

* test: add param introduction

* test: remove unnecessary blank lines

* feat: support merged rule details in rules check

* feat: add some unit tests

* feat: support per-rule system rule merging

* fix: consider corner case

* fix: preserve first matching rule entry

* Adjust project rule detail argument order
This commit is contained in:
不许对我狗叫丶 2026-06-24 17:47:03 +08:00 • committed by GitHub
parent fa030d4a1a
commit 8196b07b4c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 272 additions and 40 deletions
Download patch file Download diff file Expand all files Collapse all files

4
README.ja-JP.md
View file

@ -440,7 +440,8 @@ OCRは4層の優先度チェーンを使ってレビュールールを解決し
"rules": [
{
"path": "force-api/**/*.java",
"rule": "All new methods must validate required parameters for null values"
"rule": "All new methods must validate required parameters for null values",
"merge_system_rule": true
},
{
"path": "**/*mapper*.xml",
@ -451,6 +452,7 @@ OCRは4層の優先度チェーンを使ってレビュールールを解決し
```
- `path`は`**`による再帰マッチと`{java,kt}`のブレース展開をサポートします。
- `merge_system_rule`は任意です。`true`の場合、一致した組み込みシステムルールがこのユーザールールとマージされます。
- 各層の中では、ルールは宣言順に評価されます — 最初にマッチしたものが採用されます。
- ルールファイルが存在しない場合は、何も出力せずスキップされます。

4
README.ko-KR.md
View file

@ -440,7 +440,8 @@ OCR은 네 계층의 priority chain으로 review rule을 해석합니다. 각
"rules": [
{
"path": "force-api/**/*.java",
"rule": "All new methods must validate required parameters for null values"
"rule": "All new methods must validate required parameters for null values",
"merge_system_rule": true
},
{
"path": "**/*mapper*.xml",
@ -451,6 +452,7 @@ OCR은 네 계층의 priority chain으로 review rule을 해석합니다. 각
```
- `path`는 `**` recursive matching과 `{java,kt}` brace expansion을 지원합니다.
- `merge_system_rule`은 optional입니다. `true`이면 매칭된 built-in system rule을 이 user rule과 병합합니다.
- 각 계층 안에서는 rule이 선언 순서대로 평가되며 첫 번째 match가 선택됩니다.
- rule file이 없으면 조용히 건너뜁니다.

4
README.md
View file

@ -442,7 +442,8 @@ Layers 1–3 share the same JSON format:
"rules": [
{
"path": "force-api/**/*.java",
"rule": "All new methods must validate required parameters for null values"
"rule": "All new methods must validate required parameters for null values",
"merge_system_rule": true
},
{
"path": "**/*mapper*.xml",
@ -453,6 +454,7 @@ Layers 1–3 share the same JSON format:
```
- `path` supports `**` recursive matching and `{java,kt}` brace expansion.
- `merge_system_rule` is optional. When `true`, the matched built-in system rule is merged with this user rule; otherwise the user rule replaces the system rule.
- Within each layer, rules are evaluated in declaration order — the first match wins.
- If a rule file does not exist, it is silently skipped.

4
README.ru-RU.md
View file

@ -442,7 +442,8 @@ OCR разрешает правила ревью по цепочке приор
"rules": [
{
"path": "force-api/**/*.java",
"rule": "Все новые методы должны проверять обязательные параметры на null"
"rule": "Все новые методы должны проверять обязательные параметры на null",
"merge_system_rule": true
},
{
"path": "**/*mapper*.xml",
@ -453,6 +454,7 @@ OCR разрешает правила ревью по цепочке приор
```
- `path` поддерживает рекурсивное сопоставление `**` и расширение фигурных скобок `{java,kt}`.
- `merge_system_rule` необязателен. Если указано `true`, совпавшее встроенное системное правило объединяется с этим пользовательским правилом.
- Внутри каждого уровня правила проверяются в порядке объявления — побеждает первое совпадение.
- Если файл правил не существует, он молча пропускается.

4
README.zh-CN.md
View file

@ -430,7 +430,8 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则
"rules": [
{
"path": "force-api/**/*.java",
"rule": "所有新方法必须对必填参数进行空值校验"
"rule": "所有新方法必须对必填参数进行空值校验",
"merge_system_rule": true
},
{
"path": "**/*mapper*.xml",
@ -441,6 +442,7 @@ OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则
```
- `path` 支持 `**` 递归匹配和 `{java,kt}` 大括号展开。
- `merge_system_rule` 为可选字段。设为 `true` 时，命中的内置系统规则会与该用户规则合并；否则用户规则会替换系统规则。
- 在每一层内，规则按声明顺序评估 —— 首次匹配生效。
- 如果规则文件不存在，将被静默跳过。

84
internal/config/rules/system_rules.go
View file

@ -178,8 +178,9 @@ func expandBraces(s string) []string {
// ProjectRuleEntry is a single entry in .opencodereview/rule.json.
type ProjectRuleEntry struct {
Path string `json:"path"`
Rule string `json:"rule"`
Path string `json:"path"`
Rule string `json:"rule"`
MergeSystemRule bool `json:"merge_system_rule,omitempty"`
}
// ProjectRule holds rules loaded from <repoDir>/.opencodereview/rule.json.
@ -279,7 +280,12 @@ func NewResolver(repoDir, customRulePath string) (Resolver, *FileFilter, error)
filter := buildFileFilter(customRule, projectRule, globalRule)
return &composedResolver{custom: customRule, project: projectRule, global: globalRule, system: sysRule}, filter, nil
return &composedResolver{
custom: customRule,
project: projectRule,
global: globalRule,
system: sysRule,
}, filter, nil
}
// buildFileFilter picks the highest-priority layer that has any include/exclude
@ -352,63 +358,75 @@ func loadProjectRule(repoDir string) (*ProjectRule, error) {
return &pr, nil
}
// Resolve checks each layer in priority order; first match wins.
// Resolve checks each layer in priority order; first match wins. User rules
// replace the system rule by default; rules with merge_system_rule keep the
// matched system rule alongside the user rule.
func (c *composedResolver) Resolve(path string) string {
if rule := matchProjectRule(c.custom, path); rule != "" {
return rule
}
if rule := matchProjectRule(c.project, path); rule != "" {
return rule
}
if rule := matchProjectRule(c.global, path); rule != "" {
return rule
for _, layer := range []*ProjectRule{c.custom, c.project, c.global} {
if entry := matchProjectRuleEntry(layer, path); entry != nil {
if entry.MergeSystemRule {
return c.mergeWithSystemRule(path, entry.Rule)
}
return entry.Rule
}
}
return c.system.Resolve(path)
}
func (c *composedResolver) mergeWithSystemRule(path, rule string) string {
systemRule := c.system.Resolve(path)
if systemRule == "" {
return rule
}
return "## System-Specific Rules (Mandatory)\n\n" +
systemRule +
"\n\n---\n\n" +
"## User-Specific Rules (Mandatory)\n\n" +
rule
}
// ResolveDetail returns the matched rule along with its source layer and pattern.
// When a user rule sets merge_system_rule, Rule contains the merged system+user
// rule text while Source and Pattern still describe the user rule that won the
// priority chain.
func (c *composedResolver) ResolveDetail(path string) RuleDetail {
if detail := matchProjectRuleDetail(c.custom, path, "custom"); detail != nil {
if detail := c.matchProjectRuleDetail(c.custom, path, "custom"); detail != nil {
return *detail
}
if detail := matchProjectRuleDetail(c.project, path, "project"); detail != nil {
if detail := c.matchProjectRuleDetail(c.project, path, "project"); detail != nil {
return *detail
}
if detail := matchProjectRuleDetail(c.global, path, "global"); detail != nil {
if detail := c.matchProjectRuleDetail(c.global, path, "global"); detail != nil {
return *detail
}
return c.system.resolveDetail(path)
}
func matchProjectRule(pr *ProjectRule, path string) string {
if pr == nil {
return ""
func (c *composedResolver) matchProjectRuleDetail(pr *ProjectRule, path string, source string) *RuleDetail {
entry := matchProjectRuleEntry(pr, path)
if entry == nil {
return nil
}
lowerPath := strings.ToLower(path)
for _, entry := range pr.Rules {
expanded := expandBraces(entry.Path)
for _, p := range expanded {
if matched, _ := doublestar.Match(strings.ToLower(p), lowerPath); matched {
return entry.Rule
}
}
rule := entry.Rule
if entry.MergeSystemRule {
rule = c.mergeWithSystemRule(path, rule)
}
return ""
return &RuleDetail{Rule: rule, Source: source, Pattern: entry.Path}
}
func matchProjectRuleDetail(pr *ProjectRule, path, source string) *RuleDetail {
func matchProjectRuleEntry(pr *ProjectRule, path string) *ProjectRuleEntry {
if pr == nil {
return nil
}
lowerPath := strings.ToLower(path)
for _, entry := range pr.Rules {
if entry.Rule == "" {
continue
}
for i := range pr.Rules {
entry := &pr.Rules[i]
expanded := expandBraces(entry.Path)
for _, p := range expanded {
if matched, _ := doublestar.Match(strings.ToLower(p), lowerPath); matched {
return &RuleDetail{Rule: entry.Rule, Source: source, Pattern: entry.Path}
return entry
}
}
}

198
internal/config/rules/system_rules_test.go
View file

@ -249,6 +249,36 @@ func TestNewResolver_ProjectRuleHighestPriority(t *testing.T) {
}
}
func TestNewResolver_ProjectRuleFirstMatchWinsWithinFile(t *testing.T) {
t.Setenv("HOME", t.TempDir())
// Project rule file:
// <repo>/.opencodereview/rule.json
// Path under test:
// internal/config/rules/system_rules.go -> matches both project entries.
// This verifies declaration order inside one JSON rule file: the first
// matching entry wins even when a later entry is more specific.
dir := t.TempDir()
ocrDir := filepath.Join(dir, ".opencodereview")
if err := os.MkdirAll(ocrDir, 0o755); err != nil {
t.Fatalf("mkdir: %v", err)
}
ruleJSON := `{"rules":[{"path":"internal/**/*.go","rule":"first-go-rule"},{"path":"internal/config/**/*.go","rule":"second-config-rule"}]}`
if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(ruleJSON), 0o644); err != nil {
t.Fatalf("write rule.json: %v", err)
}
resolver, _, err := NewResolver(dir, "")
if err != nil {
t.Fatalf("NewResolver: %v", err)
}
got := resolver.Resolve("internal/config/rules/system_rules.go")
if got != "first-go-rule" {
t.Fatalf("expected first matching project rule, got %q", got)
}
}
func TestNewResolver_ProjectRuleFallsBackToSystem(t *testing.T) {
dir := t.TempDir()
ocrDir := filepath.Join(dir, ".opencodereview")
@ -295,6 +325,128 @@ func TestNewResolver_CustomRuleOverridesDefault(t *testing.T) {
}
}
func TestNewResolver_ProjectRuleReplacesSystemRuleByDefault(t *testing.T) {
t.Setenv("HOME", t.TempDir())
// Project rule file:
// <repo>/.opencodereview/rule.json
// Path under test:
// main.go -> matches the project **/*.go rule.
// This verifies the default behavior: a user rule replaces the system rule
// unless the matched rule entry opts into merge_system_rule.
dir := t.TempDir()
ocrDir := filepath.Join(dir, ".opencodereview")
if err := os.MkdirAll(ocrDir, 0o755); err != nil {
t.Fatalf("mkdir: %v", err)
}
ruleJSON := `{"rules":[{"path":"**/*.go","rule":"project-go-rule"}]}`
if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(ruleJSON), 0o644); err != nil {
t.Fatalf("write rule.json: %v", err)
}
resolver, _, err := NewResolver(dir, "")
if err != nil {
t.Fatalf("NewResolver: %v", err)
}
got := resolver.Resolve("main.go")
if got != "project-go-rule" {
t.Fatalf("expected only project rule when merge is disabled, got %q", got)
}
}
func TestNewResolver_ProjectRuleMergesSystemRule(t *testing.T) {
t.Setenv("HOME", t.TempDir())
// Project rule file:
// <repo>/.opencodereview/rule.json
// Path under test:
// main.go -> matches both the system Go rule and the project **/*.go rule.
// This verifies merge_system_rule keeps both rules without depending on the
// exact merge markdown or ordering.
dir := t.TempDir()
ocrDir := filepath.Join(dir, ".opencodereview")
if err := os.MkdirAll(ocrDir, 0o755); err != nil {
t.Fatalf("mkdir: %v", err)
}
ruleJSON := `{"rules":[{"path":"**/*.go","rule":"project-go-rule","merge_system_rule":true}]}`
if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(ruleJSON), 0o644); err != nil {
t.Fatalf("write rule.json: %v", err)
}
resolver, _, err := NewResolver(dir, "")
if err != nil {
t.Fatalf("NewResolver: %v", err)
}
systemRule, err := LoadDefault()
if err != nil {
t.Fatalf("LoadDefault: %v", err)
}
wantSystemRule := systemRule.Resolve("main.go")
wantUserRule := "project-go-rule"
got := resolver.Resolve("main.go")
systemIdx := strings.Index(got, wantSystemRule)
if systemIdx < 0 {
t.Fatalf("expected merged system rule, got %q", truncate(got, 120))
}
userIdx := strings.Index(got, wantUserRule)
if userIdx < 0 {
t.Fatalf("expected merged project rule, got %q", truncate(got, 120))
}
}
func TestNewResolver_MergeSystemRuleKeepsRulePriority(t *testing.T) {
t.Setenv("HOME", t.TempDir())
// Project rule file:
// <repo>/.opencodereview/rule.json
// Custom rule file:
// <custom>/custom_rules.json, passed as --rule equivalent.
// Path under test:
// main.go -> matches custom main.go first, then project **/*.go.
// This verifies merging does not change layer priority: custom still wins.
repoDir := t.TempDir()
ocrDir := filepath.Join(repoDir, ".opencodereview")
if err := os.MkdirAll(ocrDir, 0o755); err != nil {
t.Fatalf("mkdir: %v", err)
}
projectRule := `{"rules":[{"path":"**/*.go","rule":"project-go-rule"}]}`
if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(projectRule), 0o644); err != nil {
t.Fatalf("write rule.json: %v", err)
}
customDir := t.TempDir()
customRule := `{"rules":[{"path":"main.go","rule":"custom-main-rule","merge_system_rule":true}]}`
customPath := filepath.Join(customDir, "custom_rules.json")
if err := os.WriteFile(customPath, []byte(customRule), 0o644); err != nil {
t.Fatalf("write custom rule: %v", err)
}
resolver, _, err := NewResolver(repoDir, customPath)
if err != nil {
t.Fatalf("NewResolver: %v", err)
}
systemRule, err := LoadDefault()
if err != nil {
t.Fatalf("LoadDefault: %v", err)
}
wantSystemRule := systemRule.Resolve("main.go")
got := resolver.Resolve("main.go")
if !strings.Contains(got, wantSystemRule) {
t.Fatalf("expected merged system rule, got %q", truncate(got, 120))
}
if !strings.Contains(got, "custom-main-rule") {
t.Fatalf("expected custom rule to win, got %q", truncate(got, 120))
}
if strings.Contains(got, "project-go-rule") {
t.Fatalf("project rule should not be merged when custom rule matches first, got %q", truncate(got, 120))
}
}
func TestNewResolver_CustomOverridesProject(t *testing.T) {
// Setup --rule file (highest priority)
customDir := t.TempDir()
@ -613,6 +765,52 @@ func TestResolveDetail_ProjectOverridesSystem(t *testing.T) {
}
}
func TestResolveDetail_MergeSystemRule(t *testing.T) {
t.Setenv("HOME", t.TempDir())
// Project rule file:
// <repo>/.opencodereview/rule.json
// Path under test:
// src/main/foo.java -> matches both the system Java rule and the project rule.
// This verifies ResolveDetail reports the matched user rule metadata while
// returning merged rule text when the entry sets merge_system_rule.
dir := t.TempDir()
ocrDir := filepath.Join(dir, ".opencodereview")
if err := os.MkdirAll(ocrDir, 0o755); err != nil {
t.Fatalf("mkdir: %v", err)
}
ruleJSON := `{"rules":[{"path":"src/**/*.java","rule":"project-java-rule","merge_system_rule":true}]}`
if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(ruleJSON), 0o644); err != nil {
t.Fatalf("write: %v", err)
}
resolver, _, err := NewResolver(dir, "")
if err != nil {
t.Fatalf("NewResolver: %v", err)
}
dr := resolver.(DetailResolver)
systemRule, err := LoadDefault()
if err != nil {
t.Fatalf("LoadDefault: %v", err)
}
wantSystemRule := systemRule.Resolve("src/main/foo.java")
detail := dr.ResolveDetail("src/main/foo.java")
if detail.Source != "project" {
t.Errorf("expected source 'project', got %q", detail.Source)
}
if detail.Pattern != "src/**/*.java" {
t.Errorf("expected pattern 'src/**/*.java', got %q", detail.Pattern)
}
if !strings.Contains(detail.Rule, wantSystemRule) {
t.Fatalf("expected merged system rule, got %q", truncate(detail.Rule, 120))
}
if !strings.Contains(detail.Rule, "project-java-rule") {
t.Fatalf("expected merged project rule, got %q", truncate(detail.Rule, 120))
}
}
func TestResolveDetail_CustomOverridesAll(t *testing.T) {
// Project rule
repoDir := t.TempDir()

5
plugins/open-code-review/skills/open-code-review/SKILL.md
View file

@ -186,6 +186,8 @@ If the user wants project-specific rules, OCR resolves them in this priority ord
3. `~/.opencodereview/rule.json`
4. Built-in system defaults (lowest)
By default, the first matching user rule replaces the built-in system rule. Set `merge_system_rule: true` on a rule entry when the matched system rule and user rule should both be included.
Rule file format:
```json
@ -193,7 +195,8 @@ Rule file format:
"rules": [
{
"path": "**/*.java",
"rule": "All new methods must validate required parameters for null"
"rule": "All new methods must validate required parameters for null",
"merge_system_rule": true
},
{
"path": "**/*mapper*.xml",

5
skills/open-code-review/SKILL.md
View file

@ -181,6 +181,8 @@ If the user wants project-specific rules, OCR resolves them in this priority ord
3. `~/.opencodereview/rule.json`
4. Built-in system defaults (lowest)
By default, the first matching user rule replaces the built-in system rule. Set `merge_system_rule: true` on a rule entry when the matched system rule and user rule should both be included.
Rule file format:
```json
@ -188,7 +190,8 @@ Rule file format:
"rules": [
{
"path": "**/*.java",
"rule": "All new methods must validate required parameters for null"
"rule": "All new methods must validate required parameters for null",
"merge_system_rule": true
},
{
"path": "**/*mapper*.xml",