From b6f015a9f6418f302a263bac38abc3ba6dadecf7 Mon Sep 17 00:00:00 2001 From: Danila Poyarkov Date: Thu, 10 Sep 2026 23:54:03 +0300 Subject: [PATCH] fix: address Select forwarding and MCP readiness review --- packages/mcp/src/index.ts | 5 ++ src/app/automation/bridge/child-ready.ts | 34 +++++++++++++ src/app/automation/bridge/vite-plugin.ts | 51 ++++++++++--------- src/components/ui/select/AppSelect.vue | 2 +- tests/e2e/properties/select-trigger.spec.ts | 11 ++++ .../engine/app/automation/child-ready.test.ts | 20 ++++++++ .../app/automation/mcp-vite-plugin.test.ts | 35 ++++++++----- 7 files changed, 122 insertions(+), 36 deletions(-) create mode 100644 src/app/automation/bridge/child-ready.ts create mode 100644 tests/engine/app/automation/child-ready.test.ts diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 9f7f53444..519154833 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -98,6 +98,11 @@ const handle = await startServer({ appAttachTimeoutMs }) +const readyMarker = process.env.OPENPENCIL_MCP_READY_MARKER +if (readyMarker && /^open-pencil-ready:[a-f0-9-]{36}$/.test(readyMarker)) { + process.stderr.write(`${readyMarker}\n`) +} + process.stderr.write(`OpenPencil MCP server\n`) if (handle.socketPath) process.stderr.write(` Socket: ${handle.socketPath}\n`) if (handle.httpPort) process.stderr.write(` HTTP: http://127.0.0.1:${handle.httpPort}\n`) diff --git a/src/app/automation/bridge/child-ready.ts b/src/app/automation/bridge/child-ready.ts new file mode 100644 index 000000000..b77ea9516 --- /dev/null +++ b/src/app/automation/bridge/child-ready.ts @@ -0,0 +1,34 @@ +import type { ChildProcess } from 'node:child_process' +import { createInterface } from 'node:readline' +import type { Readable } from 'node:stream' + +/** Readiness is acknowledged over the owned child pipe, never by a public HTTP listener. */ +export function waitForChildReady( + child: ChildProcess & { stderr: Readable }, + marker: string, + timeoutMs = 10_000 +): Promise { + return new Promise((resolve, reject) => { + const lines = createInterface({ input: child.stderr }) + const timer = setTimeout(() => finish(new Error('MCP child readiness timed out')), timeoutMs) + function finish(error?: Error) { + clearTimeout(timer) + lines.close() + child.off('exit', onExit) + child.off('error', onError) + if (error) reject(error) + else resolve() + } + function onExit() { + finish(new Error('MCP child exited before readiness acknowledgement')) + } + function onError(error: Error) { + finish(error) + } + child.once('exit', onExit) + child.once('error', onError) + lines.on('line', (line) => { + if (line === marker) finish() + }) + }) +} diff --git a/src/app/automation/bridge/vite-plugin.ts b/src/app/automation/bridge/vite-plugin.ts index 93082df21..1689ef17e 100644 --- a/src/app/automation/bridge/vite-plugin.ts +++ b/src/app/automation/bridge/vite-plugin.ts @@ -1,5 +1,5 @@ import { spawn } from 'node:child_process' -import { createHash } from 'node:crypto' +import { createHash, randomUUID } from 'node:crypto' import { mkdir } from 'node:fs/promises' import type { IncomingMessage } from 'node:http' import { tmpdir } from 'node:os' @@ -15,6 +15,7 @@ import { parseDevMCPConfiguration, type DevMCPConfiguration } from '../mcp/dev-control' +import { waitForChildReady } from './child-ready' interface AutomationEnvironmentOptions { authToken: string | null @@ -103,28 +104,22 @@ export async function readDevMCPConfiguration(request: IncomingMessage): Promise export async function waitForAutomationHealth( browserURL: string, fetcher: typeof fetch = fetch, - options: { authToken?: string | null; assertRunning?: () => void } = {} + options: { assertRunning?: () => void } = {} ): Promise { const healthURL = `${browserURL.replace(/^ws/, 'http')}/health` for (let attempt = 0; attempt < CHILD_HEALTH_ATTEMPTS; attempt++) { options.assertRunning?.() + let healthy = false try { - const response = await fetcher(healthURL, { - signal: AbortSignal.timeout(2000), - headers: options.authToken ? { Authorization: `Bearer ${options.authToken}` } : undefined - }) - if (response.ok) { - const authenticated = - !options.authToken || - Array.isArray(((await response.json()) as { tools?: unknown }).tools) - options.assertRunning?.() - if (authenticated) return - } + const response = await fetcher(healthURL, { signal: AbortSignal.timeout(2000) }) + healthy = response.ok } catch (error) { if (attempt === CHILD_HEALTH_ATTEMPTS - 1) { console.warn(`[MCP] Health check failed at ${healthURL}`, error) } } + options.assertRunning?.() + if (healthy) return await new Promise((resolve) => { setTimeout(resolve, CHILD_HEALTH_DELAY_MS) }) @@ -205,18 +200,23 @@ export function automationPlugin( const spawnArgs = options.portlessServiceName ? ['run', '--name', options.portlessServiceName, ...command] : command.slice(1) + const readyMarker = `open-pencil-ready:${randomUUID()}` const spawned = spawn(spawnCommand, spawnArgs, { stdio: ['ignore', 'inherit', 'pipe'], - env: createAutomationEnvironment({ - authToken, - baseEnv: process.env, - configuration, - corsOrigin: options.corsOrigin, - discoveryPath, - httpPort: options.httpPort, - socketPath - }) + env: { + ...createAutomationEnvironment({ + authToken, + baseEnv: process.env, + configuration, + corsOrigin: options.corsOrigin, + discoveryPath, + httpPort: options.httpPort, + socketPath + }), + OPENPENCIL_MCP_READY_MARKER: readyMarker + } }) + const ready = waitForChildReady(spawned, readyMarker) child = spawned spawned.on('error', (err) => { @@ -242,8 +242,13 @@ export function automationPlugin( if (child === spawned) child = null }) + try { + await ready + } catch (error) { + spawned.kill() + throw error + } await waitForAutomationHealth(options.browserURL, fetch, { - authToken: configuration.authenticationEnabled ? authToken : null, assertRunning() { if (child !== spawned || spawned.exitCode !== null || spawned.signalCode !== null) { throw new Error('MCP child exited before becoming ready') diff --git a/src/components/ui/select/AppSelect.vue b/src/components/ui/select/AppSelect.vue index 94c6f2919..b6b49f972 100644 --- a/src/components/ui/select/AppSelect.vue +++ b/src/components/ui/select/AppSelect.vue @@ -34,7 +34,7 @@ const styles = tv(theme)()