atbb / CLAUDE.md

# Claude.md

The role of this file is to describe common mistakes and confusion points that agents might encounter as they work in this project. If you ever encounter something in the project that surprises you, please alert the developer working with you and indicate that this is the case in the Claude.md file to help prevent future agents from having the same issue.

This project is greenfield. It is okay to change the schema entirely or make what might be breaking changes. We will sort out any backfill or backwards compatibility requirements when they are actually needed.

## Development

### Setup

```sh
devenv shell                    # enter Nix dev shell (Node.js, pnpm, turbo)
pnpm install                    # install all workspace dependencies
cp .env.example .env            # configure environment variables
```

### Auto-Fixing Lint Issues

Before committing, auto-fix safe lint violations:

```sh
# Fix all packages
pnpm turbo lint:fix

# Fix specific package
pnpm --filter @atbb/appview lint:fix
```

## Testing Standards

**CRITICAL: Always run tests before committing code or requesting code review.**

### Environment Variables in Tests

**CRITICAL**: Turbo blocks environment variables by default for cache safety. Tests requiring env vars must declare them in `turbo.json`:

```json
{
  "tasks": {
    "test": {
      "dependsOn": ["^build"],
      "env": ["DATABASE_URL"]
    }
  }
}
```

**Symptoms of missing declaration:**
- Tests pass when run directly (`pnpm --filter @atbb/appview test`)
- Tests fail when run via Turbo (`pnpm test`) with undefined env vars
- CI fails even though env vars are set in workflow
- Database errors like `database "username" does not exist` (postgres defaults to system username when DATABASE_URL is unset)

**Why this matters:** Turbo's caching requires deterministic inputs. Environment variables that leak into tasks without declaration would make cache hits unpredictable. By explicitly declaring env vars in `turbo.json`, you tell Turbo to include them in the task's input hash and pass them through to the test process.

**When adding new env vars to tests:** Update `turbo.json` immediately, or tests will mysteriously fail when run via Turbo but pass when run directly.

**Placeholder tests are prohibited:**
```typescript
// ❌ FORBIDDEN: Stub tests provide false confidence
it("assigns role successfully when admin has authority", async () => {
  expect(true).toBe(true);  // NOT A REAL TEST
});

// ✅ REQUIRED: Real tests with actual assertions
it("assigns role successfully when admin has authority", async () => {
  const admin = await createUser(ctx, "Admin");
  const member = await createUser(ctx, "Member");
  const moderatorRole = await createRole(ctx, "Moderator", [], 20);

  const res = await app.request(`/api/admin/members/${member.did}/role`, {
    method: "POST",
    headers: authHeaders(admin),
    body: JSON.stringify({ roleUri: moderatorRole.uri })
  });

  expect(res.status).toBe(200);
  const data = await res.json();
  expect(data.roleAssigned).toBe("Moderator");

  // Verify database state changed
  const updatedMember = await getMembership(ctx, member.did);
  expect(updatedMember.roleUri).toBe(moderatorRole.uri);
});
```

**Skipped tests (`test.skip`, `it.skip`) must have a Linear issue tracking why.** Skipping without a tracked reason is prohibited.

### Before Requesting Code Review

**CRITICAL: Run this checklist before requesting review to catch issues early:**

```sh
# 1. Verify all tests pass
pnpm test

# 2. Check runtime dependencies are correctly placed
# (Runtime imports must be in dependencies, not devDependencies)
grep -r "from 'drizzle-orm'" apps/*/src  # If found, verify in dependencies
grep -r "from 'postgres'" apps/*/src    # If found, verify in dependencies

# 3. Verify error test coverage is comprehensive
# For API endpoints, ensure you have tests for:
# - Input validation (missing fields, wrong types, malformed JSON)
# - Error classification (network→503, server→500)
# - Error message clarity (user-friendly, no stack traces)
```

**Common mistake:** Adding error tests AFTER review feedback instead of DURING implementation. Write error tests immediately after implementing the happy path — they often reveal bugs in error classification and input validation that are better caught before review.

