diff --git a/.changeset/prevent-opening-mutation-repeats.md b/.changeset/prevent-opening-mutation-repeats.md new file mode 100644 index 000000000..98f87d520 --- /dev/null +++ b/.changeset/prevent-opening-mutation-repeats.md @@ -0,0 +1,5 @@ +--- +"agent-bundle": patch +--- + +Prevent `agent-bundle dev` `/web` refreshes from silently repeating non-read-only opening tools after cache eviction or failure (#646) diff --git a/packages/agent-bundle/src/dev/web-host-routes.ts b/packages/agent-bundle/src/dev/web-host-routes.ts index 7248a192c..c53ab61eb 100644 --- a/packages/agent-bundle/src/dev/web-host-routes.ts +++ b/packages/agent-bundle/src/dev/web-host-routes.ts @@ -49,8 +49,18 @@ import { const manifestFileName = 'agent-bundle.manifest.json'; const maxRetainedOpeningCalls = 64; +const maxRetainedOpeningExecutions = 256; const maxRetainedOpeningResults = 64; +interface WebOpeningCall extends McpAppOpeningCall { + readonly notice?: string; +} + +type OpeningExecution = + | Readonly<{ readonly call: Promise; readonly outcome: 'in-flight' }> + | Readonly<{ readonly outcome: 'succeeded' }> + | Readonly<{ readonly outcome: 'failed-unknown' }>; + interface WebHostEpochReference { close(): Promise; readonly epoch: Readonly<{ readonly id: string }>; @@ -175,7 +185,8 @@ export class WebHostRoutes { readonly #sandboxOrigin: () => string | undefined; readonly #sessionToken: string; readonly #openingCalls = new Map(); - readonly #openingResults = new Map>(); + readonly #openingExecutions = new Map(); + readonly #openingResults = new Map(); readonly #sessions = new Map>(); #closed = false; @@ -193,6 +204,7 @@ export class WebHostRoutes { if (this.#closed) return; this.#closed = true; this.#openingCalls.clear(); + this.#openingExecutions.clear(); this.#openingResults.clear(); const sessions = [...this.#sessions.values()]; this.#sessions.clear(); @@ -298,6 +310,7 @@ export class WebHostRoutes { autoApprove: app.allow, input: call.input, opening, + ...(call.notice === undefined ? {} : { openingNotice: call.notice }), previewProfile: 'portable', result: call.result, sessionId: registered.session.id, @@ -323,32 +336,50 @@ export class WebHostRoutes { * The call that opens the page. A tool annotated `readOnlyHint: true` runs * once per page load — a refresh re-reads live state. Any other opening * tool may mutate, so a page open is not an unbounded mutation: its first - * call per session, tool, App, and input is retained while still in - * flight (concurrent first loads share it) and every later load of the - * same page rebinds its result instead of re-running the tool; a failed - * call is dropped so the next load retries, and a new session (a new - * epoch after rebuild) runs the tool once again. + * call per session, tool, App, and input owns a bounded execution record. + * Concurrent first loads share an in-flight call; later loads rebind a + * retained result, or receive a fail-closed result when that result was + * evicted or the call failed. A new session (a new epoch after rebuild) + * may run the tool once again. */ async #openingCallFor( source: AppSelectionSource, sessionId: string, resolved: ResolvedAppOpening, - ): Promise { - const call = async (): Promise => Object.freeze({ + ): Promise { + const call = async (): Promise => Object.freeze({ input: resolved.input, result: await source.callTool(resolved.tool.name, resolved.input), }); const readOnly = isRecord(resolved.tool['annotations']) && resolved.tool['annotations']['readOnlyHint'] === true; if (readOnly) return call(); const key = `${sessionId}\0${resolved.tool.name}\0${resolved.resourceUri}\0${digest(resolved.input)}`; - const retained = this.#openingResults.get(key); - if (retained !== undefined) return retained; + const execution = this.#openingExecutions.get(key); + if (execution?.outcome === 'in-flight') return execution.call; + if (execution?.outcome === 'succeeded') { + return this.#openingResults.get(key) ?? + this.#unavailableOpeningCall(resolved.input, 'Opening tool result no longer retained; re-run explicitly from the App.'); + } + if (execution?.outcome === 'failed-unknown') { + return this.#unavailableOpeningCall(resolved.input, 'Opening tool outcome is unknown; re-run explicitly from the App.'); + } + if (this.#openingExecutions.size >= maxRetainedOpeningExecutions) { + return this.#unavailableOpeningCall(resolved.input, 'Automatic opening limit reached; run the tool explicitly from the App.'); + } const pending = call(); - this.#retainOpeningResult(key, pending); + const inFlight = Object.freeze({ call: pending, outcome: 'in-flight' }) satisfies OpeningExecution; + this.#openingExecutions.set(key, inFlight); try { - return await pending; + const result = await pending; + if (this.#openingExecutions.get(key) === inFlight) { + this.#openingExecutions.set(key, Object.freeze({ outcome: 'succeeded' })); + this.#retainOpeningResult(key, result); + } + return result; } catch (error) { - if (this.#openingResults.get(key) === pending) this.#openingResults.delete(key); + if (this.#openingExecutions.get(key) === inFlight) { + this.#openingExecutions.set(key, Object.freeze({ outcome: 'failed-unknown' })); + } throw error; } } @@ -425,17 +456,14 @@ export class WebHostRoutes { const session = await service.open({ epochId, serverName, target }); const lease: McpAppSessionLease = await service.acquireAppLease(session.id); let disposed = false; - let unsubscribe = (): void => undefined; const dispose = async (): Promise => { if (disposed) return; disposed = true; - unsubscribe(); await lease.release(); }; const watched = lease.watchSessionClosed(() => { this.#forgetSession(key, session.id); }); - unsubscribe = watched.unsubscribe; if (watched.closed) { await dispose(); throw new Error('MCP App session closed while it was being registered.'); @@ -452,8 +480,11 @@ export class WebHostRoutes { this.#dropSessionState(sessionId); const current = this.#sessions.get(key); if (current === undefined) return; - this.#sessions.delete(key); - void current.then((registered) => registered.dispose()).catch(() => undefined); + void current.then(async (registered) => { + if (registered.session.id !== sessionId || this.#sessions.get(key) !== current) return; + this.#sessions.delete(key); + await registered.dispose(); + }).catch(() => undefined); } #dropSessionState(sessionId: string): void { @@ -461,6 +492,9 @@ export class WebHostRoutes { for (const openingKey of this.#openingCalls.keys()) { if (openingKey.startsWith(prefix)) this.#openingCalls.delete(openingKey); } + for (const executionKey of this.#openingExecutions.keys()) { + if (executionKey.startsWith(prefix)) this.#openingExecutions.delete(executionKey); + } for (const resultKey of this.#openingResults.keys()) { if (resultKey.startsWith(prefix)) this.#openingResults.delete(resultKey); } @@ -474,7 +508,7 @@ export class WebHostRoutes { } } - #retainOpeningResult(key: string, call: Promise): void { + #retainOpeningResult(key: string, call: WebOpeningCall): void { this.#openingResults.set(key, call); for (const oldest of this.#openingResults.keys()) { if (this.#openingResults.size <= maxRetainedOpeningResults) break; @@ -482,6 +516,20 @@ export class WebHostRoutes { } } + #unavailableOpeningCall( + input: Readonly>, + message: string, + ): WebOpeningCall { + return Object.freeze({ + input, + notice: message, + result: Object.freeze({ + content: Object.freeze([Object.freeze({ text: message, type: 'text' })]), + isError: true, + }), + }); + } + #openingCallKey(sessionId: string, toolName: string, opening: string): string { return `${sessionId}\0${toolName}\0${opening}`; } diff --git a/packages/agent-bundle/src/web-host/browser/main.ts b/packages/agent-bundle/src/web-host/browser/main.ts index 26a9bc495..bfbcd5d01 100644 --- a/packages/agent-bundle/src/web-host/browser/main.ts +++ b/packages/agent-bundle/src/web-host/browser/main.ts @@ -106,7 +106,10 @@ const revisionOf = (frame: McpAppRelayFrame | undefined): number => frame?.documentPolicy?.revision ?? 0; const start = async (): Promise => { - setStatus(`Binding ${seed.toolName} to the App…`); + setStatus( + seed.openingNotice ?? `Binding ${seed.toolName} to the App…`, + seed.openingNotice === undefined ? 'info' : 'warn', + ); const created = await api>( 'POST', `/api/mcp/sessions/${encodeURIComponent(seed.sessionId)}/apps`, @@ -275,7 +278,10 @@ const start = async (): Promise => { event.data.method !== 'ui/notifications/sandbox-proxy-ready' || Object.hasOwn(event.data, 'id') ) return; - setStatus(`Serving ${seed.title} over the bound session.`, 'ok'); + setStatus( + seed.openingNotice ?? `Serving ${seed.title} over the bound session.`, + seed.openingNotice === undefined ? 'ok' : 'warn', + ); }); frameHost.replaceChildren(iframe); diff --git a/packages/agent-bundle/src/web-host/browser/seed.ts b/packages/agent-bundle/src/web-host/browser/seed.ts index e90906c08..c516cc3d5 100644 --- a/packages/agent-bundle/src/web-host/browser/seed.ts +++ b/packages/agent-bundle/src/web-host/browser/seed.ts @@ -11,6 +11,8 @@ export interface WebHostPageSeed { readonly input: McpAppJsonValue; /** Opaque per-page id of the opening call, set by hosts that serve many pages over one session (dev `/web`). */ readonly opening?: string; + /** Fail-closed reason an automatic mutating opening was not repeated. */ + readonly openingNotice?: string; readonly previewProfile: McpAppProfileId; readonly result: McpAppJsonValue; readonly sessionId: string; diff --git a/packages/agent-bundle/tests/web-host-routes-unit.test.ts b/packages/agent-bundle/tests/web-host-routes-unit.test.ts index 5daa7aa4f..422a0dc35 100644 --- a/packages/agent-bundle/tests/web-host-routes-unit.test.ts +++ b/packages/agent-bundle/tests/web-host-routes-unit.test.ts @@ -53,6 +53,7 @@ const codexServer = (): Readonly> => ({ }); interface FixtureOptions { + readonly openingInput?: Readonly>; readonly projections: Readonly>>>; readonly targets: readonly string[]; } @@ -67,6 +68,7 @@ const writeFixture = async (root: string, options: FixtureOptions): Promise void)[] = []; gateToolCalls = false; readonly #leases = new Map(); + readonly #closeWatchers = new Map void>>(); readonly #retire = new Set(); async open(options: { readonly epochId: string; readonly serverName: string; readonly target: string }): Promise { const id = `session-${String(this.opened.length + 1)}`; this.opened.push({ epochId: options.epochId, id, serverName: options.serverName, target: options.target }); this.#leases.set(id, 0); + this.#closeWatchers.set(id, new Set()); const session = { callTool: async () => { this.toolCalls += 1; @@ -139,7 +143,15 @@ class FakeSessionService { if (remaining === 0 && this.#retire.delete(sessionId)) void this.closeSession(sessionId); }, session: {}, - watchSessionClosed: () => ({ closed: false, unsubscribe: () => undefined }), + watchSessionClosed: (listener: () => void) => { + const watchers = this.#closeWatchers.get(sessionId); + if (watchers === undefined) return { closed: true, unsubscribe: () => undefined }; + watchers.add(listener); + return { + closed: false, + unsubscribe: () => { watchers.delete(listener); }, + }; + }, }; } @@ -161,6 +173,9 @@ class FakeSessionService { this.closed.push(sessionId); this.#leases.delete(sessionId); this.#retire.delete(sessionId); + const watchers = this.#closeWatchers.get(sessionId) ?? []; + this.#closeWatchers.delete(sessionId); + for (const watcher of watchers) watcher(); return true; } @@ -368,6 +383,30 @@ describe('WebHostRoutes session retirement', () => { expect(harness.service.opened).toHaveLength(1); expect(harness.service.closed).toEqual([]); }); + + it('does not let a retired session close remove a replacement under the same epoch key', async () => { + const root = await artifactRoot(); + await writeFixture(root, { projections: { '.mcp.json': claudeServer() }, targets: ['claude'] }); + const harness = await startHarness(root, 'epoch-1'); + expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(200); + const firstSessionId = harness.service.opened[0]!.id; + const pageLease = await harness.service.leaseAsPage(firstSessionId); + + harness.setEpoch('epoch-2'); + harness.routes.adoptActiveEpoch('epoch-2'); + harness.setEpoch('epoch-1'); + expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(200); + const replacementSessionId = harness.service.opened[1]!.id; + + await pageLease.release(); + await settle(); + expect(harness.service.closed).toEqual([firstSessionId]); + + harness.setEpoch('epoch-2'); + harness.routes.adoptActiveEpoch('epoch-2'); + await settle(); + expect(harness.service.closed).toEqual([firstSessionId, replacementSessionId]); + }); }); describe('WebHostRoutes opening-tool policy', () => { @@ -415,13 +454,107 @@ describe('WebHostRoutes opening-tool policy', () => { expect(harness.service.toolCalls).toBe(1); }); - it('drops a failed mutating opening call so the next load retries', async () => { + it('does not repeat a mutation after its retained result is evicted', async () => { + const root = await artifactRoot(); + const options = { + projections: { '.mcp.json': claudeServer() }, + targets: ['claude'], + } as const; + await writeFixture(root, { ...options, openingInput: { index: 0 } }); + const harness = await startHarness(root); + expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(200); + for (let index = 1; index <= 64; index += 1) { + await writeFixture(root, { ...options, openingInput: { index } }); + expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(200); + } + await writeFixture(root, { ...options, openingInput: { index: 0 } }); + const revisited = await fetch(`${harness.url}/web/status/status`); + expect(revisited.status).toBe(200); + expect(await revisited.text()).toContain( + '"openingNotice":"Opening tool result no longer retained; re-run explicitly from the App."', + ); + expect(harness.service.toolCalls).toBe(65); + }); + + it('keeps an in-flight mutation authoritative while completed results are evicted', async () => { + const root = await artifactRoot(); + const options = { + projections: { '.mcp.json': claudeServer() }, + targets: ['claude'], + } as const; + await writeFixture(root, { ...options, openingInput: { index: 0 } }); + const harness = await startHarness(root); + harness.service.gateToolCalls = true; + const first = fetch(`${harness.url}/web/status/status`); + for (let turn = 0; turn < 200 && harness.service.toolCallReleases.length === 0; turn += 1) { + await new Promise((done) => setTimeout(done, 5)); + } + harness.service.gateToolCalls = false; + for (let index = 1; index <= 64; index += 1) { + await writeFixture(root, { ...options, openingInput: { index } }); + expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(200); + } + await writeFixture(root, { ...options, openingInput: { index: 0 } }); + const revisited = fetch(`${harness.url}/web/status/status`); + const revisitState = await Promise.race([ + revisited.then(() => 'settled' as const), + new Promise<'waiting'>((resolve) => setTimeout(() => resolve('waiting'), 50)), + ]); + expect(revisitState).toBe('waiting'); + expect(harness.service.toolCalls).toBe(65); + harness.service.toolCallReleases.splice(0).forEach((release) => release()); + expect((await Promise.all([first, revisited])).map((response) => response.status)).toEqual([200, 200]); + }); + + it('does not retry a mutating opening call after its outcome becomes unknown', async () => { const root = await artifactRoot(); await writeFixture(root, { projections: { '.mcp.json': claudeServer() }, targets: ['claude'] }); const harness = await startHarness(root); harness.service.failNextToolCall = true; expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(502); - expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(200); - expect(harness.service.toolCalls).toBe(2); + const revisited = await fetch(`${harness.url}/web/status/status`); + expect(revisited.status).toBe(200); + expect(await revisited.text()).toContain( + '"openingNotice":"Opening tool outcome is unknown; re-run explicitly from the App."', + ); + expect(harness.service.toolCalls).toBe(1); + }); + + it('releases mutation records after a leased retired session actually closes', async () => { + const root = await artifactRoot(); + await writeFixture(root, { projections: { '.mcp.json': claudeServer() }, targets: ['claude'] }); + const harness = await startHarness(root, 'epoch-1'); + for (let index = 1; index <= 257; index += 1) { + expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(200); + const sessionId = harness.service.opened.at(-1)!.id; + const pageLease = await harness.service.leaseAsPage(sessionId); + harness.setEpoch(`epoch-${String(index + 1)}`); + harness.routes.adoptActiveEpoch(`epoch-${String(index + 1)}`); + await settle(); + await pageLease.release(); + await settle(); + } + const final = await (await fetch(`${harness.url}/web/status/status`)).text(); + expect(final).not.toContain('"openingNotice":'); + expect(harness.service.toolCalls).toBe(258); + }); + + it('fails closed when the bounded mutation execution ledger is full', async () => { + const root = await artifactRoot(); + const options = { + projections: { '.mcp.json': claudeServer() }, + targets: ['claude'], + } as const; + const harness = await startHarness(root); + for (let index = 0; index < 256; index += 1) { + await writeFixture(root, { ...options, openingInput: { index } }); + expect((await fetch(`${harness.url}/web/status/status`)).status).toBe(200); + } + await writeFixture(root, { ...options, openingInput: { index: 256 } }); + const saturated = await (await fetch(`${harness.url}/web/status/status`)).text(); + expect(saturated).toContain( + '"openingNotice":"Automatic opening limit reached; run the tool explicitly from the App."', + ); + expect(harness.service.toolCalls).toBe(256); }); }); diff --git a/website/docs/en/guide/authoring/mcp.mdx b/website/docs/en/guide/authoring/mcp.mdx index 704c86133..d6bd72d61 100644 --- a/website/docs/en/guide/authoring/mcp.mdx +++ b/website/docs/en/guide/authoring/mcp.mdx @@ -1067,7 +1067,12 @@ resolved launch identity; a successful rebuild retires unused sessions of older still leasing one keep it until their last lease releases), and a failed rebuild retires nothing. An opening tool annotated `readOnlyHint: true` runs on every page load; any other opening tool runs once per session, -tool, App, and input, and a refresh rebinds that retained result. +tool, App, and input. Concurrent first loads share the same in-flight call, and refreshes rebind +its retained result without gaining authority to repeat the operation. Results are bounded and +may be evicted independently of the execution record; after eviction, after a failed call whose +side effects are unknown, or when the bounded execution ledger is full, the page receives an +error result directing the user to run the tool explicitly from the App instead of retrying it +automatically. `agent-bundle serve-app` is unchanged: it is the checkout-time form that builds or points at an artifact and needs the framework installed. Every option is in the diff --git a/website/docs/en/guide/development/workbench.mdx b/website/docs/en/guide/development/workbench.mdx index c67592a30..5c0ee23a0 100644 --- a/website/docs/en/guide/development/workbench.mdx +++ b/website/docs/en/guide/development/workbench.mdx @@ -105,7 +105,10 @@ and consent behavior, bound to the current epoch's server session. Apps not list are 404. The launch resolves from the artifact's declared projections (`?target=` is validated, and materially different launches answer 409 naming the choices), web sessions are keyed by epoch, server, and launch identity and retire only when a rebuild publishes a new epoch, and a -non-read-only opening tool runs once per session, tool, App, and input. See +non-read-only opening tool runs once per session, tool, App, and input. Its bounded execution +record remains authoritative if the retained result is evicted or the call fails, so refresh +never silently repeats a possible mutation; the page instead directs the user to run the tool +explicitly from the App. See [Exposing an App in the browser](../authoring/mcp.mdx#exposing-an-app-in-the-browser). A Skill leaf renders the emitted Skill document by default. Source/generated differences, diff --git a/website/docs/zh/guide/authoring/mcp.mdx b/website/docs/zh/guide/authoring/mcp.mdx index 6c6a4ffb1..b1a712d0f 100644 --- a/website/docs/zh/guide/authoring/mcp.mdx +++ b/website/docs/zh/guide/authoring/mcp.mdx @@ -921,8 +921,10 @@ args、cwd、声明的 env、运行时绑定）的 projection 无需询问即可 并列出可选项——只有 Claude 或 Codex projection 的构建不需要 portable projection 或 `mcp.json` 就能 打开它的 App。Web 会话按 epoch、服务器与解析后的启动身份缓存；一次成功的重建会退役旧 epoch 中未被 使用的会话（仍被页面租用的保持有效，直到最后一个租约释放时关闭），失败的重建则不退役任何会话。标注了 `readOnlyHint: true` 的 -开场工具在每次页面加载时运行；其他开场工具在每个会话、工具、App 与输入组合下只运行一次，刷新会重新 -绑定保留的结果。 +开场工具在每次页面加载时运行；其他开场工具在每个会话、工具、App 与输入组合下只运行一次。并发的首次加载 +共享同一个进行中的调用，刷新只重新绑定保留的结果，并不会重新获得执行操作的权限。结果按独立上限保留； +结果被淘汰、调用失败而副作用未知，或有界执行记录已满时，页面会收到一份错误结果，要求用户从 App +显式运行该工具，而不会自动重试。 `agent-bundle serve-app` 保持不变：它是 checkout 时的形态，会构建或指向一份产物，并需要已安装的 框架。全部选项见[命令行参考](../../reference/cli.mdx#serve-app)。 diff --git a/website/docs/zh/guide/development/workbench.mdx b/website/docs/zh/guide/development/workbench.mdx index c1a062848..9dcbd84b3 100644 --- a/website/docs/zh/guide/development/workbench.mdx +++ b/website/docs/zh/guide/development/workbench.mdx @@ -88,7 +88,9 @@ App 叶子把沙箱化的 MCP App 预览放在工作区中央，并绑定到它 `GET /web//`——同样的页面、中继、路由、沙箱代理与同意行为，绑定到当前 epoch 的服务器会话。 未列在 `web.apps` 中的 App 返回 404。启动方式从产物声明的 projection 解析（`?target=` 会被校验， 实质不同的启动方式以 409 应答并列出可选项）；Web 会话按 epoch、服务器与启动身份缓存，只在重建发布 -新 epoch 时退役；非只读的开场工具在每个会话、工具、App 与输入组合下只运行一次。见 +新 epoch 时退役；非只读的开场工具在每个会话、工具、App 与输入组合下只运行一次。即使保留的结果被淘汰 +或调用失败，其有界执行记录仍然有效，因此刷新不会静默重复可能的变更；页面会改为要求用户从 App +显式运行该工具。见 [在浏览器中暴露 App](../authoring/mcp.mdx#在浏览器中暴露-app)。 Skill 叶子默认渲染输出的 Skill 文档。源码/生成差异、frontmatter、资源与 eval 覆盖位于检查器中。Skill