49 lines
3.3 KiB
Markdown
49 lines
3.3 KiB
Markdown
# 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`
|
||
|
||
## 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` | **Partially matches** — E1.M4 remains **In Progress** (correct); the long **E1.M4 note** still summarizes NEO-29/31/28 but does not mention NEO-30 (optional discoverability; not a plan requirement). |
|
||
| `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:** No mandatory row edit beyond what landed in `documentation_and_implementation_alignment.md`; optional follow-up is a one-line NEO-30 mention in the register’s E1.M4 narrative note for consistency.
|
||
|
||
## 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.
|
||
|
||
## Nits
|
||
|
||
1. **Nit:** `AbilityCastApi` class `<remarks>` uses `<c>AbilityCastResponse.reasonCode</c>` (JSON casing). C# readers may expect `ReasonCode`; harmless for intent.
|
||
|
||
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.
|
||
|
||
## 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.
|