6.5 KiB

Raw Blame History

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 — E7M4-10 client contract issue + progress HUD (Godot)

Verdict

Request changes

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 — E7M4-10 section still has unchecked AC `[ ]`; no Landed (NEO-153) note (contrast E7M4-09 / NEO-152)
`docs/decomposition/modules/E7_M4_ContractMissionGenerator.md`	Partially matches — server slices through NEO-152 documented; no client HUD line for E7M4-10 / NEO-153 (NEO-122 precedent on E7.M1)
`docs/decomposition/modules/documentation_and_implementation_alignment.md`	Partially matches — E7.M4 row ends at E7M4-09 / NEO-152; E7M4-10 / NEO-153 not in body or trailing refs
`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

Blocking issues

Stale _issue_contract_patch overwrites completed contract row — In contract_hud_controller.gd, _on_contracts_received calls _reapply_issue_contract_patch_if_needed() after copying the GET snapshot. That helper always merges _issue_contract_patch (set on successful issue with status: active) and never clears it. On encounter-complete refresh, the server returns status: completed, but the merge replaces that row with the stale active patch. ContractActiveLabel then shows active instead of completed (AC failure). Quest HUD avoids this in _reapply_accept_quest_patch_if_needed: merge only while the server row is still not_started, else _accept_quest_patch = {}. Contract HUD should adopt the same pattern — e.g. merge only when the instance id is missing from the GET snapshot, otherwise clear the patch:

func _reapply_issue_contract_patch_if_needed() -> void:
	if _issue_contract_patch.is_empty():
		return
	var patch_id := str(_issue_contract_patch.get("contractInstanceId", "")).strip_edges()
	if patch_id.is_empty():
		return
	var row: Dictionary = ContractClient.contract_row(patch_id, _last_snapshot)
	if row.is_empty():
		_last_snapshot = ContractClient.merge_contract_row_into_snapshot(
			_last_snapshot, _issue_contract_patch
		)
	else:
		_issue_contract_patch = {}

Add a GdUnit case: issue patch present → second _on_contracts_received with completed snapshot → active label contains completed, not active.

Suggestions

Update decomposition docs for landed client slice — After fixing the patch bug: add E7M4-10 / NEO-153 to E7_M4_ContractMissionGenerator.md (client HUD + README/manual QA links, mirroring NEO-151/152 server lines); extend E7.M4 row in documentation_and_implementation_alignment.md with [NEO-153 plan] in trailing refs; check E7M4-10 AC and add Landed (NEO-153) in E7M4-pre-production-backlog.md.
Faction-standing reward re-paint (plan § Open questions) — Plan adopted re-render on definitions_ready and faction standing received when summary is cached. Controller connects definitions_ready but _faction_standing_client is stored and never wired (rep grant display names would stay raw if a future contract bundle includes reputationGrants). Low impact for prototype bundle (no rep), but align with plan or note deferral in plan reconciliation.
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.

Nits

Nit: _faction_standing_client field is assigned in setup() but unused — only FactionStandingClient.display_name_for static is called; remove the field or connect standing signals per suggestion 2.
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.

Verification

# 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.

6.5 KiB Raw Blame History Unescape Escape