From ffd8316fca861e40a7dd4172cd100f4666249567 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 11:21:14 +0000 Subject: [PATCH] refactor(layering): declare R1-R3 as a policy table, and test them R1-R3 were three hand-written predicate functions. Each was short, but each buried its boundary in control flow: you had to read the early-returns to learn that R3 tolerates dynamic imports, or that R1 opens exactly one door. They are now data in scripts/layering/zone-policy.ts -- which zones a boundary governs, which import kinds it tolerates, which path prefixes are its declared seam -- walked by one small evaluator. A fourth zone boundary becomes a table entry rather than a fourth predicate to keep consistent with the other three. R1 is deliberately two entries rather than one with a special case, because "kernel may import contracts type-only" and "kernel may import nothing else at all" are two statements, and writing them separately is what makes the single open door visible. The refactor exposed a real gap: checkLayeringRules had NO unit test. The only thing exercising R1-R3 was the real tree, which is clean, so a rule that had silently stopped matching would have looked exactly like a rule being obeyed. zone-policy.test.ts now asserts each boundary fires and each documented exemption holds, including that src/daemon/client/ is excluded from the daemon seam. Verified end-to-end by injecting one violation per rule plus an exempt file: the gate reports 4 zone-policy violations (R3 twice, catching the daemon/client case), ignores the type-only and dynamic edges, and exits 1. Also records in docs/dependency-graph-findings.md the result of spiking eslint-plugin-boundaries under oxlint's jsPlugins, so nobody repeats it. It does work -- jsPlugins loads npm ESLint plugins with no ESLint install, and R1-R3 are all expressible once you know importKind: "value" and settings["boundaries/dependency-nodes"]. Not adopted: no ratchet mechanism (the thing that took R6 from 61 to 7 incrementally), it cannot express R4-R9 so the architecture would be defined in two places, it misreads inline `{ type Foo }` specifiers as value imports (one false positive on providers/limrun/android.ts), message interpolation renders empty under the current selector syntax, and it costs 230 transitive packages on an API documented as alpha. No behaviour change: 932 source files, R6 = 7, R7 = 41 fields, R8 clean, R9 = 102, all identical to before. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur --- .github/workflows/ci.yml | 2 +- CONTEXT.md | 7 +- docs/dependency-graph-findings.md | 45 +++++++++ package.json | 2 +- scripts/layering/check.ts | 86 ++++------------- scripts/layering/zone-policy.test.ts | 135 +++++++++++++++++++++++++++ scripts/layering/zone-policy.ts | 122 ++++++++++++++++++++++++ 7 files changed, 326 insertions(+), 73 deletions(-) create mode 100644 scripts/layering/zone-policy.test.ts create mode 100644 scripts/layering/zone-policy.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 05bec2d3cc..78cc3fe2e7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -110,7 +110,7 @@ jobs: # import-direction lint over the resolved graph. See scripts/layering/check.ts # and CONTEXT.md (Architecture: folder DAG + layering lint). run: | - node --experimental-strip-types --test scripts/layering/model.test.ts + node --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts node --experimental-strip-types scripts/layering/check.ts - name: Check the depgraph report agrees with the gate diff --git a/CONTEXT.md b/CONTEXT.md index 84fdc2e4c4..c45e7207b3 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -255,7 +255,12 @@ The perfect-shape refactor is complete and merged. Its end-state: follow-up surfaces. - Folder DAG + layering lint. `scripts/layering/check.ts` enforces five rules across four scopes in CI. GLOBALLY, across every production source file, it enforces the R1-R3 move rules (kernel-sink, - commands-floor, platforms-seam) and rejects all production static value-import cycles. Separately, + commands-floor, platforms-seam) and rejects all production static value-import cycles. R1-R3 are + declared as data in `scripts/layering/zone-policy.ts` (`ZONE_POLICIES`): which zones a boundary + governs, which import kinds it tolerates, and which path prefixes are its declared seam. A fourth + zone boundary is a table entry, not a fourth predicate. `zone-policy.test.ts` asserts each + boundary fires and each exemption holds — necessary because the tree is clean, so a rule that + stopped matching would look exactly like a rule being obeyed. Separately, it ranks an explicit target spine — as rank groups, lowest (kernel sink) to highest, where `A ◄ B` means B may not be outranked by A (the back-edge order the gate rejects), NOT that every displayed import exists: diff --git a/docs/dependency-graph-findings.md b/docs/dependency-graph-findings.md index 07896b743c..6913e1b1f9 100644 --- a/docs/dependency-graph-findings.md +++ b/docs/dependency-graph-findings.md @@ -387,6 +387,51 @@ Still duplicated across zones, each needing the same treatment: `fill requires t `settings clear-app-state requires an app id…` (`core/dispatch.ts` ↔ two platform modules), and `snapshot is not supported by this backend` (three files inside `commands/`). +## 7. `eslint-plugin-boundaries` under oxlint: evaluated, works, not adopted + +Spiked properly (installed, configured against the real tree, verified by injection) so nobody has +to repeat it. **It runs** — oxlint's `jsPlugins` loads npm ESLint plugins directly, so this needs no +ESLint install, no second config and no extra CI step: + +```json +{ "jsPlugins": [{ "name": "boundaries", "specifier": "eslint-plugin-boundaries" }] } +``` + +R1, R2 and R3 are all expressible, and all three were confirmed to fire on injected violations while +every documented exemption held. The two non-obvious settings that make R3 work: + +| need | mechanism | +|---|---| +| ignore type-only edges | `importKind: "value"` on the policy | +| ignore dynamic `import()` | `settings["boundaries/dependency-nodes"] = ["import", "export"]` | + +**Why we did not adopt it.** None of these is fatal alone; together they lose more than the +declarative syntax gains, and `ZONE_POLICIES` gets that syntax anyway: + +- **No ratchet.** The mechanism that took R6 from 61 → 7 across three PRs is a baseline that + tolerates N and fails on N+1. A per-file lint rule has no cross-run aggregate, so it cannot do + this. Any new boundary with existing violations — the platforms facade has ~89 — would need one + disable comment per site. +- **It cannot replace the gate.** R4 (cycles), R5/R6 (spine ranking + ratchet), R7 (field + ownership — not an import rule at all), R8 (CI job closure) and R9 (cycle size) are whole-graph or + non-import properties. Adopting it means R1-R3 live in one system and R4-R9 in another, so + "where is our architecture defined" gets two answers. +- **Inline type specifiers are misread.** `import { type A, type B } from '…'` is fully erased at + runtime, but the plugin classifies it as a value import (its `importKind` is statement-level). + That produced one real false positive here — `providers/limrun/android.ts` — flagged as an R3 + violation the gate correctly ignores. Only 10 of 1,481 type-only edges use this form, so the blast + radius is small, but `statementIsTypeOnly` in `model.ts` handles both spellings and the plugin + does not. +- **Message specificity regressed.** With the current non-deprecated selector syntax, + `{{dependency.type}}` and `{{file.type}}` interpolated to empty strings, so violations read + `must not import commands/` with no zone named. ADR 0010 wants every error actionable. +- **Cost.** 230 transitive packages, and `jsPlugins` is documented as "in alpha and not subject to + semver." A single config attempt produced five deprecation warnings (`mode`, `rules`, legacy + selectors, legacy templates, rule-level `importKind`) spanning two major migrations. + +Worth re-evaluating if the monorepo migration happens — per-package ESLint configs change the +calculus — or once `jsPlugins` is stable and the ratchet gap is addressable. + ## Suggested order from here 1. **Move the 10 outward-facing `daemon/types.ts` types into `contracts/`** (§2). Mechanical, and diff --git a/package.json b/package.json index 0da425b7d9..1837bfae1c 100644 --- a/package.json +++ b/package.json @@ -125,7 +125,7 @@ "check:affected:test": "node --experimental-strip-types --test scripts/check-affected/model.test.ts scripts/check-affected/run.test.ts", "check:coverage-changed": "node --experimental-strip-types scripts/coverage-changed/run.ts", "check:coverage-changed:test": "node --experimental-strip-types --test scripts/coverage-changed/model.test.ts scripts/coverage-changed/run.test.ts", - "check:layering": "node --experimental-strip-types --test scripts/layering/model.test.ts && node --experimental-strip-types scripts/layering/check.ts", + "check:layering": "node --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts && node --experimental-strip-types scripts/layering/check.ts", "depgraph": "node --experimental-strip-types scripts/depgraph/build.ts", "depgraph:test": "node --experimental-strip-types --test scripts/depgraph/model.test.ts", "check:production-exports": "fallow dead-code --config fallow-production-exports.json --production --unused-exports --fail-on-issues", diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index 1d6733d9f9..07a5ac386b 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -45,16 +45,9 @@ import { resolveImportEdges, topFolder, typeInversionPair, - type ImportEdge, type ResolvedImportEdge, } from './model.ts'; - -type EdgeContext = { - file: string; - fromTop: string; - toTop: string; - imp: ImportEdge; -}; +import { policyLead, policyViolation, ZONE_POLICIES } from './zone-policy.ts'; type Violation = { rule: string; @@ -96,71 +89,24 @@ function isProductionSourceFile(file: string): boolean { return file.endsWith('.ts') && !/(?:^|\/)__tests__\//.test(file) && !/\.test\.ts$/.test(file); } -function isCoreInteractor(file: string): boolean { - return file.startsWith('src/core/interactors/'); -} - -function isDaemonServer(file: string): boolean { - return file.startsWith('src/daemon/') && !file.startsWith('src/daemon/client/'); -} - -function isSdkBarrel(file: string): boolean { - return file.startsWith('src/sdk/'); -} - -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/.`, - }; -} - -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.`, - }; -} - -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) || isSdkBarrel(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]; - +// R1-R3 are declared as a policy table in zone-policy.ts. This walks it; the boundaries +// themselves are data, so adding one is a table entry rather than a fourth predicate. function checkLayeringRules(edges: readonly ResolvedImportEdge[]): Violation[] { const violations: Violation[] = []; for (const edge of edges) { - const fromTop = topFolder(edge.file); - const toTop = topFolder(edge.target); - if (fromTop === toTop) continue; - const ctx: EdgeContext = { file: edge.file, fromTop, toTop, imp: edge }; - for (const rule of RULES) { - const violation = rule(ctx); - if (violation) violations.push(violation); + const fromZone = topFolder(edge.file); + const toZone = topFolder(edge.target); + if (fromZone === toZone) continue; + const ctx = { file: edge.file, fromZone, toZone, imp: edge }; + for (const policy of ZONE_POLICIES) { + const hint = policyViolation(policy, ctx); + if (hint === null) continue; + violations.push({ + rule: policy.rule, + file: edge.file, + line: edge.line, + message: `${policyLead(ctx)} ${hint}`, + }); } } return violations; diff --git a/scripts/layering/zone-policy.test.ts b/scripts/layering/zone-policy.test.ts new file mode 100644 index 0000000000..3f4854bda0 --- /dev/null +++ b/scripts/layering/zone-policy.test.ts @@ -0,0 +1,135 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { policyLead, policyViolation, ZONE_POLICIES, type ZonePolicy } from './zone-policy.ts'; + +// R1-R3 were three predicate functions with no unit test for eight months: the only thing +// exercising them was the real tree, which is clean, so a rule that had silently stopped matching +// would have looked exactly like a rule that was being obeyed. These tests assert each boundary +// fires AND each documented exemption holds, so the table cannot quietly become decoration. + +type Kind = { typeOnly?: boolean; dynamic?: boolean }; + +function edge(file: string, fromZone: string, toZone: string, kind: Kind = {}) { + return { + file, + fromZone, + toZone, + imp: { + spec: `../${toZone}/thing.ts`, + line: 1, + dynamic: kind.dynamic ?? false, + typeOnly: kind.typeOnly ?? false, + }, + }; +} + +/** Every policy that fires for an edge, by rule id. */ +function firing(e: ReturnType): string[] { + return ZONE_POLICIES.filter((policy) => policyViolation(policy, e) !== null).map((p) => p.rule); +} + +function policiesFor(rule: string): ZonePolicy[] { + return ZONE_POLICIES.filter((policy) => policy.rule === rule); +} + +test('R1 kernel-sink closes every kernel out-edge except a type-only contracts re-export', () => { + // The one open door. + assert.deepEqual( + firing(edge('src/kernel/errors.ts', 'kernel', 'contracts', { typeOnly: true })), + [], + ); + + // Same target, value import: closed. A runtime edge out of the sink is the thing R1 exists for. + assert.deepEqual(firing(edge('src/kernel/errors.ts', 'kernel', 'contracts')), ['R1 kernel-sink']); + + // Any other target is closed to BOTH kinds — unlike R3, R1 does not tolerate type-only, because + // "kernel is declared in terms of utils" is still a claim that kernel is not the sink. + for (const kind of [{}, { typeOnly: true }, { dynamic: true }]) { + assert.deepEqual( + firing(edge('src/kernel/errors.ts', 'kernel', 'utils', kind)), + ['R1 kernel-sink'], + `kernel -> utils ${JSON.stringify(kind)} must violate R1`, + ); + } +}); + +test('R2 commands-floor closes the four zones below the command surface, whatever the kind', () => { + for (const zone of ['kernel', 'platforms', 'core', 'daemon']) { + for (const kind of [{}, { typeOnly: true }, { dynamic: true }]) { + assert.ok( + firing(edge(`src/${zone}/thing.ts`, zone, 'commands', kind)).includes('R2 commands-floor'), + `${zone} -> commands ${JSON.stringify(kind)} must violate R2`, + ); + } + } + + // Zones at or above the command surface are not governed by R2. + assert.deepEqual(firing(edge('src/cli/run.ts', 'cli', 'commands')), []); + assert.deepEqual(firing(edge('src/mcp/tools.ts', 'mcp', 'commands')), []); +}); + +test('R3 platforms-seam closes static value imports and opens the three declared seam owners', () => { + // Closed from an ordinary zone. + assert.deepEqual(firing(edge('src/cli/run.ts', 'cli', 'platforms')), ['R3 platforms-seam']); + + // Open to the seam owners. + for (const file of [ + 'src/core/interactors/android.ts', + 'src/daemon/handlers/perf.ts', + 'src/sdk/android-adb.ts', + ]) { + assert.deepEqual(firing(edge(file, 'core', 'platforms')), [], `${file} is a seam owner`); + } + + // daemon/client/ is inside the daemon prefix but is NOT part of the seam: it is the client half + // of the process boundary (ADR 0008), so it must not reach platform code at all. + assert.deepEqual(firing(edge('src/daemon/client/daemon-client.ts', 'daemon', 'platforms')), [ + 'R3 platforms-seam', + ]); + + // Both tolerated kinds are the documented escape hatch that preserves CLI cold-start. + assert.deepEqual(firing(edge('src/cli/run.ts', 'cli', 'platforms', { dynamic: true })), []); + assert.deepEqual(firing(edge('src/cli/run.ts', 'cli', 'platforms', { typeOnly: true })), []); +}); + +test('every policy carries a hint, and the lead names the kind it rejected', () => { + // ADR 0010: an error that does not say what to do instead is a rule nobody can act on. + for (const policy of ZONE_POLICIES) { + assert.ok(policy.hint.length > 20, `${policy.rule} needs an actionable hint`); + } + + assert.match(policyLead(edge('a', 'kernel', 'utils')), /kernel\/ must not import utils\//); + assert.match( + policyLead(edge('a', 'kernel', 'commands', { typeOnly: true })), + /must not type-only import commands\//, + ); + assert.match( + policyLead(edge('a', 'cli', 'platforms', { dynamic: true })), + /must not dynamic import platforms\//, + ); +}); + +test('a policy states its scope with exactly one of `to` or `exceptTo`', () => { + // Both would make the target set ambiguous; neither would silently govern every target, which is + // never what a boundary means. + for (const policy of ZONE_POLICIES) { + assert.notEqual( + Boolean(policy.to) === Boolean(policy.exceptTo), + true, + `${policy.rule} must set exactly one of to/exceptTo`, + ); + } +}); + +test('R1 is two policies, so the single open door stays visible', () => { + // Collapsing these into one entry with a special case is what made the original predicate hard + // to read. If someone merges them, this fails and says why. + const r1 = policiesFor('R1 kernel-sink'); + assert.equal(r1.length, 2); + assert.deepEqual( + r1.find((p) => p.to)?.tolerates, + ['type-only'], + 'the contracts door is type-only', + ); + assert.equal(r1.find((p) => p.exceptTo)?.tolerates, undefined, 'every other target is closed'); +}); diff --git a/scripts/layering/zone-policy.ts b/scripts/layering/zone-policy.ts new file mode 100644 index 0000000000..f13e43cebe --- /dev/null +++ b/scripts/layering/zone-policy.ts @@ -0,0 +1,122 @@ +import type { ImportEdge } from './model.ts'; + +/** + * R1-R3 as data: which zone may import which, and on what terms. + * + * These three rules used to be three hand-written predicate functions. They were short, but each + * one buried its boundary in control flow — you had to read the early-returns to learn that R3 + * tolerates dynamic imports, or that R1 opens exactly one door. Stating them as a table means the + * policy is readable without reading code, and a fourth boundary is a table entry rather than a + * fourth function to keep consistent with the other three. + * + * The evaluator is deliberately small. Everything a policy can say is in `ZonePolicy`, so a rule + * that needs more than these fields does NOT belong here: R4 (cycles), R5/R6 (spine ranking), + * R7 (field ownership), R8 (CI job closure) and R9 (cycle size) are whole-graph or non-import + * properties, and each keeps its own checker. + */ + +/** Edge kinds a boundary can tolerate. Both are erased or deferred, so both can be legitimate. */ +export type ToleratedKind = 'type-only' | 'dynamic'; + +export type ZonePolicy = { + /** Rule id reported on violation, e.g. `R1 kernel-sink`. */ + rule: string; + /** Source zones this policy governs. Omit for "every zone". */ + from?: readonly string[]; + /** Target zones this policy forbids importing. Omit and use `exceptTo` for "everything but". */ + to?: readonly string[]; + /** Target zones this policy does NOT govern — the complement form of `to`. */ + exceptTo?: readonly string[]; + /** + * Edge kinds that do not violate this boundary. A type-only edge costs nothing at runtime; a + * dynamic edge defers the cost past startup. Omitted means the boundary is closed to every kind. + */ + tolerates?: readonly ToleratedKind[]; + /** Path prefixes allowed to cross regardless: the declared seam. */ + seam?: readonly string[]; + /** Subtrees of `seam` that are NOT part of it (a seam owner with a non-owning child). */ + seamExcept?: readonly string[]; + /** Why the boundary exists and what to do instead. ADR 0010: every error carries a hint. */ + hint: string; +}; + +/** + * The policy table. Order is presentation only — every policy is evaluated against every edge. + * + * R1 is two entries rather than one with a special case, because "kernel may import contracts + * type-only" and "kernel may import nothing else at all" are genuinely two statements. Writing + * them separately is what makes the single open door visible. + */ +export const ZONE_POLICIES: readonly ZonePolicy[] = [ + { + rule: 'R1 kernel-sink', + from: ['kernel'], + to: ['contracts'], + tolerates: ['type-only'], + hint: 'The only allowed kernel out-edge is a type-only re-export from contracts/.', + }, + { + rule: 'R1 kernel-sink', + from: ['kernel'], + exceptTo: ['contracts'], + hint: + 'kernel is the sink of the import DAG: everything may depend on it and it may depend on ' + + 'nothing. The only allowed kernel out-edge is a type-only re-export from contracts/.', + }, + { + rule: 'R2 commands-floor', + from: ['kernel', 'platforms', 'core', 'daemon'], + to: ['commands'], + hint: + 'commands/ is the command surface, above these zones. Depend on shared kernel/contracts ' + + 'instead; if two zones need the same rule, put the rule below both of them.', + }, + { + rule: 'R3 platforms-seam', + to: ['platforms'], + tolerates: ['type-only', 'dynamic'], + seam: ['src/core/interactors/', 'src/daemon/', 'src/sdk/'], + seamExcept: ['src/daemon/client/'], + hint: + 'Only src/core/interactors/, the daemon server and the sdk barrel may statically import ' + + 'platforms/; elsewhere use a dynamic import() or a type-only import to preserve CLI ' + + 'cold-start.', + }, +]; + +function kindOf(imp: ImportEdge): ToleratedKind | 'value' { + if (imp.dynamic) return 'dynamic'; + if (imp.typeOnly) return 'type-only'; + return 'value'; +} + +function crossesSeam(policy: ZonePolicy, file: string): boolean { + if (!policy.seam?.some((prefix) => file.startsWith(prefix))) return false; + return !policy.seamExcept?.some((prefix) => file.startsWith(prefix)); +} + +/** + * Whether `policy` governs this edge AND the edge violates it. Returns the hint on violation so + * the caller can build the message, or `null` when the policy is silent about this edge. + */ +export function policyViolation( + policy: ZonePolicy, + edge: { file: string; fromZone: string; toZone: string; imp: ImportEdge }, +): string | null { + if (policy.from && !policy.from.includes(edge.fromZone)) return null; + if (policy.to && !policy.to.includes(edge.toZone)) return null; + if (policy.exceptTo?.includes(edge.toZone)) return null; + + const kind = kindOf(edge.imp); + if (kind !== 'value' && policy.tolerates?.includes(kind)) return null; + if (crossesSeam(policy, edge.file)) return null; + + return policy.hint; +} + +/** The one-line lead every zone-policy violation shares, before its rule-specific hint. */ +export function policyLead(edge: { fromZone: string; toZone: string; imp: ImportEdge }): string { + const kind = kindOf(edge.imp); + const qualifier = kind === 'value' ? '' : `${kind} `; + return `${edge.fromZone}/ must not ${qualifier}import ${edge.toZone}/ (imports '${edge.imp.spec}').`; +}