pattern-critic skill

---
name: pattern-critic
description: Critic agent that reviews pattern code for violations of documented rules, gotchas, and anti-patterns. Produces categorized checklist output with [PASS]/[FAIL] for each rule.
---

# Pattern Critic

Systematically review pattern code for violations of Common Tools documentation rules and gotchas.

## Workflow

1. Read the pattern file to review
2. Check each category below against the code
3. Output results in the checklist format (see Output Format)
4. For any [FAIL], include the line number and fix

## Violation Categories

### 1. Module Scope Violations

Check that these are NOT inside the pattern body:

| Violation | Fix |
|-----------|-----|
| `handler()` defined inside pattern | Move to module scope, or use `action()` instead |
| `lift()` immediately invoked (`lift(...)(args)`) | Use `computed()` or define lift at module scope |
| Helper functions defined inside pattern | Move to module scope |

**Allowed inside patterns:** `computed()`, `action()`, `.map()` callbacks, JSX event handlers.

### 2. Reactivity Violations

| Violation | Fix |
|-----------|-----|
| `[NAME]: someProp` (reactive value) | `[NAME]: computed(() => someProp)` |
| `[NAME]: \`text ${someProp}\`` | `[NAME]: computed(() => \`text ${someProp}\`)` |
| `Writable.of(reactiveValue)` | Initialize empty, set in handler/action |
| `.get()` on computed/lift result | Access directly (only Writable has .get()) |
| `items.filter(...)` inline in JSX | Wrap in `computed()` outside JSX |
| `items.sort(...)` inline in JSX | Wrap in `computed()` outside JSX |
| Nested computed with outer scope vars | Pre-compute with lift or outer computed |
| lift() closing over reactive deps | Pass deps as explicit params |
| Cells from composed patterns in ifElse | Wrap in local `computed()` |

### 3. Conditional Rendering

| Violation | Fix |
|-----------|-----|
| `onClick` inside `computed()` | Move button outside, use `disabled` attr |

**Note:** Ternaries work fine in JSX - the transformer auto-converts them to `ifElse()`. Both `{show ? <Element /> : null}` and `{ifElse(show, ...)}` are valid.

### 4. Type System

| Violation | Fix |
|-----------|-----|
| Array without `Default<T[], []>` | Add default to prevent undefined |
| Missing `Writable<>` for `.set()`/`.push()` | Add `Writable<T>` to input type |
| `Map` or `Set` in cell data | Use plain objects/arrays (serialization) |
| Custom `id` property for identity | Use `equals()` function instead |

### 5. Binding

| Violation | Fix |
|-----------|-----|
| `checked={item.done}` | `$checked={item.done}` (add $ prefix) |
| `value={title}` | `$value={title}` (add $ prefix) |
| `$checked={item}` (whole item) | `$checked={item.done}` (bind property) |
| Wrong event name | Use `onct-send`, `onct-input`, `onct-change` |

### 6. Style Syntax

| Element Type | Required Syntax |
|--------------|-----------------|
| HTML (`div`, `span`) | Object: `style={{ backgroundColor: "#fff" }}` |
| Custom (`ct-*`) | String: `style="background-color: #fff;"` |

| Violation | Fix |
|-----------|-----|
| String style on HTML | Convert to object syntax |
| Object style on ct-* | Convert to string syntax |
| kebab-case props on ct-* | Use camelCase: `allowCustom` not `allow-custom` |

### 7. Handler Binding

| Violation | Fix |
|-----------|-----|
| `onClick={addItem({ title: "x", items })}` | Event data comes at runtime, bind state only |
| Creating handlers inside `.map()` | Create handler once at module/pattern scope |

### 8. Stream/Async

| Violation | Fix |
|-----------|-----|
| `Stream.of()` | Doesn't exist. Bound handler IS the stream |
| `.subscribe()` on stream | Doesn't exist. Return stream from pattern |
| `async/await` in handlers | Use `fetchData()` (blocks UI otherwise) |
| `await generateText(...)` | Reactive, not a promise. Use `.result` |
| `await generateObject(...)` | Reactive, not a promise. Use `.result` |

