NEO-44: Address code review — docs, debug log, Bruno playerId.
Update E5.M1/E2.M2 module docs for gig XP path; LogDebug on failed
gig store write; Bruno defeat spine uses {{playerId}}; strike review items.
pull/118/head
parent
1e06b7174d
commit
9d9b53332f
|
|
@ -71,7 +71,7 @@ script:pre-request {
|
|||
}
|
||||
|
||||
get {
|
||||
url: {{baseUrl}}/game/players/dev-local-1/gig-progression
|
||||
url: {{baseUrl}}/game/players/{{playerId}}/gig-progression
|
||||
body: none
|
||||
auth: none
|
||||
}
|
||||
|
|
|
|||
|
|
@ -27,7 +27,9 @@
|
|||
|
||||
**NEO-43 landed (prep):** **`MissionRewardSkillXpGrant`** for scripted **`mission_reward`** skill XP ([implementation plan](../../plans/NEO-43-implementation-plan.md)); manual QA **[`NEO-43.md`](../../manual-qa/NEO-43.md)**; full quest hand-in awaits **E7.M2**.
|
||||
|
||||
**Still backlog within E2.M2 / Slice 3:** **NEO-44** (gig XP from combat, Epic 5 — not skill XP via E2.M2). **NEO-42** end-to-end craft wiring awaits **E3.M2** success handler.
|
||||
**NEO-44 landed (E5.M1 / combat path, not E2.M2 skill grant):** combat defeat on **`AbilityCastApi`** accept when **`targetDefeated`** grants **25** gig XP to prototype main gig **`breach`** via **`CombatDefeatGigXpGrant`** + **`IPlayerGigProgressionStore`** ([NEO-44 plan](../../plans/NEO-44-implementation-plan.md)); read model **`GET /game/players/{id}/gig-progression`**; manual QA **[`NEO-44.md`](../../manual-qa/NEO-44.md)**; **[server README — Gig progression (NEO-44)](../../../server/README.md#gig-progression-snapshot-neo-44)**; module handoff [E5.M1](E5_M1_CombatRulesEngine.md).
|
||||
|
||||
**Still backlog within E2.M2 / Slice 3:** **NEO-42** end-to-end craft wiring awaits **E3.M2** success handler (prep helper landed).
|
||||
|
||||
**E2.M3:** [MasteryAndPerkUnlocks](E2_M3_MasteryAndPerkUnlocks.md) re-evaluates perk eligibility on skill **level-up** after grants ([NEO-45](https://linear.app/neon-sprawl/issue/NEO-45) → [NEO-49](https://linear.app/neon-sprawl/issue/NEO-49)).
|
||||
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@
|
|||
|
||||
**Authority:** [Client vs server](client_server_authority.md#e5m1-combatrulesengine) — server validates intents and computes all outcomes; never trust client damage/healing/crit/death.
|
||||
|
||||
Core tab-target combat resolution: valid actions against a locked target, hit resolution, cooldown and resource timing, and threat state as needed for prototype encounters. Presents readable outcomes in fixed isometric view and feeds XP awards through [E2.M2](E2_M2_XpAwardAndLevelEngine.md).
|
||||
Core tab-target combat resolution: valid actions against a locked target, hit resolution, cooldown and resource timing, and threat state as needed for prototype encounters. Presents readable outcomes in fixed isometric view. **Combat defeat** awards **gig XP** via **`IPlayerGigProgressionStore`** ([NEO-44](../../plans/NEO-44-implementation-plan.md)) — **not** E2.M2 skill XP ([progression.md](../../game-design/progression.md)).
|
||||
|
||||
**PvP:** Single engine for PvE and PvP; player-target hostility **gates** on [E6.M1](E6_M1_PvPEligibilityAndFlagState.md). See [PvP and the combat engine](pvp_combat_integration.md).
|
||||
|
||||
|
|
@ -23,7 +23,7 @@ Core tab-target combat resolution: valid actions against a locked target, hit re
|
|||
- Combat state machine for tab-target flow (attacks, abilities per prototype scope).
|
||||
- Validate and apply `CombatAction`; produce `CombatResolution` for UI and logs (include **PvP deny reasons** when player targets are blocked; see [pvp_combat_integration.md](pvp_combat_integration.md)).
|
||||
- Integrate with targeting/interaction from **E1.M3** (target lock, valid target checks).
|
||||
- Award combat XP via E2.M2 integration where specified in slices.
|
||||
- Award **gig XP** on combat defeat via dedicated gig progression store ([NEO-44](../../plans/NEO-44-implementation-plan.md)); do **not** route default encounter clears through E2.M2 skill XP.
|
||||
|
||||
## Key contracts
|
||||
|
||||
|
|
@ -36,7 +36,7 @@ Core tab-target combat resolution: valid actions against a locked target, hit re
|
|||
## Module dependencies
|
||||
|
||||
- **E1.M3** — [InteractionAndTargetingLayer](E1_M3_InteractionAndTargetingLayer.md): `TargetState`, selection, and alignment with **E1.M4** ([AbilityInputScaffold](E1_M4_AbilityInputScaffold.md)) for `AbilityCastRequest` wiring.
|
||||
- **E2.M2** — [XpAwardAndLevelEngine](E2_M2_XpAwardAndLevelEngine.md): combat XP grants.
|
||||
- **E2.M2** — [XpAwardAndLevelEngine](E2_M2_XpAwardAndLevelEngine.md): **skill** XP for gather/craft/mission callers only; combat → gig XP stays on E5.M1 (NEO-44).
|
||||
|
||||
## Dependents (by design)
|
||||
|
||||
|
|
@ -44,7 +44,7 @@ Core tab-target combat resolution: valid actions against a locked target, hit re
|
|||
- **E5.M3** — EncounterAndRewardTables.
|
||||
- **E1.M4** — AbilityInputScaffold sends `AbilityCastRequest` to combat.
|
||||
- **E7.M1** — QuestStateMachine for combat-related quest steps.
|
||||
- **E2.M2** — Invoked as integration caller for XP (not a structural child).
|
||||
- **E2.M2** — Skill XP integration caller for gather/craft/mission paths (not structural child; not combat gig XP).
|
||||
|
||||
## Related implementation slices
|
||||
|
||||
|
|
|
|||
|
|
@ -18,8 +18,8 @@ NEO-44 delivers **E5M1-10**: when **`AbilityCastApi`** accepts a cast with **`co
|
|||
|----------|--------|
|
||||
| [`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) | **Partially matches** — Status and **Related implementation slices** cite NEO-44 landed; **Purpose** / **Responsibilities** bullets still say combat XP flows through E2.M2 (stale — see Suggestions). |
|
||||
| [`docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md`](../decomposition/modules/E2_M2_XpAwardAndLevelEngine.md) | **Partially matches** — **Purpose** correctly separates combat → gig XP; **Implementation snapshot** still lists NEO-44 as backlog (stale). |
|
||||
| [`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. |
|
||||
|
|
@ -33,15 +33,15 @@ 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.
|
||||
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.
|
||||
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.
|
||||
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.
|
||||
- ~~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.
|
||||
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
using Microsoft.Extensions.Logging;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using NeonSprawl.Server.Game.Gigs;
|
||||
using Xunit;
|
||||
|
|
@ -42,4 +43,23 @@ public sealed class CombatDefeatGigXpGrantTests
|
|||
Assert.Null(exception);
|
||||
Assert.Empty(gigStore.GetXpTotals("missing-player"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task GrantOnCombatDefeat_WhenPlayerUnknown_ShouldNotThrow_WithLogger()
|
||||
{
|
||||
// Arrange
|
||||
await using var factory = new InMemoryWebApplicationFactory();
|
||||
_ = factory.CreateClient();
|
||||
using var scope = factory.Services.CreateScope();
|
||||
var gigStore = scope.ServiceProvider.GetRequiredService<IPlayerGigProgressionStore>();
|
||||
var logger = scope.ServiceProvider.GetRequiredService<ILoggerFactory>()
|
||||
.CreateLogger(nameof(CombatDefeatGigXpGrant));
|
||||
|
||||
// Act
|
||||
var exception = Record.Exception(() =>
|
||||
CombatDefeatGigXpGrant.GrantOnCombatDefeat("missing-player", gigStore, logger));
|
||||
|
||||
// Assert
|
||||
Assert.Null(exception);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -64,6 +64,7 @@ public static class AbilityCastApi
|
|||
IAbilityDefinitionRegistry abilities,
|
||||
ICombatEntityHealthStore healthStore,
|
||||
IPlayerGigProgressionStore gigStore,
|
||||
ILoggerFactory loggerFactory,
|
||||
TimeProvider clock) =>
|
||||
{
|
||||
// NEO-30: not a Slice 3 `ability_cast_denied` site — no AbilityCastResponse body.
|
||||
|
|
@ -229,7 +230,10 @@ public static class AbilityCastApi
|
|||
|
||||
if (combatResult.TargetDefeated)
|
||||
{
|
||||
CombatDefeatGigXpGrant.GrantOnCombatDefeat(id.Trim(), gigStore);
|
||||
CombatDefeatGigXpGrant.GrantOnCombatDefeat(
|
||||
id.Trim(),
|
||||
gigStore,
|
||||
loggerFactory.CreateLogger(nameof(CombatDefeatGigXpGrant)));
|
||||
}
|
||||
|
||||
// NEO-30 telemetry hook site: `ability_cast_requested` at authoritative accept (TODO(E9.M1): catalog emit).
|
||||
|
|
|
|||
|
|
@ -7,13 +7,21 @@ namespace NeonSprawl.Server.Game.Gigs;
|
|||
public static class CombatDefeatGigXpGrant
|
||||
{
|
||||
/// <summary>Applies one combat-defeat grant to the prototype main gig when <paramref name="playerId"/> is writable.</summary>
|
||||
public static void GrantOnCombatDefeat(string playerId, IPlayerGigProgressionStore gigStore)
|
||||
public static void GrantOnCombatDefeat(
|
||||
string playerId,
|
||||
IPlayerGigProgressionStore gigStore,
|
||||
ILogger? logger = null)
|
||||
{
|
||||
_ = gigStore.TryApplyXpDelta(
|
||||
if (!gigStore.TryApplyXpDelta(
|
||||
playerId,
|
||||
GigProgressionConstants.PrototypeMainGigId,
|
||||
GigProgressionConstants.CombatDefeatXpAmount,
|
||||
out _,
|
||||
out _);
|
||||
out _))
|
||||
{
|
||||
logger?.LogDebug(
|
||||
"Combat defeat gig XP grant skipped for player {PlayerId} (store write failed)",
|
||||
playerId);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue