From 308fe94a20a73e4186b81245f7449e6abdd2333c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Kwa=C5=9Bniak?= Date: Wed, 10 Jun 2026 20:29:20 +0200 Subject: [PATCH] refactor: break import cycles; add module-boundary law + design doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First PR of the module-isolation series (see docs/design/module-isolation-2026-06.md, distilled from a full import-graph audit): - src/version.ts extracted — orchestrator.ts and cli.ts imported VERSION from index.ts while index.ts imported run() back from the orchestrator. The cycle only worked because a hoisted const survives ESM live-binding order. - RunOptions/RunSummary declarations moved to orchestrator/options.ts so prepare.ts no longer imports upward from its own module entry. - tests/module-boundaries.test.ts — the dependency matrix is now law, enforced by the standard test gate. Both rules active: matrix membership (zero exceptions needed — the layering already held everywhere except the two cycles) and contracted-module index-only imports (ratchet; grows as contract PRs land). - oxlint import/no-cycle enabled as a secondary check (verified it fires on a deliberate cycle before trusting it). Zero behavior change; public API byte-identical. --- .oxlintrc.json | 3 +- docs/design/module-isolation-2026-06.md | 139 ++++++++++++++++++++++++ src/cli.ts | 2 +- src/index.ts | 2 +- src/orchestrator.ts | 45 +------- src/orchestrator/options.ts | 47 ++++++++ src/orchestrator/prepare.ts | 2 +- src/version.ts | 4 + tests/module-boundaries.test.ts | 100 +++++++++++++++++ 9 files changed, 298 insertions(+), 46 deletions(-) create mode 100644 docs/design/module-isolation-2026-06.md create mode 100644 src/orchestrator/options.ts create mode 100644 src/version.ts create mode 100644 tests/module-boundaries.test.ts diff --git a/.oxlintrc.json b/.oxlintrc.json index ae44c9d..f04ee4e 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -8,7 +8,8 @@ "no-unused-vars": "warn", "unicorn/no-useless-spread": "off", "typescript/unbound-method": "off", - "typescript/await-thenable": "off" + "typescript/await-thenable": "off", + "import/no-cycle": "error" }, "ignorePatterns": ["node_modules", "dist"] } diff --git a/docs/design/module-isolation-2026-06.md b/docs/design/module-isolation-2026-06.md new file mode 100644 index 0000000..2cd5cd4 --- /dev/null +++ b/docs/design/module-isolation-2026-06.md @@ -0,0 +1,139 @@ +# Module isolation — design + +> **Status:** accepted, in progress (June 2026) +> **Feeds:** Phase 4 of `architecture-overhaul-2026-06.md` +> **Principle source:** PR #108 (modules-first), adopted in place — no rebuild. + +## What we're solving + +`src/` already has directory-shaped concerns (`workspace/`, `graph/`, +`cache/`, `exec/`, `orchestrator/`, `cli/`, `util/`), but nothing +enforces them: 30+ cross-directory imports target internal files +directly, two import cycles exist (one through `index.ts`, one inside +orchestrator), three orchestrator files are actually workspace +concerns, and one file (`execute-task.ts`) mixes execution glue with +cache-key derivation that two other files reach in for. This doc is +(1) the audit, (2) the target module contracts, (3) the mechanical PR +series. + +## 1. Audit findings (2026-06-10, tip of main) + +### Cycles + +1. **`index.ts` ↔ `orchestrator.ts`** (and `cli.ts → index.ts`): + `orchestrator.ts` / `cli.ts` import `VERSION` from `./index.js` + while `index.ts` imports `run` from `./orchestrator.js`. Works only + because `VERSION` is a hoisted const under ESM live bindings. + Fix: extract `src/version.ts`. +2. **`orchestrator.ts` ↔ `orchestrator/prepare.ts`** (file-level, + type-only): `prepare.ts` imports `RunOptions` upward from the + module entry. Fix: move `RunOptions`/`RunSummary` to + `orchestrator/options.ts`; entry re-exports. + +No other cycles at file or directory level. + +### Deep imports past would-be boundaries + +- `cli/run.ts → orchestrator/plan-format.ts` — plan rendering is CLI + presentation living in the wrong module; move to `cli/`. +- `cli/cache.ts → cache/cache.ts` — legitimate composition, must flow + through the cache contract once it exists. +- `plan.ts`/`prepare.ts → execute-task.ts` for the hashing surface + only — evidence `execute-task.ts` hosts two concerns; split + `task-hash.ts`. +- All other cross-directory edges are directionally fine and need + only re-pointing at module index files. + +### Type homes + +| Type | Verdict | +| --------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `TaskNode`, `TaskOutcome`, `TaskStatus` | Stay in `graph` — they are the graph/scheduler's product. The cache-flavored statuses are opaque to the scheduler (documented; a generic `TaskOutcome` is ceremony for one consumer). | +| `ProjectEntry` | Move to `workspace` — it's "discovered project + loaded config", the joint output of discovery + loading. Today's home in graph forces `nested-dirs` (pure workspace geometry) to import from graph. | +| `CacheLayer` + friends | Stay in `cache/cache.ts`, exported via `cache/index.ts`. | +| `RunOptions`/`RunSummary` | Move declarations to `orchestrator/options.ts` (kills cycle 2). | +| `VERSION` | Move to `src/version.ts` (kills cycle 1); `index.ts` re-export keeps the public name. | + +### Mixed-concern files + +- `orchestrator/execute-task.ts` → split the hashing surface + (`computeTaskHash`, `computeGroupHash`, `hashTaskConfig`, + `hashProjectPackageJson`, `createHashCache`) into + `orchestrator/task-hash.ts`. NOT into `cache/` — key-part selection + composes graph types; pushing it down would force `cache → graph`. +- `orchestrator/nested-dirs.ts`, `orchestrator/fingerprint.ts` → + move to `workspace/` (workspace geometry / workspace-file hashing). +- `orchestrator/plan-format.ts` → move to `cli/` (pure presentation; + orchestrator keeps the `RunPlan` data types). +- `orchestrator/upstream.ts` stays — same `cache → graph` argument as + task-hash. + +## 2. Target design + +Eight modules. A module is a directory with an `index.ts` contract, +or a single root file when it has no internals to hide. + +| Module | Form | Contract highlights | +| -------------- | -------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `util` | dir + index | `UserError`, `xxh3*`, `relPosix`/`toPosix`, `ulid` | +| `config` | single file `src/config.ts` | schema types + `defineProject`/`defineWorkspace`. Stays at ROOT — cache/exec/graph/workspace all consume it; nesting under workspace inverts direction | +| `workspace` | dir + index | discovery, loaders, package-graph, filter, affected, `ProjectEntry` (moved in), `computeNestedProjectDirs` (moved in), `computeWorkspaceFingerprint` (moved in) | +| `graph` | dir + index | task-graph, scheduler, dependency-spec, `TaskNode`/`TaskOutcome`/`TaskStatus` | +| `cache` | dir + index | `Cache`, `CacheLayer`, `LayeredCache`, `RemoteCache`, inputs/outputs resolution. `tar.ts` stays internal | +| `exec` | dir + index | runner, env, sandbox-runtime | +| `orchestrator` | dir + index (entry moves to `orchestrator/run.ts`) | `run`, `planRun`, options/summary types, `Logger`, `defaultLogger`, `RunPlan` types | +| `cli` | dir + index (`cli.ts` moves in) | dispatcher + test-facing parser exports | + +Root files outside the module set: `bin.ts`, `index.ts` (public +façade — exports unchanged), `version.ts` (new). + +### Allowed dependency matrix (rows import columns, via index only) + +| | util | config | version | workspace | graph | cache | exec | orchestrator | cli | +| ---------------- | ---- | ------ | ------- | --------- | ----- | ----- | ---- | ------------ | --- | +| **workspace** | ✓ | ✓ | | — | | | | | | +| **graph** | ✓ | ✓ | | ✓ | — | | | | | +| **cache** | ✓ | ✓ | | | | — | | | | +| **exec** | ✓ | ✓ | | | | | — | | | +| **orchestrator** | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | — | | +| **cli** | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | | ✓ | — | +| **index** | | ✓ | ✓ | | ✓ | | | ✓ | | +| **bin** | ✓ | | | | | | | | ✓ | + +Composition happens only at `orchestrator` and `cli`. `cli → cache` +stays (prune/stats open the cache without a run). `cli → exec` is +deliberately absent. + +### Enforcement + +Primary: `tests/module-boundaries.test.ts` — scans `src/**/*.ts` +import specifiers, asserts (rule 1) every cross-module edge is in the +ALLOWED matrix, and (rule 2) imports of modules listed in a +`CONTRACTED` set target the module's `index.ts` only. `CONTRACTED` +starts empty and grows as contract PRs land — a ratchet. Tests in +`tests/` are exempt (they may exercise internals). Zero new deps; +runs under the existing `bun test` CI gate. + +Secondary: `import/no-cycle` in oxlint, if available — verify it +fires before trusting it; the boundary test catches directory-level +cycles either way. + +## 3. Migration plan (each PR: zero behavior change, suite green) + +| PR | What | +| --- | ------------------------------------------------------------------------------------------------------------------- | +| 1 | This doc + boundary test (matrix law from day one) + cycle breaks: `version.ts`, `orchestrator/options.ts` | +| 2 | Relocations: `ProjectEntry` → workspace; `nested-dirs`, `fingerprint` → workspace; `plan-format` → cli | +| 3 | Split `execute-task.ts` hashing surface → `orchestrator/task-hash.ts` | +| 4 | Contracts: `cache/index.ts`, `exec/index.ts`, `util/index.ts`; rewrite importers; CONTRACTED += {cache, exec, util} | +| 5 | Contracts: `workspace/index.ts`, `graph/index.ts`; CONTRACTED += {workspace, graph} | +| 6 | Entries: `orchestrator.ts` → `orchestrator/{index,run}.ts`; `cli.ts` → `cli/index.ts`; CONTRACTED complete | +| 7 | Docs: architecture.md module map, modules/ pages, CLAUDE.md layout | + +## Out of scope + +Test colocation (tests stay in `tests/`); renaming public exports; +splitting `cache/cache.ts` internals; exec's `RunOptions` rename +(flagged, optional); package-level splitting; any behavior change. +Cache key derivation untouched — no CACHE_VERSION bump anywhere in +the series. diff --git a/src/cli.ts b/src/cli.ts index a30a840..7d2d2b7 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -2,7 +2,7 @@ // `src/cli/.ts`. Re-exports below are for the test suite, // which asserts on the pure parsers and formatters directly. -import { VERSION } from './index.js' +import { VERSION } from './version.js' import { runCmd } from './cli/run.js' import { watchCmd } from './cli/watch.js' import { cacheCmd } from './cli/cache.js' diff --git a/src/index.ts b/src/index.ts index d1e809c..3949c88 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,6 +1,6 @@ // Public API for @vzn/vx. -export const VERSION = '0.0.0' +export { VERSION } from './version.js' // Schema types and helpers (used by user vx.config files and presets). export type { diff --git a/src/orchestrator.ts b/src/orchestrator.ts index aff4c09..a3fdbf7 100644 --- a/src/orchestrator.ts +++ b/src/orchestrator.ts @@ -4,7 +4,7 @@ import type { RunRecord } from './cache/cache.js' import { LayeredCache } from './cache/layered-cache.js' -import { VERSION } from './index.js' +import { VERSION } from './version.js' import { runGraph, type TaskOutcome } from './graph/scheduler.js' import { isGroupTask } from './graph/task-graph.js' import { ulid } from './util/ulid.js' @@ -20,47 +20,8 @@ import { writeRunProfile, writeRunSummary } from './orchestrator/run-artifacts.t import { formatRunSummary } from './orchestrator/summary.ts' export type { Logger } from './orchestrator/logger.ts' - -export interface RunOptions { - cwd: string - /** - * Task specs to run. Each may be a bare task name (`'build'`) — - * applied across `projects` to every project that declares it — - * or an anchored `'pkg#task'` — added directly to the requested - * set regardless of `projects`. - */ - tasks: readonly string[] - projects?: string[] - concurrency?: number - /** Skip cache reads AND writes. Every task runs and nothing is persisted. */ - noCache?: boolean - /** - * Filter `dependsOn` expansion. `'all'` drops every edge (just the - * requested task runs). A string array drops only those task names - * from both `self` and `dependencies` buckets. - */ - excludeDependencies?: 'all' | readonly string[] - /** Forwarded to the last step of each task's exec array (shell-quoted). */ - forwardArgs?: readonly string[] - /** - * If set, write a per-run JSON summary at end of run. Empty string - * picks the default path `/runs/.json`; anything - * else is treated as the literal file path (cwd-relative). - */ - summarize?: string - /** - * If set, write a Chrome-trace JSON profile of the run's wallclock - * spans. Path is cwd-relative. Default `profile.json` is selected - * by the CLI parser, not here. - */ - profile?: string - log?: Logger -} - -export interface RunSummary { - ok: boolean - outcomes: TaskOutcome[] -} +export type { RunOptions, RunSummary } from './orchestrator/options.ts' +import type { RunOptions, RunSummary } from './orchestrator/options.ts' export async function run(options: RunOptions): Promise { // Color decision: a custom logger (tests, embedders) handles its diff --git a/src/orchestrator/options.ts b/src/orchestrator/options.ts new file mode 100644 index 0000000..2ded829 --- /dev/null +++ b/src/orchestrator/options.ts @@ -0,0 +1,47 @@ +// Run option/summary types live in a leaf file (not the module entry) +// so internals like prepare.ts can import them without an upward +// import of orchestrator.ts — the module entry must stay cycle-free. + +import type { TaskOutcome } from '../graph/scheduler.js' +import type { Logger } from './logger.ts' + +export interface RunOptions { + cwd: string + /** + * Task specs to run. Each may be a bare task name (`'build'`) — + * applied across `projects` to every project that declares it — + * or an anchored `'pkg#task'` — added directly to the requested + * set regardless of `projects`. + */ + tasks: readonly string[] + projects?: string[] + concurrency?: number + /** Skip cache reads AND writes. Every task runs and nothing is persisted. */ + noCache?: boolean + /** + * Filter `dependsOn` expansion. `'all'` drops every edge (just the + * requested task runs). A string array drops only those task names + * from both `self` and `dependencies` buckets. + */ + excludeDependencies?: 'all' | readonly string[] + /** Forwarded to the last step of each task's exec array (shell-quoted). */ + forwardArgs?: readonly string[] + /** + * If set, write a per-run JSON summary at end of run. Empty string + * picks the default path `/runs/.json`; anything + * else is treated as the literal file path (cwd-relative). + */ + summarize?: string + /** + * If set, write a Chrome-trace JSON profile of the run's wallclock + * spans. Path is cwd-relative. Default `profile.json` is selected + * by the CLI parser, not here. + */ + profile?: string + log?: Logger +} + +export interface RunSummary { + ok: boolean + outcomes: TaskOutcome[] +} diff --git a/src/orchestrator/prepare.ts b/src/orchestrator/prepare.ts index 1016cf2..128d197 100644 --- a/src/orchestrator/prepare.ts +++ b/src/orchestrator/prepare.ts @@ -29,7 +29,7 @@ import { computeWorkspaceFingerprint } from './fingerprint.js' import { wrapWithRemoteCache } from './remote-cache-setup.js' import { createHashCache, type HashCache } from './execute-task.js' import type { Logger } from './logger.js' -import type { RunOptions } from '../orchestrator.js' +import type { RunOptions } from './options.js' export interface PreparedRun { workspaceRoot: string diff --git a/src/version.ts b/src/version.ts new file mode 100644 index 0000000..b0d29f1 --- /dev/null +++ b/src/version.ts @@ -0,0 +1,4 @@ +// Leaf module so orchestrator/cli can read the version without +// importing the public façade (index.ts imports the orchestrator — +// that edge must stay one-directional). +export const VERSION = '0.0.0' diff --git a/tests/module-boundaries.test.ts b/tests/module-boundaries.test.ts new file mode 100644 index 0000000..74a0662 --- /dev/null +++ b/tests/module-boundaries.test.ts @@ -0,0 +1,100 @@ +// Module-boundary law. See docs/design/module-isolation-2026-06.md. +// +// Rule 1: a file in module A may import from module B only when the +// ALLOWED matrix grants A → B. +// Rule 2: once a module is listed in CONTRACTED, cross-module imports +// of it must target its index.ts (the contract), never an +// internal file. CONTRACTED is a ratchet — it grows as the +// contract PRs land and never shrinks. +// +// Only src/ is scanned. Tests are exempt: they may exercise internals. + +import path from 'node:path' +import { describe, expect, it } from 'bun:test' + +const SRC = path.join(import.meta.dir, '..', 'src') + +/** Module of a src-relative file path. Root files are their own modules. */ +function moduleOf(rel: string): string { + const seg = rel.split('/') + if (seg.length > 1) return seg[0]! + return rel.replace(/\.ts$/, '') // bin / index / cli / orchestrator / config / version +} + +const ALLOWED: Record = { + util: [], + config: [], + version: [], + workspace: ['util', 'config'], + graph: ['util', 'config', 'workspace'], + cache: ['util', 'config'], + exec: ['util', 'config'], + orchestrator: ['util', 'config', 'version', 'workspace', 'graph', 'cache', 'exec'], + cli: ['util', 'config', 'version', 'workspace', 'graph', 'cache', 'orchestrator'], + index: ['config', 'version', 'graph', 'orchestrator'], + bin: ['util', 'cli'], +} + +// Modules whose contract (index.ts) is the only legal cross-module +// import target. Grows per migration PR; target: every directory module. +const CONTRACTED: readonly string[] = [] + +interface Edge { + from: string + to: string + fromModule: string + toModule: string + specifier: string +} + +async function collectEdges(): Promise { + const edges: Edge[] = [] + const glob = new Bun.Glob('**/*.ts') + for await (const rel of glob.scan({ cwd: SRC })) { + const norm = rel.split(path.sep).join('/') + const text = await Bun.file(path.join(SRC, rel)).text() + // Static `import ... from 'x'` / `export ... from 'x'` specifiers. + // No dynamic imports exist on boundary paths today; if one appears + // the matrix below is the place to encode the decision. + for (const m of text.matchAll(/^(?:import|export)[^'"]*from\s+['"]([^'"]+)['"]/gm)) { + const spec = m[1]! + if (!spec.startsWith('.')) continue // bare imports = packages, not modules + const resolved = path + .normalize(path.join(path.dirname(norm), spec)) + .split(path.sep) + .join('/') + .replace(/\.(js|ts)$/, '') + const fromModule = moduleOf(norm) + const toModule = moduleOf(`${resolved}.ts`) + if (fromModule === toModule) continue + edges.push({ from: norm, to: resolved, fromModule, toModule, specifier: spec }) + } + } + return edges +} + +describe('module boundaries', () => { + it('every cross-module import edge is in the ALLOWED matrix', async () => { + const edges = await collectEdges() + expect(edges.length).toBeGreaterThan(0) + const violations = edges.filter((e) => { + const allowed = ALLOWED[e.fromModule] + // Unknown module = new top-level file/dir; force a matrix decision. + if (allowed === undefined) return true + return !allowed.includes(e.toModule) + }) + expect( + violations.map((v) => `${v.from} → ${v.specifier} (${v.fromModule} → ${v.toModule})`), + ).toEqual([]) + }) + + it('contracted modules are imported only via their index', async () => { + const edges = await collectEdges() + const violations = edges.filter( + (e) => CONTRACTED.includes(e.toModule) && e.to !== `${e.toModule}/index`, + ) + expect( + violations.map((v) => `${v.from} → ${v.specifier} (must import ${v.toModule}/index)`), + ).toEqual([]) + }) +})