A cast could put a character inside a wall - #2320
Merged
erwan-joly merged 2 commits intoAug 28, 2026
Merged
Conversation
WalkPacketHandler refuses a destination that is not walkable, along with four other checks - server speed, distance against speed/2, the coordinate checksum and travel time. UpdateAttackerPosition writes the same two fields from a client-supplied position with no check at all, so anything the walk path refuses can be had by sending it in a u_s packet instead. This adds back the one check that needs nothing the packet lacks: IsWalkable, exactly as WalkPacketHandler calls it. The distance and speed rules are deliberately left out - u_s carries no speed, and a per-step bound of speed/2 would refuse the teleport skills, which legitimately move a character further (Skill.dat declares them as BCard type 30 subtype 21, up to 8 fields for the Mage's Teleport). A null MapInstance still allows the move, so nothing changes for entities that have no map yet. TESTED. Builds clean. NOT covered by a unit test: reaching UpdateAttackerPosition needs a real MapInstance, whose constructor takes eleven dependencies, and the fixture for it would be larger than the fix. Saying so rather than implying otherwise. The equivalent guard was written and tested on the other code base this comes from, where the decision is a pure function. Not played - the servers were not started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. Walkthrough
ChangesBattle Position Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change prevents cast movement from placing a character in a non-walkable position while keeping movement validation consistent; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
erwan-joly
reviewed
Aug 27, 2026
| } | ||
| } | ||
|
|
||
| // WalkPacketHandler refuses a destination that is not walkable; this path wrote the same |
Contributor
Author
There was a problem hiding this comment.
Removed in c158583 — the comment is gone, the guard itself is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
erwan-joly
approved these changes
Aug 28, 2026
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
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.
u_scarries a position, andUpdateAttackerPositionwrote it straight intoPositionX/PositionY.WalkPacketHandlerrefuses a destination that is not walkable; this path had no such check, so the same movement was allowed by one road and refused by the other.The guard is the walk path's own
IsWalkable, so the two agree by construction rather than by two copies of a rule.Tested:
NosCore.GameObject.Tests442/442 andNosCore.PacketHandlers.Tests410/410, solution builds with zero warnings. Not played - I do not start the servers.Summary by CodeRabbit