fix(api): keep every equipment slot inside the array and off old clients - #76
Merged
Merged
Conversation
The equipment array was written when the game had six slots and was never grown, so the two that arrived since, body in 1.20.5 and saddle in 1.21.5, sit at ordinals six and seven and land outside it. Anyone handing one of those to setItem, getItem or clearSlot got an ArrayIndexOutOfBoundsException, which meant saddling a horse or putting armor on a wolf was not something a caller could express at all. Sizing the array from the enum means the next slot the game adds costs nothing here. Holding a slot is only half of it, because a slot travels in the packet as its ordinal from 1.9 onwards, and a version that predates the slot has no number it could map back to. Sending a saddle to a 1.21.4 client is a seven it will read as garbage. So createPacket now leaves out what the running server version does not know rather than trusting that every slot is safe everywhere, and isSlotSupported is public because a caller that wants to decide something else on the same question should not have to keep its own table of version numbers. It takes the version as an argument so the answer is a pure function of what is asked, with the running version filled in by the overload. The version check it would have used was inverted: verifyVersion threw when the server was newer than the version passed rather than older, so getOffhand raised InvalidVersionException on every server past 1.9 and its single caller had been dead for as long as it existed.
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change delegates version comparison to ChangesVersion compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Tofaa2
commented
Jul 31, 2026
Owner
LGTM |
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
WrapperEntityEquipment has held six item stacks since before the game had more than six equipment slots, and it indexes them by the slot's ordinal. Body arrived in 1.20.5 at ordinal six and saddle in 1.21.5 at ordinal seven, so both fall off the end of that array. The result is that setItem, getItem and clearSlot throw an ArrayIndexOutOfBoundsException for either of them, well before a packet is ever built, which means there is currently no way through this class to saddle a horse or to put armor on a wolf. Sizing the array from the enum rather than from a number typed in by hand also means the next slot Mojang adds costs nothing here.
Holding a slot is only half the problem, though. A slot travels inside the equipment packet as its ordinal from 1.9 onwards, so a server version that predates the slot has no number the client could map back to it: a saddle sent to a 1.21.4 client is a seven that reads as garbage. Because of that, createPacket now leaves out the slots the running server version does not know rather than assuming every slot is safe to send everywhere. The same was already true of the offhand below 1.9, which nothing on the sending side checked.
That check is exposed as isSlotSupported because callers deciding anything else on the same question should not have to keep a private table of version numbers, which is exactly what downstream ends up doing today. It takes the version as an argument so the answer is a pure function of what you ask about, with an overload that fills in the running version for the common case.
One thing turned up on the way. VersionChecker.verifyVersion compared the wrong way around: it threw when the server was newer than the version handed to it rather than older. Its only caller is getOffhand, which therefore raised InvalidVersionException on every server past 1.9, so the guard has been backwards for as long as it has existed and the fix here needed a version check that works.
Verified against packetevents 2.13.0 that body and saddle are ordinals six and seven, that a six long array throws on the latter, and that the slots left in the packet come out contiguous on 1.8.8, 1.9, 1.20.4, 1.20.5, 1.21.4, 1.21.5 and 1.21.11.
Summary by CodeRabbit
Bug Fixes
New Features