diff --git a/.agents/skills/spotbugs/SKILL.md b/.agents/skills/spotbugs/SKILL.md new file mode 100644 index 00000000000..30ddf924ac5 --- /dev/null +++ b/.agents/skills/spotbugs/SKILL.md @@ -0,0 +1,84 @@ +--- +name: spotbugs +description: >- + Enable one mechanically enforceable lint or static-analysis rule in blocking CI, + preferring SpotBugs. Use when adding a SpotBugs detector, turning a repetitive PR + review theme into a lint, adding a ForbiddenAPIs signature, or writing a custom + Gradle source check. +user-invocable: true +context: fork +allowed-tools: + - Bash + - Read + - Grep + - Glob + - Edit + - Write +--- + +# Add a SpotBugs check + +Grow the blocking SpotBugs job **one check at a time**. Do not turn this +into a style-guide dump or a Spotless / google-java-format gate. + +Background: [`gradle/spotbugs.gradle`](../../../gradle/spotbugs.gradle), +[`gradle/spotbugFilters/exclude.xml`](../../../gradle/spotbugFilters/exclude.xml), +[detector list](https://spotbugs-in-kengo-toda.readthedocs.io/en/lqc-list-detectors/detectors.html). +Fallbacks: [other vehicles](references/other-vehicles.md). + +## This repository + +- First-party production sources: every Java module that applies `gradle/java_no_deps.gradle` +- Tasks that may report: `spotbugsMain`, `spotbugsMain_java11` +- Run: `./gradlew spotbugsMain -PskipTests` (optionally qualify with `:module:spotbugsMain`) +- CI: `check_base` / `check_inst` / `check_profiling` / `check_debugger` run Gradle `check`, which includes those tasks + +Preserve tests, smoke tests (`:dd-smoke-tests`), generated sources, and vendored trees out unless the user explicitly expands the filter. + +`omitVisitors` in `gradle/spotbugs.gradle` is the deferred-check list. `exclude.xml` is the path/class filter. `@SuppressFBWarnings` is the local override. + +## Hard constraints + +- Enable **one** check per change. Prefer removing one name from `omitVisitors` over adding a new detector. +- Do not enable a detector family or bulk-remove `omitVisitors` entries. +- Keep `ignoreFailures = false` on Main tasks. Do not mark GitLab `check_*` jobs `allow_failure`. +- Do not enable SpotBugs on test or smoke-test source sets unless asked. +- Production source sets include `main` and version-specific sets such as `main_java11`, `main_java17`, `main_java21` and `main_java25`, etc. Check the shared SpotBugs convention for the enabled sets and report any coverage gaps. +- Do not widen `exclude.xml`, `onlyAnalyze`, or the production-source task filter unless asked. +- Do not use `@SuppressFBWarnings` as the way to land a noisy check. If the detector is too noisy, leave the visitor omitted. For an intentional local exception, keep the detector enabled and annotate that site with `@SuppressFBWarnings(value = "…", justification = "…")`. +- Do not treat this as a format change. Run `./gradlew spotlessApply` only if you touched Java/Gradle files that Spotless owns. +- Keep `omitVisitors` accurate: a deferred detector belongs in that list, not in a comment elsewhere. + +## Workflow + +1. Classify the rule. Prefer SpotBugs. Read `gradle/spotbugs.gradle` and the [detector list](https://spotbugs-in-kengo-toda.readthedocs.io/en/lqc-list-detectors/detectors.html). + - If a visitor in `omitVisitors` matches, that is the check to enable. + - If a matching visitor is absent from `omitVisitors`, check its defaults in the SpotBugs version in use and the task configuration. Stop only if the detector is already enabled. + - If no SpotBugs detector can express the rule, follow [other vehicles](references/other-vehicles.md). Do not invent a custom SpotBugs detector unless the user asks. +2. Enable one visitor in the shared SpotBugs convention. Remove it from `omitVisitors` if present, or explicitly enable it if it is disabled by default. Review all bug patterns it reports and use `@SuppressFBWarnings` with a justification for intentional exceptions. +3. Run SpotBugs from the repo root: + + ```bash + ./gradlew spotbugsMain -PskipTests + ``` + + If the rule is scoped to one module, run that module's `spotbugsMain` first, then a broader `spotbugsMain` before finishing. +4. If it fails: + - Mechanical first-party fixes in `src/main/java` (and `src/main/java11`) are OK in the same change. + - Do not edit generated, vendored, or smoke-test sources to silence SpotBugs. + - Do not paper over findings with a blanket `exclude.xml` match. A justified `@SuppressFBWarnings` on an intentional exception is OK; a pile of suppressions to make a noisy detector pass is not. + - If the cleanup is large or opinionated, undo this change's visitor activation and stop. Report the failing classes and the changes they need to the main assistant, so it can ask the user before starting a repo-wide rewrite. +5. Keep the check only if `spotbugsMain` exits 0. + +## Local command + +```bash +./gradlew spotbugsMain -PskipTests +``` + +HTML reports: `*/build/reports/spotbugs/spotbugsMain.html`. +Bug codes in those reports (`NP_…`, `DCN_…`) are **not** the `omitVisitors` names. Visitors are detector class names (`FindNullDeref`, `DefaultEncodingDetector`). Suppressions use the bug code: + +```java +@SuppressFBWarnings(value = "NP_BOOLEAN_RETURN_NULL", justification = "…") +``` diff --git a/.agents/skills/spotbugs/references/other-vehicles.md b/.agents/skills/spotbugs/references/other-vehicles.md new file mode 100644 index 00000000000..75ccd354f6e --- /dev/null +++ b/.agents/skills/spotbugs/references/other-vehicles.md @@ -0,0 +1,69 @@ +# Other vehicles (only if SpotBugs cannot express the rule) + +Use this file after the [SpotBugs skill](../SKILL.md) classification step finds no matching detector. Do not start here. + +Pick the **narrowest** vehicle that can fail CI with low false positives. One rule per change, same as SpotBugs. + +## ForbiddenAPIs — unconditional banned call or class + +Plugin: `de.thetaphi.forbiddenapis`. Grep for that id (or `apply plugin: "de.thetaphi.forbiddenapis"`) and follow `signaturesFiles` to the shared lists; do not add a new signature file. + +Two lists exist today: + +- `main.txt` — common rules, applied by the default plugin config +- `instrumentation.txt` — extra rules the instrumentation convention adds on top of `main.txt` + +Put a repo-wide ban in `main.txt`. Put a ban that should apply only to instrumentation modules in `instrumentation.txt`. Leave plugin/task enablement in the module configuration. + +Use when the rule is “never call this method/class” with **no location qualifier** (not “only in `@Advice`”, not “only in premain”). + +1. Add or update one rule in the matching signature file, including all required overloads. + - For a new rule, set its own `@defaultMessage` before its signatures, or give each signature an explicit inline message. + - For an existing rule, update its signatures and message in place. Keep added overloads with that rule and check which signatures inherit a changed `@defaultMessage`. + + For example, when extending a `System.getenv()` ban to its other overload, keep both signatures under the same message: + + ```text + @defaultMessage Use ConfigHelper to access environment variables. + java.lang.System#getenv() + java.lang.System#getenv(java.lang.String) + ``` +2. Preserve existing task enablement (they are disabled on purpose). +3. Verify: + + ```bash + ./gradlew forbiddenApis -PskipTests + ``` + +Escape hatch already in the codebase: `@SuppressForbidden`. Do not invent a second one. + +## Custom Gradle check — path-scoped or structural + +Existing pattern: [`ConfigInversionLinter`](../../../../buildSrc/src/main/kotlin/datadog/gradle/plugin/config/ConfigInversionLinter.kt), +[`InstrumentationNamingPlugin`](../../../../buildSrc/src/main/kotlin/datadog/gradle/plugin/naming/InstrumentationNamingPlugin.kt). +JavaParser is already a `buildSrc` dependency. + +Use when the rule needs source structure or a path filter SpotBugs cannot name, for example: + +- only inside `@Advice.OnMethodEnter` / `@Advice.OnMethodExit` +- only under bootstrap / premain +- module-directory or config-inventory conventions + +Do **not** add the rule to `ConfigInversionLinter` unless it is about supported configuration. Register a dedicated verification task and a GitLab job (or attach it to an existing `check*` aggregator) so it is a merge gate. + +Verify with the new task, then `./gradlew spotlessApply` on the Gradle/Kotlin files you touched. + +## Custom SpotBugs detector + +Do not write one unless the user explicitly asks **and** ForbiddenAPIs plus a Gradle source check are both a poor fit. Custom detectors are bytecode visitors with `findbugs.xml` / `messages.xml` plugin wiring; they are a poor default agent output. + +## Do not use + +| Vehicle | Why | +|---|---| +| Spotless | Formatter, not a review-theme lint | +| CodeNarc | Groovy-only; new Groovy is already blocked | +| ErrorProne / NullAway | Wired only in `:dd-java-agent:agent-iast` | +| `static-analysis.datadog.yml` | No in-repo CI job; not a required check | +| CodeQL / Trivy | Security scanners; not PR merge gates | +| One-off GitHub Action scripts | Fine for PR metadata, not for source conventions | diff --git a/.claude/skills/spotbugs b/.claude/skills/spotbugs new file mode 120000 index 00000000000..e681b4890a7 --- /dev/null +++ b/.claude/skills/spotbugs @@ -0,0 +1 @@ +../../.agents/skills/spotbugs \ No newline at end of file