Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions CONTENT-GAP-AUDIT.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Content Gap Audit (Design-Patterns Repo)

## Critical
- Added missing **Transactional Outbox** system pattern and **Dual Writes** anti-pattern coverage.
- Added migration path **Direct Publish -> Transactional Outbox** to reduce data/event inconsistency risk.

## High
- Added **Event Sourcing** as a common system pattern not previously covered.
- Added **N+1 Query Problem** anti-pattern because it is a high-frequency production issue.

## Medium
- Added **Version Baseline** section in framework catalog to make version intent explicit and reviewable.
- Verified all references listed in `skills/design-patterns/SKILL.md` exist in repository.

## Low
- Ran a markdown link/path scan (no actionable broken local links detected).
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use “Markdown” capitalization for consistency.

Minor docs polish: “markdown” should be “Markdown”.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~16-~16: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...d` exist in repository. ## Low - Ran a markdown link/path scan (no actionable broken lo...

(MARKDOWN_NNP)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CONTENT-GAP-AUDIT.md` at line 16, Update the capitalization of the word
"markdown" to "Markdown" in CONTENT-GAP-AUDIT.md where the sentence currently
reads "Ran a markdown link/path scan (no actionable broken local links
detected)."—locate the phrase and change it to "Ran a Markdown link/path scan
(no actionable broken local links detected)." to maintain consistent use of
"Markdown".

- Minor wording cleanup happened indirectly via added sections.
8 changes: 4 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ Step 4: Thin routes.ts to HTTP-only controller
| Capability | Reference |
|------------|----------|
| 🧪 **Testing Patterns** — How to test every pattern | `references/testing-patterns.md` |
| 🚫 **Anti-Patterns Catalog** — 15 code smells with detection & cures | `references/anti-patterns.md` |
| 🔄 **Migration Playbook** — 5 step-by-step refactoring guides | `references/migration-playbook.md` |
| 🚫 **Anti-Patterns Catalog** — 17 code smells with detection & cures | `references/anti-patterns.md` |
| 🔄 **Migration Playbook** — 6 step-by-step refactoring guides | `references/migration-playbook.md` |
| 📄 **Output Formats** — ADR, Health Check, Comparison Matrix | `references/output-format.md` |

### Framework-Specific Implementations
Expand Down Expand Up @@ -180,8 +180,8 @@ skills/design-patterns/
├── system-patterns.md # Microservices, Event-Driven, Cloud
├── framework-catalog.md # Next.js, FastAPI, NestJS, Django, Express, Go
├── testing-patterns.md # ✨ NEW — Unit/Integration/E2E test code
├── anti-patterns.md # ✨ NEW — 15 anti-patterns with detection
├── migration-playbook.md # ✨ NEW — 5 migration guides
├── anti-patterns.md # ✨ NEW — 17 anti-patterns with detection
├── migration-playbook.md # ✨ NEW — 6 migration guides
└── output-format.md # ✨ NEW — ADR, Reports, Health Check

