🤖feat: clickable chat text support and NMS decomposition#77
Open
PimvanderLoos wants to merge 3 commits into
Open
🤖feat: clickable chat text support and NMS decomposition#77PimvanderLoos wants to merge 3 commits into
PimvanderLoos wants to merge 3 commits into
Conversation
This was referenced May 13, 2026
There was a problem hiding this comment.
Pull request overview
Adds a new GET_PLAYER_CHAT_COMPONENTS agent action that exposes the raw JSON of outbound chat components so tests can assert on click events / hover text, and decomposes the large BotPlayerNmsAdapterV1_21_R7 into reusable NmsReflectionUtils and NmsValueExtractor helpers. Also fixes the 1.21.11 ComponentSerialization.CODEC lookup and adds a PlayerHandleAssert.hasClickableChatText() assertion.
Changes:
- New end-to-end path for draining outbound chat components as JSON (NMS adapter → agent action → gateway →
PlayerHandle.chatComponents()→ChatComponentSnapshot). - Extract reflection helpers (
NmsReflectionUtils) and recursive component/text extractor (NmsValueExtractor) fromBotPlayerNmsAdapterV1_21_R7; per-player message and component queues replace the per-callArrayList. - New assertion
hasClickableChatTextand broader test coverage (adapter unit tests, dispatcher tests, player action tests).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lightkeeper-runtime-core/.../AgentAction.java |
Adds GET_PLAYER_CHAT_COMPONENTS enum value. |
lightkeeper-nms-parent/lightkeeper-nms-api/.../IBotPlayerNmsAdapter.java |
Adds drainChatComponents(UUID) API method. |
lightkeeper-nms-parent/.../BotPlayerNmsAdapterV1_21_R7.java |
Adds component-codec lookup, per-player queues, JSON extraction, and delegates reflection helpers. |
lightkeeper-nms-parent/.../NmsReflectionUtils.java |
New shared reflection utility class. |
lightkeeper-nms-parent/.../NmsValueExtractor.java |
New shared recursive value extractor. |
lightkeeper-nms-parent/.../BotPlayerNmsAdapterV1_21_R7Test.java |
Adds tests for drains, component extraction, and reflection refactor. |
lightkeeper-agent-spigot/.../AgentRequestDispatcher.java |
Routes new action to player actions. |
lightkeeper-agent-spigot/.../AgentPlayerActions.java |
Adds handleGetPlayerChatComponents. |
lightkeeper-agent-spigot/.../AgentSyntheticPlayerStore.java |
Tracks per-player chat component history. |
lightkeeper-agent-spigot/.../AgentRequestDispatcherTest.java |
Tests dispatcher routing and validation behavior. |
lightkeeper-agent-spigot/.../AgentPlayerActionsTest.java |
Tests new player action. |
lightkeeper-framework-junit/.../IFrameworkGatewayView.java |
Adds playerChatComponents gateway method. |
lightkeeper-framework-junit/.../DefaultLightkeeperFramework.java |
Implements gateway delegation. |
lightkeeper-framework-junit/.../UdsAgentClient.java |
Sends new agent action and parses JSON response. |
lightkeeper-framework-junit/.../PlayerHandle.java |
Adds chatComponents() accessor. |
lightkeeper-framework-junit/.../ChatComponentSnapshot.java |
New record holding raw component JSON. |
lightkeeper-framework-junit/.../PlayerHandleAssert.java |
Adds hasClickableChatText assertion. |
lightkeeper-framework-junit/.../HandleAssertionsTest.java |
Tests new assertion success and failure cases. |
lightkeeper-framework-junit/.../PlayerHandleTest.java |
Tests chatComponents() and additional click delegation paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+97
to
+109
| public PlayerHandleAssert hasClickableChatText(String expectedText) | ||
| { | ||
| final var actual = nonNullActual(); | ||
| final boolean found = actual.chatComponents().stream() | ||
| .anyMatch(component -> component.json().contains(expectedText) && | ||
| component.json().contains("clickEvent")); | ||
| if (!found) | ||
| { | ||
| failWithMessage("Expected player '%s' to receive clickable chat text matching '%s', but none was found.", | ||
| actual.name(), expectedText); | ||
| } | ||
| return this; | ||
| } |
Comment on lines
+21
to
24
| import java.util.Optional; | ||
| import java.util.Locale; | ||
| import java.util.Map; | ||
| import java.util.Objects; |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds end-to-end capture and retrieval of synthetic-player chat components: reflection utilities and extractor, adapter draining and per-player queues, agent storage and request handling, framework gateway types and client, and tests/assertions. ChangesChat Component Capture and Retrieval
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/assertions/PlayerHandleAssert.java`:
- Around line 100-103: The assertion currently checks component.json() for
independent substrings expectedText and "clickEvent", which can false-positive;
update PlayerHandleAssert to parse each chat component JSON (from
actual.chatComponents()) into JSON objects and structurally verify that a text
node whose text equals or contains expectedText has an associated "clickEvent"
field (or that an element in "extra" with "text": expectedText has
"clickEvent"), rather than searching raw substrings; modify the anyMatch
predicate on component.json() to parse the JSON and inspect the object tree for
a text node matching expectedText and a clickEvent property on that same node.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.java`:
- Around line 172-173: Replace uses of ownerClass.getField(preferredName) in
NmsReflectionUtils with a declared-field lookup that walks the class hierarchy:
implement a helper (e.g. findDeclaredField(Class<?> type, String fieldName))
that iterates cursor = type; cursor != null; cursor = cursor.getSuperclass() and
calls cursor.getDeclaredField(fieldName) until found, then call
field.setAccessible(true) on the returned Field; update both occurrences (the
one currently using ownerClass.getField(preferredName) and the other at the
second occurrence) to use this helper to correctly find non-public fields in
superclasses.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java`:
- Around line 119-126: The loop in NmsValueExtractor (iterating over
value.getClass().getMethods()) applies the MAX_METHODS cap against a
non-deterministic Method order; to make accessor selection deterministic,
collect the Method[] from value.getClass().getMethods(), sort it (e.g. by method
name and signature or Method::toString) before the for-loop that uses
inspectedMethodCount and isSafeAccessor, then iterate the sorted array and apply
the existing MAX_METHODS logic so the same accessors are examined across runs.
- Around line 84-85: The code uses a shared 'seen' map for cycle detection and
does seen.put(value, Boolean.TRUE) with a permanent entry, causing sibling
branches to falsely skip objects; change to path-scoped detection by inserting
the value into 'seen' only for the duration of processing and removing it
afterward via try-finally: after checking null/depth, do a put-if-absent check,
then wrap the recursive/branch processing in try { ... } finally {
seen.remove(value); } so each traversal path unwinds its seen entry; apply this
pattern wherever 'seen.put(value, Boolean.TRUE) != null' appears (the checked
branch around variable 'value' in NmsValueExtractor and the similar checks
between lines 91-141).
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.java`:
- Around line 723-726: The tests use nondeterministic guards (System.nanoTime()
>= 0L) before throwing in the test doubles, causing flaky behavior; modify the
failing stubs in the classes referenced (FailingChannel and ThrowingCodec) so
they throw the intended exception unconditionally (remove the System.nanoTime()
check) — locate the throwing methods in FailingChannel and ThrowingCodec and
replace the conditional throw with a direct throw of the same
IllegalStateException (preserving the message "boom" or existing message) so the
stubs always fail deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 83569f89-9204-4cfd-91cd-c0d94b89df14
📒 Files selected for processing (19)
lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.javalightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcher.javalightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentSyntheticPlayerStore.javalightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.javalightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcherTest.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/ChatComponentSnapshot.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IFrameworkGatewayView.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/PlayerHandle.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/assertions/PlayerHandleAssert.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFramework.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/UdsAgentClient.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/PlayerHandleTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/assertions/HandleAssertionsTest.javalightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotPlayerNmsAdapter.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.javalightkeeper-runtime-core/src/main/java/nl/pim16aap2/lightkeeper/runtime/agent/AgentAction.java
Comment on lines
+100
to
+103
| final boolean found = actual.chatComponents().stream() | ||
| .anyMatch(component -> component.json().contains(expectedText) && | ||
| component.json().contains("clickEvent")); | ||
| if (!found) |
There was a problem hiding this comment.
Match clickable text structurally, not by independent substrings.
Current logic passes if expectedText and clickEvent appear anywhere in the same JSON string, even when the matched text itself is not clickable. This can yield false-positive assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/assertions/PlayerHandleAssert.java`
around lines 100 - 103, The assertion currently checks component.json() for
independent substrings expectedText and "clickEvent", which can false-positive;
update PlayerHandleAssert to parse each chat component JSON (from
actual.chatComponents()) into JSON objects and structurally verify that a text
node whose text equals or contains expectedText has an associated "clickEvent"
field (or that an element in "extra" with "text": expectedText has
"clickEvent"), rather than searching raw substrings; modify the anyMatch
predicate on component.json() to parse the JSON and inspect the object tree for
a text node matching expectedText and a clickEvent property on that same node.
Comment on lines
+172
to
+173
| final Field field = ownerClass.getField(preferredName); | ||
| field.setAccessible(true); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the NmsReflectionUtils.java file
find . -name "NmsReflectionUtils.java" -type fRepository: PimvanderLoos/LightKeeper
Length of output: 193
🏁 Script executed:
# Get the file size first to determine how to read it
wc -l lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.javaRepository: PimvanderLoos/LightKeeper
Length of output: 195
🏁 Script executed:
# Read the entire file to understand the context and structure
cat -n lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.javaRepository: PimvanderLoos/LightKeeper
Length of output: 17761
Use getDeclaredField() with hierarchy search for preferred-name field lookup.
getField(preferredName) only finds public fields. When the preferred name refers to a non-public field in the class hierarchy, the lookup fails and forces fragile type-based fallback matching, which can bind the wrong field in obfuscated NMS classes.
Suggested fix
- final Field field = ownerClass.getField(preferredName);
+ final Field field = findDeclaredField(ownerClass, preferredName);
field.setAccessible(true);
return field;private static Field findDeclaredField(Class<?> type, String fieldName) throws NoSuchFieldException {
for (Class<?> cursor = type; cursor != null; cursor = cursor.getSuperclass()) {
try {
return cursor.getDeclaredField(fieldName);
} catch (NoSuchFieldException ignored) {
// keep searching
}
}
throw new NoSuchFieldException(fieldName);
}Also applies to: 213-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.java`
around lines 172 - 173, Replace uses of ownerClass.getField(preferredName) in
NmsReflectionUtils with a declared-field lookup that walks the class hierarchy:
implement a helper (e.g. findDeclaredField(Class<?> type, String fieldName))
that iterates cursor = type; cursor != null; cursor = cursor.getSuperclass() and
calls cursor.getDeclaredField(fieldName) until found, then call
field.setAccessible(true) on the returned Field; update both occurrences (the
one currently using ownerClass.getField(preferredName) and the other at the
second occurrence) to use this helper to correctly find non-public fields in
superclasses.
Comment on lines
+84
to
+85
| if (value == null || depth < 0 || seen.put(value, Boolean.TRUE) != null) | ||
| return null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "NmsValueExtractor.java" -type fRepository: PimvanderLoos/LightKeeper
Length of output: 192
🏁 Script executed:
cat -n ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.javaRepository: PimvanderLoos/LightKeeper
Length of output: 6580
🏁 Script executed:
rg -r "NmsValueExtractor" --type java -B 2 -A 2Repository: PimvanderLoos/LightKeeper
Length of output: 466
🏁 Script executed:
rg "NmsValueExtractor" --type java -B 2 -A 2Repository: PimvanderLoos/LightKeeper
Length of output: 4649
🏁 Script executed:
rg "new IdentityHashMap" --type java -B 3 -A 3Repository: PimvanderLoos/LightKeeper
Length of output: 8080
🏁 Script executed:
wc -l ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.javaRepository: PimvanderLoos/LightKeeper
Length of output: 210
🏁 Script executed:
grep -n "cyclic\|shared\|false negative" ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.javaRepository: PimvanderLoos/LightKeeper
Length of output: 194
🏁 Script executed:
sed -n '105,135p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.javaRepository: PimvanderLoos/LightKeeper
Length of output: 803
🏁 Script executed:
sed -n '70,200p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.javaRepository: PimvanderLoos/LightKeeper
Length of output: 3946
🏁 Script executed:
sed -n '200,300p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.javaRepository: PimvanderLoos/LightKeeper
Length of output: 3938
🏁 Script executed:
grep -n "first matching\|find.*leaf\|shared" ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.java | head -20Repository: PimvanderLoos/LightKeeper
Length of output: 51
🏁 Script executed:
sed -n '1,100p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.javaRepository: PimvanderLoos/LightKeeper
Length of output: 4719
🏁 Script executed:
sed -n '200,350p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.javaRepository: PimvanderLoos/LightKeeper
Length of output: 5556
🏁 Script executed:
rg "extractText|extractComponentJson" ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.java -B 3 -A 10Repository: PimvanderLoos/LightKeeper
Length of output: 1847
🏁 Script executed:
sed -n '76,142p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.javaRepository: PimvanderLoos/LightKeeper
Length of output: 2571
Shared objects cause false negatives when visited before sufficient depth is available.
The seen map is never cleared between sibling branches, so once an object is added during one traversal path, it is permanently skipped in all subsequent paths within the same extraction call. This can hide valid leaves when a shared object is first encountered at insufficient depth.
Example: if object X is reachable via both methodA() and methodB() on the same parent, and methodA() is invoked first with remaining depth=2 but yields no leaf (or one filtered by skipBlank), then X is marked in seen. When methodB() is later invoked with remaining depth=3, the same object X is skipped entirely, even though the additional depth budget could have found a valid leaf.
Use path-scoped cycle detection with try-finally to unwind seen entries:
Suggested fix
- if (value == null || depth < 0 || seen.put(value, Boolean.TRUE) != null)
+ if (value == null || depth < 0 || seen.containsKey(value))
return null;
+ seen.put(value, Boolean.TRUE);
+ try
+ {
...
- return null;
+ return null;
+ }
+ finally
+ {
+ seen.remove(value);
+ }Also applies to: 91-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java`
around lines 84 - 85, The code uses a shared 'seen' map for cycle detection and
does seen.put(value, Boolean.TRUE) with a permanent entry, causing sibling
branches to falsely skip objects; change to path-scoped detection by inserting
the value into 'seen' only for the duration of processing and removing it
afterward via try-finally: after checking null/depth, do a put-if-absent check,
then wrap the recursive/branch processing in try { ... } finally {
seen.remove(value); } so each traversal path unwinds its seen entry; apply this
pattern wherever 'seen.put(value, Boolean.TRUE) != null' appears (the checked
branch around variable 'value' in NmsValueExtractor and the similar checks
between lines 91-141).
Comment on lines
+119
to
+126
| int inspectedMethodCount = 0; | ||
| for (final Method method : value.getClass().getMethods()) | ||
| { | ||
| if (!isSafeAccessor.test(method)) | ||
| continue; | ||
| if (inspectedMethodCount >= MAX_METHODS) | ||
| break; | ||
| ++inspectedMethodCount; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java | sed -n '100,140p'Repository: PimvanderLoos/LightKeeper
Length of output: 1958
🏁 Script executed:
cat -n lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java | head -50Repository: PimvanderLoos/LightKeeper
Length of output: 2533
🏁 Script executed:
rg "MAX_METHODS" lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.javaRepository: PimvanderLoos/LightKeeper
Length of output: 258
🏁 Script executed:
cat -n lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java | sed -n '35,46p'Repository: PimvanderLoos/LightKeeper
Length of output: 730
🌐 Web query:
Java Class.getMethods() order guarantee documentation
💡 Result:
Java Class.getMethods has no guaranteed order for the returned Method[] elements. The official Javadoc explicitly states: “The elements in the returned array are not sorted and are not in any particular order.” [1] Practical implication: you should not rely on the array ordering being stable or matching source/declaration order; if you need deterministic behavior, sort the returned Method[] yourself (e.g., by name, parameter types, or a signature string). [1]
Citations:
Sort methods before applying MAX_METHODS cap to ensure deterministic accessor selection.
Class.getMethods() returns elements in undefined order (per Java spec). Since the code limits accessor invocation to 24 methods, the non-deterministic iteration order causes which accessors get examined to vary across runs and JVM implementations. The design explicitly aims to find "the first" matching leaf (per the class docstring), which requires deterministic ordering. Sort methods by name or signature before enforcing the cap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java`
around lines 119 - 126, The loop in NmsValueExtractor (iterating over
value.getClass().getMethods()) applies the MAX_METHODS cap against a
non-deterministic Method order; to make accessor selection deterministic,
collect the Method[] from value.getClass().getMethods(), sort it (e.g. by method
name and signature or Method::toString) before the for-loop that uses
inspectedMethodCount and isSafeAccessor, then iterate the sorted array and apply
the existing MAX_METHODS logic so the same accessors are examined across runs.
Comment on lines
+723
to
+726
| if (System.nanoTime() >= 0L) | ||
| throw new IllegalStateException("boom"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Java, is System.nanoTime() guaranteed to be non-negative, or can it return negative values?
💡 Result:
System.nanoTime is not guaranteed to be non-negative; it can return negative values. Oracle’s JavaDoc states that nanoTime returns “nanoseconds since some fixed but arbitrary origin time (perhaps in the future, so values may be negative)” [1][2]. Therefore callers must not assume the returned long is >= 0. Practical note: for elapsed-time measurements, you should compute differences like end - start and treat the result as the elapsed duration; comparisons/elapsed computations are meaningful when you subtract two nanoTime values taken within the same JVM instance [1].
Citations:
- 1: https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/System.html
- 2: https://docs.oracle.com/javase/8/docs/api/java/lang/System.html
Make failure stubs deterministic by throwing unconditionally.
Both test doubles (FailingChannel and ThrowingCodec) gate exceptions behind System.nanoTime() >= 0L. Since System.nanoTime() can return negative values (per Oracle's JavaDoc), this condition is unreliable and causes non-deterministic test behavior. Remove the condition so these methods throw unconditionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.java`
around lines 723 - 726, The tests use nondeterministic guards (System.nanoTime()
>= 0L) before throwing in the test doubles, causing flaky behavior; modify the
failing stubs in the classes referenced (FailingChannel and ThrowingCodec) so
they throw the intended exception unconditionally (remove the System.nanoTime()
check) — locate the throwing methods in FailingChannel and ThrowingCodec and
replace the conditional throw with a direct throw of the same
IllegalStateException (preserving the message "boom" or existing message) so the
stubs always fail deterministically.
PimvanderLoos
force-pushed
the
split/03-inventory-events
branch
from
c9a325b to
dfe6b23
Compare
PimvanderLoos
force-pushed
the
split/04-chat-components
branch
from
49399da to
1c752fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcherTest.java`:
- Around line 453-483: The tests call AgentRequestDispatcher.handleRequestLine
with ClearCapturedEventsCommand and UnregisterEventListenerCommand using blank
class names but only assert success; update each test to also verify that the
underlying capture/unregister mocks were NOT invoked (i.e., the blank class name
is ignored). Locate the DispatcherFixture used in the tests and add assertions
using your mocking framework (e.g., Mockito.verifyNoInteractions(...) or
Mockito.verify(..., never())) against the mock(s) that perform event
capture/unregister (the fixture's event capture service or event listener
registry) so that for requests constructed with new
ClearCapturedEventsCommand(..., "") and new UnregisterEventListenerCommand(...,
"") no capture or unregister method calls occurred.
In
`@lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/PlayerHandleTest.java`:
- Around line 294-302: The test
leftClickBlock_shouldDelegateWithCoordinatesUsingDefaultBlockFace in
PlayerHandleTest is too loose: it verifies only PLAYER_UUID and "UP" while using
any(Vector3Di.class), so incorrect coordinates can pass; update the test to
capture or assert the actual Vector3Di passed to frameworkGateway.leftClickBlock
(e.g., use an ArgumentCaptor<Vector3Di> or eq(new Vector3Di(5,64,6))) and assert
its x/y/z equal 5/64/6, and do the same tightening for the other overload test
around lines 319-327 that currently accepts any(Vector3Di.class).
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.java`:
- Around line 197-217: resolveComponentJsonCodec currently uses
getField/getFields and fails if the codec field is non-public; change the lookup
to use getDeclaredField first and fall back to iterating getDeclaredFields so
non-public static fields matching codecClass are discovered, call
setAccessible(true) before reading the field value, and keep the existing
NoSuchFieldException behavior (throw if nothing matches). Update references in
the method resolveComponentJsonCodec(Class<?> componentSerializationClass,
Class<?> codecClass) to use getDeclaredField/getDeclaredFields instead of
getField/getFields and preserve the static/type checks and field.get(null)
access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70a11e81-a9ba-458d-83cf-a3a9a05fac58
📒 Files selected for processing (18)
lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.javalightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcher.javalightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentSyntheticPlayerStore.javalightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.javalightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcherTest.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/ChatComponentSnapshot.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IFrameworkGatewayView.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/PlayerHandle.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/assertions/PlayerHandleAssert.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFramework.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/UdsAgentClient.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/PlayerHandleTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/assertions/HandleAssertionsTest.javalightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotPlayerNmsAdapter.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.java
Comment on lines
+453
to
+483
| @Test | ||
| void handleRequestLine_shouldSucceedClearCapturedEventsWithBlankClassName() | ||
| throws Exception | ||
| { | ||
| // setup | ||
| final DispatcherFixture fixture = createDispatcherFixture(); | ||
| final String requestLine = toJson(new ClearCapturedEventsCommand("request-clear-blank", "")); | ||
|
|
||
| // execute | ||
| final AgentRequestDispatcher.RequestDispatchResult result = | ||
| fixture.dispatcher().handleRequestLine(requestLine, true); | ||
|
|
||
| // verify - blank is silently ignored, operation succeeds | ||
| assertThat(result.response().success()).isTrue(); | ||
| } | ||
|
|
||
| @Test | ||
| void handleRequestLine_shouldSucceedUnregisterEventListenerWithBlankClassName() | ||
| throws Exception | ||
| { | ||
| // setup | ||
| final DispatcherFixture fixture = createDispatcherFixture(); | ||
| final String requestLine = toJson(new UnregisterEventListenerCommand("request-unregister-blank", "")); | ||
|
|
||
| // execute | ||
| final AgentRequestDispatcher.RequestDispatchResult result = | ||
| fixture.dispatcher().handleRequestLine(requestLine, true); | ||
|
|
||
| // verify - blank is silently ignored, operation succeeds | ||
| assertThat(result.response().success()).isTrue(); | ||
| } |
There was a problem hiding this comment.
Assert that blank class names are truly ignored (no event-capture call).
Line 465 and Line 481 state blank values are ignored, but the tests only assert success. Add non-invocation checks so this contract is enforced.
Suggested assertion additions
void handleRequestLine_shouldSucceedClearCapturedEventsWithBlankClassName()
throws Exception
@@
assertThat(result.response().success()).isTrue();
+ verify(fixture.eventCapture(), never()).clearCapturedEvents(anyString());
}
@@
void handleRequestLine_shouldSucceedUnregisterEventListenerWithBlankClassName()
throws Exception
@@
assertThat(result.response().success()).isTrue();
+ verify(fixture.eventCapture(), never()).unregisterListener(anyString());
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcherTest.java`
around lines 453 - 483, The tests call AgentRequestDispatcher.handleRequestLine
with ClearCapturedEventsCommand and UnregisterEventListenerCommand using blank
class names but only assert success; update each test to also verify that the
underlying capture/unregister mocks were NOT invoked (i.e., the blank class name
is ignored). Locate the DispatcherFixture used in the tests and add assertions
using your mocking framework (e.g., Mockito.verifyNoInteractions(...) or
Mockito.verify(..., never())) against the mock(s) that perform event
capture/unregister (the fixture's event capture service or event listener
registry) so that for requests constructed with new
ClearCapturedEventsCommand(..., "") and new UnregisterEventListenerCommand(...,
"") no capture or unregister method calls occurred.
Comment on lines
+294
to
+302
| void leftClickBlock_shouldDelegateWithCoordinatesUsingDefaultBlockFace() | ||
| { | ||
| // execute | ||
| final PlayerHandle result = playerHandle.leftClickBlock(5, 64, 6); | ||
|
|
||
| // verify | ||
| assertThat(result).isSameAs(playerHandle); | ||
| verify(frameworkGateway).leftClickBlock(eq(PLAYER_UUID), any(Vector3Di.class), eq("UP")); | ||
| } |
There was a problem hiding this comment.
Verify forwarded coordinates in overload delegation tests.
Line 301 and Line 326 only assert UUID + "UP" and accept any Vector3Di, so incorrect coordinate forwarding would still pass.
Suggested test tightening
- verify(frameworkGateway).leftClickBlock(eq(PLAYER_UUID), any(Vector3Di.class), eq("UP"));
+ verify(frameworkGateway).leftClickBlock(PLAYER_UUID, new Vector3Di(5, 64, 6), "UP");
...
- verify(frameworkGateway).rightClickBlock(eq(PLAYER_UUID), any(Vector3Di.class), eq("UP"));
+ verify(frameworkGateway).rightClickBlock(PLAYER_UUID, new Vector3Di(9, 64, 10), "UP");Also applies to: 319-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/PlayerHandleTest.java`
around lines 294 - 302, The test
leftClickBlock_shouldDelegateWithCoordinatesUsingDefaultBlockFace in
PlayerHandleTest is too loose: it verifies only PLAYER_UUID and "UP" while using
any(Vector3Di.class), so incorrect coordinates can pass; update the test to
capture or assert the actual Vector3Di passed to frameworkGateway.leftClickBlock
(e.g., use an ArgumentCaptor<Vector3Di> or eq(new Vector3Di(5,64,6))) and assert
its x/y/z equal 5/64/6, and do the same tightening for the other overload test
around lines 319-327 that currently accepts any(Vector3Di.class).
Comment on lines
+197
to
217
| private static Object resolveComponentJsonCodec(Class<?> componentSerializationClass, Class<?> codecClass) | ||
| throws ReflectiveOperationException | ||
| { | ||
| try | ||
| { | ||
| return Class.forName(className, false, classLoader); | ||
| return componentSerializationClass.getField("CODEC").get(null); | ||
| } | ||
| catch (ClassNotFoundException ignored) | ||
| { | ||
| return Class.forName(className); | ||
| } | ||
| } | ||
|
|
||
| private static Class<?> resolveFirstClass(ClassLoader classLoader, String... classNames) | ||
| throws ClassNotFoundException | ||
| { | ||
| ClassNotFoundException lastException = null; | ||
| for (final String className : classNames) | ||
| catch (NoSuchFieldException ignored) | ||
| { | ||
| try | ||
| { | ||
| return resolveClass(className, classLoader); | ||
| } | ||
| catch (ClassNotFoundException exception) | ||
| for (final Field field : componentSerializationClass.getFields()) | ||
| { | ||
| lastException = exception; | ||
| if (!Modifier.isStatic(field.getModifiers()) || !codecClass.isAssignableFrom(field.getType())) | ||
| continue; | ||
|
|
||
| field.setAccessible(true); | ||
| return field.get(null); | ||
| } | ||
| throw new NoSuchFieldException( | ||
| "Failed to resolve component JSON codec field on " + componentSerializationClass.getName() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "BotPlayerNmsAdapterV1_21_R7.java" -type fRepository: PimvanderLoos/LightKeeper
Length of output: 202
🏁 Script executed:
sed -n '197,217p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.javaRepository: PimvanderLoos/LightKeeper
Length of output: 900
🏁 Script executed:
# Check if there are other version adapters and how they handle codec resolution
find . -name "*.java" -path "*/lightkeeper-nms*" -type f | xargs grep -l "resolveComponentJsonCodec" | head -5Repository: PimvanderLoos/LightKeeper
Length of output: 202
🏁 Script executed:
# Find other NMS adapter files
find ./lightkeeper-nms-parent -name "BotPlayerNmsAdapter*.java" -type f | head -10Repository: PimvanderLoos/LightKeeper
Length of output: 339
🏁 Script executed:
# Find where resolveComponentJsonCodec is called
rg "resolveComponentJsonCodec" ./lightkeeper-nms-parent --context 3Repository: PimvanderLoos/LightKeeper
Length of output: 2742
🏁 Script executed:
# Check the test file to see if codec resolution is tested
cat -n ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.javaRepository: PimvanderLoos/LightKeeper
Length of output: 35744
🏁 Script executed:
# Check how adapter uses NMS reflection and if there are patterns for non-public field access
head -100 ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.javaRepository: PimvanderLoos/LightKeeper
Length of output: 4719
🏁 Script executed:
# Check if NmsReflectionUtils or other utility methods handle non-public field access
grep -r "setAccessible" ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java --context 2 | head -30Repository: PimvanderLoos/LightKeeper
Length of output: 3989
🏁 Script executed:
# Check NmsReflectionUtils to see patterns for field discovery in this codebase
sed -n '1,100p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.javaRepository: PimvanderLoos/LightKeeper
Length of output: 3311
🏁 Script executed:
# Check how NmsReflectionUtils handles field resolution - especially getDeclaredFields patterns
grep -n "getDeclaredField\|getField\|getDeclaredFields\|getFields" ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.javaRepository: PimvanderLoos/LightKeeper
Length of output: 355
🏁 Script executed:
# Check the methods that use getDeclaredFields to understand pattern
sed -n '200,235p' ./lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.javaRepository: PimvanderLoos/LightKeeper
Length of output: 1239
Use getDeclaredField lookup for codec resolution to avoid brittle startup failures.
resolveComponentJsonCodec() uses public-only field lookup (getField() and getFields()). If the codec field is non-public (as with Minecraft's obfuscated internals), initialization fails even though the member exists. The fallback iteration should use getDeclaredFields() to inspect all fields regardless of visibility, consistent with patterns already established in NmsReflectionUtils.resolveFieldByNameOrAcceptedType().
Suggested patch
private static Object resolveComponentJsonCodec(Class<?> componentSerializationClass, Class<?> codecClass)
throws ReflectiveOperationException
{
try
{
- return componentSerializationClass.getField("CODEC").get(null);
+ final Field codec = componentSerializationClass.getDeclaredField("CODEC");
+ codec.setAccessible(true);
+ return codec.get(null);
}
catch (NoSuchFieldException ignored)
{
- for (final Field field : componentSerializationClass.getFields())
+ for (final Field field : componentSerializationClass.getDeclaredFields())
{
if (!Modifier.isStatic(field.getModifiers()) || !codecClass.isAssignableFrom(field.getType()))
continue;
field.setAccessible(true);
return field.get(null);
}
throw new NoSuchFieldException(
"Failed to resolve component JSON codec field on " + componentSerializationClass.getName()
);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.java`
around lines 197 - 217, resolveComponentJsonCodec currently uses
getField/getFields and fails if the codec field is non-public; change the lookup
to use getDeclaredField first and fall back to iterating getDeclaredFields so
non-public static fields matching codecClass are discovered, call
setAccessible(true) before reading the field value, and keep the existing
NoSuchFieldException behavior (throw if nothing matches). Update references in
the method resolveComponentJsonCodec(Class<?> componentSerializationClass,
Class<?> codecClass) to use getDeclaredField/getDeclaredFields instead of
getField/getFields and preserve the static/type checks and field.get(null)
access.
Adds GET_PLAYER_CHAT_COMPONENTS so tests can retrieve the raw JSON chat
components delivered to a synthetic player, enabling assertions on
clickable/hoverable text. Also decomposes BotPlayerNmsAdapterV1_21_R7
(1027 lines) into smaller focused units to keep it maintainable.
Changes:
- AgentAction: add GET_PLAYER_CHAT_COMPONENTS
- AgentSyntheticPlayerStore: add componentHistory field
- AgentPlayerActions: add handleGetPlayerChatComponents (drains via NMS)
- AgentRequestDispatcher: wire GET_PLAYER_CHAT_COMPONENTS
- IBotPlayerNmsAdapter: add drainChatComponents(UUID)
- BotPlayerNmsAdapterV1_21_R7: add drainChatComponents; fix 1.21.11
ComponentSerializer lookup; refactored to delegate to:
- NmsReflectionUtils (13 generic reflection helpers, ~280 lines)
- NmsValueExtractor (parameterised recursive extractor shared by
extractText and extractComponentJson)
- ChatComponentSnapshot: new record wrapping component JSON
- IFrameworkGatewayView / DefaultLightkeeperFramework / UdsAgentClient:
add client-side playerChatComponents()
- PlayerHandle: add chatComponents() returning List<ChatComponentSnapshot>
- PlayerHandleAssert: add hasClickableChatText() with proper JSON parse
(uses ObjectMapper.readTree to check clickEvent key, not substring)
Tests:
- BotPlayerNmsAdapterV1_21_R7Test (19 tests — drainChatComponents, NMS)
- HandleAssertionsTest: hasClickableChatText assertions
- PlayerHandleTest: chatComponents delegation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Port IllegalArgumentException→INVALID_ARGUMENT mapping and blank eventClassName validation from the old Map-based dispatcher to the typed-command dispatcher. Adapt AgentPlayerActionsTest and AgentRequestDispatcherTest to use typed protocol commands instead of the old AgentAction enum. Part of the split/04-chat-components rebase onto split/03-inventory-events. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PimvanderLoos
force-pushed
the
split/03-inventory-events
branch
from
dfe6b23 to
a3d4c2a
Compare
PimvanderLoos
force-pushed
the
split/04-chat-components
branch
from
1c752fa to
04fe72f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.java (1)
172-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreferred-name field lookup should walk declared fields in hierarchy.
At Line 172 and Line 213,
getField(...)only checks public fields, which can bypass the intended field and force brittle fallback matching.Proposed fix
- final Field field = ownerClass.getField(preferredName); + final Field field = findDeclaredField(ownerClass, preferredName); field.setAccessible(true); return field; @@ - final Field field = ownerClass.getField(preferredName); + final Field field = findDeclaredField(ownerClass, preferredName); field.setAccessible(true); return field;+ private static Field findDeclaredField(Class<?> type, String fieldName) throws NoSuchFieldException + { + for (Class<?> cursor = type; cursor != null; cursor = cursor.getSuperclass()) + { + try + { + return cursor.getDeclaredField(fieldName); + } + catch (NoSuchFieldException ignored) + { + // Continue searching in super class. + } + } + throw new NoSuchFieldException(fieldName); + }Also applies to: 213-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.java` around lines 172 - 173, The lookup using ownerClass.getField(preferredName) in NmsReflectionUtils only finds public fields and should instead walk the class hierarchy checking declared fields: replace the getField calls with a loop that starts at ownerClass and iterates owner = owner.getSuperclass() until null, calling owner.getDeclaredField(preferredName) and, if found, calling setAccessible(true) and returning that Field; do this for both occurrences referenced (the lookup at the block using preferredName and the similar one around line 213) so private/protected/package fields in superclasses are correctly discovered.lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java (2)
119-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake accessor selection deterministic before applying
MAX_METHODS.At Lines 119-126,
getMethods()order is unspecified, so the capped scan can return different leaves across JVMs/runs.Proposed fix
+import java.util.Arrays; +import java.util.Comparator; @@ - int inspectedMethodCount = 0; - for (final Method method : value.getClass().getMethods()) + int inspectedMethodCount = 0; + final Method[] methods = value.getClass().getMethods(); + Arrays.sort(methods, Comparator.comparing(Method::toGenericString)); + for (final Method method : methods) { if (!isSafeAccessor.test(method)) continue;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java` around lines 119 - 126, The loop over value.getClass().getMethods() uses unspecified JVM order so applying the MAX_METHODS cap yields nondeterministic accessor selection; before iterating, produce a deterministic ordering (e.g. sort the Method[] by Method::getName and tie-breakers like parameter types/return type or declaring class) and then apply the isSafeAccessor test and inspectedMethodCount limit; update the loop that references value.getClass().getMethods(), isSafeAccessor, MAX_METHODS, and inspectedMethodCount to iterate over the sorted list so selection is stable across runs.
84-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse path-scoped cycle tracking to avoid false negatives.
At Line 84,
seen.put(...)is never unwound, so shared nodes reached via later siblings can be skipped even when they would succeed on that branch.Proposed fix
- if (value == null || depth < 0 || seen.put(value, Boolean.TRUE) != null) + if (value == null || depth < 0 || seen.containsKey(value)) return null; + seen.put(value, Boolean.TRUE); + try + { final String leaf = leafExtractor.apply(value); if (leaf != null) return (!skipBlank || !leaf.isBlank()) ? leaf : null; @@ - return null; + return null; + } + finally + { + seen.remove(value); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java` around lines 84 - 141, The current cycle tracking uses seen.put(...) at the top and never unwinds it, causing shared nodes to be skipped across different sibling branches; modify extract(...) so it first checks if seen.containsKey(value) and returns null if present, then immediately before descending into children (for Optional, Collection, array, and reflective-access branches) insert seen.put(value, Boolean.TRUE) and wrap the entire child-exploration logic in a try/finally that removes the entry in finally (seen.remove(value)); ensure this change touches the extract method and preserves the leafExtractor/isSafeAccessor/skipBlank logic and MAX_METHODS behavior so each recursion uses path-scoped marking rather than a permanent global mark.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.java`:
- Around line 172-173: The lookup using ownerClass.getField(preferredName) in
NmsReflectionUtils only finds public fields and should instead walk the class
hierarchy checking declared fields: replace the getField calls with a loop that
starts at ownerClass and iterates owner = owner.getSuperclass() until null,
calling owner.getDeclaredField(preferredName) and, if found, calling
setAccessible(true) and returning that Field; do this for both occurrences
referenced (the lookup at the block using preferredName and the similar one
around line 213) so private/protected/package fields in superclasses are
correctly discovered.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.java`:
- Around line 119-126: The loop over value.getClass().getMethods() uses
unspecified JVM order so applying the MAX_METHODS cap yields nondeterministic
accessor selection; before iterating, produce a deterministic ordering (e.g.
sort the Method[] by Method::getName and tie-breakers like parameter
types/return type or declaring class) and then apply the isSafeAccessor test and
inspectedMethodCount limit; update the loop that references
value.getClass().getMethods(), isSafeAccessor, MAX_METHODS, and
inspectedMethodCount to iterate over the sorted list so selection is stable
across runs.
- Around line 84-141: The current cycle tracking uses seen.put(...) at the top
and never unwinds it, causing shared nodes to be skipped across different
sibling branches; modify extract(...) so it first checks if
seen.containsKey(value) and returns null if present, then immediately before
descending into children (for Optional, Collection, array, and reflective-access
branches) insert seen.put(value, Boolean.TRUE) and wrap the entire
child-exploration logic in a try/finally that removes the entry in finally
(seen.remove(value)); ensure this change touches the extract method and
preserves the leafExtractor/isSafeAccessor/skipBlank logic and MAX_METHODS
behavior so each recursion uses path-scoped marking rather than a permanent
global mark.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4272112b-8c29-410a-bce9-f5aaac34f152
📒 Files selected for processing (18)
lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.javalightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcher.javalightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentSyntheticPlayerStore.javalightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.javalightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcherTest.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/ChatComponentSnapshot.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IFrameworkGatewayView.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/PlayerHandle.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/assertions/PlayerHandleAssert.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFramework.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/UdsAgentClient.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/PlayerHandleTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/assertions/HandleAssertionsTest.javalightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotPlayerNmsAdapter.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsValueExtractor.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7Test.java
PimvanderLoos
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Plugins that open clickable chat menus need test coverage. The raw
component JSON must be inspectable so assertions can verify link targets
and hover text without launching a full client.
Also uses this PR to decompose
BotPlayerNmsAdapterV1_21_R7(1027 lines)before it becomes unmanageable — all the reflection utilities have zero
NMS coupling and duplicate logic across text/component extraction.
How
AgentAction: addGET_PLAYER_CHAT_COMPONENTSAgentPlayerActions.handleGetPlayerChatComponents: drains via NMSIBotPlayerNmsAdapter.drainChatComponents(UUID): contract in APIBotPlayerNmsAdapterV1_21_R7: fix 1.21.11 ComponentSerializer lookup;extract into:
NmsReflectionUtils— 13 generic reflection helpers (~280 lines)NmsValueExtractor— parameterised recursive value extractor sharedby
extractTextandextractComponentJsonPlayerHandleAssert.hasClickableChatText(): parses JSON viaObjectMapper.readTreeto check forclickEventkey (not substring)Tests
BotPlayerNmsAdapterV1_21_R7Test— 19 testsHandleAssertionsTest— clickable chat assertionsPart 4 of 6. Targets #76 (split/03-inventory-events).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests