neon-sprawl/docs/reviews/2026-06-27-chore-cleanup.md

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<string, int>(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<GamePositionOptions> / startup validation — correct dead-code removal; DI registration unchanged.

Verification

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).