NEO-56: address code review follow-ups
Extend E3.M3 dependency register footnote; expand post-mutation deny hook comments; clarify Deny payload at call sites; strike review items Done.pull/91/head
parent
b8e0cce582
commit
deed0c4314
|
|
@ -48,7 +48,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
|
||||||
| E3.M4 | SinkAndDurabilityLifecycle | E3.M3, E8.M3 | DurabilityState, ItemSinkEvent, RepairCostRule | Pre-production | Planned |
|
| E3.M4 | SinkAndDurabilityLifecycle | E3.M3, E8.M3 | DurabilityState, ItemSinkEvent, RepairCostRule | Pre-production | Planned |
|
||||||
| E3.M5 | EconomyBalancePolicy | E3.M4, E9.M2 | EconomyPolicy, PriceBandRule, FaucetSinkRatio | Pre-production | Planned |
|
| E3.M5 | EconomyBalancePolicy | E3.M4, E9.M2 | EconomyPolicy, PriceBandRule, FaucetSinkRatio | Pre-production | Planned |
|
||||||
|
|
||||||
**E3.M3 note:** Epic 3 **Slice 1** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) → [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56); label **`E3.M3`**. See [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md), [E3_M3_ItemizationAndInventorySchema.md](E3_M3_ItemizationAndInventorySchema.md). **NEO-50** (content + CI), **NEO-51** (server fail-fast load), **NEO-52** (`IItemDefinitionRegistry` + DI), **NEO-53** (`GET /game/world/item-definitions`), **NEO-54** (inventory store + stack/slot rules engine), and **NEO-55** (`GET`/`POST /game/players/{id}/inventory`) moved the register row to **In Progress**; later slices update the alignment table as they land.
|
**E3.M3 note:** Epic 3 **Slice 1** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) → [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56); label **`E3.M3`**. See [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md), [E3_M3_ItemizationAndInventorySchema.md](E3_M3_ItemizationAndInventorySchema.md). **NEO-50** (content + CI), **NEO-51** (server fail-fast load), **NEO-52** (`IItemDefinitionRegistry` + DI), **NEO-53** (`GET /game/world/item-definitions`), **NEO-54** (inventory store + stack/slot rules engine), **NEO-55** (`GET`/`POST /game/players/{id}/inventory`), and **NEO-56** (comment-only `item_created` / `inventory_transfer_denied` telemetry hook sites in `PlayerInventoryOperations`) — Epic 3 Slice 1 backlog **E3M3-01**–**E3M3-07** complete; register row **In Progress** until Slice 2+; later slices update the alignment table as they land.
|
||||||
|
|
||||||
### Epic 4 — World Topology
|
### Epic 4 — World Topology
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -106,3 +106,11 @@
|
||||||
| **Partial stack placement on add** | NEO-54 all-or-nothing add — single `item_created` emit for full requested quantity on `Applied`. | `adopted` |
|
| **Partial stack placement on add** | NEO-54 all-or-nothing add — single `item_created` emit for full requested quantity on `Applied`. | `adopted` |
|
||||||
|
|
||||||
None blocking.
|
None blocking.
|
||||||
|
|
||||||
|
## Decisions (post-review)
|
||||||
|
|
||||||
|
| Topic | Decision |
|
||||||
|
|--------|----------|
|
||||||
|
| **PR scope** | Branch includes chore `3d43b00` (kickoff `AskQuestion` + recommendation rule hardening). Intentional tooling hygiene on the story branch; call out in PR description if reviewers want story-only diff vs `main`. |
|
||||||
|
| **Post-mutation deny hooks** | Expanded to multi-line NEO-49-style blocks with payload fields (`mutationKind=add` / `remove`) per code review. |
|
||||||
|
| **Dependency register** | E3.M3 footnote extended with NEO-56 to match alignment table. |
|
||||||
|
|
|
||||||
|
|
@ -19,13 +19,13 @@ NEO-56 adds **comment-only** E9.M1 telemetry anchor sites in `PlayerInventoryOpe
|
||||||
| [`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/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/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/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/modules/module_dependency_register.md`](../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`](../decomposition/epics/epic_03_crafting_economy.md) | **Matches** — Slice 1 vocabulary `item_created`, `inventory_full` / transfer failures via stable `reasonCode`. |
|
| [`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. |
|
| [`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. |
|
| [`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. |
|
| [`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.
|
Register/tracking: alignment table, E3.M3 module page, and dependency register footnote updated for NEO-56.
|
||||||
|
|
||||||
## Blocking issues
|
## Blocking issues
|
||||||
|
|
||||||
|
|
@ -33,17 +33,17 @@ None.
|
||||||
|
|
||||||
## Suggestions
|
## 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.
|
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.~~ **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.
|
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](../plans/NEO-56-implementation-plan.md) **Decisions (post-review)**; mention in PR body when opening.
|
||||||
|
|
||||||
## Nits
|
## Nits
|
||||||
|
|
||||||
- Nit: Plan allowed a cross-reference in `PlayerInventoryMutationOutcome.cs` **or** `PlayerInventoryReasonCodes.cs`; only ReasonCodes was updated — acceptable (“or” satisfied).
|
- 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: 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.
|
- ~~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
|
## Verification
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -51,7 +51,10 @@ public static class PlayerInventoryOperations
|
||||||
|
|
||||||
if (denyReason is not null)
|
if (denyReason is not null)
|
||||||
{
|
{
|
||||||
// NEO-56 telemetry hook site: `inventory_transfer_denied` + reasonCode (TODO(E9.M1): catalog emit).
|
// --- Telemetry hook site (NEO-56): future E9.M1 catalog event `inventory_transfer_denied` ---
|
||||||
|
// TODO(E9.M1): catalog emit — reasonCode (e.g. inventory_full). Planned payload: playerId, itemId, quantity,
|
||||||
|
// reasonCode, mutationKind=add; optional source when Slice 2+ callers exist. No ingest or ILogger here (comments-only).
|
||||||
|
|
||||||
return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Denied, denyReason, result);
|
return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Denied, denyReason, result);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -121,7 +124,10 @@ public static class PlayerInventoryOperations
|
||||||
|
|
||||||
if (denyReason is not null)
|
if (denyReason is not null)
|
||||||
{
|
{
|
||||||
// NEO-56 telemetry hook site: `inventory_transfer_denied` + reasonCode (TODO(E9.M1): catalog emit).
|
// --- Telemetry hook site (NEO-56): future E9.M1 catalog event `inventory_transfer_denied` ---
|
||||||
|
// TODO(E9.M1): catalog emit — reasonCode (e.g. insufficient_quantity). Planned payload: playerId, itemId, quantity,
|
||||||
|
// reasonCode, mutationKind=remove; optional source when Slice 2+ callers exist. No ingest or ILogger here (comments-only).
|
||||||
|
|
||||||
return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Denied, denyReason, result);
|
return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Denied, denyReason, result);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -132,8 +138,8 @@ public static class PlayerInventoryOperations
|
||||||
{
|
{
|
||||||
// --- Telemetry hook site (NEO-56): future E9.M1 catalog event `inventory_transfer_denied` ---
|
// --- Telemetry hook site (NEO-56): future E9.M1 catalog event `inventory_transfer_denied` ---
|
||||||
// TODO(E9.M1): catalog emit — reasonCode from PlayerInventoryReasonCodes (invalid_quantity, invalid_item, etc.).
|
// TODO(E9.M1): catalog emit — reasonCode from PlayerInventoryReasonCodes (invalid_quantity, invalid_item, etc.).
|
||||||
// Planned payload: playerId, reasonCode, itemId, quantity, mutationKind (add|remove); optional source when Slice 2+ callers exist.
|
// Planned payload: playerId, reasonCode; itemId, quantity, mutationKind (add|remove) at emit call sites (not on Deny signature today).
|
||||||
// No ingest or ILogger here (comments-only).
|
// Optional source when Slice 2+ callers exist. No ingest or ILogger here (comments-only).
|
||||||
|
|
||||||
if (store.TryGetSnapshot(playerId, out var snapshot))
|
if (store.TryGetSnapshot(playerId, out var snapshot))
|
||||||
{
|
{
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue