fix: address Select forwarding and MCP readiness review
This commit is contained in:
parent
a4a916c97a
commit
b6f015a9f6
|
|
@ -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`)
|
||||
|
|
|
|||
34
src/app/automation/bridge/child-ready.ts
Normal file
34
src/app/automation/bridge/child-ready.ts
Normal file
|
|
@ -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<void> {
|
||||
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()
|
||||
})
|
||||
})
|
||||
}
|
||||
|
|
@ -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<void> {
|
||||
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<void>((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')
|
||||
|
|
|
|||
|
|
@ -34,7 +34,7 @@ const styles = tv(theme)()
|
|||
|
||||
<template>
|
||||
<SelectRoot v-model="modelValue">
|
||||
<SelectTrigger v-if="$slots.trigger" as-child>
|
||||
<SelectTrigger v-if="$slots.trigger" as-child v-bind="$attrs" :aria-label="label">
|
||||
<slot name="trigger" />
|
||||
</SelectTrigger>
|
||||
<SelectTrigger
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ for (const custom of [false, true]) {
|
|||
const { default: AppSelect } = await import(selectPath)
|
||||
const { default: IconButton } = await import(buttonPath)
|
||||
const value = ref('first')
|
||||
const disabled = ref(false)
|
||||
const host = document.createElement('div')
|
||||
host.dataset.slot = 'select-contract-fixture'
|
||||
document.body.append(host)
|
||||
|
|
@ -24,6 +25,8 @@ for (const custom of [false, true]) {
|
|||
AppSelect,
|
||||
{
|
||||
modelValue: value.value,
|
||||
disabled: disabled.value,
|
||||
'data-command': 'choose-test-style',
|
||||
'onUpdate:modelValue': (next: string) => {
|
||||
value.value = next
|
||||
},
|
||||
|
|
@ -39,6 +42,9 @@ for (const custom of [false, true]) {
|
|||
app.mount(host)
|
||||
return {
|
||||
value: () => value.value,
|
||||
disable: () => {
|
||||
disabled.value = true
|
||||
},
|
||||
dispose() {
|
||||
app.unmount()
|
||||
host.remove()
|
||||
|
|
@ -61,6 +67,11 @@ for (const custom of [false, true]) {
|
|||
await page.keyboard.press('Escape')
|
||||
await expect(trigger).toBeFocused()
|
||||
if (!custom) await expect(trigger).toHaveText('Second')
|
||||
await expect(trigger).toHaveAttribute('data-command', 'choose-test-style')
|
||||
await fixture.evaluate((state) => state.disable())
|
||||
await expect(trigger).toBeDisabled()
|
||||
await trigger.dispatchEvent('click')
|
||||
await expect(page.getByRole('option')).toHaveCount(0)
|
||||
} finally {
|
||||
await fixture.evaluate((state) => state.dispose())
|
||||
await fixture.dispose()
|
||||
|
|
|
|||
20
tests/engine/app/automation/child-ready.test.ts
Normal file
20
tests/engine/app/automation/child-ready.test.ts
Normal file
|
|
@ -0,0 +1,20 @@
|
|||
import { expect, test } from 'bun:test'
|
||||
import { spawn } from 'node:child_process'
|
||||
|
||||
import { waitForChildReady } from '@/app/automation/bridge/child-ready'
|
||||
|
||||
test('accepts a readiness marker split across child output chunks', async () => {
|
||||
const child = spawn(
|
||||
process.execPath,
|
||||
['-e', 'process.stderr.write("ready:"); setTimeout(() => process.stderr.write("test\\n"), 10)'],
|
||||
{ stdio: ['ignore', 'ignore', 'pipe'] }
|
||||
)
|
||||
await waitForChildReady(child, 'ready:test')
|
||||
})
|
||||
|
||||
test('rejects child exit without readiness even if other output looks healthy', async () => {
|
||||
const child = spawn(process.execPath, ['-e', 'process.stderr.write("HTTP healthy\\n")'], {
|
||||
stdio: ['ignore', 'ignore', 'pipe']
|
||||
})
|
||||
await expect(waitForChildReady(child, 'ready:test')).rejects.toThrow('exited before readiness')
|
||||
})
|
||||
|
|
@ -100,18 +100,29 @@ describe('MCP Vite development server', () => {
|
|||
])
|
||||
})
|
||||
|
||||
test('does not accept an unrelated healthy endpoint during authenticated startup', async () => {
|
||||
let requests = 0
|
||||
await waitForAutomationHealth(
|
||||
'ws://localhost:7682',
|
||||
async (_input, init) => {
|
||||
expect(new Headers(init?.headers).get('authorization')).toBe('Bearer test-owned-token')
|
||||
requests++
|
||||
return Response.json(requests === 1 ? { status: 'ok' } : { status: 'no_app', tools: [] })
|
||||
},
|
||||
{ authToken: 'test-owned-token' }
|
||||
)
|
||||
expect(requests).toBe(2)
|
||||
test('health probes do not send credentials', async () => {
|
||||
await waitForAutomationHealth('ws://localhost:7682', async (_input, init) => {
|
||||
expect(new Headers(init?.headers).has('authorization')).toBe(false)
|
||||
return new Response(null)
|
||||
})
|
||||
})
|
||||
|
||||
test('propagates child exit while the request is in flight', async () => {
|
||||
let running = true
|
||||
await expect(
|
||||
waitForAutomationHealth(
|
||||
'ws://localhost:7682',
|
||||
async () => {
|
||||
running = false
|
||||
return new Response(null)
|
||||
},
|
||||
{
|
||||
assertRunning() {
|
||||
if (!running) throw new Error('child exited in flight')
|
||||
}
|
||||
}
|
||||
)
|
||||
).rejects.toThrow('child exited in flight')
|
||||
})
|
||||
|
||||
test('rejects a failed child before probing another endpoint', async () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue