-
Notifications
You must be signed in to change notification settings - Fork 0
fix(deploy): resolve authored persona imports from the installed CLI tree #325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+263
−8
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { cp, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'; | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { createRequire } from 'node:module'; | ||
| import { tmpdir } from 'node:os'; | ||
| import path from 'node:path'; | ||
| import test from 'node:test'; | ||
| import { fileURLToPath, pathToFileURL } from 'node:url'; | ||
|
|
||
| import { nodeModulesChain, packageNodePaths, withUnresolvedImportHint } from './persona-source.js'; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
| const HERE = path.dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| test('nodeModulesChain treats an ancestor node_modules as a search root', () => { | ||
| const chain = nodeModulesChain('/prefix/node_modules/@agentworkforce/deploy/dist'); | ||
| assert.ok( | ||
| chain.includes('/prefix/node_modules'), | ||
| `installed tree root missing from ${JSON.stringify(chain)}` | ||
| ); | ||
| assert.ok( | ||
| !chain.some((dir) => dir.endsWith('node_modules/node_modules')), | ||
| `doubled node_modules segment in ${JSON.stringify(chain)}` | ||
| ); | ||
| }); | ||
|
|
||
| test('packageNodePaths searches the persona tree before the CLI tree', () => { | ||
| const paths = packageNodePaths('/repo/customer-success/app-signal/persona.ts'); | ||
| assert.equal(paths[0], path.join('/repo/customer-success/app-signal', 'node_modules')); | ||
| assert.equal(new Set(paths).size, paths.length, 'duplicate entries'); | ||
| assert.ok( | ||
| paths.includes(nodeModulesChain(HERE).find((dir) => dir.endsWith('node_modules')) as string), | ||
| 'this package install root missing' | ||
| ); | ||
| }); | ||
|
|
||
| test('withUnresolvedImportHint names the missing package and where to install it', () => { | ||
| const hinted = withUnresolvedImportHint( | ||
| new Error( | ||
| 'Build failed with 1 error:\npersona.ts:2:30: ERROR: Could not resolve "@agentworkforce/turn-kit"' | ||
| ), | ||
| '/repo/customer-success/app-signal/persona.ts' | ||
| ); | ||
| assert.ok(hinted instanceof Error); | ||
| assert.match(hinted.message, /npm install @agentworkforce\/turn-kit/); | ||
| assert.match(hinted.message, /\/repo\/customer-success\/app-signal/); | ||
| }); | ||
|
|
||
| test('withUnresolvedImportHint installs the package, not the deep import path', () => { | ||
| const hinted = withUnresolvedImportHint( | ||
| new Error( | ||
| [ | ||
| 'ERROR: Could not resolve "@relayfile/adapter-core/triggers"', | ||
| 'ERROR: Could not resolve "lodash/fp"' | ||
| ].join('\n') | ||
| ), | ||
| '/repo/persona.ts' | ||
| ) as Error; | ||
| assert.match(hinted.message, /npm install @relayfile\/adapter-core lodash$/m); | ||
| }); | ||
|
|
||
| test('withUnresolvedImportHint quotes shell metacharacters', () => { | ||
| const hinted = withUnresolvedImportHint( | ||
| new Error(String.raw`ERROR: Could not resolve "evil; touch pwned"`), | ||
| "/repo/it's a dir/persona.ts" | ||
| ) as Error; | ||
| const command = hinted.message.split('\n').at(-1) as string; | ||
| assert.equal(command, String.raw` cd '/repo/it'\''s a dir' && npm install 'evil; touch pwned'`); | ||
| // Round-trip through a shell to prove the injected command cannot run. | ||
| assert.deepEqual( | ||
| execFileSync('/bin/sh', ['-c', `set -- ${command.trim().split('npm install ')[1]}; printf '%s' "$1"`], { | ||
| encoding: 'utf8' | ||
| }), | ||
| 'evil; touch pwned' | ||
| ); | ||
| }); | ||
|
|
||
| test('withUnresolvedImportHint ignores relative imports', () => { | ||
| const message = 'ERROR: Could not resolve "./missing.js"'; | ||
| const hinted = withUnresolvedImportHint(new Error(message), '/repo/persona.ts'); | ||
| assert.equal((hinted as Error).message, message); | ||
| }); | ||
|
|
||
| /** | ||
| * The shipped failure: a globally installed CLI compiling a persona in a repo | ||
| * that has no `node_modules`. Reproduced by copying the built package into an | ||
| * installed layout so `import.meta.url` inside persona-source resolves the way | ||
| * it does for a real `npm i -g agentworkforce`. | ||
| */ | ||
| test('authored persona resolves a CLI-tree package with no project node_modules', async () => { | ||
| const root = await mkdtemp(path.join(tmpdir(), 'agentworkforce-install-')); | ||
| const originalCwd = process.cwd(); | ||
| try { | ||
| const installRoot = path.join(root, 'prefix', 'node_modules'); | ||
| const deployDir = path.join(installRoot, '@agentworkforce', 'deploy'); | ||
| await mkdir(deployDir, { recursive: true }); | ||
| await cp(HERE, path.join(deployDir, 'dist'), { recursive: true }); | ||
| await writeFile( | ||
| path.join(deployDir, 'package.json'), | ||
| JSON.stringify({ name: '@agentworkforce/deploy', type: 'module' }), | ||
| 'utf8' | ||
| ); | ||
| // esbuild is a real dependency of the copied package; link it into the | ||
| // fake tree so the copy can load at all. | ||
| await symlink( | ||
| path.dirname(require.resolve('esbuild/package.json')), | ||
| path.join(installRoot, 'esbuild') | ||
| ); | ||
|
|
||
| const kitDir = path.join(installRoot, '@agentworkforce', 'persona-kit'); | ||
| await mkdir(kitDir, { recursive: true }); | ||
| await writeFile( | ||
| path.join(kitDir, 'package.json'), | ||
| JSON.stringify({ name: '@agentworkforce/persona-kit', type: 'module', main: 'index.js' }), | ||
| 'utf8' | ||
| ); | ||
| await writeFile( | ||
| path.join(kitDir, 'index.js'), | ||
| 'export function definePersona(input) { return input; }\n', | ||
| 'utf8' | ||
| ); | ||
|
|
||
| // The persona lives outside the install tree and has no node_modules. | ||
| const personaDir = path.join(root, 'watchdog-agents', 'customer-success', 'app-signal'); | ||
| await mkdir(personaDir, { recursive: true }); | ||
| const personaPath = path.join(personaDir, 'persona.ts'); | ||
| await writeFile( | ||
| personaPath, | ||
| [ | ||
| "import { definePersona } from '@agentworkforce/persona-kit';", | ||
| '', | ||
| "export default definePersona({ id: 'app-signal', onEvent: './agent.ts' });", | ||
| '' | ||
| ].join('\n'), | ||
| 'utf8' | ||
| ); | ||
|
|
||
| const installed = (await import( | ||
| pathToFileURL(path.join(deployDir, 'dist', 'persona-source.js')).href | ||
| )) as typeof import('./persona-source.js'); | ||
|
|
||
| // The user's cwd is their own repo, which has no node_modules either — | ||
| // without this the test's cwd (this monorepo) resolves the import and the | ||
| // assertion holds even against the broken implementation. | ||
| process.chdir(personaDir); | ||
| const result = await installed.loadPersonaSourceFile(personaPath); | ||
| assert.deepEqual(result.persona, { id: 'app-signal', onEvent: './agent.ts' }); | ||
| } finally { | ||
| process.chdir(originalCwd); | ||
| await rm(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import { randomUUID } from 'node:crypto'; | |
| import { builtinModules } from 'node:module'; | ||
| import { mkdtemp, readFile, rm, stat } from 'node:fs/promises'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { dirname, join, resolve } from 'node:path'; | ||
| import { basename, dirname, join, resolve } from 'node:path'; | ||
| import { fileURLToPath, pathToFileURL } from 'node:url'; | ||
|
|
||
| import { build, type Loader, type Plugin } from 'esbuild'; | ||
|
|
@@ -74,6 +74,8 @@ export async function loadPersonaSourceFile( | |
| resolveExtensions: RESOLVE_EXTENSIONS, | ||
| plugins: [preserveLocalImportMetaUrlPlugin()], | ||
| nodePaths: packageNodePaths(absInput) | ||
| }).catch((err) => { | ||
| throw withUnresolvedImportHint(err, absInput); | ||
| }); | ||
|
|
||
| const mod = await import(pathToFileURL(compiledPath).href); | ||
|
|
@@ -106,14 +108,112 @@ function extensionOf(inputPath: string): string { | |
| return idx === -1 ? '' : normalized.slice(idx); | ||
| } | ||
|
|
||
| /** | ||
| * esbuild `nodePaths` fallbacks for compiling authored `.ts`/`.js` personas | ||
| * and agents. | ||
| * | ||
| * An authored persona lives in the user's own repo, which on a fresh install | ||
| * usually has no `node_modules` at all — the CLI is installed globally, so | ||
| * `@agentworkforce/*` exists only inside the CLI's own install tree. esbuild | ||
| * resolves bare imports by walking up from the importer, so without these | ||
| * fallbacks `import { definePersona } from '@agentworkforce/persona-kit'` | ||
| * fails with "Could not resolve". | ||
| * | ||
| * Fallbacks are the full `node_modules` lookup chains of (in order) the | ||
| * persona file, this module (the installed `@agentworkforce/deploy`), and the | ||
| * cwd. The chain is computed the way Node's own resolver does it, including | ||
| * the case a naive `join(dir, 'node_modules')` gets wrong: when an ancestor | ||
| * *is* `node_modules`, that directory is itself the search root. Missing that | ||
| * produced `<prefix>/node_modules/node_modules` — a path that only exists in | ||
| * a monorepo checkout, which is why every installed CLI failed here while the | ||
| * dev layout worked. | ||
| * | ||
| * Order matters: the persona's own dependencies win over the CLI's copies, | ||
| * and every entry is only consulted after normal resolution has failed. | ||
| */ | ||
| export function packageNodePaths(absInput: string): string[] { | ||
| const here = dirname(fileURLToPath(import.meta.url)); | ||
| return [ | ||
| join(dirname(absInput), 'node_modules'), | ||
| join(here, '..', 'node_modules'), | ||
| join(here, '..', '..', '..', 'node_modules'), | ||
| join(process.cwd(), 'node_modules') | ||
| ]; | ||
| const seen = new Set<string>(); | ||
| const paths: string[] = []; | ||
| for (const root of [dirname(resolve(absInput)), here, process.cwd()]) { | ||
| for (const dir of nodeModulesChain(root)) { | ||
| if (seen.has(dir)) continue; | ||
| seen.add(dir); | ||
| paths.push(dir); | ||
| } | ||
| } | ||
| return paths; | ||
| } | ||
|
|
||
| /** Every `node_modules` directory Node would search from `fromDir` upward. */ | ||
| export function nodeModulesChain(fromDir: string): string[] { | ||
| const chain: string[] = []; | ||
| let dir = resolve(fromDir); | ||
| for (;;) { | ||
| chain.push(basename(dir) === 'node_modules' ? dir : join(dir, 'node_modules')); | ||
| const parent = dirname(dir); | ||
| if (parent === dir) break; | ||
| dir = parent; | ||
| } | ||
| return chain; | ||
| } | ||
|
|
||
| /** | ||
| * Turn esbuild's bare "Could not resolve" into an actionable install hint. | ||
| * | ||
| * After `packageNodePaths`, an `@agentworkforce/*` import the CLI ships | ||
| * resolves on its own; what reaches here is a package neither the project nor | ||
| * the CLI has (a persona kit the CLI does not depend on, a third-party SDK the | ||
| * handler imports). Naming it plus the directory to install it in is the | ||
| * difference between a dead end and a one-line fix. | ||
| */ | ||
| export function withUnresolvedImportHint(error: unknown, absInput: string): unknown { | ||
| if (!(error instanceof Error)) return error; | ||
| const missing = unresolvedSpecifiers(error.message); | ||
| if (missing.length === 0) return error; | ||
|
|
||
| const projectDir = dirname(resolve(absInput)); | ||
| const install = missing.map(shellQuote).join(' '); | ||
| error.message = [ | ||
| error.message, | ||
| `Install the missing package${missing.length > 1 ? 's' : ''} where the persona lives:`, | ||
| ` cd ${shellQuote(projectDir)} && npm install ${install}` | ||
| ].join('\n'); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| return error; | ||
| } | ||
|
|
||
| function unresolvedSpecifiers(message: string): string[] { | ||
| const found = new Set<string>(); | ||
| for (const match of message.matchAll(/Could not resolve "([^"]+)"/g)) { | ||
| const specifier = match[1]; | ||
| // Relative/absolute imports are authoring mistakes, not missing installs. | ||
| if (specifier.startsWith('.') || specifier.startsWith('/')) continue; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: On Windows, absolute drive-letter and UNC imports pass this filter and receive a misleading npm package-install hint. Skip platform-independent Windows-rooted paths before adding install hints. Prompt for AI agents |
||
| found.add(packageNameOf(specifier)); | ||
| } | ||
| return [...found]; | ||
| } | ||
|
|
||
| /** | ||
| * The package to install for an import specifier. A deep import | ||
| * (`@relayfile/adapter-core/triggers`, `lodash/fp`) must be reduced to its | ||
| * package name — npm reads the full specifier as a local directory or a git | ||
| * remote and would install the wrong thing, or nothing. | ||
| */ | ||
| function packageNameOf(specifier: string): string { | ||
| const segments = specifier.split('/'); | ||
| return specifier.startsWith('@') ? segments.slice(0, 2).join('/') : segments[0]; | ||
| } | ||
|
|
||
| /** | ||
| * Quote a value for the copy-pasteable `cd … && npm install …` hint. Both the | ||
| * path and the specifiers originate in the user's own source, but the hint is | ||
| * written to be pasted into a shell, so a specifier like `x; rm -rf ~` must not | ||
| * survive as two commands. Ordinary paths and package names are left bare so | ||
| * the common message stays readable. | ||
| */ | ||
| function shellQuote(value: string): string { | ||
| if (value.length > 0 && /^[\w@./:+-]+$/.test(value)) return value; | ||
| return `'${value.split("'").join(`'\\''`)}'`; | ||
| } | ||
|
|
||
| export function preserveLocalImportMetaUrlPlugin(): Plugin { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: These tests hardcode POSIX absolute paths and assert exact string equality against path.join() outputs, so they break on Windows where dirname(resolve(...)) returns drive-prefixed, backslash-normalized paths. The integration test's symlink setup also needs privileges/Developer Mode there. Since this is a cross-platform globally installed CLI, either build paths with os.tmpdir()-based roots and posix.compare/path normalization when comparing, or skip the path-equality assertions on win32.
Prompt for AI agents