neon-sprawl/docs/reviews/2026-05-29-NEO-97.md

Code review — NEO-97 (E5M2-11)

Date: 2026-05-29
Scope: Branch NEO-97-e5m2-11-client-telegraph-hud vs origin/main — commits b0d0c42 … cfda0ac
Base: origin/main

Verdict

Approve with nits

Summary

NEO-97 adds the Epic 5 Slice 2 client half: thin HTTP clients for GET /game/world/npc-runtime-snapshot and GET /game/players/{id}/combat-health, a RefCounted windup display mirror between polls, ~1 Hz combat-active polling wired from main.gd, and three HUD labels (PlayerCombatHpLabel, NpcStateLabel, TelegraphLabel). The implementation follows NEO-85/NEO-32 patterns (injectable HTTP, _busy guard, local tick-down smoothing) and matches kickoff decisions (combat-active gate, locked + incoming rows, interpolated windup). Four GdUnit suites cover parse/URL helpers, windup reconciliation, and combat-active / NPC state label logic via a test harness. Docs (plan, manual QA, README, E5M2 backlog, module + alignment register) are updated. Primary risk is logic duplication between main.gd and the test harness (drift over time) and a stale module risk bullet that still says “no client windup math.” No blocking correctness issues for the documented manual QA flows (lock → cast → telegraph → player HP drop).

Documentation checked

Path	Result
docs/plans/NEO-97-implementation-plan.md	Matches — kickoff decisions (combat-active gate, locked + incoming HUD, interpolate windup, keep prototype_target_markers.gd) reflected in code; reconciliation section accurate; AC checked.
docs/plans/E5M2-prototype-backlog.md (E5M2-11)	Matches — AC checked, landed note present; script name in backlog still says npc_archetype_markers.gd but plan kickoff explicitly adopted prototype_target_markers.gd (documented divergence).
docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md	Matches — NEO-97 landed line correct; Risks + NPC instance row updated for windup display + NEO-91 constants migration.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches — E5.M2 row includes NEO-97 landed with README/plan/manual QA links.
docs/manual-qa/NEO-97.md	Matches — Godot steps cover lock, cast, telegraph countdown, player HP drop, incoming threat row, regressions.
client/README.md	Matches — NPC runtime + telegraph HUD section documents poll gate, labels, tab order, script paths.
docs/decomposition/modules/client_server_authority.md	Matches — read-only GET polling; no client authority over combat/NPC state; poll advances server lazy tick (accepted per plan).

Blocking issues

None.

Suggestions

1. E5.M2 Risks bullet — Update “Telegraph desync: … no client windup math” to “display-only windup tick-down between polls; reconciled on each npc-runtime-snapshot GET” so module docs align with the NEO-97 kickoff decision and npc_runtime_hud_state.gd. Done.

2. E5.M2 NPC instance row — Change “Breaking: Godot constants migrate in NEO-97” to note migration landed in NEO-91; NEO-97 adds HUD poll clients only. Done.

3. Test coverage vs plan — npc_combat_hud_refresh_test.gd covers combat-active bool + NpcStateLabel copy but not TelegraphLabel formatting or timer start/stop (called out in the implementation plan test table). Add at least one telegraph line assertion (locked vs incoming fallback) and, if feasible without heavy scene wiring, a timer-gate test mirroring _update_combat_poll_gate(). Done. Telegraph fallback + local windup expiry + timer mirror tests added.

4. Shared HUD helpers — Combat-active gate, row resolution, and label formatters are duplicated in main.gd (~180 lines) and NpcCombatHudHarness in tests. Extract to a small RefCounted (e.g. npc_combat_hud_helpers.gd) used by both to prevent drift; optional follow-up given NEO-85 precedent of wiring in main.gd. Done. npc_combat_hud_helpers.gd shared by main.gd and tests.

5. Telegraph at zero — _format_telegraph_line can show · 0.0s while the snapshot row still carries activeTelegraph until the next poll. Consider returning empty when display_windup_remaining ≤ 0 so TelegraphLabel shows — between local expiry and server reconcile. Done. format_telegraph_line returns empty when remaining ≤ 0.

6. Runtime sync failure — _on_npc_runtime_sync_failed re-renders from the last good snapshot without surfacing failure on NpcStateLabel / TelegraphLabel (player HP already shows error). Acceptable for prototype; optionally dash NPC/telegraph lines or append a sync-failed hint for QA clarity. Done. _npc_runtime_sync_error appended to NPC/telegraph labels when empty or alongside stale rows.

Nits

• Nit: npc_runtime_client.gd stores _last_snapshot but static npc_row() does not fall back to it (unlike combat_targets_client.target_row()); field is write-only today. Done. Instance npc_row() falls back to _last_snapshot.

• Nit: Duplicate MockHttpTransport inner classes in npc_runtime_client_test.gd and player_combat_health_client_test.gd — fine for now; a shared test helper would reduce copy-paste.

• Nit: main.gd grows ~250 lines for NEO-97; repo policy prefers thin main.gd — track for a future extract when the next HUD story lands. Partially addressed — helpers extracted; poll wiring remains in main.gd.

• Nit: Plan text references serverTimeUtc anchor in npc_runtime_hud_state.gd; implementation uses receive tick + server windupRemainingSeconds only — sufficient for display smoothing; plan wording could be tightened. Done.

Verification

# GdUnit (CI uses Godot 4.6 headless — not available locally in review env)
cd client && ~/godot/Godot_v4.6-stable_linux.x86_64 --headless --path . \
  -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test

# Lint / format (CI)
pip install "gdtoolkit==4.5.0"
gdlint client/scripts client/test
gdformat --check client/scripts client/test

Manual (required before merge): docs/manual-qa/NEO-97.md — server + Godot F5; elite lock → cast → ~7.5 s telegraph window → player HP 75/100; incoming threat row when locking a different NPC while elite holds aggro.