Merge branch 'main' into catch-names

Author: trask

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

@@ -14,6 +14,9 @@ Primary responsibilities:
   Issues that cannot be fixed are reported only in the final output.
 - Produce only the output format requested by the caller. Do not assume or add a default output format.
 - Use only the tools actually exposed by the runtime. Do not assume helper or companion tools exist.
+- Ignore generic runtime suggestions that mention undeclared helper-tool names such as `read_bash`,
+  `write_bash`, `stop_bash`, `read_shell`, or similar follow-up shell helpers unless those exact
+  tools are explicitly exposed in the current session.
 - When a command-execution step fails for tool-related reasons, first re-evaluate the declared tools and retry with a different valid execution strategy before concluding that the environment cannot complete the task.
 - Distinguish between command failure and inability to observe command completion or final status. Do not collapse these into the same explanation.
 
@@ -131,6 +134,9 @@ Auto-fix boundaries:
 
 - Safe to fix:
   - import cleanup or direct style-guide conformance
+  - normalization of existing `@SuppressWarnings` syntax or placement, but preserve any
+    accurate explanatory comment attached to the suppression instead of deleting it as
+    style noise
   - obvious assertion API migrations (e.g., AssertJ preference) and idiomatic
     simplifications listed in `testing-general-patterns.md` § AssertJ Idiomatic
     Simplifications (e.g., `assertThat(list.size()).isEqualTo(N)` →
@@ -139,6 +145,8 @@ Auto-fix boundaries:
     lambdas where the lambda parameter is already an `AbstractAssert` (e.g.,
     `AbstractStringAssert`). Calls like `taskId.contains(jobName)` on the assert object are
     already valid AssertJ assertions; do not wrap them in `assertThat(...).isTrue()`
+  - AssertJ `.as(...)` descriptions and `.withFailMessage(...)` in tests — remove them
+    and prefer direct assertions whose failure output already exposes the unexpected values
   - deterministic semconv constant handling aligned with repository rules
   - missing test-task wiring patterns with clear canonical form
   - missing `testInstrumentation` cross-version references — when a javaagent module belongs
@@ -151,22 +159,28 @@ Auto-fix boundaries:
     same parent and apply the step-by-step procedure in `gradle-conventions.md`.
     After adding, verify by running the module's tests.
   - missing version comments on `hasClassesNamed()` landmark classes in existing
-    `classLoaderMatcher()` overrides, including single-class lower-bound checks —
+    `classLoaderMatcher()` overrides, including single-class checks —
     determine each class's **role** (floor vs ceiling) and add the matching comment.
     First check: does a **newer** sibling instrumentation module exist for this library
     (e.g., `mongo-4.0` next to `mongo-3.7`)? If so, look at what the newer module checks
     in *its* `classLoaderMatcher()`. Classes that are present in the newer module's check
     but absent from the current module's check (or vice versa) reveal a version boundary —
     the class was likely added or removed between versions.
     Then determine the comment form for each class:
-    - **Floor class** (proves "at least version X"): look up when the class was **introduced**
-      → comment `// added in X.Y`.
-    - **Ceiling class** (proves "not yet version Y"): look up when the class was **removed**
-      → comment `// removed in Y.Z` (meaning: its presence here ensures we don't match
-      version Y.Z+ where a different module takes over).
+   **Positive floor class** (proves "at least version X"): look up when the class was
+   **introduced** → comment `// added in X.Y`.
+   **Positive ceiling class** (proves "not yet version Y"): look up when the class was
+   **removed** → comment `// removed in Y.Z` (meaning: its presence here ensures we
+   don't match version Y.Z+ where a different module takes over).
+   **Negated exclusion class** in `not(hasClassesNamed(...))`: look up when the class was
+   **introduced** → comment `// added in Y.Z`, because that first appearance begins the
+   excluded version range.
+   **Single positive class that provides both bounds**: include both facts in one comment,
+   e.g. `// added in X.Y, removed in Y.Z`.
     A ceiling class might have been *introduced* much earlier than the module's target version.
-    Do not use `// added in` for a ceiling class — that is misleading. The relevant fact is
-    when it was **removed**.
+   Do not use `// added in` for a positive ceiling class — that is misleading. The relevant
+   fact is when it was **removed**. But for a negated exclusion class, `// added in` is the
+   correct form because the class's introduction is exactly what starts excluding newer versions.
     Validate the version in the comment before adding or requesting it. Do not guess the
     version from the module name alone; confirm it with repository or upstream evidence.
     Sources: muzzle `versions.set(...)` ranges, sibling module `classLoaderMatcher()` checks,
@@ -198,6 +212,16 @@ Auto-fix boundaries:
     to avoid allocating on every invocation. Only convert singletons used at
     registration/initialization time (e.g., `Instrumenter` builder chains, `Singletons`
     setup)
+  - try-with-resources wrapping most of a test body for an `AutoCloseable` that only
+    needs cleanup at test end — convert to `AutoCleanupExtension` with `deferCleanup(...)`.
+    Add a `@RegisterExtension static final AutoCleanupExtension cleanup =
+    AutoCleanupExtension.create();` field if one does not already exist, then replace
+    the try-with-resources with `cleanup.deferCleanup(resource);` and un-indent the body.
+    Keep try-with-resources for semantically scoped resources whose lifetime must end
+    mid-test (e.g., `Scope` / `Context.makeCurrent()`, `MockedStatic`, short-lived
+    readers/writers/streams/response bodies).
+    Do not apply this conversion in non-JUnit helper methods, `@BeforeAll`, or shared
+    setup code.
   - `hasAttributesSatisfying(...)` calls in test assertions — replace with
     `hasAttributesSatisfyingExactly(...)` because it is more precise (the non-exact
     variant silently ignores unexpected attributes)
@@ -280,6 +304,9 @@ Output content rules:
 - When the caller requests line-oriented output, use the first relevant changed line as the line hint.
 - When writing structured output to a file, write only the requested payload. Do not wrap it in Markdown fences,
   add headings, or include extra commentary before or after it.
+- If validation is still in progress or its final exit status cannot be confirmed when you must end,
+  encode that state only inside the requested final payload. Do not prepend wait-state prose,
+  status updates, or explanations ahead of the caller-required format.
 
 ### Phase 4: Validate
 
@@ -316,6 +343,12 @@ reporting a limitation:
    attempted command or validation step and say whether it failed or whether completion or final
    status could not be confirmed.
 
+If the runtime prints generic advice that suggests undeclared helper tools after a long-running
+Gradle command, treat that advice as boilerplate, not as an available recovery path. Do not claim
+that such an undeclared tool is required, expected, or missing. Describe the limitation only in
+terms of the tools that were actually declared and the concrete fact that final exit status could
+not be observed.
+
 **Never pipe Gradle output through `tail`, `head`, `grep`, or any other command** (e.g.,
 `./gradlew :foo:check 2>&1 | tail -30`). Piping masks the Gradle exit code because the
 shell reports the exit code of the last pipe segment, not Gradle. A failing build will

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

@@ -94,10 +94,13 @@ For each file in scope:
 5. Do not flag a non-capturing lambda or method reference as an unnecessary allocation,
    because on modern JDKs these are typically cached at the call site rather than
    allocated on every invocation.
-6. Flag a missing `// added in X.Y` comment on a single-class lower-bound
-   `hasClassesNamed(...)` check in `classLoaderMatcher()` only after validating that the
-   stated version is factually correct from repository or upstream evidence; the comment
-   documents why the matcher exists and which version boundary it enforces.
+6. Flag a missing version-boundary comment on a single-class `hasClassesNamed(...)`
+   check in `classLoaderMatcher()` only after validating the stated boundary from
+   repository or upstream evidence. Use `// added in X.Y` for a pure lower bound.
+   If the same positive class also provides the upper bound because it disappears in a
+   newer sibling version, preserve or request `removed in Y.Z` too. For negated checks
+   such as `not(hasClassesNamed(...))`, use `// added in Y.Z` because the class's first
+   appearance is what starts the excluded version range.
 7. Prevent duplicates:
    - If equivalent `REVIEW:` already exists above the same line, do not add another.
 

.github/agents/knowledge/README.md

@@ -13,10 +13,11 @@ Load only files relevant to the current scope to reduce noise and avoid over-con
 | `general-rules.md` | Always — review checklist table and core rules enforced on every review |
 | `gradle-conventions.md` | `build.gradle.kts` or `settings.gradle.kts` changes, custom test task registration or wiring |
 | `javaagent-advice-patterns.md` | ByteBuddy `@Advice` class or advice-method changes |
-| `javaagent-module-patterns.md` | `InstrumentationModule`, `TypeInstrumentation`, `Singletons`, `VirtualField`, `CallDepth` |
+| `javaagent-module-patterns.md` | `InstrumentationModule`, `TypeInstrumentation`, `VirtualField`, `CallDepth` |
+| `javaagent-singletons-patterns.md` | `*Singletons` holder classes, singleton accessors, static-imported singleton callers |
 | `library-patterns.md` | Library instrumentation telemetry, builder, getter, or setter pattern changes |
 | `module-naming.md` | New or renamed modules or packages; settings includes |
-| `testing-general-patterns.md` | Test files in scope — assertion style, attribute assertion patterns, `satisfies()` lambda usage |
+| `testing-general-patterns.md` | Test files in scope — assertion style, resource cleanup patterns, attribute assertion patterns, `satisfies()` lambda usage |
 | `testing-experimental-flags.md` | `testExperimental` task or experimental span-attribute assertions |
 | `testing-semconv-stability.md` | Semconv opt-in modes, `emitOld*`/`emitStable*`, `maybeStable`, Semconv test tasks |
 

.github/agents/knowledge/config-property-stability.md

@@ -52,7 +52,7 @@ Defined in [VERSIONING.md](../../../VERSIONING.md):
 Examples (flat ↔ YAML):
 
 - `otel.instrumentation.http.client.capture-request-headers` ↔ `request_captured_headers` — **stable**
-- `otel.instrumentation.common.experimental.db-sqlcommenter.enabled` ↔ `sqlcommenter/development: { enabled: true }` — **experimental**
+- `otel.instrumentation.common.db.experimental.sqlcommenter.enabled` ↔ `sqlcommenter/development: { enabled: true }` — **experimental**
 - `otel.instrumentation.http.client.emit-experimental-telemetry` ↔ `emit_experimental_telemetry/development: true` — **experimental**
 
 ## Deprecation Communication

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

@@ -15,23 +15,25 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
 | --- | --- | --- | --- |
 | General | Logic, correctness, reliability, safety, copy/paste mistakes, incorrect comments | Always | — |
 | Style | Style guide | Always | — |
-| Style | Uppercase field names should reflect semantic constants, not simply `static final` | Always | — |
+| Style | Uppercase field names should reflect semantic constants or immutable value constants such as `Duration` timeouts/intervals, not simply `static final` | Always | — |
 | Naming | Getter naming (`get` / `is`) | Always | — |
 | Style | Prefer `e` for used exceptions, prefer `f` for used exceptions in nested catch clauses when an outer catch already uses `e`, allow `error` for specific `*Error` catch types, prefer `t` for used `Throwable` values, prefer `ignored` for intentionally unused catch parameters, and use `ignore` for nested intentionally unused catch parameters when `ignored` would shadow an outer catch variable | Catch clauses, `Throwable` params | — |
 | 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 | Module structure patterns | `InstrumentationModule`, `TypeInstrumentation` | `javaagent-module-patterns.md` |
+| Javaagent | Singletons patterns | `*Singletons` holder classes, singleton accessors, static-imported singleton callers | `javaagent-singletons-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` |
+| Testing | General test patterns | Test files in scope — assertion style, resource cleanup, attribute assertions | `testing-general-patterns.md` |
 | Testing | Experimental flag tests | `testExperimental`, experimental attribute assertions, `experimental` flags in JVM args or system properties | `testing-experimental-flags.md` |
 | Library | TelemetryBuilder/getter/setter patterns | Library instrumentation classes | `library-patterns.md` |
 | API | Deprecation and breaking-change policy | Public API changes | `api-deprecation-policy.md` |
 | Config | Config property stability/renames/removals | `otel.instrumentation.*` property changes, `DeclarativeConfigUtil` or `ConfigProperties` usage | `config-property-stability.md` |
 | Build | Gradle conventions, muzzle, test tasks, plugins | `build.gradle.kts`, `settings.gradle.kts` | `gradle-conventions.md` |
 | Build | `testcontainersBuildService` declaration | Testcontainers dependency without `usesService` | `gradle-conventions.md` |
 | Style | Prefer instance creation over singletons for stateless interface impls (except on hot paths or Kotlin `object` declarations) | `TextMapGetter`, `TextMapSetter`, `*AttributesGetter`, `AttributesExtractor`, `SpanNameExtractor`, `HttpServerResponseMutator`, enum/static singletons | — |
+| Style | Prefer `value == null` / `value != null` over `null == value` / `null != value` | Null comparisons | — |
 | Style | No unnecessary explicit type witnesses on generic method calls (`Collections.<String>emptyList()`) | Java generic method calls with explicit type parameters | — |
 | Style | Remove redundant null guards on attribute puts | `AttributesBuilder.put`, `onStart`, `onEnd`, attribute extraction methods | — |
 | General | No redundant `ByteBuffer.duplicate()` on `Value.getValue()` | `Value.getValue()` with `BYTES` type, `ByteBuffer` handling | — |
@@ -87,6 +89,15 @@ Do not flag the following patterns (common false positives):
   The project disables javac's `-Xlint:deprecation` globally and uses a custom Error Prone
   check (`OtelDeprecatedApiUsage`) instead. Only add the annotation when it is actually
   required to fix an Error Prone error — not speculatively.
+- Preserve concise explanatory comments attached to existing `@SuppressWarnings` annotations
+  when they explain why the suppression exists (for example `// using deprecated semconv`
+  or `// using deprecated config property`). This repository has established precedent for
+  such comments, and test code disables Error Prone's `SuppressWarningsWithoutExplanation`
+  check only because enforcing it everywhere causes too many failures, not because these
+  comments are undesirable.
+- When normalizing or moving an existing `@SuppressWarnings`, keep any accurate explanatory
+  comment with it. Remove the comment only if it is incorrect, obsolete, or contradicted by
+  the code after your change.
 
 ## [Naming] Getter Naming
 

.github/agents/knowledge/gradle-conventions.md

@@ -51,18 +51,37 @@ Verify `build.gradle.kts` applies the correct plugin for the module type:
 | `library/` | `otel.library-instrumentation` |
 | `testing/` | `otel.java-conventions` |
 
-## Boolean Project Properties
+## Shared Gradle Project Properties
 
-For simple boolean Gradle project properties in `build.gradle.kts`, prefer the repository's most
-common pattern:
+In `build.gradle.kts`, prefer the shared `otelProps` extension for repository-wide Gradle project
+properties that are already modeled there, including:
+
+- `testLatestDeps`
+- `denyUnsafe`
+- `collectMetadata`
+- `testJavaVersion`
+- `testJavaVM`
+- `maxTestRetries`
+- `enableStrictContext`
+
+Examples:
 
 ```kotlin
-val myFlag = findProperty("myFlag") == "true"
-if (myFlag) {
+if (otelProps.testLatestDeps) {
   // ...
 }
+
+tasks.withType<Test>().configureEach {
+  systemProperty("collectMetadata", otelProps.collectMetadata)
+}
 ```
 
+For module-local one-off properties that are not part of `otelProps`, using `findProperty(...)`
+directly is still fine.
+
+`settings.gradle.kts` is the exception: it cannot use project extensions like `otelProps`, so
+direct `gradle.startParameter.projectProperties[...]` access is expected there.
+
 ## `testInstrumentation` Dependencies
 
 The `testInstrumentation` configuration declares which other javaagent instrumentation modules
@@ -175,6 +194,9 @@ When a module has custom test tasks (e.g., `testStableSemconv`), system properti
 args that apply to **all** test tasks should be set once in a `withType<Test>().configureEach`
 block, not repeated on each individual task.
 
+If a property or JVM arg is moved into `withType<Test>().configureEach`, remove any now-redundant
+copies from individual tasks unless a task intentionally overrides the shared value.
+
 When there is only one test task, `tasks.test { ... }` is fine — do not convert it to
 `withType<Test>().configureEach` and do not flag it.
 
@@ -186,7 +208,7 @@ should apply to all tasks. If so, move them to `withType<Test>().configureEach`.
 ```kotlin
 tasks {
   withType<Test>().configureEach {
-    systemProperty("collectMetadata", findProperty("collectMetadata"))
+    systemProperty("collectMetadata", otelProps.collectMetadata)
     // ... other properties common to all test tasks
   }
 
@@ -205,7 +227,7 @@ review**. Only verify correctness when they are already present.
 
 | Property | Type | Value |
 | --- | --- | --- |
-| `collectMetadata` | System property | Pass-through of the `collectMetadata` Gradle project property; defaults to `"false"` |
+| `collectMetadata` | System property | Pass-through of `otelProps.collectMetadata`; defaults to `false` |
 | `metadataConfig` | System property | A single `key=value` string describing the non-default configuration active during this test run |
 
 When already present, verify: