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-05-25-NEO-86.md

5.6 KiB
Raw Blame History

Code review — NEO-86 playable tab-target combat capstone (Godot)

Date: 2026-05-25
Scope: Branch NEO-86-playable-combat-capstone · commits 8190614–b97cace vs origin/main
Base: origin/main

Verdict

Approve with nits

Summary

NEO-86 closes E5M1-12 and Epic 5 Slice 1 client acceptance: a capstone manual QA script, README integration checklist, GigProgressionClient mirroring SkillProgressionClient, GigXpLabel under the economy HUD, and defeat-triggered GET …/gig-progression refresh wired from _on_cast_result_received. No server changes; gig XP remains server-authoritative (NEO-44 grant + GET reconcile). Five new GdUnit cases pass locally. Residual risk is low — thin-client wiring atop NEO-85 combat HUD; brief lag between defeat cast feedback and gig label update until GET completes is an adopted trade-off (same pattern as combat-target refresh).

Documentation checked

Document Result
docs/plans/NEO-86-implementation-plan.md Matches — kickoff decisions (Godot gig row, docs-first + gap-fill, fresh-server baseline), scope, acceptance checklist, and reconciliation align with shipped code.
docs/plans/E5M1-prototype-backlog.md · E5M1-12 Matches — AC checked; landed note cites gig client, manual QA, README.
docs/decomposition/modules/E5_M1_CombatRulesEngine.md Matches — Status Ready; E5M1-12 (NEO-86) slice added; Slice 1 client capstone complete.
docs/decomposition/modules/module_dependency_register.md Matches — E5.M1 note cites NEO-86 landed + capstone manual QA link.
docs/decomposition/modules/documentation_and_implementation_alignment.md · E5.M1 Partially matches — row updated with NEO-86 landed + Slice 1 client capstone note, but Status column still In Progress while E5_M1_CombatRulesEngine.md header is Ready (see Suggestion 1).
docs/decomposition/modules/client_server_authority.md Matches — gig XP from server GET only; defeat refresh triggered by wire targetDefeated, not client-side XP math.
client/README.md Partially matches — end-to-end combat loop section and test scope list are good; NEO-85 cross-link path is wrong (see Suggestion 2).
docs/manual-qa/NEO-86.md Matches — single-session Godot script, combat math table, economy HUD toggle step, no Bruno/curl; acceptance boxes intentionally unchecked pre-human QA.
Full-stack epic decomposition Matches — client capstone with Godot manual QA; not Bruno-only prototype proof.

Blocking issues

None.

Suggestions

  1. E5.M1 alignment status vs module doc — E5_M1_CombatRulesEngine.md header is Ready with E5M1-01–12 landed; documentation_and_implementation_alignment.md E5.M1 row still In Progress. Promote to Ready (or add a one-line note why server slice remains in progress) so register, module page, and alignment table stay consistent.

  2. Broken NEO-85 README link — In client/README.md § End-to-end combat loop, [NEO-85](NEO-85.md) resolves relative to client/ and 404s. Use [NEO-85](../docs/manual-qa/NEO-85.md) like the NEO-28/NEO-31 links in the same line.

  3. Schema-mismatch HTTP test gap — Plan §Tests lists invalid schema → progression_sync_failed for gig_progression_client_test.gd; implementation covers parse unit test + 404 only. Add a mock 200 with schemaVersion: 2 (or missing mainGigId) asserting progression_sync_failed with the schema mismatch reason — parity with combat_targets_client_test.gd test_invalid_schema_emits_sync_failed.

  4. Deny-path refresh test gap — Plan §Tests lists deny cast must not trigger gig sync; gig_feedback_refresh_test.gd covers defeat accept and non-defeat accept only. Add a third case with accepted: false and assert gig_sync_calls == 0.

Nits

  • Nit: GigRefreshHarness duplicates defeat-gated refresh logic from main.gd rather than testing main.gd directly — matches combat_feedback_refresh_test.gd / craft_feedback_refresh_test.gd precedent; acceptable for prototype scope.

  • Nit: Plan describes gig_row using a cached snapshot when the second arg is omitted; client has no internal cache — main.gd passes _last_gig_snapshot explicitly. Behavior is correct; plan wording could note cache lives in main.gd.

  • Nit: Between defeat cast accept and gig GET completion, CastFeedbackLabel shows authoritative defeat copy while GigXpLabel may still show pre-defeat XP until GET lands — same event-driven lag as combat HP label; optional future polish could show interim copy from cast resolution if UX matters.

Verification

cd /home/don/neon-sprawl/client
godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \
  -a res://test/gig_progression_client_test.gd \
  -a res://test/gig_feedback_refresh_test.gd

GdUnit (local run): 5/5 passed (3 + 2 suites).

Manual: docs/manual-qa/NEO-86.md — fresh server restart, Godot F5, Tab lock prototype_target_alpha, four prototype_pulse casts, verify GigXpLabel breach: L1 · 25 xp, deny on defeated target, economy HUD collapse step 9.