From 70e3e9a17de29f16131da7e3e75ab81e204d1240 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 28 Jun 2026 10:01:08 -0400 Subject: [PATCH] NEO-150: Address code review findings Extract ContractPrototypeTestFixtures, add skill XP over-cap test, null-guard template, validation-order comment, and NEO-151 handoff note. --- docs/plans/NEO-150-implementation-plan.md | 3 +- docs/reviews/2026-06-28-NEO-150.md | 10 +- .../ContractEconomyValidationTests.cs | 99 ++++++------------- .../ContractGeneratorOperationsTests.cs | 56 +---------- .../ContractPrototypeTestFixtures.cs | 75 ++++++++++++++ .../Contracts/ContractEconomyValidation.cs | 2 + 6 files changed, 117 insertions(+), 128 deletions(-) create mode 100644 server/NeonSprawl.Server.Tests/Game/Contracts/ContractPrototypeTestFixtures.cs diff --git a/docs/plans/NEO-150-implementation-plan.md b/docs/plans/NEO-150-implementation-plan.md index cef9d2a..4569d04 100644 --- a/docs/plans/NEO-150-implementation-plan.md +++ b/docs/plans/NEO-150-implementation-plan.md @@ -54,7 +54,7 @@ Slice 4 AC — issued contracts validate against **prototype economy caps** and - **`ContractEconomyValidation.TryValidateTemplate`** — delegates to `PrototypeE7M4ContractCatalogRules.TryGetBandCapGateError` + `TryGetCrossRefError` for one template row against live registries. - **`ContractGeneratorOperations.TryIssue`** — extended signature (encounter/item/skill/faction registries); economy validation after template selection, before persist. - **`ContractGeneratorReasonCodes`:** `economy_cap_exceeded`, `invalid_reward_bundle`. -- **Tests:** `ContractEconomyValidationTests` (3 unit), `ContractGeneratorOperationsTests` (+2 deny paths); all `TryIssue` call sites updated; **911** tests green. +- **Tests:** `ContractEconomyValidationTests` (4 unit), `ContractGeneratorOperationsTests` (+2 deny paths), shared `ContractPrototypeTestFixtures`; all `TryIssue` call sites updated; **912** tests green. - **Docs:** `server/README.md`, `E7_M4_ContractMissionGenerator.md`, alignment register, module dependency register. ## Technical approach @@ -165,6 +165,7 @@ Manual Godot QA: **none** (server policy; capstone NEO-154). | `TryIssue` signature churn | Update all call sites in this story; NEO-151 HTTP wrapper inherits extended signature | `adopted` | | Client deny copy for `invalid_reward_bundle` | Defer readable copy to NEO-153; server ships stable code now | `deferred` | | Telemetry `reward_anomaly` at economy deny | Defer comment-only hooks to NEO-152 | `deferred` | +| **NEO-151 HTTP `TryIssue` wiring** | Production DI path must pass the same four registries already used in `ContractGeneratorOperationsIntegrationTests` (`IEncounterDefinitionRegistry`, `IItemDefinitionRegistry`, `ISkillDefinitionRegistry`, `IFactionDefinitionRegistry`) — verify at NEO-151 kickoff | `pending` | ## Client counterpart diff --git a/docs/reviews/2026-06-28-NEO-150.md b/docs/reviews/2026-06-28-NEO-150.md index 15d7baa..40d26b0 100644 --- a/docs/reviews/2026-06-28-NEO-150.md +++ b/docs/reviews/2026-06-28-NEO-150.md @@ -34,16 +34,16 @@ None. ## Suggestions -1. **Deduplicate prototype registry test fixtures** — `ContractEconomyValidationTests.CreatePrototypeRegistries()` and `ContractGeneratorOperationsTests.CreatePrototypeCatalogRegistries()` are nearly identical (~50 lines). Consider a shared internal test helper (e.g. `ContractPrototypeTestFixtures`) to avoid drift when prototype catalog ids change. +1. ~~**Deduplicate prototype registry test fixtures** — `ContractEconomyValidationTests.CreatePrototypeRegistries()` and `ContractGeneratorOperationsTests.CreatePrototypeCatalogRegistries()` are nearly identical (~50 lines). Consider a shared internal test helper (e.g. `ContractPrototypeTestFixtures`) to avoid drift when prototype catalog ids change.~~ **Done.** — `ContractPrototypeTestFixtures.CreateCatalogRegistries()`. -2. **Broaden deny-path test matrix (optional)** — Current tests exercise item qty over cap and unknown item id only. Band-cap and cross-ref helpers also cover skill XP, reputation grants, unknown encounter, and disallowed `mission_reward` skill sources (covered at loader level in `ContractTemplateCatalogLoaderTests`). One additional `[Theory]` or a skill-Xp-over-cap case in `ContractEconomyValidationTests` would document parity without much cost. +2. ~~**Broaden deny-path test matrix (optional)** — Current tests exercise item qty over cap and unknown item id only. Band-cap and cross-ref helpers also cover skill XP, reputation grants, unknown encounter, and disallowed `mission_reward` skill sources (covered at loader level in `ContractTemplateCatalogLoaderTests`). One additional `[Theory]` or a skill-Xp-over-cap case in `ContractEconomyValidationTests` would document parity without much cost.~~ **Done.** — `TryValidateTemplate_ShouldDenyEconomyCapExceeded_WhenSkillXpOverBandCap`. -3. **NEO-151 call-site checklist** — When HTTP lands, confirm the production DI path passes the same four registries already wired in integration tests. No code change needed here; worth a one-line note in NEO-151 plan. +3. ~~**NEO-151 call-site checklist** — When HTTP lands, confirm the production DI path passes the same four registries already wired in integration tests. No code change needed here; worth a one-line note in NEO-151 plan.~~ **Done.** — handoff row in [NEO-150 plan](../plans/NEO-150-implementation-plan.md) open questions (`pending` for NEO-151 kickoff). ## Nits -- Nit: `ContractEconomyValidation.TryValidateTemplate` does not null-check `template`; safe today because `TrySelectTemplate` only returns success with a non-null template, but `ArgumentNullException.ThrowIfNull(template)` would mirror the registry guards. -- Nit: Issue-time validation order is band-cap → cross-ref; startup loader runs cross-ref → band-cap. Intentional per plan (cap failures get `economy_cap_exceeded`), but worth keeping in mind if both gates ever fail on the same row during debugging. +- ~~Nit: `ContractEconomyValidation.TryValidateTemplate` does not null-check `template`; safe today because `TrySelectTemplate` only returns success with a non-null template, but `ArgumentNullException.ThrowIfNull(template)` would mirror the registry guards.~~ **Done.** +- ~~Nit: Issue-time validation order is band-cap → cross-ref; startup loader runs cross-ref → band-cap. Intentional per plan (cap failures get `economy_cap_exceeded`), but worth keeping in mind if both gates ever fail on the same row during debugging.~~ **Done.** — inline comment in `ContractEconomyValidation`. - Nit: Per-issue allocation of encounter/faction hash sets and skill dictionary is fine for prototype; revisit only if issue volume becomes hot-path. ## Verification diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractEconomyValidationTests.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractEconomyValidationTests.cs index 2bf9563..7da5044 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractEconomyValidationTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractEconomyValidationTests.cs @@ -1,24 +1,17 @@ using NeonSprawl.Server.Game.Contracts; using NeonSprawl.Server.Game.Encounters; -using NeonSprawl.Server.Game.Factions; -using NeonSprawl.Server.Game.Items; using NeonSprawl.Server.Game.Quests; -using NeonSprawl.Server.Game.Skills; namespace NeonSprawl.Server.Tests.Game.Contracts; public sealed class ContractEconomyValidationTests { - private const string TemplateId = "prototype_contract_clear_combat_pocket"; - private const string EncounterId = "prototype_combat_pocket"; - private const string GridOperatorsFactionId = "prototype_faction_grid_operators"; - [Fact] public void TryValidateTemplate_ShouldReturnTrue_WhenPrototypeFreezeRowAtCaps() { // Arrange - var template = PrototypeE7M4ContractCatalogRules.ExpectedTemplateFreeze[TemplateId]; - var registries = CreatePrototypeRegistries(); + var template = PrototypeE7M4ContractCatalogRules.ExpectedTemplateFreeze[ContractPrototypeTestFixtures.TemplateId]; + var registries = ContractPrototypeTestFixtures.CreateCatalogRegistries(); // Act var ok = ContractEconomyValidation.TryValidateTemplate( template, @@ -36,7 +29,7 @@ public sealed class ContractEconomyValidationTests public void TryValidateTemplate_ShouldDenyEconomyCapExceeded_WhenItemQuantityOverBandCap() { // Arrange - var prototype = PrototypeE7M4ContractCatalogRules.ExpectedTemplateFreeze[TemplateId]; + var prototype = PrototypeE7M4ContractCatalogRules.ExpectedTemplateFreeze[ContractPrototypeTestFixtures.TemplateId]; var overCapTemplate = prototype with { CompletionRewardBundle = new QuestRewardBundleRow( @@ -44,7 +37,33 @@ public sealed class ContractEconomyValidationTests prototype.CompletionRewardBundle.SkillXpGrants, prototype.CompletionRewardBundle.ReputationGrants), }; - var registries = CreatePrototypeRegistries(); + var registries = ContractPrototypeTestFixtures.CreateCatalogRegistries(); + // Act + var ok = ContractEconomyValidation.TryValidateTemplate( + overCapTemplate, + registries.EncounterRegistry, + registries.ItemRegistry, + registries.SkillRegistry, + registries.FactionRegistry, + out var reasonCode); + // Assert + Assert.False(ok); + Assert.Equal(ContractGeneratorReasonCodes.EconomyCapExceeded, reasonCode); + } + + [Fact] + public void TryValidateTemplate_ShouldDenyEconomyCapExceeded_WhenSkillXpOverBandCap() + { + // Arrange + var prototype = PrototypeE7M4ContractCatalogRules.ExpectedTemplateFreeze[ContractPrototypeTestFixtures.TemplateId]; + var overCapTemplate = prototype with + { + CompletionRewardBundle = new QuestRewardBundleRow( + prototype.CompletionRewardBundle.ItemGrants, + [new QuestSkillXpGrantRow("salvage", 26)], + prototype.CompletionRewardBundle.ReputationGrants), + }; + var registries = ContractPrototypeTestFixtures.CreateCatalogRegistries(); // Act var ok = ContractEconomyValidation.TryValidateTemplate( overCapTemplate, @@ -62,7 +81,7 @@ public sealed class ContractEconomyValidationTests public void TryValidateTemplate_ShouldDenyInvalidRewardBundle_WhenItemIdUnknown() { // Arrange - var prototype = PrototypeE7M4ContractCatalogRules.ExpectedTemplateFreeze[TemplateId]; + var prototype = PrototypeE7M4ContractCatalogRules.ExpectedTemplateFreeze[ContractPrototypeTestFixtures.TemplateId]; var unknownItemTemplate = prototype with { CompletionRewardBundle = new QuestRewardBundleRow( @@ -70,7 +89,7 @@ public sealed class ContractEconomyValidationTests prototype.CompletionRewardBundle.SkillXpGrants, prototype.CompletionRewardBundle.ReputationGrants), }; - var registries = CreatePrototypeRegistries(); + var registries = ContractPrototypeTestFixtures.CreateCatalogRegistries(); // Act var ok = ContractEconomyValidation.TryValidateTemplate( unknownItemTemplate, @@ -83,58 +102,4 @@ public sealed class ContractEconomyValidationTests Assert.False(ok); Assert.Equal(ContractGeneratorReasonCodes.InvalidRewardBundle, reasonCode); } - - private static CatalogRegistries CreatePrototypeRegistries() - { - var encounterRows = new Dictionary(StringComparer.Ordinal) - { - [EncounterId] = new( - EncounterId, - "Prototype Combat Pocket", - "defeat_all_targets", - ["prototype_npc_melee", "prototype_npc_ranged", "prototype_npc_elite"], - "prototype_combat_pocket_clear"), - }; - var encounterRegistry = new EncounterDefinitionRegistry( - new EncounterDefinitionCatalog("/tmp/encounters", encounterRows, catalogJsonFileCount: 1)); - - var itemRows = new Dictionary(StringComparer.Ordinal) - { - ["scrap_metal_bulk"] = new( - "scrap_metal_bulk", - "Scrap Metal (Bulk)", - "material", - StackMax: 999, - InventorySlotKind: "bag", - Rarity: null, - BindPolicy: null, - DurabilityMax: null), - }; - var itemRegistry = new ItemDefinitionRegistry( - new ItemDefinitionCatalog("/tmp/items", itemRows, catalogJsonFileCount: 1)); - - var skillRows = new Dictionary(StringComparer.Ordinal) - { - ["salvage"] = new("salvage", "gather", "Salvage", ["activity", "mission_reward"]), - ["refine"] = new("refine", "process", "Refine", ["activity", "mission_reward", "trainer"]), - ["intrusion"] = new("intrusion", "tech", "Intrusion", ["activity", "mission_reward", "book_or_item"]), - }; - var skillRegistry = new SkillDefinitionRegistry( - new SkillDefinitionCatalog("/tmp/skills", skillRows, catalogJsonFileCount: 1)); - - var factionRows = new Dictionary(StringComparer.Ordinal) - { - [GridOperatorsFactionId] = PrototypeE7M3FactionCatalogRules.ExpectedFactionFreeze[GridOperatorsFactionId], - }; - var factionRegistry = new FactionDefinitionRegistry( - new FactionDefinitionCatalog("/tmp/factions", factionRows, catalogJsonFileCount: 1)); - - return new CatalogRegistries(encounterRegistry, itemRegistry, skillRegistry, factionRegistry); - } - - private sealed record CatalogRegistries( - IEncounterDefinitionRegistry EncounterRegistry, - IItemDefinitionRegistry ItemRegistry, - ISkillDefinitionRegistry SkillRegistry, - IFactionDefinitionRegistry FactionRegistry); } diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractGeneratorOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractGeneratorOperationsTests.cs index 078256b..fca68a5 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractGeneratorOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractGeneratorOperationsTests.cs @@ -336,7 +336,7 @@ public sealed class ContractGeneratorOperationsTests var positionOptions = Options.Create(new GamePositionOptions { DevPlayerId = PlayerId }); var instanceStore = new InMemoryContractInstanceStore(positionOptions); var standingStore = CreateStandingStore(); - var catalogRegistries = CreatePrototypeCatalogRegistries(); + var catalogRegistries = ContractPrototypeTestFixtures.CreateCatalogRegistries(); return new GeneratorTestDependencies( templateRegistry, catalogRegistries.EncounterRegistry, @@ -348,54 +348,6 @@ public sealed class ContractGeneratorOperationsTests new FakeTimeProvider(IssuedAt)); } - private static CatalogRegistries CreatePrototypeCatalogRegistries() - { - var encounterRows = new Dictionary(StringComparer.Ordinal) - { - [EncounterId] = new( - EncounterId, - "Prototype Combat Pocket", - "defeat_all_targets", - ["prototype_npc_melee", "prototype_npc_ranged", "prototype_npc_elite"], - "prototype_combat_pocket_clear"), - }; - var encounterRegistry = new EncounterDefinitionRegistry( - new EncounterDefinitionCatalog("/tmp/encounters", encounterRows, catalogJsonFileCount: 1)); - - var itemRows = new Dictionary(StringComparer.Ordinal) - { - ["scrap_metal_bulk"] = new( - "scrap_metal_bulk", - "Scrap Metal (Bulk)", - "material", - StackMax: 999, - InventorySlotKind: "bag", - Rarity: null, - BindPolicy: null, - DurabilityMax: null), - }; - var itemRegistry = new ItemDefinitionRegistry( - new ItemDefinitionCatalog("/tmp/items", itemRows, catalogJsonFileCount: 1)); - - var skillRows = new Dictionary(StringComparer.Ordinal) - { - ["salvage"] = new("salvage", "gather", "Salvage", ["activity", "mission_reward"]), - ["refine"] = new("refine", "process", "Refine", ["activity", "mission_reward", "trainer"]), - ["intrusion"] = new("intrusion", "tech", "Intrusion", ["activity", "mission_reward", "book_or_item"]), - }; - var skillRegistry = new SkillDefinitionRegistry( - new SkillDefinitionCatalog("/tmp/skills", skillRows, catalogJsonFileCount: 1)); - - var factionRows = new Dictionary(StringComparer.Ordinal) - { - [GridOperatorsFactionId] = PrototypeE7M3FactionCatalogRules.ExpectedFactionFreeze[GridOperatorsFactionId], - }; - var factionRegistry = new FactionDefinitionRegistry( - new FactionDefinitionCatalog("/tmp/factions", factionRows, catalogJsonFileCount: 1)); - - return new CatalogRegistries(encounterRegistry, itemRegistry, skillRegistry, factionRegistry); - } - private static InMemoryFactionStandingStore CreateStandingStore() { var byId = new Dictionary(StringComparer.Ordinal) @@ -423,12 +375,6 @@ public sealed class ContractGeneratorOperationsTests IFactionStandingStore StandingStore, TimeProvider TimeProvider); - private sealed record CatalogRegistries( - IEncounterDefinitionRegistry EncounterRegistry, - IItemDefinitionRegistry ItemRegistry, - ISkillDefinitionRegistry SkillRegistry, - IFactionDefinitionRegistry FactionRegistry); - /// Simulates store create failure without populating the out snapshot (Bugbot regression). private sealed class CreateActiveNullSnapshotStore(IContractInstanceStore inner) : IContractInstanceStore { diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractPrototypeTestFixtures.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractPrototypeTestFixtures.cs new file mode 100644 index 0000000..9842dd9 --- /dev/null +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractPrototypeTestFixtures.cs @@ -0,0 +1,75 @@ +using NeonSprawl.Server.Game.Encounters; +using NeonSprawl.Server.Game.Factions; +using NeonSprawl.Server.Game.Items; +using NeonSprawl.Server.Game.Skills; + +namespace NeonSprawl.Server.Tests.Game.Contracts; + +/// +/// Shared in-memory prototype definition registries for contract generator and economy tests (NEO-150). +/// Keep ids aligned with freeze table. +/// +internal static class ContractPrototypeTestFixtures +{ + public const string TemplateId = "prototype_contract_clear_combat_pocket"; + public const string EncounterId = "prototype_combat_pocket"; + public const string GridOperatorsFactionId = "prototype_faction_grid_operators"; + + public static ContractPrototypeCatalogRegistries CreateCatalogRegistries() + { + var encounterRows = new Dictionary(StringComparer.Ordinal) + { + [EncounterId] = new( + EncounterId, + "Prototype Combat Pocket", + "defeat_all_targets", + ["prototype_npc_melee", "prototype_npc_ranged", "prototype_npc_elite"], + "prototype_combat_pocket_clear"), + }; + var encounterRegistry = new EncounterDefinitionRegistry( + new EncounterDefinitionCatalog("/tmp/encounters", encounterRows, catalogJsonFileCount: 1)); + + var itemRows = new Dictionary(StringComparer.Ordinal) + { + ["scrap_metal_bulk"] = new( + "scrap_metal_bulk", + "Scrap Metal (Bulk)", + "material", + StackMax: 999, + InventorySlotKind: "bag", + Rarity: null, + BindPolicy: null, + DurabilityMax: null), + }; + var itemRegistry = new ItemDefinitionRegistry( + new ItemDefinitionCatalog("/tmp/items", itemRows, catalogJsonFileCount: 1)); + + var skillRows = new Dictionary(StringComparer.Ordinal) + { + ["salvage"] = new("salvage", "gather", "Salvage", ["activity", "mission_reward"]), + ["refine"] = new("refine", "process", "Refine", ["activity", "mission_reward", "trainer"]), + ["intrusion"] = new("intrusion", "tech", "Intrusion", ["activity", "mission_reward", "book_or_item"]), + }; + var skillRegistry = new SkillDefinitionRegistry( + new SkillDefinitionCatalog("/tmp/skills", skillRows, catalogJsonFileCount: 1)); + + var factionRows = new Dictionary(StringComparer.Ordinal) + { + [GridOperatorsFactionId] = PrototypeE7M3FactionCatalogRules.ExpectedFactionFreeze[GridOperatorsFactionId], + }; + var factionRegistry = new FactionDefinitionRegistry( + new FactionDefinitionCatalog("/tmp/factions", factionRows, catalogJsonFileCount: 1)); + + return new ContractPrototypeCatalogRegistries( + encounterRegistry, + itemRegistry, + skillRegistry, + factionRegistry); + } +} + +internal sealed record ContractPrototypeCatalogRegistries( + IEncounterDefinitionRegistry EncounterRegistry, + IItemDefinitionRegistry ItemRegistry, + ISkillDefinitionRegistry SkillRegistry, + IFactionDefinitionRegistry FactionRegistry); diff --git a/server/NeonSprawl.Server/Game/Contracts/ContractEconomyValidation.cs b/server/NeonSprawl.Server/Game/Contracts/ContractEconomyValidation.cs index 61e0ae5..621a1af 100644 --- a/server/NeonSprawl.Server/Game/Contracts/ContractEconomyValidation.cs +++ b/server/NeonSprawl.Server/Game/Contracts/ContractEconomyValidation.cs @@ -22,6 +22,7 @@ public static class ContractEconomyValidation IFactionDefinitionRegistry factionRegistry, out string? reasonCode) { + ArgumentNullException.ThrowIfNull(template); ArgumentNullException.ThrowIfNull(encounterRegistry); ArgumentNullException.ThrowIfNull(itemRegistry); ArgumentNullException.ThrowIfNull(skillRegistry); @@ -33,6 +34,7 @@ public static class ContractEconomyValidation [template.Id] = template, }; + // Band-cap before cross-ref so deny maps to economy_cap_exceeded (startup loader runs cross-ref first). if (PrototypeE7M4ContractCatalogRules.TryGetBandCapGateError(rowsById) is not null) { reasonCode = ContractGeneratorReasonCodes.EconomyCapExceeded;