From 6cd2c0cdda66af942cb394e6e038fc41a154159c Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 19:08:26 -0400 Subject: [PATCH] NEO-73: add code review for client gather feedback branch. --- docs/reviews/2026-05-24-NEO-73.md | 73 +++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 docs/reviews/2026-05-24-NEO-73.md diff --git a/docs/reviews/2026-05-24-NEO-73.md b/docs/reviews/2026-05-24-NEO-73.md new file mode 100644 index 0000000..d3c6615 --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-73.md @@ -0,0 +1,73 @@ +# Code review — NEO-73 client gather feedback on interact + +**Date:** 2026-05-24 +**Scope:** Branch `NEO-73-e3m1-client-gather-feedback-on-interact` · commits `4682db4`–`b52428e` vs `origin/main` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** — implementation matches the adopted plan and E3S5-02 acceptance criteria; GdUnit suites pass. Non-blocking gaps: alignment register still lists NEO-73 as planned, and `_pending_gather_feedback` can stick on interact HTTP failures until a later inventory refresh. + +## Summary + +The branch adds **`skill_progression_client.gd`**, **`prototype_interactable_picker.gd`**, gather/skill HUD labels, **`interaction_result_received`** on **`interaction_request_client.gd`**, and orchestration in **`main.gd`** (nearest-node **R**, inventory delta on **`GatherFeedbackLabel`**, salvage row on **`SkillProgressionLabel`**, auto-refresh after successful gather). Four new GdUnit suites extend interaction and client coverage; manual QA and plan reconciliation are present. Design follows NEO-72/NEO-28 patterns (thin HTTP clients, `_busy` guard, authority config copy, HUD error lines). **`main.gd`** grows by ~150 lines but the picker and skill client are appropriately extracted. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-73-implementation-plan.md`](../plans/NEO-73-implementation-plan.md) | **Matches** — kickoff decisions (nearest **R**, inventory delta copy, dedicated skill label) reflected; acceptance checklist and reconciliation complete. | +| [`docs/plans/E3S5-client-prototype-backlog.md`](../plans/E3S5-client-prototype-backlog.md) · **E3S5-02** | **Matches** — acceptance criteria checked; scope/out-of-scope honored. | +| [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Partially matches** — cites NEO-73 as Slice 5 client backlog but no “landed” client note yet (plan §8 deferred to merge). | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Partially matches** — E3.M1 row still says **Slice 5 client (planned): gather feedback HUD NEO-73**; should flip to **landed** on merge. E3.M3 row still lists NEO-73 under “remaining.” | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — client displays outcomes; gather resolution stays server-side via **`POST …/interact`**. | +| [`docs/manual-qa/NEO-73.md`](../manual-qa/NEO-73.md) | **Matches** — four-node **R** walkthrough, depletion deny, terminal does not overwrite gather line, no manual **I**. | +| [`client/README.md`](../../client/README.md) gather feedback section | **Matches** — endpoints, **R** nearest-node rule, HUD labels, boot hydrate. Automated-tests **Scope** bullet still omits NEO-73 suites (see Suggestions). | +| Full-stack epic decomposition | **Matches** — E3S5-02 **client** counterpart for server NEO-63; Godot manual QA present; does not claim Slice 5 capstone (NEO-75) complete. | + +## Blocking issues + +(none) + +## Suggestions + +1. **Update alignment register + E3.M1 module note on merge** — Plan §8 explicitly deferred [`documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) E3.M1 row and E3.M1 client slice note until implementation completes. Add a **NEO-73 landed** clause (gather HUD, **`skill_progression_client`**, four-node **R** picker) and move NEO-73 off E3.M3 “remaining Slice 5” list. + +2. **Clear `_pending_gather_feedback` when interact HTTP fails** — `InteractionRequestClient` only emits **`interaction_result_received`** on parsed **200**; non-200 paths print and return. `main.gd` sets **`_pending_gather_feedback = true`** in **`_forward_interact_resource_nearest`** before POST. If the request fails (500, malformed body, transport error), the flag stays set and a later **`inventory_received`** (e.g. manual **I**) can run **`_finalize_pending_gather_feedback`** with a stale **`_gather_pre_scrap_qty`**, painting a spurious gather delta. Clear the flag on non-200 (new signal or connect to a failure hook) or reset in **`_on_interaction_result_for_gather`** only after a matching success/deny **200**. + +3. **`client/README.md` automated tests Scope** — Add **`skill_progression_client.gd`**, **`prototype_interactable_picker.gd`**, and extended **`interaction_request_client_test.gd`** to the Scope bullet list (same discoverability fix as NEO-72). + +4. **GdUnit: deny path for gather refresh** — `gather_feedback_refresh_test.gd` only asserts the allow → dual-refresh path. A deny mock asserting **zero** inventory/skill sync calls would lock the “no refresh on **`node_depleted`**” contract. + +## Nits + +- Nit: **`Gather: ok — inventory refresh failed`** (`main.gd` **`_on_inventory_sync_failed`**) reads as success; consider **`Gather: inventory refresh failed`** for clarity. +- Nit: Rapid **R** while a gather POST is in flight overwrites **`_gather_pre_scrap_qty`**; delta can be wrong if two responses complete out of order. Prototype-acceptable; last-wins POST coalescing already exists on the interaction client. +- Nit: **`KNOWN_RESOURCE_NODE_IDS`** in **`prototype_interactable_picker.gd`** duplicates the frozen four-node table — intentional catalog-empty fallback; keep in sync with **`E3_M1`** if ids ever change (they are frozen). + +## Verification + +GdUnit (verified on review): + +```bash +cd client +godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \ + -a res://test/skill_progression_client_test.gd \ + -a res://test/gather_feedback_refresh_test.gd \ + -a res://test/prototype_interactable_picker_test.gd \ + -a res://test/interaction_request_client_test.gd +``` + +**14/14** passed; no `PackedStringArray`/`PackedByteArray` header conversion errors on **`skill_progression_client.gd`**. + +Manual (required before merge — real **`HTTPRequest`** + four nodes): + +1. `cd server/NeonSprawl.Server && dotnet run` +2. Godot **F5** — follow [`docs/manual-qa/NEO-73.md`](../manual-qa/NEO-73.md) (boot salvage row, **R** at each anchor without **I**, depletion deny, terminal **E** leaves gather label unchanged). + +Optional lint (pre-push hook parity): + +```bash +gdlint client/scripts client/test +gdformat --check client/scripts client/test +```