diff --git a/docs/plans/NEO-146-implementation-plan.md b/docs/plans/NEO-146-implementation-plan.md index bba779f..2083e50 100644 --- a/docs/plans/NEO-146-implementation-plan.md +++ b/docs/plans/NEO-146-implementation-plan.md @@ -68,7 +68,8 @@ Durable per-player **`ContractInstance`** rows (active/completed) and append-onl - **Types:** `ContractInstanceStatus`, `ContractInstanceState`, `ContractInstanceIds`, `ContractInstanceReasonCodes`, `ContractOutcomeRow`, `ContractOutcomeIds`. - **Stores:** `IContractInstanceStore` / `IContractOutcomeStore`; in-memory + Postgres (`V011`, `V012`) when connection string set; partial unique index + app-level one-active enforcement. - **DI:** `AddContractInstanceStores` in `Program.cs`; in-memory override in `InMemoryWebApplicationFactory`. -- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`866` tests green). +- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`867` tests green). +- **In-memory outcome guard:** `InMemoryContractOutcomeStore.TryAppend` denies when **`IContractInstanceStore.TryGet`** is false (Postgres FK parity). - **Docs:** `server/README.md` contract instance + outcome store sections. - **Bruno:** `bruno/neon-sprawl-server/contract-stores/` health smoke (startup-only; no contract HTTP in E7M4-03). diff --git a/docs/reviews/2026-06-24-NEO-146-r2.md b/docs/reviews/2026-06-24-NEO-146-r2.md index 7afb6c1..d8350dc 100644 --- a/docs/reviews/2026-06-24-NEO-146-r2.md +++ b/docs/reviews/2026-06-24-NEO-146-r2.md @@ -16,7 +16,7 @@ Follow-up review after the initial NEO-146 review and a broad C# style/analyzer | Document | Alignment | |----------|-----------| -| `docs/plans/NEO-146-implementation-plan.md` | **Matches** — AC complete; reconciliation includes Bruno smoke, `MakeInstanceKey` locking, Postgres clear adopted, review follow-up tests. Test count says `866`; suite now **867** (minor drift). | +| `docs/plans/NEO-146-implementation-plan.md` | **Matches** — AC complete; reconciliation includes Bruno smoke, `MakeInstanceKey` locking, Postgres clear adopted, review follow-up tests. Test count **867**. | | `docs/plans/E7M4-pre-production-backlog.md` (E7M4-03) | **Matches** | | `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — runtime stores paragraph (NEO-146). | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7M4-03 / NEO-146 stores row added. | @@ -43,13 +43,13 @@ None. ## Suggestions -1. **PR / commit scope for future stories** — Commits `b455ff5`…`01e82f3` touch ~50 files outside `Game/Contracts/` (catalog loaders, integration-test `using` layout, bootstrap `Lock` gates, mutation-outcome docs). Mechanical and green, but it widens review/merge-conflict surface for an infrastructure ticket. Consider a separate `chore:` PR for repo-wide analyzer/style passes when the diff is this large. +1. ~~**PR / commit scope for future stories**~~ — **Intentional.** Repo-wide analyzer/style cleanup (`b455ff5`…`01e82f3`) was bundled on this branch by author request; no split `chore:` PR for E7M4-03. -2. **In-memory outcome instance guard (optional parity)** — Postgres `TryAppend` returns `false` when `contract_instance` is missing (FK); in-memory still accepts a normalized row without checking instance existence. README documents Postgres ordering; orchestrators (NEO-149) will create the instance first. If strict cross-backend parity matters before NEO-149, add an optional `IContractInstanceStore` lookup in `InMemoryContractOutcomeStore.TryAppend` or a shared guard helper — otherwise document the asymmetry explicitly in README (one line under in-memory storage). +2. ~~**In-memory outcome instance guard (optional parity)**~~ — **Done.** `InMemoryContractOutcomeStore` injects **`IContractInstanceStore`** and denies **`TryAppend`** when **`TryGet`** is false; unit test + README/plan updated (Postgres FK parity). ## Nits -- Nit: Plan reconciliation cites **866** tests; re-review run reports **867** — update the count on next doc touch. +- ~~Nit: Plan reconciliation cites **866** tests; re-review run reports **867**~~ — **Done.** Plan reconciliation updated to **867**. - Nit: `ContractInstanceReasonCodes` remains unused until NEO-147 orchestrators — expected deferral; no action in E7M4-03. @@ -70,4 +70,4 @@ Optional Postgres integration (when `ConnectionStrings__NeonSprawl` is set): dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractInstancePersistence|FullyQualifiedName~ContractOutcomePersistence" ``` -Merge-ready: prior review feedback is closed; remaining items are process/parity nits only. +Merge-ready: all suggestions addressed or intentionally accepted; remaining nits are expected deferrals only. diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractOutcomeStoreTests.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractOutcomeStoreTests.cs index 73f0787..9acbaf2 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractOutcomeStoreTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractOutcomeStoreTests.cs @@ -1,4 +1,6 @@ +using Microsoft.Extensions.Options; using NeonSprawl.Server.Game.Contracts; +using NeonSprawl.Server.Game.PositionState; using NeonSprawl.Server.Game.Rewards; using Xunit; @@ -9,13 +11,17 @@ public sealed class InMemoryContractOutcomeStoreTests private const string PlayerId = "dev-local-1"; private const string InstanceId = "prototype_contract_instance_001"; private const string OutcomeRowId = "outcome-row-001"; + private const string TemplateId = "prototype_contract_clear_combat_pocket"; + private const string SeedBucket = "2026-06-22"; + private static readonly DateTimeOffset IssuedAt = new(2026, 6, 22, 10, 0, 0, TimeSpan.Zero); private static readonly DateTimeOffset RecordedAt = new(2026, 6, 22, 12, 0, 0, TimeSpan.Zero); [Fact] public void TryAppend_ShouldPersistAndQueryByInstanceAndIdempotencyKey() { // Arrange - var store = new InMemoryContractOutcomeStore(); + var (instanceStore, store) = CreateStores(); + SeedActiveInstance(instanceStore); var row = CreatePrototypeOutcome(); // Act var appended = store.TryAppend(row); @@ -36,7 +42,8 @@ public sealed class InMemoryContractOutcomeStoreTests public void TryAppend_ShouldDenyDuplicateRowId() { // Arrange - var store = new InMemoryContractOutcomeStore(); + var (instanceStore, store) = CreateStores(); + SeedActiveInstance(instanceStore); var row = CreatePrototypeOutcome(); Assert.True(store.TryAppend(row)); // Act @@ -49,7 +56,8 @@ public sealed class InMemoryContractOutcomeStoreTests public void TryAppend_ShouldDenyDuplicateIdempotencyKey() { // Arrange - var store = new InMemoryContractOutcomeStore(); + var (instanceStore, store) = CreateStores(); + SeedActiveInstance(instanceStore); var row = CreatePrototypeOutcome(); Assert.True(store.TryAppend(row)); // Act @@ -62,7 +70,8 @@ public sealed class InMemoryContractOutcomeStoreTests public void TryAppend_ShouldFailClosed_WhenRequiredFieldsEmpty() { // Arrange - var store = new InMemoryContractOutcomeStore(); + var (instanceStore, store) = CreateStores(); + SeedActiveInstance(instanceStore); var invalidRow = CreatePrototypeOutcome() with { PlayerId = " " }; // Act var appended = store.TryAppend(invalidRow); @@ -71,6 +80,37 @@ public sealed class InMemoryContractOutcomeStoreTests Assert.Empty(store.GetOutcomesForInstance(InstanceId)); } + [Fact] + public void TryAppend_ShouldReturnFalse_WhenContractInstanceMissing() + { + // Arrange + var (_, store) = CreateStores(); + var row = CreatePrototypeOutcome(); + // Act + var appended = store.TryAppend(row); + // Assert + Assert.False(appended); + Assert.Empty(store.GetOutcomesForInstance(InstanceId)); + } + + private static (InMemoryContractInstanceStore InstanceStore, InMemoryContractOutcomeStore OutcomeStore) CreateStores() + { + var options = Options.Create(new GamePositionOptions { DevPlayerId = PlayerId }); + var instanceStore = new InMemoryContractInstanceStore(options); + return (instanceStore, new InMemoryContractOutcomeStore(instanceStore)); + } + + private static void SeedActiveInstance(InMemoryContractInstanceStore instanceStore) + { + Assert.True(instanceStore.TryCreateActive( + PlayerId, + InstanceId, + TemplateId, + SeedBucket, + IssuedAt, + out _)); + } + private static ContractOutcomeRow CreatePrototypeOutcome() { var idempotencyKey = ContractOutcomeIds.MakeIdempotencyKey(PlayerId, InstanceId); diff --git a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractOutcomeStore.cs b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractOutcomeStore.cs index 0e502df..7340c06 100644 --- a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractOutcomeStore.cs +++ b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractOutcomeStore.cs @@ -4,7 +4,7 @@ using NeonSprawl.Server.Game.Rewards; namespace NeonSprawl.Server.Game.Contracts; /// Thread-safe in-memory append-only contract outcome audit log (NEO-146). -public sealed class InMemoryContractOutcomeStore : IContractOutcomeStore +public sealed class InMemoryContractOutcomeStore(IContractInstanceStore instanceStore) : IContractOutcomeStore { private readonly ConcurrentDictionary rowsById = new(StringComparer.Ordinal); @@ -20,6 +20,11 @@ public sealed class InMemoryContractOutcomeStore : IContractOutcomeStore return false; } + if (!instanceStore.TryGet(normalized.PlayerId, normalized.ContractInstanceId, out _)) + { + return false; + } + lock (idLocks.GetOrAdd(normalized.Id, _ => new object())) { if (rowsById.ContainsKey(normalized.Id) || diff --git a/server/README.md b/server/README.md index 1f79191..dc42b43 100644 --- a/server/README.md +++ b/server/README.md @@ -292,12 +292,12 @@ Append-only contract completion audit rows live in **`IContractOutcomeStore`**. **Store interface methods:** -- **`TryAppend`** — first row id or idempotency key returns `true`; duplicates return `false`. Missing **`contract_instance`** row (Postgres FK) also returns `false` — orchestrators (NEO-149) must create the instance before appending an outcome. +- **`TryAppend`** — first row id or idempotency key returns `true`; duplicates return `false`. Missing **`contract_instance`** row returns `false` (Postgres FK; in-memory checks **`IContractInstanceStore.TryGet`**) — orchestrators (NEO-149) must create the instance before appending an outcome. - **`TryGetByIdempotencyKey`** — read one outcome for delivery replay / audit tests. - **`GetOutcomesForInstance`** — list outcomes for one contract instance id (ordered by recorded time). - **`TryClearForInstance`** — dev fixture only. -**Storage:** in-memory when Postgres is unset; **`PostgresContractOutcomeStore`** appends to **`contract_outcome`** ([`V012__contract_outcome.sql`](../db/migrations/V012__contract_outcome.sql)) with JSONB grant snapshots when configured. Plan: [NEO-146 implementation plan](../../docs/plans/NEO-146-implementation-plan.md). +**Storage:** in-memory when Postgres is unset (**`InMemoryContractOutcomeStore`** requires a matching instance row in **`IContractInstanceStore`**); **`PostgresContractOutcomeStore`** appends to **`contract_outcome`** ([`V012__contract_outcome.sql`](../db/migrations/V012__contract_outcome.sql)) with JSONB grant snapshots when configured. Plan: [NEO-146 implementation plan](../../docs/plans/NEO-146-implementation-plan.md). ## Quest definitions (NEO-115)