🤖 test: full-login validation matrix and loopback provisioner guard (LK-18 PR2)#101
Conversation
…n provisioning FULL_LOGIN bots complete a real offline-mode login over loopback TCP, so a config overlay that flips online-mode=true (server.properties), settings .bungeecord: true (spigot.yml), or proxies.velocity.enabled: true (config/paper-global.yml) would silently break every full-login join with a timeout. LoopbackLoginGuard validates the fully materialized target server directory (after overlay application) at the end of prepare-server and fails the build with an actionable message naming FULL_LOGIN instead. First slice of the S6 PR2 validation matrix: the guard hardens the provisioning invariant the whole matrix relies on (report section 4.5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
Logs the invoking player's Player#getLocale in a deterministic single-line format (LK_LOCALE name=... locale=...). The full-login locale IT asserts the server-side view of the configuration-phase ClientInformation locale from the captured console output, because full-login bots have no embedded channel to drain received messages from. Second slice of the S6 PR2 validation matrix: the observable for the withLocale (LK-12) assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
…trix Covers on both distros: the documented AsyncPlayerPreLoginEvent -> PlayerLoginEvent -> PlayerJoinEvent order via tick stamps (the Paper/Spigot login-event divergence moves the login event relative to the configuration phase only, so the same ordering is the per-distro expectation), ban and whitelist denials surfaced as typed BotJoinDeniedException carrying the kick reason (whitelist denial asserted fail-fast, then flipped to a successful join via whitelist add), the real PlayerQuitEvent on removal, and quit/rejoin with the same server-derived offline UUID plus position restored from player data. Third slice of the S6 PR2 validation matrix (report tests 1, 3, 4, 5, 6, 13). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
…survival Covers on both distros: teleport acknowledgement (position applied and the session survives the ack round trip), login under the default-on compression threshold (asserted explicitly from the materialized server.properties so a silent flip to disabled fails the test), five back-to-back full logins simultaneously online with distinct offline UUIDs (the synchronized UDS transport allows one in-flight request, so wire-level concurrency is documented as not deliverable rather than faked), the configuration-phase locale observed server-side via /lktestlocale, chat parity through the real AsyncPlayerChatEvent, and LuckPerms Permissible-injection survival: LuckPerms is provisioned from Modrinth (pinned v5.5.53-bukkit) and a console lp grant must become visible through the joined player's live hasPermission. Final slice of the S6 PR2 validation matrix (report tests 2, 7, 8, 9, 10, 12). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
The merged bot-round fix (1a5c92f) makes FULL_LOGIN honor the requested placement: the agent teleports a (re)joining bot to the requested location or world spawn post-join, deliberately overriding the position loaded from player data. The rejoin test's persistence observable therefore moves from the live position to the inventory (console give before quit, findItem after rejoin), which the quit path saves and the placement does not touch. The identity assertion (same server-derived offline UUID) is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
Regression caught by the new denial matrix on both lanes: 1a5c92f's channel close listener (which surfaces raw remote closes) fires INLINE when terminate() closes the channel, and completeDenied called terminate() before completing the outcome. The listener's generic 'Connection closed during <phase>' denial therefore won the one-shot completion race, discarding the server's actual kick reason (ban/whitelist text). The same masking applied to the fail-loud unhandled-packet error and packet-servicing failures. Fix: resolve the outcome first, then terminate. Verified by fullLogin_shouldRejectBannedPlayerWithTypedDenial and fullLogin_shouldRejectWhenWhitelistExcludesPlayer, which assert the kick reason text on Paper and Spigot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
Second regression caught by the matrix, on the Paper lane: removing a
FULL_LOGIN bot through the legacy PlayerList removal left its real TCP
connection open. ~30 seconds later the server timed the connection out and ran
PlayerList.remove a second time, hitting Paper's EntityScheduler 'Already
retired' IllegalStateException — which crashes the whole server ('Encountered
an unexpected exception'), killing the agent and every later test in the lane.
Fix: track FULL_LOGIN joins and remove those bots via Player#kickPlayer, the
real disconnect flow — the play-phase disconnect packet reaches the login
driver (which closes its channel), the player is removed exactly once, and
PlayerQuitEvent fires on the regular quit path. Legacy-spawn bots keep the
reflective removal. Covered by a new agent unit test and by the quit/rejoin
integration tests on both lanes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
Third regression caught by the matrix (twice, Paper lane): connectToServer
syncs on the Netty connect future, but Connection.channel is only assigned in
channelActive() on the event loop, so the driver thread can still observe
isConnected() == false after connectToServer returns. In that window
initiateServerboundPlayConnection silently defers the entire protocol/listener
setup into the connection's pending-actions queue — which nothing ever drains
for this untucked client-side connection — while send() queues the hello
separately. The hello then hits the encoder with no LOGIN protocol and no
listener ('Sending unknown packet serverbound/minecraft:hello', 'packetListener
is null'), and the join stalls until the server's 30-second ReadTimeoutHandler
closes it (now surfaced as 'Connection closed during LOGIN').
Fix: after connectToServer, wait (bounded, 5s) until the channel field is
assigned and open — vanilla's isConnected() contract — so the setup and the
hello take their immediate, in-program-order paths. Loopback channels activate
in well under a millisecond; the wait is a named-constant bounded spin.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
1 (MAJOR): removeRegisteredPlayer now detects a vetoed kick. PlayerKickEvent is cancellable on both distros, so a kick-cancelling plugin turned the removal of a full-login bot into a silent no-op while the agent reported success — a permanent ghost bot whose driver keeps answering keep-alives. The disconnect completes synchronously inside kickPlayer on both jars, so an isOnline() probe after the kick reliably detects the veto; it now throws a typed AgentProtocolException(REQUEST_FAILED) naming the cancelled PlayerKickEvent BEFORE any agent state is mutated (store entry retained, removal retryable, no legacy-removal fallback). Covered by a kick-cancelling unit test. 2: all matrix @timeout values raised 60 -> 180 seconds, safely above the 120s agent join budget and the 150s client watchdog, so a stalled login surfaces as the typed PLAYER_JOIN_TIMEOUT instead of a JUnit interrupt mid-RPC. 3: dropped the whitelist denial's <15s wall-clock assertion (starved-runner flake); the typed BotJoinDeniedException-not-timeout distinction proves the fail-fast semantics, documented in a comment. 4: LoopbackLoginGuard accepts case-insensitive YAML booleans (True/TRUE) for bungeecord/velocity-enabled and legal properties spacing ('online-mode = true'); unit tests extended with all three spellings. 5: awaitChannelActive documents that the non-volatile cross-thread read mirrors vanilla's own isConnected() access pattern and that the reflective Field.get defeats hoisting. 6: renamed fullLogin_shouldSupportFiveConcurrentJoins -> fullLogin_shouldSustainFiveSimultaneousSessions to match what is asserted (sequential joins over the synchronized transport, simultaneous sessions). Skipped per review: offlineUuid dedup across PR1's smoke IT (additive-scope rule; fold on next touch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
|
@coderabbitai full review |
|
@codex review |
✅ Action performedFull review finished. |
|
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 player removal now uses the real disconnect path, NMS login sequencing waits for active channels, provisioning validates loopback settings, and integration coverage expands across sessions, lifecycle events, permissions, locale, and denial handling. ChangesFULL_LOGIN behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant FullLoginBot
participant MinecraftServer
participant AgentPlayerActions
TestSuite->>FullLoginBot: start FULL_LOGIN session
FullLoginBot->>MinecraftServer: connect and complete login
MinecraftServer-->>TestSuite: login, join, chat, and state events
TestSuite->>AgentPlayerActions: remove player
AgentPlayerActions->>MinecraftServer: kick through real disconnect flow
MinecraftServer-->>TestSuite: quit event and cleared player state
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 |
There was a problem hiding this comment.
Pull request overview
This PR expands LightKeeper’s new FULL_LOGIN bot pipeline with a comprehensive validation-matrix integration test suite, adds a locale observability hook in the standalone test plugin, and hardens provisioning/runtime behavior to fail loudly (and deterministically) when configurations or lifecycle flows would break loopback full-logins.
Changes:
- Added two integration-test classes (+ shared helpers) covering login-event ordering, typed denials, persistence, compression, locale propagation, multi-session stability, chat parity, and LuckPerms login-time injection survival.
- Added a
prepare-serverguard (LoopbackLoginGuard) that rejects server configs known to break loopbackFULL_LOGIN(online-mode / BungeeCord / Velocity forwarding). - Fixed several full-login lifecycle edge cases: outcome completion vs channel-close masking, bounded wait for channel activation, and real disconnect flow for removing full-login bots (kick + veto detection).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java | Unit tests for deterministic locale logging behavior used by ITs. |
| lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml | Declares the new /lktestlocale command for the test plugin. |
| lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java | Adds /lktestlocale executor that logs Player#getLocale() in a stable format. |
| lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java | Wires the new locale command into plugin startup. |
| lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java | Ensures denial/exception outcomes complete before channel termination to preserve real reasons. |
| lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java | Adds a bounded wait for Netty channel activation to avoid stalled-login races. |
| lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java | Unit tests for guard parsing and failure modes across supported config shapes. |
| lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java | Introduces provisioning-time fail-loud validation for loopback-incompatible server configs. |
| lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java | Invokes the guard after overlays/assets are applied. |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java | Matrix part 1: event order, denials, quit/rejoin identity + persistence. |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java | Matrix part 2: in-session behaviors (teleport ack, compression, locale, chat, LuckPerms, concurrency). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java | Shared helpers for offline UUID derivation, event attribution, and position assertions. |
| lightkeeper-integration-tests/pom.xml | Provisions LuckPerms from Modrinth for the Permissible-injection survival check. |
| lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java | Adds tests ensuring full-login removals use kick and fail loud on kick veto. |
| lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java | Tracks full-login bot IDs and removes them via real disconnect flow with veto detection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final String trimmed = line.trim(); | ||
| if (trimmed.startsWith("#")) | ||
| continue; |
There was a problem hiding this comment.
Fixed in 83ab657: the guard now skips both '#' and '!' comment prefixes per the java.util.Properties spec, with the commented-settings test extended to cover the '!' spelling.
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml (1)
8-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a permission entry for
/lktestlocale, matchinglktestgui/lktesterror.Every other command in this file is gated by a
lightkeeper.testplugin.<command>permission; the newlktestlocalecommand has none, so it's callable by any player unlike its siblings.🔧 Proposed fix
commands: lktestgui: description: Opens the standalone LightKeeper integration test GUI usage: /lktestgui lktesterror: description: Emits a SEVERE or WARNING log event for LightKeeper error-capture integration tests usage: /lktesterror <severe|warning> lktestlocale: description: Logs the invoking player's client locale for LightKeeper full-login locale integration tests usage: /lktestlocale + permission: lightkeeper.testplugin.lktestlocale permissions: lightkeeper.testplugin.lktestgui: description: Allows opening the LightKeeper test GUI. default: false lightkeeper.testplugin.lktesterror: description: Allows emitting LightKeeper error-capture test log events. default: false + lightkeeper.testplugin.lktestlocale: + description: Allows logging the invoking player's client locale. + default: false🤖 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-spigot-test-plugin/src/main/resources/plugin.yml` around lines 8 - 24, Add a lightkeeper.testplugin.lktestlocale permission entry under permissions in plugin.yml, matching the existing lktestgui and lktesterror entries with an appropriate description and default: false, so the lktestlocale command is permission-gated consistently.lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java (1)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment sections use combined
// execute + verifyinstead of separate// setup/// execute/// verify.Minor style deviation from the explicit three-section test-body convention; behavior and assertions are otherwise correct and thorough.
As per path instructions, "Test method names must follow
methodToTest_whatWeTest, and test bodies must contain explicit// setup,// execute, and// verifysections."Also applies to: 43-46, 54-60, 69-74, 89-94, 103-108, 117-122, 136-141, 159-162, 179-182
🤖 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-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java` around lines 33 - 36, Update all affected test methods in LoopbackLoginGuardTest to use separate explicit // setup, // execute, and // verify sections, replacing combined // execute + verify comments while preserving the existing assertions and behavior.Source: Path instructions
lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java (1)
269-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication with
LoopbackLoginGuard's properties-line scan.
readCompressionThresholdreimplements a small ad hoc.propertiesline-scan already present inLoopbackLoginGuard.validateServerProperties, but in a different module. Not worth a shared abstraction across module boundaries for ~8 lines.🤖 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/LightkeeperFullLoginSessionIT.java` around lines 269 - 279, Remove the duplicated readCompressionThreshold properties-line scan and reuse the existing parsing logic from LoopbackLoginGuard.validateServerProperties where practical, avoiding a new cross-module abstraction. Preserve extraction and validation of the network-compression-threshold value and the existing missing-entry failure behavior.
🤖 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-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java`:
- Around line 48-78: Ensure every full-login bot in
LightkeeperFullLoginSessionIT is removed via a finally block, including the
tests at lines 48-78, 80-117, 165-191, and 193-224. Move each player.remove()
cleanup into finally so it executes when eventually(...) or any assertion fails,
while preserving the existing test logic.
---
Nitpick comments:
In
`@lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java`:
- Around line 269-279: Remove the duplicated readCompressionThreshold
properties-line scan and reuse the existing parsing logic from
LoopbackLoginGuard.validateServerProperties where practical, avoiding a new
cross-module abstraction. Preserve extraction and validation of the
network-compression-threshold value and the existing missing-entry failure
behavior.
In
`@lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java`:
- Around line 33-36: Update all affected test methods in LoopbackLoginGuardTest
to use separate explicit // setup, // execute, and // verify sections, replacing
combined // execute + verify comments while preserving the existing assertions
and behavior.
In `@lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml`:
- Around line 8-24: Add a lightkeeper.testplugin.lktestlocale permission entry
under permissions in plugin.yml, matching the existing lktestgui and lktesterror
entries with an appropriate description and default: false, so the lktestlocale
command is permission-gated consistently.
🪄 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: 4fe9f3b8-d69e-4d0a-a004-14e6ec34d8b7
📒 Files selected for processing (15)
lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.javalightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.javalightkeeper-integration-tests/pom.xmllightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.javalightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.javalightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.javalightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.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-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.javalightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.javalightkeeper-spigot-test-plugin/src/main/resources/plugin.ymllightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Goal: address Copilot's bot-round finding on PR #101 — java.util.Properties recognizes both '#' and '!' as comment prefixes, so a commented-out '! online-mode=true' must not fail provisioning. Guard skips both; the commented-settings unit test now covers the '!' spelling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
Why
How
LoopbackLoginGuard:prepare-servernow fails loudly (naming FULL_LOGIN) ononline-mode=true, active BungeeCord, or Velocity forwarding — the configs that break loopback logins; case/spacing-tolerant parsing, runs after all overlays./lktestlocaletest-plugin observable for server-side locale assertions.kickPlayer) with a fail-loud check forPlayerKickEventvetoes — the old path left the TCP connection open and could crash Paper via double-removal; the driver now waits (bounded, fail-loud) for channel activation before sending hello, closing a stalled-login race.Tests
mvn --batch-mode -Perrorprone clean verify checkstyle:checkstyle pmd:check→ BUILD SUCCESS (full reactor; 49/49 ITs on both Paper and Spigot lanes).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Validation