diff --git a/docs/reviews/2026-06-17-NEO-142.md b/docs/reviews/2026-06-17-NEO-142.md new file mode 100644 index 0000000..7415676 --- /dev/null +++ b/docs/reviews/2026-06-17-NEO-142.md @@ -0,0 +1,54 @@ +# Code review — NEO-142 (E7M3-10) + +**Date:** 2026-06-17 +**Scope:** Branch `NEO-142-client-faction-standing-gate-hud` — commits `fb9a8ab` … `e396228` vs `main` +**Base:** `main` + +## Verdict + +**Approve with nits** + +## Summary + +This branch lands **E7M3-10**: Godot client faction standing HUD and readable **`faction_gate_blocked`** accept feedback, wired through existing quest HUD patterns (NEO-122 / NEO-131). New **`faction_standing_client.gd`** mirrors **`skill_progression_client.gd`** (GET parse, pending-sync coalesce, 404 handling). **`quest_hud_controller.gd`** renders **`FactionStandingLabel`**, refreshes standing on boot + in-session completion transitions, formats **`reputationGrants`** reward lines, and builds readable gate deny copy from cached **`factionGateRules`** + last standing snapshot. **`quest_definitions_client.gd`** adds **`faction_gate_rules_for`**. GdUnit coverage spans client parse/sync, HUD render/deny/refresh, and operator-chain rep reward line. Docs: implementation plan, **`client/README.md`** faction subsection, **`docs/manual-qa/NEO-142.md`**. The branch also bundles **NEO-141 story-end cleanup** (remove **`docs/manual-qa/NEO-141.md`**, plan/README wording) and a small unrelated **`RewardRouterOperations`** collection-expression refactor. **24** targeted GdUnit tests pass locally. Low merge risk — read-only client projections; no server contract changes beyond doc/chore. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-142-implementation-plan.md` | **Partially matches** — kickoff decisions, file list, technical approach, and reconciliation bullets align with the shipped diff; top **Acceptance criteria checklist** and **Implementation reconciliation** header still show in-progress / unchecked (update at story end). | +| `docs/plans/E7M3-pre-production-backlog.md` | **Partially matches** — E7M3-10 scope bullets match; E7M3-10 AC boxes still unchecked (expected until manual QA / story end). NEO-141 landed note updated (manual QA link removed). | +| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Partially matches** — module intent and contracts satisfied by client read/HUD; Status line still lists NEO-142 as pending slice (→ **E7M3-11**); should note **E7M3-10 landed** after merge. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M3 row ends at NEO-141; no NEO-142 client HUD landed note yet. | +| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — no register row edit required beyond module doc / alignment table. | +| `docs/manual-qa/NEO-142.md` | **Matches** — component checklist, honest gate-deny testing note (operator chain grants rep atomically; optional dev fixture for live deny). | +| `client/README.md` | **Matches** — **Faction standing + gate feedback HUD (NEO-142)** subsection documents GET endpoints, label placement, refresh triggers, and error pattern. Does not yet include **End-to-end faction reputation loop** capstone (E7M3-11 / NEO-143 scope per backlog). | +| `docs/plans/NEO-141-implementation-plan.md` | **Matches** — bundled story-end manual QA removal consistent with NEO-140 precedent. | + +## Blocking issues + +None. + +## Suggestions + +1. **Update decomposition tracking when NEO-142 closes.** After manual QA, extend **`E7_M3_FactionReputationLedger.md`** Status and **`documentation_and_implementation_alignment.md`** E7.M3 row with **E7M3-10 landed (NEO-142)** — client faction standing GET + HUD + gate deny + rep reward lines; link **`client/README.md`** faction subsection and **`docs/manual-qa/NEO-142.md`**. + +2. **Reconcile plan AC at story end.** Check off **`docs/plans/NEO-142-implementation-plan.md`** acceptance criteria and E7M3-10 backlog AC after Godot manual verification; mark **Implementation reconciliation** complete (same pattern as NEO-140/141 reviews). + +3. **PR description should call bundled commits.** If opening one PR from this branch, mention NEO-142 HUD work plus NEO-141 manual-QA removal and the **`RewardRouterOperations`** collection-expression chore so reviewers are not surprised by server diff noise. + +## Nits + +- Nit: **`quest_hud_controller.gd`** **`_faction_display_name`** probes an instance **`display_name_for`** on **`FactionStandingClient`**, but the script exposes **`display_name_for`** as **`static`** only — the instance branch is likely dead; fallback static call is correct either way. +- Nit: **`_title_case_token`** is duplicated in **`faction_standing_client.gd`** and **`quest_hud_controller.gd`** — acceptable for prototype; consolidate only if a shared helper already exists elsewhere. +- Nit: No dedicated GdUnit for **`quest_definitions_client.faction_gate_rules_for`** — gate rules are exercised indirectly via **`faction_standing_hud_test.gd`**; optional direct parse test if the helper grows. +- Nit: Brief standing-label staleness on the same frame as completion (reward label shows **`+15 rep`** before faction GET returns) is adopted plan behavior; manual QA step 4 should confirm label catches up within one round-trip. + +## Verification + +```bash +cd /Users/don/neon-sprawl/client && godot --headless --path . -s addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode \ + -a test/faction_standing_client_test.gd -a test/faction_standing_hud_test.gd -a test/quest_reward_hud_test.gd +``` + +Manual verification: **`docs/manual-qa/NEO-142.md`** — fresh server + Godot **F5**; operator chain → **`Grid Operators: 15`** + reward rep line; optional dev fixture for live **`faction_gate_blocked`** deny copy.