Skip to content

Promote noImportCycles from warn to error (after breaking the 9 server cycles) #1005

@srid

Description

@srid

Context

lint/suspicious/noImportCycles is currently at biome's default warn severity. There are 9 hits in packages/server/src/, all in a tangled cycle involving surface.tsterminalBackend/local.tsactivity.tssession.tsterminalBackend/metadata.ts:

packages/server/src/activity.ts:18              → surface.ts
packages/server/src/session.ts:16               → surface.ts
packages/server/src/surface.ts:56               → session.ts
packages/server/src/surface.ts:66               → terminalBackend/index.ts
packages/server/src/terminalBackend/index.ts:24 → terminalBackend/local.ts (transitive)
packages/server/src/terminalBackend/local.ts:71 → activity.ts
packages/server/src/terminalBackend/local.ts:75 → surface.ts
packages/server/src/terminalBackend/local.ts:90 → terminalBackend/metadata.ts (transitive)
packages/server/src/terminalBackend/metadata.ts:40 → surface.ts

Because the rule is warn, just check exits 0 in their presence. That gives no signal — and the cycles are a real footgun. #1003 (biome --write --unsafe chore) demonstrated the cost concretely: biome's alphabetical import sort reshuffled the order in surface.ts + terminals.ts, the cycle then converged via a different path, and localTerminalBackend ended up in TDZ when surface.ts:70's top-level const localBackend = getTerminalBackendFor({ kind: "local" }) ran. Production boot crashed; unit tests passed (vite-node uses a different module-evaluation order than Node's production ESM loader). Only just smoke caught it, on CI, after push.

The latent fragility had been visible as 9 yellow warnings for months. Nobody acted on them because warnings are ignorable.

What to do

Two-phase:

Phase 1 — break the cycles

Apply the surfaceCtx.ts holder pattern from the abandoned architecture PR #998 cycle 1 (see that PR's description, section "Cycle 1 — late-bound holders break server load-order cycles"). That work eliminated all 7 (then-)server noImportCycles violations by:

  • Extracting surfaceCtx.ts as a Proxy-fronted holder.
  • surface.ts populates the holder once at startup via setSurfaceCtx(built).
  • Domain modules (activity.ts, session.ts, terminalBackend/metadata.ts, etc.) import surfaceCtx from the holder, not from surface.ts.
  • Only surface.ts imports the domain modules.

Bidirectional arrow → one-way arrow + one-way registration. The same shape will work here; the cycle count is 9 today rather than 7 because the merge from master and subsequent commits added a couple more edges, but the structural fix is identical.

Phase 2 — promote the rule

Once just check reports zero noImportCycles hits, edit biome.jsonc to pin the severity:

{
  "linter": {
    "rules": {
      "suspicious": {
        "noImportCycles": "error"
      }
    }
  }
}

After this, any PR that introduces a cycle (or re-introduces one of these) fails just check immediately. The TDZ-on-reorder class becomes structurally impossible (no cycles ⇒ no evaluation-order accidents).

Acceptance

  • All 9 (or however many at the time) server noImportCycles hits resolved via the holder pattern.
  • biome.jsonc pins lint/suspicious/noImportCycles to error.
  • just check exits 0 at HEAD.
  • just smoke exits 0 at HEAD (confirms the production loader is happy with the new module graph).
  • Optional: a regression test or unit test that imports the server entrypoint via Node's production loader (not vite-node) — to catch the next "vite-node masks a real bug" class.

Why not just fix #1003's biome-ignore patches?

#1003 pins the import order with // biome-ignore-start assist/source/organizeImports markers as a stop-gap. That's reactive (after-the-fact, per-file, depends on remembering to add the marker every time). The structural fix — no cycles — is preventive and applies repo-wide for free once landed.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions