67 lines
8.2 KiB
Markdown
67 lines
8.2 KiB
Markdown
# Code review: NEO-42 (craft/refine → refine skill XP, prep helper)
|
|
|
|
**Date:** 2026-05-10
|
|
|
|
**Scope:** Branch `NEO-42-craft-refine-skill-xp-activity` (commits `20a141b`, `15bf598`) vs `origin/main`. Linear issue NEO-42. Diff: 7 files, +377 / -0.
|
|
|
|
**Base:** `origin/main`
|
|
|
|
---
|
|
|
|
## Verdict
|
|
|
|
**Approve with nits** — Plan-aligned preparatory slice: adds `RefineActivitySkillXpGrant` + `RefineSkillXpConstants` (parallel to NEO-41 gather constants), defensive deny-path test factory, and docs (server README, manual QA, implementation plan). No live E3.M2 call site exists yet (module absent), which is explicitly acknowledged in the plan, README, and manual QA. All `dotnet test NeonSprawl.sln` tests pass (**139** total, **3** new under `RefineActivitySkillXpGrantTests`). Two doc-alignment suggestions and a few nits below.
|
|
|
|
## Summary
|
|
|
|
This PR adds a thin static helper `RefineActivitySkillXpGrant.GrantOnSuccessfulCraftOrRefine(playerId, registry, xpStore, levelCurve)` that delegates to `SkillProgressionGrantOperations.TryApplyGrant` with `refine` + `activity` + **10** XP, plus a constants holder. There is no recipe/craft execution path in the repo to call it from, so the helper is shipped as a known callsite for a future E3.M2 merge. Three xUnit tests use the existing AAA pattern: a happy-path that asserts the in-memory store gained 10 XP under `refine`, a happy-path that round-trips through `GET …/skill-progression`, and a deny path using a new `RefineActivityDeniedRegistryWebApplicationFactory` (refine catalog row without `activity` in `allowedXpSourceKinds`). Server README and `docs/manual-qa/NEO-42.md` document the contract and verification flow via the existing `POST …/skill-progression` control endpoint. **Risk:** Low; pure additive surface, server authority unchanged, helper is unreferenced until E3.M2 wires it.
|
|
|
|
## Documentation checked
|
|
|
|
- `docs/plans/NEO-42-implementation-plan.md` — **matches** (kickoff clarifications honored, both acceptance checkboxes correspond to landed artifacts, files added/modified list matches diff, helper-then-E3.M2-call shape preserved).
|
|
- `docs/decomposition/modules/module_dependency_register.md` — **matches** for E3.M2 row (kept **Planned**, defensible since no `CraftRequest` / `RecipeDef` / `CraftResult` implemented); **matches** for E2.M2 row (already **In Progress**).
|
|
- `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — **matches** (post-follow-up: **NEO-42 landed (prep)** paragraph + backlog text updated). ~~**partially matches**: the "Implementation snapshot" still asserts "Still backlog within this module: Slice 3 NEO-42, NEO-43"; NEO-42 has landed as a preparatory helper and should get a parallel "NEO-42 landed (prep)" paragraph the way NEO-41 did. See Suggestion 1.~~ **Done.**
|
|
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches** (E2.M2 tracking row extended with NEO-42 prep + plan link). ~~**partially matches**: the E2.M2 tracking-table row enumerates NEO-37 → NEO-41 with landed notes but does not yet add a NEO-42 fragment. See Suggestion 1.~~ **Done.**
|
|
- `docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md` — **matches** (**Implementation snapshot** prep paragraph added). ~~**partially matches**: doc still has no Implementation snapshot; adding a single bullet noting the preparatory helper + plan would help E3.M2 readers find the wiring point when they later implement `CraftResult`. See Suggestion 2.~~ **Done.**
|
|
- `docs/manual-qa/NEO-42.md` — **matches** (`book_or_item` is correctly outside `refine.allowedXpSourceKinds = [activity, mission_reward, trainer]` in `content/skills/prototype_skills.json`; control POST + GET steps reflect the real grant path).
|
|
- `server/README.md` (diff) — **matches** plan (Craft / refine hook → skill XP (NEO-42) section).
|
|
- Cross-cutting `client_server_authority.md` / `contracts.md` — **N/A** (no new public surface; helper lives behind future server caller).
|
|
|
|
**Register / tracking:** ~~Implementation tracking table E2.M2 row needs a NEO-42 paragraph; register Status fields require no change.~~ E2.M2 row updated; register Status fields unchanged. **Done.**
|
|
|
|
## Blocking issues
|
|
|
|
_None._
|
|
|
|
## Suggestions
|
|
|
|
1. ~~**Update E2.M2 implementation-snapshot docs to reflect NEO-42 landing.** Two locations:~~ **Done.**
|
|
- ~~`docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — replace / supplement the line "Still backlog within this module: Slice 3 NEO-42, NEO-43" with a "**NEO-42 landed (prep):** `RefineActivitySkillXpGrant` + `RefineSkillXpConstants` + deny-path factory; awaiting E3.M2 success handler to invoke" paragraph, mirroring the NEO-41 paragraph.~~
|
|
- ~~`docs/decomposition/modules/documentation_and_implementation_alignment.md` — extend the E2.M2 tracking row's snapshot column with the equivalent NEO-42 sentence and add the plan link. This keeps the same pattern NEO-41 set in `0a141ba`-style follow-ups.~~
|
|
|
|
2. ~~**Add an Implementation snapshot to `E3_M2_RefinementAndRecipeExecution.md`.** Status can remain **Planned** in the register (no `CraftRequest` / `RecipeDef` / `CraftResult` yet), but a one-paragraph snapshot under Summary noting "preparatory helper `RefineActivitySkillXpGrant` ready for the future success handler; plan: NEO-42" would help the next implementer find the existing wiring point. Mirrors how NEO-25 / NEO-26 noted partial work without prematurely flipping module status.~~ **Done.**
|
|
|
|
## Nits
|
|
|
|
- ~~**Nit (test 3 assertion strength):** `RefineActivitySkillXpGrantTests.GrantOnSuccessfulCraftOrRefine_WhenRefineDisallowsActivity_ShouldLeaveRefineXpZero` currently does:~~ **Done.** (`Assert.False(totals.ContainsKey("refine"));`)
|
|
|
|
```csharp
|
|
var totals = xpStore.GetXpTotals("dev-local-1");
|
|
_ = totals.TryGetValue("refine", out var refineXp);
|
|
Assert.Equal(0, refineXp);
|
|
```
|
|
|
|
~~Because `TryApplyGrant` returns before `TryApplyXpDelta` on a deny, the `refine` key should never exist in the store. `Assert.False(totals.ContainsKey("refine"));` (or `Assert.False(totals.TryGetValue("refine", out _));`) expresses that semantic exactly and would catch a regression where the store unexpectedly created a 0-valued entry. Current form passes both states.~~
|
|
|
|
- ~~**Nit (helper symmetry with gather call site):** `RefineActivitySkillXpGrant.GrantOnSuccessfulCraftOrRefine` calls `playerId.Trim()` before delegating, but the NEO-41 inline call in `InteractionApi.cs` passes the already-normalized `playerKey` straight through. Harmless either way (store re-normalizes), but picking one direction across both gather and refine hooks keeps callers consistent for whoever wires E3.M2 next.~~ **Done.** (Trim removed; `<remarks>` documents normalized `playerId`.)
|
|
|
|
- **Nit (factory duplication):** `RefineActivityDeniedRegistryWebApplicationFactory` is ~90% identical to `SalvageActivityDeniedRegistryWebApplicationFactory` (only the stub-registry composition and namespace differ). Optional follow-up: extract a small `InMemoryFactoryWithSkillRegistryOverride<TRegistry>` base in `NeonSprawl.Server.Tests` to keep future deny-path factories DRY (combat XP, intrusion, etc.). Not worth blocking on for two factories.
|
|
|
|
- ~~**Nit (acceptance-criteria phrasing):** Plan checkbox "Craft/refine completion **shall** grant skill XP via NEO-38 path" is ticked, but no craft/refine completion path exists yet. The plan acknowledges this in the same bullet ("E3.M2 must call `GrantOnSuccessfulCraftOrRefine` on success … no craft route in repo yet"). Consider rewording to "**Preparatory** helper invokes NEO-38 grant path; E3.M2 success handler wiring tracked as follow-up" so future readers don't misread the box as a fully implemented end-to-end behavior. Optional.~~ **Done.**
|
|
|
|
## Verification
|
|
|
|
- `dotnet test NeonSprawl.sln` — **passed** (**139** tests, including the 3 new `RefineActivitySkillXpGrantTests`).
|
|
- Manual: follow `docs/manual-qa/NEO-42.md` — `POST …/move` → `POST …/skill-progression` with `{skillId: "refine", sourceKind: "activity", amount: 10}` → expect `granted: true`, then re-`POST` with `sourceKind: "book_or_item"` → expect `granted: false`, `reasonCode: "source_kind_not_allowed"`.
|
|
- After E3.M2 lands: confirm a single call to `RefineActivitySkillXpGrant.GrantOnSuccessfulCraftOrRefine` on the craft-success branch (not on deny / cost-failure) and re-verify via `GET …/skill-progression`.
|