Instruction file imported from bowlneba/neba-website (.github/instructions/pull-request-review.instructions.md). Copyright stays with the author.

Pull Request Review Guidelines

For GitHub Copilot: Use these guidelines when reviewing pull requests. Flag violations as suggestions for refactoring, not blocking requests. The codebase follows Vertical Slice Architecture, DDD, and CQRS patterns.

Architecture & Feature Boundaries

Domain (Feature Domain Types in Features/*/Domain/)

Treat each feature domain namespace as a separate bounded context. Types under Features/Tournaments/Domain, Features/Bowlers/Domain, Features/BowlingCenters/Domain, etc. should never directly reference each other's domain objects — review as if they were separate assemblies without project references.

Exception — strongly-typed IDs only: A feature domain may import a strongly-typed ID from another feature's domain (e.g., HallOfFame importing BowlerId from Features.Bowlers.Domain) when it needs to record a cross-context foreign key relationship. This is analogous to a database FK: the context stores the identifier, not the object. Importing the full aggregate, entity, value objects, or domain services of another feature is still prohibited.

Do NOT flag internal navigation properties on domain aggregates. These are intentional: EF Core entity configurations in Neba.Api/Database/ live in the same assembly (Neba.Api) and rely on internal access for mapping and query projections. The internal modifier is what prevents external assemblies (Contracts, Website) from using these properties. Flagging them as an accessibility problem misreads the intent.

Do NOT flag ToString() as missing or inherited from Object on [StronglyTypedId("ulid-full")] partial structs (e.g., BowlerId, SeasonId, TournamentId). The override is generated by the ulid-full.typedid template into a source-generated partial. The declaration file intentionally contains only the [StronglyTypedId] attribute — the full implementation including ToString(), GetHashCode(), Equals(), and all interfaces is invisible to PR diff analysis but present at compile time.

Flag when:

• A feature domain folder imports domain objects from another feature domain folder (aggregates, entities, value objects, domain services, or enums — e.g., Tournaments/Domain importing Bowler, not BowlerId)
• Cross-cutting domain base types (AggregateRoot, IDomainEvent) are duplicated inside a feature folder instead of using the shared Neba.Api/Domain/ types
• Domain entities expose public setters or mutable collections
• Aggregates lack domain event support when state changes occur
• Business logic appears outside the domain layer
• A child entity owned by an aggregate does not have an internal static ErrorOr<T> Create(...) factory — every child entity must own its structural invariants through this factory, even if validation is minimal (e.g., non-negative amount). The internal modifier ensures construction is only possible from the same assembly (the aggregate root or InternalsVisibleTo test helpers)
• A child entity owned by an aggregate has a public static Create(...) factory — it should be internal so construction is only possible through the aggregate root (by convention — both live in Neba.Api)
• An aggregate's assign/add method validates child entity invariants directly (e.g., checking blockScore > 0 on Season) instead of delegating to the child entity's internal static Create(...) factory
• A child entity is instantiated directly via new outside the aggregate root — application or test code must go through the aggregate's assign methods
• An application handler computes a domain formula and passes the derived result to an aggregate — raw input data should be passed instead; the formula belongs in the domain (e.g., computing minimumGames = floor(4.5 × count) in a handler rather than passing statEligibleTournamentCount to the aggregate)
• A new aggregate, entity, or value object is introduced without a corresponding entry in docs/ubiquitous-language.md — every new domain type needs a UL definition so the vocabulary stays shared across code, docs, and conversation
• A new aggregate, entity, or value object has an XML <summary> comment that contradicts or omits the purpose described in the UL — comments don't need to be word-for-word matches, but must convey the same concept to an engineer reading the code cold
• An existing domain type is touched and its XML <summary> is absent or misleading relative to its UL entry — take a quick pass over the UL when reviewing domain changes and flag any pre-existing gaps encountered along the way

Handlers (Feature Slice Use Case Folders)

Handlers live at Features/{Feature}/{UseCase}/, co-located with their command/query type and DTO. Each handler implements IQueryHandler<,> or ICommandHandler<,> and injects AppDbContext directly.

Commands must return ErrorOr<T> — flag any command handler that throws exceptions for business rule violations or returns raw types.

Cross-feature data access is allowed in handlers — a handler may query data from multiple feature domains inline (e.g., counting stat-eligible tournaments when assigning a season award). The handler provides facts to the aggregate; the aggregate enforces rules.

Flag when:

• Command handlers don't return ErrorOr<T>
• Query handlers return domain entities instead of DTOs/response types
• Direct instantiation of another feature's aggregates (should go through the aggregate's own factory methods)
• Missing CancellationToken propagation in async methods
• Query handlers use .AsTracking() or omit .AsNoTracking() for read-only operations

Do NOT flag query or command handlers that appear to be simple pass-throughs. All handlers are wrapped by TracedQueryHandlerDecorator / TracedCommandHandlerDecorator, which provides automatic telemetry (activity spans, duration tracking, structured error logging). Bypassing the handler pipeline would lose this observability. See ADR-0003.

Infrastructure (Neba.Api/Database/, Neba.Api/Caching/, etc.)

Infrastructure concerns live in dedicated folders within Neba.Api. There is no repository abstraction — handlers inject AppDbContext directly.

Flag when:

• EF Core entity configurations expose domain-computed properties to the persistence layer (EF configurations should only map columns, not derive business values)
• Raw SQL bypasses EF Core without a clear documented reason
• Feature domain types directly reference EF Core attributes or types (domain types should stay framework-free)

API Layer (Neba.Api)

Structure: Each endpoint lives in a use case folder with endpoint, summary, validator, command/query, and handler:

Neba.Api/Features/Tournaments/CreateTournament/
├── CreateTournamentEndpoint.cs
├── CreateTournamentSummary.cs
├── CreateTournamentValidator.cs
├── CreateTournamentCommand.cs
└── CreateTournamentCommandHandler.cs

Flag when:

• Business logic appears in endpoints (should be in domain types or handler)
• Files in wrong folders (e.g., validator in Contracts project, handler outside its use case folder)
• Missing Summary class
• Mixing concerns (multiple use cases in one folder)
• Handler or DTO defined outside the use case folder it belongs to

Endpoint Configuration

Every endpoint's Configure() method must include:

• HTTP verb and RESTful route
• Group<TEndpointGroup>() configured
• Version() explicitly specified (even if defaulting to 1)
• Authorization explicitly configured (AllowAnonymous(), Roles(), or Policies())
• Tags() with domain and visibility (e.g., "Tournaments", "Authenticated")
• Description() with WithName() (required for OpenAPI)
• Produces()/ProducesProblemDetails() for all status codes

Flag when:

• Authorization not explicitly configured (implicit defaults are not allowed)
• Missing WithName() in Description
• Visibility tag doesn't match authorization (e.g., AllowAnonymous() with "Authenticated" tag)
• Action-based routes (/api/tournaments/create instead of /api/tournaments)
• Missing status code documentation

Validation

Validators should only contain structural validation:

• ✅ Required fields, length constraints, range validation, format validation
• ❌ Cross-property validation (belongs in handler)
• ❌ Database lookups (belongs in handler)
• ❌ Business rules (belongs in domain or handler)

Flag when:

• Validator injects repositories or services
• Validator contains MustAsync with database queries
• Validation logic appears in endpoint handler instead of validator
• Missing validator when request has input to validate

Error Handling

All errors must return ProblemDetails (RFC 9457) via FastEndpoints' built-in UseProblemDetails(). Use AddError() + Send.ErrorsAsync(statusCode) to return ErrorOr<T> errors with the appropriate HTTP status code.

Exception: A bare Send.NotFoundAsync() (HTTP 404 with no body) is acceptable when the 404 status code itself is sufficient documentation of the error — e.g. a simple "document not found" GET endpoint where the caller only needs to know the resource doesn't exist. Do NOT flag this pattern.

Flag when:

• Custom error response bodies used instead of ProblemDetails (for errors other than bare 404)
• Not handling all error cases from ErrorOr<T> result
• Using SendAsync() with custom error objects
• Missing error case handling (assuming success without checking result.IsError)
• Using result.IsFailure or result.Error instead of result.IsError / result.FirstError (ErrorOr API)

Error Codes

Error codes must follow the Entity.ErrorCode convention (PascalCase, dot-separated). See ADR-0004.

Error Types (Error.Validation vs Error.Conflict)

Use the retry test to choose between them: if the caller could retry the exact same request and succeed — without changing their payload — it is a state conflict, not a validation failure.

• Error.Validation (422) — the input itself is structurally wrong; the caller must change their payload to fix it (e.g., a score of 0, a missing required field)
• Error.Conflict (409) — the input is valid but the system's current state prevents the operation (e.g., season not yet closed, bowler already registered)

Flag when:

• Error codes don't follow Entity.ErrorCode pattern (e.g., "documentNotFound" instead of "Document.NotFound")
• Error codes use lowercase or camelCase instead of PascalCase
• Error codes are missing (empty string or generic code)
• Application error classes are not named {Entity}Errors or are not internal static
• Error.Validation is used for a state/precondition failure where the caller could retry unchanged (should be Error.Conflict)
• Error.Conflict is used for a structural input problem (should be Error.Validation)

Summary Classes

Every endpoint needs a Summary class with:

• Short summary and detailed description
• ExampleRequest with realistic data
• Response<T>() examples for all status codes (200/201, 400, 404, 409, etc.)

Flag when:

• Missing Summary class
• Summary too brief (e.g., "Create tournament" instead of full description)
• Missing or unrealistic example data
• Missing response examples for error cases

Mapping

Mapping should be inline in the endpoint (Request → Command, DTO → Response).

Flag when:

• Separate mapper classes created
• AutoMapper or similar libraries used
• Mapping logic is overly complex (may indicate wrong abstraction level)

Contracts Layer (Neba.Api.Contracts)

Structure: Contracts organized by use case folders:

Neba.Api.Contracts/Tournaments/CreateTournament/
├── CreateTournamentRequest.cs
└── TournamentInput.cs

Request wraps Input for commands:

public record CreateTournamentRequest
{
    public TournamentInput Tournament { get; init; } = new();
}

Flag when:

• Request doesn't wrap Input (properties directly on request)
• Contracts organized by type (Requests/, Responses/) instead of use case
• Using { get; set; } instead of { get; init; }
• Missing XML documentation (<summary>, <example> tags)
• Refit interface not updated with new endpoint method

Blazor (Neba.Website.Server / Neba.Website.Client)

Flag when:

• Pages contain business logic (pages should be thin orchestrators)
• Components fetch data directly (should receive via parameters)
• Services don't return ErrorOr<T>
• Components inject services other than UI services (notifications, navigation)
• Feature-specific components placed in generic Components/ folder
• Missing loading state handling
• Components placed in Client project without clear justification (offline, browser APIs, latency-sensitive)
• Data-entry pages/forms (anything with EditForm, file uploads, or similar user input) don't guard against losing unsaved changes on Cancel, in-app navigation, or refresh/close — wrap with <DirtyFormGuard IsDirty="@_isDirty" /> (Components/DirtyFormGuard.razor) and track _isDirty via EditContext.OnFieldChanged plus explicit MarkDirty() calls for anything outside the EditForm (file uploads, non-InputBase bound fields). See CreateArticle.razor for the reference implementation.
• A field bound to a [Required] model property uses a bare <label> instead of <FormLabel TargetId="..." For="@(() => _model.X)"> (Components/FormLabel.razor) — required fields must show a "(required)" tag, not a bare asterisk or no indicator at all. Exception: forms where every field is required (e.g. Login.razor) may keep plain <label> elements, since marking every field adds no information.

REST API Conventions

URL Structure

• Plural nouns only: /tournaments, /bowlers, /bowling-centers
• No verbs in URLs: GET /tournaments/{id} not GET /getTournament/{id}
• Kebab-case for multi-word resources: /bowling-centers not /bowlingCenters
• Nested resources for relationships: /tournaments/{id}/squads

HTTP Methods

Query Parameters

• camelCase: pageSize, sortBy, includeInactive
• Pagination: page (1-indexed), pageSize
• Filtering: Use resource attribute names (status=active, type=senior)

Response Envelopes

Single item:

{
  "data": { "id": "...", "name": "..." }
}

Collection:

{
  "items": [...],
  "totalCount": 100
}

Collection types should be IReadOnlyCollection<T>.

Paginated collection:

{
  "items": [...],
  "totalCount": 100,
  "page": 1,
  "pageSize": 20
}

Error responses: Must use Problem Details format (RFC 7807) for all error status codes.

Flag when:

• Response shapes don't match these envelopes
• Collections use List<T> or IEnumerable<T> instead of IReadOnlyCollection<T> in public API contracts
• Error responses don't use Problem Details
• Pagination uses 0-indexed pages

SmartEnum Serialization

Query/GET responses return the display Name; commands (POST/PUT/PATCH) accept the short Value/code. Example: PhoneNumberType — a GetSponsorDetail response returns "Home" (.Name); a CreateSponsor request accepts "H" (.Value), rehydrated via PhoneNumberType.FromValue(...). This mirrors how a human reads a response versus how a client references a fixed value back.

Named exceptions — code both ways, not a bug:

• UsState, CanadianProvince, Country — GET responses and commands both use the short code ("MA", not "Massachusetts"). The postal/ISO code is itself a meaningful, widely-recognized identifier, not enum plumbing, so there's no round-trip benefit to expanding it.
• Permissions — GET responses (GetCurrentUserResponse.Permissions) return .Value (e.g. "News.CreateArticle"), never .Name. This field is a functional authorization key the client compares against a policy constant (Permissions.CreateArticle.PolicyName) — it's never displayed as text to a user, so the GET-returns-English rule doesn't apply.
• TournamentType, PatternLengthCategory, PatternRatioCategory — GET responses and commands both use .Name/.FromName (e.g. "Tournament of Champions", "Sport"), never .Value. Unlike PhoneNumberType's "H"/"M"/"F"-style codes, these enums' Value is an arbitrary sequential int (100, 101, 102... / 1, 2, 3) with no meaning outside the enum definition itself — it isn't a stable external identifier, just a plumbing detail. Sending it over the wire instead of the name gains no decoupling (renaming the display string wouldn't touch the int, and vice versa) while making requests/responses harder to read and hand-test. Same reasoning as the UsState/CanadianProvince/Country exception: use whichever field is the legitimate stable identifier, and for these three, that's the name.
• Roles — CreateUserInput.Roles/CreateUserCommand.Roles use plain role-name strings ("Webmaster", "Manager", ...), validated against the fixed Roles.All list. This was originally planned as a RoleId reference (roles are ASP.NET Identity rows, ApplicationRole : IdentityRole<Ulid>, not a SmartEnum) but the built flow found Roles.cs is six hardcoded constants seeded at startup with no admin UI to create/rename roles — roles function as a closed, compile-time vocabulary in practice, same shape as a SmartEnum's name, so a name string is the deliberate choice here, not a shortcut. Revisit if roles ever become dynamically DB-managed (an admin UI to create/rename roles) — at that point a name string stops being stable and RoleId becomes the right shape again.

No structural enforcement exists — there's no global JSON converter or analyzer; each handler must apply this by hand (.Name when projecting to a GET DTO, .FromValue/.FromName when parsing a command). This has already drifted once (a stats DTO emitting Gender.Value — caught but not fixed since the field is never actually serialized to a public contract). Flag any new query handler that projects a SmartEnum property via .Value into a public response DTO, or any command handler that parses a SmartEnum via .FromName instead of .FromValue, unless the property falls into one of the exceptions above.

Observability

Logging

Every service and handler should use ILogger<T>. Flag when:

• New services or handlers lack logger injection
• Logging sensitive data (PII, credentials, tokens)
• Missing correlation IDs in log scopes for operations that span multiple steps
• Using wrong log levels:
  • Debug: Development context, verbose details
  • Information: Business events (tournament created, score recorded)
  • Warning: Recoverable issues, degraded functionality
  • Error: Failures requiring attention

Distributed Tracing

Flag when:

• New command handlers lack activity spans for business operations
• Spans missing relevant tags (entity IDs, operation type)
• Cross-boundary operations (HTTP, database, external APIs) aren't traced

Suggest spans for:

• Business-critical operations (score submission, registration, tournament completion)
• Operations involving multiple steps
• Operations where timing breakdown aids debugging

Entity-Level Auditing

EF-level audit (Audit.EntityFramework, configured in Neba.Api/Auditing/AuditingConfiguration.cs) uses UseOptIn() — an entity type produces no audit row on insert/update/delete unless it's explicitly registered via .Include<T>(). There is no compiler or runtime error when this is skipped; the entity just silently goes unaudited.

Flag when:

• A PR adds a new aggregate root or entity under Features/*/Domain/ that should be audited (i.e. it's user-facing content or state, analogous to existing audited types like Bowler, Tournament, Sponsor) but is not added to the .Include<T>() chain in AuditingConfiguration.cs

Not every new entity needs this — use judgment (e.g. pure value objects or entities owned entirely by an already-audited aggregate don't need their own Include<T>()), but any new top-level aggregate root should be checked against the list.

Metrics

Look for opportunities to suggest metrics:

• Counters: Business events (registrations, scores recorded), errors by type
• Histograms: Operation durations, response times

Flag when:

• Critical business operations lack metric instrumentation
• Metrics capture high-cardinality dimensions (user IDs, timestamps)

Testing

Required Coverage

New code should maintain 80%+ coverage (enforced by SonarQube). Flag when:

• New services lack corresponding unit tests
• New command/query handlers lack tests
• Complex components lack bUnit tests
• Critical user flows lack E2E test consideration

Test Patterns

Unit tests must use factory methods for entity creation:

// Correct - uses factory, only specifies what matters for test
var tournament = TournamentFactory.Create(type: TournamentType.Senior);

// Incorrect - manual instantiation
var tournament = new Tournament("Test", DateTime.Now, TournamentType.Senior, ...);

Tests must have trait attributes and display names:

// Correct - has traits and display name
[UnitTest]
[Component("Tournaments.Registration")]
public class RegisterBowlerTests
{
    [Fact(DisplayName = "Should fail when squad is at capacity")]
    public void Should_Fail_When_Squad_At_Capacity() { }

    [Theory(DisplayName = "Should validate age eligibility")]
    [InlineData(49, false, DisplayName = "Under 50 rejected for senior")]
    [InlineData(50, true, DisplayName = "At 50 accepted for senior")]
    public void Should_Validate_Age(int age, bool expected) { }
}

// Incorrect - missing traits and display names
public class RegisterBowlerTests
{
    [Fact]
    public void Should_Fail_When_Squad_At_Capacity() { }
}

MockBehavior.Strict eliminates the need for .Verify() calls:

With MockBehavior.Strict, any call without a matching Setup throws immediately. This means:

• "Was called with expected args" — redundant; the Setup with specific args already enforces this. If the code calls the method with wrong args (or doesn't call it), the test fails.
• "Was not called" — redundant; if no Setup exists for a method, Strict throws on any invocation.

// Correct - Strict setup IS the verification; no .Verify() needed
_storageServiceMock
    .Setup(s => s.UploadFileAsync(
        "documents",
        query.DocumentName,
        expectedDocument.Content,
        expectedDocument.ContentType,
        It.Is<IDictionary<string, string>>(m =>
            m["source-document-id"] == expectedDocument.Id),
        TestContext.Current.CancellationToken))
    .Returns(Task.CompletedTask);

// ... execute code ...

// Assert on the result — no .Verify() calls needed
result.IsError.ShouldBeFalse();

// Incorrect - redundant .Verify() when using MockBehavior.Strict
_storageServiceMock.Verify(
    s => s.UploadFileAsync(It.IsAny<string>(), ...), Times.Once);

Key principle: With MockBehavior.Strict, Setup declarations define the expected interaction contract. The test fails immediately if the code deviates from that contract — no explicit .Verify() needed.

Flag when:

• Tests manually instantiate domain entities instead of using factories
• New entity, value object, DTO, or response type is added without a corresponding factory class in Neba.TestFactory (excludes SmartEnums, strongly-typed IDs, and command/query/job input objects — those don't need factories)
• A new [StronglyTypedId("ulid-full")] type is added without an explicit New() factory method in its partial struct body (source generators don't run in Stryker's Roslyn compilation; New() must be in real source — see ADR-0006)
• Tests don't follow the Arrange-Act-Assert pattern
• Integration tests don't use Bogus factories with seeds for reproducibility
• Missing Verify (snapshot) tests for mapping operations
• Tests missing [UnitTest] or [IntegrationTest] trait attribute
• Tests missing [Component] trait attribute
• Facts or Theories missing DisplayName parameter
• InlineData missing TestDisplayName for theory test cases
• Test method names not following <MethodName>_Should<ExpectedOutcome>_When<Condition> pattern
• Mocking ILogger<T> instead of using NullLogger<T>.Instance (when log content doesn't matter) or FakeLogger<T> from Microsoft.Extensions.Logging.Testing (when asserting on log output)
• Using new Mock<T>() without MockBehavior.Strict parameter
• Using null! instead of #nullable disable/#nullable enable for null testing
• Using .Verify() calls when MockBehavior.Strict already enforces the interaction contract via Setup
• Any usage of FluentAssertions (using FluentAssertions;, .Should().Be*(), etc.) instead of Shouldly

Null testing pattern: When testing methods that don't accept nullable references but need null passed:

[Fact]
public void Method_ShouldThrow_WhenNull()
{
#nullable disable
    string value = null;

    Should.Throw<ArgumentNullException>(() => SomeMethod(value));
#nullable enable
}

What to Test

Logging in tests: Never mock ILogger<T>. Use NullLogger<T>.Instance when you don't need to assert on log output. Use FakeLogger<T> from Microsoft.Extensions.Logging.Testing (namespace inside the Microsoft.Extensions.Diagnostics.Testing NuGet package) when you need to assert on log level, message content, or structured attributes — it's a real ILogger<T> implementation, not a mock. Assert via logger.Collector.GetSnapshot(), which returns IReadOnlyList<FakeLogRecord> with .Level and .Message on each entry.

E2E Coverage for New UI Features

Any new routable Blazor page (a new @page route, or a new mode of an existing page such as create/edit/delete) must ship with a Playwright E2E spec in tests/e2e/ exercising it. This is required, not merely suggested — flag its absence the same way a missing unit test on a handler would be flagged. A docs-screenshot script under tests/e2e/docs-screenshots/ (used only to generate docs/help/ images per ADR-0007) does not satisfy this — it is excluded from the normal npm run test:e2e run and typically stops short of actually submitting/mutating data.

At minimum, the spec should cover, for the new page/flow:

• The happy path (successful submit/action navigates or updates the UI as expected, e.g. a success toast)
• Validation failure on required fields (client-side)
• A server-side failure surfaces the page's error alert/toast and leaves the user on the page (use the mock API server's /__mock/fail?path=...&status=... + /__mock/reset?path=... pattern — see News.spec.ts's "edit article" describe block for the reference shape)
• Authorization boundary: unauthenticated/unpermissioned access is blocked (button hidden and/or direct navigation shows the permission message), and authenticated-with-permission access works (page.request.post('/__test/login?permissions=...'))

When a new page reuses an existing pattern (e.g. another "edit" form styled like EditSponsor.razor/EditArticle.razor), the corresponding mock server route (tests/e2e/mock-api/mock-api-server.ts) usually needs a matching handler (e.g. a new PUT/POST/DELETE branch) — check it was added alongside the spec, not just the spec in isolation.

Also suggest additional E2E tests (beyond the required minimum above) for:

• Multi-step user flows
• Complex form validation with error recovery
• Dirty-form-guard discard/keep-editing behavior on pages with DirtyFormGuard

User Help Documentation

Every user-facing command or feature must ship with a help doc at docs/help/<feature-or-command-name>.md. See ADR-0007 for the rationale and required structure.

Flag when:

• A new user-facing command (endpoint + UI that an admin or end user directly triggers, e.g. create/update/delete actions) is added without a corresponding new file under docs/help/
• An existing documented command's UI changes (new steps, new fields, different flow) but its docs/help/*.md file and screenshots aren't updated to match
• A help doc is missing prerequisites (required role/permission), numbered steps, or at least one screenshot per distinct UI state
• Screenshots are added ad hoc instead of via the project's Playwright screenshot-generation flow (see ADR-0007) — inconsistent screenshot sourcing makes them hard to regenerate later

Do NOT flag purely internal/background changes (e.g. a background job, an internal refactor, an API-only change with no direct UI trigger) for missing help docs — this requirement applies to user-triggered commands only.

Policy Documentation

Every authorization policy must have an entry in docs/policies/README.md. See ADR-0008 for the rationale and required structure.

Flag when:

• A new policy (a new AddPolicy(...) call in SecurityConfiguration.cs / AccountConfiguration.cs) is added without a corresponding row in docs/policies/README.md
• A policy with real behavioral nuance (OR/AND-of-many permission semantics, exceptions) doesn't have a dedicated docs/policies/<policy-name>.md file linked from its README row
• An existing policy's semantics change (e.g. a permission added to a policy's OR-set) but its docs/policies/ entry isn't updated to match

Do NOT flag the dynamic per-permission Permission:{value} policies individually — they're covered collectively by the single README row describing the mechanism, not one row per permission value.

Code Style & Conventions

C# Language Features

Use extension members (C# 14) instead of extension methods:

// Correct - extension member syntax
public static class ServiceExtensions
{
    extension(WebApplication app)
    {
        public WebApplication MapDefaultEndpoints()
        {
            // implementation
            return app;
        }
    }
}

// Incorrect - legacy extension method syntax
public static class ServiceExtensions
{
    public static WebApplication MapDefaultEndpoints(this WebApplication app)
    {
        // implementation
        return app;
    }
}

Flag when:

• Extension methods use the legacy this parameter syntax instead of extension() blocks
• Multiple extension methods for the same type aren't grouped in a single extension() block

Exception: [LoggerMessage] partial methods must use the legacy this parameter syntax as required by the source generator.

Naming

• Files: Match type name (CreateTournamentEndpoint.cs contains CreateTournamentEndpoint)
• Feature folders: Plural (Tournaments/, Bowlers/)
• Routes: Lowercase, plural (/tournaments/{id})

Project Structure

Flag when:

• Feature-specific components not alongside their pages
• Generic components contain domain knowledge
• Files in wrong project (Client vs Server without justification)

Contracts (Neba.Api.Contracts)

See detailed criteria in API Layer section above. Additionally flag when:

• Contract types contain logic beyond simple computed properties
• A required property is added, renamed, or removed on a type deserialized by the Blazor client (any Refit response used by src/Neba.Website.Server) without a matching update to tests/e2e/mock-api/mock-api-server.ts. A missing required field fails System.Text.Json deserialization on the Blazor server at runtime — the Playwright failure that results looks like ordinary flakiness (a timeout waiting for an element that never renders), not an obvious schema mismatch, so it's easy to miss in review. A Husky pre-push task (contract-mock-check) warns when src/Neba.Api.Contracts/** changes without a corresponding mock-api-server.ts change, but it only checks that the file changed, not that the right fields were added — still confirm the mock's fields actually match.

Common Anti-Patterns to Flag

Banned Libraries

The following libraries are explicitly prohibited from direct use in application code:

Note on transitive dependencies: Some packages (e.g., Hangfire) have transitive dependencies on Newtonsoft.Json. The package may exist in the dependency graph, but direct usage in our code is prohibited. Flag any using Newtonsoft.Json statements or direct calls to JsonConvert.

Review Checklist

When reviewing, verify:

Architecture & Code Quality

• Feature boundaries respected (no cross-feature domain references)
• Commands return ErrorOr<T>
• Queries return DTOs, not entities
• Extension methods use extension() block syntax, not legacy this parameter
• DateTimeOffset used instead of DateTime for points in time

Ubiquitous Language

• Every new aggregate, entity, and value object has an entry in docs/ubiquitous-language.md
• XML <summary> comments on new domain types convey the same concept as their UL entry (not word-for-word, but purpose-aligned)
• A quick scan of existing UL entries and XML comments for domain types touched in this PR — flag any pre-existing gaps or contradictions found in passing

API Endpoints

• Use case folder structure followed (Endpoint, Summary, Validator)
• Authorization explicitly configured (AllowAnonymous(), Roles(), or Policies())
• WithName() present in Description
• Tags match authorization (Public/Authenticated/Admin)
• All status codes documented with Produces()/ProducesProblemDetails()
• Validator present (if request has input to validate)
• Validator contains only structural validation (no DB lookups, no business rules)
• All errors return ProblemDetails
• Summary class with realistic examples
• Inline mapping (no separate mapper classes)

Contracts

• Request wraps Input for commands
• XML documentation on all public types and properties
• Using { get; init; } not { get; set; }
• Refit interface updated

REST Conventions

• REST conventions followed (plural nouns, no verbs in URLs)
• Response envelopes consistent
• Query/GET responses project SmartEnum properties via .Name, not .Value (except UsState/CanadianProvince/Country/Permissions — see SmartEnum Serialization)
• Command (POST/PUT/PATCH) requests parse SmartEnum properties via .FromValue, not .FromName

Testing

• Tests use factories, not manual instantiation
• New entity/value object/DTO/response has a corresponding factory in Neba.TestFactory (SmartEnums, strongly-typed IDs, and input objects are exempt)
• New [StronglyTypedId("ulid-full")] type has an explicit New() factory method in its partial struct body (not relying solely on source generation — see ADR-0006)
• Tests have [UnitTest] or [IntegrationTest] trait
• Tests have [Component] trait
• Tests have DisplayName on Facts and Theories
• New code has corresponding tests
• API endpoint integration tests cover success, validation failure, and auth failure
• New routable Blazor page/flow has a Playwright E2E spec in tests/e2e/ (not just a docs-screenshots/ script) covering happy path, validation failure, server-error handling, and the auth boundary
• Mock API server (tests/e2e/mock-api/mock-api-server.ts) has a matching route handler for any new endpoint the E2E spec exercises
• Any required property added/renamed/removed on a Neba.Api.Contracts type consumed by the Blazor client has its mock JSON in tests/e2e/mock-api/mock-api-server.ts updated to match

Observability

• Logging present with appropriate levels
• Spans added for business operations
• No sensitive data logged
• New auditable aggregate roots added to .Include<T>() in AuditingConfiguration.cs

Blazor

• Blazor components don't fetch data directly
• Data-entry pages/forms use DirtyFormGuard to warn before losing unsaved changes
• Fields bound to [Required] model properties use FormLabel (not a bare <label>) so required fields show a "(required)" tag

User Help Documentation

• New user-facing commands have a corresponding docs/help/*.md file (ADR-0007)
• Changes to a documented command's UI are reflected in its help doc and screenshots

Policy Documentation

• New authorization policies have a row in docs/policies/README.md (ADR-0008)
• Policies with OR/AND-of-many or otherwise non-trivial semantics have a dedicated docs/policies/<policy-name>.md file