From c166da0a249883c1f9ef32e339c3ac2c12fe7a6b Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Thu, 29 Oct 2026 11:20:36 +0000 Subject: [PATCH 1/4] fix: wait for animations that start off screen or at zero size The browser settle ignored animations whose target was outside the viewport or had an empty box. A drawer sliding in from the edge, a bottom sheet sliding up, or an accordion opening from height 0 starts in that state, so the settle ended on the first frame while the element was still moving (checked in Chromium: drawer, modal and accordion settled at 0 ms; a fade-in waited 408 ms). animationsSettled now waits for every running, finite animation of an element. Infinite animations (spinners, the fox, skeleton shimmer) are still ignored, and the 1 s cap is unchanged. Review: https://github.com/MetaMask/client-mcp-core/pull/65#discussion_r4410641752 --- src/server/create-server.ts | 2 +- src/tools/utils/constants.ts | 7 ++++-- src/tools/utils/discovery.ts | 2 +- src/tools/utils/in-page.test.ts | 38 ++++++++++++--------------------- src/tools/utils/in-page.ts | 37 ++++++++++++-------------------- 5 files changed, 35 insertions(+), 51 deletions(-) diff --git a/src/server/create-server.ts b/src/server/create-server.ts index d9dd8cd..84ef8e8 100644 --- a/src/server/create-server.ts +++ b/src/server/create-server.ts @@ -471,7 +471,7 @@ export function createServer(config: ServerConfig): ServerInstance { if (category !== 'readonly' && category !== 'discovery') { await page.waitForLoadState('domcontentloaded').catch(() => undefined); signal.throwIfAborted(); - // Waits only for running, finite, in-viewport animations (infinite + // Waits only for running, finite animations of elements (infinite // spinners are ignored), checked once per frame, capped at 1 s. await waitForAnimationsToSettle( page, diff --git a/src/tools/utils/constants.ts b/src/tools/utils/constants.ts index 2d6b087..0313c10 100644 --- a/src/tools/utils/constants.ts +++ b/src/tools/utils/constants.ts @@ -18,8 +18,11 @@ export const MAX_QUEUE_TIMEOUT_MS = 600_000; /** * Cap on the wait for animations to settle after a mutating browser call. - * Only running, finite animations of elements in the viewport are waited - * for; infinite ones (spinners, the fox, skeleton shimmer) are ignored. + * Only running, finite animations of elements are waited for (also off + * screen or at zero size, e.g. a drawer sliding in); infinite ones (spinners, + * the fox, skeleton shimmer) are ignored. A long finite animation anywhere on + * the page (a slow off-screen carousel, a Web Animations API animation on a + * hidden element) can add up to this cap to a mutating call. */ export const ANIMATION_SETTLE_TIMEOUT_MS = 1_000; diff --git a/src/tools/utils/discovery.ts b/src/tools/utils/discovery.ts index 2ac5b33..e205cc8 100644 --- a/src/tools/utils/discovery.ts +++ b/src/tools/utils/discovery.ts @@ -274,7 +274,7 @@ async function collectTestIdsViaLocators( } /** - * Waits until the page's running, finite, in-viewport animations end (see + * Waits until the page's running, finite element animations end (see * `animationsSettled`), checking once per animation frame, for at most * `timeoutMs`. Never throws: a timeout or an unreadable page ends the wait. * diff --git a/src/tools/utils/in-page.test.ts b/src/tools/utils/in-page.test.ts index 985b2f2..d92e34e 100644 --- a/src/tools/utils/in-page.test.ts +++ b/src/tools/utils/in-page.test.ts @@ -514,14 +514,14 @@ function targetElement(rect: { } /** - * Runs animationsSettled against fake animations in a 800x600 viewport. + * Runs animationsSettled against fake animations. Target rects are relative + * to an 800x600 viewport, but animationsSettled does not read them. * * @param animations - The page's animations. * @returns The predicate result. */ function settled(animations: FakeAnimation[]): boolean { const fn = compileInPage(animationsSettled, { - window: { innerWidth: 800, innerHeight: 600 }, document: { getAnimations: () => animations }, }); return fn(); @@ -565,30 +565,20 @@ describe('animationsSettled', () => { ).toBe(true); }); - it('ignores targets outside the viewport', () => { - expect( - settled([ - animation({ target: targetElement({ x: -10, y: 0 }) }), - animation({ target: targetElement({ x: 0, y: -10 }) }), - animation({ target: targetElement({ x: 800, y: 0 }) }), - animation({ target: targetElement({ x: 0, y: 600 }) }), - ]), - ).toBe(true); - }); - - it('ignores targets with an empty box', () => { - expect( - settled([ - animation({ target: targetElement({ x: 5, y: 5, width: 0 }) }), - animation({ target: targetElement({ x: 5, y: 5, height: 0 }) }), - ]), - ).toBe(true); + it.each([ + ['right of the viewport (drawer sliding in)', { x: 800, y: 0 }], + ['below the viewport (bottom sheet sliding up)', { x: 0, y: 600 }], + ['left of the viewport', { x: -10, y: 0 }], + ['above the viewport', { x: 0, y: -10 }], + ])('counts a target that starts %s', (_label, rect) => { + expect(settled([animation({ target: targetElement(rect) })])).toBe(false); }); - it('counts targets that only partly intersect the viewport', () => { - expect( - settled([animation({ target: targetElement({ x: -5, y: 595 }) })]), - ).toBe(false); + it.each([ + ['zero height (accordion opening)', { x: 5, y: 5, height: 0 }], + ['zero width', { x: 5, y: 5, width: 0 }], + ])('counts a target with %s', (_label, rect) => { + expect(settled([animation({ target: targetElement(rect) })])).toBe(false); }); it('is not settled when one animation still counts', () => { diff --git a/src/tools/utils/in-page.ts b/src/tools/utils/in-page.ts index 8631e73..1137391 100644 --- a/src/tools/utils/in-page.ts +++ b/src/tools/utils/in-page.ts @@ -25,23 +25,23 @@ export type InPageTestIdItem = { /** * Reports whether the page's animations have settled. * - * Only animations that can end soon and that the user can see are counted: - * an animation keeps the page unsettled when it is running, it is finite - * (its computed `iterations` and `endTime` are not `Infinity`), and its - * effect targets an element whose bounding box has a non-zero size and - * intersects the viewport. Animations without an effect or without an - * element target are ignored. Infinite animations (spinners, the fox, - * skeleton shimmer) never settle, so they are ignored too. A finite - * animation that is still in a long `delay` phase reports `running`, so it - * counts as running until it ends or the wait reaches its cap. + * Only animations that will end are counted: an animation keeps the page + * unsettled when it is running, it is finite (its computed `iterations` and + * `endTime` are not `Infinity`), and its effect targets an element. + * Animations without an effect or without an element target are ignored. + * Infinite animations (spinners, the fox, skeleton shimmer) never settle, so + * they are ignored too. The target's position and size are not checked: an + * element that slides in from outside the viewport (drawer, bottom sheet) or + * grows from zero size (accordion) is still moving even while it is not + * visible yet. A finite animation that is still in a long `delay` phase + * reports `running`, so it counts as running until it ends or the wait + * reaches its cap. * * Runs in the page: must stay self-contained. * - * @returns True when no running, finite, in-viewport animation remains. + * @returns True when no running, finite animation of an element remains. */ export function animationsSettled(): boolean { - const viewportWidth = window.innerWidth; - const viewportHeight = window.innerHeight; for (const animation of document.getAnimations()) { if (animation.playState !== 'running') { continue; @@ -55,17 +55,8 @@ export function animationsSettled(): boolean { continue; } const target = (effect as Partial).target ?? null; - if (!target || typeof target.getBoundingClientRect !== 'function') { - continue; - } - const rect = target.getBoundingClientRect(); - const hasSize = rect.width > 0 && rect.height > 0; - const inViewport = - rect.right > 0 && - rect.bottom > 0 && - rect.left < viewportWidth && - rect.top < viewportHeight; - if (hasSize && inViewport) { + // An element target (it has a layout box API). Other targets are ignored. + if (target && typeof target.getBoundingClientRect === 'function') { return false; } } From 02183a2dcd9805c3b0ae5f0ad4994d26bac4e312 Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Thu, 29 Oct 2026 11:20:36 +0000 Subject: [PATCH 2/4] fix: do not click or type after the request timed out clickTool and typeTool checked the request signal only before the target wait. When the queue timed out during the wait, the agent got "timed out" but the click or type still ran once the target appeared (most likely in run_steps, where the whole batch shares one 30 s queue timeout). IPlatformDriver.click and type take an optional signal. Both drivers call signal.throwIfAborted() after the target wait, right before the action (mobile type also checks between the focus tap and typeText). The tools pass context.signal and return MM_REQUEST_ABORTED when the signal is aborted. An action that already started is not cancelled. @metamask/device-mcp takes no signal, so its waitForElement, tapElement and typeText run until they end; the driver checks between them. Review: https://github.com/MetaMask/client-mcp-core/pull/64#discussion_r4410499219 --- src/platform/mobile-platform-driver.test.ts | 95 +++++++++++++++++++++ src/platform/mobile-platform-driver.ts | 12 +++ src/platform/playwright-driver.test.ts | 70 +++++++++++++++ src/platform/playwright-driver.ts | 9 ++ src/platform/types.ts | 12 +++ src/tools/driver-delegation.test.ts | 14 +++ src/tools/interaction.test.ts | 39 +++++++++ src/tools/interaction.ts | 10 +++ 8 files changed, 261 insertions(+) diff --git a/src/platform/mobile-platform-driver.test.ts b/src/platform/mobile-platform-driver.test.ts index 7219e44..84ef23b 100644 --- a/src/platform/mobile-platform-driver.test.ts +++ b/src/platform/mobile-platform-driver.test.ts @@ -692,6 +692,101 @@ describe('MobilePlatformDriver', () => { }); }); + describe('abort during the target wait', () => { + it('does not tap when the signal aborts while waiting', async () => { + const controller = new AbortController(); + const reason = new Error('Tool execution timed out after 30000ms'); + const backend = createMockBackend({ + waitForElement: vi.fn().mockImplementation(async () => { + controller.abort(reason); + return makeElement(); + }), + }); + const driver = new MobilePlatformDriver(backend); + + await expect( + driver.click( + 'testId', + 'confirm', + new Map(), + 5000, + undefined, + controller.signal, + ), + ).rejects.toBe(reason); + expect(backend.tapElement).not.toHaveBeenCalled(); + }); + + it('does not tap or type when the signal aborts while waiting', async () => { + const controller = new AbortController(); + const backend = createMockBackend({ + waitForElement: vi.fn().mockImplementation(async () => { + controller.abort(new Error('timed out')); + return makeElement(); + }), + }); + const driver = new MobilePlatformDriver(backend); + + await expect( + driver.type( + 'testId', + 'input', + 'text', + new Map(), + 5000, + undefined, + controller.signal, + ), + ).rejects.toThrowError('timed out'); + expect(backend.tapElement).not.toHaveBeenCalled(); + expect(backend.typeText).not.toHaveBeenCalled(); + }); + + it('taps and types when the signal is not aborted', async () => { + const backend = createMockBackend(); + const driver = new MobilePlatformDriver(backend); + const { signal } = new AbortController(); + + await driver.click('testId', 'btn', new Map(), 5000, undefined, signal); + await driver.type( + 'testId', + 'input', + 'text', + new Map(), + 5000, + undefined, + signal, + ); + + expect(backend.tapElement).toHaveBeenCalledTimes(2); + expect(backend.typeText).toHaveBeenCalledWith('text'); + }); + + it('does not type when the signal aborts during the focus tap', async () => { + const controller = new AbortController(); + const backend = createMockBackend({ + tapElement: vi.fn().mockImplementation(async () => { + controller.abort(new Error('timed out')); + return { success: true, x: 1, y: 1, targetDescription: 'input' }; + }), + }); + const driver = new MobilePlatformDriver(backend); + + await expect( + driver.type( + 'testId', + 'input', + 'text', + new Map(), + 5000, + undefined, + controller.signal, + ), + ).rejects.toThrowError('timed out'); + expect(backend.typeText).not.toHaveBeenCalled(); + }); + }); + describe('type', () => { it('waits, taps to focus, then types', async () => { const backend = createMockBackend(); diff --git a/src/platform/mobile-platform-driver.ts b/src/platform/mobile-platform-driver.ts index 0dfc37a..e1180cf 100644 --- a/src/platform/mobile-platform-driver.ts +++ b/src/platform/mobile-platform-driver.ts @@ -118,6 +118,9 @@ export class MobilePlatformDriver implements IPlatformDriver { * @param refMap - Map of a11y refs to stable identifiers. * @param timeoutMs - Maximum wait time in milliseconds. * @param within - Must be undefined — scoped search is not supported on mobile. + * @param signal - When aborted during the target wait, throws instead of + * tapping. device-mcp takes no signal: a wait or tap that is already + * running inside the backend is not cancelled. * @returns Click result with success status and target info. */ async click( @@ -126,11 +129,14 @@ export class MobilePlatformDriver implements IPlatformDriver { refMap: Map, timeoutMs: number, within?: WithinScope, + signal?: AbortSignal, ): Promise { rejectWithinScope(within); const query = resolveTargetToQuery(targetType, targetValue, refMap); const deadline = Date.now() + timeoutMs; await this.#backend.waitForElement(query, timeoutMs); + // The request may have timed out while we waited for the target. + signal?.throwIfAborted(); const remaining = deadline - Date.now(); await withTimeout(this.#backend.tapElement(query), remaining, 'tapElement'); return { clicked: true, target: `${targetType}:${targetValue}` }; @@ -143,6 +149,9 @@ export class MobilePlatformDriver implements IPlatformDriver { * @param refMap - Map of a11y refs to stable identifiers. * @param timeoutMs - Maximum wait time in milliseconds. * @param within - Must be undefined — scoped search is not supported on mobile. + * @param signal - When aborted during the target wait or after the focus + * tap, throws instead of tapping or typing. device-mcp takes no signal: a + * wait, tap or typeText that is already running is not cancelled. * @returns Type result with success status and text length. */ async type( @@ -152,13 +161,16 @@ export class MobilePlatformDriver implements IPlatformDriver { refMap: Map, timeoutMs: number, within?: WithinScope, + signal?: AbortSignal, ): Promise { rejectWithinScope(within); const query = resolveTargetToQuery(targetType, targetValue, refMap); const deadline = Date.now() + timeoutMs; await this.#backend.waitForElement(query, timeoutMs); + signal?.throwIfAborted(); let remaining = deadline - Date.now(); await withTimeout(this.#backend.tapElement(query), remaining, 'tapElement'); + signal?.throwIfAborted(); remaining = deadline - Date.now(); await withTimeout(this.#backend.typeText(text), remaining, 'typeText'); return { diff --git a/src/platform/playwright-driver.test.ts b/src/platform/playwright-driver.test.ts index 477105d..2677f19 100644 --- a/src/platform/playwright-driver.test.ts +++ b/src/platform/playwright-driver.test.ts @@ -154,6 +154,76 @@ describe('PlaywrightPlatformDriver', () => { }); }); + describe('abort during the target wait', () => { + it('does not click when the signal aborts while waiting', async () => { + const locator = createMockLocator(); + const controller = new AbortController(); + const reason = new Error('Tool execution timed out after 30000ms'); + vi.spyOn(discoveryModule, 'waitForTarget').mockImplementation( + async () => { + controller.abort(reason); + return locator as any; + }, + ); + + const driver = createDriver(); + await expect( + driver.click( + 'testId', + 'confirm', + new Map(), + 5000, + undefined, + controller.signal, + ), + ).rejects.toBe(reason); + expect(locator.click).not.toHaveBeenCalled(); + }); + + it('does not type when the signal aborts while waiting', async () => { + const locator = createMockLocator(); + const controller = new AbortController(); + vi.spyOn(discoveryModule, 'waitForTarget').mockImplementation( + async () => { + controller.abort(new Error('timed out')); + return locator as any; + }, + ); + + const driver = createDriver(); + await expect( + driver.type( + 'testId', + 'input', + 'text', + new Map(), + 5000, + undefined, + controller.signal, + ), + ).rejects.toThrowError('timed out'); + expect(locator.fill).not.toHaveBeenCalled(); + }); + + it('clicks when the signal is not aborted', async () => { + const locator = createMockLocator(); + vi.spyOn(discoveryModule, 'waitForTarget').mockResolvedValue( + locator as any, + ); + + const driver = createDriver(); + await driver.click( + 'testId', + 'btn', + new Map(), + 5000, + undefined, + new AbortController().signal, + ); + expect(locator.click).toHaveBeenCalled(); + }); + }); + describe('type', () => { it('delegates to waitForTarget and locator.fill', async () => { const locator = createMockLocator(); diff --git a/src/platform/playwright-driver.ts b/src/platform/playwright-driver.ts index 5f37ff7..268cf35 100644 --- a/src/platform/playwright-driver.ts +++ b/src/platform/playwright-driver.ts @@ -66,6 +66,8 @@ export class PlaywrightPlatformDriver implements IPlatformDriver { * @param refMap - Map of a11y refs to selectors. * @param timeoutMs - Maximum time in milliseconds for the interaction. * @param within - Optional parent scope for chained locator resolution. + * @param signal - When aborted during the target wait, throws instead of + * clicking. * @returns The click result with success status and target info. */ async click( @@ -74,6 +76,7 @@ export class PlaywrightPlatformDriver implements IPlatformDriver { refMap: Map, timeoutMs: number, within?: WithinScope, + signal?: AbortSignal, ): Promise { const page = this.#getPage(); const deadline = Date.now() + timeoutMs; @@ -94,6 +97,8 @@ export class PlaywrightPlatformDriver implements IPlatformDriver { ); } + // The request may have timed out while we waited for the target. + signal?.throwIfAborted(); try { await locator.click({ timeout: remaining }); } catch (error) { @@ -122,6 +127,8 @@ export class PlaywrightPlatformDriver implements IPlatformDriver { * @param refMap - Map of a11y refs to selectors. * @param timeoutMs - Maximum time in milliseconds. * @param within - Optional parent scope. + * @param signal - When aborted during the target wait, throws instead of + * typing. * @returns The type result with success status and text length. */ async type( @@ -131,6 +138,7 @@ export class PlaywrightPlatformDriver implements IPlatformDriver { refMap: Map, timeoutMs: number, within?: WithinScope, + signal?: AbortSignal, ): Promise { const page = this.#getPage(); const deadline = Date.now() + timeoutMs; @@ -151,6 +159,7 @@ export class PlaywrightPlatformDriver implements IPlatformDriver { ); } + signal?.throwIfAborted(); await locator.fill(text, { timeout: remaining }); return { diff --git a/src/platform/types.ts b/src/platform/types.ts index caf9e28..0a4d837 100644 --- a/src/platform/types.ts +++ b/src/platform/types.ts @@ -64,14 +64,25 @@ export type ScreenCapture = { }; export type IPlatformDriver = { + /** + * Waits for the target, then clicks it. When `signal` is aborted while the + * driver waits for the target, the driver throws the abort reason and does + * not click. A click that already started is not cancelled. + */ click( targetType: TargetType, targetValue: string, refMap: Map, timeoutMs: number, within?: WithinScope, + signal?: AbortSignal, ): Promise; + /** + * Waits for the target, then types into it. When `signal` is aborted while + * the driver waits for the target, the driver throws the abort reason and + * does not type. Typing that already started is not cancelled. + */ type( targetType: TargetType, targetValue: string, @@ -79,6 +90,7 @@ export type IPlatformDriver = { refMap: Map, timeoutMs: number, within?: WithinScope, + signal?: AbortSignal, ): Promise; waitForElement( diff --git a/src/tools/driver-delegation.test.ts b/src/tools/driver-delegation.test.ts index df2c349..f499701 100644 --- a/src/tools/driver-delegation.test.ts +++ b/src/tools/driver-delegation.test.ts @@ -213,9 +213,22 @@ describe('tool delegation to context.driver', () => { expect.any(Map), expect.any(Number), undefined, + undefined, ); }); + it('clickTool and typeTool pass the request signal to the driver', async () => { + const driver = createMockDriver(); + const { signal } = new AbortController(); + const context = { ...createContextWithDriver(driver), signal }; + + await clickTool({ testId: 'btn' }, context); + await typeTool({ testId: 'input', text: 'hello' }, context); + + expect(vi.mocked(driver.click).mock.calls[0]?.[5]).toBe(signal); + expect(vi.mocked(driver.type).mock.calls[0]?.[6]).toBe(signal); + }); + it('typeTool delegates to driver.type', async () => { const driver = createMockDriver(); const context = createContextWithDriver(driver); @@ -233,6 +246,7 @@ describe('tool delegation to context.driver', () => { expect.any(Map), expect.any(Number), undefined, + undefined, ); }); diff --git a/src/tools/interaction.test.ts b/src/tools/interaction.test.ts index 9d9c1c1..bc6e268 100644 --- a/src/tools/interaction.test.ts +++ b/src/tools/interaction.test.ts @@ -1280,5 +1280,44 @@ describe('interaction', () => { expect(waitSpy).not.toHaveBeenCalled(); }, ); + + it.each([ + [ + 'click', + async (context: ToolContext) => + clickTool({ testId: 'confirm' }, context), + 'click', + ], + [ + 'type', + async (context: ToolContext) => + typeTool({ testId: 'input', text: 'x' }, context), + 'fill', + ], + ] as const)( + '%s returns MM_REQUEST_ABORTED and does not act when the request times out during the target wait', + async (_name, run, action) => { + const controller = new AbortController(); + const locator = createMockLocator(); + vi.spyOn(discoveryModule, 'waitForTarget').mockImplementation( + async () => { + controller.abort(new Error('Tool execution timed out')); + return locator as any; + }, + ); + const context = { + ...createMockContext(), + signal: controller.signal, + } as ToolContext; + + const result = await run(context); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error.code).toBe(ErrorCodes.MM_REQUEST_ABORTED); + } + expect(locator[action]).not.toHaveBeenCalled(); + }, + ); }); }); diff --git a/src/tools/interaction.ts b/src/tools/interaction.ts index 7dcc3b4..1f60c75 100644 --- a/src/tools/interaction.ts +++ b/src/tools/interaction.ts @@ -149,9 +149,14 @@ export async function clickTool( context.refMap, timeoutMs, resolveWithinScope(input.within), + context.signal, ); return createToolSuccess(result); } catch (error) { + const aborted = abortedResponse(context); + if (aborted) { + return aborted; + } const errorInfo = classifyClickError(error); if (errorInfo.code === ErrorCodes.MM_WAIT_TIMEOUT) { return createToolError( @@ -190,9 +195,14 @@ export async function typeTool( context.refMap, timeoutMs, resolveWithinScope(input.within), + context.signal, ); return createToolSuccess(result); } catch (error) { + const aborted = abortedResponse(context); + if (aborted) { + return aborted; + } const errorInfo = classifyTypeError(error); if (errorInfo.code === ErrorCodes.MM_WAIT_TIMEOUT) { return createToolError( From 05a610e715dbb3a865f12a0565bbfdb37ee812f5 Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Thu, 29 Oct 2026 11:20:36 +0000 Subject: [PATCH 3/4] fix!: cap run_steps batchTimeoutMs at 30 s batchTimeoutMs accepted up to 300 s, but run_steps has no timeoutMs, so with the default request limit the queue and the CLI stop the call after 30 s. A larger batchTimeoutMs could never return its partial results. The schema now accepts 1000-30000 ms, like the cdp timeoutMs. The deadline is checked between steps, so partial results need a value below the request limit (the schema description says so). BREAKING CHANGE: run_steps rejects batchTimeoutMs above 30000 with a validation error, also when the server's requestTimeoutMs was raised. Split longer flows into several batches. Review: https://github.com/MetaMask/client-mcp-core/pull/64#discussion_r4410435592 --- src/validation/schemas.test.ts | 20 ++++++++++++++++++++ src/validation/schemas.ts | 7 +++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/validation/schemas.test.ts b/src/validation/schemas.test.ts index 2071230..c340a73 100644 --- a/src/validation/schemas.test.ts +++ b/src/validation/schemas.test.ts @@ -930,3 +930,23 @@ describe('runStepsInputSchema includeObservations', () => { expect(parsed.error?.issues[0]?.message).toContain('use observe'); }); }); + +describe('runStepsInputSchema batchTimeoutMs', () => { + it.each([1000, 30_000])('accepts %i', (batchTimeoutMs) => { + expect( + toolSchemas.run_steps.safeParse({ + steps: [{ tool: 'get_state' }], + batchTimeoutMs, + }).success, + ).toBe(true); + }); + + it('rejects values above the 30 s request ceiling', () => { + expect( + toolSchemas.run_steps.safeParse({ + steps: [{ tool: 'get_state' }], + batchTimeoutMs: 30_001, + }).success, + ).toBe(false); + }); +}); diff --git a/src/validation/schemas.ts b/src/validation/schemas.ts index 1d037c5..719e1b9 100644 --- a/src/validation/schemas.ts +++ b/src/validation/schemas.ts @@ -591,10 +591,13 @@ export const runStepsInputSchema = z.object({ .number() .int() .min(1000) - .max(300_000) + .max(30_000) .describe( 'Overall timeout for the batch in milliseconds. ' + - 'When exceeded, remaining steps are marked as skipped and partial results are returned.', + 'When exceeded, remaining steps are marked as skipped and partial results are returned. ' + + 'The deadline is checked between steps. Capped at 30 000, the default request limit of ' + + 'the queue and the CLI; to get partial results, leave room for the step in progress ' + + 'and the observation (e.g. 20 000).', ) .optional(), // Removed in favour of `observe`. Rejected (not ignored) so old callers From 4084787463b44f4c97f4ceb372c06a3af76cb7f7 Mon Sep 17 00:00:00 2001 From: cryptotavares Date: Thu, 29 Oct 2026 11:20:37 +0000 Subject: [PATCH 4/4] docs: refs after --observe none, reads and animations, abort limits - --observe none does not refresh refs. Role/name, DOM-path and mobile label refs can match a different element after the screen changed: target by testId or run describe-screen first. - Reads and describe-screen do not wait for animations, so chaining a --observe none mutation with a read can catch a screen mid-animation. - A click or type that timed out during the target wait does not act. An action already in progress is not stopped: Playwright's checks inside locator.click/fill, and on mobile the device-mcp tap and typeText. - run_steps batchTimeoutMs: range, checked between steps, 30 s request limit. Review: https://github.com/MetaMask/client-mcp-core/pull/62#discussion_r4410204413 https://github.com/MetaMask/client-mcp-core/pull/62#discussion_r4410155258 https://github.com/MetaMask/client-mcp-core/pull/64#discussion_r4410435592 https://github.com/MetaMask/client-mcp-core/pull/64#discussion_r4410499219 --- README.md | 8 ++++++-- SKILL.md | 8 ++++++-- src/cli/mm.ts | 4 +++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 91a7e67..bcde817 100644 --- a/README.md +++ b/README.md @@ -410,7 +410,7 @@ The daemon routes `POST /tool/:name` requests through the registry, applies Zod | `get_contract_address` | Looks up the deployed address of a contract by name from the session's deployment registry. | | `list_contracts` | Lists all contracts deployed in the current session with addresses and deployment timestamps. | | **Batching** | | -| `run_steps` | Executes a batch of tool invocations sequentially. Supports `stopOnError` to halt on first failure, a top-level `observe` level for the single observation collected after the batch (default `diff`; without one, the last executed step's `observe` is used), and `batchTimeoutMs` to set an overall deadline (remaining steps are skipped on timeout). Accepts tool aliases like `navigate_home` / `navigate-home`. Returns per-step results with timing. | +| `run_steps` | Executes a batch of tool invocations sequentially. Supports `stopOnError` to halt on first failure, a top-level `observe` level for the single observation collected after the batch (default `diff`; without one, the last executed step's `observe` is used), and `batchTimeoutMs` (1 000–30 000 ms) to set an overall deadline (checked between steps; remaining steps are skipped). The whole `run_steps` call has the default 30 s request limit, so to get partial results set `batchTimeoutMs` below it (for example 20 000) and keep batches short. Accepts tool aliases like `navigate_home` / `navigate-home`. Returns per-step results with timing. | | **Advanced** | | | `mock_network` | Adds, clears, lists, and inspects targeted Playwright network mocks on the active browser context. Unmatched same-origin requests are continued unchanged. | | `cdp` | Sends a raw Chrome DevTools Protocol command, dispatched through the active platform driver. **Browser:** targets the page's Chrome CDP session (full Runtime/DOM/Network/Page surface); destructive methods (`Browser.close`, `Target.closeTarget`, etc.) are blocked. **Mobile (iOS/Android):** targets the app's React Native Hermes JS runtime via Metro's inspector proxy (delegates to `@metamask/device-mcp`, needs a DEBUG build with Metro running); only the JS-engine subset exists (`Runtime`, `Debugger`, `Log`, `HeapProfiler` — no DOM/Page/Network) and `Runtime.terminateExecution` / `Inspector.detached` are blocked. Optional `metroPort` / `appId` are mobile-only (ignored on browser). Categorized as `cdp`: no observation is collected and refs are not refreshed, so run `describe_screen` after a call that changes state. See [Browser vs Mobile CDP](#browser-vs-mobile-hermes-cdp). | @@ -549,11 +549,15 @@ Every tool input (and the `/launch` and `/cleanup` bodies) accepts an optional ` When `observe` is absent, the default comes from the tool category: `diff` for **mutating** tools (click, type, navigate, launch, cleanup, build, etc.) and `run_steps`; `none` for **read-only**, **discovery** and `cdp` tools. An explicit `diff` or `full` on a read-only, discovery or `cdp` tool collects and returns an observation. +`observe: 'none'` does not refresh the refs. After a call with `none` that changes the screen, the refs from the last observation or `describe_screen` still point at the previous screen. Refs backed by a test ID or an element id fail with `MM_TARGET_NOT_FOUND` when the element is gone. Refs built from role and name, a DOM path, or a mobile label can match a different element on the new screen. After `none`, target by test ID, or call `describe_screen` (or use `observe: 'full'`) before using refs again. + +On the browser, only mutating, batch and `cdp` calls wait for animations to settle (up to 1 s; every running finite animation counts, also off screen, so a long finite animation adds up to 1 s) before the observation. A read-only or discovery call with `observe: 'diff'` or `'full'` (and `describe_screen`) reads the page at once, so right after a `none` mutation it can catch an animation in progress. To get a settled observation, let the mutation itself observe (`diff` or `full`) instead of chaining it with `none`. + For `run_steps`, the top-level `observe` sets the level of the one observation collected after the batch. Without it, the `observe` in the last executed step's `args` is used; per-step `observe` on other steps is ignored. `includeObservations` was removed. Observations contain `testIds` and `a11y` only. They do not include app state; call `get_state` for that. -A call that times out is cancelled: its signal is aborted, it stops at its next step, it collects no observation, and its refs are dropped. +A call that times out is cancelled: its signal is aborted, it stops at its next step, it collects no observation, and its refs are dropped. `click` and `type` check the signal again after they wait for the target, so a call that timed out during that wait does not click or type. An action that already started is not cancelled. On the browser, the action includes Playwright's own checks inside `locator.click` / `locator.fill` (stable, enabled, not covered), which can wait up to the rest of the call's `timeoutMs`. On mobile, `@metamask/device-mcp` takes no signal: its element wait, tap and `typeText` run until they end, and the driver checks the signal between them. ## CLI Reference diff --git a/SKILL.md b/SKILL.md index 81d9f39..f6be665 100644 --- a/SKILL.md +++ b/SKILL.md @@ -51,6 +51,10 @@ mm navigate-home --observe full # after navigation: get the whole screen mm get-state --observe full # a read that also returns an observation ``` +**After `--observe none`, refs are not refreshed.** If the call changed the screen, the refs you have still describe the previous screen. A ref backed by a testId fails loudly (`MM_TARGET_NOT_FOUND`), but a ref built from role and name, a DOM path or a mobile label can click a **different element**. After `--observe none`, target by `--testid`, or run `mm describe-screen` before you use refs again. + +**Only mutating and batch calls wait for animations** (browser, up to 1 s) before they observe. `mm describe-screen` and a read with `--observe full` read the page at once, so `mm click … --observe none && mm get-state --observe full` can catch a screen mid-animation. To get a settled screen after an action, let the action observe: `mm click … --observe full`. + Observations contain `testIds` and `a11y` only — no app `state`. Call `mm get-state` for state. On mobile, each observation and `describe-screen` comes from one device snapshot. The Android system status bar and navigation bar and the Expo DevLauncher "Tools" button (iOS `gearshape.fill`) are not part of the app and are removed from `testIds` and `a11y`. Other system windows, such as the biometric prompt (`com.android.systemui:id/biometric_*`), are kept. @@ -91,7 +95,7 @@ After a mutating action, the response includes fresh screen data: You can use the `ref` values from `observations.a11y.nodes` for the next interaction — no `describe-screen` needed. Refs are numbered in screen order: on an unchanged screen they stay the same, but when elements appear or disappear above an element its ref can change. With `--observe diff`, those elements are listed in the diff with their new refs, so always take refs from the latest response. Note that refs in compacted observations may be summary nodes (e.g., `"55 options (refs e2–e56)"`) when there are 3+ options under a combobox or listbox. -**Timeouts cancel the call:** when a call times out (`Tool execution timed out`), it is cancelled. It stops at its next step and cannot overwrite the refs of later calls. The action itself may still have happened, so run `describe-screen` before retrying. +**Timeouts cancel the call:** when a call times out (`Tool execution timed out`), it is cancelled. It stops at its next step and cannot overwrite the refs of later calls. A `click` or `type` that timed out while waiting for its target does not click or type. An action that already started may still have happened (browser: Playwright's checks inside the click; mobile: a tap in progress is not stopped), so run `describe-screen` before retrying. **Quick reference:** @@ -468,7 +472,7 @@ Executes multiple tool invocations in sequence from a JSON array. Each step spec mm run-steps '{"steps":[{"tool":"click","args":{"a11yRef":"e3"}},{"tool":"wait_for","args":{"a11yRef":"e5"}}]}' ``` -Supports `stopOnError` (halt on first failure) and returns per-step results with timing. A top-level `observe` (`none` / `diff` / `full`, default `diff`) sets the observation collected after the batch; see `run_steps` and `observe` above. Use `batchTimeoutMs` to set an overall deadline — if exceeded, remaining steps are marked as skipped and partial results are returned immediately. The summary includes a `skipped` count alongside `succeeded` and `failed`. +Supports `stopOnError` (halt on first failure) and returns per-step results with timing. A top-level `observe` (`none` / `diff` / `full`, default `diff`) sets the observation collected after the batch; see `run_steps` and `observe` above. Use `batchTimeoutMs` (1000–30000 ms) to set an overall deadline — it is checked between steps; if exceeded, remaining steps are marked as skipped and partial results are returned. The whole `run_steps` call is stopped after 30 s (the request limit), so use a lower `batchTimeoutMs` (for example 20000) to get partial results, and split longer flows into several batches. The summary includes a `skipped` count alongside `succeeded` and `failed`. Tool aliases are supported in steps: `navigate_home` / `navigate-home`, `navigate_settings` / `navigate-settings`, and `navigate_notification` / `navigate-notification` resolve to `navigate` with the appropriate `screen` argument. You can also use `ref` as shorthand for `a11yRef` in step args and within targets. diff --git a/src/cli/mm.ts b/src/cli/mm.ts index 8b60472..f19d5f5 100644 --- a/src/cli/mm.ts +++ b/src/cli/mm.ts @@ -1866,7 +1866,9 @@ Global Options: Overrides MM_PROJECT and git-based discovery. --observe Observation returned after the call: none | diff | full. Default: diff for mutating commands and run-steps, - none for reads, discovery and cdp. + none for reads, discovery and cdp. none does not + refresh refs: run describe-screen before using refs + again. --pretty Indent JSON output. Default: one line per result when stdout is not a terminal (agents, pipes), indented on a terminal.