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..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 { @@ -52,10 +52,19 @@ 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))); + 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 { @@ -102,14 +111,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 +211,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/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'); + }); +}); 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' }); + }); });