From f372b8745449707680d9a7f29cae0fca8dc84655 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 1 Jul 2026 09:31:07 -0700 Subject: [PATCH 01/16] Add design doc for attribute-policy linting and test-tuning Design for two related features: - A generalized, config-driven `attr-policy` buildifier warning (static), covering eternal-timeout allow-lists, forbidden test tags, and future attribute/rule-kind constraints via .buildifier.json. - A separate `testpolicy` tool that reads test-execution stats from a metrics warehouse and applies timeout/flaky recommendations via buildozer. Includes background on the current warning architecture, config schema, wiring points, a flakiness-scoring model, phased task breakdown, and open questions for review. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...attribute-policy-and-test-tuning-design.md | 453 ++++++++++++++++++ 1 file changed, 453 insertions(+) create mode 100644 docs/attribute-policy-and-test-tuning-design.md diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md new file mode 100644 index 000000000..ccba62b73 --- /dev/null +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -0,0 +1,453 @@ +# Design: Attribute-Policy Linting & Empirical Test-Tuning + +Status: **Draft for review** +Owner: (tbd) +Last updated: 2026-07-01 + +--- + +## 1. Summary + +We want two related capabilities for Bazel `BUILD` files: + +1. **Static attribute-policy linting** in `buildifier` — enforce declarative rules about + attribute values, e.g. forbid `timeout = "eternal"` unless the target is on an + approved allow-list, forbid `exclusive` in a test's `tags`, and similar + attribute/rule-kind constraints. Purely static, config-driven, no runtime data. + +2. **Empirical test-tuning** in a **new, separate tool** (`testpolicy`) — read + historical test-execution stats from a metrics warehouse, compute recommended + `timeout` and `flaky` values (and a flakiness score / recommended retry count), + and apply them via `buildozer` commands / PRs. `buildifier` never touches the + warehouse. + +These are split deliberately: `buildifier` stays a fast, hermetic, offline static +linter; all data-dependent analysis lives in a tool that can query a warehouse and +open PRs. + +### Decisions locked in (from design review) + +| Question | Decision | +|---|---| +| Source of empirical data | Metrics DB / warehouse (queried by the new tool) | +| How empirical recommendations are applied | `buildozer` commands + PRs; `buildifier` stays purely static | +| Where policy config lives | Extend the existing `.buildifier.json` config | +| Upstream Bazel work (`flaky=N`, parallel attempts) | **Out of scope**; documented as a future RFC | + +--- + +## 2. Background: how `buildifier` warnings work today + +(Reference for implementing agents. File paths are current as of this draft.) + +- **Warnings are pure functions** over one parsed file. Three signatures / + registries in `warn/warn.go`: + - `FileWarningMap: map[string]func(f *build.File) []*LinterFinding` + - `MultiFileWarningMap: map[string]func(f *build.File, fileReader *FileReader) []*LinterFinding` + - `RuleWarningMap: map[string]func(call *build.CallExpr, pkg string) *LinterFinding` +- **Findings** are created with `makeLinterFinding(node, message, ...LinterReplacement)` + (autofix optional). Nodes carry positions via `.Span()`. +- **Rule/attribute access** (`build/rule.go`): + - `f.Rules(kind string) []*Rule` (`kind == ""` → all) + - `rule.Kind() string`, `rule.Name() string`, `rule.ExplicitName() string` + - `rule.Attr(key) Expr`, `rule.AttrString(key) string`, `rule.AttrStrings(key) []string` + - `rule.AttrDefn(key) *AssignExpr` +- **Config-derived globals** already exist: the `tables` package holds process-global + overrides loaded from config (`tables.ParseAndUpdateJSONDefinitions`, applied in + `buildifier/config/config.go` `Validate()` around lines 214–231). We mirror this + pattern for policy config. +- **Config file**: `.buildifier.json` → `buildifier/config.Config` (struct at + `buildifier/config/config.go:93`). Located via `-config` flag, + `BUILDIFIER_CONFIG` env var, or workspace root. `Validate()` applies side effects + (like loading tables). Lint entry point is `buildifier/utils/utils.go:126` + `Lint(...)` → `warn.FileWarnings(...)`. +- **Suppression**: `# buildifier: disable=` comments already work for + every registered warning; the new warning gets this for free. +- **Label utilities** (`labels/labels.go`): `labels.Parse(target) Label`, + `labels.Equal(l1, l2, pkg) bool`; `Label{Repository, Package, Target}`. +- **Tests**: `warn/*_test.go` use `checkFindingsAndFix(t, categories, input, output, + expected, scope)` and scope constants (`scopeBuild`, `scopeBzl`, ...) from + `warn/warn_test.go`. + +--- + +## 3. Workstream A — `attr-policy` warning (buildifier) + +### 3.1 Goal + +A **single generalized** warning, `attr-policy`, driven entirely by config. It covers +the initial asks (eternal-timeout allow-list, no `exclusive` on tests) and any future +"attribute constrained on rule-kind unless allow-listed" rule **without new Go code**. + +Rationale for one generalized warning vs. many hardcoded ones: buildifier warnings are +individually toggleable by name, but the *policy content* here is site-specific and +belongs in config, not in the binary. One warning + rich config keeps the binary +generic and lets each repo express its own policy. + +### 3.2 Config schema (extends `.buildifier.json`) + +```jsonc +{ + "attrPolicy": { + "rules": [ + { + "name": "no-eternal-timeout", // stable id, shown in the finding + "ruleKinds": ["*_test"], // globs matched against rule.Kind(); omit/[] = any kind + "attr": "timeout", + "forbidValues": ["eternal"], // scalar-attr constraint + "allowlist": ["//slow/...", "//foo:big_test"], + "message": "'eternal' timeout requires approval; add the target to the attrPolicy allowlist." + }, + { + "name": "no-exclusive-tests", + "ruleKinds": ["*_test"], + "attr": "tags", + "forbidListItems": ["exclusive"], // list-membership constraint + "allowlist": [] + } + ] + } +} +``` + +**Per-rule fields:** + +| Field | Type | Meaning | +|---|---|---| +| `name` | string (required) | Stable identifier, included in the finding message. Must be unique. | +| `ruleKinds` | []string | Globs matched against `rule.Kind()`. Empty/absent ⇒ matches all kinds. | +| `attr` | string (required) | Attribute to inspect. | +| `forbidValues` | []string | Scalar attr must not equal any of these. | +| `requireValues` | []string | If attr present, must equal one of these. (If also want "must be present", see `required`.) | +| `forbidListItems` | []string | List attr must not contain any of these items. | +| `requireListItems` | []string | List attr must contain all of these items. | +| `required` | bool | Attr must be present at all. | +| `allowlist` | []string | Labels exempt from this rule. Exact labels or `//pkg/...` recursive globs. | +| `message` | string | Custom message. If absent, a default is synthesized from the constraint. | + +Exactly one *constraint family* (`forbidValues`/`requireValues` **or** +`forbidListItems`/`requireListItems`) should be set per rule; `required` is +orthogonal. Validation enforces this (see 3.5). + +**Semantics:** +- Empty/absent `attrPolicy` ⇒ warning is a no-op. Safe to enable in `--warnings=all`. +- A target matches a policy rule if its `Kind()` matches any `ruleKinds` glob **and** + its full label is **not** in `allowlist`. +- Full label computed as `//{f.Pkg}:{rule.Name()}` then compared with `labels.Equal` + (handles `:name` == package-dir shorthand). Recursive `//pkg/...` entries match any + target whose package is `pkg` or under it. +- Finding is anchored on the offending attribute node + (`rule.Attr(attr).Span()`); if the constraint is `required` and the attr is missing, + anchor on `rule.Call`. + +### 3.3 Go types + +New file `buildifier/config/attrpolicy.go` (or inline in `config.go`) — keep JSON tags: + +```go +type AttrPolicy struct { + Rules []AttrPolicyRule `json:"rules,omitempty"` +} + +type AttrPolicyRule struct { + Name string `json:"name"` + RuleKinds []string `json:"ruleKinds,omitempty"` + Attr string `json:"attr"` + ForbidValues []string `json:"forbidValues,omitempty"` + RequireValues []string `json:"requireValues,omitempty"` + ForbidListItems []string `json:"forbidListItems,omitempty"` + RequireListItems []string `json:"requireListItems,omitempty"` + Required bool `json:"required,omitempty"` + Allowlist []string `json:"allowlist,omitempty"` + Message string `json:"message,omitempty"` +} +``` + +Add to `Config` struct (`buildifier/config/config.go:93`): + +```go +AttrPolicy *AttrPolicy `json:"attrPolicy,omitempty"` +``` + +### 3.4 Wiring config → warning (mirror the `tables` pattern) + +The warning function has signature `func(f *build.File) []*LinterFinding` and cannot +take config directly (would churn 100+ warning signatures). Instead use a package-level +global in `warn`, set once during config application — exactly how `tables` works. + +- In `warn/warn_attr_policy.go`, define: + ```go + // AttrPolicyConfig is process-global policy, set from buildifier config before linting. + var AttrPolicyConfig []AttrPolicyRuleCompiled // compiled form (globs precompiled) + + func SetAttrPolicy(rules []AttrPolicyRuleCompiled) { AttrPolicyConfig = rules } + ``` + To avoid an import cycle (`warn` must not import `buildifier/config`), define the + *compiled* policy type in the `warn` package and have the config layer translate + `config.AttrPolicy` → `[]warn.AttrPolicyRuleCompiled` and call `warn.SetAttrPolicy`. +- Apply in `buildifier/config/config.go` `Validate()` (next to the tables block, + ~line 231): if `c.AttrPolicy != nil`, compile and call `warn.SetAttrPolicy(...)`. + Return a validation error on malformed rules. + +> **Import-cycle note for agents:** confirm direction. `buildifier/config` may import +> `warn`; `warn` must not import `buildifier/config`. If `warn` already imports config +> anywhere, keep the compiled type in `warn` regardless. This is the one wiring risk — +> verify before writing code. + +### 3.5 Validation (`buildifier/config/validation.go`) + +On load, reject: +- duplicate `name`s, +- empty `name` or empty `attr`, +- a rule with no constraint set, +- a rule mixing scalar and list constraint families, +- malformed globs in `ruleKinds` / malformed labels in `allowlist`. + +Emit clear errors (`fmt.Errorf("attrPolicy rule %q: ...", name)`). + +### 3.6 Warning implementation sketch (`warn/warn_attr_policy.go`) + +```go +func attrPolicyWarning(f *build.File) []*LinterFinding { + if f.Type != build.TypeBuild || len(AttrPolicyConfig) == 0 { + return nil + } + var findings []*LinterFinding + for _, rule := range f.Rules("") { + kind := rule.Kind() + label := labels.Label{Package: f.Pkg, Target: rule.Name()}.Format() + for _, p := range AttrPolicyConfig { + if !p.matchesKind(kind) || p.allowed(label, f.Pkg) { + continue + } + if fnd := p.check(rule, f); fnd != nil { + findings = append(findings, fnd) + } + } + } + return findings +} +``` + +- `p.check` handles the constraint families using `rule.AttrString` / `rule.AttrStrings` + and `rule.Attr(attr)` for the anchor node; returns `makeLinterFinding(node, msg)`. +- Scope: BUILD files only (targets live there). Return `nil` for other file types. +- **Autofix: none initially.** We cannot infer a correct scalar value. The one + mechanically-safe fix (remove a forbidden list item / forbidden tag) is a good + fast-follow via `LinterReplacement`; leave a `// TODO(attr-policy): autofix remove-item`. + +### 3.7 Registration, docs, tests + +- Register in `warn/warn.go`: `FileWarningMap["attr-policy"] = attrPolicyWarning`. +- Decide default-on vs. opt-in: add to `nonDefaultWarnings` if it should be opt-in + (recommended, since it no-ops without config anyway — but being config-gated it's + harmless in the default set too; pick opt-in to be conservative). +- Docs: add an entry to `WARNINGS.md` and `warn/docs/warnings.textproto` describing the + warning **and** the `attrPolicy` config block (with the two example rules). +- Add a `-config=example` sample entry so `buildifier -config=example` prints an + `attrPolicy` stub (see `config.Example()`). +- Tests `warn/warn_attr_policy_test.go`: + - Set `warn.SetAttrPolicy(...)` in the test, then use `checkFindings`/ + `checkFindingsAndFix` with `scopeBuild`. + - Cases: forbidden scalar value flagged; allow-listed target exempt; recursive + `//pkg/...` allow-list; forbidden list item in `tags`; `ruleKinds` glob match & + non-match; missing-when-`required`; `# buildifier: disable=attr-policy` suppression; + empty config = no findings. + - Config tests in `buildifier/config/config_test.go`: parse the sample JSON; + validation rejects malformed rules. + +### 3.8 Acceptance criteria (Workstream A) + +- `buildifier --lint=warn --warnings=attr-policy BUILD` flags eternal timeout on + non-allow-listed test targets and `exclusive` tags on tests, given the sample config. +- Zero findings when `attrPolicy` is absent. +- All new + existing `warn` and `config` tests pass; `WARNINGS.md` regeneration (if + applicable) is consistent. + +--- + +## 4. Workstream B — `testpolicy` tool (empirical tuning) + +A **new binary** beside `buildifier`/`buildozer`. It never runs inside buildifier and +never blocks pre-commit. + +### 4.1 Layout + +``` +testpolicy/ + main.go // CLI: window, filters, dry-run/report/apply modes + source/ // warehouse adapter + source.go // interface + data model + bigquery.go // concrete impl (behind a build tag / flag) + analyze/ + timeout.go // timeout recommender + flaky.go // flakiness scoring + flaky/attempt recommender + emit/ + buildozer.go // emit buildozer command script + pr.go // group by owner, open PRs (later phase) + report/ // human-readable + machine (JSON) report +``` + +### 4.2 Warehouse adapter interface (data-source-agnostic) + +```go +// source/source.go +type TargetStats struct { + Label string + Runs int + DurationP50 time.Duration + DurationP95 time.Duration + DurationMax time.Duration + DeclaredTimeout string // as observed in runs, if available + TimeoutFailures int // failures attributed to hitting the timeout + // Per-attempt outcomes for flakiness math: + Attempts int // total attempts observed + PassByAttempt []float64 // PassByAttempt[k] = P(pass by attempt k+1), empirical +} + +type Source interface { + // Query returns stats for targets matching the filter over [since, until]. + Query(ctx context.Context, since, until time.Time, filter Filter) ([]TargetStats, error) +} +``` + +Keeping this interface is what makes the "Metrics DB / warehouse" decision concrete +while letting the query backend be swapped/tested with a fake. + +### 4.3 Timeout recommender (`analyze/timeout.go`) + +- Bazel buckets: `short≈60s`, `moderate≈300s`, `long≈900s`, `eternal≈3600s`. +- Recommend the **smallest bucket** where `DurationP95 * safetyFactor` fits (default + `safetyFactor = 1.5`, configurable). +- **Bump up** if either: observed runtime is within `X%` (default 20%) of the current + limit, **or** `TimeoutFailures > 0`. +- **Never recommend `eternal`** unless the target is on the eternal allow-list (shared + with Workstream A's config so policy stays single-sourced). If data says a target + needs > `long` and isn't allow-listed, emit a **report finding for a human**, not an + auto-edit. +- Output per target: `{current, recommended, reason}`. + +### 4.4 Flakiness scoring (`analyze/flaky.go`) + +Answers "does a single retry likely pass, or does it need multiple?". + +- Let `a = P(a single attempt passes | not chronically broken)`, estimated from history + (prefer empirical `PassByAttempt` when present; else independence model). +- Under independence: `P(pass within N) = 1 - (1-a)^N`. To reach target pass-rate `T`: + + ``` + N ≥ ceil( log(1 - T) / log(1 - a) ) + ``` + +- Classification: + | N | Meaning | Recommendation | + |---|---|---| + | ≤ 1 | not flaky | `flaky = False` (or leave unset) | + | 2 | one retry suffices | `flaky = True` | + | 3 | within Bazel default cap (3 attempts) | `flaky = True` | + | > 3 | needs more than Bazel allows today | `flaky = True` **+ report**: recommend `--flaky_test_attempts` / future `flaky=N` | + | very low `a` | chronically broken | **do not** mask with retries; report for owner | + +- **Reality check (must be in tool output & docs):** Bazel's `flaky` is a **boolean** + today (retries up to 3, **sequentially**). `flaky = 2` is not valid syntax; there is + no per-target attempt count and no parallel-attempt option. The tool sets + `flaky = True/False` now and *records* the recommended attempt count `N` in the + report. Real per-target counts + parallel attempts require an upstream Bazel change + (§6), which is out of scope. + +### 4.5 Emit (`emit/`) + +- **Phase 2 (first):** `report`/`dry-run` only — print recommendations + reasons; emit a + `buildozer` command script to stdout/file, e.g.: + ``` + buildozer 'set timeout "long"' //pkg:target + buildozer 'set flaky True' //pkg:other + ``` +- **Phase 3:** `apply` mode runs buildozer and/or opens PRs grouped by CODEOWNERS. +- Always emit a machine-readable JSON report for auditability. + +### 4.6 Safety / guardrails + +- Require a minimum `Runs` sample size before recommending (default e.g. 20); otherwise + report "insufficient data". +- Only ever *raise* timeouts automatically; *lowering* a timeout is report-only (risk of + new flakes) unless `--allow-lowering`. +- Respect the eternal allow-list from `.buildifier.json` so the two systems agree. +- Dry-run is the default mode. + +### 4.7 Acceptance criteria (Workstream B) + +- With a fake `Source`, `testpolicy report` produces correct timeout & flaky + recommendations and a valid buildozer script for a fixture dataset. +- No warehouse credentials required for tests (fake source). +- `apply` mode is gated behind an explicit flag and off by default. + +--- + +## 5. Interaction between the two workstreams + +- The **eternal-timeout allow-list lives once** in `.buildifier.json` (`attrPolicy`). + Both `buildifier` (enforce) and `testpolicy` (never recommend eternal off-list) read + it. Consider a tiny shared loader package so the schema isn't duplicated. +- `buildifier` enforces the *invariants*; `testpolicy` proposes the *values*. A + `testpolicy`-generated PR must itself pass `attr-policy` lint — i.e. the tool won't + propose an edit that buildifier would reject. + +--- + +## 6. Out of scope (future Bazel RFC) + +- Per-target flaky **attempt count** (`flaky = N`). +- **Parallel** retry attempts (spawn N attempts at once; pass if any passes) instead of + sequential retry-on-failure. +- These require changes to Bazel itself (attribute semantics + test execution). We + document the desired end-state and the recommended `N` values the `testpolicy` tool + computes, so an RFC has data behind it. Not built here. + +--- + +## 7. Phasing & work breakdown (for agent hand-off) + +Each task is independently ownable; dependencies noted. "AC" = acceptance criteria above. + +### Phase 1 — buildifier `attr-policy` (Workstream A) +- **A1. Config types + parsing** — add `AttrPolicy`/`AttrPolicyRule` to + `buildifier/config`, JSON round-trip test. *(no deps)* +- **A2. Validation** — `buildifier/config/validation.go` rules from §3.5. *(dep: A1)* +- **A3. Warning + compiled policy type + global** — `warn/warn_attr_policy.go`, + glob & allow-list matching, register in `warn/warn.go`. *(no deps; can stub config)* +- **A4. Config→warn wiring** — compile in `Validate()`, resolve import-cycle question + (§3.4 note). *(dep: A1, A3)* +- **A5. Tests** — `warn/warn_attr_policy_test.go` cases from §3.7. *(dep: A3/A4)* +- **A6. Docs** — `WARNINGS.md`, `warn/docs/warnings.textproto`, `-config=example` + stub. *(dep: A1, A3)* + +### Phase 2 — `testpolicy` skeleton + timeout recommender (Workstream B) +- **B1. Tool scaffold + CLI + fake source** — `testpolicy/main.go`, `source` interface, + in-memory fake. *(no deps)* +- **B2. Timeout recommender** — §4.3 + tests on fixtures. *(dep: B1)* +- **B3. buildozer emitter + report** — §4.5 dry-run path. *(dep: B1)* + +### Phase 3 — flakiness + application +- **B4. Flakiness scoring** — §4.4 + tests. *(dep: B1)* +- **B5. Warehouse (BigQuery/DB) source impl** — behind flag. *(dep: B1)* +- **B6. `apply` mode + PR grouping (CODEOWNERS)** — §4.5. *(dep: B2/B3/B4)* + +### Phase 4 — future +- **C1. Bazel RFC** for `flaky=N` + parallel attempts (§6). *(data from B4)* + +--- + +## 8. Open questions + +1. **Import direction** between `warn` and `buildifier/config` — confirm no cycle + (§3.4). If one exists, the compiled-policy-type-in-`warn` approach resolves it. +2. Should `attr-policy` be **default-on** (config-gated no-op) or opt-in via + `--warnings`? (Leaning opt-in.) +3. Warehouse **schema/columns** available for `TargetStats` — especially whether + per-attempt outcomes (`PassByAttempt`) exist, or only aggregate pass/fail. This + changes flakiness estimation fidelity. +4. Timeout **safety factor** and bucket cutoffs — defaults proposed; confirm with SRE/CI. +5. PR routing — CODEOWNERS-based? One PR per owner, or batched? +6. Do we want the shared eternal-allow-list loader as its own small package now, or + duplicate-read for Phase 1 and refactor later? From 0b440b1b81641138cfc916b5054bbe84bb23ff1c Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 1 Jul 2026 10:00:25 -0700 Subject: [PATCH 02/16] Drop per-target flaky attempts and parallelism from design These referenced future Bazel-side features (flaky=N, parallel retry attempts) that distracted from the buildifier/buildozer scope. Flakiness scoring now only drives a boolean flaky recommendation. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...attribute-policy-and-test-tuning-design.md | 43 ++++++------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index ccba62b73..981c9d001 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -17,9 +17,8 @@ We want two related capabilities for Bazel `BUILD` files: 2. **Empirical test-tuning** in a **new, separate tool** (`testpolicy`) — read historical test-execution stats from a metrics warehouse, compute recommended - `timeout` and `flaky` values (and a flakiness score / recommended retry count), - and apply them via `buildozer` commands / PRs. `buildifier` never touches the - warehouse. + `timeout` and `flaky` values (and a flakiness score), and apply them via + `buildozer` commands / PRs. `buildifier` never touches the warehouse. These are split deliberately: `buildifier` stays a fast, hermetic, offline static linter; all data-dependent analysis lives in a tool that can query a warehouse and @@ -32,7 +31,6 @@ open PRs. | Source of empirical data | Metrics DB / warehouse (queried by the new tool) | | How empirical recommendations are applied | `buildozer` commands + PRs; `buildifier` stays purely static | | Where policy config lives | Extend the existing `.buildifier.json` config | -| Upstream Bazel work (`flaky=N`, parallel attempts) | **Out of scope**; documented as a future RFC | --- @@ -339,21 +337,18 @@ Answers "does a single retry likely pass, or does it need multiple?". N ≥ ceil( log(1 - T) / log(1 - a) ) ``` -- Classification: - | N | Meaning | Recommendation | +- `N` is used internally to distinguish "a single retry almost always recovers it" + from "retries rarely help", which drives the `flaky` recommendation: + | `N` | Meaning | Recommendation | |---|---|---| | ≤ 1 | not flaky | `flaky = False` (or leave unset) | - | 2 | one retry suffices | `flaky = True` | - | 3 | within Bazel default cap (3 attempts) | `flaky = True` | - | > 3 | needs more than Bazel allows today | `flaky = True` **+ report**: recommend `--flaky_test_attempts` / future `flaky=N` | + | 2–3 | retries reliably recover it | `flaky = True` | + | > 3 | retries rarely recover it | report for owner; `flaky` won't reliably help | | very low `a` | chronically broken | **do not** mask with retries; report for owner | -- **Reality check (must be in tool output & docs):** Bazel's `flaky` is a **boolean** - today (retries up to 3, **sequentially**). `flaky = 2` is not valid syntax; there is - no per-target attempt count and no parallel-attempt option. The tool sets - `flaky = True/False` now and *records* the recommended attempt count `N` in the - report. Real per-target counts + parallel attempts require an upstream Bazel change - (§6), which is out of scope. +- **Reality check (must be in tool output & docs):** Bazel's `flaky` is a **boolean**; + the tool only ever recommends `flaky = True` / `flaky = False`. `N` is an internal + signal for classification, not something written into the BUILD file. ### 4.5 Emit (`emit/`) @@ -395,18 +390,7 @@ Answers "does a single retry likely pass, or does it need multiple?". --- -## 6. Out of scope (future Bazel RFC) - -- Per-target flaky **attempt count** (`flaky = N`). -- **Parallel** retry attempts (spawn N attempts at once; pass if any passes) instead of - sequential retry-on-failure. -- These require changes to Bazel itself (attribute semantics + test execution). We - document the desired end-state and the recommended `N` values the `testpolicy` tool - computes, so an RFC has data behind it. Not built here. - ---- - -## 7. Phasing & work breakdown (for agent hand-off) +## 6. Phasing & work breakdown (for agent hand-off) Each task is independently ownable; dependencies noted. "AC" = acceptance criteria above. @@ -433,12 +417,9 @@ Each task is independently ownable; dependencies noted. "AC" = acceptance criter - **B5. Warehouse (BigQuery/DB) source impl** — behind flag. *(dep: B1)* - **B6. `apply` mode + PR grouping (CODEOWNERS)** — §4.5. *(dep: B2/B3/B4)* -### Phase 4 — future -- **C1. Bazel RFC** for `flaky=N` + parallel attempts (§6). *(data from B4)* - --- -## 8. Open questions +## 7. Open questions 1. **Import direction** between `warn` and `buildifier/config` — confirm no cycle (§3.4). If one exists, the compiled-policy-type-in-`warn` approach resolves it. From d4b8142438d9a79b964d7b26993c8c0ef46a963f Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 1 Jul 2026 10:05:47 -0700 Subject: [PATCH 03/16] Fix allow-list pattern semantics; add suppression & enforcement - Allow-list now specifies a Bazel-style target-pattern grammar (exact, :all, /...) with a dedicated matcher, since labels.Equal is exact-only and no in-memory pattern matcher exists in the repo. Fixes the mismatch between the //slow/... example and the labels.Equal-based description. - New "Suppression & enforcement" section: two-tier model (suppressible local linter + authoritative CI gate), per-rule `suppressible` config field, suppression audit, and the optional NonSuppressible core change as an open question. Adds A7/A8 enforcement tasks. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...attribute-policy-and-test-tuning-design.md | 112 ++++++++++++++++-- 1 file changed, 103 insertions(+), 9 deletions(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 981c9d001..2d9b693db 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -120,20 +120,37 @@ generic and lets each repo express its own policy. | `forbidListItems` | []string | List attr must not contain any of these items. | | `requireListItems` | []string | List attr must contain all of these items. | | `required` | bool | Attr must be present at all. | -| `allowlist` | []string | Labels exempt from this rule. Exact labels or `//pkg/...` recursive globs. | +| `allowlist` | []string | Target patterns exempt from this rule (see grammar below). | +| `suppressible` | bool (default `true`) | Whether `# buildifier: disable=attr-policy` silences this rule. `false` = a "hard" rule enforced authoritatively by CI regardless of in-file comments (see §6). | | `message` | string | Custom message. If absent, a default is synthesized from the constraint. | Exactly one *constraint family* (`forbidValues`/`requireValues` **or** `forbidListItems`/`requireListItems`) should be set per rule; `required` is orthogonal. Validation enforces this (see 3.5). +**Allow-list pattern grammar.** Entries are Bazel-style target patterns (a subset of +what users already know from `bazel build`): + +| Pattern | Matches | +|---|---| +| `//pkg:name` or `//pkg` | exactly the target `//pkg:name` (`//pkg` ⇒ `//pkg:pkg`) | +| `//pkg:all` (also `//pkg:*`) | any target *directly* in package `pkg` | +| `//pkg/...` | any target in package `pkg` or any package beneath it (recursive) | +| `//...` | every target | + +There is **no off-the-shelf matcher** for this in the repo — `labels.Equal` is +exact-only, and buildozer's `/...` handling is for filesystem BUILD discovery, not +in-memory label matching. So the warning ships a small matcher (`allowlistMatch`) that +parses each entry once at config-compile time and, for a given target +`//{f.Pkg}:{rule.Name()}`, tests: exact via `labels.Equal`; `:all`/`:*` via package +equality; `/...` via `pkg == P || strings.HasPrefix(pkg, P+"/")`. + **Semantics:** - Empty/absent `attrPolicy` ⇒ warning is a no-op. Safe to enable in `--warnings=all`. - A target matches a policy rule if its `Kind()` matches any `ruleKinds` glob **and** - its full label is **not** in `allowlist`. -- Full label computed as `//{f.Pkg}:{rule.Name()}` then compared with `labels.Equal` - (handles `:name` == package-dir shorthand). Recursive `//pkg/...` entries match any - target whose package is `pkg` or under it. + its label matches **no** entry in `allowlist`. +- Target label computed as `//{f.Pkg}:{rule.Name()}` and tested with `allowlistMatch` + per the grammar above (not a bare `labels.Equal`, which can't express patterns). - Finding is anchored on the offending attribute node (`rule.Attr(attr).Span()`); if the constraint is `required` and the attr is missing, anchor on `rule.Call`. @@ -157,6 +174,7 @@ type AttrPolicyRule struct { RequireListItems []string `json:"requireListItems,omitempty"` Required bool `json:"required,omitempty"` Allowlist []string `json:"allowlist,omitempty"` + Suppressible *bool `json:"suppressible,omitempty"` // pointer so absent ⇒ default true Message string `json:"message,omitempty"` } ``` @@ -176,7 +194,7 @@ global in `warn`, set once during config application — exactly how `tables` wo - In `warn/warn_attr_policy.go`, define: ```go // AttrPolicyConfig is process-global policy, set from buildifier config before linting. - var AttrPolicyConfig []AttrPolicyRuleCompiled // compiled form (globs precompiled) + var AttrPolicyConfig []AttrPolicyRuleCompiled // compiled form: kind globs + allow-list patterns precompiled func SetAttrPolicy(rules []AttrPolicyRuleCompiled) { AttrPolicyConfig = rules } ``` @@ -199,7 +217,8 @@ On load, reject: - empty `name` or empty `attr`, - a rule with no constraint set, - a rule mixing scalar and list constraint families, -- malformed globs in `ruleKinds` / malformed labels in `allowlist`. +- malformed globs in `ruleKinds` / malformed target patterns in `allowlist` (must + match the §3.2 grammar; reject a bare `...`, repository-qualified entries, etc.). Emit clear errors (`fmt.Errorf("attrPolicy rule %q: ...", name)`). @@ -227,6 +246,8 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { } ``` +- `p.allowed` runs the precompiled `allowlistMatch` (§3.2 grammar) over the target + label — exact / `:all` / `/...` — not a bare `labels.Equal`. - `p.check` handles the constraint families using `rule.AttrString` / `rule.AttrStrings` and `rule.Attr(attr)` for the anchor node; returns `makeLinterFinding(node, msg)`. - Scope: BUILD files only (targets live there). Return `nil` for other file types. @@ -390,7 +411,70 @@ Answers "does a single retry likely pass, or does it need multiple?". --- -## 6. Phasing & work breakdown (for agent hand-off) +## 6. Suppression & enforcement + +Every buildifier warning is silenceable with `# buildifier: disable=` (also +`# buildozer: disable=`). Suppression is centralized: `runWarningsFunction` drops any +finding for which `DisabledWarning(f, line, category)` is true (`warn/warn.go:281`), +keyed only on the category string. There is **no per-warning "cannot be suppressed" +flag** today — so out of the box a user can write: + +```python +# buildifier: disable=attr-policy +my_test(name = "x", timeout = "eternal") +``` + +…and the policy evaporates locally. We handle this deliberately rather than fighting it. + +**Reframe:** a `disable=` comment is not a silent back door — it lives in the BUILD +file, shows up in the diff, in `git blame`, and in code review. The governance question +is not "can it be bypassed?" (it can) but "is the bypass visible and attributable?" +(yes). That points to a two-tier model. + +### 6.1 Two-tier enforcement + +1. **buildifier `attr-policy` — fast, local, suppressible.** Normal buildifier UX: + dev-time feedback, silenceable with a visible comment. Advisory; not a gate. +2. **Authoritative CI gate — non-bypassable.** A CI job (naturally hosted in the + `testpolicy` tool, which already loads the policy config) re-evaluates the *same* + `.buildifier.json` policy against the tree and, for rules with `suppressible: false`, + **ignores `disable=` comments entirely**. In-file suppression can't defeat it because + the gate doesn't consult the file's comments for hard rules. This is where + "eternal timeout requires the allow-list, full stop" actually lives. + +Result: the **sanctioned** escape hatch is the allow-list (a reviewed edit to +`.buildifier.json`, ideally CODEOWNER-guarded); the **unsanctioned** one (a `disable=` +comment on a hard rule) is ignored by the gate and surfaced as debt. + +### 6.2 `suppressible` config field + +Per-rule `suppressible` (§3.2, default `true`) tunes this: +- `true` — soft rule; local `disable=attr-policy` silences it (advisory policy). +- `false` — hard rule; the CI gate enforces it regardless of comments. buildifier + locally still shows (and, unless we take the core change in §6.4, still lets users + *locally* suppress) it, but the merge gate is authoritative. + +### 6.3 Suppression audit + +A cheap job/report that inventories every `disable=attr-policy` (and, if we add +sub-scoping, `attr-policy=`) comment across the repo, so escape hatches are +**visible and burn-down-able** instead of accumulating silently. Pairs well with +CODEOWNERS on `.buildifier.json` and on BUILD files so both allow-list edits and +suppressions land on a policy owner. + +### 6.4 Optional core change (open question) + +If we want the *local* linter — not just CI — to refuse suppression for hard rules, +that's a small but real change to buildifier's contract: add a `NonSuppressible bool` +to `LinterFinding` and have `runWarningsFunction` skip the `DisabledWarning` check when +it's set; `attr-policy` sets it per-finding from the rule's `suppressible` flag. Clean +and gives per-rule granularity, **but** it breaks the long-standing invariant that every +warning is silenceable, so it needs buy-in before upstreaming. Recommendation: keep the +CI gate as the real enforcement regardless, and treat this as a nice-to-have. See §8. + +--- + +## 7. Phasing & work breakdown (for agent hand-off) Each task is independently ownable; dependencies noted. "AC" = acceptance criteria above. @@ -417,14 +501,24 @@ Each task is independently ownable; dependencies noted. "AC" = acceptance criter - **B5. Warehouse (BigQuery/DB) source impl** — behind flag. *(dep: B1)* - **B6. `apply` mode + PR grouping (CODEOWNERS)** — §4.5. *(dep: B2/B3/B4)* +### Phase 1b — enforcement (Workstream A, in parallel with Phase 2) +- **A7. Authoritative CI gate** — evaluate policy ignoring `disable=` for + `suppressible: false` rules (§6.1/§6.2); non-zero exit on violation. *(dep: A1–A4)* +- **A8. Suppression audit** — inventory `disable=attr-policy` comments across the + repo as a report (§6.3). *(dep: A3)* + --- -## 7. Open questions +## 8. Open questions 1. **Import direction** between `warn` and `buildifier/config` — confirm no cycle (§3.4). If one exists, the compiled-policy-type-in-`warn` approach resolves it. 2. Should `attr-policy` be **default-on** (config-gated no-op) or opt-in via `--warnings`? (Leaning opt-in.) +3. **`NonSuppressible` core change (§6.4)** — do we extend buildifier so hard rules + can't be silenced by `disable=` locally, accepting a break to the "every warning is + suppressible" contract? Or rely solely on the CI gate? (Leaning CI-gate-only for the + first cut.) 3. Warehouse **schema/columns** available for `TargetStats` — especially whether per-attempt outcomes (`PassByAttempt`) exist, or only aggregate pass/fail. This changes flakiness estimation fidelity. From 18296c7576cd1df4e9139168433b9d995b4d213e Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 1 Jul 2026 10:07:38 -0700 Subject: [PATCH 04/16] Make timeout bucket->seconds a configured input in testpolicy The timeout attribute is a bucket keyword; the seconds each bucket allows default to 60/300/900/3600 but a repo can override them via --test_timeout (usually in .bazelrc). The recommender can't resolve a bucket to seconds without the repo's actual mapping, so introduce a timeoutBuckets config (sourced from .bazelrc or explicit), document the size->timeout implicit fallback, and add an open question about per-config ambiguity. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...attribute-policy-and-test-tuning-design.md | 40 ++++++++++++++----- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 2d9b693db..ff319cda9 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -300,7 +300,7 @@ testpolicy/ bigquery.go // concrete impl (behind a build tag / flag) analyze/ timeout.go // timeout recommender - flaky.go // flakiness scoring + flaky/attempt recommender + flaky.go // flakiness scoring + flaky (bool) recommender emit/ buildozer.go // emit buildozer command script pr.go // group by owner, open PRs (later phase) @@ -317,7 +317,7 @@ type TargetStats struct { DurationP50 time.Duration DurationP95 time.Duration DurationMax time.Duration - DeclaredTimeout string // as observed in runs, if available + DeclaredTimeout string // bucket keyword (short|moderate|long|eternal) as observed, if available; may be empty (then derived from `size`, see §4.3) TimeoutFailures int // failures attributed to hitting the timeout // Per-attempt outcomes for flakiness math: Attempts int // total attempts observed @@ -335,11 +335,27 @@ while letting the query backend be swapped/tested with a fake. ### 4.3 Timeout recommender (`analyze/timeout.go`) -- Bazel buckets: `short≈60s`, `moderate≈300s`, `long≈900s`, `eternal≈3600s`. -- Recommend the **smallest bucket** where `DurationP95 * safetyFactor` fits (default - `safetyFactor = 1.5`, configurable). +**Bucket → seconds is a configured input, not a constant.** The `timeout` attribute is a +*bucket keyword* (`short`/`moderate`/`long`/`eternal`); the number of seconds each +bucket allows defaults to `60 / 300 / 900 / 3600` but a repo can override it with +`--test_timeout=short,moderate,long,eternal` (usually in `.bazelrc`). The recommender +cannot resolve a bucket to seconds — nor pick a bucket for an observed duration — +without the repo's actual mapping. So: + +- The tool takes a **`timeoutBuckets` config** (`map[string]int` keyword→seconds), + defaulting to Bazel's `60/300/900/3600`. Populate it either by parsing the repo's + `.bazelrc` for `--test_timeout`, or via an explicit flag/config value. Surface the + effective mapping in the report so recommendations are auditable. +- **Timeout may be implicit.** If a target sets no `timeout`, Bazel derives the bucket + from `size` (`small→short`, `medium→moderate`, `large→long`, `enormous→eternal`), then + resolves seconds via the same `timeoutBuckets` map. The recommender must apply this + fallback when `DeclaredTimeout` is empty, and decide whether to write a `timeout` attr + or adjust `size` (recommend setting `timeout` explicitly to avoid perturbing other + `size`-driven behavior like resource reservations). +- Recommend the **smallest bucket** whose configured seconds ≥ `DurationP95 * + safetyFactor` (default `safetyFactor = 1.5`, configurable). - **Bump up** if either: observed runtime is within `X%` (default 20%) of the current - limit, **or** `TimeoutFailures > 0`. + bucket's configured seconds, **or** `TimeoutFailures > 0`. - **Never recommend `eternal`** unless the target is on the eternal allow-list (shared with Workstream A's config so policy stays single-sourced). If data says a target needs > `long` and isn't allow-listed, emit a **report finding for a human**, not an @@ -519,10 +535,14 @@ Each task is independently ownable; dependencies noted. "AC" = acceptance criter can't be silenced by `disable=` locally, accepting a break to the "every warning is suppressible" contract? Or rely solely on the CI gate? (Leaning CI-gate-only for the first cut.) -3. Warehouse **schema/columns** available for `TargetStats` — especially whether +4. Warehouse **schema/columns** available for `TargetStats` — especially whether per-attempt outcomes (`PassByAttempt`) exist, or only aggregate pass/fail. This changes flakiness estimation fidelity. -4. Timeout **safety factor** and bucket cutoffs — defaults proposed; confirm with SRE/CI. -5. PR routing — CODEOWNERS-based? One PR per owner, or batched? -6. Do we want the shared eternal-allow-list loader as its own small package now, or +5. **`timeoutBuckets` sourcing (§4.3)** — parse `.bazelrc` for `--test_timeout`, or + require it as explicit tool config? Note `--test_timeout` can differ per bazelrc + `--config`/platform, so "the" mapping may be ambiguous; do we pin one config, or + analyze per-config? +6. Timeout **safety factor** and bucket cutoffs — defaults proposed; confirm with SRE/CI. +7. PR routing — CODEOWNERS-based? One PR per owner, or batched? +8. Do we want the shared eternal-allow-list loader as its own small package now, or duplicate-read for Phase 1 and refactor later? From 38629cbffd76f05750116d0bec4f63cec960c8ae Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 1 Jul 2026 10:08:52 -0700 Subject: [PATCH 05/16] Reference bazelbuild/bazel#30108 for per-target retry-count limit The flaky attribute and --flaky_test_attempts are disconnected, so a BUILD file can't express how flaky a target is. Cite the upstream issue in the flakiness reality-check as the reason the tool only recommends a boolean flaky and keeps N internal. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/attribute-policy-and-test-tuning-design.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index ff319cda9..7363cc1ad 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -385,7 +385,13 @@ Answers "does a single retry likely pass, or does it need multiple?". - **Reality check (must be in tool output & docs):** Bazel's `flaky` is a **boolean**; the tool only ever recommends `flaky = True` / `flaky = False`. `N` is an internal - signal for classification, not something written into the BUILD file. + signal for classification, not something written into the BUILD file. There is no way + today to express *how* flaky a target is (a per-target retry count) in a BUILD file: + the `flaky` attribute and the `--flaky_test_attempts` flag are disconnected, so + per-target attempt counts require unwieldy regex lists on the flag. This limitation is + tracked upstream in [bazelbuild/bazel#30108](https://github.com/bazelbuild/bazel/issues/30108); + if/when it lands, the classifier's `N` becomes directly expressible and this section + should be revisited. ### 4.5 Emit (`emit/`) From b22d812fabba100b08d2a59a32ff9d13e3148083 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 1 Jul 2026 10:09:50 -0700 Subject: [PATCH 06/16] Treat lowering timeouts as a normal recommendation Bazel's default size is medium => moderate (300s), so many fast tests sit at moderate unnecessarily and should drop to short. Remove the --allow-lowering gate; the safetyFactor headroom, bump-on-flake rules, and minimum-sample-size guard already make downgrades safe. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/attribute-policy-and-test-tuning-design.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 7363cc1ad..256c4f07f 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -408,8 +408,13 @@ Answers "does a single retry likely pass, or does it need multiple?". - Require a minimum `Runs` sample size before recommending (default e.g. 20); otherwise report "insufficient data". -- Only ever *raise* timeouts automatically; *lowering* a timeout is report-only (risk of - new flakes) unless `--allow-lowering`. +- **Lowering timeouts is a normal, first-class recommendation** — not gated behind a + flag. Bazel's default `size` is `medium` ⇒ `moderate` (300s), so many genuinely fast + tests sit at `moderate` unnecessarily; dropping them to `short` speeds failure + detection and scheduling. Safety comes from the recommender itself, not from + suppressing downgrades: the `safetyFactor` headroom and the bump-up-on-`TimeoutFailures` + / near-limit rules (§4.3) already keep a downgrade from cutting it too close, and the + minimum-sample-size guard avoids acting on noise. - Respect the eternal allow-list from `.buildifier.json` so the two systems agree. - Dry-run is the default mode. From a3bbea85759f8d9cc25398e4b72dccdc000ead57 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 1 Jul 2026 10:12:29 -0700 Subject: [PATCH 07/16] Scope testpolicy to emitting buildozer commands only The tool's job ends when the buildozer command script is produced. Remove apply mode, PR grouping/routing, and CODEOWNERS batching from the tool's responsibilities; executing edits and opening PRs are downstream concerns of the calling CI/automation. Drops the pr.go component, the B6 task, and the PR-routing open question. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...attribute-policy-and-test-tuning-design.md | 53 ++++++++++--------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 256c4f07f..13a2c00f3 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -17,19 +17,21 @@ We want two related capabilities for Bazel `BUILD` files: 2. **Empirical test-tuning** in a **new, separate tool** (`testpolicy`) — read historical test-execution stats from a metrics warehouse, compute recommended - `timeout` and `flaky` values (and a flakiness score), and apply them via - `buildozer` commands / PRs. `buildifier` never touches the warehouse. + `timeout` and `flaky` values (and a flakiness score), and **emit the `buildozer` + commands** that would repair the repo. `buildifier` never touches the warehouse. These are split deliberately: `buildifier` stays a fast, hermetic, offline static -linter; all data-dependent analysis lives in a tool that can query a warehouse and -open PRs. +linter; all data-dependent analysis lives in a tool that queries a warehouse and +produces buildozer commands. **The tool's job ends when those commands are produced** — +running them, opening PRs, and routing to reviewers are downstream concerns left to +whatever CI/automation invokes the tool. ### Decisions locked in (from design review) | Question | Decision | |---|---| | Source of empirical data | Metrics DB / warehouse (queried by the new tool) | -| How empirical recommendations are applied | `buildozer` commands + PRs; `buildifier` stays purely static | +| How empirical recommendations are applied | Tool emits `buildozer` commands (its terminal deliverable); applying/PRs are downstream. `buildifier` stays purely static | | Where policy config lives | Extend the existing `.buildifier.json` config | --- @@ -294,7 +296,7 @@ never blocks pre-commit. ``` testpolicy/ - main.go // CLI: window, filters, dry-run/report/apply modes + main.go // CLI: window, filters, report output source/ // warehouse adapter source.go // interface + data model bigquery.go // concrete impl (behind a build tag / flag) @@ -302,8 +304,7 @@ testpolicy/ timeout.go // timeout recommender flaky.go // flakiness scoring + flaky (bool) recommender emit/ - buildozer.go // emit buildozer command script - pr.go // group by owner, open PRs (later phase) + buildozer.go // emit buildozer command script (terminal deliverable) report/ // human-readable + machine (JSON) report ``` @@ -395,14 +396,18 @@ Answers "does a single retry likely pass, or does it need multiple?". ### 4.5 Emit (`emit/`) -- **Phase 2 (first):** `report`/`dry-run` only — print recommendations + reasons; emit a - `buildozer` command script to stdout/file, e.g.: - ``` - buildozer 'set timeout "long"' //pkg:target - buildozer 'set flaky True' //pkg:other - ``` -- **Phase 3:** `apply` mode runs buildozer and/or opens PRs grouped by CODEOWNERS. -- Always emit a machine-readable JSON report for auditability. +The tool's **terminal output** is a `buildozer` command script that would repair the +repo, printed to stdout/file, e.g.: +``` +buildozer 'set timeout "short"' //pkg:target +buildozer 'set flaky True' //pkg:other +``` +- Also emit a machine-readable JSON report (recommendations + reasons + effective + `timeoutBuckets`) for auditability. +- The tool **does not run buildozer, commit, or open PRs.** Executing the script, + batching edits, opening PRs, and routing to reviewers are downstream responsibilities + of whatever CI/automation calls `testpolicy`. Keeping the boundary here makes the tool + trivially testable (assert on emitted commands) and reusable by any apply/review flow. ### 4.6 Safety / guardrails @@ -420,10 +425,10 @@ Answers "does a single retry likely pass, or does it need multiple?". ### 4.7 Acceptance criteria (Workstream B) -- With a fake `Source`, `testpolicy report` produces correct timeout & flaky - recommendations and a valid buildozer script for a fixture dataset. +- With a fake `Source`, `testpolicy` produces correct timeout & flaky recommendations + and a valid buildozer command script for a fixture dataset. - No warehouse credentials required for tests (fake source). -- `apply` mode is gated behind an explicit flag and off by default. +- Output is deterministic (stable ordering) so the emitted script can be asserted on. --- @@ -433,8 +438,8 @@ Answers "does a single retry likely pass, or does it need multiple?". Both `buildifier` (enforce) and `testpolicy` (never recommend eternal off-list) read it. Consider a tiny shared loader package so the schema isn't duplicated. - `buildifier` enforces the *invariants*; `testpolicy` proposes the *values*. A - `testpolicy`-generated PR must itself pass `attr-policy` lint — i.e. the tool won't - propose an edit that buildifier would reject. + `testpolicy`-emitted edit must itself satisfy `attr-policy` — i.e. the tool won't emit + a buildozer command that buildifier would then reject. --- @@ -523,10 +528,9 @@ Each task is independently ownable; dependencies noted. "AC" = acceptance criter - **B2. Timeout recommender** — §4.3 + tests on fixtures. *(dep: B1)* - **B3. buildozer emitter + report** — §4.5 dry-run path. *(dep: B1)* -### Phase 3 — flakiness + application +### Phase 3 — flakiness + real data source - **B4. Flakiness scoring** — §4.4 + tests. *(dep: B1)* - **B5. Warehouse (BigQuery/DB) source impl** — behind flag. *(dep: B1)* -- **B6. `apply` mode + PR grouping (CODEOWNERS)** — §4.5. *(dep: B2/B3/B4)* ### Phase 1b — enforcement (Workstream A, in parallel with Phase 2) - **A7. Authoritative CI gate** — evaluate policy ignoring `disable=` for @@ -554,6 +558,5 @@ Each task is independently ownable; dependencies noted. "AC" = acceptance criter `--config`/platform, so "the" mapping may be ambiguous; do we pin one config, or analyze per-config? 6. Timeout **safety factor** and bucket cutoffs — defaults proposed; confirm with SRE/CI. -7. PR routing — CODEOWNERS-based? One PR per owner, or batched? -8. Do we want the shared eternal-allow-list loader as its own small package now, or +7. Do we want the shared eternal-allow-list loader as its own small package now, or duplicate-read for Phase 1 and refactor later? From 7f978ffdbe178caedbef10d5ef8b0d250e9570bb Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 1 Jul 2026 11:09:51 -0700 Subject: [PATCH 08/16] Document boolean and dict attribute constraints in attr-policy schema. Show how policies like local = True and execution_requirements entries are encoded via forbidValues and forbidDictEntries. --- ...attribute-policy-and-test-tuning-design.md | 90 +++++++++++++++---- 1 file changed, 71 insertions(+), 19 deletions(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 13a2c00f3..14670c813 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -77,7 +77,9 @@ whatever CI/automation invokes the tool. A **single generalized** warning, `attr-policy`, driven entirely by config. It covers the initial asks (eternal-timeout allow-list, no `exclusive` on tests) and any future -"attribute constrained on rule-kind unless allow-listed" rule **without new Go code**. +"attribute constrained on rule-kind unless allow-listed" rule **without new Go code** — +including boolean literals (`local = True`) and dict attributes +(`execution_requirements = {"no-cache": "1"}`). Rationale for one generalized warning vs. many hardcoded ones: buildifier warnings are individually toggleable by name, but the *policy content* here is site-specific and @@ -104,6 +106,21 @@ generic and lets each repo express its own policy. "attr": "tags", "forbidListItems": ["exclusive"], // list-membership constraint "allowlist": [] + }, + { + "name": "no-local-tests", + "ruleKinds": ["*_test"], + "attr": "local", + "forbidValues": ["True"], // boolean literal constraint (see below) + "message": "Tests must not set local = True; use a hermetic test instead." + }, + { + "name": "no-no-cache", + "attr": "execution_requirements", + "forbidDictEntries": { // dict key→value constraint (see below) + "no-cache": "1" + }, + "message": "Do not set execution_requirements['no-cache'] = '1'." } ] } @@ -117,18 +134,40 @@ generic and lets each repo express its own policy. | `name` | string (required) | Stable identifier, included in the finding message. Must be unique. | | `ruleKinds` | []string | Globs matched against `rule.Kind()`. Empty/absent ⇒ matches all kinds. | | `attr` | string (required) | Attribute to inspect. | -| `forbidValues` | []string | Scalar attr must not equal any of these. | +| `forbidValues` | []string | Scalar attr must not equal any of these (see scalar matching below). | | `requireValues` | []string | If attr present, must equal one of these. (If also want "must be present", see `required`.) | | `forbidListItems` | []string | List attr must not contain any of these items. | | `requireListItems` | []string | List attr must contain all of these items. | +| `forbidDictEntries` | object (string→string) | Dict attr must not contain any of these key→value pairs. | +| `requireDictEntries` | object (string→string) | Dict attr must contain all of these key→value pairs. | +| `forbidDictKeys` | []string | Dict attr must not contain any of these keys (value ignored). | | `required` | bool | Attr must be present at all. | | `allowlist` | []string | Target patterns exempt from this rule (see grammar below). | | `suppressible` | bool (default `true`) | Whether `# buildifier: disable=attr-policy` silences this rule. `false` = a "hard" rule enforced authoritatively by CI regardless of in-file comments (see §6). | | `message` | string | Custom message. If absent, a default is synthesized from the constraint. | +**Scalar matching (`forbidValues` / `requireValues`).** Covers string attributes +(`timeout = "eternal"`) and boolean/identifier literals (`local = True`, +`flaky = False`). At check time, read the attribute with `rule.AttrString` first, +then fall back to `rule.AttrLiteral` (which returns `True`/`False` for boolean +literals and identifier names for unquoted tokens). Compare the normalized string +form. An absent attribute does not match `forbidValues`; it only fails +`requireValues` when the attribute is present but wrong. + +**Dict matching (`forbidDictEntries` / `requireDictEntries` / `forbidDictKeys`).** +Covers `string_dict` / `label_dict` attributes such as `execution_requirements`. +The attribute must be a `*build.DictExpr`; look up keys with `edit.DictionaryGet`. +Dict values are normalized the same way as scalars (string literal or +`AttrLiteral` on the value node). Example: `execution_requirements = {"no-cache": +"1"}` is flagged by the `no-no-cache` rule above because the dict contains key +`no-cache` with value `"1"`. `forbidDictKeys` is a weaker variant that flags the +presence of a key regardless of its value (useful when any non-default value is +undesirable but the exact value varies). + Exactly one *constraint family* (`forbidValues`/`requireValues` **or** -`forbidListItems`/`requireListItems`) should be set per rule; `required` is -orthogonal. Validation enforces this (see 3.5). +`forbidListItems`/`requireListItems` **or** +`forbidDictEntries`/`requireDictEntries`/`forbidDictKeys`) should be set per +rule; `required` is orthogonal. Validation enforces this (see 3.5). **Allow-list pattern grammar.** Entries are Bazel-style target patterns (a subset of what users already know from `bazel build`): @@ -167,17 +206,20 @@ type AttrPolicy struct { } type AttrPolicyRule struct { - Name string `json:"name"` - RuleKinds []string `json:"ruleKinds,omitempty"` - Attr string `json:"attr"` - ForbidValues []string `json:"forbidValues,omitempty"` - RequireValues []string `json:"requireValues,omitempty"` - ForbidListItems []string `json:"forbidListItems,omitempty"` - RequireListItems []string `json:"requireListItems,omitempty"` - Required bool `json:"required,omitempty"` - Allowlist []string `json:"allowlist,omitempty"` - Suppressible *bool `json:"suppressible,omitempty"` // pointer so absent ⇒ default true - Message string `json:"message,omitempty"` + Name string `json:"name"` + RuleKinds []string `json:"ruleKinds,omitempty"` + Attr string `json:"attr"` + ForbidValues []string `json:"forbidValues,omitempty"` + RequireValues []string `json:"requireValues,omitempty"` + ForbidListItems []string `json:"forbidListItems,omitempty"` + RequireListItems []string `json:"requireListItems,omitempty"` + ForbidDictEntries map[string]string `json:"forbidDictEntries,omitempty"` + RequireDictEntries map[string]string `json:"requireDictEntries,omitempty"` + ForbidDictKeys []string `json:"forbidDictKeys,omitempty"` + Required bool `json:"required,omitempty"` + Allowlist []string `json:"allowlist,omitempty"` + Suppressible *bool `json:"suppressible,omitempty"` // pointer so absent ⇒ default true + Message string `json:"message,omitempty"` } ``` @@ -218,7 +260,7 @@ On load, reject: - duplicate `name`s, - empty `name` or empty `attr`, - a rule with no constraint set, -- a rule mixing scalar and list constraint families, +- a rule mixing scalar, list, and dict constraint families, - malformed globs in `ruleKinds` / malformed target patterns in `allowlist` (must match the §3.2 grammar; reject a bare `...`, repository-qualified entries, etc.). @@ -250,8 +292,15 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { - `p.allowed` runs the precompiled `allowlistMatch` (§3.2 grammar) over the target label — exact / `:all` / `/...` — not a bare `labels.Equal`. -- `p.check` handles the constraint families using `rule.AttrString` / `rule.AttrStrings` - and `rule.Attr(attr)` for the anchor node; returns `makeLinterFinding(node, msg)`. +- `p.check` handles the constraint families: + - **Scalars:** `attrScalarString(rule, attr)` → `rule.AttrString`, else + `rule.AttrLiteral`; compare against `forbidValues` / `requireValues`. + - **Lists:** `rule.AttrStrings`. + - **Dicts:** `rule.Attr(attr)` as `*build.DictExpr`; `edit.DictionaryGet` per + key; normalize values like scalars. `forbidDictKeys` flags any listed key + that is present. + Returns `makeLinterFinding(node, msg)` anchored on the offending attr node (or a + specific dict entry's value node when possible). - Scope: BUILD files only (targets live there). Return `nil` for other file types. - **Autofix: none initially.** We cannot infer a correct scalar value. The one mechanically-safe fix (remove a forbidden list item / forbidden tag) is a good @@ -264,7 +313,8 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { (recommended, since it no-ops without config anyway — but being config-gated it's harmless in the default set too; pick opt-in to be conservative). - Docs: add an entry to `WARNINGS.md` and `warn/docs/warnings.textproto` describing the - warning **and** the `attrPolicy` config block (with the two example rules). + warning **and** the `attrPolicy` config block (with the example rules, including + boolean and dict constraints). - Add a `-config=example` sample entry so `buildifier -config=example` prints an `attrPolicy` stub (see `config.Example()`). - Tests `warn/warn_attr_policy_test.go`: @@ -273,6 +323,8 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { - Cases: forbidden scalar value flagged; allow-listed target exempt; recursive `//pkg/...` allow-list; forbidden list item in `tags`; `ruleKinds` glob match & non-match; missing-when-`required`; `# buildifier: disable=attr-policy` suppression; + forbidden boolean literal (`local = True`); forbidden dict entry + (`execution_requirements = {"no-cache": "1"}`); `forbidDictKeys` on a dict key; empty config = no findings. - Config tests in `buildifier/config/config_test.go`: parse the sample JSON; validation rejects malformed rules. From 1cdb5f6a593ee94a4e777fed21e5e6ff74c6c71a Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Thu, 2 Jul 2026 07:56:59 -0700 Subject: [PATCH 09/16] Expand testpolicy design for shard_count and integer flaky. Document timeout, flaky, and shard_count as execution-reflecting attrs; add shard_count recommender (future), integer flaky semantics from figma/bazel#13, and attr-policy maxValue for shard_count after figma/bazel#12. --- ...attribute-policy-and-test-tuning-design.md | 132 ++++++++++++++---- 1 file changed, 101 insertions(+), 31 deletions(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 14670c813..7cf81200d 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -12,14 +12,19 @@ We want two related capabilities for Bazel `BUILD` files: 1. **Static attribute-policy linting** in `buildifier` — enforce declarative rules about attribute values, e.g. forbid `timeout = "eternal"` unless the target is on an - approved allow-list, forbid `exclusive` in a test's `tags`, and similar - attribute/rule-kind constraints. Purely static, config-driven, no runtime data. + approved allow-list, forbid `exclusive` in a test's `tags`, cap `shard_count` at 50, + and similar attribute/rule-kind constraints. Purely static, config-driven, no runtime data. 2. **Empirical test-tuning** in a **new, separate tool** (`testpolicy`) — read historical test-execution stats from a metrics warehouse, compute recommended - `timeout` and `flaky` values (and a flakiness score), and **emit the `buildozer` + `timeout`, `flaky`, and (in theory) `shard_count` values, and **emit the `buildozer` commands** that would repair the repo. `buildifier` never touches the warehouse. + `timeout`, `flaky`, and `shard_count` are the three test attributes meant to reflect + **how the test currently executes**, not how it was designed. `shard_count` tuning is + feasible only if telemetry can separate fixture/SUT setup time from assertion time; + otherwise the tool cannot tell whether more shards would shorten the critical path. + These are split deliberately: `buildifier` stays a fast, hermetic, offline static linter; all data-dependent analysis lives in a tool that queries a warehouse and produces buildozer commands. **The tool's job ends when those commands are produced** — @@ -34,6 +39,16 @@ whatever CI/automation invokes the tool. | How empirical recommendations are applied | Tool emits `buildozer` commands (its terminal deliverable); applying/PRs are downstream. `buildifier` stays purely static | | Where policy config lives | Extend the existing `.buildifier.json` config | +### Motivating use cases + +| Policy | Encoding | Context | +|---|---|---| +| No `timeout = "eternal"` without approval | `forbidValues` + `allowlist` | Initial ask | +| No `exclusive` in test `tags` | `forbidListItems` | Initial ask | +| No `local = True` on tests | `forbidValues: ["True"]` | Hermetic-test policy | +| No `execution_requirements["no-cache"] = "1"` | `forbidDictEntries` | Cache-bypass policy | +| `shard_count` ≤ 50 on tests | `maxValue: 50` | [figma/bazel#12](https://github.com/figma/bazel/pull/12) removed Bazel's hardcoded 50-shard cap; repos that want to keep that limit (or any other bound) can enforce it in buildifier instead of forking Bazel | + --- ## 2. Background: how `buildifier` warnings work today @@ -78,8 +93,8 @@ whatever CI/automation invokes the tool. A **single generalized** warning, `attr-policy`, driven entirely by config. It covers the initial asks (eternal-timeout allow-list, no `exclusive` on tests) and any future "attribute constrained on rule-kind unless allow-listed" rule **without new Go code** — -including boolean literals (`local = True`) and dict attributes -(`execution_requirements = {"no-cache": "1"}`). +including boolean literals (`local = True`), dict attributes +(`execution_requirements = {"no-cache": "1"}`), and numeric bounds (`shard_count` ≤ 50). Rationale for one generalized warning vs. many hardcoded ones: buildifier warnings are individually toggleable by name, but the *policy content* here is site-specific and @@ -121,6 +136,14 @@ generic and lets each repo express its own policy. "no-cache": "1" }, "message": "Do not set execution_requirements['no-cache'] = '1'." + }, + { + "name": "max-shard-count", + "ruleKinds": ["*_test"], + "attr": "shard_count", + "maxValue": 50, // numeric range constraint (see below) + "allowlist": ["//huge_suite:..."], + "message": "shard_count must not exceed 50; add the target to the allowlist for larger suites." } ] } @@ -141,6 +164,8 @@ generic and lets each repo express its own policy. | `forbidDictEntries` | object (string→string) | Dict attr must not contain any of these key→value pairs. | | `requireDictEntries` | object (string→string) | Dict attr must contain all of these key→value pairs. | | `forbidDictKeys` | []string | Dict attr must not contain any of these keys (value ignored). | +| `minValue` | int | Numeric attr must be ≥ this (inclusive). At least one of `minValue` / `maxValue` required for the numeric family. | +| `maxValue` | int | Numeric attr must be ≤ this (inclusive). | | `required` | bool | Attr must be present at all. | | `allowlist` | []string | Target patterns exempt from this rule (see grammar below). | | `suppressible` | bool (default `true`) | Whether `# buildifier: disable=attr-policy` silences this rule. `false` = a "hard" rule enforced authoritatively by CI regardless of in-file comments (see §6). | @@ -164,10 +189,19 @@ Dict values are normalized the same way as scalars (string literal or presence of a key regardless of its value (useful when any non-default value is undesirable but the exact value varies). +**Numeric matching (`minValue` / `maxValue`).** Covers integer attributes such as +`shard_count`. Read the attribute with `rule.AttrLiteral` (a `*build.LiteralExpr` +whose `Token` is the decimal representation) and parse with `strconv.Atoi`. An absent +attribute does not violate `maxValue` / `minValue` — only an explicitly set value +outside the range is flagged. This is the mechanism repos use to re-impose limits +Bazel no longer enforces globally (e.g. the former 50-shard cap removed in +[figma/bazel#12](https://github.com/figma/bazel/pull/12)). + Exactly one *constraint family* (`forbidValues`/`requireValues` **or** `forbidListItems`/`requireListItems` **or** -`forbidDictEntries`/`requireDictEntries`/`forbidDictKeys`) should be set per -rule; `required` is orthogonal. Validation enforces this (see 3.5). +`forbidDictEntries`/`requireDictEntries`/`forbidDictKeys` **or** +`minValue`/`maxValue`) should be set per rule; `required` is orthogonal. Validation +enforces this (see 3.5). **Allow-list pattern grammar.** Entries are Bazel-style target patterns (a subset of what users already know from `bazel build`): @@ -216,6 +250,8 @@ type AttrPolicyRule struct { ForbidDictEntries map[string]string `json:"forbidDictEntries,omitempty"` RequireDictEntries map[string]string `json:"requireDictEntries,omitempty"` ForbidDictKeys []string `json:"forbidDictKeys,omitempty"` + MinValue *int `json:"minValue,omitempty"` // pointer: absent vs set-to-zero + MaxValue *int `json:"maxValue,omitempty"` Required bool `json:"required,omitempty"` Allowlist []string `json:"allowlist,omitempty"` Suppressible *bool `json:"suppressible,omitempty"` // pointer so absent ⇒ default true @@ -260,7 +296,9 @@ On load, reject: - duplicate `name`s, - empty `name` or empty `attr`, - a rule with no constraint set, -- a rule mixing scalar, list, and dict constraint families, +- a rule mixing scalar, list, dict, and numeric constraint families, +- a numeric rule with neither `minValue` nor `maxValue` set, or with `minValue` > + `maxValue` when both are set, - malformed globs in `ruleKinds` / malformed target patterns in `allowlist` (must match the §3.2 grammar; reject a bare `...`, repository-qualified entries, etc.). @@ -299,6 +337,8 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { - **Dicts:** `rule.Attr(attr)` as `*build.DictExpr`; `edit.DictionaryGet` per key; normalize values like scalars. `forbidDictKeys` flags any listed key that is present. + - **Numerics:** `strconv.Atoi(rule.AttrLiteral(attr))`; flag if value `< minValue` + or `> maxValue` (bounds inclusive; absent bound ignored). Returns `makeLinterFinding(node, msg)` anchored on the offending attr node (or a specific dict entry's value node when possible). - Scope: BUILD files only (targets live there). Return `nil` for other file types. @@ -314,7 +354,7 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { harmless in the default set too; pick opt-in to be conservative). - Docs: add an entry to `WARNINGS.md` and `warn/docs/warnings.textproto` describing the warning **and** the `attrPolicy` config block (with the example rules, including - boolean and dict constraints). + boolean, dict, and numeric constraints). - Add a `-config=example` sample entry so `buildifier -config=example` prints an `attrPolicy` stub (see `config.Example()`). - Tests `warn/warn_attr_policy_test.go`: @@ -325,14 +365,16 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { non-match; missing-when-`required`; `# buildifier: disable=attr-policy` suppression; forbidden boolean literal (`local = True`); forbidden dict entry (`execution_requirements = {"no-cache": "1"}`); `forbidDictKeys` on a dict key; - empty config = no findings. + `shard_count` above `maxValue` flagged, within range OK, absent OK; allow-listed + high-shard target exempt; empty config = no findings. - Config tests in `buildifier/config/config_test.go`: parse the sample JSON; validation rejects malformed rules. ### 3.8 Acceptance criteria (Workstream A) - `buildifier --lint=warn --warnings=attr-policy BUILD` flags eternal timeout on - non-allow-listed test targets and `exclusive` tags on tests, given the sample config. + non-allow-listed test targets, `exclusive` tags on tests, and `shard_count > 50`, + given the sample config. - Zero findings when `attrPolicy` is absent. - All new + existing `warn` and `config` tests pass; `WARNINGS.md` regeneration (if applicable) is consistent. @@ -354,7 +396,8 @@ testpolicy/ bigquery.go // concrete impl (behind a build tag / flag) analyze/ timeout.go // timeout recommender - flaky.go // flakiness scoring + flaky (bool) recommender + flaky.go // flakiness scoring + flaky recommender + shard.go // (future) shard_count recommender — needs setup vs assertion timing emit/ buildozer.go // emit buildozer command script (terminal deliverable) report/ // human-readable + machine (JSON) report @@ -375,6 +418,10 @@ type TargetStats struct { // Per-attempt outcomes for flakiness math: Attempts int // total attempts observed PassByAttempt []float64 // PassByAttempt[k] = P(pass by attempt k+1), empirical + // Per-shard timing (optional; needed for shard_count recommender): + ShardCount int // declared shard_count, if any + ShardDurationP95 []time.Duration // per-shard P95, when available + SetupDurationP95 time.Duration // fixture/SUT setup time P95, when separable from assertions } type Source interface { @@ -431,28 +478,46 @@ Answers "does a single retry likely pass, or does it need multiple?". from "retries rarely help", which drives the `flaky` recommendation: | `N` | Meaning | Recommendation | |---|---|---| - | ≤ 1 | not flaky | `flaky = False` (or leave unset) | - | 2–3 | retries reliably recover it | `flaky = True` | - | > 3 | retries rarely recover it | report for owner; `flaky` won't reliably help | + | ≤ 1 | not flaky | `flaky = 0` (or leave unset) | + | 2–3 | retries reliably recover it | `flaky = N - 1` (extra retries beyond the first attempt) | + | > 3 | retries rarely recover it | report for owner; more retries won't reliably help | | very low `a` | chronically broken | **do not** mask with retries; report for owner | -- **Reality check (must be in tool output & docs):** Bazel's `flaky` is a **boolean**; - the tool only ever recommends `flaky = True` / `flaky = False`. `N` is an internal - signal for classification, not something written into the BUILD file. There is no way - today to express *how* flaky a target is (a per-target retry count) in a BUILD file: - the `flaky` attribute and the `--flaky_test_attempts` flag are disconnected, so - per-target attempt counts require unwieldy regex lists on the flag. This limitation is - tracked upstream in [bazelbuild/bazel#30108](https://github.com/bazelbuild/bazel/issues/30108); - if/when it lands, the classifier's `N` becomes directly expressible and this section - should be revisited. +- **Semantics (with integer `flaky`):** When `--flaky_test_attempts=default`, Bazel runs + `1 + flaky` total attempts for flaky targets (`flaky = 0` ⇒ one attempt). The tool + writes the integer directly: `buildozer 'set flaky 2' //pkg:target` for three total + attempts. This is implemented in [figma/bazel#13](https://github.com/figma/bazel/pull/13); + upstream tracking in [bazelbuild/bazel#30108](https://github.com/bazelbuild/bazel/issues/30108). + When CI sets `--flaky_test_attempts=N` (bare integer), that flag overrides the attribute + for all targets — the tool should surface that in its report so owners know BUILD-file + edits may have no effect. -### 4.5 Emit (`emit/`) +### 4.5 Shard-count recommender (`analyze/shard.go`) — future + +`shard_count` is the third execution-reflecting test attribute alongside `timeout` and +`flaky`: all three describe **how the test currently executes**, not how it was designed. + +Tuning `shard_count` is feasible **only if** telemetry can separate fixture/SUT setup time +from assertion (test-case) time. Without that split, the tool cannot tell whether more +shards would shorten the critical path or just duplicate expensive setup across shards. + +When per-shard data is available: + +- Recommend raising `shard_count` when per-shard assertion time is high and setup is + amortizable (e.g. many cases per shard, setup ≪ case time). +- Recommend lowering `shard_count` when shards are mostly idle or setup dominates. +- Respect `attr-policy` caps (e.g. `shard_count` ≤ 50) from Workstream A. + +Output per target: `{current, recommended, reason}`; emit `buildozer 'set shard_count N'`. + +### 4.6 Emit (`emit/`) The tool's **terminal output** is a `buildozer` command script that would repair the repo, printed to stdout/file, e.g.: ``` buildozer 'set timeout "short"' //pkg:target -buildozer 'set flaky True' //pkg:other +buildozer 'set flaky 2' //pkg:other +buildozer 'set shard_count 4' //pkg:sharded ``` - Also emit a machine-readable JSON report (recommendations + reasons + effective `timeoutBuckets`) for auditability. @@ -461,7 +526,7 @@ buildozer 'set flaky True' //pkg:other of whatever CI/automation calls `testpolicy`. Keeping the boundary here makes the tool trivially testable (assert on emitted commands) and reusable by any apply/review flow. -### 4.6 Safety / guardrails +### 4.7 Safety / guardrails - Require a minimum `Runs` sample size before recommending (default e.g. 20); otherwise report "insufficient data". @@ -475,10 +540,11 @@ buildozer 'set flaky True' //pkg:other - Respect the eternal allow-list from `.buildifier.json` so the two systems agree. - Dry-run is the default mode. -### 4.7 Acceptance criteria (Workstream B) +### 4.8 Acceptance criteria (Workstream B) -- With a fake `Source`, `testpolicy` produces correct timeout & flaky recommendations - and a valid buildozer command script for a fixture dataset. +- With a fake `Source`, `testpolicy` produces correct `timeout`, `flaky`, and (when + fixture data includes per-shard timing) `shard_count` recommendations and a valid + buildozer command script for a fixture dataset. - No warehouse credentials required for tests (fake source). - Output is deterministic (stable ordering) so the emitted script can be asserted on. @@ -578,12 +644,16 @@ Each task is independently ownable; dependencies noted. "AC" = acceptance criter - **B1. Tool scaffold + CLI + fake source** — `testpolicy/main.go`, `source` interface, in-memory fake. *(no deps)* - **B2. Timeout recommender** — §4.3 + tests on fixtures. *(dep: B1)* -- **B3. buildozer emitter + report** — §4.5 dry-run path. *(dep: B1)* +- **B3. buildozer emitter + report** — §4.6 dry-run path. *(dep: B1)* ### Phase 3 — flakiness + real data source - **B4. Flakiness scoring** — §4.4 + tests. *(dep: B1)* - **B5. Warehouse (BigQuery/DB) source impl** — behind flag. *(dep: B1)* +### Phase 4 — shard_count tuning (future) +- **B6. Shard-count recommender** — §4.5; requires per-shard telemetry with setup vs + assertion split. *(dep: B1, B5)* + ### Phase 1b — enforcement (Workstream A, in parallel with Phase 2) - **A7. Authoritative CI gate** — evaluate policy ignoring `disable=` for `suppressible: false` rules (§6.1/§6.2); non-zero exit on violation. *(dep: A1–A4)* From b54ae9d2cd01aa2264942139de9eec7bbee84606 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Thu, 2 Jul 2026 08:21:36 -0700 Subject: [PATCH 10/16] Add config-driven attr-policy linting and buildifier JSON schema. Implement Workstream A from the design doc: a single attr-policy warning driven by attrPolicy rules in .buildifier.json, plus a JSON Schema for safer editor and agent edits to buildifier config. --- WARNINGS.md | 42 ++++ buildifier/README.md | 16 ++ buildifier/config/BUILD.bazel | 6 +- buildifier/config/attrpolicy.go | 218 ++++++++++++++++++ buildifier/config/attrpolicy_test.go | 170 ++++++++++++++ buildifier/config/buildifier.schema.json | 271 +++++++++++++++++++++++ buildifier/config/config.go | 20 ++ buildifier/config/config_test.go | 22 +- warn/BUILD.bazel | 3 + warn/attr_policy.go | 104 +++++++++ warn/docs/warnings.textproto | 35 +++ warn/warn.go | 2 + warn/warn_attr_policy.go | 164 ++++++++++++++ warn/warn_attr_policy_test.go | 189 ++++++++++++++++ 14 files changed, 1260 insertions(+), 2 deletions(-) create mode 100644 buildifier/config/attrpolicy.go create mode 100644 buildifier/config/attrpolicy_test.go create mode 100644 buildifier/config/buildifier.schema.json create mode 100644 warn/attr_policy.go create mode 100644 warn/warn_attr_policy.go create mode 100644 warn/warn_attr_policy_test.go diff --git a/WARNINGS.md b/WARNINGS.md index 2514b8813..c063b37c0 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -10,6 +10,7 @@ Warning categories supported by buildifier's linter: * [`attr-non-empty`](#attr-non-empty) * [`attr-output-default`](#attr-output-default) * [`attr-package-metadata`](#attr-package-metadata) + * [`attr-policy`](#attr-policy) * [`attr-single-file`](#attr-single-file) * [`build-args-kwargs`](#build-args-kwargs) * [`bzl-visibility`](#bzl-visibility) @@ -231,6 +232,47 @@ Using `package_metadata` as an attribute name may cause unexpected behavior. Its -------------------------------------------------------------------------------- +## Attribute value violates a configured policy rule + + * Category name: `attr-policy` + * Automatic fix: no + * [Disabled by default](buildifier/README.md#linter) + * [Suppress the warning](#suppress): `# buildifier: disable=attr-policy` + +Enforces declarative attribute constraints configured in `.buildifier.json` under +`attrPolicy`. Each rule names an attribute and a constraint family (scalar, +list, dict, or numeric bounds). Targets matching an `allowlist` pattern are +exempt. + +Example: + +```json +{ + "attrPolicy": { + "rules": [ + { + "name": "no-eternal-timeout", + "ruleKinds": ["*_test"], + "attr": "timeout", + "forbidValues": ["eternal"], + "allowlist": ["//slow/..."] + }, + { + "name": "max-shard-count", + "ruleKinds": ["*_test"], + "attr": "shard_count", + "maxValue": 50 + } + ] + } +} +``` + +A JSON Schema for `.buildifier.json` (including `attrPolicy`) lives at +`buildifier/config/buildifier.schema.json`. + +-------------------------------------------------------------------------------- + ## `single_file` is deprecated * Category name: `attr-single-file` diff --git a/buildifier/README.md b/buildifier/README.md index f183c334b..a5aa14193 100644 --- a/buildifier/README.md +++ b/buildifier/README.md @@ -93,6 +93,22 @@ type). See also the [full list](../WARNINGS.md) or the supported warnings. +## Configuration (`.buildifier.json`) + +Buildifier reads optional settings from `.buildifier.json` in the workspace (or +from the path in `BUILDIFIER_CONFIG` / `--config`). The file supports lint +options, custom tables, and config-driven policy rules under `attrPolicy` (see +the [`attr-policy`](../WARNINGS.md#attr-policy) warning). + +A [JSON Schema](config/buildifier.schema.json) is available for editor +validation and agent tooling. Reference it from your config file: + +```json +{ + "$schema": "https://raw.githubusercontent.com/bazelbuild/buildtools/main/buildifier/config/buildifier.schema.json" +} +``` + ## Setup and usage via Bazel You can also invoke buildifier via the Bazel rule. diff --git a/buildifier/config/BUILD.bazel b/buildifier/config/BUILD.bazel index 070d75605..7c6362c41 100644 --- a/buildifier/config/BUILD.bazel +++ b/buildifier/config/BUILD.bazel @@ -1,14 +1,18 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +exports_files(["buildifier.schema.json"]) + go_library( name = "config", srcs = [ + "attrpolicy.go", "config.go", "validation.go", ], importpath = "github.com/bazelbuild/buildtools/buildifier/config", visibility = ["//buildifier:__pkg__"], deps = [ + "//labels", "//tables", "//warn", "//wspace", @@ -17,7 +21,7 @@ go_library( go_test( name = "config_test", - srcs = ["config_test.go"], + srcs = ["config_test.go", "attrpolicy_test.go"], embed = [":config"], ) diff --git a/buildifier/config/attrpolicy.go b/buildifier/config/attrpolicy.go new file mode 100644 index 000000000..aa9c99463 --- /dev/null +++ b/buildifier/config/attrpolicy.go @@ -0,0 +1,218 @@ +package config + +import ( + "fmt" + "path" + "strings" + + "github.com/bazelbuild/buildtools/labels" + "github.com/bazelbuild/buildtools/warn" +) + +// AttrPolicy is the attrPolicy block in .buildifier.json. +type AttrPolicy struct { + Rules []AttrPolicyRule `json:"rules,omitempty"` +} + +// AttrPolicyRule is a single attribute policy rule. +type AttrPolicyRule struct { + Name string `json:"name"` + RuleKinds []string `json:"ruleKinds,omitempty"` + Attr string `json:"attr"` + ForbidValues []string `json:"forbidValues,omitempty"` + RequireValues []string `json:"requireValues,omitempty"` + ForbidListItems []string `json:"forbidListItems,omitempty"` + RequireListItems []string `json:"requireListItems,omitempty"` + ForbidDictEntries map[string]string `json:"forbidDictEntries,omitempty"` + RequireDictEntries map[string]string `json:"requireDictEntries,omitempty"` + ForbidDictKeys []string `json:"forbidDictKeys,omitempty"` + MinValue *int `json:"minValue,omitempty"` + MaxValue *int `json:"maxValue,omitempty"` + Required bool `json:"required,omitempty"` + Allowlist []string `json:"allowlist,omitempty"` + Suppressible *bool `json:"suppressible,omitempty"` + Message string `json:"message,omitempty"` +} + +func compileAttrPolicy(policy *AttrPolicy) ([]warn.AttrPolicyRuleCompiled, error) { + if policy == nil { + return nil, nil + } + seen := make(map[string]bool) + var compiled []warn.AttrPolicyRuleCompiled + for i := range policy.Rules { + rule := &policy.Rules[i] + c, err := compileAttrPolicyRule(rule, seen) + if err != nil { + return nil, fmt.Errorf("attrPolicy.rules[%d]: %w", i, err) + } + compiled = append(compiled, c) + } + return compiled, nil +} + +func compileAttrPolicyRule(rule *AttrPolicyRule, seen map[string]bool) (warn.AttrPolicyRuleCompiled, error) { + name := strings.TrimSpace(rule.Name) + if name == "" { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: name is required", rule.Name) + } + if seen[name] { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: duplicate name", name) + } + seen[name] = true + + attr := strings.TrimSpace(rule.Attr) + if attr == "" { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: attr is required", name) + } + + family, families, err := attrPolicyConstraintFamily(rule) + if err != nil { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: %w", name, err) + } + if families == 0 && !rule.Required { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: at least one constraint or required=true is required", name) + } + + for _, kindGlob := range rule.RuleKinds { + if _, err := path.Match(kindGlob, "x"); err != nil { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: malformed ruleKinds glob %q: %w", name, kindGlob, err) + } + } + + allowlist, err := compileAllowlistPatterns(name, rule.Allowlist) + if err != nil { + return warn.AttrPolicyRuleCompiled{}, err + } + + suppressible := true + if rule.Suppressible != nil { + suppressible = *rule.Suppressible + } + + return warn.AttrPolicyRuleCompiled{ + Name: name, + RuleKinds: append([]string(nil), rule.RuleKinds...), + Attr: attr, + Family: family, + ForbidValues: append([]string(nil), rule.ForbidValues...), + RequireValues: append([]string(nil), rule.RequireValues...), + ForbidListItems: append([]string(nil), rule.ForbidListItems...), + RequireListItems: append([]string(nil), rule.RequireListItems...), + ForbidDictEntries: copyStringMap(rule.ForbidDictEntries), + RequireDictEntries: copyStringMap(rule.RequireDictEntries), + ForbidDictKeys: append([]string(nil), rule.ForbidDictKeys...), + MinValue: cloneIntPtr(rule.MinValue), + MaxValue: cloneIntPtr(rule.MaxValue), + Required: rule.Required, + Allowlist: allowlist, + Suppressible: suppressible, + Message: rule.Message, + }, nil +} + +func attrPolicyConstraintFamily(rule *AttrPolicyRule) (warn.AttrPolicyConstraintFamily, int, error) { + scalar := len(rule.ForbidValues) > 0 || len(rule.RequireValues) > 0 + list := len(rule.ForbidListItems) > 0 || len(rule.RequireListItems) > 0 + dict := len(rule.ForbidDictEntries) > 0 || len(rule.RequireDictEntries) > 0 || len(rule.ForbidDictKeys) > 0 + numeric := rule.MinValue != nil || rule.MaxValue != nil + + families := 0 + var family warn.AttrPolicyConstraintFamily + if scalar { + families++ + family = warn.AttrPolicyScalarFamily + } + if list { + families++ + family = warn.AttrPolicyListFamily + } + if dict { + families++ + family = warn.AttrPolicyDictFamily + } + if numeric { + families++ + family = warn.AttrPolicyNumericFamily + } + if families > 1 { + return 0, families, fmt.Errorf("cannot mix scalar, list, dict, and numeric constraint families") + } + if numeric && rule.MinValue != nil && rule.MaxValue != nil && *rule.MinValue > *rule.MaxValue { + return 0, families, fmt.Errorf("minValue must be <= maxValue") + } + return family, families, nil +} + +func compileAllowlistPatterns(ruleName string, entries []string) ([]warn.AttrPolicyAllowlistPattern, error) { + var patterns []warn.AttrPolicyAllowlistPattern + for _, entry := range entries { + p, err := parseAllowlistPattern(entry) + if err != nil { + return nil, fmt.Errorf("attrPolicy rule %q: allowlist entry %q: %w", ruleName, entry, err) + } + patterns = append(patterns, p) + } + return patterns, nil +} + +func parseAllowlistPattern(entry string) (warn.AttrPolicyAllowlistPattern, error) { + if entry == "" { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("empty pattern") + } + if strings.HasPrefix(entry, "@") { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("repository-qualified entries are not supported") + } + if entry == "..." { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("bare ... is not supported") + } + if entry == "//..." { + return warn.AttrPolicyAllowlistPattern{Kind: warn.AttrPolicyAllowAll}, nil + } + if !strings.HasPrefix(entry, "//") { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("pattern must start with //") + } + if strings.HasSuffix(entry, "/...") { + pkg := strings.TrimPrefix(entry, "//") + pkg = strings.TrimSuffix(pkg, "/...") + if pkg == "" { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("invalid recursive pattern") + } + if strings.Contains(pkg, ":") { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("invalid recursive pattern") + } + return warn.AttrPolicyAllowlistPattern{Kind: warn.AttrPolicyAllowRecursive, Pkg: pkg}, nil + } + + label := labels.Parse(entry) + if label.Target == "all" || label.Target == "*" { + if label.Package == "" { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("invalid package pattern") + } + return warn.AttrPolicyAllowlistPattern{Kind: warn.AttrPolicyAllowPackageAll, Pkg: label.Package}, nil + } + return warn.AttrPolicyAllowlistPattern{ + Kind: warn.AttrPolicyAllowExact, + Pkg: label.Package, + Target: label.Target, + }, nil +} + +func copyStringMap(in map[string]string) map[string]string { + if len(in) == 0 { + return nil + } + out := make(map[string]string, len(in)) + for k, v := range in { + out[k] = v + } + return out +} + +func cloneIntPtr(v *int) *int { + if v == nil { + return nil + } + c := *v + return &c +} diff --git a/buildifier/config/attrpolicy_test.go b/buildifier/config/attrpolicy_test.go new file mode 100644 index 000000000..518dd23fc --- /dev/null +++ b/buildifier/config/attrpolicy_test.go @@ -0,0 +1,170 @@ +/* +Copyright 2026 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "strings" + "testing" + + "github.com/bazelbuild/buildtools/warn" +) + +func TestCompileAttrPolicy(t *testing.T) { + policy := &AttrPolicy{ + Rules: []AttrPolicyRule{ + { + Name: "no-eternal-timeout", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + ForbidValues: []string{"eternal"}, + Allowlist: []string{"//slow/...", "//foo:big_test"}, + }, + { + Name: "max-shard-count", + RuleKinds: []string{"*_test"}, + Attr: "shard_count", + MaxValue: intPtr(50), + }, + }, + } + compiled, err := compileAttrPolicy(policy) + if err != nil { + t.Fatalf("compileAttrPolicy() error = %v", err) + } + if len(compiled) != 2 { + t.Fatalf("len(compiled) = %d, want 2", len(compiled)) + } + if compiled[0].Name != "no-eternal-timeout" || compiled[0].Family != warn.AttrPolicyScalarFamily { + t.Fatalf("compiled[0] = %+v", compiled[0]) + } + if compiled[1].MaxValue == nil || *compiled[1].MaxValue != 50 { + t.Fatalf("compiled[1].MaxValue = %+v", compiled[1].MaxValue) + } +} + +func TestCompileAttrPolicyValidation(t *testing.T) { + for name, tc := range map[string]struct { + policy *AttrPolicy + wantErr string + }{ + "duplicate name": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}}, + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}}, + }}, + wantErr: `duplicate name`, + }, + "missing name": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Attr: "timeout", ForbidValues: []string{"eternal"}}, + }}, + wantErr: `name is required`, + }, + "missing attr": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", ForbidValues: []string{"eternal"}}, + }}, + wantErr: `attr is required`, + }, + "no constraints": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout"}, + }}, + wantErr: `at least one constraint`, + }, + "mixed families": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}, ForbidListItems: []string{"exclusive"}}, + }}, + wantErr: `cannot mix scalar, list, dict, and numeric`, + }, + "numeric min greater than max": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "shard_count", MinValue: intPtr(10), MaxValue: intPtr(5)}, + }}, + wantErr: `minValue must be <= maxValue`, + }, + "bad ruleKinds glob": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}, RuleKinds: []string{"["}}, + }}, + wantErr: `malformed ruleKinds`, + }, + "bad allowlist": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}, Allowlist: []string{"@repo//foo:bar"}}, + }}, + wantErr: `repository-qualified`, + }, + "required only": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", Required: true}, + }}, + }, + } { + t.Run(name, func(t *testing.T) { + _, err := compileAttrPolicy(tc.policy) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %v, want substring %q", err, tc.wantErr) + } + }) + } +} + +func TestParseAllowlistPattern(t *testing.T) { + for name, tc := range map[string]struct { + entry string + wantErr string + kind warn.AttrPolicyAllowlistKind + pkg string + target string + }{ + "all": {entry: "//...", kind: warn.AttrPolicyAllowAll}, + "exact": {entry: "//foo:bar", kind: warn.AttrPolicyAllowExact, pkg: "foo", target: "bar"}, + "package": {entry: "//pkg", kind: warn.AttrPolicyAllowExact, pkg: "pkg", target: "pkg"}, + "package all": {entry: "//pkg:all", kind: warn.AttrPolicyAllowPackageAll, pkg: "pkg"}, + "recursive": {entry: "//slow/...", kind: warn.AttrPolicyAllowRecursive, pkg: "slow"}, + "repo qualified": {entry: "@repo//foo:bar", wantErr: "repository-qualified"}, + "bare ellipsis": {entry: "...", wantErr: "bare ..."}, + } { + t.Run(name, func(t *testing.T) { + got, err := parseAllowlistPattern(tc.entry) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %v, want substring %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.Kind != tc.kind || got.Pkg != tc.pkg || got.Target != tc.target { + t.Fatalf("parseAllowlistPattern(%q) = %+v", tc.entry, got) + } + }) + } +} + +func intPtr(v int) *int { + return &v +} diff --git a/buildifier/config/buildifier.schema.json b/buildifier/config/buildifier.schema.json new file mode 100644 index 000000000..47a46e2e3 --- /dev/null +++ b/buildifier/config/buildifier.schema.json @@ -0,0 +1,271 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/bazelbuild/buildtools/blob/main/buildifier/config/buildifier.schema.json", + "title": "Buildifier configuration", + "description": "Schema for .buildifier.json. Use with editor JSON Schema support or agent tooling for safer config edits.", + "type": "object", + "additionalProperties": false, + "properties": { + "type": { + "type": "string", + "enum": ["auto", "build", "bzl", "workspace", "default", "module"], + "description": "Input file type. auto selects based on filename." + }, + "format": { + "type": "string", + "enum": ["text", "json"], + "description": "Diagnostics format when mode is check." + }, + "mode": { + "type": "string", + "enum": ["check", "diff", "fix", "print_if_changed"], + "description": "Formatting mode." + }, + "diffMode": { + "type": "boolean", + "description": "Alias for mode=diff." + }, + "lint": { + "type": "string", + "enum": ["off", "warn", "fix"], + "description": "Lint mode." + }, + "warnings": { + "type": "string", + "description": "Comma-separated warning identifiers, or all/default, optionally with +/- modifiers." + }, + "warningsList": { + "type": "array", + "items": { "type": "string" }, + "description": "List form of warnings." + }, + "recursive": { + "type": "boolean", + "description": "Find Starlark files recursively." + }, + "verbose": { + "type": "boolean" + }, + "diffCommand": { + "type": "string" + }, + "multiDiff": { + "type": "boolean" + }, + "tables": { + "type": "string", + "description": "Path to JSON tables file replacing built-in tables." + }, + "addTables": { + "type": "string", + "description": "Path to JSON tables file merged with built-in tables." + }, + "path": { + "type": "string", + "description": "Workspace-relative path assumed for a single formatted BUILD file." + }, + "buildifier_disable": { + "type": "array", + "items": { "type": "string" } + }, + "allowsort": { + "type": "array", + "items": { "type": "string" } + }, + "attrPolicy": { + "type": "object", + "description": "Config-driven attribute policy rules enforced by the attr-policy lint warning.", + "additionalProperties": false, + "properties": { + "rules": { + "type": "array", + "items": { "$ref": "#/$defs/attrPolicyRule" } + } + }, + "required": ["rules"] + } + }, + "$defs": { + "attrPolicyRule": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { + "type": "string", + "minLength": 1, + "description": "Stable rule identifier shown in findings. Must be unique." + }, + "ruleKinds": { + "type": "array", + "items": { "type": "string", "minLength": 1 }, + "description": "Globs matched against rule kind, e.g. *_test. Empty/absent matches all kinds." + }, + "attr": { + "type": "string", + "minLength": 1, + "description": "Attribute name to inspect." + }, + "forbidValues": { + "type": "array", + "items": { "type": "string" }, + "description": "Scalar attribute must not equal any listed value (string or boolean literal)." + }, + "requireValues": { + "type": "array", + "items": { "type": "string" }, + "description": "If the attribute is present, its value must be one of these." + }, + "forbidListItems": { + "type": "array", + "items": { "type": "string" }, + "description": "List attribute must not contain any listed item." + }, + "requireListItems": { + "type": "array", + "items": { "type": "string" }, + "description": "List attribute must contain all listed items." + }, + "forbidDictEntries": { + "type": "object", + "additionalProperties": { "type": "string" }, + "description": "Dict attribute must not contain these key/value pairs." + }, + "requireDictEntries": { + "type": "object", + "additionalProperties": { "type": "string" }, + "description": "Dict attribute must contain all listed key/value pairs." + }, + "forbidDictKeys": { + "type": "array", + "items": { "type": "string" }, + "description": "Dict attribute must not contain any listed key." + }, + "minValue": { + "type": "integer", + "description": "Numeric attribute must be greater than or equal to this value." + }, + "maxValue": { + "type": "integer", + "description": "Numeric attribute must be less than or equal to this value." + }, + "required": { + "type": "boolean", + "default": false, + "description": "Attribute must be present." + }, + "allowlist": { + "type": "array", + "items": { + "type": "string", + "pattern": "^//" + }, + "description": "Target patterns exempt from this rule, e.g. //pkg:name, //pkg:all, //pkg/..., //..." + }, + "suppressible": { + "type": "boolean", + "default": true, + "description": "Whether # buildifier: disable=attr-policy silences this rule." + }, + "message": { + "type": "string", + "description": "Custom finding message. Rule name is prefixed automatically." + } + }, + "required": ["name", "attr"], + "oneOf": [ + { + "description": "Scalar constraint family", + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] } + ], + "not": { + "anyOf": [ + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] } + ] + } + }, + { + "description": "List constraint family", + "anyOf": [ + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] } + ], + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] } + ] + } + }, + { + "description": "Dict constraint family", + "anyOf": [ + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] } + ], + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] } + ] + } + }, + { + "description": "Numeric constraint family", + "anyOf": [ + { "required": ["minValue"] }, + { "required": ["maxValue"] } + ], + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] } + ] + } + }, + { + "description": "Presence-only rule", + "required": ["required"], + "properties": { + "required": { "const": true } + }, + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] } + ] + } + } + ] + } + } +} diff --git a/buildifier/config/config.go b/buildifier/config/config.go index e9783b949..57f6705da 100644 --- a/buildifier/config/config.go +++ b/buildifier/config/config.go @@ -130,6 +130,8 @@ type Config struct { DisableRewrites ArrayFlags `json:"buildifier_disable,omitempty"` // AllowSort specifies additional sort contexts to treat as safe AllowSort ArrayFlags `json:"allowsort,omitempty"` + // AttrPolicy configures config-driven attribute policy lint rules. + AttrPolicy *AttrPolicy `json:"attrPolicy,omitempty"` // Help is true if the -h flag is set Help bool `json:"-"` @@ -231,6 +233,12 @@ func (c *Config) Validate(args []string) error { } } + compiledAttrPolicy, err := compileAttrPolicy(c.AttrPolicy) + if err != nil { + return err + } + warn.SetAttrPolicy(compiledAttrPolicy) + warningsList := c.WarningsList if c.Warnings != "" { warningsList = append(warningsList, c.Warnings) @@ -276,5 +284,17 @@ func Example() *Config { c.Mode = "fix" c.Lint = "fix" c.WarningsList = warn.AllWarnings + c.AttrPolicy = &AttrPolicy{ + Rules: []AttrPolicyRule{ + { + Name: "no-eternal-timeout", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + ForbidValues: []string{"eternal"}, + Allowlist: []string{"//slow/..."}, + Message: "'eternal' timeout requires approval; add the target to the attrPolicy allowlist.", + }, + }, + } return c } diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index c9291769e..a4af2227e 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -50,6 +50,7 @@ func ExampleExample() { // "attr-licenses", // "attr-non-empty", // "attr-output-default", + // "attr-policy", // "attr-single-file", // "build-args-kwargs", // "bzl-visibility", @@ -143,7 +144,25 @@ func ExampleExample() { // "unreachable", // "unsorted-dict-items", // "unused-variable" - // ] + // ], + // "attrPolicy": { + // "rules": [ + // { + // "name": "no-eternal-timeout", + // "ruleKinds": [ + // "*_test" + // ], + // "attr": "timeout", + // "forbidValues": [ + // "eternal" + // ], + // "allowlist": [ + // "//slow/..." + // ], + // "message": "'eternal' timeout requires approval; add the target to the attrPolicy allowlist." + // } + // ] + // } // } } @@ -271,6 +290,7 @@ func TestValidate(t *testing.T) { "attr-licenses", "attr-non-empty", "attr-output-default", + "attr-policy", "attr-single-file", "build-args-kwargs", "bzl-visibility", diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel index 377a38975..9ae0624ec 100644 --- a/warn/BUILD.bazel +++ b/warn/BUILD.bazel @@ -6,6 +6,8 @@ go_library( "multifile.go", "types.go", "warn.go", + "attr_policy.go", + "warn_attr_policy.go", "warn_bazel.go", "warn_bazel_api.go", "warn_bazel_operation.go", @@ -36,6 +38,7 @@ go_test( size = "small", srcs = [ "types_test.go", + "warn_attr_policy_test.go", "warn_bazel_api_test.go", "warn_bazel_operation_test.go", "warn_bazel_test.go", diff --git a/warn/attr_policy.go b/warn/attr_policy.go new file mode 100644 index 000000000..98428be24 --- /dev/null +++ b/warn/attr_policy.go @@ -0,0 +1,104 @@ +package warn + +import ( + "path" + "strings" + + "github.com/bazelbuild/buildtools/labels" +) + +// AttrPolicyConstraintFamily identifies which constraint fields apply to a policy rule. +type AttrPolicyConstraintFamily int + +const ( + AttrPolicyScalarFamily AttrPolicyConstraintFamily = iota + AttrPolicyListFamily + AttrPolicyDictFamily + AttrPolicyNumericFamily +) + +// AttrPolicyAllowlistKind is a compiled allow-list pattern kind. +type AttrPolicyAllowlistKind int + +const ( + AttrPolicyAllowAll AttrPolicyAllowlistKind = iota + AttrPolicyAllowExact + AttrPolicyAllowPackageAll + AttrPolicyAllowRecursive +) + +// AttrPolicyAllowlistPattern is a compiled allow-list entry. +type AttrPolicyAllowlistPattern struct { + Kind AttrPolicyAllowlistKind + Pkg string + Target string +} + +// AttrPolicyRuleCompiled is a compiled attribute policy rule. +type AttrPolicyRuleCompiled struct { + Name string + RuleKinds []string + Attr string + Family AttrPolicyConstraintFamily + + ForbidValues []string + RequireValues []string + ForbidListItems []string + RequireListItems []string + ForbidDictEntries map[string]string + RequireDictEntries map[string]string + ForbidDictKeys []string + MinValue *int + MaxValue *int + + Required bool + Allowlist []AttrPolicyAllowlistPattern + Suppressible bool + Message string +} + +// AttrPolicyConfig is process-global policy, set from buildifier config before linting. +var AttrPolicyConfig []AttrPolicyRuleCompiled + +// SetAttrPolicy replaces the active attribute policy rules. +func SetAttrPolicy(rules []AttrPolicyRuleCompiled) { + AttrPolicyConfig = rules +} + +func matchesRuleKind(globs []string, kind string) bool { + if len(globs) == 0 { + return true + } + for _, g := range globs { + if matched, err := path.Match(g, kind); err == nil && matched { + return true + } + } + return false +} + +func allowlistMatches(patterns []AttrPolicyAllowlistPattern, label, pkg string) bool { + for _, p := range patterns { + if allowlistPatternMatches(p, label, pkg) { + return true + } + } + return false +} + +func allowlistPatternMatches(p AttrPolicyAllowlistPattern, label, pkg string) bool { + switch p.Kind { + case AttrPolicyAllowAll: + return true + case AttrPolicyAllowRecursive: + l := labels.Parse(label) + return l.Package == p.Pkg || strings.HasPrefix(l.Package, p.Pkg+"/") + case AttrPolicyAllowPackageAll: + return labels.Parse(label).Package == p.Pkg + case AttrPolicyAllowExact: + want := labels.Label{Package: p.Pkg, Target: p.Target}.Format() + return labels.Equal(label, want, pkg) + default: + return false + } +} diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index 8be362ea0..a8220f829 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -83,6 +83,41 @@ warnings: { autofix: false } +warnings: { + name: "attr-policy" + header: "Attribute value violates a configured policy rule" + description: + "Enforces declarative attribute constraints configured in `.buildifier.json` under\n" + "`attrPolicy`. Each rule names an attribute and a constraint family (scalar,\n" + "list, dict, or numeric bounds). Targets matching an `allowlist` pattern are\n" + "exempt.\n\n" + "Example:\n\n" + "```json\n" + "{\n" + " \"attrPolicy\": {\n" + " \"rules\": [\n" + " {\n" + " \"name\": \"no-eternal-timeout\",\n" + " \"ruleKinds\": [\"*_test\"],\n" + " \"attr\": \"timeout\",\n" + " \"forbidValues\": [\"eternal\"],\n" + " \"allowlist\": [\"//slow/...\"]\n" + " },\n" + " {\n" + " \"name\": \"max-shard-count\",\n" + " \"ruleKinds\": [\"*_test\"],\n" + " \"attr\": \"shard_count\",\n" + " \"maxValue\": 50\n" + " }\n" + " ]\n" + " }\n" + "}\n" + "```\n\n" + "A JSON Schema for `.buildifier.json` (including `attrPolicy`) lives at\n" + "`buildifier/config/buildifier.schema.json`." + autofix: false +} + warnings: { name: "attr-single-file" header: "`single_file` is deprecated" diff --git a/warn/warn.go b/warn/warn.go index 2d5d71b44..a9d7479e1 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -117,6 +117,7 @@ var RuleWarningMap = map[string]func(call *build.CallExpr, pkg string) *LinterFi // FileWarningMap lists the warnings that run on the whole file. var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ "allowed-symbol-load-locations": symbolLoadLocationWarning, + "attr-policy": attrPolicyWarning, "attr-applicable_licenses": attrApplicableLicensesWarning, "attr-cfg": attrConfigurationWarning, "attr-license": attrLicenseWarning, @@ -225,6 +226,7 @@ var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader) // nonDefaultWarnings contains warnings that are enabled by default because they're not applicable // for all files and cause too much diff noise when applied. var nonDefaultWarnings = map[string]bool{ + "attr-policy": true, "unsorted-dict-items": true, // dict items should be sorted } diff --git a/warn/warn_attr_policy.go b/warn/warn_attr_policy.go new file mode 100644 index 000000000..6e63ce0b4 --- /dev/null +++ b/warn/warn_attr_policy.go @@ -0,0 +1,164 @@ +package warn + +import ( + "fmt" + "slices" + "strconv" + "strings" + + "github.com/bazelbuild/buildtools/build" + "github.com/bazelbuild/buildtools/edit" + "github.com/bazelbuild/buildtools/labels" +) + +func attrPolicyWarning(f *build.File) []*LinterFinding { + if f.Type != build.TypeBuild || len(AttrPolicyConfig) == 0 { + return nil + } + var findings []*LinterFinding + for _, rule := range f.Rules("") { + kind := rule.Kind() + label := labels.Label{Package: f.Pkg, Target: rule.Name()}.Format() + for _, p := range AttrPolicyConfig { + if !matchesRuleKind(p.RuleKinds, kind) || allowlistMatches(p.Allowlist, label, f.Pkg) { + continue + } + findings = append(findings, attrPolicyCheckRule(rule, p)...) + } + } + return findings +} + +func attrPolicyCheckRule(rule *build.Rule, p AttrPolicyRuleCompiled) []*LinterFinding { + var findings []*LinterFinding + attrExpr := rule.Attr(p.Attr) + + if p.Required && attrExpr == nil { + findings = append(findings, makeLinterFinding(rule.Call, attrPolicyMessage(p, + fmt.Sprintf("attribute %q is required", p.Attr)))) + return findings + } + + switch p.Family { + case AttrPolicyScalarFamily: + if attrExpr == nil { + return findings + } + value := attrScalarString(rule, p.Attr) + for _, forbidden := range p.ForbidValues { + if value == forbidden { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not be %q", p.Attr, forbidden)))) + break + } + } + if len(p.RequireValues) > 0 && !slices.Contains(p.RequireValues, value) { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must be one of %s", p.Attr, quoteList(p.RequireValues))))) + } + case AttrPolicyListFamily: + if attrExpr == nil { + return findings + } + items := rule.AttrStrings(p.Attr) + if items == nil { + return findings + } + for _, forbidden := range p.ForbidListItems { + if slices.Contains(items, forbidden) { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not contain %q", p.Attr, forbidden)))) + } + } + for _, required := range p.RequireListItems { + if !slices.Contains(items, required) { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must contain %q", p.Attr, required)))) + } + } + case AttrPolicyDictFamily: + if attrExpr == nil { + return findings + } + dict, ok := attrExpr.(*build.DictExpr) + if !ok { + return findings + } + for key, forbiddenValue := range p.ForbidDictEntries { + if valueExpr := edit.DictionaryGet(dict, key); valueExpr != nil { + if exprScalarString(valueExpr) == forbiddenValue { + findings = append(findings, makeLinterFinding(valueExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not contain %q: %q", p.Attr, key, forbiddenValue)))) + } + } + } + for key, requiredValue := range p.RequireDictEntries { + valueExpr := edit.DictionaryGet(dict, key) + if valueExpr == nil || exprScalarString(valueExpr) != requiredValue { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must contain %q: %q", p.Attr, key, requiredValue)))) + } + } + for _, key := range p.ForbidDictKeys { + if valueExpr := edit.DictionaryGet(dict, key); valueExpr != nil { + findings = append(findings, makeLinterFinding(valueExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not contain key %q", p.Attr, key)))) + } + } + case AttrPolicyNumericFamily: + if attrExpr == nil { + return findings + } + value, err := strconv.Atoi(rule.AttrLiteral(p.Attr)) + if err != nil { + return findings + } + if p.MinValue != nil && value < *p.MinValue { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must be >= %d", p.Attr, *p.MinValue)))) + } + if p.MaxValue != nil && value > *p.MaxValue { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must be <= %d", p.Attr, *p.MaxValue)))) + } + } + return findings +} + +func attrPolicyMessage(p AttrPolicyRuleCompiled, defaultMsg string) string { + if p.Message != "" { + return fmt.Sprintf("[%s] %s", p.Name, p.Message) + } + return fmt.Sprintf("[%s] %s", p.Name, defaultMsg) +} + +func attrScalarString(rule *build.Rule, key string) string { + if s := rule.AttrString(key); s != "" { + return s + } + return rule.AttrLiteral(key) +} + +func exprScalarString(expr build.Expr) string { + if expr == nil { + return "" + } + if s, ok := expr.(*build.StringExpr); ok { + return s.Value + } + if i, ok := expr.(*build.Ident); ok { + return i.Name + } + if l, ok := expr.(*build.LiteralExpr); ok { + return l.Token + } + return "" +} + +func quoteList(values []string) string { + quoted := make([]string, len(values)) + for i, v := range values { + quoted[i] = fmt.Sprintf("%q", v) + } + return strings.Join(quoted, ", ") +} diff --git a/warn/warn_attr_policy_test.go b/warn/warn_attr_policy_test.go new file mode 100644 index 000000000..a05a33bec --- /dev/null +++ b/warn/warn_attr_policy_test.go @@ -0,0 +1,189 @@ +/* +Copyright 2026 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package warn + +import ( + "testing" +) + +func attrPolicyTestRules() []AttrPolicyRuleCompiled { + return []AttrPolicyRuleCompiled{ + { + Name: "no-eternal-timeout", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + Family: AttrPolicyScalarFamily, + ForbidValues: []string{"eternal"}, + Allowlist: []AttrPolicyAllowlistPattern{ + {Kind: AttrPolicyAllowExact, Pkg: "test/package", Target: "big_test"}, + }, + }, + { + Name: "no-exclusive-tests", + RuleKinds: []string{"*_test"}, + Attr: "tags", + Family: AttrPolicyListFamily, + ForbidListItems: []string{"exclusive"}, + }, + { + Name: "no-local-tests", + RuleKinds: []string{"*_test"}, + Attr: "local", + Family: AttrPolicyScalarFamily, + ForbidValues: []string{"True"}, + }, + { + Name: "no-no-cache", + Attr: "execution_requirements", + Family: AttrPolicyDictFamily, + ForbidDictEntries: map[string]string{ + "no-cache": "1", + }, + }, + { + Name: "max-shard-count", + RuleKinds: []string{"*_test"}, + Attr: "shard_count", + Family: AttrPolicyNumericFamily, + MaxValue: intPtr(50), + Allowlist: []AttrPolicyAllowlistPattern{ + {Kind: AttrPolicyAllowExact, Pkg: "test/package", Target: "massive_test"}, + }, + }, + } +} + +func intPtr(v int) *int { + return &v +} + +func TestAttrPolicyWarning(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy(attrPolicyTestRules()) + + checkFindings(t, "attr-policy", ` +cc_test(name = "ok", timeout = "short") +cc_test(name = "bad", timeout = "eternal") +cc_test(name = "big_test", timeout = "eternal") +cc_test(name = "exclusive", tags = ["exclusive"]) +cc_test(name = "local", local = True) +cc_test(name = "cached", execution_requirements = {"no-cache": "1"}) +cc_test(name = "sharded", shard_count = 100) +cc_test(name = "massive_test", shard_count = 100) +cc_library(name = "lib", timeout = "eternal") +`, []string{ + `:2: [no-eternal-timeout] attribute "timeout" must not be "eternal"`, + `:4: [no-exclusive-tests] attribute "tags" must not contain "exclusive"`, + `:5: [no-local-tests] attribute "local" must not be "True"`, + `:6: [no-no-cache] attribute "execution_requirements" must not contain "no-cache": "1"`, + `:7: [max-shard-count] attribute "shard_count" must be <= 50`, + }, scopeBuild) + + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "recursive-allow", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + Family: AttrPolicyScalarFamily, + ForbidValues: []string{"eternal"}, + Allowlist: []AttrPolicyAllowlistPattern{ + {Kind: AttrPolicyAllowRecursive, Pkg: "slow"}, + }, + }, + }) + cleanup := setUpTestPackage("slow/nested") + defer cleanup() + checkFindings(t, "attr-policy", ` +cc_test(name = "allowed", timeout = "eternal") +`, nil, scopeBuild) + + cleanup2 := setUpTestPackage("fast") + defer cleanup2() + checkFindings(t, "attr-policy", ` +cc_test(name = "bad", timeout = "eternal") +`, []string{ + `:1: [recursive-allow] attribute "timeout" must not be "eternal"`, + }, scopeBuild) + + SetAttrPolicy(nil) + checkFindings(t, "attr-policy", ` +cc_test(name = "bad", timeout = "eternal") +`, nil, scopeBuild) +} + +func TestAttrPolicyRequired(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "needs-timeout", + Attr: "timeout", + Family: AttrPolicyScalarFamily, + Required: true, + }, + }) + checkFindings(t, "attr-policy", ` +cc_test(name = "missing") +cc_test(name = "present", timeout = "short") +`, []string{ + `:1: [needs-timeout] attribute "timeout" is required`, + }, scopeBuild) +} + +func TestAttrPolicyRuleKindGlob(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "tests-only", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + Family: AttrPolicyScalarFamily, + ForbidValues: []string{"eternal"}, + }, + }) + checkFindings(t, "attr-policy", ` +cc_test(name = "bad", timeout = "eternal") +cc_library(name = "lib", timeout = "eternal") +`, []string{ + `:1: [tests-only] attribute "timeout" must not be "eternal"`, + }, scopeBuild) +} + +func TestAllowlistPatternMatches(t *testing.T) { + tests := []struct { + pattern AttrPolicyAllowlistPattern + label string + pkg string + want bool + }{ + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowAll}, "//any:target", "any", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowExact, Pkg: "foo", Target: "bar"}, "//foo:bar", "foo", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowExact, Pkg: "foo", Target: "bar"}, "//foo:baz", "foo", false}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowPackageAll, Pkg: "foo"}, "//foo:bar", "foo", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowPackageAll, Pkg: "foo"}, "//bar:baz", "bar", false}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowRecursive, Pkg: "slow"}, "//slow:big", "slow", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowRecursive, Pkg: "slow"}, "//slow/nested:big", "slow/nested", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowRecursive, Pkg: "slow"}, "//fast:big", "fast", false}, + } + for _, tc := range tests { + if got := allowlistPatternMatches(tc.pattern, tc.label, tc.pkg); got != tc.want { + t.Errorf("allowlistPatternMatches(%+v, %q, %q) = %v, want %v", tc.pattern, tc.label, tc.pkg, got, tc.want) + } + } +} From abe64c84fcaf9492d36a83c10f5e5d50f126057e Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 6 Jul 2026 10:15:12 -0700 Subject: [PATCH 11/16] Fix CI: sort BUILD srcs and update integration test golden. Match buildifier --config=example output after attr-policy and attrPolicy were added to Example(). --- buildifier/config/BUILD.bazel | 2 +- buildifier/integration_test.sh | 21 ++++++++++++++++++++- warn/BUILD.bazel | 2 +- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/buildifier/config/BUILD.bazel b/buildifier/config/BUILD.bazel index 7c6362c41..20803e38a 100644 --- a/buildifier/config/BUILD.bazel +++ b/buildifier/config/BUILD.bazel @@ -21,7 +21,7 @@ go_library( go_test( name = "config_test", - srcs = ["config_test.go", "attrpolicy_test.go"], + srcs = ["attrpolicy_test.go", "config_test.go"], embed = [":config"], ) diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh index b9a6389c5..3df3633fa 100755 --- a/buildifier/integration_test.sh +++ b/buildifier/integration_test.sh @@ -265,6 +265,7 @@ cat > golden/.buildifier.example.json < golden/.buildifier.example.json < Date: Tue, 7 Jul 2026 06:17:23 -0700 Subject: [PATCH 12/16] Address Gemini review feedback on attr-policy design and lint. Use valid JSON in config examples, anchor list findings on offending items, document flakiness formula boundary cases, and skip stale targets in testpolicy. --- ...attribute-policy-and-test-tuning-design.md | 44 ++++++++++++++----- warn/warn_attr_policy.go | 19 +++++++- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 7cf81200d..b1e8f39b6 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -103,15 +103,18 @@ generic and lets each repo express its own policy. ### 3.2 Config schema (extends `.buildifier.json`) -```jsonc +`.buildifier.json` is parsed with standard `encoding/json` (no comments). Examples +below are valid JSON you can copy into config. + +```json { "attrPolicy": { "rules": [ { - "name": "no-eternal-timeout", // stable id, shown in the finding - "ruleKinds": ["*_test"], // globs matched against rule.Kind(); omit/[] = any kind + "name": "no-eternal-timeout", + "ruleKinds": ["*_test"], "attr": "timeout", - "forbidValues": ["eternal"], // scalar-attr constraint + "forbidValues": ["eternal"], "allowlist": ["//slow/...", "//foo:big_test"], "message": "'eternal' timeout requires approval; add the target to the attrPolicy allowlist." }, @@ -119,20 +122,20 @@ generic and lets each repo express its own policy. "name": "no-exclusive-tests", "ruleKinds": ["*_test"], "attr": "tags", - "forbidListItems": ["exclusive"], // list-membership constraint + "forbidListItems": ["exclusive"], "allowlist": [] }, { "name": "no-local-tests", "ruleKinds": ["*_test"], "attr": "local", - "forbidValues": ["True"], // boolean literal constraint (see below) + "forbidValues": ["True"], "message": "Tests must not set local = True; use a hermetic test instead." }, { "name": "no-no-cache", "attr": "execution_requirements", - "forbidDictEntries": { // dict key→value constraint (see below) + "forbidDictEntries": { "no-cache": "1" }, "message": "Do not set execution_requirements['no-cache'] = '1'." @@ -141,7 +144,7 @@ generic and lets each repo express its own policy. "name": "max-shard-count", "ruleKinds": ["*_test"], "attr": "shard_count", - "maxValue": 50, // numeric range constraint (see below) + "maxValue": 50, "allowlist": ["//huge_suite:..."], "message": "shard_count must not exceed 50; add the target to the allowlist for larger suites." } @@ -226,9 +229,12 @@ equality; `/...` via `pkg == P || strings.HasPrefix(pkg, P+"/")`. its label matches **no** entry in `allowlist`. - Target label computed as `//{f.Pkg}:{rule.Name()}` and tested with `allowlistMatch` per the grammar above (not a bare `labels.Equal`, which can't express patterns). -- Finding is anchored on the offending attribute node - (`rule.Attr(attr).Span()`); if the constraint is `required` and the attr is missing, - anchor on `rule.Call`. +- Finding is anchored on the offending node. Default: the attribute expression + (`rule.Attr(attr)`). Exceptions for precise IDE highlighting: + - **List items:** anchor on the matching string literal inside the list (e.g. the + `"exclusive"` entry in `tags`), not the whole list. + - **Dict entries:** anchor on the offending value node when possible. + - **Missing required attr:** anchor on `rule.Call`. ### 3.3 Go types @@ -333,7 +339,8 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { - `p.check` handles the constraint families: - **Scalars:** `attrScalarString(rule, attr)` → `rule.AttrString`, else `rule.AttrLiteral`; compare against `forbidValues` / `requireValues`. - - **Lists:** `rule.AttrStrings`. + - **Lists:** `rule.AttrStrings`; for `forbidListItems`, anchor on the matching + list element node (not the whole list). - **Dicts:** `rule.Attr(attr)` as `*build.DictExpr`; `edit.DictionaryGet` per key; normalize values like scalars. `forbidDictKeys` flags any listed key that is present. @@ -474,6 +481,14 @@ Answers "does a single retry likely pass, or does it need multiple?". N ≥ ceil( log(1 - T) / log(1 - a) ) ``` +**Boundary cases** (handle before applying the formula): + +| Condition | Handling | +|---|---| +| `a = 1` (never fails) | `N = 1`; recommend `flaky = 0` / leave unset | +| `a = 0` or `a` below a minimum threshold (e.g. 0.05) | Classify as chronically broken; do **not** recommend retries | +| `a` very close to 1 (e.g. `a ≥ 0.999`) | Treat as `a = 1` to avoid `log(0)` | + - `N` is used internally to distinguish "a single retry almost always recovers it" from "retries rarely help", which drives the `flaky` recommendation: | `N` | Meaning | Recommendation | @@ -525,6 +540,10 @@ buildozer 'set shard_count 4' //pkg:sharded batching edits, opening PRs, and routing to reviewers are downstream responsibilities of whatever CI/automation calls `testpolicy`. Keeping the boundary here makes the tool trivially testable (assert on emitted commands) and reusable by any apply/review flow. +- **Skip stale targets:** warehouse rows may refer to deleted or renamed targets. + Before emitting a `buildozer` line, verify the label still exists in the workspace + (e.g. index targets while walking BUILD files, or a fast `bazel query` check). Report + skipped labels in the JSON output instead of emitting commands that would fail. ### 4.7 Safety / guardrails @@ -539,6 +558,7 @@ buildozer 'set shard_count 4' //pkg:sharded minimum-sample-size guard avoids acting on noise. - Respect the eternal allow-list from `.buildifier.json` so the two systems agree. - Dry-run is the default mode. +- Do not emit `buildozer` commands for labels absent from the workspace (see §4.6). ### 4.8 Acceptance criteria (Workstream B) diff --git a/warn/warn_attr_policy.go b/warn/warn_attr_policy.go index 6e63ce0b4..20c4ba6e1 100644 --- a/warn/warn_attr_policy.go +++ b/warn/warn_attr_policy.go @@ -66,7 +66,11 @@ func attrPolicyCheckRule(rule *build.Rule, p AttrPolicyRuleCompiled) []*LinterFi } for _, forbidden := range p.ForbidListItems { if slices.Contains(items, forbidden) { - findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + node := attrExpr + if itemExpr := listItemExpr(attrExpr, forbidden); itemExpr != nil { + node = itemExpr + } + findings = append(findings, makeLinterFinding(node, attrPolicyMessage(p, fmt.Sprintf("attribute %q must not contain %q", p.Attr, forbidden)))) } } @@ -162,3 +166,16 @@ func quoteList(values []string) string { } return strings.Join(quoted, ", ") } + +func listItemExpr(attrExpr build.Expr, item string) build.Expr { + list, ok := attrExpr.(*build.ListExpr) + if !ok { + return nil + } + for _, elem := range list.List { + if str, ok := elem.(*build.StringExpr); ok && str.Value == item { + return elem + } + } + return nil +} From df97879bfad2228594e1d1c242d0708b24f90a45 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 8 Jul 2026 05:33:57 -0700 Subject: [PATCH 13/16] chore(bazel): buildifier --- buildifier/config/BUILD.bazel | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/buildifier/config/BUILD.bazel b/buildifier/config/BUILD.bazel index 20803e38a..4cffd85d1 100644 --- a/buildifier/config/BUILD.bazel +++ b/buildifier/config/BUILD.bazel @@ -21,7 +21,10 @@ go_library( go_test( name = "config_test", - srcs = ["attrpolicy_test.go", "config_test.go"], + srcs = [ + "attrpolicy_test.go", + "config_test.go", + ], embed = [":config"], ) From ea52fddd0d498028a93f9106d77a447bfad37fff Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 15 Jul 2026 09:25:21 -0600 Subject: [PATCH 14/16] Enable attr-policy by default with Bazel shard_count constraint. Include attr-policy in DefaultWarnings and apply a built-in max-shard-count rule (shard_count <= 50 on *_test rules) when attrPolicy is unset, matching Bazel's default test attribute validation. --- WARNINGS.md | 4 ++- buildifier/config/attrpolicy.go | 4 +-- buildifier/config/attrpolicy_test.go | 33 +++++++++++++++++++ buildifier/config/config_test.go | 3 ++ ...attribute-policy-and-test-tuning-design.md | 14 ++++---- warn/attr_policy.go | 27 +++++++++++++++ warn/docs/warnings.textproto | 4 ++- warn/warn.go | 1 - warn/warn_attr_policy.go | 5 +-- warn/warn_attr_policy_test.go | 6 +++- 10 files changed, 87 insertions(+), 14 deletions(-) diff --git a/WARNINGS.md b/WARNINGS.md index c063b37c0..01c1c07ea 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -236,7 +236,6 @@ Using `package_metadata` as an attribute name may cause unexpected behavior. Its * Category name: `attr-policy` * Automatic fix: no - * [Disabled by default](buildifier/README.md#linter) * [Suppress the warning](#suppress): `# buildifier: disable=attr-policy` Enforces declarative attribute constraints configured in `.buildifier.json` under @@ -244,6 +243,9 @@ Enforces declarative attribute constraints configured in `.buildifier.json` unde list, dict, or numeric bounds). Targets matching an `allowlist` pattern are exempt. +When no `attrPolicy` block is configured, the warning applies Bazel's default +`shard_count` constraint on test rules (`shard_count` must be ≤ 50). + Example: ```json diff --git a/buildifier/config/attrpolicy.go b/buildifier/config/attrpolicy.go index aa9c99463..420f580d5 100644 --- a/buildifier/config/attrpolicy.go +++ b/buildifier/config/attrpolicy.go @@ -35,8 +35,8 @@ type AttrPolicyRule struct { } func compileAttrPolicy(policy *AttrPolicy) ([]warn.AttrPolicyRuleCompiled, error) { - if policy == nil { - return nil, nil + if policy == nil || len(policy.Rules) == 0 { + return warn.DefaultAttrPolicyRules(), nil } seen := make(map[string]bool) var compiled []warn.AttrPolicyRuleCompiled diff --git a/buildifier/config/attrpolicy_test.go b/buildifier/config/attrpolicy_test.go index 518dd23fc..020f92fbd 100644 --- a/buildifier/config/attrpolicy_test.go +++ b/buildifier/config/attrpolicy_test.go @@ -24,6 +24,39 @@ import ( ) func TestCompileAttrPolicy(t *testing.T) { + for name, tc := range map[string]struct { + policy *AttrPolicy + wantLen int + wantName string + }{ + "nil uses default shard_count rule": { + policy: nil, + wantLen: 1, + wantName: "max-shard-count", + }, + "empty rules uses default shard_count rule": { + policy: &AttrPolicy{}, + wantLen: 1, + wantName: "max-shard-count", + }, + } { + t.Run(name, func(t *testing.T) { + compiled, err := compileAttrPolicy(tc.policy) + if err != nil { + t.Fatalf("compileAttrPolicy() error = %v", err) + } + if len(compiled) != tc.wantLen { + t.Fatalf("len(compiled) = %d, want %d", len(compiled), tc.wantLen) + } + if compiled[0].Name != tc.wantName { + t.Fatalf("compiled[0].Name = %q, want %q", compiled[0].Name, tc.wantName) + } + if compiled[0].MaxValue == nil || *compiled[0].MaxValue != 50 { + t.Fatalf("compiled[0].MaxValue = %+v", compiled[0].MaxValue) + } + }) + } + policy := &AttrPolicy{ Rules: []AttrPolicyRule{ { diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index a4af2227e..57c6fe066 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -393,6 +393,7 @@ func TestValidate(t *testing.T) { "attr-licenses", "attr-non-empty", "attr-output-default", + "attr-policy", "attr-single-file", "build-args-kwargs", "bzl-visibility", @@ -495,6 +496,7 @@ func TestValidate(t *testing.T) { "attr-licenses", "attr-non-empty", "attr-output-default", + "attr-policy", "attr-single-file", "build-args-kwargs", "bzl-visibility", @@ -597,6 +599,7 @@ func TestValidate(t *testing.T) { "attr-licenses", "attr-non-empty", "attr-output-default", + "attr-policy", "attr-single-file", "build-args-kwargs", "bzl-visibility", diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index b1e8f39b6..8f9dcbf8c 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -356,9 +356,10 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { ### 3.7 Registration, docs, tests - Register in `warn/warn.go`: `FileWarningMap["attr-policy"] = attrPolicyWarning`. -- Decide default-on vs. opt-in: add to `nonDefaultWarnings` if it should be opt-in - (recommended, since it no-ops without config anyway — but being config-gated it's - harmless in the default set too; pick opt-in to be conservative). +- Decide default-on vs. opt-in: **default-on** (included in `DefaultWarnings`). When no + `attrPolicy` config is present, the warning enforces Bazel's default `shard_count` + constraint (`maxValue: 50` on `*_test` rules). Custom rules replace the default + entirely once `attrPolicy.rules` is non-empty. - Docs: add an entry to `WARNINGS.md` and `warn/docs/warnings.textproto` describing the warning **and** the `attrPolicy` config block (with the example rules, including boolean, dict, and numeric constraints). @@ -373,7 +374,7 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { forbidden boolean literal (`local = True`); forbidden dict entry (`execution_requirements = {"no-cache": "1"}`); `forbidDictKeys` on a dict key; `shard_count` above `maxValue` flagged, within range OK, absent OK; allow-listed - high-shard target exempt; empty config = no findings. + high-shard target exempt; empty config applies default `shard_count` ≤ 50 rule. - Config tests in `buildifier/config/config_test.go`: parse the sample JSON; validation rejects malformed rules. @@ -686,8 +687,9 @@ Each task is independently ownable; dependencies noted. "AC" = acceptance criter 1. **Import direction** between `warn` and `buildifier/config` — confirm no cycle (§3.4). If one exists, the compiled-policy-type-in-`warn` approach resolves it. -2. Should `attr-policy` be **default-on** (config-gated no-op) or opt-in via - `--warnings`? (Leaning opt-in.) +2. ~~Should `attr-policy` be **default-on** (config-gated no-op) or opt-in via + `--warnings`?~~ **Resolved:** default-on; with no `attrPolicy` config, enforces + Bazel's `shard_count` ≤ 50 constraint on test rules. 3. **`NonSuppressible` core change (§6.4)** — do we extend buildifier so hard rules can't be silenced by `disable=` locally, accepting a break to the "every warning is suppressible" contract? Or rely solely on the CI gate? (Leaning CI-gate-only for the diff --git a/warn/attr_policy.go b/warn/attr_policy.go index 98428be24..9ca9f6ddf 100644 --- a/warn/attr_policy.go +++ b/warn/attr_policy.go @@ -57,6 +57,26 @@ type AttrPolicyRuleCompiled struct { Message string } +const defaultShardCountMax = 50 + +// DefaultAttrPolicyRules returns the built-in attribute policy rules applied when +// no attrPolicy configuration is present. Currently this mirrors Bazel's default +// shard_count constraint on test rules. +func DefaultAttrPolicyRules() []AttrPolicyRuleCompiled { + maxValue := defaultShardCountMax + return []AttrPolicyRuleCompiled{ + { + Name: "max-shard-count", + RuleKinds: []string{"*_test"}, + Attr: "shard_count", + Family: AttrPolicyNumericFamily, + MaxValue: &maxValue, + Message: "Having more than 50 shards is indicative of poor test organization. Please reduce the number of shards.", + Suppressible: true, + }, + } +} + // AttrPolicyConfig is process-global policy, set from buildifier config before linting. var AttrPolicyConfig []AttrPolicyRuleCompiled @@ -65,6 +85,13 @@ func SetAttrPolicy(rules []AttrPolicyRuleCompiled) { AttrPolicyConfig = rules } +func effectiveAttrPolicyConfig() []AttrPolicyRuleCompiled { + if len(AttrPolicyConfig) > 0 { + return AttrPolicyConfig + } + return DefaultAttrPolicyRules() +} + func matchesRuleKind(globs []string, kind string) bool { if len(globs) == 0 { return true diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index a8220f829..e7623ee77 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -87,10 +87,12 @@ warnings: { name: "attr-policy" header: "Attribute value violates a configured policy rule" description: - "Enforces declarative attribute constraints configured in `.buildifier.json` under\n" + "Enforces declarative attribute constraints configured in `.buildifier.json` under\n" "`attrPolicy`. Each rule names an attribute and a constraint family (scalar,\n" "list, dict, or numeric bounds). Targets matching an `allowlist` pattern are\n" "exempt.\n\n" + "When no `attrPolicy` block is configured, the warning applies Bazel's default\n" + "`shard_count` constraint on test rules (`shard_count` must be ≤ 50).\n\n" "Example:\n\n" "```json\n" "{\n" diff --git a/warn/warn.go b/warn/warn.go index a9d7479e1..f09b04dc6 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -226,7 +226,6 @@ var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader) // nonDefaultWarnings contains warnings that are enabled by default because they're not applicable // for all files and cause too much diff noise when applied. var nonDefaultWarnings = map[string]bool{ - "attr-policy": true, "unsorted-dict-items": true, // dict items should be sorted } diff --git a/warn/warn_attr_policy.go b/warn/warn_attr_policy.go index 20c4ba6e1..272aa5576 100644 --- a/warn/warn_attr_policy.go +++ b/warn/warn_attr_policy.go @@ -12,14 +12,15 @@ import ( ) func attrPolicyWarning(f *build.File) []*LinterFinding { - if f.Type != build.TypeBuild || len(AttrPolicyConfig) == 0 { + if f.Type != build.TypeBuild { return nil } + config := effectiveAttrPolicyConfig() var findings []*LinterFinding for _, rule := range f.Rules("") { kind := rule.Kind() label := labels.Label{Package: f.Pkg, Target: rule.Name()}.Format() - for _, p := range AttrPolicyConfig { + for _, p := range config { if !matchesRuleKind(p.RuleKinds, kind) || allowlistMatches(p.Allowlist, label, f.Pkg) { continue } diff --git a/warn/warn_attr_policy_test.go b/warn/warn_attr_policy_test.go index a05a33bec..b3993eb93 100644 --- a/warn/warn_attr_policy_test.go +++ b/warn/warn_attr_policy_test.go @@ -123,7 +123,11 @@ cc_test(name = "bad", timeout = "eternal") SetAttrPolicy(nil) checkFindings(t, "attr-policy", ` cc_test(name = "bad", timeout = "eternal") -`, nil, scopeBuild) +cc_test(name = "sharded", shard_count = 100) +cc_test(name = "ok", shard_count = 4) +`, []string{ + `:2: [max-shard-count] Having more than 50 shards is indicative of poor test organization. Please reduce the number of shards.`, + }, scopeBuild) } func TestAttrPolicyRequired(t *testing.T) { From b13398bde758a659e8b66923bde21d83fc6fab18 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 15 Jul 2026 10:56:12 -0600 Subject: [PATCH 15/16] Add default attr-policy checks for deprecated license attributes. Introduce forbidPresence constraint support and flag licenses on all rules and output_licenses on binary-producing rule kinds when attrPolicy is unset, since Bazel provides no built-in warning for these deprecated attributes. --- WARNINGS.md | 7 +++- buildifier/config/attrpolicy.go | 13 +++++- buildifier/config/attrpolicy_test.go | 23 +++++++++-- buildifier/config/buildifier.schema.json | 41 ++++++++++++++++--- ...attribute-policy-and-test-tuning-design.md | 3 +- warn/attr_policy.go | 39 ++++++++++++++---- warn/docs/warnings.textproto | 7 +++- warn/warn_attr_policy.go | 5 +++ warn/warn_attr_policy_test.go | 34 +++++++++++++++ 9 files changed, 149 insertions(+), 23 deletions(-) diff --git a/WARNINGS.md b/WARNINGS.md index 01c1c07ea..f9db9946c 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -243,8 +243,11 @@ Enforces declarative attribute constraints configured in `.buildifier.json` unde list, dict, or numeric bounds). Targets matching an `allowlist` pattern are exempt. -When no `attrPolicy` block is configured, the warning applies Bazel's default -`shard_count` constraint on test rules (`shard_count` must be ≤ 50). +When no `attrPolicy` block is configured, the warning applies these built-in rules: + +* `shard_count` must be ≤ 50 on `*_test` rules (matching Bazel's default constraint) +* `licenses` must not be set on any rule (deprecated; see [bazel#188](https://github.com/bazelbuild/bazel/issues/188)) +* `output_licenses` must not be set on `genrule`, `cc_binary`, `cc_toolchain`, `java_binary`, or `java_plugin` (deprecated; see [bazel#7444](https://github.com/bazelbuild/bazel/issues/7444)) Example: diff --git a/buildifier/config/attrpolicy.go b/buildifier/config/attrpolicy.go index 420f580d5..28566aa7a 100644 --- a/buildifier/config/attrpolicy.go +++ b/buildifier/config/attrpolicy.go @@ -29,6 +29,7 @@ type AttrPolicyRule struct { MinValue *int `json:"minValue,omitempty"` MaxValue *int `json:"maxValue,omitempty"` Required bool `json:"required,omitempty"` + ForbidPresence bool `json:"forbidPresence,omitempty"` Allowlist []string `json:"allowlist,omitempty"` Suppressible *bool `json:"suppressible,omitempty"` Message string `json:"message,omitempty"` @@ -70,8 +71,11 @@ func compileAttrPolicyRule(rule *AttrPolicyRule, seen map[string]bool) (warn.Att if err != nil { return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: %w", name, err) } + if rule.ForbidPresence && rule.Required { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: forbidPresence and required cannot both be true", name) + } if families == 0 && !rule.Required { - return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: at least one constraint or required=true is required", name) + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: at least one constraint, required=true, or forbidPresence=true is required", name) } for _, kindGlob := range rule.RuleKinds { @@ -116,6 +120,7 @@ func attrPolicyConstraintFamily(rule *AttrPolicyRule) (warn.AttrPolicyConstraint list := len(rule.ForbidListItems) > 0 || len(rule.RequireListItems) > 0 dict := len(rule.ForbidDictEntries) > 0 || len(rule.RequireDictEntries) > 0 || len(rule.ForbidDictKeys) > 0 numeric := rule.MinValue != nil || rule.MaxValue != nil + presence := rule.ForbidPresence families := 0 var family warn.AttrPolicyConstraintFamily @@ -135,8 +140,12 @@ func attrPolicyConstraintFamily(rule *AttrPolicyRule) (warn.AttrPolicyConstraint families++ family = warn.AttrPolicyNumericFamily } + if presence { + families++ + family = warn.AttrPolicyForbidPresenceFamily + } if families > 1 { - return 0, families, fmt.Errorf("cannot mix scalar, list, dict, and numeric constraint families") + return 0, families, fmt.Errorf("cannot mix scalar, list, dict, numeric, and presence constraint families") } if numeric && rule.MinValue != nil && rule.MaxValue != nil && *rule.MinValue > *rule.MaxValue { return 0, families, fmt.Errorf("minValue must be <= maxValue") diff --git a/buildifier/config/attrpolicy_test.go b/buildifier/config/attrpolicy_test.go index 020f92fbd..9018f78b7 100644 --- a/buildifier/config/attrpolicy_test.go +++ b/buildifier/config/attrpolicy_test.go @@ -31,12 +31,12 @@ func TestCompileAttrPolicy(t *testing.T) { }{ "nil uses default shard_count rule": { policy: nil, - wantLen: 1, + wantLen: 3, wantName: "max-shard-count", }, "empty rules uses default shard_count rule": { policy: &AttrPolicy{}, - wantLen: 1, + wantLen: 3, wantName: "max-shard-count", }, } { @@ -123,7 +123,7 @@ func TestCompileAttrPolicyValidation(t *testing.T) { policy: &AttrPolicy{Rules: []AttrPolicyRule{ {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}, ForbidListItems: []string{"exclusive"}}, }}, - wantErr: `cannot mix scalar, list, dict, and numeric`, + wantErr: `cannot mix scalar, list, dict, numeric, and presence`, }, "numeric min greater than max": { policy: &AttrPolicy{Rules: []AttrPolicyRule{ @@ -148,6 +148,23 @@ func TestCompileAttrPolicyValidation(t *testing.T) { {Name: "x", Attr: "timeout", Required: true}, }}, }, + "forbid presence only": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "licenses", ForbidPresence: true}, + }}, + }, + "forbid presence and required": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "licenses", ForbidPresence: true, Required: true}, + }}, + wantErr: `forbidPresence and required cannot both be true`, + }, + "forbid presence mixed with scalar": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidPresence: true, ForbidValues: []string{"eternal"}}, + }}, + wantErr: `cannot mix scalar, list, dict, numeric, and presence`, + }, } { t.Run(name, func(t *testing.T) { _, err := compileAttrPolicy(tc.policy) diff --git a/buildifier/config/buildifier.schema.json b/buildifier/config/buildifier.schema.json index 47a46e2e3..cbbb36809 100644 --- a/buildifier/config/buildifier.schema.json +++ b/buildifier/config/buildifier.schema.json @@ -153,6 +153,11 @@ "default": false, "description": "Attribute must be present." }, + "forbidPresence": { + "type": "boolean", + "default": false, + "description": "Attribute must not be present." + }, "allowlist": { "type": "array", "items": { @@ -187,7 +192,8 @@ { "required": ["requireDictEntries"] }, { "required": ["forbidDictKeys"] }, { "required": ["minValue"] }, - { "required": ["maxValue"] } + { "required": ["maxValue"] }, + { "required": ["forbidPresence"] } ] } }, @@ -205,7 +211,8 @@ { "required": ["requireDictEntries"] }, { "required": ["forbidDictKeys"] }, { "required": ["minValue"] }, - { "required": ["maxValue"] } + { "required": ["maxValue"] }, + { "required": ["forbidPresence"] } ] } }, @@ -223,7 +230,8 @@ { "required": ["forbidListItems"] }, { "required": ["requireListItems"] }, { "required": ["minValue"] }, - { "required": ["maxValue"] } + { "required": ["maxValue"] }, + { "required": ["forbidPresence"] } ] } }, @@ -241,7 +249,8 @@ { "required": ["requireListItems"] }, { "required": ["forbidDictEntries"] }, { "required": ["requireDictEntries"] }, - { "required": ["forbidDictKeys"] } + { "required": ["forbidDictKeys"] }, + { "required": ["forbidPresence"] } ] } }, @@ -261,7 +270,29 @@ { "required": ["requireDictEntries"] }, { "required": ["forbidDictKeys"] }, { "required": ["minValue"] }, - { "required": ["maxValue"] } + { "required": ["maxValue"] }, + { "required": ["forbidPresence"] } + ] + } + }, + { + "description": "Forbid-presence rule", + "required": ["forbidPresence"], + "properties": { + "forbidPresence": { "const": true } + }, + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] }, + { "required": ["required"] } ] } } diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 8f9dcbf8c..48f671023 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -374,7 +374,8 @@ func attrPolicyWarning(f *build.File) []*LinterFinding { forbidden boolean literal (`local = True`); forbidden dict entry (`execution_requirements = {"no-cache": "1"}`); `forbidDictKeys` on a dict key; `shard_count` above `maxValue` flagged, within range OK, absent OK; allow-listed - high-shard target exempt; empty config applies default `shard_count` ≤ 50 rule. + high-shard target exempt; empty config applies built-in defaults (`shard_count` ≤ 50, + deprecated `licenses`, deprecated `output_licenses` on binary-producing rules). - Config tests in `buildifier/config/config_test.go`: parse the sample JSON; validation rejects malformed rules. diff --git a/warn/attr_policy.go b/warn/attr_policy.go index 9ca9f6ddf..1db99ed74 100644 --- a/warn/attr_policy.go +++ b/warn/attr_policy.go @@ -15,6 +15,7 @@ const ( AttrPolicyListFamily AttrPolicyDictFamily AttrPolicyNumericFamily + AttrPolicyForbidPresenceFamily ) // AttrPolicyAllowlistKind is a compiled allow-list pattern kind. @@ -59,19 +60,41 @@ type AttrPolicyRuleCompiled struct { const defaultShardCountMax = 50 +var defaultOutputLicensesRuleKinds = []string{ + "genrule", + "cc_binary", + "cc_toolchain", + "java_binary", + "java_plugin", +} + // DefaultAttrPolicyRules returns the built-in attribute policy rules applied when -// no attrPolicy configuration is present. Currently this mirrors Bazel's default -// shard_count constraint on test rules. +// no attrPolicy configuration is present. func DefaultAttrPolicyRules() []AttrPolicyRuleCompiled { maxValue := defaultShardCountMax return []AttrPolicyRuleCompiled{ { - Name: "max-shard-count", - RuleKinds: []string{"*_test"}, - Attr: "shard_count", - Family: AttrPolicyNumericFamily, - MaxValue: &maxValue, - Message: "Having more than 50 shards is indicative of poor test organization. Please reduce the number of shards.", + Name: "max-shard-count", + RuleKinds: []string{"*_test"}, + Attr: "shard_count", + Family: AttrPolicyNumericFamily, + MaxValue: &maxValue, + Message: "Having more than 50 shards is indicative of poor test organization. Please reduce the number of shards.", + Suppressible: true, + }, + { + Name: "no-licenses", + Attr: "licenses", + Family: AttrPolicyForbidPresenceFamily, + Message: "The licenses attribute is deprecated; use package(default_applicable_licenses = ...) and applicable_licenses on targets instead (https://github.com/bazelbuild/bazel/issues/188).", + Suppressible: true, + }, + { + Name: "no-output-licenses", + RuleKinds: append([]string(nil), defaultOutputLicensesRuleKinds...), + Attr: "output_licenses", + Family: AttrPolicyForbidPresenceFamily, + Message: "The output_licenses attribute is deprecated; use applicable_licenses instead (https://github.com/bazelbuild/bazel/issues/7444).", Suppressible: true, }, } diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index e7623ee77..5f3485dad 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -91,8 +91,11 @@ warnings: { "`attrPolicy`. Each rule names an attribute and a constraint family (scalar,\n" "list, dict, or numeric bounds). Targets matching an `allowlist` pattern are\n" "exempt.\n\n" - "When no `attrPolicy` block is configured, the warning applies Bazel's default\n" - "`shard_count` constraint on test rules (`shard_count` must be ≤ 50).\n\n" + "When no `attrPolicy` block is configured, the warning applies these built-in rules:\n\n" + "* `shard_count` must be ≤ 50 on `*_test` rules (matching Bazel's default constraint)\n" + "* `licenses` must not be set on any rule (deprecated; see [bazel#188](https://github.com/bazelbuild/bazel/issues/188))\n" + "* `output_licenses` must not be set on `genrule`, `cc_binary`, `cc_toolchain`, " + "`java_binary`, or `java_plugin` (deprecated; see [bazel#7444](https://github.com/bazelbuild/bazel/issues/7444))\n\n" "Example:\n\n" "```json\n" "{\n" diff --git a/warn/warn_attr_policy.go b/warn/warn_attr_policy.go index 272aa5576..e92c697a0 100644 --- a/warn/warn_attr_policy.go +++ b/warn/warn_attr_policy.go @@ -126,6 +126,11 @@ func attrPolicyCheckRule(rule *build.Rule, p AttrPolicyRuleCompiled) []*LinterFi findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, fmt.Sprintf("attribute %q must be <= %d", p.Attr, *p.MaxValue)))) } + case AttrPolicyForbidPresenceFamily: + if attrExpr != nil { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not be set", p.Attr)))) + } } return findings } diff --git a/warn/warn_attr_policy_test.go b/warn/warn_attr_policy_test.go index b3993eb93..b760ca970 100644 --- a/warn/warn_attr_policy_test.go +++ b/warn/warn_attr_policy_test.go @@ -125,8 +125,42 @@ cc_test(name = "bad", timeout = "eternal") cc_test(name = "bad", timeout = "eternal") cc_test(name = "sharded", shard_count = 100) cc_test(name = "ok", shard_count = 4) +cc_library(name = "lib", licenses = ["notice"]) +cc_binary(name = "bin", licenses = ["notice"], output_licenses = ["notice"]) +cc_library(name = "lib2", output_licenses = ["notice"]) `, []string{ `:2: [max-shard-count] Having more than 50 shards is indicative of poor test organization. Please reduce the number of shards.`, + `:4: [no-licenses] The licenses attribute is deprecated; use package(default_applicable_licenses = ...) and applicable_licenses on targets instead (https://github.com/bazelbuild/bazel/issues/188).`, + `:5: [no-licenses] The licenses attribute is deprecated; use package(default_applicable_licenses = ...) and applicable_licenses on targets instead (https://github.com/bazelbuild/bazel/issues/188).`, + `:5: [no-output-licenses] The output_licenses attribute is deprecated; use applicable_licenses instead (https://github.com/bazelbuild/bazel/issues/7444).`, + }, scopeBuild) +} + +func TestAttrPolicyForbidPresence(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "no-licenses", + Attr: "licenses", + Family: AttrPolicyForbidPresenceFamily, + }, + { + Name: "no-output-licenses", + RuleKinds: append([]string(nil), defaultOutputLicensesRuleKinds...), + Attr: "output_licenses", + Family: AttrPolicyForbidPresenceFamily, + }, + }) + + checkFindings(t, "attr-policy", ` +cc_library(name = "lib", licenses = ["notice"]) +cc_binary(name = "bin", output_licenses = ["notice"]) +cc_library(name = "lib2", output_licenses = ["notice"]) +cc_library(name = "clean") +`, []string{ + `:1: [no-licenses] attribute "licenses" must not be set`, + `:2: [no-output-licenses] attribute "output_licenses" must not be set`, }, scopeBuild) } From a39b779ee653c1dc8cedc44a4a364da1713d8ab1 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 3 Aug 2026 11:42:39 -0700 Subject: [PATCH 16/16] Add allowListItems list constraint to attr-policy. Support closed-world list lexicons with exact matches and trailing-* prefix patterns, enabling tag allow-list enforcement without a separate buildtools patch. --- WARNINGS.md | 10 ++++- buildifier/config/attrpolicy.go | 4 +- buildifier/config/attrpolicy_test.go | 5 +++ buildifier/config/buildifier.schema.json | 13 +++++- ...attribute-policy-and-test-tuning-design.md | 1 + warn/attr_policy.go | 1 + warn/docs/warnings.textproto | 10 ++++- warn/warn_attr_policy.go | 28 +++++++++++++ warn/warn_attr_policy_test.go | 40 +++++++++++++++++++ 9 files changed, 106 insertions(+), 6 deletions(-) diff --git a/WARNINGS.md b/WARNINGS.md index f9db9946c..f3297692d 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -240,8 +240,9 @@ Using `package_metadata` as an attribute name may cause unexpected behavior. Its Enforces declarative attribute constraints configured in `.buildifier.json` under `attrPolicy`. Each rule names an attribute and a constraint family (scalar, -list, dict, or numeric bounds). Targets matching an `allowlist` pattern are -exempt. +list, dict, numeric bounds, or presence). List rules support `allowListItems` for +closed-world lexicons; entries ending in `*` match by prefix. Targets matching an +`allowlist` pattern are exempt. When no `attrPolicy` block is configured, the warning applies these built-in rules: @@ -267,6 +268,11 @@ Example: "ruleKinds": ["*_test"], "attr": "shard_count", "maxValue": 50 + }, + { + "name": "allowed-tags", + "attr": "tags", + "allowListItems": ["manual", "assistant-ds*", "exclusive"] } ] } diff --git a/buildifier/config/attrpolicy.go b/buildifier/config/attrpolicy.go index 28566aa7a..01663cc7e 100644 --- a/buildifier/config/attrpolicy.go +++ b/buildifier/config/attrpolicy.go @@ -23,6 +23,7 @@ type AttrPolicyRule struct { RequireValues []string `json:"requireValues,omitempty"` ForbidListItems []string `json:"forbidListItems,omitempty"` RequireListItems []string `json:"requireListItems,omitempty"` + AllowListItems []string `json:"allowListItems,omitempty"` ForbidDictEntries map[string]string `json:"forbidDictEntries,omitempty"` RequireDictEntries map[string]string `json:"requireDictEntries,omitempty"` ForbidDictKeys []string `json:"forbidDictKeys,omitempty"` @@ -103,6 +104,7 @@ func compileAttrPolicyRule(rule *AttrPolicyRule, seen map[string]bool) (warn.Att RequireValues: append([]string(nil), rule.RequireValues...), ForbidListItems: append([]string(nil), rule.ForbidListItems...), RequireListItems: append([]string(nil), rule.RequireListItems...), + AllowListItems: append([]string(nil), rule.AllowListItems...), ForbidDictEntries: copyStringMap(rule.ForbidDictEntries), RequireDictEntries: copyStringMap(rule.RequireDictEntries), ForbidDictKeys: append([]string(nil), rule.ForbidDictKeys...), @@ -117,7 +119,7 @@ func compileAttrPolicyRule(rule *AttrPolicyRule, seen map[string]bool) (warn.Att func attrPolicyConstraintFamily(rule *AttrPolicyRule) (warn.AttrPolicyConstraintFamily, int, error) { scalar := len(rule.ForbidValues) > 0 || len(rule.RequireValues) > 0 - list := len(rule.ForbidListItems) > 0 || len(rule.RequireListItems) > 0 + list := len(rule.ForbidListItems) > 0 || len(rule.RequireListItems) > 0 || len(rule.AllowListItems) > 0 dict := len(rule.ForbidDictEntries) > 0 || len(rule.RequireDictEntries) > 0 || len(rule.ForbidDictKeys) > 0 numeric := rule.MinValue != nil || rule.MaxValue != nil presence := rule.ForbidPresence diff --git a/buildifier/config/attrpolicy_test.go b/buildifier/config/attrpolicy_test.go index 9018f78b7..70657fdd4 100644 --- a/buildifier/config/attrpolicy_test.go +++ b/buildifier/config/attrpolicy_test.go @@ -153,6 +153,11 @@ func TestCompileAttrPolicyValidation(t *testing.T) { {Name: "x", Attr: "licenses", ForbidPresence: true}, }}, }, + "allow list items only": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "tags", AllowListItems: []string{"manual", "assistant-ds*"}}, + }}, + }, "forbid presence and required": { policy: &AttrPolicy{Rules: []AttrPolicyRule{ {Name: "x", Attr: "licenses", ForbidPresence: true, Required: true}, diff --git a/buildifier/config/buildifier.schema.json b/buildifier/config/buildifier.schema.json index cbbb36809..0acbdf2bb 100644 --- a/buildifier/config/buildifier.schema.json +++ b/buildifier/config/buildifier.schema.json @@ -125,6 +125,11 @@ "items": { "type": "string" }, "description": "List attribute must contain all listed items." }, + "allowListItems": { + "type": "array", + "items": { "type": "string", "minLength": 1 }, + "description": "Every list item must match an allowed value. Entries ending in * are prefix patterns; all others are exact matches." + }, "forbidDictEntries": { "type": "object", "additionalProperties": { "type": "string" }, @@ -188,6 +193,7 @@ "anyOf": [ { "required": ["forbidListItems"] }, { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, { "required": ["forbidDictEntries"] }, { "required": ["requireDictEntries"] }, { "required": ["forbidDictKeys"] }, @@ -201,7 +207,8 @@ "description": "List constraint family", "anyOf": [ { "required": ["forbidListItems"] }, - { "required": ["requireListItems"] } + { "required": ["requireListItems"] }, + { "required": ["allowListItems"] } ], "not": { "anyOf": [ @@ -229,6 +236,7 @@ { "required": ["requireValues"] }, { "required": ["forbidListItems"] }, { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, { "required": ["minValue"] }, { "required": ["maxValue"] }, { "required": ["forbidPresence"] } @@ -247,6 +255,7 @@ { "required": ["requireValues"] }, { "required": ["forbidListItems"] }, { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, { "required": ["forbidDictEntries"] }, { "required": ["requireDictEntries"] }, { "required": ["forbidDictKeys"] }, @@ -266,6 +275,7 @@ { "required": ["requireValues"] }, { "required": ["forbidListItems"] }, { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, { "required": ["forbidDictEntries"] }, { "required": ["requireDictEntries"] }, { "required": ["forbidDictKeys"] }, @@ -287,6 +297,7 @@ { "required": ["requireValues"] }, { "required": ["forbidListItems"] }, { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, { "required": ["forbidDictEntries"] }, { "required": ["requireDictEntries"] }, { "required": ["forbidDictKeys"] }, diff --git a/docs/attribute-policy-and-test-tuning-design.md b/docs/attribute-policy-and-test-tuning-design.md index 48f671023..8ca90164d 100644 --- a/docs/attribute-policy-and-test-tuning-design.md +++ b/docs/attribute-policy-and-test-tuning-design.md @@ -164,6 +164,7 @@ below are valid JSON you can copy into config. | `requireValues` | []string | If attr present, must equal one of these. (If also want "must be present", see `required`.) | | `forbidListItems` | []string | List attr must not contain any of these items. | | `requireListItems` | []string | List attr must contain all of these items. | +| `allowListItems` | []string | Every list item must match an allowed value. Entries ending in `*` are prefix patterns; all others are exact matches. | | `forbidDictEntries` | object (string→string) | Dict attr must not contain any of these key→value pairs. | | `requireDictEntries` | object (string→string) | Dict attr must contain all of these key→value pairs. | | `forbidDictKeys` | []string | Dict attr must not contain any of these keys (value ignored). | diff --git a/warn/attr_policy.go b/warn/attr_policy.go index 1db99ed74..b45b85caa 100644 --- a/warn/attr_policy.go +++ b/warn/attr_policy.go @@ -46,6 +46,7 @@ type AttrPolicyRuleCompiled struct { RequireValues []string ForbidListItems []string RequireListItems []string + AllowListItems []string ForbidDictEntries map[string]string RequireDictEntries map[string]string ForbidDictKeys []string diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index 5f3485dad..bd9d2276b 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -89,8 +89,9 @@ warnings: { description: "Enforces declarative attribute constraints configured in `.buildifier.json` under\n" "`attrPolicy`. Each rule names an attribute and a constraint family (scalar,\n" - "list, dict, or numeric bounds). Targets matching an `allowlist` pattern are\n" - "exempt.\n\n" + "list, dict, numeric bounds, or presence). List rules support `allowListItems` for\n" + "closed-world lexicons; entries ending in `*` match by prefix. Targets matching an\n" + "`allowlist` pattern are exempt.\n\n" "When no `attrPolicy` block is configured, the warning applies these built-in rules:\n\n" "* `shard_count` must be ≤ 50 on `*_test` rules (matching Bazel's default constraint)\n" "* `licenses` must not be set on any rule (deprecated; see [bazel#188](https://github.com/bazelbuild/bazel/issues/188))\n" @@ -113,6 +114,11 @@ warnings: { " \"ruleKinds\": [\"*_test\"],\n" " \"attr\": \"shard_count\",\n" " \"maxValue\": 50\n" + " },\n" + " {\n" + " \"name\": \"allowed-tags\",\n" + " \"attr\": \"tags\",\n" + " \"allowListItems\": [\"manual\", \"assistant-ds*\", \"exclusive\"]\n" " }\n" " ]\n" " }\n" diff --git a/warn/warn_attr_policy.go b/warn/warn_attr_policy.go index e92c697a0..ce2aa9cc4 100644 --- a/warn/warn_attr_policy.go +++ b/warn/warn_attr_policy.go @@ -81,6 +81,17 @@ func attrPolicyCheckRule(rule *build.Rule, p AttrPolicyRuleCompiled) []*LinterFi fmt.Sprintf("attribute %q must contain %q", p.Attr, required)))) } } + for _, item := range items { + if len(p.AllowListItems) > 0 && !allowListItemMatches(p.AllowListItems, item) { + node := attrExpr + if itemExpr := listItemExpr(attrExpr, item); itemExpr != nil { + node = itemExpr + } + findings = append(findings, makeLinterFinding(node, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not contain %q; allowed values are %s", + p.Attr, item, quoteList(p.AllowListItems))))) + } + } case AttrPolicyDictFamily: if attrExpr == nil { return findings @@ -185,3 +196,20 @@ func listItemExpr(attrExpr build.Expr, item string) build.Expr { } return nil } + +// allowListItemMatches reports whether item satisfies any allowListItems entry. +// Entries ending in * are prefix patterns; all other entries are exact matches. +func allowListItemMatches(allowed []string, item string) bool { + for _, pattern := range allowed { + if strings.HasSuffix(pattern, "*") { + if strings.HasPrefix(item, strings.TrimSuffix(pattern, "*")) { + return true + } + continue + } + if item == pattern { + return true + } + } + return false +} diff --git a/warn/warn_attr_policy_test.go b/warn/warn_attr_policy_test.go index b760ca970..552fff080 100644 --- a/warn/warn_attr_policy_test.go +++ b/warn/warn_attr_policy_test.go @@ -203,6 +203,46 @@ cc_library(name = "lib", timeout = "eternal") }, scopeBuild) } +func TestAttrPolicyAllowListItems(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "allowed-tags", + Attr: "tags", + Family: AttrPolicyListFamily, + AllowListItems: []string{"manual", "assistant-ds*", "exclusive"}, + }, + }) + + checkFindings(t, "attr-policy", ` +cc_test(name = "ok", tags = ["manual", "assistant-ds-community", "exclusive"]) +cc_test(name = "bad", tags = ["my-new-tag"]) +`, []string{ + `:2: [allowed-tags] attribute "tags" must not contain "my-new-tag"; allowed values are "manual", "assistant-ds*", "exclusive"`, + }, scopeBuild) +} + +func TestAllowListItemMatches(t *testing.T) { + tests := []struct { + allowed []string + item string + want bool + }{ + {[]string{"manual", "exclusive"}, "manual", true}, + {[]string{"manual", "exclusive"}, "exclusive", true}, + {[]string{"manual", "exclusive"}, "other", false}, + {[]string{"assistant-ds*"}, "assistant-ds", true}, + {[]string{"assistant-ds*"}, "assistant-ds-community", true}, + {[]string{"assistant-ds*"}, "assistant-hero", false}, + } + for _, tc := range tests { + if got := allowListItemMatches(tc.allowed, tc.item); got != tc.want { + t.Errorf("allowListItemMatches(%v, %q) = %v, want %v", tc.allowed, tc.item, got, tc.want) + } + } +} + func TestAllowlistPatternMatches(t *testing.T) { tests := []struct { pattern AttrPolicyAllowlistPattern