From 569b6b41399f05874d30421396c0daa4c81ff711 Mon Sep 17 00:00:00 2001 From: Elliot Drel Date: Tue, 6 Oct 2026 18:29:02 -0400 Subject: [PATCH 1/6] fix(browser): scope snapshots to foreground dialogs and paginate controls Keep hidden and inert background subtrees out of dialog snapshots without dropping reachable offscreen controls. Preserve global actionable indices across bounded pages and expose scope, totals, and continuation in both native backends. Add real Chromium regression coverage, malformed-option tests, Rust formatter coverage, native e2e checks, and CI/documentation updates. Fixes #18. --- .github/workflows/ci.yml | 1 + README.md | 14 ++ chrome-extension/background.js | 79 +++++++++-- chrome-extension/background.test.mjs | 27 +++- macos/Sources/main.swift | 31 ++++- scripts/e2e-browser.mjs | 37 ++++++ scripts/snapshot-dom.test.mjs | 189 +++++++++++++++++++++++++++ windows-linux/src/browser.rs | 31 ++++- windows-linux/src/tools.rs | 14 +- 9 files changed, 406 insertions(+), 17 deletions(-) create mode 100644 scripts/snapshot-dom.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 33050ab..6ec3933 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -48,6 +48,7 @@ jobs: run: | cargo build --release --locked --manifest-path windows-linux/Cargo.toml chrome=$(npx -y @puppeteer/browsers install chrome@stable --path "$RUNNER_TEMP/cft" | tail -1 | sed 's/^[^ ]* //') + node scripts/snapshot-dom.test.mjs --chrome "$chrome" node scripts/e2e-browser.mjs --server "$PWD/windows-linux/target/release/munim-computer-use" --chrome "$chrome" swift: diff --git a/README.md b/README.md index 2f357cc..18e1f58 100644 --- a/README.md +++ b/README.md @@ -200,6 +200,18 @@ This is a guard rail for an agent that follows its instructions, not a sandbox. Every action tool also takes `return_state`, which returns the state after the action in the same call. +`browser_snapshot` automatically scopes to the topmost visible dialog when one is +open, so covered background controls cannot consume the dialog's budget. Hidden +and inert subtrees are excluded; reachable offscreen controls remain available. +Native modeless dialogs and explicit `aria-modal="false"` do not scope the page. +It returns up to `limit` controls (default and maximum `250`), reports `scope`, +`offset`, `limit`, and `total`, and explicitly says when it is truncated and which +`offset` to request next. Continue on the same `tab_id` and `session_id`. Indices +are global within the selected scope and stay actionable across pages of an +unchanged UI; after a UI change or navigation, take a fresh snapshot from +`offset=0`. An action's `return_state` also starts at `offset=0` and uses the same +scope and truncation reporting. + Names, argument shapes and descriptions are identical on every platform, and CI enforces it (`node scripts/check-tool-parity.mjs`); a model that learned them on a Mac needs nothing new on Windows. To change a tool, edit both literals at once with `scripts/tool-defs.mjs` rather than by hand. ## Repository layout @@ -228,6 +240,8 @@ Linux notes: element actions work everywhere; coordinate clicks need an X11 or X cd windows-linux && cargo test # Rust server node chrome-extension/background.test.mjs # extension, against a fake Chrome node scripts/check-tool-parity.mjs # Swift and Rust tool lists match +# Snapshot DOM regressions (dependency-free CDP, isolated profile): +node scripts/snapshot-dom.test.mjs --chrome # The browser tools end to end, in a throwaway Chrome for Testing profile that # never touches your own Chrome (npx playwright install chromium to get one): node scripts/e2e-browser.mjs --server --chrome diff --git a/chrome-extension/background.js b/chrome-extension/background.js index 41484ca..63dbd96 100644 --- a/chrome-extension/background.js +++ b/chrome-extension/background.js @@ -555,7 +555,9 @@ async function send(tabId, method, params = {}) { /// A compact outline of the interactive elements on the page, with ids the /// agent can click. Mirrors the accessibility-tree tools on the desktop side. -const SNAPSHOT_JS = `(() => { +const SNAPSHOT_JS = `((options = {}) => { + const offset = options.offset ?? 0; + const limit = options.limit ?? 250; const out = []; const sel = 'a,button,input,textarea,select,cfc-select,mat-option,[role=button],[role=link],[role=textbox],[role=combobox],[role=listbox],[role=option],[role=menu],[role=menuitem],[aria-haspopup],[contenteditable=true],summary'; let i = 0; @@ -563,35 +565,88 @@ const SNAPSHOT_JS = `(() => { // would otherwise keep its old number, and querySelector, which returns the // first match, could send a click or a credential fill to it instead. for (const old of document.querySelectorAll('[data-cu-idx]')) old.removeAttribute('data-cu-idx'); - for (const el of document.querySelectorAll(sel)) { + // Accessibility-hidden/inert ancestors are not actionable. Do not use viewport + // intersection here: indexed click scrolls reachable offscreen controls into view. + const visible = (el) => { const r = el.getBoundingClientRect(); - if (r.width < 2 || r.height < 2) continue; + if (r.width < 2 || r.height < 2) return false; + let escapedInert = false; + for (let node = el; node; node = node.parentElement) { + if (node.hidden || node.getAttribute('aria-hidden')?.toLowerCase() === 'true') return false; + if (node.inert && !escapedInert) return false; + // showModal() escapes ancestor inertness, but an explicitly inert dialog + // or subtree within it remains inert. + if (node.matches('dialog:modal')) escapedInert = true; + const style = getComputedStyle(node); + if (style.display === 'none' || style.contentVisibility === 'hidden' || Number(style.opacity) === 0) return false; + } const style = getComputedStyle(el); - if (style.visibility === 'hidden' || style.display === 'none') continue; + return style.visibility !== 'hidden' && style.visibility !== 'collapse'; + }; + // Native modal dialogs live in Chrome's top layer, above ordinary z-indexes. + // For custom dialogs, use paint order at their visible centre, not DOM order + // or raw z-index (which is only meaningful inside a stacking context). + // Modeless native dialogs and explicit aria-modal=false keep page controls. + const dialogs = Array.from(document.querySelectorAll('dialog:modal,[role=dialog]:not([aria-modal=false i]),[role=alertdialog]:not([aria-modal=false i]),[aria-modal=true i]')).filter(visible); + const native = dialogs.filter((dialog) => dialog.matches(':modal')); + // Keep nested custom dialogs inside the native top layer eligible too. + const candidates = dialogs.filter((dialog) => !native.length || native.some((layer) => layer.contains(dialog))).reverse(); + let modal = null; + for (const candidate of candidates) { + const r = candidate.getBoundingClientRect(); + const left = Math.max(0, r.left), right = Math.min(innerWidth, r.right); + const top = Math.max(0, r.top), bottom = Math.min(innerHeight, r.bottom); + if (left >= right || top >= bottom) continue; + const painted = document.elementsFromPoint((left + right) / 2, (top + bottom) / 2); + // The first dialog in the hit-test stack wins, including nested dialogs. + for (const hit of painted) { + const owner = candidates.find((dialog) => dialog.contains(hit)); + if (owner) { modal = owner; break; } + } + if (modal) break; + } + // A background tab can lack a hit-test stack. Keep its visible dialog usable + // rather than falling back to the background document and losing it again. + if (!modal) modal = candidates.find((dialog) => { + const r = dialog.getBoundingClientRect(); + return r.right > 0 && r.bottom > 0 && r.left < innerWidth && r.top < innerHeight; + }) || null; + const scope = modal || document; + for (const el of scope.querySelectorAll(sel)) { + if (!visible(el)) continue; + const index = i++; + // Tag the full selected scope so indices stay global and earlier pages remain + // actionable when paging an unchanged UI. Only label/serialize this page. + el.setAttribute('data-cu-idx', String(index)); + if (index < offset || index >= offset + limit) continue; + const r = el.getBoundingClientRect(); // A password field's value is the secret itself: never let it reach the // model as a label (browser_request_credentials fills these instead). const secret = el.tagName === 'INPUT' && el.type === 'password'; const label = (el.getAttribute('aria-label') || el.innerText || (secret ? '' : el.value) || el.getAttribute('title') || el.getAttribute('placeholder') || '') .replace(/\\s+/g, ' ').trim().slice(0, 90); - el.setAttribute('data-cu-idx', String(i)); const kind = el.tagName === 'INPUT' && el.type && el.type !== 'text' ? '[' + el.type + ']' : ''; out.push({ - i: i++, + i: index, tag: el.tagName.toLowerCase() + kind, label, x: Math.round(r.left + r.width / 2), y: Math.round(r.top + r.height / 2), - inView: r.top >= 0 && r.bottom <= innerHeight, + inView: r.top >= 0 && r.bottom <= innerHeight && r.left >= 0 && r.right <= innerWidth, }); - if (i >= 250) break; } - return { title: document.title, url: location.href, elements: out }; + const end = Math.min(i, offset + out.length); + return { title: document.title, url: location.href, elements: out, + scope: modal ? 'modal' : 'page', total: i, offset, limit, + truncated: end < i, nextOffset: end < i ? end : null }; })()`; -async function snapshot(tabId) { +async function snapshot(tabId, { offset = 0, limit = 250 } = {}) { + if (!Number.isSafeInteger(offset) || offset < 0 || offset > 2147483647) throw new Error("offset must be an integer from 0 to 2147483647"); + if (!Number.isSafeInteger(limit) || limit < 1 || limit > 250) throw new Error("limit must be an integer from 1 to 250"); const res = await send(tabId, "Runtime.evaluate", { - expression: SNAPSHOT_JS, + expression: SNAPSHOT_JS.slice(0, -2) + `(${JSON.stringify({ offset, limit })})`, returnByValue: true, }); if (res?.exceptionDetails) throw new Error(res.exceptionDetails.text || "evaluate failed"); @@ -1548,7 +1603,7 @@ const handlers = { const clientId = requireClientId(p); assertOwned(clientId, p.tabId); await checkTab(clientId, p.tabId, p.sites, "read"); - return snapshot(p.tabId); + return snapshot(p.tabId, { offset: p.offset, limit: p.limit }); }, read: async (p) => { const clientId = requireClientId(p); diff --git a/chrome-extension/background.test.mjs b/chrome-extension/background.test.mjs index 5824032..5ec8404 100644 --- a/chrome-extension/background.test.mjs +++ b/chrome-extension/background.test.mjs @@ -640,12 +640,36 @@ test("a debugger session Chrome ended is attached again on the next command", as pageEval = () => ({ result: { value: {} } }); }); +test("snapshot forwards bounded pagination and refuses malformed options before evaluation", async () => { + const expressions = []; + pageEval = (method, params) => { + if (params.expression?.includes("querySelectorAll(sel)")) expressions.push(params.expression); + return { result: { value: {} } }; + }; + await call("snapshot", { tabId: pageTab, offset: 250, limit: 20 }); + assert.ok(expressions[0].endsWith('({"offset":250,"limit":20})')); + const count = expressions.length; + const malformed = [ + { offset: -1 }, { offset: "250" }, { offset: 1.5 }, { offset: null }, + { offset: 2147483648 }, { offset: Number.MAX_SAFE_INTEGER + 1 }, + { limit: 0 }, { limit: 251 }, { limit: "10" }, { limit: null }, + ]; + for (const args of malformed) { + assert.match(await refuses("snapshot", { tabId: pageTab, ...args }), /offset must|limit must/); + } + assert.equal(expressions.length, count, "malformed pagination reached page script"); + await call("snapshot", { tabId: pageTab }); + assert.ok(expressions.at(-1).endsWith('({"offset":0,"limit":250})')); + pageEval = () => ({ result: { value: {} } }); +}); + test("a snapshot clears indices an earlier snapshot left on now-hidden elements", () => { const element = (tag, { hidden = false, idx } = {}) => { const attributes = new Map(idx === undefined ? [] : [["data-cu-idx", idx]]); return { tagName: tag.toUpperCase(), innerText: tag, + matches: () => false, hidden, getAttribute: (name) => attributes.get(name) ?? null, setAttribute: (name, value) => attributes.set(name, String(value)), @@ -661,12 +685,13 @@ test("a snapshot clears indices an earlier snapshot left on now-hidden elements" const document = { title: "Step 2", querySelectorAll: (selector) => - selector === "[data-cu-idx]" ? elements.filter((el) => el.hasAttribute("data-cu-idx")) : elements, + selector === "[data-cu-idx]" ? elements.filter((el) => el.hasAttribute("data-cu-idx")) : selector.startsWith("dialog") ? [] : elements, }; const page = vm.createContext({ document, location: { href: "https://shop.example/step2" }, innerHeight: 800, + innerWidth: 1200, getComputedStyle: () => ({ visibility: "visible", display: "block" }), }); const result = vm.runInContext(vm.runInContext("SNAPSHOT_JS", extensionContext), page); diff --git a/macos/Sources/main.swift b/macos/Sources/main.swift index 134c1a3..54215c6 100644 --- a/macos/Sources/main.swift +++ b/macos/Sources/main.swift @@ -2620,6 +2620,15 @@ func bridgeText(_ result: BridgeOutcome, _ describe: ([String: Any]) -> String) func describeSnapshot(_ payload: [String: Any]) -> String { let elements = payload["elements"] as? [[String: Any]] ?? [] var lines = ["\(payload["title"] as? String ?? "?") [\(payload["url"] as? String ?? "")]"] + if let total = payload["total"] as? Int { + let offset = payload["offset"] as? Int ?? 0 + let limit = payload["limit"] as? Int ?? 250 + let scope = payload["scope"] as? String ?? "page" + lines.append("scope: \(scope); offset=\(offset), limit=\(limit), total=\(total)") + if payload["truncated"] as? Bool == true, let next = payload["nextOffset"] as? Int { + lines.append("truncated — continue with browser_snapshot offset=\(next), limit=\(limit) (same tab_id/session_id; unchanged UI keeps indices)") + } + } for element in elements { let index = element["i"] as? Int ?? -1 let tag = element["tag"] as? String ?? "?" @@ -2632,7 +2641,13 @@ func describeSnapshot(_ payload: [String: Any]) -> String { func toolBrowserSnapshot(_ args: [String: Any]) -> String { guard let tabId = args["tab_id"] as? Int else { return "error: missing required argument 'tab_id'" } - return bridgeText(browserSessionCall("snapshot", ["tabId": tabId], args: args), describeSnapshot) + var params: [String: Any] = ["tabId": tabId] + // Preserve invalid values too: the extension validates the same contract on + // every platform instead of silently replacing a malformed request. + for key in ["offset", "limit"] { + if let value = args[key] { params[key] = value } + } + return bridgeText(browserSessionCall("snapshot", params, args: args), describeSnapshot) } /// browser_read: the page text, and where to continue if it was cut off. @@ -3608,7 +3623,7 @@ let toolDefs: [[String: Any]] = [ ], [ "name": "browser_snapshot", - "description": "List the interactive elements (links, buttons, inputs) on the page in one of the agent's tabs, with the index each one has for browser_click, plus the page title and URL. Inputs show their type, such as input[password]. Works on a background tab, so the user can be looking at something else. Use it before every browser_click, because indices change when the page changes; use browser_read for the page's text. Read-only.", + "description": "List the interactive elements (links, buttons, inputs) on the page in one of the agent's tabs, with the index each one has for browser_click, plus the page title and URL. Inputs show their type, such as input[password]. Works on a background tab, so the user can be looking at something else. Use it before every browser_click, because indices change when the page changes; use browser_read for the page's text. Read-only. Automatically scopes to the topmost visible dialog when one is open, excluding hidden/inert controls but retaining reachable offscreen controls. Results report scope, total and truncation. Continue with offset and limit on the same tab/session; indices stay global across pages of an unchanged UI. Snapshot again after UI changes.", "inputSchema": [ "type": "object", "properties": [ @@ -3622,6 +3637,18 @@ let toolDefs: [[String: Any]] = [ "type": "integer", "description": "tab_id of one of the agent's tabs, from browser_open_tab or browser_list_tabs", ], + "offset": [ + "type": "integer", + "minimum": 0, + "maximum": 2147483647, + "description": "Control offset for pagination (default 0). Use the continuation offset from the previous result on an unchanged UI.", + ], + "limit": [ + "type": "integer", + "minimum": 1, + "maximum": 250, + "description": "Maximum controls returned per page (default 250, maximum 250).", + ], ], "required": ["tab_id"], ], diff --git a/scripts/e2e-browser.mjs b/scripts/e2e-browser.mjs index 6030785..e40b26e 100644 --- a/scripts/e2e-browser.mjs +++ b/scripts/e2e-browser.mjs @@ -474,6 +474,43 @@ check("an invalid policy file blocks instead of being ignored", async () => { fs.rmSync(policyPath); }); +check("snapshot scopes appended modal controls and indexed actions reach them", async () => { + await ok("browser_navigate", { tab_id: tab, url: `${base}/next`, return_state: true }); + const background = Array.from({ length: 260 }, (_, i) => ``).join(""); + const html = `
${background}
`; + await evaluateIn((url) => url === `${base}/next`, `document.body.innerHTML = ${JSON.stringify(html)}; document.getElementById('modal-next').onclick = () => document.title = 'Modal target clicked'; true`); + const state = await ok("browser_snapshot", { tab_id: tab }); + assert.match(state, /scope: modal; offset=0, limit=250, total=3/); + assert.match(state, /\[0\] input "Domain"/); + assert.match(state, /\[1\] button "Next"/); + assert.match(state, /\[2\] button "Verify"/); + assert.doesNotMatch(state, /Background/); + const after = await ok("browser_click", { tab_id: tab, index: 1, return_state: true }); + assert.match(after, /Modal target clicked/); + assert.match(after, /scope: modal/); +}); + +check("snapshot pagination reaches all controls with stable global indices", async () => { + const html = Array.from({ length: 270 }, (_, i) => ``).join(""); + await evaluateIn((url) => url === `${base}/next`, `document.body.innerHTML = ${JSON.stringify(html)}; document.querySelectorAll('button')[250].onclick = () => document.title = 'Page two clicked'; true`); + const first = await ok("browser_snapshot", { tab_id: tab }); + assert.match(first, /scope: page; offset=0, limit=250, total=270/); + assert.match(first, /truncated — continue with browser_snapshot offset=250, limit=250/); + assert.doesNotMatch(first, /\[250\] button/); + const second = await ok("browser_snapshot", { tab_id: tab, offset: 250, limit: 20 }); + assert.match(second, /scope: page; offset=250, limit=20, total=270/); + assert.match(second, /\[250\] button "Control 250"/); + assert.match(second, /\[269\] button "Control 269"/); + assert.doesNotMatch(second, /truncated/); + const after = await ok("browser_click", { tab_id: tab, index: 250, return_state: true }); + assert.match(after, /Page two clicked/); + for (const options of [{ offset: -1 }, { limit: 251 }]) { + const result = await tool("browser_snapshot", { tab_id: tab, ...options }); + assert.equal(result.isError, true); + assert.match(result.text, /offset must|limit must/); + } +}); + let failed = 0; for (const [name, body] of checks) { try { diff --git a/scripts/snapshot-dom.test.mjs b/scripts/snapshot-dom.test.mjs new file mode 100644 index 0000000..272742a --- /dev/null +++ b/scripts/snapshot-dom.test.mjs @@ -0,0 +1,189 @@ +#!/usr/bin/env node +// Real Chromium regression, dependency-free CDP; never uses the user's profile. +// node scripts/snapshot-dom.test.mjs --chrome +// Add --source to run the same regression against a baseline. +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; +import vm from "node:vm"; +import { spawn } from "node:child_process"; +import { fileURLToPath } from "node:url"; + +const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const option = (name) => { + const at = process.argv.indexOf(name); + return at >= 0 ? process.argv[at + 1] : undefined; +}; +const chromePath = option("--chrome"); +if (!chromePath) throw new Error("provide --chrome "); +const scratch = process.env.TMPDIR || path.join(root, ".snapshot-test"); +fs.mkdirSync(scratch, { recursive: true }); +const profile = fs.mkdtempSync(path.join(scratch, "snapshot-dom-")); +const chrome = spawn(chromePath, [ + `--user-data-dir=${profile}`, + "--headless=new", + "--remote-debugging-port=0", + "--no-first-run", + ...(process.platform === "linux" ? ["--no-sandbox"] : []), + "about:blank", +], { stdio: ["ignore", "ignore", "pipe"] }); +let chromeLog = ""; +chrome.stderr.on("data", (chunk) => (chromeLog += chunk)); +chrome.on("error", (error) => (chromeLog += error.message)); +let socket; +try { + let port; + for (let n = 0; n < 100; n++) { + try { + port = Number(fs.readFileSync(path.join(profile, "DevToolsActivePort"), "utf8").split("\n")[0]); + break; + } catch {} + if (!chrome.pid || chrome.exitCode !== null || chrome.signalCode !== null) break; + await new Promise((resolve) => setTimeout(resolve, 100)); + } + assert.ok(port, `Chromium did not start:\n${chromeLog}`); + // DevTools publishes its port before the initial page target is ready. + let target; + for (let n = 0; n < 100 && !target; n++) { + const targets = await (await fetch(`http://127.0.0.1:${port}/json`)).json(); + target = targets.find((entry) => entry.type === "page" && entry.webSocketDebuggerUrl); + if (!target) await new Promise((resolve) => setTimeout(resolve, 100)); + } + assert.ok(target, `Chromium did not publish a page target:\n${chromeLog}`); + socket = new WebSocket(target.webSocketDebuggerUrl); + await new Promise((resolve, reject) => { + socket.addEventListener("open", resolve, { once: true }); + socket.addEventListener("error", reject, { once: true }); + }); + let id = 0; + const pending = new Map(); + socket.addEventListener("message", (event) => { + const reply = JSON.parse(event.data); + pending.get(reply.id)?.(reply); + pending.delete(reply.id); + }); + async function command(method, params) { + const current = ++id; + const result = new Promise((resolve, reject) => { + const timer = setTimeout(() => { + pending.delete(current); + reject(new Error(`CDP timeout: ${method}`)); + }, 10000); + pending.set(current, (reply) => { + clearTimeout(timer); + resolve(reply); + }); + }); + socket.send(JSON.stringify({ id: current, method, params })); + const reply = await result; + assert.ok(!reply.error, JSON.stringify(reply)); + return reply.result; + } + async function evaluate(expression) { + const result = await command("Runtime.evaluate", { expression, returnByValue: true, awaitPromise: true }); + assert.ok(!result.exceptionDetails, JSON.stringify(result)); + return result.result.value; + } + const source = fs.readFileSync(option("--source") || path.join(root, "chrome-extension/background.js"), "utf8"); + const declaration = source.slice(source.indexOf("const SNAPSHOT_JS ="), source.indexOf("\nasync function snapshot(")); + const expression = vm.runInNewContext(declaration + "\nSNAPSHOT_JS"); + const clickDeclaration = source.slice(source.indexOf("const CLICK_JS ="), source.indexOf("\n/// Click a snapshotted element")); + const click = vm.runInNewContext(clickDeclaration + "\nCLICK_JS"); + const snapshot = (options = {}) => evaluate(expression.slice(0, -2) + `(${JSON.stringify(options)})`); + const set = (html) => evaluate(`document.body.innerHTML = ${JSON.stringify(html)}; true`); + const buttons = (count, prefix = "Background") => Array.from({ length: count }, (_, i) => ``).join(""); + await set(`
${buttons(260)}
`); + const modal = await snapshot(); + console.log(`original reproduction: ${modal.elements.length} controls; labels=${modal.elements.slice(0, 3).map((el) => el.label).join(", ")}`); + assert.deepEqual(modal.elements.map((el) => el.label), ["Domain", "Next", "Verify"]); + assert.equal(modal.scope, "modal"); + console.log("PASS appended modal excludes 260 hidden background controls"); + // Keep evaluating the original target after activating a different tab. + const foreground = await command("Target.createTarget", { url: "about:blank" }); + await command("Target.activateTarget", { targetId: foreground.targetId }); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Domain", "Next", "Verify"]); + console.log("PASS modal snapshot works in a background tab"); + + await set(`${buttons(260)}
`); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Foreground"]); + console.log("PASS modal budget excludes ordinary covered background"); + + await set('
'); + assert.equal((await snapshot()).elements[0].label, "Top"); + await evaluate('document.body.innerHTML += \'\'; document.querySelector("dialog").showModal(); true'); + assert.equal((await snapshot()).elements[0].label, "Native top layer"); + await evaluate('document.querySelector("dialog").innerHTML += \'
\'; true'); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Nested native"]); + console.log("PASS painted stacking and native modal top layer"); + + await set('
'); + await evaluate('document.querySelector("dialog").showModal(); true'); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Escaped inert"]); + console.log("PASS native modal escapes ancestor inertness, not explicit descendant inertness"); + + await set(''); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Page action", "Modeless"]); + await set('
'); + assert.equal((await snapshot()).scope, "page"); + assert.equal((await snapshot()).total, 2); + console.log("PASS modeless and explicitly nonmodal dialogs retain page controls"); + + await set('
'); + assert.equal((await snapshot()).elements[0].label, "High context"); + await set('
'); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Nested"]); + console.log("PASS stacking contexts and nested topmost dialog"); + + await set(`${buttons(260)}
${buttons(300, "Modal")}
`); + const modalFirst = await snapshot({ limit: 100 }); + assert.equal(modalFirst.total, 300); + assert.equal(modalFirst.nextOffset, 100); + const modalLast = await snapshot({ offset: 200, limit: 100 }); + assert.equal(modalLast.elements[0].label, "Modal 200"); + assert.equal(modalLast.elements[0].i, 200); + assert.equal(modalLast.nextOffset, null); + await evaluate('globalThis.savedHitTest = document.elementsFromPoint; document.elementsFromPoint = () => []; true'); + assert.equal((await snapshot()).scope, "modal"); + await evaluate('document.elementsFromPoint = globalThis.savedHitTest; delete globalThis.savedHitTest; true'); + console.log("PASS large modal pagination and background-tab hit-test fallback"); + + await set('
'); + const accessible = await snapshot(); + assert.deepEqual(accessible.elements.map((el) => el.label), ["Offscreen", ""]); + assert.equal(accessible.elements[0].inView, false); + assert.ok(!JSON.stringify(accessible).includes("DO-NOT-LEAK")); + console.log("PASS hidden/inert ancestors excluded, offscreen retained, password protected"); + + await set(buttons(520, "Control")); + const first = await snapshot(); + assert.equal(first.elements.length, 250); + assert.equal(first.total, 520); + assert.equal(first.truncated, true); + assert.equal(first.nextOffset, 250); + const second = await snapshot({ offset: first.nextOffset, limit: 250 }); + assert.equal(second.elements[0].i, 250); + assert.equal(second.elements[0].label, "Control 250"); + assert.equal(await evaluate('document.querySelector(\'[data-cu-idx="0"]\').innerText'), "Control 0"); + assert.equal(await evaluate('document.querySelectorAll(\'[data-cu-idx="250"]\').length'), 1); + await evaluate('document.querySelector(\'[data-cu-idx="250"]\').onclick = () => document.title = "clicked page two"; true'); + assert.equal((await evaluate(click(second.elements[0].i))).ok, true); + assert.equal(await evaluate("document.title"), "clicked page two"); + const last = await snapshot({ offset: second.nextOffset }); + assert.equal(last.elements.length, 20); + assert.equal(last.truncated, false); + assert.equal(last.nextOffset, null); + assert.equal((await snapshot({ offset: 1000 })).elements.length, 0); + await evaluate('document.querySelector(\'[data-cu-idx="0"]\').hidden = true; true'); + await snapshot(); + assert.equal(await evaluate('document.querySelectorAll(\'button[hidden][data-cu-idx]\').length'), 0); + console.log("PASS bounded pagination, unique actionable global indices, stale-index cleanup"); +} finally { + socket?.close(); + if (chrome.pid && chrome.exitCode === null && chrome.signalCode === null) { + const exited = new Promise((resolve) => chrome.once("exit", resolve)); + chrome.kill(); + await exited; + } + // Windows child processes can release profile files a moment after exit. + fs.rmSync(profile, { recursive: true, force: true, maxRetries: 20, retryDelay: 100 }); +} diff --git a/windows-linux/src/browser.rs b/windows-linux/src/browser.rs index 574e224..d6a4d76 100644 --- a/windows-linux/src/browser.rs +++ b/windows-linux/src/browser.rs @@ -657,7 +657,7 @@ fn normalise(_command: &str, args: &Value) -> Value { if let Some(session) = args.get("session_id").filter(|v| !v.is_null()) { map.insert("sessionId".into(), session.clone()); } - for key in ["url", "text", "key", "index", "x", "y", "all", "query", "offset", "fields", "reason"] { + for key in ["url", "text", "key", "index", "x", "y", "all", "query", "offset", "limit", "fields", "reason"] { if let Some(value) = args.get(key) { map.insert(key.into(), value.clone()); } @@ -924,6 +924,17 @@ fn describe_snapshot(result: &Value) -> String { result.get("title").and_then(Value::as_str).unwrap_or("?"), result.get("url").and_then(Value::as_str).unwrap_or("") )]; + if let Some(total) = result.get("total").and_then(Value::as_u64) { + let offset = result.get("offset").and_then(Value::as_u64).unwrap_or(0); + let limit = result.get("limit").and_then(Value::as_u64).unwrap_or(250); + let scope = result.get("scope").and_then(Value::as_str).unwrap_or("page"); + lines.push(format!("scope: {scope}; offset={offset}, limit={limit}, total={total}")); + if result.get("truncated").and_then(Value::as_bool) == Some(true) { + if let Some(next) = result.get("nextOffset").and_then(Value::as_u64) { + lines.push(format!("truncated — continue with browser_snapshot offset={next}, limit={limit} (same tab_id/session_id; unchanged UI keeps indices)")); + } + } + } for element in result .get("elements") .and_then(Value::as_array) @@ -1206,6 +1217,24 @@ mod tests { assert!(!rendered.contains("removed the tab group"), "{rendered}"); } + #[test] + fn snapshot_pagination_is_forwarded_and_described() { + let params = normalise("snapshot", &json!({"tab_id": 7, "offset": 250, "limit": 20})); + assert_eq!(params["offset"], 250); + assert_eq!(params["limit"], 20); + let result = json!({ + "title": "Many controls", "url": "https://example.com/", "scope": "modal", + "offset": 250, "limit": 20, "total": 300, "truncated": true, "nextOffset": 270, + "elements": [{"i": 250, "tag": "button", "label": "Next", "inView": true}] + }); + let text = describe_snapshot(&result); + assert!(text.contains("scope: modal; offset=250, limit=20, total=300")); + assert!(text.contains("truncated — continue with browser_snapshot offset=270, limit=20")); + assert!(text.contains("[250] button \"Next\"")); + let last = describe_snapshot(&json!({"total": 300, "offset": 300, "truncated": false})); + assert!(!last.contains("continue with")); + } + #[test] fn snapshots_flag_offscreen_elements() { let rendered = describe_snapshot(&json!({ diff --git a/windows-linux/src/tools.rs b/windows-linux/src/tools.rs index 8d8a4d8..9e24b24 100644 --- a/windows-linux/src/tools.rs +++ b/windows-linux/src/tools.rs @@ -788,7 +788,7 @@ fn all_tool_defs() -> Value { }, { "name": "browser_snapshot", - "description": "List the interactive elements (links, buttons, inputs) on the page in one of the agent's tabs, with the index each one has for browser_click, plus the page title and URL. Inputs show their type, such as input[password]. Works on a background tab, so the user can be looking at something else. Use it before every browser_click, because indices change when the page changes; use browser_read for the page's text. Read-only.", + "description": "List the interactive elements (links, buttons, inputs) on the page in one of the agent's tabs, with the index each one has for browser_click, plus the page title and URL. Inputs show their type, such as input[password]. Works on a background tab, so the user can be looking at something else. Use it before every browser_click, because indices change when the page changes; use browser_read for the page's text. Read-only. Automatically scopes to the topmost visible dialog when one is open, excluding hidden/inert controls but retaining reachable offscreen controls. Results report scope, total and truncation. Continue with offset and limit on the same tab/session; indices stay global across pages of an unchanged UI. Snapshot again after UI changes.", "inputSchema": { "type": "object", "properties": { @@ -801,6 +801,18 @@ fn all_tool_defs() -> Value { "tab_id": { "type": "integer", "description": "tab_id of one of the agent's tabs, from browser_open_tab or browser_list_tabs" + }, + "offset": { + "type": "integer", + "minimum": 0, + "maximum": 2147483647, + "description": "Control offset for pagination (default 0). Use the continuation offset from the previous result on an unchanged UI." + }, + "limit": { + "type": "integer", + "minimum": 1, + "maximum": 250, + "description": "Maximum controls returned per page (default 250, maximum 250)." } }, "required": ["tab_id"] From 7cb9d81da973da17c443fdbf57f28e159a3122c7 Mon Sep 17 00:00:00 2001 From: Elliot Drel Date: Tue, 6 Oct 2026 18:24:50 -0400 Subject: [PATCH 2/6] fix(browser): expose bounded SPA readiness and safe re-observation Wait for an expected visible CSS selector on state-returning actions or read-only snapshots instead of treating a loaded tab as application readiness. Preserve action success separately from readiness met, timeout, error, or not_requested. Forward the contract and render status consistently in Rust and Swift, with site-policy checks on every observation. Add a real Chromium regression proven to fail on deb4c7a, delayed-dialog MCP integration checks, contract tests, and safe retry documentation. Verified Windows and Linux Rust suites, 16 full browser integration checks, extension/build tests, and tool schema parity. --- README.md | 65 ++++++++++++- chrome-extension/background.js | 96 +++++++++++++++---- chrome-extension/background.test.mjs | 111 ++++++++++++++++++++++ macos/Sources/main.swift | 92 +++++++++++++++++-- scripts/e2e-browser.mjs | 40 ++++++++ scripts/spa-readiness.test.mjs | 132 +++++++++++++++++++++++++++ windows-linux/src/browser.rs | 65 ++++++++++++- windows-linux/src/tools.rs | 70 +++++++++++++- 8 files changed, 635 insertions(+), 36 deletions(-) create mode 100644 scripts/spa-readiness.test.mjs diff --git a/README.md b/README.md index 18e1f58..aca6f65 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,7 @@ Also looked at: [mediar-ai/mcp-server-macos-use](https://github.com/mediar-ai/mc - **Parallel tasks, one extension.** Every MCP process gets its own tab group, and one process can run several tasks by passing a stable `session_id` on its browser calls. A task cannot drive or adopt another task's tabs, and cleanup (including a process exiting) closes only its own. Any number of MCP processes share the one extension: the first owns it and the rest go through it, and if the owner exits another takes over without closing anyone's tabs. Tasks share Chrome's cookies and logins, and desktop apps and the clipboard are not isolated. - **Model-agnostic.** No vision model is required for interaction; local models work too. - **Look → act → verify.** `hover` for mouse-over menus, `wait` for loads, `query` to find a control by label without reading a whole tree. -- **Act and look in one call.** Pass `return_state: true` to any action (`click`, `type_text`, `set_value`, `browser_click`, `browser_navigate`, …) and the result carries the app's fresh accessibility tree, or the page's fresh snapshot, taken once the UI has settled. That halves the round trips of a look-act-verify loop. `state_query` narrows the desktop tree the same way `query` does. +- **Act and look in one call.** Pass `return_state: true` to any action (`click`, `type_text`, `set_value`, `browser_click`, `browser_navigate`, …) and the result carries the app's fresh accessibility tree, or the page's current snapshot. Browser actions report readiness separately: a loaded tab is not proof that delayed SPA UI has rendered. Use `wait_for_selector` for an expected visible element (see below). `state_query` narrows the desktop tree the same way `query` does. - **Pages as text.** `browser_read` returns a page's readable text with its headings, including what is scrolled out of view, from a tab in the background. It comes in chunks you can continue with `offset`, `query` keeps only the matching lines under their heading, and `include_links` lists the links. - **Passwords stay out of the conversation.** `browser_request_credentials` asks the user to sign in in a small Chrome window that shows the site's real origin. What they type goes straight into the page's fields and is never returned to the model. `browser_snapshot` never shows a password field's value, and on the desktop, typing into password fields is refused by default. - **Pick up what you are looking at.** `get_app_state` and `screenshot` take `app: "frontmost"` for the app in front of the user, so a client can hand the agent "this window" in one step. @@ -198,7 +198,68 @@ This is a guard rail for an agent that follows its instructions, not a sandbox. | Clipboard | `clipboard_read`, `clipboard_write` (plain text) | | Browser | `browser_open_tab`, `browser_list_tabs`, `browser_use_tab`, `browser_release_tab`, `browser_select_tab`, `browser_navigate`, `browser_snapshot`, `browser_read`, `browser_click`, `browser_type`, `browser_request_credentials`, `browser_press_key`, `browser_close_tab`, `browser_close_all_tabs` | -Every action tool also takes `return_state`, which returns the state after the action in the same call. +Every action tool also takes `return_state`, which returns current state after the action in the same call. + +### Browser readiness and safe re-observation + +`browser_click`, `browser_type`, `browser_press_key`, and `browser_navigate` accept +`wait_for_selector` together with `return_state: true`. `browser_snapshot` accepts +the same wait **without performing an action**: + +```json +{"tab_id": 42, "index": 0, "return_state": true, + "wait_for_selector": "[role=dialog] button", "wait_timeout_ms": 3000} +``` + +The extension polls for any matching visible element in the top-level document, +including same-document rendering after the tab reports `complete`. It uses the +snapshot's geometric visibility rule (at least 2×2 CSS pixels and not CSS-hidden; +not a guarantee of clickability, lack of occlusion, or opacity). Frames and shadow +roots are not searched. Choose a selector specific to the **new expected UI**; +an element already present can satisfy it immediately. No arbitrary page script +is accepted. + +- `wait_timeout_ms`: defaults to `8000`, accepts integers `0`–`10000`; `0` checks + once. Requires `wait_for_selector`. The budget covers polling, not browser/API + latency or user site-approval prompts. Site rules are rechecked before every + observation and before the final snapshot. +- `readiness: met (visible_selector, … ms)`: the specified condition was observed, + not a guarantee that all network requests, animations, or application work ended. +- `readiness: timeout`: the condition was not observed within the budget. The + action's success remains separate, and the current snapshot is still returned. +- `readiness: error`: the condition could not be evaluated (for example invalid + CSS); the action can still have succeeded. Correct the wait and re-observe. +- Without a selector, `return_state` retains the short pause and bounded + navigation-load wait, but reports `not_requested (document_load, … ms)` rather + than claiming SPA readiness. A navigation-load budget expiry reports `timeout`. + A plain `browser_snapshot` without a wait remains an immediate observation. + +If a returned snapshot looks unchanged, **do not repeat a non-idempotent click** +just because expected UI is absent. Wait/re-observe first: + +```json +{"tab_id": 42, "wait_for_selector": "[role=dialog] button", "wait_timeout_ms": 3000} +``` + +Send this to `browser_snapshot`, inspect its readiness and fresh indices, then +choose the next action. A timeout or tool error does not roll back a click, +submission, typing, or navigation. Use current state to decide whether a retry is +safe. Invalid wait types/bounds or an action wait without `return_state: true` are +rejected before acting. These options require an updated extension as well as the +native server; older extensions may omit readiness metadata. + +Regression checks: `node --test chrome-extension/background.test.mjs` covers the +wait contract and site-policy guards. The real-browser CI harness +`scripts/e2e-browser.mjs` tests a dialog delayed by 1100 ms without navigation, +one click only, plus timeout and read-only recovery. The standalone Chromium +regression can use an existing Playwright installation without adding a required +dependency: + +```bash +PLAYWRIGHT_MODULE=/absolute/path/to/playwright/index.mjs node --test scripts/spa-readiness.test.mjs +# Same assertions against the pre-fix source; expected to fail: +SOURCE_REF=deb4c7a PLAYWRIGHT_MODULE=/absolute/path/to/playwright/index.mjs node --test scripts/spa-readiness.test.mjs +``` `browser_snapshot` automatically scopes to the topmost visible dialog when one is open, so covered background controls cannot consume the dialog's budget. Hidden diff --git a/chrome-extension/background.js b/chrome-extension/background.js index 63dbd96..6fdabdf 100644 --- a/chrome-extension/background.js +++ b/chrome-extension/background.js @@ -1162,30 +1162,86 @@ async function unmarkTab(tabId) { const sleep = (ms) => new Promise((resume) => setTimeout(resume, ms)); -/** - * Wait for the page to react to an action before snapshotting it for - * `return_state`: a short pause for handlers to run, then — if the action - * started a navigation — until the tab finishes loading (bounded). - */ +/** Validate the wait contract before an action can have side effects. */ +function validateReadiness(p, action = false) { + if (p.waitForSelector === undefined && p.waitTimeoutMs === undefined) return; + if (typeof p.waitForSelector !== "string" || !p.waitForSelector.trim() || p.waitForSelector.length > 2000) { + throw new Error("wait_for_selector must be a nonempty CSS selector (at most 2000 characters)"); + } + if (action && p.returnState !== true) throw new Error("wait_for_selector requires return_state=true on actions"); + if (p.waitTimeoutMs !== undefined && (!Number.isInteger(p.waitTimeoutMs) || p.waitTimeoutMs < 0 || p.waitTimeoutMs > 10000)) { + throw new Error("wait_timeout_ms must be an integer from 0 to 10000"); + } +} + +/** A visible match in the top-level document, not arbitrary application readiness. */ +function visibleSelectorInPage(selector) { + let matches; + try { + matches = document.querySelectorAll(selector); + } catch { + return { error: "invalid CSS selector" }; + } + return { + met: [...matches].some((el) => { + const rect = el.getBoundingClientRect(); + const style = getComputedStyle(el); + return ( + rect.width >= 2 && rect.height >= 2 && style.visibility !== "hidden" && + style.visibility !== "collapse" && style.display !== "none" + ); + }), + }; +} + +/** Poll an explicit condition, including delayed same-document rendering. */ +async function waitForReadiness(p) { + validateReadiness(p); + const start = Date.now(); + const deadline = start + (p.waitTimeoutMs ?? 8000); + const outcome = (status, extra = {}) => ({ + status, condition: "visible_selector", selector: p.waitForSelector, + elapsedMs: Date.now() - start, ...extra, + }); + while (true) { + // Recheck on every observation: the action or a redirect may leave the + // allowed site during the wait. Never evaluate selectors on a blocked page. + await checkTab(requireClientId(p), p.tabId, p.sites, "read"); + const tab = await chrome.tabs.get(p.tabId); + if (tab.status !== "loading") { + const res = await send(p.tabId, "Runtime.evaluate", { + expression: `(${visibleSelectorInPage.toString()})(${JSON.stringify(p.waitForSelector)})`, + returnByValue: true, + }); + if (res?.exceptionDetails) return outcome("error", { error: res.exceptionDetails.text || "evaluate failed" }); + const value = res?.result?.value; + if (value?.error) return outcome("error", { error: value.error }); + if (value?.met === true) return outcome("met"); + } + if (Date.now() >= deadline) return outcome("timeout"); + await sleep(Math.min(100, deadline - Date.now())); + } +} + +/** Navigation settling is only a heuristic; tab.complete says nothing about SPAs. */ async function settle(tabId, pauseMs = 350, loadBudgetMs = 8000) { + const start = Date.now(); await sleep(pauseMs); const deadline = Date.now() + loadBudgetMs; - while (Date.now() < deadline) { - const tab = await chrome.tabs.get(tabId).catch(() => null); - if (!tab || tab.status !== "loading") return; - await sleep(150); + while (true) { + const tab = await chrome.tabs.get(tabId); + if (tab.status !== "loading") return { status: "not_requested", condition: "document_load", elapsedMs: Date.now() - start }; + if (Date.now() >= deadline) return { status: "timeout", condition: "document_load", elapsedMs: Date.now() - start }; + await sleep(Math.min(150, deadline - Date.now())); } } -/** Attach a fresh snapshot to an action's result when the caller asked for one. */ +/** Attach current state and separate action success from readiness/timeout. */ async function withState(p, result) { if (p.returnState !== true) return result; - await settle(p.tabId); - // The action may have left the page it was allowed on (a link, a redirect, - // a submitted form): apply the site rules to where the tab is now before - // reading it, as browser_snapshot does. + const readiness = p.waitForSelector !== undefined ? await waitForReadiness(p) : await settle(p.tabId); await checkTab(requireClientId(p), p.tabId, p.sites, "read"); - return { ...result, snapshot: await snapshot(p.tabId) }; + return { ...result, readiness, snapshot: await snapshot(p.tabId) }; } /** @@ -1594,16 +1650,21 @@ const handlers = { return { closed: p.tabId }; }, navigate: async (p) => { + validateReadiness(p, true); const clientId = requireClientId(p); assertOwned(clientId, p.tabId); await checkSite(clientId, p.url, p.sites, "open"); return withState(p, await navigate(p.tabId, p.url)); }, snapshot: async (p) => { + validateReadiness(p); const clientId = requireClientId(p); assertOwned(clientId, p.tabId); await checkTab(clientId, p.tabId, p.sites, "read"); - return snapshot(p.tabId, { offset: p.offset, limit: p.limit }); + const readiness = p.waitForSelector !== undefined ? await waitForReadiness(p) : undefined; + await checkTab(clientId, p.tabId, p.sites, "read"); + const state = await snapshot(p.tabId, { offset: p.offset, limit: p.limit }); + return readiness ? { ...state, readiness } : state; }, read: async (p) => { const clientId = requireClientId(p); @@ -1612,6 +1673,7 @@ const handlers = { return readPage(p.tabId, { query: p.query, maxChars: p.maxChars, offset: p.offset, includeLinks: p.includeLinks }); }, click: async (p) => { + validateReadiness(p, true); const clientId = requireClientId(p); assertOwned(clientId, p.tabId); await checkTab(clientId, p.tabId, p.sites, "click in"); @@ -1619,12 +1681,14 @@ const handlers = { return withState(p, result); }, type: async (p) => { + validateReadiness(p, true); const clientId = requireClientId(p); assertOwned(clientId, p.tabId); await checkTab(clientId, p.tabId, p.sites, "type in"); return withState(p, await typeText(p.tabId, p.text)); }, press: async (p) => { + validateReadiness(p, true); const clientId = requireClientId(p); assertOwned(clientId, p.tabId); await checkTab(clientId, p.tabId, p.sites, "press keys in"); diff --git a/chrome-extension/background.test.mjs b/chrome-extension/background.test.mjs index 5ec8404..411c9ef 100644 --- a/chrome-extension/background.test.mjs +++ b/chrome-extension/background.test.mjs @@ -567,9 +567,120 @@ test("return_state attaches a fresh snapshot to an action", async () => { assert.equal(plain.snapshot, undefined, "no snapshot unless asked"); const withState = await callAndWait("click", { tabId: pageTab, index: 3, returnState: true }); assert.equal(withState.snapshot.title, "After"); + assert.equal(withState.readiness.status, "not_requested", "tab.complete must not claim SPA readiness"); pageEval = () => ({ result: { value: {} } }); }); +test("every stateful action forwards an explicit wait and preserves its successful result", async () => { + for (const [command, args] of [["click", { index: 3 }], ["type", { text: "hello" }], ["press", { key: "Enter" }], ["navigate", { url: "https://shop.example/next" }]]) { + let observations = 0; + pageEval = (_method, params) => { + const expression = params.expression ?? ""; + if (expression.includes("visibleSelectorInPage")) { + observations++; + return { result: { value: { met: observations >= 2 } } }; + } + if (expression.includes("data-cu-idx=\"3\"")) return { result: { value: { ok: true, tag: "button", x: 5, y: 5 } } }; + if (expression.includes("querySelectorAll(sel)")) return { result: { value: { title: "Ready", elements: [] } } }; + return { result: { value: {} } }; + }; + const result = await callAndWait(command, { tabId: pageTab, ...args, returnState: true, waitForSelector: "#ready", waitTimeoutMs: 1000 }); + assert.equal(result.readiness.status, "met", command); + assert.equal(result.snapshot.title, "Ready", command); + assert.equal(observations, 2, command); + } + pageEval = () => ({ result: { value: {} } }); +}); + +test("snapshot waits without another action, including an immediate timeout", async () => { + let actions = 0; + pageEval = (method, params) => { + const expression = params.expression ?? ""; + if (method.startsWith("Input.") || expression.includes("data-cu-idx=\"")) actions++; + if (expression.includes("visibleSelectorInPage")) return { result: { value: { met: false } } }; + return { result: { value: { title: "Current", elements: [] } } }; + }; + const result = await callAndWait("snapshot", { tabId: pageTab, waitForSelector: "#missing", waitTimeoutMs: 0 }); + assert.equal(result.readiness.status, "timeout"); + assert.equal(result.title, "Current"); + assert.equal(actions, 0); + pageEval = () => ({ result: { value: {} } }); +}); + +test("invalid wait contracts are rejected before any stateful action", async () => { + let sideEffects = 0; + const before = tabs.get(pageTab).url; + pageEval = () => { sideEffects++; return { result: { value: {} } }; }; + const invalid = [ + { waitForSelector: "#ready" }, + { returnState: true, waitForSelector: "" }, + { returnState: true, waitForSelector: 1 }, + { returnState: true, waitForSelector: "x".repeat(2001) }, + { returnState: true, waitTimeoutMs: 100 }, + ...[-1, 10001, 0.5, "100", null].map((waitTimeoutMs) => ({ returnState: true, waitForSelector: "#ready", waitTimeoutMs })), + ]; + for (const command of ["click", "type", "press", "navigate"]) { + for (const params of invalid) { + const error = await refuses(command, { tabId: pageTab, index: 3, text: "x", key: "Enter", url: "https://changed.example", ...params }); + assert.match(error, /wait_for_selector|wait_timeout_ms/); + } + } + assert.equal(sideEffects, 0); + assert.equal(tabs.get(pageTab).url, before); + pageEval = () => ({ result: { value: {} } }); +}); + +test("a readiness evaluation error is not reported as a failed click", async () => { + let clicks = 0; + pageEval = (_method, params) => { + const expression = params.expression ?? ""; + if (expression.includes("data-cu-idx=\"3\"")) { + clicks++; + return { result: { value: { ok: true, x: 5, y: 5 } } }; + } + if (expression.includes("visibleSelectorInPage")) return { result: { value: { error: "invalid CSS selector" } } }; + return { result: { value: { title: "Current", elements: [] } } }; + }; + const result = await callAndWait("click", { tabId: pageTab, index: 3, returnState: true, waitForSelector: "[" }); + assert.equal(result.ok, true); + assert.equal(result.readiness.status, "error"); + assert.equal(result.readiness.error, "invalid CSS selector"); + assert.equal(clicks, 1); + pageEval = () => ({ result: { value: {} } }); +}); + +test("an explicit wait rechecks site rules before each selector observation", async () => { + const tab = tabs.get(pageTab); + const before = tab.url; + let observations = 0; + pageEval = (_method, params) => { + if ((params.expression ?? "").includes("visibleSelectorInPage")) { + observations++; + tab.url = "https://bank.example/accounts"; + return { result: { value: { met: false } } }; + } + return { result: { value: {} } }; + }; + const error = await callAndWait("snapshot", { tabId: pageTab, waitForSelector: "#ready", waitTimeoutMs: 1000, sites: [{ pattern: "bank.example", rule: "block" }] }).then( + () => "expected refusal", (failure) => failure.message, + ); + assert.match(error, /blocked by the user's Computer Use policy/); + assert.equal(observations, 1, "a selector was evaluated after the redirect was blocked"); + tab.url = before; + pageEval = () => ({ result: { value: {} } }); +}); + +test("navigation loading is bounded and is not confused with an explicit condition", async () => { + const tab = tabs.get(pageTab); + tab.status = "loading"; + const start = Date.now(); + const result = await vm.runInContext(`settle(${pageTab}, 0, 25)`, extensionContext); + assert.equal(result.status, "timeout"); + assert.equal(result.condition, "document_load"); + assert.ok(Date.now() - start < 500); + tab.status = "complete"; +}); + test("a click index that is not a plain number never reaches the page as script", async () => { const evaluated = []; pageEval = (method, params) => { diff --git a/macos/Sources/main.swift b/macos/Sources/main.swift index 54215c6..a14e909 100644 --- a/macos/Sources/main.swift +++ b/macos/Sources/main.swift @@ -2451,7 +2451,7 @@ func browserCallTimeout(_ command: String, _ args: [String: Any], sitesAsk: Bool } // The extension's site approval prompt waits up to two minutes. if sitesAsk { timeout = max(timeout, 120 + promptGrace) } - if args["return_state"] as? Bool == true { timeout += 10 } + if args["return_state"] as? Bool == true || args["wait_for_selector"] != nil { timeout += 15 } return timeout } @@ -2459,6 +2459,8 @@ func browserCallTimeout(_ command: String, _ args: [String: Any], sitesAsk: Bool func withReturnState(_ params: [String: Any], _ args: [String: Any]) -> [String: Any] { var params = params if args["return_state"] as? Bool == true { params["returnState"] = true } + if let selector = args["wait_for_selector"] { params["waitForSelector"] = selector } + if let timeout = args["wait_timeout_ms"] { params["waitTimeoutMs"] = timeout } return params } @@ -2466,7 +2468,19 @@ func withReturnState(_ params: [String: Any], _ args: [String: Any]) -> [String: /// extension sent one back (return_state). func withPageAfter(_ line: String, _ payload: [String: Any]) -> String { guard let snapshot = payload["snapshot"] as? [String: Any] else { return line } - return line + "\n\npage after the action (earlier indices are no longer valid):\n" + describeSnapshot(snapshot) + return line + "\n\n" + describeReadiness(payload) + "page after the action (earlier indices are no longer valid):\n" + describeSnapshot(snapshot) +} + +/// Readiness is separate from action success; never imply a retry is safe. +func describeReadiness(_ payload: [String: Any]) -> String { + guard let readiness = payload["readiness"] as? [String: Any] else { return "" } + let status = readiness["status"] as? String ?? "unknown" + let condition = readiness["condition"] as? String ?? "unknown" + let elapsed = readiness["elapsedMs"] as? Int ?? 0 + var line = "readiness: \(status) (\(condition), \(elapsed) ms)" + if let selector = readiness["selector"] as? String { line += " selector=\(selector)" } + if let error = readiness["error"] as? String { line += " — \(error)" } + return line + "; only the stated condition was checked, not general app readiness. Re-observe with browser_snapshot before repeating a non-idempotent action.\n" } func toolBrowserOpenTab(_ args: [String: Any]) -> String { @@ -2647,7 +2661,9 @@ func toolBrowserSnapshot(_ args: [String: Any]) -> String { for key in ["offset", "limit"] { if let value = args[key] { params[key] = value } } - return bridgeText(browserSessionCall("snapshot", params, args: args), describeSnapshot) + return bridgeText(browserSessionCall("snapshot", withReturnState(params, args), args: args)) { payload in + describeReadiness(payload) + describeSnapshot(payload) + } } /// browser_read: the page text, and where to continue if it was cut off. @@ -3649,6 +3665,18 @@ let toolDefs: [[String: Any]] = [ "maximum": 250, "description": "Maximum controls returned per page (default 250, maximum 250).", ], + "wait_for_selector": [ + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI.", + ], + "wait_timeout_ms": [ + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action.", + ], ], "required": ["tab_id"], ], @@ -3733,7 +3761,19 @@ let toolDefs: [[String: Any]] = [ ], "return_state": [ "type": "boolean", - "description": "After acting, wait for the page to settle (and finish loading, if the action navigated) and append a fresh browser_snapshot of this tab, so you can pick the next index in the same call. Its indices replace earlier ones.", + "description": "Append a current snapshot and readiness status after acting. Without wait_for_selector, only a short pause and bounded navigation wait are used; SPA readiness is not guaranteed. Snapshot indices replace earlier ones. A timeout does not mean the action failed: re-observe with browser_snapshot before repeating it.", + ], + "wait_for_selector": [ + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI.", + ], + "wait_timeout_ms": [ + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action.", ], ], "required": ["tab_id"], @@ -3768,7 +3808,19 @@ let toolDefs: [[String: Any]] = [ ], "return_state": [ "type": "boolean", - "description": "After acting, wait for the page to settle (and finish loading, if the action navigated) and append a fresh browser_snapshot of this tab, so you can pick the next index in the same call. Its indices replace earlier ones.", + "description": "Append a current snapshot and readiness status after acting. Without wait_for_selector, only a short pause and bounded navigation wait are used; SPA readiness is not guaranteed. Snapshot indices replace earlier ones. A timeout does not mean the action failed: re-observe with browser_snapshot before repeating it.", + ], + "wait_for_selector": [ + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI.", + ], + "wait_timeout_ms": [ + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action.", ], ], "required": ["tab_id", "text"], @@ -3858,7 +3910,19 @@ let toolDefs: [[String: Any]] = [ ], "return_state": [ "type": "boolean", - "description": "After acting, wait for the page to settle (and finish loading, if the action navigated) and append a fresh browser_snapshot of this tab, so you can pick the next index in the same call. Its indices replace earlier ones.", + "description": "Append a current snapshot and readiness status after acting. Without wait_for_selector, only a short pause and bounded navigation wait are used; SPA readiness is not guaranteed. Snapshot indices replace earlier ones. A timeout does not mean the action failed: re-observe with browser_snapshot before repeating it.", + ], + "wait_for_selector": [ + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI.", + ], + "wait_timeout_ms": [ + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action.", ], ], "required": ["tab_id", "key"], @@ -3915,7 +3979,19 @@ let toolDefs: [[String: Any]] = [ ], "return_state": [ "type": "boolean", - "description": "After acting, wait for the page to settle (and finish loading, if the action navigated) and append a fresh browser_snapshot of this tab, so you can pick the next index in the same call. Its indices replace earlier ones.", + "description": "Append a current snapshot and readiness status after acting. Without wait_for_selector, only a short pause and bounded navigation wait are used; SPA readiness is not guaranteed. Snapshot indices replace earlier ones. A timeout does not mean the action failed: re-observe with browser_snapshot before repeating it.", + ], + "wait_for_selector": [ + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI.", + ], + "wait_timeout_ms": [ + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action.", ], ], "required": ["tab_id", "url"], @@ -3960,7 +4036,7 @@ func negotiatedProtocolVersion(_ params: [String: Any]) -> String { } /// Returned in the `initialize` result; identical in the Rust server. -let serverInstructions = "Munim Computer Use operates this computer's desktop apps and, through the browser_* tools, the user's signed-in Chrome. Look, act, verify: call list_apps to find the app, then get_app_state (narrow it with query) before acting, and act on element ids such as e12 rather than screen coordinates. Ids belong to one snapshot, so call get_app_state again after the UI changes. Use screenshot to check a result or to see content the accessibility tree cannot describe, and zoom to read small text. Where the platform allows, input is delivered to the target app in the background and the agent has its own pointer, so the user can keep working; call activate_app only when a keystroke needs keyboard focus. For web pages prefer the browser_* tools, which work in the agent's own tab group, and release any tab adopted with browser_use_tab when done. For concurrent tasks sharing this MCP server, pass a distinct session_id on every browser call for each task; keep it stable, including cleanup. Separate sessions share website logins and cookies. Desktop apps and clipboard are not session-isolated. Pass return_state on an action to get the updated state back in the same call instead of reading again. Use browser_read to read a page's text. Never ask for, or type, a password or code yourself: browser_request_credentials lets the user enter it without you seeing it. The user's Computer Use policy can block apps and sites or require their approval; when a call says so, do not work around it. Ask the user before anything irreversible, such as sending, deleting, purchasing or submitting forms on their behalf." +let serverInstructions = "Munim Computer Use operates this computer's desktop apps and, through the browser_* tools, the user's signed-in Chrome. Look, act, verify: call list_apps to find the app, then get_app_state (narrow it with query) before acting, and act on element ids such as e12 rather than screen coordinates. Ids belong to one snapshot, so call get_app_state again after the UI changes. Use screenshot to check a result or to see content the accessibility tree cannot describe, and zoom to read small text. Where the platform allows, input is delivered to the target app in the background and the agent has its own pointer, so the user can keep working; call activate_app only when a keystroke needs keyboard focus. For web pages prefer the browser_* tools, which work in the agent's own tab group, and release any tab adopted with browser_use_tab when done. For concurrent tasks sharing this MCP server, pass a distinct session_id on every browser call for each task; keep it stable, including cleanup. Separate sessions share website logins and cookies. Desktop apps and clipboard are not session-isolated. Pass return_state on an action to get current state back in the same call. For delayed browser UI, use wait_for_selector with return_state and inspect readiness; a loaded tab is not SPA readiness. A timeout does not undo the action: re-observe with browser_snapshot and the same wait before repeating a non-idempotent action. Use browser_read to read a page's text. Never ask for, or type, a password or code yourself: browser_request_credentials lets the user enter it without you seeing it. The user's Computer Use policy can block apps and sites or require their approval; when a call says so, do not work around it. Ask the user before anything irreversible, such as sending, deleting, purchasing or submitting forms on their behalf." // MARK: - Clipboard diff --git a/scripts/e2e-browser.mjs b/scripts/e2e-browser.mjs index e40b26e..36dd399 100644 --- a/scripts/e2e-browser.mjs +++ b/scripts/e2e-browser.mjs @@ -67,6 +67,18 @@ const article = [ 'Next page', ].join("\n"); const pages = { + "/spa": `Delayed dialog`, "/article": `Article${article}`, "/login": `Sign in
@@ -362,6 +374,34 @@ check("browser_click with return_state comes back with the next page", async () assert.match(text, /button "Continue"/); }); +check("return_state waits for a delayed SPA dialog after exactly one click", async () => { + await ok("browser_navigate", { tab_id: tab, url: `${base}/spa`, return_state: true }); + const snapshot = await ok("browser_snapshot", { tab_id: tab }); + const index = Number(/\[(\d+)\] button "Open dialog"/.exec(snapshot)[1]); + const text = await ok("browser_click", { + tab_id: tab, index, return_state: true, wait_for_selector: "[role=dialog] button", wait_timeout_ms: 3000, + }); + assert.match(text, /readiness: met \(visible_selector/); + assert.match(text, /button "Next"/); + const counts = await evaluateIn((url) => url === `${base}/spa`, "[window.clicks, document.querySelectorAll('[role=dialog]').length, location.href]"); + assert.deepEqual(counts, [1, 1, `${base}/spa`]); +}); + +check("a timed-out SPA observation can be waited on without clicking again", async () => { + await ok("browser_navigate", { tab_id: tab, url: `${base}/spa`, return_state: true }); + const snapshot = await ok("browser_snapshot", { tab_id: tab }); + const index = Number(/\[(\d+)\] button "Open dialog"/.exec(snapshot)[1]); + const text = await ok("browser_click", { + tab_id: tab, index, return_state: true, wait_for_selector: "[role=dialog] button", wait_timeout_ms: 0, + }); + assert.match(text, /clicked in tab/); + assert.match(text, /readiness: timeout/); + const observed = await ok("browser_snapshot", { tab_id: tab, wait_for_selector: "[role=dialog] button", wait_timeout_ms: 3000 }); + assert.match(observed, /readiness: met/); + assert.match(observed, /button "Next"/); + assert.deepEqual(await evaluateIn((url) => url === `${base}/spa`, "[window.clicks, document.querySelectorAll('[role=dialog]').length]"), [1, 1]); +}); + check("browser_navigate with return_state waits for the load", async () => { const text = await ok("browser_navigate", { tab_id: tab, url: `${base}/login`, return_state: true }); assert.match(text, /Sign in {2}\[/); diff --git a/scripts/spa-readiness.test.mjs b/scripts/spa-readiness.test.mjs new file mode 100644 index 0000000..2613cf9 --- /dev/null +++ b/scripts/spa-readiness.test.mjs @@ -0,0 +1,132 @@ +// Real Chromium regression for #19, without installing a runtime dependency. +// PLAYWRIGHT_MODULE=/absolute/path/to/playwright/index.mjs node --test scripts/spa-readiness.test.mjs +// SOURCE_REF=deb4c7a runs the same assertions against the original implementation. +import assert from "node:assert/strict"; +import fs from "node:fs"; +import vm from "node:vm"; +import { execFileSync } from "node:child_process"; +import { pathToFileURL } from "node:url"; +import { test } from "node:test"; + +const source = process.env.SOURCE_REF + ? execFileSync("git", ["show", `${process.env.SOURCE_REF}:chrome-extension/background.js`], { encoding: "utf8" }) + : fs.readFileSync(new URL("../chrome-extension/background.js", import.meta.url), "utf8"); +const between = (start, end) => { + const normalized = source.replace(/\r\n/g, "\n"); + const first = normalized.indexOf(start); + const last = normalized.indexOf(end, first); + assert.ok(first >= 0 && last > first, `missing source boundary: ${start} / ${end}`); + return normalized.slice(first, last); +}; + +async function fixture(run) { + assert.ok(process.env.PLAYWRIGHT_MODULE, "Set PLAYWRIGHT_MODULE to an existing Playwright index.mjs"); + const { chromium } = await import(pathToFileURL(process.env.PLAYWRIGHT_MODULE).href); + const browser = await chromium.launch({ headless: true }); + try { + const page = await browser.newPage(); + await page.setContent(``); + const context = vm.createContext({ + setTimeout, Date, + chrome: { tabs: { get: async () => ({ status: "complete" }) } }, + requireClientId: () => "test", + checkTab: async () => {}, + send: async (_tabId, _method, params) => { + try { return { result: { value: await page.evaluate(params.expression) } }; } + catch (error) { return { exceptionDetails: { text: error.message } }; } + }, + }); + vm.runInContext( + between("const SNAPSHOT_JS", "async function clickAt") + + between("const CLICK_JS", "/// Click a snapshotted") + + between("const sleep =", "/**\n * The page's readable text"), context, + ); + const evaluate = (expression) => vm.runInContext(expression, context); + const initial = await evaluate("snapshot(1)"); + const index = initial.elements.find((el) => el.label === "Open dialog").i; + assert.equal((await page.evaluate(evaluate(`CLICK_JS(${index})`))).ok, true); + await run({ page, evaluate }); + } finally { + await browser.close(); + } +} + +test("a single click waits for the delayed SPA dialog without navigation", async () => { + await fixture(async ({ page, evaluate }) => { + const start = Date.now(); + const result = await evaluate('withState({tabId:1,returnState:true,waitForSelector:"[role=dialog] button",waitTimeoutMs:3000},{ok:true})'); + const elapsed = Date.now() - start; + console.log(`post-click wait ${elapsed}ms; Next=${result.snapshot.elements.some((el) => el.label === "Next")}`); + assert.ok(result.snapshot.elements.some((el) => el.label === "Next"), "snapshot returned before the delayed dialog rendered"); + assert.equal(result.readiness.status, "met"); + assert.equal(result.readiness.condition, "visible_selector"); + assert.ok(elapsed >= 900 && elapsed < 3500); + assert.equal(await page.evaluate("window.clicks"), 1); + assert.equal(await page.locator("[role=dialog]").count(), 1); + assert.equal(page.url(), "about:blank"); + }); +}); + +test("timeout preserves the successful action; read-only re-observation never clicks again", async () => { + await fixture(async ({ page, evaluate }) => { + const result = await evaluate('withState({tabId:1,returnState:true,waitForSelector:"[role=dialog] button",waitTimeoutMs:100},{ok:true})'); + assert.equal(result.ok, true); + assert.equal(result.readiness.status, "timeout"); + assert.ok(!result.snapshot.elements.some((el) => el.label === "Next")); + const readiness = await evaluate('waitForReadiness({tabId:1,waitForSelector:"[role=dialog] button",waitTimeoutMs:3000})'); + assert.equal(readiness.status, "met"); + assert.ok((await evaluate("snapshot(1)")).elements.some((el) => el.label === "Next")); + assert.equal(await page.evaluate("window.clicks"), 1); + assert.equal(await page.locator("[role=dialog]").count(), 1); + }); +}); + +test("invalid selector syntax is a readiness error, not a failed or duplicated action", async () => { + await fixture(async ({ page, evaluate }) => { + const result = await evaluate('withState({tabId:1,returnState:true,waitForSelector:"[",waitTimeoutMs:100},{ok:true})'); + assert.equal(result.ok, true); + assert.equal(result.readiness.status, "error"); + assert.equal(result.readiness.error, "invalid CSS selector"); + assert.equal(await page.evaluate("window.clicks"), 1); + const malicious = '[id="x"]); window.injected = true; //'; + const readiness = await evaluate(`waitForReadiness({tabId:1,waitForSelector:${JSON.stringify(malicious)},waitTimeoutMs:0})`); + assert.equal(readiness.status, "error"); + assert.equal(await page.evaluate("window.injected"), undefined); + }); +}); + +test("an unconfigured action snapshot does not claim application readiness", async () => { + await fixture(async ({ evaluate }) => { + const result = await evaluate("withState({tabId:1,returnState:true},{ok:true})"); + assert.equal(result.ok, true); + assert.equal(result.readiness.status, "not_requested"); + assert.equal(result.readiness.condition, "document_load"); + }); +}); + +test("a hidden match does not satisfy visible-selector readiness", async () => { + await fixture(async ({ page, evaluate }) => { + await page.evaluate(() => { + const hidden = document.createElement("button"); + hidden.id = "ready"; + hidden.style.display = "none"; + document.body.append(hidden); + setTimeout(() => { hidden.style.display = "block"; }, 600); + }); + const start = Date.now(); + const readiness = await evaluate('waitForReadiness({tabId:1,waitForSelector:"#ready",waitTimeoutMs:2000})'); + assert.equal(readiness.status, "met"); + assert.ok(Date.now() - start >= 450); + }); +}); diff --git a/windows-linux/src/browser.rs b/windows-linux/src/browser.rs index d6a4d76..c903256 100644 --- a/windows-linux/src/browser.rs +++ b/windows-linux/src/browser.rs @@ -641,8 +641,8 @@ fn call_timeout(command: &str, args: &Value, sites_ask: bool) -> Duration { // The extension's site approval prompt waits up to two minutes. timeout = timeout.max(Duration::from_secs(120) + PROMPT_GRACE); } - if args.get("return_state").and_then(Value::as_bool) == Some(true) { - timeout += Duration::from_secs(10); + if args.get("return_state").and_then(Value::as_bool) == Some(true) || args.get("wait_for_selector").is_some() { + timeout += Duration::from_secs(15); } timeout } @@ -662,7 +662,13 @@ fn normalise(_command: &str, args: &Value) -> Value { map.insert(key.into(), value.clone()); } } - for (from, to) in [("max_chars", "maxChars"), ("include_links", "includeLinks"), ("return_state", "returnState")] { + for (from, to) in [ + ("max_chars", "maxChars"), + ("include_links", "includeLinks"), + ("return_state", "returnState"), + ("wait_for_selector", "waitForSelector"), + ("wait_timeout_ms", "waitTimeoutMs"), + ] { if let Some(value) = args.get(from) { map.insert(to.into(), value.clone()); } @@ -721,7 +727,7 @@ fn describe(command: &str, result: &Value, args: &Value) -> String { .unwrap_or(-1); format!("released tab {tab} back to the user") } - "snapshot" => describe_snapshot(result), + "snapshot" => format!("{}{}", describe_readiness(result), describe_snapshot(result)), "read" => describe_read(result), "request_credentials" => describe_credentials(result, args), "close_all_tabs" => { @@ -787,7 +793,8 @@ fn describe(command: &str, result: &Value, args: &Value) -> String { // return_state: the page as it stands after the action. match result.get("snapshot") { Some(snapshot) => format!( - "{line}\n\npage after the action (earlier indices are no longer valid):\n{}", + "{line}\n\n{}page after the action (earlier indices are no longer valid):\n{}", + describe_readiness(result), describe_snapshot(snapshot) ), None => line, @@ -796,6 +803,23 @@ fn describe(command: &str, result: &Value, args: &Value) -> String { } } +/// Readiness is separate from action success; never imply a retry is safe. +fn describe_readiness(result: &Value) -> String { + let Some(readiness) = result.get("readiness") else { return String::new() }; + let status = readiness.get("status").and_then(Value::as_str).unwrap_or("unknown"); + let condition = readiness.get("condition").and_then(Value::as_str).unwrap_or("unknown"); + let elapsed = readiness.get("elapsedMs").and_then(Value::as_u64).unwrap_or(0); + let mut line = format!("readiness: {status} ({condition}, {elapsed} ms)"); + if let Some(selector) = readiness.get("selector").and_then(Value::as_str) { + line.push_str(&format!(" selector={selector}")); + } + if let Some(error) = readiness.get("error").and_then(Value::as_str) { + line.push_str(&format!(" — {error}")); + } + line.push_str("; only the stated condition was checked, not general app readiness. Re-observe with browser_snapshot before repeating a non-idempotent action.\n"); + line +} + /// browser_read: the page text, and where to continue if it was cut off. fn describe_read(result: &Value) -> String { let text = result.get("text").and_then(Value::as_str).unwrap_or(""); @@ -1089,6 +1113,37 @@ mod tests { assert_eq!(params["timeoutMs"], json!(600_000), "the wait is capped"); } + #[test] + fn readiness_arguments_and_headroom_reach_every_stateful_browser_command() { + let args = json!({ "tab_id": 1, "return_state": true, "wait_for_selector": "[role=dialog] button", "wait_timeout_ms": 10000 }); + for command in ["click", "type", "press", "navigate", "snapshot"] { + let params = normalise(command, &args); + assert_eq!(params["waitForSelector"], args["wait_for_selector"]); + assert_eq!(params["waitTimeoutMs"], json!(10000)); + assert_eq!(call_timeout(command, &args, false), Duration::from_secs(35)); + } + assert_eq!(call_timeout("snapshot", &json!({ "wait_for_selector": "#ready" }), false), Duration::from_secs(35)); + // Invalid values are preserved for extension validation before acting. + assert_eq!(normalise("click", &json!({ "wait_timeout_ms": -1 }))["waitTimeoutMs"], json!(-1)); + } + + #[test] + fn readiness_status_survives_action_and_snapshot_text_rendering() { + for status in ["met", "timeout", "error", "not_requested"] { + let readiness = json!({ "status": status, "condition": "visible_selector", "selector": "#ready", "elapsedMs": 100, "error": "invalid CSS selector" }); + let snapshot = json!({ "title": "Current", "url": "https://s.example", "elements": [] }); + let action = describe("click", &json!({ "ok": true, "snapshot": snapshot, "readiness": readiness }), &json!({ "tab_id": 1 })); + assert!(action.starts_with("clicked in tab 1"), "{action}"); + let observation = describe("snapshot", &json!({ "title": "Current", "elements": [], "readiness": readiness }), &json!({})); + for text in [action, observation] { + assert!(text.contains(&format!("readiness: {status} (visible_selector, 100 ms)")), "{text}"); + assert!(text.contains("selector=#ready"), "{text}"); + assert!(text.contains("invalid CSS selector"), "{text}"); + assert!(text.contains("Re-observe with browser_snapshot"), "{text}"); + } + } + } + #[test] fn prompts_get_more_time_than_plain_commands() { assert_eq!(call_timeout("click", &json!({}), false), Duration::from_secs(20)); diff --git a/windows-linux/src/tools.rs b/windows-linux/src/tools.rs index 9e24b24..048931f 100644 --- a/windows-linux/src/tools.rs +++ b/windows-linux/src/tools.rs @@ -24,7 +24,7 @@ pub fn browser_control_enabled() -> bool { } /// Returned in the `initialize` result; identical in the Swift server. -pub const SERVER_INSTRUCTIONS: &str = "Munim Computer Use operates this computer's desktop apps and, through the browser_* tools, the user's signed-in Chrome. Look, act, verify: call list_apps to find the app, then get_app_state (narrow it with query) before acting, and act on element ids such as e12 rather than screen coordinates. Ids belong to one snapshot, so call get_app_state again after the UI changes. Use screenshot to check a result or to see content the accessibility tree cannot describe, and zoom to read small text. Where the platform allows, input is delivered to the target app in the background and the agent has its own pointer, so the user can keep working; call activate_app only when a keystroke needs keyboard focus. For web pages prefer the browser_* tools, which work in the agent's own tab group, and release any tab adopted with browser_use_tab when done. For concurrent tasks sharing this MCP server, pass a distinct session_id on every browser call for each task; keep it stable, including cleanup. Separate sessions share website logins and cookies. Desktop apps and clipboard are not session-isolated. Pass return_state on an action to get the updated state back in the same call instead of reading again. Use browser_read to read a page's text. Never ask for, or type, a password or code yourself: browser_request_credentials lets the user enter it without you seeing it. The user's Computer Use policy can block apps and sites or require their approval; when a call says so, do not work around it. Ask the user before anything irreversible, such as sending, deleting, purchasing or submitting forms on their behalf."; +pub const SERVER_INSTRUCTIONS: &str = "Munim Computer Use operates this computer's desktop apps and, through the browser_* tools, the user's signed-in Chrome. Look, act, verify: call list_apps to find the app, then get_app_state (narrow it with query) before acting, and act on element ids such as e12 rather than screen coordinates. Ids belong to one snapshot, so call get_app_state again after the UI changes. Use screenshot to check a result or to see content the accessibility tree cannot describe, and zoom to read small text. Where the platform allows, input is delivered to the target app in the background and the agent has its own pointer, so the user can keep working; call activate_app only when a keystroke needs keyboard focus. For web pages prefer the browser_* tools, which work in the agent's own tab group, and release any tab adopted with browser_use_tab when done. For concurrent tasks sharing this MCP server, pass a distinct session_id on every browser call for each task; keep it stable, including cleanup. Separate sessions share website logins and cookies. Desktop apps and clipboard are not session-isolated. Pass return_state on an action to get current state back in the same call. For delayed browser UI, use wait_for_selector with return_state and inspect readiness; a loaded tab is not SPA readiness. A timeout does not undo the action: re-observe with browser_snapshot and the same wait before repeating a non-idempotent action. Use browser_read to read a page's text. Never ask for, or type, a password or code yourself: browser_request_credentials lets the user enter it without you seeing it. The user's Computer Use policy can block apps and sites or require their approval; when a call says so, do not work around it. Ask the user before anything irreversible, such as sending, deleting, purchasing or submitting forms on their behalf."; pub fn tool_defs() -> Value { let Value::Array(defs) = all_tool_defs() else { @@ -813,6 +813,18 @@ fn all_tool_defs() -> Value { "minimum": 1, "maximum": 250, "description": "Maximum controls returned per page (default 250, maximum 250)." + }, + "wait_for_selector": { + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI." + }, + "wait_timeout_ms": { + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action." } }, "required": ["tab_id"] @@ -898,7 +910,19 @@ fn all_tool_defs() -> Value { }, "return_state": { "type": "boolean", - "description": "After acting, wait for the page to settle (and finish loading, if the action navigated) and append a fresh browser_snapshot of this tab, so you can pick the next index in the same call. Its indices replace earlier ones." + "description": "Append a current snapshot and readiness status after acting. Without wait_for_selector, only a short pause and bounded navigation wait are used; SPA readiness is not guaranteed. Snapshot indices replace earlier ones. A timeout does not mean the action failed: re-observe with browser_snapshot before repeating it." + }, + "wait_for_selector": { + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI." + }, + "wait_timeout_ms": { + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action." } }, "required": ["tab_id"] @@ -933,7 +957,19 @@ fn all_tool_defs() -> Value { }, "return_state": { "type": "boolean", - "description": "After acting, wait for the page to settle (and finish loading, if the action navigated) and append a fresh browser_snapshot of this tab, so you can pick the next index in the same call. Its indices replace earlier ones." + "description": "Append a current snapshot and readiness status after acting. Without wait_for_selector, only a short pause and bounded navigation wait are used; SPA readiness is not guaranteed. Snapshot indices replace earlier ones. A timeout does not mean the action failed: re-observe with browser_snapshot before repeating it." + }, + "wait_for_selector": { + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI." + }, + "wait_timeout_ms": { + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action." } }, "required": ["tab_id", "text"] @@ -1023,7 +1059,19 @@ fn all_tool_defs() -> Value { }, "return_state": { "type": "boolean", - "description": "After acting, wait for the page to settle (and finish loading, if the action navigated) and append a fresh browser_snapshot of this tab, so you can pick the next index in the same call. Its indices replace earlier ones." + "description": "Append a current snapshot and readiness status after acting. Without wait_for_selector, only a short pause and bounded navigation wait are used; SPA readiness is not guaranteed. Snapshot indices replace earlier ones. A timeout does not mean the action failed: re-observe with browser_snapshot before repeating it." + }, + "wait_for_selector": { + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI." + }, + "wait_timeout_ms": { + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action." } }, "required": ["tab_id", "key"] @@ -1080,7 +1128,19 @@ fn all_tool_defs() -> Value { }, "return_state": { "type": "boolean", - "description": "After acting, wait for the page to settle (and finish loading, if the action navigated) and append a fresh browser_snapshot of this tab, so you can pick the next index in the same call. Its indices replace earlier ones." + "description": "Append a current snapshot and readiness status after acting. Without wait_for_selector, only a short pause and bounded navigation wait are used; SPA readiness is not guaranteed. Snapshot indices replace earlier ones. A timeout does not mean the action failed: re-observe with browser_snapshot before repeating it." + }, + "wait_for_selector": { + "type": "string", + "minLength": 1, + "maxLength": 2000, + "description": "Wait for any visible element matching this CSS selector in the top-level document (not frames or shadow roots). On actions requires return_state=true. Returns readiness met, timeout or error; only this condition is checked, not general app readiness. Prefer a selector specific to the expected new UI." + }, + "wait_timeout_ms": { + "type": "integer", + "minimum": 0, + "maximum": 10000, + "description": "Bound for wait_for_selector in milliseconds (default 8000, maximum 10000; 0 checks once). Requires wait_for_selector. Timeout preserves action success and returns current state. Use browser_snapshot with the same wait to re-observe without repeating an action." } }, "required": ["tab_id", "url"] From ebe3edd7617ed03db25ff28ef89b7e0bbc8921cc Mon Sep 17 00:00:00 2001 From: Elliot Drel Date: Wed, 7 Oct 2026 10:17:38 -0400 Subject: [PATCH 3/6] fix(browser): share visibility rules and decouple readiness from resource loading --- README.md | 23 +-- chrome-extension/background.js | 65 ++++--- chrome-extension/background.test.mjs | 20 +++ scripts/e2e-browser.mjs | 5 +- scripts/snapshot-dom.test.mjs | 260 ++++++++++++++++++--------- scripts/spa-readiness.test.mjs | 132 -------------- 6 files changed, 248 insertions(+), 257 deletions(-) delete mode 100644 scripts/spa-readiness.test.mjs diff --git a/README.md b/README.md index aca6f65..560c260 100644 --- a/README.md +++ b/README.md @@ -212,9 +212,10 @@ the same wait **without performing an action**: ``` The extension polls for any matching visible element in the top-level document, -including same-document rendering after the tab reports `complete`. It uses the -snapshot's geometric visibility rule (at least 2×2 CSS pixels and not CSS-hidden; -not a guarantee of clickability, lack of occlusion, or opacity). Frames and shadow +independently of resource loading, including same-document rendering. It shares +the snapshot's visibility rules: at least 2×2 CSS pixels; no hidden, aria-hidden, +inert or fully transparent ancestor. This is not a guarantee of clickability or +lack of occlusion. Frames and shadow roots are not searched. Choose a selector specific to the **new expected UI**; an element already present can satisfy it immediately. No arbitrary page script is accepted. @@ -251,14 +252,16 @@ native server; older extensions may omit readiness metadata. Regression checks: `node --test chrome-extension/background.test.mjs` covers the wait contract and site-policy guards. The real-browser CI harness `scripts/e2e-browser.mjs` tests a dialog delayed by 1100 ms without navigation, -one click only, plus timeout and read-only recovery. The standalone Chromium -regression can use an existing Playwright installation without adding a required -dependency: +one click only, plus timeout and read-only recovery. The dependency-free CDP +regressions cover both snapshot scoping/pagination and SPA readiness. Run against +an isolated Chrome/Chromium profile; no Playwright module is required: ```bash -PLAYWRIGHT_MODULE=/absolute/path/to/playwright/index.mjs node --test scripts/spa-readiness.test.mjs -# Same assertions against the pre-fix source; expected to fail: -SOURCE_REF=deb4c7a PLAYWRIGHT_MODULE=/absolute/path/to/playwright/index.mjs node --test scripts/spa-readiness.test.mjs +node scripts/snapshot-dom.test.mjs --chrome /path/to/chrome +# Extract the pre-fix source; each suite must fail independently: +git show deb4c7a:chrome-extension/background.js > original-background.js +node scripts/snapshot-dom.test.mjs --chrome /path/to/chrome --source original-background.js --suite snapshot +node scripts/snapshot-dom.test.mjs --chrome /path/to/chrome --source original-background.js --suite readiness ``` `browser_snapshot` automatically scopes to the topmost visible dialog when one is @@ -301,7 +304,7 @@ Linux notes: element actions work everywhere; coordinate clicks need an X11 or X cd windows-linux && cargo test # Rust server node chrome-extension/background.test.mjs # extension, against a fake Chrome node scripts/check-tool-parity.mjs # Swift and Rust tool lists match -# Snapshot DOM regressions (dependency-free CDP, isolated profile): +# Snapshot and SPA readiness regressions (dependency-free CDP, isolated profile): node scripts/snapshot-dom.test.mjs --chrome # The browser tools end to end, in a throwaway Chrome for Testing profile that # never touches your own Chrome (npx playwright install chromium to get one): diff --git a/chrome-extension/background.js b/chrome-extension/background.js index 6fdabdf..52a5c49 100644 --- a/chrome-extension/background.js +++ b/chrome-extension/background.js @@ -553,6 +553,23 @@ async function send(tabId, method, params = {}) { return chrome.debugger.sendCommand({ tabId }, method, params); } +/** Shared by snapshots and explicit readiness checks, serialized into the page. */ +function isElementVisibleInPage(el) { + const r = el.getBoundingClientRect(); + if (r.width < 2 || r.height < 2) return false; + let escapedInert = false; + for (let node = el; node; node = node.parentElement) { + if (node.hidden || node.getAttribute("aria-hidden")?.toLowerCase() === "true") return false; + if (node.inert && !escapedInert) return false; + // Native showModal() escapes ancestor inertness, not its own inert subtrees. + if (node.matches("dialog:modal")) escapedInert = true; + const style = getComputedStyle(node); + if (style.display === "none" || style.contentVisibility === "hidden" || Number(style.opacity) === 0) return false; + } + const style = getComputedStyle(el); + return style.visibility !== "hidden" && style.visibility !== "collapse"; +} + /// A compact outline of the interactive elements on the page, with ids the /// agent can click. Mirrors the accessibility-tree tools on the desktop side. const SNAPSHOT_JS = `((options = {}) => { @@ -567,22 +584,7 @@ const SNAPSHOT_JS = `((options = {}) => { for (const old of document.querySelectorAll('[data-cu-idx]')) old.removeAttribute('data-cu-idx'); // Accessibility-hidden/inert ancestors are not actionable. Do not use viewport // intersection here: indexed click scrolls reachable offscreen controls into view. - const visible = (el) => { - const r = el.getBoundingClientRect(); - if (r.width < 2 || r.height < 2) return false; - let escapedInert = false; - for (let node = el; node; node = node.parentElement) { - if (node.hidden || node.getAttribute('aria-hidden')?.toLowerCase() === 'true') return false; - if (node.inert && !escapedInert) return false; - // showModal() escapes ancestor inertness, but an explicitly inert dialog - // or subtree within it remains inert. - if (node.matches('dialog:modal')) escapedInert = true; - const style = getComputedStyle(node); - if (style.display === 'none' || style.contentVisibility === 'hidden' || Number(style.opacity) === 0) return false; - } - const style = getComputedStyle(el); - return style.visibility !== 'hidden' && style.visibility !== 'collapse'; - }; + const visible = ${isElementVisibleInPage.toString()}; // Native modal dialogs live in Chrome's top layer, above ordinary z-indexes. // For custom dialogs, use paint order at their visible centre, not DOM order // or raw z-index (which is only meaningful inside a stacking context). @@ -1175,7 +1177,7 @@ function validateReadiness(p, action = false) { } /** A visible match in the top-level document, not arbitrary application readiness. */ -function visibleSelectorInPage(selector) { +function visibleSelectorInPage(selector, visible) { let matches; try { matches = document.querySelectorAll(selector); @@ -1183,14 +1185,7 @@ function visibleSelectorInPage(selector) { return { error: "invalid CSS selector" }; } return { - met: [...matches].some((el) => { - const rect = el.getBoundingClientRect(); - const style = getComputedStyle(el); - return ( - rect.width >= 2 && rect.height >= 2 && style.visibility !== "hidden" && - style.visibility !== "collapse" && style.display !== "none" - ); - }), + met: [...matches].some(visible), }; } @@ -1207,17 +1202,21 @@ async function waitForReadiness(p) { // Recheck on every observation: the action or a redirect may leave the // allowed site during the wait. Never evaluate selectors on a blocked page. await checkTab(requireClientId(p), p.tabId, p.sites, "read"); - const tab = await chrome.tabs.get(p.tabId); - if (tab.status !== "loading") { - const res = await send(p.tabId, "Runtime.evaluate", { - expression: `(${visibleSelectorInPage.toString()})(${JSON.stringify(p.waitForSelector)})`, + // An explicit DOM condition is independent of document/resource loading. + // In particular, a slow image must not hide an already-rendered dialog. + let res; + try { + res = await send(p.tabId, "Runtime.evaluate", { + expression: `(${visibleSelectorInPage.toString()})(${JSON.stringify(p.waitForSelector)}, ${isElementVisibleInPage.toString()})`, returnByValue: true, }); - if (res?.exceptionDetails) return outcome("error", { error: res.exceptionDetails.text || "evaluate failed" }); - const value = res?.result?.value; - if (value?.error) return outcome("error", { error: value.error }); - if (value?.met === true) return outcome("met"); + } catch (error) { + return outcome("error", { error: error.message || "evaluate failed" }); } + if (res?.exceptionDetails) return outcome("error", { error: res.exceptionDetails.text || "evaluate failed" }); + const value = res?.result?.value; + if (value?.error) return outcome("error", { error: value.error }); + if (value?.met === true) return outcome("met"); if (Date.now() >= deadline) return outcome("timeout"); await sleep(Math.min(100, deadline - Date.now())); } diff --git a/chrome-extension/background.test.mjs b/chrome-extension/background.test.mjs index 411c9ef..8493899 100644 --- a/chrome-extension/background.test.mjs +++ b/chrome-extension/background.test.mjs @@ -649,6 +649,26 @@ test("a readiness evaluation error is not reported as a failed click", async () pageEval = () => ({ result: { value: {} } }); }); +test("a readiness transport failure preserves the successful action and current snapshot", async () => { + let clicks = 0; + pageEval = (_method, params) => { + const expression = params.expression ?? ""; + if (expression.includes("data-cu-idx=\"3\"")) { + clicks++; + return { result: { value: { ok: true, x: 5, y: 5 } } }; + } + if (expression.includes("visibleSelectorInPage")) throw new Error("readiness transport unavailable"); + return { result: { value: { title: "Current", elements: [] } } }; + }; + const result = await callAndWait("click", { tabId: pageTab, index: 3, returnState: true, waitForSelector: "#ready" }); + assert.equal(result.ok, true); + assert.equal(result.readiness.status, "error"); + assert.equal(result.readiness.error, "readiness transport unavailable"); + assert.equal(result.snapshot.title, "Current"); + assert.equal(clicks, 1); + pageEval = () => ({ result: { value: {} } }); +}); + test("an explicit wait rechecks site rules before each selector observation", async () => { const tab = tabs.get(pageTab); const before = tab.url; diff --git a/scripts/e2e-browser.mjs b/scripts/e2e-browser.mjs index 36dd399..8f90d09 100644 --- a/scripts/e2e-browser.mjs +++ b/scripts/e2e-browser.mjs @@ -43,7 +43,7 @@ const PREFILLED = "PREFILLED-VALUE-MUST-NOT-LEAK"; const sleep = (ms) => new Promise((resume) => setTimeout(resume, ms)); // Short base path: a Unix socket path must fit in 104 bytes on macOS. -const root = fs.mkdtempSync(path.join(os.platform() === "win32" ? os.tmpdir() : "/tmp", "cu-e2e-")); +const root = fs.mkdtempSync(path.join(os.platform() === "win32" ? (process.env.TMPDIR || os.tmpdir()) : "/tmp", "cu-e2e-")); const children = []; function cleanup() { for (const child of children) { @@ -51,7 +51,8 @@ function cleanup() { child.kill("SIGKILL"); } catch {} } - fs.rmSync(root, { recursive: true, force: true }); + // Chrome's children may still be exiting after SIGKILL; retry filesystem races. + fs.rmSync(root, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); } process.on("exit", cleanup); diff --git a/scripts/snapshot-dom.test.mjs b/scripts/snapshot-dom.test.mjs index 272742a..62b597b 100644 --- a/scripts/snapshot-dom.test.mjs +++ b/scripts/snapshot-dom.test.mjs @@ -1,7 +1,8 @@ #!/usr/bin/env node // Real Chromium regression, dependency-free CDP; never uses the user's profile. // node scripts/snapshot-dom.test.mjs --chrome -// Add --source to run the same regression against a baseline. +// Add --source to run against a baseline; --suite snapshot or +// --suite readiness isolates either root-cause regression. Default: both. import assert from "node:assert/strict"; import fs from "node:fs"; import path from "node:path"; @@ -14,6 +15,8 @@ const option = (name) => { const at = process.argv.indexOf(name); return at >= 0 ? process.argv[at + 1] : undefined; }; +const suite = option("--suite") || "all"; +assert.ok(["all", "snapshot", "readiness"].includes(suite), "invalid --suite"); const chromePath = option("--chrome"); if (!chromePath) throw new Error("provide --chrome "); const scratch = process.env.TMPDIR || path.join(root, ".snapshot-test"); @@ -84,99 +87,196 @@ try { assert.ok(!result.exceptionDetails, JSON.stringify(result)); return result.result.value; } - const source = fs.readFileSync(option("--source") || path.join(root, "chrome-extension/background.js"), "utf8"); - const declaration = source.slice(source.indexOf("const SNAPSHOT_JS ="), source.indexOf("\nasync function snapshot(")); + const source = fs.readFileSync(option("--source") || path.join(root, "chrome-extension/background.js"), "utf8").split(String.fromCharCode(13)).join(""); + const start = source.includes("function isElementVisibleInPage(") ? "function isElementVisibleInPage(" : "const SNAPSHOT_JS ="; + const declaration = source.slice(source.indexOf(start), source.indexOf("\nasync function snapshot(")); const expression = vm.runInNewContext(declaration + "\nSNAPSHOT_JS"); const clickDeclaration = source.slice(source.indexOf("const CLICK_JS ="), source.indexOf("\n/// Click a snapshotted element")); const click = vm.runInNewContext(clickDeclaration + "\nCLICK_JS"); const snapshot = (options = {}) => evaluate(expression.slice(0, -2) + `(${JSON.stringify(options)})`); const set = (html) => evaluate(`document.body.innerHTML = ${JSON.stringify(html)}; true`); const buttons = (count, prefix = "Background") => Array.from({ length: count }, (_, i) => ``).join(""); - await set(`
${buttons(260)}
`); - const modal = await snapshot(); - console.log(`original reproduction: ${modal.elements.length} controls; labels=${modal.elements.slice(0, 3).map((el) => el.label).join(", ")}`); - assert.deepEqual(modal.elements.map((el) => el.label), ["Domain", "Next", "Verify"]); - assert.equal(modal.scope, "modal"); - console.log("PASS appended modal excludes 260 hidden background controls"); - // Keep evaluating the original target after activating a different tab. - const foreground = await command("Target.createTarget", { url: "about:blank" }); - await command("Target.activateTarget", { targetId: foreground.targetId }); - assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Domain", "Next", "Verify"]); - console.log("PASS modal snapshot works in a background tab"); + if (suite !== "readiness") { + await set(`
${buttons(260)}
`); + const modal = await snapshot(); + console.log(`original reproduction: ${modal.elements.length} controls; labels=${modal.elements.slice(0, 3).map((el) => el.label).join(", ")}`); + assert.deepEqual(modal.elements.map((el) => el.label), ["Domain", "Next", "Verify"]); + assert.equal(modal.scope, "modal"); + console.log("PASS appended modal excludes 260 hidden background controls"); + // Keep evaluating the original target after activating a different tab. + const foreground = await command("Target.createTarget", { url: "about:blank" }); + await command("Target.activateTarget", { targetId: foreground.targetId }); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Domain", "Next", "Verify"]); + console.log("PASS modal snapshot works in a background tab"); - await set(`${buttons(260)}
`); - assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Foreground"]); - console.log("PASS modal budget excludes ordinary covered background"); + await set(`${buttons(260)}
`); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Foreground"]); + console.log("PASS modal budget excludes ordinary covered background"); - await set('
'); - assert.equal((await snapshot()).elements[0].label, "Top"); - await evaluate('document.body.innerHTML += \'\'; document.querySelector("dialog").showModal(); true'); - assert.equal((await snapshot()).elements[0].label, "Native top layer"); - await evaluate('document.querySelector("dialog").innerHTML += \'
\'; true'); - assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Nested native"]); - console.log("PASS painted stacking and native modal top layer"); + await set('
'); + assert.equal((await snapshot()).elements[0].label, "Top"); + await evaluate('document.body.innerHTML += \'\'; document.querySelector("dialog").showModal(); true'); + assert.equal((await snapshot()).elements[0].label, "Native top layer"); + await evaluate('document.querySelector("dialog").innerHTML += \'
\'; true'); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Nested native"]); + console.log("PASS painted stacking and native modal top layer"); - await set('
'); - await evaluate('document.querySelector("dialog").showModal(); true'); - assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Escaped inert"]); - console.log("PASS native modal escapes ancestor inertness, not explicit descendant inertness"); + await set('
'); + await evaluate('document.querySelector("dialog").showModal(); true'); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Escaped inert"]); + console.log("PASS native modal escapes ancestor inertness, not explicit descendant inertness"); - await set(''); - assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Page action", "Modeless"]); - await set('
'); - assert.equal((await snapshot()).scope, "page"); - assert.equal((await snapshot()).total, 2); - console.log("PASS modeless and explicitly nonmodal dialogs retain page controls"); + await set(''); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Page action", "Modeless"]); + await set('
'); + assert.equal((await snapshot()).scope, "page"); + assert.equal((await snapshot()).total, 2); + console.log("PASS modeless and explicitly nonmodal dialogs retain page controls"); - await set('
'); - assert.equal((await snapshot()).elements[0].label, "High context"); - await set('
'); - assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Nested"]); - console.log("PASS stacking contexts and nested topmost dialog"); + await set('
'); + assert.equal((await snapshot()).elements[0].label, "High context"); + await set('
'); + assert.deepEqual((await snapshot()).elements.map((el) => el.label), ["Nested"]); + console.log("PASS stacking contexts and nested topmost dialog"); - await set(`${buttons(260)}
${buttons(300, "Modal")}
`); - const modalFirst = await snapshot({ limit: 100 }); - assert.equal(modalFirst.total, 300); - assert.equal(modalFirst.nextOffset, 100); - const modalLast = await snapshot({ offset: 200, limit: 100 }); - assert.equal(modalLast.elements[0].label, "Modal 200"); - assert.equal(modalLast.elements[0].i, 200); - assert.equal(modalLast.nextOffset, null); - await evaluate('globalThis.savedHitTest = document.elementsFromPoint; document.elementsFromPoint = () => []; true'); - assert.equal((await snapshot()).scope, "modal"); - await evaluate('document.elementsFromPoint = globalThis.savedHitTest; delete globalThis.savedHitTest; true'); - console.log("PASS large modal pagination and background-tab hit-test fallback"); + await set(`${buttons(260)}
${buttons(300, "Modal")}
`); + const modalFirst = await snapshot({ limit: 100 }); + assert.equal(modalFirst.total, 300); + assert.equal(modalFirst.nextOffset, 100); + const modalLast = await snapshot({ offset: 200, limit: 100 }); + assert.equal(modalLast.elements[0].label, "Modal 200"); + assert.equal(modalLast.elements[0].i, 200); + assert.equal(modalLast.nextOffset, null); + await evaluate('globalThis.savedHitTest = document.elementsFromPoint; document.elementsFromPoint = () => []; true'); + assert.equal((await snapshot()).scope, "modal"); + await evaluate('document.elementsFromPoint = globalThis.savedHitTest; delete globalThis.savedHitTest; true'); + console.log("PASS large modal pagination and background-tab hit-test fallback"); - await set('
'); - const accessible = await snapshot(); - assert.deepEqual(accessible.elements.map((el) => el.label), ["Offscreen", ""]); - assert.equal(accessible.elements[0].inView, false); - assert.ok(!JSON.stringify(accessible).includes("DO-NOT-LEAK")); - console.log("PASS hidden/inert ancestors excluded, offscreen retained, password protected"); + await set('
'); + const accessible = await snapshot(); + assert.deepEqual(accessible.elements.map((el) => el.label), ["Offscreen", ""]); + assert.equal(accessible.elements[0].inView, false); + assert.ok(!JSON.stringify(accessible).includes("DO-NOT-LEAK")); + console.log("PASS hidden/inert ancestors excluded, offscreen retained, password protected"); - await set(buttons(520, "Control")); - const first = await snapshot(); - assert.equal(first.elements.length, 250); - assert.equal(first.total, 520); - assert.equal(first.truncated, true); - assert.equal(first.nextOffset, 250); - const second = await snapshot({ offset: first.nextOffset, limit: 250 }); - assert.equal(second.elements[0].i, 250); - assert.equal(second.elements[0].label, "Control 250"); - assert.equal(await evaluate('document.querySelector(\'[data-cu-idx="0"]\').innerText'), "Control 0"); - assert.equal(await evaluate('document.querySelectorAll(\'[data-cu-idx="250"]\').length'), 1); - await evaluate('document.querySelector(\'[data-cu-idx="250"]\').onclick = () => document.title = "clicked page two"; true'); - assert.equal((await evaluate(click(second.elements[0].i))).ok, true); - assert.equal(await evaluate("document.title"), "clicked page two"); - const last = await snapshot({ offset: second.nextOffset }); - assert.equal(last.elements.length, 20); - assert.equal(last.truncated, false); - assert.equal(last.nextOffset, null); - assert.equal((await snapshot({ offset: 1000 })).elements.length, 0); - await evaluate('document.querySelector(\'[data-cu-idx="0"]\').hidden = true; true'); - await snapshot(); - assert.equal(await evaluate('document.querySelectorAll(\'button[hidden][data-cu-idx]\').length'), 0); - console.log("PASS bounded pagination, unique actionable global indices, stale-index cleanup"); + await set(buttons(520, "Control")); + const first = await snapshot(); + assert.equal(first.elements.length, 250); + assert.equal(first.total, 520); + assert.equal(first.truncated, true); + assert.equal(first.nextOffset, 250); + const second = await snapshot({ offset: first.nextOffset, limit: 250 }); + assert.equal(second.elements[0].i, 250); + assert.equal(second.elements[0].label, "Control 250"); + assert.equal(await evaluate('document.querySelector(\'[data-cu-idx="0"]\').innerText'), "Control 0"); + assert.equal(await evaluate('document.querySelectorAll(\'[data-cu-idx="250"]\').length'), 1); + await evaluate('document.querySelector(\'[data-cu-idx="250"]\').onclick = () => document.title = "clicked page two"; true'); + assert.equal((await evaluate(click(second.elements[0].i))).ok, true); + assert.equal(await evaluate("document.title"), "clicked page two"); + const last = await snapshot({ offset: second.nextOffset }); + assert.equal(last.elements.length, 20); + assert.equal(last.truncated, false); + assert.equal(last.nextOffset, null); + assert.equal((await snapshot({ offset: 1000 })).elements.length, 0); + await evaluate('document.querySelector(\'[data-cu-idx="0"]\').hidden = true; true'); + await snapshot(); + assert.equal(await evaluate('document.querySelectorAll(\'button[hidden][data-cu-idx]\').length'), 0); + console.log("PASS bounded pagination, unique actionable global indices, stale-index cleanup"); + + } + + if (suite !== "snapshot") { + const between = (first, last) => { + const begin = source.indexOf(first); + const end = source.indexOf(last, begin); + assert.ok(begin >= 0 && end > begin, `missing source boundary: ${first}`); + return source.slice(begin, end); + }; + const tab = { status: "complete" }; + const context = vm.createContext({ + setTimeout, Date, + chrome: { tabs: { get: async () => tab } }, + requireClientId: () => "test", + checkTab: async () => {}, + send: async (_tabId, _method, params) => { + try { return { result: { value: await evaluate(params.expression) } }; } + catch (error) { return { exceptionDetails: { text: error.message } }; } + }, + }); + vm.runInContext( + between(start, "async function clickAt") + + between("const CLICK_JS", "/// Click a snapshotted") + + between("const sleep =", "/**\n * The page's readable text"), context, + ); + const extension = (code) => vm.runInContext(code, context); + const openDelayedDialog = async () => { + await set(`
${buttons(260)}
`); + await evaluate(`(() => { + window.clicks = 0; + document.getElementById('open').onclick = () => { + window.clicks++; + setTimeout(() => { + const modal = document.createElement('div'); + modal.setAttribute('role', 'dialog'); + modal.style.cssText = 'position:fixed;inset:0;background:white'; + modal.innerHTML = '

Connect your Domain

'; + document.body.append(modal); + }, 1100); + }; + })()`); + const initial = await extension("snapshot(1)"); + const index = initial.elements.find((el) => el.label === "Open dialog").i; + assert.equal((await evaluate(extension(`CLICK_JS(${index})`))).ok, true); + }; + await openDelayedDialog(); + const before = Date.now(); + const result = await extension('withState({tabId:1,returnState:true,waitForSelector:"[role=dialog] button",waitTimeoutMs:3000},{ok:true})'); + const rendered = await evaluate("document.querySelector('[role=dialog] button') !== null"); + console.log(`post-click wait ${Date.now() - before}ms; Next=${result.snapshot.elements.some((el) => el.label === "Next")}; dialogRendered=${rendered}`); + assert.equal(rendered, true, "snapshot returned before the delayed dialog rendered"); + assert.ok(result.snapshot.elements.some((el) => el.label === "Next"), "snapshot returned before the delayed dialog rendered"); + assert.equal(result.readiness.status, "met"); + assert.equal(result.snapshot.scope, "modal"); + assert.deepEqual(await evaluate("[window.clicks, document.querySelectorAll('[role=dialog]').length]"), [1, 1]); + console.log("PASS delayed same-document dialog with 260 background controls, one click only"); + + await openDelayedDialog(); + const timedOut = await extension('withState({tabId:1,returnState:true,waitForSelector:"[role=dialog] button",waitTimeoutMs:0},{ok:true})'); + assert.equal(timedOut.ok, true); + assert.equal(timedOut.readiness.status, "timeout"); + const observed = await extension('waitForReadiness({tabId:1,waitForSelector:"[role=dialog] button",waitTimeoutMs:3000})'); + assert.equal(observed.status, "met"); + assert.ok((await extension("snapshot(1)")).elements.some((el) => el.label === "Next")); + assert.deepEqual(await evaluate("[window.clicks, document.querySelectorAll('[role=dialog]').length]"), [1, 1]); + console.log("PASS timeout preserves action success and read-only re-observation does not replay it"); + + await set(''); + tab.status = "loading"; + assert.equal((await extension('waitForReadiness({tabId:1,waitForSelector:"#ready",waitTimeoutMs:100})')).status, "met"); + tab.status = "complete"; + const unconfigured = await extension("withState({tabId:1,returnState:true},{ok:true})"); + assert.equal(unconfigured.readiness.status, "not_requested"); + console.log("PASS explicit condition independent of resource loading; no implicit SPA-readiness claim"); + + await set('
'); + for (const selector of ["#aria-hidden", "#inert", "#transparent"]) { + assert.equal((await extension(`waitForReadiness({tabId:1,waitForSelector:${JSON.stringify(selector)},waitTimeoutMs:0})`)).status, "timeout"); + } + await evaluate('setTimeout(() => { document.getElementById("revealed").style.display = "block"; }, 600); true'); + const hiddenStart = Date.now(); + assert.equal((await extension('waitForReadiness({tabId:1,waitForSelector:"#revealed",waitTimeoutMs:2000})')).status, "met"); + assert.ok(Date.now() - hiddenStart >= 450); + console.log("PASS shared hidden/inert/opacity rules and delayed visible match"); + + const invalid = await extension('withState({tabId:1,returnState:true,waitForSelector:"[",waitTimeoutMs:0},{ok:true})'); + assert.equal(invalid.ok, true); + assert.equal(invalid.readiness.status, "error"); + assert.equal(invalid.readiness.error, "invalid CSS selector"); + const malicious = '[id="x"]); window.injected = true; //'; + assert.equal((await extension(`waitForReadiness({tabId:1,waitForSelector:${JSON.stringify(malicious)},waitTimeoutMs:0})`)).status, "error"); + assert.equal(await evaluate("window.injected"), undefined); + console.log("PASS invalid CSS reported separately from action success; selector cannot inject script"); + } } finally { socket?.close(); if (chrome.pid && chrome.exitCode === null && chrome.signalCode === null) { diff --git a/scripts/spa-readiness.test.mjs b/scripts/spa-readiness.test.mjs deleted file mode 100644 index 2613cf9..0000000 --- a/scripts/spa-readiness.test.mjs +++ /dev/null @@ -1,132 +0,0 @@ -// Real Chromium regression for #19, without installing a runtime dependency. -// PLAYWRIGHT_MODULE=/absolute/path/to/playwright/index.mjs node --test scripts/spa-readiness.test.mjs -// SOURCE_REF=deb4c7a runs the same assertions against the original implementation. -import assert from "node:assert/strict"; -import fs from "node:fs"; -import vm from "node:vm"; -import { execFileSync } from "node:child_process"; -import { pathToFileURL } from "node:url"; -import { test } from "node:test"; - -const source = process.env.SOURCE_REF - ? execFileSync("git", ["show", `${process.env.SOURCE_REF}:chrome-extension/background.js`], { encoding: "utf8" }) - : fs.readFileSync(new URL("../chrome-extension/background.js", import.meta.url), "utf8"); -const between = (start, end) => { - const normalized = source.replace(/\r\n/g, "\n"); - const first = normalized.indexOf(start); - const last = normalized.indexOf(end, first); - assert.ok(first >= 0 && last > first, `missing source boundary: ${start} / ${end}`); - return normalized.slice(first, last); -}; - -async function fixture(run) { - assert.ok(process.env.PLAYWRIGHT_MODULE, "Set PLAYWRIGHT_MODULE to an existing Playwright index.mjs"); - const { chromium } = await import(pathToFileURL(process.env.PLAYWRIGHT_MODULE).href); - const browser = await chromium.launch({ headless: true }); - try { - const page = await browser.newPage(); - await page.setContent(``); - const context = vm.createContext({ - setTimeout, Date, - chrome: { tabs: { get: async () => ({ status: "complete" }) } }, - requireClientId: () => "test", - checkTab: async () => {}, - send: async (_tabId, _method, params) => { - try { return { result: { value: await page.evaluate(params.expression) } }; } - catch (error) { return { exceptionDetails: { text: error.message } }; } - }, - }); - vm.runInContext( - between("const SNAPSHOT_JS", "async function clickAt") + - between("const CLICK_JS", "/// Click a snapshotted") + - between("const sleep =", "/**\n * The page's readable text"), context, - ); - const evaluate = (expression) => vm.runInContext(expression, context); - const initial = await evaluate("snapshot(1)"); - const index = initial.elements.find((el) => el.label === "Open dialog").i; - assert.equal((await page.evaluate(evaluate(`CLICK_JS(${index})`))).ok, true); - await run({ page, evaluate }); - } finally { - await browser.close(); - } -} - -test("a single click waits for the delayed SPA dialog without navigation", async () => { - await fixture(async ({ page, evaluate }) => { - const start = Date.now(); - const result = await evaluate('withState({tabId:1,returnState:true,waitForSelector:"[role=dialog] button",waitTimeoutMs:3000},{ok:true})'); - const elapsed = Date.now() - start; - console.log(`post-click wait ${elapsed}ms; Next=${result.snapshot.elements.some((el) => el.label === "Next")}`); - assert.ok(result.snapshot.elements.some((el) => el.label === "Next"), "snapshot returned before the delayed dialog rendered"); - assert.equal(result.readiness.status, "met"); - assert.equal(result.readiness.condition, "visible_selector"); - assert.ok(elapsed >= 900 && elapsed < 3500); - assert.equal(await page.evaluate("window.clicks"), 1); - assert.equal(await page.locator("[role=dialog]").count(), 1); - assert.equal(page.url(), "about:blank"); - }); -}); - -test("timeout preserves the successful action; read-only re-observation never clicks again", async () => { - await fixture(async ({ page, evaluate }) => { - const result = await evaluate('withState({tabId:1,returnState:true,waitForSelector:"[role=dialog] button",waitTimeoutMs:100},{ok:true})'); - assert.equal(result.ok, true); - assert.equal(result.readiness.status, "timeout"); - assert.ok(!result.snapshot.elements.some((el) => el.label === "Next")); - const readiness = await evaluate('waitForReadiness({tabId:1,waitForSelector:"[role=dialog] button",waitTimeoutMs:3000})'); - assert.equal(readiness.status, "met"); - assert.ok((await evaluate("snapshot(1)")).elements.some((el) => el.label === "Next")); - assert.equal(await page.evaluate("window.clicks"), 1); - assert.equal(await page.locator("[role=dialog]").count(), 1); - }); -}); - -test("invalid selector syntax is a readiness error, not a failed or duplicated action", async () => { - await fixture(async ({ page, evaluate }) => { - const result = await evaluate('withState({tabId:1,returnState:true,waitForSelector:"[",waitTimeoutMs:100},{ok:true})'); - assert.equal(result.ok, true); - assert.equal(result.readiness.status, "error"); - assert.equal(result.readiness.error, "invalid CSS selector"); - assert.equal(await page.evaluate("window.clicks"), 1); - const malicious = '[id="x"]); window.injected = true; //'; - const readiness = await evaluate(`waitForReadiness({tabId:1,waitForSelector:${JSON.stringify(malicious)},waitTimeoutMs:0})`); - assert.equal(readiness.status, "error"); - assert.equal(await page.evaluate("window.injected"), undefined); - }); -}); - -test("an unconfigured action snapshot does not claim application readiness", async () => { - await fixture(async ({ evaluate }) => { - const result = await evaluate("withState({tabId:1,returnState:true},{ok:true})"); - assert.equal(result.ok, true); - assert.equal(result.readiness.status, "not_requested"); - assert.equal(result.readiness.condition, "document_load"); - }); -}); - -test("a hidden match does not satisfy visible-selector readiness", async () => { - await fixture(async ({ page, evaluate }) => { - await page.evaluate(() => { - const hidden = document.createElement("button"); - hidden.id = "ready"; - hidden.style.display = "none"; - document.body.append(hidden); - setTimeout(() => { hidden.style.display = "block"; }, 600); - }); - const start = Date.now(); - const readiness = await evaluate('waitForReadiness({tabId:1,waitForSelector:"#ready",waitTimeoutMs:2000})'); - assert.equal(readiness.status, "met"); - assert.ok(Date.now() - start >= 450); - }); -}); From f2a466b3d5c1cb01241d251332116a737b2669bb Mon Sep 17 00:00:00 2001 From: Elliot Drel Date: Wed, 7 Oct 2026 10:23:43 -0400 Subject: [PATCH 4/6] test(browser): terminate isolated smoke-test process trees before profile cleanup --- scripts/e2e-browser.mjs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/e2e-browser.mjs b/scripts/e2e-browser.mjs index 8f90d09..0cb00ef 100644 --- a/scripts/e2e-browser.mjs +++ b/scripts/e2e-browser.mjs @@ -48,7 +48,10 @@ const children = []; function cleanup() { for (const child of children) { try { - child.kill("SIGKILL"); + // Each POSIX child owns an isolated process group, including Chromium's + // renderers and native hosts. Killing just its launcher leaves writers. + if (os.platform() === "win32") child.kill("SIGKILL"); + else process.kill(-child.pid, "SIGKILL"); } catch {} } // Chrome's children may still be exiting after SIGKILL; retry filesystem races. @@ -132,6 +135,7 @@ const policyPath = path.join(root, "policy.json"); // ── the MCP server ────────────────────────────────────────────────────────── const server = spawn(serverBinary, ["--profile", profilePath], { + detached: os.platform() !== "win32", env: { ...process.env, COMPUTER_USE_POLICY: policyPath }, stdio: ["pipe", "pipe", "pipe"], }); @@ -196,7 +200,7 @@ const chromeArgs = [ ...(os.platform() === "linux" ? ["--no-sandbox"] : []), "about:blank", ]; -const chrome = spawn(chromeBinary, chromeArgs, { stdio: ["ignore", "ignore", "pipe"] }); +const chrome = spawn(chromeBinary, chromeArgs, { detached: os.platform() !== "win32", stdio: ["ignore", "ignore", "pipe"] }); children.push(chrome); let chromeLog = ""; chrome.stderr.on("data", (chunk) => (chromeLog += chunk)); From 1646abc1dfef109a2d15b83995517a1556f863df Mon Sep 17 00:00:00 2001 From: Elliot Drel Date: Wed, 7 Oct 2026 11:12:03 -0400 Subject: [PATCH 5/6] fix(browser): address modeless widget, transparent control and profile audit findings --- README.md | 9 ++++- chrome-extension/background.js | 18 ++++++++-- scripts/snapshot-dom.test.mjs | 61 +++++++++++++++++++++++++++++++--- 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 560c260..9db546f 100644 --- a/README.md +++ b/README.md @@ -214,7 +214,9 @@ the same wait **without performing an action**: The extension polls for any matching visible element in the top-level document, independently of resource loading, including same-document rendering. It shares the snapshot's visibility rules: at least 2×2 CSS pixels; no hidden, aria-hidden, -inert or fully transparent ancestor. This is not a guarantee of clickability or +inert or fully transparent ancestor. Self-transparent native form controls remain +available for styled upload/checkbox hit targets; transparent containers do not. +This is not a guarantee of clickability or lack of occlusion. Frames and shadow roots are not searched. Choose a selector specific to the **new expected UI**; an element already present can satisfy it immediately. No arbitrary page script @@ -268,6 +270,11 @@ node scripts/snapshot-dom.test.mjs --chrome /path/to/chrome --source original-ba open, so covered background controls cannot consume the dialog's budget. Hidden and inert subtrees are excluded; reachable offscreen controls remain available. Native modeless dialogs and explicit `aria-modal="false"` do not scope the page. +Native modal dialogs and explicit `aria-modal="true"` are definitive candidates. +Implicit ARIA dialogs are heuristically scoped only when they cover the viewport +centre and are topmost there, rather than merely at their own centre. With no +hit-test stack, centre coverage is the fallback. Corner chat widgets and bottom +cookie banners therefore retain the page's indexed controls. It returns up to `limit` controls (default and maximum `250`), reports `scope`, `offset`, `limit`, and `total`, and explicitly says when it is truncated and which `offset` to request next. Continue on the same `tab_id` and `session_id`. Indices diff --git a/chrome-extension/background.js b/chrome-extension/background.js index 52a5c49..65e9fa2 100644 --- a/chrome-extension/background.js +++ b/chrome-extension/background.js @@ -557,6 +557,9 @@ async function send(tabId, method, params = {}) { function isElementVisibleInPage(el) { const r = el.getBoundingClientRect(); if (r.width < 2 || r.height < 2) return false; + // Styled labels often use transparent native controls as real hit targets. + // Do not extend this exception to containers: a faded-out dialog is not ready. + const transparentControl = el.matches('input,button,select,textarea'); let escapedInert = false; for (let node = el; node; node = node.parentElement) { if (node.hidden || node.getAttribute("aria-hidden")?.toLowerCase() === "true") return false; @@ -564,7 +567,8 @@ function isElementVisibleInPage(el) { // Native showModal() escapes ancestor inertness, not its own inert subtrees. if (node.matches("dialog:modal")) escapedInert = true; const style = getComputedStyle(node); - if (style.display === "none" || style.contentVisibility === "hidden" || Number(style.opacity) === 0) return false; + if (style.display === "none" || style.contentVisibility === "hidden" || + (Number(style.opacity) === 0 && (node !== el || !transparentControl))) return false; } const style = getComputedStyle(el); return style.visibility !== "hidden" && style.visibility !== "collapse"; @@ -589,7 +593,17 @@ const SNAPSHOT_JS = `((options = {}) => { // For custom dialogs, use paint order at their visible centre, not DOM order // or raw z-index (which is only meaningful inside a stacking context). // Modeless native dialogs and explicit aria-modal=false keep page controls. - const dialogs = Array.from(document.querySelectorAll('dialog:modal,[role=dialog]:not([aria-modal=false i]),[role=alertdialog]:not([aria-modal=false i]),[aria-modal=true i]')).filter(visible); + const dialogs = Array.from(document.querySelectorAll('dialog:modal,[role=dialog]:not([aria-modal=false i]),[role=alertdialog]:not([aria-modal=false i]),[aria-modal=true i]')).filter((dialog) => { + if (!visible(dialog)) return false; + if (dialog.matches('dialog:modal,[aria-modal=true i]')) return true; + // An implicit ARIA dialog is not automatically modal. Preserve full-page + // overlays, but never let a corner widget or bottom banner hide the page. + const x = innerWidth / 2, y = innerHeight / 2; + const r = dialog.getBoundingClientRect(); + if (x < r.left || x >= r.right || y < r.top || y >= r.bottom) return false; + const painted = document.elementsFromPoint(x, y); + return !painted.length || dialog.contains(painted[0]); + }); const native = dialogs.filter((dialog) => dialog.matches(':modal')); // Keep nested custom dialogs inside the native top layer eligible too. const candidates = dialogs.filter((dialog) => !native.length || native.some((layer) => layer.contains(dialog))).reverse(); diff --git a/scripts/snapshot-dom.test.mjs b/scripts/snapshot-dom.test.mjs index 62b597b..dec0e9f 100644 --- a/scripts/snapshot-dom.test.mjs +++ b/scripts/snapshot-dom.test.mjs @@ -6,6 +6,7 @@ import assert from "node:assert/strict"; import fs from "node:fs"; import path from "node:path"; +import os from "node:os"; import vm from "node:vm"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; @@ -16,10 +17,25 @@ const option = (name) => { return at >= 0 ? process.argv[at + 1] : undefined; }; const suite = option("--suite") || "all"; -assert.ok(["all", "snapshot", "readiness"].includes(suite), "invalid --suite"); +assert.ok(["all", "snapshot", "readiness", "audit-r1", "audit-r2", "audit-r3"].includes(suite), "invalid --suite"); const chromePath = option("--chrome"); if (!chromePath) throw new Error("provide --chrome "); -const scratch = process.env.TMPDIR || path.join(root, ".snapshot-test"); +if (suite === "all" || suite === "audit-r3") { + // Exercise the actual profile-root declaration without creating a profile or + // writing into the checkout, even when testing the pre-fix runner. + const runner = fs.readFileSync(option("--runner-source") || fileURLToPath(import.meta.url), "utf8"); + const declaration = runner.split(String.fromCharCode(10)).find((line) => line.startsWith("const scratch =")); + assert.ok(declaration, "missing scratch declaration"); + for (const env of [{}, { TMPDIR: "designated-scratch" }]) { + const actual = vm.runInNewContext(declaration + "; scratch", { + process: { env }, path, root, os: { tmpdir: () => "system-temp-fixture" }, + }); + assert.equal(actual, env.TMPDIR || "system-temp-fixture", "profile fallback must be outside the checkout"); + } + console.log("PASS scratch override and absent-TMPDIR fallback stay outside the checkout"); + if (suite === "audit-r3") process.exit(0); +} +const scratch = process.env.TMPDIR || os.tmpdir(); fs.mkdirSync(scratch, { recursive: true }); const profile = fs.mkdtempSync(path.join(scratch, "snapshot-dom-")); const chrome = spawn(chromePath, [ @@ -96,7 +112,7 @@ try { const snapshot = (options = {}) => evaluate(expression.slice(0, -2) + `(${JSON.stringify(options)})`); const set = (html) => evaluate(`document.body.innerHTML = ${JSON.stringify(html)}; true`); const buttons = (count, prefix = "Background") => Array.from({ length: count }, (_, i) => ``).join(""); - if (suite !== "readiness") { + if (suite === "all" || suite === "snapshot") { await set(`
${buttons(260)}
`); const modal = await snapshot(); console.log(`original reproduction: ${modal.elements.length} controls; labels=${modal.elements.slice(0, 3).map((el) => el.label).join(", ")}`); @@ -185,7 +201,44 @@ try { } - if (suite !== "snapshot") { + if (suite === "all" || suite === "audit-r1") { + await set('
'); + const banner = await snapshot(); + assert.equal(banner.scope, "page", "a modeless cookie banner must not suppress the page"); + assert.deepEqual(banner.elements.map((el) => el.label), ["Home", "Pricing", "Sign in", "Buy now", "Accept cookies"]); + await evaluate('document.getElementById("buy").onclick = () => globalThis.pageActionClicked = true; true'); + assert.equal((await evaluate(click(banner.elements.find((el) => el.label === "Buy now").i))).ok, true); + assert.equal(await evaluate("globalThis.pageActionClicked"), true); + await set('
'); + const chat = await snapshot(); + assert.equal(chat.scope, "page", "a corner widget must not suppress the page"); + assert.deepEqual(chat.elements.map((el) => el.label), ["Buy now", "Checkout", "Message", "Send"]); + // A missing background-tab hit-test stack must not promote this widget. + await evaluate('globalThis.savedHitTest = document.elementsFromPoint; document.elementsFromPoint = () => []; true'); + assert.equal((await snapshot()).scope, "page"); + await evaluate('document.elementsFromPoint = globalThis.savedHitTest; delete globalThis.savedHitTest; true'); + console.log("PASS modeless cookie/chat widgets preserve indexed page actions, including absent hit tests"); + } + + if (suite === "all" || suite === "audit-r2") { + await set(''); + const controls = await snapshot(); + assert.deepEqual(controls.elements.map((el) => el.label), ["Upload resume", "Enable notifications"], "transparent native hit targets must remain indexed"); + const toggle = controls.elements.find((el) => el.tag === "input[checkbox]"); + assert.equal((await evaluate(click(toggle.i))).ok, true); + assert.equal(await evaluate('document.getElementById("toggle").checked'), true); + const visible = vm.runInNewContext(declaration + "; isElementVisibleInPage"); + const selectorCode = source.slice(source.indexOf("function visibleSelectorInPage("), source.indexOf("/** Poll an explicit condition")); + const selector = vm.runInNewContext(selectorCode + "; visibleSelectorInPage"); + const ready = (target) => evaluate(`(${selector.toString()})(${JSON.stringify(target)}, ${visible.toString()})`); + assert.equal((await ready("#toggle")).met, true); + await set(''); + assert.equal((await snapshot()).scope, "page", "a fully transparent dialog is not a visible modal"); + assert.equal((await ready("#transparent-dialog")).met, false, "a transparent container must not satisfy visible readiness"); + console.log("PASS transparent native inputs stay indexed/clickable/ready while transparent containers stay hidden"); + } + + if (suite === "all" || suite === "readiness") { const between = (first, last) => { const begin = source.indexOf(first); const end = source.indexOf(last, begin); From 98606830b5a4e81e436bc5d7fad7be5efeb1077f Mon Sep 17 00:00:00 2001 From: Elliot Drel Date: Wed, 7 Oct 2026 12:59:17 -0400 Subject: [PATCH 6/6] test: stop isolated Chrome process trees before profile cleanup Fork-main CI exposed ENOTEMPTY after all DOM assertions passed. A launcher-only kill left POSIX Chromium children writing to the disposable profile. Own a private process group and terminate it even if the launcher has already exited. Four deterministic regressions were proven failing before the repair; run them in CI. (cherry picked from commit 655fc19132449429da55fd90d04bddc6696ff0f3) --- .github/workflows/ci.yml | 1 + scripts/snapshot-cleanup.test.mjs | 64 +++++++++++++++++++++++++++++++ scripts/snapshot-dom.test.mjs | 20 +++++++--- 3 files changed, 79 insertions(+), 6 deletions(-) create mode 100644 scripts/snapshot-cleanup.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6ec3933..526f73b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,6 +78,7 @@ jobs: with: node-version: 22 - run: node --test chrome-extension/background.test.mjs + - run: node scripts/snapshot-cleanup.test.mjs - run: node scripts/build-extension.test.mjs - run: node scripts/check-tool-parity.mjs - run: node scripts/tool-defs.mjs --check diff --git a/scripts/snapshot-cleanup.test.mjs b/scripts/snapshot-cleanup.test.mjs new file mode 100644 index 0000000..a6cdd54 --- /dev/null +++ b/scripts/snapshot-cleanup.test.mjs @@ -0,0 +1,64 @@ +#!/usr/bin/env node +// Deterministic teardown regression: no browser, filesystem writes or real kills. +import assert from "node:assert/strict"; +import fs from "node:fs"; +import vm from "node:vm"; +import { EventEmitter } from "node:events"; + +const source = fs.readFileSync(new URL("./snapshot-dom.test.mjs", import.meta.url), "utf8"); +const launchStart = source.indexOf("const chrome = spawn("); +const launchEnd = source.indexOf("let chromeLog =", launchStart); +const cleanupStart = source.lastIndexOf("} finally {"); +assert.ok(launchStart >= 0 && launchEnd > launchStart && cleanupStart >= 0); +const launch = source.slice(launchStart, launchEnd); +const cleanup = source.slice(cleanupStart + "} finally {".length, source.lastIndexOf("}")); + +let failed = 0; +for (const platform of ["linux", "darwin"]) { + for (const alreadyExited of [false, true]) { + const label = `${platform}: ${alreadyExited ? "exited launcher" : "running launcher"}, surviving profile writer`; + const chrome = new EventEmitter(); + chrome.pid = 31337; + chrome.exitCode = alreadyExited ? 0 : null; + chrome.signalCode = null; + let descendantWriting = true; + let launchOptions; + const exit = () => { + if (chrome.exitCode !== null || chrome.signalCode !== null) return; + chrome.signalCode = "SIGKILL"; + queueMicrotask(() => chrome.emit("exit", null, "SIGKILL")); + }; + chrome.kill = () => { exit(); return true; }; // Parent-only kill leaves writer. + const context = { + chromePath: "fixture-only-chrome", profile: "fixture-only-profile", + socket: { close() {} }, + spawn(_executable, _args, options) { launchOptions = options; return chrome; }, + process: { + platform, + kill(pid, signal) { + assert.equal(pid, -chrome.pid, "signal only the private browser process group"); + assert.equal(signal, "SIGKILL"); + assert.equal(launchOptions.detached, true, "browser must own its process group"); + descendantWriting = false; + exit(); + }, + }, + fs: { + rmSync(profile, options) { + assert.equal(profile, "fixture-only-profile"); + assert.equal(options.recursive, true); + assert.equal(descendantWriting, false, "Chrome descendant still writing when profile deletion starts"); + assert.ok(chrome.exitCode !== null || chrome.signalCode !== null, "wait for the launcher to exit"); + }, + }, + }; + try { + await vm.runInNewContext(`(async () => { ${launch}\n${cleanup}\n})()`, context); + console.log(`PASS ${label}`); + } catch (error) { + failed++; + console.error(`FAIL ${label}: ${error.message}`); + } + } +} +if (failed) process.exitCode = 1; diff --git a/scripts/snapshot-dom.test.mjs b/scripts/snapshot-dom.test.mjs index dec0e9f..698e94e 100644 --- a/scripts/snapshot-dom.test.mjs +++ b/scripts/snapshot-dom.test.mjs @@ -45,7 +45,7 @@ const chrome = spawn(chromePath, [ "--no-first-run", ...(process.platform === "linux" ? ["--no-sandbox"] : []), "about:blank", -], { stdio: ["ignore", "ignore", "pipe"] }); +], { detached: process.platform !== "win32", stdio: ["ignore", "ignore", "pipe"] }); let chromeLog = ""; chrome.stderr.on("data", (chunk) => (chromeLog += chunk)); chrome.on("error", (error) => (chromeLog += error.message)); @@ -332,11 +332,19 @@ try { } } finally { socket?.close(); - if (chrome.pid && chrome.exitCode === null && chrome.signalCode === null) { - const exited = new Promise((resolve) => chrome.once("exit", resolve)); - chrome.kill(); - await exited; + const exited = chrome.pid && chrome.exitCode === null && chrome.signalCode === null + ? new Promise((resolve) => chrome.once("exit", resolve)) : null; + if (chrome.pid) { + try { + // POSIX Chromium children can outlive their launcher and keep writing. + // Signal only this test's private process group, even after launcher exit. + if (process.platform === "win32") chrome.kill(); + else process.kill(-chrome.pid, "SIGKILL"); + } catch (error) { + if (error.code !== "ESRCH") throw error; + } } - // Windows child processes can release profile files a moment after exit. + if (exited) await exited; + // Children can release profile files a moment after the group is terminated. fs.rmSync(profile, { recursive: true, force: true, maxRetries: 20, retryDelay: 100 }); }