neon-sprawl/docs/reviews/2026-06-07-NEO-121.md

Code review — NEO-121 (E7M1-10)

Date: 2026-06-07
Scope: Branch NEO-121-quest-telemetry-hooks vs origin/main — commits 72b98a6 … 705a7cd
Base: origin/main

Verdict

Approve with nits

Summary

NEO-121 adds comment-only E9.M1 telemetry hook sites to QuestStateOperations for Epic 7 Slice 1 events quest_start, quest_step_complete, quest_complete, and quest_accept_denied. Hooks are placed at mutation commit sites (after successful store writes) and accept-deny paths funnel through a new DenyAccept helper (mirroring NEO-56 inventory_transfer_denied). DenyFromProgressSnapshotAccept ensures activate-failure rereads also hit the deny hook. No runtime behavior, wire contract, or API surface changes — only comments plus accept-path deny routing that still delegates to Deny. Documentation is thorough: implementation plan reconciliation, server/README.md event table, E7.M1 module snapshot, alignment register, backlog checkboxes, and docs/manual-qa/NEO-121.md. Server-only infrastructure slice; Godot HUD correctly deferred to NEO-122. Risk is negligible.

Documentation checked

Path	Result
docs/plans/NEO-121-implementation-plan.md	Matches — kickoff decisions adopted (engine-only anchors, deny helper, idempotent complete exclusion); acceptance checklist checked; reconciliation accurate.
docs/plans/E7M1-prototype-backlog.md (E7M1-10)	Matches — acceptance checkboxes checked; landed note + README link.
docs/decomposition/modules/E7_M1_QuestStateMachine.md	Matches — NEO-121 implementation snapshot + Risks and telemetry bullet.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches — E7M1-10 / NEO-121 landed note appended.
docs/decomposition/modules/module_dependency_register.md	Matches — E7M1-10 note appended; row stays Planned until NEO-123 (per plan).
server/README.md	Matches — Quest telemetry hooks subsection with event table and delegation note.
docs/manual-qa/NEO-121.md	Matches — comment-only verification checklist; optional Bruno/regression steps.
Full-stack epic decomposition	N/A — infrastructure-only; plan explicitly documents no client counterpart.

Register / tracking: E7.M1 correctly remains Planned until client capstone NEO-123; E7M1-10 completion noted in register note only — no further status change required.

Blocking issues

(none)

Suggestions

(none)

Nits

1. Nit: Plan technical approach lists optional schemaVersion in quest_start planned payload; the inline comment omits it. Harmless for comment-only work; consider adding for parity when touching the file again. Done. Added optional schemaVersion to quest_start inline comment.

2. Nit: DenyFromProgressSnapshotAccept duplicates DenyFromProgressSnapshot except for DenyAccept vs Deny — intentional funneling, but E9.M1 wiring may want playerId / questId threaded into DenyAccept (comment already documents those fields; implementer will need caller context). Done. Merged into DenyFromProgressSnapshot with optional accept-deny context; DenyAccept now takes playerId and questId at all call sites.

Verification

# Full regression (691 tests passed at review time)
dotnet test NeonSprawl.sln

# Quest-focused subset
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Quest"

Manual (per docs/manual-qa/NEO-121.md):

• Walk QuestStateOperations.cs hook comment blocks and confirm no duplicate hooks in QuestAcceptApi, QuestProgressApi, or QuestObjectiveWiring.
• Optional Bruno sanity in bruno/neon-sprawl-server/quest-progress/ — accept happy path + duplicate deny unchanged.