# Code review — NEO-153 client contract issue + progress HUD

**Date:** 2026-06-28  
**Scope:** Branch `NEO-153-e7m4-10-client-contract-issue-progress-hud-godot` — commits `9d756c2`..`365e577` (6 commits)  
**Base:** `main`  
**Issue:** [NEO-153](https://linear.app/neon-sprawl/issue/NEO-153) — E7M4-10 client contract issue + progress HUD (Godot)  
**Follow-up:** Review findings addressed in follow-up commits (issue-patch merge fix, docs, faction-standing re-paint, regression test).

## Verdict

~~**Request changes**~~ **Addressed** — blocking patch bug fixed; decomposition docs updated; follow-up tests green.

## Summary

This branch delivers the E7M4-10 Godot client slice: `contract_client.gd` (GET list + POST issue with separate sync/issue busy guards), `contract_hud_controller.gd` (Shift+C prototype issue, active/completed label, deny copy, in-session reward transition), thin `main.gd` wiring, scene labels, GdUnit coverage (12 cases), `client/README.md`, and manual QA. The implementation closely follows NEO-122/NEO-131 quest HUD patterns and pairs NEO-151 server HTTP correctly.

Overall risk is moderate: client-only, but **`_reapply_issue_contract_patch_if_needed` always merges a stale issue patch over authoritative GET snapshots**, which can regress `ContractActiveLabel` to **`active`** after the server reports **`completed`** — a primary acceptance-criteria failure. Docs tracking (E7M4 backlog AC, E7.M4 module page, alignment register) is not updated for the landed client slice. GdUnit contract tests pass locally.

## Documentation checked

| Document | Result |
|----------|--------|
| `docs/plans/NEO-153-implementation-plan.md` | **Matches** — shipped reconciliation aligns with code; AC checklist marked complete; kickoff decisions (separate controller, fixed dev seed, encounter-complete refresh) reflected |
| `docs/plans/E7M4-pre-production-backlog.md` | ~~**Partially matches**~~ **Matches** — E7M4-10 AC checked; **Landed (NEO-153)** note added **Done.** |
| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | ~~**Partially matches**~~ **Matches** — **Contract client HUD (NEO-153)** line added **Done.** |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | ~~**Partially matches**~~ **Matches** — E7M4-10 / NEO-153 in E7.M4 row + trailing refs **Done.** |
| `docs/decomposition/modules/contracts.md` | **N/A** — policy doc; no client HUD surface |
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — client displays server snapshots; no client-side issue authority |
| `docs/manual-qa/NEO-153.md` | **Matches** — Godot-only checklist; edge cases documented |
| `client/README.md` | **Matches** — contract HUD subsection with keys, refresh triggers, server deps |

Register/tracking table and E7.M4 module page **should be updated** after merge (same pattern as NEO-152 follow-up).

## Blocking issues

1. ~~**Stale `_issue_contract_patch` overwrites completed contract row**~~ — **Done.** `_reapply_issue_contract_patch_if_needed` now merges only when the instance id is **missing** from the GET snapshot; otherwise clears `_issue_contract_patch` (NEO-122 accept-patch precedent). Regression test `test_completed_get_clears_stale_issue_patch_on_active_label` in `contract_hud_controller_test.gd`.

## Suggestions

1. ~~**Update decomposition docs for landed client slice**~~ — **Done.** `E7_M4_ContractMissionGenerator.md`, `documentation_and_implementation_alignment.md`, `E7M4-pre-production-backlog.md` updated.

2. ~~**Faction-standing reward re-paint (plan § Open questions)**~~ — **Done.** `contract_hud_controller.gd` connects `faction_standing_received` to re-render reward label when a cached summary exists.

3. **Split or label the `chore:` commit** — `fea9928` (warnings-as-errors, dotnet format CI/hooks) is unrelated to NEO-153. Harmless on branch, but consider a separate PR or explicit note in merge description so reviewers do not conflate infra with HUD work. *(Deferred — merge-description note only.)*

## Nits

- ~~Nit: `_faction_standing_client` field is assigned in `setup()` but unused~~ — **Done.** wired for reward label re-paint.
- Nit: Branch includes a one-line server `CompareOrdinal` sort tie-break in `InMemoryContractInstanceStore.cs` — fine as hygiene; no NEO-153 client impact.
- ~~Nit: `test_reward_label_populates_on_active_to_completed_transition` bypasses the issue-patch path; extend after blocking fix so regression is covered.~~ — **Done.** `test_completed_get_clears_stale_issue_patch_on_active_label` added.

## Verification

```bash
# GdUnit (NEO-153 suites)
cd client && godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/contract_client_test.gd -a res://test/contract_hud_controller_test.gd

# Full client suite (CI parity)
cd client && godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test

# Server (unchanged behavior; optional)
dotnet test NeonSprawl.sln
```

**Manual (required before merge):** `docs/manual-qa/NEO-153.md` steps 1–5 — confirm **`ContractActiveLabel`** shows **`completed`** (not **`active`**) after pocket clear and reward line appears.