Coding.md - Standards a Machine Can Enforce

# Coding.md - Development Standards and Guidelines
<!-- Coding standards, git workflow, code review, debugging, and error handling -->
<!-- Establishes team conventions for consistent, maintainable code -->
<!-- Last updated: 2026-07-27 -->

## The Enforcement Hierarchy

Write this section first, because it determines whether the rest of the document is worth anything.

A standard exists at exactly one of four strengths. Know which one each of your rules is at, and be honest about it:

```mermaid
flowchart TD
  A["Tier 1: Deterministic scanner - formatter, linter, type checker, test gate"] --> B["Tier 2: Hook - runs on an event, cannot be talked out of it"]
  B --> C["Tier 3: Reviewed by a human or an independent review agent"]
  C --> D["Tier 4: Prose in this document - advisory only"]
```

- **Tier 1 - the scanner.** The lint config, the formatter config, the type checker, the test suite, the build. These are the real standard. They fail identically for a distracted human at 5 pm and for an agent running unattended at 3 am. Every rule that can be expressed as a scanner rule should be.
- **Tier 2 - the hook.** Things a scanner cannot express but that must never be skipped: block a commit that touches a generated directory, refuse a push that adds a secret-shaped string, run the formatter on staged files. A hook executes regardless of what any human or model intended.
- **Tier 3 - review.** Judgement calls: is this the right abstraction, does this name mean what it says, is this migration safe on production row counts.
- **Tier 4 - prose.** Everything left over. This document. It documents intent and explains the "why" behind tiers 1 to 3. It is genuinely useful, and it is also the tier most likely to be skipped - by a hurried engineer skimming, and by a model that decided the paragraph did not apply to its task.

**The working rule**: if a standard matters, move it down a tier until it executes. If it cannot be encoded, write in this document *why not* and *who checks it*. An unowned tier-4 rule is not a standard, it is a wish.

This matters more now than it used to. When most code was typed by hand, prose standards were absorbed slowly through code review and osmosis. When a large share of diffs are drafted by an agent, the gate is the only thing every change reliably passes through.

### Linting and Formatting Configuration

All rules are non-negotiable. Fix lint errors before committing. Never disable a rule inline without a comment explaining why, and expect that comment to be reviewed.

**ESLint configuration**:
```javascript
module.exports = {
  extends: [
    'eslint:recommended',
    'plugin:@typescript-eslint/strict-type-checked',
    'plugin:react-hooks/recommended',
    'prettier',  // Must be last - disables formatting rules that conflict with Prettier
  ],
  rules: {
    // Explicit return types on exported functions
    '@typescript-eslint/explicit-function-return-type': ['error', {
      allowExpressions: true,
      allowTypedFunctionExpressions: true,
    }],
    // No unused variables (prefix with _ to ignore intentionally)
    '@typescript-eslint/no-unused-vars': ['error', {
      argsIgnorePattern: '^_',
      varsIgnorePattern: '^_',
    }],
    // Nullish coalescing over logical OR for defaults
    '@typescript-eslint/prefer-nullish-coalescing': 'error',
    // No floating promises - must await or void
    '@typescript-eslint/no-floating-promises': 'error',
    // No console.log in production code (use the logger)
    'no-console': ['error', { allow: ['warn', 'error'] }],
    // Consistent import ordering
    'import/order': ['error', {
      groups: ['builtin', 'external', 'internal', 'parent', 'sibling'],
      'newlines-between': 'always',
      alphabetize: { order: 'asc' },
    }],
  },
};
```

**Prettier configuration**:
```json
{
  "semi": true,
  "singleQuote": true,
  "tabWidth": 2,
  "trailingComma": "all",
  "printWidth": 100,
  "bracketSpacing": true,
  "arrowParens": "always"
}
```

**Running the checks**:
```bash
npm run lint              # Check all files for lint errors
npm run lint:fix          # Auto-fix what is fixable
npm run format            # Run the formatter over all files
npm run format:check      # Verify formatting without writing (CI)
npm run type-check        # Type checker, no emit
npm run verify            # All of the above plus unit tests - the single gate
```

Everything a contributor needs to prove a change is good should be reachable from one command. If `npm run verify` passes, the change meets the mechanical standard. If it does not, no amount of explaining in the pull request description makes up for it.

## Coding Standards

### General Principles

1. **Readability over cleverness** - Code is read far more often than it is written. If a clever one-liner needs a comment to explain, write it as clear multi-line code instead.
2. **Explicit over implicit** - Name things clearly. Avoid abbreviations unless universally understood (`id`, `url`, `api`). A function named `processData` is worse than `transformOrderLineItems`.
3. **Single responsibility** - Each function does one thing. Each file owns one concept. If the description needs an "and", split it.
4. **Fail fast and loud** - Validate at the boundary. Throw descriptive errors. Never swallow an exception silently.
5. **No dead code** - Delete unused functions, commented-out blocks, and obsolete files. Version control is the backup.
6. **Small, reviewable diffs** - A change nobody can hold in their head is a change nobody actually reviewed. Split it.

### Function Guidelines

```typescript
// Good: clear name, typed parameters, single purpose, handles the empty case
export function calculateOrderTotal(
  lineItems: OrderLineItem[],
  discountPercent: number,
  taxRate: number,
): Money {
  if (lineItems.length === 0) {
    return Money.zero();
  }

  const subtotal = lineItems.reduce(
    (sum, item) => sum.add(item.price.multiply(item.quantity)),
    Money.zero(),
  );

  const discount = subtotal.multiply(discountPercent / 100);
  const taxable = subtotal.subtract(discount);
  const tax = taxable.multiply(taxRate);

  return taxable.add(tax);
}

// Bad: vague name, untyped, does too much, no edge cases
function process(data: any, flag: boolean) {
  // 200 lines of mixed concerns
}
```

### Comments and Documentation

```typescript
// Comments explain WHY, not WHAT.
// Good:
// We retry three times because the payment gateway returns 503 during its
// nightly maintenance window (23:00 - 00:00 UTC).
const MAX_PAYMENT_RETRIES = 3;

// Bad:
// Set max retries to 3
const MAX_PAYMENT_RETRIES = 3;

/**
 * Resolves the effective permission level for a user on a resource.
 * Checks direct grants first, then walks up the folder hierarchy to find
 * inherited permissions. Returns the highest permission found.
 *
 * @param userId - The user whose permissions to check
 * @param resourceId - The document or folder to check against
 * @returns The highest permission level, or null if there is no access
 */
export async function resolvePermission(
  userId: string,
  resourceId: string,
): Promise<PermissionLevel | null> {
  // Implementation
}
```

A comment that restates the code is worse than no comment, because it is one more thing that can go stale. A comment that records a decision - why this constant, why this order, which incident caused this guard - is the most durable documentation your codebase has.

## Naming Conventions

This table is the team's canonical naming reference. Other context files should link here rather than restate it.

| Context | Convention | Example |
|---------|-----------|---------|
| Files (components) | kebab-case | `order-summary.tsx` |
| Files (utilities) | kebab-case | `format-currency.ts` |
| Files (tests) | kebab-case + `.test` | `order-summary.test.ts` |
| Components | PascalCase | `OrderSummary` |
| Functions | camelCase | `calculateDiscount` |
| Variables | camelCase | `orderTotal` |
| Constants | UPPER_SNAKE_CASE | `MAX_RETRY_COUNT` |
| Types and interfaces | PascalCase | `OrderLineItem` |
| Database tables | snake_case, plural | `order_line_items` |
| Database columns | snake_case | `created_at` |
| API endpoints | kebab-case | `/api/v1/order-items` |
| Environment variables | UPPER_SNAKE_CASE | `DATABASE_URL` |
| CSS classes | kebab-case | `order-summary-card` |
| Git branches | kebab-case with prefix | `feature/order-export` |

Where a naming rule can be enforced by a lint rule or a file-name check in CI, enforce it there. The table is the reference; the scanner is the standard.

## Error Handling

### Service Layer Errors
```typescript
// Domain-specific error classes
export class NotFoundError extends Error {
  constructor(entity: string, id: string) {
    super(`${entity} with id ${id} not found`);
    this.name = 'NotFoundError';
  }
}

export class ValidationError extends Error {
  constructor(
    message: string,
    public readonly field: string,
    public readonly value: unknown,
  ) {
    super(message);
    this.name = 'ValidationError';
  }
}

export class PermissionDeniedError extends Error {
  constructor(action: string, resource: string) {
    super(`Permission denied: cannot ${action} on ${resource}`);
    this.name = 'PermissionDeniedError';
  }
}

// Used in service functions
export async function deleteDocument(id: string, userId: string): Promise<void> {
  const doc = await getById(id);
  if (!doc) throw new NotFoundError('Document', id);
  if (doc.ownerId !== userId) throw new PermissionDeniedError('delete', `document/${id}`);

  await db.delete(documents).where(eq(documents.id, id));
}
```

