From d5fc648dd62198973134114b82e999105c848d00 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 25 May 2026 14:00:53 -0400 Subject: [PATCH] NEO-86: Add code review for combat capstone branch. --- docs/reviews/2026-05-25-NEO-86.md | 62 +++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 docs/reviews/2026-05-25-NEO-86.md diff --git a/docs/reviews/2026-05-25-NEO-86.md b/docs/reviews/2026-05-25-NEO-86.md new file mode 100644 index 0000000..a5021f9 --- /dev/null +++ b/docs/reviews/2026-05-25-NEO-86.md @@ -0,0 +1,62 @@ +# 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`](../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`](../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`](../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`](../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`](../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`](../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`](../../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`](../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 + +```bash +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`](../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.