57 lines
4.7 KiB
Markdown
57 lines
4.7 KiB
Markdown
# Code review — NEO-56 inventory telemetry hook sites
|
||
|
||
**Date:** 2026-05-24
|
||
**Scope:** Branch `NEO-56-item-created-transfer-failure-telemetry-hooks` · commits `786513b`–`dc5141e` vs `origin/main` (includes chore `3d43b00` kickoff rule hardening)
|
||
**Base:** `origin/main`
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-56 adds **comment-only** E9.M1 telemetry anchor sites in `PlayerInventoryOperations`: **`item_created`** before successful add (`Applied`), and **`inventory_transfer_denied`** + **`reasonCode`** on every rule **`Denied`** path (centralized early validation in **`Deny`**, inline markers on post-mutation denies). No runtime behavior, API, or test changes; docs (plan, manual QA, README, E3.M3 slice, alignment table) match kickoff decisions (engine-only, add-success only, single deny event shape, comments-only). Risk is very low for merge. The branch also carries an unrelated **kickoff rule** hardening commit (`story-kickoff.md`, `AGENTS.md`) — fine if intentional, otherwise consider splitting before PR.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-56-implementation-plan.md`](../plans/NEO-56-implementation-plan.md) | **Matches** — engine-only hooks, kickoff clarifications recorded, acceptance checklist complete; comments-only regression noted. |
|
||
| [`docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Matches** — NEO-56 telemetry bullet under **Related implementation slices** with README + manual QA links. |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M3 row cites NEO-56 landed hook sites; no E9.M1 ingest. |
|
||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M3 note lists NEO-50→NEO-56 range but footnote still stops at NEO-55 for “moved the register row” (see Suggestion 1). |
|
||
| [`docs/decomposition/epics/epic_03_crafting_economy.md`](../decomposition/epics/epic_03_crafting_economy.md) | **Matches** — Slice 1 vocabulary `item_created`, `inventory_full` / transfer failures via stable `reasonCode`. |
|
||
| [`docs/manual-qa/NEO-56.md`](../manual-qa/NEO-56.md) | **Matches** — code-review checklist, regression command, optional Bruno sanity. |
|
||
| [`server/README.md`](../../server/README.md) | **Matches** — NEO-56 telemetry subsection under Player inventory. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — hooks remain on server-authoritative engine; no HTTP-layer duplication. |
|
||
|
||
Register/tracking: alignment table and E3.M3 module page updated for NEO-56; dependency register footnote could add NEO-56 for parity.
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. **Dependency register footnote (optional)** — Extend the **E3.M3 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to mention **NEO-56** (telemetry hook sites) alongside NEO-50–NEO-55, matching the alignment table and NEO-55 review pattern.
|
||
|
||
2. **PR scope clarity (optional)** — Commit `3d43b00` (kickoff `AskQuestion` + recommendation rule) is unrelated to NEO-56 inventory hooks. If the PR should be story-scoped only, cherry-pick or split; if bundled tooling hygiene is intentional, call it out in the PR description.
|
||
|
||
## Nits
|
||
|
||
- Nit: Plan allowed a cross-reference in `PlayerInventoryMutationOutcome.cs` **or** `PlayerInventoryReasonCodes.cs`; only ReasonCodes was updated — acceptable (“or” satisfied).
|
||
|
||
- Nit: Post-mutation **`Denied`** hooks (lines ~54, ~124 in `PlayerInventoryOperations.cs`) use a one-line marker; **`Deny`** and **`item_created`** use fuller multi-line blocks (NEO-49 style). Consistent enough with NEO-30 deny precedent; optional payload-field notes on post-mutation paths would mirror **`Deny`** for E9.M1 implementers.
|
||
|
||
- Nit: **`Deny`** hook comments list planned payload fields `itemId`, `quantity`, `mutationKind`, but the method signature does not carry those today — fine for comments-only; E9.M1 will likely extend **`Deny`** or pass a context struct when wiring emit.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
cd server
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj 2>/dev/null || dotnet test NeonSprawl.sln
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerInventory"
|
||
```
|
||
|
||
Manual: walk [`docs/manual-qa/NEO-56.md`](../manual-qa/NEO-56.md) code-review checklist (confirm no hooks in `PlayerInventoryApi.cs`, `TODO(E9.M1)` present, no new logging).
|