fix(review): validate skill roots at mount and isolate provider default roots
ds-review-bot round 1 on the repository-plugin runtime: - a manifest-declared skill root absent or non-directory in the installed package now fails the plugin load (skill-local treats a missing root as legitimately empty, which silently mounted a skill-less plugin) - includeDefaultRoots: false no longer inherits $DSH_BUNDLED_SKILL_DIR, so isolated repository providers see only their explicit roots - prepared wrapper baseUrl schema requires the file: scheme, failing hostile URLs at the declared validation boundary - preparedPath reuses format.ts's isOutside; SERVER_NAME_PATTERN is exported and pinned equal to dsh-mcp-client's, with the restatement justified (the prepare bin keeps a zod-only module graph); the unexplained `as never` cast now carries its schemastery rationale - the import-free wrapper assertion also rejects dynamic import( - the headless fixture wrapper is regenerated by the real prepareDshPlugin and a drift test pins fixture == generator output - prepareDshPlugin JSDoc states the non-atomic publish repair contract
This commit is contained in:
Tianyi Cui
@@ -2,5 +2,5 @@
|
||||
# side as of the last confirmed-consistent state. Both languages carry equal authority;
|
||||
# after editing either side, bring the other along and re-record with:
|
||||
# pnpm run verify-translation-pairing --write packages/cordis/repository-plugin/README.md
|
||||
README.md: dab6287304f083e5c0ae128d5a3cb861332c076a
|
||||
README.zh.md: 790e601ad022ffa20c7d02a88353e972bf8bffe2
|
||||
README.md: 80744eb489d1714f59ba6e53207476a8ce222e24
|
||||
README.zh.md: d297b44e4a065fa99865e3a42d2c823c7b7c5848
|
||||
|
||||
@@ -36,7 +36,7 @@ The containing package manager still runs the configured repository package's li
|
||||
|
||||
## Runtime composition
|
||||
|
||||
Loading this package registers one effect-scoped Loader builtin. Each generated wrapper delegates to that builtin with its own module URL and prepared manifest. Repository skill roots mount as a uniquely named `dsh-skill-local` provider with default project/user roots excluded and watching disabled; cached package generations are immutable. Wrapper disposal removes the provider and all composed MCP clients through normal Cordis child-fiber teardown.
|
||||
Loading this package registers one effect-scoped Loader builtin. Each generated wrapper delegates to that builtin with its own module URL and prepared manifest. The runtime validates every declared skill root as an existing in-package directory before mounting — a package whose generated outputs were dropped (a `files`/`.npmignore` mistake, a damaged cache entry) fails the plugin load instead of silently mounting a skill-less plugin. Repository skill roots mount as a uniquely named `dsh-skill-local` provider with default project/user roots excluded and watching disabled; cached package generations are immutable. Wrapper disposal removes the provider and all composed MCP clients through normal Cordis child-fiber teardown.
|
||||
|
||||
## Common MCP format
|
||||
|
||||
|
||||
@@ -36,7 +36,7 @@
|
||||
|
||||
## 运行时组合
|
||||
|
||||
加载本包会注册一个 effect-scoped Loader builtin。每个生成的包装模块都把自身模块 URL 和已准备的 manifest 委托给该 builtin。Repository skill 根以唯一命名的 `dsh-skill-local` 提供方挂载，排除默认项目／用户根并禁用监视；缓存 package generation 是不可变的。包装模块 dispose 时，会通过正常的 Cordis 子 fiber teardown 移除提供方和所有组合的 MCP client。
|
||||
加载本包会注册一个 effect-scoped Loader builtin。每个生成的包装模块都把自身模块 URL 和已准备的 manifest 委托给该 builtin。运行时在挂载前会校验每个声明的 skill 根都是包内实际存在的目录——生成输出被丢弃的包（`files`／`.npmignore` 配置失误、缓存条目损坏）会使插件加载失败，而不是静默挂载一个没有 skill 的插件。Repository skill 根以唯一命名的 `dsh-skill-local` 提供方挂载，排除默认项目／用户根并禁用监视；缓存 package generation 是不可变的。包装模块 dispose 时，会通过正常的 Cordis 子 fiber teardown 移除提供方和所有组合的 MCP client。
|
||||
|
||||
## 通用 MCP 格式
|
||||
|
||||
|
||||
@@ -31,7 +31,10 @@ const preparedManifestSchema = z.object({
|
||||
mcpServers: z.string().min(1).optional(),
|
||||
}).strict()
|
||||
const preparedConfigSchema = z.object({
|
||||
baseUrl: z.url(),
|
||||
// Wrappers pass import.meta.url, which is always file: for an installed
|
||||
// package; any other scheme would only fail later inside fileURLToPath with
|
||||
// an uncontextualized TypeError, so reject it at this validation boundary.
|
||||
baseUrl: z.url({ protocol: /^file$/ }),
|
||||
manifest: preparedManifestSchema,
|
||||
}).strict()
|
||||
|
||||
@@ -70,7 +73,14 @@ export function parsePreparedPluginConfig(value: unknown): PreparedPluginConfig
|
||||
}
|
||||
}
|
||||
|
||||
function isOutside(root: string, candidate: string): boolean {
|
||||
/**
|
||||
* Whether `candidate` resolves outside `root` — the containment check shared
|
||||
* by prepare-time asset copying and runtime prepared-path resolution.
|
||||
* @param root - directory that must contain the candidate.
|
||||
* @param candidate - absolute path to test.
|
||||
* @returns true when the candidate escapes the root.
|
||||
*/
|
||||
export function isOutside(root: string, candidate: string): boolean {
|
||||
const path = relative(root, candidate)
|
||||
/* v8 ignore next -- Different-drive Windows relative paths cannot be produced on POSIX coverage hosts. */
|
||||
return path === '..' || path.startsWith(`..${sep}`) || isAbsolute(path)
|
||||
@@ -95,11 +105,22 @@ async function sourcePath(pluginDirectory: string, sourceRoot: string, configure
|
||||
}
|
||||
|
||||
function wrapperSource(manifest: PreparedPluginManifest): string {
|
||||
// The manifest is static, so the wrapper's service dependencies are too:
|
||||
// declaring them gates the wrapper fiber until the composition provides
|
||||
// them, which means the runtime's SkillLocal/McpClient children activate
|
||||
// within the wrapper's own load epoch and their failures (duplicate
|
||||
// provider names, damaged packages) reject the wrapper's Loader
|
||||
// transaction instead of leaving a silently PENDING or FAILED child.
|
||||
const inject = [
|
||||
'loader',
|
||||
...manifest.skills.length > 0 ? ['skills'] : [],
|
||||
...manifest.mcpServers === undefined ? [] : ['tools'],
|
||||
]
|
||||
return [
|
||||
'// Generated by dsh-plugin-prepare. Do not edit.',
|
||||
`const manifest = ${JSON.stringify(manifest)}`,
|
||||
`export const name = ${JSON.stringify(manifest.name)}`,
|
||||
"export const inject = ['loader']",
|
||||
`export const inject = ${JSON.stringify(inject)}`,
|
||||
'export async function apply(ctx) {',
|
||||
` const runtime = ctx.loader.builtins[${JSON.stringify(REPOSITORY_PLUGIN_BUILTIN)}]`,
|
||||
` if (runtime === undefined) throw new Error(${JSON.stringify(`missing Cordis builtin ${REPOSITORY_PLUGIN_BUILTIN}`)})`,
|
||||
@@ -111,6 +132,10 @@ function wrapperSource(manifest: PreparedPluginManifest): string {
|
||||
|
||||
/**
|
||||
* Validate and package one `.dsh-plugin` directory into static assets plus a fixed wrapper.
|
||||
* Outputs are staged and committed by rename, but the final publish (remove
|
||||
* old outputs, rename assets, rename entry) is not one atomic step: a crash
|
||||
* mid-publish can leave assets without an entry or neither. Rerunning prepare
|
||||
* repairs the package; partial outputs are never importable as a plugin.
|
||||
* @param directory - `.dsh-plugin` package directory; defaults to the prepare process cwd.
|
||||
* @returns the generated static manifest.
|
||||
*/
|
||||
|
||||
@@ -3,8 +3,8 @@
|
||||
* @module @deepseek-ai/dsh-repository-plugin
|
||||
*/
|
||||
|
||||
import { readFile } from 'node:fs/promises'
|
||||
import { dirname, isAbsolute, relative, resolve, sep } from 'node:path'
|
||||
import { readFile, stat } from 'node:fs/promises'
|
||||
import { dirname, isAbsolute, resolve } from 'node:path'
|
||||
import { fileURLToPath } from 'node:url'
|
||||
import type { Context } from 'cordis'
|
||||
import type {} from '@cordisjs/plugin-loader'
|
||||
@@ -12,6 +12,7 @@ import * as SkillLocal from '@deepseek-ai/dsh-skill-local'
|
||||
import * as McpClient from '@deepseek-ai/dsh-mcp-client'
|
||||
import {
|
||||
REPOSITORY_PLUGIN_BUILTIN,
|
||||
isOutside,
|
||||
parsePreparedPluginConfig,
|
||||
type PreparedPluginConfig,
|
||||
} from './format.ts'
|
||||
@@ -34,24 +35,42 @@ function preparedPath(baseUrl: string, configured: string): string {
|
||||
if (isAbsolute(configured)) throw new Error(`prepared DSH plugin path must be relative: ${JSON.stringify(configured)}`)
|
||||
const directory = dirname(fileURLToPath(baseUrl))
|
||||
const path = resolve(directory, configured)
|
||||
const rel = relative(directory, path)
|
||||
/* v8 ignore next -- Different-drive Windows relative paths cannot be produced on POSIX coverage hosts. */
|
||||
if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) {
|
||||
if (isOutside(directory, path)) {
|
||||
throw new Error(`prepared DSH plugin path escapes its package: ${JSON.stringify(configured)}`)
|
||||
}
|
||||
return path
|
||||
}
|
||||
|
||||
async function preparedDirectory(baseUrl: string, configured: string): Promise<string> {
|
||||
const path = preparedPath(baseUrl, configured)
|
||||
// A manifest-declared skill root missing from the installed package (files/
|
||||
// .npmignore dropping generated outputs, a damaged cache entry) must fail
|
||||
// the plugin load: the skill provider treats an absent root as legitimately
|
||||
// empty, which would silently mount a skill-less plugin.
|
||||
let info
|
||||
try {
|
||||
info = await stat(path)
|
||||
} catch (cause) {
|
||||
throw new Error(`prepared DSH plugin skill root is missing from the installed package: ${JSON.stringify(configured)}`, { cause })
|
||||
}
|
||||
if (!info.isDirectory()) {
|
||||
throw new Error(`prepared DSH plugin skill root is not a directory: ${JSON.stringify(configured)}`)
|
||||
}
|
||||
return path
|
||||
}
|
||||
|
||||
async function applyPrepared(ctx: Context, value: PreparedPluginConfig): Promise<void> {
|
||||
const config = parsePreparedPluginConfig(value)
|
||||
const directory = dirname(fileURLToPath(config.baseUrl))
|
||||
const skillDirectories = config.manifest.skills.map(path => preparedPath(config.baseUrl, path))
|
||||
const skillDirectories = await Promise.all(config.manifest.skills.map(path => preparedDirectory(config.baseUrl, path)))
|
||||
const mcpConfigs = config.manifest.mcpServers === undefined
|
||||
? []
|
||||
: resolveMcpServers(
|
||||
parseMcpDocument(await readFile(preparedPath(config.baseUrl, config.manifest.mcpServers), 'utf8')),
|
||||
process.env,
|
||||
directory,
|
||||
// Schemastery call signatures collapse the parameter to `never` under
|
||||
// NodeNext; ResolvedMcpServer is shaped for the Config union by design.
|
||||
).map(input => McpClient.Config(input as never))
|
||||
|
||||
await ctx.effect(async function* () {
|
||||
|
||||
@@ -5,7 +5,14 @@
|
||||
|
||||
import { z } from 'zod'
|
||||
|
||||
const SERVER_NAME_PATTERN = /^[A-Za-z0-9_-]{1,32}$/
|
||||
/**
|
||||
* Restates dsh-mcp-client's `SERVER_NAME_PATTERN` rather than importing it:
|
||||
* the prepare bin must stay a zod-only module graph (no tools seam, no MCP
|
||||
* SDK). Exported so `repository-plugin.spec.ts` pins equality with the
|
||||
* client's exported pattern — prepare-time validation cannot drift from the
|
||||
* registry that enforces uniqueness.
|
||||
*/
|
||||
export const SERVER_NAME_PATTERN = /^[A-Za-z0-9_-]{1,32}$/
|
||||
const ENVIRONMENT_NAME_PATTERN = /^[A-Za-z_][A-Za-z0-9_]*$/
|
||||
const PLACEHOLDER_PATTERN = /\$\{([^}]*)\}/g
|
||||
|
||||
@@ -89,7 +96,7 @@ export function parseMcpDocument(content: string): McpDocument {
|
||||
if (!result.success) throw new Error(`invalid .mcp.json:\n${z.prettifyError(result.error)}`)
|
||||
for (const [serverName, definition] of Object.entries(result.data.mcpServers)) {
|
||||
if (!SERVER_NAME_PATTERN.test(serverName)) {
|
||||
throw new Error(`invalid .mcp.json: server name ${JSON.stringify(serverName)} must match [A-Za-z0-9_-]{1,32}`)
|
||||
throw new Error(`invalid .mcp.json: server name ${JSON.stringify(serverName)} must match ${SERVER_NAME_PATTERN.source}`)
|
||||
}
|
||||
visitStrings(serverName, definition, assertTemplate)
|
||||
}
|
||||
|
||||
@@ -1,7 +1,15 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { parseMcpDocument, resolveMcpServers } from '../src/mcp.ts'
|
||||
import { SERVER_NAME_PATTERN as CLIENT_SERVER_NAME_PATTERN } from '@deepseek-ai/dsh-mcp-client'
|
||||
import { SERVER_NAME_PATTERN, parseMcpDocument, resolveMcpServers } from '../src/mcp.ts'
|
||||
|
||||
describe('repository plugin common .mcp.json support', () => {
|
||||
it('validates server names with exactly the pattern the MCP client registry enforces', () => {
|
||||
// mcp.ts restates the pattern to keep the prepare bin's module graph
|
||||
// zod-only; this pin is the drift guard.
|
||||
expect(SERVER_NAME_PATTERN.source).toBe(CLIENT_SERVER_NAME_PATTERN.source)
|
||||
expect(SERVER_NAME_PATTERN.flags).toBe(CLIENT_SERVER_NAME_PATTERN.flags)
|
||||
})
|
||||
|
||||
it('maps Expo-style HTTP servers to the existing Streamable HTTP client config', () => {
|
||||
const document = parseMcpDocument(JSON.stringify({
|
||||
mcpServers: {
|
||||
|
||||
@@ -59,7 +59,9 @@ describe('dsh-plugin-prepare', () => {
|
||||
})
|
||||
const wrapper = await readFile(join(directory, RepositoryPlugin.PREPARED_ENTRY_FILENAME), 'utf8')
|
||||
expect(wrapper).toContain(`ctx.loader.builtins["${RepositoryPlugin.REPOSITORY_PLUGIN_BUILTIN}"]`)
|
||||
expect(wrapper).not.toMatch(/\b(?:import|from)\s/)
|
||||
// Import-free means no static AND no dynamic imports; `import.meta.url`
|
||||
// (no whitespace, no call parenthesis) is the one allowed appearance.
|
||||
expect(wrapper).not.toMatch(/\b(?:import|from)\s|\bimport\s*\(/)
|
||||
await expect(readFile(join(directory, 'dsh-plugin-assets/skills/0/repository-fixture/SKILL.md'), 'utf8'))
|
||||
.resolves.toContain('Static instructions.')
|
||||
await expect(readFile(join(directory, 'dsh-plugin-assets/.mcp.json'), 'utf8'))
|
||||
@@ -218,6 +220,34 @@ describe('prepared repository plugin Loader composition', () => {
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('fails the plugin load when a declared skill root is missing or not a directory', async () => {
|
||||
const root = await temporaryDirectory('missing-skill-root')
|
||||
await writeFile(join(root, 'not-a-directory'), 'text')
|
||||
const ctx = new Context()
|
||||
ctx.baseUrl = pathToFileURL(root).href + '/'
|
||||
await ctx.plugin(Loader)
|
||||
await ctx.plugin(SkillService)
|
||||
await ctx.plugin(RepositoryPlugin)
|
||||
|
||||
for (const [filename, skillPath, message] of [
|
||||
['missing.mjs', 'dsh-plugin-assets/skills/0', 'skill root is missing from the installed package'],
|
||||
['file.mjs', 'not-a-directory', 'skill root is not a directory'],
|
||||
] as const) {
|
||||
const wrapper = join(root, filename)
|
||||
await writeFile(wrapper, [
|
||||
"export const inject = ['loader']",
|
||||
'export async function apply(ctx) {',
|
||||
` await ctx.plugin(ctx.loader.builtins['${RepositoryPlugin.REPOSITORY_PLUGIN_BUILTIN}'], {`,
|
||||
` baseUrl: import.meta.url, manifest: { name: 'damaged', skills: [${JSON.stringify(skillPath)}] },`,
|
||||
' })',
|
||||
'}',
|
||||
'',
|
||||
].join('\n'))
|
||||
await expect(ctx.loader.create({ name: pathToFileURL(wrapper).href })).rejects.toThrow(message)
|
||||
}
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('rejects duplicate builtin ownership and preserves a later replacement on teardown', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(Loader)
|
||||
|
||||
Reference in New Issue
Block a user