Trim AGENTS.md of duplicate content (#12003)

Trim AGENTS.md of duplicate content

feat(ai): Restore techdebt review

Co-authored-by: PerfectSlayer <bruce.bujon@datadoghq.com>
Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

Author: 3 people

.agents/skills/techdebt/SKILL.md

@@ -1,6 +1,12 @@
 ---
 name: techdebt
-description: Analyze branch changes for technical debt, code duplication, and unnecessary complexity
+description: >-
+  Review a code diff / branch / PR for technical debt — code duplication,
+  unnecessary complexity / over-engineering, and redundant or dead code. Use
+  whenever the user wants a tech-debt, cleanup, or refactor review, asks to check
+  a branch or PR for duplication / complexity / dead code before opening a PR, or
+  mentions "techdebt". Refactor-only: it reports issues and offers
+  behavior-preserving fixes; it never changes behavior.
 user-invocable: true
 context: fork
 allowed-tools:

.claude/CLAUDE.md

@@ -2,6 +2,6 @@
 
 ## Claude Code workflow
 
-- Before creating a pull request, run `/techdebt` to check for technical debt, code duplication, and unnecessary complexity in the branch changes.
+- Before creating a pull request, run the [Review Guidelines](../AGENTS.md#review-guidelines) checks over the branch changes.
 - After running `/migrate-groovy-to-java`, run `/review-groovy-migration` on the migrated files before opening a PR.
 - Always write new unit tests using JUnit 5 and Java. If the target test file is `.groovy`, run the `/migrate-groovy-to-java` skill on it first.

AGENTS.md

@@ -38,6 +38,7 @@ docs/                     Developer documentation (see below)
 | Testing guide (6 test types) | [docs/how_to_test.md](docs/how_to_test.md) |
 | Working with Gradle | [docs/how_to_work_with_gradle.md](docs/how_to_work_with_gradle.md) |
 | Bootstrap/premain constraints | [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md) |
+| Instrumentation/advice constraints | [docs/instrumentation_design_guidelines.md](docs/instrumentation_design_guidelines.md) |
 | CI/CD workflows | [.github/workflows/README.md](.github/workflows/README.md) |
 
 **When working on a topic above, read the linked file first** — they are the source of truth maintained by humans.
@@ -56,14 +57,12 @@ docs/                     Developer documentation (see below)
 ## Code conventions
 
 - **Formatting**: google-java-format enforced via Spotless. Run `./gradlew spotlessApply` before committing.
-- **Static imports**: Prefer static imports over class-qualified calls for call-style helpers — JUnit `Assertions`, Mockito (`mock`, `when`, `verify`, `anyString`, `RETURNS_DEFAULTS`, ...), Hamcrest/AssertJ matchers, internal test DSLs, and similar. Same goes for production code: `Collections.emptyList()` is fine, but if you find yourself repeatedly writing `Foo.bar(...)` where `Foo` adds no information at the call site, static-import `bar`. Wildcard `import static x.*` is disallowed (enforced by IDE config in CONTRIBUTING.md).
+- **Static imports**: Prefer static imports over class-qualified calls for call-style helpers, in both test (Assertions.assertEquals, Mockito.mock) and production code (Collections.emptyList). Wildcard imports disallowed — see CONTRIBUTING.md.
 - **Instrumentation layout**: `dd-java-agent/instrumentation/{framework}/{framework}-{minVersion}/`
 - **Instrumentation pattern**: Type matching → Method matching → Advice class (bytecode advice, not AOP)
 - **Test frameworks**: Always use JUnit 5 for unit tests. Only use Groovy / Spock tests for instrumentation and smoke tests.
 - **Forked tests**: Use `ForkedTest` suffix when tests need a separate JVM
 - **Flaky tests**: Annotate with `@Flaky` — they are skipped in CI by default
-- **Instrumentation one-shot methods**: Never extract the return values of `triggerClasses()`, `contextStore()`, `classLoaderMatcher()`, or `methodAdvice()` into static constants. These are called once by the framework — extracting to a constant adds constant-pool bloat with no benefit.
-- **Scope lifecycle order**: Keep the scope open until all work that needs the current active span is done — decorator calls, async callback registration, etc. Required order: decorator calls and callback registration, then `scope.close()`, then `span.finish()`.
 
 ## PR conventions
 
@@ -74,23 +73,10 @@ docs/                     Developer documentation (see below)
 
 ## Review Guidelines
 
-Before marking a PR ready, run the applicable reviews below over the branch changes (the diff since the merge-base with `master`). These are **advisory, precision-first** checks — they surface high-severity issues early and are not merge gates. The linked guidelines are the tool-agnostic source of truth.
+- **Technical debt**: run `/techdebt` over branch changes before marking a PR ready to catch code duplication, unnecessary complexity, and dead code (refactor-only, never changes behavior) — see [.agents/skills/techdebt/SKILL.md](.agents/skills/techdebt/SKILL.md).
+- **Performance**: run `/perf-review` over branch changes before marking a PR ready (advisory, not a merge gate) — see [.agents/skills/perf-review/SKILL.md](.agents/skills/perf-review/SKILL.md).
 
-### Performance Review
+## Critical constraints
 
-Follow the performance-review guidelines in [.agents/skills/perf-review/SKILL.md](.agents/skills/perf-review/SKILL.md) — the do-no-harm / assume-hot posture, the hot-path checks (universal + Java J1–J11 + ByteBuddy-Advice idioms), and the confidence/severity model, with the detailed rubric in its `references/`. Scope: hot-path allocation, unbounded memory, repeated work, escaping objects, native-boundary crossings, and JVM-specific pitfalls.
-
-## Bootstrap constraints (critical)
-
-Code running in the agent's `premain` phase must **not** use:
-- `java.util.logging.*` — locks in log manager before app configures it
-- `java.nio.file.*` — triggers premature provider initialization
-- `javax.management.*` — causes class loading issues
-
-See [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md) for details and alternatives.
-
-## Advice constraints (critical)
-
-Advice methods (OnMethodEnter/OnMethodExit) are inlined into the instrumented class's bytecode.
-Calling a static interface method there, such as Context.root() or Context.current(), can cause
-a VerifyError at runtime — use Java8BytecodeBridge (rootContext, currentContext, etc.) instead.
+- **Bootstrap**: never use `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*` in `premain` code — see [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md).
+- **Advice**: never call a static interface method from advice — can cause a `VerifyError` — see [docs/instrumentation_design_guidelines.md](docs/instrumentation_design_guidelines.md).

CONTRIBUTING.md

@@ -63,6 +63,16 @@ For IntelliJ IDEA, we suggest the following settings and plugin.
 * To run test in a specific JDK use the `testJvm` property, e.g. `-PtestJvm=11`
 * Install the [Google Java Format](https://plugins.jetbrains.com/plugin/8527-google-java-format) plugin
 
+### Static imports
+
+Prefer static imports over class-qualified calls for call-style helpers — JUnit `Assertions`, Mockito (`mock`,
+`when`, `verify`, `anyString`, `RETURNS_DEFAULTS`, ...), Hamcrest/AssertJ matchers, internal test DSLs, and similar.
+
+Same goes for production code: `Collections.emptyList()` is fine, but if you find yourself repeatedly writing
+`Foo.bar(...)` where `Foo` adds no information at the call site, static-import `bar`.
+
+Wildcard imports (`import x.*` or `import static x.*`) are disallowed — see the IntelliJ IDEA settings above.
+
 ### Troubleshooting
 
 * Gradle fails with a "too many open files" error.

docs/instrumentation_design_guidelines.md

@@ -0,0 +1,83 @@
+# Instrumentation and Advice Design Guidelines
+
+This document outlines design constraints and best practices when writing or refactoring instrumentation and advice code.
+Following these guidelines will help avoid constant-pool bloat and subtle span/scope lifecycle bugs.
+
+## Background
+
+Instrumentation classes wire up type/method matchers and advice once, when the framework loads them. Advice methods
+are inlined into the instrumented application's bytecode and run on every invocation of the matched method, so their
+ordering directly affects when spans are visible to nested work.
+
+## Constraints to Follow
+
+### 1. Instrumentation one-shot methods
+
+**Why to avoid extracting to constants:**
+
+- `triggerClasses()`, `contextStore()`, `classLoaderMatcher()`, and `methodAdvice()` are called exactly once by the
+  framework when the instrumentation is registered
+- Extracting their return values into static constants adds constant-pool bloat with no runtime benefit
+
+**What to do instead:**
+
+Return the values directly from the method; don't cache them in a field.
+
+```java
+// BAD - unnecessary static constant
+private static final ElementMatcher.Junction<ClassLoader> CL_MATCHER = hasClassesNamed("foo.Bar");
+
+@Override
+public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
+  return CL_MATCHER;
+}
+
+// GOOD - construct directly in the one-shot method
+@Override
+public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
+  return hasClassesNamed("foo.Bar");
+}
+```
+
+### 2. Scope lifecycle order
+
+**Why to avoid closing the scope early:**
+
+- Keeping the scope open until all work needing the current active span is done ensures decorator calls and async
+  callback registration see the span as active
+- Closing the scope before that work runs can make nested code lose track of the current span
+
+**What to do instead:**
+
+Required order: decorator calls and callback registration, then `scope.close()`, then `span.finish()`.
+
+```java
+// GOOD
+DECORATE.onOperation(span, request);
+registerCallback(span);
+scope.close();
+span.finish();
+```
+
+### 3. Static interface methods in advice
+
+**Why to avoid calling them directly:**
+
+- Advice methods (`@Advice.OnMethodEnter`/`@Advice.OnMethodExit`) are inlined into the instrumented class's bytecode
+- Calling a static interface method there, such as `Context.current()` or `Context.root()`, can cause a
+  `VerifyError` at runtime
+
+**What to use instead:**
+
+Use `Java8BytecodeBridge` (`currentContext()`, `rootContext()`, etc.) in place of the static interface method,
+static-imported for readability.
+
+```java
+import static datadog.trace.bootstrap.instrumentation.api.Java8BytecodeBridge.currentContext;
+
+// BAD - static interface method call, can cause VerifyError when inlined
+Context ctx = Context.current();
+
+// GOOD - use the bytecode bridge, static-imported
+Context ctx = currentContext();
+```