From b8e0cce5824ca5f7e6fc6f656b9d7d54328e73de Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 13:33:39 -0400 Subject: [PATCH] NEO-56: add code review for inventory telemetry hook sites --- docs/reviews/2026-05-24-NEO-56.md | 56 +++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 docs/reviews/2026-05-24-NEO-56.md diff --git a/docs/reviews/2026-05-24-NEO-56.md b/docs/reviews/2026-05-24-NEO-56.md new file mode 100644 index 0000000..2092aa1 --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-56.md @@ -0,0 +1,56 @@ +# 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).