From 8196b07b4cce8926c230c557dec3c4e16ff0f846 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=8D=E8=AE=B8=E5=AF=B9=E6=88=91=E7=8B=97=E5=8F=AB?= =?UTF-8?q?=E4=B8=B6?= <77058925+guqihao-7@users.noreply.github.com> Date: Wed, 24 Jun 2026 17:47:03 +0800 Subject: [PATCH] Add option to merge system and user review rules (#161) * 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 --- README.ja-JP.md | 4 +- README.ko-KR.md | 4 +- README.md | 4 +- README.ru-RU.md | 4 +- README.zh-CN.md | 4 +- internal/config/rules/system_rules.go | 84 +++++--- internal/config/rules/system_rules_test.go | 198 ++++++++++++++++++ .../skills/open-code-review/SKILL.md | 5 +- skills/open-code-review/SKILL.md | 5 +- 9 files changed, 272 insertions(+), 40 deletions(-) diff --git a/README.ja-JP.md b/README.ja-JP.md index d2cc1a5..6d26c7e 100644 --- a/README.ja-JP.md +++ b/README.ja-JP.md @@ -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`の場合、一致した組み込みシステムルールがこのユーザールールとマージされます。 - 各層の中では、ルールは宣言順に評価されます — 最初にマッチしたものが採用されます。 - ルールファイルが存在しない場合は、何も出力せずスキップされます。 diff --git a/README.ko-KR.md b/README.ko-KR.md index 28e8323..88b077a 100644 --- a/README.ko-KR.md +++ b/README.ko-KR.md @@ -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이 없으면 조용히 건너뜁니다. diff --git a/README.md b/README.md index 574def5..d843d80 100644 --- a/README.md +++ b/README.md @@ -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. diff --git a/README.ru-RU.md b/README.ru-RU.md index 2c27f0e..6f947fb 100644 --- a/README.ru-RU.md +++ b/README.ru-RU.md @@ -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`, совпавшее встроенное системное правило объединяется с этим пользовательским правилом. - Внутри каждого уровня правила проверяются в порядке объявления — побеждает первое совпадение. - Если файл правил не существует, он молча пропускается. diff --git a/README.zh-CN.md b/README.zh-CN.md index 6a430ef..0da8a1e 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -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` 时，命中的内置系统规则会与该用户规则合并；否则用户规则会替换系统规则。 - 在每一层内，规则按声明顺序评估 —— 首次匹配生效。 - 如果规则文件不存在，将被静默跳过。 diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index fe0780b..84de415 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -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 /.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 } } } diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index e99391a..5731ebf 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -249,6 +249,36 @@ func TestNewResolver_ProjectRuleHighestPriority(t *testing.T) { } } +func TestNewResolver_ProjectRuleFirstMatchWinsWithinFile(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + // Project rule file: + // /.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: + // /.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: + // /.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: + // /.opencodereview/rule.json + // Custom rule file: + // /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: + // /.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() diff --git a/plugins/open-code-review/skills/open-code-review/SKILL.md b/plugins/open-code-review/skills/open-code-review/SKILL.md index d81dc4d..814d328 100644 --- a/plugins/open-code-review/skills/open-code-review/SKILL.md +++ b/plugins/open-code-review/skills/open-code-review/SKILL.md @@ -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", diff --git a/skills/open-code-review/SKILL.md b/skills/open-code-review/SKILL.md index ff61473..843f217 100644 --- a/skills/open-code-review/SKILL.md +++ b/skills/open-code-review/SKILL.md @@ -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",