refactor(skills): reframe testing-anti-patterns as writing-good-tests

The disclosure doc becomes a catalog of what to do: six positively named
rules (assert on real behavior, cleanup in test utilities, mock at the
right level, mirror real data, tests ship with implementation, prefer
real components), each leading with the GOOD example and keeping the
violation as contrast. Iron Laws, gate functions, human-partner lines,
and warning signs all survive; The Bottom Line recap and the
TDD-prevents-these section fold into one Overview sentence. SKILL.md's
pointer moves into the Good Tests section it belongs with. Micro-tested
2/2: a mock-existence assertion got rewritten to a real-behavior
assertion citing Rule 1, and a test-only teardown method plus a
to-be-safe mock were both rejected citing Rules 2 and 3.

skills/test-driven-development/SKILL.md

@@ -203,6 +203,11 @@ Next failing test for next feature.
 | **Clear** | Name describes behavior | `test('test1')` |
 | **Shows intent** | Demonstrates desired API | Obscures what code should do |
 
+When adding mocks or test utilities, read [writing-good-tests.md](writing-good-tests.md) for the rules that keep tests honest:
+- Assert on real behavior, never on mock behavior
+- Keep test-only code in test utilities, out of production classes
+- Understand a dependency's side effects before mocking it
+
 ## Why Order Matters
 
 **"I'll write tests after to verify it works"**
@@ -354,13 +359,6 @@ Bug found? Write failing test reproducing it. Follow TDD cycle. Test proves fix
 
 Never fix bugs without a test.
 
