From 7ba070eb1c276c651bd072d3474b0ebe73c10aa9 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Tue, 1 Sep 2026 21:09:41 +0000 Subject: [PATCH 1/2] fix(cli): worker-name collisions, negative positionals, fail-closed rendered workers, canonical result output Post-merge review findings from #186, #175, and #178: reserve generated *-flight.mjs worker destinations during entry planning so a conventional script colliding with a rendered worker is a build-time AB5000 diagnostic instead of a silent overwrite; treat a lone - token as a positional when the next positional expects a number (keeping -- escape and unknown single-dash rejection intact); fail all pending rendered requests closed when the worker exits with any code, including 0; route rejected progress reports into the request failure path instead of voiding them; serialize plain CLI results with the canonical stableJson writer (undefined emits null); and validate the exitCode result mapping before writing stdout so an invalid mapping cannot emit a success line first. The inlined cli-entry runtime now ignores its owning source root during builds because it pulls sibling core imports. --- .changeset/fix-rendered-cli-boundaries.md | 8 +++ packages/agent-bundle/src/build/entries.ts | 16 +++-- .../agent-bundle/src/build/entry-shell.ts | 6 +- .../agent-bundle/src/build/package-build.ts | 3 +- packages/agent-bundle/src/cli-entry.ts | 15 +++-- .../tests/cli-routes-build.test.ts | 17 ++++++ .../agent-bundle/tests/cli-routes.test.ts | 38 +++++++++++- packages/agent-bundle/tests/cli.test.ts | 35 +++++++++++ .../agent-bundle/tests/entry-shell.test.ts | 59 +++++++++++++++++++ 9 files changed, 183 insertions(+), 14 deletions(-) create mode 100644 .changeset/fix-rendered-cli-boundaries.md diff --git a/.changeset/fix-rendered-cli-boundaries.md b/.changeset/fix-rendered-cli-boundaries.md new file mode 100644 index 000000000..67d7f30eb --- /dev/null +++ b/.changeset/fix-rendered-cli-boundaries.md @@ -0,0 +1,8 @@ +--- +"agent-bundle": patch +--- + +Fail rendered CLI requests closed when their worker exits or progress +forwarding rejects, reserve generated Flight worker output names, accept +negative numeric positionals, and canonicalize command results only after +validating result-derived exit codes. diff --git a/packages/agent-bundle/src/build/entries.ts b/packages/agent-bundle/src/build/entries.ts index ec45ebe8d..8719872af 100644 --- a/packages/agent-bundle/src/build/entries.ts +++ b/packages/agent-bundle/src/build/entries.ts @@ -52,7 +52,7 @@ const eventRuntimeModulePath = (module: 'ipc' | 'project'): string => { * (`routes/public.ts`, `core/*`) as it is inlined, so the whole owning package * is what has to be ignored rather than the single aliased file. */ -const runtimeIgnoredRoot = (path: string): string => { +export const runtimeIgnoredRoot = (path: string): string => { const normalized = path.replaceAll('\\', '/'); const marker = normalized.includes('/dist/') ? '/dist/' : '/src/'; return resolve(normalized.slice(0, normalized.lastIndexOf(marker))); @@ -102,14 +102,20 @@ export const planCompiledEntries = ( entries: readonly NormalizedScript[], options: { readonly cwd: string; readonly outDir: string }, ): readonly PlannedScriptEntry[] => { - const names = new Set(); + const destinations = new Set(); return Object.freeze(entries.map((script) => { const filename = outputName(script); - if (script.name.length === 0 || names.has(filename)) { + if (script.name.length === 0 || destinations.has(filename)) { throw new Error(`Duplicate compiled script destination ${JSON.stringify(`scripts/${filename}`)}.`); } - names.add(filename); + destinations.add(filename); const workerFile = `${script.name}-flight.mjs`; + if (script.rendered === true) { + if (destinations.has(workerFile)) { + throw new Error(`Duplicate compiled script destination ${JSON.stringify(`scripts/${workerFile}`)}.`); + } + destinations.add(workerFile); + } return { mode: script.mode, name: script.name, @@ -196,7 +202,7 @@ export const compileEntries = async ( }; })()]; })), - ...(cliRuntimeShell === undefined ? {} : { ignoredSourcePaths: [cliRuntimeShell] }), + ...(cliRuntimeShell === undefined ? {} : { ignoredSourcePaths: [runtimeIgnoredRoot(cliRuntimeShell)] }), outputRoot: options.outDir, ...(options.tools === undefined ? {} : { tools: options.tools }), }); diff --git a/packages/agent-bundle/src/build/entry-shell.ts b/packages/agent-bundle/src/build/entry-shell.ts index a1954a486..663cad380 100644 --- a/packages/agent-bundle/src/build/entry-shell.ts +++ b/packages/agent-bundle/src/build/entry-shell.ts @@ -129,11 +129,11 @@ const renderedSessionSource = (workerFile: string): readonly string[] => [ ' let sequence = 0;', ' const failPending = (error) => { for (const entry of [...pending.values()]) entry.fail(error); pending.clear(); };', " worker.on('error', failPending);", - " worker.on('exit', (code) => { if (code !== 0) failPending(new Error(`Generated render worker exited with code ${String(code)}.`)); });", + " worker.on('exit', (code) => { if (pending.size > 0) failPending(new Error(`Generated render worker exited with code ${String(code)}.`)); });", " worker.on('message', (message) => {", ' const entry = pending.get(message.id);', ' if (entry === undefined) return;', - " if (message.type === 'progress') { void entry.progress?.report(message.update); return; }", + " if (message.type === 'progress') { Promise.resolve().then(() => entry.progress?.report(message.update)).catch(entry.fail); return; }", " if (message.type === 'chunk') { entry.enqueue(message.bytes); return; }", ' pending.delete(message.id);', " entry.signal.removeEventListener('abort', entry.abort);", @@ -149,7 +149,7 @@ const renderedSessionSource = (workerFile: string): readonly string[] => [ " abort: () => { worker.postMessage({ id, type: 'cancel' }); pending.delete(id); try { streamController.error(new DOMException('Agent render was aborted', 'AbortError')); } catch {} },", ' close: () => { try { streamController.close(); } catch {} },', ' enqueue: (bytes) => { try { streamController.enqueue(bytes); } catch {} },', - ' fail: (error) => { pending.delete(id); try { streamController.error(error); } catch {} },', + " fail: (error) => { pending.delete(id); dispatch.signal.removeEventListener('abort', entry.abort); try { streamController.error(error); } catch {} },", ' progress: dispatch.progress,', ' signal: dispatch.signal,', ' };', diff --git a/packages/agent-bundle/src/build/package-build.ts b/packages/agent-bundle/src/build/package-build.ts index 6e7047d45..b7c1fdd6e 100644 --- a/packages/agent-bundle/src/build/package-build.ts +++ b/packages/agent-bundle/src/build/package-build.ts @@ -6,6 +6,7 @@ import type { AgentBundleToolsConfig, NormalizedPlugin } from '../core/types.ts' import { assertInside } from '../core/paths.ts'; import { listArtifactFiles, publishArtifact, resolveArtifactDestination } from './emit.ts'; import { scanEntryExports } from './entry-exports.ts'; +import { runtimeIgnoredRoot } from './entries.ts'; import { cliEntryRuntimePath, cliEntryRuntimeSpecifier, @@ -219,7 +220,7 @@ export const buildPackageOutputs = async (options: { const evidence = await buildWithRslib({ cwd: projectRoot, entries, - ...(cliRuntimeShell === undefined ? {} : { ignoredSourcePaths: [cliRuntimeShell] }), + ...(cliRuntimeShell === undefined ? {} : { ignoredSourcePaths: [runtimeIgnoredRoot(cliRuntimeShell)] }), logLevel: 'error', outputRoot: stageRoot, ...(options.tools === undefined ? {} : { tools: options.tools }), diff --git a/packages/agent-bundle/src/cli-entry.ts b/packages/agent-bundle/src/cli-entry.ts index ca7524cd9..355485ef3 100644 --- a/packages/agent-bundle/src/cli-entry.ts +++ b/packages/agent-bundle/src/cli-entry.ts @@ -1,4 +1,5 @@ import type { CompiledCliCommand, CompiledCliOption } from './routes/types.ts'; +import { stableJson } from './core/digest.ts'; /** * The framework-owned routed-CLI shell (#102 stage 2): command-tree @@ -312,6 +313,7 @@ const coercePositional = (option: CompiledCliOption, value: string): unknown => /** Parses one resolved command's remaining argv against its compiled option surface. */ const parseCommandArgv = (command: CompiledCliCommand, argv: readonly string[]): ParsedArgv => { const options = new Map(namedOptions(command).map((option) => [option.option, option])); + const positionals = sortedPositionals(command); const values = new Map(); const bare: string[] = []; let json = false; @@ -363,11 +365,15 @@ const parseCommandArgv = (command: CompiledCliCommand, argv: readonly string[]): readOption(raw); continue; } - if (raw.startsWith('-') && raw.length > 1) throw new CliUsageError(`Unknown option: ${raw}.`); + if (raw.startsWith('-') && raw.length > 1) { + const positional = positionals[bare.length] ?? + (positionals[positionals.length - 1]?.repeated === true ? positionals[positionals.length - 1] : undefined); + const negativeNumber = positional?.kind === 'number' && /^-\d/u.test(raw) && Number.isFinite(Number(raw)); + if (!negativeNumber) throw new CliUsageError(`Unknown option: ${raw}.`); + } bare.push(raw); } - const positionals = sortedPositionals(command); let cursor = 0; for (const option of positionals) { if (option.repeated) { @@ -636,8 +642,9 @@ export const runGeneratedCliEntry = async (options: RunGeneratedCliOptions): Pro if (parsed.ndjson) throw new CliUsageError('--ndjson requires a rendered command.'); const result = await options.execute(command, parsed.input, { json: parsed.json, signal }); signal.throwIfAborted(); - writeOut(`${JSON.stringify(result)}\n`); - return resultExitCode(command.exitCode, result); + const exitCode = resultExitCode(command.exitCode, result); + writeOut(`${stableJson(result === undefined ? null : result)}\n`); + return exitCode; } catch (error) { if (signal.aborted || (error instanceof DOMException && error.name === 'AbortError')) { writeErr('Aborted.\n'); diff --git a/packages/agent-bundle/tests/cli-routes-build.test.ts b/packages/agent-bundle/tests/cli-routes-build.test.ts index 3db175b6b..11e3b877a 100644 --- a/packages/agent-bundle/tests/cli-routes-build.test.ts +++ b/packages/agent-bundle/tests/cli-routes-build.test.ts @@ -104,6 +104,15 @@ it('builds and runs the generated routed-CLI executable', { retry: 2, timeout: 1 '}', '', ].join('\n')), + writeProjectFile(root, 'src/cli/exit-zero.tsx', [ + "import { z } from 'zod';", + 'export const inputSchema = z.object({}).strict();', + 'export const resultSchema = z.object({ ok: z.boolean() }).strict();', + 'export default async function ExitZero() {', + ' process.exit(0);', + '}', + '', + ].join('\n')), writeProjectFile(root, 'src/scripts/summarize.tsx', [ "import React from 'react';", "import { Agent } from '@agent-bundle/runtime';", @@ -186,6 +195,14 @@ it('builds and runs the generated routed-CLI executable', { retry: 2, timeout: 1 // Rendered input-validation failures stay usage failures. await expect(execFile(binPath, ['report'])).rejects.toMatchObject({ code: 2, stdout: '' }); + // A worker that exits cleanly before completing a request must fail that + // request explicitly instead of leaving its Flight stream unsettled. + await expect(execFile(binPath, ['exit-zero'], { timeout: 5_000 })).rejects.toMatchObject({ + code: 1, + stderr: 'Generated render worker exited with code 0.\n', + stdout: '', + }); + // The rendered .tsx script (#102 stage 3) ships beside plain scripts in // the target artifact with the same output contract. const scriptPath = join(root, 'artifact', 'portable', 'scripts', 'summarize.mjs'); diff --git a/packages/agent-bundle/tests/cli-routes.test.ts b/packages/agent-bundle/tests/cli-routes.test.ts index 2dc31bad7..11b322f3e 100644 --- a/packages/agent-bundle/tests/cli-routes.test.ts +++ b/packages/agent-bundle/tests/cli-routes.test.ts @@ -419,6 +419,17 @@ describe('generated CLI shell', () => { rendered: false, routeId: 'cli:library/audit', }, + { + aliases: [], + description: 'Apply a signed offset.', + exitCode: 'zero', + options: [ + { key: 'offset', kind: 'number', option: 'offset', positional: 0, repeated: false, required: true }, + ], + path: ['offset'], + rendered: false, + routeId: 'cli:offset', + }, ]; interface RunResult { @@ -446,7 +457,7 @@ describe('generated CLI shell', () => { execute: async (command, input, context) => { calls.push({ command, input, json: context.json }); if (options.throws !== undefined) throw options.throws; - return options.result ?? { ok: true }; + return Object.hasOwn(options, 'result') ? options.result : { ok: true }; }, name: 'curator', ...(options.signal === undefined ? {} : { signal: options.signal }), @@ -515,6 +526,30 @@ describe('generated CLI shell', () => { expect(variadic.calls[0]!.input).toEqual({ format: 'json', report: 'out.json', sources: ['a', '--b'] }); }); + it('accepts negative numeric positionals without weakening single-dash option handling', async () => { + const negative = await run(['offset', '-5']); + expect(negative.code).toBe(0); + expect(negative.calls[0]!.input).toEqual({ offset: -5 }); + + const unknown = await run(['offset', '-x']); + expect(unknown.code).toBe(2); + expect(unknown.stderr).toContain('Unknown option: -x.'); + expect(unknown.calls).toEqual([]); + + const escaped = await run(['offset', '--', '-5']); + expect(escaped.code).toBe(0); + expect(escaped.calls[0]!.input).toEqual({ offset: -5 }); + }); + + it('writes undefined results as canonical JSON null', async () => { + const result = await run(['doctor', '/library'], { result: undefined }); + expect(result.code).toBe(0); + expect(result.stdout).toBe('null\n'); + + const ordered = await run(['doctor', '/library'], { result: { z: 1, a: 2 } }); + expect(ordered.stdout).toBe('{"a":2,"z":1}\n'); + }); + it('maps usage failures to exit 2 with a help hint on stderr', async () => { const cases: readonly (readonly [readonly string[], string])[] = [ [['unknown'], 'Unknown command: unknown.'], @@ -559,6 +594,7 @@ describe('generated CLI shell', () => { const missing = await run(['library', 'audit', '--report', 'r', 'a'], { result: { ok: true } }); expect(missing.code).toBe(1); + expect(missing.stdout).toBe(''); expect(missing.stderr).toContain('exitCode result policy'); }); diff --git a/packages/agent-bundle/tests/cli.test.ts b/packages/agent-bundle/tests/cli.test.ts index f0fa78e86..8534a7f62 100644 --- a/packages/agent-bundle/tests/cli.test.ts +++ b/packages/agent-bundle/tests/cli.test.ts @@ -544,3 +544,38 @@ it('reports source validation diagnostics on stderr before staging an artifact', await rm(resolve(project.root, '..'), { force: true, recursive: true }); } }, 30_000 * timeScale); + +it('reports a generated Flight worker collision before compiling scripts', async () => { + const project = await createCliProject(); + try { + await mkdir(join(project.root, 'src', 'scripts'), { recursive: true }); + await Promise.all([ + writeFile( + join(project.root, 'src', 'scripts', 'report.tsx'), + 'export default async function Report() { return null; }\n', + ), + writeFile( + join(project.root, 'src', 'scripts', 'report-flight.ts'), + 'export const main = async () => 0;\n', + ), + ]); + + const result = await runSourceCliWithOutput([ + 'build', + '--root', project.root, + '--output', project.output, + '--target', 'portable', + '--json', + ]); + + expect(result.code).toBe(1); + expect(result.stdout).toBe(''); + expect(JSON.parse(result.stderr)).toMatchObject([{ + code: 'AB5000', + message: 'Duplicate compiled script destination "scripts/report-flight.mjs".', + severity: 'error', + }]); + } finally { + await rm(resolve(project.root, '..'), { force: true, recursive: true }); + } +}, 30_000 * timeScale); diff --git a/packages/agent-bundle/tests/entry-shell.test.ts b/packages/agent-bundle/tests/entry-shell.test.ts index c886c0b4f..36d13f1ef 100644 --- a/packages/agent-bundle/tests/entry-shell.test.ts +++ b/packages/agent-bundle/tests/entry-shell.test.ts @@ -1,4 +1,6 @@ +import { execFile as executeFile } from 'node:child_process'; import { access, readFile } from 'node:fs/promises'; +import { promisify } from 'node:util'; import { describe, expect, it } from '@rstest/core'; @@ -6,6 +8,7 @@ import { scanEntryExportsSource, stripCommentsAndStrings } from '../src/build/en import * as entryShellModule from '../src/build/entry-shell.ts'; import { generatedExecutableEntrySource, + generatedRenderedScriptEntrySource, generatedStdioMcpEntrySource, mcpEntryRuntimePath, mcpEntryRuntimeSpecifier, @@ -13,6 +16,8 @@ import { mcpServerRuntimeSpecifier, } from '../src/build/entry-shell.ts'; +const execFile = promisify(executeFile); + describe('entry export scanning', () => { it('detects declaration-form main exports', () => { expect(scanEntryExportsSource('export const main = async () => 0;')).toEqual({ @@ -87,6 +92,60 @@ describe('generated entry templates', () => { expect(source).toContain("if (typeof code === 'number') process.exitCode = code;"); expect(generatedExecutableEntrySource({ entrySource: '/e.ts', exportName: 'default' })).toContain('entry["default"]'); }); + + it('routes a rejected progress report into the generated request failure path', async () => { + const generated = generatedRenderedScriptEntrySource({ + name: 'report', + routeId: 'script:report', + workerFile: 'report-flight.mjs', + }); + const factoryStart = generated.indexOf('const openRenderedSession'); + const factoryEnd = generated.indexOf('\nawait runGeneratedRenderedScriptProcess'); + const factory = generated.slice(factoryStart, factoryEnd) + .replaceAll('import.meta.url', JSON.stringify(import.meta.url)); + const harness = [ + "import { EventEmitter } from 'node:events';", + factory, + 'class FakeWorker extends EventEmitter {', + ' stdout = new EventEmitter();', + ' stderr = new EventEmitter();', + ' postMessage(message) {', + " if (message.type === 'render') queueMicrotask(() => this.emit('message', { id: message.id, type: 'progress', update: { completed: 1 } }));", + ' }', + ' async terminate() { return 0; }', + '}', + 'const Worker = FakeWorker;', + 'const createAgentRenderDispatcher = (host) => ({', + ' stream: ({ signal }) => new ReadableStream({', + ' async start(controller) {', + ' try {', + " const flight = await host.execute({ progress: { report: async () => { throw new Error('progress rejected'); } }, signal });", + ' await flight.getReader().read();', + ' controller.close();', + ' } catch (error) { controller.error(error); }', + ' },', + ' }),', + '});', + "process.on('unhandledRejection', (error) => process.stderr.write(`UNHANDLED:${error instanceof Error ? error.message : String(error)}\\n`));", + 'const signal = new AbortController().signal;', + "const session = openRenderedSession({ invocation: {}, props: {}, request: {}, routeId: 'script:report', signal, validate: (value) => value });", + 'try {', + ' await Promise.race([', + ' session.events().getReader().read(),', + " new Promise((_, reject) => setTimeout(() => reject(new Error('timeout')), 100)),", + ' ]);', + " process.stdout.write('RESOLVED\\n');", + '} catch (error) {', + " process.stdout.write(`REJECTED:${error instanceof Error ? error.message : String(error)}\\n`);", + '} finally {', + ' await session.close();', + ' await new Promise((resolve) => setImmediate(resolve));', + '}', + ].join('\n'); + + const result = await execFile(process.execPath, ['--input-type=module', '--eval', harness]); + expect(result).toMatchObject({ stderr: '', stdout: 'REJECTED:progress rejected\n' }); + }); }); From 7fd0e6efa6ec12d4a8dd7bae00aac63462e6929d Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Tue, 1 Sep 2026 21:32:45 +0000 Subject: [PATCH 2/2] fix(build): anchor the runtime ignored root to the module's marker ancestor Review follow-up on #210: runtimeIgnoredRoot matched any /dist/ segment in the path, so a checkout living under a dist directory resolved the ignored root to the wrong ancestor and silently ignored the whole checkout. The root is now the parent of the nearest src or dist ancestor of the runtime module itself, and an unmarked path is a loud error. --- packages/agent-bundle/src/build/entries.ts | 15 ++++++++++--- packages/agent-bundle/tests/entries.test.ts | 25 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 packages/agent-bundle/tests/entries.test.ts diff --git a/packages/agent-bundle/src/build/entries.ts b/packages/agent-bundle/src/build/entries.ts index 8719872af..e7868b348 100644 --- a/packages/agent-bundle/src/build/entries.ts +++ b/packages/agent-bundle/src/build/entries.ts @@ -1,6 +1,6 @@ import { existsSync } from 'node:fs'; import { readFile, stat } from 'node:fs/promises'; -import { extname, relative, resolve } from 'node:path'; +import { basename, dirname, extname, relative, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { @@ -54,8 +54,17 @@ const eventRuntimeModulePath = (module: 'ipc' | 'project'): string => { */ export const runtimeIgnoredRoot = (path: string): string => { const normalized = path.replaceAll('\\', '/'); - const marker = normalized.includes('/dist/') ? '/dist/' : '/src/'; - return resolve(normalized.slice(0, normalized.lastIndexOf(marker))); + let directory = dirname(normalized); + while (true) { + if (basename(directory) === 'dist' || basename(directory) === 'src') { + return resolve(dirname(directory)); + } + const parent = dirname(directory); + if (parent === directory) { + throw new Error(`Runtime module is not under an owning package src or dist directory: ${JSON.stringify(path)}.`); + } + directory = parent; + } }; export interface CompiledEntry { diff --git a/packages/agent-bundle/tests/entries.test.ts b/packages/agent-bundle/tests/entries.test.ts new file mode 100644 index 000000000..8f5e5638c --- /dev/null +++ b/packages/agent-bundle/tests/entries.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, it } from '@rstest/core'; + +import { runtimeIgnoredRoot } from '../src/build/entries.ts'; + +describe('runtime ignored root', () => { + it('anchors a source runtime to its package when the checkout is under dist', () => { + expect(runtimeIgnoredRoot('/tmp/dist/checkout/packages/agent-bundle/src/cli-entry.ts')) + .toBe('/tmp/dist/checkout/packages/agent-bundle'); + }); + + it('resolves the normal source layout', () => { + expect(runtimeIgnoredRoot('/work/agent-bundle/src/cli-entry.ts')) + .toBe('/work/agent-bundle'); + }); + + it('resolves the normal installed distribution layout', () => { + expect(runtimeIgnoredRoot('/x/node_modules/agent-bundle/dist/cli-entry.js')) + .toBe('/x/node_modules/agent-bundle'); + }); + + it('uses the runtime file parent when an earlier dist segment is present', () => { + expect(runtimeIgnoredRoot('/var/cache/dist/project/src/cli-entry.ts')) + .toBe('/var/cache/dist/project'); + }); +});