# Architecture Reviewer — Smell Baseline

Adapted from [mattpocock/skills#394](https://github.com/mattpocock/skills/pull/394) (Martin Fowler's "Bad Smells in Code", *Refactoring* ch.3) with three vexjoy modifications established by A/B test:

1. **Named language-idiom counter-examples come first** (per-language Go/TS/Python list of patterns that are NOT smells, even though the smell taxonomy might suggest they are).
2. **Severity cap.** Baseline smells default to LOW, may rise to MEDIUM if compounded with a real defect, **never** HIGH/CRITICAL from a smell alone.
3. **Lower-signal OO smells demoted** with language caveats (Middle Man, Refused Bequest, Feature Envy, Message Chains).

A/B context: the verbatim PR #394 baseline produced 7 trap false positives and 24 invented findings across 6 setups; this modified version produced 3 trap false positives and 11 invented findings on the same setups (B′ won 6/6). Setup-level results: `/tmp/ab394/` artifacts; full transcript at `tasks/w609k70v1.output`.

---

## The brief (pass verbatim to the Architecture reviewer)

> Smell baseline (always-on, language-idiom-overridden). On top of your language's idiomatic standards, carry this curated Fowler-smell baseline. It applies even when nothing else flags the diff.
>
> **Two binding rules — read these BEFORE applying any smell:**
>
> ### Rule 1: Language idioms override. Always check the counter-examples first.
>
> A smell label is **suppressed** when the code matches an idiomatic pattern. The following counter-examples are NOT smells — flagging them is a review defect:
>
> **Go:**
> - `switch v := x.(type)` (type switch on an interface) — NOT *Repeated Switches*. This is the idiomatic Go dispatch mechanism. Suppress.
> - Single interface in a package with one current implementation — NOT *Speculative Generality* if there's a test double, a mock, or a second impl is announced. Otherwise flag at LOW.
> - `default:` case that logs/skips on unknown type — NOT a "silent failure." This is the standard defensive `%T` pattern.
>
> **TypeScript:**
> - `switch` on a discriminated-union `kind` field — NOT *Repeated Switches*. This is idiomatic TS exhaustiveness. Suppress.
> - Chained `.map().filter().reduce()` on arrays — NOT *Message Chains*. This is functional composition, not navigation. Suppress.
> - `string` literal union types (`'card' | 'bank' | 'wallet'`) — NOT *Primitive Obsession*. The compiler enforces the domain. Suppress.
>
> **Python:**
> - `if/elif` on enum members at a single call site — NOT *Repeated Switches*. The smell needs the same cascade **at two or more sites**. Suppress when there's exactly one site.
> - ABCs (`abc.ABC`) with one concrete implementation and one current caller — NOT *Speculative Generality*. Suppress.
> - `dict[str, Any]` for genuinely heterogeneous payloads (event buses, plugin metadata) — NOT *Primitive Obsession* by default. Flag only at LOW if a clearer type is obvious.
>
> When in doubt about whether the code matches an idiom, **err toward suppression and don't flag the smell at all**. A missed smell is recoverable; a false-positive smell teaches the reader to ignore the reviewer.
>
> ### Rule 2: Severity cap. Baseline smells default to LOW. They may rise to MEDIUM only when they compound with an idiomatic or correctness violation. Never HIGH or CRITICAL from a smell alone.
>
> A real bug found while reading a smelly area gets HIGH/CRITICAL on its own merits as a normal Architecture finding — not as a smell.
>
> ---
>
> ## The 12 smells (each: *what it is* → *how to fix*)
>
> **High-signal (apply first):**
>
> - **Mysterious Name** — a function, variable, or type whose name doesn't reveal what it does or holds. → rename it; if no honest name comes, the design's murky.
> - **Duplicated Code** — the same logic shape appears in more than one hunk or file in the change. → extract the shared shape, call it from both.
> - **Data Clumps** — the same few fields or params keep travelling together (a type wanting to be born). → bundle them into one type, pass that.
> - **Primitive Obsession** — a primitive standing in for a domain concept that deserves its own type. → give the concept its own small type. *(See Rule 1 for TS union-type and Python `dict[str, Any]` exceptions.)*
> - **Repeated Switches** — the same `switch`/`if`-cascade on the same type recurs at **two or more sites** in the diff. → replace with polymorphism, or one map both sites share. *(See Rule 1 for Go type-switch, TS discriminated-union, and Python single-site exceptions.)*
> - **Speculative Generality** — abstraction, parameters, or hooks added for needs the spec doesn't have **and no current caller uses**. → delete it; inline back until a real need shows.
> - **Shotgun Surgery** — one logical change forces scattered edits across many files in the diff. → gather what changes together into one module.
> - **Divergent Change** — one file or module is edited for several unrelated reasons. → split so each module changes for one reason.
>
> **Lower-signal (apply only when very confident; OO-heavy):**
>
> - **Feature Envy** — a method that reaches into another object's data more than its own. → move the method onto the data it envies.
> - **Message Chains** — long `a.b.c.d` *property navigation* through nested data (NOT method chains on collections). → hide the walk behind one method on the first object.
> - **Middle Man** — a class or function that mostly just delegates onward with no added behavior. → cut it, call the real target direct. *(Lower priority in Go: thin wrappers for interface satisfaction are idiomatic.)*
> - **Refused Bequest** — a subclass or implementer that ignores or overrides most of what it inherits. → drop the inheritance, use composition. *(Lower priority in Go: no inheritance.)*
>
> ---
>
> ## Output format (for Architecture findings)
>
> - `[Architecture] <one-line description> — file:line`
> - If a baseline smell, prefix with the label: `[Architecture] possible <Smell Name>: <one-line> — file:line`
> - Severity: CRITICAL / HIGH / MEDIUM / LOW (baseline smells default to LOW, MEDIUM if compounded — never HIGH/CRITICAL from a smell alone)
>
> ## Rules
>
> - Read-only — observe and report, never modify.
> - Skip anything tooling (linter, formatter, type-checker) already enforces.
> - Distinguish hard violations from judgement calls. Baseline smells are always judgement calls.
> - Cite `file:line` for every finding.
>
> **Re-read Rule 1 before reporting any smell-labeled finding.**

---

## Maintenance

When adding a new language-idiom counter-example: state the exact pattern, name the smell it would have triggered, and end with "Suppress." (or, for borderline cases, "Flag at LOW only when..."). Vague guidance ("be careful with…") is not a suppression rule and will not change the model's behavior — the A/B showed that named, code-shaped counter-examples did the work.

# Audit Playbook

Structured checklists for full-repo review. Each category lists specific patterns to look for and the evidence required to confirm a finding. Findings without evidence are speculation, not findings.

Uses `score-component.py` grades as structural triage context. A score alone never determines severity: each finding still needs file-and-line evidence and the category's severity guide. Each category names a primary reviewer role; overlap is intentional — independent confirmation strengthens findings. See "Reviewer Role Cross-Reference" for how roles map to `parallel-code-review` and to comprehensive-review wave lenses.

---

## How to Use

The orchestrator loads this playbook and pastes the relevant category checklists — plus each component's `score-component.py` score and grade — into every review agent's prompt (agents run in fresh context and cannot load it themselves). Each agent then, for each category it owns:

1. Check every pattern in the "Look for" list against the file under review.
2. Record a finding only when evidence exists (file path, line number, concrete observation).
3. Assign severity per the category's severity guide.
4. Cross-reference the `score-component.py` scores in the prompt — structural issues the script flagged need confirmation, not automatic escalation.

---

## Category 1: Correctness

**Primary reviewer**: Business Logic

**Look for**:

| Pattern | Evidence required |
|---|---|
| Off-by-one in loops, slices, ranges | Line number + boundary values that trigger the error |
| Nil/null dereference on optional returns | Call site that skips the nil check + the function signature |
| Race condition on shared state | Two goroutines/threads accessing the same variable without synchronization; name both access points |
| Unchecked error return | `file:line` where error is discarded + what failure it masks |
| Type coercion that loses precision | The two types involved + a value that would be truncated |
| Dead code path (unreachable branch) | The condition that is always true/false + why |
| Incorrect boolean logic (swapped AND/OR, missing negation) | The expression + an input that produces wrong output |

**Severity guide**: Data loss or silent wrong answer = CRITICAL. Logic error with limited blast radius = HIGH. Unreachable code = MEDIUM.

---

## Category 2: Security

**Primary reviewer**: Security

**Defer to the `security` skill for comprehensive security audits.** This category catches surface-level security issues during a repo-wide sweep; it does not replace the dedicated 25-pattern regex scanner + 40-class LLM taxonomy in `security`.

**Look for**:

| Pattern | Evidence required |
|---|---|
| Hardcoded secrets, API keys, tokens | `file:line` + the literal or pattern matched |
| SQL/command injection (unsanitized input in query/exec) | The input source + the query/command it flows into |
| Missing authentication on a route/endpoint | The route definition + absence of auth middleware |
| Overly broad file permissions (world-writable, 777) | The chmod/permission call + the file it affects |
| Secrets in logs, error messages, or client responses | `file:line` + the variable being logged |

**Severity guide**: Exploitable vulnerability = CRITICAL. Missing defense-in-depth = HIGH. Hardening opportunity = MEDIUM.

**Boundary**: Findings requiring deep threat modeling (attack trees, trust boundary analysis, cryptographic protocol review) belong in `security`, not here.

---

## Category 3: Performance

**Primary reviewer**: Architecture

**Look for**:

| Pattern | Evidence required |
|---|---|
| N+1 query pattern (loop issuing one query per item) | The loop + the query inside it + the collection size if known |
| Unbounded collection growth (append without cap or eviction) | The data structure + the growth path + absence of bounds |
| Blocking call on hot path (synchronous I/O in request handler) | The call + the path it blocks |
| Redundant computation (same value computed multiple times) | Both computation sites + the shared input |
| Missing index on frequently queried column | The query + the table + absence of index in schema |
| Large allocation in tight loop | The allocation + loop bounds |

**Severity guide**: Measurable latency/memory impact on production path = HIGH. Theoretical concern without load evidence = MEDIUM. Micro-optimization = LOW.

---

## Category 4: Test Coverage

**Primary reviewer**: Business Logic

**Look for**:

| Pattern | Evidence required |
|---|---|
| Public function with zero test coverage | Function signature + grep confirming no test calls it |
| Test that asserts nothing (no assertion, only prints) | `file:line` of the test + absence of assert/expect |
| Test that mocks the thing it tests (tautological) | The mock setup + the assertion that checks the mock |
| Missing edge-case test (boundary values, empty inputs, errors) | The function's contract + the untested boundary |
| Flaky test indicators (sleep, time-dependent, order-dependent) | The timing call or shared state + the test name |
| Test file without corresponding source file (orphaned) | The test path + the missing source path |

**Severity guide**: Core business logic untested = HIGH. Utility/helper untested = MEDIUM. Missing edge case on tested function = LOW.

---

## Category 5: Tech Debt

**Primary reviewer**: Architecture

**Look for**:

| Pattern | Evidence required |
|---|---|
| TODO/FIXME/HACK comments older than 6 months | `file:line` + `git log` date of the comment |
| Duplicated logic across 3+ files | The duplicated block + all file locations |
| Deprecated API usage (language or library) | The deprecated call + the recommended replacement |
| Inconsistent patterns for the same operation | Two+ examples of the same operation done differently |
| Overly complex function (cyclomatic complexity, deep nesting) | The function + nesting depth or branch count |
| Dead dependencies (imported but unused) | The import + grep confirming no usage |

**Severity guide**: Deprecated API with removal timeline = HIGH. Duplication causing maintenance risk = MEDIUM. Cosmetic inconsistency = LOW.

---

## Category 6: Dependencies

**Primary reviewer**: Security

**Look for**:

| Pattern | Evidence required |
|---|---|
| Known vulnerability in pinned version | The package + version + CVE or advisory ID |
| Unpinned dependency (floating version) | The dependency spec + the lockfile state |
| Abandoned dependency (no commits in 12+ months) | The package name + last commit date |
| Unnecessary dependency (functionality available in stdlib) | The import + the stdlib equivalent |
| Version conflict between direct and transitive deps | The two version specs + the conflict |
| License incompatibility | The dependency license + the project license |

**Severity guide**: Known CVE = CRITICAL. Abandoned with no alternative = HIGH. Unpinned = MEDIUM. Unnecessary = LOW.

---

## Category 7: Developer Experience

**Primary reviewer**: Architecture

**Look for**:

| Pattern | Evidence required |
|---|---|
| Missing or wrong setup instructions (README, CONTRIBUTING) | The instruction + what actually happens when followed |
| Unclear error messages (codes without context, raw stack traces) | The error output + what the user needs to know instead |
| Inconsistent naming (mixed camelCase/snake_case in same layer) | Two+ examples from the same package/module |
| Missing type annotations on public interfaces | The function signature + the language's annotation convention |
| Complex build/run steps not scripted | The manual steps required + absence of script |
| Missing CLAUDE.md or stale project conventions | The convention + evidence it no longer matches code |

**Severity guide**: Setup instructions that fail = HIGH. Naming inconsistency = MEDIUM. Missing optional annotations = LOW.

---

## Category 8: Documentation

**Primary reviewer**: Business Logic

**Look for**:

| Pattern | Evidence required |
|---|---|
| Stale docstring (describes old behavior) | The docstring + the current code that contradicts it |
| Missing docstring on public API | The function/class + its public visibility |
| README that does not match current project state | The README claim + the actual state |
| Commented-out code without explanation | `file:line` + absence of explanatory comment |
| Architecture doc that references deleted components | The reference + the missing component |

**Severity guide**: Actively misleading docs = HIGH. Missing docs on public API = MEDIUM. Missing docs on internal code = LOW.

---

## Using score-component.py

Grade bands come from `total / max_total` (A 90-100, B 75-89, C 60-74, D
40-59, F 0-39). Record them in the deterministic-health table and use lower
scores to prioritize inspection. **A score alone never determines severity.**

Wave agents use scores to avoid duplicate work: if `score-component.py` flags
a structural issue, the reviewer confirms the specific condition and cites the
line. The category severity guide, user impact, and evidence determine the
report tier.

---

## Reviewer Role Cross-Reference

When review runs via `parallel-code-review`, its three roles map to audit categories:

| Reviewer | Primary categories | Secondary (cross-check) |
|---|---|---|
| Security | Security, Dependencies | Correctness (injection paths) |
| Business Logic | Correctness, Test Coverage, Documentation | Tech Debt (stale TODOs) |
| Architecture | Performance, Tech Debt, Developer Experience | Test Coverage (structural gaps) |

When review runs via comprehensive-review waves (the full-repo-review path), the orchestrator maps categories to wave lenses:

| Wave lens | Categories |
|---|---|
| security | Security, Dependencies |
| business-logic, silent-failures | Correctness |
| test-analyzer | Test Coverage |
| quality, type-design, language-specialist | Tech Debt |
| comment-analyzer, docs-validator | Documentation |
| newcomer, docs-validator | Developer Experience |
| architecture reviewer | Performance, Tech Debt |
| Wave 2 deep-dive agents | Re-check the same categories as their Wave 1 counterpart lens, at depth |
| Wave 3 adversarial agents | Challenge weak evidence, false consensus, missed user impact, and low-value recommendations across all categories |

Each reviewer covers its primary categories exhaustively and its secondary categories opportunistically. This division prevents both gaps and redundant deep-dives.

# Go-Specific Review Patterns

When reviewing Go code, watch for these patterns that linters miss:

## Type Export Design
- [ ] Are implementation types unnecessarily exported?
- [ ] Should types be unexported with only constructors exported?
- **Red flag**: `type FooStore struct{}` exported but only implements an interface

## Concurrency Patterns
- [ ] Does batch+callback pattern protect against concurrent writes?
- [ ] Does `commit()` only remove specific items, not clear all?
- [ ] Are loop variables using outdated patterns? (Go 1.22+ doesn't need cloning)
  - [ ] No `i := i` reassignment inside loops
  - [ ] No closure arguments for loop variables: `go func(id int) { }(i)`
- **Red flag**: `s.events = nil` in commit callback
- **Red flag**: `go func(x int) { ... }(loopVar)` - closure argument unnecessary since Go 1.22

## Resource Management
- [ ] Is `defer f.Close()` placed AFTER error check?
- [ ] Are database connection pools shared, not duplicated?
- [ ] Is file traversal done once, not repeated for size calculation?
- **Red flag**: `defer f.Close()` immediately after `os.OpenFile()`

## Metrics & Observability
- [ ] Are Prometheus counter metrics pre-initialized with `.Add(0)`?
- [ ] Are all known label combinations initialized at startup?
- **Red flag**: CounterVec registered but not initialized

## Testing Patterns
- [ ] Are interface implementation tests deduplicated?
- [ ] Do tests use `assert.Equal` (no reflection) for comparable types?
- [ ] Does test setup use `prometheus.NewPedanticRegistry()`?
- **Red flag**: Copy-pasted tests for FileStore, MemoryStore, SQLStore

## Code Organization
- [ ] Is function extraction justified (reuse or complexity hiding)?
- [ ] Are unnecessary helper functions wrapping stdlib calls?
- **Red flag**: Helper that just calls through to another function

---

# Organization Library Ecosystem Patterns

When reviewing projects that use shared organization libraries, apply these additional checks:

## Library Usage
- [ ] Are optional fields using the organization's preferred option type?
- [ ] Is SQL iteration using helper functions instead of manual `rows.Next()` loops?
- [ ] Are tests using the organization's assertion helpers?
- **Red flag**: Manual SQL row iteration with defer/Next/Scan/Err pattern when helpers exist

## Test Assertions
- [ ] Is the correct assertion function used for the type being compared?
- [ ] Is deep comparison only used for non-comparable types (slices, maps, structs)?
- **Red flag**: Deep comparison used for simple types like int, string, bool

## Test Infrastructure
- [ ] Are DB tests using the organization's test database helpers?
- [ ] Are Prometheus tests using `NewPedanticRegistry()`?
- **Red flag**: Raw `sql.Open()` in test setup instead of test helpers

## Dead Code
- [ ] Are there leftover `*_migration.sql` files without usage?
- [ ] Are there helper functions that just wrap single stdlib calls?
- [ ] Are there redundant checks (e.g., empty string check before regex)?
- **Red flag**: Wrapper functions that add no value over the underlying call

## Database Naming
- [ ] Do functions using database-specific syntax indicate this in names?
- **Red flag**: Generic `SQLStoreFactory` that uses database-specific syntax

# Receiving Review Feedback

When YOU are the one receiving code review feedback (not giving it), apply these patterns:

## The Reception Pattern

```
WHEN receiving code review feedback:

1. READ: Complete feedback without reacting
2. UNDERSTAND: Restate requirement in own words (or ask)
3. VERIFY: Check against codebase reality
4. EVALUATE: Technically sound for THIS codebase?
5. RESPOND: Technical acknowledgment or reasoned pushback
6. IMPLEMENT: One item at a time, test each
```

## No Performative Agreement

**Replace performative responses with action:**
- Restate the technical requirement
- Ask clarifying questions
- Push back with technical reasoning when feedback is incorrect
- Start working immediately (actions > words)

**Skip these filler phrases:** "You're absolutely right!", "Great point!", "Excellent feedback!", "Thanks for catching that!"

**When feedback IS correct:**
```
"Fixed. [Brief description of what changed]"
"Good catch - [specific issue]. Fixed in [location]."
[Just fix it and show in the code]
```

## YAGNI Check for "Professional" Features

```
IF reviewer suggests "implementing properly":
  grep codebase for actual usage

  IF unused: "This endpoint isn't called. Remove it (YAGNI)?"
  IF used: Then implement properly
```

## Handling Unclear Feedback

```
IF any item is unclear:
  STOP - do not implement anything yet
  ASK for clarification on unclear items

WHY: Items may be related. Partial understanding = wrong implementation.
```

**Example:**
```
Reviewer: "Fix items 1-6"
You understand 1,2,3,6. Unclear on 4,5.

WRONG: Implement 1,2,3,6 now, ask about 4,5 later
RIGHT: "I understand items 1,2,3,6. Need clarification on 4 and 5 before proceeding."
```

## When to Push Back

Push back when:
- Suggestion breaks existing functionality
- Reviewer lacks full context
- Violates YAGNI (unused feature)
- Technically incorrect for this stack
- Legacy/compatibility reasons exist

**How to push back:**
- Use technical reasoning, not defensiveness
- Ask specific questions
- Reference working tests/code

**Example:**
```
Reviewer: "Remove legacy code"
WRONG: "You're absolutely right! Let me remove that..."
RIGHT: "Checking... build target is 10.15+, this API needs 13+. Need legacy for backward compat. Fix bundle ID or drop pre-13 support?"
```

## Implementation Order

```
FOR multi-item feedback:
  1. Clarify anything unclear FIRST
  2. Then implement in this order:
     - Blocking issues (breaks, security)
     - Simple fixes (typos, imports)
     - Complex fixes (refactoring, logic)
  3. Test each fix individually
  4. Verify no regressions
```

## External vs Internal Reviewers

**From external reviewers:**
```
BEFORE implementing:
  1. Check: Technically correct for THIS codebase?
  2. Check: Breaks existing functionality?
  3. Check: Reason for current implementation?
  4. Check: Does reviewer understand full context?

IF suggestion seems wrong:
  Push back with technical reasoning

IF can't easily verify:
  Say so: "I can't verify this without [X]. Should I investigate/proceed?"
```

# Full-Repo Review Report Template

Use this template when generating `full-repo-review-report.md`.

---

```markdown
# Full-Repo Review Report

**Date**: YYYY-MM-DD
**Files reviewed**: N
**Total findings**: N (Critical: N, High: N, Medium: N, Low: N)

## Deterministic Health Scores

Scores from `score-component.py --all-agents --all-skills`.

| Component | Score | Grade | Key Issues |
|-----------|-------|-------|------------|
| example-agent | 68/90 | B | Missing workflow gate |

### Score Summary
- **A (90-100%)**: N components
- **B (75-89%)**: N components
- **C (60-74%)**: N components -- listed as HIGH findings below
- **D/F (<60%)**: N components -- listed as CRITICAL findings below

## Critical (fix immediately)

Security vulnerabilities, broken functionality, data loss risks.

- **{file}:{line}** -- [{category}] {description}
  - Fix: {suggested fix}
  - Source: {wave N agent / score-component.py}

## High (fix this sprint)

Significant quality issues, missing error handling, test gaps.

- **{file}:{line}** -- [{category}] {description}
  - Fix: {suggested fix}
  - Source: {wave N agent / score-component.py}

## Medium (fix when touching these files)

Style violations, naming drift, documentation gaps.

- **{file}:{line}** -- [{category}] {description}
  - Fix: {suggested fix}
  - Source: {wave N agent}

## Low (nice to have)

Minor improvements, optional enhancements.

- **{file}:{line}** -- [{category}] {description}
  - Fix: {suggested fix}
  - Source: {wave N agent}

## Systemic Patterns

Issues that appear across 3+ files. These are the highest-leverage fixes.

- **{pattern name}**: Seen in N files ({file1}, {file2}, ...). {description of the pattern}. Fix: {recommended approach}.

## Review Metadata

- **Waves executed**: 0, 1, 2, 3
- **Duration**: N minutes
- **Score pre-check**: pass / warn (script failed, review continued) / fail
- **comprehensive-review version**: N.N.N
- **Files by type**: N scripts, N hooks, N skills, N agents, N docs
```