Add config-driven attr-policy linting - #1479
Conversation
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) <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces a design document for implementing static attribute-policy linting in buildifier and empirical test-tuning in a new testpolicy tool. The review feedback provides valuable improvements to the design, including ensuring the configuration examples use standard JSON to prevent parsing errors, refining linter error anchoring for list attributes, handling mathematical boundary cases in the flakiness formula, and adding guardrails to verify target existence before running buildozer commands.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Show how policies like local = True and execution_requirements entries are encoded via forbidValues and forbidDictEntries.
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.
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.
Match buildifier --config=example output after attr-policy and attrPolicy were added to Example().
Use valid JSON in config examples, anchor list findings on offending items, document flakiness formula boundary cases, and skip stale targets in testpolicy.
|
@vladmos could you at least comment whether this FR is in a shape that can be accepted upstream? |
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.
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.
|
|
||
| func compileAttrPolicy(policy *AttrPolicy) ([]warn.AttrPolicyRuleCompiled, error) { | ||
| if policy == nil || len(policy.Rules) == 0 { | ||
| return warn.DefaultAttrPolicyRules(), nil |
There was a problem hiding this comment.
(moving the discussion from the private messaging to Github for visibility)
Should we provide a way to let people define their own rules and use the default at the same time, without copying and maintaining the list of default rules? My suggestion was to allow something like
"attrPolicy": {
"keepDefault": false, // true by default
"rules": [...],
}
would it be a common usecase that a repo maintainer wants to define something custom for their repo without disabling the default checks?
There was a problem hiding this comment.
I would guess the majority of corporate monorepo users benefit from creating some enforcement of their local conventions (for example, a lexicon of allowed tags values so they don't proliferate meaninglessly)
Personally, I'd like to keep the full list in one place, so I would just copy and modify the default settings. But I'm happy to follow your guidance.
Some complications with keepDefault:
- To understand the policy, you have to look up the defaults - but if they change from one release to the next, it's easy to make the mistake of looking up the HEAD documentation and concluding you have something enforced.
- have to explain and test for override situation where default has one value and user has another. Especially tricky for nested values: given a default
{
"name": "no-eternal-timeout",
"ruleKinds": ["*_test"],
"attr": "timeout",
"forbidValues": ["eternal"],
"allowlist": ["//slow/..."]
}
what happens if a user does
"keepDefault": true,
...
{
"name": "no-long-timeout",
"ruleKinds": ["*_test"],
"attr": "timeout",
"forbidValues": ["long"],
"allowlist": ["//other/..."]
}
does the forbidValues compose, or is it overridden? How would a user say that no timeout values should be forbidden? What about "allowlist"? Can users provide a name that collides with a default?
|
(note, I'm getting my CLA resolved, that check should be green early next week) |
Support closed-world list lexicons with exact matches and trailing-* prefix patterns, enabling tag allow-list enforcement without a separate buildtools patch.
Summary
attr-policy: config-driven BUILD attribute checks viaattrPolicyin.buildifier.json, plus a JSON schema for config validation.shard_count≤ 50 on tests, and flags deprecatedlicenses/output_licensesusage.licensesandoutput_licensesare legacy attrs from the pre-rules_licenseera (bazel#188, bazel#7444). Bazel still accepts them silently — no analysis error, no--incompatible_*flag (contrastdistribs, which had--incompatible_no_package_distribs), and the built-indeprecationattribute only warns on deprecated targets, not deprecated attribute names. Existing buildifierattr-licensesonly catches the name in rule definitions (.bzl), not on BUILD targets. These default checks are the practical way to surface and finish removal.testpolicytool (not implemented here).Closes #1480
Test plan
bazel test //warn:warn_test //buildifier/config:config_test //warn/docs:docs_test