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}"; } - }