diff --git a/bruno/neon-sprawl-server/interaction/Post interact prototype resource node gather.bru b/bruno/neon-sprawl-server/interaction/Post interact prototype resource node gather.bru index df33408..0ce5486 100644 --- a/bruno/neon-sprawl-server/interaction/Post interact prototype resource node gather.bru +++ b/bruno/neon-sprawl-server/interaction/Post interact prototype resource node gather.bru @@ -6,6 +6,7 @@ meta { docs { NEO-41: successful interact on `resource_node` kind grants `salvage` XP (`sourceKind: activity`, 10 XP) via same server path as POST skill-progression. Run `Post move near prototype resource node` first. Optional: GET skill-progression to read `salvage.xp`. + Server tests include a stub-registry case where `activity` is disallowed for `salvage` (interact still allowed; XP unchanged). } post { diff --git a/docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md b/docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md index 9666de9..380fd6e 100644 --- a/docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md +++ b/docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md @@ -7,7 +7,7 @@ | **Module ID** | E3.M1 | | **Epic** | [Epic 3 — Crafting Economy](../epics/epic_03_crafting_economy.md) | | **Stage target** | Prototype | -| **Status** | Planned (see [dependency register](module_dependency_register.md)) | +| **Status** | In Progress (see [dependency register](module_dependency_register.md); prototype gather XP anchor [NEO-41](../../plans/NEO-41-implementation-plan.md)) | ## Purpose diff --git a/docs/plans/NEO-41-implementation-plan.md b/docs/plans/NEO-41-implementation-plan.md index fd860f7..85bc380 100644 --- a/docs/plans/NEO-41-implementation-plan.md +++ b/docs/plans/NEO-41-implementation-plan.md @@ -64,7 +64,8 @@ | `server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs` | Shared NEO-38 grant apply + snapshot envelope for POST and gather hook. | | `server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantApplyOutcome.cs` | `TryApplyGrant` outcome kind + optional response (deny / granted / store missing). | | `server/NeonSprawl.Server/Game/Skills/GatherSkillXpConstants.cs` | Prototype `salvage` + `activity` + XP amount for resource-node gather. | -| `server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs` | In-range `resource_node` interact increases `salvage` XP on GET snapshot; terminal interact does not. | +| `server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs` | In-range `resource_node` interact increases `salvage` XP on GET snapshot; terminal interact does not; stub-registry deny path. | +| `server/NeonSprawl.Server.Tests/Game/Interaction/SalvageActivityDeniedRegistryWebApplicationFactory.cs` | Test host mirroring in-memory overrides with **`salvage`** excluding **`activity`** for defensive coverage. | | `docs/manual-qa/NEO-41.md` | curl: move near node → interact → GET skill-progression. | | `bruno/neon-sprawl-server/position/Post move near prototype resource node.bru` | Setup move for NEO-41 Bruno flow. | | `bruno/neon-sprawl-server/interaction/Post interact prototype resource node gather.bru` | Bruno: interact `prototype_resource_node_alpha` after move. | @@ -84,7 +85,7 @@ |------|----------| | `server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionGrantApiTests.cs` | Regression: existing POST cases still pass after refactor to shared helper. | | New or extended **`Interaction*Tests.cs`** | `resource_node` interact from seeded/in-range position → GET `…/skill-progression` shows increased **`salvage`** XP; **`prototype_terminal`** interact → **`salvage`** XP unchanged. | -| Optional | Grant helper unit tests for deny paths if extraction makes them cheap to test without HTTP. | +| **`SalvageActivityDeniedRegistryWebApplicationFactory`** + third test method | Stub **`ISkillDefinitionRegistry`** where **`salvage`** disallows **`activity`**: interact still **`allowed: true`**, **`salvage`** XP stays **0** (no throw). | ## Open questions / risks diff --git a/docs/reviews/2026-05-10-NEO-41.md b/docs/reviews/2026-05-10-NEO-41.md index cbeeb32..67d36fb 100644 --- a/docs/reviews/2026-05-10-NEO-41.md +++ b/docs/reviews/2026-05-10-NEO-41.md @@ -6,15 +6,17 @@ **Base:** `origin/main` +**Follow-up:** Suggestions and nits below are **done** (strikethrough + **Done.**). + --- ## Verdict -**Approve with nits** — Implementation matches the adopted plan and server tests pass; one documentation alignment nit should be fixed before or immediately after merge. +**Approve** — Implementation matches the adopted plan; review nits and optional test suggestion addressed in follow-up commit. ## Summary -This work introduces shared `SkillProgressionGrantOperations.TryApplyGrant` (extracted from NEO-38 POST handling), wires `POST /game/players/{id}/interact` to call it when the resolved prototype entry has `kind: resource_node`, using fixed `salvage` + `activity` + 10 XP via `GatherSkillXpConstants`. Denied grants and missing progression store are intentionally silent on the interact path (outcome remains allowed), matching the plan. NEO-40 telemetry hook comments now live on the shared grant path so both HTTP POST and interact-triggered grants see the same hook sites. Docs, Bruno requests, manual QA, and decomposition updates accompany the code. **Risk:** Low; behavior is additive, authority remains server-side, and 135 server tests pass (`dotnet test NeonSprawl.sln`). +This work introduces shared `SkillProgressionGrantOperations.TryApplyGrant` (extracted from NEO-38 POST handling), wires `POST /game/players/{id}/interact` to call it when the resolved prototype entry has `kind: resource_node`, using fixed `salvage` + `activity` + 10 XP via `GatherSkillXpConstants`. Denied grants and missing progression store are intentionally silent on the interact path (outcome remains allowed), matching the plan. NEO-40 telemetry hook comments now live on the shared grant path so both HTTP POST and interact-triggered grants see the same hook sites. Docs, Bruno requests, manual QA, and decomposition updates accompany the code. **Risk:** Low; behavior is additive, authority remains server-side, and **136** server tests pass (`dotnet test NeonSprawl.sln`). ## Documentation checked @@ -22,12 +24,12 @@ This work introduces shared `SkillProgressionGrantOperations.TryApplyGrant` (ext - `docs/decomposition/modules/module_dependency_register.md` — **matches** (E3.M1 row moved to **In Progress**; E2.M2 note consistent with NEO-41). - `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches** (E2.M2 / E3.M1 tracking rows updated for NEO-41). - `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — **matches** (NEO-41 implementation snapshot paragraph). -- `docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md` — **partially matches** (prototype anchor paragraph added per plan; **Summary** table **Status** still says **Planned** while the dependency register lists E3.M1 as **In Progress** — conflicts with [documentation_and_implementation_alignment.md](documentation_and_implementation_alignment.md) rule that Summary **Status** should match the register). +- `docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md` — **Matches** — Summary **Status** set to **In Progress** (aligned with register); prototype anchor paragraph retained. - `docs/manual-qa/NEO-41.md` — **matches** intent (move → interact → GET progression). - `server/README.md` (diff) — **matches** plan (prototype gather XP pointer). - Cross-cutting `client_server_authority.md` / `contracts.md` — **N/A** for this slice (no new client authority; HTTP spikes unchanged in contract “Ready” sense). -**Register / tracking:** This PR already updates the implementation tracking table and register; no further tracking update required once the E3.M1 module page Summary **Status** is aligned (see nits). +**Register / tracking:** Implementation tracking table + register updated; E3.M1 module Summary **Status** aligned with register in follow-up. ## Blocking issues @@ -35,15 +37,15 @@ _None._ ## Suggestions -1. **Optional test from plan:** The implementation plan called out an optional unit/integration test for a “corrupt catalog” / disallowed `sourceKind` path ensuring the interact handler does not throw and still returns `Allowed: true`. The current tests cover terminal (no XP) and resource node (XP) happy paths; a focused test with a stub registry that denies `activity` for `salvage` would fully close the stated defensive scenario without needing production catalog corruption. +1. ~~**Optional test from plan:** The implementation plan called out an optional unit/integration test for a “corrupt catalog” / disallowed `sourceKind` path ensuring the interact handler does not throw and still returns `Allowed: true`. The current tests cover terminal (no XP) and resource node (XP) happy paths; a focused test with a stub registry that denies `activity` for `salvage` would fully close the stated defensive scenario without needing production catalog corruption.~~ **Done.** (`SalvageActivityDeniedRegistryWebApplicationFactory` + `PostInteract_ResourceNode_WhenSalvageDisallowsActivity_ShouldStillAllowInteract_AndNotApplyXp`.) -2. **E3.M1 Summary table:** Update the **Status** field in `docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md` from **Planned** to **In Progress** so it matches `module_dependency_register.md` and [documentation_and_implementation_alignment.md](documentation_and_implementation_alignment.md) (“Summary table Status field should match the register row”). +2. ~~**E3.M1 Summary table:** Update the **Status** field in `docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md` from **Planned** to **In Progress** so it matches `module_dependency_register.md` and [documentation_and_implementation_alignment.md](documentation_and_implementation_alignment.md) (“Summary table Status field should match the register row”).~~ **Done.** ## Nits -- **Nit:** `_ = SkillProgressionGrantOperations.TryApplyGrant(...)` is clear enough; if the team prefers explicit fire-and-forget documentation, a one-line comment that the outcome is intentionally ignored on the interact path could mirror the plan wording (optional). +- ~~**Nit:** `_ = SkillProgressionGrantOperations.TryApplyGrant(...)` is clear enough; if the team prefers explicit fire-and-forget documentation, a one-line comment that the outcome is intentionally ignored on the interact path could mirror the plan wording (optional).~~ **Done.** (Comment on interact path in `InteractionApi.cs`.) ## Verification -- `dotnet test NeonSprawl.sln` — **passed** (135 tests) on review date. +- `dotnet test NeonSprawl.sln` — **passed** (**136** tests) after follow-up. - Manual: follow `docs/manual-qa/NEO-41.md` and/or Bruno flows under `bruno/neon-sprawl-server/position/` and `bruno/neon-sprawl-server/interaction/` for end-to-end smoke. diff --git a/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs b/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs index 314c35b..19a682d 100644 --- a/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Interaction/InteractionResourceNodeGatherXpTests.cs @@ -79,4 +79,40 @@ public sealed class InteractionResourceNodeGatherXpTests var salvage = body!.Skills!.Single(static s => s.Id == "salvage"); Assert.Equal(GatherSkillXpConstants.ActivityXpPerResourceNodeGather, salvage.Xp); } + + [Fact] + public async Task PostInteract_ResourceNode_WhenSalvageDisallowsActivity_ShouldStillAllowInteract_AndNotApplyXp() + { + // Arrange — stub registry: salvage exists but activity is not in allowedXpSourceKinds (defensive / corrupt-catalog stand-in). + await using var factory = new SalvageActivityDeniedRegistryWebApplicationFactory(); + var client = factory.CreateClient(); + var move = new MoveCommandRequest + { + SchemaVersion = MoveCommandRequest.CurrentSchemaVersion, + Target = new PositionVector { X = 10, Y = 0.9, Z = -6 }, + }; + await client.PostAsJsonAsync("/game/players/dev-local-1/move", move); + + // Act + var interact = await client.PostAsJsonAsync( + "/game/players/dev-local-1/interact", + new InteractionRequest + { + SchemaVersion = InteractionRequest.CurrentSchemaVersion, + InteractableId = PrototypeInteractableRegistry.PrototypeResourceNodeAlphaId, + }); + var snapshot = await client.GetAsync("/game/players/dev-local-1/skill-progression"); + + // Assert + Assert.Equal(HttpStatusCode.OK, interact.StatusCode); + var envelope = await interact.Content.ReadFromJsonAsync(); + Assert.NotNull(envelope); + Assert.True(envelope!.Allowed); + + Assert.Equal(HttpStatusCode.OK, snapshot.StatusCode); + var body = await snapshot.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + var salvage = body!.Skills!.Single(static s => s.Id == "salvage"); + Assert.Equal(0, salvage.Xp); + } } diff --git a/server/NeonSprawl.Server.Tests/Game/Interaction/SalvageActivityDeniedRegistryWebApplicationFactory.cs b/server/NeonSprawl.Server.Tests/Game/Interaction/SalvageActivityDeniedRegistryWebApplicationFactory.cs new file mode 100644 index 0000000..ab4dc01 --- /dev/null +++ b/server/NeonSprawl.Server.Tests/Game/Interaction/SalvageActivityDeniedRegistryWebApplicationFactory.cs @@ -0,0 +1,112 @@ +using System.Diagnostics.CodeAnalysis; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Time.Testing; +using NeonSprawl.Server.Game.AbilityInput; +using NeonSprawl.Server.Game.PositionState; +using NeonSprawl.Server.Game.Skills; +using Npgsql; + +namespace NeonSprawl.Server.Tests.Game.Interaction; + +/// +/// Same in-memory overrides as , plus a stub +/// where salvage does not allow activity — for NEO-41 defensive tests without mutating disk catalog. +/// +public sealed class SalvageActivityDeniedRegistryWebApplicationFactory : WebApplicationFactory +{ + protected override void ConfigureWebHost(IWebHostBuilder builder) + { + builder.UseEnvironment("Testing"); + + var skillsDir = SkillCatalogPathResolution.TryDiscoverSkillsDirectory(AppContext.BaseDirectory) + ?? throw new InvalidOperationException( + "Could not discover repo content/skills from AppContext.BaseDirectory; run tests from the neon-sprawl clone."); + builder.UseSetting("Content:SkillsDirectory", skillsDir); + + builder.ConfigureTestServices(services => + { + for (var i = services.Count - 1; i >= 0; i--) + { + var d = services[i]; + if (d.ServiceType == typeof(IPositionStateStore) || + d.ServiceType == typeof(IPlayerHotbarLoadoutStore) || + d.ServiceType == typeof(IPlayerSkillProgressionStore) || + d.ServiceType == typeof(TimeProvider) || + d.ServiceType == typeof(IPlayerAbilityCooldownStore) || + d.ServiceType == typeof(ISkillDefinitionRegistry) || + d.ServiceType == typeof(NpgsqlDataSource) || + (d.ServiceType == typeof(IHostedService) && + (d.ImplementationType == typeof(PostgresDevPlayerSeedHostedService) || + d.ImplementationType == typeof(NpgsqlDataSourceShutdownHostedService)))) + { + services.RemoveAt(i); + } + } + + var fakeTime = new FakeTimeProvider(new DateTimeOffset(2026, 4, 30, 12, 0, 0, TimeSpan.Zero)); + services.AddSingleton(fakeTime); + services.AddSingleton(); + services.AddSingleton(); + services.AddSingleton(); + services.AddSingleton(); + services.AddSingleton(); + }); + } +} + +/// Prototype trio ids; salvage excludes activity so gather interact grant path denies without throwing. +internal sealed class SalvageActivityDeniedSkillRegistry : ISkillDefinitionRegistry +{ + private static readonly SkillDefRow SalvageNoActivity = new( + "salvage", + "gather", + "Salvage", + new[] { "mission_reward" }); + + private static readonly SkillDefRow Refine = new( + "refine", + "process", + "Refine", + new[] { "activity", "mission_reward", "trainer" }); + + private static readonly SkillDefRow Intrusion = new( + "intrusion", + "tech", + "Intrusion", + new[] { "activity", "mission_reward", "book_or_item" }); + + private static readonly IReadOnlyList Ordered = + [ + Intrusion, + Refine, + SalvageNoActivity, + ]; + + public bool TryGetDefinition(string? skillId, [NotNullWhen(true)] out SkillDefRow? definition) + { + if (skillId is null) + { + definition = null; + return false; + } + + var k = skillId.Trim(); + foreach (var row in Ordered) + { + if (string.Equals(row.Id, k, StringComparison.OrdinalIgnoreCase)) + { + definition = row; + return true; + } + } + + definition = null; + return false; + } + + public IReadOnlyList GetDefinitionsInIdOrder() => Ordered; +} diff --git a/server/NeonSprawl.Server/Game/Interaction/InteractionApi.cs b/server/NeonSprawl.Server/Game/Interaction/InteractionApi.cs index e8d421e..4ed189f 100644 --- a/server/NeonSprawl.Server/Game/Interaction/InteractionApi.cs +++ b/server/NeonSprawl.Server/Game/Interaction/InteractionApi.cs @@ -71,6 +71,7 @@ public static class InteractionApi if (string.Equals(entry.Kind, PrototypeInteractableRegistry.ResourceNodeKind, StringComparison.Ordinal)) { // NEO-41: prototype gather completion — same grant rules/persistence as POST …/skill-progression. + // Interact response stays Allowed=true; grant outcome is intentionally ignored (silent no-op on deny / store miss per plan). _ = SkillProgressionGrantOperations.TryApplyGrant( playerKey, GatherSkillXpConstants.SalvageSkillId,