mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-10-02 09:24:52 +08:00
feat(allowlist): exclude secret paths from review (#1262)
* feat(allowlist): exclude secret paths from review * docs(review-rules): document secret exclusions Built on the security finding by @wu21-web and the initial implementation in #1244 by @katepallewarprathmesh-sketch. Co-authored-by: Prathmesh Katepallewar <284222233+katepallewarprathmesh-sketch@users.noreply.github.com> Co-authored-by: Tao Xin <149216116+wu21-web@users.noreply.github.com>
This commit is contained in:
committed by
kite
co-authored by
Prathmesh Katepallewar
Tao Xin
parent
b3dbcb634c
commit
6d5bc36790
@@ -1989,6 +1989,8 @@ func (a *Agent) logExclusions(decisions []fileDecision) {
|
||||
switch dec.Reason {
|
||||
case ExcludeBinary:
|
||||
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — binary file\n", effectivePath(dec.Diff))
|
||||
case ExcludeSecret:
|
||||
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — matches a built-in secret path\n", effectivePath(dec.Diff))
|
||||
case ExcludeUserRule, ExcludeExtension, ExcludeDefaultPath:
|
||||
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — filtered by path/extension rules\n", effectivePath(dec.Diff))
|
||||
default:
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
package agent
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"os"
|
||||
@@ -18,6 +19,7 @@ import (
|
||||
"github.com/alibaba/open-code-review/internal/llm"
|
||||
"github.com/alibaba/open-code-review/internal/model"
|
||||
"github.com/alibaba/open-code-review/internal/session"
|
||||
"github.com/alibaba/open-code-review/internal/stdout"
|
||||
"github.com/alibaba/open-code-review/internal/tool"
|
||||
)
|
||||
|
||||
@@ -323,6 +325,33 @@ func TestParseFilterToolCalls(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// logExclusions switches on the exclusion reason with a `continue` default, so
|
||||
// a reason with no case arm is dropped from the log and the filtered count alike.
|
||||
func TestLogExclusions_SecretPath(t *testing.T) {
|
||||
agent := New(Args{})
|
||||
decisions := []fileDecision{
|
||||
{Diff: model.Diff{NewPath: ".env"}, Reason: ExcludeSecret},
|
||||
}
|
||||
|
||||
var buf bytes.Buffer
|
||||
restore := stdout.Swap(&buf)
|
||||
agent.logExclusions(decisions)
|
||||
restore()
|
||||
|
||||
got := buf.String()
|
||||
if !strings.Contains(got, ".env") {
|
||||
t.Errorf("log = %q, want it to name the skipped secret path", got)
|
||||
}
|
||||
if !strings.Contains(got, "secret path") {
|
||||
t.Errorf("log = %q, want secret-specific wording, not the path/extension wording", got)
|
||||
}
|
||||
// The rollup only prints when staticSkipped was incremented, so its presence
|
||||
// proves the decision reached a real case arm rather than `default`.
|
||||
if !strings.Contains(got, "Filtered 1 file(s)") {
|
||||
t.Errorf("log = %q, want the secret exclusion counted in the rollup", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestExtFromPath(t *testing.T) {
|
||||
a := New(Args{})
|
||||
|
||||
|
||||
@@ -24,6 +24,7 @@ const (
|
||||
ExcludeUserRule = model.ExcludeUserRule
|
||||
ExcludeExtension = model.ExcludeExtension
|
||||
ExcludeDefaultPath = model.ExcludeDefaultPath
|
||||
ExcludeSecret = model.ExcludeSecret
|
||||
ExcludeProviderDirectory = model.ExcludeProviderDirectory
|
||||
ExcludeDeleted = model.ExcludeDeleted
|
||||
ExcludeBinary = model.ExcludeBinary
|
||||
|
||||
@@ -407,6 +407,115 @@ func TestWhyExcluded_PriorityOrder(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// Credential paths are excluded before either user rule is consulted, so
|
||||
// neither an include nor an exclude can change the outcome or the reason.
|
||||
func TestWhyExcluded_SecretPath(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
filter *rules.FileFilter
|
||||
path string
|
||||
expected ExcludeReason
|
||||
}{
|
||||
{
|
||||
name: "secret path excluded with no user config",
|
||||
path: ".env",
|
||||
expected: ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "nested secret path excluded",
|
||||
path: "foo/bar/.env",
|
||||
expected: ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "extensionless key excluded",
|
||||
path: "id_rsa",
|
||||
expected: ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "ssh directory contents excluded",
|
||||
path: "foo/.ssh/id_ed25519",
|
||||
expected: ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "include cannot override a secret path",
|
||||
filter: &rules.FileFilter{Include: []string{"**/.env"}},
|
||||
path: ".env",
|
||||
expected: ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "user exclude does not change the reason",
|
||||
filter: &rules.FileFilter{Exclude: []string{"**/.env"}},
|
||||
path: ".env",
|
||||
expected: ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "env template with explicit include stays reviewable",
|
||||
filter: &rules.FileFilter{Include: []string{"**/.env.example"}},
|
||||
path: ".env.example",
|
||||
expected: ExcludeNone,
|
||||
},
|
||||
{
|
||||
// Still not reviewable, but for the pre-existing extension reason.
|
||||
name: "env template without include keeps unsupported_ext",
|
||||
path: ".env.example",
|
||||
expected: ExcludeExtension,
|
||||
},
|
||||
{
|
||||
name: "public key is not a secret path",
|
||||
path: "id_rsa.pub",
|
||||
expected: ExcludeExtension,
|
||||
},
|
||||
{
|
||||
name: "dockerfile unchanged",
|
||||
path: "Dockerfile",
|
||||
expected: ExcludeNone,
|
||||
},
|
||||
{
|
||||
name: "makefile unchanged",
|
||||
path: "Makefile",
|
||||
expected: ExcludeNone,
|
||||
},
|
||||
{
|
||||
name: "ordinary unsupported extension unchanged",
|
||||
path: "src/notes.txt",
|
||||
expected: ExcludeExtension,
|
||||
},
|
||||
{
|
||||
name: "ordinary user exclude still reports user_exclude",
|
||||
filter: &rules.FileFilter{Exclude: []string{"vendor/**"}},
|
||||
path: "vendor/foo/bar.go",
|
||||
expected: ExcludeUserRule,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
agent := New(Args{FileFilter: tt.filter})
|
||||
got := agent.whyExcluded(model.Diff{NewPath: tt.path})
|
||||
if got != tt.expected {
|
||||
t.Errorf("whyExcluded(%q) = %q, want %q", tt.path, got, tt.expected)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestWhyExcluded_SecretRename(t *testing.T) {
|
||||
agent := New(Args{
|
||||
FileFilter: &rules.FileFilter{
|
||||
Include: []string{"**/.env.example"},
|
||||
},
|
||||
})
|
||||
|
||||
diff := model.Diff{
|
||||
OldPath: ".env",
|
||||
NewPath: ".env.example",
|
||||
}
|
||||
|
||||
if got := agent.whyExcluded(diff); got != ExcludeSecret {
|
||||
t.Fatalf("whyExcluded() = %q, want %q", got, ExcludeSecret)
|
||||
}
|
||||
}
|
||||
|
||||
// TestSelectFilesDeletionGate covers the one gate selectFiles adds over the
|
||||
// static ones: a deletion is never selected, however reviewable its path looks.
|
||||
// The static gates themselves are covered by the whyExcluded tables above.
|
||||
|
||||
@@ -62,11 +62,11 @@ func (a *Agent) selectFiles(diffs []model.Diff) []fileDecision {
|
||||
return decisions
|
||||
}
|
||||
|
||||
// whyExcluded applies the static gates — binary, user exclude/include, then
|
||||
// the extension allowlist and default-path patterns — and returns the specific
|
||||
// reason a file is excluded, or ExcludeNone when it survives all of them. It
|
||||
// answers only what the path and the diff header say; the size gate lives in
|
||||
// selectFiles because it needs the resolved token limit.
|
||||
// whyExcluded applies the static gates — binary, the built-in secret paths,
|
||||
// user exclude/include, then the extension allowlist and default-path patterns
|
||||
// — and returns the specific reason a file is excluded, or ExcludeNone when it
|
||||
// survives all of them. It answers only what the path and the diff header say;
|
||||
// the size gate lives in selectFiles because it needs the resolved token limit.
|
||||
func (a *Agent) whyExcluded(d model.Diff) ExcludeReason {
|
||||
if d.IsBinary {
|
||||
return ExcludeBinary
|
||||
@@ -75,6 +75,12 @@ func (a *Agent) whyExcluded(d model.Diff) ExcludeReason {
|
||||
path := effectivePath(d)
|
||||
f := a.args.FileFilter
|
||||
|
||||
// Ahead of both user rules: no include glob can admit a credential path,
|
||||
// and no user exclude can claim it under a different reason.
|
||||
if allowedext.IsSecretPath(d.OldPath) || allowedext.IsSecretPath(d.NewPath) {
|
||||
return ExcludeSecret
|
||||
}
|
||||
|
||||
if f != nil && f.IsUserExcluded(path) {
|
||||
return ExcludeUserRule
|
||||
}
|
||||
|
||||
@@ -0,0 +1,15 @@
|
||||
[
|
||||
"**/.env",
|
||||
"**/.env.local",
|
||||
"**/.env.*.local",
|
||||
"**/.ssh/**",
|
||||
"**/id_rsa",
|
||||
"**/id_dsa",
|
||||
"**/id_ecdsa",
|
||||
"**/id_ed25519",
|
||||
"**/.netrc",
|
||||
"**/_netrc",
|
||||
"**/.npmrc",
|
||||
"**/.pypirc",
|
||||
"**/.dockercfg"
|
||||
]
|
||||
@@ -0,0 +1,54 @@
|
||||
// SPDX-License-Identifier: Apache-2.0
|
||||
// Copyright 2026 alibaba/open-code-review Contributors
|
||||
|
||||
package allowedext
|
||||
|
||||
import (
|
||||
_ "embed"
|
||||
"encoding/json"
|
||||
"strings"
|
||||
"sync"
|
||||
|
||||
"github.com/bmatcuk/doublestar/v4"
|
||||
)
|
||||
|
||||
// Secret paths are kept apart from default_exclude_patterns.json on purpose:
|
||||
// the default exclude list holds review noise that an include rule is allowed
|
||||
// to bring back, while these paths must not enter the review scope at all, so
|
||||
// no include rule can admit them. Matching follows the same glob and case rules
|
||||
// as IsExcludedPath; see the package comment in allowed_ext.go for the syntax.
|
||||
|
||||
//go:embed default_secret_patterns.json
|
||||
var secretData []byte
|
||||
|
||||
var (
|
||||
secretPatterns []string // raw patterns from JSON, lowercased by initSecret
|
||||
secretOnce sync.Once
|
||||
)
|
||||
|
||||
func initSecret() {
|
||||
if err := json.Unmarshal(secretData, &secretPatterns); err != nil {
|
||||
panic("allowedext: failed to parse default_secret_patterns.json: " + err.Error())
|
||||
}
|
||||
for i, p := range secretPatterns {
|
||||
secretPatterns[i] = strings.ToLower(p)
|
||||
}
|
||||
}
|
||||
|
||||
// IsSecretPath returns true when the given file path matches any built-in
|
||||
// secret pattern — credential files such as .env, id_rsa or .netrc that should
|
||||
// never be sent to a model as part of a review. The check is case-insensitive
|
||||
// and purely path-based; file contents are never inspected.
|
||||
//
|
||||
// A path that is not a secret is not thereby reviewable: it still has to pass
|
||||
// the extension allowlist and the default exclude patterns.
|
||||
func IsSecretPath(path string) bool {
|
||||
secretOnce.Do(initSecret)
|
||||
lowerPath := strings.ToLower(path)
|
||||
for _, pattern := range secretPatterns {
|
||||
if matched, _ := doublestar.Match(pattern, lowerPath); matched {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
@@ -0,0 +1,88 @@
|
||||
// SPDX-License-Identifier: Apache-2.0
|
||||
// Copyright 2026 alibaba/open-code-review Contributors
|
||||
|
||||
package allowedext
|
||||
|
||||
import (
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestIsSecretPath(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
path string
|
||||
want bool
|
||||
}{
|
||||
// .env and its per-environment variants
|
||||
{"env at root", ".env", true},
|
||||
{"env nested", "foo/.env", true},
|
||||
{"env deeply nested", "a/b/c/.env", true},
|
||||
{"env local at root", ".env.local", true},
|
||||
{"env local nested", "foo/.env.local", true},
|
||||
{"env scoped local at root", ".env.production.local", true},
|
||||
{"env scoped local nested", "foo/.env.production.local", true},
|
||||
|
||||
// SSH material: the directory pattern carries everything inside it,
|
||||
// including files the key-name patterns do not list (config, known_hosts).
|
||||
{"ssh dir at root", ".ssh/id_ed25519", true},
|
||||
{"ssh dir nested", "nested/.ssh/id_ed25519", true},
|
||||
{"ssh config", ".ssh/config", true},
|
||||
{"ssh known_hosts", "home/.ssh/known_hosts", true},
|
||||
{"ssh nested key dir", "a/b/c/.ssh/keys/id_rsa", true},
|
||||
|
||||
// Private keys outside a .ssh directory
|
||||
{"id_rsa at root", "id_rsa", true},
|
||||
{"id_rsa nested", "foo/id_rsa", true},
|
||||
{"id_dsa", "id_dsa", true},
|
||||
{"id_ecdsa", "keys/id_ecdsa", true},
|
||||
{"id_ed25519", "id_ed25519", true},
|
||||
|
||||
// Credential files for network and package tooling
|
||||
{"netrc", ".netrc", true},
|
||||
{"netrc nested", "home/.netrc", true},
|
||||
{"windows netrc", "_netrc", true},
|
||||
{"npmrc", ".npmrc", true},
|
||||
{"npmrc nested", "foo/.npmrc", true},
|
||||
{"pypirc", ".pypirc", true},
|
||||
{"dockercfg", ".dockercfg", true},
|
||||
|
||||
// Templates are not credentials. They stay out of the secret list so an
|
||||
// explicit include can still bring them into review.
|
||||
{"env example", ".env.example", false},
|
||||
{"env example nested", "foo/.env.example", false},
|
||||
{"env sample", ".env.sample", false},
|
||||
{"env template", ".env.template", false},
|
||||
|
||||
// Public and derived files that only look like key material
|
||||
{"public key", "id_rsa.pub", false},
|
||||
{"key backup", "foo/id_rsa.backup", false},
|
||||
|
||||
// "credentials" is deliberately absent from the pattern list: it is a
|
||||
// common ordinary identifier, not a conventional credential filename.
|
||||
{"bare credentials file", "credentials", false},
|
||||
{"credentials package dir", "src/credentials/package.go", false},
|
||||
{"credentials in filename", "internal/credentials_loader.go", false},
|
||||
|
||||
// Legitimate extensionless files must keep their existing behavior.
|
||||
{"dockerfile", "Dockerfile", false},
|
||||
{"makefile", "Makefile", false},
|
||||
{"nested dockerfile", "build/Dockerfile", false},
|
||||
|
||||
// Files that merely carry the .env extension are outside this list;
|
||||
// the extension allowlist governs them, unchanged by #1240.
|
||||
{"env extension file", "prod.env", false},
|
||||
{"env extension nested", "config/staging.env", false},
|
||||
|
||||
// Case-insensitive, matching IsExcludedPath.
|
||||
{"uppercase env", ".ENV", true},
|
||||
{"uppercase key", "ID_RSA", true},
|
||||
{"mixed case ssh dir", "Foo/.SSH/id_ed25519", true},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
if got := IsSecretPath(tt.path); got != tt.want {
|
||||
t.Errorf("IsSecretPath(%q) = %v, want %v", tt.path, got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -91,6 +91,7 @@ func TestExcludeReasonConstants(t *testing.T) {
|
||||
ExcludeUserRule: "user_exclude",
|
||||
ExcludeExtension: "unsupported_ext",
|
||||
ExcludeDefaultPath: "default_path",
|
||||
ExcludeSecret: "secret_exclude",
|
||||
ExcludeProviderDirectory: "provider_directory",
|
||||
ExcludeDeleted: "deleted",
|
||||
ExcludeBinary: "binary",
|
||||
|
||||
@@ -12,6 +12,10 @@ const (
|
||||
ExcludeUserRule ExcludeReason = "user_exclude"
|
||||
ExcludeExtension ExcludeReason = "unsupported_ext"
|
||||
ExcludeDefaultPath ExcludeReason = "default_path"
|
||||
// ExcludeSecret is a built-in credential path (for example .env or
|
||||
// id_rsa). Like provider_directory and unlike default_path, an include
|
||||
// rule cannot make the file reviewable.
|
||||
ExcludeSecret ExcludeReason = "secret_exclude"
|
||||
// ExcludeProviderDirectory is an unconditional diff-provider directory
|
||||
// exclusion (for example vendor/ or node_modules/). Unlike default_path,
|
||||
// an include rule cannot make the file reviewable.
|
||||
|
||||
@@ -472,6 +472,8 @@ func (a *Agent) logSelection(decisions []scanSelection) {
|
||||
}
|
||||
if decision.item.IsBinary {
|
||||
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — binary file\n", decision.item.Path)
|
||||
} else if decision.reason == model.ExcludeSecret {
|
||||
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — matches a built-in secret path\n", decision.item.Path)
|
||||
} else {
|
||||
fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s — filtered by path/extension rules\n", decision.item.Path)
|
||||
}
|
||||
@@ -501,6 +503,11 @@ func (a *Agent) whyExcluded(it model.ScanItem) model.ExcludeReason {
|
||||
return model.ExcludeBinary
|
||||
}
|
||||
path := it.Path
|
||||
// Ahead of both user rules, matching internal/agent: no include glob can
|
||||
// admit a credential path, and no user exclude can reclassify it. See #1240.
|
||||
if allowedext.IsSecretPath(path) {
|
||||
return model.ExcludeSecret
|
||||
}
|
||||
if a.args.FileFilter != nil && a.args.FileFilter.IsUserExcluded(path) {
|
||||
return model.ExcludeUserRule
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
package scan
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"fmt"
|
||||
"strings"
|
||||
@@ -15,6 +16,7 @@ import (
|
||||
"github.com/alibaba/open-code-review/internal/llmloop"
|
||||
"github.com/alibaba/open-code-review/internal/model"
|
||||
"github.com/alibaba/open-code-review/internal/session"
|
||||
"github.com/alibaba/open-code-review/internal/stdout"
|
||||
"github.com/alibaba/open-code-review/internal/tool"
|
||||
)
|
||||
|
||||
@@ -133,6 +135,33 @@ func TestSelectScanItems_Reviewability(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// Without its own branch a credential path is reported as "filtered by
|
||||
// path/extension rules", which misstates why it was skipped.
|
||||
func TestLogSelection_SecretPath(t *testing.T) {
|
||||
a := NewAgent(Args{
|
||||
Template: makeTemplateWithFullScan(),
|
||||
Session: session.New(t.TempDir(), "main", "test", session.SessionOptions{
|
||||
ReviewMode: session.ReviewModeFullScan,
|
||||
}),
|
||||
})
|
||||
decisions := []scanSelection{
|
||||
{item: model.ScanItem{Path: ".env"}, reason: model.ExcludeSecret},
|
||||
}
|
||||
|
||||
var buf bytes.Buffer
|
||||
restore := stdout.Swap(&buf)
|
||||
a.logSelection(decisions)
|
||||
restore()
|
||||
|
||||
got := buf.String()
|
||||
if !strings.Contains(got, "secret path") {
|
||||
t.Errorf("log = %q, want secret-specific wording", got)
|
||||
}
|
||||
if strings.Contains(got, "path/extension rules") {
|
||||
t.Errorf("log = %q, want the secret path not reported as a path/extension filter", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWhyExcluded_AllBranches(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
@@ -201,6 +230,72 @@ func TestWhyExcluded_AllBranches(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// Scan must apply the same secret precedence as the review side, so a credential
|
||||
// path is classified identically whichever selection path reaches it.
|
||||
func TestWhyExcluded_SecretPath(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
path string
|
||||
filter *rules.FileFilter
|
||||
want model.ExcludeReason
|
||||
}{
|
||||
{
|
||||
name: "secret path excluded with no user config",
|
||||
path: ".env",
|
||||
want: model.ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "nested secret path excluded",
|
||||
path: "foo/bar/.env",
|
||||
want: model.ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "ssh directory contents excluded",
|
||||
path: "foo/.ssh/id_ed25519",
|
||||
want: model.ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "include cannot override a secret path",
|
||||
path: ".env",
|
||||
filter: &rules.FileFilter{Include: []string{"**/.env"}},
|
||||
want: model.ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "user exclude does not change the reason",
|
||||
path: ".env",
|
||||
filter: &rules.FileFilter{Exclude: []string{"**/.env"}},
|
||||
want: model.ExcludeSecret,
|
||||
},
|
||||
{
|
||||
name: "env template with explicit include stays reviewable",
|
||||
path: ".env.example",
|
||||
filter: &rules.FileFilter{Include: []string{"**/.env.example"}},
|
||||
want: model.ExcludeNone,
|
||||
},
|
||||
{
|
||||
name: "dockerfile unchanged",
|
||||
path: "Dockerfile",
|
||||
want: model.ExcludeNone,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
a := NewAgent(Args{
|
||||
Template: makeTemplateWithFullScan(),
|
||||
FileFilter: tt.filter,
|
||||
Session: session.New(t.TempDir(), "main", "test", session.SessionOptions{
|
||||
ReviewMode: session.ReviewModeFullScan,
|
||||
}),
|
||||
})
|
||||
got := a.whyExcluded(model.ScanItem{Path: tt.path, Content: "x"})
|
||||
if got != tt.want {
|
||||
t.Errorf("whyExcluded(%q) = %q, want %q", tt.path, got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestExtFromPath(t *testing.T) {
|
||||
tests := []struct {
|
||||
path string
|
||||
|
||||
@@ -54,7 +54,7 @@ Three independent fields:
|
||||
through the `unsupported_ext` and `default_path` checks and may still
|
||||
be reviewed.
|
||||
- `exclude` — optional. Glob patterns for files OCR must *not* review.
|
||||
Highest precedence within the filter.
|
||||
Highest precedence among user-configured filters.
|
||||
- `rules` — array of `{path, rule}` entries, evaluated **in declaration
|
||||
order**. The first `path` whose glob matches the file determines the
|
||||
prompt OCR sends to the model for that file.
|
||||
@@ -77,24 +77,28 @@ for matching:
|
||||
|
||||
## How files are filtered
|
||||
|
||||
The filter is a five-gate algorithm in
|
||||
The filter is a six-gate algorithm in
|
||||
[`internal/agent/selection.go`](https://github.com/alibaba/open-code-review/blob/main/internal/agent/selection.go).
|
||||
For each diff, OCR asks:
|
||||
|
||||
1. **`binary`** — Is the file binary? Excluded.
|
||||
2. **`user_exclude`** — Does the path match any user `exclude` pattern?
|
||||
2. **`secret_exclude`** — Does either path match a
|
||||
[built-in secret-path pattern](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/default_secret_patterns.json)?
|
||||
Excluded. This protection runs before user rules and cannot be overridden
|
||||
by an `include` pattern.
|
||||
3. **`user_exclude`** — Does the path match any user `exclude` pattern?
|
||||
Excluded.
|
||||
3. **`user_include`** — If the user defined `include`, does the path
|
||||
4. **`user_include`** — If the user defined `include`, does the path
|
||||
match? If yes, **kept immediately** (bypasses the `unsupported_ext`
|
||||
and `default_path` gates below).
|
||||
4. **`unsupported_ext`** — Is the file extension in the
|
||||
5. **`unsupported_ext`** — Is the file extension in the
|
||||
[allowlist](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/supported_file_types.json)?
|
||||
Excluded if not.
|
||||
5. **`default_path`** — Does the path match a built-in test-file exclude
|
||||
6. **`default_path`** — Does the path match a built-in test-file exclude
|
||||
pattern (`**/*_test.go`, `**/*.test.{js,jsx,ts,tsx}`, `**/*_spec.rb`,
|
||||
…)? Excluded.
|
||||
|
||||
Files that survive all five gates are sent to the LLM, unless the diff
|
||||
Files that survive all six gates are sent to the LLM, unless the diff
|
||||
alone exceeds 80% of `max_tokens`: `selectFiles` applies that ceiling
|
||||
after the gates and excludes the file as `too_large`. It also marks a
|
||||
file whose new path is `/dev/null` as `deleted`; there's no new content
|
||||
|
||||
@@ -43,7 +43,7 @@ OCR は**4 層の優先順位チェーン**でルールを解決します。各
|
||||
3 つの独立したフィールドがあります:
|
||||
|
||||
- `include`: 任意。組み込みのデフォルト除外パターン(テストファイルの除外。下記参照)を*バイパス*するための glob パターンです。ホワイトリストではありません。どの `include` パターンにも一致しないファイルも、依然として `unsupported_ext` と `default_path` のチェックを通過し、レビューされる可能性があります。
|
||||
- `exclude`: 任意。OCR がレビューしないファイルの glob パターンです。フィルタリングで最も優先されます。
|
||||
- `exclude`: 任意。OCR がレビューしないファイルの glob パターンです。ユーザー設定のフィルターの中で最も優先されます。
|
||||
- `rules`: `{path, rule}` エントリの配列で、**宣言順**に評価されます。そのファイルに最初に一致した `path` glob のエントリが、OCR がモデルに送る prompt を決定します。
|
||||
|
||||
### glob の機能
|
||||
@@ -60,15 +60,16 @@ OCR は [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest
|
||||
|
||||
## ファイルがどのようにフィルタリングされるか
|
||||
|
||||
フィルタリングは 5 段階のゲートアルゴリズムで、[`internal/agent/selection.go`](https://github.com/alibaba/open-code-review/blob/main/internal/agent/selection.go) にあります。各 diff について、OCR は順に次を問います:
|
||||
フィルタリングは 6 段階のゲートアルゴリズムで、[`internal/agent/selection.go`](https://github.com/alibaba/open-code-review/blob/main/internal/agent/selection.go) にあります。各 diff について、OCR は順に次を問います:
|
||||
|
||||
1. **`binary`**: ファイルはバイナリか? 除外します。
|
||||
2. **`user_exclude`**: パスがいずれかのユーザー `exclude` パターンに一致するか? 除外します。
|
||||
3. **`user_include`**: ユーザーが `include` を定義している場合、パスは一致するか? 一致するなら**即座に保持**します(下記の `unsupported_ext` と `default_path` のゲートをバイパス)。
|
||||
4. **`unsupported_ext`**: ファイルの拡張子は[ホワイトリスト](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/supported_file_types.json)にあるか? なければ除外します。
|
||||
5. **`default_path`**: パスがいずれかの組み込みテストファイル除外パターン(`**/*_test.go`、`**/*.test.{js,jsx,ts,tsx}`、`**/*_spec.rb`……)に一致するか? 除外します。
|
||||
2. **`secret_exclude`**: 古いパスまたは新しいパスが[組み込みのシークレットパスパターン](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/default_secret_patterns.json)に一致するか? 一致するなら除外します。この保護はユーザールールより先に適用され、`include` パターンでは上書きできません。
|
||||
3. **`user_exclude`**: パスがいずれかのユーザー `exclude` パターンに一致するか? 除外します。
|
||||
4. **`user_include`**: ユーザーが `include` を定義している場合、パスは一致するか? 一致するなら**即座に保持**します(下記の `unsupported_ext` と `default_path` のゲートをバイパス)。
|
||||
5. **`unsupported_ext`**: ファイルの拡張子は[ホワイトリスト](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/supported_file_types.json)にあるか? なければ除外します。
|
||||
6. **`default_path`**: パスがいずれかの組み込みテストファイル除外パターン(`**/*_test.go`、`**/*.test.{js,jsx,ts,tsx}`、`**/*_spec.rb`……)に一致するか? 除外します。
|
||||
|
||||
5 つのゲートをすべて通過したファイルだけが LLM に送られます。ただし diff だけで `max_tokens` の 80% を超える場合は例外で、`selectFiles` がゲートのあとにその上限を適用し、そのファイルを `too_large` として除外します。同じく、新しいパスが `/dev/null` であるファイルは `deleted` と記されます。レビューすべき新しい内容がありません。`ocr review --preview` を使えば、token を消費せずにこのフィルタリング結果を出力できます。
|
||||
6 つのゲートをすべて通過したファイルだけが LLM に送られます。ただし diff だけで `max_tokens` の 80% を超える場合は例外で、`selectFiles` がゲートのあとにその上限を適用し、そのファイルを `too_large` として除外します。同じく、新しいパスが `/dev/null` であるファイルは `deleted` と記されます。レビューすべき新しい内容がありません。`ocr review --preview` を使えば、token を消費せずにこのフィルタリング結果を出力できます。
|
||||
|
||||
### デフォルトパスの除外
|
||||
|
||||
|
||||
@@ -51,7 +51,7 @@ OCR은 **네 겹의 우선순위 사슬**로 규칙을 해석합니다. 파일
|
||||
*건너뛰는* glob 패턴입니다. 화이트리스트가 아닙니다. 어떤 `include` 패턴에도
|
||||
걸리지 않은 파일도 `unsupported_ext`와 `default_path` 검사를 계속 거치며 리뷰될 수
|
||||
있습니다.
|
||||
- `exclude` — 선택. OCR이 리뷰하면 *안 되는* 파일의 glob 패턴입니다. 필터 안에서
|
||||
- `exclude` — 선택. OCR이 리뷰하면 *안 되는* 파일의 glob 패턴입니다. 사용자 설정 필터 안에서
|
||||
가장 높은 우선순위를 가집니다.
|
||||
- `rules` — `{path, rule}` 항목의 배열이며 **선언 순서대로** 평가합니다. 파일에
|
||||
처음 일치하는 `path`가 그 파일을 리뷰할 때 OCR이 모델에 보낼 프롬프트를 정합니다.
|
||||
@@ -75,20 +75,22 @@ OCR은 [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublesta
|
||||
|
||||
필터는
|
||||
[`internal/agent/selection.go`](https://github.com/alibaba/open-code-review/blob/main/internal/agent/selection.go)에
|
||||
있는 다섯 관문 알고리즘입니다. diff마다 OCR이 다음을 묻습니다.
|
||||
있는 여섯 관문 알고리즘입니다. diff마다 OCR이 다음을 묻습니다.
|
||||
|
||||
1. **`binary`** — 바이너리 파일인가? 그렇다면 제외.
|
||||
2. **`user_exclude`** — 경로가 사용자 `exclude` 패턴에 걸리는가? 그렇다면 제외.
|
||||
3. **`user_include`** — 사용자가 `include`를 정의했다면 경로가 거기 걸리는가?
|
||||
2. **`secret_exclude`** — 이전 경로나 새 경로가 [내장 시크릿 경로 패턴](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/default_secret_patterns.json)에 걸리는가? 그렇다면 제외.
|
||||
이 보호는 사용자 규칙보다 먼저 적용되며 `include` 패턴으로 우회할 수 없습니다.
|
||||
3. **`user_exclude`** — 경로가 사용자 `exclude` 패턴에 걸리는가? 그렇다면 제외.
|
||||
4. **`user_include`** — 사용자가 `include`를 정의했다면 경로가 거기 걸리는가?
|
||||
걸리면 **바로 통과**합니다(아래 `unsupported_ext`와 `default_path` 관문을
|
||||
건너뜁니다).
|
||||
4. **`unsupported_ext`** — 파일 확장자가
|
||||
5. **`unsupported_ext`** — 파일 확장자가
|
||||
[허용 목록](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/supported_file_types.json)에
|
||||
있는가? 없으면 제외.
|
||||
5. **`default_path`** — 경로가 내장 테스트 파일 제외 패턴(`**/*_test.go`,
|
||||
6. **`default_path`** — 경로가 내장 테스트 파일 제외 패턴(`**/*_test.go`,
|
||||
`**/*.test.{js,jsx,ts,tsx}`, `**/*_spec.rb` 등)에 걸리는가? 그렇다면 제외.
|
||||
|
||||
다섯 관문을 모두 통과한 파일이 LLM으로 갑니다. 다만 diff만으로 `max_tokens`의
|
||||
여섯 관문을 모두 통과한 파일이 LLM으로 갑니다. 다만 diff만으로 `max_tokens`의
|
||||
80%를 넘으면 `selectFiles`가 관문 뒤에서 그 상한을 적용해 `too_large`로
|
||||
제외합니다. `selectFiles`는 새 경로가 `/dev/null`인 파일도 `deleted`로
|
||||
표시합니다. 리뷰할 새 내용이 없다는 뜻입니다. 토큰을 쓰지 않고 이 필터의 결과만
|
||||
|
||||
@@ -56,7 +56,7 @@ OCR разрешает правила через **четырёхуровнев
|
||||
проходят проверки `unsupported_ext` и `default_path` и могут быть
|
||||
отревьюены.
|
||||
- `exclude` — необязательно. Glob-шаблоны для файлов, которые OCR *не должен*
|
||||
ревьюить. Наивысший приоритет внутри фильтра.
|
||||
ревьюить. Наивысший приоритет среди пользовательских правил фильтрации.
|
||||
- `rules` — массив записей `{path, rule}`, вычисляемых **в порядке объявления**.
|
||||
Первый `path`, чей glob совпадает с файлом, определяет промпт, который OCR
|
||||
отправляет модели для этого файла.
|
||||
@@ -79,24 +79,26 @@ OCR использует [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com
|
||||
|
||||
## Как фильтруются файлы
|
||||
|
||||
Фильтр — пятношаговый алгоритм в
|
||||
Фильтр — шестишаговый алгоритм в
|
||||
[`internal/agent/selection.go`](https://github.com/alibaba/open-code-review/blob/main/internal/agent/selection.go).
|
||||
Для каждого diff OCR спрашивает:
|
||||
|
||||
1. **`binary`** — Файл бинарный? Исключается.
|
||||
2. **`user_exclude`** — Путь совпадает с каким-либо пользовательским шаблоном
|
||||
2. **`secret_exclude`** — Старый или новый путь совпадает со [встроенным шаблоном секретного пути](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/default_secret_patterns.json)? Исключается.
|
||||
Эта защита применяется до пользовательских правил и не может быть переопределена шаблоном `include`.
|
||||
3. **`user_exclude`** — Путь совпадает с каким-либо пользовательским шаблоном
|
||||
`exclude`? Исключается.
|
||||
3. **`user_include`** — Если пользователь задал `include`, путь совпадает? Если
|
||||
4. **`user_include`** — Если пользователь задал `include`, путь совпадает? Если
|
||||
да, **сразу остаётся** (обходит проверки `unsupported_ext` и `default_path`
|
||||
ниже).
|
||||
4. **`unsupported_ext`** — Расширение файла есть в
|
||||
5. **`unsupported_ext`** — Расширение файла есть в
|
||||
[списке разрешённых](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/supported_file_types.json)?
|
||||
Исключается, если нет.
|
||||
5. **`default_path`** — Путь совпадает со встроенным шаблоном исключения
|
||||
6. **`default_path`** — Путь совпадает со встроенным шаблоном исключения
|
||||
тестовых файлов (`**/*_test.go`, `**/*.test.{js,jsx,ts,tsx}`,
|
||||
`**/*_spec.rb`, …)? Исключается.
|
||||
|
||||
Файлы, прошедшие все пять проверок, отправляются в LLM, если только сам diff
|
||||
Файлы, прошедшие все шесть проверок, отправляются в LLM, если только сам diff
|
||||
не превышает 80% от `max_tokens`: `selectFiles` применяет этот предел после
|
||||
проверок и исключает файл как `too_large`. Он же помечает файл, чей новый
|
||||
путь — `/dev/null`, причиной `deleted`; нового содержимого для ревью нет.
|
||||
|
||||
@@ -48,7 +48,7 @@ OCR 用一条**四层优先级链**解析规则。对每个文件路径,按序
|
||||
- `include`——可选。glob 模式,用于*绕过*内置的默认排除模式(测试文件排除——见
|
||||
下文)。它不是白名单:不匹配任何 `include` 模式的文件仍会经过
|
||||
`unsupported_ext` 和 `default_path` 检查,可能仍被评审。
|
||||
- `exclude`——可选。OCR 不予评审的文件 glob 模式。过滤中优先级最高。
|
||||
- `exclude`——可选。OCR 不予评审的文件 glob 模式。在用户配置的过滤规则中优先级最高。
|
||||
- `rules`——`{path, rule}` 条目数组,按**声明顺序**求值。第一个 `path` glob
|
||||
匹配该文件的条目,决定 OCR 发给模型的 prompt。
|
||||
|
||||
@@ -68,21 +68,23 @@ OCR 用 [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest
|
||||
|
||||
## 文件如何被过滤
|
||||
|
||||
过滤是一个五重门算法,位于
|
||||
过滤是一个六重门算法,位于
|
||||
[`internal/agent/selection.go`](https://github.com/alibaba/open-code-review/blob/main/internal/agent/selection.go)。
|
||||
对每个 diff,OCR 依次问:
|
||||
|
||||
1. **`binary`**——文件是二进制吗?排除。
|
||||
2. **`user_exclude`**——路径匹配任何用户 `exclude` 模式吗?排除。
|
||||
3. **`user_include`**——若用户定义了 `include`,路径匹配吗?若是,**立即保留**
|
||||
2. **`secret_exclude`**——旧路径或新路径是否匹配某个[内置敏感路径模式](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/default_secret_patterns.json)?若是,排除。
|
||||
此保护在用户规则之前执行,不能被 `include` 模式覆盖。
|
||||
3. **`user_exclude`**——路径匹配任何用户 `exclude` 模式吗?排除。
|
||||
4. **`user_include`**——若用户定义了 `include`,路径匹配吗?若是,**立即保留**
|
||||
(绕过下面的 `unsupported_ext` 和 `default_path` 门)。
|
||||
4. **`unsupported_ext`**——文件扩展名在
|
||||
5. **`unsupported_ext`**——文件扩展名在
|
||||
[白名单](https://github.com/alibaba/open-code-review/blob/main/internal/config/allowlist/supported_file_types.json)
|
||||
里吗?不在则排除。
|
||||
5. **`default_path`**——路径匹配某个内置测试文件排除模式
|
||||
6. **`default_path`**——路径匹配某个内置测试文件排除模式
|
||||
(`**/*_test.go`、`**/*.test.{js,jsx,ts,tsx}`、`**/*_spec.rb`……)吗?排除。
|
||||
|
||||
通过全部五重门的文件才发给 LLM,除非仅 diff 本身就超过 `max_tokens` 的 80%:
|
||||
通过全部六重门的文件才发给 LLM,除非仅 diff 本身就超过 `max_tokens` 的 80%:
|
||||
`selectFiles` 在各门之后施加该上限,并把文件排除为 `too_large`。它同样把新路径
|
||||
为 `/dev/null` 的文件标记为 `deleted`;没有新内容可评审。用 `ocr review
|
||||
--preview` 可在不花 token 的情况下打印此过滤结果。
|
||||
|
||||
Reference in New Issue
Block a user