refactor: split the framework API into server/worlds/bots/events facets (D2/D3)#99
Merged
Conversation
Add the four facet interfaces that carve the ILightkeeperFramework god-interface into cohesive slices, per the D2/D3 2.0 API restructure: - IServerControl: command execution, console output, platform, filesystem access, process lifecycle, tick counter, captured server errors. - IWorlds: main world, create (defaults/spec), template load, builder. - IBots: join synthetic players (with/without UUID), builder. - IEvents: capture Bukkit events (thin today; grows in S6/S7). These interfaces are the new canonical API surface. Wiring the accessors and routing implementations through them lands in the follow-up refactor commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
…1 delegates Complete the D2/D3 evolve-in-place restructure: the facet accessors become the single canonical codepath and every flat v1 method becomes a deprecated delegate. - ILightkeeperFramework gains abstract server()/worlds()/bots()/events() accessors; each former v1 method is now a @deprecated(forRemoval = true) default that delegates into the matching facet. waitUntil() stays plain (no facet mapping); currentServerTick() is deprecated on the public interface but DefaultLightkeeperFramework keeps its concrete impl because the identical signature is also on the untouched IFrameworkGatewayView seam. - New package-private facade impls (ServerControlFacade/WorldsFacade/BotsFacade/ EventsFacade) own the moved method bodies via constructor-injected internals; the deleted v1 impls no longer exist on DefaultLightkeeperFramework, which retains only lifecycle/close/ensureOpen plumbing and the gateway seam. - Route in-repo production consumers (LightkeeperFrameworkAssert, FailureDiagnosticsWriter) through the facet accessors so the deprecated names have no remaining production callers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
… split Finish the D2/D3 restructure on the test side so the deprecated names have no remaining callers outside their dedicated delegate coverage. - Migrate every framework unit test and integration test that called a v1 method to the facet form (framework.worlds().main(), server().currentTick(), bots().join(...), events().capture(...), etc.), including the mock-based consumers that now stub framework.server() and its facet methods. - Add DeprecatedDelegateTest: a facet-stub ILightkeeperFramework proves every deprecated default delegates to the matching facet method (deprecation warnings suppressed only here). - Add ServerControlFacadeTest/WorldsFacadeTest/BotsFacadeTest/EventsFacadeTest: per-facade delegation + null/blank validation coverage. - Extend FrameworkApiVisibilityTest to pin the facade impls non-public with non-public constructors, consistent with the existing API-visibility pins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
Bring the README in line with the D2/D3 2.0 API: the Quick Start and Vault examples now use framework.worlds().main() and framework.bots().builder(), the Core Features bullets reference the facet forms (worlds().fromTemplate(...), server().stop()/start()/restart()/crash(), server().directory()/ pluginDataDirectory(...)), and a note flags the flat v1 names as deprecated-for-removal in favour of the server()/worlds()/bots()/events() facets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
Goal: fold the Opus review round into the facet pass. Collapses the duplicated tick read — ServerControlFacade.currentTick() now delegates to the framework's gateway-mandated currentServerTick(), restoring a single source of truth — and rewrites the shared-server-down error message (plus its test expectations) to steer users to server().crash()/ stop()/start()/restart() instead of the v1 names this branch deprecates. 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
📝 WalkthroughWalkthroughThe framework API is reorganized into ChangesFacet API and compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IntegrationTest
participant ILightkeeperFramework
participant WorldsFacade
participant BotsFacade
participant ServerControlFacade
participant EventsFacade
IntegrationTest->>ILightkeeperFramework: access worlds(), bots(), server(), events()
ILightkeeperFramework-->>IntegrationTest: return facet implementations
IntegrationTest->>WorldsFacade: main() or create(WorldSpec)
IntegrationTest->>BotsFacade: join(...) or builder()
IntegrationTest->>ServerControlFacade: executeCommand(...) or currentTick()
IntegrationTest->>EventsFacade: capture(eventClassName)
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors LightKeeper’s JUnit framework API away from a large “god-interface” (ILightkeeperFramework) into four focused facet interfaces—IServerControl, IWorlds, IBots, and IEvents—while preserving source/binary compatibility for one release via deprecated default-method delegates. This aligns the framework module’s public API with the v2 design direction without changing the protocol layer.
Changes:
- Introduces facet interfaces (
server(),worlds(),bots(),events()) and package-private internal facade implementations as the single canonical codepaths. - Keeps all v1 flat methods as
@Deprecated(forRemoval = true)default methods delegating to the facets, and migrates in-repo usage (tests + README examples) to the facet API. - Adds/updates unit tests to pin delegation correctness and ensure facade implementation visibility constraints.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates documentation/examples to use facet-based API and notes v1 deprecations. |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperWorldTemplateIT.java | Migrates template-world interactions to server()/worlds() facets. |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperSharedLifecycleIT.java | Migrates mainWorld() usage to worlds().main(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperServerLifecycleIT.java | Migrates lifecycle + directory access to server() and bots to bots(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperServerErrorCaptureIT.java | Migrates server error capture/commands to server().errors() + server().executeCommand(...). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperPermissionsIT.java | Migrates world/bot creation to worlds() and bots() facets. |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperMixedFreshLifecycleIT.java | Migrates mainWorld() calls to worlds().main(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperMethodFreshServerIT.java | Migrates mainWorld() call to worlds().main(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFreshServerIT.java | Migrates mainWorld() call to worlds().main(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFrameworkIT.java | Migrates world creation to worlds().create(...). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperExtensionIT.java | Migrates world creation to worlds().create() and main world to worlds().main(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperEntityQueryIT.java | Migrates server commands/tick reads to server() and world creation to worlds(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperDiagnosticsIT.java | Migrates to worlds().main(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperChatAndCancelIT.java | Migrates bots, event capture, and tick reads to bots()/events()/server(). |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperBotIT.java | Migrates bot/world/server/event usage to facet API. |
| lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperBlockStateIT.java | Migrates to worlds().create(...) / worlds().main(). |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/WorldsFacadeTest.java | Adds unit coverage for WorldsFacade behavior and validation. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/ServerControlFacadeTest.java | Adds unit coverage for ServerControlFacade command/output/platform/errors. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/FailureDiagnosticsWriterTest.java | Updates diagnostics tests to mock/use framework.server() facet. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/EventsFacadeTest.java | Adds unit coverage for EventsFacade capture/validation. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkServerErrorsTest.java | Migrates server error access to server().errors() and crash to server().crash(). |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkLifecycleTest.java | Migrates lifecycle/output calls to server() facet. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkGatewayTest.java | Migrates directory/template/tick access to facets (server(), worlds()). |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacadeTest.java | Adds unit coverage for BotsFacade join/builder behavior and validation. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/FrameworkApiVisibilityTest.java | Pins non-public visibility for internal facade implementations. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/DeprecatedDelegateTest.java | Adds contract test ensuring every deprecated v1 method delegates to its facet equivalent. |
| lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/assertions/HandleAssertionsTest.java | Updates assertion tests to use framework.server() facet for errors/output. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IWorlds.java | Introduces public worlds facet interface. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IServerControl.java | Introduces public server-control facet interface. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/WorldsFacade.java | Adds package-private canonical implementation for worlds facet. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/ServerControlFacade.java | Adds package-private canonical implementation for server facet. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/FailureDiagnosticsWriter.java | Routes diagnostics rendering through framework.server() facet. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/EventsFacade.java | Adds package-private canonical implementation for events facet. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFramework.java | Refactors framework to own shared internals and delegate public API to facet facades. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacade.java | Adds package-private canonical implementation for bots facet. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/ILightkeeperFramework.java | Adds facet accessors and deprecates v1 flat methods with default delegates. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IEvents.java | Introduces public events facet interface. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IBots.java | Introduces public bots facet interface. |
| lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/assertions/LightkeeperFrameworkAssert.java | Updates assertion helpers to use server() facet for output/platform/errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. 🚀 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". |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkLifecycleTest.java (1)
72-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest names still reference the deprecated v1 method names.
These test bodies now exercise
server().output()/crash()/stop()/start()/restart()(e.g., Lines 86, 108, 133, 159, 188, 208, 229-232, 273-276, 300, 332, 357, 378-384, 402-403, 426-431), but the test method names (serverOutput_...,crashServer_...,stopServer_...,startServer_...,restartServer_...) still refer to the old, now-deprecated flat API. As per path instructions,**/src/test/java/**/*.java: "Test method names must followmethodToTest_whatWeTest".Renaming to reflect the facet methods (e.g.
output_shouldReturnServerProcessOutputSnapshot,crash_shouldInvalidatePlayersAndKillProcess) would keep names aligned with what's actually under test.Also applies to: 93-114, 117-140, 143-165, 168-191, 194-211, 214-253, 256-281, 284-310, 313-338, 341-363, 366-386, 389-410, 413-433
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkLifecycleTest.java` around lines 72 - 90, Rename the affected test methods in DefaultLightkeeperFrameworkLifecycleTest to use the facet method under test rather than the deprecated flat API prefixes. Update names such as serverOutput_, crashServer_, stopServer_, startServer_, and restartServer_ to output_, crash_, stop_, start_, and restart_, while preserving each test’s existing behavior and descriptive suffix.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacade.java`:
- Around line 86-107: Update createFromBuilder in BotsFacade to emit the same
INFO lifecycle log as join() after the player is successfully created, using the
created player details and existing logging conventions. Keep player
registration and wrapping behavior unchanged.
In
`@lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/ServerControlFacade.java`:
- Around line 92-180: Update the lifecycle log messages in crash(), doStop(),
start(), and restart() to include actionable server context, using available
runtimeManifest values such as the server directory identifier and runtime
protocol version. Preserve the existing lifecycle actions and log levels while
making each message identify the affected runtime.
In
`@lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacadeTest.java`:
- Around line 102-106: Add unit tests in BotsFacadeTest for the validation paths
of BotsFacade.join: verify that passing a null uuid throws NullPointerException
mentioning "uuid", and passing a null world throws NullPointerException
mentioning "world". Use the existing framework setup and AssertJ exception
assertions, suppressing nullness warnings only where needed to intentionally
cross the non-null API boundary.
In
`@lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/WorldsFacadeTest.java`:
- Around line 97-101: Add a unit test alongside the existing WorldsFacade tests
covering WorldsFacade.create with a null WorldSpec. Intentionally bypass the
non-null API constraint, invoke framework.worlds().create(null), and assert that
it throws NullPointerException with a message containing “worldSpec”.
---
Outside diff comments:
In
`@lightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkLifecycleTest.java`:
- Around line 72-90: Rename the affected test methods in
DefaultLightkeeperFrameworkLifecycleTest to use the facet method under test
rather than the deprecated flat API prefixes. Update names such as
serverOutput_, crashServer_, stopServer_, startServer_, and restartServer_ to
output_, crash_, stop_, start_, and restart_, while preserving each test’s
existing behavior and descriptive suffix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 777e8447-b65f-4e84-9db5-6e3004315645
📒 Files selected for processing (39)
README.mdlightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IBots.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IEvents.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/ILightkeeperFramework.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IServerControl.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/IWorlds.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/assertions/LightkeeperFrameworkAssert.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/EventsFacade.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/FailureDiagnosticsWriter.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/ServerControlFacade.javalightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/WorldsFacade.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/DeprecatedDelegateTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/FrameworkApiVisibilityTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/assertions/HandleAssertionsTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/BotsFacadeTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkGatewayTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkLifecycleTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/DefaultLightkeeperFrameworkServerErrorsTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/EventsFacadeTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/FailureDiagnosticsWriterTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/ServerControlFacadeTest.javalightkeeper-framework-junit/src/test/java/nl/pim16aap2/lightkeeper/framework/internal/WorldsFacadeTest.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperBlockStateIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperBotIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperChatAndCancelIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperDiagnosticsIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperEntityQueryIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperExtensionIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFrameworkIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFreshServerIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperMethodFreshServerIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperMixedFreshLifecycleIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperPermissionsIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperServerErrorCaptureIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperServerLifecycleIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperSharedLifecycleIT.javalightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperWorldTemplateIT.java
Comment on lines
+92
to
+180
| @Override | ||
| public void crash() | ||
| { | ||
| framework.ensureOpen(); | ||
| LOG.log(System.Logger.Level.INFO, "LK_FRAMEWORK: Crashing Minecraft server."); | ||
| playerScopeRegistry.invalidateAll(); | ||
| agentClient.close(); | ||
| minecraftServerProcess.kill(); | ||
| framework.markServerDown(); | ||
| } | ||
|
|
||
| @Override | ||
| public void stop() | ||
| { | ||
| framework.ensureOpen(); | ||
| if (!minecraftServerProcess.isRunning()) | ||
| throw new IllegalStateException("Cannot stop the server because it is not running."); | ||
| doStop(); | ||
| } | ||
|
|
||
| /** | ||
| * Graceful-stop implementation without the running-state precondition. | ||
| * | ||
| * <p>Tolerates a server that died on its own after the caller's running check: player cleanup swallows | ||
| * per-player failures, the client close is idempotent, and the process stop handles dead processes. This is | ||
| * what lets {@link #restart()} avoid a check-then-act race on the running state. | ||
| */ | ||
| private void doStop() | ||
| { | ||
| LOG.log(System.Logger.Level.INFO, "LK_FRAMEWORK: Stopping Minecraft server gracefully."); | ||
| try | ||
| { | ||
| // Remove synthetic players through the live agent first so they quit cleanly instead of being | ||
| // persisted as offline players in the world's playerdata by the server's shutdown save. | ||
| playerScopeRegistry.cleanupAll(agentClient::removePlayer); | ||
| agentClient.close(); | ||
| } | ||
| finally | ||
| { | ||
| framework.markServerDown(); | ||
| minecraftServerProcess.stop(DefaultLightkeeperFramework.SHUTDOWN_TIMEOUT); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void start() | ||
| { | ||
| framework.ensureOpen(); | ||
| if (minecraftServerProcess.isRunning()) | ||
| throw new IllegalStateException("Cannot start the server because it is already running."); | ||
|
|
||
| LOG.log(System.Logger.Level.INFO, "LK_FRAMEWORK: Starting Minecraft server."); | ||
| minecraftServerProcess.start(DefaultLightkeeperFramework.STARTUP_TIMEOUT); | ||
| try | ||
| { | ||
| agentClient.rehandshake( | ||
| DefaultLightkeeperFramework.AGENT_CONNECT_TIMEOUT, | ||
| runtimeManifest.agentAuthToken(), | ||
| runtimeManifest.runtimeProtocolVersion(), | ||
| Objects.requireNonNullElse(runtimeManifest.agentJarSha256(), "") | ||
| ); | ||
| framework.preloadConfiguredWorlds(); | ||
| } | ||
| catch (Exception exception) | ||
| { | ||
| // Return to a clean 'down' state so a retry of start() remains possible; leaving the process running | ||
| // here would wedge the start path (running + down means only restart() could recover). | ||
| try | ||
| { | ||
| minecraftServerProcess.stop(DefaultLightkeeperFramework.SHUTDOWN_TIMEOUT); | ||
| } | ||
| catch (Exception stopException) | ||
| { | ||
| exception.addSuppressed(stopException); | ||
| } | ||
| throw exception; | ||
| } | ||
| framework.markServerUp(); | ||
| } | ||
|
|
||
| @Override | ||
| public void restart() | ||
| { | ||
| framework.ensureOpen(); | ||
| LOG.log(System.Logger.Level.INFO, "LK_FRAMEWORK: Restarting Minecraft server."); | ||
| if (minecraftServerProcess.isRunning()) | ||
| doStop(); | ||
| start(); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Lifecycle log messages lack actionable identifiers.
crash(), doStop(), start(), and restart() log generic messages (e.g., "Crashing Minecraft server.") without identifiers like the server directory or runtime protocol version, even though runtimeManifest is already a field on this class.
As per coding guidelines, **/src/main/java/**/*.java: "Log at lifecycle boundaries such as start, stop, major transitions, and failures... include actionable context such as actions, identifiers, paths, and protocol versions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@lightkeeper-framework-junit/src/main/java/nl/pim16aap2/lightkeeper/framework/internal/ServerControlFacade.java`
around lines 92 - 180, Update the lifecycle log messages in crash(), doStop(),
start(), and restart() to include actionable server context, using available
runtimeManifest values such as the server directory identifier and runtime
protocol version. Preserve the existing lifecycle actions and log levels while
making each message identify the affected runtime.
Source: Coding guidelines
Owner
Author
There was a problem hiding this comment.
Declining for this PR for the same behavioral-equivalence reason — these messages moved verbatim from DefaultLightkeeperFramework. Enriching lifecycle logs with the server directory/protocol version is a good standalone follow-up.
Contributor
There was a problem hiding this comment.
@PimvanderLoos, understood — makes sense to keep this PR's log messages behaviorally equivalent to the original and defer enrichment to a follow-up. Let me know if you'd like me to open a tracking issue for enriching the lifecycle log messages with server directory/protocol version, and I'll file it.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Owner
Author
|
@codex review |
Goal: address CodeRabbit's round-one findings on the facet PR — the join(uuid/world) and create(worldSpec) fail-fast paths were validated in the facades but not pinned by tests. Adds the three missing NullPointerException tests following the existing NullAway-suppressed boundary-crossing pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
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
ILightkeeperFrameworkhad grown into a 23-method god-interface; D2 of the API v2 design calls for facets as the long-term shape, and D3 for migrating in place while there is still only one consumer.How
IServerControl/IWorlds/IBots/IEvents, reached viaserver()/worlds()/bots()/events(); implementations are package-private facade classes ininternalholding the moved (line-for-line) method bodies — the single canonical codepath.@Deprecated(forRemoval = true)default method delegating into its facet, so AA's existing call sites stay source- and binary-compatible for one release.PlayerHandle→BotHandlerename deliberately deferred to the S6 login driver.Tests
mvn --batch-mode -Perrorprone clean verify checkstyle:checkstyle pmd:check→ BUILD SUCCESS (full reactor incl. both IT lanes).DeprecatedDelegateTestproves every v1 default delegates (argument + return pass-through,verifyon the facet); four*FacadeTestclasses cover the moved logic;FrameworkApiVisibilityTestpins facade visibility.🤖 Generated with Claude Code
Summary by CodeRabbit