### 9. LLM Integration

| Violation | Fix |
|-----------|-----|
| Array as root schema for generateObject | Wrap in object: `{ items: T[] }` |
| Missing `/// <cts-enable />` directive | Add at top of file |
| Prompt derived from agent-written cells | Causes infinite loop. Use separate cells |
| Invalid model name format | Use `vendor:model` (e.g., `anthropic:claude-sonnet-4-5`) |

### 10. Performance

| Violation | Fix |
|-----------|-----|
| Handler created per-item in `.map()` | Create handler once, bind with item |
| Expensive computation inside loop | Pre-compute outside, reference result |

### 11. Design Review

Check the domain model quality:

| Check | What to look for |
|-------|------------------|
| Clear entity boundaries | Each pattern represents one concept (Card, Column, Board) |
| Actions match user intent | Handler names reflect what user wants (addCard, moveCard, removeCard) |
| Unidirectional data flow | Parent owns state, children receive props |
| Normalized state | No duplicate data, single source of truth |
| Self-documenting types | Type names and fields are clear without comments |
| Appropriate granularity | Not too fine (trivial patterns) or too coarse (god patterns) |

### 12. Regression Check (for updates only)

When reviewing changes to existing code:

| Check | What to verify |
|-------|----------------|
| Tests still pass | Run existing tests after changes |
| Type signatures preserved | Or intentionally changed with migration path |
| Handlers still work | Existing functionality not broken |
| No unintended side effects | Changes scoped to intended area |

## Output Format

```
## Pattern Review: [filename]

### 1. Module Scope
- [PASS] No handler() inside pattern
- [FAIL] lift() immediately invoked (line 23)
  Fix: Use computed() or move lift to module scope

### 2. Reactivity
- [PASS] [NAME] properly wrapped
- [FAIL] Writable.of(deck.name) uses reactive value (line 15)
  Fix: Initialize empty, set in action()

### 3. Conditional Rendering
- [PASS] Using ifElse() correctly
- [N/A] No conditional rendering

[...continue for all categories...]

### 11. Design Review
- [PASS] Clear entity boundaries
- [WARN] Handler names could be clearer (moveCard vs reorderCard)
- [PASS] Unidirectional data flow

### 12. Regression Check (if updating)
- [PASS] Existing tests pass
- [N/A] No type signature changes

## Summary
- Passed: 22
- Failed: 3
- Warnings: 1
- N/A: 2

## Priority Fixes
1. [Line 15] Writable.of() with reactive value
2. [Line 23] lift() inside pattern
3. [Line 45] Missing $ prefix on binding
```

## Documentation References

- `docs/development/debugging/README.md` - Error reference table
- `docs/development/debugging/gotchas/` - Individual gotcha files
- `docs/common/components/COMPONENTS.md` - UI components and binding
- `docs/common/capabilities/llm.md` - LLM integration

## Quick Patterns

### Correct Module-Scope Handler
```typescript
const addItem = handler<{ title: string }, { items: Writable<Item[]> }>(
  ({ title }, { items }) => items.push({ title })
);

export default pattern<Input, Input>(({ items }) => ({
  [UI]: <ct-button onClick={addItem({ items })}>Add</ct-button>,
  items,
}));
```

### Correct Reactive [NAME]
```typescript
export default pattern<Input>(({ deck }) => ({
  [NAME]: computed(() => `Study: ${deck.name}`),
  // ...
}));
```

### Correct Conditional Rendering
```typescript
// Both are valid - ternaries auto-transform to ifElse()
{showDetails ? <div>Details content</div> : null}
{ifElse(showDetails, <div>Details content</div>, null)}
```

### Correct Style Syntax
```typescript
<div style={{ display: "flex", gap: "1rem" }}>
  <ct-vstack style="flex: 1; padding: 1rem;">
    Content
  </ct-vstack>
</div>
```