NEO-153: fix stale issue patch merge and address review findings.
Clear _issue_contract_patch when GET includes the instance row (NEO-122 precedent); wire faction_standing_received for reward re-paint; add regression test; update E7M4 decomposition docs and review strikethroughs.pull/195/head
parent
051aae695b
commit
6a48315ba5
|
|
@ -62,6 +62,11 @@ func setup(
|
|||
_contract_client.connect("contract_issue_failed", Callable(self, "_on_issue_failed"))
|
||||
if is_instance_valid(_item_defs_client) and _item_defs_client.has_signal("definitions_ready"):
|
||||
_item_defs_client.connect("definitions_ready", Callable(self, "_on_item_definitions_ready"))
|
||||
if is_instance_valid(_faction_standing_client):
|
||||
if _faction_standing_client.has_signal("faction_standing_received"):
|
||||
_faction_standing_client.connect(
|
||||
"faction_standing_received", Callable(self, "_on_faction_standing_received")
|
||||
)
|
||||
_render_active_label()
|
||||
_render_issue_feedback_label(ISSUE_IDLE_HINT)
|
||||
_render_reward_label()
|
||||
|
|
@ -155,6 +160,11 @@ func _on_item_definitions_ready(_definitions_by_id: Dictionary) -> void:
|
|||
_render_reward_label()
|
||||
|
||||
|
||||
func _on_faction_standing_received(_snapshot: Dictionary) -> void:
|
||||
if not _last_reward_instance_id.is_empty():
|
||||
_render_reward_label()
|
||||
|
||||
|
||||
func _mark_contract_active_in_session(instance_id: String) -> void:
|
||||
var iid := instance_id.strip_edges()
|
||||
if not iid.is_empty():
|
||||
|
|
@ -192,9 +202,16 @@ func _detect_and_apply_completion_transitions(snapshot: Dictionary) -> void:
|
|||
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 = {}
|
||||
|
||||
|
||||
func _refresh_economy_hud() -> void:
|
||||
|
|
|
|||
|
|
@ -218,3 +218,18 @@ func test_reward_label_populates_on_active_to_completed_transition() -> void:
|
|||
# Assert
|
||||
assert_str(built["reward_label"].text).contains("Contract rewards:")
|
||||
assert_str(built["reward_label"].text).contains("Salvage +15 XP")
|
||||
|
||||
|
||||
func test_completed_get_clears_stale_issue_patch_on_active_label() -> void:
|
||||
# Arrange — simulate successful issue patch still held when encounter-complete GET arrives
|
||||
var built: Dictionary = await _build_controller(
|
||||
NoopHttpTransport.new(), NoopHttpTransport.new()
|
||||
)
|
||||
var issue_row: Dictionary = (_active_snapshot()["contracts"] as Array)[0] as Dictionary
|
||||
built["controller"].set("_issue_contract_patch", issue_row.duplicate(true))
|
||||
built["controller"].set("_tracked_instance_id", INSTANCE_ID)
|
||||
# Act
|
||||
built["controller"].call("_on_contracts_received", _completed_snapshot())
|
||||
# Assert
|
||||
assert_str(built["active_label"].text).contains("completed")
|
||||
assert_str(built["active_label"].text).not_contains(": active —")
|
||||
|
|
|
|||
|
|
@ -71,6 +71,8 @@ Epic 7 **Slice 4** — `contract_issued`, `contract_complete`, reward anomalies.
|
|||
|
||||
**Contract telemetry hooks (NEO-152):** comment-only **`contract_issued`**, **`contract_complete`**, and **`reward_anomaly`** anchor sites in generator/completion ops + shared reward router rollback; [server README — Contract telemetry hooks (NEO-152)](../../../server/README.md#contract-telemetry-hooks-neo-152); plan [NEO-152](../../plans/NEO-152-implementation-plan.md).
|
||||
|
||||
**Contract client HUD (NEO-153):** **`contract_client.gd`**, **`contract_hud_controller.gd`**, **`ContractActiveLabel`** / **`ContractIssueFeedbackLabel`** / **`ContractRewardDeliveryLabel`** — **Shift+C** prototype issue, encounter objective display, completion reward transition; [client README — Contract issue + progress HUD (NEO-153)](../../../client/README.md#contract-issue--progress-hud-neo-153); [`NEO-153` manual QA](../../manual-qa/NEO-153.md); plan [NEO-153](../../plans/NEO-153-implementation-plan.md).
|
||||
|
||||
## Source anchors
|
||||
|
||||
- Master plan: [`neon_sprawl_vision.plan.md`](../../../neon_sprawl_vision.plan.md) — Epic 7.
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -333,10 +333,12 @@ Working backlog for **Epic 7 — Slice 4** ([contract generator](../decompositio
|
|||
|
||||
**Acceptance criteria**
|
||||
|
||||
- [ ] Active contract visible with encounter objective id after issue.
|
||||
- [ ] Completed contract shows reward summary line on HUD.
|
||||
- [x] Active contract visible with encounter objective id after issue.
|
||||
- [x] Completed contract shows reward summary line on HUD.
|
||||
|
||||
**Server counterpart:** [NEO-150](https://linear.app/neon-sprawl/issue/NEO-150), [NEO-152](https://linear.app/neon-sprawl/issue/NEO-152).
|
||||
**Landed (NEO-153):** **`contract_client.gd`**, **`contract_hud_controller.gd`**, **Shift+C** prototype issue, contract HUD labels; [client README — Contract issue + progress HUD (NEO-153)](../../client/README.md#contract-issue--progress-hud-neo-153); [`NEO-153` manual QA](../manual-qa/NEO-153.md); plan [NEO-153](../../plans/NEO-153-implementation-plan.md).
|
||||
|
||||
**Server counterpart:** [NEO-151](https://linear.app/neon-sprawl/issue/NEO-151), [NEO-152](https://linear.app/neon-sprawl/issue/NEO-152).
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -58,9 +58,9 @@ Player can **issue** and **track** an active contract in Godot without Bruno —
|
|||
## Implementation reconciliation (shipped)
|
||||
|
||||
- **`contract_client.gd`:** GET/POST parse v1; **`active_contract_row`**, **`completion_reward_summary`**; separate sync/issue HTTP guards.
|
||||
- **`contract_hud_controller.gd`:** **Shift+C** prototype issue; active/completed label with **`encounterTemplateId`**; deny copy; in-session reward transition + economy refresh.
|
||||
- **`contract_hud_controller.gd`:** **Shift+C** prototype issue; active/completed label with **`encounterTemplateId`**; deny copy; in-session reward transition + economy refresh; issue patch cleared when GET row exists (NEO-122 accept-patch precedent); faction-standing re-paint on reward label.
|
||||
- **`main.tscn` / `main.gd`:** **`ContractClient`** node + three HUD labels; encounter-complete contract refresh hook.
|
||||
- **Tests:** `contract_client_test.gd` (7 cases), `contract_hud_controller_test.gd` (5 cases) — GdUnit green.
|
||||
- **Tests:** `contract_client_test.gd` (7 cases), `contract_hud_controller_test.gd` (6 cases) — GdUnit green.
|
||||
- **Docs:** `client/README.md` contract HUD section; `docs/manual-qa/NEO-153.md`.
|
||||
|
||||
## Technical approach
|
||||
|
|
@ -222,4 +222,4 @@ No new **C#** tests (client-only; server covered by **`ContractApiTests`**). No
|
|||
| Duplicate reward formatter vs shared utility | **Duplicate minimal grant line helpers** inside **`contract_hud_controller.gd`** — avoid coupling to quest controller private methods | `adopted` |
|
||||
| Issue while encounter already completed (server v1 edge) | **Document in manual QA** — prototype flow is issue-then-clear; HUD still shows **`active`** until server completes contract on a *future* matching clear (NEO-149 README edge case) | `adopted` |
|
||||
| Concurrent GET + issue | Separate **`_sync_busy`** / **`_issue_busy`** guards; coalesce pending sync like quest client | `adopted` |
|
||||
| Grant display names before item/faction defs load | Re-render contract reward label on **`definitions_ready`** / faction standing received when summary cached (NEO-122 item-defs re-paint precedent) | `adopted` |
|
||||
| Grant display names before item/faction defs load | Re-render contract reward label on **`definitions_ready`** / **`faction_standing_received`** when summary cached (NEO-122 item-defs re-paint precedent) | `adopted` |
|
||||
|
|
|
|||
|
|
@ -4,10 +4,11 @@
|
|||
**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**
|
||||
~~**Request changes**~~ **Addressed** — blocking patch bug fixed; decomposition docs updated; follow-up tests green.
|
||||
|
||||
## Summary
|
||||
|
||||
|
|
@ -20,9 +21,9 @@ Overall risk is moderate: client-only, but **`_reapply_issue_contract_patch_if_n
|
|||
| 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/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 |
|
||||
|
|
@ -32,39 +33,21 @@ Register/tracking table and E7.M4 module page **should be updated** after merge
|
|||
|
||||
## Blocking issues
|
||||
|
||||
1. **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:
|
||||
|
||||
```gdscript
|
||||
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`**.
|
||||
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** — 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`.
|
||||
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)** — 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.
|
||||
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.
|
||||
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 — only `FactionStandingClient.display_name_for` static is called; remove the field or connect standing signals per suggestion 2.
|
||||
- ~~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.
|
||||
- ~~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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue