mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-07-09 17:28:58 +00:00
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.
This commit is contained in:
parent
58defe6c03
commit
d44fd31989
6 changed files with 412 additions and 31 deletions
34
README.md
34
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 | `<repoDir>/.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**:
|
||||
|
|
|
|||
|
|
@ -143,6 +143,40 @@ ocr viewer
|
|||
ocr viewer --addr :3000
|
||||
```
|
||||
|
||||
## 评审规则
|
||||
|
||||
OCR 通过四层优先级链解析评审规则。每层采用首次匹配原则:如果文件路径匹配到某个模式,则使用该规则;否则穿透到下一层。
|
||||
|
||||
| 优先级 | 来源 | 路径 | 描述 |
|
||||
|--------|------|------|------|
|
||||
| 1(最高) | `--rule` 参数 | 用户指定路径 | CLI 显式覆盖 |
|
||||
| 2 | 项目配置 | `<repoDir>/.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 遵循**三阶段工作流**:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 <repoDir>/.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 ""
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue