From e4e75f5dc7c5769cad4f62ac91cb772f087eaaa9 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 31 May 2026 15:43:37 -0400 Subject: [PATCH] NEO-103: Address code review follow-ups. Mark E5M3-04 landed in module Summary, assert fixedGrants order in C# test, add Bruno ordinal comment, strike through review items. --- .../Get encounter definitions.bru | 1 + .../modules/E5_M3_EncounterAndRewardTables.md | 2 +- docs/reviews/2026-05-31-NEO-103.md | 8 +++---- .../EncounterDefinitionsWorldApiTests.cs | 21 ++++++++++++------- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/bruno/neon-sprawl-server/encounter-definitions/Get encounter definitions.bru b/bruno/neon-sprawl-server/encounter-definitions/Get encounter definitions.bru index 0646c75..4686f1e 100644 --- a/bruno/neon-sprawl-server/encounter-definitions/Get encounter definitions.bru +++ b/bruno/neon-sprawl-server/encounter-definitions/Get encounter definitions.bru @@ -23,6 +23,7 @@ tests { test("encounters are ascending by id (ordinal)", function () { const body = res.getBody(); const ids = body.encounters.map((x) => x.id); + // Match server StringComparer.Ordinal (prototype ids are ASCII snake_case). const sorted = [...ids].sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)); expect(ids).to.eql(sorted); }); diff --git a/docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md b/docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md index 18b88f1..a67f963 100644 --- a/docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md +++ b/docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md @@ -7,7 +7,7 @@ | **Module ID** | E5.M3 | | **Epic** | [Epic 5 — PvE Combat](../epics/epic_05_pve_combat.md) | | **Stage target** | Prototype | -| **Status** | In Progress — Slice 3 backlog [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md): **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) catalog + CI landed · **E5M3-02** [NEO-101](https://linear.app/neon-sprawl/issue/NEO-101) server load landed · **E5M3-03** [NEO-102](https://linear.app/neon-sprawl/issue/NEO-102) registries landed · **E5M3-04** [NEO-103](https://linear.app/neon-sprawl/issue/NEO-103) → **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) | +| **Status** | In Progress — Slice 3 backlog [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md): **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) catalog + CI landed · **E5M3-02** [NEO-101](https://linear.app/neon-sprawl/issue/NEO-101) server load landed · **E5M3-03** [NEO-102](https://linear.app/neon-sprawl/issue/NEO-102) registries landed · **E5M3-04** [NEO-103](https://linear.app/neon-sprawl/issue/NEO-103) HTTP read landed · **E5M3-05** [NEO-104](https://linear.app/neon-sprawl/issue/NEO-104) → **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) | | **Linear** | Label **`E5.M3`** · [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md) **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) · **E5M3-02** [NEO-101](https://linear.app/neon-sprawl/issue/NEO-101) · **E5M3-03** [NEO-102](https://linear.app/neon-sprawl/issue/NEO-102) · **E5M3-04** [NEO-103](https://linear.app/neon-sprawl/issue/NEO-103) · **E5M3-05** [NEO-104](https://linear.app/neon-sprawl/issue/NEO-104) · **E5M3-06** [NEO-105](https://linear.app/neon-sprawl/issue/NEO-105) · **E5M3-07** [NEO-106](https://linear.app/neon-sprawl/issue/NEO-106) · **E5M3-08** [NEO-108](https://linear.app/neon-sprawl/issue/NEO-108) · **E5M3-09** [NEO-107](https://linear.app/neon-sprawl/issue/NEO-107) · **E5M3-10** [NEO-109](https://linear.app/neon-sprawl/issue/NEO-109) · **E5M3-11** [NEO-110](https://linear.app/neon-sprawl/issue/NEO-110) · **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) | ## Purpose diff --git a/docs/reviews/2026-05-31-NEO-103.md b/docs/reviews/2026-05-31-NEO-103.md index 801b956..46df0ed 100644 --- a/docs/reviews/2026-05-31-NEO-103.md +++ b/docs/reviews/2026-05-31-NEO-103.md @@ -18,7 +18,7 @@ NEO-103 adds **`GET /game/world/encounter-definitions`**, mirroring the establis |------|--------| | `docs/plans/NEO-103-implementation-plan.md` | **Matches** — nested `rewardTable`, nested `completionCriteria`, registry injection, fail-fast reward-table guard, Bruno + AAA test, README; acceptance checklist checked; reconciliation section accurate. | | `docs/plans/E5M3-prototype-backlog.md` (E5M3-04) | **Matches** — acceptance criteria checked; landed note cites plan. | -| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Partially matches** — **Implementation snapshot** HTTP bullet added; Summary **Status** row still lists **E5M3-04 NEO-103 →** without a **landed** marker (NEO-101/102 updated Summary when their slices shipped). | +| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — **Implementation snapshot** HTTP bullet added; Summary **Status** row marks **E5M3-04 / NEO-103 HTTP read landed** (review follow-up). | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-103** HTTP read; runtime **NEO-104+** still outstanding. | | `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-103 landed** + README + Bruno. | | Full-stack epic decomposition | **N/A** — plan explicitly excludes client counterpart until NEO-110 / NEO-111. | @@ -30,13 +30,13 @@ None. ## Suggestions -1. **Sync `E5_M3_EncounterAndRewardTables.md` Summary Status** — Update the **Status** table row to mark **E5M3-04 / NEO-103 HTTP read landed** (and point forward to **E5M3-05 / NEO-104**), mirroring how **NEO-101** and **NEO-102** landed markers appear in the same row. The implementation snapshot and register are already updated; only the Summary line lags. +1. ~~**Sync `E5_M3_EncounterAndRewardTables.md` Summary Status** — Update the **Status** table row to mark **E5M3-04 / NEO-103 HTTP read landed** (and point forward to **E5M3-05 / NEO-104**), mirroring how **NEO-101** and **NEO-102** landed markers appear in the same row. The implementation snapshot and register are already updated; only the Summary line lags.~~ **Done.** Summary **Status** row updated. -2. **Optional: assert `fixedGrants` order in C# test** — Bruno asserts the exact grant sequence (`scrap_metal_bulk` then `contract_handoff_token`); the integration test checks count and per-item quantities via `Single` but not array order. Adding an ordered assertion (or a frozen grants constant) would lock catalog-order regressions in `dotnet test` without relying on Bruno alone (NEO-53 precedent). +2. ~~**Optional: assert `fixedGrants` order in C# test** — Bruno asserts the exact grant sequence (`scrap_metal_bulk` then `contract_handoff_token`); the integration test checks count and per-item quantities via `Single` but not array order. Adding an ordered assertion (or a frozen grants constant) would lock catalog-order regressions in `dotnet test` without relying on Bruno alone (NEO-53 precedent).~~ **Done.** `FrozenPrototypeGrantsInOrder` + indexed assertions in `EncounterDefinitionsWorldApiTests`. ## Nits -- Nit: Bruno **`encounters are ascending by id (ordinal)`** uses a hand-rolled string compare; peer requests (e.g. item-definitions) document **StringComparer.Ordinal** in a comment. Harmless for ASCII snake_case ids; copy the comment for parity if touching the file. +- ~~Nit: Bruno **`encounters are ascending by id (ordinal)`** uses a hand-rolled string compare; peer requests (e.g. item-definitions) document **StringComparer.Ordinal** in a comment. Harmless for ASCII snake_case ids; copy the comment for parity if touching the file.~~ **Done.** Ordinal comment added to Bruno id-order test. - Nit: With a single prototype encounter, the id-order Bruno test and C# **`Assert.Single`** are structurally redundant today; they become valuable when the roster expands—no change required now. diff --git a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterDefinitionsWorldApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterDefinitionsWorldApiTests.cs index 1ada5af..39e475b 100644 --- a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterDefinitionsWorldApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterDefinitionsWorldApiTests.cs @@ -1,4 +1,3 @@ -using System.Linq; using System.Net; using System.Net.Http.Json; using NeonSprawl.Server.Game.Encounters; @@ -11,6 +10,13 @@ public class EncounterDefinitionsWorldApiTests /// Frozen prototype encounter id. Keep in sync with Bruno. public const string PrototypeCombatPocket = "prototype_combat_pocket"; + /// Frozen prototype grant sequence in catalog order. Keep in sync with Bruno. + public static readonly (string ItemId, int Quantity)[] FrozenPrototypeGrantsInOrder = + [ + ("scrap_metal_bulk", 10), + ("contract_handoff_token", 1), + ]; + [Fact] public async Task GetEncounterDefinitions_ShouldReturnSchemaV1_WithFrozenEncounterAndNestedRewardTable() { @@ -39,12 +45,11 @@ public class EncounterDefinitionsWorldApiTests Assert.NotNull(encounter.RewardTable); Assert.Equal("prototype_combat_pocket_clear", encounter.RewardTable.Id); Assert.Equal("Prototype Combat Pocket Clear", encounter.RewardTable.DisplayName); - Assert.Equal(2, encounter.RewardTable.FixedGrants.Count); - - var scrapGrant = encounter.RewardTable.FixedGrants.Single(g => g.ItemId == "scrap_metal_bulk"); - Assert.Equal(10, scrapGrant.Quantity); - - var tokenGrant = encounter.RewardTable.FixedGrants.Single(g => g.ItemId == "contract_handoff_token"); - Assert.Equal(1, tokenGrant.Quantity); + Assert.Equal(FrozenPrototypeGrantsInOrder.Length, encounter.RewardTable.FixedGrants.Count); + for (var i = 0; i < FrozenPrototypeGrantsInOrder.Length; i++) + { + Assert.Equal(FrozenPrototypeGrantsInOrder[i].ItemId, encounter.RewardTable.FixedGrants[i].ItemId); + Assert.Equal(FrozenPrototypeGrantsInOrder[i].Quantity, encounter.RewardTable.FixedGrants[i].Quantity); + } } }