don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-24-NEO-56.md

4.9 KiB
Raw Permalink Blame History

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 Matches — engine-only hooks, kickoff clarifications recorded, acceptance checklist complete; comments-only regression noted.
docs/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 Matches — E3.M3 row cites NEO-56 landed hook sites; no E9.M1 ingest.
docs/decomposition/modules/module_dependency_register.md Matches — E3.M3 footnote includes NEO-56 telemetry hooks; Slice 1 backlog E3M3-01–07 noted complete.
docs/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 Matches — code-review checklist, regression command, optional Bruno sanity.
server/README.md Matches — NEO-56 telemetry subsection under Player inventory.
docs/decomposition/modules/client_server_authority.md Matches — hooks remain on server-authoritative engine; no HTTP-layer duplication.

Register/tracking: alignment table, E3.M3 module page, and dependency register footnote updated for NEO-56.

Blocking issues

None.

Suggestions

  1. Dependency register footnote (optional) — Extend the E3.M3 note in 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. Done.

  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. Done. — documented in NEO-56 plan Decisions (post-review); mention in PR body when opening.

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. Done. — multi-line blocks with payload fields on add/remove post-mutation denies.

  • 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. Done. — comment clarifies emit call sites supply itemId/quantity/mutationKind.

Verification

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 code-review checklist (confirm no hooks in PlayerInventoryApi.cs, TODO(E9.M1) present, no new logging).