From eeb633d68ecff24eb6b217871fbbcc17dd3426b5 Mon Sep 17 00:00:00 2001 From: drakezhang Date: Mon, 7 Sep 2026 13:25:54 +0800 Subject: [PATCH 1/3] fix(extension): verify fill results before reporting success --- .../src/tools/__tests__/fill.test.ts | 370 ++++++++++++++++++ .../src/tools/__tests__/interaction.test.ts | 34 +- apps/extension/src/tools/interaction.ts | 217 ++++++++-- 3 files changed, 574 insertions(+), 47 deletions(-) create mode 100644 apps/extension/src/tools/__tests__/fill.test.ts diff --git a/apps/extension/src/tools/__tests__/fill.test.ts b/apps/extension/src/tools/__tests__/fill.test.ts new file mode 100644 index 0000000..e256c56 --- /dev/null +++ b/apps/extension/src/tools/__tests__/fill.test.ts @@ -0,0 +1,370 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { SessionManager } from "@/session-manager/manager"; +import type { CdpRunner } from "@/tools/shared"; +import { handleFill } from "../interaction"; + +interface ScriptParams { + functionDeclaration: string; + arguments?: Array<{ value: unknown }>; +} + +async function setup(markup = '') { + document.body.innerHTML = markup; + const element = document.body.firstElementChild as HTMLInputElement; + vi.spyOn(document, "hasFocus").mockReturnValue(true); + const manager = new SessionManager({ + agentWindow: { + create: async () => 100, + remove: async () => {}, + ensureActiveTab: async () => 4, + }, + }); + const ctx = await manager.start("aa11"); + ctx.refStore.set("e1", 12, { tabId: 4 }); + const script = vi.fn(async (params: ScriptParams): Promise => { + try { + const fn = new Function(`return (${params.functionDeclaration})`)(); + return { + result: { + value: await fn.apply( + element, + (params.arguments ?? []).map((arg) => arg.value), + ), + }, + }; + } catch { + return { exceptionDetails: { text: "test page script failed" } }; + } + }); + const insert = vi.fn(async (text: string) => { + const focused = document.activeElement as HTMLInputElement; + if (focused.readOnly || focused.disabled) return; + const start = focused.selectionStart ?? focused.value.length; + const end = focused.selectionEnd ?? start; + const value = focused.value.slice(0, start) + text + focused.value.slice(end); + focused.value = focused.maxLength >= 0 ? value.slice(0, focused.maxLength) : value; + focused.dispatchEvent(new Event("input", { bubbles: true })); + }); + const release = vi.fn(async () => ({})); + const send = vi.fn(async (_tabId: number, method: string, params?: object) => { + switch (method) { + case "DOM.describeNode": + return { + node: { + nodeName: element.tagName, + attributes: Array.from(element.attributes).flatMap((attr) => [attr.name, attr.value]), + }, + }; + case "DOM.scrollIntoViewIfNeeded": + return {}; + case "DOM.focus": + element.focus(); + return {}; + case "DOM.resolveNode": + return { object: { objectId: "fill-target" } }; + case "Runtime.callFunctionOn": + return script(params as ScriptParams); + case "Input.insertText": + await insert((params as { text: string }).text); + return {}; + case "Runtime.releaseObject": + return release(); + default: + throw new Error(`unexpected ${method}`); + } + }); + return { + element, + script, + insert, + release, + send, + fill: (value = "hello", clearBefore = true, signal?: AbortSignal) => + handleFill( + manager, + { session_id: "aa11", ref: "e1", value, clear_before: clearBefore }, + { + cdp: { send: send as CdpRunner["send"] }, + tabsApi: { + get: async (id) => ({ id, windowId: 100, active: true }) as chrome.tabs.Tab, + query: async () => [{ id: 4, windowId: 100, active: true } as chrome.tabs.Tab], + }, + signal, + }, + ), + }; +} + +afterEach(() => { + vi.restoreAllMocks(); + document.body.innerHTML = ""; +}); + +describe("fill result verification", () => { + it.each([ + "readonly", + "disabled", + ])("preserves a %s field before rejecting it", async (attribute) => { + const h = await setup(``); + expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /not editable/ }); + expect(h.element.value).toBe("old"); + expect(h.insert).not.toHaveBeenCalled(); + expect(h.release).toHaveBeenCalledOnce(); + }); + + it.each([ + "checkbox", + "radio", + "range", + "date", + "file", + ])("rejects an unsupported %s input before clearing", async (type) => { + const h = await setup(``); + const before = h.element.value; + expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /input type/ }); + expect(h.element.value).toBe(before); + expect(h.insert).not.toHaveBeenCalled(); + }); + + it("rejects an overlong value before clearing", async () => { + const h = await setup(''); + expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /maxlength/ }); + expect(h.element.value).toBe("old"); + expect(h.insert).not.toHaveBeenCalled(); + }); + + it("does not type into a field that gained focus during clearing", async () => { + const h = await setup(); + const other = document.createElement("input"); + document.body.append(other); + h.element.addEventListener("input", () => other.focus()); + expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /lost focus/ }); + expect(other.value).toBe(""); + expect(h.insert).not.toHaveBeenCalled(); + }); + + it.each([ + "remove", + "readonly", + "restore", + "microtask-restore", + ])("stops when clearing causes the page to %s the target", async (action) => { + const h = await setup(); + h.element.addEventListener("input", () => { + if (action === "remove") h.element.remove(); + if (action === "readonly") h.element.readOnly = true; + if (action === "restore") h.element.value = "restored"; + if (action === "microtask-restore") + queueMicrotask(() => { + h.element.value = "restored"; + }); + }); + expect(await h.fill()).toMatchObject({ code: "cdp_failed" }); + expect(h.insert).not.toHaveBeenCalled(); + }); + + it("fails when insertText is acknowledged without changing the value", async () => { + const h = await setup(); + h.insert.mockImplementation(async () => {}); + expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /expected value/ }); + expect(h.release).toHaveBeenCalledOnce(); + }); + + it("detects truncation when maxlength changes after preparation", async () => { + const h = await setup(); + h.element.addEventListener( + "input", + () => { + h.element.maxLength = 3; + }, + { once: true }, + ); + expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /expected value/ }); + expect(h.element.value).toBe("hel"); + }); + + it.each([ + "input", + "change", + "microtask-change", + ])("detects a value reverted by the %s handler", async (event) => { + const h = await setup(); + h.element.addEventListener(event === "microtask-change" ? "change" : event, () => { + if (event === "microtask-change") + queueMicrotask(() => { + h.element.value = ""; + }); + else h.element.value = ""; + }); + expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /expected value/ }); + }); + + it("compares content rather than length without including field values in the error", async () => { + const h = await setup(''); + h.element.addEventListener("change", () => { + h.element.value = "other-secret"; + }); + const result = await h.fill("given-secret"); + expect(result).toMatchObject({ code: "cdp_failed", message: /expected value/ }); + expect(JSON.stringify(result)).not.toMatch(/given-secret|other-secret/); + }); + + it("does not report success for a target replaced by a change handler", async () => { + const h = await setup(); + h.element.addEventListener("change", () => h.element.replaceWith(h.element.cloneNode())); + expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /expected value/ }); + }); + + it.each([ + 1, 2, 3, 4, + ])("rejects script exceptions at phase %s and releases the target", async (phase) => { + const h = await setup(); + const run = h.script.getMockImplementation()!; + let count = 0; + h.script.mockImplementation(async (params) => + ++count === phase ? { exceptionDetails: { text: "page secret" } } : run(params), + ); + const result = await h.fill(); + expect(result).toMatchObject({ code: "cdp_failed" }); + expect(JSON.stringify(result)).not.toContain("page secret"); + if (phase <= 2) expect(h.insert).not.toHaveBeenCalled(); + expect(h.release).toHaveBeenCalledOnce(); + }); + + it.each([1, 2, 4])("rejects missing results at phase %s", async (phase) => { + const h = await setup(); + const run = h.script.getMockImplementation()!; + let count = 0; + h.script.mockImplementation(async (params) => + ++count === phase ? { result: { type: "undefined" } } : run(params), + ); + expect(await h.fill()).toMatchObject({ code: "cdp_failed" }); + if (phase <= 2) expect(h.insert).not.toHaveBeenCalled(); + }); + + it.each(["input", "change"])("checks state after nested microtasks from %s", async (event) => { + const h = await setup(); + h.element.addEventListener(event, () => { + queueMicrotask(() => + queueMicrotask(() => { + h.element.value = "reverted"; + }), + ); + }); + expect(await h.fill()).toMatchObject({ code: "cdp_failed" }); + if (event === "input") expect(h.insert).not.toHaveBeenCalled(); + }); + + it.each([ + "", + '', + "", + ])("verifies successful text in %s", async (markup) => { + const h = await setup(markup); + expect(await h.fill("你好🙂")).toMatchObject({ value_length: 4 }); + expect(h.element.value).toBe("你好🙂"); + expect(h.release).toHaveBeenCalledOnce(); + }); + + it("verifies append against the old value plus the requested text", async () => { + const h = await setup(); + h.element.focus(); + h.element.setSelectionRange(3, 3); + expect(await h.fill("!", false)).toMatchObject({ value_length: 1 }); + expect(h.element.value).toBe("old!"); + }); + + it.each([ + ["", "a\r\nb", "a\nb"], + ["", "a\nb", "a b"], + ["", "a\r\nb", "a b"], + [ + '', + " a@example.com , b@example.com ", + "a@example.com,b@example.com", + ], + ])("accepts native normalization for %s", async (markup, requested, normalized) => { + if (markup.startsWith(" { + h.element.value = normalized; + }); + expect(await h.fill(requested)).toMatchObject({ value_length: normalized.length }); + }); + + it("does not count an editable padding break as an extra typed character", async () => { + const h = await setup('
'); + // These two layout/editability properties are missing in Happy DOM. + let innerText = ""; + Object.defineProperty(h.element, "isContentEditable", { get: () => true }); + Object.defineProperty(h.element, "innerText", { get: () => innerText }); + h.insert.mockImplementation(async () => { + h.element.innerHTML = "one

"; + innerText = "one\n\n"; + }); + expect(await h.fill("one\n")).toMatchObject({ value_length: 4 }); + }); + + it("rejects a non-append selection before typing", async () => { + const h = await setup(); + h.element.focus(); + h.element.setSelectionRange(1, 1); + expect(await h.fill("!", false)).toMatchObject({ code: "cdp_failed" }); + expect(h.element.value).toBe("old"); + expect(h.insert).not.toHaveBeenCalled(); + }); + + it.each([true, false])("verifies an empty request with clear_before=%s", async (clearBefore) => { + const h = await setup(); + expect(await h.fill("", clearBefore)).toMatchObject({ value_length: 0 }); + expect(h.element.value).toBe(clearBefore ? "" : "old"); + expect(h.insert).not.toHaveBeenCalled(); + }); + + it("cancels after insertion without sending change events", async () => { + const h = await setup(); + const controller = new AbortController(); + h.insert.mockImplementation(async () => controller.abort()); + expect(await h.fill("hello", true, controller.signal)).toMatchObject({ code: "cancelled" }); + expect(h.script).toHaveBeenCalledTimes(2); + expect(h.release).toHaveBeenCalledOnce(); + }); + + it("cancels after resolving the remote object without clearing the field", async () => { + const h = await setup(); + const controller = new AbortController(); + const send = h.send.getMockImplementation()!; + h.send.mockImplementation(async (tabId, method, params) => { + const result = await send(tabId, method, params); + if (method === "DOM.resolveNode") controller.abort(); + return result; + }); + expect(await h.fill("hello", true, controller.signal)).toMatchObject({ code: "cancelled" }); + expect(h.element.value).toBe("old"); + expect(h.script).not.toHaveBeenCalled(); + expect(h.release).toHaveBeenCalledOnce(); + }); + + it("honors cancellation during verification", async () => { + const h = await setup(); + const controller = new AbortController(); + h.element.addEventListener("change", () => controller.abort()); + expect(await h.fill("hello", true, controller.signal)).toMatchObject({ code: "cancelled" }); + expect(h.release).toHaveBeenCalledOnce(); + }); + + it("keeps the verified outcome when releasing the remote object fails", async () => { + const h = await setup(); + h.release.mockRejectedValueOnce(new Error("target closed")); + expect(await h.fill()).toMatchObject({ value_length: 5 }); + }); +}); diff --git a/apps/extension/src/tools/__tests__/interaction.test.ts b/apps/extension/src/tools/__tests__/interaction.test.ts index 45ab74e..7d552b2 100644 --- a/apps/extension/src/tools/__tests__/interaction.test.ts +++ b/apps/extension/src/tools/__tests__/interaction.test.ts @@ -540,6 +540,18 @@ describe("handleHover", () => { }); }); +function successfulFillScript(params: unknown) { + const args = (params as { arguments?: Array<{ value: unknown }> }).arguments ?? []; + return { + result: { + value: + args.length === 2 + ? { before: "", expected: args[0].value, valueLength: String(args[0].value).length } + : true, + }, + }; +} + describe("handleFill", () => { it("returns not_found for unknown ref", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); @@ -586,7 +598,8 @@ describe("handleFill", () => { "DOM.scrollIntoViewIfNeeded": () => ({}), "DOM.focus": () => ({}), "DOM.resolveNode": () => ({ object: { objectId: "obj-1" } }), - "Runtime.callFunctionOn": () => ({ result: { type: "undefined" } }), + "Runtime.callFunctionOn": successfulFillScript, + "Runtime.releaseObject": () => ({}), "Input.insertText": () => ({}), }); const res = await handleFill( @@ -600,13 +613,12 @@ describe("handleFill", () => { expect(res.used_ref).toBe("e1"); const insert = fake.sent.find((c) => c.method === "Input.insertText"); expect(insert?.params).toEqual({ text: "hello" }); - // clear_before defaults to true → callFunctionOn invoked once to - // clear, once to fire input/change after typing. + // Preparation, readiness, notifications, and final verification. const callFns = fake.sent.filter((c) => c.method === "Runtime.callFunctionOn"); - expect(callFns.length).toBeGreaterThanOrEqual(2); + expect(callFns).toHaveLength(4); }); - it("clear_before=false skips the wipe call", async () => { + it("passes clear_before=false to preparation and verifies the result", async () => { const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) }); const ctx = await sm.start("aa11"); ctx.refStore.set("e1", 1, { tabId: 4 }); @@ -618,12 +630,11 @@ describe("handleFill", () => { "DOM.focus": () => ({}), "DOM.resolveNode": () => ({ object: { objectId: "obj-2" } }), "Runtime.callFunctionOn": (p) => { - const fn = (p as { functionDeclaration?: string }).functionDeclaration ?? ""; - // No "this.value = ''" clearing on a no-clear path; only the - // post-input dispatchEvent. - expect(fn).not.toMatch(/this\.value\s*=\s*''/); - return { result: { type: "undefined" } }; + const args = (p as { arguments?: Array<{ value: unknown }> }).arguments ?? []; + if (args.length === 2) expect(args[1].value).toBe(false); + return successfulFillScript(p); }, + "Runtime.releaseObject": () => ({}), "Input.insertText": () => ({}), }); const res = await handleFill( @@ -650,7 +661,8 @@ describe("handleFill", () => { "DOM.scrollIntoViewIfNeeded": () => ({}), "DOM.focus": () => ({}), "DOM.resolveNode": () => ({ object: { objectId: "obj-3" } }), - "Runtime.callFunctionOn": () => ({ result: { type: "undefined" } }), + "Runtime.callFunctionOn": successfulFillScript, + "Runtime.releaseObject": () => ({}), "Input.insertText": () => ({}), }); const res = await handleFill( diff --git a/apps/extension/src/tools/interaction.ts b/apps/extension/src/tools/interaction.ts index 18608ca..5a48349 100644 --- a/apps/extension/src/tools/interaction.ts +++ b/apps/extension/src/tools/interaction.ts @@ -609,6 +609,36 @@ type SelectMutationResult = // tool.fill // --------------------------------------------------------------------------- +interface FillPreparation { + before?: string; + expected?: string; + valueLength?: number; + error?: string; +} + +interface FillScriptReply { + result?: { value?: T }; + exceptionDetails?: unknown; +} + +const FILL_EDITABLE_FUNCTION = `function() { + const tag = this.tagName.toLowerCase(); + const supported = tag === 'input' + ? ['text', 'search', 'tel', 'url', 'email', 'password', 'number'].includes(this.type) + : tag === 'textarea' || this.isContentEditable; + return this.isConnected && supported && !this.readOnly && !this.matches(':disabled'); +}`; + +// Chrome renders a trailing editable newline with an empty

. +// innerText includes the padding break; it is not an extra typed character. +const FILL_VALUE_FUNCTION = `function() { + if (!this.isContentEditable) return this.value; + const value = this.innerText; + const tail = this.lastChild; + const padding = tail && tail.nodeName === 'DIV' && tail.childNodes.length === 1 && tail.firstChild.nodeName === 'BR'; + return padding && value.endsWith('\\n\\n') ? value.slice(0, -1) : value; +}`; + export async function handleFill( manager: SessionManager, params: FillParams, @@ -678,57 +708,172 @@ export async function handleFill( const clearBefore = params.clear_before ?? true; try { - if (clearBefore) { - // Clear input/textarea value or wipe contenteditable innerText, - // then fire `input` so frameworks observe the empty state. - await nodeCdp.send(target.tabId, "Runtime.callFunctionOn", { - objectId, - functionDeclaration: `function() { - if (this.isContentEditable) { this.textContent = ''; } - else { - const proto = this instanceof HTMLTextAreaElement - ? HTMLTextAreaElement.prototype - : HTMLInputElement.prototype; - const descriptor = Object.getOwnPropertyDescriptor(proto, 'value'); - if (descriptor && descriptor.set) descriptor.set.call(this, ''); - else this.value = ''; - } - this.dispatchEvent(new Event('input', { bubbles: true })); - }`, - returnByValue: true, - }); - } if (throwIfAborted(deps.signal)) { return { code: "cancelled", message: "fill aborted" }; } + // Check editability before clearing. Keep the expected result tied to + // this object, including the existing value on the append path. + const prepared = await nodeCdp.send>( + target.tabId, + "Runtime.callFunctionOn", + { + objectId, + functionDeclaration: `function(value, clearBefore) { + const tag = this.tagName.toLowerCase(); + const native = tag === 'input' || tag === 'textarea'; + if (!(${FILL_EDITABLE_FUNCTION}).call(this)) return { error: 'fill target is not editable or its input type is unsupported' }; + const focused = () => this.getRootNode().activeElement === this && this.ownerDocument.hasFocus(); + if (!focused()) return { error: 'fill target does not have focus' }; + const before = clearBefore ? '' : (${FILL_VALUE_FUNCTION}).call(this); + if (!clearBefore && value !== '' && native && typeof this.selectionStart === 'number' && + (this.selectionStart !== before.length || this.selectionEnd !== before.length)) { + return { error: 'fill append requires the caret at the end of the field' }; + } + let expected = before + value; + if (native) { + // Use the browser's own value sanitization (e.g. textarea + // line endings) without changing the live control. + const normalizer = this.ownerDocument.createElement(tag); + if (tag === 'input') { + normalizer.type = this.type; + normalizer.multiple = this.multiple; + } + // insertText treats line breaks in a single-line input as spaces. + if (tag === 'input') expected = expected.replace(/\\r\\n|\\r|\\n/g, ' '); + normalizer.value = expected; + if (expected !== '' && normalizer.value === '' && this.type === 'number') { + return { error: 'fill value is not valid for a number input' }; + } + expected = normalizer.value; + if (this.type !== 'number' && this.maxLength >= 0 && expected.length > this.maxLength) { + return { error: 'fill value exceeds the target maxlength' }; + } + } else { + expected = expected.replace(/\\r\\n?/g, '\\n'); + } + if (clearBefore) { + if (native) { + const proto = tag === 'textarea' ? HTMLTextAreaElement.prototype : HTMLInputElement.prototype; + Object.getOwnPropertyDescriptor(proto, 'value').set.call(this, ''); + } else { + this.textContent = ''; + } + this.dispatchEvent(new Event('input', { bubbles: true })); + } + return { before, expected, valueLength: Math.max(0, expected.length - before.length) }; + }`, + arguments: [{ value: params.value }, { value: clearBefore }], + returnByValue: true, + }, + ); + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + if (prepared.exceptionDetails) { + return { code: "cdp_failed", message: "fill preparation script failed" }; + } + const preparation = prepared.result?.value; + if ( + preparation?.error || + typeof preparation?.before !== "string" || + typeof preparation?.expected !== "string" || + typeof preparation.valueLength !== "number" + ) { + return { + code: "cdp_failed", + message: preparation?.error ?? "fill preparation returned an unexpected result", + }; + } + // A separate call runs after the clearing event's microtasks drain. + // Confirm the target before sending input to the focused element. + const ready = await nodeCdp.send>( + target.tabId, + "Runtime.callFunctionOn", + { + objectId, + functionDeclaration: `function(before) { + return (${FILL_EDITABLE_FUNCTION}).call(this) && + this.getRootNode().activeElement === this && this.ownerDocument.hasFocus() && + (${FILL_VALUE_FUNCTION}).call(this) === before; + }`, + arguments: [{ value: preparation.before }], + returnByValue: true, + }, + ); + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + if (ready.exceptionDetails || ready.result?.value !== true) { + return { + code: "cdp_failed", + message: "fill target changed, lost focus, or could not be checked before typing", + }; + } // CDP `Input.insertText` handles IME / multi-byte input out of the // box, much more reliably than per-key `dispatchKeyEvent`. - await deps.cdp.send(target.tabId, "Input.insertText", { text: params.value }); + if (params.value !== "") { + await deps.cdp.send(target.tabId, "Input.insertText", { text: params.value }); + } if (throwIfAborted(deps.signal)) { return { code: "cancelled", message: "fill aborted" }; } - // Fire `input` + `change` so React / Vue controlled inputs commit. - await nodeCdp.send(target.tabId, "Runtime.callFunctionOn", { - objectId, - functionDeclaration: `function() { - this.dispatchEvent(new Event('input', { bubbles: true })); - this.dispatchEvent(new Event('change', { bubbles: true })); - }`, - returnByValue: true, + // Keep the existing notifications, then read in a separate call so + // nested microtasks queued by the page cannot race the verification. + const notified = await nodeCdp.send>( + target.tabId, + "Runtime.callFunctionOn", + { + objectId, + functionDeclaration: `function() { + this.dispatchEvent(new Event('input', { bubbles: true })); + this.dispatchEvent(new Event('change', { bubbles: true })); + }`, + returnByValue: true, + }, + ); + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + if (notified.exceptionDetails) { + return { code: "cdp_failed", message: "fill notification script failed" }; + } + const verified = await nodeCdp.send>( + target.tabId, + "Runtime.callFunctionOn", + { + objectId, + functionDeclaration: `function(expected) { + return this.isConnected && (${FILL_VALUE_FUNCTION}).call(this) === expected; + }`, + arguments: [{ value: preparation.expected }], + returnByValue: true, + }, + ); + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + if (verified.exceptionDetails || verified.result?.value !== true) { + return { + code: "cdp_failed", + message: verified.exceptionDetails + ? "fill verification script failed" + : "fill could not verify the expected value; observe the page before retrying", + }; + } + return attachDialogs(deps.cdp, target.tabId, dialogCursor, { + tab_id: target.tabId, + used_ref: node.usedRef, + used_selector: node.usedSelector, + value_length: preparation.valueLength, }); } catch (err) { return { code: "cdp_failed", message: err instanceof Error ? err.message : String(err), }; + } finally { + await nodeCdp.send(target.tabId, "Runtime.releaseObject", { objectId }).catch(() => undefined); } - - return attachDialogs(deps.cdp, target.tabId, dialogCursor, { - tab_id: target.tabId, - used_ref: node.usedRef, - used_selector: node.usedSelector, - value_length: params.value.length, - }); } // --------------------------------------------------------------------------- From f2184e74c5395add06153d997e7d283d993e4659 Mon Sep 17 00:00:00 2001 From: drakezhang Date: Mon, 7 Sep 2026 15:28:21 +0800 Subject: [PATCH 2/3] fix(fill): support background input and automatic append --- .../src/tools/__tests__/fill.test.ts | 208 +++++++++++++-- .../src/tools/__tests__/interaction.test.ts | 12 +- apps/extension/src/tools/interaction.ts | 243 ++++++++++++------ apps/extension/src/transport/types.ts | 5 + crates/bsk-cli/skill/SKILL.md | 4 + crates/bsk-cli/src/cli/render_error.rs | 92 ++++++- .../dsh-plugin-browserskill/skill/SKILL.md | 4 + .../tests/runner.test.ts | 22 ++ skill/SKILL.md | 4 + 9 files changed, 493 insertions(+), 101 deletions(-) diff --git a/apps/extension/src/tools/__tests__/fill.test.ts b/apps/extension/src/tools/__tests__/fill.test.ts index e256c56..53adbf5 100644 --- a/apps/extension/src/tools/__tests__/fill.test.ts +++ b/apps/extension/src/tools/__tests__/fill.test.ts @@ -11,7 +11,6 @@ interface ScriptParams { async function setup(markup = '') { document.body.innerHTML = markup; const element = document.body.firstElementChild as HTMLInputElement; - vi.spyOn(document, "hasFocus").mockReturnValue(true); const manager = new SessionManager({ agentWindow: { create: async () => 100, @@ -67,6 +66,13 @@ async function setup(markup = '') { case "Input.insertText": await insert((params as { text: string }).text); return {}; + case "Input.dispatchKeyEvent": + if ((params as { commands?: string[] }).commands?.includes("moveToEndOfDocument")) { + if (typeof element.selectionStart === "number") { + element.setSelectionRange(element.value.length, element.value.length); + } + } + return {}; case "Runtime.releaseObject": return release(); default: @@ -106,7 +112,10 @@ describe("fill result verification", () => { "disabled", ])("preserves a %s field before rejecting it", async (attribute) => { const h = await setup(``); - expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /not editable/ }); + expect(await h.fill()).toMatchObject({ + code: "invalid_params", + data: { reason: "target_not_fillable" }, + }); expect(h.element.value).toBe("old"); expect(h.insert).not.toHaveBeenCalled(); expect(h.release).toHaveBeenCalledOnce(); @@ -121,26 +130,87 @@ describe("fill result verification", () => { ])("rejects an unsupported %s input before clearing", async (type) => { const h = await setup(``); const before = h.element.value; - expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /input type/ }); + expect(await h.fill()).toMatchObject({ + code: "invalid_params", + data: { reason: "target_not_fillable" }, + }); expect(h.element.value).toBe(before); expect(h.insert).not.toHaveBeenCalled(); }); it("rejects an overlong value before clearing", async () => { const h = await setup(''); - expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /maxlength/ }); + expect(await h.fill()).toMatchObject({ + code: "invalid_params", + message: /maxlength/, + data: { reason: "fill_value_invalid" }, + }); expect(h.element.value).toBe("old"); expect(h.insert).not.toHaveBeenCalled(); }); - it("does not type into a field that gained focus during clearing", async () => { + it.each([ + true, + false, + ])("classifies a DOM.focus rejection on a disabled=%s target", async (disabled) => { + const h = await setup(``); + const send = h.send.getMockImplementation()!; + h.send.mockImplementation(async (tabId, method, params) => { + if (method === "DOM.focus") throw new Error("Element is not focusable"); + return send(tabId, method, params); + }); + expect(await h.fill()).toMatchObject({ + code: disabled ? "invalid_params" : "cdp_failed", + data: { reason: disabled ? "target_not_fillable" : "fill_failed" }, + }); + expect(h.element.value).toBe("old"); + expect(h.insert).not.toHaveBeenCalled(); + expect(h.release).toHaveBeenCalledOnce(); + }); + + it("fills a background document without requiring foreground browser focus", async () => { + const h = await setup(); + vi.spyOn(document, "hasFocus").mockReturnValue(false); + expect(await h.fill()).toMatchObject({ value_length: 5 }); + expect(h.element.value).toBe("hello"); + }); + + it("restores focus once after clearing without typing into another field", async () => { const h = await setup(); const other = document.createElement("input"); document.body.append(other); h.element.addEventListener("input", () => other.focus()); - expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /lost focus/ }); + expect(await h.fill()).toMatchObject({ value_length: 5 }); + expect(h.element.value).toBe("hello"); + expect(other.value).toBe(""); + expect(h.send.mock.calls.filter(([, method]) => method === "DOM.focus")).toHaveLength(2); + }); + + it.each([ + "focus", + "value", + ])("rechecks %s after restoring focus, without retrying again", async (action) => { + const h = await setup(); + const other = document.createElement("input"); + document.body.append(other); + h.element.addEventListener( + "input", + () => { + other.focus(); + h.element.addEventListener("focus", () => { + if (action === "focus") queueMicrotask(() => other.focus()); + else h.element.value = "changed"; + }); + }, + { once: true }, + ); + expect(await h.fill()).toMatchObject({ + code: "cdp_failed", + data: { reason: action === "focus" ? "fill_focus_lost" : "fill_target_changed" }, + }); expect(other.value).toBe(""); expect(h.insert).not.toHaveBeenCalled(); + expect(h.send.mock.calls.filter(([, method]) => method === "DOM.focus")).toHaveLength(2); }); it.each([ @@ -159,14 +229,20 @@ describe("fill result verification", () => { h.element.value = "restored"; }); }); - expect(await h.fill()).toMatchObject({ code: "cdp_failed" }); + expect(await h.fill()).toMatchObject({ + code: "cdp_failed", + data: { reason: "fill_target_changed" }, + }); expect(h.insert).not.toHaveBeenCalled(); }); it("fails when insertText is acknowledged without changing the value", async () => { const h = await setup(); h.insert.mockImplementation(async () => {}); - expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /expected value/ }); + expect(await h.fill()).toMatchObject({ + code: "cdp_failed", + data: { reason: "fill_value_mismatch" }, + }); expect(h.release).toHaveBeenCalledOnce(); }); @@ -179,7 +255,10 @@ describe("fill result verification", () => { }, { once: true }, ); - expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /expected value/ }); + expect(await h.fill()).toMatchObject({ + code: "cdp_failed", + data: { reason: "fill_value_mismatch" }, + }); expect(h.element.value).toBe("hel"); }); @@ -212,34 +291,39 @@ describe("fill result verification", () => { it("does not report success for a target replaced by a change handler", async () => { const h = await setup(); h.element.addEventListener("change", () => h.element.replaceWith(h.element.cloneNode())); - expect(await h.fill()).toMatchObject({ code: "cdp_failed", message: /expected value/ }); + expect(await h.fill()).toMatchObject({ + code: "cdp_failed", + data: { reason: "fill_target_changed" }, + }); }); it.each([ - 1, 2, 3, 4, + 1, 2, 3, 4, 5, ])("rejects script exceptions at phase %s and releases the target", async (phase) => { const h = await setup(); + vi.spyOn(document, "hasFocus").mockReturnValue(false); const run = h.script.getMockImplementation()!; let count = 0; h.script.mockImplementation(async (params) => ++count === phase ? { exceptionDetails: { text: "page secret" } } : run(params), ); const result = await h.fill(); - expect(result).toMatchObject({ code: "cdp_failed" }); + expect(result).toMatchObject({ code: "cdp_failed", data: { reason: "fill_failed" } }); expect(JSON.stringify(result)).not.toContain("page secret"); - if (phase <= 2) expect(h.insert).not.toHaveBeenCalled(); + if (phase <= 3) expect(h.insert).not.toHaveBeenCalled(); expect(h.release).toHaveBeenCalledOnce(); }); - it.each([1, 2, 4])("rejects missing results at phase %s", async (phase) => { + it.each([1, 2, 3, 5])("rejects missing results at phase %s", async (phase) => { const h = await setup(); + vi.spyOn(document, "hasFocus").mockReturnValue(false); const run = h.script.getMockImplementation()!; let count = 0; h.script.mockImplementation(async (params) => ++count === phase ? { result: { type: "undefined" } } : run(params), ); expect(await h.fill()).toMatchObject({ code: "cdp_failed" }); - if (phase <= 2) expect(h.insert).not.toHaveBeenCalled(); + if (phase <= 3) expect(h.insert).not.toHaveBeenCalled(); }); it.each(["input", "change"])("checks state after nested microtasks from %s", async (event) => { @@ -270,7 +354,7 @@ describe("fill result verification", () => { const h = await setup(); h.element.focus(); h.element.setSelectionRange(3, 3); - expect(await h.fill("!", false)).toMatchObject({ value_length: 1 }); + expect(await h.fill("!", false)).toMatchObject({ value_length: 4 }); expect(h.element.value).toBe("old!"); }); @@ -314,18 +398,100 @@ describe("fill result verification", () => { expect(await h.fill("one\n")).toMatchObject({ value_length: 4 }); }); - it("rejects a non-append selection before typing", async () => { + it.each([ + [0, 0], + [1, 1], + [0, 3], + ])("appends after moving selection %s:%s to the end", async (start, end) => { const h = await setup(); h.element.focus(); - h.element.setSelectionRange(1, 1); - expect(await h.fill("!", false)).toMatchObject({ code: "cdp_failed" }); - expect(h.element.value).toBe("old"); + h.element.setSelectionRange(start, end); + expect(await h.fill("🙂", false)).toMatchObject({ value_length: 5 }); + expect(h.element.value).toBe("old🙂"); + }); + + it.each([ + "number", + "email", + ])("appends to %s without setting an unsupported selection API", async (type) => { + const h = await setup(``); + vi.spyOn(h.element, "selectionStart", "get").mockReturnValue(null); + vi.spyOn(h.element, "selectionEnd", "get").mockReturnValue(null); + const selection = vi.spyOn(h.element, "setSelectionRange").mockImplementation(() => { + throw new DOMException("unsupported", "InvalidStateError"); + }); + expect(await h.fill("3", false)).toMatchObject({ value_length: 3 }); + expect(h.element.value).toBe("123"); + expect(selection).not.toHaveBeenCalled(); + }); + + it("rejects invalid number text before clearing", async () => { + const h = await setup(''); + expect(await h.fill("abc")).toMatchObject({ + code: "invalid_params", + data: { reason: "fill_value_invalid" }, + }); + expect(h.element.value).toBe("12"); expect(h.insert).not.toHaveBeenCalled(); }); + it("reports page formatting as unconfirmed without retrying the write", async () => { + const h = await setup(); + h.element.addEventListener("change", () => { + h.element.value = h.element.value.toUpperCase(); + }); + expect(await h.fill()).toMatchObject({ + code: "cdp_failed", + data: { reason: "fill_value_mismatch" }, + }); + expect(h.element.value).toBe("HELLO"); + expect(h.insert).toHaveBeenCalledOnce(); + }); + + it.each([ + ["focus", false], + ["focus", true], + ["value", false], + ["value", true], + ["cancel", false], + ["cancel", true], + ["throw", false], + ["throw", true], + ] as const)("stops when positioning triggers %s (clear=%s) and releases the key", async (action, clearBefore) => { + const h = await setup(); + vi.spyOn(document, "hasFocus").mockReturnValue(false); + const controller = new AbortController(); + const other = document.createElement("input"); + document.body.append(other); + const send = h.send.getMockImplementation()!; + h.send.mockImplementation(async (tabId, method, params) => { + const reply = await send(tabId, method, params); + if ( + method === "Input.dispatchKeyEvent" && + (params as { type: string }).type === "rawKeyDown" + ) { + if (action === "focus") other.focus(); + if (action === "value") h.element.value = "changed"; + if (action === "cancel") controller.abort(); + if (action === "throw") throw new Error("dispatch failed"); + } + return reply; + }); + const result = await h.fill("!", clearBefore, controller.signal); + expect(result).toMatchObject({ code: action === "cancel" ? "cancelled" : "cdp_failed" }); + expect(h.insert).not.toHaveBeenCalled(); + expect(other.value).toBe(""); + expect(h.send).toHaveBeenCalledWith( + 4, + "Input.dispatchKeyEvent", + expect.objectContaining({ type: "keyUp" }), + ); + expect(h.release).toHaveBeenCalledOnce(); + }); + it.each([true, false])("verifies an empty request with clear_before=%s", async (clearBefore) => { const h = await setup(); - expect(await h.fill("", clearBefore)).toMatchObject({ value_length: 0 }); + expect(await h.fill("", clearBefore)).toMatchObject({ value_length: clearBefore ? 0 : 3 }); expect(h.element.value).toBe(clearBefore ? "" : "old"); expect(h.insert).not.toHaveBeenCalled(); }); diff --git a/apps/extension/src/tools/__tests__/interaction.test.ts b/apps/extension/src/tools/__tests__/interaction.test.ts index 7d552b2..7cb1d4d 100644 --- a/apps/extension/src/tools/__tests__/interaction.test.ts +++ b/apps/extension/src/tools/__tests__/interaction.test.ts @@ -541,13 +541,16 @@ describe("handleHover", () => { }); function successfulFillScript(params: unknown) { - const args = (params as { arguments?: Array<{ value: unknown }> }).arguments ?? []; + const script = params as { arguments?: Array<{ value: unknown }>; functionDeclaration: string }; + const args = script.arguments ?? []; return { result: { value: args.length === 2 - ? { before: "", expected: args[0].value, valueLength: String(args[0].value).length } - : true, + ? { before: "", expected: args[0].value } + : script.functionDeclaration.startsWith("function(expected)") + ? { connected: true, matches: true, valueLength: String(args[0].value).length } + : "ready", }, }; } @@ -613,7 +616,7 @@ describe("handleFill", () => { expect(res.used_ref).toBe("e1"); const insert = fake.sent.find((c) => c.method === "Input.insertText"); expect(insert?.params).toEqual({ text: "hello" }); - // Preparation, readiness, notifications, and final verification. + // Foreground replacement needs no extra caret-positioning round trip. const callFns = fake.sent.filter((c) => c.method === "Runtime.callFunctionOn"); expect(callFns).toHaveLength(4); }); @@ -634,6 +637,7 @@ describe("handleFill", () => { if (args.length === 2) expect(args[1].value).toBe(false); return successfulFillScript(p); }, + "Input.dispatchKeyEvent": () => ({}), "Runtime.releaseObject": () => ({}), "Input.insertText": () => ({}), }); diff --git a/apps/extension/src/tools/interaction.ts b/apps/extension/src/tools/interaction.ts index 5a48349..9e36c2d 100644 --- a/apps/extension/src/tools/interaction.ts +++ b/apps/extension/src/tools/interaction.ts @@ -609,11 +609,28 @@ type SelectMutationResult = // tool.fill // --------------------------------------------------------------------------- -interface FillPreparation { - before?: string; - expected?: string; - valueLength?: number; - error?: string; +type FillFailureReason = + | "target_not_fillable" + | "fill_value_invalid" + | "fill_target_changed" + | "fill_focus_lost" + | "fill_value_mismatch" + | "fill_failed"; + +type FillPreparation = + | { before: string; expected: string } + | { reason: FillFailureReason; error: string }; + +type FillReadiness = "ready" | "background" | "fill_target_changed" | "fill_focus_lost"; + +function fillError(reason: FillFailureReason, message: string): RpcError { + return rpcError( + reason === "target_not_fillable" || reason === "fill_value_invalid" + ? "invalid_params" + : "cdp_failed", + reason, + message, + ); } interface FillScriptReply { @@ -690,12 +707,8 @@ export async function handleFill( if (throwIfAborted(deps.signal)) { return { code: "cancelled", message: "fill aborted" }; } - await nodeCdp.send(target.tabId, "DOM.focus", { backendNodeId: node.backendNodeId }); } catch (err) { - return { - code: "cdp_failed", - message: err instanceof Error ? err.message : String(err), - }; + return fillError("fill_failed", err instanceof Error ? err.message : String(err)); } if (throwIfAborted(deps.signal)) { @@ -708,6 +721,31 @@ export async function handleFill( const clearBefore = params.clear_before ?? true; try { + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + try { + await nodeCdp.send(target.tabId, "DOM.focus", { backendNodeId: node.backendNodeId }); + } catch (error) { + // Chrome rejects DOM.focus for disabled controls, including inherited + // fieldset state. Classify the live target before treating this as a + // browser failure; the normal path needs no extra round trip. + const editable = await nodeCdp.send>( + target.tabId, + "Runtime.callFunctionOn", + { objectId, functionDeclaration: FILL_EDITABLE_FUNCTION, returnByValue: true }, + ); + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + if (!editable.exceptionDetails && editable.result?.value === false) { + return fillError( + "target_not_fillable", + "fill target is not editable or its input type is unsupported", + ); + } + throw error; + } if (throwIfAborted(deps.signal)) { return { code: "cancelled", message: "fill aborted" }; } @@ -721,14 +759,9 @@ export async function handleFill( functionDeclaration: `function(value, clearBefore) { const tag = this.tagName.toLowerCase(); const native = tag === 'input' || tag === 'textarea'; - if (!(${FILL_EDITABLE_FUNCTION}).call(this)) return { error: 'fill target is not editable or its input type is unsupported' }; - const focused = () => this.getRootNode().activeElement === this && this.ownerDocument.hasFocus(); - if (!focused()) return { error: 'fill target does not have focus' }; + if (!(${FILL_EDITABLE_FUNCTION}).call(this)) return { reason: 'target_not_fillable', error: 'fill target is not editable or its input type is unsupported' }; + if (this.getRootNode().activeElement !== this) return { reason: 'fill_focus_lost', error: 'fill target does not have focus' }; const before = clearBefore ? '' : (${FILL_VALUE_FUNCTION}).call(this); - if (!clearBefore && value !== '' && native && typeof this.selectionStart === 'number' && - (this.selectionStart !== before.length || this.selectionEnd !== before.length)) { - return { error: 'fill append requires the caret at the end of the field' }; - } let expected = before + value; if (native) { // Use the browser's own value sanitization (e.g. textarea @@ -742,11 +775,11 @@ export async function handleFill( if (tag === 'input') expected = expected.replace(/\\r\\n|\\r|\\n/g, ' '); normalizer.value = expected; if (expected !== '' && normalizer.value === '' && this.type === 'number') { - return { error: 'fill value is not valid for a number input' }; + return { reason: 'fill_value_invalid', error: 'fill value is not valid for a number input' }; } expected = normalizer.value; if (this.type !== 'number' && this.maxLength >= 0 && expected.length > this.maxLength) { - return { error: 'fill value exceeds the target maxlength' }; + return { reason: 'fill_value_invalid', error: 'fill value exceeds the target maxlength' }; } } else { expected = expected.replace(/\\r\\n?/g, '\\n'); @@ -760,7 +793,7 @@ export async function handleFill( } this.dispatchEvent(new Event('input', { bubbles: true })); } - return { before, expected, valueLength: Math.max(0, expected.length - before.length) }; + return { before, expected }; }`, arguments: [{ value: params.value }, { value: clearBefore }], returnByValue: true, @@ -770,44 +803,95 @@ export async function handleFill( return { code: "cancelled", message: "fill aborted" }; } if (prepared.exceptionDetails) { - return { code: "cdp_failed", message: "fill preparation script failed" }; + return fillError("fill_failed", "fill preparation script failed"); } const preparation = prepared.result?.value; - if ( - preparation?.error || - typeof preparation?.before !== "string" || - typeof preparation?.expected !== "string" || - typeof preparation.valueLength !== "number" - ) { - return { - code: "cdp_failed", - message: preparation?.error ?? "fill preparation returned an unexpected result", - }; + if (preparation && "error" in preparation) { + return fillError(preparation.reason, preparation.error); + } + if (typeof preparation?.before !== "string" || typeof preparation?.expected !== "string") { + return fillError("fill_failed", "fill preparation returned an unexpected result"); } // A separate call runs after the clearing event's microtasks drain. - // Confirm the target before sending input to the focused element. - const ready = await nodeCdp.send>( - target.tabId, - "Runtime.callFunctionOn", - { - objectId, - functionDeclaration: `function(before) { - return (${FILL_EDITABLE_FUNCTION}).call(this) && - this.getRootNode().activeElement === this && this.ownerDocument.hasFocus() && - (${FILL_VALUE_FUNCTION}).call(this) === before; - }`, - arguments: [{ value: preparation.before }], - returnByValue: true, - }, - ); + // DOM focus works in background tabs. Use document.hasFocus() only to + // detect deferred focus events, never to reject background input. + const checkReady = async (): Promise => { + const reply = await nodeCdp.send>( + target.tabId, + "Runtime.callFunctionOn", + { + objectId, + functionDeclaration: `function(before) { + if (!(${FILL_EDITABLE_FUNCTION}).call(this) || + (${FILL_VALUE_FUNCTION}).call(this) !== before) return 'fill_target_changed'; + if (this.getRootNode().activeElement !== this) return 'fill_focus_lost'; + return this.ownerDocument.hasFocus() ? 'ready' : 'background'; + }`, + arguments: [{ value: preparation.before }], + returnByValue: true, + }, + ); + const state = reply.result?.value; + if ( + reply.exceptionDetails || + (state !== "ready" && + state !== "background" && + state !== "fill_target_changed" && + state !== "fill_focus_lost") + ) { + return fillError( + "fill_failed", + "fill readiness script failed or returned an unexpected result", + ); + } + return state; + }; + let ready = await checkReady(); if (throwIfAborted(deps.signal)) { return { code: "cancelled", message: "fill aborted" }; } - if (ready.exceptionDetails || ready.result?.value !== true) { - return { - code: "cdp_failed", - message: "fill target changed, lost focus, or could not be checked before typing", - }; + // Restore focus once only if clearing left this same target editable + // and unchanged. Recheck after focus handlers have run. + if (clearBefore && ready === "fill_focus_lost") { + await nodeCdp.send(target.tabId, "DOM.focus", { backendNodeId: node.backendNodeId }); + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + ready = await checkReady(); + } + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + if (typeof ready !== "string") return ready; + if (ready !== "ready" && ready !== "background") { + return fillError(ready, "fill target changed or lost focus before typing"); + } + if (params.value !== "" && (!clearBefore || ready === "background")) { + // Native editing commands also support number/email inputs, whose + // selection APIs cannot set a caret, and multiline contenteditables. + // Also do this for background replacement: CDP input focuses the + // renderer, delivering deferred focus events before it inserts text. + // Use a command-only event so this does not invoke an End shortcut. + try { + await deps.cdp.send(target.tabId, "Input.dispatchKeyEvent", { + type: "rawKeyDown", + commands: ["moveToEndOfDocument"], + }); + } finally { + await deps.cdp.send(target.tabId, "Input.dispatchKeyEvent", { type: "keyUp" }); + } + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + // Focus and key handlers can change the target or move focus too. + ready = await checkReady(); + if (throwIfAborted(deps.signal)) { + return { code: "cancelled", message: "fill aborted" }; + } + if (typeof ready !== "string") return ready; + if (ready !== "ready" && ready !== "background") { + return fillError(ready, "fill target changed or lost focus while positioning the caret"); + } } // CDP `Input.insertText` handles IME / multi-byte input out of the // box, much more reliably than per-key `dispatchKeyEvent`. @@ -835,42 +919,51 @@ export async function handleFill( return { code: "cancelled", message: "fill aborted" }; } if (notified.exceptionDetails) { - return { code: "cdp_failed", message: "fill notification script failed" }; + return fillError("fill_failed", "fill notification script failed"); } - const verified = await nodeCdp.send>( - target.tabId, - "Runtime.callFunctionOn", - { - objectId, - functionDeclaration: `function(expected) { - return this.isConnected && (${FILL_VALUE_FUNCTION}).call(this) === expected; + const verified = await nodeCdp.send< + FillScriptReply<{ connected: boolean; matches: boolean; valueLength: number }> + >(target.tabId, "Runtime.callFunctionOn", { + objectId, + functionDeclaration: `function(expected) { + const value = (${FILL_VALUE_FUNCTION}).call(this); + return { connected: this.isConnected, matches: value === expected, valueLength: value.length }; }`, - arguments: [{ value: preparation.expected }], - returnByValue: true, - }, - ); + arguments: [{ value: preparation.expected }], + returnByValue: true, + }); if (throwIfAborted(deps.signal)) { return { code: "cancelled", message: "fill aborted" }; } - if (verified.exceptionDetails || verified.result?.value !== true) { - return { - code: "cdp_failed", - message: verified.exceptionDetails - ? "fill verification script failed" - : "fill could not verify the expected value; observe the page before retrying", - }; + const verification = verified.result?.value; + if ( + verified.exceptionDetails || + typeof verification?.connected !== "boolean" || + typeof verification?.matches !== "boolean" || + typeof verification?.valueLength !== "number" + ) { + return fillError( + "fill_failed", + "fill verification script failed or returned an unexpected result", + ); + } + if (!verification.connected) { + return fillError("fill_target_changed", "fill target was removed or replaced during input"); + } + if (!verification.matches) { + return fillError( + "fill_value_mismatch", + "fill could not verify the expected value; observe the page before deciding whether to retry", + ); } return attachDialogs(deps.cdp, target.tabId, dialogCursor, { tab_id: target.tabId, used_ref: node.usedRef, used_selector: node.usedSelector, - value_length: preparation.valueLength, + value_length: verification.valueLength, }); } catch (err) { - return { - code: "cdp_failed", - message: err instanceof Error ? err.message : String(err), - }; + return fillError("fill_failed", err instanceof Error ? err.message : String(err)); } finally { await nodeCdp.send(target.tabId, "Runtime.releaseObject", { objectId }).catch(() => undefined); } diff --git a/apps/extension/src/transport/types.ts b/apps/extension/src/transport/types.ts index fc9968a..f604993 100644 --- a/apps/extension/src/transport/types.ts +++ b/apps/extension/src/transport/types.ts @@ -26,6 +26,11 @@ export type RpcErrorReason = | "ref_not_found" | "selector_not_found" | "target_not_fillable" + | "fill_value_invalid" + | "fill_target_changed" + | "fill_focus_lost" + | "fill_value_mismatch" + | "fill_failed" | "target_not_select" | "option_not_found" | "single_select_value_count" diff --git a/crates/bsk-cli/skill/SKILL.md b/crates/bsk-cli/skill/SKILL.md index 357625c..f27ab2f 100644 --- a/crates/bsk-cli/skill/SKILL.md +++ b/crates/bsk-cli/skill/SKILL.md @@ -172,6 +172,10 @@ intended. Read `bsk upload --help` and `bsk download --help` for all flags and e - Stale ref: observe again and retry the intended action once. - Unknown tab or session: list current tabs/sessions; never guess identifiers. - Timeout: inspect current page state before deciding whether one longer purposeful wait is useful. +- Fill result unconfirmed (`fill_value_mismatch`): observe the field first; the page may have + formatted the value. Continue if the visible result satisfies the user's intent. Otherwise correct + the remaining difference; do not blindly repeat fill or immediately request human help. For other + fill errors, follow the returned hint and inspect current state before retrying. - Unsupported command: continue with available capabilities; suggest updating only when the missing command is necessary. - Unrecoverable failure: report the blocker and stop the session in a finally-style path. diff --git a/crates/bsk-cli/src/cli/render_error.rs b/crates/bsk-cli/src/cli/render_error.rs index 01f80dd..ef9312f 100644 --- a/crates/bsk-cli/src/cli/render_error.rs +++ b/crates/bsk-cli/src/cli/render_error.rs @@ -43,6 +43,11 @@ pub mod reason { pub const REF_NOT_FOUND: &str = "ref_not_found"; pub const SELECTOR_NOT_FOUND: &str = "selector_not_found"; pub const TARGET_NOT_FILLABLE: &str = "target_not_fillable"; + pub const FILL_VALUE_INVALID: &str = "fill_value_invalid"; + pub const FILL_TARGET_CHANGED: &str = "fill_target_changed"; + pub const FILL_FOCUS_LOST: &str = "fill_focus_lost"; + pub const FILL_VALUE_MISMATCH: &str = "fill_value_mismatch"; + pub const FILL_FAILED: &str = "fill_failed"; pub const TARGET_NOT_SELECT: &str = "target_not_select"; pub const OPTION_NOT_FOUND: &str = "option_not_found"; pub const SINGLE_SELECT_VALUE_COUNT: &str = "single_select_value_count"; @@ -247,7 +252,42 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend (ErrorCode::InvalidParams, reason::TARGET_NOT_FILLABLE) => RenderInfo { summary: "target element is not fillable", hint: Some( - "choose an input, textarea, or contenteditable element from the latest snapshot", + "observe the page and choose an enabled, editable text input, textarea, or contenteditable; use the appropriate interaction for other control types", + ), + exit_code: base.exit_code, + }, + (ErrorCode::InvalidParams, reason::FILL_VALUE_INVALID) => RenderInfo { + summary: "the requested text does not fit the field constraints", + hint: Some( + "check the field's input type and maxlength; use a compatible value without silently truncating or changing the user's intended data", + ), + exit_code: base.exit_code, + }, + (ErrorCode::CdpFailed, reason::FILL_TARGET_CHANGED) => RenderInfo { + summary: "the fill target changed during the action", + hint: Some( + "observe the current page and field value; if the intended result is missing, use a fresh editable target before retrying", + ), + exit_code: base.exit_code, + }, + (ErrorCode::CdpFailed, reason::FILL_FOCUS_LOST) => RenderInfo { + summary: "the page moved focus away from the fill target", + hint: Some( + "observe the page for a dialog or rerender and resolve it before retrying; fill manages field focus and does not require bringing the browser to the foreground", + ), + exit_code: base.exit_code, + }, + (ErrorCode::CdpFailed, reason::FILL_VALUE_MISMATCH) => RenderInfo { + summary: "the fill result could not be confirmed", + hint: Some( + "observe the field before retrying; the page may have formatted the value. Continue if the visible result satisfies the user's intent; otherwise correct the remaining difference. Do not blindly repeat fill or immediately request human help", + ), + exit_code: base.exit_code, + }, + (ErrorCode::CdpFailed, reason::FILL_FAILED) => RenderInfo { + summary: "fill encountered a browser or page-script error", + hint: Some( + "observe the page before retrying because the field may already have changed; retry only if the intended result is missing and the target is still available", ), exit_code: base.exit_code, }, @@ -442,6 +482,56 @@ mod tests { ErrorCode::NoBrowserConnected, ]; + #[test] + fn fill_reasons_provide_recovery_without_changing_exit_codes() { + for (code, reason, hint_fragment) in [ + ( + ErrorCode::InvalidParams, + reason::TARGET_NOT_FILLABLE, + "enabled, editable", + ), + ( + ErrorCode::InvalidParams, + reason::FILL_VALUE_INVALID, + "maxlength", + ), + ( + ErrorCode::CdpFailed, + reason::FILL_TARGET_CHANGED, + "fresh editable target", + ), + ( + ErrorCode::CdpFailed, + reason::FILL_FOCUS_LOST, + "does not require bringing the browser", + ), + ( + ErrorCode::CdpFailed, + reason::FILL_VALUE_MISMATCH, + "Continue if the visible result satisfies", + ), + ( + ErrorCode::CdpFailed, + reason::FILL_FAILED, + "may already have changed", + ), + ] { + let data = serde_json::json!({ "reason": reason }); + let info = info_for_error(code, Some(&data)); + assert!(info.hint.unwrap().contains(hint_fragment), "{reason}"); + assert_eq!(info.exit_code, info_for(code).exit_code); + } + let mismatch = serde_json::json!({ "reason": reason::FILL_VALUE_MISMATCH }); + let info = info_for_error(ErrorCode::CdpFailed, Some(&mismatch)); + assert_eq!(info.summary, "the fill result could not be confirmed"); + assert!(info.hint.unwrap().contains("Do not blindly repeat fill")); + let unknown = serde_json::json!({ "reason": "future_fill_reason" }); + assert_eq!( + info_for_error(ErrorCode::CdpFailed, Some(&unknown)), + info_for(ErrorCode::CdpFailed) + ); + } + #[test] fn element_not_visible_overrides_permission_denied_copy() { let data = serde_json::json!({ "reason": reason::ELEMENT_NOT_VISIBLE }); diff --git a/packages/dsh-plugin-browserskill/skill/SKILL.md b/packages/dsh-plugin-browserskill/skill/SKILL.md index a2d9c4e..464a24e 100644 --- a/packages/dsh-plugin-browserskill/skill/SKILL.md +++ b/packages/dsh-plugin-browserskill/skill/SKILL.md @@ -97,6 +97,10 @@ Continue from returned sequence cursors instead of rereading the same buffer. - Unknown tab: list tabs instead of guessing. - Unknown session: list owned sessions or start one; never try foreign ids. - Timeout: inspect current state before deciding whether one longer purposeful wait is useful. +- Fill result unconfirmed: observe the field first; the page may have formatted the value. Continue + if the visible result satisfies the user's intent. Otherwise correct the remaining difference; + do not blindly repeat fill or immediately request human help. For other fill errors, follow the + returned hint and inspect current state before retrying. - Unrecoverable failure: report the blocker and stop the owned session. Arbitrary page-script evaluation and interaction recording are intentionally unsupported. Do not diff --git a/packages/dsh-plugin-browserskill/tests/runner.test.ts b/packages/dsh-plugin-browserskill/tests/runner.test.ts index 37b2de1..ad6d81a 100644 --- a/packages/dsh-plugin-browserskill/tests/runner.test.ts +++ b/packages/dsh-plugin-browserskill/tests/runner.test.ts @@ -124,6 +124,28 @@ describe("parseBskJson", () => { expect(() => parseBskJson({ ...base, code: 1, stderr: "boom" }, "x")).toThrow(/boom/); }); + it("surfaces fill recovery guidance to the model without retrying", async () => { + const hint = + "observe the field before retrying; the page may have formatted the value. Continue if the visible result satisfies the user's intent"; + const reply = { + ...base, + code: 3, + stdout: JSON.stringify({ + code: "cdp_failed", + message: "fill could not verify the expected value", + data: { reason: "fill_value_mismatch" }, + hint, + }), + }; + let calls = 0; + const result = await runWithSessionBusyRetry(async () => { + calls++; + return reply; + }); + expect(calls).toBe(1); + expect(() => parseBskJson(result, "fill")).toThrow(hint); + }); + it("reports killed-by-interrupt children (null exit code) as interrupted", () => { expect(() => parseBskJson({ ...base, code: null }, "navigate")).toThrow(/interrupted/); }); diff --git a/skill/SKILL.md b/skill/SKILL.md index 357625c..f27ab2f 100644 --- a/skill/SKILL.md +++ b/skill/SKILL.md @@ -172,6 +172,10 @@ intended. Read `bsk upload --help` and `bsk download --help` for all flags and e - Stale ref: observe again and retry the intended action once. - Unknown tab or session: list current tabs/sessions; never guess identifiers. - Timeout: inspect current page state before deciding whether one longer purposeful wait is useful. +- Fill result unconfirmed (`fill_value_mismatch`): observe the field first; the page may have + formatted the value. Continue if the visible result satisfies the user's intent. Otherwise correct + the remaining difference; do not blindly repeat fill or immediately request human help. For other + fill errors, follow the returned hint and inspect current state before retrying. - Unsupported command: continue with available capabilities; suggest updating only when the missing command is necessary. - Unrecoverable failure: report the blocker and stop the session in a finally-style path. From 321494d93fac0c3d39985b65e0b510220dd91dbe Mon Sep 17 00:00:00 2001 From: drakezhang Date: Mon, 7 Sep 2026 15:56:13 +0800 Subject: [PATCH 3/3] test(fill): cover caret positioning and clarify append behavior --- .../src/tools/__tests__/fill.test.ts | 55 ++++++++++++++++--- packages/dsh-plugin-browserskill/src/tools.ts | 4 +- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/apps/extension/src/tools/__tests__/fill.test.ts b/apps/extension/src/tools/__tests__/fill.test.ts index 53adbf5..c7dba29 100644 --- a/apps/extension/src/tools/__tests__/fill.test.ts +++ b/apps/extension/src/tools/__tests__/fill.test.ts @@ -35,13 +35,24 @@ async function setup(markup = '') { return { exceptionDetails: { text: "test page script failed" } }; } }); + // Number/email controls have a browser caret even though their selection + // getters return null. Model that caret instead of assuming it is at the end. + const hiddenCaret = { start: 0, end: 0 }; + const moveToEnd = vi.fn(async () => { + if (typeof element.selectionStart === "number") { + element.setSelectionRange(element.value.length, element.value.length); + } else { + hiddenCaret.start = hiddenCaret.end = element.value.length; + } + }); const insert = vi.fn(async (text: string) => { const focused = document.activeElement as HTMLInputElement; if (focused.readOnly || focused.disabled) return; - const start = focused.selectionStart ?? focused.value.length; - const end = focused.selectionEnd ?? start; + const start = focused.selectionStart ?? hiddenCaret.start; + const end = focused.selectionEnd ?? hiddenCaret.end; const value = focused.value.slice(0, start) + text + focused.value.slice(end); focused.value = focused.maxLength >= 0 ? value.slice(0, focused.maxLength) : value; + hiddenCaret.start = hiddenCaret.end = Math.min(start + text.length, focused.value.length); focused.dispatchEvent(new Event("input", { bubbles: true })); }); const release = vi.fn(async () => ({})); @@ -68,9 +79,7 @@ async function setup(markup = '') { return {}; case "Input.dispatchKeyEvent": if ((params as { commands?: string[] }).commands?.includes("moveToEndOfDocument")) { - if (typeof element.selectionStart === "number") { - element.setSelectionRange(element.value.length, element.value.length); - } + await moveToEnd(); } return {}; case "Runtime.releaseObject": @@ -81,6 +90,8 @@ async function setup(markup = '') { }); return { element, + hiddenCaret, + moveToEnd, script, insert, release, @@ -410,11 +421,21 @@ describe("fill result verification", () => { expect(h.element.value).toBe("old🙂"); }); - it.each([ - "number", - "email", - ])("appends to %s without setting an unsupported selection API", async (type) => { + it.each( + ["number", "email"].flatMap((type) => + [ + [0, 0], + [1, 1], + [0, 2], + ].map(([start, end]) => ({ type, start, end })), + ), + )("appends to $type from selection $start:$end without using its unsupported API", async ({ + type, + start, + end, + }) => { const h = await setup(``); + Object.assign(h.hiddenCaret, { start, end }); vi.spyOn(h.element, "selectionStart", "get").mockReturnValue(null); vi.spyOn(h.element, "selectionEnd", "get").mockReturnValue(null); const selection = vi.spyOn(h.element, "setSelectionRange").mockImplementation(() => { @@ -425,6 +446,22 @@ describe("fill result verification", () => { expect(selection).not.toHaveBeenCalled(); }); + it.each([ + "number", + "email", + ])("does not report successful %s append when the caret command is ignored", async (type) => { + const h = await setup(``); + vi.spyOn(h.element, "selectionStart", "get").mockReturnValue(null); + vi.spyOn(h.element, "selectionEnd", "get").mockReturnValue(null); + h.moveToEnd.mockResolvedValueOnce(undefined); + expect(await h.fill("3", false)).toMatchObject({ + code: "cdp_failed", + data: { reason: "fill_value_mismatch" }, + }); + expect(h.element.value).toBe("312"); + expect(h.insert).toHaveBeenCalledOnce(); + }); + it("rejects invalid number text before clearing", async () => { const h = await setup(''); expect(await h.fill("abc")).toMatchObject({ diff --git a/packages/dsh-plugin-browserskill/src/tools.ts b/packages/dsh-plugin-browserskill/src/tools.ts index 9bdbabe..db8738f 100644 --- a/packages/dsh-plugin-browserskill/src/tools.ts +++ b/packages/dsh-plugin-browserskill/src/tools.ts @@ -636,7 +636,9 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): session: SESSION_PARAM, noClear: { type: "boolean", - description: "Skip the default wipe-the-field-first pass (append instead).", + description: + "Append to the end of the existing value instead of clearing it. " + + "The tool moves the caret automatically; no prior click or selection is needed.", }, }, output: {