-## Testing Anti-Patterns
-
-When adding mocks or test utilities, read [testing-anti-patterns.md](testing-anti-patterns.md) to avoid common pitfalls:
-- Testing mock behavior instead of real behavior
-- Adding test-only methods to production classes
-- Mocking without understanding dependencies
-
 ## Final Rule
 
 ```

skills/test-driven-development/testing-anti-patterns.md

@@ -1,299 +0,0 @@
-# Testing Anti-Patterns
-
-**Load this reference when:** writing or changing tests, adding mocks, or tempted to add test-only methods to production code.
-
-## Overview
-
-Tests must verify real behavior, not mock behavior. Mocks are a means to isolate, not the thing being tested.
-
-**Core principle:** Test what the code does, not what the mocks do.
-
-**Following strict TDD prevents these anti-patterns.**
-
-## The Iron Laws
-
-```
-1. NEVER test mock behavior
-2. NEVER add test-only methods to production classes
-3. NEVER mock without understanding dependencies
-```
-
-## Anti-Pattern 1: Testing Mock Behavior
-
-**The violation:**
-```typescript
-// ❌ BAD: Testing that the mock exists
-test('renders sidebar', () => {
-  render(<Page />);
-  expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument();
-});
-```
-
-**Why this is wrong:**
-- You're verifying the mock works, not that the component works
-- Test passes when mock is present, fails when it's not
-- Tells you nothing about real behavior
-
-**your human partner's correction:** "Are we testing the behavior of a mock?"
-
-**The fix:**
-```typescript
-// ✅ GOOD: Test real component or don't mock it
-test('renders sidebar', () => {
-  render(<Page />);  // Don't mock sidebar
-  expect(screen.getByRole('navigation')).toBeInTheDocument();
-});
-
-// OR if sidebar must be mocked for isolation:
-// Don't assert on the mock - test Page's behavior with sidebar present
-```
-
-### Gate Function
-
-```
-BEFORE asserting on any mock element:
-  Ask: "Am I testing real component behavior or just mock existence?"
-
-  IF testing mock existence:
-    STOP - Delete the assertion or unmock the component
-
-  Test real behavior instead
-```
-
-## Anti-Pattern 2: Test-Only Methods in Production
-
-**The violation:**
-```typescript
-// ❌ BAD: destroy() only used in tests
-class Session {
-  async destroy() {  // Looks like production API!
-    await this._workspaceManager?.destroyWorkspace(this.id);
-    // ... cleanup
-  }
-}
-
-// In tests
-afterEach(() => session.destroy());
-```
-
-**Why this is wrong:**
-- Production class polluted with test-only code
-- Dangerous if accidentally called in production
-- Violates YAGNI and separation of concerns
-- Confuses object lifecycle with entity lifecycle
-
-**The fix:**
-```typescript
-// ✅ GOOD: Test utilities handle test cleanup
-// Session has no destroy() - it's stateless in production
-
-// In test-utils/
-export async function cleanupSession(session: Session) {
-  const workspace = session.getWorkspaceInfo();
-  if (workspace) {
-    await workspaceManager.destroyWorkspace(workspace.id);
-  }
-}
-
-// In tests
-afterEach(() => cleanupSession(session));
-```
-
-### Gate Function
-
-```
-BEFORE adding any method to production class:
-  Ask: "Is this only used by tests?"
-
-  IF yes:
-    STOP - Don't add it
-    Put it in test utilities instead
-
-  Ask: "Does this class own this resource's lifecycle?"
-
-  IF no:
-    STOP - Wrong class for this method
-```
-
-## Anti-Pattern 3: Mocking Without Understanding
-
-**The violation:**
-```typescript
-// ❌ BAD: Mock breaks test logic
-test('detects duplicate server', () => {
-  // Mock prevents config write that test depends on!
-  vi.mock('ToolCatalog', () => ({
-    discoverAndCacheTools: vi.fn().mockResolvedValue(undefined)
-  }));
-
-  await addServer(config);
-  await addServer(config);  // Should throw - but won't!
-});
-```
-
-**Why this is wrong:**
-- Mocked method had side effect test depended on (writing config)
-- Over-mocking to "be safe" breaks actual behavior
-- Test passes for wrong reason or fails mysteriously
-
-**The fix:**
-```typescript
-// ✅ GOOD: Mock at correct level
-test('detects duplicate server', () => {
-  // Mock the slow part, preserve behavior test needs
-  vi.mock('MCPServerManager'); // Just mock slow server startup
-
-  await addServer(config);  // Config written
-  await addServer(config);  // Duplicate detected ✓
-});
-```
-
-### Gate Function
-
-```
-BEFORE mocking any method:
-  STOP - Don't mock yet
-
-  1. Ask: "What side effects does the real method have?"
-  2. Ask: "Does this test depend on any of those side effects?"
-  3. Ask: "Do I fully understand what this test needs?"
-
-  IF depends on side effects:
-    Mock at lower level (the actual slow/external operation)
-    OR use test doubles that preserve necessary behavior
-    NOT the high-level method the test depends on
-
-  IF unsure what test depends on:
-    Run test with real implementation FIRST
-    Observe what actually needs to happen
-    THEN add minimal mocking at the right level
-
-  Red flags:
-    - "I'll mock this to be safe"
-    - "This might be slow, better mock it"
-    - Mocking without understanding the dependency chain
-```
-
-## Anti-Pattern 4: Incomplete Mocks
-
-**The violation:**
-```typescript
-// ❌ BAD: Partial mock - only fields you think you need
-const mockResponse = {
-  status: 'success',
-  data: { userId: '123', name: 'Alice' }
-  // Missing: metadata that downstream code uses
-};
-
-// Later: breaks when code accesses response.metadata.requestId
-```
-
-**Why this is wrong:**
-- **Partial mocks hide structural assumptions** - You only mocked fields you know about
-- **Downstream code may depend on fields you didn't include** - Silent failures
-- **Tests pass but integration fails** - Mock incomplete, real API complete
-- **False confidence** - Test proves nothing about real behavior
-
-**The Iron Rule:** Mock the COMPLETE data structure as it exists in reality, not just fields your immediate test uses.
-
-**The fix:**
-```typescript
-// ✅ GOOD: Mirror real API completeness
-const mockResponse = {
-  status: 'success',
-  data: { userId: '123', name: 'Alice' },
-  metadata: { requestId: 'req-789', timestamp: 1234567890 }
-  // All fields real API returns
-};
-```
-
-### Gate Function
-
-```
-BEFORE creating mock responses:
-  Check: "What fields does the real API response contain?"
-
-  Actions:
-    1. Examine actual API response from docs/examples
-    2. Include ALL fields system might consume downstream
-    3. Verify mock matches real response schema completely
-
-  Critical:
-    If you're creating a mock, you must understand the ENTIRE structure
-    Partial mocks fail silently when code depends on omitted fields
-
-  If uncertain: Include all documented fields
-```
-
-## Anti-Pattern 5: Integration Tests as Afterthought
-
-**The violation:**
-```
-✅ Implementation complete
-❌ No tests written
-"Ready for testing"
-```
-
-**Why this is wrong:**
-- Testing is part of implementation, not optional follow-up
-- TDD would have caught this
-- Can't claim complete without tests
-
-**The fix:**
-```
-TDD cycle:
-1. Write failing test
-2. Implement to pass
-3. Refactor
-4. THEN claim complete
-```
-
-## When Mocks Become Too Complex
-
-**Warning signs:**
-- Mock setup longer than test logic
-- Mocking everything to make test pass
-- Mocks missing methods real components have
-- Test breaks when mock changes
-
-**your human partner's question:** "Do we need to be using a mock here?"
-
-**Consider:** Integration tests with real components often simpler than complex mocks
-
-## TDD Prevents These Anti-Patterns
-
-**Why TDD helps:**
-1. **Write test first** → Forces you to think about what you're actually testing
-2. **Watch it fail** → Confirms test tests real behavior, not mocks
-3. **Minimal implementation** → No test-only methods creep in
-4. **Real dependencies** → You see what the test actually needs before mocking
-
-**If you're testing mock behavior, you violated TDD** - you added mocks without watching test fail against real code first.
-
-## Quick Reference
-
-| Anti-Pattern | Fix |
-|--------------|-----|
-| Assert on mock elements | Test real component or unmock it |
-| Test-only methods in production | Move to test utilities |
-| Mock without understanding | Understand dependencies first, mock minimally |
-| Incomplete mocks | Mirror real API completely |
-| Tests as afterthought | TDD - tests first |
-| Over-complex mocks | Consider integration tests |
-
-## Red Flags
-
-- Assertion checks for `*-mock` test IDs
-- Methods only called in test files
-- Mock setup is >50% of test
-- Test fails when you remove mock
-- Can't explain why mock is needed
-- Mocking "just to be safe"
-
-## The Bottom Line
-
-**Mocks are tools to isolate, not things to test.**
-
-If TDD reveals you're testing mock behavior, you've gone wrong.
-
-Fix: Test real behavior or question why you're mocking at all.

skills/test-driven-development/writing-good-tests.md

@@ -0,0 +1,248 @@
+# Writing Good Tests
+
+**Load this reference when:** writing or changing tests, adding mocks, or
+adding cleanup/helper methods for tests.
+
+## Overview
+
+Good tests verify real behavior. Mocks exist to isolate the code under
+test — they are never the thing being tested.
+
+**Core principle:** Test what the code does, not what the mocks do.
+
+Strict TDD produces every rule below naturally: a test written first and
+watched failing against real code only earns a mock when the real
+dependency proves slow or external. A test asserting on a mock means TDD
+was skipped somewhere.
+
+## The Iron Laws
+
+```
+1. Assert on real behavior, never on mock behavior
+2. Production classes carry production methods only
+3. Understand a dependency's side effects before mocking it
+```
+
+## Rule 1: Assert on Real Behavior
+
+```typescript
+// ✅ GOOD: Test the real component
+test('renders sidebar', () => {
+  render(<Page />);  // Sidebar unmocked
+  expect(screen.getByRole('navigation')).toBeInTheDocument();
+});
+```
+
+If the sidebar must be mocked for isolation, assert on Page's behavior
+with the sidebar present — the mock itself earns no assertions.
+
+```typescript
+// ❌ The violation: asserting that the mock exists
+test('renders sidebar', () => {
+  render(<Page />);
+  expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument();
+});
+```
+
+A mock assertion passes when the mock is present and fails when it is
+absent — it says nothing about the component. **your human partner's
+correction:** "Are we testing the behavior of a mock?"
+
+### Gate Function
+
+```
+BEFORE asserting on any mock element:
+  Ask: "Am I testing real component behavior or just mock existence?"
+
+  IF testing mock existence:
+    STOP - Delete the assertion or unmock the component
+
+  Test real behavior instead
+```
+
+## Rule 2: Keep Test Cleanup in Test Utilities
+
+```typescript
+// ✅ GOOD: Test utilities own test cleanup
+// Session has no destroy() - it's stateless in production
+
+// In test-utils/
+export async function cleanupSession(session: Session) {
+  const workspace = session.getWorkspaceInfo();
+  if (workspace) {
+    await workspaceManager.destroyWorkspace(workspace.id);
+  }
+}
+
+// In tests
+afterEach(() => cleanupSession(session));
+```
+
+```typescript
+// ❌ The violation: destroy() exists only for tests
+class Session {
+  async destroy() {  // Looks like production API!
+    await this._workspaceManager?.destroyWorkspace(this.id);
+    // ... cleanup
+  }
+}
+
+// In tests
+afterEach(() => session.destroy());
+```
+
+A test-only method pollutes the production class, is dangerous if
+production code ever calls it, and confuses object lifecycle with entity
+lifecycle.
+
+### Gate Function
+
+```
+BEFORE adding any method to a production class:
+  Ask: "Is this only used by tests?"
+
+  IF yes:
+    STOP - Put it in test utilities instead
+
+  Ask: "Does this class own this resource's lifecycle?"
+
+  IF no:
+    STOP - Wrong class for this method
+```
+
+## Rule 3: Mock at the Right Level
+
+Learn what the real method does — every side effect — before replacing
+it. Mock the slow or external operation and preserve the behavior your
+test depends on.
+
+```typescript
+// ✅ GOOD: Mock the slow part, preserve behavior the test needs
+test('detects duplicate server', () => {
+  vi.mock('MCPServerManager'); // Just mock slow server startup
+
+  await addServer(config);  // Config written
+  await addServer(config);  // Duplicate detected ✓
+});
+```
+
+```typescript
+// ❌ The violation: the mock swallows the side effect the test depends on
+test('detects duplicate server', () => {
+  // Mock prevents the config write that duplicate detection reads!
+  vi.mock('ToolCatalog', () => ({
+    discoverAndCacheTools: vi.fn().mockResolvedValue(undefined)
+  }));
+
+  await addServer(config);
+  await addServer(config);  // Should throw - but won't!
+});
+```
+
+### Gate Function
+
+```
+BEFORE mocking any method:
+  STOP - Understand before replacing
+
+  1. Ask: "What side effects does the real method have?"
+  2. Ask: "Does this test depend on any of those side effects?"
+  3. Ask: "Do I fully understand what this test needs?"
+
+  IF the test depends on side effects:
+    Mock at the lower level (the actual slow/external operation)
+    OR use test doubles that preserve the necessary behavior
+    — keep the high-level method the test depends on real
+
+  IF unsure what the test depends on:
+    Run the test with the real implementation FIRST
+    Observe what actually needs to happen
+    THEN add minimal mocking at the right level
+
+  Warning signs:
+    - "I'll mock this to be safe"
+    - "This might be slow, better mock it"
+    - Mocking before tracing the dependency chain
+```
+
+## Rule 4: Mirror Real Data Completely
+
+Mock the COMPLETE data structure as it exists in reality, not just the
+fields your immediate test uses.
+
+```typescript
+// ✅ GOOD: Mirror real API completeness
+const mockResponse = {
+  status: 'success',
+  data: { userId: '123', name: 'Alice' },
+  metadata: { requestId: 'req-789', timestamp: 1234567890 }
+  // All fields real API returns
+};
+```
+
+```typescript
+// ❌ The violation: only the fields you thought you needed
+const mockResponse = {
+  status: 'success',
+  data: { userId: '123', name: 'Alice' }
+  // Missing: metadata that downstream code uses
+};
+
+// Later: breaks when code accesses response.metadata.requestId
+```
+
+Partial mocks hide structural assumptions and fail silently when
+downstream code reads an omitted field: the test passes while integration
+breaks.
+
+### Gate Function
+
+```
+BEFORE creating mock responses:
+  Check: "What fields does the real API response contain?"
+
+  Actions:
+    1. Examine the actual API response from docs/examples
+    2. Include ALL fields the system might consume downstream
+    3. Verify the mock matches the real response schema completely
+
+  If uncertain: include all documented fields
+```
+
+## Rule 5: Tests Ship With the Implementation
+
+Testing is part of implementation. The TDD cycle — failing test, minimal
+implementation, refactor — is what "complete" means; "implementation
+complete, ready for testing" describes an unfinished task.
+
+## Rule 6: Prefer Real Components Over Complex Mocks
+
+Integration tests with real components are often simpler than elaborate
+mocks. Reach for one when you see:
+
+- Mock setup longer than the test logic
+- Mocking everything to make the test pass
+- Mocks missing methods the real components have
+- Tests breaking when the mock changes
+
+**your human partner's question:** "Do we need to be using a mock here?"
+
+## Quick Reference
+
+| When you... | Do |
+|-------------|-----|
+| Want to assert on a mocked element | Test the real component, or unmock it |
+| Need cleanup that only tests use | Put it in test utilities |
+| Are about to mock a method | Learn its side effects first; mock the slow/external level |
+| Build a mock response | Mirror the real structure completely |
+| Finish an implementation | Tests already exist (TDD) — or it is unfinished |
+| Watch mock setup balloon | Switch to an integration test with real components |
+
+## Warning Signs
+
+- An assertion checks for a `*-mock` test ID
+- A method is called only from test files
+- Mock setup is more than half the test
+- The test fails when you remove the mock
+- You can't explain why the mock is needed
+- Mocking "just to be safe"