From 702f63bf7e74f24ee5ddc8ca793b56b6aa25e909 Mon Sep 17 00:00:00 2001 From: Ricky Schema Cascade Date: Mon, 24 Aug 2026 11:28:47 +0200 Subject: [PATCH 1/2] fix(deploy): resolve authored persona imports from the installed CLI tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agentworkforce deploy ./persona.ts` failed on a fresh install with `Could not resolve "@agentworkforce/persona-kit"` whenever the user's repo had no node_modules of its own — which is the normal case for a globally installed CLI. `packageNodePaths` built esbuild's fallback roots as `join(here, '..', '..', '..', 'node_modules')`. In an installed layout `here` is `/node_modules/@agentworkforce/deploy/dist`, so three levels up is already `/node_modules` and the extra segment yielded `/node_modules/node_modules` — a path that only exists in the dev monorepo checkout (`repo/packages/deploy/dist` → `repo/node_modules`), which is why this passed CI while every published CLI failed. The `cwd/node_modules` fallback masked it for anyone deploying from a repo that happened to have persona-kit installed. Build the full node_modules lookup chain instead, using Node's own semantics — including the case the old join got wrong, where an ancestor that *is* node_modules is itself the search root. Chains are taken from the persona file, this installed package, and the cwd, deduped and ordered so the project's own dependencies still win. Also annotate esbuild's bare "Could not resolve" with the package name and the directory to install it in, for packages neither the project nor the CLI ships (e.g. `@agentworkforce/turn-kit`, which is not a CLI dependency). Verified against a clean `npm install agentworkforce@4.1.47` tree with the persona in a node_modules-free repo: reproduced the reported error, and both `loadPersonaSourceFile` and `extractAgentSpec` now succeed. The new integration test copies the built package into a fake install layout and fails with the reported error when only `packageNodePaths` is reverted. Co-Authored-By: Claude Opus 5 --- packages/deploy/src/extract-agent.ts | 5 +- packages/deploy/src/persona-source.test.ts | 122 +++++++++++++++++++++ packages/deploy/src/persona-source.ts | 90 +++++++++++++-- 3 files changed, 209 insertions(+), 8 deletions(-) create mode 100644 packages/deploy/src/persona-source.test.ts diff --git a/packages/deploy/src/extract-agent.ts b/packages/deploy/src/extract-agent.ts index 7c050630..b3d30c00 100644 --- a/packages/deploy/src/extract-agent.ts +++ b/packages/deploy/src/extract-agent.ts @@ -13,7 +13,8 @@ import { assertReadableFile, extractDefaultExport, packageNodePaths, - preserveLocalImportMetaUrlPlugin + preserveLocalImportMetaUrlPlugin, + withUnresolvedImportHint } from './persona-source.js'; /** @@ -107,6 +108,8 @@ export async function extractAgentSpec(onEventPath: string): Promise { + throw withUnresolvedImportHint(err, onEventPath); }); const mod = await import(pathToFileURL(compiledPath).href); diff --git a/packages/deploy/src/persona-source.test.ts b/packages/deploy/src/persona-source.test.ts new file mode 100644 index 00000000..bee16ad0 --- /dev/null +++ b/packages/deploy/src/persona-source.test.ts @@ -0,0 +1,122 @@ +import assert from 'node:assert/strict'; +import { cp, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'; +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 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 }); + } +}); diff --git a/packages/deploy/src/persona-source.ts b/packages/deploy/src/persona-source.ts index 73b133b2..581ba8a8 100644 --- a/packages/deploy/src/persona-source.ts +++ b/packages/deploy/src/persona-source.ts @@ -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,88 @@ 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 `/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(); + 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)); + error.message = [ + error.message, + `Install the missing package${missing.length > 1 ? 's' : ''} where the persona lives:`, + ` cd ${projectDir} && npm install ${missing.join(' ')}` + ].join('\n'); + return error; +} + +function unresolvedSpecifiers(message: string): string[] { + const found = new Set(); + 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; + found.add(specifier); + } + return [...found]; } export function preserveLocalImportMetaUrlPlugin(): Plugin { From 7dc348a93b3d6710b02f9f474c7be01947f48835 Mon Sep 17 00:00:00 2001 From: Ricky Schema Cascade Date: Mon, 24 Aug 2026 11:41:15 +0200 Subject: [PATCH 2/2] fix(deploy): harden the unresolved-import hint (PR feedback) Two review findings on the install hint, both real: - CodeRabbit: the hint is written to be pasted into a shell, so a specifier or path carrying shell metacharacters (`evil; touch pwned`, a directory with an apostrophe) rendered as multiple commands. Quote both, leaving ordinary paths and package names bare so the common message stays readable. - codex: a deep import (`@relayfile/adapter-core/triggers`, `lodash/fp`) was passed to npm verbatim, which npm reads as a local directory or a git remote. Reduce specifiers to the package name first. Tests cover both, including a round-trip through /bin/sh proving the injected command cannot execute. Co-Authored-By: Claude Opus 5 --- packages/deploy/src/persona-source.test.ts | 30 ++++++++++++++++++++++ packages/deploy/src/persona-source.ts | 28 ++++++++++++++++++-- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/packages/deploy/src/persona-source.test.ts b/packages/deploy/src/persona-source.test.ts index bee16ad0..168717bc 100644 --- a/packages/deploy/src/persona-source.test.ts +++ b/packages/deploy/src/persona-source.test.ts @@ -1,5 +1,6 @@ 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'; @@ -45,6 +46,35 @@ test('withUnresolvedImportHint names the missing package and where to install it 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'); diff --git a/packages/deploy/src/persona-source.ts b/packages/deploy/src/persona-source.ts index 581ba8a8..c4d6b348 100644 --- a/packages/deploy/src/persona-source.ts +++ b/packages/deploy/src/persona-source.ts @@ -173,10 +173,11 @@ export function withUnresolvedImportHint(error: unknown, absInput: string): unkn 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 ${projectDir} && npm install ${missing.join(' ')}` + ` cd ${shellQuote(projectDir)} && npm install ${install}` ].join('\n'); return error; } @@ -187,11 +188,34 @@ function unresolvedSpecifiers(message: string): string[] { const specifier = match[1]; // Relative/absolute imports are authoring mistakes, not missing installs. if (specifier.startsWith('.') || specifier.startsWith('/')) continue; - found.add(specifier); + 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 { return { name: 'agentworkforce-preserve-local-import-meta-url',