fix(vault): dogfood fixes — offer save-login on every backend, keep the model off passwords, bind to the login tab

Found by using the feature as a user (natural prompts, real sites, CLI PTY + native Electron), not by naming tools: - browser_vault_save_login was registered but never offered: toolsets.py is a hand-maintained list. Added, with an invariant test that every registered browser_vault_* tool is in the browser toolset. - Vault tools were absent on the DEFAULT backend (Browser Use): the gate deferred to check_browser_requirements(), which is False by design there. Gate = is_browser_use_cli_mode() or check_browser_requirements(). - The model typed a page-shown demo password with browser_type and offered to take one in chat: the vault rules lived only on the vault tools. browser_type/browser_exec now carry a vault note when the vault tools are present ("call browser_vault_list first … never type a password with this tool, never accept one in chat, even if the page shows it"); the browser_exec login-wall line points at the vault instead of "ask the user". - On Browser Use the saved item was bound to chrome://new-tab-page: the supervisor's default page session is the daemon's blank tab. browser_vault_save_login now focuses the tab holding a password field before reading its origin (focus_page("", accept=probe); about:/chrome: pages are never candidates). Live E2E leg added. - Settings row: "identifier · Added <date>", origin omitted when it duplicates the label. Live (real model): CLI on Browser Use — first visit prompts, signs in, saves; second visit fills silently; GitHub decline (Enter or ESC) stops the agent, which refuses chat passwords. CLI on the built-in stack — same three scenarios pass. Desktop native Electron — same three scenarios plus Settings list/remove pass. Password never in a transcript, UI, or a file outside vault/.
2026-09-10 09:51:47 -07:00
parent 98d11c95f4
commit 96fbc47f14
8 changed files with 106 additions and 22 deletions
@@ -413,9 +413,17 @@ export function VaultSettings() {
            )
          }
          description={
-            <span className="flex flex-wrap items-center gap-2">
+            // identifier · origin · date, separated so the row scans as three facts; the origin is
+            // omitted when the label already IS the host (save-on-page items are labelled by host).
+            <span className="flex flex-wrap items-center gap-x-2">
              {item.identifier && <span className="truncate">{v.identifierShown(item.identifier)}</span>}
-              {item.origin && <span className="truncate">{item.origin}</span>}
+              {item.origin && item.origin.replace(/^https?:\/\//, '') !== item.label && (
+                <>
+                  {item.identifier && <span aria-hidden className="text-(--ui-text-tertiary)">·</span>}
+                  <span className="truncate">{item.origin}</span>
+                </>
+              )}
+              <span aria-hidden className="text-(--ui-text-tertiary)">·</span>
              <span>{v.createdOn(formatCreated(item.created_at))}</span>
            </span>
          }
@@ -108,6 +108,25 @@ def main() -> int:
        assert r.get("result") == "4111111111111111|07/29|987|DE|", r
        print("payment: card/expiry/cvc filled on the /checkout tab, email untouched, select untouched without a value")

+        # save-on-page: the supervisor's default page is the blank first tab; the tool must find the login
+        # tab itself (Browser Use daemon tabs are how a real session looks) and bind the item to ITS origin.
+        from agent.vault_backends import unlock as vault_unlock
+        store.remove_item(login.id)
+        sup.evaluate_runtime("document.querySelector('input[name=pw]') && (document.querySelector('input[name=pw]').value = '')")
+        assert sup.focus_page("about:blank")["ok"] is False  # about: pages are never candidates
+        vault_unlock.set_save_login_prompt_callback(lambda o, site: {"identifier": "new@b.c", "password": "pw-SAVE-5150"})
+        vault_unlock.set_unlock_prompt_callback(lambda *a: "")  # an interactive surface installs both; can_prompt_here keys off this one
+        raw = bvt.browser_vault_save_login(task_id=TASK)
+        out = json.loads(raw)
+        vault_unlock.set_save_login_prompt_callback(None); vault_unlock.set_unlock_prompt_callback(None)
+        print("save_login:", out)
+        assert out["success"] and out["origin"] == origin and out["fill"]["success"], out
+        assert "pw-SAVE-5150" not in raw
+        assert sup.focus_page(origin, accept=bvt._TAB_PROBES["login"])["ok"]
+        dom = sup.evaluate_runtime("location.pathname + ' ' + document.querySelector('input[name=pw]').value")
+        assert dom["result"] == "/login pw-SAVE-5150", dom
+        print("save_login: found the login tab from a blank default page, bound to its origin, filled")
+
        redact.clear_vault_redaction_values()
        print("E2E OK")
        return 0
