fix review findings: harden the app bins + built-bin smokes, arch-exception doc, snapshot fixture-guard
BLOCKER — the published lib/bin.js (stdio + acp) was exercised only via tsx
(demo:* / the src/bin.ts smokes); the built artifact under plain `node` was
unguarded. Root-cause on the BUILT bin:
1. Settle race: boot() returned once loader.create() registered the include
ENTRY, but the include loads its child plugins asynchronously — so boot()
(and main()) resolved while the app plugins (stdin reader, agent loop, ACP
bridge) were still mounting. A CLI with no attached handles yet exits 0
silently, and a load error surfaces as an unhandled rejection AFTER boot.
Fix: `await ctx.loader.await()` after create() — settle the whole tree.
2. Config-path robustness: hand the include the config's ABSOLUTE file:// URL
so resolution never depends on ctx.baseUrl / can never fall back to cwd.
Both bins fixed identically. NOTE: the cordis Loader resolves a config's bare
plugin specifiers via its internal module loader, active only under
`node --expose-internals`; the bin cannot add a node flag itself, so this is
documented in the bin JSDoc + both package READMEs (the demos already comply).
The repo `examples/*/cordis.yml` are tsx-only artifacts (workspace plugins
resolve through the tsconfig paths map, not node_modules), so they are not a
valid plain-node bin target — the smokes use a real-install-shaped temp dir.
Fail loud on a load failure: boot() previously exited 0 SILENTLY when a config
path's directory does not exist — the include plugin fails to IMPORT, the cordis
Loader catches+LOGS it and leaves the entry with no fiber (no rejection), and
`loader.await()` does not rethrow (EntryTree.await uses Promise.allSettled). Fix:
boot() now calls assertEntriesLoaded(ctx) after the tree settles and throws on
any entry with no fiber, so a typo'd config dir exits non-zero with a clear
message. main() also installs an unhandledRejection guard (installFailLoud) that
replaces Node's stack dump with a single labelled stderr line for the
companion case (a missing config FILE in a real dir, whose include-init throw
surfaces as a rejection Node already exits non-zero on). Regression tests added
to both built-bin smokes (missing dir + missing file → non-zero exit + stderr);
verified the missing-dir test fails on the pre-fix bin.
Built-bin smokes (the reviewer's ask): packages/ui/{stdio,acp}-agent/tests/
built-bin.e2e.ts run the REAL lib/bin.js under `node` (NOT tsx) in a temp
consumer dir, asserting the stdio echo round-trip / the acp initialize response
+ stdout purity, plus the fail-loud cases above. They build-gate (skip if lib/
absent) and run in a new ci.yml step after the build.
Issue 2 — packages/README.md + docs/architecture.md said "plugins depend on
interfaces, never on the concrete loop", but dsh-agent-core imports the concrete
dsh-agent-loop. Scope the rule to EXTENSION plugins and carve out the sanctioned
COMPOSITION/bundle exception (dsh-agent-core composes the concrete spine); note
it in the implemented RFC too.
Issue 3 — examples/acp-agent/tests/acp.snapshot.ts fixture-guard claimed
no-model scenarios need no session.jsonl, but runScenario() always boots
llm-replay with the session.jsonl path and loadReplayScript() throws when it is
absent. Require session.jsonl for ALL scenarios (no-model ones ship a
header-only fixture) and rewrite the comment to match reality.
This commit is contained in:
@@ -13,7 +13,7 @@
|
||||
*/
|
||||
|
||||
import { pathToFileURL } from 'node:url'
|
||||
import { basename, dirname, resolve } from 'node:path'
|
||||
import { dirname, resolve } from 'node:path'
|
||||
import { Context } from 'cordis'
|
||||
import Loader from '@cordisjs/plugin-loader'
|
||||
|
||||
@@ -38,10 +38,72 @@ function loadEnv(): void {
|
||||
}
|
||||
|
||||
/**
|
||||
* Boot the Loader against `configPath` (resolved from the CWD). `baseUrl` is
|
||||
* pinned to the config's directory and the include is handed only the basename,
|
||||
* so the config's relative plugin/include paths resolve exactly as the upstream
|
||||
* `cordis` bin does. Returns the root context (the process owns its lifetime).
|
||||
* Make a load failure fail loud with a clear message on stderr. Covers the
|
||||
* failure path the entry-tree check below cannot: when the include's
|
||||
* `[Service.init]` throws (e.g. a config FILE that does not exist in a real
|
||||
* directory), the cordis Loader surfaces it as an unhandled promise rejection
|
||||
* AFTER `boot()` has resolved — `loader.await()` does NOT rethrow it, because
|
||||
* `EntryTree.await()` uses `Promise.allSettled`, which swallows rejections.
|
||||
* Node's default handler already exits non-zero on an unhandled rejection, so
|
||||
* this does not change the exit code; it replaces Node's noisy stack dump with a
|
||||
* single labelled line and guarantees `process.exit(1)`. Install before `boot()`.
|
||||
*/
|
||||
export function installFailLoud(): void {
|
||||
process.on('unhandledRejection', (err: unknown) => {
|
||||
process.stderr.write(`dsh-stdio-agent: fatal load failure: ${err instanceof Error ? err.stack ?? err.message : String(err)}\n`)
|
||||
process.exit(1)
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* After the tree settles, assert every loader entry actually started. This is
|
||||
* the load-bearing guard against the SILENT-exit-0 bug: when a plugin module
|
||||
* fails to IMPORT (e.g. a config path in a non-existent directory, so the include
|
||||
* plugin itself cannot be resolved), the cordis Loader catches the import error
|
||||
* and only LOGS it (`entry._init`), leaving the entry with no `fiber` and
|
||||
* producing no rejection — so the process would otherwise exit 0 with a usable
|
||||
* config typo reported only as a log line. A started entry has a `fiber`; an
|
||||
* 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.
|
||||
*/
|
||||
function assertEntriesLoaded(ctx: Context): void {
|
||||
const failed = [...ctx.loader.entries()].filter(entry => entry.fiber === undefined)
|
||||
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)`)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Boot the Loader against `configPath` (resolved from the CWD). The include is
|
||||
* handed the config's ABSOLUTE `file://` URL as its `path`, so resolution never
|
||||
* depends on `ctx.baseUrl` (an absolute URL ignores the base) and can never fall
|
||||
* back to the cwd. `baseUrl` is still pinned to the config's directory so the
|
||||
* config's OWN relative plugin/include paths (e.g. `./src/mock-llm.ts`) resolve
|
||||
* against it. Returns the root context once the whole tree has settled.
|
||||
*
|
||||
* The `await ctx.loader.await()` is load-bearing: `loader.create()` returns once
|
||||
* the include ENTRY is registered, but the include then loads its child plugins
|
||||
* asynchronously. Without awaiting the tree, `boot()` (and `main()`) would
|
||||
* resolve while the app plugins — the stdin reader, the agent loop — are still
|
||||
* mounting, and a CLI process with no attached handles yet exits 0 silently.
|
||||
* Awaiting the tree keeps the process alive until the app's handles are attached.
|
||||
*
|
||||
* `loader.await()` does NOT, however, rethrow load errors (`EntryTree.await()`
|
||||
* uses `Promise.allSettled`), so failures are surfaced two ways: a plugin that
|
||||
* fails to IMPORT leaves an entry with no fiber, caught here by
|
||||
* {@link assertEntriesLoaded} (this `boot()` rejects); a plugin whose init
|
||||
* THROWS surfaces as an unhandled rejection caught by {@link installFailLoud}
|
||||
* (installed by `main()` before this runs). Together they make any load failure
|
||||
* exit non-zero with a clear message.
|
||||
*
|
||||
* Bare plugin specifiers in the config (`@deepseek-ai/dsh-*`, npm packages) are
|
||||
* resolved by the cordis Loader's internal module loader, which is only active
|
||||
* under `node --expose-internals` (the flag the `demo:echo`/`demo:coding` scripts
|
||||
* pass). Without it the Loader falls back to resolving relative to its own module
|
||||
* and cannot find the config's plugins, so a consumer running the built bin must
|
||||
* pass `--expose-internals` (or install the plugins where node hoists them).
|
||||
*/
|
||||
export async function boot(configPath: string): Promise<Context> {
|
||||
const absolute = resolve(process.cwd(), configPath)
|
||||
@@ -50,17 +112,20 @@ export async function boot(configPath: string): Promise<Context> {
|
||||
await ctx.plugin(Loader)
|
||||
await ctx.loader.create({
|
||||
name: '@cordisjs/plugin-include',
|
||||
config: { path: `./${basename(absolute)}` },
|
||||
config: { path: pathToFileURL(absolute).href },
|
||||
})
|
||||
await ctx.loader.await()
|
||||
assertEntriesLoaded(ctx)
|
||||
return ctx
|
||||
}
|
||||
|
||||
/**
|
||||
* Entry point: load `.env`, then boot the config named on argv (default
|
||||
* `./cordis.yml`). Awaited at the module top level by the published bin
|
||||
* (`#!/usr/bin/env node` shebang via the package's `bin` field).
|
||||
* Entry point: install the fail-loud guard, load `.env`, then boot the config
|
||||
* named on argv (default `./cordis.yml`). Awaited at the module top level by the
|
||||
* published bin (`#!/usr/bin/env node` shebang via the package's `bin` field).
|
||||
*/
|
||||
export async function main(argv: string[] = process.argv.slice(2)): Promise<void> {
|
||||
installFailLoud()
|
||||
loadEnv()
|
||||
await boot(argv[0] ?? './cordis.yml')
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user