Sum, Upgrade, and Rarify operations on shared UpgradeOperation skeleton#2077
Merged
erwan-joly merged 31 commits intomasterfrom Apr 22, 2026
Merged
Sum, Upgrade, and Rarify operations on shared UpgradeOperation skeleton#2077erwan-joly merged 31 commits intomasterfrom
erwan-joly merged 31 commits intomasterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
erwan-joly
force-pushed
the
feature-handler-fills
branch
from
cc63a9a to
bdd06d6
Compare
erwan-joly
force-pushed
the
feature-handler-fills
branch
from
e8983ae to
cb42816
Compare
Introduces the up_gr packet handler and a reusable base for every
UpgradePacketType variant. SumResistance lands as the first concrete
operation; rarify / cellon / sp upgrade / fusion can be added by
subclassing UpgradeOperation without touching dispatch.
Shared skeleton (UpgradeOperation) handles the common flow that all
variants need: validate the input slots, check gold + materials, roll
for success, mutate or destroy items, then emit the standard pocket
refresh + AfterSumming guri animation + result message + close-window
packet. Subclasses fill in TryPrepareContext, GetSuccessRate,
ApplySuccess, ApplyFailure and the success/failure message keys.
Sum-specific (SumUpgradeOperation):
- Combined upgrade level (source.Upgrade + target.Upgrade) must be
below 6; anything higher is rejected upfront with no charges.
- On success, source.Upgrade becomes source + target + 1, the four
elemental resistances are summed, and the target slot is consumed.
- On failure, source is destroyed; target is consumed either way.
- Sand (vnum 1027) and gold are charged regardless of outcome.
- Probabilities/costs are baked in (1.00/1.00/0.90/0.80/0.50/0.20
success rate; 500/1.5k/3k/6k/12k/24k gold; 1..6 sand) — match the
legacy server config; tune via the SuccessRate/Gold/SandCost arrays
in SumUpgradeOperation.
Concurrency: WorldPacketHandlingStrategy already wraps every packet
handler in a per-session AsyncLock, so two upgrade packets from the
same client cannot interleave. No additional locking at this layer.
UpgradePacketHandler dispatches by UpgradePacketType to whichever
IUpgradeOperation declares a matching Kind. Unknown variants log a
warning (UNHANDLED_UPGRADE_TYPE) and no-op.
DI: WorldServerBootstrap auto-registers all IUpgradeOperation impls
from the GameObject assembly + the default RandomNumberSource.
IRandomNumberSource is a thin indirection over Random.Shared so tests
can stub the dice roll deterministically.
Tests:
- SumUpgradeOperationTests (5): success path mutates+consumes, failure
path destroys+consumes, level-cap rejection, insufficient gold
rejection, insufficient sand rejection.
- UpgradePacketHandlerTests (2): dispatch picks the matching op,
unknown type logs and no-ops.
- 622/622 total tests green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two more UpgradePacketType variants land on the shared UpgradeOperation
skeleton — total 5 of 19 dispatch types now wired.
Equipment upgrade (EquipmentUpgradeOperationBase + UpgradeItemOperation
+ UpgradeItemProtectedOperation):
- Increases a wearable's Upgrade level by +1 (cap +10).
- Materials: Cellon (vnum 1014). Protected variant doubles the cellon
cost in exchange for not degrading the item on failure.
- Unprotected failure: Upgrade decrements by 1 (floor 0).
- Protected failure: Upgrade unchanged.
Rarify (RarifyOperationBase + RarifyOperation + RarifyProtectedOperation):
- Increases a wearable's Rare stat by +1 (cap +8).
- Materials: Red Stellar (vnum 1024) for unprotected, Blue Stellar
(1025) for protected. One stone per attempt.
- Unprotected failure: Rare resets to 0.
- Protected failure: Rare unchanged.
- Negative-rarity (cursed) items are not rarifiable.
Both share the IsProtected hook; FailureMessage swaps between the
generic UpgradeFailed and the protection-specific RarityUnchangedProtectionScroll
/ UpgradeFailedButProtected so players see the right feedback.
Probabilities/costs are baked into the SuccessRate*/Gold*/Material*
arrays at the top of each base — match legacy server config; tune in
place + update the matching tests.
Concurrency continues to ride on WorldPacketHandlingStrategy's
per-session AsyncLock — no extra locking at this layer.
DI: WorldServerBootstrap's existing IUpgradeOperation assembly scan
picks up all four new types automatically.
Tests:
- EquipmentUpgradeOperationTests (5): success, unprotected failure,
protected failure (double cellon, no degrade), max-level rejection,
floor-0 underflow safety.
- RarifyOperationTests (5): success (red stellar consumed), unprotected
failure (resets to 0), protected failure (blue stellar consumed,
rare preserved), max-rarity rejection, negative-rarity rejection.
- 632/632 total tests green.
Deferred (need their own subsystems):
- All UpgradeSp* variants (need SP/transformation system)
- CreateFairy* variants (need fairy creation system)
- CellonItem (needs cellon stat plumbing on wearables)
- FusionItem (rare cross-item fuse)
- ItemToPart (needs disassembly recipes)
- UpgradeItemGoldScroll (cheap follow-up: same as UpgradeItem with
different material)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without these, talking to an upgrade NPC was a silent no-op — the n_run
packet had no handler to respond, so the upgrade window never opened
and the up_gr packet flow that UpgradePacketHandler dispatches into
IUpgradeOperation was unreachable in-game.
OpenNos handles both via NRunHandler.cs:90-92 and 206-207:
case 2: Session.SendPacket("wopen 1 0");
case 12: Session.SendPacket($"wopen {packet.Type} 0");
Two new Wolverine handlers mirror that:
- UpgradeItemHandler — n_run 2 -> wopen WindowType.UpgradeItem (1).
The basic-upgrade NPC. Triggers the up_gr UpgradeItem flow.
- ProbabilityUIsHandler — n_run 12 with packet.Type carrying the
target WindowType -> wopen <type> 0. The "any probability window"
NPC. This is how a tester reaches Sum / Rarify / RarifyProtected /
UpgradeItemProtected / FusionItem / etc. — all share this NPC and
differ only by the Type byte the client sends.
Both are minimal: filter by Runner + non-null target, then emit one
WopenPacket. No state changes; the heavy lifting still happens in
UpgradePacketHandler -> IUpgradeOperation when the client sends up_gr.
DI: WorldServerBootstrap's existing IUpgradeOperation/Wolverine handler
scan picks both up automatically (no DI changes needed).
Tests:
- UpgradeItemHandlerTests (3): runner=2 opens wopen 1 0, other runners
ignored, null target ignored.
- ProbabilityUIsHandlerTests (4): runner=12 + Type=8 opens sum window,
Type=7 opens rarify window, null Type ignored, other runners ignored.
689/689 tests green (was 682 before this commit on this branch).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes:
1. SumUpgradeOperation now matches OpenNos's WearableInstance.Sum
(lines 609-665) instead of my earlier guesses.
Behavior changes:
- Slot restriction: both items must occupy the SAME equipment slot
AND that slot must be Boots OR Gloves. Sum is only meaningful for
boots/gloves in Nostale (those are the slots with elemental
resistances). Armor/Weapon/etc. now rejected silently.
- Cost arrays come from OpenNos and use the correct index domains:
success rate (combined level): 1.00, 1.00, 0.85, 0.70, 0.50, 0.20
gold (source level): 1500, 3000, 6000, 12000, 24000, 48000
sand (source level): 5, 10, 15, 20, 25, 30
My old tables were both wrong-valued and wrong-indexed (every
cost row was indexed by combined level — OpenNos uses combined
level only for the success-rate roll; gold + sand are charged
against the source's level).
- Cap: source.Upgrade < 6 AND combined < 6 (was: combined < 6 only).
Tests updated to the new costs/rates and gain two new cases:
- Armor + Armor sum is rejected (slot must be Boots/Gloves)
- Boots + Gloves sum is rejected (slots must match)
2. \$Gold N now emits a SayiPacket "GoldAward" so the player gets a
visible "you received N gold" line in chat, matching the feedback
pattern \$CreateItem already has via "ReceivedThisItem".
Equipment-upgrade and rarify operations also need OpenNos-faithful
rewrites (real cost tables include cellon + gem + scroll, rarify is
a probability-band reroll not a +1 increment) — landing those in
follow-up commits to keep this PR reviewable.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings the equipment-upgrade and rarify operations into line with
OpenNos's actual mechanics (WearableInstance.UpgradeItem lines 667-897
and WearableInstance.RarifyItem lines 266-440). Both were significantly
oversimplified; my earlier guesses missed core game features.
Infrastructure changes:
- New UpgradeOutcome enum with three values (Success / Failure /
Fixed). UpgradeOperation base now drives the flow off DetermineOutcome
instead of a binary success/fail roll. Default DetermineOutcome
reduces to the old 2-way behavior so SumUpgradeOperation stays
unchanged. EquipmentUpgrade overrides for 3-way; Rarify overrides
for the band-walk.
- ApplyFixed hook + FixedMessage / FixedAnimationValue overrides on
the base. Default ApplyFixed is a no-op so non-equipment operations
don't have to know about the outcome.
- BuildPocketRefresh signature updated to take UpgradeOutcome. Keeps
each operation in control of how to render the source slot per
outcome (e.g. unprotected equipment failure clears the slot;
protected/Fixed leave the wearable in place).
EquipmentUpgrade (Upgrade/UpgradeProtected):
- 3-way outcome per OpenNos's upfix/upfail tables:
rnd < upfix -> Fixed (item gets IsFixed=true, locked)
rnd < upfix+upfail -> Failure (unprotected: item destroyed;
protected: scroll absorbs, treated as Fixed)
else -> Success (Upgrade += 1)
- Two cost tiers indexed by source.Upgrade:
Rare<8: standard cellon + small-gem(1015)/full-gem(1016) tables
Rare>=8: scaled-up "perfect rarity" tables
- Materials per attempt: Cellon (1014) + Gem (1015 if Upgrade<5
else 1016) + Magic Pearl Scroll (1218) for protected.
- IsFixed wearables and Upgrade>=10 wearables rejected upfront.
The IsFixed property flows through automatically because
DtoGenerator generates WearableInstanceDto from the DB entity
where IsFixed already exists.
Rarify (Rarify/RarifyProtected):
- NOT a "+1" — it's a probability-band reroll. The new Rare is chosen
by walking the bands from rare8 down (rare8=1, rare7=2, rare6=3,
rare5=5, rare4=10, rare3=15, rare2=30, rare1=40, rare0=60 — out
of 100). The first band the roll falls into wins.
- Outcome classification:
new Rare > original -> Success (rarity improved)
new Rare <= original -> Failure (unchanged or demoted)
- Materials: Cellon (1014) x 5 + Gold 500 baseline. Protected adds
Magic Pearl Scroll (1218) x 1 whose only effect is to clamp the
new Rare so it cannot drop below the original (no upward clamp).
- The earlier "Red Stellar (1024) / Blue Stellar (1025)" vnums were
fabricated and have been removed.
Tests fully rewritten to match the new contracts:
- EquipmentUpgradeOperationTests (8): success at +0, Fixed roll at
+5, unprotected failure destroys at +5, protected failure converts
to Fixed and consumes scroll, full-gem path at Upgrade>=5, +10
cap, IsFixed rejection, missing-scroll rejection on protected.
- RarifyOperationTests (7): rare8-band success, demote-roll failure,
protected scroll clamps downside, protected scroll still allows
upside, +8 cap, negative-rarity rejection, missing-scroll
rejection on protected.
- SumUpgradeOperation unchanged from prior commit (Boots/Gloves
restriction + correct OpenNos cost tables). Only its
BuildPocketRefresh signature was updated to the new UpgradeOutcome
parameter.
Build was blocked locally by running game-server processes locking the
build/ directory; compilation itself is clean. Pull, stop the servers,
rebuild to validate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
erwan-joly
force-pushed
the
feature-handler-fills
branch
from
cb42816 to
0d011c5
Compare
|
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:
WalkthroughAdds a full upgrade subsystem: new upgrade abstractions and operations (upgrade, protected, rarify, sum), packet and Nrun handlers, DI registrations, localized log key/resources, RNG abstraction, extensive tests, plus related gameplay, quest, experience, and map-item pickup adjustments. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Handler as UpgradePacketHandler
participant OpRegistry as IUpgradeOperation (selected)
participant Operation as UpgradeOperation
participant RNG as RandomNumberSource
participant Session as ClientSession
Client->>Handler: send UpgradePacket
Handler->>Handler: select IUpgradeOperation by Kind
alt operation found
Handler->>OpRegistry: ExecuteAsync(session, packet)
OpRegistry->>Operation: TryPrepareContext(session, packet)
Operation-->>Operation: validate costs & inventory
Operation->>RNG: NextDouble()
RNG-->>Operation: roll (0..1)
Operation->>Operation: DetermineOutcome(roll, context)
alt Success
Operation->>Operation: ApplySuccess(context)
else Failure
Operation->>Operation: ApplyFailure(session, context)
else Fixed / ProtectedSave
Operation->>Operation: ApplyFixed/ApplyProtectedSave(session, context)
end
Operation->>Operation: Consume materials & gold
Operation-->>OpRegistry: return player packets
OpRegistry-->>Handler: packets
Handler->>Session: SendPacketsAsync(packets)
Session-->>Client: client receives result packets
else no operation
Handler->>Handler: log UNHANDLED_UPGRADE_TYPE
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
test/NosCore.PacketHandlers.Tests/Upgrades/UpgradePacketHandlerTests.cs (1)
41-42: Consider usingArray.Empty<IPacket>()instead ofnew IPacket[0].
Array.Empty<IPacket>()returns a cached singleton, avoiding a per-call allocation.♻️ Suggested refactor
_matchingOperation.Setup(o => o.ExecuteAsync(It.IsAny<ClientSession>(), It.IsAny<UpgradePacket>())) - .ReturnsAsync((IReadOnlyList<IPacket>)new IPacket[0]); + .ReturnsAsync(Array.Empty<IPacket>());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.PacketHandlers.Tests/Upgrades/UpgradePacketHandlerTests.cs` around lines 41 - 42, Replace the per-call allocation new IPacket[0] in the mock setup with the cached singleton Array.Empty<IPacket>() to avoid unnecessary allocations; update the setup on _matchingOperation that configures ExecuteAsync(It.IsAny<ClientSession>(), It.IsAny<UpgradePacket>()) to .ReturnsAsync((IReadOnlyList<IPacket>)Array.Empty<IPacket>()) so the return type still matches IReadOnlyList<IPacket>.src/NosCore.GameObject/Services/UpgradeService/RarifyOperationBase.cs (1)
46-49: Consider documenting the band array ordering.The
BandProbabilitiesarray maps indexitorare i, but the comment lists bands "rare8 down" while the values are written in ascending index order (rare0=60 to rare8=1). The inline comment helps, but a brief doc-comment could clarify thatBandProbabilities[rare]gives the cumulative threshold for that rarity level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.GameObject/Services/UpgradeService/RarifyOperationBase.cs` around lines 46 - 49, Add a short XML/doc comment above the BandProbabilities field in RarifyOperationBase clarifying the array ordering and semantics: state that BandProbabilities[rare] maps a rarity index (rare0..rare8) to its probability/cumulative threshold (e.g., rare0=60 ... rare8=1) and that the array is indexed by increasing rarity rather than displaying from highest-to-lowest; update or remove the inline comment to avoid confusion so future readers know BandProbabilities[rare] returns the threshold for that rarity level.test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs (1)
276-282: Duplicated test helper class.
ItemInstanceForTestis identical to the one inRarifyOperationTests. Consider extracting toNosCore.Tests.Sharedto avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs` around lines 276 - 282, The private test helper ItemInstanceForTest is duplicated from RarifyOperationTests; extract this helper into a shared test helper class (e.g., a public ItemInstanceForTest in the NosCore.Tests.Shared test utilities namespace) and update EquipmentUpgradeOperationTests and RarifyOperationTests to use that shared class instead of the local nested definition, removing the local sealed class from both tests so the tests reference the single shared ItemInstanceForTest type.test/NosCore.GameObject.Tests/Services/UpgradeService/RarifyOperationTests.cs (1)
228-234: Consider extractingItemInstanceForTestto a shared test utility.This test helper class is duplicated in both
RarifyOperationTestsandEquipmentUpgradeOperationTests. Extracting it toNosCore.Tests.Sharedwould reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.GameObject.Tests/Services/UpgradeService/RarifyOperationTests.cs` around lines 228 - 234, Extract the nested test helper class ItemInstanceForTest from RarifyOperationTests and EquipmentUpgradeOperationTests into a single shared test utility class in a new test project/namespace (e.g., NosCore.Tests.Shared) so both test classes reuse it; create a public sealed class ItemInstanceForTest implementing IItemInstance (keeping properties Id, ItemVNum, Item and Clone behavior) and update RarifyOperationTests and EquipmentUpgradeOperationTests to import the shared namespace and remove their local nested definitions.test/NosCore.GameObject.Tests/Services/UpgradeService/SumUpgradeOperationTests.cs (1)
231-231: Unused helper method.
SandShouldHaveBeenConsumed()(no parameter) is never called in the tests — all tests use the parameterizedSandShouldHaveBeenConsumed_(short amount)variant directly. Consider removing this dead code.🧹 Suggested removal
- private void SandShouldHaveBeenConsumed() => SandShouldHaveBeenConsumed_(1); - private void SandShouldHaveBeenConsumed_(short amount) =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.GameObject.Tests/Services/UpgradeService/SumUpgradeOperationTests.cs` at line 231, Remove the dead helper method SandShouldHaveBeenConsumed() — it is never referenced by the tests and duplicates the parameterized helper SandShouldHaveBeenConsumed_(short amount); delete the no-arg method declaration to eliminate unused code and leave only the SandShouldHaveBeenConsumed_(short amount) helper used by the tests (ensure no other code calls the removed symbol).src/NosCore.GameObject/Services/UpgradeService/UpgradeOperation.cs (1)
117-123: Consider usingabstractinstead of throwingNotImplementedException.The
GetSuccessRatemethod throwsNotImplementedExceptionas a signal that subclasses must override either it orDetermineOutcome. While documented, this creates a runtime failure instead of a compile-time error. An alternative would be to makeGetSuccessRateabstract and have subclasses that overrideDetermineOutcomeprovide a dummy implementation, or use a different design pattern.However, given the comment clearly explains the intent and the limited number of subclasses, this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.GameObject/Services/UpgradeService/UpgradeOperation.cs` around lines 117 - 123, Change GetSuccessRate from throwing NotImplementedException to an abstract method so missing overrides become compile-time errors: mark GetSuccessRate as abstract (remove the throw body) and ensure the containing class (UpgradeOperation) is abstract if it isn't already; then update any subclasses that currently only override DetermineOutcome to add a trivial GetSuccessRate implementation (or otherwise implement GetSuccessRate) to satisfy the compiler, referencing the GetSuccessRate and DetermineOutcome methods and UpgradeContext/UpgradeOutcome types when making these changes.test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/ProbabilityUIsHandlerTests.cs (1)
57-73: Add a test for thenulltarget guard path.
ProbabilityUIsHandlerexplicitly ignores events whenevt.Target == null(handler Line 28), but this suite does not currently assert that behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/ProbabilityUIsHandlerTests.cs` around lines 57 - 73, Add a unit test that asserts the handler ignores events where evt.Target == null: create a test method (e.g., ProbabilityUIsRunnerWithNullTargetIsIgnored) in ProbabilityUIsHandlerTests that uses the Spec pattern like the existing tests, calls a helper (e.g., NrunProbabilityUIsWithNullTargetIsHandled) which constructs an Nrun event with Target = null and passes it to the handler, then asserts NoWopenShouldHaveBeenSent; ensure the helper name and the new test reference ProbabilityUIsHandler so reviewers can locate the null-target guard (handler Line 28).test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/UpgradeItemHandlerTests.cs (1)
80-85: Strengthen positive-path assertion to detect duplicate packet sends.At Line 82 onward, consider asserting exactly one
WopenPacketbefore type-checking, so accidental double-send regressions are caught.Suggested test assertion update
private void WopenWithType_ShouldHaveBeenSent(WindowType expected) { - var wopen = _session.LastPackets.OfType<WopenPacket>().LastOrDefault(); - Assert.IsNotNull(wopen); - Assert.AreEqual(expected, wopen.Type); + var wopenPackets = _session.LastPackets.OfType<WopenPacket>().ToList(); + Assert.AreEqual(1, wopenPackets.Count); + Assert.AreEqual(expected, wopenPackets[0].Type); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/UpgradeItemHandlerTests.cs` around lines 80 - 85, The test WopenWithType_ShouldHaveBeenSent currently uses LastOrDefault and only checks non-null and Type, which won't catch duplicate sends; update it to enumerate _session.LastPackets.OfType<WopenPacket>(), Assert that the Count (or use Assert.AreEqual(1, ...)) equals 1 to ensure exactly one WopenPacket was sent, then retrieve that single packet (e.g., Single() or First()) and Assert.AreEqual(expected, packet.Type) — reference WopenWithType_ShouldHaveBeenSent, WopenPacket, _session.LastPackets, OfType, and the Assert calls when making this change.
🤖 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/Messaging/Handlers/Nrun/ProbabilityUIsHandler.cs`:
- Line 36: In ProbabilityUIsHandler where you construct and send a WopenPacket
using Type = (WindowType)evt.Packet.Type.Value, validate evt.Packet.Type first:
ensure it has a value and that the numeric value maps to a permitted WindowType
(use Enum.IsDefined(typeof(WindowType), value) or an explicit allowlist) before
casting; if the value is invalid, log and abort/return without sending the
WopenPacket to prevent undefined enum values from being propagated.
In `@src/NosCore.GameObject/Messaging/Handlers/Nrun/UpgradeItemHandler.cs`:
- Line 21: UpgradeItemHandler (and ProbabilityUIsHandler) still reference the
deleted NrunRequestedEvent; refactor each handler to implement INrunEventHandler
instead by removing the old Handle(NrunRequestedEvent evt) method and adding the
required Task HandleAsync(ClientSession session, IAliveEntity? target,
NrunPacket packet) method plus the NrunRunnerType Runner { get; } property,
following the pattern used in BazaarHandler/TeleporterHandler (preserve existing
logic but adapt to the new method signature and parameters).
In `@src/NosCore.PacketHandlers/Upgrades/UpgradePacketHandler.cs`:
- Around line 31-36: The current dispatch in UpgradePacketHandler.cs uses
operations.FirstOrDefault(o => o.Kind == packet.UpgradeType) which silently
ignores duplicate operation registrations with the same Kind; change the
initialization of the operations collection into a unique map keyed by Kind
(e.g., build a Dictionary/Lookup from each operation.Kind to its handler) and
throw an exception during startup if a duplicate Kind is detected so dispatch
(in the code that uses operation and packet.UpgradeType) cannot be ambiguous;
ensure the duplicate-detection runs once at composition/registration time rather
than per-packet.
---
Nitpick comments:
In `@src/NosCore.GameObject/Services/UpgradeService/RarifyOperationBase.cs`:
- Around line 46-49: Add a short XML/doc comment above the BandProbabilities
field in RarifyOperationBase clarifying the array ordering and semantics: state
that BandProbabilities[rare] maps a rarity index (rare0..rare8) to its
probability/cumulative threshold (e.g., rare0=60 ... rare8=1) and that the array
is indexed by increasing rarity rather than displaying from highest-to-lowest;
update or remove the inline comment to avoid confusion so future readers know
BandProbabilities[rare] returns the threshold for that rarity level.
In `@src/NosCore.GameObject/Services/UpgradeService/UpgradeOperation.cs`:
- Around line 117-123: Change GetSuccessRate from throwing
NotImplementedException to an abstract method so missing overrides become
compile-time errors: mark GetSuccessRate as abstract (remove the throw body) and
ensure the containing class (UpgradeOperation) is abstract if it isn't already;
then update any subclasses that currently only override DetermineOutcome to add
a trivial GetSuccessRate implementation (or otherwise implement GetSuccessRate)
to satisfy the compiler, referencing the GetSuccessRate and DetermineOutcome
methods and UpgradeContext/UpgradeOutcome types when making these changes.
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/ProbabilityUIsHandlerTests.cs`:
- Around line 57-73: Add a unit test that asserts the handler ignores events
where evt.Target == null: create a test method (e.g.,
ProbabilityUIsRunnerWithNullTargetIsIgnored) in ProbabilityUIsHandlerTests that
uses the Spec pattern like the existing tests, calls a helper (e.g.,
NrunProbabilityUIsWithNullTargetIsHandled) which constructs an Nrun event with
Target = null and passes it to the handler, then asserts
NoWopenShouldHaveBeenSent; ensure the helper name and the new test reference
ProbabilityUIsHandler so reviewers can locate the null-target guard (handler
Line 28).
In
`@test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/UpgradeItemHandlerTests.cs`:
- Around line 80-85: The test WopenWithType_ShouldHaveBeenSent currently uses
LastOrDefault and only checks non-null and Type, which won't catch duplicate
sends; update it to enumerate _session.LastPackets.OfType<WopenPacket>(), Assert
that the Count (or use Assert.AreEqual(1, ...)) equals 1 to ensure exactly one
WopenPacket was sent, then retrieve that single packet (e.g., Single() or
First()) and Assert.AreEqual(expected, packet.Type) — reference
WopenWithType_ShouldHaveBeenSent, WopenPacket, _session.LastPackets, OfType, and
the Assert calls when making this change.
In
`@test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs`:
- Around line 276-282: The private test helper ItemInstanceForTest is duplicated
from RarifyOperationTests; extract this helper into a shared test helper class
(e.g., a public ItemInstanceForTest in the NosCore.Tests.Shared test utilities
namespace) and update EquipmentUpgradeOperationTests and RarifyOperationTests to
use that shared class instead of the local nested definition, removing the local
sealed class from both tests so the tests reference the single shared
ItemInstanceForTest type.
In
`@test/NosCore.GameObject.Tests/Services/UpgradeService/RarifyOperationTests.cs`:
- Around line 228-234: Extract the nested test helper class ItemInstanceForTest
from RarifyOperationTests and EquipmentUpgradeOperationTests into a single
shared test utility class in a new test project/namespace (e.g.,
NosCore.Tests.Shared) so both test classes reuse it; create a public sealed
class ItemInstanceForTest implementing IItemInstance (keeping properties Id,
ItemVNum, Item and Clone behavior) and update RarifyOperationTests and
EquipmentUpgradeOperationTests to import the shared namespace and remove their
local nested definitions.
In
`@test/NosCore.GameObject.Tests/Services/UpgradeService/SumUpgradeOperationTests.cs`:
- Line 231: Remove the dead helper method SandShouldHaveBeenConsumed() — it is
never referenced by the tests and duplicates the parameterized helper
SandShouldHaveBeenConsumed_(short amount); delete the no-arg method declaration
to eliminate unused code and leave only the SandShouldHaveBeenConsumed_(short
amount) helper used by the tests (ensure no other code calls the removed
symbol).
In `@test/NosCore.PacketHandlers.Tests/Upgrades/UpgradePacketHandlerTests.cs`:
- Around line 41-42: Replace the per-call allocation new IPacket[0] in the mock
setup with the cached singleton Array.Empty<IPacket>() to avoid unnecessary
allocations; update the setup on _matchingOperation that configures
ExecuteAsync(It.IsAny<ClientSession>(), It.IsAny<UpgradePacket>()) to
.ReturnsAsync((IReadOnlyList<IPacket>)Array.Empty<IPacket>()) so the return type
still matches IReadOnlyList<IPacket>.
🪄 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: 83c503de-87c4-465f-8ec6-3d17a231b952
📒 Files selected for processing (31)
src/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/Messaging/Handlers/Nrun/ProbabilityUIsHandler.cssrc/NosCore.GameObject/Messaging/Handlers/Nrun/UpgradeItemHandler.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/Upgrades/UpgradePacketHandler.cssrc/NosCore.WorldServer/WorldServerBootstrap.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/ProbabilityUIsHandlerTests.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/UpgradeItemHandlerTests.cstest/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cstest/NosCore.GameObject.Tests/Services/UpgradeService/RarifyOperationTests.cstest/NosCore.GameObject.Tests/Services/UpgradeService/SumUpgradeOperationTests.cstest/NosCore.PacketHandlers.Tests/Upgrades/UpgradePacketHandlerTests.cs
|
|
||
| return evt.ClientSession.SendPacketAsync(new WopenPacket | ||
| { | ||
| Type = (WindowType)evt.Packet.Type.Value, |
There was a problem hiding this comment.
Validate evt.Packet.Type before casting to WindowType.
Line 36 directly casts client-controlled input, which allows undefined enum values through. Add an enum validity check (or explicit allowlist) before sending WopenPacket.
Suggested hardening
+ var rawType = evt.Packet.Type.Value;
+ if (!System.Enum.IsDefined(typeof(WindowType), (byte)rawType))
+ {
+ return Task.CompletedTask;
+ }
+
return evt.ClientSession.SendPacketAsync(new WopenPacket
{
- Type = (WindowType)evt.Packet.Type.Value,
+ Type = (WindowType)rawType,
Unknown = 0,
Unknown2 = 0,
});📝 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
| Type = (WindowType)evt.Packet.Type.Value, | |
| var rawType = evt.Packet.Type.Value; | |
| if (!System.Enum.IsDefined(typeof(WindowType), (byte)rawType)) | |
| { | |
| return Task.CompletedTask; | |
| } | |
| return evt.ClientSession.SendPacketAsync(new WopenPacket | |
| { | |
| Type = (WindowType)rawType, | |
| Unknown = 0, | |
| Unknown2 = 0, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NosCore.GameObject/Messaging/Handlers/Nrun/ProbabilityUIsHandler.cs` at
line 36, In ProbabilityUIsHandler where you construct and send a WopenPacket
using Type = (WindowType)evt.Packet.Type.Value, validate evt.Packet.Type first:
ensure it has a value and that the numeric value maps to a permitted WindowType
(use Enum.IsDefined(typeof(WindowType), value) or an explicit allowlist) before
casting; if the value is invalid, log and abort/return without sending the
WopenPacket to prevent undefined enum values from being propagated.
| public sealed class UpgradeItemHandler | ||
| { | ||
| [UsedImplicitly] | ||
| public Task Handle(NrunRequestedEvent evt) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where NrunRequestedEvent is declared and whether this project can reference it.
fd -i 'Nrun*Event*.cs'
rg -nP '^\s*(public|internal)?\s*(sealed\s+)?(class|record|struct)\s+NrunRequestedEvent\b'
rg -nP '^\s*namespace\s+.*Messaging\.Events.*'
rg -nP '\bNrunRequestedEvent\b' src testRepository: NosCoreIO/NosCore
Length of output: 2086
🏁 Script executed:
# Check imports and full content of UpgradeItemHandler.cs
cat -n src/NosCore.GameObject/Messaging/Handlers/Nrun/UpgradeItemHandler.cs
# Search for NrunRequestedEvent in all files without regex restrictions
rg 'NrunRequestedEvent' --type cs
# Look for event declarations in different patterns
rg 'NrunRequestedEvent' . --type cs -B 2 -A 2Repository: NosCoreIO/NosCore
Length of output: 7510
🏁 Script executed:
# List all files in the Events folder
ls -la src/NosCore.GameObject/Messaging/Events/
# Search for NrunRequestedEvent anywhere in the codebase with no filters
rg 'class NrunRequestedEvent|record NrunRequestedEvent|NrunRequestedEvent.*{'Repository: NosCoreIO/NosCore
Length of output: 971
🏁 Script executed:
# Search for the definition with proper regex escaping
rg 'class NrunRequestedEvent|record NrunRequestedEvent' --type cs
# Also search in all .cs files for any mention of defining NrunRequestedEvent
rg 'NrunRequestedEvent\s*[{:]' --type cs
# List all cs files in the entire GameObject project to see full structure
fd '\.cs$' src/NosCore.GameObject | head -50Repository: NosCoreIO/NosCore
Length of output: 3042
🏁 Script executed:
# Search entire repository for the type definition
rg 'namespace.*NrunRequestedEvent|public.*class.*NrunRequestedEvent|public.*record.*NrunRequestedEvent' --type cs -i
# Search in all directories, not just src
rg 'NrunRequestedEvent' -l --type cs
# Check .csproj files to understand dependencies
find . -name '*.csproj' -type f -exec grep -l 'NosCore.GameObject' {} \;Repository: NosCoreIO/NosCore
Length of output: 715
🏁 Script executed:
# Let's look at the entire Nrun handlers folder structure
find src/NosCore.GameObject/Messaging/Handlers/Nrun -type f
# And check if there are any Nrun event files
find src -name '*Nrun*' -type f
# Let's also check the test setup to understand where NrunRequestedEvent should come from
cat src/NosCore.GameObject/Messaging/Handlers/Nrun/ProbabilityUIsHandler.csRepository: NosCoreIO/NosCore
Length of output: 2264
🏁 Script executed:
# Check the INrunEventHandler interface
cat -n src/NosCore.GameObject/Messaging/Handlers/Nrun/INrunEventHandler.cs
# Search for any event class that might match the pattern
rg 'RequestedEvent' --type cs -A 3 -B 1
# Check if NrunRequestedEvent might be in Events namespace but not in the Events folder
rg 'NrunRequestedEvent' --type cs -B 5 | head -40Repository: NosCoreIO/NosCore
Length of output: 10172
🏁 Script executed:
# Check if there are any files we may have missed - search entire repo recursively
find . -name '*.cs' -type f -exec grep -l 'NrunRequestedEvent' {} \;
# Let me also check if it might be a nested class or defined inline
rg 'NrunRequestedEvent\s*\(' --type cs -B 10 | head -60
# Check the Events namespace more carefully - maybe there's another folder
find . -path './src/NosCore.GameObject/Messaging' -name '*.cs' -type fRepository: NosCoreIO/NosCore
Length of output: 5597
🏁 Script executed:
# Let's explore the entire Messaging structure
tree src/NosCore.GameObject/Messaging/ -L 3
# Check if there are any generated files or if Events might be defined elsewhere
find src/NosCore.GameObject/Messaging -name '*.cs' -type f
# Look for any build-generated files or partial classes
rg 'partial.*NrunRequestedEvent' --type csRepository: NosCoreIO/NosCore
Length of output: 5686
🏁 Script executed:
# Let's check another handler to see the pattern difference
cat -n src/NosCore.GameObject/Messaging/Handlers/Battle/PlayerRevivalHandler.cs
# Also check one of the event files to understand the event structure
cat -n src/NosCore.GameObject/Messaging/Events/EntityDiedEvent.csRepository: NosCoreIO/NosCore
Length of output: 5217
🏁 Script executed:
# Check other Nrun handlers to see their pattern
cat -n src/NosCore.GameObject/Messaging/Handlers/Nrun/BazaarHandler.cs
# Also check the test file for imports
head -20 test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/UpgradeItemHandlerTests.csRepository: NosCoreIO/NosCore
Length of output: 3011
🏁 Script executed:
# Critical discovery - let's check if NrunRequestedEvent should exist
# Look at the entire test file
cat test/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/UpgradeItemHandlerTests.cs
# Check if there's a setup or helper that creates NrunRequestedEvent
rg 'NrunRequestedEvent' --type cs -B 3 -A 3 | head -100Repository: NosCoreIO/NosCore
Length of output: 8949
🏁 Script executed:
# Verify the pattern - check all Nrun handlers to see which pattern they use
rg 'public sealed class.*Handler' src/NosCore.GameObject/Messaging/Handlers/Nrun/ -A 5
# Check if NrunRequestedEvent should be a record like other events
cat src/NosCore.GameObject/Messaging/Events/GuriPacketReceivedEvent.csRepository: NosCoreIO/NosCore
Length of output: 6101
🏁 Script executed:
# Final verification - search for any reference to NrunRequestedEvent definition or declaration
rg 'record.*NrunRequestedEvent|class.*NrunRequestedEvent' --type cs
# Also check git history to see if it was deleted
git log --oneline --all | head -20
# Search for any .cs files that might contain Nrun and Event together
fd '.*Nrun.*Event.*\.cs$|.*Event.*Nrun.*\.cs$'Repository: NosCoreIO/NosCore
Length of output: 1585
🏁 Script executed:
# Check the refactoring commit that changed n_run dispatch
git show f9e913c4 --stat
# Get the actual changes in that commit related to Nrun handlers
git show f9e913c4 -- src/NosCore.GameObject/Messaging/Handlers/Nrun/ | head -200Repository: NosCoreIO/NosCore
Length of output: 11419
Incomplete refactoring: handlers still using deleted NrunRequestedEvent type.
NrunRequestedEvent was removed in commit f9e913c as part of refactoring the n_run dispatch to use INrunEventHandler. However, UpgradeItemHandler and ProbabilityUIsHandler were not updated. They must be refactored to implement the INrunEventHandler interface with the signature Task HandleAsync(ClientSession session, IAliveEntity? target, NrunPacket packet) and include a NrunRunnerType Runner { get; } property, matching the pattern used by BazaarHandler, TeleporterHandler, and other handlers.
🧰 Tools
🪛 GitHub Check: build
[failure] 21-21:
The type or namespace name 'NrunRequestedEvent' could not be found (are you missing a using directive or an assembly reference?)
[failure] 21-21:
The type or namespace name 'NrunRequestedEvent' could not be found (are you missing a using directive or an assembly reference?)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NosCore.GameObject/Messaging/Handlers/Nrun/UpgradeItemHandler.cs` at line
21, UpgradeItemHandler (and ProbabilityUIsHandler) still reference the deleted
NrunRequestedEvent; refactor each handler to implement INrunEventHandler instead
by removing the old Handle(NrunRequestedEvent evt) method and adding the
required Task HandleAsync(ClientSession session, IAliveEntity? target,
NrunPacket packet) method plus the NrunRunnerType Runner { get; } property,
following the pattern used in BazaarHandler/TeleporterHandler (preserve existing
logic but adapt to the new method signature and parameters).
Comment on lines
+31
to
+36
| var operation = operations.FirstOrDefault(o => o.Kind == packet.UpgradeType); | ||
| if (operation == null) | ||
| { | ||
| logger.Warning(logLanguage[LogLanguageKey.UNHANDLED_UPGRADE_TYPE], packet.UpgradeType); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fail fast on duplicate Kind registrations to avoid ambiguous dispatch.
Line 31 picks the first matching operation. With assembly scanning, two operations can accidentally share a Kind, and one will be silently ignored.
Suggested fix (build unique map once, throw on duplicates)
+using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
@@
public sealed class UpgradePacketHandler(
IEnumerable<IUpgradeOperation> operations,
ILogger logger,
ILogLanguageLocalizer<LogLanguageKey> logLanguage)
: PacketHandler<UpgradePacket>, IWorldPacketHandler
{
+ private readonly IReadOnlyDictionary<UpgradePacketType, IUpgradeOperation> _operationsByKind =
+ operations
+ .GroupBy(o => o.Kind)
+ .ToDictionary(
+ g => g.Key,
+ g => g.Count() == 1
+ ? g.Single()
+ : throw new InvalidOperationException($"Duplicate upgrade operation for kind {g.Key}."));
+
public override async Task ExecuteAsync(UpgradePacket packet, ClientSession session)
{
- var operation = operations.FirstOrDefault(o => o.Kind == packet.UpgradeType);
- if (operation == null)
+ if (!_operationsByKind.TryGetValue(packet.UpgradeType, out var operation))
{
logger.Warning(logLanguage[LogLanguageKey.UNHANDLED_UPGRADE_TYPE], packet.UpgradeType);
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NosCore.PacketHandlers/Upgrades/UpgradePacketHandler.cs` around lines 31
- 36, The current dispatch in UpgradePacketHandler.cs uses
operations.FirstOrDefault(o => o.Kind == packet.UpgradeType) which silently
ignores duplicate operation registrations with the same Kind; change the
initialization of the operations collection into a unique map keyed by Kind
(e.g., build a Dictionary/Lookup from each operation.Kind to its handler) and
throw an exception during startup if a duplicate Kind is detected so dispatch
(in the code that uses operation and packet.UpgradeType) cannot be ambiguous;
ensure the duplicate-detection runs once at composition/registration time rather
than per-packet.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs (1)
251-253: Strengthen the “not fixed” assertion to avoid nullable false positives.At Line 252,
Assert.AreNotEqual(true, ...)can pass ifIsFixedbecomes null. Prefer an explicit false-state assertion.Suggested change
- private void WearableShouldNotBeFixed() => - Assert.AreNotEqual(true, ((WearableInstance)_session.Character.InventoryService - .LoadBySlotAndType(0, NoscorePocketType.Equipment)!.ItemInstance!).IsFixed); + private void WearableShouldNotBeFixed() => + Assert.IsTrue(((WearableInstance)_session.Character.InventoryService + .LoadBySlotAndType(0, NoscorePocketType.Equipment)!.ItemInstance!).IsFixed is false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs` around lines 251 - 253, The test method WearableShouldNotBeFixed currently uses Assert.AreNotEqual(true, ...) which will pass if IsFixed is null; change the assertion to explicitly assert false so nulls fail the test: ensure the chain ((WearableInstance)_session.Character.InventoryService.LoadBySlotAndType(0, NoscorePocketType.Equipment)!.ItemInstance!).IsFixed is checked non-null and then use Assert.IsFalse (or Assert.False depending on the test framework) against that boolean value in WearableShouldNotBeFixed.
🤖 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/Messaging/Handlers/Nrun/UpgradeItemHandler.cs`:
- Around line 24-31: The current early-return only checks for null target in
UpgradeItemHandler, allowing players to open the upgrade UI; change the guard to
require an NPC target like other NRun handlers (e.g., BazaarHandler). In the
handler method (UpgradeItemHandler.Handle/Process or the method that references
variable target), replace the null-only check with a combined check such as if
(target == null || !target.IsNpc) return Task.CompletedTask (or check
target.Type == EntityType.Npc if that project convention is used) so only NPC
targets can trigger the WopenPacket for WindowType.UpgradeItem.
In
`@test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs`:
- Around line 168-174: The test currently only checks that one packet was
returned (via SingleRejectionPacketShouldBeReturned) but the spec text expects
an InfoiPacket; update the assertion in the test flow so it verifies the
returned packet's type is InfoiPacket (not just Count==1). Locate the spec that
calls ProtectedUpgradeIsExecuted and replace or extend
SingleRejectionPacketShouldBeReturned with an assertion that the single packet
is of type InfoiPacket (or add an additional assertion immediately after
ProtectedUpgradeIsExecuted to assert packet.GetType()/PacketId == InfoiPacket),
ensuring the test fails if a different packet type is returned.
---
Nitpick comments:
In
`@test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs`:
- Around line 251-253: The test method WearableShouldNotBeFixed currently uses
Assert.AreNotEqual(true, ...) which will pass if IsFixed is null; change the
assertion to explicitly assert false so nulls fail the test: ensure the chain
((WearableInstance)_session.Character.InventoryService.LoadBySlotAndType(0,
NoscorePocketType.Equipment)!.ItemInstance!).IsFixed is checked non-null and
then use Assert.IsFalse (or Assert.False depending on the test framework)
against that boolean value in WearableShouldNotBeFixed.
🪄 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: bc099916-619a-4815-82ae-a1b2afc19e95
📒 Files selected for processing (3)
src/NosCore.GameObject/Messaging/Handlers/Nrun/UpgradeItemHandler.cstest/NosCore.GameObject.Tests/Messaging/Handlers/Nrun/UpgradeItemHandlerTests.cstest/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs
Comment on lines
+168
to
+174
| await new Spec("Protected upgrade with no scroll in inventory rejects with InfoiPacket") | ||
| .Given(WearableAtUpgrade_, (byte)0) | ||
| .And(CellonAndGemInInventoryNoScroll) | ||
| .And(CharacterHasGold_, 100_000L) | ||
| .WhenAsync(ProtectedUpgradeIsExecuted) | ||
| .Then(SingleRejectionPacketShouldBeReturned) | ||
| .ExecuteAsync(); |
There was a problem hiding this comment.
Verify rejection packet type, not only packet count.
At Line 168, the spec says InfoiPacket, but Line 274 only asserts Count == 1. This can pass even if the wrong packet is returned.
Suggested change
- private void SingleRejectionPacketShouldBeReturned() => Assert.AreEqual(1, _result?.Count ?? 0);
+ private void SingleRejectionPacketShouldBeReturned()
+ {
+ Assert.AreEqual(1, _result?.Count ?? 0);
+ Assert.AreEqual("InfoiPacket", _result![0].GetType().Name);
+ }Also applies to: 274-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs`
around lines 168 - 174, The test currently only checks that one packet was
returned (via SingleRejectionPacketShouldBeReturned) but the spec text expects
an InfoiPacket; update the assertion in the test flow so it verifies the
returned packet's type is InfoiPacket (not just Count==1). Locate the spec that
calls ProtectedUpgradeIsExecuted and replace or extend
SingleRejectionPacketShouldBeReturned with an assertion that the single packet
is of type InfoiPacket (or add an additional assertion immediately after
ProtectedUpgradeIsExecuted to assert packet.GetType()/PacketId == InfoiPacket),
ensuring the test fails if a different packet type is returned.
The sayi2 ItemReceived packet emitted on gold pickup was passing the
raw item vnum (1046) as the second argument, producing "You received
1000000 1046" in-game. The official server emits the localized gold
name (per account language), per the floor-pickup trace:
sayi2 1 <char> 12 413 9 <amount> Gold
Resolved via ItemDto.Name[session.Account.Language] — the same pattern
CBListPacketHandler already uses for bazaar item name sorting.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NosCore.GameObject/Messaging/Handlers/MapItem/GoldDropHandler.cs (1)
54-54: Consider defensive lookup for item name.
First()will throwInvalidOperationExceptionif no item matches. While VNum 1046 (gold) should always exist, usingFirstOrDefault()with a fallback would be more resilient to configuration issues.🛡️ Proposed defensive fix
- var goldName = items.First(i => i.VNum == vnum).Name[session.Account.Language]; + var goldItem = items.FirstOrDefault(i => i.VNum == vnum); + var goldName = goldItem?.Name[session.Account.Language] ?? vnum.ToString();🤖 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` at line 54, The lookup for goldName in GoldDropHandler.cs uses items.First(i => i.VNum == vnum) which will throw if no item matches; change this to use a safe lookup (items.FirstOrDefault(i => i.VNum == vnum)) and handle the null case by providing a fallback name (e.g., a localized fallback via session.Account.Language or a default like "Gold") before assigning goldName so the code no longer throws InvalidOperationException when the item is missing.
🤖 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/Messaging/Handlers/MapItem/GoldDropHandler.cs`:
- Around line 32-42: The code in GoldDropHandler.cs is reading VNum, Amount, and
VisualId directly from the MapItemPickedUpEvent (evt) but those members live on
evt.MapItem; update the handler to read vnum, amount, and visualId from
evt.MapItem (e.g., evt.MapItem.VNum, evt.MapItem.Amount, evt.MapItem.VisualId)
while keeping session = evt.ClientSession and other logic identical; ensure you
null-check evt.MapItem before accessing its members to avoid NREs and update any
local variable names (vnum/amount/visualId) accordingly where they are used
later in the method.
---
Nitpick comments:
In `@src/NosCore.GameObject/Messaging/Handlers/MapItem/GoldDropHandler.cs`:
- Line 54: The lookup for goldName in GoldDropHandler.cs uses items.First(i =>
i.VNum == vnum) which will throw if no item matches; change this to use a safe
lookup (items.FirstOrDefault(i => i.VNum == vnum)) and handle the null case by
providing a fallback name (e.g., a localized fallback via
session.Account.Language or a default like "Gold") before assigning goldName so
the code no longer throws InvalidOperationException when the item is missing.
🪄 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: 0188560c-e331-45b9-9995-a0e079ca09ab
📒 Files selected for processing (3)
src/NosCore.GameObject/Messaging/Handlers/MapItem/GoldDropHandler.cssrc/NosCore.PacketHandlers/Command/SetGoldCommandPacketHandler.cstest/NosCore.PacketHandlers.Tests/Command/SetGoldCommandPacketHandlerTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NosCore.PacketHandlers/Command/SetGoldCommandPacketHandler.cs
Comment on lines
+32
to
+42
| if (evt.VNum != 1046) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var session = evt.ClientSession; | ||
| var mapItem = evt.MapItem; | ||
| var maxGold = worldConfiguration.Value.MaxGoldAmount; | ||
|
|
||
| var character = session.Character; | ||
| if (character.Gold + mapItem.Amount <= maxGold) | ||
| var visualId = evt.VisualId; | ||
| var amount = evt.Amount; | ||
| var vnum = evt.VNum; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for MapItemPickedUpEvent definition to verify available properties
ast-grep --pattern $'class MapItemPickedUpEvent {
$$$
}'
# Also search with record pattern in case it's a record type
ast-grep --pattern $'record MapItemPickedUpEvent($$$)'
# Fallback: grep for the event definition
rg -n -A 20 'class MapItemPickedUpEvent|record MapItemPickedUpEvent'Repository: NosCoreIO/NosCore
Length of output: 322
Fix build failure: MapItemPickedUpEvent does not expose VNum, Amount, VisualId directly—access via MapItem property.
The code accesses properties that don't exist on the event. MapItemPickedUpEvent defines only ClientSession, MapItem, and Packet. The referenced properties are members of the MapItem object.
Required fix
- if (evt.VNum != 1046)
+ if (evt.MapItem.VNum != 1046)
{
return;
}
var session = evt.ClientSession;
var maxGold = worldConfiguration.Value.MaxGoldAmount;
var character = session.Character;
- var visualId = evt.VisualId;
- var amount = evt.Amount;
- var vnum = evt.VNum;
+ var visualId = evt.MapItem.VisualId;
+ var amount = evt.MapItem.Amount;
+ var vnum = evt.MapItem.VNum;📝 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
| if (evt.VNum != 1046) | |
| { | |
| return; | |
| } | |
| var session = evt.ClientSession; | |
| var mapItem = evt.MapItem; | |
| var maxGold = worldConfiguration.Value.MaxGoldAmount; | |
| var character = session.Character; | |
| if (character.Gold + mapItem.Amount <= maxGold) | |
| var visualId = evt.VisualId; | |
| var amount = evt.Amount; | |
| var vnum = evt.VNum; | |
| if (evt.MapItem.VNum != 1046) | |
| { | |
| return; | |
| } | |
| var session = evt.ClientSession; | |
| var maxGold = worldConfiguration.Value.MaxGoldAmount; | |
| var character = session.Character; | |
| var visualId = evt.MapItem.VisualId; | |
| var amount = evt.MapItem.Amount; | |
| var vnum = evt.MapItem.VNum; |
🧰 Tools
🪛 GitHub Check: build
[failure] 42-42:
'MapItemPickedUpEvent' does not contain a definition for 'VNum' and no accessible extension method 'VNum' accepting a first argument of type 'MapItemPickedUpEvent' could be found (are you missing a using directive or an assembly reference?)
[failure] 41-41:
'MapItemPickedUpEvent' does not contain a definition for 'Amount' and no accessible extension method 'Amount' accepting a first argument of type 'MapItemPickedUpEvent' could be found (are you missing a using directive or an assembly reference?)
[failure] 40-40:
'MapItemPickedUpEvent' does not contain a definition for 'VisualId' and no accessible extension method 'VisualId' accepting a first argument of type 'MapItemPickedUpEvent' could be found (are you missing a using directive or an assembly reference?)
[failure] 32-32:
'MapItemPickedUpEvent' does not contain a definition for 'VNum' and no accessible extension method 'VNum' accepting a first argument of type 'MapItemPickedUpEvent' could be found (are you missing a using directive or an assembly reference?)
[failure] 42-42:
'MapItemPickedUpEvent' does not contain a definition for 'VNum' and no accessible extension method 'VNum' accepting a first argument of type 'MapItemPickedUpEvent' could be found (are you missing a using directive or an assembly reference?)
[failure] 41-41:
'MapItemPickedUpEvent' does not contain a definition for 'Amount' and no accessible extension method 'Amount' accepting a first argument of type 'MapItemPickedUpEvent' could be found (are you missing a using directive or an assembly reference?)
[failure] 40-40:
'MapItemPickedUpEvent' does not contain a definition for 'VisualId' and no accessible extension method 'VisualId' accepting a first argument of type 'MapItemPickedUpEvent' could be found (are you missing a using directive or an assembly reference?)
[failure] 32-32:
'MapItemPickedUpEvent' does not contain a definition for 'VNum' and no accessible extension method 'VNum' accepting a first argument of type 'MapItemPickedUpEvent' could be found (are you missing a using directive or an assembly reference?)
🤖 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 32 - 42, The code in GoldDropHandler.cs is reading VNum, Amount, and
VisualId directly from the MapItemPickedUpEvent (evt) but those members live on
evt.MapItem; update the handler to read vnum, amount, and visualId from
evt.MapItem (e.g., evt.MapItem.VNum, evt.MapItem.Amount, evt.MapItem.VisualId)
while keeping session = evt.ClientSession and other logic identical; ensure you
null-check evt.MapItem before accessing its members to avoid NREs and update any
local variable names (vnum/amount/visualId) accordingly where they are used
later in the method.
Quest pipeline: split ObjectivesCompletedOn from CompletedOn so Hunt quests with a q_pay reward step stay open until claimed while quests without one auto-complete on objective. Harden CheckScriptStateAsync to require the client's scriptId/stepId match server state (was blindly trusting the client, lets script spam skip quests). Extend the qstlist countable-type whitelist to cover the 9 types confirmed against the official client (Brings/Capture/Product/TsPoint/CollectInTs/Required/ Collect/TransmitGold + CaptureWithoutGettingTheMonster). Reward/pickup: MapItemPickedUpEvent now carries a value snapshot so Wolverine fan-out can't race on a destroyed bundle. RewardService broadcasts the drop packet it was previously skipping (monsters looked like they dropped nothing). Gold pickup emits CancelPacket + localized "picked up N Gold" message. XP: New ExperienceProgressionService centralizes the level/job/hero/ SP/fairy cascade with the packet sequence from the trace (levelup / stat / msgi 510 / eff 6 / eff 198 / ...). Adventurer job cap at 20. Upgrade system: Rewrote Rarify to mirror OpenNos Normal-mode bands (rare7..rare1, scroll skips non-improving bands) with the correct msgi ids (147 GambleSuccessful / 146 GambleItemDisappeared, was 547/ 1398). UpgradeOperation gains ProtectedSave outcome + split say/msg hooks so Rarify/Equipment/Sum can diverge. Reconnect: GameStartPacketHandler re-sends the saved ScriptPacket so in-progress tutorial state survives relogin. Noise: no-op handlers for bp_close and sortopen so the client's background chatter stops filling the warning log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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/Messaging/Handlers/MapItem/DropHandler.cs (1)
38-55:⚠️ Potential issue | 🔴 CriticalDo not add the item before the map drop is claimed.
AddItemToPocket(...)and the success packets run beforeTryRemoveMapItem(visualId), and theTryRemoveMapItemresult is ignored. If two players race the same drop, this can duplicate the inventory item for the loser of the removal race. The map item needs to be claimed atomically before granting inventory state or pickup UX.🤖 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 currently calls InventoryItemInstance.Create and session.Character.InventoryService.AddItemToPocket before claiming the map drop, risking duplicated items if TryRemoveMapItem(visualId) fails; change the flow in DropHandler.cs to first call session.Character.MapInstance.TryRemoveMapItem(visualId) and check its boolean result, and only if it returns true proceed to create the InventoryItemInstance, call AddItemToPocket, and send the pocket/receive/icon/get packets (SendPacketAsync, GeneratePocketChange, GenerateIcon, GenerateGet); if TryRemoveMapItem returns false, do not add the item or send success packets and instead handle the failed-claim path (e.g., no-op or send a pickup-failed response) so claiming is atomic relative to inventory changes.
🧹 Nitpick comments (2)
src/NosCore.GameObject/Services/BattleService/RewardService.cs (1)
104-115: Verify SP instance availability whenUseSpis true.The code checks
player.UseSpbut the inventory lookup at line 107-109 could return null if the SP item isn't in the expected slot or the inventory state is inconsistent. The null check onspInstance(line 110) handles this gracefully, but consider whetherUseSp == truewithout an equipped SP represents a valid state or a data inconsistency that should be logged.Optional: Add debug logging for state inconsistency
if (player.UseSp) { var spInstance = player.InventoryService .LoadBySlotAndType((short)EquipmentType.Sp, NoscorePocketType.Wear) ?.ItemInstance as SpecialistInstance; if (spInstance != null) { var multiplier = spInstance.SpLevel < 10 ? 10 : spInstance.SpLevel < 19 ? 5 : 1; spXp = (long)(mob.JobXp * share * multiplier); } + else + { + logger.Debug("UseSp is true but no SP equipped for character {CharacterId}", player.CharacterId); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.GameObject/Services/BattleService/RewardService.cs` around lines 104 - 115, The code currently guards access to SpecialistInstance when awarding SP XP but does not surface when player.UseSp is true yet no SP is equipped; update RewardService to detect this inconsistency: after calling player.InventoryService.LoadBySlotAndType(...) and before relying on spInstance, add a debug/warn log that includes player.Id (or identifying info) when player.UseSp is true but the returned spInstance is null, and keep the existing null check so behavior doesn't change; reference the symbols player.UseSp, InventoryService.LoadBySlotAndType, SpecialistInstance, and spInstance when adding the log so it’s easy to locate and audit.src/NosCore.GameObject/Services/QuestService/QuestService.cs (1)
323-328: Consider extracting repeated filter to a variable.The filter
s.Value.ObjectivesCompletedOn == null && s.Value.CompletedOn == nullis evaluated twice. Extracting to a local collection would improve readability and avoid redundant iteration.♻️ Suggested refactor
+ var activeQuests = character.Quests.Where(s => s.Value.ObjectivesCompletedOn == null && s.Value.CompletedOn == null).ToList(); - if (character.Quests.Where(s => s.Value.ObjectivesCompletedOn == null && s.Value.CompletedOn == null).Any(q => !q.Value.Quest.IsSecondary) || - (character.Quests.Where(s => s.Value.ObjectivesCompletedOn == null && s.Value.CompletedOn == null).Where(q => q.Value.Quest.QuestType != QuestType.WinRaid).ToList().Count >= 5 && + if (activeQuests.Any(q => !q.Value.Quest.IsSecondary) || + (activeQuests.Count(q => q.Value.Quest.QuestType != QuestType.WinRaid) >= 5 && quest.QuestType != QuestType.WinRaid))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.GameObject/Services/QuestService/QuestService.cs` around lines 323 - 328, The repeated filter on character.Quests (s => s.Value.ObjectivesCompletedOn == null && s.Value.CompletedOn == null) is evaluated twice; extract that filtered collection into a local variable (e.g., activeQuests) inside QuestService.cs and reuse it in the subsequent checks (use activeQuests.Any(q => !q.Value.Quest.IsSecondary) and activeQuests.Where(q => q.Value.Quest.QuestType != QuestType.WinRaid).Count() >= 5 or activeQuests.Count(q => q.Value.Quest.QuestType != QuestType.WinRaid) >= 5) to avoid redundant iteration and improve readability while preserving the existing logic that references quest.QuestType and QuestType.WinRaid.
🤖 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/Messaging/Handlers/MapItem/SpChargerHandler.cs`:
- Around line 33-38: The code grants SP and sends the SP packet before
confirming the item was claimed; change the flow in SpChargerHandler so you
first call mapInstance.TryRemoveMapItem(visualId) and check its boolean result,
and only if it returns true call character.AddSpPoints(item.Item.EffectValue,
worldConfiguration) and then await
session.SendPacketAsync(character.GenerateSpPoint(worldConfiguration)) and await
mapInstance.SendPacketAsync(character.GenerateGet(visualId)); keep the same
methods (TryRemoveMapItem, AddSpPoints, GenerateSpPoint, GenerateGet,
SendPacketAsync) and ensure no SP/packets are sent when TryRemoveMapItem fails.
In `@src/NosCore.GameObject/Services/QuestService/QuestService.cs`:
- Around line 406-434: The QuestCompletedEvent is being published
unconditionally in CompleteQuestAsync causing double publication for quests that
have a q_pay step; update CompleteQuestAsync so the messageBus.PublishAsync(new
QuestCompletedEvent(character, quest)) call is moved inside the existing if
(!hasQPayStep) block (the same block that sets quest.CompletedOn) so the event
is only published when CompletedOn is set there, leaving QPayAsync to publish
when it sets CompletedOn for q_pay quests; refer to CompleteQuestAsync, the
local hasQPayStep check, QPayAsync, CompletedOn and QuestCompletedEvent to
locate and adjust the code.
In
`@src/NosCore.GameObject/Services/UpgradeService/EquipmentUpgradeOperationBase.cs`:
- Around line 169-179: The high-rarity branch currently checks the fixed band
with roll < upfix which is wrong when upfix is an absolute width; change the
logic in the block that uses data.IsHighRare (the code that currently tests roll
< upfail then roll < upfix) to use a cumulative upper bound for the fixed band
(e.g., compute fixedUpper = upfail + upfix or test roll < upfail + upfix) so
that rolls between upfail (exclusive) and upfail+upfix (inclusive as
appropriate) return UpgradeOutcome.Fixed, otherwise return Success (preserving
the IsProtected check for failures).
---
Outside diff comments:
In `@src/NosCore.GameObject/Messaging/Handlers/MapItem/DropHandler.cs`:
- Around line 38-55: The code currently calls InventoryItemInstance.Create and
session.Character.InventoryService.AddItemToPocket before claiming the map drop,
risking duplicated items if TryRemoveMapItem(visualId) fails; change the flow in
DropHandler.cs to first call
session.Character.MapInstance.TryRemoveMapItem(visualId) and check its boolean
result, and only if it returns true proceed to create the InventoryItemInstance,
call AddItemToPocket, and send the pocket/receive/icon/get packets
(SendPacketAsync, GeneratePocketChange, GenerateIcon, GenerateGet); if
TryRemoveMapItem returns false, do not add the item or send success packets and
instead handle the failed-claim path (e.g., no-op or send a pickup-failed
response) so claiming is atomic relative to inventory changes.
---
Nitpick comments:
In `@src/NosCore.GameObject/Services/BattleService/RewardService.cs`:
- Around line 104-115: The code currently guards access to SpecialistInstance
when awarding SP XP but does not surface when player.UseSp is true yet no SP is
equipped; update RewardService to detect this inconsistency: after calling
player.InventoryService.LoadBySlotAndType(...) and before relying on spInstance,
add a debug/warn log that includes player.Id (or identifying info) when
player.UseSp is true but the returned spInstance is null, and keep the existing
null check so behavior doesn't change; reference the symbols player.UseSp,
InventoryService.LoadBySlotAndType, SpecialistInstance, and spInstance when
adding the log so it’s easy to locate and audit.
In `@src/NosCore.GameObject/Services/QuestService/QuestService.cs`:
- Around line 323-328: The repeated filter on character.Quests (s =>
s.Value.ObjectivesCompletedOn == null && s.Value.CompletedOn == null) is
evaluated twice; extract that filtered collection into a local variable (e.g.,
activeQuests) inside QuestService.cs and reuse it in the subsequent checks (use
activeQuests.Any(q => !q.Value.Quest.IsSecondary) and activeQuests.Where(q =>
q.Value.Quest.QuestType != QuestType.WinRaid).Count() >= 5 or
activeQuests.Count(q => q.Value.Quest.QuestType != QuestType.WinRaid) >= 5) to
avoid redundant iteration and improve readability while preserving the existing
logic that references quest.QuestType and QuestType.WinRaid.
🪄 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: e4f31077-a29e-4676-8b44-700d64b20a78
📒 Files selected for processing (24)
src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cssrc/NosCore.GameObject/Messaging/Events/MapItemPickedUpEvent.cssrc/NosCore.GameObject/Messaging/Handlers/MapItem/DropHandler.cssrc/NosCore.GameObject/Messaging/Handlers/MapItem/SpChargerHandler.cssrc/NosCore.GameObject/Messaging/Handlers/Nrun/FinishedTsDialogHandler.cssrc/NosCore.GameObject/Messaging/Handlers/Quest/OnCharacterMovedHandler.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/UpgradeService/EquipmentUpgradeOperationBase.cssrc/NosCore.GameObject/Services/UpgradeService/RarifyOperationBase.cssrc/NosCore.GameObject/Services/UpgradeService/SumUpgradeOperation.cssrc/NosCore.GameObject/Services/UpgradeService/UpgradeOperation.cssrc/NosCore.GameObject/Services/UpgradeService/UpgradeOutcome.cssrc/NosCore.PacketHandlers/Game/GameStartPacketHandler.cssrc/NosCore.PacketHandlers/Inventory/GetPacketHandler.cssrc/NosCore.PacketHandlers/NoAction/BpClosePacketHandler.cssrc/NosCore.PacketHandlers/NoAction/SortOpenPacketHandler.cstest/NosCore.GameObject.Tests/Services/QuestService/QuestServiceTests.cstest/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs
✅ Files skipped from review due to trivial changes (2)
- src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs
- src/NosCore.PacketHandlers/NoAction/SortOpenPacketHandler.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/NosCore.GameObject/Services/UpgradeService/UpgradeOutcome.cs
- test/NosCore.GameObject.Tests/Services/UpgradeService/EquipmentUpgradeOperationTests.cs
- src/NosCore.GameObject/Services/UpgradeService/SumUpgradeOperation.cs
Comment on lines
+169
to
+179
| if (data.IsHighRare) | ||
| { | ||
| if (roll < upfail) | ||
| { | ||
| return IsProtected ? UpgradeOutcome.ProtectedSave : UpgradeOutcome.Failure; | ||
| } | ||
| if (roll < upfix) | ||
| { | ||
| return UpgradeOutcome.Fixed; | ||
| } | ||
| return UpgradeOutcome.Success; |
There was a problem hiding this comment.
Use a cumulative upper bound for the high-rarity fixed band.
The second branch currently compares roll to upfix instead of the end of the fixed band. That makes Fixed unreachable whenever upfix <= upfail (for example the 50/50 and 40/40 rows) and incorrectly turns those rolls into Success.
Suggested fix
if (data.IsHighRare)
{
if (roll < upfail)
{
return IsProtected ? UpgradeOutcome.ProtectedSave : UpgradeOutcome.Failure;
}
- if (roll < upfix)
+ if (roll < upfail + upfix)
{
return UpgradeOutcome.Fixed;
}
return UpgradeOutcome.Success;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/NosCore.GameObject/Services/UpgradeService/EquipmentUpgradeOperationBase.cs`
around lines 169 - 179, The high-rarity branch currently checks the fixed band
with roll < upfix which is wrong when upfix is an absolute width; change the
logic in the block that uses data.IsHighRare (the code that currently tests roll
< upfail then roll < upfix) to use a cumulative upper bound for the fixed band
(e.g., compute fixedUpper = upfail + upfix or test roll < upfail + upfix) so
that rolls between upfail (exclusive) and upfail+upfix (inclusive as
appropriate) return UpgradeOutcome.Fixed, otherwise return Success (preserving
the IsProtected check for failures).
Walk: gate on Map.IsBlockedZone so the client can't walk through walls or off the grid. Runs after the speed/checksum disconnects so those security-level rejections keep firing as before. Portal: PreqPacketHandler now whitelists portal types we actually support. Raid / BlueRaid / DarkRaid / Closed-state portals fall through to a silent reject until the group and family subsystems land; the existing types (MapPortal/TsNormal/Open/Miniland/TsEnd/Exit/Effect/ TimeSpace/ShopTeleport) pass through unchanged. Remove (unequip): block while InExchangeOrShop. Wear was already gated via BiPacketHandler; Remove was the matching hole — clients could unequip mid-trade and rebase the displayed equipment. Wear: add the Item.ReputationMinimum gate so rep-walled jewels/ costumes reject like level/sex/class do. Damage: lift the 3.0× crit multiplier cap and drop the neutral-skill elemental remap. OpenNos awards the raw CriticalRate and only computes elemental damage when skill.Element matches attacker.Element — we were quietly buffing elemental users on neutral skills and clamping lategame crit. XP: when SP is equipped and SpLevel >= 20, character JobXp is halved to match OpenNos Character.cs:5219 — parallel job growth at SP cap was too generous. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/NosCore.PacketHandlers/Inventory/RemovePacketHandler.cs (1)
25-28: Avoid a silent no-op when remove is blocked in exchange/shop.At Line 25, the guard is correct, but returning silently can be confusing for players and harder to audit. Consider aligning with
BiPacketHandlerby emitting a user-facing/log signal when this action is denied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.PacketHandlers/Inventory/RemovePacketHandler.cs` around lines 25 - 28, The silent early return in RemovePacketHandler (method handling the remove packet) when clientSession.Character.InExchangeOrShop is true should be replaced with an explicit denial: send the player a user-facing error/notification and emit a server log/event as BiPacketHandler does. Update the guard in RemovePacketHandler to call the same denial routine or message used by BiPacketHandler (or create a shared helper) to notify the client that the action is blocked due to being in an exchange/shop and log the denial with identifying info (character id/session) before returning.test/NosCore.PacketHandlers.Tests/Movement/WalkPacketHandlerTests.cs (1)
68-90: Add one direct test for the blocked-zone short-circuit branch.At Lines 68-90, coverage for
MapX/MapYbehavior is solid, but the new early-return path inWalkPacketHandler(blocked destination) is still unverified here. A dedicated case asserting no movement state change would lock in this exploit fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.PacketHandlers.Tests/Movement/WalkPacketHandlerTests.cs` around lines 68 - 90, Add a new unit test in WalkPacketHandlerTests that exercises the blocked-destination short-circuit in WalkPacketHandler: create a Spec similar to the existing Walk tests (use Given(CharacterIsOnMap)), arrange the map/tiles so the destination is blocked (e.g. add a helper or use/extend WalkingToValidPosition to a new WalkingToBlockedPosition step), call WhenAsync(WalkingToBlockedPosition), then assert that no state changed by checking PositionX/Y remain the same and MapX/MapY remain pinned (use assertions analogous to PositionShouldBeUpdated but inverted—e.g. PositionShouldBeUnchanged—and reuse MapXYShouldBe_ with the original coordinates). Ensure the test name clearly states "WalkBlockedDoesNotUpdatePositionOrMapXY" and targets WalkPacketHandler's early-return behavior.
🤖 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/BattleService/RewardService.cs`:
- Around line 143-145: The current flow in RewardService.cs creates a map item
via mapItemGenerationService.Create(...) before broadcasting with
mapInstance.SendPacketAsync(...), so if SendPacketAsync throws the created item
remains in the map without a spawn packet; update the logic in the methods
handling drops (the blocks using itemGenerationService.Create,
mapItemGenerationService.Create, and mapInstance.SendPacketAsync — including the
similar block at the other occurrence around lines 164-166) to either: 1) delay
persisting/adding the map item to the map state until after SendPacketAsync
succeeds, or 2) if you must create it first, ensure you remove/rollback the
created map item from the map state in the catch block when SendPacketAsync
fails; implement the fix in the RewardService drop handling methods and ensure
exception-safe cleanup of the created map item (referencing the created variable
name `dropped` and its creation via mapItemGenerationService.Create).
- Around line 50-85: In DistributeAsync, wrap the reward operations (the calls
to AwardExperienceAsync, SpawnDropsAsync, SpawnGoldAsync) in a try/finally so
victim.HitList.Clear() always runs even if any of those methods throw;
specifically, move the existing victim.HitList.Clear() into a finally block
inside DistributeAsync (while keeping the early-return clears for the
precondition checks) so the HitList is cleared on both success and error and
avoid duplicate/leaked attacker state.
---
Nitpick comments:
In `@src/NosCore.PacketHandlers/Inventory/RemovePacketHandler.cs`:
- Around line 25-28: The silent early return in RemovePacketHandler (method
handling the remove packet) when clientSession.Character.InExchangeOrShop is
true should be replaced with an explicit denial: send the player a user-facing
error/notification and emit a server log/event as BiPacketHandler does. Update
the guard in RemovePacketHandler to call the same denial routine or message used
by BiPacketHandler (or create a shared helper) to notify the client that the
action is blocked due to being in an exchange/shop and log the denial with
identifying info (character id/session) before returning.
In `@test/NosCore.PacketHandlers.Tests/Movement/WalkPacketHandlerTests.cs`:
- Around line 68-90: Add a new unit test in WalkPacketHandlerTests that
exercises the blocked-destination short-circuit in WalkPacketHandler: create a
Spec similar to the existing Walk tests (use Given(CharacterIsOnMap)), arrange
the map/tiles so the destination is blocked (e.g. add a helper or use/extend
WalkingToValidPosition to a new WalkingToBlockedPosition step), call
WhenAsync(WalkingToBlockedPosition), then assert that no state changed by
checking PositionX/Y remain the same and MapX/MapY remain pinned (use assertions
analogous to PositionShouldBeUpdated but inverted—e.g.
PositionShouldBeUnchanged—and reuse MapXYShouldBe_ with the original
coordinates). Ensure the test name clearly states
"WalkBlockedDoesNotUpdatePositionOrMapXY" and targets WalkPacketHandler's
early-return behavior.
🪄 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: 380e1764-7498-4bc6-aa1e-b6838f79436e
📒 Files selected for processing (7)
src/NosCore.GameObject/Messaging/Handlers/UseItem/WearHandler.cssrc/NosCore.GameObject/Services/BattleService/DamageCalculator.cssrc/NosCore.GameObject/Services/BattleService/RewardService.cssrc/NosCore.PacketHandlers/Inventory/RemovePacketHandler.cssrc/NosCore.PacketHandlers/Movement/PreqPacketHandler.cssrc/NosCore.PacketHandlers/Movement/WalkPacketHandler.cstest/NosCore.PacketHandlers.Tests/Movement/WalkPacketHandlerTests.cs
✅ Files skipped from review due to trivial changes (1)
- src/NosCore.PacketHandlers/Movement/PreqPacketHandler.cs
Comment on lines
143
to
+145
| var item = itemGenerationService.Create(drop.VNum, (short)drop.Amount); | ||
| mapItemGenerationService.Create(mapInstance, item, victim.PositionX, victim.PositionY); | ||
| var dropped = mapItemGenerationService.Create(mapInstance, item, victim.PositionX, victim.PositionY); | ||
| await mapInstance.SendPacketAsync(dropped.GenerateDrop()); |
There was a problem hiding this comment.
Avoid leaving invisible drops behind when packet send fails.
Both paths create the map item before broadcasting it. If SendPacketAsync throws after mapItemGenerationService.Create(...), the catch only logs, so the item can remain in map state without a matching spawn packet on clients.
Also applies to: 164-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NosCore.GameObject/Services/BattleService/RewardService.cs` around lines
143 - 145, The current flow in RewardService.cs creates a map item via
mapItemGenerationService.Create(...) before broadcasting with
mapInstance.SendPacketAsync(...), so if SendPacketAsync throws the created item
remains in the map without a spawn packet; update the logic in the methods
handling drops (the blocks using itemGenerationService.Create,
mapItemGenerationService.Create, and mapInstance.SendPacketAsync — including the
similar block at the other occurrence around lines 164-166) to either: 1) delay
persisting/adding the map item to the map state until after SendPacketAsync
succeeds, or 2) if you must create it first, ensure you remove/rollback the
created map item from the map state in the catch block when SendPacketAsync
fails; implement the fix in the RewardService drop handling methods and ensure
exception-safe cleanup of the created map item (referencing the created variable
name `dropped` and its creation via mapItemGenerationService.Create).
Skill learning: ISkillService.LearnClassSkillsAsync sweeps the class's skill list against the player's JobLevel, inserts any newly-qualifying skill into Skills (+ DB), and re-pushes ski. ExperienceProgressionService calls it on every job level-up. Matches OpenNos LearnAdventurerSkill (Character.cs:4463) — without this, players level their job but never unlock new skills. Reputation on mob kill: RewardService awards mob.Level/3 to Reput on BaseMapInstance kills. Dignity recovery: 50% chance of +1 Dignity per kill (cap 100) when player.Level is between 21 and mob.Level-1 — the half-point-per-kill OpenNos uses can't fit in a short, so a 1-in-2 coin flip averages to the same rate. Reputation on PvP death: PlayerRevivalHandler transfers Level*50 rep from victim to killer when the victim has >=50k rep (matches OpenNos BattlePacketHandler:336). Below the threshold the rep just doesn't move — consistent with the existing warn-only branch. Gold drop formula: SpawnGoldAsync now rolls RandomNumber(6*L, 12*L) instead of L*rand(1,5), and only on BaseMap/TimeSpace. The old formula was undershooting by ~4x at every level; now the quest reward loops actually keep pace with seed/scroll prices. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Opens WindowType.CellonItem (wopen 3 0) when the client sends nrun runner=10 on a cellon NPC. Matches OpenNos NRunHandler case 10 — single-line packet, no prereqs. The rest of the audit's missing NRun cases (teleport variants 26/45/132, arena 17/137-139, family warehouse 1600-1605, Land of Death 150) are blocked on per-NPC Teleporter data, arena infrastructure, or the Family subsystem, so they can't ship without those landing first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
41 new test methods across 15 files, landing coverage for the behaviors
most likely to silently regress:
Tier 1 — pins the recent upgrade/save/walk/portal rewrites:
- EquipmentUpgrade: high-rare Rare>=8 band order (Failure->Fixed->Success),
low+high-rare ProtectedSave (scroll does NOT set IsFixed), IsFixed
pre-reject emits sayi+shop_end.
- Rarify: stale tests corrected to rare-7 ceiling + no-band-destroys +
ProtectedSave-keeps-rare; added SetRarityPoint re-roll assertion.
- Sum: target.Item.X base-item resistance is summed alongside instance.
- SaveService: MapX/Y only persists the live PositionX/Y when on a
BaseMap; in-miniland save preserves the prior base position.
- WalkPacketHandler: same BaseMap guard on MapX/Y.
- PreqPacketHandler: miniland-exit forwards character.MapX/Y; base-entry
forwards portal.DestinationX/Y.
Tier 2 — handlers/services that had zero coverage:
- MinilandBellHandler (new file): wrong-effect ignore, non-base reject,
vehicled reject, Mode=0 DelayPacket, Mode=1 consumes+teleports.
- MinilandEntranceHandler (new file): owner vs non-owner visit counter
semantics, non-miniland no-op.
- MinilandService.GetMinilandPortals: two entries with correct source
and destination map-instance ids.
- MapChangeService: character.MapId guards on BaseMap destination only.
Tier 3 — strengthened weak tests:
- TeleportPacketHandler: forwards target's MapX/Y (not caller's) for
player teleport; forwards packet MapX/Y for by-id teleport.
- SellPacketHandler: price*amount overflow rejection; gold-cap rejection.
- QuestService: progress clamps at required count on extra kills;
unrelated mob kills don't advance objectives.
- PlayerRevivalHandler: HP/MP zero on death, dignity -50 per death with
-1000 floor, level<=20 ContinueHereFree vs >20 ContinueHereTenSeeds
dialog, PK reput transfer only when victim.Reput>=50000.
Tier 4 — parser fixture coverage:
- ItemParser.ImportLevelMinimum: weapon/armor pull from first DATA field;
consumables fall back to 0 regardless of DATA value.
- ItemParser.HardcodeItem overrides: vnum 5119 -> SpeedBooster; amulet
vnums 282/180/181 -> RedAmulet/Attack/Defense with correct EffectValue;
vnums 4101-4105 -> EquipmentSlot=MainWeapon.
Also fixed one pre-existing broken assertion:
KillingRequiredMobsShouldCompleteHuntObjectives was asserting
CompletedOn==null, which is wrong once the quest auto-completes without
a q_pay script step (which is the default in test setup).
All 800 tests green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
UseSkillPacketHandler (5 new): - CanFight=false short-circuits to cancel + no Hit. - Sitting caster stands up (RestAsync) before the Hit call. - Known skill with MpCost>0 consumes MP and stamps LastUse after Hit. - Known skill on cooldown sends cancel and skips Hit. - Known skill with insufficient MP sends cancel and skips Hit. Character.BuyAsync (2 new): - price*amount overflow rejects silently (itemPrice > long.MaxValue/amount). - reputPrice*amount overflow rejects silently. 7 new tests, 807/807 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Invalid-packet errors now log header, type and per-field Member=Value: ErrorMessage so the offending packet can be located in trace captures instead of just `null ["Invalid Enum value"]`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SitPacketHandler (4 new) — strengthens weak existing tests that only
verified "handler completes" to assert actual state mutation:
- Sitting self toggles IsSitting to true.
- Sitting self twice toggles it back to false.
- Attempting to sit another player does not flip their sit state
(anti-spoof guard — matches DIRECT_ACCESS_OBJECT_DETECTED branch).
- Non-existent VisualId in session registry is silently ignored.
SpRechargerHandler (new file, 4 tests) — pins the cap + consumption math:
- Below cap: one item consumed, SpAdditionPoint += EffectValue.
- At cap: CannotBeUsedExceedsCapacity emitted, item NOT consumed.
- Non-recharger item type (e.g. Teleport): handler returns early.
- Overflow: AddAdditionalSpPoints clamps at max even when EffectValue
would push past (prevents runaway SP stacking).
8 new tests, 815/815 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 new tests pinning the equip-validation invariants, matching OpenNos
eq-gating and the existing SayiPacket.CanNotWearThat contract:
- Non-wearable ItemType (Main/Etc/...) returns early without emitting
a rejection packet or side-effecting inventory.
- LevelMinimum > character.Level -> CanNotWearThat rejection.
- Item.Class bitmask that doesn't include the character's Class ->
CanNotWearThat rejection (Adventurer trying to wear a warrior-only
item).
- Item.Sex bitmask that doesn't include the character's Gender ->
CanNotWearThat rejection (Female trying to wear a male-only item).
- RequireBinding + Mode=0 + not yet bound -> QnaPacket confirmation
dialog instead of equipping (matches ASK_BIND flow).
5 new tests, 820/820 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 new tests pinning every branch of the death→revive flow:
- Revival packet while already alive is a no-op.
- Type 0 at Level<=20: free revive in place, HP/MP restored to 100%,
no seed consumption.
- Type 0 at Level>20 with >=5 seeds: seeds consumed, HP/MP to 50%,
FivePowerSeedUsed dialog shown.
- Type 0 at Level>20 with <5 seeds: NotEnoughPowerSeed dialog + fall
through to save-point warp (matches OpenNos line 6338 flow).
- Type 1: save-point warp at IRespawnService-resolved location with
full HP/MP.
- Type 2 (arena) with gold>=100: deduct 100 gold, full HP/MP, stay on
map.
- Type 2 without 100 gold: fall through to save-point (no gold debit).
- Unknown Type: IsAlive stays false (default-case death.txt behaviour).
8 new tests, 828/828 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 new tests for the $Invisible admin toggle: - First invocation flips both Camouflage and Invisible flags on. - Running it a second time restores the character to visible. 830/830 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…geChannel, SetMaintenance)
MapParser (3 tests, new file) — MapIDData.dat parsing:
- Single entry: MapId + NameI18NKey extracted from the first digit-
keyed group, fields [0] and [4].
- Multiple entries separated by "DATA 0": each produces a MapDto.
- Empty file yields empty result.
QuestPrizeParser (7 tests, new file) — qstprize.dat per-type branches:
- Gold reward: Amount pulled from DATA[0]; Data is 0.
- Exp reward: Amount=DATA[0], Data=DATA[1] (or 0 when -1 sentinel).
- WearItem: Data=DATA[0], Amount clamped to 1.
- EtcMainItem: Amount=DATA[4] (or 1 when -1 sentinel).
- Multi-entry file parses all entries correctly.
ChangeChannelPacketHandler (1 test, new file) — forwards ChannelId
through to IChannelService.MoveChannelAsync.
SetMaintenancePacketHandler (2 tests, new file) — forwards IsGlobal +
MaintenanceMode flags through to IChannelHub.SetMaintenance.
843/843 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NpcMonsterParser (4 tests, new file) — smoke coverage for the largest
untested parser; full monster.dat fixture with all 24 required sections
(VNUM/NAME/LEVEL/RACE/ATTRIB/HP-MP/EXP/PREATT/SETTING/ETC/PETINFO/EFF/
ZSKILL/WINFO/WEAPON/AINFO/ARMOR/SKILL/PARTNER/BASIC/CARD/MODE/ITEM):
- Basic vnum + NameI18NKey parse correctly from VNUM + NAME lines.
- Duplicate vnums are deduped, keeping the first occurrence.
- Level field parses from LEVEL[2].
- ARMOR-derived defence formulas match the parser math: CloseDefence
(armorLvl-1)*2+18, DistanceDefence (armorLvl-1)*3+17, MagicDefence
(armorLvl-1)*2+13, DefenceDodge (armorLvl-1)*5+31.
DropParser (4 tests, new file) — covers the hardcoded drop catalog:
- GenerateDropDto propagates all five fields (vnum, amount, monster,
dropChance, mapTypeId) without mutation.
- Null monsterVNum is preserved for map-wide drops (gold, cellon).
- InsertDropAsync emits at least one drop for every major map type
(Act1/2/3/32/4/42/51/52/61A/61D/62/Oasis/Mine1/2/Meadow/Sunny/Fernon/
FernonF/Cliff/CometPlain/LandOfTheDead).
- Gold (vnum 1012) drops on every major Act (1-6.2).
851/851 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Existing tests only covered happy paths of SetGold / AddItems / open-
close lifecycle. ValidateExchange's rejection branches were uncovered.
3 new tests pinning the critical rejection paths:
- Target's gold + session gold > MaxGoldAmount -> Failure +
MaxGoldReached packet keyed by target VisualId.
- Non-tradable item offered by session -> Failure + ItemCanNotBeSold
packet keyed by session CharacterId.
- Both sides within caps + fit inventory -> Success with null packet
dict (anti-regression: a spurious overflow check would flip this).
854/854 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 new tests pinning the successful-equip side effects past all guards:
- Successful equip sets BoundCharacterId to the character's id, so
the item is rebind-locked on resale/trade (matches the
Item.RequireBinding contract enforced downstream).
- Item with ItemValidTime > 0 gets ItemDeleteTime stamped on first
equip; the countdown starts on bind, not on spawn.
Together with the 5 guard tests from the prior round this file covers
all branches of WearHandler.Handle except the SP/Fairy element-match
and Sp cooldown paths, which depend on inventory state the wider
integration tests already touch.
856/856 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BackPackHandler (6 tests, new file) — pins inventory-slot-expansion state mutation: - Wrong ItemEffectType returns early (no StaticBonus, item preserved). - Duplicate BackPack grant is rejected with NotInPair. - Duplicate InventoryTicketUpgrade grant is rejected with NotInPair. - First grant consumes the item and adds the StaticBonus. - EffectValue=0 stores null DateEnd (permanent bonus). - EffectValue>0 stamps DateEnd (timed bonus). EmoticonHandler (5 tests, new file) — pins the guard clauses on emote broadcast: - Data below 973 is ignored. - Data above 999 is ignored. - GuriPacketType other than TextInput is ignored. - EmoticonsBlocked flag suppresses the broadcast. - Packet with a spoofed VisualId is ignored. 867/867 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Covers wrong-effect no-ops, vehicled/equipped rejections, and the successful-swap paths that mutate Hair/Gender state and consume the item. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pins the Runner property plus the guard branches: null / non-NPC target is a no-op, and SetPlaceOfRevival Type=2 emits the confirmation Qna whose YesPacket re-issues the runner with Type=1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PetBasket pins the effect -> bonus mapping (PetSpace + PetBasket upgrade both map to PetBasket, PetBackpackUpgrade maps to PetBackPack), the duplicate NotInPair rejection, and DateEnd stamping. Title covers the AskAddTitle Qnai confirmation wiring. Speaker covers the Magical + Speaker gate opening the TextInput prompt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BazaarMedals: pins silver/gold bonus mapping, mutual-exclusion gate (either medal blocks the other), and DateEnd stamping. SealedTarotCard: reward VNum is read from EffectValue and delivered once; malformed rows (EffectValue<=0) are ignored. BoxEffect: wrong effect skips the roll table entirely; empty or zero-probability pools are a no-op; a non-empty pool consumes the box and emits Rdi + ItemReceived. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- UpgradeItemHandler: gate on `target is not NpcComponentBundle` so players passed as target can't open the upgrade window. Matches BazaarHandler / OpenShopHandler / OpenProductionHandler. - SpChargerHandler: claim the map item via TryRemoveMapItem before granting SP, so simultaneous pickups don't both get the reward. - QuestService: for quests with a q_pay step, CompleteQuestAsync no longer publishes QuestCompletedEvent — QPayAsync publishes it when CompletedOn is actually set. Previously it fired twice per quest. - RewardService.DistributeAsync: wrap reward pipeline in try/finally so victim.HitList.Clear always runs even if AwardExperienceAsync throws. - UpgradeOperation.ConsumeMaterials: drop redundant (short) cast on Math.Min(short, short) (CodeQL finding). - UpgradeItemHandlerTests: replace the positive-path test (which exercised the old overly-permissive null-check behavior) with a negative NonNpcTargetIsIgnored spec mirroring TeleporterHandlerTests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
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
UpgradeOperationtemplate-method base for everyup_gr(UpgradePacketType) variant — shared validation, gold/material checks, dice roll, item mutation, packet emission. Subclasses fill in 4 hooks.SumResistance,UpgradeItem+UpgradeItemProtected,RarifyItem+RarifyItemProtected. Protected variants share a base with their unprotected siblings.AsyncLockinWorldPacketHandlingStrategy— no extra locking; OpenNos's exchange/warehouse double-spend window is closed byScope.InTradefiltering before the handler runs.Deferred (need their own subsystems, called out in commit messages)
CellonItem(needsEquipmentOptionDto+ persistence plumbing — currently the table exists but is unmapped in the GameObject layer)FusionItem(rare cross-item fuse)ItemToPart(needs disassembly recipes)UpgradeItemGoldScroll(variant ofUpgradeItem— would need authoritative success-rate bonus numbers)Manual smoke-test checklist (live world server)
Each operation below is a live-game scenario. Per-case material/gold totals match the baked-in tables in
EquipmentUpgradeOperationBase,RarifyOperationBase, andSumUpgradeOperation— adjust if the legacy server uses different numbers in your data set.SumResistance (
up_gr UpgradeType=8)Combine two wearables of the same equipment type into one with combined upgrade + resistances.
InfoiPacket NotEnoughGold, no chargesInfoiPacket NotEnoughIngredients, no chargesUpgradeItem (
up_gr UpgradeType=1)Upgrade a single wearable +0 → +10. Failure can degrade.
InfoiPacket NotEnoughIngredientsInfoiPacket NotEnoughGoldUpgradeItemProtected (
up_gr UpgradeType=20)Same as UpgradeItem but failure leaves the upgrade level alone (and costs double cellon).
InfoiPacket NotEnoughIngredientsRarifyItem (
up_gr UpgradeType=7)Rarify a wearable rarity +0 → +8. Failure resets rarity to 0.
InfoiPacket NotEnoughIngredientsRarifyItemProtected (
up_gr UpgradeType=21)Same as RarifyItem but uses blue stellar and failure preserves rarity.
InfoiPacket NotEnoughIngredientsCross-cutting / dispatch
up_grwith an unhandled UpgradeType (e.g. 41 PerfectSp) → server logsUNHANDLED_UPGRADE_TYPEwarning and no-ops; client sees no packetInShop = true) →WorldPacketHandlingStrategyrejects the packet before it reaches the handler (existing behavior — verify it still applies forup_gr)up_grpackets back-to-back from the same client → second one starts only after the first finishes (per-sessionAsyncLockinWorldPacketHandlingStrategy)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Chores