From 64e0b6d119055b3805fe7699df7e6e892a3fa2f8 Mon Sep 17 00:00:00 2001 From: emeraldleaf Date: Thu, 28 May 2026 13:10:13 -0600 Subject: [PATCH] chore(claude): encode model-naming convention + AsSpan-on-hot-paths rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two rules requested for encoding. Both framed to fit NextAurora's actual context rather than as blanket assertions that would contradict existing rules. 1. MODEL NAMING (intent-based, by role) — CLAUDE.md "Coding Standards" + .coderabbit.yaml **/*.cs naming guidance. The source tip was "prefer Request/Response over Dto." Encoded as the convention NextAurora already follows, which is *more* intent-based: - CQRS messages: *Command (writes) / *Query (reads) — more specific than *Request (says write-vs-read + flows the Wolverine pipeline). - gRPC contracts: *Request / *Response pairs (gRPC idiom). - Read projections: *Dto — acceptable ONLY here (query-result shape; not HTTP-coupled, so don't rename to *Response). Rule in one line: generic *Dto on a REQUEST model hides intent — name it by what it does; reserve *Dto for read projections. Deliberately does NOT mandate "Request/Response everywhere" (that would downgrade the write side from the more-specific Command/Query). 2. AsSpan over Substring — CLAUDE.md "Performance Rules" (right after "Measure before optimizing", which governs it). Zero-allocation slicing, real win in synchronous hot string loops. Encoded with two guardrails so it's a narrow tool, not a default: (a) governed by measure-before-optimizing — apply on profiled hot paths, not reflexively; (b) Span is a ref struct — can't cross an await boundary, so rarely usable in NextAurora's async-everywhere handlers. No such hot path exists today (string work is incidental; bottleneck is always I/O), so the rule is explicitly preventative. CLAUDE.md-only — no .coderabbit.yaml/reviewer surface (it's a judgment-on-profiling call, not a code-shape check), same as the streams rule. Paraphrase-drift audit: run /check-rules locally to audit paraphrases against this diff. Done — both rules are net-new; no existing See-CLAUDE.md marker covers model-naming or AsSpan, so nothing to realign. (The naming rule codifies the de-facto convention already visible in the type names; AsSpan has no existing usage to conflict.) Co-Authored-By: Claude Opus 4.7 --- .coderabbit.yaml | 11 +++++++++++ CLAUDE.md | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index fdbe946c..d4aba0c0 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -41,6 +41,17 @@ reviews: HandleAsync (NOT Handle). Private instance fields prefixed with _; constants and static readonly use PascalCase. Sync-over-async patterns (.Result, .Wait(), .GetAwaiter().GetResult()) are banned at build time — flag any sneak-through. + + MODEL NAMING (intent-based, by role). Flag a generic `*Dto` suffix on a + REQUEST model — name inbound CQRS messages `*Command` (writes) / `*Query` + (reads), gRPC contracts `*Request`/`*Response`. `*Dto` is reserved for + read-side projections (the shape a query handler projects to, e.g. + OrderSummaryDto) and is acceptable ONLY there. So: a new `CreateXDto` or + `XRequestDto` on a command/query is a Should-consider (rename to + `*Command`/`*Query`); a `*Dto` on a query-result projection is fine. Don't + recommend renaming existing `*Dto` read projections to `*Response` — + they're query contracts, not HTTP-response models. See CLAUDE.md + "Coding Standards → Model names are intent-based, by role". Interfaces earn their keep through consumer substitution (test mock or 2+ impls today, OR concrete near-term roadmap) — flag speculative interfaces. Repository wrappers around EF Core (IFooRepository + FooRepository) are explicitly NOT diff --git a/CLAUDE.md b/CLAUDE.md index 44a578cc..741c9a10 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,6 +112,11 @@ project's lesson, not an inconsistency to clean up. - Private *instance* fields prefixed with `_` (camelCase). Constants and `static readonly` fields use PascalCase per .NET convention — do NOT prefix with `_` (e.g. `OrdersPlaced`, `Carriers`, `TraceIdKey`). The `.editorconfig` enforces this split via separate naming rules - Async methods suffixed with `Async` - Interfaces prefixed with `I` +- **Model names are intent-based, by role — prefer a specific suffix over a generic `Dto` on a request model.** A generic suffix like `CreateOrderDto` only says "data transfer object"; it doesn't say what the type *does* or where it belongs. Name by role instead: + - **CQRS messages:** `*Command` for writes (`PlaceOrderCommand`, `ProcessPaymentCommand`), `*Query` for reads (`GetOrderByIdQuery`, `SearchProductsQuery`). These are *more* intent-revealing than a generic `*Request` — they say write-vs-read *and* that the type flows the Wolverine message pipeline. This is the canonical inbound shape; do NOT introduce a `*Dto`/`*Request` request model where a Command/Query fits. + - **gRPC contracts:** `*Request` / `*Response` pairs (`GetProductRequest` / `ProductResponse`) — the gRPC idiom, already used in CatalogService. + - **Read projections:** `*Dto` (`OrderSummaryDto`, `ProductDto`) — the query-result shape a query handler projects to. `Dto` is acceptable *here only*, where it correctly signals "read-side projection / transfer shape" in a CQRS context; it is NOT coupled to HTTP, so don't rename it to `*Response`. + - The rule in one line: **generic `*Dto` on a request model hides intent — name it by what it does; reserve `*Dto` for read projections.** - Use `var` when type is apparent - TreatWarningsAsErrors is enabled - zero warnings allowed - Static analyzers: Meziantou, SonarAnalyzer, Roslynator @@ -208,6 +213,7 @@ These are always-on. Deeper guidance (modern EF features, transactions, caching - **Cache invalidation in the write path**: if a handler mutates a cached entity, it must invalidate or update the cache in the same handler — not "later" or "via TTL". - **Migrations are immutable once applied**: never edit a migration that has run anywhere (dev included). Destructive changes (drop column/table, rename, NOT NULL on existing column) need a multi-step plan, not a single migration. - **Measure before optimizing**: don't add caching, compiled queries, `ValueTask`, or `AsSplitQuery()` on intuition. Use BenchmarkDotNet for code paths, `dotnet-counters`/k6 for system behavior, `ToQueryString()` for EF. +- **`AsSpan` over `Substring` for zero-allocation slicing — but only on profiled hot paths, and mind the async constraint.** `Substring(...)` allocates a new `string` per call; `AsSpan(...)` returns a `ReadOnlySpan` view over the original with no allocation, and `string.Concat`, `int.Parse`, etc. have span overloads. This is a real win in *synchronous, hot, string-heavy loops* — parsers, formatters, tokenizers, bulk ID/field manipulation. **Two guardrails make it a narrow tool, not a default:** (1) it's a micro-optimization governed by "Measure before optimizing" above — apply it where profiling shows string-allocation pressure, not reflexively; (2) **`Span`/`ReadOnlySpan` is a `ref struct` — stack-only, can't cross an `await` boundary or be captured in a lambda/field**, so it's rarely usable inside NextAurora's async-everywhere request handlers (the compiler will stop you). NextAurora has no such hot path today — its string work is incidental (log templates, IDs), and the bottleneck is always I/O (EF, gRPC, HTTP), never `Substring`. The rule is preventative: *if* a synchronous string-crunching hot path appears and profiling justifies it, reach for `AsSpan`; until then, `Substring` is fine and clearer. - **Dapper is the sanctioned escape hatch from EF**, not a peer abstraction. Reach for it only when (a) the SQL is provider-specific and doesn't translate cleanly, (b) profiling proves EF is the bottleneck on a hot path, or (c) the query is a SQL aggregation where LINQ obscures intent. Always use `ctx.Database.GetDbConnection()` so Dapper shares the EF connection and any ambient transaction — never open a separate `NpgsqlConnection`/`SqlConnection`. Writes always go through aggregates + EF (Dapper bypasses concurrency tokens, domain validation, and the outbox). Full rationale: [docs/performance-and-data-correctness.md "Decision: when to reach past EF Core (Dapper escape hatch)"](docs/performance-and-data-correctness.md#decision-when-to-reach-past-ef-core-dapper-escape-hatch). ## Testing