fix/hunt-quest-completion

[erwan-joly]

Summary

51 commits covering hunt/kill-quest fixes, the broader quest/script/revive flow refresh they exposed, expanded monster.dat parsing, and related operational tidy-up.

Quests & scripts

• Hunt / NumberOfKill quests: dedicated IQuestTypeHandlers, OnEntityDiedHandler drives progress, reward-race closed via "claim before payout"
• GoTo quests subscribe to CharacterMovedEvent (WalkPacketHandler publishes)
• qt Validate / Achieve route through script state instead of quest lookup
• q_pay separates "objectives met" from "turned in"
• qstlist field values + ShowDialog match the on-wire format
• CurrentScriptId persisted so tutorial survives relog

Revive / death flow

• Trace-accurate revive: dlgi + revival handler + DiePacket filter
• Monster death + respawn effect matches official capture
• CARD slot 2 "after dying" BCards (e.g. mandra Cry): BCard.Slot added + migration, NpcCombatCatalog.GetDeathBCards, DeathBCardHandler applies DecreaseKillerHp / IncreaseKillerHp

Combat AI

• MonsterAi now drives both monsters and NPC guards through INonPlayableEntity — mixed target pool (players + other-race monsters), faction = Race, stationary entities attack-in-range without pursuing

Miniland

• InitializeAsync self-heals missing MinilandDto at login (legacy characters + failed inserts)
• Dropped redundant insert from CharNewPacketHandler
• Disconnect-handler ordering: Save before Miniland cleanup

Inventory

• Mount (pocket 9) + Raid (pocket 10) tabs + noscore_pocket_type enum migration

Parser infra

• FluentParserBuilder.Field(...) learns a reads: tuple array so helper-backed lambdas mark their columns Parsed instead of being mislabelled NonParsed
• Doc generator suppresses NonParsed Doc rows when a Parsed Field covers the same column
• DatDocumentationSnapshotTests regenerates per-.dat Markdown under documentation/dat/
• --folder CLI flag for non-interactive parser runs

monster.dat parsing expansion

• CanWalk (replaces MapMonster.IsMoving) sourced from ETC bit 0
• ETC bitmask expanded into 13 booleans (normal-form)
• 14 new known columns: IsPercentileDmg, CanOnlyBeDmgedByJajamaruLastSkill, VisibleOnMinimapAsGreenDot, IsValhallaPartner, EffectIdConstantly, EffectIdOnDeath, PetInfoVal1..4, IconId, SpawnMobOrColor, SpriteSize, CellSize, GroupAttack, BasicHitChance, DashSpeed
• Only genuinely-unknown columns remain flagged NonParsed (ARMOR raw stats, unknown0..5, partnerUnused)

Ops / polish

• Wolverine ExtensionDiscovery.ManualOnly silences the BuildHost-netcore/*.dll log spam while keeping handler discovery on
• Directory.Packages.props central versioning (already in place, bumped NosCore.Packets 16.3 → 16.8 along the way)
• TryGetComponent / HasComponent guarded against dead entity handles
• Gold pickup sayi2 passes item vnum instead of pre-localized name

Test plan

• Hunt + NumberOfKill quests complete on kill, payout once
• GoTo quests advance on walk
• Mandra death reduces killer HP by the CARD[2] FirstData% (mandra vnum 19/20/21 sanity check)
• Guard vnum 322 attacks hostile mobs within NoticeRange
• Legacy character 4 / 5 login + save no longer throws No Miniland registered
• LoginServer boots without PendingModelChangesWarning
• dotnet test NosCore.sln green
• DatDocumentationSnapshotTests regenerates clean diffs against committed documentation/dat/*.md

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • All classes optionally available at character creation; Mount and Raid inventory pockets; in-game quest add/complete actions and completion events; client-driven revival options; new consumable effects (hair color/style, gender change, sealed tarot/box rewards, pet-basket bonuses).

• Improvements

  • Enhanced quest objective tracking and completion flows; richer NPC/monster AI and reward distribution; movement now publishes position updates; starter equipment/skill selection refined.

• Documentation

  • New DAT format reference docs for items, skills, monsters, quests, cards.

• Infrastructure

  • CI workflow and Docker tagging/publishing updates; centralized package/version management.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds repo-wide MSBuild/central package management and CI/Docker workflow updates; expands DB schema (quest objectives, NPC fields, BCard slot, pocket enums); refactors quest/AI/item-use flows into event-driven handlers; extends parser tooling with documentation generation; and updates many project files and tests.

Changes

Cohort / File(s)	Summary
CI / Docker / Compose \.github/workflows/dotnet.yml, docker-compose.yml, build-exe.cmd	Bumped GH Action/docker-action versions; multi-tag image publishing (latest + sha); upgraded Postgres image and volume path; updated runtime targets; whitespace/formatting normalizations.
Repo MSBuild & NuGet Directory.Build.props, Directory.Packages.props, NuGet.config, **/*.csproj	Add global props (LangVersion/Nullable/warnings moved); central package versions file; removed many per-project pinned PackageReference versions; NuGet sources cleared/redeclared.
World config & core models configuration/world.yml, src/NosCore.Core/Configuration/WorldConfiguration.cs	Reworked starter equipment/skills into per-class × StarterOrigin maps; added AllClassAvailableOnCreate, StarterOrigin enum; BasicEquipment gains Rare and Upgrade.
Enums & DB entities src/NosCore.Data/Enumerations/*, src/NosCore.Database/Entities/*	Added ApplyHairStyle and new pocket enum values (Mount, Raid); added BCard.Slot; removed MapMonster.IsMoving; expanded NpcMonster/NpcMonsterSkill with many fields; added CharacterQuestObjective entity and navigation properties.
EF Core migrations & snapshot src/NosCore.Database/Migrations/*, src/NosCore.Database/NosCoreContextModelSnapshot.cs	New migrations: create CharacterQuestObjective table; add BCard.Slot; add many NpcMonster/NpcMonsterSkill columns; add CanWalk; drop MapMonster.IsMoving; update PostgreSQL enum annotations; snapshot updated.
Quest system & messaging src/NosCore.GameObject/Services/QuestService/*, src/NosCore.GameObject/Messaging/Handlers/Quest/*, src/NosCore.GameObject/Messaging/Events/*, src/NosCore.GameObject/Services/QuestService/IQuestService.cs	Event-driven quest changes: IQuestService extended, CompleteQuestAsync added; KillQuestHandlerBase introduced; multiple quest handlers and handlers for character-move/kill/complete added; quest persistence/objective reconciliation updated.
AI & battle services src/NosCore.GameObject/Services/BattleService/*, src/NosCore.GameObject/Messaging/Handlers/Battle/*, src/NosCore.GameObject/Services/BattleService/NpcCombatCatalog.cs	Generalized AI to INonPlayableEntity (IMonsterAi/MonsterAi refactor); new DeathBCardHandler and death-card lookup; reward distribution now takes hit snapshot; spectator die broadcasts and ally checks updated.
Item-use handlers src/NosCore.GameObject/Messaging/Handlers/UseItem/*	Added many item-use handlers: BoxEffectHandler, ChangeGenderHandler, HairDieHandler, PetBasketHandler, SealedTarotCardHandler; each implements effect validation, reward/item creation, inventory and packet flows.
Networking & session lifecycle src/NosCore.GameObject/Networking/*, src/NosCore.GameObject/Hosting/Modules/NetworkingModule.cs	Add ISessionDisconnectHandler.Order default; deterministic disconnect ordering; wrapper methods to dispatch session async calls with exception logging.
Packet handlers & gameplay flows src/NosCore.PacketHandlers/*	RevivalPacketHandler added (client-driven revive paths); WalkPacketHandler now publishes CharacterMovedEvent; character creation/select read starter-origin config; quest packet flows adjusted.
Map/world & inventory src/NosCore.GameObject/.../MapInstance.cs, src/NosCore.GameObject/Ecs/MapWorld.cs, src/NosCore.GameObject/Services/InventoryService/InventoryService.cs, src/NosCore.GameObject/Ecs/Extensions/CharacterEntityExtension.cs	Component lookups respect entity liveness; NPC movement derived from NpcMonster.CanWalk; added Mount and Raid pockets to inventory generation and inv packets; VisualComponent NoAttack flag change on NPC creation.
Save & Miniland src/NosCore.GameObject/Services/SaveService/SaveService.cs, src/NosCore.GameObject/Services/MinilandService/MinilandService.cs	SaveService persists CharacterQuestObjective rows with reconciliation/upsert/delete; Miniland initialization attempts create when missing and throws clearer InvalidOperationException on failure.
Parser infra & documentation src/NosCore.Parser/Parsers/Generic/*, src/NosCore.Parser/Parsers/*, test/NosCore.Parser.Tests/*	Introduce DatFieldMetadata and FluentParserBuilder metadata, DatDocumentationGenerator; extract BuildParser helpers across parsers; add documentation snapshot tests and generator tests.
Parser CLI & runtime src/NosCore.Parser/Parser*.cs, src/NosCore.Parser/ParserCliOptions.cs	Add ParserCliOptions for non-interactive folder runs; parser supports non-interactive execution and stops host when done; bootstrap registers CLI options and skips interactive wait when invoked with folder.
Tests & fixtures test/**	Update many tests to new dependencies (quest-objective DAO, message bus, item builder), add DAT doc tests and snapshots, remove IsMoving-based parser tests, update expected exception types and mock setups.
Misc tooling & generators tools/*, src/NosCore.GameObject/Messaging/WolverineHostExtensions.cs, tools/NosCore.EcsGenerator/*	csproj/tooling files normalized and unpinned; Wolverine discovery changed to ManualOnly; EcsGenerator conversion operators now throw on missing component.

sequenceDiagram
    participant Client as Client
    participant WalkHandler as WalkPacketHandler
    participant MessageBus as MessageBus
    participant OnMove as OnCharacterMovedHandler
    participant QuestService as QuestService
    participant DB as Database

    Client->>WalkHandler: Send Walk packet
    WalkHandler->>MessageBus: PublishAsync(CharacterMovedEvent)
    MessageBus-->>OnMove: Deliver CharacterMovedEvent
    OnMove->>QuestService: ValidateAsync(character, quest)
    QuestService->>DB: CompleteQuestAsync / persist quest & objectives
    DB-->>QuestService: Persist ACK
    QuestService->>MessageBus: PublishAsync(QuestCompletedEvent)
    MessageBus-->>OnMove: Deliver QuestCompletedEvent

Loading

🎯 5 (Critical) | ⏱️ ~120 minutes

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/hunt-quest-completion

[coderabbitai]

Actionable comments posted: 18

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/NosCore.GameObject/Services/InventoryService/InventoryService.cs (1)

28-37: ⚠️ Potential issue | 🟠 Major

Remove byte cast from GetMaxSlots return to prevent silent inventory slot-count wraparound.

Line 28 casts the switch result to byte. Since Expensions values are bytes (0–255), computed slot counts can exceed 255 and wrap around (e.g., 260 becomes 4, 305 becomes 49), causing inventory capacity checks to silently fail.

Suggested refactor

-            return (byte)(pocket switch
+            return pocket switch
             {
                 NoscorePocketType.Miniland => 50 + Expensions[pocket],
                 NoscorePocketType.Specialist => 45 + Expensions[pocket],
                 NoscorePocketType.Costume => 60 + Expensions[pocket],
                 NoscorePocketType.Wear => 17,
                 NoscorePocketType.Mount => 20 + Expensions[pocket],
                 NoscorePocketType.Raid => 63 + Expensions[pocket],
                 _ => configuration.Value.BackpackSize + Expensions[pocket]
-            });
+            };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Services/InventoryService/InventoryService.cs` around
lines 28 - 37, The GetMaxSlots method in InventoryService (the return currently
written as (byte)(pocket switch ...)) must stop casting the computed slot count
to byte to avoid wraparound; change the GetMaxSlots return type from byte to
int, remove the (byte) cast so the switch expressions return an int (e.g.,
configuration.Value.BackpackSize + Expensions[pocket] will become int), and then
update all callers of GetMaxSlots to accept an int (and adjust any places that
previously assumed a byte, including validation/serialization code) so inventory
slot math no longer silently wraps.

src/NosCore.GameObject/Services/QuestService/QuestService.cs (1)

303-311: ⚠️ Potential issue | 🟠 Major

Daily quest cooldown is checked against the wrong quest instance.

This branch uses characterQuest.Value?.CompletedOn, but characterQuest is the active quest lookup (CompletedOn == null). For a previously finished daily quest, that value is always null, so the once-per-day gate is bypassed.

Suggested fix

-            if (quest.IsDaily && (characterQuest.Value?.CompletedOn?.Plus(Duration.FromDays(1)) > clock.GetCurrentInstant()))
+            var lastCompletion = charQues.Value?.CompletedOn;
+            if (quest.IsDaily && (lastCompletion?.Plus(Duration.FromDays(1)) > clock.GetCurrentInstant()))
             {
                 await character.SendPacketAsync(new MsgiPacket
                 {

🤖 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
303 - 311, The daily-quest cooldown check is using
characterQuest.Value?.CompletedOn (the active quest entry which is null while
in-progress) so it always bypasses the once-per-day gate; change the check to
use the last completed quest entry for this quest (e.g., find the character's
quest record where QuestId == quest.Id and CompletedOn != null, assign it to a
variable like lastCompletedQuest and use lastCompletedQuest.CompletedOn) and
compare lastCompletedQuest.CompletedOn.Plus(Duration.FromDays(1)) to
clock.GetCurrentInstant(); keep the existing MsgiPacket/Message behavior
unchanged.

src/NosCore.GameObject/Services/BattleService/MonsterAi.cs (1)

201-245: ⚠️ Potential issue | 🟠 Major

Selected monster skills are never actually used.

PickSkill(...) always returns CastId: 0, and AttackAsync(...) also stores cooldowns under key 0. That means every “skill” attack is sent as a basic hit and all skills share the same cooldown bucket.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Services/BattleService/MonsterAi.cs` around lines 201
- 245, PickSkill is returning CastId: 0 and AttackAsync is checking/storing
cooldowns under key 0, so skills never get used or have per-skill cooldowns;
update PickSkill to set ChosenSkill.CastId to the actual skill identifier (use
sk.SkillVNum, cast to the expected long if needed) instead of 0, and update
AttackAsync to check and set cooldowns using the passed castId (use
TryGetValue(castId) and NextUsableAt[castId] = ...) rather than hardcoding 0 so
each skill uses its own cast id and cooldown bucket; adjust any necessary type
casts between int and long for SkillVNum/castId.

🧹 Nitpick comments (19)

Directory.Build.props (1)

3-3: Pin LangVersion for reproducible CI builds.

No global.json exists to pin the SDK version, and LangVersion is set to latest. This allows compiler behavior to change after SDK upgrades. Set an explicit language version (e.g., 12.0 or latest-preview) instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Directory.Build.props` at line 3, The LangVersion property in
Directory.Build.props is set to "latest", which makes CI builds
non-reproducible; change the <LangVersion> value from "latest" to an explicit
compiler version (for example "12.0" or "latest-preview") so the C# language
level is pinned for reproducible CI builds—update the LangVersion element
accordingly in Directory.Build.props.

src/NosCore.PacketHandlers/Quest/QTPacketHandler.cs (1)

40-48: Consider using TryGetValue or a more idiomatic check.

The default KeyValuePair check works but is slightly awkward. A more readable alternative would be to use LINQ's .FirstOrDefault() with a null check pattern or use TryGetValue if you have the key.

♻️ Optional: Simplify the null check

 case QuestActionType.GiveUp:
-    var charQuest = session.Character.Quests.FirstOrDefault(q => q.Value.QuestId == qtPacket.Data);
-    if (charQuest.Equals(new KeyValuePair<Guid, CharacterQuest>()))
+    var charQuest = session.Character.Quests.FirstOrDefault(q => q.Value.QuestId == qtPacket.Data);
+    if (charQuest.Key == Guid.Empty)
     {
         return;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.PacketHandlers/Quest/QTPacketHandler.cs` around lines 40 - 48,
The current GiveUp handling uses FirstOrDefault() and compares charQuest to a
new KeyValuePair to detect absence; replace that awkward check with an idiomatic
lookup: attempt to find the quest by key/value using a clearer pattern (e.g.,
use session.Character.Quests.Any/First with a null check or, if you have the
quest id as a key, use session.Character.Quests.TryGetValue) and only call
TryRemove(charQuest.Key, out var questToRemove) and
questToRemove?.GenerateQstiPacket(false) when the quest actually exists; update
the QuestActionType.GiveUp branch to perform the clearer presence check and then
remove and generate the packet.

src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs (1)

144-166: Consider sending GenerateStat() to the map instance instead of just the session.

Currently, the TpPacket and RevivePacket are broadcast to mapInstance, but GenerateStat() is only sent to the session. Depending on game requirements, other players might need to see the revived player's updated stats.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs` around lines 144 -
166, The BroadcastReviveAsync method currently sends the TpPacket and
RevivePacket to mapInstance but only calls
session.SendPacketAsync(character.GenerateStat()), so other players won't see
the revived player's updated stats; change that call to await
mapInstance.SendPacketAsync(character.GenerateStat()).ConfigureAwait(false)
(keeping the same ConfigureAwait usage and null checks for mapInstance) so the
stat update is broadcast to everyone on the map; keep BroadcastReviveAsync,
mapInstance, session and GenerateStat referenced exactly as in the diff.

test/NosCore.Parser.Tests/MapNpcParserTests.cs (1)

55-57: Optional: add one non-walkable fixture case.

With all mock monsters set to CanWalk = true, the stationary branch is not exercised here. A single CanWalk = false case would improve regression coverage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/NosCore.Parser.Tests/MapNpcParserTests.cs` around lines 55 - 57, Add a
non-walkable mock NPC to exercise the stationary branch: in the test fixture
where you create new() objects with properties NpcMonsterVNum and CanWalk (the
three entries with NpcMonsterVNum = 1, 2, 100), add a fourth entry with a unique
NpcMonsterVNum (e.g., 200) and CanWalk = false so the parser's stationary logic
is covered by this test.

test/NosCore.PacketHandlers.Tests/Movement/WalkPacketHandlerTests.cs (1)

39-45: Consider asserting the movement event side effect.

Now that the handler is wired with a message bus, add one assertion path that verifies the move publishes the expected event to prevent silent regressions.

🤖 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 39 - 45, The test currently passes a plain new
Mock<Wolverine.IMessageBus>().Object to WalkPacketHandler so it cannot verify
side effects; replace that with a Mock<Wolverine.IMessageBus> instance, pass
messageBusMock.Object into the Handler constructor, exercise the handler, and
then Verify that the message bus published/sent the expected movement event (use
messageBusMock.Verify(...) for the Publish/Send method and assert the event type
and key properties like playerId/from/to or coordinates match the test packet)
to ensure the move emits the correct event.

documentation/dat/quest.dat.md (2)

3-4: Empty fenced code block should be removed or given content.

The empty code block serves no purpose and triggers the MD040 linting warning. Either remove it or add the actual raw format sample if this is a placeholder.

🧹 Proposed fix

-```
-```
+<!-- Raw format sample to be documented -->

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@documentation/dat/quest.dat.md` around lines 3 - 4, The markdown file
documentation/dat/quest.dat.md contains an empty fenced code block that triggers
MD040; remove the empty triple-backtick block or replace it with a meaningful
raw format example. Locate the empty fenced code block in
documentation/dat/quest.dat.md and either delete those lines entirely or
populate them with the intended raw format sample (add a short descriptive
comment or example payload) so the lint warning is resolved.

---

7-26: Type names will render incorrectly due to backtick escaping.

Types like Nullable1andICollection1 contain unescaped backticks that break markdown rendering. Consider using friendlier type names (e.g., int?, short?, ICollection<QuestObjective>).

Also, add a blank line before the table per MD058.

📝 Example fix for first few rows

 ## Computed / multi-section fields
+
 | DTO property | Type | Source | Description |
 |---|---|---|---|
 | AutoFinish | Boolean |  |  |
 | DescI18NKey | String |  |  |
-| EndDialogId | Nullable`1 |  |  |
+| EndDialogId | int? |  |  |

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@documentation/dat/quest.dat.md` around lines 7 - 26, The table contains raw
generic type names like Nullable`1 and ICollection`1 (and primitive names like
Int16/Byte) that render poorly; replace them with friendly, markdown-safe type
representations (e.g., use short nullable syntax like "int?" or "short?" or
explicit generics like "ICollection<QuestObjective>",
"ICollection<QuestQuestReward>", and "QuestType") or escape/backtick them
properly so backticks don't break rendering, and ensure each DTO property row
(e.g., AutoFinish, DescI18NKey, EndDialogId, LevelMin/LevelMax, QuestId,
QuestObjective, QuestQuestReward, NextQuestId, RequiredQuestId, StartDialogId,
TargetMap/TargetX/TargetY, TitleI18NKey) uses the updated type names; also add a
single blank line immediately before the table to satisfy MD058.

src/NosCore.Database/Migrations/20260421054001_AddBCardSlot.cs (1)

13-18: Inconsistent C# and SQL type declaration for the new column.

The Slot property is declared as byte in the BCard entity (line 49 of BCard.cs), and the migration correctly uses AddColumn<byte>. However, specifying type: "smallint" creates a mismatch: byte is an unsigned 8-bit type, while PostgreSQL's smallint is a signed 16-bit type. This works in practice because EF Core handles the conversion and the value range (0–255) fits within smallint, but using AddColumn<short> would make the mapping explicit and consistent with the SQL type.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Database/Migrations/20260421054001_AddBCardSlot.cs` around lines
13 - 18, The migration AddBCardSlot uses AddColumn<byte> but specifies SQL type
"smallint", causing a C#/SQL type mismatch with the BCard.Slot property; update
the migration to use AddColumn<short> (and defaultValue: (short)0) so the CLR
type aligns with the SQL "smallint" mapping in the AddBCardSlot migration and
remains consistent with the BCard.Slot mapping intent.

src/NosCore.GameObject/Services/MinilandService/MinilandService.cs (1)

109-116: Guard the upsert result before mapping to avoid latent null-path failures.

minilandInfoDto is used later for mapping without an explicit post-upsert check. Adding a guard keeps failure semantics explicit instead of surfacing later as a mapping/null dereference issue.

Proposed reliability guard

             if (minilandInfoDto == null)
             {
                 minilandInfoDto = await minilandDao.TryInsertOrUpdateAsync(new MinilandDto
                 {
                     MinilandId = Guid.NewGuid(),
                     State = MinilandState.Open,
                     MinilandMessage = ((short)Game18NConstString.Welcome).ToString(),
                     OwnerId = characterId,
                     WelcomeMusicInfo = 3800
                 });
+                if (minilandInfoDto == null)
+                {
+                    throw new InvalidOperationException($"Failed to initialize Miniland row for character {characterId}.");
+                }
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Services/MinilandService/MinilandService.cs` around
lines 109 - 116, After calling TryInsertOrUpdateAsync to assign minilandInfoDto
in MinilandService.cs, add an explicit null-check on minilandInfoDto before any
mapping or further use (the call to TryInsertOrUpdateAsync that constructs a new
MinilandDto is the location). If the upsert returned null, handle it immediately
(throw a descriptive exception, return a failed Result/Response, or log and
abort) so downstream mapping code does not encounter a null dereference; ensure
the error message references the OwnerId/characterId and that mapping only runs
when minilandInfoDto is non-null.

src/NosCore.PacketHandlers/Game/ReqInfoPacketHandler.cs (1)

123-124: Include session context in mate no-op debug logs.

Adding session or character identifiers here would make production tracing easier when this path is hit frequently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.PacketHandlers/Game/ReqInfoPacketHandler.cs` around lines 123 -
124, The debug log in ReqInfoPacketHandler (the logger.Debug call that logs
"req_info 6 <mateTransportId={TransportId}> received but mate subsystem is not
wired") lacks session/character context; update this log to include the current
session and/or character identifiers (for example session.Id, session.AccountId,
or packet.SourceVNum/TargetVNum/CharacterVNum as available) so production traces
can correlate events, and pass those values into the logger.Debug call alongside
TransportId; ensure you reference the same logger instance and keep the message
concise but containing the chosen session/character fields.

src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs (1)

985-988: Avoid silently swallowing missing starter-pack config.

Falling back to an empty list makes misconfigurations hard to detect and can leave class-changed characters without expected equipment. Consider failing fast (or at least logging loudly) when the class/origin entry is missing.

🔧 Strict-fail option

-        var itemsToAdd = worldConfiguration.Value.BasicEquipments.TryGetValue(classType.ToString(), out var byOrigin)
-            && byOrigin.TryGetValue(StarterOrigin.CreateAndUpgrade, out var pack)
-            ? pack
-            : new List<BasicEquipment>();
+        if (!worldConfiguration.Value.BasicEquipments.TryGetValue(classType.ToString(), out var byOrigin) ||
+            !byOrigin.TryGetValue(StarterOrigin.CreateAndUpgrade, out var pack))
+        {
+            throw new InvalidOperationException(
+                $"Missing starter equipment config for class '{classType}' and origin '{StarterOrigin.CreateAndUpgrade}'.");
+        }
+        var itemsToAdd = pack;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs` around lines
985 - 988, The current assignment to itemsToAdd silently falls back to an empty
list when BasicEquipments lacks the classType or the
StarterOrigin.CreateAndUpgrade entry; update the logic in PlayerBundleExtensions
(the block that uses worldConfiguration.Value.BasicEquipments, classType and
StarterOrigin.CreateAndUpgrade) to detect missing configuration and fail loudly:
if BasicEquipments.TryGetValue(classType.ToString(), out var byOrigin) or
byOrigin.TryGetValue(StarterOrigin.CreateAndUpgrade, out var pack) returns
false, either throw a descriptive exception (including classType and origin) or
log an error and return early instead of returning an empty list so
misconfigurations are immediately visible. Ensure the error path references the
same symbols (worldConfiguration.Value.BasicEquipments, classType,
StarterOrigin.CreateAndUpgrade, itemsToAdd).

documentation/dat/Item.dat.md (2)

3-8: Add language specifier to fenced code block.

Per markdownlint MD040, fenced code blocks should have a language specified. Since this is a record layout template, consider using text or plaintext.

📝 Suggested fix

-```
+```text
 	FLAG	0	0	0	{IsSoldable}	...

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@documentation/dat/Item.dat.md` around lines 3 - 8, The fenced code block
showing the Item.dat record template (the block containing the tokens "FLAG",
"INDEX", "NAME", "VNUM") lacks a language specifier; update the opening
triple-backtick to include a language such as text or plaintext (e.g., ```text)
so the block complies with markdownlint MD040 and remains a plain record-layout
snippet.

---

41-41: Add blank line before table for proper markdown rendering.

The INDEX table (and subsequent tables) should be preceded by a blank line for consistent rendering across markdown parsers.

📝 Suggested fix

 | 26 | NonParsed | UnknownLastBit |  |  |

+
 ## INDEX

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@documentation/dat/Item.dat.md` at line 41, Insert a single blank line between
the "## INDEX" heading and the table that follows so the Markdown table renders
correctly; locate the "## INDEX" heading in Item.dat.md and add an empty line
immediately after it (and apply the same blank-line-before-table fix to any
subsequent tables in this file).

src/NosCore.GameObject/Messaging/Handlers/UseItem/ChangeGenderHandler.cs (1)

47-48: Consider checking item amount before removal.

The handler removes 1 item without first verifying that evt.InventoryItem.ItemInstance.Amount >= 1. While this is likely always true for usable items, a defensive check could prevent unexpected behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Messaging/Handlers/UseItem/ChangeGenderHandler.cs`
around lines 47 - 48, In ChangeGenderHandler, before calling
InventoryService.RemoveItemAmountFromInventory(1,
evt.InventoryItem.ItemInstanceId) and sending the pocket update via
evt.InventoryItem.GeneratePocketChange(...) / session.SendPacketAsync(...),
first verify evt.InventoryItem.ItemInstance.Amount >= 1 (or > 0); if the check
fails, skip the removal and the pocket-change send (or handle as an error by
logging or returning), otherwise perform the removal and then send the pocket
change — this avoids removing items when amount is zero and prevents
inconsistent state.

src/NosCore.PacketHandlers/CharacterScreen/CharNewJobPacketHandler.cs (1)

57-57: Track the TODO for recreation cooldown.

The 30-day cooldown for MartialArtist recreation is noted but not implemented. Consider creating a tracking issue if this is required functionality.

Would you like me to open an issue to track the MartialArtist recreation cooldown implementation?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.PacketHandlers/CharacterScreen/CharNewJobPacketHandler.cs` at
line 57, The TODO comment "//todo add cooldown for recreate 30days" in
CharNewJobPacketHandler indicates missing work to enforce a 30-day cooldown for
MartialArtist recreation; create a formal tracking issue in the repo (including
desired behavior, acceptance criteria, and any DB changes), then update the code
comment in CharNewJobPacketHandler (and any related method like HandleNewJob or
the class CharNewJobPacketHandler) to reference the newly created issue ID
(e.g., TODO: see issue `#123`) so the requirement is tracked, and add the issue to
the project board/milestone and changelog so implementation isn't lost.

src/NosCore.Parser/Parsers/MapMonsterParser.cs (2)

38-42: Minor: Unused npcMonster variable.

The out var npcMonster result from TryGetValue is never used. If only existence checking is needed, ContainsKey would be cleaner.

♻️ Simplify to ContainsKey

-if (!npcMonsterdb.TryGetValue(vnum, out var npcMonster))
+if (!npcMonsterdb.ContainsKey(vnum))
 {
     continue;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parsers/MapMonsterParser.cs` around lines 38 - 42, The
code calls npcMonsterdb.TryGetValue(vnum, out var npcMonster) but never uses
npcMonster; replace this with an existence check (e.g.,
npcMonsterdb.ContainsKey(vnum)) or use the discard pattern (out _) to avoid the
unused variable in MapMonsterParser.cs; update the condition that currently uses
TryGetValue so it only checks presence of the key and keeps the same control
flow.

---

55-56: Consider using dictionary for mapMonsterdb lookups.

While npcMonsterdb was optimized to use dictionary lookup (O(1)), mapMonsterdb still uses FirstOrDefault (O(n)) on every packet iteration. For large datasets, converting mapMonsterdb to a HashSet<int> keyed by MapMonsterId would improve performance.

♻️ Optional performance improvement

-var mapMonsterdb = mapMonsterDao.LoadAll().ToList();
+var existingMonsterIds = mapMonsterDao.LoadAll().Select(m => m.MapMonsterId).ToHashSet();
 var npcMonsterdb = npcMonsterDao.LoadAll().ToDictionary(n => n.NpcMonsterVNum);
 ...
-if ((mapMonsterdb.FirstOrDefault(s => s.MapMonsterId.Equals(monster.MapMonsterId)) != null)
+if (existingMonsterIds.Contains(monster.MapMonsterId)
     || (monsters.Count(i => i.MapMonsterId == monster.MapMonsterId) != 0))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parsers/MapMonsterParser.cs` around lines 55 - 56, Replace
the O(n) FirstOrDefault lookup on mapMonsterdb with an O(1) membership check by
converting mapMonsterdb to a HashSet<int> (or Dictionary<int, ...>) keyed by
MapMonsterId and then using Contains; update the code in MapMonsterParser (where
mapMonsterdb is populated and where the conditional uses
mapMonsterdb.FirstOrDefault and monster.MapMonsterId) to build the HashSet once
and change the conditional to mapMonsterIdSet.Contains(monster.MapMonsterId)
(keeping the existing check against monsters.Count(...) or optionally replace
that with a HashSet lookup of monsters' MapMonsterId as well).

test/NosCore.Parser.Tests/DatDocumentationSnapshotTests.cs (1)

46-52: Consider making snapshot tests verify rather than regenerate.

These tests always regenerate documentation files, which has side effects on the working tree. Consider either:

1. Verify mode (typical for snapshot tests): Compare generated content against existing files and fail if they differ. This catches unintentional drift.
2. Separate tooling: Move regeneration to a CLI command or build task, keeping tests focused on verification.

With the current approach, forgetting to commit regenerated docs after parser changes won't cause CI failures.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/NosCore.Parser.Tests/DatDocumentationSnapshotTests.cs` around lines 46 -
52, The RegenerateFor<T> helper currently always writes generated markdown to
disk (using DatDocumentationGenerator.Generate, DocumentationPaths.For and
builder.FileName) causing tests to mutate the working tree; change it to
verification mode: compute md = DatDocumentationGenerator.Generate(builder),
read the existing file at DocumentationPaths.For(builder.FileName + ".md"), and
assert the contents are equal (failing the test if they differ). Optionally
support an explicit regeneration flag (e.g., env var or test switch) to
overwrite the file only when intentionally requested, but default behavior must
be comparison/assertion so CI fails on drift.

src/NosCore.Parser/Parsers/Generic/FluentParserBuilder.cs (1)

148-155: Consider adding validation for column descriptions length.

The ExpectedColumns method accepts columnDescriptions but doesn't validate that its length matches count. If they differ, the generated documentation may be misleading.

🔧 Optional: Add length validation

 public FluentParserBuilder<T> ExpectedColumns(string section, int count, params string[] columnDescriptions)
 {
+    if (columnDescriptions is { Length: > 0 } && columnDescriptions.Length != count)
+    {
+        throw new ArgumentException(
+            $"Column descriptions length ({columnDescriptions.Length}) must match count ({count})",
+            nameof(columnDescriptions));
+    }
     _sectionSchemas.Add(new DatSectionSchema(section, count,
         columnDescriptions is { Length: > 0 } ? columnDescriptions : null));
     return this;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parsers/Generic/FluentParserBuilder.cs` around lines 148 -
155, The ExpectedColumns method on FluentParserBuilder<T> currently accepts
columnDescriptions without verifying its length against count; add a validation
at the start of FluentParserBuilder<T>.ExpectedColumns so that when
columnDescriptions is provided (i.e., Length > 0) its Length must equal count,
otherwise throw an ArgumentException (or ArgumentOutOfRangeException) describing
the section name, expected count and actual descriptions length; keep the
existing behavior of passing null to new DatSectionSchema when no descriptions
are provided.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@build-exe.cmd`:
- Around line 1-6: The .cmd script currently uses LF-only line endings which
breaks Windows batch execution; convert the entire file to CRLF line endings so
the commands (dotnet build -r win-x64, dotnet build -r linux-x64, dotnet build
-r linux-musl-x64, mkdir .\build\Configuration, cd Configuration, copy *.*
..\build\Configuration\) have CRLF termination. Fix by saving the file with CRLF
in your editor or running a line-ending conversion, and to prevent regressions
add a .gitattributes rule to enforce CRLF for *.cmd files (or set core.autocrlf
appropriately) so the file stays CRLF in the repo.
- Around line 1-3: The build script runs three dotnet build commands that can
mask failures because later commands overwrite ERRORLEVEL; modify each
invocation of "dotnet build -r win-x64", "dotnet build -r linux-x64", and
"dotnet build -r linux-musl-x64" to fail-fast by checking the exit status and
aborting on failure (e.g., append a CMD-style guard such as exiting with
non-zero when the build fails) so any failed step stops the script immediately.

In `@docker-compose.yml`:
- Line 4: The docker-compose service currently upgrades the image to "image:
postgres:17.2-alpine3.21" while still using the existing bind mount
"./postgres:/var/lib/postgresql/data", which will break for developers with PG12
data; either update the docker-compose bind mount to a new directory (e.g.,
change "./postgres" to "./postgres17" so Postgres 17 initializes a fresh data
dir) or add a migration/remove-data instruction in the repo documentation
describing how to dump/pg_upgrade or safely reset "./postgres" before starting
Postgres 17; locate the postgres service in docker-compose.yml (look for the
image: postgres:17.2-alpine3.21 line and the volume mapping
"./postgres:/var/lib/postgresql/data") and implement one of these two remedies.

In `@documentation/dat/Card.dat.md`:
- Around line 3-55: The generated markdown is tripping MD040/MD058 because the
code fence lacks a language and there are missing blank lines around tables;
update the template that emits the card snapshot so the top block uses a fenced
code block with a language (e.g., ```text) and ensure a single blank line is
emitted before and after each table header (e.g., before and after lines like
"## EFFECT" and the table starting with "| Column | Status | Name | Type |
Description |"). Locate the generator output that writes the snippet containing
"EFFECT	{EffectId}" and the table blocks (the sections titled "## EFFECT", "##
GROUP", etc.) and modify the rendering helpers (renderCodeBlock / renderTable or
equivalent) to inject the language token and surrounding blank lines so future
snapshots are lint-clean.

In `@documentation/dat/monster.dat.md`:
- Around line 3-416: The markdown snapshot contains a fenced schema block and
many tables that trigger MD040/MD058; update the top fenced block (the
triple-backtick block that contains the AINFO...ZSKILL schema) to include a
language token (e.g. change ``` to ```text) and ensure each rendered table (the
sections starting with headings like ## AINFO, ## ARMOR, etc.) is surrounded by
a blank line above and below the table so there is an empty line before the
first |--- and after the final row; adjust the block and table spacing in the
generator or the produced markdown so future regenerations emit the fenced block
with a language and every table has the required blank lines.

In `@documentation/dat/Skill.dat.md`:
- Around line 3-8: Remove the stray empty fenced-code block and ensure the
markdown table under the "Computed / multi-section fields" heading is preceded
by a single blank line; specifically, delete the empty ``` block and add one
blank line between the "Computed / multi-section fields" heading and the table
header row so the table (| DTO property | Type | Source | Description |) renders
without markdownlint MD040/MD058 warnings.

In `@src/NosCore.GameObject/Messaging/Handlers/Battle/DeathBCardHandler.cs`:
- Around line 40-59: The handler currently mutates killer.Hp (the assignments in
the DeathBCardHandler case for DecreaseKillerHp/IncreaseKillerHp) before calling
killer.SendPacketAsync and killer.MapInstance.SendPacketAsync, so packet send
failures can cause retries to reapply the HP change; to fix this, make the
post-update sends non-fatal by wrapping the await calls to
killer.SendPacketAsync(...) and killer.MapInstance.SendPacketAsync(...) in
try/catch blocks that log errors but do not rethrow (or otherwise swallow send
exceptions), or alternatively move the mutation behind an idempotent boundary
(e.g., compute the new HP in a local variable and persist/apply it only after
successful non-fatal send attempts or after an idempotency check), ensuring the
changed flag and killer.Hp assignment are only committed once and send failures
do not cause duplicated application.

In `@src/NosCore.GameObject/Messaging/Handlers/Quest/OnEntityDiedHandler.cs`:
- Around line 42-50: The current batch wait (Task.WhenAll) in
OnEntityDiedHandler causes one contributor failure to abort the whole death
event; instead, call questService.OnMonsterKilledAsync per contributor and
handle failures individually: iterate the hits, use TryFindCharacter to get
character, then await OnMonsterKilledAsync inside a try/catch (or add per-task
continuation that catches/logs exceptions) so each contributor is processed
independently and exceptions are logged but not rethrown to abort other updates.

In `@src/NosCore.GameObject/Messaging/Handlers/UseItem/BoxEffectHandler.cs`:
- Around line 83-98: The handler currently captures only FirstOrDefault() from
InventoryService.AddItemToPocket in BoxEffectHandler (variable 'added') and
sends only one pocket change; instead, keep the full collection returned by
AddItemToPocket (e.g., call it addedEntries), check if the collection is null or
empty and if so send the NotEnoughSpace SayiPacket, otherwise iterate
addedEntries and send each entry's GeneratePocketChange packet (and then
continue sending the RdiPacket), ensuring all touched inventory entries from
AddItemToPocket are forwarded to the client.

In `@src/NosCore.GameObject/Messaging/Handlers/UseItem/PetBasketHandler.cs`:
- Around line 76-77: The pocket-change packet is being generated from the
pre-use state, so move the send to after the consume call: first call
session.Character.InventoryService.RemoveItemAmountFromInventory(1,
itemInstance.ItemInstanceId) to update the server-side stack, then call
session.SendPacketAsync(itemInstance.GeneratePocketChange((PocketType)itemInstance.Type,
itemInstance.Slot)) so the packet reflects the post-use state; reference
itemInstance.GeneratePocketChange, session.SendPacketAsync and
RemoveItemAmountFromInventory.

In `@src/NosCore.GameObject/Services/BattleService/BattleService.cs`:
- Around line 230-235: The AreAllies helper currently checks VisualType (in
AreAllies(IAliveEntity a, IAliveEntity b)) which treats all VisualType.Npc as
allies; change it to use the entity combat faction/race field instead (e.g.,
a.CombatFaction or a.Race) and compare factions (or call an existing
faction-alliance method like IsAlliedWith) so alliance is determined by combat
faction equality or alliance rules between a and b rather than VisualType;
update any null/unknown faction handling accordingly.

In `@src/NosCore.GameObject/Services/BattleService/MonsterAi.cs`:
- Line 107: The code currently lets stationary entities chase because callers in
MonsterAi call StepTowardAsync(...) unconditionally and StepTowardAsync mutates
PositionX/PositionY without verifying movement capability; update the logic so
callers (the out-of-range branch at the call sites around lines where
StepTowardAsync is invoked, e.g., the call at the first occurrence and the block
covering lines 253–285) only call StepTowardAsync if entity.CanWalk is true, and
also harden StepTowardAsync itself to early-return (no position mutation) when
the passed entity's CanWalk is false; reference the MonsterAi class and the
StepTowardAsync method when making these checks.

In
`@src/NosCore.GameObject/Services/QuestService/Handlers/KillQuestHandlerBase.cs`:
- Around line 21-42: The quest objective mutation (quest.ObjectiveProgress /
ObjectiveProgress.AddOrUpdate) is currently coupled to the awaited notification
(character.SendPacketAsync(quest.GenerateQstiPacket(false))) so a packet failure
can surface as a failed operation while state already changed; make the
notification best-effort by wrapping the SendPacketAsync call in a try/catch
that logs but does not rethrow (or otherwise swallow the exception) so
Incrementing logic in KillQuestHandlerBase remains committed even if
SendPacketAsync fails; keep the progressed check and mutation as-is
(ObjectiveProgress.AddOrUpdate, quest.ObjectiveProgress[...]) and only change
how GenerateQstiPacket/SendPacketAsync is invoked to avoid propagating
exceptions.

In `@src/NosCore.GameObject/Services/QuestService/Quest.cs`:
- Line 67: The ObjectiveCount is currently hardcoded to 5; update the assignment
where ObjectiveCount is set (in the creation of QuestObjectiveSubPackets) to use
the actual number of built objectives by replacing the literal with the dynamic
count (e.g., (byte)objectives.Count) so ObjectiveCount reflects the size of the
'objectives' list passed to QuestObjectiveSubPackets.

In `@src/NosCore.GameObject/Services/QuestService/QuestService.cs`:
- Around line 143-173: QPayAsync currently sets charQuest.CompletedOn before
applying rewards which allows item rewards to be skipped when AddItemToPocket
returns null, effectively losing rewards; change the flow in QPayAsync so you do
not persist CompletedOn (or broadcast QuestCompletedEvent / send quest packet)
until after all rewards are successfully applied, and if any ApplyRewardAsync
(or underlying AddItemToPocket) fails to add an item, undo any temporary state
and return false (or leave CompletedOn null). Concretely: inside QPayAsync (and
similarly in the code block at 176-209) acquire the lock on charQuest, check
CompletedOn, reserve/mark a transient "in-progress" state if needed, then call
ApplyRewardAsync for each reward and detect failures from
AddItemToPocket/ApplyRewardAsync; only set charQuest.CompletedOn =
clock.GetCurrentInstant(), call
character.SendPacketAsync(character.GenerateQuestPacket()), and publish new
QuestCompletedEvent when all rewards succeeded; if a reward fails, roll back any
partial inventory changes or avoid setting CompletedOn and return false.

In `@src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs`:
- Around line 43-44: The code in RevivalPacketHandler.cs uses SeedsRequired = 10
but sends Game18NConstString.FivePowerSeedUsed, causing a mismatch; fix by
either setting SeedsRequired to 5 (if the game should consume five seeds) or
replacing Game18NConstString.FivePowerSeedUsed with the correct ten-seed message
constant (or add one) so the message matches the SeedsRequired value; update any
related references in the revival method that use SeedsRequired and the message
constant (e.g., where Game18NConstString.FivePowerSeedUsed is referenced) to
keep behavior and UI text consistent.

In `@src/NosCore.Parser/Parsers/Generic/DatFieldMetadata.cs`:
- Around line 36-39: DatSectionSchema uses a mutable string[] for
ColumnDescriptions which breaks value-object semantics; change the property type
to ImmutableArray<string>? and add the System.Collections.Immutable using, then
update all call sites that construct DatSectionSchema (and any factory methods)
to pass ImmutableArray.Create(...) or call .ToImmutableArray() on existing
string[] instances so callers and comparisons use structural, immutable arrays
instead of reference-equality arrays.

In `@test/NosCore.GameObject.Tests/Services/QuestService/QuestServiceTests.cs`:
- Around line 220-236: Rename the two test methods and their Spec description
strings to reflect they assert objective completion (not quest CompletedOn);
update method names KillingRequiredMobsShouldCompleteHuntQuest ->
KillingRequiredMobsShouldCompleteHuntQuestObjectives (or
KillingRequiredMobsShouldCompleteObjectives) and
KillingEarlyMobsShouldNotCompleteHuntQuest ->
KillingEarlyMobsShouldNotCompleteHuntQuestObjectives (or similar), and change
the Spec("Killing the last required mob should mark quest CompletedOn") to
something like Spec("Killing the last required mob should complete objectives")
and Spec("Killing a mob before target count should not complete quest") to
Spec("Killing a mob before target count should not complete objectives") so the
Spec instantiation and the test method names (e.g., KillingTheRequiredMob,
KillingOneMob, QuestShouldBeMarkedCompleted, QuestShouldStillBeIncomplete)
clearly indicate objective-level assertions rather than CompletedOn.

---

Outside diff comments:
In `@src/NosCore.GameObject/Services/BattleService/MonsterAi.cs`:
- Around line 201-245: PickSkill is returning CastId: 0 and AttackAsync is
checking/storing cooldowns under key 0, so skills never get used or have
per-skill cooldowns; update PickSkill to set ChosenSkill.CastId to the actual
skill identifier (use sk.SkillVNum, cast to the expected long if needed) instead
of 0, and update AttackAsync to check and set cooldowns using the passed castId
(use TryGetValue(castId) and NextUsableAt[castId] = ...) rather than hardcoding
0 so each skill uses its own cast id and cooldown bucket; adjust any necessary
type casts between int and long for SkillVNum/castId.

In `@src/NosCore.GameObject/Services/InventoryService/InventoryService.cs`:
- Around line 28-37: The GetMaxSlots method in InventoryService (the return
currently written as (byte)(pocket switch ...)) must stop casting the computed
slot count to byte to avoid wraparound; change the GetMaxSlots return type from
byte to int, remove the (byte) cast so the switch expressions return an int
(e.g., configuration.Value.BackpackSize + Expensions[pocket] will become int),
and then update all callers of GetMaxSlots to accept an int (and adjust any
places that previously assumed a byte, including validation/serialization code)
so inventory slot math no longer silently wraps.

In `@src/NosCore.GameObject/Services/QuestService/QuestService.cs`:
- Around line 303-311: The daily-quest cooldown check is using
characterQuest.Value?.CompletedOn (the active quest entry which is null while
in-progress) so it always bypasses the once-per-day gate; change the check to
use the last completed quest entry for this quest (e.g., find the character's
quest record where QuestId == quest.Id and CompletedOn != null, assign it to a
variable like lastCompletedQuest and use lastCompletedQuest.CompletedOn) and
compare lastCompletedQuest.CompletedOn.Plus(Duration.FromDays(1)) to
clock.GetCurrentInstant(); keep the existing MsgiPacket/Message behavior
unchanged.

---

Nitpick comments:
In `@Directory.Build.props`:
- Line 3: The LangVersion property in Directory.Build.props is set to "latest",
which makes CI builds non-reproducible; change the <LangVersion> value from
"latest" to an explicit compiler version (for example "12.0" or
"latest-preview") so the C# language level is pinned for reproducible CI
builds—update the LangVersion element accordingly in Directory.Build.props.

In `@documentation/dat/Item.dat.md`:
- Around line 3-8: The fenced code block showing the Item.dat record template
(the block containing the tokens "FLAG", "INDEX", "NAME", "VNUM") lacks a
language specifier; update the opening triple-backtick to include a language
such as text or plaintext (e.g., ```text) so the block complies with
markdownlint MD040 and remains a plain record-layout snippet.
- Line 41: Insert a single blank line between the "## INDEX" heading and the
table that follows so the Markdown table renders correctly; locate the "##
INDEX" heading in Item.dat.md and add an empty line immediately after it (and
apply the same blank-line-before-table fix to any subsequent tables in this
file).

In `@documentation/dat/quest.dat.md`:
- Around line 3-4: The markdown file documentation/dat/quest.dat.md contains an
empty fenced code block that triggers MD040; remove the empty triple-backtick
block or replace it with a meaningful raw format example. Locate the empty
fenced code block in documentation/dat/quest.dat.md and either delete those
lines entirely or populate them with the intended raw format sample (add a short
descriptive comment or example payload) so the lint warning is resolved.
- Around line 7-26: The table contains raw generic type names like Nullable`1
and ICollection`1 (and primitive names like Int16/Byte) that render poorly;
replace them with friendly, markdown-safe type representations (e.g., use short
nullable syntax like "int?" or "short?" or explicit generics like
"ICollection<QuestObjective>", "ICollection<QuestQuestReward>", and "QuestType")
or escape/backtick them properly so backticks don't break rendering, and ensure
each DTO property row (e.g., AutoFinish, DescI18NKey, EndDialogId,
LevelMin/LevelMax, QuestId, QuestObjective, QuestQuestReward, NextQuestId,
RequiredQuestId, StartDialogId, TargetMap/TargetX/TargetY, TitleI18NKey) uses
the updated type names; also add a single blank line immediately before the
table to satisfy MD058.

In `@src/NosCore.Database/Migrations/20260421054001_AddBCardSlot.cs`:
- Around line 13-18: The migration AddBCardSlot uses AddColumn<byte> but
specifies SQL type "smallint", causing a C#/SQL type mismatch with the
BCard.Slot property; update the migration to use AddColumn<short> (and
defaultValue: (short)0) so the CLR type aligns with the SQL "smallint" mapping
in the AddBCardSlot migration and remains consistent with the BCard.Slot mapping
intent.

In `@src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs`:
- Around line 985-988: The current assignment to itemsToAdd silently falls back
to an empty list when BasicEquipments lacks the classType or the
StarterOrigin.CreateAndUpgrade entry; update the logic in PlayerBundleExtensions
(the block that uses worldConfiguration.Value.BasicEquipments, classType and
StarterOrigin.CreateAndUpgrade) to detect missing configuration and fail loudly:
if BasicEquipments.TryGetValue(classType.ToString(), out var byOrigin) or
byOrigin.TryGetValue(StarterOrigin.CreateAndUpgrade, out var pack) returns
false, either throw a descriptive exception (including classType and origin) or
log an error and return early instead of returning an empty list so
misconfigurations are immediately visible. Ensure the error path references the
same symbols (worldConfiguration.Value.BasicEquipments, classType,
StarterOrigin.CreateAndUpgrade, itemsToAdd).

In `@src/NosCore.GameObject/Messaging/Handlers/UseItem/ChangeGenderHandler.cs`:
- Around line 47-48: In ChangeGenderHandler, before calling
InventoryService.RemoveItemAmountFromInventory(1,
evt.InventoryItem.ItemInstanceId) and sending the pocket update via
evt.InventoryItem.GeneratePocketChange(...) / session.SendPacketAsync(...),
first verify evt.InventoryItem.ItemInstance.Amount >= 1 (or > 0); if the check
fails, skip the removal and the pocket-change send (or handle as an error by
logging or returning), otherwise perform the removal and then send the pocket
change — this avoids removing items when amount is zero and prevents
inconsistent state.

In `@src/NosCore.GameObject/Services/MinilandService/MinilandService.cs`:
- Around line 109-116: After calling TryInsertOrUpdateAsync to assign
minilandInfoDto in MinilandService.cs, add an explicit null-check on
minilandInfoDto before any mapping or further use (the call to
TryInsertOrUpdateAsync that constructs a new MinilandDto is the location). If
the upsert returned null, handle it immediately (throw a descriptive exception,
return a failed Result/Response, or log and abort) so downstream mapping code
does not encounter a null dereference; ensure the error message references the
OwnerId/characterId and that mapping only runs when minilandInfoDto is non-null.

In `@src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs`:
- Around line 144-166: The BroadcastReviveAsync method currently sends the
TpPacket and RevivePacket to mapInstance but only calls
session.SendPacketAsync(character.GenerateStat()), so other players won't see
the revived player's updated stats; change that call to await
mapInstance.SendPacketAsync(character.GenerateStat()).ConfigureAwait(false)
(keeping the same ConfigureAwait usage and null checks for mapInstance) so the
stat update is broadcast to everyone on the map; keep BroadcastReviveAsync,
mapInstance, session and GenerateStat referenced exactly as in the diff.

In `@src/NosCore.PacketHandlers/CharacterScreen/CharNewJobPacketHandler.cs`:
- Line 57: The TODO comment "//todo add cooldown for recreate 30days" in
CharNewJobPacketHandler indicates missing work to enforce a 30-day cooldown for
MartialArtist recreation; create a formal tracking issue in the repo (including
desired behavior, acceptance criteria, and any DB changes), then update the code
comment in CharNewJobPacketHandler (and any related method like HandleNewJob or
the class CharNewJobPacketHandler) to reference the newly created issue ID
(e.g., TODO: see issue `#123`) so the requirement is tracked, and add the issue to
the project board/milestone and changelog so implementation isn't lost.

In `@src/NosCore.PacketHandlers/Game/ReqInfoPacketHandler.cs`:
- Around line 123-124: The debug log in ReqInfoPacketHandler (the logger.Debug
call that logs "req_info 6 <mateTransportId={TransportId}> received but mate
subsystem is not wired") lacks session/character context; update this log to
include the current session and/or character identifiers (for example
session.Id, session.AccountId, or packet.SourceVNum/TargetVNum/CharacterVNum as
available) so production traces can correlate events, and pass those values into
the logger.Debug call alongside TransportId; ensure you reference the same
logger instance and keep the message concise but containing the chosen
session/character fields.

In `@src/NosCore.PacketHandlers/Quest/QTPacketHandler.cs`:
- Around line 40-48: The current GiveUp handling uses FirstOrDefault() and
compares charQuest to a new KeyValuePair to detect absence; replace that awkward
check with an idiomatic lookup: attempt to find the quest by key/value using a
clearer pattern (e.g., use session.Character.Quests.Any/First with a null check
or, if you have the quest id as a key, use session.Character.Quests.TryGetValue)
and only call TryRemove(charQuest.Key, out var questToRemove) and
questToRemove?.GenerateQstiPacket(false) when the quest actually exists; update
the QuestActionType.GiveUp branch to perform the clearer presence check and then
remove and generate the packet.

In `@src/NosCore.Parser/Parsers/Generic/FluentParserBuilder.cs`:
- Around line 148-155: The ExpectedColumns method on FluentParserBuilder<T>
currently accepts columnDescriptions without verifying its length against count;
add a validation at the start of FluentParserBuilder<T>.ExpectedColumns so that
when columnDescriptions is provided (i.e., Length > 0) its Length must equal
count, otherwise throw an ArgumentException (or ArgumentOutOfRangeException)
describing the section name, expected count and actual descriptions length; keep
the existing behavior of passing null to new DatSectionSchema when no
descriptions are provided.

In `@src/NosCore.Parser/Parsers/MapMonsterParser.cs`:
- Around line 38-42: The code calls npcMonsterdb.TryGetValue(vnum, out var
npcMonster) but never uses npcMonster; replace this with an existence check
(e.g., npcMonsterdb.ContainsKey(vnum)) or use the discard pattern (out _) to
avoid the unused variable in MapMonsterParser.cs; update the condition that
currently uses TryGetValue so it only checks presence of the key and keeps the
same control flow.
- Around line 55-56: Replace the O(n) FirstOrDefault lookup on mapMonsterdb with
an O(1) membership check by converting mapMonsterdb to a HashSet<int> (or
Dictionary<int, ...>) keyed by MapMonsterId and then using Contains; update the
code in MapMonsterParser (where mapMonsterdb is populated and where the
conditional uses mapMonsterdb.FirstOrDefault and monster.MapMonsterId) to build
the HashSet once and change the conditional to
mapMonsterIdSet.Contains(monster.MapMonsterId) (keeping the existing check
against monsters.Count(...) or optionally replace that with a HashSet lookup of
monsters' MapMonsterId as well).

In `@test/NosCore.PacketHandlers.Tests/Movement/WalkPacketHandlerTests.cs`:
- Around line 39-45: The test currently passes a plain new
Mock<Wolverine.IMessageBus>().Object to WalkPacketHandler so it cannot verify
side effects; replace that with a Mock<Wolverine.IMessageBus> instance, pass
messageBusMock.Object into the Handler constructor, exercise the handler, and
then Verify that the message bus published/sent the expected movement event (use
messageBusMock.Verify(...) for the Publish/Send method and assert the event type
and key properties like playerId/from/to or coordinates match the test packet)
to ensure the move emits the correct event.

In `@test/NosCore.Parser.Tests/DatDocumentationSnapshotTests.cs`:
- Around line 46-52: The RegenerateFor<T> helper currently always writes
generated markdown to disk (using DatDocumentationGenerator.Generate,
DocumentationPaths.For and builder.FileName) causing tests to mutate the working
tree; change it to verification mode: compute md =
DatDocumentationGenerator.Generate(builder), read the existing file at
DocumentationPaths.For(builder.FileName + ".md"), and assert the contents are
equal (failing the test if they differ). Optionally support an explicit
regeneration flag (e.g., env var or test switch) to overwrite the file only when
intentionally requested, but default behavior must be comparison/assertion so CI
fails on drift.

In `@test/NosCore.Parser.Tests/MapNpcParserTests.cs`:
- Around line 55-57: Add a non-walkable mock NPC to exercise the stationary
branch: in the test fixture where you create new() objects with properties
NpcMonsterVNum and CanWalk (the three entries with NpcMonsterVNum = 1, 2, 100),
add a fourth entry with a unique NpcMonsterVNum (e.g., 200) and CanWalk = false
so the parser's stationary logic is covered by this test.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify line endings in build-exe.cmd (CRLF expected for .cmd files).
python - <<'PY'
from pathlib import Path
p = Path("build-exe.cmd")
b = p.read_bytes()
crlf = b.count(b"\r\n")
lf = b.count(b"\n")
print(f"{p}: CRLF={crlf}, LF={lf}, CRLF_only={lf==crlf and lf>0}")
PY

Repository: NosCoreIO/NosCore

Length of output: 105

---

Use CRLF line endings for this .cmd script.

The file currently uses LF-only endings. Batch files (.cmd) on Windows should standardize to CRLF for proper compatibility.

🧰 Tools

🪛 Blinter (1.0.112)

[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 6 LF sequences found

(E018)

---

[error] 6-6: Unvalidated path traversal. Explanation: Path operations may allow directory traversal attacks with .. sequences. Recommendation: Validate paths and remove .. sequences before file operations. Context: Path contains .. which may allow directory traversal

(SEC011)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@build-exe.cmd` around lines 1 - 6, The .cmd script currently uses LF-only
line endings which breaks Windows batch execution; convert the entire file to
CRLF line endings so the commands (dotnet build -r win-x64, dotnet build -r
linux-x64, dotnet build -r linux-musl-x64, mkdir .\build\Configuration, cd
Configuration, copy *.* ..\build\Configuration\) have CRLF termination. Fix by
saving the file with CRLF in your editor or running a line-ending conversion,
and to prevent regressions add a .gitattributes rule to enforce CRLF for *.cmd
files (or set core.autocrlf appropriately) so the file stays CRLF in the repo.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if ObjectiveCount was previously computed or if 5 is a protocol constant
rg -nB5 -A5 'ObjectiveCount' --type=cs

Repository: NosCoreIO/NosCore

Length of output: 1009

---

🏁 Script executed:

# Find QuestSubPacket definition to understand what ObjectiveCount should be
rg -nA20 'class QuestSubPacket\|record QuestSubPacket' --type=cs

Repository: NosCoreIO/NosCore

Length of output: 43

---

🏁 Script executed:

# Check how objectives are populated in GenerateQuestSubPacket
rg -nB20 'ObjectiveCount = 5' --type=cs | head -50

Repository: NosCoreIO/NosCore

Length of output: 2124

---

🏁 Script executed:

# Search for any other ObjectiveCount references in tests or documentation
rg -n 'ObjectiveCount' --type=cs

Repository: NosCoreIO/NosCore

Length of output: 153

---

🏁 Script executed:

# Search for QuestSubPacket in all files
rg -n 'QuestSubPacket' --type=cs | head -20

Repository: NosCoreIO/NosCore

Length of output: 600

---

🏁 Script executed:

# Look for packet or protocol definitions
fd -e 'cs' -path '*packet*' -o -path '*Packet*' | head -20

Repository: NosCoreIO/NosCore

Length of output: 229

---

🏁 Script executed:

# Check what Quest.QuestObjectives contains
rg -nB5 -A5 'QuestObjectives' --type=cs | head -50

Repository: NosCoreIO/NosCore

Length of output: 4845

---

🏁 Script executed:

# Search all C# files for QuestSubPacket class definition
rg 'class QuestSubPacket|record QuestSubPacket' --type=cs -A10

Repository: NosCoreIO/NosCore

Length of output: 43

---

🏁 Script executed:

# Find all packet-related files
fd 'Packet' --type=f | head -30

Repository: NosCoreIO/NosCore

Length of output: 1758

---

🏁 Script executed:

# Check if QuestSubPacket might be in a different project
fd 'QuestSubPacket' --type=f

Repository: NosCoreIO/NosCore

Length of output: 43

---

🏁 Script executed:

# View the entire Quest.cs file to see imports and understand the context better
head -30 src/NosCore.GameObject/Services/QuestService/Quest.cs

Repository: NosCoreIO/NosCore

Length of output: 942

---

🏁 Script executed:

# Search for QuestSubPacket across entire repo with less filters
rg 'QuestSubPacket' -l

Repository: NosCoreIO/NosCore

Length of output: 180

---

🏁 Script executed:

# Look for any project references or packets assembly
find . -name '*.csproj' | head -10

Repository: NosCoreIO/NosCore

Length of output: 542

---

🏁 Script executed:

# Look for any Packets-related directories or files
find . -name '*Packets*' -o -name '*Quest*' | grep -v node_modules | head -20

Repository: NosCoreIO/NosCore

Length of output: 1255

---

🏁 Script executed:

# Search for tests or usage examples of QuestSubPacket
rg 'QuestSubPacket' -B2 -A2 --type=cs

Repository: NosCoreIO/NosCore

Length of output: 1827

---

🏁 Script executed:

# Check if there's any documentation or comments about ObjectiveCount
rg 'ObjectiveCount|objective.*count|count.*objective' -i --type=cs | head -20

Repository: NosCoreIO/NosCore

Length of output: 1325

---

🏁 Script executed:

# Look at the SaveServiceTests file for quest structure insights
grep -n -B5 -A5 'Updating quest objective' test/NosCore.GameObject.Tests/Services/SaveService/SaveServiceTests.cs

Repository: NosCoreIO/NosCore

Length of output: 568

---

🏁 Script executed:

# Check how many objectives quests typically have in the codebase
rg 'QuestObjectives' -B3 -A3 --type=cs | grep -A3 -B3 'Count\|\.Add\|\.List'

Repository: NosCoreIO/NosCore

Length of output: 2765

---

🏁 Script executed:

# Search for any constants or configuration related to quest objectives
rg 'const.*[0-9]|readonly.*[0-9]|= 5' --type=cs | grep -i quest

Repository: NosCoreIO/NosCore

Length of output: 3000

---

Change ObjectiveCount from hardcoded 5 to the actual objective count.

The ObjectiveCount field is hardcoded to 5 regardless of how many objectives the quest actually has. Since objectives is built dynamically in the loop above (lines 46–60) and passed to QuestObjectiveSubPackets, the objective count should reflect the actual size of the objectives list, likely (byte)objectives.Count.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Services/QuestService/Quest.cs` at line 67, The
ObjectiveCount is currently hardcoded to 5; update the assignment where
ObjectiveCount is set (in the creation of QuestObjectiveSubPackets) to use the
actual number of built objectives by replacing the literal with the dynamic
count (e.g., (byte)objectives.Count) so ObjectiveCount reflects the size of the
'objectives' list passed to QuestObjectiveSubPackets.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Quest turn-in can silently eat item rewards on a full inventory.

QPayAsync(...) marks the quest completed before rewards are applied, and ApplyRewardAsync(...) just skips item rewards when AddItemToPocket(...) returns null. That turns the quest in successfully while dropping the reward on the floor.

Also applies to: 176-209

🤖 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
143 - 173, QPayAsync currently sets charQuest.CompletedOn before applying
rewards which allows item rewards to be skipped when AddItemToPocket returns
null, effectively losing rewards; change the flow in QPayAsync so you do not
persist CompletedOn (or broadcast QuestCompletedEvent / send quest packet) until
after all rewards are successfully applied, and if any ApplyRewardAsync (or
underlying AddItemToPocket) fails to add an item, undo any temporary state and
return false (or leave CompletedOn null). Concretely: inside QPayAsync (and
similarly in the code block at 176-209) acquire the lock on charQuest, check
CompletedOn, reserve/mark a transient "in-progress" state if needed, then call
ApplyRewardAsync for each reward and detect failures from
AddItemToPocket/ApplyRewardAsync; only set charQuest.CompletedOn =
clock.GetCurrentInstant(), call
character.SendPacketAsync(character.GenerateQuestPacket()), and publish new
QuestCompletedEvent when all rewards succeeded; if a reward fails, roll back any
partial inventory changes or avoid setting CompletedOn and return false.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

How do C# records handle equality and hashing for array properties like string[]? Is array comparison structural or reference-based?

💡 Result:

C# records provide structural (value-based) equality overall, but for array properties like string[], the comparison is reference-based, not structural (content-based).

Citations:

• 1: https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/types/records
• 2: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-10.0/record-structs
• 3: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record

---

🏁 Script executed:

cd /tmp && find . -type f -name "*.cs" 2>/dev/null | head -20

Repository: NosCoreIO/NosCore

Length of output: 43

---

🏁 Script executed:

rg "DatSectionSchema" -A 3 -B 3

Repository: NosCoreIO/NosCore

Length of output: 3165

---

Replace string[]? with ImmutableArray<string>? for value-object semantics.

In C# records, array properties like string[] use reference-based equality instead of structural (content-based) equality. Two DatSectionSchema instances with identical array contents but different references will not be equal—this breaks the value-object contract expected from a record, especially for schema metadata that may be cached or compared. Use ImmutableArray<string>? to gain both structural equality and immutability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parsers/Generic/DatFieldMetadata.cs` around lines 36 - 39,
DatSectionSchema uses a mutable string[] for ColumnDescriptions which breaks
value-object semantics; change the property type to ImmutableArray<string>? and
add the System.Collections.Immutable using, then update all call sites that
construct DatSectionSchema (and any factory methods) to pass
ImmutableArray.Create(...) or call .ToImmutableArray() on existing string[]
instances so callers and comparisons use structural, immutable arrays instead of
reference-equality arrays.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

src/NosCore.GameObject/Services/QuestService/QuestService.cs (2)

204-219: Consider overflow protection for currency/XP rewards.

The gold, XP, and reputation additions cast and multiply without bounds checking:

character.Gold += (long)reward.Data * amount;

If Gold has a max cap (common in games), large rewards could overflow or exceed the cap silently.

💡 Suggested safeguard pattern

 case Data.Enumerations.Quest.QuestRewardType.Gold:
 case Data.Enumerations.Quest.QuestRewardType.BaseGoldByAmount:
 case Data.Enumerations.Quest.QuestRewardType.CapturedGold:
 case Data.Enumerations.Quest.QuestRewardType.UnknowGold:
-    character.Gold += (long)reward.Data * amount;
+    character.Gold = Math.Min(character.Gold + (long)reward.Data * amount, MaxGold);
     break;

🤖 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
204 - 219, The additions to character.Gold, character.LevelXp,
character.JobLevelXp and character.Reput in QuestService (using
(long)reward.Data * amount) need overflow/cap protection; replace the direct
arithmetic with a safe clamped add (e.g., compute delta =
checked((long)reward.Data * amount) inside a try/checked or use a saturating
multiplication helper, then add via a ClampAdd helper that ensures the final
value is between the minimum and the character-specific max (use
Character.MaxGold/MaxLevelXp/MaxJobLevelXp/MaxReput or long.MaxValue if no
domain constants exist) and persist via the existing character setters/methods
to avoid silent overflow or wraparound). Ensure negative reward.Data or amount
are handled (no subtract unless intended) and add unit tests for large
reward.Data*amount combos to validate clamping behavior.

---

178-192: Minor TOCTOU window remains between pre-check and reward application.

Although the pre-check significantly reduces the risk, a concurrent inventory change between lines 176 and 230 could still cause AddItemToPocket to fail after CompletedOn is set. The current logging at lines 233-234 is appropriate for this rare edge case.

For a more robust solution, consider reserving inventory slots atomically or applying rewards within the same lock scope (if inventory operations can be made synchronous).

🤖 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
178 - 192, The current flow sets charQuest.CompletedOn under lock but then
releases the lock before calling ApplyRewardAsync, leaving a TOCTOU where
inventory changes can cause AddItemToPocket to fail; fix by either (A)
performing reward application while still holding the same lock for charQuest
(move the foreach loop that calls ApplyRewardAsync inside the lock and make
ApplyRewardAsync synchronous or provide a guarded synchronous path), or (B)
implement an atomic reservation API on the inventory (e.g.,
InventoryService.ReserveSlots/ReserveItems) and call that under the lock before
setting charQuest.CompletedOn, then release the lock and call ApplyRewardAsync
knowing slots are reserved; update references to charQuest.CompletedOn,
ApplyRewardAsync, and AddItemToPocket (or InventoryService) accordingly.

src/NosCore.GameObject/Messaging/Handlers/UseItem/BoxEffectHandler.cs (1)

55-85: Add debug logs on silent early-return paths.

Returning silently on invalid/misconfigured reward data makes live troubleshooting harder. Consider structured debug/warn logs before the early exits at Lines 55, 61, 78, and 85.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Messaging/Handlers/UseItem/BoxEffectHandler.cs` around
lines 55 - 85, The BoxEffectHandler currently returns silently on several
early-exit conditions (pool.Count == 0, sum <= 0, chosen == null after the
probability loop, reward == null after itemProvider.Create); add structured
debug/warn log statements at each of these branches to surface context (e.g.,
log pool size, sum, roll, cumulative, details of entries or
chosen?.ItemGeneratedVNum/Amount/Probability, and the boxRare value) so
operators can diagnose misconfigured reward data; use the handler's existing
logger instance (or inject an ILogger<BoxEffectHandler> if none exists) and log
at appropriate levels (Debug for expected but helpful info, Warn/Error for
misconfiguration) immediately before each early return.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@build-exe.cmd`:
- Line 4: Change the directory creation to be idempotent by guarding it with an
existence check: replace the existing mkdir .\build\Configuration || exit /b 1
line with an if not exist check so the script only attempts to create
.\build\Configuration when it doesn't already exist (e.g. if not exist
".\build\Configuration" mkdir ".\build\Configuration"); this prevents failure on
reruns while preserving the original intent of creating the directory.

In `@src/NosCore.GameObject/Messaging/Handlers/Quest/OnEntityDiedHandler.cs`:
- Line 40: The handler is iterating evt.HitSnapshot but EntityDiedEvent has no
such property; either add a HitSnapshot property to EntityDiedEvent (e.g.,
IReadOnlyDictionary<Entity,int> HitSnapshot) and populate it where the event is
created, or change OnEntityDiedHandler to obtain contributor data from the
victim entity (use Victim to GetComponent<DamageTracker>() or similar
damage-tracking API, call its method like GetHitSnapshot/GetContributors,
null-check the component, and iterate that result instead); update references in
OnEntityDiedHandler (the foreach over evt.HitSnapshot) to the chosen source and
handle missing data gracefully.

In `@src/NosCore.Parser/Parsers/Generic/DatDocumentationGenerator.cs`:
- Around line 61-99: The generated docs emit an empty ```text``` fence when
orderedSections is empty; in DatDocumentationGenerator (the method building the
shape StringBuilder) guard the entire shape-generation and sb.Append(...) block
with a conditional check (e.g., if orderedSections.Count > 0) so you only create
the "```text" fence, iterate sections, and append the shape when there are
sections to render; locate the code that builds shape (references: shape,
orderedSections, fieldsBySection, docsBySection, firstDataColumn, sb) and wrap
that whole pass to skip emitting any text-fenced template for computed-only
parsers.

---

Nitpick comments:
In `@src/NosCore.GameObject/Messaging/Handlers/UseItem/BoxEffectHandler.cs`:
- Around line 55-85: The BoxEffectHandler currently returns silently on several
early-exit conditions (pool.Count == 0, sum <= 0, chosen == null after the
probability loop, reward == null after itemProvider.Create); add structured
debug/warn log statements at each of these branches to surface context (e.g.,
log pool size, sum, roll, cumulative, details of entries or
chosen?.ItemGeneratedVNum/Amount/Probability, and the boxRare value) so
operators can diagnose misconfigured reward data; use the handler's existing
logger instance (or inject an ILogger<BoxEffectHandler> if none exists) and log
at appropriate levels (Debug for expected but helpful info, Warn/Error for
misconfiguration) immediately before each early return.

In `@src/NosCore.GameObject/Services/QuestService/QuestService.cs`:
- Around line 204-219: The additions to character.Gold, character.LevelXp,
character.JobLevelXp and character.Reput in QuestService (using
(long)reward.Data * amount) need overflow/cap protection; replace the direct
arithmetic with a safe clamped add (e.g., compute delta =
checked((long)reward.Data * amount) inside a try/checked or use a saturating
multiplication helper, then add via a ClampAdd helper that ensures the final
value is between the minimum and the character-specific max (use
Character.MaxGold/MaxLevelXp/MaxJobLevelXp/MaxReput or long.MaxValue if no
domain constants exist) and persist via the existing character setters/methods
to avoid silent overflow or wraparound). Ensure negative reward.Data or amount
are handled (no subtract unless intended) and add unit tests for large
reward.Data*amount combos to validate clamping behavior.
- Around line 178-192: The current flow sets charQuest.CompletedOn under lock
but then releases the lock before calling ApplyRewardAsync, leaving a TOCTOU
where inventory changes can cause AddItemToPocket to fail; fix by either (A)
performing reward application while still holding the same lock for charQuest
(move the foreach loop that calls ApplyRewardAsync inside the lock and make
ApplyRewardAsync synchronous or provide a guarded synchronous path), or (B)
implement an atomic reservation API on the inventory (e.g.,
InventoryService.ReserveSlots/ReserveItems) and call that under the lock before
setting charQuest.CompletedOn, then release the lock and call ApplyRewardAsync
knowing slots are reserved; update references to charQuest.CompletedOn,
ApplyRewardAsync, and AddItemToPocket (or InventoryService) accordingly.

🪄 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: 8ae6b9d3-8715-4c6d-ba3c-3f900605b73a

📥 Commits

Reviewing files that changed from the base of the PR and between 7347ba4 and 2cccef5.

📒 Files selected for processing (13)

• build-exe.cmd
• docker-compose.yml
• documentation/dat/Card.dat.md
• documentation/dat/Item.dat.md
• documentation/dat/Skill.dat.md
• documentation/dat/monster.dat.md
• documentation/dat/quest.dat.md
• src/NosCore.GameObject/Messaging/Handlers/Battle/DeathBCardHandler.cs
• src/NosCore.GameObject/Messaging/Handlers/Quest/OnEntityDiedHandler.cs
• src/NosCore.GameObject/Messaging/Handlers/UseItem/BoxEffectHandler.cs
• src/NosCore.GameObject/Services/QuestService/QuestService.cs
• src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs
• src/NosCore.Parser/Parsers/Generic/DatDocumentationGenerator.cs

✅ Files skipped from review due to trivial changes (4)

• documentation/dat/Card.dat.md
• documentation/dat/monster.dat.md
• documentation/dat/Item.dat.md
• src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs

🚧 Files skipped from review as they are similar to previous changes (1)

• docker-compose.yml

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Make directory creation idempotent for reruns.

Line 4 will fail when .\build\Configuration already exists, which makes repeated local runs brittle.

Suggested patch

-mkdir .\build\Configuration || exit /b 1
+if not exist ".\build\Configuration" mkdir ".\build\Configuration" || exit /b 1

📝 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

	mkdir .\build\Configuration || exit /b 1
	if not exist ".\build\Configuration" mkdir ".\build\Configuration" || exit /b 1

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@build-exe.cmd` at line 4, Change the directory creation to be idempotent by
guarding it with an existence check: replace the existing mkdir
.\build\Configuration || exit /b 1 line with an if not exist check so the script
only attempts to create .\build\Configuration when it doesn't already exist
(e.g. if not exist ".\build\Configuration" mkdir ".\build\Configuration"); this
prevents failure on reruns while preserving the original intent of creating the
directory.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Build failure: HitSnapshot property does not exist on EntityDiedEvent.

The pipeline and static analysis confirm that EntityDiedEvent only defines Victim, Killer, and RevivalMode properties. This code cannot compile.

Either add a HitSnapshot property to EntityDiedEvent (e.g., IReadOnlyDictionary<Entity, int> HitSnapshot) or retrieve contributor data through an alternative means such as querying the victim's damage tracking component.

#!/bin/bash
# Verify EntityDiedEvent definition and check if HitSnapshot exists anywhere
echo "=== EntityDiedEvent definition ==="
ast-grep --pattern 'record EntityDiedEvent($$$)'

echo ""
echo "=== Any HitSnapshot property in the codebase ==="
rg -n 'HitSnapshot' --type cs

🧰 Tools

🪛 GitHub Actions: .NET

[error] 40-40: dotnet build failed (CS1061): 'EntityDiedEvent' does not contain a definition for 'HitSnapshot' and no accessible extension method 'HitSnapshot' accepting a first argument of type 'EntityDiedEvent' could be found (missing using directive or assembly reference?).

🪛 GitHub Check: build

[failure] 40-40:
Cannot infer the type of implicitly-typed discard.

---

[failure] 40-40:
Cannot infer the type of implicitly-typed deconstruction variable '_'.

---

[failure] 40-40:
Cannot infer the type of implicitly-typed deconstruction variable 'handle'.

---

[failure] 40-40:
'EntityDiedEvent' does not contain a definition for 'HitSnapshot' and no accessible extension method 'HitSnapshot' accepting a first argument of type 'EntityDiedEvent' could be found (are you missing a using directive or an assembly reference?)

---

[failure] 40-40:
Cannot infer the type of implicitly-typed discard.

---

[failure] 40-40:
Cannot infer the type of implicitly-typed deconstruction variable '_'.

---

[failure] 40-40:
Cannot infer the type of implicitly-typed deconstruction variable 'handle'.

---

[failure] 40-40:
'EntityDiedEvent' does not contain a definition for 'HitSnapshot' and no accessible extension method 'HitSnapshot' accepting a first argument of type 'EntityDiedEvent' 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/Quest/OnEntityDiedHandler.cs` at
line 40, The handler is iterating evt.HitSnapshot but EntityDiedEvent has no
such property; either add a HitSnapshot property to EntityDiedEvent (e.g.,
IReadOnlyDictionary<Entity,int> HitSnapshot) and populate it where the event is
created, or change OnEntityDiedHandler to obtain contributor data from the
victim entity (use Victim to GetComponent<DamageTracker>() or similar
damage-tracking API, call its method like GetHitSnapshot/GetContributors,
null-check the component, and iterate that result instead); update references in
OnEntityDiedHandler (the foreach over evt.HitSnapshot) to the chosen source and
handle missing data gracefully.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Skip the shape template for computed-only parsers.

When orderedSections is empty, this still writes an empty text fence. That is the root cause of the stray block already visible in the generated documentation/dat/Skill.dat.md and documentation/dat/quest.dat.md snapshots. Guard this whole pass so computed-only docs don't emit invalid/noisy Markdown.

Proposed fix

-            var shape = new StringBuilder();
-            shape.AppendLine("```text");
-            foreach (var section in orderedSections)
-            {
-                shape.Append('\t').Append(section);
-                fieldsBySection.TryGetValue(section, out var sectionFields);
-                docsBySection.TryGetValue(section, out var sectionDocs);
-
-                var maxField = sectionFields?.Max(f => f.Column ?? -1) ?? -1;
-                var maxDoc = sectionDocs?.Max(d => d.Column) ?? -1;
-                var columnCount = Math.Max(maxField, maxDoc) + 1;
-
-                for (var col = firstDataColumn; col < columnCount; col++)
-                {
-                    var match = sectionFields?.FirstOrDefault(f => f.Column == col);
-                    var doc = sectionDocs?.FirstOrDefault(d => d.Column == col);
-                    string placeholder;
-                    if (match != null)
-                    {
-                        placeholder = $"{{{match.PropertyName}}}";
-                    }
-                    else if (doc != null)
-                    {
-                        placeholder = $"{{{doc.Name}}}";
-                    }
-                    else
-                    {
-                        // Matches the existing parser comment-header convention: `0`
-                        // for columns we don't currently read but fall within the
-                        // inferred section extent.
-                        placeholder = "0";
-                    }
-                    shape.Append('\t').Append(placeholder);
-                }
-                shape.AppendLine();
-            }
-            shape.AppendLine("```");
-            sb.Append(shape);
-            sb.AppendLine();
+            if (orderedSections.Count > 0)
+            {
+                var shape = new StringBuilder();
+                shape.AppendLine("```text");
+                foreach (var section in orderedSections)
+                {
+                    shape.Append('\t').Append(section);
+                    fieldsBySection.TryGetValue(section, out var sectionFields);
+                    docsBySection.TryGetValue(section, out var sectionDocs);
+
+                    var maxField = sectionFields?.Max(f => f.Column ?? -1) ?? -1;
+                    var maxDoc = sectionDocs?.Max(d => d.Column) ?? -1;
+                    var columnCount = Math.Max(maxField, maxDoc) + 1;
+
+                    for (var col = firstDataColumn; col < columnCount; col++)
+                    {
+                        var match = sectionFields?.FirstOrDefault(f => f.Column == col);
+                        var doc = sectionDocs?.FirstOrDefault(d => d.Column == col);
+                        string placeholder;
+                        if (match != null)
+                        {
+                            placeholder = $"{{{match.PropertyName}}}";
+                        }
+                        else if (doc != null)
+                        {
+                            placeholder = $"{{{doc.Name}}}";
+                        }
+                        else
+                        {
+                            placeholder = "0";
+                        }
+
+                        shape.Append('\t').Append(placeholder);
+                    }
+
+                    shape.AppendLine();
+                }
+
+                shape.AppendLine("```");
+                sb.Append(shape);
+                sb.AppendLine();
+            }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	var shape = new StringBuilder();
	shape.AppendLine("```text");
	foreach (var section in orderedSections)
	{
	shape.Append('\t').Append(section);
	fieldsBySection.TryGetValue(section, out var sectionFields);
	docsBySection.TryGetValue(section, out var sectionDocs);
	var maxField = sectionFields?.Max(f => f.Column ?? -1) ?? -1;
	var maxDoc = sectionDocs?.Max(d => d.Column) ?? -1;
	var columnCount = Math.Max(maxField, maxDoc) + 1;
	for (var col = firstDataColumn; col < columnCount; col++)
	{
	var match = sectionFields?.FirstOrDefault(f => f.Column == col);
	var doc = sectionDocs?.FirstOrDefault(d => d.Column == col);
	string placeholder;
	if (match != null)
	{
	placeholder = $"{{{match.PropertyName}}}";
	}
	else if (doc != null)
	{
	placeholder = $"{{{doc.Name}}}";
	}
	else
	{
	// Matches the existing parser comment-header convention: `0`
	// for columns we don't currently read but fall within the
	// inferred section extent.
	placeholder = "0";
	}
	shape.Append('\t').Append(placeholder);
	}
	shape.AppendLine();
	}
	shape.AppendLine("```");
	sb.Append(shape);
	sb.AppendLine();
	if (orderedSections.Count > 0)
	{
	var shape = new StringBuilder();
	shape.AppendLine("

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parsers/Generic/DatDocumentationGenerator.cs` around lines
61 - 99, The generated docs emit an empty ```text``` fence when orderedSections
is empty; in DatDocumentationGenerator (the method building the shape
StringBuilder) guard the entire shape-generation and sb.Append(...) block with a
conditional check (e.g., if orderedSections.Count > 0) so you only create the
"```text" fence, iterate sections, and append the shape when there are sections
to render; locate the code that builds shape (references: shape,
orderedSections, fieldsBySection, docsBySection, firstDataColumn, sb) and wrap
that whole pass to skip emitting any text-fenced template for computed-only
parsers.

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (8)

src/NosCore.GameObject/Services/SaveService/SaveService.cs (1)

139-147: Optional: Consider dictionary lookup for ID reconciliation.

The nested FirstOrDefault lookup has O(n×m) complexity. While quest counts are typically small, a dictionary-based approach would be cleaner:

♻️ Suggested optimization

+               var existingByKey = existingObjectives
+                   .ToDictionary(o => (o.CharacterQuestId, o.QuestObjectiveId));
                foreach (var live in liveObjectives)
                {
-                   var match = existingObjectives.FirstOrDefault(o =>
-                       o.CharacterQuestId == live.CharacterQuestId && o.QuestObjectiveId == live.QuestObjectiveId);
-                   if (match != null)
+                   if (existingByKey.TryGetValue((live.CharacterQuestId, live.QuestObjectiveId), out var match))
                    {
                        live.Id = match.Id;
                    }
                }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Services/SaveService/SaveService.cs` around lines 139
- 147, The current reconciliation loop in SaveService.cs uses nested
FirstOrDefault calls (iterating liveObjectives and calling
existingObjectives.FirstOrDefault) causing O(n*m) work; replace this with a
dictionary keyed by a composite of CharacterQuestId and QuestObjectiveId (e.g.,
a tuple or concatenated key) built from existingObjectives, then iterate
liveObjectives and look up the match by key to assign live.Id = match.Id when
found; update references to CharacterQuestId and QuestObjectiveId and ensure
null-checks remain.

src/NosCore.Parser/Parser.cs (2)

55-65: Unused stoppingToken parameter.

The stoppingToken parameter is declared but never used in this method. Consider removing it if the factory import methods don't support cancellation, or propagate it if they do.

♻️ Proposed fix

-private async Task RunNonInteractiveAsync(string folder, CancellationToken stoppingToken)
+private async Task RunNonInteractiveAsync(string folder)

And update the call site on line 32:

-await RunNonInteractiveAsync(cli.Folder!, stoppingToken).ConfigureAwait(false);
+await RunNonInteractiveAsync(cli.Folder!).ConfigureAwait(false);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parser.cs` around lines 55 - 65, The stoppingToken
parameter of RunNonInteractiveAsync is unused; either remove stoppingToken from
RunNonInteractiveAsync (and update its callers) if cancellation isn't supported,
or propagate it by calling factory.ImportPacketsAsync(stoppingToken) and
RunFullImportAsync(stoppingToken) (and adjust those method signatures if
needed); ensure factory.SetFolder(folder) stays and update all call sites of
RunNonInteractiveAsync to match the chosen signature.

---

145-146: Placeholder prompts without implementation may confuse users.

Several prompts read user input but perform no action: PARSE_TIMESPACES (line 146), PARSE_NPCMONSTERDATA (line 161), PARSE_TELEPORTERS (line 184), PARSE_SHOPSKILLS (line 191), and PARSE_RECIPES (line 194).

Consider either:

1. Removing these prompts until the features are implemented
2. Adding a "not yet implemented" message after the prompt
3. Adding a comment explaining why they're retained

♻️ Example: Show "not implemented" feedback

 logger.Information($"{logLanguage[LogLanguageKey.PARSE_TIMESPACES]} [Y/n]");
-Console.ReadKey(true);
+key = Console.ReadKey(true);
+if (key.KeyChar != 'n') logger.Warning("Timespaces import not yet implemented.");

Also applies to: 160-161, 183-184, 190-191, 193-194

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parser.cs` around lines 145 - 146, The parser currently
shows prompt lines like
logger.Information($"{logLanguage[LogLanguageKey.PARSE_TIMESPACES]} [Y/n]");
followed by Console.ReadKey(true) (and similar keys for
LogLanguageKey.PARSE_NPCMONSTERDATA, PARSE_TELEPORTERS, PARSE_SHOPSKILLS,
PARSE_RECIPES) but takes no action, which can confuse users; update each prompt
handling (references: LogLanguageKey.PARSE_TIMESPACES,
LogLanguageKey.PARSE_NPCMONSTERDATA, LogLanguageKey.PARSE_TELEPORTERS,
LogLanguageKey.PARSE_SHOPSKILLS, LogLanguageKey.PARSE_RECIPES and the
surrounding logger.Information/Console.ReadKey calls) to either remove the
prompt, or immediately follow the prompt with a clear "Not yet implemented"
feedback message (e.g., log an informational message and/or write to console) or
add an inline comment explaining why the prompt is retained so callers see
explicit behavior instead of a dead input; make the same change consistently for
all listed keys.

src/NosCore.Parser/Parsers/CardParser.cs (1)

66-66: Consider reducing visibility to private.

SkillParser.AddBCards is private (per context snippet). Since this method is only called internally via the builder delegate on line 52, it could follow the same pattern for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parsers/CardParser.cs` at line 66, The AddBCards method is
declared public but is only used internally via the builder delegate in
SkillParser; change its visibility to private to match the internal-only usage
and the pattern used by other builder methods — update the declaration of
AddBCards (method name AddBCards in class SkillParser) from public to private
and ensure any references (the builder delegate that calls AddBCards) remain
unchanged.

test/NosCore.Parser.Tests/DatDocumentationSnapshotTests.cs (1)

52-60: Consider adding a comment about snapshot generation workflow.

The test behavior when snapshots don't exist (comparing against empty string) might be surprising for new contributors. A brief comment explaining the UPDATE_SNAPSHOTS=1 workflow would help.

📝 Example documentation comment

         private static void RegenerateFor<T>(FluentParserBuilder<T> builder) where T : new()
         {
             var md = DatDocumentationGenerator.Generate(builder);
             var path = DocumentationPaths.For(builder.FileName + ".md");
             Directory.CreateDirectory(Path.GetDirectoryName(path)!);

+            // To regenerate snapshots: UPDATE_SNAPSHOTS=1 dotnet test --filter DatDocumentationSnapshotTests
             if (Environment.GetEnvironmentVariable("UPDATE_SNAPSHOTS") == "1")
             {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/NosCore.Parser.Tests/DatDocumentationSnapshotTests.cs` around lines 52 -
60, Add a short inline comment above the
Environment.GetEnvironmentVariable("UPDATE_SNAPSHOTS") check in
DatDocumentationSnapshotTests explaining the snapshot regeneration workflow:
describe that setting UPDATE_SNAPSHOTS=1 causes File.WriteAllText(path, md) to
overwrite or create the .md snapshot for builder.FileName, while the default
behavior compares the generated md to an existing file (or empty string if
missing) and will fail the Assert.AreEqual(existing, md) if they differ; mention
how to regenerate snapshots locally using that env var for new contributors.

src/NosCore.GameObject/Services/BattleService/MonsterAi.cs (1)

164-224: Consider performance implications of O(n) enemy scans per tick.

DetectNearbyEnemy iterates through all monsters, players, and NPCs on the map every AI tick (400ms). For maps with many entities, this could become a performance bottleneck.

For v1 this is acceptable, but consider spatial partitioning (grid-based or quadtree) if scaling becomes an issue.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Services/BattleService/MonsterAi.cs` around lines 164
- 224, DetectNearbyEnemy currently does O(n) scans over map.Monsters,
sessionRegistry.GetClientSessionsByMapInstance(...)/player entities, and
map.Npcs each AI tick using distanceCalculator, which will not scale for large
maps; to fix, replace the full-list scans with a spatial query: introduce or use
a spatial index (grid/quadtree) to store entities per MapInstance and query only
entities within entity.NpcMonster.NoticeRange (or at least nearby buckets)
inside DetectNearbyEnemy, updating the index on entity movement/spawn/despawn
and using the index instead of iterating map.Monsters/map.Npcs and
sessionRegistry lookups; ensure the spatial index API provides a radius or
bounding-box query and keep the same distanceCalculator-based tie-breaker and
return type.

src/NosCore.GameObject/Services/BattleService/RewardService.cs (1)

135-153: Consider extracting TryFindCharacter to avoid duplication.

This helper is duplicated in OnEntityDiedHandler (see context snippet 1, lines 56-71). Both perform identical logic: check if handle is alive in the ECS world, get the identity component, verify it's a player, and wrap in PlayerComponentBundle.

♻️ Suggested approach

Extract to a shared static helper in a common location (e.g., EcsExtensions or a new EntityResolver utility class) that both RewardService and OnEntityDiedHandler can call.

// In a shared utility class
public static bool TryFindCharacter(IAliveEntity contextEntity, Entity handle, out ICharacterEntity character)
{
    // ... shared implementation
}

🤖 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
135 - 153, The TryFindCharacter implementation is duplicated between
RewardService and OnEntityDiedHandler; extract it into a single shared static
helper (e.g., a public static method named TryFindCharacter in a new
EcsExtensions or EntityResolver utility class) and replace the local
implementations in RewardService.TryFindCharacter and the OnEntityDiedHandler
copy to call that shared method; ensure the method signature remains bool
TryFindCharacter(IAliveEntity victim, Entity handle, out ICharacterEntity
character), the utility is in a namespace both files can access, and its
accessibility is public/internal as needed so both callers compile.

src/NosCore.PacketHandlers/Game/GameStartPacketHandler.cs (1)

76-79: Fire-and-forget may silently swallow exceptions.

The discarded task (_ = questProvider.RunScriptAsync(...)) means any exception from RunScriptAsync will be unobserved. Consider awaiting or adding exception logging:

🔧 Option A: Await the call

 if (session.Character.CurrentScriptId == null)
 {
-    _ = questProvider.RunScriptAsync(session.Character);
+    await questProvider.RunScriptAsync(session.Character);
 }

🔧 Option B: Add exception logging if fire-and-forget is intentional

 if (session.Character.CurrentScriptId == null)
 {
-    _ = questProvider.RunScriptAsync(session.Character);
+    _ = questProvider.RunScriptAsync(session.Character).ContinueWith(
+        t => logger.Error(t.Exception, "Script initialization failed"),
+        TaskContinuationOptions.OnlyOnFaulted);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.PacketHandlers/Game/GameStartPacketHandler.cs` around lines 76 -
79, The fire-and-forget call to questProvider.RunScriptAsync(session.Character)
can swallow exceptions because the returned Task is discarded; either await the
call in GameStartPacketHandler (so await
questProvider.RunScriptAsync(session.Character) and make the caller async) or,
if you intentionally want fire-and-forget, attach a continuation to the Task to
catch and log exceptions (e.g., call .ContinueWith or using Task.Run with
try/catch) so any exception from RunScriptAsync is logged against
session.Character.CurrentScriptId and/or the session ID; update the
GameStartPacketHandler where questProvider.RunScriptAsync is invoked
accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/NosCore.GameObject/Services/MinilandService/MinilandService.cs`:
- Around line 109-122: The current upsert in MinilandService (where MinilandDto
is created with a new MinilandId and TryInsertOrUpdateAsync is called) allows
duplicate miniland rows for the same OwnerId under concurrent logins; change the
concurrency model by either adding a unique DB constraint on OwnerId in the EF
configuration (use HasIndex(e => e.OwnerId).IsUnique()) or modify the upsert to
use OwnerId as the natural key (ensure TryInsertOrUpdateAsync / DAO upsert logic
keys on OwnerId instead of MinilandId and performs database-level conflict
resolution so concurrent inserts merge rather than create duplicates); update
any fallback reads like FirstOrDefaultAsync(s => s.OwnerId == characterId) to
expect a single row once the unique constraint/upsert change is applied.

In `@src/NosCore.GameObject/Services/QuestService/QuestService.cs`:
- Around line 374-389: The completion check in OnMonsterKilledAsync is wrong:
after calling handler.OnMonsterKilledAsync the code checks if (quest.CompletedOn
!= null) which will always be false; change this to verify objectives are
satisfied (e.g., if (quest.ObjectiveProgress >= quest.Quest.ObjectiveAmount) )
and then call CompleteQuestAsync(character, quest); locate the method
OnMonsterKilledAsync in QuestService and update the post-handler condition to
use quest.ObjectiveProgress and quest.Quest.ObjectiveAmount (or the quest's
existing IsComplete-like property) so CompleteQuestAsync is invoked when
objectives are met.

In `@src/NosCore.GameObject/Services/SaveService/SaveService.cs`:
- Around line 156-163: The early return when
characterQuestObjectiveDao.TryInsertOrUpdateAsync(liveObjectives) fails prevents
subsequent respawn persistence; change the flow in SaveService.Save (or the
containing method) so respawn persistence (respawnDao.TryInsertOrUpdateAsync or
whichever method persists liveRespawns) is attempted regardless of
objectivesSaved: log the InvalidOperationException and continue instead of
returning, attempt respawn save, aggregate success/failure for final handling,
and ensure both errors are logged (use the same logger and contextual
characterId) so transient objective failures don't skip respawn persistence.

In `@src/NosCore.Parser/Parser.cs`:
- Around line 67-80: RunInteractiveAsync reads folder from Console.ReadLine()
into variable folder but immediately calls factory.SetFolder(folder!) which uses
the null-forgiving operator and can throw if folder is null; validate folder
before calling SetFolder (e.g., check if folder is null or whitespace after
Console.ReadLine(), re-prompt or log and return/cancel) and remove the
null-forgiving operator so factory.SetFolder is only called with a non-null,
validated string; reference RunInteractiveAsync, the folder local variable,
Console.ReadLine(), and factory.SetFolder when making the change.

---

Nitpick comments:
In `@src/NosCore.GameObject/Services/BattleService/MonsterAi.cs`:
- Around line 164-224: DetectNearbyEnemy currently does O(n) scans over
map.Monsters, sessionRegistry.GetClientSessionsByMapInstance(...)/player
entities, and map.Npcs each AI tick using distanceCalculator, which will not
scale for large maps; to fix, replace the full-list scans with a spatial query:
introduce or use a spatial index (grid/quadtree) to store entities per
MapInstance and query only entities within entity.NpcMonster.NoticeRange (or at
least nearby buckets) inside DetectNearbyEnemy, updating the index on entity
movement/spawn/despawn and using the index instead of iterating
map.Monsters/map.Npcs and sessionRegistry lookups; ensure the spatial index API
provides a radius or bounding-box query and keep the same
distanceCalculator-based tie-breaker and return type.

In `@src/NosCore.GameObject/Services/BattleService/RewardService.cs`:
- Around line 135-153: The TryFindCharacter implementation is duplicated between
RewardService and OnEntityDiedHandler; extract it into a single shared static
helper (e.g., a public static method named TryFindCharacter in a new
EcsExtensions or EntityResolver utility class) and replace the local
implementations in RewardService.TryFindCharacter and the OnEntityDiedHandler
copy to call that shared method; ensure the method signature remains bool
TryFindCharacter(IAliveEntity victim, Entity handle, out ICharacterEntity
character), the utility is in a namespace both files can access, and its
accessibility is public/internal as needed so both callers compile.

In `@src/NosCore.GameObject/Services/SaveService/SaveService.cs`:
- Around line 139-147: The current reconciliation loop in SaveService.cs uses
nested FirstOrDefault calls (iterating liveObjectives and calling
existingObjectives.FirstOrDefault) causing O(n*m) work; replace this with a
dictionary keyed by a composite of CharacterQuestId and QuestObjectiveId (e.g.,
a tuple or concatenated key) built from existingObjectives, then iterate
liveObjectives and look up the match by key to assign live.Id = match.Id when
found; update references to CharacterQuestId and QuestObjectiveId and ensure
null-checks remain.

In `@src/NosCore.PacketHandlers/Game/GameStartPacketHandler.cs`:
- Around line 76-79: The fire-and-forget call to
questProvider.RunScriptAsync(session.Character) can swallow exceptions because
the returned Task is discarded; either await the call in GameStartPacketHandler
(so await questProvider.RunScriptAsync(session.Character) and make the caller
async) or, if you intentionally want fire-and-forget, attach a continuation to
the Task to catch and log exceptions (e.g., call .ContinueWith or using Task.Run
with try/catch) so any exception from RunScriptAsync is logged against
session.Character.CurrentScriptId and/or the session ID; update the
GameStartPacketHandler where questProvider.RunScriptAsync is invoked
accordingly.

In `@src/NosCore.Parser/Parser.cs`:
- Around line 55-65: The stoppingToken parameter of RunNonInteractiveAsync is
unused; either remove stoppingToken from RunNonInteractiveAsync (and update its
callers) if cancellation isn't supported, or propagate it by calling
factory.ImportPacketsAsync(stoppingToken) and RunFullImportAsync(stoppingToken)
(and adjust those method signatures if needed); ensure factory.SetFolder(folder)
stays and update all call sites of RunNonInteractiveAsync to match the chosen
signature.
- Around line 145-146: The parser currently shows prompt lines like
logger.Information($"{logLanguage[LogLanguageKey.PARSE_TIMESPACES]} [Y/n]");
followed by Console.ReadKey(true) (and similar keys for
LogLanguageKey.PARSE_NPCMONSTERDATA, PARSE_TELEPORTERS, PARSE_SHOPSKILLS,
PARSE_RECIPES) but takes no action, which can confuse users; update each prompt
handling (references: LogLanguageKey.PARSE_TIMESPACES,
LogLanguageKey.PARSE_NPCMONSTERDATA, LogLanguageKey.PARSE_TELEPORTERS,
LogLanguageKey.PARSE_SHOPSKILLS, LogLanguageKey.PARSE_RECIPES and the
surrounding logger.Information/Console.ReadKey calls) to either remove the
prompt, or immediately follow the prompt with a clear "Not yet implemented"
feedback message (e.g., log an informational message and/or write to console) or
add an inline comment explaining why the prompt is retained so callers see
explicit behavior instead of a dead input; make the same change consistently for
all listed keys.

In `@src/NosCore.Parser/Parsers/CardParser.cs`:
- Line 66: The AddBCards method is declared public but is only used internally
via the builder delegate in SkillParser; change its visibility to private to
match the internal-only usage and the pattern used by other builder methods —
update the declaration of AddBCards (method name AddBCards in class SkillParser)
from public to private and ensure any references (the builder delegate that
calls AddBCards) remain unchanged.

In `@test/NosCore.Parser.Tests/DatDocumentationSnapshotTests.cs`:
- Around line 52-60: Add a short inline comment above the
Environment.GetEnvironmentVariable("UPDATE_SNAPSHOTS") check in
DatDocumentationSnapshotTests explaining the snapshot regeneration workflow:
describe that setting UPDATE_SNAPSHOTS=1 causes File.WriteAllText(path, md) to
overwrite or create the .md snapshot for builder.FileName, while the default
behavior compares the generated md to an existing file (or empty string if
missing) and will fail the Assert.AreEqual(existing, md) if they differ; mention
how to regenerate snapshots locally using that env var for new contributors.

🪄 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: f1458ade-0577-4d0d-b1e5-4fa473eead28

📥 Commits

Reviewing files that changed from the base of the PR and between 2cccef5 and b841631.

📒 Files selected for processing (24)

• src/NosCore.GameObject/Messaging/Events/EntityDiedEvent.cs
• src/NosCore.GameObject/Messaging/Handlers/Battle/RewardDistributionHandler.cs
• src/NosCore.GameObject/Messaging/Handlers/UseItem/PetBasketHandler.cs
• src/NosCore.GameObject/Messaging/Handlers/UseItem/SealedTarotCardHandler.cs
• src/NosCore.GameObject/Networking/ClientSession/ClientSession.cs
• src/NosCore.GameObject/Services/BattleService/BattleService.cs
• src/NosCore.GameObject/Services/BattleService/IRewardService.cs
• src/NosCore.GameObject/Services/BattleService/MonsterAi.cs
• src/NosCore.GameObject/Services/BattleService/RewardService.cs
• src/NosCore.GameObject/Services/MapChangeService/MapChangeService.cs
• src/NosCore.GameObject/Services/MapInstanceGenerationService/MapInstance.cs
• src/NosCore.GameObject/Services/MinilandService/MinilandService.cs
• src/NosCore.GameObject/Services/QuestService/QuestService.cs
• src/NosCore.GameObject/Services/SaveService/SaveService.cs
• src/NosCore.PacketHandlers/Game/GameStartPacketHandler.cs
• src/NosCore.PacketHandlers/Quest/QTPacketHandler.cs
• src/NosCore.Parser/Parser.cs
• src/NosCore.Parser/Parsers/CardParser.cs
• src/NosCore.Parser/Parsers/QuestParser.cs
• src/NosCore.Parser/Parsers/SkillParser.cs
• test/NosCore.GameObject.Tests/Messaging/Handlers/Battle/RewardDistributionHandlerTests.cs
• test/NosCore.GameObject.Tests/Services/BattleService/MonsterAiTests.cs
• test/NosCore.Parser.Tests/DatDocumentationSnapshotTests.cs
• tools/NosCore.EcsGenerator/ComponentBundleGenerator.cs

🚧 Files skipped from review as they are similar to previous changes (6)

• src/NosCore.GameObject/Services/MapInstanceGenerationService/MapInstance.cs
• src/NosCore.Parser/Parsers/QuestParser.cs
• src/NosCore.Parser/Parsers/SkillParser.cs
• src/NosCore.PacketHandlers/Quest/QTPacketHandler.cs
• src/NosCore.GameObject/Messaging/Handlers/UseItem/SealedTarotCardHandler.cs
• src/NosCore.GameObject/Messaging/Handlers/UseItem/PetBasketHandler.cs

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Locate MinilandDto shape =="
rg -n -C3 --type=cs '\bclass\s+MinilandDto\b|OwnerId|MinilandId'

echo
echo "== Check EF configurations/migrations for unique constraint/index on OwnerId =="
rg -n -C3 --type=cs 'HasIndex\s*\(.*OwnerId|IsUnique\s*\(|CreateIndex\s*\(.*OwnerId|Miniland.*OwnerId'

echo
echo "== Inspect TryInsertOrUpdateAsync declarations/implementations =="
rg -n -C3 --type=cs '\bTryInsertOrUpdateAsync\s*\('

Repository: NosCoreIO/NosCore

Length of output: 50374

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search for TryInsertOrUpdateAsync implementation =="
rg -n 'TryInsertOrUpdateAsync' --type=cs -A 10 | head -80

echo
echo "== Check for unique constraint on OwnerId in entity config =="
fd -e cs -x grep -l 'Miniland' {} \; | grep -E '(Config|Entity|Miniland)' | head -10

echo
echo "== Examine Miniland entity definition =="
fd -e cs -path '*/Entities/*' -name '*Miniland*' | head -5

Repository: NosCoreIO/NosCore

Length of output: 7116

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find TryInsertOrUpdateAsync definition (not calls) =="
rg -n 'async.*TryInsertOrUpdateAsync|TryInsertOrUpdateAsync.*\{' --type=cs -B 2 -A 15 | head -100

echo
echo "== Check IDao interface =="
fd -e cs -name '*IDao*' -o -name '*Dao*' | grep -i interface | head -5

echo
echo "== Look for generic DAO/repository base class =="
fd -e cs -path '*/Dao/*' -o -path '*/Repository/*' | grep -E '(Base|Generic|Abstract)' | head -10

Repository: NosCoreIO/NosCore

Length of output: 2395

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search for IDao interface definition =="
fd -e cs -name '*Dao.cs' | head -10

echo
echo "== Find the actual TryInsertOrUpdateAsync method definition =="
rg -A 20 'Task.*TryInsertOrUpdateAsync' --type=cs | grep -E '(public|async|Task|TryInsertOrUpdateAsync|\{|return)' | head -50

echo
echo "== Look in Data project for DAO/repository base =="
fd -e cs -path '*/Data/*' -name '*.cs' | grep -i dao | head -5

Repository: NosCoreIO/NosCore

Length of output: 351

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find DAO-related files =="
find src -name '*Dao.cs' -o -name '*Dao*.cs' | head -20

echo
echo "== Search for TryInsertOrUpdateAsync with proper context =="
rg 'TryInsertOrUpdateAsync' --type=cs -B 3 -A 15 -m 1

echo
echo "== Check src/NosCore.Data for data access implementations =="
ls -la src/NosCore.Data/ | head -20

Repository: NosCoreIO/NosCore

Length of output: 50374

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== List NosCore.Data directory structure =="
ls -la src/NosCore.Data/ 2>/dev/null | head -20

echo
echo "== Search for IDao interface =="
find src/NosCore.Data -name '*.cs' -type f | head -20

echo
echo "== Look for the TryInsertOrUpdateAsync method definition =="
rg 'public.*async.*Task.*TryInsertOrUpdateAsync' --type=cs -A 5 | head -50

Repository: NosCoreIO/NosCore

Length of output: 1834

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search for TryInsertOrUpdateAsync definition in entire repo =="
rg 'TryInsertOrUpdateAsync\s*\(' --type=cs -B 5 | grep -E '(public|private|protected|async|Task|TryInsertOrUpdateAsync)' | head -80

echo
echo "== Check NosCore.Database for DAO files =="
find src/NosCore.Database -name '*.cs' | grep -i dao | head -10

echo
echo "== Look for GenericDao or IDao in NosCore.Database =="
find src/NosCore.Database -name '*Dao*' -o -name '*Repository*' | head -10

Repository: NosCoreIO/NosCore

Length of output: 10416

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search for IDao interface definition =="
rg 'interface.*IDao|TryInsertOrUpdateAsync' --type=cs -B 2 -A 10 | grep -A 10 'interface' | head -100

echo
echo "== Check NosCore.Shared or NosCore.Core for base classes =="
find src -type d -name '*Shared' -o -type d -name '*Core' | head -10

echo
echo "== Look for DAO implementation/abstraction =="
find src -name '*.cs' -exec grep -l 'class.*Dao\|abstract.*class' {} \; | head -15

Repository: NosCoreIO/NosCore

Length of output: 103

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Examine MinilandService.cs completely to see DAO usage =="
cat -n src/NosCore.GameObject/Services/MinilandService/MinilandService.cs | head -150

echo
echo "== Check the file for TryInsertOrUpdateAsync and what object calls it =="
grep -n 'minilandDao\|TryInsertOrUpdateAsync' src/NosCore.GameObject/Services/MinilandService/MinilandService.cs | head -20

Repository: NosCoreIO/NosCore

Length of output: 8023

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find IDao interface in NosCore.Dao =="
find src -name '*.cs' -path '*/Dao/*' | head -20

echo
echo "== Search for IDao<T, TKey> interface definition =="
rg 'interface IDao' --type=cs -A 20 | head -80

echo
echo "== Look in NosCore.Dao for the interface =="
ls -la src/NosCore.Dao 2>/dev/null || echo "Not found in standard location"
</parameter>

Repository: NosCoreIO/NosCore

Length of output: 154

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find all projects in src =="
ls -d src/NosCore.* | head -20

echo
echo "== Search for where TryInsertOrUpdateAsync is defined (method signature) =="
rg -A 15 'async.*TryInsertOrUpdateAsync.*\{' --type=cs | head -150

Repository: NosCoreIO/NosCore

Length of output: 384

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search in NosCore.Core for IDao =="
find src/NosCore.Core -name '*.cs' | head -20

echo
echo "== Search entire src for 'interface IDao' =="
rg 'interface\s+IDao' --type=cs -A 30

echo
echo "== Search for generic TryInsertOrUpdateAsync method definition =="
rg -U 'TryInsertOrUpdateAsync\s*\([^)]*\)\s*{' --type=cs -A 20 | head -100

Repository: NosCoreIO/NosCore

Length of output: 937

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check NosCore.Database for entity/DAO setup =="
ls -la src/NosCore.Database/ 2>/dev/null | head -30

echo
echo "== Search the entire codebase for where TryInsertOrUpdateAsync is defined =="
git grep -n 'public.*Task.*TryInsertOrUpdateAsync' | head -20

echo
echo "== Look at the actual repository/DAO pattern - check imports in MinilandService =="
head -30 src/NosCore.GameObject/Services/MinilandService/MinilandService.cs | grep -E 'using|IDao'

Repository: NosCoreIO/NosCore

Length of output: 710

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check package references in NosCore projects =="
grep -r 'TryInsertOrUpdateAsync' --include='*.csproj' src/ || echo "Not in csproj"

echo
echo "== Look for NuGet package or reference =="
rg 'TryInsertOrUpdateAsync' --type=cs --no-heading | grep -i 'namespace\|package' | head -10

echo
echo "== Check if it comes from EntityFramework or custom extension =="
rg 'using.*Dao|namespace.*Dao' --type=cs | head -20

echo
echo "== Look for the method via git log or actual implementation =="
git log --all -S 'TryInsertOrUpdateAsync' --oneline | head -5

echo
echo "== Direct search for method implementation with context =="
rg 'TryInsertOrUpdateAsync' --type=cs -B 10 -A 5 | grep -E 'public|async|Task|where T|Add|SaveChanges' | head -40

Repository: NosCoreIO/NosCore

Length of output: 168

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check all cs files for IDao interface =="
rg 'IDao' --type=cs -l | head -10

echo
echo "== Get context on IDao from MinilandService =="
rg 'IDao\<' --type=cs -B 2 -A 5 | head -60

echo
echo "== Check the .csproj files for package dependencies =="
find src -name '*.csproj' -exec cat {} \; | grep -i 'packagereference\|project.*reference' | grep -i 'dao\|efcore' | head -20

Repository: NosCoreIO/NosCore

Length of output: 821

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check all cs files for IDao interface =="
rg 'IDao' --type=cs -A 3 | head -80

echo
echo "== Check if IDao comes from a nuget package in dependencies =="
cat src/NosCore.GameObject/NosCore.GameObject.csproj | grep -i 'packagereference\|project' | head -30

echo
echo "== Look at one test file to see IDao usage =="
head -100 test/NosCore.Tests.Shared/TestHelpers.cs | tail -50

Repository: NosCoreIO/NosCore

Length of output: 11205

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Look for NosCore.Dao namespace/project =="
find . -name '*Dao*' -type f | grep -v '.git\|node_modules\|bin\|obj' | head -20

echo
echo "== Search for IDao in all namespaces =="
rg 'namespace.*Dao|using.*Dao.Interfaces' --type=cs | head -20

echo
echo "== Check the Nuget packages =="
cat src/NosCore.GameObject/NosCore.GameObject.csproj | grep -A 50 'PackageReference' | grep -i 'noscore'

Repository: NosCoreIO/NosCore

Length of output: 105

---

Add unique constraint on Miniland.OwnerId or use database-level conflict resolution.

The code has a race condition: concurrent logins for the same character can both pass the null check at line 106 and create separate miniland rows with different MinilandIds. The database schema confirms OwnerId has only a non-unique index, not a unique constraint. The TryInsertOrUpdateAsync call at line 109 generates a new MinilandId each time, so without a unique constraint on OwnerId, both inserts succeed despite representing the same character. The fallback read at line 119 masks this by fetching whichever row exists first, but duplicate rows remain in the database.

Fix: Either add HasIndex(e => e.OwnerId).IsUnique() in the EF configuration, or rework the upsert to key on OwnerId instead of MinilandId, ensuring only one miniland row per character even under concurrent initialization.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Services/MinilandService/MinilandService.cs` around
lines 109 - 122, The current upsert in MinilandService (where MinilandDto is
created with a new MinilandId and TryInsertOrUpdateAsync is called) allows
duplicate miniland rows for the same OwnerId under concurrent logins; change the
concurrency model by either adding a unique DB constraint on OwnerId in the EF
configuration (use HasIndex(e => e.OwnerId).IsUnique()) or modify the upsert to
use OwnerId as the natural key (ensure TryInsertOrUpdateAsync / DAO upsert logic
keys on OwnerId instead of MinilandId and performs database-level conflict
resolution so concurrent inserts merge rather than create duplicates); update
any fallback reads like FirstOrDefaultAsync(s => s.OwnerId == characterId) to
expect a single row once the unique constraint/upsert change is applied.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how handlers implement OnMonsterKilledAsync and whether they set CompletedOn
rg -n --type=cs "OnMonsterKilledAsync" -A 20 | head -100

Repository: NosCoreIO/NosCore

Length of output: 11053

---

🏁 Script executed:

# Check all quest type handlers to see if any set CompletedOn
rg -n --type=cs "class.*Handler.*Quest\|OnMonsterKilledAsync\|CompletedOn\s*=" src/NosCore.GameObject/Services/QuestService/Handlers/ -A 3 | head -150

Repository: NosCoreIO/NosCore

Length of output: 43

---

🏁 Script executed:

# Get the full CompleteQuestAsync method to understand the firstCompletion logic
sed -n '398,425p' src/NosCore.GameObject/Services/QuestService/QuestService.cs

Repository: NosCoreIO/NosCore

Length of output: 873

---

🏁 Script executed:

# Check if handlers might set CompletedOn indirectly or if other handlers exist
find src/NosCore.GameObject/Services/QuestService/Handlers -name "*.cs" -type f

Repository: NosCoreIO/NosCore

Length of output: 368

---

🏁 Script executed:

# Check GoToQuestHandler and HuntQuestHandler implementation
cat -n src/NosCore.GameObject/Services/QuestService/Handlers/GoToQuestHandler.cs

Repository: NosCoreIO/NosCore

Length of output: 1429

---

🏁 Script executed:

cat -n src/NosCore.GameObject/Services/QuestService/Handlers/HuntQuestHandler.cs

Repository: NosCoreIO/NosCore

Length of output: 644

---

🏁 Script executed:

# Check NumberOfKillQuestHandler
cat -n src/NosCore.GameObject/Services/QuestService/Handlers/NumberOfKillQuestHandler.cs

Repository: NosCoreIO/NosCore

Length of output: 660

---

🏁 Script executed:

# Verify AreObjectivesComplete() exists on CharacterQuest
rg -n --type=cs "AreObjectivesComplete" | head -20

Repository: NosCoreIO/NosCore

Length of output: 695

---

Inverted completion check prevents quest completion.

Line 384 has a logic error. The condition if (quest.CompletedOn != null) will always be false because:

1. The loop filters for CompletedOn is null only (line 376)
2. Quest handlers like KillQuestHandlerBase only update ObjectiveProgress, never set CompletedOn
3. Therefore, CompleteQuestAsync is never called, and quests are never marked complete even when objectives are satisfied

The correct condition should check if objectives are complete:

🐛 Suggested fix

 await handler.OnMonsterKilledAsync(character, mob, quest);
-if (quest.CompletedOn != null)
+if (quest.AreObjectivesComplete())
 {
     await CompleteQuestAsync(character, quest);
 }

📝 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

	public async Task OnMonsterKilledAsync(ICharacterEntity character, NpcMonsterDto mob)
	{
	var tasks = character.Quests.Values
	.Where(q => q.CompletedOn is null)
	.Select(q =>
	foreach (var quest in character.Quests.Values.Where(q => q.CompletedOn is null).ToList())
	{
	var handler = questTypeHandlers.FirstOrDefault(h => h.QuestType == quest.Quest.QuestType);
	if (handler == null)
	{
	var handler = questTypeHandlers.FirstOrDefault(h => h.QuestType == q.Quest.QuestType);
	return handler?.OnMonsterKilledAsync(character, mob, q) ?? Task.CompletedTask;
	});
	return Task.WhenAll(tasks);
	continue;
	}
	await handler.OnMonsterKilledAsync(character, mob, quest);
	if (quest.CompletedOn != null)
	{
	await CompleteQuestAsync(character, quest);
	}
	}
	}
	public async Task OnMonsterKilledAsync(ICharacterEntity character, NpcMonsterDto mob)
	{
	foreach (var quest in character.Quests.Values.Where(q => q.CompletedOn is null).ToList())
	{
	var handler = questTypeHandlers.FirstOrDefault(h => h.QuestType == quest.Quest.QuestType);
	if (handler == null)
	{
	continue;
	}
	await handler.OnMonsterKilledAsync(character, mob, quest);
	if (quest.AreObjectivesComplete())
	{
	await CompleteQuestAsync(character, quest);
	}
	}
	}

🤖 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
374 - 389, The completion check in OnMonsterKilledAsync is wrong: after calling
handler.OnMonsterKilledAsync the code checks if (quest.CompletedOn != null)
which will always be false; change this to verify objectives are satisfied
(e.g., if (quest.ObjectiveProgress >= quest.Quest.ObjectiveAmount) ) and then
call CompleteQuestAsync(character, quest); locate the method
OnMonsterKilledAsync in QuestService and update the post-handler condition to
use quest.ObjectiveProgress and quest.Quest.ObjectiveAmount (or the quest's
existing IsComplete-like property) so CompleteQuestAsync is invoked when
objectives are met.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Early return prevents respawn persistence.

When the objective upsert fails (line 162), the method returns before saving respawns (line 165). This means a transient objective persistence issue would also lose any respawn data changes, which may not be the intended behavior.

Consider whether respawn persistence should be attempted independently of quest objective success, or if this coupling is acceptable for your use case.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.GameObject/Services/SaveService/SaveService.cs` around lines 156
- 163, The early return when
characterQuestObjectiveDao.TryInsertOrUpdateAsync(liveObjectives) fails prevents
subsequent respawn persistence; change the flow in SaveService.Save (or the
containing method) so respawn persistence (respawnDao.TryInsertOrUpdateAsync or
whichever method persists liveRespawns) is attempted regardless of
objectivesSaved: log the InvalidOperationException and continue instead of
returning, attempt respawn save, aggregate success/failure for final handling,
and ensure both errors are logged (use the same logger and contextual
characterId) so transient objective failures don't skip respawn persistence.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Potential null reference from Console.ReadLine().

Console.ReadLine() returns null if the input stream is closed (e.g., Ctrl+C, redirected empty input). The null-forgiving operator on line 80 masks this without validation, which could cause unexpected behavior in SetFolder.

🛡️ Proposed fix to validate folder input

 logger.Warning(logLanguage[LogLanguageKey.ENTER_PATH]);
 var folder = Console.ReadLine();
+if (string.IsNullOrWhiteSpace(folder))
+{
+    throw new ArgumentException("No folder path provided.");
+}
 var inputRedirected = Console.IsInputRedirected;

Then remove the null-forgiving operator:

-factory.SetFolder(folder!);
+factory.SetFolder(folder);

📝 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

	private async Task RunInteractiveAsync(CancellationToken stoppingToken)
	{
	logger.Warning(logLanguage[LogLanguageKey.ENTER_PATH]);
	var folder = Console.ReadLine();
	var inputRedirected = Console.IsInputRedirected;
	logger.Information(
	$"{logLanguage[LogLanguageKey.PARSE_ALL]} [Y/n]");
	var key = default(ConsoleKeyInfo);
	if (!inputRedirected)
	{
	key = Console.ReadKey(true);
	}
	factory.SetFolder(folder!);
	private async Task RunInteractiveAsync(CancellationToken stoppingToken)
	{
	logger.Warning(logLanguage[LogLanguageKey.ENTER_PATH]);
	var folder = Console.ReadLine();
	if (string.IsNullOrWhiteSpace(folder))
	{
	throw new ArgumentException("No folder path provided.");
	}
	var inputRedirected = Console.IsInputRedirected;
	logger.Information(
	$"{logLanguage[LogLanguageKey.PARSE_ALL]} [Y/n]");
	var key = default(ConsoleKeyInfo);
	if (!inputRedirected)
	{
	key = Console.ReadKey(true);
	}
	factory.SetFolder(folder);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.Parser/Parser.cs` around lines 67 - 80, RunInteractiveAsync reads
folder from Console.ReadLine() into variable folder but immediately calls
factory.SetFolder(folder!) which uses the null-forgiving operator and can throw
if folder is null; validate folder before calling SetFolder (e.g., check if
folder is null or whitespace after Console.ReadLine(), re-prompt or log and
return/cancel) and remove the null-forgiving operator so factory.SetFolder is
only called with a non-null, validated string; reference RunInteractiveAsync,
the folder local variable, Console.ReadLine(), and factory.SetFolder when making
the change.

[coderabbitai]

Actionable comments posted: 2

🤖 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.PacketHandlers/Battle/RevivalPacketHandler.cs`:
- Around line 116-125: ReviveInArenaAsync currently falls back to
ReviveAtSavePointAsync when character.Gold < ArenaReviveCost without notifying
the player; update ReviveInArenaAsync to send the same kind of feedback message
used in ReviveInPlaceAsync (e.g., the NotEnoughPowerSeed or equivalent
"NotEnoughGold"/insufficient funds packet/message) immediately before calling
ReviveAtSavePointAsync so the player is informed of why they were sent to the
save point; reference ReviveInArenaAsync, ArenaReviveCost, and
ReviveAtSavePointAsync when locating where to add the SendPacketAsync call that
emits the user-facing error message.
- Line 169: The call awaits
session.SendPacketAsync(character.Group!.GeneratePinit()) should first guard
against a null Group; update the RevivalPacketHandler to retrieve the nullable
Group into a local (e.g., var group = character.Group) and only call
session.SendPacketAsync(group.GeneratePinit()) when group != null (matching the
MapChangeService pattern), thereby avoiding the null-forgiving operator and
potential NullReferenceException.

🪄 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: 54160ae8-f309-413e-9498-bdcdc2decd32

📥 Commits

Reviewing files that changed from the base of the PR and between b841631 and 4f5af61.

📒 Files selected for processing (1)

• src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Missing feedback message when gold is insufficient for arena revive.

When seeds are insufficient in ReviveInPlaceAsync, the player receives a NotEnoughPowerSeed message before falling back to save point. However, when gold is insufficient here, the fallback happens silently. This inconsistency may confuse players.

💡 Suggested improvement for consistent UX

 private async Task ReviveInArenaAsync(ClientSession session)
 {
     var character = session.Character;
-    if (character.Gold < ArenaReviveCost) { await ReviveAtSavePointAsync(session).ConfigureAwait(false); return; }
+    if (character.Gold < ArenaReviveCost)
+    {
+        await session.SendPacketAsync(new MsgiPacket
+        {
+            Type = MessageType.Default,
+            Message = Game18NConstString.NotEnoughGold, // or appropriate constant
+        }).ConfigureAwait(false);
+        await ReviveAtSavePointAsync(session).ConfigureAwait(false);
+        return;
+    }

     character.Gold -= ArenaReviveCost;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs` around lines 116 -
125, ReviveInArenaAsync currently falls back to ReviveAtSavePointAsync when
character.Gold < ArenaReviveCost without notifying the player; update
ReviveInArenaAsync to send the same kind of feedback message used in
ReviveInPlaceAsync (e.g., the NotEnoughPowerSeed or equivalent
"NotEnoughGold"/insufficient funds packet/message) immediately before calling
ReviveAtSavePointAsync so the player is informed of why they were sent to the
save point; reference ReviveInArenaAsync, ArenaReviveCost, and
ReviveAtSavePointAsync when locating where to add the SendPacketAsync call that
emits the user-facing error message.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if Group property can be null and how it's typically accessed
echo "=== Checking Group property definition ==="
ast-grep --pattern 'Group { get; $$ }'

echo ""
echo "=== Checking how GeneratePinit is called elsewhere ==="
rg -n -C2 'GeneratePinit\(\)' --type cs

echo ""
echo "=== Checking for null checks before Group access ==="
rg -n 'Group\s*!=\s*null|Group\s*==\s*null|Group\?\.' --type cs | head -30

Repository: NosCoreIO/NosCore

Length of output: 7366

---

🏁 Script executed:

# Find Character class definition and Group property
echo "=== Character class definition and Group property ==="
fd -n "Character\.cs$" -e cs | head -10

# Search for Group property definition in Character class
echo ""
echo "=== Searching for Group property definition ==="
rg -n "Group\s*{.*get;.*}" --type cs -A1 | head -20

# Check how characters are instantiated and if Group is initialized
echo ""
echo "=== Checking character instantiation and Group initialization ==="
rg -n "new.*Character|Character\s*=\s*" --type cs | head -20

Repository: NosCoreIO/NosCore

Length of output: 3575

---

🏁 Script executed:

# Find Character-related files
echo "=== Finding Character class files ==="
fd "Character" --type f -e cs | grep -i "character.cs" | head -10

# Search for Group property in entity/dto files
echo ""
echo "=== Looking for Group property definition ==="
rg -n "Group\s*\{" --type cs -A2 | head -30

# Check if Group has a null coalescing or default initialization
echo ""
echo "=== Checking Group property declarations ==="
rg -n "public.*Group" --type cs -B1 | head -40

# Check GroupDisconnectHandler to understand Group initialization
echo ""
echo "=== GroupDisconnectHandler context ==="
cat src/NosCore.GameObject/Networking/ClientSession/DisconnectHandlers/GroupDisconnectHandler.cs | head -50

Repository: NosCoreIO/NosCore

Length of output: 4007

---

🏁 Script executed:

# Get full context of RevivalPacketHandler, especially BroadcastReviveAsync method
echo "=== RevivalPacketHandler.cs around line 169 ==="
sed -n '140,175p' src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs

# Check if there's any Group initialization/assignment in the BroadcastReviveAsync method
echo ""
echo "=== Checking BroadcastReviveAsync full method ==="
rg -n "BroadcastReviveAsync" --type cs -A50 src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs | head -70

# Check GameStartPacketHandler to see context of the "Group init even for solo players" comment
echo ""
echo "=== GameStartPacketHandler context around line 125 ==="
sed -n '115,135p' src/NosCore.PacketHandlers/Game/GameStartPacketHandler.cs

Repository: NosCoreIO/NosCore

Length of output: 5978

---

Add null check before accessing character.Group on line 169.

The Group property is nullable (Group?), but the code uses a null-forgiving operator without validating that Group is not null. If a character is not in a group or the group reference is cleared, this will throw a NullReferenceException. Align with the defensive pattern used in MapChangeService which explicitly checks if (group != null) before calling GeneratePinit().

Suggested fix

-await session.SendPacketAsync(character.Group!.GeneratePinit()).ConfigureAwait(false);
+if (character.Group != null)
+{
+    await session.SendPacketAsync(character.Group.GeneratePinit()).ConfigureAwait(false);
+}

📝 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

	await session.SendPacketAsync(character.Group!.GeneratePinit()).ConfigureAwait(false);
	if (character.Group != null)
	{
	await session.SendPacketAsync(character.Group.GeneratePinit()).ConfigureAwait(false);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NosCore.PacketHandlers/Battle/RevivalPacketHandler.cs` at line 169, The
call awaits session.SendPacketAsync(character.Group!.GeneratePinit()) should
first guard against a null Group; update the RevivalPacketHandler to retrieve
the nullable Group into a local (e.g., var group = character.Group) and only
call session.SendPacketAsync(group.GeneratePinit()) when group != null (matching
the MapChangeService pattern), thereby avoiding the null-forgiving operator and
potential NullReferenceException.