From dc6d2cacc0c590032f8227ae7dc9df7bc44ba6ef Mon Sep 17 00:00:00 2001 From: Qiyuanqiii <2297740147@qq.com> Date: Thu, 20 Aug 2026 17:19:18 +0800 Subject: [PATCH 1/3] feat(lang): add Starlark review support Co-authored-by: wu21-web --- internal/agent/preview_test.go | 28 +++++++++++ internal/config/allowlist/allowed_ext_test.go | 6 +++ .../allowlist/supported_file_types.json | 5 +- internal/config/rules/rule_docs/starlark.md | 48 +++++++++++++++++++ internal/config/rules/system_rules.json | 3 +- internal/config/rules/system_rules_test.go | 5 ++ internal/scan/coverage_test.go | 5 ++ internal/tool/file_find.go | 4 +- internal/tool/file_find_test.go | 29 +++++++++++ pages/src/content/docs/en/review-rules.md | 1 + pages/src/content/docs/en/tools.md | 3 +- pages/src/content/docs/ja/review-rules.md | 1 + pages/src/content/docs/ja/tools.md | 2 +- pages/src/content/docs/ru/review-rules.md | 1 + pages/src/content/docs/ru/tools.md | 4 +- pages/src/content/docs/zh/review-rules.md | 1 + pages/src/content/docs/zh/tools.md | 2 +- 17 files changed, 139 insertions(+), 9 deletions(-) create mode 100644 internal/config/rules/rule_docs/starlark.md diff --git a/internal/agent/preview_test.go b/internal/agent/preview_test.go index f0b157651..4e3864f95 100644 --- a/internal/agent/preview_test.go +++ b/internal/agent/preview_test.go @@ -140,6 +140,34 @@ func TestWhyExcluded_ExtensionFilter(t *testing.T) { }, expected: ExcludeNone, }, + { + name: "supported Starlark bzl extension", + diff: model.Diff{ + NewPath: "rules/library.bzl", + }, + expected: ExcludeNone, + }, + { + name: "supported Starlark star extension", + diff: model.Diff{ + NewPath: "config.star", + }, + expected: ExcludeNone, + }, + { + name: "supported Bazel extension", + diff: model.Diff{ + NewPath: "MODULE.bazel", + }, + expected: ExcludeNone, + }, + { + name: "extensionless Starlark build file", + diff: model.Diff{ + NewPath: "pkg/BUILD", + }, + expected: ExcludeNone, + }, { name: "file without extension", diff: model.Diff{ diff --git a/internal/config/allowlist/allowed_ext_test.go b/internal/config/allowlist/allowed_ext_test.go index 464f07c4c..5f27794bd 100644 --- a/internal/config/allowlist/allowed_ext_test.go +++ b/internal/config/allowlist/allowed_ext_test.go @@ -85,6 +85,12 @@ func TestIsAllowedExt(t *testing.T) { {".THRIFT", true}, {".capnp", true}, {".CAPNP", true}, + {".bzl", true}, + {".BZL", true}, + {".bazel", true}, + {".BAZEL", true}, + {".star", true}, + {".STAR", true}, {".txt", false}, {".md", false}, {".png", false}, diff --git a/internal/config/allowlist/supported_file_types.json b/internal/config/allowlist/supported_file_types.json index 7b95d138e..cbca2ae00 100644 --- a/internal/config/allowlist/supported_file_types.json +++ b/internal/config/allowlist/supported_file_types.json @@ -94,5 +94,8 @@ ".libsonnet", ".zig", ".thrift", - ".capnp" + ".capnp", + ".bzl", + ".bazel", + ".star" ] diff --git a/internal/config/rules/rule_docs/starlark.md b/internal/config/rules/rule_docs/starlark.md new file mode 100644 index 000000000..b85a591f2 --- /dev/null +++ b/internal/config/rules/rule_docs/starlark.md @@ -0,0 +1,48 @@ +#### Obvious Typos or Spelling Errors +- Spelling errors in rule names, attribute names, function names, or user-facing docstrings that affect readability or correctness + +#### Dead Code +- Unused `.bzl` exports, private functions, or macro-generated targets that add no value and clutter the workspace +- Unused provider fields or rule attributes that are defined but never consumed + +#### BUILD File Simplicity and Abstraction +- Flag BUILD files that introduce variables or macros solely to avoid minor repetition — BUILD files should remain simple, repetitive, and easy to read by both humans and tools +- Flag custom functions or macros in BUILD files that hide the list of targets, sources, or dependencies, making it harder for users to quickly understand the file +- Flag macros that expand into many native rules if the expansion is non-obvious and increases debugging complexity + +#### Starlark Code Style and Formatting +- Indentation must use **4 spaces**; flag use of 2 spaces or tabs +- Flag missing docstrings at the top of `.bzl` files or on public functions +- Flag naming violations: variables and functions must be `snake_case`; top-level private values must start with a single underscore (e.g., `_private`), but local variables should **not** use the underscore prefix +- Line length: no strict limit, but flag lines that exceed 79 characters when they could be easily split without harming readability +- Keyword arguments: require spaces around the `=` sign (e.g., `name = value`); flag compact style `name=value` +- Boolean attributes must use `True`/`False`, not `1`/`0` +- Flag production-code uses of `print()` unless guarded by a hardcoded `DEBUG = False` — otherwise they spam all users + +#### Macro Design and Usage +- Prefer rules over macros whenever possible; macros should only be used for top-level targets intended to be referenced directly from the CLI or BUILD files +- For macros that generate internal helper targets (not meant to be referenced directly): + - Flag helper targets that are **not** prefixed with the main target's `name` (e.g., `name_bar` or `_name_bar`) + - Flag helpers that lack `visibility = ["//visibility:private"]` + - Flag helpers that do **not** have a `manual` tag to avoid wildcard expansion (`:all`, `:...`, etc.) +- Flag macros that use the `name` parameter to derive dependencies or input files not generated by the macro itself — `name` should only be used to name generated targets +- Flag macro parameters whose names differ from the corresponding rule attributes (e.g., using `src` instead of `srcs` when passing to `srcs`) +- Flag calls to macros that use positional arguments instead of keyword-only arguments + +#### Rule and Aspect Design +- Rule, aspect, and attribute names must be `snake_case` +- Rule names should be nouns that clearly describe the artifact produced (e.g., `*_library`, `*_binary`, `*_test`, `*_import`) +- Common attributes must follow standard naming and types: + - `srcs`: `label_list`, typically allowing files (source files) + - `deps`: `label_list`, typically **not** allowing files (compilation dependencies) + - `data`: `label_list`, allowing files (runtime/test data) + - `runtime_deps`: `label_list` for deps not needed at compile time +- Flag attributes with non-obvious behavior (string templates, special tool requirements) that lack a `doc` string +- Rule implementation functions should be private (leading underscore), e.g., `_myrule_impl` +- Rules must pass data via well-defined providers; flag providers or provider fields that are undeclared or undocumented +- Flag rule designs that are not extensible — other rules should be able to interact with providers and reuse actions +- Flag rule implementations that violate performance guidelines (e.g., unnecessary analysis-phase work, ignoring caching opportunities) + +#### Tooling and Maintainability +- Flag `.bzl` or BUILD files that are not formatted with **Buildifier** (both formatter and linter) +- Flag missing or inadequate tests for macros and rules — test coverage should follow the project's testing guidelines diff --git a/internal/config/rules/system_rules.json b/internal/config/rules/system_rules.json index d41ca28c8..b6adf8856 100644 --- a/internal/config/rules/system_rules.json +++ b/internal/config/rules/system_rules.json @@ -41,6 +41,7 @@ "**/*.{jsonnet,libsonnet}": "jsonnet.md", "**/*.zig": "zig.md", "**/*.thrift": "thrift.md", - "**/*.capnp": "capnp.md" + "**/*.capnp": "capnp.md", + "**/{BUILD,WORKSPACE,*.bzl,*.bazel,*.star}": "starlark.md" } } diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index ed2e21785..4a9e4c678 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -135,6 +135,11 @@ func TestResolve_DefaultRules(t *testing.T) { {"if/common.thrift", "Field IDs and Wire Compatibility"}, {"schema/addressbook.capnp", "Ordinals and Wire Compatibility"}, {"src/rpc.capnp", "Ordinals and Wire Compatibility"}, + {"rules/library.bzl", "BUILD File Simplicity and Abstraction"}, + {"config.star", "BUILD File Simplicity and Abstraction"}, + {"MODULE.bazel", "BUILD File Simplicity and Abstraction"}, + {"pkg/BUILD", "BUILD File Simplicity and Abstraction"}, + {"WORKSPACE", "BUILD File Simplicity and Abstraction"}, } for _, tt := range tests { diff --git a/internal/scan/coverage_test.go b/internal/scan/coverage_test.go index 1059f1e80..841dfe0cc 100644 --- a/internal/scan/coverage_test.go +++ b/internal/scan/coverage_test.go @@ -178,6 +178,11 @@ func TestWhyExcluded_AllBranches(t *testing.T) { item: model.ScanItem{Path: "main.go", Content: "x"}, want: model.ExcludeNone, }, + { + name: "Starlark file passes", + item: model.ScanItem{Path: "rules/library.bzl", Content: "x"}, + want: model.ExcludeNone, + }, } for _, tt := range tests { diff --git a/internal/tool/file_find.go b/internal/tool/file_find.go index 44c2b955d..5d1ade696 100644 --- a/internal/tool/file_find.go +++ b/internal/tool/file_find.go @@ -174,7 +174,7 @@ func (p *FileFindProvider) listWalkFiles(ctx context.Context) ([]string, error) // shouldSkipFile returns true if a git ls-files output path should be skipped. // Keeps only widely useful files (those with recognizable extensions). func shouldSkipFile(path string) bool { - // Keep extensionless build/config files like Makefile, Dockerfile, LICENSE + // Keep well-known extensionless build/config files. base := path if idx := strings.LastIndex(path, "/"); idx != -1 { base = path[idx+1:] @@ -183,7 +183,7 @@ func shouldSkipFile(path string) bool { if !hasExt { // Allow well-known extensionless files switch base { - case "Makefile", "Dockerfile", "LICENSE", "Vagrantfile", "Containerfile": + case "Makefile", "Dockerfile", "LICENSE", "Vagrantfile", "Containerfile", "BUILD", "WORKSPACE": return false } return true // skip other extensionless files diff --git a/internal/tool/file_find_test.go b/internal/tool/file_find_test.go index 518b259ab..d4891c79e 100644 --- a/internal/tool/file_find_test.go +++ b/internal/tool/file_find_test.go @@ -118,6 +118,8 @@ func setupFileFindRepo(t *testing.T) string { write("Makefile", "all:\n") write("Dockerfile", "FROM scratch\n") write("LICENSE", "MIT\n") + write("pkg/BUILD", "filegroup(name = \"all\")\n") + write("WORKSPACE", "workspace(name = \"example\")\n") write("data_binary", "binary\n") run("git", "add", ".") @@ -138,6 +140,30 @@ func TestFileFind_GitRepo_WorkspaceMode(t *testing.T) { } } +func TestFileFind_GitRepo_StarlarkBuildFiles(t *testing.T) { + dir := setupFileFindRepo(t) + p := NewFileFind(&FileReader{RepoDir: dir, Mode: ModeWorkspace}) + + tests := []struct { + query string + want string + }{ + {query: "BUILD", want: "pkg/BUILD"}, + {query: "WORKSPACE", want: "WORKSPACE"}, + } + for _, tt := range tests { + t.Run(tt.query, func(t *testing.T) { + got, err := p.Execute(context.Background(), map[string]any{"query_name": tt.query}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(got, tt.want) { + t.Errorf("expected %s, got: %s", tt.want, got) + } + }) + } +} + func TestFileFind_GitRepo_CommitMode(t *testing.T) { dir := setupFileFindRepo(t) cmd := exec.Command("git", "rev-parse", "HEAD") @@ -213,6 +239,9 @@ func TestShouldSkipFile(t *testing.T) { {"LICENSE", false}, {"Vagrantfile", false}, {"Containerfile", false}, + {"BUILD", false}, + {"pkg/BUILD", false}, + {"WORKSPACE", false}, {"some_binary", true}, {"dir/unknown_file", true}, } diff --git a/pages/src/content/docs/en/review-rules.md b/pages/src/content/docs/en/review-rules.md index 16f2af8dd..636753d6e 100644 --- a/pages/src/content/docs/en/review-rules.md +++ b/pages/src/content/docs/en/review-rules.md @@ -182,6 +182,7 @@ matching order: | `**/*.{jsonnet,libsonnet}` | `jsonnet.md` — Jsonnet configuration templates and libraries. | | `**/*.thrift` | `thrift.md` — Apache Thrift IDL wire compatibility. | | `**/*.capnp` | `capnp.md` — Cap'n Proto schema wire compatibility. | +| `**/{BUILD,WORKSPACE,*.bzl,*.bazel,*.star}` | `starlark.md` — Starlark and Bazel build, macro, and rule design. | | *(fallback)* | `default.md` | The resolved rule body becomes the `{{system_rule}}` placeholder in the diff --git a/pages/src/content/docs/en/tools.md b/pages/src/content/docs/en/tools.md index c4ba6c60c..c176c8983 100644 --- a/pages/src/content/docs/en/tools.md +++ b/pages/src/content/docs/en/tools.md @@ -253,7 +253,8 @@ Find files in the repo by filename keyword (substring match). The candidate set comes from `git ls-files --cached --others --exclude-standard` in workspace mode, or `git ls-tree -r --name-only ` in range / commit mode. Extensionless files are skipped except -for `Makefile`, `Dockerfile`, `LICENSE`, `Vagrantfile`, `Containerfile`. +for `Makefile`, `Dockerfile`, `LICENSE`, `Vagrantfile`, `Containerfile`, +`BUILD`, and `WORKSPACE`. ### Output diff --git a/pages/src/content/docs/ja/review-rules.md b/pages/src/content/docs/ja/review-rules.md index f4347bf5b..247acfadf 100644 --- a/pages/src/content/docs/ja/review-rules.md +++ b/pages/src/content/docs/ja/review-rules.md @@ -144,6 +144,7 @@ OCR は [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest | `**/*.{jsonnet,libsonnet}` | `jsonnet.md`: Jsonnet の設定テンプレートとライブラリ。 | | `**/*.thrift` | `thrift.md`: Apache Thrift IDL のワイヤ互換性。 | | `**/*.capnp` | `capnp.md`: Cap'n Proto スキーマのワイヤ互換性。 | +| `**/{BUILD,WORKSPACE,*.bzl,*.bazel,*.star}` | `starlark.md`: Starlark と Bazel のビルド、マクロ、ルール設計。 | | *(fallback)* | `default.md` | 解決されたルール本文は、plan および main task prompt 内の `{{system_rule}}` プレースホルダーの内容になります。 diff --git a/pages/src/content/docs/ja/tools.md b/pages/src/content/docs/ja/tools.md index b543b3b82..c4732e24e 100644 --- a/pages/src/content/docs/ja/tools.md +++ b/pages/src/content/docs/ja/tools.md @@ -225,7 +225,7 @@ hunk ヘッダー `@@ -x,y +m,n @@` から範囲を計算すべきです—— 候補セットは、ワークスペースモードでは `git ls-files --cached --others --exclude-standard` から、 区間 / commit モードでは `git ls-tree -r --name-only ` から得られます。拡張子のないファイルは スキップされますが、`Makefile`、`Dockerfile`、`LICENSE`、`Vagrantfile`、 -`Containerfile` は例外です。 +`Containerfile`、`BUILD`、`WORKSPACE` は例外です。 ### 出力 diff --git a/pages/src/content/docs/ru/review-rules.md b/pages/src/content/docs/ru/review-rules.md index 8f7c8f40d..2bac18e40 100644 --- a/pages/src/content/docs/ru/review-rules.md +++ b/pages/src/content/docs/ru/review-rules.md @@ -184,6 +184,7 @@ OCR использует [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com | `**/*.{jsonnet,libsonnet}` | `jsonnet.md` — шаблоны конфигурации и библиотеки Jsonnet. | | `**/*.thrift` | `thrift.md` — совместимость Apache Thrift IDL на уровне wire. | | `**/*.capnp` | `capnp.md` — совместимость схем Cap'n Proto на уровне wire. | +| `**/{BUILD,WORKSPACE,*.bzl,*.bazel,*.star}` | `starlark.md` — Starlark и проектирование сборок, макросов и правил Bazel. | | *(fallback)* | `default.md` | Разрешённое тело правила становится значением плейсхолдера `{{system_rule}}` diff --git a/pages/src/content/docs/ru/tools.md b/pages/src/content/docs/ru/tools.md index f20a90625..14b2afd68 100644 --- a/pages/src/content/docs/ru/tools.md +++ b/pages/src/content/docs/ru/tools.md @@ -255,8 +255,8 @@ LINE_RANGE: 10-80 Набор кандидатов формируется командой `git ls-files --cached --others --exclude-standard` в режиме рабочей области или командой `git ls-tree -r --name-only ` в режиме диапазона / коммита. Файлы без -расширения пропускаются, кроме `Makefile`, `Dockerfile`, `LICENSE`, `Vagrantfile` -и `Containerfile`. +расширения пропускаются, кроме `Makefile`, `Dockerfile`, `LICENSE`, `Vagrantfile`, +`Containerfile`, `BUILD` и `WORKSPACE`. ### Вывод diff --git a/pages/src/content/docs/zh/review-rules.md b/pages/src/content/docs/zh/review-rules.md index f9f83bf2b..6653af2dc 100644 --- a/pages/src/content/docs/zh/review-rules.md +++ b/pages/src/content/docs/zh/review-rules.md @@ -165,6 +165,7 @@ OCR 用 [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest | `**/*.{jsonnet,libsonnet}` | `jsonnet.md`——Jsonnet 配置模板与库。 | | `**/*.thrift` | `thrift.md`——Apache Thrift IDL 线协议兼容性。 | | `**/*.capnp` | `capnp.md`——Cap'n Proto schema 线协议兼容性。 | +| `**/{BUILD,WORKSPACE,*.bzl,*.bazel,*.star}` | `starlark.md`——Starlark 与 Bazel 构建、宏和规则设计。 | | *(fallback)* | `default.md` | 解析出的规则正文成为 plan 和 main task prompt 中 `{{system_rule}}` 占位符的内容。 diff --git a/pages/src/content/docs/zh/tools.md b/pages/src/content/docs/zh/tools.md index d9d3fc4d4..6a98496a5 100644 --- a/pages/src/content/docs/zh/tools.md +++ b/pages/src/content/docs/zh/tools.md @@ -224,7 +224,7 @@ hunk 头 `@@ -x,y +m,n @@` 计算范围——通常 `m-50` 到 `m+n+50`。 候选集在工作区模式下来自 `git ls-files --cached --others --exclude-standard`， 在区间 / commit 模式下来自 `git ls-tree -r --name-only `。无扩展名的文件 被跳过，但 `Makefile`、`Dockerfile`、`LICENSE`、`Vagrantfile`、 -`Containerfile` 例外。 +`Containerfile`、`BUILD`、`WORKSPACE` 例外。 ### 输出 From cf180762e0bce3a3a2e4534972e5e1dec0f5a37a Mon Sep 17 00:00:00 2001 From: Tao Xin Date: Thu, 20 Aug 2026 20:28:16 +0900 Subject: [PATCH 2/3] chore: update documentation Co-authored-by: Qiyuanqiii --- internal/config/rules/rule_docs/starlark.md | 274 ++++++++++++++++---- 1 file changed, 226 insertions(+), 48 deletions(-) diff --git a/internal/config/rules/rule_docs/starlark.md b/internal/config/rules/rule_docs/starlark.md index b85a591f2..b6c60896e 100644 --- a/internal/config/rules/rule_docs/starlark.md +++ b/internal/config/rules/rule_docs/starlark.md @@ -1,48 +1,226 @@ -#### Obvious Typos or Spelling Errors -- Spelling errors in rule names, attribute names, function names, or user-facing docstrings that affect readability or correctness - -#### Dead Code -- Unused `.bzl` exports, private functions, or macro-generated targets that add no value and clutter the workspace -- Unused provider fields or rule attributes that are defined but never consumed - -#### BUILD File Simplicity and Abstraction -- Flag BUILD files that introduce variables or macros solely to avoid minor repetition — BUILD files should remain simple, repetitive, and easy to read by both humans and tools -- Flag custom functions or macros in BUILD files that hide the list of targets, sources, or dependencies, making it harder for users to quickly understand the file -- Flag macros that expand into many native rules if the expansion is non-obvious and increases debugging complexity - -#### Starlark Code Style and Formatting -- Indentation must use **4 spaces**; flag use of 2 spaces or tabs -- Flag missing docstrings at the top of `.bzl` files or on public functions -- Flag naming violations: variables and functions must be `snake_case`; top-level private values must start with a single underscore (e.g., `_private`), but local variables should **not** use the underscore prefix -- Line length: no strict limit, but flag lines that exceed 79 characters when they could be easily split without harming readability -- Keyword arguments: require spaces around the `=` sign (e.g., `name = value`); flag compact style `name=value` -- Boolean attributes must use `True`/`False`, not `1`/`0` -- Flag production-code uses of `print()` unless guarded by a hardcoded `DEBUG = False` — otherwise they spam all users - -#### Macro Design and Usage -- Prefer rules over macros whenever possible; macros should only be used for top-level targets intended to be referenced directly from the CLI or BUILD files -- For macros that generate internal helper targets (not meant to be referenced directly): - - Flag helper targets that are **not** prefixed with the main target's `name` (e.g., `name_bar` or `_name_bar`) - - Flag helpers that lack `visibility = ["//visibility:private"]` - - Flag helpers that do **not** have a `manual` tag to avoid wildcard expansion (`:all`, `:...`, etc.) -- Flag macros that use the `name` parameter to derive dependencies or input files not generated by the macro itself — `name` should only be used to name generated targets -- Flag macro parameters whose names differ from the corresponding rule attributes (e.g., using `src` instead of `srcs` when passing to `srcs`) -- Flag calls to macros that use positional arguments instead of keyword-only arguments - -#### Rule and Aspect Design -- Rule, aspect, and attribute names must be `snake_case` -- Rule names should be nouns that clearly describe the artifact produced (e.g., `*_library`, `*_binary`, `*_test`, `*_import`) -- Common attributes must follow standard naming and types: - - `srcs`: `label_list`, typically allowing files (source files) - - `deps`: `label_list`, typically **not** allowing files (compilation dependencies) - - `data`: `label_list`, allowing files (runtime/test data) - - `runtime_deps`: `label_list` for deps not needed at compile time -- Flag attributes with non-obvious behavior (string templates, special tool requirements) that lack a `doc` string -- Rule implementation functions should be private (leading underscore), e.g., `_myrule_impl` -- Rules must pass data via well-defined providers; flag providers or provider fields that are undeclared or undocumented -- Flag rule designs that are not extensible — other rules should be able to interact with providers and reuse actions -- Flag rule implementations that violate performance guidelines (e.g., unnecessary analysis-phase work, ignoring caching opportunities) - -#### Tooling and Maintainability -- Flag `.bzl` or BUILD files that are not formatted with **Buildifier** (both formatter and linter) -- Flag missing or inadequate tests for macros and rules — test coverage should follow the project's testing guidelines +# Starlark Code Review Guide + +> Guidance for AI reviewers. Starlark is a deterministic, hermetic configuration +> language (a dialect of Python) used primarily by Bazel. It is **not** Python: +> the syntax is a strict subset, but the semantics differ, and many Python +> features are intentionally absent. Unless noted, this guide assumes Bazel's +> Starlark dialect. + +## Obvious Typos or Spelling Errors + +- Spelling errors in variable, function, provider, rule, attribute, and target + names **at their declaration sites**; do not report spelling errors at call + sites. +- Spelling errors in `fail()` or `print()` messages, comments, and docstrings + that affect readability. +- Typos in `load()` symbol names and label strings (these can cause load or + analysis failures). +- Misspelled attribute names in rule or macro calls (for example `sorce` instead + of `srcs`), because they are usually a runtime error or are silently ignored. + +## Determinism and Purity + +Starlark is designed to be deterministic and side-effect free. Review code that +could break hermeticity. + +**Key checks:** + +- No reliance on wall-clock time, randomness, host filesystem state, + environment variables, or network access in ordinary `.bzl` or `BUILD` code. +- Rule implementations must produce the same result for the same inputs and + `ctx`. +- Repository rules may perform network or filesystem access, but their result + must still be deterministic for a given set of inputs. +- No `print()` in production code (see Error Reporting). + +## Recursion and Termination + +**Key checks:** + +- No direct or mutual recursion; Starlark rejects recursion at runtime. +- No `while` loops; Starlark has only `for` loops over finite sequences. +- Starlark `for` loops always terminate because they iterate over finite + collections; `break` and `continue` cannot make a loop non-terminating, so + flag excessive iteration cost instead (see Depsets and Efficiency). +- Avoid deeply nested comprehensions that make termination or cost hard to + reason about. + +## Frozen Values and Mutation + +Starlark favors immutability. Only `list` and `dict` are mutable, and only +inside the evaluation context that created them. Bazel 8.1 also added a +mutable core `set` type, so `set` values are subject to the same rules. + +**Key checks:** + +- Do not mutate a value loaded from another `.bzl` file, a rule attribute, or a + value returned by a rule or provider; those values are frozen. +- Do not mutate a collection while iterating over it (a dynamic error). +- Avoid mutable default arguments; defaults are evaluated once and shared + across calls. +- Do not rely on object identity; Starlark has no `is` operator. + +**Example:** + +```python +# Bad: mutates a frozen value +# other.bzl +load(":defs.bzl", "registry") + +def use(): + registry.append("x") # runtime error: registry is frozen + +# Good: the function owns the mutable value; loaded module globals are frozen +# after evaluation and cannot be mutated from another file +def register(name): + registry = [] + registry.append(name) + return registry +``` + +```python +# Bad: mutable default argument is shared between calls +def add(name, names=[]): + names.append(name) + return names + +# Good: use None and create the list inside +def add(name, names=None): + names = names or [] + names.append(name) + return names +``` + +## Safe Data Handling (Python Differences) + +**Key checks:** + +- Ordered comparisons (`<`, `<=`, `>`, `>=`) are defined only within a single + value type; `==` and `!=` may compare across types. +- Strings are not iterable; use `s.elems()` to iterate over characters. +- Dictionary literals cannot contain duplicate keys. +- Dictionary iteration order is deterministic. +- There is no implicit string concatenation; use `+`. +- In Bazel, `int` is limited to 32-bit signed values and overflow is an error. +- Use `struct()` instead of `class`; use `provider()` for rule outputs. +- Define named functions with `def`; Bazel's dialect has no `lambda`. +- There is no `import`; use `load()`, placed at the top of the file. +- There is no `global` or `nonlocal`. + +**Example:** + +```python +# Bad: Python-style exception handling does not exist +def lookup(data, key): + try: + return data[key] + except KeyError: + return None + +# Good: check first and fail with a clear message +def lookup(data, key): + if key not in data: + fail("missing key %r in %r" % (key, data)) + return data[key] +``` + +## Depsets and Efficiency + +**Key checks:** + +- Aggregate transitive information with `depset`; do not flatten transitive + depsets with `to_list()` inside a loop, which is O(N^2) over the dependency + graph. +- Avoid `depset.to_list()` except for debugging. +- Build a depset in a single call; do not construct depsets inside a loop. +- Use `ctx.actions.args()` for command lines instead of flattening depsets or + building giant strings. +- Pass depsets directly as action inputs; do not convert them to lists. + +**Example:** + +```python +# Bad: repeatedly flattening depsets with to_list() is O(N^2) +files = [] +for dep in ctx.attr.deps: + files += dep[MyProvider].files.to_list() + +# Good: keep the depsets nested and merge once +files = depset( + direct = ctx.files.srcs, + transitive = [dep[MyProvider].files for dep in ctx.attr.deps], +) +``` + +## Error Reporting + +**Key checks:** + +- Use `fail()` to report validation errors and unrecoverable conditions. +- `print()` is for debugging only; it must not appear in production code, or it + must be gated behind a hardcoded `DEBUG` flag. +- Error messages should name the offending value and the expected condition. +- Do not silently swallow errors, and do not return `None` where the caller + expects a value without documenting that behavior. + +## Loads, Symbols, and Encapsulation + +**Key checks:** + +- Use `load()` (not `import`), with all loads at the top of the file. +- Minimize exported symbols per `.bzl` file; mark file-private top-level + symbols with a leading `_`. +- Remove unused loads, variables, and parameters. +- Prefer rules over macros; macros expand before graph analysis and hide the + real build graph. +- Flag macro-generated helper targets that are not prefixed with the main + target's `name` (for example `name_bar` or `_name_bar`). +- Flag helper targets that lack `visibility = ["//visibility:private"]` or a + `manual` tag; without `manual`, wildcard patterns such as `:all` and `:...` + expand them. +- Keep `BUILD` files simple and explicit (DAMP over DRY); avoid top-level list + comprehensions and shared `deps` variables. + +## Naming Conventions + +**Requirements:** + +- Variables, functions, rules, attributes, and targets use `snake_case`. +- Constants use `UPPER_SNAKE_CASE`. +- File-private top-level symbols start with `_`. +- Rule and macro implementation functions are named `__impl`. +- Rule names are nouns describing the produced artifact; common suffixes + include `_library`, `_binary`, `_test`, and `_import`. +- Common attributes follow standard names and types: `srcs` is a `label_list` + (typically source files), `deps` is a `label_list` (typically *not* files), + `data` is a `label_list` (runtime/test data), and `runtime_deps` is a + `label_list` for dependencies not needed at compile time. +- A macro's `name` parameter is first, and generated targets are prefixed with + `name` plus `_`. +- Call rules and macros with keyword arguments, spaces around `=`, and `name` + first. + +## Documentation + +- Add a docstring at the top of each `.bzl` file and for each public function. +- Document rules, aspects, providers, and attributes using the `doc` argument. +- Rules pass data through well-defined providers; flag provider fields that + are undeclared or undocumented. +- Use comments to explain *why*, not what the code does. + +## Formatting + +- Formatting must match `buildifier`; do not nitpick formatting it would + produce. +- Prefer double quotes for strings. +- Use four-space indentation. +- Use a single blank line between top-level definitions. + +## Do Not Report + +- Code that already avoids Python-isms correctly, such as using `==` instead of + `is`, or `fail()` instead of `try`/`except`. +- Formatting that `buildifier` is responsible for. +- Spelling errors at call sites rather than declaration sites. +- The absence of features Starlark intentionally omits (`class`, `while`, + `import`, recursion, and so on). From 9f2dabd515806225b4eb27e1a160057bbc46ce33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A5=88=E6=84=BFQiii?= <2297740147@qq.com> Date: Thu, 20 Aug 2026 23:19:35 +0800 Subject: [PATCH 3/3] Update internal/config/rules/system_rules_test.go Co-authored-by: Tao Xin --- internal/config/rules/system_rules_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 4a9e4c678..1ab3db497 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -135,11 +135,11 @@ func TestResolve_DefaultRules(t *testing.T) { {"if/common.thrift", "Field IDs and Wire Compatibility"}, {"schema/addressbook.capnp", "Ordinals and Wire Compatibility"}, {"src/rpc.capnp", "Ordinals and Wire Compatibility"}, - {"rules/library.bzl", "BUILD File Simplicity and Abstraction"}, - {"config.star", "BUILD File Simplicity and Abstraction"}, - {"MODULE.bazel", "BUILD File Simplicity and Abstraction"}, - {"pkg/BUILD", "BUILD File Simplicity and Abstraction"}, - {"WORKSPACE", "BUILD File Simplicity and Abstraction"}, + {"rules/library.bzl", "Starlark Code Review Guide"}, + {"config.star", "Starlark Code Review Guide"}, + {"MODULE.bazel", "Starlark Code Review Guide"}, + {"pkg/BUILD", "Starlark Code Review Guide"}, + {"WORKSPACE", "Starlark Code Review Guide"}, } for _, tt := range tests {