Chore/claude encode session lessons#27
Conversation
The 8-file architecture-reviewer agent pass on 2026-05-24 surfaced 12 must-fix items + 17 should-considers across security, EF Core, async, distributed systems, and observability. The fixes shipped as PRs; this commit captures the patterns so future code generation prevents the same bugs rather than re-discovering them in review. CLAUDE.md — 4 sections expanded: 1. "Security Requirements" — concrete IDOR-prevention pattern (was just "validate buyerId matches authenticated user"; now spells out the endpoint→query→handler→null→404 shape with reference templates + the 403-vs-404 reasoning). Adds explicit JWT defaults (ValidateIssuerSigningKey=true, ClockSkew=30s — not the dangerous 5-minute default). Adds the trace-ID-exposure rule (use Activity.TraceId.ToString, not Activity.Id which leaks the span). Also explicitly cross-references the integration-test requirement. 2. "Transactional Outbox (Wolverine)" — adds the outbox-outside-handler atomicity trap that caused the PaymentRecoveryJob silent event drop. Wolverine's AutoApplyTransactions only wraps handler chains; code running outside (BackgroundService, recovery sweepers) needs an explicit SaveChangesAsync between PublishAsync and Commit, or Wolverine's staged envelope never reaches the outbox table. Includes the canonical safe wrapper as runnable code. 3. "Observability — HTTP middleware order" — strict ordering rule (UseExceptionHandler → UseAuthentication → CorrelationIdMiddleware → UseAuthorization). The first ServiceDefaults bug was middleware reading context.User before UseAuthentication populated it (UserId silently always null); the over-correction placed it after UseAuthorization which dropped UserId from 401/403 audit logs. Documents both failure modes + the correct middle ground. 4. "Testing" — IDOR test is now REQUIRED for every scoped-entity endpoint. Authorization behavior is only proven by an authorization-failure test; dotnet build + unit tests passing isn't sufficient. Also adds outbox-in-non-handler test requirement for sweepers/jobs. .coderabbit.yaml — 3 new path_instructions blocks: - **/Endpoints/**/*.cs gets explicit IDOR-prevention guidance with the canonical 4-step pattern + mass-assignment check + endpoint hygiene reminders. - **/*RecoveryJob*.cs gets outbox-outside-handler trap guidance + per-iteration scope rule + distributed-lock rule. - NextAurora.ServiceDefaults/**/*.cs gets the middleware-order rule, JWT explicit-defaults rule, trace-ID-exposure rule, and a hint that changes here should bump unit-test coverage (because the project has historically been undertested and codecov/patch will complain). architecture-reviewer agent — new "Pattern checklist" section with specific per-file-category scan rules. Six categories covered: Endpoints (IDOR + mass-assignment), RecoveryJob (outbox + scope + lock), ServiceDefaults (middleware + JWT + trace-ID), query handlers (AsNoTracking + pagination + N+1), aggregates (rich domain entity + collection encapsulation + concurrency token), workflows (bash hygiene + persist-credentials + permissions). The agent now scans for these patterns explicitly instead of re-deriving from first principles on every review. Issues doc (/tmp/nextaurora-issues-to-create.md) — 8 new backlog entries from this session's findings, sized + scoped with acceptance criteria. Includes the IDOR-test backfill, ServiceDefaults integration tests, PaymentRecoveryJob outbox integration test, per-iteration DI scope refactor, CORS helper, security headers, PlaceOrder defense-in- depth, and HTTPS-redirect consolidation. Paraphrase sweep: zero stale paraphrases. The new rules are NEW additions, not modifications of existing wording, so none of the existing `See CLAUDE.md` markers in code or docs need updating. The in-flight fix PRs (IDOR, outbox, middleware) will add new markers in their respective inline comments when they merge — that's PR-scope, not lessons-learned-PR-scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rule
The existing "Debugging Discipline" rule mandates that lessons from
debugging sessions get encoded in CLAUDE.md + supporting docs before
moving on. This commit generalizes that same discipline to ALL review
surfaces — architecture-reviewer agent passes, CodeRabbit findings,
manual code review, integration-test failures, prod incidents, security
audits.
CLAUDE.md: new "Continuous Rule Encoding (the compounding loop)" section
adjacent to Debugging Discipline. It explicitly mandates that when an
anti-pattern / rule / spec is identified worth keeping, it gets written
to ALL applicable artifacts in the same session:
1. CLAUDE.md — the canonical rule
2. .coderabbit.yaml path_instructions — PR-time detection
3. architecture-reviewer agent Pattern Checklist — review-time detection
4. .claude/skills/ — if the procedure is non-trivial
5. docs/STATUS.md Open Issues — if deferred
6. Supporting docs (architecture.md, performance-and-data-correctness.md,
dev-loop.md) — when the *why* needs more than a one-liner
And it makes the threshold explicit: encode if a future author could
repeat the mistake OR re-derive the rule from first principles. Don't
encode trivial style nits; do encode security/perf/concurrency/
distributed-systems patterns. The closing line is the load-bearing
commitment: "A merged fix PR without the corresponding rule is a
half-finished job."
architecture-reviewer agent: added Step 7 to "How to work" — when the
agent identifies a pattern worth encoding, it must propose WHERE
(CLAUDE.md section / .coderabbit.yaml path / Pattern Checklist
category) and the proposed wording. Added a "Rules to encode" optional
section to the output format so the suggestions have a place to land
in the report rather than getting buried.
The point of these two changes together: the compounding loop becomes
mechanical rather than aspirational. Every review-surface finding now
has a clear path from "discovered" to "encoded so we don't discover
it again" — and the agent itself prompts for the encoding so we don't
forget.
Paraphrase sweep: clean. New section is purely additive; no existing
rule wording changed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…in dev-loop md/svg
Round-trips the "Continuous Rule Encoding" meta-rule (introduced in the
preceding commit) into the surfaces a human or AI assistant would actually
read when working in this repo:
.claude/README.md (new): the missing "how to use this directory" guide.
- Directory layout
- Decision tree for picking between agents / commands / skills
- Per-surface deep-dive: what each is, when to use, how to invoke,
how to author, current inventory in this repo
- Hooks + architecture-map sections (briefly; pointers to source)
- "When to add a new extension" table that mirrors the Continuous Rule
Encoding decision matrix from CLAUDE.md
docs/dev-loop.md: adds "Continuous Rule Encoding (the compounding feedback
loop)" section between the 5 stages and the Gaps list. Includes the
encoding-surface table (CLAUDE.md / .coderabbit.yaml / agent / skill /
status / docs) + a tiny ASCII diagram of the loop + an explicit pointer
to .claude/README.md for invocation details. Names the load-bearing rule:
"a merged fix PR without the corresponding .claude/ encoding is a
half-finished job."
docs/dev-loop.svg: two changes.
1. Iteration-loop arrow label updated from "findings → fix → push
→ re-review" to "findings → fix → encode rule in .claude/
→ push → re-review". The encoding step is now visible at a
glance, not buried in prose.
2. New "Continuous Rule Encoding" section at the bottom (180px tall,
purple/AI-LLM color matching the Stage 1 Edit-time block). Shows the
Finding → Encode-in-four-places → Next-cycle-prevented flow
inline. Cross-references .claude/README.md for invocation patterns.
3. SVG viewBox + width + height bumped from 1750 to 1940 to accommodate
the new section. Background rect resized to match.
Why all three: a human reading dev-loop.md gets the prose version. A human
viewing dev-loop.svg gets the visual. A human (or AI) opening the .claude/
directory gets the README explaining what each file in there is for.
Three surfaces, same message, picked up at whichever angle the reader
approaches from.
The architecture-reviewer agent already has Step 7 (suggest encodings)
from the previous commit. Together with this commit, the encoding loop is
now mechanical rather than aspirational at every surface.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughThis PR establishes a continuous feedback loop for encoding architectural patterns across documentation (CLAUDE.md), code review configuration (.coderabbit.yaml), agent prompts, and developer workflow docs. It introduces security patterns for IDOR prevention, explicit JWT validation, HTTP middleware ordering, and outbox atomicity, plus prescriptive checklists in the architecture-reviewer agent and rule-encoding guidance in CodeRabbit. ChangesArchitectural Rule Encoding Infrastructure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 31-46: CLAUDE.md changed security rules (owner-scoped
authorization pattern, explicit TokenValidationParameters, traceId usage,
validators, etc.), so run the local audit command (/check-rules) and update
every paraphrase that ends with "See CLAUDE.md" to reflect the new specifics:
ensure references to the owner-check pattern mention ClaimTypes.NameIdentifier →
RequestingBuyerId flow and the handler-return-null → endpoint-404 behavior (see
OrderEndpoints.cs/ShippingEndpoints.cs/CatalogEndpoints.cs examples), require
FluentValidation for commands, update AddDefaultAuthentication /
TokenValidationParameters to set ValidateIssuerSigningKey=true,
ValidateAudience/ValidateIssuer/ValidateLifetime=true and
ClockSkew=TimeSpan.FromSeconds(30), and change guidance to use
Activity.TraceId.ToString() (not Activity.Id) per GlobalExceptionHandler.cs;
search the repo for "See CLAUDE.md" and repair each paraphrase to match these
exact requirements.
- Around line 283-296: The fenced C# snippet for ExecuteInTransactionAsync is
missing blank lines before and after the triple-backtick fences; add a blank
line immediately above the opening ```csharp and a blank line immediately below
the closing ``` so the fenced code block is separated from surrounding
paragraphs (adjust the block containing ExecuteInTransactionAsync accordingly to
satisfy MD031).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2f67a7f-7ec4-48c3-bdd1-0b8e33cb2041
⛔ Files ignored due to path filters (1)
docs/dev-loop.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
.claude/README.md.claude/agents/architecture-reviewer.md.coderabbit.yamlCLAUDE.mddocs/dev-loop.md
What changed
How it was built
Verification
Touches (check only if applies)
See CLAUDE.mdparaphrase (run/check-rulesto audit drift)Summary by CodeRabbit