From 701b39c1dbbce8b480777b19c630735897137e49 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 16:38:14 -0500 Subject: [PATCH 01/10] feat: ban Pattern.compile() in forbidden APIs for hot-path performance --- gradle/forbiddenApiFilters/main.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gradle/forbiddenApiFilters/main.txt b/gradle/forbiddenApiFilters/main.txt index 334e0d79822..72e9bf10410 100644 --- a/gradle/forbiddenApiFilters/main.txt +++ b/gradle/forbiddenApiFilters/main.txt @@ -7,6 +7,10 @@ java.lang.String#split(java.lang.String,int) java.lang.String#replaceAll(java.lang.String,java.lang.String) java.lang.String#replaceFirst(java.lang.String,java.lang.String) +@defaultMessage Use String.indexOf/substring/contains for performance. Regex creates objects on every call. +java.util.regex.Pattern#compile(java.lang.String) +java.util.regex.Pattern#compile(java.lang.String, int) + # can initialize java.util.logging when ACCP is installed, prefer RandomUtils instead java.util.UUID#randomUUID() From 1973e62c6fdd03a8da90a2ce92c7b90eec6add53 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 16:40:14 -0500 Subject: [PATCH 02/10] feat: add empty instrumentation stub detector Gradle plugin --- buildSrc/build.gradle.kts | 5 + .../plugin/lint/EmptyInstrumentationLinter.kt | 92 +++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/EmptyInstrumentationLinter.kt diff --git a/buildSrc/build.gradle.kts b/buildSrc/build.gradle.kts index ad06adecc27..ec939f6634e 100644 --- a/buildSrc/build.gradle.kts +++ b/buildSrc/build.gradle.kts @@ -59,6 +59,11 @@ gradlePlugin { id = "dd-trace-java.instrumentation-naming" implementationClass = "datadog.gradle.plugin.naming.InstrumentationNamingPlugin" } + + create("empty-instrumentation-linter") { + id = "dd-trace-java.empty-instrumentation-linter" + implementationClass = "datadog.gradle.plugin.lint.EmptyInstrumentationLinter" + } } } diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/EmptyInstrumentationLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/EmptyInstrumentationLinter.kt new file mode 100644 index 00000000000..48db7a9e240 --- /dev/null +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/EmptyInstrumentationLinter.kt @@ -0,0 +1,92 @@ +package datadog.gradle.plugin.lint + +import org.gradle.api.GradleException +import org.gradle.api.Plugin +import org.gradle.api.Project + +class EmptyInstrumentationLinter : Plugin { + override fun apply(target: Project) { + target.tasks.register("checkEmptyInstrumentations") { + group = "verification" + description = "Detects empty instrumentation stub classes with no transform() calls in methodAdvice()" + + doLast { + val instrumentationsDir = target.rootProject.file("dd-java-agent/instrumentation") + + if (!instrumentationsDir.exists() || !instrumentationsDir.isDirectory) { + throw GradleException( + "Instrumentations directory not found: ${instrumentationsDir.absolutePath}" + ) + } + + val violations = mutableListOf() + val hasAdvicePattern = Regex("""implements\s+[^{]*\b(?:HasMethodAdvice|HasAdvice)\b""") + val methodAdviceStartPattern = Regex("""(?:public\s+)?(?:\S+\s+)?methodAdvice\s*\(""") + val transformCallPattern = Regex("""\btransform\s*\(""") + + instrumentationsDir.walk() + .filter { it.isFile && it.name.endsWith(".java") } + .forEach { file -> + val lines = file.readLines() + val content = lines.joinToString("\n") + + if (!hasAdvicePattern.containsMatchIn(content)) return@forEach + + // Find methodAdvice( method and check its body for transform( calls + var methodAdviceLineIndex = -1 + for (i in lines.indices) { + if (methodAdviceStartPattern.containsMatchIn(lines[i])) { + methodAdviceLineIndex = i + break + } + } + + if (methodAdviceLineIndex < 0) return@forEach + + // Extract method body by tracking brace depth + var braceDepth = 0 + var methodBodyStart = -1 + val methodBodyLines = mutableListOf() + + for (i in methodAdviceLineIndex until lines.size) { + val line = lines[i] + for (ch in line) { + when (ch) { + '{' -> { + braceDepth++ + if (braceDepth == 1) methodBodyStart = i + } + '}' -> { + braceDepth-- + if (braceDepth == 0 && methodBodyStart >= 0) { + methodBodyLines.add(line) + break + } + } + } + } + if (methodBodyStart >= 0) { + methodBodyLines.add(line) + } + if (braceDepth == 0 && methodBodyStart >= 0) break + } + + val methodBody = methodBodyLines.joinToString("\n") + if (!transformCallPattern.containsMatchIn(methodBody)) { + val classNamePattern = Regex("""class\s+(\w+)""") + val className = classNamePattern.find(content)?.groupValues?.get(1) ?: "" + val relPath = file.relativeTo(target.rootProject.projectDir).path + violations.add("EMPTY STUB: $relPath:${methodAdviceLineIndex + 1} — $className.methodAdvice() contains no transform() calls") + } + } + + if (violations.isNotEmpty()) { + violations.forEach { target.logger.error(it) } + throw GradleException("Empty instrumentation stubs found! See errors above.") + } else { + target.logger.lifecycle("✓ No empty instrumentation stubs found") + } + } + } + } +} From 0c80ea610e47f2b91569192bdfea54e7d7064578 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 16:41:51 -0500 Subject: [PATCH 03/10] feat: add typos-cli CI check via GitHub Actions --- .github/workflows/typos.yaml | 15 +++++++++++++++ .typos.toml | 31 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 .github/workflows/typos.yaml create mode 100644 .typos.toml diff --git a/.github/workflows/typos.yaml b/.github/workflows/typos.yaml new file mode 100644 index 00000000000..63e91d8c2a3 --- /dev/null +++ b/.github/workflows/typos.yaml @@ -0,0 +1,15 @@ +name: Typos Check +on: + pull_request: + branches: + - master +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true +jobs: + typos: + name: Typos Check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: crate-ci/typos@v1 diff --git a/.typos.toml b/.typos.toml new file mode 100644 index 00000000000..a80ded12794 --- /dev/null +++ b/.typos.toml @@ -0,0 +1,31 @@ +[files] +extend-exclude = [ + "build/", + "workspace/", + ".gradle/", + "gradle.lockfile", + "settings-gradle.lockfile", + "LICENSE-3rdparty.csv", + "*.jar", + "*.class", + "*.so", + "*.dylib", +] +extend-include = [ + "*.java", + "*.kt", + "*.groovy", + "*.md", + "*.yaml", + "*.yml", + "*.gradle", + "*.gradle.kts", +] + +[default.extend-words] +ot = "ot" +ba = "ba" +nd = "nd" +crate = "crate" +hashi = "hashi" +pullrequest = "pullrequest" From 2cd82208979295023f13fde655711a5376b0d4c9 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 16:42:47 -0500 Subject: [PATCH 04/10] feat: add unnecessary else after return linter Gradle plugin --- buildSrc/build.gradle.kts | 5 + .../plugin/lint/UnnecessaryElseLinter.kt | 111 ++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/UnnecessaryElseLinter.kt diff --git a/buildSrc/build.gradle.kts b/buildSrc/build.gradle.kts index ad06adecc27..b66af83fdf9 100644 --- a/buildSrc/build.gradle.kts +++ b/buildSrc/build.gradle.kts @@ -59,6 +59,11 @@ gradlePlugin { id = "dd-trace-java.instrumentation-naming" implementationClass = "datadog.gradle.plugin.naming.InstrumentationNamingPlugin" } + + create("unnecessary-else-linter") { + id = "dd-trace-java.unnecessary-else-linter" + implementationClass = "datadog.gradle.plugin.lint.UnnecessaryElseLinter" + } } } diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/UnnecessaryElseLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/UnnecessaryElseLinter.kt new file mode 100644 index 00000000000..6c6584cb2c6 --- /dev/null +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/UnnecessaryElseLinter.kt @@ -0,0 +1,111 @@ +package datadog.gradle.plugin.lint + +import org.gradle.api.Plugin +import org.gradle.api.Project + +class UnnecessaryElseLinter : Plugin { + override fun apply(target: Project) { + target.tasks.register("checkUnnecessaryElse") { + group = "verification" + description = "Scan changed Java files for unnecessary else blocks after return/throw/continue/break" + + doLast { + val baseSha = "9b933669729ea8a7af00f5cf3c36b6720ec433bd" + val changedFiles = getChangedJavaFiles(target, baseSha) + val repoRoot = target.rootProject.projectDir.toPath() + val warnings = mutableListOf() + + changedFiles.forEach { file -> + if (file.exists()) { + val relPath = repoRoot.relativize(file.toPath()).toString() + checkFile(file, relPath, warnings) + } + } + + if (warnings.isNotEmpty()) { + warnings.forEach { target.logger.warn(it) } + target.logger.warn("Found ${warnings.size} unnecessary else block(s) — advisory only.") + } else { + target.logger.info("No unnecessary else blocks found in changed files.") + } + } + } + } +} + +private fun getChangedJavaFiles(project: Project, baseSha: String): List { + return try { + val process = ProcessBuilder("git", "diff", "--name-only", baseSha, "HEAD") + .directory(project.rootProject.projectDir) + .redirectErrorStream(true) + .start() + val output = process.inputStream.bufferedReader().readText() + process.waitFor() + output.trim().lines() + .filter { it.isNotBlank() && it.endsWith(".java") } + .map { project.rootProject.projectDir.resolve(it) } + .filter { it.exists() } + } catch (e: Exception) { + project.logger.warn("checkUnnecessaryElse: could not get changed files — ${e.message}") + emptyList() + } +} + +private fun checkFile(file: java.io.File, relPath: String, warnings: MutableList) { + val lines = file.readLines() + val elsePattern = Regex("""^\s*\}\s*else\s*(\{.*)?$""") + + for (i in lines.indices) { + if (!elsePattern.containsMatchIn(lines[i])) continue + if (isUnnecessaryElse(lines, i)) { + warnings.add("STYLE: $relPath:${i + 1} — unnecessary else after return/throw/continue/break") + } + } +} + +private fun isUnnecessaryElse(lines: List, elseLineIdx: Int): Boolean { + // Walk backwards from "} else {" to find the last meaningful statement + var j = elseLineIdx - 1 + while (j >= 0) { + val trimmed = lines[j].trim() + if (trimmed.isEmpty() || isCommentLine(trimmed)) { + j-- + continue + } + // The last non-empty line before "} else {" must end with ";" to be a statement boundary + if (!trimmed.endsWith(";")) return false + + // Check if this single line is itself an exit statement + if (isExitStatement(trimmed)) return true + + // For multi-line statements (e.g. multi-line return/throw), walk further back + // looking for the start of the statement (a line not ending with a continuation) + var k = j - 1 + while (k >= 0) { + val prev = lines[k].trim() + if (prev.isEmpty() || isCommentLine(prev)) { + k-- + continue + } + // Another complete statement or block boundary — stop + if (prev.endsWith(";") || prev.endsWith("{") || prev.endsWith("}")) return false + // This line is a continuation of the same statement + if (startsWithExitKeyword(prev)) return true + k-- + } + return false + } + return false +} + +private fun isCommentLine(trimmed: String) = + trimmed.startsWith("//") || trimmed.startsWith("*") || trimmed.startsWith("/*") + +private fun isExitStatement(trimmed: String) = + startsWithExitKeyword(trimmed) + +private fun startsWithExitKeyword(trimmed: String) = + trimmed.startsWith("return ") || trimmed == "return;" || + trimmed.startsWith("throw ") || + trimmed == "continue;" || trimmed.startsWith("continue ") || + trimmed == "break;" || trimmed.startsWith("break ") From fa6947094134b250512c682f6f102e0dc917f8e0 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 16:42:49 -0500 Subject: [PATCH 05/10] feat: add CI guard for temporary debug flags --- scripts/check-ci-debug-flags.sh | 152 ++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100755 scripts/check-ci-debug-flags.sh diff --git a/scripts/check-ci-debug-flags.sh b/scripts/check-ci-debug-flags.sh new file mode 100755 index 00000000000..41250893cde --- /dev/null +++ b/scripts/check-ci-debug-flags.sh @@ -0,0 +1,152 @@ +#!/usr/bin/env bash +set -euo pipefail + +DRY_RUN=false +VIOLATIONS=0 + +usage() { + cat <:: +EOF +} + +for arg in "$@"; do + case "$arg" in + --dry-run) DRY_RUN=true ;; + --help) usage; exit 0 ;; + *) echo "Unknown argument: $arg" >&2; usage >&2; exit 1 ;; + esac +done + +if $DRY_RUN; then + DIFF=$(git diff HEAD) +else + DIFF=$(git diff --cached) +fi + +if [[ -z "$DIFF" ]]; then + exit 0 +fi + +# Parse unified diff, tracking current file and line numbers +current_file="" +current_line=0 + +warn() { + local file="$1" + local line="$2" + local msg="$3" + echo "WARNING: $file:$line: $msg" + VIOLATIONS=$((VIOLATIONS + 1)) +} + +# Process diff line by line +while IFS= read -r line; do + # Track current file from diff header + if [[ "$line" =~ ^\+\+\+\ b/(.+)$ ]]; then + current_file="${BASH_REMATCH[1]}" + current_line=0 + continue + fi + + # Track line numbers from hunk headers + if [[ "$line" =~ ^@@\ -[0-9]+(,[0-9]+)?\ \+([0-9]+)(,[0-9]+)?\ @@.* ]]; then + current_line="${BASH_REMATCH[2]}" + # Subtract 1 because we'll increment before checking added lines + current_line=$((current_line - 1)) + continue + fi + + # Skip removed lines and diff metadata + if [[ "$line" =~ ^- ]] || [[ "$line" =~ ^(diff|index|---) ]]; then + continue + fi + + # Count context and added lines + if [[ "$line" =~ ^(\+| ) ]]; then + current_line=$((current_line + 1)) + fi + + # Only check added lines (starting with +) + if [[ ! "$line" =~ ^\+ ]]; then + continue + fi + + content="${line:1}" # Strip leading + + + [[ -z "$current_file" ]] && continue + + # Pattern 1: .gitlab-ci.yml and .gitlab/**/*.yml + if [[ "$current_file" == ".gitlab-ci.yml" || "$current_file" == .gitlab/*.yml || "$current_file" == .gitlab/**/*.yml ]]; then + # NON_DEFAULT_JVMS set to true + if echo "$content" | grep -qE 'NON_DEFAULT_JVMS\s*:\s*"?true"?'; then + warn "$current_file" "$current_line" "NON_DEFAULT_JVMS set to true (debug/temporary CI flag)" + fi + # Hardcoded branch names in if: or only: conditions (not master, main, release/*) + if echo "$content" | grep -qE '^\s*(if|only)\s*:' ; then + if echo "$content" | grep -qE '(if|only)\s*:.*\$CI_COMMIT_BRANCH\s*==\s*"[^"]*"' || \ + echo "$content" | grep -qE "(if|only)\s*:.*refs/heads/[^\s]*"; then + # Check it's not master/main/release + if ! echo "$content" | grep -qE '(master|main|release/)'; then + warn "$current_file" "$current_line" "Hardcoded branch name in CI condition (not master/main/release/*)" + fi + fi + fi + # Also check for hardcoded branch refs in only/if without $CI_COMMIT_BRANCH syntax + if echo "$content" | grep -qE '^\s*-\s*[a-zA-Z0-9_/.-]+$'; then + branch_val=$(echo "$content" | grep -oE '[a-zA-Z0-9_/.-]+$') + if [[ -n "$branch_val" ]] && ! echo "$branch_val" | grep -qE '^(master|main|release/.*)$'; then + # This is too broad, skip + : + fi + fi + fi + + # Pattern 2: .github/workflows/*.yaml + if [[ "$current_file" == .github/workflows/*.yaml || "$current_file" == .github/workflows/*.yml ]]; then + if echo "$content" | grep -qE '^\s*if\s*:'; then + # Check for hardcoded branch names that are not master/main/release + if echo "$content" | grep -qE "github\.ref\s*==\s*'refs/heads/[^']*'|github\.ref_name\s*==\s*'[^']*'"; then + if ! echo "$content" | grep -qE "(master|main|release/)"; then + warn "$current_file" "$current_line" "Hardcoded branch name in GitHub Actions if condition (not master/main/release/*)" + fi + fi + fi + fi + + # Pattern 3: **/*.groovy and **/*.java test files + if [[ "$current_file" == *.groovy || "$current_file" == *.java ]]; then + if echo "$content" | grep -qF -- '-Ddd.trace.debug=true'; then + warn "$current_file" "$current_line" "-Ddd.trace.debug=true found in test configuration (debug flag)" + fi + fi + + # Pattern 4: **/*.gradle and **/*.gradle.kts + if [[ "$current_file" == *.gradle || "$current_file" == *.gradle.kts ]]; then + # Commented-out apply plugin: or id( lines + if echo "$content" | grep -qE '^\s*//\s*(apply plugin:|id\()'; then + warn "$current_file" "$current_line" "Commented-out plugin declaration (suggests temporary disable)" + fi + fi + +done <<< "$DIFF" + +if [[ $VIOLATIONS -gt 0 ]]; then + echo "" + echo "Found $VIOLATIONS debug flag violation(s)." + if ! $DRY_RUN; then + exit 1 + fi +fi + +exit 0 From 6187b8880ecf50e0a9059e61449f5c20fde32c01 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 16:44:03 -0500 Subject: [PATCH 06/10] feat: add extraneous file blocker script --- scripts/check-extraneous-files.sh | 81 +++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100755 scripts/check-extraneous-files.sh diff --git a/scripts/check-extraneous-files.sh b/scripts/check-extraneous-files.sh new file mode 100755 index 00000000000..9393ce3a777 --- /dev/null +++ b/scripts/check-extraneous-files.sh @@ -0,0 +1,81 @@ +#!/usr/bin/env bash +set -euo pipefail + +BASE_BRANCH="master" + +usage() { + echo "Usage: $(basename "$0") [--base ] [--help]" + echo "" + echo "Checks for extraneous files that should not be committed to the repository." + echo "Compares new files added against the base branch." + echo "" + echo "Options:" + echo " --base Base branch to compare against (default: master)" + echo " --help Print this usage message" + echo "" + echo "Blocked patterns:" + echo " *_REPORT.md or *_REFERENCE.md at any level (AI-generated reports)" + echo " Top-level *.sh scripts not in scripts/, gradle/, or .gitlab/ directories" + echo " Files named QUICK_REFERENCE* or CHEATSHEET*" + echo " run-*.sh scripts at any level (ad-hoc test runners)" + exit 0 +} + +while [[ $# -gt 0 ]]; do + case "$1" in + --base) + BASE_BRANCH="$2" + shift 2 + ;; + --help) + usage + ;; + *) + echo "Unknown option: $1" >&2 + usage + ;; + esac +done + +violations=0 + +while IFS= read -r file; do + reason="" + + # Pattern 1: *_REPORT.md or *_REFERENCE.md at any level + basename_file="$(basename "$file")" + if [[ "$basename_file" == *_REPORT.md || "$basename_file" == *_REFERENCE.md ]]; then + reason="AI-generated report/reference file" + fi + + # Pattern 2: Top-level *.sh scripts not in scripts/, gradle/, or .gitlab/ + if [[ -z "$reason" && "$file" == *.sh ]]; then + dir="$(dirname "$file")" + if [[ "$dir" == "." ]]; then + reason="Top-level shell script (move to scripts/, gradle/, or .gitlab/)" + fi + fi + + # Pattern 3: Files named QUICK_REFERENCE* or CHEATSHEET* + if [[ -z "$reason" ]]; then + if [[ "$basename_file" == QUICK_REFERENCE* || "$basename_file" == CHEATSHEET* ]]; then + reason="Quick reference or cheatsheet file" + fi + fi + + # Pattern 4: run-*.sh scripts at any level + if [[ -z "$reason" && "$basename_file" == run-*.sh ]]; then + reason="Ad-hoc test runner script (run-*.sh)" + fi + + if [[ -n "$reason" ]]; then + echo "BLOCKED: $file — $reason" + violations=$((violations + 1)) + fi +done < <(git diff --name-only --diff-filter=A "${BASE_BRANCH}...HEAD" 2>/dev/null || git diff --name-only --diff-filter=A "${BASE_BRANCH}" 2>/dev/null) + +if [[ $violations -gt 0 ]]; then + exit 1 +fi + +exit 0 From be96cce96cddfa3fd1b293126c52064f7eb9a1ed Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 16:44:27 -0500 Subject: [PATCH 07/10] feat: add camelCase naming convention linter Gradle plugin --- buildSrc/build.gradle.kts | 5 + .../plugin/lint/NamingConventionLinter.kt | 121 ++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/NamingConventionLinter.kt diff --git a/buildSrc/build.gradle.kts b/buildSrc/build.gradle.kts index ad06adecc27..8a45c682f38 100644 --- a/buildSrc/build.gradle.kts +++ b/buildSrc/build.gradle.kts @@ -59,6 +59,11 @@ gradlePlugin { id = "dd-trace-java.instrumentation-naming" implementationClass = "datadog.gradle.plugin.naming.InstrumentationNamingPlugin" } + + create("naming-convention-linter") { + id = "dd-trace-java.naming-convention-linter" + implementationClass = "datadog.gradle.plugin.lint.NamingConventionLinter" + } } } diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/NamingConventionLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/NamingConventionLinter.kt new file mode 100644 index 00000000000..e3b36a5732a --- /dev/null +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/NamingConventionLinter.kt @@ -0,0 +1,121 @@ +package datadog.gradle.plugin.lint + +import org.gradle.api.Plugin +import org.gradle.api.Project + +class NamingConventionLinter : Plugin { + override fun apply(target: Project) { + target.tasks.register("checkNamingConventions") { + group = "verification" + description = "Checks Java files changed vs. base branch for snake_case method/variable names" + + doLast { + val repoRoot = target.rootProject.projectDir + + // Get changed .java files via git diff against base branch + val gitOutput = try { + val process = ProcessBuilder("git", "diff", "--name-only", "--diff-filter=ACM", "origin/master...HEAD") + .directory(repoRoot) + .redirectErrorStream(true) + .start() + process.inputStream.bufferedReader().readText().trim() + } catch (e: Exception) { + target.logger.warn("checkNamingConventions: could not run git diff — skipping. ${e.message}") + return@doLast + } + + if (gitOutput.isBlank()) { + target.logger.lifecycle("checkNamingConventions: no changed files found.") + return@doLast + } + + val changedJavaFiles = gitOutput.lines() + .filter { it.endsWith(".java") } + .map { repoRoot.resolve(it) } + .filter { it.exists() } + + if (changedJavaFiles.isEmpty()) { + target.logger.lifecycle("checkNamingConventions: no changed .java files.") + return@doLast + } + + // Matches method-like declarations with underscores in the name + // e.g. "void my_method(" or "public String some_name(" + val methodWithUnderscore = Regex( + """(?:public|private|protected|static|void|int|long|boolean|String|\w+)\s+(\w*_\w+)\s*\(""" + ) + + // Matches local variable declarations with underscores + // e.g. "int my_var = " or "final String some_name;" + val localVarWithUnderscore = Regex( + """(?:int|long|String|boolean|var|final)\s+(\w*_\w+)\s*[=;]""" + ) + + // All-uppercase constant pattern: words with underscores, all caps + val constantPattern = Regex("""^[A-Z][A-Z0-9_]*$""") + + val warnings = mutableListOf() + + for (file in changedJavaFiles) { + val relativePath = repoRoot.toPath().relativize(file.toPath()).toString() + val isTestFile = relativePath.contains("/src/test/") || relativePath.contains("\\src\\test\\") + val isMainFile = relativePath.contains("/src/main/") || relativePath.contains("\\src\\main\\") + + val lines = file.readLines() + var pendingTestAnnotation = false + + for ((idx, line) in lines.withIndex()) { + val trimmed = line.trim() + val lineNumber = idx + 1 + + // Track @Test annotations + if (trimmed == "@Test" || trimmed.startsWith("@Test(")) { + pendingTestAnnotation = true + continue + } + // Reset annotation flag on non-annotation, non-empty lines + if (trimmed.isNotEmpty() && !trimmed.startsWith("@") && !trimmed.startsWith("//")) { + val hadTestAnnotation = pendingTestAnnotation + pendingTestAnnotation = false + + // Check method names (applies to all files except when @Test annotated or in test dir) + if (!isTestFile && !hadTestAnnotation) { + methodWithUnderscore.find(line)?.let { match -> + val name = match.groupValues[1] + if (!constantPattern.matches(name) && !isNativeMethod(line)) { + warnings.add("NAMING: $relativePath:$lineNumber — snake_case identifier '$name' should be camelCase") + } + } + } + + // Check local variables (only in src/main/ files) + if (isMainFile) { + localVarWithUnderscore.find(line)?.let { match -> + val name = match.groupValues[1] + if (!constantPattern.matches(name)) { + warnings.add("NAMING: $relativePath:$lineNumber — snake_case identifier '$name' should be camelCase") + } + } + } + } else if (trimmed.startsWith("@")) { + // Keep the annotation flag active across annotation lines + } else { + pendingTestAnnotation = false + } + } + } + + if (warnings.isNotEmpty()) { + target.logger.warn("checkNamingConventions: found ${warnings.size} naming convention warning(s):") + warnings.forEach { target.logger.warn(it) } + } else { + target.logger.lifecycle("checkNamingConventions: no naming convention issues found.") + } + } + } + } + + private fun isNativeMethod(line: String): Boolean { + return line.contains("native ") + } +} From 7fdad44282a8e6cf8c0669449fd3725dfd911a2d Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 17:47:42 -0500 Subject: [PATCH 08/10] =?UTF-8?q?feat:=20add=20Wave=202=20static=20checks?= =?UTF-8?q?=20=E2=80=94=20Javadoc=20linter,=20CPD,=20ArchUnit,=20AssertJ?= =?UTF-8?q?=20preference,=20boxed=20primitives?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Task 009: Ban boxed primitive valueOf() in forbidden APIs - Task 010: JavadocLinter Gradle plugin (empty @return/@param detection) - Task 011: CopyPasteDetectorPlugin (hash-based duplicate method detection) - Task 012: ArchUnit architecture tests (bootstrap/core/instrumentation boundaries) - Task 013: AssertJPreferenceLinter (flag JUnit assertions in new test files) Co-Authored-By: Claude Opus 4.6 --- buildSrc/build.gradle.kts | 15 ++++ .../plugin/lint/AssertJPreferenceLinter.kt | 60 +++++++++++++ .../plugin/lint/CopyPasteDetectorPlugin.kt | 88 +++++++++++++++++++ .../gradle/plugin/lint/JavadocLinter.kt | 72 +++++++++++++++ dd-java-agent/build.gradle | 1 + .../datadog/trace/agent/ArchitectureTest.java | 88 +++++++++++++++++++ gradle/forbiddenApiFilters/main.txt | 8 ++ 7 files changed, 332 insertions(+) create mode 100644 buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/AssertJPreferenceLinter.kt create mode 100644 buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/CopyPasteDetectorPlugin.kt create mode 100644 buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/JavadocLinter.kt create mode 100644 dd-java-agent/src/test/java/datadog/trace/agent/ArchitectureTest.java diff --git a/buildSrc/build.gradle.kts b/buildSrc/build.gradle.kts index 450d5443b4c..979b1b74200 100644 --- a/buildSrc/build.gradle.kts +++ b/buildSrc/build.gradle.kts @@ -74,6 +74,21 @@ gradlePlugin { id = "dd-trace-java.naming-convention-linter" implementationClass = "datadog.gradle.plugin.lint.NamingConventionLinter" } + + create("javadoc-linter") { + id = "dd-trace-java.javadoc-linter" + implementationClass = "datadog.gradle.plugin.lint.JavadocLinter" + } + + create("copy-paste-detector") { + id = "dd-trace-java.copy-paste-detector" + implementationClass = "datadog.gradle.plugin.lint.CopyPasteDetectorPlugin" + } + + create("assertj-preference-linter") { + id = "dd-trace-java.assertj-preference-linter" + implementationClass = "datadog.gradle.plugin.lint.AssertJPreferenceLinter" + } } } diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/AssertJPreferenceLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/AssertJPreferenceLinter.kt new file mode 100644 index 00000000000..258c8a5905d --- /dev/null +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/AssertJPreferenceLinter.kt @@ -0,0 +1,60 @@ +package datadog.gradle.plugin.lint + +import org.gradle.api.Plugin +import org.gradle.api.Project +import java.io.ByteArrayOutputStream + +class AssertJPreferenceLinter : Plugin { + override fun apply(target: Project) { + target.tasks.register("checkAssertJPreference") { + group = "verification" + description = "Flags JUnit assertion imports in new test files — prefer AssertJ" + + doLast { + val newTestFiles = getNewTestFiles(target) + if (newTestFiles.isEmpty()) { + target.logger.lifecycle("checkAssertJPreference: No new test files to check") + return@doLast + } + + val warnings = mutableListOf() + val junitAssertImport = Regex("""import\s+(?:static\s+)?org\.junit\.jupiter\.api\.Assertions""") + + newTestFiles.forEach { file -> + if (!file.exists()) return@forEach + val lines = file.readLines() + val relPath = file.relativeTo(target.rootProject.projectDir).path + + for (i in lines.indices) { + if (junitAssertImport.containsMatchIn(lines[i])) { + warnings.add("STYLE: $relPath:${i + 1} — Prefer AssertJ (org.assertj.core.api.Assertions) over JUnit assertions for richer API") + } + } + } + + if (warnings.isNotEmpty()) { + warnings.forEach { target.logger.warn(it) } + target.logger.warn("checkAssertJPreference: ${warnings.size} file(s) using JUnit assertions instead of AssertJ") + } else { + target.logger.lifecycle("checkAssertJPreference: All new test files use AssertJ") + } + } + } + } + + private fun getNewTestFiles(project: Project): List { + return try { + val stdout = ByteArrayOutputStream() + project.exec { + commandLine("git", "diff", "--name-only", "--diff-filter=A", "HEAD~1") + standardOutput = stdout + isIgnoreExitValue = true + } + stdout.toString().trim().lines() + .filter { it.endsWith(".java") && it.contains("src/test/") } + .map { project.rootProject.file(it) } + } catch (e: Exception) { + emptyList() + } + } +} diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/CopyPasteDetectorPlugin.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/CopyPasteDetectorPlugin.kt new file mode 100644 index 00000000000..bb3bc28e364 --- /dev/null +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/CopyPasteDetectorPlugin.kt @@ -0,0 +1,88 @@ +package datadog.gradle.plugin.lint + +import org.gradle.api.Plugin +import org.gradle.api.Project +import java.io.File + +class CopyPasteDetectorPlugin : Plugin { + override fun apply(target: Project) { + target.tasks.register("checkCodeDuplication") { + group = "verification" + description = "Detect copy-pasted code using hash-based method body comparison" + + doLast { + val dirs = listOf("dd-java-agent/instrumentation", "dd-trace-core", "internal-api") + .map { target.rootProject.file(it) } + .filter { it.exists() } + + val methodHashes = mutableMapOf>() + var totalFiles = 0 + + dirs.forEach { dir -> + dir.walkTopDown() + .filter { it.isFile && it.extension == "java" && !it.path.contains("/build/") && !it.path.contains("/generated/") } + .forEach { file -> + totalFiles++ + extractMethodBodies(file).forEach { (name, body) -> + val normalized = normalizeCode(body) + if (normalized.length > 200) { + val hash = normalized.hashCode() + val location = "${file.relativeTo(target.rootProject.projectDir).path}:$name" + methodHashes.getOrPut(hash) { mutableListOf() }.add(location) + } + } + } + } + + val duplicates = methodHashes.filter { it.value.size > 1 } + if (duplicates.isNotEmpty()) { + target.logger.warn("CPD: Found ${duplicates.size} group(s) of duplicate methods across $totalFiles files:") + duplicates.entries.take(20).forEach { (_, locations) -> + target.logger.warn(" DUPLICATE GROUP (${locations.size} copies):") + locations.forEach { loc -> target.logger.warn(" - $loc") } + } + if (duplicates.size > 20) { + target.logger.warn(" ... and ${duplicates.size - 20} more groups") + } + } else { + target.logger.lifecycle("CPD: No significant duplicates found across $totalFiles files") + } + } + } + } + + private fun extractMethodBodies(file: File): List> { + val content = file.readText() + val results = mutableListOf>() + val methodPattern = Regex("""(?:public|private|protected|static|final|synchronized|\s)+[\w<>\[\],\s]+\s+(\w+)\s*\([^)]*\)[^{]*\{""") + + methodPattern.findAll(content).forEach { match -> + val methodName = match.groupValues[1] + val startIdx = match.range.last + 1 + var braceCount = 1 + var idx = startIdx + while (idx < content.length && braceCount > 0) { + when (content[idx]) { + '{' -> braceCount++ + '}' -> braceCount-- + } + idx++ + } + if (braceCount == 0) { + val body = content.substring(startIdx, idx - 1) + if (body.lines().size >= 5) { + results.add(methodName to body) + } + } + } + return results + } + + private fun normalizeCode(code: String): String { + return code.lines() + .map { it.trim() } + .filter { it.isNotEmpty() && !it.startsWith("//") && !it.startsWith("*") } + .joinToString("\n") + .replace(Regex("""\s+"""), " ") + } +} diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/JavadocLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/JavadocLinter.kt new file mode 100644 index 00000000000..e44da9a2896 --- /dev/null +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/JavadocLinter.kt @@ -0,0 +1,72 @@ +package datadog.gradle.plugin.lint + +import org.gradle.api.Plugin +import org.gradle.api.Project +import java.io.ByteArrayOutputStream + +class JavadocLinter : Plugin { + override fun apply(target: Project) { + target.tasks.register("checkJavadocQuality") { + group = "verification" + description = "Detects empty @return and @param tags in Javadoc comments" + + doLast { + val changedFiles = getChangedJavaFiles(target) + if (changedFiles.isEmpty()) { + target.logger.lifecycle("checkJavadocQuality: No changed Java files to check") + return@doLast + } + + val warnings = mutableListOf() + val emptyReturnPattern = Regex("""@return\s*$""") + val emptyParamPattern = Regex("""@param\s+\w+\s*$""") + + changedFiles.forEach { file -> + if (!file.exists()) return@forEach + val lines = file.readLines() + val relPath = file.relativeTo(target.rootProject.projectDir).path + + for (i in lines.indices) { + val trimmed = lines[i].trim().removePrefix("* ").removePrefix("*") + if (emptyReturnPattern.containsMatchIn(trimmed)) { + // Check next non-empty line isn't a continuation + val nextContent = lines.getOrNull(i + 1)?.trim()?.removePrefix("* ")?.removePrefix("*")?.trim() ?: "" + if (nextContent.isEmpty() || nextContent.startsWith("@") || nextContent.startsWith("*/")) { + warnings.add("JAVADOC: $relPath:${i + 1} — empty @return tag") + } + } + if (emptyParamPattern.containsMatchIn(trimmed)) { + val nextContent = lines.getOrNull(i + 1)?.trim()?.removePrefix("* ")?.removePrefix("*")?.trim() ?: "" + if (nextContent.isEmpty() || nextContent.startsWith("@") || nextContent.startsWith("*/")) { + warnings.add("JAVADOC: $relPath:${i + 1} — empty @param tag") + } + } + } + } + + if (warnings.isNotEmpty()) { + warnings.forEach { target.logger.warn(it) } + target.logger.warn("checkJavadocQuality: ${warnings.size} Javadoc warning(s) found") + } else { + target.logger.lifecycle("checkJavadocQuality: No empty Javadoc tags found") + } + } + } + } + + private fun getChangedJavaFiles(project: Project): List { + return try { + val stdout = ByteArrayOutputStream() + project.exec { + commandLine("git", "diff", "--name-only", "--diff-filter=ACMR", "HEAD~1") + standardOutput = stdout + isIgnoreExitValue = true + } + stdout.toString().trim().lines() + .filter { it.endsWith(".java") } + .map { project.rootProject.file(it) } + } catch (e: Exception) { + emptyList() + } + } +} diff --git a/dd-java-agent/build.gradle b/dd-java-agent/build.gradle index c583bb6f588..ee02f045946 100644 --- a/dd-java-agent/build.gradle +++ b/dd-java-agent/build.gradle @@ -353,6 +353,7 @@ dependencies { testImplementation project(':utils:test-agent-utils:decoder') testImplementation libs.bundles.test.logging + testImplementation 'com.tngtech.archunit:archunit-junit5:1.3.0' testImplementation libs.guava testImplementation libs.okhttp testImplementation group: 'io.opentracing', name: 'opentracing-util', version: '0.31.0' diff --git a/dd-java-agent/src/test/java/datadog/trace/agent/ArchitectureTest.java b/dd-java-agent/src/test/java/datadog/trace/agent/ArchitectureTest.java new file mode 100644 index 00000000000..8325caf66a0 --- /dev/null +++ b/dd-java-agent/src/test/java/datadog/trace/agent/ArchitectureTest.java @@ -0,0 +1,88 @@ +package datadog.trace.agent; + +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses; + +import com.tngtech.archunit.core.domain.JavaClasses; +import com.tngtech.archunit.core.importer.ClassFileImporter; +import com.tngtech.archunit.core.importer.ImportOption; +import com.tngtech.archunit.lang.ArchRule; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** + * Architecture fitness tests enforcing module dependency rules. These rules prevent the most common + * violations found during PR reviews: bootstrap depending on core, instrumentations reaching into + * core internals, and forbidden JDK APIs in bootstrap code. + */ +class ArchitectureTest { + + private static JavaClasses allClasses; + + @BeforeAll + static void importClasses() { + allClasses = + new ClassFileImporter() + .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) + .importPackages("datadog.trace"); + } + + @Test + void bootstrapShouldNotDependOnCore() { + ArchRule rule = + noClasses() + .that() + .resideInAPackage("datadog.trace.bootstrap..") + .should() + .dependOnClassesThat() + .resideInAPackage("datadog.trace.core..") + .because("Bootstrap classes load before core and must not depend on core internals"); + + rule.check(allClasses); + } + + @Test + void instrumentationShouldNotDependOnCoreInternals() { + ArchRule rule = + noClasses() + .that() + .resideInAPackage("datadog.trace.instrumentation..") + .should() + .dependOnClassesThat() + .resideInAPackage("datadog.trace.core.internal..") + .because("Instrumentations should use internal-api, not core internals directly"); + + rule.check(allClasses); + } + + @Test + void bootstrapShouldNotUseJavaUtilLogging() { + ArchRule rule = + noClasses() + .that() + .resideInAPackage("datadog.trace.bootstrap..") + .should() + .dependOnClassesThat() + .resideInAPackage("java.util.logging..") + .because( + "java.util.logging locks in the log manager before the app configures it" + + " (see docs/bootstrap_design_guidelines.md)"); + + rule.check(allClasses); + } + + @Test + void bootstrapShouldNotUseJavaxManagement() { + ArchRule rule = + noClasses() + .that() + .resideInAPackage("datadog.trace.bootstrap..") + .should() + .dependOnClassesThat() + .resideInAPackage("javax.management..") + .because( + "javax.management causes class loading issues in premain" + + " (see docs/bootstrap_design_guidelines.md)"); + + rule.check(allClasses); + } +} diff --git a/gradle/forbiddenApiFilters/main.txt b/gradle/forbiddenApiFilters/main.txt index 72e9bf10410..50e5fc7aeed 100644 --- a/gradle/forbiddenApiFilters/main.txt +++ b/gradle/forbiddenApiFilters/main.txt @@ -54,3 +54,11 @@ java.lang.reflect.Field#setLong(java.lang.Object,long) java.lang.reflect.Field#setFloat(java.lang.Object,float) java.lang.reflect.Field#setDouble(java.lang.Object,double) java.lang.invoke.MethodHandles.Lookup#unreflectSetter(java.lang.reflect.Field) + +# avoid unnecessary boxing of primitives — use primitive types directly +@defaultMessage Avoid unnecessary boxing. Use primitive types (long, int, double) instead of boxed types when possible. +java.lang.Long#valueOf(long) +java.lang.Integer#valueOf(int) +java.lang.Double#valueOf(double) +java.lang.Float#valueOf(float) +java.lang.Boolean#valueOf(boolean) From e142783c08722885292bf59c0ca0df63299450d9 Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 17:50:24 -0500 Subject: [PATCH 09/10] feat: wire all static check plugins into build.gradle.kts and CI pipeline - Apply 6 new lint plugins in root build.gradle.kts - Add check_pr_hygiene CI job for shell-based PR checks - All new checks are advisory (warnings only) except checkEmptyInstrumentations Co-Authored-By: Claude Opus 4.6 --- .gitlab-ci.yml | 11 +++++++++++ build.gradle.kts | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 7de33f10177..2f065f368c9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -468,6 +468,17 @@ check_build_src: variables: GRADLE_TARGET: ":buildSrc:build" +check_pr_hygiene: + extends: .gradle_build + needs: [] + stage: tests + script: + - bash scripts/check-ci-debug-flags.sh || true + - bash scripts/check-extraneous-files.sh || true + rules: + - if: $CI_PIPELINE_SOURCE == "merge_request_event" + - if: $CI_COMMIT_BRANCH && $CI_COMMIT_BRANCH != "master" + check_base: extends: .check_job variables: diff --git a/build.gradle.kts b/build.gradle.kts index bb6371c42a5..0d70567bac6 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -9,6 +9,12 @@ plugins { id("dd-trace-java.tracer-version") id("dd-trace-java.dump-hanged-test") id("dd-trace-java.config-inversion-linter") + id("dd-trace-java.empty-instrumentation-linter") + id("dd-trace-java.unnecessary-else-linter") + id("dd-trace-java.naming-convention-linter") + id("dd-trace-java.javadoc-linter") + id("dd-trace-java.copy-paste-detector") + id("dd-trace-java.assertj-preference-linter") id("dd-trace-java.ci-jobs") id("com.diffplug.spotless") version "8.2.1" From f65a67cfd5bd7134ee98f2c16d859a05793eb63d Mon Sep 17 00:00:00 2001 From: bm1549 Date: Wed, 4 Mar 2026 18:01:27 -0500 Subject: [PATCH 10/10] =?UTF-8?q?fix:=20address=20code=20review=20findings?= =?UTF-8?q?=20=E2=80=94=20hardcoded=20SHA,=20incremental=20checks,=20Freez?= =?UTF-8?q?ingArchRule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - UnnecessaryElseLinter: replace hardcoded SHA with origin/master...HEAD - JavadocLinter, AssertJPreferenceLinter: use origin/master...HEAD instead of HEAD~1 - EmptyInstrumentationLinter: make incremental (only check changed files) - ArchitectureTest: wrap rules with FreezingArchRule to baseline existing violations - EmptyInstrumentationLinter: fix duplicate-line bug in brace tracking Co-Authored-By: Claude Opus 4.6 --- .../plugin/lint/AssertJPreferenceLinter.kt | 2 +- .../plugin/lint/EmptyInstrumentationLinter.kt | 116 +++++++++++------- .../gradle/plugin/lint/JavadocLinter.kt | 2 +- .../plugin/lint/UnnecessaryElseLinter.kt | 7 +- .../datadog/trace/agent/ArchitectureTest.java | 11 +- 5 files changed, 83 insertions(+), 55 deletions(-) diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/AssertJPreferenceLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/AssertJPreferenceLinter.kt index 258c8a5905d..b2a87354071 100644 --- a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/AssertJPreferenceLinter.kt +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/AssertJPreferenceLinter.kt @@ -46,7 +46,7 @@ class AssertJPreferenceLinter : Plugin { return try { val stdout = ByteArrayOutputStream() project.exec { - commandLine("git", "diff", "--name-only", "--diff-filter=A", "HEAD~1") + commandLine("git", "diff", "--name-only", "--diff-filter=A", "origin/master...HEAD") standardOutput = stdout isIgnoreExitValue = true } diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/EmptyInstrumentationLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/EmptyInstrumentationLinter.kt index 48db7a9e240..d561e0987c8 100644 --- a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/EmptyInstrumentationLinter.kt +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/EmptyInstrumentationLinter.kt @@ -3,6 +3,7 @@ package datadog.gradle.plugin.lint import org.gradle.api.GradleException import org.gradle.api.Plugin import org.gradle.api.Project +import java.io.ByteArrayOutputStream class EmptyInstrumentationLinter : Plugin { override fun apply(target: Project) { @@ -19,74 +20,99 @@ class EmptyInstrumentationLinter : Plugin { ) } + // Only check files changed on this branch to avoid flagging existing stubs + val changedFiles = getChangedInstrumentationFiles(target, instrumentationsDir) + val violations = mutableListOf() val hasAdvicePattern = Regex("""implements\s+[^{]*\b(?:HasMethodAdvice|HasAdvice)\b""") val methodAdviceStartPattern = Regex("""(?:public\s+)?(?:\S+\s+)?methodAdvice\s*\(""") val transformCallPattern = Regex("""\btransform\s*\(""") - instrumentationsDir.walk() - .filter { it.isFile && it.name.endsWith(".java") } - .forEach { file -> - val lines = file.readLines() - val content = lines.joinToString("\n") + val filesToCheck = if (changedFiles != null) { + changedFiles.filter { it.isFile && it.name.endsWith(".java") } + } else { + // Fallback: check all files if git diff fails + instrumentationsDir.walk() + .filter { it.isFile && it.name.endsWith(".java") } + .toList() + } - if (!hasAdvicePattern.containsMatchIn(content)) return@forEach + filesToCheck.forEach { file -> + val lines = file.readLines() + val content = lines.joinToString("\n") - // Find methodAdvice( method and check its body for transform( calls - var methodAdviceLineIndex = -1 - for (i in lines.indices) { - if (methodAdviceStartPattern.containsMatchIn(lines[i])) { - methodAdviceLineIndex = i - break - } + if (!hasAdvicePattern.containsMatchIn(content)) return@forEach + + var methodAdviceLineIndex = -1 + for (i in lines.indices) { + if (methodAdviceStartPattern.containsMatchIn(lines[i])) { + methodAdviceLineIndex = i + break } + } - if (methodAdviceLineIndex < 0) return@forEach + if (methodAdviceLineIndex < 0) return@forEach - // Extract method body by tracking brace depth - var braceDepth = 0 - var methodBodyStart = -1 - val methodBodyLines = mutableListOf() + var braceDepth = 0 + var methodBodyStart = -1 + val methodBodyLines = mutableListOf() - for (i in methodAdviceLineIndex until lines.size) { - val line = lines[i] - for (ch in line) { - when (ch) { - '{' -> { - braceDepth++ - if (braceDepth == 1) methodBodyStart = i - } - '}' -> { - braceDepth-- - if (braceDepth == 0 && methodBodyStart >= 0) { - methodBodyLines.add(line) - break - } + for (i in methodAdviceLineIndex until lines.size) { + val line = lines[i] + for (ch in line) { + when (ch) { + '{' -> { + braceDepth++ + if (braceDepth == 1) methodBodyStart = i + } + '}' -> { + braceDepth-- + if (braceDepth == 0 && methodBodyStart >= 0) { + break } } } - if (methodBodyStart >= 0) { - methodBodyLines.add(line) - } - if (braceDepth == 0 && methodBodyStart >= 0) break } - - val methodBody = methodBodyLines.joinToString("\n") - if (!transformCallPattern.containsMatchIn(methodBody)) { - val classNamePattern = Regex("""class\s+(\w+)""") - val className = classNamePattern.find(content)?.groupValues?.get(1) ?: "" - val relPath = file.relativeTo(target.rootProject.projectDir).path - violations.add("EMPTY STUB: $relPath:${methodAdviceLineIndex + 1} — $className.methodAdvice() contains no transform() calls") + if (methodBodyStart >= 0) { + methodBodyLines.add(line) } + if (braceDepth == 0 && methodBodyStart >= 0) break + } + + val methodBody = methodBodyLines.joinToString("\n") + if (!transformCallPattern.containsMatchIn(methodBody)) { + val classNamePattern = Regex("""class\s+(\w+)""") + val className = classNamePattern.find(content)?.groupValues?.get(1) ?: "" + val relPath = file.relativeTo(target.rootProject.projectDir).path + violations.add("EMPTY STUB: $relPath:${methodAdviceLineIndex + 1} — $className.methodAdvice() contains no transform() calls") } + } if (violations.isNotEmpty()) { violations.forEach { target.logger.error(it) } - throw GradleException("Empty instrumentation stubs found! See errors above.") + throw GradleException("Found ${violations.size} new empty instrumentation stub(s)! See errors above.") } else { - target.logger.lifecycle("✓ No empty instrumentation stubs found") + target.logger.lifecycle("checkEmptyInstrumentations: no new empty stubs found") } } } } + + private fun getChangedInstrumentationFiles(project: Project, instrumentationsDir: java.io.File): List? { + return try { + val stdout = ByteArrayOutputStream() + project.exec { + commandLine("git", "diff", "--name-only", "--diff-filter=ACM", "origin/master...HEAD") + standardOutput = stdout + isIgnoreExitValue = true + } + stdout.toString().trim().lines() + .filter { it.endsWith(".java") && it.startsWith("dd-java-agent/instrumentation/") } + .map { project.rootProject.file(it) } + .filter { it.exists() } + } catch (e: Exception) { + project.logger.warn("checkEmptyInstrumentations: could not get changed files, checking all — ${e.message}") + null + } + } } diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/JavadocLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/JavadocLinter.kt index e44da9a2896..311cb2d79be 100644 --- a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/JavadocLinter.kt +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/JavadocLinter.kt @@ -58,7 +58,7 @@ class JavadocLinter : Plugin { return try { val stdout = ByteArrayOutputStream() project.exec { - commandLine("git", "diff", "--name-only", "--diff-filter=ACMR", "HEAD~1") + commandLine("git", "diff", "--name-only", "--diff-filter=ACMR", "origin/master...HEAD") standardOutput = stdout isIgnoreExitValue = true } diff --git a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/UnnecessaryElseLinter.kt b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/UnnecessaryElseLinter.kt index 6c6584cb2c6..c2d9860af3d 100644 --- a/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/UnnecessaryElseLinter.kt +++ b/buildSrc/src/main/kotlin/datadog/gradle/plugin/lint/UnnecessaryElseLinter.kt @@ -10,8 +10,7 @@ class UnnecessaryElseLinter : Plugin { description = "Scan changed Java files for unnecessary else blocks after return/throw/continue/break" doLast { - val baseSha = "9b933669729ea8a7af00f5cf3c36b6720ec433bd" - val changedFiles = getChangedJavaFiles(target, baseSha) + val changedFiles = getChangedJavaFiles(target) val repoRoot = target.rootProject.projectDir.toPath() val warnings = mutableListOf() @@ -33,9 +32,9 @@ class UnnecessaryElseLinter : Plugin { } } -private fun getChangedJavaFiles(project: Project, baseSha: String): List { +private fun getChangedJavaFiles(project: Project): List { return try { - val process = ProcessBuilder("git", "diff", "--name-only", baseSha, "HEAD") + val process = ProcessBuilder("git", "diff", "--name-only", "--diff-filter=ACM", "origin/master...HEAD") .directory(project.rootProject.projectDir) .redirectErrorStream(true) .start() diff --git a/dd-java-agent/src/test/java/datadog/trace/agent/ArchitectureTest.java b/dd-java-agent/src/test/java/datadog/trace/agent/ArchitectureTest.java index 8325caf66a0..ee58e0c6e7e 100644 --- a/dd-java-agent/src/test/java/datadog/trace/agent/ArchitectureTest.java +++ b/dd-java-agent/src/test/java/datadog/trace/agent/ArchitectureTest.java @@ -6,6 +6,7 @@ import com.tngtech.archunit.core.importer.ClassFileImporter; import com.tngtech.archunit.core.importer.ImportOption; import com.tngtech.archunit.lang.ArchRule; +import com.tngtech.archunit.library.freeze.FreezingArchRule; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -13,6 +14,8 @@ * Architecture fitness tests enforcing module dependency rules. These rules prevent the most common * violations found during PR reviews: bootstrap depending on core, instrumentations reaching into * core internals, and forbidden JDK APIs in bootstrap code. + * + *