RELEASE-NOTES.md
Expand Down
4 changes: 2 additions & 2 deletions skills/design-patterns/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,6 @@ Every design-patterns analysis MUST follow:
- `references/system-patterns.md` — Microservices, Event-Driven, Circuit Breaker, Saga, Cloud patterns
- `references/framework-catalog.md` — Pattern implementations for Next.js, FastAPI, NestJS, Django, Express, Go
- `references/testing-patterns.md` — How to test every pattern (unit, integration, e2e) with real test code
- `references/anti-patterns.md` — 15 anti-patterns catalog with detection commands, severities, and cures
- `references/migration-playbook.md` — 5 step-by-step migration guides with verification checkpoints
- `references/anti-patterns.md` — 17 anti-patterns catalog with detection commands, severities, and cures
- `references/migration-playbook.md` — 6 step-by-step migration guides with verification checkpoints
- `references/output-format.md` — ADR template, Refactoring Report, Health Check, Pattern Comparison
40 changes: 37 additions & 3 deletions skills/design-patterns/references/anti-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -341,11 +341,31 @@ class Order {

**Cure:** Start with Module Boundaries → Service Layer → Eventually Clean Architecture


---

### 13. N+1 Query Problem

**Severity:** 🔴 High

**Detection:** Repeated query pattern inside loops or ORM lazy-loading waterfalls.

```typescript
// 🔴 N+1: one query for orders + one query per order for items
const orders = await prisma.order.findMany();
for (const order of orders) {
order.items = await prisma.orderItem.findMany({ where: { orderId: order.id } });
}
```

**Cure:** Eager loading (`include`/`select`), batching, and query-level aggregation.


---

## System-Level Anti-Patterns

### 13. Distributed Monolith
### 14. Distributed Monolith

**Severity:** 🚨 Critical

Expand All @@ -361,7 +381,7 @@ class Order {

---

### 14. Chatty Services
### 15. Chatty Services

**Severity:** 🔴 High

Expand All @@ -371,7 +391,7 @@ class Order {

---

### 15. No Timeout / No Retry
### 16. No Timeout / No Retry

**Severity:** 🔴 High

Expand Down Expand Up @@ -421,3 +441,17 @@ Run this checklist on any module before refactoring:
- [ ] Shared database between services?
- [ ] Services that must deploy together?
```


### 17. Dual Writes (DB + Event Bus without Outbox)

**Severity:** 🚨 Critical

**Detection:** Application writes DB and publishes message in separate non-atomic steps.

**Symptoms:**
- Order exists in DB but `order.created` event missing
- Event published but DB transaction rolled back
- Hard-to-reconcile states across services

**Cure:** Transactional Outbox pattern with idempotent consumers.
14 changes: 12 additions & 2 deletions skills/design-patterns/references/architecture-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,20 @@ export class UserRepository {
```python
# routers/user.py — Controller
from fastapi import APIRouter, Depends
from schemas.user import CreateUserDTO, User
from services.user_service import UserService

router = APIRouter()

@router.post("/users", status_code=201)
@router.post("/users", status_code=201, response_model=User)
async def create_user(data: CreateUserDTO, service: UserService = Depends()):
return await service.create(data)

# services/user_service.py — Business Logic
from fastapi import Depends, HTTPException
from repositories.user_repository import UserRepository
from utils.security import hash_password

Comment on lines +112 to +115
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing type imports in the service snippet.

CreateUserDTO and User are used later in this snippet but not imported here, so the example is not self-contained.

Proposed doc fix
 from fastapi import Depends, HTTPException
+from schemas.user import CreateUserDTO, User
 from repositories.user_repository import UserRepository
 from utils.security import hash_password
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@skills/design-patterns/references/architecture-patterns.md` around lines 112
- 115, The snippet is missing the type imports for CreateUserDTO and User; add
explicit imports for the CreateUserDTO type and the User model/class so the
example is self-contained (e.g., import CreateUserDTO and User from your
DTO/schema and model modules respectively), and ensure the top-of-file import
block that currently contains Depends, HTTPException, UserRepository, and
hash_password also includes CreateUserDTO and User.

class UserService:
def __init__(self, repo: UserRepository = Depends()):
self.repo = repo
Expand All @@ -117,12 +122,17 @@ class UserService:
if existing:
raise HTTPException(409, "Email already exists")
hashed = hash_password(data.password)
return await self.repo.save({**data.dict(), "password": hashed})
return await self.repo.save({**data.model_dump(), "password": hashed})

# repositories/user_repository.py — Data Access
from db.client import database

class UserRepository:
async def find_by_email(self, email: str) -> User | None:
return await database.users.find_one({"email": email})

async def save(self, data: dict) -> User:
return await database.users.insert_one(data)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return a User object from FastAPI save example

UserRepository.save is annotated to return User, but database.users.insert_one(data) returns an insert result object in common Mongo drivers (e.g., Motor/PyMongo), not the created document. In this snippet, create_user also declares response_model=User, so copying this code will produce response-validation failures or missing user fields after create; the example should return a user document (e.g., fetch by inserted id) instead of the raw insert result.

Useful? React with 👍 / 👎.

Comment on lines +134 to +135
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

PyMongo insert_one return type what does it return

💡 Result:

Collection.insert_one(document, ...) returns a pymongo.results.InsertOneResult object. [1]

From that result you typically use:

  • result.inserted_id → the inserted document’s _id [1]
  • result.acknowledged → whether the write was acknowledged (e.g., False with WriteConcern(w=0)) [1]

Example:

result = collection.insert_one({"x": 1})
_id = result.inserted_id
``` [2]

**Sources:** [1] PyMongo `InsertOneResult` API docs, [2] PyMongo `Collection.insert_one` API docs

---

🌐 Web query:

`Motor async MongoDB driver insert_one return type`

💡 Result:

In Motor (e.g., `AsyncIOMotorCollection.insert_one()`), `insert_one` is an async coroutine, so you do:

```py
result = await collection.insert_one(doc)

and it returns a pymongo.results.InsertOneResult (type annotated as InsertOneResult). You typically use result.inserted_id (and result.acknowledged). [1]

Source(s): [1] (motor.readthedocs.io)

Citations:


Fix the return type contract: save() must return a User, not InsertOneResult.

The code declares async def save(self, data: dict) -> User: but insert_one() returns a pymongo.results.InsertOneResult object containing only metadata (inserted_id, acknowledged). This breaks the return contract expected by the service layer and controller.

Proposed doc fix
 class UserRepository:
     async def save(self, data: dict) -> User:
-        return await database.users.insert_one(data)
+        result = await database.users.insert_one(data)
+        return await database.users.find_one({"_id": result.inserted_id})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def save(self, data: dict) -> User:
return await database.users.insert_one(data)
async def save(self, data: dict) -> User:
result = await database.users.insert_one(data)
return await database.users.find_one({"_id": result.inserted_id})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@skills/design-patterns/references/architecture-patterns.md` around lines 134
- 135, The save() function currently returns the InsertOneResult from
database.users.insert_one(data) but is declared to return a User; update save to
return a populated User instance: call database.users.insert_one(data), get
result.inserted_id, then fetch the created document (e.g.,
database.users.find_one({"_id": inserted_id})) or construct a User from the
original data plus the inserted_id, and return that User object; also update the
function's internal variable names and typing if needed so save() actually
returns a User rather than InsertOneResult (referencing save and
database.users.insert_one).

```

---
Expand Down
1 change: 1 addition & 0 deletions skills/design-patterns/references/code-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ if (process.env.NODE_ENV !== "production") globalForPrisma.prisma = prisma;
**Python Example:**
```python
# config.py — Thread-safe Singleton
import os
import threading

class AppConfig:
Expand Down
16 changes: 16 additions & 0 deletions skills/design-patterns/references/framework-catalog.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,22 @@

---


## Version Baseline (Validate Quarterly)

> Keep examples aligned with actively maintained major versions.

| Framework | Baseline in this catalog |
|-----------|--------------------------|
| Next.js | 15.x (App Router + Server Actions) |
| FastAPI | 0.110+ (Pydantic v2 style) |
| NestJS | 10.x+ |
| Django | 5.x |
| Express | 5.x |
| Go | 1.22+ |

---

## Next.js (App Router)

### Recommended Architecture
Expand Down
63 changes: 63 additions & 0 deletions skills/design-patterns/references/migration-playbook.md
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,69 @@ Week 4: Remove old code from monolith

**Step 6: Repeat** for next least-coupled module.


---

## Migration 6: Direct Publish → Transactional Outbox

**Difficulty:** ⭐⭐⭐⭐ (Advanced)
**Time:** 2-5 days
**Risk:** High (touches write path + async workers)

### Problem Signature
```typescript
await orderRepo.save(order); // DB write
await eventBus.publish(new OrderCreated); // separate write (can fail)
```

### Steps

**Step 1: Add outbox table**
```sql
CREATE TABLE outbox_events (
id UUID PRIMARY KEY,
aggregate_type TEXT NOT NULL,
aggregate_id TEXT NOT NULL,
event_type TEXT NOT NULL,
payload JSONB NOT NULL,
status TEXT NOT NULL DEFAULT 'pending',
created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
sent_at TIMESTAMPTZ
);
```

**Step 2: Write business row + outbox row in one transaction**
```typescript
await prisma.$transaction(async (tx) => {
await tx.order.create({ data: orderData });
await tx.outboxEvent.create({
data: {
aggregateType: 'order',
aggregateId: orderData.id,
eventType: 'order.created',
Comment on lines +387 to +393
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use persisted order ID, not orderData.id, in outbox event.

orderData.id may not exist when IDs are DB-generated, causing invalid aggregateId in the outbox record.

Suggested fix
 await prisma.$transaction(async (tx) => {
-  await tx.order.create({ data: orderData });
+  const createdOrder = await tx.order.create({ data: orderData });
   await tx.outboxEvent.create({
     data: {
       aggregateType: 'order',
-      aggregateId: orderData.id,
+      aggregateId: createdOrder.id,
       eventType: 'order.created',
-      payload: orderData,
+      payload: createdOrder,
     },
   });
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await prisma.$transaction(async (tx) => {
await tx.order.create({ data: orderData });
await tx.outboxEvent.create({
data: {
aggregateType: 'order',
aggregateId: orderData.id,
eventType: 'order.created',
await prisma.$transaction(async (tx) => {
const createdOrder = await tx.order.create({ data: orderData });
await tx.outboxEvent.create({
data: {
aggregateType: 'order',
aggregateId: createdOrder.id,
eventType: 'order.created',
payload: createdOrder,
},
});
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@skills/design-patterns/references/migration-playbook.md` around lines 387 -
393, The outbox event is using orderData.id which may be undefined for
DB-generated IDs; inside the prisma.$transaction callback, capture the created
order returned by tx.order.create (e.g., assign its result to a variable) and
use that persisted order's id when calling tx.outboxEvent.create for aggregateId
instead of orderData.id; update references in the transaction block
(tx.order.create and tx.outboxEvent.create) to use the created order's id.

payload: orderData,
},
});
});
```

**Step 3: Add outbox publisher worker**
- Poll pending rows in small batches
- Publish to broker
- Mark rows as sent
- Retry with exponential backoff

**Step 4: Make consumers idempotent**
- Deduplicate by event ID
- Store processed event IDs

**Step 5: Verify**
```bash
# Simulate broker outage, ensure events remain in outbox pending state
npm test
```


---

## Migration Verification Checklist
Expand Down
46 changes: 46 additions & 0 deletions skills/design-patterns/references/system-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,50 @@ Phase 3: /api/users + /api/orders → New
Phase N: 100% traffic → New, Legacy decommissioned
```


---

## 9. Event Sourcing Pattern

### Problem
Auditability and temporal debugging are hard when you only store the latest row state.

### Solution
Persist immutable domain events as the source of truth, and rebuild state from the event stream.

### When to Use
- Need full audit history (fintech, compliance-heavy domains)
- Need time-travel debugging ("what did we know at T1?")
- Complex domain workflows where replay is valuable

### Trade-offs
- Higher implementation complexity than CRUD
- Requires event versioning strategy
- Read models usually need separate projections

---

## 10. Transactional Outbox Pattern

### Problem
Dual-write risk: write DB row succeeds, publish event fails (or vice versa) causing inconsistency.

### Solution
Write business data and outbox record in ONE local transaction, then publish asynchronously from the outbox.

### Minimal Flow
```
1) Begin transaction
2) Write business entity (e.g., order)
3) Write outbox event row (e.g., order.created)
4) Commit
5) Outbox worker publishes event and marks row as sent
```

### Why It Matters
Outbox is the default reliability companion for Event-Driven + Saga systems.


---

## Pattern Decision Matrix
Expand All @@ -295,3 +339,5 @@ Phase N: 100% traffic → New, Legacy decommissioned
| Legacy migration | Strangler Fig | Medium | Any |
| Cross-cutting concerns | Sidecar | Medium | 3+ |
| Monolith too big | Microservices | Very High | 8+ |
| Need full audit trail + replay | Event Sourcing | High | 5+ |
| Reliable DB + event publication | Transactional Outbox | Medium | 3+ |
27 changes: 25 additions & 2 deletions skills/design-patterns/references/testing-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ describe("EventBus", () => {
describe("CommandHistory", () => {
let history: CommandHistory;
let cart: Cart;
const laptop: CartItem = { id: "1", name: "Laptop", qty: 1 };

beforeEach(() => {
history = new CommandHistory();
Expand Down Expand Up @@ -192,9 +193,11 @@ describe("Middleware Decorators", () => {
**The Key:** Mock the Repository, test the Service logic.

```typescript
import type { Mocked } from "vitest";

describe("UserService", () => {
let service: UserService;
let mockRepo: jest.Mocked<UserRepository>;
let mockRepo: Mocked<UserRepository>;

beforeEach(() => {
mockRepo = {
Expand Down Expand Up @@ -232,6 +235,7 @@ describe("UserRepository (Integration)", () => {
beforeAll(async () => {
// Use test database
await prisma.$connect();
repo = new UserRepository(prisma);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Repository API shape now conflicts with architecture reference.

This example uses new UserRepository(prisma) (instance API), while skills/design-patterns/references/architecture-patterns.md (Line 87-Line 94) still documents static repository methods. These two docs currently teach incompatible contracts.

Please align both references to one canonical style (instance + DI or static utility).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@skills/design-patterns/references/testing-patterns.md` at line 238, The two
docs present conflicting Repository APIs: the testing-patterns example
instantiates UserRepository with "new UserRepository(prisma)" while the
architecture-patterns reference describes static repository methods; pick one
canonical style and make both docs consistent. Either (A) convert the
testing-patterns example to the static API by replacing instance usage with the
documented static methods (adjust any calls to UserRepository.find...,
UserRepository.create... accordingly), or (B) update the architecture-patterns
reference to document the instance + DI approach by replacing static examples
with constructor-based usage and showing dependency injection for UserRepository
and its methods; ensure all examples and code snippets consistently reference
the chosen API shape (UserRepository constructor + instance methods or
UserRepository static methods).

});

afterEach(async () => {
Expand All @@ -256,15 +260,27 @@ describe("UserRepository (Integration)", () => {
```typescript
// Command side — test business rules
describe("CreateOrderHandler", () => {
let handler: CreateOrderHandler;
const mockRepo = { save: vi.fn() };
const mockEventBus = { publish: vi.fn() };

beforeEach(() => {
handler = new CreateOrderHandler(mockRepo, mockEventBus);
});

it("validates stock before creating order", async () => {
const handler = new CreateOrderHandler(mockRepo, mockEventBus);
await expect(handler.execute(new CreateOrderCommand("user-1", [
{ productId: "p-1", quantity: 999 }, // Exceeds stock
]))).rejects.toThrow("Insufficient stock");
});

it("publishes OrderCreated event", async () => {
const validCommand = new CreateOrderCommand("user-1", [
{ productId: "p-1", quantity: 1 },
]);

await handler.execute(validCommand);

expect(mockEventBus.publish).toHaveBeenCalledWith(
expect.objectContaining({ type: "OrderCreated" })
);
Expand All @@ -273,6 +289,13 @@ describe("CreateOrderHandler", () => {

// Query side — test data retrieval
describe("GetOrderSummaryHandler", () => {
let handler: GetOrderSummaryHandler;
const readDb = { query: vi.fn() };

beforeEach(() => {
handler = new GetOrderSummaryHandler(readDb);
});
Comment on lines +292 to +297
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Query handler setup is incomplete for the asserted result shape.

readDb.query is never arranged, but the test later asserts denormalized fields. Add a deterministic mock return in beforeEach (or in the test case) so the example is runnable.

Proposed doc fix
 beforeEach(() => {
   handler = new GetOrderSummaryHandler(readDb);
+  readDb.query.mockResolvedValue({
+    orderId: "ord-1",
+    userName: "Test User",
+    itemCount: 2,
+  });
 });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@skills/design-patterns/references/testing-patterns.md` around lines 292 -
297, The test fails to arrange readDb.query, so inside the beforeEach that
constructs handler = new GetOrderSummaryHandler(readDb) set a deterministic mock
return for readDb.query (e.g., mockResolvedValue / mockReturnValue depending on
whether GetOrderSummaryHandler expects a promise) that returns the denormalized
fields the assertions expect; update the mock on the readDb object
(readDb.query) so the example is runnable and returns the exact shape asserted
by the test.


it("returns denormalized view", async () => {
const result = await handler.execute(new GetOrderSummaryQuery("ord-1"));
expect(result).toMatchObject({
Expand Down