neon-sprawl/docs/reviews/2026-05-25-NEO-44.md

# Code review — NEO-44 gig XP on combat defeat

**Date:** 2026-05-25  
**Scope:** Branch `NEO-44-gig-xp-combat-defeat` · commits `30d379c`–`71a6761` vs `origin/main`  
**Base:** `origin/main`

## Verdict

**Approve with nits**

## Summary

NEO-44 delivers **E5M1-10**: when **`AbilityCastApi`** accepts a cast with **`combatResolution.targetDefeated === true`**, the server grants **25** gig XP to prototype main gig **`breach`** via **`CombatDefeatGigXpGrant`** and a dedicated **`IPlayerGigProgressionStore`** (in-memory + Postgres **`V007`**), explicitly **not** calling **`SkillProgressionGrantOperations`**. A read-only **`GET /game/players/{id}/gig-progression`** snapshot (NEO-37 gate pattern) exposes **`mainGigId`**, fixed **`level: 1`**, and accumulating **`xp`**. Integration tests cover GET 404/default, grant helper, defeat chain (4th pulse +25, 5th no delta, skill snapshot unchanged), and optional Postgres persistence; Bruno `gig-progression/` + **`docs/manual-qa/NEO-44.md`** complete verification. Implementation matches the kickoff plan and acceptance checklist. Residual risk is low — server-only slice; Godot gig row remains **NEO-86** out of scope.

## Documentation checked

| Document | Result |
|----------|--------|
| [`docs/plans/NEO-44-implementation-plan.md`](../plans/NEO-44-implementation-plan.md) | **Matches** — kickoff decisions (`breach`, 25 XP, GET-only wire, Postgres + in-memory), scope, expected outcomes table, reconciliation, and acceptance checklist align with code. |
| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-10** | **Matches** — AC checked; landed note and gig-vs-skill invariant documented. |
| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Matches** — Purpose/Responsibilities updated for gig XP via NEO-44 (not E2.M2 skill XP). |
| [`docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md`](../decomposition/modules/E2_M2_XpAwardAndLevelEngine.md) | **Matches** — NEO-44 landed paragraph added; removed from backlog list. |
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E2.M2 and E5.M1 rows updated with NEO-44 landed + gig progression links. |
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-authoritative defeat grant; client polls GET when NEO-86 lands. |
| [`docs/game-design/progression.md`](../game-design/progression.md) | **Matches** — combat → gig XP invariant referenced in README and manual QA. |
| [`server/README.md`](../../server/README.md) | **Matches** — gig progression GET section, combat defeat invariant, ability-cast NEO-44 note. |
| Full-stack epic decomposition | **Matches** — plan documents NEO-85/NEO-86 client counterpart; no Godot change in scope. |
| [`docs/manual-qa/NEO-44.md`](../manual-qa/NEO-44.md) | **Matches** — curl defeat spine + skill control snapshot. |

## Blocking issues

None.

## Suggestions

1. ~~**Stale E5.M1 Purpose / Responsibilities** — [`E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) line 17 still says combat “feeds XP awards through E2.M2”; line 26 says “Award combat XP via E2.M2 integration.” NEO-44 explicitly routes combat defeat to **`IPlayerGigProgressionStore`**. Update those bullets to “gig XP via dedicated store (NEO-44); not E2.M2 skill XP” so module intent matches the landed slice.~~ **Done.** Purpose, Responsibilities, and E2.M2 dependency bullets updated.

2. ~~**Stale E2.M2 implementation snapshot** — [`E2_M2_XpAwardAndLevelEngine.md`](../decomposition/modules/E2_M2_XpAwardAndLevelEngine.md) **Still backlog** paragraph lists NEO-44; alignment register already marks NEO-44 landed. Add a **NEO-44 landed (E5.M1)** paragraph (cross-link E5.M1 / gig GET) and remove NEO-44 from backlog.~~ **Done.** NEO-44 landed paragraph added; backlog line trimmed to NEO-42 craft wiring only.

3. ~~**Best-effort grant observability (optional)** — `CombatDefeatGigXpGrant` discards **`TryApplyXpDelta`** return value per kickoff; acceptable for prototype. Consider a debug/diagnostic log when apply returns **`false`** on Postgres (unknown player, FK miss) before production hardening — cast accept should remain unchanged.~~ **Done.** `LogDebug` when store write fails; `AbilityCastApi` passes logger from `ILoggerFactory`.

## Nits

- ~~Nit: Bruno **`Get gig progression after defeat spine.bru`** hardcodes **`dev-local-1`** in the GET URL while pre-request uses **`playerId`** env — use **`{{playerId}}`** (or env var) for consistency if multi-player Bruno runs matter.~~ **Done.** GET URL uses **`{{playerId}}`**.

- Nit: No automated test for **`prototype_target_beta`** defeat grant — plan adopts shared hook; alpha chain + integration test suffice for prototype; optional one-pulse-on-beta smoke if beta HP path diverges later.

- Nit: **`Get gig progression.bru`** asserts **`xp`** is a number but not **`0`** on fresh server — docs note Postgres may retain prior totals; intentional for shared DB dev; fine for CI in-memory.

## Verification

```bash
cd /home/don/neon-sprawl/server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~Gigs|FullyQualifiedName~PostAbilityCast_ShouldGrantGigXp"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~AbilityCastApiTests"
```

**NEO-44 filtered tests:** 6/6 passed (local run).  
**AbilityCastApiTests:** 21/21 passed (local run).

**Postgres (optional):** `GigProgressionGrantPersistenceIntegrationTests` when **`ConnectionStrings__NeonSprawl`** is set.

**Bruno (manual):** Run `bruno/neon-sprawl-server/gig-progression/` — seq 1 fresh GET; seq 2 defeat spine (~13s pre-request) asserts **`breach.xp === 25`** and salvage/refine unchanged.