fix(review): audit prepared-wrapper activation and pin generated shapes
ds-review-bot round 1 on the DSH-home integration: - generated wrappers now inject the services their manifest needs (skills/ tools beside loader), and loadPreparedRepository rejects a wrapper fiber that settles anything but ACTIVE — a composition missing a required service fails the repository transaction instead of committing an ACTIVE row over a silently PENDING child (critical finding) - the github: source ref segment excludes '#', so 'a#b' refs fail at the config parser with the promised syntax instead of inside pnpm - watchPersonalPatches re-reads the include's non-patch options per refresh instead of a registration-time snapshot - the TUI smoke's cache-seeded wrapper is produced by the real prepareDshPlugin (cache LAYOUT stays a deliberate external pin) - new Loader integration test drives a live repositories update through entry.update: generation swap, old skills removed, failed candidate rolled back to the previous generation
This commit is contained in:
@@ -5,15 +5,23 @@
|
||||
|
||||
import { join, resolve } from 'node:path'
|
||||
import { pathToFileURL } from 'node:url'
|
||||
import type { Context, Fiber, Plugin } from 'cordis'
|
||||
import type { Context, Fiber, FiberState, Plugin } from 'cordis'
|
||||
import type { RepositoryCache } from '@cordisjs/plugin-loader/repository'
|
||||
import { resolveDshHome } from '@deepseek-ai/dsh-paths'
|
||||
import { PREPARED_ENTRY_FILENAME } from './format.ts'
|
||||
|
||||
// Value mirror: Cordis's const enum has no runtime object to import. Keep
|
||||
// aligned with `packages/cordis/tool-cordis/src/fiber-state.ts`.
|
||||
const FIBER_ACTIVE = 2 as FiberState.ACTIVE
|
||||
|
||||
/** Directory under the Harness home containing immutable repository generations. */
|
||||
export const DEFAULT_REPOSITORY_CACHE_DIRECTORY = 'repository-plugins'
|
||||
|
||||
const GITHUB_SOURCE_PATTERN = /^github:([^/\s#&]+)\/([^/\s#&]+)#([^\s&]+)(?:&path:(\/[^\s&]+))?$/
|
||||
// The ref segment excludes `#` so `github:o/r#a#b` fails here — at the config
|
||||
// parser, with the syntax the error message promises — instead of inside the
|
||||
// cache's pnpm install ('misconfiguration fails loud at the earliest
|
||||
// resolvable point').
|
||||
const GITHUB_SOURCE_PATTERN = /^github:([^/\s#&]+)\/([^/\s#&]+)#([^\s#&]+)(?:&path:(\/[^\s&]+))?$/
|
||||
|
||||
function validPluginPath(path: string): boolean {
|
||||
const segments = path.split('/').slice(1)
|
||||
@@ -67,6 +75,18 @@ export async function loadPreparedRepository(
|
||||
try {
|
||||
const plugin = await import(/* @vite-ignore */pathToFileURL(filename).href) as Plugin
|
||||
const fiber = ctx.plugin(plugin)
|
||||
await fiber
|
||||
// Awaiting a service-gated fiber returns while it is still PENDING (the
|
||||
// generated wrapper injects `skills`/`tools` per its manifest). This
|
||||
// runtime commits the repository configuration transactionally, so a
|
||||
// composition that never provides a required service must reject the
|
||||
// transaction here — not settle ACTIVE with a silently pending child.
|
||||
if (fiber.state !== FIBER_ACTIVE) {
|
||||
const missing = Object.keys(fiber.inject).filter(service => fiber.ctx.get(service) === undefined)
|
||||
/* v8 ignore next 2 -- the 'unknown' arm needs a service to appear after the state read; not deterministically stageable. */
|
||||
const detail = missing.join(', ') || 'unknown'
|
||||
throw new Error(`prepared wrapper did not activate (waiting for services: ${detail})`)
|
||||
}
|
||||
return await fiber
|
||||
} catch (cause) {
|
||||
throw new Error(`failed to load prepared repository Plugin ${JSON.stringify(specifier)} from ${filename}`, { cause })
|
||||
|
||||
@@ -296,6 +296,7 @@ describe('configured GitHub repository sources', () => {
|
||||
for (const source of [
|
||||
'github:owner/repository',
|
||||
'github:owner/repository#',
|
||||
'github:owner/repository#a#b',
|
||||
'https://github.com/owner/repository#ref',
|
||||
'github:owner/repository#ref&path:relative/.dsh-plugin',
|
||||
]) {
|
||||
@@ -350,6 +351,52 @@ describe('configured GitHub repository sources', () => {
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('swaps generations on a live source-list update and rolls a failed candidate back', async () => {
|
||||
// The headline flow: a personal-config edit reaches this plugin as a
|
||||
// Loader entry.update, which restarts the row's fiber (old cleanup, then
|
||||
// new apply — so the 'already registered' builtin guard must not fire).
|
||||
const roots: Record<string, string> = {}
|
||||
for (const generation of ['one', 'two'] as const) {
|
||||
const root = await temporaryDirectory(`live-${generation}`)
|
||||
await writeSkill(join(root, 'skills'), `live-skill-${generation}`)
|
||||
const directory = await writePlugin(root, `live-fixture-${generation}`, { skills: ['../skills'] })
|
||||
await RepositoryPlugin.prepareDshPlugin(directory)
|
||||
roots[`github:owner/repository#${generation}&path:/.dsh-plugin`] = directory
|
||||
}
|
||||
vi.spyOn(RepositoryCache.prototype, 'resolve').mockImplementation(async (specifier) => {
|
||||
const directory = roots[specifier]
|
||||
if (directory === undefined) throw new Error(`unprepared generation ${specifier}`)
|
||||
return directory
|
||||
})
|
||||
|
||||
// Route the row through the Loader builtin table exactly as a config tree
|
||||
// would; the module itself is the row's plugin.
|
||||
const ctx2 = new Context()
|
||||
await ctx2.plugin(Loader)
|
||||
await ctx2.plugin(SkillService)
|
||||
ctx2.loader.builtins['repository-plugins'] = RepositoryPlugin
|
||||
const entryId = await ctx2.loader.create({
|
||||
name: 'cordis:repository-plugins',
|
||||
config: { repositories: ['github:owner/repository#one'] },
|
||||
})
|
||||
await ctx2.loader.await()
|
||||
await expect(ctx2.skills.get('live-skill-one')).resolves.toMatchObject({ provider: 'repository:live-fixture-one' })
|
||||
|
||||
const entry = ctx2.loader.resolve(entryId)
|
||||
await entry.update({ config: { repositories: ['github:owner/repository#two'] } })
|
||||
await ctx2.loader.await()
|
||||
await expect(ctx2.skills.get('live-skill-one')).resolves.toBeUndefined()
|
||||
await expect(ctx2.skills.get('live-skill-two')).resolves.toMatchObject({ provider: 'repository:live-fixture-two' })
|
||||
|
||||
// A failed candidate (unprepared source) rejects the update and the
|
||||
// transactional Loader restores the previous generation.
|
||||
await expect(entry.update({ config: { repositories: ['github:owner/repository#missing'] } }))
|
||||
.rejects.toThrow('unprepared generation')
|
||||
await ctx2.loader.await()
|
||||
await expect(ctx2.skills.get('live-skill-two')).resolves.toMatchObject({ provider: 'repository:live-fixture-two' })
|
||||
await ctx2.fiber.dispose()
|
||||
})
|
||||
|
||||
it('rejects duplicate generations and cleans the builtin after cache preparation fails', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(Loader)
|
||||
@@ -368,6 +415,28 @@ describe('configured GitHub repository sources', () => {
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('rejects a wrapper left pending by a composition without its required services', async () => {
|
||||
// A skills-declaring generation mounted where no skills service exists:
|
||||
// the wrapper fiber stays PENDING, and the transaction must fail loud
|
||||
// instead of committing an ACTIVE row over a silently inert child.
|
||||
const root = await temporaryDirectory('pending-services')
|
||||
await writeSkill(join(root, 'skills'), 'pending-service-skill')
|
||||
const directory = await writePlugin(root, 'pending-service-fixture', { skills: ['../skills'] })
|
||||
await RepositoryPlugin.prepareDshPlugin(directory)
|
||||
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(Loader)
|
||||
// Deliberately NO SkillService.
|
||||
await expect(loadPreparedRepository(ctx, { resolve: async () => directory }, 'github:owner/repository#pending&path:/.dsh-plugin'))
|
||||
.rejects.toMatchObject({
|
||||
message: expect.stringContaining('failed to load prepared repository Plugin') as string,
|
||||
cause: expect.objectContaining({
|
||||
message: expect.stringContaining('waiting for services: skills') as string,
|
||||
}) as Error,
|
||||
})
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('labels a missing prepared wrapper with its exact source and path', async () => {
|
||||
const root = await temporaryDirectory('missing-wrapper')
|
||||
const ctx = new Context()
|
||||
|
||||
@@ -323,8 +323,11 @@ export async function watchPersonalPatches(
|
||||
const entry = bootstrapIncludes.get(ctx)
|
||||
if (entry === undefined) throw new Error(`${binName}: personal config watching requires the root Include entry`)
|
||||
const filename = join(dir, PERSONAL_CONFIG_FILENAME)
|
||||
const { patches: _initialPatches, ...includeConfig } = entry.options.config as Include.Config
|
||||
return hmr.registerConfig(filename, async () => {
|
||||
// Re-read the include's non-patch options per refresh: a writer that
|
||||
// updates the root Include's other options between refreshes (none exists
|
||||
// today) must not have them silently reverted by a personal reload.
|
||||
const { patches: _previousPatches, ...includeConfig } = entry.options.config as Include.Config
|
||||
const personalPatches = loadPersonalPatches(binName, dir) ?? []
|
||||
const patches = compose(personalPatches)
|
||||
await entry.update({
|
||||
|
||||
Reference in New Issue
Block a user