NEO-135: address code review findings
Update decomposition docs for E7M3-03, add in-memory read-clamp test via test seed hook, fix empty factionId deny code, AAA integration test, Postgres FK orchestration notes, and strike review suggestions Done.pull/174/head
parent
c066121c9d
commit
d5f16f4cf4
|
|
@ -7,7 +7,7 @@
|
|||
| **Module ID** | E7.M3 |
|
||||
| **Epic** | [Epic 7 — Quest / Faction](../epics/epic_07_quest_faction.md) |
|
||||
| **Stage target** | Pre-production |
|
||||
| **Status** | In Progress — **E7M3-01 catalog landed** ([NEO-133](https://linear.app/neon-sprawl/issue/NEO-133)); **E7M3-02 server faction catalog load landed** ([NEO-134](https://linear.app/neon-sprawl/issue/NEO-134)): fail-fast `content/factions/*_factions.json`, `IFactionDefinitionRegistry`, quest faction cross-ref + E7M3 bundle/grid gates. Slice 3 backlog [E7M3-pre-production-backlog.md](../../plans/E7M3-pre-production-backlog.md): **E7M3-03** [NEO-135](https://linear.app/neon-sprawl/issue/NEO-135) → **E7M3-11** [NEO-143](https://linear.app/neon-sprawl/issue/NEO-143). Upstream: E7.M1 **Ready**, E7.M2 **Ready**. |
|
||||
| **Status** | In Progress — **E7M3-01 catalog landed** ([NEO-133](https://linear.app/neon-sprawl/issue/NEO-133)); **E7M3-02 server faction catalog load landed** ([NEO-134](https://linear.app/neon-sprawl/issue/NEO-134)): fail-fast `content/factions/*_factions.json`, `IFactionDefinitionRegistry`, quest faction cross-ref + E7M3 bundle/grid gates; **E7M3-03 faction standing + reputation delta stores landed** ([NEO-135](https://linear.app/neon-sprawl/issue/NEO-135)): `IFactionStandingStore`, `IReputationDeltaStore`, in-memory + Postgres `V009`/`V010`. Slice 3 backlog [E7M3-pre-production-backlog.md](../../plans/E7M3-pre-production-backlog.md): **E7M3-04** [NEO-136](https://linear.app/neon-sprawl/issue/NEO-136) → **E7M3-11** [NEO-143](https://linear.app/neon-sprawl/issue/NEO-143). Upstream: E7.M1 **Ready**, E7.M2 **Ready**. |
|
||||
| **Linear** | Label **`E7.M3`** · [E7M3-pre-production-backlog.md](../../plans/E7M3-pre-production-backlog.md) |
|
||||
|
||||
## Purpose
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -63,7 +63,7 @@ Durable per-player **`FactionStanding`** snapshot and append-only **`ReputationD
|
|||
- **Types:** `FactionStandingIds`, reason codes, read/mutation outcomes, `ReputationDeltaRow`, `ReputationDeltaSourceKinds`.
|
||||
- **Stores:** `IFactionStandingStore` / `IReputationDeltaStore`; in-memory + Postgres (`V009`, `V010`) when connection string set.
|
||||
- **DI:** `AddFactionStandingStores` in `Program.cs`; in-memory override in `InMemoryWebApplicationFactory`.
|
||||
- **Tests:** `InMemoryFactionStandingStoreTests`, `InMemoryReputationDeltaStoreTests`, Postgres persistence integration tests; host DI smoke (`766` tests green).
|
||||
- **Tests:** `InMemoryFactionStandingStoreTests`, `InMemoryReputationDeltaStoreTests`, Postgres persistence integration tests; host DI smoke (`768` tests green).
|
||||
- **Docs:** `server/README.md` faction standing + reputation delta store sections.
|
||||
|
||||
## Technical approach
|
||||
|
|
@ -73,7 +73,7 @@ Durable per-player **`FactionStanding`** snapshot and append-only **`ReputationD
|
|||
| Type | Role |
|
||||
|------|------|
|
||||
| `FactionStandingIds` | Normalize `playerId` / `factionId`; composite key helper (mirror `QuestProgressIds`) |
|
||||
| `FactionStandingReasonCodes` | `unknown_faction`, `player_not_writable`, `invalid_delta` |
|
||||
| `FactionStandingReasonCodes` | `unknown_faction`, `player_not_writable` (`invalid_delta` deferred to NEO-136 `ReputationOperations`) |
|
||||
| `FactionStandingReadOutcome` | `Success`, optional `ReasonCode`, `Standing` (clamped) |
|
||||
| `FactionStandingMutationOutcome` | `Success`, optional `ReasonCode`, `PreviousStanding`, `NewStanding` |
|
||||
| `ReputationDeltaSourceKinds` | `QuestCompletion = "quest_completion"` (prototype constant) |
|
||||
|
|
|
|||
|
|
@ -18,8 +18,8 @@ This branch lands **E7M3-03**: durable per-player **`IFactionStandingStore`** sn
|
|||
|------|--------|
|
||||
| `docs/plans/NEO-135-implementation-plan.md` | **Matches** — shipped types, stores, migrations, DI, tests, and README reconcile with plan; AC checklist marked complete. Plan types table lists `invalid_delta` reason code not present in code (reserved for NEO-136 — see nits). |
|
||||
| `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-03 scope (dual stores, clamp, audit append, no public apply API, no client counterpart) aligns with implementation. Backlog AC checkboxes still unchecked (plan doc is authoritative for this branch). |
|
||||
| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Partially matches** — module purpose/contracts align; **Status** line still stops at E7M3-02 (NEO-134) and does not record E7M3-03 store landing. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M3 row still lists **E7M3-03+** as future; should note stores landed after merge. |
|
||||
| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-03 stores landed (NEO-135). |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row updated for E7M3-03 / NEO-135. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — register row already lists `FactionStanding` / `ReputationDelta` contracts. |
|
||||
| `server/README.md` | **Matches** — new faction standing + reputation delta store sections; correctly defers auditable apply to NEO-136. |
|
||||
| `bruno/neon-sprawl-server/faction-standing/` | **N/A plan item** — health smoke folder added (reasonable host-start verification; not listed in plan). |
|
||||
|
|
@ -30,18 +30,18 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Decomposition status** — Update `E7_M3_FactionReputationLedger.md` **Status** (one line) and `documentation_and_implementation_alignment.md` E7.M3 row to record **E7M3-03 faction standing + reputation delta stores** landed ([NEO-135](https://linear.app/neon-sprawl/issue/NEO-135)). Same pattern as NEO-134 review follow-up.
|
||||
1. ~~**Decomposition status** — Update `E7_M3_FactionReputationLedger.md` **Status** (one line) and `documentation_and_implementation_alignment.md` E7.M3 row to record **E7M3-03 faction standing + reputation delta stores** landed ([NEO-135](https://linear.app/neon-sprawl/issue/NEO-135)). Same pattern as NEO-134 review follow-up.~~ **Done.** Module status + alignment table updated for E7M3-03 / NEO-135.
|
||||
|
||||
2. **In-memory read clamp test** — Plan §7 lists “Read clamp when stored value exceeds max” under `InMemoryFactionStandingStoreTests`. `TryGetStanding_ShouldClampRead_WhenStoredValueExceedsMax` exists only in Postgres integration tests. Add an in-memory case (e.g. apply to max then manually seed out-of-band via a second apply + direct dictionary seed is not possible — use apply overflow then verify read clamp, or test via Postgres-only path is insufficient for unit layer). Simplest: after clamp-to-max apply, verify read returns **100**; for true out-of-band raw, mirror Postgres test with reflection/internal test hook **or** document that read-clamp defense is integration-tested only. Prefer a dedicated in-memory test that applies deltas to max then asserts read clamp on subsequent reads at boundary (partial coverage) plus keeping Postgres test for stale DB values.
|
||||
2. ~~**In-memory read clamp test** — Plan §7 lists “Read clamp when stored value exceeds max” under `InMemoryFactionStandingStoreTests`. `TryGetStanding_ShouldClampRead_WhenStoredValueExceedsMax` exists only in Postgres integration tests. Add an in-memory case (e.g. apply to max then manually seed out-of-band via a second apply + direct dictionary seed is not possible — use apply overflow then verify read clamp, or test via Postgres-only path is insufficient for unit layer). Simplest: after clamp-to-max apply, verify read returns **100**; for true out-of-band raw, mirror Postgres test with reflection/internal test hook **or** document that read-clamp defense is integration-tested only. Prefer a dedicated in-memory test that applies deltas to max then asserts read clamp on subsequent reads at boundary (partial coverage) plus keeping Postgres test for stale DB values.~~ **Done.** Added `TryGetStanding_ShouldClampRead_WhenRawStoredValueExceedsMax` using internal `SeedRawStandingForTests`; Postgres integration test retained for stale DB values.
|
||||
|
||||
3. **Plan types table drift** — `NEO-135-implementation-plan.md` §1 lists `FactionStandingReasonCodes.invalid_delta`; shipped code only has `unknown_faction` and `player_not_writable`. Add a one-line note in the plan that `invalid_delta` is deferred to NEO-136 `ReputationOperations`, or add the constant now without wiring (avoid dead code — note-only is fine).
|
||||
3. ~~**Plan types table drift** — `NEO-135-implementation-plan.md` §1 lists `FactionStandingReasonCodes.invalid_delta`; shipped code only has `unknown_faction` and `player_not_writable`. Add a one-line note in the plan that `invalid_delta` is deferred to NEO-136 `ReputationOperations`, or add the constant now without wiring (avoid dead code — note-only is fine).~~ **Done.** Plan types table notes `invalid_delta` deferred to NEO-136.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: `PostgresReputationDeltaStore.TryAppend` relies on FK to `player_position`; a row for a non-existent player throws rather than returning `false`. Acceptable if NEO-136 always applies standing first; worth a one-line remark in README or interface XML for orchestrators.
|
||||
- ~~Nit: `PostgresReputationDeltaStore.TryAppend` relies on FK to `player_position`; a row for a non-existent player throws rather than returning `false`. Acceptable if NEO-136 always applies standing first; worth a one-line remark in README or interface XML for orchestrators.~~ **Done.** Remark added to `IReputationDeltaStore.TryAppend` XML and `server/README.md`.
|
||||
- Nit: Host DI smoke (`Host_ShouldResolveFactionStandingStoresFromDi_WhenStartupSucceeds`) lives in `InMemoryFactionStandingStoreTests.cs` rather than a host-focused test file — fine per plan “optional one-liner.”
|
||||
- Nit: `FactionStandingPersistenceIntegrationTests` places `Assert.True(outcome.Success)` inside the Act block for the first factory write — minor AAA deviation; assertions could move entirely to Assert phase.
|
||||
- Nit: Empty `factionId` on read/write returns `player_not_writable` rather than `unknown_faction` — consistent with empty-id handling elsewhere, but slightly imprecise reason code.
|
||||
- ~~Nit: `FactionStandingPersistenceIntegrationTests` places `Assert.True(outcome.Success)` inside the Act block for the first factory write — minor AAA deviation; assertions could move entirely to Assert phase.~~ **Done.**
|
||||
- ~~Nit: Empty `factionId` on read/write returns `player_not_writable` rather than `unknown_faction` — consistent with empty-id handling elsewhere, but slightly imprecise reason code.~~ **Done.** Empty faction id now returns `unknown_faction`.
|
||||
|
||||
## Verification
|
||||
|
||||
|
|
|
|||
|
|
@ -23,12 +23,11 @@ public sealed class FactionStandingPersistenceIntegrationTests(PostgresIntegrati
|
|||
await ResetFactionStandingTableAsync();
|
||||
|
||||
// Act — write through first host
|
||||
FactionStandingMutationOutcome writeOutcome;
|
||||
using (var firstScope = Factory.Services.CreateScope())
|
||||
{
|
||||
var store = firstScope.ServiceProvider.GetRequiredService<IFactionStandingStore>();
|
||||
var outcome = store.TryApplyStandingDelta(PlayerId, GridFactionId, 15);
|
||||
Assert.True(outcome.Success);
|
||||
Assert.Equal(15, outcome.NewStanding);
|
||||
writeOutcome = store.TryApplyStandingDelta(PlayerId, GridFactionId, 15);
|
||||
}
|
||||
|
||||
FactionStandingReadOutcome readBack;
|
||||
|
|
@ -40,6 +39,8 @@ public sealed class FactionStandingPersistenceIntegrationTests(PostgresIntegrati
|
|||
}
|
||||
|
||||
// Assert
|
||||
Assert.True(writeOutcome.Success);
|
||||
Assert.Equal(15, writeOutcome.NewStanding);
|
||||
Assert.True(readBack.Success);
|
||||
Assert.Equal(15, readBack.Standing);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -72,6 +72,35 @@ public sealed class InMemoryFactionStandingStoreTests
|
|||
Assert.Equal(-100, store.TryGetStanding(PlayerId, GridFactionId).Standing);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryGetStanding_ShouldClampRead_WhenRawStoredValueExceedsMax()
|
||||
{
|
||||
// Arrange
|
||||
var store = CreateStore();
|
||||
store.SeedRawStandingForTests(PlayerId, GridFactionId, 999);
|
||||
// Act
|
||||
var outcome = store.TryGetStanding(PlayerId, GridFactionId);
|
||||
// Assert
|
||||
Assert.True(outcome.Success);
|
||||
Assert.Null(outcome.ReasonCode);
|
||||
Assert.Equal(100, outcome.Standing);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryGetStanding_ShouldReturnUnknownFaction_WhenFactionIdEmpty()
|
||||
{
|
||||
// Arrange
|
||||
var store = CreateStore();
|
||||
// Act
|
||||
var read = store.TryGetStanding(PlayerId, " ");
|
||||
var write = store.TryApplyStandingDelta(PlayerId, "", 5);
|
||||
// Assert
|
||||
Assert.False(read.Success);
|
||||
Assert.Equal(FactionStandingReasonCodes.UnknownFaction, read.ReasonCode);
|
||||
Assert.False(write.Success);
|
||||
Assert.Equal(FactionStandingReasonCodes.UnknownFaction, write.ReasonCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryGetStanding_ShouldDenyUnknownFaction_WithStructuredReason()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -3,7 +3,10 @@ namespace NeonSprawl.Server.Game.Factions;
|
|||
/// <summary>Append-only reputation delta audit log (NEO-135).</summary>
|
||||
public interface IReputationDeltaStore
|
||||
{
|
||||
/// <summary>First append with a given row <c>Id</c> returns <c>true</c>; duplicate ids return <c>false</c>.</summary>
|
||||
/// <summary>
|
||||
/// First append with a given row <c>Id</c> returns <c>true</c>; duplicate ids return <c>false</c>.
|
||||
/// Postgres: row references <c>player_position</c> — orchestrators (NEO-136) should apply standing before audit append.
|
||||
/// </summary>
|
||||
bool TryAppend(ReputationDeltaRow row);
|
||||
|
||||
/// <summary>Audit rows for one player ordered by <see cref="ReputationDeltaRow.RecordedAt"/> then <c>Id</c> ascending.</summary>
|
||||
|
|
|
|||
|
|
@ -38,11 +38,16 @@ public sealed class InMemoryFactionStandingStore(
|
|||
{
|
||||
var player = FactionStandingIds.NormalizePlayerId(playerId);
|
||||
var faction = FactionStandingIds.NormalizeFactionId(factionId);
|
||||
if (player.Length == 0 || faction.Length == 0)
|
||||
if (player.Length == 0)
|
||||
{
|
||||
return DenyRead(FactionStandingReasonCodes.PlayerNotWritable);
|
||||
}
|
||||
|
||||
if (faction.Length == 0)
|
||||
{
|
||||
return DenyRead(FactionStandingReasonCodes.UnknownFaction);
|
||||
}
|
||||
|
||||
if (!CanWritePlayer(player))
|
||||
{
|
||||
return DenyRead(FactionStandingReasonCodes.PlayerNotWritable);
|
||||
|
|
@ -63,11 +68,16 @@ public sealed class InMemoryFactionStandingStore(
|
|||
{
|
||||
var player = FactionStandingIds.NormalizePlayerId(playerId);
|
||||
var faction = FactionStandingIds.NormalizeFactionId(factionId);
|
||||
if (player.Length == 0 || faction.Length == 0)
|
||||
if (player.Length == 0)
|
||||
{
|
||||
return DenyMutation(FactionStandingReasonCodes.PlayerNotWritable);
|
||||
}
|
||||
|
||||
if (faction.Length == 0)
|
||||
{
|
||||
return DenyMutation(FactionStandingReasonCodes.UnknownFaction);
|
||||
}
|
||||
|
||||
if (!CanWritePlayer(player))
|
||||
{
|
||||
return DenyMutation(FactionStandingReasonCodes.PlayerNotWritable);
|
||||
|
|
@ -94,4 +104,21 @@ public sealed class InMemoryFactionStandingStore(
|
|||
|
||||
private static FactionStandingMutationOutcome DenyMutation(string reasonCode) =>
|
||||
new(false, reasonCode, 0, 0);
|
||||
|
||||
/// <summary>Test-only seed of raw standing before read-clamp (NEO-135 unit tests).</summary>
|
||||
internal void SeedRawStandingForTests(string playerId, string factionId, int rawStanding)
|
||||
{
|
||||
var player = FactionStandingIds.NormalizePlayerId(playerId);
|
||||
var faction = FactionStandingIds.NormalizeFactionId(factionId);
|
||||
if (player.Length == 0 || faction.Length == 0 || !CanWritePlayer(player))
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
var key = FactionStandingIds.MakeStandingKey(player, faction);
|
||||
lock (keyLocks.GetOrAdd(key, _ => new object()))
|
||||
{
|
||||
standingByKey[key] = rawStanding;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -24,11 +24,16 @@ public sealed class PostgresFactionStandingStore(
|
|||
{
|
||||
var player = FactionStandingIds.NormalizePlayerId(playerId);
|
||||
var faction = FactionStandingIds.NormalizeFactionId(factionId);
|
||||
if (player.Length == 0 || faction.Length == 0)
|
||||
if (player.Length == 0)
|
||||
{
|
||||
return DenyRead(FactionStandingReasonCodes.PlayerNotWritable);
|
||||
}
|
||||
|
||||
if (faction.Length == 0)
|
||||
{
|
||||
return DenyRead(FactionStandingReasonCodes.UnknownFaction);
|
||||
}
|
||||
|
||||
if (!factionRegistry.TryGetDefinition(faction, out var definition) || definition is null)
|
||||
{
|
||||
return DenyRead(FactionStandingReasonCodes.UnknownFaction);
|
||||
|
|
@ -65,11 +70,16 @@ public sealed class PostgresFactionStandingStore(
|
|||
{
|
||||
var player = FactionStandingIds.NormalizePlayerId(playerId);
|
||||
var faction = FactionStandingIds.NormalizeFactionId(factionId);
|
||||
if (player.Length == 0 || faction.Length == 0)
|
||||
if (player.Length == 0)
|
||||
{
|
||||
return DenyMutation(FactionStandingReasonCodes.PlayerNotWritable);
|
||||
}
|
||||
|
||||
if (faction.Length == 0)
|
||||
{
|
||||
return DenyMutation(FactionStandingReasonCodes.UnknownFaction);
|
||||
}
|
||||
|
||||
if (!factionRegistry.TryGetDefinition(faction, out var definition) || definition is null)
|
||||
{
|
||||
return DenyMutation(FactionStandingReasonCodes.UnknownFaction);
|
||||
|
|
|
|||
|
|
@ -78,6 +78,8 @@ Append-only **`ReputationDelta`** audit rows live in **`IReputationDeltaStore`**
|
|||
- **`TryAppend`** — first row id returns `true`; duplicate ids return `false`.
|
||||
- **`GetDeltasForPlayer`** — ordered by **`recordedAt`** then **`id`** (prototype test/audit read).
|
||||
|
||||
**Postgres orchestration:** audit rows FK to **`player_position`** — append after standing apply succeeds (NEO-136 **`ReputationOperations`**); a missing player row causes insert failure rather than a structured `false` from **`TryAppend`**.
|
||||
|
||||
**Storage:** in-memory singletons when **`ConnectionStrings:NeonSprawl`** is unset (standing store seeds configured dev player only). When Postgres is configured, **`PostgresFactionStandingStore`** persists to **`player_faction_standing`** ([`V009__player_faction_standing.sql`](../db/migrations/V009__player_faction_standing.sql)) and **`PostgresReputationDeltaStore`** appends to **`reputation_delta_audit`** ([`V010__reputation_delta_audit.sql`](../db/migrations/V010__reputation_delta_audit.sql)). Plan: [NEO-135 implementation plan](../../docs/plans/NEO-135-implementation-plan.md).
|
||||
|
||||
## Resource-node catalog (`content/resource-nodes`, NEO-58)
|
||||
|
|
|
|||
Loading…
Reference in New Issue