Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBumps two package versions; adds a new upgrade subsystem (operations, contexts, outcomes, RNG, DI, packet routing, and many tests); introduces an experience-progression service; converts map-item pickup events to snapshots; tightens battle/reward/quest/save/wear/skill/portal/movement logic; adds i18n key and many tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ClientSession
participant Handler as UpgradePacketHandler
participant Ops as IUpgradeOperation[]
participant Op as IUpgradeOperation (selected)
participant Inventory as Inventory/Player
participant Map as MapInstance
participant Net as PacketSender
rect rgba(200,200,255,0.5)
Client->>Handler: sends UpgradePacket (UpgradeType, payload)
Handler->>Ops: find operation where op.Kind == UpgradeType
note right of Ops: if none -> log UNHANDLED_UPGRADE_TYPE and return
Ops-->>Handler: matching Op
end
rect rgba(200,255,200,0.5)
Handler->>Op: await Op.ExecuteAsync(session, packet)
Op->>Inventory: validate source/target items & consume materials/gold
Op->>Map: emit visual/effect and drop packets
Op-->>Handler: returns list of IPacket (say/msgi/shopclose/pocket refresh)
end
rect rgba(255,200,200,0.5)
Handler->>Net: SendPacketsAsync(session, returned packets)
Net->>Client: deliver packets
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/NosCore.GameObject/Messaging/Handlers/MapItem/DropHandler.cs (1)
38-55:⚠️ Potential issue | 🔴 CriticalVerify map-item removal before awarding the drop.
Line 39 adds the item to the pocket, but Line 54 does not check whether
TryRemoveMapItem(visualId)actually succeeded. If two pickups race, this path can still grant the item even though the map item was already gone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.GameObject/Messaging/Handlers/MapItem/DropHandler.cs` around lines 38 - 55, The code awards the item before confirming removal of the map item: check the boolean result of MapInstance.TryRemoveMapItem(visualId) and only proceed with sending pocket-change, SayiPacket and GenerateIcon when TryRemoveMapItem returns true; if TryRemoveMapItem returns false, roll back the inventory addition (use session.Character.InventoryService to remove the InventoryItemInstance you added or avoid adding it until removal succeeds), avoid sending the success packets, and notify the player or ignore the pickup. Locate references to InventoryItemInstance.Create / session.Character.InventoryService.AddItemToPocket (inv variable) and MapInstance.TryRemoveMapItem in DropHandler.cs to implement this conditional-commit or rollback logic.src/NosCore.GameObject/Messaging/Handlers/MapItem/GoldDropHandler.cs (2)
54-64:⚠️ Potential issue | 🟠 MajorAvoid hard-failing the pickup on missing gold metadata.
items.First(i => i.VNum == vnum).Name[session.Account.Language]assumes both the gold item and the requested localization always exist. If either lookup misses, this throws after the gold balance was already mutated on Line 52.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.GameObject/Messaging/Handlers/MapItem/GoldDropHandler.cs` around lines 54 - 64, The current expression items.First(i => i.VNum == vnum).Name[session.Account.Language] can throw if the gold item or the requested localization is missing; change it to a safe lookup: find the item with items.FirstOrDefault(i => i.VNum == vnum), then check for null and whether Name contains session.Account.Language, and if either is missing set goldName to a sensible fallback (e.g., a default localized "Gold" string or the item's first available Name entry); ensure this check occurs before calling session.SendPacketAsync with the Sayi2Packet so pickup mutation does not cause an exception, and optionally log a warning (use the existing logger) when metadata is missing instead of hard-failing.
44-89:⚠️ Potential issue | 🔴 CriticalCheck
TryRemoveMapItembefore crediting gold.
character.Goldis updated and packets are sent before Line 82 attempts to remove the map item. If that removal fails, the player still keeps the gold, which duplicates the pickup and leaves client/server state inconsistent.Suggested fix
var character = session.Character; var visualId = evt.VisualId; var amount = evt.Amount; var vnum = evt.VNum; + var mapInstance = character.MapInstance; + + if (!mapInstance.TryRemoveMapItem(visualId)) + { + return; + } if (character.Gold + amount <= maxGold) { if (evt.Packet.PickerType == VisualType.Npc) { @@ } - character = session.Character; - var mapInstance = character.MapInstance; await session.SendPacketAsync(character.GenerateGold()); - mapInstance.TryRemoveMapItem(visualId); await mapInstance.SendPacketAsync(character.GenerateGet(visualId)); await session.SendPacketAsync(new CancelPacket { Type = CancelPacketType.CancelPicking, TargetId = visualId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.GameObject/Messaging/Handlers/MapItem/GoldDropHandler.cs` around lines 44 - 89, The code currently credits character.Gold and sends pickup packets before calling mapInstance.TryRemoveMapItem(visualId); change the flow so you attempt mapInstance.TryRemoveMapItem(visualId) first and only if it returns true do you update character.Gold, send the pickup notifications (GenerateIcon/Sayi2Packet or MsgiPacket), and then send GenerateGold/GenerateGet/CancelPacket; if TryRemoveMapItem returns false, do not modify character.Gold or send pickup confirmation—send a failure/rollback packet or nothing to keep server/client consistent. Ensure you update references in GoldDropHandler.cs around TryRemoveMapItem, character.Gold, GenerateIcon, Sayi2Packet/MsgiPacket, GenerateGold, GenerateGet, and CancelPacket accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/NosCore.GameObject/Services/QuestService/Handlers/KillQuestHandlerBase.cs`:
- Around line 27-29: In KillQuestHandlerBase the local vars
mobVNum/current/required are reused across the objectives loop so the
Sayi2Packet is built only for the last matched objective; change the logic to
either (A) inside the loop, after you update progress for a matched
AdvancedObjective, immediately create/send the Sayi2Packet (or call the existing
method that builds it) using that objective's mobVNum/current/required, or (B)
collect a small list of per-objective progress snapshots (mobVNum, current,
required, objectiveId) as you process each AdvancedObjective and then emit a
separate Sayi2Packet for each snapshot; ensure you reference the existing
Sayi2Packet construction path and the method handling AdvancedObjective updates
in KillQuestHandlerBase so each matched objective produces its own progress
toast rather than overwriting values.
In `@src/NosCore.GameObject/Services/UpgradeService/SumUpgradeOperation.cs`:
- Around line 65-88: Reject self-targeted sum by adding an explicit
distinct-item check: after resolving source and target (the variables `source`,
`target`, and their casted `WearableInstance s` and `t`) and before proceeding
to build the sum context, verify that `source.InventoryItemInstanceId` (or
another unique identifier on the inventory instance) is not equal to
`target.InventoryItemInstanceId`; if they are equal, return null. Place this
check in SumUpgradeOperation (around where `source`, `target`, `s`, and `t` are
validated) so the operation never attempts to sum an item into itself.
In `@src/NosCore.PacketHandlers/Command/SetGoldCommandPacketHandler.cs`:
- Line 44: The gold item lookup using items.First(i => i.VNum == GoldVNum) and
direct indexing into Name[session.Account.Language] can throw if the item or
localization is missing; update SetGoldCommandPacketHandler to use a safe lookup
(e.g., FirstOrDefault or single-match check) to get the item and then check for
null, and use a safe name retrieval (e.g.,
Name.TryGetValue(session.Account.Language, out var goldName) or fallback to a
default/neutral locale) before proceeding; if the item or localization is
absent, handle gracefully (send an error/notice to the session or abort the
command) instead of allowing an exception.
In `@test/NosCore.GameObject.Tests/Messaging/Handlers/Guri/MfaHandlerTests.cs`:
- Around line 15-18: The test fails due to ambiguous reference to GuriPacket
caused by importing both NosCore.Packets.ClientPackets.UI and
NosCore.Packets.ServerPackets.UI; update the using directives or add a type
alias to disambiguate (for example alias the server or client GuriPacket) and
then qualify the constructor call (the new GuriPacket usage in MfaHandlerTests
at line ~114) to use the intended type; ensure only the correct GuriPacket
(client or server) is referenced where GuriPacket is constructed so the CS0104
ambiguity is resolved.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/Map/MinilandEntranceHandlerTests.cs`:
- Around line 48-57: The test
EnteringOwnMinilandSendsMlinfoAndDoesNotIncrementVisitors asserts only
MlinfoPacketWasSent but the scenario text expects both mlinfo and mlobjlst;
update the Spec fluent assertion chain (in the test method and the other similar
test) to include .And(MlobjlstPacketShouldHaveBeenSent) after
.Then(MlinfoPacketShouldHaveBeenSent) so the object-list emission is verified;
the change should reference the existing Spec, HandlingEnterEvent,
MlinfoPacketShouldHaveBeenSent and MlobjlstPacketShouldHaveBeenSent helpers.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/BackPackHandlerTests.cs`:
- Around line 84-91: The spec text in FirstBackPackGrantConsumesItemAndAddsBonus
claims "and emits EffectActivated" but there is no assertion for that; update
the test by either adding an assertion that the EffectActivated event/packet was
emitted (e.g., chain an additional .And(EffectActivatedShouldBeEmitted) or
similar assertion helper used in other specs) or remove the "and emits
EffectActivated" phrase from the Spec description string so the test description
matches its assertions; locate the Spec instantiation in
FirstBackPackGrantConsumesItemAndAddsBonus and modify the
Given/WhenAsync/Then/And chain accordingly.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/BazaarMedalsHandlerTests.cs`:
- Around line 71-78: The GoldMerchantGrantsGoldBonusAndConsumesItem test is
missing the expiry assertion present in
SilverMerchantGrantsSilverBonusAndConsumesItem; update
GoldMerchantGrantsGoldBonusAndConsumesItem (the Spec chain that currently ends
with .Then(StaticBonusListShouldContain_,
StaticBonusType.BazaarMedalGold).And(ItemStackCountShouldBe_, (short)0)) to
include the same DateEnd assertion used in the silver test (i.e., add the same
.And(...) step that verifies the bonus DateEnd is set/has the expected value/is
in the future) so the gold flow checks expiry consistently.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/ChangeGenderHandlerTests.cs`:
- Around line 155-159: Remove the "new" member hiding in ItemInstanceForTest:
delete the duplicated Id and ItemVNum fields and instead initialize the
inherited properties via a constructor on ItemInstanceForTest (assign the base
DTO's Id and ItemVNum and populate Item there), so accesses through the base
type or IItemInstance see the same values as the test fixture; update any usages
to call the new constructor that accepts short vnum.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/HairDieHandlerTests.cs`:
- Around line 95-101: The test UndefinedColorFallsBackToDarkPurple must ensure
the pre-state isn't already DarkPurple and verify the item was consumed: update
the Spec setup (the Given clause that uses ItemInInventoryWithEffect_Value_) to
also set the actor's initial HairColor to a known non-DarkPurple value (e.g.,
HairColorType.SomeOther) and add an additional Then assertion after UsingTheItem
to confirm the inventory item was consumed (or count decreased) before
ExecuteAsync; keep the existing Then(HairColorShouldBe_,
HairColorType.DarkPurple) to verify fallback.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/SpeakerHandlerTests.cs`:
- Around line 110-118: The test GuriTextInputShouldBeSentForCurrentSlot
currently uses LastOrDefault on _session.LastPackets and can pass if multiple
GuriPacket entries of type GuriPacketType.TextInput exist; update the test to
assert there is exactly one matching packet (e.g., filter
_session.LastPackets.OfType<GuriPacket>().Where(p => p.Type ==
GuriPacketType.TextInput) and assert Count == 1) and then retrieve that single
packet (use Single() or SingleOrDefault with an assert) before asserting
Argument, SecondArgument, and EntityId to prevent false positives.
In
`@test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs`:
- Around line 209-212: The method ValidatingExchange is marked async but
contains no await (causing CS1998); remove the async modifier and make the
method synchronous by either changing its signature to a void (private void
ValidatingExchange()) or, if callers expect a Task, keep Task and return
Task.CompletedTask after calling _validationResult =
_realExchange!.ValidateExchange(_sessionA!, _sessionB!.Character); so you no
longer use an async state machine for ValidatingExchange.
In
`@test/NosCore.GameObject.Tests/Services/MinilandService/MinilandServiceTests.cs`:
- Around line 230-238: The test OldNosvillePortalIsWiredCorrectly currently
asserts destination instance but not destination map id; add an assertion that
p.DestinationMapId equals the expected map id (use the Miniland instance's map
id), e.g. Assert.AreEqual(_minilandInstance!.MapId, p.DestinationMapId) to
ensure the portal points to the correct destination map.
In `@test/NosCore.PacketHandlers.Tests/Battle/RevivalPacketHandlerTests.cs`:
- Around line 236-238: Remove the property hiding by deleting the new modifiers
on Id and ItemVNum in ItemInstanceForTest so they set the base ItemInstanceDto
properties instead; move the Guid.NewGuid() assignment and ItemVNum
initialization into the ItemInstanceForTest constructor and initialize Item
there (e.g., Item = new() { VNum = vnum, Type = NoscorePocketType.Main });
ensure ItemInstanceForTest implements IItemInstance (and can be sealed) and
retains a Clone implementation if needed so InventoryItemInstance.Create (and
its optional InventoryItemInstanceDto path) will read the correctly initialized
base properties.
In `@test/NosCore.PacketHandlers.Tests/Movement/SitPacketHandlerTests.cs`:
- Around line 134-147: The test SittingOnUnknownVisualId uses a hardcoded magic
VisualId (999999) which can become a real ID and make the test flaky; change the
test to use a stable non-colliding value (e.g., int.MaxValue or a dedicated
constant like UnknownVisualId) or generate an explicitly unused ID via a test
helper, and update the SitSubPacket creation in SittingOnUnknownVisualId (used
by Handler.ExecuteAsync with SitPacket and SitSubPacket, VisualType.Player) to
use that stable/derived value instead of 999999.
- Around line 87-95: The test AttemptingToSitAnotherPlayerDoesNotFlipTheirState
is nondeterministic because it only sets OtherPlayerIsStanding and doesn't fix
the caller's sit state or ensure the target is on-map; update the Spec setup to
explicitly place the target session on-map (e.g., add or replace
CharacterIsOnMap with OtherCharacterIsOnMap/OtherSessionOnMap helper) and
explicitly set the caller's initial sit state (use a helper like
CallerIsStanding or CallerIsSitting or add IsSittingPrecondition) before
WhenAsync(SittingOtherPlayer), then assert OtherPlayerIsSittingShouldBe_ false
and IsSittingShouldBe_ false so the test proves the handler ignores cross-player
sit toggles deterministically.
---
Outside diff comments:
In `@src/NosCore.GameObject/Messaging/Handlers/MapItem/DropHandler.cs`:
- Around line 38-55: The code awards the item before confirming removal of the
map item: check the boolean result of MapInstance.TryRemoveMapItem(visualId) and
only proceed with sending pocket-change, SayiPacket and GenerateIcon when
TryRemoveMapItem returns true; if TryRemoveMapItem returns false, roll back the
inventory addition (use session.Character.InventoryService to remove the
InventoryItemInstance you added or avoid adding it until removal succeeds),
avoid sending the success packets, and notify the player or ignore the pickup.
Locate references to InventoryItemInstance.Create /
session.Character.InventoryService.AddItemToPocket (inv variable) and
MapInstance.TryRemoveMapItem in DropHandler.cs to implement this
conditional-commit or rollback logic.
In `@src/NosCore.GameObject/Messaging/Handlers/MapItem/GoldDropHandler.cs`:
- Around line 54-64: The current expression items.First(i => i.VNum ==
vnum).Name[session.Account.Language] can throw if the gold item or the requested
localization is missing; change it to a safe lookup: find the item with
items.FirstOrDefault(i => i.VNum == vnum), then check for null and whether Name
contains session.Account.Language, and if either is missing set goldName to a
sensible fallback (e.g., a default localized "Gold" string or the item's first
available Name entry); ensure this check occurs before calling
session.SendPacketAsync with the Sayi2Packet so pickup mutation does not cause
an exception, and optionally log a warning (use the existing logger) when
metadata is missing instead of hard-failing.
- Around line 44-89: The code currently credits character.Gold and sends pickup
packets before calling mapInstance.TryRemoveMapItem(visualId); change the flow
so you attempt mapInstance.TryRemoveMapItem(visualId) first and only if it
returns true do you update character.Gold, send the pickup notifications
(GenerateIcon/Sayi2Packet or MsgiPacket), and then send
GenerateGold/GenerateGet/CancelPacket; if TryRemoveMapItem returns false, do not
modify character.Gold or send pickup confirmation—send a failure/rollback packet
or nothing to keep server/client consistent. Ensure you update references in
GoldDropHandler.cs around TryRemoveMapItem, character.Gold, GenerateIcon,
Sayi2Packet/MsgiPacket, GenerateGold, GenerateGet, and CancelPacket accordingly.
---
Nitpick comments:
In `@src/NosCore.GameObject/Messaging/Handlers/Battle/PlayerRevivalHandler.cs`:
- Around line 55-58: Extract the magic numbers in PlayerRevivalHandler into
named class constants: replace the inline 50000 threshold and 50L multiplier
with clearly named constants (e.g., ReputationThreshold and
ReputationMultiplier) defined at the top of the PlayerRevivalHandler class (use
the same numeric types as player.Reput/transfer, e.g., long for both), then use
those constants in the conditional (player.Reput >= ReputationThreshold) and
when computing transfer (player.Level * ReputationMultiplier) to centralize
tuning and avoid hard-coded domain rules.
In `@src/NosCore.GameObject/Services/BattleService/DamageCalculator.cs`:
- Around line 100-101: The critical damage computation in DamageCalculator.cs
directly applies context.MainCritHit/100.0 to baseDamage (affecting baseDamage
and isCritical) without any ceiling; update the calculation to cap the critical
multiplier to the intended maximum (e.g., limit the applied multiplier to a
sensible ceiling such as 3.0x or the prior cap) before applying it to
baseDamage, and add a short comment beside the calculation documenting the
expected MainCritHit range and the chosen cap to aid future maintainability.
In `@src/NosCore.GameObject/Services/SkillService/SkillService.cs`:
- Around line 76-94: Duplicate SkiPacket construction and skill ordering appears
in LoadSkill and LearnClassSkillsAsync; extract into a private helper (e.g.,
SendSkillPacketAsync or BuildAndSendSkillPacket) that takes the Character (or
its skills and classByte) and performs the ordering of
character.Skills.Values.Where(s => s.Skill != null).OrderBy(s =>
s.Skill!.CastId).Select(s => s.SkillVNum) and then sends the SkiPacket with
PrimarySkillVnum/SecondarySkillVnum computed from classByte and SkillVnums set
to the ordered list; replace the duplicated blocks in LoadSkill and
LearnClassSkillsAsync with calls to that helper to keep behavior identical.
In `@src/NosCore.GameObject/Services/UpgradeService/IUpgradeOperation.cs`:
- Around line 16-20: The interface IUpgradeOperation needs a clarified contract:
document (via XML comments on IUpgradeOperation and its members Kind and
ExecuteAsync) that each UpgradePacketType value must have exactly one
implementing operation (Kind is unique across registered handlers) and that
ExecuteAsync(ClientSession, UpgradePacket) MUST return a non-null
IReadOnlyList<IPacket> (an empty list is allowed but never null); update the
interface comments to state these guarantees and add guidance for
callers/registration code to validate uniqueness of Kind at startup (e.g.,
during DI/handler registration) and to enforce non-null return values from
implementations like ExecuteAsync.
In `@src/NosCore.GameObject/Services/UpgradeService/UpgradeContext.cs`:
- Around line 14-20: The UpgradeContext record currently exposes ExtraData as an
object which is type-unsafe; replace it with a dedicated marker interface or
generic parameter so callers must supply a well-known type rather than arbitrary
objects. Update UpgradeContext (e.g., UpgradeContext<TExtra> or UpgradeContext
with an IUpgradeExtraData? ExtraData) and adjust any callers that construct or
consume UpgradeContext, InventoryItemInstance-related upgrade handlers, and
UpgradeService methods to use the new typed ExtraData; preserve nullability if
needed and add explicit casts only at centralized boundaries where conversion
from legacy object is unavoidable.
In `@src/NosCore.PacketHandlers/Movement/PreqPacketHandler.cs`:
- Around line 85-97: Replace the long switch in IsPortalAccessible with a
centralized static readonly HashSet<PortalType> (e.g., private static readonly
HashSet<PortalType> AllowedPortals or s_allowedPortals) that lists the allowed
PortalType values and then return AllowedPortals.Contains(type) from
IsPortalAccessible; this centralizes the allowlist, makes it easier to update as
the PortalType enum changes, and keeps the IsPortalAccessible method a simple
containment check.
In `@src/NosCore.PacketHandlers/Upgrades/UpgradePacketHandler.cs`:
- Around line 31-36: Replace the non-deterministic FirstOrDefault resolution
with a fail-fast SingleOrDefault on the operations collection (change the lookup
that assigns operation from operations.FirstOrDefault(...) to use
SingleOrDefault) so duplicate Kind registrations surface immediately;
additionally handle the case where SingleOrDefault throws
InvalidOperationException (or detect duplicates via operations.Where(o => o.Kind
== packet.UpgradeType).Take(2).Count() > 1) by logging a clear error (include
packet.UpgradeType and context) and returning early, and keep the existing
null-path that logs logger.Warning when no match is found; update references in
UpgradePacketHandler where operation, operations and packet.UpgradeType are
used.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/Battle/PlayerRevivalHandlerTests.cs`:
- Around line 95-104: The test KilledByOtherPlayerWithHighReputTransfersPortion
should be extended to also assert that the killer receives the FD update packet
after the reputation transfer: after the WhenAsync call
(PlayerDiesKilledByOtherPlayer) and alongside the existing
Then/KillerReputationShouldBe_ check, add an assertion that verifies the handler
enqueued/sent the FD update packet to the killer (use whatever test helper or
mock verifies outgoing packets in this suite, e.g., check the mock socket or
message collector for an FD update packet for the killer character ID), ensuring
the new packet side-effect from PlayerRevivalHandler is covered.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/Guri/EmoticonHandlerTests.cs`:
- Around line 38-86: Add a positive-path test in the EmoticonHandlerTests class
that exercises HandlingGuri_ with a valid emoticon input so the suite doesn't
report false-green; create a new [TestMethod] (e.g.,
ValidEmoticonBroadcastsEffect) that uses the existing Spec helper:
.Given(EmoticonsAreAllowed) .WhenAsync(HandlingGuri_, GuriPacketType.TextInput,
973, _Owner()) and assert the opposite of the ignored cases by calling
Then(AnEffPacketShouldHaveBeenBroadcast) (or the existing equivalent assertion),
then ExecuteAsync(); follow the same pattern/fixtures as the other tests so it
compiles and runs with the test helpers used in the file.
In `@test/NosCore.GameObject.Tests/Messaging/Handlers/Guri/MfaHandlerTests.cs`:
- Around line 50-58: For each ignored-path test (WrongArgumentIsIgnored,
NonZeroVisualIdIsIgnored, AlreadyValidatedSessionIsIgnored,
AccountWithoutMfaSecretIsIgnored) add the additional assertion
NoMfaRePromptShouldBeSent to the Spec chain so the test verifies both that no
incorrect-password response is sent and that no MFA re-prompt occurs; locate the
Spec usages that currently call Then(NoIncorrectPasswordShouldBeSent) (they use
HandlingGuri_ with GuriPacketType.TextInput) and append
Then(NoMfaRePromptShouldBeSent) (or combine into a single Then call containing
both expectations) to lock down the ignored behavior.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/SetPlaceOfRevivalHandlerTests.cs`:
- Around line 86-115: Extract the repeated NrunPacket initializer into a single
helper (e.g., BuildSetPlaceOfRevivalPacket or GetSetPlaceOfRevivalPacket) and
have NrunIsHandledWithType_, NrunIsHandledWithType_AndNullTarget, and
NrunIsHandledWithType_AndGenericAliveTarget call that helper; the helper should
return a new NrunPacket with Runner = NrunRunnerType.SetPlaceOfRevival, Type set
from the caller, VisualType = VisualType.Npc, and VisualId = 1 so the three
helpers only supply the type and target.
- Line 23: The file-wide suppression "#pragma warning disable 618" is too broad;
narrow the scope by moving the suppression to just surround the obsolete usage
in the SetPlaceOfRevivalHandlerTests class (wrap only the specific line(s) or
statement(s) that reference the obsolete API) and immediately add "#pragma
warning restore 618" after those lines so other obsolete warnings in the file
are not masked; locate usages inside the SetPlaceOfRevivalHandlerTests test
methods and apply the disable/restore pair around them.
- Around line 63-81: Add a new positive test that uses the existing Spec flow to
exercise Type=1 with a valid NPC target: create a test (e.g.
ValidNpcTargetUpdatesRespawnAndSendsConfirmation) that calls
WhenAsync(NrunIsHandledWithType_AndNpcTarget, (short)1) and then asserts that
IRespawnService.SetRespawnPoint(...) was called exactly once with the expected
coordinates and map, and that the confirmation packet was emitted (use the same
Spec.Then/.And pattern and existing helpers or add new assertion helpers like
RespawnPointShouldHaveBeenUpdated and ConfirmationMsgShouldBeSent to locate the
call to IRespawnService.SetRespawnPoint and to verify the outgoing confirmation
message), then ExecuteAsync(); ensure you reference the real method name
IRespawnService.SetRespawnPoint and the Spec/WhenAsync/Then/ExecuteAsync flow
when adding the test.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/TeleporterHandlerTests.cs`:
- Around line 80-98: Extract the duplicated NrunPacket construction into a
private helper (e.g., CreateTeleportNrunPacket) and update both
NrunIsHandledWithType_AndNullTarget and
NrunIsHandledWithType_AndGenericAliveTarget to call that helper; the helper
should return an NrunPacket pre-populated with Runner = NrunRunnerType.Teleport,
VisualType = VisualType.Npc, VisualId = 1 and accept the variable Type (short
type) as a parameter so both _handler.HandleAsync calls use the same packet
instance construction logic.
- Around line 63-72: The test NonNpcTargetIsIgnored currently checks gold and
map change but misses asserting the absence of a NotEnoughGold message; update
TeleporterHandlerTests.NonNpcTargetIsIgnored to also assert no NotEnoughGold
message is produced (mirror the null-target test's message-side-effect check) by
adding the existing helper assertion used for messages (e.g., the
NoNotEnoughGold.../NoNotEnoughGoldMessageShouldHaveBeenInvoked helper) alongside
NoMapChangeShouldHaveBeenInvoked so the spec verifies gold, map, and message
side-effects.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/BoxEffectHandlerTests.cs`:
- Around line 138-140: RollPoolIs_ currently stubs _rollDao.Where(...) to always
return the full pool regardless of the predicate, so change the mock to be
predicate-aware: update the Setup call on _rollDao for Where to accept an
Expression<Func<RollGeneratedItemDto,bool>> (use
It.Is<Expression<Func<RollGeneratedItemDto,bool>>>(...)) and in the Returns
evaluate that expression against the provided pool (e.g., compile or use
pool.AsQueryable().Where(expr)) so the mock returns only items matching the
handler's filter; reference RollPoolIs_, _rollDao.Setup, Where, and
RollGeneratedItemDto when making the change.
- Around line 166-180: NoRdiPacketShouldBeSent currently only asserts no
RdiPacket was sent; extend it to also assert that no SayiPacket with Message ==
Game18NConstString.ItemReceived exists to catch regressions that emit chat
notifications. Update the method (referencing NoRdiPacketShouldBeSent,
_session.LastPackets, SayiPacket and Game18NConstString.ItemReceived) to keep
the existing RdiPacket assertion and add an assertion that
_session.LastPackets.OfType<SayiPacket>().None(p => p.Message ==
Game18NConstString.ItemReceived) (or equivalent Assert.IsFalse(...Any(...))) so
the test fails if an ItemReceived chat was emitted.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/PetBasketHandlerTests.cs`:
- Around line 128-135: The test TimedBasketSetsDateEnd currently only checks
StaticBonusDateEndShouldNotBeNull; change it to assert the actual expiry equals
now + 30 days (allowing a small tolerance, e.g., a few seconds) by replacing or
augmenting the Then step so it reads the saved DateEnd from the created item and
compares it to DateTime.UtcNow.AddDays(30) (or the equivalent test "now"
provider used in tests) using an approximate equality; reference the Spec flow
inputs ItemInInventoryWithEffect_, ItemEffectType.PetBasketUpgrade,
ItemHasEffectValueDays_ (30) and the action UsingTheItem to locate where to
assert the DateEnd.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/SealedTarotCardHandlerTests.cs`:
- Around line 64-68: The test Spec "EffectValue<=0 is a malformed tarot row: no
reward, no consumption" currently only asserts the tarot stack count via
Then(TarotStackShouldStillBe_, (short)1); add an additional Then assertion after
WhenAsync(UsingTheItem) to verify that no reward was granted (e.g., use the
existing "Then" helper that asserts player rewards/no reward — locate the Then
helper used elsewhere in SealedTarotCardHandlerTests such as any
Then(PlayerShouldHaveNoReward_, Then(NoRewardGranted_), or similar) and call it
in this spec so the test checks both "no consumption" and "no reward".
- Around line 129-133: The test double ItemInstanceForTest should stop hiding
base members Id and ItemVNum; remove the "public new" properties and add a
constructor on ItemInstanceForTest(short vnum) that assigns the base DTO
properties (Id and ItemVNum) and initializes Item (e.g., set base.Id =
Guid.NewGuid(); base.ItemVNum = vnum; Item = new Item { VNum = vnum, Type =
NoscorePocketType.Main }); update usages to rely on the base properties so the
class no longer masks ItemInstanceDto members.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/SpeakerHandlerTests.cs`:
- Around line 87-89: Replace the three hardcoded slot literals (7) in
SpeakerHandlerTests setup, packet input, and assertion with a single constant to
avoid drift; add a private const int (e.g., TestSlot = 7) near the test class or
fixture and use TestSlot wherever _item.Slot is set, where the input packet
references the slot, and where the assertion checks the resulting slot (locate
references in SpeakerHandlerTests using _item,
_session.Character.InventoryService and the packet creation/assertion lines).
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/SpRechargerHandlerTests.cs`:
- Around line 41-49: The SetupAsync test mutates the shared
TestHelpers.Instance.WorldConfiguration.Value.MaxAdditionalSpPoints without
restoring it; save the original value into a private field (e.g.,
_prevMaxAdditionalSp) inside SetupAsync before changing it, and implement a
[TestCleanup] method that resets
TestHelpers.Instance.WorldConfiguration.Value.MaxAdditionalSpPoints back to
_prevMaxAdditionalSp (or alternatively construct a test-local options instance
and pass that into the SpRechargerHandler constructor instead of mutating the
shared WorldConfiguration); reference the SetupAsync method, the
MaxAdditionalSpPoints property, and the SpRechargerHandler/_handler to locate
and apply the fix.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/TitleHandlerTests.cs`:
- Around line 66-75: The test helper ItemOfType_ currently always sets VNum =
TitleVNum which couples negative-path tests to the title VNum; change
ItemOfType_ so it assigns VNum conditionally (use TitleVNum when itemType
indicates a title, otherwise use a distinct non-title VNum constant or
parameter), then construct the ItemInstanceForTest with that chosen VNum and
continue creating _item via InventoryItemInstance.Create; update references to
TitleVNum, ItemOfType_, ItemInstanceForTest, InventoryItemInstance.Create and
_item accordingly.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/WearHandlerTests.cs`:
- Around line 55-135: Add explicit unit tests covering the ReputationMinimum
branch by creating two new Spec-based tests: one
"ReputationTooLowIsRejectedWithCanNotWearThat" that uses
Given(ItemInInventoryOfType_, ItemType.Armor), And(ItemRequiresReputation_,
<value>), And(CharacterHasReputation_, <lowerValue>), WhenAsync(UsingTheItem)
and Then(CanNotWearThatShouldHaveBeenSent); and one
"ReputationSufficientAllowsEquip" that uses Given(ItemInInventoryOfType_,
ItemType.Armor), And(ItemRequiresReputation_, <value>),
And(CharacterHasReputation_, <equalOrHigherValue>),
WhenAsync(UsingTheItemWithMode_, (byte)1) and Then(ItemShouldBeBoundToCharacter)
(and NoCanNotWearPacketSent) to assert successful equip; follow the same Spec
helpers and naming pattern as existing tests (Spec, Given, And, WhenAsync,
UsingTheItem, UsingTheItemWithMode_, Then).
In
`@test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs`:
- Around line 180-190: The test setup in SessionAndTargetAreSetUp calls
_realExchange.OpenExchange(...) but doesn't verify it succeeded, causing
downstream failures to be opaque; update SessionAndTargetAreSetUp to assert the
OpenExchange call succeeded (e.g., verify its return value or check exchange
state) immediately after calling _realExchange.OpenExchange with
_sessionA.Character.CharacterId and _sessionB.Character.VisualId so the test
fails fast if opening the exchange fails.
In `@test/NosCore.GameObject.Tests/ShopTests.cs`:
- Around line 128-136: The test
BuyingRejectsWhenReputPriceTimesAmountWouldOverflowLong currently only asserts
packet behavior; add an assertion that the character's reputation remains
unchanged after the attempted buy to guard against state mutation. Update the
Spec chain used in the test (the Given CharacterHasReputation_ setup and
WhenAsync AttemptingToBuyTwoOfExtremeReputPricedItemAsync action) to include a
Then step that verifies the character's reputation equals the original value
(the 9_999_999_999L provided by CharacterHasReputation_) so the spec ensures
reput immutability in the overflow rejection case.
In `@test/NosCore.PacketHandlers.Tests/Battle/RevivalPacketHandlerTests.cs`:
- Around line 200-205: The tests only assert HP restoration in
HpShouldBeFullyRestored and HpShouldBeRestoredToPercent_, so add corresponding
MP assertions: in HpShouldBeFullyRestored also assert that _session.Character.Mp
equals _session.Character.MaxMp, and in HpShouldBeRestoredToPercent_(int
percent) also assert that _session.Character.Mp equals Math.Max(1,
_session.Character.MaxMp * percent / 100); update method names or add new
helpers if needed to reflect HP/MP checks together (referencing
HpShouldBeFullyRestored and HpShouldBeRestoredToPercent_ to locate where to
insert the MP assertions).
In `@test/NosCore.PacketHandlers.Tests/Battle/UseSkillPacketHandlerTests.cs`:
- Around line 213-215: The test helper SkillWasUsedJustNow currently sets
_learnedSkill.LastUse = DateTime.Now which couples tests to local time; change
it to use DateTime.UtcNow (and update any assertions/handler usage to use
UtcNow) OR refactor to inject a controllable clock interface (e.g.,
IClock.NowUtc) into the handler and tests, set the fake clock's NowUtc in
SkillWasUsedJustNow, and update all places referencing LastUse/Now to use the
Utc clock accessor (e.g., IClock.NowUtc or DateTime.UtcNow) so cooldown
assertions are timezone-independent and testable.
In
`@test/NosCore.PacketHandlers.Tests/Command/ChangeChannelPacketHandlerTests.cs`:
- Around line 37-44: Add boundary-case coverage for ChannelId values by
extending the existing
ChangeChannelForwardsTheRequestedChannelIdToChannelService test: create
additional test cases (or convert to a data-driven test) that call the Spec with
ChangingChannelTo_ and MoveChannelAsyncShouldBeCalledWith_ for ChannelId 0 and
ChannelId 255 in addition to the existing nominal value 3, ensuring the
forwarding behavior is validated for lower and upper bounds; keep the same Spec
flow and assertions so the handler still calls IChannelService.MoveChannelAsync
with the exact ChannelId passed.
In
`@test/NosCore.PacketHandlers.Tests/Command/InvisibleCommandPacketHandlerTests.cs`:
- Around line 30-31: Replace the null-forgiving operator used when assigning
Session.Character.MapInstance in the test fixture with an explicit null
check/assertion so misconfigured fixtures fail with a clear diagnostic; locate
the setup where Session.Character.MapInstance =
TestHelpers.Instance.MapInstanceAccessorService.GetBaseMapById(1)! and change it
to assert the result is not null (e.g., capture the result from
GetBaseMapById(1), call Assert.NotNull or throw a descriptive exception if null)
before assigning to Session.Character.MapInstance, leaving the
InvisibleCommandPacketHandler() initialization unchanged.
In
`@test/NosCore.PacketHandlers.Tests/Command/SetGoldCommandPacketHandlerTests.cs`:
- Around line 47-66: Add a new unit test to cover the self-target branch where
the command's Name is null or equals the sender so local update/UI notification
(bypassing PubSub) is exercised: create a test like
SettingGoldForSelfShouldUpdateLocally that uses the existing Spec flow
(Given(CharacterIsOnMap) and a new or reused Given/And step TargetPlayerIsSelf
or TargetNameIsNull), call WhenAsync(SettingGoldForTargetPlayer) and assert the
local-update behavior with a new Then step StatDataShouldBeUpdatedLocally (or
similar) that checks the handler updated the sender's stats without publishing
to PubSub; reference the existing Spec helpers SettingGoldForTargetPlayer,
CharacterIsOnMap, and create/assert a new Then helper to verify local/UI
notification.
In
`@test/NosCore.PacketHandlers.Tests/Command/SetMaintenancePacketHandlerTests.cs`:
- Around line 64-67: The test currently only verifies that
ChannelHub.SetMaintenance(expectedGlobal, expectedMode) was called once but
doesn't assert there were no other calls; update the test method
ChannelHubSetMaintenanceShouldBeCalledWith_ to call
ChannelHub.VerifyNoOtherCalls() after the existing ChannelHub.Verify(...) call
so any unexpected extra invocations on the ChannelHub mock will fail the test.
- Around line 37-53: Two tests duplicate the same Spec flow with only different
input tuples; replace them with a single parameterized data-driven test. Create
a DataTestMethod (e.g., rename to SetMaintenanceForwardsFlags_DataDriven) and
add DataRow attributes for the two input tuples ([DataRow(true, true)] and
[DataRow(false, false)]), then inside the test call the same Spec chain using
the parameters with SettingMaintenance_ and
ChannelHubSetMaintenanceShouldBeCalledWith_ to forward the tuple values to the
existing assertions. Ensure you keep the Spec invocation and helper delegates
(SettingMaintenance_, ChannelHubSetMaintenanceShouldBeCalledWith_) unchanged so
only the test signature and attributes are modified.
In `@test/NosCore.PacketHandlers.Tests/Movement/PreqPacketHandlerTests.cs`:
- Around line 202-208: The assertion in
ChangeMapInstanceAsyncWasCalledWithPortalCoords_ uses It.IsAny<Guid>() so it
doesn't check the destination map instance id; change the helper to accept a
Guid expectedInstanceId (e.g.,
ChangeMapInstanceAsyncWasCalledWithPortalCoords_(Guid expectedInstanceId, short
expectedX, short expectedY)) and update the Verify call to assert the instance
id with It.Is<Guid>(g => g == expectedInstanceId) when verifying
MapChangeService.ChangeMapInstanceAsync, and update all test callers to pass the
expected instance id.
In `@test/NosCore.PacketHandlers.Tests/Movement/WalkPacketHandlerTests.cs`:
- Around line 143-146: The test currently mutates the shared MapInstance by
setting Session.Character.MapInstance.MapInstanceType =
MapInstanceType.NormalInstance in MapInstanceBecomesNormalInstance; instead
create and assign a dedicated non-shared MapInstance to the character (e.g.,
construct a new MapInstance with MapInstanceType =
MapInstanceType.NormalInstance, copying any required properties) and set
Session.Character.MapInstance = newInstance to avoid mutating a potentially
shared base object.
In `@test/NosCore.PacketHandlers.Tests/Shops/SellPacketHandlerTests.cs`:
- Around line 98-108: Extend the test SellingRejectsWhenSaleWouldBreachMaxGold
to explicitly assert character gold remains unchanged at MaxGoldAmount in the
rejection path: add an additional assertion after
Then(ShouldReceiveMaxGoldReachedMessage) (e.g. another .And(...) on the Spec)
that verifies the character's Gold equals MaxGoldAmount (use an existing helper
like ShouldHaveGoldAmount or implement a small helper assertion that checks the
character.Gold property against MaxGoldAmount) so the spec verifies message,
inventory and gold immutability.
In `@test/NosCore.PacketHandlers.Tests/Upgrades/UpgradePacketHandlerTests.cs`:
- Around line 104-105: The current WarningShouldHaveBeenLogged() verifier is too
loose — update the _logger.Verify call to assert the specific UpgradePacketType
(UpgradePacketType.FusionItem) and the expected localized template string
instead of It.IsAny<string>(); retrieve the expected template from the test's
localizer (the same key used when emitting the warning) and use that value in
the Verify (e.g., via It.Is<string>(s => s == expectedTemplate) or a regex
match) so the test only passes when the exact warning message and FusionItem
type are logged.
- Around line 52-70: Add a new test method in UpgradePacketHandlerTests that
verifies the handler forwards returned packets: create a Spec similar to the
existing tests (use Spec("Forwarded packet is dispatched when operation returns
a packet")), call .WhenAsync with a helper like
OperationThatReturnsForwardedPacketIsHandled (mirror
SumResistancePacketIsHandled/UnknownUpgradeTypePacketIsHandled), then assert the
forwarding behavior with a Then/And step such as
ForwardedPacketShouldHaveBeenSentExactlyOnce and
OtherOperationShouldNotHaveExecuted (reuse naming pattern
MatchingOperationShouldHaveExecutedExactlyOnce/NoOperationShouldHaveExecuted);
wire the spec to execute asynchronously so it integrates with the test harness
and mock dispatcher used elsewhere in this file.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 55c058d4-e381-4258-8b62-1a662dfcade3
📒 Files selected for processing (98)
Directory.Packages.propssrc/NosCore.Data/Enumerations/I18N/LanguageKey.cssrc/NosCore.Data/Resource/LocalizedResources.cs.resxsrc/NosCore.Data/Resource/LocalizedResources.de.resxsrc/NosCore.Data/Resource/LocalizedResources.es.resxsrc/NosCore.Data/Resource/LocalizedResources.fr.resxsrc/NosCore.Data/Resource/LocalizedResources.it.resxsrc/NosCore.Data/Resource/LocalizedResources.pl.resxsrc/NosCore.Data/Resource/LocalizedResources.resxsrc/NosCore.Data/Resource/LocalizedResources.ru.resxsrc/NosCore.Data/Resource/LocalizedResources.tr.resxsrc/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cssrc/NosCore.GameObject/Messaging/Events/MapItemPickedUpEvent.cssrc/NosCore.GameObject/Messaging/Handlers/Battle/PlayerRevivalHandler.cssrc/NosCore.GameObject/Messaging/Handlers/MapItem/DropHandler.cssrc/NosCore.GameObject/Messaging/Handlers/MapItem/GoldDropHandler.cssrc/NosCore.GameObject/Messaging/Handlers/MapItem/SpChargerHandler.cssrc/NosCore.GameObject/Messaging/Handlers/Nrun/CellonItemHandler.cssrc/NosCore.GameObject/Messaging/Handlers/Nrun/FinishedTsDialogHandler.cssrc/NosCore.GameObject/Messaging/Handlers/Nrun/UpgradeItemHandler.cssrc/NosCore.GameObject/Messaging/Handlers/Quest/OnCharacterMovedHandler.cssrc/NosCore.GameObject/Messaging/Handlers/UseItem/WearHandler.cssrc/NosCore.GameObject/Services/BattleService/DamageCalculator.cssrc/NosCore.GameObject/Services/BattleService/RewardService.cssrc/NosCore.GameObject/Services/ExperienceService/ExperienceProgressionService.cssrc/NosCore.GameObject/Services/ExperienceService/IExperienceProgressionService.cssrc/NosCore.GameObject/Services/QuestService/Handlers/KillQuestHandlerBase.cssrc/NosCore.GameObject/Services/QuestService/Quest.cssrc/NosCore.GameObject/Services/QuestService/QuestService.cssrc/NosCore.GameObject/Services/SaveService/SaveService.cssrc/NosCore.GameObject/Services/SkillService/ISkillService.cssrc/NosCore.GameObject/Services/SkillService/SkillService.cssrc/NosCore.GameObject/Services/UpgradeService/EquipmentUpgradeOperationBase.cssrc/NosCore.GameObject/Services/UpgradeService/IRandomNumberSource.cssrc/NosCore.GameObject/Services/UpgradeService/IUpgradeOperation.cssrc/NosCore.GameObject/Services/UpgradeService/RarifyOperation.cssrc/NosCore.GameObject/Services/UpgradeService/RarifyOperationBase.cssrc/NosCore.GameObject/Services/UpgradeService/SumUpgradeOperation.cssrc/NosCore.GameObject/Services/UpgradeService/UpgradeContext.cssrc/NosCore.GameObject/Services/UpgradeService/UpgradeItemOperation.cssrc/NosCore.GameObject/Services/UpgradeService/UpgradeOperation.cssrc/NosCore.GameObject/Services/UpgradeService/UpgradeOutcome.cssrc/NosCore.PacketHandlers/Command/SetGoldCommandPacketHandler.cssrc/NosCore.PacketHandlers/Game/GameStartPacketHandler.cssrc/NosCore.PacketHandlers/Inventory/GetPacketHandler.cssrc/NosCore.PacketHandlers/Inventory/RemovePacketHandler.cssrc/NosCore.PacketHandlers/Movement/PreqPacketHandler.cssrc/NosCore.PacketHandlers/Movement/WalkPacketHandler.cssrc/NosCore.PacketHandlers/NoAction/BpClosePacketHandler.cssrc/NosCore.PacketHandlers/NoAction/SortOpenPacketHandler.cssrc/NosCore.PacketHandlers/Upgrades/UpgradePacketHandler.cssrc/NosCore.WebApi/Controllers/AuthController.cssrc/NosCore.WorldServer/WorldServerBootstrap.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Battle/PlayerRevivalHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Guri/EmoticonHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Guri/MfaHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Map/MinilandEntranceHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/SetPlaceOfRevivalHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/TeleporterHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/UpgradeItemHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/BackPackHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/BazaarMedalsHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/BoxEffectHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/ChangeGenderHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/HairDieHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/MinilandBellHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/PetBasketHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/SealedTarotCardHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/SpRechargerHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/SpeakerHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/TitleHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/UseItem/WearHandlerTests.cstest/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cstest/NosCore.GameObject.Tests/Services/MapChangeService/MapChangeServiceTests.cstest/NosCore.GameObject.Tests/Services/MinilandService/MinilandServiceTests.cstest/NosCore.GameObject.Tests/Services/QuestService/QuestServiceTests.cstest/NosCore.GameObject.Tests/Services/SaveService/SaveServiceTests.cstest/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cstest/NosCore.GameObject.Tests/Services/UpgradeService/RarifyOperationTests.cstest/NosCore.GameObject.Tests/Services/UpgradeService/SumUpgradeOperationTests.cstest/NosCore.GameObject.Tests/ShopTests.cstest/NosCore.PacketHandlers.Tests/Battle/RevivalPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Battle/UseSkillPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Command/ChangeChannelPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Command/InvisibleCommandPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Command/SetGoldCommandPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Command/SetMaintenancePacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Command/TeleportPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Movement/PreqPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Movement/SitPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Movement/WalkPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Shops/SellPacketHandlerTests.cstest/NosCore.PacketHandlers.Tests/Upgrades/UpgradePacketHandlerTests.cstest/NosCore.Parser.Tests/DropParserTests.cstest/NosCore.Parser.Tests/ItemParserTests.cstest/NosCore.Parser.Tests/MapParserTests.cstest/NosCore.Parser.Tests/NpcMonsterParserTests.cstest/NosCore.Parser.Tests/QuestPrizeParserTests.cs
| { | ||
| session.Character.Gold = goldPacket.Gold; | ||
| await session.SendPacketAsync(session.Character.GenerateGold()); | ||
| var goldName = items.First(i => i.VNum == GoldVNum).Name[session.Account.Language]; |
There was a problem hiding this comment.
Guard the gold item/localization lookup to prevent runtime failure.
At Line 44, First(...) and direct language indexing can throw if the item list is incomplete or that locale key is missing, which crashes the self-target command path.
💡 Proposed fix
- var goldName = items.First(i => i.VNum == GoldVNum).Name[session.Account.Language];
+ var goldItem = items.FirstOrDefault(i => i.VNum == GoldVNum);
+ var goldName = goldItem?.Name != null &&
+ goldItem.Name.TryGetValue(session.Account.Language, out var localizedGoldName)
+ ? localizedGoldName
+ : "Gold";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var goldName = items.First(i => i.VNum == GoldVNum).Name[session.Account.Language]; | |
| var goldItem = items.FirstOrDefault(i => i.VNum == GoldVNum); | |
| var goldName = goldItem?.Name != null && | |
| goldItem.Name.TryGetValue(session.Account.Language, out var localizedGoldName) | |
| ? localizedGoldName | |
| : "Gold"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NosCore.PacketHandlers/Command/SetGoldCommandPacketHandler.cs` at line
44, The gold item lookup using items.First(i => i.VNum == GoldVNum) and direct
indexing into Name[session.Account.Language] can throw if the item or
localization is missing; update SetGoldCommandPacketHandler to use a safe lookup
(e.g., FirstOrDefault or single-match check) to get the item and then check for
null, and use a safe name retrieval (e.g.,
Name.TryGetValue(session.Account.Language, out var goldName) or fallback to a
default/neutral locale) before proceeding; if the item or localization is
absent, handle gracefully (send an error/notice to the session or abort the
command) instead of allowing an exception.
Comment on lines
+236
to
+238
| public new Guid Id { get; set; } = Guid.NewGuid(); | ||
| public new short ItemVNum { get; set; } = vnum; | ||
| public Item Item { get; set; } = new() { VNum = vnum, Type = NoscorePocketType.Main }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect create factory implementation and the relevant DTO/interface contracts.
fd -i 'InventoryItemInstance.cs' --exec sed -n '1,260p' {}
fd -i 'ItemInstanceDto.cs' --exec sed -n '1,260p' {}
fd -i 'IItemInstance.cs' --exec sed -n '1,260p' {}
# Locate exact symbol usages around ItemVNum/Id in creation path.
rg -n -C3 --type=cs '\bCreate\s*\(.*IItemInstance|\bCreate\s*\(.*ItemInstanceDto'
rg -n -C3 --type=cs '\bItemVNum\b|\bId\b'Repository: NosCoreIO/NosCore
Length of output: 50374
Remove new keyword from Id and ItemVNum properties in ItemInstanceForTest class.
The new modifier hides base properties from ItemInstanceDto, creating polymorphism issues. Code accessing these properties through the base interface (e.g., InventoryItemInstance.Create() with the optional InventoryItemInstanceDto parameter) will bypass the test values and read uninitialized defaults instead.
Set the base properties directly:
public sealed class ItemInstanceForTest(short vnum) : NosCore.Data.Dto.ItemInstanceDto, IItemInstance
{
public ItemInstanceForTest(short vnum)
{
Id = Guid.NewGuid();
ItemVNum = vnum;
Item = new() { VNum = vnum, Type = NoscorePocketType.Main };
}
public Item Item { get; set; }
public object Clone() => MemberwiseClone();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/NosCore.PacketHandlers.Tests/Battle/RevivalPacketHandlerTests.cs` around
lines 236 - 238, Remove the property hiding by deleting the new modifiers on Id
and ItemVNum in ItemInstanceForTest so they set the base ItemInstanceDto
properties instead; move the Guid.NewGuid() assignment and ItemVNum
initialization into the ItemInstanceForTest constructor and initialize Item
there (e.g., Item = new() { VNum = vnum, Type = NoscorePocketType.Main });
ensure ItemInstanceForTest implements IItemInstance (and can be sealed) and
retains a Clone implementation if needed so InventoryItemInstance.Create (and
its optional InventoryItemInstanceDto path) will read the correctly initialized
base properties.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NosCore.WebApi/Controllers/AuthController.cs (1)
100-109: Good authorization hardening; consider returningForbid()for SID mismatches.
[Authorize]here is the right security change. For claim mismatch,Forbid()is a clearer authz response thanBadRequest(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.WebApi/Controllers/AuthController.cs` around lines 100 - 109, In GetAuthCode in AuthController, replace the BadRequest(...) response used when the SID claim doesn't match platformGameAccount.PlatformGameAccountId with an authorization failure response (return Forbid()) so that claim mismatches produce a 403-style authz denial instead of a 400 bad request; locate the identity/claims check around GetAuthCode and swap the BadRequest(logLanguage[LogLanguageKey.AUTH_INCORRECT]) return for Forbid() (or Forbid(authenticationSchemes) if you need scheme-specific behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NosCore.WebApi/Controllers/AuthController.cs`:
- Around line 100-109: In GetAuthCode in AuthController, replace the
BadRequest(...) response used when the SID claim doesn't match
platformGameAccount.PlatformGameAccountId with an authorization failure response
(return Forbid()) so that claim mismatches produce a 403-style authz denial
instead of a 400 bad request; locate the identity/claims check around
GetAuthCode and swap the BadRequest(logLanguage[LogLanguageKey.AUTH_INCORRECT])
return for Forbid() (or Forbid(authenticationSchemes) if you need
scheme-specific behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c617517f-2390-4b5b-8be5-3e3f61b84969
📒 Files selected for processing (2)
src/NosCore.WebApi/Controllers/AuthController.cssrc/NosCore.WebApi/WebApiBootstrap.cs
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHub.cs (1)
55-68:⚠️ Potential issue | 🟠 MajorHandle malformed auth tokens as a clean auth failure.
This still throws on odd-length or non-hex input, so a bad client token can bubble out as a hub error instead of returning
null. Please make the decode path fail closed and letGetAwaitingConnectionAsyncreject the token cleanly.Possible shape of the fix
- private static string HexStringToString(string hexString) + private static string? HexStringToString(string hexString) { if (Guid.TryParse(hexString, out _)) { return hexString; } + + if ((hexString.Length & 1) != 0) + { + return null; + } - var bb = Enumerable.Range(0, hexString.Length) - .Where(x => x % 2 == 0) - .Select(x => Convert.ToByte(hexString.Substring(x, 2), 16)) - .ToArray(); - return Encoding.UTF8.GetString(bb); + try + { + var bb = Enumerable.Range(0, hexString.Length) + .Where(x => x % 2 == 0) + .Select(x => Convert.ToByte(hexString.Substring(x, 2), 16)) + .ToArray(); + return Encoding.UTF8.GetString(bb); + } + catch (FormatException) + { + return null; + } }- var sessionGuid = HexStringToString(token); + var sessionGuid = HexStringToString(token); + if (sessionGuid == null) + { + return Task.FromResult<string?>(null); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHub.cs` around lines 55 - 68, The hex-decode path that converts hexString to bytes can throw on odd-length or non-hex input and should instead fail closed; update the logic in AuthHub (the hexString decode block) to first verify hexString.Length % 2 == 0 and then perform a safe parse (either parse each pair with a hex TryParse or wrap Convert.ToByte in a try/catch), and on any parse error return null so GetAwaitingConnectionAsync receives a clean auth failure rather than allowing exceptions to bubble out.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHub.cs`:
- Around line 77-80: The AuthHub class exposes StoreAuthCodeAsync without any
authentication; secure it by adding authorization either via the [Authorize]
attribute on the AuthHub class (so AuthHub and its method StoreAuthCodeAsync
require authenticated callers) or by applying .RequireAuthorization() to the
MapHub<AuthHub>() registration in MasterServerBootstrap.cs (the
MapHub<AuthHub>() call referenced in the review). Ensure the chosen approach
enforces the same authentication scheme used by the app and update any tests or
client startup code to supply valid credentials if necessary.
In
`@src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHubClient.cs`:
- Around line 31-35: The StoreAuthCodeAsync method currently starts the
_hubConnection, then calls InvokeAsync, but if InvokeAsync throws the connection
isn't stopped; modify StoreAuthCodeAsync to start the connection, then call
InvokeAsync inside a try block and call await _hubConnection.StopAsync() in a
finally block so StopAsync always runs even on exceptions; reference the
existing method name StoreAuthCodeAsync and the fields/methods
_hubConnection.StartAsync, _hubConnection.InvokeAsync, and
_hubConnection.StopAsync when making the change.
In `@src/NosCore.WebApi/WebApiBootstrap.cs`:
- Around line 54-56: The authorization services are not registered even though
app.UseAuthorization() and [Authorize] in AuthController are used; update
WebApiBootstrap.cs to register authorization by calling
builder.Services.AddAuthorization() (e.g., after
builder.Services.AddAuthentication(...) and ConfigureJwtBearerOptions) so the
authorization middleware has the required services and policies available for
app.UseAuthorization() to enforce.
---
Outside diff comments:
In `@src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHub.cs`:
- Around line 55-68: The hex-decode path that converts hexString to bytes can
throw on odd-length or non-hex input and should instead fail closed; update the
logic in AuthHub (the hexString decode block) to first verify hexString.Length %
2 == 0 and then perform a safe parse (either parse each pair with a hex TryParse
or wrap Convert.ToByte in a try/catch), and on any parse error return null so
GetAwaitingConnectionAsync receives a clean auth failure rather than allowing
exceptions to bubble out.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f223f1de-306a-4013-a23b-41d40052cb36
📒 Files selected for processing (5)
src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHub.cssrc/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHubClient.cssrc/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/IAuthHub.cssrc/NosCore.WebApi/Controllers/AuthController.cssrc/NosCore.WebApi/WebApiBootstrap.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NosCore.WebApi/Controllers/AuthController.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NosCore.WebApi/WebApiBootstrap.cs`:
- Around line 66-67: AuthHubClient holds a HubConnection that is stopped but
never disposed; implement IAsyncDisposable on AuthHubClient and add an async
DisposeAsync implementation that awaits StopAsync (if started) and then awaits
_hubConnection.DisposeAsync(), nulling the field; then change the DI
registration for AuthHubClient (the
containerBuilder.RegisterType<AuthHubClient>().AsImplementedInterfaces()) to
ensure scope-based disposal (e.g., call InstancePerLifetimeScope() or the
per-request equivalent) so the container will call IAsyncDisposable.DisposeAsync
when the instance is released.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d5e0ef35-9345-4ee1-8870-7ed0bd74885f
📒 Files selected for processing (3)
src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHub.cssrc/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHubClient.cssrc/NosCore.WebApi/WebApiBootstrap.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHub.cs
- src/NosCore.GameObject/InterChannelCommunication/Hubs/AuthHub/AuthHubClient.cs
erwan-joly
added a commit
that referenced
this pull request
Apr 23, 2026
Gameplay / defensive: - SumUpgradeOperation: reject self-targeted sum (same slot/type twice would double-remove on failure or sum an item into itself on success). - KillQuestHandlerBase: emit one Sayi2 progress toast per advanced objective instead of overwriting per-loop locals and sending only the last one. - AuthHubClient: implement IAsyncDisposable so the underlying HubConnection is released when the client is disposed. Test improvements (coverage + determinism): - MinilandEntranceHandler: assert MlobjlstPacket in the own-miniland spec whose text claimed it. - BackPackHandler: drop the "and emits EffectActivated" phrase from the first-grant spec — no such assertion exists. - BazaarMedalsHandler: add StaticBonusDateEndShouldNotBeNull to the gold path so it mirrors the silver path coverage. - HairDieHandler: set a non-DarkPurple pre-state and assert item consumption in the fallback-color test. - SpeakerHandler: assert exactly one GuriTextInput packet instead of selecting the last match. - ExchangeServiceTests.ValidatingExchange: drop unused async (CS1998). - MinilandService: assert DestinationMapId for the Old Nosville portal. - SitPacketHandler: make the other-player test deterministic (explicit caller standing + other on same map); use int.MaxValue instead of hardcoded 999999 for the unknown-VisualId test. Skipped (not real bugs): - `new Guid Id`/`new short ItemVNum` in ~15 test fixtures — pattern works with current call sites and CodeRabbit's proposed constructor fix has a compile error alongside the primary constructor. - SetGoldCommandPacketHandler item/locale null-safety — $Gold is a GM-only command and this exact First(...).Name[lang] pattern is used across the Bazaar handlers; behavior is intentional. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
(Commit message corrected — the previous push carried an unrelated message from a parallel NosCore.PacketLogger change that landed in the wrong repo by mistake.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Gameforge Spark auth flow uses POST /api/v1/auth/sessions (no `thin` prefix) followed by POST /api/v1/auth/thin/codes. Our controller only exposed `sessions` under the `thin` / `api/Auth` prefixes, so third-party Gameforge auth clients hitting the bare `/api/v1/auth/sessions` path got a 404 even though the handler logic is identical. Adding an absolute route to the existing action keeps all current paths working (api/v1/auth/thin/sessions, api/Auth/sessions) while also matching what Gameforge's tooling expects.
The /api/v1/auth/thin/codes handler reads User.Identity.Claims to verify that the caller's JWT has a Sid claim matching the posted platformGameAccountId. That check has been silently failing on every call because the WebApi app never had the authentication middleware hooked up — only UseAuthorization() was registered, and no JWT bearer scheme was added, so the Authorization: Bearer header was ignored and User.Identity was always an unauthenticated ClaimsIdentity with no claims. Fix: - Register ConfigureJwtBearerOptions and AddJwtBearer so the existing JWT validation parameters (same ones MasterServer uses) apply. - Call UseAuthentication() before UseAuthorization() so the pipeline actually parses the token. - Add [Authorize] on GetAuthCode so the endpoint returns 401 for missing/invalid tokens instead of masking the problem as a 400 from the manual Sid claim check. sessions and MfaEnabled already carry [AllowAnonymous] so they stay open as intended.
Every NosCore process registered IAuthCodeService as a SingleInstance, so the auth code WebApi.AuthController.GetAuthCode stored never made it to the LoginServer process that has to validate it when NoS0577 arrives. The hub/client pattern was already in place for GetAwaitingConnectionAsync but the store side was missing. - IAuthHub gains StoreAuthCodeAsync(authCode, accountName); AuthHub implements it against the local IAuthCodeService (on MasterServer, that's the canonical store), AuthHubClient forwards it over SignalR. - AuthController calls authHub.StoreAuthCodeAsync instead of the local IAuthCodeService.StoreAuthCode and no longer needs IAuthCodeService in its constructor. - WebApi is reconfigured to register HubConnectionFactory + AuthHubClient (instead of the local AuthHub + local AuthCodeService), so codes issued here land in MasterServer's store and are visible to LoginServer's existing GetAwaitingConnectionAsync lookup.
The explicit SingleInstance registration of AuthCodeService was being
silently overridden by the `RegisterAssemblyTypes(... EndsWith("Service"))`
scan that ran after it — in Autofac the last registration wins for a
given interface, so IAuthCodeService ended up with the scan's default
(per-dependency) lifetime. Every call into the hub therefore resolved
a new AuthCodeService with an empty auth-code dictionary, so codes
stored via StoreAuthCodeAsync were never visible to the next call's
GetAwaitingConnectionAsync.
Exclude AuthCodeService from the assembly scan and register it as a
singleton afterwards so the shared dictionary survives across hub
method calls.
Also fix the tests broken by the earlier AuthController signature
change: AuthControllerTests constructs the controller without the
removed IAuthCodeService dependency and calls the renamed async
GetAuthCodeAsync, and MfaHandlerTests disambiguates GuriPacket against
the new ServerPackets.UI.GuriPacket added in NosCore.Packets 16.9.
- WebApi: register AddAuthorization() to back the UseAuthorization middleware that the [Authorize] on GetAuthCodeAsync relies on. - AuthHub: require authentication via [Authorize] on the class so unauthenticated SignalR callers can't spoof StoreAuthCodeAsync / SetAwaitingConnectionAsync against arbitrary account names. The HubConnectionFactory already signs a Root-scoped JWT with the shared WebApi password, so existing clients (AuthHubClient in WebApi / LoginServer / WorldServer) keep working. - AuthHubClient: wrap every StartAsync/InvokeAsync pair in try/finally so StopAsync still runs if the invoke throws — without this the HubConnection's underlying WebSocket/network resources leak when a call fails.
Gameplay / defensive: - SumUpgradeOperation: reject self-targeted sum (same slot/type twice would double-remove on failure or sum an item into itself on success). - KillQuestHandlerBase: emit one Sayi2 progress toast per advanced objective instead of overwriting per-loop locals and sending only the last one. - AuthHubClient: implement IAsyncDisposable so the underlying HubConnection is released when the client is disposed. Test improvements (coverage + determinism): - MinilandEntranceHandler: assert MlobjlstPacket in the own-miniland spec whose text claimed it. - BackPackHandler: drop the "and emits EffectActivated" phrase from the first-grant spec — no such assertion exists. - BazaarMedalsHandler: add StaticBonusDateEndShouldNotBeNull to the gold path so it mirrors the silver path coverage. - HairDieHandler: set a non-DarkPurple pre-state and assert item consumption in the fallback-color test. - SpeakerHandler: assert exactly one GuriTextInput packet instead of selecting the last match. - ExchangeServiceTests.ValidatingExchange: drop unused async (CS1998). - MinilandService: assert DestinationMapId for the Old Nosville portal. - SitPacketHandler: make the other-player test deterministic (explicit caller standing + other on same map); use int.MaxValue instead of hardcoded 999999 for the unknown-VisualId test. Skipped (not real bugs): - `new Guid Id`/`new short ItemVNum` in ~15 test fixtures — pattern works with current call sites and CodeRabbit's proposed constructor fix has a compile error alongside the primary constructor. - SetGoldCommandPacketHandler item/locale null-safety — $Gold is a GM-only command and this exact First(...).Name[lang] pattern is used across the Bazaar handlers; behavior is intentional. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
erwan-joly
force-pushed
the
feature-handler-fills
branch
from
6f31835 to
d888812
Compare
4 tasks
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Localization
Tests