Analyze whether code fits with established codebase patterns and conventions
Analyzes code against established codebase patterns and conventions to identify inconsistencies and alignment issues.
/plugin marketplace add dgriffith/bad-daves-robot-army/plugin install dgriffith-bad-daves-robot-army@dgriffith/bad-daves-robot-armyUsing @agent-mentor analyze whether specific code aligns with established codebase patterns, conventions, and architectural principles.
The user invoked: /does-this-clash {code_reference}
Examples:
/does-this-clash src/services/new-feature.ts - Check if new code fits patterns/does-this-clash PR #123 - Review PR for pattern consistency/does-this-clash this commit - Check recent changes/does-this-clash - Interactive mode to identify what to checkThis is a pattern consistency analysis, not subjective taste critique. The goal is to help developers write code that fits naturally into the existing codebase by identifying where their code diverges from established conventions.
Focus on observable patterns, not opinions. This tool:
This is NOT about "right" vs "wrong" - it's about consistency vs. divergence, and helping developers understand the existing patterns before they choose to diverge from them.
Determine:
If unclear, ask:
Map the existing patterns in the codebase:
Identify the established architecture:
# What's the folder structure convention?
tree src/ -L 2 -d
# Where do similar things live?
find . -name "*Service.ts" -o -name "*Controller.ts" -o -name "*Repository.ts"
# What's the layering pattern?
# Controllers -> Services -> Repositories?
# Features -> Domain -> Infrastructure?
Questions to answer:
Naming Conventions:
# What naming style is used?
grep -r "^class \|^function \|^const \|^interface " src/ | head -20
# Variable naming patterns
# camelCase? snake_case? PascalCase?
# Descriptive names? Abbreviations? Prefixes?
File Naming:
# How are files named?
ls src/**/*.ts | head -20
# kebab-case? camelCase? PascalCase?
# .service.ts? Service.ts? -service.ts?
Function/Method Patterns:
How does the codebase handle errors?
# Exceptions or error returns?
grep -r "throw new" src/ | head -10
grep -r "return.*Error\|Result<" src/ | head -10
# Try-catch patterns
grep -r "try {" src/ -A 5 | head -20
# Error types
grep -r "class.*Error extends" src/
Common patterns:
How are dependencies handled?
# Dependency injection?
grep -r "constructor.*inject\|@Inject" src/ | head -10
# Service locator?
grep -r "Container.get\|ServiceLocator" src/ | head -10
# Direct imports?
grep -r "import.*Service.*from" src/ | head -10
Common patterns:
How does code talk to data?
# ORM usage
grep -r "Repository\|Entity\|@Column" src/ | head -10
# Raw SQL
grep -r "SELECT \|query(.*SELECT\|sql\`" src/ | head -10
# Query builders
grep -r "\.where(.*\.select(" src/ | head -10
Common patterns:
How are tests structured?
# Test file naming
ls **/*.test.* **/*.spec.* | head -10
# Test structure
grep -r "describe(.*it(" tests/ | head -20
# Mocking patterns
grep -r "jest.mock\|sinon\|vi.mock" tests/ | head -10
Common patterns:
How are interfaces designed?
# REST conventions
grep -r "router\.\(get\|post\|put\|delete\)" src/ | head -20
# GraphQL patterns
grep -r "type Query\|type Mutation" src/ | head -10
# Internal APIs
grep -r "export.*function\|export.*class" src/ | head -20
Common patterns:
How is state handled?
# State libraries
grep -r "useState\|createStore\|atom\|signal" src/ | head -10
# State mutation patterns
grep -r "\.setState\|state\.\w+\s*=" src/ | head -10
Common patterns:
Compare the target code against discovered patterns:
For each pattern category, identify:
✅ Aligned (Follows Pattern):
⚠️ Divergent (Different Approach):
❌ Contradictory (Conflicts with Pattern):
Before flagging divergence, investigate if there's a reason:
Check for intentional pattern evolution:
# Is this part of a migration?
git log --grep="migrate\|refactor\|modernize" -- {file}
# Are similar recent changes using new pattern?
git log --since="3 months ago" --oneline -- src/{area}/ | head -20
# Is there a TODO or ADR about this?
grep -r "TODO.*migrate\|FIXME.*update" . --include="*.md"
Look for external constraints:
Check for similar code:
# Is this actually following a subset pattern?
grep -r "similar-pattern" src/{other-area}/
Create a markdown file at /reports/pattern-consistency-{scope}-{timestamp}.md with the analysis.
# Pattern Consistency Analysis: [Code Reference]
## What Was Analyzed
**Scope**: [File/Module/PR being checked]
**Type**: [Service/Component/Util/Test/etc.]
**Purpose**: [What this code does]
**Context**: [New feature / Bug fix / Refactor / etc.]
## Summary
**Overall Consistency**: ✅ Well-aligned / ⚠️ Some divergence / ❌ Significant conflicts
**Key Findings:**
- [Most important observation]
- [Second most important observation]
- [Third observation if relevant]
**TL;DR**: [One sentence assessment]
---
## Established Codebase Patterns
### Architectural Organization
**Pattern**: [How the codebase is organized]
src/ ├── features/ # Feature-based organization │ ├── auth/ │ ├── users/ │ └── billing/ ├── shared/ # Shared utilities └── infrastructure/ # External concerns
**Principle**: Features are self-contained, shared code is minimal
**Examples**: `src/features/auth/`, `src/features/users/`
### Naming Conventions
**Pattern**: [Established naming style]
- **Files**: `kebab-case.ts` (e.g., `user-service.ts`, `auth-controller.ts`)
- **Classes**: `PascalCase` (e.g., `UserService`, `AuthController`)
- **Functions**: `camelCase` (e.g., `validateUser`, `createSession`)
- **Constants**: `UPPER_SNAKE_CASE` (e.g., `MAX_RETRIES`, `API_TIMEOUT`)
- **Interfaces**: `PascalCase` with descriptive names (e.g., `UserRepository`, `AuthConfig`)
**Examples**:
- `src/features/auth/auth-service.ts` exports `AuthService` class
- `src/shared/utils/string-helpers.ts` exports `camelCase` functions
### Error Handling
**Pattern**: [How errors are handled]
**Approach**: Custom error classes + try-catch at boundaries
```typescript
// Custom errors for domain problems
class UserNotFoundError extends Error {
constructor(userId: string) {
super(`User ${userId} not found`);
}
}
// Try-catch at controller boundaries
async handleRequest(req, res) {
try {
const result = await service.process(req.body);
res.json(result);
} catch (error) {
if (error instanceof UserNotFoundError) {
res.status(404).json({ error: error.message });
} else {
logger.error(error);
res.status(500).json({ error: "Internal error" });
}
}
}
// Services throw, controllers catch
Examples: src/features/auth/errors.ts, src/shared/middleware/error-handler.ts
Why this pattern: Exceptions for exceptional cases, caught at boundaries, logged centrally
Pattern: [How dependencies are injected]
Approach: Constructor injection with interfaces
interface UserRepository {
findById(id: string): Promise<User | null>;
}
class UserService {
constructor(private userRepo: UserRepository) {}
async getUser(id: string): Promise<User> {
const user = await this.userRepo.findById(id);
if (!user) throw new UserNotFoundError(id);
return user;
}
}
Examples: src/features/users/user-service.ts
Why this pattern: Testability, loose coupling, clear dependencies
Pattern: [How data is accessed]
Approach: Repository pattern with Prisma ORM
class UserRepository {
constructor(private db: PrismaClient) {}
async findById(id: string): Promise<User | null> {
return this.db.user.findUnique({ where: { id } });
}
}
Not used: Raw SQL, query builders (except for complex reporting queries)
Examples: src/features/users/user-repository.ts
Why this pattern: Type safety, consistent API, easy testing
Pattern: [How tests are structured]
Approach: Jest with describe/it, mocks for external dependencies
describe('UserService', () => {
let service: UserService;
let mockRepo: jest.Mocked<UserRepository>;
beforeEach(() => {
mockRepo = {
findById: jest.fn(),
};
service = new UserService(mockRepo);
});
it('should throw when user not found', async () => {
mockRepo.findById.mockResolvedValue(null);
await expect(service.getUser('123'))
.rejects
.toThrow(UserNotFoundError);
});
});
Examples: src/features/users/__tests__/user-service.test.ts
Why this pattern: Clear structure, isolated tests, easy to read
Pattern: [How APIs are structured]
Approach: RESTful with resource-based routes
router.get('/users/:id', userController.getUser);
router.post('/users', userController.createUser);
router.put('/users/:id', userController.updateUser);
router.delete('/users/:id', userController.deleteUser);
Response format: JSON with consistent shape
// Success
{ data: { /* resource */ } }
// Error
{ error: { message: string, code?: string } }
Examples: src/features/users/user-routes.ts
Your code is in src/features/billing/billing-service.ts
Why this is good: Follows feature-based organization, billing is clearly a feature
Pattern match: Same structure as auth/ and users/ features
Your code: BillingService directly imports from src/infrastructure/database/prisma
Established pattern: Services use Repository abstractions, not direct DB access
Examples of pattern:
// Established pattern (from UserService)
class UserService {
constructor(private userRepo: UserRepository) {} // ← Repository abstraction
}
// Your code
class BillingService {
constructor(private db: PrismaClient) {} // ← Direct DB access
}
Why this matters:
Found in: src/features/billing/billing-service.ts:12
How to align:
// Create repository abstraction
interface BillingRepository {
findInvoice(id: string): Promise<Invoice | null>;
saveInvoice(invoice: Invoice): Promise<void>;
}
class BillingService {
constructor(private billingRepo: BillingRepository) {} // ← Matches pattern
}
Counterpoint to consider: Is billing-specific query complexity why you chose direct access? If so, document this decision and consider if the trade-off is worth the inconsistency.
BillingService uses PascalCase, matches pattern
billing-service.ts uses kebab-case, matches pattern
Established pattern: Descriptive verb-noun (e.g., createUser, validateToken)
Your code: Abbreviated names (e.g., procInv, calcTtl)
Examples of divergence:
// Established pattern
async function validateUser(userId: string): Promise<boolean>
async function createSession(user: User): Promise<Session>
// Your code
async function procInv(inv: Invoice): Promise<void> // ← Abbreviated
async function calcTtl(items: Item[]): Promise<number> // ← Abbreviated
Why this matters:
How to align:
async function processInvoice(invoice: Invoice): Promise<void>
async function calculateTotal(items: Item[]): Promise<number>
Established pattern: Throw custom error classes
Your code: Returns null or undefined on errors
Examples of conflict:
// Established pattern (from UserService)
async function getUser(id: string): Promise<User> {
const user = await repo.findById(id);
if (!user) throw new UserNotFoundError(id); // ← Throws custom error
return user;
}
// Your code (from BillingService)
async function getInvoice(id: string): Promise<Invoice | null> {
const invoice = await db.invoice.findUnique({ where: { id } });
return invoice; // ← Returns null on not found
}
Why this matters:
How to align:
class InvoiceNotFoundError extends Error {
constructor(invoiceId: string) {
super(`Invoice ${invoiceId} not found`);
}
}
async function getInvoice(id: string): Promise<Invoice> {
const invoice = await repo.findById(id);
if (!invoice) throw new InvoiceNotFoundError(id);
return invoice;
}
Found in: src/features/billing/billing-service.ts:45, :67, :89
Your code uses constructor injection consistently
Established pattern: Jest with describe/it nesting, beforeEach setup
Your code: Flat test() calls, setup in each test
Examples of divergence:
// Established pattern
describe('UserService', () => {
let service: UserService;
beforeEach(() => {
service = new UserService(mockRepo);
});
it('should create user', async () => { /* ... */ });
it('should validate email', async () => { /* ... */ });
});
// Your code
test('create invoice works', async () => {
const service = new BillingService(mockDb); // ← Setup in each test
/* ... */
});
test('calculate total works', async () => {
const service = new BillingService(mockDb); // ← Duplicate setup
/* ... */
});
Why this matters:
How to align: Use describe/it structure with beforeEach
Found in: src/features/billing/__tests__/billing-service.test.ts
Aligned (4 areas):
Divergent (3 areas):
Conflicting (1 area):
High Impact (Fix Soon):
Medium Impact (Should Fix): 3. Function naming - Readability and consistency matter 4. Test structure - Maintainability and consistency
Low Impact (Nice to Have): None identified
Change from:
async function getInvoice(id: string): Promise<Invoice | null> {
return await db.invoice.findUnique({ where: { id } });
}
Change to:
async function getInvoice(id: string): Promise<Invoice> {
const invoice = await repo.findById(id);
if (!invoice) throw new InvoiceNotFoundError(id);
return invoice;
}
Why: Matches established pattern, makes errors explicit, consistent with 23 other services
Files to update: billing-service.ts:45, :67, :89
Create: BillingRepository interface + implementation
Change from:
class BillingService {
constructor(private db: PrismaClient) {}
}
Change to:
class BillingService {
constructor(private billingRepo: BillingRepository) {}
}
Why: Matches layering pattern, improves testability, consistent with UserService, AuthService, etc.
Files to create: billing-repository.ts, billing-repository.test.ts
Expand abbreviated names:
procInv → processInvoicecalcTtl → calculateTotalgenRpt → generateReportWhy: Readability, consistency with 200+ other descriptive function names
Files to update: billing-service.ts (8 functions)
Refactor tests to use describe/it with beforeEach setup
Why: Consistency with 47 other test files, better organization
Files to update: billing-service.test.ts
These aren't criticisms - they're genuine questions to help you think through the trade-offs:
Q: Did you skip the repository layer for a specific reason?
If intentional: Consider documenting why billing is different, or this becomes confusing
If not intentional: Aligning with the pattern would help maintainability
Q: What's the advantage of null returns over throwing errors in your use case?
Consider: The codebase uses exceptions consistently - null checks would be needed everywhere in your code
Q: Is there a character limit or verbosity concern?
Consider: Most function names in the codebase are fully spelled out
That's totally valid! Sometimes new patterns are better than established ones.
If you're intentionally introducing a different approach:
Example documentation:
/**
* Billing module uses null returns instead of exceptions for not-found cases
* to match the payment provider API semantics. See ADR-023 for rationale.
*/
Why it matters:
It's not about "right" vs "wrong":
How to discover patterns:
When to follow vs. diverge:
As you work in codebases, you'll develop intuition for:
This analysis helps build that intuition explicitly.
/why-this-way [pattern] - Understand why existing patterns exist/explain [code] - Deep dive into how something works/what-would-break [change] - Understand impact of changing patternsAnalysis completed: [timestamp] Patterns analyzed: [count] Files compared: [count] Similar code examined: [count examples]
Remember: Consistency is valuable, but not sacred. This analysis helps you make informed decisions about when to follow patterns and when to diverge from them. The goal is intentional code, not blind conformity.
## Investigation Techniques
### Pattern Discovery Methods
**Statistical Analysis:**
```bash
# Count pattern occurrences to establish "established"
grep -r "class.*Service" src/ | wc -l # 23 services
grep -r "throw new.*Error" src/ | wc -l # 145 throws
grep -r "return null" src/ | wc -l # 12 null returns
# If 23 services use pattern A and 1 uses pattern B, A is established
Find Representative Examples:
# Find the most similar code to what's being checked
# If checking a service, find other services
find src/features -name "*-service.ts" | head -5
# If checking tests, find similar tests
find . -name "*.test.ts" -path "*/features/*" | head -5
Architectural Layering:
# Map import patterns to understand layers
grep -r "^import.*from" src/features/users/ | \
sed 's/.*from "\(.*\)".*/\1/' | \
sort | uniq -c | sort -rn
# See what each layer imports
# Controllers import Services?
# Services import Repositories?
# Repositories import Database?
Convention Documentation:
# Check for explicit conventions
cat CONTRIBUTING.md docs/STYLE_GUIDE.md README.md
# Check for linter rules (explicit conventions)
cat .eslintrc.json .prettierrc
# Check for architectural decision records
find . -name "*.md" -path "*/adr/*"
Check for Migration in Progress:
# Recent commits changing patterns
git log --since="6 months ago" --grep="refactor\|migrate" --oneline
# Mixed old/new patterns suggest transition
git log --all --oneline -- src/features/*/new-pattern.ts
Check for External Requirements:
# Framework requirements
cat package.json | grep -E "next|nest|express"
# Library patterns
grep -r "@nestjs/common" src/ # NestJS has opinions
grep -r "from 'next" src/ # Next.js has conventions
Side-by-Side Pattern Comparison:
Always show:
Example:
// Established pattern (from user-service.ts:23)
async function getUser(id: string): Promise<User> {
const user = await this.repo.findById(id);
if (!user) throw new UserNotFoundError(id);
return user;
}
// Your code (from billing-service.ts:45)
async function getInvoice(id: string): Promise<Invoice | null> {
return await this.db.invoice.findUnique({ where: { id } });
}
// Difference: null return vs. exception throw
// Why it matters: Inconsistent error handling, callers must handle differently
DO:
DON'T:
For every divergence, explain:
Include questions like:
Valid reasons:
Invalid reasons:
Include sections like:
### How to Discover Patterns Yourself
1. Look at multiple similar examples
2. Count occurrences (1 instance ≠ pattern)
3. Check documentation
4. Look for repeated structures
5. Ask: "How do other parts do this?"
### Building Pattern Intuition
As you read code, ask:
- "Is this the only place that does it this way?"
- "Does this feel different from similar code?"
- "Are the same concerns handled differently elsewhere?"
Good news! Your code aligns well with established patterns.
**Alignment: 9/10**
✅ Matches architectural structure
✅ Follows naming conventions
✅ Uses established error handling
✅ Consistent testing approach
**Minor observations:**
- [One small thing if any]
**Teaching moment:** You've clearly studied the codebase patterns. This code will feel natural to maintainers.
[Save brief report]
Your code is mostly aligned with a few areas of divergence from established patterns.
**Alignment: 6/10**
Let me show you the patterns and where your code differs:
[Detailed analysis of patterns and divergences]
**High priority:**
- [Most important divergence to address]
**Medium priority:**
- [Less critical divergences]
**Teaching moment:** [Explain why patterns matter and how to discover them]
[Save detailed report]
Your code takes a different approach than the established codebase patterns. Let's map out the differences:
[Comprehensive analysis]
**If this is intentional:**
That's valid! New patterns can be improvements. Consider:
- Documenting why
- Being consistent within your module
- Discussing with team
**If this is accidental:**
Here's how to align with established patterns: [specific steps]
**Teaching moment:** [Pattern discovery and consistency value]
[Save comprehensive report]
Your code differs from established patterns, but I can see why:
**Divergence detected:**
[What's different]
**Possible justifications:**
- [External API requirements]
- [Performance constraints]
- [Library requirements]
**Questions to consider:**
- Is this difference intentional?
- Should it be documented?
- Is this a billing-specific need?
If there's a good reason, that's totally valid! Just consider documenting it so future maintainers understand.
[Save report with context]
Remember: The goal is to help developers write code that fits naturally into the codebase, not to enforce arbitrary rules. Consistency has value, but so does pattern evolution. This tool helps make those choices intentionally rather than accidentally.
YOU MUST CREATE THE REPORT FILE. This is not optional.
Create the report file using the Write tool at the specified path:
/reports/{command-name}-{scope}-{timestamp}.mdYYYY-MM-DD-HHmmss/reports/architecture-review-entire-project-2025-10-14-143022.mdFill in ALL sections of the report template
Confirm completion by telling the user:
❌ DON'T: Just summarize findings in the chat ❌ DON'T: Say "I'll create a report" without actually doing it ❌ DON'T: Leave sections incomplete or with placeholders ❌ DON'T: Forget to use the Write tool
✅ DO: Always use the Write tool to create the markdown file ✅ DO: Fill in every section with real findings ✅ DO: Provide the full path to the user when done ✅ DO: Include actionable recommendations
Before responding to the user, verify:
Remember: The report is the primary deliverable. The chat summary is secondary.