# Code Style Guide

This guide codifies the design principles and coding conventions used across ows-coda. The codebase prioritizes **testability** through dependency injection and null objects, **safety** through exhaustive switch statements and strict TypeScript, and **clarity** through narrow interfaces and named functions. Every rule here reflects a deliberate choice — most were learned the hard way. When in doubt, follow the existing code; when the existing code is inconsistent, follow this guide.

## Table of contents

- [TypeScript strictness](#typescript-strictness)
- [Naming](#naming)
- [Interfaces and types](#interfaces-and-types)
- [Dependency injection](#dependency-injection)
- [Null object pattern](#null-object-pattern)
- [Shared code](#shared-code)
- [Function design](#function-design)
- [Error handling](#error-handling)
- [Validation and assertions](#validation-and-assertions)
- [Collections and iterables](#collections-and-iterables)
- [Tool and skill handlers](#tool-and-skill-handlers)
- [Configuration and defaults](#configuration-and-defaults)
- [Testing](#testing)

---

## TypeScript strictness

The codebase enforces strict TypeScript with zero tolerance for `any`.

- **`strict: true`** in all tsconfigs — `noImplicitAny`, `strictNullChecks`, `noUncheckedIndexedAccess` are all enabled
- **Zero `any`** — `@typescript-eslint/no-explicit-any` is an error. Use `unknown` with type guards or discriminated unions instead
- **Unused variables** — prefix with `_` (e.g., `_unused`) to suppress lint errors

```typescript
// Bad
function parse(data: any): any { ... }

// Good
function parse(data: unknown): ParsedResult { ... }
```

---

## Naming

### No `I` prefix on interfaces

The interface name is the concept. The concrete implementation name adds the distinguishing detail.

```typescript
// Bad
interface ISnapshotStore { ... }
class S3SnapshotStore implements ISnapshotStore { ... }

// Good
interface SnapshotStore { ... }
class S3SnapshotStore implements SnapshotStore { ... }
```

Real examples: `SyncCache` / `LRUCache`, `KeyValueStore` / `NullKeyValueStore`, `BlobStore` / `S3BlobStore`.

### Minimal method names

When the type already provides context, keep method names short. A `CircuitBreakerStrategy` doesn't need `recordFailure()` — `failure()` is unambiguous.

```typescript
// Bad — redundant with the type
class CircuitBreakerStrategy {
  recordFailure(): boolean { ... }
  recordSuccess(): void { ... }
  resetState(): void { ... }
}

// Good — the type provides context
class CircuitBreakerStrategy {
  failure(): boolean { ... }
  success(): void { ... }
  clear(): void { ... }
}
```

### File naming

- Kebab-case for files: `key-value-store.ts`, `null-blob-store.ts`
- Match the primary export: `lruCache.ts` exports `LRUCache`, `circuitBreaker.ts` exports `CircuitBreaker`
- Test files mirror source: `lruCache.ts` → `__tests__/lruCache.test.ts`
- Barrel exports via `index.ts` at module boundaries

---

## Interfaces and types

### Narrow interfaces

Define the smallest useful contract. Consumers should depend on what they actually use, not the full surface of a concrete class.

```typescript
// packages/common/src/storage/key-value-store.ts
export interface KeyValueStore {
  get(key: string): Promise<string | null>;
  set(key: string, value: string, ttlSeconds?: number): Promise<void>;
  del(...keys: string[]): Promise<void>;
  disconnect(): Promise<void>;
}
```

This interface is consumed by `ConversationCache`, `MemoryCacheStore`, Redis-backed stores, and the null object — none of which need to know about each other.

### Required over optional

If a value is always available at construction time, make it required. Optional properties push null-checking into every consumer.

```typescript
// Bad — forces every callsite to check
interface Options {
  strategy?: CircuitBreakerStrategy;
  cooldownMs?: number;
}

// Good — required when always available, with sensible defaults at the config layer
interface CircuitBreakerOptions<TArgs extends unknown[], TResult> {
  fn: (...args: TArgs) => Promise<TResult>;
  strategy: CircuitBreakerStrategy;
  cooldownMs: number;
  successThreshold: number;
  onStateChange?: (state: CircuitState) => void; // truly optional callback
}
```

### Generic type parameters

Use generics to make data structures and algorithms reusable. Constrain type parameters only when the implementation requires it.

```typescript
export interface SyncCache<K, V> {
  readonly size: number;
  get(key: K): V | undefined;
  set(key: K, value: V): this;
  has(key: K): boolean;
  delete(key: K): boolean;
  clear(): void;
}

export class LRUCache<K, V> implements SyncCache<K, V> { ... }
export class NullCache<K, V> implements SyncCache<K, V> { ... }
```

---

## Dependency injection

Depend on interfaces, not implementations. Inject dependencies through constructors.

### Constructor injection

```typescript
// Good — depends on the interface, injected at construction
export class SnowflakeSchemaCache {
  constructor(private readonly fetcher: SnowflakeSchemaFetcher) {}
}

// Good — multiple dependencies, all injected
export class CircuitBreaker<TArgs extends unknown[], TResult> {
  private readonly fn: (...args: TArgs) => Promise<TResult>;
  private readonly strategy: CircuitBreakerStrategy;

  constructor(options: CircuitBreakerOptions<TArgs, TResult>) {
    this.fn = options.fn;
    this.strategy = options.strategy;
    // ...
  }
}
```

### Why

- **Testability** — swap in a mock or null object without touching the implementation
- **Flexibility** — switch from Redis to in-memory without changing the consumer
- **Clarity** — constructor signatures document what a class actually needs

### Anti-patterns

```typescript
// Bad — hard dependency on a concrete class
import { RedisClient } from "ioredis";
class ConversationCache {
  private redis = new RedisClient();
}

// Bad — importing a concrete class to use as a type
import { S3BlobStore } from "./s3-blob-store";
class Uploader {
  constructor(private store: S3BlobStore) {}
}
```

---

## Null object pattern

When a dependency is optional, provide a null object instead of making the property optional and guarding every access.

```typescript
// Bad — optional with guards everywhere
class Pipeline {
  constructor(private cache?: SyncCache<string, number>) {}

  process(key: string) {
    if (this.cache) {
      const cached = this.cache.get(key);
      if (cached !== undefined) return cached;
    }
    const result = this.compute(key);
    if (this.cache) {
      this.cache.set(key, result);
    }
    return result;
  }
}

// Good — null object eliminates all guards
class Pipeline {
  constructor(private cache: SyncCache<string, number> = new NullCache()) {}

  process(key: string) {
    const cached = this.cache.get(key);
    if (cached !== undefined) return cached;
    const result = this.compute(key);
    this.cache.set(key, result);
    return result;
  }
}
```

The codebase provides null objects for all core abstractions:

| Interface          | Null object            | Location                                              |
| ------------------ | ---------------------- | ----------------------------------------------------- |
| `SyncCache<K, V>`  | `NullCache<K, V>`      | `packages/common/src/cache/nullCache.ts`              |
| `AsyncCache<K, V>` | `NullAsyncCache<K, V>` | `packages/common/src/cache/nullAsyncCache.ts`         |
| `KeyValueStore`    | `NullKeyValueStore`    | `packages/common/src/storage/null-key-value-store.ts` |
| `BlobStore`        | `NullBlobStore`        | `packages/common/src/storage/null-blob-store.ts`      |

When adding a new interface that might be optional, add its null object at the same time.

---

## Shared code

### Package naming

All packages use the `@coda/*` scope. **Import from the canonical package.** Do not import `@coda/async` symbols through `@coda/common` — import directly from `@coda/async`.

### Where shared code lives

| Domain                       | Package                 | Examples                                                            |
| ---------------------------- | ----------------------- | ------------------------------------------------------------------- |
| Data structures, algorithms  | `@coda/data-structures` | `BinaryHeap`, `binarySearch`, `getTopK`, `AsyncQueue`               |
| Async patterns               | `@coda/async`           | `retry`, `CircuitBreaker`, `Semaphore`, `SingleFlight`, `Throttler` |
| Search & NLP                 | `@coda/search`          | `HybridSearch`, `tokenize`, `matchGlossary`, `HnswIndex`            |
| RPC infrastructure           | `@coda/api-common`      | `callRpc`, `RpcResult`, `createTransport`                           |
| Storage, graph, redis, utils | `@coda/common`          | `Graph`, `S3BlobStore`, `LRUCache`, `toErrorMessage`, `Millis`      |

**Before writing a new helper**, check if it already exists. **Before adding a helper to an app directory**, ask whether it could live in a package instead.

### RPC client pattern

RPC clients are created at startup via `createClientSet` in `client-registry.ts` and injected via constructor DI. Each client class (e.g. `AccessClient`, `RunnerClient`) wraps a ConnectRPC transport and exposes typed methods that return `RpcResult<T>`.

```typescript
// apps/server/src/rpc/client-registry.ts
export function createServerClients(config: ServerClientsConfig) {
  return createClientSet(
    {
      access: { url: config.accessUrl, create: (c) => new AccessClient(c) },
      search: { url: config.searchUrl, create: (c) => new SearchClient(c) },
      // ...
    },
    shared,
  );
}
```

- Entries with no URL produce `null` — consumers check for null or use handler-level `enabled()` predicates.
- Shared interceptors (header forwarding, user-agent) are applied to all transports.
- No lazy singletons — clients are created once at startup and passed to handlers.

---

## Function design

### Named functions over anonymous callbacks

Especially for strategy/callback patterns. Named functions improve readability, stack traces, and independent testability.

```typescript
// Bad — anonymous inline
const breaker = new CircuitBreaker({
  fn: async (query) => {
    const conn = await pool.acquire();
    try { return await conn.execute(query); }
    finally { conn.release(); }
  },
  strategy: { failure: () => { count++; return count >= 5; }, ... },
});

// Good — named, testable independently
async function executeSnowflakeQuery(query: string): Promise<Result> {
  const conn = await pool.acquire();
  try { return await conn.execute(query); }
  finally { conn.release(); }
}

const breaker = new CircuitBreaker({
  fn: executeSnowflakeQuery,
  strategy: new ConsecutiveFailureStrategy(5),
});
```

### Callback-accepting functions

When designing functions that accept callbacks (retry strategies, comparators, transformers), type the callback parameter explicitly and document its contract.

```typescript
export async function retry<T>(
  maxRetries: number,
  strategy: RetryStrategy,
  fn: (attempt: number) => Promise<RetryResult<T>>,
): Promise<T> { ... }

export function getTopK<T>(
  k: number,
  compareFn: CompareFn<T>,
  values: Iterable<T>,
): T[] { ... }
```

---

## Error handling

### Switch defaults throw

Every switch statement must handle all cases explicitly. The default case should throw to catch unhandled variants at compile time (via `never`) or runtime.

```typescript
// Bad — silent fallback
switch (source.type) {
  case "account":
    return fetchAccount(source);
  case "contract":
    return fetchContract(source);
  default:
    return null; // silently swallows unknown types
}

// Good — exhaustive with throw
switch (source.type) {
  case "account":
    return fetchAccount(source);
  case "contract":
    return fetchContract(source);
  default: {
    const _exhaustive: never = source.type;
    throw new Error(`Unhandled source type: ${source.type}`);
  }
}
```

Handle `null`/`undefined` **before** the switch with `??` or an early return, not inside a default case.

### Error types

Use built-in error types with clear semantics:

| Error type      | When to use                                                          |
| --------------- | -------------------------------------------------------------------- |
| `RangeError`    | Numeric values outside acceptable bounds                             |
| `TypeError`     | Wrong type passed to a function                                      |
| `Error`         | General logic errors, unhandled cases                                |
| Custom subclass | Domain-specific errors that callers catch (e.g., `CircuitOpenError`) |

```typescript
// packages/common/src/collections/ringBuffer.ts
if (capacity < 1) {
  throw new RangeError("RingBuffer capacity must be >= 1");
}
```

---

## Validation and assertions

### Structural validation only in utilities

Generic utilities validate structure (is it a positive integer? is the array non-empty?), not policy (is the value within the allowed range for this feature?). Policy limits belong in the consumer's config or Zod layer.

```typescript
// Good — structural validation in the utility
export function assertPositiveInt(value: number): void {
  if (!Number.isInteger(value) || value < 1) {
    throw new RangeError("Expected a positive integer");
  }
}

// Good — policy validation in the consumer's config
const ConfigSchema = z.object({
  maxRetries: z.number().int().min(1).max(10),
  cooldownMs: z.number().int().min(100).max(60_000),
});
```

### No `name` parameter in assertion helpers

Stack traces provide enough context. Keep signatures minimal.

```typescript
// Bad
assertPositiveInt(value, "cooldownMs");

// Good — the stack trace tells you where it was called
assertPositiveInt(value);
```

---

## Collections and iterables

### `Iterable<T>` for collection inputs

If a parameter only needs to iterate (no random access, no `.length`), type it as `Iterable<T>`. This accepts arrays, sets, maps, generators, and custom iterables.

```typescript
// Bad — unnecessarily restrictive
function getTopK<T>(k: number, compareFn: CompareFn<T>, values: T[]): T[] { ... }

// Good — works with any iterable
function getTopK<T>(k: number, compareFn: CompareFn<T>, values: Iterable<T>): T[] { ... }
```

Use `T[]` for outputs and when you need `.length` or indexed access.

### `Date.now()` over `performance.now()`

For timestamps and elapsed-time calculations, prefer `Date.now()`. It's more portable, produces debuggable values, and provides sufficient precision for operational use cases.

```typescript
// Good
const start = Date.now();
await operation();
const elapsedMs = Date.now() - start;
```

Reserve `performance.now()` for sub-millisecond benchmarking in isolated performance tests.

---

## Tool and skill handlers

### Handlers own their availability

Each tool handler owns its `enabled()` predicate. No external registry or name list decides whether a tool is available — the handler itself does.

```typescript
const handleSearch: ToolHandlerObject = {
  enabled: () => true,
  execute: async (input) => { ... },
};

// When disabled, provide a reason
function disabledHandlers(): Record<string, AnyToolHandler> {
  const disabled: ToolHandlerObject = {
    enabled: () => false,
    disabledReason: "Notion is not connected for this user",
    execute: () => Promise.resolve({ data: null, error: DISABLED_REASON }),
  };
  // ...
}
```

This pattern keeps tool availability decentralized and testable — test both enabled and disabled states.

---

## Configuration and defaults

### No hardcoded model assumptions

Expose model-size-dependent limits (token counts, context windows, batch sizes) as configurable parameters with sensible defaults. Never bake model-specific values into business logic.

```typescript
// Bad
const MAX_TOKENS = 4096;

// Good
interface AgentOptions {
  maxTokens?: number; // default: from model config
}
```

### Zod for config validation

Use Zod schemas at the application boundary to validate environment variables and config files. Keep validation logic separate from business logic.

---

## Testing

See the full [Testing Guide](testing.md) for Vitest configuration, mocking patterns, and test structure.

Key conventions for code style:

- **Mock factories** — build reusable factory functions that return objects with `vi.fn()` stubs
- **`mockResolvedValueOnce`** — control responses per test, not globally
- **`vi.clearAllMocks()`** — in `beforeEach` to reset state between tests
- **Test enabled/disabled states** — tools with `enabled()` predicates should test both paths
- **No `any` in tests** — the zero-any policy applies to test code too