## Lexicon Conventions

- **Source of truth is YAML** in `packages/lexicon/lexicons/`. Never edit generated JSON or TypeScript.
- **Build pipeline:** YAML → JSON (`scripts/build.ts`) → TypeScript (`@atproto/lex-cli gen-api`).
- **Adding a new lexicon:** Create a `.yaml` file under `lexicons/space/atbb/`, run `pnpm --filter @atbb/lexicon build`.
- **Record keys:** Use `key: tid` for collections (multiple records per repo). Use `key: literal:self` for singletons.
- **References:** Use `com.atproto.repo.strongRef` wrapped in named defs (e.g., `forumRef`, `subjectRef`).
- **Extensible fields:** Use `knownValues` (not `enum`) for strings that may grow (permissions, reaction types, mod actions).
- **Record ownership:**
  - Forum DID owns: `forum.forum`, `forum.category`, `forum.board`, `forum.role`, `forum.theme`, `forum.themePolicy`, `modAction`
  - User DID owns: `post`, `membership`, `reaction`

## AT Protocol Conventions

- **Unified post model:** There is no separate "topic" type. A `space.atbb.post` without a `reply` ref is a topic starter; one with a `reply` ref is a reply.
- **Reply chains:** `replyRef` has both `root` (thread starter) and `parent` (direct parent) — same pattern as `app.bsky.feed.post`.
- **MVP trust model:** The AppView holds the Forum DID's signing keys directly and writes forum-level records on behalf of admins/mods after verifying their role. This will be replaced by AT Protocol privilege delegation post-MVP.

## TypeScript / Hono Gotchas

- **`@types/node` is required** as a devDependency in every package that uses `process.env` or other Node APIs. `tsx` doesn't need it at runtime, but `tsc` builds will fail without it.
- **Hono JSX `children`:** Use `PropsWithChildren<T>` from `hono/jsx` for components that accept children. Unlike React, Hono's `FC<T>` does not include `children` implicitly.
- **HTMX attributes in JSX:** The `typed-htmx` package provides types for `hx-*` attributes. See `apps/web/src/global.d.ts` for the augmentation.
- **Glob expansion in npm scripts:** `@atproto/lex-cli` needs file paths, not globs. Use `bash -c 'shopt -s globstar && ...'` to expand `**/*.json` in npm scripts.
- **`.env` loading:** Dev scripts use Node's `--env-file=../../.env` flag to load the root `.env` file. No `dotenv` dependency needed.
- **API endpoint parameter type guards:** Never trust TypeScript types for user input. Change handler parameter types from `string` to `unknown` and add explicit `typeof` checks. TypeScript types are erased at runtime — a request missing the `text` field will pass type checking but crash with `TypeError: text.trim is not a function`.
  ```typescript
  // ❌ BAD: Assumes text is always a string at runtime
  export function validatePostText(text: string): { valid: boolean } {
    const trimmed = text.trim();  // Crashes if text is undefined!
    // ...
  }

  // ✅ GOOD: Type guard protects against runtime type mismatches
  export function validatePostText(text: unknown): { valid: boolean } {
    if (typeof text !== "string") {
      return { valid: false, error: "Text is required and must be a string" };
    }
    const trimmed = text.trim();  // Safe - text is proven to be a string
    // ...
  }
  ```
- **Hono JSON parsing safety:** `await c.req.json()` throws `SyntaxError` for malformed JSON. Always wrap in try-catch and return 400 for client errors:
  ```typescript
  let body: any;
  try {
    body = await c.req.json();
  } catch {
    return c.json({ error: "Invalid JSON in request body" }, 400);
  }
  ```

## Middleware Patterns

### Middleware Composition

**CRITICAL: Authentication must precede authorization checks.**

When using multiple middleware functions, order matters:

