diff --git a/.changeset/review-threads-wave4.md b/.changeset/review-threads-wave4.md new file mode 100644 index 000000000..23a1f1e0b --- /dev/null +++ b/.changeset/review-threads-wave4.md @@ -0,0 +1,37 @@ +--- +"agent-bundle": patch +"create-agent-bundle": patch +--- + +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. 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/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..43533c116 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,19 @@ 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; +/** + * 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', @@ -80,6 +92,10 @@ export interface McpProbeServiceOptions { options: RemoteTransportOptions, ) => McpProbeTransport; 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; @@ -126,16 +142,52 @@ 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. + * 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. 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 = /(? - /(?:file:|(?:^|[\s"'([{=,:])\/[^\s,;{}()[\]<>"']+|(?:^|[\s"'([{=,:])[A-Za-z]:[\\/]|\\\\)/u.test(value); + localUriPathPattern.test(value) || + /(?: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. + * 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. + * 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 = /(? { - const redacted = redactCredentialText(value).replace( + const redacted = redactCredentialText(value).replace(urlUserinfoPattern, '$1[REDACTED]@').replace( bundlePathPattern(bundleRoot), (match) => { const normalized = match.replace(/^file:\/\//u, '').replaceAll('\\', '/'); @@ -231,6 +283,22 @@ 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 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); + } catch { + return Promise.resolve(); + } +}; + export class McpProbeService { readonly #clock: () => number; readonly #createClient: () => McpProbeClient; @@ -239,9 +307,12 @@ export class McpProbeService { readonly #createStreamableHttpTransport: NonNullable; readonly #inFlight = new Map>(); readonly #now: () => Date; + readonly #pendingTeardowns = new Set>(); + readonly #pluginDataTeardownCapMs: number; readonly #prepared: McpProbeServiceOptions['prepared']; readonly #projectRoot: string; readonly #registry: TargetRegistry; + readonly #removePluginData: (pluginData: string) => Promise; readonly #timeoutMs: number; constructor(options: McpProbeServiceOptions) { @@ -262,9 +333,13 @@ 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(); + this.#removePluginData = options.removePluginData ?? removePluginData; this.#timeoutMs = positiveTimeout(options.timeoutMs ?? mcpProbeTimeoutMs); } @@ -284,6 +359,21 @@ export class McpProbeService { return probe; } + /** + * 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.#inFlight.size > 0 || this.#pendingTeardowns.size > 0) { + await Promise.allSettled([...this.#inFlight.values(), ...this.#pendingTeardowns]); + } + } + async #run(options: { readonly host: McpProbeHost; readonly serverName: string; @@ -305,8 +395,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 +408,82 @@ 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. + * + * 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. 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; + 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(() => { + capWon = true; + resolvePromise(); + }, this.#pluginDataTeardownCapMs); + }); + const pending = Promise.race([teardown, capped]) + .then(() => { + if (cap !== undefined) clearTimeout(cap); + return this.#removePluginData(pluginData); + }) + .then(() => 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)); } #runtime(host: McpProbeHost): TargetMcpRuntimeContract { @@ -374,12 +527,29 @@ 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; + } + // 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 { @@ -387,7 +557,7 @@ export class McpProbeService { client.connect(transport), options.startedAt, 'connect', - () => transport.close(), + closeTransport, ); } catch (error) { const failure = failureSnapshot( @@ -419,7 +589,7 @@ export class McpProbeService { client.listTools(), options.startedAt, 'protocol', - () => transport.close(), + closeTransport, ); } catch (error) { const protocolError = error instanceof McpProbeTimeoutError @@ -468,14 +638,24 @@ 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. - const teardown = Promise.allSettled([client.close(), transport.close()]); + // 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. + // 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. The transport close is the one a + // timeout may already have started. + const teardown = Promise.allSettled([ + settledClose(() => client.close()), + closeTransport(), + ]); + 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); } @@ -534,13 +714,16 @@ 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 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/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/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/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/adapter-metadata.test.ts b/packages/agent-bundle/tests/adapter-metadata.test.ts index 6911bd0cc..d681d4013 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', }, ], }); @@ -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/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'); 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-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 }); + } +}); diff --git a/packages/agent-bundle/tests/mcp-probe-service.test.ts b/packages/agent-bundle/tests/mcp-probe-service.test.ts index 832484ac4..a2586d860 100644 --- a/packages/agent-bundle/tests/mcp-probe-service.test.ts +++ b/packages/agent-bundle/tests/mcp-probe-service.test.ts @@ -162,6 +162,166 @@ 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', + }, + { + 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 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 }, + 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', + }, + { + 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', + }, + { + description: 'Glued id_https://dave:pw1@example.test/private and ref9https://gwen:pw2@example.test/', + inputSchema: { type: 'object' as const }, + name: 'url-userinfo-after-identifier', + }, + { + description: 'Glued sock_unix:///home/frank/private.sock', + inputSchema: { type: 'object' as const }, + name: 'local-uri-after-identifier', + }, + { + description: 'Glued ref9https://example.test/docs and x_wss://relay.example.test/feed survive', + inputSchema: { type: 'object' as const }, + name: 'network-url-after-identifier', + }, + ], + }), + }), + }); + + 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]', + // 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 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 + // 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 inside userinfo is masked with the rest of the credential. + 'Backslash in the password https://[REDACTED]@example.test/ and ws://[REDACTED]@example.test/feed', + // Neither rule is anchored to a word boundary: a URL glued to a preceding + // identifier (`_` or a digit in front of the scheme) is still masked... + 'Glued id_https://[REDACTED]@example.test/private and ref9https://[REDACTED]@example.test/', + // ...a glued local-resource URI still fails closed... + '[REDACTED]', + // ...and a glued network link is still exempt from the path rule. + 'Glued ref9https://example.test/docs and x_wss://relay.example.test/feed survive', + ]); + const serialized = JSON.stringify(report); + for (const secret of ['hunter2', 'pa55', 'alice', 'pa@ss', '@ss@', 's3cret', 'bob', 'carol', 'dave', 'gwen', 'frank', 'pw1', 'pw2']) { + expect(serialized).not.toContain(secret); + } + } finally { + await rm(root, { force: true, recursive: true }); + } +}); + it('truncates server instructions to the named text budget', async () => { const root = await createBundle(); try { @@ -271,6 +431,94 @@ 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('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(); @@ -329,6 +577,288 @@ 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('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('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('retries plugin-data removal once a capped teardown finally settles (#397 review)', async () => { + const root = await createBundle(); + // 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; + const closeReleased = new Promise((resolvePromise) => { + 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'); + 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' }); + 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'); + + // 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'); + + // Once the transport finishes closing and releases the directory, the + // best-effort retry chained to that settlement removes it. + releaseClose(); + 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(['removal-rejected', 'settled', 'transport-closed', 'plugin-data-removed']); + await expect(readFile(join(pluginData, 'proof.txt'), 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + await rm(parent, { force: true, recursive: true }); + await rm(root, { force: true, recursive: true }); + } +}); + +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; 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..995f17acd 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 { join } from 'node:path'; +import { mkdir, mkdtemp, readdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { isAbsolute, 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,66 @@ 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. + const owner = rstestWorkerRootOwner(root); + expect(owner).toMatchObject({ + cwd: process.cwd(), + pid: process.pid, + workerId: process.env['RSTEST_WORKER_ID'] ?? '0', + }); + // 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 () => { + 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..3ee13cc7a 100644 --- a/packages/create-agent-bundle/src/scaffold.ts +++ b/packages/create-agent-bundle/src/scaffold.ts @@ -99,12 +99,82 @@ 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) { + // 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 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 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')); + } + 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 +203,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`'); + // 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'); + 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 }; +};