Skip to content

Commit 101ff67

Browse files
committed
Improve code-review-and-fix agent: shared-module safety checks
- Expand sibling-validation rule from :testing-only to all shared/common modules (directory names containing -common, :library, :bootstrap, testing-common) — run :check for all sibling javaagent/library modules after modifying a shared module - Add visibility-reduction guard for shared modules: search sibling callers before reducing access; fix only when no sibling references the member, report otherwise to prevent IllegalAccessError in ByteBuddy advice
1 parent 9061ef6 commit 101ff67

1 file changed

Lines changed: 26 additions & 9 deletions

File tree

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

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,15 @@ Auto-fix boundaries:
139139

140140
- Safe to fix:
141141
- import cleanup or direct style-guide conformance
142+
**Extra step for shared/common modules**: when the modified file resides in a module
143+
whose directory name contains `-common`, or whose Gradle path ends with `:testing`,
144+
`:library`, or `:bootstrap`, **first search all sibling modules** under the same
145+
instrumentation parent for callers of the type or member before applying the fix.
146+
If no sibling module references it, the visibility reduction is safe — apply it.
147+
If any sibling module calls it — regardless of whether that caller shares the same
148+
Java package — do **not** reduce visibility; report the finding instead. Advice
149+
classes that call shared helpers are a common source of `IllegalAccessError` at
150+
runtime even when javac cross-module compilation succeeds.
142151
- normalization of existing `@SuppressWarnings` syntax or placement, but preserve any
143152
accurate explanatory comment attached to the suppression instead of deleting it as
144153
style noise
@@ -397,22 +406,30 @@ Execute these steps strictly in order — do not reorder:
397406
pre-existing failure — note it in the final output but do not block the commit.
398407
5. Never commit code that fails tests you can reproduce locally.
399408

400-
**Testing-module dependent validation**: when any modified module is a `testing` module
401-
(its Gradle path ends with `:testing`), you must **also** run `:check` (both normal and
402-
`-PtestLatestDeps=true`) for every sibling `library` and `javaagent` module under the
403-
same instrumentation parent. `testing` modules contain shared abstract test base classes
404-
consumed by those siblings — changes to visibility, method signatures, or class structure
405-
in the `testing` module can break compilation or tests in dependent modules.
406-
407-
To find siblings, list the parent directory of the `testing` module and look for
409+
**Shared-module dependent validation**: when any modified module is a shared module
410+
consumed by sibling instrumentation modules, you must **also** run `:check` (both normal
411+
and `-PtestLatestDeps=true`) for every sibling `library` and `javaagent` module under the
412+
same instrumentation parent. A module is shared if its Gradle path ends with `:testing`,
413+
or its directory name contains `-common` (e.g., `couchbase-2-common`, `netty-common`),
414+
or it is named `library`, `bootstrap`, or `testing-common`. Changes to visibility,
415+
method signatures, or class structure in a shared module can break compilation or tests
416+
in dependent sibling modules — including failures that compile cleanly but throw
417+
`IllegalAccessError` at runtime inside ByteBuddy advice.
418+
419+
To find siblings, list the parent directory of the shared module and look for
408420
`library/`, `javaagent/`, and any version-variant directories that contain `library/`
409-
or `javaagent/` submodules. Run `:check` for each.
421+
or `javaagent/` submodules. Also check `settings.gradle.kts` for every module under
422+
the same instrumentation group. Run `:check` for each sibling that transitively
423+
depends on the modified shared module.
410424

411425
Example: if you modify files in
412426
`:instrumentation:foo:foo-1.0:testing`, also run `:check` for
413427
`:instrumentation:foo:foo-1.0:library`,
414428
`:instrumentation:foo:foo-1.0:javaagent`, and any version-variant siblings such as
415429
`:instrumentation:foo:foo-2.0:library` if it depends on the `foo-1.0:testing` module.
430+
Likewise, if you modify files in `:instrumentation:couchbase:couchbase-2-common:javaagent`,
431+
also run `:check` for `:instrumentation:couchbase:couchbase-2.0:javaagent` and
432+
`:instrumentation:couchbase:couchbase-2.6:javaagent`.
416433

417434
Do not move on to step 2 until every required `:check` run from this step, including
418435
sibling-module validation and any re-runs after fixes or reverts, has fully completed

0 commit comments

Comments
 (0)