Repository navigation
Conversation
RequestExchangeType.Confirmed/Cancelled are only ever sent while InExchange is already true, but ExchangeRequestPacket's declared scope is InGame only (missing InTrade), so WorldPacketHandlingStrategy's generic scope gate silently rejected every confirm and cancel before the handler ran. Special-case those two request types past the gate. Separately, ExchangeRequestPacketHandler's Confirmed case had its success/failure branches inverted: a validated exchange (Success, null packet dict) took the branch that dereferences the packet dict, throwing on every legitimately successful confirm. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NosCoreIO/NosCore/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe change corrects exchange scope validation, self-target rejection, phantom offer handling, confirmation result branching, and destination inventory notifications. Tests cover self-exchange requests, zero-amount offer fillers, and merged partial stacks. ChangesExchange handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix Suggested reviewers: 🚥 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 |
…e receiver's amount ExcListPacket.SubPackets deserializes with a minimum of 3 default-valued fillers regardless of how many real sub-packets were sent, because the trailing list has no wire count prefix. Amount has a declared [Range(1, ...)], so a filler (Amount == 0) can never be a genuine offer - same phantom-entry shape as QstlistPacket/FinitPacket, filtered in ExcListPacketHandler the same way those are filtered client-side. Separately, ExchangeRequestPacketHandler's Requested case had no guard against a player targeting their own VisualId, letting a self-invite be sent, accepted, and opened. And ExchangeService.ProcessExchange built the receiving side's inventory notification from the sender's own item object instead of the item actually added to the receiver's inventory. Partial transfers mutate the sender's item in place when removing the offered amount, so the receiver was notified with the sender's post-removal remaining balance rather than their own real (possibly merged) total. The persisted inventory was always correct; only the notification packet was wrong. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
erwan-joly
reviewed
Sep 25, 2026
| } | ||
|
|
||
| if (session.HasSelectedCharacter && (attr.Scopes & Scope.InTrade) == 0 && session.Character.InExchangeOrShop) | ||
| var isExchangeLifecycleAction = packet is ExchangeRequestPacket |
Collaborator
There was a problem hiding this comment.
likely the fix shouldnt be here but in noscorepackets then
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.
Summary
ExchangeRequestPacketdeclaresScopes = InGameonly, butRequestExchangeType.Confirmed/Cancelledare only ever sent while a trade is already open (InExchange = true).WorldPacketHandlingStrategy.ValidateScopewas silently dropping every confirm and cancel before the handler ran because the packet lacksScope.InTrade. Special-cased those two request types past the gate.ExchangeRequestPacketHandler'sConfirmedcase had its success/failure branches inverted: a validated exchange (Success, null packet dict) took the branch that dereferences the packet dict, throwing on every legitimately successful confirm.Test plan
dotnet test test/NosCore.PacketHandlers.Tests --filter FullyQualifiedName~Exchange(11/11 pass)dotnet test test/NosCore.GameObject.Tests --filter FullyQualifiedName~Exchange(23/23 pass)dotnet test NosCore.sln(GameObject.Tests 538/538, PacketHandlers.Tests 414/414, WebApi.Tests 17/17, Database.Tests 11/11 all pass; unrelated pre-existing failures in Core.Tests/Parser.Tests over Bazaar resource keys and BCard vocabulary, untouched by this change)?? Generated with Claude Code
Summary by CodeRabbit