diff options
| author | amankumarsingh77 <amankumarsingh7702@gmail.com> | 2026-07-28 19:55:53 +0530 |
|---|---|---|
| committer | Aman Kumar <145339995+amankumarsingh77@users.noreply.github.com> | 2026-08-02 19:12:46 +0530 |
| commit | cfa1892e17e3696da004c71a4942d5b12186b12c (patch) | |
| tree | 228b15fc7ada4193bdcabeaf913421a2958215ce /src | |
| parent | 254de089ee468795bac175d99e09ddaec4ac1469 (diff) | |
fix(domains): one fill engine, so browser_fill stops lying about checkboxes
browser_fill and browser_fill_form carried separate in-page implementations that
disagreed on the same element. The worst case was silent: browser_fill on a
checkbox fired the HTMLInputElement value setter, which writes the submit value
and leaves `checked` alone, then reported status ok — a no-op the agent had no
reason to doubt. Both tools now share one engine, with browser_fill's documented
<select> rejection kept as an explicit pre-check and per-tool focus behaviour kept
as a parameter.
Deliberate, user-approved behaviour changes:
- browser_fill on a DISABLED input now returns an error instead of success. The
old success was the bug; browser_fill_form already refused it.
- browser_fill on a checkbox/radio now toggles `checked` instead of the value.
- browser_fill_form on a contenteditable now uses execCommand("insertText"),
so it drives ProseMirror/Slack-style editors as browser_fill already did.
browser_fill keeps appending interactiveDiff and republishing refs; fill_form
still does not.
Report: docs/superpowers/reports/2026-07-26-bug-audit.md#f5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Diffstat (limited to 'src')
| -rw-r--r-- | src/domains/fill-engine.ts | 95 | ||||
| -rw-r--r-- | src/domains/form-batch.ts | 55 | ||||
| -rw-r--r-- | src/domains/form.ts | 115 |
3 files changed, 143 insertions, 122 deletions
diff --git a/src/domains/fill-engine.ts b/src/domains/fill-engine.ts new file mode 100644 index 0000000..32102fe --- /dev/null +++ b/src/domains/fill-engine.ts @@ -0,0 +1,95 @@ +/** + * The one in-page fill implementation, shared by browser_fill and + * browser_fill_form so the two can no longer disagree about the same element. + * + * `this` is the target element and `value` is the requested value. Two call-site + * differences are parameters rather than forks: + * rejectSelect — browser_fill refuses a <select> and points at + * browser_select_option; browser_fill_form fills it. + * focusFirst — browser_fill_form focuses the field first; browser_fill + * documents that it does not move focus. + * + * Returns a PageResult (see form.ts): { ok, reason?, kind?, value?, text?, + * checked?, changed?, options?, tag? }. + */ + +export type FillOptions = { + readonly rejectSelect: boolean; + readonly focusFirst: boolean; +}; + +const ENGINE = ` + const el = this; + if (!el || el.nodeType !== 1) return { ok: false, reason: "ref does not point to an element" }; + const tagName = el.tagName || ""; + const tag = tagName.toLowerCase(); + const type = (el.type || "").toLowerCase(); + + if (__rejectSelect && tag === "select") { + return { ok: false, reason: "element is a <select>", kind: "select", tag: tagName }; + } + if (el.disabled) return { ok: false, reason: "element is disabled", tag: tagName }; + + const fire = function (t) { el.dispatchEvent(new Event(t, { bubbles: true })); }; + if (__focusFirst) { try { el.focus(); } catch (e) {} } + + if (tag === "select") { + const want = String(value); + let matched = null; + for (let i = 0; i < el.options.length; i++) { + const o = el.options[i]; + if (o.value === want || o.label === want || o.text === want) { matched = o; break; } + } + if (!matched) { + const opts = []; + for (let i = 0; i < el.options.length; i++) opts.push({ value: el.options[i].value, text: el.options[i].text }); + return { ok: false, reason: "no matching option", kind: "select", options: opts, tag: tagName }; + } + el.value = matched.value; + fire("input"); fire("change"); + return { ok: true, kind: "select", value: el.value, text: matched.text, tag: tagName }; + } + + if (tag === "input" && (type === "checkbox" || type === "radio")) { + const want = value === true || value === "true" || value === "on" || value === 1; + let changed = false; + if (el.checked !== want) { + try { el.click(); } catch (e) {} + if (el.checked !== want) { el.checked = want; fire("input"); fire("change"); } + changed = true; + } + return { ok: true, kind: type, checked: el.checked, changed: changed, tag: tagName }; + } + + if (el.isContentEditable) { + try { el.focus(); } catch (e) {} + let done = false; + try { + const sel = window.getSelection(); + if (sel) { sel.removeAllRanges(); const rng = document.createRange(); rng.selectNodeContents(el); sel.addRange(rng); } + done = document.execCommand("insertText", false, String(value)); + } catch (e) { done = false; } + if (!done) { + el.textContent = String(value); + el.dispatchEvent(new InputEvent("input", { bubbles: true })); + } + return { ok: true, kind: "contenteditable", value: el.textContent || "", tag: tagName }; + } + + if (tag === "input" || tag === "textarea") { + const proto = tag === "textarea" ? window.HTMLTextAreaElement.prototype : window.HTMLInputElement.prototype; + const desc = Object.getOwnPropertyDescriptor(proto, "value"); + if (desc && desc.set) desc.set.call(el, String(value)); + else el.value = String(value); + fire("input"); fire("change"); + return { ok: true, kind: el.type || tag, value: el.value, tag: tagName }; + } + + return { ok: false, reason: "element is not a fillable field (tag=" + tag + ")", tag: tagName }; +`; + +export const fillBody = (opts: FillOptions): string => + `const __rejectSelect = ${opts.rejectSelect}; const __focusFirst = ${opts.focusFirst};\n${ENGINE}`; + +export const fillDeclaration = (opts: FillOptions): string => + `function (value) { ${fillBody(opts)} }`; diff --git a/src/domains/form-batch.ts b/src/domains/form-batch.ts index c4b4df6..1d49cf6 100644 --- a/src/domains/form-batch.ts +++ b/src/domains/form-batch.ts @@ -1,61 +1,10 @@ import { Type } from "typebox"; import { type Result, err, ok } from "../util/result"; import { defineBrowserTool, type ToolErr, type ToolOk } from "../util/tool"; +import { fillDeclaration } from "./fill-engine"; import { detailsOf, resolveAndCall } from "./form"; -const FILL_FN = ` -function(value) { - const el = this; - if (!el || el.nodeType !== 1) return { ok: false, reason: "ref does not point to an element" }; - if (el.disabled) return { ok: false, reason: "element is disabled" }; - const tag = (el.tagName || "").toLowerCase(); - const type = (el.type || "").toLowerCase(); - try { el.focus(); } catch (e) {} - const fire = function(t) { el.dispatchEvent(new Event(t, { bubbles: true })); }; - - if (tag === "select") { - const want = String(value); - let matched = null; - for (let i = 0; i < el.options.length; i++) { - const o = el.options[i]; - if (o.value === want || o.label === want || o.text === want) { matched = o; break; } - } - if (!matched) { - const opts = []; - for (let i = 0; i < el.options.length; i++) opts.push({ value: el.options[i].value, text: el.options[i].text }); - return { ok: false, reason: "no matching option", kind: "select", options: opts }; - } - el.value = matched.value; - fire("input"); fire("change"); - return { ok: true, kind: "select", value: el.value, text: matched.text }; - } - - if (tag === "input" && (type === "checkbox" || type === "radio")) { - const want = value === true || value === "true" || value === "on" || value === 1; - if (el.checked !== want) { - try { el.click(); } catch (e) {} - if (el.checked !== want) { el.checked = want; fire("input"); fire("change"); } - } - return { ok: true, kind: type, checked: el.checked }; - } - - if (el.isContentEditable) { - el.textContent = String(value); - fire("input"); fire("change"); - return { ok: true, kind: "contenteditable", value: el.textContent }; - } - - if (tag === "input" || tag === "textarea") { - const proto = tag === "textarea" ? window.HTMLTextAreaElement.prototype : window.HTMLInputElement.prototype; - const desc = Object.getOwnPropertyDescriptor(proto, "value"); - if (desc && desc.set) desc.set.call(el, String(value)); - else el.value = String(value); - fire("input"); fire("change"); - return { ok: true, kind: tag, value: el.value }; - } - - return { ok: false, reason: "element is not a fillable field (tag=" + tag + ")" }; -}`; +const FILL_FN = fillDeclaration({ rejectSelect: false, focusFirst: true }); const FieldValue = Type.Union([Type.String(), Type.Boolean()], { description: "Value to set. String for text inputs / textareas / selects / contenteditable; boolean for checkboxes & radios.", diff --git a/src/domains/form.ts b/src/domains/form.ts index 26268d9..d8cbf87 100644 --- a/src/domains/form.ts +++ b/src/domains/form.ts @@ -4,6 +4,7 @@ import { safeJs } from "../util/js-template"; import { type Result, err, ok } from "../util/result"; import { defineBrowserTool, type ToolErr, type ToolOk } from "../util/tool"; import { cdpCall } from "./cdp-call"; +import { fillBody } from "./fill-engine"; import { interactiveDiff, resolveRefToObjectId } from "./ref-resolve"; const FillArgs = Type.Object({ @@ -29,7 +30,13 @@ const FillArgs = Type.Object({ */ const runOnElement = async ( client: BrowserClient, - opts: { ref?: string | undefined; selector?: string | undefined; fnBody: string; arg: unknown }, + opts: { + ref?: string | undefined; + selector?: string | undefined; + fnBody: string; + arg: unknown; + notFound?: unknown; + }, ): Promise<Result<unknown, ToolErr>> => { if (opts.ref !== undefined) { const objectId = await resolveRefToObjectId(client, opts.ref); @@ -52,7 +59,7 @@ const runOnElement = async ( const prelude = safeJs` (() => { const el = document.querySelector(${opts.selector}); - if (!el) return { status: "not_found" }; + if (!el) return ${opts.notFound ?? { status: "not_found" }}; const __arg = ${opts.arg}; return (function (arg) {`; const expr = `${prelude} ${opts.fnBody} }).call(el, __arg); })()`; @@ -61,75 +68,21 @@ const runOnElement = async ( return ok(r.data); }; -/** - * Result shape returned by the in-page fill script. Discriminated by `status` - * so the handler can map page-level outcomes to typed ToolErr kinds. - */ -type FillResult = - | { status: "ok"; tag: string; kind: string; value: string } - | { status: "not_found" } - | { status: "not_fillable"; tag: string }; +const FILL_BODY = `const value = arg;\n${fillBody({ rejectSelect: true, focusFirst: false })}`; -const isFillResult = (v: unknown): v is FillResult => { - if (typeof v !== "object" || v === null || !("status" in v)) return false; - if (v.status === "not_found") return true; - if (v.status === "not_fillable") return "tag" in v && typeof v.tag === "string"; - if (v.status === "ok") { - return "tag" in v && typeof v.tag === "string" - && "kind" in v && typeof v.kind === "string" - && "value" in v && typeof v.value === "string"; - } - return false; -}; - -// Element-scoped fill logic. `this` is the target element, `arg` is the value. -// Write via the native prototype value setter so React's _valueTracker -// registers the change — assigning el.value directly does NOT, which is why -// controlled inputs revert on re-render. Shared verbatim by the ref path -// (callFunctionOn) and the selector path (querySelector + .call). -const FILL_FN = ` - const el = this; - const tag = el.tagName; - const proto = - el instanceof HTMLTextAreaElement ? HTMLTextAreaElement.prototype - : el instanceof HTMLInputElement ? HTMLInputElement.prototype - : null; - if (proto) { - const desc = Object.getOwnPropertyDescriptor(proto, "value"); - const setter = desc && desc.set; - if (setter) setter.call(el, arg); - else el.value = arg; - el.dispatchEvent(new Event("input", { bubbles: true })); - el.dispatchEvent(new Event("change", { bubbles: true })); - return { status: "ok", tag, kind: el.type || tag.toLowerCase(), value: el.value }; - } - if (el.isContentEditable) { - // Rich-text editors (Slack/Notion/ProseMirror) listen for beforeinput/input. - el.focus(); - let done = false; - try { - const sel = window.getSelection(); - if (sel) { sel.removeAllRanges(); const rng = document.createRange(); rng.selectNodeContents(el); sel.addRange(rng); } - done = document.execCommand("insertText", false, arg); - } catch (e) { done = false; } - if (!done) { - el.textContent = arg; - el.dispatchEvent(new InputEvent("input", { bubbles: true })); - } - return { status: "ok", tag, kind: "contenteditable", value: el.textContent || "" }; - } - return { status: "not_fillable", tag }; -`; +const NOT_FOUND: PageResult = { ok: false, reason: "not_found" }; export const fillTool = defineBrowserTool({ name: "browser_fill", label: "Browser Fill", description: - "Fill a form field (input/textarea/contenteditable). PREFERRED: pass `ref` (e.g. 'e7') from browser_snapshot — survives re-renders. Fallback: a CSS `selector`. Writes through the native value setter and fires bubbling 'input'/'change' events, so React/Vue/Angular controlled components and rich-text editors update correctly — unlike browser_type. Returns the field's value after writing, plus a compact diff of page changes.", + "Fill a form field (input/textarea/contenteditable), or tick a checkbox/radio by passing 'true'/'false'. PREFERRED: pass `ref` (e.g. 'e7') from browser_snapshot — survives re-renders. Fallback: a CSS `selector`. Writes through the native value setter and fires bubbling 'input'/'change' events, so React/Vue/Angular controlled components and rich-text editors update correctly — unlike browser_type. Refuses a disabled field rather than writing a value the page will ignore. Returns the field's value after writing, plus a compact diff of page changes.", promptSnippet: "Fill a form field by ref (preferred) or selector (works with React/Vue controlled inputs)", promptGuidelines: [ "PREFER `ref` from browser_snapshot (the '[eN]' handle) over a guessed CSS selector — refs survive re-renders and don't require you to invent a selector.", "Pass the desired value; no browser_click is needed first.", + "For a checkbox or radio pass 'true' or 'false' — it toggles `checked`, not the submit value. browser_set_checked does the same thing with a boolean.", + "A disabled field is refused: the error says so, rather than reporting a write the page never accepted.", "A 'ref is stale' error means the page changed — re-run browser_snapshot to get fresh refs.", "Use browser_type instead only for keystroke-sensitive widgets (autocomplete, masked/segmented inputs).", "Fill sets the value but does not focus or submit. To submit a tag/autocomplete input, follow with browser_dispatch_key({ ref, key: 'Enter' }) — not browser_press_key, which targets the focused element and may miss this field.", @@ -139,28 +92,50 @@ export const fillTool = defineBrowserTool({ parameters: FillArgs, concurrency: "serialized", async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> { - const r = await runOnElement(client, { ref: args.ref, selector: args.selector, fnBody: FILL_FN, arg: args.value }); + const r = await runOnElement(client, { + ref: args.ref, + selector: args.selector, + fnBody: FILL_BODY, + arg: args.value, + notFound: NOT_FOUND, + }); if (!r.success) return r; - const res = isFillResult(r.data) ? r.data : undefined; + const res = isPageResult(r.data) ? r.data : undefined; const target = args.ref ?? args.selector ?? ""; - if (res === undefined || res.status === "not_found") { + if (res === undefined || res.reason === "not_found") { return err({ kind: "invalid_state", message: `No element matched: ${target}`, details: { ref: args.ref, selector: args.selector }, }); } - if (res.status === "not_fillable") { + if (!res.ok) { + const details = { ref: args.ref, selector: args.selector, ...(res.tag !== undefined ? { tag: res.tag } : {}) }; + if (res.kind === "select") { + return err({ + kind: "invalid_state", + message: `Element <select> is not fillable (not an input, textarea, or contenteditable): ${target}. For <select> use browser_select_option.`, + details, + }); + } return err({ kind: "invalid_state", - message: `Element <${res.tag.toLowerCase()}> is not fillable (not an input, textarea, or contenteditable): ${target}. For <select> use browser_select_option.`, - details: { ref: args.ref, selector: args.selector, tag: res.tag }, + message: `Could not fill ${target}: ${res.reason ?? "unknown reason"}`, + details, }); } + const shown = res.value !== undefined ? JSON.stringify(res.value) : `checked=${res.checked === true}`; const diff = await interactiveDiff(client); return ok({ - text: `Filled ${target} = ${JSON.stringify(res.value)}${diff}`, - details: { ref: args.ref, selector: args.selector, value: args.value, verified: res.value, tag: res.tag, kind: res.kind }, + text: `Filled ${target} = ${shown}${diff}`, + details: { + ref: args.ref, + selector: args.selector, + value: args.value, + verified: res.value ?? res.checked, + ...(res.tag !== undefined ? { tag: res.tag } : {}), + ...(res.kind !== undefined ? { kind: res.kind } : {}), + }, }); }, }); @@ -320,6 +295,7 @@ export type PageResult = { readonly text?: string; readonly checked?: boolean; readonly changed?: boolean; + readonly tag?: string; readonly options?: ReadonlyArray<{ value: string; text: string }>; }; @@ -336,6 +312,7 @@ const isPageResult = (v: unknown): v is PageResult => { if ("text" in v && typeof v.text !== "string") return false; if ("checked" in v && typeof v.checked !== "boolean") return false; if ("changed" in v && typeof v.changed !== "boolean") return false; + if ("tag" in v && typeof v.tag !== "string") return false; if ("options" in v && !(Array.isArray(v.options) && v.options.every(isOption))) return false; return true; }; |
