From 0fdf46052cf755dbe008f5e9a0702fba83392614 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 05:30:19 +0000 Subject: [PATCH 01/19] =?UTF-8?q?fix:=20address=20review=20threads=20wave?= =?UTF-8?q?=204=20=E2=80=94=20worker=20invocation,=20codex=20path=20regex,?= =?UTF-8?q?=20tool=5Finput=20scope,=20CLI=20help,=20README=20targets,=20wo?= =?UTF-8?q?rker-root=20cleanup,=20probe=20redaction/teardown?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/framework-mode.md | 9 +- docs/local-ci.md | 8 +- packages/agent-bundle/src/adapters/codex.ts | 2 +- packages/agent-bundle/src/adapters/plugin.ts | 2 +- .../adapters/schemas/codex/PROVENANCE.json | 7 +- .../adapters/schemas/codex/plugin.schema.json | 2 +- packages/agent-bundle/src/cli.ts | 4 +- .../src/dev/playground/mcp-probe-service.ts | 103 +++++++++++++--- .../agent-bundle/src/events/projection.ts | 19 ++- .../tests/adapter-metadata.test.ts | 8 +- .../tests/cli-routes-build.test.ts | 17 ++- packages/agent-bundle/tests/cli.test.ts | 12 ++ .../tests/codex-plugin-validation.test.ts | 14 +++ .../agent-bundle/tests/entry-shell.test.ts | 53 ++++++++ .../agent-bundle/tests/event-project.test.ts | 27 +++++ .../agent-bundle/tests/host-adapters.test.ts | 19 +++ .../tests/mcp-probe-service.test.ts | 113 ++++++++++++++++++ .../tests/route-unit/event-project.test.ts | 52 ++++++++ .../tests/rstest-worker-isolation.test.ts | 71 ++++++++++- packages/create-agent-bundle/src/scaffold.ts | 68 ++++++++++- .../tests/scaffold.test.ts | 55 +++++++++ rstest.worker-isolation.ts | 46 ++++++- scripts/local-ci.mjs | 49 ++++++-- scripts/rstest-worker-roots.d.mts | 23 ++++ scripts/rstest-worker-roots.mjs | 72 +++++++++++ 25 files changed, 799 insertions(+), 56 deletions(-) create mode 100644 scripts/rstest-worker-roots.d.mts create mode 100644 scripts/rstest-worker-roots.mjs diff --git a/docs/framework-mode.md b/docs/framework-mode.md index 3f8792246..ac68be7c0 100644 --- a/docs/framework-mode.md +++ b/docs/framework-mode.md @@ -188,9 +188,12 @@ export default defineConfig({ }); ``` -`output.distPath` defaults to `dist`. A CLI `--output ` overrides the -configured path, so precedence is CLI `--output`, then `output.distPath`, then -`dist`; existing projects are unchanged. The configured directory is excluded +`output.distPath` defaults to `dist` for the programmatic `build()` API and to +`artifact` for the `agent-bundle build` and `agent-bundle prepack` commands, +which also emit the npm package build into `dist/`. A CLI `--output ` +overrides the configured path, so precedence is CLI `--output`, then +`output.distPath`, then the operation default; existing projects are +unchanged. The configured directory is excluded from project source snapshots (as `dist` always was), ignored by the dev watcher, and used by Workbench host discovery and doctor drift checks. diff --git a/docs/local-ci.md b/docs/local-ci.md index 6d19d7434..26246af4f 100644 --- a/docs/local-ci.md +++ b/docs/local-ci.md @@ -46,7 +46,13 @@ still fails its own scan. Rstest re-hashes that leg directory, worker ID, and invocation identity to `/tmp/ab-rstest-` before exposing its worker `TMPDIR`; this leaves headroom below Linux's 108-byte `sun_path` cap for nested socket fixtures without sacrificing per-leg, per-worker, or concurrent-run -isolation. Legs live under +isolation. Because those hashed roots live beside the leg directory rather +than inside it, each one carries an owner marker (`.ab-rstest-owner.json`) +naming the leg `TMPDIR` and process it was derived from; the runner removes +the roots owned by a leg's `TMPDIR` — and only those, once their creating +process has exited — before the leg starts (leftovers of an interrupted run) +and after it finishes (`scripts/rstest-worker-roots.mjs`), so reruns cannot +accumulate worker caches or interrupted-test fixtures under `/tmp`. Legs live under `.worktrees/local-ci/` (gitignored), are reused across runs for warm caches, and can be recreated with `--fresh`. diff --git a/packages/agent-bundle/src/adapters/codex.ts b/packages/agent-bundle/src/adapters/codex.ts index e8f2ab4d6..f1fbbf609 100644 --- a/packages/agent-bundle/src/adapters/codex.ts +++ b/packages/agent-bundle/src/adapters/codex.ts @@ -175,7 +175,7 @@ const hookContract = Object.freeze({ wrapperSource: (entry) => nativeHookWrapperSource(entry, 'Codex'), } satisfies TargetHookContract); const metadata = Object.freeze({ - adapterRevision: '1.8.0', + adapterRevision: '1.9.0', observedVersion: capabilityTable.observedCliVersion, schemas: schemaDescriptorsFrom(schemaProvenance, schemaProvenance.observedCliVersion), }); diff --git a/packages/agent-bundle/src/adapters/plugin.ts b/packages/agent-bundle/src/adapters/plugin.ts index 733e8c25c..0b3992655 100644 --- a/packages/agent-bundle/src/adapters/plugin.ts +++ b/packages/agent-bundle/src/adapters/plugin.ts @@ -228,7 +228,7 @@ const artifactValidation = deepFreeze({ }); const metadata = Object.freeze({ - adapterRevision: '1.23.0', + adapterRevision: '1.24.0', observedVersion: `${claudeAdapter.metadata.observedVersion}+${codexAdapter.metadata.observedVersion}+${cursorAdapter.metadata.observedVersion}`, // Metadata schemas must exactly match the validation contract: each host's // documents, with one shared Claude-format hook schema (the pinned Codex diff --git a/packages/agent-bundle/src/adapters/schemas/codex/PROVENANCE.json b/packages/agent-bundle/src/adapters/schemas/codex/PROVENANCE.json index 0e59c10fc..7c16d4e55 100644 --- a/packages/agent-bundle/src/adapters/schemas/codex/PROVENANCE.json +++ b/packages/agent-bundle/src/adapters/schemas/codex/PROVENANCE.json @@ -16,7 +16,8 @@ "Inline mcpServers values must be objects; inline hook documents use the same closed eleven-event, command-or-mcp_tool handler shape as hooks.schema.json.", "The closed interface object admits every documented install-surface field; brandColor requires a six-digit hexadecimal value, external links require http(s), asset paths must stay inside the plugin root, and screenshots must be ./assets/-relative PNG paths.", "The apps pointer is const-locked to ./.app.json, and app.schema.json requires a nonempty apps map whose entries carry exactly one nonempty registered-connection id.", - "Component and interface-asset path patterns treat backslashes as separators and reject them outright so Windows-form parent traversal cannot escape the plugin root, and they reject control characters and Unicode line terminators (U+0000-U+001F, U+007F, U+2028, U+2029) so a line break cannot hide a parent segment from the containment lookahead; HTTP(S) URL scheme patterns are case-insensitive to match WHATWG URL protocol normalization." + "Component and interface-asset path patterns treat backslashes as separators and reject them outright so Windows-form parent traversal cannot escape the plugin root, and they reject control characters and Unicode line terminators (U+0000-U+001F, U+007F, U+2028, U+2029) so a line break cannot hide a parent segment from the containment lookahead; HTTP(S) URL scheme patterns are case-insensitive to match WHATWG URL protocol normalization.", + "The screenshots item pattern applies the same containment lookahead and character exclusions as the other asset paths (backslashes rejected as separators, control characters and Unicode line terminators rejected) on top of its ./assets/ prefix and .png suffix, so a Windows-form or line-break-hidden parent segment cannot escape the assets directory." ], "marketplace.schema.json": [ "Transcribed 2026-09-02 from the Marketplace metadata section of https://developers.openai.com/plugins/build/plugins: top-level name, interface.displayName, and plugins[] entries with name, source, policy, and category.", @@ -52,8 +53,8 @@ "url": "https://github.com/openai/codex/blob/main/codex-rs/core/config.schema.json" }, "plugin.schema.json": { - "bytes": 6598, - "sha256": "4ad476545c96c83d899c4524dcccd4eb4fe7d3299c307878a0cd46a237f48d58", + "bytes": 6651, + "sha256": "074c6c71966a3e6560ccbceb8d82ec6a40cb1eccee2f2d863fb4ef1e2276a814", "url": "https://github.com/openai/codex/blob/main/codex-rs/skills/src/assets/samples/plugin-creator/references/plugin-json-spec.md" } }, diff --git a/packages/agent-bundle/src/adapters/schemas/codex/plugin.schema.json b/packages/agent-bundle/src/adapters/schemas/codex/plugin.schema.json index c9403b984..d896d1d90 100644 --- a/packages/agent-bundle/src/adapters/schemas/codex/plugin.schema.json +++ b/packages/agent-bundle/src/adapters/schemas/codex/plugin.schema.json @@ -125,7 +125,7 @@ "logoDark": { "pattern": "^\\./(?!(?:.*[/\\\\])?\\.\\.(?:[/\\\\]|$))[^\\\\\\u0000-\\u001F\\u007F\\u2028\\u2029]+$", "type": "string" }, "longDescription": { "minLength": 1, "pattern": "\\S", "type": "string" }, "privacyPolicyURL": { "pattern": "^[Hh][Tt][Tt][Pp][Ss]?://", "type": "string" }, - "screenshots": { "items": { "pattern": "^\\./assets/(?!.*(?:^|/)\\.\\.(?:/|$)).+\\.png$", "type": "string" }, "type": "array" }, + "screenshots": { "items": { "pattern": "^\\./assets/(?!(?:.*[/\\\\])?\\.\\.(?:[/\\\\]|$))[^\\\\\\u0000-\\u001F\\u007F\\u2028\\u2029]+\\.png$", "type": "string" }, "type": "array" }, "shortDescription": { "minLength": 1, "pattern": "\\S", "type": "string" }, "termsOfServiceURL": { "pattern": "^[Hh][Tt][Tt][Pp][Ss]?://", "type": "string" }, "websiteURL": { "pattern": "^[Hh][Tt][Tt][Pp][Ss]?://", "type": "string" } diff --git a/packages/agent-bundle/src/cli.ts b/packages/agent-bundle/src/cli.ts index af7b1225e..e0b5030f5 100644 --- a/packages/agent-bundle/src/cli.ts +++ b/packages/agent-bundle/src/cli.ts @@ -544,7 +544,7 @@ export const runCli = async ( const buildCommand = configureSourceOptions( program.command('build').description('Build a validated Agent Bundle artifact'), - ).option('--output ', 'Artifact output path relative to --root (overrides config output.distPath; default dist)'); + ).option('--output ', 'Artifact output path relative to --root (overrides config output.distPath; default artifact, since dist is the npm package build output)'); buildCommand.action(async (options: BuildCommandOptions) => { const { build } = await import('./api.ts'); const result = await build({ ...projectOptions(options), output: options.output, packageOutputs: true }); @@ -554,7 +554,7 @@ export const runCli = async ( const prepackCommand = configureSourceOptions( program.command('prepack').description('Build and validate the npm pack inventory'), - ).option('--output ', 'Artifact output path relative to --root (overrides config output.distPath; default dist)'); + ).option('--output ', 'Artifact output path relative to --root (overrides config output.distPath; default artifact, since dist is the npm package build output)'); prepackCommand.action(async (options: BuildCommandOptions) => { const { prepack } = await import('./api.ts'); const result = await (dependencies.prepack ?? prepack)({ diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 28880f129..1f747629c 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -45,7 +45,14 @@ export const mcpProbeFailureTextLimit = 2_048; const mcpProbeCapabilityLimit = 32; const mcpProbeNameTextLimit = 256; +/** How long a probe response waits for transport teardown before detaching it. */ const mcpProbeTeardownWaitMs = 50; +/** + * Upper bound a detached teardown may hold the plugin-data directory. The + * stdio transport's close runs its own TERM/KILL sequence, so this only guards + * against a transport whose close never settles. + */ +export const mcpProbePluginDataTeardownCapMs = 10_000; const safeCapabilityName = /^[A-Za-z][A-Za-z0-9_-]{0,63}$/u; const connectionErrorCodes = new Set([ 'EACCES', @@ -126,8 +133,13 @@ const bundlePathPattern = (bundleRoot: string): RegExp => { return new RegExp(String.raw`(?:file:\/\/)?${root}${suffix}`, 'gu'); }; +/** + * An absolute POSIX path starts the text or follows a separator; a `:` counts + * as a separator (`cwd:/private`) only when it is not the `://` of a URI + * scheme, so `https://example.com/docs` is link guidance, not a local path. + */ const hasAbsolutePath = (value: string): boolean => - /(?:file:|(?:^|[\s"'([{=,:])\/[^\s,;{}()[\]<>"']+|(?:^|[\s"'([{=,:])[A-Za-z]:[\\/]|\\\\)/u.test(value); + /(?:file:|(?:^|[\s"'([{=,]|:(?!\/\/))\/[^\s,;{}()[\]<>"']+|(?:^|[\s"'([{=,:])[A-Za-z]:[\\/]|\\\\)/u.test(value); /** * Probe text follows the Dev Log browser-wire precedent without coupling this @@ -239,6 +251,7 @@ export class McpProbeService { readonly #createStreamableHttpTransport: NonNullable; readonly #inFlight = new Map>(); readonly #now: () => Date; + readonly #pendingTeardowns = new Set>(); readonly #prepared: McpProbeServiceOptions['prepared']; readonly #projectRoot: string; readonly #registry: TargetRegistry; @@ -284,6 +297,17 @@ export class McpProbeService { return probe; } + /** + * Resolve once every detached teardown — a transport close that outlived + * its probe's response boundary, followed by that probe's plugin-data + * removal — has settled. Probe responses never wait on this. + */ + async settle(): Promise { + while (this.#pendingTeardowns.size > 0) { + await Promise.allSettled([...this.#pendingTeardowns]); + } + } + async #run(options: { readonly host: McpProbeHost; readonly serverName: string; @@ -305,8 +329,10 @@ export class McpProbeService { const runtime = this.#runtime(options.host); const server = await this.#server(bundleRoot, options.host, runtime, options.serverName); const pluginData = await this.#createPluginData(); + let launch: ResolvedMcpSessionLaunch; + let projectedLaunch: McpProbeLaunch; try { - const launch = resolveMcpSessionLaunch({ + launch = resolveMcpSessionLaunch({ pluginData, resolved: { runtime, @@ -316,21 +342,52 @@ export class McpProbeService { }, workspaceRoot: this.#projectRoot, }); - const projectedLaunch = inspectorLaunch( + projectedLaunch = inspectorLaunch( mcpSessionInspectorConfig(launch, bundleRoot).launch, ); - return await this.#execute({ - bundleRoot, - generatedAt, - host: options.host, - launch, - projectedLaunch, - serverName: options.serverName, - startedAt, - }); - } finally { + } catch (error) { + // No transport was opened, so nothing can still hold the directory. await rm(pluginData, { force: true, recursive: true }); + throw error; } + // From here on the transport teardown owns plugin-data removal (#execute): + // the launched server may have the directory open until its close settles. + return this.#execute({ + bundleRoot, + generatedAt, + host: options.host, + launch, + pluginData, + projectedLaunch, + serverName: options.serverName, + startedAt, + }); + } + + /** + * Remove the probe's plugin-data directory once transport teardown has + * settled (or the teardown cap has elapsed), never at the response + * boundary: a stdio server that still holds the directory open while it + * shuts down would otherwise race the removal — on Windows the `rm` can + * reject outright and turn an honest timed-out report into a generic + * failure, elsewhere the directory can vanish under the exiting child. + * Removal failures stay on this detached path; they never reach the report. + */ + #removePluginDataAfter(teardown: Promise, pluginData: string): Promise { + let cap: NodeJS.Timeout | undefined; + const capped = new Promise((resolvePromise) => { + cap = setTimeout(resolvePromise, mcpProbePluginDataTeardownCapMs); + cap.unref(); + }); + const pending = Promise.race([teardown, capped]) + .then(() => { + if (cap !== undefined) clearTimeout(cap); + return rm(pluginData, { force: true, recursive: true }); + }) + .then(() => undefined, () => undefined); + this.#pendingTeardowns.add(pending); + void pending.then(() => this.#pendingTeardowns.delete(pending)); + return pending; } #runtime(host: McpProbeHost): TargetMcpRuntimeContract { @@ -374,12 +431,21 @@ export class McpProbeService { readonly generatedAt: string; readonly host: McpProbeHost; readonly launch: ResolvedMcpSessionLaunch; + readonly pluginData: string; readonly projectedLaunch: McpProbeLaunch; readonly serverName: string; readonly startedAt: number; }): Promise { - const client = this.#createClient(); - const transport = this.#transport(options.launch); + let client: McpProbeClient; + let transport: McpProbeTransport; + try { + client = this.#createClient(); + transport = this.#transport(options.launch); + } catch (error) { + // Nothing was launched, so the directory cannot be in use. + await rm(options.pluginData, { force: true, recursive: true }); + throw error; + } let report: McpProbeReport; try { try { @@ -468,14 +534,17 @@ export class McpProbeService { } finally { let timer: NodeJS.Timeout | undefined; // Keep transport teardown running through its TERM/KILL path without - // allowing a stalled close to extend the probe's total time budget. + // allowing a stalled close to extend the probe's total time budget. The + // plugin-data removal is chained behind that teardown, so a close that + // outlives this wait detaches together with the removal it gates. const teardown = Promise.allSettled([client.close(), transport.close()]); + const cleanup = this.#removePluginDataAfter(teardown, options.pluginData); const teardownWait = new Promise((resolvePromise) => { timer = setTimeout(resolvePromise, mcpProbeTeardownWaitMs); timer.unref(); }); try { - await Promise.race([teardown, teardownWait]); + await Promise.race([cleanup, teardownWait]); } finally { if (timer !== undefined) clearTimeout(timer); } diff --git a/packages/agent-bundle/src/events/projection.ts b/packages/agent-bundle/src/events/projection.ts index 13b6b4783..92d4b212a 100644 --- a/packages/agent-bundle/src/events/projection.ts +++ b/packages/agent-bundle/src/events/projection.ts @@ -221,15 +221,22 @@ export const validateNativeEventEnvelope = ( } if (canonicalEvent === 'permission/request') { requireNativeString(native, 'tool_name'); - // The pinned permission-request input schema declares `"tool_input": true` - // (any JSON value), so presence is required but shape is tool-defined. - if (!Object.hasOwn(native, 'tool_input') || native.tool_input === undefined) { - return nativeEventError('native tool_input is required'); - } - requirePermissionMode(native); if (target === 'codex') { + // Only the pinned Codex permission-request input schema declares + // `"tool_input": true` (any JSON value): presence is required but the + // shape is tool-defined. Claude's PermissionRequest envelope stays + // object-shaped like its other tool events. + if (!Object.hasOwn(native, 'tool_input') || native.tool_input === undefined) { + return nativeEventError('native tool_input is required'); + } + requirePermissionMode(native); requireNativeString(native, 'turn_id'); requireNativeString(native, 'model'); + } else { + if (typeof native.tool_input !== 'object' || native.tool_input === null || Array.isArray(native.tool_input)) { + return nativeEventError('native tool_input must be an object'); + } + requirePermissionMode(native); } } if (canonicalEvent === 'permission/denied') { diff --git a/packages/agent-bundle/tests/adapter-metadata.test.ts b/packages/agent-bundle/tests/adapter-metadata.test.ts index 6911bd0cc..ba9fd5b72 100644 --- a/packages/agent-bundle/tests/adapter-metadata.test.ts +++ b/packages/agent-bundle/tests/adapter-metadata.test.ts @@ -68,7 +68,7 @@ it('records exact immutable metadata for every built-in target', () => { ], }); expect(registryMetadata(registry, 'codex')).toEqual({ - adapterRevision: '1.8.0', + adapterRevision: '1.9.0', observedVersion: '0.147.0', schemas: [ { @@ -94,7 +94,7 @@ it('records exact immutable metadata for every built-in target', () => { { name: 'plugin', revision: '0.147.0', - sha256: '4ad476545c96c83d899c4524dcccd4eb4fe7d3299c307878a0cd46a237f48d58', + sha256: '074c6c71966a3e6560ccbceb8d82ec6a40cb1eccee2f2d863fb4ef1e2276a814', }, ], }); @@ -145,7 +145,7 @@ it('records exact immutable metadata for every built-in target', () => { ], }); expect(registryMetadata(registry, 'cursor')).toEqual({ - adapterRevision: '1.8.0', + adapterRevision: '1.9.0', observedVersion: '2026-08-28', schemas: [ { @@ -170,7 +170,7 @@ it('records exact immutable metadata for every built-in target', () => { }, ], }); - expect(registryMetadata(registry, 'plugin').adapterRevision).toBe('1.23.0'); + expect(registryMetadata(registry, 'plugin').adapterRevision).toBe('1.24.0'); }); it('records observed capability versions and rehashes schema snapshots against pinned provenance', async () => { diff --git a/packages/agent-bundle/tests/cli-routes-build.test.ts b/packages/agent-bundle/tests/cli-routes-build.test.ts index 6a0aec090..895aa8351 100644 --- a/packages/agent-bundle/tests/cli-routes-build.test.ts +++ b/packages/agent-bundle/tests/cli-routes-build.test.ts @@ -69,11 +69,17 @@ it('builds and runs the generated routed-CLI executable', { retry: 2, timeout: 1 '', ].join('\n')), // A conventional request context provider (#313): every generated request - // scope — plain CLI, rendered CLI, rendered script — mounts the same value. + // scope — plain CLI, rendered CLI, projected MCP command, rendered script — + // mounts the same value. writeProjectFile(root, 'src/providers/library-tooling.ts', [ 'export default async function libraryTooling({ invocation, signal }) {', " if (signal.aborted) throw new DOMException('aborted', 'AbortError');", - " return { kind: invocation.kind, tool: 'ffprobe 6.1' };", + // Branching on the documented kind fails loudly if a surface ever posts + // no invocation to its worker again (#319 review). + " switch (invocation.kind) {", + " case 'cli': case 'script': case 'tool': return { kind: invocation.kind, tool: 'ffprobe 6.1' };", + " default: throw new Error(`unexpected invocation kind ${String(invocation.kind)}`);", + ' }', '}', '', ].join('\n')), @@ -143,11 +149,11 @@ it('builds and runs the generated routed-CLI executable', { retry: 2, timeout: 1 "import { z } from 'zod';", "export const config = { annotations: { readOnlyHint: true }, description: 'Looks up one value.' };", 'export const inputSchema = z.object({ message: z.string().default("ready") }).strict();', - "export const resultSchema = z.object({ invocation: z.literal('tool'), message: z.string(), operationId: z.string() }).strict();", + "export const resultSchema = z.object({ invocation: z.literal('tool'), message: z.string(), operationId: z.string(), tooling: z.string() }).strict();", 'export default async function Lookup({ input }) {', ' const context = await agent();', " await context.progress.report({ completed: 1, message: 'lookup', total: 1 });", - ' const result = { invocation: context.invocation.kind, message: input.message, operationId: context.invocation.operationId };', + ' const result = { invocation: context.invocation.kind, message: input.message, operationId: context.invocation.operationId, tooling: `${context.providers.libraryTooling.kind}:${context.providers.libraryTooling.tool}` };', ' return {`Lookup: ${input.message}`};', '}', '', @@ -271,10 +277,13 @@ it('builds and runs the generated routed-CLI executable', { retry: 2, timeout: 1 const projectedJson = await execFile(binPath, [ 'harness', 'lookup', '--input', '{"message":"packed"}', '--json', ]); + // The projected command's provider sees `invocation.kind === 'tool'`, not the + // CLI surface it was typed on (#319 review). expect(JSON.parse(projectedJson.stdout)).toEqual({ invocation: 'tool', message: 'packed', operationId: 'tool:harness/lookup', + tooling: 'tool:ffprobe 6.1', }); const projectedNdjson = await execFile(binPath, [ 'harness', 'lookup', '--input', '{"message":"events"}', '--ndjson', diff --git a/packages/agent-bundle/tests/cli.test.ts b/packages/agent-bundle/tests/cli.test.ts index 5dea84d4e..dee0bcdd9 100644 --- a/packages/agent-bundle/tests/cli.test.ts +++ b/packages/agent-bundle/tests/cli.test.ts @@ -165,6 +165,18 @@ it('parses the nested development proxy command', async () => { }); }); +it('describes the operation-specific artifact default in build and prepack help', async () => { + // Both CLI commands build with package outputs, so their artifact default is + // `artifact/` (the npm package build owns `dist/`); the API-level `dist` + // default must not leak into --help (#319 review). + for (const command of ['build', 'prepack']) { + const result = await runSourceCliWithOutput([command, '--help']); + expect(result.code).toBe(0); + expect(result.stdout).toMatch(/--output [\s\S]*default artifact/u); + expect(result.stdout).not.toContain('default dist'); + } +}); + it('requires a server name for the nested development proxy command', async () => { const result = await runSourceCliWithOutput(['dev', 'proxy', '--root', '/tmp/plugin']); diff --git a/packages/agent-bundle/tests/codex-plugin-validation.test.ts b/packages/agent-bundle/tests/codex-plugin-validation.test.ts index 4a18a6b36..3d12e7d11 100644 --- a/packages/agent-bundle/tests/codex-plugin-validation.test.ts +++ b/packages/agent-bundle/tests/codex-plugin-validation.test.ts @@ -291,6 +291,20 @@ it('reports app-server-only schema output as unassessable information even in st it('rejects malformed fixtures for every locally validated Codex schema', async () => { const malformed = [ ['.codex-plugin/plugin.json', { ...validDocuments['.codex-plugin/plugin.json'], name: 'Invalid Name' }], + // Line terminators must not let a parent-directory segment slip past the + // component-path traversal guard (#364 review): `$` and `.` are + // line-sensitive in JS regular expressions. + ['.codex-plugin/plugin.json', { ...validDocuments['.codex-plugin/plugin.json'], hooks: './hooks\n/../../outside.json' }], + ['.codex-plugin/plugin.json', { ...validDocuments['.codex-plugin/plugin.json'], mcpServers: './mcp\r/../outside.json' }], + ['.codex-plugin/plugin.json', { ...validDocuments['.codex-plugin/plugin.json'], skills: './skills\u2028/../../outside/' }], + ['.codex-plugin/plugin.json', { + ...validDocuments['.codex-plugin/plugin.json'], + interface: { ...validDocuments['.codex-plugin/plugin.json'].interface, logo: './assets\n/../../outside.png' }, + }], + ['.codex-plugin/plugin.json', { + ...validDocuments['.codex-plugin/plugin.json'], + interface: { ...validDocuments['.codex-plugin/plugin.json'].interface, screenshots: ['./assets/../outside.png'] }, + }], ['hooks/hooks.json', { hooks: { Stop: [{ hooks: [{ command: '', type: 'command' }] }] } }], ['.mcp.json', { mcpServers: { fixture: { type: 'streamable-http', url: 'not a uri' } } }], ['.agents/plugins/marketplace.json', { diff --git a/packages/agent-bundle/tests/entry-shell.test.ts b/packages/agent-bundle/tests/entry-shell.test.ts index 0e81e1ce4..c90e2f3c4 100644 --- a/packages/agent-bundle/tests/entry-shell.test.ts +++ b/packages/agent-bundle/tests/entry-shell.test.ts @@ -412,6 +412,59 @@ it('generates projected MCP commands with the same tool invocation and request c expect(source).toContain("invocation: { kind: 'tool', props: { input: parsed, operationId: command.routeId } }"); expect(source).toContain('request: { artifactEpoch: "route-fixture@1.2.3", kind: \'tool\', operationId: command.routeId, surface: command.mcp.tool }'); expect(source).toContain('props: { input: parsed }'); + // The worker mounts providers from `message.invocation`, so the render + // message must carry the dispatched invocation (#319 review). + expect(source).toContain("worker.postMessage({ id, invocation, props, request, routeId, type: 'render' })"); +}); + +it('forwards the dispatched invocation to the rendered worker in every rendered surface', async () => { + const generated = generatedRenderedScriptEntrySource({ + name: 'report', + routeId: 'script:report', + workerFile: 'report-flight.mjs', + }); + expect(generated).toContain("worker.postMessage({ id, invocation, props, request, routeId, type: 'render' })"); + 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') return;", + " process.stdout.write(`POSTED:${JSON.stringify(message.invocation)}\\n`);", + " queueMicrotask(() => this.emit('message', { id: message.id, type: 'end' }));", + ' }', + ' async terminate() { return 0; }', + '}', + 'const Worker = FakeWorker;', + // The real dispatcher hands host.execute the invocation from stream(). + 'const createAgentRenderDispatcher = (host) => ({', + ' stream: ({ invocation, signal }) => new ReadableStream({', + ' async start(controller) {', + ' try {', + ' const flight = await host.execute({ invocation, progress: undefined, signal });', + ' await flight.getReader().read();', + ' controller.close();', + ' } catch (error) { controller.error(error); }', + ' },', + ' }),', + '});', + 'const signal = new AbortController().signal;', + "const session = openRenderedSession({ invocation: { kind: 'script', props: { input: ['a'], name: 'report' } }, props: {}, request: {}, routeId: 'script:report', signal, validate: (value) => value });", + 'await session.events().getReader().read();', + 'await session.close();', + ].join('\n'); + + const result = await execFile(process.execPath, ['--input-type=module', '--eval', harness]); + expect(result).toMatchObject({ + stderr: '', + stdout: 'POSTED:{"kind":"script","props":{"input":["a"],"name":"report"}}\n', + }); }); it('generates deterministic per-request provider execution in the shared Flight worker', () => { diff --git a/packages/agent-bundle/tests/event-project.test.ts b/packages/agent-bundle/tests/event-project.test.ts index 71ec75026..94a6d6ffd 100644 --- a/packages/agent-bundle/tests/event-project.test.ts +++ b/packages/agent-bundle/tests/event-project.test.ts @@ -338,6 +338,33 @@ it('validates permission and stop-failure envelopes against the pinned host cont target: 'codex', })).toThrow(/turn_id/u); + // Only the pinned Codex input schema declares `tool_input: true`: Codex + // admits every JSON shape while Claude's envelope stays object-shaped + // (#364 review). + for (const toolInput of [null, [], 'apply_patch', 7, false]) { + const shaped = { ...codexRequest, tool_input: toolInput }; + expect(validateNativeEventEnvelope(shaped, { + canonicalEvent: 'permission/request', + nativeEvent: 'PermissionRequest', + target: 'codex', + })).toBe(shaped); + expect(() => validateNativeEventEnvelope({ ...claudeRequest, tool_input: toolInput }, { + canonicalEvent: 'permission/request', + nativeEvent: 'PermissionRequest', + target: 'claude', + })).toThrow(/native tool_input must be an object/u); + } + expect(() => validateNativeEventEnvelope({ ...codexRequest, tool_input: undefined }, { + canonicalEvent: 'permission/request', + nativeEvent: 'PermissionRequest', + target: 'codex', + })).toThrow(/native tool_input is required/u); + expect(() => validateNativeEventEnvelope({ ...claudeRequest, tool_input: undefined }, { + canonicalEvent: 'permission/request', + nativeEvent: 'PermissionRequest', + target: 'claude', + })).toThrow(/native tool_input must be an object/u); + const claudeDenied = { cwd: '/workspace', hook_event_name: 'PermissionDenied', diff --git a/packages/agent-bundle/tests/host-adapters.test.ts b/packages/agent-bundle/tests/host-adapters.test.ts index b5c7025d0..e212c9a2c 100644 --- a/packages/agent-bundle/tests/host-adapters.test.ts +++ b/packages/agent-bundle/tests/host-adapters.test.ts @@ -1151,12 +1151,31 @@ it('admits documented Codex component path and inline manifest forms', async () { hooks: ['./hooks/start.json', './hooks/tools.json'], skills: './skills/' }, { hooks: hookDocument, mcpServers: { docs: { type: 'http', url: 'https://example.test/mcp' } }, skills: './skills/' }, { hooks: [hookDocument], skills: './skills/' }, + { + interface: { + ...manifest.interface, + composerIcon: './assets/icon.png', + logo: './assets/logo.png', + screenshots: ['./assets/overview.png', './assets/nested/detail.png'], + }, + skills: './skills/', + }, ]) { expect(validate({ ...manifest, ...componentFields }), JSON.stringify(validate.errors)).toBe(true); } for (const invalid of [ { hooks: '../hooks.json', skills: './skills/' }, { hooks: ['./hooks.json', '../outside.json'], skills: './skills/' }, + // Embedded line terminators must not hide a parent-directory segment from + // the traversal lookahead (#364 review). + { hooks: './hooks\n/../../outside.json', skills: './skills/' }, + { hooks: ['./hooks\r/../outside.json'], skills: './skills/' }, + { mcpServers: './mcp\u2028/../outside.json', skills: './skills/' }, + { skills: './skills\u2029/../../outside/' }, + { interface: { ...manifest.interface, logo: './assets\n/../../outside.png' }, skills: './skills/' }, + { interface: { ...manifest.interface, composerIcon: './icon\u2028/../../outside.png' }, skills: './skills/' }, + { interface: { ...manifest.interface, screenshots: ['./assets/../outside.png'] }, skills: './skills/' }, + { interface: { ...manifest.interface, screenshots: ['./assets/..\\..\\outside.png'] }, skills: './skills/' }, { hooks: [], skills: './skills/' }, { hooks: [{ description: 'missing hooks map' }], skills: './skills/' }, { mcpServers: '../.mcp.json', skills: './skills/' }, diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 832484ac4..b2f66a49e 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -162,6 +162,65 @@ it('redacts absolute paths after key-value and list separators', async () => { } }); +it('keeps URLs while redacting real absolute and bundle paths (#316 review)', async () => { + const root = await createBundle(); + try { + const service = serviceFor(root, { + createClient: () => client({ + getInstructions: () => 'See https://example.com/docs/getting-started and http://localhost:8080/health for guidance.', + getServerVersion: () => ({ + name: 'timeline', + title: 'Docs: https://docs.example.com/timeline', + version: '1.2.3', + }), + listTools: async () => ({ + tools: [ + { + description: `Reads ${join(root, 'data', 'catalog.json')} and serves https://example.com/api`, + inputSchema: { type: 'object' as const }, + name: 'bundle-path-and-url', + }, + { + description: 'Docs at https://example.com/docs; config at /etc/private/timeline.json', + inputSchema: { type: 'object' as const }, + name: 'url-then-absolute-path', + }, + { + description: 'cwd:/var/private/timeline', + inputSchema: { type: 'object' as const }, + name: 'colon-separated-absolute-path', + }, + { + description: 'file:///home/alice/private.json', + inputSchema: { type: 'object' as const }, + name: 'file-url', + }, + ], + }), + }), + }); + + const report = await service.probe({ host: 'claude', serverName: 'timeline' }); + + // A URI scheme's `://` is not a path separator: link guidance survives. + expect(report.snapshot?.instructions) + .toBe('See https://example.com/docs/getting-started and http://localhost:8080/health for guidance.'); + expect(report.snapshot?.serverInfo.title).toBe('Docs: https://docs.example.com/timeline'); + expect(report.snapshot?.tools.map((tool) => tool.description)).toEqual([ + // Bundle paths become the label and the URL beside them stays. + 'Reads /data/catalog.json and serves https://example.com/api', + // A real absolute path anywhere in the text still fails closed... + '[REDACTED]', + // ...including after a genuine `:` separator... + '[REDACTED]', + // ...and file: URLs are local paths. + '[REDACTED]', + ]); + } finally { + await rm(root, { force: true, recursive: true }); + } +}); + it('truncates server instructions to the named text budget', async () => { const root = await createBundle(); try { @@ -329,6 +388,60 @@ it('throws typed not-found errors for unavailable trusted probe targets', async } }); +it('removes plugin data only after a slow transport teardown settles (#316 review)', async () => { + const root = await createBundle(); + let pluginData: string | undefined; + const events: string[] = []; + const closeStarted = Promise.withResolvers(); + let releaseClose!: () => void; + const closeReleased = new Promise((resolvePromise) => { + releaseClose = resolvePromise; + }); + try { + const service = serviceFor(root, { + createPluginData: async () => { + pluginData = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-probe-data-')); + await writeFile(join(pluginData, 'proof.txt'), 'present'); + return pluginData; + }, + createStdioTransport: () => transport(async () => { + // A stdio server that takes longer than the 50 ms response-boundary + // wait to exit: the directory it may still hold must survive until + // this close settles. + closeStarted.resolve(); + await closeReleased; + events.push('transport-closed'); + }), + }); + + const report = await Promise.race([ + service.probe({ host: 'claude', serverName: 'timeline' }), + new Promise((_resolve, reject) => setTimeout( + () => reject(new Error('The probe response waited on the slow teardown.')), + 2_000, + ).unref()), + ]); + events.push('report-returned'); + await closeStarted.promise; + + // The response came back with teardown still pending and the plugin data intact. + expect(report.status).toBe('ok'); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).resolves.toBe('present'); + await new Promise((resolvePromise) => setTimeout(resolvePromise, 120)); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).resolves.toBe('present'); + + // Once the transport close settles, the detached path removes the directory. + releaseClose(); + await service.settle(); + events.push('plugin-data-removed'); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); + expect(events).toEqual(['report-returned', 'transport-closed', 'plugin-data-removed']); + } finally { + if (pluginData !== undefined) await rm(pluginData, { force: true, recursive: true }); + await rm(root, { force: true, recursive: true }); + } +}); + it('removes the fresh plugin data directory after every probe', async () => { const root = await createBundle(); let pluginData: string | undefined; diff --git a/packages/agent-bundle/tests/route-unit/event-project.test.ts b/packages/agent-bundle/tests/route-unit/event-project.test.ts index 16e89ffed..af5a5fb5b 100644 --- a/packages/agent-bundle/tests/route-unit/event-project.test.ts +++ b/packages/agent-bundle/tests/route-unit/event-project.test.ts @@ -5,6 +5,7 @@ import { createElement, Suspense } from 'react'; import { createCanonicalEventProps, projectEventDocument, + validateNativeEventEnvelope, } from '../../src/events/project.ts'; import { renderRoute } from '../../src/test/render.ts'; @@ -528,6 +529,57 @@ it('projects permission/request decisions through the pinned PermissionRequest o .toThrow(/no additional-context channel/u); }); +it('admits any-JSON permission/request tool_input only for Codex and keeps Claude object-shaped', async () => { + const claudeEnvelope = { + cwd: '/workspace', + hook_event_name: 'PermissionRequest', + permission_mode: 'default', + session_id: 'session-1', + tool_input: { command: 'rm -rf build' }, + tool_name: 'Bash', + transcript_path: '/workspace/transcript.jsonl', + }; + const codexEnvelope = { + ...claudeEnvelope, + model: 'gpt-5-codex', + tool_input: 'apply_patch', + tool_name: 'apply_patch', + transcript_path: null, + turn_id: 'turn-1', + }; + const options = { canonicalEvent: 'permission/request', nativeEvent: 'PermissionRequest' } as const; + + // Codex's pinned input schema declares `tool_input: true`, so a scalar + // envelope validates and renders through the same route as an object one. + const codexNative = validateNativeEventEnvelope(codexEnvelope, { ...options, target: 'codex' }); + const codexProps = createCanonicalEventProps( + 'permission/request', codexNative, 'codex', 'PermissionRequest', '0.147.0', new AbortController().signal, + ); + const denied = await renderRoute({ + default: async ({ native }: { readonly native: Readonly> }) => createElement( + Agent.Result, + { value: { outcome: 'deny', reason: `Denied ${String(native.tool_input)}.` } }, + ), + }, { + input: { canonical: codexProps.canonical, native: codexProps.native }, + kind: 'event-route', + routeId: 'event:permission/request', + }); + expect(projectEventDocument(denied.document, 'permission/request', 'codex', 'PermissionRequest')).toEqual({ + hookSpecificOutput: { + decision: { behavior: 'deny', message: 'Denied apply_patch.' }, + hookEventName: 'PermissionRequest', + }, + }); + + // Claude's envelope keeps the object-shaped contract of its other tool events. + for (const toolInput of [null, [], 'rm -rf build', 7]) { + expect(() => validateNativeEventEnvelope({ ...claudeEnvelope, tool_input: toolInput }, { ...options, target: 'claude' })) + .toThrow(/native tool_input must be an object/u); + } + expect(validateNativeEventEnvelope(claudeEnvelope, { ...options, target: 'claude' })).toBe(claudeEnvelope); +}); + it('projects permission/denied and stop/failure as observation-only Claude families', async () => { const deniedProps = createCanonicalEventProps( 'permission/denied', diff --git a/packages/agent-bundle/tests/rstest-worker-isolation.test.ts b/packages/agent-bundle/tests/rstest-worker-isolation.test.ts index c4cc985a4..1b74693c4 100644 --- a/packages/agent-bundle/tests/rstest-worker-isolation.test.ts +++ b/packages/agent-bundle/tests/rstest-worker-isolation.test.ts @@ -1,8 +1,16 @@ +import { mkdir, mkdtemp, readdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { expect, it } from '@rstest/core'; -import { rstestWorkerRootPath } from '../../../rstest.worker-isolation.ts'; +import { + removeOwnedRstestWorkerRoots, + rstestWorkerRootOwnerFile, + rstestWorkerRootPrefix, + rstestWorkerRootsParent, +} from '../../../scripts/rstest-worker-roots.mjs'; +import { rstestWorkerRoot, rstestWorkerRootOwner, rstestWorkerRootPath } from '../../../rstest.worker-isolation.ts'; it('keeps Doctor socket fixtures below the Linux AF_UNIX pathname cap', () => { const longLocalCiRoot = join( @@ -27,3 +35,64 @@ it('isolates concurrent Rstest invocations that share a host temporary root', () expect(firstRoot).not.toBe(secondRoot); }); + +it('stamps every worker root with the owner marker the local-CI runner cleans up by', () => { + const root = rstestWorkerRoot(); + expect(root.startsWith(join(rstestWorkerRootsParent, rstestWorkerRootPrefix)) || process.platform === 'win32').toBe(true); + // The setup file already isolated this worker, so TMPDIR points at the + // root itself; the marker records the HOST temp root it was derived from + // and the process that owns it. + expect(rstestWorkerRootOwner(root)).toMatchObject({ + cwd: process.cwd(), + pid: process.pid, + temporaryRoot: expect.stringMatching(/^\//u), + workerId: process.env['RSTEST_WORKER_ID'] ?? '0', + }); + expect(rstestWorkerRootOwner(root)?.temporaryRoot).not.toBe(root); +}); + +it('removes only the finished roots owned by one host temporary root', async () => { + const parent = await mkdtemp(join(tmpdir(), 'ab-rstest-roots-parent-')); + const legTmp = '/tmp/abci-deadbeef-verify-node24'; + const otherLegTmp = '/tmp/abci-deadbeef-verify-node26'; + const writeRoot = async (name: string, owner: Readonly> | undefined): Promise => { + const root = join(parent, name); + await mkdir(join(root, 'cache', 'cmd-1-1'), { recursive: true }); + await writeFile(join(root, 'cache', 'cmd-1-1', 'leftover'), 'x'); + if (owner !== undefined) await writeFile(join(root, rstestWorkerRootOwnerFile), `${JSON.stringify(owner)}\n`); + return root; + }; + try { + const finished = await writeRoot(`${rstestWorkerRootPrefix}0000000000000001`, { cwd: '/w', pid: 4_000_001, temporaryRoot: legTmp, workerId: '1' }); + const interrupted = await writeRoot(`${rstestWorkerRootPrefix}0000000000000002`, { cwd: '/w', pid: 4_000_002, temporaryRoot: legTmp, workerId: '2' }); + const live = await writeRoot(`${rstestWorkerRootPrefix}0000000000000003`, { cwd: '/w', pid: 4_000_003, temporaryRoot: legTmp, workerId: '3' }); + const otherLeg = await writeRoot(`${rstestWorkerRootPrefix}0000000000000004`, { cwd: '/w', pid: 4_000_004, temporaryRoot: otherLegTmp, workerId: '1' }); + const unmarked = await writeRoot(`${rstestWorkerRootPrefix}0000000000000005`, undefined); + const corrupt = await writeRoot(`${rstestWorkerRootPrefix}0000000000000006`, undefined); + await writeFile(join(corrupt, rstestWorkerRootOwnerFile), '{not json'); + const unrelated = await writeRoot('agent-bundle-artifact-000001', { cwd: '/w', pid: 4_000_007, temporaryRoot: legTmp, workerId: '1' }); + + const result = await removeOwnedRstestWorkerRoots({ + isAlive: (pid) => pid === 4_000_003, + parent, + temporaryRoot: legTmp, + }); + + expect(result).toEqual({ removed: [finished, interrupted], retained: [live] }); + expect((await readdir(parent)).sort()).toEqual([ + unrelated, otherLeg, live, unmarked, corrupt, + ].map((root) => root.slice(parent.length + 1)).sort()); + await expect(readdir(join(live, 'cache', 'cmd-1-1'))).resolves.toEqual(['leftover']); + + // Once its owner has exited the retained root is removed on the next pass; + // a pass with nothing to do is not an error, and neither is a missing parent. + await expect(removeOwnedRstestWorkerRoots({ isAlive: () => false, parent, temporaryRoot: legTmp })) + .resolves.toEqual({ removed: [live], retained: [] }); + await expect(removeOwnedRstestWorkerRoots({ isAlive: () => false, parent, temporaryRoot: legTmp })) + .resolves.toEqual({ removed: [], retained: [] }); + await expect(removeOwnedRstestWorkerRoots({ parent: join(parent, 'missing'), temporaryRoot: legTmp })) + .resolves.toEqual({ removed: [], retained: [] }); + } finally { + await rm(parent, { force: true, recursive: true }); + } +}); diff --git a/packages/create-agent-bundle/src/scaffold.ts b/packages/create-agent-bundle/src/scaffold.ts index cd574d490..4e0cdcb6f 100644 --- a/packages/create-agent-bundle/src/scaffold.ts +++ b/packages/create-agent-bundle/src/scaffold.ts @@ -99,12 +99,75 @@ const rewriteConfigTargets = (contents: string, targets: readonly TargetName[]): return contents.replace(defaultTargetsLiteral, `targets: [${renderTargets(targets)}]`); }; +/** The hosts the generated installer bin accepts, in the package build's order. */ +const installableHosts = (targets: readonly TargetName[]): readonly TargetName[] => + (['claude', 'codex', 'cursor'] as const) + .filter((host) => targets.some((target) => target === host || target === 'plugin')); + +/** + * Template READMEs are written against the default targets, so their install + * example names `claude`. The checked-in shape is one shell comment followed + * by `npx install claude`, plus the prose sentence naming the + * ` install ` command; both markers are drift-checked. + */ +const readmeInstallExample = /^(# after publishing[^\n]*)\n(npx \S+) install claude\n/mu; +const readmeInstallProse = /^Installing the npm package does not mutate any host; run the generated\n`(\S+) install ` command explicitly\.\n/mu; + +/** + * Rewrite a template README's install instructions for the selected targets: + * one example line per installable host, or — when no `claude`, `codex`, + * `cursor`, or `plugin` target is selected and therefore no installer bin is + * generated — an explanation of how to get one. Templates without an install + * section (the skills-only template) pass through unchanged. + */ +const rewriteReadmeInstall = (contents: string, targets: readonly TargetName[]): string => { + const example = readmeInstallExample.exec(contents); + const prose = readmeInstallProse.exec(contents); + if (example === null && prose === null) return contents; + if (example === null || prose === null) { + throw new Error('Template drift: README.md install example and prose must both be present or both absent.'); + } + const hosts = installableHosts(targets); + // Every group is unconditional in the patterns above. + const comment = example[1] ?? ''; + const exampleBin = example[2] ?? ''; + const proseBin = prose[1] ?? ''; + if (hosts.length === 0) { + return contents + .replace(readmeInstallExample, [ + '# no installer bin is generated for these targets; add claude, codex, or cursor', + '# to `targets` in agent-bundle.config.ts to get one', + '', + ].join('\n')) + .replace(readmeInstallProse, [ + 'Installing the npm package does not mutate any host. This project selects', + `no installable host target (${renderTargets(targets)}), so no \`${proseBin} install\``, + 'command is generated; add `claude`, `codex`, or `cursor` to `targets` in', + '`agent-bundle.config.ts` to generate one.', + '', + ].join('\n')); + } + return contents + .replace(readmeInstallExample, [ + comment, + ...hosts.map((host) => `${exampleBin} install ${host}`), + '', + ].join('\n')) + .replace(readmeInstallProse, [ + 'Installing the npm package does not mutate any host; run the generated', + `\`${proseBin} install \` command explicitly. The installer accepts the`, + `selected host targets only: ${hosts.map((host) => `\`${host}\``).join(', ')}.`, + '', + ].join('\n')); +}; + /** * Copy one template directory into the target, substituting the placeholder * project name in every file, rewriting `package.json` (real package name, * `workspace:*` framework placeholder pinned to the resolved spec, installer - * bins omitted when no installable host is selected) and the config's target - * list. Returns the emitted project-relative paths, sorted. + * bins omitted when no installable host is selected), the config's target + * list, and the README's install instructions. Returns the emitted + * project-relative paths, sorted. */ export const scaffold = async (request: ScaffoldRequest): Promise => { const templateManifest = JSON.parse( @@ -133,6 +196,7 @@ export const scaffold = async (request: ScaffoldRequest): Promise { } }); + it('renders README install instructions for the selected targets', async () => { + const [defaults, cursorOnly, pluginOnly, portableOnly, minimal] = await Promise.all([ + scaffoldTemplate('cli-tool', { pluginName: 'greeter' }), + scaffoldTemplate('mcp-server', { pluginName: 'status-plugin', targets: ['cursor'] }), + scaffoldTemplate('mcp-server', { pluginName: 'status-plugin', targets: ['plugin'] }), + scaffoldTemplate('cli-tool', { pluginName: 'greeter', targets: ['portable'] }), + scaffoldTemplate('minimal', { pluginName: 'skills-only', targets: ['portable'] }), + ]); + try { + // Default targets (portable, codex, claude): one line per installable host, + // in the package build's host order; the template's hard-coded `claude` + // example never survives as the only instruction (#317 review). + const defaultReadme = await readFile(join(defaults.root, 'README.md'), 'utf8'); + expect(defaultReadme).toContain([ + '# after publishing/installing the package', + 'npx greeter-install install claude', + 'npx greeter-install install codex', + '', + ].join('\n')); + expect(defaultReadme).not.toContain('install cursor'); + expect(defaultReadme).toContain('The installer accepts the\nselected host targets only: `claude`, `codex`.'); + + // A cursor-only scaffold's installer rejects `claude`, so the README must + // not suggest it. + const cursorReadme = await readFile(join(cursorOnly.root, 'README.md'), 'utf8'); + expect(cursorReadme).toContain('npx status-plugin install cursor\n'); + expect(cursorReadme).not.toContain('install claude'); + expect(cursorReadme).not.toContain('install codex'); + + // The composite plugin target installs into every host. + const pluginReadme = await readFile(join(pluginOnly.root, 'README.md'), 'utf8'); + expect(pluginReadme).toContain([ + 'npx status-plugin install claude', + 'npx status-plugin install codex', + 'npx status-plugin install cursor', + ].join('\n')); + + // Portable-only scaffolds ship no installer bin at all. + const portableReadme = await readFile(join(portableOnly.root, 'README.md'), 'utf8'); + expect(portableReadme).not.toMatch(/^npx \S+ install /mu); + expect(portableReadme).toContain("no installable host target ('portable')"); + expect(portableReadme).toContain('add `claude`, `codex`, or `cursor` to `targets`'); + + // The skills-only template has no install section and passes through. + const minimalReadme = await readFile(join(minimal.root, 'README.md'), 'utf8'); + expect(minimalReadme).toBe( + (await readFile(join(templatesRoot, 'minimal', 'README.md'), 'utf8')).replaceAll(placeholderName, 'skills-only'), + ); + } finally { + await Promise.all([defaults, cursorOnly, pluginOnly, portableOnly, minimal].map( + ({ root }) => rm(root, { force: true, recursive: true }), + )); + } + }); + it('leaves the skills-only template without package-build packaging fields', async () => { const { root } = await scaffoldTemplate('minimal'); try { diff --git a/rstest.worker-isolation.ts b/rstest.worker-isolation.ts index 4329eb0e9..c1fada37a 100644 --- a/rstest.worker-isolation.ts +++ b/rstest.worker-isolation.ts @@ -1,12 +1,52 @@ import { createHash } from 'node:crypto'; -import { mkdirSync } from 'node:fs'; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { rstestWorkerRootOwnerFile } from './scripts/rstest-worker-roots.mjs'; + export const rstestWorkerId = (): string => process.env['RSTEST_WORKER_ID'] ?? '0'; const hostTemporaryRoot = tmpdir(); +/** + * Owner marker for a hashed worker root. The root's name is not predictable + * from outside (the hash includes this process id), so the marker is how a + * runner that owns `temporaryRoot` — scripts/local-ci.mjs and its per-leg + * TMPDIR — recognizes and removes the roots a finished run left behind + * without touching another run's live roots. + */ +export interface RstestWorkerRootOwner { + readonly cwd: string; + readonly pid: number; + readonly temporaryRoot: string; + readonly workerId: string; +} + +export const rstestWorkerRootOwner = (root: string): RstestWorkerRootOwner | undefined => { + const path = join(root, rstestWorkerRootOwnerFile); + if (!existsSync(path)) return undefined; + return JSON.parse(readFileSync(path, 'utf8')) as RstestWorkerRootOwner; +}; + +const writeOwnerMarker = (root: string, workerId: string): void => { + const path = join(root, rstestWorkerRootOwnerFile); + if (existsSync(path)) return; + const owner: RstestWorkerRootOwner = { + cwd: process.cwd(), + pid: process.pid, + temporaryRoot: hostTemporaryRoot, + workerId, + }; + try { + writeFileSync(path, `${JSON.stringify(owner)}\n`, { flag: 'wx' }); + } catch (error) { + // Another module of this same invocation won the race; its marker names + // the same owner. + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + } +}; + export const rstestWorkerRootPath = ( temporaryRoot: string, workerId: string, @@ -26,8 +66,10 @@ export const rstestWorkerRootPath = ( }; export const rstestWorkerRoot = (): string => { - const root = rstestWorkerRootPath(hostTemporaryRoot, rstestWorkerId()); + const workerId = rstestWorkerId(); + const root = rstestWorkerRootPath(hostTemporaryRoot, workerId); mkdirSync(root, { recursive: true }); + writeOwnerMarker(root, workerId); return root; }; diff --git a/scripts/local-ci.mjs b/scripts/local-ci.mjs index 8e9b52b61..f9a68dde2 100644 --- a/scripts/local-ci.mjs +++ b/scripts/local-ci.mjs @@ -35,7 +35,11 @@ * it (#110). The temp roots deliberately live under the SYSTEM temp * directory, not the repo worktree: Chrome creates AF_UNIX sockets inside * TMPDIR, and the kernel caps socket paths at 108 bytes — a repo-nested - * TMPDIR overflows that and crashes every browser test at launch. + * TMPDIR overflows that and crashes every browser test at launch. Rstest in + * turn derives per-worker roots (/tmp/ab-rstest-) BESIDE the leg + * TMPDIR for the same reason; those roots carry an owner marker naming the + * leg TMPDIR, and the runner removes the ones it owns before and after each + * leg (scripts/rstest-worker-roots.mjs) so reruns cannot accumulate them. */ import { createHash } from 'node:crypto'; import { spawn } from 'node:child_process'; @@ -47,6 +51,8 @@ import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { promisify } from 'node:util'; +import { removeOwnedRstestWorkerRoots } from './rstest-worker-roots.mjs'; + const execFile = promisify(executeFile); const repositoryRoot = resolve(dirname(fileURLToPath(import.meta.url)), '..'); @@ -282,17 +288,40 @@ const runStep = async (leg, step, stepIndex) => { }; /** Steps run sequentially inside a leg; a failure skips the leg's remaining steps (matching a hosted job's step semantics). */ +/** + * Rstest derives each worker's temp root as `/tmp/ab-rstest-` beside + * (not inside) the leg's private TMPDIR — see rstest.worker-isolation.ts and + * docs/local-ci.md — so recreating the leg TMPDIR alone would let worker + * caches and interrupted-test fixtures accumulate under /tmp across reruns. + * Each root carries an owner marker naming the host TMPDIR it was derived + * from; removing the roots owned by this leg's TMPDIR (whose creating + * processes have exited) ties their lifetime to the leg without touching any + * other run's live roots. + */ +const removeLegWorkerRoots = async (temporaryDirectory, legName, phase) => { + const { removed, retained } = await removeOwnedRstestWorkerRoots({ temporaryRoot: temporaryDirectory }); + if (removed.length > 0) console.log(` ${legName}: removed ${removed.length} ${phase} rstest worker root(s) under /tmp`); + if (retained.length > 0) { + console.warn(` ${legName}: retained ${retained.length} rstest worker root(s) whose owning process is still alive`); + } + return { removed, retained }; +}; + const runLeg = async (leg) => { const results = []; let failed = false; - for (const [index, step] of leg.steps.entries()) { - if (failed) { - results.push({ leg: leg.name, step: step.id, hostedJob: step.hostedJob, status: 'skipped', durationMs: 0 }); - continue; + try { + for (const [index, step] of leg.steps.entries()) { + if (failed) { + results.push({ leg: leg.name, step: step.id, hostedJob: step.hostedJob, status: 'skipped', durationMs: 0 }); + continue; + } + const result = await runStep(leg, step, index); + results.push(result); + if (result.status === 'fail') failed = true; } - const result = await runStep(leg, step, index); - results.push(result); - if (result.status === 'fail') failed = true; + } finally { + await removeLegWorkerRoots(leg.temporaryDirectory, leg.name, 'finished'); } return results; }; @@ -403,11 +432,15 @@ const main = async () => { const repositoryHash = createHash('sha256').update(repositoryRoot).digest('hex').slice(0, 8); const temporaryDirectory = join(tmpdir(), `abci-${repositoryHash}-${plan.name}`); await rm(temporaryDirectory, { recursive: true, force: true }); + // Hashed rstest worker roots a crashed or interrupted prior run of this + // same leg left under /tmp are owned by this TMPDIR too. + await removeLegWorkerRoots(temporaryDirectory, plan.name, 'stale'); await mkdir(temporaryDirectory, { recursive: true }); legs.push({ ...plan, directory, syntheticBinDirectory, + temporaryDirectory, environment: buildLegEnvironment(syntheticBinDirectory, { ...plan.environmentOverrides, TMPDIR: temporaryDirectory, diff --git a/scripts/rstest-worker-roots.d.mts b/scripts/rstest-worker-roots.d.mts new file mode 100644 index 000000000..55676ddd3 --- /dev/null +++ b/scripts/rstest-worker-roots.d.mts @@ -0,0 +1,23 @@ +interface RemoveOwnedRstestWorkerRootsOptions { + /** Liveness probe for the owning process id; defaults to `process.kill(pid, 0)`. */ + isAlive?: (pid: number) => boolean; + /** Directory scanned for worker roots; defaults to `rstestWorkerRootsParent`. */ + parent?: string; + /** The host `TMPDIR` whose derived worker roots may be removed. */ + temporaryRoot: string; +} + +interface RemoveOwnedRstestWorkerRootsResult { + removed: string[]; + retained: string[]; +} + +export declare const rstestWorkerRootsParent: string; + +export declare const rstestWorkerRootPrefix: string; + +export declare const rstestWorkerRootOwnerFile: string; + +export declare const removeOwnedRstestWorkerRoots: ( + options: RemoveOwnedRstestWorkerRootsOptions, +) => Promise; diff --git a/scripts/rstest-worker-roots.mjs b/scripts/rstest-worker-roots.mjs new file mode 100644 index 000000000..2bd1c078c --- /dev/null +++ b/scripts/rstest-worker-roots.mjs @@ -0,0 +1,72 @@ +/** + * Ownership and cleanup of the hashed Rstest worker roots. + * + * `rstest.worker-isolation.ts` derives each worker's private temp root as + * `/tmp/ab-rstest-` — directly under the system temp directory, never + * under the host `TMPDIR`, because Chrome and the Doctor socket fixtures create + * AF_UNIX sockets inside it and Linux caps socket paths at 108 bytes. The hash + * includes the invoking process id, so a runner such as `scripts/local-ci.mjs` + * cannot predict the paths a finished leg created. Every root therefore carries + * an owner marker naming the host `TMPDIR` it was derived from and the process + * that created it; a runner that owns that `TMPDIR` can remove exactly those + * roots once the run has finished, and nothing else. + */ +import { readdir, readFile, rm } from 'node:fs/promises'; +import { join } from 'node:path'; + +/** Parent directory of every non-Windows worker root (see rstestWorkerRootPath). */ +export const rstestWorkerRootsParent = '/tmp'; +/** Directory-name prefix of every non-Windows worker root. */ +export const rstestWorkerRootPrefix = 'ab-rstest-'; +/** Owner marker written into each worker root by `rstestWorkerRoot()`. */ +export const rstestWorkerRootOwnerFile = '.ab-rstest-owner.json'; + +const processIsAlive = (pid) => { + try { + process.kill(pid, 0); + return true; + } catch (error) { + // EPERM means the process exists but belongs to another user. + return error !== null && typeof error === 'object' && error.code === 'EPERM'; + } +}; + +/** + * Remove the worker roots owned by a finished run: those whose owner marker + * names `temporaryRoot` as the host `TMPDIR` they were derived from and whose + * creating process has exited. Roots without a readable marker, roots owned by + * another `TMPDIR`, and roots whose owning process is still alive are never + * touched. Returns the roots removed and the live roots retained. + */ +export const removeOwnedRstestWorkerRoots = async (options) => { + const parent = options.parent ?? rstestWorkerRootsParent; + const isAlive = options.isAlive ?? processIsAlive; + const removed = []; + const retained = []; + let entries; + try { + entries = await readdir(parent, { withFileTypes: true }); + } catch { + return { removed, retained }; + } + for (const entry of entries) { + if (!entry.isDirectory() || !entry.name.startsWith(rstestWorkerRootPrefix)) continue; + const root = join(parent, entry.name); + let owner; + try { + owner = JSON.parse(await readFile(join(root, rstestWorkerRootOwnerFile), 'utf8')); + } catch { + continue; + } + if (owner === null || typeof owner !== 'object' || owner.temporaryRoot !== options.temporaryRoot) continue; + if (Number.isSafeInteger(owner.pid) && owner.pid > 0 && isAlive(owner.pid)) { + retained.push(root); + continue; + } + await rm(root, { recursive: true, force: true }); + removed.push(root); + } + removed.sort(); + retained.sort(); + return { removed, retained }; +}; From e39f3f117e5d4b6551015d8ee1b448ee2f5bf24f Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 06:01:07 +0000 Subject: [PATCH 02/19] fix(doctor): probe runtime endpoints concurrently; docs: scope the canvas silence claim; changeset --- .changeset/review-threads-wave4.md | 21 ++ .../agent-bundle-walkthrough.canvas.tsx | 2 +- packages/agent-bundle/src/install/doctor.ts | 298 +++++++++++------- packages/agent-bundle/tests/doctor.test.ts | 32 ++ 4 files changed, 239 insertions(+), 114 deletions(-) create mode 100644 .changeset/review-threads-wave4.md diff --git a/.changeset/review-threads-wave4.md b/.changeset/review-threads-wave4.md new file mode 100644 index 000000000..07d9d49d1 --- /dev/null +++ b/.changeset/review-threads-wave4.md @@ -0,0 +1,21 @@ +--- +"agent-bundle": patch +"create-agent-bundle": patch +--- + +Address the fourth wave of review findings. Rendered CLI commands, projected +MCP commands, and rendered scripts now forward the dispatched `invocation` to +the Flight worker, so conventional context providers that branch on +`invocation.kind` receive it instead of `undefined`. The transcribed Codex +plugin schema rejects line terminators in component, interface-asset, and +screenshot paths so a newline followed by `/../` cannot slip past the traversal +guard (codex adapter 1.7.0, plugin adapter 1.22.0). Any-JSON `permission/request` +`tool_input` is admitted only for Codex, whose pinned schema declares it; Claude +envelopes stay object-shaped. `agent-bundle build --help` and `prepack --help` +describe the `artifact` default those commands actually use. The MCP probe +redaction no longer treats a URI scheme's `://` as a path separator, so +documentation links survive, and plugin-data removal now follows the transport +teardown instead of racing a slow stdio shutdown. Doctor probes runtime +endpoints concurrently (eight at a time), bounding a directory of silent +runtimes as a whole. Scaffolded READMEs render install instructions for the +selected targets rather than a hard-coded `install claude`. diff --git a/docs/canvases/agent-bundle-walkthrough.canvas.tsx b/docs/canvases/agent-bundle-walkthrough.canvas.tsx index 557649336..dbf934a78 100644 --- a/docs/canvases/agent-bundle-walkthrough.canvas.tsx +++ b/docs/canvases/agent-bundle-walkthrough.canvas.tsx @@ -644,7 +644,7 @@ export default function AgentBundleWalkthrough() { n={5} title="Thin client prints the host-native response and exits 0" channel="wrapper → Claude · stdout" - note="Claude blocks the Write and surfaces the reason to the model. On tool/before the wrapper always answers: an explicit hookSpecificOutput.permissionDecision ('allow' unless the route denied, optionally with updatedInput / additionalContext) — even when the route renders no decision. Silence is reserved for observation-only families such as session/end." + note="Claude blocks the Write and surfaces the reason to the model. On tool/before the wrapper always answers: an explicit hookSpecificOutput.permissionDecision ('allow' unless the route denied, optionally with updatedInput / additionalContext) — even when the route renders no decision. That explicit-allow rule is specific to Claude/Codex tool/before: other families, including decision-capable ones such as stop and prompt/submit, project undefined (silence) when the route neither denies nor adds context, and observation-only families such as session/end are always silent." payload={WIRE_STDOUT} last /> diff --git a/packages/agent-bundle/src/install/doctor.ts b/packages/agent-bundle/src/install/doctor.ts index a12982136..9643d71e4 100644 --- a/packages/agent-bundle/src/install/doctor.ts +++ b/packages/agent-bundle/src/install/doctor.ts @@ -8,6 +8,7 @@ import { type Diagnostic, type DiagnosticSeverity, } from '../core/diagnostics.ts'; +import { mapConcurrent } from '../core/async.ts'; import { isErrno } from '../core/errors.ts'; import { exists } from '../core/paths.ts'; import { validateClaudePluginFiles } from '../host-contracts/claude-plugin-validation.ts'; @@ -1038,6 +1039,155 @@ const probeEndpoint = (path: string): Promise => new Promise((res socket.once('error', onError); }); +/** + * Endpoints are probed concurrently, this many at a time, so a directory of + * listeners that accept connections but never answer (the silent-runtime + * failure Doctor exists to diagnose) costs roughly one status-probe timeout + * per batch rather than one per endpoint (#324 review). + */ +export const doctorEndpointProbeConcurrency = 8; +const doctorRuntimeStatusTimeoutMs = 1_000; + +interface EndpointInspection { + readonly diagnostics: readonly Diagnostic[]; + readonly finding?: DoctorFinding; + readonly live: number; + readonly staleLocks: number; + readonly staleSockets: number; +} + +const quietInspection: EndpointInspection = Object.freeze({ + diagnostics: Object.freeze([]), + live: 0, + staleLocks: 0, + staleSockets: 0, +}); + +const inspectSocketEndpoint = async (path: string): Promise => { + try { + const state = await probeEndpoint(path); + if (state === 'missing') return quietInspection; + if (state === 'live') { + const diagnostics: Diagnostic[] = []; + let runtime: DoctorRuntimeStatus; + try { + const probed = await requestEventRuntimeStatus({ endpoint: path, timeoutMs: doctorRuntimeStatusTimeoutMs }); + runtime = probed; + if (probed.status === 'unsupported') { + diagnostics.push(diagnostic( + 'AB7317', + `Runtime socket ${JSON.stringify(path)} predates read-only runtime identity introspection.`, + 'Restart the runtime after upgrading Agent Bundle to expose its process-lifetime identity.', + 'info', + )); + } else if (probed.status === 'unavailable') { + diagnostics.push(diagnostic( + 'AB7318', + `Runtime socket ${JSON.stringify(path)} became unavailable during its status probe.`, + 'Restart the runtime or inspect the socket, then rerun Doctor.', + 'error', + )); + } + } catch (error) { + runtime = Object.freeze({ status: 'failed' }); + diagnostics.push(diagnostic( + 'AB7318', + `Runtime socket ${JSON.stringify(path)} status probe failed: ` + + `${error instanceof Error ? error.message : String(error)}`, + 'Inspect the runtime protocol and socket responsiveness, then rerun Doctor.', + 'error', + )); + } + return { ...quietInspection, diagnostics, finding: { path, runtime, state: 'live' }, live: 1 }; + } + return { + ...quietInspection, + diagnostics: [diagnostic( + 'AB7314', + `Runtime socket ${JSON.stringify(path)} refuses connections and is stale.`, + 'Remove the stale socket manually or start the runtime; Doctor never removes it.', + 'warning', + )], + finding: { path, state: 'stale-socket' }, + staleSockets: 1, + }; + } catch (error) { + return { + ...quietInspection, + diagnostics: [diagnostic( + 'AB7315', + `Runtime socket ${JSON.stringify(path)} could not be probed: ` + + `${error instanceof Error ? error.message : String(error)}`, + 'Inspect the socket and directory permissions, then rerun Doctor.', + 'error', + )], + }; + } +}; + +const inspectLockEndpoint = async (path: string, platform: NodeJS.Platform): Promise => { + const sibling = path.slice(0, -'.lock'.length); + try { + const siblingState = await probeEndpoint(sibling); + if (siblingState === 'live') return { ...quietInspection, finding: { path, state: 'live' } }; + let rawOwner: string; + try { + rawOwner = await readFile(path, 'utf8'); + } catch (error) { + if (isErrno(error, 'ENOENT')) return quietInspection; + throw error; + } + const owner = parseEndpointClaimOwner(rawOwner); + if (owner === undefined) { + return { + ...quietInspection, + diagnostics: [diagnostic( + 'AB7314', + `Runtime claim ${JSON.stringify(path)} has no valid owner record, so the runtime cannot verify it and fails closed.`, + 'After verifying no runtime is starting, remove the lock manually.', + 'warning', + )], + finding: { path, state: 'stale-lock' }, + staleLocks: 1, + }; + } + if (await isEndpointClaimOwnerProvablyDead(owner, platform)) { + return { + ...quietInspection, + diagnostics: [diagnostic( + 'AB7314', + `Runtime claim ${JSON.stringify(path)} is orphaned because owner pid ${owner.pid} is provably dead.`, + 'The runtime reclaims provably-dead claims automatically at the next start, or remove the lock manually.', + 'warning', + )], + finding: { path, state: 'stale-lock' }, + staleLocks: 1, + }; + } + return { + ...quietInspection, + diagnostics: [diagnostic( + 'AB7314', + `Runtime claim ${JSON.stringify(path)} is held by pid ${owner.pid}, which cannot be proven dead, so the runtime fails closed rather than stealing it.`, + 'If runtimes hang at startup, verify the owning process and remove the lock manually only once it is gone.', + 'info', + )], + finding: { path, state: 'live' }, + }; + } catch (error) { + return { + ...quietInspection, + diagnostics: [diagnostic( + 'AB7315', + `Runtime claim ${JSON.stringify(path)} could not be inspected: ` + + `${error instanceof Error ? error.message : String(error)}`, + 'Inspect the claim and directory permissions, then rerun Doctor.', + 'error', + )], + }; + } +}; + const scanEndpoints = async ( directory: string, platform: NodeJS.Platform, @@ -1084,124 +1234,46 @@ const scanEndpoints = async ( summary: Object.freeze({ live: 0, staleLocks: 0, staleSockets: 0 }), }); } + // Every endpoint is inspected independently and the per-entry results are + // stitched back together in directory order, so the report is byte-stable + // regardless of which probe answers first. + const socketEntries = entries.filter((entry) => /^event-.+\.sock$/u.test(entry)); + const lockEntries = entries.filter((candidate) => /^event-.+\.lock$/u.test(candidate)); + const socketResults: EndpointInspection[] = new Array(socketEntries.length); + const lockResults: EndpointInspection[] = new Array(lockEntries.length); + await mapConcurrent( + [ + ...socketEntries.map((entry, index) => ({ entry, index, kind: 'socket' as const })), + ...lockEntries.map((entry, index) => ({ entry, index, kind: 'lock' as const })), + ], + doctorEndpointProbeConcurrency, + async ({ entry, index, kind }) => { + const path = join(directory, entry); + switch (kind) { + case 'socket': + socketResults[index] = await inspectSocketEndpoint(path); + break; + case 'lock': + lockResults[index] = await inspectLockEndpoint(path, platform); + break; + default: { + const exhaustive: never = kind; + throw new TypeError(`Unknown endpoint entry kind ${String(exhaustive)}.`); + } + } + }, + ); const findings: DoctorFinding[] = []; const diagnostics: Diagnostic[] = []; let live = 0; let staleLocks = 0; let staleSockets = 0; - const socketEntries = entries.filter((entry) => /^event-.+\.sock$/u.test(entry)); - for (const entry of socketEntries) { - const path = join(directory, entry); - try { - const state = await probeEndpoint(path); - if (state === 'missing') continue; - if (state === 'live') { - live += 1; - let runtime: DoctorRuntimeStatus; - try { - const probed = await requestEventRuntimeStatus({ endpoint: path, timeoutMs: 1_000 }); - runtime = probed; - if (probed.status === 'unsupported') { - diagnostics.push(diagnostic( - 'AB7317', - `Runtime socket ${JSON.stringify(path)} predates read-only runtime identity introspection.`, - 'Restart the runtime after upgrading Agent Bundle to expose its process-lifetime identity.', - 'info', - )); - } else if (probed.status === 'unavailable') { - diagnostics.push(diagnostic( - 'AB7318', - `Runtime socket ${JSON.stringify(path)} became unavailable during its status probe.`, - 'Restart the runtime or inspect the socket, then rerun Doctor.', - 'error', - )); - } - } catch (error) { - runtime = Object.freeze({ status: 'failed' }); - diagnostics.push(diagnostic( - 'AB7318', - `Runtime socket ${JSON.stringify(path)} status probe failed: ` + - `${error instanceof Error ? error.message : String(error)}`, - 'Inspect the runtime protocol and socket responsiveness, then rerun Doctor.', - 'error', - )); - } - findings.push({ path, runtime, state: 'live' }); - continue; - } - staleSockets += 1; - findings.push({ path, state: 'stale-socket' }); - diagnostics.push(diagnostic( - 'AB7314', - `Runtime socket ${JSON.stringify(path)} refuses connections and is stale.`, - 'Remove the stale socket manually or start the runtime; Doctor never removes it.', - 'warning', - )); - } catch (error) { - diagnostics.push(diagnostic( - 'AB7315', - `Runtime socket ${JSON.stringify(path)} could not be probed: ` + - `${error instanceof Error ? error.message : String(error)}`, - 'Inspect the socket and directory permissions, then rerun Doctor.', - 'error', - )); - } - } - for (const entry of entries.filter((candidate) => /^event-.+\.lock$/u.test(candidate))) { - const path = join(directory, entry); - const sibling = path.slice(0, -'.lock'.length); - try { - const siblingState = await probeEndpoint(sibling); - if (siblingState === 'live') { - findings.push({ path, state: 'live' }); - continue; - } - let rawOwner: string; - try { - rawOwner = await readFile(path, 'utf8'); - } catch (error) { - if (isErrno(error, 'ENOENT')) continue; - throw error; - } - const owner = parseEndpointClaimOwner(rawOwner); - if (owner === undefined) { - staleLocks += 1; - findings.push({ path, state: 'stale-lock' }); - diagnostics.push(diagnostic( - 'AB7314', - `Runtime claim ${JSON.stringify(path)} has no valid owner record, so the runtime cannot verify it and fails closed.`, - 'After verifying no runtime is starting, remove the lock manually.', - 'warning', - )); - continue; - } - if (await isEndpointClaimOwnerProvablyDead(owner, platform)) { - staleLocks += 1; - findings.push({ path, state: 'stale-lock' }); - diagnostics.push(diagnostic( - 'AB7314', - `Runtime claim ${JSON.stringify(path)} is orphaned because owner pid ${owner.pid} is provably dead.`, - 'The runtime reclaims provably-dead claims automatically at the next start, or remove the lock manually.', - 'warning', - )); - continue; - } - findings.push({ path, state: 'live' }); - diagnostics.push(diagnostic( - 'AB7314', - `Runtime claim ${JSON.stringify(path)} is held by pid ${owner.pid}, which cannot be proven dead, so the runtime fails closed rather than stealing it.`, - 'If runtimes hang at startup, verify the owning process and remove the lock manually only once it is gone.', - 'info', - )); - } catch (error) { - diagnostics.push(diagnostic( - 'AB7315', - `Runtime claim ${JSON.stringify(path)} could not be inspected: ` + - `${error instanceof Error ? error.message : String(error)}`, - 'Inspect the claim and directory permissions, then rerun Doctor.', - 'error', - )); - } + for (const inspection of [...socketResults, ...lockResults]) { + if (inspection.finding !== undefined) findings.push(inspection.finding); + diagnostics.push(...inspection.diagnostics); + live += inspection.live; + staleLocks += inspection.staleLocks; + staleSockets += inspection.staleSockets; } const frozenDiagnostics = freezeDiagnostics(diagnostics); return Object.freeze({ diff --git a/packages/agent-bundle/tests/doctor.test.ts b/packages/agent-bundle/tests/doctor.test.ts index c38beadc1..85e8c5364 100644 --- a/packages/agent-bundle/tests/doctor.test.ts +++ b/packages/agent-bundle/tests/doctor.test.ts @@ -10,6 +10,7 @@ import { runCli } from '../src/cli.ts'; import { eventRuntimeEndpoint } from '../src/events/ipc.ts'; import { doctorEndpointDirectory, + doctorEndpointProbeConcurrency, runDoctor, type DoctorCommandRunner, type DoctorHost, @@ -1140,6 +1141,37 @@ it('bounds a silent runtime status probe', async () => { } }); +it('bounds a directory of silent runtimes as a whole by probing endpoints concurrently', async () => { + const fixture = await temporaryDoctor(); + // Twice the concurrency cap would still be far below the serial cost: + // probed one at a time these would take at least `count` seconds. + const count = 6; + const endpoints = Array.from({ length: count }, (_, index) => + join(fixture.endpointDirectory, `event-silent-${String(index)}.sock`)); + const servers = await Promise.all(endpoints.map((endpoint) => listen(endpoint))); + try { + expect(count).toBeLessThanOrEqual(doctorEndpointProbeConcurrency); + const started = Date.now(); + const report = await runDoctor({ + endpointDirectory: fixture.endpointDirectory, + home: fixture.home, + hosts: [], + }); + const elapsed = Date.now() - started; + // One batch of 1 s status-probe timeouts plus slack, not N seconds. + expect(elapsed).toBeLessThan(3_500); + // Every silent endpoint is still reported, in directory order. + expect(report.endpoints.findings.map((finding) => finding.path)).toEqual([...endpoints].sort((left, right) => left.localeCompare(right))); + expect(report.endpoints.findings).toEqual(endpoints.map(() => + expect.objectContaining({ runtime: { status: 'failed' }, state: 'live' }))); + expect(report.diagnostics.filter((entry) => entry.code === 'AB7318')).toHaveLength(count); + expect(report.endpoints.summary).toMatchObject({ live: count, staleLocks: 0, staleSockets: 0 }); + } finally { + await Promise.all(servers.map((server) => close(server))); + await fixture.cleanup(); + } +}); + it('reports stale sockets and stale locks as warnings', async () => { const fixture = await temporaryDoctor(); const staleSocket = join(fixture.endpointDirectory, 'event-stale.sock'); From 6b921782e17907ff9c5df792e45f37c1aed99d9a Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 06:03:56 +0000 Subject: [PATCH 03/19] chore: changeset revision text after rebase --- .changeset/review-threads-wave4.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/review-threads-wave4.md b/.changeset/review-threads-wave4.md index 07d9d49d1..83e6822de 100644 --- a/.changeset/review-threads-wave4.md +++ b/.changeset/review-threads-wave4.md @@ -9,7 +9,7 @@ the Flight worker, so conventional context providers that branch on `invocation.kind` receive it instead of `undefined`. The transcribed Codex plugin schema rejects line terminators in component, interface-asset, and screenshot paths so a newline followed by `/../` cannot slip past the traversal -guard (codex adapter 1.7.0, plugin adapter 1.22.0). Any-JSON `permission/request` +guard (codex adapter 1.8.0, plugin adapter 1.23.0). Any-JSON `permission/request` `tool_input` is admitted only for Codex, whose pinned schema declares it; Claude envelopes stay object-shaped. `agent-bundle build --help` and `prepack --help` describe the `artifact` default those commands actually use. The MCP probe From 2263156c749f7b05cdd9d1b13b6f8462fb360aad Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 06:06:46 +0000 Subject: [PATCH 04/19] fix(probe): never await a stalled close on the exhausted-budget path --- .../src/dev/playground/mcp-probe-service.ts | 5 ++- .../tests/mcp-probe-service.test.ts | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 1f747629c..59de57c0e 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -603,7 +603,10 @@ export class McpProbeService { ): Promise { const remaining = Math.max(0, this.#timeoutMs - (this.#clock() - startedAt)); if (remaining === 0) { - await onTimeout().catch(() => undefined); + // Same contract as the timer path below: the transport's own close + // (TERM/KILL for stdio) keeps running, but a stalled close never holds + // the timed-out report — #execute's bounded teardown owns that wait. + void onTimeout().catch(() => undefined); throw new McpProbeTimeoutError(kind); } let timer: NodeJS.Timeout | undefined; diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index b2f66a49e..a237e2d1b 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -330,6 +330,46 @@ it('returns a timed-out report without awaiting stalled teardown', async () => { } }); +it('returns a timed-out report without awaiting stalled teardown when the budget is spent before connecting', async () => { + const root = await createBundle(); + let transportCloses = 0; + let guard: NodeJS.Timeout | undefined; + let ticks = 0; + try { + const service = serviceFor(root, { + // The clock reads 0 at probe start and the whole budget later at every + // subsequent read, so the connect step finds no time remaining. + clock: () => (ticks++ === 0 ? 0 : 10_000), + createClient: () => client({ + close: async () => new Promise((resolvePromise) => setTimeout(resolvePromise, 250)), + connect: () => new Promise(() => undefined), + }), + createStdioTransport: () => transport(async () => { + transportCloses += 1; + await new Promise((resolvePromise) => setTimeout(resolvePromise, 250)); + }), + timeoutMs: 10, + }); + + const report = await Promise.race([ + service.probe({ host: 'claude', serverName: 'timeline' }), + new Promise((_resolve, reject) => { + guard = setTimeout( + () => reject(new Error('The budget-exhausted probe remained blocked on teardown.')), + 150, + ); + }), + ]); + + expect(report.status).toBe('timed-out'); + expect(report.failure?.kind).toBe('connect'); + expect(transportCloses).toBeGreaterThan(0); + } finally { + if (guard !== undefined) clearTimeout(guard); + await rm(root, { force: true, recursive: true }); + } +}); + it('coalesces only identical in-flight probes and clears them after settlement', async () => { const root = await createBundle(); const connectStarted = Promise.withResolvers(); From 5d51483fc464a4c91ac5faf1bd35fd0d6e29958d Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 06:40:48 +0000 Subject: [PATCH 05/19] fix(dev): join detached probe plugin-data cleanup into Workbench shutdown Keep the probe teardown cap referenced and await McpProbeService.settle() from the dev server close path so a stalled stdio teardown can no longer leave a plugin-data directory behind when Workbench shuts down. --- .../src/dev/playground/mcp-probe-service.ts | 4 +- .../agent-bundle/src/dev/workbench-server.ts | 11 ++- .../tests/mcp-probe-dev-server.test.ts | 95 +++++++++++++++++++ 3 files changed, 107 insertions(+), 3 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 59de57c0e..44792e530 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -375,9 +375,11 @@ export class McpProbeService { */ #removePluginDataAfter(teardown: Promise, pluginData: string): Promise { let cap: NodeJS.Timeout | undefined; + // The cap stays referenced on purpose: it is the only handle guaranteeing + // the removal runs when a stalled teardown outlives Workbench shutdown, + // and it is cleared the moment the teardown settles. const capped = new Promise((resolvePromise) => { cap = setTimeout(resolvePromise, mcpProbePluginDataTeardownCapMs); - cap.unref(); }); const pending = Promise.race([teardown, capped]) .then(() => { diff --git a/packages/agent-bundle/src/dev/workbench-server.ts b/packages/agent-bundle/src/dev/workbench-server.ts index 01de56aee..75a3e731e 100644 --- a/packages/agent-bundle/src/dev/workbench-server.ts +++ b/packages/agent-bundle/src/dev/workbench-server.ts @@ -893,14 +893,21 @@ export const startDevServer = async (options: StartDevServerOptions): Promise => { + const closeForeground = async (): Promise => { foregroundClosing = true; // Fence authenticated runtime routes before the foreground begins closing; // the lifecycle retains the service for its ordered preview cleanup. void appPreviews.prepareClose().catch(() => undefined); void mcpApps?.prepareClose().catch(() => undefined); clientSurfaces.beginClose(); - return foreground.close(); + try { + await foreground.close(); + } finally { + // Probe transports whose teardown outlived their response boundary own + // their plugin-data removal; joining them here (bounded by the probe's + // teardown cap) keeps shutdown from leaving those directories behind. + await mcpProbe.settle(); + } }; try { const sandbox = await (options.testing?.createSandboxProxy ?? createMcpAppSandboxProxy)({ hostOrigin: foreground.url }); diff --git a/packages/agent-bundle/tests/mcp-probe-dev-server.test.ts b/packages/agent-bundle/tests/mcp-probe-dev-server.test.ts index be446fcf9..6713f7f72 100644 --- a/packages/agent-bundle/tests/mcp-probe-dev-server.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-dev-server.test.ts @@ -104,3 +104,98 @@ it('runs an authenticated initialize and tools/list probe against a real built s await rm(project.root, { force: true, maxRetries: 5, recursive: true, retryDelay: 50 }); } }); + +it('joins detached probe plugin-data cleanup into Workbench shutdown', { timeout: 30_000 }, async () => { + const project = await createProjectFixture({ + config: [ + 'export default {', + ' mcp: {', + ' servers: {', + " timeline: { entry: { prebuilt: './prebuilt/runtime/mcp/stdio.js' }, transport: 'stdio' },", + ' },', + ' },', + " payload: { runtime: './prebuilt/runtime' },", + " plugin: { name: 'mcp-probe-dev-server-shutdown', version: '1.0.0' },", + " targets: ['claude'],", + '};', + '', + ].join('\n'), + files: { 'package.json': '{"type":"module"}\n' }, + prefix: 'agent-bundle-mcp-probe-dev-server-shutdown-', + }); + const assetsRoot = join(project.root, 'workbench'); + const exampleRuntime = join(import.meta.dirname, '..', '..', '..', 'examples', 'rsc-agent-runtime', 'dist', 'runtime'); + const pluginData = join(project.root, 'probe-plugin-data'); + let transportCloseSettled = false; + let server: Awaited> | undefined; + await Promise.all([ + mkdir(assetsRoot, { recursive: true }), + mkdir(join(project.root, 'dist'), { recursive: true }), + mkdir(join(project.root, 'prebuilt'), { recursive: true }), + ]); + await Promise.all([ + cp(exampleRuntime, join(project.root, 'prebuilt', 'runtime'), { recursive: true }), + symlink(agentBundleNodeModules, join(project.root, 'node_modules'), 'dir'), + writeFile(join(assetsRoot, 'index.html'), 'MCP probe'), + ]); + try { + const built = await build({ output: join(project.root, 'dist'), root: project.root }); + expect(built.diagnostics.filter((entry) => entry.severity === 'error')).toEqual([]); + server = await startDevServer({ + assets: createWorkbenchAssetSource({ root: assetsRoot }), + open: false, + port: 0, + root: project.root, + testing: { + mcpProbeOptions: { + createClient: () => ({ + close: async () => undefined, + connect: async () => undefined, + getInstructions: () => undefined, + getNegotiatedProtocolVersion: () => '2025-11-25', + getServerCapabilities: () => ({ tools: {} }), + getServerVersion: () => ({ name: 'timeline', version: '1.0.0' }), + listTools: async () => ({ tools: [] }), + }), + createPluginData: async () => { + await mkdir(pluginData, { recursive: true }); + await writeFile(join(pluginData, 'proof.txt'), 'present'); + return pluginData; + }, + // A stdio server whose shutdown outlives the 50 ms response boundary. + createStdioTransport: () => ({ + close: async () => { + await new Promise((resolvePromise) => setTimeout(resolvePromise, 400)); + transportCloseSettled = true; + }, + send: async () => undefined, + start: async () => undefined, + }), + }, + }, + }); + const bootstrap = await fetch(`${server.url}/api/project/session`, { headers: { 'sec-fetch-site': 'same-origin' } }); + const { token } = await bootstrap.json() as { readonly token: string }; + const response = await fetch(`${server.url}/api/discovery/probes`, { + body: JSON.stringify({ host: 'claude', serverName: 'timeline' }), + headers: { 'content-type': 'application/json', origin: server.url, 'x-agent-bundle-session': token }, + method: 'POST', + }); + expect(response.status).toBe(200); + expect((await response.json() as McpProbeReport).status).toBe('ok'); + // The response returned while the transport was still closing, so the + // plugin data is still held... + expect(transportCloseSettled).toBe(false); + await access(join(pluginData, 'proof.txt')); + + // ...and Workbench shutdown joins the detached teardown before resolving. + const closing = server; + server = undefined; + await closing.close(); + expect(transportCloseSettled).toBe(true); + await expect(access(join(pluginData, 'proof.txt'))).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + await server?.close().catch(() => undefined); + await rm(project.root, { force: true, maxRetries: 5, recursive: true, retryDelay: 50 }); + } +}); From 5f3e885b4fd5c5c293eff48c50e4b5ad403810e2 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 07:09:13 +0000 Subject: [PATCH 06/19] fix(dev): fence in-flight probes in McpProbeService.settle() A probe registers its detached teardown only at its response boundary, so a settle fence over teardowns alone let Workbench shutdown resolve while a still-connecting probe held a transport and plugin-data directory. settle() now also awaits in-flight probes. --- .../src/dev/playground/mcp-probe-service.ts | 14 +++-- .../tests/mcp-probe-service.test.ts | 53 +++++++++++++++++++ 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 44792e530..2827d3bda 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -298,13 +298,17 @@ export class McpProbeService { } /** - * Resolve once every detached teardown — a transport close that outlived - * its probe's response boundary, followed by that probe's plugin-data - * removal — has settled. Probe responses never wait on this. + * Resolve once every in-flight probe has answered and every detached + * teardown — a transport close that outlived its probe's response boundary, + * followed by that probe's plugin-data removal — has settled. In-flight + * probes are part of the fence because a probe registers its teardown only + * when it reaches its response boundary; a fence over teardowns alone would + * let shutdown finish while a still-connecting probe holds a transport and a + * plugin-data directory. Probe responses never wait on this. */ async settle(): Promise { - while (this.#pendingTeardowns.size > 0) { - await Promise.allSettled([...this.#pendingTeardowns]); + while (this.#inFlight.size > 0 || this.#pendingTeardowns.size > 0) { + await Promise.allSettled([...this.#inFlight.values(), ...this.#pendingTeardowns]); } } diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index a237e2d1b..997716b3f 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -482,6 +482,59 @@ it('removes plugin data only after a slow transport teardown settles (#316 revie } }); +it('settle() fences in-flight probes, not only already-registered teardowns (#397 review)', async () => { + const root = await createBundle(); + let pluginData: string | undefined; + const events: string[] = []; + let releaseConnect!: () => void; + const connectReleased = new Promise((resolvePromise) => { + releaseConnect = resolvePromise; + }); + try { + const service = serviceFor(root, { + createClient: () => client({ + connect: async () => { + // A probe that is still connecting when shutdown begins: its + // teardown is not registered yet, so a fence over teardowns alone + // would resolve immediately. + await connectReleased; + }, + }), + createPluginData: async () => { + pluginData = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-probe-data-')); + await writeFile(join(pluginData, 'proof.txt'), 'present'); + return pluginData; + }, + createStdioTransport: () => transport(async () => { + await new Promise((resolvePromise) => setTimeout(resolvePromise, 120)); + events.push('transport-closed'); + }), + }); + + const probe = service.probe({ host: 'claude', serverName: 'timeline' }); + void probe.then(() => { events.push('report-returned'); }); + // Let the probe reach its (blocked) connect before shutdown starts. + await new Promise((resolvePromise) => setTimeout(resolvePromise, 20)); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).resolves.toBe('present'); + + let settled = false; + const settle = service.settle().then(() => { settled = true; }); + await new Promise((resolvePromise) => setTimeout(resolvePromise, 50)); + expect(settled).toBe(false); + + releaseConnect(); + await settle; + events.push('settled'); + const report = await probe; + expect(report.status).toBe('ok'); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); + expect(events).toEqual(['report-returned', 'transport-closed', 'settled']); + } finally { + if (pluginData !== undefined) await rm(pluginData, { force: true, recursive: true }); + await rm(root, { force: true, recursive: true }); + } +}); + it('removes the fresh plugin data directory after every probe', async () => { const root = await createBundle(); let pluginData: string | undefined; From 2e125d6006cb1b840208b91959c63c93a8f41bff Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 07:18:30 +0000 Subject: [PATCH 07/19] fix(dev,create): guard probe teardown from synchronous close throws; document restoring installer bin McpProbeService wraps each close() so a synchronous throw becomes a settled promise: the other close still runs and plugin-data removal is always registered. Portable-only scaffold READMEs now name the package.json bin entry that must be restored alongside the config target to re-enable the installer command. --- .../src/dev/playground/mcp-probe-service.ts | 25 +++++++- .../tests/mcp-probe-service.test.ts | 64 +++++++++++++++++++ packages/create-agent-bundle/src/scaffold.ts | 13 +++- .../tests/scaffold.test.ts | 11 ++++ 4 files changed, 107 insertions(+), 6 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 2827d3bda..3b6ee5efa 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -243,6 +243,19 @@ const positiveTimeout = (value: number): number => { return value; }; +/** + * Invoke a close callback so that both a synchronous throw and a rejection + * become one settled promise; teardown chains must never depend on a close + * being well behaved. + */ +const settledClose = (close: () => Promise): Promise => { + try { + return Promise.resolve(close()).then(() => undefined, () => undefined); + } catch { + return Promise.resolve(); + } +}; + export class McpProbeService { readonly #clock: () => number; readonly #createClient: () => McpProbeClient; @@ -543,7 +556,13 @@ export class McpProbeService { // allowing a stalled close to extend the probe's total time budget. The // plugin-data removal is chained behind that teardown, so a close that // outlives this wait detaches together with the removal it gates. - const teardown = Promise.allSettled([client.close(), transport.close()]); + // Each close is invoked in isolation: a synchronously throwing close + // must neither skip the other close nor abort before the plugin-data + // removal below is registered. + const teardown = Promise.allSettled([ + settledClose(() => client.close()), + settledClose(() => transport.close()), + ]); const cleanup = this.#removePluginDataAfter(teardown, options.pluginData); const teardownWait = new Promise((resolvePromise) => { timer = setTimeout(resolvePromise, mcpProbeTeardownWaitMs); @@ -612,13 +631,13 @@ export class McpProbeService { // Same contract as the timer path below: the transport's own close // (TERM/KILL for stdio) keeps running, but a stalled close never holds // the timed-out report — #execute's bounded teardown owns that wait. - void onTimeout().catch(() => undefined); + void settledClose(onTimeout); throw new McpProbeTimeoutError(kind); } let timer: NodeJS.Timeout | undefined; const timedOut = new Promise((_resolve, reject) => { timer = setTimeout(() => { - void onTimeout().catch(() => undefined); + void settledClose(onTimeout); reject(new McpProbeTimeoutError(kind)); }, remaining); timer.unref(); diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 997716b3f..010f16f67 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -535,6 +535,70 @@ it('settle() fences in-flight probes, not only already-registered teardowns (#39 } }); +it('still closes the transport and removes plugin data when a close() throws synchronously (#397 review)', async () => { + const root = await createBundle(); + let pluginData: string | undefined; + let transportClosed = false; + try { + const service = serviceFor(root, { + createClient: () => client({ + close: () => { + // Not a rejection: a synchronous throw from the SDK client's close. + throw new Error('client close exploded synchronously'); + }, + }), + createPluginData: async () => { + pluginData = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-probe-data-')); + await writeFile(join(pluginData, 'proof.txt'), 'present'); + return pluginData; + }, + createStdioTransport: () => transport(async () => { + transportClosed = true; + }), + }); + + const report = await service.probe({ host: 'claude', serverName: 'timeline' }); + expect(report.status).toBe('ok'); + await service.settle(); + expect(transportClosed).toBe(true); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + if (pluginData !== undefined) await rm(pluginData, { force: true, recursive: true }); + await rm(root, { force: true, recursive: true }); + } +}); + +it('reports a timeout even when the timeout teardown throws synchronously', async () => { + const root = await createBundle(); + let pluginData: string | undefined; + let ticks = 0; + try { + const service = serviceFor(root, { + // Budget already spent at the connect step, so its synchronous-throwing + // timeout teardown runs on the spent-budget path. + clock: () => (ticks++ === 0 ? 0 : 10_000), + createClient: () => client({ connect: () => new Promise(() => undefined) }), + createPluginData: async () => { + pluginData = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-probe-data-')); + await writeFile(join(pluginData, 'proof.txt'), 'present'); + return pluginData; + }, + createStdioTransport: () => transport(() => { + throw new Error('transport close exploded synchronously'); + }), + timeoutMs: 10, + }); + const report = await service.probe({ host: 'claude', serverName: 'timeline' }); + expect(report.status).toBe('timed-out'); + expect(report.failure?.kind).toBe('connect'); + await service.settle(); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + if (pluginData !== undefined) await rm(pluginData, { force: true, recursive: true }); + await rm(root, { force: true, recursive: true }); + } +}); + it('removes the fresh plugin data directory after every probe', async () => { const root = await createBundle(); let pluginData: string | undefined; diff --git a/packages/create-agent-bundle/src/scaffold.ts b/packages/create-agent-bundle/src/scaffold.ts index 4e0cdcb6f..3ee13cc7a 100644 --- a/packages/create-agent-bundle/src/scaffold.ts +++ b/packages/create-agent-bundle/src/scaffold.ts @@ -133,17 +133,24 @@ const rewriteReadmeInstall = (contents: string, targets: readonly TargetName[]): const exampleBin = example[2] ?? ''; const proseBin = prose[1] ?? ''; if (hosts.length === 0) { + // The scaffold also dropped this bin mapping from package.json, and the + // build never restores manifest entries, so re-enabling installers needs + // both edits: the config target and the bin entry the npx command resolves. + const binEntry = `"${proseBin}": "./dist/bin/${proseBin}.js"`; return contents .replace(readmeInstallExample, [ '# no installer bin is generated for these targets; add claude, codex, or cursor', - '# to `targets` in agent-bundle.config.ts to get one', + '# to `targets` in agent-bundle.config.ts and restore the package.json bin entry', + `# ${binEntry} to get one`, '', ].join('\n')) .replace(readmeInstallProse, [ 'Installing the npm package does not mutate any host. This project selects', `no installable host target (${renderTargets(targets)}), so no \`${proseBin} install\``, - 'command is generated; add `claude`, `codex`, or `cursor` to `targets` in', - '`agent-bundle.config.ts` to generate one.', + 'command is generated and its `bin` entry was dropped from `package.json`. To', + 'generate one, add `claude`, `codex`, or `cursor` to `targets` in', + `\`agent-bundle.config.ts\` and restore \`${binEntry}\` under \`bin\` in`, + '`package.json`; the build emits the installer file but never edits the manifest.', '', ].join('\n')); } diff --git a/packages/create-agent-bundle/tests/scaffold.test.ts b/packages/create-agent-bundle/tests/scaffold.test.ts index 470285a6b..fbb3a5a33 100644 --- a/packages/create-agent-bundle/tests/scaffold.test.ts +++ b/packages/create-agent-bundle/tests/scaffold.test.ts @@ -208,6 +208,17 @@ describe('scaffold', () => { expect(portableReadme).not.toMatch(/^npx \S+ install /mu); expect(portableReadme).toContain("no installable host target ('portable')"); expect(portableReadme).toContain('add `claude`, `codex`, or `cursor` to `targets`'); + // Re-enabling installers needs the dropped package.json bin entry back too, + // and the README names exactly the mapping the template shipped. + const templateManifest = JSON.parse( + await readFile(join(templatesRoot, 'cli-tool', 'package_json'), 'utf8'), + ) as { readonly bin: Record }; + const installerBin = `${placeholderName}-install`; + expect(templateManifest.bin[installerBin]).toBeDefined(); + const droppedEntry = `"greeter-install": "${templateManifest.bin[installerBin]?.replaceAll(placeholderName, 'greeter')}"`; + expect(portableReadme).toContain(`# ${droppedEntry} to get one`); + expect(portableReadme).toContain(`restore \`${droppedEntry}\` under \`bin\` in`); + expect(portableReadme).toContain('never edits the manifest'); // The skills-only template has no install section and passes through. const minimalReadme = await readFile(join(minimal.root, 'README.md'), 'utf8'); From c4814a1f4a723a7729aa9edc7b0d1a61a6bc07a0 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 07:25:51 +0000 Subject: [PATCH 08/19] fix(dev): mask URL userinfo in probe text before the URL path exemption URLs are exempt from the probe report's absolute-path fail-closed rule, so Basic-auth userinfo (scheme://user:secret@host) would have reached the browser-facing report verbatim. It is masked as scheme://[REDACTED]@host before that check. --- .../src/dev/playground/mcp-probe-service.ts | 14 +++++++++++--- .../tests/mcp-probe-service.test.ts | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 3b6ee5efa..dc590d760 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -141,13 +141,21 @@ const bundlePathPattern = (bundleRoot: string): RegExp => { const hasAbsolutePath = (value: string): boolean => /(?:file:|(?:^|[\s"'([{=,]|:(?!\/\/))\/[^\s,;{}()[\]<>"']+|(?:^|[\s"'([{=,:])[A-Za-z]:[\\/]|\\\\)/u.test(value); +/** + * URL userinfo (`scheme://user:secret@host`) is a credential that the generic + * credential redaction does not recognize; because URLs are exempt from the + * absolute-path fail-closed rule, the userinfo is stripped before that check. + */ +const urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#@"'<>]+@/giu; + /** * Probe text follows the Dev Log browser-wire precedent without coupling this - * read-only service to log retention: credential text is removed first, - * bundle paths become a label, and every other absolute path fails closed. + * read-only service to log retention: credential text is removed first, URL + * userinfo is masked, bundle paths become a label, and every other absolute + * path fails closed. */ const redactProbeText = (value: string, bundleRoot: string, maximum: number): string => { - const redacted = redactCredentialText(value).replace( + const redacted = redactCredentialText(value).replace(urlUserinfoPattern, '$1[REDACTED]@').replace( bundlePathPattern(bundleRoot), (match) => { const normalized = match.replace(/^file:\/\//u, '').replaceAll('\\', '/'); diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 010f16f67..3b4d00bc3 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -195,6 +195,16 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as inputSchema: { type: 'object' as const }, name: 'file-url', }, + { + description: 'Private docs at https://alice:hunter2@example.test/private and postgres://svc:pa55@db.internal:5432/app', + inputSchema: { type: 'object' as const }, + name: 'url-with-userinfo', + }, + { + description: 'Bare user https://alice@example.test/private; contact ops@example.test', + inputSchema: { type: 'object' as const }, + name: 'url-with-bare-user-and-email', + }, ], }), }), @@ -215,7 +225,15 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as '[REDACTED]', // ...and file: URLs are local paths. '[REDACTED]', + // URL userinfo is a credential: it is masked while the link survives. + 'Private docs at https://[REDACTED]@example.test/private and postgres://[REDACTED]@db.internal:5432/app', + // A bare user is masked too; an email address is not URL userinfo. + 'Bare user https://[REDACTED]@example.test/private; contact ops@example.test', ]); + const serialized = JSON.stringify(report); + expect(serialized).not.toContain('hunter2'); + expect(serialized).not.toContain('pa55'); + expect(serialized).not.toContain('alice'); } finally { await rm(root, { force: true, recursive: true }); } From 5b5634118c41706f190b9cfde3ef56b3b0f3e3ab Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 07:34:02 +0000 Subject: [PATCH 09/19] fix(dev): retry capped probe plugin-data removal once the teardown settles When the teardown cap wins and the removal fails because the transport still holds the directory (Windows EPERM), chain one more removal to the teardown's eventual settlement and fence it in settle() instead of swallowing the failure. Adds a pluginDataTeardownCapMs testing seam. --- .../src/dev/playground/mcp-probe-service.ts | 33 +++++++++-- .../tests/mcp-probe-service.test.ts | 56 ++++++++++++++++++- 2 files changed, 84 insertions(+), 5 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index dc590d760..77385c761 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -87,6 +87,8 @@ export interface McpProbeServiceOptions { options: RemoteTransportOptions, ) => McpProbeTransport; readonly now?: () => Date; + /** Testing seam for the detached teardown cap; production keeps the named constant. */ + readonly pluginDataTeardownCapMs?: number; readonly prepared: () => Readonly<{ readonly bundleSource: string }> | undefined; readonly projectRoot: string; readonly registry?: TargetRegistry; @@ -256,6 +258,9 @@ const positiveTimeout = (value: number): number => { * become one settled promise; teardown chains must never depend on a close * being well behaved. */ +const removePluginData = (pluginData: string): Promise => + rm(pluginData, { force: true, maxRetries: 3, recursive: true, retryDelay: 50 }); + const settledClose = (close: () => Promise): Promise => { try { return Promise.resolve(close()).then(() => undefined, () => undefined); @@ -273,6 +278,7 @@ export class McpProbeService { readonly #inFlight = new Map>(); readonly #now: () => Date; readonly #pendingTeardowns = new Set>(); + readonly #pluginDataTeardownCapMs: number; readonly #prepared: McpProbeServiceOptions['prepared']; readonly #projectRoot: string; readonly #registry: TargetRegistry; @@ -296,6 +302,9 @@ export class McpProbeService { : { headers: transportOptions.headers }, })); this.#now = options.now ?? (() => new Date()); + this.#pluginDataTeardownCapMs = positiveTimeout( + options.pluginDataTeardownCapMs ?? mcpProbePluginDataTeardownCapMs, + ); this.#prepared = options.prepared; this.#projectRoot = resolve(options.projectRoot); this.#registry = options.registry ?? createDefaultRegistry(); @@ -397,24 +406,40 @@ export class McpProbeService { * reject outright and turn an honest timed-out report into a generic * failure, elsewhere the directory can vanish under the exiting child. * Removal failures stay on this detached path; they never reach the report. + * + * When the cap wins the race and the removal then fails — the transport is + * still alive and holds the directory, which on Windows is an `EPERM` — one + * more removal is chained to the teardown's eventual settlement instead of + * swallowing the failure for good; `settle()` fences that retry too. */ #removePluginDataAfter(teardown: Promise, pluginData: string): Promise { let cap: NodeJS.Timeout | undefined; + let capWon = false; // The cap stays referenced on purpose: it is the only handle guaranteeing // the removal runs when a stalled teardown outlives Workbench shutdown, // and it is cleared the moment the teardown settles. const capped = new Promise((resolvePromise) => { - cap = setTimeout(resolvePromise, mcpProbePluginDataTeardownCapMs); + cap = setTimeout(() => { + capWon = true; + resolvePromise(); + }, this.#pluginDataTeardownCapMs); }); const pending = Promise.race([teardown, capped]) .then(() => { if (cap !== undefined) clearTimeout(cap); - return rm(pluginData, { force: true, recursive: true }); + return removePluginData(pluginData); }) - .then(() => undefined, () => undefined); + .then(() => undefined, () => { + if (!capWon) return; + this.#track(teardown.then(() => removePluginData(pluginData)).then(() => undefined, () => undefined)); + }); + this.#track(pending); + return pending; + } + + #track(pending: Promise): void { this.#pendingTeardowns.add(pending); void pending.then(() => this.#pendingTeardowns.delete(pending)); - return pending; } #runtime(host: McpProbeHost): TargetMcpRuntimeContract { diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 3b4d00bc3..909ab38a9 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -1,4 +1,4 @@ -import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { chmod, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -617,6 +617,60 @@ it('reports a timeout even when the timeout teardown throws synchronously', asyn } }); +it('retries plugin-data removal once a capped teardown finally settles (#397 review)', async () => { + const root = await createBundle(); + // The plugin-data directory lives under a parent made read-only while the + // transport is "alive": that stands in for the Windows EPERM a still-running + // child produces when the capped removal races it. + const parent = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-probe-held-')); + const pluginData = join(parent, 'plugin-data'); + let releaseClose!: () => void; + const closeReleased = new Promise((resolvePromise) => { + releaseClose = resolvePromise; + }); + const events: string[] = []; + try { + const service = serviceFor(root, { + createPluginData: async () => { + await mkdir(pluginData); + await writeFile(join(pluginData, 'proof.txt'), 'present'); + await chmod(parent, 0o555); + return pluginData; + }, + createStdioTransport: () => transport(async () => { + await closeReleased; + events.push('transport-closed'); + }), + pluginDataTeardownCapMs: 100, + }); + + const report = await service.probe({ host: 'claude', serverName: 'timeline' }); + expect(report.status).toBe('ok'); + // The cap fires and the first removal fails against the held directory. + await new Promise((resolvePromise) => setTimeout(resolvePromise, 250)); + await expect(readFile(join(pluginData, 'proof.txt'), 'utf8')).resolves.toBe('present'); + + let settled = false; + const settle = service.settle().then(() => { settled = true; }); + await new Promise((resolvePromise) => setTimeout(resolvePromise, 50)); + // A capped-and-failed removal is not "done": shutdown still waits on the retry. + expect(settled).toBe(false); + + // The transport finishes closing and releases the directory; the chained + // retry removes it and only then does the fence resolve. + await chmod(parent, 0o755); + releaseClose(); + await settle; + events.push('settled'); + expect(events).toEqual(['transport-closed', 'settled']); + await expect(readFile(join(pluginData, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + await chmod(parent, 0o755).catch(() => undefined); + await rm(parent, { force: true, recursive: true }); + await rm(root, { force: true, recursive: true }); + } +}); + it('removes the fresh plugin data directory after every probe', async () => { const root = await createBundle(); let pluginData: string | undefined; From 9c92c5e77cdedb6854bf6607c49debe4be745e68 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 07:41:56 +0000 Subject: [PATCH 10/19] fix(dev): keep the capped plugin-data retry outside the settle() fence The retry chained to a stalled teardown is best-effort: tracking it would let a transport that never settles hold Workbench shutdown open, which is the case the cap exists to bound. --- .../src/dev/playground/mcp-probe-service.ts | 13 ++++--- .../tests/mcp-probe-service.test.ts | 35 +++++++++++++------ 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 77385c761..d2bea7ca8 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -410,7 +410,10 @@ export class McpProbeService { * When the cap wins the race and the removal then fails — the transport is * still alive and holds the directory, which on Windows is an `EPERM` — one * more removal is chained to the teardown's eventual settlement instead of - * swallowing the failure for good; `settle()` fences that retry too. + * swallowing the failure for good. That retry is best-effort and stays + * outside the `settle()` fence on purpose: a transport that never settles + * would otherwise hold Workbench shutdown open indefinitely, which is the + * very case the cap bounds. */ #removePluginDataAfter(teardown: Promise, pluginData: string): Promise { let cap: NodeJS.Timeout | undefined; @@ -431,15 +434,11 @@ export class McpProbeService { }) .then(() => undefined, () => { if (!capWon) return; - this.#track(teardown.then(() => removePluginData(pluginData)).then(() => undefined, () => undefined)); + void teardown.then(() => removePluginData(pluginData)).catch(() => undefined); }); - this.#track(pending); - return pending; - } - - #track(pending: Promise): void { this.#pendingTeardowns.add(pending); void pending.then(() => this.#pendingTeardowns.delete(pending)); + return pending; } #runtime(host: McpProbeHost): TargetMcpRuntimeContract { diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 909ab38a9..92158e0b3 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -650,19 +650,34 @@ it('retries plugin-data removal once a capped teardown finally settles (#397 rev await new Promise((resolvePromise) => setTimeout(resolvePromise, 250)); await expect(readFile(join(pluginData, 'proof.txt'), 'utf8')).resolves.toBe('present'); - let settled = false; - const settle = service.settle().then(() => { settled = true; }); - await new Promise((resolvePromise) => setTimeout(resolvePromise, 50)); - // A capped-and-failed removal is not "done": shutdown still waits on the retry. - expect(settled).toBe(false); + // The fence stays bounded by the cap: a transport that never settles must + // not hold Workbench shutdown open, so settle() resolves with the retry + // still outstanding. + await Promise.race([ + service.settle(), + new Promise((_resolve, reject) => setTimeout( + () => reject(new Error('settle() waited on the stalled transport past the cap.')), + 500, + ).unref()), + ]); + events.push('settled'); + await expect(readFile(join(pluginData, 'proof.txt'), 'utf8')).resolves.toBe('present'); - // The transport finishes closing and releases the directory; the chained - // retry removes it and only then does the fence resolve. + // Once the transport finishes closing and releases the directory, the + // best-effort retry chained to that settlement removes it. await chmod(parent, 0o755); releaseClose(); - await settle; - events.push('settled'); - expect(events).toEqual(['transport-closed', 'settled']); + const deadline = Date.now() + 2_000; + while (Date.now() < deadline) { + try { + await readFile(join(pluginData, 'proof.txt'), 'utf8'); + } catch { + break; + } + await new Promise((resolvePromise) => setTimeout(resolvePromise, 20)); + } + events.push('plugin-data-removed'); + expect(events).toEqual(['settled', 'transport-closed', 'plugin-data-removed']); await expect(readFile(join(pluginData, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); } finally { await chmod(parent, 0o755).catch(() => undefined); From 1af10c340face5435946e84cf338203fef3b5238 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 07:50:45 +0000 Subject: [PATCH 11/19] test(dev): make probe removal-failure and worker-marker tests platform-independent Inject the plugin-data removal through a testing seam instead of relying on directory mode bits, and assert the owner marker's temporaryRoot with path.isAbsolute() so the suite holds as root, in containers, and on Windows. --- .../src/dev/playground/mcp-probe-service.ts | 8 +++++-- .../tests/mcp-probe-service.test.ts | 23 ++++++++++++------- .../tests/rstest-worker-isolation.test.ts | 10 ++++---- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index d2bea7ca8..a2ed407e5 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -89,6 +89,8 @@ export interface McpProbeServiceOptions { readonly now?: () => Date; /** Testing seam for the detached teardown cap; production keeps the named constant. */ readonly pluginDataTeardownCapMs?: number; + /** Testing seam for plugin-data removal; production removes the directory recursively. */ + readonly removePluginData?: (pluginData: string) => Promise; readonly prepared: () => Readonly<{ readonly bundleSource: string }> | undefined; readonly projectRoot: string; readonly registry?: TargetRegistry; @@ -282,6 +284,7 @@ export class McpProbeService { readonly #prepared: McpProbeServiceOptions['prepared']; readonly #projectRoot: string; readonly #registry: TargetRegistry; + readonly #removePluginData: (pluginData: string) => Promise; readonly #timeoutMs: number; constructor(options: McpProbeServiceOptions) { @@ -308,6 +311,7 @@ export class McpProbeService { this.#prepared = options.prepared; this.#projectRoot = resolve(options.projectRoot); this.#registry = options.registry ?? createDefaultRegistry(); + this.#removePluginData = options.removePluginData ?? removePluginData; this.#timeoutMs = positiveTimeout(options.timeoutMs ?? mcpProbeTimeoutMs); } @@ -430,11 +434,11 @@ export class McpProbeService { const pending = Promise.race([teardown, capped]) .then(() => { if (cap !== undefined) clearTimeout(cap); - return removePluginData(pluginData); + return this.#removePluginData(pluginData); }) .then(() => undefined, () => { if (!capWon) return; - void teardown.then(() => removePluginData(pluginData)).catch(() => undefined); + void teardown.then(() => this.#removePluginData(pluginData)).catch(() => undefined); }); this.#pendingTeardowns.add(pending); void pending.then(() => this.#pendingTeardowns.delete(pending)); diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 92158e0b3..a1ce9efc2 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -1,4 +1,4 @@ -import { chmod, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -619,9 +619,10 @@ it('reports a timeout even when the timeout teardown throws synchronously', asyn it('retries plugin-data removal once a capped teardown finally settles (#397 review)', async () => { const root = await createBundle(); - // The plugin-data directory lives under a parent made read-only while the - // transport is "alive": that stands in for the Windows EPERM a still-running - // child produces when the capped removal races it. + // While the transport is "alive" the injected removal rejects the way a + // still-running child makes `rm` fail on Windows (EPERM); once the transport + // has closed it delegates to the real removal. No mode bits are involved, so + // the failure is identical as root, in containers, and on Windows. const parent = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-probe-held-')); const pluginData = join(parent, 'plugin-data'); let releaseClose!: () => void; @@ -629,19 +630,27 @@ it('retries plugin-data removal once a capped teardown finally settles (#397 rev releaseClose = resolvePromise; }); const events: string[] = []; + let transportAlive = true; try { const service = serviceFor(root, { createPluginData: async () => { await mkdir(pluginData); await writeFile(join(pluginData, 'proof.txt'), 'present'); - await chmod(parent, 0o555); return pluginData; }, createStdioTransport: () => transport(async () => { await closeReleased; + transportAlive = false; events.push('transport-closed'); }), pluginDataTeardownCapMs: 100, + removePluginData: async (target) => { + if (transportAlive) { + events.push('removal-rejected'); + throw Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }); + } + await rm(target, { force: true, recursive: true }); + }, }); const report = await service.probe({ host: 'claude', serverName: 'timeline' }); @@ -665,7 +674,6 @@ it('retries plugin-data removal once a capped teardown finally settles (#397 rev // Once the transport finishes closing and releases the directory, the // best-effort retry chained to that settlement removes it. - await chmod(parent, 0o755); releaseClose(); const deadline = Date.now() + 2_000; while (Date.now() < deadline) { @@ -677,10 +685,9 @@ it('retries plugin-data removal once a capped teardown finally settles (#397 rev await new Promise((resolvePromise) => setTimeout(resolvePromise, 20)); } events.push('plugin-data-removed'); - expect(events).toEqual(['settled', 'transport-closed', 'plugin-data-removed']); + expect(events).toEqual(['removal-rejected', 'settled', 'transport-closed', 'plugin-data-removed']); await expect(readFile(join(pluginData, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); } finally { - await chmod(parent, 0o755).catch(() => undefined); await rm(parent, { force: true, recursive: true }); await rm(root, { force: true, recursive: true }); } diff --git a/packages/agent-bundle/tests/rstest-worker-isolation.test.ts b/packages/agent-bundle/tests/rstest-worker-isolation.test.ts index 1b74693c4..995f17acd 100644 --- a/packages/agent-bundle/tests/rstest-worker-isolation.test.ts +++ b/packages/agent-bundle/tests/rstest-worker-isolation.test.ts @@ -1,6 +1,6 @@ import { mkdir, mkdtemp, readdir, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { isAbsolute, join } from 'node:path'; import { expect, it } from '@rstest/core'; @@ -42,13 +42,15 @@ it('stamps every worker root with the owner marker the local-CI runner cleans up // The setup file already isolated this worker, so TMPDIR points at the // root itself; the marker records the HOST temp root it was derived from // and the process that owns it. - expect(rstestWorkerRootOwner(root)).toMatchObject({ + const owner = rstestWorkerRootOwner(root); + expect(owner).toMatchObject({ cwd: process.cwd(), pid: process.pid, - temporaryRoot: expect.stringMatching(/^\//u), workerId: process.env['RSTEST_WORKER_ID'] ?? '0', }); - expect(rstestWorkerRootOwner(root)?.temporaryRoot).not.toBe(root); + // Absolute in the platform's own shape (`/tmp`, `C:\Temp`, a UNC root). + expect(isAbsolute(owner?.temporaryRoot ?? '')).toBe(true); + expect(owner?.temporaryRoot).not.toBe(root); }); it('removes only the finished roots owned by one host temporary root', async () => { From 8d424a63e624cb008cc31209d3f91cfd4ba200b9 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 07:59:28 +0000 Subject: [PATCH 12/19] fix(dev): reuse the timeout-started transport close in the probe teardown A budget timeout starts transport.close() early; the teardown now awaits that same memoized promise instead of a duplicate close a non-reentrant transport answers immediately, so plugin-data removal follows the real TERM/KILL path. --- .../src/dev/playground/mcp-probe-service.ts | 17 +++++-- .../tests/mcp-probe-service.test.ts | 48 +++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index a2ed407e5..1a777286e 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -501,6 +501,14 @@ export class McpProbeService { await rm(options.pluginData, { force: true, recursive: true }); throw error; } + // One close promise per probe: a timeout starts the transport's TERM/KILL + // path early, and the teardown below must follow that same close rather + // than a duplicate call a non-reentrant transport answers immediately. + let transportClose: Promise | undefined; + const closeTransport = (): Promise => { + transportClose ??= settledClose(() => transport.close()); + return transportClose; + }; let report: McpProbeReport; try { try { @@ -508,7 +516,7 @@ export class McpProbeService { client.connect(transport), options.startedAt, 'connect', - () => transport.close(), + closeTransport, ); } catch (error) { const failure = failureSnapshot( @@ -540,7 +548,7 @@ export class McpProbeService { client.listTools(), options.startedAt, 'protocol', - () => transport.close(), + closeTransport, ); } catch (error) { const protocolError = error instanceof McpProbeTimeoutError @@ -594,10 +602,11 @@ export class McpProbeService { // outlives this wait detaches together with the removal it gates. // Each close is invoked in isolation: a synchronously throwing close // must neither skip the other close nor abort before the plugin-data - // removal below is registered. + // removal below is registered. The transport close is the one a + // timeout may already have started. const teardown = Promise.allSettled([ settledClose(() => client.close()), - settledClose(() => transport.close()), + closeTransport(), ]); const cleanup = this.#removePluginDataAfter(teardown, options.pluginData); const teardownWait = new Promise((resolvePromise) => { diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index a1ce9efc2..7b14f72f0 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -388,6 +388,54 @@ it('returns a timed-out report without awaiting stalled teardown when the budget } }); +it('chains plugin-data removal to the close a timeout already started, not a duplicate close (#397 review)', async () => { + const root = await createBundle(); + let pluginData: string | undefined; + let ticks = 0; + let closeCalls = 0; + let releaseClose!: () => void; + const closeReleased = new Promise((resolvePromise) => { + releaseClose = resolvePromise; + }); + try { + const service = serviceFor(root, { + clock: () => (ticks++ === 0 ? 0 : 10_000), + createClient: () => client({ connect: () => new Promise(() => undefined) }), + createPluginData: async () => { + pluginData = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-probe-data-')); + await writeFile(join(pluginData, 'proof.txt'), 'present'); + return pluginData; + }, + // A non-reentrant transport: the first close runs the real (slow) + // TERM/KILL path, any duplicate close answers immediately. + createStdioTransport: () => transport(async () => { + closeCalls += 1; + if (closeCalls > 1) return; + await closeReleased; + }), + timeoutMs: 10, + }); + + const report = await service.probe({ host: 'claude', serverName: 'timeline' }); + expect(report.status).toBe('timed-out'); + expect(closeCalls).toBe(1); + + // The response is back but the first close is still running: the plugin + // data must survive until that close — not a duplicate — settles. + await new Promise((resolvePromise) => setTimeout(resolvePromise, 150)); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).resolves.toBe('present'); + + releaseClose(); + await service.settle(); + expect(closeCalls).toBe(1); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + releaseClose(); + if (pluginData !== undefined) await rm(pluginData, { force: true, recursive: true }); + await rm(root, { force: true, recursive: true }); + } +}); + it('coalesces only identical in-flight probes and clears them after settlement', async () => { const root = await createBundle(); const connectStarted = Promise.withResolvers(); From 0660df0638a5e772840b031200ebe6527bdc4b1b Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 08:06:50 +0000 Subject: [PATCH 13/19] fix(dev): mask URL userinfo through the final authority delimiter The userinfo match now runs greedily to the last @ before a WHATWG authority terminator, so a raw @ or quote inside a password cannot leave part of the credential in the probe report. --- .../src/dev/playground/mcp-probe-service.ts | 6 +++++- .../agent-bundle/tests/mcp-probe-service.test.ts | 12 ++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 1a777286e..e1e296167 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -149,8 +149,12 @@ const hasAbsolutePath = (value: string): boolean => * URL userinfo (`scheme://user:secret@host`) is a credential that the generic * credential redaction does not recognize; because URLs are exempt from the * absolute-path fail-closed rule, the userinfo is stripped before that check. + * The authority runs until whitespace or a WHATWG authority terminator + * (`/`, `?`, `#`, `\`); within it the match is greedy through the *final* + * `@`, the delimiter URL parsers honour, so a raw `@` or quote inside a + * password cannot leave part of the credential behind. */ -const urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#@"'<>]+@/giu; +const urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#\\]*@/giu; /** * Probe text follows the Dev Log browser-wire precedent without coupling this diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 7b14f72f0..b057d937b 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -205,6 +205,11 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as inputSchema: { type: 'object' as const }, name: 'url-with-bare-user-and-email', }, + { + description: 'Raw @ in the password https://alice:pa@ss@example.test/private?next=me@x and quote https://al"ice:s3cret@example.test/#top', + inputSchema: { type: 'object' as const }, + name: 'url-with-at-and-quote-in-userinfo', + }, ], }), }), @@ -229,11 +234,18 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as 'Private docs at https://[REDACTED]@example.test/private and postgres://[REDACTED]@db.internal:5432/app', // A bare user is masked too; an email address is not URL userinfo. 'Bare user https://[REDACTED]@example.test/private; contact ops@example.test', + // Masking runs through the final authority `@` (the delimiter URL parsers + // honour), so a raw `@` or quote inside the password leaves nothing behind, + // while an `@` in the query is not userinfo. + 'Raw @ in the password https://[REDACTED]@example.test/private?next=me@x and quote https://[REDACTED]@example.test/#top', ]); const serialized = JSON.stringify(report); expect(serialized).not.toContain('hunter2'); expect(serialized).not.toContain('pa55'); expect(serialized).not.toContain('alice'); + expect(serialized).not.toContain('pa@ss'); + expect(serialized).not.toContain('@ss@'); + expect(serialized).not.toContain('s3cret'); } finally { await rm(root, { force: true, recursive: true }); } From f6b45d435ecdeb5a84015f6c4b7c4faced2c8230 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 08:13:42 +0000 Subject: [PATCH 14/19] fix(dev): treat backslash as URL userinfo when masking probe text Only /, ?, and # end the authority for every WHATWG scheme; a backslash is valid userinfo for non-special schemes such as postgres://, so it no longer stops the mask short of the credential. --- .../src/dev/playground/mcp-probe-service.ts | 13 ++++++++----- .../agent-bundle/tests/mcp-probe-service.test.ts | 10 ++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index e1e296167..02c765847 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -149,12 +149,15 @@ const hasAbsolutePath = (value: string): boolean => * URL userinfo (`scheme://user:secret@host`) is a credential that the generic * credential redaction does not recognize; because URLs are exempt from the * absolute-path fail-closed rule, the userinfo is stripped before that check. - * The authority runs until whitespace or a WHATWG authority terminator - * (`/`, `?`, `#`, `\`); within it the match is greedy through the *final* - * `@`, the delimiter URL parsers honour, so a raw `@` or quote inside a - * password cannot leave part of the credential behind. + * The authority runs until whitespace or one of the authority terminators + * every WHATWG scheme shares (`/`, `?`, `#`); within it the match is greedy + * through the *final* `@`, the delimiter URL parsers honour, so a raw `@`, + * quote, or backslash inside a password cannot leave part of the credential + * behind. `\` is deliberately not a terminator here: it only ends the + * authority for special schemes, while non-special ones such as + * `postgres://` accept it inside userinfo — masking errs toward redaction. */ -const urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#\\]*@/giu; +const urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#]*@/giu; /** * Probe text follows the Dev Log browser-wire precedent without coupling this diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index b057d937b..6a07d22e1 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -210,6 +210,11 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as inputSchema: { type: 'object' as const }, name: 'url-with-at-and-quote-in-userinfo', }, + { + description: String.raw`Non-special scheme postgres://alice:se\cret@example.test/db and https://bob:pw\x@example.test/`, + inputSchema: { type: 'object' as const }, + name: 'url-with-backslash-in-userinfo', + }, ], }), }), @@ -238,6 +243,9 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as // honour), so a raw `@` or quote inside the password leaves nothing behind, // while an `@` in the query is not userinfo. 'Raw @ in the password https://[REDACTED]@example.test/private?next=me@x and quote https://[REDACTED]@example.test/#top', + // A backslash is userinfo for non-special schemes; masking treats it as + // such for every scheme rather than leaving a credential behind. + 'Non-special scheme postgres://[REDACTED]@example.test/db and https://[REDACTED]@example.test/', ]); const serialized = JSON.stringify(report); expect(serialized).not.toContain('hunter2'); @@ -246,6 +254,8 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as expect(serialized).not.toContain('pa@ss'); expect(serialized).not.toContain('@ss@'); expect(serialized).not.toContain('s3cret'); + expect(serialized).not.toContain('cret'); + expect(serialized).not.toContain('bob'); } finally { await rm(root, { force: true, recursive: true }); } From 9ff70e4a4698f9acea19fd08d32514002abef08a Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 08:23:53 +0000 Subject: [PATCH 15/19] fix(dev): fail closed on local URI paths and retry early-failed plugin-data removal Only http(s)/ws(s) URLs are exempt from the probe report's absolute-path rule; unix:///, vscode://file/... and other scheme://.../ forms fail closed. When the teardown settles but the first removal fails (Windows still holding the directory), one bounded, fenced retry follows. --- .../src/dev/playground/mcp-probe-service.ts | 31 ++++++- .../tests/mcp-probe-service.test.ts | 88 +++++++++++++++---- 2 files changed, 101 insertions(+), 18 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 02c765847..8c4b1b079 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -53,6 +53,11 @@ const mcpProbeTeardownWaitMs = 50; * against a transport whose close never settles. */ export const mcpProbePluginDataTeardownCapMs = 10_000; +/** + * Delay before the one bounded removal retry that follows a teardown which + * settled while the child still held the directory for a moment (Windows). + */ +const mcpProbePluginDataRetryDelayMs = 250; const safeCapabilityName = /^[A-Za-z][A-Za-z0-9_-]{0,63}$/u; const connectionErrorCodes = new Set([ 'EACCES', @@ -141,8 +146,15 @@ const bundlePathPattern = (bundleRoot: string): RegExp => { * An absolute POSIX path starts the text or follows a separator; a `:` counts * as a separator (`cwd:/private`) only when it is not the `://` of a URI * scheme, so `https://example.com/docs` is link guidance, not a local path. + * That exemption is limited to network schemes (`http`, `https`, `ws`, + * `wss`): any other `scheme://…/…` — `unix:///home/…`, `vscode://file/home/…`, + * `file:` — may carry a machine-local path in its authority or path and fails + * closed like a bare absolute path. */ +const localUriPathPattern = /\b(?!(?:https?|wss?):\/\/)[a-z][a-z0-9+.-]*:\/\/[^\s/]*\//iu; + const hasAbsolutePath = (value: string): boolean => + localUriPathPattern.test(value) || /(?:file:|(?:^|[\s"'([{=,]|:(?!\/\/))\/[^\s,;{}()[\]<>"']+|(?:^|[\s"'([{=,:])[A-Za-z]:[\\/]|\\\\)/u.test(value); /** @@ -444,12 +456,25 @@ export class McpProbeService { return this.#removePluginData(pluginData); }) .then(() => undefined, () => { - if (!capWon) return; - void teardown.then(() => this.#removePluginData(pluginData)).catch(() => undefined); + if (capWon) { + void teardown.then(() => this.#removePluginData(pluginData)).catch(() => undefined); + return; + } + // The teardown settled (a close may have failed fast) but the child + // still held the directory for a moment: one bounded, fenced retry. + this.#track( + new Promise((resolvePromise) => { setTimeout(resolvePromise, mcpProbePluginDataRetryDelayMs); }) + .then(() => this.#removePluginData(pluginData)) + .then(() => undefined, () => undefined), + ); }); + this.#track(pending); + return pending; + } + + #track(pending: Promise): void { this.#pendingTeardowns.add(pending); void pending.then(() => this.#pendingTeardowns.delete(pending)); - return pending; } #runtime(host: McpProbeHost): TargetMcpRuntimeContract { diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 6a07d22e1..a9a0b44cb 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -196,10 +196,30 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as name: 'file-url', }, { - description: 'Private docs at https://alice:hunter2@example.test/private and postgres://svc:pa55@db.internal:5432/app', + description: 'Private docs at https://alice:hunter2@example.test/private and wss://svc:pa55@relay.example.test:5432/app', inputSchema: { type: 'object' as const }, name: 'url-with-userinfo', }, + { + description: 'Socket unix:///home/alice/private.sock', + inputSchema: { type: 'object' as const }, + name: 'local-uri-empty-authority', + }, + { + description: 'Open vscode://file/home/alice/project in the editor', + inputSchema: { type: 'object' as const }, + name: 'local-uri-authority-then-path', + }, + { + description: 'Database postgres://svc:pa55@db.internal:5432/app', + inputSchema: { type: 'object' as const }, + name: 'non-network-scheme-with-path', + }, + { + description: 'Registry oci://registry.example.test and mail mailto:ops@example.test', + inputSchema: { type: 'object' as const }, + name: 'non-network-scheme-without-path', + }, { description: 'Bare user https://alice@example.test/private; contact ops@example.test', inputSchema: { type: 'object' as const }, @@ -211,7 +231,7 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as name: 'url-with-at-and-quote-in-userinfo', }, { - description: String.raw`Non-special scheme postgres://alice:se\cret@example.test/db and https://bob:pw\x@example.test/`, + description: String.raw`Backslash in the password https://bob:pw\x@example.test/ and ws://carol:a\b@example.test/feed`, inputSchema: { type: 'object' as const }, name: 'url-with-backslash-in-userinfo', }, @@ -235,27 +255,30 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as '[REDACTED]', // ...and file: URLs are local paths. '[REDACTED]', - // URL userinfo is a credential: it is masked while the link survives. - 'Private docs at https://[REDACTED]@example.test/private and postgres://[REDACTED]@db.internal:5432/app', + // URL userinfo is a credential: it is masked while the network link survives. + 'Private docs at https://[REDACTED]@example.test/private and wss://[REDACTED]@relay.example.test:5432/app', + // Only network schemes are exempt from the path rule: a local-resource + // URI with an empty authority... + '[REDACTED]', + // ...or with a path after its authority carries a machine-local path... + '[REDACTED]', + // ...and any other scheme with a path component fails closed too. + '[REDACTED]', + // A non-network scheme without a path component is not a path. + 'Registry oci://registry.example.test and mail mailto:ops@example.test', // A bare user is masked too; an email address is not URL userinfo. 'Bare user https://[REDACTED]@example.test/private; contact ops@example.test', // Masking runs through the final authority `@` (the delimiter URL parsers // honour), so a raw `@` or quote inside the password leaves nothing behind, // while an `@` in the query is not userinfo. 'Raw @ in the password https://[REDACTED]@example.test/private?next=me@x and quote https://[REDACTED]@example.test/#top', - // A backslash is userinfo for non-special schemes; masking treats it as - // such for every scheme rather than leaving a credential behind. - 'Non-special scheme postgres://[REDACTED]@example.test/db and https://[REDACTED]@example.test/', + // A backslash inside userinfo is masked with the rest of the credential. + 'Backslash in the password https://[REDACTED]@example.test/ and ws://[REDACTED]@example.test/feed', ]); const serialized = JSON.stringify(report); - expect(serialized).not.toContain('hunter2'); - expect(serialized).not.toContain('pa55'); - expect(serialized).not.toContain('alice'); - expect(serialized).not.toContain('pa@ss'); - expect(serialized).not.toContain('@ss@'); - expect(serialized).not.toContain('s3cret'); - expect(serialized).not.toContain('cret'); - expect(serialized).not.toContain('bob'); + for (const secret of ['hunter2', 'pa55', 'alice', 'pa@ss', '@ss@', 's3cret', 'bob', 'carol']) { + expect(serialized).not.toContain(secret); + } } finally { await rm(root, { force: true, recursive: true }); } @@ -763,6 +786,41 @@ it('retries plugin-data removal once a capped teardown finally settles (#397 rev } }); +it('retries removal once, fenced, when the teardown settled but the directory was still held (#397 review)', async () => { + const root = await createBundle(); + let pluginData: string | undefined; + let removals = 0; + try { + const service = serviceFor(root, { + createPluginData: async () => { + pluginData = await mkdtemp(join(tmpdir(), 'agent-bundle-mcp-probe-data-')); + await writeFile(join(pluginData, 'proof.txt'), 'present'); + return pluginData; + }, + // The transport's close fails fast, so the teardown settles at once + // while (on Windows) the child still holds the directory for a moment. + createStdioTransport: () => transport(() => Promise.reject(new Error('close failed fast'))), + removePluginData: async (target) => { + removals += 1; + if (removals === 1) { + throw Object.assign(new Error('EPERM: operation not permitted'), { code: 'EPERM' }); + } + await rm(target, { force: true, recursive: true }); + }, + }); + + const report = await service.probe({ host: 'claude', serverName: 'timeline' }); + expect(report.status).toBe('ok'); + // The fence covers the delayed retry: settle() resolves only after it ran. + await service.settle(); + expect(removals).toBe(2); + await expect(readFile(join(pluginData!, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + if (pluginData !== undefined) await rm(pluginData, { force: true, recursive: true }); + await rm(root, { force: true, recursive: true }); + } +}); + it('removes the fresh plugin data directory after every probe', async () => { const root = await createBundle(); let pluginData: string | undefined; From 89588c1a97cc54d0347052da8b76efcea4edee45 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 08:31:59 +0000 Subject: [PATCH 16/19] fix(dev): let URL userinfo masking run through whitespace URL parsers percent-encode spaces and strip tabs/newlines inside userinfo, so whitespace no longer ends the authority run; only /, ?, and # do. A path-less URL followed by an @ before those terminators is over-masked by design. --- .../src/dev/playground/mcp-probe-service.ts | 18 +++++++++-------- .../tests/mcp-probe-service.test.ts | 20 +++++++++++++++++-- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 8c4b1b079..6191c81a4 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -161,15 +161,17 @@ const hasAbsolutePath = (value: string): boolean => * URL userinfo (`scheme://user:secret@host`) is a credential that the generic * credential redaction does not recognize; because URLs are exempt from the * absolute-path fail-closed rule, the userinfo is stripped before that check. - * The authority runs until whitespace or one of the authority terminators - * every WHATWG scheme shares (`/`, `?`, `#`); within it the match is greedy - * through the *final* `@`, the delimiter URL parsers honour, so a raw `@`, - * quote, or backslash inside a password cannot leave part of the credential - * behind. `\` is deliberately not a terminator here: it only ends the - * authority for special schemes, while non-special ones such as - * `postgres://` accept it inside userinfo — masking errs toward redaction. + * The authority runs until one of the terminators every WHATWG scheme shares + * (`/`, `?`, `#`); within it the match is greedy through the *final* `@`, the + * delimiter URL parsers honour, so a raw `@`, quote, backslash, or whitespace + * inside a password (parsers percent-encode spaces and strip embedded tabs and + * newlines) cannot leave part of the credential behind. Nothing short of those + * three terminators ends the run on purpose — `\` is userinfo for non-special + * schemes and whitespace is encoded rather than rejected — so a path-less URL + * followed on the same text by an `@` before any `/`, `?`, or `#` is masked + * as well: for a browser-facing report that over-redaction is the safe side. */ -const urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#]*@/giu; +const urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^/?#]*@/giu; /** * Probe text follows the Dev Log browser-wire precedent without coupling this diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index a9a0b44cb..42cf3cf84 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -216,10 +216,20 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as name: 'non-network-scheme-with-path', }, { - description: 'Registry oci://registry.example.test and mail mailto:ops@example.test', + description: 'Registry oci://registry.example.test stays a link', inputSchema: { type: 'object' as const }, name: 'non-network-scheme-without-path', }, + { + description: 'Whitespace https://alice:se cret@example.test/private and tab https://bob:x\ty@example.test/', + inputSchema: { type: 'object' as const }, + name: 'url-with-whitespace-in-userinfo', + }, + { + description: 'Path-less https://example.test then ops@example.test before any slash', + inputSchema: { type: 'object' as const }, + name: 'path-less-url-then-email', + }, { description: 'Bare user https://alice@example.test/private; contact ops@example.test', inputSchema: { type: 'object' as const }, @@ -265,7 +275,13 @@ it('keeps URLs while redacting real absolute and bundle paths (#316 review)', as // ...and any other scheme with a path component fails closed too. '[REDACTED]', // A non-network scheme without a path component is not a path. - 'Registry oci://registry.example.test and mail mailto:ops@example.test', + 'Registry oci://registry.example.test stays a link', + // Whitespace inside userinfo is encoded (spaces) or stripped (tabs) by URL + // parsers, so it does not end the mask early. + 'Whitespace https://[REDACTED]@example.test/private and tab https://[REDACTED]@example.test/', + // The documented trade-off: an `@` after a path-less URL, before any + // `/`, `?`, or `#`, is masked as if it were userinfo (over-redaction). + 'Path-less https://[REDACTED]@example.test before any slash', // A bare user is masked too; an email address is not URL userinfo. 'Bare user https://[REDACTED]@example.test/private; contact ops@example.test', // Masking runs through the final authority `@` (the delimiter URL parsers From 5ebf7403a7810d4009342e5906fd50ce8f1b7950 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 08:46:59 +0000 Subject: [PATCH 17/19] test(adapters): keep the cursor adapter revision pin at 1.8.0 after the codex re-pin --- packages/agent-bundle/tests/adapter-metadata.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/agent-bundle/tests/adapter-metadata.test.ts b/packages/agent-bundle/tests/adapter-metadata.test.ts index ba9fd5b72..d681d4013 100644 --- a/packages/agent-bundle/tests/adapter-metadata.test.ts +++ b/packages/agent-bundle/tests/adapter-metadata.test.ts @@ -145,7 +145,7 @@ it('records exact immutable metadata for every built-in target', () => { ], }); expect(registryMetadata(registry, 'cursor')).toEqual({ - adapterRevision: '1.9.0', + adapterRevision: '1.8.0', observedVersion: '2026-08-28', schemas: [ { From 461e87c36c3a60606703aefdc7e1ff6415097d23 Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 08:55:00 +0000 Subject: [PATCH 18/19] chore(changeset): rewrite the wave-4 changeset as a user-facing release summary --- .changeset/review-threads-wave4.md | 48 ++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/.changeset/review-threads-wave4.md b/.changeset/review-threads-wave4.md index 83e6822de..23a1f1e0b 100644 --- a/.changeset/review-threads-wave4.md +++ b/.changeset/review-threads-wave4.md @@ -3,19 +3,35 @@ "create-agent-bundle": patch --- -Address the fourth wave of review findings. Rendered CLI commands, projected -MCP commands, and rendered scripts now forward the dispatched `invocation` to -the Flight worker, so conventional context providers that branch on -`invocation.kind` receive it instead of `undefined`. The transcribed Codex -plugin schema rejects line terminators in component, interface-asset, and -screenshot paths so a newline followed by `/../` cannot slip past the traversal -guard (codex adapter 1.8.0, plugin adapter 1.23.0). Any-JSON `permission/request` -`tool_input` is admitted only for Codex, whose pinned schema declares it; Claude -envelopes stay object-shaped. `agent-bundle build --help` and `prepack --help` -describe the `artifact` default those commands actually use. The MCP probe -redaction no longer treats a URI scheme's `://` as a path separator, so -documentation links survive, and plugin-data removal now follows the transport -teardown instead of racing a slow stdio shutdown. Doctor probes runtime -endpoints concurrently (eight at a time), bounding a directory of silent -runtimes as a whole. Scaffolded READMEs render install instructions for the -selected targets rather than a hard-coded `install claude`. +Harden the Codex plugin manifest, MCP probe reports, Doctor endpoint scans, CLI +help, and scaffolded README install instructions (#397). + +- Reject line terminators, control characters, and backslash-form parent + segments in the pinned Codex `plugin.json` `screenshots` paths, matching the + component and interface-asset patterns; a manifest that relies on them now + fails `AB6012` (pinned-schema rejection) and `AB6032` (Codex host validation) + instead of validating. The Codex adapter is revision `1.9.0` and the composite + `plugin` adapter `1.24.0`. +- Admit any-JSON `tool_input` on `permission/request` event envelopes only for + the `codex` target, whose pinned schema declares it; `claude` envelopes keep + the documented object requirement. +- `agent-bundle build --help` and `agent-bundle prepack --help` now state the + `artifact` default for `--output` that those commands actually use. +- Workbench MCP probe reports keep `http(s)`/`ws(s)` documentation links while + masking URL userinfo (`scheme://user:secret@host`) through the final authority + delimiter, and fail closed on local-resource URIs such as `unix:///…` or + `vscode://file/…` and on every other `scheme://…/…` form. Plugin-data + directories are removed only after the transport teardown settles (bounded by + a 10 s cap, with one fenced retry when a still-exiting child held the + directory), a synchronously throwing `close()` no longer skips cleanup, a + timeout's transport close is reused rather than duplicated, and Workbench + shutdown (`server.close()`) joins in-flight probes and their detached + cleanups. +- `agent-bundle doctor` probes runtime socket and lock endpoints eight at a + time, so a directory of silent runtimes is bounded as a whole instead of + costing one timeout per endpoint. +- `create-agent-bundle` renders README install instructions for the selected + `--targets` (one `npx install ` line per installable host) instead + of a hard-coded `install claude`; portable-only scaffolds explain that no + installer bin is generated and name the `package.json` `bin` entry to restore + alongside the config target to get one. From f88e117c509e615dace2eb6ee43ce66e6a7c7ade Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Thu, 3 Sep 2026 09:10:49 +0000 Subject: [PATCH 19/19] fix(workbench): redact URIs glued to a preceding identifier in probe reports --- .../src/dev/playground/mcp-probe-service.ts | 13 +++++++--- .../tests/mcp-probe-service.test.ts | 24 ++++++++++++++++++- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts index 6191c81a4..43533c116 100644 --- a/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts +++ b/packages/agent-bundle/src/dev/playground/mcp-probe-service.ts @@ -149,9 +149,13 @@ const bundlePathPattern = (bundleRoot: string): RegExp => { * That exemption is limited to network schemes (`http`, `https`, `ws`, * `wss`): any other `scheme://…/…` — `unix:///home/…`, `vscode://file/home/…`, * `file:` — may carry a machine-local path in its authority or path and fails - * closed like a bare absolute path. + * closed like a bare absolute path. The scheme is anchored to the start of its + * own character run, not to a word boundary, so an identifier glued in front + * of it (`_unix:///home/…`, `id9unix:///home/…`) cannot hide the URI; the + * exemption looks through any such prefix to the network scheme that ends the + * run, so a glued `id9https://…` link still survives. */ -const localUriPathPattern = /\b(?!(?:https?|wss?):\/\/)[a-z][a-z0-9+.-]*:\/\/[^\s/]*\//iu; +const localUriPathPattern = /(? localUriPathPattern.test(value) || @@ -170,8 +174,11 @@ const hasAbsolutePath = (value: string): boolean => * schemes and whitespace is encoded rather than rejected — so a path-less URL * followed on the same text by an `@` before any `/`, `?`, or `#` is masked * as well: for a browser-facing report that over-redaction is the safe side. + * Like the local-URI rule, the scheme is anchored to the start of its own + * character run rather than to a word boundary, so a URL glued to a preceding + * identifier (`_https://user:secret@…`) is masked too. */ -const urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^/?#]*@/giu; +const urlUserinfoPattern = /(?