Fix classLoaderMatcher review instructions (#16489)

Author: trask

.github/agents/code-review-and-fix.agent.md

@@ -112,11 +112,18 @@ Auto-fix boundaries:
     to a library family with sibling version modules, it must list all siblings via
     `testInstrumentation`. Check `settings.gradle.kts` for sibling `:javaagent` modules
     under the same parent. After adding, verify by running the module's tests.
-  - missing version comments on `hasClassesNamed()` landmark classes in
-    `classLoaderMatcher()` — look up the library version that introduced each class (check
-    muzzle `versions.set(...)` ranges, module directory name, existing code comments, and
-    Javadoc/release notes) and add a `// added in X.Y` or `// removed in X.Y` comment above
-    each class name string
+  - missing version comments on `hasClassesNamed()` landmark classes in existing
+    `classLoaderMatcher()` overrides (multi-class checks or `.and(not(...))` chains only) —
+    look up the library version that introduced each class (check muzzle `versions.set(...)`
+    ranges, module directory name, existing code comments, and Javadoc/release notes) and
+    add a `// added in X.Y` or `// removed in X.Y` comment above each class name string.
+    Do NOT add a `classLoaderMatcher()` override where one does not already exist —
+    this method is only for version-boundary detection when muzzle is insufficient,
+    not for optimization (use `TypeInstrumentation.classLoaderOptimization()` instead)
+  - redundant `isMethod()` in method matchers inside `transform()` when the code is
+    already being modified — `isMethod()` only serves to exclude constructors, but
+    `named(...)` already excludes them because constructors are named `<init>`
+    (e.g., `isMethod().and(named("execute"))` → `named("execute")`)
   - singleton-to-instance-creation conversion for stateless telemetry interface
     implementations (`TextMapGetter`, `TextMapSetter`, `*AttributesGetter`,
     `AttributesExtractor`, `SpanNameExtractor`, `HttpServerResponseMutator`) — replace

.github/agents/knowledge/general-rules.md

@@ -19,7 +19,7 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
 | Naming | Module/package naming | New or renamed modules/packages | `module-naming.md` |
 | Javaagent | Advice patterns | `@Advice` classes | `javaagent-advice-patterns.md` |
 | Javaagent | Module structure patterns | `InstrumentationModule`, `TypeInstrumentation`, `Singletons` | `javaagent-module-patterns.md` |
-| Javaagent | Missing `classLoaderMatcher()` | `InstrumentationModule` without `classLoaderMatcher()` override | `javaagent-module-patterns.md` |
+| Javaagent | Incorrect `classLoaderMatcher()` | `classLoaderMatcher()` override that is redundant (muzzle already handles it) or missing when needed (muzzle cannot distinguish version range) | `javaagent-module-patterns.md` |
 | Semconv | Library vs javaagent semconv constant usage | Semconv constants/assertions | — |
 | Semconv | Dual semconv testing | `SemconvStability`, `maybeStable`, semconv Gradle tasks | `testing-semconv-stability.md` |
 | Testing | General test patterns | Test files in scope | `testing-general-patterns.md` |

.github/agents/knowledge/javaagent-module-patterns.md

@@ -140,14 +140,14 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 
 #### Rules for `classLoaderMatcher()`
 
-- **Every `InstrumentationModule` should override `classLoaderMatcher()`** with a
-  `hasClassesNamed(...)` check for a class from the module's target library. This allows
-  the agent to skip the entire module early — before evaluating type matchers or muzzle —
-  when the target library is absent.
-  **Exceptions** — do not flag: modules under `instrumentation/internal/` (JDK/agent internals),
-  modules instrumenting JDK classes on all supported Java versions (executors,
-  http-url-connection, java-util-logging, rmi, jdbc), config-driven modules with no fixed
-  target library (`methods`, `external-annotations`), and test-only modules.
+- **Do NOT add `classLoaderMatcher()` for optimization.** The "skip when library is absent"
+  optimization belongs on `TypeInstrumentation.classLoaderOptimization()`, not here.
+  `classLoaderMatcher()` is only for **version-boundary detection** — cases where muzzle
+  alone cannot distinguish the target version range. Most modules do not need it.
+- **Do NOT flag modules that lack a `classLoaderMatcher()` override.** The default (`any()`)
+  is correct when muzzle can detect version boundaries on its own (the common case). Only
+  flag a *missing* override when the module targets a specific version range and the
+  versioning signal (added/removed landmark class) is not part of the classes muzzle inspects.
 - **Version comments on landmark classes.** For multi-class checks, `.and(not(...))`
   chains, or cases where the landmark version differs from the module's base version,
   each `hasClassesNamed()` call must have a `//` comment stating the library version
@@ -185,7 +185,7 @@ public class MyClassInstrumentation implements TypeInstrumentation {
   @Override
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
-        isMethod().and(named("execute")).and(takesArguments(1)),
+        named("execute").and(takesArguments(1)),
         getClass().getName() + "$ExecuteAdvice");
   }
 
@@ -196,6 +196,32 @@ public class MyClassInstrumentation implements TypeInstrumentation {
 }
 ```
 
+### `classLoaderOptimization()` — Skip When Library Absent
+
+Override `classLoaderOptimization()` when the `typeMatcher()` uses expensive matchers
+(e.g., `implementsInterface(...)`, `extendsClass(...)`, `isAnnotatedWith(...)`). This is
+a fast pre-filter that runs before type matching: if a class from the target library is
+absent, the expensive type matcher is skipped entirely.
+
+```java
+@Override
+public ElementMatcher<ClassLoader> classLoaderOptimization() {
+  return hasClassesNamed("com.example.library.TargetClass");
+}
+```
+
+**This is the correct place for "skip when library is absent" optimization** — not
+`InstrumentationModule.classLoaderMatcher()`. The module-level `classLoaderMatcher()` is
+ANDed with `classLoaderOptimization()` at runtime, so the type-level check alone is
+sufficient for optimization.
+
+**When to override:**
+
+- The `typeMatcher()` uses hierarchy matchers (`implementsInterface`, `extendsClass`) or
+  annotation matchers — these require bytecode inspection of super classes/interfaces.
+- The `typeMatcher()` uses `named(...)` or `namedOneOf(...)` — no override needed because
+  name-only matchers are already fast (they check only the class name, no bytecode).
+
 ### Rules
 
 - Do not flag or change the visibility or `final` modifier on `TypeInstrumentation`,
@@ -206,6 +232,9 @@ public class MyClassInstrumentation implements TypeInstrumentation {
   `extendsClass(...)` and `implementsInterface(...)` are appropriate when the instrumentation
   targets subclasses or implementors of a type.
 - `transform()` wires method matchers to advice classes via `applyAdviceToMethod()`.
+- Do not add `isMethod()` in method matchers inside `transform()`. `isMethod()` only
+  serves to exclude constructors, but `named(...)` already excludes them because
+  constructors are named `<init>`. Remove `isMethod()` when not needed.
 - Reference the advice class using `getClass().getName() + "$InnerClassName"` — not
   `InnerClassName.class.getName()`, `OuterClass.class.getName()`, or a string literal.
   Any `.class.getName()` reference — whether to the inner advice class or the outer