From 299de3c79633afef36f0de7b420e31815ec90357 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 27 Jun 2026 23:24:18 -0400 Subject: [PATCH] chore: address cleanup review follow-ups and Postgres test flake Fix gig persistence integration flake with FakeTimeProvider on the Postgres host, restore StringComparer.Ordinal on quest counter copies, remove the Bruno docs block, and record resolved review items. --- bruno/neon-sprawl-server/collection.bru | 4 -- docs/reviews/2026-06-27-chore-cleanup.md | 57 +++++++++++++++++++ ...ressionGrantPersistenceIntegrationTests.cs | 22 +++++-- .../PostgresWebApplicationFactory.cs | 20 +++++++ .../NeonSprawl.Server.Tests/GlobalUsings.cs | 1 + .../Quests/PostgresPlayerQuestStateStore.cs | 3 +- .../Game/Quests/QuestProgressIds.cs | 1 - 7 files changed, 98 insertions(+), 10 deletions(-) create mode 100644 docs/reviews/2026-06-27-chore-cleanup.md diff --git a/bruno/neon-sprawl-server/collection.bru b/bruno/neon-sprawl-server/collection.bru index 67ab726..1ea72f1 100644 --- a/bruno/neon-sprawl-server/collection.bru +++ b/bruno/neon-sprawl-server/collection.bru @@ -1,7 +1,3 @@ meta { name: Neon Sprawl Server } - -docs { - chore/cleanup analyzer pass: no HTTP route or request-body changes. -} diff --git a/docs/reviews/2026-06-27-chore-cleanup.md b/docs/reviews/2026-06-27-chore-cleanup.md new file mode 100644 index 0000000..1e67b05 --- /dev/null +++ b/docs/reviews/2026-06-27-chore-cleanup.md @@ -0,0 +1,57 @@ +# Code review — chore/cleanup analyzer pass + +**Date:** 2026-06-27 +**Scope:** Branch `chore/cleanup` — commit `b233fa0` vs `2cf8bcd` (merge base on `main`) +**Base:** `2cf8bcd` (merge of PR #190) + +**Follow-up:** Suggestions and nits below are **done** (strikethrough + **Done.**). + +## Verdict + +**Approve with nits** + +## Summary + +This is a broad mechanical refactor across `NeonSprawl.Server` and `NeonSprawl.Server.Tests` (231 files, net −524 lines): collection expressions, primary-constructor catalog types, concrete private helper return types (CA1859), logging `IsEnabled` guards (CA1873), record XML doc fixes (RCS1263/RCS1139), `nameof` for SQL parameters, quest-store mutator delegate → `Func` + `MutateRowResult` record struct, removal of redundant `using` directives via new `GlobalUsings.cs`, and a substantial `csharp-style.md` appendix documenting the patterns. No HTTP routes, request bodies, or game contracts change. Build is clean (0 warnings). Full `dotnet test` passed **905/906** locally with one intermittent Postgres integration failure under parallel load; the same Postgres specs pass in isolation on this branch and on the parent commit. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/` (NEO-* / E7M4 stories) | **N/A** — unticketed `chore:` branch; no feature acceptance criteria | +| `.cursor/rules/csharp-style.md` | **Matches** — new sections document patterns applied in this diff (collection expressions, CA1859/CA1873, RCS1015, record XML docs, test GlobalUsings) | +| `docs/decomposition/modules/module_dependency_register.md` | **N/A** — no module behavior or authority changes | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **N/A** — no implementation-status update required | +| `bruno/neon-sprawl-server/collection.bru` | **Matches** — ~~Partially matches~~ docs block removed (**Done.**) | + +## Blocking issues + +None. + +## Suggestions + +1. ~~**Remove the Bruno `docs { }` block** in `bruno/neon-sprawl-server/collection.bru`. The note (“chore/cleanup analyzer pass: no HTTP route or request-body changes”) belongs in the PR/commit message, not in the Bruno collection metadata consumers may read as API documentation.~~ **Done.** Removed the `docs { }` block from `collection.bru`. + +2. ~~**Preserve explicit `StringComparer.Ordinal` in quest objective counter copies.** In `PostgresPlayerQuestStateStore.MutateObjectiveCounter`, the old code used `new Dictionary(row.ObjectiveCounters, StringComparer.Ordinal)`. The new `ToDictionary` call relies on default string equality (ordinal in practice). For consistency with the rest of the quests module and defense-in-depth, pass `StringComparer.Ordinal` to `ToDictionary` or keep the dictionary copy constructor.~~ **Done.** `ToDictionary(..., StringComparer.Ordinal)` in `PostgresPlayerQuestStateStore.cs`. + +3. ~~**Confirm Postgres integration stability under parallel xUnit.** Full-suite runs intermittently fail one of `FactionStandingPersistenceIntegrationTests`, `GigProgressionGrantPersistenceIntegrationTests`, or `SkillProgressionGrantPersistenceIntegrationTests` (expected persisted value vs `0`), while filtered runs pass on both `2cf8bcd` and `b233fa0`. This looks like pre-existing collection/fixture contention rather than a logic regression from this diff, but worth a quick check that `[Collection("Postgres integration")]` isolation is sufficient before merge if CI runs the full suite in parallel.~~ **Done.** Root cause for gig flake: `GigProgressionGrantPersistenceIntegrationTests` used wall-clock `Task.Delay` while cooldown uses `TimeProvider`; casts could return HTTP 200 with `on_cooldown` (`Accepted: false`) without defeat. Fixed by registering `FakeTimeProvider` on `PostgresWebApplicationFactory` and advancing the fake clock in the gig persistence test (same pattern as `InMemoryWebApplicationFactory`). Full suite **906/906** after fix. + +## Nits + +- ~~Nit: `GlobalUsings.cs` pulls in domain namespaces (`NeonSprawl.Server.Game.Npc`, `NeonSprawl.Server.Tests.Game.Npc`, etc.). Works today (0 build warnings), but a one-line file comment listing intentional globals would help future contributors avoid accidental name clashes.~~ **Done.** File header comment added. +- ~~Nit: `QuestProgressIds.cs` has a whitespace-only trailing blank line before the closing brace — harmless; could drop in a follow-up.~~ **Done.** +- Nit: `QuestStateOperations.DenyAccept` no longer threads `playerId` / `questId`; telemetry comment correctly says “capture at call site when wiring E9.M1.” Fine for now — just remember at E9.M1. +- Nit: `NpcRuntimeOperations.AdvanceAll` drops the post–idle→aggro `TryGet` refresh; the while-loop `TryGet` on the first iteration makes this equivalent. No action needed. +- Nit: `PostgresPositionStateStore` drops unused `IOptions` / startup validation — correct dead-code removal; DI registration unchanged. + +## Verification + +```bash +cd server/NeonSprawl.Server && dotnet build +cd server/NeonSprawl.Server.Tests && dotnet test +cd server/NeonSprawl.Server.Tests && dotnet test --filter "FullyQualifiedName~PersistenceIntegrationTests" +``` + +Expect **906** passed; if a single Postgres persistence test fails under full parallel run, re-run filtered as above to distinguish flake from regression. + +Manual Godot QA: none (no client or player-visible surface changes). diff --git a/server/NeonSprawl.Server.Tests/Game/Gigs/GigProgressionGrantPersistenceIntegrationTests.cs b/server/NeonSprawl.Server.Tests/Game/Gigs/GigProgressionGrantPersistenceIntegrationTests.cs index ab11c70..1651673 100644 --- a/server/NeonSprawl.Server.Tests/Game/Gigs/GigProgressionGrantPersistenceIntegrationTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Gigs/GigProgressionGrantPersistenceIntegrationTests.cs @@ -24,7 +24,10 @@ public sealed class GigProgressionGrantPersistenceIntegrationTests(PostgresInteg TargetId = PrototypeNpcRegistry.PrototypeNpcMeleeId, }; - // Act — defeat alpha through first host (four pulses) + // Act — defeat alpha through first host (pulse loop; fake clock avoids wall-clock cooldown flakes) + Assert.NotNull(Factory.FakeClock); + var clock = Factory.FakeClock; + clock.Advance(TimeSpan.FromSeconds(3) + TimeSpan.FromMilliseconds(50)); using (var firstClient = Factory.CreateClient()) { await firstClient.PostAsJsonAsync( @@ -41,15 +44,26 @@ public sealed class GigProgressionGrantPersistenceIntegrationTests(PostgresInteg SchemaVersion = TargetSelectRequest.CurrentSchemaVersion, TargetId = PrototypeNpcRegistry.PrototypeNpcMeleeId, }); - for (var i = 0; i < 4; i++) + AbilityCastResponse? defeatBody = null; + for (var i = 0; i < 12; i++) { var response = await firstClient.PostAsJsonAsync("/game/players/dev-local-1/ability-cast", cast); response.EnsureSuccessStatusCode(); - if (i < 3) + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + Assert.True(body!.Accepted, body.ReasonCode); + Assert.NotNull(body.CombatResolution); + if (body.CombatResolution!.TargetDefeated) { - await Task.Delay(TimeSpan.FromSeconds(3.2)); + defeatBody = body; + break; } + + clock.Advance(TimeSpan.FromSeconds(3) + TimeSpan.FromMilliseconds(50)); } + + Assert.NotNull(defeatBody); + Assert.True(defeatBody!.CombatResolution!.TargetDefeated); } await using var secondFactory = new PostgresWebApplicationFactory(); diff --git a/server/NeonSprawl.Server.Tests/Game/PositionState/PostgresWebApplicationFactory.cs b/server/NeonSprawl.Server.Tests/Game/PositionState/PostgresWebApplicationFactory.cs index 837cee9..55538e2 100644 --- a/server/NeonSprawl.Server.Tests/Game/PositionState/PostgresWebApplicationFactory.cs +++ b/server/NeonSprawl.Server.Tests/Game/PositionState/PostgresWebApplicationFactory.cs @@ -1,6 +1,8 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Time.Testing; using NeonSprawl.Server.Game.Combat; using NeonSprawl.Server.Game.Crafting; using NeonSprawl.Server.Game.Gathering; @@ -12,6 +14,9 @@ namespace NeonSprawl.Server.Tests.Game.PositionState; /// Integration host with bound from environment. public sealed class PostgresWebApplicationFactory : WebApplicationFactory { + /// Controllable UTC clock for cast cooldown integration tests (NEO-32). + public FakeTimeProvider? FakeClock { get; private set; } + protected override void ConfigureWebHost(IWebHostBuilder builder) { var cs = Environment.GetEnvironmentVariable("ConnectionStrings__NeonSprawl"); @@ -60,5 +65,20 @@ public sealed class PostgresWebApplicationFactory : WebApplicationFactory + { + for (var i = services.Count - 1; i >= 0; i--) + { + if (services[i].ServiceType == typeof(TimeProvider)) + { + services.RemoveAt(i); + } + } + + var fakeTime = new FakeTimeProvider(new DateTimeOffset(2026, 4, 30, 12, 0, 0, TimeSpan.Zero)); + FakeClock = fakeTime; + services.AddSingleton(fakeTime); + }); } } diff --git a/server/NeonSprawl.Server.Tests/GlobalUsings.cs b/server/NeonSprawl.Server.Tests/GlobalUsings.cs index 3a5b1f8..f7646f6 100644 --- a/server/NeonSprawl.Server.Tests/GlobalUsings.cs +++ b/server/NeonSprawl.Server.Tests/GlobalUsings.cs @@ -1,3 +1,4 @@ +// Intentional test-project globals (avoid duplicating per-file usings; watch for name clashes with local types). global using Microsoft.Extensions.DependencyInjection; global using Microsoft.Extensions.Logging.Abstractions; global using Microsoft.Extensions.Options; diff --git a/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs b/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs index 0add1d7..e6340f2 100644 --- a/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs +++ b/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs @@ -200,7 +200,8 @@ public sealed class PostgresPlayerQuestStateStore(Npgsql.NpgsqlDataSource dataSo Dictionary counters = row.ObjectiveCounters.ToDictionary( static pair => pair.Key, - static pair => pair.Value); + static pair => pair.Value, + StringComparer.Ordinal); counters[objective] = newCount; return new MutateRowResult( true, diff --git a/server/NeonSprawl.Server/Game/Quests/QuestProgressIds.cs b/server/NeonSprawl.Server/Game/Quests/QuestProgressIds.cs index a1f9a7e..113ff79 100644 --- a/server/NeonSprawl.Server/Game/Quests/QuestProgressIds.cs +++ b/server/NeonSprawl.Server/Game/Quests/QuestProgressIds.cs @@ -33,5 +33,4 @@ public static class QuestProgressIds return $"{p}\0{q}"; } - }