Repository navigation
Basis for java agent rule skill #12816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
d27fba9
00be7e0
de30dc6
eecf082
810bc62
b554899
3153330
5234b16
5996536
51e2193
b619afb
90b4c8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
|
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 = "…") | ||
| ``` | ||
| 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. | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For repository-wide banned-call rules, Forbidden APIs is not currently a blocking gate in every production module: Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||
|
|
||||||||||||||
| ## 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”). | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
This helps the skill choose the right file for a new ban while keeping tool enablement in the module configuration. dd-trace-java/buildSrc/src/main/kotlin/dd-trace-java.module.instrumentation.gradle.kts Lines 28 to 33 in d27fba9
|
||||||||||||||
|
|
||||||||||||||
| 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 | | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ../../.agents/skills/spotbugs |
Uh oh!
There was an error while loading. Please reload this page.