```typescript
// ✅ CORRECT: requireAuth runs first, sets c.get("user")
app.post(
  "/api/topics",
  requireAuth(ctx),                    // Step 1: Restore session, set user
  requirePermission(ctx, "createTopics"), // Step 2: Check permission
  async (c) => {
    const user = c.get("user")!;       // Safe - guaranteed by middleware chain
    // ... handler logic
  }
);

// ❌ WRONG: requirePermission runs first, user is undefined
app.post(
  "/api/topics",
  requirePermission(ctx, "createTopics"), // user not set yet!
  requireAuth(ctx),
  async (c) => { /* ... */ }
);
```

**Why this matters:** `requirePermission` depends on `c.get("user")` being set by `requireAuth`. If authentication middleware doesn't run first, permission checks always fail with 401.

**Testing middleware composition:**
```typescript
it("middleware chain executes in correct order", async () => {
  // Verify requireAuth sets user before requirePermission checks it
  const res = await app.request("/api/topics", {
    method: "POST",
    headers: { Cookie: "atbb_session=valid_token" }
  });

  // Should succeed if both middlewares run in order
  expect(res.status).not.toBe(401);
});
```

## Error Handling Standards

Follow these patterns for robust, debuggable production code:

### API Route Handlers

**Required for all database-backed endpoints:**
1. Validate input parameters before database queries (return 400 for invalid input)
2. Wrap database queries in try-catch with structured logging
3. Check resource existence explicitly (return 404 for missing resources)
4. Return proper HTTP status codes (400/404/500, not always 500)

**Example pattern:**
```typescript
export function createForumRoutes(ctx: AppContext) {
  return new Hono().get("/", async (c) => {
    try {
      const [forum] = await ctx.db
        .select()
        .from(forums)
        .where(eq(forums.rkey, "self"))
        .limit(1);

      if (!forum) {
        return c.json({ error: "Forum not found" }, 404);
      }

      return c.json({ /* success response */ });
    } catch (error) {
      console.error("Failed to query forum metadata", {
        operation: "GET /api/forum",
        error: error instanceof Error ? error.message : String(error),
      });
      return c.json(
        { error: "Failed to retrieve forum metadata. Please try again later." },
        500
      );
    }
  });
}
```

### Catch Block Guidelines

