fix: address ds-review-bot round — insert-aliasing clones, settlement gates, closure module fallback
- Clone patch lists per generation (boot + composeLive): the include pushes insert rows by reference and mutates them in place, so a reused object baked user overrides into bundle rows and removal could not revert; the built-bin hot-reload e2e now asserts an override AND its removal reverting. - The headless runner awaits Loader settlement before prompting (its inject gate covers only apiProxy/httpServer) and abandons cleanly when the tree died during the wait. - healProfilesModuleFallback walks the app's full dependency+peer closure: out-of-tree plugins import seam packages (dsh-compact, dsh-subprocess, ...) that only implementations reach, and peers are how seams are declared. - Profile init writes pnpm-workspace.yaml (nodeLinker: hoisted), not .npmrc — pnpm >=10 reads settings from the workspace manifest. - Web dumps reject boot-only flags instead of printing a tree that differs from the same invocation's boot; --port validates at the flag; --dump-default-config no longer parses the (possibly broken) user layer; trustedHosts flag derivation merges over the composed value instead of replacing it; web-runtime gains surfaceContext (headless disables the GUI prompt/bash-vars the old -p never mounted); 'node_modules' is a reserved profile name; plugin-warning names the recovery step; client AGENTS.md registration surfaces point at the web-app bundle. - Ship session-reference/tmux-context/tool-ask-user as app dependencies for terminal front-door patch layers (turtle-ui), same stance as mcp-client.
This commit is contained in:
@@ -24,6 +24,9 @@
|
||||
"@deepseek-ai/dsh-paths": "workspace:^",
|
||||
"@deepseek-ai/dsh-pty": "workspace:^",
|
||||
"@deepseek-ai/dsh-pty-local": "workspace:^",
|
||||
"@deepseek-ai/dsh-session-reference": "workspace:^",
|
||||
"@deepseek-ai/dsh-tmux-context": "workspace:^",
|
||||
"@deepseek-ai/dsh-tool-ask-user": "workspace:^",
|
||||
"@deepseek-ai/dsh-tool-bash-persistent": "workspace:^",
|
||||
"@deepseek-ai/dsh-tool-cordis": "workspace:^",
|
||||
"@deepseek-ai/dsh-web-app": "workspace:^",
|
||||
|
||||
@@ -168,9 +168,19 @@ Examples:
|
||||
if (defaultOnly && patches.length > 0) {
|
||||
program.error('error: --dump-default-config prints the bundle layers and takes no --patch')
|
||||
}
|
||||
// The dump is boot-free and does not derive flag patches; silently
|
||||
// dropping them would print a tree that differs from the same
|
||||
// invocation's boot.
|
||||
if (options.host !== undefined || options.port !== undefined || options.dev === true
|
||||
|| options.workspaceRoot !== undefined || options.trustedHost !== undefined) {
|
||||
program.error('error: config dumps take no web flags (--host/--port/--dev/--workspace-root/--trusted-host)')
|
||||
}
|
||||
resolved = { mode: 'dump-config', profile: 'web', defaultOnly, patches }
|
||||
return
|
||||
}
|
||||
if (options.port !== undefined && !/^\d+$/.test(options.port)) {
|
||||
program.error(`error: --port must be a number, got ${JSON.stringify(options.port)}`)
|
||||
}
|
||||
resolved = {
|
||||
mode: 'web',
|
||||
patches,
|
||||
|
||||
@@ -29,7 +29,10 @@ const NAME = 'dsh'
|
||||
*/
|
||||
export function runDumpConfig(profile: string, defaultOnly: boolean, patches: readonly string[]): void {
|
||||
healProfilesModuleFallback(INSTALL_ANCHOR)
|
||||
const loaded = loadProfile(NAME, profile, INSTALL_ANCHOR)
|
||||
// The default dump never reads the user layer: it doubles as the recovery
|
||||
// diagnostic for a broken cordis.patch.yml, so parsing that file here would
|
||||
// defeat its purpose.
|
||||
const loaded = loadProfile(NAME, profile, INSTALL_ANCHOR, undefined, { userLayer: !defaultOnly })
|
||||
const layers: ConfigDumpLayer[] = loaded.layers.map(layer => ({
|
||||
label: layer.packageName,
|
||||
patches: layer.patches,
|
||||
|
||||
@@ -57,7 +57,10 @@ function reconcilePlugins(before: ProfileManifest, profileDir: string): void {
|
||||
for (const packageName of afterDeps) {
|
||||
if (beforeDeps.has(packageName) || plugins.includes(packageName)) continue
|
||||
if (!exportsPatch(packageName, profileDir)) {
|
||||
process.stderr.write(`${NAME}: warning: ${packageName} declares no dsh.patch — installed as a plain dependency, not a profile layer\n`)
|
||||
process.stderr.write(
|
||||
`${NAME}: warning: ${packageName} declares no dsh.patch — installed as a plain dependency, not a profile layer `
|
||||
+ '(if it gains one later, add it to dsh.plugins in the profile\'s package.json)\n',
|
||||
)
|
||||
continue
|
||||
}
|
||||
plugins.push(packageName)
|
||||
|
||||
@@ -186,15 +186,22 @@ export async function runProfile(options: RunProfileOptions): Promise<{ ctx: Con
|
||||
composed.profile.layers.reduce((n, layer) => n + layer.patches.length, 0)
|
||||
+ composed.profile.patches.length,
|
||||
)
|
||||
const composeLive = (profilePatches: PatchOptions[]): PatchOptions[] => [
|
||||
// Fresh clones per generation: the include pushes `insert` rows into the
|
||||
// mounted tree BY REFERENCE and later id-targeted patches mutate those
|
||||
// objects in place. Reusing one parsed patch object across applications
|
||||
// would bake a user override into the bundle's in-memory insert row, so
|
||||
// removing the override could never revert the row to the bundle default.
|
||||
const composeLive = (profilePatches: PatchOptions[]): PatchOptions[] => structuredClone([
|
||||
...composed.profile.layers.flatMap(layer => layer.patches),
|
||||
...profilePatches,
|
||||
...overlayAndFlags,
|
||||
]
|
||||
])
|
||||
// One-shot runs exit through the runner; watching would only hold the
|
||||
// process open after its exit request.
|
||||
const watchProfilePatch = options.task === undefined
|
||||
const ctx = await boot(NAME, rootConfig, composed.patches, async (hostCtx) => {
|
||||
// Cloned for the same insert-aliasing reason as composeLive: the boot
|
||||
// application must not mutate the objects later reloads recompose from.
|
||||
const ctx = await boot(NAME, rootConfig, structuredClone(composed.patches), async (hostCtx) => {
|
||||
app.current = hostCtx
|
||||
if (options.task !== undefined) {
|
||||
const io: HeadlessIo = {
|
||||
|
||||
@@ -85,7 +85,15 @@ function deriveWebFlagPatches(
|
||||
if (flags.workspaceRoot !== undefined) put('api-gateway', 'workspaceRoot', flags.workspaceRoot)
|
||||
const composedHost = (rows.get('webserver')?.config as { host?: string } | undefined)?.host
|
||||
const { lanAddresses, trustedHosts } = resolveLanTrust(flags.host ?? composedHost, flags.trustedHosts ?? [])
|
||||
if (trustedHosts.length > 0) put('connection', 'trustedHosts', trustedHosts)
|
||||
if (trustedHosts.length > 0) {
|
||||
// Additive over the composed value: a cordis.patch.yml-configured fence
|
||||
// authority must survive the derived LAN literals and flag extras — a
|
||||
// silent drop of security-relevant fence configuration.
|
||||
const composedTrusted = (rows.get('connection')?.config as { trustedHosts?: string[] } | undefined)?.trustedHosts ?? []
|
||||
put('connection', 'trustedHosts', [...composedTrusted, ...trustedHosts])
|
||||
}
|
||||
// mode and lanAddresses are launcher-derived on every boot (--dev also
|
||||
// inserts the client-hmr row), never pass-throughs of composed values.
|
||||
put('web-runtime', 'mode', flags.dev ? 'development' : 'production')
|
||||
put('web-runtime', 'lanAddresses', lanAddresses)
|
||||
const patches = [...overrides.entries()].map(([id, bag]): PatchOptions => {
|
||||
@@ -98,9 +106,11 @@ function deriveWebFlagPatches(
|
||||
}
|
||||
|
||||
/**
|
||||
* Serve the browser UI from the web profile. Flags are passed through only
|
||||
* when given; absent, the composed profile values stand. The URL line is
|
||||
* printed by the web-app bundle's runtime row after Loader settlement.
|
||||
* Serve the browser UI from the web profile. Host/port/workspace-root flags
|
||||
* are passed through only when given (absent, the composed profile values
|
||||
* stand); `web-runtime.mode` and `lanAddresses` are launcher-derived on
|
||||
* every boot. The URL line is printed by the web-app bundle's runtime row
|
||||
* after Loader settlement.
|
||||
* @param flags - the parsed `dsh web` flag family.
|
||||
*/
|
||||
export async function runWeb(flags: WebFlags): Promise<void> {
|
||||
|
||||
@@ -76,6 +76,12 @@ describe('parseDshArgs', () => {
|
||||
expect(exitCode(['web', '--dump-config', '--dump-default-config'])).toBe(1)
|
||||
expect(exitCode(['web', '--dump-default-config', '--patch', 'w.yml'])).toBe(1)
|
||||
expect(exitCode(['web', '--patch='])).toBe(1)
|
||||
// Boot-free dumps derive no flag patches; silently dropping the flags
|
||||
// would print a tree that differs from the same invocation's boot.
|
||||
expect(exitCode(['web', '--dump-config', '--port', '8080'])).toBe(1)
|
||||
expect(exitCode(['web', '--dump-config', '--dev'])).toBe(1)
|
||||
// A non-numeric port fails at the flag, not deep in the webserver schema.
|
||||
expect(exitCode(['web', '--port', 'abc'])).toBe(1)
|
||||
expect(exitCode(['plugin', 'add', 'x'])).toBe(1) // --profile required
|
||||
expect(exitCode(['plugin', '--profile', 'tui'])).toBe(1) // nothing to forward
|
||||
expect(exitCode(['plugin', '--profile', ''])).toBe(1)
|
||||
|
||||
@@ -55,11 +55,15 @@ function createProfileLifecycleFixture(): ProfileLifecycleFixture {
|
||||
mkdirSync(bundleDir, { recursive: true })
|
||||
writeFileSync(join(bundleDir, 'plugin.mjs'), [
|
||||
"import { writeFileSync } from 'node:fs'",
|
||||
"import { join } from 'node:path'",
|
||||
"export const name = 'profile-lifecycle-fixture'",
|
||||
'export function apply(ctx) {',
|
||||
'export function apply(ctx, config = {}) {',
|
||||
' let active = true',
|
||||
' // Keep the event loop alive so process lifetime is signal-owned, like a real surface.',
|
||||
' const heartbeat = setInterval(() => {}, 1000)',
|
||||
' // Echo the mounted generation so the hot-reload e2e can assert both an',
|
||||
' // applied override and its removal reverting to this bundle default.',
|
||||
" writeFileSync(join(process.env.DSH_HOME, 'config-echo'), String(config.generation ?? 'bundle-default'))",
|
||||
" writeFileSync(process.env.RAW_READY_FILE, 'ready')",
|
||||
' void ctx.loader.await().then(() => {',
|
||||
" if (active) writeFileSync(process.env.RAW_SETTLED_FILE, 'settled')",
|
||||
@@ -166,24 +170,36 @@ describe.skipIf(!existsSync(dshBin))('dsh BUILT bin (node lib/bin.js, no tsx)',
|
||||
}
|
||||
}, 30_000)
|
||||
|
||||
it('fully settles a custom profile, hot-reloads its patch layer, and disposes on a signal', async () => {
|
||||
it('fully settles a custom profile, hot-reloads its patch layer with removal reverting, and disposes on a signal', async () => {
|
||||
const fixture = createProfileLifecycleFixture()
|
||||
const child = startProfileLifecycle(fixture)
|
||||
const profilePatch = join(fixture.home, 'profiles', 'lifecycle', 'cordis.patch.yml')
|
||||
const configFile = join(fixture.home, 'config-echo')
|
||||
try {
|
||||
await waitForFile(fixture.settled)
|
||||
// The live profile layer: even without an hmr row in the composition,
|
||||
// the launcher mounts a config-only watcher, so an edited
|
||||
// cordis.patch.yml lands in the running tree (the reload disposes the
|
||||
// patched row's old fiber — observable as the disposed marker — and
|
||||
// mounts the new config, which re-writes the ready marker).
|
||||
// mounts the new config, which echoes its generation and re-writes the
|
||||
// ready marker).
|
||||
rmSync(fixture.ready)
|
||||
writeFileSync(join(fixture.home, 'profiles', 'lifecycle', 'cordis.patch.yml'), [
|
||||
writeFileSync(profilePatch, [
|
||||
'- id: profile-lifecycle-fixture',
|
||||
' config:',
|
||||
' generation: 2',
|
||||
'',
|
||||
].join('\n'))
|
||||
await waitForFile(fixture.ready)
|
||||
expect(readFileSync(configFile, 'utf8')).toBe('2')
|
||||
// Removal reverts: the bundle's inserted row must return to its own
|
||||
// default config, not keep the removed override — the insert-aliasing
|
||||
// regression (a shared patch object mutated in place by a former
|
||||
// generation would make this impossible).
|
||||
rmSync(fixture.ready)
|
||||
writeFileSync(profilePatch, '[]\n')
|
||||
await waitForFile(fixture.ready)
|
||||
expect(readFileSync(configFile, 'utf8')).toBe('bundle-default')
|
||||
child.kill('SIGTERM')
|
||||
const result = await child
|
||||
expect(result.exitCode).toBe(0)
|
||||
|
||||
Reference in New Issue
Block a user