Code review sweep (run 25235735340)

[otelbot]

Automated code review sweep walked the following modules in order
and stopped after accumulating at least 10 modified files:

• servlet-3.0:javaagent-unit-tests
• servlet-3.0:library
• servlet-3.0:testing
• servlet-5.0:javaagent
• servlet-5.0:javaagent-unit-tests
• servlet-5.0:jetty11-testing
• servlet-5.0:jetty12-testing
• servlet-5.0:library
• servlet-5.0:testing
• servlet-5.0:tomcat-testing

---

Module: servlet-3.0:javaagent-unit-tests

Module path: instrumentation/servlet/servlet-3.0/javaagent-unit-tests

Summary

No safe repository-guideline fixes were applied after reviewing all files under instrumentation/servlet/servlet-3.0/javaagent-unit-tests and validating the related metadata.yaml entries by inspection.

Applied Changes

No safe automated changes were applied.

Module: servlet-3.0:library

Module path: instrumentation/servlet/servlet-3.0/library

Summary

No safe repository-guideline fixes were found in instrumentation/servlet/servlet-3.0/library; no files were changed.

Applied Changes

No safe automated changes were applied.

Module: servlet-3.0:testing

Module path: instrumentation/servlet/servlet-3.0/testing

Summary

Applied safe repository-guideline fixes in instrumentation/servlet/servlet-3.0/testing and committed them as 337802da.

Applied Changes

Testing

File: AbstractServlet3Test.java:38
Change: Registered `AutoCleanupExtension` and replaced manual `ExperimentalSnippetHolder.setSnippet("")` cleanup with `cleanup.deferCleanup(...)` immediately after setting the snippet.
Reason: The testing cleanup guideline prefers `AutoCleanupExtension.deferCleanup(...)` so cleanup runs after the test even when assertions fail; this also matches the existing servlet 5 testing pattern.

Correctness

File: AbstractTomcatServlet3Test.java:100
Change: Changed the Tomcat webapp child path from `"/webapps/ROOT"` to `"webapps/ROOT"`.
Reason: General correctness: an absolute child path passed to `new File(baseDir, ...)` ignores the JUnit `@TempDir`; using a relative path keeps the test webapp directory isolated under the per-test temp directory.

Module: servlet-5.0:javaagent

Module path: instrumentation/servlet/servlet-5.0/javaagent

Summary

Applied and committed safe review fixes for servlet-5.0 javaagent: replaced the stateless HttpServerResponseMutator enum singleton with direct new Servlet5HttpServerResponseMutator() usage.

Applied Changes

Style

File: Servlet5HttpServerResponseMutator.java:11
Change: Converted `Servlet5HttpServerResponseMutator` from an `enum` singleton to a regular `class` and removed `INSTANCE`.
Reason: Repository review guidance prefers direct instance creation over enum/static singletons for stateless telemetry interface implementations such as `HttpServerResponseMutator`, outside hot paths.

File: JakartaServletServiceAdvice.java:87
Change: Replaced `Servlet5HttpServerResponseMutator.INSTANCE` with `new Servlet5HttpServerResponseMutator()`.
Reason: Repository review guidance says stateless telemetry interface implementations should be instantiated directly at registration/initialization call sites instead of using singleton access.

Module: servlet-5.0:javaagent-unit-tests

Module path: instrumentation/servlet/servlet-5.0/javaagent-unit-tests

Summary

No safe repository-guideline fixes were needed under instrumentation/servlet/servlet-5.0/javaagent-unit-tests; scoped tests and build.gradle.kts already align with the loaded style, testing, Gradle, and mandatory metadata.yaml guidance.

Applied Changes

No safe automated changes were applied.

Module: servlet-5.0:jetty11-testing

Module path: instrumentation/servlet/servlet-5.0/jetty11-testing

Summary

Applied safe visibility fixes in jetty11-testing and committed them in c8a56cb5.

Applied Changes

Style

File: JettyServlet5Test.java:26
Change: Changed the `@RegisterExtension` `testing` field from `protected` to `private`.
Reason: Repository style guide requires static fields to use the most restrictive visibility; the field is not accessed by subclasses or other classes.

File: JettyServletHandlerTest.java:30
Change: Changed the `@RegisterExtension` `testing` field from `protected` to `private`.
Reason: Repository style guide requires static fields to use the most restrictive visibility; the field is only used by JUnit extension registration in the declaring class.

Module: servlet-5.0:jetty12-testing

Module path: instrumentation/servlet/servlet-5.0/jetty12-testing

Summary

No safe fixes were applied. One issue remains because the review workflow says not to auto-fix missing testExperimental task wiring when experimental flags are set unconditionally.

Applied Changes

No safe automated changes were applied.

Unresolved Items

File: build.gradle.kts
Reason: `tasks.test` unconditionally enables `-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter` without a dedicated `testExperimental` task; `testing-experimental-flags.md` says experimental flags should be isolated in `testExperimental`, but the review guidelines classify this as a non-auto-fix requiring manual follow-up.

Module: servlet-5.0:library

Module path: instrumentation/servlet/servlet-5.0/library

Summary

Applied and committed one safe repository-guideline fix for servlet-5.0 library nullability accuracy.

Applied Changes

Nullability

File: Servlet5Accessor.java:52
Change: Added `@Nullable` to `Servlet5Accessor` methods and the `setRequestAttribute` value parameter where Jakarta Servlet API calls can produce or accept `null`.
Reason: Repository nullability guidance requires `@Nullable` annotations where a concrete runtime source can pass or return `null`; methods such as `getQueryString()`, `getAttribute()`, `getHeader()`, `getPathInfo()`, and `getUserPrincipal()` may return `null`, and `setAttribute(..., null)` is allowed by the servlet API.

Module: servlet-5.0:testing

Module path: instrumentation/servlet/servlet-5.0/testing

Summary

Applied safe repository-guideline fixes in servlet-5.0:testing and committed them as ef3dcd1f.

Applied Changes

General

File: AbstractTomcatServlet5Test.java:100
Change: Changed Tomcat webapp setup to create `webapps/ROOT` under the JUnit `@TempDir`, and switched per-test cleanup to `TestAccessLogValve.clearLoggedIds()`.
Reason: Repository general correctness guidance requires fixing deterministic reliability issues; `new File(baseDir, "/webapps/ROOT")` ignores `baseDir`, and cleanup should use the synchronized accessor added for the shared access-log state.

File: TestAccessLogValve.java:29
Change: Made `getLoggedIds()` return a synchronized snapshot, added synchronized `clearLoggedIds()`, and made the wait timeout check depend on whether the expected IDs were observed.
Reason: Repository general correctness guidance calls for fixing concurrency hazards and reliability issues; `loggedIds` is written under synchronization, so reads and clears must use the same lock, and timeout handling should fail only when the expected entries are still absent.

Module: servlet-5.0:tomcat-testing

Module path: instrumentation/servlet/servlet-5.0/tomcat-testing

Summary

Applied one safe repository-guideline fix in tomcat-testing; the working tree is clean after committing the change.

Applied Changes

General

File: TomcatServlet5MappingTest.java:31
Change: Changed `new File(baseDir, "/webapps/ROOT")` to `new File(baseDir, "webapps/ROOT")` so the Tomcat webapp directory is created under the JUnit `@TempDir`.
Reason: Repository review guidelines require fixing correctness issues; an absolute child path makes `File` ignore `baseDir`, defeating temporary-directory isolation.

---

Download code review diagnostics