**DO:**
- Catch specific error types when possible (`instanceof RangeError`, `instanceof SyntaxError`)
- Re-throw unexpected errors (don't swallow programming bugs like `TypeError`)
- Log with structured context: operation name, relevant IDs, error message
- Return user-friendly messages (no stack traces in production)
- Classify errors by user action (400, 503) vs server bugs (500)

**DON'T:**
- Use bare `catch` blocks that hide all error types
- Return generic "try again later" for client errors (400) vs server errors (500)
- Fabricate data in catch blocks (return null or fail explicitly)
- Use empty catch blocks or catch without logging
- Put two distinct operations in the same try block when they have different failure semantics — a failure in the second operation will report as failure of the first

**Try Block Granularity:**

When a try block covers multiple distinct operations in sequence, errors from later steps get reported with the wrong context. Split into separate try blocks when operations have meaningfully different failure messages:

```typescript
// ❌ BAD: DB re-query failure reports "Failed to create category" even though
// the PDS write already succeeded — misleading for operators debugging
try {
  const result = await createCategory(...);  // PDS write succeeded
  categoryUri = result.uri;
  const [cat] = await db.select()...;        // DB re-query fails here
} catch (error) {
  consola.error("Failed to create category:", ...);  // Inaccurate!
}

// ✅ GOOD: each operation has its own try block and specific error message
try {
  const result = await createCategory(...);
  categoryUri = result.uri;
} catch (error) {
  consola.error("Failed to create category:", ...);
}

try {
  const [cat] = await db.select()...;
} catch (error) {
  consola.error("Failed to look up category ID after creation:", ...);
}
```

**Programming Error Re-Throwing Pattern:**

```typescript
// ✅ CORRECT: Re-throw programming errors, catch runtime errors
try {
  const result = await ctx.db.select()...;
  return processResult(result);
} catch (error) {
  // Re-throw programming errors (code bugs) - don't hide them
  if (error instanceof TypeError ||
      error instanceof ReferenceError ||
      error instanceof SyntaxError) {
    console.error("CRITICAL: Programming error detected", {
      error: error.message,
      stack: error.stack,
      operation: "checkPermission"
    });
    throw error;  // Let global error handler catch it
  }

  // Log and handle expected runtime errors (DB failures, network issues)
  console.error("Database query failed", {
    operation: "checkPermission",
    error: error instanceof Error ? error.message : String(error)
  });
  return null;  // Fail safely for business logic
}
```

**Why re-throw programming errors:**
- `TypeError` = code bug (e.g., `role.permisions.includes()` typo)
- `ReferenceError` = code bug (e.g., using undefined variable)
- `SyntaxError` = code bug (e.g., malformed JSON.parse in your code)
- These should crash during development, not be silently logged
- Catching them hides bugs and makes debugging impossible

**Error Classification Helper:**

Create helper functions to classify errors consistently:

```typescript
// File: src/lib/errors.ts
export function isProgrammingError(error: unknown): boolean {
  return error instanceof TypeError ||
         error instanceof ReferenceError ||
         error instanceof SyntaxError;
}

export function isNetworkError(error: unknown): boolean {
  if (!(error instanceof Error)) return false;
  const msg = error.message.toLowerCase();
  return msg.includes("fetch failed") ||
         msg.includes("econnrefused") ||
         msg.includes("enotfound") ||
         msg.includes("timeout");
}

// Usage in route handlers:
} catch (error) {
  if (isProgrammingError(error)) {
    throw error;  // Don't catch programming bugs
  }

  if (isNetworkError(error)) {
    return c.json({
      error: "Unable to reach external service. Please try again later."
    }, 503);  // User should retry
  }

  return c.json({
    error: "An unexpected error occurred. Please contact support."
  }, 500);  // Server bug, needs investigation
}
```

### Helper Functions

**Validation helpers should:**
- Return `null` for invalid input (not throw)
- Re-throw unexpected errors
- Use specific error type checking

**Example:**
```typescript
export function parseBigIntParam(value: string): bigint | null {
  try {
    return BigInt(value);
  } catch (error) {
    if (error instanceof RangeError || error instanceof SyntaxError) {
      return null;  // Expected error for invalid input
    }
    throw error;  // Unexpected error - let it bubble up
  }
}
```

**Serialization helpers should:**
- Avoid silent fallbacks (log warnings if fabricating data)
- Prefer returning `null` over fake values (`"0"`, `new Date()`)
- Document fallback behavior in JSDoc if unavoidable

## Security-Critical Code Standards

When implementing authentication, authorization, or permission systems, follow these additional requirements:

### 1. Fail-Closed Security

**Security checks must deny access by default when encountering errors.**

```typescript
// ✅ CORRECT: Fail closed - deny access on any error
export async function checkPermission(
  ctx: AppContext,
  did: string,
  permission: string
): Promise<boolean> {
  try {
    const [membership] = await ctx.db.select()...;
    if (!membership || !membership.roleUri) {
      return false;  // No membership = deny access
    }

    const [role] = await ctx.db.select()...;
    if (!role) {
      return false;  // Role deleted = deny access
    }

    // Permissions live in role_permissions join table, not role.permissions array
    const [match] = await ctx.db
      .select()
      .from(rolePermissions)
      .where(and(
        eq(rolePermissions.roleId, role.id),
        or(
          eq(rolePermissions.permission, permission),
          eq(rolePermissions.permission, "*")
        )
      ))
      .limit(1);

    return !!match;
  } catch (error) {
    if (isProgrammingError(error)) throw error;

    console.error("Failed to check permissions - denying access", {
      did, permission, error: String(error)
    });
    return false;  // Error = deny access (fail closed)
  }
}

// ❌ WRONG: Fail open - grants access on error
} catch (error) {
  console.error("Permission check failed");
  return true;  // SECURITY BUG: grants access on DB error!
}
```

**Test fail-closed behavior:**
```typescript
it("denies access when database query fails (fail closed)", async () => {
  vi.spyOn(ctx.db, "select").mockRejectedValueOnce(new Error("DB connection lost"));

  const result = await checkPermission(ctx, "did:plc:user", "createTopics");

  expect(result).toBe(false);  // Prove fail-closed behavior
  expect(console.error).toHaveBeenCalledWith(
    expect.stringContaining("denying access"),
    expect.any(Object)
  );
});
```

### 2. Startup Failures for Missing Security Infrastructure

**Security-critical infrastructure must fail fast on startup, not at first request.**

```typescript
// ✅ CORRECT: Throw error on startup if ForumAgent unavailable
export async function seedDefaultRoles(ctx: AppContext) {
  const agent = ctx.forumAgent;
  if (!agent) {
    console.error("CRITICAL: ForumAgent not available - role system non-functional", {
      operation: "seedDefaultRoles",
      forumDid: ctx.config.forumDid
    });
    throw new Error(
      "Cannot seed roles without ForumAgent - permission system would be broken"
    );
  }
  // ... seeding logic
}

// ❌ WRONG: Silent failure allows server to start without roles
if (!agent) {
  console.warn("ForumAgent not available, skipping role seeding");
  return { created: 0, skipped: 0 };  // Server starts but is broken!
}
```

**Why this matters:** If the permission system is broken, every request will fail authorization. It's better to fail startup loudly than silently deploy a non-functional system.

## Documentation & Project Tracking

**Keep these synchronized when completing work:**

1. **`docs/atproto-forum-plan.md`** — Master project plan with phase checklist
   - Mark items complete `[x]` when implementation is done and tested
   - Add brief status notes with file references and Linear issue IDs
   - Update immediately after completing milestones

2. **Linear issues** — Task tracker at https://linear.app/atbb
   - Update status: Backlog → In Progress → Done
   - Add comments documenting implementation details when marking Done
   - Keep status in sync with actual codebase state, not planning estimates

3. **`docs/plans/` convention:**
   - Active/in-progress plan documents go directly in `docs/plans/`
   - Completed plan documents move to `docs/plans/complete/` when work is shipped

4. **Workflow:** When finishing a task:
   ```sh
   # 1. Run tests to verify implementation is correct
   pnpm test

   # 2. If tests pass, commit your changes
   git add .
   git commit -m "feat: your changes"

   # 3. Update plan document: mark [x] and add completion note
   # 4. Update Linear: change status to Done, add implementation comment
   # 5. Push and request code review
   # 6. After review approval: include "docs:" prefix when committing plan updates
   ```

**Why this matters:** The plan document and Linear can drift from reality as code evolves. Regular synchronization prevents rediscovering completed work and ensures accurate project status.

## Bruno API Collections

**CRITICAL: Update Bruno collections in the SAME commit as route implementation.**

The `bruno/` directory contains [Bruno](https://www.usebruno.com/) collections for API documentation and interactive testing. See `CONTRIBUTING.md` for the full update workflow and file template.

**Common Mistakes:**

**DON'T:**
- Commit API changes without updating Bruno collections
- Use hardcoded URLs instead of environment variables (`{{appview_url}}`)
- Skip documenting error cases (only document 200 responses)
- Leave placeholder/example data that doesn't match actual API behavior
- Forget to update assertions when response format changes

**DO:**
- Update Bruno files in the same commit as route implementation
- Use environment variables for all URLs and test data
- Document all HTTP status codes the endpoint can return

## Git Conventions

- Do not include `Co-Authored-By` lines in commit messages.
- `prior-art/` contains git submodules (Rust AppView, original lexicons, delegation spec) — reference material only, not used at build time.
- Worktrees with submodules need `submodule deinit --all -f` then `worktree remove --force` to clean up.