### API Layer Error Mapping
```typescript
// Map domain errors to responses in exactly one place
function mapErrorToResponse(error: unknown): ErrorResponse {
  if (error instanceof NotFoundError) {
    return { status: 404, code: 'NOT_FOUND', message: error.message };
  }
  if (error instanceof ValidationError) {
    return { status: 400, code: 'VALIDATION_ERROR', message: error.message };
  }
  if (error instanceof PermissionDeniedError) {
    return { status: 403, code: 'FORBIDDEN', message: error.message };
  }

  // Unknown errors: log the detail, return a generic message
  logger.error('Unhandled error', { error });
  return { status: 500, code: 'INTERNAL_ERROR', message: 'An unexpected error occurred' };
}
```

### Async Error Handling
```typescript
// Always await. No floating promises.
// Good:
await sendNotification(userId, message);

// Good, when fire-and-forget is deliberate:
void sendNotification(userId, message);   // Explicit void signals intent

// Bad - floating promise, errors silently lost:
sendNotification(userId, message);

// Wrap external calls with timeout and retry
export async function callExternalApi<T>(
  fn: () => Promise<T>,
  options: { retries?: number; timeoutMs?: number } = {},
): Promise<T> {
  const { retries = 3, timeoutMs = 5000 } = options;

  for (let attempt = 1; attempt <= retries; attempt++) {
    try {
      return await Promise.race([
        fn(),
        new Promise<never>((_, reject) =>
          setTimeout(() => reject(new Error('Request timeout')), timeoutMs),
        ),
      ]);
    } catch (error) {
      if (attempt === retries) throw error;
      await sleep(Math.pow(2, attempt) * 100);   // Exponential backoff
    }
  }
  throw new Error('Unreachable');
}
```

## Git Workflow

### Branch Naming
```bash
feature/user-profile-photo-upload
bugfix/order-total-rounding-error
hotfix/payment-webhook-signature-validation
refactor/extract-notification-service
docs/api-migration-guide
test/add-payment-integration-tests
chore/upgrade-build-toolchain
```

### Commit Messages
Conventional commits, with a scope where it helps:
```bash
feat(auth): add magic link login option
fix(orders): correct tax calculation for exempt items
refactor(notifications): extract email templates to a separate module
docs(api): document rate limiting headers and behavior
test(payments): add integration tests for the refund flow
perf(search): add a trigram index for full-text document search
chore(deps): upgrade the UI framework and fix the breaking changes
```

If an agent drafted the change, say so in the commit trailer or the pull request body. Not as a disclaimer - as provenance. When you are bisecting a regression six months from now, knowing how a diff was produced is useful signal.

### Pull Request Process
1. **Create the PR** with the template filled out. Title follows the commit convention.
2. **Link the issue** with "Closes #123".
3. **Self-review** - read your own diff before requesting review. Fix the obvious things first. This applies double to a diff you did not type yourself.
4. **Request reviewers** - one from the code owners, one from anyone else on the team.
5. **Address feedback** - respond to every comment, even if only "Done".
6. **Wait for CI** - lint, types, tests, and build all pass. No exceptions, no admin merge.
7. **Squash merge** - the PR title becomes the commit message on the main branch.

## Code Review Checklist

### Reviewer Checklist (Human)
- [ ] Does the code do what the PR description says it does?
- [ ] Are there sufficient tests? Do they cover edge cases and error paths?
- [ ] Is error handling comprehensive? Are errors logged with enough context?
- [ ] Any security concerns? (Injection, XSS, auth bypass, exposed secrets)
- [ ] Is the code consistent with existing patterns in the codebase?
- [ ] Are new dependencies justified? Did the author evaluate alternatives?
- [ ] Any dead code, commented-out code, or TODO without a linked issue?
- [ ] Will this cause problems for other features or teams?
- [ ] Is the database migration reversible? Does it handle existing data?
- [ ] Are the log messages useful for debugging in production?
- [ ] Does the diff contain anything the author cannot explain?

That last item is the one that catches the failure mode specific to generated code: a plausible-looking block nobody can account for. "The tool wrote it" is not an answer. If the author cannot explain a line, it does not merge.

### Review Agent Checklist

Automated review is genuinely valuable, and it fails in a specific way if you set it up carelessly. Two rules do most of the work.

**Never let one agent write, review, fix, and approve the same change.** A reviewer that produced the plan inherits every blind spot in the plan. It will not question the assumption it started from, because to it that assumption is not an assumption. Worse, a reviewer with write access can revise the code in response to its own finding before any human sees the finding - the defect and the evidence disappear together, and the pull request looks clean.

**Prefer several narrow reviewers over one broad one.** A reviewer with one job and a short prompt produces specific, checkable findings. A reviewer told to "review this PR" produces a summary.

