solid git:20260523.deb156aC

Immutable. This exact content never changes and is served at /api/v1/blob/30e9b7d19571242c.

# SOLID Skill

## Overview

SOLID is Robert C. Martin's five object-oriented design principles. In a
TypeScript codebase like Flight School — which mixes a Next.js API surface,
a Copilot SDK adapter layer, GitHub data fetching, and a streaming UI —
SOLID is mostly a tool for keeping modules **swappable, testable, and
free of hidden coupling**. We don't have deep class hierarchies, but we
do have:

- multiple AI providers behind one interface,
- multiple persistence backends (in-memory dev, Cosmos in production),
- many API routes that share auth + rate-limit + audit concerns, and
- streams generated by very different sources (Copilot SDK, mocks, tests).

SOLID gives us shared vocabulary for when to split a module, when to add
an interface, and when *not* to.

## Authoritative reference

This skill is reference material. **The source of truth for code style,
naming, exports, and module organization is
[`.github/instructions/typescript.instructions.md`](../../instructions/typescript.instructions.md).**

If anything here appears to conflict with that file, the instructions
win. In particular:

- Single Responsibility Functions, module organization, and DRY rules
  are already covered by `typescript.instructions.md` — this skill links
  to them rather than restating them.
- Documentation conventions (TSDoc) are covered by
  [`documentation.instructions.md`](../../instructions/documentation.instructions.md).

---

## S — Single Responsibility Principle

> A module should have one, and only one, reason to change.

In practice: split modules along **axes of change**. If two pieces of
logic change for different reasons (different team, different deploy
cadence, different test setup), they don't belong in the same file.

### Spot it

- A file imports both `fs`/`fetch` *and* business rules.
- One module exports a "god object" used by everything (`utils.ts`,
  `helpers.ts`, ambient singletons).
- A test for one function must mock five unrelated subsystems.
- A change to UI copy forces a change in the data layer.

### Example in this codebase

The GitHub client used to be a process-wide singleton Octokit that also
owned **ambient token resolution** — reading `GITHUB_TOKEN` from the
environment or shelling out to `gh auth token`. That single module owned
three responsibilities at once: ambient token discovery, Octokit
construction, and request instrumentation — and shared one cached
instance across users. The "reason to change" was different for each
(multitenancy, telemetry, auth), but they were entangled.

The current `src/lib/github/client.ts` has no ambient auth surface at
all. Token resolution lives in the auth layer
(`src/lib/auth/context.ts`); this module only knows how to *use* a
token it was handed:

```ts
// src/lib/github/client.ts
export function getOctokitForToken(token: string): Octokit {
  const instance = new Octokit({ auth: token });
  instrumentOctokitRequests(instance);
  return instance;
}

export async function getOctokitForRequest(): Promise<Octokit> {
  const { accessToken } = await requireUserContext();
  return getOctokitForToken(accessToken);
}
```

Each function has one job: build an Octokit for a given token, or bind
to the current request's authenticated user. The file-level docstring is
explicit about what was *removed* and why:

> There is intentionally **no** ambient token resolution in this
> module — no `GITHUB_TOKEN` env fallback, no `gh auth token` CLI
> lookup, no process-wide cache. Every caller must supply a token that
> came from the authenticated user context.

The previous module collapsed three axes of change (instance lifecycle,
ambient auth discovery, per-user authorisation) into one file. Splitting
them — and deleting the ambient path entirely — is SRP in service of a
security boundary: a leak test (`src/lib/github/client.test.ts`'s
`'does not export legacy token resolvers'`) now pins that the helpers
can never come back.

