neon-sprawl/docs/reviews/2026-06-28-NEO-152.md

4.4 KiB
Raw Permalink Blame History

Code review — NEO-152 contract telemetry hook sites (comment-only)

Date: 2026-06-28
Scope: Branch NEO-152-e7m4-09-contract-telemetry-hook-sites-comment-only — commits 6e5ea78..0d089a6 (2 commits)
Base: main
Issue: NEO-152 — E7M4-09 contract telemetry hook sites (comment-only)

Follow-up: Review suggestions addressed in docs-only follow-up commit.

Verdict

Approve with nits

Summary

This branch anchors future E9.M1 catalog events on the server-authoritative contract path with comment-only hook sites and no runtime behavior change. contract_issued and economy-deny reward_anomaly land in ContractGeneratorOperations.TryIssue; contract_complete extends the NEO-149 partial block to full NEO-121 style; shared-router reward_anomaly documents partial-apply rollback on CompensatingRevertAll, and the NEO-130 reward_delivery comment is updated for contract SourceKind.

Hook placement follows NEO-121/130/141 precedents (engine-only, TODO(E9.M1), no API-layer duplicates). Docs (implementation plan, README subsection, E7.M4 module, alignment register, E7M4 backlog) are updated. Full suite passes (933 tests). Risk is negligible — comments only; no wire or ingest change.

Documentation checked

Document Result
docs/plans/NEO-152-implementation-plan.md Matches — AC checklist complete; shipped reconciliation aligns with diff; kickoff decisions (engine-only, no duplicate reward_delivery, economy deny at orchestrator) reflected in code
docs/plans/E7M4-pre-production-backlog.md Matches — E7M4-09 AC checked; landed note + README/plan links
docs/decomposition/modules/E7_M4_ContractMissionGenerator.md Matches — NEO-152 telemetry hooks line added after NEO-151 HTTP
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E7.M4 row includes E7M4-09 / NEO-152 body text + explicit NEO-152 plan in trailing refs
docs/decomposition/modules/module_dependency_register.md Not re-read in diff — no register change required; E7.M4 already tracked
server/README.md MatchesContract telemetry hooks (NEO-152) subsection with event table + NEO-130 cross-link
Client counterpart (NEO-153) N/A — correctly out of scope; plan does not claim player-visible slice complete

Register/tracking table updates are included in this branch.

Blocking issues

None.

Suggestions

  1. Add NEO-152 plan link to alignment register footer — The E7.M4 row body cites E7M4-09 / NEO-152, but the pipe-separated trailing refs list [NEO-144][NEO-151] then label E7.M4 on NEO-144NEO-154 without [NEO-152 plan]. Add for parity with adjacent landed stories. Done. Added [NEO-152 plan] to alignment register trailing refs.

  2. Note method-level vs call-site rollback anchor in plan — Plan §4 C2 and the files table say stub comments at CompensatingRevertAll invocations in TryDeliverBundle; implementation uses a single block on CompensatingRevertAll (four call sites). Functionally equivalent and DRY (similar to NEO-121 central Deny); optional one-line note in plan Implementation reconciliation that the adopted anchor is the private method. Done. Plan reconciliation, §1 (shipped state), §4 C2, README table, and files table updated.

Nits

  • Nit: reward_anomaly at economy validation deny is semantically a lint/cap deny rather than a delivery rollback; naming matches backlog/NEO-150 deferral — no change needed, but E9.M1 wiring should treat reasonCode distinctly from router rollback anomalies.
  • Nit: Plan §1 Current state table still describes pre-ship “no hook” state; Implementation reconciliation is authoritative — optional cleanup of §1 on merge to past tense. Done. §1 renamed to Shipped state (post-NEO-152) with past-tense rows.

Verification

dotnet test NeonSprawl.sln

Manual (optional — comment-only):

  • Open ContractGeneratorOperations.cs, ContractCompletionOperations.cs, and RewardRouterOperations.cs; confirm TODO(E9.M1) markers, planned payload fields, and no new log/ingest calls.
  • Confirm ContractIssueApi / ContractListApi have no NEO-152 hooks (engine-only).