|
| 1 | +--- |
| 2 | +name: code-review-checklist |
| 3 | +description: Structured checklist for reviewing code changes in the freebuff codebase. Covers correctness, conventions, security, and testing. |
| 4 | +license: MIT |
| 5 | +metadata: |
| 6 | + category: development |
| 7 | + audience: developers |
| 8 | +--- |
| 9 | + |
| 10 | +# Code Review Checklist |
| 11 | + |
| 12 | +Use this checklist when reviewing pull requests or evaluating code changes in the freebuff repository. |
| 13 | + |
| 14 | +## 1. Correctness |
| 15 | + |
| 16 | +- [ ] Does the change do what the PR description claims? |
| 17 | +- [ ] Are edge cases handled (empty inputs, null values, error paths)? |
| 18 | +- [ ] Are async operations properly awaited or error-handled? |
| 19 | +- [ ] Does the change break any existing behavior (check for unintended side effects)? |
| 20 | + |
| 21 | +## 2. TypeScript & Code Style |
| 22 | + |
| 23 | +- [ ] No `any` types — use proper typing or generics |
| 24 | +- [ ] Imports follow existing patterns (e.g., `./utils/auth` vs `./utils/config-dir` — check for import cycles) |
| 25 | +- [ ] No circular dependencies (especially in `cli/src/` — `project-files → auth → logger → project-files` is a known cycle) |
| 26 | +- [ ] Functions are exported only if used outside their module |
| 27 | +- [ ] Error messages are descriptive and actionable |
| 28 | + |
| 29 | +## 3. Conventions (from AGENTS.md) |
| 30 | + |
| 31 | +- [ ] Uses `bun` runtime and package manager (not npm/yarn) |
| 32 | +- [ ] Dependency injection used over module mocking in tests |
| 33 | +- [ ] Interactive CLI tests run in tmux (not headless) |
| 34 | +- [ ] No force-pushes to `main` |
| 35 | +- [ ] Monorepo boundaries respected (`cli/`, `sdk/`, `common/`, `agents/`, `packages/`) |
| 36 | + |
| 37 | +## 4. Security |
| 38 | + |
| 39 | +- [ ] No secrets, API keys, or tokens committed (even in test fixtures) |
| 40 | +- [ ] User input is validated before use (especially from `credentials.json`) |
| 41 | +- [ ] Auth tokens are not logged or exposed in error messages |
| 42 | +- [ ] File operations use safe paths (no unescaped user input in `path.join`) |
| 43 | + |
| 44 | +## 5. Testing |
| 45 | + |
| 46 | +- [ ] New code has corresponding tests |
| 47 | +- [ ] Tests use `bun:test` (not jest/vitest) |
| 48 | +- [ ] Mocked modules don't leak state across test files (bun mocks are process-wide) |
| 49 | +- [ ] Tests cover both success and error paths |
| 50 | +- [ ] No hardcoded filesystem paths in tests (use temp dirs or mocks) |
| 51 | + |
| 52 | +## 6. Freebuff-Specific |
| 53 | + |
| 54 | +- [ ] Session changes respect `IS_FREEBUFF` guards |
| 55 | +- [ ] Auth changes don't break the login modal → polling → credential save flow |
| 56 | +- [ ] Free session state transitions are handled: `none → active → ended → none` |
| 57 | +- [ ] Changes to `freebuff-session-store.ts` maintain the one-instance-per-account invariant |
| 58 | + |
| 59 | +## 7. Files to Watch |
| 60 | + |
| 61 | +| File | Why | |
| 62 | +|------|-----| |
| 63 | +| `cli/src/utils/auth.ts` | Credential storage — changes affect login/logout/profile | |
| 64 | +| `cli/src/state/freebuff-session-store.ts` | Session state — changes affect all session lifecycle | |
| 65 | +| `cli/src/hooks/use-freebuff-session.ts` | Poll loop — complex async state machine | |
| 66 | +| `cli/src/commands/command-registry.ts` | Slash commands — user-facing behavior | |
| 67 | +| `sdk/src/index.ts` | Public API — breaking changes affect external users | |
0 commit comments