fix(spigot): support ViaVersion 5.x legacy initializer unwrapping#67
Merged
Merged
Conversation
On plain Spigot/CraftBukkit, ViaVersion wraps the server's child handler with BukkitChannelInitializer (its legacy injector; on Paper it registers a ChannelInitializeListener instead, which Connect picks up for free). SpigotInjector unwrapped that with getOriginal(), which ViaVersion 5.0 renamed to original() and pulled up into WrappedChannelInitializer. Connect compiled against Via 4.0.0, so this built fine and threw NoSuchMethodError at runtime with any Via >= 5.0. That NoSuchMethodError is an Error, so it escaped the catch (Exception) in ConnectPlatform.enable(), escaped onEnable(), and Bukkit disabled the plugin: Connect never bound its local channel and no Connect player could join at all. Latent since ViaVersion 5.0 and present in every release since. Resolve the accessor reflectively by name (original(), then getOriginal()) so both Via majors keep working, and degrade to skipping the unwrap - with a warning - if a future rename removes both, rather than refusing to enable. Also widen the catch in ConnectPlatform.enable() to Throwable so any injector Error stays a contained, logged injection failure instead of a plugin disable. The compile-only Via dep moves 4.0.0 -> 5.11.0 (plus viaversion-common, which now holds the ViaChannelInitializer superclass); it is platform-provided, so this does not change what the shaded jar ships or the minimum supported Via. Verified: the shipped connect-spigot.jar unwraps correctly against both Via 4.0.0 and Via 5.11.0, and no longer references getOriginal() in its constant pool.
…focused core test
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.
Intent
Fix a real, currently-shipping connect-java bug proven by a prior triage: on plain Spigot/CraftBukkit with ViaVersion >= 5.0, Connect fails to enable at all and NO player can join.
Root cause (already diagnosed and reproduced against the shipped 0.12.3 bytecode with Via 5.11.0, three independent proofs): SpigotInjector.getChildHandler() called ((BukkitChannelInitializer) childHandler).getOriginal() to unwrap ViaVersion's legacy (non-Paper) channel-initializer wrapper so Connect does not double-inject Via's handlers. ViaVersion 5.0 renamed that accessor to original() and pulled it up into the WrappedChannelInitializer interface / ViaChannelInitializer. connect-java compiled against the compile-only Via 4.0.0 pin (Versions.kt), so it built fine and threw NoSuchMethodError at runtime. NoSuchMethodError is an Error, not an Exception, so it escaped ConnectPlatform.enable()'s catch (Exception), escaped onEnable(), and Bukkit disabled the plugin outright. Paper and Paper forks are NOT affected: there Via registers a Paper ChannelInitializeListener instead of wrapping the child handler, so the unwrap branch is dead code. Latent since Via 5.0, present in every release.
Deliberate decisions made (so these are intended, not oversights):
Mandatory captain condition - a signature-level regression test that provably fails pre-fix and passes post-fix, matching the repo convention beside BedrockVelocityGuice7ProvisioningTest:
Deliberately OUT OF SCOPE per the captain, do not flag as missing work: the triage's secondary observations are separate follow-up tickets - PacketHandlerAddon's unguarded pipeline.addAfter("encoder", ...) landmine, DebugAddon being a silent no-op on modern Paper, and adding ViaVersion to spigot plugin.yml softdepend. Also intentionally NOT referenced or closed: minekube/connect issue GeyserMC#96 (the triage could not reproduce that Paper-specific report and closing it is an outward-facing decision the captain owns), so no issue reference belongs in the commit or PR. AGENTS.md (CLAUDE.md symlink) gained a concise 'Third-party platform APIs (ViaVersion & friends)' section recording the reflective-boundary convention, the Paper-vs-Spigot injection split, and the catch-Throwable containment rule.
What Changed
original()first, thengetOriginal()) so plain Spigot supports Via 5.x while retaining Via 4.x compatibility.Errors during platform enablement with logged failures, preventing reflective API drift from escapingonEnable().Risk Assessment
✅ Low: The wording fixes are now coherent and the full source change remains bounded with no additional material issues identified.
Testing
Exercised the real ViaVersion 5.11 legacy unwrap path, unknown-API degradation, non-Via control, and catch-Throwable containment; inspected the shaded Spigot artifact, which contains no ViaVersion classes or direct getOriginal() bytecode call. Evidence was saved outside the worktree; no UI screenshot applies to this server-side plugin behavior.
Evidence: Focused validation log
/var/folders/1y/cjgf53nj31n_dxsspqnjfjvc0000gn/T/no-mistakes-evidence/01KYF4RTB3NYAN6G4FGYHZTX2A/connect-spigot.jar)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
core/src/main/java/com/minekube/connect/ConnectPlatform.java:136- The newcatch (Throwable)atcore/src/main/java/com/minekube/connect/ConnectPlatform.java:136returnsfalse, butSpigotPlatform.enable()immediately callsdisablePlugin(plugin)wheneversuper.enable()returns false (spigot/src/main/java/com/minekube/connect/SpigotPlatform.java:54-58). Thus a future injectorErroris caught and logged but still disables Connect, contradicting the required criterion that it be “a logged, contained injection failure instead of a plugin disable.”🔧 Fix: Clarified injector-failure wording and verified focused core test
2 warnings still open:
core/src/main/java/com/minekube/connect/ConnectPlatform.java:139- The updated inline comment leaves the old fragment “Letting that escape onEnable() makes the platform” immediately before “Catch it here…”, producing a malformed and misleading explanation of the catch behavior. Rewrite the paragraph as one coherent statement.spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java:339- This Javadoc says every failure degrades, but then claims a missing accessor surfaces asNoSuchMethodErrorand causes enablement failure. With reflective lookup, missing names produceNoSuchMethodException, are skipped, and the wrapper is returned with a warning. Mark theNoSuchMethodErrorstatement as the pre-fix behavior or remove it to keep the operational documentation accurate.🔧 Fix: Fixed malformed catch comment and stale unwrap Javadoc
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
./gradlew :spigot:test --tests com.minekube.connect.inject.spigot.SpigotInjectorViaLegacyPathTest :core:test --tests com.minekube.connect.ConnectPlatformEnableFailureContainmentTest./gradlew :spigot:shadowJar --console=plainShaded JAR inspection for packaged ViaVersion classes and directgetOriginal()Methodref entries✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.