atbb / CONTRIBUTING.md

# Contributing to atBB

This document covers development workflow, testing standards, and coding patterns for contributors. For project-specific gotchas and non-obvious quirks, see [`CLAUDE.md`](CLAUDE.md).

## Testing Workflow

### When to Run Tests

**Before every commit:**
```sh
pnpm test
git add <specific files>
git commit -m "feat: your changes"
```

**Before requesting code review:**
```sh
pnpm build  # Ensure clean build
pnpm test   # Verify all tests pass
# Only then push and request review
```

**After fixing review feedback:**
```sh
# Make fixes
pnpm test   # Verify tests still pass
# Push updates
```

### Test Requirements

All new features must include tests:
- **API endpoints:** Success cases, error cases, edge cases
- **Business logic:** All code paths and error conditions
- **Error handling:** All catch blocks tested
- **Security features:** Authentication, authorization, input validation

### Test Quality Standards

- Tests must be independent (no shared state between tests)
- Use descriptive test names that explain what is being tested
- Mock external dependencies (databases, APIs, network calls)
- Test error paths, not just happy paths
- Verify logging and error messages are correct

### Test Coverage Expectations

While we don't enforce strict coverage percentages, aim for:
- **Critical paths:** 100% coverage (authentication, authorization, data integrity)
- **Error handling:** All catch blocks should be tested
- **API endpoints:** All routes should have tests
- **Business logic:** All functions with branching logic should be tested

Do not:
- Skip writing tests to "move faster" — untested code breaks in production
- Write tests after requesting review — tests inform implementation
- Rely on manual testing alone — automated tests catch regressions

## Error Handling Patterns

### Defensive Programming

All list queries must include defensive limits:
```typescript
.from(categories)
.orderBy(categories.sortOrder)
.limit(1000);  // Prevent memory exhaustion on unbounded queries
```

Always filter soft-deleted records in read queries:
```typescript
.where(and(
  eq(posts.rootPostId, topicId),
  eq(posts.deleted, false),
  eq(posts.bannedByMod, false)
))
```

Use explicit ordering for consistent, reproducible results:
```typescript
.orderBy(asc(posts.createdAt))  // Chronological order for replies
```

### Global Error Handler

Every Hono app must have a global error handler as a safety net. See `apps/appview/src/create-app.ts` for the production implementation:

```typescript
app.onError((err, c) => {
  console.error("Unhandled error in route handler", {
    path: c.req.path,
    method: c.req.method,
    error: err.message,
    stack: err.stack,
  });
  return c.json(
    {
      error: "An internal error occurred. Please try again later.",
      ...(process.env.NODE_ENV !== "production" && {
        details: err.message,
      }),
    },
    500
  );
});
```

### Testing Error Classification

Test that errors return the right status code — users need actionable feedback:

| Status | When to return |
|--------|----------------|
| 400 | Invalid input, missing required fields, malformed JSON |
| 404 | Resource doesn't exist |
| 503 | Network errors, PDS connection failures — user should retry |
| 500 | Unexpected errors, database errors — needs investigation |

```typescript
// ✅ Network errors should return 503 (user should retry)
it("returns 503 when PDS connection fails", async () => {
  mockPutRecord.mockRejectedValueOnce(new Error("fetch failed"));
  const res = await app.request("/api/topics", {
    method: "POST",
    body: JSON.stringify({ text: "Test" })
  });
  expect(res.status).toBe(503);
});

// ✅ Server errors should return 500 (needs investigation)
it("returns 500 for unexpected database errors", async () => {
  mockPutRecord.mockRejectedValueOnce(new Error("Database connection lost"));
  const res = await app.request("/api/topics", {
    method: "POST",
    body: JSON.stringify({ text: "Test" })
  });
  expect(res.status).toBe(500);
});

// ✅ Input validation should return 400
it("returns 400 for malformed JSON", async () => {
  const res = await app.request("/api/topics", {
    method: "POST",
    headers: { "Content-Type": "application/json" },
    body: "{ invalid json }"
  });
  expect(res.status).toBe(400);
});
```

## Security Code Review Checklist

Before requesting review for authentication/authorization code:

- [ ] All permission checks fail closed (deny access on error)
- [ ] Database errors in security checks are caught and logged
- [ ] Programming errors (TypeError) are re-thrown, not caught
- [ ] Privilege escalation is prevented (equal/higher authority blocked)
- [ ] Tests cover unauthorized (401), forbidden (403), and error cases
- [ ] Error messages don't leak internal details (priority values, permission names)
- [ ] Middleware composition is correct (auth before permission checks)
- [ ] Startup fails fast if security infrastructure is unavailable

**Example test suite structure for security-critical endpoints:**
```typescript
describe("POST /api/admin/members/:did/role (security-critical)", () => {
  describe("Authorization", () => {
    it("returns 401 when not authenticated", async () => { /* ... */ });
    it("returns 403 when user lacks manageRoles permission", async () => { /* ... */ });
  });

  describe("Privilege Escalation Prevention", () => {
    it("prevents admin from assigning owner role (higher authority)", async () => {
      // Admin (priority 10) tries to assign Owner (priority 0) → 403
    });
    it("allows admin to assign moderator role (lower authority)", async () => {
      // Admin (priority 10) assigns Moderator (priority 20) → 200
    });
  });

  describe("Error Handling", () => {
    it("returns 503 when PDS connection fails (network error)", async () => { /* ... */ });
    it("returns 500 when database query fails (server error)", async () => { /* ... */ });
    it("returns 400 for malformed roleUri (input validation)", async () => { /* ... */ });
  });
});
```

## Bruno API Collections

The `bruno/` directory contains [Bruno](https://www.usebruno.com/) collections for API documentation and interactive testing.

### When to Update

| Event | Action |
|-------|--------|
| New endpoint | Create a `.bru` file in `bruno/AppView API/<feature>/` |
| Modified endpoint | Update the corresponding `.bru` file |
| Removed endpoint | Delete the `.bru` file; update `bruno/README.md` if referenced |
| New environment variable | Update `bruno/environments/local.bru` and `bruno/environments/dev.bru`; document in `bruno/README.md` |

Always update Bruno files **in the same commit** as the route implementation.

### File Template

```bru
meta {
  name: Endpoint Name
  type: http
  seq: 1
}

get {
  url: {{appview_url}}/api/path
}

params:query {
  param1: {{variable}}
}

headers {
  Content-Type: application/json
}

body:json {
  {
    "field": "value"
  }
}

assert {
  res.status: eq 200
  res.body.field: isDefined
}

docs {
  Brief description of what this endpoint does.

  Path/query/body params:
  - param1: Description (type, required/optional)

  Returns:
  {
    "field": "value"
  }

  Error codes:
  - 400: Bad request (invalid input)
  - 401: Unauthorized (requires auth)
  - 404: Not found
  - 500: Server error

  Notes:
  - Any special considerations or validation rules
}
```

### Before Committing

1. Open the collection in Bruno
2. Test each modified request against your local dev server (`pnpm dev`)
3. Verify assertions pass (green checkmarks)
4. Verify documentation is accurate and complete
5. Check that error scenarios are documented (not just happy path)