fix review findings: don't treat disabled entries as load failures; scope the verify-package-paths lib skip to a real package root
assertEntriesLoaded() flagged ANY fiber-less entry as a failed import, but a `disabled: true` entry settles without a fiber by design (Entry.refresh() skips init() when disabled) — a valid "plugin off" config, not a broken import. Both app bins now filter `fiber === undefined && !entry.disabled`. The stdio built-bin smoke gains a disabled-(unresolvable)-entry config that must still boot. The verify-package-paths lib-skip was unconditional and ran before the moved-package check, so a stale group-less `packages/acp-agent/lib/bin.js` (the exact drift this gate catches) was silently ignored just for containing `lib`. Scope the skip: only exempt `lib` when it is the segment after an EXISTING `packages/<group>/<pkg>` root, so a real-but-unbuilt `lib/bin.js` is still exempt while a stale package path flags.
This commit is contained in:
@@ -85,9 +85,13 @@ export function installFailLoud(): void {
|
||||
* `fiber` and producing no rejection — so the process would otherwise exit 0. A
|
||||
* started entry has a `fiber`; throw on any entry still missing one so `boot()`
|
||||
* rejects.
|
||||
*
|
||||
* A `disabled` entry is the one legitimate fiber-less state: `Entry.refresh()`
|
||||
* deliberately skips `init()` for it, so it settles without a fiber by design —
|
||||
* a valid "plugin turned off" config, not a failed import. Exclude it.
|
||||
*/
|
||||
function assertEntriesLoaded(ctx: Context): void {
|
||||
const failed = [...ctx.loader.entries()].filter(entry => entry.fiber === undefined)
|
||||
const failed = [...ctx.loader.entries()].filter(entry => entry.fiber === undefined && !entry.disabled)
|
||||
if (failed.length > 0) {
|
||||
const names = failed.map(entry => entry.options.name).join(', ')
|
||||
throw new Error(`dsh-acp-agent: plugin(s) failed to load: ${names} (see the error(s) logged above)`)
|
||||
|
||||
@@ -66,9 +66,14 @@ export function installFailLoud(): void {
|
||||
* entry with `fiber === undefined` after the tree settled never loaded. Throw on
|
||||
* any such entry so `boot()` rejects (and the top-level `await` fails the process
|
||||
* non-zero) instead of returning a half-empty context.
|
||||
*
|
||||
* A `disabled` entry is the one legitimate fiber-less state: `Entry.refresh()`
|
||||
* deliberately skips `init()` for it, so it settles without a fiber by design.
|
||||
* That is a valid config (a consumer turning an optional plugin off), not a
|
||||
* failed import — exclude it so the guard catches only real load failures.
|
||||
*/
|
||||
function assertEntriesLoaded(ctx: Context): void {
|
||||
const failed = [...ctx.loader.entries()].filter(entry => entry.fiber === undefined)
|
||||
const failed = [...ctx.loader.entries()].filter(entry => entry.fiber === undefined && !entry.disabled)
|
||||
if (failed.length > 0) {
|
||||
const names = failed.map(entry => entry.options.name).join(', ')
|
||||
throw new Error(`dsh-stdio-agent: plugin(s) failed to load: ${names} (see the error(s) logged above)`)
|
||||
|
||||
@@ -53,8 +53,13 @@ async function pkgName(absDir: string): Promise<string> {
|
||||
* Build a temp consumer dir: `node_modules` with the workspace + vendor packages
|
||||
* symlinked in, a `src/` carrying the example mock backend, and a `cordis.yml`
|
||||
* that wires them onto the stdio app. Returns the dir (caller removes it).
|
||||
*
|
||||
* `disabledBrokenEntry` appends an entry that points at a non-existent plugin but
|
||||
* is marked `disabled: true`. The Loader leaves a disabled entry fiber-less by
|
||||
* design, so it exercises that the fail-loud entry-load guard does NOT mistake a
|
||||
* valid disabled entry for a failed import.
|
||||
*/
|
||||
async function makeConsumer(welcome: string): Promise<string> {
|
||||
async function makeConsumer(welcome: string, disabledBrokenEntry = false): Promise<string> {
|
||||
const dir = await mkdtemp(join(tmpdir(), 'stdio-built-bin-'))
|
||||
const nm = join(dir, 'node_modules')
|
||||
for (const rel of dshPackages) {
|
||||
@@ -88,6 +93,9 @@ async function makeConsumer(welcome: string): Promise<string> {
|
||||
' model: mock-echo',
|
||||
' systemPrompt: \'demo\'',
|
||||
` welcome: '${welcome}'`,
|
||||
...disabledBrokenEntry
|
||||
? ['- id: off', ' name: \'./src/does-not-exist.ts\'', ' disabled: true']
|
||||
: [],
|
||||
'',
|
||||
].join('\n'))
|
||||
return dir
|
||||
@@ -143,6 +151,18 @@ describe.skipIf(!existsSync(stdioBin))('dsh-stdio-agent BUILT bin (node lib/bin.
|
||||
expect(code).toBe(0)
|
||||
}, 30_000)
|
||||
|
||||
it('boots cleanly when the config disables an (otherwise unresolvable) entry', async () => {
|
||||
// A `disabled: true` entry settles without a fiber by design; the fail-loud
|
||||
// entry-load guard must NOT mistake it for a failed import. Even though its
|
||||
// plugin path does not exist, the app boots and the round-trip works.
|
||||
consumer = await makeConsumer('DISABLED-OK ready.', true)
|
||||
const { stdout, code, stderr } = await runBuiltBin(consumer, './cordis.yml', 'echo hi')
|
||||
expect(stderr).not.toContain('failed to load')
|
||||
expect(stdout).toContain('DISABLED-OK ready.')
|
||||
expect(stdout).toContain('[tool result] ECHO: HI')
|
||||
expect(code).toBe(0)
|
||||
}, 30_000)
|
||||
|
||||
it('fails LOUD (non-zero exit + stderr) on a config whose directory does not exist', async () => {
|
||||
// A consumer who typos the config path must get a clear failure, not silent
|
||||
// success. This dir does not exist, so the include PLUGIN itself fails to
|
||||
|
||||
@@ -28,10 +28,13 @@
|
||||
* Scope mirrors the other doc gates plus repo-authored TypeScript: Markdown
|
||||
* across README/docs/packages/AGENTS, and `.ts` under packages/** and
|
||||
* examples/** (excluding built `lib/`, `*.d.ts`, and vendored upstream source).
|
||||
* A reference whose target path goes through a `lib/` segment is also skipped:
|
||||
* that is a build OUTPUT (`packages/ui/acp-agent/lib/bin.js`), emitted only by
|
||||
* `pnpm run build`, which CI runs AFTER this gate — flagging it would be a false
|
||||
* positive on a path that is correct but not yet on disk.
|
||||
* A reference to a package's build OUTPUT (`packages/<group>/<pkg>/lib/…`,
|
||||
* e.g. `packages/ui/acp-agent/lib/bin.js` cited by a built-bin smoke) is also
|
||||
* skipped — it is emitted only by `pnpm run build`, which CI runs AFTER this
|
||||
* gate, so flagging it would be a false positive on a path that is correct but
|
||||
* not yet on disk. That skip is scoped to a REAL package root: a stale
|
||||
* group-less `packages/acp-agent/lib/bin.js` is still flagged (its root does not
|
||||
* exist — exactly the moved-package drift this gate catches).
|
||||
*
|
||||
* Run: `tsx scripts/verify-package-paths.ts`.
|
||||
*/
|
||||
@@ -115,13 +118,17 @@ function findViolations(absPath: string): Violation[] {
|
||||
// class may have swallowed (`packages/core/tools.` / `…/tools/`).
|
||||
const ref = m[0].replace(/[./]+$/, '')
|
||||
if (existsSync(resolve(root, ref))) continue
|
||||
// A reference INTO a package's built `lib/` is a build-output path, not an
|
||||
// A reference INTO a package's built `lib/` is a build OUTPUT, not an
|
||||
// authored-source location: it does not exist until `pnpm run build` emits
|
||||
// it, and CI runs this gate BEFORE the build step. This gate reports stale
|
||||
// SOURCE paths (a moved package), so skip `lib/` targets the same way the
|
||||
// file scan excludes `lib/` files — a `packages/ui/acp-agent/lib/bin.js`
|
||||
// citation in a built-bin smoke is correct, just not yet on disk at lint.
|
||||
if (ref.split('/').includes('lib')) continue
|
||||
// it, and CI runs this gate BEFORE the build step. Skip it — but ONLY when
|
||||
// the `packages/<group>/<pkg>` ROOT it sits under is real and on disk, so
|
||||
// `packages/ui/acp-agent/lib/bin.js` (correct, just not yet built) is
|
||||
// exempt while a stale `packages/acp-agent/lib/bin.js` (group-less, the
|
||||
// exact moved-package drift this gate exists to catch) still flags. A bare
|
||||
// `lib` segment is not a blanket escape hatch.
|
||||
const parts = ref.split('/')
|
||||
const libAt = parts.indexOf('lib')
|
||||
if (libAt === 3 && existsSync(resolve(root, parts.slice(0, 3).join('/')))) continue
|
||||
// Only a stale path to a REAL (moved) package is a violation; a segment
|
||||
// matching a live package name is the drift signal.
|
||||
const segments = ref.split('/').slice(1)
|
||||
|
||||
Reference in New Issue
Block a user