See also: [Single Responsibility Functions
rule](../../instructions/typescript.instructions.md#single-responsibility-functions).

---

## O — Open/Closed Principle

> Modules should be open for extension, but closed for modification.

In TypeScript this rarely means inheritance. It usually means: design
the **shape of a call site** so new behaviour can be added by composing
new pieces, not by editing the existing function.

### Spot it

- Adding a new feature requires editing a `switch` / `if-else` ladder in
  a shared function.
- Every new API route copies the same setup boilerplate (auth, rate
  limit, audit) and then drifts.
- The only way to add behaviour is a new boolean parameter.

### Example in this codebase

`withUserGuards` in `src/lib/security/guard.ts` is built so any new
expensive API route gets the same auth + rate-limit + concurrent-cap +
audit treatment without modifying the wrapper itself. Cross-cutting
concerns are *configured*, not re-implemented:

```ts
// src/lib/security/guard.ts
export async function withUserGuards<T>(
  opts: GuardOptions,
  work: (ctx: UserContext) => Promise<T>,
): Promise<T> {
  const ctx = await requireUserContext();
  // ...rate limit (skipped if opts.rateLimit is omitted)...
  // ...concurrent cap (skipped if opts.concurrentCap is omitted)...
  auditLog({ type: opts.eventType, /* ... */ });
  try { return await work(ctx); } finally { release?.(); }
}
```

A new route adds the wrapper and supplies its own `eventType` / limits.
The guard module stays closed; the route extends it via composition:

```ts
export async function POST(request: NextRequest) {
  return withUserGuards(
    { rateLimit: { limit: 10, windowMs: 60_000 }, concurrentCap: 2,
      eventType: 'copilot.session.create' },
    async ({ userId }) => { /* the actual work */ },
  );
}
```

When we need a new guard (say, billing checks), we extend `GuardOptions`
or compose another wrapper — without touching every route.

---

## L — Liskov Substitution Principle

> Subtypes must be usable in place of their supertypes without surprising
> callers.

In TypeScript this is mostly about **interface contracts**: every
implementation of an interface must honour the same observable
behaviour. Throwing where the contract said you return is an LSP
violation, even if the types compile.

### Spot it

- Implementations of an interface differ in which methods *actually*
  work.
- A "real" implementation throws on calls that the in-memory one
  silently returns `null` for (or vice versa).
- Callers add `instanceof` checks to recover.

### Example in this codebase

`TokenStore` in `src/lib/auth/token-store.ts` has two implementations:

```ts
// src/lib/auth/token-store.ts
export interface TokenStore {
  getToken(userId: string): Promise<StoredToken | null>;
  setToken(userId: string, token: StoredToken): Promise<void>;
  deleteToken(userId: string): Promise<void>;
  cleanupExpired(): Promise<number>;
}

export class InMemoryTokenStore implements TokenStore { /* process-local Map */ }

export class CosmosTokenStore implements TokenStore {
  // Cosmos DB-backed with envelope encryption:
  //  - per-record AES-256-GCM data encryption key (DEK)
  //  - DEK wrapped with an Azure Key Vault KEK via `wrapKey(A256KW)`
  //  - documents partitioned by userId so a buggy or hostile caller
  //    cannot decrypt another user's row.
}
```

The intent — captured in the file docstring — is explicitly Liskov:

> The {@link TokenStore} contract is the Liskov boundary: callers MUST
> be able to swap implementations without behavioural change.

Both implementations honour the same observable contract: unknown users
return `null`, expired records return `null` (callers treat "not found"
and "expired" identically), `setToken` upserts, `deleteToken` is
idempotent. The Cosmos store adds confidentiality at rest (envelope
encryption, AAD bound to `userId`/`expiresAt`/`kekId` so tampered
documents fail decryption), but it does **not** change the observable
behaviour — that is the whole point. A caller wired through
`getTokenStore()` cannot tell which one it has, and tests can swap the
in-memory store in without rewriting code.

Where LSP could go wrong here: if `CosmosTokenStore` started throwing on
"user not found" instead of returning `null`, or made `deleteToken`
non-idempotent, every caller of `getTokenStore()` would need to learn
the difference. Keeping the contract identical is what makes the
substitution invisible.

---

## I — Interface Segregation Principle

> Don't force consumers to depend on methods they don't use.

In TypeScript: prefer **several small, focused interfaces** over one
fat one. A function that only needs an ID and a token shouldn't have to
accept the entire `SessionOptions` bag.

### Spot it

- Callers pass `undefined` for half the fields of an options object.
- Tests build huge fixture objects to call a small function.
- Adding a field to a "context" type ripples through unrelated call
  sites.

### Example in this codebase

`src/lib/copilot/server.ts` defines a narrow `SessionIdentity` interface
that is the *only* per-request input most session factories need:

```ts
// src/lib/copilot/server.ts
export interface SessionIdentity {
  userId: string;
  gitHubToken: string;
}

export async function createLoggedCoachSession(
  identity: SessionIdentity,
  operationName = 'Coach Session',
  inputPrompt = '',
) { /* ... */ }
```

Internally the Copilot SDK takes a wider `SessionOptions` (from
`src/lib/copilot/types.ts`):

```ts
// src/lib/copilot/types.ts
export interface SessionOptions {
  // Multi-tenant invariant — BOTH required:
  userId: string;       // partitions the session cache
  gitHubToken: string;  // ghu_ token forwarded to the SDK / MCP
  // Optional configuration:
  systemMessage?: string;
  includeMcpTools?: boolean;
  tools?: string[];
  model?: string;
}
```

But callers in API routes — `src/app/api/focus/route.ts`,
`src/app/api/jobs/job-executors.ts` — only ever see `SessionIdentity`
(the narrow `{ userId, gitHubToken }` pair, which happens to coincide
with the *required* subset of `SessionOptions`). They cannot accidentally
over-configure a session (wrong model, wrong tool list, wrong system
message) because the wide options shape is not in their interface.
`SessionOptions` is reserved for the small set of factory functions that
genuinely need it.

Note that `userId` and `gitHubToken` are non-optional on `SessionOptions`
itself — defaulting either would let sessions leak between users sharing
a sticky-routed process. ISP doesn't mean "make everything optional";
it means "don't expose fields callers shouldn't reason about."

The same idea applies to React props: prefer many small prop types over
one giant `ComponentProps`.

---

## D — Dependency Inversion Principle

> High-level modules should not depend on low-level modules. Both should
> depend on abstractions.

In TypeScript this usually appears as: the **transport** or
**representation** is abstracted, so the high-level orchestrator
(API route, hook) doesn't know whether the underlying source is a real
Copilot session, a fake, or a test stub.

### Spot it

- An API route directly constructs an SDK client and reads from its
  raw events.
- Replacing the event source in a test requires patching globals.
- Two routes implement the same SSE wire format slightly differently.

### Example in this codebase

`createSSEResponse` in `src/lib/api/streaming-utils.ts` is the
high-level abstraction every streaming route depends on. Routes hand it
an **async generator of events** — they do not encode SSE framing, they
do not own heartbeats, and they do not know whether the events came
from a Copilot SDK session, a mock, or an iterator under test:

```ts
// src/lib/api/streaming-utils.ts
export function createSSEResponse<T extends SSEStreamEvent, TMeta extends SSEStreamEvent = SSEStreamEvent>(
  streamGenerator: () => AsyncGenerator<T, void, undefined>,
  options?: CreateSSEResponseOptions<TMeta>,
): Response {
  // ...handles encoding, heartbeats, [DONE] marker, error frames, cleanup...
}
```

A route then composes whatever event source it wants:

```ts
const { stream, cleanup, model } = await createStreamingSession(prompt);

return createSSEResponse(
  async function* () {
    for await (const event of stream) {
      yield event;
    }
  },
  { onComplete: () => ({ type: 'meta', model, /* ... */ }), cleanup },
);
```

The route depends on the *abstraction* (an async generator of typed
events) — not on the Copilot SDK, not on `ReadableStream`, not on the
SSE wire format. Tests can hand `createSSEResponse` a generator of
fixture events and exercise the full pipeline without spinning up an
SDK client.

That inversion is what makes the streaming tests in
`src/lib/api/response-utils.test.ts` and `validation-utils.test.ts`
trivial: the wire layer and the source are no longer entangled.

---

## When to apply

Apply SOLID **as a diagnostic, not a checklist**. Reach for it when:

- **SRP** — a single file mixes I/O (fetch, fs, child_process) with
  business rules, or one module is touched by every PR.
- **OCP** — you're about to add another `switch` arm or a boolean flag
  to a shared function. Consider passing in a strategy or composing a
  wrapper instead.
- **LSP** — you have more than one implementation of an interface, and
  the substitution is meant to be invisible to callers (env-specific
  backends, fakes for tests).
- **ISP** — a function takes a wide options object but uses three
  fields. Consider exporting a narrower interface for that call site.
- **DIP** — high-level code (API routes, hooks, orchestrators) is
  hard-wired to a specific transport, vendor SDK, or storage engine.

## When NOT to over-apply

SOLID is a tool, not a religion. In this codebase, the following
patterns are intentionally pragmatic:

- **No interface for single-implementation code.** Don't introduce a
  `FooService` interface when only one `Foo` exists. Wait until a real
  second implementation (a fake, a vendor swap) appears.
- **Small files can mix concerns.** A 30-line utility module doesn't
  need to be split into three files just to honour SRP. SRP is about
  *axes of change*, not line counts.
- **Don't segregate interfaces preemptively.** Splitting `SessionOptions`
  into five tiny interfaces no caller asks for adds noise. Segregate
  when a consumer is genuinely forced to know fields it doesn't use.
- **Inversion has a cost.** Adding an abstraction (`TokenStore`,
  `createSSEResponse`) is justified by *more than one* current or
  imminent implementation. A one-off indirection just hides the call
  graph.
- **YAGNI beats OCP.** Don't pre-build extension points for hypothetical
  future guards, providers, or routes. Refactor when the second case
  arrives — by then you know what the abstraction actually needs.

If applying a SOLID principle makes the code harder to read or harder
to grep, you've probably over-applied it. Step back.

## See also

- [`typescript.instructions.md`](../../instructions/typescript.instructions.md) — authoritative TypeScript style, DRY rules, naming, exports, module organization.
- [`documentation.instructions.md`](../../instructions/documentation.instructions.md) — TSDoc standards for the interfaces you create.
- [`.github/skills/dry/SKILL.md`](../dry/SKILL.md) — companion skill on DRY (forward link; may not yet exist).
- [`.github/skills/copilot-sdk/SKILL.md`](../copilot-sdk/SKILL.md) — Copilot SDK patterns referenced in the DIP example.