Break the 9 server import cycles; promote noImportCycles to error (#1007)

**Nine `lint/suspicious/noImportCycles` violations in
`packages/server/src/` all routed through one bidirectional edge**:
`surface.ts` and four domain modules importing each other in a tangle
that biome had been merely _warning_ about for months. Extract a
`surfaceCtx.ts` Proxy-fronted holder so `surface.ts` registers the typed
mutation map once at startup; domain modules import the ctx from the
holder instead. The bidirectional arrow collapses to a one-way arrow
plus a one-way registration — same shape that PR [#998 cycle
1](#998) prototyped.

With zero cycles remaining, promote the rule from biome's default `warn`
to `error` in `biome.jsonc`. _The TDZ-on-import-reorder hazard that
crashed production in [#1003](#1003)
— biome's alphabetical sort reshuffled imports, the cycle converged via
a different path, `localTerminalBackend` ended up in TDZ at module load,
only `just smoke` caught it on CI — becomes structurally impossible._

### The shape that changed

```
Before                                        After

surface.ts ◀──▶ session.ts                    surface.ts ──▶ session.ts ──┐
           ◀──▶ activity.ts (via local.ts)    surface.ts ──▶ activity.ts ─┤
           ◀──▶ terminalBackend/local.ts      surface.ts ──▶ local.ts ────┼──▶ surfaceCtx.ts
           ◀──▶ terminalBackend/metadata.ts   surface.ts ──▶ metadata.ts ─┘         ▲
                                                     │                              │
                                                     └────── setSurfaceCtx ─────────┘

(local.ts also pulled surface.ts via unwrapGit;            local.ts ──▶ unwrapGit.ts
 router.ts pulled it for the same.)                        router.ts ──▶ unwrapGit.ts
```

`unwrapGit` had to come out alongside `surfaceCtx` —
`terminalBackend/local.ts` imported _both_ from `surface.ts`. Pulling
only the ctx out would leave `local.ts → surface.ts →
terminalBackend/index.ts → local.ts` cycling via `unwrapGit`. It's a
pure `GitResult → ORPCError` adapter; its own neutral file is the right
boundary, and `router.ts` picks up the new import too.

### Refinements during review

| Lens | Finding | Resolution |
|---|---|---|
| **hickey** | `setSurfaceCtx` silently overwrites `held` on a second
call — _"write once" contract is convention, not constraint_ | Guard
added |
| **lowy** _(cross-validate)_ | Strict `held !== undefined` would block
harmless same-ctx re-registration (future test isolation / HMR) |
Refined predicate to `held !== undefined && held !== ctx` — throws only
on a _genuinely different_ ctx |
| **code-police** | `session.test.ts` + `metadata.test.ts` previously
inherited a populated ctx via `surface.ts`'s import side-effect; with
the holder, the Proxy throws when not initialized | Added
`__resetSurfaceCtxForTest()` + `noopSurfaceCtxForTest()` and wired them
into the affected tests |
| **code-police** | Stale doc comment in `router.ts` still pointed at
`./surface.ts` for `surfaceCtx` | Updated |

### Validation

- `just check` — clean (zero `noImportCycles`, zero type errors,
pre-existing 40 warnings unrelated).
- `pnpm vitest run` in `packages/server` — **46/46 unit tests pass**
(the 14 that needed the new test-helper plumbing now have it).
- CI `just smoke` covers the original production-loader hazard
end-to-end.

> _Reviewer note_: also retires the `biome-ignore-start
assist/source/organizeImports` markers in `surface.ts`. With no cycle
for the alphabetical sort to converge along, the load-order constraint
they protected is gone.

Closes #1005.

### Try it locally

```sh
nix run github:juspay/kolu/noImportCycles
```

_Generated by [`/do`](https://github.com/srid/agency) on Claude Code
(model `claude-opus-4-7`)._

Author: srid

biome.jsonc

@@ -100,7 +100,12 @@
       "suspicious": {
         // Terminal code legitimately parses ANSI escape sequences —
         // the rule is a permanent false positive against xterm diagnostics.
-        "noControlCharactersInRegex": "off"
+        "noControlCharactersInRegex": "off",
+        // Promoted from the default `warn` after #1005 broke the 9 server
+        // cycles via the `surfaceCtx.ts` holder pattern. As `error`, any
+        // PR that re-introduces a cycle fails `just check` immediately —
+        // the TDZ-on-import-reorder class becomes structurally impossible.
+        "noImportCycles": "error"
       },
       "complexity": {
         // Tailwind `!important` overrides are intentional in kolu's

packages/server/src/activity.ts

@@ -15,7 +15,7 @@ import type {
   RecentRepo,
 } from "kolu-common/surface";
 import { log } from "./log.ts";
-import { surfaceCtx } from "./surface.ts";
+import { surfaceCtx } from "./surfaceCtx.ts";
 
 const MAX_RECENT_REPOS = 20;
 const MAX_RECENT_AGENTS = 10;

packages/server/src/router.ts

@@ -3,10 +3,10 @@
  * hand-listed raw oRPC handlers (terminal lifecycle, attach, git
  * mutations, server info).
  *
- * The typed reactive layer goes through `surfaceRouter` / `surfaceCtx`
- * (see `./surface.ts`). Domain mutations import `surfaceCtx` directly
- * from there. This file is just the glue between the surface fragment
- * and the raw RPCs.
+ * The typed reactive layer goes through `surfaceRouter` (from `./surface.ts`)
+ * and `surfaceCtx` (from `./surfaceCtx.ts`). Domain mutations import
+ * `surfaceCtx` from `./surfaceCtx.ts` directly. This file is just the glue
+ * between the surface fragment and the raw RPCs.
  */
 
 import { ORPCError } from "@orpc/server";
@@ -23,10 +23,11 @@ import { match } from "ts-pattern";
 import { serverHostname, serverProcessId } from "./hostname.ts";
 import { log } from "./log.ts";
 import { pwaIdentityForHostname } from "./pwaIdentity.ts";
-import { surfaceRouter, t, unwrapGit } from "./surface.ts";
+import { surfaceRouter, t } from "./surface.ts";
 import { getTerminal, type TerminalProcess } from "./terminal-registry.ts";
 import { getTerminalBackendFor } from "./terminalBackend/index.ts";
 import { saveTerminalFile } from "./terminalScratch.ts";
+import { unwrapGit } from "./unwrapGit.ts";
 import {
   createTerminal,
   killAllTerminals,

packages/server/src/session.test.ts

@@ -1,6 +1,9 @@
 import * as assert from "node:assert";
-import type { SavedTerminal } from "kolu-common/surface";
-import { afterAll, describe, expect, it } from "vitest";
+import type { SavedSession, SavedTerminal } from "kolu-common/surface";
+import { confStore } from "@kolu/surface/server";
+import { afterAll, beforeAll, describe, expect, it } from "vitest";
+import { __resetSurfaceCtxForTest, setSurfaceCtx } from "./surfaceCtx.ts";
+import { store } from "./state.ts";
 import {
   clearSavedSession,
   getSavedSession,
@@ -27,8 +30,34 @@ const terminal: SavedTerminal = {
 };
 
 describe("session persistence", () => {
+  beforeAll(() => {
+    // surface.ts is not imported by this test module (no full backend init),
+    // so we supply a minimal ctx where cells.session is backed by the real
+    // confStore. This makes writeSession → surfaceCtx.cells.session.set(v)
+    // actually persist to the conf store, which getSavedSession() reads back.
+    const sessionStore = confStore<SavedSession | null>(store, "session");
+    setSurfaceCtx({
+      cells: new Proxy({} as never, {
+        get: (_, key) =>
+          key === "session"
+            ? sessionStore
+            : { get: () => undefined, set: () => {}, patch: () => {} },
+      }),
+      collections: new Proxy({} as never, {
+        get: () => ({
+          upsert: () => {},
+          remove: () => {},
+          readAll: () => new Map(),
+          readOne: () => undefined,
+        }),
+      }),
+      events: new Proxy({} as never, { get: () => ({ publish: () => {} }) }),
+    } as never);
+  });
+
   afterAll(() => {
     clearSavedSession();
+    __resetSurfaceCtxForTest();
   });
 
   it("returns null when no session is saved", () => {

packages/server/src/session.ts

@@ -13,7 +13,7 @@ import type { SavedSession, SavedTerminal } from "kolu-common/surface";
 import { log } from "./log.ts";
 import { terminalsDirtyChannel } from "./publisher.ts";
 import { store } from "./state.ts";
-import { surfaceCtx } from "./surface.ts";
+import { surfaceCtx } from "./surfaceCtx.ts";
 
 /** Pending autosave timer — declared at module top so `setSavedSession`
  *  and the surface cell's `store.set` adapter can cancel it (see comment

packages/server/src/surface.ts

@@ -5,12 +5,16 @@
  *   - `surfaceRouter` — `oc.router({ surface: {...} })` fragment for the
  *     host router; spread alongside hand-listed raw oRPC procedures in
  *     `router.ts`.
- *   - `surfaceCtx` — typed `cells / collections / events` mutation map.
- *     Domain modules (`activity.ts`, `session.ts`, `terminals.ts`,
- *     `terminalBackend/metadata.ts`) import this and call `surfaceCtx.cells.X.set(...)`,
- *     `surfaceCtx.collections.X.upsert(k, v)`, `surfaceCtx.events.X.publish(i, p)`.
- *     The framework owns the apply+publish chain (and per-input event
- *     channel); domain code never sees a channel name string.
+ *   - The typed `cells / collections / events` mutation map (`surfaceCtx`)
+ *     is built here and registered into `./surfaceCtx.ts` via
+ *     `setSurfaceCtx(...)`. Domain modules (`activity.ts`, `session.ts`,
+ *     `terminalBackend/local.ts`, `terminalBackend/metadata.ts`) import
+ *     `surfaceCtx` from `./surfaceCtx.ts` — not from here — and call
+ *     `surfaceCtx.cells.X.set(...)`, `.collections.X.upsert(k, v)`,
+ *     `.events.X.publish(i, p)`. The framework owns the apply+publish
+ *     chain; domain code never sees a channel name string. Routing the
+ *     ctx through `./surfaceCtx.ts` is what breaks the bidirectional
+ *     import cycle that would otherwise form (#1005).
  *
  * Publisher channel names are framework-derived: `<surface-key>:changed`
  * for cells, `<surface-key>:keys` + `<surface-key>:<key>` for collections,
@@ -28,7 +32,7 @@ import {
   implementSurface,
   publisherChannel,
 } from "@kolu/surface/server";
-import { implement, ORPCError } from "@orpc/server";
+import { implement } from "@orpc/server";
 import { contract } from "kolu-common/contract";
 import { TerminalNotFoundError } from "kolu-common/errors";
 import type {
@@ -42,11 +46,9 @@ import {
   type FsReadFileOutput,
   fsListAllOutputEqual,
   fsReadFileOutputEqual,
-  type GitResult,
   gitDiffOutputEqual,
   gitStatusOutputEqual,
 } from "kolu-git";
-import { match } from "ts-pattern";
 import {
   buildIframePreviewUrl,
   isIframePreviewable,
@@ -55,17 +57,9 @@ import { log } from "./log.ts";
 import { publisher } from "./publisher.ts";
 import { cancelPendingAutosave, getSavedSession } from "./session.ts";
 import { store } from "./state.ts";
-// Load-order is cycle-sensitive: `terminalBackend/index.ts` must finish
-// loading (so `localTerminalBackend` is initialized) before line 61 below
-// calls `getTerminalBackendFor`. `terminal-registry.ts` participates in the
-// surface cycle via `local.ts`; if its import runs before
-// `terminalBackend/index.ts`, the cycle reaches line 61 in a state where
-// `localTerminalBackend` is still in TDZ. Biome's alphabetical sort would
-// swap these two lines and break the production build.
-// biome-ignore-start assist/source/organizeImports: cycle-sensitive load order
-import { getTerminalBackendFor } from "./terminalBackend/index.ts";
+import { setSurfaceCtx } from "./surfaceCtx.ts";
 import { getTerminal, listTerminals } from "./terminal-registry.ts";
-// biome-ignore-end assist/source/organizeImports: cycle-sensitive load order
+import { getTerminalBackendFor } from "./terminalBackend/index.ts";
 
 const localBackend = getTerminalBackendFor({ kind: "local" });
 
@@ -89,35 +83,6 @@ const savedSessionStore: CellStore<SavedSession | null> =
 
 // ── Surface implementation ─────────────────────────────────────────────
 
-/** Unwrap a `GitResult` or throw an `ORPCError` for the client. Shared
- *  with the raw git handlers in `router.ts`. */
-export function unwrapGit<T>(result: GitResult<T>): T {
-  if (result.ok) return result.value;
-  const { status, message } = match(result.error)
-    .with({ code: "BASE_BRANCH_NOT_FOUND" }, (e) => ({
-      status: "PRECONDITION_FAILED" as const,
-      message: e.message,
-    }))
-    .with({ code: "WORKTREE_NAME_COLLISION" }, (e) => ({
-      status: "CONFLICT" as const,
-      message: e.message,
-    }))
-    .with({ code: "PATH_ESCAPES_ROOT" }, (e) => ({
-      status: "INTERNAL_SERVER_ERROR" as const,
-      message: `path escapes root: ${e.child}`,
-    }))
-    .with({ code: "GIT_FAILED" }, (e) => ({
-      status: "INTERNAL_SERVER_ERROR" as const,
-      message: e.message,
-    }))
-    .with({ code: "NOT_A_REPO" }, () => ({
-      status: "INTERNAL_SERVER_ERROR" as const,
-      message: "Not a git repository",
-    }))
-    .exhaustive();
-  throw new ORPCError(status, { message });
-}
-
 const { router: surfaceRouterFragment, ctx: surfaceCtxBuilt } =
   implementSurface(surface, {
     channel: <T>(name: string) => publisherChannel<T>(publisher, name),
@@ -288,4 +253,4 @@ const { router: surfaceRouterFragment, ctx: surfaceCtxBuilt } =
   });
 
 export const surfaceRouter = surfaceRouterFragment;
-export const surfaceCtx = surfaceCtxBuilt;
+setSurfaceCtx(surfaceCtxBuilt);

packages/server/src/surfaceCtx.ts

@@ -0,0 +1,90 @@
+/**
+ * Late-bound holder for the typed surface mutation context.
+ *
+ * `surface.ts` calls `setSurfaceCtx(built)` once at startup, right after
+ * `implementSurface(...)` returns its `{ router, ctx }` pair. Domain
+ * modules — `activity.ts`, `session.ts`, `terminalBackend/local.ts`,
+ * `terminalBackend/metadata.ts` — import `surfaceCtx` from here instead
+ * of from `surface.ts`. The bidirectional edge that used to form between
+ * `surface.ts` and every domain module collapses to a one-way arrow
+ * (`surface.ts → domain`) plus a one-way registration
+ * (`surface.ts → surfaceCtx.ts`).
+ *
+ * Without this holder, biome's `noImportCycles` flags every domain
+ * module's `surfaceCtx` import, and the production Node ESM loader can
+ * land on an import order where `surface.ts`'s top-level
+ * `getTerminalBackendFor({ kind: "local" })` runs while
+ * `localTerminalBackend` is still in TDZ — production crashes that
+ * vite-node's evaluation order does not reproduce in unit tests (#1005).
+ *
+ * The Proxy throws on access before `setSurfaceCtx` has been called.
+ * If a domain module ever moves a `surfaceCtx.X` access from a function
+ * body to the module top level, the throw surfaces it at startup rather
+ * than yielding `undefined` and crashing later.
+ */
+
+import type { SurfaceCtx } from "@kolu/surface/server";
+import type { surface } from "kolu-common/surface";
+
+type Ctx = SurfaceCtx<(typeof surface)["spec"]>;
+
+let held: Ctx | undefined;
+
+export function setSurfaceCtx(ctx: Ctx): void {
+  // Process singleton — `surface.ts` calls this exactly once at startup.
+  // Throwing on a *different* ctx (rather than on any second call) keeps
+  // the invariant honest while staying tolerant of an accidental same-ctx
+  // re-registration from a future test or hot-reload scenario.
+  if (held !== undefined && held !== ctx) {
+    throw new Error(
+      "setSurfaceCtx called twice with different contexts — surface.ts must call this exactly once",
+    );
+  }
+  held = ctx;
+}
+
+/** Reset the held ctx to `undefined`. Only for use in unit tests that
+ *  need to supply a fresh mock ctx per test without hitting the
+ *  double-call guard. Production code must never call this. */
+export function __resetSurfaceCtxForTest(): void {
+  held = undefined;
+}
+
+/** A no-op ctx for unit tests that exercise code paths touching
+ *  `surfaceCtx` but don't care about surface publish side-effects
+ *  (e.g. metadata publish-routing tests, session persistence tests).
+ *  Cast via `unknown` because the full spec-typed Ctx would require
+ *  listing every cell/collection/event; the cast is localised here
+ *  so callers stay readable. */
+export function noopSurfaceCtxForTest(): Ctx {
+  const noop = () => {};
+  const noopReadAll = () => new Map();
+  const noopReadOne = () => undefined;
+  return {
+    cells: new Proxy({} as Ctx["cells"], {
+      get: () => ({ get: noopReadOne, set: noop, patch: noop }),
+    }),
+    collections: new Proxy({} as Ctx["collections"], {
+      get: () => ({
+        upsert: noop,
+        remove: noop,
+        readAll: noopReadAll,
+        readOne: noopReadOne,
+      }),
+    }),
+    events: new Proxy({} as Ctx["events"], {
+      get: () => ({ publish: noop }),
+    }),
+  } as unknown as Ctx;
+}
+
+export const surfaceCtx: Ctx = new Proxy({} as Ctx, {
+  get(_, prop) {
+    if (!held) {
+      throw new Error(
+        `surfaceCtx accessed before surface.ts initialized it (.${String(prop)})`,
+      );
+    }
+    return Reflect.get(held, prop);
+  },
+});

packages/server/src/terminalBackend/local.ts

@@ -51,7 +51,7 @@ import { trackRecentAgent, trackRecentRepo } from "../activity.ts";
 import { koluShellDir } from "../koluRoot.ts";
 import { log } from "../log.ts";
 import { terminalChannels, terminalsDirtyChannel } from "../publisher.ts";
-import { surfaceCtx, unwrapGit } from "../surface.ts";
+import { surfaceCtx } from "../surfaceCtx.ts";
 import {
   drainTerminals,
   getTerminal,
@@ -61,6 +61,7 @@ import {
   unregisterTerminal,
 } from "../terminal-registry.ts";
 import { cleanupTerminalScratch } from "../terminalScratch.ts";
+import { unwrapGit } from "../unwrapGit.ts";
 import {
   createMetadata,
   updateServerLiveMetadata,

packages/server/src/terminalBackend/metadata.test.ts

@@ -11,6 +11,11 @@
 import { afterEach, beforeEach, describe, expect, it } from "vitest";
 import { terminalsDirtyChannel } from "../publisher.ts";
 import type { TerminalProcess } from "../terminal-registry.ts";
+import {
+  __resetSurfaceCtxForTest,
+  noopSurfaceCtxForTest,
+  setSurfaceCtx,
+} from "../surfaceCtx.ts";
 import {
   updateClientMetadata,
   updateServerLiveMetadata,
@@ -48,6 +53,9 @@ async function settle(): Promise<void> {
 }
 
 beforeEach(async () => {
+  // surface.ts is not imported by this test module; supply a no-op ctx
+  // so calls to publishSnapshot (via surfaceCtx.collections…upsert) don't throw.
+  setSurfaceCtx(noopSurfaceCtxForTest());
   dirtyCount = 0;
   stopWatch = terminalsDirtyChannel.consume({
     onEvent: () => {
@@ -65,6 +73,7 @@ afterEach(() => {
   // keep firing into the shared `dirtyCount` and bystander tests see
   // inflated counts.
   stopWatch?.();
+  __resetSurfaceCtxForTest();
 });
 
 describe("metadata publish routing", () => {

packages/server/src/terminalBackend/metadata.ts

@@ -37,7 +37,7 @@ import type {
 import { prUnavailableReason, prValue } from "kolu-github/schemas";
 import { log } from "../log.ts";
 import { terminalsDirtyChannel } from "../publisher.ts";
-import { surfaceCtx } from "../surface.ts";
+import { surfaceCtx } from "../surfaceCtx.ts";
 import type { TerminalProcess } from "../terminal-registry.ts";
 
 /** Create initial metadata state for a new terminal. `lastActivityAt: 0`