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-04-30-NEO-30.md

3.6 KiB
Raw Permalink Blame History

Code review — NEO-30 (cast funnel telemetry hooks)

Date: 2026-04-30

Scope: Branch NEO-30-cast-funnel-telemetry-hooks; head b547878 (subject: NEO-30: cast funnel telemetry hook sites and docs). Review vs origin/main (implicit).

Base: origin/main

Follow-up: Suggestions and nits below are done (strikethrough + Done.); register row and XML doc tweaks landed in the same change-set.

Verdict

Approve

Summary

The branch delivers NEO-30 as agreed: comment-only telemetry hook sites on the authoritative cast handler (AbilityCastApi), explicit exclusion of HTTP 400/404 from Slice 3 ability_cast_denied JSON semantics, client/doc cross-links, alignment and module snapshot updates, a thin manual QA checklist, and Bruno meta touch for pre-commit pairing. No runtime behavior change beyond documentation strings; risk is low.

Documentation checked

Path Result
docs/plans/NEO-30-implementation-plan.md Matches — kickoff decisions (comment-only server hooks, no new server logging), file list, acceptance criteria, and technical approach align with the diff.
docs/decomposition/modules/E1_M4_AbilityInputScaffold.md Matches — snapshot and E1M4-05 row cite NEO-30, hook anchors, and E9.M1 deferral.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E1.M4 row documents NEO-30 telemetry hook sites.
docs/decomposition/modules/module_dependency_register.md Matches (post-review) — E1.M4 note now includes NEO-28 + NEO-30 with plan links.
docs/decomposition/modules/E9_M1_TelemetryEventSchema.md N/A — deferred per plan; TODO(E9.M1) in code matches.
docs/decomposition/modules/client_server_authority.md / contracts.md Matches — no change to authority or wire contracts; comments reinforce server as authoritative deny/reason source.
docs/manual-qa/NEO-30.md Matches — concrete checks for comments, 400/404 exclusion, and shared NEO-28/31 vocabulary.

Register / tracking table: E1.M4 narrative note updated to include NEO-30 (and NEO-28) with plan links — see Suggestions resolution above.

Blocking issues

(none)

Suggestions

  1. Module dependency register (optional): Add NEO-30 to the E1.M4 note paragraph in docs/decomposition/modules/module_dependency_register.md so readers who start from the register see hook-site documentation alongside NEO-29/31/28 — mirrors the alignment table and avoids “only in alignment doc” drift. Done. E1.M4 note now includes NEO-28 and NEO-30 with plan links (module_dependency_register.md).

Nits

  1. Nit: AbilityCastApi class <remarks> uses <c>AbilityCastResponse.reasonCode</c> (JSON casing). C# readers may expect ReasonCode; harmless for intent. Done. Remarks now use <see cref="AbilityCastResponse.ReasonCode"/> and note wire JSON reasonCode (AbilityCastApi.cs).

  2. Nit: AbilityCastApiTests <see cref="AbilityCastApi.MapAbilityCastApi"/> targets an extension method; some doc generators are picky. If XML doc warnings appear in CI later, switch to a fully qualified cref or drop the see to the containing type only. Done. <see> targets AbilityCastApi only; extension name in plain <c>MapAbilityCastApi</c> (AbilityCastApiTests.cs).

Verification

  • dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~AbilityCastApiTests" — 14 passed (2026-04-30).
  • Execute docs/manual-qa/NEO-30.md for Godot Output / HUD vocabulary before merge if this is the final story handoff.