From cfa1892e17e3696da004c71a4942d5b12186b12c Mon Sep 17 00:00:00 2001
From: amankumarsingh77 <amankumarsingh7702@gmail.com>
Date: Tue, 28 Jul 2026 19:55:53 +0530
Subject: fix(domains): one fill engine, so browser_fill stops lying about
 checkboxes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 src/domains/fill-engine.ts |  95 +++++++++++++++++++++++++++++++++++++
 src/domains/form-batch.ts  |  55 +---------------------
 src/domains/form.ts        | 115 ++++++++++++++++++---------------------------
 3 files changed, 143 insertions(+), 122 deletions(-)
 create mode 100644 src/domains/fill-engine.ts

(limited to 'src')

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;
 };
-- 
cgit v1.2.3