Set it up like this:

- [ ] The review agent is a separate invocation from the authoring agent, with its own context. It does not receive the author's plan or reasoning - only the diff and the standards.
- [ ] The review agent has **read-only** access to the repository. It emits findings; it does not edit. If it can silently fix, it can silently hide.
- [ ] Findings are **immutable and recorded** - written to the PR as comments or to an artifact that survives the run. A finding that only existed in a scrollback is not a control.
- [ ] Each reviewer is **narrowly scoped**: one for security, one for test coverage and quality, one for migration and data safety, one for API and contract compatibility. Scope them by concern, not by file.
- [ ] Deterministic scanners run **before** any review agent. Never spend model attention on something the linter already knows. If the formatter, type checker, or dependency audit fails, stop there.
- [ ] Review agent findings are **advisory input to a human**, not an approval. Approval is a human act, tiered by risk: routine changes need one approver; changes touching auth, payments, migrations, or infrastructure need a named owner.
- [ ] The agent's tool access during review is **logged**, and the log is shipped somewhere durable alongside the finding.
- [ ] There is a documented path to **override** a finding, and taking it leaves a record with a name attached.

The pattern to internalise: governance attaches to the **action**, not to the artifact. It no longer matters much whether a human or a model typed a line. What matters is that the change passed an independent gate that had no stake in the change being approved.

### Review Comment Prefixes
```text
nit: minor style suggestion, non-blocking
suggestion: alternative approach worth considering, non-blocking
question: seeking clarification, may or may not be blocking
concern: potential problem that should be addressed before merge
blocker: must be fixed before approval
praise: calling out something done well (do this often)
```

## Debugging Guide

### Local Debugging Setup
```bash
# Verbose application logging
LOG_LEVEL=debug npm run dev

# Run with the Node inspector for breakpoint debugging
npm run dev:debug
# Then attach your editor's debugger, or a browser's inspector client

# Run a single test file in watch mode for fast iteration
npm test -- --watch src/services/order.service.test.ts
```

### Common Issues

**"Module not found" after pulling the latest changes**
```bash
# The dependency tree is out of sync - reinstall
rm -rf node_modules
npm install
```

**Type errors after switching branches**
```bash
# The type checker cache is stale
npx tsc --build --clean
npm run type-check
```
Then restart your editor's language server. Every editor has this command under a different name - "Restart TS Server", "Restart language server", or simply reopening the workspace. Learn yours; it resolves a surprising share of phantom type errors.

**Database migration failed**
```bash
# Check migration status
npm run db:migrate:status

# If it is stuck, look for an orphaned lock
SELECT * FROM _migrations_lock;

# Release it only after confirming no migration is actually running
UPDATE _migrations_lock SET is_locked = false;
```

**Tests pass locally but fail in CI**
```bash
# Usually a timing issue or a missing environment variable.
# Reproduce CI conditions locally:
CI=true npm test

# Check for timezone-dependent assertions:
TZ=UTC npm test
```

**A change passes review but breaks in production**

Work backwards through the enforcement hierarchy. Which tier should have caught it? If the answer is "tier 4, the prose said not to do that", you have found your next scanner rule. Every production incident is an argument for moving one standard down a tier.

## Performance Guidelines

### Code-Level
- Paginate every list query. Default page size 20, maximum 100.
- Debounce user input handlers (search, autocomplete) by about 300ms.
- Memoize expensive computations only when profiling shows it helps.
- Use `Promise.all` for independent async work instead of sequential awaits.
- Do not allocate objects or arrays inside render functions.

### Database
- Index the columns used in WHERE, JOIN, and ORDER BY clauses.
- Run the query planner over any query that touches a large table before it ships.
- Never `SELECT *`. List the columns you need.
- Put a LIMIT on anything that could return an unbounded result.
- Batch inserts instead of looping single inserts.

### Security Checklist
- [ ] All user input is validated against a schema at the API boundary
- [ ] Queries use parameterized statements, never string concatenation
- [ ] User-generated content is escaped before rendering
- [ ] Authentication is checked on every protected route and endpoint
- [ ] Secrets live in the secret store, never in version control
- [ ] Dependencies are audited on a schedule, and the audit is a CI gate
- [ ] File uploads are validated for type and size before processing
- [ ] Rate limiting is enabled on authentication and public endpoints
- [ ] Any automated agent with repository write access has a scoped, revocable credential and an egress allowlist

Each box above should map to a tier 1 or tier 2 control. If a box is only checked by someone remembering to check it, write down who that someone is.