75 lines
4.9 KiB
Markdown
75 lines
4.9 KiB
Markdown
# Code review — NS-15 (authoritative position read API)
|
|
|
|
## Review metadata
|
|
|
|
| Field | Value |
|
|
|--------|--------|
|
|
| **Date** | 2026-03-29 |
|
|
| **Scope** | Branch `NS-15-position-state-api` vs `origin/main`, plus **working tree**: modified tracked files and **untracked** implementation/tests/agent files (full change set as present locally) |
|
|
| **Base** | `origin/main` |
|
|
| **Issue** | NS-15 — E1.M1: Authoritative PositionState API (in-memory) |
|
|
|
|
---
|
|
|
|
## Verdict
|
|
|
|
**Request changes** (for merge readiness). Implementation quality is **Approve with nits**; the **request changes** is because the feature and tests are largely **not yet in git** relative to `origin/main` (untracked paths), so a PR built only from pushed commits would be incomplete.
|
|
|
|
## Summary
|
|
|
|
The NS-15 work adds an in-memory `IPositionStateStore`, `Game` configuration for a dev player id and default position, `PositionStateApi.MapPositionStateApi()` for `GET /game/players/{id}/position`, XML-documented DTOs with `schemaVersion`, integration tests via `WebApplicationFactory<Program>`, and `Program.Waf.cs` for the test host. `Program.cs` stays thin (DI + map extension calls only), matching `docs/plans/NS-15-implementation-plan.md`. README documents `curl` and sample JSON. Separately, `.cursor/rules/csharp-style.md` gains primary-constructor guidance, and the code-review agent + `AGENTS.md` add process docs. Overall risk is **low** for a read-only spike; no persistence or client integration.
|
|
|
|
## Blocking issues
|
|
|
|
1. **Version control gap** — As of this review, `server/NeonSprawl.Server/Game/`, `server/NeonSprawl.Server.Tests/`, `server/NeonSprawl.Server/Program.Waf.cs`, `.cursor/rules/code-review-agent.md`, and `AGENTS.md` are **untracked**, while `Program.cs`, `appsettings.json`, `server/README.md`, `NeonSprawl.sln`, `docs/plans/NS-15-implementation-plan.md`, and `.cursor/rules/csharp-style.md` are **modified but uncommitted**. **Before merge:** `git add` all intended files and push so CI/reviewers see the real diff.
|
|
|
|
## Suggestions
|
|
|
|
1. **Null-safe `DefaultPosition`** — In `InMemoryPositionStateStore`, configuration binding could theoretically set `GamePositionOptions.DefaultPosition` to `null`. Use `var p = o.DefaultPosition ?? new DefaultPositionOptions();` before reading `p.X` / `p.Y` / `p.Z` to avoid a startup `NullReferenceException`.
|
|
|
|
2. **Case sensitivity** — Lookups use `StringComparer.Ordinal`. Document that player ids in URLs are case-sensitive, or normalize route id and stored keys the same way if product expects case-insensitivity.
|
|
|
|
3. **Implementation plan markdown** — In `docs/plans/NS-15-implementation-plan.md`, the acceptance section mixes raw `**` markers with bullets (e.g. `` `**dotnet test**` ``) and no longer uses `- [ ]` checkboxes. Restoring valid Markdown and checkboxes improves scanability.
|
|
|
|
4. **Solution file noise** — If `NeonSprawl.sln` picks up a UTF-8 BOM or Windows-style path separators, consider normalizing to reduce cross-platform diff churn (non-functional).
|
|
|
|
5. **PR description** — When opening the PR, paste the sample JSON contract (as in README) so Jira AC (“document field names”) is satisfied outside the repo alone.
|
|
|
|
## Nits
|
|
|
|
- **Nit:** `SchemaVersion` is set to `1` in both `PositionStateResponse` and the handler; a single named constant could prevent drift.
|
|
- **Nit:** No logging on 404 vs success—acceptable for NS-15; revisit when debugging multi-player seeds.
|
|
|
|
## Verification
|
|
|
|
Run from repo root (**.NET 10 SDK** on `PATH`):
|
|
|
|
```bash
|
|
git status # expect clean after add/commit, no missing Game/ or Tests/
|
|
dotnet build NeonSprawl.sln
|
|
dotnet test NeonSprawl.sln
|
|
```
|
|
|
|
Manual:
|
|
|
|
```bash
|
|
cd server/NeonSprawl.Server && dotnet run
|
|
# elsewhere:
|
|
curl -s http://localhost:5253/game/players/dev-local-1/position
|
|
```
|
|
|
|
Expect HTTP 200 and JSON including `schemaVersion`, `playerId`, `position` with `x`/`y`/`z`, and `sequence`. Unknown id should return 404.
|
|
|
|
## Files touched (review scope)
|
|
|
|
**Modified (tracked):** `.cursor/rules/csharp-style.md`, `NeonSprawl.sln`, `docs/plans/NS-15-implementation-plan.md`, `server/NeonSprawl.Server/Program.cs`, `server/NeonSprawl.Server/appsettings.json`, `server/README.md`
|
|
|
|
**Untracked (must be added for a complete PR):** `.cursor/rules/code-review-agent.md`, `AGENTS.md`, `server/NeonSprawl.Server/Program.Waf.cs`, `server/NeonSprawl.Server/Game/PositionState/*.cs`, `server/NeonSprawl.Server.Tests/*`
|
|
|
|
## Conformance notes
|
|
|
|
- **architecture-authority:** Server-held in-memory state, HTTP read spike; no client or move submission—matches NS-15 out-of-scope.
|
|
- **testing-expectations:** First server module gets `NeonSprawl.Server.Tests` with integration-style coverage for happy path and 404—appropriate.
|
|
- **csharp-style:** Test class uses primary constructor and `using Xunit;`—aligned with updated rule.
|
|
- **Plan alignment:** Separate `PositionStateApi`, `PositionStateServiceCollectionExtensions`, no position routes inlined in `Program.cs`—matches plan.
|