diff --git a/.claude/scripts/check-file-moves.sh b/.claude/scripts/check-file-moves.sh new file mode 100755 index 00000000..2fb3ca02 --- /dev/null +++ b/.claude/scripts/check-file-moves.sh @@ -0,0 +1,84 @@ +#!/usr/bin/env bash +# PostToolUse hook on Bash. When the command is `git mv` or `git rm`, finds the source +# path(s) and greps the repo for refs to them. Prints findings as additionalContext so +# the model sees the worklist in the same session the move happened. +# +# Purpose: catch doc/comment/Dockerfile drift in the same session a file gets moved or +# deleted — same compounding loop as check-claude-md-refs.sh but for renames/deletions +# instead of CLAUDE.md edits. +# +# Why git mv / git rm specifically (not plain mv / rm): plain mv/rm is used constantly +# for temp files, build artifacts, and ephemeral work. Restricting to git-tracked +# operations dramatically cuts false-positive noise. The cost is missing the case where +# someone uses plain `mv` on a tracked file — that's the gap CodeRabbit's +# path_instruction for file rename/delete picks up at review time. +# +# See CLAUDE.md "File-move discipline" for the canonical rule. + +set -uo pipefail + +REPO_ROOT="/Users/joshuadell/NovaCraft" + +command=$(jq -r '.tool_input.command // ""' 2>/dev/null) + +# Only fire on git mv / git rm. Plain mv/rm is too noisy. +case "$command" in + *"git mv "*) ;; + *"git rm "*) ;; + *) exit 0 ;; +esac + +old_paths=() + +# git mv — capture the FIRST positional arg (the source). +if [[ "$command" =~ git[[:space:]]+mv[[:space:]]+([^[:space:]]+) ]]; then + src="${BASH_REMATCH[1]}" + case "$src" in + -*) ;; # skip if it looked like a flag + *) old_paths+=("$src") ;; + esac +fi + +# git rm [flags] ... — capture all non-flag tokens after `git rm`. +if [[ "$command" =~ git[[:space:]]+rm[[:space:]] ]]; then + args=$(echo "$command" | sed -E 's/^.*git[[:space:]]+rm[[:space:]]+//' | tr -s ' ') + for token in $args; do + case "$token" in + -*) continue ;; + "") continue ;; + *) old_paths+=("$token") ;; + esac + done +fi + +# Nothing to do. +if [ ${#old_paths[@]} -eq 0 ]; then + exit 0 +fi + +# For each old path, grep for refs and collect findings. --fixed-strings so paths +# containing dots / slashes match literally instead of as regex. +findings="" +for old_path in "${old_paths[@]}"; do + case "$old_path" in + ""|"*"|"."|"./"|".."|"./*") continue ;; + esac + + matches=$(grep -rln --fixed-strings "$old_path" \ + --include='*.md' --include='*.cs' --include='*.props' --include='*.csproj' \ + --include='*.yml' --include='*.yaml' --include='*.sh' \ + --include='Dockerfile*' \ + "$REPO_ROOT" 2>/dev/null \ + | head -30 \ + || true) + if [ -n "$matches" ]; then + findings+=$(printf "Refs to '%s' still present in:\n%s\n\n" "$old_path" "$matches") + fi +done + +if [ -z "$findings" ]; then + exit 0 +fi + +msg=$(printf 'File-move/delete detected. Refs to the OLD path may need updating before commit:\n\n%s\nUpdate the paraphrases in the same PR. See CLAUDE.md "File-move discipline".' "$findings") +jq -n --arg m "$msg" '{hookSpecificOutput: {hookEventName: "PostToolUse", additionalContext: $m}}' diff --git a/.claude/settings.json b/.claude/settings.json index 40c01b0b..2610c106 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -61,6 +61,15 @@ "command": "/Users/joshuadell/NovaCraft/.claude/scripts/check-claude-md-refs.sh" } ] + }, + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "/Users/joshuadell/NovaCraft/.claude/scripts/check-file-moves.sh" + } + ] } ] } diff --git a/.coderabbit.yaml b/.coderabbit.yaml index d4aba0c0..e7002906 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -447,6 +447,81 @@ reviews: in bash run blocks, missing `permissions:` blocks, missing `concurrency:` groups, secrets in plaintext, hardcoded tokens. + - path: "NextAurora.AppHost/AppHost.cs" + instructions: | + TOPOLOGY DOC PAIRING (CLAUDE.md "Doc-and-diagram discipline"). AppHost.cs + is the canonical source of the system topology — service count, transport + wiring, DB choices, external deps. When this file changes in a PR (new + service registration, new `WithReference`, new `AddAzure*`, new + `WaitFor`, removed resource), the topology depiction artifacts MUST be + updated in the same PR, OR the PR description names a tracking issue + for the deferred update. + + Required pairing: + - `docs/architecture.md` (the prose system map + communication patterns) + - `docs/nextaurora-architecture.svg` (rendered diagram github.com inlines) + - `docs/nextaurora-architecture.excalidraw` (editable source) + + Reviewers look at the .svg on the PR page to understand "what does this + system look like now?" — if the .svg doesn't show the new service or + transport, every subsequent PR review reasons against a stale picture. + Same for architecture.md: it's the doc people grep when onboarding or + debugging a comms issue. Flag this as a finding when AppHost.cs changes + without paired updates. Acceptable PR-description waivers: "topology + change is internal/transient (smoke-test scaffold) and will be reverted + in PR #N" or "docs/diagram updated in tracking issue #N, this PR ships + the code only because [reason]." + + - path: "NextAurora.ServiceDefaults/Extensions.cs" + instructions: | + SERVICE-REQUEST-FLOW DOC PAIRING (CLAUDE.md "Doc-and-diagram discipline"). + Extensions.cs configures the middleware pipeline ordering (auth → + correlation-id → authorization), JWT validation, and the + AddDefaultAuthentication / MapDefaultEndpoints contracts every service + composes with. When this changes (middleware order, JWT params, default + endpoint behavior), the service-request-flow depiction artifacts MUST + be updated in the same PR or named in a deferred issue: + + Required pairing: + - `docs/architecture.md` Observability section (middleware-order rules) + - `docs/service-request-flow.svg` + `docs/service-request-flow.excalidraw` + + Same reasoning as the AppHost.cs rule above — reviewers reason against + the .svg picture of the request lifecycle. Flag missing pairs. + + - path: "**" + instructions: | + FILE-MOVE DISCIPLINE (CLAUDE.md "File-move discipline"). When this PR's + diff includes a file deletion or rename, grep the repo for refs to the + OLD path and flag any matches as findings. Look in: every `*.md` + (README, docs/, .claude/, .github/), inline comments in `*.cs` + (especially comments ending with literal `See CLAUDE.md.`), `Dockerfile*` + COPY/ADD instructions, `*.csproj` ProjectReference Include="...", + `.github/workflows/*.yml` `run:` blocks, `.coderabbit.yaml` + path_instructions, `.claude/scripts/*.sh` literal-path strings, + and `.claude/commands/*.md` slash-command bodies. + + Failing to update these is exactly the drift class that produced the + broken-Dockerfile + broken-CLAUDE.md-link + stale-demo-doc fallout from + the simplicity refactor (PR #31 collapsed CatalogService to a single + project; refs to the pre-collapse 4-project layout sat silent in + `Dockerfile.catalog`, `docs/demo-deployment*.md`, and + `docs/performance-and-data-correctness.md` for months — only caught + when the doc-currency sweep happened in PR #112). Even a single missed + ref is a finding. + + Mechanical enforcement already covers part of this: the + `.github/workflows/ci.yml` "Broken-link audit" step blocks the merge + when relative markdown links to local source files don't resolve. + That catches the broken-link case but not the "ref still points at a + file that happens to exist for a different reason" case — which is + where this CodeRabbit pass earns its keep. + + Allowlist: `.claude/audits/INDEX.md` legitimately links to gitignored + per-article files (copyright reason — see + `.claude/commands/article-audit.md` step 5 "Copyright note"). The CI + guard excludes it; do NOT flag links from INDEX.md as drift. + # Chat: allow @coderabbitai mentions to ask follow-up questions on a PR. chat: auto_reply: true diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 75c945ce..bb6476c1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -109,6 +109,24 @@ jobs: exit 1 fi + # CLAUDE.md size budget — enforces the "Keep this file lean" discipline from + # CLAUDE.md "Continuous Rule Encoding" surface 1. Every byte of CLAUDE.md is loaded + # into every Claude Code session AND is cognitive overhead for human readers. Soft + # warning at 400 lines, hard fail at 500. When approaching either, audit each rule + # and move detail to the relevant paired doc in docs/ or skill in .claude/skills/; + # CLAUDE.md keeps headline + one-paragraph summary + link. + - name: CLAUDE.md size budget + run: | + lines=$(wc -l < CLAUDE.md) + echo "CLAUDE.md is $lines lines." + if [ "$lines" -gt 500 ]; then + echo "::error file=CLAUDE.md::CLAUDE.md exceeds 500-line hard limit (current: $lines). Move detail to docs/ or .claude/skills/ and leave a headline + link. See CLAUDE.md 'Continuous Rule Encoding' surface 1." + exit 1 + fi + if [ "$lines" -gt 400 ]; then + echo "::warning file=CLAUDE.md::CLAUDE.md is approaching the size budget ($lines lines; soft limit 400, hard limit 500). Consider moving detail out." + fi + # Broken-link audit: relative markdown links to local source files (.cs/.csproj/.props/ # .yml/.svg/.excalidraw/.md) that don't resolve. Catches drift from file renames and # the kind of fallout the simplicity refactor produced — CLAUDE.md citations pointing at @@ -144,6 +162,81 @@ jobs: # step 5 "Copyright note" — the contract is "INDEX ships, per-article files don't." # On a contributor's machine the links resolve; on the CI runner they don't, by design. + # Broken-COPY audit: Dockerfile* COPY/ADD lines that reference source paths which + # don't exist in the build context (the repo root). This is the gap the markdown + # audit above can't close — `Dockerfile.catalog` rotted silently for months after + # PR #31 (VSA collapse) because the broken paths only failed at deploy time, not + # at build/test time. The Fly demo at https://catalog-api-demo.fly.dev kept running + # pre-collapse code; any redeploy would have failed on the first COPY of a + # nonexistent csproj. Same shape as the markdown audit: parse, check existence, fail. + # Handles the common COPY forms: `COPY src dst`, `COPY src1 src2 dst` (multi-src), + # and `COPY --from=stage src dst` (cross-stage; skips the src check since src is in + # another build stage, not the repo). Skips wildcards (`COPY src/*.json .`) since + # those resolve at build time, not against the repo tree. See CLAUDE.md "File-move + # discipline" for the canonical rule. + - name: Broken-COPY audit — Dockerfile source paths + run: | + fail=0 + while IFS= read -r dockerfile; do + # Read each COPY/ADD line, strip the destination (last token), check each source. + while IFS= read -r line; do + # Skip `COPY --from=...` cross-stage copies (src is from a build stage, not the repo). + case "$line" in *"--from="*) continue ;; esac + # Strip the instruction keyword and any flags. + args=$(echo "$line" | sed -E 's/^[[:space:]]*(COPY|ADD)[[:space:]]+//; s/--[a-zA-Z0-9=._/-]+[[:space:]]+//g') + # Last token is the destination; everything before is source(s). + srcs=$(echo "$args" | awk 'NF>1 {for(i=1;i/dev/null || true) + while IFS= read -r svg; do + excalidraw="${svg%.svg}.excalidraw" + if [ ! -f "$excalidraw" ]; then + echo "::error file=$svg::rendered .svg without .excalidraw source → expected $excalidraw" + fail=1 + fi + done < <(find docs -maxdepth 2 -type f -name '*.svg' 2>/dev/null || true) + exit "$fail" + # Testcontainers-based integration tests, in their own job: they need Docker (the # ubuntu-latest runner ships it at the standard /var/run/docker.sock, so Testcontainers # auto-detects — no DOCKER_HOST override, unlike macOS Docker Desktop locally). Kept diff --git a/CLAUDE.md b/CLAUDE.md index 12480b15..595fbd8d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -170,20 +170,33 @@ The bar isn't "document every bug fix." It's: **if the failure mode would surpri **When tightening or changing a CLAUDE.md rule**, grep the repo for files that paraphrase it (inline comments in `.cs`/`.props`/`.csproj`, supporting docs, README sections) and update each so they stay aligned. CLAUDE.md is canonical; everywhere else summarizes. Convention: any inline comment that summarizes a CLAUDE.md rule ends with `See CLAUDE.md.` so it's findable via `grep -rn "See CLAUDE.md"`. A PostToolUse hook surfaces candidate files automatically when CLAUDE.md is edited (see `.claude/settings.json`). -This rule is for everyone working in this repo (humans, AI assistants, future-you). Don't wait to be asked. +**File-move discipline — when deleting or renaming a file**, grep the repo for refs to the *old* path and update them in the same PR. Look in: `*.md` (docs, READMEs, INDEX), inline comments in `*.cs`, `Dockerfile*` COPY/ADD lines, `*.csproj` ProjectReference Include, `.github/workflows/*.yml` `run:` blocks, `.coderabbit.yaml` path_instructions. **Same compounding-loop principle as the CLAUDE.md rule above**, just for file structure instead of rule text. Failing to do this is the same drift class as the simplicity refactor fallout — `Dockerfile.catalog` referenced `CatalogService/CatalogService.Api/CatalogService.Api.csproj` for months after PR #31 collapsed CatalogService to a single project, broken silently until someone tried to redeploy. Enforcement is layered: (a) the `check-file-moves.sh` PostToolUse hook prints stale-ref candidates after `git mv` / `git rm`, (b) the CI broken-link audit in `.github/workflows/ci.yml` catches stale markdown links at PR-merge gate, (c) the `.coderabbit.yaml` "**" path_instruction flags missed refs at review time, (d) this rule documents the discipline. + +**Doc-and-diagram discipline — docs and diagrams are the review surface, not byproducts.** When reviewers (human or CodeRabbit) want to *observe* what the system does today, they read `docs/architecture.md` and look at `docs/nextaurora-architecture.svg`. If those are stale, every review reasons against a fiction — and drift becomes invisible until someone hits the discrepancy in production. So when code or config changes affect what a doc or diagram *depicts*, the doc/diagram updates in the same PR — or the PR description names the deferred follow-up issue. Concrete pairings (extend as new categories appear): + +- **Topology changes** (new service / new transport / new DB / new external dep): `NextAurora.AppHost/AppHost.cs` ↔ `docs/architecture.md` ↔ `docs/nextaurora-architecture.{svg,excalidraw}` +- **Communication-pattern changes** (new gRPC client, new ASB subscription, new endpoint family): same trio plus `docs/messaging-transport-selection.md` +- **EF / caching / outbox / migration mechanism changes**: `docs/performance-and-data-correctness.md` + `docs/ef-core.md` + `docs/cqrs-data-access.md` + sibling diagram (e.g. `docs/efcore-query-write.{svg,excalidraw}`, `docs/transactional-outbox.{svg,excalidraw}`, `docs/hybridcache-flow.{svg,excalidraw}`) +- **Loop / process changes** (new hook, new skill, new CI step, new agent): `docs/dev-loop.md` + `.github/AI_WORKFLOW.md` + `docs/dev-loop.{svg,excalidraw}` +- **Service-request-flow changes** (middleware order, auth flow, correlation propagation): `NextAurora.ServiceDefaults/Extensions.cs` ↔ `docs/architecture.md` Observability section ↔ `docs/service-request-flow.{svg,excalidraw}` + +Diagrams are always paired: every `docs/*.excalidraw` ships with a sibling `docs/*.svg`. The `.excalidraw` is the authoritative editable source; the `.svg` is what github.com renders inline (so reviewers see it on the PR page). Editing one without re-rendering the other breaks the pairing — the source no longer matches what reviewers actually see. The render pipeline lives in `.claude/scripts/rebuild-diagrams.sh` (Playwright-driven; reads `.excalidraw`, writes `.svg` + `.png`). Enforcement is layered: (a) the CI `Diagram-pair audit` step fails the build if any `.excalidraw` lacks its `.svg` (or vice versa), (b) the `.coderabbit.yaml` path_instructions for topology-touching files flag missing doc/diagram pairs at review time, (c) this rule documents the discipline. The "is the rendered SVG still in sync with the source?" deeper check isn't mechanically enforced yet (would require the Playwright render in CI); for now, when you edit a `.excalidraw`, run `.claude/scripts/rebuild-diagrams.sh` before committing. + +All three rules are 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: +When you find an antipattern, rule, or specification worth encoding, write to the **smallest set** of these that applies. Default to NOT touching CLAUDE.md unless the rule is genuinely always-on: -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. +1. **CLAUDE.md** — the canonical **always-on** rules layer. **Keep this file lean.** Every byte is loaded into every Claude Code session and every line is cognitive overhead for human readers. Add a rule here ONLY if every session needs it. **One-paragraph maximum per rule.** If a rule needs more than ~6 lines, the rule itself stays as a bolded headline + one-paragraph summary in CLAUDE.md; the detail (the *why*, the failure modes, edge cases, worked examples) moves to a paired doc in `docs/` or the relevant skill, and CLAUDE.md gets a `See [docs/X.md "section"](docs/X.md#section)` pointer. Test: *could this rule + its rationale fit on one screen?* If no, decompose. **CI enforces a soft size budget on this file (warning at 400 lines, hard fail at 500)** to prevent silent bloat. +2. **`.coderabbit.yaml`** `path_instructions` — file-pattern-scoped guidance so CodeRabbit catches future violations at PR-review time without re-deriving the rule. **Most per-file rules belong here, not in CLAUDE.md.** 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. **GitHub Issues** — if the finding is deferred or partial. Open an issue with `rule-encoding-deferred` (when code shipped but the encoding is still pending) or the relevant `type/*` + `area/*` labels. The issue is the durable record; STATUS.md no longer carries an "Open issues" list (it's now a thin entry-point doc pointing at the issues board). -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. +4. **[`.claude/skills/`](.claude/skills/) + [`.claude/commands/`](.claude/commands/)** — if the pattern is non-trivial enough to warrant a procedure (multi-step reasoning, specialized vocabulary), it becomes a skill or slash command. +5. **Supporting docs + paired diagrams** ([`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), and their sibling `.svg`/`.excalidraw` pairs) — when the *why* deserves more than a one-liner, or when reviewers need a picture to reason against. See "Doc-and-diagram discipline" above. + +**Deferral surface (NOT part of the encoding loop).** GitHub Issues (`rule-encoding-deferred` label) tracks findings where code shipped but the encoding hasn't yet. **The issue is a placeholder — a TODO that ensures the encoding eventually happens.** The issue itself is NOT the encoding; the PR that adds the rule to one of the five surfaces above is. Closed issues are not read again in future sessions; the rules live in the surfaces, not in the issue tracker. 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. @@ -221,29 +234,29 @@ This rule is for humans, AI assistants, and future-you. Don't wait to be asked. ## Performance Rules -These are always-on. Deeper guidance (modern EF features, transactions, caching strategies, GC pressure, migrations, benchmarking) lives in the `dotnet-performance` skill. +These are always-on headlines. **Full rationale, edge cases, and worked examples live in [docs/performance-and-data-correctness.md](docs/performance-and-data-correctness.md) "The 14 always-on rules" and the [`dotnet-performance` skill](.claude/skills/dotnet-performance/SKILL.md).** CLAUDE.md keeps the rule + the one-line "what must be true" — not the deep dive. -- **EF Core reads — project in EF, not in memory.** Read paths must `AsNoTracking()` + `.Select(...)` into a DTO **inside the IQueryable**, and the repo/query method **returns the DTO**, not a domain entity. Two distinct wins: (1) SQL emits only the DTO's columns instead of every column on the entity, (2) when the DTO includes a nested collection (`Lines = o.Lines.Select(...).ToList()`), EF Core auto-splits into a separate query for the children — so the parent isn't repeated across a JOIN and there are no cartesian rows over the wire. Mapping in the handler (`repo.GetX() → entity → Mapper.ToDto(entity)`) is the anti-pattern: it forces EF into a single-JOIN query (the entity graph must materialize as the JOIN says), you pay for the wasted columns *and* the cartesian rows from the JOIN, and you double-materialize (rows → entity → DTO). If you genuinely must materialize an entity graph without tracking — rare on a read path — use `AsNoTrackingWithIdentityResolution()` so duplicate **client-side objects** stitch into one parent; note the cartesian SQL rows still hit the wire (for that, `AsSplitQuery()`). Writes load the aggregate tracked because they mutate it. **Read/write split is the rule, not "future cleanup":** when the same `GetByIdAsync` is shared between a query handler and a command/event/saga handler, add a sibling read method that projects to DTO; the entity-returning method stays for the write path. See [docs/cqrs-data-access.md](docs/cqrs-data-access.md) for the canonical shape per architecture style (VSA: sibling DTO method on the existing repo interface; Clean: separate `IFooReadStore` in Application) and "Why projection kills cartesian rows" for the EF mechanism. +- **EF Core reads — project in EF, not in memory.** `AsNoTracking()` + `.Select(...)` into a DTO *inside the IQueryable*; the method returns the DTO, not the entity. Writes load the aggregate tracked. Read/write split is the rule, not future cleanup. See [docs/performance-and-data-correctness.md "EF Core reads use AsNoTracking() + projection"](docs/performance-and-data-correctness.md#1-ef-core-reads-use-asnotracking--projection) and [docs/cqrs-data-access.md](docs/cqrs-data-access.md) for the per-architecture-style shapes. - **No N+1**: use `Include` or projection. Never query inside a `foreach` over results from another query. -- **Non-sargable predicates defeat indexes — fix at write time, not at read time.** A `Where(...)` that wraps the column in a function (`u.Email.ToLower() == x`, `o.CreatedAt.Date == today`) can't use a B-tree index on that column even if one exists — the planner falls back to a full scan. The right fix is at write time: normalize on insert/update (e.g. `EmailNormalized` column populated by the aggregate factory + projected to in `Where(u => u.EmailNormalized == emailNormalized)`), or use a case-insensitive collation at the column level. **Leading-wildcard substring search (`LIKE '%text%'`, `EF.Functions.ILike(p.Name, "%text%")`) isn't B-tree-indexable in any database** — escalate to Postgres `tsvector` full-text search or a dedicated search engine (Elasticsearch/OpenSearch/Meilisearch) when load justifies it. Reference: [CatalogService/Features/SearchProducts.cs](CatalogService/Features/SearchProducts.cs) documents the leading-wildcard trade-off explicitly (intentional; full-text is the named next step if it becomes a bottleneck). The deeper principle: indexes carry a write cost — every insert/update touches every index on the table — so an index the planner can't use is pure overhead, not free defense-in-depth. Adding more indexes isn't a universal speed-up; treat the index list like an interface — each one earns its keep against a real query. -- **Async on request paths**: `await` everywhere. Never `.Result`, `.Wait()`, or `.GetAwaiter().GetResult()`. Every async method on a request path takes and propagates `CancellationToken`. -- **Parallelize independent awaits with `Task.WhenAll` — sequential `await`s serialize latency for free.** Async makes a single wait non-blocking; it does not make a *sequence* of waits cheap. When a handler makes N independent I/O calls — N gRPC requests to different services, N HTTP calls to different external APIs, N queries against *different* DbContexts (one per service) — sequential `await`s pay the sum of all latencies, while `Task.WhenAll` pays the max. The anti-shape: `var user = await ...; var orders = await ...; var notifications = await ...;` — three calls that don't depend on each other but execute serially over the request's full latency. The right shape: kick all three off, await once, project the results. Reference: [OrderService/Features/PlaceOrder.cs:93](OrderService/Features/PlaceOrder.cs) — `Task.WhenAll(request.Lines.Select(line => catalogClient.GetProductAsync(line.ProductId, ct)))` is the canonical shape (gRPC fan-out over independent line items, parallelism over the wire, no shared mutable state). The file documents the DbContext safety caveat at lines 89–92 explicitly. **Don't parallelize:** (a) dependent operations where the output of one feeds the input of another (`var user = await ...; var orders = await GetByUserId(user.Id, ct);`), (b) operations sharing the same EF `DbContext` scope — DbContext is NOT thread-safe and parallel EF queries against the same context throw or corrupt state (use `IDbContextFactory` to mint one context per task; see "DbContext is not thread-safe" rule below), (c) operations whose failures must be observed independently — `Task.WhenAll` surfaces only the first exception; the rest run to completion but get swallowed. Use `Task.WhenAll(...)` followed by inspection of each task's `.Exception` (or `Task.WhenEach` in .NET 10) when multi-failure surfacing matters. -- **Long-running work belongs on the message bus, not the synchronous HTTP handler.** If a write path would take more than ~1s (multi-step external API chain, aggregation over thousands of rows, bulk import, report generation), reshape the endpoint as **202 Accepted**: validate + persist a tracking row + publish a Wolverine message + return `202` with the polling key in the body and a `Location` header pointing to a status endpoint. A background handler does the actual work; the client polls (`GET /jobs/{id}`, `GET /orders/{id}`, etc.) or receives a push (SignalR/SSE/email when the job completes). **What counts as "the tracking row":** the aggregate being created can BE the tracking row when its ID is the polling key — that's the `POST /api/v1/orders` shape (the `Order` row IS the tracking record; `GET /api/v1/orders/{id}` IS the polling endpoint). A separate jobs table is only needed when there's no natural aggregate (bulk import of CSV with no per-row entity, report generation with no persistent output). **The synchronous parts commit atomically two ways:** (a) when the endpoint dispatches via `bus.InvokeAsync(command, ct)`, `AutoApplyTransactions` wraps the handler — `SaveChanges` flushes the entity write and Wolverine's staged envelope in one DB transaction; (b) when the endpoint persists + publishes inline (no handler dispatch), use the `BeginTransactionAsync` → work + `PublishAsync` → `SaveChangesAsync` → `CommitAsync` wrap from the Outbox-outside-handler trap (see Observability → Transactional Outbox below). Skipping `SaveChangesAsync` after `PublishAsync` and before `Commit` silently drops the staged envelope. **Why it matters:** the HTTP request holds a thread, a DB connection, and a concurrency-budget slot for the full duration of the handler — a small spike on a slow endpoint can starve the rest of the API. Response time and work duration are different things; the rule is to keep response time bounded. NextAurora already has the full machinery — Wolverine + Service Bus + outbox + saga handlers — so the rule is "use it when a handler would otherwise block." The current `POST /api/v1/orders` is the canonical reference: the handler validates + persists the `Order` + stages `OrderPlacedEvent` + returns `OrderId`, then PaymentService + ShippingService handle downstream work async via the saga. Note that `bus.InvokeAsync` awaits the *Place Order handler* synchronously — what makes this the right shape isn't the response code, it's that the handler only does validate-persist-stage and stays sub-second; the minutes-scale work is downstream consumers of the staged event. **Same rule for Wolverine handlers themselves**: a handler body that runs for minutes is the same anti-pattern with a different colored connection — break the work into a follow-up message handler. **Cloud-managed alternatives** when the worker pool needs scale-to-zero or a multi-step durable workflow: AWS SQS + Lambda or Azure Service Bus + Azure Functions for stateless workers; Azure Durable Functions or AWS Step Functions for multi-step orchestration with timers/retries; Temporal for hours-to-days workflows with first-class durable execution. Trade-off is the usual one — less ops, more vendor coupling. -- **Fan-out belongs on the message bus, not in a synchronous handler loop.** A handler that iterates a recipient list inline (`foreach (var follower in followers) await _sender.SendAsync(...)`) holds the request open for N × per-recipient-latency, concentrates the work on one process, and creates traffic spikes that can starve the rest of the system (millions of follower notifications fired by one celebrity post). The right shape: publish **one Wolverine message per recipient** (or per batch of K recipients) and return immediately; per-recipient handlers run in parallel under Wolverine's `MaxDegreeOfParallelism` throttle, set per-handler in `Program.cs` (`opts.LocalQueueFor().MaxDegreeOfParallelism(N)`). The throttle gives natural back-pressure — fast producers can't starve slow consumers, and a notification spike doesn't pin a thread or saturate the downstream provider. This is the same principle as "Long-running work belongs on the message bus" applied to fan-out specifically: *accept the work, don't do the work*. Not retroactively violated today (NotificationService receives one inbound event = one outbound notification), but the rule is preventative for any future broadcast-to-N feature (multi-tenant announcements, post-with-followers, abandoned-cart drips, etc.). -- **Pagination**: every list endpoint must paginate with a server-side size cap (≤ 100). Use keyset pagination for large offsets. -- **Bulk ops**: use `ExecuteUpdateAsync` / `ExecuteDeleteAsync` — never load thousands of rows just to mutate or delete them. -- **Optimistic concurrency**: every updatable aggregate must have a concurrency token (Postgres `xmin` or a row-version column). Last-write-wins is not acceptable. -- **Entity IDs use `Guid.CreateVersion7()`, not `Guid.NewGuid()`.** UUID v7 (first 48 bits = Unix-ms timestamp, remaining 74 bits random) is time-ordered, so PK inserts append-extend the B-tree index instead of splitting pages everywhere — kills the index-fragmentation tax that random UUID v4 inserts pay on every write. .NET 9+ API, no third-party package needed, drop-in same `Guid` type. Apply in aggregate factory methods (`Order.Create`, `Payment.Create`, etc.) — the canonical spot to mint domain IDs. **Trade-off:** v7's timestamp is decodable from the ID, so the mint time leaks to anyone holding it. Fine for buyer-scoped resources (IDOR check gates visibility — non-owners can't see the ID at all) and for naturally-public timestamped resources (Product creation time isn't sensitive; often returned in the response anyway). **Don't use v7** for IDs where the mint time IS sensitive (security tokens, admin-only internal references). Existing v4 IDs in the DB stay as-is — v4 and v7 coexist in the same `Guid` column with no migration required; the rule applies to *new* IDs. -- **Outbox atomicity**: the entity write and outbox-row write commit in the same transaction. Prefer one `SaveChanges` call; otherwise use `BeginTransactionAsync` explicitly. -- **`DbContext` is not thread-safe**: parallel queries (`Task.WhenAll`) require `IDbContextFactory` — one context per task. The scoped per-request context handles only sequential work. -- **Structured logging**: use message templates (`"User {UserId} logged in"`) with parameter placeholders, never string concatenation or interpolation. This is also required for the correlation/user/session scope to work. +- **Non-sargable predicates defeat indexes — fix at write time.** A `Where(...)` that wraps the column in a function (`u.Email.ToLower() == x`) can't use a B-tree index. Normalize on insert/update (an `EmailNormalized` column populated by the aggregate factory) or use a case-insensitive column collation. Leading-wildcard substring search (`LIKE '%text%'`) needs `tsvector`/Elasticsearch/Meilisearch when load justifies it — until then, document the trade-off at the call site. See [docs/performance-and-data-correctness.md](docs/performance-and-data-correctness.md) + `CatalogService/Features/SearchProducts.cs` for the documented trade-off pattern. +- **Async on request paths**: `await` everywhere. Never `.Result`, `.Wait()`, or `.GetAwaiter().GetResult()` — banned at build time by `BannedSymbols.txt` and blocked at edit time by the `block-sync-over-async` hook. Every async method takes and propagates `CancellationToken`. +- **Parallelize independent awaits with `Task.WhenAll` — sequential `await`s serialize latency for free.** Reference shape: [OrderService/Features/PlaceOrder.cs:93](OrderService/Features/PlaceOrder.cs). Caveats: don't parallelize dependent operations, don't share a `DbContext` across tasks (use `IDbContextFactory`), and use per-task exception inspection when multi-failure observability matters. See [docs/performance-and-data-correctness.md "Parallel awaits"](docs/performance-and-data-correctness.md). +- **Long-running work belongs on the message bus, not the synchronous HTTP handler.** If a write path takes more than ~1s, reshape as **202 Accepted**: validate + persist a tracking row + publish a Wolverine message + return immediately. The aggregate being created can BE the tracking row (the `POST /api/v1/orders` shape). Same rule applies to Wolverine handlers themselves — minutes-scale work belongs in a follow-up message. See [docs/performance-and-data-correctness.md "Long-running work belongs on the message bus"](docs/performance-and-data-correctness.md#long-running-work-belongs-on-the-message-bus-not-the-synchronous-http-handler) for the full pattern + cloud-managed alternatives (Durable Functions, Step Functions, Temporal). +- **Fan-out belongs on the message bus, not a synchronous handler loop.** A handler iterating a recipient list inline holds the request open for N × latency. Right shape: publish one Wolverine message per recipient (or per batch of K), throttle with `MaxDegreeOfParallelism`. See [docs/architecture.md](docs/architecture.md). +- **Pagination**: every list endpoint paginates with a server-side size cap (≤ 100). Keyset pagination for large offsets. +- **Bulk ops**: use `ExecuteUpdateAsync` / `ExecuteDeleteAsync` — never load thousands of rows to mutate or delete them. +- **Optimistic concurrency**: every updatable aggregate has a concurrency token (Postgres `xmin` or a row-version column). Last-write-wins is not acceptable. See [docs/performance-and-data-correctness.md "Optimistic concurrency tokens"](docs/performance-and-data-correctness.md#decision-optimistic-concurrency-tokens). +- **Entity IDs use `Guid.CreateVersion7()`, not `Guid.NewGuid()`.** Time-ordered, so PK inserts append-extend the B-tree index instead of fragmenting it. Apply in aggregate factory methods. Trade-off: v7's timestamp is decodable from the ID — don't use for IDs where mint time is sensitive (security tokens, admin-only refs). Existing v4 IDs coexist with new v7 IDs in the same `Guid` column. +- **Outbox atomicity**: entity write + outbox-row write commit in the same transaction. Prefer one `SaveChanges` call; otherwise `BeginTransactionAsync` explicitly. See [Observability → Transactional Outbox](#transactional-outbox-wolverine) for the non-handler-code trap. +- **`DbContext` is not thread-safe**: parallel queries (`Task.WhenAll`) require `IDbContextFactory` — one context per task. +- **Structured logging**: message templates (`"User {UserId} logged in"`) with parameter placeholders, never concatenation or interpolation. Required for correlation/user/session scope. - **No logging in tight loops**: log summaries (`"Processed {Count} items"`), not per-item lines. -- **DB connection hold time**: open → query → dispose. Don't `await` unrelated work (HTTP calls, message publishes) while a connection is open. -- **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). +- **DB connection hold time**: open → query → dispose. Don't `await` unrelated work (HTTP, messaging) while a connection is open. +- **Cache invalidation in the write path**: a handler mutating a cached entity invalidates or updates the cache in the same handler — not "later" or "via TTL". +- **Migrations are immutable once applied**: never edit a migration that has run anywhere. Destructive changes (drop column/table, rename, NOT NULL on existing column) need a multi-step plan. +- **Measure before optimizing**: BenchmarkDotNet for code paths, `dotnet-counters`/k6 for system behavior, `ToQueryString()` for EF. Don't add caching, compiled queries, `ValueTask`, or `AsSplitQuery()` on intuition. +- **`AsSpan` over `Substring`** on profiled synchronous hot paths only. `Span` is a `ref struct` — can't cross `await` boundaries, so it's rarely usable in NextAurora's async-everywhere handlers. The compiler will stop you. Until profiling shows a string-allocation hot path, `Substring` is fine. +- **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, (b) profiling proves EF is the bottleneck, or (c) LINQ obscures intent on a SQL aggregation. Always use `ctx.Database.GetDbConnection()` so Dapper shares the EF connection + ambient transaction. Writes still go through aggregates + EF. See [docs/performance-and-data-correctness.md "Dapper escape hatch"](docs/performance-and-data-correctness.md#decision-when-to-reach-past-ef-core-dapper-escape-hatch). ## Testing @@ -267,79 +280,22 @@ dotnet run --project NextAurora.AppHost # Starts everything via Aspire ## Observability & Context Propagation -### Correlation ID, User ID, Session ID - -Every HTTP request and Service Bus message carries three context identifiers: - -| Concept | Activity Baggage Key | HTTP / SB Property | Logger Scope Key | -|---------|--------------------|--------------------|-----------------| -| Correlation | `correlation.id` | `X-Correlation-Id` | `CorrelationId` | -| User | `user.id` | `X-User-Id` | `UserId` | -| Session | `session.id` | `X-Session-Id` | `SessionId` | - -**Sources:** -- `correlation.id` — from `X-Correlation-Id` request header, or generated from trace ID -- `user.id` — from `ClaimTypes.NameIdentifier` JWT claim (`sub`); null when unauthenticated -- `session.id` — from `X-Session-Id` request header (client-generated browser/app session UUID); null if not provided - -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). +Correlation/user/session context is propagated across HTTP + Service Bus by `CorrelationIdMiddleware` (HTTP entry), `ContextPropagationMiddleware` (Wolverine incoming), and `OutgoingContextMiddleware` (outbound). Wired via `opts.AddNextAuroraContextPropagation()` in each service's `Program.cs`. **Full mechanism + headers/baggage mapping + scope behavior + wiring details: see [docs/observability-and-context-propagation.md](docs/observability-and-context-propagation.md).** -### Wolverine middleware classes must use instance methods +**Always-on traps:** -`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. +- **HTTP middleware order — strict.** `CorrelationIdMiddleware` runs AFTER `UseAuthentication` (it reads `context.User` to populate `UserId`) and BEFORE `UseAuthorization` (so 401/403 denials log with the user). Canonical order in `MapDefaultEndpoints` (see [Extensions.cs](NextAurora.ServiceDefaults/Extensions.cs)): + ```csharp + app.UseExceptionHandler(); + app.UseAuthentication(); + app.UseMiddleware(); + app.UseAuthorization(); + ``` -### Wolverine pipeline scope +- **Wolverine middleware classes must use instance methods.** `opts.Policies.AddMiddleware()` only discovers `Before`/`After`/`Finally` (and `Async` variants) as instance methods on a public class with a public constructor. Static methods aren't discovered → `InvalidWolverineMiddlewareException` at host startup. Suppress S2325 ("should be static") with a `Justification` referencing this rule. -`ContextPropagationMiddleware` opens a `logger.BeginScope()` before invoking each handler so **every log line emitted anywhere in the handler** carries `CorrelationId`, `UserId`, and `SessionId` automatically. Wolverine's `Policies.LogMessageStarting()` adds handler name + elapsed time on top of that. - -Order in the Wolverine pipeline: FluentValidation policy (`opts.UseFluentValidation()`, rejects invalid commands before handlers run) → `ContextPropagationMiddleware` (opens logger scope) → handler. `opts.Policies.AutoApplyTransactions()` wraps each EF-touching handler chain so outgoing messages are persisted to the outbox in the same DB transaction as the entity write. - -### Wolverine envelope context extraction - -Handlers don't extract context manually — `ContextPropagationMiddleware` does it for them. The middleware reads `Envelope.Headers["X-Correlation-Id" | "X-User-Id" | "X-Session-Id"]` (Wolverine's transport-agnostic header bag, mapped to Service Bus `ApplicationProperties` over the wire), restores them into Activity baggage, and opens a `logger.BeginScope()`. After the handler runs, `Finally()` disposes the scope. - -Outgoing context is stamped by `OutgoingContextMiddleware`, which reads Activity baggage and writes the same headers onto outgoing envelopes. The full mechanism is registered via `opts.AddNextAuroraContextPropagation()` in each service's `Program.cs`. - -Never add null/empty keys to logging scope dictionaries — use `if (x is not null) scope["Key"] = x`. Always pass `StringComparer.Ordinal` when constructing `Dictionary` (per Meziantou MA0002). - -### Transactional Outbox (Wolverine) - -Each event-publishing service (Order, Payment, Shipping) runs Wolverine's transactional outbox. Outgoing events are persisted to a `wolverine.*` schema in the same DB transaction as the entity write, then dispatched to Azure Service Bus by Wolverine's background flush. Configuration lives in each service's `Program.cs`: - -```csharp -opts.PersistMessagesWithSqlServer(connectionString, "wolverine"); // or PersistMessagesWithPostgresql -opts.UseEntityFrameworkCoreTransactions(); -opts.Policies.AutoApplyTransactions(); -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: [PaymentRecoveryJob](PaymentService/Infrastructure/PaymentRecoveryJob.cs) — the canonical inline implementation of this wrapper (the previous `IPaymentRepository.ExecuteInTransactionAsync` wrapper was deleted in the simplicity refactor; the pattern itself is unchanged, just inlined). 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. +- **Transactional outbox — outside a Wolverine handler, atomicity trap.** `AutoApplyTransactions` only wraps Wolverine handler chains. Code OUTSIDE handlers (`BackgroundService` sweepers, recovery jobs, admin endpoints publishing from non-handler context) needs an explicit wrap: `BeginTransactionAsync` → entity write + `PublishAsync` → **`SaveChangesAsync`** → `CommitAsync`. Skipping the `SaveChangesAsync` between publish and commit silently drops the staged envelope. Reference: [PaymentRecoveryJob](PaymentService/Infrastructure/PaymentRecoveryJob.cs). Full mechanism + canonical wrapper: see [docs/observability-and-context-propagation.md "Outbox outside a Wolverine handler"](docs/observability-and-context-propagation.md#outbox-outside-a-wolverine-handler--atomicity-trap). -### Event Replay +- **Structured logging scope hygiene**: never add null/empty keys to scope dictionaries — `if (x is not null) scope["Key"] = x`. Always pass `StringComparer.Ordinal` when constructing `Dictionary` (per Meziantou MA0002). -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. +Replay rides Wolverine's `IMessageStore` + DLQ tooling. The previous hand-rolled `EventLogs` table was deleted as dead code post-Wolverine. diff --git a/docs/dev-loop-scaffolding.excalidraw b/docs/dev-loop-scaffolding.excalidraw new file mode 100644 index 00000000..4c4504b6 --- /dev/null +++ b/docs/dev-loop-scaffolding.excalidraw @@ -0,0 +1,51 @@ +{ + "type": "excalidraw", + "version": 2, + "source": "https://excalidraw.com", + "elements": [ + {"type":"text","id":"title","x":600,"y":30,"width":500,"height":36,"text":"Continuous Rule Encoding Loop","originalText":"Continuous Rule Encoding Loop","fontSize":28,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#1e40af","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":1001,"version":1,"versionNonce":1002,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"subtitle","x":600,"y":75,"width":500,"height":22,"text":"How AI-assisted rules compound over time","originalText":"How AI-assisted rules compound over time","fontSize":16,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#64748b","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":1003,"version":1,"versionNonce":1004,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"ellipse","id":"auth","x":750,"y":135,"width":200,"height":70,"strokeColor":"#c2410c","backgroundColor":"#fed7aa","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":1010,"version":1,"versionNonce":1011,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"auth_text","type":"text"},{"id":"arr_auth_spec","type":"arrow"},{"id":"loopback","type":"arrow"}],"link":null,"locked":false,"roundness":null}, + {"type":"text","id":"auth_text","x":770,"y":159,"width":160,"height":22,"text":"AUTHORING","originalText":"AUTHORING","fontSize":18,"fontFamily":3,"textAlign":"center","verticalAlign":"middle","strokeColor":"#374151","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":1012,"version":1,"versionNonce":1013,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":"auth","lineHeight":1.25}, + {"type":"arrow","id":"arr_auth_spec","x":850,"y":207,"width":0,"height":75,"strokeColor":"#1e3a5f","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":1020,"version":1,"versionNonce":1021,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"points":[[0,0],[0,75]],"startBinding":{"elementId":"auth","focus":0,"gap":2},"endBinding":null,"startArrowhead":null,"endArrowhead":"arrow"}, + {"type":"text","id":"spec_title","x":600,"y":295,"width":500,"height":28,"text":"ENFORCEMENT SPECTRUM","originalText":"ENFORCEMENT SPECTRUM","fontSize":20,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#1e40af","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2001,"version":1,"versionNonce":2002,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"spec_sub","x":550,"y":328,"width":600,"height":20,"text":"rules promote down the spectrum as they prove value →","originalText":"rules promote down the spectrum as they prove value →","fontSize":13,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#64748b","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2003,"version":1,"versionNonce":2004,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"rectangle","id":"t1","x":80,"y":370,"width":480,"height":200,"strokeColor":"#1e3a5f","backgroundColor":"#dbeafe","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2010,"version":1,"versionNonce":2011,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"arr_t1_t2","type":"arrow"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"t1_label","x":100,"y":390,"width":440,"height":20,"text":"TIER 1","originalText":"TIER 1","fontSize":14,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#64748b","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2012,"version":1,"versionNonce":2013,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"t1_name","x":100,"y":430,"width":440,"height":40,"text":"CONVENTION","originalText":"CONVENTION","fontSize":28,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#1e40af","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2014,"version":1,"versionNonce":2015,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"t1_tag","x":100,"y":495,"width":440,"height":50,"text":"Relies on noticing.\nNoticing degrades first.","originalText":"Relies on noticing.\nNoticing degrades first.","fontSize":16,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#374151","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2016,"version":1,"versionNonce":2017,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.3}, + {"type":"rectangle","id":"t2","x":600,"y":370,"width":480,"height":200,"strokeColor":"#1e3a5f","backgroundColor":"#93c5fd","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2020,"version":1,"versionNonce":2021,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"arr_t1_t2","type":"arrow"},{"id":"arr_t2_t3","type":"arrow"},{"id":"arr_spec_merge","type":"arrow"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"t2_label","x":620,"y":390,"width":440,"height":20,"text":"TIER 2","originalText":"TIER 2","fontSize":14,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#1e40af","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2022,"version":1,"versionNonce":2023,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"t2_name","x":620,"y":430,"width":440,"height":40,"text":"PR-REVIEW","originalText":"PR-REVIEW","fontSize":28,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#1e40af","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2024,"version":1,"versionNonce":2025,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"t2_tag","x":620,"y":495,"width":440,"height":50,"text":"Fires automatically.\nReviewer can dismiss.","originalText":"Fires automatically.\nReviewer can dismiss.","fontSize":16,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#1e40af","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2026,"version":1,"versionNonce":2027,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.3}, + {"type":"rectangle","id":"t3","x":1120,"y":370,"width":480,"height":200,"strokeColor":"#1e3a5f","backgroundColor":"#3b82f6","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2030,"version":1,"versionNonce":2031,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"arr_t2_t3","type":"arrow"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"t3_label","x":1140,"y":390,"width":440,"height":20,"text":"TIER 3","originalText":"TIER 3","fontSize":14,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#ffffff","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2032,"version":1,"versionNonce":2033,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"t3_name","x":1140,"y":430,"width":440,"height":40,"text":"MECHANICAL","originalText":"MECHANICAL","fontSize":28,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#ffffff","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2034,"version":1,"versionNonce":2035,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"t3_tag","x":1140,"y":495,"width":440,"height":50,"text":"Fails loudly.\nNo human has to notice.","originalText":"Fails loudly.\nNo human has to notice.","fontSize":16,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#ffffff","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2036,"version":1,"versionNonce":2037,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.3}, + {"type":"arrow","id":"arr_t1_t2","x":562,"y":470,"width":36,"height":0,"strokeColor":"#1e3a5f","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2040,"version":1,"versionNonce":2041,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"points":[[0,0],[36,0]],"startBinding":{"elementId":"t1","focus":0,"gap":2},"endBinding":{"elementId":"t2","focus":0,"gap":2},"startArrowhead":null,"endArrowhead":"arrow"}, + {"type":"arrow","id":"arr_t2_t3","x":1082,"y":470,"width":36,"height":0,"strokeColor":"#1e3a5f","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":2042,"version":1,"versionNonce":2043,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"points":[[0,0],[36,0]],"startBinding":{"elementId":"t2","focus":0,"gap":2},"endBinding":{"elementId":"t3","focus":0,"gap":2},"startArrowhead":null,"endArrowhead":"arrow"}, + {"type":"arrow","id":"arr_spec_merge","x":850,"y":572,"width":0,"height":48,"strokeColor":"#1e3a5f","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":3001,"version":1,"versionNonce":3002,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"points":[[0,0],[0,48]],"startBinding":{"elementId":"t2","focus":0,"gap":2},"endBinding":{"elementId":"merge","focus":0,"gap":2},"startArrowhead":null,"endArrowhead":"arrow"}, + {"type":"rectangle","id":"merge","x":700,"y":620,"width":300,"height":60,"strokeColor":"#047857","backgroundColor":"#a7f3d0","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":3010,"version":1,"versionNonce":3011,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"arr_spec_merge","type":"arrow"},{"id":"arr_merge_enc","type":"arrow"},{"id":"merge_text","type":"text"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"merge_text","x":720,"y":637,"width":260,"height":26,"text":"PR + CI → MERGE","originalText":"PR + CI → MERGE","fontSize":18,"fontFamily":3,"textAlign":"center","verticalAlign":"middle","strokeColor":"#374151","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":3012,"version":1,"versionNonce":3013,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":"merge","lineHeight":1.25}, + {"type":"arrow","id":"arr_merge_enc","x":850,"y":682,"width":0,"height":38,"strokeColor":"#1e3a5f","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":2,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4001,"version":1,"versionNonce":4002,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"points":[[0,0],[0,38]],"startBinding":{"elementId":"merge","focus":0,"gap":2},"endBinding":null,"startArrowhead":null,"endArrowhead":"arrow"}, + {"type":"text","id":"enc_title","x":400,"y":740,"width":900,"height":28,"text":"CONTINUOUS RULE ENCODING — five surfaces","originalText":"CONTINUOUS RULE ENCODING — five surfaces","fontSize":20,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#1e40af","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4010,"version":1,"versionNonce":4011,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"text","id":"enc_sub","x":300,"y":775,"width":1100,"height":20,"text":"default to the smallest surface · keep CLAUDE.md lean","originalText":"default to the smallest surface · keep CLAUDE.md lean","fontSize":13,"fontFamily":3,"textAlign":"center","verticalAlign":"top","strokeColor":"#64748b","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4012,"version":1,"versionNonce":4013,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25}, + {"type":"rectangle","id":"s1","x":80,"y":820,"width":290,"height":70,"strokeColor":"#1e3a5f","backgroundColor":"#dbeafe","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4020,"version":1,"versionNonce":4021,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"s1_text","type":"text"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"s1_text","x":100,"y":835,"width":250,"height":40,"text":"CLAUDE.md\n(kept lean)","originalText":"CLAUDE.md\n(kept lean)","fontSize":15,"fontFamily":3,"textAlign":"center","verticalAlign":"middle","strokeColor":"#374151","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4022,"version":1,"versionNonce":4023,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":"s1","lineHeight":1.3}, + {"type":"rectangle","id":"s2","x":390,"y":820,"width":290,"height":70,"strokeColor":"#1e3a5f","backgroundColor":"#dbeafe","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4030,"version":1,"versionNonce":4031,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"s2_text","type":"text"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"s2_text","x":410,"y":835,"width":250,"height":40,"text":".coderabbit.yaml\npath_instructions","originalText":".coderabbit.yaml\npath_instructions","fontSize":15,"fontFamily":3,"textAlign":"center","verticalAlign":"middle","strokeColor":"#374151","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4032,"version":1,"versionNonce":4033,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":"s2","lineHeight":1.3}, + {"type":"rectangle","id":"s3","x":700,"y":820,"width":290,"height":70,"strokeColor":"#1e3a5f","backgroundColor":"#dbeafe","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4040,"version":1,"versionNonce":4041,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"s3_text","type":"text"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"s3_text","x":720,"y":835,"width":250,"height":40,"text":"Architecture-reviewer\nagent checklist","originalText":"Architecture-reviewer\nagent checklist","fontSize":15,"fontFamily":3,"textAlign":"center","verticalAlign":"middle","strokeColor":"#374151","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4042,"version":1,"versionNonce":4043,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":"s3","lineHeight":1.3}, + {"type":"rectangle","id":"s4","x":1010,"y":820,"width":290,"height":70,"strokeColor":"#1e3a5f","backgroundColor":"#dbeafe","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4050,"version":1,"versionNonce":4051,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"s4_text","type":"text"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"s4_text","x":1030,"y":835,"width":250,"height":40,"text":"Skills + commands\n(.claude/)","originalText":"Skills + commands\n(.claude/)","fontSize":15,"fontFamily":3,"textAlign":"center","verticalAlign":"middle","strokeColor":"#374151","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4052,"version":1,"versionNonce":4053,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":"s4","lineHeight":1.3}, + {"type":"rectangle","id":"s5","x":1320,"y":820,"width":290,"height":70,"strokeColor":"#c2410c","backgroundColor":"#fed7aa","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4060,"version":1,"versionNonce":4061,"isDeleted":false,"groupIds":[],"boundElements":[{"id":"s5_text","type":"text"},{"id":"loopback","type":"arrow"}],"link":null,"locked":false,"roundness":{"type":3}}, + {"type":"text","id":"s5_text","x":1340,"y":835,"width":250,"height":40,"text":"Docs + paired diagrams\n(the picture)","originalText":"Docs + paired diagrams\n(the picture)","fontSize":15,"fontFamily":3,"textAlign":"center","verticalAlign":"middle","strokeColor":"#374151","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":4062,"version":1,"versionNonce":4063,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":"s5","lineHeight":1.3}, + {"type":"arrow","id":"loopback","x":1610,"y":855,"width":750,"height":685,"strokeColor":"#c2410c","backgroundColor":"transparent","fillStyle":"solid","strokeWidth":3,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":5001,"version":1,"versionNonce":5002,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"points":[[0,0],[60,0],[60,-685],[-650,-685]],"startBinding":{"elementId":"s5","focus":0,"gap":2},"endBinding":{"elementId":"auth","focus":0,"gap":2},"startArrowhead":null,"endArrowhead":"arrow"}, + {"type":"text","id":"loopback_label","x":1620,"y":480,"width":160,"height":22,"text":"next session is smarter","originalText":"next session is smarter","fontSize":14,"fontFamily":3,"textAlign":"center","verticalAlign":"middle","strokeColor":"#c2410c","backgroundColor":"#ffffff","fillStyle":"solid","strokeWidth":1,"strokeStyle":"solid","roughness":0,"opacity":100,"angle":0,"seed":5003,"version":1,"versionNonce":5004,"isDeleted":false,"groupIds":[],"boundElements":null,"link":null,"locked":false,"containerId":null,"lineHeight":1.25} + ], + "appState": { + "viewBackgroundColor": "#ffffff", + "gridSize": 20 + }, + "files": {} +} diff --git a/docs/dev-loop-scaffolding.svg b/docs/dev-loop-scaffolding.svg new file mode 100644 index 00000000..49537790 --- /dev/null +++ b/docs/dev-loop-scaffolding.svg @@ -0,0 +1,2 @@ +Continuous Rule Encoding LoopHow AI-assisted rules compound over timeAUTHORINGENFORCEMENT SPECTRUMrules promote down the spectrum as they prove value →TIER 1CONVENTIONRelies on noticing.Noticing degrades first.TIER 2PR-REVIEWFires automatically.Reviewer can dismiss.TIER 3MECHANICALFails loudly.No human has to notice.PR + CI → MERGECONTINUOUS RULE ENCODING — five surfacesdefault to the smallest surface · keep CLAUDE.md leanCLAUDE.md(kept lean).coderabbit.yamlpath_instructionsArchitecture-revieweragent checklistSkills + commands(.claude/)Docs + paired diagrams(the picture)next session is smarter \ No newline at end of file diff --git a/docs/dev-loop.md b/docs/dev-loop.md index bc8ad1f0..7e745d9c 100644 --- a/docs/dev-loop.md +++ b/docs/dev-loop.md @@ -41,18 +41,26 @@ Per [CLAUDE.md "Continuous Rule Encoding"](../CLAUDE.md#continuous-rule-encoding when *any* 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 the **six surfaces** below as apply. +across as many of the **five surfaces** below as apply. Default to the +**smallest set** — and the *smallest entry on CLAUDE.md*, because every +byte there is loaded into every session. -### The six surfaces (canonical list) +### The five surfaces (canonical list) | # | Surface | What goes here | Catches violations at... | |---|---|---|---| -| 1 | [`CLAUDE.md`](../CLAUDE.md) | Canonical hard/soft rule with reference templates and the *why* in one line | Code-generation time (Claude reads CLAUDE.md every session) | -| 2 | [`.coderabbit.yaml`](../.coderabbit.yaml) `path_instructions` | File-pattern-scoped guidance — add a new glob if no existing one fits | PR-time CodeRabbit review | +| 1 | [`CLAUDE.md`](../CLAUDE.md) | **Always-on** rules only. Prefer one-line headline + link to the deeper doc; full paragraphs are usually a sign the rule belongs in surface 5 with a pointer here. | Code-generation time (Claude reads CLAUDE.md every session) | +| 2 | [`.coderabbit.yaml`](../.coderabbit.yaml) `path_instructions` | File-pattern-scoped guidance — add a new glob if no existing one fits. **Most per-file rules belong here, not in CLAUDE.md.** | PR-time CodeRabbit review | | 3 | [`.claude/agents/architecture-reviewer.md`](../.claude/agents/architecture-reviewer.md) Pattern Checklist | Per-file-category scan rule the agent applies on every review | Local agent invocation, before code lands | -| 4 | [`.claude/skills/`](../.claude/skills/) | Procedural knowledge worth a dedicated bundle (multi-step, specialized vocab) | On-demand, or when the user describes the right intent | -| 5 | [GitHub Issues](https://github.com/emeraldleaf/NextAurora/issues) (labels: `rule-encoding-deferred`, `type/*`, `area/*`) | Deferred or partial fixes — the durable work ledger. STATUS.md is the entry-point doc; the issues board is where individual work items live. | Cross-session pickup, board triage | -| 6 | 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 | +| 4 | [`.claude/skills/`](../.claude/skills/) + [`.claude/commands/`](../.claude/commands/) | Procedural knowledge worth a dedicated bundle (multi-step, specialized vocab) | On-demand, or when the user describes the right intent | +| 5 | Supporting docs + paired diagrams ([architecture.md](architecture.md), [performance-and-data-correctness.md](performance-and-data-correctness.md), this file, and `docs/*.{svg,excalidraw}` pairs) | The *why* behind a rule, the visual depiction reviewers reason against | Onboarding, PR review, future-you | + +**Deferral surface (NOT part of the loop):** [GitHub Issues](https://github.com/emeraldleaf/NextAurora/issues) +with the `rule-encoding-deferred` label tracks findings where code shipped +but the encoding hasn't yet. The issue is a placeholder — a TODO that the +encoding eventually happens. **It is not itself the encoding** — closed +issues aren't read by future sessions. The rule lives in surfaces 1–5; the +issue exists only until it does. ### What triggers an encoding (the threshold) @@ -295,7 +303,7 @@ suggestions. Every PR-review pass checks for them. ### `.coderabbit.yaml` `path_instructions` — surface #2 of the encoding loop -`.coderabbit.yaml` is one of the six surfaces from "The reflexive step" — it's +`.coderabbit.yaml` is one of the five surfaces from "The reflexive step" — it's how a rule encoded once in CLAUDE.md gets re-checked at PR-review time without re-deriving it. Today's glob set (~12 entries) covers: diff --git a/docs/observability-and-context-propagation.md b/docs/observability-and-context-propagation.md new file mode 100644 index 00000000..3a1d6fd3 --- /dev/null +++ b/docs/observability-and-context-propagation.md @@ -0,0 +1,96 @@ +# Observability & Context Propagation + +Deep-dive companion to [CLAUDE.md "Observability & Context Propagation"](../CLAUDE.md#observability--context-propagation), which keeps the always-on traps and points here for mechanism + wiring detail. + +NextAurora propagates three context identifiers across HTTP and Service Bus +boundaries so every log line, span, and event in a request's lifecycle can +be correlated: + +| Concept | Activity Baggage Key | HTTP / SB Property | Logger Scope Key | +|---|---|---|---| +| Correlation | `correlation.id` | `X-Correlation-Id` | `CorrelationId` | +| User | `user.id` | `X-User-Id` | `UserId` | +| Session | `session.id` | `X-Session-Id` | `SessionId` | + +**Sources:** + +- `correlation.id` — from the `X-Correlation-Id` request header, or generated from the trace ID when absent. +- `user.id` — from the `ClaimTypes.NameIdentifier` JWT claim (`sub`); null when unauthenticated. +- `session.id` — from the `X-Session-Id` request header (client-generated browser/app session UUID); null if not provided. + +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. + +## Wolverine pipeline scope + +`ContextPropagationMiddleware` opens a `logger.BeginScope()` before invoking each handler so **every log line emitted anywhere in the handler** carries `CorrelationId`, `UserId`, and `SessionId` automatically. Wolverine's `Policies.LogMessageStarting()` adds handler name + elapsed time on top of that. + +Order in the Wolverine pipeline: + +1. FluentValidation policy (`opts.UseFluentValidation()`) — rejects invalid commands before handlers run +2. `ContextPropagationMiddleware` — opens logger scope +3. Handler +4. `opts.Policies.AutoApplyTransactions()` — wraps each EF-touching handler chain so outgoing messages are persisted to the outbox in the same DB transaction as the entity write + +## Wolverine envelope context extraction + +Handlers don't extract context manually — `ContextPropagationMiddleware` does it for them. The middleware reads `Envelope.Headers["X-Correlation-Id" | "X-User-Id" | "X-Session-Id"]` (Wolverine's transport-agnostic header bag, mapped to Service Bus `ApplicationProperties` over the wire), restores them into Activity baggage, and opens a `logger.BeginScope()`. After the handler runs, `Finally()` disposes the scope. + +Outgoing context is stamped by `OutgoingContextMiddleware`, which reads Activity baggage and writes the same headers onto outgoing envelopes. The full mechanism is registered via `opts.AddNextAuroraContextPropagation()` in each service's `Program.cs`. + +**Structured logging scope hygiene:** never add null/empty keys to logging scope dictionaries — use `if (x is not null) scope["Key"] = x`. Always pass `StringComparer.Ordinal` when constructing `Dictionary` (per Meziantou MA0002). + +## Transactional Outbox (Wolverine) + +Each event-publishing service (Order, Payment, Shipping) runs Wolverine's transactional outbox. Outgoing events are persisted to a `wolverine.*` schema in the same DB transaction as the entity write, then dispatched to Azure Service Bus by Wolverine's background flush. Configuration lives in each service's `Program.cs`: + +```csharp +opts.PersistMessagesWithSqlServer(connectionString, "wolverine"); // or PersistMessagesWithPostgresql +opts.UseEntityFrameworkCoreTransactions(); +opts.Policies.AutoApplyTransactions(); +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. Full rationale + failure modes: [docs/performance-and-data-correctness.md](performance-and-data-correctness.md). + +### 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: [PaymentRecoveryJob](../PaymentService/Infrastructure/PaymentRecoveryJob.cs) — the canonical inline implementation of this wrapper (the previous `IPaymentRepository.ExecuteInTransactionAsync` wrapper was deleted in the simplicity refactor; the pattern is unchanged, just inlined). 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. diff --git a/docs/performance-and-data-correctness.md b/docs/performance-and-data-correctness.md index e8f5e497..5b07df64 100644 --- a/docs/performance-and-data-correctness.md +++ b/docs/performance-and-data-correctness.md @@ -147,6 +147,105 @@ See [decision: optimistic concurrency tokens](#decision-optimistic-concurrency-t --- +## Additional always-on patterns + +These extend the 14 rules above with patterns that don't fit cleanly into a single rule shape. Same enforcement bar. + +### Non-sargable predicates defeat indexes — fix at write time + +A `Where(...)` that wraps the column in a function (`u.Email.ToLower() == x`, `o.CreatedAt.Date == today`) can't use a B-tree index on that column even if one exists — the planner falls back to a full scan. + +**The fix is at write time, not at read time:** +- Normalize on insert/update (e.g. an `EmailNormalized` column populated by the aggregate factory + projected to in `Where(u => u.EmailNormalized == emailNormalized)`) +- Or use a case-insensitive collation at the column level + +**Leading-wildcard substring search (`LIKE '%text%'`, `EF.Functions.ILike(p.Name, "%text%")`) isn't B-tree-indexable in any database** — escalate to Postgres `tsvector` full-text search or a dedicated search engine (Elasticsearch / OpenSearch / Meilisearch) when load justifies it. Reference: [`CatalogService/Features/SearchProducts.cs`](../CatalogService/Features/SearchProducts.cs) documents the leading-wildcard trade-off explicitly (intentional; full-text is the named next step if it becomes a bottleneck). + +**Deeper principle:** indexes carry a write cost — every insert/update touches every index on the table — so an index the planner can't use is pure overhead, not free defense-in-depth. Adding more indexes isn't a universal speed-up; treat the index list like an interface — each one earns its keep against a real query. + +### Parallelize independent awaits with `Task.WhenAll` + +Sequential `await`s serialize latency for free. Async makes a single wait non-blocking; it does not make a *sequence* of waits cheap. When a handler makes N independent I/O calls — N gRPC requests to different services, N HTTP calls to different external APIs, N queries against *different* DbContexts (one per service) — sequential `await`s pay the sum of all latencies, while `Task.WhenAll` pays the max. + +**Anti-shape:** +```csharp +var user = await GetUserAsync(id, ct); +var orders = await GetOrdersAsync(otherId, ct); // doesn't depend on user +var notifications = await GetNotificationsAsync(ct); // doesn't depend on either +``` + +**Right shape:** kick all three off, await once, project the results. + +**Reference shape:** [`OrderService/Features/PlaceOrder.cs:93`](../OrderService/Features/PlaceOrder.cs) — `Task.WhenAll(request.Lines.Select(line => catalogClient.GetProductAsync(line.ProductId, ct)))` is the canonical gRPC fan-out over independent line items, parallelism over the wire, no shared mutable state. The file documents the DbContext safety caveat at lines 89–92 explicitly. + +**Don't parallelize:** +- (a) **Dependent operations** where the output of one feeds the input of another (`var user = await ...; var orders = await GetByUserId(user.Id, ct);`) +- (b) **Operations sharing the same EF `DbContext` scope** — DbContext is NOT thread-safe and parallel EF queries against the same context throw or corrupt state. Use `IDbContextFactory` to mint one context per task (see rule 8 above) +- (c) **Operations whose failures must be observed independently** — `Task.WhenAll` surfaces only the first exception; the rest run to completion but get swallowed. Use `Task.WhenAll(...)` followed by inspection of each task's `.Exception` (or `Task.WhenEach` in .NET 10) when multi-failure surfacing matters + +### Long-running work belongs on the message bus, not the synchronous HTTP handler + +If a write path would take more than ~1s (multi-step external API chain, aggregation over thousands of rows, bulk import, report generation), reshape the endpoint as **202 Accepted**: validate + persist a tracking row + publish a Wolverine message + return `202` with the polling key in the body and a `Location` header pointing to a status endpoint. A background handler does the actual work; the client polls (`GET /jobs/{id}`, `GET /orders/{id}`, etc.) or receives a push (SignalR / SSE / email when the job completes). + +**What counts as "the tracking row":** the aggregate being created can BE the tracking row when its ID is the polling key — that's the `POST /api/v1/orders` shape (the `Order` row IS the tracking record; `GET /api/v1/orders/{id}` IS the polling endpoint). A separate jobs table is only needed when there's no natural aggregate (bulk import of CSV with no per-row entity, report generation with no persistent output). + +**The synchronous parts commit atomically two ways:** +- (a) when the endpoint dispatches via `bus.InvokeAsync(command, ct)`, `AutoApplyTransactions` wraps the handler — `SaveChanges` flushes the entity write and Wolverine's staged envelope in one DB transaction +- (b) when the endpoint persists + publishes inline (no handler dispatch), use the `BeginTransactionAsync` → work + `PublishAsync` → `SaveChangesAsync` → `CommitAsync` wrap from the Outbox-outside-handler trap (see [observability-and-context-propagation.md "Outbox outside a Wolverine handler"](observability-and-context-propagation.md#outbox-outside-a-wolverine-handler--atomicity-trap)). Skipping `SaveChangesAsync` after `PublishAsync` and before `Commit` silently drops the staged envelope + +**Why it matters:** the HTTP request holds a thread, a DB connection, and a concurrency-budget slot for the full duration of the handler — a small spike on a slow endpoint can starve the rest of the API. Response time and work duration are different things; the rule is to keep response time bounded. + +NextAurora already has the full machinery — Wolverine + Service Bus + outbox + saga handlers — so the rule is "use it when a handler would otherwise block." The current `POST /api/v1/orders` is the canonical reference: the handler validates + persists the `Order` + stages `OrderPlacedEvent` + returns `OrderId`, then PaymentService + ShippingService handle downstream work async via the saga. Note that `bus.InvokeAsync` awaits the *Place Order handler* synchronously — what makes this the right shape isn't the response code, it's that the handler only does validate-persist-stage and stays sub-second; the minutes-scale work is downstream consumers of the staged event. + +**Same rule for Wolverine handlers themselves**: a handler body that runs for minutes is the same anti-pattern with a different colored connection — break the work into a follow-up message handler. + +**Cloud-managed alternatives** when the worker pool needs scale-to-zero or a multi-step durable workflow: +- AWS SQS + Lambda or Azure Service Bus + Azure Functions for stateless workers +- Azure Durable Functions or AWS Step Functions for multi-step orchestration with timers/retries +- Temporal for hours-to-days workflows with first-class durable execution + +Trade-off is the usual one — less ops, more vendor coupling. + +### Fan-out belongs on the message bus, not in a synchronous handler loop + +A handler that iterates a recipient list inline (`foreach (var follower in followers) await _sender.SendAsync(...)`) holds the request open for N × per-recipient-latency, concentrates the work on one process, and creates traffic spikes that can starve the rest of the system (millions of follower notifications fired by one celebrity post). + +**The right shape:** publish **one Wolverine message per recipient** (or per batch of K recipients) and return immediately; per-recipient handlers run in parallel under Wolverine's `MaxDegreeOfParallelism` throttle, set per-handler in `Program.cs`: + +```csharp +opts.LocalQueueFor().MaxDegreeOfParallelism(N); +``` + +The throttle gives natural back-pressure — fast producers can't starve slow consumers, and a notification spike doesn't pin a thread or saturate the downstream provider. This is the same principle as "Long-running work belongs on the message bus" applied to fan-out specifically: **accept the work, don't do the work.** + +Not retroactively violated today (NotificationService receives one inbound event = one outbound notification), but the rule is preventative for any future broadcast-to-N feature (multi-tenant announcements, post-with-followers, abandoned-cart drips, etc.). + +### Entity IDs use `Guid.CreateVersion7()`, not `Guid.NewGuid()` + +UUID v7 (first 48 bits = Unix-ms timestamp, remaining 74 bits random) is **time-ordered**, so PK inserts append-extend the B-tree index instead of splitting pages everywhere — kills the index-fragmentation tax that random UUID v4 inserts pay on every write. .NET 9+ API, no third-party package needed, drop-in same `Guid` type. + +Apply in aggregate factory methods (`Order.Create`, `Payment.Create`, etc.) — the canonical spot to mint domain IDs. + +**Trade-off:** v7's timestamp is decodable from the ID, so the mint time leaks to anyone holding it. Fine for: +- Buyer-scoped resources (IDOR check gates visibility — non-owners can't see the ID at all) +- Naturally-public timestamped resources (Product creation time isn't sensitive; often returned in the response anyway) + +**Don't use v7** for IDs where the mint time IS sensitive (security tokens, admin-only internal references). + +Existing v4 IDs in the DB stay as-is — v4 and v7 coexist in the same `Guid` column with no migration required; the rule applies to *new* IDs. + +### `AsSpan` over `Substring` for zero-allocation slicing — narrow tool + +`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. 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" (rule 14) — apply 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. + +--- + ## Decision: optimistic concurrency tokens ### Problem