Skip to content
84 changes: 84 additions & 0 deletions .agents/skills/spotbugs/SKILL.md
Original file line number Diff line number Diff line change
@@ -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
Comment thread
colin-higgins marked this conversation as resolved.
Comment thread
colin-higgins marked this conversation as resolved.
```

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 = "…")
```
69 changes: 69 additions & 0 deletions .agents/skills/spotbugs/references/other-vehicles.md
Original file line number Diff line number Diff line change
@@ -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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Account for non-blocking Forbidden APIs modules

For repository-wide banned-call rules, Forbidden APIs is not currently a blocking gate in every production module: dd-java-agent/agent-otel/otel-bootstrap/build.gradle overrides forbiddenApis.ignoreFailures to true. A violation confined to that module therefore does not fail CI, contrary to the selection criterion here; either document the coverage exception, remove the override as part of enabling the rule, or choose a vehicle that can enforce it there.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This is true, and this is intentional, on some specific modules only, currently only 3 modules disable it.
It might be worth mentioning it though.


## 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”).

@bric3 bric3 Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: We may want to we mention the extra signature lists for specific module types?

  • main.txt holds the common rules,
  • while the instrumentation convention also adds instrumentation.txt.

This helps the skill choose the right file for a new ban while keeping tool enablement in the module configuration.

forbiddenApis {
signaturesFiles = files(
"$rootDir/gradle/forbiddenApiFilters/main.txt",
"$rootDir/gradle/forbiddenApiFilters/instrumentation.txt"
)
}


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 |
1 change: 1 addition & 0 deletions .claude/skills/spotbugs
Loading