From 01a02deb6b1fc1cbb7ba78dabfed7866f1958a3a Mon Sep 17 00:00:00 2001 From: DavidBabinec Date: Sat, 5 Sep 2026 11:05:18 +0200 Subject: [PATCH] fix(mcp): a hung browser tool no longer wedges the workspace bridge The workspace services relayed tools one at a time, so a tool whose promise never settled blocked every later request on the stream until the tab reloaded, while get_context kept reporting the workspace as connected because it only read the bridge registry. Each relayed tool is now bounded by a 60 s client deadline under the relay's 90 s timeout, and get_context probes each registered bridge with a real round-trip so a stuck tab reads as unresponsive instead of connected. --- docs/e2e/feature-matrix.md | 3 + docs/e2e/feature-validation.tsv | 1 + docs/features/mcp-connectors.md | 4 +- server/ai/mcp/editorBridge.ts | 43 +++++ server/ai/mcp/server.ts | 4 +- server/ai/mcp/tools/contextTool.ts | 26 ++- server/ai/runtime/transport.ts | 4 +- server/ai/runtime/types.ts | 14 +- .../agent/mcpBridgeRequestLoop.test.ts | 141 ++++++++++++++++ src/__tests__/ai/bridgeTimeoutAbort.test.ts | 10 ++ src/__tests__/ai/editorBridgePing.test.ts | 114 +++++++++++++ src/__tests__/ai/mcpContextTool.test.ts | 88 ++++++++-- src/admin/ai/useMcpWorkspaceBridge.ts | 67 ++++++-- src/core/ai/bridgePing.ts | 14 ++ src/core/ai/index.ts | 1 + tests/e2e/mcp-bridge-liveness.e2e.ts | 150 ++++++++++++++++++ 16 files changed, 651 insertions(+), 33 deletions(-) create mode 100644 src/__tests__/agent/mcpBridgeRequestLoop.test.ts create mode 100644 src/__tests__/ai/editorBridgePing.test.ts create mode 100644 src/core/ai/bridgePing.ts create mode 100644 tests/e2e/mcp-bridge-liveness.e2e.ts diff --git a/docs/e2e/feature-matrix.md b/docs/e2e/feature-matrix.md index 2f195d1ae..4271883c2 100644 --- a/docs/e2e/feature-matrix.md +++ b/docs/e2e/feature-matrix.md @@ -284,6 +284,7 @@ CONTENT-010 note: `content-seo-meta.e2e.ts` authors and publishes a Posts entry | AI-006 | P2 | partial | Audit | Review AI usage rollups by user/scope/model/day | Usage generated by chat | AI Audit tab/dashboard widget | Audit tab renders model, scope, token, and daily rollups from a real streamed chat turn | empty usage, deleted labels, bad timezone, dashboard widget, mobile table | | AI-007 | P2 | partial | Drivers | Use provider REST drivers without SDK lock-in | Mocked or local provider | AI runtime/provider drivers | Direct drivers map messages/tools, stream events, usage, model catalogues, context windows, and prices without provider SDKs | malformed SSE, rate limits, unknown pricing | | AI-010 | P2 | ✅ | Tool Bridge | Keep the workspace MCP bridge stream readable in a real browser | Logged in | Open the Site editor, observe `/admin/api/ai/editor-bridge` | The bridge responds 200 as `text/event-stream` with no-transform caching and the client holds the stream open without parse errors | proxy reframing, buffered body, client teardown loop | +| AI-011 | P2 | ✅ | Tool Bridge | Learn from `get_context` whether the open workspace will actually service browser tools | Fresh login, personal access token | Open the Site editor, call `get_context` over MCP, wedge the tab, call again | `siteConnected` is true only while the tab answers a probe; a wedged tab reads as `unresponsive: ['site']` and recovers once it answers again; an unsupported `` insert errors cleanly without breaking later calls | stale registration reported as connected, probe latency, reconnect after a stale result, step-up rotating the session | AI-001 note: Ollama base-URL credential create/list/delete and offline auto-default guarding are automated in `ai.e2e.ts`; live provider model tests, credential update/masking, default reassignment, and mobile layout remain lower-level or future browser coverage. @@ -299,6 +300,8 @@ AI-007 note: direct driver coverage is automated in `src/__tests__/ai/*Mapping.t AI-002 note: Data-scope default selection, save feedback, reload persistence, clear feedback, and post-clear credential deletion are automated in `ai.e2e.ts` using an offline Ollama credential and deterministic fallback model list. Site/content/plugin scope permutations, no-credential empty state, stale credential recovery, live model catalogues, permission variants, and mobile layout remain future browser or lower-level coverage. +AI-011 note: `mcp-bridge-liveness.e2e.ts` mints its own personal access token through the step-up and token endpoints from inside the page, drives `get_context`, `site_list_documents`, `site_insert_html`, and `site_get_node_html` over the real MCP endpoint, and wedges the tab with a 7 s busy loop to prove the probe times out and recovers. The 60 s per-tool client deadline is unit-covered in `mcpBridgeRequestLoop.test.ts`; hosted-proxy stream drops remain manual. + ## Plugins | ID | Priority | Auto | Area | User Goal | Setup | Path | Expected Outcome | Watch For | diff --git a/docs/e2e/feature-validation.tsv b/docs/e2e/feature-validation.tsv index b575e1b88..7146b2681 100644 --- a/docs/e2e/feature-validation.tsv +++ b/docs/e2e/feature-validation.tsv @@ -150,3 +150,4 @@ CONTENT-010 Entry SEO title and description reach the published As a cont ADMIN-009 Settings Escape dismissal after focus loss As an admin user, I want Escape to close the Settings modal even after I click non-focusable chrome so the dialog never traps me. Escape closes the Settings modal after a click on a heading or dead space moves focus to body. Focus on body; focus inside nested controls; backdrop click still works. Escape handling is document-level, not a React onKeyDown scoped to the dialog subtree. src/admin/modals/Settings/SettingsModal.tsx; tests/e2e/settings-escape.e2e.ts Regression spec for the fix shipped in PR #272; unit coverage drives fireEvent at chosen elements, only a browser reproduces real focus state. Happy: open Settings from the Site editor toolbar, click the first heading, press Escape, dialog hides. Passing in the repaired suite run 2026-08-30 0 None Rescued spec recovered from a prior verification session and added as tests/e2e/settings-escape.e2e.ts. Repair details in PR #461. 2026-08-30 ADMIN-010 AI workspace section navigation entry As an owner, I want the AI workspace reachable from the admin section navigation so I do not have to type the URL. A capability-gated AI link renders in the section navigation with href /admin/ai and routes on click. Users without AI capabilities see no entry; landing routes still include /admin/ai. Navigation uses the in-house admin router; the entry is gated on ai.providers.manage or ai.audit.read. src/admin/shared/AdminSectionNavigation/AdminSectionNavigation.tsx; tests/e2e/ai-nav.e2e.ts Regression spec for the fix shipped in PR #346. Happy: from Dashboard, the AI link is visible with the right href and clicking it lands on /admin/ai. Passing in the repaired suite run 2026-08-30 0 None Rescued spec recovered from a prior verification session and added as tests/e2e/ai-nav.e2e.ts. Repair details in PR #461. 2026-08-30 AI-010 Workspace MCP bridge stream stays readable As a connector user, I want the editor bridge stream to survive reverse proxies so connected agents keep working. The bridge responds 200 with content-type text/event-stream, cache-control no-cache, no-transform, and x-accel-buffering no, and the browser client holds the stream open without parse or stream errors. Buffering proxies reframing the body; client tearing the stream down as unreadable. The body stays newline-delimited JSON while the media type advertises an event stream so intermediaries flush incrementally. server/ai/mcp/editorBridge.ts; src/admin/ai/useMcpWorkspaceBridge.ts; tests/e2e/mcp-editor-bridge.e2e.ts Regression spec for the fix shipped in PR #282; unit coverage stubs fetch, only a browser exercises the real stream reader. Happy: open the Site editor, capture the bridge response, assert the streaming headers, and verify no mcp-workspace-bridge console errors for three seconds. Passing in the repaired suite run 2026-08-30 0 None Rescued spec recovered from a prior verification session and added as tests/e2e/mcp-editor-bridge.e2e.ts. Repair details in PR #461. 2026-08-30 +AI-011 get_context reports live workspace connectivity As a connector user, I want get_context to tell me whether the open workspace will actually service browser tools, so a stuck tab is diagnosed instead of retried. siteConnected and contentConnected are true only when that tab answered a liveness probe within 2.5 s; a registered workspace that does not answer is listed under unresponsive; the flag recovers once the tab answers again; an unsupported insert returns a validation error and later calls keep working. Registration kept by the idle lease after the tab stopped answering; busy main thread; stale probe result forcing a reconnect; step-up rotating the session used to mint the token. The probe is a real round-trip of the MCP_BRIDGE_PING_TOOL pseudo-tool answered by the client request loop itself; each relayed tool is bounded by a 60 s client deadline under the 90 s relay timeout. server/ai/mcp/editorBridge.ts; server/ai/mcp/tools/contextTool.ts; src/admin/ai/useMcpWorkspaceBridge.ts; tests/e2e/mcp-bridge-liveness.e2e.ts Runs on a fresh login because minting a personal access token requires a step-up. Hosted reverse-proxy stream drops are not reproducible locally. Happy: login, mint a token via step-up + access-tokens, open the Site editor, assert get_context connected; insert a bare and assert the validation error and a following read; wedge the tab with a 7 s busy loop, assert unresponsive: [site]; poll until connected again. Passing in the verification run 2026-09-05 0 None Added for issue #490 alongside the client per-tool deadline and the get_context liveness probe. 2026-09-05 diff --git a/docs/features/mcp-connectors.md b/docs/features/mcp-connectors.md index 30febc736..4173e5228 100644 --- a/docs/features/mcp-connectors.md +++ b/docs/features/mcp-connectors.md @@ -141,7 +141,7 @@ executeAiTool(...) / live editor bridge | `auth.ts` | Resolves OAuth access tokens or personal tokens to `{ connectorId, userId, capabilities }`; returns a discovery-aware 401 otherwise. | | `transports/http.ts` | Authenticated, Origin-validated `createMcpHandler` entry for MCP 2026-07-28 plus the stateless 2025 fallback. | | `server.ts` / `registry.ts` | Low-level SDK server, TypeBox input schemas, catalog deduplication, and capability filtering. | -| `editorBridge.ts` | Per-user, per-scope live workspace bridge. The stream carries an **idle lease** (120s, re-armed by every relayed tool request) so an active batch is never cut mid-flight; only quiet streams recycle. The workspace's reconnect loop (`useMcpWorkspaceBridge`) reopens a recycled healthy stream immediately off the stream-end network event — deliberately timer-free, because hidden webviews (backgrounded browser tabs) clamp timers to minutes while network events still fire — and a tab becoming visible short-circuits any pending retry delay. | +| `editorBridge.ts` | Per-user, per-scope live workspace bridge. The stream carries an **idle lease** (120s, re-armed by every relayed tool request) so an active batch is never cut mid-flight; only quiet streams recycle. The workspace's reconnect loop (`useMcpWorkspaceBridge`) reopens a recycled healthy stream immediately off the stream-end network event — deliberately timer-free, because hidden webviews (backgrounded browser tabs) clamp timers to minutes while network events still fire — and a tab becoming visible short-circuits any pending retry delay. `pingEditorBridge` is the **liveness probe**: a real round-trip of the `MCP_BRIDGE_PING_TOOL` pseudo-tool (`@core/ai`) through the stream, answered by the client loop itself within 2.5 s, or the workspace reads as `unresponsive`. Registration alone is not connectivity — a tab whose loop is stuck, or whose connection died behind a proxy, keeps its entry until the idle lease recycles it. | | `tools/publishTool.ts` | Explicit canonical full-site publish with MCP audit metadata. | | `tools/uploadMediaTool.ts` | Server-resolved image upload (`media_upload`) — inline base64 or SSRF-guarded `sourceUrl` download, through the shared media pipeline. | @@ -155,6 +155,8 @@ Server-resolved tools work without an editor open. They include content reads, ` Browser tools run against the connection owner's live workspace. Site structure, HTML/CSS, page lifecycle, design-token, content mutation, code-asset, and live-DOM tools route to the matching open Site or Content workspace. If that workspace is not open, the tool returns a scope-specific error while headless tools remain available. `tools/list` states that requirement in each browser tool's description, so a client learns the precondition when it picks the tool rather than from a failed call. +The workspace services relayed tools one at a time, in order. Each tool (plus its persistence step) is bounded by a 60 s client-side deadline (`useMcpWorkspaceBridge`), under the relay's 90 s per-call timeout (`server/ai/runtime/transport.ts`): a tool whose promise never settles returns an error naming it and the loop moves on, instead of blocking every later request until the tab reloads. `get_context` reports `siteConnected` / `contentConnected` from the liveness probe above — true only when that tab answered just now — and lists workspaces that opened a bridge but did not answer under `unresponsive`, which means "reload that tab", not "retry". + There is intentionally no headless page-tree mutation path. The open editor store is the single source of truth for draft edits; a second DB mutation path would desynchronize node state and overwrite the live document. Relayed edits need no post-tool save step: store mutations stream to the collab relay the moment they land, and every headless read (plus `site_publish`) flushes the relay server-side before it touches the DB — so a following read or publish always observes the edit. There is no client-side save flush, and no window in which the MCP caller can see stale data. Writes remain drafts. Clients should finish and verify an edit sequence, then call `site_publish` once only when deployment was requested. diff --git a/server/ai/mcp/editorBridge.ts b/server/ai/mcp/editorBridge.ts index 8c2a7f334..1ab2cc1ec 100644 --- a/server/ai/mcp/editorBridge.ts +++ b/server/ai/mcp/editorBridge.ts @@ -19,6 +19,7 @@ * connector can only ever reach the open workspace of its OWN owner and a * content tool can never be dispatched to the site editor (or vice versa). */ +import { MCP_BRIDGE_PING_TOOL } from '@core/ai' import type { AiBrowserBridge, AiStreamEvent } from '../runtime/types' import { createBridge, encodeStreamEvent } from '../runtime' @@ -29,6 +30,23 @@ interface EditorBridgeEntry { } export type EditorBridgeScope = 'site' | 'content' + +/** + * What a liveness probe learned about a workspace bridge: + * - `live` — the tab answered just now; browser tools will be serviced. + * - `unresponsive` — a stream is registered but nothing answered in time. + * The tab's request loop is stuck or its connection died without the + * server noticing. Relayed tools will time out until that tab reloads. + * - `closed` — no stream is registered for this scope. + */ +export type EditorBridgeLiveness = 'live' | 'unresponsive' | 'closed' + +/** + * How long a workspace gets to answer a probe. A healthy loop answers in + * milliseconds; a stuck one never does, and a caller orienting itself should + * not wait the full per-tool timeout to learn that. + */ +const BRIDGE_PING_TIMEOUT_MS = 2_500 /** * How long a stream may sit with NO tool traffic before the server drops it. * This is an IDLE lease: every relayed tool request re-arms it, so an active @@ -54,6 +72,31 @@ export function hasEditorBridge(userId: string, scope: EditorBridgeScope): boole return byUser.get(userId)?.has(scope) ?? false } +/** + * Prove the registered workspace still answers. Registration only says a tab + * opened a stream: a tab whose request loop is stuck behind a tool that never + * settled, or whose connection died behind a proxy, keeps its entry until the + * idle lease recycles it, so `hasEditorBridge` alone reported "connected" + * while every relayed call timed out (#490). The probe is a real round-trip + * through the same stream and the same client loop every tool uses. + */ +export async function pingEditorBridge( + userId: string, + scope: EditorBridgeScope, + timeoutMs: number = BRIDGE_PING_TIMEOUT_MS, +): Promise { + const bridge = getEditorBridgeForUser(userId, scope) + if (!bridge) return 'closed' + try { + const answer = await bridge.callBrowser(MCP_BRIDGE_PING_TOOL, {}, { timeoutMs }) + return answer.ok ? 'live' : 'unresponsive' + } catch (_err) { + // A rejected probe (timed out, or the stream was torn down mid-probe) IS + // the finding; the registry says whether anything is left to talk to. + return hasEditorBridge(userId, scope) ? 'unresponsive' : 'closed' + } +} + /** * Open the long-lived stream the editor consumes. The server pushes * `toolRequest` events down it whenever an MCP browser tool is invoked for this diff --git a/server/ai/mcp/server.ts b/server/ai/mcp/server.ts index f3c8148d9..b9b920a67 100644 --- a/server/ai/mcp/server.ts +++ b/server/ai/mcp/server.ts @@ -167,7 +167,7 @@ export function buildMcpServer(ctx: McpServerContext): Server { } bridge = browserScope === 'content' ? { - callBrowser: async (toolName, input) => { + callBrowser: async (toolName, input, options) => { await authorizeMcpContentTool( ctx.db, ctx.userId, @@ -177,7 +177,7 @@ export function buildMcpServer(ctx: McpServerContext): Server { ) const current = getEditorBridgeForUser(ctx.userId, browserScope) if (!current) throw new Error(NO_WORKSPACE_MESSAGE[browserScope]) - return current.callBrowser(toolName, input) + return current.callBrowser(toolName, input, options) }, } : live diff --git a/server/ai/mcp/tools/contextTool.ts b/server/ai/mcp/tools/contextTool.ts index d63241d84..224da54dd 100644 --- a/server/ai/mcp/tools/contextTool.ts +++ b/server/ai/mcp/tools/contextTool.ts @@ -7,14 +7,15 @@ * 2. which "everywhere" / post-type templates wrap pages (so the agent isn't * surprised by a nav/footer it didn't author). * - * Headless: editor presence comes from the bridge registry; templates + author - * come straight from the DB. No browser snapshot. + * Headless: editor presence is a live probe through each registered bridge + * (`pingEditorBridge`); templates + author come straight from the DB. No + * browser snapshot. */ import { Type } from '@core/utils/typeboxHelpers' import type { CoreCapability } from '@core/capabilities' import type { AiTool, ToolContext } from '../../runtime/types' import { getDraftSite } from '../../../repositories/site' -import { hasEditorBridge } from '../editorBridge' +import { pingEditorBridge, type EditorBridgeScope } from '../editorBridge' const CONTEXT_READ_CAPS: readonly CoreCapability[] = [ 'site.read', @@ -50,7 +51,7 @@ export const contextMcpTools: AiTool[] = [ { name: 'get_context', description: - 'Orient yourself before editing: reports whether the Site editor and Content workspace are connected (browser tools require their matching workspace), and which templates wrap pages — an "everywhere" template applies a nav/footer/etc. to every page, so anything you author is in addition to it. Pass entryId to also learn whether a template wraps that specific page. Headless — no editor needed. Call this first if a browser tool returns an "open the workspace" error.', + 'Orient yourself before editing: reports whether the Site editor and Content workspace are connected (browser tools require their matching workspace), and which templates wrap pages — an "everywhere" template applies a nav/footer/etc. to every page, so anything you author is in addition to it. `siteConnected` / `contentConnected` are live: true only when that open tab answered a probe just now. `unresponsive` lists workspaces whose tab opened a bridge but did not answer — that tab is stuck or its connection died, relayed tools to it will time out, and the fix is to reload that tab, not to retry. Pass entryId to also learn whether a template wraps that specific page. Headless — no editor needed. Call this first if a browser tool returns an "open the workspace" error or times out.', scope: 'site', execution: 'server', inputSchema: GetContextInput, @@ -75,11 +76,24 @@ export const contextMcpTools: AiTool[] = [ })) .sort((a, b) => a.priority - b.priority) + // Probe both workspaces for real rather than reading the registry: an + // entry whose tab stopped answering read as "connected" for as long as + // the idle lease kept it, which sent callers chasing their own inputs + // while every relayed tool timed out (#490). + const [siteLiveness, contentLiveness] = await Promise.all([ + pingEditorBridge(ctx.userId, 'site'), + pingEditorBridge(ctx.userId, 'content'), + ]) + const unresponsive: EditorBridgeScope[] = [] + if (siteLiveness === 'unresponsive') unresponsive.push('site') + if (contentLiveness === 'unresponsive') unresponsive.push('content') + const result: Record = { site: site ? { name: site.name } : null, editor: { - siteConnected: hasEditorBridge(ctx.userId, 'site'), - contentConnected: hasEditorBridge(ctx.userId, 'content'), + siteConnected: siteLiveness === 'live', + contentConnected: contentLiveness === 'live', + unresponsive, }, templates, } diff --git a/server/ai/runtime/transport.ts b/server/ai/runtime/transport.ts index c6cebf784..1109df571 100644 --- a/server/ai/runtime/transport.ts +++ b/server/ai/runtime/transport.ts @@ -90,7 +90,7 @@ export function createBridge( let destroyed = false const bridge: AiBrowserBridge = { - callBrowser(toolName, input) { + callBrowser(toolName, input, options) { if (destroyed) { return Promise.reject(new Error('AI chat stream ended before tool result arrived.')) } @@ -108,7 +108,7 @@ export function createBridge( } const timer = setTimeout( () => settle(new Error(`Browser tool "${toolName}" result timed out.`)), - timeoutMs, + options?.timeoutMs ?? timeoutMs, ) const onAbort = () => settle(new Error('AI chat stream aborted before tool result arrived.')) const cleanup = () => { diff --git a/server/ai/runtime/types.ts b/server/ai/runtime/types.ts index b75e9b4b7..d2bf2ade8 100644 --- a/server/ai/runtime/types.ts +++ b/server/ai/runtime/types.ts @@ -194,13 +194,23 @@ export type AiStreamEvent = // tools can yield a `toolRequest` and await the browser POST. // --------------------------------------------------------------------------- +export interface AiBrowserCallOptions { + /** + * Per-call wait before the pending result is reclaimed, overriding the + * bridge-wide timeout. A liveness probe wants seconds, not the 90 s a slow + * legitimate write is allowed. + */ + timeoutMs?: number +} + export interface AiBrowserBridge { /** * Forward a `toolRequest` to the browser and resolve with whatever the * browser POSTs back to /admin/api/ai/tool-result. Rejects if the stream - * closes before a result arrives (browser disconnected, stream aborted). + * closes before a result arrives (browser disconnected, stream aborted) or + * the wait times out. */ - callBrowser(toolName: string, input: unknown): Promise + callBrowser(toolName: string, input: unknown, options?: AiBrowserCallOptions): Promise } // --------------------------------------------------------------------------- diff --git a/src/__tests__/agent/mcpBridgeRequestLoop.test.ts b/src/__tests__/agent/mcpBridgeRequestLoop.test.ts new file mode 100644 index 000000000..93e08e995 --- /dev/null +++ b/src/__tests__/agent/mcpBridgeRequestLoop.test.ts @@ -0,0 +1,141 @@ +/** + * #490 — one browser tool that never settles must not wedge the workspace + * bridge. The request loop services relayed tools serially, so a hung + * dispatcher used to block every later `toolRequest` on the stream until the + * tab reloaded: each later call timed out server-side while `get_context` + * kept reporting the workspace as connected. The loop now bounds each tool, + * answers liveness probes itself, and keeps going. + */ +import { afterEach, describe, expect, it } from 'bun:test' +import { MCP_BRIDGE_PING_TOOL } from '@core/ai' +import type { AiToolOutput } from '@core/ai' +import { + executeMcpBridgeRequest, + runMcpWorkspaceBridgeConnection, +} from '@admin/ai/useMcpWorkspaceBridge' + +const realFetch = globalThis.fetch + +afterEach(() => { + globalThis.fetch = realFetch +}) + +interface PostedResult { + requestId: string + result: AiToolOutput +} + +/** + * Serve one bridge stream carrying `events`, then answer every tool-result + * POST with 200 and record it. Later connection attempts get 401 so the + * caller's loop ends deterministically. + */ +function stubBridge(events: object[]): { posted: PostedResult[] } { + const posted: PostedResult[] = [] + let streamsServed = 0 + globalThis.fetch = (async (input, init) => { + const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url + if (url.startsWith('/admin/api/ai/tool-result')) { + const body = JSON.parse(String(init?.body)) as PostedResult + posted.push({ requestId: body.requestId, result: body.result }) + return new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }) + } + streamsServed += 1 + if (streamsServed > 1) return new Response(null, { status: 401 }) + return new Response( + [...events.map((event) => JSON.stringify(event)), ''].join('\n'), + { headers: { 'Content-Type': 'text/event-stream' } }, + ) + }) as typeof fetch + return { posted } +} + +const never = () => new Promise(() => {}) + +describe('executeMcpBridgeRequest', () => { + it('bounds a tool that never settles and names the tool in the error', async () => { + const result = await executeMcpBridgeRequest(never, 'site_render_snapshot', {}, undefined, 20) + expect(result.ok).toBe(false) + expect(result.error).toContain('site_render_snapshot') + expect(result.error).toMatch(/did not finish/) + }) + + it('returns a tool result that arrives inside the deadline untouched', async () => { + const result = await executeMcpBridgeRequest( + async () => ({ ok: true, data: { nodeIds: ['n1'] } }), + 'site_insert_html', + {}, + undefined, + 1_000, + ) + expect(result).toEqual({ ok: true, data: { nodeIds: ['n1'] } }) + }) + + it('counts the persistence step against the same deadline', async () => { + const result = await executeMcpBridgeRequest( + async () => ({ ok: true }), + 'content_set_document_fields', + {}, + () => new Promise(() => {}), + 20, + ) + expect(result.ok).toBe(false) + expect(result.error).toMatch(/did not finish/) + }) +}) + +describe('runMcpWorkspaceBridgeConnection', () => { + it('keeps serving later requests after one tool hangs', async () => { + const { posted } = stubBridge([ + { type: 'bridgeReady', bridgeId: 'b1' }, + { type: 'toolRequest', requestId: 'hangs', toolName: 'site_render_snapshot', input: {} }, + { type: 'toolRequest', requestId: 'after', toolName: 'site_get_node_html', input: { nodeId: 'n' } }, + ]) + const dispatched: string[] = [] + const dispatch = async (toolName: string): Promise => { + dispatched.push(toolName) + if (toolName === 'site_render_snapshot') return never() + return { ok: true, data: { html: '

hi

' } } + } + + const lifecycle = new AbortController() + const outcome = await Promise.race([ + runMcpWorkspaceBridgeConnection('site', dispatch, undefined, lifecycle.signal, { toolDeadlineMs: 30 }), + new Promise<'stuck'>((resolve) => setTimeout(() => resolve('stuck'), 1_500)), + ]) + lifecycle.abort() + + expect(outcome).toBe('transient') + expect(dispatched).toEqual(['site_render_snapshot', 'site_get_node_html']) + expect(posted.map((p) => p.requestId)).toEqual(['hangs', 'after']) + expect(posted[0]?.result.ok).toBe(false) + expect(posted[0]?.result.error).toMatch(/did not finish/) + expect(posted[1]?.result).toEqual({ ok: true, data: { html: '

hi

' } }) + }) + + it('answers a liveness probe itself without invoking the workspace dispatcher', async () => { + const { posted } = stubBridge([ + { type: 'bridgeReady', bridgeId: 'b1' }, + { type: 'toolRequest', requestId: 'probe', toolName: MCP_BRIDGE_PING_TOOL, input: {} }, + ]) + let dispatches = 0 + const lifecycle = new AbortController() + const outcome = await runMcpWorkspaceBridgeConnection( + 'site', + async () => { + dispatches += 1 + return { ok: true } + }, + undefined, + lifecycle.signal, + ) + lifecycle.abort() + + expect(outcome).toBe('transient') + expect(dispatches).toBe(0) + expect(posted).toEqual([{ requestId: 'probe', result: { ok: true, data: { alive: true } } }]) + }) +}) diff --git a/src/__tests__/ai/bridgeTimeoutAbort.test.ts b/src/__tests__/ai/bridgeTimeoutAbort.test.ts index 338daa63a..e23ac3d5d 100644 --- a/src/__tests__/ai/bridgeTimeoutAbort.test.ts +++ b/src/__tests__/ai/bridgeTimeoutAbort.test.ts @@ -47,6 +47,16 @@ describe('bridge tool-call settlement', () => { destroy() }) + test('a per-call timeout overrides the bridge-wide one', async () => { + // Bridge-wide wait is long; the call asks for a short one (a liveness + // probe must not wait the 90 s a slow legitimate write is allowed). + const { bridge, destroy } = createBridge(() => {}, undefined, 60_000) + const started = Date.now() + await expect(bridge.callBrowser('probe', {}, { timeoutMs: 20 })).rejects.toThrow(/timed out/i) + expect(Date.now() - started).toBeLessThan(5_000) + destroy() + }) + test('a delivered tool result settles the call and clears the timeout', async () => { let requestId = '' const { bridgeId, bridge, destroy } = createBridge((ev: AiStreamEvent) => { diff --git a/src/__tests__/ai/editorBridgePing.test.ts b/src/__tests__/ai/editorBridgePing.test.ts new file mode 100644 index 000000000..8ceb108e7 --- /dev/null +++ b/src/__tests__/ai/editorBridgePing.test.ts @@ -0,0 +1,114 @@ +/** + * #490 — `get_context` reported `siteConnected: true` while every relayed + * browser tool timed out. Registration only proves a tab opened a stream; a + * stuck request loop or a connection that died behind a proxy keeps the entry + * until the idle lease recycles it. `pingEditorBridge` must therefore be a + * real round-trip, and a registered stream that does not answer must read as + * unresponsive, not connected. + */ +import { describe, expect, it } from 'bun:test' +import { MCP_BRIDGE_PING_TOOL } from '@core/ai' +import { resolveBridgeToolResult } from '../../../server/ai/runtime' +import { + createEditorBridgeStream, + hasEditorBridge, + pingEditorBridge, +} from '../../../server/ai/mcp/editorBridge' + +const decoder = new TextDecoder() + +interface StreamEvent { + type: string + bridgeId?: string + requestId?: string + toolName?: string +} + +/** + * Drain the stream like the workspace does, handing every parsed record to + * `onEvent`. Returns a stop function that releases the reader. + */ +function consume( + stream: ReadableStream, + onEvent: (event: StreamEvent) => void, +): () => void { + const reader = stream.getReader() + let stopped = false + void (async () => { + let buffer = '' + try { + for (;;) { + const { done, value } = await reader.read() + if (done || stopped) break + buffer += decoder.decode(value, { stream: true }) + const lines = buffer.split('\n') + buffer = lines.pop() ?? '' + for (const line of lines) { + const trimmed = line.trim() + if (trimmed) onEvent(JSON.parse(trimmed)) + } + } + } catch { + /* torn down by the test */ + } + })() + return () => { + stopped = true + reader.cancel().catch(() => {}) + } +} + +describe('pingEditorBridge', () => { + it('is closed when no workspace has registered a bridge', async () => { + expect(await pingEditorBridge(`ping-none-${Date.now()}`, 'site', 50)).toBe('closed') + }) + + it('is live when the workspace loop answers the probe', async () => { + const userId = `ping-live-${Date.now()}` + const controller = new AbortController() + const stream = createEditorBridgeStream(userId, 'site', controller.signal) + let bridgeId = '' + const stop = consume(stream, (event) => { + if (event.type === 'bridgeReady') bridgeId = event.bridgeId ?? '' + if (event.type === 'toolRequest' && event.toolName === MCP_BRIDGE_PING_TOOL) { + resolveBridgeToolResult(bridgeId, event.requestId ?? '', { ok: true, data: { alive: true } }) + } + }) + try { + expect(await pingEditorBridge(userId, 'site', 1000)).toBe('live') + } finally { + stop() + controller.abort() + } + }) + + it('is unresponsive, not connected, when a registered stream never answers', async () => { + const userId = `ping-stuck-${Date.now()}` + const controller = new AbortController() + // A consumer that reads the stream but never posts a result — the shape of + // a tab whose request loop is blocked behind a tool that never settled. + const stop = consume(createEditorBridgeStream(userId, 'site', controller.signal), () => {}) + try { + expect(hasEditorBridge(userId, 'site')).toBe(true) + expect(await pingEditorBridge(userId, 'site', 40)).toBe('unresponsive') + // The registry still lists it: the probe reports, it does not evict. + expect(hasEditorBridge(userId, 'site')).toBe(true) + } finally { + stop() + controller.abort() + } + }) + + it('is closed when the stream is torn down while the probe is in flight', async () => { + const userId = `ping-torn-${Date.now()}` + const controller = new AbortController() + const stop = consume(createEditorBridgeStream(userId, 'site', controller.signal), () => {}) + try { + const probe = pingEditorBridge(userId, 'site', 1000) + controller.abort() + expect(await probe).toBe('closed') + } finally { + stop() + } + }) +}) diff --git a/src/__tests__/ai/mcpContextTool.test.ts b/src/__tests__/ai/mcpContextTool.test.ts index 74d726d27..b13a0a452 100644 --- a/src/__tests__/ai/mcpContextTool.test.ts +++ b/src/__tests__/ai/mcpContextTool.test.ts @@ -2,7 +2,9 @@ import { afterEach, beforeEach, describe, expect, it } from 'bun:test' import { createCapabilityTestHarness, type CapabilityTestHarness } from '../helpers/capabilityHarness' import { contextMcpTools } from '../../../server/ai/mcp/tools/contextTool' import { createEditorBridgeStream } from '../../../server/ai/mcp/editorBridge' +import { resolveBridgeToolResult } from '../../../server/ai/runtime' import type { ToolContext } from '../../../server/ai/runtime/types' +import { MCP_BRIDGE_PING_TOOL } from '@core/ai' function ctxFor(harness: CapabilityTestHarness): ToolContext { return { @@ -64,31 +66,97 @@ describe('get_context', () => { expect(out.page.wrappedByTemplates).toContain('Shell') }) - it('reports Site and Content workspace connections independently', async () => { + it('reports Site and Content workspace connections independently, from a live probe', async () => { const siteCtrl = new AbortController() const contentCtrl = new AbortController() - createEditorBridgeStream('no-editor-user', 'site', siteCtrl.signal) + const stopSite = answerPings(createEditorBridgeStream('no-editor-user', 'site', siteCtrl.signal)) try { const siteOnly = (await getContext.handler!({}, ctxFor(harness))) as { - editor: { siteConnected: boolean; contentConnected: boolean } + editor: { siteConnected: boolean; contentConnected: boolean; unresponsive: string[] } } expect(siteOnly.editor).toEqual({ siteConnected: true, contentConnected: false, + unresponsive: [], }) - createEditorBridgeStream('no-editor-user', 'content', contentCtrl.signal) - const both = (await getContext.handler!({}, ctxFor(harness))) as { - editor: { siteConnected: boolean; contentConnected: boolean } + const stopContent = answerPings(createEditorBridgeStream('no-editor-user', 'content', contentCtrl.signal)) + try { + const both = (await getContext.handler!({}, ctxFor(harness))) as { + editor: { siteConnected: boolean; contentConnected: boolean; unresponsive: string[] } + } + expect(both.editor).toEqual({ + siteConnected: true, + contentConnected: true, + unresponsive: [], + }) + } finally { + stopContent() } - expect(both.editor).toEqual({ - siteConnected: true, - contentConnected: true, - }) } finally { + stopSite() siteCtrl.abort() contentCtrl.abort() } }) + + // #490: a registered stream whose tab no longer answers used to read as + // connected for as long as the idle lease kept its entry, while every + // relayed tool timed out. It must read as unresponsive instead. + it('reports a registered workspace that does not answer as unresponsive, not connected', async () => { + const siteCtrl = new AbortController() + // Nothing reads or answers this stream — the tab is stuck. + createEditorBridgeStream('no-editor-user', 'site', siteCtrl.signal) + try { + const out = (await getContext.handler!({}, ctxFor(harness))) as { + editor: { siteConnected: boolean; contentConnected: boolean; unresponsive: string[] } + } + expect(out.editor).toEqual({ + siteConnected: false, + contentConnected: false, + unresponsive: ['site'], + }) + } finally { + siteCtrl.abort() + } + }, 10_000) }) + +/** + * Consume a bridge stream the way an open workspace does and answer liveness + * probes, so the probe round-trip is real. Returns a stop function. + */ +function answerPings(stream: ReadableStream): () => void { + const reader = stream.getReader() + const decoder = new TextDecoder() + let bridgeId = '' + let stopped = false + void (async () => { + let buffer = '' + try { + for (;;) { + const { done, value } = await reader.read() + if (done || stopped) break + buffer += decoder.decode(value, { stream: true }) + const lines = buffer.split('\n') + buffer = lines.pop() ?? '' + for (const line of lines) { + const trimmed = line.trim() + if (!trimmed) continue + const event = JSON.parse(trimmed) as { type: string; bridgeId?: string; requestId?: string; toolName?: string } + if (event.type === 'bridgeReady') bridgeId = event.bridgeId ?? '' + if (event.type === 'toolRequest' && event.toolName === MCP_BRIDGE_PING_TOOL) { + resolveBridgeToolResult(bridgeId, event.requestId ?? '', { ok: true, data: { alive: true } }) + } + } + } + } catch { + /* torn down by the test */ + } + })() + return () => { + stopped = true + reader.cancel().catch(() => {}) + } +} diff --git a/src/admin/ai/useMcpWorkspaceBridge.ts b/src/admin/ai/useMcpWorkspaceBridge.ts index 182b5c58f..c5b8f6727 100644 --- a/src/admin/ai/useMcpWorkspaceBridge.ts +++ b/src/admin/ai/useMcpWorkspaceBridge.ts @@ -6,13 +6,22 @@ */ import { useEffect } from 'react' import { Type } from '@core/utils/typeboxHelpers' -import type { AiToolOutput } from '@core/ai' +import { MCP_BRIDGE_PING_TOOL, type AiToolOutput } from '@core/ai' import { getErrorMessage } from '@core/utils/errorMessage' import { readNdjsonStream } from './ndjsonStream' import { postToolResult } from './toolResultApi' const MCP_BRIDGE_PATH = '/admin/api/ai/editor-bridge' const RECONNECT_DELAY_MS = 3000 +/** + * Longest one relayed tool (plus its persistence step) may run before the + * loop gives up on it and moves on. Requests are serviced serially, so a tool + * whose promise never settled used to block every later request on the stream + * until the tab reloaded (#490). Kept under the relay's 90 s per-call timeout + * (`server/ai/runtime/transport.ts`) so the caller reads this diagnosis rather + * than a bare timeout, and so the result POST still finds its waiter. + */ +const BROWSER_TOOL_DEADLINE_MS = 60_000 // Auth failures (logged out / brief blip during a server restart) back off // longer but still retry so the bridge self-heals once the session is valid. const AUTH_RETRY_DELAY_MS = 15000 @@ -42,28 +51,55 @@ export type McpToolDispatcher = ( export type McpAfterSuccessfulTool = () => Promise /** - * Run one relayed tool and any workspace-specific persistence step. Keeping - * the persistence callback inside the same try/catch is deliberate: a tool is - * not successful until its mutation is durably saved for the MCP caller's - * next request. + * Run one relayed tool and any workspace-specific persistence step, bounded + * by `deadlineMs`. Keeping the persistence callback inside the same try/catch + * is deliberate: a tool is not successful until its mutation is durably saved + * for the MCP caller's next request. Past the deadline the caller gets an + * error naming the tool and the loop is free again; the tool itself cannot be + * cancelled and may still finish in the background, which the error says. */ export async function executeMcpBridgeRequest( dispatchTool: McpToolDispatcher, toolName: string, input: unknown, afterSuccessfulTool?: McpAfterSuccessfulTool, + deadlineMs: number = BROWSER_TOOL_DEADLINE_MS, ): Promise { + let deadline: ReturnType | undefined + const expired = new Promise((resolve) => { + deadline = setTimeout(() => { + resolve({ + ok: false, + error: + `Browser tool "${toolName}" did not finish within ${Math.round(deadlineMs / 1000)}s. ` + + 'The editor may still complete it in the background, so read the document before ' + + 'retrying; if later calls keep failing, reload the workspace tab.', + }) + }, deadlineMs) + }) + const run = (async (): Promise => { + try { + const result = await dispatchTool(toolName, input) + if (result.ok && afterSuccessfulTool) await afterSuccessfulTool() + return result + } catch (err) { + return { ok: false, error: getErrorMessage(err, 'Tool failed.') } + } + })() try { - const result = await dispatchTool(toolName, input) - if (result.ok && afterSuccessfulTool) await afterSuccessfulTool() - return result - } catch (err) { - return { ok: false, error: getErrorMessage(err, 'Tool failed.') } + return await Promise.race([run, expired]) + } finally { + clearTimeout(deadline) } } type McpBridgeConnectionOutcome = 'auth' | 'transient' +interface McpBridgeConnectionOptions { + /** Test seam: the per-tool deadline shrinks to milliseconds in unit tests. */ + toolDeadlineMs?: number +} + /** * Run one editor-bridge connection. Every attempt owns a fresh controller; * its signal is also linked to the hook lifetime so unmount still tears down @@ -76,6 +112,7 @@ export async function runMcpWorkspaceBridgeConnection( dispatchTool: McpToolDispatcher, afterSuccessfulTool: McpAfterSuccessfulTool | undefined, lifecycleSignal: AbortSignal, + options: McpBridgeConnectionOptions = {}, ): Promise { const connectionController = new AbortController() const signal = AbortSignal.any([lifecycleSignal, connectionController.signal]) @@ -101,11 +138,21 @@ export async function runMcpWorkspaceBridgeConnection( continue } + if (event.toolName === MCP_BRIDGE_PING_TOOL) { + // Liveness probe from get_context. Answered here, by the loop itself: + // a reply proves the stream AND this request loop are servicing calls, + // which is what "connected" has to mean. It never reaches the + // workspace dispatcher. + await postToolResult(bridgeId, event.requestId, { ok: true, data: { alive: true } }, signal) + continue + } + const result = await executeMcpBridgeRequest( dispatchTool, event.toolName, event.input, afterSuccessfulTool, + options.toolDeadlineMs, ) await postToolResult(bridgeId, event.requestId, result, signal) } diff --git a/src/core/ai/bridgePing.ts b/src/core/ai/bridgePing.ts new file mode 100644 index 000000000..dc2670fa5 --- /dev/null +++ b/src/core/ai/bridgePing.ts @@ -0,0 +1,14 @@ +/** + * Liveness probe for a workspace bridge (the MCP relay's stream into an open + * Site editor or Content workspace tab). + * + * `get_context` relays this pseudo-tool through the same stream and the same + * client request loop every browser tool travels; the loop answers it itself + * and never touches the workspace dispatcher. A registered stream whose loop + * is stuck behind a tool that never settled, or whose connection died behind + * a proxy without the server noticing, does not answer — which is the + * difference between "a tab opened a stream at some point" and "browser tools + * will work right now". Not a registered MCP tool: an external client cannot + * call it by name. + */ +export const MCP_BRIDGE_PING_TOOL = 'mcp_bridge_ping' diff --git a/src/core/ai/index.ts b/src/core/ai/index.ts index 636fe323e..c60b47301 100644 --- a/src/core/ai/index.ts +++ b/src/core/ai/index.ts @@ -6,6 +6,7 @@ export { aiToolOk, } from './toolOutput' export type { AiToolImage, AiToolOutput } from './toolOutput' +export { MCP_BRIDGE_PING_TOOL } from './bridgePing' export { AiContentBlockSchema, AiContentViewBlockSchema, diff --git a/tests/e2e/mcp-bridge-liveness.e2e.ts b/tests/e2e/mcp-bridge-liveness.e2e.ts new file mode 100644 index 000000000..cb7966ed4 --- /dev/null +++ b/tests/e2e/mcp-bridge-liveness.e2e.ts @@ -0,0 +1,150 @@ +import { expect, test, type APIRequestContext, type Page } from '@playwright/test' +import { login } from './helpers/auth' +import { ANONYMOUS_STATE, OWNER, PUBLIC_BASE_URL } from './helpers/constants' + +/** + * AI-011 — `get_context` reports what the workspace bridge can do right now. + * + * #490: a hosted deployment reported `siteConnected: true` while every + * relayed browser tool failed. Registration only proved a tab had opened the + * stream. The flag is now a real round-trip through the bridge, so a tab whose + * main thread is wedged reads as `unresponsive`, and it recovers once the tab + * answers again. Runs on a fresh login: minting a personal access token needs + * a step-up, which rotates the session, so the shared owner state must not be + * used here. + */ +test.use({ storageState: ANONYMOUS_STATE }) + +const MCP_URL = `${PUBLIC_BASE_URL}/_instatic/mcp` +let nextId = 1 + +async function mintAccessToken(page: Page): Promise { + // Same-origin browser fetches: the step-up opens the session's window, then + // the token endpoint accepts the create. Both run inside the page so the + // browser's own Origin and cookies apply, exactly as the admin UI does it. + const token = await page.evaluate(async (password) => { + const post = (path: string, body: unknown) => fetch(path, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + credentials: 'same-origin', + body: JSON.stringify(body), + }) + const stepUp = await post('/admin/api/cms/auth/step-up', { password }) + if (!stepUp.ok) throw new Error(`step-up failed: ${stepUp.status}`) + const created = await post('/admin/api/ai/mcp/access-tokens', { + label: 'AI-011 liveness', + capabilities: ['ai.tools.write', 'site.read', 'site.structure.edit'], + }) + if (!created.ok) throw new Error(`token create failed: ${created.status} ${await created.text()}`) + const body = (await created.json()) as { accessToken: string } + return body.accessToken + }, OWNER.password) + expect(token).toMatch(/^imcp_pat_/) + return token +} + +interface ToolCall { + ms: number + isError: boolean + text: string +} + +async function callTool( + request: APIRequestContext, + token: string, + name: string, + args: Record, +): Promise { + const params = { + name, + arguments: args, + _meta: { + 'io.modelcontextprotocol/protocolVersion': '2026-07-28', + 'io.modelcontextprotocol/clientInfo': { name: 'e2e-ai-011', version: '0' }, + 'io.modelcontextprotocol/clientCapabilities': {}, + }, + } + const started = Date.now() + const res = await request.post(MCP_URL, { + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', + Authorization: `Bearer ${token}`, + 'MCP-Protocol-Version': '2026-07-28', + 'Mcp-Method': 'tools/call', + 'Mcp-Name': name, + }, + data: { jsonrpc: '2.0', id: nextId++, method: 'tools/call', params }, + }) + expect(res.status()).toBe(200) + const json = (await res.json()) as { + result?: { isError?: boolean; content?: Array<{ type: string; text?: string }> } + } + return { + ms: Date.now() - started, + isError: json.result?.isError ?? false, + text: json.result?.content?.map((c) => c.text ?? '').join('') ?? '', + } +} + +function editorState(call: ToolCall): { siteConnected: boolean; contentConnected: boolean; unresponsive: string[] } { + return (JSON.parse(call.text) as { editor: { siteConnected: boolean; contentConnected: boolean; unresponsive: string[] } }).editor +} + +test('get_context reflects whether the open Site editor actually answers (AI-011)', async ({ page, request }) => { + test.setTimeout(90_000) + await login(page) + const token = await mintAccessToken(page) + + const bridgeOpened = page.waitForResponse((r) => new URL(r.url()).pathname === '/admin/api/ai/editor-bridge') + await page.goto('/admin/site') + expect((await bridgeOpened).status()).toBe(200) + + // A live editor answers the probe. + await expect.poll(async () => editorState(await callTool(request, token, 'get_context', {})).siteConnected, { + timeout: 15_000, + }).toBe(true) + expect(editorState(await callTool(request, token, 'get_context', {}))).toEqual({ + siteConnected: true, + contentConnected: false, + unresponsive: [], + }) + + // The reporter's step: an element the importer has no module for is + // rejected, and the bridge keeps serving afterwards. + const docs = await callTool(request, token, 'site_list_documents', {}) + const rootNodeId = /"rootNodeId":"([^"]+)"/.exec(docs.text)?.[1] + expect(rootNodeId).toBeTruthy() + const insert = await callTool(request, token, 'site_insert_html', { + parentId: rootNodeId, + index: 0, + html: '', + }) + expect(insert.isError).toBe(true) + expect(insert.text).toContain('no importable elements') + expect((await callTool(request, token, 'site_get_node_html', { nodeId: rootNodeId })).isError).toBe(false) + + // Wedge the tab: a busy main thread cannot read the stream, so the probe + // must time out and the workspace must read as unresponsive — the state that + // used to report `siteConnected: true`. + await page.evaluate(() => { + setTimeout(() => { + const until = Date.now() + 7_000 + while (Date.now() < until) { + /* hold the main thread */ + } + }, 0) + }) + const wedged = await callTool(request, token, 'get_context', {}) + expect(editorState(wedged)).toEqual({ + siteConnected: false, + contentConnected: false, + unresponsive: ['site'], + }) + expect(wedged.ms).toBeGreaterThanOrEqual(2_000) + + // Once the tab answers again the bridge reconnects and the flag recovers. + await expect.poll(async () => editorState(await callTool(request, token, 'get_context', {})), { + timeout: 30_000, + }).toEqual({ siteConnected: true, contentConnected: false, unresponsive: [] }) +})