don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
pull/192/head
VinPropane 2026-06-28 10:01:08 -04:00
parent e3d8458a66
commit 70e3e9a17d
6 changed files with 117 additions and 128 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
docs/plans/NEO-150-implementation-plan.md
View File

@ -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

10
docs/reviews/2026-06-28-NEO-150.md
View File

@ -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

99
server/NeonSprawl.Server.Tests/Game/Contracts/ContractEconomyValidationTests.cs
View File

@ -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<string, EncounterDefRow>(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<string, ItemDefRow>(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<string, SkillDefRow>(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<string, FactionDefRow>(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);
}

56
server/NeonSprawl.Server.Tests/Game/Contracts/ContractGeneratorOperationsTests.cs
View File

@ -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<string, EncounterDefRow>(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<string, ItemDefRow>(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<string, SkillDefRow>(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<string, FactionDefRow>(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<string, FactionDefRow>(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);
/// <summary>Simulates store create failure without populating the out snapshot (Bugbot regression).</summary>
private sealed class CreateActiveNullSnapshotStore(IContractInstanceStore inner) : IContractInstanceStore
{

75
server/NeonSprawl.Server.Tests/Game/Contracts/ContractPrototypeTestFixtures.cs 100644
View File

@ -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;
/// <summary>
/// Shared in-memory prototype definition registries for contract generator and economy tests (NEO-150).
/// Keep ids aligned with <see cref="Game.Contracts.PrototypeE7M4ContractCatalogRules"/> freeze table.
/// </summary>
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<string, EncounterDefRow>(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<string, ItemDefRow>(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<string, SkillDefRow>(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<string, FactionDefRow>(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);

2
server/NeonSprawl.Server/Game/Contracts/ContractEconomyValidation.cs
View File

@ -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;