Skip to content

🤖 test: full-login validation matrix and loopback provisioner guard (LK-18 PR2)#101

Merged
PimvanderLoos merged 10 commits into
masterfrom
feat/s6-login-validation
Jul 16, 2026
Merged

🤖 test: full-login validation matrix and loopback provisioner guard (LK-18 PR2)#101
PimvanderLoos merged 10 commits into
masterfrom
feat/s6-login-validation

Conversation

@PimvanderLoos

@PimvanderLoos PimvanderLoos commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Why

How

  • Two IT classes (11 tests, both lanes): login-event order with tick stamps, ban/whitelist typed denials (deny→allow), rejoin identity + inventory persistence, real quit events, teleport-ack, compression, five simultaneous sessions, server-observed locale, chat parity, and LuckPerms Permissible-injection survival (LP provisioned from Modrinth, granted via console, asserted live).
  • LoopbackLoginGuard: prepare-server now fails loudly (naming FULL_LOGIN) on online-mode=true, active BungeeCord, or Velocity forwarding — the configs that break loopback logins; case/spacing-tolerant parsing, runs after all overlays.
  • /lktestlocale test-plugin observable for server-side locale assertions.
  • Three production regression fixes the matrix exposed in the merged base: denial outcomes complete before the channel closes (kick reasons no longer masked by the close listener); full-login bots leave via the real disconnect flow (kickPlayer) with a fail-loud check for PlayerKickEvent vetoes — 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).
  • New unit tests: guard parsing edge cases (10), locale command executor (2), kick-veto and kick-routing paths in the agent (2).
  • Pre-PR deep review applied (verdict gated on the kick-veto check — fixed): all three regression fixes' mechanical claims verified against both distros' bytecode.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved FULL_LOGIN connection reliability by waiting for the network channel to become ready before starting login.
    • Preserved specific server denial and connection failure reasons instead of replacing them with generic messages.
    • Ensured fully logged-in synthetic players disconnect through the normal server flow.
    • Improved cleanup when player removal is cancelled or interrupted.
  • Validation

    • Server preparation now detects incompatible online-mode and proxy-forwarding settings before startup.

PimvanderLoos and others added 9 commits July 15, 2026 23:29
…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
@PimvanderLoos

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@PimvanderLoos
PimvanderLoos requested a review from Copilot July 16, 2026 05:05
@PimvanderLoos

Copy link
Copy Markdown
Owner Author

@codex review

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e8e5e5cc-4726-48db-af99-74a7ecc7eb9b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

FULL_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.

Changes

FULL_LOGIN behavior

Layer / File(s) Summary
FULL_LOGIN player removal
lightkeeper-agent-spigot/src/main/.../AgentPlayerActions.java, lightkeeper-agent-spigot/src/test/.../AgentPlayerActionsTest.java
Tracks FULL_LOGIN players, removes them through kickPlayer, detects cancelled kicks, and centralizes cleanup.
Connection activation and outcome ordering
lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/.../BotLoginReflection.java, BotLoginSession.java
Waits for open channels before login setup and completes denial or failure outcomes before termination.
Provisioned server loopback validation
lightkeeper-maven-plugin/src/main/.../LoopbackLoginGuard.java, PrepareServerMojo.java, .../LoopbackLoginGuardTest.java
Rejects unsafe online-mode and proxy-forwarding settings after provisioning and before manifest creation.
FULL_LOGIN session behavior coverage
lightkeeper-integration-tests/pom.xml, lightkeeper-integration-tests/src/test/.../*, lightkeeper-spigot-test-plugin/src/.../*
Adds integration coverage for sessions, teleportation, compression, concurrent identities, chat, locale, and LuckPerms permissions.
FULL_LOGIN lifecycle validation
lightkeeper-integration-tests/src/test/.../LightkeeperFullLoginValidationIT.java
Validates event ordering, typed denials, whitelist behavior, persistence across rejoin, and quit events.

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
Loading

Possibly related PRs

  • PimvanderLoos/LightKeeper#80: Earlier AgentPlayerActions player lifecycle and typed protocol-command changes relate to this removal refactor.

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main additions: a full-login validation matrix and a loopback provisioner guard.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-server guard (LoopbackLoginGuard) that rejects server configs known to break loopback FULL_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.

Comment on lines +86 to +88
final String trimmed = line.trim();
if (trimmed.startsWith("#"))
continue;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 394311d83b

ℹ️ 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".

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml (1)

8-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a permission entry for /lktestlocale, matching lktestgui/lktesterror.

Every other command in this file is gated by a lightkeeper.testplugin.<command> permission; the new lktestlocale command 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 value

Comment sections use combined // execute + verify instead 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 // verify sections."

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 value

Minor duplication with LoopbackLoginGuard's properties-line scan.

readCompressionThreshold reimplements a small ad hoc .properties line-scan already present in LoopbackLoginGuard.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

📥 Commits

Reviewing files that changed from the base of the PR and between 068c759 and 394311d.

📒 Files selected for processing (15)
  • lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java
  • lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java
  • lightkeeper-integration-tests/pom.xml
  • lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java
  • lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java
  • lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java
  • lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java
  • lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java
  • lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java
  • lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java
  • lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java
  • lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java
  • lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java
  • lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml
  • lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.41379% with 24 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ap2/lightkeeper/nms/v121r7/BotLoginReflection.java 0.00% 15 Missing ⚠️
...tkeeper/maven/provisioning/LoopbackLoginGuard.java 90.38% 3 Missing and 2 partials ⚠️
...16aap2/lightkeeper/nms/v121r7/BotLoginSession.java 0.00% 4 Missing ⚠️

📢 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
@PimvanderLoos
PimvanderLoos merged commit 87ef9fb into master Jul 16, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants