# Architecture Knowledge

## Structure & Design

**File Organization:**

| Criteria | Judgment |
|----------|----------|
| Single file > 200 lines | Consider splitting |
| Single file > 300 lines | REJECT |
| Single file with multiple responsibilities | REJECT |
| Unrelated code coexisting | REJECT |

**Module Structure:**

- High cohesion: Related functionality grouped together
- Low coupling: Minimal inter-module dependencies
- No circular dependencies
- Appropriate directory hierarchy

**Function Design:**

- One responsibility per function
- Consider splitting functions over 30 lines
- Side effects clearly defined

**Layer Design:**

- Dependency direction: Upper layers -> Lower layers (reverse prohibited)
- Controller -> Service -> Repository flow maintained
- 1 interface = 1 responsibility (no giant Service classes)

**Directory Structure:**

Structure pattern selection:

| Pattern | Use Case | Example |
|---------|----------|---------|
| Layered | Small scale, CRUD-centric | `controllers/`, `services/`, `repositories/` |
| Vertical Slice | Medium-large scale, high feature independence | `features/auth/`, `features/order/` |
| Hybrid | Common foundation + feature modules | `core/` + `features/` |

Vertical Slice Architecture (organizing code by feature):

```
src/
├── features/
│   ├── auth/
│   │   ├── LoginCommand.ts
│   │   ├── LoginHandler.ts
│   │   ├── AuthRepository.ts
│   │   └── auth.test.ts
│   └── order/
│       ├── CreateOrderCommand.ts
│       ├── CreateOrderHandler.ts
│       └── ...
└── shared/           # Shared across features
    ├── database/
    └── middleware/
```

Vertical Slice criteria:

| Criteria | Judgment |
|----------|----------|
| Single feature spans 3+ layers | Consider slicing |
| Minimal inter-feature dependencies | Recommend slicing |
| Over 50% shared processing | Keep layered |
| Team organized by features | Slicing required |

Prohibited patterns:

| Pattern | Problem |
|---------|---------|
| Bloated `utils/` | Becomes graveyard of unclear responsibilities |
| Lazy placement in `common/` | Dependencies become unclear |
| Excessive nesting (4+ levels) | Navigation difficulty |
| Mixed features and layers | `features/services/` prohibited |

**Separation of Concerns:**

- Read and write responsibilities separated
- Data fetching at root (View/Controller), passed to children
- Error handling centralized (no try-catch scattered everywhere)
- Business logic not leaking into Controller/View

## Code Quality Detection

**Explanatory Comment (What/How) Detection Criteria:**

Detect comments that simply restate code behavior in natural language.

| Judgment | Criteria |
|----------|----------|
| REJECT | Restates code behavior in natural language |
| REJECT | Repeats what is already obvious from function/variable names |
| REJECT | JSDoc that only paraphrases the function name without adding information |
| OK | Explains why a particular implementation was chosen |
| OK | Explains the reason behind seemingly unusual behavior |
| Best | No comment needed — the code itself communicates intent |

```typescript
// REJECT - Restates code (What)
// If interrupted, abort immediately
if (status === 'interrupted') {
  return ABORT_STEP;
}

// REJECT - Restates the loop
// Check transitions in order
for (const transition of step.transitions) {

// REJECT - Repeats the function name
/** Check if status matches transition condition. */
export function matchesCondition(status: Status, condition: TransitionCondition): boolean {

// OK - Design decision (Why)
// User interruption takes priority over piece-defined transitions
if (status === 'interrupted') {
  return ABORT_STEP;
}

// OK - Reason behind seemingly odd behavior
// stay can cause loops, but is only used when explicitly specified by the user
return step.name;
```

**Direct State Mutation Detection Criteria:**

Detect direct mutation of arrays or objects.

```typescript
// REJECT - Direct array mutation
const steps: Step[] = getSteps();
steps.push(newStep);           // Mutates original array
steps.splice(index, 1);       // Mutates original array
steps[0].status = 'done';     // Nested object also mutated directly

// OK - Immutable operations
const withNew = [...steps, newStep];
const without = steps.filter((_, i) => i !== index);
const updated = steps.map((s, i) =>
  i === 0 ? { ...s, status: 'done' } : s
);

// REJECT - Direct object mutation
function updateConfig(config: Config) {
  config.logLevel = 'debug';   // Mutates argument directly
  config.steps.push(newStep);  // Nested mutation too
  return config;
}

// OK - Returns new object
function updateConfig(config: Config): Config {
  return {
    ...config,
    logLevel: 'debug',
    steps: [...config.steps, newStep],
  };
}
```

## Security (Basic Checks)

- Injection prevention (SQL, Command, XSS)
- User input validation
- Hardcoded sensitive information

## Testability

- Dependency injection enabled
- Mockable design
- Tests are written

## Anti-Pattern Detection

REJECT when these patterns are found:

| Anti-Pattern | Problem |
|--------------|---------|
| God Class/Component | Single class with too many responsibilities |
| Feature Envy | Frequently accessing other modules' data |
| Shotgun Surgery | Single change ripples across multiple files |
| Over-generalization | Variants and extension points not currently needed |
| Hidden Dependencies | Child components implicitly calling APIs etc. |
| Non-idiomatic | Custom implementation ignoring language/FW conventions |

## Abstraction Level Evaluation

**Conditional Branch Proliferation Detection:**

| Pattern | Judgment |
|---------|----------|
| Same if-else pattern in 3+ places | Abstract with polymorphism → REJECT |
| switch/case with 5+ branches | Consider Strategy/Map pattern |
| Flag arguments changing behavior | Split into separate functions → REJECT |
| Type-based branching (instanceof/typeof) | Replace with polymorphism → REJECT |
| Nested conditionals (3+ levels) | Early return or extract → REJECT |

**Abstraction Level Mismatch Detection:**

| Pattern | Problem | Fix |
|---------|---------|-----|
| Low-level details in high-level processing | Hard to read | Extract details to functions |
| Mixed abstraction levels in one function | Cognitive load | Align to same granularity |
| DB operations mixed with business logic | Responsibility violation | Separate to Repository layer |
| Config values mixed with processing logic | Hard to change | Externalize configuration |

**Good Abstraction Examples:**

```typescript
// Proliferating conditionals
function process(type: string) {
  if (type === 'A') { /* process A */ }
  else if (type === 'B') { /* process B */ }
  else if (type === 'C') { /* process C */ }
  // ...continues
}

// Abstract with Map pattern
const processors: Record<string, () => void> = {
  A: processA,
  B: processB,
  C: processC,
};
function process(type: string) {
  processors[type]?.();
}
```

```typescript
// Mixed abstraction levels
function createUser(data: UserData) {
  // High level: business logic
  validateUser(data);
  // Low level: DB operation details
  const conn = await pool.getConnection();
  await conn.query('INSERT INTO users...');
  conn.release();
}

// Aligned abstraction levels
function createUser(data: UserData) {
  validateUser(data);
  await userRepository.save(data);  // Details hidden
}
```

## Workaround Detection

Don't overlook compromises made to "just make it work."

| Pattern | Example |
|---------|---------|
| Unnecessary package additions | Mystery libraries added just to make things work |
| Test deletion/skipping | `@Disabled`, `.skip()`, commented out |
| Empty implementations/stubs | `return null`, `// TODO: implement`, `pass` |
| Mock data in production | Hardcoded dummy data |
| Swallowed errors | Empty `catch {}`, `rescue nil` |
| Magic numbers | Unexplained `if (status == 3)` |

## Strict TODO Comment Prohibition

"We'll do it later" never gets done. What's not done now is never done.

TODO comments are immediate REJECT.

```kotlin
// REJECT - Future-looking TODO
// TODO: Add authorization check by facility ID
fun deleteCustomHoliday(@PathVariable id: String) {
    deleteCustomHolidayInputPort.execute(input)
}

// APPROVE - Implement now
fun deleteCustomHoliday(@PathVariable id: String) {
    val currentUserFacilityId = getCurrentUserFacilityId()
    val holiday = findHolidayById(id)
    require(holiday.facilityId == currentUserFacilityId) {
        "Cannot delete holiday from another facility"
    }
    deleteCustomHolidayInputPort.execute(input)
}
```

Only acceptable TODO cases:

| Condition | Example | Judgment |
|-----------|---------|----------|
| External dependency prevents implementation + Issued | `// TODO(#123): Implement after API key obtained` | Acceptable |
| Technical constraint prevents + Issued | `// TODO(#456): Waiting for library bug fix` | Acceptable |
| "Future implementation", "add later" | `// TODO: Add validation` | REJECT |
| "No time for now" | `// TODO: Refactor` | REJECT |

Correct handling:
- Needed now → Implement now
- Not needed now → Delete the code
- External blocker → Create issue and include ticket number in comment

## DRY Violation Detection

Detect duplicate code.

| Pattern | Judgment |
|---------|----------|
| Same logic in 3+ places | Immediate REJECT - Extract to function/method |
| Same validation in 2+ places | Immediate REJECT - Extract to validator function |
| Similar components 3+ | Immediate REJECT - Create shared component |
| Copy-paste derived code | Immediate REJECT - Parameterize or abstract |

AHA principle (Avoid Hasty Abstractions) balance:
- 2 duplications → Wait and see
- 3 duplications → Extract immediately
- Different domain duplications → Don't abstract (e.g., customer validation vs admin validation are different)

## Spec Compliance Verification

Verify that changes comply with the project's documented specifications.

Verification targets:

| Target | What to Check |
|--------|---------------|
| CLAUDE.md / README.md | Conforms to schema definitions, design principles, constraints |
| Type definitions / Zod schemas | New fields reflected in schemas |
| YAML/JSON config files | Follows documented format |

Specific checks:

1. When config files (YAML, etc.) are modified or added:
   - Cross-reference with schema definitions in CLAUDE.md, etc.
   - No ignored or invalid fields present
   - No required fields missing

2. When type definitions or interfaces are modified:
   - Documentation schema descriptions are updated
   - Existing config files are compatible with new schema

REJECT when these patterns are found:

| Pattern | Problem |
|---------|---------|
| Fields not in the spec | Ignored or unexpected behavior |
| Invalid values per spec | Runtime error or silently ignored |
| Violation of documented constraints | Against design intent |

## Call Chain Verification

When new parameters/fields are added, verify not just the changed file but also callers.

Verification steps:
1. When finding new optional parameters or interface fields, `Grep` all callers
2. Check if all callers pass the new parameter
3. If fallback value (`?? default`) exists, verify if fallback is used as intended

Danger patterns:

| Pattern | Problem | Detection |
|---------|---------|-----------|
| `options.xxx ?? fallback` where all callers omit `xxx` | Feature implemented but always falls back | grep callers |
| Tests set values directly with mocks | Don't go through actual call chain | Check test construction |
| `executeXxx()` doesn't receive `options` it uses internally | No route to pass value from above | Check function signature |

```typescript
// Missing wiring: No route to receive projectCwd
export async function executePiece(config, cwd, task) {
  const engine = new PieceEngine(config, cwd, task);  // No options
}

// Wired: Can pass projectCwd
export async function executePiece(config, cwd, task, options?) {
  const engine = new PieceEngine(config, cwd, task, options);
}
```

Logically dead code due to caller constraints:

Call chain verification applies not only to "missing wiring" but also to the reverse — unnecessary guards for conditions that callers already guarantee.

| Pattern | Problem | Detection |
|---------|---------|-----------|
| TTY check when all callers require TTY | Unreachable branch remains | grep all callers' preconditions |
| Null guard when callers already check null | Redundant defense | Trace caller constraints |
| Runtime type check when TypeScript types constrain | Not trusting type safety | Check TypeScript type constraints |

Verification steps:
1. When finding defensive branches (TTY check, null guard, etc.), grep all callers
2. If all callers already guarantee the condition, guard is unnecessary → REJECT
3. If some callers don't guarantee it, keep the guard

## Quality Attributes

| Attribute | Review Point |
|-----------|--------------|
| Scalability | Design handles increased load |
| Maintainability | Easy to modify and fix |
| Observability | Logging and monitoring enabled |

## Big Picture

Don't get lost in minor "clean code" nitpicks.

Verify:
- How will this code evolve in the future
- Is scaling considered
- Is technical debt being created
- Does it align with business requirements
- Is naming consistent with the domain

## Change Scope Assessment

Check change scope and include in report (non-blocking).

| Scope Size | Lines Changed | Action |
|------------|---------------|--------|
| Small | ~200 lines | Review as-is |
| Medium | 200-500 lines | Review as-is |
| Large | 500+ lines | Continue review. Suggest splitting if possible |

Note: Some tasks require large changes. Don't REJECT based on line count alone.

Verify:
- Changes are logically cohesive (no unrelated changes mixed in)
- Coder's scope declaration matches actual changes

Include as suggestions (non-blocking):
- If splittable, present splitting proposal