diff --git a/docs/reviews/2026-05-29-NEO-97.md b/docs/reviews/2026-05-29-NEO-97.md new file mode 100644 index 0000000..ca7e362 --- /dev/null +++ b/docs/reviews/2026-05-29-NEO-97.md @@ -0,0 +1,68 @@ +# 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` | **Partially matches** — NEO-97 landed line under Related implementation slices is correct; **Risks** bullet still says “no client windup math” (conflicts with kickoff interpolate decision); NPC instance row still says “Breaking: Godot constants migrate in NEO-97” though constants landed in NEO-91. | +| `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`. + +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. + +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()`. + +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`. + +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. + +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. + +## 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. + +- 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. + +- 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. + +## Verification + +```bash +# 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`](../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.