🤖 feat: full-login bot driver over real TCP loopback (LK-18, protocol v10)#100
Merged
Conversation
…col.VERSION to 10 Goal: give the wire protocol the vocabulary the S6 FULL_LOGIN driver (LK-18) needs before the nms/agent/framework layers can route by join mode. How this slots into S6/LK-18: - New top-level JoinMode enum (FULL_LOGIN | LEGACY_SPAWN), serialized by name. - CreatePlayer.Command gains joinMode + @nullable locale (LK-12); uuid becomes @nullable and is validated null under FULL_LOGIN (server derives the offline UUID from the name) and required under LEGACY_SPAWN. locale must be non-blank when present. - RuntimeProtocol.VERSION 9->10 so framework/agent handshake enforces the matched pair. - New AgentErrorCode.PLAYER_JOIN_DENIED / PLAYER_JOIN_TIMEOUT for the agent to surface login denials and timeouts as typed errors. - Serialization + validation tests extended for the joinMode/locale/uuid-null rules; the reflective everyCommand_shouldRejectBlankRequestId guard picks up the new fields. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
…roxy over client listeners) Goal: implement the S6/LK-18 FULL_LOGIN mechanism — drive the real vanilla login pipeline over loopback TCP so bots fire AsyncPreLogin/Login/Join and get a server-derived offline UUID, behind a swappable interface. How this slots into S6/LK-18: - nms-api: new IBotLoginDriver seam (Bukkit/JDK-typed only, so an MCProtocolLib transport could replace it) + BotLoginRequest / IBotLoginOutcome (Joined | Denied | TimedOut) / BotJoinPhase. IBotPlayerNmsAdapter gains loginDriver(). - nms-v1_21_R7: BotLoginReflection resolves the whole login surface fully reflectively with per-distribution fallbacks (Connection/NetworkManager, ConnectionProtocol/ EnumProtocol, obfuscated login+game packet classes vs shared common/configuration names, EventLoopGroupHolder, LoginProtocols/ConfigurationProtocols/GameProtocols resolved structurally because Spigot obfuscates the static field names, game ProtocolInfo bound via RegistryFriendlyByteBuf.decorator + a synthesized GameProtocols.Context). BotLoginSession is the JDK Proxy's InvocationHandler, dispatching on the packet class (never the method name, since Spigot obfuscates every method to 'a') and driving the login -> configuration -> play phase machine (compression, login-ack, known-packs echo, ClientInformation/locale LK-12, code-of-conduct, config-finish, keepalive/ping, teleport-ack). Fail-loud on unhandled login/config packets (naming the class); warn-once in play; kicks surfaced as Denied. - Legacy placeNewPlayer spawn path is untouched and remains the LEGACY_SPAWN route. - BotLoginPipelineDriver resolves the surface once and runs an independent session per login; wired lazily from the adapter so legacy-spawn-only usage never pays for it. - Unit tests cover the pure, server-decoupled helpers (structural method/enum resolution, offline-UUID scheme, request validation); the live pipeline is covered by the ITs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
…th bounded timeout; kicks->typed errors Goal: turn the S6/LK-18 driver into a real join on the agent side, so FULL_LOGIN bots join through the pipeline and denials/timeouts become typed errors, while LEGACY_SPAWN keeps today's fast path. How this slots into S6/LK-18: - handleCreatePlayer branches on joinMode. LEGACY_SPAWN keeps the existing main-thread reflective spawn (now asserting the non-null UUID the protocol guarantees). - FULL_LOGIN orchestrates the join off the main thread (the handler already runs on a per-connection virtual thread): registers a one-shot PlayerJoinEvent listener filtered to the bot name, invokes botLoginDriver.login(name, serverPort, locale, timeout), and awaits the join latch. Denied -> AgentProtocolException(PLAYER_JOIN_DENIED, kick reason); TimedOut / no join event -> PLAYER_JOIN_TIMEOUT. On success it registers the joined Player, applies health/permissions, and returns the same CreatePlayer.Response shape carrying the server-derived offline UUID. - The bounded timeout reuses the agent's configured sync-operation timeout, now exposed on AgentMainThreadExecutor so one knob governs how long the agent blocks before the client watchdog fires. - FULL_LOGIN bots are real Players, so every other AgentPlayerActions handler works unchanged and stays pure Bukkit (swappability invariant preserved). - Tests: existing spawn tests pin LEGACY_SPAWN; new test asserts a login denial surfaces as AgentProtocolException(PLAYER_JOIN_DENIED). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
… BotJoin{Denied,Timeout}Exception
Goal: expose the S6/LK-18 full-login path to consumers as an opt-in on the bot builder,
while keeping the default legacy spawn, so tests can join real-login bots and assert
denials/timeouts as typed errors.
How this slots into S6/LK-18:
- IPlayerBuilder gains withLocale(String) (LK-12; validated non-blank) and a fullLogin()
join-mode selector. The default stays LEGACY_SPAWN (flipping the default is a PR2/design
decision), and bots().join(...) keeps LEGACY_SPAWN.
- The join blocks until joined because the agent withholds the CreatePlayer response until
PlayerJoinEvent; no new framework wait loop is needed. Under FULL_LOGIN the wire command
carries no UUID (the server derives the offline UUID from the name); the response UUID is
adopted as the handle's UUID as today.
- New public BotJoinDeniedException / BotJoinTimeoutException; UdsAgentTransport maps the
PLAYER_JOIN_DENIED / PLAYER_JOIN_TIMEOUT wire codes to them at the single failure choke
point so every other command keeps its existing IllegalStateException behavior.
- IFrameworkGatewayView is unchanged (the builder path does not flow through it).
- joinMode/locale are threaded builder -> framework -> BotsFacade -> UdsAgentClient; the
BotsFacade unit test pins the LEGACY_SPAWN default.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
… protocol switch
Goal: make the S6/LK-18 full-login driver actually complete the pipeline on both Paper
and Spigot. Both fixes were surfaced by the mandated two-distro IT gate (A3/A4), each as a
named, local failure — the fail-loud policy working as designed.
How this slots into S6/LK-18:
- ServerboundLoginAcknowledged/FinishConfiguration/AcceptCodeOfConduct are codec
singletons: the IdDispatchCodec encodes them by identity against the registered
INSTANCE, so a freshly constructed instance fails to encode ("Can't encode ..., expected
..."). Resolve and send the INSTANCE field (by type, since Spigot obfuscates its name).
- setupInboundProtocol validates that the listener's protocol() already matches the
protocol it is being switched to, so the Proxy must report the new phase before the
transition. Advance phase to CONFIGURATION/PLAY before calling enterConfigurationPhase/
enterPlayPhase (was: after).
- Surface the pipeline's root-cause message on the wire so future driver gaps are named at
the framework boundary, not only in the server log.
Verified: the two full-login smoke tests pass on both the Paper and Spigot integration lanes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
…ro gate) Goal: prove the S6/LK-18 FULL_LOGIN driver end-to-end on both distributions and guard the default legacy-spawn path, satisfying the A3/A4 two-distro-before-merge gate. How this slots into S6/LK-18: - fullLogin_shouldJoinThroughRealLoginPipelineOnBothDistros: builder .fullLogin() join; registers AsyncPlayerPreLoginEvent + PlayerJoinEvent captures BEFORE joining and asserts both fired (real login), and asserts the handle carries the server-derived offline UUID. - legacySpawn_shouldStillSpawnBotsAfterDriverIntroduction: regression guard that the default builder path still spawns a usable, driveable bot after the driver's introduction. - Both run automatically on the integration-paper and integration-spigot Failsafe lanes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
…ation, denials) Goal: document the S6/LK-18 FULL_LOGIN behavior consumers now opt into via builder.fullLogin()/withLocale, per PR1 scope. How this slots into S6/LK-18: - New "Full-login bots" README section covering the three behavioral differences from the default spawn: real AsyncPreLogin/Login/Join events fire, the offline UUID is server-derived from the name (builder UUID ignored), and joins can be denied (BotJoinDeniedException) or time out (BotJoinTimeoutException); the call blocks until joined. Notes the offline-mode / proxy-forwarding-off requirement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
…orprone) Goal: keep the S6/LK-18 driver clean under the errorprone/NullAway profile, which only runs under -Perrorprone. How this slots into S6/LK-18: the synthesized GameProtocols.Context InvocationHandler returns null for unhandled methods, so its return type must be @nullable under the module's @NullMarked package. Surfaced by the -Perrorprone verify lane. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
BLOCKER 1: BotLoginSession no longer stops servicing packets when the outcome completes — that dropped every post-join KEEP_ALIVE/PING/TELEPORT, so the server's keep-alive watchdog kicked FULL_LOGIN bots shortly after joining. Packet dispatch is now gated on a volatile 'terminated' flag set only on denial, disconnect, transport close, timeout, or an internal servicing failure; after a successful join the session keeps answering keep-alives/pings and acknowledging teleports for the connection's lifetime. BLOCKER 1b (found by the new keep-alive IT on Paper): Paper arms its inbound packet-rate limiter (Connection.allPacketCounts) on every Connection instance, including the bot's own client-side one, which legitimately receives thousands of clientbound packets per second (chunks, entity movement) — so the bot's connection killed ITSELF for "packet spam" ~7s after joining (killForPacketSpam; the tell was the client trying to encode a clientbound disconnect through its serverbound outbound codec). The driver now disarms the limiter on its own client connection (nulls the counter field, resolved by type) before the handshake; the server's per-player connection and its protections are a separate instance and remain untouched. No-op on Spigot. MAJOR 2: handleFullLoginPlayer no longer stacks two full sync-operation timeouts (driver login + join-event wait ~ 240s worst case, past the 150s client watchdog). Both waits now share ONE deadline bounded by syncOperationTimeoutSeconds: the join latch only gets the residual time, preserving the RuntimeProtocol margin invariant (agent times out first). MINOR 3: BotLoginSession.run() closes the connection when sendHello fails after openConnection succeeded (terminate() now covers every terminal path except Joined). MINOR 5: handleFullLoginPlayer fails fast with IllegalStateException when invoked on the server main thread (the driver + join latch would deadlock the very loop that must fire PlayerJoinEvent). MINOR 6: (a) new IT fullLogin_shouldSurviveKeepAliveCycleWhileIdle — full-login bot idles 35s (a full keep-alive interval + kick window) and must not fire PlayerQuitEvent, then still accept a teleport; runs on both Paper and Spigot lanes and caught both BLOCKER 1 and BLOCKER 1b. (b) unit tests for the TimedOut->PLAYER_JOIN_TIMEOUT mapping and the login-ok-but-no-PlayerJoinEvent residual-timeout branch (short-budget fixture, inline-scheduler Bukkit statics); the denial test reworked off the main thread to match the MINOR 5 guard. NIT 7: BotJoinPhase javadoc now links IBotLoginOutcome.Denied. NIT 8: NmsReflectionUtils.resolveFieldByNameOrAcceptedType's fallback skips static fields and Object-typed fields so the Connection channel-field resolution rests on a meaningful type match, not declaration order. DEFERRED (documented only): one-line comments on the resource-pack and cookie-request CONSUME entries noting they are consumed without a response (PR2 provisioner territory). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@codex review |
Contributor
✅ Action performedFull review finished. |
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughFull-login bot creation now drives the vanilla login pipeline over loopback TCP, while the existing legacy-spawn path remains available. Builder and protocol contracts carry join mode and locale, NMS code reports terminal outcomes, the agent waits for join events, and typed denial/timeout errors are exposed. ChangesFull-login bot lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Builder
participant Agent
participant LoginDriver
participant Server
Builder->>Agent: create FULL_LOGIN player
Agent->>LoginDriver: login request
LoginDriver->>Server: loopback login handshake
Server-->>LoginDriver: login and play packets
LoginDriver-->>Agent: joined, denied, or timed out
Agent-->>Builder: player handle or typed error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dafc2d9b59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+160
to
+164
| clientboundFlow = NmsReflectionUtils.resolveEnumConstant(packetFlowClass, "CLIENTBOUND"); | ||
| final Object serverboundFlow = NmsReflectionUtils.resolveEnumConstant(packetFlowClass, "SERVERBOUND"); | ||
| protocolLogin = NmsReflectionUtils.resolveEnumConstant(connectionProtocolClass, "LOGIN"); | ||
| protocolConfiguration = NmsReflectionUtils.resolveEnumConstant(connectionProtocolClass, "CONFIGURATION"); | ||
| protocolPlay = NmsReflectionUtils.resolveEnumConstant(connectionProtocolClass, "PLAY"); |
There was a problem hiding this comment.
On Spigot this lazy full-login initialization still resolves packet-flow and protocol constants by unobfuscated Enum.name() values. NmsReflectionUtils.resolveEnumConstant only compares Enum.name(), while the existing v1_21_R7 adapter already has a fallback because Spigot's EnumProtocolDirection constants are typically obfuscated as a/b; the same applies to these protocol enums. As a result, .fullLogin() fails during loginDriver() initialization on the Spigot integration lane before any bot can join.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Addressed in 1a5c92f as hardening: the Spigot lane empirically passes today (a runtime probe confirmed Spigot's remapper obfuscates the enum fields but preserves the Enum.name() strings), but resolveEnumConstant now takes a verified expected-ordinal fallback (declaration order survives obfuscation) with a fail-loud out-of-range throw, so a future mapping change can't break resolution silently.
Comment on lines
+222
to
+224
| final int port = Bukkit.getServer().getPort(); | ||
| final IBotLoginOutcome outcome = botPlayerNmsAdapter.loginDriver().login( | ||
| new BotLoginRequest(name, port, locale, Duration.ofSeconds(timeoutSeconds))); |
There was a problem hiding this comment.
When callers use builder().atLocation(customWorld, x, y, z).fullLogin().build(), the framework still sends worldName/x/y/z, but this handler builds a BotLoginRequest that carries only name/port/locale/timeout and never reads those placement fields. Vanilla login will put the bot at the server's default spawn, so subsequent handle actions that depend on the configured world/location run against the wrong place unless the test manually teleports after build.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 1a5c92f: the world is validated up front and the joined bot is teleported on the main thread to the requested coordinates (or the world's spawn when only a world was given). The join smoke IT now joins at an explicit location and asserts the position via a bounded entity query on both lanes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.java (1)
208-232: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd failure coverage for the new reflection resolution paths.
Test that field fallback skips static/
Objectcandidates and that both method helpers throwNoSuchMethodExceptionwhen no compatible member exists. These failure paths determine whether obfuscated server changes fail predictably.As per coding guidelines, “For new functionality, test both happy-path behavior and failure/validation behavior with clear assertions.”
Also applies to: 437-507
🤖 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 208 - 232, Add tests for NmsReflectionUtils.resolveFieldByNameOrAcceptedType verifying fallback ignores static fields and Object-typed candidates, then throws NoSuchFieldException when no compatible instance field exists. Add failure-path tests for both method helper functions covering cases with no compatible member and asserting NoSuchMethodException is thrown.Source: Coding guidelines
🧹 Nitpick comments (2)
lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginIT.java (1)
31-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrictly format the setup, execute, and verify section markers.
As per coding guidelines, test bodies must contain explicit
// setup,// execute, and// verifysections. Adding descriptive text on the same line as the marker may violate strict automated checks or formatting rules. Consider moving the descriptive text to the following line to ensure full compliance. This applies to all test methods in this file.🧹 Proposed refactor (example for the first test)
- // setup — register the event captures BEFORE joining so the login events cannot be missed. + // setup + // register the event captures BEFORE joining so the login events cannot be missed. final var world = framework.worlds().main(); final String name = "lkfulllogin"; try (var preLoginCapture = framework.events().capture("org.bukkit.event.player.AsyncPlayerPreLoginEvent"); var joinCapture = framework.events().capture("org.bukkit.event.player.PlayerJoinEvent")) { - // execute — a full-login join drives the real handshake/login/configuration/play pipeline. + // execute + // a full-login join drives the real handshake/login/configuration/play pipeline. final var player = framework.bots().builder() .withName(name) .atSpawn(world) .withLocale("en_us") .fullLogin() .build(); - // verify — the bot exists with the server-derived offline UUID (FULL_LOGIN ignores any builder UUID). + // verify + // the bot exists with the server-derived offline UUID (FULL_LOGIN ignores any builder UUID).🤖 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-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginIT.java` around lines 31 - 48, Update every test method in LightkeeperFullLoginIT so section markers are standalone comments exactly formatted as // setup, // execute, and // verify; move any descriptive text onto separate following comment lines while preserving the existing test flow.Source: Coding guidelines
lightkeeper-protocol/src/test/java/nl/pim16aap2/lightkeeper/protocol/AgentCommandValidationTest.java (1)
336-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit setup, execute, and verify sections in the new tests.
lightkeeper-protocol/src/test/java/nl/pim16aap2/lightkeeper/protocol/AgentCommandValidationTest.java#L336-L395: separate every combined or omitted test phase.lightkeeper-nms-parent/lightkeeper-nms-api/src/test/java/nl/pim16aap2/lightkeeper/nms/api/BotLoginRequestTest.java#L15-L62: add distinct setup, execution, and verification sections to every test.As per coding guidelines, test bodies must contain explicit
// setup,// execute, and// verifysections.🤖 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-protocol/src/test/java/nl/pim16aap2/lightkeeper/protocol/AgentCommandValidationTest.java` around lines 336 - 395, Separate each test in AgentCommandValidationTest.java (lines 336-395) and BotLoginRequestTest.java (lines 15-62) into explicit // setup, // execute, and // verify sections. Move object construction or test inputs into setup, exception/assertion invocation into execute, and assertions into verify, ensuring every test body contains all three sections.Source: Coding guidelines
🤖 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/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java`:
- Around line 196-224: Update the FULL_LOGIN flow around deadlineNanos,
mainThreadExecutor, and BotLoginRequest so listener registration,
loginDriver().login, and listener cleanup each use only the remaining shared
deadline. Pass the remaining duration to BotLoginRequest and use deadline-aware
main-thread calls for registration and cleanup. Ensure cleanup failures are
suppressed or otherwise retained without replacing the original typed denial or
timeout.
- Around line 237-242: Update the joinLatch.await handling in AgentPlayerActions
so InterruptedException is caught, the thread interrupt flag is restored, and an
AgentProtocolException with AgentErrorCode.INTERRUPTED is thrown. Preserve the
existing PLAYER_JOIN_TIMEOUT behavior for deadline expiry or an await returning
false, matching AgentMainThreadExecutor’s interruption handling.
- Around line 192-195: Update handleFullLoginPlayer() to read and apply
command.worldName(), command.x(), command.y(), and command.z() after the player
joins, placing the player at the requested location; alternatively, explicitly
reject FULL_LOGIN requests containing location data.
In
`@lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java`:
- Around line 603-612: Separate the combined execute-and-verify blocks in the
affected tests into explicit // setup, // execute, and // verify sections. In
AgentPlayerActionsTest, capture the throwable from
fixture.playerActions().handleCreatePlayer(command) during // execute, then
perform the AgentProtocolException type, errorCode, and message assertions
during // verify; apply the same structure to the tests around the other
referenced blocks.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java`:
- Around line 304-311: Update openConnection to clean up the newly created
connection when disarmPaperPacketLimiter or initiateLoginConnectionMethod.invoke
fails before the connection is returned. Keep the existing successful setup and
return behavior, close the connection through its available channel/close
mechanism, and rethrow the original ReflectiveOperationException.
- Around line 612-636: Update packet registration in BotLoginReflection so
ClientboundCustomQueryPacket, ClientboundResourcePackPushPacket, and
ClientboundCookieRequestPacket are no longer consumed as optional no-response
packets. Implement their matching serverbound replies in the login handling
flow, or remove them from interception so the normal protocol handlers can
respond; preserve optional registration only for packets that require no reply.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java`:
- Around line 249-257: Update the packet-servicing failure handling around the
relevant login-session method and its findNoArgAccessor call to catch
IllegalStateException as well as ReflectiveOperationException, ensuring every
failure completes outcome exceptionally. When a failure occurs after the Joined
phase, also close or terminate the underlying channel/connection rather than
only setting terminated, while preserving the existing failure logging behavior.
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflectionHelpersTest.java`:
- Around line 47-90: Update the four tests in BotLoginReflectionHelpersTest so
each body has explicit // setup, // execute, and // verify sections. Add //
setup before preparing inputs or invoking the subject, separate the combined //
execute + verify block in resolveEnumConstant_shouldThrowWhenNameIsUnknown into
distinct sections, and keep the existing assertions and behavior unchanged.
---
Outside diff comments:
In
`@lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.java`:
- Around line 208-232: Add tests for
NmsReflectionUtils.resolveFieldByNameOrAcceptedType verifying fallback ignores
static fields and Object-typed candidates, then throws NoSuchFieldException when
no compatible instance field exists. Add failure-path tests for both method
helper functions covering cases with no compatible member and asserting
NoSuchMethodException is thrown.
---
Nitpick comments:
In
`@lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginIT.java`:
- Around line 31-48: Update every test method in LightkeeperFullLoginIT so
section markers are standalone comments exactly formatted as // setup, //
execute, and // verify; move any descriptive text onto separate following
comment lines while preserving the existing test flow.
In
`@lightkeeper-protocol/src/test/java/nl/pim16aap2/lightkeeper/protocol/AgentCommandValidationTest.java`:
- Around line 336-395: Separate each test in AgentCommandValidationTest.java
(lines 336-395) and BotLoginRequestTest.java (lines 15-62) into explicit //
setup, // execute, and // verify sections. Move object construction or test
inputs into setup, exception/assertion invocation into execute, and assertions
into verify, ensuring every test body contains all three sections.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 267746db-8d0b-4b3e-8f9b-e20492481520
📒 Files selected for processing (33)
README.mdlightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentMainThreadExecutor.javalightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.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/BotJoinDeniedException.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/BotJoinTimeoutException.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IPlayerBuilder.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacade.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFramework.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultPlayerBuilder.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/UdsAgentClient.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/UdsAgentTransport.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacadeTest.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginIT.javalightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/BotJoinPhase.javalightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/BotLoginRequest.javalightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotLoginDriver.javalightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotLoginOutcome.javalightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotPlayerNmsAdapter.javalightkeeper-nms-parent/lightkeeper-nms-api/src/test/java/nl/pim16aap2/lightkeeper/nms/api/BotLoginRequestTest.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginPipelineDriver.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.javalightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.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/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflectionHelpersTest.javalightkeeper-protocol/src/main/java/nl/pim16aap2/lightkeeper/protocol/AgentErrorCode.javalightkeeper-protocol/src/main/java/nl/pim16aap2/lightkeeper/protocol/CreatePlayer.javalightkeeper-protocol/src/main/java/nl/pim16aap2/lightkeeper/protocol/JoinMode.javalightkeeper-protocol/src/test/java/nl/pim16aap2/lightkeeper/protocol/AgentCommandSerializationTest.javalightkeeper-protocol/src/test/java/nl/pim16aap2/lightkeeper/protocol/AgentCommandValidationTest.javalightkeeper-runtime-core/src/main/java/nl/pim16aap2/lightkeeper/runtime/RuntimeProtocol.java
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new FULL_LOGIN bot join mode that drives the complete vanilla login pipeline over a real TCP loopback connection (Paper/Spigot 1.21.11), enabling plugins that depend on pre-login/login events to observe synthetic players. It bumps the runtime protocol to v10 and threads the new join mode + optional locale through protocol, agent, and framework layers, with unit + integration test coverage.
Changes:
- Add protocol v10 support for
CreatePlayerwithJoinMode(FULL_LOGIN/LEGACY_SPAWN) and optionallocale. - Implement a reflective, cross-distro full-login pipeline driver + session that services keep-alive/ping/teleport-ack post-join.
- Expose the feature in the framework builder API and add agent/framework mappings plus integration tests.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents full-login bots usage and behavioral differences vs legacy spawn. |
| lightkeeper-runtime-core/src/main/java/nl/pim16aap2/lightkeeper/runtime/RuntimeProtocol.java | Bumps runtime protocol version to 10. |
| lightkeeper-protocol/src/test/java/nl/pim16aap2/lightkeeper/protocol/AgentCommandValidationTest.java | Adds validation tests for CreatePlayer.Command joinMode/uuid/locale rules. |
| lightkeeper-protocol/src/test/java/nl/pim16aap2/lightkeeper/protocol/AgentCommandSerializationTest.java | Extends serialization round-trip coverage for joinMode + locale. |
| lightkeeper-protocol/src/main/java/nl/pim16aap2/lightkeeper/protocol/JoinMode.java | Introduces wire-level join mode enum for bot placement semantics. |
| lightkeeper-protocol/src/main/java/nl/pim16aap2/lightkeeper/protocol/CreatePlayer.java | Extends CreatePlayer.Command with joinMode and optional locale, plus validation. |
| lightkeeper-protocol/src/main/java/nl/pim16aap2/lightkeeper/protocol/AgentErrorCode.java | Adds typed error codes for join denied/timeout. |
| lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/test/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflectionHelpersTest.java | Adds unit tests for reflection helpers used by the full-login driver. |
| lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/NmsReflectionUtils.java | Adds structural enum/method resolution helpers and tightens field fallback rules. |
| lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotPlayerNmsAdapterV1_21_R7.java | Lazily provides a full-login driver via new adapter API method. |
| lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java | Implements the login/config/play packet-handling session and post-join servicing. |
| lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java | Resolves the reflective surface to open loopback connections and handle protocol phases. |
| lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginPipelineDriver.java | Defines the concrete v1_21_R7 full-login driver that runs independent sessions. |
| lightkeeper-nms-parent/lightkeeper-nms-api/src/test/java/nl/pim16aap2/lightkeeper/nms/api/BotLoginRequestTest.java | Adds validation tests for BotLoginRequest. |
| lightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotPlayerNmsAdapter.java | Extends NMS adapter API with loginDriver() seam for full-login joins. |
| lightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotLoginOutcome.java | Adds sealed outcome model for joined/denied/timed-out full-login attempts. |
| lightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/IBotLoginDriver.java | Introduces interface for driving full-login over loopback TCP. |
| lightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/BotLoginRequest.java | Adds immutable request model for full-login inputs and validation. |
| lightkeeper-nms-parent/lightkeeper-nms-api/src/main/java/nl/pim16aap2/lightkeeper/nms/api/BotJoinPhase.java | Adds join-phase enum surfaced on denial outcomes. |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginIT.java | Adds end-to-end ITs validating real login events, UUID derivation, and keep-alive survival. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacadeTest.java | Updates mocks/expectations for new createPlayer signature (joinMode/locale). |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IPlayerBuilder.java | Adds fullLogin() and withLocale(...) to the public builder API. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/UdsAgentTransport.java | Maps join denied/timeout agent errors to typed framework exceptions. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/UdsAgentClient.java | Threads joinMode/locale through CreatePlayer.Command and UUID rules for FULL_LOGIN. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultPlayerBuilder.java | Stores joinMode/locale builder state and forwards it into framework creation. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFramework.java | Extends internal builder creation path with joinMode/locale parameters. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacade.java | Propagates joinMode/locale through bot creation calls (default LEGACY_SPAWN). |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/BotJoinTimeoutException.java | Adds typed framework exception for full-login timeouts. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/BotJoinDeniedException.java | Adds typed framework exception for full-login denials. |
| lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentRequestDispatcherTest.java | Updates create-player dispatch test JSON to include joinMode/locale. |
| lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java | Adds tests for typed full-login denial/timeout mappings and join-event timeout behavior. |
| lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java | Routes create-player to full-login vs legacy-spawn, and awaits PlayerJoinEvent under a shared deadline. |
| lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentMainThreadExecutor.java | Exposes sync-operation timeout accessor for long-running orchestration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+192
to
+196
| final String name = command.name(); | ||
| final String locale = command.locale(); | ||
| final Double health = command.health(); | ||
| final String permissionsCsv = command.permissionsCsv(); | ||
| final long timeoutSeconds = mainThreadExecutor.syncOperationTimeoutSeconds(); |
Owner
Author
There was a problem hiding this comment.
Fixed in 1a5c92f (final @Nullable String locale). Note the errorprone/NullAway gate passed either way — locals are flow-typed — so this was clarity, not a contract violation.
Comment on lines
76
to
+80
| * Propagates the callable's own failure (unwrapped from {@link ExecutionException}); throws | ||
| * {@link AgentProtocolException} with {@link AgentErrorCode#TIMEOUT} when the operation exceeds the | ||
| * configured timeout, or {@link AgentErrorCode#INTERRUPTED} when the waiting thread is interrupted. | ||
| */ | ||
| /** |
Owner
Author
There was a problem hiding this comment.
Fixed in 1a5c92f: each method carries its own javadoc block again.
1 (Codex P1): resolveEnumConstant hardened with a documented ordinal fallback. Declaration
orders verified against BOTH 1.21.11 jars (javap on Paper; runtime probe on Spigot, whose
enum FIELDS are obfuscated to a/b.. but whose Enum.name() strings are preserved):
PacketFlow SERVERBOUND=0, CLIENTBOUND=1; ConnectionProtocol HANDSHAKING=0, PLAY=1,
STATUS=2, LOGIN=3, CONFIGURATION=4. Fallback logs at DEBUG; out-of-range ordinal still
fails loud.
2 (Codex P2 + CodeRabbit 3586962018): FULL_LOGIN now honors the command's placement. The
wire sends worldName always and x/y/z only from the builder's atLocation, so the post-join
main-thread registration teleports to (world,x,y,z) when coordinates are present and to
the world's spawn otherwise; the world is validated up front (before any connection) in
the listener-registration callable. The join smoke IT now joins at an explicit location
and asserts the position via the entity-query API on both lanes.
3 (CodeRabbit 3586962026): every blocking phase draws from the ONE shared deadline. The
finally-block listener cleanup is bounded to clamp(remaining, 2s, 10s) via a new explicit-
timeout callOnMainThread overload, and can no longer mask the primary failure: a cleanup
exception is addSuppressed onto it when one exists and logged otherwise.
4 (CodeRabbit 3586962030): joinLatch.await interruption now re-interrupts and surfaces as
AgentProtocolException(INTERRUPTED), matching AgentMainThreadExecutor's contract.
5 (CodeRabbit 3586962045): openConnection closes the freshly opened TCP connection when
limiter disarming or handshake initiation throws after connectToServer succeeded.
6 (CodeRabbit 3586962058): reply-required packets now get vanilla-client answers instead
of being consumed: login custom query -> ServerboundCustomQueryAnswerPacket(txId, null)
("unknown"); cookie request -> ServerboundCookieResponsePacket(key, null) (absent
payload); resource-pack push -> DECLINED (a require-pack server then kicks, surfacing as a
typed denial). Constructions verified against both jars; the per-distro cookie key type
(Identifier vs MinecraftKey) and the nested action enum ($Action vs $a) are resolved
structurally from the response constructors; DECLINED ordinal 1 verified on both. The old
CONSUME limitation comments are gone; only the pop (never answered by vanilla) remains
consumed.
7 (CodeRabbit 3586962063): packet-servicing failures now catch RuntimeException too (a
missing structural accessor throws IllegalStateException), so the outcome can never be
left pending; terminate() now ALWAYS closes the channel, including servicing failures
after a successful join.
8 (CodeRabbit 3586962041 + 3586962073): the three FULL_LOGIN agent tests and the enum
helper test use strict // setup / // execute / // verify blocks (catchThrowable pattern).
9 (Copilot 3586969189): the locale local is annotated @nullable for clarity.
10 (Copilot 3586969220): AgentMainThreadExecutor's orphaned javadoc reattached; both
callOnMainThread overloads and the timeout getter each carry their own doc.
13 (CI, highest priority): the integration-spigot CI failure (fullLogin smoke test burning
its full 120s in the driver while the same lane's keep-alive login passed 40s earlier on a
warm server) is a silent-close defect, not cold-start slowness. Evidence: channelInactive
only RECORDS the disconnection; PacketListener.onDisconnect fires solely from
Connection.handleDisconnection(), which is driven by the client/server tick loops — and
the driver's client connection is in neither, so a raw remote close (e.g. the server's
30-second ReadTimeoutHandler abandoning a stalled login, verified present in the server
pipeline, which sends no disconnect packet) was invisible and the join waited out its
whole budget. Fix: a reflective closeFuture listener on the bot's channel surfaces any
close immediately as a typed Denied("Connection closed during <phase>"), and TimedOut now
carries the stalled phase so any future timeout names where the login stopped.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
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
How
BotLoginPipelineDriver(behindIBotLoginDriver): performs the complete login over a real TCP connection to the server's own port. Fully reflective — 14 per-distro class-name pairs (Paper is Mojang-mapped, Spigot obfuscated) and zero per-distro member names: every member resolves structurally by signature/return/generic type, verified unambiguous against both real 1.21.11 jars. Fail-loud packet policy: unhandled packet types error with the class name; disconnects surface the kick reason in every phase.CreatePlayer.joinMode(FULL_LOGIN/LEGACY_SPAWN) + optionallocale(applied viaClientInformationin the config phase); under FULL_LOGIN the uuid must be null (the server derives the offline UUID from the name). Agent awaitsPlayerJoinEventunder a single shared deadline within the sync-timeout budget and maps denials/timeouts to typed errors (BotJoinDeniedException/BotJoinTimeoutExceptionframework-side).bots().builder().fullLogin().withLocale(...); default join stays LEGACY_SPAWN. Post-join the session keeps servicing keep-alive/ping/teleport-ack for the connection's lifetime; on Paper the bot connection's inbound packet-spam limiter is disabled pre-handshake (a test client legitimately receives thousands of packets/s — server-side protections untouched).Tests
mvn --batch-mode -Perrorprone clean verify checkstyle:checkstyle pmd:check→ BUILD SUCCESS (full reactor, both IT lanes).LightkeeperFullLoginIT(both lanes): real-pipeline join asserting AsyncPlayerPreLoginEvent + PlayerJoinEvent + server-derived offline UUID; a 35s idle keep-alive soak (caught two real bugs during development: the post-join packet-servicing drop and Paper's client-side spam kick); LEGACY_SPAWN regression guard.🤖 Generated with Claude Code
Summary by CodeRabbit