Uses {@link FreezingArchRule} to baseline existing violations so only new violations fail. */ class ArchitectureTest { @@ -37,7 +40,7 @@ void bootstrapShouldNotDependOnCore() { .resideInAPackage("datadog.trace.core..") .because("Bootstrap classes load before core and must not depend on core internals"); - rule.check(allClasses); + FreezingArchRule.freeze(rule).check(allClasses); } @Test @@ -51,7 +54,7 @@ void instrumentationShouldNotDependOnCoreInternals() { .resideInAPackage("datadog.trace.core.internal..") .because("Instrumentations should use internal-api, not core internals directly"); - rule.check(allClasses); + FreezingArchRule.freeze(rule).check(allClasses); } @Test @@ -67,7 +70,7 @@ void bootstrapShouldNotUseJavaUtilLogging() { "java.util.logging locks in the log manager before the app configures it" + " (see docs/bootstrap_design_guidelines.md)"); - rule.check(allClasses); + FreezingArchRule.freeze(rule).check(allClasses); } @Test @@ -83,6 +86,6 @@ void bootstrapShouldNotUseJavaxManagement() { "javax.management causes class loading issues in premain" + " (see docs/bootstrap_design_guidelines.md)"); - rule.check(allClasses); + FreezingArchRule.freeze(rule).check(allClasses); } }