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/scripts/layering/check.ts b/scripts/layering/check.ts index 785a7e5a48..dd1f39ef20 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -161,6 +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, + 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'; @@ -539,6 +544,7 @@ export const LAYERING_RULE_IDS = [ 'session-authority-overlay', 'daemon-layers', 'package-export-surface', + 'plugin-framework-independence', 'test-colocation', ] as const; @@ -618,8 +624,17 @@ 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( + parseManifests(readTrackedPackageManifests(repoRoot)), + context.allTypeScriptSources, + PLUGIN_FRAMEWORK_ISOLATION, + ), 'test-colocation': (context) => checkTestColocation(context.ratchets.unmatchedTests, context.reference.unmatchedTests), }; diff --git a/scripts/layering/layering-ast.ts b/scripts/layering/layering-ast.ts index 9c9e31572a..808672c0c5 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; diff --git a/scripts/layering/model-mock-bindings.test.ts b/scripts/layering/model-mock-bindings.test.ts new file mode 100644 index 0000000000..19ec5dfd16 --- /dev/null +++ b/scripts/layering/model-mock-bindings.test.ts @@ -0,0 +1,181 @@ +// 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 counts everywhere in the file, even where a local shadows 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'); }", + "try {} catch (mocks) { mocks.mock('d'); }", + ), + ['a', 'b', 'c', 'd'], + ); +}); + +test('an alias call is read inside every construct that can hold a module literal', () => { + assert.deepEqual( + specs( + "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'], + ); +}); + +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('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( + "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.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..5bf7c45c5c 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -3,7 +3,15 @@ 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 { + asNode, + destructuredDynamicImportBindings, + memberName, + nodeList, + propertyName, + visitAst, + type AstNode, +} from './layering-ast.ts'; import { declaredRootModuleZone } from './root-module-zones.ts'; import { importGraphFromResolvedEdges, @@ -296,6 +304,135 @@ 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', + 'unmock', + 'doUnmock', + 'importActual', + 'importMock', +]); +const EXPRESSION_WRAPPERS = new Set([ + 'ChainExpression', + 'ParenthesizedExpression', + 'TSAsExpression', + 'TSSatisfiesExpression', + 'TSNonNullExpression', + 'TSTypeAssertion', +]); + +/** 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); + } + 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, 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): 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); + } + 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) && + key !== undefined && + MODULE_MOCK_RECEIVERS.has(key) + ); +} + +function mockCallMethod(callee: unknown, bindings: VitestBindings): string | undefined { + const member = unwrapExpression(callee); + if (member?.type !== 'MemberExpression') return undefined; + if (!isMockApiReceiver(member.object, bindings)) return undefined; + const method = memberKey(member); + 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. + * + * 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 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); + if (parsed.errors.length > 0) { + throw new SyntaxError( + `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') return; + if (mockCallMethod(node.callee, bindings) === 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..76ac1130d2 --- /dev/null +++ b/scripts/layering/package-boundaries-specifier-sites.test.ts @@ -0,0 +1,81 @@ +// `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'), + ), + [], + ); +}); + +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/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/package-isolation.test.ts b/scripts/layering/package-isolation.test.ts new file mode 100644 index 0000000000..edd5fe4cd0 --- /dev/null +++ b/scripts/layering/package-isolation.test.ts @@ -0,0 +1,347 @@ +// 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, + parseManifests, + 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( + parseManifests(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 = parseManifests(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(parseManifests(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('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}\n{ const mocks = other; mocks.doMock(${entry}); }`, + `import * as testing from 'vitest';\nconst f = (testing) => testing.vi.mock(${entry});`, + `${vitest}\nfunction f(x = mocks.mock(${entry})) { var mocks; }`, + `${vitest}\nswitch (mocks.mock(${entry})) { case 0: let mocks; }`, + `${vitest}\nclass C { static { 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 [ + '../../../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..4abd714f53 --- /dev/null +++ b/scripts/layering/package-isolation.ts @@ -0,0 +1,188 @@ +// 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 `PackageIsolationPolicy` +// declaration passed to the check. + +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.', +}; + +export type DependencyManifest = { + name?: string; + exports?: unknown; + dependencies?: Record; + devDependencies?: Record; + optionalDependencies?: Record; + peerDependencies?: Record; +}; + +export type ParsedManifests = ReadonlyMap; + +export function parseManifests(manifests: ReadonlyMap): ParsedManifests { + 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: 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); + } + 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: ParsedManifests, + 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( + parsed: ParsedManifests, + sources: ReadonlyMap, + policy: PackageIsolationPolicy, +): LayeringViolation[] { + 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/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. diff --git a/scripts/layering/test-kit-isolation.test.ts b/scripts/layering/test-kit-isolation.test.ts index 8bd1f8b984..59aa0a4944 100644 --- a/scripts/layering/test-kit-isolation.test.ts +++ b/scripts/layering/test-kit-isolation.test.ts @@ -1,9 +1,9 @@ -// 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'; +import { parseManifests } from './package-isolation.ts'; import { checkTestKitIsolation } from './test-kit-isolation.ts'; type Manifest = Record; @@ -47,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', () => { @@ -94,127 +94,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..42b760c020 100644 --- a/scripts/layering/test-kit-isolation.ts +++ b/scripts/layering/test-kit-isolation.ts @@ -1,48 +1,26 @@ -// 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, + TEST_KIT_ISOLATION, + type ParsedManifests, +} 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 +32,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, + manifests: ParsedManifests, 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 = manifests.get(testKitFile); if (!testKit) return violations; const exported = (testKit.exports as Record | undefined) ?? {}; @@ -104,45 +64,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 +78,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; } 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..2e8ce91dca --- /dev/null +++ b/src/provider-plugin-policy.test.ts @@ -0,0 +1,188 @@ +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 } 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 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, + '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 withConnection( + 'example', + env, + 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( + withConnection( + 'example', + env, + policy, + async (connection) => await connection.verify({ flags, env }), + ), + { code: 'COMMAND_FAILED', message: 'verification failed' }, + ); + assert.deepEqual(shutdowns, ['example', 'example']); +}); + +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 }; + }, +});