From d44fd3198999eb398e56605cd0f0daa1315ceef3 Mon Sep 17 00:00:00 2001 From: kite Date: Mon, 25 May 2026 17:45:53 +0800 Subject: [PATCH] feat(rules): unify rule resolution behind Resolver interface with four-layer priority Consolidate review rules into a composedResolver that supports four layers (--rule flag > project .open-code-review/rule.json > global ~/rule.json > embedded system default), each with first-match-wins fall-through semantics. --- README.md | 34 ++++ README.zh-CN.md | 34 ++++ cmd/opencodereview/review_cmd.go | 24 +-- internal/agent/agent.go | 2 +- internal/config/rules/system_rules.go | 152 ++++++++++++++-- internal/config/rules/system_rules_test.go | 197 +++++++++++++++++++++ 6 files changed, 412 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 77b8103..e6ef98f 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,40 @@ ocr viewer ocr viewer --addr :3000 ``` +## Review Rules + +OCR resolves review rules using a four-layer priority chain. Each layer uses first-match-wins: if a file path matches a pattern, that rule is used; otherwise it falls through to the next layer. + +| Priority | Source | Path | Description | +|----------|--------|------|-------------| +| 1 (highest) | `--rule` flag | User-specified path | CLI explicit override | +| 2 | Project config | `/.open-code-review/rule.json` | Per-project rules, can be committed to git | +| 3 | Global config | `~/.open-code-review/rule.json` | User-wide personal preferences | +| 4 (lowest) | System default | Embedded `system_rules.json` | Built-in rules covering common languages and file types | + +### Rule File Format + +Layers 1–3 share the same JSON format: + +```json +{ + "rules": [ + { + "path": "force-api/**/*.java", + "rule": "All new methods must validate required parameters for null values" + }, + { + "path": "**/*mapper*.xml", + "rule": "Check SQL for injection risks, parameter errors, and missing closing tags" + } + ] +} +``` + +- `path` supports `**` recursive matching and `{java,kt}` brace expansion. +- Within each layer, rules are evaluated in declaration order — the first match wins. +- If a rule file does not exist, it is silently skipped. + ## Architecture The review agent follows a **three-phase workflow**: diff --git a/README.zh-CN.md b/README.zh-CN.md index f17afa6..87b3de7 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -143,6 +143,40 @@ ocr viewer ocr viewer --addr :3000 ``` +## 评审规则 + +OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则：如果文件路径匹配到某个模式，则使用该规则；否则穿透到下一层。 + +| 优先级 | 来源 | 路径 | 描述 | +|--------|------|------|------| +| 1（最高） | `--rule` 参数 | 用户指定路径 | CLI 显式覆盖 | +| 2 | 项目配置 | `/.open-code-review/rule.json` | 项目级规则，可提交到 git | +| 3 | 全局配置 | `~/.open-code-review/rule.json` | 用户级个人偏好 | +| 4（最低） | 系统默认 | 内嵌 `system_rules.json` | 覆盖常见语言和文件类型的内置规则 | + +### 规则文件格式 + +第 1–3 层使用相同的 JSON 格式： + +```json +{ + "rules": [ + { + "path": "force-api/**/*.java", + "rule": "所有新方法必须对必填参数进行空值校验" + }, + { + "path": "**/*mapper*.xml", + "rule": "检查 SQL 注入风险、参数错误和缺少闭合标签" + } + ] +} +``` + +- `path` 支持 `**` 递归匹配和 `{java,kt}` 大括号展开。 +- 在每一层内，规则按声明顺序评估 —— 首次匹配生效。 +- 如果规则文件不存在，将被静默跳过。 + ## 架构 审查 Agent 遵循**三阶段工作流**： diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index c99bac4..80bf979 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -40,15 +40,14 @@ func runReview(args []string) error { return fmt.Errorf("invalid config: %w", err) } - sysRule, err := rules.LoadDefault() + repoDir, err := resolveRepoDir(opts.repoDir) if err != nil { - return fmt.Errorf("load default system rule: %w", err) + return fmt.Errorf("resolve repo: %w", err) } - if opts.rulePath != "" { - sysRule, err = loadSystemRule(opts.rulePath) - if err != nil { - return fmt.Errorf("load system rule: %w", err) - } + + resolver, err := rules.NewResolver(repoDir, opts.rulePath) + if err != nil { + return fmt.Errorf("load rules: %w", err) } toolEntries, err := toolsconfig.Load(opts.toolConfigPath) @@ -58,11 +57,6 @@ func runReview(args []string) error { planToolDefs := agent.BuildToolDefs(toolEntries, true) mainToolDefs := agent.BuildToolDefs(toolEntries, false) - repoDir, err := resolveRepoDir(opts.repoDir) - if err != nil { - return fmt.Errorf("resolve repo: %w", err) - } - appCfg, err := LoadAppConfig(defaultConfigPath()) if err != nil { return fmt.Errorf("load app config: %w", err) @@ -97,7 +91,7 @@ func runReview(args []string) error { Commit: opts.commit, DiffMap: diffMap, Template: *tpl, - SystemRule: sysRule, + SystemRule: resolver, LLMClient: llmClient, Tools: tools, PlanToolDefs: planToolDefs, @@ -166,10 +160,6 @@ func runReview(args []string) error { return nil } -func loadSystemRule(path string) (*rules.SystemRule, error) { - return rules.LoadFile(path) -} - func resolveRepoDir(input string) (string, error) { if input == "" { var err error diff --git a/internal/agent/agent.go b/internal/agent/agent.go index dda8ca2..f6fd404 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -39,7 +39,7 @@ type Args struct { Template template.Template // SystemRule holds path-based review rules loaded from a JSON config. - SystemRule *rules.SystemRule + SystemRule rules.Resolver // LLM client for model inference. LLMClient llm.LLMClient diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index 6c5bb02..8370f8a 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -6,11 +6,17 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "strings" "github.com/bmatcuk/doublestar/v4" ) +// Resolver resolves a review rule for a file path. +type Resolver interface { + Resolve(path string) string +} + // PathRule is a single pattern→rule entry preserving declaration order. type PathRule struct { Pattern string @@ -86,19 +92,6 @@ func LoadDefault() (*SystemRule, error) { return &rule, nil } -// LoadFile parses a system_rules.json file from disk. -func LoadFile(path string) (*SystemRule, error) { - data, err := os.ReadFile(path) - if err != nil { - return nil, fmt.Errorf("read rule file %s: %w", path, err) - } - var rule SystemRule - if err := json.Unmarshal(data, &rule); err != nil { - return nil, fmt.Errorf("unmarshal rule file: %w", err) - } - return &rule, nil -} - // Resolve returns the rule text for a given file path. // Patterns with brace expansion like "*.{go,py}" are expanded into "*.go", "*.py". // The first match wins; if none match, it falls back to DefaultRule. @@ -140,3 +133,136 @@ func expandBraces(s string) []string { } return results } + +// ProjectRuleEntry is a single entry in .open-code-review/rule.json. +type ProjectRuleEntry struct { + Path string `json:"path"` + Rule string `json:"rule"` +} + +// ProjectRule holds rules loaded from /.open-code-review/rule.json. +type ProjectRule struct { + Rules []ProjectRuleEntry `json:"rules"` +} + +// composedResolver implements Resolver with layered priority. +type composedResolver struct { + custom *ProjectRule // highest: --rule flag + project *ProjectRule // high: .open-code-review/rule.json + global *ProjectRule // low: ~/.open-code-review/rule.json + system *SystemRule // lowest: embedded default +} + +// NewResolver builds a Resolver with the following priority: +// 1. Custom rule file specified via --rule flag (first match wins) +// 2. Project-local .open-code-review/rule.json (first match wins) +// 3. Global ~/.open-code-review/rule.json (first match wins) +// 4. Embedded system default rules +func NewResolver(repoDir, customRulePath string) (Resolver, error) { + sysRule, err := LoadDefault() + if err != nil { + return nil, err + } + + var customRule *ProjectRule + if customRulePath != "" { + cr, err := loadRuleFile(customRulePath) + if err != nil { + return nil, err + } + customRule = cr + } + + var projectRule *ProjectRule + if repoDir != "" { + pr, err := loadProjectRule(repoDir) + if err != nil { + return nil, err + } + projectRule = pr + } + + globalRule, err := loadGlobalRule() + if err != nil { + return nil, err + } + + return &composedResolver{custom: customRule, project: projectRule, global: globalRule, system: sysRule}, nil +} + +func loadGlobalRule() (*ProjectRule, error) { + home, err := os.UserHomeDir() + if err != nil { + return nil, nil + } + path := filepath.Join(home, ".open-code-review", "rule.json") + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, fmt.Errorf("read global rule %s: %w", path, err) + } + var pr ProjectRule + if err := json.Unmarshal(data, &pr); err != nil { + return nil, fmt.Errorf("unmarshal global rule: %w", err) + } + return &pr, nil +} + +func loadRuleFile(path string) (*ProjectRule, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read rule file %s: %w", path, err) + } + var pr ProjectRule + if err := json.Unmarshal(data, &pr); err != nil { + return nil, fmt.Errorf("unmarshal rule file %s: %w", path, err) + } + return &pr, nil +} + +func loadProjectRule(repoDir string) (*ProjectRule, error) { + path := filepath.Join(repoDir, ".open-code-review", "rule.json") + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, fmt.Errorf("read project rule %s: %w", path, err) + } + var pr ProjectRule + if err := json.Unmarshal(data, &pr); err != nil { + return nil, fmt.Errorf("unmarshal project rule: %w", err) + } + return &pr, nil +} + +// Resolve checks each layer in priority order; first match wins. +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 + } + return c.system.Resolve(path) +} + +func matchProjectRule(pr *ProjectRule, path string) string { + if pr == nil { + return "" + } + for _, entry := range pr.Rules { + expanded := expandBraces(entry.Path) + for _, p := range expanded { + if matched, _ := doublestar.Match(p, path); matched { + return entry.Rule + } + } + } + return "" +} diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index f5989d2..5958671 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -1,6 +1,8 @@ package rules import ( + "os" + "path/filepath" "strings" "testing" ) @@ -172,3 +174,198 @@ func truncate(s string, maxLen int) string { } return s[:maxLen] + "..." } + +func TestNewResolver_DefaultOnly(t *testing.T) { + resolver, err := NewResolver(t.TempDir(), "") + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + got := resolver.Resolve("src/main.java") + if !strings.Contains(got, "逻辑错误识别") { + t.Errorf("expected system default java rule, got %q", truncate(got, 80)) + } +} + +func TestNewResolver_ProjectFileMissing(t *testing.T) { + resolver, err := NewResolver(t.TempDir(), "") + if err != nil { + t.Fatalf("NewResolver should not fail when project rule is missing: %v", err) + } + got := resolver.Resolve("readme.md") + if got == "" { + t.Errorf("expected non-empty default rule") + } +} + +func TestNewResolver_ProjectRuleHighestPriority(t *testing.T) { + dir := t.TempDir() + ocrDir := filepath.Join(dir, ".open-code-review") + if err := os.MkdirAll(ocrDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + ruleJSON := `{"rules":[{"path":"force-api/**/*.java","rule":"project-java-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) + } + + tests := []struct { + path string + want string + }{ + {"force-api/src/foo.java", "project-java-rule"}, + {"other/src/bar.java", "逻辑错误识别"}, + } + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + got := resolver.Resolve(tt.path) + if !strings.Contains(got, tt.want) { + t.Errorf("Resolve(%q) = %q, want containing %q", tt.path, truncate(got, 80), tt.want) + } + }) + } +} + +func TestNewResolver_ProjectRuleFallsBackToSystem(t *testing.T) { + dir := t.TempDir() + ocrDir := filepath.Join(dir, ".open-code-review") + if err := os.MkdirAll(ocrDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + ruleJSON := `{"rules":[{"path":"special/**/*.go","rule":"special-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("other/main.go") + if !strings.Contains(got, "逻辑问题") { + t.Errorf("expected system go rule, got %q", truncate(got, 80)) + } +} + +func TestNewResolver_CustomRuleOverridesDefault(t *testing.T) { + dir := t.TempDir() + customRule := `{"rules":[{"path":"**/*.go","rule":"custom-go-rule"}]}` + customPath := filepath.Join(dir, "custom_rules.json") + if err := os.WriteFile(customPath, []byte(customRule), 0o644); err != nil { + t.Fatalf("write custom rule: %v", err) + } + + resolver, err := NewResolver(t.TempDir(), customPath) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + + got := resolver.Resolve("main.go") + if got != "custom-go-rule" { + t.Errorf("expected custom-go-rule, got %q", got) + } + // --rule not matched → falls through to system default + got = resolver.Resolve("readme.md") + if !strings.Contains(got, "错别字") { + t.Errorf("expected system default rule, got %q", truncate(got, 80)) + } +} + +func TestNewResolver_CustomOverridesProject(t *testing.T) { + // Setup --rule file (highest priority) + customDir := t.TempDir() + customRule := `{"rules":[{"path":"**/*.java","rule":"custom-java-rule"}]}` + customPath := filepath.Join(customDir, "custom_rules.json") + if err := os.WriteFile(customPath, []byte(customRule), 0o644); err != nil { + t.Fatalf("write custom rule: %v", err) + } + + // Setup project rule with narrower pattern + repoDir := t.TempDir() + ocrDir := filepath.Join(repoDir, ".open-code-review") + if err := os.MkdirAll(ocrDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + projRule := `{"rules":[{"path":"force-api/**/*.java","rule":"project-java-rule"},{"path":"**/*.go","rule":"project-go-rule"}]}` + if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(projRule), 0o644); err != nil { + t.Fatalf("write rule.json: %v", err) + } + + resolver, err := NewResolver(repoDir, customPath) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + + tests := []struct { + path string + want string + }{ + {"force-api/src/foo.java", "custom-java-rule"}, // --rule wins (highest priority) + {"other/src/bar.java", "custom-java-rule"}, // --rule wins + {"main.go", "project-go-rule"}, // --rule misses → project wins + {"readme.md", "错别字"}, // all miss → system default + } + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + got := resolver.Resolve(tt.path) + if !strings.Contains(got, tt.want) { + t.Errorf("Resolve(%q) = %q, want containing %q", tt.path, truncate(got, 80), tt.want) + } + }) + } +} + +func TestNewResolver_ProjectFileMalformed(t *testing.T) { + dir := t.TempDir() + ocrDir := filepath.Join(dir, ".open-code-review") + if err := os.MkdirAll(ocrDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte("{invalid json"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + _, err := NewResolver(dir, "") + if err == nil { + t.Errorf("expected error for malformed project rule.json") + } +} + +func TestNewResolver_BraceExpansionInProjectRule(t *testing.T) { + dir := t.TempDir() + ocrDir := filepath.Join(dir, ".open-code-review") + if err := os.MkdirAll(ocrDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + ruleJSON := `{"rules":[{"path":"src/**/*.{java,kt}","rule":"jvm-rule"}]}` + 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) + } + + tests := []struct { + path string + want string + }{ + {"src/main/foo.java", "jvm-rule"}, + {"src/main/bar.kt", "jvm-rule"}, + {"src/main/baz.go", "逻辑问题"}, + } + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + got := resolver.Resolve(tt.path) + if !strings.Contains(got, tt.want) { + t.Errorf("Resolve(%q) = %q, want containing %q", tt.path, truncate(got, 80), tt.want) + } + }) + } +}