Code Review

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:

Cross-References

For deeper guidance on specific areas, use these sibling skills:

Choosing a Mode

Pick one before starting:

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 <number>
• 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 <number> --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 <number> --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:

  # 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<T>; application handlers implement IApplicationEventHandler<T>. 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", "<HappyPath|Validation|EdgeCase|ErrorCondition|Integration|...>")] 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.