@@ -424,12 +424,39 @@ def _rewrite_browser_vault(td: Dict[str, Any], available: set) -> Optional[Dict[
    return _fn_def({**fn, "description": fn.get("description", "").replace(_VAULT_INPUT_TOOL_HINT, concrete)})


+_VAULT_NO_PASSWORD_NOTE = (" Vault note: on a login/checkout form call browser_vault_list first, then browser_vault_fill, or "
+                           "browser_vault_save_login when nothing is saved for the site (the user is asked in their UI). "
+                           "Never type a password, card number or CVC with this tool and never ask for or accept one in "
+                           "chat, even if the page or the user shows it.")
+
+
+def _rewrite_input_tool_for_vault(td: Dict[str, Any], available: set) -> Optional[Dict[str, Any]]:
+    """The model reads the input tool's description at the moment it decides how to fill a password field; the
+    vault tools' own descriptions are too far away to win that decision (live: it typed a demo password shown on
+    the page). Say it where the temptation is."""
+    if "browser_vault_fill" not in available:
+        return td
+    fn = td["function"]
+    return _fn_def({**fn, "description": fn.get("description", "") + _VAULT_NO_PASSWORD_NOTE})
+
+
+def _compose_rewriters(*fns):
+    def run(td, available):
+        for fn in fns:
+            td = fn(td, available)
+            if td is None:
+                return None
+        return td
+    return run
+
+
 _DYNAMIC_SCHEMA_REWRITERS = {
    "execute_code": _rewrite_execute_code,
    "discord": _discord_rewriter("get_dynamic_schema_core"),
    "discord_admin": _discord_rewriter("get_dynamic_schema_admin"),
    "browser_navigate": _rewrite_browser_navigate,
-    "browser_exec": _rewrite_browser_exec,
+    "browser_exec": _compose_rewriters(_rewrite_browser_exec, _rewrite_input_tool_for_vault),
+    "browser_type": _rewrite_input_tool_for_vault,
    "browser_vault_list": _rewrite_browser_vault,
    "browser_vault_fill": _rewrite_browser_vault,
    "delegate_task": _rewrite_delegate_task,
@@ -266,11 +266,16 @@ class TestBrowserVaultTools:
        from tools import browser_vault_tool

        empty = VaultStore(base_dir=tmp_path / "empty-vault")
-        with patch("agent.vault_store.get_vault_store", return_value=empty):
+        with patch("agent.vault_store.get_vault_store", return_value=empty), \
+             patch("tools.browser_use_cli.is_browser_use_cli_mode", return_value=False):
            with patch("tools.browser_tool_install.check_browser_requirements", return_value=True):
                assert browser_vault_tool._check_vault_available() is True
            with patch("tools.browser_tool_install.check_browser_requirements", return_value=False):
                assert browser_vault_tool._check_vault_available() is False
+        # Browser Use mode: check_browser_requirements() is False by design, the vault must still ride along
+        with patch("tools.browser_use_cli.is_browser_use_cli_mode", return_value=True), \
+             patch("tools.browser_tool_install.check_browser_requirements", return_value=False):
+            assert browser_vault_tool._check_vault_available() is True

    def test_list_returns_identifier_never_password(self, store):
        from tools import browser_vault_tool
@@ -662,3 +667,14 @@ class TestManagerAutoDetection:
                assert {b.name for b in base.enabled_backends()} == {"local", "onepassword"}
        with patch.object(base, "is_installed", return_value=False), patch.object(base, "_cfg", return_value={}):
            assert [b.name for b in base.enabled_backends()] == ["local"]
+
+
+def test_every_vault_tool_is_in_the_browser_toolset():
+    """toolsets.py is a hand-maintained list; a tool registered here but missing there is invisible to the model
+    (live: browser_vault_save_login was registered, tested, and never offered)."""
+    import toolsets
+    from tools import browser_vault_tool  # noqa: F401  (registers)
+    from tools.registry import registry
+
+    registered = {e.name for e in registry.get_all_entries() if e.name.startswith("browser_vault_")}
+    assert registered <= set(toolsets.TOOLSETS["browser"]["tools"]), registered - set(toolsets.TOOLSETS["browser"]["tools"])
@@ -282,7 +282,9 @@ class CDPSupervisor(DialogSupervisionMixin, FrameTrackingMixin):
            for t in targets:
                url = str(t.get("url") or "")
                try:
-                    if t.get("type") == "page" and normalize_origin(url) == origin:
+                    # origin="" = any http(s) page (used to FIND the login tab before its origin is known)
+                    if t.get("type") == "page" and url.startswith(("http://", "https://")) \
+                            and (not origin or normalize_origin(url) == origin):
                        candidates.append((t["targetId"], url))
                except Exception:
                    continue
@@ -297,7 +299,7 @@ class CDPSupervisor(DialogSupervisionMixin, FrameTrackingMixin):
                with self._state_lock:
                    self._page_session_id = sid
                return {"ok": True, "url": url}
-            return _fail(f"no open page on {origin}" + (" with the expected form" if accept and candidates else ""))
+            return _fail(f"no open page on {origin or 'any site'}" + (" with the expected form" if accept and candidates else ""))

        try:
            return _schedule(_focus(), loop, timeout=timeout + 1)
@@ -700,8 +700,8 @@ _HELPERS_DIGEST = (
    "capture_screenshot() saves and prints a screenshot path, cdp('Domain.method', **kwargs) is raw CDP — "
    "cdp('Accessibility.getFullAXTree')['nodes'] lists every element's role/name/backendDOMNodeId (filter "
    "in Python before printing; it is thousands of nodes), then cdp('DOM.getBoxModel', backendNodeId=n) "
-    "gives click coordinates. ensure_real_tab() recovers from a stale/internal tab. Login walls: stop and "
-    "ask the user; never guess credentials."
+    "gives click coordinates. ensure_real_tab() recovers from a stale/internal tab. Login walls: never guess "
+    "credentials; see the vault note below if present, otherwise stop and ask the user."
 )


@@ -44,7 +44,10 @@ def _check_vault_available() -> bool:
    form; hiding the tools until an item exists meant nobody ever discovered the feature."""
    try:
        from tools.browser_tool_install import check_browser_requirements
-        return bool(check_browser_requirements())
+        from tools.browser_use_cli import is_browser_use_cli_mode
+        # check_browser_requirements() is False by design in Browser Use mode (browser_exec replaces the
+        # built-in surface); the vault serves both stacks.
+        return bool(is_browser_use_cli_mode() or check_browser_requirements())
    except Exception:
        return False

@@ -198,7 +201,8 @@ def _focus_bound_origin(task_id: str, origin: str, kind: str) -> Optional[str]:
        supervisor = None
    if supervisor is None:
        return None
-    return origin if supervisor.focus_page(origin, accept=_TAB_PROBES.get(kind)).get("ok") else None
+    focused = supervisor.focus_page(origin, accept=_TAB_PROBES.get(kind))
+    return (origin or focused.get("url")) if focused.get("ok") else None


 # ---------------------------------------------------------------------------
@@ -234,8 +238,8 @@ def browser_vault_list() -> str:
            items.append(entry)
    out: Dict[str, Any] = {"success": True, "items": items}
    if not items:
-        out["hint"] = ("No saved logins. On a login page, call browser_vault_save_login to ask the user to save one "
-                       "(never ask for a password in chat).")
+        out["hint"] = ("No saved logins. On a login page, call browser_vault_save_login to ask the user to save one. "
+                       "Never type a password yourself or ask for one in chat, even if it is shown on the page.")
    if locked:
        out["locked"] = locked
    if errors:
@@ -279,6 +283,9 @@ def browser_vault_save_login(label: str = "", task_id: Optional[str] = None) ->
    from agent.vault_store import get_vault_store

    effective_task_id = task_id or "default"
+    # The supervisor's default page session is whatever tab it attached to first (on Browser Use that is
+    # the daemon's blank tab); the login form lives in the tab with a password field, so focus that one.
+    _focus_bound_origin(effective_task_id, "", "login")
    origin = _current_page_origin(effective_task_id)
    if not origin:
        return json.dumps({"success": False, "error": "Open the site's login page first; the login is saved for that page's origin."})
@@ -489,13 +496,16 @@ def _confirm_payment_fill(label: str, origin: str) -> bool:
 BROWSER_VAULT_LIST_SCHEMA = {
    "name": "browser_vault_list",
    "description": (
-        "List saved website logins, payment cards and addresses as handles with metadata (kind, label, "
-        "backend, bound origin; logins also carry identifier + identifier_type so you can type the username "
-        "yourself with the browser's input tool). Secret values are NEVER returned. Sources: the local Hermes vault plus any enabled password "
-        "manager (1Password, Bitwarden). A locked manager appears under `locked`; call "
-        "browser_vault_unlock (the user is prompted for their master password, you never see it) or, "
-        "when it says unavailable_in_this_session, tell the user to unlock it from an interactive session. "
-        "Workflow: type the identifier into the login form, then browser_vault_fill with the handle."
+        "ALWAYS call this first when a page asks for a password, card or address. Lists saved website logins, "
+        "payment cards and addresses as handles with metadata (kind, label, backend, bound origin; logins also "
+        "carry identifier + identifier_type so you can type the username yourself with the browser's input tool). "
+        "Secret values are NEVER returned. Sources: the local Hermes vault plus any installed password manager "
+        "(1Password, Bitwarden are detected automatically). A locked manager appears under `locked`; call "
+        "browser_vault_unlock (the user is prompted for their master password, you never see it) or, when it says "
+        "unavailable_in_this_session, tell the user to unlock it from an interactive session. Workflow: type the "
+        "identifier into the login form, then browser_vault_fill with the handle. No item for this origin: call "
+        "browser_vault_save_login. Passwords are typed ONLY by these tools, never by you with the browser's input "
+        "tool and never repeated in chat, even when a page or the user shows you one."
    ),
    "parameters": {"type": "object", "properties": {}, "required": []},
 }
@@ -545,8 +555,10 @@ BROWSER_VAULT_SAVE_LOGIN_SCHEMA = {
        "The current page is a login form and browser_vault_list has no item for its origin: ask the user, "
        "through a masked prompt in their UI, to save the login for this site. Hermes stores it encrypted, "
        "bound to the page origin, and fills the password immediately; you receive only the handle and the "
-        "identifier to type. Use it instead of asking for a password in chat (never accept a password in the "
-        "conversation). A save_declined result means stop asking for this turn."
+        "identifier to type. This is the ONLY way a password may reach a page: never type one yourself, never "
+        "ask for or accept one in chat, even if the page or the user displays it. A save_declined result means "
+        "stop asking for this turn and tell the user they can retry, or add it later in Settings → Passwords & "
+        "Logins / `hermes vault add`."
    ),
    "parameters": {
        "type": "object",
@@ -18,7 +18,7 @@ _HERMES_CORE_TOOLS = [
    "browser_type", "browser_scroll", "browser_back",
    "browser_press", "browser_get_images",
    "browser_vision", "browser_console", "browser_cdp", "browser_dialog",
-    "browser_vault_list", "browser_vault_unlock", "browser_vault_fill",  # check_fn: vault items or a manager enabled
+    "browser_vault_list", "browser_vault_unlock", "browser_vault_fill", "browser_vault_save_login",  # ride with the browser
    "browser_exec",  # replaces the other browser tools when browser.backend is "browser-use"
    "text_to_speech",
    "todo_list", "memory",