From 9abea6a04ac9cf2be8894849b969089a9f50c242 Mon Sep 17 00:00:00 2001 From: emeraldleaf Date: Sat, 23 May 2026 22:40:04 -0600 Subject: [PATCH 1/3] chore(claude): encode lessons learned from architecture-review session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 8-file architecture-reviewer agent pass on 2026-05-24 surfaced 12 must-fix items + 17 should-considers across security, EF Core, async, distributed systems, and observability. The fixes shipped as PRs; this commit captures the patterns so future code generation prevents the same bugs rather than re-discovering them in review. CLAUDE.md — 4 sections expanded: 1. "Security Requirements" — concrete IDOR-prevention pattern (was just "validate buyerId matches authenticated user"; now spells out the endpoint→query→handler→null→404 shape with reference templates + the 403-vs-404 reasoning). Adds explicit JWT defaults (ValidateIssuerSigningKey=true, ClockSkew=30s — not the dangerous 5-minute default). Adds the trace-ID-exposure rule (use Activity.TraceId.ToString, not Activity.Id which leaks the span). Also explicitly cross-references the integration-test requirement. 2. "Transactional Outbox (Wolverine)" — adds the outbox-outside-handler atomicity trap that caused the PaymentRecoveryJob silent event drop. Wolverine's AutoApplyTransactions only wraps handler chains; code running outside (BackgroundService, recovery sweepers) needs an explicit SaveChangesAsync between PublishAsync and Commit, or Wolverine's staged envelope never reaches the outbox table. Includes the canonical safe wrapper as runnable code. 3. "Observability — HTTP middleware order" — strict ordering rule (UseExceptionHandler → UseAuthentication → CorrelationIdMiddleware → UseAuthorization). The first ServiceDefaults bug was middleware reading context.User before UseAuthentication populated it (UserId silently always null); the over-correction placed it after UseAuthorization which dropped UserId from 401/403 audit logs. Documents both failure modes + the correct middle ground. 4. "Testing" — IDOR test is now REQUIRED for every scoped-entity endpoint. Authorization behavior is only proven by an authorization-failure test; dotnet build + unit tests passing isn't sufficient. Also adds outbox-in-non-handler test requirement for sweepers/jobs. .coderabbit.yaml — 3 new path_instructions blocks: - **/Endpoints/**/*.cs gets explicit IDOR-prevention guidance with the canonical 4-step pattern + mass-assignment check + endpoint hygiene reminders. - **/*RecoveryJob*.cs gets outbox-outside-handler trap guidance + per-iteration scope rule + distributed-lock rule. - NextAurora.ServiceDefaults/**/*.cs gets the middleware-order rule, JWT explicit-defaults rule, trace-ID-exposure rule, and a hint that changes here should bump unit-test coverage (because the project has historically been undertested and codecov/patch will complain). architecture-reviewer agent — new "Pattern checklist" section with specific per-file-category scan rules. Six categories covered: Endpoints (IDOR + mass-assignment), RecoveryJob (outbox + scope + lock), ServiceDefaults (middleware + JWT + trace-ID), query handlers (AsNoTracking + pagination + N+1), aggregates (rich domain entity + collection encapsulation + concurrency token), workflows (bash hygiene + persist-credentials + permissions). The agent now scans for these patterns explicitly instead of re-deriving from first principles on every review. Issues doc (/tmp/nextaurora-issues-to-create.md) — 8 new backlog entries from this session's findings, sized + scoped with acceptance criteria. Includes the IDOR-test backfill, ServiceDefaults integration tests, PaymentRecoveryJob outbox integration test, per-iteration DI scope refactor, CORS helper, security headers, PlaceOrder defense-in- depth, and HTTPS-redirect consolidation. Paraphrase sweep: zero stale paraphrases. The new rules are NEW additions, not modifications of existing wording, so none of the existing `See CLAUDE.md` markers in code or docs need updating. The in-flight fix PRs (IDOR, outbox, middleware) will add new markers in their respective inline comments when they merge — that's PR-scope, not lessons-learned-PR-scope. Co-Authored-By: Claude Opus 4.7 --- .claude/agents/architecture-reviewer.md | 57 ++++++++++++++++++++++++- .coderabbit.yaml | 54 +++++++++++++++++++++++ CLAUDE.md | 39 ++++++++++++++++- 3 files changed, 148 insertions(+), 2 deletions(-) diff --git a/.claude/agents/architecture-reviewer.md b/.claude/agents/architecture-reviewer.md index dccda06e..76e38cfb 100644 --- a/.claude/agents/architecture-reviewer.md +++ b/.claude/agents/architecture-reviewer.md @@ -25,7 +25,9 @@ report. You do **not** write code or edit files — you read, analyze, and repor - "Coding Standards" - "Performance Rules" - "Key Conventions" - - "Observability & Context Propagation" (if the target touches handlers or middleware) + - "Security Requirements" → IDOR pattern, JWT defaults, trace-ID exposure + - "Observability & Context Propagation" → HTTP middleware order, Wolverine middleware + - "Testing" → IDOR test required, outbox-in-non-handler test required 2. **Read the architecture map** at `.claude/architecture-map.md` for service/file orientation if present — it'll tell you which service the target lives in and what @@ -47,6 +49,59 @@ report. You do **not** write code or edit files — you read, analyze, and repor 6. **No-find reviews are valid.** If the change is small and clean, say so plainly. Don't pad. +## Pattern checklist — scan for these on every relevant review + +Specific bug-classes that have bitten this repo before. When the target file matches a category, check for the pattern explicitly. Cite a finding when you see the bug; cite as "Aligned" when you see the correct pattern in place. + +### When reviewing `**/Endpoints/**/*.cs` (or anything registering HTTP routes) + +- **IDOR check (CRITICAL).** Every GET-by-id, GET-by-scope, PATCH, PUT, DELETE on a buyer/seller-scoped entity must: + - Read `ClaimTypes.NameIdentifier` from JWT at the endpoint + - Pass `RequestingBuyerId` (or `RequestingSellerId`) into the query/command + - Handler returns `null` on entity-owner mismatch + - Endpoint translates `null` → 404 (NOT 403) + - Reference: `OrderEndpoints.cs:GET /orders/{id}`, `ShippingEndpoints.cs:GET /shipments/order/{orderId}`. Any deviation is a Must-fix IDOR. +- **Mass assignment.** Any `[FromBody]` or minimal-API body parameter binding a record/class that contains a server-controlled field (`BuyerId`, `SellerId`, `Status`, `Price`, `IsDeleted`). The endpoint must verify the field matches the JWT claim or strip it from the bound type. +- **`MapV1ApiGroup` used** (not hand-rolled `NewVersionedApi().MapGroup().HasApiVersion()` chains). +- **`.RequireAuthorization()` at group level** unless explicitly public. +- **List endpoints clamp pagination** server-side (`ClampPaging` or equivalent, cap ≤ 100). + +### When reviewing `**/*RecoveryJob*.cs` or any `BackgroundService` / cron-style sweeper + +- **Outbox-outside-handler atomicity (CRITICAL).** If the sweeper calls `eventPublisher.PublishAsync(...)` then commits an EF transaction, the wrapper MUST call `await context.SaveChangesAsync(ct)` AFTER the publish and BEFORE `tx.CommitAsync(ct)`. Without it, Wolverine's staged envelope never reaches `wolverine.outgoing_envelopes` and the event is silently dropped. Reference: `PaymentRepository.ExecuteInTransactionAsync`. +- **DI scope per iteration.** The sweep loop should create a fresh `IServiceScope` per iteration (per row, per stale entity), NOT reuse one scope across the whole sweep. Reusing the scope means the EF change tracker accumulates every row's entity for the duration of the sweep + creates a future-parallel-refactor footgun. +- **Distributed lock for cross-replica work.** Sweepers running on N replicas need `DistributedLock.SqlServer` (`sp_getapplock`) or equivalent. Acquired with `TimeSpan.Zero` (no-wait), released in `await using` for exception safety. +- **`TimeProvider` injected**, not `DateTime.UtcNow` direct (test determinism). +- **Per-iteration try/catch** so one bad row doesn't crash the whole sweep. + +### When reviewing `NextAurora.ServiceDefaults/**/*.cs` + +- **HTTP middleware order** in `MapDefaultEndpoints` must be: `UseExceptionHandler` → `UseAuthentication` → `CorrelationIdMiddleware` → `UseAuthorization`. Any other order is a regression — see CLAUDE.md "Observability". +- **JWT `TokenValidationParameters`** explicit `ValidateIssuerSigningKey = true` AND `ClockSkew = TimeSpan.FromSeconds(30)` (NOT the 5-minute default). Default ClockSkew is a security regression on short-lived tokens. +- **`GlobalExceptionHandler` traceId** uses `Activity.Current?.TraceId.ToString()`, NOT `Activity.Current?.Id` (which leaks the span ID in the W3C traceparent). +- **No exception message leak.** Response body never contains `ex.Message`, `ex.StackTrace`, `ex.ToString()`. + +### When reviewing query handlers (`**/Features/Get*.cs`, `**/Application/Handlers/Get*.cs`) + +- **AsNoTracking + projection** for read paths. Either `.AsNoTracking() + .Select(...)` to a DTO, OR `AsNoTrackingWithIdentityResolution()` when `Include` is needed without tracking. Plain `AsNoTracking() + Include` duplicates the included entity per row. +- **Pagination cap.** List queries must accept `(page, pageSize)` with server-side enforcement. +- **N+1 detection.** Any `foreach` over query results that queries inside. + +### When reviewing aggregates (`**/Domain/*.cs`) + +- **Rich Domain Entity shape.** Factory method (`static Create(...)`) with validation; private setters; named state-transition methods (`MarkAsPaid`, not `Status = Paid`); status-guard inside the transition method for idempotency under at-least-once delivery. +- **No mutable collection exposure.** `public IReadOnlyList` over `private readonly List _items`; add via named methods (`AddLine`), not direct mutation. +- **Layer dependencies.** Domain depends on nothing — no EF, no logging, no Wolverine. +- **Concurrency token present** (Postgres `xmin` shadow or SQL Server `RowVersion` shadow byte[] property in DbContext config — entity itself stays clean). + +### When reviewing `.github/workflows/*.yml` + +- **`set -euo pipefail`** at top of every bash `run:` block. +- **`persist-credentials: false`** on `actions/checkout` when the job doesn't push back. +- **Explicit `permissions:` block** with least-privilege. +- **`concurrency:` group** to avoid wasted runs on rapid pushes. +- **NOT a finding**: individual unpinned `@vN` action references (Gap 4 — batch pinning is deferred). NOT a finding: bracket spacing `[ main ]` vs `[main]` (matches repo convention). + ## Output format ``` diff --git a/.coderabbit.yaml b/.coderabbit.yaml index d479120a..d7f04634 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -68,6 +68,60 @@ reviews: error responses go through GlobalExceptionHandler — never expose internal state, entity IDs, or stack traces to clients. + IDOR PREVENTION (highest-stakes finding category for this repo): every endpoint that + returns OR mutates a scoped entity (Order, Payment, Shipment, Product update, etc.) + must enforce scope. The canonical shape: + 1. Endpoint reads ClaimTypes.NameIdentifier from JWT → requestingBuyerId/sellerId + 2. Query/command carries the requesting principal as a field (NOT trusted from body) + 3. Handler compares entity.BuyerId/SellerId to request.RequestingBuyerId/SellerId + 4. Handler returns null on mismatch; endpoint translates null → 404 (NOT 403 — + 403 leaks existence, 404 is indistinguishable from "not found") + Flag any GET-by-id, GET-by-scope, PATCH, PUT, or DELETE endpoint missing this pattern. + Reference templates: OrderEndpoints.cs:GET /orders/{id}, ShippingEndpoints.cs:GET + /shipments/order/{orderId}, CatalogEndpoints.cs:PUT /products/{id} (seller variant). + + - path: "**/*RecoveryJob*.cs" + instructions: | + BackgroundService sweepers + recovery jobs run OUTSIDE Wolverine's handler pipeline, + so AutoApplyTransactions does NOT wrap them. The outbox-atomicity trap: PublishAsync + stages an envelope to the tracker, but it's only persisted when SaveChangesAsync runs. + If a wrapper does BeginTransactionAsync → entity write + publish → Commit *without* + an explicit SaveChangesAsync between publish and commit, the staged envelope never + reaches wolverine.outgoing_envelopes — event is silently dropped, saga stalls. + + Flag any non-handler code path that publishes events without an explicit + SaveChangesAsync after the publish and before the transaction commit. Reference: + PaymentRepository.ExecuteInTransactionAsync (canonical safe wrapper). + + Also: BackgroundService sweepers should create a fresh DI scope per iteration of + their sweep loop, NOT reuse one scope across the whole sweep — change tracker + bloat for the duration of the sweep + future parallel-refactor footgun. + + - path: "NextAurora.ServiceDefaults/**/*.cs" + instructions: | + Security-critical infrastructure. Specific things to flag: + + - HTTP middleware order in MapDefaultEndpoints must be: + UseExceptionHandler → UseAuthentication → CorrelationIdMiddleware → UseAuthorization + CorrelationIdMiddleware MUST run after UseAuthentication (else context.User is empty + and UserId scope is always null) AND before UseAuthorization (so 401/403 denials + get UserId in their log scope for audit). Any other ordering is a regression. + + - JWT TokenValidationParameters must explicitly set: + ValidateIssuerSigningKey = true + ClockSkew = TimeSpan.FromSeconds(30) // NOT the 5-minute default + ValidateAudience/Issuer/Lifetime = true + Default ClockSkew (5 min) makes revoked tokens valid for 5 extra minutes — + material on short-lived access tokens. + + - GlobalExceptionHandler must use Activity.Current?.TraceId.ToString() (32 hex chars), + NOT Activity.Current?.Id (the full W3C traceparent with span — leaks server-side + call structure to clients). + + - Tests: changes to this project warrant unit-test coverage of the changed lines. + ServiceDefaults has historically been undertested; bumping coverage of new lines is + worth the small effort to keep codecov/patch from regressing repo-wide. + - path: "**/*Migration*.cs" instructions: | EF Core migrations are IMMUTABLE once applied anywhere (dev included). Never edit diff --git a/CLAUDE.md b/CLAUDE.md index 150c7c03..46f007ca 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,9 +28,19 @@ NextAurora is a .NET 10 microservices e-commerce platform using Aspire, Azure Se ### Security Requirements - **Authentication**: All non-public endpoints must use `.RequireAuthorization()`. JWT Bearer authentication. -- **Authorization**: Users can only access their own resources. Validate `buyerId` matches authenticated user. +- **Authorization (the concrete pattern, not just the principle)**: Users can only access their own resources. The canonical shape for buyer-scoped reads — applied because a missing scope check is an IDOR (CWE-639), and IDORs slip through tests-by-omission: + - Endpoint reads `ClaimTypes.NameIdentifier` from the JWT → passes as `RequestingBuyerId` into the query/command. + - Handler loads the entity, then **returns null** on owner mismatch (NOT throws, NOT returns 403). + - Endpoint translates `null` to **404**. Returning **403 is wrong** here — it leaks existence ("this resource exists, just not yours"). 404 is indistinguishable from "not found." + - Reference templates: `OrderEndpoints.cs:GET /orders/{id}` (commit-on-record after fix), `ShippingEndpoints.cs:GET /shipments/order/{orderId}`, `CatalogEndpoints.cs:PUT /products/{id}` (seller-scope variant — defense in depth at endpoint AND handler). + - **An integration test asserting buyer X cannot read buyer Y's entity is required** when adding any scoped-entity endpoint. The absence of such a test is how IDORs survive — see CLAUDE.md "Testing" rule. +- **JWT validation (explicit, not implicit)**: `TokenValidationParameters` must explicitly set: + - `ValidateIssuerSigningKey = true` (default validates via JWKS, but explicit is auditable). + - `ClockSkew = TimeSpan.FromSeconds(30)` — default is **5 minutes**, which means revoked/expired tokens stay accepted for 5 extra minutes. Material on typical 15-minute access-token lifetimes. + - `ValidateAudience`, `ValidateIssuer`, `ValidateLifetime` all `true`. See [Extensions.cs `AddDefaultAuthentication`](NextAurora.ServiceDefaults/Extensions.cs). - **Input Validation**: All commands must have FluentValidation validators. Validate at the API boundary before reaching handlers. - **Error Handling**: Never expose internal state, stack traces, or entity IDs in API responses. Log details server-side, return generic errors with correlation IDs to clients. + - **Response `traceId` field uses `Activity.TraceId.ToString()` only** (32 hex chars), NOT `Activity.Id` (the full W3C traceparent `00---` — span ID leaks server-side handler call structure to clients). See [GlobalExceptionHandler.cs](NextAurora.ServiceDefaults/GlobalExceptionHandler.cs). - **HTTPS**: Enforce HTTPS redirection in production. - **CORS**: Explicit CORS policy allowing only known frontend origins. - **Rate Limiting**: Applied to search and payment endpoints at minimum. @@ -178,6 +188,8 @@ These are always-on. Deeper guidance (modern EF features, transactions, caching - Unit tests for domain logic and handlers - Integration tests with Testcontainers for infrastructure — `tests/{Service}.Tests.Integration`, booting the real API via `WebApplicationFactory`. **CatalogService** slice (Postgres + Redis: caching + concurrency token) and **OrderService** slice (SQL Server + Wolverine stubbed transport: outbox, saga handlers, `RowVersion` token) exist; pattern documented in each project's README. - **Integration tests need Docker.** On macOS, Docker Desktop's socket is at `~/.docker/run/docker.sock`, not `/var/run/docker.sock` — Testcontainers fails fast with `DockerUnavailableException` unless `DOCKER_HOST` points there (or Docker Desktop's "default Docker socket" toggle is on). CI runners have it at the standard path. +- **IDOR test is required for every new endpoint that returns or mutates a scoped entity.** Add an integration test that authenticates as buyer X, requests a resource owned by buyer Y, and asserts the response is 404 (NOT 200, NOT 403 — see Security Requirements). The absence of such a test is exactly how the original `GET /api/v1/orders/{id}` IDOR survived undetected for the lifetime of the codebase. A `dotnet build` clean and unit tests passing aren't sufficient — *authorization behavior is only proven by an authorization-failure test*. +- **Outbox-in-non-handler test.** Code paths that publish events from outside a Wolverine handler (BackgroundService sweepers, recovery jobs) need an integration test that asserts a row appears in `wolverine.outgoing_envelopes` in the same transaction as the entity write. See the outbox-outside-handler trap in Observability → Transactional Outbox. The PaymentRecoveryJob outbox bug survived because no test asserted that the staged envelope actually persisted. - Run `dotnet build` to verify - all analyzer warnings are errors ## Build & Run @@ -207,6 +219,17 @@ Every HTTP request and Service Bus message carries three context identifiers: All three are set by `CorrelationIdMiddleware` (HTTP entry point) and by `ContextPropagationMiddleware` (Wolverine incoming-message middleware, async entry point). All three are propagated onto outgoing Wolverine messages by `OutgoingContextMiddleware`. Both middlewares are wired via the `opts.AddNextAuroraContextPropagation()` extension in each service's `Program.cs`. +**HTTP middleware order — strict.** `CorrelationIdMiddleware` reads `ClaimTypes.NameIdentifier` from `context.User` to populate the `UserId` scope. That requires running AFTER `UseAuthentication` (otherwise `context.User` is empty and `UserId` is silently always null — defeats the audit pipeline). It also must run BEFORE `UseAuthorization` so the `UserId` scope is active during the authorization decision — any 401/403 denial gets logged with the authenticated user's ID, preserving the audit trail for "user X tried to access resource they shouldn't." Canonical order in `MapDefaultEndpoints`: + +```csharp +app.UseExceptionHandler(); // wraps every error below +app.UseAuthentication(); // populates context.User +app.UseMiddleware(); // reads User, opens log scope +app.UseAuthorization(); // 401/403 attributed to UserId +``` + +Reference: [Extensions.cs `MapDefaultEndpoints`](NextAurora.ServiceDefaults/Extensions.cs). + ### Wolverine middleware classes must use instance methods `opts.Policies.AddMiddleware()` only discovers `Before`/`After`/`Finally` (and their `Async` variants) as **instance methods** on a public class with a public constructor. Static methods aren't discovered — registration throws `InvalidWolverineMiddlewareException` at host startup. This applies even when the method has no instance state. Suppress S2325 ("should be static") with a `Justification` referencing this rule rather than satisfying the analyzer. @@ -238,6 +261,20 @@ opts.Policies.UseDurableOutboxOnAllSendingEndpoints(); `builder.Services.AddResourceSetupOnStartup()` auto-creates outbox tables on app startup. This means the entity write and the event publish either both commit or neither does — no more lost events on bus failure or process crash. See [docs/performance-and-data-correctness.md](docs/performance-and-data-correctness.md) for the full rationale and failure modes addressed. +**Outbox outside a Wolverine handler — atomicity trap.** `AutoApplyTransactions` only wraps Wolverine handler chains. Code that runs OUTSIDE a handler (`BackgroundService` sweepers, cron-style recovery jobs, admin endpoints, anything publishing events from a non-handler context) does NOT get the outbox-atomic transaction wrap for free. The trap: `IMessageBus.PublishAsync(...)` stages an envelope into the `wolverine.outgoing_envelopes` tracker, but **the envelope is only persisted when `SaveChangesAsync` runs after the publish call**. Wolverine's `UseEntityFrameworkCoreTransactions` intercepts `SaveChanges` to bridge the staged envelope into the DB transaction. If your wrapper does `BeginTransactionAsync` → entity write + publish → `Commit` *without an explicit `SaveChangesAsync` between the publish and the commit*, the envelope stays in the tracker, the transaction commits without it, and the event is silently dropped. + +The canonical safe wrapper: +```csharp +public async Task ExecuteInTransactionAsync(Func work, CancellationToken ct = default) +{ + await using var tx = await context.Database.BeginTransactionAsync(ct); + await work(ct); // entity write + PublishAsync inside here + await context.SaveChangesAsync(ct); // flushes Wolverine's staged envelope + await tx.CommitAsync(ct); +} +``` +Reference: [PaymentRepository.ExecuteInTransactionAsync](PaymentService/Infrastructure/PaymentRepository.cs) (fixed in the commit captured by docs/STATUS.md). When adding a non-handler code path that publishes events, **either** wrap it in this pattern **or** factor the publish back into a Wolverine handler triggered by an internal scheduled message. + ### Event Replay Replay is handled through Wolverine's own message-store and DLQ tooling. The previous hand-rolled `EventLogs` table and `/admin/events` endpoints were deleted as dead code post-Wolverine — they were only ever populated by replay records of replays. If operator-facing event browsing is needed, build it on top of `IMessageStore` (Wolverine's API) or the `wolverine.outgoing_envelopes` table directly. From f25548db3e0be6f4d2231d7cf64edad8ec6b9276 Mon Sep 17 00:00:00 2001 From: emeraldleaf Date: Sat, 23 May 2026 22:45:28 -0600 Subject: [PATCH 2/3] chore(claude): encode "Continuous Rule Encoding" as an explicit meta-rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing "Debugging Discipline" rule mandates that lessons from debugging sessions get encoded in CLAUDE.md + supporting docs before moving on. This commit generalizes that same discipline to ALL review surfaces — architecture-reviewer agent passes, CodeRabbit findings, manual code review, integration-test failures, prod incidents, security audits. CLAUDE.md: new "Continuous Rule Encoding (the compounding loop)" section adjacent to Debugging Discipline. It explicitly mandates that when an anti-pattern / rule / spec is identified worth keeping, it gets written to ALL applicable artifacts in the same session: 1. CLAUDE.md — the canonical rule 2. .coderabbit.yaml path_instructions — PR-time detection 3. architecture-reviewer agent Pattern Checklist — review-time detection 4. .claude/skills/ — if the procedure is non-trivial 5. docs/STATUS.md Open Issues — if deferred 6. Supporting docs (architecture.md, performance-and-data-correctness.md, dev-loop.md) — when the *why* needs more than a one-liner And it makes the threshold explicit: encode if a future author could repeat the mistake OR re-derive the rule from first principles. Don't encode trivial style nits; do encode security/perf/concurrency/ distributed-systems patterns. The closing line is the load-bearing commitment: "A merged fix PR without the corresponding rule is a half-finished job." architecture-reviewer agent: added Step 7 to "How to work" — when the agent identifies a pattern worth encoding, it must propose WHERE (CLAUDE.md section / .coderabbit.yaml path / Pattern Checklist category) and the proposed wording. Added a "Rules to encode" optional section to the output format so the suggestions have a place to land in the report rather than getting buried. The point of these two changes together: the compounding loop becomes mechanical rather than aspirational. Every review-surface finding now has a clear path from "discovered" to "encoded so we don't discover it again" — and the agent itself prompts for the encoding so we don't forget. Paraphrase sweep: clean. New section is purely additive; no existing rule wording changed. Co-Authored-By: Claude Opus 4.7 --- .claude/agents/architecture-reviewer.md | 7 +++++++ CLAUDE.md | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/.claude/agents/architecture-reviewer.md b/.claude/agents/architecture-reviewer.md index 76e38cfb..3930fe2a 100644 --- a/.claude/agents/architecture-reviewer.md +++ b/.claude/agents/architecture-reviewer.md @@ -49,6 +49,8 @@ report. You do **not** write code or edit files — you read, analyze, and repor 6. **No-find reviews are valid.** If the change is small and clean, say so plainly. Don't pad. +7. **Suggest rule encodings for patterns worth keeping.** If a finding (Must-fix OR Aligned-but-non-obvious) represents a pattern future authors could repeat, propose where it should be encoded. Per CLAUDE.md "Continuous Rule Encoding," the fix lives in a PR but the *rule* lives in `.claude/` — both should land together. Suggest concretely: "This belongs as a CLAUDE.md section X bullet" or "This warrants a new `.coderabbit.yaml` path_instruction for `**/Y/*.cs`" or "Add to the Pattern checklist in this agent under category Z." Don't drop the rule on the floor. + ## Pattern checklist — scan for these on every relevant review Specific bug-classes that have bitten this repo before. When the target file matches a category, check for the pattern explicitly. Cite a finding when you see the bug; cite as "Aligned" when you see the correct pattern in place. @@ -119,6 +121,11 @@ Specific bug-classes that have bitten this repo before. When the target file mat ## Aligned (N) - ... +## Rules to encode (N) ← optional; only if Step 7 surfaced something +- **** (from Must-fix #X or Aligned #Y above): + - Belongs in: `` (e.g. `CLAUDE.md "Security Requirements"`, `.coderabbit.yaml path_instructions for **/Endpoints/*.cs`, architecture-reviewer agent Pattern Checklist → Endpoints category) + - Proposed wording: + ## Summary <2-3 sentences. Net verdict: ready to merge / needs changes / architectural question to discuss.> ``` diff --git a/CLAUDE.md b/CLAUDE.md index 46f007ca..96fb965d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -137,6 +137,25 @@ The bar isn't "document every bug fix." It's: **if the failure mode would surpri This rule is for everyone working in this repo (humans, AI assistants, future-you). Don't wait to be asked. +## Continuous Rule Encoding (the compounding loop) + +The Debugging Discipline rule above covers failures discovered while *debugging*. The same discipline applies to patterns + antipatterns discovered via *any* review surface — architecture-reviewer agent passes, CodeRabbit findings, manual code review, integration-test failures, prod incidents, security audits. **Anything that earns the label "we should never write this again" or "we should always do this when" belongs encoded in `.claude/` config + supporting docs, the same session it's identified.** Otherwise the same finding resurfaces in a future review and the cycle wastes attention. + +When you find an antipattern, rule, or specification worth encoding, write to ALL of these that apply: + +1. **CLAUDE.md** — the canonical hard/soft rule. The most relevant existing section. New section only if no fit. +2. **`.coderabbit.yaml`** `path_instructions` — file-pattern-scoped guidance so CodeRabbit catches future violations at PR-review time without re-deriving the rule. Use the existing `path:` glob entries; add a new one if no fit. +3. **[`.claude/agents/architecture-reviewer.md`](.claude/agents/architecture-reviewer.md)** "Pattern checklist" — a scan rule the agent applies on every review touching the relevant file category. So the next architectural pass catches it before code lands. +4. **[`.claude/skills/`](.claude/skills/)** — if the pattern is non-trivial enough to warrant a procedure (multi-step reasoning, specialized vocabulary), it becomes a skill. Otherwise the path_instructions + CLAUDE.md rule is enough. +5. **[`docs/STATUS.md`](docs/STATUS.md) "Open issues"** — if the finding is deferred or partial. +6. **Supporting docs** ([`docs/architecture.md`](docs/architecture.md), [`docs/performance-and-data-correctness.md`](docs/performance-and-data-correctness.md), [`docs/dev-loop.md`](docs/dev-loop.md)) — when the *why* deserves more than a CLAUDE.md one-liner. + +The threshold for encoding is the same as the Debugging Discipline rule: **if the next person could repeat the mistake (or re-derive the rule from first principles), the rule belongs in writing.** Don't encode trivial style nits or one-off mistakes; do encode security patterns, performance traps, concurrency hazards, distributed-systems gotchas, anti-IDOR patterns, anti-IDOR-test patterns, outbox traps, anything cross-cutting. + +When you push a fix PR for a real finding, the *fix itself* lives in the PR but the *rule* lives in `.claude/`. The two should land together when feasible (single PR with both), or as paired PRs when separation is cleaner. **A merged fix PR without the corresponding rule is a half-finished job** — the next instance of the same antipattern will slip through. + +This rule is for humans, AI assistants, and future-you. Don't wait to be asked. + ## Package Management - Central Package Management via `Directory.Packages.props` - all versions defined there From 7e4b400a2da519a5dff94ae8358bcf9ed941fcea Mon Sep 17 00:00:00 2001 From: emeraldleaf Date: Sat, 23 May 2026 23:04:28 -0600 Subject: [PATCH 3/3] =?UTF-8?q?docs(dev-loop):=20close=20the=20loop=20?= =?UTF-8?q?=E2=80=94=20README=20for=20.claude/=20+=20encoding=20loop=20in?= =?UTF-8?q?=20dev-loop=20md/svg?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-trips the "Continuous Rule Encoding" meta-rule (introduced in the preceding commit) into the surfaces a human or AI assistant would actually read when working in this repo: .claude/README.md (new): the missing "how to use this directory" guide. - Directory layout - Decision tree for picking between agents / commands / skills - Per-surface deep-dive: what each is, when to use, how to invoke, how to author, current inventory in this repo - Hooks + architecture-map sections (briefly; pointers to source) - "When to add a new extension" table that mirrors the Continuous Rule Encoding decision matrix from CLAUDE.md docs/dev-loop.md: adds "Continuous Rule Encoding (the compounding feedback loop)" section between the 5 stages and the Gaps list. Includes the encoding-surface table (CLAUDE.md / .coderabbit.yaml / agent / skill / status / docs) + a tiny ASCII diagram of the loop + an explicit pointer to .claude/README.md for invocation details. Names the load-bearing rule: "a merged fix PR without the corresponding .claude/ encoding is a half-finished job." docs/dev-loop.svg: two changes. 1. Iteration-loop arrow label updated from "findings → fix → push → re-review" to "findings → fix → encode rule in .claude/ → push → re-review". The encoding step is now visible at a glance, not buried in prose. 2. New "Continuous Rule Encoding" section at the bottom (180px tall, purple/AI-LLM color matching the Stage 1 Edit-time block). Shows the Finding → Encode-in-four-places → Next-cycle-prevented flow inline. Cross-references .claude/README.md for invocation patterns. 3. SVG viewBox + width + height bumped from 1750 to 1940 to accommodate the new section. Background rect resized to match. Why all three: a human reading dev-loop.md gets the prose version. A human viewing dev-loop.svg gets the visual. A human (or AI) opening the .claude/ directory gets the README explaining what each file in there is for. Three surfaces, same message, picked up at whichever angle the reader approaches from. The architecture-reviewer agent already has Step 7 (suggest encodings) from the previous commit. Together with this commit, the encoding loop is now mechanical rather than aspirational at every surface. Co-Authored-By: Claude Opus 4.7 --- .claude/README.md | 182 ++++++++++++++++++++++++++++++++++++++++++++++ docs/dev-loop.md | 37 ++++++++++ docs/dev-loop.svg | 32 +++++++- 3 files changed, 247 insertions(+), 4 deletions(-) create mode 100644 .claude/README.md diff --git a/.claude/README.md b/.claude/README.md new file mode 100644 index 00000000..d1f1c922 --- /dev/null +++ b/.claude/README.md @@ -0,0 +1,182 @@ +# `.claude/` — repository AI workflow configuration + +This directory holds the configuration that shapes how Claude Code interacts with this repository. Everything here is checked into git so the same workflow runs for every developer (and for AI assistants that pick up work in a fresh session). + +If you just want the canonical project rules, those live in [`/CLAUDE.md`](../CLAUDE.md), not here. This directory holds the *tooling* — agents, slash commands, skills, hooks, and the architecture map — that enforces and amplifies those rules. + +For the visual + narrative of how all this plumbing fits together, see [`docs/dev-loop.md`](../docs/dev-loop.md). + +--- + +## Directory layout + +``` +.claude/ +├── agents/ Specialized subagents (Agent tool) +├── architecture-map.md Repo-wide code-graph for AI orientation +├── commands/ Slash commands (/command-name) +├── scripts/ Hook scripts wired in settings.json +├── settings.json Permissions, hooks, additional dirs +├── settings.local.json Local-only overrides (gitignored) +└── skills/ On-demand specialized knowledge (Skill tool) +``` + +--- + +## The three extension surfaces — agents, commands, skills + +These three look superficially similar but solve different problems. Picking the right one matters because each has a different invocation cost + cognitive overhead. + +### Quick decision tree + +``` +Is the work a recurring motion you want as a single keystroke? +├── YES → slash command in .claude/commands/ +└── NO → Does it need a focused, independent reviewer + that won't share your conversation context? + ├── YES → subagent in .claude/agents/ + └── NO → Is it specialized knowledge the model + should pull in on demand? + ├── YES → skill in .claude/skills/ + └── NO → It's probably a CLAUDE.md rule, + not an extension surface. +``` + +### Agents (`.claude/agents/`) + +**What:** Standalone subagents the main conversation can dispatch. They start fresh with no shared context — only the prompt + the files they read. They produce a report; you decide what to do with it. + +**When to use:** +- Independent review or audit work (architecture, security, code review) +- Long-running research that would bloat the main conversation +- Parallel work that doesn't depend on the main conversation state +- Anything that benefits from "fresh eyes" — i.e. *not* primed by the conversation that surfaced the question + +**When NOT to use:** +- Quick iterative back-and-forth (use the main conversation) +- Anything that needs the conversation's prior context +- Tasks where the user expects to interactively steer + +**How to invoke:** +- From Claude Code: the model calls the Agent tool with `subagent_type: ` and a self-contained prompt +- Agents you dispatch run in their own context window with the tools listed in their frontmatter + +**Authoring:** +- A markdown file in `.claude/agents/` with YAML frontmatter: `name`, `description`, `tools` (the tools the agent can use) +- The body is the system prompt for the subagent — its job, how it works, output format, hard rules +- The `description` field is critical — Claude Code surfaces it when deciding which agent to dispatch + +**Current inventory:** +| Agent | Purpose | +|---|---| +| `architecture-reviewer.md` | Reviews a file/diff against this project's SOLID/DDD/VSA-vs-Clean/Performance/Security rules from CLAUDE.md. Produces a categorized report (Must fix / Should consider / Aligned / Rules to encode). Best invoked with a specific file path. | + +### Commands (`.claude/commands/`) + +**What:** Markdown files that define `/command-name` slash commands. They bake repeated motions into a single keystroke + a consistent procedure. Run in the main conversation (no separate context). + +**When to use:** +- Frequently-repeated procedure with multiple steps that should be done the same way each time +- Refusable patterns (e.g. "scaffold a VSA feature slice — but refuse for CatalogService because it uses Clean Architecture") +- Audit motions (`/check-rules`) +- Status / project-state updates (`/sync-status`) + +**When NOT to use:** +- One-off tasks (just ask in chat) +- Things that need fresh context (use an agent) +- Procedures so trivial they don't warrant a name + +**How to invoke:** +- Type `/command-name` in Claude Code chat +- Optional positional arguments expand into `$ARGUMENTS` in the command body +- The command's markdown body becomes the prompt for the next assistant turn — so you can include instructions, hard rules, file references, etc. + +**Authoring:** +- A markdown file in `.claude/commands/` with YAML frontmatter: `description` (one-line) and `argument-hint` (shown in chat when typing `/`) +- The body is the prompt: tell the model what to do, what to read, what to refuse, what to output + +**Current inventory:** +| Command | What it does | +|---|---| +| `/new-feature-slice ` | Scaffolds a VSA feature slice matching the `PlaceOrder.cs` canonical shape. Refuses for CatalogService (Clean Architecture). | +| `/sync-status` | Refreshes `docs/STATUS.md` from `git log` + open issues, diff-style with confirmation. | +| `/check-rules` | Audits every `See CLAUDE.md` paraphrase against the canonical rule. | + +### Skills (`.claude/skills/`) + +**What:** Specialized knowledge bundles the model loads on demand. Unlike agents (which run in a separate context), a skill's content gets *injected into the main conversation* when triggered — so the model can apply specialized procedures, vocabularies, or decision criteria mid-task. + +**When to use:** +- Procedural knowledge that doesn't apply to every conversation (so it shouldn't be in CLAUDE.md, which is always loaded) +- Cross-cutting expertise (debugging discipline, performance tuning, test-driven development) +- Externally-authored bundles you want to install once (community skills from `obra/superpowers`, `trailofbits/skills`, etc.) +- Domain-specific reference material (e.g. dotnet-performance for EF Core hot-path review) + +**When NOT to use:** +- Universal project rules (those go in CLAUDE.md) +- One-off procedures (just describe in chat) +- Things narrow enough to be a slash command + +**How to invoke:** +- **Manual:** the model calls the Skill tool with `skill: ` based on the user's request +- **Auto-trigger:** Claude Code reads each skill's frontmatter `description` field and matches against user-message intent. If the description says "Use when ... debugging ... bug ... test failure ...", the skill auto-loads when the user reports a bug. +- A skill's full contents (`SKILL.md` + referenced files) become part of the model's working context for the rest of the conversation + +**Authoring:** +- A directory in `.claude/skills//` containing at minimum `SKILL.md` with YAML frontmatter +- `name` (kebab-case, matches directory) +- `description` (load-bearing — this is what triggers auto-invocation; include trigger keywords) +- Optional sub-files (`references/`, `scripts/`) referenced from `SKILL.md` + +**Current inventory:** +| Skill | When it fires | +|---|---| +| `dotnet-performance` | EF Core queries, async/await, concurrency, GC, caching, middleware, migrations | +| `excalidraw-diagram` | Generating architecture diagrams | +| `skill-security-auditor` | Pre-install security gate for community skills | +| `verification-before-completion` | About to claim "done/fixed/passing" without evidence | +| `systematic-debugging` | Any bug, test failure, unexpected behavior (auto-triggers) | +| `variant-analysis` | Find one bug, search for sibling patterns across the codebase | +| `test-driven-development` | RED-GREEN-REFACTOR enforcement | +| `using-git-worktrees` | Feature work needing workspace isolation | +| `writing-plans` + `executing-plans` | Spec-driven multi-step work with review checkpoints | + +--- + +## Hooks (`.claude/scripts/`) + +Hooks are Bash scripts wired in `settings.json` that fire on Claude Code events. They run in the harness (not in the model's context) and can block tool calls, inject context, or audit changes. + +**Current hooks:** +| Hook | Event | Script | +|---|---|---| +| Block sync-over-async | `PreToolUse` (Edit/Write on `.cs`) | `scripts/block-sync-over-async.sh` | +| Inject STATUS.md | `SessionStart` | `scripts/inject-status.sh` | +| Cross-reference paraphrases | `PostToolUse` (Edit/Write on CLAUDE.md) | `scripts/check-claude-md-refs.sh` | + +Hooks live in `settings.json` because they need to be discoverable + version-controlled per repo. They're not described in detail here — see the script files for what each does and the canonical [`docs/dev-loop.md`](../docs/dev-loop.md) "Edit-time" section for the workflow context. + +--- + +## The architecture map (`architecture-map.md`) + +A structured, AI-consumable map of the repository: services, their shapes (Clean vs VSA), what database each owns, event flow, port interfaces, aggregates, concurrency tokens. Read by the `architecture-reviewer` agent before every review; useful for humans too. + +This file is canonical for **structure** (what's where). The canonical rules are in [`/CLAUDE.md`](../CLAUDE.md). When services/aggregates/events change materially, refresh the map (the file has regen commands at the bottom). + +--- + +## When to add a new extension + +Per CLAUDE.md "Continuous Rule Encoding": when a review surfaces an antipattern or rule worth encoding, it goes into the appropriate `.claude/` surface in the same session: + +| Finding shape | Goes in | +|---|---| +| A pattern detection that should fire on every PR touching `X/*.cs` files | `.coderabbit.yaml` `path_instructions` (file-pattern-scoped) | +| A multi-step procedure that's run repeatedly with consistent shape | `.claude/commands/.md` (slash command) | +| A specialized review/audit that needs fresh context | `.claude/agents/.md` (subagent) | +| Specialized knowledge that the model loads on demand | `.claude/skills//SKILL.md` (skill) | +| A hard rule with no narrow-trigger | `/CLAUDE.md` (the canonical rule file) | +| An automated reaction to a Claude Code event | `settings.json` hook + `.claude/scripts/.sh` | + +A fix PR + the corresponding `.claude/` encoding should land together. Per CLAUDE.md "A merged fix PR without the corresponding rule is a half-finished job." diff --git a/docs/dev-loop.md b/docs/dev-loop.md index ca2575b2..099cde05 100644 --- a/docs/dev-loop.md +++ b/docs/dev-loop.md @@ -141,6 +141,43 @@ Two integration slices today: **CatalogService** (Postgres + Redis) and **OrderS --- +## Continuous Rule Encoding (the compounding feedback loop) + +The five stages above describe how code *flows forward* from edit to runtime. This section describes how the loop *closes* — how findings discovered at any later stage become rules that prevent the same finding from appearing in the next cycle. + +Per CLAUDE.md "Continuous Rule Encoding," when a review surface (PR-time CodeRabbit, architecture-reviewer agent pass, integration-test failure, prod incident, security audit) surfaces a pattern or antipattern worth keeping, it gets encoded the same session — across as many of these surfaces as apply: + +| Encoding surface | What goes here | Catches violations at... | +|---|---|---| +| [`CLAUDE.md`](../CLAUDE.md) | Canonical hard/soft rule with reference templates | Code-generation time (Claude reads CLAUDE.md every session) | +| [`.coderabbit.yaml`](../.coderabbit.yaml) `path_instructions` | File-pattern-scoped guidance for CodeRabbit | PR-time review | +| [`.claude/agents/architecture-reviewer.md`](../.claude/agents/architecture-reviewer.md) Pattern Checklist | Per-file-category scan rules | Local agent invocation (any time) | +| [`.claude/skills/`](../.claude/skills/) | Procedural knowledge worth a dedicated bundle | On-demand or auto-trigger when the user describes the right intent | +| [`.coderabbit.yaml`](../.coderabbit.yaml) global instructions | Cross-file conventions | PR-time, all paths | +| [`.claude/scripts/`](../.claude/scripts/) hook + `settings.json` registration | Mechanical guardrails (block bad edits, inject context) | Pre/Post Edit, SessionStart, etc. | +| [`docs/STATUS.md`](STATUS.md) "Open issues" | Deferred work | Cross-session pickup | +| Supporting docs ([architecture.md](architecture.md), [performance-and-data-correctness.md](performance-and-data-correctness.md), this file) | The *why* behind a rule when it's longer than one line | Onboarding, future-you | + +**For invocation patterns** — when to use an agent vs a slash command vs a skill, and how each is triggered — see [`.claude/README.md`](../.claude/README.md). The decision tree there is the single source of truth. + +**Load-bearing rule** (from CLAUDE.md "Continuous Rule Encoding"): *a merged fix PR without the corresponding `.claude/` encoding is a half-finished job.* The fix lives in the PR but the rule lives in `.claude/` — both should land together (single PR with both, or paired PRs when separation is cleaner). + +**The architecture-reviewer agent now prompts for encodings.** Step 7 of its workflow surfaces a "Rules to encode" section in its output report, asking concretely *which CLAUDE.md section / `.coderabbit.yaml` path / Pattern Checklist category* a finding belongs to. So the encoding doesn't get dropped between "found in review" and "fixed in PR." + +``` + ┌──── encode in .claude/ + docs ────┐ + ↓ │ + Edit → Build → Test → PR → Merge → Runtime │ + │ │ + └─── review surface ────────┘ + (CodeRabbit / agent / + test failure / incident) +``` + +The compounding effect: each finding fixed-and-encoded reduces the probability that the next instance ever reaches PR review. + +--- + ## Gaps — and the pragmatic solution for each The gaps below are real. Each one is sized for how much the *actual* problem warrants — not how much could theoretically be done. diff --git a/docs/dev-loop.svg b/docs/dev-loop.svg index 316a98c1..c3db6d64 100644 --- a/docs/dev-loop.svg +++ b/docs/dev-loop.svg @@ -1,7 +1,7 @@ - + - +