From 64b52a51d7c06c357201af99bb527c74c1d46b5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 1 Jul 2026 07:59:04 +0200 Subject: [PATCH] feat: enforce import-direction DAG (Phase-5 layering lint) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalize the inline CI "Layering Guard" grep into a structured import-direction lint (scripts/layering/check.ts) over the resolved import graph, per plans/perfect-shape.md §5.5. The full target DAG (kernel ◄ platforms ◄ core ◄ commands ◄ {cli, client, daemon/server}; client ◄ daemon/client) is only partly realized — the client/remote/metro extraction, the daemon/server split, and the utils dissolution are still pending Phase-5 moves, so the tree still holds legitimate back-edges (platforms→core, commands→cli, utils→*). Enforcing the whole DAG today would need a mass import rewrite that Phase 5 defers. The lint therefore enforces the three invariants the completed moves (kernel/, daemon/client/) already guarantee and that are green today: R1 kernel-sink — nothing under src/kernel/ imports another zone, except the one type-only kernel→contracts re-export. R2 commands-floor — nothing below the command surface (kernel, platforms, core, daemon) imports src/commands/. Generalizes the former guard (daemon + platforms). R3 platforms-seam — platforms/ is statically imported only at the core interactor seam (src/core/interactors/) and by the daemon server; elsewhere use a dynamic import() or a type-only import, preserving CLI cold-start. Dynamic import('../platforms/*') and `import type` stay allowed. Fixes the three pre-existing R3 violations by converting static platforms value imports to dynamic imports (all in already-async call sites, behavior-preserving and cold-start-improving): - src/client/client.ts debug.symbols → lazy symbolicateCrashArtifact - src/cli/commands/web.ts setup/doctor → lazy agent-browser-tool - src/core/dispatch-interactions.ts runner-sequence → lazy (matches the file's own dynamic-import pattern) Wire the check into the Layering Guard CI job and add a check:layering package.json script (also folded into check:tooling). scripts/layering/** is excluded from fallow (untested CI script, like scripts/perf/**). --- .fallowrc.json | 1 + .github/workflows/ci.yml | 29 ++-- package.json | 3 +- scripts/layering/check.ts | 249 ++++++++++++++++++++++++++++++ src/cli/commands/web.ts | 15 +- src/client/client.ts | 8 +- src/core/dispatch-interactions.ts | 7 +- 7 files changed, 276 insertions(+), 36 deletions(-) create mode 100644 scripts/layering/check.ts diff --git a/.fallowrc.json b/.fallowrc.json index 21746458c..979bc358a 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -27,6 +27,7 @@ "ignorePatterns": [ "examples/test-app/**", "scripts/perf/**", + "scripts/layering/**", "ios-runner/AgentDeviceRunner/AgentDeviceRunnerUITests.xctestplan", "scripts/write-xcuitest-cache-metadata.mjs" ], diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3ae17fd70..624192063 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -96,24 +96,17 @@ jobs: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - name: Reject commands/ imports below the CLI surface layer - run: | - # daemon/ and platforms/ should not directly depend on the CLI commands layer. - # This grep is a direct-import guard; deeper type-only transitive imports are handled - # by keeping shared daemon/client contracts outside src/commands. - set +e - matches=$(git grep -n -E "from '.*commands/" -- src/daemon src/platforms ':!src/**/__tests__/**' ':!*.test.ts') - grep_status=$? - set -e - if [ "$grep_status" -gt 1 ]; then - exit "$grep_status" - fi - if [ -n "$matches" ]; then - echo "$matches" - count=$(printf '%s\n' "$matches" | wc -l) - echo "::error title=Layering drift::$count import(s) of src/commands/* from src/daemon or src/platforms. Depend on shared contracts instead." - exit 1 - fi + - name: Setup toolchain + uses: ./.github/actions/setup-node-pnpm + with: + install-deps: false + + - name: Check import-direction DAG (kernel-sink, commands-floor, platforms-seam) + # Generalizes the former inline commands/-import grep into a structured + # import-direction lint over the resolved graph. See scripts/layering/check.ts + # and plans/perfect-shape.md §5.5. Invoked directly (no deps needed) so the + # job stays fast and does not require a pnpm install. + run: node --experimental-strip-types scripts/layering/check.ts fallow: name: Fallow Code Quality diff --git a/package.json b/package.json index fe275e4ed..232c733a2 100644 --- a/package.json +++ b/package.json @@ -108,11 +108,12 @@ "fallow:all": "fallow --summary", "fallow:baseline": "(fallow dead-code --save-baseline fallow-baselines/dead-code.json --summary || true) && (fallow health --save-baseline fallow-baselines/health.json --summary || true)", "check:fallow": "fallow audit", + "check:layering": "node --experimental-strip-types scripts/layering/check.ts", "check:quick": "pnpm lint && pnpm typecheck", "sync:mcp-metadata": "node scripts/sync-mcp-metadata.mjs", "check:mcp-metadata": "node scripts/sync-mcp-metadata.mjs --check", "version": "node scripts/sync-mcp-metadata.mjs && git add server.json", - "check:tooling": "pnpm lint && pnpm typecheck && pnpm check:mcp-metadata && pnpm build", + "check:tooling": "pnpm lint && pnpm typecheck && pnpm check:layering && pnpm check:mcp-metadata && pnpm build", "check:unit": "pnpm test:unit && pnpm test:smoke", "check": "pnpm check:tooling && pnpm check:fallow && pnpm check:unit", "prepack": "pnpm check:mcp-metadata && pnpm build:all && pnpm package:android-snapshot-helper:npm && pnpm package:android-multitouch-helper:npm", diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts new file mode 100644 index 000000000..0adacb1bf --- /dev/null +++ b/scripts/layering/check.ts @@ -0,0 +1,249 @@ +// Import-direction lint — enforces the folder DAG established by the Phase-5 +// folder moves (see plans/perfect-shape.md §5.5). +// +// This generalizes the former inline "Layering Guard" CI grep (which only +// checked that src/daemon and src/platforms do not import src/commands) into a +// structured check over the resolved import graph. +// +// Target DAG (imports point DOWN, toward the kernel sink): +// kernel ◄ platforms ◄ core ◄ commands ◄ { cli, client, daemon/server } +// client ◄ daemon/client remote, metro ◄ daemon/client +// sdk = re-export barrels only +// +// That full DAG is a *target*. Several Phase-5 folder moves are still pending +// (the client/remote/metro extraction, the daemon/server split, and the utils +// dissolution), so the tree still contains legitimate back-edges between +// folders that have not yet been separated (e.g. platforms→core, commands→cli, +// utils→*). Enforcing the whole DAG today would require a mass import rewrite, +// which Phase 5 explicitly defers. This lint therefore enforces only the three +// invariants that the *completed* moves (kernel/, daemon/client/) already +// guarantee and that hold green today: +// +// R1 kernel is the dependency sink — nothing under src/kernel/ imports +// another zone, except a type-only re-export from src/contracts/ (the one +// documented kernel→contracts type seam). +// R2 the command surface is a floor — nothing below it (kernel, platforms, +// core, daemon) imports src/commands/. Generalizes the former guard, +// which covered only daemon + platforms. +// R3 platforms/ is statically imported only at the interactor seam +// (src/core/interactors/) and by the daemon server (src/daemon/ minus +// src/daemon/client/). Everywhere else must reach platforms via a dynamic +// import() or a type-only import, which preserves CLI cold-start. +// +// Dynamic `import('../platforms/*')` and `import type` are always allowed by R3. +// +// Run: node --experimental-strip-types scripts/layering/check.ts + +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import path from 'node:path'; + +type ImportEdge = { + spec: string; + dynamic: boolean; + typeOnly: boolean; + line: number; +}; + +type EdgeContext = { + file: string; + fromTop: string; + toTop: string; + imp: ImportEdge; +}; + +type Violation = { + rule: string; + file: string; + line: number; + message: string; +}; + +const repoRoot = execFileSync('git', ['rev-parse', '--show-toplevel'], { + encoding: 'utf8', +}).trim(); + +function listSourceFiles(): string[] { + const out = execFileSync('git', ['ls-files', 'src/**/*.ts'], { + cwd: repoRoot, + encoding: 'utf8', + }); + return out + .split('\n') + .filter(Boolean) + .filter((f) => !/(?:^|\/)__tests__\//.test(f) && !/\.test\.ts$/.test(f)); +} + +// The DAG node ("zone") a src-relative path belongs to: its first path segment +// under src/, e.g. src/core/interactors/apple.ts → "core". +function topFolder(rel: string): string { + const match = /^src\/([^/]+)\//.exec(rel); + return match ? match[1] : '(root)'; +} + +function isCoreInteractor(rel: string): boolean { + return rel.startsWith('src/core/interactors/'); +} + +function isDaemonServer(rel: string): boolean { + return rel.startsWith('src/daemon/') && !rel.startsWith('src/daemon/client/'); +} + +// ── Import extraction ─────────────────────────────────────────────────────── + +function scanDynamicImports(line: string, lineNo: number): ImportEdge[] { + const edges: ImportEdge[] = []; + const re = /import\s*\(\s*['"]([^'"]+)['"]/g; + let match: RegExpExecArray | null; + while ((match = re.exec(line))) { + edges.push({ spec: match[1]!, dynamic: true, typeOnly: false, line: lineNo }); + } + return edges; +} + +function scanSideEffectImport(line: string, lineNo: number): ImportEdge | null { + const match = /^\s*import\s+['"]([^'"]+)['"]/.exec(line); + return match ? { spec: match[1]!, dynamic: false, typeOnly: false, line: lineNo } : null; +} + +// A `from '...'` clause on lines[i] (possibly the tail of a multi-line import). +// Walk back to the statement start to read the import/export keyword and whether +// it is a whole-statement `type` import. +function scanFromImport(lines: string[], i: number): ImportEdge | null { + const fromMatch = /(?:^|[\s;}])from\s+['"]([^'"]+)['"]/.exec(lines[i]!); + if (!fromMatch) return null; + + let start = i; + while (start >= 0 && !/^\s*(?:import|export)\b/.test(lines[start]!)) { + start--; + } + if (start < 0) return null; + + const typeOnly = /^\s*(?:import|export)\s+type\b/.test(lines[start]!); + return { spec: fromMatch[1]!, dynamic: false, typeOnly, line: start + 1 }; +} + +function parseImports(source: string): ImportEdge[] { + const lines = source.split('\n'); + const edges: ImportEdge[] = []; + for (let i = 0; i < lines.length; i++) { + edges.push(...scanDynamicImports(lines[i]!, i + 1)); + const sideEffect = scanSideEffectImport(lines[i]!, i + 1); + if (sideEffect) { + edges.push(sideEffect); + continue; + } + const fromImport = scanFromImport(lines, i); + if (fromImport) edges.push(fromImport); + } + return edges; +} + +function resolveTargetZone(fromFile: string, spec: string): string | null { + if (!spec.startsWith('.')) return null; // bare / external specifier + const resolved = path.normalize(path.join(path.dirname(fromFile), spec)); + if (!resolved.startsWith('src/')) return null; // escapes src/ + return resolved; +} + +// ── Rules ─────────────────────────────────────────────────────────────────── + +// R1 — kernel is the dependency sink. +function ruleKernelSink(ctx: EdgeContext): Violation | null { + if (ctx.fromTop !== 'kernel') return null; + if (ctx.toTop === 'contracts' && ctx.imp.typeOnly) return null; + return { + rule: 'R1 kernel-sink', + file: ctx.file, + line: ctx.imp.line, + message: + `kernel must not import ${ctx.toTop}/ (imports '${ctx.imp.spec}'). ` + + `The only allowed kernel out-edge is a type-only re-export from contracts/.`, + }; +} + +// R2 — the command surface is a floor. +const BELOW_COMMANDS = new Set(['kernel', 'platforms', 'core', 'daemon']); +function ruleCommandsFloor(ctx: EdgeContext): Violation | null { + if (ctx.toTop !== 'commands' || !BELOW_COMMANDS.has(ctx.fromTop)) return null; + return { + rule: 'R2 commands-floor', + file: ctx.file, + line: ctx.imp.line, + message: + `${ctx.fromTop}/ must not import the command surface commands/ ` + + `(imports '${ctx.imp.spec}'). Depend on shared kernel/contracts instead.`, + }; +} + +// R3 — platforms/ static value imports only at the interactor seam and the +// daemon server; everywhere else use a dynamic import() or a type-only import. +function rulePlatformsSeam(ctx: EdgeContext): Violation | null { + if (ctx.toTop !== 'platforms' || ctx.imp.dynamic || ctx.imp.typeOnly) return null; + if (isCoreInteractor(ctx.file) || isDaemonServer(ctx.file)) return null; + return { + rule: 'R3 platforms-seam', + file: ctx.file, + line: ctx.imp.line, + message: + `static value import of platforms/ from ${ctx.fromTop}/ (imports '${ctx.imp.spec}'). ` + + `Only src/core/interactors/ and the daemon server may statically import platforms/; ` + + `elsewhere use a dynamic import() or a type-only import to preserve CLI cold-start.`, + }; +} + +const RULES = [ruleKernelSink, ruleCommandsFloor, rulePlatformsSeam]; + +function checkFile(file: string): Violation[] { + const source = fs.readFileSync(path.join(repoRoot, file), 'utf8'); + const fromTop = topFolder(file); + const violations: Violation[] = []; + for (const imp of parseImports(source)) { + const target = resolveTargetZone(file, imp.spec); + if (target === null) continue; + const toTop = topFolder(target); + if (toTop === fromTop) continue; // intra-zone imports are always fine + const ctx: EdgeContext = { file, fromTop, toTop, imp }; + for (const rule of RULES) { + const violation = rule(ctx); + if (violation) violations.push(violation); + } + } + return violations; +} + +// ── Report ────────────────────────────────────────────────────────────────── + +function report(files: string[], violations: Violation[]): number { + if (violations.length === 0) { + process.stdout.write( + `Layering guard: OK — ${files.length} source files satisfy the import-direction DAG ` + + `(R1 kernel-sink, R2 commands-floor, R3 platforms-seam).\n`, + ); + return 0; + } + + const byRule = new Map(); + for (const v of violations) { + const bucket = byRule.get(v.rule) ?? []; + bucket.push(v); + byRule.set(v.rule, bucket); + } + + process.stderr.write(`Layering guard: ${violations.length} import-direction violation(s)\n\n`); + for (const [rule, group] of byRule) { + process.stderr.write(` [${rule}] ${group.length} violation(s):\n`); + for (const v of group) { + process.stderr.write(` ${v.file}:${v.line} — ${v.message}\n`); + process.stderr.write( + `::error file=${v.file},line=${v.line},title=Layering drift (${v.rule})::${v.message}\n`, + ); + } + process.stderr.write('\n'); + } + return 1; +} + +const sourceFiles = listSourceFiles(); +const allViolations = sourceFiles.flatMap(checkFile); +process.exit(report(sourceFiles, allViolations)); diff --git a/src/cli/commands/web.ts b/src/cli/commands/web.ts index 488515386..ca2a50d8f 100644 --- a/src/cli/commands/web.ts +++ b/src/cli/commands/web.ts @@ -1,8 +1,4 @@ -import { - doctorManagedAgentBrowser, - setupManagedAgentBrowser, - type AgentBrowserToolStatus, -} from '../../platforms/web/agent-browser-tool.ts'; +import type { AgentBrowserToolStatus } from '../../platforms/web/agent-browser-tool.ts'; import { AppError } from '../../kernel/errors.ts'; import type { CliFlags } from '../parser/cli-flags.ts'; import { printJson } from '../../utils/output.ts'; @@ -17,6 +13,8 @@ export async function runWebCommand( switch (action) { case 'setup': { printWebSetupStart(options.flags.json); + const { setupManagedAgentBrowser } = + await import('../../platforms/web/agent-browser-tool.ts'); const status = await setupManagedAgentBrowser({ stateDir: options.stateDir, }); @@ -24,6 +22,8 @@ export async function runWebCommand( return 0; } case 'doctor': { + const { doctorManagedAgentBrowser } = + await import('../../platforms/web/agent-browser-tool.ts'); const result = await doctorManagedAgentBrowser({ stateDir: options.stateDir, }); @@ -44,10 +44,7 @@ function printWebSetupStart(json: boolean | undefined): void { process.stdout.write('Setting up managed agent-browser backend (downloads if needed)...\n'); } -function printWebSetupResult( - json: boolean | undefined, - status: Awaited>, -): void { +function printWebSetupResult(json: boolean | undefined, status: AgentBrowserToolStatus): void { if (json) { printJson({ success: true, data: { status: toPublicAgentBrowserToolStatus(status) } }); return; diff --git a/src/client/client.ts b/src/client/client.ts index b866fbd19..b42640b0a 100644 --- a/src/client/client.ts +++ b/src/client/client.ts @@ -1,7 +1,6 @@ import { sendToDaemon } from '../daemon/client/daemon-client.ts'; import { prepareMetroRuntime, reloadMetro } from '../metro/client-metro.ts'; import { resolveDaemonPaths } from '../daemon/config.ts'; -import { symbolicateCrashArtifact } from '../platforms/apple/core/debug-symbols.ts'; import { INTERNAL_COMMANDS } from '../command-catalog.ts'; import { prepareDaemonCommandRequest, @@ -332,8 +331,11 @@ export function createAgentDeviceClient( network: async (options = {}) => await executeCommand('network', options), }, debug: { - symbols: async (options) => - await symbolicateCrashArtifact({ cwd: options.cwd ?? config.cwd, ...options }), + symbols: async (options) => { + const { symbolicateCrashArtifact } = + await import('../platforms/apple/core/debug-symbols.ts'); + return symbolicateCrashArtifact({ cwd: options.cwd ?? config.cwd, ...options }); + }, }, recording: { record: async (options) => await executeCommand('record', options), diff --git a/src/core/dispatch-interactions.ts b/src/core/dispatch-interactions.ts index c84eb7953..14b27b367 100644 --- a/src/core/dispatch-interactions.ts +++ b/src/core/dispatch-interactions.ts @@ -41,11 +41,6 @@ import { computeDeterministicJitter, runRepeatedSeries, } from './dispatch-series.ts'; -import { - MAX_RUNNER_SEQUENCE_STEPS, - buildRunnerSequenceCommand, - parseRunnerSequenceResult, -} from '../platforms/apple/core/runner/runner-sequence.ts'; import type { RunnerSequenceStep } from '../platforms/apple/core/runner/runner-contract.ts'; import type { DispatchContext } from './dispatch-context.ts'; import type { Interactor, RunnerCallOptions } from './interactor-types.ts'; @@ -347,6 +342,8 @@ async function runIosSequenceChunks( context: DispatchContext | undefined, ): Promise> { const { runIosRunnerCommand } = await import('../platforms/apple/core/runner/runner-client.ts'); + const { MAX_RUNNER_SEQUENCE_STEPS, buildRunnerSequenceCommand, parseRunnerSequenceResult } = + await import('../platforms/apple/core/runner/runner-sequence.ts'); const chunks = chunkRunnerSequenceStepsByBudget(steps, MAX_RUNNER_SEQUENCE_STEPS); let firstChunkRunnerResult: Record | undefined;