The extension loader emits "Extension does not export a valid factory function" for shared libraries like cmux that live in the extensions/ directory but are not extensions. Previous fixes (#1537, #1545) added pi manifest opt-out checks in the three discovery layers, but a defense-in-depth gap remained: if any discovery path fails to filter a library, loadExtension() reports it as a broken extension. Add isNonExtensionLibrary() check in loadExtension() itself. When a module does not export a factory function, the loader now checks the nearest package.json for a "pi" manifest with no declared extensions before reporting an error. Libraries with "pi": {} are silently skipped instead of producing a spurious error on every startup. Fixes #1709 Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
GitHub
parent
973846cdc6
commit
63a61196e8
2 changed files with 241 additions and 0 deletions
|
|
@ -579,6 +579,46 @@ async function loadExtensionModule(extensionPath: string) {
|
|||
return typeof factory !== "function" ? undefined : factory;
|
||||
}
|
||||
|
||||
/**
|
||||
* Check whether a module path belongs to a non-extension library that should
|
||||
* be silently skipped rather than reported as an error.
|
||||
*
|
||||
* A directory is a non-extension library when its package.json has a "pi"
|
||||
* manifest that declares no extensions (e.g. `"pi": {}`). This is the
|
||||
* opt-out convention used by shared libraries like cmux that live inside
|
||||
* the extensions/ directory but are not extensions themselves.
|
||||
*
|
||||
* This serves as a defense-in-depth check: even if the upstream discovery
|
||||
* layers fail to filter out the library, the loader itself will not emit
|
||||
* a spurious error.
|
||||
*/
|
||||
function isNonExtensionLibrary(resolvedPath: string): boolean {
|
||||
// Walk up from the resolved file to find the nearest package.json
|
||||
let dir = path.dirname(resolvedPath);
|
||||
const root = path.parse(dir).root;
|
||||
while (dir !== root) {
|
||||
const packageJsonPath = path.join(dir, "package.json");
|
||||
if (fs.existsSync(packageJsonPath)) {
|
||||
try {
|
||||
const content = fs.readFileSync(packageJsonPath, "utf-8");
|
||||
const pkg = JSON.parse(content);
|
||||
if (pkg.pi && typeof pkg.pi === "object") {
|
||||
// Has a pi manifest — check if it declares any extensions
|
||||
const extensions = pkg.pi.extensions;
|
||||
if (!Array.isArray(extensions) || extensions.length === 0) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
// Malformed package.json — not a known library
|
||||
}
|
||||
break;
|
||||
}
|
||||
dir = path.dirname(dir);
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Create an Extension object with empty collections.
|
||||
*/
|
||||
|
|
@ -607,6 +647,12 @@ async function loadExtension(
|
|||
try {
|
||||
const factory = await loadExtensionModule(resolvedPath);
|
||||
if (!factory) {
|
||||
// Defense-in-depth: if the module is inside a directory that has
|
||||
// explicitly opted out of extension loading via its pi manifest,
|
||||
// silently skip it instead of reporting a spurious error.
|
||||
if (isNonExtensionLibrary(resolvedPath)) {
|
||||
return { extension: null, error: null };
|
||||
}
|
||||
logExtensionTiming(extensionPath, Date.now() - start, "failed");
|
||||
return { extension: null, error: `Extension does not export a valid factory function: ${extensionPath}` };
|
||||
}
|
||||
|
|
|
|||
195
src/tests/non-extension-library.test.ts
Normal file
195
src/tests/non-extension-library.test.ts
Normal file
|
|
@ -0,0 +1,195 @@
|
|||
/**
|
||||
* Regression tests for #1709: non-extension libraries in extensions/ directory
|
||||
* must not produce spurious "Extension does not export a valid factory function" errors.
|
||||
*
|
||||
* These tests verify the defense-in-depth behavior added to the extension loader:
|
||||
* when a module fails to export a factory function, the loader checks the parent
|
||||
* directory's package.json for a "pi" manifest opt-out before reporting an error.
|
||||
*
|
||||
* The isNonExtensionLibrary logic is replicated here to test the algorithm
|
||||
* independently of the loader's heavy dependency tree.
|
||||
*/
|
||||
import test, { describe } from 'node:test'
|
||||
import assert from 'node:assert/strict'
|
||||
import { existsSync, mkdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs'
|
||||
import { dirname, join, parse } from 'node:path'
|
||||
import { tmpdir } from 'node:os'
|
||||
|
||||
function makeTempDir(): string {
|
||||
const dir = join(tmpdir(), `nonext-lib-test-${Date.now()}-${Math.random().toString(36).slice(2)}`)
|
||||
mkdirSync(dir, { recursive: true })
|
||||
return dir
|
||||
}
|
||||
|
||||
/**
|
||||
* Replica of the isNonExtensionLibrary function from loader.ts.
|
||||
* Tests the same algorithm to verify correctness without importing the loader.
|
||||
*/
|
||||
function isNonExtensionLibrary(resolvedPath: string): boolean {
|
||||
let dir = dirname(resolvedPath)
|
||||
const root = parse(dir).root
|
||||
while (dir !== root) {
|
||||
const packageJsonPath = join(dir, 'package.json')
|
||||
if (existsSync(packageJsonPath)) {
|
||||
try {
|
||||
const content = readFileSync(packageJsonPath, 'utf-8')
|
||||
const pkg = JSON.parse(content)
|
||||
if (pkg.pi && typeof pkg.pi === 'object') {
|
||||
const extensions = pkg.pi.extensions
|
||||
if (!Array.isArray(extensions) || extensions.length === 0) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
// Malformed package.json
|
||||
}
|
||||
break
|
||||
}
|
||||
dir = dirname(dir)
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
describe('isNonExtensionLibrary — defense-in-depth for #1709', () => {
|
||||
test('returns true for a file inside a directory with pi: {} (cmux pattern)', () => {
|
||||
const root = makeTempDir()
|
||||
try {
|
||||
const libDir = join(root, 'cmux')
|
||||
mkdirSync(libDir)
|
||||
writeFileSync(join(libDir, 'package.json'), JSON.stringify({
|
||||
name: '@gsd/cmux',
|
||||
description: 'cmux integration library — used by other extensions, not an extension itself',
|
||||
pi: {}
|
||||
}))
|
||||
writeFileSync(join(libDir, 'index.js'), 'module.exports.utility = function() {};')
|
||||
|
||||
assert.equal(
|
||||
isNonExtensionLibrary(join(libDir, 'index.js')),
|
||||
true,
|
||||
'cmux with pi: {} should be identified as a non-extension library'
|
||||
)
|
||||
} finally {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
test('returns true for pi.extensions as empty array', () => {
|
||||
const root = makeTempDir()
|
||||
try {
|
||||
const libDir = join(root, 'lib-empty')
|
||||
mkdirSync(libDir)
|
||||
writeFileSync(join(libDir, 'package.json'), JSON.stringify({
|
||||
name: 'lib-empty',
|
||||
pi: { extensions: [] }
|
||||
}))
|
||||
writeFileSync(join(libDir, 'index.js'), 'module.exports.helper = function() {};')
|
||||
|
||||
assert.equal(
|
||||
isNonExtensionLibrary(join(libDir, 'index.js')),
|
||||
true,
|
||||
'pi: { extensions: [] } should be identified as non-extension library'
|
||||
)
|
||||
} finally {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
test('returns false for a directory without pi manifest (broken extension)', () => {
|
||||
const root = makeTempDir()
|
||||
try {
|
||||
const extDir = join(root, 'broken-ext')
|
||||
mkdirSync(extDir)
|
||||
writeFileSync(join(extDir, 'package.json'), JSON.stringify({
|
||||
name: 'broken-ext'
|
||||
}))
|
||||
writeFileSync(join(extDir, 'index.js'), 'module.exports.notAFactory = function() {};')
|
||||
|
||||
assert.equal(
|
||||
isNonExtensionLibrary(join(extDir, 'index.js')),
|
||||
false,
|
||||
'directory without pi manifest should NOT be identified as non-extension library'
|
||||
)
|
||||
} finally {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
test('returns false when pi.extensions declares actual entries', () => {
|
||||
const root = makeTempDir()
|
||||
try {
|
||||
const extDir = join(root, 'declared-ext')
|
||||
mkdirSync(extDir)
|
||||
writeFileSync(join(extDir, 'package.json'), JSON.stringify({
|
||||
name: 'declared-ext',
|
||||
pi: { extensions: ['./index.js'] }
|
||||
}))
|
||||
writeFileSync(join(extDir, 'index.js'), 'module.exports.notAFactory = function() {};')
|
||||
|
||||
assert.equal(
|
||||
isNonExtensionLibrary(join(extDir, 'index.js')),
|
||||
false,
|
||||
'directory with declared extensions should NOT be identified as non-extension library'
|
||||
)
|
||||
} finally {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
test('returns false when no package.json exists at all', () => {
|
||||
const root = makeTempDir()
|
||||
try {
|
||||
const noManifest = join(root, 'no-manifest')
|
||||
mkdirSync(noManifest)
|
||||
writeFileSync(join(noManifest, 'index.js'), 'module.exports = {};')
|
||||
|
||||
// Should return false since there is no package.json with pi manifest
|
||||
// (it will find the temp dir's absence of package.json and return false)
|
||||
assert.equal(
|
||||
isNonExtensionLibrary(join(noManifest, 'index.js')),
|
||||
false,
|
||||
'directory without any package.json should NOT be identified as non-extension library'
|
||||
)
|
||||
} finally {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
test('handles malformed package.json gracefully', () => {
|
||||
const root = makeTempDir()
|
||||
try {
|
||||
const badDir = join(root, 'bad-json')
|
||||
mkdirSync(badDir)
|
||||
writeFileSync(join(badDir, 'package.json'), 'not valid json {{{')
|
||||
writeFileSync(join(badDir, 'index.js'), 'module.exports = {};')
|
||||
|
||||
assert.equal(
|
||||
isNonExtensionLibrary(join(badDir, 'index.js')),
|
||||
false,
|
||||
'malformed package.json should not cause a crash and should return false'
|
||||
)
|
||||
} finally {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
test('pi manifest with other fields but no extensions still opts out', () => {
|
||||
const root = makeTempDir()
|
||||
try {
|
||||
const libDir = join(root, 'lib-with-skills')
|
||||
mkdirSync(libDir)
|
||||
writeFileSync(join(libDir, 'package.json'), JSON.stringify({
|
||||
name: 'lib-with-skills',
|
||||
pi: { skills: ['./my-skill.md'] }
|
||||
}))
|
||||
writeFileSync(join(libDir, 'index.js'), 'module.exports.helper = function() {};')
|
||||
|
||||
assert.equal(
|
||||
isNonExtensionLibrary(join(libDir, 'index.js')),
|
||||
true,
|
||||
'pi manifest with skills but no extensions should be identified as non-extension library'
|
||||
)
|
||||
} finally {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue