From 2db6086d2a10df933fae83a790963e3d044eb687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 24 Aug 2026 10:39:40 +0200 Subject: [PATCH] refactor(lint): retire the facade closure test for a lint rule plus budget rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `contracts-entry-closure.test.ts` (from #1959) carried two assertions. Neither replacement subsumes it alone; together they do, on two different axes. Its whole-tree scan parsed ~490 candidate files to prove nothing value-imports the two wide facades. `eslint/no-restricted-imports` states exactly that, and `allowTypeImports` draws the one distinction that made a custom walker look necessary: `import type` is erased, so it stays legal. The rule covers every file rather than the four hubs the deleted test named, and it reaches dynamic imports too. Its hub pin checked that four named hubs never reach either facade. The lint rule forbids the import edge that was the only way that could happen, and #1960's rows add an axis the old test never had: closure SIZE drift. That cardinality check is deliberately not described here as strictly stronger, because it is not — an equal-size graph substitution leaves the count intact and passes. Size drift and forbidden edges are different properties, and the two gates own one each. The override semantics are not additive, so the rule was verified per zone rather than assumed. A same-rule override REPLACES the parent, so a top-level rule would have been silently dropped for `src/**`; and the existing blanket `"off"` for `exec.ts` and the test tree would have exempted the files that carried most of the cost #1959 removed. The paths are therefore added per zone, and that `"off"` becomes a facade-only config that keeps the `node:child_process` exemption it existed for. Planted red in all three zones — `src/core/capabilities.ts`, a `src/__tests__` file, and `packages/capture-kit/src` — each flagged, while a type-only import in the same probe file was not. The first packages probe read as a pass because the sed that built it produced a type-only import; that zone was re-probed with a real value import rather than trusting the green. Relying on config is safe here because misconfiguration fails loudly: a typo'd rule name makes oxlint exit 1 with "Rule not found in plugin", rather than pass silently the way the `rg` assertions in #1976 did. The budgets stay for what no linter can express: a transitive weight property, where a module already imported grows an import and the cost arrives without any single file's import list changing. Per-file rules cannot see that, and `no-restricted-imports` can only ban specifiers named in advance — exactly what #1950/#1956/#1959 could not have named. --- .oxlintrc.json | 62 +++++++++++- src/__tests__/contracts-entry-closure.test.ts | 98 ------------------- 2 files changed, 59 insertions(+), 101 deletions(-) delete mode 100644 src/__tests__/contracts-entry-closure.test.ts diff --git a/.oxlintrc.json b/.oxlintrc.json index e0330e37d0..33a1bf1b2d 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -15,13 +15,21 @@ "argsIgnorePattern": "^_", "caughtErrorsIgnorePattern": "^_", "destructuredArrayIgnorePattern": "^_", - "fix": { "imports": "safe-fix", "variables": "suggestion" }, + "fix": { + "imports": "safe-fix", + "variables": "suggestion" + }, "varsIgnorePattern": "^_" } ], "eslint/prefer-const": "error", "eslint/no-useless-escape": "error", - "eslint/no-empty": ["error", { "allowEmptyCatch": true }], + "eslint/no-empty": [ + "error", + { + "allowEmptyCatch": true + } + ], "typescript/no-explicit-any": "off", "typescript/only-throw-error": "off", "typescript/no-var-requires": "error", @@ -38,6 +46,16 @@ { "name": "node:child_process", "message": "Use process helpers from src/utils/exec.ts instead of importing node:child_process directly." + }, + { + "name": "@agent-device/contracts/platform", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 32 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + }, + { + "name": "@agent-device/contracts/interaction", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 18 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." } ] } @@ -47,7 +65,45 @@ { "files": ["src/utils/exec.ts", "src/**/*.test.ts", "src/**/__tests__/**/*.ts"], "rules": { - "eslint/no-restricted-imports": "off" + "eslint/no-restricted-imports": [ + "error", + { + "paths": [ + { + "name": "@agent-device/contracts/platform", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 32 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + }, + { + "name": "@agent-device/contracts/interaction", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 18 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + } + ] + } + ] + } + }, + { + "files": ["packages/*/src/**/*.ts"], + "rules": { + "eslint/no-restricted-imports": [ + "error", + { + "paths": [ + { + "name": "@agent-device/contracts/platform", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 32 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + }, + { + "name": "@agent-device/contracts/interaction", + "allowTypeImports": true, + "message": "Value-importing this facade evaluates all 18 vocabulary modules it re-exports. Import the symbol from the module that owns it (@agent-device/contracts/). `import type` stays fine \u2014 it is erased." + } + ] + } + ] } }, { diff --git a/src/__tests__/contracts-entry-closure.test.ts b/src/__tests__/contracts-entry-closure.test.ts deleted file mode 100644 index 4e4fce3868..0000000000 --- a/src/__tests__/contracts-entry-closure.test.ts +++ /dev/null @@ -1,98 +0,0 @@ -import { expect, test } from 'vitest'; -import fs from 'node:fs'; -import path from 'node:path'; -import { eagerClosureOf, eagerlyEvaluatedModules } from './eager-import-closure.fixtures.ts'; - -/** - * `@agent-device/contracts/platform` and `/interaction` union 32 and 18 vocabulary - * modules. A file that value-imports either one evaluates the whole union to reach a - * single function, and because permanent hubs sat behind them (#1959) that union rode - * into ~470 of the suite's ~970 test graphs. - * - * Every one of those modules now has its own entry subpath, so the narrow import is - * always available. These two tests keep it that way from both directions: the first - * pins the four hubs the issue named, and the second closes the general case so the - * clump cannot re-form behind a hub nobody thought to list. - * - * Type-only importers are untouched and stay legal — `import type` is erased, so it - * evaluates nothing. That is the same distinction the walker itself draws, which is - * why this reads the AST through it rather than matching specifier text. - */ - -const repoRoot = path.resolve(import.meta.dirname, '../..'); -const CLUMP_ENTRIES = [ - '@agent-device/contracts/platform', - '@agent-device/contracts/interaction', -] as const; -const CLUMP_FACADES = [ - 'packages/contracts/src/facades/platform.ts', - 'packages/contracts/src/facades/interaction.ts', -].map((file) => path.resolve(repoRoot, file)); - -const HUBS = [ - 'src/core/command-descriptor/registry.ts', - 'src/core/capabilities.ts', - 'src/core/interactors/register-builtins.ts', - 'src/core/command-descriptor/platform-execution-entry.ts', -]; - -function sourceFiles(): string[] { - const found: string[] = []; - const walk = (dir: string): void => { - for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { - const full = path.join(dir, entry.name); - if (entry.isDirectory()) { - if (entry.name !== 'node_modules') walk(full); - } else if (entry.name.endsWith('.ts')) found.push(full); - } - }; - walk(path.join(repoRoot, 'src')); - for (const pkg of fs.readdirSync(path.join(repoRoot, 'packages'))) { - const src = path.join(repoRoot, 'packages', pkg, 'src'); - if (fs.existsSync(src)) walk(src); - } - return found; -} - -test('the hubs behind the contracts clump never evaluate either wide facade', () => { - const offenders = HUBS.flatMap((hub) => { - const carried = eagerClosureOf(path.resolve(repoRoot, hub)).filter((file) => - CLUMP_FACADES.includes(file), - ); - return carried.map((file) => `${hub} -> ${path.relative(repoRoot, file)}`); - }); - - expect( - offenders, - 'These hubs sit in hundreds of test graphs, so whatever they evaluate, the suite pays for ' + - 'everywhere. Import the symbol from the vocabulary module that owns it ' + - '(@agent-device/contracts/) instead of the wide facade.', - ).toEqual([]); -}); - -test('no source file value-imports the wide contracts facades', () => { - const offenders: string[] = []; - let typeOnlyImporters = 0; - for (const file of sourceFiles()) { - const source = fs.readFileSync(file, 'utf8'); - // Text-filter before parsing: a file that never names the specifier cannot import - // it, and parsing all ~3000 sources costs more than the unit lane's budget allows. - const mentions = CLUMP_ENTRIES.filter((entry) => source.includes(`${entry}'`)); - if (mentions.length === 0) continue; - const evaluated = new Set(eagerlyEvaluatedModules(file, source)); - for (const entry of mentions) { - if (evaluated.has(entry)) offenders.push(`${path.relative(repoRoot, file)} -> ${entry}`); - else typeOnlyImporters += 1; - } - } - - // Non-vacuity: an empty offender list also describes a scan that parsed nothing, so - // require that the surviving type-only importers were seen and classified as erased. - expect(typeOnlyImporters).toBeGreaterThan(300); - expect( - offenders.sort(), - 'Value-importing these facades evaluates every module they re-export from. Each of those ' + - 'modules has its own entry subpath in packages/contracts/package.json — import from that. ' + - '`import type` from the facades stays fine: it is erased, so it evaluates nothing.', - ).toEqual([]); -});