From 0888262f138d246365ba2e8a134e626e1db61be0 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 28 Jun 2026 10:25:58 -0400 Subject: [PATCH] NEO-151: Address code review findings for contract HTTP API Return 404 for whitespace path on issue POST, add seed-field validation tests, inline Postgres ListForPlayer active query, Postgres list test, Bruno whitespace-path 404 check, and strike through resolved review items. --- ...OST contract issue whitespace path 404.bru | 30 +++++++++++ docs/plans/NEO-151-implementation-plan.md | 8 +-- docs/reviews/2026-06-28-NEO-151.md | 12 ++--- .../Game/Contracts/ContractApiTests.cs | 51 +++++++++++++++++-- ...ractInstancePersistenceIntegrationTests.cs | 49 ++++++++++++++++++ .../Game/Contracts/ContractIssueApi.cs | 15 +++--- .../PostgresContractInstanceStore.cs | 20 +++++++- 7 files changed, 162 insertions(+), 23 deletions(-) create mode 100644 bruno/neon-sprawl-server/contracts/POST contract issue whitespace path 404.bru diff --git a/bruno/neon-sprawl-server/contracts/POST contract issue whitespace path 404.bru b/bruno/neon-sprawl-server/contracts/POST contract issue whitespace path 404.bru new file mode 100644 index 0000000..6e58051 --- /dev/null +++ b/bruno/neon-sprawl-server/contracts/POST contract issue whitespace path 404.bru @@ -0,0 +1,30 @@ +meta { + name: POST contract issue whitespace path 404 + type: http + seq: 4 +} + +docs { + NEO-151 review: whitespace-only path id returns 404 (position gate), matching quest accept and contract GET. +} + +post { + url: {{baseUrl}}/game/players/%20/contracts/issue + body: json + auth: none +} + +body:json { + { + "schemaVersion": 1, + "playerId": "{{playerId}}", + "templateId": "prototype_contract_clear_combat_pocket", + "seedBucket": "2026-06-28-neo151-bruno" + } +} + +tests { + test("returns 404 for whitespace path id", function () { + expect(res.getStatus()).to.equal(404); + }); +} diff --git a/docs/plans/NEO-151-implementation-plan.md b/docs/plans/NEO-151-implementation-plan.md index 0af6c1f..e2cf468 100644 --- a/docs/plans/NEO-151-implementation-plan.md +++ b/docs/plans/NEO-151-implementation-plan.md @@ -57,7 +57,7 @@ Client-readable contract issuance and state without local math — **`POST …/c - **`IContractInstanceStore.ListForPlayer`** — active first + up to 10 completed rows (`issuedAt` desc); in-memory + Postgres impls. - **`ContractIssueApi`** — `POST …/contracts/issue` delegates to **`ContractGeneratorOperations.TryIssue`** with full registry DI. - **`ContractListApi`** — `GET …/contracts` with **`completionRewardSummary`** via shared **`QuestProgressApi`** delivery mapper. -- **Tests:** 5 store list unit tests + 10 HTTP integration tests; **927** tests green. +- **Tests:** 5 store list unit tests + 14 HTTP integration tests (incl. empty seed-field `[Theory]`) + Postgres **`ListForPlayer`** persistence test; **933** tests green. - **Bruno:** `bruno/neon-sprawl-server/contracts/` — default GET, issue, encounter-clear capstone. - **Docs:** `server/README.md`, `E7_M4_ContractMissionGenerator.md`, alignment register. @@ -76,7 +76,7 @@ IReadOnlyList ListForPlayer(string playerId, int maxCompl - Append up to **`maxCompletedRows`** **completed** instances for that player, **`issuedAt` descending** (tie-break **`contractInstanceId`** ordinal). - Prototype constant **`ContractListApi.MaxCompletedRows = 10`** (kickoff decision). -**Postgres:** `SELECT … WHERE player_id = @pid AND status = 'completed' ORDER BY completed_at DESC NULLS LAST, contract_instance_id LIMIT @n` plus existing active query. +**Postgres:** `SELECT … WHERE player_id = @pid AND status = 'completed' ORDER BY issued_at DESC, contract_instance_id ASC LIMIT @n` plus active query on the same connection. **In-memory:** scan **`byInstanceId`** for player-owned completed rows, sort, take N, prepend active. @@ -102,8 +102,8 @@ Unit tests in **`InMemoryContractInstanceStoreTests`**: empty player, active onl **Handler flow:** -1. **400** when body null, **`schemaVersion`** mismatch, empty **`templateId`** / **`seedBucket`**, or body **`playerId`** ≠ trimmed path **`{id}`** (case-sensitive ordinal after trim — match inventory/quest normalization). -2. **404** when path player unknown (**`IPositionStateStore`** gate — same as quest progress). +1. **404** when path `{id}` is whitespace-only or player unknown (**`IPositionStateStore`** gate — same as quest progress / accept). +2. **400** when body null, **`schemaVersion`** mismatch, empty **`templateId`** / **`seedBucket`** / body **`playerId`**, or body **`playerId`** ≠ trimmed path `{id}` after normalize. 3. Call **`ContractGeneratorOperations.TryIssue`** with path player id, body fields, and injected registries/stores (**mirror `ContractGeneratorOperationsIntegrationTests`** DI wiring). 4. **200** **`ContractIssueResponse`** via **`MapIssueResponse`** — on success map **`ContractInstanceRowJson`** from result snapshot + template registry; on deny include **`reasonCode`** and optional snapshot row (e.g. **`active_contract_exists`**). diff --git a/docs/reviews/2026-06-28-NEO-151.md b/docs/reviews/2026-06-28-NEO-151.md index d87359f..74fed77 100644 --- a/docs/reviews/2026-06-28-NEO-151.md +++ b/docs/reviews/2026-06-28-NEO-151.md @@ -35,17 +35,17 @@ None. ## Suggestions -1. **Align empty path `{id}` on POST with sibling player APIs** — `ContractIssueApi` returns **400** when the path id normalizes to empty (whitespace-only), while `ContractListApi`, `QuestAcceptApi`, and most other player routes return **404** for `trimmedId.Length == 0`. Recommend matching the **404** position-gate pattern for consistency. +1. ~~**Align empty path `{id}` on POST with sibling player APIs** — `ContractIssueApi` returns **400** when the path id normalizes to empty (whitespace-only), while `ContractListApi`, `QuestAcceptApi`, and most other player routes return **404** for `trimmedId.Length == 0`. Recommend matching the **404** position-gate pattern for consistency.~~ **Done.** — position gate runs first; whitespace-only path returns **404**. -2. **Add POST 400 tests for missing seed fields** — Handler rejects empty **`templateId`** / **`seedBucket`** (lines 32–36 in `ContractIssueApi.cs`), but `ContractApiTests` only covers schema version and **`playerId`** mismatch. One test each (or a `[Theory]`) would lock the validation gate. +2. ~~**Add POST 400 tests for missing seed fields** — Handler rejects empty **`templateId`** / **`seedBucket`** (lines 32–36 in `ContractIssueApi.cs`), but `ContractApiTests` only covers schema version and **`playerId`** mismatch. One test each (or a `[Theory]`) would lock the validation gate.~~ **Done.** — `[Theory]` for empty/whitespace **`templateId`** and **`seedBucket`**; whitespace path **404** test added. -3. **Optional Postgres `ListForPlayer` round-trip** — Plan marks this optional; if CI Postgres is stable, a `[RequirePostgresFact]` mirroring NEO-146 store tests would guard SQL ordering/cap behavior separately from in-memory parity. +3. ~~**Optional Postgres `ListForPlayer` round-trip** — Plan marks this optional; if CI Postgres is stable, a `[RequirePostgresFact]` mirroring NEO-146 store tests would guard SQL ordering/cap behavior separately from in-memory parity.~~ **Done.** — `ListForPlayer_ShouldPrependActiveAndCapCompletedRows_OnPostgres` in `ContractInstancePersistenceIntegrationTests`. ## Nits -- Nit: **`PostgresContractInstanceStore.ListForPlayer`** opens a connection then calls **`TryGetActiveForPlayer`**, which opens a second connection. Fine at prototype volume; could inline the active SELECT on the existing connection later. -- Nit: **`GetContracts_ShouldReturnCompletionRewardSummary_WhenContractCompletedAfterEncounterClear`** invokes **`TryCompleteOnEncounterClear`** under **`// Act`** before the GET — consider moving completion into **`// Arrange`** so Act is HTTP-only (AAA clarity). -- Nit: Plan §1 Postgres snippet mentions **`completed_at DESC`** but kickoff + README + implementation use **`issuedAt` desc** — update the plan technical section on merge to remove the stale SQL note (implementation is correct). +- ~~Nit: **`PostgresContractInstanceStore.ListForPlayer`** opens a connection then calls **`TryGetActiveForPlayer`**, which opens a second connection. Fine at prototype volume; could inline the active SELECT on the existing connection later.~~ **Done.** — active SELECT inlined on the list connection. +- ~~Nit: **`GetContracts_ShouldReturnCompletionRewardSummary_WhenContractCompletedAfterEncounterClear`** invokes **`TryCompleteOnEncounterClear`** under **`// Act`** before the GET — consider moving completion into **`// Arrange`** so Act is HTTP-only (AAA clarity).~~ **Done.** +- ~~Nit: Plan §1 Postgres snippet mentions **`completed_at DESC`** but kickoff + README + implementation use **`issuedAt` desc** — update the plan technical section on merge to remove the stale SQL note (implementation is correct).~~ **Done.** ## Verification diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractApiTests.cs index d0beca4..63c2993 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractApiTests.cs @@ -34,6 +34,51 @@ public sealed class ContractApiTests Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); } + [Fact] + public async Task PostContractIssue_ShouldReturnNotFound_WhenPlayerIdIsWhitespaceOnly() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + // Act + var response = await client.PostAsync( + "/game/players/%20/contracts/issue", + JsonContent.Create(CreateIssueRequest(PlayerId))); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Theory] + [InlineData("", SeedBucket)] + [InlineData(" ", SeedBucket)] + [InlineData(TemplateId, "")] + [InlineData(TemplateId, " ")] + public async Task PostContractIssue_ShouldReturnBadRequest_WhenRequiredSeedFieldsEmpty( + string templateId, + string seedBucket) + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + // Act + var response = await client.PostAsync( + $"/game/players/{PlayerId}/contracts/issue", + JsonContent.Create( + new ContractIssueRequest + { + SchemaVersion = ContractIssueRequest.CurrentSchemaVersion, + PlayerId = PlayerId, + TemplateId = templateId, + SeedBucket = seedBucket, + })); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + [Fact] public async Task PostContractIssue_ShouldReturnBadRequest_WhenPlayerIdMismatch() { @@ -211,8 +256,6 @@ public sealed class ContractApiTests var issueBody = await issue.Content.ReadFromJsonAsync(); Assert.NotNull(issueBody?.Contract); var instanceId = issueBody!.Contract!.ContractInstanceId; - - // Act var completed = ContractCompletionOperations.TryCompleteOnEncounterClear( PlayerId, EncounterId, @@ -230,10 +273,12 @@ public sealed class ContractApiTests deps.StandingStore, deps.AuditStore, deps.TimeProvider); + Assert.True(completed.Success); + + // Act var response = await client.GetAsync($"/game/players/{PlayerId}/contracts"); // Assert - Assert.True(completed.Success); Assert.Equal(HttpStatusCode.OK, response.StatusCode); var body = await response.Content.ReadFromJsonAsync(); Assert.NotNull(body); diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractInstancePersistenceIntegrationTests.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractInstancePersistenceIntegrationTests.cs index 008f1a0..551c721 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/ContractInstancePersistenceIntegrationTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/ContractInstancePersistenceIntegrationTests.cs @@ -113,6 +113,55 @@ public sealed class ContractInstancePersistenceIntegrationTests(PostgresIntegrat Assert.Equal(firstSnapshot.ContractInstanceId, secondSnapshot.ContractInstanceId); } + [RequirePostgresFact] + public async Task ListForPlayer_ShouldPrependActiveAndCapCompletedRows_OnPostgres() + { + // Arrange + await ResetContractInstanceTableAsync(); + using var scope = Factory.Services.CreateScope(); + var store = scope.ServiceProvider.GetRequiredService(); + for (var i = 0; i < 12; i++) + { + var instanceId = $"prototype_contract_instance_pg_done_{i:D2}"; + Assert.True(store.TryCreateActive( + PlayerId, + instanceId, + TemplateId, + SeedBucket, + IssuedAt.AddHours(i), + out _)); + Assert.True(store.TryMarkComplete( + PlayerId, + instanceId, + CompletedAt.AddHours(i), + out _)); + } + + const string activeId = "prototype_contract_instance_pg_active"; + Assert.True(store.TryCreateActive( + PlayerId, + activeId, + TemplateId, + SeedBucket, + IssuedAt.AddDays(3), + out _)); + + // Act + var rows = store.ListForPlayer(PlayerId, maxCompletedRows: 10); + + // Assert + Assert.Equal(11, rows.Count); + Assert.Equal(activeId, rows[0].ContractInstanceId); + Assert.Equal(ContractInstanceStatus.Active, rows[0].Status); + Assert.All(rows.Skip(1), row => Assert.Equal(ContractInstanceStatus.Completed, row.Status)); + Assert.DoesNotContain( + rows.Skip(1), + row => string.Equals(row.ContractInstanceId, "prototype_contract_instance_pg_done_00", StringComparison.Ordinal)); + Assert.DoesNotContain( + rows.Skip(1), + row => string.Equals(row.ContractInstanceId, "prototype_contract_instance_pg_done_01", StringComparison.Ordinal)); + } + [RequirePostgresFact] public async Task CreateComplete_ShouldPersistAcrossNewFactory() { diff --git a/server/NeonSprawl.Server/Game/Contracts/ContractIssueApi.cs b/server/NeonSprawl.Server/Game/Contracts/ContractIssueApi.cs index 4877e38..502e6b3 100644 --- a/server/NeonSprawl.Server/Game/Contracts/ContractIssueApi.cs +++ b/server/NeonSprawl.Server/Game/Contracts/ContractIssueApi.cs @@ -21,6 +21,12 @@ public static class ContractIssueApi IFactionStandingStore standingStore, IRewardDeliveryStore deliveryStore, TimeProvider timeProvider) => { + var trimmedId = id.Trim(); + if (trimmedId.Length == 0 || !positions.TryGetPosition(trimmedId, out _)) + { + return Results.NotFound(); + } + if (body is null || body.SchemaVersion != ContractIssueRequest.CurrentSchemaVersion) { return Results.BadRequest(); @@ -36,19 +42,12 @@ public static class ContractIssueApi return Results.BadRequest(); } - var trimmedId = id.Trim(); var normalizedPathPlayerId = ContractInstanceIds.NormalizePlayerId(trimmedId); - if (normalizedPathPlayerId.Length == 0 || - !string.Equals(normalizedBodyPlayerId, normalizedPathPlayerId, StringComparison.Ordinal)) + if (!string.Equals(normalizedBodyPlayerId, normalizedPathPlayerId, StringComparison.Ordinal)) { return Results.BadRequest(); } - if (!positions.TryGetPosition(trimmedId, out _)) - { - return Results.NotFound(); - } - var result = ContractGeneratorOperations.TryIssue( trimmedId, normalizedTemplateId, diff --git a/server/NeonSprawl.Server/Game/Contracts/PostgresContractInstanceStore.cs b/server/NeonSprawl.Server/Game/Contracts/PostgresContractInstanceStore.cs index d4f481e..56ce4ae 100644 --- a/server/NeonSprawl.Server/Game/Contracts/PostgresContractInstanceStore.cs +++ b/server/NeonSprawl.Server/Game/Contracts/PostgresContractInstanceStore.cs @@ -331,9 +331,25 @@ public sealed class PostgresContractInstanceStore(Npgsql.NpgsqlDataSource dataSo } ContractInstanceState? active = null; - if (TryGetActiveForPlayer(player, out var activeSnapshot)) + using (var activeCmd = new Npgsql.NpgsqlCommand( + """ + SELECT contract_instance_id, template_id, player_id, status, seed_bucket, issued_at, completed_at + FROM contract_instance + WHERE player_id = @pid AND status = 'active' + LIMIT 1; + """, + conn)) { - active = activeSnapshot; + activeCmd.Parameters.AddWithValue("pid", player); + using var reader = activeCmd.ExecuteReader(); + if (reader.Read()) + { + var snapshot = ReadSnapshot(reader); + if (snapshot.Status == ContractInstanceStatus.Active) + { + active = snapshot; + } + } } var completed = new List(maxCompletedRows);