From 8ea310b05a87befb4b9aa36cdc2323e491b0775c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 15:59:12 +0200 Subject: [PATCH 01/13] refactor(plugins): supply provider policy to the plugin framework from the root The plugin loader no longer imports or lists provider-webdriver. The reserved provider ids and the { webDriver } factory-result adaptation move to src/provider-plugin-policy.ts, which every plugin load, connection and installation now receives explicitly. --- packages/plugins/package.json | 7 +- packages/plugins/src/load.test.ts | 120 ++++++------- packages/plugins/src/load.ts | 79 ++++----- packages/plugins/src/manifest.ts | 13 -- packages/plugins/src/store.test.ts | 33 ++-- packages/plugins/src/store.ts | 10 +- packages/plugins/tsconfig.json | 1 - pnpm-lock.yaml | 3 - src/__tests__/cloud-connect-testmu.test.ts | 1 + .../provider-device-runtimes.test.ts | 22 ++- src/cli/commands/plugins.ts | 8 +- .../connection/connect-provider-adapters.ts | 5 +- src/cli/connection/provider-policy.ts | 2 +- src/provider-credential-fingerprint.ts | 2 +- src/provider-device-runtimes.ts | 6 +- src/provider-plugin-policy.test.ts | 164 ++++++++++++++++++ src/provider-plugin-policy.ts | 45 +++++ 17 files changed, 361 insertions(+), 160 deletions(-) create mode 100644 src/provider-plugin-policy.test.ts create mode 100644 src/provider-plugin-policy.ts diff --git a/packages/plugins/package.json b/packages/plugins/package.json index 941f5faeff..161c940f36 100644 --- a/packages/plugins/package.json +++ b/packages/plugins/package.json @@ -7,8 +7,7 @@ "dependencies": { "@agent-device/contracts": "workspace:*", "@agent-device/host-kit": "workspace:*", - "@agent-device/kernel": "workspace:*", - "@agent-device/provider-webdriver": "workspace:*" + "@agent-device/kernel": "workspace:*" }, "exports": { "./connection": { @@ -23,10 +22,6 @@ "types": "./src/load.ts", "default": "./src/load.ts" }, - "./manifest": { - "types": "./src/manifest.ts", - "default": "./src/manifest.ts" - }, "./store": { "types": "./src/store.ts", "default": "./src/store.ts" diff --git a/packages/plugins/src/load.test.ts b/packages/plugins/src/load.test.ts index 5d7720e318..01b80c5bb3 100644 --- a/packages/plugins/src/load.test.ts +++ b/packages/plugins/src/load.test.ts @@ -2,35 +2,26 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; import { test } from 'vitest'; -import { loadProviderPlugins, withPluginConnection } from './load.ts'; import { - pluginHome, - registrationSource, - selectPlugin, - webDriverPluginSource, -} from '@agent-device/test-kit'; + loadProviderPlugins, + withPluginConnection, + type PluginLoadPolicy, + type ProviderPluginRegistration, +} from './load.ts'; +import { pluginHome, registrationSource, selectPlugin } from '@agent-device/test-kit'; import { AppError } from '@agent-device/kernel/errors'; import type { ProviderPluginHost } from './host.ts'; -import type { DeviceLease, ProviderDeviceRuntime } from '@agent-device/contracts/device'; +import type { ProviderDeviceRuntime } from '@agent-device/contracts/device'; -const realFetch = globalThis.fetch; -const lease: DeviceLease = { - leaseId: 'lease-1', - tenantId: 'team-a', - runId: 'run-a', - clientId: 'client-a', - leaseProvider: 'example', - backend: 'android-instance', - deviceKey: 'example:device-a', - createdAt: 1, - expiresAt: 2, - heartbeatAt: 1, -}; +const policy = (reservedProviders: readonly string[] = []): PluginLoadPolicy => ({ + reservedProviders, + adaptFactoryResult: () => undefined, +}); test('startup loads the factory with options and the host error constructor', async () => { const { home, env } = pluginHome(); selectPlugin(home, 'example', 'example', registrationSource('example')); - const [registration] = await loadProviderPlugins(env, ['limrun']); + const [registration] = await loadProviderPlugins(env, policy(['limrun'])); const runtime = registration!.runtime as ProviderDeviceRuntime & ProviderPluginHost; assert.deepEqual(runtime.options, { region: 'eu' }); assert.ok(runtime.createError('INVALID_ARGS', 'bad profile') instanceof AppError); @@ -45,10 +36,10 @@ test('duplicate providers and incompatible ABI refuse before executing any plugi 'limrun', `import fs from 'node:fs'; fs.writeFileSync(${JSON.stringify(marker)}, 'yes');`, ); - await assert.rejects(loadProviderPlugins(env, ['limrun']), { code: 'INVALID_ARGS' }); + await assert.rejects(loadProviderPlugins(env, policy(['limrun'])), { code: 'INVALID_ARGS' }); assert.ok(!fs.existsSync(marker)); selectPlugin(home, 'two', 'other', 'throw new Error("evaluated");', 2); - await assert.rejects(loadProviderPlugins(env, []), { code: 'INVALID_ARGS' }); + await assert.rejects(loadProviderPlugins(env, policy()), { code: 'INVALID_ARGS' }); assert.ok(!fs.existsSync(marker)); }); @@ -63,7 +54,7 @@ test('startup failure cleans up constructed runtimes, including malformed owners `instance: ${instance}`, ); selectPlugin(home, 'two', 'two', source); - await assert.rejects(loadProviderPlugins(env, []), { code: 'INVALID_ARGS' }); + await assert.rejects(loadProviderPlugins(env, policy()), { code: 'INVALID_ARGS' }); assert.ok(fs.existsSync(first)); assert.ok(fs.existsSync(invalid)); } @@ -78,7 +69,7 @@ test('cleanup throwing synchronously preserves the factory failure', async () => ); selectPlugin(home, 'one', 'one', source); selectPlugin(home, 'two', 'two', 'export default () => { throw new Error("factory failed"); };'); - await assert.rejects(loadProviderPlugins(env, []), /factory failed/); + await assert.rejects(loadProviderPlugins(env, policy()), /factory failed/); assert.ok(fs.existsSync(marker)); }); @@ -94,6 +85,7 @@ test('connection callbacks run from the installed plugin and always release runt const profile = await withPluginConnection( 'example', env, + policy(), async (connection) => await connection.resolve({ flags: { json: false, help: false, version: false }, @@ -109,6 +101,7 @@ test('connection callbacks run from the installed plugin and always release runt withPluginConnection( 'example', env, + policy(), async (connection) => await connection.verify({ flags: { json: false, help: false, version: false }, env }), ), @@ -117,47 +110,56 @@ test('connection callbacks run from the installed plugin and always release runt assert.ok(fs.existsSync(marker)); }); -test('WebDriver plugins allocate through the shared engine and refuse mismatched providers', async () => { +test('the supplied policy adapts a foreign factory result and its failure releases earlier runtimes', async () => { const { home, env } = pluginHome(); - const source = webDriverPluginSource('example', ['awsProjectArn']); - selectPlugin(home, 'example', 'example', source); - const [registration] = await loadProviderPlugins(env, []); - const runtime = registration!.runtime; - assert.equal(runtime.provider, 'example'); - assert.equal(registration!.platformModule.owner.provider, 'example'); - const requests: string[] = []; - globalThis.fetch = async (input, init) => { - requests.push(`${init?.method ?? 'GET'} ${String(input)}`); - return new Response(JSON.stringify({ value: { sessionId: 'SESSION1', capabilities: {} } })); - }; - try { - await assert.rejects( - runtime.leaseLifecycle.allocate!(lease, { flags: { awsProjectArn: 'arn:project' } }), - (error: unknown) => { - assert.ok(error instanceof AppError); - assert.equal(error.code, 'INVALID_ARGS'); - assert.equal(error.details?.provider, 'example'); - assert.deepEqual(error.details?.flags, ['--aws-project-arn']); - return true; + const first = path.join(home, 'first-shutdown'); + selectPlugin(home, 'one', 'one', registrationSource('one', first)); + selectPlugin(home, 'adapted', 'adapted', 'export default () => ({ foreign: true });'); + const contexts: unknown[] = []; + const adapted = (provider: string): ProviderPluginRegistration => + ({ + runtime: { + provider, + ownsDevice: () => false, + getInteractor: () => undefined, + deviceInventoryProvider: async () => [], + leaseLifecycle: {}, + shutdown: async () => {}, }, - ); - assert.deepEqual(requests, []); - const allocated = await runtime.leaseLifecycle.allocate!(lease, { flags: {} }); - assert.equal(allocated?.sessionId, 'SESSION1'); - assert.deepEqual(requests, ['POST https://webdriver.test/wd/hub/session']); - } finally { - await runtime.shutdown(); - globalThis.fetch = realFetch; - } - const other = pluginHome(); - selectPlugin(other.home, 'example', 'wrong', source); - await assert.rejects(loadProviderPlugins(other.env, []), { code: 'INVALID_ARGS' }); + platformModule: { + owner: { kind: 'provider-runtime', provider, instance: 'test' }, + loadRuntime: async () => { + throw new Error('lazy'); + }, + }, + }) as unknown as ProviderPluginRegistration; + const adaptForeign = (provider: string): PluginLoadPolicy => ({ + reservedProviders: [], + adaptFactoryResult: (result, { plugin, host }) => { + if (typeof result !== 'object' || result === null || !('foreign' in result)) return undefined; + contexts.push({ plugin, options: host.options }); + return adapted(provider); + }, + }); + const registrations = await loadProviderPlugins(env, adaptForeign('adapted')); + assert.deepEqual( + registrations.map(({ runtime }) => runtime.provider), + ['one', 'adapted'], + ); + assert.deepEqual(contexts, [ + { plugin: { name: 'adapted', provider: 'adapted' }, options: { region: 'eu' } }, + ]); + assert.ok(!fs.existsSync(first)); + await assert.rejects(loadProviderPlugins(env, adaptForeign('other')), { + code: 'INVALID_ARGS', + }); + assert.ok(fs.existsSync(first)); }); test('a factory returning a primitive is refused as an invalid plugin', async () => { for (const value of ['42', '"runtime"', 'true']) { const { home, env } = pluginHome(); selectPlugin(home, 'example', 'example', `export default () => ${value};`); - await assert.rejects(loadProviderPlugins(env, []), { code: 'INVALID_ARGS' }); + await assert.rejects(loadProviderPlugins(env, policy()), { code: 'INVALID_ARGS' }); } }); diff --git a/packages/plugins/src/load.ts b/packages/plugins/src/load.ts index c590506430..1ff9ec5053 100644 --- a/packages/plugins/src/load.ts +++ b/packages/plugins/src/load.ts @@ -3,34 +3,47 @@ import { AppError } from '@agent-device/kernel/errors'; import type { ProviderDeviceRuntime } from '@agent-device/contracts/device'; import type { PlatformRuntimeProviderModule } from '@agent-device/contracts/platform-runtime-operations'; import { installedPlugins } from './store.ts'; -import { - resolvePluginEntry, - assertUniquePluginProviders, - RESERVED_PLUGIN_PROVIDERS, -} from './manifest.ts'; +import { resolvePluginEntry, assertUniquePluginProviders } from './manifest.ts'; import { createPluginHost, type ProviderPluginHost } from './host.ts'; import type { PluginConnection } from './connection.ts'; -import type { CloudWebDriverProviderOptions } from '@agent-device/provider-webdriver/plugin'; -type ProviderPluginRegistration = Readonly<{ +export type ProviderPluginRegistration = Readonly<{ runtime: ProviderDeviceRuntime; platformModule: PlatformRuntimeProviderModule; connection?: PluginConnection; }>; +export type PluginFactoryContext = Readonly<{ + plugin: Readonly<{ name: string; provider: string }>; + host: ProviderPluginHost; +}>; + +/** + * What the composing root decides for every load: provider ids a plugin may not claim, and how to + * turn a factory result that is not itself a registration into one. `adaptFactoryResult` returns + * undefined for a result it does not own, which the loader then validates as a registration. + */ +export type PluginLoadPolicy = Readonly<{ + reservedProviders: readonly string[]; + adaptFactoryResult( + result: unknown, + context: PluginFactoryContext, + ): ProviderPluginRegistration | undefined | Promise; +}>; + export async function loadProviderPlugins( env: NodeJS.ProcessEnv, - reservedProviders: readonly string[], + policy: PluginLoadPolicy, onlyProvider?: string, ): Promise { const plugins = installedPlugins(env); - assertUniquePluginProviders(plugins, reservedProviders); + assertUniquePluginProviders(plugins, policy.reservedProviders); const registrations: ProviderPluginRegistration[] = []; try { for (const plugin of plugins.filter( (plugin) => onlyProvider === undefined || plugin.agentDevicePlugin.provider === onlyProvider, )) { - const registration = await instantiateProviderPlugin(plugin, env); + const registration = await instantiateProviderPlugin(plugin, env, policy); registrations.push(registration); validateProviderPlugin(registration, plugin); } @@ -44,9 +57,10 @@ export async function loadProviderPlugins( export async function withPluginConnection( provider: string, env: NodeJS.ProcessEnv, + policy: PluginLoadPolicy, runConnection: (connection: PluginConnection) => Promise, ): Promise { - const registrations = await loadProviderPlugins(env, RESERVED_PLUGIN_PROVIDERS, provider); + const registrations = await loadProviderPlugins(env, policy, provider); try { const connection = registrations.find( (entry) => entry.runtime.provider === provider, @@ -62,6 +76,7 @@ export async function withPluginConnection( async function instantiateProviderPlugin( plugin: ReturnType[number], env: NodeJS.ProcessEnv, + policy: PluginLoadPolicy, ): Promise { const module = await import( pathToFileURL(resolvePluginEntry(plugin.directory, plugin.agentDevicePlugin.entry)).href @@ -69,48 +84,18 @@ async function instantiateProviderPlugin( if (typeof module.default !== 'function') throw new AppError('INVALID_ARGS', `Plugin must export a default factory: ${plugin.name}`); const host = createPluginHost(env, plugin.selection.options); - const result = await ( - module.default as ( - host: ProviderPluginHost, - ) => - | ProviderPluginRegistration - | { webDriver: CloudWebDriverProviderOptions; connection?: PluginConnection } - | Promise< - | ProviderPluginRegistration - | { webDriver: CloudWebDriverProviderOptions; connection?: PluginConnection } - > - )(host); - let registration: ProviderPluginRegistration; - if (isWebDriverPluginResult(result)) { - if (result.webDriver?.provider !== plugin.agentDevicePlugin.provider) { - throw new AppError( - 'INVALID_ARGS', - `WebDriver plugin provider does not match its declaration: ${plugin.name}`, - ); - } - const { createCloudWebDriverRuntime } = await import('@agent-device/provider-webdriver/plugin'); - const runtime = await createCloudWebDriverRuntime({ - ...result.webDriver, - clientVersion: host.clientVersion, - }); - registration = { - runtime, - platformModule: runtime.platformRuntimeModule, - connection: result.connection, - }; - } else registration = result; + const result: unknown = await (module.default as (host: ProviderPluginHost) => unknown)(host); + const registration = + (await policy.adaptFactoryResult(result, { + plugin: { name: plugin.name, provider: plugin.agentDevicePlugin.provider }, + host, + })) ?? (result as ProviderPluginRegistration); if (!registration?.runtime || typeof registration.runtime.shutdown !== 'function') { throw new AppError('INVALID_ARGS', `Plugin must return a provider runtime: ${plugin.name}`); } return registration; } -function isWebDriverPluginResult( - result: unknown, -): result is { webDriver: CloudWebDriverProviderOptions; connection?: PluginConnection } { - return typeof result === 'object' && result !== null && 'webDriver' in result; -} - function validateProviderPlugin( registration: ProviderPluginRegistration, plugin: ReturnType[number], diff --git a/packages/plugins/src/manifest.ts b/packages/plugins/src/manifest.ts index 87fd0962ec..70e68be140 100644 --- a/packages/plugins/src/manifest.ts +++ b/packages/plugins/src/manifest.ts @@ -3,19 +3,6 @@ import path from 'node:path'; import { AppError } from '@agent-device/kernel/errors'; import type { ConnectionProviderCapabilities } from '@agent-device/contracts/remote'; -import { - CLOUD_WEBDRIVER_PROVIDERS, - type CloudWebDriverKnownProviderName, -} from '@agent-device/provider-webdriver/providers'; - -/** The one list of provider ids plugins cannot claim: connect routes and bundled runtimes. */ -export const RESERVED_PLUGIN_PROVIDERS: readonly ( - | 'cloud' - | 'proxy' - | 'limrun' - | CloudWebDriverKnownProviderName -)[] = ['cloud', 'proxy', ...Object.values(CLOUD_WEBDRIVER_PROVIDERS), 'limrun']; - const PROVIDER_PLUGIN_API_VERSION = 1; type PluginManifest = { name: string; diff --git a/packages/plugins/src/store.test.ts b/packages/plugins/src/store.test.ts index 247fb2ae53..ce686a8bf2 100644 --- a/packages/plugins/src/store.test.ts +++ b/packages/plugins/src/store.test.ts @@ -31,13 +31,13 @@ test('add, pinned update and remove preserve flags, options and old installation const { home, env } = pluginHome(); fs.writeFileSync(path.join(home, 'config.json'), JSON.stringify({ platform: 'android' })); installFixture(); - await changePlugin('add', `${packageName}@1.2.3`, env); + await changePlugin('add', `${packageName}@1.2.3`, env, []); const [first] = installedPlugins(env); const config = readPluginConfig(env); config.plugins![packageName]!.options = { region: 'eu' }; config.plugins![packageName]!.installation = '../damaged'; fs.writeFileSync(path.join(home, 'config.json'), JSON.stringify(config)); - await changePlugin('update', packageName, env); + await changePlugin('update', packageName, env, []); const [second] = installedPlugins(env); assert.notEqual(first!.directory, second!.directory); assert.deepEqual(second!.selection.options, { region: 'eu' }); @@ -47,7 +47,7 @@ test('add, pinned update and remove preserve flags, options and old installation { [packageName]: '1.2.3' }, ); assert.equal(readPluginConfig(env).platform, 'android'); - await changePlugin('remove', packageName, env); + await changePlugin('remove', packageName, env, []); assert.deepEqual(installedPlugins(env), []); assert.ok(fs.existsSync(first!.directory)); assert.ok(fs.existsSync(second!.directory)); @@ -56,13 +56,13 @@ test('add, pinned update and remove preserve flags, options and old installation test('failed npm install and incompatible replacement preserve the active selection', async () => { const { home, env } = pluginHome(); installFixture(); - await changePlugin('add', packageName, env); + await changePlugin('add', packageName, env, []); const before = fs.readFileSync(path.join(home, 'config.json'), 'utf8'); vi.mocked(runCmd).mockRejectedValue(new Error('npm failed')); - await assert.rejects(changePlugin('update', packageName, env), /npm failed/); + await assert.rejects(changePlugin('update', packageName, env, []), /npm failed/); assert.equal(fs.readFileSync(path.join(home, 'config.json'), 'utf8'), before); installFixture(packageName, 2); - await assert.rejects(changePlugin('update', packageName, env), { code: 'INVALID_ARGS' }); + await assert.rejects(changePlugin('update', packageName, env, []), { code: 'INVALID_ARGS' }); assert.equal(fs.readFileSync(path.join(home, 'config.json'), 'utf8'), before); assert.equal(fs.readdirSync(path.join(home, 'plugins')).length, 1); }); @@ -77,7 +77,10 @@ test('concurrent additions serialize without dropping the other package', async writePlugin(options!.cwd!, name, 1, name); return { stdout: '', stderr: '', exitCode: 0 }; }); - await Promise.all([changePlugin('add', 'constructor', env), changePlugin('add', 'two', env)]); + await Promise.all([ + changePlugin('add', 'constructor', env, []), + changePlugin('add', 'two', env, []), + ]); assert.deepEqual( installedPlugins(env) .map(({ name }) => name) @@ -95,7 +98,7 @@ test('invalid package sources and damaged selections fail before invoking npm', 'plugin@npm:other', 'https://example.com/plugin', ]) { - await assert.rejects(changePlugin('add', name, env), { code: 'INVALID_ARGS' }); + await assert.rejects(changePlugin('add', name, env, []), { code: 'INVALID_ARGS' }); } fs.writeFileSync( path.join(home, 'config.json'), @@ -103,13 +106,13 @@ test('invalid package sources and damaged selections fail before invoking npm', ); assert.throws(() => installedPlugins(env), { code: 'INVALID_ARGS' }); assert.equal(listPlugins(env)[0]?.compatible, false); - await changePlugin('remove', 'one', env); + await changePlugin('remove', 'one', env, []); assert.deepEqual(listPlugins(env), []); fs.writeFileSync( path.join(home, 'config.json'), JSON.stringify({ plugins: { UPPERCASE: null } }), ); - await changePlugin('remove', 'UPPERCASE', env); + await changePlugin('remove', 'UPPERCASE', env, []); assert.deepEqual(listPlugins(env), []); assert.equal(vi.mocked(runCmd).mock.calls.length, 0); }); @@ -121,9 +124,9 @@ test('provider collisions refuse activation and preserve existing selections', a code: 'INVALID_ARGS', }); assert.deepEqual(installedPlugins(env), []); - await changePlugin('add', packageName, env); + await changePlugin('add', packageName, env, []); installFixture('other'); - await assert.rejects(changePlugin('add', 'other', env), { code: 'INVALID_ARGS' }); + await assert.rejects(changePlugin('add', 'other', env, []), { code: 'INVALID_ARGS' }); assert.equal(installedPlugins(env).length, 1); }); @@ -131,7 +134,7 @@ test('broken siblings do not block pinned updates or unrelated additions', async for (const damage of ['missing', 'json', 'abi']) { const { home, env } = pluginHome(); installFixture(); - await changePlugin('add', `${packageName}@1.2.3`, env); + await changePlugin('add', `${packageName}@1.2.3`, env, []); selectPlugin(home, 'broken', 'broken', 'throw new Error("must not evaluate");', 2); const config = readPluginConfig(env); config.plugins![packageName]!.options = { region: 'eu' }; @@ -140,13 +143,13 @@ test('broken siblings do not block pinned updates or unrelated additions', async if (damage === 'missing') fs.rmSync(directory, { recursive: true }); if (damage === 'json') fs.writeFileSync(path.join(directory, 'node_modules/broken/package.json'), '{'); - await changePlugin('update', packageName, env); + await changePlugin('update', packageName, env, []); const repaired = readPluginConfig(env).plugins![packageName]!; assert.deepEqual(repaired.options, { region: 'eu' }); assert.equal(repaired.version, '1.2.3'); assert.deepEqual(readPluginConfig(env).plugins!.broken, config.plugins!.broken); installFixture('other', 1, 'other'); - await changePlugin('add', 'other', env); + await changePlugin('add', 'other', env, []); assert.throws(() => installedPlugins(env), { code: 'INVALID_ARGS' }); } }); diff --git a/packages/plugins/src/store.ts b/packages/plugins/src/store.ts index 0655c8f8fa..9f8e6850f4 100644 --- a/packages/plugins/src/store.ts +++ b/packages/plugins/src/store.ts @@ -10,11 +10,7 @@ import { resolveUserConfigPath, } from '@agent-device/host-kit/file'; import { readCurrentOwnerIdentity } from '@agent-device/host-kit/process'; -import { - readPluginManifest, - assertUniquePluginProviders, - RESERVED_PLUGIN_PROVIDERS, -} from './manifest.ts'; +import { readPluginManifest, assertUniquePluginProviders } from './manifest.ts'; type PluginSelection = { installation: string; @@ -142,8 +138,8 @@ async function stagePlugin( export async function changePlugin( action: PluginAction, input: string, - env: NodeJS.ProcessEnv = process.env, - reservedProviders: readonly string[] = RESERVED_PLUGIN_PROVIDERS, + env: NodeJS.ProcessEnv, + reservedProviders: readonly string[], ): Promise { const { name, version } = parsePluginRequest(action, input); const configPath = resolveUserConfigPath(env); diff --git a/packages/plugins/tsconfig.json b/packages/plugins/tsconfig.json index 4f983abbbc..380be0b4f5 100644 --- a/packages/plugins/tsconfig.json +++ b/packages/plugins/tsconfig.json @@ -14,7 +14,6 @@ { "path": "../contracts" }, { "path": "../host-kit" }, { "path": "../kernel" }, - { "path": "../provider-webdriver" }, { "path": "../test-kit" } ] } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8af7cc1611..7afdfceefb 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -595,9 +595,6 @@ importers: '@agent-device/kernel': specifier: workspace:* version: link:../kernel - '@agent-device/provider-webdriver': - specifier: workspace:* - version: link:../provider-webdriver devDependencies: '@agent-device/test-kit': specifier: workspace:* diff --git a/src/__tests__/cloud-connect-testmu.test.ts b/src/__tests__/cloud-connect-testmu.test.ts index bb73ac1906..7d48c31ab3 100644 --- a/src/__tests__/cloud-connect-testmu.test.ts +++ b/src/__tests__/cloud-connect-testmu.test.ts @@ -25,6 +25,7 @@ vi.mock('@agent-device/plugins/load', () => ({ withPluginConnection: async ( _provider: string, env: NodeJS.ProcessEnv, + _policy: unknown, runConnection: (connection: PluginConnection) => Promise, ) => { const registration = testMuPlugin(createPluginHost(env, undefined)); diff --git a/src/__tests__/provider-device-runtimes.test.ts b/src/__tests__/provider-device-runtimes.test.ts index c0a5f76bba..8051c94daa 100644 --- a/src/__tests__/provider-device-runtimes.test.ts +++ b/src/__tests__/provider-device-runtimes.test.ts @@ -1,12 +1,17 @@ import assert from 'node:assert/strict'; import { test } from 'vitest'; -import { RESERVED_PLUGIN_PROVIDERS } from '@agent-device/plugins/manifest'; +import { RESERVED_PLUGIN_PROVIDERS } from '../provider-plugin-policy.ts'; import { DEFAULT_PROVIDER_RUNTIME_REQUIRED_IDS, createDefaultProviderRuntimeComposition, createDaemonProviderRuntimeComposition, } from '../provider-device-runtimes.ts'; -import { pluginHome, registrationSource, selectPlugin } from '@agent-device/test-kit'; +import { + pluginHome, + registrationSource, + selectPlugin, + webDriverPluginSource, +} from '@agent-device/test-kit'; test('daemon composition registers installed provider runtimes and their lazy platform modules', async () => { const { home, env } = pluginHome(); @@ -21,6 +26,19 @@ test('daemon composition registers installed provider runtimes and their lazy pl await Promise.all(composition.runtimes.map((runtime) => runtime.shutdown())); }); +test('daemon composition registers a WebDriver plugin through the shared runtime', async () => { + const { home, env } = pluginHome(); + selectPlugin(home, 'example', 'example', webDriverPluginSource('example')); + const composition = await createDaemonProviderRuntimeComposition(env); + const plugin = composition.runtimes.find((runtime) => runtime.provider === 'example'); + assert.ok(plugin); + assert.equal( + composition.platformModules.find(({ runtime }) => runtime === plugin)?.module.owner.provider, + 'example', + ); + await Promise.all(composition.runtimes.map((runtime) => runtime.shutdown())); +}); + test('bundled composition stays independent of configured plugins and reserves all builtin IDs', async () => { const { home, env } = pluginHome(); selectPlugin(home, 'example', 'example', 'throw new Error("must not evaluate");'); diff --git a/src/cli/commands/plugins.ts b/src/cli/commands/plugins.ts index 97a8d89529..be152f6871 100644 --- a/src/cli/commands/plugins.ts +++ b/src/cli/commands/plugins.ts @@ -1,5 +1,6 @@ import { AppError } from '@agent-device/kernel/errors'; import { changePlugin, listPlugins } from '@agent-device/plugins/store'; +import { PROVIDER_PLUGIN_POLICY } from '../../provider-plugin-policy.ts'; import { writeCommandOutput } from './shared.ts'; import type { ClientCommandHandler } from './router-types.ts'; @@ -16,7 +17,12 @@ export const pluginsCommand: ClientCommandHandler = async ({ positionals, flags ); } if (action !== 'list' && name) { - await changePlugin(action as 'add' | 'update' | 'remove', name, process.env); + await changePlugin( + action as 'add' | 'update' | 'remove', + name, + process.env, + PROVIDER_PLUGIN_POLICY.reservedProviders, + ); } const plugins = listPlugins(process.env); await writeCommandOutput(flags, { plugins, restartRequired: action !== 'list' }, () => diff --git a/src/cli/connection/connect-provider-adapters.ts b/src/cli/connection/connect-provider-adapters.ts index d4de3385bc..a9087c699c 100644 --- a/src/cli/connection/connect-provider-adapters.ts +++ b/src/cli/connection/connect-provider-adapters.ts @@ -16,6 +16,7 @@ import { profileToCliFlags } from '../remote-config-flags.ts'; import { isConnectProviderName, type ConnectProvider } from './provider-policy.ts'; import { bundledCloudWebDriverConnection } from '../../provider-webdriver.ts'; import { withPluginConnection } from '@agent-device/plugins/load'; +import { PROVIDER_PLUGIN_POLICY } from '../../provider-plugin-policy.ts'; import type { PluginConnection } from '@agent-device/plugins/connection'; import { readMetroProfileFields } from './profile-fields.ts'; import { buildConnectClientId } from './client-id.ts'; @@ -88,7 +89,9 @@ async function withProviderConnection( run: (connection: PluginConnection) => Promise, ): Promise { const bundled = bundledCloudWebDriverConnection(provider, env); - return bundled ? await run(bundled) : await withPluginConnection(provider, env, run); + return bundled + ? await run(bundled) + : await withPluginConnection(provider, env, PROVIDER_PLUGIN_POLICY, run); } export async function resolveConnectProviderProfile(options: { diff --git a/src/cli/connection/provider-policy.ts b/src/cli/connection/provider-policy.ts index 3cd0871e7a..75117c7b89 100644 --- a/src/cli/connection/provider-policy.ts +++ b/src/cli/connection/provider-policy.ts @@ -4,7 +4,7 @@ import { pluginConnectionCapabilities, pluginConnectionNames, } from '@agent-device/plugins/connection'; -import { RESERVED_PLUGIN_PROVIDERS as BUILTIN_CONNECT_PROVIDERS } from '@agent-device/plugins/manifest'; +import { RESERVED_PLUGIN_PROVIDERS as BUILTIN_CONNECT_PROVIDERS } from '../../provider-plugin-policy.ts'; export type BuiltinConnectProvider = (typeof BUILTIN_CONNECT_PROVIDERS)[number]; export type ConnectProvider = BuiltinConnectProvider | (string & {}); diff --git a/src/provider-credential-fingerprint.ts b/src/provider-credential-fingerprint.ts index 125b330258..479c632e51 100644 --- a/src/provider-credential-fingerprint.ts +++ b/src/provider-credential-fingerprint.ts @@ -4,7 +4,7 @@ import { cloudWebDriverProviderDeclaration } from '@agent-device/provider-webdri import type { LIMRUN_PROVIDER } from '@agent-device/provider-limrun'; import type { EnvMap } from '@agent-device/kernel/source-value'; import { readLimrunCredentialValues } from './provider-limrun-credentials.ts'; -import { RESERVED_PLUGIN_PROVIDERS } from '@agent-device/plugins/manifest'; +import { RESERVED_PLUGIN_PROVIDERS } from './provider-plugin-policy.ts'; import { installedPlugins } from '@agent-device/plugins/store'; type CredentialValues = Readonly>; diff --git a/src/provider-device-runtimes.ts b/src/provider-device-runtimes.ts index 4dff7d4352..ec9705f5e9 100644 --- a/src/provider-device-runtimes.ts +++ b/src/provider-device-runtimes.ts @@ -50,11 +50,11 @@ export async function createDaemonProviderRuntimeComposition( ): Promise { const bundled = await createDefaultProviderRuntimeComposition(env); try { - const [{ loadProviderPlugins }, { RESERVED_PLUGIN_PROVIDERS }] = await Promise.all([ + const [{ loadProviderPlugins }, { PROVIDER_PLUGIN_POLICY }] = await Promise.all([ import('@agent-device/plugins/load'), - import('@agent-device/plugins/manifest'), + import('./provider-plugin-policy.ts'), ]); - const plugins = await loadProviderPlugins(env, RESERVED_PLUGIN_PROVIDERS); + const plugins = await loadProviderPlugins(env, PROVIDER_PLUGIN_POLICY); return Object.freeze({ ...bundled, runtimes: Object.freeze([...bundled.runtimes, ...plugins.map(({ runtime }) => runtime)]), diff --git a/src/provider-plugin-policy.test.ts b/src/provider-plugin-policy.test.ts new file mode 100644 index 0000000000..a3c9911b0f --- /dev/null +++ b/src/provider-plugin-policy.test.ts @@ -0,0 +1,164 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { afterEach, test, vi } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import type { DeviceLease } from '@agent-device/contracts/device'; +import { runCmd } from '@agent-device/host-kit/command'; +import { changePlugin, installedPlugins } from '@agent-device/plugins/store'; +import { loadProviderPlugins, withPluginConnection } from '@agent-device/plugins/load'; +import { + pluginHome, + registrationSource, + selectPlugin, + webDriverPluginSource, + writePlugin, +} from '@agent-device/test-kit'; +import { PROVIDER_PLUGIN_POLICY, RESERVED_PLUGIN_PROVIDERS } from './provider-plugin-policy.ts'; + +vi.mock('@agent-device/host-kit/command', async (importOriginal) => ({ + ...(await importOriginal()), + runCmd: vi.fn(), +})); + +const realFetch = globalThis.fetch; +afterEach(() => { + globalThis.fetch = realFetch; + vi.doUnmock('@agent-device/provider-webdriver/plugin'); +}); + +const lease: DeviceLease = { + leaseId: 'lease-1', + tenantId: 'team-a', + runId: 'run-a', + clientId: 'client-a', + leaseProvider: 'example', + backend: 'android-instance', + deviceKey: 'example:device-a', + createdAt: 1, + expiresAt: 2, + heartbeatAt: 1, +}; + +test('WebDriver plugins allocate through the shared engine and refuse mismatched providers', async () => { + const { home, env } = pluginHome(); + const source = webDriverPluginSource('example', ['awsProjectArn']); + selectPlugin(home, 'example', 'example', source); + const [registration] = await loadProviderPlugins(env, PROVIDER_PLUGIN_POLICY); + const runtime = registration!.runtime; + assert.equal(runtime.provider, 'example'); + assert.equal(registration!.platformModule.owner.provider, 'example'); + const requests: string[] = []; + globalThis.fetch = async (input, init) => { + requests.push(`${init?.method ?? 'GET'} ${String(input)}`); + return new Response(JSON.stringify({ value: { sessionId: 'SESSION1', capabilities: {} } })); + }; + try { + await assert.rejects( + runtime.leaseLifecycle.allocate!(lease, { flags: { awsProjectArn: 'arn:project' } }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.code, 'INVALID_ARGS'); + assert.equal(error.details?.provider, 'example'); + assert.deepEqual(error.details?.flags, ['--aws-project-arn']); + return true; + }, + ); + assert.deepEqual(requests, []); + const allocated = await runtime.leaseLifecycle.allocate!(lease, { flags: {} }); + assert.equal(allocated?.sessionId, 'SESSION1'); + assert.deepEqual(requests, ['POST https://webdriver.test/wd/hub/session']); + } finally { + await runtime.shutdown(); + } + const other = pluginHome(); + selectPlugin(other.home, 'example', 'wrong', source); + await assert.rejects(loadProviderPlugins(other.env, PROVIDER_PLUGIN_POLICY), { + code: 'INVALID_ARGS', + }); +}); + +test('a WebDriver plugin supplies connection callbacks and its runtime is released afterwards', async () => { + const { home, env } = pluginHome(); + selectPlugin( + home, + 'example', + 'example', + webDriverPluginSource( + 'example', + [], + "{ resolve: () => ({ profile: { leaseProvider: 'example', platform: 'android' } }), verify: async () => { throw host.createError('COMMAND_FAILED', 'verification failed'); } }", + ), + ); + const flags = { json: false, help: false, version: false }; + const resolved = await withPluginConnection( + 'example', + env, + PROVIDER_PLUGIN_POLICY, + async (connection) => await connection.resolve({ flags, stateDir: home, cwd: home, env }), + ); + assert.equal(resolved.profile.platform, 'android'); + await assert.rejects( + withPluginConnection( + 'example', + env, + PROVIDER_PLUGIN_POLICY, + async (connection) => await connection.verify({ flags, env }), + ), + { code: 'COMMAND_FAILED' }, + ); +}); + +test('native plugins load without evaluating the WebDriver runtime, WebDriver plugins do', async () => { + const evaluations = { count: 0 }; + vi.resetModules(); + vi.doMock('@agent-device/provider-webdriver/plugin', async (importOriginal) => { + evaluations.count += 1; + return await importOriginal(); + }); + const { loadProviderPlugins: load } = await import('@agent-device/plugins/load'); + const { PROVIDER_PLUGIN_POLICY: policy } = await import('./provider-plugin-policy.ts'); + const native = pluginHome(); + selectPlugin(native.home, 'example', 'example', registrationSource('example')); + const [registration] = await load(native.env, policy); + await registration!.runtime.shutdown(); + assert.equal(evaluations.count, 0); + const webDriver = pluginHome(); + selectPlugin(webDriver.home, 'example', 'example', webDriverPluginSource('example')); + const [adapted] = await load(webDriver.env, policy); + await adapted!.runtime.shutdown(); + assert.equal(evaluations.count, 1); +}); + +test('bundled and connect provider ids are refused at load before any plugin evaluates', async () => { + for (const provider of RESERVED_PLUGIN_PROVIDERS) { + const { home, env } = pluginHome(); + const marker = path.join(home, 'evaluated'); + selectPlugin( + home, + 'example', + provider, + `import fs from 'node:fs'; fs.writeFileSync(${JSON.stringify(marker)}, 'yes');`, + ); + await assert.rejects(loadProviderPlugins(env, PROVIDER_PLUGIN_POLICY), { + code: 'INVALID_ARGS', + message: `Duplicate provider plugin: ${provider}`, + }); + assert.ok(!fs.existsSync(marker), provider); + } +}); + +test('bundled and connect provider ids are refused at install and leave no selection', async () => { + for (const provider of RESERVED_PLUGIN_PROVIDERS) { + const { env } = pluginHome(); + vi.mocked(runCmd).mockImplementation(async (_command, _argv, options) => { + writePlugin(options!.cwd!, 'other', 1, provider); + return { stdout: '', stderr: '', exitCode: 0 }; + }); + await assert.rejects( + changePlugin('add', 'other', env, PROVIDER_PLUGIN_POLICY.reservedProviders), + { code: 'INVALID_ARGS', message: `Duplicate provider plugin: ${provider}` }, + ); + assert.deepEqual(installedPlugins(env), [], provider); + } +}); diff --git a/src/provider-plugin-policy.ts b/src/provider-plugin-policy.ts new file mode 100644 index 0000000000..b38e0f6e3d --- /dev/null +++ b/src/provider-plugin-policy.ts @@ -0,0 +1,45 @@ +import { AppError } from '@agent-device/kernel/errors'; +import type { PluginConnection } from '@agent-device/plugins/connection'; +import type { PluginLoadPolicy } from '@agent-device/plugins/load'; +import type { CloudWebDriverProviderOptions } from '@agent-device/provider-webdriver/plugin'; +import { + CLOUD_WEBDRIVER_PROVIDERS, + type CloudWebDriverKnownProviderName, +} from '@agent-device/provider-webdriver/providers'; + +/** The one list of provider ids plugins cannot claim: connect routes and bundled runtimes. */ +export const RESERVED_PLUGIN_PROVIDERS: readonly ( + | 'cloud' + | 'proxy' + | 'limrun' + | CloudWebDriverKnownProviderName +)[] = ['cloud', 'proxy', ...Object.values(CLOUD_WEBDRIVER_PROVIDERS), 'limrun']; + +type WebDriverFactoryResult = { + webDriver: CloudWebDriverProviderOptions; + connection?: PluginConnection; +}; + +/** + * The policy every plugin load, connection and installation in this process runs under. The + * WebDriver runtime is evaluated only for a plugin that returns `{ webDriver }`. + */ +export const PROVIDER_PLUGIN_POLICY: PluginLoadPolicy = Object.freeze({ + reservedProviders: RESERVED_PLUGIN_PROVIDERS, + adaptFactoryResult: async (result, { plugin, host }) => { + if (typeof result !== 'object' || result === null || !('webDriver' in result)) return undefined; + const { webDriver, connection } = result as WebDriverFactoryResult; + if (webDriver?.provider !== plugin.provider) { + throw new AppError( + 'INVALID_ARGS', + `WebDriver plugin provider does not match its declaration: ${plugin.name}`, + ); + } + const { createCloudWebDriverRuntime } = await import('@agent-device/provider-webdriver/plugin'); + const runtime = await createCloudWebDriverRuntime({ + ...webDriver, + clientVersion: host.clientVersion, + }); + return { runtime, platformModule: runtime.platformRuntimeModule, connection }; + }, +}); From b1166652af78f67cf44cc20eb0c637c141403b5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 15:59:15 +0200 Subject: [PATCH 02/13] chore(gates): forbid plugin framework dependencies on provider packages R85 rejects manifest entries and static, type-only, dynamic and mocked imports from packages/plugins to any packages/provider-* package, and declares the new root policy module's zone. --- scripts/layering/check.ts | 7 ++ .../plugin-framework-independence.test.ts | 96 +++++++++++++++++++ .../layering/plugin-framework-independence.ts | 90 +++++++++++++++++ scripts/layering/root-module-zones.ts | 1 + 4 files changed, 194 insertions(+) create mode 100644 scripts/layering/plugin-framework-independence.test.ts create mode 100644 scripts/layering/plugin-framework-independence.ts diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index 785a7e5a48..bcfb505a51 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -161,6 +161,7 @@ import { sourceExecutionCompatibilityViolations } from './source-execution-polic import { sessionResourceOwnershipViolations } from './session-resource-ownership.ts'; import { applicationLifecycleOwnershipViolations } from './application-lifecycle-policy.ts'; import { iosSnapshotEngineOwnershipViolations } from './ios-snapshot-engine-policy.ts'; +import { checkPluginFrameworkIndependence } from './plugin-framework-independence.ts'; import { providerSnapshotPresentationViolations } from './provider-snapshot-presentation-policy.ts'; import { snapshotAssemblyPresentationViolations } from './snapshot-assembly-presentation-policy.ts'; import { RETIRED_PATH_RULES, retiredPathRuleViolations } from './retired-paths-policy.ts'; @@ -539,6 +540,7 @@ export const LAYERING_RULE_IDS = [ 'session-authority-overlay', 'daemon-layers', 'package-export-surface', + 'plugin-framework-independence', 'test-colocation', ] as const; @@ -620,6 +622,11 @@ export const LAYERING_RULES: Readonly> = { ), ...checkTestKitIsolation(readTrackedPackageManifests(repoRoot), context.allTypeScriptSources), ], + 'plugin-framework-independence': (context) => + checkPluginFrameworkIndependence( + readTrackedPackageManifests(repoRoot), + context.allTypeScriptSources, + ), 'test-colocation': (context) => checkTestColocation(context.ratchets.unmatchedTests, context.reference.unmatchedTests), }; diff --git a/scripts/layering/plugin-framework-independence.test.ts b/scripts/layering/plugin-framework-independence.test.ts new file mode 100644 index 0000000000..4169d1743b --- /dev/null +++ b/scripts/layering/plugin-framework-independence.test.ts @@ -0,0 +1,96 @@ +// R85: the plugin framework imports and declares no concrete provider package. + +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { test } from 'node:test'; +import { + checkPluginFrameworkIndependence, + PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, +} from './plugin-framework-independence.ts'; +import { readTrackedPackageManifests } from './package-boundaries.ts'; +import { listTrackedTypeScriptFiles } from './tracked-sources.ts'; + +const repoRoot = path.resolve(import.meta.dirname, '../..'); +const PROVIDER_ENTRY = "'@agent-device/provider-webdriver/plugin'"; + +function manifests(plugins: Record = {}): Map { + return new Map([ + [ + 'packages/plugins/package.json', + JSON.stringify({ name: '@agent-device/plugins', ...plugins }), + ], + [ + 'packages/provider-webdriver/package.json', + JSON.stringify({ name: '@agent-device/provider-webdriver' }), + ], + ['packages/provider-testmu/package.json', JSON.stringify({ name: '@agent-device/testmu' })], + ['packages/kernel/package.json', JSON.stringify({ name: '@agent-device/kernel' })], + ]); +} + +function check(source: string, file = 'packages/plugins/src/load.ts') { + return checkPluginFrameworkIndependence(manifests(), new Map([[file, source]])); +} + +test('the real plugin framework declares and imports no provider package', () => { + const sources = new Map( + listTrackedTypeScriptFiles(repoRoot).map((file) => [ + file, + fs.readFileSync(path.join(repoRoot, file), 'utf8'), + ]), + ); + assert.deepEqual( + checkPluginFrameworkIndependence(readTrackedPackageManifests(repoRoot), sources), + [], + ); +}); + +test('R85 rejects a static, type-only, dynamic, re-export, inline-type or mocked provider import', () => { + for (const source of [ + `import { createCloudWebDriverRuntime } from ${PROVIDER_ENTRY};`, + `import type { CloudWebDriverProviderOptions } from ${PROVIDER_ENTRY};`, + `const { createCloudWebDriverRuntime } = await import(${PROVIDER_ENTRY});`, + `export { createCloudWebDriverRuntime } from ${PROVIDER_ENTRY};`, + `type Options = import(${PROVIDER_ENTRY}).CloudWebDriverProviderOptions;`, + `vi.mock(${PROVIDER_ENTRY}, () => ({}));`, + `import plugin from '@agent-device/testmu';`, + ]) { + const [result, ...rest] = check(source); + assert.equal(rest.length, 0, source); + assert.equal(result?.rule, PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, source); + assert.equal(result?.file, 'packages/plugins/src/load.ts', source); + } +}); + +test('R85 rejects a provider package listed in any dependency field of the plugins manifest', () => { + for (const field of [ + 'dependencies', + 'devDependencies', + 'optionalDependencies', + 'peerDependencies', + ]) { + const result = checkPluginFrameworkIndependence( + manifests({ [field]: { '@agent-device/provider-webdriver': 'workspace:*' } }), + new Map(), + ); + assert.deepEqual( + result.map(({ file }) => file), + ['packages/plugins/package.json'], + field, + ); + } +}); + +test('R85 admits the foundations the framework builds on and imports elsewhere in the repo', () => { + assert.deepEqual(check(`import { AppError } from '@agent-device/kernel/errors';`), []); + assert.deepEqual( + checkPluginFrameworkIndependence( + manifests({ dependencies: { '@agent-device/kernel': 'workspace:*' } }), + new Map([ + ['src/provider-plugin-policy.ts', `import('@agent-device/provider-webdriver/plugin');`], + ]), + ), + [], + ); +}); diff --git a/scripts/layering/plugin-framework-independence.ts b/scripts/layering/plugin-framework-independence.ts new file mode 100644 index 0000000000..06520aa14b --- /dev/null +++ b/scripts/layering/plugin-framework-independence.ts @@ -0,0 +1,90 @@ +// Catches: `@agent-device/plugins` — the manifest, store, host and loader every provider plugin +// runs under — acquiring a dependency on a concrete provider package, whether as a manifest entry +// or as a static, type-only, dynamic or `vi.mock` import. Bundled-provider knowledge (which ids +// plugins may not claim, how a `{ webDriver }` factory result becomes a runtime) belongs to the +// composition that selects providers, which hands it to the loader as a policy. +// Evidence: #3712 extracted the loader with `provider-webdriver` still imported by its manifest and +// loader; the provider-independence follow-up moved that knowledge to src/provider-plugin-policy.ts. +// Cost: one pass over the tracked manifests and the plugins package's sources. +// Kill criterion: a maintainer decision that the plugin framework may depend on a concrete +// provider, or that provider-* packages stop being the unit of provider implementation. +// +// The forbidden set is derived from the workspace layout, not from name patterns: every package +// under `packages/provider-*`, whatever name it publishes (e.g. `@agent-device/testmu`). + +import type { LayeringViolation } from './model.ts'; +import { specifierSites } from './package-boundaries.ts'; + +export const PLUGIN_FRAMEWORK_INDEPENDENCE_RULE = 'R85 plugin-framework-independence'; + +const PLUGINS_DIR = 'packages/plugins'; +const PROVIDER_MANIFEST = /^packages\/provider-[^/]+\/package\.json$/; + +type DependencyManifest = { + name?: string; + dependencies?: Record; + devDependencies?: Record; + optionalDependencies?: Record; + peerDependencies?: Record; +}; + +function declaredNames(manifest: DependencyManifest): string[] { + return [ + ...Object.keys(manifest.dependencies ?? {}), + ...Object.keys(manifest.devDependencies ?? {}), + ...Object.keys(manifest.optionalDependencies ?? {}), + ...Object.keys(manifest.peerDependencies ?? {}), + ]; +} + +function packageName(specifier: string): string { + const parts = specifier.split('/'); + return specifier.startsWith('@') ? parts.slice(0, 2).join('/') : parts[0]!; +} + +function violation(file: string, line: number, message: string): LayeringViolation { + return { rule: PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, file, line, message }; +} + +/** The plugin framework package declares and imports no `packages/provider-*` package. */ +export function checkPluginFrameworkIndependence( + manifests: ReadonlyMap, + sources: ReadonlyMap, +): LayeringViolation[] { + const providers = new Set(); + for (const [file, text] of manifests) { + const { name } = JSON.parse(text) as DependencyManifest; + if (PROVIDER_MANIFEST.test(file) && name) providers.add(name); + } + + const violations: LayeringViolation[] = []; + const pluginsManifest = `${PLUGINS_DIR}/package.json`; + const manifest = manifests.get(pluginsManifest); + for (const name of manifest ? declaredNames(JSON.parse(manifest) as DependencyManifest) : []) { + if (providers.has(name)) { + violations.push( + violation( + pluginsManifest, + 1, + `${name} is a provider package; the plugin framework must not declare it. ` + + `Provider knowledge reaches the loader as a policy from the composing root.`, + ), + ); + } + } + for (const [file, source] of sources) { + if (!file.startsWith(`${PLUGINS_DIR}/`)) continue; + for (const site of specifierSites(file, source)) { + if (!providers.has(packageName(site.specifier))) continue; + violations.push( + violation( + file, + site.line, + `${site.specifier} is a provider package; the plugin framework must not import it ` + + `(static, type-only, dynamic or mocked). Pass provider knowledge in as a policy from the composing root.`, + ), + ); + } + } + return violations; +} diff --git a/scripts/layering/root-module-zones.ts b/scripts/layering/root-module-zones.ts index 899aa752a9..bbd5ee733e 100644 --- a/scripts/layering/root-module-zones.ts +++ b/scripts/layering/root-module-zones.ts @@ -51,6 +51,7 @@ export const ROOT_MODULE_ZONES: Readonly> = { 'src/provider-credential-fingerprint.ts', 'src/provider-device-runtimes.ts', 'src/provider-limrun-credentials.ts', + 'src/provider-plugin-policy.ts', 'src/provider-webdriver.ts', ], // The operation host the platform runtime loads through `import()` (`loadHost`) and its facets. From 02bbf31c5494df4edc169f44f1ddf59ee68a2abd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 16:32:23 +0200 Subject: [PATCH 03/13] test(plugins): observe the runtime release after a WebDriver plugin connection --- src/provider-plugin-policy.test.ts | 36 +++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/provider-plugin-policy.test.ts b/src/provider-plugin-policy.test.ts index a3c9911b0f..2e8ce91dca 100644 --- a/src/provider-plugin-policy.test.ts +++ b/src/provider-plugin-policy.test.ts @@ -6,7 +6,7 @@ import { AppError } from '@agent-device/kernel/errors'; import type { DeviceLease } from '@agent-device/contracts/device'; import { runCmd } from '@agent-device/host-kit/command'; import { changePlugin, installedPlugins } from '@agent-device/plugins/store'; -import { loadProviderPlugins, withPluginConnection } from '@agent-device/plugins/load'; +import { loadProviderPlugins } from '@agent-device/plugins/load'; import { pluginHome, registrationSource, @@ -79,6 +79,28 @@ test('WebDriver plugins allocate through the shared engine and refuse mismatched }); test('a WebDriver plugin supplies connection callbacks and its runtime is released afterwards', async () => { + const shutdowns: string[] = []; + vi.resetModules(); + vi.doMock('@agent-device/provider-webdriver/plugin', async (importOriginal) => { + const original = + await importOriginal(); + return { + ...original, + createCloudWebDriverRuntime: async ( + ...args: Parameters + ) => { + const runtime = await original.createCloudWebDriverRuntime(...args); + const shutdown = runtime.shutdown.bind(runtime); + runtime.shutdown = async () => { + shutdowns.push(runtime.provider); + await shutdown(); + }; + return runtime; + }, + }; + }); + const { withPluginConnection: withConnection } = await import('@agent-device/plugins/load'); + const { PROVIDER_PLUGIN_POLICY: policy } = await import('./provider-plugin-policy.ts'); const { home, env } = pluginHome(); selectPlugin( home, @@ -91,22 +113,24 @@ test('a WebDriver plugin supplies connection callbacks and its runtime is releas ), ); const flags = { json: false, help: false, version: false }; - const resolved = await withPluginConnection( + const resolved = await withConnection( 'example', env, - PROVIDER_PLUGIN_POLICY, + policy, async (connection) => await connection.resolve({ flags, stateDir: home, cwd: home, env }), ); assert.equal(resolved.profile.platform, 'android'); + assert.deepEqual(shutdowns, ['example']); await assert.rejects( - withPluginConnection( + withConnection( 'example', env, - PROVIDER_PLUGIN_POLICY, + policy, async (connection) => await connection.verify({ flags, env }), ), - { code: 'COMMAND_FAILED' }, + { code: 'COMMAND_FAILED', message: 'verification failed' }, ); + assert.deepEqual(shutdowns, ['example', 'example']); }); test('native plugins load without evaluating the WebDriver runtime, WebDriver plugins do', async () => { From fbcf4e531165836cd6943f3aea2d41588b5c11ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 16:32:23 +0200 Subject: [PATCH 04/13] chore(gates): read Vitest mock-family module sites from the AST specifierSites matched only quoted vi.mock-style arguments by regex, so a template-literal, computed-member, importMock or doUnmock provider mock bypassed R85. The shared extractor now parses mock-family calls, which also stops comments from counting as sites. --- scripts/layering/model.test.ts | 64 +++++++++++++++++++ scripts/layering/model.ts | 53 ++++++++++++++- ...package-boundaries-specifier-sites.test.ts | 48 ++++++++++++++ scripts/layering/package-boundaries.ts | 23 +++---- .../plugin-framework-independence.test.ts | 17 +++++ 5 files changed, 190 insertions(+), 15 deletions(-) create mode 100644 scripts/layering/package-boundaries-specifier-sites.test.ts diff --git a/scripts/layering/model.test.ts b/scripts/layering/model.test.ts index 818b8dd51b..6c507ddafb 100644 --- a/scripts/layering/model.test.ts +++ b/scripts/layering/model.test.ts @@ -22,6 +22,7 @@ import { collectZones, findValueImportCycles, parseImports, + parseModuleMockSpecifiers, resolveImportEdges, unclassifiedZones, } from './model.ts'; @@ -793,3 +794,66 @@ test('largestTypeCycleSize counts type-only cycles and ignores dynamic ones', () ); assert.equal(largestTypeCycleSize(valueCycle), 2); }); + +test('parseModuleMockSpecifiers reads every Vitest mock-family call with a literal module', () => { + const source = [ + "vi.mock('a', () => ({}));", + 'vi.mock("b");', + 'vi.mock(`c`, () => ({}));', + "vi.doMock('d');", + "vi.unmock('e');", + 'vi.doUnmock(`f`);', + "await vi.importActual('h');", + "await vi.importMock('i');", + "vi['mock']('j');", + "vi[`doMock`]('k');", + "vi?.mock('l');", + "vitest.mock('m');", + "vi.mock(('n' as string));", + ].join('\n'); + assert.deepEqual( + parseModuleMockSpecifiers(source), + [ + ['a', 1], + ['b', 2], + ['c', 3], + ['d', 4], + ['e', 5], + ['f', 6], + ['h', 7], + ['i', 8], + ['j', 9], + ['k', 10], + ['l', 11], + ['m', 12], + ['n', 13], + ].map(([spec, line]) => ({ spec, line })), + ); + assert.deepEqual(parseModuleMockSpecifiers('\n\nvi.mock(\n `multi`,\n);'), [ + { spec: 'multi', line: 3 }, + ]); +}); + +test('parseModuleMockSpecifiers ignores comments, unrelated objects and non-literal arguments', () => { + const source = [ + "// vi.mock('commented');", + "/* vi.doMock('block') */", + 'const text = "vi.mock(\'in-string\')";', + "other.mock('unrelated');", + "vi.fn('not-a-module-api');", + "vi.mockObject('not-a-module-api');", + "vi.spyOn('not-a-module-api');", + "vi.mocked('not-a-module-api');", + "vi[method]('computed-member');", + 'vi.mock(`interpolated-${name}`);', + 'vi.mock(variable);', + 'vi.mock();', + "vi.mock(import('dynamic-form'));", + "vi.mock.mockReset('nested');", + ].join('\n'); + assert.deepEqual(parseModuleMockSpecifiers(source), []); +}); + +test('parseModuleMockSpecifiers rejects malformed source instead of returning partial sites', () => { + assert.throws(() => parseModuleMockSpecifiers("vi.mock('a'"), SyntaxError); +}); diff --git a/scripts/layering/model.ts b/scripts/layering/model.ts index 008e42894f..04e43bc031 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -3,7 +3,7 @@ import { DAEMON_SOURCE_ROOT } from './tracked-sources.ts'; import { PLATFORMS } from '@agent-device/kernel/device'; import { createGraph, genDFS, getStronglyConnectedComponents, getSubgraph } from '@statelyai/graph'; import { parseSync } from 'oxc-parser'; -import { destructuredDynamicImportBindings, visitAst } from './layering-ast.ts'; +import { destructuredDynamicImportBindings, memberName, visitAst } from './layering-ast.ts'; import { declaredRootModuleZone } from './root-module-zones.ts'; import { importGraphFromResolvedEdges, @@ -296,6 +296,57 @@ export function parseImports(source: string): ImportEdge[] { .map(({ edge }) => edge); } +const MODULE_MOCK_RECEIVERS = new Set(['vi', 'vitest']); +// Vitest's module-path-taking APIs: every member of `vi` whose first argument names a module. +const MODULE_MOCK_METHODS = new Set([ + 'mock', + 'doMock', + 'mockActual', + 'unmock', + 'doUnmock', + 'importActual', + 'importMock', +]); + +function mockCallMethod(callee: unknown): string | undefined { + if (callee === null || typeof callee !== 'object') return undefined; + const record = callee as Record; + if (record.type === 'ChainExpression') return mockCallMethod(record.expression); + if (record.type !== 'MemberExpression') return undefined; + const object = record.object as Record | null; + if (object?.type !== 'Identifier' || !MODULE_MOCK_RECEIVERS.has(object.name as string)) { + return undefined; + } + const method = record.computed === true ? literalSpecifier(record.property) : memberName(record); + return method !== undefined && MODULE_MOCK_METHODS.has(method) ? method : undefined; +} + +/** + * Module specifiers named by Vitest mock-family calls (`vi.mock('x')`, `vi.doMock`, `vi.unmock`, + * `vi.importActual`, `vi.importMock`, ...), with their 1-based lines. The loader resolves and + * instantiates the named module under test just as an `import` would, yet `parseImports` never + * sees a call expression. Only a literal first argument counts — a quoted string or a template + * without substitutions; `vi.mock(import('x'))` is already a dynamic `ImportExpression` edge. + */ +export function parseModuleMockSpecifiers(source: string): { spec: string; line: number }[] { + const parsed = parseSync('layering-mocks.ts', source); + if (parsed.errors.length > 0) { + throw new SyntaxError( + `Cannot parse mocks: ${parsed.errors.map(({ message }) => message).join('; ')}`, + ); + } + const sites: { spec: string; line: number }[] = []; + visitAst(parsed.program, (node) => { + if (node.type !== 'CallExpression' || mockCallMethod(node.callee) === undefined) return; + const args = node.arguments; + const spec = Array.isArray(args) ? literalSpecifier(args[0]) : undefined; + if (spec !== undefined) { + sites.push({ spec, line: sourceLine(source, node.start as number | undefined) }); + } + }); + return sites; +} + export function topFolder(file: string): string { const packageMatch = /^packages\/([^/]+)\//.exec(file); if (packageMatch) return packageMatch[1]!; diff --git a/scripts/layering/package-boundaries-specifier-sites.test.ts b/scripts/layering/package-boundaries-specifier-sites.test.ts new file mode 100644 index 0000000000..1cc843bc7c --- /dev/null +++ b/scripts/layering/package-boundaries-specifier-sites.test.ts @@ -0,0 +1,48 @@ +// `specifierSites` is the one literal-site extractor every R11/R83/R85 walk shares: import edges +// plus Vitest mock-family module arguments. Compact sibling of package-boundaries.test.ts, which +// sits at its size ratchet. + +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { specifierSites } from './package-boundaries.ts'; + +function sites(source: string): string[] { + return specifierSites('src/a.test.ts', source).map( + ({ line, specifier }) => `${line}:${specifier}`, + ); +} + +test('specifier sites carry mock-family module arguments with their lines', () => { + assert.deepEqual( + sites( + [ + "vi.mock('a');", + 'vi.mock(`b`);', + "vi['doMock']('c');", + "await vi.importMock('d');", + 'vi.doUnmock(`e`);', + "import { x } from './f.ts';", + ].join('\n'), + ), + ['1:a', '2:b', '3:c', '4:d', '5:e', '6:./f.ts'], + ); +}); + +test('specifier sites report a module once when an import edge and a mock call share its line', () => { + assert.deepEqual(sites("vi.mock(import('a'), () => ({}));"), ['1:a']); + assert.deepEqual(sites("const a = await vi.importActual('a');"), ['1:a']); +}); + +test('specifier sites skip comments, interpolated templates and unrelated objects', () => { + assert.deepEqual( + sites( + [ + "// vi.mock('commented');", + 'vi.mock(`interpolated-${name}`);', + "other.mock('unrelated');", + "vi[method]('computed');", + ].join('\n'), + ), + [], + ); +}); diff --git a/scripts/layering/package-boundaries.ts b/scripts/layering/package-boundaries.ts index ab88b1aa54..e8534b8216 100644 --- a/scripts/layering/package-boundaries.ts +++ b/scripts/layering/package-boundaries.ts @@ -31,7 +31,7 @@ import fs from 'node:fs'; import path from 'node:path'; -import { parseImports, resolveWorkspaceSpecifier } from './model.ts'; +import { parseImports, parseModuleMockSpecifiers, resolveWorkspaceSpecifier } from './model.ts'; import { PACKAGE_EXPORT_SURFACE_RULE, readExportManifests, @@ -86,27 +86,22 @@ export type SpecifierSite = { * and both quote styles are covered by one scanner instead of a private regex * that silently missed double-quoted routes. * - * `vi.mock`/`vi.doMock`/`vi.mockActual`/`vi.unmock`/`vi.importActual` join the sweep - * by literal scan: they are plain call expressions the AST import parser never - * visits, yet the loader resolves their specifier and instantiates that module under - * test — the same double-instance route a relative tunnel takes. `importOriginal - * ` needs no extra branch; the parser already sees it as a + * Vitest mock-family calls (`vi.mock`, `vi.doMock`, `vi.unmock`, `vi.importActual`, + * `vi.importMock`, ...) join the sweep through `parseModuleMockSpecifiers`: they are plain call + * expressions the import parser never visits, yet the loader resolves their specifier and + * instantiates that module under test — the same double-instance route a relative tunnel takes. + * `importOriginal` needs no extra branch; the parser already sees it as a * TSImportType. */ -const VI_MOCK_CALL_RE = - /\bvi\.(?:mock|doMock|mockActual|unmock|importActual)\(\s*(['"])([^'"\n]*)\1/g; - export function specifierSites(file: string, source: string): SpecifierSite[] { const sites: SpecifierSite[] = parseImports(source).map((edge) => ({ file, line: edge.line, specifier: edge.spec, })); - for (const match of source.matchAll(VI_MOCK_CALL_RE)) { - const line = source.slice(0, match.index).split('\n').length; - const specifier = match[2]!; - if (!sites.some((site) => site.line === line && site.specifier === specifier)) - sites.push({ file, line, specifier }); + for (const { spec, line } of parseModuleMockSpecifiers(source)) { + if (!sites.some((site) => site.line === line && site.specifier === spec)) + sites.push({ file, line, specifier: spec }); } return sites.sort((a, b) => a.line - b.line); } diff --git a/scripts/layering/plugin-framework-independence.test.ts b/scripts/layering/plugin-framework-independence.test.ts index 4169d1743b..e5a882d65d 100644 --- a/scripts/layering/plugin-framework-independence.test.ts +++ b/scripts/layering/plugin-framework-independence.test.ts @@ -54,6 +54,13 @@ test('R85 rejects a static, type-only, dynamic, re-export, inline-type or mocked `export { createCloudWebDriverRuntime } from ${PROVIDER_ENTRY};`, `type Options = import(${PROVIDER_ENTRY}).CloudWebDriverProviderOptions;`, `vi.mock(${PROVIDER_ENTRY}, () => ({}));`, + `vi.mock(${PROVIDER_ENTRY.replaceAll("'", '`')}, () => ({}));`, + `vi['mock'](${PROVIDER_ENTRY}, () => ({}));`, + `vi.doMock(${PROVIDER_ENTRY});`, + `vi.unmock(${PROVIDER_ENTRY});`, + `await vi.importActual(${PROVIDER_ENTRY});`, + `await vi.importMock(${PROVIDER_ENTRY});`, + `vi.mock(import(${PROVIDER_ENTRY}));`, `import plugin from '@agent-device/testmu';`, ]) { const [result, ...rest] = check(source); @@ -94,3 +101,13 @@ test('R85 admits the foundations the framework builds on and imports elsewhere i [], ); }); + +test('R85 ignores a provider named only in a comment, a non-literal mock argument or another object', () => { + for (const source of [ + `// vi.mock(${PROVIDER_ENTRY});`, + 'vi.mock(`@agent-device/provider-webdriver/${entry}`);', + `other.mock(${PROVIDER_ENTRY});`, + ]) { + assert.deepEqual(check(source), [], source); + } +}); From e063c1eb9b74ea3b29ff2738ce055130528aa1e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 16:56:46 +0200 Subject: [PATCH 05/13] chore(gates): resolve Vitest import aliases when reading mock-family module sites parseModuleMockSpecifiers matched only the global vi/vitest receivers, so import { vi as mocks } from 'vitest' or a vitest namespace import hid a provider mock from R85 and every other specifierSites walk. The shared extractor now resolves top-level value imports from vitest (named vi/vitest aliases and namespace members), honours lexical redeclaration of those local names, and still ignores type-only imports, other modules and other exports. Bindings made by a dynamic import of vitest are not followed. --- scripts/layering/layering-ast.ts | 125 +++++++++++++ scripts/layering/model-mock-bindings.test.ts | 167 ++++++++++++++++++ scripts/layering/model.ts | 119 +++++++++++-- ...package-boundaries-specifier-sites.test.ts | 33 ++++ .../plugin-framework-independence.test.ts | 9 + 5 files changed, 441 insertions(+), 12 deletions(-) create mode 100644 scripts/layering/model-mock-bindings.test.ts diff --git a/scripts/layering/layering-ast.ts b/scripts/layering/layering-ast.ts index 9c9e31572a..970dcf16dc 100644 --- a/scripts/layering/layering-ast.ts +++ b/scripts/layering/layering-ast.ts @@ -1,6 +1,18 @@ /** Shared AST helpers for permanent layering policies. */ export type ProductionSource = Readonly<{ path: string; source: string }>; +export type AstNode = Record; + +export function asNode(value: unknown): AstNode | undefined { + return value !== null && typeof value === 'object' && !Array.isArray(value) + ? (value as AstNode) + : undefined; +} + +export function nodeList(value: unknown): unknown[] { + return Array.isArray(value) ? value : []; +} + export function memberName(node: Record): string | undefined { const property = node.property as Record | undefined; if (!property) return undefined; @@ -122,3 +134,116 @@ export function destructuredDynamicImportBindings( visit(program); return captures; } + +function patternNames(pattern: unknown, into: string[]): void { + const node = asNode(pattern); + if (node === undefined) return; + switch (node.type) { + case 'Identifier': + into.push(node.name as string); + break; + case 'ObjectPattern': + for (const property of nodeList(node.properties)) { + const entry = asNode(property); + patternNames(entry?.type === 'Property' ? entry.value : entry, into); + } + break; + case 'ArrayPattern': + for (const element of nodeList(node.elements)) patternNames(element, into); + break; + case 'AssignmentPattern': + patternNames(node.left, into); + break; + case 'RestElement': + patternNames(node.argument, into); + break; + case 'TSParameterProperty': + patternNames(node.parameter, into); + break; + } +} + +function lexicalNames(statements: unknown[], into: string[]): void { + for (const statement of statements) { + const node = asNode(statement); + if (node?.type === 'VariableDeclaration' && node.kind !== 'var') { + for (const declarator of nodeList(node.declarations)) { + patternNames(asNode(declarator)?.id, into); + } + } else if (node?.type === 'FunctionDeclaration' || node?.type === 'ClassDeclaration') { + patternNames(node.id, into); + } + } +} + +function isFunctionNode(node: AstNode): boolean { + return ( + node.type === 'FunctionDeclaration' || + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + ); +} + +function hoistedVarNames(node: unknown, into: string[]): void { + for (const child of Array.isArray(node) ? node : [node]) { + const record = asNode(child); + if (record === undefined || isFunctionNode(record)) continue; + if (record.type === 'VariableDeclaration' && record.kind === 'var') { + for (const declarator of nodeList(record.declarations)) { + patternNames(asNode(declarator)?.id, into); + } + } + for (const value of Object.values(record)) hoistedVarNames(value, into); + } +} + +/** Names `node` itself declares for its own subtree: parameters, `var`s, block-level lexical declarations. */ +function scopeBindings(node: AstNode): string[] { + const names: string[] = []; + if (isFunctionNode(node)) { + for (const param of nodeList(node.params)) patternNames(param, names); + if (node.type === 'FunctionExpression') patternNames(node.id, names); + hoistedVarNames(node.body, names); + } else if ( + node.type === 'BlockStatement' || + node.type === 'StaticBlock' || + node.type === 'TSModuleBlock' + ) { + lexicalNames(nodeList(node.body), names); + } else if (node.type === 'ClassExpression') { + patternNames(node.id, names); + } else if (node.type === 'SwitchStatement') { + for (const switchCase of nodeList(node.cases)) { + lexicalNames(nodeList(asNode(switchCase)?.consequent), names); + } + } else if (node.type === 'ForStatement') { + lexicalNames([node.init], names); + } else if (node.type === 'ForInStatement' || node.type === 'ForOfStatement') { + lexicalNames([node.left], names); + } else if (node.type === 'CatchClause') { + patternNames(node.param, names); + } + return names; +} + +/** + * `visitAst` that also hands the visitor the names declared by the enclosing scopes: function + * parameters and `var`s, block-level `let`/`const`/`function`/`class`, loop heads and `catch` + * parameters. A `with` statement or `eval` can rebind names invisibly and is not modelled. + */ +export function visitAstScoped( + node: unknown, + shadowed: ReadonlySet, + visitor: (node: AstNode, shadowed: ReadonlySet) => void, +): void { + if (Array.isArray(node)) { + for (const child of node) visitAstScoped(child, shadowed, visitor); + return; + } + const record = asNode(node); + if (record === undefined) return; + const declared = scopeBindings(record); + const scope = declared.length > 0 ? new Set([...shadowed, ...declared]) : shadowed; + visitor(record, scope); + for (const child of Object.values(record)) visitAstScoped(child, scope, visitor); +} diff --git a/scripts/layering/model-mock-bindings.test.ts b/scripts/layering/model-mock-bindings.test.ts new file mode 100644 index 0000000000..02182b2031 --- /dev/null +++ b/scripts/layering/model-mock-bindings.test.ts @@ -0,0 +1,167 @@ +// How `parseModuleMockSpecifiers` resolves the receiver of a mock-family call: the global +// `vi`/`vitest`, or a local name an `import ... from 'vitest'` declaration binds to them. + +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { parseModuleMockSpecifiers } from './model.ts'; + +function specs(...lines: string[]): string[] { + return parseModuleMockSpecifiers(lines.join('\n')).map(({ spec }) => spec); +} + +test('a named import aliasing vi or vitest is a mock receiver', () => { + assert.deepEqual( + specs( + "import { vi as mocks, vitest as runner } from 'vitest';", + "mocks.mock('a');", + "runner.doMock('b');", + "mocks['unmock']('c');", + "await mocks.importActual('d');", + "mocks?.importMock('e');", + "(mocks as typeof vi).mock('f');", + "mocks!.doUnmock('g');", + ), + ['a', 'b', 'c', 'd', 'e', 'f', 'g'], + ); +}); + +test('a string-named import and an unaliased import resolve like a named alias', () => { + assert.deepEqual( + specs("import { 'vi' as quoted, vi } from 'vitest';", "quoted.mock('a');", "vi.mock('b');"), + ['a', 'b'], + ); +}); + +test('a vitest namespace import reaches the mock API through vi and vitest members', () => { + assert.deepEqual( + specs( + "import * as testing from 'vitest';", + "testing.vi.mock('a');", + "testing.vitest.doMock('b');", + "testing['vi'].unmock('c');", + "testing.vi['importActual']('d');", + "testing[`vitest`].importMock('e');", + "testing?.vi?.mock('f');", + "(testing.vi as typeof vi).mock('g');", + ), + ['a', 'b', 'c', 'd', 'e', 'f', 'g'], + ); +}); + +test('only a value import from vitest binds a mock receiver', () => { + assert.deepEqual( + specs( + "import { vi as fromOther } from 'not-vitest';", + "import { vi as fromRelative } from './vitest.ts';", + "import type { vi as typeDeclaration } from 'vitest';", + "import { type vi as typeSpecifier } from 'vitest';", + "import { expect as notMock, describe as alsoNotMock } from 'vitest';", + "import * as otherNamespace from 'not-vitest';", + "import type * as typeNamespace from 'vitest';", + "fromOther.mock('a');", + "fromRelative.mock('b');", + "typeDeclaration.mock('c');", + "typeSpecifier.mock('d');", + "notMock.mock('e');", + "alsoNotMock.mock('f');", + "otherNamespace.vi.mock('g');", + "typeNamespace.vi.mock('h');", + ), + [], + ); +}); + +test('a vitest namespace member other than vi or vitest is not the mock API', () => { + assert.deepEqual( + specs( + "import * as testing from 'vitest';", + "testing.expect.mock('a');", + "testing[member].mock('b');", + "testing['expect'].mock('c');", + ), + [], + ); +}); + +test('an alias is not a mock receiver where an enclosing scope redeclares its name', () => { + assert.deepEqual( + specs( + "import { vi as mocks } from 'vitest';", + "import * as testing from 'vitest';", + "function parameter(mocks) { mocks.mock('a'); }", + "const arrow = (testing) => testing.vi.mock('b');", + "{ const mocks = other; mocks.mock('c'); }", + "function hoisted() { if (flag) { var mocks = other; } mocks.mock('d'); }", + "try {} catch (mocks) { mocks.mock('e'); }", + "for (const { mocks } of list) mocks.mock('f');", + "switch (kind) { case 1: class testing {} testing.vi.mock('g'); }", + "const expression = function mocks() { mocks.mock('h'); };", + "const named = class mocks { run() { mocks.mock('i'); } };", + "namespace Fixture { const testing = other; testing.vi.mock('j'); }", + ), + [], + ); + assert.deepEqual( + specs( + "import { vi as mocks } from 'vitest';", + 'function unrelated(mocks) { return mocks; }', + '{ const mocks = other; }', + "mocks.mock('outside');", + ), + ['outside'], + ); +}); + +test('the global vi and vitest names stay mock receivers beside import aliases', () => { + assert.deepEqual( + specs( + "import { vi as mocks } from 'vitest';", + "vi.mock('a');", + "vitest.mock('b');", + "async function setup() { const { vi } = await import('vitest'); vi.doMock('c'); }", + ), + ['a', 'b', 'c'], + ); +}); + +test('an alias call still ignores comments, non-literal modules and non-module methods', () => { + assert.deepEqual( + specs( + "import { vi as mocks } from 'vitest';", + "import * as testing from 'vitest';", + "// mocks.mock('commented');", + "/* testing.vi.doMock('block') */", + 'const text = "mocks.mock(\'in-string\')";', + 'mocks.mock(variable);', + 'mocks.mock(`interpolated-${name}`);', + 'testing.vi.mock();', + "mocks.fn('not-a-module-api');", + "mocks.mocked('not-a-module-api');", + "mocks[method]('computed-member');", + "mocks.mock.mockReset('nested');", + "mocks('call-not-member');", + ), + [], + ); +}); + +test('a Vitest binding made by a dynamic import is not followed', () => { + assert.deepEqual( + specs( + "const { vi: mocks } = await import('vitest');", + "const testing = await import('vitest');", + "mocks.mock('a');", + "testing.vi.mock('b');", + ), + [], + ); +}); + +test('a module named by an alias call keeps its source line', () => { + assert.deepEqual( + parseModuleMockSpecifiers( + "import { vi as mocks } from 'vitest';\n\nmocks.mock(\n `multi`,\n);", + ), + [{ spec: 'multi', line: 3 }], + ); +}); diff --git a/scripts/layering/model.ts b/scripts/layering/model.ts index 04e43bc031..b0f219d144 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -3,7 +3,16 @@ import { DAEMON_SOURCE_ROOT } from './tracked-sources.ts'; import { PLATFORMS } from '@agent-device/kernel/device'; import { createGraph, genDFS, getStronglyConnectedComponents, getSubgraph } from '@statelyai/graph'; import { parseSync } from 'oxc-parser'; -import { destructuredDynamicImportBindings, memberName, visitAst } from './layering-ast.ts'; +import { + asNode, + destructuredDynamicImportBindings, + memberName, + nodeList, + propertyName, + visitAst, + visitAstScoped, + type AstNode, +} from './layering-ast.ts'; import { declaredRootModuleZone } from './root-module-zones.ts'; import { importGraphFromResolvedEdges, @@ -307,17 +316,96 @@ const MODULE_MOCK_METHODS = new Set([ 'importActual', 'importMock', ]); +const EXPRESSION_WRAPPERS = new Set([ + 'ChainExpression', + 'ParenthesizedExpression', + 'TSAsExpression', + 'TSSatisfiesExpression', + 'TSNonNullExpression', + 'TSTypeAssertion', +]); -function mockCallMethod(callee: unknown): string | undefined { - if (callee === null || typeof callee !== 'object') return undefined; - const record = callee as Record; - if (record.type === 'ChainExpression') return mockCallMethod(record.expression); - if (record.type !== 'MemberExpression') return undefined; - const object = record.object as Record | null; - if (object?.type !== 'Identifier' || !MODULE_MOCK_RECEIVERS.has(object.name as string)) { - return undefined; +/** Local names a file binds to Vitest's `vi`/`vitest` (`apis`) or to the whole module (`namespaces`). */ +type VitestBindings = { apis: ReadonlySet; namespaces: ReadonlySet }; + +function unwrapExpression(node: unknown): AstNode | undefined { + let current = asNode(node); + while (current !== undefined && EXPRESSION_WRAPPERS.has(current.type as string)) { + current = asNode(current.expression); } - const method = record.computed === true ? literalSpecifier(record.property) : memberName(record); + return current; +} + +function memberKey(member: AstNode): string | undefined { + return member.computed === true ? literalSpecifier(member.property) : memberName(member); +} + +/** + * Value bindings from top-level `import ... from 'vitest'` declarations: `{ vi as x }` and + * `{ vitest as x }` name the mock API, `* as ns` names a namespace whose `vi`/`vitest` members are + * the mock API. Type-only declarations and specifiers bind no value, and every other export or + * module binds nothing here. + */ +function vitestImportBindings(program: unknown): VitestBindings { + const apis = new Set(); + const namespaces = new Set(); + for (const statement of nodeList(asNode(program)?.body)) { + const declaration = asNode(statement); + if ( + declaration?.type !== 'ImportDeclaration' || + declaration.importKind === 'type' || + literalSpecifier(declaration.source) !== 'vitest' + ) { + continue; + } + for (const entry of nodeList(declaration.specifiers)) { + const specifier = asNode(entry); + const local = asNode(specifier?.local)?.name; + if (specifier === undefined || specifier.importKind === 'type' || typeof local !== 'string') { + continue; + } + if (specifier.type === 'ImportNamespaceSpecifier') namespaces.add(local); + else if (MODULE_MOCK_RECEIVERS.has(propertyName(specifier.imported) ?? '')) apis.add(local); + } + } + return { apis, namespaces }; +} + +/** + * The global names `vi` and `vitest` always count. An imported alias or namespace counts only + * while no enclosing scope redeclares its local name. + */ +function isMockApiReceiver( + node: unknown, + bindings: VitestBindings, + shadowed: ReadonlySet, +): boolean { + const receiver = unwrapExpression(node); + if (receiver?.type === 'Identifier') { + const name = receiver.name as string; + return MODULE_MOCK_RECEIVERS.has(name) || (bindings.apis.has(name) && !shadowed.has(name)); + } + if (receiver?.type !== 'MemberExpression') return false; + const owner = unwrapExpression(receiver.object); + const key = memberKey(receiver); + return ( + owner?.type === 'Identifier' && + bindings.namespaces.has(owner.name as string) && + !shadowed.has(owner.name as string) && + key !== undefined && + MODULE_MOCK_RECEIVERS.has(key) + ); +} + +function mockCallMethod( + callee: unknown, + bindings: VitestBindings, + shadowed: ReadonlySet, +): string | undefined { + const member = unwrapExpression(callee); + if (member?.type !== 'MemberExpression') return undefined; + if (!isMockApiReceiver(member.object, bindings, shadowed)) return undefined; + const method = memberKey(member); return method !== undefined && MODULE_MOCK_METHODS.has(method) ? method : undefined; } @@ -327,6 +415,11 @@ function mockCallMethod(callee: unknown): string | undefined { * instantiates the named module under test just as an `import` would, yet `parseImports` never * sees a call expression. Only a literal first argument counts — a quoted string or a template * without substitutions; `vi.mock(import('x'))` is already a dynamic `ImportExpression` edge. + * + * The receiver is the global `vi`/`vitest`, a name a top-level `import { vi as x } from 'vitest'` + * binds to either, or a member `vi`/`vitest` of a `import * as ns from 'vitest'` namespace. Only + * those import declarations are resolved: a value copied through a variable or a dynamic import + * is not followed. */ export function parseModuleMockSpecifiers(source: string): { spec: string; line: number }[] { const parsed = parseSync('layering-mocks.ts', source); @@ -335,9 +428,11 @@ export function parseModuleMockSpecifiers(source: string): { spec: string; line: `Cannot parse mocks: ${parsed.errors.map(({ message }) => message).join('; ')}`, ); } + const bindings = vitestImportBindings(parsed.program); const sites: { spec: string; line: number }[] = []; - visitAst(parsed.program, (node) => { - if (node.type !== 'CallExpression' || mockCallMethod(node.callee) === undefined) return; + visitAstScoped(parsed.program, new Set(), (node, shadowed) => { + if (node.type !== 'CallExpression') return; + if (mockCallMethod(node.callee, bindings, shadowed) === undefined) return; const args = node.arguments; const spec = Array.isArray(args) ? literalSpecifier(args[0]) : undefined; if (spec !== undefined) { diff --git a/scripts/layering/package-boundaries-specifier-sites.test.ts b/scripts/layering/package-boundaries-specifier-sites.test.ts index 1cc843bc7c..76ac1130d2 100644 --- a/scripts/layering/package-boundaries-specifier-sites.test.ts +++ b/scripts/layering/package-boundaries-specifier-sites.test.ts @@ -46,3 +46,36 @@ test('specifier sites skip comments, interpolated templates and unrelated object [], ); }); + +test('specifier sites carry mock-family arguments called through an imported Vitest alias or namespace', () => { + assert.deepEqual( + sites( + [ + "import { vi as mocks, vitest as runner } from 'vitest';", + "import * as testing from 'vitest';", + "mocks.mock('a');", + "runner['doMock']('b');", + "testing.vi.mock('c');", + "testing['vitest'].unmock('d');", + ].join('\n'), + ), + ['1:vitest', '2:vitest', '3:a', '4:b', '5:c', '6:d'], + ); +}); + +test('specifier sites skip alias calls on other modules, other exports and type-only imports', () => { + assert.deepEqual( + sites( + [ + "import { vi as fromOther } from 'not-vitest';", + "import { expect as notMock } from 'vitest';", + "import type { vi as typeOnly } from 'vitest';", + "fromOther.mock('a');", + "notMock.mock('b');", + "typeOnly.mock('c');", + "function shadow(notMock) { notMock.mock('d'); }", + ].join('\n'), + ), + ['1:not-vitest', '2:vitest', '3:vitest'], + ); +}); diff --git a/scripts/layering/plugin-framework-independence.test.ts b/scripts/layering/plugin-framework-independence.test.ts index e5a882d65d..a6fafa2fee 100644 --- a/scripts/layering/plugin-framework-independence.test.ts +++ b/scripts/layering/plugin-framework-independence.test.ts @@ -62,6 +62,10 @@ test('R85 rejects a static, type-only, dynamic, re-export, inline-type or mocked `await vi.importMock(${PROVIDER_ENTRY});`, `vi.mock(import(${PROVIDER_ENTRY}));`, `import plugin from '@agent-device/testmu';`, + `import { vi as mocks } from 'vitest';\nmocks.mock(${PROVIDER_ENTRY});`, + `import { vitest as mocks } from 'vitest';\nmocks.doMock(${PROVIDER_ENTRY});`, + `import * as testing from 'vitest';\ntesting.vi.mock(${PROVIDER_ENTRY});`, + `import * as testing from 'vitest';\ntesting['vitest']['importActual'](${PROVIDER_ENTRY});`, ]) { const [result, ...rest] = check(source); assert.equal(rest.length, 0, source); @@ -107,6 +111,11 @@ test('R85 ignores a provider named only in a comment, a non-literal mock argumen `// vi.mock(${PROVIDER_ENTRY});`, 'vi.mock(`@agent-device/provider-webdriver/${entry}`);', `other.mock(${PROVIDER_ENTRY});`, + `import { vi as mocks } from 'not-vitest';\nmocks.mock(${PROVIDER_ENTRY});`, + `import { expect as mocks } from 'vitest';\nmocks.mock(${PROVIDER_ENTRY});`, + `import type { vi as mocks } from 'vitest';\nmocks.mock(${PROVIDER_ENTRY});`, + `import * as testing from 'vitest';\ntesting.expect.mock(${PROVIDER_ENTRY});`, + `import { vi as mocks } from 'vitest';\nfunction f(mocks) { mocks.mock(${PROVIDER_ENTRY}); }`, ]) { assert.deepEqual(check(source), [], source); } From 5e132e9df10afda2d42f2c85f50ce053ba6fe7f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 17:30:29 +0200 Subject: [PATCH 06/13] chore(gates): drop the nonexistent mockActual from the module-mock method set Vitest exposes importActual, not mockActual. The unsupported name made R85 and every specifierSites walk flag a call no test can make. --- scripts/layering/model-mock-bindings.test.ts | 16 ++++++++++++++++ scripts/layering/model.ts | 1 - .../plugin-framework-independence.test.ts | 1 + 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/scripts/layering/model-mock-bindings.test.ts b/scripts/layering/model-mock-bindings.test.ts index 02182b2031..ab77ceca06 100644 --- a/scripts/layering/model-mock-bindings.test.ts +++ b/scripts/layering/model-mock-bindings.test.ts @@ -145,6 +145,22 @@ test('an alias call still ignores comments, non-literal modules and non-module m ); }); +test('only the module-path methods Vitest exposes count, through every receiver form', () => { + assert.deepEqual( + specs( + "import { vi as mocks } from 'vitest';", + "import * as testing from 'vitest';", + "mocks.mockActual('a');", + "testing.vi.mockActual('b');", + "vi.mockActual('c');", + "mocks.importActual('d');", + "testing.vi.importActual('e');", + "vi.importActual('f');", + ), + ['d', 'e', 'f'], + ); +}); + test('a Vitest binding made by a dynamic import is not followed', () => { assert.deepEqual( specs( diff --git a/scripts/layering/model.ts b/scripts/layering/model.ts index b0f219d144..e7ac7636c2 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -310,7 +310,6 @@ const MODULE_MOCK_RECEIVERS = new Set(['vi', 'vitest']); const MODULE_MOCK_METHODS = new Set([ 'mock', 'doMock', - 'mockActual', 'unmock', 'doUnmock', 'importActual', diff --git a/scripts/layering/plugin-framework-independence.test.ts b/scripts/layering/plugin-framework-independence.test.ts index a6fafa2fee..9ee6399105 100644 --- a/scripts/layering/plugin-framework-independence.test.ts +++ b/scripts/layering/plugin-framework-independence.test.ts @@ -116,6 +116,7 @@ test('R85 ignores a provider named only in a comment, a non-literal mock argumen `import type { vi as mocks } from 'vitest';\nmocks.mock(${PROVIDER_ENTRY});`, `import * as testing from 'vitest';\ntesting.expect.mock(${PROVIDER_ENTRY});`, `import { vi as mocks } from 'vitest';\nfunction f(mocks) { mocks.mock(${PROVIDER_ENTRY}); }`, + `import { vi as mocks } from 'vitest';\nmocks.mockActual(${PROVIDER_ENTRY});`, ]) { assert.deepEqual(check(source), [], source); } From 9b9f4355a091c21f25ca912521e8cc71ebebf04d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 18:14:15 +0200 Subject: [PATCH 07/13] chore(gates): drop shadow-scope resolution from the Vitest mock-site reader An alias a local redeclares now counts as the alias, so the reader can only over-report. Import-bound aliases and namespaces stay resolved, and no production file aliases vi, so the scope walk bought no coverage. --- scripts/layering/layering-ast.ts | 113 ------------------ scripts/layering/model-mock-bindings.test.ts | 24 +--- scripts/layering/model.ts | 30 ++--- ...package-boundaries-specifier-sites.test.ts | 1 - 4 files changed, 12 insertions(+), 156 deletions(-) diff --git a/scripts/layering/layering-ast.ts b/scripts/layering/layering-ast.ts index 970dcf16dc..808672c0c5 100644 --- a/scripts/layering/layering-ast.ts +++ b/scripts/layering/layering-ast.ts @@ -134,116 +134,3 @@ export function destructuredDynamicImportBindings( visit(program); return captures; } - -function patternNames(pattern: unknown, into: string[]): void { - const node = asNode(pattern); - if (node === undefined) return; - switch (node.type) { - case 'Identifier': - into.push(node.name as string); - break; - case 'ObjectPattern': - for (const property of nodeList(node.properties)) { - const entry = asNode(property); - patternNames(entry?.type === 'Property' ? entry.value : entry, into); - } - break; - case 'ArrayPattern': - for (const element of nodeList(node.elements)) patternNames(element, into); - break; - case 'AssignmentPattern': - patternNames(node.left, into); - break; - case 'RestElement': - patternNames(node.argument, into); - break; - case 'TSParameterProperty': - patternNames(node.parameter, into); - break; - } -} - -function lexicalNames(statements: unknown[], into: string[]): void { - for (const statement of statements) { - const node = asNode(statement); - if (node?.type === 'VariableDeclaration' && node.kind !== 'var') { - for (const declarator of nodeList(node.declarations)) { - patternNames(asNode(declarator)?.id, into); - } - } else if (node?.type === 'FunctionDeclaration' || node?.type === 'ClassDeclaration') { - patternNames(node.id, into); - } - } -} - -function isFunctionNode(node: AstNode): boolean { - return ( - node.type === 'FunctionDeclaration' || - node.type === 'FunctionExpression' || - node.type === 'ArrowFunctionExpression' - ); -} - -function hoistedVarNames(node: unknown, into: string[]): void { - for (const child of Array.isArray(node) ? node : [node]) { - const record = asNode(child); - if (record === undefined || isFunctionNode(record)) continue; - if (record.type === 'VariableDeclaration' && record.kind === 'var') { - for (const declarator of nodeList(record.declarations)) { - patternNames(asNode(declarator)?.id, into); - } - } - for (const value of Object.values(record)) hoistedVarNames(value, into); - } -} - -/** Names `node` itself declares for its own subtree: parameters, `var`s, block-level lexical declarations. */ -function scopeBindings(node: AstNode): string[] { - const names: string[] = []; - if (isFunctionNode(node)) { - for (const param of nodeList(node.params)) patternNames(param, names); - if (node.type === 'FunctionExpression') patternNames(node.id, names); - hoistedVarNames(node.body, names); - } else if ( - node.type === 'BlockStatement' || - node.type === 'StaticBlock' || - node.type === 'TSModuleBlock' - ) { - lexicalNames(nodeList(node.body), names); - } else if (node.type === 'ClassExpression') { - patternNames(node.id, names); - } else if (node.type === 'SwitchStatement') { - for (const switchCase of nodeList(node.cases)) { - lexicalNames(nodeList(asNode(switchCase)?.consequent), names); - } - } else if (node.type === 'ForStatement') { - lexicalNames([node.init], names); - } else if (node.type === 'ForInStatement' || node.type === 'ForOfStatement') { - lexicalNames([node.left], names); - } else if (node.type === 'CatchClause') { - patternNames(node.param, names); - } - return names; -} - -/** - * `visitAst` that also hands the visitor the names declared by the enclosing scopes: function - * parameters and `var`s, block-level `let`/`const`/`function`/`class`, loop heads and `catch` - * parameters. A `with` statement or `eval` can rebind names invisibly and is not modelled. - */ -export function visitAstScoped( - node: unknown, - shadowed: ReadonlySet, - visitor: (node: AstNode, shadowed: ReadonlySet) => void, -): void { - if (Array.isArray(node)) { - for (const child of node) visitAstScoped(child, shadowed, visitor); - return; - } - const record = asNode(node); - if (record === undefined) return; - const declared = scopeBindings(record); - const scope = declared.length > 0 ? new Set([...shadowed, ...declared]) : shadowed; - visitor(record, scope); - for (const child of Object.values(record)) visitAstScoped(child, scope, visitor); -} diff --git a/scripts/layering/model-mock-bindings.test.ts b/scripts/layering/model-mock-bindings.test.ts index ab77ceca06..4b6b770020 100644 --- a/scripts/layering/model-mock-bindings.test.ts +++ b/scripts/layering/model-mock-bindings.test.ts @@ -83,32 +83,14 @@ test('a vitest namespace member other than vi or vitest is not the mock API', () ); }); -test('an alias is not a mock receiver where an enclosing scope redeclares its name', () => { +test('an alias a local redeclares still counts, so the walk only over-reports', () => { assert.deepEqual( specs( "import { vi as mocks } from 'vitest';", - "import * as testing from 'vitest';", "function parameter(mocks) { mocks.mock('a'); }", - "const arrow = (testing) => testing.vi.mock('b');", - "{ const mocks = other; mocks.mock('c'); }", - "function hoisted() { if (flag) { var mocks = other; } mocks.mock('d'); }", - "try {} catch (mocks) { mocks.mock('e'); }", - "for (const { mocks } of list) mocks.mock('f');", - "switch (kind) { case 1: class testing {} testing.vi.mock('g'); }", - "const expression = function mocks() { mocks.mock('h'); };", - "const named = class mocks { run() { mocks.mock('i'); } };", - "namespace Fixture { const testing = other; testing.vi.mock('j'); }", - ), - [], - ); - assert.deepEqual( - specs( - "import { vi as mocks } from 'vitest';", - 'function unrelated(mocks) { return mocks; }', - '{ const mocks = other; }', - "mocks.mock('outside');", + "mocks.mock('b');", ), - ['outside'], + ['a', 'b'], ); }); diff --git a/scripts/layering/model.ts b/scripts/layering/model.ts index e7ac7636c2..75d6ba2b3e 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -10,7 +10,6 @@ import { nodeList, propertyName, visitAst, - visitAstScoped, type AstNode, } from './layering-ast.ts'; import { declaredRootModuleZone } from './root-module-zones.ts'; @@ -370,19 +369,12 @@ function vitestImportBindings(program: unknown): VitestBindings { return { apis, namespaces }; } -/** - * The global names `vi` and `vitest` always count. An imported alias or namespace counts only - * while no enclosing scope redeclares its local name. - */ -function isMockApiReceiver( - node: unknown, - bindings: VitestBindings, - shadowed: ReadonlySet, -): boolean { +/** The global names `vi` and `vitest`, and the local names a `vitest` import binds to them. */ +function isMockApiReceiver(node: unknown, bindings: VitestBindings): boolean { const receiver = unwrapExpression(node); if (receiver?.type === 'Identifier') { const name = receiver.name as string; - return MODULE_MOCK_RECEIVERS.has(name) || (bindings.apis.has(name) && !shadowed.has(name)); + return MODULE_MOCK_RECEIVERS.has(name) || bindings.apis.has(name); } if (receiver?.type !== 'MemberExpression') return false; const owner = unwrapExpression(receiver.object); @@ -390,20 +382,15 @@ function isMockApiReceiver( return ( owner?.type === 'Identifier' && bindings.namespaces.has(owner.name as string) && - !shadowed.has(owner.name as string) && key !== undefined && MODULE_MOCK_RECEIVERS.has(key) ); } -function mockCallMethod( - callee: unknown, - bindings: VitestBindings, - shadowed: ReadonlySet, -): string | undefined { +function mockCallMethod(callee: unknown, bindings: VitestBindings): string | undefined { const member = unwrapExpression(callee); if (member?.type !== 'MemberExpression') return undefined; - if (!isMockApiReceiver(member.object, bindings, shadowed)) return undefined; + if (!isMockApiReceiver(member.object, bindings)) return undefined; const method = memberKey(member); return method !== undefined && MODULE_MOCK_METHODS.has(method) ? method : undefined; } @@ -418,7 +405,8 @@ function mockCallMethod( * The receiver is the global `vi`/`vitest`, a name a top-level `import { vi as x } from 'vitest'` * binds to either, or a member `vi`/`vitest` of a `import * as ns from 'vitest'` namespace. Only * those import declarations are resolved: a value copied through a variable or a dynamic import - * is not followed. + * is not followed, and a local that redeclares an alias's name still counts as the alias, so the + * walk can only over-report. */ export function parseModuleMockSpecifiers(source: string): { spec: string; line: number }[] { const parsed = parseSync('layering-mocks.ts', source); @@ -429,9 +417,9 @@ export function parseModuleMockSpecifiers(source: string): { spec: string; line: } const bindings = vitestImportBindings(parsed.program); const sites: { spec: string; line: number }[] = []; - visitAstScoped(parsed.program, new Set(), (node, shadowed) => { + visitAst(parsed.program, (node) => { if (node.type !== 'CallExpression') return; - if (mockCallMethod(node.callee, bindings, shadowed) === undefined) return; + if (mockCallMethod(node.callee, bindings) === undefined) return; const args = node.arguments; const spec = Array.isArray(args) ? literalSpecifier(args[0]) : undefined; if (spec !== undefined) { diff --git a/scripts/layering/package-boundaries-specifier-sites.test.ts b/scripts/layering/package-boundaries-specifier-sites.test.ts index 76ac1130d2..7d5f7abd7b 100644 --- a/scripts/layering/package-boundaries-specifier-sites.test.ts +++ b/scripts/layering/package-boundaries-specifier-sites.test.ts @@ -73,7 +73,6 @@ test('specifier sites skip alias calls on other modules, other exports and type- "fromOther.mock('a');", "notMock.mock('b');", "typeOnly.mock('c');", - "function shadow(notMock) { notMock.mock('d'); }", ].join('\n'), ), ['1:not-vitest', '2:vitest', '3:vitest'], From e8ee9b122bf542a7a0f8725a3439a9dbef1de538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 18:14:16 +0200 Subject: [PATCH 08/13] chore(gates): share one package-isolation walk between test-kit and the plugin framework R83 test-kit isolation and R85 plugin-framework independence each parsed manifests and swept imports. Manifest parsing, the forbidden-name set, the dependency-reach walk and the import sweep now live in package-isolation.ts behind a per-package policy table. R85 gains the transitive manifest reach it lacked: plugins -> host-kit -> a provider package passed the direct-dependency check. --- scripts/layering/check.ts | 5 +- scripts/layering/package-isolation.test.ts | 321 ++++++++++++++++++ scripts/layering/package-isolation.ts | 191 +++++++++++ .../plugin-framework-independence.test.ts | 123 ------- .../layering/plugin-framework-independence.ts | 90 ----- scripts/layering/test-kit-isolation.test.ts | 130 +------ scripts/layering/test-kit-isolation.ts | 136 +------- 7 files changed, 538 insertions(+), 458 deletions(-) create mode 100644 scripts/layering/package-isolation.test.ts create mode 100644 scripts/layering/package-isolation.ts delete mode 100644 scripts/layering/plugin-framework-independence.test.ts delete mode 100644 scripts/layering/plugin-framework-independence.ts diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index bcfb505a51..5e6db4ee8b 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -161,7 +161,7 @@ import { sourceExecutionCompatibilityViolations } from './source-execution-polic import { sessionResourceOwnershipViolations } from './session-resource-ownership.ts'; import { applicationLifecycleOwnershipViolations } from './application-lifecycle-policy.ts'; import { iosSnapshotEngineOwnershipViolations } from './ios-snapshot-engine-policy.ts'; -import { checkPluginFrameworkIndependence } from './plugin-framework-independence.ts'; +import { checkPackageIsolation, PLUGIN_FRAMEWORK_ISOLATION } from './package-isolation.ts'; import { providerSnapshotPresentationViolations } from './provider-snapshot-presentation-policy.ts'; import { snapshotAssemblyPresentationViolations } from './snapshot-assembly-presentation-policy.ts'; import { RETIRED_PATH_RULES, retiredPathRuleViolations } from './retired-paths-policy.ts'; @@ -623,9 +623,10 @@ export const LAYERING_RULES: Readonly> = { ...checkTestKitIsolation(readTrackedPackageManifests(repoRoot), context.allTypeScriptSources), ], 'plugin-framework-independence': (context) => - checkPluginFrameworkIndependence( + checkPackageIsolation( readTrackedPackageManifests(repoRoot), context.allTypeScriptSources, + PLUGIN_FRAMEWORK_ISOLATION, ), 'test-colocation': (context) => checkTestColocation(context.ratchets.unmatchedTests, context.reference.unmatchedTests), diff --git a/scripts/layering/package-isolation.test.ts b/scripts/layering/package-isolation.test.ts new file mode 100644 index 0000000000..1a68f8c5e0 --- /dev/null +++ b/scripts/layering/package-isolation.test.ts @@ -0,0 +1,321 @@ +// The shared isolation walk (R83 test-kit, R85 plugin framework): forbidden packages reached +// directly, through other workspace packages or by import, relative escapes, and reference cycles — +// run against both policies, then each policy's own forbidden class. + +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { test } from 'node:test'; +import { + checkPackageIsolation, + PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, + PLUGIN_FRAMEWORK_ISOLATION, + TEST_KIT_ISOLATION, + type PackageIsolationPolicy, +} from './package-isolation.ts'; +import { readTrackedPackageManifests } from './package-boundaries.ts'; +import { listTrackedTypeScriptFiles } from './tracked-sources.ts'; + +type Manifest = Record; + +const repoRoot = path.resolve(import.meta.dirname, '../..'); +const POLICIES = [TEST_KIT_ISOLATION, PLUGIN_FRAMEWORK_ISOLATION]; +const DEPENDENCY_FIELDS = [ + 'dependencies', + 'devDependencies', + 'optionalDependencies', + 'peerDependencies', +]; + +/** A workspace of manifests keyed by package directory name; both isolated packages build on kernel. */ +function workspace(overrides: Record = {}): Map { + const base: Record = { + 'test-kit': { + name: '@agent-device/test-kit', + dependencies: { '@agent-device/kernel': 'workspace:*' }, + }, + plugins: { + name: '@agent-device/plugins', + dependencies: { '@agent-device/kernel': 'workspace:*' }, + }, + kernel: { name: '@agent-device/kernel' }, + contracts: { name: '@agent-device/contracts' }, + 'host-kit': { name: '@agent-device/host-kit' }, + 'platform-android': { name: '@agent-device/platform-android' }, + 'platform-apple': { name: '@agent-device/platform-apple' }, + 'provider-webdriver': { name: '@agent-device/provider-webdriver' }, + 'provider-testmu': { name: '@agent-device/testmu' }, + daemon: { name: '@agent-device/daemon' }, + proxy: { name: '@agent-device/proxy' }, + }; + const merged: Record = { ...base }; + for (const [dir, manifest] of Object.entries(overrides)) { + merged[dir] = { ...base[dir], ...manifest }; + } + return new Map( + Object.entries(merged).map(([dir, manifest]) => [ + `packages/${dir}/package.json`, + JSON.stringify(manifest), + ]), + ); +} + +function dirOf(policy: PackageIsolationPolicy): string { + return policy.dir.replace('packages/', ''); +} + +function check( + policy: PackageIsolationPolicy, + overrides: Record = {}, + sources: Record = {}, +) { + return checkPackageIsolation(workspace(overrides), new Map(Object.entries(sources)), policy); +} + +function source(policy: PackageIsolationPolicy, text: string) { + return { [`${policy.dir}/src/a.ts`]: text }; +} + +test('the real repository holds every isolation policy', () => { + const sources = new Map( + listTrackedTypeScriptFiles(repoRoot).map((file) => [ + file, + fs.readFileSync(path.join(repoRoot, file), 'utf8'), + ]), + ); + const manifests = readTrackedPackageManifests(repoRoot); + for (const policy of POLICIES) { + assert.deepEqual(checkPackageIsolation(manifests, sources, policy), [], policy.name); + } +}); + +test('a repo without the isolated package has nothing to isolate', () => { + for (const policy of POLICIES) { + assert.deepEqual(checkPackageIsolation(new Map(), new Map(), policy), [], policy.name); + } +}); + +test('an isolated package built only on foundations holds', () => { + for (const policy of POLICIES) { + const dir = dirOf(policy); + assert.deepEqual( + check( + policy, + { + [dir]: { + dependencies: { + '@agent-device/kernel': 'workspace:*', + '@agent-device/contracts': 'workspace:*', + '@agent-device/host-kit': 'workspace:*', + }, + }, + }, + source(policy, "import { AppError } from '@agent-device/kernel/errors';"), + ), + [], + policy.name, + ); + } +}); + +test('a provider package is refused in every dependency field, under a non-provider name too', () => { + for (const policy of POLICIES) { + for (const name of ['@agent-device/provider-webdriver', '@agent-device/testmu']) { + for (const field of DEPENDENCY_FIELDS) { + const found = check(policy, { [dirOf(policy)]: { [field]: { [name]: 'workspace:*' } } }); + assert.equal(found.length, 1, `${policy.name} ${name} in ${field}`); + assert.equal(found[0]!.file, `${policy.dir}/package.json`); + assert.equal(found[0]!.rule, policy.rule); + assert.match(found[0]!.message, new RegExp(`${policy.name} -> ${name}`)); + } + } + } +}); + +test('a provider reached through other workspace packages is refused with its chain', () => { + for (const policy of POLICIES) { + for (const field of DEPENDENCY_FIELDS) { + const found = check(policy, { + [dirOf(policy)]: { + dependencies: { '@agent-device/host-kit': 'workspace:*' }, + }, + 'host-kit': { dependencies: { '@agent-device/contracts': 'workspace:*' } }, + contracts: { [field]: { '@agent-device/provider-webdriver': 'workspace:*' } }, + }); + assert.equal(found.length, 1, `${policy.name} via ${field}`); + assert.match( + found[0]!.message, + new RegExp( + `${policy.name} -> @agent-device/host-kit -> @agent-device/contracts -> ` + + `@agent-device/provider-webdriver`, + ), + ); + } + } +}); + +test('a package outside the isolated package reach may declare a forbidden package', () => { + for (const policy of POLICIES) { + assert.deepEqual( + check(policy, { + proxy: { dependencies: { '@agent-device/provider-webdriver': 'workspace:*' } }, + }), + [], + policy.name, + ); + } +}); + +test('a package the isolated package reaches cannot declare it back, directly or transitively', () => { + for (const policy of POLICIES) { + const direct = check(policy, { + kernel: { devDependencies: { [policy.name]: 'workspace:*' } }, + }); + assert.equal(direct.length, 1, policy.name); + assert.equal(direct[0]!.file, 'packages/kernel/package.json'); + assert.match(direct[0]!.message, /project-reference cycle/); + + const transitive = check(policy, { + kernel: { dependencies: { '@agent-device/contracts': 'workspace:*' } }, + contracts: { optionalDependencies: { [policy.name]: 'workspace:*' } }, + }); + assert.equal(transitive.length, 1, policy.name); + assert.equal(transitive[0]!.file, 'packages/contracts/package.json'); + assert.match( + transitive[0]!.message, + new RegExp( + `${policy.name} -> @agent-device/kernel -> @agent-device/contracts -> ${policy.name}`, + ), + ); + + assert.deepEqual( + check(policy, { proxy: { devDependencies: { [policy.name]: 'workspace:*' } } }), + [], + policy.name, + ); + } +}); + +test('a diamond and a shared foundation are not a cycle or a false ban', () => { + for (const policy of POLICIES) { + assert.deepEqual( + check(policy, { + [dirOf(policy)]: { + dependencies: { + '@agent-device/host-kit': 'workspace:*', + '@agent-device/contracts': 'workspace:*', + }, + }, + 'host-kit': { dependencies: { '@agent-device/kernel': 'workspace:*' } }, + contracts: { dependencies: { '@agent-device/kernel': 'workspace:*' } }, + }), + [], + policy.name, + ); + } +}); + +test('a static, type-only, dynamic, re-export, inline-type or mocked import of a provider is refused', () => { + for (const policy of POLICIES) { + const entry = "'@agent-device/provider-webdriver/plugin'"; + for (const text of [ + `import { createCloudWebDriverRuntime } from ${entry};`, + `import type { CloudWebDriverProviderOptions } from ${entry};`, + `const { createCloudWebDriverRuntime } = await import(${entry});`, + `export { createCloudWebDriverRuntime } from ${entry};`, + `type Options = import(${entry}).CloudWebDriverProviderOptions;`, + `import '@agent-device/testmu';`, + `vi.mock(${entry}, () => ({}));`, + `vi.mock(${entry.replaceAll("'", '`')}, () => ({}));`, + `vi['mock'](${entry});`, + `vi.doMock(${entry});`, + `await vi.importActual(${entry});`, + `vi.mock(import(${entry}));`, + `import { vi as mocks } from 'vitest';\nmocks.mock(${entry});`, + `import * as testing from 'vitest';\ntesting.vi.mock(${entry});`, + ]) { + const found = check(policy, {}, source(policy, text)); + assert.equal(found.length, 1, `${policy.name}: ${text}`); + assert.equal(found[0]!.rule, policy.rule); + assert.equal(found[0]!.file, `${policy.dir}/src/a.ts`); + } + } +}); + +test('an import ignores comments, non-literal mock arguments, other objects and its own directory', () => { + for (const policy of POLICIES) { + const entry = "'@agent-device/provider-webdriver/plugin'"; + for (const text of [ + `// vi.mock(${entry});`, + 'vi.mock(`@agent-device/provider-webdriver/${name}`);', + `other.mock(${entry});`, + `import { vi as mocks } from 'not-vitest';\nmocks.mock(${entry});`, + `import type { vi as mocks } from 'vitest';\nmocks.mock(${entry});`, + "import { b } from './b.ts';\nimport { c } from '@agent-device/daemon-contracts/device-claims';", + ]) { + assert.deepEqual(check(policy, {}, source(policy, text)), [], `${policy.name}: ${text}`); + } + } +}); + +test('relative reach out of the package directory is refused', () => { + for (const policy of POLICIES) { + for (const specifier of [ + '../../../src/platform-runtime-gateway.ts', + '../../kernel/src/errors.ts', + ]) { + const found = check(policy, {}, source(policy, `import { x } from '${specifier}';`)); + assert.equal(found.length, 1, `${policy.name}: ${specifier}`); + } + } +}); + +test('test-kit also refuses platform, daemon and root packages that the plugin framework may use', () => { + const forbidden = [ + '@agent-device/platform-apple', + '@agent-device/platform-android/runner', + '@agent-device/daemon', + 'agent-device', + 'agent-device/commands', + ]; + for (const name of forbidden) { + const dependency = name + .split('/') + .slice(0, name.startsWith('@') ? 2 : 1) + .join('/'); + for (const field of DEPENDENCY_FIELDS) { + const found = check(TEST_KIT_ISOLATION, { 'test-kit': { [field]: { [dependency]: '*' } } }); + assert.equal(found.length, 1, `${dependency} in ${field}`); + assert.match(found[0]!.message, /belongs in it/); + } + assert.equal( + check(TEST_KIT_ISOLATION, {}, source(TEST_KIT_ISOLATION, `import { x } from '${name}';`)) + .length, + 1, + name, + ); + assert.deepEqual( + check( + PLUGIN_FRAMEWORK_ISOLATION, + {}, + source(PLUGIN_FRAMEWORK_ISOLATION, `import { x } from '${name}';`), + ), + [], + name, + ); + } + assert.deepEqual( + check(PLUGIN_FRAMEWORK_ISOLATION, { + plugins: { dependencies: { '@agent-device/platform-apple': 'workspace:*' } }, + }), + [], + ); +}); + +test('the plugin framework carries R85 and its provider-policy remedy', () => { + const [found] = check(PLUGIN_FRAMEWORK_ISOLATION, { + plugins: { dependencies: { '@agent-device/provider-webdriver': 'workspace:*' } }, + }); + assert.equal(found?.rule, PLUGIN_FRAMEWORK_INDEPENDENCE_RULE); + assert.match(found!.message, /policy from the composing root/); +}); diff --git a/scripts/layering/package-isolation.ts b/scripts/layering/package-isolation.ts new file mode 100644 index 0000000000..d15d631d6e --- /dev/null +++ b/scripts/layering/package-isolation.ts @@ -0,0 +1,191 @@ +// Catches: an isolated workspace package acquiring a dependency on the code it must stay below — +// as a manifest entry (directly, or through any chain of workspace packages), as an import +// (static, type-only, dynamic or `vi.mock` literal), or by reaching out of its directory — and a +// package it reaches declaring it back, which TypeScript project references reject as a cycle. +// `@agent-device/test-kit` may not build on platform, provider, daemon or root code; the plugin +// framework `@agent-device/plugins` may not build on a concrete provider package. +// Evidence: the single-entry test-kit loads every helper eagerly, so one platform import would put +// that platform's closure into every test's import graph; #3712 extracted the plugin loader with +// `provider-webdriver` still imported by its manifest and loader, and a chain through another +// workspace package would have reached the same provider unseen by a direct-dependency check. +// Cost: one pass over the tracked manifests and the isolated packages' sources. +// Kill criterion: a maintainer decision that an isolated package may depend on the code its policy +// forbids, or that provider-* packages stop being the unit of provider implementation. +// +// The forbidden set is derived from the workspace layout, not from name patterns: every package +// whose directory matches the policy, whatever name it publishes (e.g. `@agent-device/testmu` under +// `packages/provider-testmu`). A new isolated package is one more row in ISOLATION_POLICIES. + +import path from 'node:path'; +import type { LayeringViolation } from './model.ts'; +import { specifierSites } from './package-boundaries.ts'; +import { PACKAGE_EXPORT_SURFACE_RULE, TEST_KIT_PACKAGE } from './package-export-surface.ts'; + +export const PLUGIN_FRAMEWORK_INDEPENDENCE_RULE = 'R85 plugin-framework-independence'; + +export type PackageIsolationPolicy = { + rule: string; + /** Repo-relative package directory, e.g. `packages/test-kit`. */ + dir: string; + /** The name the package publishes. */ + name: string; + /** Manifest paths of the workspace packages this package may not reach. */ + forbiddenManifests: RegExp; + /** The root `agent-device` package is forbidden too. */ + forbidsRoot: boolean; + /** What is forbidden and where that code's knowledge belongs, for diagnostics. */ + forbids: string; + remedy: string; +}; + +export const TEST_KIT_ISOLATION: PackageIsolationPolicy = { + rule: PACKAGE_EXPORT_SURFACE_RULE, + dir: 'packages/test-kit', + name: TEST_KIT_PACKAGE, + forbiddenManifests: /^packages\/(?:platform-[^/]+|provider-[^/]+|daemon)\/package\.json$/, + forbidsRoot: true, + forbids: 'a platform, provider, daemon or root package', + remedy: 'A fixture that needs that code is a test of that package and belongs in it.', +}; + +export const PLUGIN_FRAMEWORK_ISOLATION: PackageIsolationPolicy = { + rule: PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, + dir: 'packages/plugins', + name: '@agent-device/plugins', + forbiddenManifests: /^packages\/provider-[^/]+\/package\.json$/, + forbidsRoot: false, + forbids: 'a concrete provider package', + remedy: 'Provider knowledge reaches the loader as a policy from the composing root.', +}; + +type DependencyManifest = { + name?: string; + exports?: unknown; + dependencies?: Record; + devDependencies?: Record; + optionalDependencies?: Record; + peerDependencies?: Record; +}; + +export function parseManifests( + manifests: ReadonlyMap, +): Map { + return new Map( + [...manifests].map(([file, text]) => [file, JSON.parse(text) as DependencyManifest]), + ); +} + +/** Every package name a manifest declares in any dependency field. */ +function declaredNames(manifest: DependencyManifest): string[] { + return [ + ...Object.keys(manifest.dependencies ?? {}), + ...Object.keys(manifest.devDependencies ?? {}), + ...Object.keys(manifest.optionalDependencies ?? {}), + ...Object.keys(manifest.peerDependencies ?? {}), + ]; +} + +const ROOT_PACKAGE = 'agent-device'; + +function forbiddenNames( + parsed: ReadonlyMap, + policy: PackageIsolationPolicy, +): Set { + const names = new Set(policy.forbidsRoot ? [ROOT_PACKAGE] : []); + for (const [file, manifest] of parsed) { + if (policy.forbiddenManifests.test(file) && manifest.name) names.add(manifest.name); + } + return names; +} + +function packageName(specifier: string): string { + const parts = specifier.split('/'); + return specifier.startsWith('@') ? parts.slice(0, 2).join('/') : parts[0]!; +} + +/** + * Manifest reach from the policy's package, breadth-first over every dependency field of the + * workspace graph. A forbidden package ends a branch with the chain that reached it; a package + * that declares the policy's package back closes a project-reference cycle. + */ +function manifestViolations( + parsed: ReadonlyMap, + policy: PackageIsolationPolicy, + forbidden: ReadonlySet, +): LayeringViolation[] { + const violations: LayeringViolation[] = []; + const manifestFile = `${policy.dir}/package.json`; + const fileByName = new Map(); + for (const [file, manifest] of parsed) if (manifest.name) fileByName.set(manifest.name, file); + + const via = new Map([[policy.name, [policy.name]]]); + const queue = [policy.name]; + while (queue.length > 0) { + const current = queue.shift()!; + const manifest = parsed.get(fileByName.get(current) ?? ''); + if (!manifest) continue; + for (const name of declaredNames(manifest)) { + const chain = [...via.get(current)!, name]; + if (name === policy.name && current !== policy.name) { + violations.push({ + rule: policy.rule, + file: fileByName.get(current)!, + line: 1, + message: + `${chain.join(' -> ')} closes a project-reference cycle: '${current}' is reached ` + + `from ${policy.name} and cannot declare it back.`, + }); + continue; + } + if (forbidden.has(name)) { + violations.push({ + rule: policy.rule, + file: manifestFile, + line: 1, + message: `${chain.join(' -> ')}: ${policy.name} builds on ${policy.forbids}. ${policy.remedy}`, + }); + continue; + } + if (!fileByName.has(name) || via.has(name)) continue; + via.set(name, chain); + queue.push(name); + } + } + return violations; +} + +/** + * One isolated package's boundary: no forbidden package reached through the manifest graph (so a + * chain through host-kit or contracts counts), no import of one or relative reach out of its + * directory (the shared import walk, so `vi.mock` literals count), and no package it reaches + * declaring it back. + */ +export function checkPackageIsolation( + manifests: ReadonlyMap, + sources: ReadonlyMap, + policy: PackageIsolationPolicy, +): LayeringViolation[] { + const parsed = parseManifests(manifests); + if (!parsed.has(`${policy.dir}/package.json`)) return []; + const forbidden = forbiddenNames(parsed, policy); + const violations = manifestViolations(parsed, policy, forbidden); + + for (const [file, source] of sources) { + if (!file.startsWith(`${policy.dir}/`)) continue; + for (const site of specifierSites(file, source)) { + const escapes = + site.specifier.startsWith('.') && + !path.posix.join(path.posix.dirname(file), site.specifier).startsWith(`${policy.dir}/`); + if (!escapes && !forbidden.has(packageName(site.specifier))) continue; + violations.push({ + rule: policy.rule, + file, + line: site.line, + message: + `${policy.name} imports '${site.specifier}'. It may not build on ${policy.forbids} ` + + `or reach out of ${policy.dir}/. ${policy.remedy}`, + }); + } + } + return violations; +} diff --git a/scripts/layering/plugin-framework-independence.test.ts b/scripts/layering/plugin-framework-independence.test.ts deleted file mode 100644 index 9ee6399105..0000000000 --- a/scripts/layering/plugin-framework-independence.test.ts +++ /dev/null @@ -1,123 +0,0 @@ -// R85: the plugin framework imports and declares no concrete provider package. - -import assert from 'node:assert/strict'; -import fs from 'node:fs'; -import path from 'node:path'; -import { test } from 'node:test'; -import { - checkPluginFrameworkIndependence, - PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, -} from './plugin-framework-independence.ts'; -import { readTrackedPackageManifests } from './package-boundaries.ts'; -import { listTrackedTypeScriptFiles } from './tracked-sources.ts'; - -const repoRoot = path.resolve(import.meta.dirname, '../..'); -const PROVIDER_ENTRY = "'@agent-device/provider-webdriver/plugin'"; - -function manifests(plugins: Record = {}): Map { - return new Map([ - [ - 'packages/plugins/package.json', - JSON.stringify({ name: '@agent-device/plugins', ...plugins }), - ], - [ - 'packages/provider-webdriver/package.json', - JSON.stringify({ name: '@agent-device/provider-webdriver' }), - ], - ['packages/provider-testmu/package.json', JSON.stringify({ name: '@agent-device/testmu' })], - ['packages/kernel/package.json', JSON.stringify({ name: '@agent-device/kernel' })], - ]); -} - -function check(source: string, file = 'packages/plugins/src/load.ts') { - return checkPluginFrameworkIndependence(manifests(), new Map([[file, source]])); -} - -test('the real plugin framework declares and imports no provider package', () => { - const sources = new Map( - listTrackedTypeScriptFiles(repoRoot).map((file) => [ - file, - fs.readFileSync(path.join(repoRoot, file), 'utf8'), - ]), - ); - assert.deepEqual( - checkPluginFrameworkIndependence(readTrackedPackageManifests(repoRoot), sources), - [], - ); -}); - -test('R85 rejects a static, type-only, dynamic, re-export, inline-type or mocked provider import', () => { - for (const source of [ - `import { createCloudWebDriverRuntime } from ${PROVIDER_ENTRY};`, - `import type { CloudWebDriverProviderOptions } from ${PROVIDER_ENTRY};`, - `const { createCloudWebDriverRuntime } = await import(${PROVIDER_ENTRY});`, - `export { createCloudWebDriverRuntime } from ${PROVIDER_ENTRY};`, - `type Options = import(${PROVIDER_ENTRY}).CloudWebDriverProviderOptions;`, - `vi.mock(${PROVIDER_ENTRY}, () => ({}));`, - `vi.mock(${PROVIDER_ENTRY.replaceAll("'", '`')}, () => ({}));`, - `vi['mock'](${PROVIDER_ENTRY}, () => ({}));`, - `vi.doMock(${PROVIDER_ENTRY});`, - `vi.unmock(${PROVIDER_ENTRY});`, - `await vi.importActual(${PROVIDER_ENTRY});`, - `await vi.importMock(${PROVIDER_ENTRY});`, - `vi.mock(import(${PROVIDER_ENTRY}));`, - `import plugin from '@agent-device/testmu';`, - `import { vi as mocks } from 'vitest';\nmocks.mock(${PROVIDER_ENTRY});`, - `import { vitest as mocks } from 'vitest';\nmocks.doMock(${PROVIDER_ENTRY});`, - `import * as testing from 'vitest';\ntesting.vi.mock(${PROVIDER_ENTRY});`, - `import * as testing from 'vitest';\ntesting['vitest']['importActual'](${PROVIDER_ENTRY});`, - ]) { - const [result, ...rest] = check(source); - assert.equal(rest.length, 0, source); - assert.equal(result?.rule, PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, source); - assert.equal(result?.file, 'packages/plugins/src/load.ts', source); - } -}); - -test('R85 rejects a provider package listed in any dependency field of the plugins manifest', () => { - for (const field of [ - 'dependencies', - 'devDependencies', - 'optionalDependencies', - 'peerDependencies', - ]) { - const result = checkPluginFrameworkIndependence( - manifests({ [field]: { '@agent-device/provider-webdriver': 'workspace:*' } }), - new Map(), - ); - assert.deepEqual( - result.map(({ file }) => file), - ['packages/plugins/package.json'], - field, - ); - } -}); - -test('R85 admits the foundations the framework builds on and imports elsewhere in the repo', () => { - assert.deepEqual(check(`import { AppError } from '@agent-device/kernel/errors';`), []); - assert.deepEqual( - checkPluginFrameworkIndependence( - manifests({ dependencies: { '@agent-device/kernel': 'workspace:*' } }), - new Map([ - ['src/provider-plugin-policy.ts', `import('@agent-device/provider-webdriver/plugin');`], - ]), - ), - [], - ); -}); - -test('R85 ignores a provider named only in a comment, a non-literal mock argument or another object', () => { - for (const source of [ - `// vi.mock(${PROVIDER_ENTRY});`, - 'vi.mock(`@agent-device/provider-webdriver/${entry}`);', - `other.mock(${PROVIDER_ENTRY});`, - `import { vi as mocks } from 'not-vitest';\nmocks.mock(${PROVIDER_ENTRY});`, - `import { expect as mocks } from 'vitest';\nmocks.mock(${PROVIDER_ENTRY});`, - `import type { vi as mocks } from 'vitest';\nmocks.mock(${PROVIDER_ENTRY});`, - `import * as testing from 'vitest';\ntesting.expect.mock(${PROVIDER_ENTRY});`, - `import { vi as mocks } from 'vitest';\nfunction f(mocks) { mocks.mock(${PROVIDER_ENTRY}); }`, - `import { vi as mocks } from 'vitest';\nmocks.mockActual(${PROVIDER_ENTRY});`, - ]) { - assert.deepEqual(check(source), [], source); - } -}); diff --git a/scripts/layering/plugin-framework-independence.ts b/scripts/layering/plugin-framework-independence.ts deleted file mode 100644 index 06520aa14b..0000000000 --- a/scripts/layering/plugin-framework-independence.ts +++ /dev/null @@ -1,90 +0,0 @@ -// Catches: `@agent-device/plugins` — the manifest, store, host and loader every provider plugin -// runs under — acquiring a dependency on a concrete provider package, whether as a manifest entry -// or as a static, type-only, dynamic or `vi.mock` import. Bundled-provider knowledge (which ids -// plugins may not claim, how a `{ webDriver }` factory result becomes a runtime) belongs to the -// composition that selects providers, which hands it to the loader as a policy. -// Evidence: #3712 extracted the loader with `provider-webdriver` still imported by its manifest and -// loader; the provider-independence follow-up moved that knowledge to src/provider-plugin-policy.ts. -// Cost: one pass over the tracked manifests and the plugins package's sources. -// Kill criterion: a maintainer decision that the plugin framework may depend on a concrete -// provider, or that provider-* packages stop being the unit of provider implementation. -// -// The forbidden set is derived from the workspace layout, not from name patterns: every package -// under `packages/provider-*`, whatever name it publishes (e.g. `@agent-device/testmu`). - -import type { LayeringViolation } from './model.ts'; -import { specifierSites } from './package-boundaries.ts'; - -export const PLUGIN_FRAMEWORK_INDEPENDENCE_RULE = 'R85 plugin-framework-independence'; - -const PLUGINS_DIR = 'packages/plugins'; -const PROVIDER_MANIFEST = /^packages\/provider-[^/]+\/package\.json$/; - -type DependencyManifest = { - name?: string; - dependencies?: Record; - devDependencies?: Record; - optionalDependencies?: Record; - peerDependencies?: Record; -}; - -function declaredNames(manifest: DependencyManifest): string[] { - return [ - ...Object.keys(manifest.dependencies ?? {}), - ...Object.keys(manifest.devDependencies ?? {}), - ...Object.keys(manifest.optionalDependencies ?? {}), - ...Object.keys(manifest.peerDependencies ?? {}), - ]; -} - -function packageName(specifier: string): string { - const parts = specifier.split('/'); - return specifier.startsWith('@') ? parts.slice(0, 2).join('/') : parts[0]!; -} - -function violation(file: string, line: number, message: string): LayeringViolation { - return { rule: PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, file, line, message }; -} - -/** The plugin framework package declares and imports no `packages/provider-*` package. */ -export function checkPluginFrameworkIndependence( - manifests: ReadonlyMap, - sources: ReadonlyMap, -): LayeringViolation[] { - const providers = new Set(); - for (const [file, text] of manifests) { - const { name } = JSON.parse(text) as DependencyManifest; - if (PROVIDER_MANIFEST.test(file) && name) providers.add(name); - } - - const violations: LayeringViolation[] = []; - const pluginsManifest = `${PLUGINS_DIR}/package.json`; - const manifest = manifests.get(pluginsManifest); - for (const name of manifest ? declaredNames(JSON.parse(manifest) as DependencyManifest) : []) { - if (providers.has(name)) { - violations.push( - violation( - pluginsManifest, - 1, - `${name} is a provider package; the plugin framework must not declare it. ` + - `Provider knowledge reaches the loader as a policy from the composing root.`, - ), - ); - } - } - for (const [file, source] of sources) { - if (!file.startsWith(`${PLUGINS_DIR}/`)) continue; - for (const site of specifierSites(file, source)) { - if (!providers.has(packageName(site.specifier))) continue; - violations.push( - violation( - file, - site.line, - `${site.specifier} is a provider package; the plugin framework must not import it ` + - `(static, type-only, dynamic or mocked). Pass provider knowledge in as a policy from the composing root.`, - ), - ); - } - } - return violations; -} diff --git a/scripts/layering/test-kit-isolation.test.ts b/scripts/layering/test-kit-isolation.test.ts index 8bd1f8b984..07c1abe34f 100644 --- a/scripts/layering/test-kit-isolation.test.ts +++ b/scripts/layering/test-kit-isolation.test.ts @@ -1,6 +1,5 @@ -// The shape of test-kit (R83): one entry, built only on packages below its consumers, no platform, -// provider, daemon or root package reached directly or transitively, and no package it reaches -// declaring it back. +// The shape of test-kit (R83): a single entry that names its exports, plus the shared isolation +// walk (package-isolation.test.ts) composed under its policy. import assert from 'node:assert/strict'; import { test } from 'node:test'; @@ -94,127 +93,8 @@ test('every condition of the "." entry must point at src/index.ts', () => { assert.deepEqual(check({ 'test-kit': { exports: { '.': './src/index.ts' } } }), []); }); -test('test-kit may not depend on a platform, provider, daemon or root package, in any field', () => { - for (const name of [ - '@agent-device/platform-apple', - '@agent-device/provider-limrun', - '@agent-device/testmu', - '@agent-device/daemon', - 'agent-device', - ]) { - for (const field of [ - 'dependencies', - 'devDependencies', - 'optionalDependencies', - 'peerDependencies', - ]) { - const found = check({ 'test-kit': { [field]: { [name]: 'workspace:*' } } }); - assert.equal(found.length, 1, `${name} in ${field}`); - assert.match(found[0]!.message, /belongs in it/); - } - } -}); - -test('a provider published under a non-provider name is forbidden by its directory', () => { - const found = check({ - 'test-kit': { dependencies: { '@agent-device/testmu': 'workspace:*' } }, - }); - assert.equal(found.length, 1); - assert.match(found[0]!.message, /test-kit -> @agent-device\/testmu/); - // A lookalike name outside those directories is an ordinary package. - assert.deepEqual( - check({ 'test-kit': { dependencies: { '@agent-device/proxy': 'workspace:*' } } }), - [], - ); -}); - -test('a forbidden package reached through another package is refused with its path', () => { - const found = check({ - kernel: { dependencies: { '@agent-device/contracts': 'workspace:*' } }, - contracts: { optionalDependencies: { '@agent-device/platform-apple': 'workspace:*' } }, - }); - assert.equal(found.length, 1); - assert.match( - found[0]!.message, - /test-kit -> @agent-device\/kernel -> @agent-device\/contracts -> @agent-device\/platform-apple/, - ); -}); - -test('a package test-kit reaches directly or transitively cannot declare test-kit', () => { - const direct = check({ - kernel: { devDependencies: { '@agent-device/test-kit': 'workspace:*' } }, - }); - assert.equal(direct.length, 1); - assert.equal(direct[0]!.file, 'packages/kernel/package.json'); - assert.match(direct[0]!.message, /project-reference cycle/); - - const transitive = check({ - kernel: { dependencies: { '@agent-device/contracts': 'workspace:*' } }, - contracts: { optionalDependencies: { '@agent-device/test-kit': 'workspace:*' } }, - }); - assert.equal(transitive.length, 1); - assert.equal(transitive[0]!.file, 'packages/contracts/package.json'); - assert.match( - transitive[0]!.message, - /test-kit -> @agent-device\/kernel -> @agent-device\/contracts -> @agent-device\/test-kit/, - ); -}); - -test('a package outside test-kit reach may declare test-kit', () => { - assert.deepEqual( - check({ proxy: { devDependencies: { '@agent-device/test-kit': 'workspace:*' } } }), - [], - ); -}); - -test('test-kit may not import a platform, provider, daemon or root module, or reach out of its directory', () => { - for (const specifier of [ - '@agent-device/platform-android', - '@agent-device/platform-apple/runner', - '@agent-device/provider-limrun/connection', - '@agent-device/testmu/connection-verification', - '@agent-device/daemon/testing/context', - '@agent-device/daemon', - 'agent-device/commands', - '../../../src/platform-runtime-gateway.ts', - '../../kernel/src/errors.ts', - ]) { - const found = check( - {}, - new Map([['packages/test-kit/src/a.ts', `import { x } from '${specifier}';`]]), - ); - assert.equal(found.length, 1, specifier); - assert.equal(found[0]!.file, 'packages/test-kit/src/a.ts'); - } - // A sibling inside its own directory, and a lookalike package name, are fine. - assert.deepEqual( - check( - {}, - new Map([ - [ - 'packages/test-kit/src/a.ts', - "import { b } from './b.ts';\nimport { c } from '@agent-device/daemon-contracts/device-claims';", - ], - ]), - ), - [], - ); -}); - -test('the shared import walk applies: a vi.mock literal reaches a forbidden package too', () => { - const found = check( - {}, - new Map([ - [ - 'packages/test-kit/src/a.ts', - "import { vi } from 'vitest';\nvi.mock('@agent-device/platform-apple/runner', () => ({}));", - ], - ]), - ); +test('the shared isolation walk runs under the test-kit policy', () => { + const found = check({ 'test-kit': { dependencies: { '@agent-device/daemon': 'workspace:*' } } }); assert.equal(found.length, 1); - assert.equal(found[0]!.line, 2); -}); - -test('a repo without a test-kit package has nothing to isolate', () => { - assert.deepEqual(checkTestKitIsolation(new Map(), new Map()), []); + assert.match(found[0]!.message, /test-kit -> @agent-device\/daemon/); }); diff --git a/scripts/layering/test-kit-isolation.ts b/scripts/layering/test-kit-isolation.ts index 42919e4b89..b40d02b941 100644 --- a/scripts/layering/test-kit-isolation.ts +++ b/scripts/layering/test-kit-isolation.ts @@ -1,48 +1,22 @@ -// Catches: `@agent-device/test-kit` growing a dependency on the code it exists to test, or a -// package it builds on declaring it back. test-kit is the one home of test helpers shared across -// packages, so it may build only on the packages below the ones that consume it. A fixture that -// needs a platform, provider, daemon or root module's code is a test of that package and belongs -// in it; and TypeScript project references are acyclic, so a package test-kit reaches (directly or -// through other packages) cannot declare test-kit and keeps its helpers beside its tests. -// Evidence: the single-entry test-kit loads every helper eagerly, so one platform import would put -// that platform's closure into every test's import graph, and a package test-kit builds on that -// declared test-kit would close a reference cycle `tsc -b` rejects. -// Cost: one pass over the tracked manifests and test-kit's sources. -// Kill criterion: a maintainer decision that test-kit may depend on platform, provider, daemon or -// root code. +// Catches: `@agent-device/test-kit` growing a second entry, a star re-export, or a dependency on +// the code it exists to test. test-kit is the one home of test helpers shared across packages and +// every helper is a named export of its single entry, so a fixture that needs a platform, +// provider, daemon or root module's code is a test of that package and belongs in it. +// Evidence: the single-entry test-kit loads every helper eagerly, so a second entry or a star +// re-export would publish a module's whole surface into every test's import graph. +// Cost: one pass over the tracked manifests and test-kit's entry. +// Kill criterion: a maintainer decision that test-kit may expose subpath entries or depend on +// platform, provider, daemon or root code. // -// Part of R83 package-export-surface. The forbidden set is derived from the workspace layout, not -// from name patterns: every package under `packages/platform-*` or `packages/provider-*` (whatever -// name it publishes, e.g. `@agent-device/testmu`), the daemon package, and the root package. +// Part of R83 package-export-surface. The dependency, import and reference-cycle half is the +// shared walk in package-isolation.ts under TEST_KIT_ISOLATION. -import path from 'node:path'; import type { LayeringViolation } from './model.ts'; -import { specifierSites } from './package-boundaries.ts'; import { PACKAGE_EXPORT_SURFACE_RULE, TEST_KIT_PACKAGE } from './package-export-surface.ts'; +import { checkPackageIsolation, parseManifests, TEST_KIT_ISOLATION } from './package-isolation.ts'; -const TEST_KIT_DIR = 'packages/test-kit'; +const TEST_KIT_DIR = TEST_KIT_ISOLATION.dir; const TEST_KIT_ENTRY = './src/index.ts'; -const ROOT_PACKAGE = 'agent-device'; -const FORBIDDEN_PACKAGE_DIR = /^packages\/(?:platform-[^/]+|provider-[^/]+|daemon)\/package\.json$/; - -type DependencyManifest = { - name?: string; - exports?: unknown; - dependencies?: Record; - devDependencies?: Record; - optionalDependencies?: Record; - peerDependencies?: Record; -}; - -/** Every package name a manifest declares in any dependency field. */ -function declaredNames(manifest: DependencyManifest): string[] { - return [ - ...Object.keys(manifest.dependencies ?? {}), - ...Object.keys(manifest.devDependencies ?? {}), - ...Object.keys(manifest.optionalDependencies ?? {}), - ...Object.keys(manifest.peerDependencies ?? {}), - ]; -} function stringLeaves(value: unknown): string[] { if (typeof value === 'string') return [value]; @@ -54,36 +28,18 @@ function violation(file: string, message: string, line = 1): LayeringViolation { return { rule: PACKAGE_EXPORT_SURFACE_RULE, file, line, message }; } -/** The package names test-kit may not build on, from the workspace layout. */ -function forbiddenNames(parsed: ReadonlyMap): Set { - const names = new Set([ROOT_PACKAGE]); - for (const [file, manifest] of parsed) { - if (FORBIDDEN_PACKAGE_DIR.test(file) && manifest.name) names.add(manifest.name); - } - return names; -} - -function isForbiddenSpecifier(specifier: string, forbidden: ReadonlySet): boolean { - const parts = specifier.split('/'); - const name = specifier.startsWith('@') ? parts.slice(0, 2).join('/') : parts[0]!; - return forbidden.has(name); -} - /** - * The shape of the one shared test package: a single `.` entry; no dependency, direct or through - * other workspace packages, on a platform, provider, daemon or root package; no import of one or - * relative reach out of its directory (the shared import walk, so `vi.mock` literals count); and no - * package it reaches declaring it back. + * The shape of the one shared test package: a single `.` entry that names its exports, plus the + * shared isolation walk (no platform, provider, daemon or root package reached directly, through + * other packages or by import; no package it reaches declaring it back). */ export function checkTestKitIsolation( manifests: ReadonlyMap, sources: ReadonlyMap, ): LayeringViolation[] { const violations: LayeringViolation[] = []; - const parsed = new Map(); - for (const [file, text] of manifests) parsed.set(file, JSON.parse(text) as DependencyManifest); const testKitFile = `${TEST_KIT_DIR}/package.json`; - const testKit = parsed.get(testKitFile); + const testKit = parseManifests(manifests).get(testKitFile); if (!testKit) return violations; const exported = (testKit.exports as Record | undefined) ?? {}; @@ -104,45 +60,7 @@ export function checkTestKitIsolation( ), ); } - - const fileByName = new Map(); - for (const [file, manifest] of parsed) if (manifest.name) fileByName.set(manifest.name, file); - const forbidden = forbiddenNames(parsed); - - // Breadth-first over the workspace dependency graph from test-kit; `via` is the path to a package. - const via = new Map([[TEST_KIT_PACKAGE, [TEST_KIT_PACKAGE]]]); - const queue = [TEST_KIT_PACKAGE]; - while (queue.length > 0) { - const current = queue.shift()!; - const manifest = parsed.get(fileByName.get(current) ?? ''); - if (!manifest) continue; - for (const name of declaredNames(manifest)) { - const chain = [...via.get(current)!, name]; - if (name === TEST_KIT_PACKAGE && current !== TEST_KIT_PACKAGE) { - violations.push( - violation( - fileByName.get(current)!, - `${chain.join(' -> ')} closes a project-reference cycle: keep '${current}' test helpers ` + - `beside its tests.`, - ), - ); - continue; - } - if (forbidden.has(name)) { - violations.push( - violation( - testKitFile, - `${chain.join(' -> ')}: ${TEST_KIT_PACKAGE} builds on a platform, provider, daemon or root ` + - `package. A fixture that needs that code is a test of that package and belongs in it.`, - ), - ); - continue; - } - if (!fileByName.has(name) || via.has(name)) continue; - via.set(name, chain); - queue.push(name); - } - } + violations.push(...checkPackageIsolation(manifests, sources, TEST_KIT_ISOLATION)); const entrySource = sources.get(`${TEST_KIT_DIR}/src/index.ts`); const starExport = entrySource === undefined ? null : /^[ \t]*export\s*\*/m.exec(entrySource); @@ -156,23 +74,5 @@ export function checkTestKitIsolation( ), ); } - - for (const [file, source] of sources) { - if (!file.startsWith(`${TEST_KIT_DIR}/`)) continue; - for (const site of specifierSites(file, source)) { - const escapes = - site.specifier.startsWith('.') && - !path.posix.join(path.posix.dirname(file), site.specifier).startsWith(`${TEST_KIT_DIR}/`); - if (!escapes && !isForbiddenSpecifier(site.specifier, forbidden)) continue; - violations.push( - violation( - file, - `${TEST_KIT_PACKAGE} imports '${site.specifier}'. It may build only on the packages ` + - `below it, never on a platform, provider, daemon or root module.`, - site.line, - ), - ); - } - } return violations; } From 54e0f0383d69ee0ce94f356473f3c44395aacff9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 18:38:34 +0200 Subject: [PATCH 09/13] chore(gates): restore lexical shadowing in the Vitest mock-site reader A local that redeclares an imported Vitest alias is not the mock API; the shared isolation walk must not report a provider edge for it. --- scripts/layering/layering-ast.ts | 113 ++++++++++++++++++ scripts/layering/model-mock-bindings.test.ts | 24 +++- scripts/layering/model.ts | 30 +++-- ...package-boundaries-specifier-sites.test.ts | 1 + scripts/layering/package-isolation.test.ts | 21 ++++ 5 files changed, 177 insertions(+), 12 deletions(-) diff --git a/scripts/layering/layering-ast.ts b/scripts/layering/layering-ast.ts index 808672c0c5..970dcf16dc 100644 --- a/scripts/layering/layering-ast.ts +++ b/scripts/layering/layering-ast.ts @@ -134,3 +134,116 @@ export function destructuredDynamicImportBindings( visit(program); return captures; } + +function patternNames(pattern: unknown, into: string[]): void { + const node = asNode(pattern); + if (node === undefined) return; + switch (node.type) { + case 'Identifier': + into.push(node.name as string); + break; + case 'ObjectPattern': + for (const property of nodeList(node.properties)) { + const entry = asNode(property); + patternNames(entry?.type === 'Property' ? entry.value : entry, into); + } + break; + case 'ArrayPattern': + for (const element of nodeList(node.elements)) patternNames(element, into); + break; + case 'AssignmentPattern': + patternNames(node.left, into); + break; + case 'RestElement': + patternNames(node.argument, into); + break; + case 'TSParameterProperty': + patternNames(node.parameter, into); + break; + } +} + +function lexicalNames(statements: unknown[], into: string[]): void { + for (const statement of statements) { + const node = asNode(statement); + if (node?.type === 'VariableDeclaration' && node.kind !== 'var') { + for (const declarator of nodeList(node.declarations)) { + patternNames(asNode(declarator)?.id, into); + } + } else if (node?.type === 'FunctionDeclaration' || node?.type === 'ClassDeclaration') { + patternNames(node.id, into); + } + } +} + +function isFunctionNode(node: AstNode): boolean { + return ( + node.type === 'FunctionDeclaration' || + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + ); +} + +function hoistedVarNames(node: unknown, into: string[]): void { + for (const child of Array.isArray(node) ? node : [node]) { + const record = asNode(child); + if (record === undefined || isFunctionNode(record)) continue; + if (record.type === 'VariableDeclaration' && record.kind === 'var') { + for (const declarator of nodeList(record.declarations)) { + patternNames(asNode(declarator)?.id, into); + } + } + for (const value of Object.values(record)) hoistedVarNames(value, into); + } +} + +/** Names `node` itself declares for its own subtree: parameters, `var`s, block-level lexical declarations. */ +function scopeBindings(node: AstNode): string[] { + const names: string[] = []; + if (isFunctionNode(node)) { + for (const param of nodeList(node.params)) patternNames(param, names); + if (node.type === 'FunctionExpression') patternNames(node.id, names); + hoistedVarNames(node.body, names); + } else if ( + node.type === 'BlockStatement' || + node.type === 'StaticBlock' || + node.type === 'TSModuleBlock' + ) { + lexicalNames(nodeList(node.body), names); + } else if (node.type === 'ClassExpression') { + patternNames(node.id, names); + } else if (node.type === 'SwitchStatement') { + for (const switchCase of nodeList(node.cases)) { + lexicalNames(nodeList(asNode(switchCase)?.consequent), names); + } + } else if (node.type === 'ForStatement') { + lexicalNames([node.init], names); + } else if (node.type === 'ForInStatement' || node.type === 'ForOfStatement') { + lexicalNames([node.left], names); + } else if (node.type === 'CatchClause') { + patternNames(node.param, names); + } + return names; +} + +/** + * `visitAst` that also hands the visitor the names declared by the enclosing scopes: function + * parameters and `var`s, block-level `let`/`const`/`function`/`class`, loop heads and `catch` + * parameters. A `with` statement or `eval` can rebind names invisibly and is not modelled. + */ +export function visitAstScoped( + node: unknown, + shadowed: ReadonlySet, + visitor: (node: AstNode, shadowed: ReadonlySet) => void, +): void { + if (Array.isArray(node)) { + for (const child of node) visitAstScoped(child, shadowed, visitor); + return; + } + const record = asNode(node); + if (record === undefined) return; + const declared = scopeBindings(record); + const scope = declared.length > 0 ? new Set([...shadowed, ...declared]) : shadowed; + visitor(record, scope); + for (const child of Object.values(record)) visitAstScoped(child, scope, visitor); +} diff --git a/scripts/layering/model-mock-bindings.test.ts b/scripts/layering/model-mock-bindings.test.ts index 4b6b770020..ab77ceca06 100644 --- a/scripts/layering/model-mock-bindings.test.ts +++ b/scripts/layering/model-mock-bindings.test.ts @@ -83,14 +83,32 @@ test('a vitest namespace member other than vi or vitest is not the mock API', () ); }); -test('an alias a local redeclares still counts, so the walk only over-reports', () => { +test('an alias is not a mock receiver where an enclosing scope redeclares its name', () => { assert.deepEqual( specs( "import { vi as mocks } from 'vitest';", + "import * as testing from 'vitest';", "function parameter(mocks) { mocks.mock('a'); }", - "mocks.mock('b');", + "const arrow = (testing) => testing.vi.mock('b');", + "{ const mocks = other; mocks.mock('c'); }", + "function hoisted() { if (flag) { var mocks = other; } mocks.mock('d'); }", + "try {} catch (mocks) { mocks.mock('e'); }", + "for (const { mocks } of list) mocks.mock('f');", + "switch (kind) { case 1: class testing {} testing.vi.mock('g'); }", + "const expression = function mocks() { mocks.mock('h'); };", + "const named = class mocks { run() { mocks.mock('i'); } };", + "namespace Fixture { const testing = other; testing.vi.mock('j'); }", ), - ['a', 'b'], + [], + ); + assert.deepEqual( + specs( + "import { vi as mocks } from 'vitest';", + 'function unrelated(mocks) { return mocks; }', + '{ const mocks = other; }', + "mocks.mock('outside');", + ), + ['outside'], ); }); diff --git a/scripts/layering/model.ts b/scripts/layering/model.ts index 75d6ba2b3e..e7ac7636c2 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -10,6 +10,7 @@ import { nodeList, propertyName, visitAst, + visitAstScoped, type AstNode, } from './layering-ast.ts'; import { declaredRootModuleZone } from './root-module-zones.ts'; @@ -369,12 +370,19 @@ function vitestImportBindings(program: unknown): VitestBindings { return { apis, namespaces }; } -/** The global names `vi` and `vitest`, and the local names a `vitest` import binds to them. */ -function isMockApiReceiver(node: unknown, bindings: VitestBindings): boolean { +/** + * The global names `vi` and `vitest` always count. An imported alias or namespace counts only + * while no enclosing scope redeclares its local name. + */ +function isMockApiReceiver( + node: unknown, + bindings: VitestBindings, + shadowed: ReadonlySet, +): boolean { const receiver = unwrapExpression(node); if (receiver?.type === 'Identifier') { const name = receiver.name as string; - return MODULE_MOCK_RECEIVERS.has(name) || bindings.apis.has(name); + return MODULE_MOCK_RECEIVERS.has(name) || (bindings.apis.has(name) && !shadowed.has(name)); } if (receiver?.type !== 'MemberExpression') return false; const owner = unwrapExpression(receiver.object); @@ -382,15 +390,20 @@ function isMockApiReceiver(node: unknown, bindings: VitestBindings): boolean { return ( owner?.type === 'Identifier' && bindings.namespaces.has(owner.name as string) && + !shadowed.has(owner.name as string) && key !== undefined && MODULE_MOCK_RECEIVERS.has(key) ); } -function mockCallMethod(callee: unknown, bindings: VitestBindings): string | undefined { +function mockCallMethod( + callee: unknown, + bindings: VitestBindings, + shadowed: ReadonlySet, +): string | undefined { const member = unwrapExpression(callee); if (member?.type !== 'MemberExpression') return undefined; - if (!isMockApiReceiver(member.object, bindings)) return undefined; + if (!isMockApiReceiver(member.object, bindings, shadowed)) return undefined; const method = memberKey(member); return method !== undefined && MODULE_MOCK_METHODS.has(method) ? method : undefined; } @@ -405,8 +418,7 @@ function mockCallMethod(callee: unknown, bindings: VitestBindings): string | und * The receiver is the global `vi`/`vitest`, a name a top-level `import { vi as x } from 'vitest'` * binds to either, or a member `vi`/`vitest` of a `import * as ns from 'vitest'` namespace. Only * those import declarations are resolved: a value copied through a variable or a dynamic import - * is not followed, and a local that redeclares an alias's name still counts as the alias, so the - * walk can only over-report. + * is not followed. */ export function parseModuleMockSpecifiers(source: string): { spec: string; line: number }[] { const parsed = parseSync('layering-mocks.ts', source); @@ -417,9 +429,9 @@ export function parseModuleMockSpecifiers(source: string): { spec: string; line: } const bindings = vitestImportBindings(parsed.program); const sites: { spec: string; line: number }[] = []; - visitAst(parsed.program, (node) => { + visitAstScoped(parsed.program, new Set(), (node, shadowed) => { if (node.type !== 'CallExpression') return; - if (mockCallMethod(node.callee, bindings) === undefined) return; + if (mockCallMethod(node.callee, bindings, shadowed) === undefined) return; const args = node.arguments; const spec = Array.isArray(args) ? literalSpecifier(args[0]) : undefined; if (spec !== undefined) { diff --git a/scripts/layering/package-boundaries-specifier-sites.test.ts b/scripts/layering/package-boundaries-specifier-sites.test.ts index 7d5f7abd7b..76ac1130d2 100644 --- a/scripts/layering/package-boundaries-specifier-sites.test.ts +++ b/scripts/layering/package-boundaries-specifier-sites.test.ts @@ -73,6 +73,7 @@ test('specifier sites skip alias calls on other modules, other exports and type- "fromOther.mock('a');", "notMock.mock('b');", "typeOnly.mock('c');", + "function shadow(notMock) { notMock.mock('d'); }", ].join('\n'), ), ['1:not-vitest', '2:vitest', '3:vitest'], diff --git a/scripts/layering/package-isolation.test.ts b/scripts/layering/package-isolation.test.ts index 1a68f8c5e0..92dfc4a8fd 100644 --- a/scripts/layering/package-isolation.test.ts +++ b/scripts/layering/package-isolation.test.ts @@ -258,6 +258,27 @@ test('an import ignores comments, non-literal mock arguments, other objects and } }); +test('an alias an enclosing scope redeclares is a local object, not the mock API', () => { + for (const policy of POLICIES) { + const entry = "'@agent-device/provider-webdriver/plugin'"; + const vitest = "import { vi as mocks } from 'vitest';"; + for (const text of [ + `${vitest}\nfunction f(mocks) { mocks.mock(${entry}); }`, + `${vitest}\nconst f = (mocks) => mocks.doMock(${entry});`, + `${vitest}\n{ const mocks = other; mocks.mock(${entry}); }`, + `import * as testing from 'vitest';\nconst f = (testing) => testing.vi.mock(${entry});`, + ]) { + assert.deepEqual(check(policy, {}, source(policy, text)), [], `${policy.name}: ${text}`); + } + const found = check( + policy, + {}, + source(policy, `${vitest}\nfunction f(mocks) { return mocks; }\nmocks.mock(${entry});`), + ); + assert.equal(found.length, 1, policy.name); + } +}); + test('relative reach out of the package directory is refused', () => { for (const policy of POLICIES) { for (const specifier of [ From 3a2b428b83c3df84f906a049136e5b022eecf184 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 18:57:19 +0200 Subject: [PATCH 10/13] chore(gates): parse the workspace manifests once for the package-isolation walks The test-kit shape check and the shared isolation walk each parsed every workspace manifest. The walk now takes the parsed map, so the caller parses once at the boundary. --- scripts/layering/check.ts | 13 ++++++++++--- scripts/layering/package-isolation.test.ts | 15 ++++++++++++--- scripts/layering/package-isolation.ts | 18 +++++++----------- scripts/layering/test-kit-isolation.test.ts | 3 ++- scripts/layering/test-kit-isolation.ts | 10 +++++++--- 5 files changed, 38 insertions(+), 21 deletions(-) diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index 5e6db4ee8b..dd1f39ef20 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -161,7 +161,11 @@ import { sourceExecutionCompatibilityViolations } from './source-execution-polic import { sessionResourceOwnershipViolations } from './session-resource-ownership.ts'; import { applicationLifecycleOwnershipViolations } from './application-lifecycle-policy.ts'; import { iosSnapshotEngineOwnershipViolations } from './ios-snapshot-engine-policy.ts'; -import { checkPackageIsolation, PLUGIN_FRAMEWORK_ISOLATION } from './package-isolation.ts'; +import { + checkPackageIsolation, + parseManifests, + PLUGIN_FRAMEWORK_ISOLATION, +} from './package-isolation.ts'; import { providerSnapshotPresentationViolations } from './provider-snapshot-presentation-policy.ts'; import { snapshotAssemblyPresentationViolations } from './snapshot-assembly-presentation-policy.ts'; import { RETIRED_PATH_RULES, retiredPathRuleViolations } from './retired-paths-policy.ts'; @@ -620,11 +624,14 @@ export const LAYERING_RULES: Readonly> = { context.ratchets.testOnlyExports, context.reference.testOnlyExports, ), - ...checkTestKitIsolation(readTrackedPackageManifests(repoRoot), context.allTypeScriptSources), + ...checkTestKitIsolation( + parseManifests(readTrackedPackageManifests(repoRoot)), + context.allTypeScriptSources, + ), ], 'plugin-framework-independence': (context) => checkPackageIsolation( - readTrackedPackageManifests(repoRoot), + parseManifests(readTrackedPackageManifests(repoRoot)), context.allTypeScriptSources, PLUGIN_FRAMEWORK_ISOLATION, ), diff --git a/scripts/layering/package-isolation.test.ts b/scripts/layering/package-isolation.test.ts index 92dfc4a8fd..3ec24f07f1 100644 --- a/scripts/layering/package-isolation.test.ts +++ b/scripts/layering/package-isolation.test.ts @@ -8,6 +8,7 @@ import path from 'node:path'; import { test } from 'node:test'; import { checkPackageIsolation, + parseManifests, PLUGIN_FRAMEWORK_INDEPENDENCE_RULE, PLUGIN_FRAMEWORK_ISOLATION, TEST_KIT_ISOLATION, @@ -69,7 +70,11 @@ function check( overrides: Record = {}, sources: Record = {}, ) { - return checkPackageIsolation(workspace(overrides), new Map(Object.entries(sources)), policy); + return checkPackageIsolation( + parseManifests(workspace(overrides)), + new Map(Object.entries(sources)), + policy, + ); } function source(policy: PackageIsolationPolicy, text: string) { @@ -83,7 +88,7 @@ test('the real repository holds every isolation policy', () => { fs.readFileSync(path.join(repoRoot, file), 'utf8'), ]), ); - const manifests = readTrackedPackageManifests(repoRoot); + const manifests = parseManifests(readTrackedPackageManifests(repoRoot)); for (const policy of POLICIES) { assert.deepEqual(checkPackageIsolation(manifests, sources, policy), [], policy.name); } @@ -91,7 +96,11 @@ test('the real repository holds every isolation policy', () => { test('a repo without the isolated package has nothing to isolate', () => { for (const policy of POLICIES) { - assert.deepEqual(checkPackageIsolation(new Map(), new Map(), policy), [], policy.name); + assert.deepEqual( + checkPackageIsolation(parseManifests(new Map()), new Map(), policy), + [], + policy.name, + ); } }); diff --git a/scripts/layering/package-isolation.ts b/scripts/layering/package-isolation.ts index d15d631d6e..25738f0535 100644 --- a/scripts/layering/package-isolation.ts +++ b/scripts/layering/package-isolation.ts @@ -58,7 +58,7 @@ export const PLUGIN_FRAMEWORK_ISOLATION: PackageIsolationPolicy = { remedy: 'Provider knowledge reaches the loader as a policy from the composing root.', }; -type DependencyManifest = { +export type DependencyManifest = { name?: string; exports?: unknown; dependencies?: Record; @@ -67,9 +67,9 @@ type DependencyManifest = { peerDependencies?: Record; }; -export function parseManifests( - manifests: ReadonlyMap, -): Map { +export type ParsedManifests = ReadonlyMap; + +export function parseManifests(manifests: ReadonlyMap): ParsedManifests { return new Map( [...manifests].map(([file, text]) => [file, JSON.parse(text) as DependencyManifest]), ); @@ -87,10 +87,7 @@ function declaredNames(manifest: DependencyManifest): string[] { const ROOT_PACKAGE = 'agent-device'; -function forbiddenNames( - parsed: ReadonlyMap, - policy: PackageIsolationPolicy, -): Set { +function forbiddenNames(parsed: ParsedManifests, policy: PackageIsolationPolicy): Set { const names = new Set(policy.forbidsRoot ? [ROOT_PACKAGE] : []); for (const [file, manifest] of parsed) { if (policy.forbiddenManifests.test(file) && manifest.name) names.add(manifest.name); @@ -109,7 +106,7 @@ function packageName(specifier: string): string { * that declares the policy's package back closes a project-reference cycle. */ function manifestViolations( - parsed: ReadonlyMap, + parsed: ParsedManifests, policy: PackageIsolationPolicy, forbidden: ReadonlySet, ): LayeringViolation[] { @@ -161,11 +158,10 @@ function manifestViolations( * declaring it back. */ export function checkPackageIsolation( - manifests: ReadonlyMap, + parsed: ParsedManifests, sources: ReadonlyMap, policy: PackageIsolationPolicy, ): LayeringViolation[] { - const parsed = parseManifests(manifests); if (!parsed.has(`${policy.dir}/package.json`)) return []; const forbidden = forbiddenNames(parsed, policy); const violations = manifestViolations(parsed, policy, forbidden); diff --git a/scripts/layering/test-kit-isolation.test.ts b/scripts/layering/test-kit-isolation.test.ts index 07c1abe34f..59aa0a4944 100644 --- a/scripts/layering/test-kit-isolation.test.ts +++ b/scripts/layering/test-kit-isolation.test.ts @@ -3,6 +3,7 @@ import assert from 'node:assert/strict'; import { test } from 'node:test'; +import { parseManifests } from './package-isolation.ts'; import { checkTestKitIsolation } from './test-kit-isolation.ts'; type Manifest = Record; @@ -46,7 +47,7 @@ const SOURCE = new Map([ ]); function check(overrides: Record = {}, sources = SOURCE) { - return checkTestKitIsolation(workspace(overrides), sources); + return checkTestKitIsolation(parseManifests(workspace(overrides)), sources); } test('the test-kit shape holds: one entry, built only on packages below it', () => { diff --git a/scripts/layering/test-kit-isolation.ts b/scripts/layering/test-kit-isolation.ts index b40d02b941..42b760c020 100644 --- a/scripts/layering/test-kit-isolation.ts +++ b/scripts/layering/test-kit-isolation.ts @@ -13,7 +13,11 @@ import type { LayeringViolation } from './model.ts'; import { PACKAGE_EXPORT_SURFACE_RULE, TEST_KIT_PACKAGE } from './package-export-surface.ts'; -import { checkPackageIsolation, parseManifests, TEST_KIT_ISOLATION } from './package-isolation.ts'; +import { + checkPackageIsolation, + TEST_KIT_ISOLATION, + type ParsedManifests, +} from './package-isolation.ts'; const TEST_KIT_DIR = TEST_KIT_ISOLATION.dir; const TEST_KIT_ENTRY = './src/index.ts'; @@ -34,12 +38,12 @@ function violation(file: string, message: string, line = 1): LayeringViolation { * other packages or by import; no package it reaches declaring it back). */ export function checkTestKitIsolation( - manifests: ReadonlyMap, + manifests: ParsedManifests, sources: ReadonlyMap, ): LayeringViolation[] { const violations: LayeringViolation[] = []; const testKitFile = `${TEST_KIT_DIR}/package.json`; - const testKit = parseManifests(manifests).get(testKitFile); + const testKit = manifests.get(testKitFile); if (!testKit) return violations; const exported = (testKit.exports as Record | undefined) ?? {}; From 759e348b0116de9c98e5397da8ab53df390260e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 18:57:19 +0200 Subject: [PATCH 11/13] chore(gates): keep scope boundaries in the Vitest mock-site reader A parameter initializer, a switch discriminant, a class static block and a namespace body each have a scope boundary the reader flattened into the enclosing scope, so a later declaration hid a real mock of a provider. --- scripts/layering/layering-ast.ts | 56 ++++++++++++++---- scripts/layering/model-mock-bindings.test.ts | 60 ++++++++++++++++++++ scripts/layering/package-isolation.test.ts | 14 +++++ 3 files changed, 119 insertions(+), 11 deletions(-) diff --git a/scripts/layering/layering-ast.ts b/scripts/layering/layering-ast.ts index 970dcf16dc..9bd663bb29 100644 --- a/scripts/layering/layering-ast.ts +++ b/scripts/layering/layering-ast.ts @@ -184,10 +184,15 @@ function isFunctionNode(node: AstNode): boolean { ); } +/** Bodies that open their own `var` scope: a `var` inside one never reaches the enclosing function. */ +function isVarScopeBoundary(node: AstNode): boolean { + return isFunctionNode(node) || node.type === 'StaticBlock' || node.type === 'TSModuleBlock'; +} + function hoistedVarNames(node: unknown, into: string[]): void { for (const child of Array.isArray(node) ? node : [node]) { const record = asNode(child); - if (record === undefined || isFunctionNode(record)) continue; + if (record === undefined || isVarScopeBoundary(record)) continue; if (record.type === 'VariableDeclaration' && record.kind === 'var') { for (const declarator of nodeList(record.declarations)) { patternNames(asNode(declarator)?.id, into); @@ -197,18 +202,20 @@ function hoistedVarNames(node: unknown, into: string[]): void { } } -/** Names `node` itself declares for its own subtree: parameters, `var`s, block-level lexical declarations. */ +/** + * Names `node` declares for its own subtree: parameters, block-level lexical declarations, and the + * `var`s of a static block or namespace body. A function's own `var`s join its body only + * (`childScope`), since a parameter initializer runs before the body declarations exist. + */ function scopeBindings(node: AstNode): string[] { const names: string[] = []; if (isFunctionNode(node)) { for (const param of nodeList(node.params)) patternNames(param, names); if (node.type === 'FunctionExpression') patternNames(node.id, names); + } else if (node.type === 'StaticBlock' || node.type === 'TSModuleBlock') { + lexicalNames(nodeList(node.body), names); hoistedVarNames(node.body, names); - } else if ( - node.type === 'BlockStatement' || - node.type === 'StaticBlock' || - node.type === 'TSModuleBlock' - ) { + } else if (node.type === 'BlockStatement') { lexicalNames(nodeList(node.body), names); } else if (node.type === 'ClassExpression') { patternNames(node.id, names); @@ -226,10 +233,36 @@ function scopeBindings(node: AstNode): string[] { return names; } +function withNames(scope: ReadonlySet, names: readonly string[]): ReadonlySet { + return names.length > 0 ? new Set([...scope, ...names]) : scope; +} + +/** + * The scope a child property sees. Most children see everything the node declares; a switch + * discriminant runs before the case declarations exist, and only a function body sees the + * function's `var`s. + */ +function childScope( + node: AstNode, + key: string, + outer: ReadonlySet, + inner: ReadonlySet, +): ReadonlySet { + if (node.type === 'SwitchStatement' && key === 'discriminant') return outer; + if (isFunctionNode(node) && key === 'body') { + const vars: string[] = []; + hoistedVarNames(node.body, vars); + return withNames(inner, vars); + } + return inner; +} + /** * `visitAst` that also hands the visitor the names declared by the enclosing scopes: function * parameters and `var`s, block-level `let`/`const`/`function`/`class`, loop heads and `catch` - * parameters. A `with` statement or `eval` can rebind names invisibly and is not modelled. + * parameters. A parameter initializer, a switch discriminant, a static block and a namespace + * body keep their own boundaries. A `with` statement or `eval` can rebind names invisibly and is + * not modelled. */ export function visitAstScoped( node: unknown, @@ -242,8 +275,9 @@ export function visitAstScoped( } const record = asNode(node); if (record === undefined) return; - const declared = scopeBindings(record); - const scope = declared.length > 0 ? new Set([...shadowed, ...declared]) : shadowed; + const scope = withNames(shadowed, scopeBindings(record)); visitor(record, scope); - for (const child of Object.values(record)) visitAstScoped(child, scope, visitor); + for (const [key, child] of Object.entries(record)) { + visitAstScoped(child, childScope(record, key, shadowed, scope), visitor); + } } diff --git a/scripts/layering/model-mock-bindings.test.ts b/scripts/layering/model-mock-bindings.test.ts index ab77ceca06..3d535d4220 100644 --- a/scripts/layering/model-mock-bindings.test.ts +++ b/scripts/layering/model-mock-bindings.test.ts @@ -112,6 +112,66 @@ test('an alias is not a mock receiver where an enclosing scope redeclares its na ); }); +test('a parameter initializer sees the parameters but not the body declarations', () => { + const alias = "import { vi as mocks } from 'vitest';"; + assert.deepEqual( + specs( + alias, + "function bodyVar(x = mocks.mock('a')) { var mocks; }", + "const arrow = (x = mocks.doMock('b')) => { var mocks; };", + "function bodyFunction(x = mocks.mock('c')) { function mocks() {} }", + "const method = { run(x = mocks.mock('d')) { var mocks; } };", + ), + ['a', 'b', 'c', 'd'], + ); + assert.deepEqual( + specs( + alias, + "function earlier(mocks, x = mocks.mock('e')) {}", + "const arrow = (mocks, x = mocks.mock('f')) => x;", + "const expression = function mocks(x = mocks.mock('g')) {};", + "function body(x) { var mocks = other; return mocks.mock('h'); }", + ), + [], + ); +}); + +test('a switch discriminant sees the enclosing scope, not the case declarations', () => { + const alias = "import { vi as mocks } from 'vitest';"; + assert.deepEqual(specs(alias, "switch (mocks.mock('a')) { case 0: let mocks; }"), ['a']); + assert.deepEqual( + specs(alias, "switch (kind) { case 0: const mocks = other; mocks.mock('b'); }"), + [], + ); + assert.deepEqual( + specs(alias, "switch (kind) { case 0: let mocks; break; default: mocks.mock('c'); }"), + [], + ); +}); + +test('a static block and a namespace body are var scopes of their own', () => { + const alias = "import { vi as mocks } from 'vitest';"; + assert.deepEqual( + specs( + alias, + "function staticVar() { class C { static { var mocks; } } mocks.mock('a'); }", + "function namespaceVar() { namespace N { var mocks = other; } mocks.mock('b'); }", + "class D { static { var mocks = other; } } mocks.mock('c');", + ), + ['a', 'b', 'c'], + ); + assert.deepEqual( + specs( + alias, + "class C { static { var mocks = other; mocks.mock('d'); } }", + "class E { static { if (flag) { var mocks = other; } mocks.mock('e'); } }", + "namespace N { var mocks = other; mocks.mock('f'); }", + "namespace M { if (flag) { var mocks = other; } mocks.mock('g'); }", + ), + [], + ); +}); + test('the global vi and vitest names stay mock receivers beside import aliases', () => { assert.deepEqual( specs( diff --git a/scripts/layering/package-isolation.test.ts b/scripts/layering/package-isolation.test.ts index 3ec24f07f1..2c97c0a082 100644 --- a/scripts/layering/package-isolation.test.ts +++ b/scripts/layering/package-isolation.test.ts @@ -288,6 +288,20 @@ test('an alias an enclosing scope redeclares is a local object, not the mock API } }); +test('a mock of a provider survives a shadow that only a later scope boundary declares', () => { + for (const policy of POLICIES) { + const entry = "'@agent-device/provider-webdriver/plugin'"; + const vitest = "import { vi as mocks } from 'vitest';"; + for (const text of [ + `${vitest}\nfunction f(x = mocks.mock(${entry})) { var mocks; }`, + `${vitest}\nswitch (mocks.mock(${entry})) { case 0: let mocks; }`, + `${vitest}\nfunction f() { class C { static { var mocks; } } mocks.mock(${entry}); }`, + ]) { + assert.equal(check(policy, {}, source(policy, text)).length, 1, `${policy.name}: ${text}`); + } + } +}); + test('relative reach out of the package directory is refused', () => { for (const policy of POLICIES) { for (const specifier of [ From 708b3238662119cfd58e81c314eaa0b26334a6b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 21:05:53 +0200 Subject: [PATCH 12/13] chore(gates): read Vitest mock-site aliases without lexical scope tracking An imported vi/vitest alias or namespace now counts everywhere in the file, as the global vi and vitest already do. A local that shadows an alias name is over-reported; no mock call is missed. Drops visitAstScoped and its binding machinery. --- scripts/layering/layering-ast.ts | 147 ------------------- scripts/layering/model-mock-bindings.test.ts | 80 ++-------- scripts/layering/model.ts | 31 ++-- scripts/layering/package-isolation.test.ts | 24 +-- scripts/layering/package-isolation.ts | 3 +- 5 files changed, 25 insertions(+), 260 deletions(-) diff --git a/scripts/layering/layering-ast.ts b/scripts/layering/layering-ast.ts index 9bd663bb29..808672c0c5 100644 --- a/scripts/layering/layering-ast.ts +++ b/scripts/layering/layering-ast.ts @@ -134,150 +134,3 @@ export function destructuredDynamicImportBindings( visit(program); return captures; } - -function patternNames(pattern: unknown, into: string[]): void { - const node = asNode(pattern); - if (node === undefined) return; - switch (node.type) { - case 'Identifier': - into.push(node.name as string); - break; - case 'ObjectPattern': - for (const property of nodeList(node.properties)) { - const entry = asNode(property); - patternNames(entry?.type === 'Property' ? entry.value : entry, into); - } - break; - case 'ArrayPattern': - for (const element of nodeList(node.elements)) patternNames(element, into); - break; - case 'AssignmentPattern': - patternNames(node.left, into); - break; - case 'RestElement': - patternNames(node.argument, into); - break; - case 'TSParameterProperty': - patternNames(node.parameter, into); - break; - } -} - -function lexicalNames(statements: unknown[], into: string[]): void { - for (const statement of statements) { - const node = asNode(statement); - if (node?.type === 'VariableDeclaration' && node.kind !== 'var') { - for (const declarator of nodeList(node.declarations)) { - patternNames(asNode(declarator)?.id, into); - } - } else if (node?.type === 'FunctionDeclaration' || node?.type === 'ClassDeclaration') { - patternNames(node.id, into); - } - } -} - -function isFunctionNode(node: AstNode): boolean { - return ( - node.type === 'FunctionDeclaration' || - node.type === 'FunctionExpression' || - node.type === 'ArrowFunctionExpression' - ); -} - -/** Bodies that open their own `var` scope: a `var` inside one never reaches the enclosing function. */ -function isVarScopeBoundary(node: AstNode): boolean { - return isFunctionNode(node) || node.type === 'StaticBlock' || node.type === 'TSModuleBlock'; -} - -function hoistedVarNames(node: unknown, into: string[]): void { - for (const child of Array.isArray(node) ? node : [node]) { - const record = asNode(child); - if (record === undefined || isVarScopeBoundary(record)) continue; - if (record.type === 'VariableDeclaration' && record.kind === 'var') { - for (const declarator of nodeList(record.declarations)) { - patternNames(asNode(declarator)?.id, into); - } - } - for (const value of Object.values(record)) hoistedVarNames(value, into); - } -} - -/** - * Names `node` declares for its own subtree: parameters, block-level lexical declarations, and the - * `var`s of a static block or namespace body. A function's own `var`s join its body only - * (`childScope`), since a parameter initializer runs before the body declarations exist. - */ -function scopeBindings(node: AstNode): string[] { - const names: string[] = []; - if (isFunctionNode(node)) { - for (const param of nodeList(node.params)) patternNames(param, names); - if (node.type === 'FunctionExpression') patternNames(node.id, names); - } else if (node.type === 'StaticBlock' || node.type === 'TSModuleBlock') { - lexicalNames(nodeList(node.body), names); - hoistedVarNames(node.body, names); - } else if (node.type === 'BlockStatement') { - lexicalNames(nodeList(node.body), names); - } else if (node.type === 'ClassExpression') { - patternNames(node.id, names); - } else if (node.type === 'SwitchStatement') { - for (const switchCase of nodeList(node.cases)) { - lexicalNames(nodeList(asNode(switchCase)?.consequent), names); - } - } else if (node.type === 'ForStatement') { - lexicalNames([node.init], names); - } else if (node.type === 'ForInStatement' || node.type === 'ForOfStatement') { - lexicalNames([node.left], names); - } else if (node.type === 'CatchClause') { - patternNames(node.param, names); - } - return names; -} - -function withNames(scope: ReadonlySet, names: readonly string[]): ReadonlySet { - return names.length > 0 ? new Set([...scope, ...names]) : scope; -} - -/** - * The scope a child property sees. Most children see everything the node declares; a switch - * discriminant runs before the case declarations exist, and only a function body sees the - * function's `var`s. - */ -function childScope( - node: AstNode, - key: string, - outer: ReadonlySet, - inner: ReadonlySet, -): ReadonlySet { - if (node.type === 'SwitchStatement' && key === 'discriminant') return outer; - if (isFunctionNode(node) && key === 'body') { - const vars: string[] = []; - hoistedVarNames(node.body, vars); - return withNames(inner, vars); - } - return inner; -} - -/** - * `visitAst` that also hands the visitor the names declared by the enclosing scopes: function - * parameters and `var`s, block-level `let`/`const`/`function`/`class`, loop heads and `catch` - * parameters. A parameter initializer, a switch discriminant, a static block and a namespace - * body keep their own boundaries. A `with` statement or `eval` can rebind names invisibly and is - * not modelled. - */ -export function visitAstScoped( - node: unknown, - shadowed: ReadonlySet, - visitor: (node: AstNode, shadowed: ReadonlySet) => void, -): void { - if (Array.isArray(node)) { - for (const child of node) visitAstScoped(child, shadowed, visitor); - return; - } - const record = asNode(node); - if (record === undefined) return; - const scope = withNames(shadowed, scopeBindings(record)); - visitor(record, scope); - for (const [key, child] of Object.entries(record)) { - visitAstScoped(child, childScope(record, key, shadowed, scope), visitor); - } -} diff --git a/scripts/layering/model-mock-bindings.test.ts b/scripts/layering/model-mock-bindings.test.ts index 3d535d4220..19ec5dfd16 100644 --- a/scripts/layering/model-mock-bindings.test.ts +++ b/scripts/layering/model-mock-bindings.test.ts @@ -83,7 +83,7 @@ test('a vitest namespace member other than vi or vitest is not the mock API', () ); }); -test('an alias is not a mock receiver where an enclosing scope redeclares its name', () => { +test('an alias counts everywhere in the file, even where a local shadows its name', () => { assert.deepEqual( specs( "import { vi as mocks } from 'vitest';", @@ -91,84 +91,22 @@ test('an alias is not a mock receiver where an enclosing scope redeclares its na "function parameter(mocks) { mocks.mock('a'); }", "const arrow = (testing) => testing.vi.mock('b');", "{ const mocks = other; mocks.mock('c'); }", - "function hoisted() { if (flag) { var mocks = other; } mocks.mock('d'); }", - "try {} catch (mocks) { mocks.mock('e'); }", - "for (const { mocks } of list) mocks.mock('f');", - "switch (kind) { case 1: class testing {} testing.vi.mock('g'); }", - "const expression = function mocks() { mocks.mock('h'); };", - "const named = class mocks { run() { mocks.mock('i'); } };", - "namespace Fixture { const testing = other; testing.vi.mock('j'); }", - ), - [], - ); - assert.deepEqual( - specs( - "import { vi as mocks } from 'vitest';", - 'function unrelated(mocks) { return mocks; }', - '{ const mocks = other; }', - "mocks.mock('outside');", - ), - ['outside'], - ); -}); - -test('a parameter initializer sees the parameters but not the body declarations', () => { - const alias = "import { vi as mocks } from 'vitest';"; - assert.deepEqual( - specs( - alias, - "function bodyVar(x = mocks.mock('a')) { var mocks; }", - "const arrow = (x = mocks.doMock('b')) => { var mocks; };", - "function bodyFunction(x = mocks.mock('c')) { function mocks() {} }", - "const method = { run(x = mocks.mock('d')) { var mocks; } };", + "try {} catch (mocks) { mocks.mock('d'); }", ), ['a', 'b', 'c', 'd'], ); - assert.deepEqual( - specs( - alias, - "function earlier(mocks, x = mocks.mock('e')) {}", - "const arrow = (mocks, x = mocks.mock('f')) => x;", - "const expression = function mocks(x = mocks.mock('g')) {};", - "function body(x) { var mocks = other; return mocks.mock('h'); }", - ), - [], - ); }); -test('a switch discriminant sees the enclosing scope, not the case declarations', () => { - const alias = "import { vi as mocks } from 'vitest';"; - assert.deepEqual(specs(alias, "switch (mocks.mock('a')) { case 0: let mocks; }"), ['a']); - assert.deepEqual( - specs(alias, "switch (kind) { case 0: const mocks = other; mocks.mock('b'); }"), - [], - ); - assert.deepEqual( - specs(alias, "switch (kind) { case 0: let mocks; break; default: mocks.mock('c'); }"), - [], - ); -}); - -test('a static block and a namespace body are var scopes of their own', () => { - const alias = "import { vi as mocks } from 'vitest';"; +test('an alias call is read inside every construct that can hold a module literal', () => { assert.deepEqual( specs( - alias, - "function staticVar() { class C { static { var mocks; } } mocks.mock('a'); }", - "function namespaceVar() { namespace N { var mocks = other; } mocks.mock('b'); }", - "class D { static { var mocks = other; } } mocks.mock('c');", - ), - ['a', 'b', 'c'], - ); - assert.deepEqual( - specs( - alias, - "class C { static { var mocks = other; mocks.mock('d'); } }", - "class E { static { if (flag) { var mocks = other; } mocks.mock('e'); } }", - "namespace N { var mocks = other; mocks.mock('f'); }", - "namespace M { if (flag) { var mocks = other; } mocks.mock('g'); }", + "import { vi as mocks } from 'vitest';", + "function initializer(x = mocks.mock('a')) { var mocks; }", + "switch (mocks.mock('b')) { case 0: let mocks; }", + "class C { static { mocks.doMock('c'); } }", + "namespace N { mocks.unmock('d'); }", ), - [], + ['a', 'b', 'c', 'd'], ); }); diff --git a/scripts/layering/model.ts b/scripts/layering/model.ts index e7ac7636c2..fc033ebaec 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -10,7 +10,6 @@ import { nodeList, propertyName, visitAst, - visitAstScoped, type AstNode, } from './layering-ast.ts'; import { declaredRootModuleZone } from './root-module-zones.ts'; @@ -371,18 +370,15 @@ function vitestImportBindings(program: unknown): VitestBindings { } /** - * The global names `vi` and `vitest` always count. An imported alias or namespace counts only - * while no enclosing scope redeclares its local name. + * The global names `vi` and `vitest` always count, and so does every name `vitestImportBindings` + * found, wherever it appears in the file: a local that shadows an imported alias is still read as + * the mock API. That over-reports a rare shadowed name and never misses a mock call. */ -function isMockApiReceiver( - node: unknown, - bindings: VitestBindings, - shadowed: ReadonlySet, -): boolean { +function isMockApiReceiver(node: unknown, bindings: VitestBindings): boolean { const receiver = unwrapExpression(node); if (receiver?.type === 'Identifier') { const name = receiver.name as string; - return MODULE_MOCK_RECEIVERS.has(name) || (bindings.apis.has(name) && !shadowed.has(name)); + return MODULE_MOCK_RECEIVERS.has(name) || bindings.apis.has(name); } if (receiver?.type !== 'MemberExpression') return false; const owner = unwrapExpression(receiver.object); @@ -390,20 +386,15 @@ function isMockApiReceiver( return ( owner?.type === 'Identifier' && bindings.namespaces.has(owner.name as string) && - !shadowed.has(owner.name as string) && key !== undefined && MODULE_MOCK_RECEIVERS.has(key) ); } -function mockCallMethod( - callee: unknown, - bindings: VitestBindings, - shadowed: ReadonlySet, -): string | undefined { +function mockCallMethod(callee: unknown, bindings: VitestBindings): string | undefined { const member = unwrapExpression(callee); if (member?.type !== 'MemberExpression') return undefined; - if (!isMockApiReceiver(member.object, bindings, shadowed)) return undefined; + if (!isMockApiReceiver(member.object, bindings)) return undefined; const method = memberKey(member); return method !== undefined && MODULE_MOCK_METHODS.has(method) ? method : undefined; } @@ -417,8 +408,8 @@ function mockCallMethod( * * The receiver is the global `vi`/`vitest`, a name a top-level `import { vi as x } from 'vitest'` * binds to either, or a member `vi`/`vitest` of a `import * as ns from 'vitest'` namespace. Only - * those import declarations are resolved: a value copied through a variable or a dynamic import - * is not followed. + * those import declarations are resolved, and a recognized name counts everywhere in the file; a + * value copied through a variable or a dynamic import is not followed. */ export function parseModuleMockSpecifiers(source: string): { spec: string; line: number }[] { const parsed = parseSync('layering-mocks.ts', source); @@ -429,9 +420,9 @@ export function parseModuleMockSpecifiers(source: string): { spec: string; line: } const bindings = vitestImportBindings(parsed.program); const sites: { spec: string; line: number }[] = []; - visitAstScoped(parsed.program, new Set(), (node, shadowed) => { + visitAst(parsed.program, (node) => { if (node.type !== 'CallExpression') return; - if (mockCallMethod(node.callee, bindings, shadowed) === undefined) return; + if (mockCallMethod(node.callee, bindings) === undefined) return; const args = node.arguments; const spec = Array.isArray(args) ? literalSpecifier(args[0]) : undefined; if (spec !== undefined) { diff --git a/scripts/layering/package-isolation.test.ts b/scripts/layering/package-isolation.test.ts index 2c97c0a082..edd5fe4cd0 100644 --- a/scripts/layering/package-isolation.test.ts +++ b/scripts/layering/package-isolation.test.ts @@ -267,35 +267,17 @@ test('an import ignores comments, non-literal mock arguments, other objects and } }); -test('an alias an enclosing scope redeclares is a local object, not the mock API', () => { +test('a mock of a provider is refused through an alias even where a local shadows the alias name', () => { for (const policy of POLICIES) { const entry = "'@agent-device/provider-webdriver/plugin'"; const vitest = "import { vi as mocks } from 'vitest';"; for (const text of [ `${vitest}\nfunction f(mocks) { mocks.mock(${entry}); }`, - `${vitest}\nconst f = (mocks) => mocks.doMock(${entry});`, - `${vitest}\n{ const mocks = other; mocks.mock(${entry}); }`, + `${vitest}\n{ const mocks = other; mocks.doMock(${entry}); }`, `import * as testing from 'vitest';\nconst f = (testing) => testing.vi.mock(${entry});`, - ]) { - assert.deepEqual(check(policy, {}, source(policy, text)), [], `${policy.name}: ${text}`); - } - const found = check( - policy, - {}, - source(policy, `${vitest}\nfunction f(mocks) { return mocks; }\nmocks.mock(${entry});`), - ); - assert.equal(found.length, 1, policy.name); - } -}); - -test('a mock of a provider survives a shadow that only a later scope boundary declares', () => { - for (const policy of POLICIES) { - const entry = "'@agent-device/provider-webdriver/plugin'"; - const vitest = "import { vi as mocks } from 'vitest';"; - for (const text of [ `${vitest}\nfunction f(x = mocks.mock(${entry})) { var mocks; }`, `${vitest}\nswitch (mocks.mock(${entry})) { case 0: let mocks; }`, - `${vitest}\nfunction f() { class C { static { var mocks; } } mocks.mock(${entry}); }`, + `${vitest}\nclass C { static { mocks.mock(${entry}); } }`, ]) { assert.equal(check(policy, {}, source(policy, text)).length, 1, `${policy.name}: ${text}`); } diff --git a/scripts/layering/package-isolation.ts b/scripts/layering/package-isolation.ts index 25738f0535..4abd714f53 100644 --- a/scripts/layering/package-isolation.ts +++ b/scripts/layering/package-isolation.ts @@ -14,7 +14,8 @@ // // The forbidden set is derived from the workspace layout, not from name patterns: every package // whose directory matches the policy, whatever name it publishes (e.g. `@agent-device/testmu` under -// `packages/provider-testmu`). A new isolated package is one more row in ISOLATION_POLICIES. +// `packages/provider-testmu`). A new isolated package is one more `PackageIsolationPolicy` +// declaration passed to the check. import path from 'node:path'; import type { LayeringViolation } from './model.ts'; From 89825a5449612b43c6a0f59cd1dbb704a95d13c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 16 Oct 2026 21:08:28 +0200 Subject: [PATCH 13/13] chore(gates): name the mock-receiver shapes the Vitest mock-site reader does not follow --- scripts/layering/model.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts/layering/model.ts b/scripts/layering/model.ts index fc033ebaec..5bf7c45c5c 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -408,8 +408,9 @@ function mockCallMethod(callee: unknown, bindings: VitestBindings): string | und * * The receiver is the global `vi`/`vitest`, a name a top-level `import { vi as x } from 'vitest'` * binds to either, or a member `vi`/`vitest` of a `import * as ns from 'vitest'` namespace. Only - * those import declarations are resolved, and a recognized name counts everywhere in the file; a - * value copied through a variable or a dynamic import is not followed. + * those import declarations are resolved, and a recognized name counts everywhere in the file. A + * value copied through a variable or a destructuring, reached through `globalThis`, `require` or + * a dynamic import, or called through `.call`/`.apply` or a comma expression is not followed. */ export function parseModuleMockSpecifiers(source: string): { spec: string; line: number }[] { const parsed = parseSync('layering-mocks.ts', source);