--- name: code-review description: > Review pull requests and code changes for OpenCoreMMO against the project's architecture, conventions, and style defined in AGENTS.md. Covers DDD layering, naming, testing policy, security, CI/CD, Lua scripting, and the EventAggregator / Factory / DI patterns. Works with the GitHub CLI for PR review workflows. compatibility: Requires GitHub CLI (`gh`) for PR review workflows. Designed for NeoServer project conventions. --- ## Scope This skill enforces the conventions documented in `AGENTS.md` during code review. Use it when: - Reviewing a pull request (yours or someone else's) - Auditing code for consistency before committing - Checking whether a change follows project architecture - Writing a PR review comment that cites project policy ## Policy **The review never modifies code.** It only produces feedback: | Mode | Where feedback goes | |---|---| | **Review PR** | Inline comments on each changed file **plus** a general summary comment on the PR | | **Review Staging** | A temporary `review.md` file outside the project folder (system temp directory) | ## Cross-References For deeper guidance on specific areas, use these sibling skills: | Area | Skill | |---|---| | Writing unit tests | `unit-testing` (tests/NeoServer.Domain.Tests etc.) | | Writing functional/integration tests | `functional-testing` (tests/NeoServer.Server.Tests) | | Opening a new PR | `pull-request` (branch naming, PR template) | --- ## Choosing a Mode Pick one before starting: | Mode | When to use | |---|---| | **Review PR** | Someone opened a pull request. You fetch it, inspect, and leave feedback. | | **Review Staging** | You have local changes (staged or unstaged) and want to audit them before committing. | --- ### Review PR Feedback goes to **each changed file** (inline) **plus** a general summary on the PR. Will never modify code. - [ ] 1. **Fetch the PR** — `gh pr checkout ` - [ ] 2. **Build** — `dotnet build src/Standalone --configuration Release` - [ ] 3. **Run tests** — `dotnet test tests/` (or single project if the change is scoped) - [ ] 4. **Get changed files** — `gh pr view --json files --jq '.files[].path'` - [ ] 5. **Walk the checklist** below. For each finding, record: - File path and line number - Which checklist item was violated - Suggestion (reference a template from Common Review Comments) - [ ] 6. **Post inline comments** per file via `gh api` (see CLI Reference) - [ ] 7. **Post general summary** on the PR via `gh pr review --comment --body "$(cat review-summary.md)"` --- ### Review Staging Use before committing to catch issues early. Works on staged and/or unstaged changes. Writes feedback to a temp file — never modifies project files. - [ ] 1. **Check status** — `git status` to see what's staged and unstaged - [ ] 2. **Review the diff** - Staged: `git diff --staged` - Unstaged: `git diff` - Both: `git diff HEAD` - [ ] 3. **List new files** — `git diff --staged --name-status` or `git ls-files --others --exclude-standard` for untracked - [ ] 4. **Build** — `dotnet build src/Standalone --configuration Release` - [ ] 5. **Run tests** — `dotnet test tests/` (or the relevant project) - [ ] 6. **Walk the checklist** below against the diff - [ ] 7. **Write feedback** to a temp file outside the project: ```bash # Windows echo "# Review Findings - $(date)" > "%TEMP%\review-opencoremmo.md" echo. >> "%TEMP%\review-opencoremmo.md" git diff HEAD >> "%TEMP%\review-opencoremmo.md" # then append findings from the checklist walk # Linux/macOS echo "# Review Findings - $(date)" > /tmp/review-opencoremmo.md echo "" >> /tmp/review-opencoremmo.md git diff HEAD >> /tmp/review-opencoremmo.md ``` The file is disposable — read it, address issues locally, then delete. --- ## Review Checklist ### 1. Architecture & Layering No domain project (`NeoServer.Domain`) may depend on infrastructure (networking, database, IoC). - [ ] **Domain purity** — Does `NeoServer.Domain` reference any `NeoServer.Data`, `NeoServer.Networking`, or IoC types? If so, it must be moved. - [ ] **Layer direction** — Dependencies flow inward (Infrastructure → Application → Domain). No inward → outward references. - [ ] **Domain services** — Business logic lives in domain services (`*Service`), not in entities or handlers. Entities own identity and state, not workflows. - [ ] **EventAggregator** — Cross-component communication uses `IEventAggregator.Invoke()`, not direct handler-to-handler calls. Events are `record` types implementing `IEvent`. - [ ] **Handler order** — Network handlers implement `INetworkingEventHandler`; application handlers implement `IApplicationEventHandler`. Network handlers fire first. Verify the correct interface is used. - [ ] **Factory creation** — Game entities (items, creatures, monsters, NPCs, tiles) are created through their factory (`IItemFactory`, `ICreatureFactory`, etc.), not directly with `new`. Factories handle wiring and data store lookups. ### 2. Dependency Injection All game services are **singletons**. - [ ] **Registration** — Is every new service/factory/handler registered in the correct module under `src/Standalone/IoC/Modules/`? (`ServiceInjection.cs`, `FactoryInjection.cs`, `EventInjection.cs`, etc.) - [ ] **Module placement** — Does the registration belong in the existing module or needs a new one? - [ ] **Lifetime** — Verify singletons don't hold request-scoped state (game services are singletons by policy). - [ ] **Interface extraction** — Does the service have an `I*` interface consumed by other layers? ### 3. Naming & Code Style - [ ] **Namespaces** — Match folder structure exactly. Root: `NeoServer.*`. - [ ] **Interface prefix** — Always `I` (e.g., `IItemMovementService`, not `ItemMovementServiceInterface`). - [ ] **Suffix conventions**: | Type | Suffix | Example | |---|---|---| | Event record | `Event` | `PlayerWalkedEvent` | | Event handler | `EventHandler` | `PlayerWalkedEventHandler` | | Service | `Service` | `DecayService` | | Factory | `Factory` | `MonsterFactory` | | Data store | `Store` | `ItemTypeStore` | | Loader | `Loader` | `ItemTypeLoader` | | Routine | `Routine` | `GameCreatureRoutine` | - [ ] **File-scoped namespaces** — Use `namespace NeoServer.X.Y;` not block-scoped `namespace ... { }`. - [ ] **Records for DTOs** — Events and data-transfer types use `record`, not `class`. - [ ] **Composition over inheritance** — Entity behavior is composed via injected services, not base class hierarchy. - [ ] **No magic strings/numbers** — Configuration values come from `appsettings.json` via the options pattern. ### 4. Testing - [ ] **Tests added or updated** — Does the PR include tests? New features and bug fixes must be covered. - [ ] **xUnit + FluentAssertions** — No NUnit, MSTest, or raw `Assert.Equal()`. Use `Should().BeX()`. - [ ] **AAA structure** — Every `[Fact]` is Arrange-Act-Assert clearly separated by blank lines. - [ ] **Behavioral naming** — Format: `{Actor}_does_{something}_when_{condition}` (e.g., `Player_cannot_push_null_creature`). - [ ] **Test isolation** — No shared objects between tests. Each test creates its own instances. - [ ] **Mocking policy** — Domain entities, services, and commands are real objects. Only mock `I*Repository` and infrastructure interfaces (`IConnection`, `IEventAggregator`). - [ ] **Test project placement** — Does the test file live in the correct test project under `tests/`? The folder structure mirrors `src/`: - Domain → `tests/NeoServer.Domain.Tests` - Networking → `tests/NeoServer.Networking.Tests` - Server → `tests/NeoServer.Server.Tests` - etc. - [ ] **Traits** — `[Trait("Category", "")]` present on each test. - [ ] **Build + test pass** — Run `dotnet test tests/` before approving. ### 5. Security - [ ] **RSA key** — New code must not hardcode or commit the RSA key. It stays in `data/key.pem` and is loaded via `Rsa.LoadPem()`. - [ ] **Credentials** — Database passwords and secrets come from environment variables, not `appsettings.json` or code. - [ ] **Input validation** — Network packets, login, chat, trade, and any user-facing input is validated before processing. Invalid input is rejected, not silently ignored. - [ ] **Permissions** — Player actions respect group/permission checks (`groups.json`). Verify new commands check `Player.AccessLevel` or equivalent. ### 6. In-Memory Data Stores - [ ] **Immutability** — Data stores (`ItemTypeStore`, `MonsterTypeStore`, etc.) are populated at startup by loaders and are read-only at runtime. No new code may mutate them after load. - [ ] **Thread safety** — If a new store is added, verify it uses appropriate concurrency protection (or stays immutable after initialization). ### 7. Dispatcher / Scheduler / Persistence - [ ] **Thread affinity** — Game actions go through `IDispatcher`. Time-based events through `IScheduler`. Database writes through `IPersistenceDispatcher`. No raw `Task.Run` or `new Thread` for game logic. - [ ] **Blocking** — Avoid blocking the dispatcher thread with long operations. ### 8. Lua Scripting (if applicable) - [ ] **Hot-reload** — Lua scripts in `data/scripts/` must be safe to reload at runtime. No hard state assumptions in script-side globals. - [ ] **Security sandbox** — Lua bindings must not expose dangerous host operations (file I/O, process execution, raw SQL) to scripts. ### 9. Configurability - [ ] **New settings** — If adding a configurable value, does it go into `appsettings.json` under the appropriate section (`server`, `game`, `client`, `database`)? - [ ] **Database provider** — New data-access code must support all active providers (`INMEMORY`, `SQLITE`, `POSTGRESQL`) or document the limitation. ### 10. Commit Hygiene - [ ] **Conventional commits** — PR commits use `feat:`, `fix:`, `refactor:`, `test:`, `chore:`, etc. - [ ] **Single concern** — Each commit does one thing. No mixed refactor + feature in one commit. --- ## Common Review Comments These templates work for both delivery paths: - **Per-file (inline)** — Replace `[file]` and `[line]` with the actual location, post via `gh api` - **General summary** — Collect all findings into one markdown block, post via `gh pr review --comment --body` - **Staging temp file** — Paste findings into `review-opencoremmo.md` ### Domain dependency leak > **Architecture:** `NeoServer.Domain` must not depend on infrastructure projects. The reference to `NeoServer.Data` / `NeoServer.Networking` in `[file]` violates DDD layering. Move this logic to an application service or inject the dependency via an interface defined in the domain. ### Missing factory usage > **Pattern:** Game entities should be created through their factory (`IItemFactory`, `ICreatureFactory`, etc.) rather than with `new`. Factories handle data store lookups and proper initialization. Please use `IItemFactory.CreateItem(typeId, location)` here. ### Wrong handler interface > **Design:** Network handlers implement `INetworkingEventHandler` (higher priority), application handlers implement `IApplicationEventHandler`. This handler is in the networking layer but implements `IApplicationEventHandler` — use `INetworkingEventHandler` instead. ### Missing DI registration > **DI:** The new service `[Name]` is not registered in any IoC module under `src/Standalone/IoC/Modules/`. Add it to the appropriate module (e.g., `ServiceInjection.cs` for game services). ### Test uses mocks for domain types > **Testing policy:** Domain entities, services, and commands should be real objects in tests. Only mock `I*Repository` and infrastructure interfaces (`IConnection`, `IEventAggregator`). Replace the mock of `[DomainType]` with a real instance. ### No tests > **Testing:** This change introduces new behavior but no test coverage. Please add tests — see `AGENTS.md` for conventions (AAA, behavioral naming, xUnit + FluentAssertions). ### Test isolation violation > **Testing:** Tests must not share mutable objects. Each `[Fact]` should create its own instances. Shared state between tests leads to order-dependent failures. ### Missing Trait > **Style:** Every `[Fact]` should include `[Trait("Category", "")]`. Valid values: `HappyPath`, `Validation`, `EdgeCase`, `ErrorCondition`, `Integration`. ### Data store mutation after load > **Architecture:** `[StoreName]` is populated at startup and treated as read-only at runtime. This code mutates store contents after initialization, which is unsafe. Load the data at startup or add a new store. ### Wrong thread for game action > **Threading:** Game actions must be dispatched through `IDispatcher`. Direct `Task.Run` or background threads bypass the game loop and cause race conditions on game state. --- ## CLI Reference ### PR Review — Inline Comments (per file) Use `gh api` to post inline review comments on specific lines of specific files. Requires the PR number, commit SHA, file path, line number, and comment body. ```bash # Get the latest commit SHA on the PR SHA=$(gh pr view --json headRefOid --jq '.headRefOid') # Post one inline comment per API call gh api repos/:owner/:repo/pulls//comments \ --field body="**Architecture:** `NeoServer.Domain` must not depend on infrastructure..." \ --field commit_id="$SHA" \ --field path="src/NeoServer.Domain/SomeEntity.cs" \ --field line=42 # For multi-line, add --field start_line=41 --field start_side="RIGHT" --field side="RIGHT" ``` Key fields: | Field | Value | |---|---| | `body` | Comment text (use template from Common Review Comments) | | `commit_id` | SHA of the commit the file is at (use `headRefOid`) | | `path` | File path relative to repo root | | `line` | Line number the comment targets | | `side` | `LEFT` (old) or `RIGHT` (new diff side) | | `start_line` | For multi-line comments, the first line | ### PR Review — General Summary ```bash # Post a single general summary comment on the PR gh pr review --comment --body "$(cat review-summary.md)" # Or approve with summary gh pr review --approve --body "$(cat review-summary.md)" # Or request changes with summary gh pr review --request-changes --body "$(cat review-summary.md)" # Check CI status gh pr checks ``` ### Quick file list ```bash gh pr view --json files --jq '.files[].path' gh pr view --json files --jq '.files[] | {path, additions, deletions, status}' ``` ### Staging Review — Temp File ```bash # What's changed git status git diff --name-status HEAD # Full diffs git diff --staged # staged only git diff # unstaged only git diff HEAD # all changes # New untracked files git ls-files --others --exclude-standard # Per-file review git diff HEAD -- src/NeoServer.Domain/SomeFile.cs # Write review feedback to temp file (outside project) # Windows: set "OUTFILE=%TEMP%\review-opencoremmo.md" ( echo # Review Findings - %DATE% echo. echo ## Files Changed echo. git diff --name-status HEAD echo. echo ## Findings echo. echo - Architecture: OK echo - Naming: OK echo - Testing: Missing tests for new service ) > "%OUTFILE%" # Linux/macOS: OUTFILE=/tmp/review-opencoremmo.md cat > $OUTFILE << 'EOF' # Review Findings ## Files Changed ## Findings - Architecture: OK - Naming: OK - Testing: Missing tests for new service EOF # Read the file, act on findings, then delete rm "%TEMP%\review-opencoremmo.md" 2>nul ```