NEO-41: address code review (E3.M1 status, deny-path test)

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 {

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
 

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
 

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.

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<InteractionResponse>();
+        Assert.NotNull(envelope);
+        Assert.True(envelope!.Allowed);
+
+        Assert.Equal(HttpStatusCode.OK, snapshot.StatusCode);
+        var body = await snapshot.Content.ReadFromJsonAsync<SkillProgressionSnapshotResponse>();
+        Assert.NotNull(body);
+        var salvage = body!.Skills!.Single(static s => s.Id == "salvage");
+        Assert.Equal(0, salvage.Xp);
+    }
 }

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;
+
+/// <summary>
+/// Same in-memory overrides as <see cref="InMemoryWebApplicationFactory"/>, plus a stub <see cref="ISkillDefinitionRegistry"/>
+/// where <c>salvage</c> does not allow <c>activity</c> — for NEO-41 defensive tests without mutating disk catalog.
+/// </summary>
+public sealed class SalvageActivityDeniedRegistryWebApplicationFactory : WebApplicationFactory<Program>
+{
+    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<TimeProvider>(fakeTime);
+            services.AddSingleton<IPositionStateStore, InMemoryPositionStateStore>();
+            services.AddSingleton<IPlayerHotbarLoadoutStore, InMemoryPlayerHotbarLoadoutStore>();
+            services.AddSingleton<IPlayerSkillProgressionStore, InMemoryPlayerSkillProgressionStore>();
+            services.AddSingleton<IPlayerAbilityCooldownStore, InMemoryPlayerAbilityCooldownStore>();
+            services.AddSingleton<ISkillDefinitionRegistry, SalvageActivityDeniedSkillRegistry>();
+        });
+    }
+}
+
+/// <summary>Prototype trio ids; <c>salvage</c> excludes <c>activity</c> so gather interact grant path denies without throwing.</summary>
+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<SkillDefRow> 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<SkillDefRow> GetDefinitionsInIdOrder() => Ordered;
+}

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,